Re: [PATCH v2 1/2] dt-bindings: pinctrl: airoha: Add EN7581 pinctrl controller
From: Lorenzo Bianconi <lorenzo@kernel.org>
Date: 2024-08-30 10:55:35
Also in:
linux-arm-kernel, linux-gpio, linux-mediatek
[...]
quoted
quoted
quoted
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 theSorry, 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.quoted
This is similar to how it's done by broadcom GPIO MFD [1] that alsoThere 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.quoted
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";
};
};
quoted
}; pwm: pwm { compatible = "airoha,en7581-pwm"; #pwm-cells = <3>; status = "disabled";And why is it disabled? No external resources. There is no benefit of this node.
This is just a copy-paster error. Regards, Lorenzo
quoted
}; }; I also link the implementation of the MFD driver [2] [1] https://elixir.bootlin.com/linux/v6.10.7/source/Documentation/devicetree/bindings/mfd/brcm,bcm6318-gpio-sysctl.yaml [2] https://github.com/Ansuel/linux/blob/airoha-mfd/drivers/mfd/airoha-en7581-gpio-mfd.cBest regards, Krzysztof
Attachments
- signature.asc [application/pgp-signature] 228 bytes