[PATCH v3 0/1] leds: st1202: Add hardware-accelerated blink support

[PATCH v3 0/1] leds: st1202: Add hardware-accelerated blink support

[Manuel Fombuena]

This patch adds blink_set() to the ST1202 LED driver, enabling
hardware-accelerated blinking via the timer trigger.

A series of nine fixes to the pattern engine and brightness handling
was recently applied to for-leds-next:

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

With those fixes in place, the pattern engine can be used reliably to
implement blink_set(): a two-step pattern (full brightness for delay_on,
off for delay_off) is programmed and started in infinite repeat mode.
Requested delays are clamped to the hardware range and rounded up to
the nearest 22ms step.

During review of the fix series, several pre-existing issues were
identified in the driver — including brightness_set() being assigned to
a non-blocking callback, the global sequencer affecting all channels on
pattern operations, and missing brightness scaling in pattern_set().
These do not affect blink_set(): the callback is not invoked from atomic
context, the function explicitly programs all other channels' PWM slots
to zero before starting the sequencer, and channel brightness is set
directly via the ILED register. The pre-existing issues will be
addressed in a follow-up submission.

Tested on LED1202 hardware via I2C on a Linksys MX4200v2 router running
OpenWrt. Hardware blinking confirmed functional with the timer trigger.

--- Changes in v3 ---

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

  Clamp delay inputs to the hardware maximum before calling roundup() to
  prevent integer overflow for extreme values near ULONG_MAX. The final
  clamp_val() enforces the full [22ms, 5610ms] range as before.

--- Changes in v2 ---

  Use short-form ternary for default delay substitution.
  Perform roundup before clamp_val to ensure rounding cannot exceed
  the hardware maximum.
  Capitalise commit subject per LED subsystem convention.
  Drop Assisted-by tag per maintainer suggestion.

v1: https://lore.kernel.org/all/GV1PR08MB8497C5B8CEB2CE19743DFCFFC5FA2@GV1PR08MB8497.eurprd08.prod.outlook.com/
v2: https://lore.kernel.org/all/GV1PR08MB84979858573735294D41A189C5C02@GV1PR08MB8497.eurprd08.prod.outlook.com/

Manuel Fombuena (1):
  leds: st1202: Add hardware-accelerated blink support

 drivers/leds/leds-st1202.c | 84 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

-- 
2.55.0

[PATCH v3 1/1] leds: st1202: Add hardware-accelerated blink support

[Manuel Fombuena]

Implement blink_set() to enable hardware-accelerated blinking via the
timer trigger. The LED1202 pattern engine is used to produce a two-step
sequence: full brightness for delay_on, off for delay_off, repeating
indefinitely.

Requested delays are clamped to the hardware maximum before rounding up
to the nearest 22ms step, then clamped again to the full hardware range
[22ms, 5610ms]. The pre-clamp prevents integer overflow in roundup() for
extreme input values near ULONG_MAX. A zero delay is replaced with the
default of 500ms independently for each of delay_on and delay_off.

The LED1202 pattern sequencer is global and its timing registers are
shared across all channels, so only one blink configuration can be
active at a time. Other active channels have their PWM slots zeroed for
both pattern steps so they remain dark rather than outputting unintended
values when the sequencer runs. The target channel's ILED register is
set to full brightness and the channel is enabled, since the timer
trigger deactivates the current trigger before calling blink_set which
would otherwise leave the channel disabled.

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

diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
index 168df5ecf27b..3600409d4bc2 100644
--- a/drivers/leds/leds-st1202.c
+++ b/drivers/leds/leds-st1202.c
@@ -15,6 +15,7 @@
 #include <linux/slab.h>
 #include <linux/string.h>
 
+#define ST1202_BLINK_DEFAULT_DELAY         500
 #define ST1202_CHAN_DISABLE_ALL            0x00
 #define ST1202_CHAN_ENABLE_HIGH            0x03
 #define ST1202_CHAN_ENABLE_LOW             0x02
@@ -275,6 +276,88 @@ static int st1202_led_pattern_set(struct led_classdev *ldev,
 	return 0;
 }
 
