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

[Martijn de Gouw]

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@xxxxxxxxxxxxxxx>
>>
>>> 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@xxxxxxxxxxxxxxxxxxxxxxxxx>
>>> ---
>>>   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