Add LTC3220 driver #3005

edelweiseescala · 2025-10-28T08:30:53Z

PR Description

Add LTC3220 driver and documentation.
Datasheet: LTC3220
Tested on RPI4 using the following eval boards: DC1265A-A, DC1265A-B

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

drivers/leds/leds-ltc3220.c

UtsavAgarwalADI · 2025-10-31T13:09:59Z

Could you format your commits so that there are spaces between labels? eg:

arm64:defconfig:xyz
arm64: defconfig: xyz

Additionally, could you add some more context regarding the commits?

You can see more examples here: https://lore.kernel.org/linux-arm-kernel/

drivers/leds/leds-ltc3220.c

vasbimpikasadi

That shows great effort! It's a bit sparse on comments and I've noted some changes which I think have to be fixed before this gets approved.

drivers/leds/leds-ltc3220.c

include/linux/platform_data/leds-ltc3220.h

drivers/leds/leds-ltc3220.c

include/linux/platform_data/leds-ltc3220.h

nunojsa

First round from me. This one will need some iterations before going upstream. I'm also not that familiar with the LED subsystem so I might give it a look at some point to be more helpful.

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

drivers/leds/leds-ltc3220.c

nunojsa · 2025-10-31T15:27:29Z

drivers/leds/leds-ltc3220.c

+MODULE_AUTHOR("Edelweise Escala <edelweise.escala@analog.com>");
+MODULE_DESCRIPTION("LED driver for LTC3220 controllers");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:leds-ltc3220");


Typically not used anymore

include/linux/platform_data/leds-ltc3220.h

edelweiseescala · 2025-11-04T10:48:20Z

V2 Changelog:

dt-bindings: leds: ltc3220: Document LTC3220 18 channel LED Driver
-used adi
-dropped unnecessary descriptions
-dropped | formatting
-fixed led pattern

leds: ltc3220: add driver
-Fix naming which does not follow Linux convention, also fixed naming for defines with onw LTC3220 prefix. Follow kernel style
-Remove unnecessary headers, and fixed header arrangement
-Remove unnecessary functions (ltc3220_write)
-Used FIELD_PREP()
-Use firmware agnostic properties
-Use only GPL2
-Led brightness now uses ltc3220_set_led_data
Instead of 0-63 current level it is now 0-255 where in
0-63 - normal LED mode
64-127 - blink mode
127-191 - gradation mode
191-255 - GPO
this removes the need for another custom ABI for individual led mode.

Added leds: ltc3220: Documentation for Custom ABI
-documentation for the added custom ABI

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

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

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

drivers/leds/leds-ltc3220.c

edelweiseescala · 2025-11-11T03:39:15Z

V3 Changelog:

fixed wrong files on commit

dt-bindings: leds: ltc3220: Document LTC3220 18 channel LED Driver
-fixed example format

leds: ltc3220: add driver
-used fsleep for reset
-fixed log message

leds: ltc3220: Documentation for Custom ABI
-add newline at EOF

machschmitt

Hi @edelweiseescala,

A few comments from my side.
Besides the code changes, I'd suggest to also update your SoB tag with better wording of your name.

machschmitt · 2025-11-11T17:05:21Z

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

+required:
+  - compatible
+  - reg
+  - reset-gpios


It is not common for chips to require a reset. Does the device really fail to work or not function without a hardware reset? It is often nice to do a reset before start managing a device. Though, if it can be operated without a reset, I'd drop the reset-gpios requirement.

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

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

machschmitt · 2025-11-12T12:55:11Z

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

+
+What:		/sys/class/leds/<led>/force_cpo_mode
+Date:		November 2025
+Contact:	Edelweise Escala <edelweise.escala@analog.com>


same here and in the other doc entries.

machschmitt · 2025-11-12T13:39:50Z

drivers/leds/leds-ltc3220.c

+#define LTC3220_ULED18				0x12
+#define LTC3220_GRAD_BLINK			0x13
+
+#define LTC3220_BIT0				BIT(0)


As a reviewer, I don't think this define helped much in understanding the code.
I'd either drop it and use BIT(0) in place, or separate and give more meaningful names. e.g.

#define LTC3220_GRAD_COUNT_UP BIT(0) #define LTC3220_COMMAND_QUICK_WRITE BIT(0)

machschmitt · 2025-11-12T14:33:10Z

drivers/leds/leds-ltc3220.c

+	struct led_classdev led_cdev;
+	struct device *dev;


are these led_cdev and *dev fields not used?

machschmitt · 2025-11-12T14:50:05Z

drivers/leds/leds-ltc3220.c

