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

[Lee Jones <lee@kernel.org>]

On Wed, 12 Nov 2025, Nam Tran wrote:

> 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.

I'll let you look around and decide for yourself.

If this is not optional, you should check this call for errors.

> > > +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()?

At the moment, it looks as though you save the array of `led`s and pull
out the `chip` pointer from the first one (in .remove() below).  Why not
just store the `chip` in clientdata in the first place?

> 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?

You only need to provide differentiation when you support more than one
device.

-- 
Lee Jones [李琼斯]