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

[Lee Jones <lee@kernel.org>]

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 [李琼斯]