+	if (ret)
+		return ret;
+
+	device_for_each_child_node(&client->dev, child) {


Use device_for_each_child_node_scoped(), then no need for fwnode_handle_put().

machschmitt · 2025-11-12T14:56:45Z

drivers/leds/leds-ltc3220.c

+		if (ret != 0 || !source || source > LTC3220_NUM_LEDS) {
+			dev_err(&client->dev, "Couldn't read LED address: %d\n",
+				ret);
+			fwnode_handle_put(child);
+			return -ENOMEM;
+		}


This means that all leds are required? Are all LEDs required? If not, I'd dev_war() and continue; instead of returning error.

drivers/leds/leds-ltc3220.c

edelweiseescala · 2025-11-13T08:53:21Z

V4 Changelog:

fixed name on SoB

dt-bindings: leds: ltc3220: Document LTC3220 18 channel LED Driver
-removed reset on rquired
-made reset-gpio property clearer

leds: ltc3220: add driver
-use more meaningful bit name
-removed unused fields
-use device_for_each_child_node_scoped
-add some comments
-dropped i2c client on leds, used i2c leint on ltc3220_state struct

leds: ltc3220: Documentation for Custom ABI
-add kernel version

nunojsa · 2025-11-13T18:31:59Z

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

+		0 - Enables mode logic to control mode changes based on dropout signal
+		1 - Forces charge pump into 1.5x mode
+		2 - Forces charge pump into 2x mode
+		3 - Forces charge pump into 1x mode


Hmm this one seems something HW specific? Is this something that should change at runtime? If I'm right, consider a devicetree attr

nunojsa · 2025-11-13T18:33:13Z

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

+Contact:	Edelweise Escala <edelweise.escala@analog.com>
+Description:	LTC3220 shutdown command. If you write 1 it
+		shuts down part while preserving data in registers.
+		If you write 0 device is in normal operation.


This seems like something you should implement under PM (Power management). Either runtime PM or normal suspend. Pick one :)

nunojsa · 2025-11-13T18:38:57Z

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

+		which allows parallel write on all 18 LED registers, when data is being written on
+		REG1. If you write 1 quick_write is enabled, 0 to disable.
+
+What:		/sys/class/leds/<led>/gradation_speed


I guess this one could be better named gradation_period_ms. Then you would actually pass the period in milliseconds which is a better API for the user. This is possible because we have three distinct periods which match 3 distinct ramp times.

nunojsa · 2025-11-13T18:47:31Z

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

+		2 - Ramp time of 0.48s and Period of 0.625s
+		3 - Ramp time of 0.96s and Period of 1.25s
+
+What:		/sys/class/leds/<led>/gradation_is_increasing


For this one I would suggest gradation_mode and accept the strings "ramp_up" and "ramp_down". From the IIO world you could also add an additional gradation_mode_available which would be on RO and you would give the user the available possibilities. An example for debugfs:

https://elixir.bootlin.com/linux/v6.18-rc5/source/drivers/iio/adc/ad9467.c#L998

Here you need something different likely

nunojsa · 2025-11-13T18:53:38Z

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

+		(0..3) where:
+		0 - On Time of 0.625s and Period of 1.25s
+		1 - On Time of 0.156s and Period of 1.25s
+		2 - On Time of 0.625s and Period of 2.5s


Can we somehow implement this with?

https://elixir.bootlin.com/linux/v6.18-rc5/source/include/linux/leds.h#L154

Make sure to look at other drivers and the subsystem to see what can be used.

nunojsa · 2025-11-13T19:00:46Z

drivers/leds/leds-ltc3220.c

+	ret = ltc3220_set_blink_and_gradation(ltc3220_state,
+										state,
+										ltc3220_state->grad_cfg.gradation_speed,
+										ltc3220_state->grad_cfg.is_increasing);


The above looks very bad

nunojsa · 2025-11-13T19:01:33Z

drivers/leds/leds-ltc3220.c

+	&dev_attr_gradation_is_increasing.attr,
+	&dev_attr_blink_cfg.attr,
+	NULL
+};


Be prepared to justify these attributes... I'm still not sure about it

nunojsa · 2025-11-13T19:01:51Z

drivers/leds/leds-ltc3220.c

+		ltc3220_gradation_is_increasing_store);
+
+static ssize_t ltc3220_blink_cfg_show(struct device *dev,
+								 struct device_attribute *attr, char *buf)


Take care about the above

nunojsa · 2025-11-13T19:01:59Z

drivers/leds/leds-ltc3220.c

