[PATCH 1/2] gspca pac7302: add LED control

[PATCH 1/2] gspca pac7302: add LED control

[Németh Márton]

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

On Labtec Webcam 2200 there is a feedback LED which can be controlled
independent from the streaming. The feedback LED can be used from
user space application to show for example detected motion or to
distinguish between the preview and "on-air" state of the video stream.

The default value of the LED control is "Auto" which keeps the previous
behaviour: LED is off when the stream is off, LED is on if the stream is
on.

Signed-off-by: Márton Németh <nm127@freemail.hu>
---
diff -r 4f102b2f7ac1 linux/drivers/media/video/gspca/pac7302.c
--- a/linux/drivers/media/video/gspca/pac7302.c	Thu Jan 28 20:35:40 2010 +0100
+++ b/linux/drivers/media/video/gspca/pac7302.c	Mon Feb 08 20:46:53 2010 +0100
@@ -6,6 +6,7 @@
  *
  * Separated from Pixart PAC7311 library by Márton Németh
  * Camera button input handling by Márton Németh <nm127@freemail.hu>
+ * LED control by Márton Németh <nm127@freemail.hu>
  * Copyright (C) 2009-2010 Márton Németh <nm127@freemail.hu>
  *
  * This program is free software; you can redistribute it and/or modify
@@ -62,6 +63,7 @@
     0   | 0xc6       | setwhitebalance()
     0   | 0xc7       | setbluebalance()
     0   | 0xdc       | setbrightcont(), setcolors()
+    1   | 0x78       | set_streaming_led()
     3   | 0x02       | setexposure()
     3   | 0x10       | setgain()
     3   | 0x11       | setcolors(), setgain(), setexposure(), sethvflip()
@@ -78,6 +80,11 @@
 MODULE_DESCRIPTION("Pixart PAC7302");
 MODULE_LICENSE("GPL");

+#define PAC7302_CID_LED (V4L2_CID_PRIVATE_BASE + 0)
+#define LED_AUTO	0
+#define LED_ON		1
+#define LED_OFF		2
+
 /* specific webcam descriptor for pac7302 */
 struct sd {
 	struct gspca_dev gspca_dev;		/* !! must be the first item */
@@ -91,6 +98,7 @@
 	unsigned char gain;
 	unsigned char exposure;
 	unsigned char autogain;
+	unsigned char led;
 	__u8 hflip;
 	__u8 vflip;
 	u8 flags;
@@ -126,6 +134,8 @@
 static int sd_getgain(struct gspca_dev *gspca_dev, __s32 *val);
 static int sd_setexposure(struct gspca_dev *gspca_dev, __s32 val);
 static int sd_getexposure(struct gspca_dev *gspca_dev, __s32 *val);
+static int sd_setled(struct gspca_dev *gspca_dev, __s32 val);
+static int sd_getled(struct gspca_dev *gspca_dev, __s32 *val);

 static const struct ctrl sd_ctrls[] = {
 /* This control is pac7302 only */
@@ -293,6 +303,20 @@
 	    .set = sd_setvflip,
 	    .get = sd_getvflip,
 	},
+	{
+	    {
+		.id      = PAC7302_CID_LED,
+		.type    = V4L2_CTRL_TYPE_MENU,
+		.name    = "LED",
+		.minimum = 0,
+		.maximum = 2,	/* 0: Auto, 1: On, 2: Off */
+		.step    = 1,
+#define LED_DEF 0
+		.default_value = LED_DEF,
+	    },
+	    .set = sd_setled,
+	    .get = sd_getled,
+	},
 };

 static const struct v4l2_pix_format vga_mode[] = {
@@ -572,6 +596,7 @@
 	sd->gain = GAIN_DEF;
 	sd->exposure = EXPOSURE_DEF;
 	sd->autogain = AUTOGAIN_DEF;
+	sd->led = LED_DEF;
 	sd->hflip = HFLIP_DEF;
 	sd->vflip = VFLIP_DEF;
 	sd->flags = id->driver_info;
@@ -716,6 +741,36 @@
 	reg_w(gspca_dev, 0x11, 0x01);
 }

+static void set_streaming_led(struct gspca_dev *gspca_dev, u8 streaming)
+{
+	struct sd *sd = (struct sd *) gspca_dev;
+	u8 data = 0;
+
+	switch (sd->led) {
+	case LED_AUTO:
+		if (streaming)
+			data = 0x01;
+		else
+			data = 0x40;
+		break;
+	case LED_ON:
+		if (streaming)
+			data = 0x01;
+		else
+			data = 0x00;
+		break;
+	case LED_OFF:
+		if (streaming)
+			data = 0x41;
+		else
+			data = 0x40;
+		break;
+	}
+
+	reg_w(gspca_dev, 0xff, 0x01);
+	reg_w(gspca_dev, 0x78, data);
+}
+
 /* this function is called at probe and resume time for pac7302 */
 static int sd_init(struct gspca_dev *gspca_dev)
 {
@@ -747,18 +802,15 @@
 	atomic_set(&sd->avg_lum, -1);

 	/* start stream */
-	reg_w(gspca_dev, 0xff, 0x01);
-	reg_w(gspca_dev, 0x78, 0x01);
+	set_streaming_led(gspca_dev, 1);

 	return gspca_dev->usb_err;
 }

 static void sd_stopN(struct gspca_dev *gspca_dev)
 {
-
 	/* stop stream */
-	reg_w(gspca_dev, 0xff, 0x01);
-	reg_w(gspca_dev, 0x78, 0x00);
+	set_streaming_led(gspca_dev, 0);
 }

 /* called on streamoff with alt 0 and on disconnect for pac7302 */
@@ -766,8 +818,7 @@
 {
 	if (!gspca_dev->present)
 		return;
-	reg_w(gspca_dev, 0xff, 0x01);
-	reg_w(gspca_dev, 0x78, 0x40);
+	set_streaming_led(gspca_dev, 0);
 }

 /* Include pac common sof detection functions */
@@ -1121,6 +1172,44 @@
 	return 0;
 }

+static int sd_setled(struct gspca_dev *gspca_dev, __s32 val)
+{
+	struct sd *sd = (struct sd *) gspca_dev;
+
+	sd->led = val;
+	set_streaming_led(gspca_dev, gspca_dev->streaming);
+	return gspca_dev->usb_err;
+}
+
+static int sd_getled(struct gspca_dev *gspca_dev, __s32 *val)
+{
+	struct sd *sd = (struct sd *) gspca_dev;
+
+	*val = sd->led;
+	return 0;
+}
+
+static int sd_querymenu(struct gspca_dev *gspca_dev,
+			struct v4l2_querymenu *menu)
+{
+	switch (menu->id) {
+	case PAC7302_CID_LED:
+		switch (menu->index) {
+		case LED_AUTO:
+			strcpy((char *)menu->name, "Auto");
+			return 0;
+		case LED_OFF:
+			strcpy((char *)menu->name, "Off");
+			return 0;
+		case LED_ON:
+			strcpy((char *)menu->name, "On");
+			return 0;
+		}
+		break;
+	}
+	return -EINVAL;
+}
+
 #ifdef CONFIG_VIDEO_ADV_DEBUG
 static int sd_dbg_s_register(struct gspca_dev *gspca_dev,
 			struct v4l2_dbg_register *reg)
@@ -1210,6 +1299,7 @@
 	.stop0 = sd_stop0,
 	.pkt_scan = sd_pkt_scan,
 	.dq_callback = do_autogain,
+	.querymenu = sd_querymenu,
 #ifdef CONFIG_VIDEO_ADV_DEBUG
 	.set_register = sd_dbg_s_register,
 	.get_chip_ident = sd_chip_ident,

Re: [PATCH 1/2] gspca pac7302: add LED control

[Jean-Francois Moine]

On Wed, 24 Feb 2010 07:52:14 +0100
Németh Márton <nm127@freemail.hu> wrote:

> On Labtec Webcam 2200 there is a feedback LED which can be controlled
> independent from the streaming. The feedback LED can be used from
> user space application to show for example detected motion or to
> distinguish between the preview and "on-air" state of the video
> stream.
	[snip]
> +#define PAC7302_CID_LED (V4L2_CID_PRIVATE_BASE + 0)
	[snip]

Hello,

Private controls must not be used. LED control should be generic (I
have many requests for that).

In March 2009, I proposed to add a V4L2_CID_LEDS control ("[PATCH] LED
control"), but someone told me to use the sysfs led interface instead.
After looking at this interface, I thought it was too complex for our
purpose and no easy for the users.

Maybe it is time to argue again...

Cheers.

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

[PATCH 1/2] gspca pac7302: allow controlling LED separately

[Németh Márton]

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

On Labtec Webcam 2200 there is a feedback LED which can be controlled
independent from the streaming. The feedback LED can be used from
user space application to show for example detected motion or to
distinguish between the preview and "on-air" state of the video stream.

The default value of the LED trigger keeps the previous behaviour:
LED is off when the stream is off, LED is on if the stream is on.

The code is working in the following three cases:
 (1) when the LED subsystem ins not configured at all;
 (2) when the LED subsystem is available, but the LED triggers are not available and
 (3) when both the LED subsystem and LED triggers are configured.

Signed-off-by: Márton Németh <nm127@freemail.hu>
---
diff -r 4f102b2f7ac1 linux/drivers/media/video/gspca/pac7302.c
--- a/linux/drivers/media/video/gspca/pac7302.c	Thu Jan 28 20:35:40 2010 +0100
+++ b/linux/drivers/media/video/gspca/pac7302.c	Sat Feb 27 00:57:32 2010 +0100
@@ -6,6 +6,7 @@
  *
  * Separated from Pixart PAC7311 library by Márton Németh
  * Camera button input handling by Márton Németh <nm127@freemail.hu>
+ * LED control by Márton Németh <nm127@freemail.hu>
  * Copyright (C) 2009-2010 Márton Németh <nm127@freemail.hu>
  *
  * This program is free software; you can redistribute it and/or modify
@@ -62,6 +63,7 @@
     0   | 0xc6       | setwhitebalance()
     0   | 0xc7       | setbluebalance()
     0   | 0xdc       | setbrightcont(), setcolors()
+    1   | 0x78       | set_streaming_led()
     3   | 0x02       | setexposure()
     3   | 0x10       | setgain()
     3   | 0x11       | setcolors(), setgain(), setexposure(), sethvflip()
@@ -72,6 +74,8 @@

 #include <linux/input.h>
 #include <media/v4l2-chip-ident.h>
+#include <linux/leds.h>
+#include <linux/workqueue.h>
 #include "gspca.h"

 MODULE_AUTHOR("Thomas Kaiser thomas@kaiser-linux.li");
@@ -91,6 +95,7 @@
 	unsigned char gain;
 	unsigned char exposure;
 	unsigned char autogain;
+	unsigned char led;
 	__u8 hflip;
 	__u8 vflip;
 	u8 flags;
@@ -101,6 +106,16 @@
 	u8 autogain_ignore_frames;

 	atomic_t avg_lum;
+
+#ifdef CONFIG_LEDS_CLASS
+	struct led_classdev led_cdev;
+	struct work_struct led_work;
+	char name[32];
+#ifdef CONFIG_LEDS_TRIGGERS
+	struct led_trigger led_trigger;
+	char trigger_name[32];
+#endif
+#endif
 };

 /* V4L2 controls supported by the driver */
@@ -572,6 +587,7 @@
 	sd->gain = GAIN_DEF;
 	sd->exposure = EXPOSURE_DEF;
 	sd->autogain = AUTOGAIN_DEF;
+	sd->led = 0;
 	sd->hflip = HFLIP_DEF;
 	sd->vflip = VFLIP_DEF;
 	sd->flags = id->driver_info;
@@ -716,6 +732,58 @@
 	reg_w(gspca_dev, 0x11, 0x01);
 }

+static void set_streaming_led(struct gspca_dev *gspca_dev, u8 streaming)
+{
+	struct sd *sd = (struct sd *) gspca_dev;
+	u8 data = 0;
+
+	if (sd->led) {
+		if (streaming)
+			data = 0x01;
+		else
+			data = 0x00;
+	} else {
+		if (streaming)
+			data = 0x41;
+		else
+			data = 0x40;
+	}
+
+	reg_w(gspca_dev, 0xff, 0x01);
+	reg_w(gspca_dev, 0x78, data);
+}
+
+#ifdef CONFIG_LEDS_CLASS
+/* Set the LED, may sleep */
+static void led_work(struct work_struct *work)
+{
+	struct sd *sd = container_of(work, struct sd, led_work);
+	struct gspca_dev *gspca_dev = &sd->gspca_dev;
+
+	mutex_lock(&gspca_dev->usb_lock);
+	set_streaming_led(gspca_dev, gspca_dev->streaming);
+	mutex_unlock(&gspca_dev->usb_lock);
+}
+
+/* LED state set request, must not sleep */
+static void led_set(struct led_classdev *led_cdev,
+		    enum led_brightness value)
+{
+	u8 new_value;
+	struct sd *sd = container_of(led_cdev, struct sd, led_cdev);
+
+	if (value == LED_OFF)
+		new_value = 0;
+	else
+		new_value = 1;
+
+	if (sd->led != new_value) {
+		sd->led = new_value;
+		schedule_work(&sd->led_work);
+	}
+}
+#endif
+
 /* this function is called at probe and resume time for pac7302 */
 static int sd_init(struct gspca_dev *gspca_dev)
 {
@@ -747,27 +815,60 @@
 	atomic_set(&sd->avg_lum, -1);

 	/* start stream */
-	reg_w(gspca_dev, 0xff, 0x01);
-	reg_w(gspca_dev, 0x78, 0x01);
+
+#if defined(CONFIG_LEDS_CLASS) && defined(CONFIG_LEDS_TRIGGERS)
+	led_trigger_event(&sd->led_trigger, LED_FULL);
+#elif defined(CONFIG_LEDS_CLASS)
+	sd->led_cdev.brightness = sd->led_cdev.max_brightness;
+	if (!(sd->led_cdev.flags & LED_SUSPENDED))
+		sd->led_cdev.brightness_set(&sd->led_cdev,
+				sd->led_cdev.brightness);
+#else
+	sd->led = 1;
+#endif
+	set_streaming_led(gspca_dev, 1);

 	return gspca_dev->usb_err;
 }

 static void sd_stopN(struct gspca_dev *gspca_dev)
 {
+	struct sd *sd = container_of(gspca_dev, struct sd, gspca_dev);

+#ifndef CONFIG_LEDS_CLASS
+	sd->led = 0;
+#endif
 	/* stop stream */
-	reg_w(gspca_dev, 0xff, 0x01);
-	reg_w(gspca_dev, 0x78, 0x00);
+	set_streaming_led(gspca_dev, 0);
+#if defined(CONFIG_LEDS_CLASS) && defined(CONFIG_LEDS_TRIGGERS)
+	led_trigger_event(&sd->led_trigger, LED_OFF);
+#elif defined(CONFIG_LEDS_CLASS)
+	sd->led_cdev.brightness = LED_OFF;
+	if (!(sd->led_cdev.flags & LED_SUSPENDED))
+		sd->led_cdev.brightness_set(&sd->led_cdev,
+				sd->led_cdev.brightness);
+#endif
 }

 /* called on streamoff with alt 0 and on disconnect for pac7302 */
 static void sd_stop0(struct gspca_dev *gspca_dev)
 {
+	struct sd *sd = container_of(gspca_dev, struct sd, gspca_dev);
+
 	if (!gspca_dev->present)
 		return;
-	reg_w(gspca_dev, 0xff, 0x01);
-	reg_w(gspca_dev, 0x78, 0x40);
+#ifndef CONFIG_LEDS_CLASS
+	sd->led = 0;
+#endif
+	set_streaming_led(gspca_dev, 0);
+#if defined(CONFIG_LEDS_CLASS) && defined(CONFIG_LEDS_TRIGGERS)
+	led_trigger_event(&sd->led_trigger, LED_OFF);
+#elif defined(CONFIG_LEDS_CLASS)
+	sd->led_cdev.brightness = LED_OFF;
+	if (!(sd->led_cdev.flags & LED_SUSPENDED))
+		sd->led_cdev.brightness_set(&sd->led_cdev,
+				sd->led_cdev.brightness);
+#endif
 }

 /* Include pac common sof detection functions */
@@ -1239,15 +1340,65 @@
 static int __devinit sd_probe(struct usb_interface *intf,
 			const struct usb_device_id *id)
 {
-	return gspca_dev_probe(intf, id, &sd_desc, sizeof(struct sd),
+	int ret;
+
+	ret = gspca_dev_probe(intf, id, &sd_desc, sizeof(struct sd),
 				THIS_MODULE);
+#ifdef CONFIG_LEDS_CLASS
+	if (ret == 0) {
+		struct gspca_dev *gspca_dev = usb_get_intfdata(intf);
+		struct sd *sd = container_of(gspca_dev, struct sd, gspca_dev);
+
+#ifdef CONFIG_LEDS_TRIGGERS
+		snprintf(sd->trigger_name, sizeof(sd->trigger_name),
+			"pac7302-%u", gspca_dev->vdev.num);
+		sd->led_trigger.name = sd->trigger_name;
+		sd->led_cdev.default_trigger = sd->trigger_name;
+#endif
+		snprintf(sd->name, sizeof(sd->name),
+			"pac7302-%u::camera", gspca_dev->vdev.num);
+		sd->led_cdev.name = sd->name;
+		sd->led_cdev.brightness_set = led_set;
+		sd->led_cdev.blink_set = NULL;
+		sd->led_cdev.flags = LED_CORE_SUSPENDRESUME;
+		INIT_WORK(&sd->led_work, led_work);
+		ret = led_classdev_register(&gspca_dev->dev->dev,
+					    &sd->led_cdev);
+		if (ret)
+			gspca_disconnect(intf);
+		else {
+#ifdef CONFIG_LEDS_TRIGGERS
+			led_trigger_register(&sd->led_trigger);
+#endif
+		}
+	}
+#endif
+
+	return ret;
 }

+#ifdef CONFIG_LEDS_CLASS
+static void sd_disconnect(struct usb_interface *intf)
+{
+	struct gspca_dev *gspca_dev = usb_get_intfdata(intf);
+	struct sd *sd = container_of(gspca_dev, struct sd, gspca_dev);
+
+#ifdef CONFIG_LEDS_TRIGGERS
+	led_trigger_unregister(&sd->led_trigger);
+#endif
+	led_classdev_unregister(&sd->led_cdev);
+	cancel_work_sync(&sd->led_work);
+	gspca_disconnect(intf);
+}
+#else
+#define sd_disconnect gspca_disconnect
+#endif
+
 static struct usb_driver sd_driver = {
 	.name = MODULE_NAME,
 	.id_table = device_table,
 	.probe = sd_probe,
-	.disconnect = gspca_disconnect,
+	.disconnect = sd_disconnect,
 #ifdef CONFIG_PM
 	.suspend = gspca_suspend,
 	.resume = gspca_resume,

Re: [PATCH 1/2] gspca pac7302: allow controlling LED separately

[Hans de Goede]

Hi,

On 02/27/2010 01:20 AM, Németh Márton wrote:
> From: Márton Németh<nm127@freemail.hu>
>
> On Labtec Webcam 2200 there is a feedback LED which can be controlled
> independent from the streaming.

This is true for a lot of cameras, so if we are going to add a way to
support control of the LED separate of the streaming state, we
should do that at the gspca_main level, and let sub drivers which
support this export a set_led callback function.

I must say I personally don't see much of a use case for this feature,
but I believe JF Moine does, so I'll leave further comments and
review of this to JF. I do believe it is important that if we go this
way we do so add the gspca_main level.

Regards,

Hans

Re: [PATCH 1/2] gspca pac7302: allow controlling LED separately

[Németh Márton]

Hi,

I missed the CONFIG_LEDS_CLASS_MODULE configuration option in the previous
version of this patch, now it is added.

Regards,

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

On Labtec Webcam 2200 there is a feedback LED which can be controlled
independent from the streaming. The feedback LED can be used from
user space application to show for example detected motion or to
distinguish between the preview and "on-air" state of the video stream.

The default value of the LED trigger keeps the previous behaviour:
LED is off when the stream is off, LED is on if the stream is on.

The code is working in the following three cases:
 (1) when the LED subsystem ins not configured at all;
 (2) when the LED subsystem is available, but the LED triggers are not available and
 (3) when both the LED subsystem and LED triggers are configured.

Signed-off-by: Márton Németh <nm127@freemail.hu>
---
diff -r d8fafa7d88dc linux/drivers/media/video/gspca/pac7302.c
--- a/linux/drivers/media/video/gspca/pac7302.c	Thu Feb 18 19:02:51 2010 +0100
+++ b/linux/drivers/media/video/gspca/pac7302.c	Sat Feb 27 09:10:44 2010 +0100
@@ -6,6 +6,7 @@
  *
  * Separated from Pixart PAC7311 library by Márton Németh
  * Camera button input handling by Márton Németh <nm127@freemail.hu>
+ * LED control by Márton Németh <nm127@freemail.hu>
  * Copyright (C) 2009-2010 Márton Németh <nm127@freemail.hu>
  *
  * This program is free software; you can redistribute it and/or modify
@@ -62,6 +63,7 @@
     0   | 0xc6       | setwhitebalance()
     0   | 0xc7       | setbluebalance()
     0   | 0xdc       | setbrightcont(), setcolors()
+    1   | 0x78       | set_streaming_led()
     3   | 0x02       | setexposure()
     3   | 0x10       | setgain()
     3   | 0x11       | setcolors(), setgain(), setexposure(), sethvflip()
@@ -72,6 +74,8 @@

 #include <linux/input.h>
 #include <media/v4l2-chip-ident.h>
+#include <linux/leds.h>
+#include <linux/workqueue.h>
 #include "gspca.h"

 MODULE_AUTHOR("Thomas Kaiser thomas@kaiser-linux.li");
@@ -91,6 +95,7 @@
 	unsigned char gain;
 	unsigned char exposure;
 	unsigned char autogain;
+	unsigned char led;
 	__u8 hflip;
 	__u8 vflip;
 	u8 flags;
@@ -101,6 +106,16 @@
 	u8 autogain_ignore_frames;

 	atomic_t avg_lum;
+
+#if defined(CONFIG_LEDS_CLASS) || defined(CONFIG_LEDS_CLASS_MODULE)
+	struct led_classdev led_cdev;
+	struct work_struct led_work;
+	char name[32];
+#ifdef CONFIG_LEDS_TRIGGERS
+	struct led_trigger led_trigger;
+	char trigger_name[32];
+#endif
+#endif
 };

 /* V4L2 controls supported by the driver */
@@ -572,6 +587,7 @@
 	sd->gain = GAIN_DEF;
 	sd->exposure = EXPOSURE_DEF;
 	sd->autogain = AUTOGAIN_DEF;
+	sd->led = 0;
 	sd->hflip = HFLIP_DEF;
 	sd->vflip = VFLIP_DEF;
 	sd->flags = id->driver_info;
@@ -716,6 +732,58 @@
 	reg_w(gspca_dev, 0x11, 0x01);
 }

