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

[Rob Herring <robh@kernel.org>]

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
>