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

[Lee Jones]

/* Sashiko Automation: Reviewed (2 Findings) */

And this one please.

On Sun, 07 Jun 2026, Jun Yan wrote:

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


> 
> This causes the PWM frequency configuration programmed in
> is31fl32xx_parse_dt() to be overwritten by the register reset
> operation performed in is31fl32xx_init_regs().
> 
> Move the PWM frequency setting logic from is31fl32xx_parse_dt() to
> is31fl32xx_init_regs() and separates device tree parsing from hardware
> initialization.
> 
> Fixes: a18983b95a61 ("leds: is31f132xx: Add support for is31fl3293")
> Signed-off-by: Jun Yan <jerrysteve1101@xxxxxxxxx>
> ---
>  drivers/leds/leds-is31fl32xx.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/leds/leds-is31fl32xx.c b/drivers/leds/leds-is31fl32xx.c
> index cbda7edbfb48..b43879bffb06 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?

> @@ -63,6 +63,7 @@ struct is31fl32xx_priv {
>  	struct i2c_client *client;
>  	struct gpio_desc *powerdown_gpio;
>  	unsigned int num_leds;
> +	bool pwm_22khz;
>  	struct is31fl32xx_led_data leds[];
>  };
>  
> @@ -346,6 +347,13 @@ static int is31fl32xx_init_regs(struct is31fl32xx_priv *priv)
>  	if (ret)
>  		return ret;
>  
> +	if ((cdef->output_frequency_setting_reg != IS31FL32XX_REG_NONE) && priv->pwm_22khz) {
> +		ret = is31fl32xx_write(priv, cdef->output_frequency_setting_reg,
> +				       IS31FL32XX_PWM_FREQUENCY_22KHZ);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	/*
>  	 * Set enable bit for all channels.
>  	 * We will control state with PWM registers alone.
> @@ -420,7 +428,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;
>  
>  	/*
> @@ -432,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 = {};
> -- 
> 2.54.0
> 

-- 
Lee Jones