Re: [PATCH RFC 1/3] dt-bindings: clock: Add Renesas versa3 clock generator bindings

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 08/03/2023 15:39, Biju Das wrote:

>>> +++ b/Documentation/devicetree/bindings/clock/renesas,versaclock3.yaml
>>
>> Filename usually is based on the compatible. Why these two are so different?
> 
> Versa3 clock generator has the following variants.
> 
> 	5P35023, 5L35021, 5L35023 and 5P35021
> 
> RZ/G2L SMARC EVK uses 5P35023. So I used generic one as file name.
> And added compatible for specific one.

And what about other devices? Since you do not add them, just keep
compatible as filename.

>>
>>> +
>>> +  clocks:
>>> +    maxItems: 1
>>> +
>>> +  renesas,settings:
>>> +    description: Optional, complete register map of the device.
>>> +      Optimized settings for the device must be provided in full
>>> +      and are written during initialization.
>>> +    $ref: /schemas/types.yaml#/definitions/uint8-array
>>> +    minItems: 37
>>
>> maxItems instead... but I am not sure that we allow register settings in DT
>> in general.
> 
> Agreed. I guess it is allowed [1]
> [1] https://lore.kernel.org/linux-renesas-soc/833d3837892f0879233695636291af97a746e584.1643968653.git.michal.simek@xilinx.com/

I don't see Rob's review on this, so what do you prove exactly?

> 
>>
>>> +
>>> +  assigned-clocks:
>>> +    minItems: 6
>>
>> Drop.
> 
> OK.
> 
>>
>>> +
>>> +  assigned-clock-rates:
>>> +    minItems: 6
>>
>> Drop.
> 
> OK.
> 
>>
>>> +
>>> +  renesas,clock-divider-read-only:
>>> +    description: Flag for setting divider in read only mode.
>>
>> Flag? Then type: boolean.
> 
> Agreed.
>>
>>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>>> +    minItems: 5
>>
>> This is broken...
> OK you mean maxItems. Based on Boolean type I will update this

I mean, it does not match the description. Maybe I just don't understand
here something, but flag is boolean. Anyway, minItems means you can have
million of items, so was it intended?

>>
>>> +
>>> +  renesas,clock-flags:
>>> +    description: Flags used in common clock frame work for configuring
>>> +      clk outputs. See include/linux/clk-provider.h
>>
>> These are not bindings, so why do you non-bindings constants as bindings?
>> They can change anytime. Choose one:
>> 1. Drop entire property,
>> 2. Make it a proper binding property, so an ABI and explain why this is DT
>> specific. None of clock providers have to do it, so you need here good
>> explanation.
> 
> I will choose 2 and will explain as user should be allowed to
> configure the output clock from clk generator, so that it has flexibility
> for
> 1) changing the rates (propagate rate change up one level)
> 2) fixed always 
> 3) don't gate the ouput clk at all.

User's choice is task for user-space, so not a good explanation for DT.

Best regards,
Krzysztof