Re: [PATCH v10 2/2] leds: ltc3220: Add Support for LTC3220 18 channel LED Driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Duplicate LED `reg` values bypass the validation check, leading to a doubly-linked list corruption in the LED core.
- [Medium] Missing LED class suspend/resume calls in system PM callbacks.
- [Medium] The `pattern_set` callback silently ignores the `repeat` parameter, breaking continuous patterns.
- [Low] Setting blink mode on a deactivated LED causes an unnecessary I2C read and hardware glitch.
--

commit ff936b952a6a427e0232229537a10438e4f34a11
Author: Edelweise Escala <edelweise.escala@analog.com>
Subject: leds: ltc3220: Add Support for LTC3220 18 channel LED Driver

Add driver for the LTC3220 18-channel LED driver with I2C interface,
individual brightness control, and hardware-assisted blink/gradation features.

> diff --git a/drivers/leds/leds-ltc3220.c b/drivers/leds/leds-ltc3220.c
> new file mode 100644
> index 0000000000000..f95174ae451ba
> --- /dev/null
> +++ b/drivers/leds/leds-ltc3220.c
[ ... ]
> +static int ltc3220_pattern_set(struct led_classdev *led_cdev,
> +			       struct led_pattern *pattern,
> +			       u32 len, int repeat)
> +{

[Severity: Medium]
Does this callback silently ignore the repeat parameter? 

If the user requests infinite repetition (for example, repeat is -1), the 
driver returns 0 without programming continuous breathing. Should this return 
-EOPNOTSUPP when repetition is requested, so the LED core can fall back to 
software-driven repetition instead of halting after a single ramp?

> +	struct ltc3220_uled_cfg *uled_cfg = container_of(led_cdev, struct ltc3220_uled_cfg,
> +							 led_cdev);
[ ... ]
> +static int ltc3220_blink_set(struct led_classdev *led_cdev,
> +			     unsigned long *delay_on,
> +			     unsigned long *delay_off)
> +{
[ ... ]
> +	/* Ensure LED has non-zero brightness before enabling blink mode */
> +	if (uled_cfg->reg_value == 0) {
> +		ret = regmap_write(ltc3220->regmap, LTC3220_ULED_REG(uled_cfg->led_index),
> +				   led_cdev->max_brightness);
> +		if (ret)
> +			return ret;
> +
> +		uled_cfg->reg_value = led_cdev->max_brightness;
> +	}
> +
> +	return regmap_update_bits(ltc3220->regmap, LTC3220_ULED_REG(uled_cfg->led_index),
> +				  LTC3220_LED_MODE_MASK,
> +				  FIELD_PREP(LTC3220_LED_MODE_MASK, LTC3220_BLINK_MODE));
> +}

[Severity: Low]
When ltc3220_blink_set() is called on an LED with zero brightness, does this 
sequence cause a visual glitch and an unnecessary I2C read? 

The first regmap_write() turns the LED on solid in normal mode, and then 
regmap_update_bits() incurs an I2C read to switch it to blink mode. Could 
the driver construct the final 8-bit value from the cached brightness and 
write it directly using a single regmap_write()?

[ ... ]
> +static int ltc3220_suspend(struct device *dev)
> +{
> +	struct ltc3220 *ltc3220 = i2c_get_clientdata(to_i2c_client(dev));
> +
> +	return regmap_update_bits(ltc3220->regmap, LTC3220_COMMAND_REG,
> +				  LTC3220_SHUTDOWN_MASK, LTC3220_SHUTDOWN_MASK);
> +}

[Severity: Medium]
Does this suspend callback miss notifying the LED core about the suspension? 

By not iterating over the registered LEDs and calling led_classdev_suspend(), 
the LED core remains unaware the device is suspended and will not pause 
triggers. Could an active software trigger attempt an I2C transaction while 
the system is suspending, potentially causing bus timeouts?

[ ... ]
> +static int ltc3220_probe(struct i2c_client *client)
> +{
[ ... ]
> +	/* First pass: validate configuration and set up LED structures */
> +	device_for_each_child_node_scoped(&client->dev, child) {
> +		struct ltc3220_uled_cfg *led;
> +		u32 source;
[ ... ]
> +		/* LED node reg/index/address goes from 1 to 18 */
> +		led_index = source - 1;
> +		led = &ltc3220->uled_cfg[led_index];
> +
> +		if (led->led_cdev.dev)
> +			return dev_err_probe(&client->dev, -EINVAL,
> +					     "Duplicate LED reg %u found", source);

[Severity: High]
Is led_cdev.dev always null during the first pass of ltc3220_probe()? 

Since ltc3220 is allocated with devm_kzalloc(), led_cdev.dev will be zero. It 
is only assigned during devm_led_classdev_register_ext() in the second pass, 
so this duplicate checking logic appears to be bypassed.

If a device tree contains duplicate reg values, will it silently overwrite 
the same uled_cfg structure during this first pass?

[ ... ]
> +	/* Second pass: register LEDs after validation */
> +	device_for_each_child_node_scoped(&client->dev, child) {
> +		struct led_init_data init_data = {};
> +		struct ltc3220_uled_cfg *led;
> +		u32 source;
> +
> +		ret = fwnode_property_read_u32(child, "reg", &source);
> +		if (ret)
> +			return ret;
> +
> +		init_data.fwnode = child;
> +		init_data.devicename = "ltc3220";
> +
> +		led_index = source - 1;
> +		led = &ltc3220->uled_cfg[led_index];
> +
> +		ret = devm_led_classdev_register_ext(&client->dev, &led->led_cdev, &init_data);

[Severity: High]
Following from the duplicate check bypass above, if a duplicate reg value is 
present, will devm_led_classdev_register_ext() call list_add_tail() twice on 
the exact same node address? This would corrupt the LED core's doubly-linked 
list and reliably cause a kernel panic upon iteration or removal.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615-ltc3220-driver-v10-0-916562c88598@analog.com?part=2