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

[Lee Jones <lee@kernel.org>]

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 [李琼斯]