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

[PATCH 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.

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

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

-- 
2.55.0

[PATCH 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 range [22ms, 5610ms] and
rounded up to the nearest 22ms step. 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>
Assisted-by: Claude:claude-sonnet-4-6
---
 drivers/leds/leds-st1202.c | 82 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 82 insertions(+)

diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
index 168df5ecf27b..fc784a854a33 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,86 @@ 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 ? *delay_on : ST1202_BLINK_DEFAULT_DELAY;
+	off = *delay_off ? *delay_off : ST1202_BLINK_DEFAULT_DELAY;
+
+	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);
+	on = roundup(on, ST1202_MILLIS_PATTERN_DUR_MIN);
+	off = roundup(off, ST1202_MILLIS_PATTERN_DUR_MIN);
+
+	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 +382,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 1/1] leds: st1202: add hardware-accelerated blink support

[Lee Jones]

On Mon, 13 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 range [22ms, 5610ms] and
> rounded up to the nearest 22ms step. 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>
> Assisted-by: Claude:claude-sonnet-4-6

Should we avoid using non-standard metadata tags such as 'Assisted-by' in the
commit message to adhere to standard upstream practices?

> ---
>  drivers/leds/leds-st1202.c | 82 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 82 insertions(+)
> 
> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index 168df5ecf27b..fc784a854a33 100644
> --- .../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,86 @@ 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 ? *delay_on : ST1202_BLINK_DEFAULT_DELAY;
> +	off = *delay_off ? *delay_off : ST1202_BLINK_DEFAULT_DELAY;

Use the short form here:

on = *delay_on: ST1202_BLINK_DEFAULT_DELAY;

> +	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);
> +	on = roundup(on, ST1202_MILLIS_PATTERN_DUR_MIN);
> +	off = roundup(off, ST1202_MILLIS_PATTERN_DUR_MIN);

Should we perform the 'roundup' before 'clamp_val' to ensure that rounding the
value up does not push it beyond 'ST1202_MILLIS_PATTERN_DUR_MAX'?

> +
> +	guard(mutex)(&chip->lock);
> +
> +	ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_SHFT);

SHFT is weird - why save that very short char and harm readability?

> +	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++) {

Does zeroing out the pattern PWM slots for other active channels permanently
overwrite their configurations or is there a mechanism to restore their state
once blinking is disabled?

> +		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));

Should this function be named 'st1202_milliseconds_to_prescaler' instead,
since we are converting a millisecond value into a register value? Also,
could we correct the spelling of 'prescaler' and 'milliseconds' to ensure the
code is clean and passes spell checks?

> +	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 +382,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 1/1] leds: st1202: add hardware-accelerated blink support

[Manuel Fombuena]

On Thu, 23 Jul 2026, Lee Jones wrote:

> > Signed-off-by: Manuel Fombuena <fombuena@outlook.com>
> > Assisted-by: Claude:claude-sonnet-4-6
>
> Should we avoid using non-standard metadata tags such as 'Assisted-by'
> in the commit message to adhere to standard upstream practices?

I'm going with the guidance found in
https://docs.kernel.org/process/coding-assistants.html which at present
still states it should be included. I read somewhere that there were
active discussions about this guideline and that the tag might be dropped
in the future, but I ignore the outcome. I will drop it if that is the
preference.

> > +	on = *delay_on ? *delay_on : ST1202_BLINK_DEFAULT_DELAY;
> > +	off = *delay_off ? *delay_off : ST1202_BLINK_DEFAULT_DELAY;
>
> Use the short form here:
>
> on = *delay_on: ST1202_BLINK_DEFAULT_DELAY;

Noted, will fix in v2.

> > +	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);
> > +	on = roundup(on, ST1202_MILLIS_PATTERN_DUR_MIN);
> > +	off = roundup(off, ST1202_MILLIS_PATTERN_DUR_MIN);
>
> Should we perform the 'roundup' before 'clamp_val' to ensure that
> rounding the value up does not push it beyond
> 'ST1202_MILLIS_PATTERN_DUR_MAX'?

Noted, will fix in v2.

> > +	ret = st1202_write_reg(chip, ST1202_CONFIG_REG, ST1202_CONFIG_REG_SHFT);
>
> SHFT is weird - why save that very short char and harm readability?

ST1202_CONFIG_REG_SHFT is a pre-existing constant from the driver. I
intend to keep this submission focused on implementing blink support.
I have this issue and all the others identified during the previous
fix-focused submission registered for a subsequent submission.

> > +	/* Zero out PWM for all other active channels to prevent them from blinking */
> > +	for (int i = 0; i < ST1202_MAX_LEDS; i++) {
>
> Does zeroing out the pattern PWM slots for other active channels
> permanently overwrite their configurations or is there a mechanism to
> restore their state once blinking is disabled?

Yes, it permanently overwrites them. The LED1202 has a single global
pattern sequencer with no snapshot-and-restore capability at the
hardware level. When blink is activated on one channel the sequencer is
reconfigured globally; other channels' PWM slots are intentionally set
to LED_OFF so they remain dark rather than blinking at the new timing.
This is described in the commit message.

> > +	ret = st1202_write_reg(chip, ST1202_PATTERN_DUR,
> > +				st1202_prescalar_to_miliseconds(on));
>
> Should this function be named 'st1202_milliseconds_to_prescaler'
> instead, since we are converting a millisecond value into a register
> value? Also, could we correct the spelling of 'prescaler' and
> 'milliseconds' to ensure the code is clean and passes spell checks?

Agreed, but as with the above, this is pre-existing and registered for
a subsequent submission.

--
Manuel Fombuena

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

[Manuel Fombuena]

On Thu, 2026-07-23 at 23:17 +0100, Manuel Fombuena wrote:
> On Thu, 23 Jul 2026, Lee Jones wrote:
> 
> > > +	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);
> > > +	on = roundup(on, ST1202_MILLIS_PATTERN_DUR_MIN);
> > > +	off = roundup(off, ST1202_MILLIS_PATTERN_DUR_MIN);
> > 
> > Should we perform the 'roundup' before 'clamp_val' to ensure that
> > rounding the value up does not push it beyond
> > 'ST1202_MILLIS_PATTERN_DUR_MAX'?

This went in a full circle. v2 swapped to roundup before clamp_val,
but then an automated review flagged integer overflow risk for extreme
inputs near ULONG_MAX, since roundup() performs an addition internally.
v3 addressed that by prepending a min_t() cap, but introduced a
redundant trailing clamp_val() that could never fire.

It turns out clamp_val() before roundup() addresses both concerns:
clamping first eliminates the overflow risk, and since
ST1202_MILLIS_PATTERN_DUR_MAX (5610) is an exact multiple of
ST1202_MILLIS_PATTERN_DUR_MIN (22), roundup() on a clamped value
cannot exceed MAX. v4 reverts to the original order with this
explanation in the commit message.
 
--
Manuel Fombuena