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

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

[Henning Schild]

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(+)

diff --git a/drivers/leds/simple/simatic-ipc-leds-gpio.c b/drivers/leds/simple/simatic-ipc-leds-gpio.c
index b9eeb8702df0..fb8d427837db 100644
--- a/drivers/leds/simple/simatic-ipc-leds-gpio.c
+++ b/drivers/leds/simple/simatic-ipc-leds-gpio.c
@@ -77,6 +77,8 @@ static int simatic_ipc_leds_gpio_probe(struct platform_device *pdev)
 
 	switch (plat->devmode) {
 	case SIMATIC_IPC_DEVICE_127E:
+		if (!IS_ENABLED(CONFIG_PINCTRL_BROXTON))
+			return -ENODEV;
 		simatic_ipc_led_gpio_table = &simatic_ipc_led_gpio_table_127e;
 		break;
 	case SIMATIC_IPC_DEVICE_227G:
-- 
2.35.1

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

[Henning Schild]

Ping. This still applies and still is relevant. Maybe got lost or stuck
in the LED subsystem.

Henning

Am Fri, 7 Oct 2022 17:33:23 +0200
schrieb Henning Schild <henning.schild@siemens.com>:

> 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(+)
> 
> diff --git a/drivers/leds/simple/simatic-ipc-leds-gpio.c
> b/drivers/leds/simple/simatic-ipc-leds-gpio.c index
> b9eeb8702df0..fb8d427837db 100644 ---
> a/drivers/leds/simple/simatic-ipc-leds-gpio.c +++
> b/drivers/leds/simple/simatic-ipc-leds-gpio.c @@ -77,6 +77,8 @@
> static int simatic_ipc_leds_gpio_probe(struct platform_device *pdev) 
>  	switch (plat->devmode) {
>  	case SIMATIC_IPC_DEVICE_127E:
> +		if (!IS_ENABLED(CONFIG_PINCTRL_BROXTON))
> +			return -ENODEV;
>  		simatic_ipc_led_gpio_table =
> &simatic_ipc_led_gpio_table_127e; break;
>  	case SIMATIC_IPC_DEVICE_227G:

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

[Lee Jones]

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

What happened in versions 1 through 3?  Please provide a change-log.

>  drivers/leds/simple/simatic-ipc-leds-gpio.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/leds/simple/simatic-ipc-leds-gpio.c b/drivers/leds/simple/simatic-ipc-leds-gpio.c
> index b9eeb8702df0..fb8d427837db 100644
> --- a/drivers/leds/simple/simatic-ipc-leds-gpio.c
> +++ b/drivers/leds/simple/simatic-ipc-leds-gpio.c
> @@ -77,6 +77,8 @@ static int simatic_ipc_leds_gpio_probe(struct platform_device *pdev)
>  
>  	switch (plat->devmode) {
>  	case SIMATIC_IPC_DEVICE_127E:
> +		if (!IS_ENABLED(CONFIG_PINCTRL_BROXTON))
> +			return -ENODEV;

I see that there is an unfortunate precedent for this in the lines
below.  However, I also see that the commit which added it was not
reviewed by Pavel.

This is an interesting problem, due to the different devices we're
attempting to support in this single driver using different
GPIO/PINCTRL drivers, which is unusual.  We usually resolve these kinds of
issues as a Kconfig 'depends' line which covers the whole driver.

Would 'depends GPIO_F7188X || PINCTRL_BROXTON' be a suitable
replacement, I wonder?  If it's possible for SIMATIC_IPC_DEVICE_127E to
be probing when only GPIO_F7188X is enabled?  If so, this would result
in the same scenario.

It also seems wrong for -EPROBE_DEFER to loop indefinitely.  Surely in
some valid circumstances dependencies are never satisfied?

>  		simatic_ipc_led_gpio_table = &simatic_ipc_led_gpio_table_127e;
>  		break;
>  	case SIMATIC_IPC_DEVICE_227G:
> -- 
> 2.35.1
> 

-- 
Lee Jones [李琼斯]

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

[Henning Schild]

Am Fri, 23 Dec 2022 11:58:13 +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> ---  
> 
> 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.

> >  drivers/leds/simple/simatic-ipc-leds-gpio.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/leds/simple/simatic-ipc-leds-gpio.c
> > b/drivers/leds/simple/simatic-ipc-leds-gpio.c index
> > b9eeb8702df0..fb8d427837db 100644 ---
> > a/drivers/leds/simple/simatic-ipc-leds-gpio.c +++
> > b/drivers/leds/simple/simatic-ipc-leds-gpio.c @@ -77,6 +77,8 @@
> > static int simatic_ipc_leds_gpio_probe(struct platform_device
> > *pdev) switch (plat->devmode) {
> >  	case SIMATIC_IPC_DEVICE_127E:
> > +		if (!IS_ENABLED(CONFIG_PINCTRL_BROXTON))
> > +			return -ENODEV;  
> 
> I see that there is an unfortunate precedent for this in the lines
> below.  However, I also see that the commit which added it was not
> reviewed by Pavel.

Right i think that might have been you in the end.

> This is an interesting problem, due to the different devices we're
> attempting to support in this single driver using different
> GPIO/PINCTRL drivers, which is unusual.  We usually resolve these
> kinds of issues as a Kconfig 'depends' line which covers the whole
> driver.

This was tried but the result was not too nice. It is really the same
gpio led driver implemented on top of multiple possible gpio chip
drivers. Making it depend on both pulls in too much in case one wants a
minimal config, writing a new driver for each backend would duplicate
too much code.

But maybe a splitting out a -common or moving stuff into headers could
help with the duplication if we want to go the "one driver for one
device" road. I would not want that and what we currently see was
discussed and approved as part of another series, when i introduced
x27G.
 
> Would 'depends GPIO_F7188X || PINCTRL_BROXTON' be a suitable
> replacement, I wonder?  If it's possible for SIMATIC_IPC_DEVICE_127E
> to be probing when only GPIO_F7188X is enabled?  If so, this would
> result in the same scenario.

No that would not work. Depending on which board we are on we depend on
another pin provider. "&&" would be but it would be kind of overkill
and not allow for a minimal kernel config in case someone wanted a
special minimal kernel for either one.

> It also seems wrong for -EPROBE_DEFER to loop indefinitely.  Surely in
> some valid circumstances dependencies are never satisfied?

Well that is what i would guess as well. But that infinite loop
waiting for a pin to appear endlessly is a part of "leds-gpio". If
"leds-gpio" had some magic to eventually bail out (maybe say we give
it X runs with some sleep back-off) i would not have to do anything
here. I consider that patch a workaround for a shortcoming in
"leds-gpio", which busy loops and fills up your disk quickly with logs
if you mention a pin that never comes. Which i imagine can quickly
happen if you have a typo on your device tree or a kernel config not
enabling a pin provider driver.

I am not sure there are no other valid reasons. And i think that indef
loop needs fixing at some point. Hopefully by a LEDs maintainer or
maybe i will even help out.

Until that day i would like to have the proposed patch merged to not
have users run into a known issue. The pattern is established and has
been discussed before and the patch it rather trivial.

Later we can see about improving and ask fundamental questions again.

Henning

> >  		simatic_ipc_led_gpio_table =
> > &simatic_ipc_led_gpio_table_127e; break;
> >  	case SIMATIC_IPC_DEVICE_227G:
> > -- 
> > 2.35.1
> >   
> 

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

[Henning Schild]

Am Mon, 2 Jan 2023 16:22:27 +0100
schrieb Henning Schild <henning.schild@siemens.com>:

> Am Fri, 23 Dec 2022 11:58:13 +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> ---    
> > 
> > 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.

Henning

> > >  drivers/leds/simple/simatic-ipc-leds-gpio.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/leds/simple/simatic-ipc-leds-gpio.c
> > > b/drivers/leds/simple/simatic-ipc-leds-gpio.c index
> > > b9eeb8702df0..fb8d427837db 100644 ---
> > > a/drivers/leds/simple/simatic-ipc-leds-gpio.c +++
> > > b/drivers/leds/simple/simatic-ipc-leds-gpio.c @@ -77,6 +77,8 @@
> > > static int simatic_ipc_leds_gpio_probe(struct platform_device
> > > *pdev) switch (plat->devmode) {
> > >  	case SIMATIC_IPC_DEVICE_127E:
> > > +		if (!IS_ENABLED(CONFIG_PINCTRL_BROXTON))
> > > +			return -ENODEV;    
> > 
> > I see that there is an unfortunate precedent for this in the lines
> > below.  However, I also see that the commit which added it was not
> > reviewed by Pavel.  
> 
> Right i think that might have been you in the end.
> 
> > This is an interesting problem, due to the different devices we're
> > attempting to support in this single driver using different
> > GPIO/PINCTRL drivers, which is unusual.  We usually resolve these
> > kinds of issues as a Kconfig 'depends' line which covers the whole
> > driver.  
> 
> This was tried but the result was not too nice. It is really the same
> gpio led driver implemented on top of multiple possible gpio chip
> drivers. Making it depend on both pulls in too much in case one wants
> a minimal config, writing a new driver for each backend would
> duplicate too much code.
> 
> But maybe a splitting out a -common or moving stuff into headers could
> help with the duplication if we want to go the "one driver for one
> device" road. I would not want that and what we currently see was
> discussed and approved as part of another series, when i introduced
> x27G.
>  
> > Would 'depends GPIO_F7188X || PINCTRL_BROXTON' be a suitable
> > replacement, I wonder?  If it's possible for SIMATIC_IPC_DEVICE_127E
> > to be probing when only GPIO_F7188X is enabled?  If so, this would
> > result in the same scenario.  
> 
> No that would not work. Depending on which board we are on we depend
> on another pin provider. "&&" would be but it would be kind of
> overkill and not allow for a minimal kernel config in case someone
> wanted a special minimal kernel for either one.
> 
> > It also seems wrong for -EPROBE_DEFER to loop indefinitely.  Surely
> > in some valid circumstances dependencies are never satisfied?  
> 
> Well that is what i would guess as well. But that infinite loop
> waiting for a pin to appear endlessly is a part of "leds-gpio". If
> "leds-gpio" had some magic to eventually bail out (maybe say we give
> it X runs with some sleep back-off) i would not have to do anything
> here. I consider that patch a workaround for a shortcoming in
> "leds-gpio", which busy loops and fills up your disk quickly with logs
> if you mention a pin that never comes. Which i imagine can quickly
> happen if you have a typo on your device tree or a kernel config not
> enabling a pin provider driver.
> 
> I am not sure there are no other valid reasons. And i think that indef
> loop needs fixing at some point. Hopefully by a LEDs maintainer or
> maybe i will even help out.
> 
> Until that day i would like to have the proposed patch merged to not
> have users run into a known issue. The pattern is established and has
> been discussed before and the patch it rather trivial.
> 
> Later we can see about improving and ask fundamental questions again.
> 
> Henning
> 
> > >  		simatic_ipc_led_gpio_table =
> > > &simatic_ipc_led_gpio_table_127e; break;
> > >  	case SIMATIC_IPC_DEVICE_227G:
> > > -- 
> > > 2.35.1
> > >     
> >   
> 

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@siemens.com>:
> 
> > Am Fri, 23 Dec 2022 11:58:13 +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> ---    
> > > 
> > > 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 [李琼斯]
 

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@kernel.org>:

> On Tue, 03 Jan 2023, Henning Schild wrote:
> 
> > Am Mon, 2 Jan 2023 16:22:27 +0100
> > schrieb Henning Schild <henning.schild@siemens.com>:
> >   
> > > Am Fri, 23 Dec 2022 11:58:13 +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> ---      
> > > > 
> > > > 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 [李琼斯]
>  

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

[Andy Shevchenko]

On Wed, Jan 04, 2023 at 03:39:24PM +0100, Henning Schild wrote:
> Am Wed, 4 Jan 2023 14:24:30 +0000
> schrieb Lee Jones <lee@kernel.org>:

...

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

A bit of offtopic here.

Are you able to get / fix / ... the firmware to work with the upstreamed
version of pin control driver for Intel Elkhart Lake?

(I'm asking this in terms of the https://bugzilla.kernel.org/show_bug.cgi?id=213365)

-- 
With Best Regards,
Andy Shevchenko

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

[Henning Schild]

Am Wed, 4 Jan 2023 17:51:33 +0200
schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

> On Wed, Jan 04, 2023 at 03:39:24PM +0100, Henning Schild wrote:
> > Am Wed, 4 Jan 2023 14:24:30 +0000
> > schrieb Lee Jones <lee@kernel.org>:  
> 
> ...
> 
> > As we speak i already have the third box to eventually support,
> > which will likely be similar but this time around with
> > PINCTRL_ELKHARTLAKE  
> 
> A bit of offtopic here.
> 
> Are you able to get / fix / ... the firmware to work with the
> upstreamed version of pin control driver for Intel Elkhart Lake?
> 
> (I'm asking this in terms of the
> https://bugzilla.kernel.org/show_bug.cgi?id=213365)
> 

I can not tell. At the moment i am in a Siemens internal review where i
see code that is not even close to being ready for upstream. Somewhat
open-coded again from what it looks like.

And i do not have the machine the code is for.

Let me say "it is complicated" but some point in time a device with
LEDs attached to PINCTRL_ELKHARTLAKE will be proposed. Likely by me,
when i hopefully have such a device on my desk.

Henning

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

[Andy Shevchenko]

On Wed, Jan 04, 2023 at 08:30:05PM +0100, Henning Schild wrote:
> Am Wed, 4 Jan 2023 17:51:33 +0200
> schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> > On Wed, Jan 04, 2023 at 03:39:24PM +0100, Henning Schild wrote:
> > > Am Wed, 4 Jan 2023 14:24:30 +0000
> > > schrieb Lee Jones <lee@kernel.org>:  

...

> > > As we speak i already have the third box to eventually support,
> > > which will likely be similar but this time around with
> > > PINCTRL_ELKHARTLAKE  
> > 
> > A bit of offtopic here.
> > 
> > Are you able to get / fix / ... the firmware to work with the
> > upstreamed version of pin control driver for Intel Elkhart Lake?
> > 
> > (I'm asking this in terms of the
> > https://bugzilla.kernel.org/show_bug.cgi?id=213365)
> > 
> 
> I can not tell. At the moment i am in a Siemens internal review where i
> see code that is not even close to being ready for upstream. Somewhat
> open-coded again from what it looks like.
> 
> And i do not have the machine the code is for.
> 
> Let me say "it is complicated" but some point in time a device with
> LEDs attached to PINCTRL_ELKHARTLAKE will be proposed. Likely by me,
> when i hopefully have such a device on my desk.

Thanks for the information.

Consider above just as a point to be aware of when you come to
the productization, so we won't need another pin control driver for
the same chip.

-- 
With Best Regards,
Andy Shevchenko

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

[Henning Schild]

Am Thu, 5 Jan 2023 11:35:48 +0200
schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:

> On Wed, Jan 04, 2023 at 08:30:05PM +0100, Henning Schild wrote:
> > Am Wed, 4 Jan 2023 17:51:33 +0200
> > schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>:  
> > > On Wed, Jan 04, 2023 at 03:39:24PM +0100, Henning Schild wrote:  
> > > > Am Wed, 4 Jan 2023 14:24:30 +0000
> > > > schrieb Lee Jones <lee@kernel.org>:    
> 
> ...
> 
> > > > As we speak i already have the third box to eventually support,
> > > > which will likely be similar but this time around with
> > > > PINCTRL_ELKHARTLAKE    
> > > 
> > > A bit of offtopic here.
> > > 
> > > Are you able to get / fix / ... the firmware to work with the
> > > upstreamed version of pin control driver for Intel Elkhart Lake?
> > > 
> > > (I'm asking this in terms of the
> > > https://bugzilla.kernel.org/show_bug.cgi?id=213365)
> > >   
> > 
> > I can not tell. At the moment i am in a Siemens internal review
> > where i see code that is not even close to being ready for
> > upstream. Somewhat open-coded again from what it looks like.
> > 
> > And i do not have the machine the code is for.
> > 
> > Let me say "it is complicated" but some point in time a device with
> > LEDs attached to PINCTRL_ELKHARTLAKE will be proposed. Likely by me,
> > when i hopefully have such a device on my desk.  
> 
> Thanks for the information.
> 
> Consider above just as a point to be aware of when you come to
> the productization, so we won't need another pin control driver for
> the same chip.

IIRC we talked about this before in some other thread and the solution
was taking a newer BIOS base version. And since i never heard about
this again i hope people did the right thing.

Henning

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

[Lee Jones]

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.

Please try to improve the whole situation for you next submission.

Applied, thanks.

-- 
Lee Jones [李琼斯]

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

[Henning Schild]

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.

> Applied, thanks.

Thanks!

Henning

> 

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

[Andy Shevchenko]

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

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

[Lee Jones]

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 [李琼斯]

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@gmail.com>:

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

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

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

[Andy Shevchenko]

On Tue, Jan 24, 2023 at 3:35 PM Henning Schild
<henning.schild@siemens.com> wrote:
> Am Tue, 24 Jan 2023 11:46:34 +0200
> schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:

...

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

If you put the proper defaults, you can get good results without
torturing distro configurations.

See how 8250 has been splitting over the time, we have +~5 new Kconfig
options and their defaults are to keep the current behaviour without
the user needing to do anything in their configurations.

-- 
With Best Regards,
Andy Shevchenko

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

[Henning Schild]

Am Tue, 24 Jan 2023 10:29:35 +0000
schrieb Lee Jones <lee@kernel.org>:

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

It has been a while and i would have to reproduce this. But basically
what happened is that i registered a leds-gpio platform device with a
lookup table, no errors returned and my "driver" would be done and
leds-gpio takes over.

All GPIO_LOOKUP_IDXs point to not yet exisiting pins, potentially never
existing when the providing driver never comes up. Now leds-gpio
internally tries again and again with a high frequency (busy?) and
printing stuff (would have to try again to see what).

I think one could modifiy any other leds-gpio and totally invalidate
the lookup table by introducing typos in the chip name of each entry.

But i will rty again and get back with a better description. Maybe the
bug was fixed in the meantime or i am doing something wrong when
registering that platform-device.

Henning

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

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

[Henning Schild]

Am Tue, 24 Jan 2023 14:52:48 +0100
schrieb Henning Schild <henning.schild@siemens.com>:

> Am Tue, 24 Jan 2023 10:29:35 +0000
> schrieb Lee Jones <lee@kernel.org>:
> 
> > 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?  
> 
> It has been a while and i would have to reproduce this. But basically
> what happened is that i registered a leds-gpio platform device with a
> lookup table, no errors returned and my "driver" would be done and
> leds-gpio takes over.
> 
> All GPIO_LOOKUP_IDXs point to not yet exisiting pins, potentially
> never existing when the providing driver never comes up. Now leds-gpio
> internally tries again and again with a high frequency (busy?) and
> printing stuff (would have to try again to see what).
> 
> I think one could modifiy any other leds-gpio and totally invalidate
> the lookup table by introducing typos in the chip name of each entry.
> 
> But i will rty again and get back with a better description. Maybe the
> bug was fixed in the meantime or i am doing something wrong when
> registering that platform-device.

I tried again and it turns out that my driver is to blaim. After
registering that leds-gpio it goes and initialized two more LED-related
pins. If those are not there i return a DEFER out of probe and that is
causing the loop. I will have to find a better way of dealing with
those two extra GPIOs and possible DEFERS on them.

gpiod = gpiod_get_index..
...
return PTR_ERR(gpiod);

is seems to be the real problem here

Sorry for the noise and thanks for asking several times, better patches
will follow. Ideas and pointers welcome.

Henning


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

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

[Henning Schild]

Am Tue, 24 Jan 2023 15:46:01 +0200
schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:

> On Tue, Jan 24, 2023 at 3:35 PM Henning Schild
> <henning.schild@siemens.com> wrote:
> > Am Tue, 24 Jan 2023 11:46:34 +0200
> > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:  
> 
> ...
> 
> > 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.  
> 
> If you put the proper defaults, you can get good results without
> torturing distro configurations.

Meaning i could try sending a patch to set "default m" for all three
SIEMENS_SIMATIC_IPC
SIEMENS_SIMATIC_IPC_WDT
LEDS_SIEMENS_SIMATIC_IPC

I was so far too shy for that. I would even go further and add
GPIO_F7188X
W83627HF_WDT

Henning

> See how 8250 has been splitting over the time, we have +~5 new Kconfig
> options and their defaults are to keep the current behaviour without
> the user needing to do anything in their configurations.
> 

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

[Andy Shevchenko]

On Wed, Jan 25, 2023 at 7:37 PM Henning Schild
<henning.schild@siemens.com> wrote:
> Am Tue, 24 Jan 2023 15:46:01 +0200
> schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
> > On Tue, Jan 24, 2023 at 3:35 PM Henning Schild
> > <henning.schild@siemens.com> wrote:
> > > Am Tue, 24 Jan 2023 11:46:34 +0200
> > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:

...

> > > 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.
> >
> > If you put the proper defaults, you can get good results without
> > torturing distro configurations.
>
> Meaning i could try sending a patch to set "default m" for all three
> SIEMENS_SIMATIC_IPC
> SIEMENS_SIMATIC_IPC_WDT
> LEDS_SIEMENS_SIMATIC_IPC

No, default to another symbol which will be the core part. Again, look
how 8250 is organized. There is no default m (at least new ones, I
don't remember about any historical defaults as such).

>
> I was so far too shy for that. I would even go further and add
> GPIO_F7188X
> W83627HF_WDT
>
> > See how 8250 has been splitting over the time, we have +~5 new Kconfig
> > options and their defaults are to keep the current behaviour without
> > the user needing to do anything in their configurations.


-- 
With Best Regards,
Andy Shevchenko

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

[Henning Schild]

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.

You will not have to. I now understood how to improve on that as i am
adding more variants needing more gpio controller drivers.

> Please try to improve the whole situation for you next submission.
> 
> Applied, thanks.

I hope this is still in the branches for a merge. It should be applied.
It does fix a problem but using a wrong pattern, but a pattern that is
already in use.

So this will fix 6.1 and above in the short term.

In the long term i will restructure to individual drivers which have a
clear dependency chain in Kconfig. I will use inheritance to arrive at
minimal code duplication and will use Kconfig switch default
inheritance to ease configuration.

Such restructuring patches will have to be written first, but they will
come. Either stand-alone or together with the next machine.

regards,
Henning

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

[Lee Jones]

On Thu, 02 Feb 2023, Henning Schild 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.
> 
> You will not have to. I now understood how to improve on that as i am
> adding more variants needing more gpio controller drivers.
> 
> > Please try to improve the whole situation for you next submission.
> > 
> > Applied, thanks.
> 
> I hope this is still in the branches for a merge. It should be applied.
> It does fix a problem but using a wrong pattern, but a pattern that is
> already in use.

What makes you think it's not applied?
 
> So this will fix 6.1 and above in the short term.
> 
> In the long term i will restructure to individual drivers which have a
> clear dependency chain in Kconfig. I will use inheritance to arrive at
> minimal code duplication and will use Kconfig switch default
> inheritance to ease configuration.
> 
> Such restructuring patches will have to be written first, but they will
> come. Either stand-alone or together with the next machine.

That's fine.  Whatever suits.

-- 
Lee Jones [李琼斯]

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

[Henning Schild]

Am Fri, 3 Feb 2023 07:59:04 +0000
schrieb Lee Jones <lee@kernel.org>:

> On Thu, 02 Feb 2023, Henning Schild 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.  
> > 
> > You will not have to. I now understood how to improve on that as i
> > am adding more variants needing more gpio controller drivers.
> >   
> > > Please try to improve the whole situation for you next submission.
> > > 
> > > Applied, thanks.  
> > 
> > I hope this is still in the branches for a merge. It should be
> > applied. It does fix a problem but using a wrong pattern, but a
> > pattern that is already in use.  
> 
> What makes you think it's not applied?

Because i had that other one potentially replacing it so it was maybe
called off. Good to know it was not stopped.

Henning

>  
> > So this will fix 6.1 and above in the short term.
> > 
> > In the long term i will restructure to individual drivers which
> > have a clear dependency chain in Kconfig. I will use inheritance to
> > arrive at minimal code duplication and will use Kconfig switch
> > default inheritance to ease configuration.
> > 
> > Such restructuring patches will have to be written first, but they
> > will come. Either stand-alone or together with the next machine.  
> 
> That's fine.  Whatever suits.
>