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

[Lorenzo Bianconi <lorenzo@kernel.org>]

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

> On Thu, Oct 10, 2024 at 12:14 PM Christian Marangi <ansuelsmth@gmail.com> wrote:
> 
> > mfd: system-controller@1fbf0200 {
> 
> Drop the mfd: thing, you probably don't want to reference the syscon
> node directly
> in the device tree. If you still give it a label just say
> en7581_syscon: system-controller...

ack, I am fine with it.

> 
> >         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>;
> 
> I think you want a separate GPIO node inside the system controller:
> 
>   en7581_gpio: gpio {
>          compatible = "airhoa,en7581-gpio";
>          interrupt-parent = <&gic>;
>          interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
> 
>          gpio-controller;
>          #gpio-cells = <2>;
> 
>          interrupt-controller;
>          #interrupt-cells = <2>;
> 
>          gpio-ranges = <&en7581_pinctrl 0 13 47>;
> };

So far I implemented the gpio functionalities in the en7581 pinctrl driver
(as it is done for other mtk pinctrl drivers) but I am fine to reuse the
gpio-en7523 driver for it. Do you prefer this second approach?

> 
> So users pick GPIOs:
> 
> foo-gpios = <&en7581_gpio ...>;
> 
> Notice that the gpio-ranges should refer to the pin controller
> node.
> 
> >
> >         #pwm-cells = <3>;
> 
> Shouldn't this be inside the pwm node?
> 
>          en7581_pwm: pwm {
>                  compatible = "airoha,en7581-pwm";
>                  #pwm-cells = <3>;
>          };
> 
> So PWM users can pick a PWM with pwms = <&en7581_pwm ...>;

ack, I am fine with it.

> 
> >         pio: pinctrl {
> 
> I would use the label en7581_pinctrl:

ack, I am fine with it.

> 
> >                 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";
> >         };
> > };
> 
> This will make subdevices probe and you can put the pure GPIO
> driver in drivers/gpio/gpio-en7581.c

We could actually reuse gpio-en7523 driver (removing the gpio part from en7581
pinctrl driver) and extend it to support irq_chip. I do not have a strong
opinion about it.

Regards,
Lorenzo

> 
> Yours,
> Linus Walleij

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