Add Support for LTC3208 #3022

jcroleda · 2025-11-20T03:56:45Z

PR Description

This PR adds driver support for LTC3208 Multi-Display Driver
Datasheet: LTC3208
Tested on Raspberry Pi 4B using the following evaluation board: DC824A

PR Type

Bug fix (a change that fixes an issue)
New feature (a change that adds new functionality)
Breaking change (a change that affects other repos or cause CIs to fail)

PR Checklist

I have conducted a self-review of my own code changes
I have compiled my changes, including the documentation
I have tested the changes on the relevant hardware
I have updated the documentation outside this repo accordingly
I have provided links for the relevant upstream lore

edelweiseescala

Hello, just sharing what I've learned so far

drivers/leds/leds-ltc3208.c

Documentation/ABI/testing/sysfs-class-led-driver-ltc3208

Documentation/devicetree/bindings/leds/leds-ltc3208.yaml

drivers/leds/leds-ltc3208.c

jcroleda · 2025-11-24T05:20:21Z

Changelog V2:

removed ltc3208_get_brightness
fixed indentations
replaced device attributes for AUX configurations in favor of devicetree properties
cleaned define macros (removed ltc3208_low_byte_data and ltc3208_byte, used field_prep for ltc3208_high_byte_data)
updated GPL license on leds-ltc3208.c
updated commit message for devicetree bindings

drivers/leds/leds-ltc3208.c

jcroleda · 2025-12-01T06:40:59Z

Changelog V3:

replaced device_property_match_string with device_property_read_string
updated LTC3208_NUM_AUX_LEDS to 4

Documentation/devicetree/bindings/leds/leds-ltc3208.yaml

drivers/leds/leds-ltc3208.c

nunojsa · 2025-12-01T10:02:07Z

drivers/leds/leds-ltc3208.c

+	if (ret) {
+		dev_err(&client->dev, "Error Writing brightness to register %u\n", reg);
+		return ret;
+	}


Again, I would just return regmap()

This is exposed to user space by brightness set and is not used in the probe function, if so should I still remove this dev_err?

Add Documentation for LTC3208 Multidisplay LED Driver. Signed-off-by: Jan Carlo Roleda <jancarlo.roleda@analog.com>

jcroleda · 2025-12-02T23:11:30Z

Changelog V4:
leds-ltc3208.yaml

Removed common.yaml ref from led patternproperty
updated descriptions for leds and AUX channel configurations for clarity

leds-ltc3208.c

fixed formatting for multiline function calls
Adjusted defines (removed name, updated option count/limits)
fixed error message locations to focus within probe()
used FIELD_PREP() in update options for consistency
replaced device_property_read_string with device_property_match_property_string to shorten aux pin configuration code in probe

Kernel driver implementation for LTC3208 Multidisplay LED Driver Signed-off-by: Jan Carlo Roleda <jancarlo.roleda@analog.com>

Add entry to Kconfig for LTC3208 driver Signed-off-by: Jan Carlo Roleda <jancarlo.roleda@analog.com>

jcroleda · 2025-12-02T23:49:51Z

Changelog V4.1:

Adjusted tab spacing

machschmitt

Hello @jcroleda,

Some additional comments. Take these with a grain of salt (specially the one about led_trigger) as I'm not experienced with the LEDs subsystem.

Besides those, I suggest to drop the "LTC3208:" part of the
"dt-bindings: leds: LTC3208: Document LTC3208 Multidisplay LED Driver"
commit title. LTC3208 has not been added to the kernel yet.

machschmitt · 2025-12-05T14:07:57Z

Documentation/devicetree/bindings/leds/leds-ltc3208.yaml

+  adi,select-rgb-sub-en:
+    type: boolean
+    description:
+      Selects whether the ENRGBS pin controls the SUB channel's output pins if set,
+      or RGB channel's output pins if unset.


I don't think this accurately describes ENRGBS pin behavior based on the details provided on data sheet page 12.
IIUC, ENRGBS works like a toggle button for the RGB leds (if register REGG bit G1 is set to 0) or like a toggle button for the SUB display (if register REGG bit G1 is set to 1). My feeling is that we should have a GPIO to control ENRGBS and/or, maybe, a GPIO hog.

this is the configuration setup for REGG G1, not the control itself of ENRGBS. I'll change the name and description for clarity.

machschmitt · 2025-12-05T16:29:44Z

drivers/leds/leds-ltc3208.c

+struct ltc3208_chip_data {
+	struct ltc3208_led *leds;
+};


