[PATCH RFC v2 3/6] ARM: pxa: Convert Spitz CF power control to GPIO descriptors

[PATCH RFC v2 3/6] ARM: pxa: Convert Spitz CF power control to GPIO descriptors

[Duje Mihanović]

Sharp's Spitz board still uses the legacy GPIO interface for controlling
the power supply to its CF and SD card slots.

Convert it to use the GPIO descriptor interface.

Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
---
 arch/arm/mach-pxa/spitz.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-pxa/spitz.c b/arch/arm/mach-pxa/spitz.c
index 91c4b208973c..616305978727 100644
--- a/arch/arm/mach-pxa/spitz.c
+++ b/arch/arm/mach-pxa/spitz.c
@@ -133,6 +133,10 @@ static unsigned long spitz_pin_config[] __initdata = {
  * Scoop GPIO expander
  ******************************************************************************/
 #if defined(CONFIG_SHARP_SCOOP) || defined(CONFIG_SHARP_SCOOP_MODULE)
+GPIO_LOOKUP_SINGLE(spitz_card_pwr_ctrl_gpio_table, "pxa2xx-mci.0",
+		"sharp-scoop", SPITZ_GPIO_CF_POWER, "cf_power",
+		GPIO_ACTIVE_HIGH);
+
 /* SCOOP Device #1 */
 static struct resource spitz_scoop_1_resources[] = {
 	[0] = {
@@ -190,6 +194,7 @@ struct platform_device spitz_scoop_2_device = {
 static void __init spitz_scoop_init(void)
 {
 	platform_device_register(&spitz_scoop_1_device);
+	gpiod_add_lookup_table(&spitz_card_pwr_ctrl_gpio_table);
 
 	/* Akita doesn't have the second SCOOP chip */
 	if (!machine_is_akita())
@@ -201,9 +206,18 @@ static void __maybe_unused spitz_card_pwr_ctrl(uint8_t enable, uint8_t new_cpr)
 {
 	unsigned short cpr;
 	unsigned long flags;
+	struct gpio_desc *cf_power;
+
+	cf_power = gpiod_get(&pxa_device_mci.dev, "cf_power", GPIOD_ASIS);
+	if (IS_ERR(cf_power)) {
+		dev_err(&pxa_device_mci.dev,
+				"failed to get power control GPIO with %ld\n",
+				PTR_ERR(cf_power));
+		return;
+	}
 
 	if (new_cpr & 0x7) {
-		gpio_set_value(SPITZ_GPIO_CF_POWER, 1);
+		gpiod_direction_output(cf_power, 1);
 		mdelay(5);
 	}
 
@@ -222,8 +236,10 @@ static void __maybe_unused spitz_card_pwr_ctrl(uint8_t enable, uint8_t new_cpr)
 
 	if (!(cpr & 0x7)) {
 		mdelay(1);
-		gpio_set_value(SPITZ_GPIO_CF_POWER, 0);
+		gpiod_direction_output(cf_power, 0);
 	}
+
+	gpiod_put(cf_power);
 }
 
 #else

-- 
2.42.0



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC v2 3/6] ARM: pxa: Convert Spitz CF power control to GPIO descriptors

[Andy Shevchenko]

On Tue, Sep 26, 2023 at 05:46:24PM +0200, Duje Mihanović wrote:
> Sharp's Spitz board still uses the legacy GPIO interface for controlling
> the power supply to its CF and SD card slots.
> 
> Convert it to use the GPIO descriptor interface.

...

> +	cf_power = gpiod_get(&pxa_device_mci.dev, "cf_power", GPIOD_ASIS);
> +	if (IS_ERR(cf_power)) {
> +		dev_err(&pxa_device_mci.dev,
> +				"failed to get power control GPIO with %ld\n",
> +				PTR_ERR(cf_power));
> +		return;
> +	}

> +	gpiod_put(cf_power);

Don't you want to use guarded gpiod_get()?
Okay, it seems not yet in the pending list, but we can survive without that.

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC v2 3/6] ARM: pxa: Convert Spitz CF power control to GPIO descriptors

[Duje Mihanović]

On Tuesday, September 26, 2023 6:15:37 PM CEST Andy Shevchenko wrote:
> On Tue, Sep 26, 2023 at 05:46:24PM +0200, Duje Mihanović wrote:
> > Sharp's Spitz board still uses the legacy GPIO interface for controlling
> > the power supply to its CF and SD card slots.
> > 
> > Convert it to use the GPIO descriptor interface.
> 
> ...
> 
> > +	cf_power = gpiod_get(&pxa_device_mci.dev, "cf_power", GPIOD_ASIS);
> > +	if (IS_ERR(cf_power)) {
> > +		dev_err(&pxa_device_mci.dev,
> > +				"failed to get power control GPIO with 
%ld\n",
> > +				PTR_ERR(cf_power));
> > +		return;
> > +	}
> > 
> > +	gpiod_put(cf_power);
> 
> Don't you want to use guarded gpiod_get()?
> Okay, it seems not yet in the pending list, but we can survive without that.

Can you please elaborate? If I understand correctly, the if statement right 
after gpiod_get is a guard.

Regards,
Duje




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC v2 3/6] ARM: pxa: Convert Spitz CF power control to GPIO descriptors

[Andy Shevchenko]

On Wed, Sep 27, 2023 at 02:56:07PM +0200, Duje Mihanović wrote:
> On Tuesday, September 26, 2023 6:15:37 PM CEST Andy Shevchenko wrote:
> > On Tue, Sep 26, 2023 at 05:46:24PM +0200, Duje Mihanović wrote:

...

> > > +	cf_power = gpiod_get(&pxa_device_mci.dev, "cf_power", GPIOD_ASIS);
> > > +	if (IS_ERR(cf_power)) {
> > > +		dev_err(&pxa_device_mci.dev,
> > > +				"failed to get power control GPIO with 
> %ld\n",
> > > +				PTR_ERR(cf_power));
> > > +		return;
> > > +	}
> > > 
> > > +	gpiod_put(cf_power);
> > 
> > Don't you want to use guarded gpiod_get()?
> > Okay, it seems not yet in the pending list, but we can survive without that.
> 
> Can you please elaborate? If I understand correctly, the if statement right 
> after gpiod_get is a guard.

It's about RAII version of the gpiod_get(). It's quite a new thing
in the Linux kernel.

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC v2 3/6] ARM: pxa: Convert Spitz CF power control to GPIO descriptors

[Linus Walleij]

On Tue, Sep 26, 2023 at 5:46 PM Duje Mihanović <duje.mihanovic@skole.hr> wrote:

> Sharp's Spitz board still uses the legacy GPIO interface for controlling
> the power supply to its CF and SD card slots.
>
> Convert it to use the GPIO descriptor interface.
>
> Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH RFC v2 6/6] ARM: pxa: Convert gumstix Bluetooth to GPIO descriptors

