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;
> 
> [ ... ]

This is already handled correctly. The resume function:
1. Reads COMMAND_REG to detect if QUICK_WRITE is enabled
2. Temporarily disables QUICK_WRITE before regcache_sync()
3. Performs regcache_sync() with QUICK_WRITE disabled
4. Re-enables QUICK_WRITE after sync completes

With QUICK_WRITE disabled during regcache_sync(), writes to LEDs 2-18 apply 
only to those specific channels, not broadcast to all. The cached zeros correctly 
restore each LED's individual off state. When QUICK_WRITE is re-enabled afterward, 
subsequent brightness writes work correctly.


> 
> > @@ -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?
> 

The driver intentionally omits LED_CORE_SUSPENDRESUME because:
1. Hardware power management is handled via the SHUTDOWN bit
2. The LED core's forced brightness-to-zero on suspend would interfere with 
   hardware blink/gradation state restoration


> >  		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://urldefense.com/v3/__https://sashiko.dev/*/patchset/20260709-ltc3220-
> driver-v13-0-
> [email protected]?part=2__;Iw!!A3Ni8CS0y2Y!5sRuO2kidckEwZZPnnr
> M2kxEPjgElsbpjq_JYWKopaoBxsxjSoGN4OBNJg9BlPKQcBKNHsRD1Lmz-
> 9bG73HqJXnacOE$