[PATCH v2 0/2] LED: Add fault state handling to LP5860 LED

[PATCH v2 0/2] LED: Add fault state handling to LP5860 LED

[Steffen Trumtrar]

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
Changes in v2:
- make some functions/defines clearer
- minor newline cleanups
- split sysfs patch into own patch
- fix scope of some variables
- Link to v1: https://lore.kernel.org/r/20260129-v6-19-topic-ti-lp5860-fault-v1-1-49c812dc06da@pengutronix.de

---
Steffen Trumtrar (2):
      led: lp5860: expose fault state via sysfs
      leds: lp5860: detect and report fault state

 Documentation/ABI/testing/sysfs-class-spi-lp5860 |  49 ++++++++
 drivers/leds/rgb/leds-lp5860-core.c              | 143 +++++++++++++++++++++++
 include/linux/platform_data/leds-lp5860.h        |  19 ++-
 3 files changed, 208 insertions(+), 3 deletions(-)
---
base-commit: 6de23f81a5e08be8fbf5e8d7e9febc72a5b5f27f
change-id: 20260129-v6-19-topic-ti-lp5860-fault-c8bd1f59fc3f
prerequisite-message-id: <20260309-v6-14-topic-ti-lp5860-v7-1-b1ed6c6a47ce@pengutronix.de>
prerequisite-patch-id: 45e295aab0d3ea7d92bf71596f8b0e18e8621ac0

Best regards,
-- 
Steffen Trumtrar <s.trumtrar@pengutronix.de>

[PATCH v2 1/2] led: lp5860: expose fault state via sysfs

[Steffen Trumtrar]

Return the fault state to the userspase via sysfs and allow to reset it.

The LP5860 has a global fault state, that just indicates that a short or
open fault was detected on any LED. This is exposed via 'fault_state'.

The 'fault_state_open' exposes the LED name and channel where an open
condition was detected.

The 'fault_state_short' exposes the LED name and channel where a short
condition was detected.

To: Mark Brown <broonie@kernel.org>
Cc: linux-spi@vger.kernel.org
Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
 Documentation/ABI/testing/sysfs-class-spi-lp5860 | 49 ++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-class-spi-lp5860 b/Documentation/ABI/testing/sysfs-class-spi-lp5860
new file mode 100644
index 0000000000000..31082bd78f51e
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-spi-lp5860
@@ -0,0 +1,49 @@
+What:           /sys/class/spi_master/spi<bus>/spi<bus>.<dev>/fault_state
+Date:           March 2026
+KernelVersion:  7.0
+Contact:        Steffen Trumtrar <kernel@pengutronix.de>
+Description:
+	Contains and sets the global fault state:
+
+	* 3: Open and short detected
+	* 2: Open detected
+	* 1: Short detected
+
+	Can be cleared by writing the corresponding value back to fault_state.
+
+	Example usage::
+
+		## Read
+		# cat /sys/class/spi_master/spi<bus>/spi<bus>.<dev>/fault_state
+		2
+
+		## Write
+		# echo 2 > /sys/class/spi_master/spi<bus>/spi<bus>.<dev>/fault_state
+
+What:           /sys/class/spi_master/spi<bus>/spi<bus>.<dev>/fault_state_open
+Date:           March 2026
+KernelVersion:  7.0
+Contact:        Steffen Trumtrar <kernel@pengutronix.de>
+Description:
+	Contains all LEDs and channels where an open condition was detected.
+	The format is ledname:channel.
+
+	Example usage::
+
+		## Read
+		# cat /sys/class/spi_master/spi<bus>/spi<bus>.<dev>/fault_state_open
+		rgb1:0 rgb2:4
+
+What:           /sys/class/spi_master/spi<bus>/spi<bus>.<dev>/fault_state_short
+Date:           March 2026
+KernelVersion:  7.0
+Contact:        Steffen Trumtrar <kernel@pengutronix.de>
+Description:
+	Contains all LEDs and channels where a short condition was detected.
+	The format is ledname:channel.
+
+	Example usage::
+
+		## Read
+		# cat /sys/class/spi_master/spi<bus>/spi<bus>.<dev>/fault_state_short
+		rgb1:0 rgb2:4

