Re: [PATCH 01/20] leds: implement LED_BRIGHTNESS_FAST flag

[Stas Sergeev]

01.06.2015 11:31, Jacek Anaszewski ÐÐÑÐÑ:
> With this approach, the LED will remain in its current blink state, in
> case LED_BRIGHTNESS_FAST flag is not set and delay to be set is below LED_SLOW_MIN_PERIOD. This is because timer is deleted at the beginning
> of led_blink_set. Effectively this can result in the LED staying turned
> on when e.g. "echo 1 > delay_on" fails.
> 
> I propose to change this part of code as follows:
> 
>         if (!(led_cdev->flags & LED_BRIGHTNESS_FAST)) {
>         if (delay_on < LED_SLOW_MIN_PERIOD ||
>             delay_off < LED_SLOW_MIN_PERIOD)
>             led_set_brightness_async(led_cdev, LED_OFF);
>             return -ERANGE;
>         }
>     }
Sounds good.

> By this occasion it turned out that we have inconsistency:
> led_set_brightness_async calls brightness_set op, which doesn't
> set LED brightness in an asynchronous way in case of every driver.
> 
> Before introduction of SET_BRIGHTNESS_SYNC and SET_BRIGHTNESS_ASYNC
> flags there was only brightness_set op. The flags and
> brightness_set_sync op was added for flash LED controllers to avoid
> delegating the job to work queue and assure that brightness is set
> as quickly as possible.
> 
> I mistakenly assumed that all drivers set the brightness with use
> of a work queue.
Indeed.

> Now, to fix the issue all drivers that set brightness in the
> asynchronous way need to set the SET_BRIGHTNESS_SYNC flag and rename
> their brightness_set ops to brightness_set_sync.
> It would be also nice to rename brightness_set op to
> brightness_set_async.
> 
> Also, led_set_brightness_async shouldn't be called directly
> anywhere, but led_set_brightness should be used, as it
> selects the appropriate op basing on the SET_BRIGHTNESS_* flag.
> 
> I'd prefer to do these modifications before merging this patch
Sounds good: I wonder if the new flag for FAST drivers will then
be needed at all: maybe it would be enough for the driver to define
a SYNC method, and we assume the SYNC methods are always fast?
In fact, the things are more complicated: some drivers do small
udelay()'s but do not use a work-queue. I was not marking them as
FAST, although perhaps they could still be marked as SYNC?

> set. I can produce the patch set within a few days. Or, maybe you
> would like to take care of it?
I would appreciate if you do.
I very much hate to do any non-trivial changes to the code I can't
even properly test (sometimes not even compile-test!).

Since you are at it, I'd also suggest to kill the work-queue in
led-core.c. Now that we fixed a scenario where it was mistakenly
used, I again think it is not needed for what it was initially advertised.
--
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/