Date:   Tue, 24 Jan 2023 10:29:35 +0000
From:   Lee Jones <lee@kernel.org>
To:     Andy Shevchenko <andy.shevchenko@gmail.com>
Cc:     Henning Schild <henning.schild@siemens.com>,
        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
Message-ID: <Y8+zD6AxuJVy5b7Y@google.com>
References: <20221007153323.1326-1-henning.schild@siemens.com>
 <Y8mv8PzL1UsP9gNh@google.com>
 <20230123214859.725cd1c3@md1za8fc.ad001.siemens.net>
 <CAHp75VfSHgdikX5=Qba62BwWofVf7gHhS2hq2OuBwHFz9riCWQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAHp75VfSHgdikX5=Qba62BwWofVf7gHhS2hq2OuBwHFz9riCWQ@mail.gmail.com>
Precedence: bulk

On Tue, 24 Jan 2023, Andy Shevchenko wrote:

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

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?

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

-- 
Lee Jones [李琼斯]