-- 
2.51.0

Re: [PATCH v2 1/2] led: lp5860: expose fault state via sysfs

[Lee Jones]

On Wed, 11 Mar 2026, Steffen Trumtrar wrote:

> Return the fault state to the userspase via sysfs and allow to reset it.

"userspace" is misspelled here and in the subject line.

> 
> The LP5860 has a global fault state, that just indicates that a short or
> open fault was detected on any LED. This is exposed via 'fault_state'.
> 
> The 'fault_state_open' exposes the LED name and channel where an open
> condition was detected.
> 
> The 'fault_state_short' exposes the LED name and channel where a short
> condition was detected.
> 
> To: Mark Brown <broonie@kernel.org>
> Cc: linux-spi@vger.kernel.org

These should be below the --- marker.

> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> ---
>  Documentation/ABI/testing/sysfs-class-spi-lp5860 | 49 ++++++++++++++++++++++++

You're the first write driver documentation like this.

That has to tell you something.

>  1 file changed, 49 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-spi-lp5860 b/Documentation/ABI/testing/sysfs-class-spi-lp5860
> new file mode 100644
> index 0000000000000..31082bd78f51e
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-spi-lp5860
> @@ -0,0 +1,49 @@
> +What:           /sys/class/spi_master/spi<bus>/spi<bus>.<dev>/fault_state

I believe this path is incorrect. Attributes for an SPI slave device should
be under `/sys/bus/spi/devices/spi<bus>.<dev>/`, not under the master
device's class directory.

> +Date:           March 2026
> +KernelVersion:  7.0
> +Contact:        Steffen Trumtrar <kernel@pengutronix.de>

This is different to your sign-off address.

> +Description:
> +	Contains and sets the global fault state:
> +
> +	* 3: Open and short detected
> +	* 2: Open detected
> +	* 1: Short detected

Exposing a raw bitmask like this is not a good ABI. The sysfs convention is
"one value per file". It would be better to have separate read-only files
like `fault_short` and `fault_open` which would contain "1" if a fault is
active and "0" otherwise.

> +
> +	Can be cleared by writing the corresponding value back to fault_state.

This "write back what you read" mechanism is non-standard and racy. A
better approach is to provide a separate write-only `fault_clear` file, or
allow writing '0' to the individual fault files to clear them.

> +
> +	Example usage::
> +
> +		## Read
> +		# cat /sys/class/spi_master/spi<bus>/spi<bus>.<dev>/fault_state
> +		2
> +
> +		## Write
> +		# echo 2 > /sys/class/spi_master/spi<bus>/spi<bus>.<dev>/fault_state
> +
> +What:           /sys/class/spi_master/spi<bus>/spi<bus>.<dev>/fault_state_open
> +Date:           March 2026
> +KernelVersion:  7.0
> +Contact:        Steffen Trumtrar <kernel@pengutronix.de>
> +Description:
> +	Contains all LEDs and channels where an open condition was detected.

I'm also really confused by the cross-over here.

Are we documenting SPI behaviour or LED?

> +	The format is ledname:channel.
> +
> +	Example usage::
> +
> +		## Read
> +		# cat /sys/class/spi_master/spi<bus>/spi<bus>.<dev>/fault_state_open
> +		rgb1:0 rgb2:4
> +
> +What:           /sys/class/spi_master/spi<bus>/spi<bus>.<dev>/fault_state_short
> +Date:           March 2026
> +KernelVersion:  7.0
> +Contact:        Steffen Trumtrar <kernel@pengutronix.de>
> +Description:
> +	Contains all LEDs and channels where a short condition was detected.
> +	The format is ledname:channel.
> +
> +	Example usage::
> +
> +		## Read
> +		# cat /sys/class/spi_master/spi<bus>/spi<bus>.<dev>/fault_state_short
> +		rgb1:0 rgb2:4
> 
> -- 
> 2.51.0
> 

