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

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

machschmitt

Hello, this is my second round of review.

Besides the comments to the code, I'd also suggest to drop the "ltc3220:" part of
"dt-bindings: leds: ltc3220: Document LTC3220 18 channel LED Driver".
ltc3220 doesn't exist in the kernel, yet.

machschmitt · 2025-12-04T20:45:45Z

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

+  adi,force-cpo-mode:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Force charge pump operation mode.
+      0 = Auto mode (default) - Automatically selects optimal charge pump mode
+      1 = 1.5x mode - 1.5x voltage multiplication
+      2 = 2x mode - 2x voltage multiplication
+      3 = 1x mode - Charge pump bypass
+    minimum: 0
+    maximum: 3
+    default: 0


Is this similar to LTC3208 force-cpo-mode ?
b087967#diff-abf16a96d0bb11e1c9ed3b93a7bf72cbd73355ea1d4c95ae5c5bf4df79fcd49dR46
Looks like it is. My suggestion is to make LTC3220 and LTC3208 have the same documentation for their charge pump properties. Also, I'd suggest making it a string like in LTC3208 PR. Sometimes we might be tempted to use integers to simplify the property description and make the value writable to device registers. Though, frequently, new devices are released with a similar feature and different values and register configurations. Leading to different (but similar in semantics) properties with specific sets of allowed values that don't do any favor to dts writers. A string will allow describing any new charge pump configuration with adi,force-cpo-mode (or adi,force-cpo-level, whatever is more comprehensive) with a reasonable property value.

matched naming with ltc3208 and used string . Thank You!

machschmitt · 2025-12-04T21:17:47Z

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

+  adi,quick-write-enable:
+    type: boolean
+    description:
+      Enable quick write mode. When enabled, writing to LED D1 (register 0x01)
+      will update all 18 LED outputs with the same brightness value.


I can't see why this needs to be in device tree. To me, this sounds more like a device runtime configuration property. What about registering a 19th led? Set led_classdev name to "all". At some point during probe, do something like

ltc3220_state->uled_cfg[<19 or last led index>].led_cdev.name = "all"; ltc3220_state->uled_cfg[i].led_cdev.brightness_set_blocking = ltc3220_set_all_led_data;

with ltc3220_set_all_led_data doing the same thing as ltc3220_set_led_data, but with is_quick_write always true.

added a 19th led in device tree with function as quick-write. Thank You!

machschmitt · 2025-12-04T21:35:50Z

drivers/leds/leds-ltc3220.c

+		return dev_err_probe(&client->dev, -ENOMEM,
+				     "Failed to allocate memory\n");


In the IIO subsystem people usually just return -ENOMEM in these cases. IIRC, trying to print a message would be of no use if the system is already lacking memory.

machschmitt · 2025-12-04T21:38:27Z

drivers/leds/leds-ltc3220.c

+	ret = device_property_read_u8(&client->dev, "adi,force-cpo-mode",
+				      &ltc3220_state->command_cfg.force_cpo_mode);
+	if (ret)
+		ltc3220_state->command_cfg.force_cpo_mode = 0;


If you follow my suggestion and make adi,force-cpo-mode a string, use device_property_match_property_string() to simplify the property parsing.

machschmitt · 2025-12-04T21:42:51Z

drivers/leds/leds-ltc3220.c

+	if (device_property_read_bool(&client->dev, "adi,quick-write-enable"))
+		ltc3220_state->command_cfg.is_quick_write = true;


This might hopefully be dropped (see comment in device tree doc).

machschmitt · 2025-12-04T21:48:43Z

drivers/leds/leds-ltc3220.c

+	u8 reg_val;
+	int ret;
+	struct i2c_client *client = ltc3220_state->client;


I personally like arranging local variables in reverse xmas tree. Don't know how LED subsystem maintainer might prefer those, though.

machschmitt · 2025-12-04T21:52:29Z

drivers/leds/leds-ltc3220.c

+#define LTC3220_ULED01				0x01
+#define LTC3220_ULED02				0x02
+#define LTC3220_ULED03				0x03
+#define LTC3220_ULED04				0x04
+#define LTC3220_ULED05				0x05
+#define LTC3220_ULED06				0x06
+#define LTC3220_ULED07				0x07
+#define LTC3220_ULED08				0x08
+#define LTC3220_ULED09				0x09
+#define LTC3220_ULED10				0x0A
+#define LTC3220_ULED11				0x0B
+#define LTC3220_ULED12				0x0C
+#define LTC3220_ULED13				0x0D
+#define LTC3220_ULED14				0x0E
+#define LTC3220_ULED15				0x0F
+#define LTC3220_ULED16				0x10
+#define LTC3220_ULED17				0x11
+#define LTC3220_ULED18				0x12


