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

Nicolas Saenz Julienne
2021-03-04T13:32:35Z

[-- Attachment #1: Type: text/plain, Size: 2832 bytes --]

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]