Re: [PATCH v2 1/4] leds: core: add generic support for color LED's

[Jacek Anaszewski <j.anaszewski@samsung.com>]

On 02/18/2016 09:34 PM, Heiner Kallweit wrote:
[...]
>>>>>             /* Time to switch the LED on. */
>>>>>             brightness = led_cdev->blink_brightness;
>>>>>             delay = led_cdev->blink_delay_on;
>>>>> @@ -133,8 +133,8 @@ static void led_set_software_blink(struct led_classdev *led_cdev,
>>>>>         if (current_brightness)
>>>>>             led_cdev->blink_brightness = current_brightness;
>>>>>         if (!led_cdev->blink_brightness)
>>>>> -        led_cdev->blink_brightness = led_cdev->max_brightness;
>>>>> -
>>>>> +        led_cdev->blink_brightness =
>>>>> +                led_validate_brightness(led_cdev, LED_FULL);
>>>>
>>>> This function name doesn't fit here, since term "validation" usually
>>>> refers to validating user provided data.
>>>>
>>>> led_confine_brightness() ?
>>>>
>>>> And why LED_FULL and not led_cdev->max_brightness?
>>>>
>>> LED_FULL is reduced to max_brightness anyway by led_validate_brightness.
>>> IMHO LED_FULL makes clearer that the intention is: switch it on.
>>> max_brightness is a device-specific property that I'd like to hide
>>> as far as possible in the generic core code.
>>
>> Without checking led_validate_brightness() internals it looks like this
>> patch was changing led_set_software_blink() semantics.
>>
> As far as I can see it doesn't. In monochrome mode
> led_validate_brightness(led_cdev, LED_FULL) evaluates to
> led_cdev->max_brightness.

I meant that looking at this change alone makes such an impression.
Besides, if we now have led_confine_brightness(), then it implies that
brightness will be limited up to LED_FULL level, whereas in fact
the upper limit will be max_brightness for monochrome LEDs.

-- 
Best regards,
Jacek Anaszewski