| Message ID | 1405087464-13762-7-git-send-email-j.anaszewski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
Hi Sakari, 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. Best Regards, Jacek Anaszewski -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. 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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Bryan and Richard, Your opinion would be much appreciated to a question myself and Jacek were pondering. Please see below. On Thu, Aug 07, 2014 at 03:12:09PM +0200, Jacek Anaszewski wrote: > 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. Ping. > >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.
On Thu, 2014-08-14 at 07:39 +0300, Sakari Ailus wrote: > Bryan and Richard, > > Your opinion would be much appreciated to a question myself and Jacek were > pondering. Please see below. > > On Thu, Aug 07, 2014 at 03:12:09PM +0200, Jacek Anaszewski wrote: > > 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. > > Ping. I'm not a fan of forcing everything to the lowest common denominator. At a basic level an LED is a binary on/off so the shear levels of complexity we end up going through is kind of scary. Forcing everything through a workqueue due to their being some hardware out there can can't do it in interrupt context is kind of sad and wasn't where the API set out from. So personally I'd prefer not to see the API changed like that however I appreciate I've been more distant from the code of late. Cheers, Richard -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Richard, Richard Purdie wrote: > On Thu, 2014-08-14 at 07:39 +0300, Sakari Ailus wrote: >> Bryan and Richard, >> >> Your opinion would be much appreciated to a question myself and Jacek were >> pondering. Please see below. >> >> On Thu, Aug 07, 2014 at 03:12:09PM +0200, Jacek Anaszewski wrote: >>> 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. >> >> Ping. > > I'm not a fan of forcing everything to the lowest common denominator. At > a basic level an LED is a binary on/off so the shear levels of > complexity we end up going through is kind of scary. Forcing everything > through a workqueue due to their being some hardware out there can can't > do it in interrupt context is kind of sad and wasn't where the API set > out from. The discussion has been centered around devices that do actually need that work queue for sleepless operation. Naturally, if the work queue isn't required by a particular device, it should not be used. What we'd prefer to avoid here is drivers having to implement two ways (synchronoys and potentially asynchronous) to perform the same actions (for V4L2 and LED API, respectively).
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c index a96a1a7..c17dda0 100644 --- a/drivers/leds/led-class.c +++ b/drivers/leds/led-class.c @@ -44,15 +44,29 @@ static ssize_t brightness_store(struct device *dev, mutex_lock(&led_cdev->led_lock); + /* + * Having LED_DEV_CAP_TORCH implies this is LED Flash Class + * device and we need to check sysfs accessibility. + */ + if (led_cdev->flags & LED_DEV_CAP_TORCH) { + if (led_sysfs_is_locked(led_cdev)) { + ret = -EBUSY; + goto unlock; + } + } + ret = kstrtoul(buf, 10, &state); if (ret) goto unlock; if (state == LED_OFF) led_trigger_remove(led_cdev); - __led_set_brightness(led_cdev, state); - return size; + if (led_cdev->flags & LED_DEV_CAP_TORCH) + ret = led_set_torch_brightness(led_cdev, state); + else + __led_set_brightness(led_cdev, state); + ret = size; unlock: mutex_unlock(&led_cdev->led_lock); diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c index d156fb6..0ce087a 100644 --- a/drivers/leds/led-core.c +++ b/drivers/leds/led-core.c @@ -143,6 +143,21 @@ int led_update_brightness(struct led_classdev *led_cdev) return ret; } EXPORT_SYMBOL(led_update_brightness); + +int led_set_torch_brightness(struct led_classdev *led_cdev, + enum led_brightness brightness) +{ + int ret = 0; + + led_cdev->brightness = min(brightness, led_cdev->max_brightness); + + if (!(led_cdev->flags & LED_SUSPENDED)) + ret = led_cdev->torch_brightness_set(led_cdev, + led_cdev->brightness); + return ret; +} +EXPORT_SYMBOL_GPL(led_set_torch_brightness); + /* Caller must ensure led_cdev->led_lock held */ void led_sysfs_lock(struct led_classdev *led_cdev) { 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. + */ + int (*torch_brightness_set)(struct led_classdev *led_cdev, + enum led_brightness brightness); /* Get LED brightness level */ enum led_brightness (*brightness_get)(struct led_classdev *led_cdev); @@ -156,6 +166,18 @@ extern void led_set_brightness(struct led_classdev *led_cdev, */ extern int led_update_brightness(struct led_classdev *led_cdev); +/** + * led_set_torch_brightness - set torch LED brightness + * @led_cdev: the LED to set + * @brightness: the brightness to set it to + * + * Returns: 0 on success or negative error value on failure + * + * Set a torch LED's brightness. + */ +extern int led_set_torch_brightness(struct led_classdev *led_cdev, + enum led_brightness brightness); +/** * led_sysfs_lock - lock LED sysfs interface * @led_cdev: the LED to set *