Re: [PATCH v3 08/10] leds: pca995x: Add sysfs files for error reporting

[Nora Schiffer <nora.schiffer@ew.tq-group.com>]

On Tue, 2026-09-29 at 11:12 +0100, Lee Jones wrote:
> On Tue, 22 Sep 2026, Nora Schiffer wrote:
> 
> > The PCA995x has builtin failure detection. Provide sysfs files for
> > individual LED status (reporting "okay", "short-circuit" or
> > "open-circuit") as well as a global "has_errors" flag. has_errors is
> > sticky and must be cleared by writing "clear" to the sysfs file.
> > 
> > Signed-off-by: Nora Schiffer <nora.schiffer@ew.tq-group.com>
> > ---
> >  drivers/leds/leds-pca995x.c | 112 +++++++++++++++++++++++++++++++++++-
> >  1 file changed, 111 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/leds/leds-pca995x.c b/drivers/leds/leds-pca995x.c
> > index 13cce0c3fdc19..57b5d2d6e035d 100644
> > --- a/drivers/leds/leds-pca995x.c
> > +++ b/drivers/leds/leds-pca995x.c
> > @@ -8,6 +8,7 @@
> >   * Copyright 2023 Marek Vasut
> >   */
> > 
> > +#include <linux/bits.h>
> >  #include <linux/delay.h>
> >  #include <linux/gpio/consumer.h>
> >  #include <linux/i2c.h>
> > @@ -24,6 +25,12 @@
> >  /* Auto-increment disabled. Normal mode */
> >  #define PCA995X_MODE1_CFG		0x00
> > 
> > +#define PCA995X_MODE2_CLRERR		BIT(4)
> > +#define PCA995X_MODE2_ERROR		BIT(6)
> > +
> > +/* Clear errors on probe, group brightness control, linear adjustment */
> > +#define PCA995X_MODE2_CFG		PCA995X_MODE2_CLRERR
> > +
> >  /* LED select registers determine the source that drives LED outputs */
> >  #define PCA995X_LED_OFF			0x0
> >  #define PCA995X_LED_ON			0x1
> > @@ -37,30 +44,37 @@
> >  #define PCA995X_IREFALL_FULL_CFG	0xFF
> >  #define PCA995X_IREFALL_HALF_CFG	(PCA995X_IREFALL_FULL_CFG / 2)
> > 
> > +#define PCA995X_EFLAG_BITS		2
> > +#define PCA995X_EFLAG_MASK		GENMASK(1, 0)
> > +
> >  #define ldev_to_led(c)	container_of(c, struct pca995x_led, ldev)
> > 
> >  struct pca995x_chipdef {
> >  	unsigned int num_leds;
> >  	u8 pwm_base;
> >  	u8 irefall;
> > +	u8 eflag_base;
> >  };
> > 
> >  static const struct pca995x_chipdef pca9952_chipdef = {
> >  	.num_leds	= 16,
> >  	.pwm_base	= 0x0a,
> >  	.irefall	= 0x43,
> > +	.eflag_base	= 0x44,
> >  };
> > 
> >  static const struct pca995x_chipdef pca9955b_chipdef = {
> >  	.num_leds	= 16,
> >  	.pwm_base	= 0x08,
> >  	.irefall	= 0x45,
> > +	.eflag_base	= 0x46,
> >  };
> > 
> >  static const struct pca995x_chipdef pca9956b_chipdef = {
> >  	.num_leds	= 24,
> >  	.pwm_base	= 0x0a,
> >  	.irefall	= 0x40,
> > +	.eflag_base	= 0x41,
> >  };
> > 
> >  struct pca995x_led {
> > @@ -112,6 +126,83 @@ static int pca995x_brightness_set(struct led_classdev *led_cdev,
> >  	}
> >  }
> > 
> > +static ssize_t status_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> > +	struct pca995x_led *led = ldev_to_led(led_cdev);
> > +	struct pca995x_chip *chip = led->chip;
> > +	const struct pca995x_chipdef *chipdef = chip->chipdef;
> > +	const char *status = "unknown";
> > +	unsigned int val;
> > +	int shift, ret;
> > +	u8 reg;
> > +
> > +	reg = chipdef->eflag_base + (led->led_no / PCA995X_OUTPUTS_PER_REG);
> > +	shift = PCA995X_EFLAG_BITS * (led->led_no % PCA995X_OUTPUTS_PER_REG);
> > +
> > +	ret = regmap_read(chip->regmap, reg, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	switch ((val >> shift) & PCA995X_EFLAG_MASK) {
> > +	case 0:
> > +		status = "okay";
> > +		break;
> > +	case 1:
> > +		status = "short-circuit";
> > +		break;
> > +	case 2:
> > +		status = "open-circuit";
> > +	}
> > +
> > +	return sysfs_emit(buf, "%s\n", status);
> > +}
> > +
> > +static DEVICE_ATTR_RO(status);
> > +
> > +static struct attribute *pca995x_led_attrs[] = {
> > +	&dev_attr_status.attr,
> > +	NULL,
> > +};
> > +ATTRIBUTE_GROUPS(pca995x_led);
> > +
> > +static ssize_t has_errors_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > +	struct pca995x_chip *chip = i2c_get_clientdata(to_i2c_client(dev));
> 
> Why not 'dev_get_drvdata(dev)' instead of bouncing through the i2c_client?
> 
> > +	unsigned int val;
> > +	int ret;
> > +
> > +	ret = regmap_read(chip->regmap, PCA995X_MODE2, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +
> 
> Stray blank line.
> 
> > +	return sysfs_emit(buf, "%d\n", !!(val & PCA995X_MODE2_ERROR));
> > +}
> > +
> > +static ssize_t has_errors_store(struct device *dev, struct device_attribute *attr,
> > +				const char *buf, size_t count)
> > +{
> > +	struct pca995x_chip *chip = i2c_get_clientdata(to_i2c_client(dev));
> 
> As above.
> 
> > +	int ret;
> > +
> > +	if (!sysfs_streq(buf, "clear"))
> > +		return -EINVAL;
> > +
> > +	ret = regmap_update_bits(chip->regmap, PCA995X_MODE2,
> > +				 PCA995X_MODE2_CLRERR, PCA995X_MODE2_CLRERR);
> > +
> > +	return ret ?: count;
> > +}
> > +
> > +static DEVICE_ATTR_RW(has_errors);
> > +
> > +static struct attribute *pca995x_attrs[] = {
> > +	&dev_attr_has_errors.attr,
> > +	NULL,
> > +};
> > +ATTRIBUTE_GROUPS(pca995x);
> > +
> >  static const struct regmap_config pca995x_regmap = {
> >  	.reg_bits = 8,
> >  	.val_bits = 8,
> > @@ -176,6 +267,7 @@ static int pca995x_probe(struct i2c_client *client)
> >  		led->led_no = reg;
> >  		led->ldev.brightness_set_blocking = pca995x_brightness_set;
> >  		led->ldev.max_brightness = 255;
> > +		led->ldev.groups = pca995x_led_groups;
> >  	}
> > 
> >  	/* Disable LED all-call address and set normal mode */
> > @@ -183,11 +275,20 @@ static int pca995x_probe(struct i2c_client *client)
> >  	if (ret)
> >  		goto err_put_nodes;
> > 
> > +	/* Clear errors on probe */
> > +	ret = regmap_write(chip->regmap, PCA995X_MODE2, PCA995X_MODE2_CFG);
> > +	if (ret)
> > +		goto err_put_nodes;
> > +
> >  	/* IREF Output current value for all LEDn outputs */
> >  	ret = regmap_write(chip->regmap, chipdef->irefall, iref);
> >  	if (ret)
> >  		goto err_put_nodes;
> > 
> > +	ret = device_add_groups(dev, pca995x_groups);
> 
> Why not devm_* and remove the clean-up?

