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

From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Henning Schild <henning.schild@siemens.com>
Cc: Lee Jones <lee@kernel.org>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Pavel Machek <pavel@ucw.cz>,
	linux-leds@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4] leds: simatic-ipc-leds-gpio: make sure we have the GPIO providing driver
Date: Tue, 24 Jan 2023 11:46:34 +0200	[thread overview]
Message-ID: <CAHp75VfSHgdikX5=Qba62BwWofVf7gHhS2hq2OuBwHFz9riCWQ@mail.gmail.com> (raw)
In-Reply-To: <20230123214859.725cd1c3@md1za8fc.ad001.siemens.net>

On Mon, Jan 23, 2023 at 10:49 PM Henning Schild
<henning.schild@siemens.com> wrote:
> Am Thu, 19 Jan 2023 21:02:40 +0000
> schrieb Lee Jones <lee@kernel.org>:
> > 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@gmail.com> Signed-off-by: Henning Schild
> > > <henning.schild@siemens.com> ---
> > >  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)
...

-- 
With Best Regards,
Andy Shevchenko