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

[Lorenzo Bianconi <lorenzo@kernel.org>]

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

On Aug 22, Conor Dooley wrote:
> On Thu, Aug 22, 2024 at 11:40:52AM +0200, Lorenzo Bianconi wrote:
> > Introduce device-tree binding documentation for Airoha EN7581 pinctrl
> > controller.
> > 
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > +  reg:
> > +    items:
> > +      - description: IOMUX base address
> > +      - description: LED IOMUX base address
> > +      - description: GPIO flash mode base address
> > +      - description: GPIO flash mode extended base address
> > +      - description: IO pin configuration base address
> > +      - description: PCIE reset open-drain base address
> > +      - description: GPIO bank0 register base address
> > +      - description: GPIO bank0 second control register base address
> > +      - description: GPIO bank1 second control register base address
> > +      - description: GPIO bank1 register base address
> 
> > +      pinctrl@1fa20214 {
> > +        compatible = "airoha,en7581-pinctrl";
> > +        reg = <0x0 0x1fa20214 0x0 0x30>,
> > +              <0x0 0x1fa2027c 0x0 0x8>,
> > +              <0x0 0x1fbf0234 0x0 0x4>,
> > +              <0x0 0x1fbf0268 0x0 0x4>,
> > +              <0x0 0x1fa2001c 0x0 0x50>,
> > +              <0x0 0x1fa2018c 0x0 0x4>,
> > +              <0x0 0x1fbf0200 0x0 0x18>,
> > +              <0x0 0x1fbf0220 0x0 0x4>,
> > +              <0x0 0x1fbf0260 0x0 0x8>,
> > +              <0x0 0x1fbf0270 0x0 0x28>;
> > +        reg-names = "iomux", "led-iomux",
> > +                    "gpio-flash-mode", "gpio-flash-mode-ext",
> > +                    "ioconf", "pcie-rst-od",
> > +                    "gpio-bank0", "gpio-ctrl1",
> > +                    "gpio-ctrl2", "gpio-bank1";
> 
> before looking at v1:
> I would really like to see an explanation for why this is a correct
> model of the hardware as part of the commit message. To me this screams
> syscon/MFD and instead of describing this as a child of a syscon and
> using regmap to access it you're doing whatever this is...
> 
> after looking at v1:
> AFAICT the PWM driver does not currently exist in mainline, so I am now
> doubly of the opinion that this needs to be an MFD and a wee bit annoyed
> that you didn't include any rationale in your cover letter or w/e for
> not going with an MFD given there was discussion on the topic in v1.

based on the reply from Rob I was thinking it is fine to just reduce the number
of IO mappings, sorry for that.

> 
> Thanks,
> Conor.

clock, pinctrl and pwm controllers need to map 3 main memory areas:
- chip-scu: <0x0 0x1fa20000 0x0 0x384>
  it is used by the clock driver for fixed freq clock configuration,
  by the pinctrl driver for io-muxing (and by the future pon drivers)
- scu: <0x1fb00020 0x0 0x94c>
  it is used by the clock/rst driver
- gpio: <0x1fbf0200 0x0 0xbc>
  it is used by the pinctrl driver to implement gpio/irq controller and
  by the pwm driver.

I guess we can model chip_scu as single syscon node used by clock and pinctrl
while pwm can be a child of the pinctrl node. Something like:

...

chip_scu: chip-scu@1fa20000 {
	compatible = "airoha,en7581-chip-scu", "syscon";
	reg = <0x0 0x1fa20000 0x0 0x384>;
};

scuclk: clock-controller@1fb00020 {
	compatible = "airoha,en7581-scu";
	reg = <0x1fb00020 0x0 0x94c>;
	airoha,chip-scu = <&chip_scu>;
	...
};

pio: pinctrl@1fbf0200 {
	compatible = "airoha,en7581-pinctrl", "simple-mfd", "syscon";
	reg = <0x1fbf0200 0x0 0xbc>;
	airoha,chip-scu = <&chip_scu>;
	....

	pwm {
		compatible = "airoha,en7581-pwm";
		...
	};
};

...

Does it work for you?

Regards,
Lorenzo

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