[PATCH v6 0/9] leds: st1202: fix multiple bugs in pattern engine and brightness handling

[PATCH v6 0/9] leds: st1202: fix multiple bugs in pattern engine and brightness handling

[Manuel Fombuena]

This series fixes several bugs in the LED1202 driver related to hardware
pattern programming and brightness control. The issues were found during
testing on a Linksys MX4200v2 router running OpenWrt.

--- Pattern sequence not stopped before reprogramming ---

The LED1202 datasheet (section 4.8) states that writes to PAT_REP and
pattern duration registers are only applied after the sequence completes
or is stopped. When running in infinite loop mode the sequence never
completes on its own, so these writes are silently ignored by the
hardware.

  Patch 1: Stop the running sequence by clearing PATS in the
  configuration register at the start of both pattern_clear() and
  pattern_set(), ensuring the hardware accepts new values immediately.

  Patch 2: Validate pattern input before stopping the sequence. An
  out-of-range duration value should be rejected without disrupting a
  running pattern, so input validation is moved ahead of the sequence
  stop.

--- pattern_clear() leaving hardware in inconsistent state ---

Several independent bugs in pattern_clear() left the hardware in a state
that affected subsequent pattern or brightness operations.

  Patch 3: Fix the duration prescaler formula and the skip marker written
  by pattern_clear(). The original formula (value / ST1202_MILLIS_PATTERN_DUR_MIN
  - 1) was off by one, producing durations ~22ms too short. Additionally,
  pattern_clear() relied on the broken formula to produce register value 0
  (the pattern skip marker) by passing the minimum duration. With the formula
  corrected, pattern_clear() now writes 0 directly to unused duration
  registers instead of going through the conversion function.

  Patch 4: Set all 8 pattern PWM slots to full after clearing. When only
  Pattern0 is restored and patterns 1-7 are left at zero, the cleared
  channel blinks if the global sequencer is subsequently started by
  another channel. Setting all 8 slots to ST1202_PATTERN_PWM_FULL ensures
  the channel outputs a steady ILED value regardless of which sequencer
  step is active.

--- Spurious pattern sequence start during setup ---

  Patch 5: Remove the write of PATS|PATSR to the configuration register
  in st1202_setup(). This accidentally started a pattern sequence before
  any pattern data was programmed, producing undefined output on startup.

--- Brightness control broken while pattern mode is active ---

  Patch 6: Fix brightness having no effect while pattern mode is active.
  When PATS=1, the LED output is ILED x Pattern_PWM / 4095, so writing
  only the ILED register has little visible effect. Rather than stopping
  the global sequencer (which would halt patterns on all other channels),
  set all 8 PWM slots for the channel to ST1202_PATTERN_PWM_FULL before
  writing ILED. With every step at full duty cycle the output is ILED
  regardless of sequencer state.

  Patch 7: Disable the hardware channel in brightness_set() when value
  is zero. Previously only the ILED DAC was zeroed while the channel
  remained enabled, causing residual current through the enabled channel
  and a visible dim glow on the LED.

--- Input validation ---

  Patch 8: Validate the reg property read from the device tree before
  using it as an array index into chip->leds[]. A value >= ST1202_MAX_LEDS
  would cause an out-of-bounds write during probe.

--- Documentation ---

  Patch 9: Correct and extend the hw_pattern documentation. The maximum
  pattern duration was stated as 5660ms but the correct value derived
  from the prescaler formula is 5610ms. The repeat field documentation
  is also corrected and the brightness range is made explicit.

--- Testing ---

Tested on LED1202 hardware via I2C. Register state verified with i2cget
at each step. Correct LED behaviour confirmed across pattern cycling,
infinite repeat, pattern_clear, and direct brightness control with
trigger=none.

--- Changes in v6 ---

  In response to automated review feedback (Sashiko) on v5:

  Patch 4: pattern_clear() restored only Pattern0 PWM to full, leaving
  patterns 1-7 at zero. If another channel subsequently starts the global
  sequencer, the cleared channel cycles through its PWM steps and blinks.
  All 8 slots are now set to ST1202_PATTERN_PWM_FULL, unifying the loop
  to a single pass from 0.

  Patch 6: brightness_set() stopped the global sequencer via a write to
  CONFIG_REG, disrupting patterns running on other channels. The sequencer
  stop is replaced by setting all 8 PWM slots for the channel to
  ST1202_PATTERN_PWM_FULL, so the channel outputs steady ILED regardless
  of sequencer state without affecting other channels.

  Other pre-existing issues identified by the automated review are
  outside the scope of this fix series and will be addressed in a
  follow-up submission.

--- Changes in v5 ---

  Patch 7: Restore Pattern0 PWM to full duty cycle in brightness_set()
  before writing ILED. When PATS is cleared the LED output is
  ILED × Pattern0_PWM / 4095. A prior zero-brightness hardware pattern
  leaves Pattern0 PWM at zero, causing brightness_set() to have no
  visible effect despite ILED being set correctly. Found through further
  hardware testing.

--- Changes in v4 ---

  In response to automated review feedback (Sashiko) on v3:

  Patch 7: pattern_set() did not re-enable the hardware channel before
  starting the sequencer. If brightness had previously been set to zero
  (disabling the channel), the pattern would run but produce no light.

  Patch 8: reg was declared as signed int but populated via
  of_property_read_u32(). A device tree value >= 0x80000000 would be
  interpreted as negative and silently bypass the bounds check. reg is
  now declared as u32.

  Other pre-existing issues identified by the automated review are
  outside the scope of this fix series and will be addressed in a
  follow-up submission.

--- Changes in v3 ---

  In response to automated review feedback (Sashiko) on v2:

  Patch 1: Extend commit message to clarify that the LED1202 has a single
  global pattern sequencer shared across all channels, and that stopping
  it in pattern_clear() is therefore an inherent hardware constraint rather
  than a deliberate design choice.

  Patches 3+4: Squashed into a single patch. The prescaler fix and the
  skip marker fix are tightly coupled — the skip marker bug was a direct
  consequence of the broken formula — and are clearer as one change.

  Patch 5 (v2): Dropped. Resetting PAT_REP in pattern_clear() is
  unnecessary because pattern_set() always stops the sequencer and writes
  its own PAT_REP value before restarting. The reset introduced a spurious
  failure point without fixing a real bug.

  Patch 4 (was 6): Start the clearing loop at Pattern1 to avoid writing
  Pattern0 twice (the loop previously zeroed it before the explicit full
  restore).

  Patch 5 (was 7): Simplify commit message — remove inaccurate claim that
  the SHFT bit is never re-enabled after probe; pattern_clear() restores
  it during probe.

  New patch 8: Validate the reg device tree property against ST1202_MAX_LEDS
  before using it as an array index.

--- Changes in v2 ---

  Patch 3: Fix commit message wording — programmed durations were ~22ms
  too short, not too long.
  Patch 7: Fix Signed-off-by typo.
  Patch 10: Add missing quotes around commit subject in Fixes: tag.

