[v2] leds: group-multicolor: Add support for initial value.

[PATCH v2] leds: group-multicolor: Add support for initial value.

Posted by Martijn de Gouw 2 months, 2 weeks ago

When the driver is loaded, it turned off all LEDs in the group. This
patch changes the driver to take over existing LED states and set
the brighness and intensity in the group accordingly.

Signed-off-by: Martijn de Gouw <martijn.de.gouw@prodrive-technologies.com>
---
 drivers/leds/rgb/leds-group-multicolor.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/rgb/leds-group-multicolor.c b/drivers/leds/rgb/leds-group-multicolor.c
index 548c7dd63ba1e..a12720d73f188 100644
--- a/drivers/leds/rgb/leds-group-multicolor.c
+++ b/drivers/leds/rgb/leds-group-multicolor.c
@@ -69,6 +69,7 @@ static int leds_gmc_probe(struct platform_device *pdev)
 	struct mc_subled *subled;
 	struct leds_multicolor *priv;
 	unsigned int max_brightness = 0;
+	unsigned int default_brightness = 0;
 	int i, ret, count = 0, common_flags = 0;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
@@ -96,6 +97,13 @@ static int leds_gmc_probe(struct platform_device *pdev)
 
 		max_brightness = max(max_brightness, led_cdev->max_brightness);
 
+		/*
+		 * If any LED is on, set brightness to the max brightness.
+		 * The actual brightness of the LED is set as intensity value.
+		 */
+		if (led_cdev->brightness)
+			default_brightness = max_brightness;
+
 		count++;
 	}
 
@@ -109,14 +117,16 @@ static int leds_gmc_probe(struct platform_device *pdev)
 
 		subled[i].color_index = led_cdev->color;
 
-		/* Configure the LED intensity to its maximum */
-		subled[i].intensity = max_brightness;
+		/* Configure the LED intensity to its current brightness */
+		subled[i].intensity = DIV_ROUND_CLOSEST(led_cdev->brightness * max_brightness,
+							led_cdev->max_brightness);
 	}
 
 	/* Initialise the multicolor's LED class device */
 	cdev = &priv->mc_cdev.led_cdev;
 	cdev->brightness_set_blocking = leds_gmc_set;
 	cdev->max_brightness = max_brightness;
+	cdev->brightness = default_brightness;
 	cdev->color = LED_COLOR_ID_MULTI;
 	priv->mc_cdev.num_colors = count;
 
-- 
2.39.2

Re: [PATCH v2] leds: group-multicolor: Add support for initial value.

Posted by Lee Jones 1 month, 3 weeks ago

INTENTIONAL TOP POST: I think the original author should have been on Cc.

Adding: Jean-Jacques Hiblot <jjhiblot@traphandler.com>

> When the driver is loaded, it turned off all LEDs in the group. This
> patch changes the driver to take over existing LED states and set
> the brighness and intensity in the group accordingly.
> 
> Signed-off-by: Martijn de Gouw <martijn.de.gouw@prodrive-technologies.com>
> ---
>  drivers/leds/rgb/leds-group-multicolor.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/leds/rgb/leds-group-multicolor.c b/drivers/leds/rgb/leds-group-multicolor.c
> index 548c7dd63ba1e..a12720d73f188 100644
> --- a/drivers/leds/rgb/leds-group-multicolor.c
> +++ b/drivers/leds/rgb/leds-group-multicolor.c
> @@ -69,6 +69,7 @@ static int leds_gmc_probe(struct platform_device *pdev)
>  	struct mc_subled *subled;
>  	struct leds_multicolor *priv;
>  	unsigned int max_brightness = 0;
> +	unsigned int default_brightness = 0;
>  	int i, ret, count = 0, common_flags = 0;
>  
>  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> @@ -96,6 +97,13 @@ static int leds_gmc_probe(struct platform_device *pdev)
>  
>  		max_brightness = max(max_brightness, led_cdev->max_brightness);
>  
> +		/*
> +		 * If any LED is on, set brightness to the max brightness.
> +		 * The actual brightness of the LED is set as intensity value.
> +		 */
> +		if (led_cdev->brightness)
> +			default_brightness = max_brightness;
> +
>  		count++;
>  	}
>  
> @@ -109,14 +117,16 @@ static int leds_gmc_probe(struct platform_device *pdev)
>  
>  		subled[i].color_index = led_cdev->color;
>  
> -		/* Configure the LED intensity to its maximum */
> -		subled[i].intensity = max_brightness;
> +		/* Configure the LED intensity to its current brightness */
> +		subled[i].intensity = DIV_ROUND_CLOSEST(led_cdev->brightness * max_brightness,
> +							led_cdev->max_brightness);
>  	}
>  
>  	/* Initialise the multicolor's LED class device */
>  	cdev = &priv->mc_cdev.led_cdev;
>  	cdev->brightness_set_blocking = leds_gmc_set;
>  	cdev->max_brightness = max_brightness;
> +	cdev->brightness = default_brightness;
>  	cdev->color = LED_COLOR_ID_MULTI;
>  	priv->mc_cdev.num_colors = count;
>  
> -- 
> 2.39.2
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH v2] leds: group-multicolor: Add support for initial value.

