[PATCH 1/3] add feedback LED control

[PATCH 1/3] add feedback LED control

[Németh Márton]

From: Márton Németh <nm127@freemail.hu>

On some webcams a feedback LED can be found. This LED usually shows
the state of streaming mode: this is the "Auto" 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").

Signed-off-by: Márton Németh <nm127@freemail.hu>
---
diff -r d8fafa7d88dc linux/Documentation/DocBook/v4l/controls.xml
--- a/linux/Documentation/DocBook/v4l/controls.xml	Thu Feb 18 19:02:51 2010 +0100
+++ b/linux/Documentation/DocBook/v4l/controls.xml	Sun Feb 28 08:41:17 2010 +0100
@@ -311,6 +311,17 @@
 Applications depending on particular custom controls should check the
 driver name and version, see <xref linkend="querycap" />.</entry>
 	  </row>
+	  <row id="v4l2-led">
+	    <entry><constant>V4L2_CID_LED</constant></entry>
+	    <entry>enum</entry>
+	    <entry>Controls the feedback LED on the camera. In auto mode
+the LED is on when the streaming is active. The LED is off when not streaming.
+The LED can be also turned on and off independent from streaming.
+Possible values for <constant>enum v4l2_led</constant> are:
+<constant>V4L2_CID_LED_AUTO</constant> (0),
+<constant>V4L2_CID_LED_ON</constant> (1) and
+<constant>V4L2_CID_LED_OFF</constant> (2).</entry>
+	  </row>
 	</tbody>
       </tgroup>
     </table>
diff -r d8fafa7d88dc linux/Documentation/DocBook/v4l/videodev2.h.xml
--- a/linux/Documentation/DocBook/v4l/videodev2.h.xml	Thu Feb 18 19:02:51 2010 +0100
+++ b/linux/Documentation/DocBook/v4l/videodev2.h.xml	Sun Feb 28 08:41:17 2010 +0100
@@ -1024,8 +1024,14 @@

 #define V4L2_CID_ROTATE                         (V4L2_CID_BASE+34)
 #define V4L2_CID_BG_COLOR                       (V4L2_CID_BASE+35)
+#define V4L2_CID_LED                            (V4L2_CID_BASE+36)
+enum v4l2_led {
+        V4L2_LED_AUTO           = 0,
+        V4L2_LED_ON             = 1,
+        V4L2_LED_OFF            = 2,
+};
 /* last CID + 1 */
-#define V4L2_CID_LASTP1                         (V4L2_CID_BASE+36)
+#define V4L2_CID_LASTP1                         (V4L2_CID_BASE+37)

 /*  MPEG-class control IDs defined by V4L2 */
 #define V4L2_CID_MPEG_BASE                      (V4L2_CTRL_CLASS_MPEG | 0x900)
diff -r d8fafa7d88dc linux/drivers/media/video/v4l2-common.c
--- a/linux/drivers/media/video/v4l2-common.c	Thu Feb 18 19:02:51 2010 +0100
+++ b/linux/drivers/media/video/v4l2-common.c	Sun Feb 28 08:41:17 2010 +0100
@@ -349,6 +349,12 @@
 		"75 useconds",
 		NULL,
 	};
+	static const char *led[] = {
+		"Auto",
+		"On",
+		"Off",
+		NULL,
+	};

 	switch (id) {
 		case V4L2_CID_MPEG_AUDIO_SAMPLING_FREQ:
@@ -389,6 +395,8 @@
 			return colorfx;
 		case V4L2_CID_TUNE_PREEMPHASIS:
 			return tune_preemphasis;
+		case V4L2_CID_LED:
+			return led;
 		default:
 			return NULL;
 	}
@@ -434,6 +442,7 @@
 	case V4L2_CID_COLORFX:			return "Color Effects";
 	case V4L2_CID_ROTATE:			return "Rotate";
 	case V4L2_CID_BG_COLOR:			return "Background color";
+	case V4L2_CID_LED:			return "Feedback LED";

 	/* MPEG controls */
 	case V4L2_CID_MPEG_CLASS: 		return "MPEG Encoder Controls";
