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

[Henning Schild]

Am Tue, 24 Jan 2023 11:46:34 +0200
schrieb Andy Shevchenko <andy.shevchenko@xxxxxxxxx>:

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

I would like to keep the number of files and CONFIG switches low, with
a focus on the config switches. Every new CONFIG=y/m has to be
requested in countless distros. So far i only dealt with debian where
ubuntu might follow, did not check others with recent enough kernels ...
like fedora if they have the SIMATIC stuff turned on or need to be
asked to do so.

Henning