[PATCH 0/2] leds: Introduce Texas Instruments LP5816 LED driver

[PATCH 0/2] leds: Introduce Texas Instruments LP5816 LED driver

[Alistair Bell]

The patch series adds support for the Texas Instruments LP5816
4-channel I2C interface RGBW LED driver.

Provides sysfs control of fade, fade duration, fade profile and
control of maximum output current.

Tested on: Qualcomm's QCS615 arm64 platform via a daughter board
display adapter.

Alistair Bell (2):
  leds: Texas Instruments LP5816 driver
  dt-bindings: leds: Add binding for ti,lp5816

 .../ABI/testing/sysfs-class-leds-lp5816       | 123 +++++
 .../devicetree/bindings/leds/ti,lp5816.yaml   |  45 ++
 MAINTAINERS                                   |  10 +
 drivers/leds/Kconfig                          |  12 +
 drivers/leds/Makefile                         |   1 +
 drivers/leds/leds-lp5816.c                    | 443 ++++++++++++++++++
 6 files changed, 634 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-leds-lp5816
 create mode 100644 Documentation/devicetree/bindings/leds/ti,lp5816.yaml
 create mode 100644 drivers/leds/leds-lp5816.c

-- 
2.54.0

[PATCH 1/2] leds: Texas Instruments LP5816 driver

[Alistair Bell]

Add support for Texas Instruments LP5816 4-channel I2C device,
the driver provides:

- Independent 4-channel control via the multicolor sysfs class
- Configurable fade effects, duration, fade profile and maximum
  operating current via sysfs

Signed-off-by: Alistair Bell <dev@alistairbell.org>
---
 MAINTAINERS                |   8 +
 drivers/leds/Kconfig       |  12 +
 drivers/leds/Makefile      |   1 +
 drivers/leds/leds-lp5816.c | 443 +++++++++++++++++++++++++++++++++++++
 4 files changed, 464 insertions(+)
 create mode 100644 drivers/leds/leds-lp5816.c

diff --git a/MAINTAINERS b/MAINTAINERS
index f37a81950..b6a8c812a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -26761,6 +26761,14 @@ F:	drivers/leds/rgb/Makefile
 F:	drivers/leds/rgb/leds-lp5812.c
 F:	drivers/leds/rgb/leds-lp5812.h
 
+TEXAS INSTRUMENTS' LP5816 RGBW LED DRIVER
+M:	Alistair Bell <dev@alistairbell.org>
+L:	linux-leds@vger.kernel.org
+S:	Maintained
+F:	drivers/leds/Kconfig
+F:	drivers/leds/Makefile
+F:	drivers/leds/leds-lp5816.c
+
 TEXAS INSTRUMENTS' LB8864 LED BACKLIGHT DRIVER
 M:	Alexander Sverdlin <alexander.sverdlin@siemens.com>
 L:	linux-leds@vger.kernel.org
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index f4a0a3c8c..ce3776adf 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -541,6 +541,18 @@ config LEDS_LP8864
 	  To compile this driver as a module, choose M here: the
 	  module will be called leds-lp8864.
 
+config LEDS_LP5816
+    tristate "LED driver for LP5816 chip"
+    depends on I2C
+    depends on LEDS_CLASS && LEDS_CLASS_MULTICOLOR
+	select REGMAP_I2C
+    help
+      Say Y to enable support for the Texas Instruments LP5816
+      RGBW LED connected via I2C.
+
+      To compile this driver as a module, choose M here:
+      the module will be called lp5816.
+
 config LEDS_CLEVO_MAIL
 	tristate "Mail LED on Clevo notebook"
 	depends on LEDS_CLASS && BROKEN
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 7db376891..5ba7de099 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -56,6 +56,7 @@ obj-$(CONFIG_LEDS_LP5523)		+= leds-lp5523.o
 obj-$(CONFIG_LEDS_LP5562)		+= leds-lp5562.o
 obj-$(CONFIG_LEDS_LP5569)		+= leds-lp5569.o
 obj-$(CONFIG_LEDS_LP55XX_COMMON)	+= leds-lp55xx-common.o
+obj-$(CONFIG_LEDS_LP5816)   	+= leds-lp5816.o
 obj-$(CONFIG_LEDS_LP8501)		+= leds-lp8501.o
 obj-$(CONFIG_LEDS_LP8788)		+= leds-lp8788.o
 obj-$(CONFIG_LEDS_LP8860)		+= leds-lp8860.o
