Re: [PATCH v2 05/15] gpiolib: introduce gpio_chip setters that return values

[Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>]

On 20.02.2025 10:57, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> ---
>   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
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland