[1/3] add feedback LED control

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

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

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.
  

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
--
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 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)
  

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.
  

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.
  

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.