Making led_set_brightness_sync() use led_set_brightness_nosleep() has 2
advantages:
- works for LED controllers that do not provide brightness_set_blocking()
- When the blocking callback is used, it uses the workqueue to update the
LED state, removing the need for mutual exclusion between
led_set_brightness_sync() and set_brightness_delayed().
Signed-off-by: Jean-Jacques Hiblot <[email protected]>
---
drivers/leds/led-core.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index f1f718dbe0f8..50e28a8f9357 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -294,15 +294,17 @@ EXPORT_SYMBOL_GPL(led_set_brightness_nosleep);
int led_set_brightness_sync(struct led_classdev *led_cdev,
enum led_brightness value)
{
+ int ret;
+
if (led_cdev->blink_delay_on || led_cdev->blink_delay_off)
return -EBUSY;
- led_cdev->brightness = min(value, led_cdev->max_brightness);
-
- if (led_cdev->flags & LED_SUSPENDED)
- return 0;
+ ret = led_set_brightness_nosleep(led_cdev, value);
+ if (!ret)
+ return ret;
- return __led_set_brightness_blocking(led_cdev, led_cdev->brightness);
+ flush_work(&led_cdev->set_brightness_work);
+ return 0;
}
EXPORT_SYMBOL_GPL(led_set_brightness_sync);
--
2.17.1
Hi Jean,
On 9/20/19 2:25 PM, Jean-Jacques Hiblot wrote:
> Making led_set_brightness_sync() use led_set_brightness_nosleep() has 2
> advantages:
> - works for LED controllers that do not provide brightness_set_blocking()
> - When the blocking callback is used, it uses the workqueue to update the
> LED state, removing the need for mutual exclusion between
> led_set_brightness_sync() and set_brightness_delayed().
And third:
- it compromises the "sync" part of the function name :-)
This function has been introduced specifically to be blocking
and have the immediate effect. Its sole client is
drivers/media/v4l2-core/v4l2-flash-led-class.c.
>
> Signed-off-by: Jean-Jacques Hiblot <[email protected]>
> ---
> drivers/leds/led-core.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index f1f718dbe0f8..50e28a8f9357 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -294,15 +294,17 @@ EXPORT_SYMBOL_GPL(led_set_brightness_nosleep);
> int led_set_brightness_sync(struct led_classdev *led_cdev,
> enum led_brightness value)
> {
> + int ret;
> +
> if (led_cdev->blink_delay_on || led_cdev->blink_delay_off)
> return -EBUSY;
>
> - led_cdev->brightness = min(value, led_cdev->max_brightness);
> -
> - if (led_cdev->flags & LED_SUSPENDED)
> - return 0;
> + ret = led_set_brightness_nosleep(led_cdev, value);
> + if (!ret)
> + return ret;
>
> - return __led_set_brightness_blocking(led_cdev, led_cdev->brightness);
> + flush_work(&led_cdev->set_brightness_work);
> + return 0;
> }
> EXPORT_SYMBOL_GPL(led_set_brightness_sync);
>
>
--
Best regards,
Jacek Anaszewski
Hi Jean-Jacques,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[cannot apply to v5.3 next-20190919]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Jean-Jacques-Hiblot/leds-Add-control-of-the-voltage-current-regulator-to-the-LED-core/20190920-220416
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 7.4.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=sh
:::::: branch date: 2 hours ago
:::::: commit date: 2 hours ago
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <[email protected]>
All errors (new ones prefixed by >>):
drivers//leds/led-core.c: In function 'led_set_brightness_sync':
>> drivers//leds/led-core.c:302:6: error: void value not ignored as it ought to be
ret = led_set_brightness_nosleep(led_cdev, value);
^
# https://github.com/0day-ci/linux/commit/54301e6f4e910f292045a1afa62ef732791e1bb5
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 54301e6f4e910f292045a1afa62ef732791e1bb5
vim +302 drivers//leds/led-core.c
81fe8e5b73e3f4 Jacek Anaszewski 2015-10-07 293
13ae79bbe4c214 Jacek Anaszewski 2015-10-07 294 int led_set_brightness_sync(struct led_classdev *led_cdev,
13ae79bbe4c214 Jacek Anaszewski 2015-10-07 295 enum led_brightness value)
13ae79bbe4c214 Jacek Anaszewski 2015-10-07 296 {
54301e6f4e910f Jean-Jacques Hiblot 2019-09-20 297 int ret;
54301e6f4e910f Jean-Jacques Hiblot 2019-09-20 298
13ae79bbe4c214 Jacek Anaszewski 2015-10-07 299 if (led_cdev->blink_delay_on || led_cdev->blink_delay_off)
13ae79bbe4c214 Jacek Anaszewski 2015-10-07 300 return -EBUSY;
13ae79bbe4c214 Jacek Anaszewski 2015-10-07 301
54301e6f4e910f Jean-Jacques Hiblot 2019-09-20 @302 ret = led_set_brightness_nosleep(led_cdev, value);
54301e6f4e910f Jean-Jacques Hiblot 2019-09-20 303 if (!ret)
54301e6f4e910f Jean-Jacques Hiblot 2019-09-20 304 return ret;
13ae79bbe4c214 Jacek Anaszewski 2015-10-07 305
54301e6f4e910f Jean-Jacques Hiblot 2019-09-20 306 flush_work(&led_cdev->set_brightness_work);
13ae79bbe4c214 Jacek Anaszewski 2015-10-07 307 return 0;
13ae79bbe4c214 Jacek Anaszewski 2015-10-07 308 }
13ae79bbe4c214 Jacek Anaszewski 2015-10-07 309 EXPORT_SYMBOL_GPL(led_set_brightness_sync);
13ae79bbe4c214 Jacek Anaszewski 2015-10-07 310
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Jacek,
On 20/09/2019 23:10, Jacek Anaszewski wrote:
> Hi Jean,
>
> On 9/20/19 2:25 PM, Jean-Jacques Hiblot wrote:
>> Making led_set_brightness_sync() use led_set_brightness_nosleep() has 2
>> advantages:
>> - works for LED controllers that do not provide brightness_set_blocking()
>> - When the blocking callback is used, it uses the workqueue to update the
>> LED state, removing the need for mutual exclusion between
>> led_set_brightness_sync() and set_brightness_delayed().
> And third:
>
> - it compromises the "sync" part of the function name :-)
Making it sync is the role of the flush_work() function. It waits until
the deferred work has been done.
JJ
> This function has been introduced specifically to be blocking
> and have the immediate effect. Its sole client is
> drivers/media/v4l2-core/v4l2-flash-led-class.c.
>
>> Signed-off-by: Jean-Jacques Hiblot <[email protected]>
>> ---
>> drivers/leds/led-core.c | 12 +++++++-----
>> 1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
>> index f1f718dbe0f8..50e28a8f9357 100644
>> --- a/drivers/leds/led-core.c
>> +++ b/drivers/leds/led-core.c
>> @@ -294,15 +294,17 @@ EXPORT_SYMBOL_GPL(led_set_brightness_nosleep);
>> int led_set_brightness_sync(struct led_classdev *led_cdev,
>> enum led_brightness value)
>> {
>> + int ret;
>> +
>> if (led_cdev->blink_delay_on || led_cdev->blink_delay_off)
>> return -EBUSY;
>>
>> - led_cdev->brightness = min(value, led_cdev->max_brightness);
>> -
>> - if (led_cdev->flags & LED_SUSPENDED)
>> - return 0;
>> + ret = led_set_brightness_nosleep(led_cdev, value);
>> + if (!ret)
>> + return ret;
>>
>> - return __led_set_brightness_blocking(led_cdev, led_cdev->brightness);
>> + flush_work(&led_cdev->set_brightness_work);
>> + return 0;
>> }
>> EXPORT_SYMBOL_GPL(led_set_brightness_sync);
>>
>>
Hi Jean,
On 9/23/19 11:14 AM, Jean-Jacques Hiblot wrote:
> Hi Jacek,
>
> On 20/09/2019 23:10, Jacek Anaszewski wrote:
>> Hi Jean,
>>
>> On 9/20/19 2:25 PM, Jean-Jacques Hiblot wrote:
>>> Making led_set_brightness_sync() use led_set_brightness_nosleep() has 2
>>> advantages:
>>> - works for LED controllers that do not provide
>>> brightness_set_blocking()
>>> - When the blocking callback is used, it uses the workqueue to update
>>> the
>>> LED state, removing the need for mutual exclusion between
>>> led_set_brightness_sync() and set_brightness_delayed().
>> And third:
>>
>> - it compromises the "sync" part of the function name :-)
>
> Making it sync is the role of the flush_work() function. It waits until
> the deferred work has been done.
The thing is not in the blocking character of the function, but rather
in the fastest possible way of setting torch brightness.
led_set_brightness_nosleep() will defer brightness_set_blocking op
to the workqueue so this condition will not be met then.
This function was added specifically for LED class flash v4l2 wrapper:
drivers/media/v4l2-core/v4l2-flash-led-class.c.
It may need an addition of support for brightness_set only drivers,
but we haven't had a use case so far, since all client flash LED
controllers are driven via blocking buses (there are not many of them).
Also, when LED flash class (and thus LED class also as a parent)
is hijacked by v4l2-flash-led wrapper, its sysfs is disabled,
so it is not possible to set e.g. timer trigger which could
interfere with the led_set_brightness_sync() (and it also returns
-EBUSY when blinking is enabled).
>> This function has been introduced specifically to be blocking
>> and have the immediate effect. Its sole client is
>> drivers/media/v4l2-core/v4l2-flash-led-class.c.
>>
>>> Signed-off-by: Jean-Jacques Hiblot <[email protected]>
>>> ---
>>> drivers/leds/led-core.c | 12 +++++++-----
>>> 1 file changed, 7 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
>>> index f1f718dbe0f8..50e28a8f9357 100644
>>> --- a/drivers/leds/led-core.c
>>> +++ b/drivers/leds/led-core.c
>>> @@ -294,15 +294,17 @@ EXPORT_SYMBOL_GPL(led_set_brightness_nosleep);
>>> int led_set_brightness_sync(struct led_classdev *led_cdev,
>>> enum led_brightness value)
>>> {
>>> + int ret;
>>> +
>>> if (led_cdev->blink_delay_on || led_cdev->blink_delay_off)
>>> return -EBUSY;
>>> - led_cdev->brightness = min(value, led_cdev->max_brightness);
>>> -
>>> - if (led_cdev->flags & LED_SUSPENDED)
>>> - return 0;
>>> + ret = led_set_brightness_nosleep(led_cdev, value);
>>> + if (!ret)
>>> + return ret;
>>> - return __led_set_brightness_blocking(led_cdev,
>>> led_cdev->brightness);
>>> + flush_work(&led_cdev->set_brightness_work);
>>> + return 0;
>>> }
>>> EXPORT_SYMBOL_GPL(led_set_brightness_sync);
>>>
>
--
Best regards,
Jacek Anaszewski
On 23/09/2019 23:03, Jacek Anaszewski wrote:
> Hi Jean,
>
> On 9/23/19 11:14 AM, Jean-Jacques Hiblot wrote:
>> Hi Jacek,
>>
>> On 20/09/2019 23:10, Jacek Anaszewski wrote:
>>> Hi Jean,
>>>
>>> On 9/20/19 2:25 PM, Jean-Jacques Hiblot wrote:
>>>> Making led_set_brightness_sync() use led_set_brightness_nosleep() has 2
>>>> advantages:
>>>> - works for LED controllers that do not provide
>>>> brightness_set_blocking()
>>>> - When the blocking callback is used, it uses the workqueue to update
>>>> the
>>>> LED state, removing the need for mutual exclusion between
>>>> led_set_brightness_sync() and set_brightness_delayed().
>>> And third:
>>>
>>> - it compromises the "sync" part of the function name :-)
>> Making it sync is the role of the flush_work() function. It waits until
>> the deferred work has been done.
> The thing is not in the blocking character of the function, but rather
> in the fastest possible way of setting torch brightness.
> led_set_brightness_nosleep() will defer brightness_set_blocking op
> to the workqueue so this condition will not be met then.
OK. I see the point there.
>
> This function was added specifically for LED class flash v4l2 wrapper:
> drivers/media/v4l2-core/v4l2-flash-led-class.c.
>
> It may need an addition of support for brightness_set only drivers,
> but we haven't had a use case so far, since all client flash LED
> controllers are driven via blocking buses (there are not many of them).
>
> Also, when LED flash class (and thus LED class also as a parent)
> is hijacked by v4l2-flash-led wrapper, its sysfs is disabled,
> so it is not possible to set e.g. timer trigger which could
> interfere with the led_set_brightness_sync() (and it also returns
> -EBUSY when blinking is enabled).
Then this is a really special use case and we don't really have to
worry about synchronization with the other ways to set the LED
brightness. I'll drop any change to this function then.
Thanks for the detailed explanation.
JJ
>
>>> This function has been introduced specifically to be blocking
>>> and have the immediate effect. Its sole client is
>>> drivers/media/v4l2-core/v4l2-flash-led-class.c.
>>>
>>>> Signed-off-by: Jean-Jacques Hiblot <[email protected]>
>>>> ---
>>>> drivers/leds/led-core.c | 12 +++++++-----
>>>> 1 file changed, 7 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
>>>> index f1f718dbe0f8..50e28a8f9357 100644
>>>> --- a/drivers/leds/led-core.c
>>>> +++ b/drivers/leds/led-core.c
>>>> @@ -294,15 +294,17 @@ EXPORT_SYMBOL_GPL(led_set_brightness_nosleep);
>>>> int led_set_brightness_sync(struct led_classdev *led_cdev,
>>>> enum led_brightness value)
>>>> {
>>>> + int ret;
>>>> +
>>>> if (led_cdev->blink_delay_on || led_cdev->blink_delay_off)
>>>> return -EBUSY;
>>>> - led_cdev->brightness = min(value, led_cdev->max_brightness);
>>>> -
>>>> - if (led_cdev->flags & LED_SUSPENDED)
>>>> - return 0;
>>>> + ret = led_set_brightness_nosleep(led_cdev, value);
>>>> + if (!ret)
>>>> + return ret;
>>>> - return __led_set_brightness_blocking(led_cdev,
>>>> led_cdev->brightness);
>>>> + flush_work(&led_cdev->set_brightness_work);
>>>> + return 0;
>>>> }
>>>> EXPORT_SYMBOL_GPL(led_set_brightness_sync);
>>>>