Messages in this thread | | | Date | Thu, 07 Aug 2014 15:12:09 +0200 | From | Jacek Anaszewski <> | Subject | Re: [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
| |