Re: [PATCH v2 1/2] dt-bindings: pinctrl: airoha: Add EN7581 pinctrl controller

[Krzysztof Kozlowski <krzk@kernel.org>]

On 30/08/2024 13:01, Conor Dooley wrote:
> On Fri, Aug 30, 2024 at 12:55:32PM +0200, Lorenzo Bianconi wrote:
>> [...]
>>
>>>>>>
>>>>>> Hi Rob, thanks a lot for the hint, I hope we can finally find a solution
>>>>>> on how to implement this.
>>>>>>
>>>>>> In Documentation the block is called GPIO Controller. As explained it does
>>>>>> expose pinctrl function AND pwm (with regs in the middle)
>>>>>>
>>>>>> Is this semplification really needed? It does pose some problem driver
>>>>>> wise (on where to put the driver, in what subsystem) and also on the
>>>>>
>>>>> Sorry, but no, dt-bindings do not affect the driver at all in such way.
>>>>> Nothing changes in your driver in such aspect, no dilemma where to put
>>>>> it (the same place as before).
>>>>>
>>>>
>>>> Ok, from the proposed node structure, is it problematic to move the
>>>> gpio-controller and -cells in the pinctrl node? And also the pwm-cells
>>>> to the pwm node?
>>>
>>> The move is just unnecessary and not neat. You design DTS based on your
>>> drivers architecture and this is exactly what we want to avoid.
>>>
>>>> This is similar to how it's done by broadcom GPIO MFD [1] that also
>>>
>>> There are 'reg' fields, which is the main problem here. I don't like
>>> that arguments because it entirely misses the discussions - about that
>>> binding or other bindings - happening prior to merge.
>>>
>>>> expose pinctrl and other device in the same register block as MFD
>>>> childs.
>>>>
>>>> This would be the final node block.
>>>>
>>>>                 mfd@1fbf0200 {
>>>>                         compatible = "airoha,en7581-gpio-mfd";
>>>>                         reg = <0x0 0x1fbf0200 0x0 0xc0>;
>>>>
>>>>                         interrupt-parent = <&gic>;
>>>>                         interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
>>>>
>>>>                         pio: pinctrl {
>>>>                                 compatible = "airoha,en7581-pinctrl";
>>>>
>>>>                                 gpio-controller;
>>>>                                 #gpio-cells = <2>;
>>>>
>>>>                                 interrupt-controller;
>>>>                                 #interrupt-cells = <2>;
>>>
>>> No resources here...
>>
>> ack. iiuc, all the properties will be in the parent node (mfd) and we will
>> have just the compatible strings in the child ones, right? Something like:
>>
>> 		mfd@1fbf0200 {
>> 			compatible = "airoha,en7581-gpio-mfd";
>> 			reg = <0x0 0x1fbf0200 0x0 0xc0>;
>> 			gpio-controller;
>> 			#gpio-cells = <2>;
>>
>> 			...
>> 			#pwm-cells = <3>;
>>
>> 			pio: pinctrl {
>> 				compatible = "airoha,en7581-pinctrl";
>> 			};
>>
>> 			pwm: pwm {
>> 				compatible = "airoha,en7581-pwm";
>> 			};
>> 		};
> 
> 
> Didn't Rob basically tell you how to do it earlier in the thread?
> What you've got now makes no sense, the compatibles only exist in that
> to probe drivers, which you can do from the mfd driver with
> mfd_add_devices() or w/e that function is called.

Yep, we are making circles.

I will repeat:
"The move is just unnecessary and not neat. You design DTS based on your
drivers architecture and this is exactly what we want to avoid."

Best regards,
Krzysztof