Re: [PATCH 1/2] leds: is31f132xx: Add support for is31fl3206

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] leds: Double execution of LED control register initialization
- [Medium] leds: Missing is31fl3206 entry in legacy i2c_device_id table
--

commit 4f6d324bacf9d9547e9fba515fa86ec07e7ad36d
Author: Flavio Amieiro <flavio@amieiro.net>

leds: is31f132xx: Add support for is31fl3206

This patch adds support for the IS31FL3206 variant to the is31fl32xx LED
driver. It introduces a custom function to enable all channels since this
chip uses a different LED control register layout.

> diff --git a/drivers/leds/leds-is31fl32xx.c b/drivers/leds/leds-is31fl32xx.c
> index 285085d274538..9c09548acabfa 100644
> --- a/drivers/leds/leds-is31fl32xx.c
> +++ b/drivers/leds/leds-is31fl32xx.c

[ ... ]

> @@ -315,6 +342,33 @@ static int is31fl32xx_reset_regs(struct is31fl32xx_priv *priv)
>  	return 0;
>  }
>  
> +static int is31fl32xx_enable_all_channels(struct is31fl32xx_priv *priv)
> +{
> +	const struct is31fl32xx_chipdef *cdef = priv->cdef;
> +	int ret;
> +
> +	if (cdef->led_control_register_base != IS31FL32XX_REG_NONE) {
> +		u8 value =
> +			GENMASK(cdef->enable_bits_per_led_control_register-1, 0);
> +		u8 num_regs = cdef->channels /
> +			cdef->enable_bits_per_led_control_register;
> +		int i;
> +
> +		for (i = 0; i < num_regs; i++) {
> +			ret = is31fl32xx_write(priv,
> +					       cdef->led_control_register_base+i,
> +					       value);
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +
> +	if (cdef->enable_all_channels_func)
> +		return cdef->enable_all_channels_func(priv);

[Severity: Medium]
Since is31fl3206_cdef sets both led_control_register_base and
enable_all_channels_func, won't this generic loop write 0x01 to all 12 control
registers first?

Because there is no early return or else branch here, won't it fall through and
call the custom enable_all_channels_func, which writes 0x10 to the exact same
registers again?

Does this double-write waste I2C bandwidth during probe/resume and briefly
leave the hardware in an unintended state?

> +
> +	return 0;
> +}
>  

[ ... ]

> @@ -552,7 +594,22 @@ static const struct is31fl32xx_chipdef is31fl3293_cdef = {
>  	.sw_shutdown_func			= is31fl3293_software_shutdown,
>  };
>  
> +static const struct is31fl32xx_chipdef is31fl3206_cdef = {
> +	.channels				= 12,
> +	.shutdown_reg				= 0x00,
> +	.pwm_update_reg			= 0x13,
> +	.global_control_reg			= 0x26,
> +	.reset_reg				= 0x2f,
> +	.output_frequency_setting_reg		= 0x27,
> +	.pwm_register_base			= 0x04,
> +	.led_control_register_base		= 0x17,
> +	.enable_bits_per_led_control_register	= 1,
> +	.brightness_steps			= 256,
> +	.enable_all_channels_func		= is31fl3206_enable_all_channels,
> +};
> +
>  static const struct of_device_id of_is31fl32xx_match[] = {
> +	{ .compatible = "issi,is31fl3206",  .data = &is31fl3206_cdef, },
>  	{ .compatible = "issi,is31fl3293",  .data = &is31fl3293_cdef, },
>  	{ .compatible = "issi,is31fl3236",  .data = &is31fl3236_cdef, },

[Severity: Medium]
This adds the "issi,is31fl3206" string to the of_device_id match table, but
shouldn't a corresponding "is31fl3206" entry also be added to the is31fl32xx_id
array (the i2c_device_id table) further down in the file?

Does omitting this entry break driver auto-loading via I2C modalias and
sysfs-based device instantiation on non-DT platforms?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013-add-is31fl3206-support-v1-0-08c066cc241d@amieiro.net?part=1