Re: [PATCH/RFC v4 06/21] leds: add API for setting torch brightness
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC v2 1/8] leds: Add sysfs and kernel internal API for flash LEDs
On Fri, 2014-03-28 at 16:28 +0100, Jacek Anaszewski wrote:
Some LED devices support two operation modes - torch and
flash. This patch provides support for flash LED devices
in the LED subsystem by introducing new sysfs attributes
and kernel internal interface. The attributes being
introduced are: flash_brightness, flash_strobe, flash_timeout,
max_flash_timeout, max_flash_brightness, flash_fault and
hw_triggered. All the flash related features are placed
in a separate module.
The modifications aim to be compatible with V4L2 framework
requirements related to the flash devices management. The
design assumes that V4L2 sub-device can take of the LED class
device control and communicate with it through the kernel
internal interface. The LED sysfs interface is made
unavailable then.
Signed-off-by: Jacek Anaszewski j.anaszew...@samsung.com
Acked-by: Kyungmin Park kyungmin.p...@samsung.com
Cc: Bryan Wu coolo...@gmail.com
Cc: Richard Purdie rpur...@rpsys.net
---
drivers/leds/Kconfig|8 +
drivers/leds/Makefile |1 +
drivers/leds/led-class.c| 56 +--
drivers/leds/led-flash.c| 375
+++
drivers/leds/led-triggers.c | 16 +-
drivers/leds/leds.h |3 +
include/linux/leds.h| 24 ++-
include/linux/leds_flash.h | 189 ++
8 files changed, 658 insertions(+), 14 deletions(-)
create mode 100644 drivers/leds/led-flash.c
create mode 100644 include/linux/leds_flash.h
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 2062682..1e1c81f 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -19,6 +19,14 @@ config LEDS_CLASS
This option enables the led sysfs class in /sys/class/leds. You'll
need this to do anything useful with LEDs. If unsure, say N.
+config LEDS_CLASS_FLASH
+ tristate Flash LEDs Support
+ depends on LEDS_CLASS
+ help
+ This option enables support for flash LED devices. Say Y if you
+ want to use flash specific features of a LED device, if they
+ are supported.
+
comment LED drivers
config LEDS_88PM860X
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 3cd76db..8861b86 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -2,6 +2,7 @@
# LED Core
obj-$(CONFIG_NEW_LEDS) += led-core.o
obj-$(CONFIG_LEDS_CLASS) += led-class.o
+obj-$(CONFIG_LEDS_CLASS_FLASH) += led-flash.o
obj-$(CONFIG_LEDS_TRIGGERS) += led-triggers.o
# LED Platform Drivers
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index f37d63c..5bac140 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -9,16 +9,18 @@
* published by the Free Software Foundation.
*/
-#include linux/module.h
-#include linux/kernel.h
+#include linux/ctype.h
+#include linux/device.h
+#include linux/err.h
#include linux/init.h
+#include linux/kernel.h
#include linux/list.h
+#include linux/module.h
+#include linux/slab.h
#include linux/spinlock.h
-#include linux/device.h
#include linux/timer.h
-#include linux/err.h
-#include linux/ctype.h
#include linux/leds.h
+#include linux/leds_flash.h
#include leds.h
static struct class *leds_class;
@@ -45,28 +47,38 @@ static ssize_t brightness_store(struct device *dev,
{
struct led_classdev *led_cdev = dev_get_drvdata(dev);
unsigned long state;
- ssize_t ret = -EINVAL;
+ ssize_t ret;
+
+ mutex_lock(led_cdev-led_lock);
+
+ if (led_sysfs_is_locked(led_cdev)) {
+ ret = -EBUSY;
+ goto unlock;
+ }
ret = kstrtoul(buf, 10, state);
if (ret)
- return ret;
+ goto unlock;
if (state == LED_OFF)
led_trigger_remove(led_cdev);
__led_set_brightness(led_cdev, state);
+ ret = size;
- return size;
+unlock:
+ mutex_unlock(led_cdev-led_lock);
+ return ret;
}
static DEVICE_ATTR_RW(brightness);
-static ssize_t led_max_brightness_show(struct device *dev,
+static ssize_t max_brightness_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
struct led_classdev *led_cdev = dev_get_drvdata(dev);
return sprintf(buf, %u\n, led_cdev-max_brightness);
}
-static DEVICE_ATTR(max_brightness, 0444, led_max_brightness_show, NULL);
+static DEVICE_ATTR_RO(max_brightness);
#ifdef CONFIG_LEDS_TRIGGERS
static DEVICE_ATTR(trigger, 0644, led_trigger_show, led_trigger_store);
@@ -173,7 +185,15 @@ EXPORT_SYMBOL_GPL(led_classdev_suspend);
*/
void led_classdev_resume(struct led_classdev *led_cdev)
{
+ struct led_flash *flash = led_cdev-flash;
+
led_cdev-brightness_set(led_cdev, led_cdev-brightness);
+ if (flash) {
+ call_flash_op(brightness_set, led_cdev,
+ flash
Re: [PATCH/RFC 1/8] leds: Add sysfs and kernel internal API for flash LEDs
On Thu, 2014-03-20 at 15:51 +0100, Jacek Anaszewski wrote: Some LED devices support two operation modes - torch and flash. This patch provides support for flash LED devices in the LED subsystem by introducing new sysfs attributes and kernel internal interface. The attributes being introduced are: flash_mode, flash_timeout, max_flash_timeout, flash_fault and hw_triggered. The modifications aim to be compatible with V4L2 framework requirements related to the flash devices management. The design assumes that V4L2 driver can take of the LED class device control and communicate with it through the kernel internal interface. The LED sysfs interface is made unavailable then. Signed-off-by: Jacek Anaszewski j.anaszew...@samsung.com Acked-by: Kyungmin Park kyungmin.p...@samsung.com Cc: Bryan Wu coolo...@gmail.com Cc: Richard Purdie rpur...@rpsys.net --- drivers/leds/led-class.c| 216 +-- drivers/leds/led-core.c | 124 +++-- drivers/leds/led-triggers.c | 17 +++- drivers/leds/leds.h |9 ++ include/linux/leds.h| 136 +++ 5 files changed, 486 insertions(+), 16 deletions(-) It seems rather sad to have to insert that amount of code into the core LED files for something which only a small number of LEDs actually use. This will increase the footprint of the core LED code significantly. Is it not possible to add this as a module/extension to the LED core rather than completely entangling them? Cheers, Richard -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC, PATCH 1/3] gspca: add LEDs subsystem connection
On Tue, 2010-03-09 at 12:27 +0100, Laurent Pinchart wrote: Hi Màrton, Thanks for the patch. On Wednesday 03 March 2010 00:42:23 Németh Márton wrote: From: Márton Németh nm...@freemail.hu On some webcams one or more LEDs can be found. One type of these LEDs are feedback LEDs: they usually shows the state of streaming mode. The LED can be programmed to constantly switched off state (e.g. for power saving reasons, preview mode) or on state (e.g. the application shows motion detection or on-air). The second type of LEDs are used to create enough light for the sensor for example visible or in infra-red light. Both type of these LEDs can be handled using the LEDs subsystem. This patch add support to connect a gspca based driver to the LEDs subsystem. They can indeed, but I'm not sure if the LEDs subsystem was designed for that kind of use cases. The LED subsystem was designed to support LED lights and the above scenarios can certainly fit that. It provides a nice system where default use cases should just work (power light on a laptop) but the system has the control to override than and do other interesting things with it if it so wishes. The LED framework was developed to handle LEDs found in embedded systems (usually connected to GPIOs) that needed to be connected to software triggers or controlled by drivers and/or specific userspace applications. Its used by many laptops so its not just embedded systems. Webcam LEDs seem a bit out of scope to me, especially the light LED that might be better handled by a V4L2 set of controls (we're currently missing controls for camera flashes, be they LEDs or Xenon based). I'll let Richard speak on this. I'm not going to push one way or another and its up to individual subsystems to way up the benefits and drawbacks and make their decision. There is nothing in the design of the system which would stop it working or being used in this case though. If the susystsme needs improvements to work well, I'm happy to accept patches too. Cheers, Richard -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
