RE: [PATCH v3 1/2] leds: ltc3208: add driver
From: Roleda, Jan carlo
Date: Sun Apr 12 2026 - 19:32:40 EST
Hello Jones,
Thank you again for the extensive reviews.
> -----Original Message-----
> From: Lee Jones <lee@xxxxxxxxxx>
> Sent: Friday, April 10, 2026 12:59 AM
> 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 v3 1/2] leds: ltc3208: add driver
>
> [External]
>
> "Add driver" is not a good subject line.
>
> > Kernel driver implementation for LTC3208 Multidisplay LED Driver
>
> A one line commit messages is not suitable fore a 300 line driver!
>
> What is the LTC3208 Multidisplay LED?
> What does it do?
> How does it operate?
> What's special about it?
> Any quirks?
I'll provide a more detailed commit description for next patch.
> > Signed-off-by: Jan Carlo Roleda <jancarlo.roleda@xxxxxxxxxx>
> > ---
> > MAINTAINERS | 7 ++
> > drivers/leds/Kconfig | 11 ++
> > drivers/leds/Makefile | 1 +
> > drivers/leds/leds-ltc3208.c | 298
> > ++++++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 317 insertions(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index
> > 55af015174a5..48bae02057d5 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -15126,6 +15126,13 @@ W: https://ez.analog.com/linux-software-
> drivers
> > F: Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml
> > F: drivers/iio/temperature/ltc2983.c
> >
> > +LTC3208 LED DRIVER
> > +M: Jan Carlo Roleda <jancarlo.roleda@xxxxxxxxxx>
> > +L: linux-leds@xxxxxxxxxxxxxxx
> > +S: Maintained
> > +W: https://ez.analog.com/linux-software-drivers
> > +F: drivers/leds/leds-ltc3208.c
> > +
> > LTC4282 HARDWARE MONITOR DRIVER
> > M: Nuno Sa <nuno.sa@xxxxxxxxxx>
> > L: linux-hwmon@xxxxxxxxxxxxxxx
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index
> > 597d7a79c988..867b120ea8ba 100644
> > --- a/drivers/leds/Kconfig
> > +++ b/drivers/leds/Kconfig
> > @@ -1029,6 +1029,17 @@ 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 supports the LED device LTC3208.
>
> You can do better!
>
> > + 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..65e65cd73d73
> > --- /dev/null
> > +++ b/drivers/leds/leds-ltc3208.c
> > @@ -0,0 +1,298 @@
> > +// 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/gpio/consumer.h> #include <linux/i2c.h> #include
> > +<linux/init.h> #include <linux/leds.h> #include
> > +<linux/mod_devicetable.h> #include <linux/module.h> #include
> > +<linux/platform_device.h> #include <linux/property.h> #include
> > +<linux/regmap.h> #include <linux/types.h> #include
> > +<linux/workqueue.h>
>
> Are all of these headers strictly necessary? For instance, it doesn't appear we
> are using GPIOs, platform devices, or workqueues in this driver.
>
> > +#define LTC3208_SET_HIGH_BYTE_DATA(x) FIELD_PREP(GENMASK(7, 4),
> (x))
Noted, I'll review the headers and remove unnecessary ones.
> > +
> > +/* 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 0x5 /* AUX DAC Select */
> > +#define LTC3208_REG_F_CAM 0x6 /* CAM (High half-byte and Low
> half-byte) current DAC*/
> > +#define LTC3208_REG_G_OPT 0x7 /* Device Options */
> > +
> > +/* Device Options register */
> > +#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)
>
> Nit: This can look nicer nested:
>
> #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 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)
>
Will follow this suggestion, thanks!
> > +#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
>
> Nit: Can we have _all_ of the values line up nicely?
>
> #define LTC3208_NUM_AUX_OPT 4
> #define LTC3208_MAX_CPO_OPT 3
>
Noted, will format this.
> > +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;
> > + struct regmap *map;
> > + struct ltc3208_led *leds;
> > +};
> > +
> > +static const struct regmap_config ltc3208_regmap_cfg = {
> > + .reg_bits = 8,
> > + .val_bits = 8,
> > +};
> > +
> > +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);
>
> You can use 100-chars to avoid this awkwardness.
>
Noted on this formatting.
> > + struct i2c_client *client = led->client;
> > + struct ltc3208_dev *dev = i2c_get_clientdata(client);
> > + struct regmap *map = dev->map;
> > + u8 current_level = brightness;
> > +
> > + /*
> > + * For registers with 4-bit splits (CAM, AUX/BLUE, GREEN/RED), the
> other
> > + * half of the byte will be retrieved from the stored DAC value before
> > + * updating the register.
> > + */
> > + switch (led->channel) {
> > + case LTC3208_CHAN_MAIN:
> > + return regmap_write(map, LTC3208_REG_C_MAIN,
> current_level);
> > + case LTC3208_CHAN_SUB:
> > + return regmap_write(map, LTC3208_REG_D_SUB,
> current_level);
> > + case LTC3208_CHAN_AUX:
> > + /* combine both low and high halves of byte */
> > + current_level =
> LTC3208_SET_HIGH_BYTE_DATA(current_level);
> > + current_level |= dev-
> >leds[LTC3208_CHAN_BLUE].cdev.brightness;
> > + return regmap_write(map, LTC3208_REG_B_AUXBLU,
> current_level);
>
> Should we be using 'regmap_update_bits()' or 'regmap_field' here instead?
> Constructing the register value by reading the software state of another LED
> instance could lead to races.
>
> > + case LTC3208_CHAN_BLUE:
> > + /* apply high bits stored in other led */
> > + current_level |= LTC3208_SET_HIGH_BYTE_DATA(dev-
> >leds[LTC3208_CHAN_AUX].cdev.brightness);
> > + return regmap_write(map, LTC3208_REG_B_AUXBLU,
> current_level);
> > + case LTC3208_CHAN_CAMH:
> > + current_level =
> LTC3208_SET_HIGH_BYTE_DATA(current_level);
> > + current_level |= dev-
> >leds[LTC3208_CHAN_CAML].cdev.brightness;
> > + return regmap_write(map, LTC3208_REG_F_CAM,
> current_level);
> > + case LTC3208_CHAN_CAML:
> > + current_level |= LTC3208_SET_HIGH_BYTE_DATA(dev-
> >leds[LTC3208_CHAN_CAMH].cdev.brightness);
> > + return regmap_write(map, LTC3208_REG_F_CAM,
> current_level);
> > + case LTC3208_CHAN_GREEN:
> > + current_level =
> LTC3208_SET_HIGH_BYTE_DATA(current_level);
> > + current_level |= dev-
> >leds[LTC3208_CHAN_RED].cdev.brightness;
> > + return regmap_write(map, LTC3208_REG_A_GRNRED,
> current_level);
> > + case LTC3208_CHAN_RED:
> > + current_level |= LTC3208_SET_HIGH_BYTE_DATA(dev-
> >leds[LTC3208_CHAN_GREEN].cdev.brightness);
> > + return regmap_write(map, LTC3208_REG_A_GRNRED,
> current_level);
>
> This lot is begging for a sub function:
>
> static int ltc3208_led_set_current_level(struct regmap *regmap, u8 reg, u8
> low, u8 high) { {
> return regmap_write(regmap, reg, SET_HIGH_BYTE(high) | low); }
>
Noted on this. Thank you for the suggestion!
> > + 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) {
> > + struct regmap *map = dev->map;
> > + u8 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);
> > +
>
> That tabbing is awkward. In these cases it's better to do the allocation after
> the declaration.
>
Noted on this.
> > + return regmap_write(map, LTC3208_REG_G_OPT, val); }
> > +
> > +static int ltc3208_update_aux_dac(struct ltc3208_dev *dev,
> > + enum ltc3208_aux_channel aux_1, enum ltc3208_aux_channel
> aux_2,
> > + enum ltc3208_aux_channel aux_3, enum ltc3208_aux_channel
> aux_4)
>
> These should sit under the '('.
>
> > +{
> > + struct regmap *map = dev->map;
> > + u8 val = FIELD_PREP(LTC3208_AUX1_MASK, aux_1) |
> > + FIELD_PREP(LTC3208_AUX2_MASK, aux_2) |
> > + FIELD_PREP(LTC3208_AUX3_MASK, aux_3) |
> > + FIELD_PREP(LTC3208_AUX4_MASK, aux_4);
>
> As above.
>
> > + return regmap_write(map, LTC3208_REG_E_AUX, val); }
> > +
> > +static int ltc3208_probe(struct i2c_client *client) {
> > + enum ltc3208_aux_channel
> aux_channels[LTC3208_NUM_AUX_LEDS];
> > + struct ltc3208_dev *data;
>
> 'data' is a terrible variable name.
>
> > + struct ltc3208_led *leds;
> > + struct regmap *map;
>
> 'regmap'
>
Will replace these variable names accordingly.
> > + int ret, i;
> > + u32 val;
> > + bool dropdis_rgb_aux4;
> > + bool dis_camhl;
> > + bool en_rgbs;
> > +
> > + map = devm_regmap_init_i2c(client, <c3208_regmap_cfg);
> > + if (IS_ERR(map))
> > + return dev_err_probe(&client->dev, PTR_ERR(map),
> > + "Failed to initialize regmap\n");
> > +
> > + data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
> > + if (!data)
> > + return -ENOMEM;
> > +
> > + leds = devm_kcalloc(&client->dev, LTC3208_NUM_LED_GRPS,
> > + sizeof(struct ltc3208_led), GFP_KERNEL);
> > + if (!leds)
> > + return -ENOMEM;
> > +
> > + data->client = client;
> > + data->map = map;
> > +
> > + /* initialize options from devicetree */
>
> Capitalise comments and it's "Device Tree", although honestly, I think the
> whole comment is superfluous.
>
I'll omit this comment in that case.
> > + 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");
>
> Use 100-chars.
>
> > + ret = ltc3208_update_options(data, en_rgbs, dis_camhl,
> > + dropdis_rgb_aux4);
> > + if (ret)
> > + return dev_err_probe(&client->dev, ret,
> > + "error writing to options register\n");
>
> Capitalise.
>
> > + /* initialize aux channel configurations from devicetree */
>
> As above and throughout.
>
Noted.
> > + for (i = 0; i < LTC3208_NUM_AUX_LEDS; i++) {
>
> for (int i = 0; ...
>
Will update to this.
> > + 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 */
> > + 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.\n");
> > + }
> > +
> > + ret = ltc3208_update_aux_dac(data, aux_channels[0],
> aux_channels[1],
> > + aux_channels[2], aux_channels[3]);
>
> Why not just aux_channels and pull the values out in the function.
>
Noted. I'll update the function to use an array pointer instead.
> > + if (ret)
> > + return dev_err_probe(&client->dev, ret,
> > + "error writing to aux %u channel
> register.\n", i);
>
> When is 'i' not 'LTC3208_NUM_AUX_LEDS'?
>
My apologies for this, it seems like I misplaced this error message from a previous iteration.
I'll remove the reference to `i` as all 4 channels are written at the same time. In the above function.
> > + i2c_set_clientdata(client, 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 || val >= LTC3208_NUM_LED_GRPS)
> > + return dev_err_probe(&client->dev, -EINVAL,
> > + "Invalid reg property for LED\n");
>
> Why aren't we propagating the real error?
>
I'll add the error code into the error strings.
> > +
> > + 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;
> > + 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);
> > + if (ret)
> > + return dev_err_probe(&client->dev, ret,
> > + "Failed to register LED %u\n", val);
> > + }
> > +
> > + data->leds = leds;
> > +
> > + 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[] = {
> > + { "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");
> >
> > --
> > 2.43.0
> >
>
> --
> Lee Jones [李琼斯]
I will submit another patch with your suggestions soon.
Regards,
Carlo