[PATCH] backlight: ktd2801: fix LED dependency

[PATCH] backlight: ktd2801: fix LED dependency

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

The new backlight driver unconditionally selects LEDS_EXPRESSWIRE, which
is in a different subsystem that may be disabled here:

WARNING: unmet direct dependencies detected for LEDS_EXPRESSWIRE
  Depends on [n]: NEW_LEDS [=n] && GPIOLIB [=y]
  Selected by [y]:
  - BACKLIGHT_KTD2801 [=y] && HAS_IOMEM [=y] && BACKLIGHT_CLASS_DEVICE [=y]

Change the select to depends, to ensure the indirect dependency is
met as well even when LED support is disabled.

Fixes: 66c76c1cd984 ("backlight: Add Kinetic KTD2801 backlight support")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/video/backlight/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index 230bca07b09d..f83f9ef037fc 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -185,7 +185,7 @@ config BACKLIGHT_KTD253
 
 config BACKLIGHT_KTD2801
 	tristate "Backlight Driver for Kinetic KTD2801"
-	select LEDS_EXPRESSWIRE
+	depends on LEDS_EXPRESSWIRE
 	help
 	  Say Y to enable the backlight driver for the Kinetic KTD2801 1-wire
 	  GPIO-controlled backlight found in Samsung Galaxy Core Prime VE LTE.
-- 
2.39.2

Re: [PATCH] backlight: ktd2801: fix LED dependency

[Daniel Thompson]

On Mon, Feb 12, 2024 at 12:18:12PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> The new backlight driver unconditionally selects LEDS_EXPRESSWIRE, which
> is in a different subsystem that may be disabled here:
>
> WARNING: unmet direct dependencies detected for LEDS_EXPRESSWIRE
>   Depends on [n]: NEW_LEDS [=n] && GPIOLIB [=y]
>   Selected by [y]:
>   - BACKLIGHT_KTD2801 [=y] && HAS_IOMEM [=y] && BACKLIGHT_CLASS_DEVICE [=y]
>
> Change the select to depends, to ensure the indirect dependency is
> met as well even when LED support is disabled.
>
> Fixes: 66c76c1cd984 ("backlight: Add Kinetic KTD2801 backlight support")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/video/backlight/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index 230bca07b09d..f83f9ef037fc 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -185,7 +185,7 @@ config BACKLIGHT_KTD253
>
>  config BACKLIGHT_KTD2801
>  	tristate "Backlight Driver for Kinetic KTD2801"
> -	select LEDS_EXPRESSWIRE
> +	depends on LEDS_EXPRESSWIRE

As far as I can tell this resolves the warning by making it impossible
to enable BACKLIGHT_KTD2801 unless a largely unrelated driver
(LEDS_KTD2692) is also enabled!

A better way to resolve this problem might be to eliminate the NEW_LEDS
dependency entirely:
~~~
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 64bb2de237e95..a08816cde78ae 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -186,10 +186,6 @@ config LEDS_EL15203000
          To compile this driver as a module, choose M here: the module
          will be called leds-el15203000.

-config LEDS_EXPRESSWIRE
-       bool
-       depends on GPIOLIB
-
 config LEDS_TURRIS_OMNIA
        tristate "LED support for CZ.NIC's Turris Omnia"
        depends on LEDS_CLASS_MULTICOLOR
@@ -936,3 +932,10 @@ comment "Simple LED drivers"
 source "drivers/leds/simple/Kconfig"

 endif # NEW_LEDS
+
+# This is library code that is useful for LEDs but can be enable/disabled
+# independently of NEW_LEDS. In fact it must be independent so it can be
+# selected from other sub-systems.
+config LEDS_EXPRESSWIRE
+       bool
+       depends on GPIOLIB
~~~

Alternatively we could add a "depends on NEW_LEDS" alongside the
existing select or just make LEDS_EXPRESSWIRE user selectable.

It also looks like we should put back the GPIOLIB dependency to both
 KTD2801 and KTD2692... and I'll take a mea-culpa for providing bad
 advice during the review cycles!


Daniel.

Re: [PATCH] backlight: ktd2801: fix LED dependency

[Duje Mihanović]

On Monday, February 12, 2024 1:44:28 PM CET Daniel Thompson wrote:
> On Mon, Feb 12, 2024 at 12:18:12PM +0100, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> > 
> > The new backlight driver unconditionally selects LEDS_EXPRESSWIRE, which
> > is in a different subsystem that may be disabled here:
> > 
> > WARNING: unmet direct dependencies detected for LEDS_EXPRESSWIRE
> > 
> >   Depends on [n]: NEW_LEDS [=n] && GPIOLIB [=y]
> >   Selected by [y]:
> >   - BACKLIGHT_KTD2801 [=y] && HAS_IOMEM [=y] && BACKLIGHT_CLASS_DEVICE 
[=y]
> > 
> > Change the select to depends, to ensure the indirect dependency is
> > met as well even when LED support is disabled.
> > 
> > Fixes: 66c76c1cd984 ("backlight: Add Kinetic KTD2801 backlight support")
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > ---
> > 
> >  drivers/video/backlight/Kconfig | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/video/backlight/Kconfig
> > b/drivers/video/backlight/Kconfig index 230bca07b09d..f83f9ef037fc 100644
> > --- a/drivers/video/backlight/Kconfig
> > +++ b/drivers/video/backlight/Kconfig
> > @@ -185,7 +185,7 @@ config BACKLIGHT_KTD253
> > 
> >  config BACKLIGHT_KTD2801
> >  
> >  	tristate "Backlight Driver for Kinetic KTD2801"
> > 
> > -	select LEDS_EXPRESSWIRE
> > +	depends on LEDS_EXPRESSWIRE
> 
> As far as I can tell this resolves the warning by making it impossible
> to enable BACKLIGHT_KTD2801 unless a largely unrelated driver
> (LEDS_KTD2692) is also enabled!
> 
> A better way to resolve this problem might be to eliminate the NEW_LEDS
> dependency entirely:

