diff mbox series

[v3,1/2] dt-bindings: rng: bcm2835: document reset support

Message ID 20210223170006.29558-2-noltari@gmail.com
State Superseded
Headers show
Series [v3,1/2] dt-bindings: rng: bcm2835: document reset support | expand

Checks

Context Check Description
robh/checkpatch success
robh/dt-meta-schema fail build log

Commit Message

Álvaro Fernández Rojas Feb. 23, 2021, 5 p.m. UTC
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(+)

Comments

Rob Herring Feb. 23, 2021, 7:34 p.m. UTC | #1
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.
Álvaro Fernández Rojas Feb. 23, 2021, 8:48 p.m. UTC | #2
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.
Nicolas Saenz Julienne March 4, 2021, 12:07 p.m. UTC | #3
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
Álvaro Fernández Rojas March 4, 2021, 12:18 p.m. UTC | #4
> 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.
Nicolas Saenz Julienne March 4, 2021, 1:30 p.m. UTC | #5
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
Álvaro Fernández Rojas March 4, 2021, 2:57 p.m. UTC | #6
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.
Rob Herring March 4, 2021, 7:58 p.m. UTC | #7
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 mbox series

Patch

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>;
     };