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

[Escala, Edelweise]

Hi Sander,

Thank you for the review!

> Hi Edelweise,
> 
> On Fri, 2026-05-08 at 12:09 +0800, Edelweise Escala wrote:
> > +static const struct regmap_config ltc3220_regmap_config = {
> > +	.reg_bits = 8,
> > +	.val_bits = 8,
> > +	.max_register = LTC3220_GRAD_BLINK_REG,
> > +	.cache_type = REGCACHE_FLAT,
> > +	.volatile_reg = ltc3220_volatile_reg, };
> 
> With this config, you are assuming that all register values will default to 0,
> otherwise the cache will not work properly. While the datasheet seems to
> indicate this is the case, I also suspect you are now seeing the warning
> 
>     "using zero-initialized flat cache, this may cause unexpected behavior"
> 
> I would suggest to use REGCACHE_FLAT_S instead.
> 

I will change cache_type from REGCACHE_FLAT to REGCACHE_FLAT_S to properly 
handle zero-initialized cache without warnings.

> 
> > +static int ltc3220_reset(struct ltc3220 *ltc3220, struct i2c_client
> > +*client) {
> > +	struct gpio_desc *reset_gpio;
> > +	int ret;
> > +
> > +	reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
> > GPIOD_OUT_HIGH);
> > +	if (IS_ERR(reset_gpio))
> > +		return dev_err_probe(&client->dev, PTR_ERR(reset_gpio),
> > "Failed on reset GPIO\n");
> > +
> > +	if (reset_gpio) {
> > +		gpiod_set_value_cansleep(reset_gpio, 0);
> > +
> > +		return devm_add_action_or_reset(&client->dev,
> > ltc3220_reset_gpio_action,
> > +						reset_gpio);
> > +	}
> > +
> > +	ret = regmap_write(ltc3220->regmap, LTC3220_COMMAND_REG, 0);
> > +	if (ret)
> > +		return ret;
> > +
> > +	for (int i = 0; i < LTC3220_NUM_LEDS; i++) {
> > +		ret = regmap_write(ltc3220->regmap, LTC3220_ULED_REG(i), 0);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return regmap_write(ltc3220->regmap, LTC3220_GRAD_BLINK_REG, 0); }
> 
> regmap_write() will always go to hardware, so this feels a bit like you're manually
> forcing the hardware to match the cache state. The implied all-0 cache could
> otherwise prevent later regmap_update_bits() calls from actually performing a
> write.
> 
> > +static DEFINE_SIMPLE_DEV_PM_OPS(ltc3220_pm_ops, ltc3220_suspend,
> > ltc3220_resume);
> > +
> > +static int ltc3220_probe(struct i2c_client *client) {
> > +	struct ltc3220 *ltc3220;
> > +	bool aggregated_led_found = false;
> > +	int num_leds = 0;
> > +	u8 led_index = 0;
> > +	int ret;
> > +
> > +	ltc3220 = devm_kzalloc(&client->dev, sizeof(*ltc3220), GFP_KERNEL);
> > +	if (!ltc3220)
> > +		return -ENOMEM;
> > +
> > +	ltc3220->regmap = devm_regmap_init_i2c(client,
> > &ltc3220_regmap_config);
> > +	if (IS_ERR(ltc3220->regmap))
> > +		return dev_err_probe(&client->dev, PTR_ERR(ltc3220->regmap),
> > +				     "Failed to initialize regmap\n");
> > +
> > +	i2c_set_clientdata(client, ltc3220);
> > +
> > +	ret = ltc3220_reset(ltc3220, client);
> > +	if (ret)
> > +		return dev_err_probe(&client->dev, ret, "Failed to reset
> > device\n");
> 
> Up to you if you want to clear the device configuration (maybe this causes LED
> flickering?), but with REGCACHE_FLAT_S, you should be able to maintain the boot
> state of the device.
> 
> Best,
> Sander

For the reset behavior, I'm keeping the current approach (clearing all 
registers on probe) since I haven't observed any flickering, and it ensures a 
known clean state. Let me know if you have concerns with this approach.

Best regards,
Edelweise