| Message ID | 20250220-gpio-set-retval-v2-5-bc4cfd38dae3@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | gpiolib: indicate errors in value setters | expand |
On 20.02.2025 10:57, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Add new variants of the set() and set_multiple() callbacks that have > integer return values allowing to indicate failures to users of the GPIO > consumer API. Until we convert all GPIO providers treewide to using > them, they will live in parallel to the existing ones. > > Make sure that providers cannot define both. Prefer the new ones and > only use the old ones as fallback. > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > --- > drivers/gpio/gpiolib.c | 27 +++++++++++++++++++++++++-- > include/linux/gpio/driver.h | 10 ++++++++++ > 2 files changed, 35 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index b1e7d368bc7d..19f78ffcc3c1 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -926,6 +926,11 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, > int base = 0; > int ret = 0; > > + /* Only allow one set() and one set_multiple(). */ > + if ((gc->set && gc->set_rv) || > + (gc->set_multiple && gc->set_multiple_rv)) > + return -EINVAL; > + > /* > * First: allocate and populate the internal stat container, and > * set up the struct device. > @@ -2757,11 +2762,21 @@ int gpiod_direction_input_nonotify(struct gpio_desc *desc) > > static int gpiochip_set(struct gpio_chip *gc, unsigned int offset, int value) > { > + int ret; > + > lockdep_assert_held(&gc->gpiodev->srcu); > > - if (WARN_ON(unlikely(!gc->set))) > + if (WARN_ON(unlikely(!gc->set && !gc->set_rv))) > return -EOPNOTSUPP; > > + if (gc->set_rv) { > + ret = gc->set_rv(gc, offset, value); > + if (ret > 0) > + ret = -EBADE; > + > + return ret; > + } > + > gc->set(gc, offset, value); > return 0; > } > @@ -3501,9 +3516,17 @@ static int gpiochip_set_multiple(struct gpio_chip *gc, > > lockdep_assert_held(&gc->gpiodev->srcu); > > - if (WARN_ON(unlikely(!gc->set_multiple && !gc->set))) > + if (WARN_ON(unlikely(!gc->set_multiple && !gc->set_multiple_rv))) > return -EOPNOTSUPP; The above change issues a warning on gpio controllers that doesn't support set_multiple() callbacks at all. I think that this wasn't intended. > > + if (gc->set_multiple_rv) { > + ret = gc->set_multiple_rv(gc, mask, bits); > + if (ret > 0) > + ret = -EBADE; > + > + return ret; > + } > + > if (gc->set_multiple) { > gc->set_multiple(gc, mask, bits); > return 0; > diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h > index 10544f4a03e5..b2627c8ed513 100644 > --- a/include/linux/gpio/driver.h > +++ b/include/linux/gpio/driver.h > @@ -347,6 +347,10 @@ struct gpio_irq_chip { > * stores them in "bits", returns 0 on success or negative error > * @set: assigns output value for signal "offset" > * @set_multiple: assigns output values for multiple signals defined by "mask" > + * @set_rv: assigns output value for signal "offset", returns 0 on success or > + * negative error value > + * @set_multiple_rv: assigns output values for multiple signals defined by > + * "mask", returns 0 on success or negative error value > * @set_config: optional hook for all kinds of settings. Uses the same > * packed config format as generic pinconf. > * @to_irq: optional hook supporting non-static gpiod_to_irq() mappings; > @@ -442,6 +446,12 @@ struct gpio_chip { > void (*set_multiple)(struct gpio_chip *gc, > unsigned long *mask, > unsigned long *bits); > + int (*set_rv)(struct gpio_chip *gc, > + unsigned int offset, > + int value); > + int (*set_multiple_rv)(struct gpio_chip *gc, > + unsigned long *mask, > + unsigned long *bits); > int (*set_config)(struct gpio_chip *gc, > unsigned int offset, > unsigned long config); > Best regards
On Thu, Feb 27, 2025 at 3:00 PM Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > On 20.02.2025 10:57, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > Add new variants of the set() and set_multiple() callbacks that have > > integer return values allowing to indicate failures to users of the GPIO > > consumer API. Until we convert all GPIO providers treewide to using > > them, they will live in parallel to the existing ones. > > > > Make sure that providers cannot define both. Prefer the new ones and > > only use the old ones as fallback. > > > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > --- > > lockdep_assert_held(&gc->gpiodev->srcu); > > > > - if (WARN_ON(unlikely(!gc->set_multiple && !gc->set))) > > + if (WARN_ON(unlikely(!gc->set_multiple && !gc->set_multiple_rv))) > > return -EOPNOTSUPP; > > The above change issues a warning on gpio controllers that doesn't > support set_multiple() callbacks at all. I think that this wasn't intended. > Eek, not at all, thanks for the report, I'll fix it. Bartosz
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index b1e7d368bc7d..19f78ffcc3c1 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -926,6 +926,11 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, int base = 0; int ret = 0; + /* Only allow one set() and one set_multiple(). */ + if ((gc->set && gc->set_rv) || + (gc->set_multiple && gc->set_multiple_rv)) + return -EINVAL; + /* * First: allocate and populate the internal stat container, and * set up the struct device. @@ -2757,11 +2762,21 @@ int gpiod_direction_input_nonotify(struct gpio_desc *desc) static int gpiochip_set(struct gpio_chip *gc, unsigned int offset, int value) { + int ret; + lockdep_assert_held(&gc->gpiodev->srcu); - if (WARN_ON(unlikely(!gc->set))) + if (WARN_ON(unlikely(!gc->set && !gc->set_rv))) return -EOPNOTSUPP; + if (gc->set_rv) { + ret = gc->set_rv(gc, offset, value); + if (ret > 0) + ret = -EBADE; + + return ret; + } + gc->set(gc, offset, value); return 0; } @@ -3501,9 +3516,17 @@ static int gpiochip_set_multiple(struct gpio_chip *gc, lockdep_assert_held(&gc->gpiodev->srcu); - if (WARN_ON(unlikely(!gc->set_multiple && !gc->set))) + if (WARN_ON(unlikely(!gc->set_multiple && !gc->set_multiple_rv))) return -EOPNOTSUPP; + if (gc->set_multiple_rv) { + ret = gc->set_multiple_rv(gc, mask, bits); + if (ret > 0) + ret = -EBADE; + + return ret; + } + if (gc->set_multiple) { gc->set_multiple(gc, mask, bits); return 0; diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h index 10544f4a03e5..b2627c8ed513 100644 --- a/include/linux/gpio/driver.h +++ b/include/linux/gpio/driver.h @@ -347,6 +347,10 @@ struct gpio_irq_chip { * stores them in "bits", returns 0 on success or negative error * @set: assigns output value for signal "offset" * @set_multiple: assigns output values for multiple signals defined by "mask" + * @set_rv: assigns output value for signal "offset", returns 0 on success or + * negative error value + * @set_multiple_rv: assigns output values for multiple signals defined by + * "mask", returns 0 on success or negative error value * @set_config: optional hook for all kinds of settings. Uses the same * packed config format as generic pinconf. * @to_irq: optional hook supporting non-static gpiod_to_irq() mappings; @@ -442,6 +446,12 @@ struct gpio_chip { void (*set_multiple)(struct gpio_chip *gc, unsigned long *mask, unsigned long *bits); + int (*set_rv)(struct gpio_chip *gc, + unsigned int offset, + int value); + int (*set_multiple_rv)(struct gpio_chip *gc, + unsigned long *mask, + unsigned long *bits); int (*set_config)(struct gpio_chip *gc, unsigned int offset, unsigned long config);