diff --git a/drivers/leds/leds-lp5816.c b/drivers/leds/leds-lp5816.c
new file mode 100644
index 000000000..584c348a7
--- /dev/null
+++ b/drivers/leds/leds-lp5816.c
@@ -0,0 +1,443 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Texas Instruments LP5816 4-Channel I2C Interface RGBW LED Driver
+ * Copyright (C) 2026 Alistair Bell <dev@alistairbell.org>
+ */
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/leds.h>
+#include <linux/led-class-multicolor.h>
+#include <linux/minmax.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+#include <linux/string.h>
+
+#define REG_CHIP_EN 0x0
+#define CHIP_EN_ENABLE 0x1
+
+#define REG_DEV_CONFIG0 0x1
+#define DEV_CONFIG0_25_5_MA 0x0
+#define DEV_CONFIG0_51_MA 0x1
+
+#define REG_DEV_CONFIG1 0x2
+#define REG_DEV_CONFIG2 0x3
+#define REG_DEV_CONFIG3 0x4
+
+#define REG_RESET_CMD 0xe
+#define RESET_CMD 0xcc
+
+#define REG_UPDATE_CMD 0xf
+#define UPDATE_CMD 0x55
+
+#define REG_OUT0_DC 0x14
+#define REG_OUT1_DC 0x15
+#define REG_OUT2_DC 0x16
+#define REG_OUT3_DC 0x17
+#define REG_OUT0_MANUAL_PWM 0x18
+
+struct lp5816 {
+	struct led_classdev_mc mcdev;
+	struct regmap *regmap;
+	/* For register read/writes. */
+	struct mutex mtx;
+	struct device *dev;
+	struct mc_subled subleds[4];
+};
+
+static const struct regmap_config lp5816_regmap_config = {
+	.max_register = 0x40,
+	.reg_bits = 8,
+	.val_bits = 8,
+};
+
+static ssize_t max_current_show(struct device *dev,
+				struct device_attribute *attr,
+								char *buf);
+static ssize_t max_current_store(struct device *dev,
+				 struct device_attribute *attr,
+								const char *buf, size_t count);
+static DEVICE_ATTR_RW(max_current);
+
+static ssize_t fade_show(struct device *dev, struct device_attribute *attr,
+			 char *buf);
+static ssize_t fade_store(struct device *dev, struct device_attribute *attr,
+			  const char *buf, size_t count);
+static DEVICE_ATTR_RW(fade);
+
+static ssize_t fade_mode_show(struct device *dev, struct device_attribute *attr,
+			      char *buf);
+static ssize_t fade_mode_store(struct device *dev,
+			       struct device_attribute *attr, const char *buf,
+							size_t count);
+static DEVICE_ATTR_RW(fade_mode);
+
+static struct attribute *lp5816_attrs[] = {
+	&dev_attr_max_current.attr,
+	&dev_attr_fade.attr,
+	&dev_attr_fade_mode.attr,
+	NULL,
+};
+
+static struct attribute_group lp5816_group = {
+	.attrs = lp5816_attrs,
+};
+
+static int lp5816_multi_lock_write(struct lp5816 *chip, const struct reg_sequence seq[], int count)
+{
+	int res;
+
+	mutex_lock(&chip->mtx);
+	res = regmap_multi_reg_write(chip->regmap, seq, count);
+	mutex_unlock(&chip->mtx);
+	return res;
+}
+
+static int lp5816_split_string(const char *in, int out[], const int lim)
+{
+	char *tk, *step, *local;
+	int n, res;
+
+	local = kstrdup(in, GFP_KERNEL);
+	if (!local)
+		return -ENOMEM;
+	step = strim(local);
+
+	n = 0;
+	while ((tk = strsep(&step, " ")) != NULL && n < lim) {
+		if (*tk == '\0')
+			continue;
+
+		res = kstrtoint(tk, 0, &out[n]);
+		if (res < 0) {
+			kfree(local);
+			return res;
+		}
+		n++;
+	}
+
+	kfree(local);
+	return n;
+}
+
+static int lp5816_startup(struct lp5816 *chip)
+{
+	int res;
+	const struct reg_sequence sequence[] = {
+		/* Ensure that all registers are the default values. */
+		{ .reg = REG_RESET_CMD, .def = RESET_CMD },
+		/* Moves from standby to normal operating mode. */
+		{ .reg = REG_CHIP_EN, .def = CHIP_EN_ENABLE },
+		/* Sets maximum operating max_current to 51mA. */
+		{ .reg = REG_DEV_CONFIG0, .def = DEV_CONFIG0_51_MA },
+		/* Enables all four outputs. */
+		{ .reg = REG_DEV_CONFIG1, .def = 0xf },
+		/* Sets the red current to 100% of Imax (controlled by CONFIG0). */
+		{ .reg = REG_OUT0_DC, .def = 0xff },
+		/* Sets the green, .def = blue and white to 80% of Imax. */
+		{ .reg = REG_OUT1_DC, .def = 0xcc },
+		{ .reg = REG_OUT2_DC, .def = 0xcc },
+		{ .reg = REG_OUT3_DC, .def = 0xcc },
+		/* Posts the update cmd. */
+		{ .reg = REG_UPDATE_CMD, .def = UPDATE_CMD }
+	};
+
+	res = regmap_multi_reg_write(chip->regmap, sequence, ARRAY_SIZE(sequence));
+	if (res < 0)
+		return dev_err_probe(chip->dev, res, "failed to write startup sequence values\n");
+	return 0;
+}
+
+static ssize_t max_current_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *cdev;
+	struct lp5816 *chip;
+	unsigned int val;
+	int res;
+
+	cdev = dev_get_drvdata(dev);
+	chip = container_of(cdev, struct lp5816, mcdev.led_cdev);
+
+	mutex_lock(&chip->mtx);
+	res = regmap_read(chip->regmap, REG_DEV_CONFIG0, &val);
+	mutex_unlock(&chip->mtx);
+
+	if (res < 0)
+		return res;
+	if (val > 1)
+		return -EPROTO;
+
+	return sysfs_emit(buf, "%d\n", val);
+}
+
+static ssize_t max_current_store(struct device *dev,
+				 struct device_attribute *attr,
+								const char *buf, size_t count)
+{
+	struct led_classdev *cdev;
+	struct lp5816 *chip;
+	int res, val;
+
+	cdev = dev_get_drvdata(dev);
+	chip = container_of(cdev, struct lp5816, mcdev.led_cdev);
+
+	res = kstrtoint(buf, 0, &val);
+	if (res < 0)
+		return res;
+	if (val < 0 || val > 1)
+		return -EINVAL;
+
+	res = lp5816_multi_lock_write(chip, (const struct reg_sequence[]) {
+		{ .reg = REG_DEV_CONFIG0, .def = val },
+		{ .reg = REG_UPDATE_CMD, .def = UPDATE_CMD } }, 2);
+	return (res < 0) ? res : count;
+}
+
+static ssize_t fade_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *cdev;
+	struct lp5816 *chip;
+	unsigned int val;
+	int res;
+
+	cdev = dev_get_drvdata(dev);
+	chip = container_of(cdev, struct lp5816, mcdev.led_cdev);
+
+	mutex_lock(&chip->mtx);
+	res = regmap_read(chip->regmap, REG_DEV_CONFIG2, &val);
+	mutex_unlock(&chip->mtx);
+
+	if (res < 0)
+		return res;
+
+	return sysfs_emit(buf, "%d %d %d %d %d\n",
+			(val >> 0) & 0x1,
+			(val >> 1) & 0x1,
+			(val >> 2) & 0x1,
+			(val >> 3) & 0x1,
+			(val >> 4) & 0xf);
+}
+
+static ssize_t fade_store(struct device *dev, struct device_attribute *attr,
+			  const char *buf, size_t count)
+{
+	struct led_classdev *cdev;
+	struct lp5816 *chip;
+	int vals[5], i, val, res;
+
+	cdev = dev_get_drvdata(dev);
+	chip = container_of(cdev, struct lp5816, mcdev.led_cdev);
+
+	res = lp5816_split_string(buf, vals, ARRAY_SIZE(vals));
+	if (res != ARRAY_SIZE(vals))
+		return (res < 0) ? res : -EINVAL;
+
+	for (i = 0, val = 0; i < ARRAY_SIZE(vals); i++) {
+		if (i < 4) {
+			if (vals[i] < 0 || vals[i] > 1)
+				return -EINVAL;
+			val |= vals[i] << i;
+		} else {
+			if (vals[i] < 0 || vals[i] > 15)
+				return -EINVAL;
+			val |= (vals[i] & 0xf) << 4;
+		}
+	}
+
+	res = lp5816_multi_lock_write(chip, (const struct reg_sequence[]) {
+			{ .reg = REG_DEV_CONFIG2, .def = val },
+			{ .reg = REG_UPDATE_CMD, .def = UPDATE_CMD }}, 2);
+	return (res < 0) ? res : count;
+}
+
+static ssize_t fade_mode_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *cdev;
+	struct lp5816 *chip;
+	int res, val;
+
+	cdev = dev_get_drvdata(dev);
+	chip = container_of(cdev, struct lp5816, mcdev.led_cdev);
+
+	mutex_lock(&chip->mtx);
+	res = regmap_read(chip->regmap, REG_DEV_CONFIG3, &val);
+	mutex_unlock(&chip->mtx);
+
+	if (res < 0)
+		return res;
+
+	val >>= 4;
+	return sysfs_emit(buf, "%d %d %d %d\n",
+			val & 0x1,
+			(val >> 1) & 0x1,
+			(val >> 2) & 0x1,
+			(val >> 3) & 0x1);
+}
+
+static ssize_t fade_mode_store(struct device *dev,
+			       struct device_attribute *attr, const char *buf,
+							size_t count)
+{
+	struct led_classdev *cdev;
+	struct lp5816 *chip;
+	int vals[4], res, i;
+	u8 val;
+
+	cdev = dev_get_drvdata(dev);
+	chip = container_of(cdev, struct lp5816, mcdev.led_cdev);
+
+	res = lp5816_split_string(buf, vals, ARRAY_SIZE(vals));
+	if (res != ARRAY_SIZE(vals))
+		return (res < 0) ? res : -EINVAL;
+
+	/*
+	 * Requires 4 values of either 0 or 1,
+	 * which disable/enable exponential fade mode per LED.
+	 */
+	for (i = 0, val = 0; i < ARRAY_SIZE(vals); i++) {
+		if (vals[i] < 0 || vals[i] > 1)
+			return -EINVAL;
+		val |= (vals[i] << i);
+	}
+
+	res = lp5816_multi_lock_write(chip, (const struct reg_sequence[]) {
+		{ .reg = REG_DEV_CONFIG3, .def = val << 4 },
+		{ .reg = REG_UPDATE_CMD, .def = UPDATE_CMD }}, 2);
+	return (res < 0) ? res : count;
+}
+
+static enum led_brightness lp5816_brightness_get(struct led_classdev *cdev)
+{
+	struct mc_subled *leds;
+	struct lp5816 *chip;
+	int intensity;
+
+	chip = container_of(cdev, struct lp5816, mcdev.led_cdev);
+
+	mutex_lock(&chip->mtx);
+	leds = lcdev_to_mccdev(cdev)->subled_info;
+	intensity = MAX(leds[0].intensity, leds[1].intensity);
+	intensity = MAX(intensity, MAX(leds[2].intensity, leds[3].intensity));
+	mutex_unlock(&chip->mtx);
+	return intensity & 0xff;
+}
+
+static int lp5816_brightness_set(struct led_classdev *cdev, enum led_brightness br)
+{
+	struct lp5816 *chip;
+	struct led_classdev_mc *mc;
+	u8 vals[4];
+
+	mc = lcdev_to_mccdev(cdev);
+	chip = container_of(cdev, struct lp5816, mcdev.led_cdev);
+
+	vals[0] = mc->subled_info[0].intensity & 0xff;
+	vals[1] = mc->subled_info[1].intensity & 0xff;
+	vals[2] = mc->subled_info[2].intensity & 0xff;
+	vals[3] = mc->subled_info[3].intensity & 0xff;
+
+	chip = container_of(cdev, struct lp5816, mcdev.led_cdev);
+
+	mutex_lock(&chip->mtx);
+	regmap_bulk_write(chip->regmap, REG_OUT0_MANUAL_PWM, vals, ARRAY_SIZE(vals));
+	mutex_unlock(&chip->mtx);
+	return 0;
+}
+
+static int lp5816_probe(struct i2c_client *client)
+{
+	struct lp5816 *chip;
+	char *name;
+	int res;
+
+	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return dev_err_probe(&client->dev, -ENOMEM,
+			"failed to allocate lp5816 internal structure\n");
+
+	i2c_set_clientdata(client, chip);
+	mutex_init(&chip->mtx);
+
+	(*chip) = (struct lp5816) {
+		.dev = &client->dev,
+		.regmap = devm_regmap_init_i2c(client, &lp5816_regmap_config),
+		.subleds = {
+			{ .color_index = LED_COLOR_ID_RED, .intensity = 0 },
+			{ .color_index = LED_COLOR_ID_GREEN, .intensity = 0 },
+			{ .color_index = LED_COLOR_ID_BLUE, .intensity = 0 },
+			{ .color_index = LED_COLOR_ID_WHITE, .intensity = 0 },
+		},
+	};
+
+	if (IS_ERR(chip->regmap))
+		return dev_err_probe(&client->dev, PTR_ERR(chip->regmap),
+				"failed to allocate register map\n");
+
+	name = devm_kasprintf(&client->dev, GFP_KERNEL, "%s:rgbw", dev_name(&client->dev));
+	if (!name)
+		return dev_err_probe(&client->dev, -ENOMEM,
+				"failed to allocate led-classdev name\n");
+
+	chip->mcdev = (struct led_classdev_mc){
+		.led_cdev = (struct led_classdev){
+			.brightness_set_blocking = lp5816_brightness_set,
+			.brightness_get = lp5816_brightness_get,
+			.name = name,
+		},
+		.num_colors = ARRAY_SIZE(chip->subleds),
+		.subled_info = chip->subleds,
+	};
+
+	res = devm_led_classdev_multicolor_register(&client->dev, &chip->mcdev);
+	if (res < 0)
+		return dev_err_probe(&client->dev, res, "failed to register classdev multicolor\n");
+
+	res = devm_device_add_group(chip->mcdev.led_cdev.dev, &lp5816_group);
+	if (res < 0)
+		return dev_err_probe(&client->dev, res, "failed to add led device group\n");
+
+	res = lp5816_startup(chip);
+	if (res < 0)
+		return dev_err_probe(&client->dev, res,
+				"failed to execute chip startup procedure\n");
+	return 0;
+}
+
+static void lp5816_remove(struct i2c_client *client)
+{
+	struct lp5816 *chip;
+
+	chip = i2c_get_clientdata(client);
+	if (!chip)
+		return;
+
+	mutex_lock(&chip->mtx);
+	regmap_multi_reg_write(chip->regmap, (const struct reg_sequence[]) {
+		{ .reg = REG_RESET_CMD, .def = RESET_CMD },
+		{ .reg = REG_UPDATE_CMD, .def = UPDATE_CMD }}, 2);
+	mutex_unlock(&chip->mtx);
+}
+
+static const struct of_device_id lp5816_of_match[] = {
+	{ .compatible = "ti,lp5816" },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(of, lp5816_of_match);
+
+static struct i2c_driver lp5816_driver = {
+	.driver = {
+		.name = "leds-lp5816",
+		.of_match_table = lp5816_of_match,
+	},
+	.probe = lp5816_probe,
+	.remove = lp5816_remove,
+};
+
+module_i2c_driver(lp5816_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Alistair Bell <dev@alistairbell.org>");
+MODULE_DESCRIPTION("TI LP5816 LED driver");
-- 
2.54.0

Re: [PATCH 1/2] leds: Texas Instruments LP5816 driver

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [Critical] The driver overwrites its initialized mutex with zeroes via a struct compound literal assignment.
- [High] The `lp5816_brightness_set` function ignores the requested global brightness and writes unscaled relative intensities.
- [High] The LED classdev is registered and sysfs groups are added before hardware startup, creating initialization races.
- [High] The `lp5816_brightness_set()` function fails to latch updated PWM values because it omits the required `UPDATE_CMD`.
- [Medium] `lp5816_brightness_get` incorrectly calculates and returns the maximum relative color intensity instead of the global brightness.
- [Medium] Hardware is reset in `.remove()` while the `devm_`-managed LED classdev is still active, causing a teardown race.
- [Low] The driver hardcodes the LED device name, violating standard Device Tree naming guidelines.
- [Low] The commit message prefix does not follow the standard naming convention for the LED subsystem.
--

--- Patch [1]: [PATCH 1/2] leds: Texas Instruments LP5816 driver ---
commit e41d9f624b4be8a22f82da1e0e301eeb37f92003
Author: Alistair Bell <dev@alistairbell.org>
leds: Texas Instruments LP5816 driver

This patch adds support for the Texas Instruments LP5816 4-channel I2C RGBW
LED driver. It provides independent 4-channel control via the multicolor
sysfs class, as well as configurable fade effects and current limits.

> leds: Texas Instruments LP5816 driver

[Severity: Low]
This isn't a bug, but does this commit message prefix follow the standard
naming convention for the LED subsystem? The guidelines usually mandate a
capitalized format like leds: lp5816: Add support for ...

> diff --git a/drivers/leds/leds-lp5816.c b/drivers/leds/leds-lp5816.c
> new file mode 100644
> index 0000000000000..584c348a7fdb2
> --- /dev/null
> +++ b/drivers/leds/leds-lp5816.c
[ ... ]
> +static enum led_brightness lp5816_brightness_get(struct led_classdev *cdev)
> +{
> +	struct mc_subled *leds;
> +	struct lp5816 *chip;
> +	int intensity;
> +
> +	chip = container_of(cdev, struct lp5816, mcdev.led_cdev);
> +
> +	mutex_lock(&chip->mtx);
> +	leds = lcdev_to_mccdev(cdev)->subled_info;
> +	intensity = MAX(leds[0].intensity, leds[1].intensity);
> +	intensity = MAX(intensity, MAX(leds[2].intensity, leds[3].intensity));
> +	mutex_unlock(&chip->mtx);
> +	return intensity & 0xff;
> +}

[Severity: Medium]
Does this return the actual global LED brightness state? It appears to
calculate and return the maximum of the relative sub-LED color weights
instead of returning the overall brightness level.

> +static int lp5816_brightness_set(struct led_classdev *cdev, enum led_brightness br)
> +{
> +	struct lp5816 *chip;
> +	struct led_classdev_mc *mc;
> +	u8 vals[4];
> +
> +	mc = lcdev_to_mccdev(cdev);
> +	chip = container_of(cdev, struct lp5816, mcdev.led_cdev);
> +
> +	vals[0] = mc->subled_info[0].intensity & 0xff;
> +	vals[1] = mc->subled_info[1].intensity & 0xff;
> +	vals[2] = mc->subled_info[2].intensity & 0xff;
> +	vals[3] = mc->subled_info[3].intensity & 0xff;
> +
> +	chip = container_of(cdev, struct lp5816, mcdev.led_cdev);
> +
> +	mutex_lock(&chip->mtx);
> +	regmap_bulk_write(chip->regmap, REG_OUT0_MANUAL_PWM, vals, ARRAY_SIZE(vals));
> +	mutex_unlock(&chip->mtx);
> +	return 0;
> +}

[Severity: High]
Does lp5816_brightness_set() ignore the requested global brightness
parameter br? It looks like it assigns the unscaled relative intensity
weights directly instead of calling led_mc_calc_color_components() to
compute the correct brightness values.

[Severity: High]
Are these updated PWM values actually applied to the hardware output?
It appears this function writes to the holding registers but does not send
the UPDATE_CMD to REG_UPDATE_CMD to latch the changes, which might leave
the hardware output entirely unchanged.

> +static int lp5816_probe(struct i2c_client *client)
> +{
> +	struct lp5816 *chip;
> +	char *name;
> +	int res;
> +
> +	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
> +	if (!chip)
> +		return dev_err_probe(&client->dev, -ENOMEM,
> +			"failed to allocate lp5816 internal structure\n");
> +
> +	i2c_set_clientdata(client, chip);
> +	mutex_init(&chip->mtx);
> +
> +	(*chip) = (struct lp5816) {
> +		.dev = &client->dev,
> +		.regmap = devm_regmap_init_i2c(client, &lp5816_regmap_config),
> +		.subleds = {

[Severity: Critical]
Does this compound literal assignment overwrite the mutex that was just
initialized? Since the assignment explicitly zeroes out all unlisted fields
in the struct, it seems this will wipe out the mutex state and could lead
to a panic or lockdep warning on the first attempt to acquire chip->mtx.

[ ... ]
> +	if (IS_ERR(chip->regmap))
> +		return dev_err_probe(&client->dev, PTR_ERR(chip->regmap),
> +				"failed to allocate register map\n");
> +
> +	name = devm_kasprintf(&client->dev, GFP_KERNEL, "%s:rgbw", dev_name(&client->dev));

[Severity: Low]
Is hardcoding the LED device name to end in :rgbw recommended here?
This might prevent standard userspace tools from correctly identifying the
LED by parsing Device Tree color and function properties per the LED
subsystem rules.

> +	if (!name)
> +		return dev_err_probe(&client->dev, -ENOMEM,
> +				"failed to allocate led-classdev name\n");
> +
> +	chip->mcdev = (struct led_classdev_mc){
> +		.led_cdev = (struct led_classdev){
> +			.brightness_set_blocking = lp5816_brightness_set,
> +			.brightness_get = lp5816_brightness_get,
> +			.name = name,
> +		},
> +		.num_colors = ARRAY_SIZE(chip->subleds),
> +		.subled_info = chip->subleds,
> +	};
> +
> +	res = devm_led_classdev_multicolor_register(&client->dev, &chip->mcdev);
> +	if (res < 0)
> +		return dev_err_probe(&client->dev, res, "failed to register classdev multicolor\n");
> +
> +	res = devm_device_add_group(chip->mcdev.led_cdev.dev, &lp5816_group);
> +	if (res < 0)
> +		return dev_err_probe(&client->dev, res, "failed to add led device group\n");
> +
> +	res = lp5816_startup(chip);

[Severity: High]
Could this registration sequence cause initialization races?
By registering the LED classdev and firing uevents before adding the custom
sysfs groups and before the hardware is actually powered on in
lp5816_startup(), udev might miss the custom groups. Additionally, any
immediate userspace writes could be wiped out when lp5816_startup()
subsequently issues a hard reset.

> +	if (res < 0)
> +		return dev_err_probe(&client->dev, res,
> +				"failed to execute chip startup procedure\n");
> +	return 0;
> +}
> +
> +static void lp5816_remove(struct i2c_client *client)
> +{
> +	struct lp5816 *chip;
> +
> +	chip = i2c_get_clientdata(client);
> +	if (!chip)
> +		return;
> +
> +	mutex_lock(&chip->mtx);
> +	regmap_multi_reg_write(chip->regmap, (const struct reg_sequence[]) {
> +		{ .reg = REG_RESET_CMD, .def = RESET_CMD },
> +		{ .reg = REG_UPDATE_CMD, .def = UPDATE_CMD }}, 2);
> +	mutex_unlock(&chip->mtx);
> +}

[Severity: Medium]
Can this manual hardware reset race with the managed teardown?
Because the LED class device is registered via devres, it is still active
when lp5816_remove() resets the chip. The LED core might attempt to set the
brightness to 0 during its unregistration phase later, writing to a reset
and sleeping chip.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260712184318.78852-1-dev@alistairbell.org?part=1

Re: [PATCH 1/2] leds: Texas Instruments LP5816 driver

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 250 bytes --]

Hello Alistair,

On Sun, Jul 12, 2026 at 07:43:17PM +0100, Alistair Bell wrote:
> +#include <linux/mod_devicetable.h>

Please don't add new users for this header file. Only use those
<linux/device-id/*.h> that you actually need (if any).

Thanks
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] leds: Texas Instruments LP5816 driver

[Krzysztof Kozlowski]

On 12/07/2026 20:43, Alistair Bell wrote:
> Add support for Texas Instruments LP5816 4-channel I2C device,
> the driver provides:
> 
> - Independent 4-channel control via the multicolor sysfs class
> - Configurable fade effects, duration, fade profile and maximum
>   operating current via sysfs
> 
> Signed-off-by: Alistair Bell <dev@alistairbell.org>
> ---
>  MAINTAINERS                |   8 +
>  drivers/leds/Kconfig       |  12 +
>  drivers/leds/Makefile      |   1 +
>  drivers/leds/leds-lp5816.c | 443 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 464 insertions(+)
>  create mode 100644 drivers/leds/leds-lp5816.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f37a81950..b6a8c812a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -26761,6 +26761,14 @@ F:	drivers/leds/rgb/Makefile
>  F:	drivers/leds/rgb/leds-lp5812.c
>  F:	drivers/leds/rgb/leds-lp5812.h
>  
> +TEXAS INSTRUMENTS' LP5816 RGBW LED DRIVER
> +M:	Alistair Bell <dev@alistairbell.org>
> +L:	linux-leds@vger.kernel.org
> +S:	Maintained
> +F:	drivers/leds/Kconfig
> +F:	drivers/leds/Makefile
> +F:	drivers/leds/leds-lp5816.c
> +
>  TEXAS INSTRUMENTS' LB8864 LED BACKLIGHT DRIVER
>  M:	Alexander Sverdlin <alexander.sverdlin@siemens.com>
>  L:	linux-leds@vger.kernel.org
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index f4a0a3c8c..ce3776adf 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -541,6 +541,18 @@ config LEDS_LP8864
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called leds-lp8864.
>  
> +config LEDS_LP5816
> +    tristate "LED driver for LP5816 chip"
> +    depends on I2C
> +    depends on LEDS_CLASS && LEDS_CLASS_MULTICOLOR
> +	select REGMAP_I2C
> +    help
> +      Say Y to enable support for the Texas Instruments LP5816
> +      RGBW LED connected via I2C.
> +
> +      To compile this driver as a module, choose M here:
> +      the module will be called lp5816.

Messed indentation. Please be sure you are sending consistent code, not
something written completely different than the rest.

> +
>  config LEDS_CLEVO_MAIL
>  	tristate "Mail LED on Clevo notebook"
>  	depends on LEDS_CLASS && BROKEN
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 7db376891..5ba7de099 100644
> --- a/drivers/leds/Makefile


...

> +
> +static ssize_t max_current_store(struct device *dev,
> +				 struct device_attribute *attr,
> +								const char *buf, size_t count)
> +{
> +	struct led_classdev *cdev;
> +	struct lp5816 *chip;
> +	int res, val;
> +
> +	cdev = dev_get_drvdata(dev);
> +	chip = container_of(cdev, struct lp5816, mcdev.led_cdev);
> +
> +	res = kstrtoint(buf, 0, &val);
> +	if (res < 0)
> +		return res;
> +	if (val < 0 || val > 1)
> +		return -EINVAL;
> +
> +	res = lp5816_multi_lock_write(chip, (const struct reg_sequence[]) {
> +		{ .reg = REG_DEV_CONFIG0, .def = val },
> +		{ .reg = REG_UPDATE_CMD, .def = UPDATE_CMD } }, 2);
> +	return (res < 0) ? res : count;
> +}


You cannot introduce own ABI duplicating existing sysfs interface.


> +
> +static int lp5816_probe(struct i2c_client *client)
> +{
> +	struct lp5816 *chip;
> +	char *name;
> +	int res;
> +
> +	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
> +	if (!chip)
> +		return dev_err_probe(&client->dev, -ENOMEM,
> +			"failed to allocate lp5816 internal structure\n");

Since when any driver has such error message?

Please look at most recently addedd and reviewed drivers and learn from
them how typical code looks like.


Best regards,
Krzysztof

[PATCH 2/2] dt-bindings: leds: Add binding for ti,lp5816

[Alistair Bell]

Add devicetree binding for Texas Instruments LP5816
4-channel I2C LED device.

Signed-off-by: Alistair Bell <dev@alistairbell.org>
---
 .../ABI/testing/sysfs-class-leds-lp5816       | 123 ++++++++++++++++++
 .../devicetree/bindings/leds/ti,lp5816.yaml   |  45 +++++++
 MAINTAINERS                                   |   2 +
 3 files changed, 170 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-leds-lp5816
 create mode 100644 Documentation/devicetree/bindings/leds/ti,lp5816.yaml

diff --git a/Documentation/ABI/testing/sysfs-class-leds-lp5816 b/Documentation/ABI/testing/sysfs-class-leds-lp5816
new file mode 100644
index 000000000..2d1fd107a
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-leds-lp5816
@@ -0,0 +1,123 @@
+What: /sys/class/leds/<device>/fade
+Date: July 2026
+KernelVersion: 7.2
+Contact: <dev@alistairbell.org>
+Description:
+	Enables/disables hardware fading of each individual LED and sets transition duration
+	between an old intensity and new intensity
+
+	Format: <enable_red> <enable_green> <enable_blue> <enable_white> <duration>
+
+	Possible values for <enable> are:
+
+	== =======
+	0  disable
+	1  enable
+	== =======
+
+	Disable: The LED intensity will change instantaneous
+	Enable: The LED intensity will transition between the old and new value either
+	linearly or exponentially based on their individual fade_mode over a period
+	specified by <duration>
+
+	Possible values for <duration> are:
+
+	==    =====
+	0     0.00s
+	1     0.05s
+	2     0.10s
+	3     0.15s
+	4     0.20s
+	5     0.25s
+	6     0.30s
+	7     0.35s
+	8     0.40s
+	9     0.45s
+	10    0.50s
+	12    2.00s
+	13    4.00s
+	14    6.00s
+	15    8.00s
+	==    =====
+
+	Reading this attribute returns the current fade configuration in the same
+	format as writing to it
+
+	Example:
+
+		$ echo 0 1 0 1 14 > /sys/class/leds/<device>/fade
+
+		Enables fade for the green and white channels whilst disabling red and blue.
+		Sets the duration to 6.0s
+
+		$ cat /sys/class/leds/<device>/fade
+		0 1 0 1 14
+
+		$ echo 0 1 0 0 > /sys/class/leds/<device>/multi_intensity
+		If the green LED is not already at full intensity you should notice it slowly getting
+		brighter until reaching 100% brightness after 6.0 seconds
+
+	Notes:
+		By default the driver initialises all fade modes to be disabled and a duration
+		of 0.0s
+
+
+What: /sys/class/leds/<device>/fade_mode
+Date: July 2026
+KernelVersion: 7.2
+Contact: <dev@alistairbell.org>
+Description:
+	Sets the fade transition of each individual LED to either linear or exponential
+
+	Format: <mode_red> <mode_green> <mode_blue> <mode_white>
+
+	Possible values for <mode_> are:
+
+	== ===========
+	0  linear
+	1  exponential
+	== ===========
+
+	Linear:
+
+	100% |           x
+	 80% |         x
+	 60% |       x
+	 40% |     x
+	 20% |   x
+	  0% |_____________   Time
+		 0 1 2 3 ... D
+
+	Constant increase/decrease in LED intensity between 0.0s and <duration>
+
+	Exponential:
+
+	100% |
+	 80% |          x
+	 60% |         x
+	 40% |        x
+	 20% |  x x x
+	  0% |_____________   Time
+		 0 1 2 3 ... D
+
+	Initially only a small visible change from the initial intensity then
+	will appear to quickly and smoothly transition to the brightness target
+
+	Example:
+		$ cat /sys/class/leds/<device>/fade_mode
+		0 0 0 0
+
+		Notice the default configuration is all LEDs in linear mode
+
+		$ echo 1 1 0 0 > /sys/class/leds/<device>/fade_mode
+
+		Sets red and green to use exponential mode and blue and white to linear
+
+		$ cat /sys/class/leds/<device>/fade_mode
+		1 1 0 0
+
+		Configuration stored and returned in the same format as was written to
+
+	Note:
+		If /sys/class/leds/<device>/fade is disabled on a specific color this configuration will
+		be saved for when fading is enabled
diff --git a/Documentation/devicetree/bindings/leds/ti,lp5816.yaml b/Documentation/devicetree/bindings/leds/ti,lp5816.yaml
new file mode 100644
index 000000000..d33489c30
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/ti,lp5816.yaml
@@ -0,0 +1,45 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/ti,lp5816.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Texas Instruments LP5816 4-Channel I2C RGBW LED Driver
+
+maintainers:
+  - Alistair Bell <dev@alistairbell.org>
+
+description: |
+  The LP5816 is a 4-channel I2C interface RGBW LED driver from Texas Instruments.
+  It supports independent control of red, green, blue, and white leds with
+  configurable fade effects and dimming.
+
+properties:
+  compatible:
+    const: ti,lp5816
+    description: |
+      Specifies device as LP5816 and the uses that kernel driver
+
+
+  reg:
+    maxItems: 1
+    description: I2C slave address
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+      compatible = "vendor,i2c-controller";
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      led-controller@2c {
+        compatible = "ti,lp5816";
+        reg = <0x2c>;
+      };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index b6a8c812a..5835f929d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -26765,6 +26765,8 @@ TEXAS INSTRUMENTS' LP5816 RGBW LED DRIVER
 M:	Alistair Bell <dev@alistairbell.org>
 L:	linux-leds@vger.kernel.org
 S:	Maintained
+F:	Documentation/ABI/testing/sysfs-class-leds-lp5816
+F:	Documentation/devicetree/bindings/leds/ti,lp5816.yaml
 F:	drivers/leds/Kconfig
 F:	drivers/leds/Makefile
 F:	drivers/leds/leds-lp5816.c
-- 
2.54.0

Re: [PATCH 2/2] dt-bindings: leds: Add binding for ti,lp5816

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [High] The driver overwrites the initialized mutex with zeroes during a compound struct assignment, causing an inevitable crash on first use.
- [High] The driver ignores the master brightness parameter and fails to use the multi-color calculation helper.
- [High] The driver resets the hardware in `remove()` before the LED is unregistered, causing a use-after-disable window.
- [High] Missing register update command in brightness setter prevents hardware from applying new values.
- [Medium] The commit introduces custom, device-specific sysfs attributes (`fade` and `fade_mode`) that hardcode specific RGBW colors, bypassing standard LED subsystem mechanisms.
- [Medium] The device tree binding fails to define standard LED child nodes and omits common standard LED properties, breaking hardware description principles.
- [Medium] The device is exposed to user-space before its hardware is initialized.
- [Low] The device tree binding description explicitly references the operating system's software driver.
- [Low] Use of undeclared identifier `MAX` causes a compilation error.
- [Low] The patch commit message violates the LED subsystem naming convention.
--

--- Patch [2]: [PATCH 2/2] dt-bindings: leds: Add binding for ti,lp5816 ---
commit b91ef8e4c561a1f06d4c4b39a848e64275caccbb
Author: Alistair Bell <dev@alistairbell.org>

dt-bindings: leds: Add binding for ti,lp5816

Add devicetree binding for Texas Instruments LP5816
4-channel I2C LED device.

> dt-bindings: leds: Add binding for ti,lp5816

[Severity: Low]
Does this commit message subject violate the LED subsystem naming convention?
The subsystem guidelines expect the prefix to follow the format:
leds: <Driver>: <Capitalized description>.

> diff --git a/Documentation/ABI/testing/sysfs-class-leds-lp5816 b/Documentation/ABI/testing/sysfs-class-leds-lp5816
> new file mode 100644
> index 0000000000000..2d1fd107abf67
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-leds-lp5816
> @@ -0,0 +1,123 @@
> +What: /sys/class/leds/<device>/fade
> +Date: July 2026
> +KernelVersion: 7.2
> +Contact: <dev@alistairbell.org>
> +Description:
> +	Enables/disables hardware fading of each individual LED and sets transition duration
> +	between an old intensity and new intensity
> +
> +	Format: <enable_red> <enable_green> <enable_blue> <enable_white> <duration>

[Severity: Medium]
Does introducing these custom, device-specific sysfs attributes that hardcode
specific RGBW colors bypass the standard LED subsystem mechanisms?
This forces user-space tools to implement custom string parsing for this
specific chip.

> diff --git a/Documentation/devicetree/bindings/leds/ti,lp5816.yaml b/Documentation/devicetree/bindings/leds/ti,lp5816.yaml
> new file mode 100644
> index 0000000000000..d33489c306dfc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/ti,lp5816.yaml
[ ... ]
> +properties:
> +  compatible:
> +    const: ti,lp5816
> +    description: |
> +      Specifies device as LP5816 and the uses that kernel driver

[Severity: Low]
Does this description improperly reference the operating system's software
driver?
Device tree bindings should be OS-agnostic hardware descriptions.

[ ... ]
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false

[Severity: Medium]
Does this schema fail to define standard LED child nodes?
By dropping child node specifications and omitting common standard LED
properties, a device tree attempting to define standard LED properties
like color or function will fail validation.

> diff --git a/drivers/leds/leds-lp5816.c b/drivers/leds/leds-lp5816.c
> new file mode 100644
> index 0000000000000..584c348a7fdb2
> --- /dev/null
> +++ b/drivers/leds/leds-lp5816.c
[ ... ]
> +static enum led_brightness lp5816_brightness_get(struct led_classdev *cdev)
> +{
> +	struct mc_subled *leds;
> +	struct lp5816 *chip;
> +	int intensity;
> +
> +	chip = container_of(cdev, struct lp5816, mcdev.led_cdev);
> +
> +	mutex_lock(&chip->mtx);
> +	leds = lcdev_to_mccdev(cdev)->subled_info;
> +	intensity = MAX(leds[0].intensity, leds[1].intensity);

[Severity: Low]
Will this result in a compilation error?
The uppercase MAX macro is undeclared; the standard kernel macro for maximum
values is lowercase max().

[ ... ]
> +static int lp5816_brightness_set(struct led_classdev *cdev, enum led_brightness br)
> +{
> +	struct lp5816 *chip;
> +	struct led_classdev_mc *mc;
> +	u8 vals[4];
> +
> +	mc = lcdev_to_mccdev(cdev);
> +	chip = container_of(cdev, struct lp5816, mcdev.led_cdev);
> +
> +	vals[0] = mc->subled_info[0].intensity & 0xff;
> +	vals[1] = mc->subled_info[1].intensity & 0xff;
> +	vals[2] = mc->subled_info[2].intensity & 0xff;
> +	vals[3] = mc->subled_info[3].intensity & 0xff;

[Severity: High]
Does this ignore the master brightness parameter?
The core functionality of setting the LED brightness appears broken because
the driver does not use led_mc_calc_color_components(mc, br) and ignores the
requested master brightness, making it impossible to turn off the LED via the
standard API.

> +
> +	chip = container_of(cdev, struct lp5816, mcdev.led_cdev);
> +
> +	mutex_lock(&chip->mtx);
> +	regmap_bulk_write(chip->regmap, REG_OUT0_MANUAL_PWM, vals, ARRAY_SIZE(vals));

[Severity: High]
Are we missing the register update command here?
Without issuing a REG_UPDATE_CMD after the bulk write, the brightness changes
requested via the standard API will fail to take effect on the hardware.

> +	mutex_unlock(&chip->mtx);
> +	return 0;
> +}
[ ... ]
> +static int lp5816_probe(struct i2c_client *client)
> +{
> +	struct lp5816 *chip;
> +	char *name;
> +	int res;
> +
> +	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
> +	if (!chip)
> +		return dev_err_probe(&client->dev, -ENOMEM,
> +			"failed to allocate lp5816 internal structure\n");
> +
> +	i2c_set_clientdata(client, chip);
> +	mutex_init(&chip->mtx);
> +
> +	(*chip) = (struct lp5816) {

[Severity: High]
Will this compound struct assignment overwrite the initialized mutex?
Standard C semantics zero-initialize omitted members in a compound literal,
which will wipe the internal state of chip->mtx and cause a kernel oops or
deadlock on the first mutex_lock() call.

> +		.dev = &client->dev,
> +		.regmap = devm_regmap_init_i2c(client, &lp5816_regmap_config),
> +		.subleds = {
> +			{ .color_index = LED_COLOR_ID_RED, .intensity = 0 },
> +			{ .color_index = LED_COLOR_ID_GREEN, .intensity = 0 },
> +			{ .color_index = LED_COLOR_ID_BLUE, .intensity = 0 },
> +			{ .color_index = LED_COLOR_ID_WHITE, .intensity = 0 },
> +		},
> +	};
[ ... ]
> +	res = devm_led_classdev_multicolor_register(&client->dev, &chip->mcdev);
> +	if (res < 0)
> +		return dev_err_probe(&client->dev, res, "failed to register classdev multicolor\n");
> +
> +	res = devm_device_add_group(chip->mcdev.led_cdev.dev, &lp5816_group);
> +	if (res < 0)
> +		return dev_err_probe(&client->dev, res, "failed to add led device group\n");
> +
> +	res = lp5816_startup(chip);

[Severity: Medium]
Is it safe to expose the device to user-space before its hardware is initialized?
If udev or user-space accesses the device after devm_led_classdev_multicolor_register()
but before lp5816_startup() finishes, the hardware reset in the startup sequence
could destroy that state.

> +	if (res < 0)
> +		return dev_err_probe(&client->dev, res,
> +				"failed to execute chip startup procedure\n");
> +	return 0;
> +}
> +
> +static void lp5816_remove(struct i2c_client *client)
> +{
> +	struct lp5816 *chip;
> +
> +	chip = i2c_get_clientdata(client);
> +	if (!chip)
> +		return;
> +
> +	mutex_lock(&chip->mtx);
> +	regmap_multi_reg_write(chip->regmap, (const struct reg_sequence[]) {
> +		{ .reg = REG_RESET_CMD, .def = RESET_CMD },
> +		{ .reg = REG_UPDATE_CMD, .def = UPDATE_CMD }}, 2);

[Severity: High]
Does resetting the hardware in remove() before the LED is unregistered cause a
use-after-disable window?

Because the LED class device is registered using a devres API, it will be
unregistered automatically after remove() returns. That automatic cleanup calls
led_set_brightness() to turn off the LED, which will write I2C commands to a
device that has already been reset.

> +	mutex_unlock(&chip->mtx);
> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260712184318.78852-1-dev@alistairbell.org?part=2

Re: [PATCH 2/2] dt-bindings: leds: Add binding for ti,lp5816

[Krzysztof Kozlowski]

On 12/07/2026 20:43, Alistair Bell wrote:
> Add devicetree binding for Texas Instruments LP5816
> 4-channel I2C LED device.
> 
> Signed-off-by: Alistair Bell <dev@alistairbell.org>

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline) or work on fork of kernel
(don't, instead use mainline). Just use b4 and everything should be
fine, although remember about `b4 prep --auto-to-cc` if you added new
patches to the patchset.

You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling. Performing review on untested code might be
a waste of time.

Please kindly resend and include all necessary To/Cc entries.


Please run scripts/checkpatch.pl on the patches and fix reported
warnings. After that, run also 'scripts/checkpatch.pl --strict' on the
patches and (probably) fix more warnings. Some warnings can be ignored,
especially from --strict run, but the code here looks like it needs a
fix. Feel free to get in touch if the warning is not clear.


A nit, subject: drop second/last, redundant "bindings for". The
"dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v7.1-rc7/source/Documentation/devicetree/bindings/submitting-patches.rst#L23

Please organize the patch documenting the compatible (DT bindings)
before the patch using that compatible.
See also:
https://elixir.bootlin.com/linux/v6.14-rc6/source/Documentation/devicetree/bindings/submitting-patches.rst#L46


Best regards,
Krzysztof