Rule for bridge yaml dt binding maintainers?

Hello,

I got confused while doing the txt -> yaml conversion at [1] and it's still not clear to me who should be added in the "maintainers" field. Clearly not the maintainers as returned by get_maintainer.pl. :)

Rob mentioned that "owners" should be manintainers but I also have trouble picking the persons who should be owners / yaml maintainers.

Looking at the completed bridge conversions in the latest linux-next, I couldn't find a rule and the majority of bindings are still txt:

$ find ./devicetree/bindings/display/bridge/ -name *txt | wc -l
23
$ find ./devicetree/bindings/display/bridge/ -name *yaml | wc -l
5

So my questions are:
1. Is there a general rule for assigning yaml file owners/maintainers?

2. Is this vagueness specific to the bridge dt bindings only?

3. Who should step up and maintain these bindings? Original/new authors, SoC, bridge, DRM maintainers etc.?

It would be useful to have a rule to make it easier to do these conversions. We (Collabora) are considering doing the conversion work.

Hi Adrian

Quoted 20 lines · Adrian Ratiu

Hello,

I got confused while doing the txt -> yaml conversion at [1] and it's still not clear to me who should be added in the "maintainers" field. Clearly not the maintainers as returned by get_maintainer.pl. :)

Rob mentioned that "owners" should be manintainers but I also have trouble picking the persons who should be owners / yaml maintainers.

Looking at the completed bridge conversions in the latest linux-next, I couldn't find a rule and the majority of bindings are still txt:

$ find ./devicetree/bindings/display/bridge/ -name *txt | wc -l
23
$ find ./devicetree/bindings/display/bridge/ -name *yaml | wc -l
5

So my questions are:
1. Is there a general rule for assigning yaml file owners/maintainers?

Quoted 7 lines

2. Is this vagueness specific to the bridge dt bindings only?

3. Who should step up and maintain these bindings? Original/new authors, SoC, bridge, DRM maintainers etc.?

It would be useful to have a rule to make it easier to do these conversions. We (Collabora) are considering doing the conversion work.

For the panel conversion I did recently it was simple:
1) If listed in MAINTAINERS - use this info
2) Otherwise use the person(s) that authored the original .txt file. Using git log --follow foo.txt
3) In a few cases I may have decided otherwise, but the above covers the majority.

I would also be great if you or someone else could:
- teach get_maintainers about .yaml file listed maintainers

- teach checkpatch that it is OK to convert .txt to .yaml

- teach checkpatch about some simple yaml validation (maybe)

