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

[Henning Schild]

Am Tue, 24 Jan 2023 14:52:48 +0100
schrieb Henning Schild <henning.schild@xxxxxxxxxxx>:

> Am Tue, 24 Jan 2023 10:29:35 +0000
> schrieb Lee Jones <lee@xxxxxxxxxx>:
> 
> > On Tue, 24 Jan 2023, Andy Shevchenko wrote:
> >   
> > > On Mon, Jan 23, 2023 at 10:49 PM Henning Schild
> > > <henning.schild@xxxxxxxxxxx> wrote:    
> > > > Am Thu, 19 Jan 2023 21:02:40 +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> ---
> > > > > >  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.

I tried again and it turns out that my driver is to blaim. After
registering that leds-gpio it goes and initialized two more LED-related
pins. If those are not there i return a DEFER out of probe and that is
causing the loop. I will have to find a better way of dealing with
those two extra GPIOs and possible DEFERS on them.

gpiod = gpiod_get_index..
...
return PTR_ERR(gpiod);

is seems to be the real problem here

Sorry for the noise and thanks for asking several times, better patches
will follow. Ideas and pointers welcome.

Henning


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