Re: [PATCH 4/5] dt-bindings: clock: airoha: Document support for AN7583 clock

[Krzysztof Kozlowski <krzk@kernel.org>]

On 28/05/2025 10:54, Christian Marangi wrote:
> On Wed, May 28, 2025 at 09:30:37AM +0200, Krzysztof Kozlowski wrote:
>> On 28/05/2025 02:49, Christian Marangi wrote:
>>>    - if:
>>>        properties:
>>>          compatible:
>>> @@ -75,6 +78,17 @@ allOf:
>>>          reg:
>>>            maxItems: 1
>>>  
>>> +      required:
>>> +        - reg
>>> +
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          const: airoha,an7583-clock
>>> +    then:
>>> +      properties:
>>> +        reg: false
>>
>>
>> No resources here, so this should be part of parent node.
>>
> 
> Ok hope you can help here. This is another case of "MFD" thing.
> 
> I was with the idea that it was O.K. to use this with very different
> devices. (current scenario Clock controller and MDIO controller)
> 
> The node structure I had in mind was
> 
> 		system-controller@1fa20000 {
> 			compatible = "airoha,an7583-scu", "syscon", "simple-mfd";
> 			reg = <0x0 0x1fb00000 0x0 0x970>;
> 
> 			scuclk: scuclk {
> 				compatible = "airoha,an7583-clock";
> 				#clock-cells = <1>;
> 				#reset-cells = <1>;
> 			};
> 
> 			mdio {
> 				compatible = "airoha,an7583-mdio";
> 				#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 you want
> 
> system-controller@1fa20000 {
>         compatible = "airoha,an7583-scu", "syscon";
>         reg = <0x0 0x1fb00000 0x0 0x970>;
> 
>         #clock-cells = <1>;
>         #reset-cells = <1>;
> 

mdio could be here just to group the bus (it's pretty common I think),
although not sure if compatible is useful then.

>         mdio_0: bus@0 {
>                 reg = <0>;
>                 resets = <&scuclk AN7583_MDIO0>;
>         };
> 
>         mdio_1: bus@1 {
>                 reg = <1>;
>                 resets = <&scuclk AN7583_MDIO1>;
>         };
> };
> 
> 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?

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

> 


Best regards,
Krzysztof