Re: [RFC RESEND] leds: lp55xx: led-cur property is not properly applied to multi-led

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

Hi 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