[PATCH v2 00/10] leds: st1202: fix multiple bugs in pattern engine and brightness handling

[PATCH v2 00/10] 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. The original formula
  (value / ST1202_MILLIS_PATTERN_DUR_MIN - 1) produced an off-by-one
  result, writing the minimum valid duration (22ms, register value 0x01)
  instead of the skip marker (0x00) for unused slots.

  Patch 4: Write the skip marker (0x00) directly to unused duration
  registers in pattern_clear() rather than going through
  st1202_duration_pattern_write(), which operates on millisecond values
  and cannot express register value zero.

  Patch 5: Reset PAT_REP to its power-on default of 1 in pattern_clear().
  A stale value — most critically 0xFF from a previous infinite loop —
  would be picked up by a subsequent pattern_set() call in the window
  between pattern_clear() returning and pattern_set() writing its own
  value.

  Patch 6: Restore Pattern0 PWM to full brightness (0x0FFF) after
  clearing. pattern_clear() zeroes all PWM slots as part of the clear,
  but leaves Pattern0 at zero, so a subsequent direct brightness write
  has no visible effect until Pattern0 PWM is restored.

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

  Patch 7: 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 8: Exit pattern mode in brightness_set() before writing the ILED
  register. With PATS set, the LED output is determined by the pattern
  engine regardless of the ILED value, making direct brightness writes
  have no visible effect while a pattern is active.

  Patch 9: 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.

--- Documentation ---

  Patch 10: 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 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/

Manuel Fombuena (10):
  leds: st1202: stop pattern sequence before reprogramming
  leds: st1202: validate pattern input before stopping the sequence
  leds: st1202: fix pattern duration register calculation
  leds: st1202: fix pattern_clear to explicitly mark unused slots as
    skip
  leds: st1202: reset PAT_REP to POR default in pattern_clear
  leds: st1202: restore Pattern0 PWM to full on 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: correct and extend hw_pattern documentation

 Documentation/leds/leds-st1202.rst |  9 ++-
 drivers/leds/leds-st1202.c         | 95 ++++++++++++++++++------------
 2 files changed, 62 insertions(+), 42 deletions(-)

-- 
2.54.0

[PATCH v2 01/10] 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.

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 2b68cd3c45f8..28e3f1e2314c 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.54.0

[PATCH v2 02/10] 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 28e3f1e2314c..5f4238181057 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.54.0

[PATCH v2 03/10] leds: st1202: fix pattern duration register calculation

[Manuel Fombuena]

The duration register (PATy_DUR) uses a direct encoding: register value N
corresponds to N × 22.2 ms, with 0 reserved as "pattern skip" (§7.10).
The driver incorrectly subtracted 1 from the register value, based on the
assumption that register 0 was the minimum duration rather than a skip
indicator. 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. With the correct
formula 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 and derive the maximum from the register width so the
relationship is explicit.

Update the documentation to reflect the correct maximum.

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         | 4 ++--
 2 files changed, 3 insertions(+), 3 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 5f4238181057..02db1006fb53 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,
-- 
2.54.0

[PATCH v2 04/10] leds: st1202: fix pattern_clear to explicitly mark unused slots as skip

[Manuel Fombuena]

pattern_clear used st1202_duration_pattern_write() with the minimum
duration (22 ms) to reset the eight pattern slots. With the old buggy
formula (value / 22 - 1) this accidentally wrote register value 0,
which the hardware interprets as skip. The corrected formula
(value / 22) maps 22 ms to register 1, a valid 22 ms duration.

As a result, any pattern_set() call with fewer than eight steps left
the remaining slots with 22 ms valid durations and zero PWM, making
the LED appear off for 7 x 22 ms out of every cycle.

Write 0 directly to the duration registers so that 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
---
 drivers/leds/leds-st1202.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
index 02db1006fb53..1ca77fbe4ec9 100644
--- a/drivers/leds/leds-st1202.c
+++ b/drivers/leds/leds-st1202.c
@@ -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.54.0

[PATCH v2 05/10] leds: st1202: reset PAT_REP to POR default in pattern_clear

[Manuel Fombuena]

The Pattern Sequence Repetition register (PAT_REP) is not reset when
clearing a pattern. This leaves a stale repeat count in the register,
most critically 0xFF if the previous pattern was set to infinite loop,
which will be picked up by a subsequent pattern_set() call in the
window between pattern_clear() returning and pattern_set() writing its
own value.

Reset PAT_REP to its power-on reset default of 1 in pattern_clear() to
ensure a clean state.

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

diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
index 1ca77fbe4ec9..9c1ae35dcb4c 100644
--- a/drivers/leds/leds-st1202.c
+++ b/drivers/leds/leds-st1202.c
@@ -204,6 +204,10 @@ static int st1202_led_pattern_clear(struct led_classdev *ldev)
 	if (ret != 0)
 		return ret;
 
+	ret = st1202_write_reg(chip, ST1202_PATTERN_REP, 1);
+	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)
-- 
2.54.0

[PATCH v2 06/10] leds: st1202: restore Pattern0 PWM to full on after clearing pattern

[Manuel Fombuena]

When PATS is cleared the LED output is the product of the ILED analog
current register and the Pattern0 PWM duty cycle. brightness_set only
writes the ILED register, so if Pattern0 PWM is zero the LED remains
dark regardless of the brightness value set.

pattern_clear zeros all pattern PWM registers including Pattern0, which
means any subsequent brightness_set call has no visible effect until a
new pattern is programmed.

