diff mbox series

[RFC,v4,5/6] ARM: pxa: Convert gumstix Bluetooth to GPIO descriptors

Message ID 20231001-pxa-gpio-v4-5-0f3b975e6ed5@skole.hr (mailing list archive)
State Superseded
Headers show
Series ARM: pxa: GPIO descriptor conversions | expand

Commit Message

Duje Mihanović Oct. 1, 2023, 2:12 p.m. UTC
Gumstix still uses the legacy GPIO interface for resetting the Bluetooth
device.

Convert it to use the GPIO descriptor interface.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
---
 arch/arm/mach-pxa/gumstix.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

Comments

Bartosz Golaszewski Oct. 2, 2023, 7:42 a.m. UTC | #1
On Sun, Oct 1, 2023 at 4:13 PM Duje Mihanović <duje.mihanovic@skole.hr> wrote:
>
> Gumstix still uses the legacy GPIO interface for resetting the Bluetooth
> device.
>
> Convert it to use the GPIO descriptor interface.
>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
> ---
>  arch/arm/mach-pxa/gumstix.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/arch/arm/mach-pxa/gumstix.c b/arch/arm/mach-pxa/gumstix.c
> index c9f0f62187bd..14e1b9274d7a 100644
> --- a/arch/arm/mach-pxa/gumstix.c
> +++ b/arch/arm/mach-pxa/gumstix.c
> @@ -20,8 +20,8 @@
>  #include <linux/delay.h>
>  #include <linux/mtd/mtd.h>
>  #include <linux/mtd/partitions.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/gpio/machine.h>
> -#include <linux/gpio.h>
>  #include <linux/err.h>
>  #include <linux/clk.h>
>
> @@ -129,6 +129,9 @@ static void gumstix_udc_init(void)
>  #endif
>
>  #ifdef CONFIG_BT
> +GPIO_LOOKUP_SINGLE(gumstix_bt_gpio_table, "pxa2xx-uart.1", "pxa-gpio",
> +               GPIO_GUMSTIX_BTRESET, "BTRST", GPIO_ACTIVE_LOW);
> +
>  /* Normally, the bootloader would have enabled this 32kHz clock but many
>  ** boards still have u-boot 1.1.4 so we check if it has been turned on and
>  ** if not, we turn it on with a warning message. */
> @@ -153,24 +156,23 @@ static void gumstix_setup_bt_clock(void)
>
>  static void __init gumstix_bluetooth_init(void)
>  {
> -       int err;
> +       struct gpio_desc *desc;
> +
> +       gpiod_add_lookup_table(&gumstix_bt_gpio_table);
>
>         gumstix_setup_bt_clock();
>
> -       err = gpio_request(GPIO_GUMSTIX_BTRESET, "BTRST");
> -       if (err) {
> +       desc = gpiod_get(&pxa_device_btuart.dev, "BTRST", GPIOD_OUT_HIGH);
> +       if (IS_ERR(desc)) {
>                 pr_err("gumstix: failed request gpio for bluetooth reset\n");
>                 return;
>         }
>
> -       err = gpio_direction_output(GPIO_GUMSTIX_BTRESET, 1);
> -       if (err) {
> -               pr_err("gumstix: can't reset bluetooth\n");
> -               return;
> -       }
> -       gpio_set_value(GPIO_GUMSTIX_BTRESET, 0);
> +       gpiod_set_value(desc, 0);
>         udelay(100);
> -       gpio_set_value(GPIO_GUMSTIX_BTRESET, 1);
> +       gpiod_set_value(desc, 1);
> +
> +       gpiod_put(desc);

This changes the way this code works. You release the descriptor here,
it returns to the driver and can be re-requested by someone else. Its
value is also not guaranteed to remain as "active". Is this what you
want?

Bart

>  }
>  #else
>  static void gumstix_bluetooth_init(void)
>
> --
> 2.42.0
>
>
Duje Mihanović Oct. 2, 2023, 2:52 p.m. UTC | #2
On Monday, October 2, 2023 9:42:52 AM CEST Bartosz Golaszewski wrote:
> This changes the way this code works. You release the descriptor here,
> it returns to the driver and can be re-requested by someone else. Its
> value is also not guaranteed to remain as "active". Is this what you
> want?

Good point. Is it enough to not call gpiod_put() at the end or is it necessary 
to use a static gpio_desc instead of a local one?

Regards,
Duje
Bartosz Golaszewski Oct. 2, 2023, 5:09 p.m. UTC | #3
On Mon, Oct 2, 2023 at 4:53 PM Duje Mihanović <duje.mihanovic@skole.hr> wrote:
>
> On Monday, October 2, 2023 9:42:52 AM CEST Bartosz Golaszewski wrote:
> > This changes the way this code works. You release the descriptor here,
> > it returns to the driver and can be re-requested by someone else. Its
> > value is also not guaranteed to remain as "active". Is this what you
> > want?
>
> Good point. Is it enough to not call gpiod_put() at the end or is it necessary
> to use a static gpio_desc instead of a local one?
>

Technically it's enough to not put it. It will live on but the
reference will be leaked and most likely this will be reported by
kmemleak. So static desc would make more sense.

Bart
diff mbox series

Patch

diff --git a/arch/arm/mach-pxa/gumstix.c b/arch/arm/mach-pxa/gumstix.c
index c9f0f62187bd..14e1b9274d7a 100644
--- a/arch/arm/mach-pxa/gumstix.c
+++ b/arch/arm/mach-pxa/gumstix.c
@@ -20,8 +20,8 @@ 
 #include <linux/delay.h>
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/partitions.h>
+#include <linux/gpio/consumer.h>
 #include <linux/gpio/machine.h>
-#include <linux/gpio.h>
 #include <linux/err.h>
 #include <linux/clk.h>
 
@@ -129,6 +129,9 @@  static void gumstix_udc_init(void)
 #endif
 
 #ifdef CONFIG_BT
+GPIO_LOOKUP_SINGLE(gumstix_bt_gpio_table, "pxa2xx-uart.1", "pxa-gpio",
+		GPIO_GUMSTIX_BTRESET, "BTRST", GPIO_ACTIVE_LOW);
+
 /* Normally, the bootloader would have enabled this 32kHz clock but many
 ** boards still have u-boot 1.1.4 so we check if it has been turned on and
 ** if not, we turn it on with a warning message. */
@@ -153,24 +156,23 @@  static void gumstix_setup_bt_clock(void)
 
 static void __init gumstix_bluetooth_init(void)
 {
-	int err;
+	struct gpio_desc *desc;
+
+	gpiod_add_lookup_table(&gumstix_bt_gpio_table);
 
 	gumstix_setup_bt_clock();
 
-	err = gpio_request(GPIO_GUMSTIX_BTRESET, "BTRST");
-	if (err) {
+	desc = gpiod_get(&pxa_device_btuart.dev, "BTRST", GPIOD_OUT_HIGH);
+	if (IS_ERR(desc)) {
 		pr_err("gumstix: failed request gpio for bluetooth reset\n");
 		return;
 	}
 
-	err = gpio_direction_output(GPIO_GUMSTIX_BTRESET, 1);
-	if (err) {
-		pr_err("gumstix: can't reset bluetooth\n");
-		return;
-	}
-	gpio_set_value(GPIO_GUMSTIX_BTRESET, 0);
+	gpiod_set_value(desc, 0);
 	udelay(100);
-	gpio_set_value(GPIO_GUMSTIX_BTRESET, 1);
+	gpiod_set_value(desc, 1);
+
+	gpiod_put(desc);
 }
 #else
 static void gumstix_bluetooth_init(void)