[PATCH 0/3] leds-st1202 improvements

[PATCH 0/3] leds-st1202 improvements

[Manuel Fombuena]

Following the feedback received on the set of patches for leds-st1202 that
I sent previously, I am sending separately for your consideration those
that are not fixes with an improved cover letter that hopefully is more
aligned with the guidelines.

[PATCH 1/3] leds: leds-st1202: initialize hardware before DT node

The purpose of this change is to make leds-st1202 initialization more
robust. The underlying idea is that a hardware initialization
failure is somewhat more likely to happen than merely filling a
led_classdev struct.

Currently, st1202_dt_init() is called first to fill the led_classdev
struct. Afterwards, st1202_setup() is called to initialize the hardware,
but it doesn't require any return from st1202_dt_init() or depends on
anything done by it to do that. This has been inferred reviewing the code
and corroborated with testing on an actual device switching the order
of the calls.

The present calling order can lead to a situation in which the hardware
fails to initialize while the led_classdev struct has been filled.

It is important to note that this situation would be more severe if a
patch that I submitted previously [1] was not applied, as in that case
devm_led_classdev_register_ext would be in st1202_dt_init and a later
failure on st1202_setup would mean that the kobjects are available in user
space but the hardware is in failed state.

While I think this change makes an improvement and I can't foresee any
issue, not applying the patch would have no consequences under normal
circumstances.

[1] https://lore.kernel.org/all/CWLP123MB54732771AC0CE5491B3C84DCC5C32@CWLP123MB5473.GBRP123.PROD.OUTLOOK.COM/

[PATCH 2/3] leds: leds-st1202: spacing and proofreading editing

These are minor changes to polish the format of a comment for consistency,
correct a typo that comes straight from the datasheet, and add punctuation
for readability.

There would be no consequences if the patch was not applied.

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

leds-st1202 requires the LED pattern trigger to work. Without it, there
would be no /sys/class/leds/<led>/hw_pattern to write the patterns to and
no way to interact with the LEDs.

With such requirement, it doesn't feel prudent to leave the selection
of LEDS_TRIGGER_PATTERN effectively on it being already selected,
since nothing on leds-st1202's Kconfig does it.

To reproduce the potential issue that I am trying to explain:

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

Without this requirement explicitly selected, the resulting kernel won't
include the LED pattern trigger so the consequence of not applying
this patch would be that downstream projects and users would need to
figure the requirement out, if their starting base .config file doesn't
have LEDS_TRIGGER_PATTERN already selected.

Manuel Fombuena (3):
  leds: leds-st1202: initialize hardware before DT node child operations
  leds: leds-st1202: spacing and proofreading editing
  leds: Kconfig: leds-st1202: add select for required
    LEDS_TRIGGER_PATTERN

 drivers/leds/Kconfig       |  1 +
 drivers/leds/leds-st1202.c | 14 +++++++-------
 2 files changed, 8 insertions(+), 7 deletions(-)

-- 
2.48.1

[PATCH 1/3] leds: leds-st1202: initialize hardware before DT node child operations

[Manuel Fombuena]

Arguably, there are more chances of errors occurring during the
initialization of the hardware, so this should complete successfully
before the devicetree node's children are initialized.

st1202_dt_init() fills the led_classdev struct.

st1202_setup() initializes the hardware. Specifically, resets the chip,
enables its phase-shift delay feature, enables the device and disables all
the LEDs channels. All that writing to registers, with no input from
st1202_dt_init().

Real-world testing corroborates that calling st1202_setup() before
st1202_dt_init() doesn't cause any issue during initialization.

Switch the order of st1202_dt_init() and st1202_setup() to ensure the
hardware is correctly initialized before the led_classdev struct is
filled.

Signed-off-by: Manuel Fombuena <fombuena@outlook.com>
---
 drivers/leds/leds-st1202.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
index 9f275f7fb159..e7dce8c26bde 100644
--- a/drivers/leds/leds-st1202.c
+++ b/drivers/leds/leds-st1202.c
@@ -349,11 +349,11 @@ static int st1202_probe(struct i2c_client *client)
 		return ret;
 	chip->client = client;
 
