[RFT,15/21] arm: omap1: ams-delta: stop using gpiochip_find()

Commit Message

Bartosz Golaszewski Sept. 5, 2023, 6:53 p.m. UTC

From: Bartosz Golaszewski <[e-mail]>

gpiochip_find() is going away as it's not hot-unplug safe. This platform
is not affected by any of the related problems as this GPIO controller
cannot really go away but in order to finally remove this function, we
need to convert it to using gpio_device_find() as well.

Signed-off-by: Bartosz Golaszewski <[e-mail]>
---
 arch/arm/mach-omap1/board-ams-delta.c | 36 +++++++++++++--------------
 1 file changed, 17 insertions(+), 19 deletions(-)

Comments

Andy Shevchenko Sept. 6, 2023, 2:48 p.m. UTC | #1

On Tue, Sep 05, 2023 at 08:53:03PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[e-mail]>
> 
> gpiochip_find() is going away as it's not hot-unplug safe. This platform
> is not affected by any of the related problems as this GPIO controller
> cannot really go away but in order to finally remove this function, we
> need to convert it to using gpio_device_find() as well.

Side question, have you used --patience when preparing this series?

Bartosz Golaszewski Sept. 6, 2023, 2:56 p.m. UTC | #2

On Wed, Sep 6, 2023 at 4:50 PM Andy Shevchenko
<[e-mail]> wrote:
>
> On Tue, Sep 05, 2023 at 08:53:03PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <[e-mail]>
> >
> > gpiochip_find() is going away as it's not hot-unplug safe. This platform
> > is not affected by any of the related problems as this GPIO controller
> > cannot really go away but in order to finally remove this function, we
> > need to convert it to using gpio_device_find() as well.
>
> Side question, have you used --patience when preparing this series?
>

Yes! Thanks for bringing it to my attention.

Bart

Linus Walleij Sept. 7, 2023, 7:31 a.m. UTC | #3

On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <[e-mail]> wrote:

> From: Bartosz Golaszewski <[e-mail]>
>
> gpiochip_find() is going away as it's not hot-unplug safe. This platform
> is not affected by any of the related problems as this GPIO controller
> cannot really go away but in order to finally remove this function, we
> need to convert it to using gpio_device_find() as well.
>
> Signed-off-by: Bartosz Golaszewski <[e-mail]>

I was cleaning this one just some merge cycle ago, now it
looks even better!
Reviewed-by: Linus Walleij <[e-mail]>

Yours,
Linus Walleij

Linus Walleij Sept. 7, 2023, 7:35 a.m. UTC | #4

Oops one more note:

On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <[e-mail]> wrote:

> From: Bartosz Golaszewski <[e-mail]>
>
> gpiochip_find() is going away as it's not hot-unplug safe. This platform
> is not affected by any of the related problems as this GPIO controller
> cannot really go away but in order to finally remove this function, we
> need to convert it to using gpio_device_find() as well.
>
> Signed-off-by: Bartosz Golaszewski <[e-mail]>
(...)
> +       struct gpio_device *gdev;
(...)
> +       gdev = gpio_device_find_by_label(OMAP_GPIO_LABEL);

This leaves a reference to the gdev right? No scoped guard?

