drivers/leds/leds-lp55xx-common.c | 34 +++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)
Hi,
I am trying to wrap my head around the implementation of the multicolor
LED support in the lp55xx family drivers.
The situation is quite straight forward when each LED is registered
and controlled individually but it gets quite messy once you use
the multi-color LED binding.
I am working with the imx6dl-yapp43-pegasus.dts board (in-tree). There
is one RGB LED driven by a LP5562 LED controller. Currently the RGB LED
is modeled as three separate LEDs and each of the LEDs has
individually tuned led-cur property. I would like to change the device
tree and use the multi-led binding to be able to use triggers on a chosen
RGB color.
When I was experimenting with that, I realized there is something wrong
with the colors and identified that the led-cur property is not properly
applied in case the multi-led binding is used. What ultimately happens is
that the led_current of the first LED in the multi-led group is set to
the value of led-cur property of the last LED in the group.
All the other LEDs in the group are left with the default reset value
of the controller (0xaf in my case).
Example:
led-controller@30 {
compatible = "ti,lp5562";
reg = <0x30>;
clock-mode = /bits/ 8 <1>;
#address-cells = <1>;
#size-cells = <0>;
status = "disabled";
multi-led@0 {
#address-cells = <1>;
#size-cells = <0>;
color = <LED_COLOR_ID_RGB>;
function = LED_FUNCTION_INDICATOR;
led@0 {
led-cur = /bits/ 8 <0x6e>;
max-cur = /bits/ 8 <0xc8>;
reg = <0>;
color = <LED_COLOR_ID_RED>;
};
led@1 {
led-cur = /bits/ 8 <0xbe>;
max-cur = /bits/ 8 <0xc8>;
reg = <1>;
color = <LED_COLOR_ID_GREEN>;
};
led@2 {
led-cur = /bits/ 8 <0xbe>;
max-cur = /bits/ 8 <0xc8>;
reg = <2>;
color = <LED_COLOR_ID_BLUE>;
};
};
Result (values read out directly with i2cget)
led@0 current = 0xbe, should be 0x6e
led@1 current = 0xaf, should be 0xbe
led@2 current = 0xaf, should be 0xbe
I tried to describe the steps that led to my discovery in the comments to
the file. Unfortunately I could not really figure out how this could be
properly fixed.
I would appreciate any comments to this problem and hopefully some ideas
how it could be solved.
Thank you,
Michal
---
drivers/leds/leds-lp55xx-common.c | 34 +++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c
index e71456a56ab8..d2fd2d296049 100644
--- a/drivers/leds/leds-lp55xx-common.c
+++ b/drivers/leds/leds-lp55xx-common.c
@@ -1060,12 +1060,17 @@ static int lp55xx_register_leds(struct lp55xx_led *led, struct lp55xx_chip *chip
return -EINVAL;
}
+ // Step 8
+ // num_channels = 1
for (i = 0; i < num_channels; i++) {
/* do not initialize channels that are not connected */
if (pdata->led_config[i].led_current == 0)
continue;
+ // The pdata->led_config[0].led_current contains the led-cur
+ // property value of the last LED from the multi-led node.
+ // Here we store that value to the first LED in that node.
led_current = pdata->led_config[i].led_current;
each = led + i;
ret = lp55xx_init_led(each, chip, i);
@@ -1119,8 +1124,16 @@ static int lp55xx_parse_common_child(struct device_node *np,
struct lp55xx_led_config *cfg,
int led_number, int *chan_nr)
{
+ // Step 6
+ // This is called 3-times (n-times in general, for each LED in the multi-led node)
+ // led_number = 0
+ // np = led@[0,1,2]
int ret;
+ // Size of the cfg is "1 lp55xx_led_config"
+ // led_number = 0 for each of the n-calls
+ // So the name, led_current and max_current variables are being
+ // overwritten until values from the last led@ subnode are stored.
of_property_read_string(np, "chan-name",
&cfg[led_number].name);
of_property_read_u8(np, "led-cur",
@@ -1139,6 +1152,11 @@ static int lp55xx_parse_multi_led_child(struct device_node *child,
struct lp55xx_led_config *cfg,
int child_number, int color_number)
{
+ // Step 5
+ // This is called 3-times (n-times in general, for each LED in the multi-led node)
+ // child_number = 0
+ // color_number = [0,1,2]
+ // child = led@[0,1,2]
int chan_nr, color_id, ret;
ret = lp55xx_parse_common_child(child, cfg, child_number, &chan_nr);
@@ -1159,6 +1177,10 @@ static int lp55xx_parse_multi_led(struct device_node *np,
struct lp55xx_led_config *cfg,
int child_number)
{
+ // Step 4
+ // This is called just once
+ // child_number = 0
+ // np = multi-led node
int num_colors = 0, ret;
for_each_available_child_of_node_scoped(np, child) {
@@ -1169,6 +1191,7 @@ static int lp55xx_parse_multi_led(struct device_node *np,
num_colors++;
}
+ // num_colors = 3
cfg[child_number].num_colors = num_colors;
return 0;
@@ -1178,6 +1201,10 @@ static int lp55xx_parse_logical_led(struct device_node *np,
struct lp55xx_led_config *cfg,
int child_number)
{
+ // Step 3
+ // This is called just once
+ // child_number = 0
+ // np = multi-led node
int led_color, ret;
int chan_nr = 0;
@@ -1189,6 +1216,7 @@ static int lp55xx_parse_logical_led(struct device_node *np,
return ret;
if (led_color == LED_COLOR_ID_RGB)
+ // We go this way
return lp55xx_parse_multi_led(np, cfg, child_number);
ret = lp55xx_parse_common_child(np, cfg, child_number, &chan_nr);
@@ -1215,18 +1243,22 @@ static struct lp55xx_platform_data *lp55xx_of_populate_pdata(struct device *dev,
if (!pdata)
return ERR_PTR(-ENOMEM);
+ // Step 2
+ // One RGB multiled, num_channels = 1
num_channels = of_get_available_child_count(np);
if (num_channels == 0) {
dev_err(dev, "no LED channels\n");
return ERR_PTR(-EINVAL);
}
+ dev_err(dev, "LED channels: %d\n", num_channels);
cfg = devm_kcalloc(dev, num_channels, sizeof(*cfg), GFP_KERNEL);
if (!cfg)
return ERR_PTR(-ENOMEM);
pdata->led_config = &cfg[0];
pdata->num_channels = num_channels;
+ // LP5562 max_channel = 4
cfg->max_channel = chip->cfg->max_channel;
for_each_available_child_of_node(np, child) {
@@ -1277,6 +1309,7 @@ int lp55xx_probe(struct i2c_client *client)
if (!pdata) {
if (np) {
+ // Step 1
pdata = lp55xx_of_populate_pdata(&client->dev, np,
chip);
if (IS_ERR(pdata))
@@ -1316,6 +1349,7 @@ int lp55xx_probe(struct i2c_client *client)
dev_info(&client->dev, "%s Programmable led chip found\n", id->name);
+ // Step 7
ret = lp55xx_register_leds(led, chip);
if (ret)
goto err_out;
--
2.43.0Hi Michal,
On 5/23/25 11:31, Michal Vokáč wrote:
> Hi,
>
> I am trying to wrap my head around the implementation of the multicolor
> LED support in the lp55xx family drivers.
>
> The situation is quite straight forward when each LED is registered
> and controlled individually but it gets quite messy once you use
> the multi-color LED binding.
>
> I am working with the imx6dl-yapp43-pegasus.dts board (in-tree). There
> is one RGB LED driven by a LP5562 LED controller. Currently the RGB LED
> is modeled as three separate LEDs and each of the LEDs has
> individually tuned led-cur property. I would like to change the device
> tree and use the multi-led binding to be able to use triggers on a chosen
> RGB color.
>
> When I was experimenting with that, I realized there is something wrong
> with the colors and identified that the led-cur property is not properly
> applied in case the multi-led binding is used. What ultimately happens is
> that the led_current of the first LED in the multi-led group is set to
> the value of led-cur property of the last LED in the group.
> All the other LEDs in the group are left with the default reset value
> of the controller (0xaf in my case).
>
> Example:
>
> led-controller@30 {
> compatible = "ti,lp5562";
> reg = <0x30>;
> clock-mode = /bits/ 8 <1>;
> #address-cells = <1>;
> #size-cells = <0>;
> status = "disabled";
>
> multi-led@0 {
> #address-cells = <1>;
> #size-cells = <0>;
> color = <LED_COLOR_ID_RGB>;
> function = LED_FUNCTION_INDICATOR;
>
> led@0 {
> led-cur = /bits/ 8 <0x6e>;
> max-cur = /bits/ 8 <0xc8>;
> reg = <0>;
> color = <LED_COLOR_ID_RED>;
> };
>
> led@1 {
> led-cur = /bits/ 8 <0xbe>;
> max-cur = /bits/ 8 <0xc8>;
> reg = <1>;
> color = <LED_COLOR_ID_GREEN>;
> };
>
> led@2 {
> led-cur = /bits/ 8 <0xbe>;
> max-cur = /bits/ 8 <0xc8>;
> reg = <2>;
> color = <LED_COLOR_ID_BLUE>;
> };
> };
>
> Result (values read out directly with i2cget)
>
> led@0 current = 0xbe, should be 0x6e
> led@1 current = 0xaf, should be 0xbe
> led@2 current = 0xaf, should be 0xbe
>
> I tried to describe the steps that led to my discovery in the comments to
> the file. Unfortunately I could not really figure out how this could be
> properly fixed.
>
> I would appreciate any comments to this problem and hopefully some ideas
> how it could be solved.
>
> Thank you,
> Michal
> ---
> drivers/leds/leds-lp55xx-common.c | 34 +++++++++++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
>
> diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c
> index e71456a56ab8..d2fd2d296049 100644
> --- a/drivers/leds/leds-lp55xx-common.c
> +++ b/drivers/leds/leds-lp55xx-common.c
> @@ -1060,12 +1060,17 @@ static int lp55xx_register_leds(struct lp55xx_led *led, struct lp55xx_chip *chip
> return -EINVAL;
> }
>
> + // Step 8
> + // num_channels = 1
> for (i = 0; i < num_channels; i++) {
>
> /* do not initialize channels that are not connected */
> if (pdata->led_config[i].led_current == 0)
> continue;
>
> + // The pdata->led_config[0].led_current contains the led-cur
> + // property value of the last LED from the multi-led node.
> + // Here we store that value to the first LED in that node.
> led_current = pdata->led_config[i].led_current;
> each = led + i;
> ret = lp55xx_init_led(each, chip, i);
> @@ -1119,8 +1124,16 @@ static int lp55xx_parse_common_child(struct device_node *np,
> struct lp55xx_led_config *cfg,
> int led_number, int *chan_nr)
> {
> + // Step 6
> + // This is called 3-times (n-times in general, for each LED in the multi-led node)
> + // led_number = 0
> + // np = led@[0,1,2]
> int ret;
>
> + // Size of the cfg is "1 lp55xx_led_config"
> + // led_number = 0 for each of the n-calls
> + // So the name, led_current and max_current variables are being
> + // overwritten until values from the last led@ subnode are stored.
It seems that struct lp55xx_led_config needs an improvement to be
capable of storing led-cur and max-cur per multi-subLEDs.
Probably there should be added a pointer property that will be
dynamically allocated either to a single element, or for multi-color
LEDs to multiple elements.
Let's say:
struct lp55xx_sub_led {
u8 led_current; /* mA x10, 0 if led is not connected */
u8 max_current;
int color_id;
int output_num;
}
struct lp55xx_led_config {
const char *name;
const char *default_trigger;
u8 chan_nr;
int num_colors;
unsigned int max_channel;
struct lp55xx_sub_led *leds;
};
Then lp55xx_parse_common_child() will need to accept also subLED id.
For monochrome LEDs it will be always 0.
> of_property_read_string(np, "chan-name",
> &cfg[led_number].name);
> of_property_read_u8(np, "led-cur",
> @@ -1139,6 +1152,11 @@ static int lp55xx_parse_multi_led_child(struct device_node *child,
> struct lp55xx_led_config *cfg,
> int child_number, int color_number)
> {
> + // Step 5
> + // This is called 3-times (n-times in general, for each LED in the multi-led node)
> + // child_number = 0
> + // color_number = [0,1,2]
> + // child = led@[0,1,2]
> int chan_nr, color_id, ret;
>
> ret = lp55xx_parse_common_child(child, cfg, child_number, &chan_nr);
> @@ -1159,6 +1177,10 @@ static int lp55xx_parse_multi_led(struct device_node *np,
> struct lp55xx_led_config *cfg,
> int child_number)
> {
> + // Step 4
> + // This is called just once
> + // child_number = 0
> + // np = multi-led node
> int num_colors = 0, ret;
>
> for_each_available_child_of_node_scoped(np, child) {
> @@ -1169,6 +1191,7 @@ static int lp55xx_parse_multi_led(struct device_node *np,
> num_colors++;
> }
>
> + // num_colors = 3
> cfg[child_number].num_colors = num_colors;
>
> return 0;
> @@ -1178,6 +1201,10 @@ static int lp55xx_parse_logical_led(struct device_node *np,
> struct lp55xx_led_config *cfg,
> int child_number)
> {
> + // Step 3
> + // This is called just once
> + // child_number = 0
> + // np = multi-led node
> int led_color, ret;
> int chan_nr = 0;
>
> @@ -1189,6 +1216,7 @@ static int lp55xx_parse_logical_led(struct device_node *np,
> return ret;
>
> if (led_color == LED_COLOR_ID_RGB)
> + // We go this way
> return lp55xx_parse_multi_led(np, cfg, child_number);
>
> ret = lp55xx_parse_common_child(np, cfg, child_number, &chan_nr);
> @@ -1215,18 +1243,22 @@ static struct lp55xx_platform_data *lp55xx_of_populate_pdata(struct device *dev,
> if (!pdata)
> return ERR_PTR(-ENOMEM);
>
> + // Step 2
> + // One RGB multiled, num_channels = 1
> num_channels = of_get_available_child_count(np);
> if (num_channels == 0) {
> dev_err(dev, "no LED channels\n");
> return ERR_PTR(-EINVAL);
> }
>
> + dev_err(dev, "LED channels: %d\n", num_channels);
> cfg = devm_kcalloc(dev, num_channels, sizeof(*cfg), GFP_KERNEL);
> if (!cfg)
> return ERR_PTR(-ENOMEM);
>
> pdata->led_config = &cfg[0];
> pdata->num_channels = num_channels;
> + // LP5562 max_channel = 4
> cfg->max_channel = chip->cfg->max_channel;
>
> for_each_available_child_of_node(np, child) {
> @@ -1277,6 +1309,7 @@ int lp55xx_probe(struct i2c_client *client)
>
> if (!pdata) {
> if (np) {
> + // Step 1
> pdata = lp55xx_of_populate_pdata(&client->dev, np,
> chip);
> if (IS_ERR(pdata))
> @@ -1316,6 +1349,7 @@ int lp55xx_probe(struct i2c_client *client)
>
> dev_info(&client->dev, "%s Programmable led chip found\n", id->name);
>
> + // Step 7
> ret = lp55xx_register_leds(led, chip);
> if (ret)
> goto err_out;
--
Best regards,
Jacek Anaszewski
On Fri, 23 May 2025, Michal Vokáč wrote:
> Hi,
>
> I am trying to wrap my head around the implementation of the multicolor
> LED support in the lp55xx family drivers.
>
> The situation is quite straight forward when each LED is registered
> and controlled individually but it gets quite messy once you use
> the multi-color LED binding.
>
> I am working with the imx6dl-yapp43-pegasus.dts board (in-tree). There
> is one RGB LED driven by a LP5562 LED controller. Currently the RGB LED
> is modeled as three separate LEDs and each of the LEDs has
> individually tuned led-cur property. I would like to change the device
> tree and use the multi-led binding to be able to use triggers on a chosen
> RGB color.
>
> When I was experimenting with that, I realized there is something wrong
> with the colors and identified that the led-cur property is not properly
> applied in case the multi-led binding is used. What ultimately happens is
> that the led_current of the first LED in the multi-led group is set to
> the value of led-cur property of the last LED in the group.
> All the other LEDs in the group are left with the default reset value
> of the controller (0xaf in my case).
Does this help you:
https://lore.kernel.org/r/20250526-led-fix-v4-1-33345f6c4a78@axis.com
> Example:
>
> led-controller@30 {
> compatible = "ti,lp5562";
> reg = <0x30>;
> clock-mode = /bits/ 8 <1>;
> #address-cells = <1>;
> #size-cells = <0>;
> status = "disabled";
>
> multi-led@0 {
> #address-cells = <1>;
> #size-cells = <0>;
> color = <LED_COLOR_ID_RGB>;
> function = LED_FUNCTION_INDICATOR;
>
> led@0 {
> led-cur = /bits/ 8 <0x6e>;
> max-cur = /bits/ 8 <0xc8>;
> reg = <0>;
> color = <LED_COLOR_ID_RED>;
> };
>
> led@1 {
> led-cur = /bits/ 8 <0xbe>;
> max-cur = /bits/ 8 <0xc8>;
> reg = <1>;
> color = <LED_COLOR_ID_GREEN>;
> };
>
> led@2 {
> led-cur = /bits/ 8 <0xbe>;
> max-cur = /bits/ 8 <0xc8>;
> reg = <2>;
> color = <LED_COLOR_ID_BLUE>;
> };
> };
>
> Result (values read out directly with i2cget)
>
> led@0 current = 0xbe, should be 0x6e
> led@1 current = 0xaf, should be 0xbe
> led@2 current = 0xaf, should be 0xbe
>
> I tried to describe the steps that led to my discovery in the comments to
> the file. Unfortunately I could not really figure out how this could be
> properly fixed.
>
> I would appreciate any comments to this problem and hopefully some ideas
> how it could be solved.
>
> Thank you,
> Michal
> ---
> drivers/leds/leds-lp55xx-common.c | 34 +++++++++++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
>
> diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c
> index e71456a56ab8..d2fd2d296049 100644
> --- a/drivers/leds/leds-lp55xx-common.c
> +++ b/drivers/leds/leds-lp55xx-common.c
> @@ -1060,12 +1060,17 @@ static int lp55xx_register_leds(struct lp55xx_led *led, struct lp55xx_chip *chip
> return -EINVAL;
> }
>
> + // Step 8
> + // num_channels = 1
> for (i = 0; i < num_channels; i++) {
>
> /* do not initialize channels that are not connected */
> if (pdata->led_config[i].led_current == 0)
> continue;
>
> + // The pdata->led_config[0].led_current contains the led-cur
> + // property value of the last LED from the multi-led node.
> + // Here we store that value to the first LED in that node.
> led_current = pdata->led_config[i].led_current;
> each = led + i;
> ret = lp55xx_init_led(each, chip, i);
> @@ -1119,8 +1124,16 @@ static int lp55xx_parse_common_child(struct device_node *np,
> struct lp55xx_led_config *cfg,
> int led_number, int *chan_nr)
> {
> + // Step 6
> + // This is called 3-times (n-times in general, for each LED in the multi-led node)
> + // led_number = 0
> + // np = led@[0,1,2]
> int ret;
>
> + // Size of the cfg is "1 lp55xx_led_config"
> + // led_number = 0 for each of the n-calls
> + // So the name, led_current and max_current variables are being
> + // overwritten until values from the last led@ subnode are stored.
> of_property_read_string(np, "chan-name",
> &cfg[led_number].name);
> of_property_read_u8(np, "led-cur",
> @@ -1139,6 +1152,11 @@ static int lp55xx_parse_multi_led_child(struct device_node *child,
> struct lp55xx_led_config *cfg,
> int child_number, int color_number)
> {
> + // Step 5
> + // This is called 3-times (n-times in general, for each LED in the multi-led node)
> + // child_number = 0
> + // color_number = [0,1,2]
> + // child = led@[0,1,2]
> int chan_nr, color_id, ret;
>
> ret = lp55xx_parse_common_child(child, cfg, child_number, &chan_nr);
> @@ -1159,6 +1177,10 @@ static int lp55xx_parse_multi_led(struct device_node *np,
> struct lp55xx_led_config *cfg,
> int child_number)
> {
> + // Step 4
> + // This is called just once
> + // child_number = 0
> + // np = multi-led node
> int num_colors = 0, ret;
>
> for_each_available_child_of_node_scoped(np, child) {
> @@ -1169,6 +1191,7 @@ static int lp55xx_parse_multi_led(struct device_node *np,
> num_colors++;
> }
>
> + // num_colors = 3
> cfg[child_number].num_colors = num_colors;
>
> return 0;
> @@ -1178,6 +1201,10 @@ static int lp55xx_parse_logical_led(struct device_node *np,
> struct lp55xx_led_config *cfg,
> int child_number)
> {
> + // Step 3
> + // This is called just once
> + // child_number = 0
> + // np = multi-led node
> int led_color, ret;
> int chan_nr = 0;
>
> @@ -1189,6 +1216,7 @@ static int lp55xx_parse_logical_led(struct device_node *np,
> return ret;
>
> if (led_color == LED_COLOR_ID_RGB)
> + // We go this way
> return lp55xx_parse_multi_led(np, cfg, child_number);
>
> ret = lp55xx_parse_common_child(np, cfg, child_number, &chan_nr);
> @@ -1215,18 +1243,22 @@ static struct lp55xx_platform_data *lp55xx_of_populate_pdata(struct device *dev,
> if (!pdata)
> return ERR_PTR(-ENOMEM);
>
> + // Step 2
> + // One RGB multiled, num_channels = 1
> num_channels = of_get_available_child_count(np);
> if (num_channels == 0) {
> dev_err(dev, "no LED channels\n");
> return ERR_PTR(-EINVAL);
> }
>
> + dev_err(dev, "LED channels: %d\n", num_channels);
> cfg = devm_kcalloc(dev, num_channels, sizeof(*cfg), GFP_KERNEL);
> if (!cfg)
> return ERR_PTR(-ENOMEM);
>
> pdata->led_config = &cfg[0];
> pdata->num_channels = num_channels;
> + // LP5562 max_channel = 4
> cfg->max_channel = chip->cfg->max_channel;
>
> for_each_available_child_of_node(np, child) {
> @@ -1277,6 +1309,7 @@ int lp55xx_probe(struct i2c_client *client)
>
> if (!pdata) {
> if (np) {
> + // Step 1
> pdata = lp55xx_of_populate_pdata(&client->dev, np,
> chip);
> if (IS_ERR(pdata))
> @@ -1316,6 +1349,7 @@ int lp55xx_probe(struct i2c_client *client)
>
> dev_info(&client->dev, "%s Programmable led chip found\n", id->name);
>
> + // Step 7
> ret = lp55xx_register_leds(led, chip);
> if (ret)
> goto err_out;
> --
> 2.43.0
>
--
Lee Jones [李琼斯]
On 12. 06. 25 15:48, Lee Jones wrote:
> On Fri, 23 May 2025, Michal Vokáč wrote:
>
>> Hi,
>>
>> I am trying to wrap my head around the implementation of the multicolor
>> LED support in the lp55xx family drivers.
>>
>> The situation is quite straight forward when each LED is registered
>> and controlled individually but it gets quite messy once you use
>> the multi-color LED binding.
>>
>> I am working with the imx6dl-yapp43-pegasus.dts board (in-tree). There
>> is one RGB LED driven by a LP5562 LED controller. Currently the RGB LED
>> is modeled as three separate LEDs and each of the LEDs has
>> individually tuned led-cur property. I would like to change the device
>> tree and use the multi-led binding to be able to use triggers on a chosen
>> RGB color.
>>
>> When I was experimenting with that, I realized there is something wrong
>> with the colors and identified that the led-cur property is not properly
>> applied in case the multi-led binding is used. What ultimately happens is
>> that the led_current of the first LED in the multi-led group is set to
>> the value of led-cur property of the last LED in the group.
>> All the other LEDs in the group are left with the default reset value
>> of the controller (0xaf in my case).
>
> Does this help you:
>
> https://lore.kernel.org/r/20250526-led-fix-v4-1-33345f6c4a78@axis.com
>
Unfortunately not.
The problem here is that the LP55xx family of LED controllers support
individually tuned current/max current for each channel but the multicolor
LED class driver was not designed with this in mind. Even though it was
actually introduced in the same series with all the relevant changes to
the lp55xx drivers.
The problem starts in lp55xx_of_populate_pdata():
// One RGB LED connected to the controller = one multiled node = one child.
num_channels = of_get_available_child_count(np);
if (num_channels == 0) {
dev_err(dev, "no LED channels\n");
return ERR_PTR(-EINVAL);
}
// So we allocate space for only one lp55xx_device_config structure.
cfg = devm_kcalloc(dev, num_channels, sizeof(*cfg), GFP_KERNEL);
if (!cfg)
return ERR_PTR(-ENOMEM);
pdata->led_config = &cfg[0];
pdata->num_channels = num_channels;
cfg->max_channel = chip->cfg->max_channel;
Later on in lp55xx_parse_common_child():
// This is called 3-times (for each LED color in the multi-led node)
// led_number = 0 for each call (because we have one multiled node)
// np = led@[0,1,2]
int ret;
// Size of the cfg is "1 lp55xx_led_config"
// led_number = 0 for each call
// So the name, led_current and max_current struct members are being
// overwritten until values from the last led@ subnode are stored.
// This seems buggy to me and we should not do that.
of_property_read_string(np, "chan-name",
&cfg[led_number].name);
of_property_read_u8(np, "led-cur",
&cfg[led_number].led_current);
of_property_read_u8(np, "max-cur",
&cfg[led_number].max_current);
// This part is OK. The reg property (chan_nr) is stored in
// output_num[num_colors] field member of the cfg structure.
ret = of_property_read_u32(np, "reg", chan_nr);
if (ret)
return ret;
And finally in lp55xx_register_leds():
// num_channels = 1 (one multi-led node)
for (i = 0; i < num_channels; i++) {
/* do not initialize channels that are not connected */
if (pdata->led_config[i].led_current == 0)
continue;
// The pdata->led_config[0].led_current contains the led-cur
// property value of the last LED from the multi-led node.
// Here we call the lp55xx_init_led() just once so we initialize
// just the first LED connected to the controller with a wrong
// value. The others are left with their default register values.
led_current = pdata->led_config[i].led_current;
each = led + i;
ret = lp55xx_init_led(each, chip, i);
My first idea was that we need to enhance the lp55xx_led_config structure
so that the led_current and max_current will be fields of [num_colors] size.
It will be also needed to add led_current and max_current members to
the mc_subled structure and the whole led-class-multiclolor.c must be
adapted accordingly.
There is also quite big difference that when the LEDs are registered individually,
max_current and led_current attributes of each LED are available in /sys.
Once you register the RGB LED as a multi-led, only the multi_intensity can be
changed but the current control is gone. But that is a different story.
It is pretty difficult to describe for me. Hopefully you get the idea what
is wrong here.
Best regards,
Michal
© 2016 - 2025 Red Hat, Inc.