+static void set_streaming_led(struct gspca_dev *gspca_dev, u8 streaming)
+{
+	struct sd *sd = (struct sd *) gspca_dev;
+	u8 data = 0;
+
+	if (sd->led) {
+		if (streaming)
+			data = 0x01;
+		else
+			data = 0x00;
+	} else {
+		if (streaming)
+			data = 0x41;
+		else
+			data = 0x40;
+	}
+
+	reg_w(gspca_dev, 0xff, 0x01);
+	reg_w(gspca_dev, 0x78, data);
+}
+
+#if defined(CONFIG_LEDS_CLASS) || defined(CONFIG_LEDS_CLASS_MODULE)
+/* Set the LED, may sleep */
+static void led_work(struct work_struct *work)
+{
+	struct sd *sd = container_of(work, struct sd, led_work);
+	struct gspca_dev *gspca_dev = &sd->gspca_dev;
+
+	mutex_lock(&gspca_dev->usb_lock);
+	set_streaming_led(gspca_dev, gspca_dev->streaming);
+	mutex_unlock(&gspca_dev->usb_lock);
+}
+
+/* LED state set request, must not sleep */
+static void led_set(struct led_classdev *led_cdev,
+		    enum led_brightness value)
+{
+	u8 new_value;
+	struct sd *sd = container_of(led_cdev, struct sd, led_cdev);
+
+	if (value == LED_OFF)
+		new_value = 0;
+	else
+		new_value = 1;
+
+	if (sd->led != new_value) {
+		sd->led = new_value;
+		schedule_work(&sd->led_work);
+	}
+}
+#endif
+
 /* this function is called at probe and resume time for pac7302 */
 static int sd_init(struct gspca_dev *gspca_dev)
 {
@@ -747,27 +815,60 @@
 	atomic_set(&sd->avg_lum, -1);

 	/* start stream */
-	reg_w(gspca_dev, 0xff, 0x01);
-	reg_w(gspca_dev, 0x78, 0x01);
+
+#if (defined(CONFIG_LEDS_CLASS) || defined(CONFIG_LEDS_CLASS_MODULE)) && defined(CONFIG_LEDS_TRIGGERS)
+	led_trigger_event(&sd->led_trigger, LED_FULL);
+#elif defined(CONFIG_LEDS_CLASS) || defined(CONFIG_LEDS_CLASS_MODULE)
+	sd->led_cdev.brightness = sd->led_cdev.max_brightness;
+	if (!(sd->led_cdev.flags & LED_SUSPENDED))
+		sd->led_cdev.brightness_set(&sd->led_cdev,
+				sd->led_cdev.brightness);
+#else
+	sd->led = 1;
+#endif
+	set_streaming_led(gspca_dev, 1);

 	return gspca_dev->usb_err;
 }

 static void sd_stopN(struct gspca_dev *gspca_dev)
 {
+	struct sd *sd = container_of(gspca_dev, struct sd, gspca_dev);

+#if !defined(CONFIG_LEDS_CLASS) && !defined(CONFIG_LEDS_CLASS_MODULE)
+	sd->led = 0;
+#endif
 	/* stop stream */
-	reg_w(gspca_dev, 0xff, 0x01);
-	reg_w(gspca_dev, 0x78, 0x00);
+	set_streaming_led(gspca_dev, 0);
+#if (defined(CONFIG_LEDS_CLASS) || defined(CONFIG_LEDS_CLASS_MODULE)) && defined(CONFIG_LEDS_TRIGGERS)
+	led_trigger_event(&sd->led_trigger, LED_OFF);
+#elif defined(CONFIG_LEDS_CLASS) || defined(CONFIG_LEDS_CLASS_MODULE)
+	sd->led_cdev.brightness = LED_OFF;
+	if (!(sd->led_cdev.flags & LED_SUSPENDED))
+		sd->led_cdev.brightness_set(&sd->led_cdev,
+				sd->led_cdev.brightness);
+#endif
 }

 /* called on streamoff with alt 0 and on disconnect for pac7302 */
 static void sd_stop0(struct gspca_dev *gspca_dev)
 {
+	struct sd *sd = container_of(gspca_dev, struct sd, gspca_dev);
+
 	if (!gspca_dev->present)
 		return;
-	reg_w(gspca_dev, 0xff, 0x01);
-	reg_w(gspca_dev, 0x78, 0x40);
+#if !defined(CONFIG_LEDS_CLASS) && !defined(CONFIG_LEDS_CLASS_MODULE)
+	sd->led = 0;
+#endif
+	set_streaming_led(gspca_dev, 0);
+#if (defined(CONFIG_LEDS_CLASS) || defined(CONFIG_LEDS_CLASS_MODULE)) && defined(CONFIG_LEDS_TRIGGERS)
+	led_trigger_event(&sd->led_trigger, LED_OFF);
+#elif defined(CONFIG_LEDS_CLASS) || defined(CONFIG_LEDS_CLASS_MODULE)
+	sd->led_cdev.brightness = LED_OFF;
+	if (!(sd->led_cdev.flags & LED_SUSPENDED))
+		sd->led_cdev.brightness_set(&sd->led_cdev,
+				sd->led_cdev.brightness);
+#endif
 }

 /* Include pac common sof detection functions */
