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

[Henning Schild <henning.schild@siemens.com>]

Am Tue, 24 Jan 2023 10:29:35 +0000
schrieb Lee Jones <lee@kernel.org>:

> On Tue, 24 Jan 2023, Andy Shevchenko wrote:
> 
> > On Mon, Jan 23, 2023 at 10:49 PM Henning Schild
> > <henning.schild@siemens.com> wrote:  
> > > Am Thu, 19 Jan 2023 21:02:40 +0000
> > > schrieb Lee Jones <lee@kernel.org>:  
> > > > 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@gmail.com> Signed-off-by: Henning Schild
> > > > > <henning.schild@siemens.com> ---
> > > > >  drivers/leds/simple/simatic-ipc-leds-gpio.c | 2 ++
> > > > >  1 file changed, 2 insertions(+)  
> > > >
> > > > FYI: I'm going to try my best not to take another one like
> > > > this.  
> > >
> > > understood!
> > >  
> > > > Please try to improve the whole situation for you next
> > > > submission.  
> > >
> > > When i have to touch this again, which i will, i will propose
> > > either "depend on all possible GPIO drivers" or introduce "#ifdef
> > > CONFIG"s. Caring most about big configs as seen in distros like
> > > debian, even for embedded systems ... i think i would prefer the
> > > first option, as it will also be easier to maintain.
> > >
> > > I do not see the whole infinite loop story on my plate, but if
> > > that got fixed i would follow up taking the fix into account.  
> 
> I still don't really know what you mean by this.  Probe deferring
> should not work this way.  Do you know why the loop is infinite on
> your platform?  What keeps triggering the re-probe?  Are you
> continually binding and unbinding drivers, forever?  Also, what is
> printing out the failure?  Maybe it should be silent?

It has been a while and i would have to reproduce this. But basically
what happened is that i registered a leds-gpio platform device with a
lookup table, no errors returned and my "driver" would be done and
leds-gpio takes over.

All GPIO_LOOKUP_IDXs point to not yet exisiting pins, potentially never
existing when the providing driver never comes up. Now leds-gpio
internally tries again and again with a high frequency (busy?) and
printing stuff (would have to try again to see what).

I think one could modifiy any other leds-gpio and totally invalidate
the lookup table by introducing typos in the chip name of each entry.

But i will rty again and get back with a better description. Maybe the
bug was fixed in the meantime or i am doing something wrong when
registering that platform-device.

Henning

> > AFAICS another possible (not sure if it's preferable) solution is to
> > split this driver to subdrivers and each of them will be dependent
> > on the corresponding pin control in Kconfig. It will satisfy both
> > of your requirements, right? Something like
> > 
> > simatic-leds-core.c
> > simatic-leds-127e.c (config ..._127E depends on PINCTRL_BROXTON)  
> 
> In theory, yes it would.  You could also introduce a core driver to
> contain all of the shared code.  Duplication would also be a travesty.
>