[Duje Mihanović]

Gumstix still uses the legacy GPIO interface for resetting the Bluetooth
device.

Convert it to use the GPIO descriptor interface.

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);
 }
 #else
 static void gumstix_bluetooth_init(void)

-- 
2.42.0



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC v2 1/6] ARM: pxa: Convert Spitz OHCI to GPIO descriptors

[Andy Shevchenko]

On Tue, Sep 26, 2023 at 05:46:22PM +0200, Duje Mihanović wrote:
> Sharp's Spitz board still uses the legacy GPIO interface for controlling
> a GPIO pin related to the USB host controller.
> 
> Convert this function to use the new GPIO descriptor interface.

...

> +	pxa_ohci->usb_host = gpiod_get_optional(&pdev->dev, "usb-host", GPIOD_OUT_LOW);
> +	if (IS_ERR(pxa_ohci->usb_host))
> +		dev_warn(&pdev->dev, "failed to get USB host GPIO with %pe\n",
> +				pxa_ohci->usb_host);

No, with _optional() you should terminate the execution and bail out. The idea
behind *optional* is that we skip errors only for the cases when GPIO is not
provided. Otherwise we need to handle the errors (yes, the dev_err() has to
be used).

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC v2 5/6] ARM: pxa: Convert Spitz hsync to GPIO descriptors

[Andy Shevchenko]

On Tue, Sep 26, 2023 at 05:46:26PM +0200, Duje Mihanović wrote:
> Sharp's Spitz still uses the legacy GPIO interface in its
> wait_for_hsync() function.
> 
> Convert it to use the GPIO descriptor interface.

...

> +static struct gpio_desc *hsync = NULL;

Assignment is redundant.

...