+static int st1202_blink_set(struct led_classdev *led_cdev,
+			unsigned long *delay_on, unsigned long *delay_off)
+{
+	struct st1202_led *led = cdev_to_st1202_led(led_cdev);
+	struct st1202_chip *chip = led->chip;
+	unsigned long on, off;
+	int ret;
+
+	on = *delay_on ?: ST1202_BLINK_DEFAULT_DELAY;
+	off = *delay_off ?: ST1202_BLINK_DEFAULT_DELAY;
+
+	on = min_t(unsigned long, on, ST1202_MILLIS_PATTERN_DUR_MAX);
+	off = min_t(unsigned long, off, ST1202_MILLIS_PATTERN_DUR_MAX);
+	on = roundup(on, ST1202_MILLIS_PATTERN_DUR_MIN);
+	off = roundup(off, ST1202_MILLIS_PATTERN_DUR_MIN);
+	on = clamp_val(on, ST1202_MILLIS_PATTERN_DUR_MIN, ST1202_MILLIS_PATTERN_DUR_MAX);
+	off = clamp_val(off, ST1202_MILLIS_PATTERN_DUR_MIN, ST1202_MILLIS_PATTERN_DUR_MAX);
+
+	guard(mutex)(&chip->lock);
+
+	ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_SHFT);
+	if (ret)
+		return ret;
+
+	/* Zero out PWM for all other active channels to prevent them from blinking */
+	for (int i = 0; i < ST1202_MAX_LEDS; i++) {
+		if (!chip->leds[i].is_active || i == led->led_num)
+			continue;
+		ret = st1202_pwm_pattern_write(chip, i, 0, LED_OFF);
+		if (ret)
+			return ret;
+		ret = st1202_pwm_pattern_write(chip, i, 1, LED_OFF);
+		if (ret)
+			return ret;
+	}
+
+	ret = st1202_pwm_pattern_write(chip, led->led_num, 0, ST1202_PATTERN_PWM_FULL);
+	if (ret)
+		return ret;
+	ret = st1202_pwm_pattern_write(chip, led->led_num, 1, LED_OFF);
+	if (ret)
+		return ret;
+
+	ret = st1202_write_reg(chip, ST1202_PATTERN_DUR,
+				st1202_prescalar_to_miliseconds(on));
+	if (ret)
+		return ret;
+	ret = st1202_write_reg(chip, ST1202_PATTERN_DUR + 1,
+				st1202_prescalar_to_miliseconds(off));
+	if (ret)
+		return ret;
+
+	for (int patt = 2; patt < ST1202_MAX_PATTERNS; patt++) {
+		ret = st1202_write_reg(chip, ST1202_PATTERN_DUR + patt, 0);
+		if (ret)
+			return ret;
+	}
+
+	ret = st1202_write_reg(chip, ST1202_PATTERN_REP, U8_MAX);
+	if (ret)
+		return ret;
+
+	ret = st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num, U8_MAX);
+	if (ret)
+		return ret;
+
+	ret = __st1202_channel_set(chip, led->led_num, true);
+	if (ret)
+		return ret;
+
+	ret = st1202_write_reg(chip, ST1202_CONFIG_REG,
+				ST1202_CONFIG_REG_PATSR | ST1202_CONFIG_REG_PATS |
+				ST1202_CONFIG_REG_SHFT);
+	if (ret)
+		return ret;
+
+	*delay_on = on;
+	*delay_off = off;
+
+	return 0;
+}
+
 static int st1202_dt_init(struct st1202_chip *chip)
 {
 	struct device *dev = &chip->client->dev;
@@ -301,6 +384,7 @@ static int st1202_dt_init(struct st1202_chip *chip)
 		led->led_cdev.pattern_set = st1202_led_pattern_set;
 		led->led_cdev.pattern_clear = st1202_led_pattern_clear;
 		led->led_cdev.default_trigger = "pattern";
+		led->led_cdev.blink_set = st1202_blink_set;
 		led->led_cdev.brightness_set = st1202_brightness_set;
 		led->led_cdev.brightness_get = st1202_brightness_get;
 	}
-- 
2.55.0

Re: [PATCH v3 1/1] leds: st1202: Add hardware-accelerated blink support

[sashiko-bot]

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

New issues:
- [High] st1202_blink_set() forcibly zeroes out the active PWM pattern slots for all other LEDs, unintentionally destroying the state of independently configured steady-ON LEDs.
- [Medium] st1202_blink_set() ignores the LED's configured brightness and forces the hardware current limit to maximum (U8_MAX).

Pre-existing issues:
- [High] The driver assigns a sleeping function (st1202_brightness_set) to the non-blocking led_cdev.brightness_set callback, leading to kernel panics when called from atomic context.
--

--- Patch [1]: [PATCH v3 1/1] leds: st1202: Add hardware-accelerated blink support ---
commit d1f0e631f570c78c507b9a24936dea1d4c6fa4f3
Author: Manuel Fombuena <fombuena@outlook.com>