If you leave a dangling reference intentionally I think it warrants
a comment ("leaving a ref here so the device will never be
free:ed").

Yours,
Linus Walleij

Bartosz Golaszewski Sept. 7, 2023, 7:57 a.m. UTC | #5

On Thu, Sep 7, 2023 at 9:35 AM Linus Walleij <[e-mail]> wrote:
>
> Oops one more note:
>
> On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <[e-mail]> wrote:
>
> > From: Bartosz Golaszewski <[e-mail]>
> >
> > gpiochip_find() is going away as it's not hot-unplug safe. This platform
> > is not affected by any of the related problems as this GPIO controller
> > cannot really go away but in order to finally remove this function, we
> > need to convert it to using gpio_device_find() as well.
> >
> > Signed-off-by: Bartosz Golaszewski <[e-mail]>
> (...)
> > +       struct gpio_device *gdev;
> (...)
> > +       gdev = gpio_device_find_by_label(OMAP_GPIO_LABEL);
>
> This leaves a reference to the gdev right? No scoped guard?
>
> If you leave a dangling reference intentionally I think it warrants
> a comment ("leaving a ref here so the device will never be
> free:ed").
>

It's right there in the comment. :)

Bart

Janusz Krzysztofik Sept. 8, 2023, 6:07 p.m. UTC | #6

Dnia czwartek, 7 września 2023 09:31:01 CEST Linus Walleij pisze:
> On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <[e-mail]> wrote:
> 
> > From: Bartosz Golaszewski <[e-mail]>
> >
> > gpiochip_find() is going away as it's not hot-unplug safe. This platform
> > is not affected by any of the related problems as this GPIO controller
> > cannot really go away but in order to finally remove this function, we
> > need to convert it to using gpio_device_find() as well.
> >
> > Signed-off-by: Bartosz Golaszewski <[e-mail]>
> 
> I was cleaning this one just some merge cycle ago, now it
> looks even better!
> Reviewed-by: Linus Walleij <[e-mail]>

Acked-by: Janusz Krzysztofik <[e-mail]>

Thanks,
Janusz

> 
> Yours,
> Linus Walleij
>

Bartosz Golaszewski Sept. 11, 2023, 11:09 a.m. UTC | #7

On Fri, Sep 8, 2023 at 8:07 PM Janusz Krzysztofik <[e-mail]> wrote:
>
> Dnia czwartek, 7 września 2023 09:31:01 CEST Linus Walleij pisze:
> > On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <[e-mail]> wrote:
> >
> > > From: Bartosz Golaszewski <[e-mail]>
> > >
> > > gpiochip_find() is going away as it's not hot-unplug safe. This platform
> > > is not affected by any of the related problems as this GPIO controller
> > > cannot really go away but in order to finally remove this function, we
> > > need to convert it to using gpio_device_find() as well.
> > >
> > > Signed-off-by: Bartosz Golaszewski <[e-mail]>
> >
> > I was cleaning this one just some merge cycle ago, now it
> > looks even better!
> > Reviewed-by: Linus Walleij <[e-mail]>
>
> Acked-by: Janusz Krzysztofik <[e-mail]>
>

Janusz,

Is it fine if I take it through the GPIO tree?

Bartosz

> Thanks,
> Janusz
>
> >
> > Yours,
> > Linus Walleij
> >
>
>
>
>

Tony Lindgren Sept. 11, 2023, 12:50 p.m. UTC | #8

* Bartosz Golaszewski <[e-mail]> [230911 11:10]:
> On Fri, Sep 8, 2023 at 8:07 PM Janusz Krzysztofik <[e-mail]> wrote:
> >
> > Dnia czwartek, 7 września 2023 09:31:01 CEST Linus Walleij pisze:
> > > On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <[e-mail]> wrote:
> > >
> > > > From: Bartosz Golaszewski <[e-mail]>
> > > >
> > > > gpiochip_find() is going away as it's not hot-unplug safe. This platform
> > > > is not affected by any of the related problems as this GPIO controller
> > > > cannot really go away but in order to finally remove this function, we
> > > > need to convert it to using gpio_device_find() as well.
> > > >
> > > > Signed-off-by: Bartosz Golaszewski <[e-mail]>
> > >
> > > I was cleaning this one just some merge cycle ago, now it
> > > looks even better!
> > > Reviewed-by: Linus Walleij <[e-mail]>
> >
> > Acked-by: Janusz Krzysztofik <[e-mail]>
> >
> 
> Janusz,
> 
> Is it fine if I take it through the GPIO tree?

Works for me at least:

Acked-by: Tony Lindgren <[e-mail]>

Janusz Krzysztofik Sept. 11, 2023, 5:17 p.m. UTC | #9

Hi Bartosz,

Dnia poniedziałek, 11 września 2023 13:09:56 CEST Bartosz Golaszewski pisze:
> On Fri, Sep 8, 2023 at 8:07 PM Janusz Krzysztofik <[e-mail]> wrote:
> >
> > Dnia czwartek, 7 września 2023 09:31:01 CEST Linus Walleij pisze:
> > > On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <[e-mail]> wrote:
> > >
> > > > From: Bartosz Golaszewski <[e-mail]>
> > > >
> > > > gpiochip_find() is going away as it's not hot-unplug safe. This platform
> > > > is not affected by any of the related problems as this GPIO controller
> > > > cannot really go away but in order to finally remove this function, we
> > > > need to convert it to using gpio_device_find() as well.
> > > >
> > > > Signed-off-by: Bartosz Golaszewski <[e-mail]>
> > >
> > > I was cleaning this one just some merge cycle ago, now it
> > > looks even better!
> > > Reviewed-by: Linus Walleij <[e-mail]>
> >
> > Acked-by: Janusz Krzysztofik <[e-mail]>
> >
> 
> Janusz,
> 
> Is it fine if I take it through the GPIO tree?

Yes, should be fine, I believe.  Tony, Aaro, any doubts?

Thanks,
Janusz

> 
> Bartosz
> 
> > Thanks,
> > Janusz
> >
> > >
> > > Yours,
> > > Linus Walleij
> > >
> >
> >
> >
> >
>

Bartosz Golaszewski Oct. 4, 2023, 11:59 a.m. UTC | #10

On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <[e-mail]> wrote:
>
> From: Bartosz Golaszewski <[e-mail]>
>
> gpiochip_find() is going away as it's not hot-unplug safe. This platform
> is not affected by any of the related problems as this GPIO controller
> cannot really go away but in order to finally remove this function, we
> need to convert it to using gpio_device_find() as well.
>
> Signed-off-by: Bartosz Golaszewski <[e-mail]>
> ---
>  arch/arm/mach-omap1/board-ams-delta.c | 36 +++++++++++++--------------
>  1 file changed, 17 insertions(+), 19 deletions(-)
>
> diff --git a/arch/arm/mach-omap1/board-ams-delta.c b/arch/arm/mach-omap1/board-ams-delta.c
> index 9808cd27e2cf..a28ea6ac1eba 100644
> --- a/arch/arm/mach-omap1/board-ams-delta.c
> +++ b/arch/arm/mach-omap1/board-ams-delta.c
> @@ -560,22 +560,6 @@ static struct gpiod_lookup_table *ams_delta_gpio_tables[] __initdata = {
>         &ams_delta_nand_gpio_table,
>  };
>
> -/*
> - * Some drivers may not use GPIO lookup tables but need to be provided
> - * with GPIO numbers.  The same applies to GPIO based IRQ lines - some
> - * drivers may even not use GPIO layer but expect just IRQ numbers.
> - * We could either define GPIO lookup tables then use them on behalf
> - * of those devices, or we can use GPIO driver level methods for
> - * identification of GPIO and IRQ numbers. For the purpose of the latter,
> - * defina a helper function which identifies GPIO chips by their labels.
> - */
> -static int gpiochip_match_by_label(struct gpio_chip *chip, void *data)
> -{
> -       char *label = data;
> -
> -       return !strcmp(label, chip->label);
> -}
> -
>  static struct gpiod_hog ams_delta_gpio_hogs[] = {
>         GPIO_HOG(LATCH2_LABEL, LATCH2_PIN_KEYBRD_DATAOUT, "keybrd_dataout",
>                  GPIO_ACTIVE_HIGH, GPIOD_OUT_LOW),
> @@ -615,14 +599,28 @@ static void __init modem_assign_irq(struct gpio_chip *chip)
>   */
>  static void __init omap_gpio_deps_init(void)
>  {
> +       struct gpio_device *gdev;
>         struct gpio_chip *chip;
>
> -       chip = gpiochip_find(OMAP_GPIO_LABEL, gpiochip_match_by_label);
> -       if (!chip) {
> -               pr_err("%s: OMAP GPIO chip not found\n", __func__);
> +       /*
> +        * Some drivers may not use GPIO lookup tables but need to be provided
> +        * with GPIO numbers. The same applies to GPIO based IRQ lines - some
> +        * drivers may even not use GPIO layer but expect just IRQ numbers.
> +        * We could either define GPIO lookup tables then use them on behalf
> +        * of those devices, or we can use GPIO driver level methods for
> +        * identification of GPIO and IRQ numbers.
> +        *
> +        * This reference will be leaked but that's alright as this device
> +        * never goes down.
> +        */
> +       gdev = gpio_device_find_by_label(OMAP_GPIO_LABEL);
> +       if (!gdev) {
> +               pr_err("%s: OMAP GPIO device not found\n", __func__);
>                 return;
>         }
>
> +       chip = gpio_device_get_chip(gdev);
> +
>         /*
>          * Start with FIQ initialization as it may have to request
>          * and release successfully each OMAP GPIO pin in turn.
> --
> 2.39.2
>

Patch applied, thanks!

Bart

Patch

diff --git a/arch/arm/mach-omap1/board-ams-delta.c b/arch/arm/mach-omap1/board-ams-delta.c
index 9808cd27e2cf..a28ea6ac1eba 100644
--- a/arch/arm/mach-omap1/board-ams-delta.c
+++ b/arch/arm/mach-omap1/board-ams-delta.c
@@ -560,22 +560,6 @@  static struct gpiod_lookup_table *ams_delta_gpio_tables[] __initdata = {
 	&ams_delta_nand_gpio_table,
 };
 
-/*
- * Some drivers may not use GPIO lookup tables but need to be provided
- * with GPIO numbers.  The same applies to GPIO based IRQ lines - some
- * drivers may even not use GPIO layer but expect just IRQ numbers.
- * We could either define GPIO lookup tables then use them on behalf
- * of those devices, or we can use GPIO driver level methods for
- * identification of GPIO and IRQ numbers. For the purpose of the latter,
- * defina a helper function which identifies GPIO chips by their labels.
- */
-static int gpiochip_match_by_label(struct gpio_chip *chip, void *data)
-{
-	char *label = data;
-
-	return !strcmp(label, chip->label);
-}
-
 static struct gpiod_hog ams_delta_gpio_hogs[] = {
 	GPIO_HOG(LATCH2_LABEL, LATCH2_PIN_KEYBRD_DATAOUT, "keybrd_dataout",
 		 GPIO_ACTIVE_HIGH, GPIOD_OUT_LOW),
@@ -615,14 +599,28 @@  static void __init modem_assign_irq(struct gpio_chip *chip)
  */
 static void __init omap_gpio_deps_init(void)
 {
+	struct gpio_device *gdev;
 	struct gpio_chip *chip;
 
-	chip = gpiochip_find(OMAP_GPIO_LABEL, gpiochip_match_by_label);
-	if (!chip) {
-		pr_err("%s: OMAP GPIO chip not found\n", __func__);
+	/*
+	 * Some drivers may not use GPIO lookup tables but need to be provided
+	 * with GPIO numbers. The same applies to GPIO based IRQ lines - some
+	 * drivers may even not use GPIO layer but expect just IRQ numbers.
+	 * We could either define GPIO lookup tables then use them on behalf
+	 * of those devices, or we can use GPIO driver level methods for
+	 * identification of GPIO and IRQ numbers.
+	 *
+	 * This reference will be leaked but that's alright as this device
+	 * never goes down.
+	 */
+	gdev = gpio_device_find_by_label(OMAP_GPIO_LABEL);
+	if (!gdev) {
+		pr_err("%s: OMAP GPIO device not found\n", __func__);
 		return;
 	}
 
+	chip = gpio_device_get_chip(gdev);
+
 	/*
 	 * Start with FIQ initialization as it may have to request
 	 * and release successfully each OMAP GPIO pin in turn.