Re: [PATCH] gpio: aggregator: Fix calling into sleeping GPIO controllers

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Tue, Feb 1, 2022 at 10:09 PM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
>
> If the parent GPIO controller is a sleeping controller (e.g. a GPIO
> controller connected to I2C), getting or setting a GPIO triggers a
> might_sleep() warning.  This happens because the GPIO Aggregator takes
> the can_sleep flag into account only for its internal locking, not for
> calling into the parent GPIO controller.
>
> Fix this by using the gpiod_[gs]et*_cansleep() APIs when calling into a
> sleeping GPIO controller.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
One nit-pick below, though.

> Reported-by: Mikko Salomäki <ms@datarespons.se>
> Fixes: 828546e24280f721 ("gpio: Add GPIO Aggregator")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> I considered splitting the .[gs]et{_multiple}() callbacks for the
> sleeping vs. nonsleeping cases, but the code size increase (measured on
> ARM) would be substantial:
>   +64 bytes for gpio_fwd_[gs]et_cansleep(),
>   +296 bytes for gpio_fwd_[gs]et_multiple_cansleep().
> ---
>  drivers/gpio/gpio-aggregator.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
> index 869dc952cf45218b..0cb2664085cf8314 100644
> --- a/drivers/gpio/gpio-aggregator.c
> +++ b/drivers/gpio/gpio-aggregator.c
> @@ -278,7 +278,8 @@ static int gpio_fwd_get(struct gpio_chip *chip, unsigned int offset)
>  {
>         struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
>
> -       return gpiod_get_value(fwd->descs[offset]);

> +       return chip->can_sleep ? gpiod_get_value_cansleep(fwd->descs[offset])
> +                              : gpiod_get_value(fwd->descs[offset]);

This indentation kills the perfectionist in me :-)
What about:

       return chip->can_sleep ?
               gpiod_get_value_cansleep(fwd->descs[offset]) :
gpiod_get_value(fwd->descs[offset]);

?

Or as variant

       struct gpio_desc *desc = fwd->descs[offset];

       return chip->can_sleep ? gpiod_get_value_cansleep(desc) :
gpiod_get_value(desc);

?

>  }
>
>  static int gpio_fwd_get_multiple(struct gpiochip_fwd *fwd, unsigned long *mask,
> @@ -293,7 +294,10 @@ static int gpio_fwd_get_multiple(struct gpiochip_fwd *fwd, unsigned long *mask,
>         for_each_set_bit(i, mask, fwd->chip.ngpio)
>                 descs[j++] = fwd->descs[i];
>
> -       error = gpiod_get_array_value(j, descs, NULL, values);
> +       if (fwd->chip.can_sleep)
> +               error = gpiod_get_array_value_cansleep(j, descs, NULL, values);
> +       else
> +               error = gpiod_get_array_value(j, descs, NULL, values);
>         if (error)
>                 return error;
>
> @@ -328,7 +332,10 @@ static void gpio_fwd_set(struct gpio_chip *chip, unsigned int offset, int value)
>  {
>         struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
>
> -       gpiod_set_value(fwd->descs[offset], value);
> +       if (chip->can_sleep)
> +               gpiod_set_value_cansleep(fwd->descs[offset], value);
> +       else
> +               gpiod_set_value(fwd->descs[offset], value);
>  }
>
>  static void gpio_fwd_set_multiple(struct gpiochip_fwd *fwd, unsigned long *mask,
> @@ -343,7 +350,10 @@ static void gpio_fwd_set_multiple(struct gpiochip_fwd *fwd, unsigned long *mask,
>                 descs[j++] = fwd->descs[i];
>         }
>
> -       gpiod_set_array_value(j, descs, NULL, values);
> +       if (fwd->chip.can_sleep)
> +               gpiod_set_array_value_cansleep(j, descs, NULL, values);
> +       else
> +               gpiod_set_array_value(j, descs, NULL, values);
>  }
>
>  static void gpio_fwd_set_multiple_locked(struct gpio_chip *chip,
> --
> 2.25.1
>


-- 
With Best Regards,
Andy Shevchenko