leds: st1202: Add hardware-accelerated blink support

This commit implements blink_set() to enable hardware-accelerated blinking
via the timer trigger on the ST1202 LED controller. It configures the
chip's pattern sequencer to alternate between full brightness and off for
the specified durations.

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index 168df5ecf27b7..3600409d4bc25 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
[ ... ]
> @@ -275,6 +276,88 @@ static int st1202_led_pattern_set(struct led_classdev *ldev,
[ ... ]
> +	/* Zero out PWM for all other active channels to prevent them from blinking */
> +	for (int i = 0; i < ST1202_MAX_LEDS; i++) {
> +		if (!chip->leds[i].is_active || i == led->led_num)
> +			continue;
> +		ret = st1202_pwm_pattern_write(chip, i, 0, LED_OFF);

[Severity: High]
Does this unintentionally destroy the state of other independently configured
steady-ON LEDs on the same chip? By forcing the PWM pattern slots to 0 for
all other active LEDs, they will be forced dark when the blink sequencer
starts, which seems to break their independent channel operation.

> +		if (ret)
> +			return ret;
> +		ret = st1202_pwm_pattern_write(chip, i, 1, LED_OFF);
> +		if (ret)
> +			return ret;
> +	}
[ ... ]
> +	ret = st1202_write_reg(chip, ST1202_PATTERN_REP, U8_MAX);
> +	if (ret)
> +		return ret;
> +
> +	ret = st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num, U8_MAX);

[Severity: Medium]
Will this hardcoded maximum current limit (U8_MAX) overwrite the user's
previously configured brightness level? It looks like this ignores the LED's
configured brightness (or led_cdev->blink_brightness) and forces blinking
LEDs to output at 100% hardware intensity.

> +	if (ret)
> +		return ret;
> +
> +	ret = __st1202_channel_set(chip, led->led_num, true);
[ ... ]
> @@ -301,6 +384,7 @@ static int st1202_dt_init(struct st1202_chip *chip)
>  		led->led_cdev.pattern_set = st1202_led_pattern_set;
>  		led->led_cdev.pattern_clear = st1202_led_pattern_clear;
>  		led->led_cdev.default_trigger = "pattern";
> +		led->led_cdev.blink_set = st1202_blink_set;
>  		led->led_cdev.brightness_set = st1202_brightness_set;

[Severity: High]
This is a pre-existing issue, but does assigning st1202_brightness_set() to
the non-blocking led_cdev.brightness_set callback risk a kernel panic?

Looking at st1202_brightness_set(), it takes a sleepable mutex:

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

If an atomic trigger (like a software timer or heartbeat) attempts to update
the LED brightness via this callback, which is strictly documented as
non-sleeping, wouldn't it result in a "scheduling while atomic" panic?

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

Re: [PATCH v3 1/1] leds: st1202: Add hardware-accelerated blink support

[Manuel Fombuena]

On Fri, 2026-07-24 at 12:12 +0000, sashiko-bot@kernel.org wrote:

> New issues:
> - [High] st1202_blink_set() forcibly zeroes out the active PWM pattern
> slots for all other LEDs, unintentionally destroying the state of
> independently configured steady-ON LEDs.

This is an inherent hardware constraint. The LED1202 uses a single
global pattern sequencer with shared timing registers, making it
impossible for two channels to run independent blink configurations
simultaneously. When blink_set() is called, the shared timing is
reconfigured for the requested delays and other channels' PWM slots are
set to LED_OFF so they remain dark rather than outputting unintended
values. This behaviour is described in the commit message.

> - [Medium] st1202_blink_set() ignores the LED's configured brightness
> and forces the hardware current limit to maximum (U8_MAX).

led_cdev->blink_brightness is set inside led_set_software_blink(), which
is the fallback path taken when blink_set() is absent or returns non-
zero. Since st1202_blink_set() returns 0 on success,
led_set_software_blink() is never reached and blink_brightness is not
updated by the core before our callback is invoked. Using it would risk
reading 0 or a stale value from a previous software blink, causing the
LED to blink invisibly. U8_MAX is intentional.

> Pre-existing issues:
> - [High] The driver assigns a sleeping function
> (st1202_brightness_set) to the non-blocking led_cdev.brightness_set
> callback, leading to kernel panics when called from atomic context.

This is a pre-existing issue outside the scope of this patch and will be
addressed in a follow-up submission.

--
Manuel Fombuena

Re: [PATCH v3 1/1] leds: st1202: Add hardware-accelerated blink support