I'd suggest
#define LTC3220_ULED(x) (x)
but these seem to have not been used?
If these are really not used, then I suggest to drop.

edelweiseescala · 2025-12-09T02:23:16Z

V8 Changelog

Documentation/devicetree/bindings/leds/leds-ltc3220.yaml
-renamed adi,force-cpo-mode to adi,force-cpo-level to match ltc3208, also used string instead of uint32
-dropped quick write but added a virtual led for quick write

drivers/leds/leds-ltc3220.c
-modified quick write to check if led is in quick write mode
-returned -ENOMEM directly
-changed checking of adi,force-cpo-level to match string
-fixed variable arrangement to reverse xmas tree
-#define LTC3220_ULED(x)

machschmitt

Hi @edelweiseescala,

Driver looks good overall. A few comments left from my side. I'd suggest to wait one week or so (to give time for fellow ADI devs to review this), then send it to the mailing list.

machschmitt · 2025-12-10T12:02:10Z

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

+             led@19 {
+                 reg = <1>;
+                 function = "quick-write";
+             };


Makes sense I guess. Having virtual hardware described in dt looks a bit unusual, but I'll leave the final word about it for dt maintainers. Also note address 1 (reg = <1>) was already being used by led@1.

machschmitt · 2025-12-10T12:29:51Z

drivers/leds/leds-ltc3220.c

+static int ltc3220_set_command(struct ltc3220_state *ltc3220_state,
+			       bool is_shutdown, bool is_quick_write)
+{
+	struct i2c_client *client = ltc3220_state->client;
+	u8 reg_val;
+	int ret;
+
+	reg_val = FIELD_PREP(LTC3220_COMMAND_SHUTDOWN, is_shutdown);
+	reg_val |= FIELD_PREP(LTC3220_CPO_COMMAND_MASK,
+			      ltc3220_state->command_cfg.force_cpo_level);
+	reg_val |= FIELD_PREP(LTC3220_COMMAND_QUICK_WRITE, is_quick_write);
+	ret = i2c_smbus_write_byte_data(client, LTC3220_COMMAND, reg_val);
+	if (ret < 0) {
+		dev_err(&client->dev, "Set command fail\n");
+		return ret;
+	}
+
+	ltc3220_state->command_cfg.is_shutdown = is_shutdown;
+
+	return 0;
+}


Boolean parameters can often be avoided when smaller tasks can be extracted into helper functions.
There's only one place where ltc3220_set_command is called with is_quick_write == true.
I suggest to extract the i2c_smbus_write_byte_data() part into a helper function (e.g. ltc3220_i2c_write(i2c_client, reg_addr, reg_val), name up to you). Then ltc3220_set_command calls ltc3220_i2c_write without setting quick write while ltc3220_set_quick_command (new function) call ltc3220_i2c_write with quick_write set. I think something similar can be done for the is_shutdown thing too. With those, ltc3220_set_command, ltc3220_set_quick_command, ltc3220_shutdown could be called according to each specific situation.

machschmitt · 2025-12-10T12:49:59Z

drivers/leds/leds-ltc3220.c

+	ret = device_property_read_string(&client->dev, "adi,force-cpo-level", &cpo_level_str);
+	if (ret) {
+		ltc3220_state->command_cfg.force_cpo_level = 0;
+	} else {
+		ret = match_string(ltc3220_cpo_levels, ARRAY_SIZE(ltc3220_cpo_levels), cpo_level_str);
+		if (ret < 0)
+			ltc3220_state->command_cfg.force_cpo_level = 0;
+		else
+			ltc3220_state->command_cfg.force_cpo_level = ret;
+	}


I'd use device_property_match_property_string() (https://elixir.bootlin.com/linux/v6.17.8/source/include/linux/property.h#L132) to simplify property parsing (see ad4000 1, ad7944 2 for examples ). Maybe this is trickier than I think? (don't recall the LTC3220 register details).

machschmitt · 2025-12-10T12:55:25Z

drivers/leds/leds-ltc3220.c

