RE: [PATCH v8 2/2] leds: ltc3208: Add driver for LTC3208 Multidisplay LED Driver

> -----Original Message-----
> From: [email protected] <[email protected]>
> Sent: Thursday, July 30, 2026 7:23 AM
> To: Roleda, Jan carlo <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH v8 2/2] leds: ltc3208: Add driver for LTC3208 Multidisplay
> LED Driver
> 
> [External]
> 
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s)
> to consider:
> - [Medium] Regcache desynchronization leads to inability to turn off LEDs left
> on by bootloader
> --
> 
> --- Patch [2]: [PATCH v8 2/2] leds: ltc3208: Add driver for LTC3208 Multidisplay
> LED Driver --- commit 438e39dda8c60e73fe40abad6d35331fd8bb4b80
> Author: Jan Carlo Roleda <[email protected]>
> 
> leds: ltc3208: Add driver for LTC3208 Multidisplay LED Driver
> 
> This commit adds support for the Analog Devices LTC3208 Multidisplay LED
> driver. It supports 7 distinct LED channels and uses I2C for configuration,
> utilizing regmap for register access and caching.
> 
> > diff --git a/drivers/leds/leds-ltc3208.c b/drivers/leds/leds-ltc3208.c
> > new file mode 100644
> > --- /dev/null
> > +++ b/drivers/leds/leds-ltc3208.c
> [ ... ]
> > +static int ltc3208_probe(struct i2c_client *client) {
> [ ... ]
> > +	regmap = devm_regmap_init_i2c(client, &ltc3208_regmap_cfg);
> > +	if (IS_ERR(regmap))
> > +		return dev_err_probe(&client->dev, PTR_ERR(regmap),
> "Failed to
> > +initialize regmap.");
> 
> [Severity: Medium]
> Does this code need to synchronize the register cache with the hardware
> during initialization, for instance by calling regcache_sync() or explicitly
> clearing the LED DAC registers?
> 
> If the bootloader or firmware leaves any LEDs physically turned on, the
> regmap cache is still initialized with a default value of 0 for the LED DAC
> registers (via ltc3208_reg_defaults), but the hardware itself is not reset to 0.
> 
> If userspace later tries to turn off the LED by setting a brightness of 0, will
> regmap_field_write() see that the requested value matches the cached value
> (0) and optimize away the I2C transaction? This would leave the LED physically
> on while the kernel and sysfs report it as off, requiring the user to first set a
> non-zero brightness to force a hardware update before they can successfully
> turn it off.
> 

Noted. I'll include the regcache_sync() during probe time to ensure synchronicity
between driver and hardware.

> > +
> > +	disable_camhl_pin = device_property_read_bool(&client->dev,
> > +"adi,disable-camhl-pin");
> [ ... ]
> 
> --
> Sashiko AI review ·
> https://urldefense.com/v3/__https://sashiko.dev/*/patchset/20260730-
> upstream-ltc3208-v8-0-
> [email protected]?part=2__;Iw!!A3Ni8CS0y2Y!-0A1_9-C-
> qQXpaWIffAi4aL6kTAUUDNPzVbmPuKZX-
> yjTavHiEmeDCGFX2H4OdtTK4DtV0uybUGYGC-COmh8K-IGsA$

Regards,
Carlo