[PATCH] led: core: fix misleading comment after workqueue removal from drivers

[PATCH] led: core: fix misleading comment after workqueue removal from drivers

[Heiner Kallweit]

Now that workqueues have been removed from individual drivers and
were replaced with a core-internal workqueue we shouldn't
encourage people to add new workqueues to drivers.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 include/linux/leds.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/linux/leds.h b/include/linux/leds.h
index bc1476f..4429887 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -50,7 +50,10 @@ struct led_classdev {
 #define LED_DEV_CAP_FLASH	(1 << 23)
 
 	/* Set LED brightness level */
-	/* Must not sleep, use a workqueue if needed */
+	/* Must not sleep. If no non-blocking version can be provided
+	 * set brightness_set_blocking only. The LED core will use an
+	 * internal work queue to create a non-blocking version.
+	 */
 	void		(*brightness_set)(struct led_classdev *led_cdev,
 					  enum led_brightness brightness);
 	/*
-- 
2.7.0

Re: [PATCH] led: core: fix misleading comment after workqueue removal from drivers

[Jacek Anaszewski]

Hi Heiner,

On 01/10/2016 06:16 PM, Heiner Kallweit wrote:
> Now that workqueues have been removed from individual drivers and
> were replaced with a core-internal workqueue we shouldn't
> encourage people to add new workqueues to drivers.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>   include/linux/leds.h | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index bc1476f..4429887 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -50,7 +50,10 @@ struct led_classdev {
>   #define LED_DEV_CAP_FLASH	(1 << 23)
>
>   	/* Set LED brightness level */
> -	/* Must not sleep, use a workqueue if needed */
> +	/* Must not sleep. If no non-blocking version can be provided
> +	 * set brightness_set_blocking only. The LED core will use an
> +	 * internal work queue to create a non-blocking version.
> +	 */

This comment isn't easily comprehensible at first glance.

How about:

"Must not sleep. Use brightness_set_blocking for drivers
that can sleep while setting brightness."

>   	void		(*brightness_set)(struct led_classdev *led_cdev,
>   					  enum led_brightness brightness);
>   	/*
>


-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH] led: core: fix misleading comment after workqueue removal from drivers

[Heiner Kallweit]

On Thu, Jan 21, 2016 at 10:03 AM, Jacek Anaszewski
<j.anaszewski@samsung.com> wrote:
> Hi Heiner,
>
> On 01/10/2016 06:16 PM, Heiner Kallweit wrote:
>>
>> Now that workqueues have been removed from individual drivers and
>> were replaced with a core-internal workqueue we shouldn't
>> encourage people to add new workqueues to drivers.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>   include/linux/leds.h | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/leds.h b/include/linux/leds.h
>> index bc1476f..4429887 100644
>> --- a/include/linux/leds.h
>> +++ b/include/linux/leds.h
>> @@ -50,7 +50,10 @@ struct led_classdev {
>>   #define LED_DEV_CAP_FLASH     (1 << 23)
>>
>>         /* Set LED brightness level */
>> -       /* Must not sleep, use a workqueue if needed */
>> +       /* Must not sleep. If no non-blocking version can be provided
>> +        * set brightness_set_blocking only. The LED core will use an
>> +        * internal work queue to create a non-blocking version.
>> +        */
>
>
> This comment isn't easily comprehensible at first glance.
>
> How about:
>
> "Must not sleep. Use brightness_set_blocking for drivers
> that can sleep while setting brightness."
>
Yes, this is better. It's simpler and clearer.
Are you going to fix the comment on your own or would you like me
to submit it as a patch?

Regards, Heiner

>>         void            (*brightness_set)(struct led_classdev *led_cdev,
>>                                           enum led_brightness brightness);
>>         /*
>>
>
>
> --
> Best Regards,
> Jacek Anaszewski

Re: [PATCH] led: core: fix misleading comment after workqueue removal from drivers

[Jacek Anaszewski]

On 01/21/2016 10:50 AM, Heiner Kallweit wrote:
> On Thu, Jan 21, 2016 at 10:03 AM, Jacek Anaszewski
> <j.anaszewski@samsung.com> wrote:
>> Hi Heiner,
>>
>> On 01/10/2016 06:16 PM, Heiner Kallweit wrote:
>>>
>>> Now that workqueues have been removed from individual drivers and
>>> were replaced with a core-internal workqueue we shouldn't
>>> encourage people to add new workqueues to drivers.
>>>
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>> ---
>>>    include/linux/leds.h | 5 ++++-
>>>    1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/linux/leds.h b/include/linux/leds.h
>>> index bc1476f..4429887 100644
>>> --- a/include/linux/leds.h
>>> +++ b/include/linux/leds.h
>>> @@ -50,7 +50,10 @@ struct led_classdev {
>>>    #define LED_DEV_CAP_FLASH     (1 << 23)
>>>
>>>          /* Set LED brightness level */
>>> -       /* Must not sleep, use a workqueue if needed */
>>> +       /* Must not sleep. If no non-blocking version can be provided
>>> +        * set brightness_set_blocking only. The LED core will use an
>>> +        * internal work queue to create a non-blocking version.
>>> +        */
>>
>>
>> This comment isn't easily comprehensible at first glance.
>>
>> How about:
>>
>> "Must not sleep. Use brightness_set_blocking for drivers
>> that can sleep while setting brightness."
>>
> Yes, this is better. It's simpler and clearer.
> Are you going to fix the comment on your own or would you like me
> to submit it as a patch?

Since you are the original author of the patch, feel free to submit
the final version with the agreed comment.

>>>          void            (*brightness_set)(struct led_classdev *led_cdev,
>>>                                            enum led_brightness brightness);
>>>          /*
>>>
>>
>>
>> --
>> Best Regards,
>> Jacek Anaszewski
>
>


-- 
Best Regards,
Jacek Anaszewski