-- 
Lee Jones [李琼斯]

[PATCH v2 2/2] leds: lp5860: detect and report fault state

[Steffen Trumtrar]

The lp5860 can detect shorts and open circuits. If an open (LOD) or
short (LSD) is detected, the global bit in LP5860_FAULT_STATE is set.
The channel that caused this can be read from the corresponding Dot_lsdX
and Dot_lodX register and bit offset.

The global bits can be cleared by writing 0xf to the LOD/LSD_clear
register.

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
 drivers/leds/rgb/leds-lp5860-core.c       | 143 ++++++++++++++++++++++++++++++
 include/linux/platform_data/leds-lp5860.h |  19 +++-
 2 files changed, 159 insertions(+), 3 deletions(-)

diff --git a/drivers/leds/rgb/leds-lp5860-core.c b/drivers/leds/rgb/leds-lp5860-core.c
index 28b4d86e11f1a..b62a1db18c05b 100644
--- a/drivers/leds/rgb/leds-lp5860-core.c
+++ b/drivers/leds/rgb/leds-lp5860-core.c
@@ -19,6 +19,149 @@ static struct lp5860_led *mcled_cdev_to_led(struct led_classdev_mc *mc_cdev)
 	return container_of(mc_cdev, struct lp5860_led, mc_cdev);
 }
 