Posted by Jean-Jacques Hiblot 1 month, 3 weeks ago


On 14/12/2025 16:21, Lee Jones wrote:
> INTENTIONAL TOP POST: I think the original author should have been on Cc.
> 
Hi,

Sorry, I didn't follow the discussion and I'm not sure what you're 
trying to achieve with this patch. So correct me if I'm wrong.
As I understand it you're trying to capture the current state of the
LEDs at probe() time to set the the relative intensity of the LEDs 
constituting the group.

> Adding: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
> 
>> When the driver is loaded, it turned off all LEDs in the group. This
>> patch changes the driver to take over existing LED states and set
>> the brighness and intensity in the group accordingly.
>>
>> Signed-off-by: Martijn de Gouw <martijn.de.gouw@prodrive-technologies.com>
>> ---
>>   drivers/leds/rgb/leds-group-multicolor.c | 14 ++++++++++++--
>>   1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/leds/rgb/leds-group-multicolor.c b/drivers/leds/rgb/leds-group-multicolor.c
>> index 548c7dd63ba1e..a12720d73f188 100644
>> --- a/drivers/leds/rgb/leds-group-multicolor.c
>> +++ b/drivers/leds/rgb/leds-group-multicolor.c
>> @@ -69,6 +69,7 @@ static int leds_gmc_probe(struct platform_device *pdev)
>>   	struct mc_subled *subled;
>>   	struct leds_multicolor *priv;
>>   	unsigned int max_brightness = 0;
>> +	unsigned int default_brightness = 0;
>>   	int i, ret, count = 0, common_flags = 0;
>>   
>>   	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> @@ -96,6 +97,13 @@ static int leds_gmc_probe(struct platform_device *pdev)
>>   
>>   		max_brightness = max(max_brightness, led_cdev->max_brightness);
>>   
>> +		/*
>> +		 * If any LED is on, set brightness to the max brightness.
>> +		 * The actual brightness of the LED is set as intensity value.
>> +		 */
>> +		if (led_cdev->brightness)
>> +			default_brightness = max_brightness;
Storing a brightness here is a bit confusing. What we're interested in 
is whether at least on of the LED is on (and thus the group is 
considered on). I've would rather use a boolean here, like is_on = true
>> +
>>   		count++;
>>   	}
>>   
>> @@ -109,14 +117,16 @@ static int leds_gmc_probe(struct platform_device *pdev)
>>   
>>   		subled[i].color_index = led_cdev->color;
>>   
>> -		/* Configure the LED intensity to its maximum */
>> -		subled[i].intensity = max_brightness;
>> +		/* Configure the LED intensity to its current brightness */
>> +		subled[i].intensity = DIV_ROUND_CLOSEST(led_cdev->brightness * max_brightness,
>> +							led_cdev->max_brightness);
You cannot use this computation in all cases. If the LEDs have not been 
turned on, their intensity will always be 0. Maybe the 'is_on' flag can 
be use to select which computation should be used.
>>   	}
>>   
>>   	/* Initialise the multicolor's LED class device */
>>   	cdev = &priv->mc_cdev.led_cdev;
>>   	cdev->brightness_set_blocking = leds_gmc_set;
>>   	cdev->max_brightness = max_brightness;

>> +	cdev->brightness = default_brightness;
cdev->brightness = is_on ? max_brightness : 0;
>>   	cdev->color = LED_COLOR_ID_MULTI;
>>   	priv->mc_cdev.num_colors = count;
>>   

