Re: [PATCH v1] 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:
- [Medium] leds: lm3533: unmasked upper bits in lm3533_led_setup() regmap update
--

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

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

This commit adds Control Bank to HVLED and LVLED muxing support to the
lm3533 LED and backlight drivers based on the led-sources device tree
property.

> diff --git a/drivers/leds/leds-lm3533.c b/drivers/leds/leds-lm3533.c
> index 7259097bd6a71..a1aeff70a90fc 100644
> --- a/drivers/leds/leds-lm3533.c
> +++ b/drivers/leds/leds-lm3533.c
> @@ -26,6 +26,11 @@
>  #define LM3533_ALS_CHANNEL_LV_MIN	1
>  #define LM3533_ALS_CHANNEL_LV_MAX	2
>  
> +#define LM3533_REG_OUTPUT_CONF1			0x10
> +#define   OUTPUT_CONF1_SHIFT			2
> +#define   OUTPUT_LVLED_MASK			0x3
> +#define LM3533_REG_OUTPUT_CONF2			0x11
> +#define   OUTPUT_CONF2_SHIFT			6
>  #define LM3533_REG_CTRLBANK_BCONF_BASE		0x1b
>  #define LM3533_REG_PATTERN_ENABLE		0x28
>  #define LM3533_REG_PATTERN_LOW_TIME_BASE	0x71
> @@ -54,6 +59,9 @@ struct lm3533_led {
>  	u32 max_current;
>  	u32 pwm;
>  
> +	int num_leds;
> +	u32 leds[LM3533_LVCTRLBANK_MAX];
> +
>  	bool have_als;
>  };
>  
> @@ -622,8 +630,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: Medium]
In drivers/leds/leds-lm3533.c, when lm3533_led_setup() configures LVLED4
(3) or LVLED5 (4) on control bank D, E, or F (led->id > 0), output_cfg_mask
and output_cfg_val have bits [9:6] set. Left-shifting them by
OUTPUT_CONF1_SHIFT (2) without masking to 8 bits leaves bits [11:8] set
when updating the 8-bit register LM3533_REG_OUTPUT_CONF1.

In drivers/base/regmap/regmap.c:_regmap_update_bits():

	ret = _regmap_read(map, reg, &orig);
	if (ret != 0)
		return ret;

	tmp = orig & ~mask;
	tmp |= val & mask;

	if (force_write || (tmp != orig) || map->force_write_field) {
		ret = _regmap_write(map, reg, tmp);

Because orig is an 8-bit value (<= 0xff) and val & mask has bits [11:8]
set, tmp exceeds 0xff and tmp != orig is always true. This forces a
register write with tmp > 0xff, which causes regmap_smbus_byte_reg_write()
in drivers/base/regmap/regmap-i2c.c:

	if (val > 0xff || reg > 0xff)
		return -EINVAL;

to reject the write with -EINVAL and fail lm3533_led_probe() on SMBus
byte-data I2C controllers, or issue an unnecessary write on raw I2C
controllers.

Should the mask and value passed to regmap_update_bits() for
LM3533_REG_OUTPUT_CONF1 be masked to 8 bits (e.g. & 0xff or u8)?

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