diff mbox series

[3/3] leds: Kconfig: leds-st1202: add select for required LEDS_TRIGGER_PATTERN

Message ID CWLP123MB5473F4DF3A668F7DD057A280C5C22@CWLP123MB5473.GBRP123.PROD.OUTLOOK.COM
State New
Headers show
Series [1/3] leds: leds-st1202: initialize hardware before DT node child operations | expand

Commit Message

Manuel Fombuena Feb. 26, 2025, 5:12 p.m. UTC
leds-st1202 requires the LED Pattern Trigger (LEDS_TRIGGER_PATTERN), which
is not selected when LED Trigger support is (LEDS_TRIGGERS).

To reproduce this:

- make menuconfig KCONFIG_CONFIG=
- select LEDS_ST1202 dependencies OF, I2C and LEDS_CLASS.
- select LEDS_ST1202
- LEDS_TRIGGERS is selected but LEDS_TRIGGER_PATTERN isn't.

The absence of LEDS_TRIGGER_PATTERN explicitly required can lead to builds
in which LEDS_ST1202 is selected while LEDS_TRIGGER_PATTERN isn't. The direct
result of that would be that /sys/class/leds/<led>/hw_pattern wouldn't be
available and there would be no way of interacting with the driver and
hardware from user space.

Add select LEDS_TRIGGER_PATTERN to Kconfig to meet the requirement and
indirectly document it as well.

Signed-off-by: Manuel Fombuena <fombuena@outlook.com>
---
 drivers/leds/Kconfig | 1 +
 1 file changed, 1 insertion(+)

Comments

Lee Jones March 6, 2025, 11:47 p.m. UTC | #1
On Wed, 26 Feb 2025, Manuel Fombuena wrote:

> leds-st1202 requires the LED Pattern Trigger (LEDS_TRIGGER_PATTERN), which
> is not selected when LED Trigger support is (LEDS_TRIGGERS).
> 
> To reproduce this:
> 
> - make menuconfig KCONFIG_CONFIG=
> - select LEDS_ST1202 dependencies OF, I2C and LEDS_CLASS.
> - select LEDS_ST1202
> - LEDS_TRIGGERS is selected but LEDS_TRIGGER_PATTERN isn't.
> 
> The absence of LEDS_TRIGGER_PATTERN explicitly required can lead to builds
> in which LEDS_ST1202 is selected while LEDS_TRIGGER_PATTERN isn't. The direct
> result of that would be that /sys/class/leds/<led>/hw_pattern wouldn't be
> available and there would be no way of interacting with the driver and
> hardware from user space.
> 
> Add select LEDS_TRIGGER_PATTERN to Kconfig to meet the requirement and
> indirectly document it as well.
> 
> Signed-off-by: Manuel Fombuena <fombuena@outlook.com>
> ---
>  drivers/leds/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 2b27d043921c..8859e8fe292a 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -971,6 +971,7 @@ config LEDS_ST1202
>  	depends on I2C
>  	depends on OF
>  	select LEDS_TRIGGERS
> +	select LEDS_TRIGGER_PATTERN

Don't you need both?

>  	help
>  	  Say Y to enable support for LEDs connected to LED1202
>  	  LED driver chips accessed via the I2C bus.
> -- 
> 2.48.1
>
Manuel Fombuena March 7, 2025, 11:34 a.m. UTC | #2
On Thu, 6 Mar 2025, Lee Jones wrote:

> On Wed, 26 Feb 2025, Manuel Fombuena wrote:
> > --- a/drivers/leds/Kconfig
> > +++ b/drivers/leds/Kconfig
> > @@ -971,6 +971,7 @@ config LEDS_ST1202
> >  	depends on I2C
> >  	depends on OF
> >  	select LEDS_TRIGGERS
> > +	select LEDS_TRIGGER_PATTERN
> 
> Don't you need both?

Not sure I understand the question, sorry. The patch adds 'select 
LEDS_TRIGGER_PATTERN' to the existing 'select LEDS_TRIGGERS'. So yes, you 
need both.

If the question were meant to be 'Do you need both?' the answer would have 
also been yes. Having only 'select LEDS_TRIGGER_PATTERN' doesn't select 
LEDS_TRIGGERS, and it would be even worse because you wouldn't have any of 
them. I tried that first, in case select LEDS_TRIGGER_PATTERN implicitly 
selects LEDS_TRIGGER, but it doesn't work like that.

--
Manuel Fombuena
Lee Jones March 7, 2025, 3:30 p.m. UTC | #3
On Fri, 07 Mar 2025, Manuel Fombuena wrote:

> On Thu, 6 Mar 2025, Lee Jones wrote:
> 
> > On Wed, 26 Feb 2025, Manuel Fombuena wrote:
> > > --- a/drivers/leds/Kconfig
> > > +++ b/drivers/leds/Kconfig
> > > @@ -971,6 +971,7 @@ config LEDS_ST1202
> > >  	depends on I2C
> > >  	depends on OF
> > >  	select LEDS_TRIGGERS
> > > +	select LEDS_TRIGGER_PATTERN
> > 
> > Don't you need both?
> 
> Not sure I understand the question, sorry. The patch adds 'select 
> LEDS_TRIGGER_PATTERN' to the existing 'select LEDS_TRIGGERS'. So yes, you 
> need both.
> 
> If the question were meant to be 'Do you need both?' the answer would have 
> also been yes. Having only 'select LEDS_TRIGGER_PATTERN' doesn't select 
> LEDS_TRIGGERS, and it would be even worse because you wouldn't have any of 
> them. I tried that first, in case select LEDS_TRIGGER_PATTERN implicitly 
> selects LEDS_TRIGGER, but it doesn't work like that.

Never mind.  I misread the diff.
diff mbox series

Patch

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 2b27d043921c..8859e8fe292a 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -971,6 +971,7 @@  config LEDS_ST1202
 	depends on I2C
 	depends on OF
 	select LEDS_TRIGGERS
+	select LEDS_TRIGGER_PATTERN
 	help
 	  Say Y to enable support for LEDs connected to LED1202
 	  LED driver chips accessed via the I2C bus.