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

[Henning Schild]

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.

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