[PATCH v5 0/2] Add Support for LTC3219 18 Channel LED Driver

[PATCH v5 0/2] Add Support for LTC3219 18 Channel LED Driver

[Edelweise Escala]

The LTC3220/LTC3220-1 is a multi-display LED driver, which contains a
high-efficiency, low-noise charge pump to provide power to up to
18 LED current sources. The LEDs are individually configurable to
64-step linear brightness control, blinking and gradation control
via 2-wire I2C interface. The blinking and gradation configuration
is shared across all LED.

LTC3220 has a quick write function which allows changing the brightness
on all LEDS simultaneously when the brightness is changed on led 1.
For this leds are aggregated in the device tree and on probe we check
if led-sources exist to enable quick write. 
We would like to know if this approach is alright?
Another way we might want to know is, is it alright to just make a
virtual led for the quick write function. Changing brightness on 
the virtual led will change the brightness for all.

Signed-off-by: Edelweise Escala <edelweise.escala@analog.com>
---
Changes in v5:
- Missed rename on bindings filename in MAINTAINERS file
- Link to v4: https://lore.kernel.org/linux-leds/20260126-ltc3220-driver-v4-0-c59517206c24@analog.com

Changes in v4:
- Rename leds-ltc3220.yaml to adi,ltc3220.yaml
- Add Reviewed-by: Conor Dooley <conor.dooley@microchip.com> on
  adi,ltc3220.yaml
Other V1 comments I think already addressed
- Subject commit message was already changed to match hardware
- Fixed wrapping after description
- Dropped "Bindings for" in descriptions and improved description to match hardware
- Dropped adi,ltc3220-1
- Dropped redundant description on reset-gpios
- Dropped adi,force-cpo-level
- Dropped adi,quick-write in favor of aggregated LED
- Used consistent quotes ^led@([1-9]|1[0-8])$
- Fixed wrapping on error messages

- Link to v3: https://lore.kernel.org/r/20260120-ltc3220-driver-v3-0-fef612ec4faa@analog.com

Changes in v3:
- Dropped quick-write on bindings and added aggregated led instead.
- Add aggregated led example.
- Modify quick write to check if there is aggregated led, if there is
  aggregated led enable quick write.
- Use DEFINE_SIMPLE_DEV_PM_OPS instead of SIMPLE_DEV_PM_OPS.
- Link to v2: https://lore.kernel.org/r/20260112-ltc3220-driver-v2-0-d043058fc4df@analog.com

Changes in v2:
leds-ltc3220.yaml changes
- Fix wrapping on description
- Improve description and commit messge to describe hardware
- Drop ltc3220-1
- Drop charge pump
ltc3220.c changes
- Fix wrapping
- Drop ltc3220-1
- Drop devname_mandatory
- Link to v1: https://lore.kernel.org/r/20260106-ltc3220-driver-v1-0-73601d6f1649@analog.com

---
Edelweise Escala (2):
      dt-bindings: leds: Add LTC3220 18 channel LED Driver
      leds: ltc3220: Add Support for LTC3220 18 channel LED Driver

 .../devicetree/bindings/leds/adi,ltc3220.yaml      | 120 ++++++
 MAINTAINERS                                        |   8 +
 drivers/leds/Kconfig                               |  10 +
 drivers/leds/Makefile                              |   1 +
 drivers/leds/leds-ltc3220.c                        | 455 +++++++++++++++++++++
 5 files changed, 594 insertions(+)
---
base-commit: 8856d7fe1758937ac528770f552ec58c388c255b
change-id: 20260106-ltc3220-driver-f9ab6cc9d1e4

Best regards,
-- 
Edelweise Escala <edelweise.escala@analog.com>

[PATCH v5 1/2] dt-bindings: leds: Add LTC3220 18 channel LED Driver

[Edelweise Escala]

LTC3220 is a multi-display LED driver with I2C interface.
The LTC3220 provides individual brightness control (64-step),
blinking, and gradation features for up to 18 LED outputs.

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Edelweise Escala <edelweise.escala@analog.com>
---
 .../devicetree/bindings/leds/adi,ltc3220.yaml      | 120 +++++++++++++++++++++
 MAINTAINERS                                        |   7 ++
 2 files changed, 127 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/adi,ltc3220.yaml b/Documentation/devicetree/bindings/leds/adi,ltc3220.yaml
new file mode 100644
index 000000000000..62f760d517aa
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/adi,ltc3220.yaml
@@ -0,0 +1,120 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/adi,ltc3220.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices LTC3220 LED Driver
+
+maintainers:
+  - Edelweise Escala <edelweise.escala@analog.com>
+
+description:
+  The LTC3220 is a multi-display LED driver, which contains a high-efficiency,
+  low-noise charge pump to provide power to up to 18 LED current sources.
+  The LEDs are individually configurable to 64-step linear brightness control,
+  blinking and gradation control via 2-wire I2C interface.
+
+  For more product information please see the link below
+    https://www.analog.com/en/products/ltc3220.html
+
+properties:
+  compatible:
+    const: adi,ltc3220
+
+  reg:
+    maxItems: 1
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+  reset-gpios:
+    maxItems: 1
+
+patternProperties:
+  '^led@([1-9]|1[0-8])$':
+    type: object
+    $ref: /schemas/leds/common.yaml#
+    unevaluatedProperties: false
+    properties:
+      reg:
+        description:
+          Output channel for the LED (1-18 maps to LED outputs D1-D18).
+          For aggregated LED control, define only one LED node with reg = <1>
+          and use led-sources to list all controlled outputs. Only reg 1 should
+          be present when using led-sources.
+        minimum: 1
+        maximum: 18
+
+    required:
+      - reg
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    // Independent LEDs
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/leds/common.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        led-controller@1c {
+            compatible = "adi,ltc3220";
+            reg = <0x1c>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+            reset-gpios = <&gpio 17 GPIO_ACTIVE_LOW>;
+
+            led@1 {
+                reg = <1>;
+                function = LED_FUNCTION_INDICATOR;
+                function-enumerator = <1>;
+            };
+
+            led@2 {
+                reg = <2>;
+                function = LED_FUNCTION_INDICATOR;
+                function-enumerator = <2>;
+            };
+
+            led@3 {
+                reg = <3>;
+                function = LED_FUNCTION_INDICATOR;
+                function-enumerator = <3>;
+            };
+        };
+    };
+
+  - |
+    // Aggregated LED
+    #include <dt-bindings/leds/common.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        led-controller@1c {
+            compatible = "adi,ltc3220";
+            reg = <0x1c>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            led@1 {
+                reg = <1>;
+                led-sources = <1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18>;
+                function = LED_FUNCTION_BACKLIGHT;
+            };
+        };
+    };
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 327d74ca7ecb..5c10cc3e3022 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14955,6 +14955,13 @@ W:	https://ez.analog.com/linux-software-drivers
 F:	Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
 F:	drivers/iio/temperature/ltc2983.c
 
+LTC3220 LED DRIVER
+M:	Edelweise Escala <edelweise.escala@analog.com>
+L:	linux-leds@vger.kernel.org
+S:	Maintained
+W:	https://ez.analog.com/linux-software-drivers
+F:	Documentation/devicetree/bindings/leds/adi,ltc3220.yaml
+
 LTC4282 HARDWARE MONITOR DRIVER
 M:	Nuno Sa <nuno.sa@analog.com>
 L:	linux-hwmon@vger.kernel.org

-- 
2.43.0

[PATCH v5 2/2] leds: ltc3220: Add Support for LTC3220 18 channel LED Driver

[Edelweise Escala]

Add driver for the LTC3220 18-channel LED driver
with I2C interface, individual brightness control, and hardware-assisted
blink/gradation features.

Signed-off-by: Edelweise Escala <edelweise.escala@analog.com>
---
 MAINTAINERS                 |   1 +
 drivers/leds/Kconfig        |  10 +
 drivers/leds/Makefile       |   1 +
 drivers/leds/leds-ltc3220.c | 455 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 467 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 5c10cc3e3022..7467537938bf 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14961,6 +14961,7 @@ L:	linux-leds@vger.kernel.org
 S:	Maintained
 W:	https://ez.analog.com/linux-software-drivers
 F:	Documentation/devicetree/bindings/leds/adi,ltc3220.yaml
+F:	drivers/leds/leds-ltc3220.c
 
 LTC4282 HARDWARE MONITOR DRIVER
 M:	Nuno Sa <nuno.sa@analog.com>
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 597d7a79c988..a1c34b2deded 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -1001,6 +1001,16 @@ config LEDS_ST1202
 	  Say Y to enable support for LEDs connected to LED1202
 	  LED driver chips accessed via the I2C bus.
 
+config LEDS_LTC3220
+	tristate "LED Driver for LTC3220/LTC3220-1"
+	depends on I2C && LEDS_CLASS
+	help
+	  If you have an 18-Channel LED Driver connected to LTC3220, or LTC3220-1
+	  say Y here to enable this driver.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called ltc3220.
+
 config LEDS_TPS6105X
 	tristate "LED support for TI TPS6105X"
 	depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 8fdb45d5b439..5301568d9e00 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -61,6 +61,7 @@ obj-$(CONFIG_LEDS_LP8788)		+= leds-lp8788.o
 obj-$(CONFIG_LEDS_LP8860)		+= leds-lp8860.o
 obj-$(CONFIG_LEDS_LP8864)		+= leds-lp8864.o
 obj-$(CONFIG_LEDS_LT3593)		+= leds-lt3593.o
