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