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

Escala, Edelweise
2026-05-04T01:31:28Z

Hello Lee,

Thank you for the review.

> > +static const struct regmap_config ltc3220_regmap_config = {
> > + .reg_bits = 8,
> > + .val_bits = 8,
> > + .max_register = LTC3220_GRAD_BLINK_REG, };
>
> .cache_type?
>

I will add
.cache_type = REGCACHE_FLAT,

> > +
> > +struct ltc3220_uled_cfg {
> > + struct ltc3220_state *ltc3220_state;
>
> This is not a linked list. Use container_of() instead.
>

Will drop struct ltc3220_state *ltc3220_state;, and use container_of()

> > + u8 reg_value;
> > + u8 led_index;
> > +};
> > +
> > +struct ltc3220_state {
>
> Drop the "_state" part.
>

Will drop "_state"

> > +static int ltc3220_shutdown(struct ltc3220_state *ltc3220_state) {
> > + return regmap_update_bits(ltc3220_state->regmap,
> LTC3220_COMMAND_REG,
> > + LTC3220_SHUTDOWN_MASK,
> LTC3220_SHUTDOWN_MASK); }
> > +
> > +static int ltc3220_resume_from_shutdown(struct ltc3220_state
> > +*ltc3220_state) {
> > + return regmap_update_bits(ltc3220_state->regmap,
> LTC3220_COMMAND_REG,
> > + LTC3220_SHUTDOWN_MASK, 0);
> > +}
>
> These do not need to be abstracted out.
>

Will drop these functions and use inline

> > +
> > +/*
> > + * Set LED brightness and mode.
> > + * The brightness value determines both the LED current and operating mode:
> > + * 0-63: Normal mode - LED current from 0-63 (off to full brightness)
> > + * 64-127: Blink mode - LED blinks with current level (brightness -
> > +64)
> > + * 128-191: Gradation mode - LED gradually changes brightness
> > +(brightness - 128)
> > + * 192-255: GPO mode - LED operates as general purpose output
> > +(brightness - 192) */ static int ltc3220_set_led_data(struct
> > +led_classdev *led_cdev,
> > + enum led_brightness brightness)
> > +{
> > + struct ltc3220_state *ltc3220_state;
> > + struct ltc3220_uled_cfg *uled_cfg;
> > + int ret;
> > + int i;
> > +
> > + uled_cfg = container_of(led_cdev, struct ltc3220_uled_cfg, led_cdev);
> > + ltc3220_state = uled_cfg->ltc3220_state;
> > +
> > + ret = regmap_write(ltc3220_state->regmap,
> LTC3220_ULED_REG(uled_cfg->led_index),
> > + brightness);
> > + if (ret < 0)
>
> Should we be using 'if (ret)' to check for errors here instead of 'if (ret < 0)'?
>

Will use if (ret)

> > + /*
> > + * When aggregated LED mode is enabled, writing to LED 1 updates all
> > + * LEDs simultaneously via quick-write mode. Update cached values for
> > + * all LEDs to reflect the synchronized state.
> > + * See Documentation/devicetree/bindings/leds/adi,ltc3220.yaml for
> how
> > + * to configure aggregated LED mode.
> > + */
> > + if (ltc3220_state->is_aggregated && uled_cfg->led_index == 0) {
> > + for (i = 0; i < LTC3220_NUM_LEDS; i++)
>
> for (int i = 0; ...
>

Will apply this.

> > + ltc3220_state->uled_cfg[i].reg_value = brightness;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static enum led_brightness ltc3220_get_led_data(struct led_classdev
> > +*led_cdev) {
> > + struct ltc3220_uled_cfg *uled_cfg = container_of(led_cdev,
> > + struct
> ltc3220_uled_cfg, led_cdev);
> > +
> > + return uled_cfg->reg_value;
> > +}
> > +
> > +/*
> > + * LTC3220 pattern support for hardware-assisted breathing/gradation.
> > + * The hardware supports 3 gradation ramp time 240ms, 480ms, 960ms)
> > + * and can ramp up or down.
> > + *
> > + * Pattern array interpretation:
> > + * pattern[0].brightness = start brightness (0-63)
> > + * pattern[0].delta_t = ramp time in milliseconds
> > + * pattern[1].brightness = end brightness (0-63)
> > + * pattern[1].delta_t = (optional, can be 0 or same as pattern[0].delta_t)
> > + */
> > +static int ltc3220_pattern_set(struct led_classdev *led_cdev,
> > + struct led_pattern *pattern,
> > + u32 len, int repeat)
> > +{
> > + struct ltc3220_uled_cfg *uled_cfg = container_of(led_cdev, struct
> ltc3220_uled_cfg,
> > + led_cdev);
>
> This is the 3rd time we do this and every time has been different.
>

Will make it consistent

> > + struct ltc3220_state *ltc3220_state = uled_cfg->ltc3220_state;
> > + u8 gradation_period;
> > + u8 start_brightness;
> > + u8 end_brightness;
> > + u8 reg_val;
>
> Something a little more descriptive please.
>

Will improve name to gradation_val

> > +static int ltc3220_pattern_clear(struct led_classdev *led_cdev) {
> > + struct ltc3220_uled_cfg *uled_cfg = container_of(led_cdev, struct
> ltc3220_uled_cfg,
> > + led_cdev);
> > + struct ltc3220_state *ltc3220_state = uled_cfg->ltc3220_state;
> > +
> > + return regmap_update_bits(ltc3220_state->regmap,
> LTC3220_GRAD_BLINK_REG,
> > +
> LTC3220_GRADATION_MASK, 0);
>
> Odd tabbing.
>

Will fix tabbing

> > + if (ret < 0)
> > + return ret;
> > +
> > + for (i = 0; i < LTC3220_NUM_LEDS; i++) {
>
> As above.
>

Will also apply changes on for loop

> > +static const struct of_device_id ltc3220_of_match[] = {
> > + { .compatible = "adi,ltc3220" },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, ltc3220_of_match);
> > +
> > +static struct i2c_driver ltc3220_led_driver = {
> > + .driver = {
> > + .name = "ltc3220",
> > + .of_match_table = ltc3220_of_match,
> > + .pm = pm_sleep_ptr(&ltc3220_pm_ops),
> > + },
> > + .probe = ltc3220_probe,
> > +};
> > +module_i2c_driver(ltc3220_led_driver);
> > +
> > +MODULE_AUTHOR("Edelweise Escala <edelweise.escala@analog.com>");
> > +MODULE_DESCRIPTION("LED driver for LTC3220 controllers");
> > +MODULE_LICENSE("GPL");
> >
> > --
> > 2.43.0
> >
>
> --
> Lee Jones

Best Regards,
Edelweise Escala