+obj-$(CONFIG_LEDS_LTC3220)		+= leds-ltc3220.o
 obj-$(CONFIG_LEDS_MAX5970)		+= leds-max5970.o
 obj-$(CONFIG_LEDS_MAX77650)		+= leds-max77650.o
 obj-$(CONFIG_LEDS_MAX77705)		+= leds-max77705.o
diff --git a/drivers/leds/leds-ltc3220.c b/drivers/leds/leds-ltc3220.c
new file mode 100644
index 000000000000..6a5d967ae7e8
--- /dev/null
+++ b/drivers/leds/leds-ltc3220.c
@@ -0,0 +1,455 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * LTC3220 18-Channel LED Driver
+ *
+ * Copyright 2026 Analog Devices Inc.
+ *
+ * Author: Edelweise Escala <edelweise.escala@analog.com>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/leds.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/types.h>
+
+/* LTC3220 Registers */
+#define LTC3220_COMMAND				0x00
+#define LTC3220_ULED(x)				(0x01 + (x))
+#define LTC3220_GRAD_BLINK			0x13
+
+#define LTC3220_GRAD_COUNT_UP		BIT(0)
+#define LTC3220_COMMAND_QUICK_WRITE	BIT(0)
+#define LTC3220_COMMAND_SHUTDOWN	BIT(3)
+
+#define LTC3220_LED_CURRENT_MASK	GENMASK(5, 0)
+#define LTC3220_LED_MODE_MASK		GENMASK(7, 6)
+#define LTC3220_BLINK_MASK			GENMASK(4, 3)
+#define LTC3220_GRADATION_MASK		GENMASK(2, 1)
+
+#define LTC3220_NUM_LEDS 18
+
+struct ltc3220_command_cfg {
+	bool is_aggregated;
+	bool is_shutdown;
+};
+
+struct ltc3220_uled_cfg {
+	struct ltc3220_state *ltc3220_state;
+	struct led_classdev led_cdev;
+	u8 reg_value;
+	u8 led_index;
+};
+
+struct ltc3220_grad_cfg {
+	bool is_increasing;
+	u8 gradation_period_ms;
+};
+
+struct ltc3220_state {
+	struct ltc3220_command_cfg command_cfg;
+	struct ltc3220_uled_cfg uled_cfg[LTC3220_NUM_LEDS];
+	struct ltc3220_grad_cfg grad_cfg;
+	struct i2c_client *client;
+	u8 blink_mode;
+};
+
+static int ltc3220_set_command(struct ltc3220_state *ltc3220_state)
+{
+	struct i2c_client *client = ltc3220_state->client;
+	u8 reg_val;
+
+	reg_val = FIELD_PREP(LTC3220_COMMAND_SHUTDOWN,
+			     ltc3220_state->command_cfg.is_shutdown);
+	reg_val |= FIELD_PREP(LTC3220_COMMAND_QUICK_WRITE,
+			      ltc3220_state->command_cfg.is_aggregated);
+
+	return i2c_smbus_write_byte_data(client, LTC3220_COMMAND, reg_val);
+}
+
+static int ltc3220_shutdown(struct ltc3220_state *ltc3220_state)
+{
+	int ret;
+
+	ltc3220_state->command_cfg.is_shutdown = true;
+	ret = ltc3220_set_command(ltc3220_state);
+	if (ret < 0)
+		ltc3220_state->command_cfg.is_shutdown = false;
+
+	return ret;
+}
+
+static int ltc3220_resume_from_shutdown(struct ltc3220_state *ltc3220_state)
+{
+	int ret;
+
+	ltc3220_state->command_cfg.is_shutdown = false;
+	ret = ltc3220_set_command(ltc3220_state);
+	if (ret < 0)
+		ltc3220_state->command_cfg.is_shutdown = true;
+
+	return ret;
+}
+
+/*
+ * Set LED brightness and mode.
+ * The brightness value determines both the LED current and operating mode:
+ * 0-63:    Normal mode - LED current from 0-63 (off to full brightness)
+ * 64-127:  Blink mode - LED blinks with current level (brightness - 64)
+ * 128-191: Gradation mode - LED gradually changes brightness (brightness - 128)
+ * 192-255: GPO mode - LED operates as general purpose output (brightness - 192)
+ */
+static int ltc3220_set_led_data(struct led_classdev *led_cdev,
+				enum led_brightness brightness)
+{
+	struct ltc3220_state *ltc3220_state;
+	struct ltc3220_uled_cfg *uled_cfg;
+	int ret;
+	int i;
+
+	uled_cfg = container_of(led_cdev, struct ltc3220_uled_cfg, led_cdev);
+	ltc3220_state = uled_cfg->ltc3220_state;
+
+	ret = i2c_smbus_write_byte_data(ltc3220_state->client,
+			LTC3220_ULED(uled_cfg->led_index), brightness);
+	if (ret < 0)
+		return ret;
+
+	uled_cfg->reg_value = brightness;
+
+	/*
+	 * When aggregated LED mode is enabled, writing to LED 1 updates all
+	 * LEDs simultaneously via quick-write mode. Update cached values for
+	 * all LEDs to reflect the synchronized state.
+	 */
+	if (ltc3220_state->command_cfg.is_aggregated && uled_cfg->led_index == 0) {
+		for (i = 0; i < LTC3220_NUM_LEDS; i++)
+			ltc3220_state->uled_cfg[i].reg_value = brightness;
+	}
+
+	return 0;
+}
+
+static enum led_brightness ltc3220_get_led_data(struct led_classdev *led_cdev)
+{
+	struct ltc3220_uled_cfg *uled_cfg;
+
+	uled_cfg = container_of(led_cdev, struct ltc3220_uled_cfg, led_cdev);
+
+	return uled_cfg->reg_value;
+}
+
+static int ltc3220_set_blink_and_gradation(struct ltc3220_state *ltc3220_state,
+		    u8 blink_cfg, u8 gradation_period_ms, bool is_increasing)
+{
+	struct i2c_client *client = ltc3220_state->client;
+	u8 reg_val;
+
+	reg_val = FIELD_PREP(LTC3220_BLINK_MASK, blink_cfg);
+	reg_val |= FIELD_PREP(LTC3220_GRADATION_MASK, gradation_period_ms);
+	reg_val |= FIELD_PREP(LTC3220_GRAD_COUNT_UP, is_increasing);
+
+	return i2c_smbus_write_byte_data(client, LTC3220_GRAD_BLINK, reg_val);
+}
+
+/*
+ * LTC3220 pattern support for hardware-assisted breathing/gradation.
+ * The hardware supports 3 gradation ramp time 240ms, 480ms, 960ms)
+ * and can ramp up or down.
+ *
+ * Pattern array interpretation:
+ *   pattern[0].brightness = start brightness (0-63)
+ *   pattern[0].delta_t = ramp time in milliseconds
+ *   pattern[1].brightness = end brightness (0-63)
+ *   pattern[1].delta_t = (optional, can be 0 or same as pattern[0].delta_t)
+ */
+static int ltc3220_pattern_set(struct led_classdev *led_cdev,
+			       struct led_pattern *pattern,
+			       u32 len, int repeat)
+{
+	struct ltc3220_state *ltc3220_state;
+	struct ltc3220_uled_cfg *uled_cfg;
+	u8 gradation_period;
+	u8 start_brightness;
+	u8 end_brightness;
+	bool is_increasing;
+	int ret;
+
+	if (len != 2)
+		return -EINVAL;
+
+	uled_cfg = container_of(led_cdev, struct ltc3220_uled_cfg, led_cdev);
+	ltc3220_state = uled_cfg->ltc3220_state;
+
+	start_brightness = pattern[0].brightness & LTC3220_LED_CURRENT_MASK;
+	end_brightness = pattern[1].brightness & LTC3220_LED_CURRENT_MASK;
+
+	is_increasing = end_brightness > start_brightness;
+
+	if (pattern[0].delta_t == 0)
+		gradation_period = 0;
+	else if (pattern[0].delta_t <= 240)
+		gradation_period = 1;
+	else if (pattern[0].delta_t <= 480)
+		gradation_period = 2;
+	else
+		gradation_period = 3;
+
+	ret = ltc3220_set_blink_and_gradation(ltc3220_state,
+					       ltc3220_state->blink_mode,
+					       gradation_period,
+					       is_increasing);
+	if (ret < 0)
+		return ret;
+
+	ltc3220_state->grad_cfg.gradation_period_ms = gradation_period;
+	ltc3220_state->grad_cfg.is_increasing = is_increasing;
+
+	ret = ltc3220_set_led_data(led_cdev, start_brightness);
+	if (ret < 0)
+		return ret;
+
+	return ltc3220_set_led_data(led_cdev, 128 + end_brightness);
+}
+
+static int ltc3220_pattern_clear(struct led_classdev *led_cdev)
+{
+	struct ltc3220_state *ltc3220_state;
+	struct ltc3220_uled_cfg *uled_cfg;
+	int ret;
+
+	uled_cfg = container_of(led_cdev, struct ltc3220_uled_cfg, led_cdev);
+	ltc3220_state = uled_cfg->ltc3220_state;
+
+	ret = ltc3220_set_blink_and_gradation(ltc3220_state,
+					       ltc3220_state->blink_mode,
+					       0, false);
+	if (ret < 0)
+		return ret;
+
+	ltc3220_state->grad_cfg.gradation_period_ms = 0;
+	ltc3220_state->grad_cfg.is_increasing = false;
+
+	return 0;
+}
+
+/*
+ * LTC3220 has a global blink configuration that affects all LEDs.
+ * This implementation allows per-LED blink requests, but the blink timing
+ * will be shared across all LEDs. The delay values are mapped to the
+ * hardware's discrete blink rates.
+ */
+static int ltc3220_blink_set(struct led_classdev *led_cdev,
+			     unsigned long *delay_on,
+			     unsigned long *delay_off)
+{
+	struct ltc3220_state *ltc3220_state;
+	struct ltc3220_uled_cfg *uled_cfg;
+	unsigned long period;
+	u8 blink_mode;
+	int ret;
+
+	uled_cfg = container_of(led_cdev, struct ltc3220_uled_cfg, led_cdev);
+	ltc3220_state = uled_cfg->ltc3220_state;
+
+	if (*delay_on == 0 && *delay_off == 0) {
+		blink_mode = 1;
+		*delay_on = 500;
+		*delay_off = 500;
+	} else {
+		period = *delay_on + *delay_off;
+
+		if (period <= 750) {
+			blink_mode = 0;
+			*delay_on = 250;
+			*delay_off = 250;
+		} else if (period <= 1500) {
+			blink_mode = 1;
+			*delay_on = 500;
+			*delay_off = 500;
+		} else if (period <= 3000) {
+			blink_mode = 2;
+			*delay_on = 1000;
+			*delay_off = 1000;
+		} else {
+			blink_mode = 3;
+			*delay_on = 2000;
+			*delay_off = 2000;
+		}
+	}
+
+	ret = ltc3220_set_blink_and_gradation(ltc3220_state, blink_mode,
+					       ltc3220_state->grad_cfg.gradation_period_ms,
+					       ltc3220_state->grad_cfg.is_increasing);
+	if (ret < 0)
+		return ret;
+
+	ltc3220_state->blink_mode = blink_mode;
+
+	return 0;
+}
+
+static void ltc3220_reset_gpio_action(void *data)
+{
+	struct gpio_desc *reset_gpio = data;
+
+	gpiod_set_value_cansleep(reset_gpio, 1);
+}
+
+static int ltc3220_reset(struct ltc3220_state *ltc3220_state, struct i2c_client *client)
+{
+	struct gpio_desc *reset_gpio;
+	int ret;
+	int i;
+
+	reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(reset_gpio))
+		return dev_err_probe(&client->dev, PTR_ERR(reset_gpio), "Failed on reset GPIO\n");
+
+	if (reset_gpio) {
+		gpiod_set_value_cansleep(reset_gpio, 0);
+
+		ret = devm_add_action_or_reset(&client->dev, ltc3220_reset_gpio_action, reset_gpio);
+		if (ret)
+			return ret;
+
+	} else {
+		ret = ltc3220_set_command(ltc3220_state);
+		if (ret < 0)
+			return ret;
+
+		for (i = 0; i < LTC3220_NUM_LEDS; i++) {
+			ret = i2c_smbus_write_byte_data(client, LTC3220_ULED(i), 0);
+			if (ret < 0)
+				return ret;
+		}
+
+		ret = ltc3220_set_blink_and_gradation(ltc3220_state, 0, 0, 0);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int ltc3220_suspend(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct ltc3220_state *ltc3220_state = i2c_get_clientdata(client);
+
+	return ltc3220_shutdown(ltc3220_state);
+}
+
+static int ltc3220_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct ltc3220_state *ltc3220_state = i2c_get_clientdata(client);
+
+	return ltc3220_resume_from_shutdown(ltc3220_state);
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(ltc3220_pm_ops, ltc3220_suspend, ltc3220_resume);
+
+static int ltc3220_probe(struct i2c_client *client)
+{
+	struct ltc3220_state *ltc3220_state;
+	bool aggregated_led_found = false;
+	int num_leds = 0;
+	u8 i = 0;
+	int ret;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
+		return dev_err_probe(&client->dev, -EIO, "SMBUS Byte Data not Supported\n");
+
+	ltc3220_state = devm_kzalloc(&client->dev, sizeof(*ltc3220_state), GFP_KERNEL);
+	if (!ltc3220_state)
+		return -ENOMEM;
+
+	ltc3220_state->client = client;
+	i2c_set_clientdata(client, ltc3220_state);
+
+	ret = ltc3220_reset(ltc3220_state, client);
+	if (ret)
+		return dev_err_probe(&client->dev, ret, "Failed to reset device\n");
+
+	device_for_each_child_node_scoped(&client->dev, child) {
+		struct led_init_data init_data = {};
+		struct ltc3220_uled_cfg *led;
+		u32 source;
+
+		ret = fwnode_property_read_u32(child, "reg", &source);
+		if (ret)
+			return dev_err_probe(&client->dev, ret, "Couldn't read LED address\n");
+
+		if (!source || source > LTC3220_NUM_LEDS)
+			return dev_err_probe(&client->dev, -EINVAL, "LED address out of range\n");
+
+		init_data.fwnode = child;
+		init_data.devicename = "ltc3220";
+
+		if (fwnode_property_present(child, "led-sources")) {
+			if (source != 1)
+				return dev_err_probe(&client->dev, -EINVAL,
+							"Aggregated LED out of range\n");
+
+			if (aggregated_led_found)
+				return dev_err_probe(&client->dev, -EINVAL,
+							"One Aggregated LED only\n");
+
+			aggregated_led_found = true;
+			ltc3220_state->command_cfg.is_aggregated = true;
+			ret = ltc3220_set_command(ltc3220_state);
+			if (ret < 0)
+				return dev_err_probe(&client->dev, ret, "Failed to set command\n");
+		}
+
+		num_leds++;
+
+		/* LED node reg/index/address goes from 1 to 18 */
+		i = source - 1;
+		led = &ltc3220_state->uled_cfg[i];
+		led->led_index = i;
+		led->reg_value = 0;
+		led->ltc3220_state = ltc3220_state;
+		led->led_cdev.brightness_set_blocking = ltc3220_set_led_data;
+		led->led_cdev.brightness_get = ltc3220_get_led_data;
+		led->led_cdev.max_brightness = 255;
+		led->led_cdev.blink_set = ltc3220_blink_set;
+		led->led_cdev.pattern_set = ltc3220_pattern_set;
+		led->led_cdev.pattern_clear = ltc3220_pattern_clear;
+
+		ret = devm_led_classdev_register_ext(&client->dev, &led->led_cdev, &init_data);
+		if (ret)
+			return dev_err_probe(&client->dev, ret, "Failed to register LED class\n");
+	}
+
+	if (aggregated_led_found && num_leds > 1)
+		return dev_err_probe(&client->dev, -EINVAL,
+					"Aggregated LED must be the only LED node\n");
+
+	return 0;
+}
+
+static const struct of_device_id ltc3220_of_match[] = {
+	{ .compatible = "adi,ltc3220" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ltc3220_of_match);
+
+static struct i2c_driver ltc3220_led_driver = {
+	.driver = {
+		.name = "ltc3220",
+		.of_match_table = ltc3220_of_match,
+		.pm	= pm_sleep_ptr(&ltc3220_pm_ops),
+	},
+	.probe = ltc3220_probe,
+};
+module_i2c_driver(ltc3220_led_driver);
+
+MODULE_AUTHOR("Edelweise Escala <edelweise.escala@analog.com>");
+MODULE_DESCRIPTION("LED driver for LTC3220 controllers");
+MODULE_LICENSE("GPL");

-- 
2.43.0

RE: [PATCH v5 0/2] Add Support for LTC3219 18 Channel LED Driver

[Escala, Edelweise]

Hi all,

I’d like to kindly follow up on this patch series.
Is there any additional feedback or action needed from my side to help move it forward upstream?

Thank you for your time and consideration.

Best regards,
Edelweise Escala

Re: [PATCH v5 0/2] Add Support for LTC3219 18 Channel LED Driver

[Lee Jones]

On Thu, 12 Feb 2026, Escala, Edelweise wrote:

> Hi all,
> 
> I’d like to kindly follow up on this patch series.
> Is there any additional feedback or action needed from my side to help move it forward upstream?
> 
> Thank you for your time and consideration.

Yes, probably.

However, the merge-window is currently open which mean maintainers and
reviewers are either concentrating on activities or taking a break.

We'll get around to reviewing your submission soon.

-- 
Lee Jones [李琼斯]

Re: [PATCH v5 2/2] leds: ltc3220: Add Support for LTC3220 18 channel LED Driver

[Lee Jones]

On Mon, 26 Jan 2026, Edelweise Escala wrote:

> Add driver for the LTC3220 18-channel LED driver
> with I2C interface, individual brightness control, and hardware-assisted
> blink/gradation features.

Odd line breaks.

> Signed-off-by: Edelweise Escala <edelweise.escala@analog.com>
> ---
>  MAINTAINERS                 |   1 +
>  drivers/leds/Kconfig        |  10 +
>  drivers/leds/Makefile       |   1 +
>  drivers/leds/leds-ltc3220.c | 455 ++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 467 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5c10cc3e3022..7467537938bf 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14961,6 +14961,7 @@ L:	linux-leds@vger.kernel.org
>  S:	Maintained
>  W:	https://ez.analog.com/linux-software-drivers
>  F:	Documentation/devicetree/bindings/leds/adi,ltc3220.yaml
> +F:	drivers/leds/leds-ltc3220.c
>  
>  LTC4282 HARDWARE MONITOR DRIVER
>  M:	Nuno Sa <nuno.sa@analog.com>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 597d7a79c988..a1c34b2deded 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -1001,6 +1001,16 @@ config LEDS_ST1202
>  	  Say Y to enable support for LEDs connected to LED1202
>  	  LED driver chips accessed via the I2C bus.
>  
> +config LEDS_LTC3220
> +	tristate "LED Driver for LTC3220/LTC3220-1"

Manufacturer?

> +	depends on I2C && LEDS_CLASS
> +	help
> +	  If you have an 18-Channel LED Driver connected to LTC3220, or LTC3220-1
> +	  say Y here to enable this driver.

More info.  What features does it have?  How is it driven?  Protocol?

> +	  To compile this driver as a module, choose M here: the module will
> +	  be called ltc3220.
> +
>  config LEDS_TPS6105X
>  	tristate "LED support for TI TPS6105X"
>  	depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 8fdb45d5b439..5301568d9e00 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -61,6 +61,7 @@ obj-$(CONFIG_LEDS_LP8788)		+= leds-lp8788.o
>  obj-$(CONFIG_LEDS_LP8860)		+= leds-lp8860.o
>  obj-$(CONFIG_LEDS_LP8864)		+= leds-lp8864.o
>  obj-$(CONFIG_LEDS_LT3593)		+= leds-lt3593.o
> +obj-$(CONFIG_LEDS_LTC3220)		+= leds-ltc3220.o
>  obj-$(CONFIG_LEDS_MAX5970)		+= leds-max5970.o
>  obj-$(CONFIG_LEDS_MAX77650)		+= leds-max77650.o
>  obj-$(CONFIG_LEDS_MAX77705)		+= leds-max77705.o
> diff --git a/drivers/leds/leds-ltc3220.c b/drivers/leds/leds-ltc3220.c
> new file mode 100644
> index 000000000000..6a5d967ae7e8
> --- /dev/null
> +++ b/drivers/leds/leds-ltc3220.c
> @@ -0,0 +1,455 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * LTC3220 18-Channel LED Driver
> + *
> + * Copyright 2026 Analog Devices Inc.
> + *
> + * Author: Edelweise Escala <edelweise.escala@analog.com>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/leds.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/types.h>
> +
> +/* LTC3220 Registers */

This can be made more clear with "REG" in the define names.

> +#define LTC3220_COMMAND				0x00
> +#define LTC3220_ULED(x)				(0x01 + (x))
> +#define LTC3220_GRAD_BLINK			0x13
> +
> +#define LTC3220_GRAD_COUNT_UP		BIT(0)
> +#define LTC3220_COMMAND_QUICK_WRITE	BIT(0)
> +#define LTC3220_COMMAND_SHUTDOWN	BIT(3)

These don't look like registers to me!

It can sometimes help to put the bits under the "REG"s and intent them:

#define LTC3220_COMMAND_REG			0x00
#define   LTC3220_COMMAND_QUICK_WRITE		BIT(0)
#define   LTC3220_COMMAND_SHUTDOWN		BIT(3)

> +#define LTC3220_LED_CURRENT_MASK	GENMASK(5, 0)
> +#define LTC3220_LED_MODE_MASK		GENMASK(7, 6)
> +#define LTC3220_BLINK_MASK			GENMASK(4, 3)
> +#define LTC3220_GRADATION_MASK		GENMASK(2, 1)
> +
> +#define LTC3220_NUM_LEDS 18

Tab with the rest

> +struct ltc3220_command_cfg {
> +	bool is_aggregated;
> +	bool is_shutdown;
> +};
> +
> +struct ltc3220_uled_cfg {
> +	struct ltc3220_state *ltc3220_state;
> +	struct led_classdev led_cdev;
> +	u8 reg_value;
> +	u8 led_index;
> +};
> +
> +struct ltc3220_grad_cfg {

What is "grad"  Any reason to shorten it?

> +	bool is_increasing;
> +	u8 gradation_period_ms;
> +};
> +
> +struct ltc3220_state {
> +	struct ltc3220_command_cfg command_cfg;
> +	struct ltc3220_uled_cfg uled_cfg[LTC3220_NUM_LEDS];
> +	struct ltc3220_grad_cfg grad_cfg;
> +	struct i2c_client *client;

Nit: Put this at the top.

> +	u8 blink_mode;
> +};
> +
> +static int ltc3220_set_command(struct ltc3220_state *ltc3220_state)
> +{
> +	struct i2c_client *client = ltc3220_state->client;
> +	u8 reg_val;
> +
> +	reg_val = FIELD_PREP(LTC3220_COMMAND_SHUTDOWN,
> +			     ltc3220_state->command_cfg.is_shutdown);
> +	reg_val |= FIELD_PREP(LTC3220_COMMAND_QUICK_WRITE,
> +			      ltc3220_state->command_cfg.is_aggregated);

Open these out to use 100-chars to improve readability.

> +	return i2c_smbus_write_byte_data(client, LTC3220_COMMAND, reg_val);
> +}
> +
> +static int ltc3220_shutdown(struct ltc3220_state *ltc3220_state)
> +{
> +	int ret;
> +
> +	ltc3220_state->command_cfg.is_shutdown = true;

-ENOSQUISH - '\n' here.

> +	ret = ltc3220_set_command(ltc3220_state);
> +	if (ret < 0)
> +		ltc3220_state->command_cfg.is_shutdown = false;
> +
> +	return ret;
> +}
> +
> +static int ltc3220_resume_from_shutdown(struct ltc3220_state *ltc3220_state)
> +{
> +	int ret;
> +
> +	ltc3220_state->command_cfg.is_shutdown = false;

-ENOSQUISH - '\n' here.

> +	ret = ltc3220_set_command(ltc3220_state);
> +	if (ret < 0)
> +		ltc3220_state->command_cfg.is_shutdown = true;
> +
> +	return ret;
> +}
> +
> +/*
> + * Set LED brightness and mode.
> + * The brightness value determines both the LED current and operating mode:
> + * 0-63:    Normal mode - LED current from 0-63 (off to full brightness)
> + * 64-127:  Blink mode - LED blinks with current level (brightness - 64)
> + * 128-191: Gradation mode - LED gradually changes brightness (brightness - 128)
> + * 192-255: GPO mode - LED operates as general purpose output (brightness - 192)
> + */
> +static int ltc3220_set_led_data(struct led_classdev *led_cdev,
> +				enum led_brightness brightness)
> +{
> +	struct ltc3220_state *ltc3220_state;
> +	struct ltc3220_uled_cfg *uled_cfg;
> +	int ret;
> +	int i;
> +
> +	uled_cfg = container_of(led_cdev, struct ltc3220_uled_cfg, led_cdev);
> +	ltc3220_state = uled_cfg->ltc3220_state;
> +
> +	ret = i2c_smbus_write_byte_data(ltc3220_state->client,
> +			LTC3220_ULED(uled_cfg->led_index), brightness);
> +	if (ret < 0)
> +		return ret;
> +
> +	uled_cfg->reg_value = brightness;

Tell us what we're doing here.

Tell us why it's okay for this to be over-written below.

> +	/*
> +	 * When aggregated LED mode is enabled, writing to LED 1 updates all
> +	 * LEDs simultaneously via quick-write mode. Update cached values for
> +	 * all LEDs to reflect the synchronized state.
> +	 */
> +	if (ltc3220_state->command_cfg.is_aggregated && uled_cfg->led_index == 0) {
> +		for (i = 0; i < LTC3220_NUM_LEDS; i++)
> +			ltc3220_state->uled_cfg[i].reg_value = brightness;
> +	}
> +
> +	return 0;
> +}
> +
> +static enum led_brightness ltc3220_get_led_data(struct led_classdev *led_cdev)
> +{
> +	struct ltc3220_uled_cfg *uled_cfg;
> +
> +	uled_cfg = container_of(led_cdev, struct ltc3220_uled_cfg, led_cdev);

Do this on the allocation line.

> +	return uled_cfg->reg_value;
> +}
> +
> +static int ltc3220_set_blink_and_gradation(struct ltc3220_state *ltc3220_state,
> +		    u8 blink_cfg, u8 gradation_period_ms, bool is_increasing)

Should line-up with the '('.
> +{
> +	struct i2c_client *client = ltc3220_state->client;
> +	u8 reg_val;
> +
> +	reg_val = FIELD_PREP(LTC3220_BLINK_MASK, blink_cfg);
> +	reg_val |= FIELD_PREP(LTC3220_GRADATION_MASK, gradation_period_ms);
> +	reg_val |= FIELD_PREP(LTC3220_GRAD_COUNT_UP, is_increasing);
> +
> +	return i2c_smbus_write_byte_data(client, LTC3220_GRAD_BLINK, reg_val);
> +}
> +
> +/*
> + * LTC3220 pattern support for hardware-assisted breathing/gradation.
> + * The hardware supports 3 gradation ramp time 240ms, 480ms, 960ms)
> + * and can ramp up or down.
> + *
> + * Pattern array interpretation:
> + *   pattern[0].brightness = start brightness (0-63)
> + *   pattern[0].delta_t = ramp time in milliseconds
> + *   pattern[1].brightness = end brightness (0-63)
> + *   pattern[1].delta_t = (optional, can be 0 or same as pattern[0].delta_t)
> + */
> +static int ltc3220_pattern_set(struct led_classdev *led_cdev,
> +			       struct led_pattern *pattern,
> +			       u32 len, int repeat)
> +{
> +	struct ltc3220_state *ltc3220_state;
> +	struct ltc3220_uled_cfg *uled_cfg;
> +	u8 gradation_period;
> +	u8 start_brightness;
> +	u8 end_brightness;
> +	bool is_increasing;
> +	int ret;
> +
> +	if (len != 2)
> +		return -EINVAL;
> +
> +	uled_cfg = container_of(led_cdev, struct ltc3220_uled_cfg, led_cdev);
> +	ltc3220_state = uled_cfg->ltc3220_state;

Do both of these on the allocation line.

> +
> +	start_brightness = pattern[0].brightness & LTC3220_LED_CURRENT_MASK;
> +	end_brightness = pattern[1].brightness & LTC3220_LED_CURRENT_MASK;

Define these magic numbers.

> +
> +	is_increasing = end_brightness > start_brightness;
> +
> +	if (pattern[0].delta_t == 0)
> +		gradation_period = 0;
> +	else if (pattern[0].delta_t <= 240)
> +		gradation_period = 1;
> +	else if (pattern[0].delta_t <= 480)
> +		gradation_period = 2;
> +	else
> +		gradation_period = 3;
> +
> +	ret = ltc3220_set_blink_and_gradation(ltc3220_state,
> +					       ltc3220_state->blink_mode,
> +					       gradation_period,
> +					       is_increasing);
> +	if (ret < 0)
> +		return ret;
> +
> +	ltc3220_state->grad_cfg.gradation_period_ms = gradation_period;
> +	ltc3220_state->grad_cfg.is_increasing = is_increasing;

It's confusing to have is_increasing as a local variable, a function
parameter and a struct attribute.  Do you really need all of them,
particularly the last 2?

> +	ret = ltc3220_set_led_data(led_cdev, start_brightness);
> +	if (ret < 0)
> +		return ret;
> +
> +	return ltc3220_set_led_data(led_cdev, 128 + end_brightness);

No magic numbers.

> +}
> +
> +static int ltc3220_pattern_clear(struct led_classdev *led_cdev)
> +{
> +	struct ltc3220_state *ltc3220_state;
> +	struct ltc3220_uled_cfg *uled_cfg;
> +	int ret;
> +
> +	uled_cfg = container_of(led_cdev, struct ltc3220_uled_cfg, led_cdev);
> +	ltc3220_state = uled_cfg->ltc3220_state;
> +
> +	ret = ltc3220_set_blink_and_gradation(ltc3220_state,
> +					       ltc3220_state->blink_mode,
> +					       0, false);
> +	if (ret < 0)
> +		return ret;
> +
> +	ltc3220_state->grad_cfg.gradation_period_ms = 0;
> +	ltc3220_state->grad_cfg.is_increasing = false;
> +
> +	return 0;
> +}
> +
> +/*
> + * LTC3220 has a global blink configuration that affects all LEDs.
> + * This implementation allows per-LED blink requests, but the blink timing
> + * will be shared across all LEDs. The delay values are mapped to the
> + * hardware's discrete blink rates.
> + */
> +static int ltc3220_blink_set(struct led_classdev *led_cdev,
> +			     unsigned long *delay_on,
> +			     unsigned long *delay_off)
> +{
> +	struct ltc3220_state *ltc3220_state;
> +	struct ltc3220_uled_cfg *uled_cfg;
> +	unsigned long period;
> +	u8 blink_mode;
> +	int ret;
> +
> +	uled_cfg = container_of(led_cdev, struct ltc3220_uled_cfg, led_cdev);
> +	ltc3220_state = uled_cfg->ltc3220_state;

As above.

> +	if (*delay_on == 0 && *delay_off == 0) {
> +		blink_mode = 1;
> +		*delay_on = 500;
> +		*delay_off = 500;
> +	} else {
> +		period = *delay_on + *delay_off;
> +
> +		if (period <= 750) {
> +			blink_mode = 0;

Define the blink mode.

> +			*delay_on = 250;
> +			*delay_off = 250;
> +		} else if (period <= 1500) {
> +			blink_mode = 1;
> +			*delay_on = 500;
> +			*delay_off = 500;
> +		} else if (period <= 3000) {
> +			blink_mode = 2;
> +			*delay_on = 1000;
> +			*delay_off = 1000;
> +		} else {
> +			blink_mode = 3;
> +			*delay_on = 2000;
> +			*delay_off = 2000;
> +		}
> +	}
> +
> +	ret = ltc3220_set_blink_and_gradation(ltc3220_state, blink_mode,
> +					       ltc3220_state->grad_cfg.gradation_period_ms,
> +					       ltc3220_state->grad_cfg.is_increasing);

Alignment.

> +	if (ret < 0)
> +		return ret;
> +
> +	ltc3220_state->blink_mode = blink_mode;

Again, do we really need this as a function param AND a struct property?

> +	return 0;
> +}
> +
> +static void ltc3220_reset_gpio_action(void *data)
> +{
> +	struct gpio_desc *reset_gpio = data;
> +
> +	gpiod_set_value_cansleep(reset_gpio, 1);
> +}
> +
> +static int ltc3220_reset(struct ltc3220_state *ltc3220_state, struct i2c_client *client)
> +{
> +	struct gpio_desc *reset_gpio;
> +	int ret;
> +	int i;
> +
> +	reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(reset_gpio))
> +		return dev_err_probe(&client->dev, PTR_ERR(reset_gpio), "Failed on reset GPIO\n");
> +
> +	if (reset_gpio) {
> +		gpiod_set_value_cansleep(reset_gpio, 0);
> +
> +		ret = devm_add_action_or_reset(&client->dev, ltc3220_reset_gpio_action, reset_gpio);
> +		if (ret)
> +			return ret;
> +

Do:

return devm_add_action_or_reset(&client->dev, ltc3220_reset_gpio_action, reset_gpio);

And remove the else.

> +	} else {
> +		ret = ltc3220_set_command(ltc3220_state);
> +		if (ret < 0)
> +			return ret;
> +
> +		for (i = 0; i < LTC3220_NUM_LEDS; i++) {
> +			ret = i2c_smbus_write_byte_data(client, LTC3220_ULED(i), 0);
> +			if (ret < 0)
> +				return ret;
> +		}
> +
> +		ret = ltc3220_set_blink_and_gradation(ltc3220_state, 0, 0, 0);

return ltc3220_set_blink_and_gradation(ltc3220_state, 0, 0, 0);

> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;

Drop.

> +}
> +
> +static int ltc3220_suspend(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct ltc3220_state *ltc3220_state = i2c_get_clientdata(client);

Replace these lines with:

struct ltc3220_state *ltc3220_state = i2c_get_clientdata(to_i2c_client(dev));

> +
> +	return ltc3220_shutdown(ltc3220_state);
> +}
> +
> +static int ltc3220_resume(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct ltc3220_state *ltc3220_state = i2c_get_clientdata(client);

As above.

> +
> +	return ltc3220_resume_from_shutdown(ltc3220_state);
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(ltc3220_pm_ops, ltc3220_suspend, ltc3220_resume);
> +
> +static int ltc3220_probe(struct i2c_client *client)
> +{
> +	struct ltc3220_state *ltc3220_state;
> +	bool aggregated_led_found = false;
> +	int num_leds = 0;
> +	u8 i = 0;
> +	int ret;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> +		return dev_err_probe(&client->dev, -EIO, "SMBUS Byte Data not Supported\n");
> +
> +	ltc3220_state = devm_kzalloc(&client->dev, sizeof(*ltc3220_state), GFP_KERNEL);
> +	if (!ltc3220_state)
> +		return -ENOMEM;
> +
> +	ltc3220_state->client = client;
> +	i2c_set_clientdata(client, ltc3220_state);
> +
> +	ret = ltc3220_reset(ltc3220_state, client);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret, "Failed to reset device\n");
> +
> +	device_for_each_child_node_scoped(&client->dev, child) {
> +		struct led_init_data init_data = {};
> +		struct ltc3220_uled_cfg *led;
> +		u32 source;
> +
> +		ret = fwnode_property_read_u32(child, "reg", &source);
> +		if (ret)
> +			return dev_err_probe(&client->dev, ret, "Couldn't read LED address\n");
> +
> +		if (!source || source > LTC3220_NUM_LEDS)
> +			return dev_err_probe(&client->dev, -EINVAL, "LED address out of range\n");
> +
> +		init_data.fwnode = child;
> +		init_data.devicename = "ltc3220";
> +
> +		if (fwnode_property_present(child, "led-sources")) {
> +			if (source != 1)
> +				return dev_err_probe(&client->dev, -EINVAL,
> +							"Aggregated LED out of range\n");
> +
> +			if (aggregated_led_found)

What is an aggregated LED.

Find somewhere to document this.

> +				return dev_err_probe(&client->dev, -EINVAL,
> +							"One Aggregated LED only\n");
> +
> +			aggregated_led_found = true;
> +			ltc3220_state->command_cfg.is_aggregated = true;
> +			ret = ltc3220_set_command(ltc3220_state);
> +			if (ret < 0)
> +				return dev_err_probe(&client->dev, ret, "Failed to set command\n");

In English please.  This will mean nothing to a user.

> +		}
> +
> +		num_leds++;
> +
> +		/* LED node reg/index/address goes from 1 to 18 */
> +		i = source - 1;

This is not an iterator.  'i' is a terrible variable name for !iterators.

> +		led = &ltc3220_state->uled_cfg[i];
> +		led->led_index = i;
> +		led->reg_value = 0;
> +		led->ltc3220_state = ltc3220_state;
> +		led->led_cdev.brightness_set_blocking = ltc3220_set_led_data;
> +		led->led_cdev.brightness_get = ltc3220_get_led_data;
> +		led->led_cdev.max_brightness = 255;
> +		led->led_cdev.blink_set = ltc3220_blink_set;
> +		led->led_cdev.pattern_set = ltc3220_pattern_set;
> +		led->led_cdev.pattern_clear = ltc3220_pattern_clear;
> +
> +		ret = devm_led_classdev_register_ext(&client->dev, &led->led_cdev, &init_data);
> +		if (ret)
> +			return dev_err_probe(&client->dev, ret, "Failed to register LED class\n");
> +	}
> +
> +	if (aggregated_led_found && num_leds > 1)
> +		return dev_err_probe(&client->dev, -EINVAL,
> +					"Aggregated LED must be the only LED node\n");

Must it?  Why?  Where does it say that?

> +
> +	return 0;
> +}
> +
> +static const struct of_device_id ltc3220_of_match[] = {
> +	{ .compatible = "adi,ltc3220" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, ltc3220_of_match);
> +
> +static struct i2c_driver ltc3220_led_driver = {
> +	.driver = {
> +		.name = "ltc3220",
> +		.of_match_table = ltc3220_of_match,
> +		.pm	= pm_sleep_ptr(&ltc3220_pm_ops),

Odd tabbing.

> +	},
> +	.probe = ltc3220_probe,
> +};
> +module_i2c_driver(ltc3220_led_driver);
> +
> +MODULE_AUTHOR("Edelweise Escala <edelweise.escala@analog.com>");
> +MODULE_DESCRIPTION("LED driver for LTC3220 controllers");
> +MODULE_LICENSE("GPL");
> 
> -- 
> 2.43.0
> 

-- 
Lee Jones [李琼斯]

RE: [PATCH v5 2/2] leds: ltc3220: Add Support for LTC3220 18 channel LED Driver

[Escala, Edelweise]

Hello Lee,

Thank you for the Review

> > Add driver for the LTC3220 18-channel LED driver with I2C interface,
> > individual brightness control, and hardware-assisted blink/gradation
> > features.
> 
> Odd line breaks.

Will fix commit line breaks.

> > Signed-off-by: Edelweise Escala <edelweise.escala@analog.com>
> > ---
> >  MAINTAINERS                 |   1 +
> >  drivers/leds/Kconfig        |  10 +
> >  drivers/leds/Makefile       |   1 +
> >  drivers/leds/leds-ltc3220.c | 455
> > ++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 467 insertions(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index
> > 5c10cc3e3022..7467537938bf 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -14961,6 +14961,7 @@ L:	linux-leds@vger.kernel.org
> >  S:	Maintained
> >  W:	https://ez.analog.com/linux-software-drivers
> >  F:	Documentation/devicetree/bindings/leds/adi,ltc3220.yaml
> > +F:	drivers/leds/leds-ltc3220.c
> >
> >  LTC4282 HARDWARE MONITOR DRIVER
> >  M:	Nuno Sa <nuno.sa@analog.com>
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index
> > 597d7a79c988..a1c34b2deded 100644
> > --- a/drivers/leds/Kconfig
> > +++ b/drivers/leds/Kconfig
> > @@ -1001,6 +1001,16 @@ config LEDS_ST1202
> >  	  Say Y to enable support for LEDs connected to LED1202
> >  	  LED driver chips accessed via the I2C bus.
> >
> > +config LEDS_LTC3220
> > +	tristate "LED Driver for LTC3220/LTC3220-1"
> 
> Manufacturer?
> 

Will add Analog Devices Inc.

> > +	depends on I2C && LEDS_CLASS
> > +	help
> > +	  If you have an 18-Channel LED Driver connected to LTC3220, or
> LTC3220-1
> > +	  say Y here to enable this driver.
> 
> More info.  What features does it have?  How is it driven?  Protocol?

Will update to

      This option enables support for the Analog Devices LTC3220
      18-channel LED controller with I2C interface.

      The driver supports individual LED brightness control (64 steps),
      hardware-assisted blinking, gradation/breathing effects, and
	  aggregated control for backlight applications.

      To compile this driver as a module, choose M here: the module will
      be called leds-ltc3220.

> > +	  To compile this driver as a module, choose M here: the module will
> > +	  be called ltc3220.
> > +
> >  config LEDS_TPS6105X
> >  	tristate "LED support for TI TPS6105X"
> >  	depends on LEDS_CLASS
> > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile index
> > 8fdb45d5b439..5301568d9e00 100644
> > --- a/drivers/leds/Makefile
> > +++ b/drivers/leds/Makefile
> > @@ -61,6 +61,7 @@ obj-$(CONFIG_LEDS_LP8788)		+= leds-
> lp8788.o
> >  obj-$(CONFIG_LEDS_LP8860)		+= leds-lp8860.o
> >  obj-$(CONFIG_LEDS_LP8864)		+= leds-lp8864.o
> >  obj-$(CONFIG_LEDS_LT3593)		+= leds-lt3593.o
> > +obj-$(CONFIG_LEDS_LTC3220)		+= leds-ltc3220.o
> >  obj-$(CONFIG_LEDS_MAX5970)		+= leds-max5970.o
> >  obj-$(CONFIG_LEDS_MAX77650)		+= leds-max77650.o
> >  obj-$(CONFIG_LEDS_MAX77705)		+= leds-max77705.o
> > diff --git a/drivers/leds/leds-ltc3220.c b/drivers/leds/leds-ltc3220.c
> > new file mode 100644 index 000000000000..6a5d967ae7e8
> > --- /dev/null
> > +++ b/drivers/leds/leds-ltc3220.c
> > @@ -0,0 +1,455 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * LTC3220 18-Channel LED Driver
> > + *
> > + * Copyright 2026 Analog Devices Inc.
> > + *
> > + * Author: Edelweise Escala <edelweise.escala@analog.com>  */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/i2c.h>
> > +#include <linux/leds.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/types.h>
> > +
> > +/* LTC3220 Registers */
> 
> This can be made more clear with "REG" in the define names.
> 
> > +#define LTC3220_COMMAND				0x00
> > +#define LTC3220_ULED(x)				(0x01 + (x))
> > +#define LTC3220_GRAD_BLINK			0x13
> > +
> > +#define LTC3220_GRAD_COUNT_UP		BIT(0)
> > +#define LTC3220_COMMAND_QUICK_WRITE	BIT(0)
> > +#define LTC3220_COMMAND_SHUTDOWN	BIT(3)
> 
> These don't look like registers to me!
> 
> It can sometimes help to put the bits under the "REG"s and intent them:

Will append REG for register and MASK for bits. Will also rearrange to put bits under REG

> #define LTC3220_COMMAND_REG			0x00
> #define   LTC3220_COMMAND_QUICK_WRITE		BIT(0)
> #define   LTC3220_COMMAND_SHUTDOWN		BIT(3)
> 
> > +#define LTC3220_LED_CURRENT_MASK	GENMASK(5, 0)
> > +#define LTC3220_LED_MODE_MASK		GENMASK(7, 6)
> > +#define LTC3220_BLINK_MASK			GENMASK(4, 3)
> > +#define LTC3220_GRADATION_MASK		GENMASK(2, 1)
> > +
> > +#define LTC3220_NUM_LEDS 18
> 
> Tab with the rest

Will fix tabs

> 
> > +struct ltc3220_command_cfg {
> > +	bool is_aggregated;
> > +	bool is_shutdown;
> > +};
> > +
> > +struct ltc3220_uled_cfg {
> > +	struct ltc3220_state *ltc3220_state;
> > +	struct led_classdev led_cdev;
> > +	u8 reg_value;
> > +	u8 led_index;
> > +};
> > +
> > +struct ltc3220_grad_cfg {
> 
> What is "grad"  Any reason to shorten it?

grad is gradation, will avoid shortening it

> > +	bool is_increasing;
> > +	u8 gradation_period_ms;
> > +};
> > +
> > +struct ltc3220_state {
> > +	struct ltc3220_command_cfg command_cfg;
> > +	struct ltc3220_uled_cfg uled_cfg[LTC3220_NUM_LEDS];
> > +	struct ltc3220_grad_cfg grad_cfg;
> > +	struct i2c_client *client;
> 
> Nit: Put this at the top.

I now removed struct i2c_client *client in favor of regmap this way I can easily update bits to address the comments on using structs

> > +	u8 blink_mode;
> > +};
> > +
> > +static int ltc3220_set_command(struct ltc3220_state *ltc3220_state) {
> > +	struct i2c_client *client = ltc3220_state->client;
> > +	u8 reg_val;
> > +
> > +	reg_val = FIELD_PREP(LTC3220_COMMAND_SHUTDOWN,
> > +			     ltc3220_state->command_cfg.is_shutdown);
> > +	reg_val |= FIELD_PREP(LTC3220_COMMAND_QUICK_WRITE,
> > +			      ltc3220_state->command_cfg.is_aggregated);
> 
> Open these out to use 100-chars to improve readability.

will fix alignemnt

> > +	return i2c_smbus_write_byte_data(client, LTC3220_COMMAND,
> reg_val);
> > +}
> > +
> > +static int ltc3220_shutdown(struct ltc3220_state *ltc3220_state) {
> > +	int ret;
> > +
> > +	ltc3220_state->command_cfg.is_shutdown = true;
> 
> -ENOSQUISH - '\n' here.

will fix alignemnt

> > +	ret = ltc3220_set_command(ltc3220_state);
> > +	if (ret < 0)
> > +		ltc3220_state->command_cfg.is_shutdown = false;
> > +
> > +	return ret;
> > +}
> > +
> > +static int ltc3220_resume_from_shutdown(struct ltc3220_state
> > +*ltc3220_state) {
> > +	int ret;
> > +
> > +	ltc3220_state->command_cfg.is_shutdown = false;
> 
> -ENOSQUISH - '\n' here.

will fix alignment

> > +	ret = ltc3220_set_command(ltc3220_state);
> > +	if (ret < 0)
> > +		ltc3220_state->command_cfg.is_shutdown = true;
> > +
> > +	return ret;
> > +}
> > +
> > +/*
> > + * Set LED brightness and mode.
> > + * The brightness value determines both the LED current and operating mode:
> > + * 0-63:    Normal mode - LED current from 0-63 (off to full brightness)
> > + * 64-127:  Blink mode - LED blinks with current level (brightness -
> > +64)
> > + * 128-191: Gradation mode - LED gradually changes brightness
> > +(brightness - 128)
> > + * 192-255: GPO mode - LED operates as general purpose output
> > +(brightness - 192)  */ static int ltc3220_set_led_data(struct
> > +led_classdev *led_cdev,
> > +				enum led_brightness brightness)
> > +{
> > +	struct ltc3220_state *ltc3220_state;
> > +	struct ltc3220_uled_cfg *uled_cfg;
> > +	int ret;
> > +	int i;
> > +
> > +	uled_cfg = container_of(led_cdev, struct ltc3220_uled_cfg, led_cdev);
> > +	ltc3220_state = uled_cfg->ltc3220_state;
> > +
> > +	ret = i2c_smbus_write_byte_data(ltc3220_state->client,
> > +			LTC3220_ULED(uled_cfg->led_index), brightness);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	uled_cfg->reg_value = brightness;
> 
> Tell us what we're doing here.
> 
> Tell us why it's okay for this to be over-written below.

LTC3220 has a quick write mode this is the is_aggregated flag , when enabled changing value on led1 will change the value for for all leds.
This is why we overwrite the value when aggregated is on. 

> > +	/*
> > +	 * When aggregated LED mode is enabled, writing to LED 1 updates all
> > +	 * LEDs simultaneously via quick-write mode. Update cached values for
> > +	 * all LEDs to reflect the synchronized state.
> > +	 */
> > +	if (ltc3220_state->command_cfg.is_aggregated && uled_cfg->led_index
> == 0) {
> > +		for (i = 0; i < LTC3220_NUM_LEDS; i++)
> > +			ltc3220_state->uled_cfg[i].reg_value = brightness;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static enum led_brightness ltc3220_get_led_data(struct led_classdev
> > +*led_cdev) {
> > +	struct ltc3220_uled_cfg *uled_cfg;
> > +
> > +	uled_cfg = container_of(led_cdev, struct ltc3220_uled_cfg,
> > +led_cdev);
> 
> Do this on the allocation line.

I will fix the allocation line.

> > +	return uled_cfg->reg_value;
> > +}
> > +
> > +static int ltc3220_set_blink_and_gradation(struct ltc3220_state
> *ltc3220_state,
> > +		    u8 blink_cfg, u8 gradation_period_ms, bool is_increasing)
> 
> Should line-up with the '('.

Will fix alignment.

> > +{
> > +	struct i2c_client *client = ltc3220_state->client;
> > +	u8 reg_val;
> > +
> > +	reg_val = FIELD_PREP(LTC3220_BLINK_MASK, blink_cfg);
> > +	reg_val |= FIELD_PREP(LTC3220_GRADATION_MASK,
> gradation_period_ms);
> > +	reg_val |= FIELD_PREP(LTC3220_GRAD_COUNT_UP, is_increasing);
> > +
> > +	return i2c_smbus_write_byte_data(client, LTC3220_GRAD_BLINK,
> > +reg_val); }
> > +
> > +/*
> > + * LTC3220 pattern support for hardware-assisted breathing/gradation.
> > + * The hardware supports 3 gradation ramp time 240ms, 480ms, 960ms)
> > + * and can ramp up or down.
> > + *
> > + * Pattern array interpretation:
> > + *   pattern[0].brightness = start brightness (0-63)
> > + *   pattern[0].delta_t = ramp time in milliseconds
> > + *   pattern[1].brightness = end brightness (0-63)
> > + *   pattern[1].delta_t = (optional, can be 0 or same as pattern[0].delta_t)
> > + */
> > +static int ltc3220_pattern_set(struct led_classdev *led_cdev,
> > +			       struct led_pattern *pattern,
> > +			       u32 len, int repeat)
> > +{
> > +	struct ltc3220_state *ltc3220_state;
> > +	struct ltc3220_uled_cfg *uled_cfg;
> > +	u8 gradation_period;
> > +	u8 start_brightness;
> > +	u8 end_brightness;
> > +	bool is_increasing;
> > +	int ret;
> > +
> > +	if (len != 2)
> > +		return -EINVAL;
> > +
> > +	uled_cfg = container_of(led_cdev, struct ltc3220_uled_cfg, led_cdev);
> > +	ltc3220_state = uled_cfg->ltc3220_state;
> 
> Do both of these on the allocation line.

I will fix the allocation line.

> > +
> > +	start_brightness = pattern[0].brightness &
> LTC3220_LED_CURRENT_MASK;
> > +	end_brightness = pattern[1].brightness &
> LTC3220_LED_CURRENT_MASK;
> 
> Define these magic numbers.

Will fix magic numbers

> > +
> > +	is_increasing = end_brightness > start_brightness;
> > +
> > +	if (pattern[0].delta_t == 0)
> > +		gradation_period = 0;
> > +	else if (pattern[0].delta_t <= 240)
> > +		gradation_period = 1;
> > +	else if (pattern[0].delta_t <= 480)
> > +		gradation_period = 2;
> > +	else
> > +		gradation_period = 3;
> > +
> > +	ret = ltc3220_set_blink_and_gradation(ltc3220_state,
> > +					       ltc3220_state->blink_mode,
> > +					       gradation_period,
> > +					       is_increasing);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ltc3220_state->grad_cfg.gradation_period_ms = gradation_period;
> > +	ltc3220_state->grad_cfg.is_increasing = is_increasing;
> 
> It's confusing to have is_increasing as a local variable, a function parameter and a
> struct attribute.  Do you really need all of them, particularly the last 2?

I plan to just use regmap this way I can easily update bits and now remove the structs

> > +	ret = ltc3220_set_led_data(led_cdev, start_brightness);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return ltc3220_set_led_data(led_cdev, 128 + end_brightness);
> 
> No magic numbers.

Will fix magic numbers

> > +}
> > +
> > +static int ltc3220_pattern_clear(struct led_classdev *led_cdev) {
> > +	struct ltc3220_state *ltc3220_state;
> > +	struct ltc3220_uled_cfg *uled_cfg;
> > +	int ret;
> > +
> > +	uled_cfg = container_of(led_cdev, struct ltc3220_uled_cfg, led_cdev);
> > +	ltc3220_state = uled_cfg->ltc3220_state;
> > +
> > +	ret = ltc3220_set_blink_and_gradation(ltc3220_state,
> > +					       ltc3220_state->blink_mode,
> > +					       0, false);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ltc3220_state->grad_cfg.gradation_period_ms = 0;
> > +	ltc3220_state->grad_cfg.is_increasing = false;
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * LTC3220 has a global blink configuration that affects all LEDs.
> > + * This implementation allows per-LED blink requests, but the blink
> > +timing
> > + * will be shared across all LEDs. The delay values are mapped to the
> > + * hardware's discrete blink rates.
> > + */
> > +static int ltc3220_blink_set(struct led_classdev *led_cdev,
> > +			     unsigned long *delay_on,
> > +			     unsigned long *delay_off)
> > +{
> > +	struct ltc3220_state *ltc3220_state;
> > +	struct ltc3220_uled_cfg *uled_cfg;
> > +	unsigned long period;
> > +	u8 blink_mode;
> > +	int ret;
> > +
> > +	uled_cfg = container_of(led_cdev, struct ltc3220_uled_cfg, led_cdev);
> > +	ltc3220_state = uled_cfg->ltc3220_state;
> 
> As above.

will fix allocation

> > +	if (*delay_on == 0 && *delay_off == 0) {
> > +		blink_mode = 1;
> > +		*delay_on = 500;
> > +		*delay_off = 500;
> > +	} else {
> > +		period = *delay_on + *delay_off;
> > +
> > +		if (period <= 750) {
> > +			blink_mode = 0;
> 
> Define the blink mode.

Will fix the magic numbers. Also corrected the blink timing calculations to
properly match the hardware's supported rates/

> > +			*delay_on = 250;
> > +			*delay_off = 250;
> > +		} else if (period <= 1500) {
> > +			blink_mode = 1;
> > +			*delay_on = 500;
> > +			*delay_off = 500;
> > +		} else if (period <= 3000) {
> > +			blink_mode = 2;
> > +			*delay_on = 1000;
> > +			*delay_off = 1000;
> > +		} else {
> > +			blink_mode = 3;
> > +			*delay_on = 2000;
> > +			*delay_off = 2000;
> > +		}
> > +	}
> > +
> > +	ret = ltc3220_set_blink_and_gradation(ltc3220_state, blink_mode,
> > +					       ltc3220_state-
> >grad_cfg.gradation_period_ms,
> > +					       ltc3220_state-
> >grad_cfg.is_increasing);
> 
> Alignment.

Will fix alignment

> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ltc3220_state->blink_mode = blink_mode;
> 
> Again, do we really need this as a function param AND a struct property?

Like the change on gradation I will now use regmap this way I can easily update bits and now remove the struct property

> > +	return 0;
> > +}
> > +
> > +static void ltc3220_reset_gpio_action(void *data) {
> > +	struct gpio_desc *reset_gpio = data;
> > +
> > +	gpiod_set_value_cansleep(reset_gpio, 1); }
> > +
> > +static int ltc3220_reset(struct ltc3220_state *ltc3220_state, struct
> > +i2c_client *client) {
> > +	struct gpio_desc *reset_gpio;
> > +	int ret;
> > +	int i;
> > +
> > +	reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
> GPIOD_OUT_HIGH);
> > +	if (IS_ERR(reset_gpio))
> > +		return dev_err_probe(&client->dev, PTR_ERR(reset_gpio),
> "Failed on
> > +reset GPIO\n");
> > +
> > +	if (reset_gpio) {
> > +		gpiod_set_value_cansleep(reset_gpio, 0);
> > +
> > +		ret = devm_add_action_or_reset(&client->dev,
> ltc3220_reset_gpio_action, reset_gpio);
> > +		if (ret)
> > +			return ret;
> > +
> 
> Do:
> 
> return devm_add_action_or_reset(&client->dev, ltc3220_reset_gpio_action,
> reset_gpio);
> 
> And remove the else.

Will do this 

> > +	} else {
> > +		ret = ltc3220_set_command(ltc3220_state);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		for (i = 0; i < LTC3220_NUM_LEDS; i++) {
> > +			ret = i2c_smbus_write_byte_data(client,
> LTC3220_ULED(i), 0);
> > +			if (ret < 0)
> > +				return ret;
> > +		}
> > +
> > +		ret = ltc3220_set_blink_and_gradation(ltc3220_state, 0, 0, 0);
> 
> return ltc3220_set_blink_and_gradation(ltc3220_state, 0, 0, 0);
> 
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> 
> Drop.

Will do this

> > +}
> > +
> > +static int ltc3220_suspend(struct device *dev) {
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	struct ltc3220_state *ltc3220_state = i2c_get_clientdata(client);
> 
> Replace these lines with:
> 
> struct ltc3220_state *ltc3220_state = i2c_get_clientdata(to_i2c_client(dev));
>
> > +
> > +	return ltc3220_shutdown(ltc3220_state); }
> > +
> > +static int ltc3220_resume(struct device *dev) {
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	struct ltc3220_state *ltc3220_state = i2c_get_clientdata(client);
> 
> As above.

I will implement this

> > +
> > +	return ltc3220_resume_from_shutdown(ltc3220_state);
> > +}
> > +
> > +static DEFINE_SIMPLE_DEV_PM_OPS(ltc3220_pm_ops, ltc3220_suspend,
> > +ltc3220_resume);
> > +
> > +static int ltc3220_probe(struct i2c_client *client) {
> > +	struct ltc3220_state *ltc3220_state;
> > +	bool aggregated_led_found = false;
> > +	int num_leds = 0;
> > +	u8 i = 0;
> > +	int ret;
> > +
> > +	if (!i2c_check_functionality(client->adapter,
> I2C_FUNC_SMBUS_BYTE_DATA))
> > +		return dev_err_probe(&client->dev, -EIO, "SMBUS Byte Data not
> > +Supported\n");
> > +
> > +	ltc3220_state = devm_kzalloc(&client->dev, sizeof(*ltc3220_state),
> GFP_KERNEL);
> > +	if (!ltc3220_state)
> > +		return -ENOMEM;
> > +
> > +	ltc3220_state->client = client;
> > +	i2c_set_clientdata(client, ltc3220_state);
> > +
> > +	ret = ltc3220_reset(ltc3220_state, client);
> > +	if (ret)
> > +		return dev_err_probe(&client->dev, ret, "Failed to reset
> > +device\n");
> > +
> > +	device_for_each_child_node_scoped(&client->dev, child) {
> > +		struct led_init_data init_data = {};
> > +		struct ltc3220_uled_cfg *led;
> > +		u32 source;
> > +
> > +		ret = fwnode_property_read_u32(child, "reg", &source);
> > +		if (ret)
> > +			return dev_err_probe(&client->dev, ret, "Couldn't read
> LED
> > +address\n");
> > +
> > +		if (!source || source > LTC3220_NUM_LEDS)
> > +			return dev_err_probe(&client->dev, -EINVAL, "LED
> address out of
> > +range\n");
> > +
> > +		init_data.fwnode = child;
> > +		init_data.devicename = "ltc3220";
> > +
> > +		if (fwnode_property_present(child, "led-sources")) {
> > +			if (source != 1)
> > +				return dev_err_probe(&client->dev, -EINVAL,
> > +							"Aggregated LED out of
> range\n");
> > +
> > +			if (aggregated_led_found)
> 
> What is an aggregated LED.
> 
> Find somewhere to document this.

Right now this is the documentation I added in the bindings
description:
          Output channel for the LED (1-18 maps to LED outputs D1-D18).
          For aggregated LED control, define only one LED node with reg = <1>
          and use led-sources to list all controlled outputs. Only reg 1 should
          be present when using led-sources.

> > +				return dev_err_probe(&client->dev, -EINVAL,
> > +							"One Aggregated LED
> only\n");
> > +
> > +			aggregated_led_found = true;
> > +			ltc3220_state->command_cfg.is_aggregated = true;
> > +			ret = ltc3220_set_command(ltc3220_state);
> > +			if (ret < 0)
> > +				return dev_err_probe(&client->dev, ret, "Failed
> to set
> > +command\n");
> 
> In English please.  This will mean nothing to a user.

Will improve logs to "Failed to set quick write mode"

> > +		}
> > +
> > +		num_leds++;
> > +
> > +		/* LED node reg/index/address goes from 1 to 18 */
> > +		i = source - 1;
> 
> This is not an iterator.  'i' is a terrible variable name for !iterators.

changed 'i' to  'led_index' 

> > +		led = &ltc3220_state->uled_cfg[i];
> > +		led->led_index = i;
> > +		led->reg_value = 0;
> > +		led->ltc3220_state = ltc3220_state;
> > +		led->led_cdev.brightness_set_blocking = ltc3220_set_led_data;
> > +		led->led_cdev.brightness_get = ltc3220_get_led_data;
> > +		led->led_cdev.max_brightness = 255;
> > +		led->led_cdev.blink_set = ltc3220_blink_set;
> > +		led->led_cdev.pattern_set = ltc3220_pattern_set;
> > +		led->led_cdev.pattern_clear = ltc3220_pattern_clear;
> > +
> > +		ret = devm_led_classdev_register_ext(&client->dev, &led-
> >led_cdev, &init_data);
> > +		if (ret)
> > +			return dev_err_probe(&client->dev, ret, "Failed to
> register LED class\n");
> > +	}
> > +
> > +	if (aggregated_led_found && num_leds > 1)
> > +		return dev_err_probe(&client->dev, -EINVAL,
> > +					"Aggregated LED must be the only LED
> node\n");
> 
> Must it?  Why?  Where does it say that?

Aggregated LED mode uses the hardware's "quick-write" feature which broadcasts
writes to all 18 channels simultaneously. This is a hardware limitation - when
quick-write mode is enabled, writing to LED channel 1 automatically updates ALL
channels. Controlling LED individually is still possible however if LED 1 is changed all 
LED value will change.

The device tree binding currently supports two mutually exclusive modes:
- Multiple independent LED nodes (quick-write disabled), OR
- Single aggregated LED node with led-sources (quick-write enabled)

This aggregated LED approach was suggested in v2 review:
https://lore.kernel.org/all/20260112-ltc3220-driver-v2-0-d043058fc4df@analog.com/

However, we'd like your recommendation on this design. Would it be better to:
1. Keep the aggregated LED mode with hardware quick-write
2. Drop aggregated mode and let userspace control all 18 LEDs individually
   (userspace can loop to set brightness if synchronized control is needed)

> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id ltc3220_of_match[] = {
> > +	{ .compatible = "adi,ltc3220" },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, ltc3220_of_match);
> > +
> > +static struct i2c_driver ltc3220_led_driver = {
> > +	.driver = {
> > +		.name = "ltc3220",
> > +		.of_match_table = ltc3220_of_match,
> > +		.pm	= pm_sleep_ptr(&ltc3220_pm_ops),
> 
> Odd tabbing.

Will Fix tabbing

> > +	},
> > +	.probe = ltc3220_probe,
> > +};
> > +module_i2c_driver(ltc3220_led_driver);
> > +
> > +MODULE_AUTHOR("Edelweise Escala <edelweise.escala@analog.com>");
> > +MODULE_DESCRIPTION("LED driver for LTC3220 controllers");
> > +MODULE_LICENSE("GPL");
> >
> > --
> > 2.43.0
> >
> 
> --
> Lee Jones [李琼斯]


Best Regards,
Edelweise Escala