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

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3381 bytes --]

[...]

> >>>
> >>> 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";
			};
		};

> 
> >                         };
> > 
> >                         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

> 
> >                         };
> >                 };
> > 
> > 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.c
> 
> 
> Best regards,
> Krzysztof
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]