JJ
>> -- 
>> 2.39.2
>>
>

Re: [PATCH v2] leds: group-multicolor: Add support for initial value.

Posted by Martijn de Gouw 1 month ago

Hi,

On 12/15/2025 11:24 PM, Jean-Jacques Hiblot wrote:
> 
> 
> On 14/12/2025 16:21, Lee Jones wrote:
>> INTENTIONAL TOP POST: I think the original author should have been on Cc.
>>
> Hi,
> 
> Sorry, I didn't follow the discussion and I'm not sure what you're trying to achieve with this patch. So correct me if I'm wrong.
> As I understand it you're trying to capture the current state of the
> LEDs at probe() time to set the the relative intensity of the LEDs constituting the group.

Yes, that's indeed what I tried to achieve.

> 
>> Adding: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
>>
>>> When the driver is loaded, it turned off all LEDs in the group. This
>>> patch changes the driver to take over existing LED states and set
>>> the brighness and intensity in the group accordingly.
>>>
>>> Signed-off-by: Martijn de Gouw <martijn.de.gouw@prodrive-technologies.com>
>>> ---
>>>   drivers/leds/rgb/leds-group-multicolor.c | 14 ++++++++++++--
>>>   1 file changed, 12 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/leds/rgb/leds-group-multicolor.c b/drivers/leds/rgb/leds-group-multicolor.c
>>> index 548c7dd63ba1e..a12720d73f188 100644
>>> --- a/drivers/leds/rgb/leds-group-multicolor.c
>>> +++ b/drivers/leds/rgb/leds-group-multicolor.c
>>> @@ -69,6 +69,7 @@ static int leds_gmc_probe(struct platform_device *pdev)
>>>       struct mc_subled *subled;
>>>       struct leds_multicolor *priv;
>>>       unsigned int max_brightness = 0;
>>> +    unsigned int default_brightness = 0;
>>>       int i, ret, count = 0, common_flags = 0;
>>>         priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>> @@ -96,6 +97,13 @@ static int leds_gmc_probe(struct platform_device *pdev)
>>>             max_brightness = max(max_brightness, led_cdev->max_brightness);
>>>   +        /*
>>> +         * If any LED is on, set brightness to the max brightness.
>>> +         * The actual brightness of the LED is set as intensity value.
>>> +         */
>>> +        if (led_cdev->brightness)
>>> +            default_brightness = max_brightness;
> Storing a brightness here is a bit confusing. What we're interested in is whether at least on of the LED is on (and thus the group is considered on). I've would rather use a boolean here, like is_on = true

I'll change it.

>>> +
>>>           count++;
>>>       }
>>>   @@ -109,14 +117,16 @@ static int leds_gmc_probe(struct platform_device *pdev)
>>>             subled[i].color_index = led_cdev->color;
>>>   -        /* Configure the LED intensity to its maximum */
>>> -        subled[i].intensity = max_brightness;
>>> +        /* Configure the LED intensity to its current brightness */
>>> +        subled[i].intensity = DIV_ROUND_CLOSEST(led_cdev->brightness * max_brightness,
>>> +                            led_cdev->max_brightness);
> You cannot use this computation in all cases. If the LEDs have not been turned on, their intensity will always be 0. Maybe the 'is_on' flag can be use to select which computation should be used.

I'm not sure what you mean, if led_cdev->brightness is 0, the result of the DIV will also be 0.
Or you want to guard this DIV explicitly with something like this:

subled[i]->intensity = led_cdev->brightness == 0 ? 0 : DIV_ROUND_CLOSEST(...);


>>>       }
>>>         /* Initialise the multicolor's LED class device */
>>>       cdev = &priv->mc_cdev.led_cdev;
>>>       cdev->brightness_set_blocking = leds_gmc_set;
>>>       cdev->max_brightness = max_brightness;
> 
>>> +    cdev->brightness = default_brightness;
> cdev->brightness = is_on ? max_brightness : 0;
>>>       cdev->color = LED_COLOR_ID_MULTI;
>>>       priv->mc_cdev.num_colors = count;
>>>   
> 
> JJ
>>> -- 
>>> 2.39.2
>>>
>>
> 

