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

Message ID 20230924-pxa-gpio-v1-1-2805b87d8894@skole.hr
State New
Headers
Series ARM: pxa: GPIO descriptor conversions |

Commit Message

Duje Mihanović Sept. 24, 2023, 4:42 p.m. UTC
  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(-)
  

Comments

andy@kernel.org Sept. 25, 2023, 7:30 a.m. UTC | #1
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?
  
Linus Walleij Sept. 27, 2023, 2:01 p.m. UTC | #2
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
  
andy@kernel.org Oct. 1, 2023, 8:18 a.m. UTC | #3
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.
  
andy@kernel.org Oct. 1, 2023, 9:21 a.m. UTC | #4
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.
  
Linus Walleij Oct. 2, 2023, 8 a.m. UTC | #5
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
  

Patch

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);
 }