Re: [PATCH v2 0/3] leds: blink resolution improvements

[Jacek Anaszewski]

On 05/05/2015 03:02 PM, Stas Sergeev wrote:
> 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.

Yes, this needs to be fixed.

> > > > > 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.

There can be indirect correlation.

> > 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.

If you have a strong belief that it is possible to implement this
feature in a manner acceptable for everyone, feel free to experiment
with the implementation. If people will find it useful and reliable
then we will merge it.

--
Best Regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/