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

[Rob Herring <robh@kernel.org>]

On Fri, Dec 18, 2020 at 5:42 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> 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?

By design. You can't have other properties outside your binding unless
you have a $ref to other schemas. Also, note that there's not a single
other ref to clock.yaml.

> > 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.

What does that Linux implementation detail have to do with the bindings?

The only other exception we have is pinctrl properties. They often
aren't that interesting unless you have more than one (maybe we should
only automatically allow the single case). That's maybe true in the
assigned-clocks case too. However the big difference I see is pinctrl
properties are almost always present whereas assign-clocks is more the
exception. So I think it's good to be explicit where they are used.

Rob