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

[Roleda, Jan carlo]

Hello Jones,

Thank you again for the review.

> -----Original Message-----
> From: Lee Jones <lee@xxxxxxxxxx>
> Sent: Thursday, April 30, 2026 11:05 PM
> To: Roleda, Jan carlo <Jancarlo.Roleda@xxxxxxxxxx>
> Cc: Pavel Machek <pavel@xxxxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>;
> Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>; Conor Dooley
> <conor+dt@xxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; linux-
> leds@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v4 2/2] leds: ltc3208: Add driver for LTC3208 Multidisplay
> LED Driver
> 
> [External]
> 
> On Thu, 16 Apr 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 currnet 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 <jancarlo.roleda@xxxxxxxxxx>
> > ---
> >  MAINTAINERS                 |   2 +-
> >  drivers/leds/Kconfig        |  12 ++
> >  drivers/leds/Makefile       |   1 +
> >  drivers/leds/leds-ltc3208.c | 278
> > ++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 292 insertions(+), 1 deletion(-)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index
> > 19b0b84e934d..48bae02057d5 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -15131,7 +15131,7 @@ M:	Jan Carlo Roleda
> <jancarlo.roleda@xxxxxxxxxx>
> >  L:	linux-leds@xxxxxxxxxxxxxxx
> >  S:	Maintained
> >  W:	https://ez.analog.com/linux-software-drivers
> > -F:	Documentation/devicetree/bindings/leds/adi,ltc3208.yaml
> 
> Is this related to this change?  Was it intentional?
> 

Sorry about that.
It appears I didn't update the MAINTAINERS file when I reordered the commits.
I'll add this back in the next patch.

> > +F:	drivers/leds/leds-ltc3208.c
> >
> >  LTC4282 HARDWARE MONITOR DRIVER
> >  M:	Nuno Sa <nuno.sa@xxxxxxxxxx>
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index
> > 597d7a79c988..d13bbec73f06 100644
> > --- a/drivers/leds/Kconfig
> > +++ b/drivers/leds/Kconfig
> > @@ -1029,6 +1029,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
> > 8fdb45d5b439..b08b539112b6 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..9da8f4b359e3
> > --- /dev/null
> > +++ b/drivers/leds/leds-ltc3208.c
> > @@ -0,0 +1,278 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * LED driver for Analog Devices LTC3208 Multi-Display Driver
> > + *
> > + * Copyright 2026 Analog Devices Inc.
> > + *
> > + * Author: Jan Carlo Roleda <jancarlo.roleda@xxxxxxxxxx>  */ #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>
> > +
> > +#define LTC3208_LED_SET_HIGH_BYTE_DATA	GENMASK(7, 4)
> > +#define LTC3208_LED_SET_LOW_BYTE_DATA	GENMASK(3, 0)
> > +
> > +/* Registers */
> > +#define LTC3208_REG_A_GRNRED		0x1 /* Green (High half-byte)
> */
> > +					/* and Red (Low half-byte) current
> DAC*/
> > +#define LTC3208_REG_B_AUXBLU		0x2 /* AUX (High half-byte) */
> > +					/* and Blue (Low half-byte) current
> DAC*/
> > +#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 half-byte
> and Low half-byte) 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
> > +
> > +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
> > +};
> > +
> > +static const char * const ltc3208_dt_aux_channels[] = {
> > +	"adi,aux1-channel", "adi,aux2-channel",
> > +	"adi,aux3-channel", "adi,aux4-channel"
> > +};
> > +
> > +static const char * const ltc3208_aux_opt[] = {
> > +	"aux", "main", "sub", "cam"
> > +};
> > +
> > +struct ltc3208_led {
> > +	struct led_classdev cdev;
> > +	struct i2c_client *client;
> > +	enum ltc3208_channel channel;
> > +};
> > +
> > +struct ltc3208_dev {
> > +	struct i2c_client *client;
> 
> We don't need 2 of these.
> 

Will remove this attribute.

> > +	struct regmap *map;
> 
> 'regmap' throughout.
> 

Will replace the naming as such.

> > +	struct ltc3208_led *leds;
> 
> __counted_by?
> 

Since the number of LEDs are constant throughout,
I think I can replace this with an array declaration instead of a pointer to allocated memory.

struct ltc3208_led leds[LTC3208_NUM_LED_GRPS];

I'll update the probe function to follow this array instead.

> > +};
> > +
> > +static const struct regmap_config ltc3208_regmap_cfg = {
> > +	.reg_bits = 8,
> > +	.val_bits = 8,
> > +};
> 
> '.cache_type'?
> 

Will add a cache_type as 'REGCACHE_FLAT_S`.

