Re: [PATCH v7 5/6] leds: is31fl32xx: Fix missing brightness_steps for is31fl3236

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incorrect current limit enforcement for IS31FL3293 overrides lower user limits and causes integer truncation for high values, risking hardware damage.
- [High] The driver exposes LEDs to userspace and the kernel before fully initializing and resetting the hardware.
--

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

leds: is31fl32xx: Fix missing brightness_steps for is31fl3236

This commit adds the missing brightness_steps field for the is31fl3236
and is31fl3236a chip definitions. This ensures brightness control is
configured properly for these chips.

> diff --git a/drivers/leds/leds-is31fl32xx.c b/drivers/leds/leds-is31fl32xx.c
> index 5b9ed5a1818fa..2d7ff4c5a08c7 100644
> --- a/drivers/leds/leds-is31fl32xx.c
> +++ b/drivers/leds/leds-is31fl32xx.c
> @@ -491,6 +491,7 @@ static const struct is31fl32xx_chipdef is31fl3236_cdef = {
>  	.pwm_register_base			= 0x01,
>  	.led_control_register_base		= 0x26,
>  	.enable_bits_per_led_control_register	= 1,
> +	.brightness_steps			= 256,
>  };
>  
>  static const struct is31fl32xx_chipdef is31fl3236a_cdef = {

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential problem
in how we handle the current limits for the is31fl3293 chip. 

Looking at is31fl3293_reset() in drivers/leds/leds-is31fl32xx.c:

	for (i = 0; i < priv->num_leds; i++) {
		...
		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);

Does this code accidentally use max() instead of min()? 

By taking the maximum, any safe limit provided via the device tree that is
lower than 20mA will be overridden to 20mA. Could this risk hardware damage
for chips rated for lower currents?

Additionally, if led_data->max_microamp is greater than 20mA, current_level
will exceed 255. Since is31fl32xx_write() accepts a u8 value, wouldn't this
result in silent integer truncation and unpredictable output currents?

[Severity: High]
This is also a pre-existing issue, but is there a state mismatch during
probe initialization?

Looking at is31fl32xx_probe():

	ret = is31fl32xx_parse_dt(dev, priv);
	if (ret)
		return ret;

	ret = is31fl32xx_init_regs(priv);

The is31fl32xx_parse_dt() function registers each led with the subsystem
using devm_led_classdev_register_ext(). This allows the led subsystem to
immediately apply triggers or default-state settings (like 'on' or 'keep')
via i2c writes.

However, right after parsing, we call is31fl32xx_init_regs(), which
performs a full hardware reset. Does this subsequent reset wipe out any
brightness or state configurations just applied by the led subsystem? 

Should the hardware initialization occur before exposing the device to the
subsystem?

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