[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAHp75Vd68gsU-NWTGv4Y7Mo4-Vq7DBePa_yVxpDq=DAN5GLEBA@mail.gmail.com>
Date: Tue, 1 Feb 2022 22:33:25 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Geert Uytterhoeven <geert+renesas@...der.be>
Cc: Linus Walleij <linus.walleij@...aro.org>,
Bartosz Golaszewski <brgl@...ev.pl>,
Mikko Salomäki <ms@...arespons.se>,
"open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] gpio: aggregator: Fix calling into sleeping GPIO controllers
On Tue, Feb 1, 2022 at 10:09 PM Geert Uytterhoeven
<geert+renesas@...der.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@...il.com>
One nit-pick below, though.
> Reported-by: Mikko Salomäki <ms@...arespons.se>
> Fixes: 828546e24280f721 ("gpio: Add GPIO Aggregator")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@...der.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
Powered by blists - more mailing lists