[PATCH RFC 0/6] ARM: pxa: GPIO descriptor conversions

[PATCH RFC 0/6] ARM: pxa: GPIO descriptor conversions

[Duje Mihanović]

Hello,

Small series to convert some of the board files in the mach-pxa directory
to use the new GPIO descriptor interface.

Most notably, the am200epd, am300epd and Spitz matrix keypad among
others are not converted in this series.

Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
---
Duje Mihanović (6):
      ARM: pxa: Convert Spitz OHCI to GPIO descriptors
      ARM: pxa: Convert Spitz LEDs to GPIO descriptors
      ARM: pxa: Convert Spitz CF power control to GPIO descriptors
      ARM: pxa: Convert reset driver to GPIO descriptors
      ARM: pxa: Convert Spitz hsync to GPIO descriptors
      ARM: pxa: Convert gumstix Bluetooth to GPIO descriptors

 arch/arm/mach-pxa/gumstix.c    | 24 ++++++++-------
 arch/arm/mach-pxa/reset.c      | 40 +++++++++----------------
 arch/arm/mach-pxa/reset.h      |  3 +-
 arch/arm/mach-pxa/spitz.c      | 68 +++++++++++++++++++++++++++++++++---------
 drivers/usb/host/ohci-pxa27x.c | 10 +++++++
 5 files changed, 92 insertions(+), 53 deletions(-)
---
base-commit: ce9ecca0238b140b88f43859b211c9fdfd8e5b70
change-id: 20230807-pxa-gpio-3ce25d574814

Best regards,
-- 
Duje Mihanović <duje.mihanovic@skole.hr>

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

[Duje Mihanović]

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.

Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
---
 arch/arm/mach-pxa/spitz.c      | 13 ++++++-------
 drivers/usb/host/ohci-pxa27x.c | 10 ++++++++++
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/arch/arm/mach-pxa/spitz.c b/arch/arm/mach-pxa/spitz.c
index cc691b199429..535e2b2e997b 100644
--- a/arch/arm/mach-pxa/spitz.c
+++ b/arch/arm/mach-pxa/spitz.c
@@ -649,23 +649,22 @@ static inline void spitz_mmc_init(void) {}
  * USB Host
  ******************************************************************************/
 #if defined(CONFIG_USB_OHCI_HCD) || defined(CONFIG_USB_OHCI_HCD_MODULE)
+GPIO_LOOKUP_SINGLE(spitz_usb_host_gpio_table, "pxa27x-ohci", "gpio-pxa",
+		SPITZ_GPIO_USB_HOST, "usb-host", GPIO_ACTIVE_LOW);
+
 static int spitz_ohci_init(struct device *dev)
 {
-	int err;
-
-	err = gpio_request(SPITZ_GPIO_USB_HOST, "USB_HOST");
-	if (err)
-		return err;
+	gpiod_add_lookup_table(&spitz_usb_host_gpio_table);
 
 	/* Only Port 2 is connected, setup USB Port 2 Output Control Register */
 	UP2OCR = UP2OCR_HXS | UP2OCR_HXOE | UP2OCR_DPPDE | UP2OCR_DMPDE;
 
-	return gpio_direction_output(SPITZ_GPIO_USB_HOST, 1);
+	return 0;
 }
 
 static void spitz_ohci_exit(struct device *dev)
 {
-	gpio_free(SPITZ_GPIO_USB_HOST);
+	gpiod_remove_lookup_table(&spitz_usb_host_gpio_table);
 }
 
 static struct pxaohci_platform_data spitz_ohci_platform_data = {
diff --git a/drivers/usb/host/ohci-pxa27x.c b/drivers/usb/host/ohci-pxa27x.c
index 357d9aee38a3..0cf222f9d64b 100644
--- a/drivers/usb/host/ohci-pxa27x.c
+++ b/drivers/usb/host/ohci-pxa27x.c
@@ -121,6 +121,7 @@ struct pxa27x_ohci {
 	void __iomem	*mmio_base;
 	struct regulator *vbus[3];
 	bool		vbus_enabled[3];
+	struct gpio_desc *usb_host;
 };
 
 #define to_pxa27x_ohci(hcd)	(struct pxa27x_ohci *)(hcd_to_ohci(hcd)->priv)
@@ -447,6 +448,12 @@ static int ohci_hcd_pxa27x_probe(struct platform_device *pdev)
 	pxa_ohci = to_pxa27x_ohci(hcd);
 	pxa_ohci->clk = usb_clk;
 	pxa_ohci->mmio_base = (void __iomem *)hcd->regs;
+	pxa_ohci->usb_host = gpiod_get(&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 %d\n",
+				(int) pxa_ohci->usb_host);
+		pxa_ohci->usb_host = NULL;
+	}
 
 	for (i = 0; i < 3; ++i) {
 		char name[6];
@@ -512,6 +519,9 @@ static void ohci_hcd_pxa27x_remove(struct platform_device *pdev)
 	for (i = 0; i < 3; ++i)
 		pxa27x_ohci_set_vbus_power(pxa_ohci, i, false);
 
+	if (pxa_ohci->usb_host)
+		gpiod_put(pxa_ohci->usb_host);
+
 	usb_put_hcd(hcd);
 }
 

