Re: [PATCH v13 RESEND 2/4] leds: add basic support for TI/National Semiconductor LP5812 LED Driver

[Lee Jones <lee@kernel.org>]

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@gmail.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@gmail.com>
>  L:	linux-leds@vger.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@siemens.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@ti.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?

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

> +	/* 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;


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

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

> +	if (ret)
> +		return ret;
> +
> +	return reg_val & 0x01;

What bit is this?  Please define it properly.

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

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

> +{
> +	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;

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

-- 
Lee Jones [李琼斯]