-	ret = st1202_dt_init(chip);
+	ret = st1202_setup(chip);
 	if (ret < 0)
 		return ret;
 
-	ret = st1202_setup(chip);
+	ret = st1202_dt_init(chip);
 	if (ret < 0)
 		return ret;
 
-- 
2.48.1

[PATCH 2/3] leds: leds-st1202: spacing and proofreading editing

[Manuel Fombuena]

Minor edits regarding use of spacing and proofreading.

There is a minor inconsistency in the use of spacing as margin in
one of the comments providing details about the datasheet.

There is also a typo that comes from the datasheet itself.

Change spacing on comment and correct typo.

Signed-off-by: Manuel Fombuena <fombuena@outlook.com>
---
 drivers/leds/leds-st1202.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
index e7dce8c26bde..4e5dd76d714d 100644
--- a/drivers/leds/leds-st1202.c
+++ b/drivers/leds/leds-st1202.c
@@ -99,9 +99,9 @@ static int st1202_pwm_pattern_write(struct st1202_chip *chip, int led_num,
 	value_h = (u8)(value >> 8);
 
 	/*
-	 *  Datasheet: Register address low = 1Eh + 2*(xh) + 18h*(yh),
-	 *  where x is the channel number (led number) in hexadecimal (x = 00h .. 0Bh)
-	 *  and y is the pattern number in hexadecimal (y = 00h .. 07h)
+	 * Datasheet: Register address low = 1Eh + 2*(xh) + 18h*(yh),
+	 * where x is the channel number (led number) in hexadecimal (x = 00h .. 0Bh)
+	 * and y is the pattern number in hexadecimal (y = 00h .. 07h)
 	 */
 	ret = st1202_write_reg(chip, (ST1202_PATTERN_PWM + (led_num * 2) + 0x18 * pattern),
 				value_l);
@@ -287,8 +287,8 @@ static int st1202_setup(struct st1202_chip *chip)
 	guard(mutex)(&chip->lock);
 
 	/*
-	 * Once the supply voltage is applied, the LED1202 executes some internal checks,
-	 * afterwords it stops the oscillator and puts the internal LDO in quiescent mode.
+	 * Once the supply voltage is applied, the LED1202 executes some internal checks.
+	 * Afterwards, it stops the oscillator and puts the internal LDO in quiescent mode.
 	 * To start the device, EN bit must be set inside the “Device Enable” register at
 	 * address 01h. As soon as EN is set, the LED1202 loads the adjustment parameters
 	 * from the internal non-volatile memory and performs an auto-calibration procedure
-- 
2.48.1

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

[Manuel Fombuena]

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
 	help
 	  Say Y to enable support for LEDs connected to LED1202
 	  LED driver chips accessed via the I2C bus.
-- 
2.48.1

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

[Lee Jones]

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
> 

-- 
Lee Jones [李琼斯]

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

[Manuel Fombuena]

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

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

[Lee Jones]

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.

-- 
Lee Jones [李琼斯]

Re: [PATCH 0/3] leds-st1202 improvements

[Lee Jones]

On Wed, 26 Feb 2025 17:04:40 +0000, Manuel Fombuena wrote:
> Following the feedback received on the set of patches for leds-st1202 that
> I sent previously, I am sending separately for your consideration those
> that are not fixes with an improved cover letter that hopefully is more
> aligned with the guidelines.
> 
> [PATCH 1/3] leds: leds-st1202: initialize hardware before DT node
> 
> [...]

Applied, thanks!

[1/3] leds: leds-st1202: initialize hardware before DT node child operations
      commit: 8a2727615f9865b69998b3162a76a20c46f97866
[2/3] leds: leds-st1202: spacing and proofreading editing
      commit: 2aa76efbbe375be31e0a4db88ef52b4a85c0038e
[3/3] leds: Kconfig: leds-st1202: add select for required LEDS_TRIGGER_PATTERN
      commit: d884bb02e14e39e9f269b50b349c37caec001bec

--
Lee Jones [李琼斯]