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

[Rob Herring <robh@kernel.org>]

On Sun, Dec 20, 2020 at 3:24 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Rob,
>
> On Fri, Dec 18, 2020 at 11:32 PM Rob Herring <robh@kernel.org> wrote:
> > On Fri, Dec 18, 2020 at 5:42 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > 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,
>
> > > > > 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?
>
> I consider Linux the reference implementation.
> Is there any other real reference implementation? ;-)

No comment. ;)

> > 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.
>
> The problem with the assigned-clock* properties is that the DT binding
> writer has no idea if they will be ever used or not.  These properties
> come into play at an even higher level than the pinctrl properties.
> While the DT binding writer usually[1] knows if there can be pinctrl
> properties or not, this is not the case for the assigned-clock*
> properties.  The former are expected and mandatory, the latter are
> optional, and are added only during the system integration phase, and
> may appear everywhere.
>
> So I think they should be allowed on any node.  Unless we decide
> assigned-clock* properties are a bad idea in general.

Okay, let's add them automatically I guess. We can at least make them
dependent on having a 'clocks' property.

Rob