+
+	ret = ltc3220_set_blink_and_gradation(ltc3220_state,
+										ltc3220_state->blink_cfg,
+										ltc3220_state->grad_cfg.gradation_speed,


nunojsa · 2025-11-13T19:02:54Z

drivers/leds/leds-ltc3220.c

+	ret = ltc3220_set_blink_and_gradation(ltc3220_state,
+										ltc3220_state->blink_cfg,
+										state,
+										ltc3220_state->grad_cfg.is_increasing);


Please make sure to fix things like the above (unless my github is rendering this in a very odd way)

edelweiseescala · 2025-11-14T10:03:42Z

V5 Changelog:

dt-bindings: leds: ltc3220: Document LTC3220 18 channel LED Driver
-added quick write and force charge pump on device tree

leds: ltc3220: add driver
-styling fixes
-used led blink
-add missing headers
-dropped reset_gpio on struct and added it on reset function instead
-added missing error handling on reset
-modified gradation increasing to accept string ramp up or ramp down
-change gradation_speed to gradation_period_ms which takes period in ms as input

leds: ltc3220: Documentation for Custom ABI
-removed quick write and force charge pump
-renamed gradation speed
-added gradation_mode_available

vasbimpikasadi

Getting really better, loads of great feedback from everyone. This is shaping up quite nicely.

vasbimpikasadi · 2025-11-14T13:58:48Z

drivers/leds/leds-ltc3220.c

+
+	ltc3220_state = devm_kzalloc(&client->dev, sizeof(*ltc3220_state), GFP_KERNEL);
+	if (!ltc3220_state)
+		return -ENODEV;


Memory allocation failure should return -ENOMEM, not -ENODEV

vasbimpikasadi · 2025-11-14T13:59:48Z

drivers/leds/leds-ltc3220.c

+							   &init_data);
+		if (ret) {
+			dev_err(&client->dev, "Fail LED class register\n");
+			return -ENOMEM;


I think ret should be propagated here to aid root causing issues should things go wrong?

vasbimpikasadi · 2025-11-14T14:02:53Z

drivers/leds/leds-ltc3220.c

+	NULL
+};
+
+static struct attribute_group device_cfg_group = {


Probably should be const? It's never modified during runtime

edelweiseescala · 2025-11-17T00:59:41Z

V6 Changelog:

leds: ltc3220: add driver
-fix returned error
-added const on device_cfg_group

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

nunojsa · 2025-11-28T16:48:30Z

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

+
+             led@6 {
+                 reg = <6>;
+             };


It would be nice to have some of the properties coming from common.yaml that the device supports. Otherwise I wonder why we $ref it.

I added function and function-enumerator.

nunojsa · 2025-11-28T16:54:41Z

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

+Contact:	Edelweise Escala <edelweise.escala@analog.com>
+Description:	LTC3220 read only command to show possible gradiation mode such as:
+		"ramp_up"
+		"ramp_down"


I'm still not sure about the above ABI. Does it make sense to have these as runtime? I'm tempted to suggest DT but you might see what the maintainer has to say about it. Maybe there's already a way to handle it through the subsystem.

Removed custom abi, now uses pattern https://elixir.bootlin.com/linux/v6.17.9/source/include/linux/leds.h#L158

nunojsa · 2025-11-28T16:56:39Z

drivers/leds/leds-ltc3220.c

+		}
+	}
+
+	ret = devm_device_add_group(&client->dev, &device_cfg_group);


I would just return devm_device_add_group()

nunojsa · 2025-11-28T16:56:54Z

drivers/leds/leds-ltc3220.c

+							   &init_data);
+		if (ret) {
+			dev_err(&client->dev, "Fail LED class register\n");
+			return ret;


dev_err_probe()

nunojsa · 2025-11-28T17:06:16Z

drivers/leds/leds-ltc3220.c

+	int ret;
+
+	if (!led_cdev->dev || !led_cdev->dev->parent)
+		return -ENODEV;


I would double check the above can really happen. If it can, maybe it's a problem on subsystem itself

nunojsa · 2025-11-28T17:07:54Z

drivers/leds/leds-ltc3220.c

+		return -ENODEV;
+
+	if (!parent_dev || strcmp(parent_dev->bus->name, "i2c") != 0)
+		return -ENODEV;


Again, all of the above conditions look questionable to me

nunojsa · 2025-11-28T17:10:40Z

drivers/leds/leds-ltc3220.c

+
+	client = to_i2c_client(parent_dev);
+	uled_cfg = container_of(led_cdev, struct ltc3220_uled_cfg, led_cdev);
+	ltc3220_state = i2c_get_clientdata(client);


You could also use container_of() to get ltc3220_state from uled_cfg assuming led_index is the position in the uled_cfg array. But still why not making easier and just have a pointer to the main ltc3220_state object from uled_cfg? Then you would just need two lines.

nunojsa · 2025-11-28T17:11:39Z

drivers/leds/leds-ltc3220.c

+		break;
+	}
+
+	return sysfs_emit(buf, "%u\n", period_ms);


Why not returning inline in each case statement? I would save some LOC

nunojsa · 2025-11-28T17:11:48Z

drivers/leds/leds-ltc3220.c

+}
+
+static ssize_t ltc3220_gradation_period_ms_show(struct device *dev,
+								 struct device_attribute *attr, char *buf)


