| Message ID | 20210223170006.29558-2-noltari@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hwrng: bcm2835: add reset support | expand |
On Tue, 23 Feb 2021 18:00:05 +0100, Álvaro Fernández Rojas wrote: > Some devices may need to perform a reset before using the RNG, such as the > BCM6368. > > Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> > --- > v3: make resets required if brcm,bcm6368-rng. > v2: document reset support. > > .../devicetree/bindings/rng/brcm,bcm2835.yaml | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/rng/brcm,bcm2835.example.dt.yaml: rng@10004180: 'resets' does not match any of the regexes: 'pinctrl-[0-9]+' From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml See https://patchwork.ozlabs.org/patch/1443582 This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.
Hi Rob, > El 23 feb 2021, a las 20:34, Rob Herring <robh@kernel.org> escribió: > > On Tue, 23 Feb 2021 18:00:05 +0100, Álvaro Fernández Rojas wrote: >> Some devices may need to perform a reset before using the RNG, such as the >> BCM6368. >> >> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> >> --- >> v3: make resets required if brcm,bcm6368-rng. >> v2: document reset support. >> >> .../devicetree/bindings/rng/brcm,bcm2835.yaml | 17 +++++++++++++++++ >> 1 file changed, 17 insertions(+) >> > > My bot found errors running 'make dt_binding_check' on your patch: > > yamllint warnings/errors: > > dtschema/dtc warnings/errors: > /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/rng/brcm,bcm2835.example.dt.yaml: rng@10004180: 'resets' does not match any of the regexes: 'pinctrl-[0-9]+' > From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml I don’t get what’s wrong here... > > See https://patchwork.ozlabs.org/patch/1443582 > > This check can fail if there are any dependencies. The base for a patch > series is generally the most recent rc1. > > If you already ran 'make dt_binding_check' and didn't see the above > error(s), then make sure 'yamllint' is installed and dt-schema is up to > date: > > pip3 install dtschema --upgrade > > Please check and re-submit. > Best regards, Álvaro.
Hi Alvaro, On Tue, 2021-02-23 at 18:00 +0100, Álvaro Fernández Rojas wrote: > Some devices may need to perform a reset before using the RNG, such as the > BCM6368. > > Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> > --- > v3: make resets required if brcm,bcm6368-rng. > v2: document reset support. > > .../devicetree/bindings/rng/brcm,bcm2835.yaml | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml b/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml > index c147900f9041..11c23e1f6988 100644 > --- a/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml > +++ b/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml > @@ -37,6 +37,21 @@ required: > > > additionalProperties: false I can't claim I fully understand all the meta stuff in shemas, so I generally just follow the patterns already available out there. That said, shoudln't this be at the end, just before the examples? Maybe the cause of that odd warning you got there? > +if: > + properties: > + compatible: > + enum: > + - brcm,bcm6368-rng > +then: > + properties: > + resets: > + maxItems: 1 > + required: > + - resets I belive you can't really make a property required when the bindings for 'brcm,bcm6368-rng' were already defined. This will break the schema for those otherwise correct devicetrees. > +else: > + properties: > + resets: false > + > examples: > - | > rng@7e104000 { > @@ -58,4 +73,6 @@ examples: > > > clocks = <&periph_clk 18>; > clock-names = "ipsec"; > + > + resets = <&periph_rst 4>; > }; Regards, Nicolas
> El 4 mar 2021, a las 13:07, Nicolas Saenz Julienne <nsaenzjulienne@suse.de> escribió: > > Hi Alvaro, > > On Tue, 2021-02-23 at 18:00 +0100, Álvaro Fernández Rojas wrote: >> Some devices may need to perform a reset before using the RNG, such as the >> BCM6368. >> >> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> >> --- >> v3: make resets required if brcm,bcm6368-rng. >> v2: document reset support. >> >> .../devicetree/bindings/rng/brcm,bcm2835.yaml | 17 +++++++++++++++++ >> 1 file changed, 17 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml b/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml >> index c147900f9041..11c23e1f6988 100644 >> --- a/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml >> +++ b/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml >> @@ -37,6 +37,21 @@ required: >> >> >> additionalProperties: false > > I can't claim I fully understand all the meta stuff in shemas, so I generally > just follow the patterns already available out there. Well, that makes two of us :). > That said, shoudln't this be at the end, just before the examples? I don’t know but I can move it there ¯\_(ツ)_/¯ > Maybe the cause of that odd warning > you got there? Which odd warning? I don’t get any warnings when running (or at least warnings related to rig, because I get warnings related to other yamls): make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml > >> +if: >> + properties: >> + compatible: >> + enum: >> + - brcm,bcm6368-rng >> +then: >> + properties: >> + resets: >> + maxItems: 1 >> + required: >> + - resets > > I belive you can't really make a property required when the bindings for > 'brcm,bcm6368-rng' were already defined. This will break the schema for those > otherwise correct devicetrees. Why not? Wouldn’t just be required for brcm,bcm6368-rng? Anyway, I can omit this, since it would be the same for clocks and those aren’t required either. > >> +else: >> + properties: >> + resets: false >> + >> examples: >> - | >> rng@7e104000 { >> @@ -58,4 +73,6 @@ examples: >> >> >> clocks = <&periph_clk 18>; >> clock-names = "ipsec"; >> + >> + resets = <&periph_rst 4>; >> }; > > Regards, > Nicolas Best regards, Álvaro.
On Thu, 2021-03-04 at 13:18 +0100, Álvaro Fernández Rojas wrote: > > > El 4 mar 2021, a las 13:07, Nicolas Saenz Julienne <nsaenzjulienne@suse.de> escribió: > > > > Hi Alvaro, > > > > On Tue, 2021-02-23 at 18:00 +0100, Álvaro Fernández Rojas wrote: > > > Some devices may need to perform a reset before using the RNG, such as the > > > BCM6368. > > > > > > Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> > > > --- > > > v3: make resets required if brcm,bcm6368-rng. > > > v2: document reset support. > > > > > > .../devicetree/bindings/rng/brcm,bcm2835.yaml | 17 +++++++++++++++++ > > > 1 file changed, 17 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml b/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml > > > index c147900f9041..11c23e1f6988 100644 > > > --- a/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml > > > +++ b/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml > > > @@ -37,6 +37,21 @@ required: > > > > > > > > > > > > additionalProperties: false > > > > I can't claim I fully understand all the meta stuff in shemas, so I generally > > just follow the patterns already available out there. > > Well, that makes two of us :). > > > That said, shoudln't this be at the end, just before the examples? > > I don’t know but I can move it there ¯\_(ツ)_/¯ Yes please do. See commit 7f464532b05 ("dt-bindings: Add missing 'additionalProperties: false'") which expands on why it is needed, and why it should be at the end. > > Maybe the cause of that odd warning > > you got there? > > Which odd warning? The one pointed out by Rob Herring's script, which I can reproduce too BTW. On the other hand I can't really tell what's wrong right away. > I don’t get any warnings when running (or at least warnings related to rig, because I get warnings related to other yamls): > make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml > > > > > > +if: > > > + properties: > > > + compatible: > > > + enum: > > > + - brcm,bcm6368-rng > > > +then: > > > + properties: > > > + resets: > > > + maxItems: 1 > > > + required: > > > + - resets > > > > I belive you can't really make a property required when the bindings for > > 'brcm,bcm6368-rng' were already defined. This will break the schema for those > > otherwise correct devicetrees. > > Why not? > Wouldn’t just be required for brcm,bcm6368-rng? Well, do all 'brcm,bcm6368-rng' users absolutely need the reset controller? If so, the original binding is wrong, which should be mentioned and a 'Fixes:' tag should be added to the commit message. Otherwise, the requirement is optional. Regards, Nicolas
Hi Nicolas, > El 4 mar 2021, a las 14:30, Nicolas Saenz Julienne <nsaenzjulienne@suse.de> escribió: > > On Thu, 2021-03-04 at 13:18 +0100, Álvaro Fernández Rojas wrote: >> >>> El 4 mar 2021, a las 13:07, Nicolas Saenz Julienne <nsaenzjulienne@suse.de> escribió: >>> >>> Hi Alvaro, >>> >>> On Tue, 2021-02-23 at 18:00 +0100, Álvaro Fernández Rojas wrote: >>>> Some devices may need to perform a reset before using the RNG, such as the >>>> BCM6368. >>>> >>>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> >>>> --- >>>> v3: make resets required if brcm,bcm6368-rng. >>>> v2: document reset support. >>>> >>>> .../devicetree/bindings/rng/brcm,bcm2835.yaml | 17 +++++++++++++++++ >>>> 1 file changed, 17 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml b/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml >>>> index c147900f9041..11c23e1f6988 100644 >>>> --- a/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml >>>> +++ b/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml >>>> @@ -37,6 +37,21 @@ required: >>>> >>>> >>>> >>>> additionalProperties: false >>> >>> I can't claim I fully understand all the meta stuff in shemas, so I generally >>> just follow the patterns already available out there. >> >> Well, that makes two of us :). >> >>> That said, shoudln't this be at the end, just before the examples? >> >> I don’t know but I can move it there ¯\_(ツ)_/¯ > > Yes please do. See commit 7f464532b05 ("dt-bindings: Add missing > 'additionalProperties: false'") which expands on why it is needed, and why it > should be at the end. > >>> Maybe the cause of that odd warning >>> you got there? >> >> Which odd warning? > > The one pointed out by Rob Herring's script, which I can reproduce too BTW. On > the other hand I can't really tell what's wrong right away. Well, I can’t reproduce that locally and I don’t know what’s wrong either, I think that the best option is to remove the full if block. > >> I don’t get any warnings when running (or at least warnings related to rig, because I get warnings related to other yamls): >> make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml >> >>> >>>> +if: >>>> + properties: >>>> + compatible: >>>> + enum: >>>> + - brcm,bcm6368-rng >>>> +then: >>>> + properties: >>>> + resets: >>>> + maxItems: 1 >>>> + required: >>>> + - resets >>> >>> I belive you can't really make a property required when the bindings for >>> 'brcm,bcm6368-rng' were already defined. This will break the schema for those >>> otherwise correct devicetrees. >> >> Why not? >> Wouldn’t just be required for brcm,bcm6368-rng? > > Well, do all 'brcm,bcm6368-rng' users absolutely need the reset controller? If > so, the original binding is wrong, which should be mentioned and a 'Fixes:' tag > should be added to the commit message. Otherwise, the requirement is optional. I’m not sure about this... > > Regards, > Nicolas Best regards, Álvaro.
On Thu, Mar 4, 2021 at 6:07 AM Nicolas Saenz Julienne <nsaenzjulienne@suse.de> wrote: > > Hi Alvaro, > > On Tue, 2021-02-23 at 18:00 +0100, Álvaro Fernández Rojas wrote: > > Some devices may need to perform a reset before using the RNG, such as the > > BCM6368. > > > > Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> > > --- > > v3: make resets required if brcm,bcm6368-rng. > > v2: document reset support. > > > > .../devicetree/bindings/rng/brcm,bcm2835.yaml | 17 +++++++++++++++++ > > 1 file changed, 17 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml b/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml > > index c147900f9041..11c23e1f6988 100644 > > --- a/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml > > +++ b/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml > > @@ -37,6 +37,21 @@ required: > > > > > > additionalProperties: false > > I can't claim I fully understand all the meta stuff in shemas, so I generally > just follow the patterns already available out there. That said, shoudln't this > be at the end, just before the examples? Maybe the cause of that odd warning > you got there? Yes, 'resets' needs to be defined under 'properties' as additionalProperties can't 'see' into if/then schemas. The top level needs to define everything and then if/then schema just add additional constraints. > > > +if: > > + properties: > > + compatible: > > + enum: > > + - brcm,bcm6368-rng > > +then: > > + properties: > > + resets: > > + maxItems: 1 > > + required: > > + - resets > > I belive you can't really make a property required when the bindings for > 'brcm,bcm6368-rng' were already defined. This will break the schema for those > otherwise correct devicetrees. Right, unless not having the property meant the device never would have worked. Rob
diff --git a/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml b/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml index c147900f9041..11c23e1f6988 100644 --- a/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml +++ b/Documentation/devicetree/bindings/rng/brcm,bcm2835.yaml @@ -37,6 +37,21 @@ required: additionalProperties: false +if: + properties: + compatible: + enum: + - brcm,bcm6368-rng +then: + properties: + resets: + maxItems: 1 + required: + - resets +else: + properties: + resets: false + examples: - | rng@7e104000 { @@ -58,4 +73,6 @@ examples: clocks = <&periph_clk 18>; clock-names = "ipsec"; + + resets = <&periph_rst 4>; };
Some devices may need to perform a reset before using the RNG, such as the BCM6368. Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> --- v3: make resets required if brcm,bcm6368-rng. v2: document reset support. .../devicetree/bindings/rng/brcm,bcm2835.yaml | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)