| Message ID | 20230924-pxa-gpio-v1-0-2805b87d8894@skole.hr |
|---|---|
| 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 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?
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
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 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
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,