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

[Henning Schild]

Am Wed, 4 Jan 2023 14:24:30 +0000
schrieb Lee Jones <lee@xxxxxxxxxx>:

> On Tue, 03 Jan 2023, Henning Schild wrote:
> 
> > 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.  
> 
> The change-log is not the issue, and you don't need to provide a
> cover-letter for a single-patch set.
> 
> The issue is that this 'solution' is a hack, built on a hack, built
> on a hack.  There shouldn't be a requirement to check Kconfig options
> from this driver.  In an ideal world the thread handling the
> -EPROBE_DEFER would not create spurious logs to trouble anyone.  What
> is it that's writing those logs?  A User or Kernel Space thread?
> Dependencies are almost universally controlled with Kconfig
> 'depends', which is how this problem should really be solved.
> 
> Taking into consideration the large backlog (nearly 100) of reviews I
> need to do and the fact that there is already a precedent for this
> behaviour inside this file, I'm tempted to apply it; however, I shall
> not be doing so without giving myself (and others) a little more time
> to think it over.

Ok.

For the future we can see how to improve on all that. The simplest
would be to have that driver depend on all possible gpio providers.
Would not allow to build super minimal kernels in case one wanted the
smallest possible ... but will be easy to maintain and not cause a
jungle of driver config switches.

As we speak i already have the third box to eventually support, which
will likely be similar but this time around with PINCTRL_ELKHARTLAKE

If that "depending on all" sounds like a plan, i can send that instead
of what we discuss here. But i prefer to keep that for the future, i
will be back with more patches anyhow.

Henning

> --
> Lee Jones [李琼斯]
>