@@ -1239,15 +1340,65 @@
 static int __devinit sd_probe(struct usb_interface *intf,
 			const struct usb_device_id *id)
 {
-	return gspca_dev_probe(intf, id, &sd_desc, sizeof(struct sd),
+	int ret;
+
+	ret = gspca_dev_probe(intf, id, &sd_desc, sizeof(struct sd),
 				THIS_MODULE);
+#if defined(CONFIG_LEDS_CLASS) || defined(CONFIG_LEDS_CLASS_MODULE)
+	if (ret == 0) {
+		struct gspca_dev *gspca_dev = usb_get_intfdata(intf);
+		struct sd *sd = container_of(gspca_dev, struct sd, gspca_dev);
+
+#ifdef CONFIG_LEDS_TRIGGERS
+		snprintf(sd->trigger_name, sizeof(sd->trigger_name),
+			"pac7302-%u", gspca_dev->vdev.num);
+		sd->led_trigger.name = sd->trigger_name;
+		sd->led_cdev.default_trigger = sd->trigger_name;
+#endif
+		snprintf(sd->name, sizeof(sd->name),
+			"pac7302-%u::camera", gspca_dev->vdev.num);
+		sd->led_cdev.name = sd->name;
+		sd->led_cdev.brightness_set = led_set;
+		sd->led_cdev.blink_set = NULL;
+		sd->led_cdev.flags = LED_CORE_SUSPENDRESUME;
+		INIT_WORK(&sd->led_work, led_work);
+		ret = led_classdev_register(&gspca_dev->dev->dev,
+					    &sd->led_cdev);
+		if (ret)
+			gspca_disconnect(intf);
+		else {
+#ifdef CONFIG_LEDS_TRIGGERS
+			led_trigger_register(&sd->led_trigger);
+#endif
+		}
+	}
+#endif
+
+	return ret;
 }

+#if defined(CONFIG_LEDS_CLASS) || defined(CONFIG_LEDS_CLASS_MODULE)
+static void sd_disconnect(struct usb_interface *intf)
+{
+	struct gspca_dev *gspca_dev = usb_get_intfdata(intf);
+	struct sd *sd = container_of(gspca_dev, struct sd, gspca_dev);
+
+#ifdef CONFIG_LEDS_TRIGGERS
+	led_trigger_unregister(&sd->led_trigger);
+#endif
+	led_classdev_unregister(&sd->led_cdev);
+	cancel_work_sync(&sd->led_work);
+	gspca_disconnect(intf);
+}
+#else
+#define sd_disconnect gspca_disconnect
+#endif
+
 static struct usb_driver sd_driver = {
 	.name = MODULE_NAME,
 	.id_table = device_table,
 	.probe = sd_probe,
-	.disconnect = gspca_disconnect,
+	.disconnect = sd_disconnect,
 #ifdef CONFIG_PM
 	.suspend = gspca_suspend,
 	.resume = gspca_resume,

Re: [PATCH 1/2] gspca pac7302: allow controlling LED separately

[Németh Márton]

Hi,
Hans de Goede wrote:
> Hi,
> 
> On 02/27/2010 01:20 AM, Németh Márton wrote:
>> From: Márton Németh<nm127@freemail.hu>
>>
>> On Labtec Webcam 2200 there is a feedback LED which can be controlled
>> independent from the streaming.
> 
> This is true for a lot of cameras, so if we are going to add a way to
> support control of the LED separate of the streaming state, we
> should do that at the gspca_main level, and let sub drivers which
> support this export a set_led callback function.

If the code is moved to gspca_main level, what shall be the name of the
LED? According to Documentation/leds-class.txt, chapter "LED Device Naming"
my proposal for "devicename" would be:

 * /sys/class/leds/video-0::camera
 * /sys/class/leds/video-1::camera
 * /sys/class/leds/video-2::camera
 * ...

or

 * /sys/class/leds/video0::camera
 * /sys/class/leds/video1::camera
 * /sys/class/leds/video2::camera
 * ...

Which is the right one?

> I must say I personally don't see much of a use case for this feature,
> but I believe JF Moine does, so I'll leave further comments and
> review of this to JF. I do believe it is important that if we go this
> way we do so add the gspca_main level.
> 
> Regards,
> 
> Hans

Re: [PATCH 1/2] gspca pac7302: allow controlling LED separately

[Jean-Francois Moine]

On Sat, 27 Feb 2010 08:53:16 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> This is true for a lot of cameras, so if we are going to add a way to
> support control of the LED separate of the streaming state, we
> should do that at the gspca_main level, and let sub drivers which
> support this export a set_led callback function.
> 
> I must say I personally don't see much of a use case for this feature,
> but I believe JF Moine does, so I'll leave further comments and
> review of this to JF. I do believe it is important that if we go this
> way we do so add the gspca_main level.

Hi,

I don't like this mechanism to switch on or off the webcam LEDs. Here
are some arguments (some of them were sent last year to the list):

1) End users.

Many Linux users don't know the kernel internals, nor which of the too
many applications they should use to make their devices work. 

Actually, there are a few X11 programs in common Linux distros which can
handle the webcam parameters: I just know about vlc and v4l2ucp, and
they don't even handle the VIDIOC_G_JPEGCOMP and VIDIOC_S_JPEGCOMP
ioctls which are part of the v4l2 API.

The LED interface uses the /sys file system. Will the webcam programs
offer access to this interface? I don't believe it. So, the users will
have to search how they can switch on or off the LEDs of their webcams,
and then, when found, they should start a X terminal and run a command
to do the job!

On the other hand, a webcam LED control, whether general or private, is
available in the graphical interface as soon as the driver offers it.

2) Memory overhead.

