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

[Escala, Edelweise]

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@xxxxxxxxxx>");
> > +MODULE_DESCRIPTION("LED driver for LTC3220 controllers");
> > +MODULE_LICENSE("GPL");
> >
> > --
> > 2.43.0
> >
> 
> --
> Lee Jones

Best Regards,
Edelweise Escala