v1: https://lore.kernel.org/all/WA0P291MB03850F4E9B4EC7389FE89779C5052@WA0P291MB0385.POLP291.PROD.OUTLOOK.COM/
v2: https://lore.kernel.org/all/WA0P291MB03855101311F0506B6448A8EC50D2@WA0P291MB0385.POLP291.PROD.OUTLOOK.COM/
v3: https://lore.kernel.org/all/GV1PR08MB849762937DA5B82DC89F1CC4C5112@GV1PR08MB8497.eurprd08.prod.outlook.com/
v4: https://lore.kernel.org/all/GV1PR08MB849700625A78304DD5FFCA3EC5E32@GV1PR08MB8497.eurprd08.prod.outlook.com/
v5: https://lore.kernel.org/all/GV1PR08MB8497ABC8E6AEFF8AAA1AF883C5E32@GV1PR08MB8497.eurprd08.prod.outlook.com/

Manuel Fombuena (9):
  leds: st1202: stop pattern sequence before reprogramming
  leds: st1202: validate pattern input before stopping the sequence
  leds: st1202: fix pattern duration prescaler and pattern_clear skip
    marker
  leds: st1202: set all pattern PWM slots to full after clearing pattern
  leds: st1202: fix spurious pattern sequence start in setup
  leds: st1202: fix brightness having no effect while pattern mode is
    active
  leds: st1202: disable channel when brightness is set to zero
  leds: st1202: validate LED reg property against channel count
  leds: st1202: correct and extend hw_pattern documentation

 Documentation/leds/leds-st1202.rst |   9 ++-
 drivers/leds/leds-st1202.c         | 102 ++++++++++++++++++-----------
 2 files changed, 67 insertions(+), 44 deletions(-)

-- 
2.55.0

[PATCH v6 1/9] leds: st1202: stop pattern sequence before reprogramming

[Manuel Fombuena]

The LED1202 datasheet (section 4.8) states that modifications to the
Pattern Sequence Repetition register (PAT_REP) and pattern duration
registers are only applied after the sequence has completed or been
stopped. When the device is running in infinite loop mode (PAT_REP =
0xFF) the sequence never completes on its own, so these writes are
silently ignored by the hardware.

Neither pattern_clear() nor pattern_set() stop the running sequence
before modifying pattern registers, causing any subsequent pattern
reprogramming to have no effect when the previous pattern was set to
infinite repeat.

Fix this by clearing PATS in the Configuration register before touching
any pattern registers in both functions, ensuring the hardware accepts
the new values immediately.

Note that the LED1202 has a single global pattern sequencer shared by
all channels: PATS, PATSR, the duration registers, and PAT_REP are
chip-wide. Stopping the sequencer in pattern_clear() therefore halts
any pattern running on other channels. This is an inherent hardware
constraint; pattern_set() restarts the sequencer when a new pattern is
programmed.

Fixes: 259230378c65 ("leds: Add LED1202 I2C driver")
Signed-off-by: Manuel Fombuena <fombuena@outlook.com>
Assisted-by: Claude:claude-sonnet-4-6
---
 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 7f68d956f694..316ed8eb054f 100644
--- a/drivers/leds/leds-st1202.c
+++ b/drivers/leds/leds-st1202.c
@@ -200,6 +200,10 @@ 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);
+	if (ret != 0)
+		return ret;
+
 	for (int patt = 0; patt < ST1202_MAX_PATTERNS; patt++) {
 		ret = st1202_pwm_pattern_write(chip, led->led_num, patt, LED_OFF);
 		if (ret != 0)
@@ -226,6 +230,10 @@ 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);
+	if (ret != 0)
+		return ret;
+
 	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)
-- 
2.55.0

[PATCH v6 2/9] leds: st1202: validate pattern input before stopping the sequence

[Manuel Fombuena]

Input validation for pattern duration is performed inside the write
loop, after the pattern sequence has already been stopped. If
validation fails mid-loop the chip is left with the sequence stopped
and partially written pattern data, with no recovery.

Move all input validation before the mutex and before any hardware
interaction, so an invalid input leaves the chip state unchanged.

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

diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
index 316ed8eb054f..a750c95ef3f8 100644
--- a/drivers/leds/leds-st1202.c
+++ b/drivers/leds/leds-st1202.c
@@ -228,6 +228,12 @@ 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)
+			return -EINVAL;
+	}
+
 	guard(mutex)(&chip->lock);
 
 	ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_SHFT);
@@ -235,10 +241,6 @@ static int st1202_led_pattern_set(struct led_classdev *ldev,
 		return ret;
 
 	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)
-			return -EINVAL;
-
 		ret = st1202_pwm_pattern_write(chip, led->led_num, patt, pattern[patt].brightness);
 		if (ret != 0)
 			return ret;
-- 
2.55.0

[PATCH v6 3/9] leds: st1202: fix pattern duration prescaler and pattern_clear skip marker

[Manuel Fombuena]

The PATy_DUR register encodes duration as N × 22.2 ms, with register
value 0 reserved as a pattern skip indicator (§7.10). The driver
incorrectly subtracted 1 from the register value:

  value / ST1202_MILLIS_PATTERN_DUR_MIN - 1

This caused two problems:

  - All programmed durations were off by one step (~22 ms too short).
  - Writing the minimum duration (22 ms) produced register value 0,
    silently skipping the pattern step instead of setting a 22 ms
    duration.

The maximum duration constant was also wrong at 5660 ms. The 8-bit
register saturates at 255, giving a maximum of 5610 ms (22 ms × 255).
Values above 5653 ms were already producing a uint8_t overflow and
writing 0 to the hardware.

Fix the formula by removing the erroneous subtraction, and derive the
maximum from the register width so the relationship is explicit. Update
the documentation to reflect the correct maximum.

This exposes a secondary issue: pattern_clear() was calling
st1202_duration_pattern_write() with ST1202_MILLIS_PATTERN_DUR_MIN to
reset unused slots, accidentally relying on the broken formula to
produce register value 0. With the corrected formula, the same call
writes 0x01 (22 ms), leaving unused slots as valid 22 ms zero-PWM
steps and making the LED appear off for 7 × 22 ms out of every cycle.

Write 0 directly to the duration registers in pattern_clear() so unused
slots are always explicitly marked as skip, independently of the
conversion formula.

Fixes: 259230378c65 ("leds: Add LED1202 I2C driver")
Signed-off-by: Manuel Fombuena <fombuena@outlook.com>
Assisted-by: Claude:claude-sonnet-4-6
---
 Documentation/leds/leds-st1202.rst | 2 +-
 drivers/leds/leds-st1202.c         | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/leds/leds-st1202.rst b/Documentation/leds/leds-st1202.rst
index 1a09fbfcedcf..a2353549469e 100644
--- a/Documentation/leds/leds-st1202.rst
+++ b/Documentation/leds/leds-st1202.rst
@@ -17,7 +17,7 @@ To be compatible with the hardware pattern format, maximum 8 tuples of
 brightness (PWM) and duration must be written to hw_pattern.
 
 - Min pattern duration: 22 ms
-- Max pattern duration: 5660 ms
+- Max pattern duration: 5610 ms
 
 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 a750c95ef3f8..b4adade49898 100644
--- a/drivers/leds/leds-st1202.c
+++ b/drivers/leds/leds-st1202.c
@@ -31,7 +31,7 @@
 #define ST1202_ILED_REG0                   0x09
 #define ST1202_MAX_LEDS                    12
 #define ST1202_MAX_PATTERNS                8
