Message-Id: <20250904161728.24020-1-trannamatk@gmail.com>
Date: Thu,  4 Sep 2025 23:17:28 +0700
From: Nam Tran <trannamatk@...il.com>
To: lee@...nel.org
Cc: 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 v13 RESEND 2/4] leds: add basic support for TI/National Semiconductor LP5812 LED Driver

On Tue, 2 Sep 2025, Lee Jones wrote:

> On Mon, 18 Aug 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 | 1086 ++++++++++++++++++++++++++++++++
> >  drivers/leds/rgb/leds-lp5812.h |  164 +++++
> >  5 files changed, 1268 insertions(+)
> >  create mode 100644 drivers/leds/rgb/leds-lp5812.c
> >  create mode 100644 drivers/leds/rgb/leds-lp5812.h
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 99512777b890..c2e1c02e206d 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -24828,6 +24828,10 @@ M:	Nam Tran <trannamatk@...il.com>
> >  L:	linux-leds@...r.kernel.org
> >  S:	Maintained
> >  F:	Documentation/devicetree/bindings/leds/ti,lp5812.yaml
> > +F:	drivers/leds/rgb/Kconfig
> > +F:	drivers/leds/rgb/Makefile
> > +F:	drivers/leds/rgb/leds-lp5812.c
> > +F:	drivers/leds/rgb/leds-lp5812.h
> >  
> >  TEXAS INSTRUMENTS' LB8864 LED BACKLIGHT DRIVER
> >  M:	Alexander Sverdlin <alexander.sverdlin@...mens.com>
> > diff --git a/drivers/leds/rgb/Kconfig b/drivers/leds/rgb/Kconfig
> > index 222d943d826a..28ef4c487367 100644
> > --- a/drivers/leds/rgb/Kconfig
> > +++ b/drivers/leds/rgb/Kconfig
> > @@ -26,6 +26,19 @@ config LEDS_KTD202X
> >  	  To compile this driver as a module, choose M here: the module
> >  	  will be called leds-ktd202x.
> >  
> > +config LEDS_LP5812
> > +	tristate "LED support for Texas Instruments LP5812"
> > +	depends on I2C
> > +	help
> > +	  If you say Y here you get support for TI LP5812 LED driver.
> > +	  The LP5812 is a 4x3 matrix RGB LED driver with autonomous
> > +	  animation engine control.
> > +
> > +	  To compile this driver as a module, choose M here: the
> > +	  module will be called leds-lp5812.
> > +
> > +	  If unsure, say N.
> > +
> >  config LEDS_NCP5623
> >  	tristate "LED support for NCP5623"
> >  	depends on I2C
> > diff --git a/drivers/leds/rgb/Makefile b/drivers/leds/rgb/Makefile
> > index a501fd27f179..be45991f63f5 100644
> > --- a/drivers/leds/rgb/Makefile
> > +++ b/drivers/leds/rgb/Makefile
> > @@ -2,6 +2,7 @@
> >  
> >  obj-$(CONFIG_LEDS_GROUP_MULTICOLOR)	+= leds-group-multicolor.o
> >  obj-$(CONFIG_LEDS_KTD202X)		+= leds-ktd202x.o
> > +obj-$(CONFIG_LEDS_LP5812)		+= leds-lp5812.o
> >  obj-$(CONFIG_LEDS_NCP5623)		+= leds-ncp5623.o
> >  obj-$(CONFIG_LEDS_PWM_MULTICOLOR)	+= leds-pwm-multicolor.o
> >  obj-$(CONFIG_LEDS_QCOM_LPG)		+= leds-qcom-lpg.o
> > diff --git a/drivers/leds/rgb/leds-lp5812.c b/drivers/leds/rgb/leds-lp5812.c
> > new file mode 100644
> > index 000000000000..fb5ea449761a
> > --- /dev/null
> > +++ b/drivers/leds/rgb/leds-lp5812.c
> > @@ -0,0 +1,1086 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * LP5812 LED driver
> > + *
> > + * Copyright (C) 2025 Texas Instruments
> > + *
> > + * Author: Jared Zhou <jared-zhou@...com>
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/i2c.h>
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/led-class-multicolor.h>
> > +#include <linux/leds.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/types.h>
> > +
> > +#include "leds-lp5812.h"
> > +
> > +static int lp5812_write(struct lp5812_chip *chip, u16 reg, u8 val)
> > +{
> > +	struct device *dev = &chip->client->dev;
> > +	struct i2c_msg msg;
> > +	u8 buf[2];
> > +	u8 extracted_bits;
> 
> What bits are being extracted?
> 
> addr_low?
> 

According to the LP5812 datasheet (page 24, I2C Data Format section),
the first byte includes 5 bits for the chip address, the next 2 bits
are register address bits [9:8], and the last bit is the R/W flag.
Therefore, I need to extract bits [9:8] from the register address and
place them into bits [2:1] of the first byte. I'll also update the
variable name to make this clearer.

> > +	int ret;
> > +
> > +	/* Extract register address bits 9 and 8 for Address Byte 1 */
> > +	extracted_bits = (reg >> 8) & 0x03;
> 
> Define all magic numbers throughout.  This includes MASKs and SHIFTs.
> 

I'll replace the inline constants with proper macros.

> > +	/* Prepare payload: Address Byte 2 (bits [7:0]) and value to write */
> > +	buf[0] = (u8)(reg & 0xFF);
> > +	buf[1] = val;
> > +
> > +	/* Construct I2C message for a write operation */
> > +	msg.addr = (chip->client->addr << 2) | extracted_bits;
> > +	msg.flags = 0;
> > +	msg.len = sizeof(buf);
> > +	msg.buf = buf;
> > +
> > +	ret = i2c_transfer(chip->client->adapter, &msg, 1);
> 
> 	if (ret == 1)
> 		return 0;
> 
> 	dev_err(dev, "I2C write error, ret=%d\n", ret);
> 	return ret < 0 ? ret : -EIO;
> 
>

This logic is cleaner. I will update it.

> > +	if (ret != 1) {
> > +		dev_err(dev, "I2C write error, ret=%d\n", ret);
> > +		ret = ret < 0 ? ret : -EIO;
> > +	} else {
> > +		ret = 0;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static int lp5812_read(struct lp5812_chip *chip, u16 reg, u8 *val)
> > +{
> > +	struct device *dev = &chip->client->dev;
> > +	struct i2c_msg msgs[2];
> > +	u8 ret_val;
> > +	u8 extracted_bits;
> > +	u8 converted_reg;
> > +	int ret;
> > +
> > +	/* Extract register address bits 9 and 8 for Address Byte 1 */
> > +	extracted_bits = (reg >> 8) & 0x03;
> > +
> > +	/* Lower 8 bits go in Address Byte 2 */
> > +	converted_reg = (u8)(reg & 0xFF);
> > +
> > +	/* Prepare I2C write message to set register address */
> > +	msgs[0].addr = (chip->client->addr << 2) | extracted_bits;
> > +	msgs[0].flags = 0;
> > +	msgs[0].len = 1;
> > +	msgs[0].buf = &converted_reg;
> > +
> > +	/* Prepare I2C read message to retrieve register value */
> > +	msgs[1].addr = (chip->client->addr << 2) | extracted_bits;
> > +	msgs[1].flags = I2C_M_RD;
> > +	msgs[1].len = 1;
> > +	msgs[1].buf = &ret_val;
> > +
> > +	ret = i2c_transfer(chip->client->adapter, msgs, 2);
> 
> As above.
>

I will update this as well.

> > +	if (ret != 2) {
> > +		dev_err(dev, "I2C read error, ret=%d\n", ret);
> > +		*val = 0;
> > +		ret = ret < 0 ? ret : -EIO;
> > +	} else {
> > +		/* Store the value retrieved from the hardware */
> > +		*val = ret_val;
> > +		ret = 0;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static int lp5812_read_tsd_config_status(struct lp5812_chip *chip, u8 *reg_val)
> > +{
> > +	return lp5812_read(chip, chip->cfg->reg_tsd_config_status.addr, reg_val);
> > +}
> > +
> > +static int lp5812_update_regs_config(struct lp5812_chip *chip)
> > +{
> > +	u8 reg_val;
> > +	int ret;
> > +
> > +	ret = lp5812_write(chip, chip->cfg->reg_cmd_update.addr, LP5812_UPDATE_CMD_VAL);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = lp5812_read_tsd_config_status(chip, &reg_val); /* Save register value */
> 
> Save register value where?
>

This function just reads the TSD config status register into reg_val for verification.
This comment is redundant. I will remove it.

> > +	if (ret)
> > +		return ret;
> > +
> > +	return reg_val & 0x01;
> 
> What bit is this?  Please define it properly.
>

This is config_err_status. I will replace it with a properly defined macro.

> > +}
> > +
> > +static int lp5812_fault_clear(struct lp5812_chip *chip, u8 value)
> > +{
> > +
> > +	if (value == 0)
> 
> What do these values mean?  Define?
> 
> A switch() would be better.
>

I agree, I'll switch to 'switch()' and replace raw numbers with enums/macros for clarity.

> > +		reg_val = LOD_CLEAR_VAL;
> > +	else if (value == 1)
> > +		reg_val = LSD_CLEAR_VAL;
> > +	else if (value == 2)
> > +		reg_val = TSD_CLEAR_VAL;
> > +	else if (value == 3)
> > +		reg_val = FAULT_CLEAR_ALL;
> > +	else
> > +		return -EINVAL;
> > +
> > +	return lp5812_write(chip, chip->cfg->reg_reset.addr, reg_val);
> > +}
> > +
> > +static void set_mix_sel_led(struct lp5812_chip *chip, int mix_sel_led)
> 
> What is a "mix_sel_led"?
> 
> If forthcoming nomenclature can't be incorporated use comments.
>

I will change the name of this function.

> > +{
> > +	if (mix_sel_led == 0)
> > +		chip->u_drive_mode.s_drive_mode.mix_sel_led_0 = 1;
> 
> What are you doing here?
> 
> Why not something like:
> 
>   chip->u_drive_mode.s_drive_mode.mix_sel_led[mix_sel_led] = true;
> 
> Or if there is only one:
> 
>   chip->u_drive_mode.s_drive_mode.mix_sel_led = mix_sel_led;
>

You're right. I'll rework it.

> > +	if (mix_sel_led == 1)
> > +		chip->u_drive_mode.s_drive_mode.mix_sel_led_1 = 1;
> > +
> > +	if (mix_sel_led == 2)
> > +		chip->u_drive_mode.s_drive_mode.mix_sel_led_2 = 1;
> > +
> > +	if (mix_sel_led == 3)
> > +		chip->u_drive_mode.s_drive_mode.mix_sel_led_3 = 1;
> > +}
> > +
> > +static ssize_t parse_drive_mode(struct lp5812_chip *chip, char *str)
> > +{
> > +	char *sub_str;
> > +	int tcm_scan_num, mix_scan_num, mix_sel_led, scan_oder[4], i, ret;
> > +
> > +	chip->u_drive_mode.s_drive_mode.mix_sel_led_0 = 0;
> > +	chip->u_drive_mode.s_drive_mode.mix_sel_led_1 = 0;
> > +	chip->u_drive_mode.s_drive_mode.mix_sel_led_2 = 0;
> > +	chip->u_drive_mode.s_drive_mode.mix_sel_led_3 = 0;
> > +
> > +	sub_str = strsep(&str, ":");
> 
> This is totally unacceptable, sorry.
> 
> One value per sysfs file.
> 
> No parsing of weird and wonderful concats of data is allowed.
> 
> I'll end the review here.

I will change solution to get value from sysfs file. No parsing or concats.
Is it acceptable to consider a string format like "tcmscan:1:0:..." as a single value?

Thank you for the detailed review.

Best regards,
Nam Tran

Powered by blists - more mailing lists