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

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

  In response to maintainer (Lee Jones) review on v6:

  Rename loop variable 'i' to 'chan'.
  Add blank lines between logical blocks within blink_set().
  Revert loop variable 'pattern' to 'patt' for consistency with the
  rest of the driver. A driver-wide rename will follow in a separate
  submission.

--- Changes in v6 ---

  In response to maintainer (Lee Jones) review on v3:

  Use standard if statements for zero-delay substitution instead of the
  short-form ternary introduced in v2.
  Rename loop variable 'patt' to 'pattern'.

--- Changes in v5 ---

  Use st1202_duration_pattern_write() for pattern duration slots 0 and
  1 in blink_set(), consistent with how st1202_led_pattern_set() uses
  the same helper for the same registers.

  Fix commit message: ST1202_MILLIS_PATTERN_DUR_MAX was wrapped across
  two lines.

--- Changes in v4 ---

  Fix delay clamping and rounding order following a review of the
  changes introduced across v2 and v3:

  v2 moved roundup() before clamp_val() so that rounding could not push
  the result past the hardware maximum. However, this left roundup()
  exposed to integer overflow for extreme inputs near ULONG_MAX, since
  roundup(x, 22) internally computes x + 21 before dividing.

  v3 addressed the overflow by prepending a min_t() cap at MAX before
  roundup(), and retained a trailing clamp_val() for the full range.
  The trailing clamp_val() turned out to be unreachable:
  ST1202_MILLIS_PATTERN_DUR_MAX (5610) is an exact multiple of
  ST1202_MILLIS_PATTERN_DUR_MIN (22), so roundup() on a value already
  capped at 5610 returns 5610 unchanged, and the result is always
  within range before the trailing clamp_val() runs.

  v4 reverts to clamp_val() before roundup(). Clamping first prevents
  overflow for extreme inputs, and since MAX is an exact multiple of MIN
  the rounded result cannot exceed MAX, addressing the v1 concern
  without the redundant operations introduced in v3.

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

  Other pre-existing issues identified by the automated review are
  outside the scope of this patch and will be addressed in a follow-up
  submission.

--- Changes in v2 ---

  In response to maintainer (Lee Jones) review on v1:

  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/
v3: https://lore.kernel.org/all/GV1PR08MB84979E014A407174EBAE9875C5CF2@GV1PR08MB8497.eurprd08.prod.outlook.com/
v4: https://lore.kernel.org/all/GV1PR08MB8497E3103AD32162BD05D907C5CB2@GV1PR08MB8497.eurprd08.prod.outlook.com/
v5: https://lore.kernel.org/all/GV1PR08MB8497E2498ED9D66AF60A3ACFC5D42@GV1PR08MB8497.eurprd08.prod.outlook.com/
v6: https://lore.kernel.org/all/GV1PR08MB84977DA9A154D7186B6EA2CBC5D22@GV1PR08MB8497.eurprd08.prod.outlook.com/

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

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

-- 
2.55.0

[PATCH v7 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] then
rounded up to the nearest 22ms step. Clamping before rounding prevents
integer overflow in roundup() for extreme input values; since
ST1202_MILLIS_PATTERN_DUR_MAX is an exact multiple of
ST1202_MILLIS_PATTERN_DUR_MIN, rounding a clamped value cannot exceed
the maximum. 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 | 89 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 89 insertions(+)

diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
index 168df5ecf27b..ead144155849 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,93 @@ 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;
+
+	if (!*delay_on)
+		*delay_on = ST1202_BLINK_DEFAULT_DELAY;
+	if (!*delay_off)
+		*delay_off = ST1202_BLINK_DEFAULT_DELAY;
+
+	on = *delay_on;
+	off = *delay_off;
+
+	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 chan = 0; chan < ST1202_MAX_LEDS; chan++) {
+		if (!chip->leds[chan].is_active || chan == led->led_num)
+			continue;
+
+		ret = st1202_pwm_pattern_write(chip, chan, 0, LED_OFF);
+		if (ret)
+			return ret;
+
+		ret = st1202_pwm_pattern_write(chip, chan, 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_duration_pattern_write(chip, 0, on);
+	if (ret)
+		return ret;
+
+	ret = st1202_duration_pattern_write(chip, 1, off);
+	if (ret)
+		return ret;
+
+	for (int pattern = 2; pattern < ST1202_MAX_PATTERNS; pattern++) {
+		ret = st1202_write_reg(chip, ST1202_PATTERN_DUR + pattern, 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 +389,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 v7 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 alters the hardware state of sibling LED channels by zeroing out their PWM configurations, permanently turning off solid-ON LEDs and desynchronizing their physical state from the LED subsystem.
- [High] Hardware blink implementation incorrectly intercepts 0-delay values, breaking timer trigger off/on states.
- [Medium] st1202_blink_set hardcodes maximum brightness, completely ignoring the user's requested blink brightness.

Pre-existing issues:
- [High] st1202_brightness_set is incorrectly assigned to the non-blocking brightness_set callback, violating the atomic context requirement.
--

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

leds: st1202: Add hardware-accelerated blink support

This commit implements blink_set() to enable hardware-accelerated blinking
using the LED1202 pattern engine. Requested delays are clamped to hardware
ranges, and active channels have their PWM slots zeroed to prevent unintended
blinking when the sequencer runs.

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index 168df5ecf27b7..ead1441558497 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)
> +{
> +	struct st1202_led *led = cdev_to_st1202_led(led_cdev);
> +	struct st1202_chip *chip = led->chip;
> +	unsigned long on, off;
> +	int ret;
> +
> +	if (!*delay_on)
> +		*delay_on = ST1202_BLINK_DEFAULT_DELAY;
> +	if (!*delay_off)
> +		*delay_off = ST1202_BLINK_DEFAULT_DELAY;

[Severity: High]
By silently overriding a requested zero delay to 500ms and returning success,
does this break the core's fallback contract?

When a user requests a steady state via the timer trigger, the core expects
the driver to return -EINVAL if it cannot natively support the zero-delay
condition. This expectation allows a software fallback via
led_set_software_blink().

> +
> +	on = *delay_on;

[ ... ]

> +	/* Zero out PWM for all other active channels to prevent them from blinking */
> +	for (int chan = 0; chan < ST1202_MAX_LEDS; chan++) {
> +		if (!chip->leds[chan].is_active || chan == led->led_num)
> +			continue;
> +
> +		ret = st1202_pwm_pattern_write(chip, chan, 0, LED_OFF);
> +		if (ret)
> +			return ret;
> +
> +		ret = st1202_pwm_pattern_write(chip, chan, 1, LED_OFF);
> +		if (ret)
> +			return ret;
> +	}

[Severity: High]
Does this loop permanently turn off solid-ON sibling LEDs and desynchronize
their physical state from the LED subsystem?

The original PWM states of these independent channels are destructed here
without being tracked or restored later.

> +
> +	ret = st1202_pwm_pattern_write(chip, led->led_num, 0, ST1202_PATTERN_PWM_FULL);

[ ... ]

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

[Severity: Medium]
Does this unconditionally hardcode the blink setting to maximum intensity?

This appears to completely ignore the user-configured led_cdev->blink_brightness
or fallback led_cdev->brightness states.

> +
> +	ret = __st1202_channel_set(chip, led->led_num, true);

[ ... ]

> @@ -301,6 +389,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 can this cause 'scheduling while atomic'
panics?

By assigning st1202_brightness_set() to the non-blocking brightness_set
callback, the LED core will invoke it directly from softirq/timer contexts.

Since st1202_brightness_set() performs sleeping I2C transfers and
immediately calls:

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

is this safe to run in an atomic context, or should this be mapped to
brightness_set_blocking instead?

>  		led->led_cdev.brightness_get = st1202_brightness_get;
>  	}

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

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

[Manuel Fombuena]

On Fri, 2026-08-14 at 09:30 +0000, sashiko-bot@kernel.org wrote:
> 
> New issues:
> - [High] st1202_blink_set alters the hardware state of sibling LED
> channels by zeroing out their PWM configurations, permanently turning
> off solid-ON LEDs and desynchronizing their physical state from the
> LED subsystem.

The zero-delay handling is not a bug. Converting a zero delay
to a default value is the established convention for blink_set()
implementations across the LED subsystem. The timer trigger does not
use zero delays to signal steady-on or steady-off states; those are
achieved through different triggers or by setting brightness directly.
The if-statement form used here was introduced in v6 at the maintainer's
request precisely because it matches how other blink_set callbacks in
the kernel handle this case.

> - [High] Hardware blink implementation incorrectly intercepts 0-delay
> values, breaking timer trigger off/on 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] st1202_blink_set hardcodes maximum brightness, completely
> ignoring the user's requested blink brightness.

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.


-- 
Manuel Fombuena