Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <6a62a531227cd4f20d77d50cdde60c7a18b9f052.1643625325.git.geert+renesas@glider.be>
In-Reply-To: <6a62a531227cd4f20d77d50cdde60c7a18b9f052.1643625325.git.geert+renesas@glider.be>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Tue, 1 Feb 2022 22:33:25 +0200
Message-ID: <CAHp75Vd68gsU-NWTGv4Y7Mo4-Vq7DBePa_yVxpDq=DAN5GLEBA@mail.gmail.com>
Subject: Re: [PATCH] gpio: aggregator: Fix calling into sleeping GPIO controllers
To:     Geert Uytterhoeven <geert+renesas@glider.be>
Cc:     Linus Walleij <linus.walleij@linaro.org>,
        Bartosz Golaszewski <brgl@bgdev.pl>,
        =?UTF-8?Q?Mikko_Salom=C3=A4ki?= <ms@datarespons.se>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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=C3=A4ki <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-aggregato=
r.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, unsig=
ned int offset)
>  {
>         struct gpiochip_fwd *fwd =3D gpiochip_get_data(chip);
>
> -       return gpiod_get_value(fwd->descs[offset]);

> +       return chip->can_sleep ? gpiod_get_value_cansleep(fwd->descs[offs=
et])
> +                              : 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 =3D 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++] =3D fwd->descs[i];
>
> -       error =3D gpiod_get_array_value(j, descs, NULL, values);
> +       if (fwd->chip.can_sleep)
> +               error =3D gpiod_get_array_value_cansleep(j, descs, NULL, =
values);
> +       else
> +               error =3D 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, uns=
igned int offset, int value)
>  {
>         struct gpiochip_fwd *fwd =3D 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 lon=
g *mask,
> @@ -343,7 +350,10 @@ static void gpio_fwd_set_multiple(struct gpiochip_fw=
d *fwd, unsigned long *mask,
>                 descs[j++] =3D 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
>


--=20
With Best Regards,
Andy Shevchenko