+static const char *lp5860_led_name(struct lp5860 *lp, unsigned int idx)
+{
+	for (int i = 0; i < lp->leds_count; i++) {
+		struct mc_subled *mc_led_info;
+		struct lp5860_led *led;
+
+		led = &lp->leds[i];
+
+		mc_led_info = led->mc_cdev.subled_info;
+
+		for (int j = 0; j < led->mc_cdev.num_colors; j++) {
+			if (mc_led_info[j].channel == idx)
+				return led->mc_cdev.led_cdev.dev->kobj.name;
+		}
+	}
+
+	return 0;
+}
+
+static ssize_t lp5860_get_fault_state(struct lp5860 *led, char *buf,
+				      unsigned int reg, unsigned int length)
+{
+	int len = 0;
+
+	for (unsigned int dot = 0; dot < length; dot++) {
+		unsigned int offset = 0;
+		unsigned int value;
+		unsigned int max_bits;
+		int ret;
+
+		ret = regmap_read(led->regmap, reg + dot, &value);
+		if (ret)
+			return ret;
+
+		max_bits = BITS_PER_BYTE;
+		/* Every 3rd Dot_x register only has 2 bits */
+		if (dot % 3 == 2)
+			max_bits = 2;
+
+		for (unsigned int bit = 0; bit < max_bits; bit++) {
+			offset++;
+
+			if (value & BIT(bit)) {
+				len += sprintf(buf + len, "%s:%d", lp5860_led_name(led, offset),
+					       offset - 1);
+				len += sprintf(buf + len, " ");
+			}
+		}
+	}
+
+	buf[len++] = '\n';
+
+	return len;
+}
+
+static ssize_t lp5860_fault_state_open_show(struct device *dev,
+					    struct device_attribute *attr,
+					    char *buf)
+{
+	struct lp5860 *led = dev_get_drvdata(dev);
+	unsigned int value;
+	int ret;
+
+	ret = regmap_read(led->regmap, LP5860_REG_FAULT_STATE, &value);
+	if (ret)
+		return ret;
+
+	if (!(value & LP5860_FAULT_STATE_OPEN_DETECTION))
+		return 0;
+
+	return lp5860_get_fault_state(led, buf, LP5860_REG_DOT_LOD_START,
+				      LP5860_DOT_LOD_LENGTH);
+}
+static LP5860_DEV_ATTR_R(fault_state_open);
+
+static ssize_t lp5860_fault_state_short_show(struct device *dev,
+					     struct device_attribute *attr, char *buf)
+{
+	struct lp5860 *led = dev_get_drvdata(dev);
+	unsigned int value;
+	int ret;
+
+	ret = regmap_read(led->regmap, LP5860_REG_FAULT_STATE, &value);
+	if (ret)
+		return ret;
+
+	if (!(value & LP5860_FAULT_STATE_SHORT_DETECTION))
+		return 0;
+
+	return lp5860_get_fault_state(led, buf, LP5860_REG_DOT_LSD_START,
+				      LP5860_DOT_LSD_LENGTH);
+}
+static LP5860_DEV_ATTR_R(fault_state_short);
+
+static ssize_t lp5860_fault_state_show(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct lp5860 *led = dev_get_drvdata(dev);
+	unsigned int value;
+	int ret;
+
+	ret = regmap_read(led->regmap, LP5860_REG_FAULT_STATE, &value);
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "%d\n", (value & 0x3));
+}
+
+static ssize_t lp5860_fault_state_store(struct device *dev,
+					struct device_attribute *attr,
+					const char *buf, size_t len)
+{
+	struct lp5860 *led = dev_get_drvdata(dev);
+	unsigned int value = 0;
+	int ret;
+
+	if (kstrtoint(buf, 0, &value))
+		return -EINVAL;
+
+	if (value & LP5860_FAULT_STATE_OPEN_DETECTION)
+		ret = regmap_write(led->regmap, LP5860_REG_LOD_CLEAR, 0xf);
+
+	if (value & LP5860_FAULT_STATE_SHORT_DETECTION)
+		ret = regmap_write(led->regmap, LP5860_REG_LSD_CLEAR, 0xf);
+
+	if (ret < 0)
+		return ret;
+
+	return len;
+}
+static LP5860_DEV_ATTR_RW(fault_state);
+
+static struct attribute *lp5860_led_attrs[] = {
+	&dev_attr_fault_state_open.attr,
+	&dev_attr_fault_state_short.attr,
+	&dev_attr_fault_state.attr,
+	NULL,
+};
+
+static const struct attribute_group lp5860_led_group = {
+	.attrs = lp5860_led_attrs,
+};
+
 static int lp5860_set_dot_onoff(struct lp5860_led *led, unsigned int dot, bool enable)
 {
 	unsigned int offset = dot / LP5860_MAX_DOT_ONOFF_GROUP_NUM;
diff --git a/include/linux/platform_data/leds-lp5860.h b/include/linux/platform_data/leds-lp5860.h
index 7bc69a7a550dd..7b2cc88d70e24 100644
--- a/include/linux/platform_data/leds-lp5860.h
+++ b/include/linux/platform_data/leds-lp5860.h
@@ -188,7 +188,9 @@
 #define LP5860_DOT_CS_ON		0x01
 #define LP5860_DOT_CS_OFF		0x00
 
-/* Dot lod value */
+/* Dot LED open detection (LOD) value */
+#define LP5860_FAULT_STATE_OPEN_DETECTION	BIT(1)
+
 #define LP5860_DOT_LOD0_OFFSET		0
 #define LP5860_DOT_LOD1_OFFSET		1
 #define LP5860_DOT_LOD2_OFFSET		2
@@ -201,7 +203,11 @@
 #define LP5860_DOT_LOD_ON		0x01
 #define LP5860_DOT_LOD_OFF		0x00
 
-/* dot lsd value */
+#define LP5860_DOT_LOD_LENGTH		0x20
+
+/* Dot LED short detection (LSD) value */
+#define LP5860_FAULT_STATE_SHORT_DETECTION	BIT(0)
+
 #define LP5860_DOT_LSD0_OFFSET		0
 #define LP5860_DOT_LSD1_OFFSET		1
 #define LP5860_DOT_LSD2_OFFSET		2
@@ -214,7 +220,8 @@
 #define LP5860_DOT_LSD_ON		0x01
 #define LP5860_DOT_LSD_OFF		0x00
 
-/* Register lod state */
+#define LP5860_DOT_LSD_LENGTH		0x20
+
 #define LP5860_GLOBAL_LOD_OFFSET	1
 #define LP5860_GLOBAL_LOD_STATE		BIT(1)
 #define LP5860_GLOBAL_LSD_OFFSET	0
@@ -248,6 +255,12 @@
  */
 #define LP5860_MAX_LED_CHANNELS		4
 
+#define LP5860_DEV_ATTR_R(name)	\
+	DEVICE_ATTR(name, 0444, lp5860_##name##_show, NULL)
+
+#define LP5860_DEV_ATTR_RW(name)	\
+	DEVICE_ATTR(name, 0644, lp5860_##name##_show, lp5860_##name##_store)
+
 struct lp5860_led {
 	struct lp5860 *chip;
 	struct led_classdev_mc mc_cdev;

-- 
2.51.0

Re: [PATCH v2 2/2] leds: lp5860: detect and report fault state

[Lee Jones]

On Wed, 11 Mar 2026, Steffen Trumtrar wrote:

> The lp5860 can detect shorts and open circuits. If an open (LOD) or
> short (LSD) is detected, the global bit in LP5860_FAULT_STATE is set.
> The channel that caused this can be read from the corresponding Dot_lsdX
> and Dot_lodX register and bit offset.
> 
> The global bits can be cleared by writing 0xf to the LOD/LSD_clear
> register.

The commit message describes what the hardware does, which is good context,
but it should also describe what this patch does. For instance, it adds
sysfs attributes to expose this fault detection functionality. Could you
please add a sentence or two about the implementation?

> 
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> ---
>  drivers/leds/rgb/leds-lp5860-core.c       | 143 ++++++++++++++++++++++++++++++
>  include/linux/platform_data/leds-lp5860.h |  19 +++-
>  2 files changed, 159 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/leds/rgb/leds-lp5860-core.c b/drivers/leds/rgb/leds-lp5860-core.c
> index 28b4d86e11f1a..b62a1db18c05b 100644
> --- a/drivers/leds/rgb/leds-lp5860-core.c
> +++ b/drivers/leds/rgb/leds-lp5860-core.c
> @@ -19,6 +19,149 @@ static struct lp5860_led *mcled_cdev_to_led(struct led_classdev_mc *mc_cdev)
>  	return container_of(mc_cdev, struct lp5860_led, mc_cdev);
>  }
>  
> +static const char *lp5860_led_name(struct lp5860 *lp, unsigned int idx)
> +{
> +	for (int i = 0; i < lp->leds_count; i++) {
> +		struct mc_subled *mc_led_info;
> +		struct lp5860_led *led;
> +
> +		led = &lp->leds[i];
> +
> +		mc_led_info = led->mc_cdev.subled_info;
> +
> +		for (int j = 0; j < led->mc_cdev.num_colors; j++) {
> +			if (mc_led_info[j].channel == idx)
> +				return led->mc_cdev.led_cdev.dev->kobj.name;
> +		}
> +	}
> +
> +	return 0;
> +}

This nested loop appears to be quite inefficient, especially since it may be
called many times from `lp5860_get_fault_state()`.

It would be better to build a reverse mapping array (channel index to
LED name) during probe to make this an O(1) lookup at runtime?

> +
> +static ssize_t lp5860_get_fault_state(struct lp5860 *led, char *buf,
> +				     unsigned int reg, unsigned int length)

The parameter name `led` here is a bit confusing, as it refers to the chip
private data, not a specific LED. This should be `lp` or `chip`.

> +{
> +	int len = 0;
> +
> +	for (unsigned int dot = 0; dot < length; dot++) {
> +		unsigned int offset = 0;
> +		unsigned int value;
> +		unsigned int max_bits;
> +		int ret;
> +
> +		ret = regmap_read(led->regmap, reg + dot, &value);
> +		if (ret)
> +			return ret;
> +
> +		max_bits = BITS_PER_BYTE;
> +		/* Every 3rd Dot_x register only has 2 bits */
> +		if (dot % 3 == 2)
> +			max_bits = 2;
> +
> +		for (unsigned int bit = 0; bit < max_bits; bit++) {
> +			offset++;
> +
> +			if (value & BIT(bit)) {
> +				len += sprintf(buf + len, "%s:%d", lp5860_led_name(led, offset),
> +					      offset - 1);

A sysfs buffer is only `PAGE_SIZE`. Using unbounded `sprintf` in a loop
risks a buffer overflow if there are many faults and/or long LED names.
It would be safer to use `scnprintf` here to ensure you don't write past
the end of the buffer.

> +				len += sprintf(buf + len, " ");

Why not add the space in during the one above?

> +			}
> +		}
> +	}
> +
> +	buf[len++] = '\n';
> +
> +	return len;
> +}
> +
> +static ssize_t lp5860_fault_state_open_show(struct device *dev,
> +					   struct device_attribute *attr,
> +					   char *buf)
> +{
> +	struct lp5860 *led = dev_get_drvdata(dev);
> +	unsigned int value;
> +	int ret;
> +
> +	ret = regmap_read(led->regmap, LP5860_REG_FAULT_STATE, &value);
> +	if (ret)
> +		return ret;
> +
> +	if (!(value & LP5860_FAULT_STATE_OPEN_DETECTION))
> +		return 0;
> +
> +	return lp5860_get_fault_state(led, buf, LP5860_REG_DOT_LOD_START,
> +				     LP5860_DOT_LOD_LENGTH);
> +}
> +static LP5860_DEV_ATTR_R(fault_state_open);
> +
> +static ssize_t lp5860_fault_state_short_show(struct device *dev,
> +					    struct device_attribute *attr, char *buf)
> +{
> +	struct lp5860 *led = dev_get_drvdata(dev);
> +	unsigned int value;
> +	int ret;
> +
> +	ret = regmap_read(led->regmap, LP5860_REG_FAULT_STATE, &value);
> +	if (ret)
> +		return ret;
> +
> +	if (!(value & LP5860_FAULT_STATE_SHORT_DETECTION))
> +		return 0;
> +
> +	return lp5860_get_fault_state(led, buf, LP5860_REG_DOT_LSD_START,
> +				     LP5860_DOT_LSD_LENGTH);
> +}
> +static LP5860_DEV_ATTR_R(fault_state_short);
> +
> +static ssize_t lp5860_fault_state_show(struct device *dev,
> +				 struct device_attribute *attr, char *buf)
> +{
> +	struct lp5860 *led = dev_get_drvdata(dev);
> +	unsigned int value;
> +	int ret;
> +
> +	ret = regmap_read(led->regmap, LP5860_REG_FAULT_STATE, &value);
> +	if (ret)
> +		return ret;
> +
> +	return sysfs_emit(buf, "%d\n", (value & 0x3));

No magic numbers.  Please define the 3.

> +}
> +
> +static ssize_t lp5860_fault_state_store(struct device *dev,
> +					struct device_attribute *attr,
> +					const char *buf, size_t len)
> +{
> +	struct lp5860 *led = dev_get_drvdata(dev);
> +	unsigned int value = 0;
> +	int ret;

The `ret` variable is not initialized here. If `value` is 0, `ret` will be
used uninitialized in the `if (ret < 0)` check below. Please initialize it
to 0.

> +
> +	if (kstrtoint(buf, 0, &value))
> +		return -EINVAL;
> +
> +	if (value & LP5860_FAULT_STATE_OPEN_DETECTION)
> +		ret = regmap_write(led->regmap, LP5860_REG_LOD_CLEAR, 0xf);

The value `0xf` seems magical. The commit message mentions it, but could we
have a define for this, like `LP5860_FAULT_CLEAR_ALL`?

Also, if this `regmap_write()` fails, the next `if` block will still be
executed, overwriting the error code in `ret`. We should probably check and
return on failure immediately.

> +
> +	if (value & LP5860_FAULT_STATE_SHORT_DETECTION)
> +		ret = regmap_write(led->regmap, LP5860_REG_LSD_CLEAR, 0xf);

Define the 0xf.

> +
> +	if (ret < 0)
> +		return ret;
> +
> +	return len;
> +}
> +static LP5860_DEV_ATTR_RW(fault_state);
> +
> +static struct attribute *lp5860_led_attrs[] = {
> +	&dev_attr_fault_state_open.attr,
> +	&dev_attr_fault_state_short.attr,
> +	&dev_attr_fault_state.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group lp5860_led_group = {
> +	.attrs = lp5860_led_attrs,
> +};

Where is this used?

> +
>  static int lp5860_set_dot_onoff(struct lp5860_led *led, unsigned int dot, bool enable)
>  {
>  	unsigned int offset = dot / LP5860_MAX_DOT_ONOFF_GROUP_NUM;
> diff --git a/include/linux/platform_data/leds-lp5860.h b/include/linux/platform_data/leds-lp5860.h
> index 7bc69a7a550dd..7b2cc88d70e24 100644
> --- a/include/linux/platform_data/leds-lp5860.h
> +++ b/include/linux/platform_data/leds-lp5860.h
> @@ -188,7 +188,9 @@
>  #define LP5860_DOT_CS_ON		0x01
>  #define LP5860_DOT_CS_OFF		0x00
>  
> -/* Dot lod value */
> +/* Dot LED open detection (LOD) value */
> +#define LP5860_FAULT_STATE_OPEN_DETECTION	BIT(1)
> +
>  #define LP5860_DOT_LOD0_OFFSET		0
>  #define LP5860_DOT_LOD1_OFFSET		1
>  #define LP5860_DOT_LOD2_OFFSET		2
> @@ -201,7 +203,11 @@
>  #define LP5860_DOT_LOD_ON		0x01
>  #define LP5860_DOT_LOD_OFF		0x00
>  
> -/* dot lsd value */
> +#define LP5860_DOT_LOD_LENGTH		0x20
> +
> +/* Dot LED short detection (LSD) value */
> +#define LP5860_FAULT_STATE_SHORT_DETECTION	BIT(0)
> +
>  #define LP5860_DOT_LSD0_OFFSET		0
>  #define LP5860_DOT_LSD1_OFFSET		1
>  #define LP5860_DOT_LSD2_OFFSET		2
> @@ -214,7 +220,8 @@
>  #define LP5860_DOT_LSD_ON		0x01
>  #define LP5860_DOT_LSD_OFF		0x00
>  
> -/* Register lod state */
> +#define LP5860_DOT_LSD_LENGTH		0x20
> +
>  #define LP5860_GLOBAL_LOD_OFFSET	1
>  #define LP5860_GLOBAL_LOD_STATE		BIT(1)
>  #define LP5860_GLOBAL_LSD_OFFSET	0
> @@ -248,6 +255,12 @@
>   */
>  #define LP5860_MAX_LED_CHANNELS		4
>  
> +#define LP5860_DEV_ATTR_R(name)	\
> +	DEVICE_ATTR(name, 0444, lp5860_##name##_show, NULL)
> +
> +#define LP5860_DEV_ATTR_RW(name)	\
> +	DEVICE_ATTR(name, 0644, lp5860_##name##_show, lp5860_##name##_store)

These macros seem like a bit of a superfluous abstraction. The standard
`DEVICE_ATTR_RO()` and `DEVICE_ATTR_RW()` macros are well-known and would
work just fine if you renamed the handler functions to drop the
`lp5860_` prefix (e.g., `fault_state_open_show()`).

> +
>  struct lp5860_led {
>  	struct lp5860 *chip;
>  	struct led_classdev_mc mc_cdev;
> 
> -- 
> 2.51.0
> 

-- 
Lee Jones [李琼斯]