Ahh, the AI had suggested this too, but it was referring to a non-exisitent API.
I didn't know the function was called devm_device_add_group() - found it now,
will fix in the next revision.


> 
> > +	if (ret)
> > +		goto err_put_nodes;
> > +
> >  	for (i = 0; i < chipdef->num_leds; i++) {
> >  		struct led_init_data init_data = {};
> > 
> > @@ -202,7 +303,7 @@ static int pca995x_probe(struct i2c_client *client)
> >  		if (ret < 0) {
> >  			dev_err_probe(dev, ret, "Could not register LED %s\n",
> >  				      chip->leds[i].ldev.name);
> > -			goto err_put_nodes;
> > +			goto err_remove_groups;
> >  		}
> > 
> >  		led_fwnodes[i] = NULL;
> > @@ -210,6 +311,9 @@ static int pca995x_probe(struct i2c_client *client)
> > 
> >  	return 0;
> > 
> > +err_remove_groups:
> > +	device_remove_groups(dev, pca995x_groups);
> > +
> 
> You can remove this if you move to devm_*.
> 
> >  err_put_nodes:
> >  	for (i = 0; i < chipdef->num_leds; i++)
> >  		fwnode_handle_put(led_fwnodes[i]);
> > @@ -217,6 +321,11 @@ static int pca995x_probe(struct i2c_client *client)
> >  	return ret;
> >  }
> > 
> > +static void pca995x_remove(struct i2c_client *client)
> > +{
> > +	device_remove_groups(&client->dev, pca995x_groups);
> > +}
> > +
> 
> As above.
> 
> >  static const struct i2c_device_id pca995x_id[] = {
> >  	{ .name = "pca9952", .driver_data = (kernel_ulong_t)&pca9952_chipdef },
> >  	{ .name = "pca9955b", .driver_data = (kernel_ulong_t)&pca9955b_chipdef },
> > @@ -239,6 +348,7 @@ static struct i2c_driver pca995x_driver = {
> >  		.of_match_table = pca995x_of_match,
> >  	},
> >  	.probe = pca995x_probe,
> > +	.remove = pca995x_remove,
> >  	.id_table = pca995x_id,
> >  };
> >  module_i2c_driver(pca995x_driver);
> > -- 
> > TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> > Amtsgericht München, HRB 105018
> > Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> > https://www.tq-group.com/
> > 
> > 
> 

-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/