[RFC,1/3] gspca: add LEDs subsystem connection

Message ID 4B8DA25F.10602@freemail.hu (mailing list archive)
State RFC, archived
Headers

Commit Message

Németh Márton March 2, 2010, 11:42 p.m. UTC
  From: Márton Németh <nm127@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.

Signed-off-by: Márton Németh <nm127@freemail.hu>
---
--
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
  

Comments

Laurent Pinchart March 9, 2010, 11:27 a.m. UTC | #1
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 <nm127@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 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. 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.
  
Richard Purdie March 9, 2010, 4:02 p.m. UTC | #2
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 <nm127@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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Patch

diff -r d8fafa7d88dc linux/drivers/media/video/gspca/gspca.h
--- a/linux/drivers/media/video/gspca/gspca.h	Thu Feb 18 19:02:51 2010 +0100
+++ b/linux/drivers/media/video/gspca/gspca.h	Wed Mar 03 00:10:19 2010 +0100
@@ -7,6 +7,8 @@ 
 #include <linux/videodev2.h>
 #include <media/v4l2-common.h>
 #include <linux/mutex.h>
+#include <linux/leds.h>
+#include <linux/workqueue.h>
 #include "compat.h"

 /* compilation option */
@@ -53,14 +55,41 @@ 
 	int nrates;
 };

+#if defined(CONFIG_LEDS_CLASS) || defined(CONFIG_LEDS_CLASS_MODULE)
+/**
+ * struct gspca_led - data structure for one LED on a camera
+ * @led_cdev: the class device for the LED
+ * @name: place for the LED name which will appear under /sys/class/leds/
+ * @led_trigger: the trigger structure for the same LED
+ * @trigger_name: place for the trigger name which will appear in the file
+ *                /sys/class/leds/{led name}/trigger
+ * @parent: pointer to the parent gspca_dev
+ */
+struct gspca_led {
+	struct led_classdev led_cdev;
+	char name[32];
+#ifdef CONFIG_LEDS_TRIGGERS
+	struct led_trigger led_trigger;
+	char trigger_name[32];
+#endif
+	struct gspca_dev *parent;
+};
+#endif
+
 /* device information - set at probe time */
 struct cam {
 	const struct v4l2_pix_format *cam_mode;	/* size nmodes */
 	const struct framerates *mode_framerates; /* must have size nmode,
 						   * just like cam_mode */
+#if defined(CONFIG_LEDS_CLASS) || defined(CONFIG_LEDS_CLASS_MODULE)
+	struct gspca_led *leds;
+#endif
 	u32 bulk_size;		/* buffer size when image transfer by bulk */
 	u32 input_flags;	/* value for ENUM_INPUT status flags */
 	u8 nmodes;		/* size of cam_mode */
+#if defined(CONFIG_LEDS_CLASS) || defined(CONFIG_LEDS_CLASS_MODULE)
+	u8 nleds;		/* number of LEDs */
+#endif
 	u8 no_urb_create;	/* don't create transfer URBs */
 	u8 bulk_nurbs;		/* number of URBs in bulk mode
 				 * - cannot be > MAX_NURBS
diff -r d8fafa7d88dc linux/drivers/media/video/gspca/gspca.c
--- a/linux/drivers/media/video/gspca/gspca.c	Thu Feb 18 19:02:51 2010 +0100
+++ b/linux/drivers/media/video/gspca/gspca.c	Wed Mar 03 00:10:19 2010 +0100
@@ -4,6 +4,7 @@ 
  * Copyright (C) 2008-2009 Jean-Francois Moine (http://moinejf.free.fr)
  *
  * Camera button input handling by Márton Németh
+ * LED subsystem connection by Márton Németh
  * Copyright (C) 2009-2010 Márton Németh <nm127@freemail.hu>
  *
  * This program is free software; you can redistribute it and/or modify it
@@ -2256,6 +2257,76 @@ 
 	.release = gspca_release,
 };

+#if defined(CONFIG_LEDS_CLASS) || defined(CONFIG_LEDS_CLASS_MODULE)
+/**
+ * gspca_create_name() - Create a name using format string and a number
+ * @name: format string on input, e.g. "video%d::feedback". The format string
+ *        may contain one and only one %d format specifier. The created name
+ *        will be put on the same location and the original format string will
+ *        be overwritten.
+ * @length: the length of the buffer in bytes pointed by the name parameter
+ * @num: the number to use when creating the name
+ *
+ * Returns zero on success.
+ */
+static int gspca_create_name(char *name, unsigned int length, int num)
+{
+	char buffer[32];
+	unsigned int l;
+
+	/* TODO: check format string: it shall contain one and only one %d */
+
+	l = min(length, sizeof(buffer));
+	snprintf(buffer, l, name, num);
+	strlcpy(name, buffer, l);
+
+	return 0;
+}
+
+static void gspca_leds_register(struct gspca_dev *gspca_dev) {
+	int i;
+	int ret;
+
+	for (i = 0; i < gspca_dev->cam.nleds; i++) {
+#ifdef CONFIG_LEDS_TRIGGERS
+		gspca_create_name(gspca_dev->cam.leds[i].trigger_name,
+				  sizeof(gspca_dev->cam.leds[i].trigger_name),
+				  gspca_dev->vdev.num);
+		gspca_dev->cam.leds[i].led_trigger.name = gspca_dev->cam.leds[i].trigger_name;
+		gspca_dev->cam.leds[i].led_cdev.default_trigger = gspca_dev->cam.leds[i].trigger_name;
+#endif
+		gspca_create_name(gspca_dev->cam.leds[i].name,
+				  sizeof(gspca_dev->cam.leds[i].name),
+				  gspca_dev->vdev.num);
+
+		gspca_dev->cam.leds[i].led_cdev.name = gspca_dev->cam.leds[i].name;
+		gspca_dev->cam.leds[i].led_cdev.blink_set = NULL;
+		gspca_dev->cam.leds[i].led_cdev.flags = LED_CORE_SUSPENDRESUME;
+		ret = led_classdev_register(&gspca_dev->dev->dev,
+					    &gspca_dev->cam.leds[i].led_cdev);
+		if (!ret) {
+#ifdef CONFIG_LEDS_TRIGGERS
+			led_trigger_register(&gspca_dev->cam.leds[i].led_trigger);
+#endif
+		}
+	}
+}
+
+static void gspca_leds_unregister(struct gspca_dev *gspca_dev)
+{
+	int i;
+	for (i = 0; i < gspca_dev->cam.nleds; i++) {
+#ifdef CONFIG_LEDS_TRIGGERS
+		led_trigger_unregister(&gspca_dev->cam.leds[i].led_trigger);
+#endif
+		led_classdev_unregister(&gspca_dev->cam.leds[i].led_cdev);
+	}
+}
+#else
+#define gspca_leds_register(gspca_dev)
+#define gspca_leds_disconnect(gspca_dev)
+#endif
+
 /*
  * probe and create a new gspca device
  *
@@ -2342,6 +2413,8 @@ 
 	if (ret == 0)
 		ret = gspca_input_create_urb(gspca_dev);

+	gspca_leds_register(gspca_dev);
+
 	return 0;
 out:
 	kfree(gspca_dev->usb_buf);
@@ -2386,6 +2459,8 @@ 
 	gspca_dev->dev = NULL;
 	mutex_unlock(&gspca_dev->usb_lock);

+	gspca_leds_unregister(gspca_dev);
+
 	usb_set_intfdata(intf, NULL);

 	/* release the device */