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@xxxxxxxxxx>
> > + 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:
signature.asc
Description: PGP signature
