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

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

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 v4 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. ST1202_MILLIS_
PATTERN_DUR_MAX is an exact multiple of ST1202_MILLIS_PATTERN_DUR_MIN,
so rounding cannot push the result beyond 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 | 82 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 82 insertions(+)

diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
index 168df5ecf27b..4e0128c7c94d 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 ?: ST1202_BLINK_DEFAULT_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 v4 1/1] leds: st1202: Add hardware-accelerated blink support

[sashiko-bot]

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

New issues:
- [High] State corruption of other active LEDs during blink configuration
- [Medium] Hardcoded maximum brightness in hardware blink setup
- [Medium] Early return on I2C failure leaves hardware in an inconsistent, broken state

Pre-existing issues:
- [High] Sleeping function assigned to non-blocking `brightness_set` callback
- [High] Use-After-Free due to missing reference count on cached `fwnode`
--

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

leds: st1202: Add hardware-accelerated blink support

This patch implements blink_set() for the st1202 LED driver to enable
hardware-accelerated blinking using the timer trigger. It utilizes the chip's
pattern engine to create the blink sequence and clamps user-requested delays
to the supported hardware ranges.

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

[Severity: High]
Is this description accurate? Looking at st1202_brightness_set(), it appears
steady-ON LEDs are configured with ST1202_PATTERN_PWM_FULL across their
pattern slots, meaning they naturally stay solid-ON when the global sequencer
is running.

Could zeroing out the PWM slots here inadvertently corrupt the hardware
state of other active LEDs, forcing them permanently off until user-space
reconfigures them?

> diff --git a/drivers/leds/leds-st1202.c b/drivers/leds/leds-st1202.c
> index 168df5ecf27b7..4e0128c7c94d4 100644
> --- a/drivers/leds/leds-st1202.c
> +++ b/drivers/leds/leds-st1202.c
[ ... ]
> @@ -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)
> +{
[ ... ]
> +	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;

[Severity: Medium]
If one of these I2C writes fails, we return early and release the mutex. Since
the sequencer was stopped by the initial ST1202_CONFIG_REG write and PWM
slots for other LEDs might now be partially clobbered, would this error path
leave the chip in a permanently inconsistent state where all active LEDs stop
functioning?

> +	}
> +
> +	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_REP, U8_MAX);
> +	if (ret)
> +		return ret;
> +
> +	ret = st1202_write_reg(chip, ST1202_ILED_REG0 + led->led_num, U8_MAX);

[Severity: Medium]
Will writing U8_MAX to the analog brightness register cause hardware blinking
to always occur at maximum brightness, completely overriding the user's
previously configured brightness level?

> +	if (ret)
> +		return ret;
> +
> +	ret = __st1202_channel_set(chip, led->led_num, true);
[ ... ]
> @@ -301,6 +382,7 @@ static int st1202_dt_init(struct st1202_chip *chip)

[Severity: High]
This isn't a bug introduced by this patch, but looking at st1202_dt_init()
earlier in this function:

    led = &chip->leds[reg];
    led->is_active = true;
    led->fwnode = of_fwnode_handle(child);

Might caching the fwnode pointer here result in a use-after-free later on?
The for_each_available_child_of_node_scoped() iterator automatically drops the
device tree node reference at the end of each iteration. If the node is
dynamically removed or memory is freed before devm_led_classdev_register_ext()
uses it, this stale pointer could be accessed.

>  		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 is there a risk of a "sleeping in atomic
context" panic here?

The LED subsystem requires brightness_set to be strictly non-blocking since
it can be called from software timers or other atomic contexts. Looking at
st1202_brightness_set(), it acquires a mutex and performs I2C transfers which
can sleep.

Should this driver rely exclusively on brightness_set_blocking instead?

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

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