-- 
2.42.0

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

[Duje Mihanović]

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>
---
 arch/arm/mach-pxa/spitz.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-pxa/spitz.c b/arch/arm/mach-pxa/spitz.c
index 535e2b2e997b..91c4b208973c 100644
--- a/arch/arm/mach-pxa/spitz.c
+++ b/arch/arm/mach-pxa/spitz.c
@@ -452,16 +452,25 @@ static inline void spitz_keys_init(void) {}
  * LEDs
  ******************************************************************************/
 #if defined(CONFIG_LEDS_GPIO) || defined(CONFIG_LEDS_GPIO_MODULE)
+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),
+		{ }
+	}
+};
+
 static struct gpio_led spitz_gpio_leds[] = {
 	{
 		.name			= "spitz:amber:charge",
 		.default_trigger	= "sharpsl-charge",
-		.gpio			= SPITZ_GPIO_LED_ORANGE,
 	},
 	{
 		.name			= "spitz:green:hddactivity",
 		.default_trigger	= "disk-activity",
-		.gpio			= SPITZ_GPIO_LED_GREEN,
 	},
 };
 
@@ -480,6 +489,11 @@ static struct platform_device spitz_led_device = {
 
 static void __init spitz_leds_init(void)
 {
+	gpiod_add_lookup_table(&spitz_led_gpio_table);
+	spitz_gpio_leds[0].gpiod = gpiod_get(&spitz_led_device.dev,
+			"led_orange", GPIOD_ASIS);
+	spitz_gpio_leds[1].gpiod = gpiod_get(&spitz_led_device.dev,
+			"led_green", GPIOD_ASIS);
 	platform_device_register(&spitz_led_device);
 }
 #else

-- 
2.42.0

[PATCH RFC 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

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

[Duje Mihanović]

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>
---
 arch/arm/mach-pxa/reset.c | 40 ++++++++++++++--------------------------
 arch/arm/mach-pxa/reset.h |  3 +--
 arch/arm/mach-pxa/spitz.c |  6 +++++-
 3 files changed, 20 insertions(+), 29 deletions(-)

diff --git a/arch/arm/mach-pxa/reset.c b/arch/arm/mach-pxa/reset.c
index 27293549f8ad..1484dce1383c 100644
--- a/arch/arm/mach-pxa/reset.c
+++ b/arch/arm/mach-pxa/reset.c
@@ -2,7 +2,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/delay.h>
-#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/io.h>
 #include <asm/proc-fns.h>
 #include <asm/system_misc.h>
@@ -14,33 +14,21 @@
 
 static void do_hw_reset(void);
 
-static int reset_gpio = -1;
+static struct gpio_desc *reset_gpio = NULL;
 
-int init_gpio_reset(int gpio, int output, int level)
+int init_gpio_reset(int output, int level)
 {
-	int rc;
-
-	rc = gpio_request(gpio, "reset generator");
-	if (rc) {
-		printk(KERN_ERR "Can't request reset_gpio\n");
-		goto out;
+	reset_gpio = gpiod_get(NULL, "reset generator", GPIOD_ASIS);
+	if (IS_ERR(reset_gpio)) {
+		printk(KERN_ERR "Can't request reset_gpio: %ld\n",
+				PTR_ERR(reset_gpio));
+		return PTR_ERR(reset_gpio);
 	}
 
 	if (output)
-		rc = gpio_direction_output(gpio, level);
+		return gpiod_direction_output(reset_gpio, level);
 	else
-		rc = gpio_direction_input(gpio);
-	if (rc) {
-		printk(KERN_ERR "Can't configure reset_gpio\n");
-		gpio_free(gpio);
-		goto out;
-	}
-
-out:
-	if (!rc)
-		reset_gpio = gpio;
-
-	return rc;
+		return gpiod_direction_input(reset_gpio);
 }
 
 /*
@@ -50,16 +38,16 @@ int init_gpio_reset(int gpio, int output, int level)
  */
 static void do_gpio_reset(void)
 {
-	BUG_ON(reset_gpio == -1);
+	BUG_ON(IS_ERR(reset_gpio));
 
 	/* drive it low */
-	gpio_direction_output(reset_gpio, 0);
+	gpiod_direction_output(reset_gpio, 0);
 	mdelay(2);
 	/* rising edge or drive high */
-	gpio_set_value(reset_gpio, 1);
+	gpiod_set_value(reset_gpio, 1);
 	mdelay(2);
 	/* falling edge */
-	gpio_set_value(reset_gpio, 0);
+	gpiod_set_value(reset_gpio, 0);
 
 	/* give it some time */
 	mdelay(10);
diff --git a/arch/arm/mach-pxa/reset.h b/arch/arm/mach-pxa/reset.h
index 963dd190bc13..5864f61a0e94 100644
--- a/arch/arm/mach-pxa/reset.h
+++ b/arch/arm/mach-pxa/reset.h
@@ -13,10 +13,9 @@ extern void pxa_register_wdt(unsigned int reset_status);
 
 /**
  * init_gpio_reset() - register GPIO as reset generator
- * @gpio: gpio nr
  * @output: set gpio as output instead of input during normal work
  * @level: output level
  */
-extern int init_gpio_reset(int gpio, int output, int level);
+extern int init_gpio_reset(int output, int level);
 
 #endif /* __ASM_ARCH_RESET_H */
diff --git a/arch/arm/mach-pxa/spitz.c b/arch/arm/mach-pxa/spitz.c
index 616305978727..94bcb187713b 100644
--- a/arch/arm/mach-pxa/spitz.c
+++ b/arch/arm/mach-pxa/spitz.c
@@ -1024,9 +1024,13 @@ static void spitz_restart(enum reboot_mode mode, const char *cmd)
 	spitz_poweroff();
 }
 
+GPIO_LOOKUP_SINGLE(spitz_reset_gpio_table, NULL, "pxa-gpio",
+		SPITZ_GPIO_ON_RESET, "reset generator", GPIO_ACTIVE_HIGH);
+
 static void __init spitz_init(void)
 {
-	init_gpio_reset(SPITZ_GPIO_ON_RESET, 1, 0);
+	gpiod_add_lookup_table(&spitz_reset_gpio_table);
+	init_gpio_reset(1, 0);
 	pm_power_off = spitz_poweroff;
 
 	PMCR = 0x00;

-- 
2.42.0

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

[Duje Mihanović]

Sharp's Spitz still uses the legacy GPIO interface in its wait_for_hsync
function.

Convert it to use the GPIO descriptor interface.

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

diff --git a/arch/arm/mach-pxa/spitz.c b/arch/arm/mach-pxa/spitz.c
index 94bcb187713b..1e38069e75f7 100644
--- a/arch/arm/mach-pxa/spitz.c
+++ b/arch/arm/mach-pxa/spitz.c
@@ -520,12 +520,14 @@ static inline void spitz_leds_init(void) {}
  * SSP Devices
  ******************************************************************************/
 #if defined(CONFIG_SPI_PXA2XX) || defined(CONFIG_SPI_PXA2XX_MODULE)
+static struct gpio_desc *hsync = NULL;
+
 static void spitz_ads7846_wait_for_hsync(void)
 {
-	while (gpio_get_value(SPITZ_GPIO_HSYNC))
+	while (gpiod_get_value(hsync))
 		cpu_relax();
 
-	while (!gpio_get_value(SPITZ_GPIO_HSYNC))
+	while (!gpiod_get_value(hsync))
 		cpu_relax();
 }
 
@@ -543,6 +545,8 @@ static struct gpiod_lookup_table spitz_ads7846_gpio_table = {
 	.table = {
 		GPIO_LOOKUP("gpio-pxa", SPITZ_GPIO_TP_INT,
 			    "pendown", GPIO_ACTIVE_LOW),
+		GPIO_LOOKUP("gpio-pxa", SPITZ_GPIO_HSYNC,
+			    "hsync", GPIO_ACTIVE_LOW),
 		{ }
 	},
 };
@@ -622,6 +626,9 @@ static void __init spitz_spi_init(void)
 
 	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));
 	pxa2xx_set_spi_info(2, &spitz_spi_info);
 	spi_register_board_info(ARRAY_AND_SIZE(spitz_spi_devices));
 }

-- 
2.42.0

[PATCH RFC 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

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

[Andy Shevchenko]

On Sun, Sep 24, 2023 at 06:42:54PM +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(&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 %d\n",
> +				(int) pxa_ohci->usb_host);

Casting is no go in 99.9% cases in printf(), so use proper specifier.
Hint: Nice looking message can be obtained by using %pe.

> +		pxa_ohci->usb_host = NULL;

Instead, call for _optional() API.

> +	}

...

> +	if (pxa_ohci->usb_host)
> +		gpiod_put(pxa_ohci->usb_host);

Linus, Bart, do we have misdesigned _optinal() GPIO APIs?

In GPIOLIB=n, the above requires that redundant check. Shouldn't we replace
gpiod_put() stub to be simply no-op?

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Sun, Sep 24, 2023 at 06:42:57PM +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.

...

> +	reset_gpio = gpiod_get(NULL, "reset generator", GPIOD_ASIS);
> +	if (IS_ERR(reset_gpio)) {
> +		printk(KERN_ERR "Can't request reset_gpio: %ld\n",
> +				PTR_ERR(reset_gpio));

pr_err()

> +		return PTR_ERR(reset_gpio);
>  	}

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Sun, Sep 24, 2023 at 06:42:58PM +0200, Duje Mihanović wrote:
> Sharp's Spitz still uses the legacy GPIO interface in its wait_for_hsync

We refer to the functions as wait_for_hsync().

> function.
> 
> Convert it to use the GPIO descriptor interface.

...

> +	hsync = gpiod_get(NULL, "hsync", GPIOD_IN);
> +	if (IS_ERR(hsync))
> +		pr_err("Failed to get hsync GPIO: %ld\n", PTR_ERR(hsync));

So, I didn't get, is this GPIO crucial for functioning or optional?
If the former, we need to stop here or bail out (if possible), do we?
Or should use _optional() API?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFC 0/6] ARM: pxa: GPIO descriptor conversions

