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

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

On Wed, 2026-09-30 at 15:48 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] leds: pca995x: unintentional overwrite of MODE2 register during probe
> --
> 
> commit 1bf2389241c12cac5965e7df3db32ae52db81482
> Author: Nora Schiffer <nora.schiffer@ew.tq-group.com>
> 
> leds: pca995x: Add sysfs files for error reporting
> 
> 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.
> 
> > diff --git a/drivers/leds/leds-pca995x.c b/drivers/leds/leds-pca995x.c
> > index df3f7ab2e1f87..1fc8de5a77bbc 100644
> > --- a/drivers/leds/leds-pca995x.c
> > +++ b/drivers/leds/leds-pca995x.c
> [ ... ]
> > @@ -185,11 +279,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);
> 
> [Severity: High]
> Does this unconditional write clobber the entire MODE2 register?
> 
> Using regmap_write() instead of regmap_update_bits() zeroes out all other
> bits in the MODE2 register.
> 
> This clobbers the power-on reset defaults of the hardware, specifically the
> OUTDRV bit (typically bit 2), which defaults to 1 (totem-pole output).
> 
> Clearing it changes the output driver to open-drain, which can cause the
> LEDs to fail to light up or behave erratically.
> 
> Could regmap_update_bits() be used here instead to only set the CLRERR bit,
> similar to how it is handled in has_errors_store()?

Okay, while the description of the specific flag is nonsense (none of the
PCA995x have an "OUTDRV" bit), the unconditional initialization of MODE2 will
have to differ between the PCA9952 and PCA9955B/PCA9956B, and only the latter
implement the error reporting as handled by my patch - I'll fix this in the next
revision.

One question: The AI reviews made conflicting suggestions in different revisions
of my series. First it suggested to fully initialize MODE2 instead of relying on
reset defaults. Now it suggested the opposite. Which should I go with?

@Lee Jones: Patches 8-10 will definitely need another revision, but if patches
1-7 look good to you, they could be applied by themselves, so the next series
would be smaller.

Best,
Nora




> 
> > +	if (ret)
> > +		goto err_put_nodes;
> > +
> 

-- 
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/