@@ -575,6 +584,7 @@
 	case V4L2_CID_EXPOSURE_AUTO:
 	case V4L2_CID_COLORFX:
 	case V4L2_CID_TUNE_PREEMPHASIS:
+	case V4L2_CID_LED:
 		qctrl->type = V4L2_CTRL_TYPE_MENU;
 		step = 1;
 		break;
diff -r d8fafa7d88dc linux/include/linux/videodev2.h
--- a/linux/include/linux/videodev2.h	Thu Feb 18 19:02:51 2010 +0100
+++ b/linux/include/linux/videodev2.h	Sun Feb 28 08:41:17 2010 +0100
@@ -1030,8 +1030,14 @@

 #define V4L2_CID_ROTATE				(V4L2_CID_BASE+34)
 #define V4L2_CID_BG_COLOR			(V4L2_CID_BASE+35)
+#define V4L2_CID_LED				(V4L2_CID_BASE+36)
+enum v4l2_led {
+	V4L2_LED_AUTO		= 0,
+	V4L2_LED_ON		= 1,
+	V4L2_LED_OFF		= 2,
+};
 /* last CID + 1 */
-#define V4L2_CID_LASTP1                         (V4L2_CID_BASE+36)
+#define V4L2_CID_LASTP1                         (V4L2_CID_BASE+37)

 /*  MPEG-class control IDs defined by V4L2 */
 #define V4L2_CID_MPEG_BASE 			(V4L2_CTRL_CLASS_MPEG | 0x900)

Re: [PATCH 1/3] add feedback LED control

[Jean-Francois Moine]

On Sun, 28 Feb 2010 08:55:04 +0100
Németh Márton <nm127@freemail.hu> wrote:

> On some webcams a feedback LED can be found. This LED usually shows
> the state of streaming mode: this is the "Auto" 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").

Hi,

There may be many LEDs on the webcams. One LED may be used for
the streaming state, Some other ones may be used to give more light in
dark rooms. One webcam, the microscope 093a:050f, has a top and a bottom
lights/illuminators; an other one, the MSI StarCam 370i 0c45:60c0, has
an infra-red light.

That's why I proposed to have bit fields in the control value to switch
on/off each LED.

Cheers.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH 1/3] add feedback LED control

[Németh Márton]

Jean-Francois Moine wrote:
> On Sun, 28 Feb 2010 08:55:04 +0100
> Németh Márton <nm127@freemail.hu> wrote:
> 
>> On some webcams a feedback LED can be found. This LED usually shows
>> the state of streaming mode: this is the "Auto" 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").
> 
> Hi,
> 
> There may be many LEDs on the webcams. One LED may be used for
> the streaming state, Some other ones may be used to give more light in
> dark rooms. One webcam, the microscope 093a:050f, has a top and a bottom
> lights/illuminators; an other one, the MSI StarCam 370i 0c45:60c0, has
> an infra-red light.
> 
> That's why I proposed to have bit fields in the control value to switch
> on/off each LED.

With a bitfield on and off state can be specified. What about the "auto" mode?
Should two bits grouped together to have auto, on and off state? Is there
already a similar control?

Is the brightness of the background light LEDs adjustable or are they just on/off?
If yes, then maybe the feedback LEDs and the background light LEDs should be
treated as different kind.

Regards,

	Márton Németh

Re: [PATCH 1/3] add feedback LED control

[Laurent Pinchart]

Hi Márton,