[Lee Jones]

On Fri, 24 Jul 2026, Manuel Fombuena wrote:

> Implement blink_set() to enable hardware-accelerated blinking via the
> timer trigger. The LED1202 pattern engine is used to produce a two-step
> sequence: full brightness for delay_on, off for delay_off, repeating
> indefinitely.
> 
> Requested delays are clamped to the hardware maximum before rounding up
> to the nearest 22ms step, then clamped again to the full hardware range
> [22ms, 5610ms]. The pre-clamp prevents integer overflow in roundup() for
> extreme input values near ULONG_MAX. A zero delay is replaced with the
> default of 500ms independently for each of delay_on and delay_off.
> 
> The LED1202 pattern sequencer is global and its timing registers are
> shared across all channels, so only one blink configuration can be
> active at a time. Other active channels have their PWM slots zeroed for
> both pattern steps so they remain dark rather than outputting unintended
> values when the sequencer runs. The target channel's ILED register is
> set to full brightness and the channel is enabled, since the timer
> trigger deactivates the current trigger before calling blink_set which
> would otherwise leave the channel disabled.
> 
> Signed-off-by: Manuel Fombuena <fombuena@outlook.com>
> ---
>  drivers/leds/leds-st1202.c | 84 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 84 insertions(+)
> 
> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index 168df5ecf27b..3600409d4bc2 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
> @@ -15,6 +15,7 @@
>  #include <linux/slab.h>
>  #include <linux/string.h>
>  
> +#define ST1202_BLINK_DEFAULT_DELAY         500
>  #define ST1202_CHAN_DISABLE_ALL            0x00
>  #define ST1202_CHAN_ENABLE_HIGH            0x03
>  #define ST1202_CHAN_ENABLE_LOW             0x02
> @@ -275,6 +276,88 @@ static int st1202_led_pattern_set(struct led_classdev *ldev,
>  	return 0;
>  }
>  
> +static int st1202_blink_set(struct led_classdev *led_cdev,
> +			unsigned long *delay_on, unsigned long *delay_off)
> +{
> +	struct st1202_led *led = cdev_to_st1202_led(led_cdev);
> +	struct st1202_chip *chip = led->chip;
> +	unsigned long on, off;
> +	int ret;
> +
> +	on = *delay_on ?: ST1202_BLINK_DEFAULT_DELAY;
> +	off = *delay_off ?: ST1202_BLINK_DEFAULT_DELAY;

Could we simplify this by initialising '*delay_on' and '*delay_off' first
using standard 'if' statements, as is common in other 'blink_set'
implementations?

> +
> +	on = min_t(unsigned long, on, ST1202_MILLIS_PATTERN_DUR_MAX);
> +	off = min_t(unsigned long, off, ST1202_MILLIS_PATTERN_DUR_MAX);

Since 'on', 'off', and the maximum duration are all of type 'unsigned long',
should we use the simpler 'min()' macro here instead of 'min_t()'?

> +	on = roundup(on, ST1202_MILLIS_PATTERN_DUR_MIN);
> +	off = roundup(off, ST1202_MILLIS_PATTERN_DUR_MIN);
> +	on = clamp_val(on, ST1202_MILLIS_PATTERN_DUR_MIN, ST1202_MILLIS_PATTERN_DUR_MAX);
> +	off = clamp_val(off, ST1202_MILLIS_PATTERN_DUR_MIN, ST1202_MILLIS_PATTERN_DUR_MAX);

Is the second 'clamp_val()' call redundant here? Since the maximum limit is
already a multiple of the minimum step, any rounded-up value should naturally
fall within the valid range.

> +
> +	guard(mutex)(&chip->lock);
> +
> +	ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_SHFT);
> +	if (ret)
> +		return ret;
> +
> +	/* Zero out PWM for all other active channels to prevent them from blinking */
> +	for (int i = 0; i < ST1202_MAX_LEDS; i++) {
> +		if (!chip->leds[i].is_active || i == led->led_num)
> +			continue;
> +		ret = st1202_pwm_pattern_write(chip, i, 0, LED_OFF);
> +		if (ret)
> +			return ret;
> +		ret = st1202_pwm_pattern_write(chip, i, 1, LED_OFF);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = st1202_pwm_pattern_write(chip, led->led_num, 0, ST1202_PATTERN_PWM_FULL);
> +	if (ret)
> +		return ret;
> +	ret = st1202_pwm_pattern_write(chip, led->led_num, 1, LED_OFF);
> +	if (ret)
> +		return ret;
> +
> +	ret = st1202_write_reg(chip, ST1202_PATTERN_DUR,
> +				st1202_prescalar_to_miliseconds(on));

We know that neither of these words are spelt correctly, right?

> +	if (ret)
> +		return ret;
> +	ret = st1202_write_reg(chip, ST1202_PATTERN_DUR + 1,
> +				st1202_prescalar_to_miliseconds(off));
> +	if (ret)
> +		return ret;
> +
> +	for (int patt = 2; patt < ST1202_MAX_PATTERNS; patt++) {

pattern

> +		ret = st1202_write_reg(chip, ST1202_PATTERN_DUR + patt, 0);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = st1202_write_reg(chip, ST1202_PATTERN_REP, U8_MAX);
> +	if (ret)
> +		return ret;
> +
> +	ret = st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num, U8_MAX);
> +	if (ret)
> +		return ret;
> +
> +	ret = __st1202_channel_set(chip, led->led_num, true);
> +	if (ret)
> +		return ret;
> +
> +	ret = st1202_write_reg(chip, ST1202_CONFIG_REG,
> +				ST1202_CONFIG_REG_PATSR | ST1202_CONFIG_REG_PATS |
> +				ST1202_CONFIG_REG_SHFT);
> +	if (ret)
> +		return ret;
> +
> +	*delay_on = on;
> +	*delay_off = off;
> +
> +	return 0;
> +}
> +
>  static int st1202_dt_init(struct st1202_chip *chip)
>  {
>  	struct device *dev = &chip->client->dev;
> @@ -301,6 +384,7 @@ static int st1202_dt_init(struct st1202_chip *chip)
>  		led->led_cdev.pattern_set = st1202_led_pattern_set;
>  		led->led_cdev.pattern_clear = st1202_led_pattern_clear;
>  		led->led_cdev.default_trigger = "pattern";
> +		led->led_cdev.blink_set = st1202_blink_set;
>  		led->led_cdev.brightness_set = st1202_brightness_set;
>  		led->led_cdev.brightness_get = st1202_brightness_get;
>  	}
> -- 
> 2.55.0
> 

