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