On Tue, 2 Sep 2025, Lee Jones wrote:

On Mon, 18 Aug 2025, Nam Tran wrote:

quoted

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?

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.

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

+	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