+		if (is_quick_write_led) {
+			led = &ltc3220_state->quick_write_led;
+			led->led_index = 0;
+			led->led_mode = QUICK_WRITE;


I'm a bit concerned about how the "quick_write" led will appear in user space? I mean, is it going to be clear for the user that this led will write to all LTC3220 connected leads? If so, then LGTM.

No because I found no documentation about it in the bindings 😄

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-11T07:44:43Z

V9 Change Log

used device_property_match_property_string
add helper function for quickwrite and shutdown

nunojsa

Some more comments from me. But we're getting ready for upstream AFAICT

nunojsa · 2025-12-11T09:32:42Z

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

+
+             led@19 {
+                 reg = <19>;
+                 function = "quick-write";


Hmm is this something defined in schemas/leds/common.yaml?

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

nunojsa · 2025-12-11T09:36:14Z

drivers/leds/leds-ltc3220.c

+		if (!fwnode_property_read_string(child, "function", &function)) {
+			if (!strcmp(function, "quick-write"))
+				is_quick_write_led = true;
+		}


I think the above is a custom property. Isn't quick write affecting all LEDs? If so this should be a device property and it should have a proper vendor prefix. So, a boolean like "adi,quick-write-enable".

Having said the above I'm still not sure where this feature fits the best... Maybe something worth asking upstream when submitting

nunojsa · 2025-12-11T09:39:49Z

drivers/leds/leds-ltc3220.c

+		ret = fwnode_property_read_u32(child, "reg", &source);
+		if (ret != 0 || !source || source > (LTC3220_NUM_LEDS + 1))
+			return dev_err_probe(&client->dev, ret ? ret : -EINVAL,
+					     "Couldn't read LED address\n");


The above is a bit weird and hard to read. Simplify it:

ret = fwnode_property_read_u32(...); if (ret) return dev_err_probe(dev, ret, ...); if (!source || source > ...) return dev_err_probe()

nunojsa · 2025-12-11T09:41:13Z

drivers/leds/leds-ltc3220.c

+		if (is_quick_write_led) {
+			led = &ltc3220_state->quick_write_led;
+			led->led_index = 0;
+			led->led_mode = QUICK_WRITE;


No because I found no documentation about it in the bindings 😄

nunojsa · 2025-12-11T09:48:39Z

drivers/leds/leds-ltc3220.c

+	struct gpio_desc *reset_gpio = data;
+
+	if (reset_gpio)
+		gpiod_set_value_cansleep(reset_gpio, 1);


As said, not sure if we need the above but as a side note for the future:

Note that the action is already only added when the reset gpio is available. So the above condition is redundant

nunojsa · 2025-12-11T09:53:11Z

drivers/leds/leds-ltc3220.c

+	ret = ltc3220_set_blink_and_gradation(ltc3220_state,
+					       blink_mode,
+					       ltc3220_state->grad_cfg.gradation_period_ms,
+					       ltc3220_state->grad_cfg.is_increasing);


Not sure if it's github rendering but the above does not look to be properly aligned to the open parenthesis.

nunojsa · 2025-12-11T09:54:04Z

drivers/leds/leds-ltc3220.c

+	ltc3220_state = uled_cfg->ltc3220_state;
+
+	if (len != 2)
+		return -EINVAL;


I would do this before getting uled_cfg.

nunojsa · 2025-12-11T09:55:01Z

drivers/leds/leds-ltc3220.c

+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to set blink/gradation\n");
+		return ret;
+	}


Personalty I would simplify and just do return i2c_smbus_write_byte_data()

nunojsa · 2025-12-11T09:55:48Z

drivers/leds/leds-ltc3220.c

+	uled_cfg = container_of(led_cdev, struct ltc3220_uled_cfg, led_cdev);
+	ltc3220_state = uled_cfg->ltc3220_state;
+
+	if (uled_cfg->led_mode == QUICK_WRITE) {


Can't we change mode once on QUICK_WRITE. If not, do it probe once and be done with it :). IOW, why do we need this dance between ltc3220_enable_quick_write() and restore_command? Might be worth a comment about it

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 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 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 force-pushed the main branch from 5f3c45f to 007f3b3 Compare December 2, 2025 01:16

machschmitt reviewed Dec 4, 2025

View reviewed changes

edelweiseescala force-pushed the main branch from 007f3b3 to 91fc283 Compare December 9, 2025 02:16

edelweiseescala force-pushed the main branch from 91fc283 to 6cae08d Compare December 9, 2025 03:29

machschmitt reviewed Dec 10, 2025

View reviewed changes

edelweiseescala force-pushed the main branch 2 times, most recently from 57787f5 to c4f6a95 Compare December 11, 2025 06:19

edelweiseescala added 2 commits December 11, 2025 14:21

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

cfe0eae

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

leds: ltc3220: add driver

698c928

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

edelweiseescala force-pushed the main branch from c4f6a95 to 300225a Compare December 11, 2025 06:50

leds: Kconfig.adi: imply ltc3220

4265a69

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

edelweiseescala force-pushed the main branch from 300225a to 4265a69 Compare December 11, 2025 07:36

nunojsa reviewed Dec 11, 2025

View reviewed changes

		return dev_err_probe(&client->dev, -ENOMEM,
		"Failed to allocate memory\n");

		if (device_property_read_bool(&client->dev, "adi,quick-write-enable"))
		ltc3220_state->command_cfg.is_quick_write = true;

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

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!

edelweiseescala commented Nov 14, 2025

Uh oh!

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

edelweiseescala commented Nov 17, 2025

edelweiseescala Dec 9, 2025 •

edited

Loading