Re: [PATCH] dt-bindings: clk: versaclock5: Miscellaneous fixes and improvements:

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Rob,

On Fri, Dec 18, 2020 at 12:59 AM Rob Herring <robh@kernel.org> wrote:
> On Wed, Dec 16, 2020 at 03:52:31PM +0100, Geert Uytterhoeven wrote:
> >   - Add reference to clock.yaml, and switch to unevaluatedProperties, to
> >     stop complaining about the presence of "assigned-clock-rates" and
> >     "assigned-clocks" in board DTS files,
> >   - Fix typo in "idt,voltage-microvolts" property name, to match example
> >     and driver code,
> >   - Add missing reference for "idt,voltage-microvolts",
> >   - Add missing "additionalProperties: false" for subnodes, to catch
> >     typos in properties,
> >   - There is no reason to wrap the (single) if condition in an allOf
> >     block,
>
> True, but more future proof with it and unnecessary churn IMO.

OK, I'll drop that part.

> >   - Fix obsolete property names in example.
> >
> > Fixes: 45c940184b501fc6 ("dt-bindings: clk: versaclock5: convert to yaml")
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > Notes:
> >   1. The use of "idt,voltage-microvolts" (with trailing S) is a bit
> >      unfortunate, as Documentation/devicetree/bindings/property-units.txt
> >      suggests to not have the trailing edge.
> >      Can we still fix the driver and bindings?  While this entered
> >      uptstream in v5.9, there are no users in next-20201216.
> >
> >   2. Due to "clock-output-names" being part of
> >      dt-schema/schemas/clock/clock.yaml, the presence of this property
> >      does not trigger an error.  Adding "clock-output-names: false"
> >      can fix that.  But given this property is deprecated, except for
> >      very specific use cases, explicitly allowing it for those few use
> >      cases would be better.
> > ---
> >  .../bindings/clock/idt,versaclock5.yaml       | 53 ++++++++++---------
> >  1 file changed, 29 insertions(+), 24 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> > index 2ac1131fd9222a86..14851e76f6342095 100644
> > --- a/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> > +++ b/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml
> > @@ -33,6 +33,9 @@ description: |
> >  maintainers:
> >    - Luca Ceresoli <luca@lucaceresoli.net>
> >
> > +allOf:
> > +  - $ref: clock.yaml#
>
> No, that's not right. clock.yaml is already applied unconditionally.

But without that, it complains about unevaluatedProperties?

> You need to define assigned-clocks, etc. here just like 'clocks' and
> define how many entries. Or convince me they should be allowed on any
> node.

They are handled by of_clk_set_defaults(), which is applied to all
clock providers.

> > @@ -73,40 +76,42 @@ patternProperties:
> >          $ref: /schemas/types.yaml#/definitions/uint32
> >          minimum: 0
> >          maximum: 6
> > -      idt,voltage-microvolt:
> > +      idt,voltage-microvolts:
>
> Can we fix the driver? No in tree users...

I think so.  Will do so.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds