[PATCH v2 00/13] leds: st1202: Naming cleanups and pre-existing fixes

[PATCH v2 00/13] leds: st1202: Naming cleanups and pre-existing fixes

[Manuel Fombuena]

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

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

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

Patches 1 to 4 are naming and readability only, with no functional
change; two of them address comments from the blink support review.
Patches 5 to 12 are fixes and carry Fixes tags, and patch 13 moves the
channel current ceiling into a single helper.

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

Patches 6 and 7 belong together. Pattern brightness values were written
into the 12-bit PWM registers unscaled, so a step asking for full
brightness produced about 6% duty cycle; and the analog current register
was never programmed at all, so a channel whose current had been left at
zero stayed dark however bright the pattern. Together they make a
pattern step of N produce the same output as writing N to brightness,
which is what the documented hw_pattern range implies. Patch 7 takes the
current up in pattern_set() and releases it in pattern_clear(), so a
rejected hw_pattern write cannot leave a channel latched on, and a board
declaring a lower ceiling through max-brightness keeps it. Releasing it
also disables the channel, because a current of zero still leaves a
faint glow on this part. Probe clears every channel, so the LEDs now
reach user space switched off, rather than lit at the power-on current.

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

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

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

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

Patches 12 and 13 deal with a max-brightness above 255, which the device
tree allows but the 8-bit current register cannot hold. Patch 12 caps
the value st1202_led_set() writes, which previously wrapped, and is kept
to that alone so it can be backported. Patch 13 then gives the ceiling a
single helper shared by the three paths that program the current.

Three points raised in earlier reviews are deliberately left alone:

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

  - st1202_blink_set() turning the other active channels off while it
    runs is intended, as described in commit 35cbd34a6a05 ("leds: st1202:
    Add hardware-accelerated blink support"). The sequencer cannot keep
    their own timing once a blink takes it over, so they are kept dark
    rather than outputting unintended values.

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

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

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

--- Changes in v2 ---

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

  Patch 5: the commit message attributed the full-scale PWM left in the
  unused slots to probe only. The brightness path and every activation
  of the pattern trigger leave it there too. Commit message corrected;
  no code change.

  Patch 6: the documentation now says that a max-brightness above 255
  saturates, since the current register is 8 bits wide.

  Patch 7: v1 wrote the channel current after starting the sequencer, so
  a new pattern began at the previous current until that write landed.
  The current is now programmed before the channel is enabled and the
  sequencer started. pattern_clear() still releases it first, and now
  also disables the channel: on hardware, a current of zero still left
  a faint glow after a rejected hw_pattern write and at boot. Probe no
  longer enables the channels; st1202_setup() already disables them
  all, and each is enabled when it is lit.

  Patch 8: the missing reference dates from the original driver, which
  already registered the LED class devices after the iterator had
  dropped its reference to the node. The Fixes tag now names the
  driver's original commit instead of commit c72e455b89f2 ("leds:
  leds-st1202: Fix NULL pointer access on race condition"), and the
  commit message covers both.

  Patch 9: st1202_channel_set() is removed, as it no longer has any
  callers.

  Patch 11: no longer moves the current write in st1202_blink_set(), so
  it stays before the sequencer is started, for the same reason as
  patch 7.

  Patch 12 (new): st1202_led_set() caps the channel current at the width
  of the register.

  Patch 13 (new): the current ceiling moves into a single helper, so the
  three paths that program the current share one limit.

  Other pre-existing issues identified by the automated review stem from
  the sequencer and its duration registers being shared by all channels,
  and are outside the scope of this series, as noted above.

v1: https://lore.kernel.org/all/GV1PR08MB8497422B7A44424C4968AEC1C5BD2@GV1PR08MB8497.eurprd08.prod.outlook.com/

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

 Documentation/leds/leds-st1202.rst |   5 +
 drivers/leds/leds-st1202.c         | 163 +++++++++++++++++++----------
 2 files changed, 111 insertions(+), 57 deletions(-)

-- 
2.55.0

[PATCH v2 01/13] leds: st1202: Correct the name of the prescaler conversion helper

[Manuel Fombuena]

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

Rename it to st1202_milliseconds_to_prescaler(). No functional change.

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

diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
index ead144155849..5a2202fb4c22 100644
--- a/drivers/leds/leds-st1202.c
+++ b/drivers/leds/leds-st1202.c
@@ -85,7 +85,7 @@ static int st1202_write_reg(struct st1202_chip *chip, int reg, uint8_t val)
 	return ret;
 }
 
-static uint8_t st1202_prescalar_to_miliseconds(unsigned int value)
+static uint8_t st1202_milliseconds_to_prescaler(unsigned int value)
 {
 	return value / ST1202_MILLIS_PATTERN_DUR_MIN;
 }
@@ -126,7 +126,7 @@ static int st1202_duration_pattern_write(struct st1202_chip *chip, int pattern,
 					unsigned int value)
 {
 	return st1202_write_reg(chip, (ST1202_PATTERN_DUR + pattern),
-				st1202_prescalar_to_miliseconds(value));
+				st1202_milliseconds_to_prescaler(value));
 }
 
 static int __st1202_channel_set(struct st1202_chip *chip, int led_num, bool active)
-- 
2.55.0

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

[Manuel Fombuena]

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

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

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

No functional change.

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

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

[PATCH v2 03/13] leds: st1202: Name pattern index variables consistently

[Manuel Fombuena]

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

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

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

No functional change.

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

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

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

[Manuel Fombuena]

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

Convert the remaining uint8_t uses to u8.

No functional change.

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

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

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

[Manuel Fombuena]

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

st1202_led_pattern_set() programs only the first len slots. The pattern
trigger calls pattern_clear() before pattern_set() only once a channel is
already in PATTERN_TYPE_HW mode, and activating the trigger resets that
mode, so the first hardware pattern written after every activation skips
it. If another channel has already programmed a longer pattern,
durations [len, 8) are still set, and this channel's PWM for those slots
holds whatever was last written there.

That used to be LED_OFF, so a channel stayed dark for the slots it had
not programmed itself. Since commit d2ca0e2b6d64 ("leds: st1202: Set all
pattern PWM slots to full after clearing pattern") pattern_clear() leaves
ST1202_PATTERN_PWM_FULL there, and so does the brightness path since
commit 7cbe470366bd ("leds: st1202: Fix brightness having no effect while
pattern mode is active"). The channel now plays its own steps and then
sits at full brightness for the rest of the sequence.

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

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

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

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

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

[Manuel Fombuena]

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

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

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

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

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

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

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

[PATCH v2 07/13] leds: st1202: Program the channel current for hardware patterns

[Manuel Fombuena]

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

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

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

therefore programs a pattern that never lights up.

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

A current of zero is not enough to release the channel, though: the
datasheet notes that it does not switch the channel off but leaves a
small uncontrolled current, which shows as a faint glow. pattern_clear()
therefore also disables the channel, and pattern_set() enables it again.
Both are done before the sequencer is stopped, so an I2C error partway
through a clear cannot leave the channel lit.

Probe clears every channel, so the LEDs now reach user space switched
off. Until now they came up lit at the chip's power-on current, because
pattern_clear() fills every slot at full scale. Probe no longer enables
the channels itself either: st1202_setup() has already disabled them
all, and each path that lights a channel enables it when needed.

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

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

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

diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
index 5558ac9612a1..41eac6195659 100644
--- a/drivers/leds/leds-st1202.c
+++ b/drivers/leds/leds-st1202.c
@@ -221,6 +221,14 @@ static int st1202_led_pattern_clear(struct led_classdev *ldev)
 
 	guard(mutex)(&chip->lock);
 
+	ret = st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num, LED_OFF);
+	if (ret != 0)
+		return ret;
+
+	ret = __st1202_channel_set(chip, led->led_num, false);
+	if (ret != 0)
+		return ret;
+
 	ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_PHASE_SHIFT);
 	if (ret != 0)
 		return ret;
@@ -287,6 +295,10 @@ static int st1202_led_pattern_set(struct led_classdev *ldev,
 	if (ret != 0)
 		return ret;
 
+	ret = st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num, max_brightness);
+	if (ret != 0)
+		return ret;
+
 	ret = __st1202_channel_set(chip, led->led_num, true);
 	if (ret != 0)
 		return ret;
@@ -455,7 +467,7 @@ static int st1202_setup(struct st1202_chip *chip)
 	/* Duration of initialization */
 	usleep_range(6500, 10000);
 
-	/* Deactivate all LEDS (channels) and activate only the ones found in Device Tree */
+	/* Deactivate all LEDs (channels); each is enabled when it is lit */
 	ret = st1202_write_reg(chip, ST1202_CHAN_ENABLE_LOW, ST1202_CHAN_DISABLE_ALL);
 	if (ret < 0)
 		return ret;
@@ -502,11 +514,6 @@ static int st1202_probe(struct i2c_client *client)
 		if (!led->is_active)
 			continue;
 
-		ret = st1202_channel_set(led->chip, led->led_num, true);
-		if (ret < 0)
-			return dev_err_probe(&client->dev, ret,
-					"Failed to activate LED channel\n");
-
 		ret = st1202_led_pattern_clear(&led->led_cdev);
 		if (ret < 0)
 			return dev_err_probe(&client->dev, ret,
-- 
2.55.0

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

[Manuel Fombuena]

st1202_dt_init() stores each LED's firmware node while walking the
device tree with for_each_available_child_of_node_scoped(), which drops
its reference to the node at the end of every iteration. The LED class
stores the pointer with device_set_node() and does not take a reference
of its own, so nothing keeps the node alive for as long as the LED
device uses it. That has been the case since the driver was added, when
the LED class devices were registered inside that same loop.

Commit c72e455b89f2 ("leds: leds-st1202: Fix NULL pointer access on race
condition") moved the registration into a separate loop in
st1202_probe(), which also leaves the stored pointer unreferenced
between the two loops.

With CONFIG_OF_DYNAMIC the node can be freed while it is still in use,
leaving led->fwnode and the LED device's firmware node dangling.

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

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

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

[PATCH v2 09/13] leds: st1202: Do not set brightness from atomic context

[Manuel Fombuena]

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

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

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

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

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

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

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

[Manuel Fombuena]

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

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

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

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

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

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

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

[Manuel Fombuena]

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

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

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

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

diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
index 42989b4fbaf3..39129206b78d 100644
--- a/drivers/leds/leds-st1202.c
+++ b/drivers/leds/leds-st1202.c
@@ -382,7 +382,8 @@ static int st1202_blink_set(struct led_classdev *led_cdev,
 	if (ret)
 		return ret;
 
-	ret = st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num, U8_MAX);
+	ret = st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num,
+				min_t(unsigned int, led_cdev->max_brightness, U8_MAX));
 	if (ret)
 		return ret;
 
-- 
2.55.0

[PATCH v2 12/13] leds: st1202: Cap the channel current in st1202_led_set()

[Manuel Fombuena]

st1202_led_set() writes the requested brightness straight into the 8-bit
channel current register. The LED core limits brightness to
max_brightness, which comes from the device tree max-brightness property
and has no upper bound, so on a board declaring more than 255 a
brightness of 256 reaches the register as 0 and turns the LED off, and
larger values wrap around.

Cap the value at U8_MAX, the width of the register, so anything above it
drives the channel at full current. The pattern and blink paths already
cap the current they program the same way.

Fixes: 259230378c65 ("leds: Add LED1202 I2C driver")
Signed-off-by: Manuel Fombuena <fombuena@outlook.com>
Assisted-by: LLM
---
 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 39129206b78d..5e4a8b80cfe4 100644
--- a/drivers/leds/leds-st1202.c
+++ b/drivers/leds/leds-st1202.c
@@ -207,7 +207,8 @@ static int st1202_led_set(struct led_classdev *ldev, enum led_brightness value)
 			return ret;
 	}
 
-	ret = st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num, value);
+	ret = st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num,
+				min_t(unsigned int, value, U8_MAX));
 	if (ret)
 		return ret;
 
-- 
2.55.0

[PATCH v2 13/13] leds: st1202: Give the channel current ceiling a single helper

[Manuel Fombuena]

The channel current register is 8 bits wide, while max_brightness comes
from the device tree and can be larger. The brightness, pattern and
blink paths each apply that limit on their own, open-coding the same
min_t() against U8_MAX, and the blink path previously drifted from the
others by ignoring max_brightness altogether.

Move the limit into st1202_iled_max() and use it in all three places.
Each path programs the same value as before: the LED core already limits
brightness to max_brightness, so capping it against st1202_iled_max() in
st1202_led_set() gives the same result as capping it against U8_MAX.

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

diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
index 5e4a8b80cfe4..8c2e1d58ba0c 100644
--- a/drivers/leds/leds-st1202.c
+++ b/drivers/leds/leds-st1202.c
@@ -92,6 +92,12 @@ static u8 st1202_milliseconds_to_prescaler(unsigned int value)
 	return value / ST1202_MILLIS_PATTERN_DUR_MIN;
 }
 
+/* The channel current register is 8 bits wide, whatever max-brightness says */
+static u8 st1202_iled_max(struct led_classdev *led_cdev)
+{
+	return min_t(unsigned int, led_cdev->max_brightness, U8_MAX);
+}
+
 static u16 st1202_brightness_to_pwm(int brightness, unsigned int max_brightness)
 {
 	if (!max_brightness)
@@ -208,7 +214,7 @@ static int st1202_led_set(struct led_classdev *ldev, enum led_brightness value)
 	}
 
 	ret = st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num,
-				min_t(unsigned int, value, U8_MAX));
+				min_t(unsigned int, value, st1202_iled_max(ldev)));
 	if (ret)
 		return ret;
 
@@ -258,7 +264,7 @@ static int st1202_led_pattern_set(struct led_classdev *ldev,
 	unsigned int max_brightness;
 	int ret;
 
-	max_brightness = min_t(unsigned int, ldev->max_brightness, U8_MAX);
+	max_brightness = st1202_iled_max(ldev);
 
 	if (len > ST1202_MAX_PATTERNS)
 		return -EINVAL;
@@ -383,8 +389,7 @@ static int st1202_blink_set(struct led_classdev *led_cdev,
 	if (ret)
 		return ret;
 
-	ret = st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num,
-				min_t(unsigned int, led_cdev->max_brightness, U8_MAX));
+	ret = st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num, st1202_iled_max(led_cdev));
 	if (ret)
 		return ret;
 
-- 
2.55.0

Re: [PATCH v2 03/13] leds: st1202: Name pattern index variables consistently

[sashiko-bot]

> The driver already names pattern indices "pattern" in the signatures of
> st1202_pwm_pattern_write() and st1202_duration_pattern_write(), and
> st1202_blink_set() follows the same convention. The remaining loops
> abbreviate it to "patt", which matches nothing else in the file.
> 
> Rename those loop variables to "pattern". In st1202_led_pattern_set()
> that name is taken by the array of pattern steps, so rename the
> parameter to "patterns", which also reflects better that it is an array.
> 
> Two st1202_pwm_pattern_write() calls exceed 100 columns with the longer
> name and are wrapped accordingly.
> 
> No functional change.
> 
> Signed-off-by: Manuel Fombuena <fombuena@outlook.com>

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

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

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

[sashiko-bot]

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

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

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

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

[sashiko-bot]

> st1202_dt_init() stores each LED's firmware node while walking the
> device tree with for_each_available_child_of_node_scoped(), which drops
> its reference to the node at the end of every iteration. The LED class
> stores the pointer with device_set_node() and does not take a reference
> of its own, so nothing keeps the node alive for as long as the LED
> device uses it. That has been the case since the driver was added, when
> the LED class devices were registered inside that same loop.
> 
> Commit c72e455b89f2 ("leds: leds-st1202: Fix NULL pointer access on race
> condition") moved the registration into a separate loop in
> st1202_probe(), which also leaves the stored pointer unreferenced
> between the two loops.
> 
> With CONFIG_OF_DYNAMIC the node can be freed while it is still in use,
> leaving led->fwnode and the LED device's firmware node dangling.
> 
> Take a reference when the node is stored and release it through a devm
> action. The action is registered before the LED class devices are, so it
> runs after they have been unregistered.
> 
> Fixes: 259230378c65 ("leds: Add LED1202 I2C driver")
> Signed-off-by: Manuel Fombuena <fombuena@outlook.com>

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

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

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

[sashiko-bot]

> The pattern duration registers are chip-wide while the PWM registers are
> per-channel, so a channel can end up driven by a sequencer timeline it
> never programmed.
> 
> st1202_led_pattern_set() programs only the first len slots. The pattern
> trigger calls pattern_clear() before pattern_set() only once a channel is
> already in PATTERN_TYPE_HW mode, and activating the trigger resets that
> mode, so the first hardware pattern written after every activation skips
> it. If another channel has already programmed a longer pattern,
> durations [len, 8) are still set, and this channel's PWM for those slots
> holds whatever was last written there.
> 
> That used to be LED_OFF, so a channel stayed dark for the slots it had
> not programmed itself. Since commit d2ca0e2b6d64 ("leds: st1202: Set all
> pattern PWM slots to full after clearing pattern") pattern_clear() leaves
> [ ... ]
> from having a single hardware sequencer and is not something this can
> correct.
> 
> Fixes: d2ca0e2b6d64 ("leds: st1202: Set all pattern PWM slots to full after clearing pattern")
> Signed-off-by: Manuel Fombuena <fombuena@outlook.com>

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

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

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

[sashiko-bot]

> st1202_prescalar_to_miliseconds() converts a duration in milliseconds
> into the prescaler value written to the pattern duration registers, so
> its name states the conversion the wrong way round. It also misspells
> both "prescaler" and "milliseconds".
> 
> Rename it to st1202_milliseconds_to_prescaler(). No functional change.
> 
> Signed-off-by: Manuel Fombuena <fombuena@outlook.com>

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

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

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

[sashiko-bot]

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

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

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

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

[sashiko-bot]

> The LED1202 PWM registers are 12 bits wide, but st1202_led_pattern_set()
> writes the 8-bit brightness value carried by struct led_pattern into
> them unscaled. A pattern step asking for full brightness therefore
> drives the channel at 255 out of 4095, about 6% duty cycle, so a pattern
> is dimmer than the same brightness set directly.
> 
> Scale the value to the full PWM range before programming it.
> 
> Scale against max_brightness rather than the register maximum. The LED
> core takes max_brightness from the DT max-brightness property, so a
> board can declare a ceiling below full scale, and the pattern trigger
> already rejects steps above it. Scaling against a hardcoded 255 would
> leave such a board unable to reach its own declared maximum from the
> pattern path while the brightness path reaches it.
> 
> Scaling divides by max_brightness. The LED core replaces a zero
> max-brightness from the device tree with LED_FULL, so the divisor should
> never be zero, but the helper returns zero rather than rely on that.
> max_brightness is capped at U8_MAX, the driver's own maximum, so a
> device tree declaring more than the part supports is treated as full
> scale. struct led_pattern brightness is signed, so clamp both ends and
> floor a negative value to LED_OFF rather than let it wrap.
> 
> Document that a step is relative to max_brightness, since the range
> given in the documentation no longer holds when the device tree sets
> max-brightness.
> 
> Fixes: 259230378c65 ("leds: Add LED1202 I2C driver")
> Signed-off-by: Manuel Fombuena <fombuena@outlook.com>

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

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

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

[sashiko-bot]

> ST1202_CONFIG_REG_SHFT is abbreviated to the point of being unreadable
> and, unlike the neighbouring PATS and PATSR bits, carries no comment
> explaining what it controls.
> 
> Rename it to ST1202_CONFIG_REG_PHASE_SHIFT and document it in the same
> style as the other configuration bits, retaining the datasheet name in
> the comment so it remains searchable.
> 
> The longer name no longer fits the existing layout in
> st1202_led_pattern_set(), where the continuation line was already 106
> columns wide. Re-wrap that register write to match the layout
> st1202_blink_set() uses for the same operation and drop the redundant
> parentheses around the bit mask.
> 
> No functional change.
> 
> Signed-off-by: Manuel Fombuena <fombuena@outlook.com>

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

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

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

[sashiko-bot]

> A channel's output is ILED x Pattern_PWM / 4095, so a pattern is only
> visible while the analog current register holds a non-zero value.
> st1202_led_pattern_set() programs the PWM slots and starts the sequencer
> but never writes ILED, so a channel whose current is zero stays dark
> while pattern_set() reports success.
> 
> Zero is reached by ordinary use. Writing 0 to brightness sets it, and so
> does switching away from the pattern trigger, because
> pattern_trig_deactivate() ends with led_set_brightness(LED_OFF). A
> sequence as simple as
> 
>   echo none > trigger
>   echo pattern > trigger
>   echo "255 500 0 500" > hw_pattern
> 
> [ ... ]
> last brightness written, decide what the channel shows while a pattern
> is running.
> 
> Fixes: 259230378c65 ("leds: Add LED1202 I2C driver")
> Signed-off-by: Manuel Fombuena <fombuena@outlook.com>

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

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

Re: [PATCH v2 13/13] leds: st1202: Give the channel current ceiling a single helper

[sashiko-bot]

> The channel current register is 8 bits wide, while max_brightness comes
> from the device tree and can be larger. The brightness, pattern and
> blink paths each apply that limit on their own, open-coding the same
> min_t() against U8_MAX, and the blink path previously drifted from the
> others by ignoring max_brightness altogether.
> 
> Move the limit into st1202_iled_max() and use it in all three places.
> Each path programs the same value as before: the LED core already limits
> brightness to max_brightness, so capping it against st1202_iled_max() in
> st1202_led_set() gives the same result as capping it against U8_MAX.
> 
> Signed-off-by: Manuel Fombuena <fombuena@outlook.com>

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

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

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

[sashiko-bot]

> st1202_blink_set() programs slots 0 and 1 of its own channel and leaves
> slots 2 to 7 at ST1202_PATTERN_PWM_FULL, the value both st1202_led_set()
> and st1202_led_pattern_clear() leave behind. Nothing keeps those slots
> out of the output except the duration registers being zero, and those
> are shared by all twelve channels.
> 
> Another channel programming a hardware pattern longer than two steps
> writes those durations again and re-arms the sequencer, so the blinking
> channel drives full scale for the extra steps. With two LEDs on one
> device, blinking one and giving the other a four step pattern, the
> blinking channel lights during the two steps it never programmed.
> 
> Zero the channel's own unused slots, the same invariant
> st1202_led_pattern_set() applies to the slots beyond its pattern length.
> blink_set() already zeroes durations 2 to 7 when it runs; the slots are
> exposed only when another channel sets those shared durations again.
> 
> A channel whose blink is shorter than the active timeline still loses
> its own timing, because the sequencer has one set of duration registers.
> This only stops it emitting output it never asked for.
> 
> Fixes: 35cbd34a6a05 ("leds: st1202: Add hardware-accelerated blink support")
> Signed-off-by: Manuel Fombuena <fombuena@outlook.com>

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

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

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

[sashiko-bot]

> st1202_blink_set() drives the channel current register to U8_MAX, the
> full scale of the part, regardless of the ceiling the board declared.
> led_classdev_register_ext() takes max_brightness from the device tree
> max-brightness property. The driver does not implement led-max-microamp,
> so that is the only way a board can ask for less than 20 mA.
> 
> The brightness and pattern paths both respect it. Blinking is the one
> that does not, so a board that declares a lower ceiling holds it until
> something enables the timer trigger.
> 
> Program the same value the other paths use, capped at the width of the
> current register so an out of range property cannot wrap it to zero.
> 
> Fixes: 35cbd34a6a05 ("leds: st1202: Add hardware-accelerated blink support")
> Signed-off-by: Manuel Fombuena <fombuena@outlook.com>

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

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

Re: [PATCH v2 12/13] leds: st1202: Cap the channel current in st1202_led_set()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] leds: broken brightness scaling and saturation when max-brightness > 255
--

commit 1bc7b66ecae1ce3e4dd27e771fe90fae2aa2ecc9
Author: Manuel Fombuena <fombuena@outlook.com>
Subject: leds: st1202: Cap the channel current in st1202_led_set()

This commit limits the channel current register value written in
st1202_led_set to U8_MAX to prevent 8-bit wrap-around when the device
tree declares a max-brightness greater than 255.

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index 39129206b78d6..5e4a8b80cfe47 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
> @@ -207,7 +207,8 @@ static int st1202_led_set(struct led_classdev *ldev, enum led_brightness value)
>  			return ret;
>  	}
>  
> -	ret = st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num, value);
> +	ret = st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num,
> +				min_t(unsigned int, value, U8_MAX));

[Severity: Medium]
Does capping the value here instead of scaling it cause broken dimming
curves when max-brightness is configured to a value greater than 255 in
the device tree?

If a value like 4095 is used for max-brightness, capping instead of
linearly scaling causes all requests above 255 to fully saturate the
output at max hardware current rather than scaling proportionately.

Additionally, does this clamping affect the digital PWM scaler in
st1202_brightness_to_pwm() when called from st1202_led_pattern_set()?

If the clamped analog limit of 255 is passed to st1202_brightness_to_pwm()
as both the clamp limit and the scaling denominator instead of the actual
ldev->max_brightness, the function appears to calculate:

255 * 4095 / 255 = 4095

This would result in a 100% PWM duty cycle for any pattern brightness
above 255.

Would the lack of linear scaling also cause confusion during sysfs reads
if st1202_brightness_get() returns the raw 8-bit register value instead
of a scaled representation up to ldev->max_brightness?

>  	if (ret)
>  		return ret;

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

Re: [PATCH v2 12/13] leds: st1202: Cap the channel current in st1202_led_set()

[Manuel Fombuena]

On Tue, 2026-09-15 at 13:06 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> - [Medium] leds: broken brightness scaling and saturation when max-
> brightness > 255
> --
> 

A max-brightness above 255 asks for more than the 8-bit current register
can hold. The driver treats max-brightness as a ceiling, as the
brightness, pattern and blink paths already do, so values above 255
saturate at full current rather than being scaled. This is documented
in leds-st1202.rst by patch 6. The pattern path and the brightness
readback follow the same rule.

-- 
Manuel Fombuena

Re: [PATCH v2 00/13] leds: st1202: Naming cleanups and pre-existing fixes

[Manuel Fombuena]

Out of the 13 patches in v2, Sashiko AI review only found 1 potential
issue in patch 12. That was addressed in a direct reply to that email
and doesn't require changes.

Unless another reviewer suggests changes, there will not be a v3 for
this series.

-- 
Manuel Fombuena

Re: [PATCH v2 00/13] leds: st1202: Naming cleanups and pre-existing fixes

[Lee Jones]

On Tue, 15 Sep 2026 13:40:05 +0100, Manuel Fombuena wrote:
> This series collects the issues identified during review of the two
> previous ST1202 submissions, the pattern engine fix series and the
> hardware-accelerated blink support:
> 
> https://lore.kernel.org/all/GV1PR08MB8497C0B898789BB73ACE6EE3C5F52@GV1PR08MB8497.eurprd08.prod.outlook.com/
> https://lore.kernel.org/all/GV1PR08MB84974C7ACD911E66864C5008C5DA2@GV1PR08MB8497.eurprd08.prod.outlook.com/
> 
> [...]

Applied, thanks!

[01/13] leds: st1202: Correct the name of the prescaler conversion helper
        commit: 226f32543668960673596288a50191052dba3352
[02/13] leds: st1202: Spell out the phase-shift configuration bit
        commit: 7520fc78c52ea55c2c83d0ecc11ab2355a753b7f
[03/13] leds: st1202: Name pattern index variables consistently
        commit: 0aed225f40613253617ad30a57492043a3448938
[04/13] leds: st1202: Use u8 consistently for 8-bit values
        commit: 38f98583bcac95a65774b73cbcc56280a71f5612
[05/13] leds: st1202: Clear unused pattern slots in pattern_set()
        commit: 119a7d375e5caedaf252198fa64e7df5d7269ab4
[06/13] leds: st1202: Scale pattern brightness to the 12-bit PWM range
        commit: 82f8b88d6ac2edf01651bbd84e03ede04b2ca80e
[07/13] leds: st1202: Program the channel current for hardware patterns
        commit: 878eec1475e77ad22059584bc617b6e671a536d9
[08/13] leds: st1202: Take a reference on the LED firmware node
        commit: 5882de94edf7a53812a76a882a95e1e86d7105d9
[09/13] leds: st1202: Do not set brightness from atomic context
        commit: 1ec35fa181981b4b9581b3dae967bac7ed82c89c
[10/13] leds: st1202: Clear unused pattern slots in blink_set()
        commit: 3268b9e91db16f240ffe07ff6caf963ba8191ce8
[11/13] leds: st1202: Honour max_brightness when blinking
        commit: 865d81a98602c828515fa6ac3c530efc4205f9f6
[12/13] leds: st1202: Cap the channel current in st1202_led_set()
        commit: ca81ea108502817e3841e98f0d3651398d9e1bd9
[13/13] leds: st1202: Give the channel current ceiling a single helper
        commit: e264a106d8fc45fd82891cb0c4f8bd9e2c48c152

--
Lee Jones [李琼斯]