Re: [PATCH v11 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 5 potential issue(s) to consider:
- [High] Typographical error in regmap_config uses an undefined enum value `REGCACHE_FLAT_S`, causing a build failure.
- [High] Aggregated mode (QUICK_WRITE) causes the regmap cache to desynchronize, breaking LEDs during regcache_sync().
- [Medium] regcache_sync() in the resume handler is a no-op because the regmap cache is never marked dirty.
- [Medium] User-provided brightness values in hardware patterns are incorrectly masked instead of clamped, causing wrap-around behavior.
- [Medium] The driver modifies the cached LED state (`uled_cfg->reg_value`) and hardware registers without driver-level synchronization, allowing concurrent operations to permanently desync the software cache from the hardware.
--

commit 531f73073c031952f85d8c75169d6129f992543c
Author: Edelweise Escala <edelweise.escala@analog.com>

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..8bb06e50d3c6a
> --- /dev/null
> +++ b/drivers/leds/leds-ltc3220.c
[ ... ]
> +static const struct regmap_config ltc3220_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = LTC3220_GRAD_BLINK_REG,
> +	.cache_type = REGCACHE_FLAT_S,

[Severity: High]
Does this cause a build failure?

The correct enum value defined by the kernel is REGCACHE_FLAT, but this uses
an undefined REGCACHE_FLAT_S.

> +};
[ ... ]
> +static int ltc3220_set_led_data(struct led_classdev *led_cdev,
> +				enum led_brightness brightness)
> +{
> +	struct ltc3220_uled_cfg *uled_cfg = container_of(led_cdev, struct ltc3220_uled_cfg,
> +							 led_cdev);
> +	struct ltc3220 *ltc3220 = container_of(uled_cfg - uled_cfg->led_index, struct ltc3220,
> +					       uled_cfg[0]);
> +	int ret;
> +
> +	ret = regmap_write(ltc3220->regmap, LTC3220_ULED_REG(uled_cfg->led_index),
> +			   brightness);

[Severity: High]
When configured in aggregated mode, the hardware's QUICK_WRITE feature is
enabled, and a single write to LED 0 implicitly updates LEDs 1-17 in
hardware.

However, the driver doesn't update the regmap cache for registers 0x02
through 0x12, leaving them at 0.

When regcache_sync() runs during resume, will it write out the cached 0s and
inadvertently turn off LEDs 1-17?

Should the driver manually sync the cache for these implicitly updated
registers?

> +	if (ret)
> +		return ret;
> +
> +	uled_cfg->reg_value = brightness;

[Severity: Medium]
Since the LED core does not strictly serialize asynchronous triggers against
all callback paths, is this unprotected assignment susceptible to a race
condition?

A concurrent call to ltc3220_set_led_data() or ltc3220_blink_set() could
cause the software reg_value to become permanently desynchronized from the
hardware register state.

Does this need a per-device or per-LED mutex to protect read-modify-write
operations?

> +
> +	/*
> +	 * When aggregated LED mode is enabled, writing to LED 1 updates all
> +	 * LEDs simultaneously via quick-write mode. Update cached values for
[ ... ]
> +static int ltc3220_pattern_set(struct led_classdev *led_cdev,
> +			       struct led_pattern *pattern,
> +			       u32 len, int repeat)
> +{
[ ... ]
> +	if (repeat != 1)
> +		return -EOPNOTSUPP;
> +
> +	start_brightness = pattern[0].brightness & LTC3220_LED_CURRENT_MASK;
> +	end_brightness = pattern[1].brightness & LTC3220_LED_CURRENT_MASK;

[Severity: Medium]
Will this bitwise masking cause wrap-around behavior if a user provides a
brightness value greater than 63 via the sysfs hw_pattern interface?

For instance, a value of 64 would wrap to 0, unexpectedly turning the LED
off.

Would it be safer to use min_t() to clamp the input instead of a
bitwise mask?

> +
> +	is_increasing = end_brightness > start_brightness;
[ ... ]
> +static int ltc3220_blink_set(struct led_classdev *led_cdev,
> +			     unsigned long *delay_on,
> +			     unsigned long *delay_off)
> +{
[ ... ]
> +	ret = regmap_update_bits(ltc3220->regmap, LTC3220_GRAD_BLINK_REG,
> +				 LTC3220_BLINK_MASK, FIELD_PREP(LTC3220_BLINK_MASK, blink_mode));
> +	if (ret)
> +		return ret;
> +
> +	/* Ensure LED has non-zero brightness before enabling blink mode */
> +	if (uled_cfg->reg_value == 0)
> +		uled_cfg->reg_value = led_cdev->max_brightness;

[Severity: Medium]
Could this rely on a stale reg_value if concurrent operations are
modifying the state without synchronization, as noted in
ltc3220_set_led_data() above?

> +
> +	return regmap_write(ltc3220->regmap, LTC3220_ULED_REG(uled_cfg->led_index),
> +			    FIELD_PREP(LTC3220_LED_MODE_MASK, LTC3220_BLINK_MODE) |
> +			    uled_cfg->reg_value);
> +}
[ ... ]
> +static int ltc3220_suspend(struct device *dev)
> +{
> +	struct ltc3220 *ltc3220 = i2c_get_clientdata(to_i2c_client(dev));
> +
> +	for (int i = 0; i < LTC3220_NUM_LEDS; i++) {
> +		if (ltc3220->uled_cfg[i].registered)
> +			led_classdev_suspend(&ltc3220->uled_cfg[i].led_cdev);
> +	}
> +
> +	return regmap_update_bits(ltc3220->regmap, LTC3220_COMMAND_REG,
> +				 LTC3220_SHUTDOWN_MASK, LTC3220_SHUTDOWN_MASK);
> +}

[Severity: Medium]
Should regcache_mark_dirty(ltc3220->regmap) be called here or in resume?

Without marking the cache dirty, the regcache_sync() in ltc3220_resume()
will silently return without restoring any registers, resulting in the
permanent loss of the blink and gradation states upon resume.

> +
> +static int ltc3220_resume(struct device *dev)
> +{
> +	struct ltc3220 *ltc3220 = i2c_get_clientdata(to_i2c_client(dev));
> +	int ret;
> +
> +	ret = regmap_update_bits(ltc3220->regmap, LTC3220_COMMAND_REG,
> +				 LTC3220_SHUTDOWN_MASK, 0);
> +	if (ret)
> +		return ret;
> +
> +	ret = regcache_sync(ltc3220->regmap);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616-ltc3220-driver-v11-0-6e51dd97ced6@analog.com?part=2