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