Message-ID: <3fb8ad2b-016d-4eee-af57-be7dec659f4c@kernel.org>
Date: Thu, 29 May 2025 11:00:39 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Christian Marangi <ansuelsmth@...il.com>
Cc: Michael Turquette <mturquette@...libre.com>,
 Stephen Boyd <sboyd@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Philipp Zabel <p.zabel@...gutronix.de>,
 Felix Fietkau <nbd@....name>, linux-clk@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/5] dt-bindings: clock: airoha: Document support for
 AN7583 clock

On 28/05/2025 14:57, Christian Marangi wrote:
>>> Again sorry if this question keeps coming around and I can totally
>>> understand if you are getting annoyed by this. The reason I always ask
>>> this is because it's a total PAIN to implement this with the driver
>>> structure due to the old "simple-mfd" model.
>>
>> ... and Rob was saying multiple times: be careful when adding
>> simple-mfd. If it bites back, then I am sorry, but everyone were warned,
>> weren't they?
>>
>> What is exactly the pain anyway? You cannot instantiate children from
>> SCU driver?
>>
> 
> Answering below since they are related.
> 
>>>
>>> (as again putting everything in a single node conflicts with the OF
>>> principle of autoprobing stuff with compatible property)
>>
>> I am not sure if I follow. What principle? Where is this principle
>> expressed?
>>
>> And you do not have in your second example additional compatibles, so
>> even if such principle exists it is not broken: everything autoprobes, I
>> think.
>>
>>>
>>
>>
> 
> The principle I'm talking about is one driver for one compatible.

There is no such principle. One compatible can map to many drivers and
many compatibles can map to one driver.

> (to be more precise excluding syscon compatible that is actually
> ignored, if a driver for the compatible is found, any other compatible
> is ignored.)
> 
> This means that declaring multiple compatible as:
> 
> compatible = "airoha,clock", "airoha,mdio"
> 
> doesn't result in the clock driver and the mdio driver probed but only
> one of the 2 (probably only clock since it does have priority)

I don't understand this example. It makes no sense - clock is not
compatible with mdio.

> 
> The "simple-mfd" compatible is just a simple compatible that indicate to
> the OF system that every child (with a compatible) should be also probed.
> And then automagically the driver gets probed.
> 
> Now the ""PAIN"" explaination. Not using the "simple-mfd" way with the
> child with compatible and putting everything in the node means having to
> create a dedicated MFD driver that just instruct to manually probe the
> clock and mdio driver. (cause the compatible system can't be used)

You already have that driver - SCU. No need for new MFD driver...


> 
> So it's 3 driver instead of 2 with the extra effort of MFD driver
> maintainer saying "Why simple-mfd is not used?"

Sorry, that's a wrong argument. You can use simple-mfd, iff it follows
standard practices. If it does not fit standard practices, you cannot
use an argument "now I need more complicated solution".

> 
> 
> There is a solution for this but I always feel it's more of a workaround
> since it doesn't really describe the HW with the DT node.

Really? All arguments you used here are driver arguments - that
something is a pain in drivers. Now you mention that hardware would not
match description.

Then let's change entire talk towards hardware description and send
patches matching hardware, not matching your MFD driver structure.

> 
> The workaround is:
> 
> 		system-controller@...20000 {
>                         /* The parent SCU node implement the clock driver */
>                         compatible = "airoha,an7583-scu", "syscon";
>                         reg = <0x0 0x1fb00000 0x0 0x970>;
> 
>                         #clock-cells = <1>;
>                         #reset-cells = <1>;
> 
>                         /* Clock driver is instructed to probe child */
>                         mdio {
>                                 compatible = "airoha,an7583-mdio";

Again, drop compatible.

>                                 #address-cells = <1>;
>                                 #size-cells = <0>;
> 
>                                 mdio_0: bus@0 {
>                                         reg = <0>;
>                                         resets = <&scuclk AN7583_MDIO0>;
>                                 };
> 
>                                 mdio_1: bus@1 {
>                                         reg = <1>;
>                                         resets = <&scuclk AN7583_MDIO1>;
>                                 };
>                         };
>                 };
> 
> 
> But this really moves the probe from the simple-mfd to the clock driver.
> 
> So it's either 3 solution
> 1. 2 driver + "simple-mfd"
> 2. 3 driver + PAIN (due to MFD required driver)
> 3. 2 driver + not very correct DT node structure

Option 4:
Describe it correctly. You have one device which is the SCU which is
clock provider and has subnode for MDIO bus. I don't care how many
drivers you have there (but I am sure one can do it in a simple way).

> 
> Maybe option 3. is more acceptable?
> 
> The SCU node is mainly clock + reset controller and the MDIO bus is an
> expansion of it?
> 
> Hope the long explaination makes sense to you (especially about the
> OF principle thing)
> 
> --
> Ansuel


Best regards,
Krzysztof

Powered by blists - more mailing lists