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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Aggregated LED mode breaks after suspend/resume because regcache_sync() overwrites LEDs 2-18 with zeros.
- [Medium] Missing LED_CORE_SUSPENDRESUME flag and missing locking in suspend/resume handlers allow race conditions.
--

--- Patch [2]: [PATCH v13 2/2] leds: ltc3220: Add Support for LTC3220 18 channel LED Driver ---
commit 530249002ce164e8156c89d4dcd1994b3713c0c8
Author: Edelweise Escala <[email protected]>

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
> --- /dev/null
> +++ b/drivers/leds/leds-ltc3220.c

[ ... ]

> @@ -371,20 +371,11 @@ static int ltc3220_suspend(struct device *dev)
>  {
>  	struct ltc3220 *ltc3220 = i2c_get_clientdata(to_i2c_client(dev));
>  	int ret;
>  

[Severity: Medium]
Should ltc3220->lock be held here?

Because LED_CORE_SUSPENDRESUME is not set on the led_classdev, triggers and
sysfs writes can still fire during suspend, potentially racing with these
state changes and executing I2C writes while the device is in shutdown.

>  	ret = regmap_update_bits(ltc3220->regmap, LTC3220_COMMAND_REG,
>  				 LTC3220_SHUTDOWN_MASK, LTC3220_SHUTDOWN_MASK);
>  	if (ret)
>  		return ret;

[ ... ]

> @@ -391,24 +382,15 @@ static int ltc3220_resume(struct device *dev)
>  {
>  	struct ltc3220 *ltc3220 = i2c_get_clientdata(to_i2c_client(dev));
>  	bool quick_write_enabled;
>  	unsigned int command_reg;
>  	int ret;

[ ... ]

>  	if (quick_write_enabled) {
>  		ret = regmap_update_bits(ltc3220->regmap, LTC3220_COMMAND_REG,
>  					 LTC3220_QUICK_WRITE_MASK, 0);
>  		if (ret)
>  			return ret;
>  	}
>  
>  	ret = regmap_update_bits(ltc3220->regmap, LTC3220_COMMAND_REG,
>  				 LTC3220_SHUTDOWN_MASK, 0);
>  	if (ret)
>  		return ret;
>  
>  	usleep_range(10000, 12000);
>  

[Severity: Medium]
Because the driver does not set LED_CORE_SUSPENDRESUME or hold ltc3220->lock
during resume, can an LED trigger fire during this sleep?

If a trigger calls ltc3220_set_led_data() here while QUICK_WRITE is temporarily
disabled, the write would only apply to a single LED instead of broadcasting
to all channels.

>  	ret = regcache_sync(ltc3220->regmap);

[Severity: High]
Does this regcache_sync() break aggregated LED mode?

Since the cache for LEDs 2-18 contains zeros from their initial state, and
QUICK_WRITE is disabled during the sync, this looks like it will explicitly
write zeros to those channels, leaving most LEDs permanently off after resume.

>  	if (ret)
>  		return ret;

[ ... ]

> @@ -499,19 +481,10 @@ static int ltc3220_probe(struct i2c_client *client)

[ ... ]

>  	/* 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;

[ ... ]

>  		init_data.fwnode = child;
>  		init_data.devicename = "ltc3220";
>  
>  		led_index = source - 1;
>  		led = &ltc3220->uled_cfg[led_index];
>  

[Severity: Medium]
Does this led_cdev need the LED_CORE_SUSPENDRESUME flag set in its flags
field so the LED core knows to suspend triggers during system suspend?

>  		ret = devm_led_classdev_register_ext(&client->dev, &led->led_cdev, &init_data);
>  		if (ret)
>  			return dev_err_probe(&client->dev, ret, "Failed to register LED class\n");
>  	}
>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2