On Sunday 28 February 2010 08:55:04 Németh Márton wrote:
> From: Márton Németh <nm127@freemail.hu>
> 
> On some webcams a feedback LED can be found. This LED usually shows
> the state of streaming mode: this is the "Auto" 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").
> 
> Signed-off-by: Márton Németh <nm127@freemail.hu>
> ---
> diff -r d8fafa7d88dc linux/Documentation/DocBook/v4l/controls.xml
> --- a/linux/Documentation/DocBook/v4l/controls.xml	Thu Feb 18 19:02:51 
2010
> +0100 +++ b/linux/Documentation/DocBook/v4l/controls.xml	Sun Feb 28
> 08:41:17 2010 +0100 @@ -311,6 +311,17 @@
>  Applications depending on particular custom controls should check the
>  driver name and version, see <xref linkend="querycap" />.</entry>
>  	  </row>
> +	  <row id="v4l2-led">
> +	    <entry><constant>V4L2_CID_LED</constant></entry>
> +	    <entry>enum</entry>
> +	    <entry>Controls the feedback LED on the camera. In auto mode
> +the LED is on when the streaming is active. The LED is off when not
> streaming. +The LED can be also turned on and off independent from
> streaming. +Possible values for <constant>enum v4l2_led</constant> are:
> +<constant>V4L2_CID_LED_AUTO</constant> (0),
> +<constant>V4L2_CID_LED_ON</constant> (1) and
> +<constant>V4L2_CID_LED_OFF</constant> (2).</entry>
> +	  </row>

There's more than just auto, on and off. On Logitech webcams, LEDs can be 
configured to blink as well.

>  	</tbody>
>        </tgroup>
>      </table>
> diff -r d8fafa7d88dc linux/Documentation/DocBook/v4l/videodev2.h.xml
> --- a/linux/Documentation/DocBook/v4l/videodev2.h.xml	Thu Feb 18 19:02:51
> 2010 +0100 +++ b/linux/Documentation/DocBook/v4l/videodev2.h.xml	Sun Feb
> 28 08:41:17 2010 +0100 @@ -1024,8 +1024,14 @@
> 
>  #define V4L2_CID_ROTATE                         (V4L2_CID_BASE+34)
>  #define V4L2_CID_BG_COLOR                       (V4L2_CID_BASE+35)
> +#define V4L2_CID_LED                            (V4L2_CID_BASE+36)
> +enum v4l2_led {
> +        V4L2_LED_AUTO           = 0,
> +        V4L2_LED_ON             = 1,
> +        V4L2_LED_OFF            = 2,
> +};
>  /* last CID + 1 */
> -#define V4L2_CID_LASTP1                         (V4L2_CID_BASE+36)
> +#define V4L2_CID_LASTP1                         (V4L2_CID_BASE+37)
> 
>  /*  MPEG-class control IDs defined by V4L2 */
>  #define V4L2_CID_MPEG_BASE                      (V4L2_CTRL_CLASS_MPEG |
> 0x900) diff -r d8fafa7d88dc linux/drivers/media/video/v4l2-common.c
> --- a/linux/drivers/media/video/v4l2-common.c	Thu Feb 18 19:02:51 2010
> +0100 +++ b/linux/drivers/media/video/v4l2-common.c	Sun Feb 28 08:41:17
> 2010 +0100 @@ -349,6 +349,12 @@
>  		"75 useconds",
>  		NULL,
>  	};
> +	static const char *led[] = {
> +		"Auto",
> +		"On",
> +		"Off",
> +		NULL,
> +	};
> 
>  	switch (id) {
>  		case V4L2_CID_MPEG_AUDIO_SAMPLING_FREQ:
> @@ -389,6 +395,8 @@
>  			return colorfx;
>  		case V4L2_CID_TUNE_PREEMPHASIS:
>  			return tune_preemphasis;
> +		case V4L2_CID_LED:
> +			return led;
>  		default:
>  			return NULL;
>  	}
> @@ -434,6 +442,7 @@
>  	case V4L2_CID_COLORFX:			return "Color Effects";
>  	case V4L2_CID_ROTATE:			return "Rotate";
>  	case V4L2_CID_BG_COLOR:			return "Background color";
> +	case V4L2_CID_LED:			return "Feedback LED";
> 
>  	/* MPEG controls */
>  	case V4L2_CID_MPEG_CLASS: 		return "MPEG Encoder Controls";
> @@ -575,6 +584,7 @@
>  	case V4L2_CID_EXPOSURE_AUTO:
>  	case V4L2_CID_COLORFX:
>  	case V4L2_CID_TUNE_PREEMPHASIS:
> +	case V4L2_CID_LED:
>  		qctrl->type = V4L2_CTRL_TYPE_MENU;
>  		step = 1;
>  		break;
> diff -r d8fafa7d88dc linux/include/linux/videodev2.h
> --- a/linux/include/linux/videodev2.h	Thu Feb 18 19:02:51 2010 +0100
> +++ b/linux/include/linux/videodev2.h	Sun Feb 28 08:41:17 2010 +0100
> @@ -1030,8 +1030,14 @@
> 
>  #define V4L2_CID_ROTATE				(V4L2_CID_BASE+34)
>  #define V4L2_CID_BG_COLOR			(V4L2_CID_BASE+35)
> +#define V4L2_CID_LED				(V4L2_CID_BASE+36)
> +enum v4l2_led {
> +	V4L2_LED_AUTO		= 0,
> +	V4L2_LED_ON		= 1,
> +	V4L2_LED_OFF		= 2,
> +};