[Andy Shevchenko]

On Sun, Sep 24, 2023 at 06:42:53PM +0200, Duje Mihanović wrote:
> Hello,
> 
> Small series to convert some of the board files in the mach-pxa directory
> to use the new GPIO descriptor interface.
> 
> Most notably, the am200epd, am300epd and Spitz matrix keypad among
> others are not converted in this series.

Log waited clean up, thank you for doing it!
My comments are placed per individual patches.

-- 
With Best Regards,
Andy Shevchenko

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

[Duje Mihanović]

On Monday, September 25, 2023 9:36:40 AM CEST Andy Shevchenko wrote:
> So, I didn't get, is this GPIO crucial for functioning or optional?
> If the former, we need to stop here or bail out (if possible), do we?
> Or should use _optional() API?

I don't know and I do not have any hardware to test on. Though I'd assume the 
touchscreen would not work properly (if at all), so I'll add a return there.

Regards,
Duje

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

[Linus Walleij]

On Mon, Sep 25, 2023 at 9:30 AM Andy Shevchenko <andy@kernel.org> wrote:

> > +     if (pxa_ohci->usb_host)
> > +             gpiod_put(pxa_ohci->usb_host);
>
> Linus, Bart, do we have misdesigned _optinal() GPIO APIs?
>
> In GPIOLIB=n, the above requires that redundant check. Shouldn't we replace
> gpiod_put() stub to be simply no-op?

