Re: [PATCH v5 14/14] video: leds: backlight: lm3533: Support getting LED sources from DT

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Out-of-bounds bitmask shift causes regcache pollution and unnecessary I2C bus writes for LM3533_REG_OUTPUT_CONF1.
--

commit a74553be2d2b60a05a193e9dc35527d3d4a425da
Author: Svyatoslav Ryhel <clamor95@gmail.com>

video: leds: backlight: lm3533: Support getting LED sources from DT

This commit adds Control Bank to HVLED/LVLED muxing support based on
the led-sources defined in the device tree.

> diff --git a/drivers/leds/leds-lm3533.c b/drivers/leds/leds-lm3533.c
> index ed810c23f30f3..9e07953814fdd 100644
> --- a/drivers/leds/leds-lm3533.c
> +++ b/drivers/leds/leds-lm3533.c

[ ... ]

> @@ -623,8 +631,36 @@ static const struct attribute_group *lm3533_led_attribute_groups[] = {
>  
>  static int lm3533_led_setup(struct lm3533_led *led)
>  {
> +	u32 output_cfg_shift = 0;
> +	u32 output_cfg_val = 0;
> +	u32 output_cfg_mask = 0;
>  	int ret;
>  
> +	if (led->num_leds) {
> +		for (int i = 0; i < led->num_leds; i++) {
> +			if (led->leds[i] >= LM3533_LVCTRLBANK_MAX)
> +				continue;
> +
> +			output_cfg_shift = led->leds[i] * 2;
> +			output_cfg_val |= led->id << output_cfg_shift;
> +			output_cfg_mask |= OUTPUT_LVLED_MASK << output_cfg_shift;
> +		}
> +
> +		/* LVLED1, LVLED2 and LVLED3 */
> +		ret = regmap_update_bits(led->regmap, LM3533_REG_OUTPUT_CONF1,
> +					 output_cfg_mask << OUTPUT_CONF1_SHIFT,
> +					 output_cfg_val << OUTPUT_CONF1_SHIFT);

[Severity: Low]
Can this shift cause bits to fall outside the 8-bit register width?

When led-sources specifies LVLED4 or LVLED5, output_cfg_mask accumulates
bits that, when shifted left by OUTPUT_CONF1_SHIFT (which is 2), will exceed
8 bits (for example, forming masks like 0x300 or 0xC00).

Because regmap_update_bits operates on unsigned int internally, these
out-of-bounds bits are applied to the cached register value. This causes
the internal comparison (tmp != orig) to incorrectly evaluate to true,
triggering an unnecessary I2C write.

Although the I2C subsystem harmlessly truncates the value to 8 bits so the
hardware state is not corrupted, does it make sense to apply an explicit
8-bit mask (e.g., & 0xFF) before passing the shifted mask and value to
regmap_update_bits() to prevent regcache pollution and redundant bus traffic?

> +		if (ret)
> +			return ret;
> +
> +		/* LVLED4 and LVLED5 */
> +		ret = regmap_update_bits(led->regmap, LM3533_REG_OUTPUT_CONF2,
> +					 output_cfg_mask >> OUTPUT_CONF2_SHIFT,
> +					 output_cfg_val >> OUTPUT_CONF2_SHIFT);
> +		if (ret)
> +			return ret;
> +	}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260617080031.99156-1-clamor95@gmail.com?part=14