lkml.org 
[lkml]   [2014]   [Aug]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH/RFC v4 06/21] leds: add API for setting torch brightness
Hi Sakari,

On 08/04/2014 02:50 PM, Sakari Ailus wrote:
> Hi Jacek,
>
> Thank you for your continued efforts on this!
>
> On Mon, Aug 04, 2014 at 02:35:26PM +0200, Jacek Anaszewski wrote:
>> On 07/16/2014 11:54 PM, Sakari Ailus wrote:
>>> Hi Jacek,
>>>
>>> Jacek Anaszewski wrote:
>>> ...
>>>> diff --git a/include/linux/leds.h b/include/linux/leds.h
>>>> index 1a130cc..9bea9e6 100644
>>>> --- a/include/linux/leds.h
>>>> +++ b/include/linux/leds.h
>>>> @@ -44,11 +44,21 @@ struct led_classdev {
>>>> #define LED_BLINK_ONESHOT_STOP (1 << 18)
>>>> #define LED_BLINK_INVERT (1 << 19)
>>>> #define LED_SYSFS_LOCK (1 << 20)
>>>> +#define LED_DEV_CAP_TORCH (1 << 21)
>>>>
>>>> /* Set LED brightness level */
>>>> /* Must not sleep, use a workqueue if needed */
>>>> void (*brightness_set)(struct led_classdev *led_cdev,
>>>> enum led_brightness brightness);
>>>> + /*
>>>> + * Set LED brightness immediately - it is required for flash led
>>>> + * devices as they require setting torch brightness to have
>>>> immediate
>>>> + * effect. brightness_set op cannot be used for this purpose because
>>>> + * the led drivers schedule a work queue task in it to allow for
>>>> + * being called from led-triggers, i.e. from the timer irq context.
>>>> + */
>>>
>>> Do we need to classify actual devices based on this? I think it's rather
>>> a different API behaviour between the LED and the V4L2 APIs.
>>>
>>> On devices that are slow to control, the behaviour should be asynchronous
>>> over the LED API and synchronous when accessed through the V4L2 API. How
>>> about implementing the work queue, as I have suggested, in the
>>> framework, so
>>> that individual drivers don't need to care about this and just implement
>>> the
>>> synchronous variant of this op? A flag could be added to distinguish
>>> devices
>>> that are fast so that the work queue isn't needed.
>>>
>>> It'd be nice to avoid individual drivers having to implement multiple
>>> ops to
>>> do the same thing, just for differing user space interfacs.
>>>
>>
>> It is not only the matter of a device controller speed. If a flash
>> device is to be made accessible from the LED subsystem, then it
>> should be also compatible with led-triggers. Some of led-triggers
>> call brightness_set op from the timer irq context and thus no
>> locking in the callback can occur. This requirement cannot be
>> met i.e. if i2c bus is to be used. This is probably the primary
>> reason for scheduling work queue tasks in brightness_set op.
>>
>> Having the above in mind, setting a brightness in a work queue
>> task must be possible for all LED Class Flash drivers, regardless
>> whether related devices have fast or slow controller.
>>
>> Let's recap the cost of possible solutions then:
>>
>> 1) Moving the work queues to the LED framework
>>
>> - it would probably require extending led_set_brightness and
>> __led_set_brightness functions by a parameter indicating whether it
>> should call brightness_set op in the work queue task or directly;
>> - all existing triggers would have to be updated accordingly;
>> - work queues would have to be removed from all the LED drivers;
>>
>> 2) adding led_set_torch_brightness API
>>
>> - no modifications in existing drivers and triggers would be required
>> - instead, only the modifications from the discussed patch would
>> be required
>>
>> Solution 1 looks cleaner but requires much more modifications.
>
> How about a combination of the two, i.e. option 1 with the old op remaining
> there for compatibility with the old drivers (with a comment telling it's
> deprecated)?
>
> This way new drivers will benefit from having to implement this just once,
> and modifications to the existing drivers could be left for later.

It's OK for me, but the opinion from the LED side guys is needed here
as well.

> The downside is that any old drivers wouldn't get V4L2 flash API but that's
> entirely acceptable in my opinion since these would hardly be needed in use
> cases that would benefit from V4L2 flash API.

In the version 4 of the patch set I changed the implementation, so that
a flash led driver must call led_classdev_flash_register to get
registered as a LED Flash Class device and v4l2_flash_init to get
V4L2 Flash API. In effect old drivers will have no chance to get V4L2
Flash API either way.

Best Regards,
Jacek Anaszewski


\
 
 \ /
  Last update: 2014-08-07 15:21    [W:0.153 / U:0.700 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site