05.05.2015 11:22, Jacek Anaszewski пишет:
On 05/04/2015 07:20 PM, Stas Sergeev wrote:
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.
It is used e.g. in the following case:
#echo "timer" > trigger
#echo 1 > brightness
Indeed, thanks.
I'll study that case next week when my board is back to me.
Looking at sources, it seems in that case it would disable the
software blinking (del_timer_sync()) without changing the
trigger back to "none", which does not make sense to me.
> 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?
An accurate explanation would require thorough investigation.
It can be related to the fact that the driver uses delays.
Even if your driver just does schedule_work() and nothing
more in an async method? Strange.
In the first place we have to take into account that Linux is not
a real time operating system. The feature you're trying to implement
is realized by hardware with use of pwm. There might be narrow group
of drivers that could benefit from it in specific circumstances
(the system couldn't be too busy at the time when timer trigger is
running), but this is too weak argument in favour of supporting small
delay intervals.
If you mean the drivers that don't have any sleeps, then the
system load is irrelevant because the hrtimer callback is AFAIK
running in an irq context. So for them it would be a clear win,
not just in a specific circumstances. Of course I wonder if it is
only leds-gpio, or anything else too. :) Though I could suspect
that leds-gpio have a very wide usage, and it may worth the
troubles even to improve just leds-gpio alone.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