Using the led class adds more kernel code and asks the webcam drivers
to create a new device. Also, the function called for changing the LED
brighness cannot sleep, so the use a workqueue is required.

On contrary, with a webcam control, only one byte (for 8 LEDs) is added
to the webcam structure and the change is immediatly done in the ioctl.

3) Development.

If nobody wants a LED control in the v4l2 interface, I think the code
added to access the led class could be splitted between the different
subsystem. For example, the workqueue handling could be done in the led
class itself...

Cheers.

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

Re: [PATCH 1/2] gspca pac7302: allow controlling LED separately

[Németh Márton]

Hi,
Jean-Francois Moine wrote:
> On Sat, 27 Feb 2010 08:53:16 +0100
> Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> This is true for a lot of cameras, so if we are going to add a way to
>> support control of the LED separate of the streaming state, we
>> should do that at the gspca_main level, and let sub drivers which
>> support this export a set_led callback function.
>>
>> I must say I personally don't see much of a use case for this feature,
>> but I believe JF Moine does, so I'll leave further comments and
>> review of this to JF. I do believe it is important that if we go this
>> way we do so add the gspca_main level.
> 
> Hi,
> 
> I don't like this mechanism to switch on or off the webcam LEDs. Here
> are some arguments (some of them were sent last year to the list):

I could accept both the V4L2 control solution or the LED subclass solution
for handling the camera LED. I miss a little the positive side of using
the LED subclass from the list, so I try take the role of that side and
add them.

> 1) End users.
> 
> Many Linux users don't know the kernel internals, nor which of the too
> many applications they should use to make their devices work. 
> 
> Actually, there are a few X11 programs in common Linux distros which can
> handle the webcam parameters: I just know about vlc and v4l2ucp, and
> they don't even handle the VIDIOC_G_JPEGCOMP and VIDIOC_S_JPEGCOMP
> ioctls which are part of the v4l2 API.
>
> The LED interface uses the /sys file system. Will the webcam programs
> offer access to this interface? I don't believe it. So, the users will
> have to search how they can switch on or off the LEDs of their webcams,
> and then, when found, they should start a X terminal and run a command
> to do the job!

The programs like v4l2ucp can be extended to handle the /sys/class/leds
interface. This is work but the user will not recognise the difference
as long as the GUI supports it.

