lkml.org 
[lkml]   [2015]   [May]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 0/3] leds: blink resolution improvements
04.05.2015 18:22, Jacek Anaszewski пишет:
> On 05/04/2015 02:12 PM, Stas Sergeev wrote:
>> Only under that condition:
>> ---
>> if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) {
>> led_cdev->delayed_set_value = brightness;
>> schedule_work(&led_cdev->set_brightness_work);
>> ---
>>
>> But the main condition is:
>> ---
>> if (led_cdev->flags & SET_BRIGHTNESS_ASYNC) {
>> led_set_brightness_async(led_cdev, brightness);
>> ---
>>
>> So I think it is actually unused.
>> I don't see why schedule_work() above can't be just replaced
>> with led_set_brightness_async(). Is there the reason not to do so?
> set_brightness_work not only sets the brightness but also
> stops software blinking, which was the primary reason
> for adding this work queue I think. Here is the commit message:
But led_trigger_set() does led_stop_software_blink(), which
IMHO means led_set_brightness() will in most cases be called
when sw blocking is already stopped. There seem to be just a
few cases where this is not true: oneshot_trig_deactivate() and
timer_trig_deactivate(), and I think I'll just change these two to
led_stop_software_blink(). I am pretty sure the work-queue is
not needed, but I'll have to test that with the patch it seems.

> ------------------------
>
> leds: delay led_set_brightness if stopping soft-blink
>
> Delay execution of led_set_brightness() if need to stop soft-blink
> timer.
>
> This allows led_set_brightness to be called in hard-irq context
> even if
> soft-blink was activated on that LED.
Instead of disabling the soft-blink beforehand, which is what
led_trigger_set()
already does? I am probably missing something.

> > Now your leds-aat1290 already asks for such a change,
>> because it can sleep but does not use a work-queue the
>> way other drivers do.
> It doesn't need this change - it defines two ops: brightness_set
> (the async one) and brightness_set_sync (the sync one). The
> former is called from led_set_brightness_async and the latter
> form led_set_brightness_sync.
> led_set_brightness_async is called from led_set_brightness
> for drivers that define SET_BRIGHTNESS_ASYNC flag and
> led_set_brightness_sync for the drivers that define
> SET_BRIGHTNESS_SYNC flags.
>
> led_timer_function calls always led_set_brightness_async.
OK, I googled the patch:
https://lkml.org/lkml/2015/3/4/960
So the async one uses the work-queue, and the sync one
does not. Since led_timer_function calls always led_set_brightness_async,
it should always be using a work-queue.
But then I fail to explain your diagnostic that with my patch and
your driver, the hrtimer gives warning about a high interrupt
latency. I thought this is because your driver does sleeps and
does not use a work queue. Its not the case. Could you please
clarify, what then caused the high interrupt latency warning in
your testing?

>> So what should we do?
>> I can try the aforementioned massive clean-up with removing
>> the work-queue from every driver and using the one in
>> led-core, but my attempts have few chances to succeed
>> because of no test-cases. Or can you do this instead, so
>> that leds-aat1290 driver is in line with the others? Or any
>> other options I can try?
>>
> It would have to be done for the LED core and all drivers
> in one patch set. We would have to get acks from all LED drivers'
> authors (or at least from majority of them).
>
> Once this is done we could think about adding optional hr timers
> based triggers and invite people for testing.
As long as all drivers use the work-queue when needed and
there is no warning about high interrupt latency, I wonder if
there are some short-cuts to that route. :)
But I first need to understand where this latency came from.


\
 
 \ /
  Last update: 2015-05-04 19:41    [W:0.081 / U:0.356 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site