>  	gpiod_add_lookup_table(&spitz_ads7846_gpio_table);
>  	gpiod_add_lookup_table(&spitz_spi_gpio_table);
> +	hsync = gpiod_get(NULL, "hsync", GPIOD_IN);
> +	if (IS_ERR(hsync)) {
> +		pr_err("Failed to get hsync GPIO: %ld\n", PTR_ERR(hsync));
> +		return;
> +	}
>  	pxa2xx_set_spi_info(2, &spitz_spi_info);
>  	spi_register_board_info(ARRAY_AND_SIZE(spitz_spi_devices));

Yeah, but the question is, if GPIO request fails, can we instantiate at least
one device and move on?

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC v2 5/6] ARM: pxa: Convert Spitz hsync to GPIO descriptors

[Duje Mihanović]

On Tuesday, September 26, 2023 6:19:23 PM CEST Andy Shevchenko wrote:
> On Tue, Sep 26, 2023 at 05:46:26PM +0200, Duje Mihanović wrote:
> >  	gpiod_add_lookup_table(&spitz_ads7846_gpio_table);
> >  	gpiod_add_lookup_table(&spitz_spi_gpio_table);
> > 
> > +	hsync = gpiod_get(NULL, "hsync", GPIOD_IN);
> > +	if (IS_ERR(hsync)) {
> > +		pr_err("Failed to get hsync GPIO: %ld\n", PTR_ERR(hsync));
> > +		return;
> > +	}
> > 
> >  	pxa2xx_set_spi_info(2, &spitz_spi_info);
> >  	spi_register_board_info(ARRAY_AND_SIZE(spitz_spi_devices));
> 
> Yeah, but the question is, if GPIO request fails, can we instantiate at 
least one device and move on?

I see. If the touchscreen is the first out of 3 devices in that array, would 
something like this do:

spi_register_board_info(ARRAY_AND_SIZE(&spitz_spi_devices[1]));

Regards,
Duje




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC v2 2/6] ARM: pxa: Convert Spitz LEDs to GPIO descriptors

[Linus Walleij]

Hi Duje,

thanks for your patch!

On Tue, Sep 26, 2023 at 5:46 PM Duje Mihanović <duje.mihanovic@skole.hr> wrote:

> Sharp's Spitz board still uses the legacy GPIO interface for configuring
> its two onboard LEDs.
>
> Convert them to use the GPIO descriptor interface.
>
> Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
(...)
> +static struct gpiod_lookup_table spitz_led_gpio_table = {
> +       .dev_id = "leds-gpio",
> +       .table = {
> +               GPIO_LOOKUP("pxa-gpio", SPITZ_GPIO_LED_ORANGE, "led_orange",
> +                               GPIO_ACTIVE_HIGH),
> +               GPIO_LOOKUP("pxa-gpio", SPITZ_GPIO_LED_GREEN, "led_green",
> +                               GPIO_ACTIVE_HIGH),

Unfortunately this is not how leds-gpio works.

You have to have the name be NULL instead of "led_orange" etc, and
put the LEDs in index order, in this case something like;

+       .dev_id = "leds-gpio",
+       .table = {
+               GPIO_LOOKUP_IDX("pxa-gpio", SPITZ_GPIO_LED_ORANGE,
NULL, 0, GPIO_ACTIVE_HIGH),
+               GPIO_LOOKUP_IDX("pxa-gpio", SPITZ_GPIO_LED_GREEN,
NULL, 1, GPIO_ACTIVE_HIGH),

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC v2 4/6] ARM: pxa: Convert reset driver to GPIO descriptors

[Andy Shevchenko]

On Tue, Sep 26, 2023 at 05:46:25PM +0200, Duje Mihanović wrote:
> The PXA reset driver still uses the legacy GPIO interface for
> configuring and asserting the reset pin.
> 
> Convert it to use the GPIO descriptor interface.

...

> +static struct gpio_desc *reset_gpio = NULL;

Assignment is not needed, it's guaranteed to be NULL by the C standard.

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC v2 4/6] ARM: pxa: Convert reset driver to GPIO descriptors

[Linus Walleij]

On Tue, Sep 26, 2023 at 5:46 PM Duje Mihanović <duje.mihanovic@skole.hr> wrote:

> The PXA reset driver still uses the legacy GPIO interface for
> configuring and asserting the reset pin.
>
> Convert it to use the GPIO descriptor interface.
>
> Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>

Looks OK to me, there are details I would do differently,
but hey: I'm just happy this gets done.
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel