RE: [PATCH v7 2/2] leds: ltc3208: Add driver for LTC3208 Multidisplay LED Driver

Hello Jones,

Thanks for the comprehensive review.

> -----Original Message-----
> From: Lee Jones <[email protected]>
> Sent: Thursday, July 16, 2026 10:25 PM
> To: Roleda, Jan carlo <[email protected]>
> Cc: Pavel Machek <[email protected]>; Rob Herring <[email protected]>;
> Krzysztof Kozlowski <[email protected]>; Conor Dooley
> <[email protected]>; [email protected]; linux-
> [email protected]; [email protected]; Uwe Kleine König
> <[email protected]>
> Subject: Re: [PATCH v7 2/2] leds: ltc3208: Add driver for LTC3208 Multidisplay
> LED Driver
> 
> [External]
> 
> On Wed, 08 Jul 2026, Jan Carlo Roleda wrote:
> 
> > Kernel driver implementation for LTC3208 Multidisplay LED Driver.
> >
> > The LTC3208 is a Multi-display LED driver, designed to control up to
> > 7 distinct LED channels (MAIN, SUB, AUX, CAMHI, CAMLO, RED, GREEN,
> > BLUE), each configurable with its own current level that is equally
> > set to its respective output current source pins for external LEDs.
> >
> > It is programmed via the I2C serial interface.
> > MAIN and SUB support 8-bit current level resolution, while AUX,
> > CAMHI/LO, RED, GREEN, and BLUE support 4-bit levels.
> >
> > The AUX LED channel can be configured to mirror the CAM, SUB, and MAIN
> > channel current levels, or as its own independent AUX channel.
> >
> > The CAM LED channel is configured as 2 separate CAMHI and CAMLO
> > register sub-channels, which current is selected via the CAMHL pin, or
> > set to CAMHI register only via setting the S_CAMHILO bit high in register G
> (0x7).
> >
> > Signed-off-by: Jan Carlo Roleda <[email protected]>
> > ---
> >  MAINTAINERS                 |   1 +
> >  drivers/leds/Kconfig        |  12 +++
> >  drivers/leds/Makefile       |   1 +
> >  drivers/leds/leds-ltc3208.c | 251
> > ++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 265 insertions(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index
> > 2fd6ffdaaf04..e3b59485ecb3 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -15229,6 +15229,7 @@ L:	[email protected]
> >  S:	Maintained
> >  W:	https://ez.analog.com/linux-software-drivers
> >  F:	Documentation/devicetree/bindings/leds/adi,ltc3208.yaml
> > +F:	drivers/leds/leds-ltc3208.c
> >
> >  LTC4282 HARDWARE MONITOR DRIVER
> >  M:	Nuno Sa <[email protected]>
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index
> > f4a0a3c8c870..d917ce3b72f4 100644
> > --- a/drivers/leds/Kconfig
> > +++ b/drivers/leds/Kconfig
> > @@ -1028,6 +1028,18 @@ config LEDS_ACER_A500
> >  	  This option enables support for the Power Button LED of
> >  	  Acer Iconia Tab A500.
> >
> > +config LEDS_LTC3208
> > +	tristate "LED Driver for Analog Devices LTC3208"
> > +	depends on LEDS_CLASS && I2C
> > +	select REGMAP_I2C
> > +	help
> > +	  Say Y to enable the LTC3208 LED driver.
> > +	  This enables the LED device LTC3208, a 7-channel, 17-current source
> > +	  multidisplay high-current LED driver, configured via I2C.
> > +
> > +	  To compile this driver as a module, choose M here: the module will
> > +	  be called ltc3208.
> > +
> >  source "drivers/leds/blink/Kconfig"
> >
> >  comment "Flash and Torch LED drivers"
> > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile index
> > 7db3768912ca..0148b87e16ba 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_LTC3208)		+= leds-ltc3208.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-ltc3208.c b/drivers/leds/leds-ltc3208.c
> > new file mode 100644 index 000000000000..484690e3763c
> > --- /dev/null
> > +++ b/drivers/leds/leds-ltc3208.c
> > @@ -0,0 +1,251 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * LED driver for Analog Devices LTC3208 Multi-Display Driver
> > + *
> > + * Copyright 2026 Analog Devices Inc.
> > + *
> > + * Author: Jan Carlo Roleda <[email protected]>  */
> >
> >  Nit: \n here.
> >
> > +#include <linux/bitfield.h>
> > +#include <linux/errno.h>
> > +#include <linux/i2c.h>
> > +#include <linux/leds.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/property.h>
> > +#include <linux/regmap.h>
> > +#include <linux/types.h>
> > +
> > +/* Registers */
> > +#define LTC3208_REG_A_GRNRED 0x1 /* Green and Red current DAC*/
> 
> Are these the registers from the datasheet?
> 

Yes, these are specified in the datasheet.

> > +#define LTC3208_REG_B_AUXBLU 0x2 /* AUX and Blue current DAC*/
> 
> You're missing a space at the end of some of these comments.
> 

Noted.

> > +#define LTC3208_REG_C_MAIN 0x3 /* Main current DAC */ #define
> > +LTC3208_REG_D_SUB 0x4 /* Sub current DAC */ #define
> > +LTC3208_REG_E_AUX_SELECT 0x5 /* AUX DAC Select */ #define
> > +LTC3208_AUX1_MASK GENMASK(1, 0) #define  LTC3208_AUX2_MASK
> GENMASK(3,
> > +2) #define  LTC3208_AUX3_MASK GENMASK(5, 4) #define
> > +LTC3208_AUX4_MASK GENMASK(7, 6) #define LTC3208_REG_F_CAM 0x6 /*
> CAM
> > +(High and Low) current DAC*/ #define LTC3208_REG_G_OPT 0x7 /* Device
> > +Options */ #define  LTC3208_OPT_CPO_MASK GENMASK(7, 6) #define
> > +LTC3208_OPT_DIS_RGBDROP BIT(3) #define  LTC3208_OPT_DIS_CAMHILO
> > +BIT(2) #define  LTC3208_OPT_EN_RGBS BIT(1)
> > +
> > +#define LTC3208_MAX_BRIGHTNESS_4BIT 0xF #define
> > +LTC3208_MAX_BRIGHTNESS_8BIT 0xFF
> > +
> > +#define LTC3208_NUM_LED_GRPS 8
> > +#define LTC3208_NUM_AUX_LEDS 4
> > +
> > +#define LTC3208_NUM_AUX_OPT 4
> > +#define LTC3208_MAX_CPO_OPT 3
> 
>  The values above are mostly unreadable.
> 
>  Can you tab all of the values above so that they line-up.
> > +
> > +enum ltc3208_aux_channel {
> > +	LTC3208_AUX_CHAN_AUX = 0,
> > +	LTC3208_AUX_CHAN_MAIN,
> > +	LTC3208_AUX_CHAN_SUB,
> > +	LTC3208_AUX_CHAN_CAM
> > +};
> > +
> > +enum ltc3208_channel {
> > +	LTC3208_CHAN_MAIN = 0,
> > +	LTC3208_CHAN_SUB,
> > +	LTC3208_CHAN_AUX,
> > +	LTC3208_CHAN_CAML,
> > +	LTC3208_CHAN_CAMH,
> > +	LTC3208_CHAN_RED,
> > +	LTC3208_CHAN_BLUE,
> > +	LTC3208_CHAN_GREEN,
> > +	LTC3208_CHAN_N_COUNT,
> > +};
> > +
> > +static const char *const ltc3208_dt_aux_channels[] = {
> > +"adi,aux1-channel",
> 
>  Why would you start the list at the end of the line like this?
> 
>  Please move it down as you do with ltc3208_led, etc.
> 

Noted. Will adjust it to be:

static const char *const ltc3208_dt_aux_channels[] = {
	"adi,aux1-channel",
	"adi,aux2-channel",
	"adi,aux3-channel",
	"adi,aux4-channel"
};

> > +						       "adi,aux2-channel",
> > +						       "adi,aux3-channel",
> > +						       "adi,aux4-channel" };
> > +
> > +static const char *const ltc3208_aux_opt[] = { "aux", "main", "sub",
> > +"cam" };
> 
>  This is okay.
> 
> > +struct ltc3208_led {
> > +	struct led_classdev cdev;
> > +	struct i2c_client *client;
> 
> Where is client used?
> 

I hadn't realized that it wasn't used anymore by the device, since regmap_field had replaced its role.
I will remove this from the struct.

> > +	struct regmap_field *rfield;
> > +};
> > +
> > +struct ltc3208 {
> 
>  I'm struggling to see the point in this.
> 

I wasn't sure how to approach this, as I mistakenly assumed that there needed to be only a single instance containing the runtime data of the device.

I will replace this by allocating memory for each LED directly instead:

// in LED initialization loop
led = devm_kzalloc(&client->dev, sizeof(*led), GFP_KERNEL);

Please let me know if there is a more appropriate approach for this.

> > +	struct ltc3208_led leds[LTC3208_NUM_LED_GRPS]; };
> > +
> > +static const struct reg_default ltc3208_reg_defaults[LTC3208_REG_G_OPT]
> = {
> > +	{LTC3208_REG_A_GRNRED, 0},
> > +	{LTC3208_REG_B_AUXBLU, 0},
> > +	{LTC3208_REG_C_MAIN, 0},
> > +	{LTC3208_REG_D_SUB, 0},
> > +	{LTC3208_REG_E_AUX_SELECT, 0},
> > +	{LTC3208_REG_F_CAM, 0},
> > +	{LTC3208_REG_G_OPT, 0}
> 
> Spaces before and after the } and { respectively.
> 
> Doesn't checkpatch.pl tell you about this stuff?
> 

Sorry about that.
I'll make sure to always pass this through checkpatch first.

> > +};
> > +
> > +static const struct regmap_config ltc3208_regmap_cfg = {
> > +	.reg_bits = 8,
> > +	.val_bits = 8,
> > +	.max_register = LTC3208_REG_G_OPT,
> > +	.cache_type = REGCACHE_FLAT_S,
> > +	.reg_defaults = ltc3208_reg_defaults,
> > +	.num_reg_defaults = LTC3208_REG_G_OPT, };
> > +
> > +static const struct reg_field
> ltc3208_led_reg_field[LTC3208_CHAN_N_COUNT] = {
> > +	[LTC3208_CHAN_MAIN] =  REG_FIELD(LTC3208_REG_C_MAIN, 0, 7),
> > +	[LTC3208_CHAN_SUB] =   REG_FIELD(LTC3208_REG_D_SUB, 0, 7),
> > +	[LTC3208_CHAN_BLUE] =  REG_FIELD(LTC3208_REG_B_AUXBLU, 0, 3),
> > +	[LTC3208_CHAN_AUX] =   REG_FIELD(LTC3208_REG_B_AUXBLU, 4, 7),
> > +	[LTC3208_CHAN_CAML] =  REG_FIELD(LTC3208_REG_F_CAM, 0, 3),
> > +	[LTC3208_CHAN_CAMH] =  REG_FIELD(LTC3208_REG_F_CAM, 4, 7),
> > +	[LTC3208_CHAN_RED] =   REG_FIELD(LTC3208_REG_A_GRNRED, 0, 3),
> > +	[LTC3208_CHAN_GREEN] = REG_FIELD(LTC3208_REG_A_GRNRED, 4,
> 7),
> 
> If you're going to insist on lining these up, I'd start with the =.
> 

Noted.

> > +};
> > +
> > +static int ltc3208_led_set_brightness(struct led_classdev *led_cdev,
> > +				      enum led_brightness brightness) {
> > +	struct ltc3208_led *led =
> > +		container_of(led_cdev, struct ltc3208_led, cdev);
> > +	u8 current_level = brightness;
> > +
> > +	return regmap_field_write(led->rfield, current_level); }
> > +
> > +static int ltc3208_probe(struct i2c_client *client) {
> > +	enum ltc3208_aux_channel
> aux_channels[LTC3208_NUM_AUX_LEDS];
> > +	struct ltc3208 *ddata;
> > +	struct regmap *regmap;
> > +	bool disable_rgb_aux4_dropout_signal;
> > +	bool disable_camhl_pin;
> > +	bool set_sub_control_pin;
> > +	int ret;
> > +	u8 reg_val;
> 
> Something more descriptive like 'options'?
> 

Noted. Will replace reg_val with dev_options.

> > +
> > +	regmap = devm_regmap_init_i2c(client, &ltc3208_regmap_cfg);
> > +	if (IS_ERR(regmap))
> > +		return dev_err_probe(&client->dev, PTR_ERR(regmap),
> > +				     "Failed to initialize regmap\n");
> > +
> > +	ddata = devm_kzalloc(&client->dev, sizeof(*ddata), GFP_KERNEL);
> > +	if (!ddata)
> > +		return -ENOMEM;
> > +
> > +	disable_camhl_pin = device_property_read_bool(&client->dev,
> > +						      "adi,disable-camhl-pin");
> 
> You can use 100-chars everywhere.  Does this help to prevent the ugly wrap?
> 

Will update the lines that can fit 100-chars.

> > +	set_sub_control_pin =
> > +		device_property_read_bool(&client->dev, "adi,cfg-enrgbs-
> pin");
> 
> If you _must_ wrap, do it after the ','.
> 
> > +	disable_rgb_aux4_dropout_signal =
> 
> As above.
> 

Noted.

> > +				device_property_read_bool(&client->dev,
> > +						"adi,disable-rgb-aux4-
> dropout");
> > +
> > +	reg_val = FIELD_PREP(LTC3208_OPT_EN_RGBS, set_sub_control_pin) |
> > +		  FIELD_PREP(LTC3208_OPT_DIS_CAMHILO,
> disable_camhl_pin) |
> > +		  FIELD_PREP(LTC3208_OPT_CPO_MASK, 0) |
> > +		  FIELD_PREP(LTC3208_OPT_DIS_RGBDROP,
> > +			     disable_rgb_aux4_dropout_signal);
> > +
> > +	ret = regmap_write(regmap, LTC3208_REG_G_OPT, reg_val);
> > +	if (ret)
> > +		return dev_err_probe(&client->dev, ret,
> > +				     "error writing to options register\n");
> 
> This isn't very user friendly.
> 
> "Failed to set <soemething> options"
> 
> > +
> > +	/* Initialize aux channel configurations */
> > +	for (int i = 0; i < LTC3208_NUM_AUX_LEDS; i++) {
> > +		ret = device_property_match_property_string(
> > +			&client->dev, ltc3208_dt_aux_channels[i],
> > +			ltc3208_aux_opt, LTC3208_NUM_AUX_OPT);
> > +		/* Fallback to default value (AUX) if not found */
> > +		if (ret == -ENODATA || ret == -EINVAL)
> > +			aux_channels[i] = LTC3208_AUX_CHAN_AUX;
> > +		else if (ret < 0)
> > +			return dev_err_probe(&client->dev, ret,
> > +					     "Error reading AUX Channel %d", i);
> 
> New line?
> 
> This whole block is a readability nightmare, please re-format.
> 

Noted. Will look into a way to refactor this.

> > +		else if (ret >= 0)
> > +			aux_channels[i] = ret;
> > +	}
> > +
> > +	reg_val = FIELD_PREP(LTC3208_AUX1_MASK, aux_channels[0]) |
> > +		  FIELD_PREP(LTC3208_AUX2_MASK, aux_channels[1]) |
> > +		  FIELD_PREP(LTC3208_AUX3_MASK, aux_channels[2]) |
> > +		  FIELD_PREP(LTC3208_AUX4_MASK, aux_channels[3]);
> > +
> > +	ret = regmap_write(regmap, LTC3208_REG_E_AUX_SELECT, reg_val);
> > +	if (ret)
> > +		return dev_err_probe(&client->dev, ret,
> > +			"error writing to aux channel register.\n");
> 
> Full-stops are not required in error messages.
> 
> Again, make it more user friendly please.
> 
> > +
> > +	device_for_each_child_node_scoped(&client->dev, child) {
> > +		struct ltc3208_led *led;
> > +		struct led_init_data init_data = {};
> > +		u32 chan;
> > +
> > +		ret = fwnode_property_read_u32(child, "reg", &chan);
> > +		if (ret)
> > +			return dev_err_probe(&client->dev, ret,
> > +					    "Failed to get reg value of LED\n");
> > +		else if (chan >= LTC3208_NUM_LED_GRPS)
> 
> No need for the else.
> 
> > +			return dev_err_probe(&client->dev, -EINVAL,
> > +					     "%u is an invalid LED ID\n", chan);
> > +		else if (ddata->leds[chan].client)
> 
> Here too.
> 
> > +			return dev_err_probe(&client->dev, -EINVAL,
> > +					"%u is already registered\n", chan);
> > +
> > +		led = &ddata->leds[chan];
> > +
> > +		led->rfield = devm_regmap_field_alloc(&client->dev, regmap,
> > +						ltc3208_led_reg_field[chan]);
> > +		if (IS_ERR(led->rfield))
> > +			return dev_err_probe(&client->dev, PTR_ERR(led-
> >rfield),
> > +					     "cannot allocate regmap field\n");
> 
> '\n' here.
> 

Noted on these comments. Will update the error messages.

> > +		led->client = client;
> > +		led->cdev.brightness_set_blocking =
> ltc3208_led_set_brightness;
> > +		led->cdev.max_brightness =
> LTC3208_MAX_BRIGHTNESS_4BIT;
> > +
> > +		if (chan == LTC3208_CHAN_MAIN || chan ==
> LTC3208_CHAN_SUB)
> > +			led->cdev.max_brightness =
> LTC3208_MAX_BRIGHTNESS_8BIT;
> > +
> > +		init_data.fwnode = child;
> > +
> > +		ret = devm_led_classdev_register_ext(&client->dev, &led-
> >cdev,
> > +						     &init_data);
> > +		if (ret)
> > +			return dev_err_probe(&client->dev, ret,
> > +					     "LED %u Register failed.\n", chan);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id ltc3208_match_table[] = {
> > +	{ .compatible = "adi,ltc3208" },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, ltc3208_match_table);
> > +
> > +static const struct i2c_device_id ltc3208_idtable[] = {
> > +	{ .name = "ltc3208" },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, ltc3208_idtable);
> 
> My memory is fuzzy on this, but I don't think you need i2c_device_id structs if
> the elements aren't numbered.
> 

Noted. Will review this section.

> > +static struct i2c_driver ltc3208_driver = {
> > +	.driver = {
> > +		.name = "ltc3208",
> > +		.of_match_table = ltc3208_match_table,
> > +	},
> > +	.id_table = ltc3208_idtable,
> > +	.probe = ltc3208_probe,
> > +};
> > +module_i2c_driver(ltc3208_driver);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Jan Carlo Roleda <[email protected]>");
> > +MODULE_DESCRIPTION("LTC3208 LED Driver");
> >
> > --
> > 2.43.0
> >
> 
> --
> Lee Jones

I will review these comments and try to submit another patch by July 29.

Thank you again!

Regards,
Carlo