* [PATCH 2/7] dt-bindings: leds: pca995x: describe output-gain property
2026-09-02 12:22 [PATCH 1/7] dt-bindings: leds: pca995x: describe reset-gpios property Nora Schiffer
@ 2026-09-02 12:22 ` Nora Schiffer
2026-09-02 12:31 ` sashiko-bot
2026-09-02 12:22 ` [PATCH 3/7] leds: pca995x: add support for reset GPIO Nora Schiffer
` (5 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Nora Schiffer @ 2026-09-02 12:22 UTC (permalink / raw)
To: Lee Jones, Pavel Machek
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Isai Gaspar,
linux, linux-leds, devicetree, linux-kernel, Nora Schiffer
Allow configuring the output current to adjust for different hardware
designs.
Signed-off-by: Nora Schiffer <nora.schiffer@ew.tq-group.com>
---
Documentation/devicetree/bindings/leds/nxp,pca995x.yaml | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/Documentation/devicetree/bindings/leds/nxp,pca995x.yaml b/Documentation/devicetree/bindings/leds/nxp,pca995x.yaml
index 201f32f1b8139..7569e1b8a38fa 100644
--- a/Documentation/devicetree/bindings/leds/nxp,pca995x.yaml
+++ b/Documentation/devicetree/bindings/leds/nxp,pca995x.yaml
@@ -36,6 +36,14 @@ properties:
"#size-cells":
const: 0
+ output-gain:
+ description:
+ Set the output gain controlling the LED current as described in the
+ PCA995x datasheets.
+ $ref: /schemas/types.yaml#/definitions/uint32
+ default: 128
+ maximum: 255
+
reset-gpios:
maxItems: 1
--
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/
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 2/7] dt-bindings: leds: pca995x: describe output-gain property
2026-09-02 12:22 ` [PATCH 2/7] dt-bindings: leds: pca995x: describe output-gain property Nora Schiffer
@ 2026-09-02 12:31 ` sashiko-bot
2026-09-02 13:10 ` Nora Schiffer
0 siblings, 1 reply; 19+ messages in thread
From: sashiko-bot @ 2026-09-02 12:31 UTC (permalink / raw)
To: Nora Schiffer; +Cc: linux-leds, conor+dt, devicetree, robh, lee
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Low] The custom, device-specific property `output-gain` lacks a mandatory vendor prefix.
- [Low] The `output-gain` property incorrectly exposes a raw hardware register value directly to the Device Tree instead of describing physical units.
- [Low] The commit message description is not capitalized, violating LED subsystem naming conventions.
--
commit 09d8712f125575e995a97c0a819c386e17eb766a
Author: Nora Schiffer <nora.schiffer@ew.tq-group.com>
dt-bindings: leds: pca995x: describe output-gain property
This commit adds the output-gain property to the pca995x device tree binding,
allowing configuration of the output current to adjust for different hardware
designs.
> dt-bindings: leds: pca995x: describe output-gain property
[Severity: Low]
This isn't a bug, but the subsystem guidelines for LED bindings specify
that the description after the prefix should be capitalized. Should
this be changed to "Describe output-gain property"?
> diff --git a/Documentation/devicetree/bindings/leds/nxp,pca995x.yaml b/Documentation/devicetree/bindings/leds/nxp,pca995x.yaml
> --- a/Documentation/devicetree/bindings/leds/nxp,pca995x.yaml
> +++ b/Documentation/devicetree/bindings/leds/nxp,pca995x.yaml
[ ... ]
> @@ -36,6 +36,14 @@ properties:
> "#size-cells":
> const: 0
>
> + output-gain:
[Severity: Low]
Because this is a custom, device-specific property, does it require a
mandatory vendor prefix (such as nxp,output-gain)? Exposing it without a
prefix could pollute the global device tree namespace and potentially conflict
with future standard properties or other bindings.
> + description:
> + Set the output gain controlling the LED current as described in the
> + PCA995x datasheets.
> + $ref: /schemas/types.yaml#/definitions/uint32
> + default: 128
> + maximum: 255
[Severity: Low]
Do these constraints expose a raw 8-bit hardware register value directly
to the device tree? Usually, device tree properties describe hardware neutrally
using standard physical units (such as microamps) rather than tying the value
to the internal register layout of the chip. Could this be abstracted to use
standard physical units instead?
--
Sashiko AI review · https://sashiko.dev/#/patchset/6d3600f99c77c31166f0b6a5ccfb7f518b53c4c9.1788350618.git.nora.schiffer@ew.tq-group.com?part=2
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/7] dt-bindings: leds: pca995x: describe output-gain property
2026-09-02 12:31 ` sashiko-bot
@ 2026-09-02 13:10 ` Nora Schiffer
2026-09-03 8:34 ` Lee Jones
2026-09-03 13:09 ` Krzysztof Kozlowski
0 siblings, 2 replies; 19+ messages in thread
From: Nora Schiffer @ 2026-09-02 13:10 UTC (permalink / raw)
To: sashiko-reviews, linux-leds, conor+dt, devicetree, robh, lee
On Wed, 2026-09-02 at 12:31 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Low] The custom, device-specific property `output-gain` lacks a mandatory vendor prefix.
Probably makes sense to add the vendor prefix, will do in v2.
> - [Low] The `output-gain` property incorrectly exposes a raw hardware register value directly to the Device Tree instead of describing physical units.
According to the datasheet, the LED current is calculated as follows:
Iled = output_gain * 900mV / Rext / 4
If we want to fully describe the output gain using physical units, something
like output-current-microamps and external-resistor-ohms would work. There
wouldn't be a meaningful default for these properties to describe in the binding
docs however, which is possible for "output-gain" (as the driver just sets that
to 127.)
Best,
Nora
> - [Low] The commit message description is not capitalized, violating LED subsystem naming conventions.
> --
>
> commit 09d8712f125575e995a97c0a819c386e17eb766a
> Author: Nora Schiffer <nora.schiffer@ew.tq-group.com>
>
> dt-bindings: leds: pca995x: describe output-gain property
>
> This commit adds the output-gain property to the pca995x device tree binding,
> allowing configuration of the output current to adjust for different hardware
> designs.
>
> > dt-bindings: leds: pca995x: describe output-gain property
>
> [Severity: Low]
> This isn't a bug, but the subsystem guidelines for LED bindings specify
> that the description after the prefix should be capitalized. Should
> this be changed to "Describe output-gain property"?
>
> > diff --git a/Documentation/devicetree/bindings/leds/nxp,pca995x.yaml b/Documentation/devicetree/bindings/leds/nxp,pca995x.yaml
> > --- a/Documentation/devicetree/bindings/leds/nxp,pca995x.yaml
> > +++ b/Documentation/devicetree/bindings/leds/nxp,pca995x.yaml
> [ ... ]
> > @@ -36,6 +36,14 @@ properties:
> > "#size-cells":
> > const: 0
> >
> > + output-gain:
>
> [Severity: Low]
> Because this is a custom, device-specific property, does it require a
> mandatory vendor prefix (such as nxp,output-gain)? Exposing it without a
> prefix could pollute the global device tree namespace and potentially conflict
> with future standard properties or other bindings.
>
> > + description:
> > + Set the output gain controlling the LED current as described in the
> > + PCA995x datasheets.
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + default: 128
> > + maximum: 255
>
> [Severity: Low]
> Do these constraints expose a raw 8-bit hardware register value directly
> to the device tree? Usually, device tree properties describe hardware neutrally
> using standard physical units (such as microamps) rather than tying the value
> to the internal register layout of the chip. Could this be abstracted to use
> standard physical units instead?
>
--
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/
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 2/7] dt-bindings: leds: pca995x: describe output-gain property
2026-09-02 13:10 ` Nora Schiffer
@ 2026-09-03 8:34 ` Lee Jones
2026-09-03 13:09 ` Krzysztof Kozlowski
1 sibling, 0 replies; 19+ messages in thread
From: Lee Jones @ 2026-09-03 8:34 UTC (permalink / raw)
To: Nora Schiffer; +Cc: sashiko-reviews, linux-leds, conor+dt, devicetree, robh
DT folks,
> On Wed, 2026-09-02 at 12:31 +0000, sashiko-bot@kernel.org wrote:
> > Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> > - [Low] The custom, device-specific property `output-gain` lacks a mandatory vendor prefix.
>
> Probably makes sense to add the vendor prefix, will do in v2.
>
> > - [Low] The `output-gain` property incorrectly exposes a raw hardware register value directly to the Device Tree instead of describing physical units.
>
> According to the datasheet, the LED current is calculated as follows:
>
> Iled = output_gain * 900mV / Rext / 4
>
> If we want to fully describe the output gain using physical units, something
> like output-current-microamps and external-resistor-ohms would work. There
> wouldn't be a meaningful default for these properties to describe in the binding
> docs however, which is possible for "output-gain" (as the driver just sets that
> to 127.)
This is a question for the Device Tree side.
--
Lee Jones
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/7] dt-bindings: leds: pca995x: describe output-gain property
2026-09-02 13:10 ` Nora Schiffer
2026-09-03 8:34 ` Lee Jones
@ 2026-09-03 13:09 ` Krzysztof Kozlowski
1 sibling, 0 replies; 19+ messages in thread
From: Krzysztof Kozlowski @ 2026-09-03 13:09 UTC (permalink / raw)
To: Nora Schiffer
Cc: sashiko-reviews, linux-leds, conor+dt, devicetree, robh, lee
On Wed, Sep 02, 2026 at 03:10:09PM +0200, Nora Schiffer wrote:
> On Wed, 2026-09-02 at 12:31 +0000, sashiko-bot@kernel.org wrote:
> > Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> > - [Low] The custom, device-specific property `output-gain` lacks a mandatory vendor prefix.
>
> Probably makes sense to add the vendor prefix, will do in v2.
>
> > - [Low] The `output-gain` property incorrectly exposes a raw hardware register value directly to the Device Tree instead of describing physical units.
>
> According to the datasheet, the LED current is calculated as follows:
>
> Iled = output_gain * 900mV / Rext / 4
>
> If we want to fully describe the output gain using physical units, something
> like output-current-microamps and external-resistor-ohms would work. There
> wouldn't be a meaningful default for these properties to describe in the binding
> docs however, which is possible for "output-gain" (as the driver just sets that
> to 127.)
Output current is already there, no? led-max-microamp? So basically you
would be re-describing it... although I understand that for your case
you need two properties (also the resistor) to calculate the value to program.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/7] leds: pca995x: add support for reset GPIO
2026-09-02 12:22 [PATCH 1/7] dt-bindings: leds: pca995x: describe reset-gpios property Nora Schiffer
2026-09-02 12:22 ` [PATCH 2/7] dt-bindings: leds: pca995x: describe output-gain property Nora Schiffer
@ 2026-09-02 12:22 ` Nora Schiffer
2026-09-02 12:41 ` sashiko-bot
2026-09-02 12:22 ` [PATCH 4/7] leds: pca995x: make output gain configurable Nora Schiffer
` (4 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Nora Schiffer @ 2026-09-02 12:22 UTC (permalink / raw)
To: Lee Jones, Pavel Machek
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Isai Gaspar,
linux, linux-leds, devicetree, linux-kernel, Nora Schiffer
The PCA995x has a reset pin. Use it to reset the LED driver if provided
in the Device Tree.
Signed-off-by: Nora Schiffer <nora.schiffer@ew.tq-group.com>
---
drivers/leds/leds-pca995x.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/leds/leds-pca995x.c b/drivers/leds/leds-pca995x.c
index fee6216cd1bd0..8eb9847b9709f 100644
--- a/drivers/leds/leds-pca995x.c
+++ b/drivers/leds/leds-pca995x.c
@@ -8,6 +8,7 @@
* Copyright 2023 Marek Vasut
*/
+#include <linux/gpio.h>
#include <linux/i2c.h>
#include <linux/leds.h>
#include <linux/module.h>
@@ -121,6 +122,7 @@ static int pca995x_probe(struct i2c_client *client)
struct fwnode_handle *led_fwnodes[PCA995X_MAX_OUTPUTS] = { 0 };
struct device *dev = &client->dev;
const struct pca995x_chipdef *chipdef;
+ struct gpio_desc *reset_gpio;
struct pca995x_chip *chip;
struct pca995x_led *led;
int i, j, reg, ret;
@@ -130,6 +132,16 @@ static int pca995x_probe(struct i2c_client *client)
if (!dev_fwnode(dev))
return -ENODEV;
+ reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+ if (IS_ERR(reset_gpio))
+ return dev_err_probe(dev, PTR_ERR(reset_gpio),
+ "failed to request reset GPIO\n");
+ if (reset_gpio) {
+ usleep_range(3, 4);
+ gpiod_set_value_cansleep(reset_gpio, 0);
+ usleep_range(1500, 1600);
+ }
+
chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
if (!chip)
return -ENOMEM;
--
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/
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 3/7] leds: pca995x: add support for reset GPIO
2026-09-02 12:22 ` [PATCH 3/7] leds: pca995x: add support for reset GPIO Nora Schiffer
@ 2026-09-02 12:41 ` sashiko-bot
0 siblings, 0 replies; 19+ messages in thread
From: sashiko-bot @ 2026-09-02 12:41 UTC (permalink / raw)
To: Nora Schiffer; +Cc: robh, lee, conor+dt, devicetree, linux-leds
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Low] The commit message description violates the LED subsystem capitalization convention.
- [Low] Using usleep_range() for a microsecond delay under 10us.
- [Low] Use of the legacy <linux/gpio.h> header instead of <linux/gpio/consumer.h>.
- [Low] Missing #include <linux/delay.h> for usleep_range().
--
commit 73fa0526608a4d478003bcf92f25fa0aa33eae7c
Author: Nora Schiffer <nora.schiffer@ew.tq-group.com>
leds: pca995x: add support for reset GPIO
This commit adds support for resetting the PCA995x LED driver during
probe by utilizing a reset GPIO pin if provided in the Device Tree.
> leds: pca995x: add support for reset GPIO
[Severity: Low]
This isn't a bug, but does this violate the LED subsystem commit message
capitalization convention? The guidelines require capitalizing the
description after the subsystem prefix, so "add" would ideally be "Add".
> diff --git a/drivers/leds/leds-pca995x.c b/drivers/leds/leds-pca995x.c
> index fee6216cd1bd0..8eb9847b9709f 100644
> --- a/drivers/leds/leds-pca995x.c
> +++ b/drivers/leds/leds-pca995x.c
> @@ -8,6 +8,7 @@
> * Copyright 2023 Marek Vasut
> */
>
> +#include <linux/gpio.h>
[Severity: Low]
Does this code use the legacy <linux/gpio.h> header instead of
<linux/gpio/consumer.h>?
The kernel typically forbids including <linux/gpio.h> in consumer drivers.
Since this driver only uses descriptor-based APIs like
devm_gpiod_get_optional() and gpiod_set_value_cansleep(), relying on
<linux/gpio.h> can cause implicit declaration build errors if
CONFIG_GPIOLIB is disabled.
[ ... ]
> @@ -130,6 +132,16 @@ static int pca995x_probe(struct i2c_client *client)
> if (!dev_fwnode(dev))
> return -ENODEV;
>
> + reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(reset_gpio))
> + return dev_err_probe(dev, PTR_ERR(reset_gpio),
> + "failed to request reset GPIO\n");
> + if (reset_gpio) {
> + usleep_range(3, 4);
[Severity: Low]
Is usleep_range() appropriate for a microsecond delay under 10us here?
According to the kernel timers-howto, usleep_range() is designed for
delays greater than 10us due to timer setup and context switching overhead.
For delays as short as 3-4us, udelay() is generally recommended to avoid
unnecessary performance inefficiency.
[Severity: Low]
Is there a missing #include <linux/delay.h> for this usleep_range() call?
Relying on implicit includes from other headers (like <linux/i2c.h>) is
fragile and could cause build breakages on certain architectures or
configurations.
> + gpiod_set_value_cansleep(reset_gpio, 0);
> + usleep_range(1500, 1600);
> + }
> +
> chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
--
Sashiko AI review · https://sashiko.dev/#/patchset/6d3600f99c77c31166f0b6a5ccfb7f518b53c4c9.1788350618.git.nora.schiffer@ew.tq-group.com?part=3
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 4/7] leds: pca995x: make output gain configurable
2026-09-02 12:22 [PATCH 1/7] dt-bindings: leds: pca995x: describe reset-gpios property Nora Schiffer
2026-09-02 12:22 ` [PATCH 2/7] dt-bindings: leds: pca995x: describe output-gain property Nora Schiffer
2026-09-02 12:22 ` [PATCH 3/7] leds: pca995x: add support for reset GPIO Nora Schiffer
@ 2026-09-02 12:22 ` Nora Schiffer
2026-09-02 12:35 ` sashiko-bot
2026-09-02 12:22 ` [PATCH 5/7] leds: pca995x: add sysfs files for error reporting Nora Schiffer
` (3 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Nora Schiffer @ 2026-09-02 12:22 UTC (permalink / raw)
To: Lee Jones, Pavel Machek
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Isai Gaspar,
linux, linux-leds, devicetree, linux-kernel, Nora Schiffer
The output gain linearly configures the LED current in 255 levels; the
absolute current depends on both the gain value and the R_ext resistor
connected to the LED driver. Make the gain configurable in the Device
Tree instead of hardcoding it to 128, allowing to adjust the current
for different hardware designs.
Signed-off-by: Nora Schiffer <nora.schiffer@ew.tq-group.com>
---
drivers/leds/leds-pca995x.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/leds/leds-pca995x.c b/drivers/leds/leds-pca995x.c
index 8eb9847b9709f..b069b93d39a54 100644
--- a/drivers/leds/leds-pca995x.c
+++ b/drivers/leds/leds-pca995x.c
@@ -126,6 +126,7 @@ static int pca995x_probe(struct i2c_client *client)
struct pca995x_chip *chip;
struct pca995x_led *led;
int i, j, reg, ret;
+ u32 iref;
chipdef = device_get_match_data(&client->dev);
@@ -153,6 +154,11 @@ static int pca995x_probe(struct i2c_client *client)
i2c_set_clientdata(client, chip);
+ if (device_property_read_u32(dev, "output-gain", &iref))
+ iref = PCA995X_IREFALL_HALF_CFG;
+ else if (iref > PCA995X_IREFALL_FULL_CFG)
+ return dev_err_probe(dev, -EINVAL, "invalid output-gain\n");
+
device_for_each_child_node_scoped(dev, child) {
ret = fwnode_property_read_u32(child, "reg", ®);
if (ret)
@@ -195,7 +201,7 @@ static int pca995x_probe(struct i2c_client *client)
return ret;
/* IREF Output current value for all LEDn outputs */
- return regmap_write(chip->regmap, chipdef->irefall, PCA995X_IREFALL_HALF_CFG);
+ return regmap_write(chip->regmap, chipdef->irefall, iref);
}
static const struct i2c_device_id pca995x_id[] = {
--
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/
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 4/7] leds: pca995x: make output gain configurable
2026-09-02 12:22 ` [PATCH 4/7] leds: pca995x: make output gain configurable Nora Schiffer
@ 2026-09-02 12:35 ` sashiko-bot
0 siblings, 0 replies; 19+ messages in thread
From: sashiko-bot @ 2026-09-02 12:35 UTC (permalink / raw)
To: Nora Schiffer; +Cc: conor+dt, linux-leds, lee, robh, devicetree
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The patch introduces an un-prefixed, hardware-specific Device Tree property `output-gain` that exposes a raw register value instead of using a standard physical property or a vendor prefix.
- [Medium] Mismatch between the driver fallback logic and the commit message regarding the default `output-gain` value.
- [Medium] The commit message description is not capitalized, violating the LED subsystem commit message formatting rules.
--
commit 68a1c0b2bd97402769422bdcabf4d7451b72efcb
Author: Nora Schiffer <nora.schiffer@ew.tq-group.com>
leds: pca995x: make output gain configurable
This commit makes the output gain of the pca995x LED driver configurable
via the Device Tree. It allows reading a value to adjust the LED current
for different hardware designs instead of using a hardcoded value.
> leds: pca995x: make output gain configurable
[Severity: Medium]
This isn't a bug, but the commit message subject does not follow the LED
subsystem's capitalization rules. Should the description after the prefix
be capitalized, for example "leds: pca995x: Make output gain configurable"?
> diff --git a/drivers/leds/leds-pca995x.c b/drivers/leds/leds-pca995x.c
> index 8eb9847b9709f..b069b93d39a54 100644
> --- a/drivers/leds/leds-pca995x.c
> +++ b/drivers/leds/leds-pca995x.c
[ ... ]
> @@ -153,6 +154,11 @@ static int pca995x_probe(struct i2c_client *client)
>
> i2c_set_clientdata(client, chip);
>
> + if (device_property_read_u32(dev, "output-gain", &iref))
[Severity: Medium]
Does introducing the un-prefixed "output-gain" property pollute the global
Device Tree namespace? Should this use a standard physical property or a
vendor prefix instead, to avoid long-term ABI maintenance debt?
> + iref = PCA995X_IREFALL_HALF_CFG;
[Severity: Medium]
The commit message explicitly claims the default output gain value was
hardcoded to 128. However, PCA995X_IREFALL_HALF_CFG is used here as the
fallback, which evaluates to 127. Is this mismatch between the commit
message and the implemented logic intended?
--
Sashiko AI review · https://sashiko.dev/#/patchset/6d3600f99c77c31166f0b6a5ccfb7f518b53c4c9.1788350618.git.nora.schiffer@ew.tq-group.com?part=4
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 5/7] leds: pca995x: add sysfs files for error reporting
2026-09-02 12:22 [PATCH 1/7] dt-bindings: leds: pca995x: describe reset-gpios property Nora Schiffer
` (2 preceding siblings ...)
2026-09-02 12:22 ` [PATCH 4/7] leds: pca995x: make output gain configurable Nora Schiffer
@ 2026-09-02 12:22 ` Nora Schiffer
2026-09-02 12:40 ` sashiko-bot
2026-09-02 12:22 ` [PATCH 6/7] leds: pca995x: do not use full on LED mode Nora Schiffer
` (2 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Nora Schiffer @ 2026-09-02 12:22 UTC (permalink / raw)
To: Lee Jones, Pavel Machek
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Isai Gaspar,
linux, linux-leds, devicetree, linux-kernel, Nora Schiffer
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 | 103 +++++++++++++++++++++++++++++++++++-
1 file changed, 102 insertions(+), 1 deletion(-)
diff --git a/drivers/leds/leds-pca995x.c b/drivers/leds/leds-pca995x.c
index b069b93d39a54..2232b74528a53 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/gpio.h>
#include <linux/i2c.h>
#include <linux/leds.h>
@@ -23,6 +24,9 @@
/* Auto-increment disabled. Normal mode */
#define PCA995X_MODE1_CFG 0x00
+#define PCA995X_MODE2_CLRERR BIT(4)
+#define PCA995X_MODE2_ERROR BIT(6)
+
/* LED select registers determine the source that drives LED outputs */
#define PCA995X_LED_OFF 0x0
#define PCA995X_LED_ON 0x1
@@ -36,30 +40,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 {
@@ -111,6 +122,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));
+ unsigned int val;
+ int ret;
+
+ ret = regmap_read(chip->regmap, PCA995X_MODE2, &val);
+ if (ret)
+ return ret;
+
+
+ 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));
+ int ret;
+
+ if (strcmp(buf, "clear\n"))
+ 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,
@@ -173,6 +261,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;
}
for (i = 0; i < PCA995X_MAX_OUTPUTS; i++) {
@@ -201,7 +290,18 @@ static int pca995x_probe(struct i2c_client *client)
return ret;
/* IREF Output current value for all LEDn outputs */
- return regmap_write(chip->regmap, chipdef->irefall, iref);
+ ret = regmap_write(chip->regmap, chipdef->irefall, iref);
+ if (ret)
+ return ret;
+
+ return sysfs_create_groups(&dev->kobj, pca995x_groups);
+}
+
+static void pca995x_remove(struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+
+ return sysfs_remove_groups(&dev->kobj, pca995x_groups);
}
static const struct i2c_device_id pca995x_id[] = {
@@ -226,6 +326,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/
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 5/7] leds: pca995x: add sysfs files for error reporting
2026-09-02 12:22 ` [PATCH 5/7] leds: pca995x: add sysfs files for error reporting Nora Schiffer
@ 2026-09-02 12:40 ` sashiko-bot
0 siblings, 0 replies; 19+ messages in thread
From: sashiko-bot @ 2026-09-02 12:40 UTC (permalink / raw)
To: Nora Schiffer; +Cc: devicetree, robh, linux-leds, lee, conor+dt
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The `has_errors_store` function uses `strcmp()` to perform an exact match on "clear\n", which incorrectly rejects valid sysfs writes that omit a trailing newline.
- [Low] The commit message description is not capitalized, violating subsystem guidelines.
--
commit 663ea7a7fd850dadfe05eb1aef14f297e78e15b5
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.
> leds: pca995x: add sysfs files for error reporting
[Severity: Low]
This isn't a bug, but the commit message description starts with a lowercase
letter. The LED subsystem guidelines request capitalizing the description
after the subsystem prefix.
> diff --git a/drivers/leds/leds-pca995x.c b/drivers/leds/leds-pca995x.c
> index b069b93d39a54..2232b74528a53 100644
> --- a/drivers/leds/leds-pca995x.c
> +++ b/drivers/leds/leds-pca995x.c
[ ... ]
> +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));
> + int ret;
> +
> + if (strcmp(buf, "clear\n"))
> + return -EINVAL;
[Severity: Medium]
Does this exact string match in has_errors_store() reject valid sysfs writes
that omit a trailing newline?
If userspace writes exactly the word clear without a newline
(for example, using echo -n clear > has_errors), strcmp will fail and return
-EINVAL. Using sysfs_streq() instead of strcmp() would transparently handle
the presence or absence of a trailing newline from sysfs inputs.
> +
> + ret = regmap_update_bits(chip->regmap, PCA995X_MODE2,
> + PCA995X_MODE2_CLRERR, PCA995X_MODE2_CLRERR);
> +
> + return ret ?: count;
> +}
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/6d3600f99c77c31166f0b6a5ccfb7f518b53c4c9.1788350618.git.nora.schiffer@ew.tq-group.com?part=5
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 6/7] leds: pca995x: do not use full on LED mode
2026-09-02 12:22 [PATCH 1/7] dt-bindings: leds: pca995x: describe reset-gpios property Nora Schiffer
` (3 preceding siblings ...)
2026-09-02 12:22 ` [PATCH 5/7] leds: pca995x: add sysfs files for error reporting Nora Schiffer
@ 2026-09-02 12:22 ` Nora Schiffer
2026-09-02 12:39 ` sashiko-bot
2026-09-02 12:22 ` [PATCH 7/7] leds: pca995x: add support for group brightness control Nora Schiffer
2026-09-02 12:37 ` [PATCH 1/7] dt-bindings: leds: pca995x: describe reset-gpios property sashiko-bot
6 siblings, 1 reply; 19+ messages in thread
From: Nora Schiffer @ 2026-09-02 12:22 UTC (permalink / raw)
To: Lee Jones, Pavel Machek
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Isai Gaspar,
linux, linux-leds, devicetree, linux-kernel, Nora Schiffer
LEDs in full on mode are unaffected by group dimming. Avoid using it,
so group dimming is applied consistently.
This slightly changes the brightness at maximum (from 100% to 255/256
duty cycle), which should be imperceptible; arguably it is also more
consistent with the other brightness levels, as it will now scale
linearly from 0/256 to 255/256 over the whole range.
Signed-off-by: Nora Schiffer <nora.schiffer@ew.tq-group.com>
---
drivers/leds/leds-pca995x.c | 33 ++++++++++++++-------------------
1 file changed, 14 insertions(+), 19 deletions(-)
diff --git a/drivers/leds/leds-pca995x.c b/drivers/leds/leds-pca995x.c
index 2232b74528a53..62f2764661b9d 100644
--- a/drivers/leds/leds-pca995x.c
+++ b/drivers/leds/leds-pca995x.c
@@ -98,28 +98,23 @@ static int pca995x_brightness_set(struct led_classdev *led_cdev,
ledout_addr = PCA995X_LEDOUT0 + (led->led_no / PCA995X_OUTPUTS_PER_REG);
shift = PCA995X_LDRX_BITS * (led->led_no % PCA995X_OUTPUTS_PER_REG);
- switch (brightness) {
- case LED_FULL:
- return regmap_update_bits(chip->regmap, ledout_addr,
- PCA995X_LDRX_MASK << shift,
- PCA995X_LED_ON << shift);
- case LED_OFF:
+ if (brightness == LED_OFF) {
return regmap_update_bits(chip->regmap, ledout_addr,
PCA995X_LDRX_MASK << shift, 0);
- default:
- /* Adjust brightness as per user input by changing individual PWM */
- ret = regmap_write(chip->regmap, pwmout_addr, brightness);
- if (ret)
- return ret;
-
- /*
- * 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);
}
+
+ /* Adjust brightness as per user input by changing individual PWM */
+ ret = regmap_write(chip->regmap, pwmout_addr, brightness);
+ if (ret)
+ return ret;
+
+ /*
+ * 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);
}
static ssize_t status_show(struct device *dev, struct device_attribute *attr, char *buf)
--
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/
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 7/7] leds: pca995x: add support for group brightness control
2026-09-02 12:22 [PATCH 1/7] dt-bindings: leds: pca995x: describe reset-gpios property Nora Schiffer
` (4 preceding siblings ...)
2026-09-02 12:22 ` [PATCH 6/7] leds: pca995x: do not use full on LED mode Nora Schiffer
@ 2026-09-02 12:22 ` Nora Schiffer
2026-09-02 12:43 ` sashiko-bot
2026-09-02 12:37 ` [PATCH 1/7] dt-bindings: leds: pca995x: describe reset-gpios property sashiko-bot
6 siblings, 1 reply; 19+ messages in thread
From: Nora Schiffer @ 2026-09-02 12:22 UTC (permalink / raw)
To: Lee Jones, Pavel Machek
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Isai Gaspar,
linux, linux-leds, devicetree, linux-kernel, Nora Schiffer
When LEDs are set to PWM mode with group control enabled, their
brightness can control using a global "group PWM" setting, modulating
the individual LEDs' brightness with a second PWM running at a different
frequency. This lowers the minimum duty cycle from 1/256 to 1/65536
(averaged over the modulated signal). Group brightness control is
particularly useful to adjust for different levels of ambient light.
For simplicity, group PWM mode is always enabled, with the reset default
of 255 as group brightness. This reduces the effective duty cycle by
1/256 at all individual brightness levels (the individual PWM signals
are modulated with the 255/256 duty cycle group PWM), which should be
imperceptible.
Signed-off-by: Nora Schiffer <nora.schiffer@ew.tq-group.com>
---
drivers/leds/leds-pca995x.c | 39 +++++++++++++++++++++++++++++++++++--
1 file changed, 37 insertions(+), 2 deletions(-)
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
@@ -31,6 +31,7 @@
#define PCA995X_LED_OFF 0x0
#define PCA995X_LED_ON 0x1
#define PCA995X_LED_PWM_MODE 0x2
+#define PCA995X_LED_PWM_MODE_GRP 0x3
#define PCA995X_LDRX_MASK 0x3
#define PCA995X_LDRX_BITS 2
@@ -48,6 +49,7 @@
struct pca995x_chipdef {
unsigned int num_leds;
u8 pwm_base;
+ u8 grppwm;
u8 irefall;
u8 eflag_base;
};
@@ -55,6 +57,7 @@ struct pca995x_chipdef {
static const struct pca995x_chipdef pca9952_chipdef = {
.num_leds = 16,
.pwm_base = 0x0a,
+ .grppwm = 0x08,
.irefall = 0x43,
.eflag_base = 0x44,
};
@@ -62,6 +65,7 @@ static const struct pca995x_chipdef pca9952_chipdef = {
static const struct pca995x_chipdef pca9955b_chipdef = {
.num_leds = 16,
.pwm_base = 0x08,
+ .grppwm = 0x06,
.irefall = 0x45,
.eflag_base = 0x46,
};
@@ -69,6 +73,7 @@ static const struct pca995x_chipdef pca9955b_chipdef = {
static const struct pca995x_chipdef pca9956b_chipdef = {
.num_leds = 24,
.pwm_base = 0x0a,
+ .grppwm = 0x08,
.irefall = 0x40,
.eflag_base = 0x41,
};
@@ -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);
}
static ssize_t status_show(struct device *dev, struct device_attribute *attr, char *buf)
@@ -186,10 +190,41 @@ static ssize_t has_errors_store(struct device *dev, struct device_attribute *att
return ret ?: count;
}
+static ssize_t group_brightness_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ struct pca995x_chip *chip = i2c_get_clientdata(to_i2c_client(dev));
+ unsigned int val;
+ int ret;
+
+ ret = regmap_read(chip->regmap, chip->chipdef->grppwm, &val);
+ if (ret)
+ return ret;
+
+ return sysfs_emit(buf, "%u\n", val);
+}
+
+static ssize_t group_brightness_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));
+ u8 val;
+ int ret;
+
+ ret = kstrtou8(buf, 0, &val);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(chip->regmap, chip->chipdef->grppwm, val);
+
+ return ret ?: count;
+}
+
static DEVICE_ATTR_RW(has_errors);
+static DEVICE_ATTR_RW(group_brightness);
static struct attribute *pca995x_attrs[] = {
&dev_attr_has_errors.attr,
+ &dev_attr_group_brightness.attr,
NULL,
};
ATTRIBUTE_GROUPS(pca995x);
--
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/
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 7/7] leds: pca995x: add support for group brightness control
2026-09-02 12:22 ` [PATCH 7/7] leds: pca995x: add support for group brightness control Nora Schiffer
@ 2026-09-02 12:43 ` sashiko-bot
0 siblings, 0 replies; 19+ messages in thread
From: sashiko-bot @ 2026-09-02 12:43 UTC (permalink / raw)
To: Nora Schiffer; +Cc: devicetree, conor+dt, robh, linux-leds, lee
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/7] dt-bindings: leds: pca995x: describe reset-gpios property
2026-09-02 12:22 [PATCH 1/7] dt-bindings: leds: pca995x: describe reset-gpios property Nora Schiffer
` (5 preceding siblings ...)
2026-09-02 12:22 ` [PATCH 7/7] leds: pca995x: add support for group brightness control Nora Schiffer
@ 2026-09-02 12:37 ` sashiko-bot
2026-09-02 16:16 ` Lee Jones
6 siblings, 1 reply; 19+ messages in thread
From: sashiko-bot @ 2026-09-02 12:37 UTC (permalink / raw)
To: Nora Schiffer; +Cc: linux-leds, conor+dt, devicetree, robh, lee
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [Low] The commit message description is not capitalized, violating the LED subsystem guidelines.
Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the driver validates the child `reg` property against `PCA995X_MAX_OUTPUTS` (24) instead of the chip-specific `num_leds`. This allows 16-channel variants to configure out-of-bounds LEDs.
--
commit ead1262f70ad0044c0eca7c693a6d898fbcc4d44
Author: Nora Schiffer <nora.schiffer@ew.tq-group.com>
dt-bindings: leds: pca995x: describe reset-gpios property
This commit adds the reset-gpios property to the PCA995x device tree binding.
It allows the reset pin of the PCA995x to be described in the Device Tree.
> dt-bindings: leds: pca995x: describe reset-gpios property
[Severity: Low]
This isn't a bug, but does this commit message subject violate the LED
subsystem capitalization rules? The description after the subsystem prefix
should be capitalized.
This is a pre-existing issue, but the driver validates the child reg
property against PCA995X_MAX_OUTPUTS (24) instead of the chip-specific
num_leds:
drivers/leds/leds-pca995x.c:pca995x_probe() {
...
device_for_each_child_node_scoped(dev, child) {
ret = fwnode_property_read_u32(child, "reg", ®);
if (ret)
return ret;
if (reg < 0 || reg >= PCA995X_MAX_OUTPUTS || led_fwnodes[reg])
return -EINVAL;
...
}
[Severity: High]
Does this allow 16-channel variants to configure out-of-bounds LEDs?
Changing the brightness of out-of-bounds LED channels (16-23) on 16-channel
variants (e.g. PCA9955B) calculates a pwmout_addr that overlaps with the
IREF registers (output current control) for channels 0-7.
Could this cause silent hardware state corruption and potential physical
damage to LEDs?
--
Sashiko AI review · https://sashiko.dev/#/patchset/6d3600f99c77c31166f0b6a5ccfb7f518b53c4c9.1788350618.git.nora.schiffer@ew.tq-group.com?part=1
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 1/7] dt-bindings: leds: pca995x: describe reset-gpios property
2026-09-02 12:37 ` [PATCH 1/7] dt-bindings: leds: pca995x: describe reset-gpios property sashiko-bot
@ 2026-09-02 16:16 ` Lee Jones
2026-09-03 6:42 ` Nora Schiffer
0 siblings, 1 reply; 19+ messages in thread
From: Lee Jones @ 2026-09-02 16:16 UTC (permalink / raw)
To: sashiko-reviews; +Cc: Nora Schiffer, linux-leds, conor+dt, devicetree, robh
On Wed, 02 Sep 2026, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> New issues:
> - [Low] The commit message description is not capitalized, violating the LED subsystem guidelines.
Please review, explain or fix the Sashiko review(s).
If this is a set, this request includes the other patches too.
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but the driver validates the child `reg` property against `PCA995X_MAX_OUTPUTS` (24) instead of the chip-specific `num_leds`. This allows 16-channel variants to configure out-of-bounds LEDs.
> --
>
> commit ead1262f70ad0044c0eca7c693a6d898fbcc4d44
> Author: Nora Schiffer <nora.schiffer@ew.tq-group.com>
>
> dt-bindings: leds: pca995x: describe reset-gpios property
>
> This commit adds the reset-gpios property to the PCA995x device tree binding.
> It allows the reset pin of the PCA995x to be described in the Device Tree.
>
> > dt-bindings: leds: pca995x: describe reset-gpios property
>
> [Severity: Low]
> This isn't a bug, but does this commit message subject violate the LED
> subsystem capitalization rules? The description after the subsystem prefix
> should be capitalized.
>
> This is a pre-existing issue, but the driver validates the child reg
> property against PCA995X_MAX_OUTPUTS (24) instead of the chip-specific
> num_leds:
>
> drivers/leds/leds-pca995x.c:pca995x_probe() {
> ...
> device_for_each_child_node_scoped(dev, child) {
> ret = fwnode_property_read_u32(child, "reg", ®);
> if (ret)
> return ret;
>
> if (reg < 0 || reg >= PCA995X_MAX_OUTPUTS || led_fwnodes[reg])
> return -EINVAL;
> ...
> }
>
> [Severity: High]
> Does this allow 16-channel variants to configure out-of-bounds LEDs?
>
> Changing the brightness of out-of-bounds LED channels (16-23) on 16-channel
> variants (e.g. PCA9955B) calculates a pwmout_addr that overlaps with the
> IREF registers (output current control) for channels 0-7.
>
> Could this cause silent hardware state corruption and potential physical
> damage to LEDs?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/6d3600f99c77c31166f0b6a5ccfb7f518b53c4c9.1788350618.git.nora.schiffer@ew.tq-group.com?part=1
--
Lee Jones
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 1/7] dt-bindings: leds: pca995x: describe reset-gpios property
2026-09-02 16:16 ` Lee Jones
@ 2026-09-03 6:42 ` Nora Schiffer
0 siblings, 0 replies; 19+ messages in thread
From: Nora Schiffer @ 2026-09-03 6:42 UTC (permalink / raw)
To: Lee Jones, sashiko-reviews; +Cc: linux-leds, conor+dt, devicetree, robh
On Wed, 2026-09-02 at 17:16 +0100, Lee Jones wrote:
> On Wed, 02 Sep 2026, sashiko-bot@kernel.org wrote:
>
> > Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> >
> > New issues:
> > - [Low] The commit message description is not capitalized, violating the LED subsystem guidelines.
>
> Please review, explain or fix the Sashiko review(s).
Hi Lee,
please see my reply to the Sashiko review on PATCH 2/7, as that one requires a
decision how we want to model the output-gain in the Device Tree. All other
Sashiko review comments look correct and will be addressed in v2.
Best,
Nora
>
> If this is a set, this request includes the other patches too.
>
> > Pre-existing issues:
> > - [High] This problem wasn't introduced by this patch, but the driver validates the child `reg` property against `PCA995X_MAX_OUTPUTS` (24) instead of the chip-specific `num_leds`. This allows 16-channel variants to configure out-of-bounds LEDs.
> > --
> >
> > commit ead1262f70ad0044c0eca7c693a6d898fbcc4d44
> > Author: Nora Schiffer <nora.schiffer@ew.tq-group.com>
> >
> > dt-bindings: leds: pca995x: describe reset-gpios property
> >
> > This commit adds the reset-gpios property to the PCA995x device tree binding.
> > It allows the reset pin of the PCA995x to be described in the Device Tree.
> >
> > > dt-bindings: leds: pca995x: describe reset-gpios property
> >
> > [Severity: Low]
> > This isn't a bug, but does this commit message subject violate the LED
> > subsystem capitalization rules? The description after the subsystem prefix
> > should be capitalized.
> >
> > This is a pre-existing issue, but the driver validates the child reg
> > property against PCA995X_MAX_OUTPUTS (24) instead of the chip-specific
> > num_leds:
> >
> > drivers/leds/leds-pca995x.c:pca995x_probe() {
> > ...
> > device_for_each_child_node_scoped(dev, child) {
> > ret = fwnode_property_read_u32(child, "reg", ®);
> > if (ret)
> > return ret;
> >
> > if (reg < 0 || reg >= PCA995X_MAX_OUTPUTS || led_fwnodes[reg])
> > return -EINVAL;
> > ...
> > }
> >
> > [Severity: High]
> > Does this allow 16-channel variants to configure out-of-bounds LEDs?
> >
> > Changing the brightness of out-of-bounds LED channels (16-23) on 16-channel
> > variants (e.g. PCA9955B) calculates a pwmout_addr that overlaps with the
> > IREF registers (output current control) for channels 0-7.
> >
> > Could this cause silent hardware state corruption and potential physical
> > damage to LEDs?
> >
> > --
> > Sashiko AI review · https://sashiko.dev/#/patchset/6d3600f99c77c31166f0b6a5ccfb7f518b53c4c9.1788350618.git.nora.schiffer@ew.tq-group.com?part=1
>
--
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/
^ permalink raw reply [flat|nested] 19+ messages in thread