| Message ID | 20190913211349.28245-1-robh@kernel.org |
|---|---|
| State | Accepted |
| Commit | e400edb141d74aa2f04d0071aadb47fdb8f7ae55 |
| Headers | show |
| Series | checkpatch: Warn if DT bindings are not in schema format | expand |
On Fri, 2019-09-13 at 16:13 -0500, Rob Herring wrote: > 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. [] > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl [] > @@ -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
On Fri, Sep 13, 2019 at 4:48 PM Joe Perches <joe@perches.com> wrote: > > On Fri, 2019-09-13 at 16:13 -0500, Rob Herring wrote: > > 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. > [] > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > [] > > @@ -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... > } Okay. I wasn't too concerned about non-git diffs as I rarely see those anymore. > > 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. > > 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
On Fri, Sep 13, 2019 at 4:48 PM Joe Perches <joe@perches.com> wrote: > > On Fri, 2019-09-13 at 16:13 -0500, Rob Herring wrote: > > 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. > [] > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > [] > > @@ -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 quite rare to see a non git generated diff these days. > 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
On Fri, 2019-09-27 at 09:02 -0500, Rob Herring wrote: > On Fri, Sep 13, 2019 at 4:48 PM Joe Perches <joe@perches.com> wrote: > > On Fri, 2019-09-13 at 16:13 -0500, Rob Herring wrote: > > > 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. > > [] > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > > [] > > > @@ -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 quite rare to see a non git generated diff these days. > > > 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. I don't understand your argument against what I proposed at all. and btw: $ git format-patch -1 --stdout <commit> | ./scripts/checkpatch.pl
On Fri, Sep 27, 2019 at 9:29 AM Joe Perches <joe@perches.com> wrote: > > On Fri, 2019-09-27 at 09:02 -0500, Rob Herring wrote: > > On Fri, Sep 13, 2019 at 4:48 PM Joe Perches <joe@perches.com> wrote: > > > On Fri, 2019-09-13 at 16:13 -0500, Rob Herring wrote: > > > > 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. > > > [] > > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > > > [] > > > > @@ -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 quite rare to see a non git generated diff these days. > > > > > 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. > > 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. > 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
On Fri, Sep 27, 2019 at 10:39 AM Rob Herring <robh@kernel.org> wrote: > > On Fri, Sep 27, 2019 at 9:29 AM Joe Perches <joe@perches.com> wrote: > > > > On Fri, 2019-09-27 at 09:02 -0500, Rob Herring wrote: > > > On Fri, Sep 13, 2019 at 4:48 PM Joe Perches <joe@perches.com> wrote: > > > > On Fri, 2019-09-13 at 16:13 -0500, Rob Herring wrote: > > > > > 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. > > > > [] > > > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > > > > [] > > > > > @@ -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 quite rare to see a non git generated diff these days. > > > > > > > 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. > > > > 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. > > > 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
On Fri, 2019-10-11 at 12:56 -0500, Rob Herring wrote: > On Fri, Sep 27, 2019 at 10:39 AM Rob Herring <robh@kernel.org> wrote: > > On Fri, Sep 27, 2019 at 9:29 AM Joe Perches <joe@perches.com> wrote: > > > On Fri, 2019-09-27 at 09:02 -0500, Rob Herring wrote: > > > > On Fri, Sep 13, 2019 at 4:48 PM Joe Perches <joe@perches.com> wrote: > > > > > On Fri, 2019-09-13 at 16:13 -0500, Rob Herring wrote: > > > > > > 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. > > > > > [] > > > > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > > > > > [] > > > > > > @@ -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 quite rare to see a non git generated diff these days. > > > > > > > > > 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. > > > > > > 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. > > > > > 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? Sure, Andrew Morton does most of the checkpatch upstreaming, but if you want to send your own pull request, I've no objection. > Rob
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 93a7edfe0f05..1cbd85f16e32 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -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",
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 <apw@canonical.com> Cc: Joe Perches <joe@perches.com> Signed-off-by: Rob Herring <robh@kernel.org> --- scripts/checkpatch.pl | 8 ++++++++ 1 file changed, 8 insertions(+) -- 2.20.1