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

[sashiko-bot@kernel.org]

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()?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1790684839.git.nora.schiffer@ew.tq-group.com?part=8