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

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Thu, Oct 10, 2024 at 12:14 PM Christian Marangi <ansuelsmth@xxxxxxxxx> 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...

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

>         pio: pinctrl {

I would use the label en7581_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";
>         };
> };

This will make subdevices probe and you can put the pure GPIO
driver in drivers/gpio/gpio-en7581.c

Yours,
Linus Walleij