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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <5603114d-4cc8-4d7c-96ae-838e70e5e385@prodrive-technologies.com>
Date: Wed, 21 Jan 2026 18:59:02 +0100
From: Martijn de Gouw <martijn.de.gouw@...drive-technologies.com>
To: Jean-Jacques Hiblot <jjhiblot@...phandler.com>, Lee Jones <lee@...nel.org>
Cc: Pavel Machek <pavel@...nel.org>, linux-leds@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] leds: group-multicolor: Add support for initial value.

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@...phandler.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@...drive-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