Re: [PATCH v5 3/5] mfd: airoha: Add support for Airoha EN7581 MFD

[Christian Marangi <ansuelsmth@xxxxxxxxx>]

On Wed, Oct 09, 2024 at 11:55:50AM +0100, Lee Jones wrote:
> On Wed, 09 Oct 2024, Lee Jones wrote:
> 
> > On Wed, 09 Oct 2024, Lorenzo Bianconi wrote:
> > 
> > > On Oct 02, Lee Jones wrote:
> > > > On Tue, 01 Oct 2024, Lorenzo Bianconi wrote:
> > > > 
> > > > > From: Christian Marangi <ansuelsmth@xxxxxxxxx>
> > > > > 
> > > > > Support for Airoha EN7581 Multi Function Device that
> > > > > expose PINCTRL functionality and PWM functionality.
> > > > 
> > > > The device is a jumble of pinctrl registers, some of which can oscillate.
> > > > 
> > > > This is *still* not an MFD.
> > > > 
> > > > If you wish to spread this functionality over 2 drivers, use syscon to
> > > > obtain the registers and simple-mfd to automatically probe the drivers.
> > > 
> > > Hi Lee,
> > > 
> > > IIUC you are suggesting two possible approaches here:
> > > 
> > > 1- have a single driver implementing both pinctrl and pwm functionalities.
> > >    This approach will not let us reuse the code for future devices that
> > >    have just one of them in common, like pwm (but we can live with that).
> > 
> > If you can have one without the other, then they are separate devices.
> > 
> > > 2- use a device node like the one below (something similar to [0])
> > > 
> > > system-controller@1fbf0200 {
> > > 	compatible = "syscon", "simple-mfd";
> > > 	reg = <0x0 0x1fbf0200 0x0 0xc0>;
> > > 
> > > 	interrupt-parent = <&gic>;
> > > 	interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
> > > 
> > > 	gpio-controller;
> > > 	#gpio-cells = <2>;
> > > 
> > > 	interrupt-controller;
> > > 	#interrupt-cells = <2>;
> > > 
> > > 	pio: pinctrl {
> > > 		compatible = "airoha,en7581-pinctrl";
> > > 
> > > 		[ some pinctrl properties here ]
> > > 	};
> > > 
> > > 	#pwm-cells = <3>;
> > > 
> > > 	pwm {
> > > 		compatible = "airoha,en7581-pwm";
> > > 	};
> > > };
> > > 
> > > Please correct me if I am wrong, but using syscon/simple-mfd as compatible
> > > string for the 'parent' device, will require to introduce the compatible strings
> > > even for the child devices in order to probe them, correct? 
> > > If so, as pointed out by Christian, this is something nacked by Rob/Krzysztof/Conor
> > > (this is the main reason why we introduced a full mfd driver here).
> > > 
> > > @Rob, Krzysztof, Conor: am I right?
> > 
> > I don't see why separate functionality shouldn't have separate
> > compatible strings, even if the registers are together.  Register layout
> > and functionality separation are not related.
> 
> We've been happy to support both pinctrl and pwm devices before:
> 
>   git grep "\-pinctrl\|\-pwm" -- drivers/mfd
>   git grep "\-pinctrl\|\-pwm" -- arch/*/boot/dts
> 
>   git grep "\-pinctrl" -- arch/*/boot/dts | wc -l
>   602
>   git grep "\-pwm" -- arch/*/boot/dts | wc -l
>   856
> 
> What makes this particular device different to all of the others?
>

Hi Lee,

this would be the final DTS following the "simple-mfd" pattern.

Can you confirm it's correct?

mfd: system-controller@1fbf0200 {
	compatible = "syscon", "simple-mfd";
	reg = <0x0 0x1fbf0200 0x0 0xc0>;
 
	interrupt-parent = <&gic>;
	interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
 
	gpio-controller;
	#gpio-cells = <2>;
 
	interrupt-controller;
	#interrupt-cells = <2>;

	gpio-ranges = <&mfd 0 13 47>;

	#pwm-cells = <3>;
 
	pio: pinctrl {
		compatible = "airoha,en7581-pinctrl";
 
		mdio_pins: mdio-pins {
			mux {
				function = "mdio";
				groups = "mdio";
			};
 
			conf {
				pins = "gpio2";
				output-high;
			};
		};
 
		pcie0_rst_pins: pcie0-rst-pins {
			conf {
				pins = "pcie_reset0";
				drive-open-drain = <1>;
			};
		};
 
		pcie1_rst_pins: pcie1-rst-pins {
			conf {
				pins = "pcie_reset1";
				drive-open-drain = <1>;
			};
		};
	};
 
	pwm {
		compatible = "airoha,en7581-pwm";
	};
};

-- 
	Ansuel