checkpatch: Warn if DT bindings are not in schema format

PATCH In review Rob Herring 2 participants 9 messages Sep 13, 2019 – Oct 11, 2019 Checkpatch

Share & tools

Rob Herring

author

2019-09-13 21:13 UTC #0 lore raw

PATCH 1/1 checkpatch: Warn if DT bindings are not in schema format

DT bindings are moving to using a json-schema based schema format instead of freeform text. Add a checkpatch.pl check to encourage using the schema for new bindings. It's not yet a requirement, but is progressively being required by some maintainers.

Cc Andy Whitcroft <[email protected]>

Cc Joe Perches <[email protected]>

Signed-off-by Rob Herring <[email protected]>

---
 scripts/checkpatch.pl | 8 ++++++++
 1 file changed, 8 insertions(+)

scripts/checkpatch.pl +8 -0

index 93a7edfe0f05..1cbd85f16e32 100755
@@ -2822,6 +2822,14 @@ sub process {
 			     "added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
 		}
 
+# Check for adding new DT bindings not in schema format
+		if (!$in_commit_log &&
+		    ($line =~ /^new file mode\s*\d+\s*$/) &&
+		    ($realfile =~ m@^Documentation/devicetree/bindings/.*\.txt$@)) {
+			WARN("DT_SCHEMA_BINDING_PATCH",
+			     "DT bindings should be in DT schema format. See: Documentation/devicetree/writing-schema.rst\n");
+		}
+
 # Check for wrappage within a valid hunk of the file
 		if ($realcnt != 0 && $line !~ m{^(?:\+|-| |\\ No newline|$)}) {
 			ERROR("CORRUPTED_PATCH",

Signature · 1 line

2.20.1

Joe Perches

maintainer

2019-09-13 21:48 UTC #1 lore raw

Quoted 4 lines · Rob Herring

DT bindings are moving to using a json-schema based schema format instead of freeform text. Add a checkpatch.pl check to encourage using the schema for new bindings. It's not yet a requirement, but is progressively being required by some maintainers.

[]

scripts/checkpatch.pl

Quoted 1 line

scripts/checkpatch.pl +0 -0

[]

scripts/checkpatch.pl sub process { · line 2832

Quoted 12 lines

scripts/checkpatch.pl +8 -0

@@ -2822,6 +2822,14 @@ sub process {
 			     "added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
 		}
 
+# Check for adding new DT bindings not in schema format
+		if (!$in_commit_log &&
+		    ($line =~ /^new file mode\s*\d+\s*$/) &&
+		    ($realfile =~ m@^Documentation/devicetree/bindings/.*\.txt$@)) {
+			WARN("DT_SCHEMA_BINDING_PATCH",
+			     "DT bindings should be in DT schema format. See: Documentation/devicetree/writing-schema.rst\n");
+		}
+

As this already seems to be git dependent, perhaps it's easier to read with a single line test like:

		if ($line =~ m{^\s*create mode\s*\d+\s*Documentation/devicetree/bindings/.*\.txt$}) {
			etc...
		}

There are ~3500 existing .txt files:

$ git ls-files -- 'Documentation/devicetree/bindings/*.txt' | wc -l
3476

Are these going to be converted somehow?

What about files like these? (not .txt)

Documentation/devicetree/bindings/timer/st,stih407-lpc Documentation/devicetree/bindings/nds32/andestech-boards Documentation/devicetree/bindings/media/nokia,n900-ir Documentation/devicetree/bindings/interrupt-controller/ti,omap4-wugen-mpu Documentation/devicetree/bindings/arm/cpu-enable-method/nuvoton,npcm750-smp Documentation/devicetree/bindings/arm/cpu-enable-method/marvell,berlin-smp Documentation/devicetree/bindings/arm/cpu-enable-method/al,alpine-smp Documentation/devicetree/bindings/arm/arm-boards

Rob Herring

author

2019-09-16 18:21 UTC #2 lore raw

Quoted 25 lines · Joe Perches

Quoted 4 lines · Rob Herring
DT bindings are moving to using a json-schema based schema format instead of freeform text. Add a checkpatch.pl check to encourage using the schema for new bindings. It's not yet a requirement, but is progressively being required by some maintainers.
[]
scripts/checkpatch.pl
Quoted 1 line
scripts/checkpatch.pl +0 -0
[]
scripts/checkpatch.pl sub process { · line 2832
Quoted 12 lines
scripts/checkpatch.pl +8 -0
@@ -2822,6 +2822,14 @@ sub process {
                           "added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
              }
 
+# Check for adding new DT bindings not in schema format
+             if (!$in_commit_log &&
+                 ($line =~ /^new file mode\s*\d+\s*$/) &&
+                 ($realfile =~ m@^Documentation/devicetree/bindings/.*\.txt$@)) {
+                     WARN("DT_SCHEMA_BINDING_PATCH",
+                          "DT bindings should be in DT schema format. See: Documentation/devicetree/writing-schema.rst\n");
+             }
+
As this already seems to be git dependent, perhaps it's easier to read with a single line test like:

Quoted 3 lines

                if ($line =~ m{^\s*create mode\s*\d+\s*Documentation/devicetree/bindings/.*\.txt$}) {
                        etc...
                }

Okay. I wasn't too concerned about non-git diffs as I rarely see those anymore.

Quoted 7 lines

There are ~3500 existing .txt files:
$ git ls-files -- 'Documentation/devicetree/bindings/*.txt' | wc -l
3476
Are these going to be converted somehow?

Patches welcome! We're working on it.

Quoted 11 lines

What about files like these? (not .txt)
Documentation/devicetree/bindings/timer/st,stih407-lpc Documentation/devicetree/bindings/nds32/andestech-boards Documentation/devicetree/bindings/media/nokia,n900-ir Documentation/devicetree/bindings/interrupt-controller/ti,omap4-wugen-mpu Documentation/devicetree/bindings/arm/cpu-enable-method/nuvoton,npcm750-smp Documentation/devicetree/bindings/arm/cpu-enable-method/marvell,berlin-smp Documentation/devicetree/bindings/arm/cpu-enable-method/al,alpine-smp Documentation/devicetree/bindings/arm/arm-boards

What about them? This check is only for new files and no one runs checkpatch.pl on binding txt files. If someone submits something without an extension, then I'll catch that in review. I'm not too worried about 8 out of 3500 cases.

Rob

Rob Herring

author

2019-09-27 14:02 UTC #3 lore raw

In reply to Joe Perches

Quoted 13 lines · Joe Perches

Quoted 4 lines · Rob Herring
DT bindings are moving to using a json-schema based schema format instead of freeform text. Add a checkpatch.pl check to encourage using the schema for new bindings. It's not yet a requirement, but is progressively being required by some maintainers.
[]
scripts/checkpatch.pl
Quoted 1 line
scripts/checkpatch.pl +0 -0
[]
Quoted 4 lines
scripts/checkpatch.pl +0 -0
@@ -2822,6 +2822,14 @@ sub process {
                           "added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
              }
 

Quoted 10 lines

scripts/checkpatch.pl sub process { · line 2832

Quoted 8 lines

scripts/checkpatch.pl +8 -0

+# Check for adding new DT bindings not in schema format
+             if (!$in_commit_log &&
+                 ($line =~ /^new file mode\s*\d+\s*$/) &&
+                 ($realfile =~ m@^Documentation/devicetree/bindings/.*\.txt$@)) {
+                     WARN("DT_SCHEMA_BINDING_PATCH",
+                          "DT bindings should be in DT schema format. See: Documentation/devicetree/writing-schema.rst\n");
+             }
+

As this already seems to be git dependent, perhaps

It's quite rare to see a non git generated diff these days.

Quoted 5 lines

it's easier to read with a single line test like:

                if ($line =~ m{^\s*create mode\s*\d+\s*Documentation/devicetree/bindings/.*\.txt$}) {
                        etc...
                }

I frequently do 'git show $commit | scripts/checkpatch.pl' and this doesn't work with that. I really should have a '--pretty=email' in there, but I just ignore the commit msg warnings. In any case, that still doesn't help because there's no diffstat. There's probably some way to turn that on or just use git-format-patch, but really we want this to work with any git diff.

Rob

Joe Perches

maintainer

2019-09-27 14:29 UTC #4 lore raw

Quoted 32 lines · Rob Herring

Quoted 12 lines · Joe Perches
Quoted 4 lines · Rob Herring
DT bindings are moving to using a json-schema based schema format instead of freeform text. Add a checkpatch.pl check to encourage using the schema for new bindings. It's not yet a requirement, but is progressively being required by some maintainers.
[]
scripts/checkpatch.pl
Quoted 1 line
scripts/checkpatch.pl +0 -0
[]
Quoted 4 lines
scripts/checkpatch.pl +0 -0
@@ -2822,6 +2822,14 @@ sub process {
                           "added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
              }
 
Quoted 10 lines
scripts/checkpatch.pl sub process { · line 2832
Quoted 8 lines
scripts/checkpatch.pl +8 -0
+# Check for adding new DT bindings not in schema format
+             if (!$in_commit_log &&
+                 ($line =~ /^new file mode\s*\d+\s*$/) &&
+                 ($realfile =~ m@^Documentation/devicetree/bindings/.*\.txt$@)) {
+                     WARN("DT_SCHEMA_BINDING_PATCH",
+                          "DT bindings should be in DT schema format. See: Documentation/devicetree/writing-schema.rst\n");
+             }
+
As this already seems to be git dependent, perhaps
It's quite rare to see a non git generated diff these days.
Quoted 5 lines
it's easier to read with a single line test like:
                if ($line =~ m{^\s*create mode\s*\d+\s*Documentation/devicetree/bindings/.*\.txt$}) {
                        etc...
                }

Quoted 6 lines

I frequently do 'git show $commit | scripts/checkpatch.pl' and this doesn't work with that. I really should have a '--pretty=email' in there, but I just ignore the commit msg warnings. In any case, that still doesn't help because there's no diffstat. There's probably some way to turn that on or just use git-format-patch, but really we want this to work with any git diff.

I don't understand your argument against what I proposed at all.

and btw:

$ git format-patch -1 --stdout <commit> | ./scripts/checkpatch.pl

Rob Herring

author

2019-09-27 15:39 UTC #5 lore raw

Quoted 34 lines · Joe Perches

Quoted 32 lines · Rob Herring
Quoted 12 lines · Joe Perches
Quoted 4 lines · Rob Herring
DT bindings are moving to using a json-schema based schema format instead of freeform text. Add a checkpatch.pl check to encourage using the schema for new bindings. It's not yet a requirement, but is progressively being required by some maintainers.
[]
scripts/checkpatch.pl
Quoted 1 line
scripts/checkpatch.pl +0 -0
[]
Quoted 4 lines
scripts/checkpatch.pl +0 -0
@@ -2822,6 +2822,14 @@ sub process {
                           "added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
              }
 
Quoted 10 lines
scripts/checkpatch.pl sub process { · line 2832
Quoted 8 lines
scripts/checkpatch.pl +8 -0
+# Check for adding new DT bindings not in schema format
+             if (!$in_commit_log &&
+                 ($line =~ /^new file mode\s*\d+\s*$/) &&
+                 ($realfile =~ m@^Documentation/devicetree/bindings/.*\.txt$@)) {
+                     WARN("DT_SCHEMA_BINDING_PATCH",
+                          "DT bindings should be in DT schema format. See: Documentation/devicetree/writing-schema.rst\n");
+             }
+
As this already seems to be git dependent, perhaps
It's quite rare to see a non git generated diff these days.
Quoted 5 lines
it's easier to read with a single line test like:
                if ($line =~ m{^\s*create mode\s*\d+\s*Documentation/devicetree/bindings/.*\.txt$}) {
                        etc...
                }

Quoted 8 lines

Quoted 6 lines
I frequently do 'git show $commit | scripts/checkpatch.pl' and this doesn't work with that. I really should have a '--pretty=email' in there, but I just ignore the commit msg warnings. In any case, that still doesn't help because there's no diffstat. There's probably some way to turn that on or just use git-format-patch, but really we want this to work with any git diff.
I don't understand your argument against what I proposed at all.

It is dependent on the commit message rather than the diff itself. I want it to work with or without a diffstat.

Quoted 3 lines

and btw:

$ git format-patch -1 --stdout <commit> | ./scripts/checkpatch.pl

Yes, I stated this was possible. My concern is there are lots of ways to generate a diff in git. My way works for *all* of them. Yours doesn't.

Rob

Rob Herring

author

2019-10-11 17:56 UTC #6 lore raw

Quoted 48 lines · Rob Herring

Quoted 34 lines · Joe Perches
Quoted 32 lines · Rob Herring
Quoted 12 lines · Joe Perches
Quoted 4 lines · Rob Herring
DT bindings are moving to using a json-schema based schema format instead of freeform text. Add a checkpatch.pl check to encourage using the schema for new bindings. It's not yet a requirement, but is progressively being required by some maintainers.
[]
scripts/checkpatch.pl
Quoted 1 line
scripts/checkpatch.pl +0 -0
[]
Quoted 4 lines
scripts/checkpatch.pl +0 -0
@@ -2822,6 +2822,14 @@ sub process {
                           "added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
              }
 
Quoted 10 lines
scripts/checkpatch.pl sub process { · line 2832
Quoted 8 lines
scripts/checkpatch.pl +8 -0
+# Check for adding new DT bindings not in schema format
+             if (!$in_commit_log &&
+                 ($line =~ /^new file mode\s*\d+\s*$/) &&
+                 ($realfile =~ m@^Documentation/devicetree/bindings/.*\.txt$@)) {
+                     WARN("DT_SCHEMA_BINDING_PATCH",
+                          "DT bindings should be in DT schema format. See: Documentation/devicetree/writing-schema.rst\n");
+             }
+
As this already seems to be git dependent, perhaps
It's quite rare to see a non git generated diff these days.
Quoted 5 lines
it's easier to read with a single line test like:
                if ($line =~ m{^\s*create mode\s*\d+\s*Documentation/devicetree/bindings/.*\.txt$}) {
                        etc...
                }
Quoted 8 lines
Quoted 6 lines
I frequently do 'git show $commit | scripts/checkpatch.pl' and this doesn't work with that. I really should have a '--pretty=email' in there, but I just ignore the commit msg warnings. In any case, that still doesn't help because there's no diffstat. There's probably some way to turn that on or just use git-format-patch, but really we want this to work with any git diff.
I don't understand your argument against what I proposed at all.
It is dependent on the commit message rather than the diff itself. I want it to work with or without a diffstat.

Quoted 7 lines

Quoted 3 lines
and btw:
$ git format-patch -1 --stdout <commit> | ./scripts/checkpatch.pl
Yes, I stated this was possible. My concern is there are lots of ways to generate a diff in git. My way works for *all* of them. Yours doesn't.

Joe, are you okay with this?

Rob

Joe Perches

maintainer

2019-10-11 18:02 UTC #7 lore raw

Quoted 49 lines · Rob Herring

Quoted 48 lines · Rob Herring
Quoted 33 lines · Joe Perches
Quoted 32 lines · Rob Herring
Quoted 12 lines · Joe Perches
Quoted 4 lines · Rob Herring
DT bindings are moving to using a json-schema based schema format instead of freeform text. Add a checkpatch.pl check to encourage using the schema for new bindings. It's not yet a requirement, but is progressively being required by some maintainers.
[]
scripts/checkpatch.pl
Quoted 1 line
scripts/checkpatch.pl +0 -0
[]
Quoted 4 lines
scripts/checkpatch.pl +0 -0
@@ -2822,6 +2822,14 @@ sub process {
                           "added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
              }
 
Quoted 10 lines
scripts/checkpatch.pl sub process { · line 2832
Quoted 8 lines
scripts/checkpatch.pl +8 -0
+# Check for adding new DT bindings not in schema format
+             if (!$in_commit_log &&
+                 ($line =~ /^new file mode\s*\d+\s*$/) &&
+                 ($realfile =~ m@^Documentation/devicetree/bindings/.*\.txt$@)) {
+                     WARN("DT_SCHEMA_BINDING_PATCH",
+                          "DT bindings should be in DT schema format. See: Documentation/devicetree/writing-schema.rst\n");
+             }
+
As this already seems to be git dependent, perhaps
It's quite rare to see a non git generated diff these days.
Quoted 5 lines
it's easier to read with a single line test like:
                if ($line =~ m{^\s*create mode\s*\d+\s*Documentation/devicetree/bindings/.*\.txt$}) {
                        etc...
                }
Quoted 8 lines
Quoted 6 lines
I frequently do 'git show $commit | scripts/checkpatch.pl' and this doesn't work with that. I really should have a '--pretty=email' in there, but I just ignore the commit msg warnings. In any case, that still doesn't help because there's no diffstat. There's probably some way to turn that on or just use git-format-patch, but really we want this to work with any git diff.
I don't understand your argument against what I proposed at all.
It is dependent on the commit message rather than the diff itself. I want it to work with or without a diffstat.
Quoted 2 lines
and btw:

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
$ git format-patch -1 --stdout <commit> | ./scripts/checkpatch.pl
Yes, I stated this was possible. My concern is there are lots of ways to generate a diff in git. My way works for *all* of them. Yours doesn't.
Joe, are you okay with this?

Sure, Andrew Morton does most of the checkpatch upstreaming, but if you want to send your own pull request, I've no objection.

Quoted 1 line

Rob

Rob Herring

author

2019-10-11 19:32 UTC #8 lore raw

Quoted 53 lines · Joe Perches

Quoted 51 lines · Rob Herring
Quoted 50 lines · Rob Herring
Quoted 33 lines · Joe Perches
Quoted 32 lines · Rob Herring
Quoted 12 lines · Joe Perches
Quoted 4 lines · Rob Herring
DT bindings are moving to using a json-schema based schema format instead of freeform text. Add a checkpatch.pl check to encourage using the schema for new bindings. It's not yet a requirement, but is progressively being required by some maintainers.
[]
scripts/checkpatch.pl
Quoted 1 line
scripts/checkpatch.pl +0 -0
[]
Quoted 4 lines
scripts/checkpatch.pl +0 -0
@@ -2822,6 +2822,14 @@ sub process {
                           "added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
              }
 
Quoted 10 lines
scripts/checkpatch.pl sub process { · line 2832
Quoted 8 lines
scripts/checkpatch.pl +8 -0
+# Check for adding new DT bindings not in schema format
+             if (!$in_commit_log &&
+                 ($line =~ /^new file mode\s*\d+\s*$/) &&
+                 ($realfile =~ m@^Documentation/devicetree/bindings/.*\.txt$@)) {
+                     WARN("DT_SCHEMA_BINDING_PATCH",
+                          "DT bindings should be in DT schema format. See: Documentation/devicetree/writing-schema.rst\n");
+             }
+
As this already seems to be git dependent, perhaps
It's quite rare to see a non git generated diff these days.
Quoted 5 lines
it's easier to read with a single line test like:
                if ($line =~ m{^\s*create mode\s*\d+\s*Documentation/devicetree/bindings/.*\.txt$}) {
                        etc...
                }
Quoted 8 lines
Quoted 6 lines
I frequently do 'git show $commit | scripts/checkpatch.pl' and this doesn't work with that. I really should have a '--pretty=email' in there, but I just ignore the commit msg warnings. In any case, that still doesn't help because there's no diffstat. There's probably some way to turn that on or just use git-format-patch, but really we want this to work with any git diff.
I don't understand your argument against what I proposed at all.
It is dependent on the commit message rather than the diff itself. I want it to work with or without a diffstat.
Quoted 3 lines
and btw:
$ git format-patch -1 --stdout <commit> | ./scripts/checkpatch.pl

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
Yes, I stated this was possible. My concern is there are lots of ways to generate a diff in git. My way works for *all* of them. Yours doesn't.
Joe, are you okay with this?
Sure, Andrew Morton does most of the checkpatch upstreaming, but if you want to send your own pull request, I've no objection.

Thanks, I've applied this to the DT tree.

Rob