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

[Sergio Paracuellos]

On Tue, Jan 14, 2025 at 8:09 AM Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote:
>
> On 13/01/2025 13:58, Sergio Paracuellos wrote:
> > On Mon, Jan 13, 2025 at 1:37 PM Krzysztof Kozlowski <krzk@xxxxxxxxxx> 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.

Understood. Thanks for making it clear.

>
> >
> > 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?

Because I was expecting to have DTS as the only user and make things
easier for it.

>
> > 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.

Understood. So having only DTSs as users is not a possible way to go.
It is clear now. Forget about this patch then.

>
>
> Best regards,
> Krzysztof

Thanks,
    Sergio Paracuellos