-#define ST1202_MILLIS_PATTERN_DUR_MAX      5660
+#define ST1202_MILLIS_PATTERN_DUR_MAX      (ST1202_MILLIS_PATTERN_DUR_MIN * U8_MAX)
 #define ST1202_MILLIS_PATTERN_DUR_MIN      22
 #define ST1202_PATTERN_DUR                 0x16
 #define ST1202_PATTERN_PWM                 0x1E
@@ -85,7 +85,7 @@ static int st1202_write_reg(struct st1202_chip *chip, int reg, uint8_t val)
 
 static uint8_t st1202_prescalar_to_miliseconds(unsigned int value)
 {
-	return value / ST1202_MILLIS_PATTERN_DUR_MIN - 1;
+	return value / ST1202_MILLIS_PATTERN_DUR_MIN;
 }
 
 static int st1202_pwm_pattern_write(struct st1202_chip *chip, int led_num,
@@ -209,7 +209,7 @@ static int st1202_led_pattern_clear(struct led_classdev *ldev)
 		if (ret != 0)
 			return ret;
 
-		ret = st1202_duration_pattern_write(chip, patt, ST1202_MILLIS_PATTERN_DUR_MIN);
+		ret = st1202_write_reg(chip, ST1202_PATTERN_DUR + patt, 0);
 		if (ret != 0)
 			return ret;
 	}
-- 
2.55.0

[PATCH v6 4/9] leds: st1202: set all pattern PWM slots to full after clearing pattern

[Manuel Fombuena]

pattern_clear() zeros all PWM registers then restores only Pattern0 to
full duty cycle. Patterns 1-7 are left at zero.

The LED1202 has a single global sequencer shared across all channels. If
another channel starts the sequencer after this one has been cleared, the
cleared channel cycles through its PWM steps — full brightness on step 0,
dark for steps 1-7 — producing an unintended blink.

Set all 8 PWM slots to ST1202_PATTERN_PWM_FULL and unify into a single
loop from 0, ensuring the channel outputs steady ILED regardless of which
sequencer step is active.

Signed-off-by: Manuel Fombuena <fombuena@outlook.com>
Assisted-by: Claude:claude-sonnet-4-6
---
 drivers/leds/leds-st1202.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
index b4adade49898..3203b3768ec5 100644
--- a/drivers/leds/leds-st1202.c
+++ b/drivers/leds/leds-st1202.c
@@ -35,6 +35,7 @@
 #define ST1202_MILLIS_PATTERN_DUR_MIN      22
 #define ST1202_PATTERN_DUR                 0x16
 #define ST1202_PATTERN_PWM                 0x1E
+#define ST1202_PATTERN_PWM_FULL            0x0FFF
 #define ST1202_PATTERN_REP                 0x15
 
 struct st1202_led {
@@ -205,7 +206,7 @@ static int st1202_led_pattern_clear(struct led_classdev *ldev)
 		return ret;
 
 	for (int patt = 0; patt < ST1202_MAX_PATTERNS; patt++) {
-		ret = st1202_pwm_pattern_write(chip, led->led_num, patt, LED_OFF);
+		ret = st1202_pwm_pattern_write(chip, led->led_num, patt, ST1202_PATTERN_PWM_FULL);
 		if (ret != 0)
 			return ret;
 
-- 
2.55.0

[PATCH v6 5/9] leds: st1202: fix spurious pattern sequence start in setup

[Manuel Fombuena]

st1202_setup() writes PATS and PATSR to the Configuration register as
its final step, which starts the hardware pattern sequencer during
device probe before any patterns have been programmed. This causes the
device to run a sequence with whatever values happen to be in the
pattern registers at the time.

Remove the write. The device reset at the start of setup restores all
registers to their power-on defaults, leaving PATS and PATSR cleared.

Fixes: 259230378c65 ("leds: Add LED1202 I2C driver")
Signed-off-by: Manuel Fombuena <fombuena@outlook.com>
Assisted-by: Claude:claude-sonnet-4-6
---
 drivers/leds/leds-st1202.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
index 3203b3768ec5..b8a571282900 100644
--- a/drivers/leds/leds-st1202.c
+++ b/drivers/leds/leds-st1202.c
@@ -333,11 +333,6 @@ static int st1202_setup(struct st1202_chip *chip)
 	if (ret < 0)
 		return ret;
 
-	ret = st1202_write_reg(chip, ST1202_CONFIG_REG,
-				ST1202_CONFIG_REG_PATS | ST1202_CONFIG_REG_PATSR);
-	if (ret < 0)
-		return ret;
-
 	return 0;
 }
 
-- 
2.55.0

[PATCH v6 6/9] leds: st1202: fix brightness having no effect while pattern mode is active

[Manuel Fombuena]

Once a hardware pattern is running (PATS=1), writing to the brightness
sysfs attribute only updates the ILED register. The visible output is
ILED x Pattern_PWM / 4095, so the change has little effect and the LED
never returns to steady static operation as the user expects.

The LED1202 has a single global sequencer shared across all channels.
Stopping it in brightness_set() to force static mode would halt running
patterns on all other active LEDs.

Instead, set all 8 PWM slots for the channel to ST1202_PATTERN_PWM_FULL
before writing ILED. With every step at full duty cycle, the output is
ILED x FULL / 4095 = ILED regardless of the sequencer state, without
disturbing other channels.

This also enables basic LED operation without the pattern trigger: with
the trigger set to none, the brightness sysfs attribute fully controls
the LED as a simple on/off device.

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

diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
index b8a571282900..4909985ade04 100644
--- a/drivers/leds/leds-st1202.c
+++ b/drivers/leds/leds-st1202.c
@@ -136,6 +136,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);
 	st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num, value);
 }
 
-- 
2.55.0

[PATCH v6 7/9] leds: st1202: disable channel when brightness is set to zero

[Manuel Fombuena]

When brightness_set() is called with LED_OFF, only the ILED register is
zeroed; the channel enable bit is left set from probe time. A hardware
channel enabled with ILED=0 still draws a small residual current, causing
a dim glow even when the LED is supposed to be off.

Fix this by splitting st1202_channel_set() into a lockless inner function
__st1202_channel_set() and a locking wrapper, then calling the inner
function from brightness_set() while it already holds the mutex. The
channel is now disabled when value is zero and re-enabled when non-zero,
in the same lock region as the ILED write.

Fixes: 259230378c65 ("leds: Add LED1202 I2C driver")
Signed-off-by: Manuel Fombuena <fombuena@outlook.com>
Assisted-by: Claude:claude-sonnet-4-6
---
 drivers/leds/leds-st1202.c | 68 ++++++++++++++++++++++----------------
 1 file changed, 39 insertions(+), 29 deletions(-)

diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
index 4909985ade04..2ecd36aae336 100644
--- a/drivers/leds/leds-st1202.c
+++ b/drivers/leds/leds-st1202.c
@@ -128,39 +128,11 @@ static int st1202_duration_pattern_write(struct st1202_chip *chip, int pattern,
 				st1202_prescalar_to_miliseconds(value));
 }
 
