[PATCH v1 00/11] leds: st1202: Naming cleanups and pre-existing fixes

[PATCH v1 00/11] leds: st1202: Naming cleanups and pre-existing fixes

[Manuel Fombuena]

This series collects the issues identified during review of the two
previous ST1202 submissions, the pattern engine fix series and the
hardware-accelerated blink support:

https://lore.kernel.org/all/GV1PR08MB8497C0B898789BB73ACE6EE3C5F52@GV1PR08MB8497.eurprd08.prod.outlook.com/
https://lore.kernel.org/all/GV1PR08MB84974C7ACD911E66864C5008C5DA2@GV1PR08MB8497.eurprd08.prod.outlook.com/

Both left behind pre-existing problems that were out of scope at the
time and deferred to a follow-up. They are grouped here because they
share that origin, rather than being sent as a string of two and three
patch series.

Patches 1 to 4 are naming and readability only, with no functional
change; two of them address comments from the blink support review.
Patches 5 to 11 are fixes and carry Fixes tags.

Patch 5 is worth a note. The eight pattern duration registers are
chip-wide while the PWM registers are per-channel, so a channel can
inherit a sequencer timeline another channel programmed and then sit at
full brightness for the slots it never set up. It clears the channel's
own PWM for those slots rather than the shared durations, which would
have truncated a longer pattern still running elsewhere on the device.

Patches 6 and 7 belong together. Pattern brightness values were written
into the 12-bit PWM registers unscaled, so a step asking for full
brightness produced about 6% duty cycle; and the analog current register
was never programmed at all, so a channel whose current had been left at
zero stayed dark however bright the pattern. Together they make a
pattern step of N produce the same output as writing N to brightness,
which is what the documented hw_pattern range implies. Patch 7 takes the
current up in pattern_set() and releases it in pattern_clear(), so a
rejected hw_pattern write cannot leave a channel latched on, and a board
declaring a lower ceiling through max-brightness keeps it.

Patch 9 is the one worth the most attention. st1202_brightness_set()
takes a mutex and performs I2C transfers while installed as the
non-blocking brightness_set callback, so it can sleep in atomic context.
brightness_set_blocking is installed as well, but the core only falls
back to it when brightness_set is absent, and that callback ignored the
requested brightness in any case. Neither can be fixed without the other.

Patches 10 and 11 do for st1202_blink_set() what patches 5 and 7 do for
the pattern path.

blink_set() programs only the two slots its on and off cycle needs, and
leaves the other six at full scale. Those six are normally invisible
because their duration registers are zero. Durations are shared across
the chip, though, so another channel can set them again, and the
blinking channel then lights during steps it never programmed. This was
reproduced on hardware by blinking one channel and giving a second one a
four step pattern.

blink_set() also drives the current register to full scale, ignoring a
lower ceiling from the max-brightness property. Patch 11 makes it use
the same value the brightness and pattern paths already use.

Two points raised in earlier reviews are deliberately left alone:

  - brightness_set() programming every PWM slot to full scale, and so
    overriding a running pattern, is intended. It is what makes the
    brightness visible while the global sequencer runs, per
    commit 7cbe470366bd ("leds: st1202: Fix brightness having no effect
    while pattern mode is active"). Stopping the sequencer instead would
    halt the patterns of every other channel.

  - Only the pattern timing is shared; the per-channel PWM envelopes are
    independent, and running patterns on several channels at once is
    what the hardware is designed for. A channel whose pattern is
    shorter than the active timeline therefore keeps the longer period.
    Correcting that needs the driver to track sequencer usage across
    channels, which is outside the scope of this series.

Tested on LED1202 hardware over I2C, on a Linksys MX4200v2 router
running OpenWrt: brightness values reaching the current register, the
atomic-context path via the heartbeat trigger, hw_pattern on two
channels of one device with different pattern lengths, a pattern
programmed on a channel whose current had been left at zero, timer
trigger delay quantisation, and driver unbind/rebind cycles for the
firmware node reference counting. dmesg clean throughout.

Patch 6 was additionally checked at the register level with i2cget. Left
unscaled, a pattern step asking for full brightness programs 0x0FF of a
12-bit register, 6.25% duty cycle; with the patch it programs 0x0FFF.
The datasheet (DS12875 rev 2, Table 7) confirms the PWM register pair is
a plain little endian 12-bit value, low byte followed by the high
nibble. Its helper scales against max_brightness, clamps both ends of a
signed input, and guards the division, although the LED core never
registers a zero max_brightness.

Manuel Fombuena (11):
  leds: st1202: Correct the name of the prescaler conversion helper
  leds: st1202: Spell out the phase-shift configuration bit
  leds: st1202: Name pattern index variables consistently
  leds: st1202: Use u8 consistently for 8-bit values
  leds: st1202: Clear unused pattern slots in pattern_set()
  leds: st1202: Scale pattern brightness to the 12-bit PWM range
  leds: st1202: Program the channel current for hardware patterns
  leds: st1202: Take a reference on the LED firmware node
  leds: st1202: Do not set brightness from atomic context
  leds: st1202: Clear unused pattern slots in blink_set()
  leds: st1202: Honour max_brightness when blinking

 Documentation/leds/leds-st1202.rst |   3 +
 drivers/leds/leds-st1202.c         | 145 +++++++++++++++++++----------
 2 files changed, 101 insertions(+), 47 deletions(-)

-- 
2.55.0

[PATCH v1 01/11] leds: st1202: Correct the name of the prescaler conversion helper

[Manuel Fombuena]

st1202_prescalar_to_miliseconds() converts a duration in milliseconds
into the prescaler value written to the pattern duration registers, so
its name states the conversion the wrong way round. It also misspells
both "prescaler" and "milliseconds".

Rename it to st1202_milliseconds_to_prescaler(). No functional change.

Signed-off-by: Manuel Fombuena <fombuena@outlook.com>
Assisted-by: LLM
---
 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 ead144155849..5a2202fb4c22 100644
--- a/drivers/leds/leds-st1202.c
+++ b/drivers/leds/leds-st1202.c
@@ -85,7 +85,7 @@ static int st1202_write_reg(struct st1202_chip *chip, int reg, uint8_t val)
 	return ret;
 }
 
-static uint8_t st1202_prescalar_to_miliseconds(unsigned int value)
+static uint8_t st1202_milliseconds_to_prescaler(unsigned int value)
 {
 	return value / ST1202_MILLIS_PATTERN_DUR_MIN;
 }
@@ -126,7 +126,7 @@ static int st1202_duration_pattern_write(struct st1202_chip *chip, int pattern,
 					unsigned int value)
 {
 	return st1202_write_reg(chip, (ST1202_PATTERN_DUR + pattern),
-				st1202_prescalar_to_miliseconds(value));
+				st1202_milliseconds_to_prescaler(value));
 }
 
 static int __st1202_channel_set(struct st1202_chip *chip, int led_num, bool active)
-- 
2.55.0

Re: [PATCH v1 01/11] leds: st1202: Correct the name of the prescaler conversion helper

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] st1202_blink_set clobbers the state of all other active LEDs on the same chip, unintentionally turning them off.
--

commit 39a1d0b65cb7b4a9e52158952579297ea332318e
Author: Manuel Fombuena <fombuena@outlook.com>

leds: st1202: Correct the name of the prescaler conversion helper
    
This commit renames the prescaler conversion helper to correct typos and
accurately reflect the conversion direction from milliseconds to prescaler.
It makes no functional changes.

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index ead1441558497..5a2202fb4c223 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
> @@ -85,7 +85,7 @@ static int st1202_write_reg(struct st1202_chip *chip, int reg, uint8_t val)
>  	return ret;
>  }
>  
> -static uint8_t st1202_prescalar_to_miliseconds(unsigned int value)
> +static uint8_t st1202_milliseconds_to_prescaler(unsigned int value)
>  {
>  	return value / ST1202_MILLIS_PATTERN_DUR_MIN;
>  }

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but it was not addressed by the
follow-up patches in the series.

Does st1202_blink_set() unintentionally clobber the state of all other active
LEDs on the same chip?

