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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] LED class devices are registered and exposed to userspace before the hardware is fully initialized, allowing initial states to be overwritten by the subsequent hardware reset.
- [High] The driver tears down the hardware manually in `remove()` while the LED class devices are still accessible to userspace.
- [High] Incorrect logic when calculating maximum current limits for IS31FL3293 leads to broken limit enforcement and truncation.
--

commit 4a46eb293f0e15dbc00b614e110870654f6c33ca
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(). This separates device tree parsing from hardware
initialization and prevents the configuration from being overwritten.

Fixes: a18983b95a61 ("leds: is31f132xx: Add support for is31fl3293")

> diff --git a/drivers/leds/leds-is31fl32xx.c b/drivers/leds/leds-is31fl32xx.c
> index 2d7ff4c5a08c7..c545a3b7a05a1 100644
> --- a/drivers/leds/leds-is31fl32xx.c
> +++ b/drivers/leds/leds-is31fl32xx.c
[ ... ]
> @@ -420,7 +429,6 @@ static struct is31fl32xx_led_data *is31fl32xx_find_led_data(
>  static int is31fl32xx_parse_dt(struct device *dev,
>  			       struct is31fl32xx_priv *priv)
>  {
> -	const struct is31fl32xx_chipdef *cdef = priv->cdef;
>  	int ret = 0;
>  
>  	/* Driving this GPIO line low takes the chip out of shutdown,
> @@ -431,17 +439,7 @@ static int is31fl32xx_parse_dt(struct device *dev,
>  		return dev_err_probe(dev, PTR_ERR(priv->powerdown_gpio),
>  				"Failed to get powerdown gpio\n");
>  
> -	if ((cdef->output_frequency_setting_reg != IS31FL32XX_REG_NONE) &&
> -	    of_property_read_bool(dev_of_node(dev), "issi,22khz-pwm")) {
> -
> -		ret = is31fl32xx_write(priv, cdef->output_frequency_setting_reg,
> -				       IS31FL32XX_PWM_FREQUENCY_22KHZ);
> -
> -		if (ret) {
> -			dev_err(dev, "Failed to write output PWM frequency register\n");
> -			return ret;
> -		}
> -	}
> +	priv->pwm_22khz = of_property_read_bool(dev_of_node(dev), "issi,22khz-pwm");
>  
>  	for_each_available_child_of_node_scoped(dev_of_node(dev), child) {
>  		struct led_init_data init_data = {};

[Severity: High]
This isn't a regression introduced by this patch, but since this commit
relates to initialization order, does registering the LEDs here before
hardware is fully initialized cause a regression?

Looking at the driver sequence, is31fl32xx_parse_dt() registers each LED by
calling devm_led_classdev_register_ext(), exposing them to userspace early.
The LED subsystem then evaluates properties like default-state and triggers,
performing asynchronous I2C writes to set initial brightness.

However, is31fl32xx_init_regs() runs afterward and performs a full register
reset via is31fl32xx_reset_regs().

Could this sequence allow boot-time configurations and early userspace
settings to be lost when the hardware is unconditionally reset?

[Severity: High]
This is also a pre-existing regression, but I noticed the driver relies on
devm_led_classdev_register_ext() for unregistration. Does this create a
race condition during device teardown?

The driver manually resets and powers down the hardware in
is31fl32xx_remove(). Because devres cleanup executes after remove()
completes, the LED devices seem to remain fully accessible to userspace
for a short window.

If a brightness change is requested during this window, would I2C writes be
dispatched to an uninitialized or powered-down chip, potentially causing
bus timeouts or stalls?

While reviewing the hardware initialization logic, I noticed another issue
in the maximum current calculations:

[Severity: High]
This is a pre-existing regression, but does the maximum current calculation
in is31fl3293_reset() correctly enforce the limits?

It calculates the limit using max(led_data->max_microamp,
IS31FL3293_MAX_MICROAMP). If the device tree specifies a limit below the
20mA chip maximum (e.g., 10mA), max() overrides it with 20mA, which defeats
the protection. If a value above 20mA is specified, max() retains the larger
value, causing the resulting current_level calculation to exceed 255.

Since is31fl32xx_write() takes a u8 for the value, this current_level would
be silently truncated (e.g., 382 becomes 126), resulting in an incorrectly
low limit.

Should the driver use min() instead of max() to properly enforce the limit,
and perhaps supply a fallback when the property is 0?

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