[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

[sashiko-bot]

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

New issues:
- [High] st1202_blink_set() destructively zeroes the PWM pattern registers for all other active LED channels, forcing them dark and breaking their steady states.
- [Medium] Blink brightness is hardcoded to maximum (`U8_MAX`), ignoring user-configured levels.
- [Low] The commit message description is not capitalized, violating LED subsystem naming conventions.

Pre-existing issues:
- [High] The sleepable st1202_brightness_set() function is incorrectly assigned to the non-blocking brightness_set callback.
--

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

leds: st1202: add hardware-accelerated blink support

This patch implements blink_set() to enable hardware-accelerated blinking via
the timer trigger. It uses the LED1202 pattern engine to sequence full
brightness and off periods, sharing timing registers globally.

> leds: st1202: add hardware-accelerated blink support

[Severity: Low]
This isn't a bug, but the subsystem guidelines request capitalizing the
description after the subsystem prefix. Should this be "Add
hardware-accelerated blink support"?

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index 168df5ecf27b7..fc784a854a33b 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
[ ... ]
> +static int st1202_blink_set(struct led_classdev *led_cdev,
> +			unsigned long *delay_on, unsigned long *delay_off)
> +{
[ ... ]
> +	/* 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;
> +	}

[Severity: High]
Does this code force all other active LEDs on the same chip to go dark and
lose their configured states?

When hardware blinking is enabled on one LED, writing LED_OFF to the pattern 0
and 1 PWM registers for other active LEDs seems like it would permanently clear
their steady states (for instance, if they were previously configured to be
constantly ON).

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

[Severity: High]
Does clearing the global durations for patterns 2 through 7 here break custom
patterns that might be running on any other LEDs?

> +
> +	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]
Is it intentional to hardcode the blink brightness to maximum (U8_MAX)?

When the timer trigger starts a blink, it sets led_cdev->blink_brightness to
the user's desired intensity. Should this register write use that configured
level instead of always causing the LED to blink at full intensity?

[ ... ]
> @@ -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;

[Severity: High]
This is a pre-existing issue, but should st1202_brightness_set() be assigned
to brightness_set?

The st1202_brightness_set() function acquires a mutex and performs I2C writes
which can sleep:

st1202_brightness_set() {
    guard(mutex)(&chip->lock);
    ...
}

The LED core uses the brightness_set callback in atomic contexts like software
timer interrupts. Should this driver only implement and assign
brightness_set_blocking to prevent scheduling while atomic bugs?

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

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

[Manuel Fombuena]

> New issues:
> - [High] st1202_blink_set() destructively zeroes the PWM pattern 
registers for all other active LED channels, forcing them dark and 
breaking their steady states.

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] Blink brightness is hardcoded to maximum (`U8_MAX`), ignoring 
user-configured levels.

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.

> - [Low] The commit message description is not capitalized, violating LED 
subsystem naming conventions.

Noted, it would be fixed in v2 if it was necessary to send it because of
code changes.
 
> Pre-existing issues:
> - [High] The sleepable st1202_brightness_set() function is incorrectly 
assigned to the non-blocking brightness_set callback.

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

--
Manuel Fombuena