[RFCv1 PATCH 0/7] gspca: allow use of control framework and other fixes

[RFCv1 PATCH 0/7] gspca: allow use of control framework and other fixes

[Hans Verkuil]

Hi all,

Here is a patch series that makes it possible to use the control framework
in gspca. The gspca core changes are very minor but as a bonus give you
priority support as well.

The hard work is in updating the subdrivers. I've done two, and I intend
to do the stv06xx driver as well, but that's the last of my gspca webcams
that I can test. Looking through the subdrivers I think that 50-70% are in
the category 'easy to convert', the others will take a bit more time
(autogain/gain type of constructs are always more complex than just a simple
brightness control).

After applying this patch series the two converted drivers pass the
v4l2-compliance test as it stands today.

Comments? Questions?

Regards.

	Hans

[RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Make the necessary changes to allow subdrivers to use the control framework.
This does not add control event support, that needs more work.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/video/gspca/gspca.c |   13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/media/video/gspca/gspca.c b/drivers/media/video/gspca/gspca.c
index ca5a2b1..56dff10 100644
--- a/drivers/media/video/gspca/gspca.c
+++ b/drivers/media/video/gspca/gspca.c
@@ -38,6 +38,7 @@
 #include <linux/uaccess.h>
 #include <linux/ktime.h>
 #include <media/v4l2-ioctl.h>
+#include <media/v4l2-ctrls.h>
 
 #include "gspca.h"
 
@@ -1006,6 +1007,8 @@ static void gspca_set_default_mode(struct gspca_dev *gspca_dev)
 
 	/* set the current control values to their default values
 	 * which may have changed in sd_init() */
+	/* does nothing if ctrl_handler == NULL */
+	v4l2_ctrl_handler_setup(gspca_dev->vdev.ctrl_handler);
 	ctrl = gspca_dev->cam.ctrls;
 	if (ctrl != NULL) {
 		for (i = 0;
@@ -1323,6 +1326,7 @@ static void gspca_release(struct video_device *vfd)
 	PDEBUG(D_PROBE, "%s released",
 		video_device_node_name(&gspca_dev->vdev));
 
+	v4l2_ctrl_handler_free(gspca_dev->vdev.ctrl_handler);
 	kfree(gspca_dev->usb_buf);
 	kfree(gspca_dev);
 }
@@ -2347,6 +2351,10 @@ int gspca_dev_probe2(struct usb_interface *intf,
 	gspca_dev->sd_desc = sd_desc;
 	gspca_dev->nbufread = 2;
 	gspca_dev->empty_packet = -1;	/* don't check the empty packets */
+	gspca_dev->vdev = gspca_template;
+	gspca_dev->vdev.parent = &intf->dev;
+	gspca_dev->module = module;
+	gspca_dev->present = 1;
 
 	/* configure the subdriver and initialize the USB device */
 	ret = sd_desc->config(gspca_dev, id);
@@ -2368,10 +2376,6 @@ int gspca_dev_probe2(struct usb_interface *intf,
 	init_waitqueue_head(&gspca_dev->wq);
 
 	/* init video stuff */
-	memcpy(&gspca_dev->vdev, &gspca_template, sizeof gspca_template);
-	gspca_dev->vdev.parent = &intf->dev;
-	gspca_dev->module = module;
-	gspca_dev->present = 1;
 	ret = video_register_device(&gspca_dev->vdev,
 				  VFL_TYPE_GRABBER,
 				  -1);
@@ -2391,6 +2395,7 @@ out:
 	if (gspca_dev->input_dev)
 		input_unregister_device(gspca_dev->input_dev);
 #endif
+	v4l2_ctrl_handler_free(gspca_dev->vdev.ctrl_handler);
 	kfree(gspca_dev->usb_buf);
 	kfree(gspca_dev);
 	return ret;
-- 
1.7.10

[RFCv1 PATCH 2/7] zc3xx: convert to the control framework.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/video/gspca/zc3xx.c |  451 +++++++++++++++----------------------
 1 file changed, 182 insertions(+), 269 deletions(-)

diff --git a/drivers/media/video/gspca/zc3xx.c b/drivers/media/video/gspca/zc3xx.c
index 7d9a4f1..e7b7599 100644
--- a/drivers/media/video/gspca/zc3xx.c
+++ b/drivers/media/video/gspca/zc3xx.c
@@ -22,6 +22,7 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/input.h>
+#include <media/v4l2-ctrls.h>
 #include "gspca.h"
 #include "jpeg.h"
 
@@ -35,26 +36,23 @@ static int force_sensor = -1;
 #define REG08_DEF 3		/* default JPEG compression (70%) */
 #include "zc3xx-reg.h"
 
-/* controls */
-enum e_ctrl {
-	BRIGHTNESS,
-	CONTRAST,
-	EXPOSURE,
-	GAMMA,
-	AUTOGAIN,
-	LIGHTFREQ,
-	SHARPNESS,
-	QUALITY,
-	NCTRLS		/* number of controls */
-};
-
-#define AUTOGAIN_DEF 1
-
 /* specific webcam descriptor */
 struct sd {
 	struct gspca_dev gspca_dev;	/* !! must be the first item */
 
-	struct gspca_ctrl ctrls[NCTRLS];
+	struct v4l2_ctrl_handler ctrl_handler;
+	struct { /* gamma/brightness/contrast control cluster */
+		struct v4l2_ctrl *gamma;
+		struct v4l2_ctrl *brightness;
+		struct v4l2_ctrl *contrast;
+	};
+	struct { /* autogain/exposure control cluster */
+		struct v4l2_ctrl *autogain;
+		struct v4l2_ctrl *exposure;
+	};
+	struct v4l2_ctrl *plfreq;
+	struct v4l2_ctrl *sharpness;
+	struct v4l2_ctrl *jpegqual;
 
 	struct work_struct work;
 	struct workqueue_struct *work_thread;
@@ -95,112 +93,6 @@ enum sensors {
 };
 
 /* V4L2 controls supported by the driver */
-static void setcontrast(struct gspca_dev *gspca_dev);
-static void setexposure(struct gspca_dev *gspca_dev);
-static int sd_setautogain(struct gspca_dev *gspca_dev, __s32 val);
-static void setlightfreq(struct gspca_dev *gspca_dev);
-static void setsharpness(struct gspca_dev *gspca_dev);
-static int sd_setquality(struct gspca_dev *gspca_dev, __s32 val);
-
-static const struct ctrl sd_ctrls[NCTRLS] = {
-[BRIGHTNESS] = {
-	    {
-		.id      = V4L2_CID_BRIGHTNESS,
-		.type    = V4L2_CTRL_TYPE_INTEGER,
-		.name    = "Brightness",
-		.minimum = 0,
-		.maximum = 255,
-		.step    = 1,
-		.default_value = 128,
-	    },
-	    .set_control = setcontrast
-	},
-[CONTRAST] = {
-	    {
-		.id      = V4L2_CID_CONTRAST,
-		.type    = V4L2_CTRL_TYPE_INTEGER,
-		.name    = "Contrast",
-		.minimum = 0,
-		.maximum = 255,
-		.step    = 1,
-		.default_value = 128,
-	    },
-	    .set_control = setcontrast
-	},
-[EXPOSURE] = {
-	    {
-		.id      = V4L2_CID_EXPOSURE,
-		.type    = V4L2_CTRL_TYPE_INTEGER,
-		.name    = "Exposure",
-		.minimum = 0x30d,
-		.maximum	= 0x493e,
-		.step		= 1,
-		.default_value  = 0x927
-	    },
-	    .set_control = setexposure
-	},
-[GAMMA] = {
-	    {
-		.id      = V4L2_CID_GAMMA,
-		.type    = V4L2_CTRL_TYPE_INTEGER,
-		.name    = "Gamma",
-		.minimum = 1,
-		.maximum = 6,
-		.step    = 1,
-		.default_value = 4,
-	    },
-	    .set_control = setcontrast
-	},
-[AUTOGAIN] = {
-	    {
-		.id      = V4L2_CID_AUTOGAIN,
-		.type    = V4L2_CTRL_TYPE_BOOLEAN,
-		.name    = "Auto Gain",
-		.minimum = 0,
-		.maximum = 1,
-		.step    = 1,
-		.default_value = AUTOGAIN_DEF,
-		.flags   = V4L2_CTRL_FLAG_UPDATE
-	    },
-	    .set = sd_setautogain
-	},
-[LIGHTFREQ] = {
-	    {
-		.id	 = V4L2_CID_POWER_LINE_FREQUENCY,
-		.type    = V4L2_CTRL_TYPE_MENU,
-		.name    = "Light frequency filter",
-		.minimum = 0,
-		.maximum = 2,	/* 0: 0, 1: 50Hz, 2:60Hz */
-		.step    = 1,
-		.default_value = 0,
-	    },
-	    .set_control = setlightfreq
-	},
-[SHARPNESS] = {
-	    {
-		.id	 = V4L2_CID_SHARPNESS,
-		.type    = V4L2_CTRL_TYPE_INTEGER,
-		.name    = "Sharpness",
-		.minimum = 0,
-		.maximum = 3,
-		.step    = 1,
-		.default_value = 2,
-	    },
-	    .set_control = setsharpness
-	},
-[QUALITY] = {
-	    {
-		.id	 = V4L2_CID_JPEG_COMPRESSION_QUALITY,
-		.type    = V4L2_CTRL_TYPE_INTEGER,
-		.name    = "Compression Quality",
-		.minimum = 40,
-		.maximum = 70,
-		.step    = 1,
-		.default_value = 70	/* updated in sd_init() */
-	    },
-	    .set = sd_setquality
-	},
-};
 
 static const struct v4l2_pix_format vga_mode[] = {
 	{320, 240, V4L2_PIX_FMT_JPEG, V4L2_FIELD_NONE,
@@ -5818,10 +5710,8 @@ static void setmatrix(struct gspca_dev *gspca_dev)
 		reg_w(gspca_dev, matrix[i], 0x010a + i);
 }
 
-static void setsharpness(struct gspca_dev *gspca_dev)
+static void setsharpness(struct gspca_dev *gspca_dev, s32 val)
 {
-	struct sd *sd = (struct sd *) gspca_dev;
-	int sharpness;
 	static const u8 sharpness_tb[][2] = {
 		{0x02, 0x03},
 		{0x04, 0x07},
@@ -5829,19 +5719,18 @@ static void setsharpness(struct gspca_dev *gspca_dev)
 		{0x10, 0x1e}
 	};
 
-	sharpness = sd->ctrls[SHARPNESS].val;
-	reg_w(gspca_dev, sharpness_tb[sharpness][0], 0x01c6);
+	reg_w(gspca_dev, sharpness_tb[val][0], 0x01c6);
 	reg_r(gspca_dev, 0x01c8);
 	reg_r(gspca_dev, 0x01c9);
 	reg_r(gspca_dev, 0x01ca);
-	reg_w(gspca_dev, sharpness_tb[sharpness][1], 0x01cb);
+	reg_w(gspca_dev, sharpness_tb[val][1], 0x01cb);
 }
 
-static void setcontrast(struct gspca_dev *gspca_dev)
+static void setcontrast(struct gspca_dev *gspca_dev,
+		s32 gamma, s32 brightness, s32 contrast)
 {
-	struct sd *sd = (struct sd *) gspca_dev;
 	const u8 *Tgamma;
-	int g, i, brightness, contrast, adj, gp1, gp2;
+	int g, i, adj, gp1, gp2;
 	u8 gr[16];
 	static const u8 delta_b[16] =		/* delta for brightness */
 		{0x50, 0x38, 0x2d, 0x28, 0x24, 0x21, 0x1e, 0x1d,
@@ -5864,10 +5753,10 @@ static void setcontrast(struct gspca_dev *gspca_dev)
 		 0xe0, 0xeb, 0xf4, 0xff, 0xff, 0xff, 0xff, 0xff},
 	};
 
-	Tgamma = gamma_tb[sd->ctrls[GAMMA].val - 1];
+	Tgamma = gamma_tb[gamma - 1];
 
-	contrast = ((int) sd->ctrls[CONTRAST].val - 128); /* -128 / 127 */
-	brightness = ((int) sd->ctrls[BRIGHTNESS].val - 128); /* -128 / 92 */
+	contrast -= 128; /* -128 / 127 */
+	brightness -= 128; /* -128 / 92 */
 	adj = 0;
 	gp1 = gp2 = 0;
 	for (i = 0; i < 16; i++) {
@@ -5894,25 +5783,23 @@ static void setcontrast(struct gspca_dev *gspca_dev)
 		reg_w(gspca_dev, gr[i], 0x0130 + i);	/* gradient */
 }
 
-static void getexposure(struct gspca_dev *gspca_dev)
+static s32 getexposure(struct gspca_dev *gspca_dev)
 {
 	struct sd *sd = (struct sd *) gspca_dev;
 
 	if (sd->sensor != SENSOR_HV7131R)
-		return;
-	sd->ctrls[EXPOSURE].val = (i2c_read(gspca_dev, 0x25) << 9)
+		return 0;
+	return (i2c_read(gspca_dev, 0x25) << 9)
 		| (i2c_read(gspca_dev, 0x26) << 1)
 		| (i2c_read(gspca_dev, 0x27) >> 7);
 }
 
-static void setexposure(struct gspca_dev *gspca_dev)
+static void setexposure(struct gspca_dev *gspca_dev, s32 val)
 {
 	struct sd *sd = (struct sd *) gspca_dev;
-	int val;
 
 	if (sd->sensor != SENSOR_HV7131R)
 		return;
-	val = sd->ctrls[EXPOSURE].val;
 	i2c_write(gspca_dev, 0x25, val >> 9, 0x00);
 	i2c_write(gspca_dev, 0x26, val >> 1, 0x00);
 	i2c_write(gspca_dev, 0x27, val << 7, 0x00);
@@ -5943,7 +5830,7 @@ static void setquality(struct gspca_dev *gspca_dev)
  *	60Hz, for American lighting
  *	0 = No Fliker (for outdoore usage)
  */
-static void setlightfreq(struct gspca_dev *gspca_dev)
+static void setlightfreq(struct gspca_dev *gspca_dev, s32 val)
 {
 	struct sd *sd = (struct sd *) gspca_dev;
 	int i, mode;
@@ -6027,7 +5914,7 @@ static void setlightfreq(struct gspca_dev *gspca_dev)
 		 tas5130c_60HZ, tas5130c_60HZScale},
 	};
 
-	i = sd->ctrls[LIGHTFREQ].val * 2;
+	i = val * 2;
 	mode = gspca_dev->cam.cam_mode[gspca_dev->curr_mode].priv;
 	if (mode)
 		i++;			/* 320x240 */
@@ -6037,14 +5924,14 @@ static void setlightfreq(struct gspca_dev *gspca_dev)
 	usb_exchange(gspca_dev, zc3_freq);
 	switch (sd->sensor) {
 	case SENSOR_GC0305:
-		if (mode				/* if 320x240 */
-		    && sd->ctrls[LIGHTFREQ].val == 1)	/* and 50Hz */
+		if (mode		/* if 320x240 */
+		    && val == 1)	/* and 50Hz */
 			reg_w(gspca_dev, 0x85, 0x018d);
 					/* win: 0x80, 0x018d */
 		break;
 	case SENSOR_OV7620:
-		if (!mode) {				/* if 640x480 */
-			if (sd->ctrls[LIGHTFREQ].val != 0) /* and filter */
+		if (!mode) {		/* if 640x480 */
+			if (val != 0)	/* and filter */
 				reg_w(gspca_dev, 0x40, 0x0002);
 			else
 				reg_w(gspca_dev, 0x44, 0x0002);
@@ -6056,16 +5943,9 @@ static void setlightfreq(struct gspca_dev *gspca_dev)
 	}
 }
 
-static void setautogain(struct gspca_dev *gspca_dev)
+static void setautogain(struct gspca_dev *gspca_dev, s32 val)
 {
-	struct sd *sd = (struct sd *) gspca_dev;
-	u8 autoval;
-
-	if (sd->ctrls[AUTOGAIN].val)
-		autoval = 0x42;
-	else
-		autoval = 0x02;
-	reg_w(gspca_dev, autoval, 0x0180);
+	reg_w(gspca_dev, val ? 0x42 : 0x02, 0x0180);
 }
 
 /* update the transfer parameters */
@@ -6165,7 +6045,6 @@ static void transfer_update(struct work_struct *work)
 				 || !gspca_dev->present
 				 || !gspca_dev->streaming)
 					goto err;
-				sd->ctrls[QUALITY].val = jpeg_qual[sd->reg08];
 				jpeg_set_qual(sd->jpeg_hdr,
 						jpeg_qual[sd->reg08]);
 			}
@@ -6503,7 +6382,6 @@ static int sd_config(struct gspca_dev *gspca_dev,
 	/* define some sensors from the vendor/product */
 	sd->sensor = id->driver_info;
 
-	gspca_dev->cam.ctrls = sd->ctrls;
 	sd->reg08 = REG08_DEF;
 
 	INIT_WORK(&sd->work, transfer_update);
@@ -6511,10 +6389,117 @@ static int sd_config(struct gspca_dev *gspca_dev,
 	return 0;
 }
 
+static int sd_setautogain(struct gspca_dev *gspca_dev, s32 val)
+{
+	if (!gspca_dev->streaming)
+		return 0;
+	setautogain(gspca_dev, val);
+
+	return gspca_dev->usb_err;
+}
+
+static int sd_setquality(struct gspca_dev *gspca_dev, s32 val)
+{
+	struct sd *sd = (struct sd *) gspca_dev;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(jpeg_qual) - 1; i++) {
+		if (val <= jpeg_qual[i])
+			break;
+	}
+	sd->reg08 = i;
+	if (!gspca_dev->streaming)
+		return 0;
+	jpeg_set_qual(sd->jpeg_hdr, val);
+	return gspca_dev->usb_err;
+}
+
+static int sd_set_jcomp(struct gspca_dev *gspca_dev,
+			struct v4l2_jpegcompression *jcomp)
+{
+	struct sd *sd = (struct sd *) gspca_dev;
+
+	if (sd->jpegqual) {
+		v4l2_ctrl_s_ctrl(sd->jpegqual, jcomp->quality);
+		jcomp->quality = v4l2_ctrl_g_ctrl(sd->jpegqual);
+		return gspca_dev->usb_err;
+	}
+	jcomp->quality = jpeg_qual[sd->reg08];
+	return 0;
+}
+
+static int sd_get_jcomp(struct gspca_dev *gspca_dev,
+			struct v4l2_jpegcompression *jcomp)
+{
+	struct sd *sd = (struct sd *) gspca_dev;
+
+	memset(jcomp, 0, sizeof *jcomp);
+	jcomp->quality = jpeg_qual[sd->reg08];
+	jcomp->jpeg_markers = V4L2_JPEG_MARKER_DHT
+			| V4L2_JPEG_MARKER_DQT;
+	return 0;
+}
+
+static int zcxx_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct sd *sd = container_of(ctrl->handler, struct sd, ctrl_handler);
+
+	switch (ctrl->id) {
+	case V4L2_CID_AUTOGAIN:
+		if (ctrl->val && sd->exposure && sd->gspca_dev.streaming)
+			sd->exposure->val = getexposure(&sd->gspca_dev);
+		break;
+	}
+	return 0;
+}
+
+static int zcxx_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct sd *sd = container_of(ctrl->handler, struct sd, ctrl_handler);
+	int ret;
+	int i;
+
+	switch (ctrl->id) {
+	/* gamma/brightness/contrast cluster */
+	case V4L2_CID_GAMMA:
+		setcontrast(&sd->gspca_dev, sd->gamma->val,
+				sd->brightness->val, sd->contrast->val);
+		return 0;
+	/* autogain/exposure cluster */
+	case V4L2_CID_AUTOGAIN:
+		ret = sd_setautogain(&sd->gspca_dev, ctrl->val);
+		if (!ret && !ctrl->val && sd->exposure)
+			setexposure(&sd->gspca_dev, sd->exposure->val);
+		return ret;
+	case V4L2_CID_POWER_LINE_FREQUENCY:
+		setlightfreq(&sd->gspca_dev, ctrl->val);
+		return 0;
+	case V4L2_CID_SHARPNESS:
+		setsharpness(&sd->gspca_dev, ctrl->val);
+		return 0;
+	case V4L2_CID_JPEG_COMPRESSION_QUALITY:
+		for (i = 0; i < ARRAY_SIZE(jpeg_qual) - 1; i++) {
+			if (ctrl->val <= jpeg_qual[i])
+				break;
+		}
+		if (i > 0 && i == sd->reg08 && ctrl->val < jpeg_qual[sd->reg08])
+			i--;
+		ctrl->val = jpeg_qual[i];
+		return sd_setquality(&sd->gspca_dev, ctrl->val);
+	}
+	return -EINVAL;
+}
+
+static const struct v4l2_ctrl_ops zcxx_ctrl_ops = {
+	.g_volatile_ctrl = zcxx_g_volatile_ctrl,
+	.s_ctrl = zcxx_s_ctrl,
+};
+
 /* this function is called at probe and resume time */
 static int sd_init(struct gspca_dev *gspca_dev)
 {
 	struct sd *sd = (struct sd *) gspca_dev;
+	struct v4l2_ctrl_handler *hdl = &sd->ctrl_handler;
 	struct cam *cam;
 	int sensor;
 	static const u8 gamma[SENSOR_MAX] = {
@@ -6688,7 +6673,6 @@ static int sd_init(struct gspca_dev *gspca_dev)
 		case 0x2030:
 			PDEBUG(D_PROBE, "Find Sensor PO2030");
 			sd->sensor = SENSOR_PO2030;
-			sd->ctrls[SHARPNESS].def = 0;	/* from win traces */
 			break;
 		case 0x7620:
 			PDEBUG(D_PROBE, "Find Sensor OV7620");
@@ -6730,30 +6714,40 @@ static int sd_init(struct gspca_dev *gspca_dev)
 		break;
 	}
 
-	sd->ctrls[GAMMA].def = gamma[sd->sensor];
-	sd->reg08 = reg08_tb[sd->sensor];
-	sd->ctrls[QUALITY].def = jpeg_qual[sd->reg08];
-	sd->ctrls[QUALITY].min = jpeg_qual[0];
-	sd->ctrls[QUALITY].max = jpeg_qual[ARRAY_SIZE(jpeg_qual) - 1];
-
-	switch (sd->sensor) {
-	case SENSOR_HV7131R:
-		gspca_dev->ctrl_dis = (1 << QUALITY);
-		break;
-	case SENSOR_OV7630C:
-		gspca_dev->ctrl_dis = (1 << LIGHTFREQ) | (1 << EXPOSURE);
-		break;
-	case SENSOR_PAS202B:
-		gspca_dev->ctrl_dis = (1 << QUALITY) | (1 << EXPOSURE);
-		break;
-	default:
-		gspca_dev->ctrl_dis = (1 << EXPOSURE);
-		break;
+	gspca_dev->vdev.ctrl_handler = hdl;
+	v4l2_ctrl_handler_init(hdl, 8);
+	sd->brightness = v4l2_ctrl_new_std(hdl, &zcxx_ctrl_ops,
+			V4L2_CID_BRIGHTNESS, 0, 255, 1, 128);
+	sd->contrast = v4l2_ctrl_new_std(hdl, &zcxx_ctrl_ops,
+			V4L2_CID_CONTRAST, 0, 255, 1, 128);
+	sd->gamma = v4l2_ctrl_new_std(hdl, &zcxx_ctrl_ops,
+			V4L2_CID_GAMMA, 1, 6, 1, gamma[sd->sensor]);
+	if (sd->sensor == SENSOR_HV7131R)
+		sd->exposure = v4l2_ctrl_new_std(hdl, &zcxx_ctrl_ops,
+			V4L2_CID_EXPOSURE, 0x30d, 0x493e, 1, 0x927);
+	sd->autogain = v4l2_ctrl_new_std(hdl, &zcxx_ctrl_ops,
+			V4L2_CID_AUTOGAIN, 0, 1, 1, 1);
+	if (sd->sensor != SENSOR_OV7630C && sd->sensor != SENSOR_PAS202B)
+		sd->plfreq = v4l2_ctrl_new_std_menu(hdl, &zcxx_ctrl_ops,
+			V4L2_CID_POWER_LINE_FREQUENCY,
+			V4L2_CID_POWER_LINE_FREQUENCY_60HZ, 0,
+			V4L2_CID_POWER_LINE_FREQUENCY_DISABLED);
+	sd->sharpness = v4l2_ctrl_new_std(hdl, &zcxx_ctrl_ops,
+			V4L2_CID_SHARPNESS, 0, 3, 1,
+			sd->sensor == SENSOR_PO2030 ? 0 : 2);
+	if (sd->sensor != SENSOR_HV7131R && sd->sensor != SENSOR_PAS202B)
+		sd->jpegqual = v4l2_ctrl_new_std(hdl, &zcxx_ctrl_ops,
+			V4L2_CID_JPEG_COMPRESSION_QUALITY,
+			jpeg_qual[0], jpeg_qual[ARRAY_SIZE(jpeg_qual) - 1], 1,
+			jpeg_qual[sd->reg08]);
+	if (hdl->error) {
+		pr_err("Could not initialize controls\n");
+		return hdl->error;
 	}
-#if AUTOGAIN_DEF
-	if (sd->ctrls[AUTOGAIN].val)
-		gspca_dev->ctrl_inac = (1 << EXPOSURE);
-#endif
+	v4l2_ctrl_cluster(3, &sd->gamma);
+	if (sd->sensor == SENSOR_HV7131R)
+		v4l2_ctrl_auto_cluster(2, &sd->autogain, 0, true);
+	sd->reg08 = reg08_tb[sd->sensor];
 
 	/* switch off the led */
 	reg_w(gspca_dev, 0x01, 0x0000);
@@ -6864,7 +6858,7 @@ static int sd_start(struct gspca_dev *gspca_dev)
 		reg_w(gspca_dev, 0x03, 0x0008);
 		break;
 	}
-	setsharpness(gspca_dev);
+	setsharpness(gspca_dev, v4l2_ctrl_g_ctrl(sd->sharpness));
 
 	/* set the gamma tables when not set */
 	switch (sd->sensor) {
@@ -6873,7 +6867,9 @@ static int sd_start(struct gspca_dev *gspca_dev)
 	case SENSOR_OV7630C:
 		break;
 	default:
-		setcontrast(gspca_dev);
+		setcontrast(&sd->gspca_dev, v4l2_ctrl_g_ctrl(sd->gamma),
+				v4l2_ctrl_g_ctrl(sd->brightness),
+				v4l2_ctrl_g_ctrl(sd->contrast));
 		break;
 	}
 	setmatrix(gspca_dev);			/* one more time? */
@@ -6886,7 +6882,7 @@ static int sd_start(struct gspca_dev *gspca_dev)
 	}
 	setquality(gspca_dev);
 	jpeg_set_qual(sd->jpeg_hdr, jpeg_qual[sd->reg08]);
-	setlightfreq(gspca_dev);
+	setlightfreq(gspca_dev, v4l2_ctrl_g_ctrl(sd->plfreq));
 
 	switch (sd->sensor) {
 	case SENSOR_ADCM2700:
@@ -6897,7 +6893,7 @@ static int sd_start(struct gspca_dev *gspca_dev)
 		reg_w(gspca_dev, 0x40, 0x0117);
 		break;
 	case SENSOR_HV7131R:
-		setexposure(gspca_dev);
+		setexposure(gspca_dev, v4l2_ctrl_g_ctrl(sd->exposure));
 		reg_w(gspca_dev, 0x00, ZC3XX_R1A7_CALCGLOBALMEAN);
 		break;
 	case SENSOR_GC0305:
@@ -6921,7 +6917,7 @@ static int sd_start(struct gspca_dev *gspca_dev)
 		break;
 	}
 
-	setautogain(gspca_dev);
+	setautogain(gspca_dev, v4l2_ctrl_g_ctrl(sd->autogain));
 
 	/* start the transfer update thread if needed */
 	if (gspca_dev->usb_err >= 0) {
@@ -6987,86 +6983,6 @@ static void sd_pkt_scan(struct gspca_dev *gspca_dev,
 	gspca_frame_add(gspca_dev, INTER_PACKET, data, len);
 }
 
-static int sd_setautogain(struct gspca_dev *gspca_dev, __s32 val)
-{
-	struct sd *sd = (struct sd *) gspca_dev;
-
-	sd->ctrls[AUTOGAIN].val = val;
-	if (val) {
-		gspca_dev->ctrl_inac |= (1 << EXPOSURE);
-	} else {
-		gspca_dev->ctrl_inac &= ~(1 << EXPOSURE);
-		if (gspca_dev->streaming)
-			getexposure(gspca_dev);
-	}
-	if (gspca_dev->streaming)
-		setautogain(gspca_dev);
-	return gspca_dev->usb_err;
-}
-
-static int sd_querymenu(struct gspca_dev *gspca_dev,
-			struct v4l2_querymenu *menu)
-{
-	switch (menu->id) {
-	case V4L2_CID_POWER_LINE_FREQUENCY:
-		switch (menu->index) {
-		case 0:		/* V4L2_CID_POWER_LINE_FREQUENCY_DISABLED */
-			strcpy((char *) menu->name, "NoFliker");
-			return 0;
-		case 1:		/* V4L2_CID_POWER_LINE_FREQUENCY_50HZ */
-			strcpy((char *) menu->name, "50 Hz");
-			return 0;
-		case 2:		/* V4L2_CID_POWER_LINE_FREQUENCY_60HZ */
-			strcpy((char *) menu->name, "60 Hz");
-			return 0;
-		}
-		break;
-	}
-	return -EINVAL;
-}
-
-static int sd_setquality(struct gspca_dev *gspca_dev, __s32 val)
-{
-	struct sd *sd = (struct sd *) gspca_dev;
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(jpeg_qual) - 1; i++) {
-		if (val <= jpeg_qual[i])
-			break;
-	}
-	if (i > 0
-	 && i == sd->reg08
-	 && val < jpeg_qual[sd->reg08])
-		i--;
-	sd->reg08 = i;
-	sd->ctrls[QUALITY].val = jpeg_qual[i];
-	if (gspca_dev->streaming)
-		jpeg_set_qual(sd->jpeg_hdr, sd->ctrls[QUALITY].val);
-	return gspca_dev->usb_err;
-}
-
-static int sd_set_jcomp(struct gspca_dev *gspca_dev,
-			struct v4l2_jpegcompression *jcomp)
-{
-	struct sd *sd = (struct sd *) gspca_dev;
-
-	sd_setquality(gspca_dev, jcomp->quality);
-	jcomp->quality = sd->ctrls[QUALITY].val;
-	return gspca_dev->usb_err;
-}
-
-static int sd_get_jcomp(struct gspca_dev *gspca_dev,
-			struct v4l2_jpegcompression *jcomp)
-{
-	struct sd *sd = (struct sd *) gspca_dev;
-
-	memset(jcomp, 0, sizeof *jcomp);
-	jcomp->quality = sd->ctrls[QUALITY].val;
-	jcomp->jpeg_markers = V4L2_JPEG_MARKER_DHT
-			| V4L2_JPEG_MARKER_DQT;
-	return 0;
-}
-
 #if defined(CONFIG_INPUT) || defined(CONFIG_INPUT_MODULE)
 static int sd_int_pkt_scan(struct gspca_dev *gspca_dev,
 			u8 *data,		/* interrupt packet data */
@@ -7085,14 +7001,11 @@ static int sd_int_pkt_scan(struct gspca_dev *gspca_dev,
 
 static const struct sd_desc sd_desc = {
 	.name = KBUILD_MODNAME,
-	.ctrls = sd_ctrls,
-	.nctrls = ARRAY_SIZE(sd_ctrls),
 	.config = sd_config,
 	.init = sd_init,
 	.start = sd_start,
 	.stop0 = sd_stop0,
 	.pkt_scan = sd_pkt_scan,
-	.querymenu = sd_querymenu,
 	.get_jcomp = sd_get_jcomp,
 	.set_jcomp = sd_set_jcomp,
 #if defined(CONFIG_INPUT) || defined(CONFIG_INPUT_MODULE)
-- 
1.7.10

[RFCv1 PATCH 3/7] sn9c20x: convert to the control framework.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/video/gspca/sn9c20x.c |  481 ++++++++++++++---------------------
 1 file changed, 198 insertions(+), 283 deletions(-)

diff --git a/drivers/media/video/gspca/sn9c20x.c b/drivers/media/video/gspca/sn9c20x.c
index 7e71aa2..ed51556 100644
--- a/drivers/media/video/gspca/sn9c20x.c
+++ b/drivers/media/video/gspca/sn9c20x.c
@@ -28,6 +28,7 @@
 #include "jpeg.h"
 
 #include <media/v4l2-chip-ident.h>
+#include <media/v4l2-ctrls.h>
 #include <linux/dmi.h>
 
 MODULE_AUTHOR("Brian Johnson <brijohn@gmail.com>, "
@@ -66,28 +67,32 @@ MODULE_LICENSE("GPL");
 #define LED_REVERSE	0x2 /* some cameras unset gpio to turn on leds */
 #define FLIP_DETECT	0x4
 
-enum e_ctrl {
-	BRIGHTNESS,
-	CONTRAST,
-	SATURATION,
-	HUE,
-	GAMMA,
-	BLUE,
-	RED,
-	VFLIP,
-	HFLIP,
-	EXPOSURE,
-	GAIN,
-	AUTOGAIN,
-	QUALITY,
-	NCTRLS		/* number of controls */
-};
-
 /* specific webcam descriptor */
 struct sd {
 	struct gspca_dev gspca_dev;
 
-	struct gspca_ctrl ctrls[NCTRLS];
+	struct v4l2_ctrl_handler ctrl_handler;
+	struct { /* color control cluster */
+		struct v4l2_ctrl *brightness;
+		struct v4l2_ctrl *contrast;
+		struct v4l2_ctrl *saturation;
+		struct v4l2_ctrl *hue;
+	};
+	struct { /* blue/red balance control cluster */
+		struct v4l2_ctrl *blue;
+		struct v4l2_ctrl *red;
+	};
+	struct { /* h/vflip control cluster */
+		struct v4l2_ctrl *hflip;
+		struct v4l2_ctrl *vflip;
+	};
+	struct v4l2_ctrl *gamma;
+	struct { /* autogain and exposure or gain control cluster */
+		struct v4l2_ctrl *autogain;
+		struct v4l2_ctrl *exposure;
+		struct v4l2_ctrl *gain;
+	};
+	struct v4l2_ctrl *jpegqual;
 
 	struct work_struct work;
 	struct workqueue_struct *work_thread;
@@ -166,175 +171,6 @@ static const struct dmi_system_id flip_dmi_table[] = {
 	{}
 };
 
-static void set_cmatrix(struct gspca_dev *gspca_dev);
-static void set_gamma(struct gspca_dev *gspca_dev);
-static void set_redblue(struct gspca_dev *gspca_dev);
-static void set_hvflip(struct gspca_dev *gspca_dev);
-static void set_exposure(struct gspca_dev *gspca_dev);
-static void set_gain(struct gspca_dev *gspca_dev);
-static void set_quality(struct gspca_dev *gspca_dev);
-
-static const struct ctrl sd_ctrls[NCTRLS] = {
-[BRIGHTNESS] = {
-	    {
-		.id      = V4L2_CID_BRIGHTNESS,
-		.type    = V4L2_CTRL_TYPE_INTEGER,
-		.name    = "Brightness",
-		.minimum = 0,
-		.maximum = 0xff,
-		.step    = 1,
-		.default_value = 0x7f
-	    },
-	    .set_control = set_cmatrix
-	},
-[CONTRAST] = {
-	    {
-		.id      = V4L2_CID_CONTRAST,
-		.type    = V4L2_CTRL_TYPE_INTEGER,
-		.name    = "Contrast",
-		.minimum = 0,
-		.maximum = 0xff,
-		.step    = 1,
-		.default_value = 0x7f
-	    },
-	    .set_control = set_cmatrix
-	},
-[SATURATION] = {
-	    {
-		.id      = V4L2_CID_SATURATION,
-		.type    = V4L2_CTRL_TYPE_INTEGER,
-		.name    = "Saturation",
-		.minimum = 0,
-		.maximum = 0xff,
-		.step    = 1,
-		.default_value = 0x7f
-	    },
-	    .set_control = set_cmatrix
-	},
-[HUE] = {
-	    {
-		.id      = V4L2_CID_HUE,
-		.type    = V4L2_CTRL_TYPE_INTEGER,
-		.name    = "Hue",
-		.minimum = -180,
-		.maximum = 180,
-		.step    = 1,
-		.default_value = 0
-	    },
-	    .set_control = set_cmatrix
-	},
-[GAMMA] = {
-	    {
-		.id      = V4L2_CID_GAMMA,
-		.type    = V4L2_CTRL_TYPE_INTEGER,
-		.name    = "Gamma",
-		.minimum = 0,
-		.maximum = 0xff,
-		.step    = 1,
-		.default_value = 0x10
-	    },
-	    .set_control = set_gamma
-	},
-[BLUE] = {
-	    {
-		.id	 = V4L2_CID_BLUE_BALANCE,
-		.type	 = V4L2_CTRL_TYPE_INTEGER,
-		.name	 = "Blue Balance",
-		.minimum = 0,
-		.maximum = 0x7f,
-		.step	 = 1,
-		.default_value = 0x28
-	    },
-	    .set_control = set_redblue
-	},
-[RED] = {
-	    {
-		.id	 = V4L2_CID_RED_BALANCE,
-		.type	 = V4L2_CTRL_TYPE_INTEGER,
-		.name	 = "Red Balance",
-		.minimum = 0,
-		.maximum = 0x7f,
-		.step	 = 1,
-		.default_value = 0x28
-	    },
-	    .set_control = set_redblue
-	},
-[HFLIP] = {
-	    {
-		.id      = V4L2_CID_HFLIP,
-		.type    = V4L2_CTRL_TYPE_BOOLEAN,
-		.name    = "Horizontal Flip",
-		.minimum = 0,
-		.maximum = 1,
-		.step    = 1,
-		.default_value = 0,
-	    },
-	    .set_control = set_hvflip
-	},
-[VFLIP] = {
-	    {
-		.id      = V4L2_CID_VFLIP,
-		.type    = V4L2_CTRL_TYPE_BOOLEAN,
-		.name    = "Vertical Flip",
-		.minimum = 0,
-		.maximum = 1,
-		.step    = 1,
-		.default_value = 0,
-	    },
-	    .set_control = set_hvflip
-	},
-[EXPOSURE] = {
-	    {
-		.id      = V4L2_CID_EXPOSURE,
-		.type    = V4L2_CTRL_TYPE_INTEGER,
-		.name    = "Exposure",
-		.minimum = 0,
-		.maximum = 0x1780,
-		.step    = 1,
-		.default_value = 0x33,
-	    },
-	    .set_control = set_exposure
-	},
-[GAIN] = {
-	    {
-		.id      = V4L2_CID_GAIN,
-		.type    = V4L2_CTRL_TYPE_INTEGER,
-		.name    = "Gain",
-		.minimum = 0,
-		.maximum = 28,
-		.step    = 1,
-		.default_value = 0,
-	    },
-	    .set_control = set_gain
-	},
-[AUTOGAIN] = {
-	    {
-		.id      = V4L2_CID_AUTOGAIN,
-		.type    = V4L2_CTRL_TYPE_BOOLEAN,
-		.name    = "Auto Exposure",
-		.minimum = 0,
-		.maximum = 1,
-		.step    = 1,
-		.default_value = 1,
-	    },
-	},
-[QUALITY] = {
-	    {
-		.id      = V4L2_CID_JPEG_COMPRESSION_QUALITY,
-		.type    = V4L2_CTRL_TYPE_INTEGER,
-		.name    = "Compression Quality",
-#define QUALITY_MIN 50
-#define QUALITY_MAX 90
-#define QUALITY_DEF 80
-		.minimum = QUALITY_MIN,
-		.maximum = QUALITY_MAX,
-		.step    = 1,
-		.default_value = QUALITY_DEF,
-	    },
-	    .set_control = set_quality
-	},
-};
-
 static const struct v4l2_pix_format vga_mode[] = {
 	{160, 120, V4L2_PIX_FMT_JPEG, V4L2_FIELD_NONE,
 		.bytesperline = 160,
@@ -1294,8 +1130,6 @@ static void ov9655_init_sensor(struct gspca_dev *gspca_dev)
 	if (gspca_dev->usb_err < 0)
 		pr_err("OV9655 sensor initialization failed\n");
 
-	/* disable hflip and vflip */
-	gspca_dev->ctrl_dis = (1 << HFLIP) | (1 << VFLIP);
 	sd->hstart = 1;
 	sd->vstart = 2;
 }
@@ -1310,9 +1144,6 @@ static void soi968_init_sensor(struct gspca_dev *gspca_dev)
 	if (gspca_dev->usb_err < 0)
 		pr_err("SOI968 sensor initialization failed\n");
 
-	/* disable hflip and vflip */
-	gspca_dev->ctrl_dis = (1 << HFLIP) | (1 << VFLIP)
-				| (1 << EXPOSURE);
 	sd->hstart = 60;
 	sd->vstart = 11;
 }
@@ -1340,8 +1171,6 @@ static void ov7670_init_sensor(struct gspca_dev *gspca_dev)
 	if (gspca_dev->usb_err < 0)
 		pr_err("OV7670 sensor initialization failed\n");
 
-	/* disable hflip and vflip */
-	gspca_dev->ctrl_dis = (1 << HFLIP) | (1 << VFLIP);
 	sd->hstart = 0;
 	sd->vstart = 1;
 }
@@ -1378,9 +1207,6 @@ static void mt9v_init_sensor(struct gspca_dev *gspca_dev)
 			pr_err("MT9V111 sensor initialization failed\n");
 			return;
 		}
-		gspca_dev->ctrl_dis = (1 << EXPOSURE)
-					| (1 << AUTOGAIN)
-					| (1 << GAIN);
 		sd->hstart = 2;
 		sd->vstart = 2;
 		sd->sensor = SENSOR_MT9V111;
@@ -1422,8 +1248,6 @@ static void mt9m112_init_sensor(struct gspca_dev *gspca_dev)
 	if (gspca_dev->usb_err < 0)
 		pr_err("MT9M112 sensor initialization failed\n");
 
-	gspca_dev->ctrl_dis = (1 << EXPOSURE) | (1 << AUTOGAIN)
-				| (1 << GAIN);
 	sd->hstart = 0;
 	sd->vstart = 2;
 }
@@ -1436,8 +1260,6 @@ static void mt9m111_init_sensor(struct gspca_dev *gspca_dev)
 	if (gspca_dev->usb_err < 0)
 		pr_err("MT9M111 sensor initialization failed\n");
 
-	gspca_dev->ctrl_dis = (1 << EXPOSURE) | (1 << AUTOGAIN)
-				| (1 << GAIN);
 	sd->hstart = 0;
 	sd->vstart = 2;
 }
@@ -1470,8 +1292,6 @@ static void mt9m001_init_sensor(struct gspca_dev *gspca_dev)
 	if (gspca_dev->usb_err < 0)
 		pr_err("MT9M001 sensor initialization failed\n");
 
-	/* disable hflip and vflip */
-	gspca_dev->ctrl_dis = (1 << HFLIP) | (1 << VFLIP);
 	sd->hstart = 1;
 	sd->vstart = 1;
 }
@@ -1488,20 +1308,18 @@ static void hv7131r_init_sensor(struct gspca_dev *gspca_dev)
 	sd->vstart = 1;
 }
 
-static void set_cmatrix(struct gspca_dev *gspca_dev)
+static void set_cmatrix(struct gspca_dev *gspca_dev,
+		s32 brightness, s32 contrast, s32 satur, s32 hue)
 {
-	struct sd *sd = (struct sd *) gspca_dev;
-	int satur;
-	s32 hue_coord, hue_index = 180 + sd->ctrls[HUE].val;
+	s32 hue_coord, hue_index = 180 + hue;
 	u8 cmatrix[21];
 
 	memset(cmatrix, 0, sizeof cmatrix);
-	cmatrix[2] = (sd->ctrls[CONTRAST].val * 0x25 / 0x100) + 0x26;
+	cmatrix[2] = (contrast * 0x25 / 0x100) + 0x26;
 	cmatrix[0] = 0x13 + (cmatrix[2] - 0x26) * 0x13 / 0x25;
 	cmatrix[4] = 0x07 + (cmatrix[2] - 0x26) * 0x07 / 0x25;
-	cmatrix[18] = sd->ctrls[BRIGHTNESS].val - 0x80;
+	cmatrix[18] = brightness - 0x80;
 
-	satur = sd->ctrls[SATURATION].val;
 	hue_coord = (hsv_red_x[hue_index] * satur) >> 8;
 	cmatrix[6] = hue_coord;
 	cmatrix[7] = (hue_coord >> 8) & 0x0f;
@@ -1529,11 +1347,10 @@ static void set_cmatrix(struct gspca_dev *gspca_dev)
 	reg_w(gspca_dev, 0x10e1, cmatrix, 21);
 }
 
-static void set_gamma(struct gspca_dev *gspca_dev)
+static void set_gamma(struct gspca_dev *gspca_dev, s32 val)
 {
-	struct sd *sd = (struct sd *) gspca_dev;
 	u8 gamma[17];
-	u8 gval = sd->ctrls[GAMMA].val * 0xb8 / 0x100;
+	u8 gval = val * 0xb8 / 0x100;
 
 	gamma[0] = 0x0a;
 	gamma[1] = 0x13 + (gval * (0xcb - 0x13) / 0xb8);
@@ -1556,26 +1373,21 @@ static void set_gamma(struct gspca_dev *gspca_dev)
 	reg_w(gspca_dev, 0x1190, gamma, 17);
 }
 
-static void set_redblue(struct gspca_dev *gspca_dev)
+static void set_redblue(struct gspca_dev *gspca_dev, s32 blue, s32 red)
 {
-	struct sd *sd = (struct sd *) gspca_dev;
-
-	reg_w1(gspca_dev, 0x118c, sd->ctrls[RED].val);
-	reg_w1(gspca_dev, 0x118f, sd->ctrls[BLUE].val);
+	reg_w1(gspca_dev, 0x118c, red);
+	reg_w1(gspca_dev, 0x118f, blue);
 }
 
-static void set_hvflip(struct gspca_dev *gspca_dev)
+static void set_hvflip(struct gspca_dev *gspca_dev, s32 hflip, s32 vflip)
 {
-	u8 value, tslb, hflip, vflip;
+	u8 value, tslb;
 	u16 value2;
 	struct sd *sd = (struct sd *) gspca_dev;
 
 	if ((sd->flags & FLIP_DETECT) && dmi_check_system(flip_dmi_table)) {
-		hflip = !sd->ctrls[HFLIP].val;
-		vflip = !sd->ctrls[VFLIP].val;
-	} else {
-		hflip = sd->ctrls[HFLIP].val;
-		vflip = sd->ctrls[VFLIP].val;
+		hflip = !hflip;
+		vflip = !vflip;
 	}
 
 	switch (sd->sensor) {
@@ -1638,13 +1450,11 @@ static void set_hvflip(struct gspca_dev *gspca_dev)
 	}
 }
 
-static void set_exposure(struct gspca_dev *gspca_dev)
+static void set_exposure(struct gspca_dev *gspca_dev, s32 expo)
 {
 	struct sd *sd = (struct sd *) gspca_dev;
 	u8 exp[8] = {0x81, sd->i2c_addr, 0x00, 0x00, 0x00, 0x00, 0x00, 0x1e};
-	int expo;
 
-	expo = sd->ctrls[EXPOSURE].val;
 	switch (sd->sensor) {
 	case SENSOR_OV7660:
 	case SENSOR_OV7670:
@@ -1676,13 +1486,11 @@ static void set_exposure(struct gspca_dev *gspca_dev)
 	i2c_w(gspca_dev, exp);
 }
 
-static void set_gain(struct gspca_dev *gspca_dev)
+static void set_gain(struct gspca_dev *gspca_dev, s32 g)
 {
 	struct sd *sd = (struct sd *) gspca_dev;
 	u8 gain[8] = {0x81, sd->i2c_addr, 0x00, 0x00, 0x00, 0x00, 0x00, 0x1d};
-	int g;
 
-	g = sd->ctrls[GAIN].val;
 	switch (sd->sensor) {
 	case SENSOR_OV7660:
 	case SENSOR_OV7670:
@@ -1721,11 +1529,11 @@ static void set_gain(struct gspca_dev *gspca_dev)
 	i2c_w(gspca_dev, gain);
 }
 
-static void set_quality(struct gspca_dev *gspca_dev)
+static void set_quality(struct gspca_dev *gspca_dev, s32 val)
 {
 	struct sd *sd = (struct sd *) gspca_dev;
 
-	jpeg_set_qual(sd->jpeg_hdr, sd->ctrls[QUALITY].val);
+	jpeg_set_qual(sd->jpeg_hdr, val);
 	reg_w1(gspca_dev, 0x1061, 0x01);	/* stop transfer */
 	reg_w1(gspca_dev, 0x10e0, sd->fmt | 0x20); /* write QTAB */
 	reg_w(gspca_dev, 0x1100, &sd->jpeg_hdr[JPEG_QT0_OFFSET], 64);
@@ -1850,16 +1658,62 @@ static int sd_config(struct gspca_dev *gspca_dev,
 	sd->older_step = 0;
 	sd->exposure_step = 16;
 
-	gspca_dev->cam.ctrls = sd->ctrls;
-
 	INIT_WORK(&sd->work, qual_upd);
 
 	return 0;
 }
 
+static int sd_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct sd *sd = container_of(ctrl->handler, struct sd, ctrl_handler);
+
+	switch (ctrl->id) {
+	/* color control cluster */
+	case V4L2_CID_BRIGHTNESS:
+		set_cmatrix(&sd->gspca_dev, sd->brightness->val,
+			sd->contrast->val, sd->saturation->val, sd->hue->val);
+		return 0;
+	case V4L2_CID_GAMMA:
+		set_gamma(&sd->gspca_dev, ctrl->val);
+		return 0;
+	/* blue/red balance cluster */
+	case V4L2_CID_BLUE_BALANCE:
+		set_redblue(&sd->gspca_dev, sd->blue->val, sd->red->val);
+		return 0;
+	/* h/vflip cluster */
+	case V4L2_CID_HFLIP:
+		set_hvflip(&sd->gspca_dev, sd->hflip->val, sd->vflip->val);
+		return 0;
+	/* standalone exposure control */
+	case V4L2_CID_EXPOSURE:
+		set_exposure(&sd->gspca_dev, ctrl->val);
+		return 0;
+	/* standalone gain control */
+	case V4L2_CID_GAIN:
+		set_gain(&sd->gspca_dev, ctrl->val);
+		return 0;
+	/* autogain + exposure or gain control cluster */
+	case V4L2_CID_AUTOGAIN:
+		if (sd->sensor == SENSOR_SOI968)
+			set_gain(&sd->gspca_dev, sd->gain->val);
+		else
+			set_exposure(&sd->gspca_dev, sd->exposure->val);
+		return 0;
+	case V4L2_CID_JPEG_COMPRESSION_QUALITY:
+		set_quality(&sd->gspca_dev, ctrl->val);
+		return 0;
+	}
+	return -EINVAL;
+}
+
+static const struct v4l2_ctrl_ops sd_ctrl_ops = {
+	.s_ctrl = sd_s_ctrl,
+};
+
 static int sd_init(struct gspca_dev *gspca_dev)
 {
 	struct sd *sd = (struct sd *) gspca_dev;
+	struct v4l2_ctrl_handler *hdl = &sd->ctrl_handler;
 	int i;
 	u8 value;
 	u8 i2c_init[9] =
@@ -1949,8 +1803,67 @@ static int sd_init(struct gspca_dev *gspca_dev)
 		pr_err("Unsupported sensor\n");
 		gspca_dev->usb_err = -ENODEV;
 	}
+	if (gspca_dev->usb_err)
+		return gspca_dev->usb_err;
+	gspca_dev->vdev.ctrl_handler = hdl;
+	v4l2_ctrl_handler_init(hdl, 13);
+
+	sd->brightness = v4l2_ctrl_new_std(hdl, &sd_ctrl_ops,
+			V4L2_CID_BRIGHTNESS, 0, 255, 1, 127);
+	sd->contrast = v4l2_ctrl_new_std(hdl, &sd_ctrl_ops,
+			V4L2_CID_CONTRAST, 0, 255, 1, 127);
+	sd->saturation = v4l2_ctrl_new_std(hdl, &sd_ctrl_ops,
+			V4L2_CID_SATURATION, 0, 255, 1, 127);
+	sd->hue = v4l2_ctrl_new_std(hdl, &sd_ctrl_ops,
+			V4L2_CID_HUE, -180, 180, 1, 0);
+	v4l2_ctrl_cluster(4, &sd->brightness);
+
+	sd->gamma = v4l2_ctrl_new_std(hdl, &sd_ctrl_ops,
+			V4L2_CID_GAMMA, 0, 255, 1, 0x10);
+	sd->blue = v4l2_ctrl_new_std(hdl, &sd_ctrl_ops,
+			V4L2_CID_BLUE_BALANCE, 0, 127, 1, 0x28);
+	sd->red = v4l2_ctrl_new_std(hdl, &sd_ctrl_ops,
+			V4L2_CID_RED_BALANCE, 0, 127, 1, 0x28);
+	v4l2_ctrl_cluster(2, &sd->blue);
+
+	if (sd->sensor != SENSOR_OV9650 && sd->sensor != SENSOR_SOI968 &&
+	    sd->sensor != SENSOR_OV7670 && sd->sensor != SENSOR_MT9M001) {
+		sd->hflip = v4l2_ctrl_new_std(hdl, &sd_ctrl_ops,
+			V4L2_CID_HFLIP, 0, 1, 1, 0);
+		sd->vflip = v4l2_ctrl_new_std(hdl, &sd_ctrl_ops,
+			V4L2_CID_VFLIP, 0, 1, 1, 0);
+		v4l2_ctrl_cluster(2, &sd->hflip);
+	}
 
-	return gspca_dev->usb_err;
+	if (sd->sensor != SENSOR_SOI968 && sd->sensor != SENSOR_MT9VPRB &&
+	    sd->sensor != SENSOR_MT9M112 && sd->sensor != SENSOR_MT9M111)
+		sd->exposure = v4l2_ctrl_new_std(hdl, &sd_ctrl_ops,
+			V4L2_CID_EXPOSURE, 0, 0x1780, 1, 0x33);
+
+	if (sd->sensor != SENSOR_MT9VPRB && sd->sensor != SENSOR_MT9M112 &&
+	    sd->sensor != SENSOR_MT9M111) {
+		sd->gain = v4l2_ctrl_new_std(hdl, &sd_ctrl_ops,
+			V4L2_CID_GAIN, 0, 28, 1, 0);
+		sd->autogain = v4l2_ctrl_new_std(hdl, &sd_ctrl_ops,
+			V4L2_CID_AUTOGAIN, 0, 1, 1, 1);
+		if (sd->sensor == SENSOR_SOI968)
+			/* this sensor doesn't have the exposure control and
+			   autogain is clustered with gain instead. This works
+			   because sd->exposure == NULL. */
+			v4l2_ctrl_auto_cluster(3, &sd->autogain, 0, false);
+		else
+			/* Otherwise autogain is clustered with exposure. */
+			v4l2_ctrl_auto_cluster(2, &sd->autogain, 0, false);
+	}
+
+	sd->jpegqual = v4l2_ctrl_new_std(hdl, &sd_ctrl_ops,
+			V4L2_CID_JPEG_COMPRESSION_QUALITY, 50, 90, 1, 80);
+	if (hdl->error) {
+		pr_err("Could not initialize controls\n");
+		return hdl->error;
+	}
+
+	return 0;
 }
 
 static void configure_sensor_output(struct gspca_dev *gspca_dev, int mode)
@@ -2067,7 +1980,7 @@ static int sd_start(struct gspca_dev *gspca_dev)
 
 	jpeg_define(sd->jpeg_hdr, height, width,
 			0x21);
-	jpeg_set_qual(sd->jpeg_hdr, sd->ctrls[QUALITY].val);
+	jpeg_set_qual(sd->jpeg_hdr, v4l2_ctrl_g_ctrl(sd->jpegqual));
 
 	if (mode & MODE_RAW)
 		fmt = 0x2d;
@@ -2104,12 +2017,17 @@ static int sd_start(struct gspca_dev *gspca_dev)
 	reg_w1(gspca_dev, 0x1189, scale);
 	reg_w1(gspca_dev, 0x10e0, fmt);
 
-	set_cmatrix(gspca_dev);
-	set_gamma(gspca_dev);
-	set_redblue(gspca_dev);
-	set_gain(gspca_dev);
-	set_exposure(gspca_dev);
-	set_hvflip(gspca_dev);
+	set_cmatrix(gspca_dev, v4l2_ctrl_g_ctrl(sd->brightness),
+			v4l2_ctrl_g_ctrl(sd->contrast),
+			v4l2_ctrl_g_ctrl(sd->saturation),
+			v4l2_ctrl_g_ctrl(sd->hue));
+	set_gamma(gspca_dev, v4l2_ctrl_g_ctrl(sd->gamma));
+	set_redblue(gspca_dev, v4l2_ctrl_g_ctrl(sd->blue),
+			v4l2_ctrl_g_ctrl(sd->red));
+	set_gain(gspca_dev, v4l2_ctrl_g_ctrl(sd->gain));
+	set_exposure(gspca_dev, v4l2_ctrl_g_ctrl(sd->exposure));
+	set_hvflip(gspca_dev, v4l2_ctrl_g_ctrl(sd->hflip),
+			v4l2_ctrl_g_ctrl(sd->vflip));
 
 	reg_w1(gspca_dev, 0x1007, 0x20);
 	reg_w1(gspca_dev, 0x1061, 0x03);
@@ -2148,6 +2066,9 @@ static void sd_stop0(struct gspca_dev *gspca_dev)
 static void do_autoexposure(struct gspca_dev *gspca_dev, u16 avg_lum)
 {
 	struct sd *sd = (struct sd *) gspca_dev;
+	s32 cur_exp = v4l2_ctrl_g_ctrl(sd->exposure);
+	s32 max = sd->exposure->maximum - sd->exposure_step;
+	s32 min = sd->exposure->minimum + sd->exposure_step;
 	s16 new_exp;
 
 	/*
@@ -2156,16 +2077,15 @@ static void do_autoexposure(struct gspca_dev *gspca_dev, u16 avg_lum)
 	 * and exposure steps
 	 */
 	if (avg_lum < MIN_AVG_LUM) {
-		if (sd->ctrls[EXPOSURE].val > 0x1770)
+		if (cur_exp > max)
 			return;
 
-		new_exp = sd->ctrls[EXPOSURE].val + sd->exposure_step;
-		if (new_exp > 0x1770)
-			new_exp = 0x1770;
-		if (new_exp < 0x10)
-			new_exp = 0x10;
-		sd->ctrls[EXPOSURE].val = new_exp;
-		set_exposure(gspca_dev);
+		new_exp = cur_exp + sd->exposure_step;
+		if (new_exp > max)
+			new_exp = max;
+		if (new_exp < min)
+			new_exp = min;
+		v4l2_ctrl_s_ctrl(sd->exposure, new_exp);
 
 		sd->older_step = sd->old_step;
 		sd->old_step = 1;
@@ -2176,15 +2096,14 @@ static void do_autoexposure(struct gspca_dev *gspca_dev, u16 avg_lum)
 			sd->exposure_step += 2;
 	}
 	if (avg_lum > MAX_AVG_LUM) {
-		if (sd->ctrls[EXPOSURE].val < 0x10)
+		if (cur_exp < min)
 			return;
-		new_exp = sd->ctrls[EXPOSURE].val - sd->exposure_step;
-		if (new_exp > 0x1700)
-			new_exp = 0x1770;
-		if (new_exp < 0x10)
-			new_exp = 0x10;
-		sd->ctrls[EXPOSURE].val = new_exp;
-		set_exposure(gspca_dev);
+		new_exp = cur_exp - sd->exposure_step;
+		if (new_exp > max)
+			new_exp = max;
+		if (new_exp < min)
+			new_exp = min;
+		v4l2_ctrl_s_ctrl(sd->exposure, new_exp);
 		sd->older_step = sd->old_step;
 		sd->old_step = 0;
 
@@ -2198,19 +2117,12 @@ static void do_autoexposure(struct gspca_dev *gspca_dev, u16 avg_lum)
 static void do_autogain(struct gspca_dev *gspca_dev, u16 avg_lum)
 {
 	struct sd *sd = (struct sd *) gspca_dev;
+	s32 cur_gain = v4l2_ctrl_g_ctrl(sd->gain);
 
-	if (avg_lum < MIN_AVG_LUM) {
-		if (sd->ctrls[GAIN].val + 1 <= 28) {
-			sd->ctrls[GAIN].val++;
-			set_gain(gspca_dev);
-		}
-	}
-	if (avg_lum > MAX_AVG_LUM) {
-		if (sd->ctrls[GAIN].val > 0) {
-			sd->ctrls[GAIN].val--;
-			set_gain(gspca_dev);
-		}
-	}
+	if (avg_lum < MIN_AVG_LUM && cur_gain < sd->gain->maximum)
+		v4l2_ctrl_s_ctrl(sd->gain, cur_gain + 1);
+	if (avg_lum > MAX_AVG_LUM && cur_gain > sd->gain->minimum)
+		v4l2_ctrl_s_ctrl(sd->gain, cur_gain - 1);
 }
 
 static void sd_dqcallback(struct gspca_dev *gspca_dev)
@@ -2218,7 +2130,7 @@ static void sd_dqcallback(struct gspca_dev *gspca_dev)
 	struct sd *sd = (struct sd *) gspca_dev;
 	int avg_lum;
 
-	if (!sd->ctrls[AUTOGAIN].val)
+	if (!v4l2_ctrl_g_ctrl(sd->autogain))
 		return;
 
 	avg_lum = atomic_read(&sd->avg_lum);
@@ -2234,10 +2146,11 @@ static void qual_upd(struct work_struct *work)
 {
 	struct sd *sd = container_of(work, struct sd, work);
 	struct gspca_dev *gspca_dev = &sd->gspca_dev;
+	s32 qual = v4l2_ctrl_g_ctrl(sd->jpegqual);
 
 	mutex_lock(&gspca_dev->usb_lock);
-	PDEBUG(D_STREAM, "qual_upd %d%%", sd->ctrls[QUALITY].val);
-	set_quality(gspca_dev);
+	PDEBUG(D_STREAM, "qual_upd %d%%", qual);
+	set_quality(gspca_dev, qual);
 	mutex_unlock(&gspca_dev->usb_lock);
 }
 
@@ -2286,14 +2199,18 @@ static void transfer_check(struct gspca_dev *gspca_dev,
 	if (new_qual != 0) {
 		sd->nchg += new_qual;
 		if (sd->nchg < -6 || sd->nchg >= 12) {
+			/* Note: we are in interrupt context, so we can't
+			   use v4l2_ctrl_g/s_ctrl here. Access the value
+			   directly instead. */
+			s32 curqual = sd->jpegqual->cur.val;
 			sd->nchg = 0;
-			new_qual += sd->ctrls[QUALITY].val;
-			if (new_qual < QUALITY_MIN)
-				new_qual = QUALITY_MIN;
-			else if (new_qual > QUALITY_MAX)
-				new_qual = QUALITY_MAX;
-			if (new_qual != sd->ctrls[QUALITY].val) {
-				sd->ctrls[QUALITY].val = new_qual;
+			new_qual += curqual;
+			if (new_qual < sd->jpegqual->minimum)
+				new_qual = sd->jpegqual->minimum;
+			else if (new_qual > sd->jpegqual->maximum)
+				new_qual = sd->jpegqual->maximum;
+			if (new_qual != curqual) {
+				sd->jpegqual->cur.val = new_qual;
 				queue_work(sd->work_thread, &sd->work);
 			}
 		}
@@ -2373,8 +2290,6 @@ static void sd_pkt_scan(struct gspca_dev *gspca_dev,
 /* sub-driver description */
 static const struct sd_desc sd_desc = {
 	.name = KBUILD_MODNAME,
-	.ctrls = sd_ctrls,
-	.nctrls = ARRAY_SIZE(sd_ctrls),
 	.config = sd_config,
 	.init = sd_init,
 	.isoc_init = sd_isoc_init,
-- 
1.7.10

[RFCv1 PATCH 4/7] gspca: use video_drvdata(file) instead of file->private_data.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Prepare for control events: free up file->private_data by using
video_drvdata(file) to get to the gspca_dev struct.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/video/gspca/gspca.c |   64 ++++++++++++++++++-------------------
 1 file changed, 31 insertions(+), 33 deletions(-)

diff --git a/drivers/media/video/gspca/gspca.c b/drivers/media/video/gspca/gspca.c
index 56dff10..5c1b53e 100644
--- a/drivers/media/video/gspca/gspca.c
+++ b/drivers/media/video/gspca/gspca.c
@@ -1061,7 +1061,7 @@ static int vidioc_g_register(struct file *file, void *priv,
 			struct v4l2_dbg_register *reg)
 {
 	int ret;
-	struct gspca_dev *gspca_dev = priv;
+	struct gspca_dev *gspca_dev = video_drvdata(file);
 
 	if (!gspca_dev->sd_desc->get_chip_ident)
 		return -EINVAL;
@@ -1085,7 +1085,7 @@ static int vidioc_s_register(struct file *file, void *priv,
 			struct v4l2_dbg_register *reg)
 {
 	int ret;
-	struct gspca_dev *gspca_dev = priv;
+	struct gspca_dev *gspca_dev = video_drvdata(file);
 
 	if (!gspca_dev->sd_desc->get_chip_ident)
 		return -EINVAL;
@@ -1110,7 +1110,7 @@ static int vidioc_g_chip_ident(struct file *file, void *priv,
 			struct v4l2_dbg_chip_ident *chip)
 {
 	int ret;
-	struct gspca_dev *gspca_dev = priv;
+	struct gspca_dev *gspca_dev = video_drvdata(file);
 
 	if (!gspca_dev->sd_desc->get_chip_ident)
 		return -EINVAL;
@@ -1130,7 +1130,7 @@ static int vidioc_g_chip_ident(struct file *file, void *priv,
 static int vidioc_enum_fmt_vid_cap(struct file *file, void  *priv,
 				struct v4l2_fmtdesc *fmtdesc)
 {
-	struct gspca_dev *gspca_dev = priv;
+	struct gspca_dev *gspca_dev = video_drvdata(file);
 	int i, j, index;
 	__u32 fmt_tb[8];
 
@@ -1172,7 +1172,7 @@ static int vidioc_enum_fmt_vid_cap(struct file *file, void  *priv,
 static int vidioc_g_fmt_vid_cap(struct file *file, void *priv,
 			    struct v4l2_format *fmt)
 {
-	struct gspca_dev *gspca_dev = priv;
+	struct gspca_dev *gspca_dev = video_drvdata(file);
 	int mode;
 
 	mode = gspca_dev->curr_mode;
@@ -1217,7 +1217,7 @@ static int vidioc_try_fmt_vid_cap(struct file *file,
 			      void *priv,
 			      struct v4l2_format *fmt)
 {
-	struct gspca_dev *gspca_dev = priv;
+	struct gspca_dev *gspca_dev = video_drvdata(file);
 	int ret;
 
 	ret = try_fmt_vid_cap(gspca_dev, fmt);
@@ -1229,7 +1229,7 @@ static int vidioc_try_fmt_vid_cap(struct file *file,
 static int vidioc_s_fmt_vid_cap(struct file *file, void *priv,
 			    struct v4l2_format *fmt)
 {
-	struct gspca_dev *gspca_dev = priv;
+	struct gspca_dev *gspca_dev = video_drvdata(file);
 	int ret;
 
 	if (mutex_lock_interruptible(&gspca_dev->queue_lock))
@@ -1268,7 +1268,7 @@ out:
 static int vidioc_enum_framesizes(struct file *file, void *priv,
 				  struct v4l2_frmsizeenum *fsize)
 {
-	struct gspca_dev *gspca_dev = priv;
+	struct gspca_dev *gspca_dev = video_drvdata(file);
 	int i;
 	__u32 index = 0;
 
@@ -1294,7 +1294,7 @@ static int vidioc_enum_framesizes(struct file *file, void *priv,
 static int vidioc_enum_frameintervals(struct file *filp, void *priv,
 				      struct v4l2_frmivalenum *fival)
 {
-	struct gspca_dev *gspca_dev = priv;
+	struct gspca_dev *gspca_dev = video_drvdata(filp);
 	int mode = wxh_to_mode(gspca_dev, fival->width, fival->height);
 	__u32 i;
 
@@ -1333,10 +1333,9 @@ static void gspca_release(struct video_device *vfd)
 
 static int dev_open(struct file *file)
 {
-	struct gspca_dev *gspca_dev;
+	struct gspca_dev *gspca_dev = video_drvdata(file);
 
 	PDEBUG(D_STREAM, "[%s] open", current->comm);
-	gspca_dev = (struct gspca_dev *) video_devdata(file);
 	if (!gspca_dev->present)
 		return -ENODEV;
 
@@ -1344,7 +1343,6 @@ static int dev_open(struct file *file)
 	if (!try_module_get(gspca_dev->module))
 		return -ENODEV;
 
-	file->private_data = gspca_dev;
 #ifdef GSPCA_DEBUG
 	/* activate the v4l2 debug */
 	if (gspca_debug & D_V4L2)
@@ -1359,7 +1357,7 @@ static int dev_open(struct file *file)
 
 static int dev_close(struct file *file)
 {
-	struct gspca_dev *gspca_dev = file->private_data;
+	struct gspca_dev *gspca_dev = video_drvdata(file);
 
 	PDEBUG(D_STREAM, "[%s] close", current->comm);
 	if (mutex_lock_interruptible(&gspca_dev->queue_lock))
@@ -1375,7 +1373,6 @@ static int dev_close(struct file *file)
 		}
 		frame_free(gspca_dev);
 	}
-	file->private_data = NULL;
 	module_put(gspca_dev->module);
 	mutex_unlock(&gspca_dev->queue_lock);
 
@@ -1387,7 +1384,7 @@ static int dev_close(struct file *file)
 static int vidioc_querycap(struct file *file, void  *priv,
 			   struct v4l2_capability *cap)
 {
-	struct gspca_dev *gspca_dev = priv;
+	struct gspca_dev *gspca_dev = video_drvdata(file);
 	int ret;
 
 	/* protect the access to the usb device */
@@ -1439,7 +1436,7 @@ static int get_ctrl(struct gspca_dev *gspca_dev,
 static int vidioc_queryctrl(struct file *file, void *priv,
 			   struct v4l2_queryctrl *q_ctrl)
 {
-	struct gspca_dev *gspca_dev = priv;
+	struct gspca_dev *gspca_dev = video_drvdata(file);
 	const struct ctrl *ctrls;
 	struct gspca_ctrl *gspca_ctrl;
 	int i, idx;
@@ -1482,7 +1479,7 @@ static int vidioc_queryctrl(struct file *file, void *priv,
 static int vidioc_s_ctrl(struct file *file, void *priv,
 			 struct v4l2_control *ctrl)
 {
-	struct gspca_dev *gspca_dev = priv;
+	struct gspca_dev *gspca_dev = video_drvdata(file);
 	const struct ctrl *ctrls;
 	struct gspca_ctrl *gspca_ctrl;
 	int idx, ret;
@@ -1531,7 +1528,7 @@ out:
 static int vidioc_g_ctrl(struct file *file, void *priv,
 			 struct v4l2_control *ctrl)
 {
-	struct gspca_dev *gspca_dev = priv;
+	struct gspca_dev *gspca_dev = video_drvdata(file);
 	const struct ctrl *ctrls;
 	int idx, ret;
 
@@ -1562,7 +1559,7 @@ out:
 static int vidioc_querymenu(struct file *file, void *priv,
 			    struct v4l2_querymenu *qmenu)
 {
-	struct gspca_dev *gspca_dev = priv;
+	struct gspca_dev *gspca_dev = video_drvdata(file);
 
 	if (!gspca_dev->sd_desc->querymenu)
 		return -EINVAL;
@@ -1572,7 +1569,7 @@ static int vidioc_querymenu(struct file *file, void *priv,
 static int vidioc_enum_input(struct file *file, void *priv,
 				struct v4l2_input *input)
 {
-	struct gspca_dev *gspca_dev = priv;
+	struct gspca_dev *gspca_dev = video_drvdata(file);
 
 	if (input->index != 0)
 		return -EINVAL;
@@ -1599,7 +1596,7 @@ static int vidioc_s_input(struct file *file, void *priv, unsigned int i)
 static int vidioc_reqbufs(struct file *file, void *priv,
 			  struct v4l2_requestbuffers *rb)
 {
-	struct gspca_dev *gspca_dev = priv;
+	struct gspca_dev *gspca_dev = video_drvdata(file);
 	int i, ret = 0, streaming;
 
 	i = rb->memory;			/* (avoid compilation warning) */
@@ -1670,7 +1667,7 @@ out:
 static int vidioc_querybuf(struct file *file, void *priv,
 			   struct v4l2_buffer *v4l2_buf)
 {
-	struct gspca_dev *gspca_dev = priv;
+	struct gspca_dev *gspca_dev = video_drvdata(file);
 	struct gspca_frame *frame;
 
 	if (v4l2_buf->index < 0
@@ -1685,7 +1682,7 @@ static int vidioc_querybuf(struct file *file, void *priv,
 static int vidioc_streamon(struct file *file, void *priv,
 			   enum v4l2_buf_type buf_type)
 {
-	struct gspca_dev *gspca_dev = priv;
+	struct gspca_dev *gspca_dev = video_drvdata(file);
 	int ret;
 
 	if (buf_type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
@@ -1726,7 +1723,7 @@ out:
 static int vidioc_streamoff(struct file *file, void *priv,
 				enum v4l2_buf_type buf_type)
 {
-	struct gspca_dev *gspca_dev = priv;
+	struct gspca_dev *gspca_dev = video_drvdata(file);
 	int ret;
 
 	if (buf_type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
@@ -1770,7 +1767,7 @@ out:
 static int vidioc_g_jpegcomp(struct file *file, void *priv,
 			struct v4l2_jpegcompression *jpegcomp)
 {
-	struct gspca_dev *gspca_dev = priv;
+	struct gspca_dev *gspca_dev = video_drvdata(file);
 	int ret;
 
 	if (!gspca_dev->sd_desc->get_jcomp)
@@ -1789,7 +1786,7 @@ static int vidioc_g_jpegcomp(struct file *file, void *priv,
 static int vidioc_s_jpegcomp(struct file *file, void *priv,
 			struct v4l2_jpegcompression *jpegcomp)
 {
-	struct gspca_dev *gspca_dev = priv;
+	struct gspca_dev *gspca_dev = video_drvdata(file);
 	int ret;
 
 	if (!gspca_dev->sd_desc->set_jcomp)
@@ -1808,7 +1805,7 @@ static int vidioc_s_jpegcomp(struct file *file, void *priv,
 static int vidioc_g_parm(struct file *filp, void *priv,
 			struct v4l2_streamparm *parm)
 {
-	struct gspca_dev *gspca_dev = priv;
+	struct gspca_dev *gspca_dev = video_drvdata(filp);
 
 	parm->parm.capture.readbuffers = gspca_dev->nbufread;
 
@@ -1834,7 +1831,7 @@ static int vidioc_g_parm(struct file *filp, void *priv,
 static int vidioc_s_parm(struct file *filp, void *priv,
 			struct v4l2_streamparm *parm)
 {
-	struct gspca_dev *gspca_dev = priv;
+	struct gspca_dev *gspca_dev = video_drvdata(filp);
 	int n;
 
 	n = parm->parm.capture.readbuffers;
@@ -1864,7 +1861,7 @@ static int vidioc_s_parm(struct file *filp, void *priv,
 
 static int dev_mmap(struct file *file, struct vm_area_struct *vma)
 {
-	struct gspca_dev *gspca_dev = file->private_data;
+	struct gspca_dev *gspca_dev = video_drvdata(file);
 	struct gspca_frame *frame;
 	struct page *page;
 	unsigned long addr, start, size;
@@ -1967,7 +1964,7 @@ static int frame_ready(struct gspca_dev *gspca_dev, struct file *file,
 static int vidioc_dqbuf(struct file *file, void *priv,
 			struct v4l2_buffer *v4l2_buf)
 {
-	struct gspca_dev *gspca_dev = priv;
+	struct gspca_dev *gspca_dev = video_drvdata(file);
 	struct gspca_frame *frame;
 	int i, j, ret;
 
@@ -2043,7 +2040,7 @@ out:
 static int vidioc_qbuf(struct file *file, void *priv,
 			struct v4l2_buffer *v4l2_buf)
 {
-	struct gspca_dev *gspca_dev = priv;
+	struct gspca_dev *gspca_dev = video_drvdata(file);
 	struct gspca_frame *frame;
 	int i, index, ret;
 
@@ -2137,7 +2134,7 @@ static int read_alloc(struct gspca_dev *gspca_dev,
 
 static unsigned int dev_poll(struct file *file, poll_table *wait)
 {
-	struct gspca_dev *gspca_dev = file->private_data;
+	struct gspca_dev *gspca_dev = video_drvdata(file);
 	int ret;
 
 	PDEBUG(D_FRAM, "poll");
@@ -2168,7 +2165,7 @@ static unsigned int dev_poll(struct file *file, poll_table *wait)
 static ssize_t dev_read(struct file *file, char __user *data,
 		    size_t count, loff_t *ppos)
 {
-	struct gspca_dev *gspca_dev = file->private_data;
+	struct gspca_dev *gspca_dev = video_drvdata(file);
 	struct gspca_frame *frame;
 	struct v4l2_buffer v4l2_buf;
 	struct timeval timestamp;
@@ -2353,6 +2350,7 @@ int gspca_dev_probe2(struct usb_interface *intf,
 	gspca_dev->empty_packet = -1;	/* don't check the empty packets */
 	gspca_dev->vdev = gspca_template;
 	gspca_dev->vdev.parent = &intf->dev;
+	video_set_drvdata(&gspca_dev->vdev, gspca_dev);
 	gspca_dev->module = module;
 	gspca_dev->present = 1;
 
-- 
1.7.10

[RFCv1 PATCH 5/7] gscpa: use v4l2_fh and add G/S_PRIORITY support.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

In order to support control event gspca has to use struct v4l2_fh.
As a bonus feature this also gives priority handling for free.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/video/gspca/gspca.c |   13 ++++++++++---
 drivers/media/video/gspca/gspca.h |    2 ++
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/media/video/gspca/gspca.c b/drivers/media/video/gspca/gspca.c
index 5c1b53e..3d3b780 100644
--- a/drivers/media/video/gspca/gspca.c
+++ b/drivers/media/video/gspca/gspca.c
@@ -39,6 +39,7 @@
 #include <linux/ktime.h>
 #include <media/v4l2-ioctl.h>
 #include <media/v4l2-ctrls.h>
+#include <media/v4l2-fh.h>
 
 #include "gspca.h"
 
@@ -1327,6 +1328,7 @@ static void gspca_release(struct video_device *vfd)
 		video_device_node_name(&gspca_dev->vdev));
 
 	v4l2_ctrl_handler_free(gspca_dev->vdev.ctrl_handler);
+	v4l2_device_unregister(&gspca_dev->v4l2_dev);
 	kfree(gspca_dev->usb_buf);
 	kfree(gspca_dev);
 }
@@ -1352,7 +1354,7 @@ static int dev_open(struct file *file)
 		gspca_dev->vdev.debug &= ~(V4L2_DEBUG_IOCTL
 					| V4L2_DEBUG_IOCTL_ARG);
 #endif
-	return 0;
+	return v4l2_fh_open(file);
 }
 
 static int dev_close(struct file *file)
@@ -1378,7 +1380,7 @@ static int dev_close(struct file *file)
 
 	PDEBUG(D_STREAM, "close done");
 
-	return 0;
+	return v4l2_fh_release(file);
 }
 
 static int vidioc_querycap(struct file *file, void  *priv,
@@ -2345,12 +2347,16 @@ int gspca_dev_probe2(struct usb_interface *intf,
 		}
 	}
 
+	ret = v4l2_device_register(&intf->dev, &gspca_dev->v4l2_dev);
+	if (ret)
+		goto out;
 	gspca_dev->sd_desc = sd_desc;
 	gspca_dev->nbufread = 2;
 	gspca_dev->empty_packet = -1;	/* don't check the empty packets */
 	gspca_dev->vdev = gspca_template;
-	gspca_dev->vdev.parent = &intf->dev;
+	gspca_dev->vdev.v4l2_dev = &gspca_dev->v4l2_dev;
 	video_set_drvdata(&gspca_dev->vdev, gspca_dev);
+	set_bit(V4L2_FL_USE_FH_PRIO, &gspca_dev->vdev.flags);
 	gspca_dev->module = module;
 	gspca_dev->present = 1;
 
@@ -2458,6 +2464,7 @@ void gspca_disconnect(struct usb_interface *intf)
 
 	/* the device is freed at exit of this function */
 	gspca_dev->dev = NULL;
+	v4l2_device_disconnect(&gspca_dev->v4l2_dev);
 	mutex_unlock(&gspca_dev->usb_lock);
 
 	usb_set_intfdata(intf, NULL);
diff --git a/drivers/media/video/gspca/gspca.h b/drivers/media/video/gspca/gspca.h
index 589009f..2aacfa3 100644
--- a/drivers/media/video/gspca/gspca.h
+++ b/drivers/media/video/gspca/gspca.h
@@ -6,6 +6,7 @@
 #include <linux/usb.h>
 #include <linux/videodev2.h>
 #include <media/v4l2-common.h>
+#include <media/v4l2-device.h>
 #include <linux/mutex.h>
 
 /* compilation option */
@@ -158,6 +159,7 @@ struct gspca_frame {
 struct gspca_dev {
 	struct video_device vdev;	/* !! must be the first item */
 	struct module *module;		/* subdriver handling the device */
+	struct v4l2_device v4l2_dev;
 	struct usb_device *dev;
 	struct file *capt_file;		/* file doing video capture */
 #if defined(CONFIG_INPUT) || defined(CONFIG_INPUT_MODULE)
-- 
1.7.10

[RFCv1 PATCH 6/7] gspca: add support for control events.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/video/gspca/gspca.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/video/gspca/gspca.c b/drivers/media/video/gspca/gspca.c
index 3d3b780..7b03f36 100644
--- a/drivers/media/video/gspca/gspca.c
+++ b/drivers/media/video/gspca/gspca.c
@@ -40,6 +40,7 @@
 #include <media/v4l2-ioctl.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-fh.h>
+#include <media/v4l2-event.h>
 
 #include "gspca.h"
 
@@ -2158,6 +2159,7 @@ static unsigned int dev_poll(struct file *file, poll_table *wait)
 		ret = POLLIN | POLLRDNORM;	/* yes */
 	else
 		ret = 0;
+	ret |= v4l2_ctrl_poll(file, wait);
 	mutex_unlock(&gspca_dev->queue_lock);
 	if (!gspca_dev->present)
 		return POLLHUP;
@@ -2269,6 +2271,8 @@ static const struct v4l2_ioctl_ops dev_ioctl_ops = {
 	.vidioc_s_register	= vidioc_s_register,
 #endif
 	.vidioc_g_chip_ident	= vidioc_g_chip_ident,
+	.vidioc_subscribe_event = v4l2_ctrl_subscribe_event,
+	.vidioc_unsubscribe_event = v4l2_event_unsubscribe,
 };
 
 static const struct video_device gspca_template = {
-- 
1.7.10

[RFCv1 PATCH 7/7] gspca: fix querycap and incorrect return codes.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Add V4L2_CAP_DEVICE_CAPS support to querycap and replace -EINVAL by
-ENOTTY whenever an ioctl is not supported.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/video/gspca/gspca.c |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/media/video/gspca/gspca.c b/drivers/media/video/gspca/gspca.c
index 7b03f36..734eacd 100644
--- a/drivers/media/video/gspca/gspca.c
+++ b/drivers/media/video/gspca/gspca.c
@@ -1066,10 +1066,10 @@ static int vidioc_g_register(struct file *file, void *priv,
 	struct gspca_dev *gspca_dev = video_drvdata(file);
 
 	if (!gspca_dev->sd_desc->get_chip_ident)
-		return -EINVAL;
+		return -ENOTTY;
 
 	if (!gspca_dev->sd_desc->get_register)
-		return -EINVAL;
+		return -ENOTTY;
 
 	if (mutex_lock_interruptible(&gspca_dev->usb_lock))
 		return -ERESTARTSYS;
@@ -1090,10 +1090,10 @@ static int vidioc_s_register(struct file *file, void *priv,
 	struct gspca_dev *gspca_dev = video_drvdata(file);
 
 	if (!gspca_dev->sd_desc->get_chip_ident)
-		return -EINVAL;
+		return -ENOTTY;
 
 	if (!gspca_dev->sd_desc->set_register)
-		return -EINVAL;
+		return -ENOTTY;
 
 	if (mutex_lock_interruptible(&gspca_dev->usb_lock))
 		return -ERESTARTSYS;
@@ -1115,7 +1115,7 @@ static int vidioc_g_chip_ident(struct file *file, void *priv,
 	struct gspca_dev *gspca_dev = video_drvdata(file);
 
 	if (!gspca_dev->sd_desc->get_chip_ident)
-		return -EINVAL;
+		return -ENOTTY;
 
 	if (mutex_lock_interruptible(&gspca_dev->usb_lock))
 		return -ERESTARTSYS;
@@ -1410,9 +1410,10 @@ static int vidioc_querycap(struct file *file, void  *priv,
 	}
 	usb_make_path(gspca_dev->dev, (char *) cap->bus_info,
 			sizeof(cap->bus_info));
-	cap->capabilities = V4L2_CAP_VIDEO_CAPTURE
+	cap->device_caps = V4L2_CAP_VIDEO_CAPTURE
 			  | V4L2_CAP_STREAMING
 			  | V4L2_CAP_READWRITE;
+	cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
 	ret = 0;
 out:
 	mutex_unlock(&gspca_dev->usb_lock);
@@ -1565,7 +1566,7 @@ static int vidioc_querymenu(struct file *file, void *priv,
 	struct gspca_dev *gspca_dev = video_drvdata(file);
 
 	if (!gspca_dev->sd_desc->querymenu)
-		return -EINVAL;
+		return -ENOTTY;
 	return gspca_dev->sd_desc->querymenu(gspca_dev, qmenu);
 }
 
@@ -1774,7 +1775,7 @@ static int vidioc_g_jpegcomp(struct file *file, void *priv,
 	int ret;
 
 	if (!gspca_dev->sd_desc->get_jcomp)
-		return -EINVAL;
+		return -ENOTTY;
 	if (mutex_lock_interruptible(&gspca_dev->usb_lock))
 		return -ERESTARTSYS;
 	gspca_dev->usb_err = 0;
@@ -1793,7 +1794,7 @@ static int vidioc_s_jpegcomp(struct file *file, void *priv,
 	int ret;
 
 	if (!gspca_dev->sd_desc->set_jcomp)
-		return -EINVAL;
+		return -ENOTTY;
 	if (mutex_lock_interruptible(&gspca_dev->usb_lock))
 		return -ERESTARTSYS;
 	gspca_dev->usb_err = 0;
-- 
1.7.10

Re: [RFCv1 PATCH 0/7] gspca: allow use of control framework and other fixes

[Hans de Goede]

Hi,

On 04/28/2012 05:09 PM, Hans Verkuil wrote:
> Hi all,
>
> Here is a patch series that makes it possible to use the control framework
> in gspca. The gspca core changes are very minor but as a bonus give you
> priority support as well.
>
> The hard work is in updating the subdrivers. I've done two, and I intend
> to do the stv06xx driver as well, but that's the last of my gspca webcams
> that I can test. Looking through the subdrivers I think that 50-70% are in
> the category 'easy to convert', the others will take a bit more time
> (autogain/gain type of constructs are always more complex than just a simple
> brightness control).
>
> After applying this patch series the two converted drivers pass the
> v4l2-compliance test as it stands today.

I haven't looked at any details yet, but from the description I love the changes :)

I was actually planning on doing something very similar myself soon-ish, so you've
saved me a bunch of work :)

I'll review this and add these to my tree. Jean-Francois, is it ok for these changes
to go upstream through my tree? The reason I'm asking is that I plan to convert
more subdrivers to the control framework for 3.5 and its easiest to have this all
in one tree then.

If you've remarks to the core changes I will make sure these get addressed in my
tree of course.

Regards,

Hans (the other Hans :)

Re: [RFCv1 PATCH 0/7] gspca: allow use of control framework and other fixes

[Jean-Francois Moine]

On Mon, 30 Apr 2012 13:13:59 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> I'll review this and add these to my tree. Jean-Francois, is it ok for these changes
> to go upstream through my tree? The reason I'm asking is that I plan to convert
> more subdrivers to the control framework for 3.5 and its easiest to have this all
> in one tree then.

Hi Hans,

As you know, I don't like them, but no matter, I am a bit tired
about the webcams (4 years now) and I'd be glad to change for new
applications (wayland, haiku, ARM?). Is anybody interested in
maintaining the gspca stuff?

Regards.

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

Re: [RFCv1 PATCH 0/7] gspca: allow use of control framework and other fixes

[Hans de Goede]

Hi,

On 05/01/2012 12:28 PM, Jean-Francois Moine wrote:
> On Mon, 30 Apr 2012 13:13:59 +0200
> Hans de Goede<hdegoede@redhat.com>  wrote:
>
>> I'll review this and add these to my tree. Jean-Francois, is it ok for these changes
>> to go upstream through my tree? The reason I'm asking is that I plan to convert
>> more subdrivers to the control framework for 3.5 and its easiest to have this all
>> in one tree then.
>
> Hi Hans,
>
> As you know, I don't like them, but no matter, I am a bit tired
> about the webcams (4 years now) and I'd be glad to change for new
> applications (wayland, haiku, ARM?).

I will be sad to see you stop maintaining gspca, but I can understand that
after 4 years you want a change. Thanks for the great job you've
done on gspca! So is there nothing we can do to change your mind?

 > Is anybody interested in maintaining the gspca stuff?

I can take over gspca maintenance if you want me to.

So if you're really going to stop maintaining gspca, may I suggest that
you start playing with arm. The trimslice, which I've just bought myself :)

I should get mine on Monday. It is a really nice machine for development
(enough cpu power and ram to compile on the machine which is a lot
easier then cross compilation). You can find the trimslice here:
http://trimslice.com/web/

I'm going to work on getting Fedora to support the trimslice as good
as possible. One interesting area where your hardware talents may
be very useful is getting FOSS support for the NVidia Tegra 2 video
core on the trimslice. There are already some people working on
an opensource (kms) driver for it but I'm sure they could use some
help.

Regards,

Hans

Re: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.

[Hans de Goede]

Hi,

I'm slowly working my way though this series today (both review, as well
as some tweaks and testing).

More comments inline...

On 04/28/2012 05:09 PM, Hans Verkuil wrote:
> From: Hans Verkuil<hans.verkuil@cisco.com>
>
> Make the necessary changes to allow subdrivers to use the control framework.
> This does not add control event support, that needs more work.
>
> Signed-off-by: Hans Verkuil<hans.verkuil@cisco.com>
> ---
>   drivers/media/video/gspca/gspca.c |   13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/video/gspca/gspca.c b/drivers/media/video/gspca/gspca.c
> index ca5a2b1..56dff10 100644
> --- a/drivers/media/video/gspca/gspca.c
> +++ b/drivers/media/video/gspca/gspca.c
> @@ -38,6 +38,7 @@
>   #include<linux/uaccess.h>
>   #include<linux/ktime.h>
>   #include<media/v4l2-ioctl.h>
> +#include<media/v4l2-ctrls.h>
>
>   #include "gspca.h"
>
> @@ -1006,6 +1007,8 @@ static void gspca_set_default_mode(struct gspca_dev *gspca_dev)
>
>   	/* set the current control values to their default values
>   	 * which may have changed in sd_init() */
> +	/* does nothing if ctrl_handler == NULL */
> +	v4l2_ctrl_handler_setup(gspca_dev->vdev.ctrl_handler);
>   	ctrl = gspca_dev->cam.ctrls;
>   	if (ctrl != NULL) {
>   		for (i = 0;
> @@ -1323,6 +1326,7 @@ static void gspca_release(struct video_device *vfd)
>   	PDEBUG(D_PROBE, "%s released",
>   		video_device_node_name(&gspca_dev->vdev));
>
> +	v4l2_ctrl_handler_free(gspca_dev->vdev.ctrl_handler);
>   	kfree(gspca_dev->usb_buf);
>   	kfree(gspca_dev);
>   }
> @@ -2347,6 +2351,10 @@ int gspca_dev_probe2(struct usb_interface *intf,
>   	gspca_dev->sd_desc = sd_desc;
>   	gspca_dev->nbufread = 2;
>   	gspca_dev->empty_packet = -1;	/* don't check the empty packets */
> +	gspca_dev->vdev = gspca_template;
> +	gspca_dev->vdev.parent =&intf->dev;
> +	gspca_dev->module = module;
> +	gspca_dev->present = 1;
>
>   	/* configure the subdriver and initialize the USB device */
>   	ret = sd_desc->config(gspca_dev, id);

You also need to move the initialization of the mutexes here, as the
v4l2_ctrl_handler_setup will call s_ctrl on all the controls, and s_ctrl
should take the usb_lock (see my review of the next patch in this series),
I'll make this change myself and merge it into your patch.

> @@ -2368,10 +2376,6 @@ int gspca_dev_probe2(struct usb_interface *intf,
>   	init_waitqueue_head(&gspca_dev->wq);
>
>   	/* init video stuff */
> -	memcpy(&gspca_dev->vdev,&gspca_template, sizeof gspca_template);
> -	gspca_dev->vdev.parent =&intf->dev;
> -	gspca_dev->module = module;
> -	gspca_dev->present = 1;
>   	ret = video_register_device(&gspca_dev->vdev,
>   				  VFL_TYPE_GRABBER,
>   				  -1);
> @@ -2391,6 +2395,7 @@ out:
>   	if (gspca_dev->input_dev)
>   		input_unregister_device(gspca_dev->input_dev);
>   #endif
> +	v4l2_ctrl_handler_free(gspca_dev->vdev.ctrl_handler);
>   	kfree(gspca_dev->usb_buf);
>   	kfree(gspca_dev);
>   	return ret;

Otherwise looks good, I've added it to my local tree (with the
described change), and will include it in my next pullreq.

Regards,

Hans

Re: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.

[Hans Verkuil]

On Sat May 5 2012 09:43:01 Hans de Goede wrote:
> Hi,
> 
> I'm slowly working my way though this series today (both review, as well
> as some tweaks and testing).

Thanks for that!

One note: I initialized the controls in sd_init. That's wrong, it should be
sd_config. sd_init is also called on resume, so that would initialize the
controls twice.

I'm working on this as well today, together with finishing the stv06xx and
mars conversion.

I will also do some suspend/resume testing today to check whether the controls
are put back correctly after a resume.

Regards,

	Hans

> 
> More comments inline...
> 
> On 04/28/2012 05:09 PM, Hans Verkuil wrote:
> > From: Hans Verkuil<hans.verkuil@cisco.com>
> >
> > Make the necessary changes to allow subdrivers to use the control framework.
> > This does not add control event support, that needs more work.
> >
> > Signed-off-by: Hans Verkuil<hans.verkuil@cisco.com>
> > ---
> >   drivers/media/video/gspca/gspca.c |   13 +++++++++----
> >   1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/video/gspca/gspca.c b/drivers/media/video/gspca/gspca.c
> > index ca5a2b1..56dff10 100644
> > --- a/drivers/media/video/gspca/gspca.c
> > +++ b/drivers/media/video/gspca/gspca.c
> > @@ -38,6 +38,7 @@
> >   #include<linux/uaccess.h>
> >   #include<linux/ktime.h>
> >   #include<media/v4l2-ioctl.h>
> > +#include<media/v4l2-ctrls.h>
> >
> >   #include "gspca.h"
> >
> > @@ -1006,6 +1007,8 @@ static void gspca_set_default_mode(struct gspca_dev *gspca_dev)
> >
> >   	/* set the current control values to their default values
> >   	 * which may have changed in sd_init() */
> > +	/* does nothing if ctrl_handler == NULL */
> > +	v4l2_ctrl_handler_setup(gspca_dev->vdev.ctrl_handler);
> >   	ctrl = gspca_dev->cam.ctrls;
> >   	if (ctrl != NULL) {
> >   		for (i = 0;
> > @@ -1323,6 +1326,7 @@ static void gspca_release(struct video_device *vfd)
> >   	PDEBUG(D_PROBE, "%s released",
> >   		video_device_node_name(&gspca_dev->vdev));
> >
> > +	v4l2_ctrl_handler_free(gspca_dev->vdev.ctrl_handler);
> >   	kfree(gspca_dev->usb_buf);
> >   	kfree(gspca_dev);
> >   }
> > @@ -2347,6 +2351,10 @@ int gspca_dev_probe2(struct usb_interface *intf,
> >   	gspca_dev->sd_desc = sd_desc;
> >   	gspca_dev->nbufread = 2;
> >   	gspca_dev->empty_packet = -1;	/* don't check the empty packets */
> > +	gspca_dev->vdev = gspca_template;
> > +	gspca_dev->vdev.parent =&intf->dev;
> > +	gspca_dev->module = module;
> > +	gspca_dev->present = 1;
> >
> >   	/* configure the subdriver and initialize the USB device */
> >   	ret = sd_desc->config(gspca_dev, id);
> 
> You also need to move the initialization of the mutexes here, as the
> v4l2_ctrl_handler_setup will call s_ctrl on all the controls, and s_ctrl
> should take the usb_lock (see my review of the next patch in this series),
> I'll make this change myself and merge it into your patch.
> 
> > @@ -2368,10 +2376,6 @@ int gspca_dev_probe2(struct usb_interface *intf,
> >   	init_waitqueue_head(&gspca_dev->wq);
> >
> >   	/* init video stuff */
> > -	memcpy(&gspca_dev->vdev,&gspca_template, sizeof gspca_template);
> > -	gspca_dev->vdev.parent =&intf->dev;
> > -	gspca_dev->module = module;
> > -	gspca_dev->present = 1;
> >   	ret = video_register_device(&gspca_dev->vdev,
> >   				  VFL_TYPE_GRABBER,
> >   				  -1);
> > @@ -2391,6 +2395,7 @@ out:
> >   	if (gspca_dev->input_dev)
> >   		input_unregister_device(gspca_dev->input_dev);
> >   #endif
> > +	v4l2_ctrl_handler_free(gspca_dev->vdev.ctrl_handler);
> >   	kfree(gspca_dev->usb_buf);
> >   	kfree(gspca_dev);
> >   	return ret;
> 
> Otherwise looks good, I've added it to my local tree (with the
> described change), and will include it in my next pullreq.
> 
> Regards,
> 
> Hans
> 

Re: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.

[Hans Verkuil]

On Sat May 5 2012 09:43:01 Hans de Goede wrote:
> Hi,
> 
> I'm slowly working my way though this series today (both review, as well
> as some tweaks and testing).
> 
> More comments inline...
> 
> On 04/28/2012 05:09 PM, Hans Verkuil wrote:
> > From: Hans Verkuil<hans.verkuil@cisco.com>
> >
> > Make the necessary changes to allow subdrivers to use the control framework.
> > This does not add control event support, that needs more work.
> >
> > Signed-off-by: Hans Verkuil<hans.verkuil@cisco.com>
> > ---
> >   drivers/media/video/gspca/gspca.c |   13 +++++++++----
> >   1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/video/gspca/gspca.c b/drivers/media/video/gspca/gspca.c
> > index ca5a2b1..56dff10 100644
> > --- a/drivers/media/video/gspca/gspca.c
> > +++ b/drivers/media/video/gspca/gspca.c
> > @@ -38,6 +38,7 @@
> >   #include<linux/uaccess.h>
> >   #include<linux/ktime.h>
> >   #include<media/v4l2-ioctl.h>
> > +#include<media/v4l2-ctrls.h>
> >
> >   #include "gspca.h"
> >
> > @@ -1006,6 +1007,8 @@ static void gspca_set_default_mode(struct gspca_dev *gspca_dev)
> >
> >   	/* set the current control values to their default values
> >   	 * which may have changed in sd_init() */
> > +	/* does nothing if ctrl_handler == NULL */
> > +	v4l2_ctrl_handler_setup(gspca_dev->vdev.ctrl_handler);
> >   	ctrl = gspca_dev->cam.ctrls;
> >   	if (ctrl != NULL) {
> >   		for (i = 0;
> > @@ -1323,6 +1326,7 @@ static void gspca_release(struct video_device *vfd)
> >   	PDEBUG(D_PROBE, "%s released",
> >   		video_device_node_name(&gspca_dev->vdev));
> >
> > +	v4l2_ctrl_handler_free(gspca_dev->vdev.ctrl_handler);
> >   	kfree(gspca_dev->usb_buf);
> >   	kfree(gspca_dev);
> >   }
> > @@ -2347,6 +2351,10 @@ int gspca_dev_probe2(struct usb_interface *intf,
> >   	gspca_dev->sd_desc = sd_desc;
> >   	gspca_dev->nbufread = 2;
> >   	gspca_dev->empty_packet = -1;	/* don't check the empty packets */
> > +	gspca_dev->vdev = gspca_template;
> > +	gspca_dev->vdev.parent =&intf->dev;
> > +	gspca_dev->module = module;
> > +	gspca_dev->present = 1;
> >
> >   	/* configure the subdriver and initialize the USB device */
> >   	ret = sd_desc->config(gspca_dev, id);
> 
> You also need to move the initialization of the mutexes here, as the
> v4l2_ctrl_handler_setup will call s_ctrl on all the controls, and s_ctrl
> should take the usb_lock (see my review of the next patch in this series),
> I'll make this change myself and merge it into your patch.

Looking at how usb_lock is used I am inclined to just set video_device->lock
to it and let the v4l2 core do all the locking for me, which will automatically
fix the missing s_ctrl lock too.

I've realized that there is a problem if you do your own locking *and* use the
control framework: if you need to set a control from within the driver, then
you do that using v4l2_ctrl_s_ctrl. But if s_ctrl has to take the driver's lock,
then you can't call v4l2_ctrl_s_ctrl with that lock already taken!

So you get:

vidioc_foo()
	lock(mylock)
	v4l2_ctrl_s_ctrl(ctrl, val)
		s_ctrl(ctrl, val)
			lock(mylock)

If the core takes care of locking then everything is fine.

All the current drivers that use v4l2_ctrl_g/s_ctrl use core locking. But this
can be a problem in the future. The only way to resolve this is to tell v4l2-ioctl.c
about your own lock so it can take it for you when calling into the control framework.

Regards,

	Hans

Re: [RFCv1 PATCH 2/7] zc3xx: convert to the control framework.

[Hans de Goede]

[-- Attachment #1: Type: text/plain, Size: 10622 bytes --]

Hi,

Reviewing and testing this one was, erm, interesting. See comments
inline.

On 04/28/2012 05:09 PM, Hans Verkuil wrote:
> From: Hans Verkuil<hans.verkuil@cisco.com>
>
> Signed-off-by: Hans Verkuil<hans.verkuil@cisco.com>
> ---
>   drivers/media/video/gspca/zc3xx.c |  451 +++++++++++++++----------------------
>   1 file changed, 182 insertions(+), 269 deletions(-)
>
> diff --git a/drivers/media/video/gspca/zc3xx.c b/drivers/media/video/gspca/zc3xx.c
> index 7d9a4f1..e7b7599 100644
> --- a/drivers/media/video/gspca/zc3xx.c
> +++ b/drivers/media/video/gspca/zc3xx.c

<snip various chunks, no comments on these>

> +static int sd_setautogain(struct gspca_dev *gspca_dev, s32 val)
> +{
> +	if (!gspca_dev->streaming)
> +		return 0;
> +	setautogain(gspca_dev, val);
> +
> +	return gspca_dev->usb_err;
> +}
> +

zcxx_s_ctrl can just as well call setautogain directly as it
does for the others. It should gspca_dev->streaming for all
controls anyways (more on that below).

> +static int sd_setquality(struct gspca_dev *gspca_dev, s32 val)
> +{
> +	struct sd *sd = (struct sd *) gspca_dev;
> +	int i;
> +
> +	for (i = 0; i<  ARRAY_SIZE(jpeg_qual) - 1; i++) {
> +		if (val<= jpeg_qual[i])
> +			break;
> +	}
> +	sd->reg08 = i;
> +	if (!gspca_dev->streaming)
> +		return 0;
> +	jpeg_set_qual(sd->jpeg_hdr, val);
> +	return gspca_dev->usb_err;
> +}
> +

This for 99% duplicates the special handling you already
have for this in zcxx_s_ctrl, which is also the only caller.

More over this gets its wrong as under certain conditions
it will end up with a different value of i then the code in
zcxx_s_ctrl, and zcxx_s_ctrl bases the value it returns
to the caller as the "achieved" value on its own calculation
of i.

So I thought lets refactor this a bit to get rid of this
duplication and that turned out to be a long journey rather
then a quick patch :) While testing my changes I found that
there are various (pre-existing) problems with how zc3xx
handles JPEG quality so I've fixed those first.

The result is a 6 patch patchset (attached), which besides
many fixes to the JPEG quality handling, also includes
a new version of this patch removing the duplication.

> +static int sd_set_jcomp(struct gspca_dev *gspca_dev,
> +			struct v4l2_jpegcompression *jcomp)
> +{
> +	struct sd *sd = (struct sd *) gspca_dev;
> +
> +	if (sd->jpegqual) {
> +		v4l2_ctrl_s_ctrl(sd->jpegqual, jcomp->quality);
> +		jcomp->quality = v4l2_ctrl_g_ctrl(sd->jpegqual);
> +		return gspca_dev->usb_err;
> +	}
> +	jcomp->quality = jpeg_qual[sd->reg08];
> +	return 0;
> +}
> +
> +static int sd_get_jcomp(struct gspca_dev *gspca_dev,
> +			struct v4l2_jpegcompression *jcomp)
> +{
> +	struct sd *sd = (struct sd *) gspca_dev;
> +
> +	memset(jcomp, 0, sizeof *jcomp);
> +	jcomp->quality = jpeg_qual[sd->reg08];
> +	jcomp->jpeg_markers = V4L2_JPEG_MARKER_DHT
> +			| V4L2_JPEG_MARKER_DQT;
> +	return 0;
> +}
> +

No need to move these upwards in the source file, leaving them
where they are makes it easier to see what is actually changed.

> +static int zcxx_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct sd *sd = container_of(ctrl->handler, struct sd, ctrl_handler);
> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_AUTOGAIN:
> +		if (ctrl->val&&  sd->exposure&&  sd->gspca_dev.streaming)
> +			sd->exposure->val = getexposure(&sd->gspca_dev);
> +		break;
> +	}
> +	return 0;
> +}

The call to getexposure needs to be surrounded by locking / unlocking
usb_lock, and gspca_de->usb_err should be checked after the call.

In general all calls to the actual hardware need to be made with
the usb_lock hold, so that they cannot race with for example
hardware accesses done from sd_start. This is important since
although the usb-core will ensure that we never can do more then
one control request at a time sometimes several control requests
need to be done in order without interference (for example for
accessing the i2c bus between the bridge and the sensor).

> +
> +static int zcxx_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct sd *sd = container_of(ctrl->handler, struct sd, ctrl_handler);
> +	int ret;
> +	int i;
> +
> +	switch (ctrl->id) {
> +	/* gamma/brightness/contrast cluster */
> +	case V4L2_CID_GAMMA:
> +		setcontrast(&sd->gspca_dev, sd->gamma->val,
> +				sd->brightness->val, sd->contrast->val);
> +		return 0;
> +	/* autogain/exposure cluster */
> +	case V4L2_CID_AUTOGAIN:
> +		ret = sd_setautogain(&sd->gspca_dev, ctrl->val);
> +		if (!ret&&  !ctrl->val&&  sd->exposure)
> +			setexposure(&sd->gspca_dev, sd->exposure->val);
> +		return ret;
> +	case V4L2_CID_POWER_LINE_FREQUENCY:
> +		setlightfreq(&sd->gspca_dev, ctrl->val);
> +		return 0;
> +	case V4L2_CID_SHARPNESS:
> +		setsharpness(&sd->gspca_dev, ctrl->val);
> +		return 0;
> +	case V4L2_CID_JPEG_COMPRESSION_QUALITY:
> +		for (i = 0; i<  ARRAY_SIZE(jpeg_qual) - 1; i++) {
> +			if (ctrl->val<= jpeg_qual[i])
> +				break;
> +		}
> +		if (i>  0&&  i == sd->reg08&&  ctrl->val<  jpeg_qual[sd->reg08])
> +			i--;
> +		ctrl->val = jpeg_qual[i];
> +		return sd_setquality(&sd->gspca_dev, ctrl->val);
> +	}
> +	return -EINVAL;
> +}
> +

Like zcxx_g_volatile_ctrl this function to should take the usb_lock
before calling setfoo. Also after acquiring the lock it should check
if the device is streaming and if it is not streaming it should not
call any setfoo functions, as those will be done on sd_start then.

Note that setfoo may work when calling them on this bridge while not
streaming, but there are no guarantees the same holds for other
bridges.

In general the control paradigm in gspca is to not send any control
values to the camera until streaming starts.

Last this function to should check gspca_dev->usb_err (which works
a bit like hdl->error accumulating io errors made by setfoo calls).

So the order for any s_ctrl in gspca should normally be:

1) take usb_lock
2) clear gspca_dev->usb_err
3) check gspca_dev->streaming if not streaming
    normally goto unlock and return success immediately
    But there may be special cases where the passed in
    value should first be rounded to the nearest supported
    value (ie the jpeg quality in zc3xx.c)
4) call setfoo functions
5) store gspca_dev->usb_err in a local "ret" variable
6) unlock
7) return ret


> +static const struct v4l2_ctrl_ops zcxx_ctrl_ops = {
> +	.g_volatile_ctrl = zcxx_g_volatile_ctrl,
> +	.s_ctrl = zcxx_s_ctrl,
> +};
> +
>   /* this function is called at probe and resume time */
>   static int sd_init(struct gspca_dev *gspca_dev)
>   {
>   	struct sd *sd = (struct sd *) gspca_dev;
> +	struct v4l2_ctrl_handler *hdl =&sd->ctrl_handler;
>   	struct cam *cam;
>   	int sensor;
>   	static const u8 gamma[SENSOR_MAX] = {
> @@ -6688,7 +6673,6 @@ static int sd_init(struct gspca_dev *gspca_dev)
>   		case 0x2030:
>   			PDEBUG(D_PROBE, "Find Sensor PO2030");
>   			sd->sensor = SENSOR_PO2030;
> -			sd->ctrls[SHARPNESS].def = 0;	/* from win traces */
>   			break;
>   		case 0x7620:
>   			PDEBUG(D_PROBE, "Find Sensor OV7620");
> @@ -6730,30 +6714,40 @@ static int sd_init(struct gspca_dev *gspca_dev)
>   		break;
>   	}
>
> -	sd->ctrls[GAMMA].def = gamma[sd->sensor];
> -	sd->reg08 = reg08_tb[sd->sensor];
> -	sd->ctrls[QUALITY].def = jpeg_qual[sd->reg08];
> -	sd->ctrls[QUALITY].min = jpeg_qual[0];
> -	sd->ctrls[QUALITY].max = jpeg_qual[ARRAY_SIZE(jpeg_qual) - 1];
> -
> -	switch (sd->sensor) {
> -	case SENSOR_HV7131R:
> -		gspca_dev->ctrl_dis = (1<<  QUALITY);
> -		break;
> -	case SENSOR_OV7630C:
> -		gspca_dev->ctrl_dis = (1<<  LIGHTFREQ) | (1<<  EXPOSURE);
> -		break;
> -	case SENSOR_PAS202B:
> -		gspca_dev->ctrl_dis = (1<<  QUALITY) | (1<<  EXPOSURE);
> -		break;
> -	default:
> -		gspca_dev->ctrl_dis = (1<<  EXPOSURE);
> -		break;
> +	gspca_dev->vdev.ctrl_handler = hdl;
> +	v4l2_ctrl_handler_init(hdl, 8);
> +	sd->brightness = v4l2_ctrl_new_std(hdl,&zcxx_ctrl_ops,
> +			V4L2_CID_BRIGHTNESS, 0, 255, 1, 128);
> +	sd->contrast = v4l2_ctrl_new_std(hdl,&zcxx_ctrl_ops,
> +			V4L2_CID_CONTRAST, 0, 255, 1, 128);
> +	sd->gamma = v4l2_ctrl_new_std(hdl,&zcxx_ctrl_ops,
> +			V4L2_CID_GAMMA, 1, 6, 1, gamma[sd->sensor]);
> +	if (sd->sensor == SENSOR_HV7131R)
> +		sd->exposure = v4l2_ctrl_new_std(hdl,&zcxx_ctrl_ops,
> +			V4L2_CID_EXPOSURE, 0x30d, 0x493e, 1, 0x927);
> +	sd->autogain = v4l2_ctrl_new_std(hdl,&zcxx_ctrl_ops,
> +			V4L2_CID_AUTOGAIN, 0, 1, 1, 1);
> +	if (sd->sensor != SENSOR_OV7630C&&  sd->sensor != SENSOR_PAS202B)
> +		sd->plfreq = v4l2_ctrl_new_std_menu(hdl,&zcxx_ctrl_ops,
> +			V4L2_CID_POWER_LINE_FREQUENCY,
> +			V4L2_CID_POWER_LINE_FREQUENCY_60HZ, 0,
> +			V4L2_CID_POWER_LINE_FREQUENCY_DISABLED);

This is wrong the PAS202B should have a powerlinefreq control.

> +	sd->sharpness = v4l2_ctrl_new_std(hdl,&zcxx_ctrl_ops,
> +			V4L2_CID_SHARPNESS, 0, 3, 1,
> +			sd->sensor == SENSOR_PO2030 ? 0 : 2);
> +	if (sd->sensor != SENSOR_HV7131R&&  sd->sensor != SENSOR_PAS202B)
> +		sd->jpegqual = v4l2_ctrl_new_std(hdl,&zcxx_ctrl_ops,
> +			V4L2_CID_JPEG_COMPRESSION_QUALITY,
> +			jpeg_qual[0], jpeg_qual[ARRAY_SIZE(jpeg_qual) - 1], 1,
> +			jpeg_qual[sd->reg08]);
> +	if (hdl->error) {
> +		pr_err("Could not initialize controls\n");
> +		return hdl->error;
>   	}
> -#if AUTOGAIN_DEF
> -	if (sd->ctrls[AUTOGAIN].val)
> -		gspca_dev->ctrl_inac = (1<<  EXPOSURE);
> -#endif
> +	v4l2_ctrl_cluster(3,&sd->gamma);
> +	if (sd->sensor == SENSOR_HV7131R)
> +		v4l2_ctrl_auto_cluster(2,&sd->autogain, 0, true);
> +	sd->reg08 = reg08_tb[sd->sensor];
>
>   	/* switch off the led */
>   	reg_w(gspca_dev, 0x01, 0x0000);
> @@ -6864,7 +6858,7 @@ static int sd_start(struct gspca_dev *gspca_dev)
>   		reg_w(gspca_dev, 0x03, 0x0008);
>   		break;
>   	}
> -	setsharpness(gspca_dev);
> +	setsharpness(gspca_dev, v4l2_ctrl_g_ctrl(sd->sharpness));
>
>   	/* set the gamma tables when not set */
>   	switch (sd->sensor) {
> @@ -6873,7 +6867,9 @@ static int sd_start(struct gspca_dev *gspca_dev)
>   	case SENSOR_OV7630C:
>   		break;
>   	default:
> -		setcontrast(gspca_dev);
> +		setcontrast(&sd->gspca_dev, v4l2_ctrl_g_ctrl(sd->gamma),
> +				v4l2_ctrl_g_ctrl(sd->brightness),
> +				v4l2_ctrl_g_ctrl(sd->contrast));
>   		break;
>   	}
>   	setmatrix(gspca_dev);			/* one more time? */
> @@ -6886,7 +6882,7 @@ static int sd_start(struct gspca_dev *gspca_dev)
>   	}
>   	setquality(gspca_dev);
>   	jpeg_set_qual(sd->jpeg_hdr, jpeg_qual[sd->reg08]);
> -	setlightfreq(gspca_dev);
> +	setlightfreq(gspca_dev, v4l2_ctrl_g_ctrl(sd->plfreq));

sd->plfreq can be null, so this needs an if (sd->plfreq)

>
>   	switch (sd->sensor) {
>   	case SENSOR_ADCM2700:

<snip>

Moving on to the next patch in the series now, hopefully that one will
go quicker :)

Regards,

Hans


[-- Attachment #2: 0001-gspca_zc3xx-Fix-setting-of-jpeg-quality-while-stream.patch --]
[-- Type: text/x-patch, Size: 1552 bytes --]

>From 78fd672b2b87177ceff710ca26d9ae15420e66e7 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Sat, 5 May 2012 12:31:48 +0200
Subject: [PATCH 1/6] gspca_zc3xx: Fix setting of jpeg quality while streaming

When the user changes the JPEG quality while the camera is streaming, the
driver should not only change the JPEG headers send to userspace, but also
actually tell the camera to use a different quantization table.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/video/gspca/zc3xx.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/video/gspca/zc3xx.c b/drivers/media/video/gspca/zc3xx.c
index 7d9a4f1..c7c9d11 100644
--- a/drivers/media/video/gspca/zc3xx.c
+++ b/drivers/media/video/gspca/zc3xx.c
@@ -5923,6 +5923,8 @@ static void setquality(struct gspca_dev *gspca_dev)
 	struct sd *sd = (struct sd *) gspca_dev;
 	s8 reg07;
 
+	jpeg_set_qual(sd->jpeg_hdr, jpeg_qual[sd->reg08]);
+
 	reg07 = 0;
 	switch (sd->sensor) {
 	case SENSOR_OV7620:
@@ -6885,7 +6887,6 @@ static int sd_start(struct gspca_dev *gspca_dev)
 		break;
 	}
 	setquality(gspca_dev);
-	jpeg_set_qual(sd->jpeg_hdr, jpeg_qual[sd->reg08]);
 	setlightfreq(gspca_dev);
 
 	switch (sd->sensor) {
@@ -7041,7 +7042,7 @@ static int sd_setquality(struct gspca_dev *gspca_dev, __s32 val)
 	sd->reg08 = i;
 	sd->ctrls[QUALITY].val = jpeg_qual[i];
 	if (gspca_dev->streaming)
-		jpeg_set_qual(sd->jpeg_hdr, sd->ctrls[QUALITY].val);
+		setquality(gspca_dev);
 	return gspca_dev->usb_err;
 }
 
-- 
1.7.10


[-- Attachment #3: 0002-gspca_zc3xx-Fix-JPEG-quality-setting-code.patch --]
[-- Type: text/x-patch, Size: 5951 bytes --]

>From 38f3eb79db0350a05ac29473f0ce4db6efaa8960 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Sat, 5 May 2012 11:53:36 +0200
Subject: [PATCH 2/6] gspca_zc3xx: Fix JPEG quality setting code

The current code is using bits 0-1 of register 8 of the zc3xx controller
to set the JPEG quality, but the correct bits are bits 1-2. Bit 0 selects
between truncation or rounding in the quantization phase of the compression,
since rounding generally gives better results it should thus always be 1.

This patch also corrects the quality percentages which belong to the 4
different settings.

Lst this patch removes the different reg 8 defaults depending on the sensor
type. Some of them where going for a default quality setting of 50%, which
generally is not necessary in any way and results in poor image quality.
75% is a good default to use for all scenarios.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/video/gspca/zc3xx.c |   64 +++++++++++++------------------------
 1 file changed, 22 insertions(+), 42 deletions(-)

diff --git a/drivers/media/video/gspca/zc3xx.c b/drivers/media/video/gspca/zc3xx.c
index c7c9d11..f770676 100644
--- a/drivers/media/video/gspca/zc3xx.c
+++ b/drivers/media/video/gspca/zc3xx.c
@@ -32,7 +32,7 @@ MODULE_LICENSE("GPL");
 
 static int force_sensor = -1;
 
-#define REG08_DEF 3		/* default JPEG compression (70%) */
+#define REG08_DEF 3		/* default JPEG compression (75%) */
 #include "zc3xx-reg.h"
 
 /* controls */
@@ -193,10 +193,10 @@ static const struct ctrl sd_ctrls[NCTRLS] = {
 		.id	 = V4L2_CID_JPEG_COMPRESSION_QUALITY,
 		.type    = V4L2_CTRL_TYPE_INTEGER,
 		.name    = "Compression Quality",
-		.minimum = 40,
-		.maximum = 70,
+		.minimum = 50,
+		.maximum = 94,
 		.step    = 1,
-		.default_value = 70	/* updated in sd_init() */
+		.default_value = 75,
 	    },
 	    .set = sd_setquality
 	},
@@ -241,8 +241,8 @@ static const struct v4l2_pix_format sif_mode[] = {
 		.priv = 0},
 };
 
-/* bridge reg08 -> JPEG quality conversion table */
-static u8 jpeg_qual[] = {40, 50, 60, 70, /*80*/};
+/* bridge reg08 bits 1-2 -> JPEG quality conversion table */
+static u8 jpeg_qual[] = {50, 75, 87, 94};
 
 /* usb exchanges */
 struct usb_action {
@@ -5923,7 +5923,7 @@ static void setquality(struct gspca_dev *gspca_dev)
 	struct sd *sd = (struct sd *) gspca_dev;
 	s8 reg07;
 
-	jpeg_set_qual(sd->jpeg_hdr, jpeg_qual[sd->reg08]);
+	jpeg_set_qual(sd->jpeg_hdr, jpeg_qual[sd->reg08 >> 1]);
 
 	reg07 = 0;
 	switch (sd->sensor) {
@@ -6079,11 +6079,12 @@ static void transfer_update(struct work_struct *work)
 	struct sd *sd = container_of(work, struct sd, work);
 	struct gspca_dev *gspca_dev = &sd->gspca_dev;
 	int change, good;
-	u8 reg07, reg11;
+	u8 reg07, qual, reg11;
 
 	/* synchronize with the main driver and initialize the registers */
 	mutex_lock(&gspca_dev->usb_lock);
 	reg07 = 0;					/* max */
+	qual = sd->reg08 >> 1;
 	reg_w(gspca_dev, reg07, 0x0007);
 	reg_w(gspca_dev, sd->reg08, ZC3XX_R008_CLOCKSETTING);
 	mutex_unlock(&gspca_dev->usb_lock);
@@ -6108,9 +6109,9 @@ static void transfer_update(struct work_struct *work)
 			case 0:				/* max */
 				reg07 = sd->sensor == SENSOR_HV7131R
 						? 0x30 : 0x32;
-				if (sd->reg08 != 0) {
+				if (qual != 0) {
 					change = 3;
-					sd->reg08--;
+					qual--;
 				}
 				break;
 			case 0x32:
@@ -6143,10 +6144,10 @@ static void transfer_update(struct work_struct *work)
 					}
 				}
 			} else {			/* reg07 max */
-				if (sd->reg08 < sizeof jpeg_qual - 1) {
+				if (qual < sizeof jpeg_qual - 1) {
 					good++;
 					if (good > 10) {
-						sd->reg08++;
+						qual++;
 						change = 2;
 					}
 				}
@@ -6161,15 +6162,16 @@ static void transfer_update(struct work_struct *work)
 					goto err;
 			}
 			if (change & 2) {
+				sd->reg08 = (qual << 1) | 1;
 				reg_w(gspca_dev, sd->reg08,
 						ZC3XX_R008_CLOCKSETTING);
 				if (gspca_dev->usb_err < 0
 				 || !gspca_dev->present
 				 || !gspca_dev->streaming)
 					goto err;
-				sd->ctrls[QUALITY].val = jpeg_qual[sd->reg08];
+				sd->ctrls[QUALITY].val = jpeg_qual[qual];
 				jpeg_set_qual(sd->jpeg_hdr,
-						jpeg_qual[sd->reg08]);
+						jpeg_qual[qual]);
 			}
 		}
 		mutex_unlock(&gspca_dev->usb_lock);
@@ -6561,27 +6563,6 @@ static int sd_init(struct gspca_dev *gspca_dev)
 		[SENSOR_PO2030] =	1,
 		[SENSOR_TAS5130C] =	1,
 	};
-	static const u8 reg08_tb[SENSOR_MAX] = {
-		[SENSOR_ADCM2700] =	1,
-		[SENSOR_CS2102] =	3,
-		[SENSOR_CS2102K] =	3,
-		[SENSOR_GC0303] =	2,
-		[SENSOR_GC0305] =	3,
-		[SENSOR_HDCS2020] =	1,
-		[SENSOR_HV7131B] =	3,
-		[SENSOR_HV7131R] =	3,
-		[SENSOR_ICM105A] =	3,
-		[SENSOR_MC501CB] =	3,
-		[SENSOR_MT9V111_1] =	3,
-		[SENSOR_MT9V111_3] =	3,
-		[SENSOR_OV7620] =	1,
-		[SENSOR_OV7630C] =	3,
-		[SENSOR_PAS106] =	3,
-		[SENSOR_PAS202B] =	3,
-		[SENSOR_PB0330] =	3,
-		[SENSOR_PO2030] =	2,
-		[SENSOR_TAS5130C] =	3,
-	};
 
 	sensor = zcxx_probeSensor(gspca_dev);
 	if (sensor >= 0)
@@ -6733,8 +6714,7 @@ static int sd_init(struct gspca_dev *gspca_dev)
 	}
 
 	sd->ctrls[GAMMA].def = gamma[sd->sensor];
-	sd->reg08 = reg08_tb[sd->sensor];
-	sd->ctrls[QUALITY].def = jpeg_qual[sd->reg08];
+	sd->ctrls[QUALITY].def = jpeg_qual[sd->reg08 >> 1];
 	sd->ctrls[QUALITY].min = jpeg_qual[0];
 	sd->ctrls[QUALITY].max = jpeg_qual[ARRAY_SIZE(jpeg_qual) - 1];
 
@@ -7029,17 +7009,17 @@ static int sd_querymenu(struct gspca_dev *gspca_dev,
 static int sd_setquality(struct gspca_dev *gspca_dev, __s32 val)
 {
 	struct sd *sd = (struct sd *) gspca_dev;
-	int i;
+	int i, qual = sd->reg08 >> 1;
 
-	for (i = 0; i < ARRAY_SIZE(jpeg_qual) - 1; i++) {
+	for (i = 0; i < ARRAY_SIZE(jpeg_qual); i++) {
 		if (val <= jpeg_qual[i])
 			break;
 	}
 	if (i > 0
-	 && i == sd->reg08
-	 && val < jpeg_qual[sd->reg08])
+	 && i == qual
+	 && val < jpeg_qual[i])
 		i--;
-	sd->reg08 = i;
+	sd->reg08 = (i << 1) | 1;
 	sd->ctrls[QUALITY].val = jpeg_qual[i];
 	if (gspca_dev->streaming)
 		setquality(gspca_dev);
-- 
1.7.10


[-- Attachment #4: 0003-gscpa_zc3xx-Always-automatically-adjust-BRC-as-neede.patch --]
[-- Type: text/x-patch, Size: 7720 bytes --]

>From 00aef65c5f72326442f4aaf0a8ad49e67e1dc10e Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Sat, 5 May 2012 14:26:33 +0200
Subject: [PATCH 3/6] gscpa_zc3xx: Always automatically adjust BRC as needed

Always automatically adjust the Bit Rate Control setting as needed, independent
of the sensor type. BRC is needed to not run out of bandwidth with higher
quality settings independent of the sensor.

Also only automatically adjust BRC, and don't adjust the JPEG quality control
automatically, as that is not needed and leads to ugly flashes when it is
changed. Note that before this patch-set the quality was never changed
either due to the bugs in the quality handling fixed in previous patches in
this set.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/video/gspca/zc3xx.c |  159 +++++++++++++------------------------
 1 file changed, 53 insertions(+), 106 deletions(-)

diff --git a/drivers/media/video/gspca/zc3xx.c b/drivers/media/video/gspca/zc3xx.c
index f770676..18ef68d 100644
--- a/drivers/media/video/gspca/zc3xx.c
+++ b/drivers/media/video/gspca/zc3xx.c
@@ -5921,22 +5921,8 @@ static void setexposure(struct gspca_dev *gspca_dev)
 static void setquality(struct gspca_dev *gspca_dev)
 {
 	struct sd *sd = (struct sd *) gspca_dev;
-	s8 reg07;
-
 	jpeg_set_qual(sd->jpeg_hdr, jpeg_qual[sd->reg08 >> 1]);
-
-	reg07 = 0;
-	switch (sd->sensor) {
-	case SENSOR_OV7620:
-		reg07 = 0x30;
-		break;
-	case SENSOR_HV7131R:
-	case SENSOR_PAS202B:
-		return;			/* done by work queue */
-	}
 	reg_w(gspca_dev, sd->reg08, ZC3XX_R008_CLOCKSETTING);
-	if (reg07 != 0)
-		reg_w(gspca_dev, reg07, 0x0007);
 }
 
 /* Matches the sensor's internal frame rate to the lighting frequency.
@@ -6070,109 +6056,62 @@ static void setautogain(struct gspca_dev *gspca_dev)
 	reg_w(gspca_dev, autoval, 0x0180);
 }
 
-/* update the transfer parameters */
-/* This function is executed from a work queue. */
-/* The exact use of the bridge registers 07 and 08 is not known.
- * The following algorithm has been adapted from ms-win traces */
+/*
+ * Update the transfer parameters.
+ * This function is executed from a work queue.
+ */
 static void transfer_update(struct work_struct *work)
 {
 	struct sd *sd = container_of(work, struct sd, work);
 	struct gspca_dev *gspca_dev = &sd->gspca_dev;
 	int change, good;
-	u8 reg07, qual, reg11;
+	u8 reg07, reg11;
 
-	/* synchronize with the main driver and initialize the registers */
-	mutex_lock(&gspca_dev->usb_lock);
-	reg07 = 0;					/* max */
-	qual = sd->reg08 >> 1;
-	reg_w(gspca_dev, reg07, 0x0007);
-	reg_w(gspca_dev, sd->reg08, ZC3XX_R008_CLOCKSETTING);
-	mutex_unlock(&gspca_dev->usb_lock);
+	/* reg07 gets set to 0 by sd_start before starting us */
+	reg07 = 0;
 
 	good = 0;
 	for (;;) {
 		msleep(100);
 
-		/* get the transfer status */
-		/* the bit 0 of the bridge register 11 indicates overflow */
 		mutex_lock(&gspca_dev->usb_lock);
 		if (!gspca_dev->present || !gspca_dev->streaming)
 			goto err;
+
+		/* Bit 0 of register 11 indicates FIFO overflow */
+		gspca_dev->usb_err = 0;
 		reg11 = reg_r(gspca_dev, 0x0011);
-		if (gspca_dev->usb_err < 0
-		 || !gspca_dev->present || !gspca_dev->streaming)
+		if (gspca_dev->usb_err)
 			goto err;
 
 		change = reg11 & 0x01;
 		if (change) {				/* overflow */
-			switch (reg07) {
-			case 0:				/* max */
-				reg07 = sd->sensor == SENSOR_HV7131R
-						? 0x30 : 0x32;
-				if (qual != 0) {
-					change = 3;
-					qual--;
-				}
-				break;
-			case 0x32:
-				reg07 -= 4;
-				break;
-			default:
-				reg07 -= 2;
-				break;
-			case 2:
-				change = 0;		/* already min */
-				break;
-			}
 			good = 0;
+
+			if (reg07 == 0) /* Bit Rate Control not enabled? */
+				reg07 = 0x32; /* Allow 98 bytes / unit */
+			else if (reg07 > 2)
+				reg07 -= 2; /* Decrease allowed bytes / unit */
+			else
+				change = 0;
 		} else {				/* no overflow */
-			if (reg07 != 0) {		/* if not max */
-				good++;
-				if (good >= 10) {
-					good = 0;
+			good++;
+			if (good >= 10) {
+				good = 0;
+				if (reg07) { /* BRC enabled? */
 					change = 1;
-					reg07 += 2;
-					switch (reg07) {
-					case 0x30:
-						if (sd->sensor == SENSOR_PAS202B)
-							reg07 += 2;
-						break;
-					case 0x32:
-					case 0x34:
+					if (reg07 < 0x32)
+						reg07 += 2;
+					else
 						reg07 = 0;
-						break;
-					}
-				}
-			} else {			/* reg07 max */
-				if (qual < sizeof jpeg_qual - 1) {
-					good++;
-					if (good > 10) {
-						qual++;
-						change = 2;
-					}
 				}
 			}
 		}
 		if (change) {
-			if (change & 1) {
-				reg_w(gspca_dev, reg07, 0x0007);
-				if (gspca_dev->usb_err < 0
-				 || !gspca_dev->present
-				 || !gspca_dev->streaming)
-					goto err;
-			}
-			if (change & 2) {
-				sd->reg08 = (qual << 1) | 1;
-				reg_w(gspca_dev, sd->reg08,
-						ZC3XX_R008_CLOCKSETTING);
-				if (gspca_dev->usb_err < 0
-				 || !gspca_dev->present
-				 || !gspca_dev->streaming)
-					goto err;
-				sd->ctrls[QUALITY].val = jpeg_qual[qual];
-				jpeg_set_qual(sd->jpeg_hdr,
-						jpeg_qual[qual]);
-			}
+			gspca_dev->usb_err = 0;
+			reg_w(gspca_dev, reg07, 0x0007);
+			if (gspca_dev->usb_err)
+				goto err;
 		}
 		mutex_unlock(&gspca_dev->usb_lock);
 	}
@@ -6720,14 +6659,10 @@ static int sd_init(struct gspca_dev *gspca_dev)
 
 	switch (sd->sensor) {
 	case SENSOR_HV7131R:
-		gspca_dev->ctrl_dis = (1 << QUALITY);
 		break;
 	case SENSOR_OV7630C:
 		gspca_dev->ctrl_dis = (1 << LIGHTFREQ) | (1 << EXPOSURE);
 		break;
-	case SENSOR_PAS202B:
-		gspca_dev->ctrl_dis = (1 << QUALITY) | (1 << EXPOSURE);
-		break;
 	default:
 		gspca_dev->ctrl_dis = (1 << EXPOSURE);
 		break;
@@ -6742,6 +6677,13 @@ static int sd_init(struct gspca_dev *gspca_dev)
 	return gspca_dev->usb_err;
 }
 
+static int sd_pre_start(struct gspca_dev *gspca_dev)
+{
+	struct sd *sd = (struct sd *) gspca_dev;
+	gspca_dev->cam.needs_full_bandwidth = (sd->reg08 >= 4) ? 1 : 0;
+	return 0;
+}
+
 static int sd_start(struct gspca_dev *gspca_dev)
 {
 	struct sd *sd = (struct sd *) gspca_dev;
@@ -6867,6 +6809,8 @@ static int sd_start(struct gspca_dev *gspca_dev)
 		break;
 	}
 	setquality(gspca_dev);
+	/* Start with BRC disabled, transfer_update will enable it if needed */
+	reg_w(gspca_dev, 0x00, 0x0007);
 	setlightfreq(gspca_dev);
 
 	switch (sd->sensor) {
@@ -6904,19 +6848,14 @@ static int sd_start(struct gspca_dev *gspca_dev)
 
 	setautogain(gspca_dev);
 
-	/* start the transfer update thread if needed */
-	if (gspca_dev->usb_err >= 0) {
-		switch (sd->sensor) {
-		case SENSOR_HV7131R:
-		case SENSOR_PAS202B:
-			sd->work_thread =
-				create_singlethread_workqueue(KBUILD_MODNAME);
-			queue_work(sd->work_thread, &sd->work);
-			break;
-		}
-	}
+	if (gspca_dev->usb_err < 0)
+		return gspca_dev->usb_err;
 
-	return gspca_dev->usb_err;
+	/* Start the transfer parameters update thread */
+	sd->work_thread = create_singlethread_workqueue(KBUILD_MODNAME);
+	queue_work(sd->work_thread, &sd->work);
+
+	return 0;
 }
 
 /* called on streamoff with alt 0 and on disconnect */
@@ -7019,8 +6958,15 @@ static int sd_setquality(struct gspca_dev *gspca_dev, __s32 val)
 	 && i == qual
 	 && val < jpeg_qual[i])
 		i--;
+
+	/* With high quality settings we need max bandwidth */
+	if (i >= 2 && gspca_dev->streaming &&
+	    !gspca_dev->cam.needs_full_bandwidth)
+		return -EBUSY;
+
 	sd->reg08 = (i << 1) | 1;
 	sd->ctrls[QUALITY].val = jpeg_qual[i];
+
 	if (gspca_dev->streaming)
 		setquality(gspca_dev);
 	return gspca_dev->usb_err;
@@ -7070,6 +7016,7 @@ static const struct sd_desc sd_desc = {
 	.nctrls = ARRAY_SIZE(sd_ctrls),
 	.config = sd_config,
 	.init = sd_init,
+	.isoc_init = sd_pre_start,
 	.start = sd_start,
 	.stop0 = sd_stop0,
 	.pkt_scan = sd_pkt_scan,
-- 
1.7.10


[-- Attachment #5: 0004-gscpa_zc3xx-Disable-the-highest-quality-setting-as-i.patch --]
[-- Type: text/x-patch, Size: 1479 bytes --]

>From 1c55e5072b567646b0e90773720ecb8030d90c99 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Sat, 5 May 2012 14:33:23 +0200
Subject: [PATCH 4/6] gscpa_zc3xx: Disable the highest quality setting as it
 is not usable

Even with BRC the highest quality setting is not usable, BRC strips so
much data from each MCU that the quality becomes worse then using a lower
quality setting to begin with.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/video/gspca/zc3xx.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/media/video/gspca/zc3xx.c b/drivers/media/video/gspca/zc3xx.c
index 18ef68d..a8282b8 100644
--- a/drivers/media/video/gspca/zc3xx.c
+++ b/drivers/media/video/gspca/zc3xx.c
@@ -194,7 +194,7 @@ static const struct ctrl sd_ctrls[NCTRLS] = {
 		.type    = V4L2_CTRL_TYPE_INTEGER,
 		.name    = "Compression Quality",
 		.minimum = 50,
-		.maximum = 94,
+		.maximum = 87,
 		.step    = 1,
 		.default_value = 75,
 	    },
@@ -241,8 +241,11 @@ static const struct v4l2_pix_format sif_mode[] = {
 		.priv = 0},
 };
 
-/* bridge reg08 bits 1-2 -> JPEG quality conversion table */
-static u8 jpeg_qual[] = {50, 75, 87, 94};
+/*
+ * Bridge reg08 bits 1-2 -> JPEG quality conversion table. Note the highest
+ * quality setting is not usable as USB 1 does not have enough bandwidth.
+ */
+static u8 jpeg_qual[] = {50, 75, 87, /* 94 */};
 
 /* usb exchanges */
 struct usb_action {
-- 
1.7.10


[-- Attachment #6: 0005-gspca_zc3xx-Use-get_jcomp-to-set-the-return-values-f.patch --]
[-- Type: text/x-patch, Size: 1694 bytes --]

>From c2a220d6cdd33615cd8dbff21c2e877a4b03c28e Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Sat, 5 May 2012 14:54:17 +0200
Subject: [PATCH 5/6] gspca_zc3xx: Use get_jcomp to set the return values from
 set_jcomp

This way not only quality gets set to the achieved value, but the
other fields of the v4l2_jpegcompression struct also get properly set.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/video/gspca/zc3xx.c |   18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/media/video/gspca/zc3xx.c b/drivers/media/video/gspca/zc3xx.c
index a8282b8..5e00a7f 100644
--- a/drivers/media/video/gspca/zc3xx.c
+++ b/drivers/media/video/gspca/zc3xx.c
@@ -6975,16 +6975,6 @@ static int sd_setquality(struct gspca_dev *gspca_dev, __s32 val)
 	return gspca_dev->usb_err;
 }
 
-static int sd_set_jcomp(struct gspca_dev *gspca_dev,
-			struct v4l2_jpegcompression *jcomp)
-{
-	struct sd *sd = (struct sd *) gspca_dev;
-
-	sd_setquality(gspca_dev, jcomp->quality);
-	jcomp->quality = sd->ctrls[QUALITY].val;
-	return gspca_dev->usb_err;
-}
-
 static int sd_get_jcomp(struct gspca_dev *gspca_dev,
 			struct v4l2_jpegcompression *jcomp)
 {
@@ -6997,6 +6987,14 @@ static int sd_get_jcomp(struct gspca_dev *gspca_dev,
 	return 0;
 }
 
+static int sd_set_jcomp(struct gspca_dev *gspca_dev,
+			struct v4l2_jpegcompression *jcomp)
+{
+	sd_setquality(gspca_dev, jcomp->quality);
+	sd_get_jcomp(gspca_dev, jcomp);
+	return gspca_dev->usb_err;
+}
+
 #if defined(CONFIG_INPUT) || defined(CONFIG_INPUT_MODULE)
 static int sd_int_pkt_scan(struct gspca_dev *gspca_dev,
 			u8 *data,		/* interrupt packet data */
-- 
1.7.10


[-- Attachment #7: 0006-gspca_zc3xx-convert-to-the-control-framework.patch --]
[-- Type: text/x-patch, Size: 18583 bytes --]

>From 6da72b85edbbcf8882d06ac1d4b9d0d991ab0142 Mon Sep 17 00:00:00 2001
From: Hans Verkuil <hans.verkuil@cisco.com>
Date: Sat, 28 Apr 2012 17:09:51 +0200
Subject: [PATCH 6/6] gspca_zc3xx: convert to the control framework

+ various bug-fixes to the conversion by Hans de Goede

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/video/gspca/zc3xx.c |  427 +++++++++++++++----------------------
 1 file changed, 167 insertions(+), 260 deletions(-)

diff --git a/drivers/media/video/gspca/zc3xx.c b/drivers/media/video/gspca/zc3xx.c
index 5e00a7f..633bb4c 100644
--- a/drivers/media/video/gspca/zc3xx.c
+++ b/drivers/media/video/gspca/zc3xx.c
@@ -22,6 +22,7 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/input.h>
+#include <media/v4l2-ctrls.h>
 #include "gspca.h"
 #include "jpeg.h"
 
@@ -35,26 +36,23 @@ static int force_sensor = -1;
 #define REG08_DEF 3		/* default JPEG compression (75%) */
 #include "zc3xx-reg.h"
 
-/* controls */
-enum e_ctrl {
-	BRIGHTNESS,
-	CONTRAST,
-	EXPOSURE,
-	GAMMA,
-	AUTOGAIN,
-	LIGHTFREQ,
-	SHARPNESS,
-	QUALITY,
-	NCTRLS		/* number of controls */
-};
-
-#define AUTOGAIN_DEF 1
-
 /* specific webcam descriptor */
 struct sd {
 	struct gspca_dev gspca_dev;	/* !! must be the first item */
 
-	struct gspca_ctrl ctrls[NCTRLS];
+	struct v4l2_ctrl_handler ctrl_handler;
+	struct { /* gamma/brightness/contrast control cluster */
+		struct v4l2_ctrl *gamma;
+		struct v4l2_ctrl *brightness;
+		struct v4l2_ctrl *contrast;
+	};
+	struct { /* autogain/exposure control cluster */
+		struct v4l2_ctrl *autogain;
+		struct v4l2_ctrl *exposure;
+	};
+	struct v4l2_ctrl *plfreq;
+	struct v4l2_ctrl *sharpness;
+	struct v4l2_ctrl *jpegqual;
 
 	struct work_struct work;
 	struct workqueue_struct *work_thread;
@@ -94,114 +92,6 @@ enum sensors {
 	SENSOR_MAX
 };
 
-/* V4L2 controls supported by the driver */
-static void setcontrast(struct gspca_dev *gspca_dev);
-static void setexposure(struct gspca_dev *gspca_dev);
-static int sd_setautogain(struct gspca_dev *gspca_dev, __s32 val);
-static void setlightfreq(struct gspca_dev *gspca_dev);
-static void setsharpness(struct gspca_dev *gspca_dev);
-static int sd_setquality(struct gspca_dev *gspca_dev, __s32 val);
-
-static const struct ctrl sd_ctrls[NCTRLS] = {
-[BRIGHTNESS] = {
-	    {
-		.id      = V4L2_CID_BRIGHTNESS,
-		.type    = V4L2_CTRL_TYPE_INTEGER,
-		.name    = "Brightness",
-		.minimum = 0,
-		.maximum = 255,
-		.step    = 1,
-		.default_value = 128,
-	    },
-	    .set_control = setcontrast
-	},
-[CONTRAST] = {
-	    {
-		.id      = V4L2_CID_CONTRAST,
-		.type    = V4L2_CTRL_TYPE_INTEGER,
-		.name    = "Contrast",
-		.minimum = 0,
-		.maximum = 255,
-		.step    = 1,
-		.default_value = 128,
-	    },
-	    .set_control = setcontrast
-	},
-[EXPOSURE] = {
-	    {
-		.id      = V4L2_CID_EXPOSURE,
-		.type    = V4L2_CTRL_TYPE_INTEGER,
-		.name    = "Exposure",
-		.minimum = 0x30d,
-		.maximum	= 0x493e,
-		.step		= 1,
-		.default_value  = 0x927
-	    },
-	    .set_control = setexposure
-	},
-[GAMMA] = {
-	    {
-		.id      = V4L2_CID_GAMMA,
-		.type    = V4L2_CTRL_TYPE_INTEGER,
-		.name    = "Gamma",
-		.minimum = 1,
-		.maximum = 6,
-		.step    = 1,
-		.default_value = 4,
-	    },
-	    .set_control = setcontrast
-	},
-[AUTOGAIN] = {
-	    {
-		.id      = V4L2_CID_AUTOGAIN,
-		.type    = V4L2_CTRL_TYPE_BOOLEAN,
-		.name    = "Auto Gain",
-		.minimum = 0,
-		.maximum = 1,
-		.step    = 1,
-		.default_value = AUTOGAIN_DEF,
-		.flags   = V4L2_CTRL_FLAG_UPDATE
-	    },
-	    .set = sd_setautogain
-	},
-[LIGHTFREQ] = {
-	    {
-		.id	 = V4L2_CID_POWER_LINE_FREQUENCY,
-		.type    = V4L2_CTRL_TYPE_MENU,
-		.name    = "Light frequency filter",
-		.minimum = 0,
-		.maximum = 2,	/* 0: 0, 1: 50Hz, 2:60Hz */
-		.step    = 1,
-		.default_value = 0,
-	    },
-	    .set_control = setlightfreq
-	},
-[SHARPNESS] = {
-	    {
-		.id	 = V4L2_CID_SHARPNESS,
-		.type    = V4L2_CTRL_TYPE_INTEGER,
-		.name    = "Sharpness",
-		.minimum = 0,
-		.maximum = 3,
-		.step    = 1,
-		.default_value = 2,
-	    },
-	    .set_control = setsharpness
-	},
-[QUALITY] = {
-	    {
-		.id	 = V4L2_CID_JPEG_COMPRESSION_QUALITY,
-		.type    = V4L2_CTRL_TYPE_INTEGER,
-		.name    = "Compression Quality",
-		.minimum = 50,
-		.maximum = 87,
-		.step    = 1,
-		.default_value = 75,
-	    },
-	    .set = sd_setquality
-	},
-};
-
 static const struct v4l2_pix_format vga_mode[] = {
 	{320, 240, V4L2_PIX_FMT_JPEG, V4L2_FIELD_NONE,
 		.bytesperline = 320,
@@ -5821,10 +5711,8 @@ static void setmatrix(struct gspca_dev *gspca_dev)
 		reg_w(gspca_dev, matrix[i], 0x010a + i);
 }
 
-static void setsharpness(struct gspca_dev *gspca_dev)
+static void setsharpness(struct gspca_dev *gspca_dev, s32 val)
 {
-	struct sd *sd = (struct sd *) gspca_dev;
-	int sharpness;
 	static const u8 sharpness_tb[][2] = {
 		{0x02, 0x03},
 		{0x04, 0x07},
@@ -5832,19 +5720,18 @@ static void setsharpness(struct gspca_dev *gspca_dev)
 		{0x10, 0x1e}
 	};
 
-	sharpness = sd->ctrls[SHARPNESS].val;
-	reg_w(gspca_dev, sharpness_tb[sharpness][0], 0x01c6);
+	reg_w(gspca_dev, sharpness_tb[val][0], 0x01c6);
 	reg_r(gspca_dev, 0x01c8);
 	reg_r(gspca_dev, 0x01c9);
 	reg_r(gspca_dev, 0x01ca);
-	reg_w(gspca_dev, sharpness_tb[sharpness][1], 0x01cb);
+	reg_w(gspca_dev, sharpness_tb[val][1], 0x01cb);
 }
 
-static void setcontrast(struct gspca_dev *gspca_dev)
+static void setcontrast(struct gspca_dev *gspca_dev,
+		s32 gamma, s32 brightness, s32 contrast)
 {
-	struct sd *sd = (struct sd *) gspca_dev;
 	const u8 *Tgamma;
-	int g, i, brightness, contrast, adj, gp1, gp2;
+	int g, i, adj, gp1, gp2;
 	u8 gr[16];
 	static const u8 delta_b[16] =		/* delta for brightness */
 		{0x50, 0x38, 0x2d, 0x28, 0x24, 0x21, 0x1e, 0x1d,
@@ -5867,10 +5754,10 @@ static void setcontrast(struct gspca_dev *gspca_dev)
 		 0xe0, 0xeb, 0xf4, 0xff, 0xff, 0xff, 0xff, 0xff},
 	};
 
-	Tgamma = gamma_tb[sd->ctrls[GAMMA].val - 1];
+	Tgamma = gamma_tb[gamma - 1];
 
-	contrast = ((int) sd->ctrls[CONTRAST].val - 128); /* -128 / 127 */
-	brightness = ((int) sd->ctrls[BRIGHTNESS].val - 128); /* -128 / 92 */
+	contrast -= 128; /* -128 / 127 */
+	brightness -= 128; /* -128 / 92 */
 	adj = 0;
 	gp1 = gp2 = 0;
 	for (i = 0; i < 16; i++) {
@@ -5897,25 +5784,15 @@ static void setcontrast(struct gspca_dev *gspca_dev)
 		reg_w(gspca_dev, gr[i], 0x0130 + i);	/* gradient */
 }
 
-static void getexposure(struct gspca_dev *gspca_dev)
+static s32 getexposure(struct gspca_dev *gspca_dev)
 {
-	struct sd *sd = (struct sd *) gspca_dev;
-
-	if (sd->sensor != SENSOR_HV7131R)
-		return;
-	sd->ctrls[EXPOSURE].val = (i2c_read(gspca_dev, 0x25) << 9)
+	return (i2c_read(gspca_dev, 0x25) << 9)
 		| (i2c_read(gspca_dev, 0x26) << 1)
 		| (i2c_read(gspca_dev, 0x27) >> 7);
 }
 
-static void setexposure(struct gspca_dev *gspca_dev)
+static void setexposure(struct gspca_dev *gspca_dev, s32 val)
 {
-	struct sd *sd = (struct sd *) gspca_dev;
-	int val;
-
-	if (sd->sensor != SENSOR_HV7131R)
-		return;
-	val = sd->ctrls[EXPOSURE].val;
 	i2c_write(gspca_dev, 0x25, val >> 9, 0x00);
 	i2c_write(gspca_dev, 0x26, val >> 1, 0x00);
 	i2c_write(gspca_dev, 0x27, val << 7, 0x00);
@@ -5934,7 +5811,7 @@ static void setquality(struct gspca_dev *gspca_dev)
  *	60Hz, for American lighting
  *	0 = No Fliker (for outdoore usage)
  */
-static void setlightfreq(struct gspca_dev *gspca_dev)
+static void setlightfreq(struct gspca_dev *gspca_dev, s32 val)
 {
 	struct sd *sd = (struct sd *) gspca_dev;
 	int i, mode;
@@ -6018,7 +5895,7 @@ static void setlightfreq(struct gspca_dev *gspca_dev)
 		 tas5130c_60HZ, tas5130c_60HZScale},
 	};
 
-	i = sd->ctrls[LIGHTFREQ].val * 2;
+	i = val * 2;
 	mode = gspca_dev->cam.cam_mode[gspca_dev->curr_mode].priv;
 	if (mode)
 		i++;			/* 320x240 */
@@ -6028,14 +5905,14 @@ static void setlightfreq(struct gspca_dev *gspca_dev)
 	usb_exchange(gspca_dev, zc3_freq);
 	switch (sd->sensor) {
 	case SENSOR_GC0305:
-		if (mode				/* if 320x240 */
-		    && sd->ctrls[LIGHTFREQ].val == 1)	/* and 50Hz */
+		if (mode		/* if 320x240 */
+		    && val == 1)	/* and 50Hz */
 			reg_w(gspca_dev, 0x85, 0x018d);
 					/* win: 0x80, 0x018d */
 		break;
 	case SENSOR_OV7620:
-		if (!mode) {				/* if 640x480 */
-			if (sd->ctrls[LIGHTFREQ].val != 0) /* and filter */
+		if (!mode) {		/* if 640x480 */
+			if (val != 0)	/* and filter */
 				reg_w(gspca_dev, 0x40, 0x0002);
 			else
 				reg_w(gspca_dev, 0x44, 0x0002);
@@ -6047,16 +5924,9 @@ static void setlightfreq(struct gspca_dev *gspca_dev)
 	}
 }
 
-static void setautogain(struct gspca_dev *gspca_dev)
+static void setautogain(struct gspca_dev *gspca_dev, s32 val)
 {
-	struct sd *sd = (struct sd *) gspca_dev;
-	u8 autoval;
-
-	if (sd->ctrls[AUTOGAIN].val)
-		autoval = 0x42;
-	else
-		autoval = 0x02;
-	reg_w(gspca_dev, autoval, 0x0180);
+	reg_w(gspca_dev, val ? 0x42 : 0x02, 0x0180);
 }
 
 /*
@@ -6449,7 +6319,6 @@ static int sd_config(struct gspca_dev *gspca_dev,
 	/* define some sensors from the vendor/product */
 	sd->sensor = id->driver_info;
 
-	gspca_dev->cam.ctrls = sd->ctrls;
 	sd->reg08 = REG08_DEF;
 
 	INIT_WORK(&sd->work, transfer_update);
@@ -6457,10 +6326,97 @@ static int sd_config(struct gspca_dev *gspca_dev,
 	return 0;
 }
 
+static int zcxx_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct sd *sd = container_of(ctrl->handler, struct sd, ctrl_handler);
+	struct gspca_dev *gspca_dev = &sd->gspca_dev;
+	int ret = 0;
+
+	switch (ctrl->id) {
+	case V4L2_CID_AUTOGAIN:
+		mutex_lock(&gspca_dev->usb_lock);
+		gspca_dev->usb_err = 0;
+		if (ctrl->val && sd->exposure && gspca_dev->streaming)
+			sd->exposure->val = getexposure(gspca_dev);
+		ret = gspca_dev->usb_err;
+		mutex_unlock(&gspca_dev->usb_lock);
+		break;
+	}
+	return ret;
+}
+
+static int zcxx_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct sd *sd = container_of(ctrl->handler, struct sd, ctrl_handler);
+	struct gspca_dev *gspca_dev = &sd->gspca_dev;
+	int i, qual, ret = 0;
+
+	mutex_lock(&gspca_dev->usb_lock);
+	gspca_dev->usb_err = 0;
+
+	if (!gspca_dev->streaming &&
+			ctrl->id != V4L2_CID_JPEG_COMPRESSION_QUALITY)
+		goto leave;
+
+	switch (ctrl->id) {
+	/* gamma/brightness/contrast cluster */
+	case V4L2_CID_GAMMA:
+		setcontrast(gspca_dev, sd->gamma->val,
+				sd->brightness->val, sd->contrast->val);
+		break;
+	/* autogain/exposure cluster */
+	case V4L2_CID_AUTOGAIN:
+		setautogain(gspca_dev, ctrl->val);
+		if (!gspca_dev->usb_err && !ctrl->val && sd->exposure)
+			setexposure(gspca_dev, sd->exposure->val);
+		break;
+	case V4L2_CID_POWER_LINE_FREQUENCY:
+		setlightfreq(gspca_dev, ctrl->val);
+		break;
+	case V4L2_CID_SHARPNESS:
+		setsharpness(gspca_dev, ctrl->val);
+		break;
+	case V4L2_CID_JPEG_COMPRESSION_QUALITY:
+		qual = sd->reg08 >> 1;
+
+		for (i = 0; i < ARRAY_SIZE(jpeg_qual); i++) {
+			if (ctrl->val <= jpeg_qual[i])
+				break;
+		}
+		if (i > 0 && i == qual && ctrl->val < jpeg_qual[i])
+			i--;
+
+		/* With high quality settings we need max bandwidth */
+		if (i >= 2 && gspca_dev->streaming &&
+		    !gspca_dev->cam.needs_full_bandwidth) {
+			ret = -EBUSY;
+			goto leave;
+		}
+
+		sd->reg08 = (i << 1) | 1;
+		ctrl->val = jpeg_qual[i];
+
+		if (gspca_dev->streaming)
+			setquality(gspca_dev);
+
+		break;
+	}
+	ret = gspca_dev->usb_err;
+leave:
+	mutex_unlock(&gspca_dev->usb_lock);
+	return ret;
+}
+
+static const struct v4l2_ctrl_ops zcxx_ctrl_ops = {
+	.g_volatile_ctrl = zcxx_g_volatile_ctrl,
+	.s_ctrl = zcxx_s_ctrl,
+};
+
 /* this function is called at probe and resume time */
 static int sd_init(struct gspca_dev *gspca_dev)
 {
 	struct sd *sd = (struct sd *) gspca_dev;
+	struct v4l2_ctrl_handler *hdl = &sd->ctrl_handler;
 	struct cam *cam;
 	int sensor;
 	static const u8 gamma[SENSOR_MAX] = {
@@ -6613,7 +6569,6 @@ static int sd_init(struct gspca_dev *gspca_dev)
 		case 0x2030:
 			PDEBUG(D_PROBE, "Find Sensor PO2030");
 			sd->sensor = SENSOR_PO2030;
-			sd->ctrls[SHARPNESS].def = 0;	/* from win traces */
 			break;
 		case 0x7620:
 			PDEBUG(D_PROBE, "Find Sensor OV7620");
@@ -6655,25 +6610,38 @@ static int sd_init(struct gspca_dev *gspca_dev)
 		break;
 	}
 
-	sd->ctrls[GAMMA].def = gamma[sd->sensor];
-	sd->ctrls[QUALITY].def = jpeg_qual[sd->reg08 >> 1];
-	sd->ctrls[QUALITY].min = jpeg_qual[0];
-	sd->ctrls[QUALITY].max = jpeg_qual[ARRAY_SIZE(jpeg_qual) - 1];
-
-	switch (sd->sensor) {
-	case SENSOR_HV7131R:
-		break;
-	case SENSOR_OV7630C:
-		gspca_dev->ctrl_dis = (1 << LIGHTFREQ) | (1 << EXPOSURE);
-		break;
-	default:
-		gspca_dev->ctrl_dis = (1 << EXPOSURE);
-		break;
+	gspca_dev->vdev.ctrl_handler = hdl;
+	v4l2_ctrl_handler_init(hdl, 8);
+	sd->brightness = v4l2_ctrl_new_std(hdl, &zcxx_ctrl_ops,
+			V4L2_CID_BRIGHTNESS, 0, 255, 1, 128);
+	sd->contrast = v4l2_ctrl_new_std(hdl, &zcxx_ctrl_ops,
+			V4L2_CID_CONTRAST, 0, 255, 1, 128);
+	sd->gamma = v4l2_ctrl_new_std(hdl, &zcxx_ctrl_ops,
+			V4L2_CID_GAMMA, 1, 6, 1, gamma[sd->sensor]);
+	if (sd->sensor == SENSOR_HV7131R)
+		sd->exposure = v4l2_ctrl_new_std(hdl, &zcxx_ctrl_ops,
+			V4L2_CID_EXPOSURE, 0x30d, 0x493e, 1, 0x927);
+	sd->autogain = v4l2_ctrl_new_std(hdl, &zcxx_ctrl_ops,
+			V4L2_CID_AUTOGAIN, 0, 1, 1, 1);
+	if (sd->sensor != SENSOR_OV7630C)
+		sd->plfreq = v4l2_ctrl_new_std_menu(hdl, &zcxx_ctrl_ops,
+			V4L2_CID_POWER_LINE_FREQUENCY,
+			V4L2_CID_POWER_LINE_FREQUENCY_60HZ, 0,
+			V4L2_CID_POWER_LINE_FREQUENCY_DISABLED);
+	sd->sharpness = v4l2_ctrl_new_std(hdl, &zcxx_ctrl_ops,
+			V4L2_CID_SHARPNESS, 0, 3, 1,
+			sd->sensor == SENSOR_PO2030 ? 0 : 2);
+	sd->jpegqual = v4l2_ctrl_new_std(hdl, &zcxx_ctrl_ops,
+			V4L2_CID_JPEG_COMPRESSION_QUALITY,
+			jpeg_qual[0], jpeg_qual[ARRAY_SIZE(jpeg_qual) - 1], 1,
+			jpeg_qual[REG08_DEF >> 1]);
+	if (hdl->error) {
+		pr_err("Could not initialize controls\n");
+		return hdl->error;
 	}
-#if AUTOGAIN_DEF
-	if (sd->ctrls[AUTOGAIN].val)
-		gspca_dev->ctrl_inac = (1 << EXPOSURE);
-#endif
+	v4l2_ctrl_cluster(3, &sd->gamma);
+	if (sd->sensor == SENSOR_HV7131R)
+		v4l2_ctrl_auto_cluster(2, &sd->autogain, 0, true);
 
 	/* switch off the led */
 	reg_w(gspca_dev, 0x01, 0x0000);
@@ -6791,7 +6759,7 @@ static int sd_start(struct gspca_dev *gspca_dev)
 		reg_w(gspca_dev, 0x03, 0x0008);
 		break;
 	}
-	setsharpness(gspca_dev);
+	setsharpness(gspca_dev, v4l2_ctrl_g_ctrl(sd->sharpness));
 
 	/* set the gamma tables when not set */
 	switch (sd->sensor) {
@@ -6800,7 +6768,9 @@ static int sd_start(struct gspca_dev *gspca_dev)
 	case SENSOR_OV7630C:
 		break;
 	default:
-		setcontrast(gspca_dev);
+		setcontrast(gspca_dev, v4l2_ctrl_g_ctrl(sd->gamma),
+				v4l2_ctrl_g_ctrl(sd->brightness),
+				v4l2_ctrl_g_ctrl(sd->contrast));
 		break;
 	}
 	setmatrix(gspca_dev);			/* one more time? */
@@ -6814,7 +6784,8 @@ static int sd_start(struct gspca_dev *gspca_dev)
 	setquality(gspca_dev);
 	/* Start with BRC disabled, transfer_update will enable it if needed */
 	reg_w(gspca_dev, 0x00, 0x0007);
-	setlightfreq(gspca_dev);
+	if (sd->plfreq)
+		setlightfreq(gspca_dev, v4l2_ctrl_g_ctrl(sd->plfreq));
 
 	switch (sd->sensor) {
 	case SENSOR_ADCM2700:
@@ -6825,7 +6796,7 @@ static int sd_start(struct gspca_dev *gspca_dev)
 		reg_w(gspca_dev, 0x40, 0x0117);
 		break;
 	case SENSOR_HV7131R:
-		setexposure(gspca_dev);
+		setexposure(gspca_dev, v4l2_ctrl_g_ctrl(sd->exposure));
 		reg_w(gspca_dev, 0x00, ZC3XX_R1A7_CALCGLOBALMEAN);
 		break;
 	case SENSOR_GC0305:
@@ -6849,7 +6820,7 @@ static int sd_start(struct gspca_dev *gspca_dev)
 		break;
 	}
 
-	setautogain(gspca_dev);
+	setautogain(gspca_dev, v4l2_ctrl_g_ctrl(sd->autogain));
 
 	if (gspca_dev->usb_err < 0)
 		return gspca_dev->usb_err;
@@ -6910,78 +6881,13 @@ static void sd_pkt_scan(struct gspca_dev *gspca_dev,
 	gspca_frame_add(gspca_dev, INTER_PACKET, data, len);
 }
 
-static int sd_setautogain(struct gspca_dev *gspca_dev, __s32 val)
-{
-	struct sd *sd = (struct sd *) gspca_dev;
-
-	sd->ctrls[AUTOGAIN].val = val;
-	if (val) {
-		gspca_dev->ctrl_inac |= (1 << EXPOSURE);
-	} else {
-		gspca_dev->ctrl_inac &= ~(1 << EXPOSURE);
-		if (gspca_dev->streaming)
-			getexposure(gspca_dev);
-	}
-	if (gspca_dev->streaming)
-		setautogain(gspca_dev);
-	return gspca_dev->usb_err;
-}
-
-static int sd_querymenu(struct gspca_dev *gspca_dev,
-			struct v4l2_querymenu *menu)
-{
-	switch (menu->id) {
-	case V4L2_CID_POWER_LINE_FREQUENCY:
-		switch (menu->index) {
-		case 0:		/* V4L2_CID_POWER_LINE_FREQUENCY_DISABLED */
-			strcpy((char *) menu->name, "NoFliker");
-			return 0;
-		case 1:		/* V4L2_CID_POWER_LINE_FREQUENCY_50HZ */
-			strcpy((char *) menu->name, "50 Hz");
-			return 0;
-		case 2:		/* V4L2_CID_POWER_LINE_FREQUENCY_60HZ */
-			strcpy((char *) menu->name, "60 Hz");
-			return 0;
-		}
-		break;
-	}
-	return -EINVAL;
-}
-
-static int sd_setquality(struct gspca_dev *gspca_dev, __s32 val)
-{
-	struct sd *sd = (struct sd *) gspca_dev;
-	int i, qual = sd->reg08 >> 1;
-
-	for (i = 0; i < ARRAY_SIZE(jpeg_qual); i++) {
-		if (val <= jpeg_qual[i])
-			break;
-	}
-	if (i > 0
-	 && i == qual
-	 && val < jpeg_qual[i])
-		i--;
-
-	/* With high quality settings we need max bandwidth */
-	if (i >= 2 && gspca_dev->streaming &&
-	    !gspca_dev->cam.needs_full_bandwidth)
-		return -EBUSY;
-
-	sd->reg08 = (i << 1) | 1;
-	sd->ctrls[QUALITY].val = jpeg_qual[i];
-
-	if (gspca_dev->streaming)
-		setquality(gspca_dev);
-	return gspca_dev->usb_err;
-}
-
 static int sd_get_jcomp(struct gspca_dev *gspca_dev,
 			struct v4l2_jpegcompression *jcomp)
 {
 	struct sd *sd = (struct sd *) gspca_dev;
 
 	memset(jcomp, 0, sizeof *jcomp);
-	jcomp->quality = sd->ctrls[QUALITY].val;
+	jcomp->quality = v4l2_ctrl_g_ctrl(sd->jpegqual);
 	jcomp->jpeg_markers = V4L2_JPEG_MARKER_DHT
 			| V4L2_JPEG_MARKER_DQT;
 	return 0;
@@ -6990,9 +6896,13 @@ static int sd_get_jcomp(struct gspca_dev *gspca_dev,
 static int sd_set_jcomp(struct gspca_dev *gspca_dev,
 			struct v4l2_jpegcompression *jcomp)
 {
-	sd_setquality(gspca_dev, jcomp->quality);
+	struct sd *sd = (struct sd *) gspca_dev;
+	int ret;
+
+	ret = v4l2_ctrl_s_ctrl(sd->jpegqual, jcomp->quality);
 	sd_get_jcomp(gspca_dev, jcomp);
-	return gspca_dev->usb_err;
+
+	return ret;
 }
 
 #if defined(CONFIG_INPUT) || defined(CONFIG_INPUT_MODULE)
@@ -7013,15 +6923,12 @@ static int sd_int_pkt_scan(struct gspca_dev *gspca_dev,
 
 static const struct sd_desc sd_desc = {
 	.name = KBUILD_MODNAME,
-	.ctrls = sd_ctrls,
-	.nctrls = ARRAY_SIZE(sd_ctrls),
 	.config = sd_config,
 	.init = sd_init,
 	.isoc_init = sd_pre_start,
 	.start = sd_start,
 	.stop0 = sd_stop0,
 	.pkt_scan = sd_pkt_scan,
-	.querymenu = sd_querymenu,
 	.get_jcomp = sd_get_jcomp,
 	.set_jcomp = sd_set_jcomp,
 #if defined(CONFIG_INPUT) || defined(CONFIG_INPUT_MODULE)
-- 
1.7.10

Re: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.

[Hans de Goede]

Hi,

On 05/05/2012 11:14 AM, Hans Verkuil wrote:
> On Sat May 5 2012 09:43:01 Hans de Goede wrote:
>> Hi,
>>
>> I'm slowly working my way though this series today (both review, as well
>> as some tweaks and testing).
>>
>> More comments inline...
>>
>> On 04/28/2012 05:09 PM, Hans Verkuil wrote:
>>> From: Hans Verkuil<hans.verkuil@cisco.com>
>>>
>>> Make the necessary changes to allow subdrivers to use the control framework.
>>> This does not add control event support, that needs more work.
>>>
>>> Signed-off-by: Hans Verkuil<hans.verkuil@cisco.com>
>>> ---
>>>    drivers/media/video/gspca/gspca.c |   13 +++++++++----
>>>    1 file changed, 9 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/media/video/gspca/gspca.c b/drivers/media/video/gspca/gspca.c
>>> index ca5a2b1..56dff10 100644
>>> --- a/drivers/media/video/gspca/gspca.c
>>> +++ b/drivers/media/video/gspca/gspca.c
>>> @@ -38,6 +38,7 @@
>>>    #include<linux/uaccess.h>
>>>    #include<linux/ktime.h>
>>>    #include<media/v4l2-ioctl.h>
>>> +#include<media/v4l2-ctrls.h>
>>>
>>>    #include "gspca.h"
>>>
>>> @@ -1006,6 +1007,8 @@ static void gspca_set_default_mode(struct gspca_dev *gspca_dev)
>>>
>>>    	/* set the current control values to their default values
>>>    	 * which may have changed in sd_init() */
>>> +	/* does nothing if ctrl_handler == NULL */
>>> +	v4l2_ctrl_handler_setup(gspca_dev->vdev.ctrl_handler);
>>>    	ctrl = gspca_dev->cam.ctrls;
>>>    	if (ctrl != NULL) {
>>>    		for (i = 0;
>>> @@ -1323,6 +1326,7 @@ static void gspca_release(struct video_device *vfd)
>>>    	PDEBUG(D_PROBE, "%s released",
>>>    		video_device_node_name(&gspca_dev->vdev));
>>>
>>> +	v4l2_ctrl_handler_free(gspca_dev->vdev.ctrl_handler);
>>>    	kfree(gspca_dev->usb_buf);
>>>    	kfree(gspca_dev);
>>>    }
>>> @@ -2347,6 +2351,10 @@ int gspca_dev_probe2(struct usb_interface *intf,
>>>    	gspca_dev->sd_desc = sd_desc;
>>>    	gspca_dev->nbufread = 2;
>>>    	gspca_dev->empty_packet = -1;	/* don't check the empty packets */
>>> +	gspca_dev->vdev = gspca_template;
>>> +	gspca_dev->vdev.parent =&intf->dev;
>>> +	gspca_dev->module = module;
>>> +	gspca_dev->present = 1;
>>>
>>>    	/* configure the subdriver and initialize the USB device */
>>>    	ret = sd_desc->config(gspca_dev, id);
>>
>> You also need to move the initialization of the mutexes here, as the
>> v4l2_ctrl_handler_setup will call s_ctrl on all the controls, and s_ctrl
>> should take the usb_lock (see my review of the next patch in this series),
>> I'll make this change myself and merge it into your patch.
>
> Looking at how usb_lock is used I am inclined to just set video_device->lock
> to it and let the v4l2 core do all the locking for me, which will automatically
> fix the missing s_ctrl lock too.

Please don't I've really bad experience with this with pwc (large latencies
on dqbuf), also gspca uses worker-threads which issue usb control requests
in various places, currently these take usb_lock to avoid them fighting with
sd_start or controls, these won't know about the global device lock and if
these start taking that lock too things will become pretty messy pretty quickly.

>
> I've realized that there is a problem if you do your own locking *and* use the
> control framework: if you need to set a control from within the driver, then
> you do that using v4l2_ctrl_s_ctrl. But if s_ctrl has to take the driver's lock,
> then you can't call v4l2_ctrl_s_ctrl with that lock already taken!
>
> So you get:
>
> vidioc_foo()
> 	lock(mylock)
> 	v4l2_ctrl_s_ctrl(ctrl, val)
> 		s_ctrl(ctrl, val)
> 			lock(mylock)

Easy solution here, remove the first lock(mylock), since we are not using v4l2-dev's
locking, we are the one doing the first lock, and if we are going to call v4l2_ctrl_s_ctrl
we should simply not do that!

Now I see that we are doing exactly that in for example vidioc_g_jpegcomp in gspca.c, so
we should stop doing that. We can make vidioc_g/s_jpegcomp only do the usb locking if
gspca_dev->vdev.ctrl_handler == NULL, and once all sub drivers are converted simply remove
it. Actually I'm thinking about making the jpegqual control part of the gspca_dev struct
itself and move all handling of vidioc_g/s_jpegcomp out of the sub drivers and into
the core.

Regards,

Hans

Re: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.

[Hans de Goede]

Hi,

On 05/05/2012 10:34 AM, Hans Verkuil wrote:
> On Sat May 5 2012 09:43:01 Hans de Goede wrote:
>> Hi,
>>
>> I'm slowly working my way though this series today (both review, as well
>> as some tweaks and testing).
>
> Thanks for that!
>
> One note: I initialized the controls in sd_init. That's wrong, it should be
> sd_config. sd_init is also called on resume, so that would initialize the
> controls twice.

You cannot move the initializing of the controls to sd_config, since in many
cases the sensor probing is done in sd_init, and we need to know the sensor
type to init the controls. I suggest that instead you give the sd_init
function a resume parameter and only init the controls if the resume parameter
is false.

> I'm working on this as well today, together with finishing the stv06xx and
> mars conversion.

Cool!

Regards,

Hans

Re: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.

[Hans Verkuil]

On Sat May 5 2012 16:46:32 Hans de Goede wrote:
> Hi,
> 
> On 05/05/2012 10:34 AM, Hans Verkuil wrote:
> > On Sat May 5 2012 09:43:01 Hans de Goede wrote:
> >> Hi,
> >>
> >> I'm slowly working my way though this series today (both review, as well
> >> as some tweaks and testing).
> >
> > Thanks for that!
> >
> > One note: I initialized the controls in sd_init. That's wrong, it should be
> > sd_config. sd_init is also called on resume, so that would initialize the
> > controls twice.
> 
> You cannot move the initializing of the controls to sd_config, since in many
> cases the sensor probing is done in sd_init, and we need to know the sensor
> type to init the controls.

Or you move the sensor probing to sd_config as I did. It makes no sense
anyway to do sensor probing every time you resume.

Unless there is another good reason for doing the probing in sd_init I prefer
to move it to sd_config.

Regards,

	Hans

> I suggest that instead you give the sd_init
> function a resume parameter and only init the controls if the resume parameter
> is false.
> 
> > I'm working on this as well today, together with finishing the stv06xx and
> > mars conversion.
> 
> Cool!
> 
> Regards,
> 
> Hans
> 

Re: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.

[Hans de Goede]

Hi,

On 05/05/2012 04:50 PM, Hans Verkuil wrote:
> On Sat May 5 2012 16:46:32 Hans de Goede wrote:
>> Hi,
>>
>> On 05/05/2012 10:34 AM, Hans Verkuil wrote:
>>> On Sat May 5 2012 09:43:01 Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> I'm slowly working my way though this series today (both review, as well
>>>> as some tweaks and testing).
>>>
>>> Thanks for that!
>>>
>>> One note: I initialized the controls in sd_init. That's wrong, it should be
>>> sd_config. sd_init is also called on resume, so that would initialize the
>>> controls twice.
>>
>> You cannot move the initializing of the controls to sd_config, since in many
>> cases the sensor probing is done in sd_init, and we need to know the sensor
>> type to init the controls.
>
> Or you move the sensor probing to sd_config as I did. It makes no sense
> anyway to do sensor probing every time you resume.
>
> Unless there is another good reason for doing the probing in sd_init I prefer
> to move it to sd_config.

Sensor probing does more then just sensor probing, it also configures
things like the i2c clockrate, and if the bus between bridge and sensor
is spi / i2c or 3-wire, or whatever ...

After a suspend resume all bets are of wrt bridge state, so we prefer to
always do a full re-init as we do on initial probe, so that we (hopefully)
will put the bridge back in a sane state.

I think moving the probing from init to config is a bad idea, the chance
that we will get regressions (after a suspend/resume) from this are too
big IMHO.

Regards,

Hans

Re: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.

[Hans Verkuil]

On Sat May 5 2012 16:44:34 Hans de Goede wrote:
> Hi,
> 
> On 05/05/2012 11:14 AM, Hans Verkuil wrote:
> > On Sat May 5 2012 09:43:01 Hans de Goede wrote:
> >> Hi,
> >>
> >> I'm slowly working my way though this series today (both review, as well
> >> as some tweaks and testing).
> >>
> >> More comments inline...
> >>
> >> On 04/28/2012 05:09 PM, Hans Verkuil wrote:
> >>> From: Hans Verkuil<hans.verkuil@cisco.com>
> >>>
> >>> Make the necessary changes to allow subdrivers to use the control framework.
> >>> This does not add control event support, that needs more work.
> >>>
> >>> Signed-off-by: Hans Verkuil<hans.verkuil@cisco.com>
> >>> ---
> >>>    drivers/media/video/gspca/gspca.c |   13 +++++++++----
> >>>    1 file changed, 9 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/media/video/gspca/gspca.c b/drivers/media/video/gspca/gspca.c
> >>> index ca5a2b1..56dff10 100644
> >>> --- a/drivers/media/video/gspca/gspca.c
> >>> +++ b/drivers/media/video/gspca/gspca.c
> >>> @@ -38,6 +38,7 @@
> >>>    #include<linux/uaccess.h>
> >>>    #include<linux/ktime.h>
> >>>    #include<media/v4l2-ioctl.h>
> >>> +#include<media/v4l2-ctrls.h>
> >>>
> >>>    #include "gspca.h"
> >>>
> >>> @@ -1006,6 +1007,8 @@ static void gspca_set_default_mode(struct gspca_dev *gspca_dev)
> >>>
> >>>    	/* set the current control values to their default values
> >>>    	 * which may have changed in sd_init() */
> >>> +	/* does nothing if ctrl_handler == NULL */
> >>> +	v4l2_ctrl_handler_setup(gspca_dev->vdev.ctrl_handler);
> >>>    	ctrl = gspca_dev->cam.ctrls;
> >>>    	if (ctrl != NULL) {
> >>>    		for (i = 0;
> >>> @@ -1323,6 +1326,7 @@ static void gspca_release(struct video_device *vfd)
> >>>    	PDEBUG(D_PROBE, "%s released",
> >>>    		video_device_node_name(&gspca_dev->vdev));
> >>>
> >>> +	v4l2_ctrl_handler_free(gspca_dev->vdev.ctrl_handler);
> >>>    	kfree(gspca_dev->usb_buf);
> >>>    	kfree(gspca_dev);
> >>>    }
> >>> @@ -2347,6 +2351,10 @@ int gspca_dev_probe2(struct usb_interface *intf,
> >>>    	gspca_dev->sd_desc = sd_desc;
> >>>    	gspca_dev->nbufread = 2;
> >>>    	gspca_dev->empty_packet = -1;	/* don't check the empty packets */
> >>> +	gspca_dev->vdev = gspca_template;
> >>> +	gspca_dev->vdev.parent =&intf->dev;
> >>> +	gspca_dev->module = module;
> >>> +	gspca_dev->present = 1;
> >>>
> >>>    	/* configure the subdriver and initialize the USB device */
> >>>    	ret = sd_desc->config(gspca_dev, id);
> >>
> >> You also need to move the initialization of the mutexes here, as the
> >> v4l2_ctrl_handler_setup will call s_ctrl on all the controls, and s_ctrl
> >> should take the usb_lock (see my review of the next patch in this series),
> >> I'll make this change myself and merge it into your patch.
> >
> > Looking at how usb_lock is used I am inclined to just set video_device->lock
> > to it and let the v4l2 core do all the locking for me, which will automatically
> > fix the missing s_ctrl lock too.
> 
> Please don't I've really bad experience with this with pwc (large latencies
> on dqbuf),

Can you refresh my memory on that? I remember that you had problems with it
but I don't think I ever followed up on it (i.e. trying to find a solution
that works with core locking).

> also gspca uses worker-threads which issue usb control requests
> in various places, currently these take usb_lock to avoid them fighting with
> sd_start or controls, these won't know about the global device lock and if
> these start taking that lock too things will become pretty messy pretty quickly.

Well, gspca takes usb_lock at the same places the core lock is taken. And it
actually fails to take the lock in the suspend and resume functions which can
lead to an oops (reproduced). Those worker threads do not handle suspend/resume
well at the moment (I have patches for that).

> 
> >
> > I've realized that there is a problem if you do your own locking *and* use the
> > control framework: if you need to set a control from within the driver, then
> > you do that using v4l2_ctrl_s_ctrl. But if s_ctrl has to take the driver's lock,
> > then you can't call v4l2_ctrl_s_ctrl with that lock already taken!
> >
> > So you get:
> >
> > vidioc_foo()
> > 	lock(mylock)
> > 	v4l2_ctrl_s_ctrl(ctrl, val)
> > 		s_ctrl(ctrl, val)
> > 			lock(mylock)
> 
> Easy solution here, remove the first lock(mylock), since we are not using v4l2-dev's
> locking, we are the one doing the first lock, and if we are going to call v4l2_ctrl_s_ctrl
> we should simply not do that!

It's really ugly that way: you basically take a low-level lock (that of the
control handler which is held whenever s_ctrl et al is called) first, then take
a high-level lock (usb_lock). That's asking for problems.

> Now I see that we are doing exactly that in for example vidioc_g_jpegcomp in gspca.c, so
> we should stop doing that. We can make vidioc_g/s_jpegcomp only do the usb locking if
> gspca_dev->vdev.ctrl_handler == NULL, and once all sub drivers are converted simply remove
> it. Actually I'm thinking about making the jpegqual control part of the gspca_dev struct
> itself and move all handling of vidioc_g/s_jpegcomp out of the sub drivers and into
> the core.

In this specific case you can do that, yes. Although the big advantage of using
core locking is that that makes it easy to prove locking correctness.

BTW, this is getting messy: both of us producing patches for the same driver :-)

My latest patch series is here:

http://git.linuxtv.org/hverkuil/media_tree.git/shortlog/refs/heads/gspca

I want to consolidate it a bit more and check for streaming in all ctrl ops and
set/return usb_err correctly (I don't do that at the moment).

I'm also avoiding making too many other changes: it's hard enough already. I see
much that can be improved, but I try to concentrate on the task itself.

BTW, I've now tested the zc3xx, mars, sn9c20x and stv06xx drivers: for all of
them suspend/resume now works and I've tested them as well on a big-endian
system :-)

Regards,

	Hans

Re: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.

[Hans de Goede]

Hi,

On 05/05/2012 04:44 PM, Hans de Goede wrote:
> Hi,
>
> On 05/05/2012 11:14 AM, Hans Verkuil wrote:
>> So you get:
>>
>> vidioc_foo()
>> lock(mylock)
>> v4l2_ctrl_s_ctrl(ctrl, val)
>> s_ctrl(ctrl, val)
>> lock(mylock)
>
> Easy solution here, remove the first lock(mylock), since we are not using v4l2-dev's
> locking, we are the one doing the first lock, and if we are going to call v4l2_ctrl_s_ctrl
> we should simply not do that!
>
> Now I see that we are doing exactly that in for example vidioc_g_jpegcomp in gspca.c, so
> we should stop doing that. We can make vidioc_g/s_jpegcomp only do the usb locking if
> gspca_dev->vdev.ctrl_handler == NULL, and once all sub drivers are converted simply remove
> it. Actually I'm thinking about making the jpegqual control part of the gspca_dev struct
> itself and move all handling of vidioc_g/s_jpegcomp out of the sub drivers and into
> the core.

Here is an updated version of this patch implementing this approach for
vidioc_g/s_jpegcomp. We may need to do something similar in other places, although I cannot
think of any such places atm,

Regards,

Hans

Re: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.

[Hans de Goede]

[-- Attachment #1: Type: text/plain, Size: 1465 bytes --]

Hi,

Oops, forgot the attachment it is here now...

Regards,

Hans


On 05/05/2012 05:05 PM, Hans de Goede wrote:
> Hi,
>
> On 05/05/2012 04:44 PM, Hans de Goede wrote:
>> Hi,
>>
>> On 05/05/2012 11:14 AM, Hans Verkuil wrote:
>>> So you get:
>>>
>>> vidioc_foo()
>>> lock(mylock)
>>> v4l2_ctrl_s_ctrl(ctrl, val)
>>> s_ctrl(ctrl, val)
>>> lock(mylock)
>>
>> Easy solution here, remove the first lock(mylock), since we are not using v4l2-dev's
>> locking, we are the one doing the first lock, and if we are going to call v4l2_ctrl_s_ctrl
>> we should simply not do that!
>>
>> Now I see that we are doing exactly that in for example vidioc_g_jpegcomp in gspca.c, so
>> we should stop doing that. We can make vidioc_g/s_jpegcomp only do the usb locking if
>> gspca_dev->vdev.ctrl_handler == NULL, and once all sub drivers are converted simply remove
>> it. Actually I'm thinking about making the jpegqual control part of the gspca_dev struct
>> itself and move all handling of vidioc_g/s_jpegcomp out of the sub drivers and into
>> the core.
>
> Here is an updated version of this patch implementing this approach for
> vidioc_g/s_jpegcomp. We may need to do something similar in other places, although I cannot
> think of any such places atm,
>
> Regards,
>
> Hans
> --
> 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

[-- Attachment #2: 0001-gspca-allow-subdrivers-to-use-the-control-framework.patch --]
[-- Type: text/x-patch, Size: 4264 bytes --]

>From eb7eb7c63156c1c040a7fddaeddcf1b1891f0fb7 Mon Sep 17 00:00:00 2001
From: Hans Verkuil <hans.verkuil@cisco.com>
Date: Sat, 28 Apr 2012 17:09:50 +0200
Subject: [PATCH 1/8] gspca: allow subdrivers to use the control framework.

Make the necessary changes to allow subdrivers to use the control framework.
This does not add control event support, that needs more work.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/video/gspca/gspca.c |   47 ++++++++++++++++++++++++++-----------
 1 file changed, 33 insertions(+), 14 deletions(-)

diff --git a/drivers/media/video/gspca/gspca.c b/drivers/media/video/gspca/gspca.c
index ca5a2b1..dfe2e8a 100644
--- a/drivers/media/video/gspca/gspca.c
+++ b/drivers/media/video/gspca/gspca.c
@@ -38,6 +38,7 @@
 #include <linux/uaccess.h>
 #include <linux/ktime.h>
 #include <media/v4l2-ioctl.h>
+#include <media/v4l2-ctrls.h>
 
 #include "gspca.h"
 
@@ -1006,6 +1007,8 @@ static void gspca_set_default_mode(struct gspca_dev *gspca_dev)
 
 	/* set the current control values to their default values
 	 * which may have changed in sd_init() */
+	/* does nothing if ctrl_handler == NULL */
+	v4l2_ctrl_handler_setup(gspca_dev->vdev.ctrl_handler);
 	ctrl = gspca_dev->cam.ctrls;
 	if (ctrl != NULL) {
 		for (i = 0;
@@ -1323,6 +1326,7 @@ static void gspca_release(struct video_device *vfd)
 	PDEBUG(D_PROBE, "%s released",
 		video_device_node_name(&gspca_dev->vdev));
 
+	v4l2_ctrl_handler_free(gspca_dev->vdev.ctrl_handler);
 	kfree(gspca_dev->usb_buf);
 	kfree(gspca_dev);
 }
@@ -1771,14 +1775,21 @@ static int vidioc_g_jpegcomp(struct file *file, void *priv,
 
 	if (!gspca_dev->sd_desc->get_jcomp)
 		return -EINVAL;
-	if (mutex_lock_interruptible(&gspca_dev->usb_lock))
-		return -ERESTARTSYS;
+
+	/* Don't take the usb_lock when using the v4l2-ctrls framework */
+	if (gspca_dev->vdev.ctrl_handler == NULL)
+		if (mutex_lock_interruptible(&gspca_dev->usb_lock))
+			return -ERESTARTSYS;
+
 	gspca_dev->usb_err = 0;
 	if (gspca_dev->present)
 		ret = gspca_dev->sd_desc->get_jcomp(gspca_dev, jpegcomp);
 	else
 		ret = -ENODEV;
-	mutex_unlock(&gspca_dev->usb_lock);
+
+	if (gspca_dev->vdev.ctrl_handler == NULL)
+		mutex_unlock(&gspca_dev->usb_lock);
+
 	return ret;
 }
 
@@ -1790,14 +1801,21 @@ static int vidioc_s_jpegcomp(struct file *file, void *priv,
 
 	if (!gspca_dev->sd_desc->set_jcomp)
 		return -EINVAL;
-	if (mutex_lock_interruptible(&gspca_dev->usb_lock))
-		return -ERESTARTSYS;
+
+	/* Don't take the usb_lock when using the v4l2-ctrls framework */
+	if (gspca_dev->vdev.ctrl_handler == NULL)
+		if (mutex_lock_interruptible(&gspca_dev->usb_lock))
+			return -ERESTARTSYS;
+
 	gspca_dev->usb_err = 0;
 	if (gspca_dev->present)
 		ret = gspca_dev->sd_desc->set_jcomp(gspca_dev, jpegcomp);
 	else
 		ret = -ENODEV;
-	mutex_unlock(&gspca_dev->usb_lock);
+
+	if (gspca_dev->vdev.ctrl_handler == NULL)
+		mutex_unlock(&gspca_dev->usb_lock);
+
 	return ret;
 }
 
@@ -2347,6 +2365,14 @@ int gspca_dev_probe2(struct usb_interface *intf,
 	gspca_dev->sd_desc = sd_desc;
 	gspca_dev->nbufread = 2;
 	gspca_dev->empty_packet = -1;	/* don't check the empty packets */
+	gspca_dev->vdev = gspca_template;
+	gspca_dev->vdev.parent = &intf->dev;
+	gspca_dev->module = module;
+	gspca_dev->present = 1;
+
+	mutex_init(&gspca_dev->usb_lock);
+	mutex_init(&gspca_dev->queue_lock);
+	init_waitqueue_head(&gspca_dev->wq);
 
 	/* configure the subdriver and initialize the USB device */
 	ret = sd_desc->config(gspca_dev, id);
@@ -2363,15 +2389,7 @@ int gspca_dev_probe2(struct usb_interface *intf,
 	if (ret)
 		goto out;
 
-	mutex_init(&gspca_dev->usb_lock);
-	mutex_init(&gspca_dev->queue_lock);
-	init_waitqueue_head(&gspca_dev->wq);
-
 	/* init video stuff */
-	memcpy(&gspca_dev->vdev, &gspca_template, sizeof gspca_template);
-	gspca_dev->vdev.parent = &intf->dev;
-	gspca_dev->module = module;
-	gspca_dev->present = 1;
 	ret = video_register_device(&gspca_dev->vdev,
 				  VFL_TYPE_GRABBER,
 				  -1);
@@ -2391,6 +2409,7 @@ out:
 	if (gspca_dev->input_dev)
 		input_unregister_device(gspca_dev->input_dev);
 #endif
+	v4l2_ctrl_handler_free(gspca_dev->vdev.ctrl_handler);
 	kfree(gspca_dev->usb_buf);
 	kfree(gspca_dev);
 	return ret;
-- 
1.7.10

Re: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.

[Hans de Goede]

Hi,

On 05/05/2012 05:02 PM, Hans Verkuil wrote:
> BTW, this is getting messy: both of us producing patches for the same driver :-)

For those reading along on the mailinglist, we've got together on irc and
coordinated how we are going to handle this there.

Regards,

Hans

Re: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.

[Jean-Francois Moine]

On Sat, 05 May 2012 16:59:30 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> > Unless there is another good reason for doing the probing in sd_init I prefer
> > to move it to sd_config.  
> 
> Sensor probing does more then just sensor probing, it also configures
> things like the i2c clockrate, and if the bus between bridge and sensor
> is spi / i2c or 3-wire, or whatever ...
> 
> After a suspend resume all bets are of wrt bridge state, so we prefer to
> always do a full re-init as we do on initial probe, so that we (hopefully)
> will put the bridge back in a sane state.
> 
> I think moving the probing from init to config is a bad idea, the chance
> that we will get regressions (after a suspend/resume) from this are too
> big IMHO.

Moving the sensor probing to sd_config is normally safe because the
init sequences which are sent actually after probing do all the
re-initialization job. An easy way to know it in zc3xx is to force the
sensor via the module parameter.

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

Re: [RFCv1 PATCH 1/7] gspca: allow subdrivers to use the control framework.

[Jean-Francois Moine]

On Sat, 05 May 2012 17:05:28 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> > Now I see that we are doing exactly that in for example vidioc_g_jpegcomp in gspca.c, so
> > we should stop doing that. We can make vidioc_g/s_jpegcomp only do the usb locking if
> > gspca_dev->vdev.ctrl_handler == NULL, and once all sub drivers are converted simply remove
> > it. Actually I'm thinking about making the jpegqual control part of the gspca_dev struct
> > itself and move all handling of vidioc_g/s_jpegcomp out of the sub drivers and into
> > the core.  
> 
> Here is an updated version of this patch implementing this approach for
> vidioc_g/s_jpegcomp. We may need to do something similar in other places, although I cannot
> think of any such places atm,

As the JPEG parameters have been redefined as standard controls, and as
there should be only a very few applications which use it, I think the
vidioc_g/s_jpegcomp code could be fully removed.

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