Re: [PATCH v8 6/6] leds: is31fl32xx: Move pwm frequency setting to init_regs()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Registering LED class devices before hardware initialization completes exposes the LEDs prematurely, causing asynchronous initialization (like default triggers) and userspace accesses to race with and be wiped out by the subsequent hardware reset.
- [High] Incorrect use of `max()` instead of `min()` in `is31fl3293_reset` leads to requested current limits being inverted and incorrectly truncated, potentially causing hardware damage to LEDs with lower maximum current ratings.
--

commit b76116518cb056910437127676df1546ae03f030
Author: Jun Yan <jerrysteve1101@gmail.com>

leds: is31fl32xx: Move pwm frequency setting to init_regs()

This commit moves the PWM frequency setting logic from is31fl32xx_parse_dt()
to is31fl32xx_init_regs() to prevent the frequency configuration from being
overwritten by the register reset operation. It separates device tree parsing
from hardware initialization.

> commit a18983b95a61 ("leds: is31f132xx: Add support for is31fl3293")
> swapped the order of is31fl32xx_parse_dt() and is31fl32xx_init_regs().

[Severity: High]
This isn't a bug introduced by this patch, but registering LED class devices
before hardware initialization completes exposes the LEDs prematurely.

In is31fl32xx_probe(), is31fl32xx_parse_dt() is called, which internally
executes devm_led_classdev_register_ext() for each LED:

drivers/leds/leds-is31fl32xx.c:is31fl32xx_probe() {
	...
	ret = is31fl32xx_parse_dt(dev, priv);
	if (ret)
		return ret;

	ret = is31fl32xx_init_regs(priv);
	...
}

This registers the LEDs to the subsystem, potentially immediately triggering
led_set_brightness_nopm() via default state settings (e.g., "default-on").
This schedules an asynchronous work item to configure the LED over I2C.

However, is31fl32xx_probe() then immediately calls is31fl32xx_init_regs(),
which performs a full hardware reset (is31fl32xx_reset_regs()).

If the hardware reset races with or occurs after the default state triggers,
will the initial LED states be wiped out?  Must the hardware be fully
initialized and reset before registering the LED devices?

> diff --git a/drivers/leds/leds-is31fl32xx.c b/drivers/leds/leds-is31fl32xx.c
> index 0c811aebcbcfe..4039b6336bcd4 100644
> --- a/drivers/leds/leds-is31fl32xx.c
> +++ b/drivers/leds/leds-is31fl32xx.c

[ ... ]

[Severity: High]
This is a pre-existing issue, but there is an incorrect use of max()
instead of min() in is31fl3293_reset():

drivers/leds/leds-is31fl32xx.c:is31fl3293_reset() {
	...
	for (i = 0; i < priv->num_leds; i++) {
		struct is31fl32xx_led_data *led_data = &priv->leds[i];
		int current_level_reg = IS31FL3293_CL_REG + led_data->channel - 1;
		int microamp = max(led_data->max_microamp, IS31FL3293_MAX_MICROAMP);
		int current_level = (microamp * 0xff) / IS31FL3293_MAX_MICROAMP;

		ret = is31fl32xx_write(priv, current_level_reg, current_level);
	...
}

This bounds the user-specified current limit against the hardware maximum of
20,000 µA (IS31FL3293_MAX_MICROAMP) using max(), forcing the current to
always be at least the max chip rating.

If an LED is only rated for 5mA, does this force it to 20mA, which could
physically damage it?

Furthermore, if a user requests a value larger than 20mA (e.g., 30,000 µA),
max() allows it.  The subsequent calculation exceeds 255. Because
is31fl32xx_write() takes a u8 argument, this value is truncated, resulting in
unexpectedly lower current.

Should the code use min() to properly enforce the hardware's maximum rating?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260607152002.446617-1-jerrysteve1101@gmail.com?part=6