> On the other hand, a webcam LED control, whether general or private, is
> available in the graphical interface as soon as the driver offers it.
> 
> 2) Memory overhead.
> 
> Using the led class adds more kernel code and asks the webcam drivers
> to create a new device. Also, the function called for changing the LED
> brighness cannot sleep, so the use a workqueue is required.

Let me show the numbers on a 32bit machine:
  sizeof(struct gspca_dev) = 2032 bytes
  sizeof(struct led_classdev) = 112 bytes
  sizeof(struct work_struct) = 28 bytes
  sizeof(struct led_trigger) = 52 bytes

Additionally two strings are also needed one for the LED device name and
one for the LED trigger. Let's take 32 bytes for each (this value can be
made smaller). This means that the necessary memory is 112+28+52+2*32 =
256 bytes.

The pac7302 driver subdriver structure with LED device, workqueue and LED
trigger is:
  sizeof(struct sd) = 2308 bytes

So the memory usage increase is 1-2308/2032 = 13.6%. I would compare the
structure size with the memory page size of the x86 system which is 4096 bytes
(see the return value of getpagesize(2)).

> On contrary, with a webcam control, only one byte (for 8 LEDs) is added
> to the webcam structure and the change is immediatly done in the ioctl.
>
> 3) Development.
> 
> If nobody wants a LED control in the v4l2 interface, I think the code
> added to access the led class could be splitted between the different
> subsystem. For example, the workqueue handling could be done in the led
> class itself...

Advantages of LED subsystem are:
4) Flexible usage of the camera LED for purposes unrelated to camera or
   unusual way, e.g. just blinking the LED with ledtrig-timer.

5) The status of the LED can be read back by reading
   /sys/class/leds/video0::camera/brightness. This is not possible when
   the "auto" menu item is selected from a V4L2 based menu control.

Regards,

	Márton Németh

Re: [PATCH 1/2] gspca pac7302: allow controlling LED separately

[Hans de Goede]

Hi,

On 02/27/2010 04:35 PM, Németh Márton wrote:
> Hi,
> Jean-Francois Moine wrote:
>> On Sat, 27 Feb 2010 08:53:16 +0100
>> Hans de Goede<hdegoede@redhat.com>  wrote:
>>
>>> This is true for a lot of cameras, so if we are going to add a way to
>>> support control of the LED separate of the streaming state, we
>>> should do that at the gspca_main level, and let sub drivers which
>>> support this export a set_led callback function.
>>>
>>> I must say I personally don't see much of a use case for this feature,
>>> but I believe JF Moine does, so I'll leave further comments and
>>> review of this to JF. I do believe it is important that if we go this
>>> way we do so add the gspca_main level.
>>
>> Hi,
>>
>> I don't like this mechanism to switch on or off the webcam LEDs. Here
>> are some arguments (some of them were sent last year to the list):
>
> I could accept both the V4L2 control solution or the LED subclass solution
> for handling the camera LED. I miss a little the positive side of using
> the LED subclass from the list, so I try take the role of that side and
> add them.
>

I have to side with JF here, I see very little use in the led class interface
for webcams. Another important reason to choose for a simple v4l2 menu control
solution here is consistency certain logitech uvc cameras already offer Led control
through a vendor extension control unit, which (when using uvcdynctrl to enable
vendor extension control units), currently shows up as a v4l2 control.

So for consistency with existing practices a v4l2 control seems better suited.

Also I must say that the led class seems overkill.

I would like to note that even if we go the v4l2 control way it still makes sense
to handle this in gspca_main, rather then implementing it separately in every
sub driver.

>> 1) End users.
>>
>> Many Linux users don't know the kernel internals, nor which of the too
>> many applications they should use to make their devices work.
>>
>> Actually, there are a few X11 programs in common Linux distros which can
>> handle the webcam parameters: I just know about vlc and v4l2ucp, and
>> they don't even handle the VIDIOC_G_JPEGCOMP and VIDIOC_S_JPEGCOMP
>> ioctls which are part of the v4l2 API.
>>
>> The LED interface uses the /sys file system. Will the webcam programs
>> offer access to this interface? I don't believe it. So, the users will
>> have to search how they can switch on or off the LEDs of their webcams,
>> and then, when found, they should start a X terminal and run a command
>> to do the job!
>
> The programs like v4l2ucp can be extended to handle the /sys/class/leds
> interface. This is work but the user will not recognise the difference
> as long as the GUI supports it.
>

Erm, no currently almost no v4l programs uses v4l2 controls properly, and
I certainly see none supporting the led sysfs interface. I say this as
someone who has done actual development on v4l2ucp, and  who is currnetly
involved in writing a GTK v4l2 control panel application.

Regards,

Hans

Re: [PATCH 1/2] gspca pac7302: allow controlling LED separately

[Jean-Francois Moine]

On Sat, 27 Feb 2010 20:12:05 +0100
Hans de Goede <hdegoede@redhat.com> wrote:
	[snip]
> I would like to note that even if we go the v4l2 control way it still
> makes sense to handle this in gspca_main, rather then implementing it
> separately in every sub driver.

There is nothing to do in the gspca_main: the control just does a
usb_control write.

> I say this as someone ... who is currnetly
> involved in writing a GTK v4l2 control panel application.

Great!

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