-- 
Lee Jones

Re: [PATCH v3 1/1] leds: st1202: Add hardware-accelerated blink support

[Manuel Fombuena]

On Thu, 2026-08-06 at 14:45 +0100, Lee Jones wrote:
> On Fri, 24 Jul 2026, Manuel Fombuena wrote:
>
> > +	on = *delay_on ?: ST1202_BLINK_DEFAULT_DELAY;
> > +	off = *delay_off ?: ST1202_BLINK_DEFAULT_DELAY;
>
> Could we simplify this by initialising '*delay_on' and '*delay_off'
> first using standard 'if' statements, as is common in other
> 'blink_set' implementations?

Fixed in v6 using standard if statements.

> > +	on = min_t(unsigned long, on, ST1202_MILLIS_PATTERN_DUR_MAX);
> > +	off = min_t(unsigned long, off, ST1202_MILLIS_PATTERN_DUR_MAX);
>
> Since 'on', 'off', and the maximum duration are all of type
> 'unsigned long', should we use the simpler 'min()' macro here
> instead of 'min_t()'?

No longer applicable. v4 restructured the clamping and rounding,
removing min_t() entirely. v5 and v6 use clamp_val() before roundup().

> > +	on = clamp_val(on, ST1202_MILLIS_PATTERN_DUR_MIN,
ST1202_MILLIS_PATTERN_DUR_MAX);
> > +	off = clamp_val(off, ST1202_MILLIS_PATTERN_DUR_MIN,
ST1202_MILLIS_PATTERN_DUR_MAX);
>
> Is the second 'clamp_val()' call redundant here?

Correct, and it was removed in v4 for the same reason. ST1202_MILLIS_
PATTERN_DUR_MAX is an exact multiple of ST1202_MILLIS_PATTERN_DUR_MIN,
so roundup() on a clamped value cannot exceed the maximum.

> > +	ret = st1202_write_reg(chip, ST1202_PATTERN_DUR,
> > +				st1202_prescalar_to_miliseconds(on));
>
> We know that neither of these words are spelt correctly, right?

Yes. Both are pre-existing in the driver and tracked for a follow-up
submission. In v5 and v6 the misspelled name no longer appears directly
in the new code. The calls go through st1202_duration_pattern_write().

> > +	for (int patt = 2; patt < ST1202_MAX_PATTERNS; patt++) {
>
> pattern

Fixed in v6.

--
Manuel Fombuena