enums shouldn't be used in kernelspace <-> userspace APIs, as their size is 
not stable across ARM ABIs. In this case it won't matter much, the enum not 
being part of any structure. If someone creates a new ioctl that uses enum 
v4l2_led as one field of its structure argument, things will break.

>  /* last CID + 1 */
> -#define V4L2_CID_LASTP1                         (V4L2_CID_BASE+36)
> +#define V4L2_CID_LASTP1                         (V4L2_CID_BASE+37)
> 
>  /*  MPEG-class control IDs defined by V4L2 */
>  #define V4L2_CID_MPEG_BASE 			(V4L2_CTRL_CLASS_MPEG | 0x900)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/3] add feedback LED control

[Laurent Pinchart]

On Sunday 28 February 2010 20:38:00 Németh Márton wrote:
> Jean-Francois Moine wrote:
> > On Sun, 28 Feb 2010 08:55:04 +0100
> > 
> > Németh Márton <nm127@freemail.hu> wrote:
> >> On some webcams a feedback LED can be found. This LED usually shows
> >> the state of streaming mode: this is the "Auto" 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").
> > 
> > Hi,
> > 
> > There may be many LEDs on the webcams. One LED may be used for
> > the streaming state, Some other ones may be used to give more light in
> > dark rooms. One webcam, the microscope 093a:050f, has a top and a bottom
> > lights/illuminators; an other one, the MSI StarCam 370i 0c45:60c0, has
> > an infra-red light.
> > 
> > That's why I proposed to have bit fields in the control value to switch
> > on/off each LED.
> 
> With a bitfield on and off state can be specified. What about the "auto"
> mode? Should two bits grouped together to have auto, on and off state? Is
> there already a similar control?

I don't like the bitfield either. As stated in my previous mail, we can have 
more than 3 states, so using 2 bits per LED will simply not scale.

> Is the brightness of the background light LEDs adjustable or are they just
> on/off? If yes, then maybe the feedback LEDs and the background light LEDs
> should be treated as different kind.

I think there should indeed be a different control for the background LEDs. 
Still, there could be more than one feedback LED.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/3] add feedback LED control

[Jean-Francois Moine]

On Sun, 28 Feb 2010 20:38:00 +0100
Németh Márton <nm127@freemail.hu> wrote:

> With a bitfield on and off state can be specified. What about the
> "auto" mode? Should two bits grouped together to have auto, on and
> off state? Is there already a similar control?
> 
> Is the brightness of the background light LEDs adjustable or are they
> just on/off? If yes, then maybe the feedback LEDs and the background
> light LEDs should be treated as different kind.

OK. My idea about switching the LEDs by v4l2 controls was not good. So,
forget about it.

Instead, some job of the led class may be done in the gspca main,
especially register/unregister.

I propose to add a LED description in the gspca structure (level
'struct cam'). There would be 'nleds' for the number of LEDS and
'leds', a pointer to an array of:

	struct gspca_led {
		struct led_classdev led_cdev;
		char led_name[32];
		struct led_trigger led_trigger;
		char trigger_name[32];
	};

