* [PATCH v7 1/6] dt-bindings: leds: issi,is31fl32xx: convert the binding to yaml
2026-05-20 13:56 [PATCH v7 0/6] leds: Add powerdown gpio for is31fl32xx Jun Yan
@ 2026-05-20 13:56 ` Jun Yan
2026-05-20 13:56 ` [PATCH v7 2/6] dt-bindings: leds: issi,is31fl32xx: add support for is31fl3236a Jun Yan
` (4 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Jun Yan @ 2026-05-20 13:56 UTC (permalink / raw)
To: linux-kernel, devicetree, linux-leds
Cc: lee, robh, krzk+dt, conor+dt, luccafachinetti, pzalewski, daniel,
Jun Yan
Convert leds-is31fl32xx to DT schema format.
Co-developed-by: Lucca Fachinetti <luccafachinetti@gmail.com>
Signed-off-by: Lucca Fachinetti <luccafachinetti@gmail.com>
Co-developed-by: Pawel Zalewski <pzalewski@thegoodpenguin.co.uk>
Signed-off-by: Pawel Zalewski <pzalewski@thegoodpenguin.co.uk>
Signed-off-by: Jun Yan <jerrysteve1101@gmail.com>
---
.../bindings/leds/issi,is31fl32xx.yaml | 151 ++++++++++++++++++
.../bindings/leds/leds-is31fl32xx.txt | 53 ------
2 files changed, 151 insertions(+), 53 deletions(-)
create mode 100644 Documentation/devicetree/bindings/leds/issi,is31fl32xx.yaml
delete mode 100644 Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
diff --git a/Documentation/devicetree/bindings/leds/issi,is31fl32xx.yaml b/Documentation/devicetree/bindings/leds/issi,is31fl32xx.yaml
new file mode 100644
index 000000000000..81f1e30e493d
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/issi,is31fl32xx.yaml
@@ -0,0 +1,151 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/issi,is31fl32xx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: IS31FL32xx and Si-En SN32xx LED controller
+
+maintainers:
+ - Lucca Fachinetti <luccafachinetti@gmail.com>
+ - Pavel Machek <pavel@ucw.cz>
+ - Jun Yan <jerrysteve1101@gmail.com>
+
+description: |
+ The IS31FL32xx/SN32xx family of LED drivers are I2C devices with multiple
+ constant-current channels, each with independent 256-level PWM control.
+ Each LED is represented as a sub-node of the device.
+
+ For more product information please see the links below:
+ https://www.lumissil.com/assets/pdf/core/IS31FL3216_DS.pdf
+ https://www.lumissil.com/assets/pdf/core/IS31FL3218_DS.pdf
+ https://www.lumissil.com/assets/pdf/core/IS31FL3235_DS.pdf
+ https://www.lumissil.com/assets/pdf/core/IS31FL3236_DS.pdf
+ https://www.lumissil.com/assets/pdf/core/IS31FL3293_DS.pdf
+
+properties:
+ compatible:
+ enum:
+ - issi,is31fl3216
+ - issi,is31fl3218
+ - issi,is31fl3235
+ - issi,is31fl3236
+ - issi,is31fl3293
+ - si-en,sn3216
+ - si-en,sn3218
+
+ reg:
+ maxItems: 1
+
+ "#address-cells":
+ const: 1
+
+ "#size-cells":
+ const: 0
+
+patternProperties:
+ "^led@([1-9a-f]|1[0-9a-f]|2[0-4])$":
+ type: object
+ $ref: common.yaml#
+ unevaluatedProperties: false
+
+ properties:
+ reg:
+ description:
+ LED channel number (1..N)
+ minimum: 1
+ maximum: 36
+
+ required:
+ - reg
+
+allOf:
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - issi,is31fl3293
+ then:
+ patternProperties:
+ "^led@":
+ properties:
+ reg:
+ maximum: 3
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - issi,is31fl3216
+ - si-en,sn3216
+ then:
+ patternProperties:
+ "^led@":
+ properties:
+ reg:
+ maximum: 16
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - issi,is31fl3218
+ - si-en,sn3218
+ then:
+ patternProperties:
+ "^led@":
+ properties:
+ reg:
+ maximum: 18
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - issi,is31fl3235
+ then:
+ patternProperties:
+ "^led@":
+ properties:
+ reg:
+ maximum: 28
+
+required:
+ - compatible
+ - reg
+ - "#address-cells"
+ - "#size-cells"
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/leds/common.h>
+
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led-controller@3c {
+ compatible = "issi,is31fl3236";
+ reg = <0x3c>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@1 {
+ reg = <1>;
+ color = <LED_COLOR_ID_RED>;
+ function = LED_FUNCTION_STATUS;
+ };
+
+ led@5 {
+ reg = <5>;
+ color = <LED_COLOR_ID_GREEN>;
+ function = LED_FUNCTION_POWER;
+ linux,default-trigger = "default-on";
+ };
+ };
+ };
+...
+
diff --git a/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt b/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
deleted file mode 100644
index 7082ed186dd9..000000000000
--- a/Documentation/devicetree/bindings/leds/leds-is31fl32xx.txt
+++ /dev/null
@@ -1,53 +0,0 @@
-Binding for ISSI IS31FL32xx and Si-En SN32xx LED Drivers
-
-The IS31FL32xx/SN32xx family of LED drivers are I2C devices with multiple
-constant-current channels, each with independent 256-level PWM control.
-Each LED is represented as a sub-node of the device.
-
-Required properties:
-- compatible: one of
- issi,is31fl3236
- issi,is31fl3235
- issi,is31fl3218
- issi,is31fl3216
- issi,is31fl3293
- si-en,sn3218
- si-en,sn3216
-- reg: I2C slave address
-- address-cells : must be 1
-- size-cells : must be 0
-
-LED sub-node properties:
-- reg : LED channel number (1..N)
-- label : (optional)
- see Documentation/devicetree/bindings/leds/common.txt
-- linux,default-trigger : (optional)
- see Documentation/devicetree/bindings/leds/common.txt
-
-
-Example:
-
-is31fl3236: led-controller@3c {
- compatible = "issi,is31fl3236";
- reg = <0x3c>;
- #address-cells = <1>;
- #size-cells = <0>;
-
- led@1 {
- reg = <1>;
- label = "EB:blue:usr0";
- };
- led@2 {
- reg = <2>;
- label = "EB:blue:usr1";
- };
- ...
- led@36 {
- reg = <36>;
- label = "EB:blue:usr35";
- };
-};
-
-For more product information please see the links below:
-http://www.issi.com/US/product-analog-fxled-driver.shtml
-http://www.si-en.com/product.asp?parentid=890
--
2.54.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v7 2/6] dt-bindings: leds: issi,is31fl32xx: add support for is31fl3236a
2026-05-20 13:56 [PATCH v7 0/6] leds: Add powerdown gpio for is31fl32xx Jun Yan
2026-05-20 13:56 ` [PATCH v7 1/6] dt-bindings: leds: issi,is31fl32xx: convert the binding to yaml Jun Yan
@ 2026-05-20 13:56 ` Jun Yan
2026-05-20 13:56 ` [PATCH v7 3/6] dt-bindings: leds: issi,is31fl32xx: Add powerdown-gpios property Jun Yan
` (3 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Jun Yan @ 2026-05-20 13:56 UTC (permalink / raw)
To: linux-kernel, devicetree, linux-leds
Cc: lee, robh, krzk+dt, conor+dt, luccafachinetti, pzalewski, daniel,
Jun Yan
Add an additional and optional control property for setting
the output PWM frequency to 22kHz that exists on is31fl3236a.
The default is 3kHz and this option puts the operational frequency
outside of the audible range.
Co-developed-by: Pawel Zalewski <pzalewski@thegoodpenguin.co.uk>
Signed-off-by: Pawel Zalewski <pzalewski@thegoodpenguin.co.uk>
Signed-off-by: Jun Yan <jerrysteve1101@gmail.com>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
---
.../bindings/leds/issi,is31fl32xx.yaml | 24 +++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/Documentation/devicetree/bindings/leds/issi,is31fl32xx.yaml b/Documentation/devicetree/bindings/leds/issi,is31fl32xx.yaml
index 81f1e30e493d..5f22d8b77edd 100644
--- a/Documentation/devicetree/bindings/leds/issi,is31fl32xx.yaml
+++ b/Documentation/devicetree/bindings/leds/issi,is31fl32xx.yaml
@@ -21,6 +21,7 @@ description: |
https://www.lumissil.com/assets/pdf/core/IS31FL3218_DS.pdf
https://www.lumissil.com/assets/pdf/core/IS31FL3235_DS.pdf
https://www.lumissil.com/assets/pdf/core/IS31FL3236_DS.pdf
+ https://www.lumissil.com/assets/pdf/core/IS31FL3236A_DS.pdf
https://www.lumissil.com/assets/pdf/core/IS31FL3293_DS.pdf
properties:
@@ -30,6 +31,7 @@ properties:
- issi,is31fl3218
- issi,is31fl3235
- issi,is31fl3236
+ - issi,is31fl3236a
- issi,is31fl3293
- si-en,sn3216
- si-en,sn3218
@@ -37,6 +39,12 @@ properties:
reg:
maxItems: 1
+ issi,22khz-pwm:
+ type: boolean
+ description:
+ When present, the chip's PWM will operate at ~22kHz as opposed
+ to ~3kHz to move the operating frequency out of the audible range.
+
"#address-cells":
const: 1
@@ -60,6 +68,22 @@ patternProperties:
- reg
allOf:
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - issi,is31fl3216
+ - issi,is31fl3218
+ - issi,is31fl3235
+ - issi,is31fl3236
+ - issi,is31fl3293
+ - si-en,sn3216
+ - si-en,sn3218
+ then:
+ properties:
+ issi,22khz-pwm: false
+
- if:
properties:
compatible:
--
2.54.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v7 3/6] dt-bindings: leds: issi,is31fl32xx: Add powerdown-gpios property
2026-05-20 13:56 [PATCH v7 0/6] leds: Add powerdown gpio for is31fl32xx Jun Yan
2026-05-20 13:56 ` [PATCH v7 1/6] dt-bindings: leds: issi,is31fl32xx: convert the binding to yaml Jun Yan
2026-05-20 13:56 ` [PATCH v7 2/6] dt-bindings: leds: issi,is31fl32xx: add support for is31fl3236a Jun Yan
@ 2026-05-20 13:56 ` Jun Yan
2026-05-20 13:56 ` [PATCH v7 4/6] leds: is31fl32xx: Add powerdown pin for hardware shutdown mode Jun Yan
` (2 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Jun Yan @ 2026-05-20 13:56 UTC (permalink / raw)
To: linux-kernel, devicetree, linux-leds
Cc: lee, robh, krzk+dt, conor+dt, luccafachinetti, pzalewski, daniel,
Jun Yan
The IS31FL32XX series features an SDB shutdown pin.
Driving it low (active low) places the chip into hardware shutdown mode
for power saving, while all register contents are preserved
and registers are not reset.
Add powerdown-gpios property to describe the GPIO connected to the
SDB pin of IS31FL32XX series LED controllers.
Signed-off-by: Jun Yan <jerrysteve1101@gmail.com>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
---
.../devicetree/bindings/leds/issi,is31fl32xx.yaml | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/Documentation/devicetree/bindings/leds/issi,is31fl32xx.yaml b/Documentation/devicetree/bindings/leds/issi,is31fl32xx.yaml
index 5f22d8b77edd..f33c8305e464 100644
--- a/Documentation/devicetree/bindings/leds/issi,is31fl32xx.yaml
+++ b/Documentation/devicetree/bindings/leds/issi,is31fl32xx.yaml
@@ -45,6 +45,15 @@ properties:
When present, the chip's PWM will operate at ~22kHz as opposed
to ~3kHz to move the operating frequency out of the audible range.
+ powerdown-gpios:
+ maxItems: 1
+ description:
+ GPIO connected to the chip's SDB pin.
+ Driving this GPIO low places the chip into hardware shutdown mode
+ for power saving. All register contents are preserved and registers
+ are not reset during shutdown. The chip exits hardware shutdown mode
+ when the SDB pin is pulled high.
+
"#address-cells":
const: 1
@@ -145,6 +154,7 @@ additionalProperties: false
examples:
- |
+ #include <dt-bindings/gpio/gpio.h>
#include <dt-bindings/leds/common.h>
i2c {
@@ -157,6 +167,8 @@ examples:
#address-cells = <1>;
#size-cells = <0>;
+ powerdown-gpios = <&gpio0 11 GPIO_ACTIVE_LOW>;
+
led@1 {
reg = <1>;
color = <LED_COLOR_ID_RED>;
--
2.54.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v7 4/6] leds: is31fl32xx: Add powerdown pin for hardware shutdown mode
2026-05-20 13:56 [PATCH v7 0/6] leds: Add powerdown gpio for is31fl32xx Jun Yan
` (2 preceding siblings ...)
2026-05-20 13:56 ` [PATCH v7 3/6] dt-bindings: leds: issi,is31fl32xx: Add powerdown-gpios property Jun Yan
@ 2026-05-20 13:56 ` Jun Yan
2026-05-20 15:13 ` sashiko-bot
2026-05-20 13:56 ` [PATCH v7 5/6] leds: is31fl32xx: Fix missing brightness_steps for is31fl3236 Jun Yan
2026-05-20 13:56 ` [PATCH v7 6/6] leds: is31fl32xx: Move pwm frequency setting to init_regs() Jun Yan
5 siblings, 1 reply; 10+ messages in thread
From: Jun Yan @ 2026-05-20 13:56 UTC (permalink / raw)
To: linux-kernel, devicetree, linux-leds
Cc: lee, robh, krzk+dt, conor+dt, luccafachinetti, pzalewski, daniel,
Jun Yan
The IS31FL32XX series features an SDB shutdown pin.
Driving it low (active low) places the chip into hardware shutdown
mode for power saving, while all register contents are preserved
and registers are not reset.
Deassert/assert the pin accordingly during probe() and remove()
to manage hardware shutdown mode.
Signed-off-by: Jun Yan <jerrysteve1101@gmail.com>
---
drivers/leds/leds-is31fl32xx.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/leds/leds-is31fl32xx.c b/drivers/leds/leds-is31fl32xx.c
index fe07acbb103a..5b9ed5a1818f 100644
--- a/drivers/leds/leds-is31fl32xx.c
+++ b/drivers/leds/leds-is31fl32xx.c
@@ -10,6 +10,7 @@
*/
#include <linux/device.h>
+#include <linux/gpio/consumer.h>
#include <linux/i2c.h>
#include <linux/kernel.h>
#include <linux/leds.h>
@@ -60,6 +61,7 @@ struct is31fl32xx_led_data {
struct is31fl32xx_priv {
const struct is31fl32xx_chipdef *cdef;
struct i2c_client *client;
+ struct gpio_desc *powerdown_gpio;
unsigned int num_leds;
struct is31fl32xx_led_data leds[];
};
@@ -421,6 +423,14 @@ static int is31fl32xx_parse_dt(struct device *dev,
const struct is31fl32xx_chipdef *cdef = priv->cdef;
int ret = 0;
+ /* Driving this GPIO line low takes the chip out of shutdown,
+ * as it is flagged as GPIO_ACTIVE_LOW in provider (such as the device tree).
+ */
+ priv->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown", GPIOD_OUT_LOW);
+ if (IS_ERR(priv->powerdown_gpio))
+ return dev_err_probe(dev, PTR_ERR(priv->powerdown_gpio),
+ "Failed to get powerdown gpio\n");
+
if ((cdef->output_frequency_setting_reg != IS31FL32XX_REG_NONE) &&
of_property_read_bool(dev_of_node(dev), "issi,22khz-pwm")) {
@@ -609,6 +619,9 @@ static void is31fl32xx_remove(struct i2c_client *client)
if (ret)
dev_err(&client->dev, "Failed to reset registers on removal (%pe)\n",
ERR_PTR(ret));
+
+ if (priv->powerdown_gpio)
+ gpiod_set_value(priv->powerdown_gpio, 1);
}
/*
--
2.54.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v7 4/6] leds: is31fl32xx: Add powerdown pin for hardware shutdown mode
2026-05-20 13:56 ` [PATCH v7 4/6] leds: is31fl32xx: Add powerdown pin for hardware shutdown mode Jun Yan
@ 2026-05-20 15:13 ` sashiko-bot
0 siblings, 0 replies; 10+ messages in thread
From: sashiko-bot @ 2026-05-20 15:13 UTC (permalink / raw)
To: Jun Yan; +Cc: conor+dt, devicetree, robh
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Asserting the powerdown GPIO in `remove()` powers down the chip before `devres` unregisters the LED class devices, causing I2C transfer failures.
- [Medium] The driver uses `gpiod_set_value()` instead of `gpiod_set_value_cansleep()` to assert the powerdown GPIO, which can trigger a kernel warning.
- [Low] The powerdown GPIO is not asserted on probe failure, leaving the device permanently powered on.
--
commit e29dbb241facd1fc077de99378c689b24d27209d
Author: Jun Yan <jerrysteve1101@gmail.com>
leds: is31fl32xx: Add powerdown pin for hardware shutdown mode
This commit adds support for an optional powerdown GPIO in the IS31FL32XX
LED driver. It asserts the pin during probe and deasserts it during
removal to manage the hardware shutdown mode for power saving.
> diff --git a/drivers/leds/leds-is31fl32xx.c b/drivers/leds/leds-is31fl32xx.c
> index fe07acbb103a8..5b9ed5a1818fa 100644
> --- a/drivers/leds/leds-is31fl32xx.c
> +++ b/drivers/leds/leds-is31fl32xx.c
[ ... ]
> @@ -421,6 +423,14 @@ static int is31fl32xx_parse_dt(struct device *dev,
> const struct is31fl32xx_chipdef *cdef = priv->cdef;
> int ret = 0;
>
> + /* Driving this GPIO line low takes the chip out of shutdown,
> + * as it is flagged as GPIO_ACTIVE_LOW in provider (such as the device tree).
> + */
> + priv->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown", GPIOD_OUT_LOW);
[Severity: Low]
Does this leave the device permanently powered on if probe fails later?
If a failure occurs during probe after the chip is taken out of shutdown,
the devm framework releases the GPIO descriptor but does not revert its
physical line level. A custom devm action might be needed to ensure the
pin is correctly reverted to shutdown mode on probe failure.
> + if (IS_ERR(priv->powerdown_gpio))
> + return dev_err_probe(dev, PTR_ERR(priv->powerdown_gpio),
> + "Failed to get powerdown gpio\n");
[ ... ]
> @@ -609,6 +619,9 @@ static void is31fl32xx_remove(struct i2c_client *client)
> if (ret)
> dev_err(&client->dev, "Failed to reset registers on removal (%pe)\n",
> ERR_PTR(ret));
> +
> + if (priv->powerdown_gpio)
> + gpiod_set_value(priv->powerdown_gpio, 1);
[Severity: Medium]
Will this cause I2C transfer failures during device removal?
The LED class devices are registered with devm_led_classdev_register_ext(),
meaning they are unregistered by devres after is31fl32xx_remove() returns.
When devres later unregisters the LED class devices,
led_classdev_unregister() may try to turn off the LEDs by issuing I2C
transactions. Since the powerdown GPIO was already asserted here, the chip
will be in hardware shutdown and these transactions will result in I2C
NACKs and error logs.
Managing the GPIO teardown via devres could ensure it executes after LED
unregistration.
[Severity: Medium]
Should this use gpiod_set_value_cansleep() instead?
The GPIO might belong to a sleepable controller, such as an I2C or SPI
expander. Since is31fl32xx_remove() executes in a sleepable context (it
calls the sleepable is31fl32xx_reset_regs() just above), calling the
non-sleeping variant on a sleepable GPIO can trigger a WARN_ON() inside
the gpiolib core.
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260520135659.1430008-1-jerrysteve1101@gmail.com?part=4
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v7 5/6] leds: is31fl32xx: Fix missing brightness_steps for is31fl3236
2026-05-20 13:56 [PATCH v7 0/6] leds: Add powerdown gpio for is31fl32xx Jun Yan
` (3 preceding siblings ...)
2026-05-20 13:56 ` [PATCH v7 4/6] leds: is31fl32xx: Add powerdown pin for hardware shutdown mode Jun Yan
@ 2026-05-20 13:56 ` Jun Yan
2026-05-20 15:36 ` sashiko-bot
2026-05-20 13:56 ` [PATCH v7 6/6] leds: is31fl32xx: Move pwm frequency setting to init_regs() Jun Yan
5 siblings, 1 reply; 10+ messages in thread
From: Jun Yan @ 2026-05-20 13:56 UTC (permalink / raw)
To: linux-kernel, devicetree, linux-leds
Cc: lee, robh, krzk+dt, conor+dt, luccafachinetti, pzalewski, daniel,
Jun Yan
Add missing brightness_steps for is31fl3236 to fix brightness control.
Fixes: a18983b95a61 ("leds: is31f132xx: Add support for is31fl3293")
Signed-off-by: Jun Yan <jerrysteve1101@gmail.com>
---
drivers/leds/leds-is31fl32xx.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/leds/leds-is31fl32xx.c b/drivers/leds/leds-is31fl32xx.c
index 5b9ed5a1818f..2d7ff4c5a08c 100644
--- a/drivers/leds/leds-is31fl32xx.c
+++ b/drivers/leds/leds-is31fl32xx.c
@@ -491,6 +491,7 @@ static const struct is31fl32xx_chipdef is31fl3236_cdef = {
.pwm_register_base = 0x01,
.led_control_register_base = 0x26,
.enable_bits_per_led_control_register = 1,
+ .brightness_steps = 256,
};
static const struct is31fl32xx_chipdef is31fl3236a_cdef = {
--
2.54.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v7 5/6] leds: is31fl32xx: Fix missing brightness_steps for is31fl3236
2026-05-20 13:56 ` [PATCH v7 5/6] leds: is31fl32xx: Fix missing brightness_steps for is31fl3236 Jun Yan
@ 2026-05-20 15:36 ` sashiko-bot
0 siblings, 0 replies; 10+ messages in thread
From: sashiko-bot @ 2026-05-20 15:36 UTC (permalink / raw)
To: Jun Yan; +Cc: conor+dt, devicetree, robh
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] Incorrect current limit enforcement for IS31FL3293 overrides lower user limits and causes integer truncation for high values, risking hardware damage.
- [High] The driver exposes LEDs to userspace and the kernel before fully initializing and resetting the hardware.
--
commit 7964dd48aeb77ef5f5990809722be581273eecbc
Author: Jun Yan <jerrysteve1101@gmail.com>
leds: is31fl32xx: Fix missing brightness_steps for is31fl3236
This commit adds the missing brightness_steps field for the is31fl3236
and is31fl3236a chip definitions. This ensures brightness control is
configured properly for these chips.
> diff --git a/drivers/leds/leds-is31fl32xx.c b/drivers/leds/leds-is31fl32xx.c
> index 5b9ed5a1818fa..2d7ff4c5a08c7 100644
> --- a/drivers/leds/leds-is31fl32xx.c
> +++ b/drivers/leds/leds-is31fl32xx.c
> @@ -491,6 +491,7 @@ static const struct is31fl32xx_chipdef is31fl3236_cdef = {
> .pwm_register_base = 0x01,
> .led_control_register_base = 0x26,
> .enable_bits_per_led_control_register = 1,
> + .brightness_steps = 256,
> };
>
> static const struct is31fl32xx_chipdef is31fl3236a_cdef = {
[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential problem
in how we handle the current limits for the is31fl3293 chip.
Looking at is31fl3293_reset() in drivers/leds/leds-is31fl32xx.c:
for (i = 0; i < priv->num_leds; i++) {
...
int microamp = max(led_data->max_microamp, IS31FL3293_MAX_MICROAMP);
int current_level = (microamp * 0xff) / IS31FL3293_MAX_MICROAMP;
ret = is31fl32xx_write(priv, current_level_reg, current_level);
Does this code accidentally use max() instead of min()?
By taking the maximum, any safe limit provided via the device tree that is
lower than 20mA will be overridden to 20mA. Could this risk hardware damage
for chips rated for lower currents?
Additionally, if led_data->max_microamp is greater than 20mA, current_level
will exceed 255. Since is31fl32xx_write() accepts a u8 value, wouldn't this
result in silent integer truncation and unpredictable output currents?
[Severity: High]
This is also a pre-existing issue, but is there a state mismatch during
probe initialization?
Looking at is31fl32xx_probe():
ret = is31fl32xx_parse_dt(dev, priv);
if (ret)
return ret;
ret = is31fl32xx_init_regs(priv);
The is31fl32xx_parse_dt() function registers each led with the subsystem
using devm_led_classdev_register_ext(). This allows the led subsystem to
immediately apply triggers or default-state settings (like 'on' or 'keep')
via i2c writes.
However, right after parsing, we call is31fl32xx_init_regs(), which
performs a full hardware reset. Does this subsequent reset wipe out any
brightness or state configurations just applied by the led subsystem?
Should the hardware initialization occur before exposing the device to the
subsystem?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260520135659.1430008-1-jerrysteve1101@gmail.com?part=5
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v7 6/6] leds: is31fl32xx: Move pwm frequency setting to init_regs()
2026-05-20 13:56 [PATCH v7 0/6] leds: Add powerdown gpio for is31fl32xx Jun Yan
` (4 preceding siblings ...)
2026-05-20 13:56 ` [PATCH v7 5/6] leds: is31fl32xx: Fix missing brightness_steps for is31fl3236 Jun Yan
@ 2026-05-20 13:56 ` Jun Yan
2026-05-20 16:03 ` sashiko-bot
5 siblings, 1 reply; 10+ messages in thread
From: Jun Yan @ 2026-05-20 13:56 UTC (permalink / raw)
To: linux-kernel, devicetree, linux-leds
Cc: lee, robh, krzk+dt, conor+dt, luccafachinetti, pzalewski, daniel,
Jun Yan
commit a18983b95a61 ("leds: is31f132xx: Add support for is31fl3293")
swapped the order of is31fl32xx_parse_dt() and is31fl32xx_init_regs().
This causes the PWM frequency configuration programmed in
is31fl32xx_parse_dt() to be overwritten by the register reset
operation performed in is31fl32xx_init_regs().
Move the PWM frequency setting logic from is31fl32xx_parse_dt() to
is31fl32xx_init_regs() and separates device tree parsing from hardware
initialization.
Fixes: a18983b95a61 ("leds: is31f132xx: Add support for is31fl3293")
Signed-off-by: Jun Yan <jerrysteve1101@gmail.com>
---
drivers/leds/leds-is31fl32xx.c | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/drivers/leds/leds-is31fl32xx.c b/drivers/leds/leds-is31fl32xx.c
index 2d7ff4c5a08c..c545a3b7a05a 100644
--- a/drivers/leds/leds-is31fl32xx.c
+++ b/drivers/leds/leds-is31fl32xx.c
@@ -63,6 +63,7 @@ struct is31fl32xx_priv {
struct i2c_client *client;
struct gpio_desc *powerdown_gpio;
unsigned int num_leds;
+ bool pwm_22khz;
struct is31fl32xx_led_data leds[];
};
@@ -346,6 +347,14 @@ static int is31fl32xx_init_regs(struct is31fl32xx_priv *priv)
if (ret)
return ret;
+ if ((cdef->output_frequency_setting_reg != IS31FL32XX_REG_NONE) &&
+ priv->pwm_22khz) {
+ ret = is31fl32xx_write(priv, cdef->output_frequency_setting_reg,
+ IS31FL32XX_PWM_FREQUENCY_22KHZ);
+ if (ret)
+ return ret;
+ }
+
/*
* Set enable bit for all channels.
* We will control state with PWM registers alone.
@@ -420,7 +429,6 @@ static struct is31fl32xx_led_data *is31fl32xx_find_led_data(
static int is31fl32xx_parse_dt(struct device *dev,
struct is31fl32xx_priv *priv)
{
- const struct is31fl32xx_chipdef *cdef = priv->cdef;
int ret = 0;
/* Driving this GPIO line low takes the chip out of shutdown,
@@ -431,17 +439,7 @@ static int is31fl32xx_parse_dt(struct device *dev,
return dev_err_probe(dev, PTR_ERR(priv->powerdown_gpio),
"Failed to get powerdown gpio\n");
- if ((cdef->output_frequency_setting_reg != IS31FL32XX_REG_NONE) &&
- of_property_read_bool(dev_of_node(dev), "issi,22khz-pwm")) {
-
- ret = is31fl32xx_write(priv, cdef->output_frequency_setting_reg,
- IS31FL32XX_PWM_FREQUENCY_22KHZ);
-
- if (ret) {
- dev_err(dev, "Failed to write output PWM frequency register\n");
- return ret;
- }
- }
+ priv->pwm_22khz = of_property_read_bool(dev_of_node(dev), "issi,22khz-pwm");
for_each_available_child_of_node_scoped(dev_of_node(dev), child) {
struct led_init_data init_data = {};
--
2.54.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v7 6/6] leds: is31fl32xx: Move pwm frequency setting to init_regs()
2026-05-20 13:56 ` [PATCH v7 6/6] leds: is31fl32xx: Move pwm frequency setting to init_regs() Jun Yan
@ 2026-05-20 16:03 ` sashiko-bot
0 siblings, 0 replies; 10+ messages in thread
From: sashiko-bot @ 2026-05-20 16:03 UTC (permalink / raw)
To: Jun Yan; +Cc: robh, conor+dt, devicetree
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues:
- [High] LED class devices are registered and exposed to userspace before the hardware is fully initialized, allowing initial states to be overwritten by the subsequent hardware reset.
- [High] The driver tears down the hardware manually in `remove()` while the LED class devices are still accessible to userspace.
- [High] Incorrect logic when calculating maximum current limits for IS31FL3293 leads to broken limit enforcement and truncation.
--
commit 4a46eb293f0e15dbc00b614e110870654f6c33ca
Author: Jun Yan <jerrysteve1101@gmail.com>
leds: is31fl32xx: Move pwm frequency setting to init_regs()
This commit moves the PWM frequency setting logic from is31fl32xx_parse_dt()
to is31fl32xx_init_regs(). This separates device tree parsing from hardware
initialization and prevents the configuration from being overwritten.
Fixes: a18983b95a61 ("leds: is31f132xx: Add support for is31fl3293")
> diff --git a/drivers/leds/leds-is31fl32xx.c b/drivers/leds/leds-is31fl32xx.c
> index 2d7ff4c5a08c7..c545a3b7a05a1 100644
> --- a/drivers/leds/leds-is31fl32xx.c
> +++ b/drivers/leds/leds-is31fl32xx.c
[ ... ]
> @@ -420,7 +429,6 @@ static struct is31fl32xx_led_data *is31fl32xx_find_led_data(
> static int is31fl32xx_parse_dt(struct device *dev,
> struct is31fl32xx_priv *priv)
> {
> - const struct is31fl32xx_chipdef *cdef = priv->cdef;
> int ret = 0;
>
> /* Driving this GPIO line low takes the chip out of shutdown,
> @@ -431,17 +439,7 @@ static int is31fl32xx_parse_dt(struct device *dev,
> return dev_err_probe(dev, PTR_ERR(priv->powerdown_gpio),
> "Failed to get powerdown gpio\n");
>
> - if ((cdef->output_frequency_setting_reg != IS31FL32XX_REG_NONE) &&
> - of_property_read_bool(dev_of_node(dev), "issi,22khz-pwm")) {
> -
> - ret = is31fl32xx_write(priv, cdef->output_frequency_setting_reg,
> - IS31FL32XX_PWM_FREQUENCY_22KHZ);
> -
> - if (ret) {
> - dev_err(dev, "Failed to write output PWM frequency register\n");
> - return ret;
> - }
> - }
> + priv->pwm_22khz = of_property_read_bool(dev_of_node(dev), "issi,22khz-pwm");
>
> for_each_available_child_of_node_scoped(dev_of_node(dev), child) {
> struct led_init_data init_data = {};
[Severity: High]
This isn't a regression introduced by this patch, but since this commit
relates to initialization order, does registering the LEDs here before
hardware is fully initialized cause a regression?
Looking at the driver sequence, is31fl32xx_parse_dt() registers each LED by
calling devm_led_classdev_register_ext(), exposing them to userspace early.
The LED subsystem then evaluates properties like default-state and triggers,
performing asynchronous I2C writes to set initial brightness.
However, is31fl32xx_init_regs() runs afterward and performs a full register
reset via is31fl32xx_reset_regs().
Could this sequence allow boot-time configurations and early userspace
settings to be lost when the hardware is unconditionally reset?
[Severity: High]
This is also a pre-existing regression, but I noticed the driver relies on
devm_led_classdev_register_ext() for unregistration. Does this create a
race condition during device teardown?
The driver manually resets and powers down the hardware in
is31fl32xx_remove(). Because devres cleanup executes after remove()
completes, the LED devices seem to remain fully accessible to userspace
for a short window.
If a brightness change is requested during this window, would I2C writes be
dispatched to an uninitialized or powered-down chip, potentially causing
bus timeouts or stalls?
While reviewing the hardware initialization logic, I noticed another issue
in the maximum current calculations:
[Severity: High]
This is a pre-existing regression, but does the maximum current calculation
in is31fl3293_reset() correctly enforce the limits?
It calculates the limit using max(led_data->max_microamp,
IS31FL3293_MAX_MICROAMP). If the device tree specifies a limit below the
20mA chip maximum (e.g., 10mA), max() overrides it with 20mA, which defeats
the protection. If a value above 20mA is specified, max() retains the larger
value, causing the resulting current_level calculation to exceed 255.
Since is31fl32xx_write() takes a u8 for the value, this current_level would
be silently truncated (e.g., 382 becomes 126), resulting in an incorrectly
low limit.
Should the driver use min() instead of max() to properly enforce the limit,
and perhaps supply a fallback when the property is 0?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260520135659.1430008-1-jerrysteve1101@gmail.com?part=6
^ permalink raw reply [flat|nested] 10+ messages in thread