-
Notifications
You must be signed in to change notification settings - Fork 910
Add LTC3220 driver #3005
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add LTC3220 driver #3005
Conversation
6a5debb to
d57b64f
Compare
drivers/leds/leds-ltc3220.c
Outdated
| } | ||
|
|
||
| static ssize_t ltc3220_shutdown_store(struct device *dev, | ||
| struct device_attribute *devAttr, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
devAttr is in camel case can you fix to match Linux Kernel style
drivers/leds/leds-ltc3220.c
Outdated
| } | ||
|
|
||
| static ssize_t ltc3220_force_cpo_2x_store(struct device *dev, | ||
| struct device_attribute *devAttr, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
drivers/leds/leds-ltc3220.c
Outdated
| } | ||
|
|
||
| static ssize_t ltc3220_force_cpo_1p5x_store(struct device *dev, | ||
| struct device_attribute *devAttr, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
drivers/leds/leds-ltc3220.c
Outdated
| } | ||
|
|
||
| static ssize_t ltc3220_quick_write_store(struct device *dev, | ||
| struct device_attribute *devAttr, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same above
drivers/leds/leds-ltc3220.c
Outdated
| } | ||
|
|
||
| static int ltc3220_set_command(struct ltc3220_platform_data *platform_data, | ||
| bool is_shutdown, bool force2x, bool force1p5, bool quickWrite) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quickWrite is in camel case can you fix it according to Linux Kernel styling guide?
|
Could you format your commits so that there are spaces between labels? eg:
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
Outdated
| #define LTC3220_ULED18 0x12 | ||
| #define LTC3220_GRAD_BLINK 0x13 | ||
|
|
||
| static int ltc3220_write(struct i2c_client *client, u8 reg, u8 val) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need another function call for this? I dont see any extra handling being done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I don't see any real benefit in these wrappers. I would also see if regmap is a possibility
vasbimpikasadi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
| { | ||
| int ret; | ||
| struct i2c_client *client = uled->client; | ||
| u8 reg_val = (mode << 6) | current_level; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could overflow. Masking can prevent that. e.g. u8 reg_val = ((mode & 0x03) << 6) | (current_level & 0x3F);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, as I said in another comment I would just consider FIELD_PREP()
|
|
||
| #define NUM_LEDS 18 | ||
|
|
||
| #define MAX_CURRENT 64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about this one, but isn't it a 6-bit field? If so, it should be 0-63 so, #define MAX_CURRENT 63
drivers/leds/leds-ltc3220.c
Outdated
| struct ltc3220_uled_cfg *uled_cfg; | ||
|
|
||
| current_level = (int)brightness; | ||
| if (current_level > MAX_CURRENT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You know the code way better than I do, but shouldn't that be an error? If it's fine to continue with current capped at max, at least a dev_info should be printed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure either, some subsystems (like hwmon) don't consider these stuff as hard errors and just tell you to do clamp()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will be removing this, upon checking led subsystem already caps the value to the max if the user inputs a value exceeding the max. Thank you!
drivers/leds/leds-ltc3220.c
Outdated
| platform_data->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH); | ||
| if (IS_ERR(platform_data->reset_gpio)) { | ||
| dev_err(&client->dev, "Fail to set reset GPIO\n"); | ||
| return -ENOMEM; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since IS_ERR() has returned an error, you should pass it on instead of hardcoding ENOMEM, ie PTR_ERR(platform_data->reset_gpio);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, return dev_err_probe(dev, PTR_ERR(platform_data->reset_gpio), ...);
| #ifndef __LINUX_I2C_LTC3220_H | ||
| #define __LINUX_I2C_LTC3220_H | ||
|
|
||
| #define NUM_LEDS 18 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a LTC3220 prefix before these defines like you do in leds-ltc3220.c i.e LTC3220_NUM_LEDS ?
nunojsa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| compatible: | ||
| enum: | ||
| - lltc,ltc3220 | ||
| - lltc,ltc3220-1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adi, now 😉
| description: | | ||
| I2C slave address | ||
| 0x1c for ltc3220 | ||
| 0x1d for ltc3220-1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop the description here. It is already obvious
| maintainers: | ||
| - Edelweise Escala <[email protected]> | ||
|
|
||
| description: | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for | formatting
| reset-gpios: | ||
| maxItems: 1 | ||
| description: | | ||
| GPIO attached to the chip's reset pin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto about the description
| $ref: common.yaml# | ||
| properties: | ||
| reg: | ||
| description: | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drop |
drivers/leds/leds-ltc3220.c
Outdated
| platform_data->led_cdev.groups = ltc3220_config_groups; | ||
| ret = devm_led_classdev_register_ext(&client->dev, | ||
| &platform_data->led_cdev, | ||
| &init_data_config); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the above about? Is it like device level settings instead of individual LED settings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is like device level settings
drivers/leds/leds-ltc3220.c
Outdated
| return -ENOMEM; | ||
| } | ||
|
|
||
| ltc3220_reset(platform_data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is typically done in the beginning. In fact, this is bogus... Note that you're doing this after calling devm_led_classdev_register_ext() which will expose the userspace interfaces. So, effectively you can now race against userspace in a way that userspace does some configuration and then a reset happens.
drivers/leds/leds-ltc3220.c
Outdated
| MODULE_AUTHOR("Edelweise Escala <[email protected]>"); | ||
| MODULE_DESCRIPTION("LED driver for LTC3220 controllers"); | ||
| MODULE_LICENSE("GPL"); | ||
| MODULE_ALIAS("platform:leds-ltc3220"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically not used anymore
| @@ -0,0 +1,64 @@ | |||
| /* SPDX-License-Identifier: GPL-2.0-or-later */ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use only GPL2
| struct gpio_desc *reset_gpio; | ||
| }; | ||
|
|
||
| #endif /* __LINUX_I2C_LTC3220_H */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please drop the header file and move it to the source code. This is not supposed to be shared with any other source file AFAICT. Also, drop platform_data from the struct. Just name it struct ltc3220 or struct ltc3220_state. Platform data reference a very old way of doing things that is no longer used
d01ee06 to
b251647
Compare
|
V2 Changelog: dt-bindings: leds: ltc3220: Document LTC3220 18 channel LED Driver leds: ltc3220: add driver Added leds: ltc3220: Documentation for Custom ABI |
03831fa to
139802f
Compare
|
V3 Changelog: fixed wrong files on commit dt-bindings: leds: ltc3220: Document LTC3220 18 channel LED Driver leds: ltc3220: add driver leds: ltc3220: Documentation for Custom ABI |
machschmitt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| required: | ||
| - compatible | ||
| - reg | ||
| - reset-gpios |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| reg = <0x1c>; | ||
| #address-cells = <1>; | ||
| #size-cells = <0>; | ||
| reset-gpios = <&gpio 17 0>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the reset-gpio property can become clearer if the example makes use gpio dt-bindings
#include <dt-bindings/gpio/gpio.h>
...
reset-gpios = <&gpio 17 GPIO_ACTIVE_LOW>;| @@ -0,0 +1,48 @@ | |||
| What: /sys/class/leds/<led>/shutdown | |||
| Date: November 2025 | |||
| Contact: Edelweise Escala <[email protected]> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other LED ABI documentation also have a kernel version line. We're currently at 6.18-rc5 so, if this were going to be merged into the next release, I think we would add
KernelVersion: 6.18
Though, as the merge window for 6.18 has gone, you might probably use the next kernel version.
|
|
||
| What: /sys/class/leds/<led>/force_cpo_mode | ||
| Date: November 2025 | ||
| Contact: Edelweise Escala <[email protected]> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here and in the other doc entries.
drivers/leds/leds-ltc3220.c
Outdated
| #define LTC3220_ULED18 0x12 | ||
| #define LTC3220_GRAD_BLINK 0x13 | ||
|
|
||
| #define LTC3220_BIT0 BIT(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
drivers/leds/leds-ltc3220.c
Outdated
| struct led_classdev led_cdev; | ||
| struct device *dev; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these led_cdev and *dev fields not used?
drivers/leds/leds-ltc3220.c
Outdated
| if (ret) | ||
| return ret; | ||
|
|
||
| device_for_each_child_node(&client->dev, child) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use device_for_each_child_node_scoped(), then no need for fwnode_handle_put().
| 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; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means that all leds are required? Are all LEDs required? If not, I'd dev_war() and continue; instead of returning error.
| return -ENOMEM; | ||
| } | ||
|
|
||
| i = source - 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks a bit odd. I'd just use source to index uled_cfg and expect led dt node reg to be limited between 0 and 17 but see it's from 1 to 18. If the reg property will be kept limited from 1 to 18, I'd add a comment here reminding that the led node reg/index/address goes from 1 to 18.
|
|
||
| struct ltc3220_state { | ||
| u8 blink_cfg; | ||
| struct i2c_client *client; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In most places (if not all) where we do ltc3220_state->client, we could instead do ltc3220_state->uled_cfg[index], with an index of some led that is known to have been parsed. That way we could drop the i2c client field from ltc3220_state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, I wonder if we could drop i2c client field from struct ltc3220_uled_cfg. Both struct ltc3220_state and struct ltc3220_uled_cfg have a client field and they point to the same device IIUC. struct led_classdev has a dev field so I'm thinking we could do
struct ltc3220_state *ltc3220_state = dev_get_drvdata(led_cdev->dev);
in ltc3220_set_led_data(), then get the i2c device from the state struct.
80d038e to
03f6cf2
Compare
|
V4 Changelog: fixed name on SoB dt-bindings: leds: ltc3220: Document LTC3220 18 channel LED Driver leds: ltc3220: add driver leds: ltc3220: Documentation for Custom ABI |
| 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm this one seems something HW specific? Is this something that should change at runtime? If I'm right, consider a devicetree attr
| Contact: Edelweise Escala <[email protected]> | ||
| 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like something you should implement under PM (Power management). Either runtime PM or normal suspend. Pick one :)
| 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
| (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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
drivers/leds/leds-ltc3220.c
Outdated
| ret = ltc3220_set_blink_and_gradation(ltc3220_state, | ||
| state, | ||
| ltc3220_state->grad_cfg.gradation_speed, | ||
| ltc3220_state->grad_cfg.is_increasing); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above looks very bad
| &dev_attr_gradation_is_increasing.attr, | ||
| &dev_attr_blink_cfg.attr, | ||
| NULL | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Be prepared to justify these attributes... I'm still not sure about it
drivers/leds/leds-ltc3220.c
Outdated
| ltc3220_gradation_is_increasing_store); | ||
|
|
||
| static ssize_t ltc3220_blink_cfg_show(struct device *dev, | ||
| struct device_attribute *attr, char *buf) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take care about the above
drivers/leds/leds-ltc3220.c
Outdated
|
|
||
| ret = ltc3220_set_blink_and_gradation(ltc3220_state, | ||
| ltc3220_state->blink_cfg, | ||
| ltc3220_state->grad_cfg.gradation_speed, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
drivers/leds/leds-ltc3220.c
Outdated
| ret = ltc3220_set_blink_and_gradation(ltc3220_state, | ||
| ltc3220_state->blink_cfg, | ||
| state, | ||
| ltc3220_state->grad_cfg.is_increasing); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make sure to fix things like the above (unless my github is rendering this in a very odd way)
Add dt-binding for ltc3220. LTC3220 18 Channel LED driver Signed-off-by: Edelweise Escala <[email protected]>
Add documentation for different Custom ABI added for LTC3220. Signed-off-by: Edelweise Escala <[email protected]>
Add driver for ltc3220. LTC3220 18 Channel LED Driver Signed-off-by: Edelweise Escala <[email protected]>
Add entry for LTC3220 driver Signed-off-by: Edelweise Escala <[email protected]>
03f6cf2 to
e16e6b1
Compare
|
V5 Changelog: dt-bindings: leds: ltc3220: Document LTC3220 18 channel LED Driver leds: ltc3220: add driver leds: ltc3220: Documentation for Custom ABI |
vasbimpikasadi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting really better, loads of great feedback from everyone. This is shaping up quite nicely.
|
|
||
| ltc3220_state = devm_kzalloc(&client->dev, sizeof(*ltc3220_state), GFP_KERNEL); | ||
| if (!ltc3220_state) | ||
| return -ENODEV; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Memory allocation failure should return -ENOMEM, not -ENODEV
| &init_data); | ||
| if (ret) { | ||
| dev_err(&client->dev, "Fail LED class register\n"); | ||
| return -ENOMEM; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think ret should be propagated here to aid root causing issues should things go wrong?
| NULL | ||
| }; | ||
|
|
||
| static struct attribute_group device_cfg_group = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably should be const? It's never modified during runtime
PR Description
PR Type
PR Checklist