(this array should be in the subdriver structure - I don't show the
#ifdef's)

Then, this would work as:

- on probe, in the 'config' function of the subdriver, this one
  initializes the led and trigger fields. The 'led_cdev.name' and
  'led_trigger.name' should point to a sprintf format with one
  argument: the video device number (ex: "video%d::toplight").

- then, at the end of gspca_dev_probe(), the gspca main creates the real
  names of the leds and triggers, and does the register job.

- all led/trigger events are treated by the subdriver, normally by a
  workqueue. This one must not be the system workqueue.

- on disconnection, the gspca main unregisters the leds and triggers
  without calling the subdriver. In the workqueue, the disconnection
  can be simply handled testing the flag 'present' after each subsystem
  call.

Cheers.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

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

[Németh Márton]

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>
---
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 */

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

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart

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

[Richard Purdie]

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

Re: [PATCH 1/3] add feedback LED control

[Mauro Carvalho Chehab]

Jean-Francois Moine wrote:
> On Sun, 28 Feb 2010 20:38:00 +0100
> Németh Márton <nm127@freemail.hu> wrote:
> 
>> With a bitfield on and off state can be specified. What about the
>> "auto" mode? Should two bits grouped together to have auto, on and
>> off state? Is there already a similar control?
>>
>> Is the brightness of the background light LEDs adjustable or are they
>> just on/off? If yes, then maybe the feedback LEDs and the background
>> light LEDs should be treated as different kind.
> 
> OK. My idea about switching the LEDs by v4l2 controls was not good. So,
> forget about it.
> 
> Instead, some job of the led class may be done in the gspca main,
> especially register/unregister.
> 
> I propose to add a LED description in the gspca structure (level
> 'struct cam'). There would be 'nleds' for the number of LEDS and
> 'leds', a pointer to an array of:
> 
> 	struct gspca_led {
> 		struct led_classdev led_cdev;
> 		char led_name[32];
> 		struct led_trigger led_trigger;
> 		char trigger_name[32];
> 	};
> 
> (this array should be in the subdriver structure - I don't show the
> #ifdef's)
> 
> Then, this would work as:
> 
> - on probe, in the 'config' function of the subdriver, this one
>   initializes the led and trigger fields. The 'led_cdev.name' and
>   'led_trigger.name' should point to a sprintf format with one
>   argument: the video device number (ex: "video%d::toplight").
> 
> - then, at the end of gspca_dev_probe(), the gspca main creates the real
>   names of the leds and triggers, and does the register job.
> 
> - all led/trigger events are treated by the subdriver, normally by a
>   workqueue. This one must not be the system workqueue.
> 
> - on disconnection, the gspca main unregisters the leds and triggers
>   without calling the subdriver. In the workqueue, the disconnection
>   can be simply handled testing the flag 'present' after each subsystem
>   call.
> 
> Cheers.
> 

The feedback LED patch series doesn't apply anymore.

patching file Documentation/DocBook/v4l/controls.xml
Hunk #1 succeeded at 324 (offset 13 lines).
patching file Documentation/DocBook/v4l/videodev2.h.xml
Hunk #1 succeeded at 1031 (offset 7 lines).
patching file drivers/media/video/v4l2-common.c
Hunk #1 succeeded at 358 (offset 9 lines).
Hunk #3 FAILED at 442.
Hunk #4 succeeded at 598 (offset 14 lines).
1 out of 4 hunks FAILED -- saving rejects to file drivers/media/video/v4l2-common.c.rej
patching file include/linux/videodev2.h
Hunk #1 FAILED at 1030.
1 out of 1 hunk FAILED -- saving rejects to file include/linux/videodev2.h.rej
>>> Patch patches/lmml_82773_1_3_add_feedback_led_control.patch doesn't apply

As the original approach weren't accepted, I'm dropping those patches from
my queue. Please re-submit after having some agreement about that.

-- 

Cheers,
Mauro