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
the forwarder.
Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
drivers/gpio/gpio-aggregator.c | 54 +++++++++++++++++++++++++++++-------------
1 file changed, 37 insertions(+), 17 deletions(-)
diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 4268ef94914d..7ca6f1953938 100644
--- 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;
-
if (chip->can_sleep)
mutex_init(&fwd->mlock);
else
--
2.39.5Hi 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
On Fri, 9 May 2025 at 10:38, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> 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>
> > /**
> > * gpiochip_fwd_create() - Create a new GPIO forwarder
> > * @dev: Parent device pointer
* @ngpios: Number of GPIOs in the forwarder.
* @descs: Array containing the GPIO descriptors to forward to.
* This array must contain @ngpios entries, and must not
be deallocated
* before the forwarder has been destroyed again.
This needs an update, as the array is copied.
> > @@ -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
On Tue, May 6, 2025 at 6:21 PM 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 > the forwarder. Reviewed-by: Andy Shevchenko <andy@kernel.org> -- With Best Regards, Andy Shevchenko
© 2016 - 2025 Red Hat, Inc.