Re: [PATCH v5 05/12] gpio: aggregator: refactor the code to add GPIO desc in the forwarder

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Thomas,

Thanks for your patch!

On Tue, 6 May 2025 at 17:21, Thomas Richard <thomas.richard@bootlin.com> wrote:
> Create a dedicated function to add a GPIO desc in the forwarder. Instead of
> passing an array of GPIO descs, now the GPIO descs are passed on by one to

one by one

> the forwarder.

Also, the passed array is no longer stored as-is, but copied.

>
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>

> --- a/drivers/gpio/gpio-aggregator.c
> +++ b/drivers/gpio/gpio-aggregator.c
> @@ -509,6 +509,10 @@ devm_gpiochip_fwd_alloc(struct device *dev, unsigned int ngpios)
>         if (!fwd)
>                 return ERR_PTR(-ENOMEM);
>
> +       fwd->descs = devm_kcalloc(dev, ngpios, sizeof(*fwd->descs), GFP_KERNEL);
> +       if (!fwd->descs)
> +               return ERR_PTR(-ENOMEM);
> +
>         chip = &fwd->chip;
>
>         chip->label = label;
> @@ -528,6 +532,36 @@ devm_gpiochip_fwd_alloc(struct device *dev, unsigned int ngpios)
>         return fwd;
>  }
>
> +static int gpiochip_fwd_gpio_add(struct gpiochip_fwd *fwd,
> +                                struct gpio_desc *desc,
> +                                unsigned int offset)
> +{
> +       struct gpio_chip *parent = gpiod_to_chip(desc);
> +       struct gpio_chip *chip = &fwd->chip;
> +
> +       if (offset > chip->ngpio)

>=

> +               return -EINVAL;
> +
> +       /*
> +        * If any of the GPIO lines are sleeping, then the entire forwarder
> +        * will be sleeping.
> +        * If any of the chips support .set_config(), then the forwarder will
> +        * support setting configs.
> +        */
> +       if (gpiod_cansleep(desc))
> +               chip->can_sleep = true;
> +
> +       if (parent && parent->set_config)
> +               chip->set_config = gpio_fwd_set_config;
> +
> +       fwd->descs[offset] = desc;
> +
> +       dev_dbg(chip->parent, "%u => gpio %d irq %d\n", offset,
> +               desc_to_gpio(desc), gpiod_to_irq(desc));
> +
> +       return 0;
> +}
> +
>  /**
>   * gpiochip_fwd_create() - Create a new GPIO forwarder
>   * @dev: Parent device pointer
> @@ -559,26 +593,12 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
>
>         chip = &fwd->chip;
>
> -       /*
> -        * If any of the GPIO lines are sleeping, then the entire forwarder
> -        * will be sleeping.
> -        * If any of the chips support .set_config(), then the forwarder will
> -        * support setting configs.
> -        */
>         for (i = 0; i < ngpios; i++) {
> -               struct gpio_chip *parent = gpiod_to_chip(descs[i]);
> -
> -               dev_dbg(dev, "%u => gpio %d irq %d\n", i,
> -                       desc_to_gpio(descs[i]), gpiod_to_irq(descs[i]));
> -
> -               if (gpiod_cansleep(descs[i]))
> -                       chip->can_sleep = true;
> -               if (parent && parent->set_config)
> -                       chip->set_config = gpio_fwd_set_config;
> +               error = gpiochip_fwd_gpio_add(fwd, descs[i], i);
> +               if (error)
> +                       return ERR_PTR(error);
>         }
>
> -       fwd->descs = descs;

So the passed array is no longer stored, and thus the caller
(gpio_aggregator_probe()) can free it after the call.

> -
>         if (chip->can_sleep)
>                 mutex_init(&fwd->mlock);
>         else
>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds