| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20251113112947.GF1949330@google.com>
Date: Thu, 13 Nov 2025 11:29:47 +0000
From: Lee Jones <lee@...nel.org>
To: Nam Tran <trannamatk@...il.com>
Cc: gregkh@...uxfoundation.org, pavel@...nel.org, rdunlap@...radead.org,
christophe.jaillet@...adoo.fr, krzk+dt@...nel.org, robh@...nel.org,
conor+dt@...nel.org, corbet@....net, linux-leds@...r.kernel.org,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
linux-doc@...r.kernel.org
Subject: Re: [PATCH v17 2/3] leds: add basic support for TI/National
Semiconductor LP5812 LED Driver
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@...il.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 [李琼斯]
Powered by blists - more mailing lists