Add request() callback to check if the GPIO descriptor was well registered
in the gpiochip_fwd before using it. This is done to handle the case where
GPIO descriptor is added at runtime in the forwarder.
If at least one GPIO descriptor was not added before the forwarder
registration, we assume the forwarder can sleep as if a GPIO is added at
runtime it may sleep.
Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
drivers/gpio/gpio-aggregator.c | 56 +++++++++++++++++++++++++++++++++++++-----
include/linux/gpio/forwarder.h | 4 +++
2 files changed, 54 insertions(+), 6 deletions(-)
diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 79fd44c2ceac..234f2f55c306 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -268,6 +268,7 @@ struct gpiochip_fwd {
spinlock_t slock; /* protects tmp[] if !can_sleep */
};
struct gpiochip_fwd_timing *delay_timings;
+ unsigned long *valid_mask;
unsigned long tmp[]; /* values and descs for multiple ops */
};
@@ -288,6 +289,21 @@ struct gpio_chip *gpio_fwd_get_gpiochip(struct gpiochip_fwd *fwd)
}
EXPORT_SYMBOL_NS_GPL(gpio_fwd_get_gpiochip, "GPIO_FORWARDER");
+/**
+ * gpio_fwd_request - Request a line of the GPIO forwarder
+ * @chip: GPIO chip in the forwarder
+ * @offset: the offset of the line to request
+ *
+ * Returns: 0 on success, or negative errno on failure.
+ */
+int gpio_fwd_request(struct gpio_chip *chip, unsigned int offset)
+{
+ struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
+
+ return test_bit(offset, fwd->valid_mask) ? 0 : -ENODEV;
+}
+EXPORT_SYMBOL_NS_GPL(gpio_fwd_request, "GPIO_FORWARDER");
+
/**
* gpio_fwd_get_direction - Return the current direction of a GPIO forwarder line
* @chip: GPIO chip in the forwarder
@@ -299,6 +315,13 @@ int gpio_fwd_get_direction(struct gpio_chip *chip, unsigned int offset)
{
struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
+ /*
+ * get_direction() is called during gpiochip registration, return input
+ * direction if there is no descriptor for the line.
+ */
+ if (!test_bit(offset, fwd->valid_mask))
+ return GPIO_LINE_DIRECTION_IN;
+
return gpiod_get_direction(fwd->descs[offset]);
}
EXPORT_SYMBOL_NS_GPL(gpio_fwd_get_direction, "GPIO_FORWARDER");
@@ -617,11 +640,16 @@ struct gpiochip_fwd *devm_gpio_fwd_alloc(struct device *dev, unsigned int ngpios
if (!fwd->descs)
return ERR_PTR(-ENOMEM);
+ fwd->valid_mask = devm_bitmap_zalloc(dev, ngpios, GFP_KERNEL);
+ if (!fwd->valid_mask)
+ return ERR_PTR(-ENOMEM);
+
chip = &fwd->chip;
chip->label = label;
chip->parent = dev;
chip->owner = THIS_MODULE;
+ chip->request = gpio_fwd_request;
chip->get_direction = gpio_fwd_get_direction;
chip->direction_input = gpio_fwd_direction_input;
chip->direction_output = gpio_fwd_direction_output;
@@ -648,24 +676,21 @@ EXPORT_SYMBOL_NS_GPL(devm_gpio_fwd_alloc, "GPIO_FORWARDER");
int gpio_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 (test_and_set_bit(offset, fwd->valid_mask))
+ return -EEXIST;
+
/*
* 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,
@@ -675,6 +700,18 @@ int gpio_fwd_gpio_add(struct gpiochip_fwd *fwd, struct gpio_desc *desc,
}
EXPORT_SYMBOL_NS_GPL(gpio_fwd_gpio_add, "GPIO_FORWARDER");
+/**
+ * gpio_fwd_gpio_free - Remove a GPIO from the forwarder
+ * @fwd: GPIO forwarder
+ * @offset: offset of GPIO to remove
+ */
+void gpio_fwd_gpio_free(struct gpiochip_fwd *fwd, unsigned int offset)
+{
+ if (test_and_clear_bit(offset, fwd->valid_mask))
+ gpiod_put(fwd->descs[offset]);
+}
+EXPORT_SYMBOL_NS_GPL(gpio_fwd_gpio_free, "GPIO_FORWARDER");
+
/**
* gpio_fwd_register - Register a GPIO forwarder
* @fwd: GPIO forwarder
@@ -685,6 +722,13 @@ int gpio_fwd_register(struct gpiochip_fwd *fwd)
{
struct gpio_chip *chip = &fwd->chip;
+ /*
+ * Some gpio_desc were not registered. They will be registered at runtime
+ * but we have to suppose they can sleep.
+ */
+ if (!bitmap_full(fwd->valid_mask, chip->ngpio))
+ chip->can_sleep = true;
+
if (chip->can_sleep)
mutex_init(&fwd->mlock);
else
diff --git a/include/linux/gpio/forwarder.h b/include/linux/gpio/forwarder.h
index b17ad2c8e031..f799b0377efd 100644
--- a/include/linux/gpio/forwarder.h
+++ b/include/linux/gpio/forwarder.h
@@ -9,6 +9,8 @@ struct gpiochip_fwd;
struct gpio_chip *gpio_fwd_get_gpiochip(struct gpiochip_fwd *fwd);
+int gpio_fwd_request(struct gpio_chip *chip, unsigned int offset);
+
int gpio_fwd_get_direction(struct gpio_chip *chip, unsigned int offset);
int gpio_fwd_direction_input(struct gpio_chip *chip, unsigned int offset);
@@ -37,6 +39,8 @@ struct gpiochip_fwd *devm_gpio_fwd_alloc(struct device *dev,
int gpio_fwd_gpio_add(struct gpiochip_fwd *fwd,
struct gpio_desc *desc, unsigned int offset);
+void gpio_fwd_gpio_free(struct gpiochip_fwd *fwd, unsigned int offset);
+
int gpio_fwd_register(struct gpiochip_fwd *fwd);
#endif
--
2.39.5Hi Thomas,
On Tue, 6 May 2025 at 17:21, Thomas Richard <thomas.richard@bootlin.com> wrote:
> Add request() callback to check if the GPIO descriptor was well registered
> in the gpiochip_fwd before using it. This is done to handle the case where
> GPIO descriptor is added at runtime in the forwarder.
>
> If at least one GPIO descriptor was not added before the forwarder
> registration, we assume the forwarder can sleep as if a GPIO is added at
> runtime it may sleep.
>
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
Thanks for your patch!
> --- a/drivers/gpio/gpio-aggregator.c
> +++ b/drivers/gpio/gpio-aggregator.c
> @@ -288,6 +289,21 @@ struct gpio_chip *gpio_fwd_get_gpiochip(struct gpiochip_fwd *fwd)
> }
> EXPORT_SYMBOL_NS_GPL(gpio_fwd_get_gpiochip, "GPIO_FORWARDER");
>
> +/**
> + * gpio_fwd_request - Request a line of the GPIO forwarder
> + * @chip: GPIO chip in the forwarder
> + * @offset: the offset of the line to request
> + *
> + * Returns: 0 on success, or negative errno on failure.
> + */
> +int gpio_fwd_request(struct gpio_chip *chip, unsigned int offset)
This function should take a gpiochip_fwd pointer instead.
> +{
> + struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
> +
> + return test_bit(offset, fwd->valid_mask) ? 0 : -ENODEV;
> +}
Not related to this patch, but just a random note:
* struct gpio_chip - abstract a GPIO controller
[...[
* @request: optional hook for chip-specific activation, such as
* enabling module power and clock; may sleep; must return 0 on success
* or negative error number on failure
* @free: optional hook for chip-specific deactivation, such as
* disabling module power and clock; may sleep
Currently all GPIOs are requested when the aggregator is created,
which means they are always powered and clocked, irrespective of a
user of the aggregator has requested them or not.
I don't see how this can be improve, except by adding suspend/resume
callbacks to gpio_chip, which could be called from the aggegator's
.free() and .request() callbacks?
> +EXPORT_SYMBOL_NS_GPL(gpio_fwd_request, "GPIO_FORWARDER");
> +
> /**
> * gpio_fwd_get_direction - Return the current direction of a GPIO forwarder line
> * @chip: GPIO chip in the forwarder
> @@ -675,6 +700,18 @@ int gpio_fwd_gpio_add(struct gpiochip_fwd *fwd, struct gpio_desc *desc,
> }
> EXPORT_SYMBOL_NS_GPL(gpio_fwd_gpio_add, "GPIO_FORWARDER");
>
> +/**
> + * gpio_fwd_gpio_free - Remove a GPIO from the forwarder
> + * @fwd: GPIO forwarder
> + * @offset: offset of GPIO to remove
> + */
> +void gpio_fwd_gpio_free(struct gpiochip_fwd *fwd, unsigned int offset)
> +{
> + if (test_and_clear_bit(offset, fwd->valid_mask))
> + gpiod_put(fwd->descs[offset]);
> +}
> +EXPORT_SYMBOL_NS_GPL(gpio_fwd_gpio_free, "GPIO_FORWARDER");
So this is _not_ the inverse of gpio_fwd_request()
(it has an extra "_gpio" in the name).
Naming is indeed tricky.
I was also wondering about gpio_fwd_get(), which does not get the
forwarder, but gets a GPIO of the forwarder...
> +
> /**
> * gpio_fwd_register - Register a GPIO forwarder
> * @fwd: GPIO forwarder
> --- a/include/linux/gpio/forwarder.h
> +++ b/include/linux/gpio/forwarder.h
> @@ -9,6 +9,8 @@ struct gpiochip_fwd;
>
> struct gpio_chip *gpio_fwd_get_gpiochip(struct gpiochip_fwd *fwd);
>
> +int gpio_fwd_request(struct gpio_chip *chip, unsigned int offset);
> +
> int gpio_fwd_get_direction(struct gpio_chip *chip, unsigned int offset);
>
> int gpio_fwd_direction_input(struct gpio_chip *chip, unsigned int offset);
> @@ -37,6 +39,8 @@ struct gpiochip_fwd *devm_gpio_fwd_alloc(struct device *dev,
> int gpio_fwd_gpio_add(struct gpiochip_fwd *fwd,
> struct gpio_desc *desc, unsigned int offset);
>
> +void gpio_fwd_gpio_free(struct gpiochip_fwd *fwd, unsigned int offset);
> +
> int gpio_fwd_register(struct gpiochip_fwd *fwd);
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:
>
> Add request() callback to check if the GPIO descriptor was well registered
> in the gpiochip_fwd before using it. This is done to handle the case where
> GPIO descriptor is added at runtime in the forwarder.
>
> If at least one GPIO descriptor was not added before the forwarder
> registration, we assume the forwarder can sleep as if a GPIO is added at
> runtime it may sleep.
...
> {
> struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
>
> + /*
> + * get_direction() is called during gpiochip registration, return input
> + * direction if there is no descriptor for the line.
> + */
> + if (!test_bit(offset, fwd->valid_mask))
> + return GPIO_LINE_DIRECTION_IN;
Can you remind me why we choose a valid return for invalid line? From
a pure code perspective this should return an error.
> return gpiod_get_direction(fwd->descs[offset]);
> }
...
> + /*
> + * Some gpio_desc were not registered. They will be registered at runtime
> + * but we have to suppose they can sleep.
suppose --> assume ?
> + */
> + if (!bitmap_full(fwd->valid_mask, chip->ngpio))
> + chip->can_sleep = true;
--
With Best Regards,
Andy Shevchenko
Hi Andy,
Thanks again for the review.
On 5/7/25 08:34, Andy Shevchenko wrote:
> On Tue, May 6, 2025 at 6:21 PM Thomas Richard
> <thomas.richard@bootlin.com> wrote:
>>
>> Add request() callback to check if the GPIO descriptor was well registered
>> in the gpiochip_fwd before using it. This is done to handle the case where
>> GPIO descriptor is added at runtime in the forwarder.
>>
>> If at least one GPIO descriptor was not added before the forwarder
>> registration, we assume the forwarder can sleep as if a GPIO is added at
>> runtime it may sleep.
>
> ...
>
>> {
>> struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
>>
>> + /*
>> + * get_direction() is called during gpiochip registration, return input
>> + * direction if there is no descriptor for the line.
>> + */
>> + if (!test_bit(offset, fwd->valid_mask))
>> + return GPIO_LINE_DIRECTION_IN;
>
> Can you remind me why we choose a valid return for invalid line? From
> a pure code perspective this should return an error.
I reproduced gpiolib behavior. During gpiochip registration, we get the
direction of all lines. In the case the line is not valid, it is marked
as input if direction_input operation exists, otherwise it is marked as
output. [1]
But in fact we could return an error and the core will mark the line as
input. Maybe ENODEV ?
[1]
https://elixir.bootlin.com/linux/v6.15-rc5/source/drivers/gpio/gpiolib.c#L1105-L1123
Regards,
Thomas
On Wed, May 7, 2025 at 1:10 PM Thomas Richard <thomas.richard@bootlin.com> wrote: > On 5/7/25 08:34, Andy Shevchenko wrote: > > On Tue, May 6, 2025 at 6:21 PM Thomas Richard > > <thomas.richard@bootlin.com> wrote: ... > >> + /* > >> + * get_direction() is called during gpiochip registration, return input > >> + * direction if there is no descriptor for the line. > >> + */ > >> + if (!test_bit(offset, fwd->valid_mask)) > >> + return GPIO_LINE_DIRECTION_IN; > > > > Can you remind me why we choose a valid return for invalid line? From > > a pure code perspective this should return an error. > > I reproduced gpiolib behavior. During gpiochip registration, we get the > direction of all lines. In the case the line is not valid, it is marked > as input if direction_input operation exists, otherwise it is marked as > output. [1] > > But in fact we could return an error and the core will mark the line as > input. Maybe ENODEV ? I am fine with this error code, but do we have similar cases already in the kernel? Do they use the same or different error code(s)? > [1] > https://elixir.bootlin.com/linux/v6.15-rc5/source/drivers/gpio/gpiolib.c#L1105-L1123 -- With Best Regards, Andy Shevchenko
On 5/7/25 15:24, Andy Shevchenko wrote: > On Wed, May 7, 2025 at 1:10 PM Thomas Richard > <thomas.richard@bootlin.com> wrote: >> On 5/7/25 08:34, Andy Shevchenko wrote: >>> On Tue, May 6, 2025 at 6:21 PM Thomas Richard >>> <thomas.richard@bootlin.com> wrote: > > ... > >>>> + /* >>>> + * get_direction() is called during gpiochip registration, return input >>>> + * direction if there is no descriptor for the line. >>>> + */ >>>> + if (!test_bit(offset, fwd->valid_mask)) >>>> + return GPIO_LINE_DIRECTION_IN; >>> >>> Can you remind me why we choose a valid return for invalid line? From >>> a pure code perspective this should return an error. >> >> I reproduced gpiolib behavior. During gpiochip registration, we get the >> direction of all lines. In the case the line is not valid, it is marked >> as input if direction_input operation exists, otherwise it is marked as >> output. [1] >> >> But in fact we could return an error and the core will mark the line as >> input. Maybe ENODEV ? > > I am fine with this error code, but do we have similar cases already > in the kernel? Do they use the same or different error code(s)? I dumped all get_direction() operations in drivers/gpio and drivers/pinctrl and returned values are: - GPIO_LINE_DIRECTION_OUT and GPIO_LINE_DIRECTION_IN (make sense). - -EINVAL (for example [1]). - -EBADE in gpiochip_get_direction() [2]. - regmap_read() return code. But from my point of view -EINVAL and -EBADE do not match our case. [1] https://elixir.bootlin.com/linux/v6.15-rc5/source/drivers/gpio/gpio-cros-ec.c#L70 [2] https://elixir.bootlin.com/linux/v6.15-rc5/source/drivers/gpio/gpiolib.c#L359 Regards, Thomas
On Wed, May 7, 2025 at 4:54 PM Thomas Richard <thomas.richard@bootlin.com> wrote: > On 5/7/25 15:24, Andy Shevchenko wrote: > > On Wed, May 7, 2025 at 1:10 PM Thomas Richard > > <thomas.richard@bootlin.com> wrote: > >> On 5/7/25 08:34, Andy Shevchenko wrote: > >>> On Tue, May 6, 2025 at 6:21 PM Thomas Richard > >>> <thomas.richard@bootlin.com> wrote: ... > >>>> + /* > >>>> + * get_direction() is called during gpiochip registration, return input > >>>> + * direction if there is no descriptor for the line. > >>>> + */ > >>>> + if (!test_bit(offset, fwd->valid_mask)) > >>>> + return GPIO_LINE_DIRECTION_IN; > >>> > >>> Can you remind me why we choose a valid return for invalid line? From > >>> a pure code perspective this should return an error. > >> > >> I reproduced gpiolib behavior. During gpiochip registration, we get the > >> direction of all lines. In the case the line is not valid, it is marked > >> as input if direction_input operation exists, otherwise it is marked as > >> output. [1] > >> > >> But in fact we could return an error and the core will mark the line as > >> input. Maybe ENODEV ? > > > > I am fine with this error code, but do we have similar cases already > > in the kernel? Do they use the same or different error code(s)? > > I dumped all get_direction() operations in drivers/gpio and > drivers/pinctrl and returned values are: > - GPIO_LINE_DIRECTION_OUT and GPIO_LINE_DIRECTION_IN (make sense). > - -EINVAL (for example [1]). > - -EBADE in gpiochip_get_direction() [2]. > - regmap_read() return code. > > But from my point of view -EINVAL and -EBADE do not match our case. Hmm... I believe we need a GPIO maintainer to have a look at this. -- With Best Regards, Andy Shevchenko
On Wed, May 7, 2025 at 5:24 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Wed, May 7, 2025 at 4:54 PM Thomas Richard > <thomas.richard@bootlin.com> wrote: > > On 5/7/25 15:24, Andy Shevchenko wrote: > > > On Wed, May 7, 2025 at 1:10 PM Thomas Richard > > > <thomas.richard@bootlin.com> wrote: > > >> On 5/7/25 08:34, Andy Shevchenko wrote: > > >>> On Tue, May 6, 2025 at 6:21 PM Thomas Richard > > >>> <thomas.richard@bootlin.com> wrote: > > ... > > > >>>> + /* > > >>>> + * get_direction() is called during gpiochip registration, return input > > >>>> + * direction if there is no descriptor for the line. > > >>>> + */ > > >>>> + if (!test_bit(offset, fwd->valid_mask)) > > >>>> + return GPIO_LINE_DIRECTION_IN; > > >>> > > >>> Can you remind me why we choose a valid return for invalid line? From > > >>> a pure code perspective this should return an error. > > >> > > >> I reproduced gpiolib behavior. During gpiochip registration, we get the > > >> direction of all lines. In the case the line is not valid, it is marked > > >> as input if direction_input operation exists, otherwise it is marked as > > >> output. [1] > > >> > > >> But in fact we could return an error and the core will mark the line as > > >> input. Maybe ENODEV ? > > > > > > I am fine with this error code, but do we have similar cases already > > > in the kernel? Do they use the same or different error code(s)? > > > > I dumped all get_direction() operations in drivers/gpio and > > drivers/pinctrl and returned values are: > > - GPIO_LINE_DIRECTION_OUT and GPIO_LINE_DIRECTION_IN (make sense). > > - -EINVAL (for example [1]). > > - -EBADE in gpiochip_get_direction() [2]. > > - regmap_read() return code. > > > > But from my point of view -EINVAL and -EBADE do not match our case. > > Hmm... I believe we need a GPIO maintainer to have a look at this. > I went with -EBADE in GPIO core to indicate that the underlying driver borked and returned an invalid value. I'm not sure if this is the right one here. I'm not against using -ENODEV. Bart
© 2016 - 2025 Red Hat, Inc.