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
