[PATCH] checkpatch: Warn if DT bindings are not in schema format

[PATCH] checkpatch: Warn if DT bindings are not in schema format

[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.

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(+)

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",
-- 
2.20.1

Re: [PATCH] checkpatch: Warn if DT bindings are not in schema format

[Joe Perches]

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

Re: [PATCH] checkpatch: Warn if DT bindings are not in schema format

[Rob Herring]

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

Re: [PATCH] checkpatch: Warn if DT bindings are not in schema format

[Rob Herring]

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

Re: [PATCH] checkpatch: Warn if DT bindings are not in schema format

[Joe Perches]

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

Re: [PATCH] checkpatch: Warn if DT bindings are not in schema format

[Rob Herring]

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

Re: [PATCH] checkpatch: Warn if DT bindings are not in schema format

[Rob Herring]

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

Re: [PATCH] checkpatch: Warn if DT bindings are not in schema format

[Joe Perches]

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

Re: [PATCH] checkpatch: Warn if DT bindings are not in schema format

[Rob Herring]

On Fri, Oct 11, 2019 at 1:02 PM Joe Perches <joe@perches.com> wrote:
>
> 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.

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

Rob