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

[Lee Jones]

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.

--
Lee Jones [李琼斯]