I believe this would be the best thing to do here. Making LEDS_EXPRESSWIRE 
user selectable doesn't make much sense to me as the library is rather low-
level (a quick grep turns up BTREE as an example of something similar) and IMO 
the GPIOLIB dependency should be handled by LEDS_EXPRESSWIRE as it's the one 
actually using the GPIO interface (except maybe for KTD2692 as it has some 
extra GPIOs not present in the other one and thus handles them itself).

Regards,
-- 
Duje

Re: [PATCH] backlight: ktd2801: fix LED dependency

[Arnd Bergmann]

On Mon, Feb 12, 2024, at 15:31, Duje Mihanović wrote:
> On Monday, February 12, 2024 1:44:28 PM CET Daniel Thompson wrote:
>> On Mon, Feb 12, 2024 at 12:18:12PM +0100, Arnd Bergmann wrote:
> I believe this would be the best thing to do here. Making LEDS_EXPRESSWIRE 
> user selectable doesn't make much sense to me as the library is rather low-
> level (a quick grep turns up BTREE as an example of something similar) and IMO 
> the GPIOLIB dependency should be handled by LEDS_EXPRESSWIRE as it's the one 
> actually using the GPIO interface (except maybe for KTD2692 as it has some 
> extra GPIOs not present in the other one and thus handles them itself).

Agree, let's do it this way. Maybe the leds-expresswire.c file should
not be in drivers/leds either, but it's already there and I can't think
of a better place for it.so just adapting Kconfig should be enough.

Please add the corresponding Makefile change as well though:

--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -135,7 +135,7 @@ obj-$(CONFIG_CPU_IDLE)              += cpuidle/
 obj-y                          += mmc/
 obj-y                          += ufs/
 obj-$(CONFIG_MEMSTICK)         += memstick/
-obj-$(CONFIG_NEW_LEDS)         += leds/
+obj-y                          += leds/
 obj-$(CONFIG_INFINIBAND)       += infiniband/
 obj-y                          += firmware/
 obj-$(CONFIG_CRYPTO)           += crypto/

Without this, the expresswire library module won't
get built unless NEW_LEDS is enabled.

     Arnd

Re: [PATCH] backlight: ktd2801: fix LED dependency

[Daniel Thompson]

On Mon, Feb 12, 2024 at 03:31:50PM +0100, Duje Mihanović wrote:
> On Monday, February 12, 2024 1:44:28 PM CET Daniel Thompson wrote:
> > On Mon, Feb 12, 2024 at 12:18:12PM +0100, Arnd Bergmann wrote:
> > > From: Arnd Bergmann <arnd@arndb.de>
> > >
> > > The new backlight driver unconditionally selects LEDS_EXPRESSWIRE, which
> > > is in a different subsystem that may be disabled here:
> > >
> > > WARNING: unmet direct dependencies detected for LEDS_EXPRESSWIRE
> > >
> > >   Depends on [n]: NEW_LEDS [=n] && GPIOLIB [=y]
> > >   Selected by [y]:
> > >   - BACKLIGHT_KTD2801 [=y] && HAS_IOMEM [=y] && BACKLIGHT_CLASS_DEVICE
> [=y]
> > >
> > > Change the select to depends, to ensure the indirect dependency is
> > > met as well even when LED support is disabled.
> > >
> > > Fixes: 66c76c1cd984 ("backlight: Add Kinetic KTD2801 backlight support")
> > > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > > ---
> > >
> > >  drivers/video/backlight/Kconfig | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/video/backlight/Kconfig
> > > b/drivers/video/backlight/Kconfig index 230bca07b09d..f83f9ef037fc 100644
> > > --- a/drivers/video/backlight/Kconfig
> > > +++ b/drivers/video/backlight/Kconfig
> > > @@ -185,7 +185,7 @@ config BACKLIGHT_KTD253
> > >
> > >  config BACKLIGHT_KTD2801
> > >
> > >  	tristate "Backlight Driver for Kinetic KTD2801"
> > >
> > > -	select LEDS_EXPRESSWIRE
> > > +	depends on LEDS_EXPRESSWIRE
> >
> > As far as I can tell this resolves the warning by making it impossible
> > to enable BACKLIGHT_KTD2801 unless a largely unrelated driver
> > (LEDS_KTD2692) is also enabled!
> >
> > A better way to resolve this problem might be to eliminate the NEW_LEDS
> > dependency entirely:
>
> I believe this would be the best thing to do here. Making LEDS_EXPRESSWIRE
> user selectable doesn't make much sense to me as the library is rather low-
> level (a quick grep turns up BTREE as an example of something similar) and IMO
> the GPIOLIB dependency should be handled by LEDS_EXPRESSWIRE as it's the one
> actually using the GPIO interface (except maybe for KTD2692 as it has some
> extra GPIOs not present in the other one and thus handles them itself).

We can keep the GPIOLIB dependency in LEDS_EXPRESSWIRE but it also needs
to be included in the KTD2801 KConfig too... otherwise we'll get similar
problems to the ones Arnd addressed here:
https://lore.kernel.org/all/20240213165602.2230970-1-arnd@kernel.org/


Daniel.