| Message ID | 20230924-pxa-gpio-v1-1-2805b87d8894@skole.hr (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ARM: pxa: GPIO descriptor conversions | expand |
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?
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
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.
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.
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
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); }
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(-)