-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 patt = 0; patt < ST1202_MAX_PATTERNS; patt++)
-		st1202_pwm_pattern_write(chip, led->led_num, patt, ST1202_PATTERN_PWM_FULL);
-	st1202_write_reg(chip, ST1202_ILED_REG0 + 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);
-	struct st1202_chip *chip = led->chip;
-	u8 value = 0;
-
-	guard(mutex)(&chip->lock);
-
-	st1202_read_reg(chip, ST1202_ILED_REG0 + led->led_num, &value);
-
-	return value;
-}
-
-static int st1202_channel_set(struct st1202_chip *chip, int led_num, bool active)
+static int __st1202_channel_set(struct st1202_chip *chip, int led_num, bool active)
 {
 	u8 chan_low, chan_high;
 	int ret;
 
-	guard(mutex)(&chip->lock);
-
 	if (led_num <= 7) {
 		ret = st1202_read_reg(chip, ST1202_CHAN_ENABLE_LOW, &chan_low);
 		if (ret < 0)
@@ -188,6 +160,40 @@ static int st1202_channel_set(struct st1202_chip *chip, int led_num, bool active
 	return 0;
 }
 
+static int st1202_channel_set(struct st1202_chip *chip, int led_num, bool active)
+{
+	guard(mutex)(&chip->lock);
+
+	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 patt = 0; patt < ST1202_MAX_PATTERNS; patt++)
+		st1202_pwm_pattern_write(chip, led->led_num, patt, 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);
+	struct st1202_chip *chip = led->chip;
+	u8 value = 0;
+
+	guard(mutex)(&chip->lock);
+
+	st1202_read_reg(chip, ST1202_ILED_REG0 + led->led_num, &value);
+
+	return value;
+}
+
 static int st1202_led_set(struct led_classdev *ldev, enum led_brightness value)
 {
 	struct st1202_led *led = cdev_to_st1202_led(ldev);
@@ -257,6 +263,10 @@ static int st1202_led_pattern_set(struct led_classdev *ldev,
 	if (ret != 0)
 		return ret;
 
+	ret = __st1202_channel_set(chip, led->led_num, true);
+	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));
 	if (ret != 0)
-- 
2.55.0

[PATCH v6 8/9] leds: st1202: validate LED reg property against channel count

[Manuel Fombuena]

The reg property from the device tree is used directly as an array index
into chip->leds[] without bounds checking. A value >= ST1202_MAX_LEDS
would cause an out-of-bounds write during probe.

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

diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
index 2ecd36aae336..168df5ecf27b 100644
--- a/drivers/leds/leds-st1202.c
+++ b/drivers/leds/leds-st1202.c
@@ -279,13 +279,19 @@ static int st1202_dt_init(struct st1202_chip *chip)
 {
 	struct device *dev = &chip->client->dev;
 	struct st1202_led *led;
-	int err, reg;
+	int err;
+	u32 reg;
 
 	for_each_available_child_of_node_scoped(dev_of_node(dev), child) {
 		err = of_property_read_u32(child, "reg", &reg);
 		if (err)
 			return dev_err_probe(dev, err, "Invalid register\n");
 
+		if (reg >= ST1202_MAX_LEDS)
+			return dev_err_probe(dev, -EINVAL,
+					"LED reg %u out of range [0, %d]\n",
+					reg, ST1202_MAX_LEDS - 1);
+
 		led = &chip->leds[reg];
 		led->is_active = true;
 		led->fwnode = of_fwnode_handle(child);
-- 
2.55.0

[PATCH v6 9/9] leds: st1202: correct and extend hw_pattern documentation

[Manuel Fombuena]

Fix the repeat section: -1 is a valid value meaning infinite repeat,
as accepted by the ledtrig-pattern sysfs interface; only 0 and values
below -1 are rejected. The previous text incorrectly stated all negative
numbers were invalid. Also remove the redundant trailing sentence since
the behaviour is now covered inline.

Add the brightness range (0-255) to the hw_pattern section, which was
previously undocumented.

Fixes: b1816b22381b ("Documentation:leds: Add leds-st1202.rst")
Signed-off-by: Manuel Fombuena <fombuena@outlook.com>
Assisted-by: Claude:claude-sonnet-4-6
---
 Documentation/leds/leds-st1202.rst | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/Documentation/leds/leds-st1202.rst b/Documentation/leds/leds-st1202.rst
index a2353549469e..ed32eb3a27d4 100644
--- a/Documentation/leds/leds-st1202.rst
+++ b/Documentation/leds/leds-st1202.rst
@@ -16,6 +16,7 @@ in terms of PWM duty-cycle and duration (ms).
 To be compatible with the hardware pattern format, maximum 8 tuples of
 brightness (PWM) and duration must be written to hw_pattern.
 
+- Brightness range: 0-255
 - Min pattern duration: 22 ms
 - Max pattern duration: 5610 ms
 
@@ -26,9 +27,7 @@ The format of the hardware pattern values should be:
 ----------------------------
 
 Specify a pattern repeat number, which is common for all channels.
-Default is 1; negative numbers and 0 are invalid.
+Default is 1. Writing 0 is invalid. Writing -1 or 255 repeats the
+pattern indefinitely.
 
 This file will always return the originally written repeat number.
-
-When the 255 value is written to it, all patterns will repeat
-indefinitely.
-- 
2.55.0

Re: [PATCH v6 5/9] leds: st1202: fix spurious pattern sequence start in setup

[sashiko-bot]

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

Pre-existing issues:
- [High] The non-blocking `brightness_set` callback is assigned to a sleeping function, and `brightness_set_blocking` improperly discards brightness values.
- [High] Per-LED pattern callbacks inappropriately modify global chip sequencer registers, corrupting state for other channels.
- [High] Device tree node `fwnode` pointer is saved without incrementing its reference count, resulting in a dangling pointer.
--

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

leds: st1202: fix spurious pattern sequence start in setup

This patch removes a final write to the configuration register in
st1202_setup(). The removal prevents the hardware pattern sequencer from
starting prematurely during device probe before any patterns have been
programmed.

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index 3203b3768ec52..b8a5712829009 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
> @@ -333,11 +333,6 @@ static int st1202_setup(struct st1202_chip *chip)
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = st1202_write_reg(chip, ST1202_CONFIG_REG,
> -				ST1202_CONFIG_REG_PATS | ST1202_CONFIG_REG_PATSR);
> -	if (ret < 0)
> -		return ret;
> -
>  	return 0;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does st1202_dt_init() create a
dangling fwnode pointer?

When retrieving the child nodes via for_each_available_child_of_node_scoped(),
the node reference is dropped automatically at the end of the loop:

st1202_dt_init() {
    ...
    for_each_available_child_of_node_scoped(dev_of_node(dev), child) {
        ...
        led->fwnode = of_fwnode_handle(child);
    }
    ...
}

Because fwnode_handle_get() is not used before assigning to led->fwnode, this
pointer becomes dangling after the loop ends and is later dereferenced in
st1202_probe() when calling devm_led_classdev_register_ext().

Can this lead to a use-after-free, especially if dynamic device trees or
overlays are used?

[Severity: High]
This is a pre-existing issue, but are the blocking and non-blocking brightness
callbacks swapped in st1202_dt_init()?

st1202_dt_init() {
    ...
    led->led_cdev.brightness_set_blocking = st1202_led_set;
    ...
    led->led_cdev.brightness_set = st1202_brightness_set;
    ...
}

The non-blocking brightness_set callback is assigned to
st1202_brightness_set(), which takes a mutex and performs I2C transactions
that can sleep:

st1202_brightness_set() {
    ...
    guard(mutex)(&chip->lock);
    st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num, value);
}

Could this cause a scheduling while atomic bug if triggered from an atomic
context like a software timer?

At the same time, the blocking callback is assigned to st1202_led_set(),
which completely discards intermediate brightness values and only toggles
the channel on or off:

st1202_led_set() {
    ...
    return st1202_channel_set(led->chip, led->led_num, !!value);
}

[Severity: High]
This isn't a bug introduced by this patch, but do the per-LED pattern
callbacks corrupt global sequencer registers for other LEDs?

In st1202_led_pattern_clear(), the function unconditionally zeroes out the
global pattern duration registers:

st1202_led_pattern_clear() {
    ...
    for (int patt = 0; patt < ST1202_MAX_PATTERNS; patt++) {
        ...
        ret = st1202_write_reg(chip, ST1202_PATTERN_DUR + patt, 0);
    }
    ...
}

Similarly, st1202_led_pattern_set() resets the global sequence enable bits
for the whole chip:

st1202_led_pattern_set() {
    ...
    ret = st1202_write_reg(chip, ST1202_CONFIG_REG, (ST1202_CONFIG_REG_PATSR |
                            ST1202_CONFIG_REG_PATS | ST1202_CONFIG_REG_SHFT));
    ...
}

Since these registers are shared across all channels on the chip, will clearing
or setting a pattern on one LED arbitrarily destroy the pattern state of
another LED running simultaneously?

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

Re: [PATCH v6 3/9] leds: st1202: fix pattern duration prescaler and pattern_clear skip marker

[sashiko-bot]

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

Pre-existing issues:
- [High] The driver exposes a single global hardware sequencer as a per-LED pattern, causing pattern operations on one channel to destructively overwrite and halt patterns on all other channels.
- [High] The `brightness_set` callback points to a sleeping function, while `brightness_set_blocking` fails to actually set the analog brightness.
- [Medium] `st1202_led_pattern_set()` fails to clear duration registers for unused pattern steps, causing stale steps to leak into new, shorter patterns.
- [Medium] Pattern brightness is written directly to 12-bit registers, but `max_brightness` is advertised as 8-bit (255), severely capping output.
--

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

leds: st1202: fix pattern duration prescaler and pattern_clear skip marker

The commit fixes the ST1202 LED driver where the pattern duration prescaler
calculation was off by one, causing durations to be too short. It corrects
the maximum duration constant to reflect the 8-bit register limit.
Additionally, it explicitly clears unused pattern slots by writing 0 to the
duration register to mark them as skipped.

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index a750c95ef3f89..b4adade49898e 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)
>  
>  static uint8_t st1202_prescalar_to_miliseconds(unsigned int value)
>  {
> -	return value / ST1202_MILLIS_PATTERN_DUR_MIN - 1;
> +	return value / ST1202_MILLIS_PATTERN_DUR_MIN;
>  }
>  
>  static int st1202_pwm_pattern_write(struct st1202_chip *chip, int led_num,

[Severity: Medium]
This is a pre-existing issue, but is the brightness value getting scaled
properly to the hardware's 12-bit range?

In drivers/leds/leds-st1202.c:st1202_pwm_pattern_write():

    value_l = (u8)value;
    value_h = (u8)(value >> 8);

The user's requested pattern brightness is scaled to the advertised
max_brightness of 255, but the driver appears to pass this value directly
to the hardware's 12-bit PWM register (max 4095). 

Does this cause requesting maximum brightness to cap the PWM duty cycle
at roughly 6.2%, making patterns extremely dim?

[ ... ]
> @@ -209,7 +209,7 @@ static int st1202_led_pattern_clear(struct led_classdev *ldev)
>  		if (ret != 0)
>  			return ret;
>  
> -		ret = st1202_duration_pattern_write(chip, patt, ST1202_MILLIS_PATTERN_DUR_MIN);
> +		ret = st1202_write_reg(chip, ST1202_PATTERN_DUR + patt, 0);
>  		if (ret != 0)
>  			return ret;
>  	}

[Severity: Medium]
This isn't a bug introduced by this patch, but while pattern_clear resets
all slots, does st1202_led_pattern_set() fail to clear duration registers
for unused pattern steps?

drivers/leds/leds-st1202.c:st1202_led_pattern_set() {
    for (int patt = 0; patt < len; patt++) {
        ret = st1202_pwm_pattern_write(chip, led->led_num, patt,
                                       pattern[patt].brightness);
        ...
        ret = st1202_duration_pattern_write(chip, patt,
                                            pattern[patt].delta_t);
    }
}

If a user programs a shorter pattern sequence after a longer one, the
loop updates only up to len steps, failing to zero out the durations of
the remaining ST1202_MAX_PATTERNS - len steps. 

Will the hardware sequencer continue to execute the stale un-cleared
trailing steps?

[Severity: High]
This is a pre-existing issue, but does clearing the pattern for one LED
destructively overwrite and halt patterns on all other channels?

In st1202_led_pattern_clear():

    ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_SHFT);

