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@xxxxxxxxxx>
> > ---
> >  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@xxxxxxxxxxxxxxx
> >  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@xxxxxxxxxx>
> > 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@xxxxxxxxxx>  */
> > +
> > +#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@xxxxxxxxxx/

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@xxxxxxxxxx>");
> > +MODULE_DESCRIPTION("LED driver for LTC3220 controllers");
> > +MODULE_LICENSE("GPL");
> >
> > --
> > 2.43.0
> >
> 
> --
> Lee Jones [李琼斯]


Best Regards,
Edelweise Escala