Regards, Martijn
-- 
Martijn de Gouw
Designer
Prodrive Technologies B.V.
Mobile: +31 63 17 76 161
Phone:  +31 40 26 76 200

Re: [PATCH v2] leds: group-multicolor: Add support for initial value.

Posted by Martijn de Gouw 2 weeks, 3 days ago

Hi

>> Hi,
>>
>> Sorry, I didn't follow the discussion and I'm not sure what you're trying to achieve with this patch. So correct me if I'm wrong.
>> As I understand it you're trying to capture the current state of the
>> LEDs at probe() time to set the the relative intensity of the LEDs constituting the group.
> Yes, that's indeed what I tried to achieve.
>
>>> Adding: Jean-Jacques Hiblot <jjhiblot@traphandler.com>
>>>
>>>> When the driver is loaded, it turned off all LEDs in the group. This
>>>> patch changes the driver to take over existing LED states and set
>>>> the brighness and intensity in the group accordingly.
>>>>
>>>> Signed-off-by: Martijn de Gouw <martijn.de.gouw@prodrive-technologies.com>
>>>> ---
>>>>   drivers/leds/rgb/leds-group-multicolor.c | 14 ++++++++++++--
>>>>   1 file changed, 12 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/leds/rgb/leds-group-multicolor.c b/drivers/leds/rgb/leds-group-multicolor.c
>>>> index 548c7dd63ba1e..a12720d73f188 100644
>>>> --- a/drivers/leds/rgb/leds-group-multicolor.c
>>>> +++ b/drivers/leds/rgb/leds-group-multicolor.c
>>>> @@ -69,6 +69,7 @@ static int leds_gmc_probe(struct platform_device *pdev)
>>>>       struct mc_subled *subled;
>>>>       struct leds_multicolor *priv;
>>>>       unsigned int max_brightness = 0;
>>>> +    unsigned int default_brightness = 0;
>>>>       int i, ret, count = 0, common_flags = 0;
>>>>         priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>>> @@ -96,6 +97,13 @@ static int leds_gmc_probe(struct platform_device *pdev)
>>>>             max_brightness = max(max_brightness, led_cdev->max_brightness);
>>>>   +        /*
>>>> +         * If any LED is on, set brightness to the max brightness.
>>>> +         * The actual brightness of the LED is set as intensity value.
>>>> +         */
>>>> +        if (led_cdev->brightness)
>>>> +            default_brightness = max_brightness;
>> Storing a brightness here is a bit confusing. What we're interested in is whether at least on of the LED is on (and thus the group is considered on). I've would rather use a boolean here, like is_on = true
> I'll change it.
>
>>>> +
>>>>           count++;
>>>>       }
>>>>   @@ -109,14 +117,16 @@ static int leds_gmc_probe(struct platform_device *pdev)
>>>>             subled[i].color_index = led_cdev->color;
>>>>   -        /* Configure the LED intensity to its maximum */
>>>> -        subled[i].intensity = max_brightness;
>>>> +        /* Configure the LED intensity to its current brightness */
>>>> +        subled[i].intensity = DIV_ROUND_CLOSEST(led_cdev->brightness * max_brightness,
>>>> +                            led_cdev->max_brightness);
>> You cannot use this computation in all cases. If the LEDs have not been turned on, their intensity will always be 0. Maybe the 'is_on' flag can be use to select which computation should be used.
> I'm not sure what you mean, if led_cdev->brightness is 0, the result of the DIV will also be 0.
> Or you want to guard this DIV explicitly with something like this:
>
> subled[i]->intensity = led_cdev->brightness == 0 ? 0 : DIV_ROUND_CLOSEST(...);


Are there any additional comments on this calculation?


>
>
>>>>       }
>>>>         /* Initialise the multicolor's LED class device */
>>>>       cdev = &priv->mc_cdev.led_cdev;
>>>>       cdev->brightness_set_blocking = leds_gmc_set;
>>>>       cdev->max_brightness = max_brightness;
>>>> +    cdev->brightness = default_brightness;
>> cdev->brightness = is_on ? max_brightness : 0;


Shall I go ahead and submit the proposed 'is_on' change?

Regards, Martijn

-- 
Martijn de Gouw
Designer
Prodrive Technologies B.V.
Mobile: +31 63 17 76 161
Phone:  +31 40 26 76 200