And in st1202_duration_pattern_write():

    return st1202_write_reg(chip, (ST1202_PATTERN_DUR + pattern),
                            st1202_prescalar_to_miliseconds(value));

Since ST1202_PATTERN_DUR and ST1202_CONFIG_REG are globally shared
registers, it appears that programming or clearing a pattern on one LED
corrupts the duration registers and sequencer state of all other active
patterns. 

Is there a lack of isolation between independent LED devices on the same
chip?

[Severity: High]
This isn't a bug introduced by this patch, but are the brightness
callbacks mapped correctly in st1202_dt_init()?

drivers/leds/leds-st1202.c:st1202_dt_init() {
    ...
    led->led_cdev.brightness_set_blocking = st1202_led_set;
    led->led_cdev.brightness_set = st1202_brightness_set;
    ...
}

The brightness_set callback is strictly required to be non-blocking, but
it is mapped to st1202_brightness_set, which is an I2C-dependent sleeping
function. 

Could this cause a kernel panic (scheduling while atomic) if a kernel LED
trigger calls it from a softirq context?

Additionally, st1202_led_set() acts only as an on/off toggle:

drivers/leds/leds-st1202.c:st1202_led_set() {
    ...
    return st1202_channel_set(led->chip, led->led_num, !!value);
}

Since this is assigned to brightness_set_blocking, it completely drops the
requested brightness magnitude. 

Will this break sysfs writes if blocking is required?

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

Re: [PATCH v6 4/9] leds: st1202: set all pattern PWM slots to full after clearing pattern

[sashiko-bot]

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

New issues:
- [Medium] The commit message hallucinates details about the baseline code behavior, falsely claiming it previously restored Pattern0 to full duty cycle.