Restore Pattern0 PWM to full duty cycle after clearing so that ILED
alone controls brightness in static mode.

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 insertions(+)

diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
index 9c1ae35dcb4c..933c50c317c1 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 {
@@ -218,6 +219,10 @@ static int st1202_led_pattern_clear(struct led_classdev *ldev)
 			return ret;
 	}
 
+	ret = st1202_pwm_pattern_write(chip, led->led_num, 0, ST1202_PATTERN_PWM_FULL);
+	if (ret != 0)
+		return ret;
+
 	return 0;
 }
 
-- 
2.54.0

[PATCH v2 07/10] 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.

This write also has a second unintended effect: it overwrites the
Configuration register without the SHFT bit, silently undoing the
phase-shift write made earlier in the same function. As a result the
phase-shift feature is never actually enabled after probe, despite the
explicit write intended to enable it.

Both problems are caused by the same write. Remove it. The device
reset at the start of setup restores all registers to their power-on
defaults, leaving PATS and PATSR cleared. The Configuration register
correctly retains its earlier SHFT-only value.

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 933c50c317c1..9150b92ccbac 100644
--- a/drivers/leds/leds-st1202.c
+++ b/drivers/leds/leds-st1202.c
@@ -341,11 +341,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.54.0

[PATCH v2 08/10] 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.

Set CONFIG_REG to SHFT (static mode) before writing ILED so that a
direct brightness write always produces a steady output at the
requested level.

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. Previously there was no working path
for this, as brightness writes had no visible effect unless a hardware
pattern had first been programmed.

This is safe because software patterns already run with PATS=0 (making
the write a no-op in that path), and the hardware pattern engine does
not call brightness_set during autonomous pattern execution.

Note that CONFIG_REG is chip-wide, so this clears the pattern mode for
all channels on the 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 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
index 9150b92ccbac..bca2d9a80b63 100644
--- a/drivers/leds/leds-st1202.c
+++ b/drivers/leds/leds-st1202.c
@@ -136,6 +136,7 @@ static void st1202_brightness_set(struct led_classdev *led_cdev,
 
 	guard(mutex)(&chip->lock);
 
+	st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_SHFT);
 	st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num, value);
 }
 
-- 
2.54.0

[PATCH v2 09/10] 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 | 62 +++++++++++++++++++++-----------------
 1 file changed, 34 insertions(+), 28 deletions(-)

diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
index bca2d9a80b63..a3f9057408e1 100644
--- a/drivers/leds/leds-st1202.c
+++ b/drivers/leds/leds-st1202.c
@@ -128,38 +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);
-
-	st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_SHFT);
-	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)
@@ -187,6 +160,39 @@ 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);
+
+	st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_SHFT);
+	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);
-- 
2.54.0

[PATCH v2 10/10] 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.54.0

Re: [PATCH v2 00/10] leds: st1202: fix multiple bugs in pattern engine and brightness handling

[Lee Jones]

Can you take a look through these Sashiko reviews please:

https://sashiko.dev/#/patchset/WA0P291MB03855101311F0506B6448A8EC50D2%40WA0P291MB0385.POLP291.PROD.OUTLOOK.COM

> 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. The original formula
>   (value / ST1202_MILLIS_PATTERN_DUR_MIN - 1) produced an off-by-one
>   result, writing the minimum valid duration (22ms, register value 0x01)
>   instead of the skip marker (0x00) for unused slots.
> 
>   Patch 4: Write the skip marker (0x00) directly to unused duration
>   registers in pattern_clear() rather than going through
>   st1202_duration_pattern_write(), which operates on millisecond values
>   and cannot express register value zero.
> 
>   Patch 5: Reset PAT_REP to its power-on default of 1 in pattern_clear().
>   A stale value — most critically 0xFF from a previous infinite loop —
>   would be picked up by a subsequent pattern_set() call in the window
>   between pattern_clear() returning and pattern_set() writing its own
>   value.
> 
>   Patch 6: Restore Pattern0 PWM to full brightness (0x0FFF) after
>   clearing. pattern_clear() zeroes all PWM slots as part of the clear,
>   but leaves Pattern0 at zero, so a subsequent direct brightness write
>   has no visible effect until Pattern0 PWM is restored.
> 
> --- Spurious pattern sequence start during setup ---
> 
>   Patch 7: 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 8: Exit pattern mode in brightness_set() before writing the ILED
>   register. With PATS set, the LED output is determined by the pattern
>   engine regardless of the ILED value, making direct brightness writes
>   have no visible effect while a pattern is active.
> 
>   Patch 9: 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.
> 
> --- Documentation ---
> 
>   Patch 10: 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 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/
> 
> Manuel Fombuena (10):
>   leds: st1202: stop pattern sequence before reprogramming
>   leds: st1202: validate pattern input before stopping the sequence
>   leds: st1202: fix pattern duration register calculation
>   leds: st1202: fix pattern_clear to explicitly mark unused slots as
>     skip
>   leds: st1202: reset PAT_REP to POR default in pattern_clear
>   leds: st1202: restore Pattern0 PWM to full on 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: correct and extend hw_pattern documentation
> 
>  Documentation/leds/leds-st1202.rst |  9 ++-
>  drivers/leds/leds-st1202.c         | 95 ++++++++++++++++++------------
>  2 files changed, 62 insertions(+), 42 deletions(-)
> 
> -- 
> 2.54.0
> 

-- 
Lee Jones