All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] dt-bindings: clk: versaclock5: Miscellaneous fixes and improvements:
@ 2020-12-16 14:52 Geert Uytterhoeven
  2020-12-17 23:59 ` Rob Herring
  0 siblings, 1 reply; 6+ messages in thread
From: Geert Uytterhoeven @ 2020-12-16 14:52 UTC (permalink / raw)
  To: Luca Ceresoli, Adam Ford, Michael Turquette, Stephen Boyd, Rob Herring
  Cc: linux-clk, devicetree, linux-renesas-soc, Geert Uytterhoeven

  - 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,
  - 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#
+
 properties:
   compatible:
     enum:
@@ -73,40 +76,42 @@ patternProperties:
         $ref: /schemas/types.yaml#/definitions/uint32
         minimum: 0
         maximum: 6
-      idt,voltage-microvolt:
+      idt,voltage-microvolts:
         description: The output drive voltage.
+        $ref: /schemas/types.yaml#/definitions/uint32
         enum: [ 1800000, 2500000, 3300000 ]
       idt,slew-percent:
         description: The Slew rate control for CMOS single-ended.
         $ref: /schemas/types.yaml#/definitions/uint32
         enum: [ 80, 85, 90, 100 ]
 
+    additionalProperties: false
+
 required:
   - compatible
   - reg
   - '#clock-cells'
 
-allOf:
-  - if:
-      properties:
-        compatible:
-          enum:
-            - idt,5p49v5933
-            - idt,5p49v5935
-    then:
-      # Devices with builtin crystal + optional external input
-      properties:
-        clock-names:
-          const: clkin
-        clocks:
-          maxItems: 1
-    else:
-      # Devices without builtin crystal
-      required:
-        - clock-names
-        - clocks
-
-additionalProperties: false
+if:
+  properties:
+    compatible:
+      enum:
+        - idt,5p49v5933
+        - idt,5p49v5935
+then:
+  # Devices with builtin crystal + optional external input
+  properties:
+    clock-names:
+      const: clkin
+    clocks:
+      maxItems: 1
+else:
+  # Devices without builtin crystal
+  required:
+    - clock-names
+    - clocks
+
+unevaluatedProperties: false
 
 examples:
   - |
@@ -135,13 +140,13 @@ examples:
             clock-names = "xin";
 
             OUT1 {
-                idt,drive-mode = <VC5_CMOSD>;
+                idt,mode = <VC5_CMOSD>;
                 idt,voltage-microvolts = <1800000>;
                 idt,slew-percent = <80>;
             };
 
             OUT4 {
-                idt,drive-mode = <VC5_LVDS>;
+                idt,mode = <VC5_LVDS>;
             };
         };
     };
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] dt-bindings: clk: versaclock5: Miscellaneous fixes and improvements:
  2020-12-16 14:52 [PATCH] dt-bindings: clk: versaclock5: Miscellaneous fixes and improvements: Geert Uytterhoeven
@ 2020-12-17 23:59 ` Rob Herring
  2020-12-18 11:42   ` Geert Uytterhoeven
  0 siblings, 1 reply; 6+ messages in thread
From: Rob Herring @ 2020-12-17 23:59 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Luca Ceresoli, Adam Ford, Michael Turquette, Stephen Boyd,
	linux-clk, devicetree, linux-renesas-soc

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.

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

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.

> +
>  properties:
>    compatible:
>      enum:
> @@ -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...

>          description: The output drive voltage.
> +        $ref: /schemas/types.yaml#/definitions/uint32
>          enum: [ 1800000, 2500000, 3300000 ]
>        idt,slew-percent:
>          description: The Slew rate control for CMOS single-ended.
>          $ref: /schemas/types.yaml#/definitions/uint32
>          enum: [ 80, 85, 90, 100 ]
>  
> +    additionalProperties: false
> +
>  required:
>    - compatible
>    - reg
>    - '#clock-cells'
>  
> -allOf:
> -  - if:
> -      properties:
> -        compatible:
> -          enum:
> -            - idt,5p49v5933
> -            - idt,5p49v5935
> -    then:
> -      # Devices with builtin crystal + optional external input
> -      properties:
> -        clock-names:
> -          const: clkin
> -        clocks:
> -          maxItems: 1
> -    else:
> -      # Devices without builtin crystal
> -      required:
> -        - clock-names
> -        - clocks
> -
> -additionalProperties: false
> +if:
> +  properties:
> +    compatible:
> +      enum:
> +        - idt,5p49v5933
> +        - idt,5p49v5935
> +then:
> +  # Devices with builtin crystal + optional external input
> +  properties:
> +    clock-names:
> +      const: clkin
> +    clocks:
> +      maxItems: 1
> +else:
> +  # Devices without builtin crystal
> +  required:
> +    - clock-names
> +    - clocks
> +
> +unevaluatedProperties: false
>  
>  examples:
>    - |
> @@ -135,13 +140,13 @@ examples:
>              clock-names = "xin";
>  
>              OUT1 {
> -                idt,drive-mode = <VC5_CMOSD>;
> +                idt,mode = <VC5_CMOSD>;
>                  idt,voltage-microvolts = <1800000>;
>                  idt,slew-percent = <80>;
>              };
>  
>              OUT4 {
> -                idt,drive-mode = <VC5_LVDS>;
> +                idt,mode = <VC5_LVDS>;
>              };
>          };
>      };
> -- 
> 2.25.1
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] dt-bindings: clk: versaclock5: Miscellaneous fixes and improvements:
  2020-12-17 23:59 ` Rob Herring