Pre-existing issues:
- [High] Clearing a pattern on one LED destructively halts the global sequencer and zeroes shared duration registers, breaking active patterns on all other channels.
- [High] The driver provides a non-blocking `brightness_set` callback that incorrectly performs sleeping operations, causing panics in atomic contexts.
- [Medium] Programming a shorter pattern fails to clear trailing sequence steps, causing leftover pattern steps to execute.
- [Medium] The user-provided 8-bit brightness is written directly to a 12-bit PWM register without scaling, causing patterns to be extremely dim.
--

commit 030b013dca5ca1bf862e53e8c7cfe3288cb86967
Author: Manuel Fombuena <fombuena@outlook.com>
Subject: leds: st1202: set all pattern PWM slots to full after clearing pattern

The commit alters st1202_led_pattern_clear() to set all 8 PWM slots
to ST1202_PATTERN_PWM_FULL. This aims to prevent unintended blinks
if another channel starts the global sequencer.

> pattern_clear() zeros all PWM registers then restores only Pattern0 to
> full duty cycle. Patterns 1-7 are left at zero.

[Severity: Medium]
Is this description of the previous behavior accurate?

The baseline code appears to iterate over all patterns and set them to
LED_OFF, without restoring Pattern0 to full duty cycle. Could the commit
message be updated to precisely reflect the original baseline behavior?

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index b4adade49898e..3203b3768ec52 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
[ ... ]
> @@ -205,7 +206,7 @@ static int st1202_led_pattern_clear(struct led_classdev *ldev)
>  		return ret;
>  
>  	for (int patt = 0; patt < ST1202_MAX_PATTERNS; patt++) {
> -		ret = st1202_pwm_pattern_write(chip, led->led_num, patt, LED_OFF);
> +		ret = st1202_pwm_pattern_write(chip, led->led_num, patt, ST1202_PATTERN_PWM_FULL);
>  		if (ret != 0)
>  			return ret;
>  

[Severity: High]
This is a pre-existing issue, but does clearing a pattern on one LED
destructively halt the global sequencer for all channels?

If a user configures a pattern on LED0, and then clears the pattern on LED1,
st1202_led_pattern_clear() writes to the global ST1202_CONFIG_REG:

    ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_SHFT);

And then zeroes the shared ST1202_PATTERN_DUR registers:

    ret = st1202_write_reg(chip, ST1202_PATTERN_DUR + patt, 0);

Since this patch now sets all PWM slots to ST1202_PATTERN_PWM_FULL for the
cleared channel, making it immune to the sequencer state, is it still
necessary to stop the sequencer and zero the shared durations in this
per-channel callback?


[Severity: High]
This is a pre-existing issue, but is it possible for the brightness_set
callback to perform sleeping operations in an atomic context?

The driver assigns st1202_brightness_set to the non-blocking callback
pointer in st1202_dt_init():

    led->led_cdev.brightness_set = st1202_brightness_set;

However, st1202_brightness_set() acquires a mutex which can sleep:

    guard(mutex)(&chip->lock);

Since the LED core can call brightness_set from atomic contexts (like
softirqs used by the timer trigger), could this cause a scheduling while
atomic kernel panic? Would it be better to rely solely on
brightness_set_blocking for this driver?


[Severity: Medium]
This is a pre-existing issue, but would programming a shorter pattern fail
to clear the trailing sequence steps?

In st1202_led_pattern_set(), the loop terminates exactly at len:

    for (int patt = 0; patt < len; patt++) {

If a user programs a pattern of length 8, and later programs a shorter
pattern of length 4, the hardware will continue executing the leftover
non-zero steps from the previous pattern. Should the driver explicitly
zero the duration register of the len-th step to mark the end of the
sequence?


[Severity: Medium]
This is a pre-existing issue, but could the user-provided 8-bit brightness
be written directly to a 12-bit PWM register without scaling?

In st1202_led_pattern_set():

    ret = st1202_pwm_pattern_write(chip, led->led_num, patt,
                                   pattern[patt].brightness);

Since the 8-bit value (0-255) is written directly into the 12-bit register
(0-4095), the maximum possible pattern brightness is only about 6 percent of
the LED capability. Should this brightness value be scaled before writing it
to the hardware?

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

Re: [PATCH v6 1/9] leds: st1202: stop pattern sequence before reprogramming

[sashiko-bot]

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

Pre-existing issues:
- [Medium] Clearing a pattern on one LED overwrites the global pattern duration registers, breaking patterns on other LEDs.
--

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

leds: st1202: stop pattern sequence before reprogramming

The LED1202 ignores modifications to pattern registers if the sequencer is in
infinite loop mode. This patch clears the PATS bit in the Configuration
register to stop the running sequence before reprogramming it.

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index 7f68d956f6948..316ed8eb054f9 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
> @@ -200,6 +200,10 @@ 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);
> +	if (ret != 0)
> +		return ret;
> +
>  	for (int patt = 0; patt < ST1202_MAX_PATTERNS; patt++) {
>  		ret = st1202_pwm_pattern_write(chip, led->led_num, patt, LED_OFF);
>  		if (ret != 0)
>  			return ret;
> 
>  		ret = st1202_write_reg(chip, ST1202_PATTERN_DUR + patt, 0);

[Severity: Medium]
This is a pre-existing issue, but does clearing a pattern on one LED
inadvertently break patterns on other LEDs?

When st1202_led_pattern_clear() is called to clear a single LED's pattern,
it zeros out the pattern duration registers for all 8 pattern steps as shown
in the loop above.

Because these duration registers are shared chip-wide among all LEDs,
overwriting them corrupts the timing for any other LEDs that are currently
running a pattern concurrently.

This patch makes the behavior actively destructive because adding the
sequencer stop ensures these duration writes are fully applied by the
hardware.

While the commit message notes that halting the sequencer is a hardware
constraint, is there a hardware requirement to also overwrite the shared
duration registers when clearing a single LED's pattern?

[ ... ]

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

Re: [PATCH v6 6/9] leds: st1202: fix brightness having no effect while pattern mode is active

[sashiko-bot]

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

New issues:
- [Medium] The newly added loop in `st1202_brightness_set` ignores I2C error returns, continuing blindly on bus failures.

Pre-existing issues:
- [High] The `st1202_brightness_set` function is incorrectly registered as the non-blocking `brightness_set` callback, but it sleeps by acquiring a mutex and performing I2C operations.
--

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

leds: st1202: fix brightness having no effect while pattern mode is active

This commit sets all 8 PWM slots for a channel to full duty cycle before
writing ILED to ensure brightness changes take effect when a hardware
pattern is running. It enables static brightness control without halting
the global sequencer.

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index b8a5712829009..4909985ade04b 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
> @@ -136,6 +136,8 @@ static void st1202_brightness_set(struct led_classdev *led_cdev,
>  
>  	guard(mutex)(&chip->lock);

[Severity: High]
This is a pre-existing issue, but does st1202_brightness_set() sleep in an
atomic context?

This function acquires a mutex and performs I2C operations which can sleep,
but it appears to be registered as the non-blocking brightness_set callback
in st1202_dt_init():

led->led_cdev.brightness_set = st1202_brightness_set;

Activating a software trigger (like 'timer' or 'heartbeat') executes
brightness_set() from a timer softirq context, where acquiring a mutex or
performing synchronous I2C operations is illegal and causes a system crash.

While not introduced by this patch, adding a loop inside this atomic
section that executes 16 additional synchronous I2C writes significantly
prolongs the illegal blocking duration.

> +	for (int patt = 0; patt < ST1202_MAX_PATTERNS; patt++)
> +		st1202_pwm_pattern_write(chip, led->led_num, patt, ST1202_PATTERN_PWM_FULL);

[Severity: Medium]
Is it safe to ignore the return value of st1202_pwm_pattern_write() here?

If an I2C transaction fails (e.g., due to a temporary bus error), this loop
does not break and unconditionally attempts all 8 iterations. Because the
underlying st1202_write_reg() internally calls dev_err() on failure, a
single bus error will cause 16 consecutive error messages to flood the
kernel log, while pointlessly blocking to attempt writes that are
guaranteed to fail.

Should the loop check the return value and abort early on failure?

>  	st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num, value);
>  }

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

