Re: [PATCH] dt-bindings: clock: add clock definitions for Ralink SoCs

[Krzysztof Kozlowski <krzk@kernel.org>]

On 13/01/2025 13:58, Sergio Paracuellos wrote:
> On Mon, Jan 13, 2025 at 1:37 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On 13/01/2025 13:29, Sergio Paracuellos wrote:
>>>>>
>>>>>> 2. What is the point of this? We do not add constants when there are no
>>>>>> users. Commit msg explains here nothing.
>>>>>
>>>>> All of the old ralink SoCs' dts files which are in the tree are not
>>>>> properly updated. I expect to have them updated somewhere in time
>>>>> merging real base stuff from openwrt dts [0] files. Not having this
>>>>> header with definitions makes very hard to update dts and then
>>>>> checking the driver code becomes a need to see the indexes for the
>>>>> clocks to properly setup a consumer node. Because of this, this file
>>>>> is added here.
>>>>
>>>> Still there is no point without the users. I do not see any reason why
>>>> this cannot be combined with fixing driver to use the header. Not
>>>> combining is an indication this is not a binding in the first place.
>>>
>>> Driver uses a bunch of arrays for the clocks (base, fixed, factor and
>>> peripheral) and they are registered consecutively in order just using
>>> the ARRAY_SIZE macro for any of them. Thus, the direct application of
>>> these definitions would be for dts consumer nodes, not the driver
>>> itself.
>>
>> So what do you constants here fix? Driver can still reorganize arrays
>> breaking everything. If defining headers for proper ABI, then use that
>> ABI to make everything build-time testable and visible. That's why this
>> is not supposed to be a separate patch from users.
> 
> I understand your point and agree that the driver can do that, but the
> idea as this driver maintainer is not to do that :).
> Is adding something like the following in the binding itself with the
> header addition a possible way to go?
> 
> --- a/Documentation/devicetree/bindings/clock/mediatek,mtmips-sysc.yaml
> +++ b/Documentation/devicetree/bindings/clock/mediatek,mtmips-sysc.yaml
> @@ -18,6 +18,12 @@ description: |
>    These SoCs have an XTAL from where the cpu clock is
>    provided as well as derived clocks for the bus and the peripherals.
> 
> +  Each clock is assigned an identifier and client nodes use this identifier
> +  to specify the clock which they consume.
> +
> +  All these identifiers could be found in:
> +  [1]: <include/dt-bindings/clock/mediatek,mtmips-sysc.h>.
> +
>  properties:
>    compatible:
>      items:
> @@ -38,7 +44,8 @@ properties:
> 
>    '#clock-cells':
>      description:
> -      The first cell indicates the clock number.
> +      The first cell indicates the clock number, see [1] for available
> +      clocks.
>      const: 1
> 
> @@ -56,6 +63,8 @@ additionalProperties: false
> 
>  examples:
>    - |
> +    #include <dt-bindings/clock/mediatek,mtmips-sysc.h>
> +
>      syscon@0 {
>        compatible = "ralink,rt5350-sysc", "syscon";
>        reg = <0x0 0x100>;

This changes nothing.

> 
> I don't like the idea of changing the driver code for using these
> constants since I do believe that it would make code uglier and less
> maintainable. I just wanted to make things easier for the device tree

Then why having constants in the first place?

> consumer nodes. So if adding this header is not a possibility with the
> changes in the yaml file I will forget about this addition and this
> patch.

Header without user is pointless. Driver and the DTS are the expected users.


Best regards,
Krzysztof