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@xxxxxxxxxx> 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