Re: [PATCH v6 7/9] leds: st1202: disable channel when brightness is set to zero

[sashiko-bot]

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

Pre-existing issues:
- [High] The driver incorrectly registers a sleeping function for the non-blocking `brightness_set` callback, leading to 'scheduling while atomic' panics.
- [High] `st1202_led_set` (mapped to `brightness_set_blocking`) fails to apply the requested brightness level.
- [High] The driver saves `fwnode` pointers from a scoped device tree iterator without incrementing their reference counts.
- [High] The `pattern_set` implementation fails to clear unused pattern slots when a pattern sequence shorter than 8 steps is provided.
--

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

leds: st1202: disable channel when brightness is set to zero

When brightness_set() is called with LED_OFF, only the ILED register is
zeroed; the channel enable bit is left set from probe time. A hardware
channel enabled with ILED=0 still draws a small residual current, causing
a dim glow even when the LED is supposed to be off.

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index 4909985ade04b..2ecd36aae336a 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
[ ... ]
> @@ -188,6 +160,40 @@ static int st1202_channel_set(struct st1202_chip *chip, int led_num, bool 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 patt = 0; patt < ST1202_MAX_PATTERNS; patt++)
> +		st1202_pwm_pattern_write(chip, led->led_num, patt, ST1202_PATTERN_PWM_FULL);
> +	st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num, value);
> +	__st1202_channel_set(chip, led->led_num, !!value);
> +}

[Severity: High]
This isn't a bug introduced by this patch, but this function sleeps by
acquiring a mutex and performing I2C operations.

Since this function is assigned to led_cdev.brightness_set (which is expected
to be non-blocking and can be called from atomic contexts like software blink
timers), will this lead to a scheduling while atomic panic? Should this be
assigned to brightness_set_blocking instead?

[ ... ]
>  static int st1202_led_set(struct led_classdev *ldev, enum led_brightness value)
>  {
>  	struct st1202_led *led = cdev_to_st1202_led(ldev);

[Severity: High]
This is a pre-existing issue, but st1202_led_set() discards the requested
brightness level.

Based on the surrounding code, this function only toggles the channel boolean:

    return st1202_channel_set(led->chip, led->led_num, !!value);

Since this function is mapped to the brightness_set_blocking callback,
wouldn't this mean sysfs writes fail to apply the actual requested brightness
to the hardware register, effectively breaking brightness control?

> @@ -257,6 +263,10 @@ static int st1202_led_pattern_set(struct led_classdev *ldev,
>  	if (ret != 0)
>  		return ret;
>  
> +	ret = __st1202_channel_set(chip, led->led_num, true);
> +	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));
>  	if (ret != 0)
>  		return ret;

[Severity: High]
This is a pre-existing issue, but does st1202_led_pattern_set() fail to clear
unused pattern slots?

If a pattern sequence shorter than 8 steps (ST1202_MAX_PATTERNS) is provided,
the loop writes up to len but doesn't explicitly write a 0 duration to the
remaining pattern slots. If a longer sequence was previously set, will the
hardware continue executing leftover uninitialized steps using stale data and
cause erratic flashing?

[Severity: High]
Additionally, there is a pre-existing use-after-free issue in
st1202_dt_init() which isn't shown in this diff.

When saving fwnode pointers from a scoped device tree iterator:

    for_each_available_child_of_node_scoped(dev_of_node(dev), child) {
        ...
        led->fwnode = of_fwnode_handle(child);
    }

The scoped iterator automatically drops the reference to the child node when
advancing or exiting. Since fwnode_handle_get() is not called, are we storing
dangling pointers that could be freed before being passed to
devm_led_classdev_register_ext() later in the probe?

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

Re: [PATCH v6 2/9] leds: st1202: validate pattern input before stopping the sequence

[sashiko-bot]

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

Pre-existing issues:
- [High] The driver improperly writes 8-bit brightness values directly into a 12-bit PWM register without scaling, resulting in extremely dim pattern output.
- [High] The driver implements the `.brightness_set` callback with a function that sleeps, violating LED subsystem requirements and causing a panic in atomic context.
- [High] Clearing one LED's pattern destructively resets the globally shared pattern sequencer and duration registers for all other LEDs.
--

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

leds: st1202: validate pattern input before stopping the sequence

Input validation for pattern duration is performed inside the write
loop, after the pattern sequence has already been stopped. This patch
moves all input validation before the mutex and before any hardware
interaction, so an invalid input leaves the chip state unchanged.

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index 316ed8eb054f9..a750c95ef3f89 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
> @@ -228,6 +228,12 @@ 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)
> +			return -EINVAL;
> +	}
> +
>  	guard(mutex)(&chip->lock);
>  
>  	ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_SHFT);
> @@ -235,10 +241,6 @@ static int st1202_led_pattern_set(struct led_classdev *ldev,
>  		return ret;
>  
>  	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)
> -			return -EINVAL;
> -
>  		ret = st1202_pwm_pattern_write(chip, led->led_num, patt, pattern[patt].brightness);

[Severity: High]
This isn't a bug introduced by this patch, but does this function improperly
write 8-bit brightness values directly into a 12-bit PWM register without
scaling?

Because the input brightness value is bounded to max_brightness (255) by
the subsystem, the upper 8 bits in st1202_pwm_pattern_write() appear to
always be 0:

drivers/leds/leds-st1202.c:st1202_pwm_pattern_write() {
    ...
    value_h = (u8)(value >> 8);
    ...
}

Will this result in extremely dim pattern output (maximum of 6.2% brightness)
and render the pattern feature functionally broken?

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

[ ... ]

Additionally, there are a few other pre-existing issues in this driver
that I noticed:

[Severity: High]
This is a pre-existing issue, but does the driver inappropriately register a
sleeping function as the non-blocking .brightness_set callback?

drivers/leds/leds-st1202.c:st1202_dt_init() {
    ...
    led->led_cdev.brightness_set = st1202_brightness_set;
    ...
}

When a trigger (like the timer trigger) calls this from atomic context,
st1202_brightness_set() takes a mutex and performs I2C operations which can
sleep:

drivers/leds/leds-st1202.c:st1202_brightness_set() {
    ...
    guard(mutex)(&chip->lock);
    ...
}

Will this cause a panic in atomic context? Should the driver only implement
.brightness_set_blocking to ensure the LED core defers the work to a
workqueue?