You mean the WARN_ON(desc) in gpiod_put() in the static inline
stub version?

I thought about it for a bit, drafted a patch removing them, and then
realized the following:

If someone is making the gpiolib optional for a driver, i.e. neither
DEPENDS ON GPIOLIB nor SELECT GPIOLIB, they are a quite
narrow segment. I would say in 9 cases out of 10 or more this is
just a driver that should depend on or select GPIOLIB.

I think such drivers should actually do the NULL checks and not be
too convenient, the reason is readability: someone reading that
driver will be thinking gpios are not optional if they can call
gpiod_set_value(), gpiod_put() etc without any sign that the
desc is optional.

If the driver uses [devm_]gpiod_get_optional() the library is not
using the stubs and does the right thing, and it is clear that
the GPIO is *runtime* optional.

But *compile time* optional, *combined* with runtime optional -
I'm not so happy if we try to avoid warnings around that. I think
it leads to confusing configs and code that looks like gpiolib is
around despite it wasn't selected.

If the code isn't depending on or selecting GPIOLIB and still
use _optional() calls, it better be ready to do some extra checks,
because this is a weird combo, it can't be common.

Could be a documentation update making this clear though.

What do you other people think?

Yours,
Linus Walleij

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

[Andy Shevchenko]

On Wed, Sep 27, 2023 at 04:01:58PM +0200, Linus Walleij wrote:
> On Mon, Sep 25, 2023 at 9:30 AM Andy Shevchenko <andy@kernel.org> wrote:

