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

["Álvaro Fernández Rojas" <noltari@gmail.com>]

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.