Re: [PATCH] leds: core: add helpers for calling brightness_set(_blocking)

[Heiner Kallweit <hkallweit1@gmail.com>]

Am 12.02.2016 um 17:10 schrieb Jacek Anaszewski:
> On 02/07/2016 01:20 AM, Heiner Kallweit wrote:
>> Add helpers for calling brightness_set(_blocking) allowing to
>> simplify the code and make it better readable.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>   drivers/leds/led-core.c | 40 ++++++++++++++++++++++++++--------------
>>   1 file changed, 26 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
>> index ad684b6..c29e4c9 100644
>> --- a/drivers/leds/led-core.c
>> +++ b/drivers/leds/led-core.c
>> @@ -25,6 +25,26 @@ EXPORT_SYMBOL_GPL(leds_list_lock);
>>   LIST_HEAD(leds_list);
>>   EXPORT_SYMBOL_GPL(leds_list);
>>
>> +static int led_set_output(struct led_classdev *cdev,
> 
> s/cdev/led_cdev/
> 
> Please adhere to the existing naming convention.
> 
> led_set_output isn't too informative name, and can be misleading.
> We already have a number of different internal functions for setting LED brightness, so we have to be careful when adding new ones.
> 
> At the moment I don't have any good candidate.
> 
> __led_set_brightness() and __led_set_brightness_blocking() as a last
> resort. We had the former at some point.
> 
> Nonetheless it would be great if you came up with some better
> names:)
> 
I was expecting such a remark as I wasn't too happy with the names either.
But I had (and still have) no really good idea for a telling name being not
too long (as I'd like to avoid Java-like variable names spanning across more
than one line). Having said that your proposal of using a double underscore
prefix might still be the best compromise.

>> +              enum led_brightness value)
>> +{
>> +    if (!cdev->brightness_set)
>> +        return -ENOTSUPP;
>> +
>> +    cdev->brightness_set(cdev, value);
>> +
>> +    return 0;
>> +}
>> +
>> +static int led_set_output_blocking(struct led_classdev *cdev,
>> +                   enum led_brightness value)
>> +{
>> +    if (!cdev->brightness_set_blocking)
>> +        return -ENOTSUPP;
>> +
>> +    return cdev->brightness_set_blocking(cdev, value);
>> +}
>> +
>>   static void led_timer_function(unsigned long data)
>>   {
>>       struct led_classdev *led_cdev = (void *)data;
>> @@ -91,13 +111,10 @@ static void set_brightness_delayed(struct work_struct *ws)
>>           led_cdev->flags &= ~LED_BLINK_DISABLE;
>>       }
>>
>> -    if (led_cdev->brightness_set)
>> -        led_cdev->brightness_set(led_cdev, led_cdev->delayed_set_value);
>> -    else if (led_cdev->brightness_set_blocking)
>> -        ret = led_cdev->brightness_set_blocking(led_cdev,
>> -                        led_cdev->delayed_set_value);
>> -    else
>> -        ret = -ENOTSUPP;
>> +    ret = led_set_output(led_cdev, led_cdev->delayed_set_value);
>> +    if (ret == -ENOTSUPP)
>> +        ret = led_set_output_blocking(led_cdev,
>> +                          led_cdev->delayed_set_value);
>>       if (ret < 0 &&
>>           /* LED HW might have been unplugged, therefore don't warn */
>>           !(ret == -ENODEV && (led_cdev->flags & LED_UNREGISTERING) &&
>> @@ -236,10 +253,8 @@ void led_set_brightness_nopm(struct led_classdev *led_cdev,
>>                     enum led_brightness value)
>>   {
>>       /* Use brightness_set op if available, it is guaranteed not to sleep */
>> -    if (led_cdev->brightness_set) {
>> -        led_cdev->brightness_set(led_cdev, value);
>> +    if (!led_set_output(led_cdev, value))
>>           return;
>> -    }
>>
>>       /* If brightness setting can sleep, delegate it to a work queue task */
>>       led_cdev->delayed_set_value = value;
>> @@ -270,10 +285,7 @@ int led_set_brightness_sync(struct led_classdev *led_cdev,
>>       if (led_cdev->flags & LED_SUSPENDED)
>>           return 0;
>>
>> -    if (led_cdev->brightness_set_blocking)
>> -        return led_cdev->brightness_set_blocking(led_cdev,
>> -                             led_cdev->brightness);
>> -    return -ENOTSUPP;
>> +    return led_set_output_blocking(led_cdev, led_cdev->brightness);
>>   }
>>   EXPORT_SYMBOL_GPL(led_set_brightness_sync);
>>
>>
> 
>