drivers/leds/leds-st1202.c:st1202_blink_set() {
    ...
	/* Zero out PWM for all other active channels to prevent them from blinking */
	for (int chan = 0; chan < ST1202_MAX_LEDS; chan++) {
		if (!chip->leds[chan].is_active || chan == led->led_num)
			continue;

		ret = st1202_pwm_pattern_write(chip, chan, 0, LED_OFF);
    ...
}

If a user writes a constant brightness to LED A via sysfs attributes, and
then sets a timer trigger on LED B to make it blink, won't this loop
explicitly destroy the PWM state of LED A (such as the ST1202_PATTERN_PWM_FULL
written by st1202_led_set to maintain constant brightness)?

Since unprivileged user-space can trigger this by writing to independent
sysfs devices, they could unintentionally turn off other active channels on
the chip.

Is there a way to isolate the blinking configuration to a single LED without
wiping out the PWM values for the rest?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/GV1PR08MB8497422B7A44424C4968AEC1C5BD2@GV1PR08MB8497.eurprd08.prod.outlook.com?part=1

[PATCH v1 02/11] leds: st1202: Spell out the phase-shift configuration bit

[Manuel Fombuena]

ST1202_CONFIG_REG_SHFT is abbreviated to the point of being unreadable
and, unlike the neighbouring PATS and PATSR bits, carries no comment
explaining what it controls.

Rename it to ST1202_CONFIG_REG_PHASE_SHIFT and document it in the same
style as the other configuration bits, retaining the datasheet name in
the comment so it remains searchable.

The longer name no longer fits the existing layout in
st1202_led_pattern_set(), where the continuation line was already 106
columns wide. Re-wrap that register write to match the layout
st1202_blink_set() uses for the same operation and drop the redundant
parentheses around the bit mask.

No functional change.

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

diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
index 5a2202fb4c22..d8a04e96fb46 100644
--- a/drivers/leds/leds-st1202.c
+++ b/drivers/leds/leds-st1202.c
@@ -24,7 +24,8 @@
 #define ST1202_CONFIG_REG_PATS             BIT(7)
 /* PATSR: Pattern sequence runs (self-clear when sequence is finished) */
 #define ST1202_CONFIG_REG_PATSR            BIT(6)
-#define ST1202_CONFIG_REG_SHFT             BIT(3)
+/* SHFT: Phase-shift delay enable */
+#define ST1202_CONFIG_REG_PHASE_SHIFT      BIT(3)
 #define ST1202_DEV_ENABLE                  0x01
 #define ST1202_DEV_ENABLE_ON               BIT(0)
 #define ST1202_DEV_ENABLE_RESET            BIT(7)
@@ -210,7 +211,7 @@ static int st1202_led_pattern_clear(struct led_classdev *ldev)
 
 	guard(mutex)(&chip->lock);
 
-	ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_SHFT);
+	ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_PHASE_SHIFT);
 	if (ret != 0)
 		return ret;
 
@@ -246,7 +247,7 @@ static int st1202_led_pattern_set(struct led_classdev *ldev,
 
 	guard(mutex)(&chip->lock);
 
-	ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_SHFT);
+	ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_PHASE_SHIFT);
 	if (ret != 0)
 		return ret;
 
@@ -268,8 +269,9 @@ static int st1202_led_pattern_set(struct led_classdev *ldev,
 	if (ret != 0)
 		return ret;
 
-	ret = st1202_write_reg(chip, ST1202_CONFIG_REG, (ST1202_CONFIG_REG_PATSR |
-							ST1202_CONFIG_REG_PATS | ST1202_CONFIG_REG_SHFT));
+	ret = st1202_write_reg(chip, ST1202_CONFIG_REG,
+				ST1202_CONFIG_REG_PATSR | ST1202_CONFIG_REG_PATS |
+				ST1202_CONFIG_REG_PHASE_SHIFT);
 	if (ret != 0)
 		return ret;
 
@@ -299,7 +301,7 @@ static int st1202_blink_set(struct led_classdev *led_cdev,
 
 	guard(mutex)(&chip->lock);
 
-	ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_SHFT);
+	ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_PHASE_SHIFT);
 	if (ret)
 		return ret;
 
@@ -353,7 +355,7 @@ static int st1202_blink_set(struct led_classdev *led_cdev,
 
 	ret = st1202_write_reg(chip, ST1202_CONFIG_REG,
 				ST1202_CONFIG_REG_PATSR | ST1202_CONFIG_REG_PATS |
-				ST1202_CONFIG_REG_SHFT);
+				ST1202_CONFIG_REG_PHASE_SHIFT);
 	if (ret)
 		return ret;
 
@@ -419,7 +421,7 @@ static int st1202_setup(struct st1202_chip *chip)
 		return ret;
 
 	/* Enable phase-shift delay feature */
-	ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_SHFT);
+	ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_PHASE_SHIFT);
 	if (ret < 0)
 		return ret;
 
-- 
2.55.0

Re: [PATCH v1 02/11] leds: st1202: Spell out the phase-shift configuration bit

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Blinking an LED destructively zeroes out the PWM values for all other channels, permanently breaking steady-ON LEDs.
- [High] Hardware state clobbering: per-LED pattern operations corrupt global sequencer configuration and step durations.
--

commit 2c776e6568e7de2682a8142a57768cace68e6086
Author: Manuel Fombuena <fombuena@outlook.com>

leds: st1202: Spell out the phase-shift configuration bit

This commit renames ST1202_CONFIG_REG_SHFT to ST1202_CONFIG_REG_PHASE_SHIFT
to improve readability and properly document the phase-shift delay feature.
It also adjusts line wrapping in the register writes to adhere to column
limits without introducing functional changes.

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index 5a2202fb4c223..d8a04e96fb46c 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
[ ... ]
> @@ -210,7 +211,7 @@ static int st1202_led_pattern_clear(struct led_classdev *ldev)
>  
>  	guard(mutex)(&chip->lock);
>  
> -	ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_SHFT);
> +	ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_PHASE_SHIFT);

[Severity: High]
This is a pre-existing issue, but by writing only the phase-shift bit to the
global ST1202_CONFIG_REG, doesn't this unconditionally clear the PATS and
PATSR bits? 

Since the ST1202 chip appears to use a single global sequencer, clearing
these bits would instantly halt the sequencer for all other active LEDs on
the chip. Additionally, when setting patterns in st1202_led_pattern_set(),
the shared global ST1202_PATTERN_DUR step durations are unconditionally
overwritten. 

Does the driver need a way to coordinate or reject conflicting requests for
the global sequencer instead of silently clobbering the state of other LEDs?

[ ... ]
> @@ -299,7 +301,7 @@ static int st1202_blink_set(struct led_classdev *led_cdev,
>  
>  	guard(mutex)(&chip->lock);
>  
> -	ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_SHFT);
> +	ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_PHASE_SHIFT);

[Severity: High]
This isn't a bug introduced by this patch, but does st1202_blink_set()
inadvertently break steady-ON LEDs? 

When a given LED starts blinking, the function iterates over all other active
channels and unconditionally zero-fills their PWM slots to prevent them from
blinking:

st1202_pwm_pattern_write(chip, chan, 0, LED_OFF);
st1202_pwm_pattern_write(chip, chan, 1, LED_OFF);

If those other channels were previously configured to be steady ON, this zero
fill causes them to turn completely off during patterns 0 and 1. When the
blinking LED is later disabled, the original full PWM slots for the other
channels are never restored, which seems to leave them permanently disabled.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/GV1PR08MB8497422B7A44424C4968AEC1C5BD2@GV1PR08MB8497.eurprd08.prod.outlook.com?part=2

[PATCH v1 03/11] leds: st1202: Name pattern index variables consistently

[Manuel Fombuena]

The driver already names pattern indices "pattern" in the signatures of
st1202_pwm_pattern_write() and st1202_duration_pattern_write(), and
st1202_blink_set() follows the same convention. The remaining loops
abbreviate it to "patt", which matches nothing else in the file.

Rename those loop variables to "pattern". In st1202_led_pattern_set()
that name is taken by the array of pattern steps, so rename the
parameter to "patterns", which also reflects better that it is an array.

Two st1202_pwm_pattern_write() calls exceed 100 columns with the longer
name and are wrapped accordingly.

No functional change.

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

diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
index d8a04e96fb46..ddb8c6027706 100644
--- a/drivers/leds/leds-st1202.c
+++ b/drivers/leds/leds-st1202.c
@@ -177,8 +177,8 @@ static void st1202_brightness_set(struct led_classdev *led_cdev,
 
 	guard(mutex)(&chip->lock);
 
-	for (int patt = 0; patt < ST1202_MAX_PATTERNS; patt++)
-		st1202_pwm_pattern_write(chip, led->led_num, patt, ST1202_PATTERN_PWM_FULL);
+	for (int pattern = 0; pattern < ST1202_MAX_PATTERNS; pattern++)
+		st1202_pwm_pattern_write(chip, led->led_num, pattern, ST1202_PATTERN_PWM_FULL);
 	st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num, value);
 	__st1202_channel_set(chip, led->led_num, !!value);
 }
@@ -215,12 +215,13 @@ static int st1202_led_pattern_clear(struct led_classdev *ldev)
 	if (ret != 0)
 		return ret;
 
-	for (int patt = 0; patt < ST1202_MAX_PATTERNS; patt++) {
-		ret = st1202_pwm_pattern_write(chip, led->led_num, patt, ST1202_PATTERN_PWM_FULL);
+	for (int pattern = 0; pattern < ST1202_MAX_PATTERNS; pattern++) {
+		ret = st1202_pwm_pattern_write(chip, led->led_num, pattern,
+						ST1202_PATTERN_PWM_FULL);
 		if (ret != 0)
 			return ret;
 
-		ret = st1202_write_reg(chip, ST1202_PATTERN_DUR + patt, 0);
+		ret = st1202_write_reg(chip, ST1202_PATTERN_DUR + pattern, 0);
 		if (ret != 0)
 			return ret;
 	}
@@ -229,7 +230,7 @@ static int st1202_led_pattern_clear(struct led_classdev *ldev)
 }
 
 static int st1202_led_pattern_set(struct led_classdev *ldev,
-				struct led_pattern *pattern,
+				struct led_pattern *patterns,
 				u32 len, int repeat)
 {
 	struct st1202_led *led = cdev_to_st1202_led(ldev);
@@ -239,9 +240,9 @@ static int st1202_led_pattern_set(struct led_classdev *ldev,
 	if (len > ST1202_MAX_PATTERNS)
 		return -EINVAL;
 
-	for (int patt = 0; patt < len; patt++) {
-		if (pattern[patt].delta_t < ST1202_MILLIS_PATTERN_DUR_MIN ||
-				pattern[patt].delta_t > ST1202_MILLIS_PATTERN_DUR_MAX)
+	for (int pattern = 0; pattern < len; pattern++) {
+		if (patterns[pattern].delta_t < ST1202_MILLIS_PATTERN_DUR_MIN ||
+				patterns[pattern].delta_t > ST1202_MILLIS_PATTERN_DUR_MAX)
 			return -EINVAL;
 	}
 
@@ -251,12 +252,13 @@ static int st1202_led_pattern_set(struct led_classdev *ldev,
 	if (ret != 0)
 		return ret;
 
-	for (int patt = 0; patt < len; patt++) {
-		ret = st1202_pwm_pattern_write(chip, led->led_num, patt, pattern[patt].brightness);
+	for (int pattern = 0; pattern < len; pattern++) {
+		ret = st1202_pwm_pattern_write(chip, led->led_num, pattern,
+						patterns[pattern].brightness);
 		if (ret != 0)
 			return ret;
 
-		ret = st1202_duration_pattern_write(chip, patt, pattern[patt].delta_t);
+		ret = st1202_duration_pattern_write(chip, pattern, patterns[pattern].delta_t);
 		if (ret != 0)
 			return ret;
 	}
-- 
2.55.0

Re: [PATCH v1 03/11] leds: st1202: Name pattern index variables consistently

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Incomplete cleanup of remaining hardware pattern durations in st1202_led_pattern_set.
- [High] Global hardware duration registers are corrupted by independent LED pattern or blink requests.
- [High] st1202_blink_set inadvertently turns off continuously lit LEDs.
--

commit 46a022fdeb18fd45b309d2df4aa547f0116cf2c4
Author: Manuel Fombuena <fombuena@outlook.com>

leds: st1202: Name pattern index variables consistently

This commit renames the loop variables for pattern indices from "patt"
to "pattern" across several functions to maintain consistency. It also
renames the "pattern" array parameter to "patterns" in
st1202_led_pattern_set() to avoid shadowing.

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index d8a04e96fb46c..ddb8c60277067 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c

[ ... ]

> @@ -251,12 +252,13 @@ static int st1202_led_pattern_set(struct led_classdev *ldev,
>  	if (ret != 0)
>  		return ret;
>  
> -	for (int patt = 0; patt < len; patt++) {
> -		ret = st1202_pwm_pattern_write(chip, led->led_num, patt, pattern[patt].brightness);
> +	for (int pattern = 0; pattern < len; pattern++) {
> +		ret = st1202_pwm_pattern_write(chip, led->led_num, pattern,
> +						patterns[pattern].brightness);
>  		if (ret != 0)
>  			return ret;
>  
> -		ret = st1202_duration_pattern_write(chip, patt, pattern[patt].delta_t);
> +		ret = st1202_duration_pattern_write(chip, pattern, patterns[pattern].delta_t);

[Severity: High]
This isn't a bug introduced by this patch, but will this write clobber
any pattern durations set by other LEDs?

Since the ST1202_PATTERN_DUR registers are global to the chip, if userspace
sets a pattern on one LED, and then sets a pattern or blink on another LED,
it seems the second LED's duration writes will overwrite the first LED's
timing.

The driver appears to expose shared hardware state as independent without
-EBUSY synchronization, and this behavior remains unresolved at the end of
the patch series.

>  		if (ret != 0)
>  			return ret;
>  	}

[Severity: High]
This is a pre-existing issue, but does this function leave durations for
trailing slots intact?

The loop sets durations up to 'len', but finishes without zeroing the global
duration registers for slots 'len' through ST1202_MAX_PATTERNS - 1.

Could this cause the hardware sequencer to execute trailing steps from a
previous longer pattern? During these ghost steps, the LED would be off,
which could lead to unexpected dark pauses that break the timing of the new
shorter pattern. This can be triggered if userspace sets a long pattern
followed by a short pattern via sysfs on the same LED.

This behavior persists through the entire patch series (including patch 5
which only clears the PWM registers, failing to clear the duration registers).


Also, looking at st1202_blink_set() which is not modified in this patch:

drivers/leds/leds-st1202.c:st1202_blink_set() {
    ...
	/* Zero out PWM for all other active channels to prevent them from blinking */
	for (int chan = 0; chan < ST1202_MAX_LEDS; chan++) {
		if (!chip->leds[chan].is_active || chan == led->led_num)
			continue;

		ret = st1202_pwm_pattern_write(chip, chan, 0, LED_OFF);
		if (ret)
			return ret;

		ret = st1202_pwm_pattern_write(chip, chan, 1, LED_OFF);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but does st1202_blink_set()
inadvertently turn off continuously lit LEDs?

This unconditionally clears the first two pattern slots for all other channels,
which ruins the ST1202_PATTERN_PWM_FULL state set by st1202_led_set() and
forces them OFF.

If a second LED is set to a continuous brightness, and userspace requests a
blink on the first LED, st1202_blink_set() will overwrite the second LED's
slots 0 and 1 to LED_OFF. Since the global sequencer loops only over slots 0
and 1 during blink, the second LED will output 0 brightness.

This remains unresolved at the end of the patch series (including patch 10).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/GV1PR08MB8497422B7A44424C4968AEC1C5BD2@GV1PR08MB8497.eurprd08.prod.outlook.com?part=3

[PATCH v1 04/11] leds: st1202: Use u8 consistently for 8-bit values

[Manuel Fombuena]

The driver spells 8-bit unsigned values both as uint8_t and as u8, a
mix present since the driver was added. The register helpers and the
prescaler conversion use uint8_t, while the rest of the driver uses u8,
which is also the preferred type in kernel code.

Convert the remaining uint8_t uses to u8.

No functional change.

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

diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
index ddb8c6027706..90f69ac16714 100644
--- a/drivers/leds/leds-st1202.c
+++ b/drivers/leds/leds-st1202.c
@@ -59,7 +59,7 @@ static struct st1202_led *cdev_to_st1202_led(struct led_classdev *cdev)
 	return container_of(cdev, struct st1202_led, led_cdev);
 }
 
-static int st1202_read_reg(struct st1202_chip *chip, int reg, uint8_t *val)
+static int st1202_read_reg(struct st1202_chip *chip, int reg, u8 *val)
 {
 	struct device *dev = &chip->client->dev;
 	int ret;
@@ -70,11 +70,11 @@ static int st1202_read_reg(struct st1202_chip *chip, int reg, uint8_t *val)
 		return ret;
 	}
 
-	*val = (uint8_t)ret;
+	*val = (u8)ret;
 	return 0;
 }
 
-static int st1202_write_reg(struct st1202_chip *chip, int reg, uint8_t val)
+static int st1202_write_reg(struct st1202_chip *chip, int reg, u8 val)
 {
 	struct device *dev = &chip->client->dev;
 	int ret;
@@ -86,7 +86,7 @@ static int st1202_write_reg(struct st1202_chip *chip, int reg, uint8_t val)
 	return ret;
 }
 
-static uint8_t st1202_milliseconds_to_prescaler(unsigned int value)
+static u8 st1202_milliseconds_to_prescaler(unsigned int value)
 {
 	return value / ST1202_MILLIS_PATTERN_DUR_MIN;
 }
-- 
2.55.0

Re: [PATCH v1 04/11] leds: st1202: Use u8 consistently for 8-bit values

[sashiko-bot]

> The driver spells 8-bit unsigned values both as uint8_t and as u8, a
> mix present since the driver was added. The register helpers and the
> prescaler conversion use uint8_t, while the rest of the driver uses u8,
> which is also the preferred type in kernel code.
> 
> Convert the remaining uint8_t uses to u8.
> 
> No functional change.
> 
> Signed-off-by: Manuel Fombuena <fombuena@outlook.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/GV1PR08MB8497422B7A44424C4968AEC1C5BD2@GV1PR08MB8497.eurprd08.prod.outlook.com?part=4

[PATCH v1 05/11] leds: st1202: Clear unused pattern slots in pattern_set()

[Manuel Fombuena]

The pattern duration registers are chip-wide while the PWM registers are
per-channel, so a channel can end up driven by a sequencer timeline it
never programmed.

st1202_led_pattern_set() programs only the first len slots. The pattern
trigger calls pattern_clear() before pattern_set() only once a channel is
already in PATTERN_TYPE_HW mode, so the first hardware pattern written to
a given channel skips it. If another channel has already programmed a
longer pattern, durations [len, 8) are still set, and this channel's PWM
for those slots is whatever st1202_led_pattern_clear() left at probe.

That used to be LED_OFF, so a channel stayed dark for the slots it had
not programmed itself. Since commit d2ca0e2b6d64 ("leds: st1202: Set all
pattern PWM slots to full after clearing pattern") it is
ST1202_PATTERN_PWM_FULL, so the channel now plays its own steps and then
sits at full brightness for the rest of the sequence.

Zero this channel's PWM for the unused slots so it stays dark for the
remainder of the timeline, as st1202_blink_set() already does for the
channels it is not driving.

The duration registers are deliberately left alone. All twelve channels
share them, so clearing them would truncate a longer pattern still
running elsewhere on the same device. A channel whose pattern is shorter
than the active timeline therefore keeps the longer period; that follows
from having a single hardware sequencer and is not something this can
correct.

Fixes: d2ca0e2b6d64 ("leds: st1202: Set all pattern PWM slots to full after clearing pattern")
Signed-off-by: Manuel Fombuena <fombuena@outlook.com>
Assisted-by: LLM
---
 drivers/leds/leds-st1202.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
index 90f69ac16714..9985b4b60989 100644
--- a/drivers/leds/leds-st1202.c
+++ b/drivers/leds/leds-st1202.c
@@ -263,6 +263,12 @@ static int st1202_led_pattern_set(struct led_classdev *ldev,
 			return ret;
 	}
 
+	for (int pattern = len; pattern < ST1202_MAX_PATTERNS; pattern++) {
+		ret = st1202_pwm_pattern_write(chip, led->led_num, pattern, LED_OFF);
+		if (ret != 0)
+			return ret;
+	}
+
 	ret = st1202_write_reg(chip, ST1202_PATTERN_REP, repeat);
 	if (ret != 0)
 		return ret;
-- 
2.55.0

Re: [PATCH v1 05/11] leds: st1202: Clear unused pattern slots in pattern_set()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Activating a blink on one channel forcibly turns off all other active LED channels during the blink cycle.
- [High] Clearing or blinking a pattern on one LED abruptly terminates or corrupts hardware patterns running on all other LEDs due to shared sequencer registers.
--

commit f903bd8ba806fde42df12463784beaf198812cd9
Author: Manuel Fombuena <fombuena@outlook.com>

leds: st1202: Clear unused pattern slots in pattern_set()

This commit clears the PWM registers for unused slots in a pattern sequence,
setting them to LED_OFF. It fixes an issue where the channel's PWM for those
slots was left at ST1202_PATTERN_PWM_FULL, causing it to sit at full brightness
for the rest of the sequence. It leaves the shared duration registers alone to
avoid truncating longer patterns on other channels.

>     Zero this channel's PWM for the unused slots so it stays dark for the
>     remainder of the timeline, as st1202_blink_set() already does for the
>     channels it is not driving.

[Severity: High]
This isn't a bug introduced by this patch, but does st1202_blink_set()
corrupt the PWM state of independent channels?

When activating a blink on one channel, it forcibly turns off all other
active LED channels during the blink cycle by explicitly zeroing out PWM
slots 0 and 1 for all other active channels:

drivers/leds/leds-st1202.c:st1202_blink_set() {
...
	/* Zero out PWM for all other active channels to prevent them from blinking */
	for (int chan = 0; chan < ST1202_MAX_LEDS; chan++) {
		if (!chip->leds[chan].is_active || chan == led->led_num)
			continue;

		ret = st1202_pwm_pattern_write(chip, chan, 0, LED_OFF);
		if (ret)
			return ret;

		ret = st1202_pwm_pattern_write(chip, chan, 1, LED_OFF);
...
}

Because the ST1202 chip shares a single hardware sequencer, zeroing these
slots causes steady-state channels (which rely on full PWM across all slots)
to output zero and go completely dark while the sequencer loops.

>     The duration registers are deliberately left alone. All twelve channels
>     share them, so clearing them would truncate a longer pattern still
>     running elsewhere on the same device. A channel whose pattern is shorter
>     than the active timeline therefore keeps the longer period; that follows
>     from having a single hardware sequencer and is not something this can
>     correct.

[Severity: High]
This is a pre-existing issue, but does clearing a pattern on one LED abruptly
terminate or corrupt hardware patterns running on all other LEDs?

Calling st1202_led_pattern_clear() on one channel unconditionally clears the
sequencer enable bits (PATS and PATSR) in the chip-wide ST1202_CONFIG_REG,
and zeroes out the globally shared pattern duration registers
(ST1202_PATTERN_DUR):

drivers/leds/leds-st1202.c:st1202_led_pattern_clear() {
...
	ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_PHASE_SHIFT);
	if (ret != 0)
		return ret;

	for (int pattern = 0; pattern < ST1202_MAX_PATTERNS; pattern++) {
		ret = st1202_pwm_pattern_write(chip, led->led_num, pattern,
						ST1202_PATTERN_PWM_FULL);
		if (ret != 0)
			return ret;

		ret = st1202_write_reg(chip, ST1202_PATTERN_DUR + pattern, 0);
...
}

Because the sequencer is a single shared hardware resource, per-channel
operations shouldn't unconditionally halt it or wipe its timeline if other
channels are actively using it. Hardware patterns on independent channels
might instantly fail or skip steps.

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index 90f69ac167147..9985b4b609891 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
> @@ -263,6 +263,12 @@ static int st1202_led_pattern_set(struct led_classdev *ldev,
>  			return ret;
>  	}
>  
> +	for (int pattern = len; pattern < ST1202_MAX_PATTERNS; pattern++) {
> +		ret = st1202_pwm_pattern_write(chip, led->led_num, pattern, LED_OFF);
> +		if (ret != 0)
> +			return ret;
> +	}
> +
>  	ret = st1202_write_reg(chip, ST1202_PATTERN_REP, repeat);
>  	if (ret != 0)
>  		return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/GV1PR08MB8497422B7A44424C4968AEC1C5BD2@GV1PR08MB8497.eurprd08.prod.outlook.com?part=5

[PATCH v1 06/11] leds: st1202: Scale pattern brightness to the 12-bit PWM range

[Manuel Fombuena]

The LED1202 PWM registers are 12 bits wide, but st1202_led_pattern_set()
writes the 8-bit brightness value carried by struct led_pattern into
them unscaled. A pattern step asking for full brightness therefore
drives the channel at 255 out of 4095, about 6% duty cycle, so a pattern
is dimmer than the same brightness set directly.

Scale the value to the full PWM range before programming it.

Scale against max_brightness rather than the register maximum. The LED
core takes max_brightness from the DT max-brightness property, so a
board can declare a ceiling below full scale, and the pattern trigger
already rejects steps above it. Scaling against a hardcoded 255 would
leave such a board unable to reach its own declared maximum from the
pattern path while the brightness path reaches it.

Scaling divides by max_brightness. The LED core replaces a zero
max-brightness from the device tree with LED_FULL, so the divisor should
never be zero, but the helper returns zero rather than rely on that.
max_brightness is capped at U8_MAX, the driver's own maximum, so a
device tree declaring more than the part supports is treated as full
scale. struct led_pattern brightness is signed, so clamp both ends and
floor a negative value to LED_OFF rather than let it wrap.

Document that a step is relative to max_brightness, since the range
given in the documentation no longer holds when the device tree sets
max-brightness.

Fixes: 259230378c65 ("leds: Add LED1202 I2C driver")
Signed-off-by: Manuel Fombuena <fombuena@outlook.com>
Assisted-by: LLM
---
 Documentation/leds/leds-st1202.rst |  3 +++
 drivers/leds/leds-st1202.c         | 16 +++++++++++++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/Documentation/leds/leds-st1202.rst b/Documentation/leds/leds-st1202.rst
index ed32eb3a27d4..27f35cfbe534 100644
--- a/Documentation/leds/leds-st1202.rst
+++ b/Documentation/leds/leds-st1202.rst
@@ -20,6 +20,9 @@ brightness (PWM) and duration must be written to hw_pattern.
 - Min pattern duration: 22 ms
 - Max pattern duration: 5610 ms
 
+If the device tree sets max-brightness, that value replaces 255 as the
+upper limit, and a step equal to it drives the channel at full scale.
+
 The format of the hardware pattern values should be:
 "brightness duration brightness duration ..."
 
diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
index 9985b4b60989..5558ac9612a1 100644
--- a/drivers/leds/leds-st1202.c
+++ b/drivers/leds/leds-st1202.c
@@ -91,6 +91,16 @@ static u8 st1202_milliseconds_to_prescaler(unsigned int value)
 	return value / ST1202_MILLIS_PATTERN_DUR_MIN;
 }
 
+static u16 st1202_brightness_to_pwm(int brightness, unsigned int max_brightness)
+{
+	if (!max_brightness)
+		return 0;
+
+	brightness = clamp_t(int, brightness, LED_OFF, max_brightness);
+
+	return brightness * ST1202_PATTERN_PWM_FULL / max_brightness;
+}
+
 static int st1202_pwm_pattern_write(struct st1202_chip *chip, int led_num,
 				int pattern, unsigned int value)
 {
@@ -235,8 +245,11 @@ static int st1202_led_pattern_set(struct led_classdev *ldev,
 {
 	struct st1202_led *led = cdev_to_st1202_led(ldev);
 	struct st1202_chip *chip = led->chip;
+	unsigned int max_brightness;
 	int ret;
 
+	max_brightness = min_t(unsigned int, ldev->max_brightness, U8_MAX);
+
 	if (len > ST1202_MAX_PATTERNS)
 		return -EINVAL;
 
@@ -254,7 +267,8 @@ static int st1202_led_pattern_set(struct led_classdev *ldev,
 
 	for (int pattern = 0; pattern < len; pattern++) {
 		ret = st1202_pwm_pattern_write(chip, led->led_num, pattern,
-						patterns[pattern].brightness);
+					st1202_brightness_to_pwm(patterns[pattern].brightness,
+								max_brightness));
 		if (ret != 0)
 			return ret;
 
-- 
2.55.0

Re: [PATCH v1 06/11] leds: st1202: Scale pattern brightness to the 12-bit PWM range

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Race condition when starting the hardware pattern sequencer: the base current (ILED_REG0) is written after the sequence is enabled.

Pre-existing issues:
- [High] The hardware pattern durations and execution sequencer are globally shared, but the driver allows per-LED independent configurations, causing destructive interference.
--

commit eaa2a40ee6c9df54ecd9aef2a8ff245af05bc058
Author: Manuel Fombuena <fombuena@outlook.com>

leds: st1202: Scale pattern brightness to the 12-bit PWM range

The LED1202 PWM registers are 12 bits wide, but st1202_led_pattern_set()
writes the 8-bit brightness value carried by struct led_pattern into
them unscaled. A pattern step asking for full brightness therefore
drives the channel at 255 out of 4095, about 6% duty cycle, so a pattern
is dimmer than the same brightness set directly.

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index 9985b4b609891..5558ac9612a15 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
[ ... ]
> @@ -254,7 +267,8 @@ static int st1202_led_pattern_set(struct led_classdev *ldev,
>  
>  	for (int pattern = 0; pattern < len; pattern++) {
>  		ret = st1202_pwm_pattern_write(chip, led->led_num, pattern,
> -						patterns[pattern].brightness);
> +					st1202_brightness_to_pwm(patterns[pattern].brightness,
> +								max_brightness));
>  		if (ret != 0)
>  			return ret;

[Severity: High]
This is a pre-existing issue, but does this cause destructive interference
across independent LED channels?

The ST1202 chip has a single global sequencer and a shared set of pattern
duration registers (ST1202_PATTERN_DUR). The driver exposes multiple
independent LED channels to userspace.

When a hardware pattern or blink is configured on one LED via
st1202_led_pattern_set() or st1202_blink_set(), the driver unconditionally
overwrites the global duration registers:

drivers/leds/leds-st1202.c:st1202_duration_pattern_write() {
    ...
    return st1202_write_reg(chip, (ST1202_PATTERN_DUR + pattern),
                            st1202_milliseconds_to_prescaler(value));
}

Similarly, st1202_led_pattern_clear() clears the PATS bit in
ST1202_CONFIG_REG, which stops the global sequencer:

drivers/leds/leds-st1202.c:st1202_led_pattern_clear() {
    ...
    ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_PHASE_SHIFT);
    ...
}

Will starting or stopping a pattern on one LED corrupt or freeze the running
pattern on all other LEDs, leading to severely incorrect functional behavior
for multi-channel usage?

[Severity: Medium]
Is there a race condition when starting the hardware pattern sequencer?

In st1202_led_pattern_set(), the analog current scalar (max_brightness) for
the sequence is written after the sequence is enabled:

drivers/leds/leds-st1202.c:st1202_led_pattern_set() {
    ...
    ret = st1202_write_reg(chip, ST1202_CONFIG_REG,
                           ST1202_CONFIG_REG_PATSR | ST1202_CONFIG_REG_PATS |
                           ST1202_CONFIG_REG_PHASE_SHIFT);
    if (ret != 0)
        return ret;

    ret = st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num, max_brightness);
    ...
}

Because I2C transactions are relatively slow and the hardware sequencer runs
independently, will the initial steps of the new pattern execute using the
stale ILED_REG0 value (which may be 0 from a prior pattern_clear)?

Does this lead to a noticeable visual glitch where the first cycle of the
pattern is truncated or plays at an incorrect brightness? Should the base
current be fully programmed before initiating the hardware sequencer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/GV1PR08MB8497422B7A44424C4968AEC1C5BD2@GV1PR08MB8497.eurprd08.prod.outlook.com?part=6

[PATCH v1 07/11] leds: st1202: Program the channel current for hardware patterns

[Manuel Fombuena]

A channel's output is ILED x Pattern_PWM / 4095, so a pattern is only
visible while the analog current register holds a non-zero value.
st1202_led_pattern_set() programs the PWM slots and starts the sequencer
but never writes ILED, so a channel whose current is zero stays dark
while pattern_set() reports success.

Zero is reached by ordinary use. Writing 0 to brightness sets it, and so
does switching away from the pattern trigger, because
pattern_trig_deactivate() ends with led_set_brightness(LED_OFF). A
sequence as simple as

  echo none > trigger
  echo pattern > trigger
  echo "255 500 0 500" > hw_pattern

therefore programs a pattern that never lights up.

Program the current in pattern_set() and release it in pattern_clear(),
so the pattern path owns ILED for as long as a pattern is loaded.
Releasing it matters because the trigger calls pattern_clear() before it
parses a new pattern: without that, a rejected or empty hw_pattern write
would leave the channel at full current with every PWM slot at full
scale, latching the LED on. Clearing before the slots are raised also
avoids a brief full-brightness flash on every reprogram.

Release the current first in pattern_clear(), before the sequencer is
stopped, and write it last in pattern_set(), once the channel is enabled
and the sequencer started. An I2C error partway through then cannot
leave the current raised on a channel whose pattern is being cleared,
nor raise it for a pattern that failed to start.

The current is programmed to the same max_brightness the PWM values are
scaled against, so a pattern step of N produces the same output as
writing N to brightness, and a board declaring a lower ceiling through
the max-brightness property keeps it on both paths.

Note that this makes a hardware pattern take the channel to its maximum
current, so brightness reads back as max_brightness once a pattern is
loaded and as zero once it is cleared. The pattern's own values, not the
last brightness written, decide what the channel shows while a pattern
is running.

Fixes: 259230378c65 ("leds: Add LED1202 I2C driver")
Signed-off-by: Manuel Fombuena <fombuena@outlook.com>
Assisted-by: LLM
---
 drivers/leds/leds-st1202.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
index 5558ac9612a1..5042fe88fbff 100644
--- a/drivers/leds/leds-st1202.c
+++ b/drivers/leds/leds-st1202.c
@@ -221,6 +221,10 @@ static int st1202_led_pattern_clear(struct led_classdev *ldev)
 
 	guard(mutex)(&chip->lock);
 
+	ret = st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num, LED_OFF);
+	if (ret != 0)
+		return ret;
+
 	ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_PHASE_SHIFT);
 	if (ret != 0)
 		return ret;
@@ -297,6 +301,10 @@ static int st1202_led_pattern_set(struct led_classdev *ldev,
 	if (ret != 0)
 		return ret;
 
+	ret = st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num, max_brightness);
+	if (ret != 0)
+		return ret;
+
 	return 0;
 }
 
-- 
2.55.0

Re: [PATCH v1 07/11] leds: st1202: Program the channel current for hardware patterns

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] `st1202_led_pattern_clear` and `st1202_led_pattern_set` manage the global sequencer state and shared duration registers on a per-channel basis, corrupting other active patterns.
--

commit e1c407fc5be28a96fa9e3c96385b607f0ca18728
Author: Manuel Fombuena <fombuena@outlook.com>

leds: st1202: Program the channel current for hardware patterns

This commit modifies the hardware pattern setup to properly program the
analog current register (ILED) so that programmed patterns are visible. It
also ensures the current is released when a pattern is cleared to avoid
latching the LED on.

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index 5558ac9612a15..5042fe88fbff7 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
> @@ -221,6 +221,10 @@ static int st1202_led_pattern_clear(struct led_classdev *ldev)
>  
>  	guard(mutex)(&chip->lock);
>  
> +	ret = st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num, LED_OFF);
> +	if (ret != 0)
> +		return ret;
> +
>  	ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_PHASE_SHIFT);
>  	if (ret != 0)
>  		return ret;

[Severity: High]
This is a pre-existing issue, but does writing ST1202_CONFIG_REG_PHASE_SHIFT
here abruptly halt the shared global sequencer for all channels?

Since st1202_led_pattern_clear() and st1202_led_pattern_set() are executed on
a per-channel basis via led_cdev.pattern_clear, clearing the PATS and PATSR
bits in the shared ST1202_CONFIG_REG appears to corrupt the state machine and
stop any hardware patterns currently executing on other LEDs on the same chip.

Additionally, further down in st1202_led_pattern_clear(), there is a loop
that writes 0 to ST1202_PATTERN_DUR + pattern. Can this zero out the chip-wide
global duration registers and destroy the pattern sequence timings for all
other active channels?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/GV1PR08MB8497422B7A44424C4968AEC1C5BD2@GV1PR08MB8497.eurprd08.prod.outlook.com?part=7

[PATCH v1 08/11] leds: st1202: Take a reference on the LED firmware node

[Manuel Fombuena]

st1202_dt_init() stores each LED's firmware node while walking the
device tree with for_each_available_child_of_node_scoped(), which drops
its reference to the node at the end of every iteration. The stored
pointer is not used until st1202_probe() registers the LED class devices
in a separate loop, by which point the driver holds no reference to the
node at all.

With CONFIG_OF_DYNAMIC the node can be freed in between, leaving
led->fwnode dangling and causing a use-after-free when it is handed to
devm_led_classdev_register_ext(). The LED class stores the pointer with
device_set_node() and does not take a reference of its own, so the node
has to stay alive for as long as the LED device does.

Take a reference when the node is stored and release it through a devm
action. The action is registered before the LED class devices are, so it
runs after they have been unregistered.

Fixes: c72e455b89f2 ("leds: leds-st1202: Fix NULL pointer access on race condition")
Signed-off-by: Manuel Fombuena <fombuena@outlook.com>
Assisted-by: LLM
---
 drivers/leds/leds-st1202.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
index 5042fe88fbff..600e004944a6 100644
--- a/drivers/leds/leds-st1202.c
+++ b/drivers/leds/leds-st1202.c
@@ -12,6 +12,7 @@
 #include <linux/i2c.h>
 #include <linux/leds.h>
 #include <linux/module.h>
+#include <linux/property.h>
 #include <linux/slab.h>
 #include <linux/string.h>
 
@@ -395,6 +396,11 @@ static int st1202_blink_set(struct led_classdev *led_cdev,
 	return 0;
 }
 
+static void st1202_fwnode_put(void *data)
+{
+	fwnode_handle_put(data);
+}
+
 static int st1202_dt_init(struct st1202_chip *chip)
 {
 	struct device *dev = &chip->client->dev;
@@ -414,7 +420,11 @@ static int st1202_dt_init(struct st1202_chip *chip)
 
 		led = &chip->leds[reg];
 		led->is_active = true;
-		led->fwnode = of_fwnode_handle(child);
+		led->fwnode = fwnode_handle_get(of_fwnode_handle(child));
+
+		err = devm_add_action_or_reset(dev, st1202_fwnode_put, led->fwnode);
+		if (err)
+			return err;
 
 		led->led_cdev.max_brightness = U8_MAX;
 		led->led_cdev.brightness_set_blocking = st1202_led_set;
-- 
2.55.0

Re: [PATCH v1 08/11] leds: st1202: Take a reference on the LED firmware node

[sashiko-bot]

> st1202_dt_init() stores each LED's firmware node while walking the
> device tree with for_each_available_child_of_node_scoped(), which drops
> its reference to the node at the end of every iteration. The stored
> pointer is not used until st1202_probe() registers the LED class devices
> in a separate loop, by which point the driver holds no reference to the
> node at all.
> 
> With CONFIG_OF_DYNAMIC the node can be freed in between, leaving
> led->fwnode dangling and causing a use-after-free when it is handed to
> devm_led_classdev_register_ext(). The LED class stores the pointer with
> device_set_node() and does not take a reference of its own, so the node
> has to stay alive for as long as the LED device does.
> 
> Take a reference when the node is stored and release it through a devm
> action. The action is registered before the LED class devices are, so it
> runs after they have been unregistered.
> 
> Fixes: c72e455b89f2 ("leds: leds-st1202: Fix NULL pointer access on race condition")
> Signed-off-by: Manuel Fombuena <fombuena@outlook.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/GV1PR08MB8497422B7A44424C4968AEC1C5BD2@GV1PR08MB8497.eurprd08.prod.outlook.com?part=8

[PATCH v1 09/11] leds: st1202: Do not set brightness from atomic context

[Manuel Fombuena]

st1202_brightness_set() takes the chip mutex and performs I2C transfers,
both of which can sleep, but it is installed as the brightness_set
callback. led_set_brightness_nopm() calls that op directly on the
assumption that it never sleeps, including from softirq context, so the
driver can sleep in atomic context.

The driver also installs brightness_set_blocking, but the core only
falls back to it when brightness_set is absent, so st1202_led_set() was
never reached. It merely turned the channel on or off and ignored the
requested brightness, so dropping brightness_set on its own would have
lost brightness control entirely.

Move the register programming into st1202_led_set() so the blocking
callback honours the brightness value, and drop brightness_set. The core
now defers the update to a work queue, where sleeping is allowed. Errors
from the register writes are propagated rather than discarded.

Programming every PWM slot to full scale is kept: it is what makes the
brightness visible while the global sequencer is running, without having
to stop the sequencer and disturb the other channels.

Fixes: 259230378c65 ("leds: Add LED1202 I2C driver")
Signed-off-by: Manuel Fombuena <fombuena@outlook.com>
Assisted-by: LLM
---
 drivers/leds/leds-st1202.c | 38 ++++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
index 600e004944a6..1267671557f0 100644
--- a/drivers/leds/leds-st1202.c
+++ b/drivers/leds/leds-st1202.c
@@ -180,20 +180,6 @@ static int st1202_channel_set(struct st1202_chip *chip, int led_num, bool active
 	return __st1202_channel_set(chip, led_num, active);
 }
 
-static void st1202_brightness_set(struct led_classdev *led_cdev,
-				enum led_brightness value)
-{
-	struct st1202_led *led = cdev_to_st1202_led(led_cdev);
-	struct st1202_chip *chip = led->chip;
-
-	guard(mutex)(&chip->lock);
-
-	for (int pattern = 0; pattern < ST1202_MAX_PATTERNS; pattern++)
-		st1202_pwm_pattern_write(chip, led->led_num, pattern, ST1202_PATTERN_PWM_FULL);
-	st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num, value);
-	__st1202_channel_set(chip, led->led_num, !!value);
-}
-
 static enum led_brightness st1202_brightness_get(struct led_classdev *led_cdev)
 {
 	struct st1202_led *led = cdev_to_st1202_led(led_cdev);
@@ -210,8 +196,29 @@ static enum led_brightness st1202_brightness_get(struct led_classdev *led_cdev)
 static int st1202_led_set(struct led_classdev *ldev, enum led_brightness value)
 {
 	struct st1202_led *led = cdev_to_st1202_led(ldev);
+	struct st1202_chip *chip = led->chip;
+	int ret;
+
+	guard(mutex)(&chip->lock);
+
+	/*
+	 * The output of a channel is ILED x Pattern_PWM / 4095. Setting every
+	 * PWM slot to full scale makes it equal ILED whatever the state of the
+	 * sequencer, so the brightness takes effect without stopping the
+	 * sequencer, which is global and would disturb the other channels.
+	 */
+	for (int pattern = 0; pattern < ST1202_MAX_PATTERNS; pattern++) {
+		ret = st1202_pwm_pattern_write(chip, led->led_num, pattern,
+						ST1202_PATTERN_PWM_FULL);
+		if (ret)
+			return ret;
+	}
+
+	ret = st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num, value);
+	if (ret)
+		return ret;
 
-	return st1202_channel_set(led->chip, led->led_num, !!value);
+	return __st1202_channel_set(chip, led->led_num, !!value);
 }
 
 static int st1202_led_pattern_clear(struct led_classdev *ldev)
@@ -432,7 +439,6 @@ static int st1202_dt_init(struct st1202_chip *chip)
 		led->led_cdev.pattern_clear = st1202_led_pattern_clear;
 		led->led_cdev.default_trigger = "pattern";
 		led->led_cdev.blink_set = st1202_blink_set;
-		led->led_cdev.brightness_set = st1202_brightness_set;
 		led->led_cdev.brightness_get = st1202_brightness_get;
 	}
 
-- 
2.55.0

Re: [PATCH v1 09/11] leds: st1202: Do not set brightness from atomic context

[sashiko-bot]

> st1202_brightness_set() takes the chip mutex and performs I2C transfers,
> both of which can sleep, but it is installed as the brightness_set
> callback. led_set_brightness_nopm() calls that op directly on the
> assumption that it never sleeps, including from softirq context, so the
> driver can sleep in atomic context.
> 
> The driver also installs brightness_set_blocking, but the core only
> falls back to it when brightness_set is absent, so st1202_led_set() was
> never reached. It merely turned the channel on or off and ignored the
> requested brightness, so dropping brightness_set on its own would have
> lost brightness control entirely.
> 
> Move the register programming into st1202_led_set() so the blocking
> callback honours the brightness value, and drop brightness_set. The core
> now defers the update to a work queue, where sleeping is allowed. Errors
> from the register writes are propagated rather than discarded.
> 
> Programming every PWM slot to full scale is kept: it is what makes the
> brightness visible while the global sequencer is running, without having
> to stop the sequencer and disturb the other channels.
> 
> Fixes: 259230378c65 ("leds: Add LED1202 I2C driver")
> Signed-off-by: Manuel Fombuena <fombuena@outlook.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/GV1PR08MB8497422B7A44424C4968AEC1C5BD2@GV1PR08MB8497.eurprd08.prod.outlook.com?part=9

[PATCH v1 10/11] leds: st1202: Clear unused pattern slots in blink_set()

[Manuel Fombuena]

st1202_blink_set() programs slots 0 and 1 of its own channel and leaves
slots 2 to 7 at ST1202_PATTERN_PWM_FULL, the value both st1202_led_set()
and st1202_led_pattern_clear() leave behind. Nothing keeps those slots
out of the output except the duration registers being zero, and those
are shared by all twelve channels.

Another channel programming a hardware pattern longer than two steps
writes those durations again and re-arms the sequencer, so the blinking
channel drives full scale for the extra steps. With two LEDs on one
device, blinking one and giving the other a four step pattern, the
blinking channel lights during the two steps it never programmed.

Zero the channel's own unused slots, the same invariant
st1202_led_pattern_set() applies to the slots beyond its pattern length.
blink_set() already zeroes durations 2 to 7 when it runs; the slots are
exposed only when another channel sets those shared durations again.

A channel whose blink is shorter than the active timeline still loses
its own timing, because the sequencer has one set of duration registers.
This only stops it emitting output it never asked for.

Fixes: 35cbd34a6a05 ("leds: st1202: Add hardware-accelerated blink support")
Signed-off-by: Manuel Fombuena <fombuena@outlook.com>
Assisted-by: LLM
---
 drivers/leds/leds-st1202.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
index 1267671557f0..b93828fbbe26 100644
--- a/drivers/leds/leds-st1202.c
+++ b/drivers/leds/leds-st1202.c
@@ -361,9 +361,11 @@ static int st1202_blink_set(struct led_classdev *led_cdev,
 	if (ret)
 		return ret;
 
-	ret = st1202_pwm_pattern_write(chip, led->led_num, 1, LED_OFF);
-	if (ret)
-		return ret;
+	for (int pattern = 1; pattern < ST1202_MAX_PATTERNS; pattern++) {
+		ret = st1202_pwm_pattern_write(chip, led->led_num, pattern, LED_OFF);
+		if (ret)
+			return ret;
+	}
 
 	ret = st1202_duration_pattern_write(chip, 0, on);
 	if (ret)
-- 
2.55.0

Re: [PATCH v1 10/11] leds: st1202: Clear unused pattern slots in blink_set()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] st1202_blink_set() incorrectly zeroes out PWM slots for other active channels, breaking solid-on LEDs and causing cross-channel interference.
- [High] st1202_led_pattern_clear() and st1202_blink_set() destructively clear shared global sequencer duration registers, halting hardware patterns on other channels.
--

commit a162d0d169d87855db4320cf80b6e7c657d7bddc
Author: Manuel Fombuena <fombuena@outlook.com>

leds: st1202: Clear unused pattern slots in blink_set()

The patch clears unused pattern slots (2 to 7) in st1202_blink_set() by
zeroing them out for the channel. This prevents a blinking channel from
emitting unrequested output when another channel programs longer hardware
patterns and re-arms the global sequencer.

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index 1267671557f09..b93828fbbe26c 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
> @@ -361,9 +361,11 @@ static int st1202_blink_set(struct led_classdev *led_cdev,

[Severity: High]
This isn't a bug introduced by this patch, but does st1202_blink_set()
corrupt the PWM state of independent, non-blinking channels?

Earlier in drivers/leds/leds-st1202.c:st1202_blink_set(), the code zeros
out other active channels:

	/* Zero out PWM for all other active channels to prevent them from blinking */
	for (int chan = 0; chan < ST1202_MAX_LEDS; chan++) {
		if (!chip->leds[chan].is_active || chan == led->led_num)
			continue;

		ret = st1202_pwm_pattern_write(chip, chan, 0, LED_OFF);
		if (ret)
			return ret;

		ret = st1202_pwm_pattern_write(chip, chan, 1, LED_OFF);
		if (ret)
			return ret;
	}

Because solid-on LEDs rely on having all their PWM slots set to
ST1202_PATTERN_PWM_FULL, overwriting their slots 0 and 1 to LED_OFF causes
them to incorrectly turn dark during the first two steps of the global
sequencer's cycle when any LED is set to blink mode via sysfs.

>  	if (ret)
>  		return ret;
>  
> -	ret = st1202_pwm_pattern_write(chip, led->led_num, 1, LED_OFF);
> -	if (ret)
> -		return ret;
> +	for (int pattern = 1; pattern < ST1202_MAX_PATTERNS; pattern++) {
> +		ret = st1202_pwm_pattern_write(chip, led->led_num, pattern, LED_OFF);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	ret = st1202_duration_pattern_write(chip, 0, on);
>  	if (ret)

[Severity: High]
This is a pre-existing issue, but do st1202_led_pattern_clear() and
st1202_blink_set() destructively clear shared global sequencer duration
registers?

Later in drivers/leds/leds-st1202.c:st1202_blink_set(), the duration registers
are reset to zero for all slots:

	for (int pattern = 2; pattern < ST1202_MAX_PATTERNS; pattern++) {
		ret = st1202_write_reg(chip, ST1202_PATTERN_DUR + pattern, 0);
		if (ret)
			return ret;
	}

And similarly in drivers/leds/leds-st1202.c:st1202_led_pattern_clear():

	ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_PHASE_SHIFT);
	if (ret != 0)
		return ret;
	...
		ret = st1202_write_reg(chip, ST1202_PATTERN_DUR + pattern, 0);

Since the ST1202 features a single, globally shared sequencer and duration
register set, zeroing these registers for one channel via sysfs truncates the
shared timeline. Does this prematurely halt or break all other channels that
are simultaneously using hardware blinking or patterns?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/GV1PR08MB8497422B7A44424C4968AEC1C5BD2@GV1PR08MB8497.eurprd08.prod.outlook.com?part=10

[PATCH v1 11/11] leds: st1202: Honour max_brightness when blinking

[Manuel Fombuena]

st1202_blink_set() drives the channel current register to U8_MAX, the
full scale of the part, regardless of the ceiling the board declared.
led_classdev_register_ext() takes max_brightness from the device tree
max-brightness property. The driver does not implement led-max-microamp,
so that is the only way a board can ask for less than 20 mA.

The brightness and pattern paths both respect it. Blinking is the one
that does not, so a board that declares a lower ceiling holds it until
something enables the timer trigger.

Program the same value the other paths use, capped at the width of the
current register so an out of range property cannot wrap it to zero.

Write it last, once the channel is enabled and the sequencer started, as
st1202_led_pattern_set() does, so an I2C error partway through does not
raise the current for a blink that failed to start.

Fixes: 35cbd34a6a05 ("leds: st1202: Add hardware-accelerated blink support")
Signed-off-by: Manuel Fombuena <fombuena@outlook.com>
Assisted-by: LLM
---
 drivers/leds/leds-st1202.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
index b93828fbbe26..2114ef8a3119 100644
--- a/drivers/leds/leds-st1202.c
+++ b/drivers/leds/leds-st1202.c
@@ -385,10 +385,6 @@ static int st1202_blink_set(struct led_classdev *led_cdev,
 	if (ret)
 		return ret;
 
-	ret = st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num, U8_MAX);
-	if (ret)
-		return ret;
-
 	ret = __st1202_channel_set(chip, led->led_num, true);
 	if (ret)
 		return ret;
@@ -399,6 +395,11 @@ static int st1202_blink_set(struct led_classdev *led_cdev,
 	if (ret)
 		return ret;
 
+	ret = st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num,
+				min_t(unsigned int, led_cdev->max_brightness, U8_MAX));
+	if (ret)
+		return ret;
+
 	*delay_on = on;
 	*delay_off = off;
 
-- 
2.55.0

Re: [PATCH v1 11/11] leds: st1202: Honour max_brightness when blinking

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Blinking one channel forcibly turns off all other active channels due to a shared global sequencer zeroing out their PWM slots.
- [High] st1202_led_set() passes the requested brightness directly to a u8 register write without capping it, causing silent integer truncation that can turn the LED off.
--

commit 3ae7cb7beb88cd5d234d548399ec8a5dcd6d48e8
Author: Manuel Fombuena <fombuena@outlook.com>

leds: st1202: Honour max_brightness when blinking

This commit updates the blinking path to respect the max-brightness property
defined in the device tree, capping the value to the width of the register.
It ensures the current is only raised after the channel is enabled and the
sequencer is started to prevent raising current for a failed blink.

>     The brightness and pattern paths both respect it. Blinking is the one
>     that does not, so a board that declares a lower ceiling holds it until
>     something enables the timer trigger.

[Severity: High]
This isn't a bug introduced by this patch, but does the brightness path
actually respect the maximum brightness properly?

Looking at st1202_led_set(), the value is directly passed to the register
write:

drivers/leds/leds-st1202.c:st1202_led_set() {
    ...
    ret = st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num, value);
    if (ret)
        return ret;
    ...
}

Because st1202_write_reg() takes a u8 for the value, if a device tree
specifies a max-brightness greater than 255 (e.g., 256), a requested
brightness of 256 will silently truncate to 0 and turn the led off instead
of setting it to max.

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index b93828fbbe26c..2114ef8a31194 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
> @@ -385,10 +385,6 @@ static int st1202_blink_set(struct led_classdev *led_cdev,

[Severity: High]
This is a pre-existing issue, but does blinking one led forcibly break the
state of all other active leds?

Looking earlier in st1202_blink_set():

drivers/leds/leds-st1202.c:st1202_blink_set() {
    ...
    /* Zero out PWM for all other active channels to prevent them from blinking */
    for (int chan = 0; chan < ST1202_MAX_LEDS; chan++) {
        if (!chip->leds[chan].is_active || chan == led->led_num)
            continue;

        ret = st1202_pwm_pattern_write(chip, chan, 0, LED_OFF);
        if (ret)
            return ret;

        ret = st1202_pwm_pattern_write(chip, chan, 1, LED_OFF);
        if (ret)
            return ret;
    }
    ...
}

If a user configures one led to be continuously on and then configures a
second led to blink, won't this loop overwrite the first led's pwm slots
and turn it off during both sequencer phases?

If the driver left the other active leds' pwm slots at
ST1202_PATTERN_PWM_FULL, wouldn't they correctly remain solidly on without
blinking while the global sequencer runs?

>  	if (ret)
>  		return ret;
>  
> -	ret = st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num, U8_MAX);
> -	if (ret)
> -		return ret;
> -
>  	ret = __st1202_channel_set(chip, led->led_num, true);
>  	if (ret)
>  		return ret;
> @@ -399,6 +395,11 @@ static int st1202_blink_set(struct led_classdev *led_cdev,
>  	if (ret)
>  		return ret;
>  
> +	ret = st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num,
> +				min_t(unsigned int, led_cdev->max_brightness, U8_MAX));
> +	if (ret)
> +		return ret;
> +
>  	*delay_on = on;
>  	*delay_off = off;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/GV1PR08MB8497422B7A44424C4968AEC1C5BD2@GV1PR08MB8497.eurprd08.prod.outlook.com?part=11