Re: [PATCH V2 1/3] dt-bindings: clk: meson: add S4 SoC clock controller bindings

[Krzysztof Kozlowski]

On 28/07/2022 11:54, Jerome Brunet wrote:
> 
> On Thu 28 Jul 2022 at 11:48, Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> wrote:
> 
>> On 28/07/2022 11:09, Jerome Brunet wrote:
>>>
>>> On Thu 28 Jul 2022 at 11:02, Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> wrote:
>>>
>>>> On 28/07/2022 10:50, Jerome Brunet wrote:
>>>>>
>>>>> On Thu 28 Jul 2022 at 10:41, Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> wrote:
>>>>>
>>>>>> On 28/07/2022 07:42, Yu Tu wrote:
>>> [...]
>>>>>>> +/*
>>>>>>> + * CLKID index values
>>>>>>> + */
>>>>>>> +
>>>>>>> +#define CLKID_FIXED_PLL			1
>>>>>>> +#define CLKID_FCLK_DIV2			3
>>>>>>> +#define CLKID_FCLK_DIV3			5
>>>>>>> +#define CLKID_FCLK_DIV4			7
>>>>>>> +#define CLKID_FCLK_DIV5			9
>>>>>>> +#define CLKID_FCLK_DIV7			11
>>>>>>
>>>>>> Why these aren't continuous? IDs are expected to be incremented by 1.
>>>>>>
>>>>>
>>>>> All clocks have IDs, it is one big table in the driver, but we are not exposing them all.
>>>>> For example, with composite 'mux / div / gate' assembly, we usually need
>>>>> only the leaf.
>>>>
>>>> I understand you do not expose them all, but that is not the reason to
>>>> increment ID by 2 or 3... Otherwise these are not IDs and you are not
>>>> expected to put register offsets into the bindings (you do not bindings
>>>> in such case).
>>>
>>> Why is it not an IDs if it not continuous in the bindings ?
>>>
>>> If there is technical reason, we'll probably end up exposing everything. It
>>> would not be a dramatic change. I asked for this over v1 because we have
>>> done that is the past and I think it makes sense.
>>>
>>> I'm happy to be convinced to do things differently. Just looking for the
>>> technical reason that require contiuous exposed IDs.
>>>
>>> The other IDs exists, but we do not expose them as bindings.
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/meson/gxbb.h#n125
>>
>> https://lore.kernel.org/linux-devicetree/CAK8P3a1APzs74YTcZ=m43G3zrmwJZKcYSTvV5eDDQX-37UY7Tw@xxxxxxxxxxxxxx/
>>
>> https://lore.kernel.org/linux-devicetree/CAK8P3a0fDJQvGLEtG0fxLkG08Fh9V7LEMPsx4AaS+2Ldo_xWxw@xxxxxxxxxxxxxx/
>>
>> https://lore.kernel.org/linux-devicetree/b60f5fd2-dc48-9375-da1c-ffcfe8292683@xxxxxxxxxx/
>>
>> The IDs are abstract numbers, where the number does not matter because
>> it is not tied to driver implementation or device programming model. The
>> driver maps ID to respective clock.
>>
>> Using some meaningful numbers as these IDs, means you tied bindings to
>> your implementation and any change in implementation requires change in
>> the bindings. This contradicts the idea of bindings.
>>
> 
> I totally agree. Bindings ID are abstract numbers.
> We do follow that. We even document it:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/meson/gxbb.h#n118
> 
> It is just a choice to not expose some IDs.
> It is not tied to the implementation at all.
> I think we actually follow the rules and the idea behind it.
> 
> We can expose then all If you still think what we are doing is not appropriate.

No, no need. You are right and I took your not-by-one-increment-ID by
other approaches I saw.

The IDs do not have to be incremental, they should not be tied to
programming model.

You have it done and documented, so thanks for explanation:

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>


Best regards,
Krzysztof