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

[Sergio Paracuellos]

On Mon, Jan 13, 2025 at 1:17 PM Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote:
>
> On 13/01/2025 12:32, Sergio Paracuellos wrote:
> > Hi Krzysztof,
> >
> > On Mon, Jan 13, 2025 at 12:18 PM Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote:
> >>
> >> On 13/01/2025 09:28, Sergio Paracuellos wrote:
> >>> Add clock missing definitions for RT2880, RT305X, RT3352, RT3383, RT5350,
> >>> MT-7620 and MT-76X8 Ralink SoCs.
> >>>
> >>> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@xxxxxxxxx>
> >>
> >> 1. Please use get_maintainers.
> >
> > I did but from my tree which is tag: v6.12.5, linux-stable/linux-6.12.y. Sorry.
>
> That's separate problem, but not one I was thinking about. Even on 6.12
> stable you would get dt list, which is missing here.
>
> This means no testing, although does not matter here, and it does not
> appear in Patchwork which means does not reach us, except my filter for
> nagging.

True. You are right. I forgot to add the device tree list to the cc
for this patch. Sorry.

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

>
> In any case there should be explanation in commit msg about such choice.

Sure, will do.

>
> >
> >>
> >>> ---
> >>>  include/dt-bindings/clock/mtmips-clk.h | 130 +++++++++++++++++++++++++
> >>
> >> Filename matching compatible.
> >
> > There are multiple compatibles regarding this, since the driver covers
>
> Driver almost does not matter.
>
> > a lot of SoCs. So I preferred to put them all in a single header file.
> > See [1]. Should I add a different file for any single compatible
> > instead?
>
> Well, are all in the same bindings file? If so, can stay but then:
> filename matching binding, because binding has proper naming scheme.

I see. Yes, they are in
Documentation/devicetree/bindings/clock/mediatek,mtmips-sysc.yaml

So I will rename this new header file to 'mediatek,mtmips-sysc.h' then.

>
> Best regards,
> Krzysztof

Thanks a lot!

Best regards,
    Sergio Paracuellos