> > +
> > +static int ltc3208_led_set_current_low(struct regmap *regmap, u8 reg,
> > +u8 level) {
> > +	return regmap_update_bits(regmap, reg,
> > +LTC3208_LED_SET_LOW_BYTE_DATA, level); }
> > +
> > +static int ltc3208_led_set_current_high(struct regmap *regmap, u8
> > +reg, u8 level) {
> > +	return regmap_update_bits(regmap, reg,
> > +LTC3208_LED_SET_HIGH_BYTE_DATA, level); }
> 
> No abstraction for the sake of it.  Use regmap_update_bits() in place instead.
> 
> It looks like 'level' is not shifted here, which means 'level & mask' inside
> 'regmap_update_bits' will evaluate to 0.  Could we use 'regmap_field' for these
> bit-level accesses instead, as it handles the shifting automatically?
> 

Noted. Will implement your suggestion.

> > +
> > +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);
> > +	struct i2c_client *client = led->client;
> > +	struct ltc3208_dev *dev = i2c_get_clientdata(client);
> 
> This confused me for a moment.
> 
> Drop the '_dev" part and call the variable 'ddata'.
> 

Noted. Will replace this to your suggestion.

> > +	struct regmap *regmap = dev->map;
> 
> 'regmap' throughout.
> 
> > +	u8 current_level = brightness;
> > +
> > +	switch (led->channel) {
> > +	case LTC3208_CHAN_MAIN:
> > +		return regmap_write(regmap, LTC3208_REG_C_MAIN,
> current_level);
> > +	case LTC3208_CHAN_SUB:
> > +		return regmap_write(regmap, LTC3208_REG_D_SUB,
> current_level);
> > +	case LTC3208_CHAN_AUX:
> > +		return ltc3208_led_set_current_high(regmap,
> LTC3208_REG_B_AUXBLU, current_level);
> > +	case LTC3208_CHAN_BLUE:
> > +		return ltc3208_led_set_current_low(regmap,
> LTC3208_REG_B_AUXBLU, current_level);
> > +	case LTC3208_CHAN_CAMH:
> > +		return ltc3208_led_set_current_high(regmap,
> LTC3208_REG_F_CAM, current_level);
> > +	case LTC3208_CHAN_CAML:
> > +		return ltc3208_led_set_current_low(regmap,
> LTC3208_REG_F_CAM, current_level);
> > +	case LTC3208_CHAN_GREEN:
> > +		return ltc3208_led_set_current_high(regmap,
> LTC3208_REG_A_GRNRED, current_level);
> > +	case LTC3208_CHAN_RED:
> > +		return ltc3208_led_set_current_low(regmap,
> LTC3208_REG_A_GRNRED, current_level);
> > +	default:
> > +		dev_err(&client->dev, "Invalid LED Channel\n");
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static int ltc3208_update_options(struct ltc3208_dev *dev,
> > +				  bool is_sub, bool is_cam_hi, bool
> is_rgb_drop) {
> 
> Since this helper function only has one call site, should we inline its logic
> directly into the probe function to reduce superfluous abstractions?
> 

Noted. I will make this and succeeding mentioned functions into inlines.

> > +	struct regmap *map = dev->map;
> > +	u8 val;
> > +
> > +	val =	FIELD_PREP(LTC3208_OPT_EN_RGBS, is_sub) |
> > +		FIELD_PREP(LTC3208_OPT_DIS_CAMHILO, is_cam_hi) |
> > +		FIELD_PREP(LTC3208_OPT_DIS_RGBDROP, is_rgb_drop);
> > +
> > +	return regmap_write(map, LTC3208_REG_G_OPT, val); }
> > +
> > +static int ltc3208_update_aux_dac(struct ltc3208_dev *dev, enum
> > +ltc3208_aux_channel *aux_chan) {
> 
> Similarly, as this function is only called once, could we inline its logic into the
> probe function?
> 
> > +	struct regmap *map = dev->map;
> > +	u8 val;
> > +
> > +	val =	FIELD_PREP(LTC3208_AUX1_MASK, aux_chan[0]) |
> > +		FIELD_PREP(LTC3208_AUX2_MASK, aux_chan[1]) |
> > +		FIELD_PREP(LTC3208_AUX3_MASK, aux_chan[2]) |
> > +		FIELD_PREP(LTC3208_AUX4_MASK, aux_chan[3]);
> > +
> > +	return regmap_write(map, LTC3208_REG_E_AUX_SELECT, val); }
> > +
> > +static int ltc3208_probe(struct i2c_client *client) {
> > +	enum ltc3208_aux_channel
> aux_channels[LTC3208_NUM_AUX_LEDS];
> > +	struct ltc3208_dev *dev_data;
> 
> Consistency.
> 
> > +	struct ltc3208_led *leds;
> > +	struct regmap *regmap;
> > +	int ret;
> > +	u32 val;
> 
> 'val' is a weak variable name.  How about 'chan'?
> 
> > +	bool dropdis_rgb_aux4;
> > +	bool dis_camhl;
> > +	bool en_rgbs;
> 
> All of these variable names are non-forthcoming.
> 
> Please improve the nomenclature of each of them.
> 

Will change the mentioned variable names to the following:

struct ltc3208 ddata;
u32 chan;
bool enable_rgb_aux4_dropout_signal;
bool enable_camhl_pin;
bool set_rgb_control_pin;

> > +	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");
> > +
> > +	dev_data = devm_kzalloc(&client->dev, sizeof(*dev_data),
> GFP_KERNEL);
> > +	if (!dev_data)
> > +		return -ENOMEM;
> > +
> > +	leds = devm_kcalloc(&client->dev, LTC3208_NUM_LED_GRPS,
> > +			    sizeof(struct ltc3208_led), GFP_KERNEL)u
> > +	if (!leds)
> > +		return -ENOMEM;
> > +
> > +	dev_data->client = client;
> > +	dev_data->map = regmap;
> > +
> > +	dis_camhl = device_property_read_bool(&client->dev, "adi,disable-
> camhl-pin");
> > +	en_rgbs = device_property_read_bool(&client->dev, "adi,cfg-enrgbs-
> pin");
> > +	dropdis_rgb_aux4 = device_property_read_bool(&client->dev,
> > +"adi,disable-rgb-aux4-dropout");
> > +
> > +	ret = ltc3208_update_options(dev_data, en_rgbs, dis_camhl,
> > +				     dropdis_rgb_aux4);
> > +	if (ret)
> > +		return dev_err_probe(&client->dev, ret, "error writing to
> options
> > +register\n");
> > +
> > +	/* Initialize aux channel configurations from devicetree */
> 
> No need to shorten words in comments and drop the "from devicetree"
> part.  It doesn't really add anything.
> 

Noted. Will update these comments.

> > +	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);
> > +		/* Use default value if absent in devicetree */
> 
> "Fall-back to default value if not found"
> 
> > +		if (ret == -EINVAL)
> > +			aux_channels[i] = LTC3208_AUX_CHAN_AUX;
> > +		else if (ret >= 0)
> > +			aux_channels[i] = ret;
> > +		else
> > +			return dev_err_probe(&client->dev, ret,
> > +					     "Failed getting aux-channel %d\n",
> i);
> 
> "Failed to set the auxiliary channel"
> 
> But when would this happen?
> 
> If we have an acceptable default value, why not use it?
> 

Will drop this error case and update the comment.

> > +	}
> > +
> > +	ret = ltc3208_update_aux_dac(dev_data, aux_channels);
> > +	if (ret)
> > +		return dev_err_probe(&client->dev, ret, "error writing to aux
> > +channel register.\n");
> > +
> > +	i2c_set_clientdata(client, dev_data);
> > +
> > +	device_for_each_child_node_scoped(&client->dev, child) {
> > +		struct ltc3208_led *led;
> > +		struct led_init_data init_data = {};
> > +
> > +		ret = fwnode_property_read_u32(child, "reg", &val);
> > +		if (ret)
> > +			return dev_err_probe(&client->dev, -EINVAL,
> > +					     "Failed to get reg value of LED.\n");
> 
> Could we propagate the original error code returned by
> 'fwnode_property_read_u32' instead of hard-coding '-EINVAL' here?
> 

Will pass in ret instead.

> > +		else if (val >= LTC3208_NUM_LED_GRPS)
> > +			return dev_err_probe(&client->dev, -EINVAL,
> > +					     "LED reg value not supported.\n");
> 
> "LED channel out of range" ?
> 

Will update the error messages to this.

> > +
> > +		led = &leds[val];
> > +		led->client = client;
> > +		led->channel = val;
> > +		led->cdev.brightness_set_blocking =
> ltc3208_led_set_brightness;
> > +		led->cdev.max_brightness =
> LTC3208_MAX_BRIGHTNESS_4BIT;
> 
> Nit: '\n' here.
> 

Noted.

> > +		if (val == LTC3208_CHAN_MAIN || val ==
> 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);
> 
> Surely this is shorter than the one 2 lines down?  Use 100-chars throughout.
> 

Will re-evaluate my formatting again.

> > +		if (ret)
> > +			return dev_err_probe(&client->dev, ret, "Failed to
> register LED %u\n", val);
> > +	}
> > +
> > +	dev_data->leds = leds;
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id ltc3208_match_table[] = {
> > +	{.compatible = "adi,ltc3208"},
> 
> Spaces between the '{' and '}'.

Noted. Will remove these spaces.

> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, ltc3208_match_table);
> > +
> > +static const struct i2c_device_id ltc3208_idtable[] = {
> > +	{ "ltc3208" },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, ltc3208_idtable);
> > +
> > +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 <jancarlo.roleda@xxxxxxxxxx>");
> > +MODULE_DESCRIPTION("LTC3208 LED Driver");
> 
> --
> Lee Jones