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

[Lee Jones <lee@kernel.org>]

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 [李琼斯]