I am looking forward to the day we have more .yaml files than .txt files in Documentation/devicetree/binding/*

Quoted 31 lines · Sam Ravnborg

Hi Adrian

Quoted 20 lines · Adrian Ratiu

Hello,

I got confused while doing the txt -> yaml conversion at [1] and it's still not clear to me who should be added in the "maintainers" field. Clearly not the maintainers as returned by get_maintainer.pl. :)

Rob mentioned that "owners" should be manintainers but I also have trouble picking the persons who should be owners / yaml maintainers.

Looking at the completed bridge conversions in the latest linux-next, I couldn't find a rule and the majority of bindings are still txt:

$ find ./devicetree/bindings/display/bridge/ -name *txt | wc -l
23
$ find ./devicetree/bindings/display/bridge/ -name *yaml | wc -l
5

So my questions are:
1. Is there a general rule for assigning yaml file owners/maintainers?

Quoted 7 lines

2. Is this vagueness specific to the bridge dt bindings only?

3. Who should step up and maintain these bindings? Original/new authors, SoC, bridge, DRM maintainers etc.?

It would be useful to have a rule to make it easier to do these conversions. We (Collabora) are considering doing the conversion work.

Quoted 6 lines

For the panel conversion I did recently it was simple:
1) If listed in MAINTAINERS - use this info
2) Otherwise use the person(s) that authored the original .txt file. Using git log --follow foo.txt
3) In a few cases I may have decided otherwise, but the above covers the majority.

Yes.

Quoted 2 lines

I would also be great if you or someone else could:
- teach get_maintainers about .yaml file listed maintainers

It already does to some extent. IIRC, there's a mode to extract email addresses from files.

I was hoping that the MAINTAINERS file split happens sometime and we can just generate a MAINTAINERS file for bindings.

Quoted 1 line

- teach checkpatch that it is OK to convert .txt to .yaml

Yeah, I should fix my bug.

Quoted 1 line

- teach checkpatch about some simple yaml validation (maybe)

I don't see checkpatch being able to check much of what comes up in review. Maybe indentation.

Quoted 6 lines

Quoted 3 lines

I would also be great if you or someone else could:
- teach get_maintainers about .yaml file listed maintainers

It already does to some extent. IIRC, there's a mode to extract email addresses from files.

--file-emails

Quoted 2 lines

I was hoping that the MAINTAINERS file split happens sometime and we can just generate a MAINTAINERS file for bindings.

I don't see the value really.

Quoted 1 line

Quoted 1 line

- teach checkpatch that it is OK to convert .txt to .yaml

I suppose that get_maintainer _could_ enable --file-emails for .yaml files.

something like this (more comments below too)

---
 scripts/get_maintainer.pl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

scripts/get_maintainer.pl +1 -1

index 6cbcd1..9d947a0 100755
@@ -527,7 +527,7 @@ foreach my $file (@ARGV) {
 	$file =~ s/^\Q${cur_path}\E//;	#strip any absolute path
 	$file =~ s/^\Q${lk_path}\E//;	#or the path to the lk tree
 	push(@files, $file);
-	if ($file ne "MAINTAINERS" && -f $file && ($keywords || $file_emails)) {
+	if ($file ne "MAINTAINERS" && -f $file && ($keywords || $file_emails || $file =~ /\.yaml$/)) {
 	    open(my $f, '<', $file)
 		or die "$P: Can't open $file: $!\n";
 	    my $text = do { local($/) ; <$f> };

Quoted 2 lines

I don't see checkpatch being able to check much of what comes up in review. Maybe indentation.

Likely better done with another external tool.

Could be added to checkpatch as an external
call like spdxcheck.py

.yaml files can contain maintainer/author addresses and it seems unlikely or unnecessary that individual MAINTAINER file section entries for each .yaml file will be created.

So dd the email addresses found in .yaml files to the default

get_maintainer output.

The email addresses are marked with "(in file)" when using the "--roles" or "--rolestats" options.

Miscellanea:

o Change $file_emails to $email_file_emails to avoid visual naming conflicts with @file_emails

Signed-off-by Joe Perches <[email protected]>

---
 scripts/get_maintainer.pl | 44 +++++++++++++++++++++++++++++---------------
 1 file changed, 29 insertions(+), 15 deletions(-)

scripts/get_maintainer.pl +29 -15

index 6cbcd1..6d973f 100755
@@ -57,7 +57,7 @@ my $status = 0;
 my $letters = "";
 my $keywords = 1;
 my $sections = 0;
-my $file_emails = 0;
+my $email_file_emails = 0;
 my $from_filename = 0;
 my $pattern_depth = 0;
 my $self_test = undef;

@@ -69,6 +69,12 @@ my $vcs_used = 0;
 
 my $exit = 0;
 
+my @files = ();
+my @fixes = ();			# If a patch description includes Fixes: lines
+my @range = ();
+my @keyword_tvi = ();
+my @file_emails = ();
+
 my %commit_author_hash;
 my %commit_signer_hash;
 

@@ -266,7 +272,7 @@ if (!GetOptions(
 		'pattern-depth=i' => \$pattern_depth,
 		'k|keywords!' => \$keywords,
 		'sections!' => \$sections,
-		'fe|file-emails!' => \$file_emails,
+		'fe|file-emails!' => \$email_file_emails,
 		'f|file' => \$from_filename,
 		'find-maintainer-files' => \$find_maintainer_files,
 		'mpath|maintainer-path=s' => \$maintainer_path,

@@ -424,6 +430,22 @@ sub read_all_maintainer_files {
     }
 }
 
+sub maintainers_in_file {
+    my ($file) = @_;
+
+    return if ($file =~ m@\bMAINTAINERS$@);
+
+    if (-f $file && ($email_file_emails || $file =~ /\.yaml$/)) {
+	open(my $f, '<', $file)
+	    or die "$P: Can't open $file: $!\n";
+	my $text = do { local($/) ; <$f> };
+	close($f);
+
+	my @poss_addr = $text =~ m$[A-Za-zÀ-ÿ\"\' \,\.\+-]*\s*[\,]*\s*[\(\<\{]{0,1}[A-Za-z0-9_\.\+-]+\@[A-Za-z0-9\.-]+\.[A-Za-z0-9]+[\)\>\}]{0,1}$g;
+	push(@file_emails, clean_file_emails(@poss_addr));
+    }
+}
+
 #
 # Read mail address map
 #

@@ -504,12 +526,6 @@ sub read_mailmap {
 
 ## use the filenames on the command line or find the filenames in the patchfiles
 
-my @files = ();
-my @fixes = ();			# If a patch description includes Fixes: lines
-my @range = ();
-my @keyword_tvi = ();
-my @file_emails = ();
-
 if (!@ARGV) {
     push(@ARGV, "&STDIN");
 }

@@ -527,7 +543,7 @@ foreach my $file (@ARGV) {
 	$file =~ s/^\Q${cur_path}\E//;	#strip any absolute path
 	$file =~ s/^\Q${lk_path}\E//;	#or the path to the lk tree
 	push(@files, $file);
-	if ($file ne "MAINTAINERS" && -f $file && ($keywords || $file_emails)) {
+	if ($file ne "MAINTAINERS" && -f $file && $keywords) {
 	    open(my $f, '<', $file)
 		or die "$P: Can't open $file: $!\n";
 	    my $text = do { local($/) ; <$f> };

@@ -539,10 +555,6 @@ foreach my $file (@ARGV) {
 		    }
 		}
 	    }
-	    if ($file_emails) {
-		my @poss_addr = $text =~ m$[A-Za-zÀ-ÿ\"\' \,\.\+-]*\s*[\,]*\s*[\(\<\{]{0,1}[A-Za-z0-9_\.\+-]+\@[A-Za-z0-9\.-]+\.[A-Za-z0-9]+[\)\>\}]{0,1}$g;
-		push(@file_emails, clean_file_emails(@poss_addr));
-	    }
 	}
     } else {
 	my $file_cnt = @files;

@@ -923,6 +935,8 @@ sub get_maintainers {
 		print("\n");
 	    }
 	}
+
+	maintainers_in_file($file);
     }
 
     if ($keywords) {

@@ -1835,7 +1849,7 @@ tm toggle maintainers
 tg toggle git entries
 tl toggle open list entries
 ts toggle subscriber list entries
-f  emails in file       [$file_emails]
+f  emails in file       [$email_file_emails]
 k  keywords in file     [$keywords]
 r  remove duplicates    [$email_remove_duplicates]
 p# pattern match depth  [$pattern_depth]

@@ -1960,7 +1974,7 @@ EOT
 		bool_invert(\$email_git_all_signature_types);
 		$rerun = 1;
 	    } elsif ($sel eq "f") {
-		bool_invert(\$file_emails);
+		bool_invert(\$email_file_emails);
 		$rerun = 1;
 	    } elsif ($sel eq "r") {
 		bool_invert(\$email_remove_duplicates);

Hi Joe.

Quoted 5 lines

.yaml files can contain maintainer/author addresses and it seems unlikely or unnecessary that individual MAINTAINER file section entries for each .yaml file will be created.

So dd the email addresses found in .yaml files to the default

     ^
     add

Signed-off-by Joe Perches <[email protected]>

Acked-by Sam Ravnborg <[email protected]>

Tested-by Sam Ravnborg <[email protected]>

The patch did not apply on top of -rc3, but it was trivial to fix. Tested and works like a charm.
Thanks for doing this!

Quoted 1 line · Sam Ravnborg

Hi Joe.

Hi Sam.

Quoted 8 lines · Joe Perches

Quoted 5 lines

.yaml files can contain maintainer/author addresses and it seems unlikely or unnecessary that individual MAINTAINER file section entries for each .yaml file will be created.

So dd the email addresses found in .yaml files to the default

     ^
     add

Andrew, can you add the a to this please?

Quoted 4 lines

Signed-off-by Joe Perches <[email protected]>

Acked-by Sam Ravnborg <[email protected]>

Tested-by Sam Ravnborg <[email protected]>

Quoted 3 lines

The patch did not apply on top of -rc3, but it was trivial to fix. Tested and works like a charm.
Thanks for doing this!

As most of my patches, it was done using -next

Quoted 20 lines · On Sun, 26 Apr 2020 23:33:02 -0700 Joe Perches <[email protected]> wrote:

Quoted 1 line · Sam Ravnborg

Hi Joe.

Hi Sam.

Quoted 8 lines · Joe Perches

Quoted 5 lines

.yaml files can contain maintainer/author addresses and it seems unlikely or unnecessary that individual MAINTAINER file section entries for each .yaml file will be created.

So dd the email addresses found in .yaml files to the default

     ^
     add

Andrew, can you add the a to this please?

Quoted 4 lines

Signed-off-by Joe Perches <[email protected]>

Acked-by Sam Ravnborg <[email protected]>

Tested-by Sam Ravnborg <[email protected]>

Quoted 6 lines

Quoted 3 lines

The patch did not apply on top of -rc3, but it was trivial to fix. Tested and works like a charm.
Thanks for doing this!

As most of my patches, it was done using -next

The patch assumes that we have

scripts/get_maintainer.pl +0 -4

-	    if ($file_emails) {
-		my @poss_addr = $text =~ m$[A-Za-z_-_\"\' \,\.\+-]*\s*[\,]*\s*[\(\<\{]{0,1}[A-Za-z0-9_\.\+-]+\@[A-Za-z0-9\.-]+\.[A-Za-z0-9]+[\)\>\}]{0,1}$g;
-		push(@file_emails, clean_file_emails(@poss_addr));
-	    }

but today's next has

	    if ($file_emails) {
		my @poss_addr = $text =~ m$[A-Za-zÀ-ÿ\"\' \,\.\+-]*\s*[\,]*\s*[\(\<\{]{0,1}[A-Za-z0-9_\.\+-]+\@[A-Za-z0-9\.-]+\.[A-Za-z0-9]+[\)\>\}]{0,1}$g;
		push(@file_emails, clean_file_emails(@poss_addr));
	    }

so what do do here?

- my @poss_addr = $text =~ m$[A-Za-zÀ-ÿ