Re: [PATCH 7/7] leds: pca995x: add support for group brightness control

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Missing explicit initialization of GRPPWM and MODE2 registers makes LED brightness dependent on undefined hardware states.
- [Low] The commit message description is not capitalized, violating LED subsystem naming conventions.
- [Low] has_errors_store uses strcmp() instead of sysfs_streq() to parse sysfs input, rejecting valid inputs without a newline.
--

commit 2816a66784d3fbb95bdf9b79d38d26cc4f4d0d96
Author: Nora Schiffer <nora.schiffer@ew.tq-group.com>

leds: pca995x: add support for group brightness control

This commit introduces group brightness control for PCA995x LEDs. It sets
the LEDs to group PWM mode, enabling global group PWM settings to modulate
individual brightness. It also adds a sysfs attribute for group brightness
and removes the non-group PWM setting.

> leds: pca995x: add support for group brightness control

[Severity: Low]
This isn't a bug, but does the commit message subject line violate the LED
subsystem naming conventions?  Should the description after the subsystem
prefix be capitalized, such as "leds: pca995x: Add support for group
brightness control"?

> diff --git a/drivers/leds/leds-pca995x.c b/drivers/leds/leds-pca995x.c
> index 62f2764661b9d..d65c33edc83ab 100644
> --- a/drivers/leds/leds-pca995x.c
> +++ b/drivers/leds/leds-pca995x.c

[ ... ]

> @@ -110,11 +115,10 @@ static int pca995x_brightness_set(struct led_classdev *led_cdev,
>  
>  	/*
>  	 * Change LDRx configuration to individual brightness via PWM.
> -	 * LED will stop blinking if it's doing so.
>  	 */
>  	return regmap_update_bits(chip->regmap, ledout_addr,
>  				  PCA995X_LDRX_MASK << shift,
> -				  PCA995X_LED_PWM_MODE << shift);
> +				  PCA995X_LED_PWM_MODE_GRP << shift);
>  }

[Severity: Medium]
Does this change make the LED brightness depend on undefined hardware states?

Without explicit initialization of the GRPPWM and MODE2 registers during
pca995x_probe(), a system without a reset GPIO might leave the PCA995x in an
unexpected state during a warm reboot.

If the driver loads and configures the LEDs into PCA995X_LED_PWM_MODE_GRP
while the registers aren't at their default values (GRPPWM=255 and
MODE2=dimming), will this cause incorrect behavior such as LEDs blinking or
remaining off instead of turning on dimly?

[ ... ]

> @@ -186,10 +190,41 @@ static ssize_t has_errors_store(struct device *dev, struct device_attribute *att
>  	return ret ?: count;
>  }

[Severity: Low]
Is the parsing logic in has_errors_store() overly strict?

It currently uses strcmp(buf, "clear\n"), which rejects valid userspace
commands like 'echo -n clear' because they lack a trailing newline.  Would it
be better to use sysfs_streq() instead to handle inputs both with and without
a newline?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/6d3600f99c77c31166f0b6a5ccfb7f518b53c4c9.1788350618.git.nora.schiffer@ew.tq-group.com?part=7