Message-ID: <CAMhs-H_6QTptfFsSEh7PKy8Fnoem1ph4j=mwT_23=J=adDDT8w@mail.gmail.com>
Date: Mon, 13 Jan 2025 13:58:05 +0100
From: Sergio Paracuellos <sergio.paracuellos@...il.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: linux-clk@...r.kernel.org, sboyd@...nel.org, mturquette@...libre.com, 
	robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org, 
	yangshiji66@...look.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] dt-bindings: clock: add clock definitions for Ralink SoCs

On Mon, Jan 13, 2025 at 1:37 PM Krzysztof Kozlowski <krzk@...nel.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>;

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

>
> Best regards,
> Krzysztof

Thanks a lot for clarifications.

Best regards,
    Sergio Paracuellos

Powered by blists - more mailing lists