Re: [PATCH v17 2/3] leds: add basic support for TI/National Semiconductor LP5812 LED Driver

[Nam Tran <trannamatk@gmail.com>]

On Thu, 6 Nov 2025, Lee Jones wrote:

> On Tue, 21 Oct 2025, Nam Tran wrote:
> 
> > The LP5812 is a 4x3 matrix RGB LED driver with an autonomous animation
> > engine and time-cross-multiplexing (TCM) support for up to 12 LEDs or
> > 4 RGB LEDs. Each LED can be configured through the related registers
> > to realize vivid and fancy lighting effects.
> > 
> > This patch adds minimal driver support for the LP5812, implementing
> > only the essential functionality: I2C communication with the device,
> > LED registration, brightness control in manual mode, and basic sysfs
> > interfaces for LED configuration and fault monitoring.
> > 
> > Signed-off-by: Nam Tran <trannamatk@gmail.com>
> > ---
> >  MAINTAINERS                    |   4 +
> >  drivers/leds/rgb/Kconfig       |  13 +
> >  drivers/leds/rgb/Makefile      |   1 +
> >  drivers/leds/rgb/leds-lp5812.c | 730 +++++++++++++++++++++++++++++++++
> >  drivers/leds/rgb/leds-lp5812.h | 197 +++++++++
> >  5 files changed, 945 insertions(+)
> >  create mode 100644 drivers/leds/rgb/leds-lp5812.c
> >  create mode 100644 drivers/leds/rgb/leds-lp5812.h
> 
> Last go - just a few nits to fix-up.

Thank you for the feedback.
I'll address these minor issues and include the fixes in the next revision.
But I have a few concerns about some of the nits.

> > +static int lp5812_parse_led(struct device_node *np,
> > +			    struct lp5812_led_config *cfg,
> > +			    int led_index)
> > +{
> > +	int num_colors = 0, ret;
> 
> As above.
> 
> > +
> > +	of_property_read_string(np, "label", &cfg[led_index].name);
> 
> Is this optional?

The 'label' property is required for proper sysfs naming. Should I update the DT binding
to mark it mandatory and adjust the driver accordingly? I'd like to confirm if this aligns
with usual conventions for such properties.

> > +static int lp5812_probe(struct i2c_client *client)
> > +{
> > +	struct lp5812_chip *chip;
> > +	struct device_node *np = dev_of_node(&client->dev);
> > +	struct lp5812_led *led;
> 
> This is all of the LEDs though, right.
> 
> So "leds" would be better.
> 
> > +	int ret;
> > +
> > +	if (!np)
> > +		return -EINVAL;
> > +
> > +	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
> > +	if (!chip)
> > +		return -ENOMEM;
> > +
> > +	chip->cfg = i2c_get_match_data(client);
> > +	ret = lp5812_of_populate_pdata(&client->dev, np, chip);
> 
> That's not all this function does though.
> 
> And it's not pdata.
> 
> lp5812_of_probe() would probably be better.
> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	led = devm_kcalloc(&client->dev, chip->num_channels, sizeof(*led), GFP_KERNEL);
> > +	if (!led)
> > +		return -ENOMEM;
> > +
> > +	chip->client = client;
> > +	mutex_init(&chip->lock);
> > +	i2c_set_clientdata(client, led);
> 
> If you're only using the chip, why not just save the chip?

Just to confirm, you mean to store all LED instances inside the lp5812_chip struct and
only save the chip in i2c_set_clientdata(), instead of allocating a separate leds array
in probe()?

I can update the code accordingly if that's the preferred approach.

> > +/* Chip specific configurations */
> > +static const struct lp5812_device_config lp5812_cfg = {
> > +	.reg_reset = {
> > +		.addr = LP5812_REG_RESET,
> > +		.val  = LP5812_RESET
> > +	},
> > +	.reg_chip_en = {
> > +		.addr = LP5812_REG_ENABLE,
> > +		.val  = LP5812_ENABLE_DEFAULT
> > +	},
> > +	.reg_dev_config_0 = {
> > +		.addr = LP5812_DEV_CONFIG0,
> > +		.val  = 0
> > +	},
> > +	.reg_dev_config_1 = {
> > +		.addr = LP5812_DEV_CONFIG1,
> > +		.val  = 0
> > +	},
> > +	.reg_dev_config_2 = {
> > +		.addr = LP5812_DEV_CONFIG2,
> > +		.val  = 0
> > +	},
> > +	.reg_dev_config_3 = {
> > +		.addr = LP5812_DEV_CONFIG3,
> > +		.val  = 0
> > +	},
> > +	.reg_dev_config_4 = {
> > +		.addr = LP5812_DEV_CONFIG4,
> > +		.val  = 0
> > +	},
> > +	.reg_dev_config_5 = {
> > +		.addr = LP5812_DEV_CONFIG5,
> > +		.val  = 0
> > +	},
> > +	.reg_dev_config_6 = {
> > +		.addr = LP5812_DEV_CONFIG6,
> > +		.val  = 0
> > +	},
> > +	.reg_dev_config_7 = {
> > +		.addr = LP5812_DEV_CONFIG7,
> > +		.val  = 0
> > +	},
> > +	.reg_dev_config_12 = {
> > +		.addr = LP5812_DEV_CONFIG12,
> > +		.val  = LP5812_DEV_CONFIG12_DEFAULT
> > +	},
> > +	.reg_cmd_update = {
> > +		.addr = LP5812_CMD_UPDATE,
> > +		.val  = 0
> > +	},
> > +	.reg_tsd_config_status = {
> > +		.addr = LP5812_TSD_CONFIG_STATUS,
> > +		.val  = 0
> > +	},
> > +	.reg_led_en_1 = {
> > +		.addr = LP5812_LED_EN_1,
> > +		.val  = 0
> > +	},
> > +	.reg_led_en_2 = {
> > +		.addr = LP5812_LED_EN_2,
> > +		.val  = 0
> > +	},
> > +	.reg_fault_clear = {
> > +		.addr = LP5812_FAULT_CLEAR,
> > +		.val  = 0
> > +	},
> > +	.reg_manual_dc_base  = {
> > +		.addr = LP5812_MANUAL_DC_BASE,
> > +		.val  = 0
> > +	},
> > +	.reg_auto_dc_base  = {
> > +		.addr = LP5812_AUTO_DC_BASE,
> > +		.val  = 0
> > +	},
> > +	.reg_manual_pwm_base  = {
> > +		.addr = LP5812_MANUAL_PWM_BASE,
> > +		.val  = 0
> > +	},
> > +	.reg_lod_status_base  = {
> > +		.addr = LP5812_LOD_STATUS,
> > +		.val  = 0
> > +	},
> > +	.reg_lsd_status_base  = {
> > +		.addr = LP5812_LSD_STATUS,
> > +		.val  = 0
> > +	}
> > +};
> 
> This is an unusual way to set out a register map.
> 
> Where have you seen this done before?
> 
> > +static const struct of_device_id of_lp5812_match[] = {
> > +	{ .compatible = "ti,lp5812", .data = &lp5812_cfg },
> 
> Seems odd to populate .data when you only have a single device.

I followed the style used in the lp55xx series drivers for the register map and device
config. I thought it makes sense to keep the same pattern to allow easier upgrade and
maintenance in the future. But you expect a more typical approach, right?

Thanks again for your review and support!

Best regards,
Nam Tran