Odd indentation

Add dt-binding for ltc3220. LTC3220 18 Channel LED driver Signed-off-by: Edelweise Escala <edelweise.escala@analog.com>

Add driver for ltc3220. LTC3220 18 Channel LED Driver Signed-off-by: Edelweise Escala <edelweise.escala@analog.com>

Add entry for LTC3220 driver Signed-off-by: Edelweise Escala <edelweise.escala@analog.com>

edelweiseescala · 2025-12-02T01:54:04Z

V7 Changelog

Documentation/devicetree/bindings/leds/leds-ltc3220.yaml
-used Minimum:1 and Maximum:18 rathern than Enum
-added properties from common.yaml

Removed Custom ABI

drivers/leds/leds-ltc3220.c
-used dev_err_probe for errors in probe
-added struct ltc3220_state on uled_cfg
-removed default_label and just rely on device tree for the name
-fixed reset function
-remove gradation custom abi and used pattern_set to handle gradation

edelweiseescala requested review from CiprianRegus, amiclaus, btogorean, buha, chegbeli, danmois, dbogdan, machschmitt, mhennerich, nunojsa and rbolboac as code owners October 28, 2025 08:30

edelweiseescala marked this pull request as draft October 28, 2025 08:38

edelweiseescala force-pushed the main branch 4 times, most recently from 6a5debb to d57b64f Compare October 29, 2025 00:33

edelweiseescala marked this pull request as ready for review October 29, 2025 02:05

artursartamonovsadi reviewed Oct 31, 2025

View reviewed changes

UtsavAgarwalADI reviewed Oct 31, 2025

View reviewed changes

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

vasbimpikasadi requested changes Oct 31, 2025

View reviewed changes

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

include/linux/platform_data/leds-ltc3220.h Outdated Show resolved Hide resolved

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

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

CalebEthridgeADI reviewed Oct 31, 2025

View reviewed changes

include/linux/platform_data/leds-ltc3220.h Outdated Show resolved Hide resolved

nunojsa reviewed Oct 31, 2025

View reviewed changes

edelweiseescala marked this pull request as draft November 4, 2025 06:27

edelweiseescala force-pushed the main branch 2 times, most recently from d01ee06 to b251647 Compare November 4, 2025 08:46

edelweiseescala marked this pull request as ready for review November 4, 2025 10:48

edelweiseescala force-pushed the main branch from b251647 to 03831fa Compare November 4, 2025 23:41

jemfgeronimo reviewed Nov 10, 2025

View reviewed changes

edelweiseescala force-pushed the main branch from 03831fa to 139802f Compare November 11, 2025 03:32

machschmitt reviewed Nov 12, 2025

View reviewed changes

edelweiseescala force-pushed the main branch 3 times, most recently from 80d038e to 03f6cf2 Compare November 13, 2025 08:20

nunojsa reviewed Nov 13, 2025

View reviewed changes

edelweiseescala marked this pull request as draft November 14, 2025 08:26

edelweiseescala force-pushed the main branch from 03f6cf2 to e16e6b1 Compare November 14, 2025 09:40

edelweiseescala marked this pull request as ready for review November 14, 2025 10:03

vasbimpikasadi requested changes Nov 14, 2025

View reviewed changes

edelweiseescala force-pushed the main branch from e16e6b1 to 5f3c45f Compare November 17, 2025 00:01

edelweiseescala requested a review from nunojsa November 21, 2025 07:20

nunojsa reviewed Nov 28, 2025

View reviewed changes

edelweiseescala added 3 commits December 2, 2025 09:15

dt-bindings: leds: ltc3220: Document LTC3220 18 channel LED Driver

1b3303e

Add dt-binding for ltc3220. LTC3220 18 Channel LED driver Signed-off-by: Edelweise Escala <edelweise.escala@analog.com>

leds: ltc3220: add driver

4702802

Add driver for ltc3220. LTC3220 18 Channel LED Driver Signed-off-by: Edelweise Escala <edelweise.escala@analog.com>

leds: Kconfig.adi: imply ltc3220

007f3b3

Add entry for LTC3220 driver Signed-off-by: Edelweise Escala <edelweise.escala@analog.com>

edelweiseescala force-pushed the main branch from 5f3c45f to 007f3b3 Compare December 2, 2025 01:16

Add LTC3220 driver #3005

Are you sure you want to change the base?

Add LTC3220 driver #3005

Conversation

edelweiseescala commented Oct 28, 2025

PR Description

PR Type

PR Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

UtsavAgarwalADI commented Oct 31, 2025

Uh oh!

Uh oh!

vasbimpikasadi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nunojsa 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

edelweiseescala commented Nov 4, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

edelweiseescala commented Nov 11, 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!

Uh oh!

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!

Uh oh!

Uh oh!

edelweiseescala commented Nov 13, 2025

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