Re: [PATCH v4] leds: simatic-ipc-leds-gpio: make sure we have the GPIO providing driver

[Henning Schild]

Am Mon, 2 Jan 2023 16:22:27 +0100
schrieb Henning Schild <henning.schild@xxxxxxxxxxx>:

> Am Fri, 23 Dec 2022 11:58:13 +0000
> schrieb Lee Jones <lee@xxxxxxxxxx>:
> 
> > On Fri, 07 Oct 2022, Henning Schild wrote:
> >   
> > > If we register a "leds-gpio" platform device for GPIO pins that do
> > > not exist we get a -EPROBE_DEFER and the probe will be tried again
> > > later. If there is no driver to provide that pin we will poll
> > > forever and also create a lot of log messages.
> > > 
> > > So check if that GPIO driver is configured, if so it will come up
> > > eventually. If not, we exit our probe function early and do not
> > > even bother registering the "leds-gpio". This method was chosen
> > > over "Kconfig depends" since this way we can add support for more
> > > devices and GPIO backends more easily without "depends":ing on all
> > > GPIO backends.
> > > 
> > > Fixes: a6c80bec3c93 ("leds: simatic-ipc-leds-gpio: Add GPIO
> > > version of Siemens driver") Reviewed-by: Andy Shevchenko
> > > <andy.shevchenko@xxxxxxxxx> Signed-off-by: Henning Schild
> > > <henning.schild@xxxxxxxxxxx> ---    
> > 
> > What happened in versions 1 through 3?  Please provide a
> > change-log.  
> 
> Not too much really, but i will write a changelog and cover letter
> when sending again. Mostly commit message stuff and later a rebase.

Lee please let me know if you insist on that changelog, in which case i
would send that same patch again with a cover-letter that will carry a
not too spectacular changelog.

Or get back on the rest of what i wrote earlier, maybe we need another
version of the patch and not just the same one again with only a
changelog added.

Henning

> > >  drivers/leds/simple/simatic-ipc-leds-gpio.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/leds/simple/simatic-ipc-leds-gpio.c
> > > b/drivers/leds/simple/simatic-ipc-leds-gpio.c index
> > > b9eeb8702df0..fb8d427837db 100644 ---
> > > a/drivers/leds/simple/simatic-ipc-leds-gpio.c +++
> > > b/drivers/leds/simple/simatic-ipc-leds-gpio.c @@ -77,6 +77,8 @@
> > > static int simatic_ipc_leds_gpio_probe(struct platform_device
> > > *pdev) switch (plat->devmode) {
> > >  	case SIMATIC_IPC_DEVICE_127E:
> > > +		if (!IS_ENABLED(CONFIG_PINCTRL_BROXTON))
> > > +			return -ENODEV;    
> > 
> > I see that there is an unfortunate precedent for this in the lines
> > below.  However, I also see that the commit which added it was not
> > reviewed by Pavel.  
> 
> Right i think that might have been you in the end.
> 
> > This is an interesting problem, due to the different devices we're
> > attempting to support in this single driver using different
> > GPIO/PINCTRL drivers, which is unusual.  We usually resolve these
> > kinds of issues as a Kconfig 'depends' line which covers the whole
> > driver.  
> 
> This was tried but the result was not too nice. It is really the same
> gpio led driver implemented on top of multiple possible gpio chip
> drivers. Making it depend on both pulls in too much in case one wants
> a minimal config, writing a new driver for each backend would
> duplicate too much code.
> 
> But maybe a splitting out a -common or moving stuff into headers could
> help with the duplication if we want to go the "one driver for one
> device" road. I would not want that and what we currently see was
> discussed and approved as part of another series, when i introduced
> x27G.
>  
> > Would 'depends GPIO_F7188X || PINCTRL_BROXTON' be a suitable
> > replacement, I wonder?  If it's possible for SIMATIC_IPC_DEVICE_127E
> > to be probing when only GPIO_F7188X is enabled?  If so, this would
> > result in the same scenario.  
> 
> No that would not work. Depending on which board we are on we depend
> on another pin provider. "&&" would be but it would be kind of
> overkill and not allow for a minimal kernel config in case someone
> wanted a special minimal kernel for either one.
> 
> > It also seems wrong for -EPROBE_DEFER to loop indefinitely.  Surely
> > in some valid circumstances dependencies are never satisfied?  
> 
> Well that is what i would guess as well. But that infinite loop
> waiting for a pin to appear endlessly is a part of "leds-gpio". If
> "leds-gpio" had some magic to eventually bail out (maybe say we give
> it X runs with some sleep back-off) i would not have to do anything
> here. I consider that patch a workaround for a shortcoming in
> "leds-gpio", which busy loops and fills up your disk quickly with logs
> if you mention a pin that never comes. Which i imagine can quickly
> happen if you have a typo on your device tree or a kernel config not
> enabling a pin provider driver.
> 
> I am not sure there are no other valid reasons. And i think that indef
> loop needs fixing at some point. Hopefully by a LEDs maintainer or
> maybe i will even help out.
> 
> Until that day i would like to have the proposed patch merged to not
> have users run into a known issue. The pattern is established and has
> been discussed before and the patch it rather trivial.
> 
> Later we can see about improving and ask fundamental questions again.
> 
> Henning
> 
> > >  		simatic_ipc_led_gpio_table =
> > > &simatic_ipc_led_gpio_table_127e; break;
> > >  	case SIMATIC_IPC_DEVICE_227G:
> > > -- 
> > > 2.35.1
> > >     
> >   
>