Do we really need this chip_data struct? I think the code would become simpler and easier to read if the array of leds became part of the ltc3208_dev struct.

machschmitt · 2025-12-05T16:56:35Z

drivers/leds/leds-ltc3208.c

+		current_level |= LTC3208_SET_HIGH_BYTE_DATA(
+			dev->chip_data.leds[LTC3208_CHAN_AUX].cdev.brightness);


Hmm, can the brightness (from enum led_brightness) be LED_FULL == 255? In that case, the current_level would not get updated because 0xFF ORed with anything would continue to be 0xFF. Is this missing

current_level &= ~GENMASK(7, 4);

?
or, alternatively

FIELD_MODIFY(GENMASK(7, 4), &current_level, dev->leds[LTC3208_CHAN_AUX].cdev.brightness);

The LED subsystem performs clamping internally to the input brightness before being passed into this set_brightness function, so it should not overlap with the existing brightness of the other channel if the target channel's max brightness is only 4-bit (0xF)

machschmitt · 2025-12-05T16:59:16Z

drivers/leds/leds-ltc3208.c

+	ret = regmap_write(map, reg, current_level);
+	if (ret) {
+		dev_err(&client->dev,
+			"Error Writing brightness to register %u\n", reg);
+		return ret;


An alternative way of having these is to return on each case. E.g.

case LTC3208_CHAN_RED: current_level |= LTC3208_SET_HIGH_BYTE_DATA( dev->chip_data.leds[LTC3208_CHAN_GREEN].cdev.brightness); return regmap_write(map, LTC3208_REG_A_GNRD, current_level);

maybe that was what @nunojsa wanted to suggest?

machschmitt · 2025-12-05T17:02:16Z

drivers/leds/leds-ltc3208.c

+	val |= FIELD_PREP(LTC3208_OPT_EN_RGBS, is_sub);
+	val |= FIELD_PREP(LTC3208_OPT_DIS_CAMHILO, is_cam_hi);
+	val |= FIELD_PREP(LTC3208_OPT_DIS_RGBDROP, is_rgb_drop);
+	val |= FIELD_PREP(LTC3208_OPT_CPO_MASK, cpo_mode);


Are these also potentially missing val &= ~MASK?

i believe my initialization u8 val = 0; serves the same purpose here

machschmitt · 2025-12-05T17:04:31Z

drivers/leds/leds-ltc3208.c

+		return dev_err_probe(&client->dev, -ENOMEM,
+				"No memory for device data\n");


return -ENOMEM is what is usually done when memory allocation fails.

machschmitt · 2025-12-05T17:05:17Z

drivers/leds/leds-ltc3208.c

+	leds = devm_kcalloc(&client->dev, LTC3208_NUM_LED_GRPS,
+			sizeof(struct ltc3208_led), GFP_KERNEL);


not checking potential errors? I mean,

if (!leds) return -ENOMEM;

?

machschmitt · 2025-12-05T17:08:41Z

drivers/leds/leds-ltc3208.c

+		current_level |= LTC3208_SET_HIGH_BYTE_DATA(
+			dev->chip_data.leds[LTC3208_CHAN_AUX].cdev.brightness);


Hmm, can the brightness (from enum led_brightness) be LED_FULL == 255? In that case, the current_level would not get updated because 0xFF ored with anything would continue to be 0xFF. Is this missing

current_level &= ~GENMASK(7, 4);

?
or, alternatively

FIELD_MODIFY(GENMASK(7, 4), &current_level, dev->leds[LTC3208_CHAN_AUX].cdev.brightness);

machschmitt · 2025-12-05T17:59:20Z

drivers/leds/leds-ltc3208.c

+	ret = ltc3208_update_options(data, en_rgbs, dis_camhl,
+		dropdis_rgb_aux4, cpo_mode);


I'm not familiar with the LED subsystem. Though, I wonder if we can have the ENRGBS pin functionality better supported as a led_trigger. Main idea is, we add/register a struct led_trigger, setting its name, activate/deactivate callbacks, and any other required fields. With the proper configuration, we should be able to get /sys/class/leds/<led>/shot provided to user space (see Documentation/ABI/testing/sysfs-class-led-trigger-oneshot and Documentation/ABI/testing/sysfs-class-led). There could be one trigger for RGB toggle and one for SUB toggle. The trigger activate/deactive handlers would write the proper REGG bit G1, then pulse the connected GPIO (see the comment about the dt property).

Not sure how much of the above actually makes sense so you might look for somebody more experienced with the LED subsystem if things don't fit well.

Or, if this gets too complicated or time consuming, it might be reasonable to assume ENRGBS is always high and not mess around with that on the initial support for LTC3208.

I think having seperate oneshot triggers for each channel might introduce specific conflicts on the use of the pin (ex. Sub trigger -> RGB trigger before oneshot period is over, will override the Sub trigger as it will switch control of ENRGBS).

However, we could add a general GPIO pin support to provide sysfs access of the pin, and possibly support the same for the CAMHL pin as it serves a similar toggle function.

machschmitt · 2025-12-05T18:14:07Z

Documentation/devicetree/bindings/leds/leds-ltc3208.yaml

+  adi,force-cpo-level:
+    $ref: /schemas/types.yaml#/definitions/string
+    description: Forces the Charge Pump Output to a specified multiplier
+    enum:
+      - "1" # none; device CPO multiplier acts on dropout signals
+      - "1.5"
+      - "2"
+    default:
+      - "1"


Have a look at 1 and check out if the charge pump properties can have the same name.

jcroleda requested review from CiprianRegus, amiclaus, btogorean, buha, chegbeli, danmois, dbogdan, machschmitt, mhennerich, nunojsa and rbolboac as code owners November 20, 2025 03:56

jcroleda force-pushed the staging/ltc3208 branch 3 times, most recently from 47e5181 to d2fada2 Compare November 20, 2025 10:25

edelweiseescala reviewed Nov 21, 2025

View reviewed changes

jcroleda force-pushed the staging/ltc3208 branch from d2fada2 to 57ca070 Compare November 24, 2025 05:19

jcroleda force-pushed the staging/ltc3208 branch from 57ca070 to 999b8f4 Compare November 25, 2025 00:36

UtsavAgarwalADI reviewed Nov 28, 2025

View reviewed changes

drivers/leds/leds-ltc3208.c Outdated Show resolved Hide resolved

UtsavAgarwalADI reviewed Nov 28, 2025

View reviewed changes

drivers/leds/leds-ltc3208.c Outdated Show resolved Hide resolved

jcroleda force-pushed the staging/ltc3208 branch from 999b8f4 to 20cc126 Compare December 1, 2025 06:38

nunojsa reviewed Dec 1, 2025

View reviewed changes

dt-bindings: leds: LTC3208: Document LTC3208 Multidisplay LED Driver

b087967

Add Documentation for LTC3208 Multidisplay LED Driver. Signed-off-by: Jan Carlo Roleda <jancarlo.roleda@analog.com>

jcroleda force-pushed the staging/ltc3208 branch from 20cc126 to e3af1f6 Compare December 2, 2025 23:09

jcroleda force-pushed the staging/ltc3208 branch from e3af1f6 to 5d8624c Compare December 2, 2025 23:18

jcroleda added 2 commits December 3, 2025 07:46

leds: ltc3208: add driver

475670c

Kernel driver implementation for LTC3208 Multidisplay LED Driver Signed-off-by: Jan Carlo Roleda <jancarlo.roleda@analog.com>

leds: Kconfig.adi: imply LTC3208

6e8165c

Add entry to Kconfig for LTC3208 driver Signed-off-by: Jan Carlo Roleda <jancarlo.roleda@analog.com>

jcroleda force-pushed the staging/ltc3208 branch from 5d8624c to 6e8165c Compare December 2, 2025 23:48

machschmitt reviewed Dec 5, 2025

View reviewed changes

		current_level \|= LTC3208_SET_HIGH_BYTE_DATA(
		dev->chip_data.leds[LTC3208_CHAN_AUX].cdev.brightness);

		return dev_err_probe(&client->dev, -ENOMEM,
		"No memory for device data\n");

		leds = devm_kcalloc(&client->dev, LTC3208_NUM_LED_GRPS,
		sizeof(struct ltc3208_led), GFP_KERNEL);

		ret = ltc3208_update_options(data, en_rgbs, dis_camhl,
		dropdis_rgb_aux4, cpo_mode);

Add Support for LTC3208 #3022

Are you sure you want to change the base?

Add Support for LTC3208 #3022

Conversation

jcroleda commented Nov 20, 2025

PR Description

PR Type

PR Checklist

Uh oh!

edelweiseescala left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jcroleda commented Nov 24, 2025

Uh oh!

Uh oh!

Uh oh!

jcroleda commented Dec 1, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcroleda commented Dec 2, 2025

Uh oh!

jcroleda commented Dec 2, 2025

Uh oh!

machschmitt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcroleda Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

jcroleda Dec 9, 2025 •

edited

Loading