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

Nam Tran
2025-11-11T17:07:36Z

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