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

[Escala, Edelweise]

After the new version another set of sashiko review. Will also handle fixing these

> -----Original Message-----
> From: sashiko-bot@xxxxxxxxxx <sashiko-bot@xxxxxxxxxx>
> Sent: Monday, June 15, 2026 1:38 PM
> To: Escala, Edelweise <Edelweise.Escala@xxxxxxxxxx>
> Cc: robh@xxxxxxxxxx; conor+dt@xxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v10 2/2] leds: ltc3220: Add Support for LTC3220 18 channel
> LED Driver
> 
> [External]
> 
> 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@xxxxxxxxxx>
> 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://urldefense.com/v3/__https://sashiko.dev/*/patchset/20260615-ltc3220-
> driver-v10-0-
> 916562c88598@xxxxxxxxxx?part=2__;Iw!!A3Ni8CS0y2Y!4ZC1ljfp19dgTz1tKIoor
> RXY65a_q24Uz9NwdL22nTU8VrPKjf97nkBWY8TZBbk8NJ7uQlaukxYptqOUCWxA
> VtNI08A$