@ 2020-12-18 11:42   ` Geert Uytterhoeven
  2020-12-18 22:32     ` Rob Herring
  0 siblings, 1 reply; 6+ messages in thread
From: Geert Uytterhoeven @ 2020-12-18 11:42 UTC (permalink / raw)
  To: Rob Herring
  Cc: Luca Ceresoli, Adam Ford, Michael Turquette, Stephen Boyd,
	linux-clk,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] dt-bindings: clk: versaclock5: Miscellaneous fixes and improvements:
  2020-12-18 11:42   ` Geert Uytterhoeven
@ 2020-12-18 22:32     ` Rob Herring
  2020-12-20 10:24       ` Geert Uytterhoeven
  0 siblings, 1 reply; 6+ messages in thread
From: Rob Herring @ 2020-12-18 22:32 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Luca Ceresoli, Adam Ford, Michael Turquette, Stephen Boyd,
	linux-clk,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] dt-bindings: clk: versaclock5: Miscellaneous fixes and improvements:
  2020-12-18 22:32     ` Rob Herring
@ 2020-12-20 10:24       ` Geert Uytterhoeven
  2020-12-21 17:21         ` Rob Herring
  0 siblings, 1 reply; 6+ messages in thread
From: Geert Uytterhoeven @ 2020-12-20 10:24 UTC (permalink / raw)
  To: Rob Herring
  Cc: Luca Ceresoli, Adam Ford, Michael Turquette, Stephen Boyd,
	linux-clk,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, Sylwester Nawrocki

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? ;-)

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

[1] Unless the bindings are written initially for an IP core in an SoC
    that doesn't have pinctrl for the related pins, and the IP core is
    reused later in an SoC that does have pinctrl.

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] dt-bindings: clk: versaclock5: Miscellaneous fixes and improvements:
  2020-12-20 10:24       ` Geert Uytterhoeven
@ 2020-12-21 17:21         ` Rob Herring
  0 siblings, 0 replies; 6+ messages in thread
From: Rob Herring @ 2020-12-21 17:21 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Luca Ceresoli, Adam Ford, Michael Turquette, Stephen Boyd,
	linux-clk,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, Sylwester Nawrocki

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-12-21 17:22 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-16 14:52 [PATCH] dt-bindings: clk: versaclock5: Miscellaneous fixes and improvements: Geert Uytterhoeven
2020-12-17 23:59 ` Rob Herring
2020-12-18 11:42   ` Geert Uytterhoeven
2020-12-18 22:32     ` Rob Herring
2020-12-20 10:24       ` Geert Uytterhoeven
2020-12-21 17:21         ` Rob Herring

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.