...

> > > +     if (pxa_ohci->usb_host)
> > > +             gpiod_put(pxa_ohci->usb_host);
> >
> > Linus, Bart, do we have misdesigned _optinal() GPIO APIs?
> >
> > In GPIOLIB=n, the above requires that redundant check. Shouldn't we replace
> > gpiod_put() stub to be simply no-op?
> 
> You mean the WARN_ON(desc) in gpiod_put() in the static inline
> stub version?
> 
> I thought about it for a bit, drafted a patch removing them, and then
> realized the following:
> 
> If someone is making the gpiolib optional for a driver, i.e. neither
> DEPENDS ON GPIOLIB nor SELECT GPIOLIB, they are a quite
> narrow segment. I would say in 9 cases out of 10 or more this is
> just a driver that should depend on or select GPIOLIB.
> 
> I think such drivers should actually do the NULL checks and not be
> too convenient, the reason is readability: someone reading that
> driver will be thinking gpios are not optional if they can call
> gpiod_set_value(), gpiod_put() etc without any sign that the
> desc is optional.
> 
> If the driver uses [devm_]gpiod_get_optional() the library is not
> using the stubs and does the right thing, and it is clear that
> the GPIO is *runtime* optional.
> 
> But *compile time* optional, *combined* with runtime optional -
> I'm not so happy if we try to avoid warnings around that. I think
> it leads to confusing configs and code that looks like gpiolib is
> around despite it wasn't selected.
> 
> If the code isn't depending on or selecting GPIOLIB and still
> use _optional() calls, it better be ready to do some extra checks,
> because this is a weird combo, it can't be common.
> 
> Could be a documentation update making this clear though.
> 
> What do you other people think?