[Severity: High]
This is a pre-existing issue, but does clearing one LED's pattern
destructively reset the globally shared pattern sequencer for all other LEDs?

In st1202_led_pattern_clear(), it writes directly to the global configuration
register, disabling the pattern sequencer for all LEDs on the chip:

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

It also appears to blindly zero out the globally shared duration registers:

drivers/leds/leds-st1202.c:st1202_led_pattern_clear() {
    ...
    ret = st1202_write_reg(chip, ST1202_PATTERN_DUR + patt, 0);
    ...
}

Will this corrupt any sequences currently running on other channels on the
same chip?

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

Re: [PATCH v6 8/9] leds: st1202: validate LED reg property against channel count

[sashiko-bot]

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

Pre-existing issues:
- [High] The driver saves an unreferenced `fwnode` pointer from an OF iterator, leading to a potential use-after-free when registering the LED device.
--

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

leds: st1202: validate LED reg property against channel count

This commit adds bounds checking for the reg property parsed from the device
tree. It ensures the value is within the ST1202_MAX_LEDS limit to prevent
out-of-bounds writes into the chip->leds array during probe.

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
[ ... ]
> @@ -279,13 +279,19 @@ static int st1202_dt_init(struct st1202_chip *chip)
[ ... ]
>  		if (reg >= ST1202_MAX_LEDS)
>  			return dev_err_probe(dev, -EINVAL,
>  					"LED reg %u out of range [0, %d]\n",
>  					reg, ST1202_MAX_LEDS - 1);
>  
>  		led = &chip->leds[reg];
>  		led->is_active = true;
>  		led->fwnode = of_fwnode_handle(child);

[Severity: High]
This isn't a bug introduced by this patch, but does st1202_dt_init() save
an unreferenced fwnode pointer here?

The scoped iterator for_each_available_child_of_node_scoped() automatically
drops the reference to the child node when the loop advances or exits. Because
the driver does not explicitly acquire a reference via fwnode_handle_get() or
of_node_get(), the saved pointer becomes unreferenced.

If the device tree node is dynamically removed (for example, via DT overlay
removal) after the loop finishes, its memory will be freed. Later in
st1202_probe(), devm_led_classdev_register_ext() calls device_add(), which
dereferences dev->fwnode and attempts to increment its reference count,
resulting in a use-after-free.

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

Re: [PATCH v6 9/9] leds: st1202: correct and extend hw_pattern documentation

[sashiko-bot]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the driver violates the LED subsystem API contract by registering a sleeping function (`st1202_brightness_set`) as the non-blocking `brightness_set` callback. This function acquires a mutex and calls I2C functions, which will cause a 'scheduling while atomic' kernel panic when called from atomic context.
--

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

leds: st1202: correct and extend hw_pattern documentation
    
Fix the repeat section: -1 is a valid value meaning infinite repeat,
as accepted by the ledtrig-pattern sysfs interface; only 0 and values
below -1 are rejected. The previous text incorrectly stated all negative
numbers were invalid. Also remove the redundant trailing sentence since
the behaviour is now covered inline.
    
Add the brightness range (0-255) to the hw_pattern section, which was
previously undocumented.

> diff --git a/Documentation/leds/leds-st1202.rst b/Documentation/leds/leds-st1202.rst
> index a2353549469ee..ed32eb3a27d45 100644
> --- a/Documentation/leds/leds-st1202.rst
> +++ b/Documentation/leds/leds-st1202.rst

[ ... ]

While this patch is a documentation update, there is a pre-existing issue in the
corresponding C file:

drivers/leds/leds-st1202.c:

> 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 patt = 0; patt < ST1202_MAX_PATTERNS; patt++)
> 		st1202_pwm_pattern_write(chip, led->led_num, patt, ST1202_PATTERN_PWM_FULL);
> 	st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num, value);
> 	__st1202_channel_set(chip, led->led_num, !!value);
> }

[Severity: High]
This isn't a bug introduced by this patch, but does the driver violate the LED
subsystem API contract by registering a sleeping function
(st1202_brightness_set) as the non-blocking brightness_set callback?

This function acquires a mutex and calls I2C functions, which will cause a
'scheduling while atomic' kernel panic when called from atomic context.

Could this crash the kernel if a user configures an LED trigger like
ledtrig-timer or ledtrig-pattern that runs in softirq (atomic) context?

The trigger's timer callback calls led_set_brightness(), which is guaranteed
not to sleep, and synchronously invokes the driver's brightness_set callback.
Because st1202_brightness_set uses guard(mutex) and performs I2C transactions,
it sleeps in an atomic context, triggering a crash. Any user with sysfs access
to configure LED triggers can trigger this path.

[ ... ]

> static int st1202_dt_init(struct st1202_chip *chip)
> {
[ ... ]
> 		led->led_cdev.max_brightness = U8_MAX;
> 		led->led_cdev.brightness_set_blocking = st1202_led_set;
> 		led->led_cdev.pattern_set = st1202_led_pattern_set;
> 		led->led_cdev.pattern_clear = st1202_led_pattern_clear;
> 		led->led_cdev.default_trigger = "pattern";
> 		led->led_cdev.brightness_set = st1202_brightness_set;
> 		led->led_cdev.brightness_get = st1202_brightness_get;
> 	}

[Severity: High]
This is a pre-existing issue, but by assigning st1202_brightness_set to the
non-blocking callback pointer, we risk atomic sleep panics.

Should this be assigned to brightness_set_blocking instead, and brightness_set
set to NULL so the LED core can correctly defer blocking operations to a
workqueue?

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

Re: [PATCH v6 6/9] leds: st1202: fix brightness having no effect while pattern mode is active

[Manuel Fombuena]

On Thu, 2 Jul 2026, sashiko-bot@kernel.org wrote:

> New issues:
> - [Medium] The newly added loop in `st1202_brightness_set` ignores I2C error returns, continuing blindly on bus failures.
> 
> [Severity: Medium]
> Is it safe to ignore the return value of st1202_pwm_pattern_write() here?
> 
> If an I2C transaction fails (e.g., due to a temporary bus error), this loop
> does not break and unconditionally attempts all 8 iterations. Because the
> underlying st1202_write_reg() internally calls dev_err() on failure, a
> single bus error will cause 16 consecutive error messages to flood the
> kernel log, while pointlessly blocking to attempt writes that are
> guaranteed to fail.
> 
> Should the loop check the return value and abort early on failure?

Intentional. A transient I2C error on one slot does not
guarantee subsequent writes will also fail. Breaking early would leave
remaining PWM slots unwritten unnecessarily, putting the channel in a
partially updated state. brightness_set() is a void callback and
cannot propagate errors regardless, and st1202_write_reg() already
logs each individual failure via dev_err().

--
Manuel Fombuena

Re: [PATCH v6 4/9] leds: st1202: set all pattern PWM slots to full after clearing pattern

[Manuel Fombuena]

On Thu, 2 Jul 2026, sashiko-bot@kernel.org wrote:

> New issues:
> - [Medium] The commit message hallucinates details about the baseline code behavior, falsely claiming it previously restored Pattern0 to full duty cycle.

Indeed, the commit message described the previous behavior
from v5 and earlier iterations of this series as the baseline. Fixed
in v7.

--
Manuel Fombuena