Re: [PATCH v2 4/4] dt-bindings: clk: versaclock5: convert to yaml

[Luca Ceresoli <luca@lucaceresoli.net>]

Hi Rob,

thanks for you review!

On 14/07/20 05:11, Rob Herring wrote:
> On Wed, Jul 08, 2020 at 09:40:35AM +0200, Luca Ceresoli wrote:
>> Convert to yaml the VersaClock bindings document. The mapping between
>> clock specifier and physical pins cannot be described formally in yaml
>> schema, then keep it verbatim in the description field.
>>
>> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>

[...]

>> +  reg:
>> +    maxItems: 1
>> +    description: I2C device address, shall be 0x68 or 0x6a.
> 
> Can be a schema:
> 
> enum: [ 0x68, 0x6a ]

Nice, will fix.

>> +
>> +  '#clock-cells':
>> +    const: 1
>> +
>> +patternProperties:
>> +  "^OUT[1-4]$":
>> +    type: object
>> +    description:
>> +      Description of one of the outputs (OUT1..OUT4). See "Clock1 Output
>> +      Configuration" in the Versaclock 5/6/6E Family Register Description
>> +      and Programming Guide.
>> +    properties:
>> +      idt,mode:
>> +        description:
>> +          The output drive mode. Values defined in dt-bindings/clk/versaclock.h
>> +        enum:
>> +          - VC5_LVPECL
> 
> This is defining a string. Can't use defines here.

How do I use the defines from include/dt-bindings then? Or should I just
use the numeric values then, like:

  idt,mode:
    description:
      The output drive mode. Values defined in
      dt-bindings/clk/versaclock.h
    minimum: 0
    maximum: 6

?

>> +      idt,voltage-microvolts:
>> +        description: The output drive voltage.
>> +        $ref: /schemas/types.yaml#/definitions/uint32
> 
> Standard unit suffixes have a type already, so drop.

Ok.

>> +allOf:
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            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
>> +      properties:
>> +        clock-names:
>> +          anyOf:
>> +            - required: [ xin ]
>> +            - required: [ clkin ]
> 
> This isn't valid. I think you want:
> 
> clock-names:
>   minItems: 1
>   items:
>     - const: xin
>     - const: clkin
> 
> This would mean 'xin' is always required, clkin is optional.

No, what I wanted to mean is that allowed cases are:
 * for idt,5p49v5933 and idt,5p49v5935:
   - only 'xin' (required)
 * for the other parts one of these:
   - only 'xin'
   - only 'clkin'
   - both 'xin' and 'clkin'

How do I express that?


A general note: as a newcomer to yaml bindings I found a steep learning
curve. Finding a correct construct (not to mention the best one) for
each situation is time consuming and frustrating. I've been looking at
existing files for suitable examples but it doesn't work very well.

Is there any guide to yaml bindings for beginners with examples of
typical cases? It would greatly help in producing better patches and
saving time for everybody.

Thanks,
-- 
Luca