The problem here indeed if the code is not selecting or being dependent on
GPIOLIB and uses _optional() calls.

I agree that this is quite a niche that should be addressed on the driver side.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Sun, Oct 01, 2023 at 11:18:41AM +0300, Andy Shevchenko wrote:
> On Wed, Sep 27, 2023 at 04:01:58PM +0200, Linus Walleij wrote:
> > On Mon, Sep 25, 2023 at 9:30 AM Andy Shevchenko <andy@kernel.org> wrote:

...

> > > > +     if (pxa_ohci->usb_host)
> > > > +             gpiod_put(pxa_ohci->usb_host);
> > >
> > > Linus, Bart, do we have misdesigned _optinal() GPIO APIs?
> > >
> > > In GPIOLIB=n, the above requires that redundant check. Shouldn't we replace
> > > gpiod_put() stub to be simply no-op?
> > 
> > You mean the WARN_ON(desc) in gpiod_put() in the static inline
> > stub version?
> > 
> > I thought about it for a bit, drafted a patch removing them, and then
> > realized the following:
> > 
> > If someone is making the gpiolib optional for a driver, i.e. neither
> > DEPENDS ON GPIOLIB nor SELECT GPIOLIB, they are a quite
> > narrow segment. I would say in 9 cases out of 10 or more this is
> > just a driver that should depend on or select GPIOLIB.
> > 
> > I think such drivers should actually do the NULL checks and not be
> > too convenient, the reason is readability: someone reading that
> > driver will be thinking gpios are not optional if they can call
> > gpiod_set_value(), gpiod_put() etc without any sign that the
> > desc is optional.
> > 
> > If the driver uses [devm_]gpiod_get_optional() the library is not
> > using the stubs and does the right thing, and it is clear that
> > the GPIO is *runtime* optional.
> > 
> > But *compile time* optional, *combined* with runtime optional -
> > I'm not so happy if we try to avoid warnings around that. I think
> > it leads to confusing configs and code that looks like gpiolib is
> > around despite it wasn't selected.
> > 
> > If the code isn't depending on or selecting GPIOLIB and still
> > use _optional() calls, it better be ready to do some extra checks,
> > because this is a weird combo, it can't be common.
> > 
> > Could be a documentation update making this clear though.
> > 
> > What do you other people think?
> 
> The problem here indeed if the code is not selecting or being dependent on
> GPIOLIB and uses _optional() calls.
> 
> I agree that this is quite a niche that should be addressed on the driver side.

One more thing, though. I think those warnings are incomplete or actually
reversed, and we outta use WARN_ON(IS_ERR(desc)), no?

This way it will fix my concerns and your concerns will be satisfied, right?
So, if gpiod_get() returns an error pointer and then we are trying to
free it with GPIOLIB=n, _then_ we will got a warning and it's obvious that
driver has to be prepared for that, otherwise if we have it NULL and
call for gpiod_get_optional(), even with GPIOLIB=n, it's fine to free, we
don't care.

-- 
With Best Regards,
Andy Shevchenko

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

[Linus Walleij]

On Sun, Oct 1, 2023 at 11:22 AM Andy Shevchenko <andy@kernel.org> wrote:

 One more thing, though. I think those warnings are incomplete or actually
> reversed, and we outta use WARN_ON(IS_ERR(desc)), no?
>
> This way it will fix my concerns and your concerns will be satisfied, right?
> So, if gpiod_get() returns an error pointer and then we are trying to
> free it with GPIOLIB=n, _then_ we will got a warning and it's obvious that
> driver has to be prepared for that, otherwise if we have it NULL and
> call for gpiod_get_optional(), even with GPIOLIB=n, it's fine to free, we
> don't care.

Since we return return ERR_PTR(-ENOSYS) when compiled out
this sounds right to me!

Yours,
Linus Walleij