leds: Add IS32FL3207 controller support
Subsystems (3)
v7 cover leds: Add IS32FL3207 controller support
Add a binding and LED-class driver for the Lumissil IS32FL3207 18-channel constant-current LED controller. The driver supports individual and multicolor LEDs, per-output current limits, an optional supply, and an optional SDB GPIO.
This is the LED prerequisite split out of the Time Card series. The dependent ptp_ocp board profiles will be resent to net-next after the LED and I2C prerequisites land.
---
Changes since v6:
- Restore the led-max-microamp schema minimum to 1 uA because the smallest representable nonzero current depends on the RISET value.
- Document the RISET-dependent nonzero current-scaling requirement for individual and multicolor outputs.
- Keep the driver patch unchanged.
Krzysztof's Reviewed-by from v3 is not carried because the binding changed materially after that review. It now supports groups beginning at outputs 16 and 17 and documents the RISET/current-limit relationship.
Validation:
- dt-doc-validate
- dt-check-style --mode strict
- yamllint
- git diff --check
- strict checkpatch
- x86-64 Clang syntax check
v6: https://lore.kernel.org/r/[email protected]/
Ahmad Byagowi (2):
dt-bindings: leds: Add IS32FL3207 controller
leds: is32fl3207: Add controller driver
.../bindings/leds/issi,is32fl3207.yaml | 285 +++++++ MAINTAINERS | 7 + drivers/leds/rgb/Kconfig | 11 + drivers/leds/rgb/Makefile | 1 + drivers/leds/rgb/leds-is32fl3207.c | 736 ++++++++++++++++++ 5 files changed, 1040 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/issi,is32fl3207.yaml create mode 100644 drivers/leds/rgb/leds-is32fl3207.c
base-commit: f8cca63a0a4f3475199d9ab7f86782bdfeb0744d
Signature · 1 line
2.50.1 (Apple Git-155)
PATCH v7 1/2 dt-bindings: leds: Add IS32FL3207 controller
The IS32FL3207 is an 18-channel constant-current LED controller with per-output PWM and current scaling.
Describe I2C addressing, power and shutdown controls, RISET, and individual or multicolor output groupings. Require board current limits for each output and document their relationship to the RISET-derived full scale.
Allow dash-suffixed multicolor group names when a group begins at channel 16 or 17, which cannot be represented by the common schema unit-address form.
--- .../bindings/leds/issi,is32fl3207.yaml | 285 ++++++++++++++++++ MAINTAINERS | 6 + 2 files changed, 291 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/issi,is32fl3207.yaml
Documentation/devicetree/bindings/leds/issi,is32fl3207.yaml new file +285 -0
@@ -0,0 +1,285 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/leds/issi,is32fl3207.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Lumissil IS32FL3207 18-channel LED controller + +maintainers: + - Ahmad Byagowi <[email protected]> + +description: | + The IS32FL3207 is an I2C LED controller with 18 individually controlled + constant-current outputs. Each output has 8-bit current scaling. PWM + brightness control uses the controller's default 8-bit, 62 kHz mode. + Outputs may be exposed as individual LEDs or grouped as multicolor LEDs. + + The data sheet is available at: + https://www.lumissil.com/assets/pdf/core/IS32FL3207_DS.pdf + +properties: + compatible: + const: issi,is32fl3207 + + reg: + enum: [0x34, 0x35, 0x36, 0x37] + + vcc-supply: + description: Regulator providing power to the VCC pin. + + enable-gpios: + maxItems: 1 + description: GPIO connected to SDB, asserted to enable the controller. + + issi,riset-ohms: + minimum: 2000 + maximum: 76500000 + description: | + Resistance in ohms of the external RISET resistor. The resistor sets the + full-scale output current according to IOUT(MAX) = 76.5 / RISET amperes. + Each LED's led-max-microamp value selects a board-safe peak current no + greater than this full scale, using the per-output scaling register. + + "#address-cells": + const: 1 + + "#size-cells": + const: 0 + +patternProperties: + "^led@[0-9a-f]+$": + type: object + $ref: common.yaml# + unevaluatedProperties: false + + properties: + reg: + minimum: 0 + maximum: 17 + description: | + Zero-based LED output number. Values 0 through 17 correspond to + OUT1 through OUT18. + + led-max-microamp: + minimum: 1 + maximum: 38250 + description: | + Board-safe peak current for this output. The value must not exceed + the full-scale current derived from issi,riset-ohms and must be high + enough to produce a nonzero per-output current-scaling value for the + configured RISET resistance. + + color: + oneOf: + - minimum: 0 + maximum: 7 + - minimum: 10 + maximum: 14 + + max-brightness: + minimum: 1 + maximum: 255 + + default-state: + description: + The driver performs a software reset and clears the PWM state during + initialization, so an existing state cannot be preserved. + enum: ["on", "off"] + + default-brightness: + maximum: 255 + description: | + Brightness used when default-state is "on". This value must not + exceed max-brightness when max-brightness is specified. + + anyOf: + - required: + - function + - required: + - color + + required: + - reg + - led-max-microamp + + "^multi-led(@[0-9a-f]|-[0-9]+)?$": + type: object + $ref: leds-class-multicolor.yaml# + unevaluatedProperties: false + + properties: + reg: + minimum: 0 + maximum: 15 + description: | + Optional zero-based lowest-numbered output used by a group named + multi-led@N. Values 0 through 15 correspond to OUT1 through OUT16. + For a multi-led-N group without reg, the driver derives the lowest + output from its component nodes. This form permits groups beginning + at channels 16 or 17 while following the common multicolor node-name + schema. + + "#address-cells": + const: 1 + + "#size-cells": + const: 0 + + max-brightness: + minimum: 1 + maximum: 255 + + default-state: + description: + The driver performs a software reset and clears the PWM state during + initialization, so an existing state cannot be preserved. + enum: ["on", "off"] + + default-brightness: + maximum: 255 + description: | + Brightness used when default-state is "on". This value must not + exceed max-brightness when max-brightness is specified. + + patternProperties: + "^led@[0-9a-f]+$": + type: object + $ref: common.yaml# + unevaluatedProperties: false + + properties: + reg: + minimum: 0 + maximum: 17 + description: | + Zero-based LED output number. Values 0 through 17 correspond to + OUT1 through OUT18. + + led-max-microamp: + minimum: 1 + maximum: 38250 + description: | + Board-safe peak current for this output. The value must not + exceed the full-scale current derived from issi,riset-ohms and + must be high enough to produce a nonzero per-output + current-scaling value for the configured RISET resistance. + + color: + oneOf: + - minimum: 0 + maximum: 7 + - minimum: 10 + maximum: 14 + + max-brightness: false + + default-state: false + + default-brightness: false + + retain-state-shutdown: false + + required: + - reg + - color + - led-max-microamp + + anyOf: + - required: [led@0] + - required: [led@1] + - required: [led@2] + - required: [led@3] + - required: [led@4] + - required: [led@5] + - required: [led@6] + - required: [led@7] + - required: [led@8] + - required: [led@9] + - required: [led@a] + - required: [led@b] + - required: [led@c] + - required: [led@d] + - required: [led@e] + - required: [led@f] + - required: [led@10] + - required: [led@11] + + required: + - "#address-cells" + - "#size-cells" + +required: + - compatible + - reg + - issi,riset-ohms + - "#address-cells" + - "#size-cells" + +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + #include <dt-bindings/leds/common.h> + + i2c { + #address-cells = <1>; + #size-cells = <0>; + + led-controller@34 { + compatible = "issi,is32fl3207"; + reg = <0x34>; + #address-cells = <1>; + enable-gpios = <&gpio0 4 GPIO_ACTIVE_HIGH>; + #size-cells = <0>; + vcc-supply = <&led_3v3>; + issi,riset-ohms = <4700>; + + multi-led@0 { + reg = <0>; + #address-cells = <1>; + color = <LED_COLOR_ID_RGB>; + function = LED_FUNCTION_STATUS; + #size-cells = <0>; + + led@0 { + reg = <0>; + color = <LED_COLOR_ID_RED>; + led-max-microamp = <8100>; + }; + + led@1 { + reg = <1>; + color = <LED_COLOR_ID_GREEN>; + led-max-microamp = <8100>; + }; + + led@2 { + reg = <2>; + color = <LED_COLOR_ID_BLUE>; + led-max-microamp = <8100>; + }; + }; + + multi-led-1 { + #address-cells = <1>; + color = <LED_COLOR_ID_MULTI>; + function = LED_FUNCTION_INDICATOR; + #size-cells = <0>; + + led@10 { + reg = <16>; + color = <LED_COLOR_ID_AMBER>; + led-max-microamp = <8100>; + }; + + led@11 { + reg = <17>; + color = <LED_COLOR_ID_WHITE>; + led-max-microamp = <8100>; + }; + }; + }; + }; +...
MAINTAINERS +6 -0
@@ -13817,6 +13817,12 @@ S: Maintained F: Documentation/filesystems/isofs.rst F: fs/isofs/ +ISSI IS32FL3207 LED DRIVER +M: Ahmad Byagowi <[email protected]> +L: [email protected] +S: Maintained +F: Documentation/devicetree/bindings/leds/issi,is32fl3207.yaml + IT87 HARDWARE MONITORING DRIVER M: Jean Delvare <[email protected]> L: [email protected]
Signature · 1 line
2.50.1 (Apple Git-155)
[email protected]
AI reviewRe: [PATCH v7 1/2] dt-bindings: leds: Add IS32FL3207 controller
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 8 lines
The IS32FL3207 is an 18-channel constant-current LED controller with per-output PWM and current scaling.
Describe I2C addressing, power and shutdown controls, RISET, and individual or multicolor output groupings. Require board current limits for each output and document their relationship to the RISET-derived full scale.
Quoted 5 lines
Allow dash-suffixed multicolor group names when a group begins at channel 16 or 17, which cannot be represented by the common schema unit-address form.
Signed-off-by Ahmad Byagowi <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Re: [PATCH v7 1/2] dt-bindings: leds: Add IS32FL3207 controller
In reply to Ahmad ByagowiQuoted 12 lines · Ahmad Byagowi
The IS32FL3207 is an 18-channel constant-current LED controller with per-output PWM and current scaling.
Describe I2C addressing, power and shutdown controls, RISET, and individual or multicolor output groupings. Require board current limits for each output and document their relationship to the RISET-derived full scale.
Allow dash-suffixed multicolor group names when a group begins at channel 16 or 17, which cannot be represented by the common schema unit-address form.
Quoted 6 lines
Signed-off-by Ahmad Byagowi <[email protected]>--- .../bindings/leds/issi,is32fl3207.yaml | 285 ++++++++++++++++++ MAINTAINERS | 6 + 2 files changed, 291 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/issi,is32fl3207.yaml
Best regards,
Krzysztof
PATCH v7 2/2 leds: is32fl3207: Add controller driver
In reply to Ahmad ByagowiAdd an I2C driver for the Lumissil IS32FL3207 18-channel LED controller.
Expose individual and multicolor LEDs through the LED class. Use the default 8-bit, 62 kHz PWM mode, derive output current from RISET, and enforce each output current limit with the scaling registers. Serialize multicolor calculation, scaling, PWM, and controller-wide update operations.
Clear retained scaling while SDB holds the outputs disabled. Release SDB and briefly enable normal operation to issue the required software reset, then keep the controller in software shutdown while registering every LED. Enable outputs only after all limits and initial brightness values are programmed.
Handle an optional supply and enable GPIO. Serialize shutdown against pending brightness updates and honor retained shutdown state.
--- MAINTAINERS | 1 + drivers/leds/rgb/Kconfig | 11 + drivers/leds/rgb/Makefile | 1 + drivers/leds/rgb/leds-is32fl3207.c | 736 +++++++++++++++++++++++++++++ 4 files changed, 749 insertions(+) create mode 100644 drivers/leds/rgb/leds-is32fl3207.c
MAINTAINERS +1 -0
@@ -13822,6 +13822,7 @@ M: Ahmad Byagowi <[email protected]> L: [email protected] S: Maintained F: Documentation/devicetree/bindings/leds/issi,is32fl3207.yaml +F: drivers/leds/rgb/leds-is32fl3207.c IT87 HARDWARE MONITORING DRIVER M: Jean Delvare <[email protected]>
drivers/leds/rgb/Kconfig +11 -0
@@ -14,6 +14,17 @@ config LEDS_GROUP_MULTICOLOR To compile this driver as a module, choose M here: the module will be called leds-group-multicolor. +config LEDS_IS32FL3207 + tristate "LED support for ISSI IS32FL3207" + depends on I2C + select REGMAP_I2C + help + Say Y here to include support for the Lumissil IS32FL3207 + 18-channel I2C LED controller. + + To compile this driver as a module, choose M here: the module will + be called leds-is32fl3207. + config LEDS_KTD202X tristate "LED support for KTD202x Chips" depends on I2C
drivers/leds/rgb/Makefile +1 -0
@@ -1,6 +1,7 @@ # SPDX-License-Identifier: GPL-2.0 obj-$(CONFIG_LEDS_GROUP_MULTICOLOR) += leds-group-multicolor.o +obj-$(CONFIG_LEDS_IS32FL3207) += leds-is32fl3207.o obj-$(CONFIG_LEDS_KTD202X) += leds-ktd202x.o obj-$(CONFIG_LEDS_LP5812) += leds-lp5812.o obj-$(CONFIG_LEDS_LP5860_CORE) += leds-lp5860-core.o
drivers/leds/rgb/leds-is32fl3207.c new file +736 -0
736 lines not on this page · raw message
Signature · 1 line
2.50.1 (Apple Git-155)
[email protected]
AI reviewRe: [PATCH v7 2/2] leds: is32fl3207: Add controller driver
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 14 lines
Add an I2C driver for the Lumissil IS32FL3207 18-channel LED controller.
Expose individual and multicolor LEDs through the LED class. Use the default 8-bit, 62 kHz PWM mode, derive output current from RISET, and enforce each output current limit with the scaling registers. Serialize multicolor calculation, scaling, PWM, and controller-wide update operations.
Clear retained scaling while SDB holds the outputs disabled. Release SDB and briefly enable normal operation to issue the required software reset, then keep the controller in software shutdown while registering every LED. Enable outputs only after all limits and initial brightness values are programmed.
Quoted 4 lines
Handle an optional supply and enable GPIO. Serialize shutdown against pending brightness updates and honor retained shutdown state.
Signed-off-by Ahmad Byagowi <[email protected]>
Sashiko has reviewed this patch and found no issues. It looks great!
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
Lee Jones
maintainerRe: [PATCH v7 2/2] leds: is32fl3207: Add controller driver
In reply to Ahmad ByagowiQuoted 111 lines · Ahmad Byagowi
Add an I2C driver for the Lumissil IS32FL3207 18-channel LED controller.
Expose individual and multicolor LEDs through the LED class. Use the default 8-bit, 62 kHz PWM mode, derive output current from RISET, and enforce each output current limit with the scaling registers. Serialize multicolor calculation, scaling, PWM, and controller-wide update operations.
Clear retained scaling while SDB holds the outputs disabled. Release SDB and briefly enable normal operation to issue the required software reset, then keep the controller in software shutdown while registering every LED. Enable outputs only after all limits and initial brightness values are programmed.
Handle an optional supply and enable GPIO. Serialize shutdown against pending brightness updates and honor retained shutdown state.
Signed-off-by Ahmad Byagowi <[email protected]>--- MAINTAINERS | 1 + drivers/leds/rgb/Kconfig | 11 + drivers/leds/rgb/Makefile | 1 + drivers/leds/rgb/leds-is32fl3207.c | 736 +++++++++++++++++++++++++++++ 4 files changed, 749 insertions(+) create mode 100644 drivers/leds/rgb/leds-is32fl3207.cMAINTAINERS +1
-0@@ -13822,6 +13822,7 @@ M: Ahmad Byagowi <[email protected]> L: [email protected] S: Maintained F: Documentation/devicetree/bindings/leds/issi,is32fl3207.yaml +F: drivers/leds/rgb/leds-is32fl3207.c IT87 HARDWARE MONITORING DRIVER M: Jean Delvare <[email protected]>drivers/leds/rgb/Kconfig' +11
-0@@ -14,6 +14,17 @@ config LEDS_GROUP_MULTICOLOR To compile this driver as a module, choose M here: the module will be called leds-group-multicolor. +config LEDS_IS32FL3207 + tristate "LED support for ISSI IS32FL3207" + depends on I2C + select REGMAP_I2C + help + Say Y here to include support for the Lumissil IS32FL3207 + 18-channel I2C LED controller. + + To compile this driver as a module, choose M here: the module will + be called leds-is32fl3207. + config LEDS_KTD202X tristate "LED support for KTD202x Chips" depends on I2Cdrivers/leds/rgb/Makefile' +1
-0@@ -1,6 +1,7 @@ # SPDX-License-Identifier: GPL-2.0 obj-$(CONFIG_LEDS_GROUP_MULTICOLOR) += leds-group-multicolor.o +obj-$(CONFIG_LEDS_IS32FL3207) += leds-is32fl3207.o obj-$(CONFIG_LEDS_KTD202X) += leds-ktd202x.o obj-$(CONFIG_LEDS_LP5812) += leds-lp5812.o obj-$(CONFIG_LEDS_LP5860_CORE) += leds-lp5860-core.odrivers/leds/rgb/leds-is32fl3207.c' new file +33
-0@@ -0,0 +1,736 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * ISSI IS32FL3207 LED controller driver + * + * Copyright 2026 Ahmad Byagowi + */ + +#include <linux/bitmap.h> +#include <linux/delay.h> +#include <linux/gpio/consumer.h> +#include <linux/i2c.h> +#include <linux/led-class-multicolor.h> +#include <linux/math64.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/property.h> +#include <linux/regmap.h> +#include <linux/regulator/consumer.h> +#include <linux/string.h> + +#include <dt-bindings/leds/common.h> + +#define IS32FL3207_NUM_CHANNELS 18 +#define IS32FL3207_MAX_BRIGHTNESS 255 + +#define IS32FL3207_REG_CONTROL 0x00 +#define IS32FL3207_REG_PWM_LOW(channel) (0x01 + 2 * (channel)) +#define IS32FL3207_REG_PWM_UPDATE 0x49 +#define IS32FL3207_REG_SCALING(channel) (0x4a + (channel)) +#define IS32FL3207_REG_GLOBAL_CURRENT 0x6e +#define IS32FL3207_REG_RESET 0x7f + +#define IS32FL3207_CONTROL_ENABLE BIT(0)
drivers/leds/rgb/leds-is32fl3207.c' line 43
Quoted 10 lines
drivers/leds/rgb/leds-is32fl3207.c' new file +10
-0+#define IS32FL3207_GLOBAL_CURRENT_MAX 0xff + +/* IOUT(MAX) in microamperes = 76,500,000 / RISET in ohms. */ +#define IS32FL3207_CURRENT_NUMERATOR 76500000ULL + +struct is32fl3207; + +struct is32fl3207_led { + struct is32fl3207 *chip; + struct led_classdev *led_cdev;
Can we avoid storing the redundant 'led_cdev' pointer here? Since 'cdev' and 'mcdev.led_cdev' are unionised and share the same memory offset, we could generalise and use '&led->cdev' whenever a pointer to the base 'struct led_classdev' is required.
drivers/leds/rgb/leds-is32fl3207.c' line 55
Quoted 12 lines
drivers/leds/rgb/leds-is32fl3207.c' new file +12
-0+ union { + struct led_classdev cdev; + struct led_classdev_mc mcdev; + }; + unsigned int channel; +}; + +struct is32fl3207 { + struct device *dev; + struct regmap *regmap; + struct gpio_desc *enable_gpio; + struct mutex lock; /* Serializes controller register updates. */
Not sure the comment is required here - this is what they usually do context of device drivers.
Quoted 55 lines
drivers/leds/rgb/leds-is32fl3207.c' new file +55
-0+ unsigned long channels[BITS_TO_LONGS(IS32FL3207_NUM_CHANNELS)]; + u32 output_max_microamp; + unsigned int num_leds; + bool shutting_down; + struct is32fl3207_led leds[] __counted_by(num_leds); +}; + +static int is32fl3207_parse_led_properties(struct is32fl3207 *chip, + struct fwnode_handle *fwnode, + unsigned int *max_brightness, + unsigned int *brightness) +{ + enum led_default_state default_state; + u32 value; + int ret; + + *max_brightness = IS32FL3207_MAX_BRIGHTNESS; + if (fwnode_property_present(fwnode, "max-brightness")) { + ret = fwnode_property_read_u32(fwnode, "max-brightness", + &value); + if (ret) + return dev_err_probe(chip->dev, ret, + "failed to read maximum brightness for %pfw\n", + fwnode); + if (!value || value > IS32FL3207_MAX_BRIGHTNESS) + return dev_err_probe(chip->dev, -EINVAL, + "invalid maximum brightness %u for %pfw\n", + value, fwnode); + + *max_brightness = value; + } + + value = *max_brightness; + if (fwnode_property_present(fwnode, "default-brightness")) { + ret = fwnode_property_read_u32(fwnode, "default-brightness", + &value); + if (ret) + return dev_err_probe(chip->dev, ret, + "failed to read default brightness for %pfw\n", + fwnode); + if (value > *max_brightness) + return dev_err_probe(chip->dev, -EINVAL, + "invalid default brightness %u for %pfw\n", + value, fwnode); + } + + default_state = led_init_default_state_get(fwnode); + if (default_state == LEDS_DEFSTATE_KEEP) + return dev_err_probe(chip->dev, -EINVAL, + "default state keep is not supported for %pfw\n", + fwnode); + + *brightness = default_state == LEDS_DEFSTATE_ON ? value : LED_OFF; + return 0; +}
drivers/leds/rgb/leds-is32fl3207.c' line 120
Quoted 10 lines
drivers/leds/rgb/leds-is32fl3207.c' new file +10
-0+ +static int is32fl3207_validate_component(struct is32fl3207 *chip, + struct fwnode_handle *fwnode) +{ + static const char * const unsupported[] = { + "default-brightness", + "default-state", + "max-brightness", + "retain-state-shutdown", + };
Really not sure about this. Validating everything that is unsupported is a slippery slope. If anything, put it in the documentation and have done.
drivers/leds/rgb/leds-is32fl3207.c' line 123
Quoted 3 lines
drivers/leds/rgb/leds-is32fl3207.c' new file +3
-0+ unsigned int i; + + for (i = 0; i < ARRAY_SIZE(unsupported); i++)
for (int i = -; ...
Quoted 6 lines
drivers/leds/rgb/leds-is32fl3207.c' new file +6
-0+ if (fwnode_property_present(fwnode, unsupported[i])) + return dev_err_probe(chip->dev, -EINVAL, + "%s is not supported for component %pfw\n", + unsupported[i], fwnode); + + return 0;
drivers/leds/rgb/leds-is32fl3207.c' line 139
Quoted 10 lines
drivers/leds/rgb/leds-is32fl3207.c' new file +10
-0+} + +static int is32fl3207_write_channels_locked(struct is32fl3207 *chip, + const struct mc_subled *subleds, + unsigned int num_channels) +{ + unsigned int i; + int ret; + + for (i = 0; i < num_channels; i++) {
for (int i = -; ...
Quoted 5 lines
drivers/leds/rgb/leds-is32fl3207.c' new file +5
-0+ ret = regmap_write(chip->regmap, + IS32FL3207_REG_PWM_LOW(subleds[i].channel), + subleds[i].brightness); + if (ret) + return ret;
drivers/leds/rgb/leds-is32fl3207.c' line 154
Quoted 10 lines
drivers/leds/rgb/leds-is32fl3207.c' new file +10
-0+ } + + return regmap_write(chip->regmap, IS32FL3207_REG_PWM_UPDATE, 0); +} + +static int is32fl3207_brightness_set(struct led_classdev *cdev, + enum led_brightness brightness) +{ + struct is32fl3207_led *led = container_of(cdev, struct is32fl3207_led, + cdev);
Use 100-chars to prevent crazy line breaks like this.
drivers/leds/rgb/leds-is32fl3207.c' line 162
Quoted 8 lines
drivers/leds/rgb/leds-is32fl3207.c' new file +8
-0+ struct mc_subled subled = { + .brightness = brightness, + .channel = led->channel, + }; + + guard(mutex)(&led->chip->lock); + if (led->chip->shutting_down) + return 0;
Is it even architecturally possible to set brightness on a shutdown or disabled LED? That sounds like a bigger problem.
Quoted 27 lines
drivers/leds/rgb/leds-is32fl3207.c' new file +27
-0+ + return is32fl3207_write_channels_locked(led->chip, &subled, 1); +} + +static int is32fl3207_mc_brightness_set(struct led_classdev *cdev, + enum led_brightness brightness) +{ + struct led_classdev_mc *mcdev = lcdev_to_mccdev(cdev); + struct is32fl3207_led *led = container_of(mcdev, struct is32fl3207_led, + mcdev); + + guard(mutex)(&led->chip->lock); + if (led->chip->shutting_down) + return 0; + + led_mc_calc_color_components(mcdev, brightness); + + return is32fl3207_write_channels_locked(led->chip, mcdev->subled_info, + mcdev->num_colors); +} + +static int is32fl3207_turn_off_locked(struct is32fl3207 *chip, + struct is32fl3207_led *led) +{ + struct led_classdev *cdev = led->led_cdev; + struct mc_subled subled = { + .brightness = LED_OFF,
drivers/leds/rgb/leds-is32fl3207.c' line 199
Quoted 10 lines
drivers/leds/rgb/leds-is32fl3207.c' new file +10
-0+ .channel = led->channel, + }; + + if (cdev->flags & LED_MULTI_COLOR) { + struct led_classdev_mc *mcdev = lcdev_to_mccdev(cdev); + + led_mc_calc_color_components(mcdev, LED_OFF); + return is32fl3207_write_channels_locked(chip, + mcdev->subled_info, + mcdev->num_colors);
Is this a patch artefact? We line-up with the '('.
Quoted 9 lines
drivers/leds/rgb/leds-is32fl3207.c' new file +9
-0+ } + + return is32fl3207_write_channels_locked(chip, &subled, 1); +} + +static int is32fl3207_configure_channel(struct is32fl3207 *chip, + struct fwnode_handle *fwnode, + unsigned int *channel) +{
drivers/leds/rgb/leds-is32fl3207.c' line 218
Quoted 10 lines
drivers/leds/rgb/leds-is32fl3207.c' new file +10
-0+ u64 scaling; + u32 max_microamp; + u32 reg; + int ret; + + ret = fwnode_property_read_u32(fwnode, "reg", ®); + if (ret) + return dev_err_probe(chip->dev, ret, + "failed to read channel for %pfw\n", + fwnode);
No line-break here. Same with some of lines below.
Quoted 14 lines
drivers/leds/rgb/leds-is32fl3207.c' new file +14
-0+ + if (reg >= IS32FL3207_NUM_CHANNELS) + return dev_err_probe(chip->dev, -EINVAL, + "channel %u is out of range\n", reg); + + if (test_bit(reg, chip->channels)) + return dev_err_probe(chip->dev, -EINVAL, + "channel %u is used more than once\n", + reg); + + ret = fwnode_property_read_u32(fwnode, "led-max-microamp", + &max_microamp); + if (ret) + return dev_err_probe(chip->dev, ret,
drivers/leds/rgb/leds-is32fl3207.c' line 242
Quoted 10 lines
drivers/leds/rgb/leds-is32fl3207.c' new file +10
-0+ "failed to read current limit for channel %u\n", + reg); + + if (!max_microamp || max_microamp > chip->output_max_microamp) + return dev_err_probe(chip->dev, -EINVAL, + "invalid current limit %u uA for channel %u\n", + max_microamp, reg); + + /* GCC is fixed at 0xff, so use each output's scaling register. */ + scaling = div_u64((u64)max_microamp * 256 * 256,
These magic numbers need defining.
drivers/leds/rgb/leds-is32fl3207.c' line 250
Quoted 8 lines
drivers/leds/rgb/leds-is32fl3207.c' new file +8
-0+ (u64)chip->output_max_microamp * + IS32FL3207_GLOBAL_CURRENT_MAX); + if (!scaling) + return dev_err_probe(chip->dev, -EINVAL, + "current limit %u uA is below channel %u resolution\n", + max_microamp, reg); + + scaling = min_t(u64, scaling, 0xff);
As above - MAX something something I presume.
Quoted 25 lines
drivers/leds/rgb/leds-is32fl3207.c' new file +25
-0+ + guard(mutex)(&chip->lock); + ret = regmap_write(chip->regmap, IS32FL3207_REG_SCALING(reg), + (unsigned int)scaling); + if (ret) + return ret; + + set_bit(reg, chip->channels); + *channel = reg; + + return 0; +} + +static int is32fl3207_register_single(struct is32fl3207 *chip, + struct fwnode_handle *fwnode, + struct is32fl3207_led *led) +{ + struct led_init_data init_data = { + .devicename = dev_name(chip->dev), + .devname_mandatory = true, + .fwnode = fwnode, + }; + unsigned int max_brightness; + unsigned int brightness; + u32 color;
drivers/leds/rgb/leds-is32fl3207.c' line 285
Quoted 10 lines
drivers/leds/rgb/leds-is32fl3207.c' new file +10
-0+ int ret; + + if (!fwnode_property_present(fwnode, "function") && + !fwnode_property_present(fwnode, "color")) + return dev_err_probe(chip->dev, -EINVAL, + "single LED %pfw requires function or color\n", + fwnode); + + ret = is32fl3207_parse_led_properties(chip, fwnode, + &max_brightness, &brightness);
Why not pass 'led' or 'cdev' and have this populate the brightnesses instead?
Passing pointers to ints gives me the ick!
Quoted 190 lines
drivers/leds/rgb/leds-is32fl3207.c' new file +190
-0+ if (ret) + return ret; + + if (fwnode_property_present(fwnode, "color")) { + ret = fwnode_property_read_u32(fwnode, "color", &color); + if (ret) + return dev_err_probe(chip->dev, ret, + "failed to read color for %pfw\n", + fwnode); + if (color >= LED_COLOR_ID_MAX || color == LED_COLOR_ID_MULTI || + color == LED_COLOR_ID_RGB) + return dev_err_probe(chip->dev, -EINVAL, + "invalid single LED color %u\n", + color); + } + + ret = is32fl3207_configure_channel(chip, fwnode, &led->channel); + if (ret) + return ret; + led->chip = chip; + led->led_cdev = &led->cdev; + led->cdev.brightness = brightness; + led->cdev.max_brightness = max_brightness; + led->cdev.brightness_set_blocking = is32fl3207_brightness_set; + + ret = is32fl3207_brightness_set(&led->cdev, brightness); + if (ret) + return ret; + + return devm_led_classdev_register_ext(chip->dev, &led->cdev, + &init_data); +} + +static int is32fl3207_register_multicolor(struct is32fl3207 *chip, + struct fwnode_handle *fwnode, + struct is32fl3207_led *led) +{ + struct led_init_data init_data = { + .devicename = dev_name(chip->dev), + .devname_mandatory = true, + .fwnode = fwnode, + }; + struct mc_subled *subleds; + DECLARE_BITMAP(color_map, LED_COLOR_ID_MAX); + unsigned int max_brightness; + unsigned int brightness; + unsigned int count; + unsigned int i = 0; + bool has_group_reg; + u32 group_color; + u32 group_reg; + unsigned int first_channel = IS32FL3207_NUM_CHANNELS; + int ret; + + ret = is32fl3207_parse_led_properties(chip, fwnode, + &max_brightness, &brightness); + if (ret) + return ret; + + ret = fwnode_property_read_u32(fwnode, "color", &group_color); + if (ret) + return dev_err_probe(chip->dev, ret, + "failed to read color for %pfw\n", fwnode); + + if (group_color != LED_COLOR_ID_RGB && + group_color != LED_COLOR_ID_MULTI) + return dev_err_probe(chip->dev, -EINVAL, + "invalid multicolor LED color %u\n", + group_color); + + has_group_reg = fwnode_property_present(fwnode, "reg"); + if (has_group_reg) { + ret = fwnode_property_read_u32(fwnode, "reg", &group_reg); + if (ret) + return dev_err_probe(chip->dev, ret, + "failed to read group index for %pfw\n", + fwnode); + } + + count = fwnode_get_child_node_count(fwnode); + if (!count || count > LED_COLOR_ID_MAX) + return dev_err_probe(chip->dev, -EINVAL, + "invalid component count %u for %pfw\n", + count, fwnode); + + subleds = devm_kcalloc(chip->dev, count, sizeof(*subleds), GFP_KERNEL); + if (!subleds) + return -ENOMEM; + bitmap_zero(color_map, LED_COLOR_ID_MAX); + + fwnode_for_each_child_node_scoped(fwnode, child) { + u32 color; + + ret = is32fl3207_validate_component(chip, child); + if (ret) + return ret; + + ret = fwnode_property_read_u32(child, "color", &color); + if (ret) + return dev_err_probe(chip->dev, ret, + "failed to read color for %pfw\n", + child); + + if (color >= LED_COLOR_ID_MAX || color == LED_COLOR_ID_MULTI || + color == LED_COLOR_ID_RGB) + return dev_err_probe(chip->dev, -EINVAL, + "invalid component color %u\n", + color); + if (test_and_set_bit(color, color_map)) + return dev_err_probe(chip->dev, -EINVAL, + "component color %u is used more than once\n", + color); + + ret = is32fl3207_configure_channel(chip, child, + &subleds[i].channel); + if (ret) + return ret; + + subleds[i].color_index = color; + subleds[i].intensity = max_brightness; + subleds[i].max_intensity = 0; + first_channel = min(first_channel, subleds[i].channel); + i++; + } + + if (has_group_reg && group_reg != first_channel) + return dev_err_probe(chip->dev, -EINVAL, + "group index %u does not match first channel %u\n", + group_reg, first_channel); + + led->chip = chip; + led->led_cdev = &led->mcdev.led_cdev; + led->mcdev.num_colors = count; + led->mcdev.subled_info = subleds; + led->mcdev.led_cdev.brightness = brightness; + led->mcdev.led_cdev.max_brightness = max_brightness; + led->mcdev.led_cdev.brightness_set_blocking = + is32fl3207_mc_brightness_set; + + ret = is32fl3207_mc_brightness_set(&led->mcdev.led_cdev, + brightness); + if (ret) + return ret; + + return devm_led_classdev_multicolor_register_ext(chip->dev, &led->mcdev, + &init_data); +} + +static int is32fl3207_register_led(struct is32fl3207 *chip, + struct fwnode_handle *fwnode, + struct is32fl3207_led *led) +{ + unsigned int count = fwnode_get_child_node_count(fwnode); + bool has_color = fwnode_property_present(fwnode, "color"); + u32 color = LED_COLOR_ID_MAX; + int ret; + + if (has_color) { + ret = fwnode_property_read_u32(fwnode, "color", &color); + if (ret) + return dev_err_probe(chip->dev, ret, + "failed to read color for %pfw\n", + fwnode); + } + + if (color == LED_COLOR_ID_RGB || color == LED_COLOR_ID_MULTI) { + if (!count) + return dev_err_probe(chip->dev, -EINVAL, + "multicolor LED %pfw has no components\n", + fwnode); + + return is32fl3207_register_multicolor(chip, fwnode, led); + } + + if (count) + return dev_err_probe(chip->dev, -EINVAL, + "single LED %pfw must not have components\n", + fwnode); + + return is32fl3207_register_single(chip, fwnode, led); +} + +static int is32fl3207_clear_retained_scaling(struct is32fl3207 *chip) +{ + u8 scaling[IS32FL3207_NUM_CHANNELS] = { }; + int ret; + + ret = regmap_write(chip->regmap, IS32FL3207_REG_CONTROL, 0); + if (ret) + return ret;
drivers/leds/rgb/leds-is32fl3207.c' line 485
Quoted 10 lines
drivers/leds/rgb/leds-is32fl3207.c' new file +10
-0+ + return regmap_bulk_write(chip->regmap, + IS32FL3207_REG_SCALING(0), scaling, + sizeof(scaling)); +} + +static int is32fl3207_hw_init(struct is32fl3207 *chip) +{ + u8 scaling[IS32FL3207_NUM_CHANNELS] = { }; + u8 pwm[2 * IS32FL3207_NUM_CHANNELS] = { };
Why 2? Define?
Quoted 49 lines
drivers/leds/rgb/leds-is32fl3207.c' new file +49
-0+ int disable_ret; + int ret; + + /* Software reset requires normal operation (SSD = 1). */ + ret = regmap_write(chip->regmap, IS32FL3207_REG_CONTROL, + IS32FL3207_CONTROL_ENABLE); + if (ret) + return ret; + + ret = regmap_write(chip->regmap, IS32FL3207_REG_RESET, 0); + if (ret) + return ret; + usleep_range(200, 300); + + ret = regmap_write(chip->regmap, IS32FL3207_REG_CONTROL, 0); + if (ret) + return ret; + + ret = regmap_write(chip->regmap, IS32FL3207_REG_GLOBAL_CURRENT, + IS32FL3207_GLOBAL_CURRENT_MAX); + if (ret) + return ret; + + ret = regmap_bulk_write(chip->regmap, IS32FL3207_REG_SCALING(0), + scaling, sizeof(scaling)); + if (ret) + return ret; + + ret = regmap_bulk_write(chip->regmap, IS32FL3207_REG_PWM_LOW(0), pwm, + sizeof(pwm)); + if (ret) + return ret; + + /* PWM data can be latched only in normal operation. */ + ret = regmap_write(chip->regmap, IS32FL3207_REG_CONTROL, + IS32FL3207_CONTROL_ENABLE); + if (ret) + return ret; + + ret = regmap_write(chip->regmap, IS32FL3207_REG_PWM_UPDATE, 0); + disable_ret = regmap_write(chip->regmap, IS32FL3207_REG_CONTROL, 0); + + return ret ?: disable_ret; +} + +static int is32fl3207_enable(struct is32fl3207 *chip) +{ + int ret; +
drivers/leds/rgb/leds-is32fl3207.c' line 544
Quoted 10 lines
drivers/leds/rgb/leds-is32fl3207.c' new file +10
-0+ guard(mutex)(&chip->lock); + + ret = regmap_write(chip->regmap, IS32FL3207_REG_CONTROL, + IS32FL3207_CONTROL_ENABLE); + if (ret) + return ret; + + ret = regmap_write(chip->regmap, IS32FL3207_REG_PWM_UPDATE, 0); + if (ret) + regmap_write(chip->regmap, IS32FL3207_REG_CONTROL, 0);
Needs a comment. What does this fall-back do?
Quoted 20 lines
drivers/leds/rgb/leds-is32fl3207.c' new file +20
-0+ + return ret; +} + +static void is32fl3207_disable_locked(struct is32fl3207 *chip) +{ + regmap_write(chip->regmap, IS32FL3207_REG_CONTROL, 0); + if (chip->enable_gpio) + gpiod_set_value_cansleep(chip->enable_gpio, 0); +} + +static void is32fl3207_disable(void *data) +{ + struct is32fl3207 *chip = data; + + guard(mutex)(&chip->lock); + chip->shutting_down = true; + is32fl3207_disable_locked(chip); +} +
drivers/leds/rgb/leds-is32fl3207.c' line 574
Quoted 10 lines
drivers/leds/rgb/leds-is32fl3207.c' new file +10
-0+static const struct regmap_config is32fl3207_regmap_config = { + .reg_bits = 8, + .val_bits = 8, + .max_register = IS32FL3207_REG_RESET, +}; + +static int is32fl3207_probe(struct i2c_client *client) +{ + struct device *dev = &client->dev; + struct is32fl3207 *ddata;
This is confusing - ddata or chip, pick one and use one throughout.
Suggest you use chip here in LEDs, ddata elsewhere.
Quoted 100 lines
drivers/leds/rgb/leds-is32fl3207.c' new file +100
-0+ unsigned int count; + unsigned int i = 0; + u32 riset_ohms; + int ret; + + count = device_get_child_node_count(dev); + if (!count || count > IS32FL3207_NUM_CHANNELS) + return dev_err_probe(dev, -EINVAL, + "invalid LED count %u\n", count); + + ddata = devm_kzalloc(dev, struct_size(ddata, leds, count), GFP_KERNEL); + if (!ddata) + return -ENOMEM; + + ddata->dev = dev; + ddata->num_leds = count; + i2c_set_clientdata(client, ddata); + + ret = device_property_read_u32(dev, "issi,riset-ohms", &riset_ohms); + if (ret) + return dev_err_probe(dev, ret, "failed to read RISET value\n"); + + if (riset_ohms < 2000) + return dev_err_probe(dev, -EINVAL, + "RISET value %u is below 2000 ohms\n", + riset_ohms); + + ddata->output_max_microamp = div_u64(IS32FL3207_CURRENT_NUMERATOR, + riset_ohms); + if (!ddata->output_max_microamp) + return dev_err_probe(dev, -EINVAL, + "RISET value %u is too large\n", + riset_ohms); + + ddata->enable_gpio = devm_gpiod_get_optional(dev, "enable", + GPIOD_OUT_LOW); + if (IS_ERR(ddata->enable_gpio)) + return dev_err_probe(dev, PTR_ERR(ddata->enable_gpio), + "failed to get enable GPIO\n"); + + ddata->regmap = devm_regmap_init_i2c(client, + &is32fl3207_regmap_config); + if (IS_ERR(ddata->regmap)) + return dev_err_probe(dev, PTR_ERR(ddata->regmap), + "failed to allocate register map\n"); + + ret = devm_mutex_init(dev, &ddata->lock); + if (ret) + return ret; + + ret = devm_regulator_get_enable_optional(dev, "vcc"); + if (ret && ret != -ENODEV) + return dev_err_probe(dev, ret, + "failed to enable VCC regulator\n"); + + ret = devm_add_action_or_reset(dev, is32fl3207_disable, ddata); + if (ret) + return ret; + + /* Let VCC settle while SDB keeps the outputs disabled. */ + usleep_range(1000, 2000); + + /* + * Registers remain accessible with SDB low. Clear retained scaling + * before releasing hardware shutdown. + */ + ret = is32fl3207_clear_retained_scaling(ddata); + if (ret) + return dev_err_probe(dev, ret, + "failed to clear retained current scaling\n"); + + if (ddata->enable_gpio) + gpiod_set_value_cansleep(ddata->enable_gpio, 1); + + /* The SDB rising edge resets the I2C interface; allow it to settle. */ + usleep_range(1000, 2000); + + ret = is32fl3207_hw_init(ddata); + if (ret) + return dev_err_probe(dev, ret, + "failed to initialize controller\n"); + + device_for_each_child_node_scoped(dev, child) { + struct is32fl3207_led *led = &ddata->leds[i]; + + ret = is32fl3207_register_led(ddata, child, led); + if (ret) + return ret; + + i++; + } + + ret = is32fl3207_enable(ddata); + if (ret) + return dev_err_probe(dev, ret, "failed to enable controller\n"); + + return 0; +} + +static void is32fl3207_shutdown(struct i2c_client *client)
drivers/leds/rgb/leds-is32fl3207.c' line 684
Quoted 10 lines
drivers/leds/rgb/leds-is32fl3207.c' new file +10
-0+{ + struct is32fl3207 *chip = i2c_get_clientdata(client); + bool retain_state = false; + unsigned int i; + int ret = 0; + + guard(mutex)(&chip->lock); + chip->shutting_down = true; + + for (i = 0; i < chip->num_leds; i++)
Needs braces.
Quoted 5 lines
drivers/leds/rgb/leds-is32fl3207.c' new file +5
-0+ if (chip->leds[i].led_cdev->flags & LED_RETAIN_AT_SHUTDOWN) { + retain_state = true; + break; + } +
drivers/leds/rgb/leds-is32fl3207.c' line 699
Quoted 10 lines
drivers/leds/rgb/leds-is32fl3207.c' new file +10
-0+ if (!retain_state) { + is32fl3207_disable_locked(chip); + return; + } + + for (i = 0; i < chip->num_leds; i++) { + struct led_classdev *cdev = chip->leds[i].led_cdev; + + if (cdev->flags & LED_RETAIN_AT_SHUTDOWN) + continue;
Nit: '\n'
Quoted 41 lines
drivers/leds/rgb/leds-is32fl3207.c' new file +37
-0+ ret = is32fl3207_turn_off_locked(chip, &chip->leds[i]); + + if (ret) { + dev_warn(chip->dev, + "failed to turn off LEDs during shutdown: %d\n", + ret); + break; + } + } +} + +static const struct of_device_id is32fl3207_of_match[] = { + { .compatible = "issi,is32fl3207" }, + { } +}; +MODULE_DEVICE_TABLE(of, is32fl3207_of_match); + +static const struct i2c_device_id is32fl3207_id[] = { + { .name = "is32fl3207" }, + { } +}; +MODULE_DEVICE_TABLE(i2c, is32fl3207_id); + +static struct i2c_driver is32fl3207_driver = { + .driver = { + .name = "is32fl3207", + .of_match_table = is32fl3207_of_match, + }, + .probe = is32fl3207_probe, + .shutdown = is32fl3207_shutdown, + .id_table = is32fl3207_id, +}; +module_i2c_driver(is32fl3207_driver); + +MODULE_AUTHOR("Ahmad Byagowi <[email protected]>"); +MODULE_DESCRIPTION("Lumissil IS32FL3207 LED controller driver"); +MODULE_LICENSE("GPL");Signature · 1 line
2.50.1 (Apple Git-155)
Signature · 1 line
Lee Jones
Re: [PATCH v7 2/2] leds: is32fl3207: Add controller driver
Lee,
Thanks for the detailed review. I will address the cleanup points in v8:
remove the redundant led_cdev pointer, populate brightness directly in the class device, use chip consistently, use loop-local indices, and fix the wrapping, alignment, braces, and spacing. I will also name the current-scaling factors and the two PWM bytes per channel, and explain the output-disable fallback when the initial PWM latch fails.
I will drop the unsupported-property list for multicolor components. The binding already disallows those properties on component nodes; I will clarify that brightness, default state, and shutdown retention belong to the group.
Quoted 2 lines
Is it even architecturally possible to set brightness on a shutdown or disabled LED? That sounds like a bigger problem.
The guard is intended for system shutdown, rather than normal LED unregistration. In the current LED tree, led_classdev_unregister() stops software blinking and flushes set_brightness_work. The system shutdown path does not call that function, and i2c_device_shutdown() calls the driver's shutdown callback without draining LED brightness work.
A previously queued brightness callback can therefore run after our shutdown callback. Both paths take the same controller mutex; setting shutting_down under that lock prevents a later callback from writing registers or changing the final retained state. This also matters when some LEDs retain their state and the controller must remain enabled.
I will retain the guard and add a comment explaining that distinction. This is based on inspection of the shutdown and LED-core paths, not a hardware reproduction of the race.
Regards,
Ahmad