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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Register Overwrite Logic Error in LED and Backlight Setup
- [Medium] Silent failure when `led-sources` exceeds maximum supported items
--

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

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

Add 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 f2502daf450d8..6db1c1fd2e20d 100644
> --- a/drivers/leds/leds-lm3533.c
> +++ b/drivers/leds/leds-lm3533.c

[ ... ]

> @@ -641,7 +651,33 @@ static const struct attribute_group *lm3533_led_attribute_groups[] = {
>  
>  static int lm3533_led_setup(struct lm3533_led *led)
>  {
> -	int ret;
> +	u32 output_cfg_shift = 0;
> +	u32 output_cfg_val = 0;
> +	int ret, i;
> +
> +	if (led->num_leds) {
> +		for (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;
> +		}
> +
> +		/* LVLED1, LVLED2 and LVLED3 */
> +		ret = regmap_update_bits(led->lm3533->regmap, LM3533_REG_OUTPUT_CONF1,
> +					 OUTPUT_CONF1_MASK,
> +					 output_cfg_val << OUTPUT_CONF1_SHIFT);

[Severity: High]
Does this code inadvertently clear configurations for other LED strings?

Because OUTPUT_CONF1_MASK is a static broad mask (GENMASK(7, 2)), the call to
regmap_update_bits() clears all LVLED1-3 fields that are not part of the
current output_cfg_val.

If multiple control banks are probed sequentially, it seems the later probe
will overwrite and break the hardware routing of previously configured banks.
Should the mask be dynamically generated based on the specific strings being
configured in led-sources instead of updating all bits?

> +		if (ret)
> +			return ret;
> +
> +		/* LVLED4 and LVLED5 */
> +		ret = regmap_update_bits(led->lm3533->regmap, LM3533_REG_OUTPUT_CONF2,
> +					 OUTPUT_CONF2_MASK,
> +					 output_cfg_val >> OUTPUT_CONF2_SHIFT);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	ret = lm3533_ctrlbank_set_max_current(&led->cb, led->max_current);

[ ... ]

> @@ -716,6 +752,23 @@ static int lm3533_led_probe(struct platform_device *pdev)
>  	led->pwm = 0;
>  	device_property_read_u32(&pdev->dev, "ti,pwm-config-mask", &led->pwm);
>  
> +	led->num_leds = device_property_count_u32(&pdev->dev, "led-sources");
> +
> +	/*
> +	 * If led-sources property is not set then either this Control Bank uses
> +	 * its default LVLED or is not linked to any LVLED at all.
> +	 */
> +	if (led->num_leds > 0 && led->num_leds <= LM3533_LVCTRLBANK_MAX) {
> +		ret = device_property_read_u32_array(&pdev->dev, "led-sources",
> +						     led->leds, led->num_leds);
> +		if (ret) {
> +			dev_err(&pdev->dev, "failed to get led-sources\n");
> +			goto err_deregister;
> +		}
> +	} else {
> +		led->num_leds = 0;
> +	}

[Severity: Medium]
Will this silently ignore out-of-bounds configurations?

If the user provides a device tree with more items than the hardware supports
(e.g., led->num_leds > LM3533_LVCTRLBANK_MAX), the check fails and the code
falls through to the else block.

Setting led->num_leds = 0 treats the oversized array as if the property was
entirely omitted, masking the error. Would it be better to explicitly return
an error like -EINVAL to alert the user of an invalid configuration?

> +
>  	ret = lm3533_led_setup(led);
>  	if (ret)
>  		goto err_deregister;

[ ... ]

> diff --git a/drivers/video/backlight/lm3533_bl.c b/drivers/video/backlight/lm3533_bl.c
> index 945625c54fc43..2119433bae456 100644
> --- a/drivers/video/backlight/lm3533_bl.c
> +++ b/drivers/video/backlight/lm3533_bl.c

[ ... ]

> @@ -253,7 +259,8 @@ static struct attribute_group lm3533_bl_attribute_group = {
>  static int lm3533_bl_setup(struct lm3533_bl *bl)
>  {
>  	int id = lm3533_bl_get_ctrlbank_id(bl);
> -	int ret;
> +	u32 output_cfg_val = 0;
> +	int ret, i;
>  
>  	ret = regmap_update_bits(bl->lm3533->regmap, LM3533_REG_CTRLBANK_AB_BCONF,
>  				 CTRLBANK_AB_BCONF_MODE(id),
> @@ -261,6 +268,20 @@ static int lm3533_bl_setup(struct lm3533_bl *bl)
>  	if (ret)
>  		return ret;
>  
> +	if (bl->num_leds) {
> +		for (i = 0; i < bl->num_leds; i++) {
> +			if (bl->led_strings[i] >= LM3533_HVCTRLBANK_COUNT)
> +				continue;
> +
> +			output_cfg_val |= id << bl->led_strings[i];
> +		}
> +
> +		ret = regmap_update_bits(bl->lm3533->regmap, LM3533_REG_OUTPUT_CONF1,
> +					 OUTPUT_CONF1_MASK, output_cfg_val);

[Severity: High]
Can this similarly clobber the other backlight control bank?

Since OUTPUT_CONF1_MASK is GENMASK(1, 0), it unconditionally updates all HVLED
routing fields at once. Configuring one backlight bank will overwrite the
configuration of the other bank.

Could the mask be limited only to the bits modified by the current bank?

> +		if (ret)
> +			return ret;
> +	}
> +
>  	ret = lm3533_ctrlbank_set_max_current(&bl->cb, bl->max_current);

[ ... ]

> @@ -336,6 +357,23 @@ static int lm3533_bl_probe(struct platform_device *pdev)
>  	bl->pwm = 0;
>  	device_property_read_u32(&pdev->dev, "ti,pwm-config-mask", &bl->pwm);
>  
> +	bl->num_leds = device_property_count_u32(&pdev->dev, "led-sources");
> +
> +	/*
> +	 * If led-sources property is not set then either this Control Bank uses
> +	 * its default HVLED or is not linked to any HVLED at all.
> +	 */
> +	if (bl->num_leds > 0 && bl->num_leds <= LM3533_HVCTRLBANK_COUNT) {
> +		ret = device_property_read_u32_array(&pdev->dev, "led-sources",
> +						     bl->led_strings,
> +						     bl->num_leds);
> +		if (ret)
> +			return dev_err_probe(&pdev->dev, ret,
> +					     "failed to get led-sources\n");
> +	} else {
> +		bl->num_leds = 0;
> +	}

[Severity: Medium]
Does this have the same silent failure behavior as the LED probe?

If bl->num_leds > LM3533_HVCTRLBANK_COUNT, it falls back to the else block
setting bl->num_leds = 0 without producing an error, ignoring the invalid
device tree array.

> +
>  	ret = lm3533_bl_setup(bl);
>  	if (ret)
>  		return ret;

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