| Message ID | 20221007153323.1326-1-henning.schild@siemens.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] leds: simatic-ipc-leds-gpio: make sure we have the GPIO providing driver | expand |
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:
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 >
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 > > >
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 > > > > > >
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 [李琼斯]
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 [李琼斯] >
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)
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
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.
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
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.
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 >
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) ...
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.
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
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.
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. >
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. >
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. >
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.
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
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.
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. >
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:
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(+)