[RFCv2 PATCH 00/17] gspca: allow use of control framework and other fixes

[RFCv2 PATCH 00/17] gspca: allow use of control framework and other fixes

[Hans Verkuil]

Hi all,

Here is my second version of this patch series.

Thanks to Hans de Goede for his help.

Changes since v1:

- HdG provided some patches that fix zc3xx-related jpeg problems
- controls are now initialized in a new cam_op: init_controls
- converted the stv06xx and mars drivers as well
- switched to V4L2 core locking, except for the DQBUF/QBUF/QUERYBUF ioctls.

Regarding locking: there are two possible methods: either do it all in the
driver, or use the V4L2 core serialization lock. Using the core lock is easy,
but, as HdG pointed out to me, it can introduce unnecessary latency if you
try to dequeue a buffer that is available, but someone else is changing a
control at the same time and is holding the core lock.

The problem on the other hand with driver locking is that in order to change
values in a control handler in the driver (which happens frequently in gspca),
you need to call v4l2_ctrl_[gs]_ctrl in order to correctly take a mutex in the
control handler, preventing someone else from changing the control at the same
time.

Since the driver needs to take its own lock when setting a control you run
into a problem: calling v4l2_ctrl_[gs]_ctrl can only be done if you are not
holding your driver lock, otherwise the s_ctrl op that actually sets the new
control value will also try to get that same lock: deadlock!

It's possible to solve this, but that requires pushing all the locking down
into the subdrivers. And that's a lot of work.

Instead I decided to make it possible to skip taking the core lock for
selected ioctls. The first patch adds that functionality.

This works much better and reduces the code size instead of adding to it.

Tested all four subdrivers with suspend/resume.

HdG: I propose that you take over unless you disagree with my locking changes
or if there are new things that crop up.

The git tree is here: git://linuxtv.org/hverkuil/media_tree.git gspca3

I will take on the first two core patches (I've got some more in that area
as well) and process them myself.

Regards,

	Hans

[RFCv2 PATCH 01/17] v4l2-dev: make it possible to skip locking for selected ioctls.

[Hans Verkuil]

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

Using the V4L2 core lock is a very robust method that is usually very good
at doing the right thing. But some drivers, particularly USB drivers, may
want to prevent the core from taking the lock for specific ioctls, particularly
buffer queuing ioctls.

The reason is that certain commands like S_CTRL can take a long time to process
over USB and all the time the core has the lock, preventing VIDIOC_DQBUF from
proceeding, even though a frame may be ready in the queue.

This introduces unwanted latency.

Since the buffer queuing commands often have their own internal lock it is
often not necessary to take the core lock. Drivers can now say that they don't
want the core to take the lock for specific ioctls.

As it is a specific opt-out it makes it clear to the reviewer that those
ioctls will need more care.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 Documentation/video4linux/v4l2-framework.txt |   27 +++-
 drivers/media/video/v4l2-dev.c               |   14 +-
 drivers/media/video/v4l2-ioctl.c             |  183 ++++++++++++++------------
 include/media/v4l2-dev.h                     |   11 ++
 4 files changed, 145 insertions(+), 90 deletions(-)

diff --git a/Documentation/video4linux/v4l2-framework.txt b/Documentation/video4linux/v4l2-framework.txt
index e3dfc26..4a313d8 100644
--- a/Documentation/video4linux/v4l2-framework.txt
+++ b/Documentation/video4linux/v4l2-framework.txt
@@ -559,19 +559,25 @@ allocated memory.
 You should also set these fields:
 
 - v4l2_dev: set to the v4l2_device parent device.
+
 - name: set to something descriptive and unique.
+
 - fops: set to the v4l2_file_operations struct.
+
 - ioctl_ops: if you use the v4l2_ioctl_ops to simplify ioctl maintenance
   (highly recommended to use this and it might become compulsory in the
   future!), then set this to your v4l2_ioctl_ops struct.
+
 - lock: leave to NULL if you want to do all the locking in the driver.
   Otherwise you give it a pointer to a struct mutex_lock and before any
   of the v4l2_file_operations is called this lock will be taken by the
-  core and released afterwards.
+  core and released afterwards. See the next section for more details.
+
 - prio: keeps track of the priorities. Used to implement VIDIOC_G/S_PRIORITY.
   If left to NULL, then it will use the struct v4l2_prio_state in v4l2_device.
   If you want to have a separate priority state per (group of) device node(s),
   then you can point it to your own struct v4l2_prio_state.
+
 - parent: you only set this if v4l2_device was registered with NULL as
   the parent device struct. This only happens in cases where one hardware
   device has multiple PCI devices that all share the same v4l2_device core.
@@ -581,6 +587,7 @@ You should also set these fields:
   (cx8802). Since the v4l2_device cannot be associated with a particular
   PCI device it is setup without a parent device. But when the struct
   video_device is setup you do know which parent PCI device to use.
+
 - flags: optional. Set to V4L2_FL_USE_FH_PRIO if you want to let the framework
   handle the VIDIOC_G/S_PRIORITY ioctls. This requires that you use struct
   v4l2_fh. Eventually this flag will disappear once all drivers use the core
@@ -613,8 +620,22 @@ v4l2_file_operations and locking
 --------------------------------
 
 You can set a pointer to a mutex_lock in struct video_device. Usually this
-will be either a top-level mutex or a mutex per device node. If you want
-finer-grained locking then you have to set it to NULL and do you own locking.
+will be either a top-level mutex or a mutex per device node. By default this
+lock will be used for each file operation and ioctl, but you can disable
+locking for selected ioctls by calling:
+
+	void v4l2_dont_use_lock(struct video_device *vdev, unsigned int cmd);
+
+E.g.: v4l2_dont_use_lock(vdev, VIDIOC_DQBUF);
+
+You have to call this before you register the video_device.
+
+Particularly with USB drivers where certain commands such as setting controls
+can take a long time you may want to do your own locking for the buffer queuing
+ioctls.
+
+If you want still finer-grained locking then you have to set mutex_lock to NULL
+and do you own locking completely.
 
 It is up to the driver developer to decide which method to use. However, if
 your driver has high-latency operations (for example, changing the exposure
diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
index 70bec54..a51a061 100644
--- a/drivers/media/video/v4l2-dev.c
+++ b/drivers/media/video/v4l2-dev.c
@@ -322,11 +322,19 @@ static long v4l2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	int ret = -ENODEV;
 
 	if (vdev->fops->unlocked_ioctl) {
-		if (vdev->lock && mutex_lock_interruptible(vdev->lock))
-			return -ERESTARTSYS;
+		bool locked = false;
+
+		if (vdev->lock) {
+			/* always lock unless the cmd is marked as "don't use lock" */
+			locked = !v4l2_is_valid_ioctl(cmd) ||
+				 !test_bit(_IOC_NR(cmd), vdev->dont_use_lock);
+
+			if (locked && mutex_lock_interruptible(vdev->lock))
+				return -ERESTARTSYS;
+		}
 		if (video_is_registered(vdev))
 			ret = vdev->fops->unlocked_ioctl(filp, cmd, arg);
-		if (vdev->lock)
+		if (locked)
 			mutex_unlock(vdev->lock);
 	} else if (vdev->fops->ioctl) {
 		/* This code path is a replacement for the BKL. It is a major
diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
index 5b2ec1f..f3c989d 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
@@ -195,93 +195,108 @@ static const char *v4l2_memory_names[] = {
 
 /* ------------------------------------------------------------------ */
 /* debug help functions                                               */
-static const char *v4l2_ioctls[] = {
-	[_IOC_NR(VIDIOC_QUERYCAP)]         = "VIDIOC_QUERYCAP",
-	[_IOC_NR(VIDIOC_RESERVED)]         = "VIDIOC_RESERVED",
-	[_IOC_NR(VIDIOC_ENUM_FMT)]         = "VIDIOC_ENUM_FMT",
-	[_IOC_NR(VIDIOC_G_FMT)]            = "VIDIOC_G_FMT",
-	[_IOC_NR(VIDIOC_S_FMT)]            = "VIDIOC_S_FMT",
-	[_IOC_NR(VIDIOC_REQBUFS)]          = "VIDIOC_REQBUFS",
-	[_IOC_NR(VIDIOC_QUERYBUF)]         = "VIDIOC_QUERYBUF",
-	[_IOC_NR(VIDIOC_G_FBUF)]           = "VIDIOC_G_FBUF",
-	[_IOC_NR(VIDIOC_S_FBUF)]           = "VIDIOC_S_FBUF",
-	[_IOC_NR(VIDIOC_OVERLAY)]          = "VIDIOC_OVERLAY",
-	[_IOC_NR(VIDIOC_QBUF)]             = "VIDIOC_QBUF",
-	[_IOC_NR(VIDIOC_DQBUF)]            = "VIDIOC_DQBUF",
-	[_IOC_NR(VIDIOC_STREAMON)]         = "VIDIOC_STREAMON",
-	[_IOC_NR(VIDIOC_STREAMOFF)]        = "VIDIOC_STREAMOFF",
-	[_IOC_NR(VIDIOC_G_PARM)]           = "VIDIOC_G_PARM",
-	[_IOC_NR(VIDIOC_S_PARM)]           = "VIDIOC_S_PARM",
-	[_IOC_NR(VIDIOC_G_STD)]            = "VIDIOC_G_STD",
-	[_IOC_NR(VIDIOC_S_STD)]            = "VIDIOC_S_STD",
-	[_IOC_NR(VIDIOC_ENUMSTD)]          = "VIDIOC_ENUMSTD",
-	[_IOC_NR(VIDIOC_ENUMINPUT)]        = "VIDIOC_ENUMINPUT",
-	[_IOC_NR(VIDIOC_G_CTRL)]           = "VIDIOC_G_CTRL",
-	[_IOC_NR(VIDIOC_S_CTRL)]           = "VIDIOC_S_CTRL",
-	[_IOC_NR(VIDIOC_G_TUNER)]          = "VIDIOC_G_TUNER",
-	[_IOC_NR(VIDIOC_S_TUNER)]          = "VIDIOC_S_TUNER",
-	[_IOC_NR(VIDIOC_G_AUDIO)]          = "VIDIOC_G_AUDIO",
-	[_IOC_NR(VIDIOC_S_AUDIO)]          = "VIDIOC_S_AUDIO",
-	[_IOC_NR(VIDIOC_QUERYCTRL)]        = "VIDIOC_QUERYCTRL",
-	[_IOC_NR(VIDIOC_QUERYMENU)]        = "VIDIOC_QUERYMENU",
-	[_IOC_NR(VIDIOC_G_INPUT)]          = "VIDIOC_G_INPUT",
-	[_IOC_NR(VIDIOC_S_INPUT)]          = "VIDIOC_S_INPUT",
-	[_IOC_NR(VIDIOC_G_OUTPUT)]         = "VIDIOC_G_OUTPUT",
-	[_IOC_NR(VIDIOC_S_OUTPUT)]         = "VIDIOC_S_OUTPUT",
-	[_IOC_NR(VIDIOC_ENUMOUTPUT)]       = "VIDIOC_ENUMOUTPUT",
-	[_IOC_NR(VIDIOC_G_AUDOUT)]         = "VIDIOC_G_AUDOUT",
-	[_IOC_NR(VIDIOC_S_AUDOUT)]         = "VIDIOC_S_AUDOUT",
-	[_IOC_NR(VIDIOC_G_MODULATOR)]      = "VIDIOC_G_MODULATOR",
-	[_IOC_NR(VIDIOC_S_MODULATOR)]      = "VIDIOC_S_MODULATOR",
-	[_IOC_NR(VIDIOC_G_FREQUENCY)]      = "VIDIOC_G_FREQUENCY",
-	[_IOC_NR(VIDIOC_S_FREQUENCY)]      = "VIDIOC_S_FREQUENCY",
-	[_IOC_NR(VIDIOC_CROPCAP)]          = "VIDIOC_CROPCAP",
-	[_IOC_NR(VIDIOC_G_CROP)]           = "VIDIOC_G_CROP",
-	[_IOC_NR(VIDIOC_S_CROP)]           = "VIDIOC_S_CROP",
-	[_IOC_NR(VIDIOC_G_SELECTION)]      = "VIDIOC_G_SELECTION",
-	[_IOC_NR(VIDIOC_S_SELECTION)]      = "VIDIOC_S_SELECTION",
-	[_IOC_NR(VIDIOC_G_JPEGCOMP)]       = "VIDIOC_G_JPEGCOMP",
-	[_IOC_NR(VIDIOC_S_JPEGCOMP)]       = "VIDIOC_S_JPEGCOMP",
-	[_IOC_NR(VIDIOC_QUERYSTD)]         = "VIDIOC_QUERYSTD",
-	[_IOC_NR(VIDIOC_TRY_FMT)]          = "VIDIOC_TRY_FMT",
-	[_IOC_NR(VIDIOC_ENUMAUDIO)]        = "VIDIOC_ENUMAUDIO",
-	[_IOC_NR(VIDIOC_ENUMAUDOUT)]       = "VIDIOC_ENUMAUDOUT",
-	[_IOC_NR(VIDIOC_G_PRIORITY)]       = "VIDIOC_G_PRIORITY",
-	[_IOC_NR(VIDIOC_S_PRIORITY)]       = "VIDIOC_S_PRIORITY",
-	[_IOC_NR(VIDIOC_G_SLICED_VBI_CAP)] = "VIDIOC_G_SLICED_VBI_CAP",
-	[_IOC_NR(VIDIOC_LOG_STATUS)]       = "VIDIOC_LOG_STATUS",
-	[_IOC_NR(VIDIOC_G_EXT_CTRLS)]      = "VIDIOC_G_EXT_CTRLS",
-	[_IOC_NR(VIDIOC_S_EXT_CTRLS)]      = "VIDIOC_S_EXT_CTRLS",
-	[_IOC_NR(VIDIOC_TRY_EXT_CTRLS)]    = "VIDIOC_TRY_EXT_CTRLS",
-#if 1
-	[_IOC_NR(VIDIOC_ENUM_FRAMESIZES)]  = "VIDIOC_ENUM_FRAMESIZES",
-	[_IOC_NR(VIDIOC_ENUM_FRAMEINTERVALS)] = "VIDIOC_ENUM_FRAMEINTERVALS",
-	[_IOC_NR(VIDIOC_G_ENC_INDEX)] 	   = "VIDIOC_G_ENC_INDEX",
-	[_IOC_NR(VIDIOC_ENCODER_CMD)] 	   = "VIDIOC_ENCODER_CMD",
-	[_IOC_NR(VIDIOC_TRY_ENCODER_CMD)]  = "VIDIOC_TRY_ENCODER_CMD",
-
-	[_IOC_NR(VIDIOC_DECODER_CMD)]	   = "VIDIOC_DECODER_CMD",
-	[_IOC_NR(VIDIOC_TRY_DECODER_CMD)]  = "VIDIOC_TRY_DECODER_CMD",
-	[_IOC_NR(VIDIOC_DBG_S_REGISTER)]   = "VIDIOC_DBG_S_REGISTER",
-	[_IOC_NR(VIDIOC_DBG_G_REGISTER)]   = "VIDIOC_DBG_G_REGISTER",
-
-	[_IOC_NR(VIDIOC_DBG_G_CHIP_IDENT)] = "VIDIOC_DBG_G_CHIP_IDENT",
-	[_IOC_NR(VIDIOC_S_HW_FREQ_SEEK)]   = "VIDIOC_S_HW_FREQ_SEEK",
+
+struct v4l2_ioctl_info {
+	unsigned int ioctl;
+	const char * const name;
+};
+
+#define IOCTL_INFO(_ioctl) [_IOC_NR(_ioctl)] = {	\
+	.ioctl = _ioctl,				\
+	.name = #_ioctl,				\
+}
+
+static struct v4l2_ioctl_info v4l2_ioctls[] = {
+	IOCTL_INFO(VIDIOC_QUERYCAP),
+	IOCTL_INFO(VIDIOC_ENUM_FMT),
+	IOCTL_INFO(VIDIOC_G_FMT),
+	IOCTL_INFO(VIDIOC_S_FMT),
+	IOCTL_INFO(VIDIOC_REQBUFS),
+	IOCTL_INFO(VIDIOC_QUERYBUF),
+	IOCTL_INFO(VIDIOC_G_FBUF),
+	IOCTL_INFO(VIDIOC_S_FBUF),
+	IOCTL_INFO(VIDIOC_OVERLAY),
+	IOCTL_INFO(VIDIOC_QBUF),
+	IOCTL_INFO(VIDIOC_DQBUF),
+	IOCTL_INFO(VIDIOC_STREAMON),
+	IOCTL_INFO(VIDIOC_STREAMOFF),
+	IOCTL_INFO(VIDIOC_G_PARM),
+	IOCTL_INFO(VIDIOC_S_PARM),
+	IOCTL_INFO(VIDIOC_G_STD),
+	IOCTL_INFO(VIDIOC_S_STD),
+	IOCTL_INFO(VIDIOC_ENUMSTD),
+	IOCTL_INFO(VIDIOC_ENUMINPUT),
+	IOCTL_INFO(VIDIOC_G_CTRL),
+	IOCTL_INFO(VIDIOC_S_CTRL),
+	IOCTL_INFO(VIDIOC_G_TUNER),
+	IOCTL_INFO(VIDIOC_S_TUNER),
+	IOCTL_INFO(VIDIOC_G_AUDIO),
+	IOCTL_INFO(VIDIOC_S_AUDIO),
+	IOCTL_INFO(VIDIOC_QUERYCTRL),
+	IOCTL_INFO(VIDIOC_QUERYMENU),
+	IOCTL_INFO(VIDIOC_G_INPUT),
+	IOCTL_INFO(VIDIOC_S_INPUT),
+	IOCTL_INFO(VIDIOC_G_OUTPUT),
+	IOCTL_INFO(VIDIOC_S_OUTPUT),
+	IOCTL_INFO(VIDIOC_ENUMOUTPUT),
+	IOCTL_INFO(VIDIOC_G_AUDOUT),
+	IOCTL_INFO(VIDIOC_S_AUDOUT),
+	IOCTL_INFO(VIDIOC_G_MODULATOR),
+	IOCTL_INFO(VIDIOC_S_MODULATOR),
+	IOCTL_INFO(VIDIOC_G_FREQUENCY),
+	IOCTL_INFO(VIDIOC_S_FREQUENCY),
+	IOCTL_INFO(VIDIOC_CROPCAP),
+	IOCTL_INFO(VIDIOC_G_CROP),
+	IOCTL_INFO(VIDIOC_S_CROP),
+	IOCTL_INFO(VIDIOC_G_SELECTION),
+	IOCTL_INFO(VIDIOC_S_SELECTION),
+	IOCTL_INFO(VIDIOC_G_JPEGCOMP),
+	IOCTL_INFO(VIDIOC_S_JPEGCOMP),
+	IOCTL_INFO(VIDIOC_QUERYSTD),
+	IOCTL_INFO(VIDIOC_TRY_FMT),
+	IOCTL_INFO(VIDIOC_ENUMAUDIO),
+	IOCTL_INFO(VIDIOC_ENUMAUDOUT),
+	IOCTL_INFO(VIDIOC_G_PRIORITY),
+	IOCTL_INFO(VIDIOC_S_PRIORITY),
+	IOCTL_INFO(VIDIOC_G_SLICED_VBI_CAP),
+	IOCTL_INFO(VIDIOC_LOG_STATUS),
+	IOCTL_INFO(VIDIOC_G_EXT_CTRLS),
+	IOCTL_INFO(VIDIOC_S_EXT_CTRLS),
+	IOCTL_INFO(VIDIOC_TRY_EXT_CTRLS),
+	IOCTL_INFO(VIDIOC_ENUM_FRAMESIZES),
+	IOCTL_INFO(VIDIOC_ENUM_FRAMEINTERVALS),
+	IOCTL_INFO(VIDIOC_G_ENC_INDEX),
+	IOCTL_INFO(VIDIOC_ENCODER_CMD),
+	IOCTL_INFO(VIDIOC_TRY_ENCODER_CMD),
+	IOCTL_INFO(VIDIOC_DECODER_CMD),
+	IOCTL_INFO(VIDIOC_TRY_DECODER_CMD),
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+	IOCTL_INFO(VIDIOC_DBG_S_REGISTER),
+	IOCTL_INFO(VIDIOC_DBG_G_REGISTER),
 #endif
-	[_IOC_NR(VIDIOC_ENUM_DV_PRESETS)]  = "VIDIOC_ENUM_DV_PRESETS",
-	[_IOC_NR(VIDIOC_S_DV_PRESET)]	   = "VIDIOC_S_DV_PRESET",
-	[_IOC_NR(VIDIOC_G_DV_PRESET)]	   = "VIDIOC_G_DV_PRESET",
-	[_IOC_NR(VIDIOC_QUERY_DV_PRESET)]  = "VIDIOC_QUERY_DV_PRESET",
-	[_IOC_NR(VIDIOC_S_DV_TIMINGS)]     = "VIDIOC_S_DV_TIMINGS",
-	[_IOC_NR(VIDIOC_G_DV_TIMINGS)]     = "VIDIOC_G_DV_TIMINGS",
-	[_IOC_NR(VIDIOC_DQEVENT)]	   = "VIDIOC_DQEVENT",
-	[_IOC_NR(VIDIOC_SUBSCRIBE_EVENT)]  = "VIDIOC_SUBSCRIBE_EVENT",
-	[_IOC_NR(VIDIOC_UNSUBSCRIBE_EVENT)] = "VIDIOC_UNSUBSCRIBE_EVENT",
-	[_IOC_NR(VIDIOC_CREATE_BUFS)]      = "VIDIOC_CREATE_BUFS",
-	[_IOC_NR(VIDIOC_PREPARE_BUF)]      = "VIDIOC_PREPARE_BUF",
+	IOCTL_INFO(VIDIOC_DBG_G_CHIP_IDENT),
+	IOCTL_INFO(VIDIOC_S_HW_FREQ_SEEK),
+	IOCTL_INFO(VIDIOC_ENUM_DV_PRESETS),
+	IOCTL_INFO(VIDIOC_S_DV_PRESET),
+	IOCTL_INFO(VIDIOC_G_DV_PRESET),
+	IOCTL_INFO(VIDIOC_QUERY_DV_PRESET),
+	IOCTL_INFO(VIDIOC_S_DV_TIMINGS),
+	IOCTL_INFO(VIDIOC_G_DV_TIMINGS),
+	IOCTL_INFO(VIDIOC_DQEVENT),
+	IOCTL_INFO(VIDIOC_SUBSCRIBE_EVENT),
+	IOCTL_INFO(VIDIOC_UNSUBSCRIBE_EVENT),
+	IOCTL_INFO(VIDIOC_CREATE_BUFS),
+	IOCTL_INFO(VIDIOC_PREPARE_BUF),
 };
 #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
 
+bool v4l2_is_valid_ioctl(unsigned int cmd)
+{
+	if (_IOC_NR(cmd) >= V4L2_IOCTLS)
+		return false;
+	return v4l2_ioctls[_IOC_NR(cmd)].ioctl == cmd;
+}
+
 /* Common ioctl debug function. This function can be used by
    external ioctl messages as well as internal V4L ioctl */
 void v4l_printk_ioctl(unsigned int cmd)
@@ -297,7 +312,7 @@ void v4l_printk_ioctl(unsigned int cmd)
 			type = "v4l2";
 			break;
 		}
-		printk("%s", v4l2_ioctls[_IOC_NR(cmd)]);
+		printk("%s", v4l2_ioctls[_IOC_NR(cmd)].name);
 		return;
 	default:
 		type = "unknown";
diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
index 96d2221..0da84dc 100644
--- a/include/media/v4l2-dev.h
+++ b/include/media/v4l2-dev.h
@@ -128,6 +128,7 @@ struct video_device
 	const struct v4l2_ioctl_ops *ioctl_ops;
 
 	/* serialization lock */
+	DECLARE_BITMAP(dont_use_lock, BASE_VIDIOC_PRIVATE);
 	struct mutex *lock;
 };
 
@@ -173,6 +174,16 @@ void video_device_release(struct video_device *vdev);
    a dubious construction at best. */
 void video_device_release_empty(struct video_device *vdev);
 
+/* returns true if cmd is a valid V4L2 ioctl */
+bool v4l2_is_valid_ioctl(unsigned int cmd);
+
+/* mark that this command shouldn't use core locking */
+static inline void v4l2_dont_use_lock(struct video_device *vdev, unsigned int cmd)
+{
+	if (_IOC_NR(cmd) < BASE_VIDIOC_PRIVATE)
+		set_bit(_IOC_NR(cmd), vdev->dont_use_lock);
+}
+
 /* helper functions to access driver private data. */
 static inline void *video_get_drvdata(struct video_device *vdev)
 {
-- 
1.7.10

[RFCv2 PATCH 02/17] v4l2-framework.txt: add paragraph on driver locking and the control framework.

[Hans Verkuil]

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

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 Documentation/video4linux/v4l2-framework.txt |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/video4linux/v4l2-framework.txt b/Documentation/video4linux/v4l2-framework.txt
index 4a313d8..2d7dd86 100644
--- a/Documentation/video4linux/v4l2-framework.txt
+++ b/Documentation/video4linux/v4l2-framework.txt
@@ -658,6 +658,14 @@ wait_prepare and wait_finish.
 The implementation of a hotplug disconnect should also take the lock before
 calling v4l2_device_disconnect.
 
+Note that if you do your own locking and want to use the control framework,
+then you have to implement the control callbacks yourself so you can take
+your own lock before calling into the control framework. Otherwise your lock
+won't be held when the v4l2_ctrl_ops are called. You can't take your lock
+there because a driver can also call e.g. v4l2_ctrl_s_ctrl with your lock
+already held, which in turn will call the s_ctrl op, which will attempt to
+take your lock again: deadlock!
+
 video_device registration
 -------------------------
 
-- 
1.7.10

[RFCv2 PATCH 03/17] 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 comes later.

It add a init_control cam_op that is called after init in probe that allows
the subdriver to set up the controls.

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

diff --git a/drivers/media/video/gspca/gspca.c b/drivers/media/video/gspca/gspca.c
index ca5a2b1..7668e24 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,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);
@@ -2357,21 +2369,17 @@ int gspca_dev_probe2(struct usb_interface *intf,
 	ret = sd_desc->init(gspca_dev);
 	if (ret < 0)
 		goto out;
+	if (sd_desc->init_controls)
+		ret = sd_desc->init_controls(gspca_dev);
+	if (ret < 0)
+		goto out;
 	gspca_set_default_mode(gspca_dev);
 
 	ret = gspca_input_connect(gspca_dev);
 	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 +2399,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;
@@ -2492,6 +2501,7 @@ int gspca_resume(struct usb_interface *intf)
 
 	gspca_dev->frozen = 0;
 	gspca_dev->sd_desc->init(gspca_dev);
+	gspca_set_default_mode(gspca_dev);
 	gspca_input_create_urb(gspca_dev);
 	if (gspca_dev->streaming)
 		return gspca_init_transfer(gspca_dev);
diff --git a/drivers/media/video/gspca/gspca.h b/drivers/media/video/gspca/gspca.h
index 589009f..8140416 100644
--- a/drivers/media/video/gspca/gspca.h
+++ b/drivers/media/video/gspca/gspca.h
@@ -115,6 +115,7 @@ struct sd_desc {
 /* mandatory operations */
 	cam_cf_op config;	/* called on probe */
 	cam_op init;		/* called on probe and resume */
+	cam_op init_controls;	/* called on probe */
 	cam_op start;		/* called on stream on after URBs creation */
 	cam_pkt_op pkt_scan;
 /* optional operations */
-- 
1.7.10

[RFCv2 PATCH 04/17] 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 7668e24..271e339 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(file);
 
 	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

[RFCv2 PATCH 05/17] 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 271e339..1d76504 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;
 
@@ -2462,6 +2468,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 8140416..c1ebf7c 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 */
@@ -159,6 +160,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

[RFCv2 PATCH 06/17] 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 1d76504..b2ddfc6 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

[RFCv2 PATCH 07/17] 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 b2ddfc6..730d8eb 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

[RFCv2 PATCH 08/17] gspca: fix locking issues related to suspend/resume.

[Hans Verkuil]

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

There are two bugs here: first the calls to stop0 (in gspca_suspend) and
gspca_init_transfer (in gspca_resume) need to be called with the usb_lock held.
That's true for the other places they are called and it is what subdrivers
expect. Quite a few will unlock the usb_lock in stop0 while waiting for a
worker thread to finish, and if usb_lock isn't held then that can cause a
kernel oops.

The other problem is that a worker thread needs to detect that it has to
halt due to a suspend. Otherwise it will just go on looping. So add tests
against gspca_dev->frozen in the worker threads that need it.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/video/gspca/finepix.c   |    2 +-
 drivers/media/video/gspca/gspca.c     |   15 +++++++++++----
 drivers/media/video/gspca/jl2005bcd.c |    2 +-
 drivers/media/video/gspca/sq905.c     |    2 +-
 drivers/media/video/gspca/sq905c.c    |    2 +-
 drivers/media/video/gspca/vicam.c     |    2 +-
 6 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/media/video/gspca/finepix.c b/drivers/media/video/gspca/finepix.c
index 0107513..1d11976 100644
--- a/drivers/media/video/gspca/finepix.c
+++ b/drivers/media/video/gspca/finepix.c
@@ -102,7 +102,7 @@ again:
 		mutex_unlock(&gspca_dev->usb_lock);
 		if (ret < 0)
 			break;
-		if (!gspca_dev->present || !gspca_dev->streaming)
+		if (gspca_dev->frozen || !gspca_dev->present || !gspca_dev->streaming)
 			break;
 
 		/* the frame comes in parts */
diff --git a/drivers/media/video/gspca/gspca.c b/drivers/media/video/gspca/gspca.c
index 730d8eb..f840bed 100644
--- a/drivers/media/video/gspca/gspca.c
+++ b/drivers/media/video/gspca/gspca.c
@@ -2499,8 +2499,11 @@ int gspca_suspend(struct usb_interface *intf, pm_message_t message)
 	destroy_urbs(gspca_dev);
 	gspca_input_destroy_urb(gspca_dev);
 	gspca_set_alt0(gspca_dev);
-	if (gspca_dev->sd_desc->stop0)
+	if (gspca_dev->sd_desc->stop0) {
+		mutex_lock(&gspca_dev->usb_lock);
 		gspca_dev->sd_desc->stop0(gspca_dev);
+		mutex_unlock(&gspca_dev->usb_lock);
+	}
 	return 0;
 }
 EXPORT_SYMBOL(gspca_suspend);
@@ -2508,14 +2511,18 @@ EXPORT_SYMBOL(gspca_suspend);
 int gspca_resume(struct usb_interface *intf)
 {
 	struct gspca_dev *gspca_dev = usb_get_intfdata(intf);
+	int ret = 0;
 
 	gspca_dev->frozen = 0;
 	gspca_dev->sd_desc->init(gspca_dev);
 	gspca_set_default_mode(gspca_dev);
 	gspca_input_create_urb(gspca_dev);
-	if (gspca_dev->streaming)
-		return gspca_init_transfer(gspca_dev);
-	return 0;
+	if (gspca_dev->streaming) {
+		mutex_lock(&gspca_dev->queue_lock);
+		ret = gspca_init_transfer(gspca_dev);
+		mutex_unlock(&gspca_dev->queue_lock);
+	}
+	return ret;
 }
 EXPORT_SYMBOL(gspca_resume);
 #endif
diff --git a/drivers/media/video/gspca/jl2005bcd.c b/drivers/media/video/gspca/jl2005bcd.c
index 53f58ef..f5b88e9 100644
--- a/drivers/media/video/gspca/jl2005bcd.c
+++ b/drivers/media/video/gspca/jl2005bcd.c
@@ -335,7 +335,7 @@ static void jl2005c_dostream(struct work_struct *work)
 		goto quit_stream;
 	}
 
-	while (gspca_dev->present && gspca_dev->streaming) {
+	while (!gspca_dev->frozen && gspca_dev->present && gspca_dev->streaming) {
 		/* Check if this is a new frame. If so, start the frame first */
 		if (!header_read) {
 			mutex_lock(&gspca_dev->usb_lock);
diff --git a/drivers/media/video/gspca/sq905.c b/drivers/media/video/gspca/sq905.c
index 2fe3c29..7b72a20 100644
--- a/drivers/media/video/gspca/sq905.c
+++ b/drivers/media/video/gspca/sq905.c
@@ -232,7 +232,7 @@ static void sq905_dostream(struct work_struct *work)
 	frame_sz = gspca_dev->cam.cam_mode[gspca_dev->curr_mode].sizeimage
 			+ FRAME_HEADER_LEN;
 
-	while (gspca_dev->present && gspca_dev->streaming) {
+	while (!gspca_dev->frozen && gspca_dev->present && gspca_dev->streaming) {
 		/* request some data and then read it until we have
 		 * a complete frame. */
 		bytes_left = frame_sz;
diff --git a/drivers/media/video/gspca/sq905c.c b/drivers/media/video/gspca/sq905c.c
index ae78363..52e42ca 100644
--- a/drivers/media/video/gspca/sq905c.c
+++ b/drivers/media/video/gspca/sq905c.c
@@ -150,7 +150,7 @@ static void sq905c_dostream(struct work_struct *work)
 		goto quit_stream;
 	}
 
-	while (gspca_dev->present && gspca_dev->streaming) {
+	while (!gspca_dev->frozen && gspca_dev->present && gspca_dev->streaming) {
 		/* Request the header, which tells the size to download */
 		ret = usb_bulk_msg(gspca_dev->dev,
 				usb_rcvbulkpipe(gspca_dev->dev, 0x81),
diff --git a/drivers/media/video/gspca/vicam.c b/drivers/media/video/gspca/vicam.c
index e48ec4d..0d532ec 100644
--- a/drivers/media/video/gspca/vicam.c
+++ b/drivers/media/video/gspca/vicam.c
@@ -225,7 +225,7 @@ static void vicam_dostream(struct work_struct *work)
 		goto exit;
 	}
 
-	while (gspca_dev->present && gspca_dev->streaming) {
+	while (!gspca_dev->frozen && gspca_dev->present && gspca_dev->streaming) {
 		ret = vicam_read_frame(gspca_dev, buffer, frame_sz);
 		if (ret < 0)
 			break;
-- 
1.7.10

[RFCv2 PATCH 09/17] gspca_zc3xx: Fix setting of jpeg quality while streaming

[Hans Verkuil]

From: Hans de Goede <hdegoede@redhat.com>

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>
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.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

[RFCv2 PATCH 10/17] gspca_zc3xx: Fix JPEG quality setting code

[Hans Verkuil]

From: Hans de Goede <hdegoede@redhat.com>

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.

Last 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>
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.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

[RFCv2 PATCH 11/17] gscpa_zc3xx: Always automatically adjust BRC as needed

[Hans Verkuil]

From: Hans de Goede <hdegoede@redhat.com>

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>
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.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

[RFCv2 PATCH 12/17] gscpa_zc3xx: Disable the highest quality setting as it is not usable

[Hans Verkuil]

From: Hans de Goede <hdegoede@redhat.com>

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>
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.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

[RFCv2 PATCH 13/17] gspca: switch to V4L2 core locking, except for the buffer queuing ioctls.

[Hans Verkuil]

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

Due to latency concerns the VIDIOC_QBUF, DQBUF and QUERYBUF do not use the
core lock, instead they rely only on queue_lock.

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

diff --git a/drivers/media/video/gspca/gspca.c b/drivers/media/video/gspca/gspca.c
index f840bed..edca4f3 100644
--- a/drivers/media/video/gspca/gspca.c
+++ b/drivers/media/video/gspca/gspca.c
@@ -850,14 +850,6 @@ static int gspca_init_transfer(struct gspca_dev *gspca_dev)
 	struct ep_tb_s ep_tb[MAX_ALT];
 	int n, ret, xfer, alt, alt_idx;
 
-	if (mutex_lock_interruptible(&gspca_dev->usb_lock))
-		return -ERESTARTSYS;
-
-	if (!gspca_dev->present) {
-		ret = -ENODEV;
-		goto unlock;
-	}
-
 	/* reset the streaming variables */
 	gspca_dev->image = NULL;
 	gspca_dev->image_len = 0;
@@ -872,7 +864,7 @@ static int gspca_init_transfer(struct gspca_dev *gspca_dev)
 	if (gspca_dev->sd_desc->isoc_init) {
 		ret = gspca_dev->sd_desc->isoc_init(gspca_dev);
 		if (ret < 0)
-			goto unlock;
+			return ret;
 	}
 	xfer = gspca_dev->cam.bulk ? USB_ENDPOINT_XFER_BULK
 				   : USB_ENDPOINT_XFER_ISOC;
@@ -883,8 +875,7 @@ static int gspca_init_transfer(struct gspca_dev *gspca_dev)
 		ep = alt_xfer(&intf->altsetting[gspca_dev->alt], xfer);
 		if (ep == NULL) {
 			pr_err("bad altsetting %d\n", gspca_dev->alt);
-			ret = -EIO;
-			goto out;
+			return -EIO;
 		}
 		ep_tb[0].alt = gspca_dev->alt;
 		alt_idx = 1;
@@ -895,8 +886,7 @@ static int gspca_init_transfer(struct gspca_dev *gspca_dev)
 		alt_idx = build_isoc_ep_tb(gspca_dev, intf, ep_tb);
 		if (alt_idx <= 0) {
 			pr_err("no transfer endpoint found\n");
-			ret = -EIO;
-			goto unlock;
+			return -EIO;
 		}
 	}
 
@@ -991,8 +981,6 @@ retry:
 	}
 out:
 	gspca_input_create_urb(gspca_dev);
-unlock:
-	mutex_unlock(&gspca_dev->usb_lock);
 	return ret;
 }
 
@@ -1062,7 +1050,6 @@ static int gspca_get_mode(struct gspca_dev *gspca_dev,
 static int vidioc_g_register(struct file *file, void *priv,
 			struct v4l2_dbg_register *reg)
 {
-	int ret;
 	struct gspca_dev *gspca_dev = video_drvdata(file);
 
 	if (!gspca_dev->sd_desc->get_chip_ident)
@@ -1071,22 +1058,13 @@ static int vidioc_g_register(struct file *file, void *priv,
 	if (!gspca_dev->sd_desc->get_register)
 		return -ENOTTY;
 
-	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_register(gspca_dev, reg);
-	else
-		ret = -ENODEV;
-	mutex_unlock(&gspca_dev->usb_lock);
-
-	return ret;
+	return gspca_dev->sd_desc->get_register(gspca_dev, reg);
 }
 
 static int vidioc_s_register(struct file *file, void *priv,
 			struct v4l2_dbg_register *reg)
 {
-	int ret;
 	struct gspca_dev *gspca_dev = video_drvdata(file);
 
 	if (!gspca_dev->sd_desc->get_chip_ident)
@@ -1095,38 +1073,21 @@ static int vidioc_s_register(struct file *file, void *priv,
 	if (!gspca_dev->sd_desc->set_register)
 		return -ENOTTY;
 
-	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_register(gspca_dev, reg);
-	else
-		ret = -ENODEV;
-	mutex_unlock(&gspca_dev->usb_lock);
-
-	return ret;
+	return gspca_dev->sd_desc->set_register(gspca_dev, reg);
 }
 #endif
 
 static int vidioc_g_chip_ident(struct file *file, void *priv,
 			struct v4l2_dbg_chip_ident *chip)
 {
-	int ret;
 	struct gspca_dev *gspca_dev = video_drvdata(file);
 
 	if (!gspca_dev->sd_desc->get_chip_ident)
 		return -ENOTTY;
 
-	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_chip_ident(gspca_dev, chip);
-	else
-		ret = -ENODEV;
-	mutex_unlock(&gspca_dev->usb_lock);
-
-	return ret;
+	return gspca_dev->sd_desc->get_chip_ident(gspca_dev, chip);
 }
 
 static int vidioc_enum_fmt_vid_cap(struct file *file, void  *priv,
@@ -1339,8 +1300,6 @@ static int dev_open(struct file *file)
 	struct gspca_dev *gspca_dev = video_drvdata(file);
 
 	PDEBUG(D_STREAM, "[%s] open", current->comm);
-	if (!gspca_dev->present)
-		return -ENODEV;
 
 	/* protect the subdriver against rmmod */
 	if (!try_module_get(gspca_dev->module))
@@ -1369,10 +1328,8 @@ static int dev_close(struct file *file)
 	/* if the file did the capture, free the streaming resources */
 	if (gspca_dev->capt_file == file) {
 		if (gspca_dev->streaming) {
-			mutex_lock(&gspca_dev->usb_lock);
 			gspca_dev->usb_err = 0;
 			gspca_stream_off(gspca_dev);
-			mutex_unlock(&gspca_dev->usb_lock);
 		}
 		frame_free(gspca_dev);
 	}
@@ -1388,15 +1345,7 @@ static int vidioc_querycap(struct file *file, void  *priv,
 			   struct v4l2_capability *cap)
 {
 	struct gspca_dev *gspca_dev = video_drvdata(file);
-	int ret;
 
-	/* protect the access to the usb device */
-	if (mutex_lock_interruptible(&gspca_dev->usb_lock))
-		return -ERESTARTSYS;
-	if (!gspca_dev->present) {
-		ret = -ENODEV;
-		goto out;
-	}
 	strlcpy((char *) cap->driver, gspca_dev->sd_desc->name,
 			sizeof cap->driver);
 	if (gspca_dev->dev->product != NULL) {
@@ -1414,10 +1363,7 @@ static int vidioc_querycap(struct file *file, void  *priv,
 			  | V4L2_CAP_STREAMING
 			  | V4L2_CAP_READWRITE;
 	cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
-	ret = 0;
-out:
-	mutex_unlock(&gspca_dev->usb_lock);
-	return ret;
+	return 0;
 }
 
 static int get_ctrl(struct gspca_dev *gspca_dev,
@@ -1486,7 +1432,7 @@ static int vidioc_s_ctrl(struct file *file, void *priv,
 	struct gspca_dev *gspca_dev = video_drvdata(file);
 	const struct ctrl *ctrls;
 	struct gspca_ctrl *gspca_ctrl;
-	int idx, ret;
+	int idx;
 
 	idx = get_ctrl(gspca_dev, ctrl->id);
 	if (idx < 0)
@@ -1506,27 +1452,16 @@ static int vidioc_s_ctrl(struct file *file, void *priv,
 			return -ERANGE;
 	}
 	PDEBUG(D_CONF, "set ctrl [%08x] = %d", ctrl->id, ctrl->value);
-	if (mutex_lock_interruptible(&gspca_dev->usb_lock))
-		return -ERESTARTSYS;
-	if (!gspca_dev->present) {
-		ret = -ENODEV;
-		goto out;
-	}
 	gspca_dev->usb_err = 0;
-	if (ctrls->set != NULL) {
-		ret = ctrls->set(gspca_dev, ctrl->value);
-		goto out;
-	}
+	if (ctrls->set != NULL)
+		return ctrls->set(gspca_dev, ctrl->value);
 	if (gspca_ctrl != NULL) {
 		gspca_ctrl->val = ctrl->value;
 		if (ctrls->set_control != NULL
 		 && gspca_dev->streaming)
 			ctrls->set_control(gspca_dev);
 	}
-	ret = gspca_dev->usb_err;
-out:
-	mutex_unlock(&gspca_dev->usb_lock);
-	return ret;
+	return gspca_dev->usb_err;
 }
 
 static int vidioc_g_ctrl(struct file *file, void *priv,
@@ -1534,30 +1469,19 @@ static int vidioc_g_ctrl(struct file *file, void *priv,
 {
 	struct gspca_dev *gspca_dev = video_drvdata(file);
 	const struct ctrl *ctrls;
-	int idx, ret;
+	int idx;
 
 	idx = get_ctrl(gspca_dev, ctrl->id);
 	if (idx < 0)
 		return -EINVAL;
 	ctrls = &gspca_dev->sd_desc->ctrls[idx];
 
-	if (mutex_lock_interruptible(&gspca_dev->usb_lock))
-		return -ERESTARTSYS;
-	if (!gspca_dev->present) {
-		ret = -ENODEV;
-		goto out;
-	}
 	gspca_dev->usb_err = 0;
-	if (ctrls->get != NULL) {
-		ret = ctrls->get(gspca_dev, &ctrl->value);
-		goto out;
-	}
+	if (ctrls->get != NULL)
+		return ctrls->get(gspca_dev, &ctrl->value);
 	if (gspca_dev->cam.ctrls != NULL)
 		ctrl->value = gspca_dev->cam.ctrls[idx].val;
-	ret = 0;
-out:
-	mutex_unlock(&gspca_dev->usb_lock);
-	return ret;
+	return 0;
 }
 
 static int vidioc_querymenu(struct file *file, void *priv,
@@ -1640,10 +1564,8 @@ static int vidioc_reqbufs(struct file *file, void *priv,
 	/* stop streaming */
 	streaming = gspca_dev->streaming;
 	if (streaming) {
-		mutex_lock(&gspca_dev->usb_lock);
 		gspca_dev->usb_err = 0;
 		gspca_stream_off(gspca_dev);
-		mutex_unlock(&gspca_dev->usb_lock);
 
 		/* Don't restart the stream when switching from read
 		 * to mmap mode */
@@ -1736,10 +1658,8 @@ static int vidioc_streamoff(struct file *file, void *priv,
 	if (mutex_lock_interruptible(&gspca_dev->queue_lock))
 		return -ERESTARTSYS;
 
-	if (!gspca_dev->streaming) {
-		ret = 0;
-		goto out;
-	}
+	if (!gspca_dev->streaming)
+		return 0;
 
 	/* check the capture file */
 	if (gspca_dev->capt_file != file) {
@@ -1748,13 +1668,8 @@ static int vidioc_streamoff(struct file *file, void *priv,
 	}
 
 	/* stop streaming */
-	if (mutex_lock_interruptible(&gspca_dev->usb_lock)) {
-		ret = -ERESTARTSYS;
-		goto out;
-	}
 	gspca_dev->usb_err = 0;
 	gspca_stream_off(gspca_dev);
-	mutex_unlock(&gspca_dev->usb_lock);
 	/* In case another thread is waiting in dqbuf */
 	wake_up_interruptible(&gspca_dev->wq);
 
@@ -1772,63 +1687,36 @@ static int vidioc_g_jpegcomp(struct file *file, void *priv,
 			struct v4l2_jpegcompression *jpegcomp)
 {
 	struct gspca_dev *gspca_dev = video_drvdata(file);
-	int ret;
 
 	if (!gspca_dev->sd_desc->get_jcomp)
 		return -ENOTTY;
-	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);
-	return ret;
+	return gspca_dev->sd_desc->get_jcomp(gspca_dev, jpegcomp);
 }
 
 static int vidioc_s_jpegcomp(struct file *file, void *priv,
 			struct v4l2_jpegcompression *jpegcomp)
 {
 	struct gspca_dev *gspca_dev = video_drvdata(file);
-	int ret;
 
 	if (!gspca_dev->sd_desc->set_jcomp)
 		return -ENOTTY;
-	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);
-	return ret;
+	return gspca_dev->sd_desc->set_jcomp(gspca_dev, jpegcomp);
 }
 
 static int vidioc_g_parm(struct file *filp, void *priv,
 			struct v4l2_streamparm *parm)
 {
-	struct gspca_dev *gspca_dev = video_drvdata(file);
+	struct gspca_dev *gspca_dev = video_drvdata(filp);
 
 	parm->parm.capture.readbuffers = gspca_dev->nbufread;
 
 	if (gspca_dev->sd_desc->get_streamparm) {
-		int ret;
-
-		if (mutex_lock_interruptible(&gspca_dev->usb_lock))
-			return -ERESTARTSYS;
-		if (gspca_dev->present) {
-			gspca_dev->usb_err = 0;
-			gspca_dev->sd_desc->get_streamparm(gspca_dev, parm);
-			ret = gspca_dev->usb_err;
-		} else {
-			ret = -ENODEV;
-		}
-		mutex_unlock(&gspca_dev->usb_lock);
-		return ret;
+		gspca_dev->usb_err = 0;
+		gspca_dev->sd_desc->get_streamparm(gspca_dev, parm);
+		return gspca_dev->usb_err;
 	}
-
 	return 0;
 }
 
@@ -1845,19 +1733,9 @@ static int vidioc_s_parm(struct file *filp, void *priv,
 		gspca_dev->nbufread = n;
 
 	if (gspca_dev->sd_desc->set_streamparm) {
-		int ret;
-
-		if (mutex_lock_interruptible(&gspca_dev->usb_lock))
-			return -ERESTARTSYS;
-		if (gspca_dev->present) {
-			gspca_dev->usb_err = 0;
-			gspca_dev->sd_desc->set_streamparm(gspca_dev, parm);
-			ret = gspca_dev->usb_err;
-		} else {
-			ret = -ENODEV;
-		}
-		mutex_unlock(&gspca_dev->usb_lock);
-		return ret;
+		gspca_dev->usb_err = 0;
+		gspca_dev->sd_desc->set_streamparm(gspca_dev, parm);
+		return gspca_dev->usb_err;
 	}
 
 	return 0;
@@ -1877,10 +1755,6 @@ static int dev_mmap(struct file *file, struct vm_area_struct *vma)
 
 	if (mutex_lock_interruptible(&gspca_dev->queue_lock))
 		return -ERESTARTSYS;
-	if (!gspca_dev->present) {
-		ret = -ENODEV;
-		goto out;
-	}
 	if (gspca_dev->capt_file != file) {
 		ret = -EINVAL;
 		goto out;
@@ -2009,11 +1883,9 @@ static int vidioc_dqbuf(struct file *file, void *priv,
 	gspca_dev->fr_o = (i + 1) % GSPCA_MAX_FRAMES;
 
 	if (gspca_dev->sd_desc->dq_callback) {
-		mutex_lock(&gspca_dev->usb_lock);
 		gspca_dev->usb_err = 0;
 		if (gspca_dev->present)
 			gspca_dev->sd_desc->dq_callback(gspca_dev);
-		mutex_unlock(&gspca_dev->usb_lock);
 	}
 
 	frame->v4l2_buf.flags &= ~V4L2_BUF_FLAG_DONE;
@@ -2366,6 +2238,7 @@ int gspca_dev_probe2(struct usb_interface *intf,
 	gspca_dev->present = 1;
 
 	mutex_init(&gspca_dev->usb_lock);
+	gspca_dev->vdev.lock = &gspca_dev->usb_lock;
 	mutex_init(&gspca_dev->queue_lock);
 	init_waitqueue_head(&gspca_dev->wq);
 
@@ -2388,6 +2261,14 @@ int gspca_dev_probe2(struct usb_interface *intf,
 	if (ret)
 		goto out;
 
+	/* These ioctls use just queue_lock and not usb_lock.
+	   This improves latency if the usb lock is taken for a
+	   long time, e.g. when changing a control value, and a
+	   new frame is ready to be dequeued. */
+	v4l2_dont_use_lock(&gspca_dev->vdev, VIDIOC_DQBUF);
+	v4l2_dont_use_lock(&gspca_dev->vdev, VIDIOC_QBUF);
+	v4l2_dont_use_lock(&gspca_dev->vdev, VIDIOC_QUERYBUF);
+
 	/* init video stuff */
 	ret = video_register_device(&gspca_dev->vdev,
 				  VFL_TYPE_GRABBER,
@@ -2493,17 +2374,16 @@ int gspca_suspend(struct usb_interface *intf, pm_message_t message)
 
 	if (!gspca_dev->streaming)
 		return 0;
+	mutex_lock(&gspca_dev->usb_lock);
 	gspca_dev->frozen = 1;		/* avoid urb error messages */
 	if (gspca_dev->sd_desc->stopN)
 		gspca_dev->sd_desc->stopN(gspca_dev);
 	destroy_urbs(gspca_dev);
 	gspca_input_destroy_urb(gspca_dev);
 	gspca_set_alt0(gspca_dev);
-	if (gspca_dev->sd_desc->stop0) {
-		mutex_lock(&gspca_dev->usb_lock);
+	if (gspca_dev->sd_desc->stop0)
 		gspca_dev->sd_desc->stop0(gspca_dev);
-		mutex_unlock(&gspca_dev->usb_lock);
-	}
+	mutex_unlock(&gspca_dev->usb_lock);
 	return 0;
 }
 EXPORT_SYMBOL(gspca_suspend);
@@ -2513,15 +2393,14 @@ int gspca_resume(struct usb_interface *intf)
 	struct gspca_dev *gspca_dev = usb_get_intfdata(intf);
 	int ret = 0;
 
+	mutex_lock(&gspca_dev->usb_lock);
 	gspca_dev->frozen = 0;
 	gspca_dev->sd_desc->init(gspca_dev);
 	gspca_set_default_mode(gspca_dev);
 	gspca_input_create_urb(gspca_dev);
-	if (gspca_dev->streaming) {
-		mutex_lock(&gspca_dev->queue_lock);
+	if (gspca_dev->streaming)
 		ret = gspca_init_transfer(gspca_dev);
-		mutex_unlock(&gspca_dev->queue_lock);
-	}
+	mutex_unlock(&gspca_dev->usb_lock);
 	return ret;
 }
 EXPORT_SYMBOL(gspca_resume);
-- 
1.7.10

[RFCv2 PATCH 14/17] gspca-zc3xx: convert to the control framework.

[Hans Verkuil]

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

The initial version was done by HV, corrections were made by HdG, and some
final small changes again by HV.

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

diff --git a/drivers/media/video/gspca/zc3xx.c b/drivers/media/video/gspca/zc3xx.c
index a8282b8..86d6d66 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);
 }
 
 /*
@@ -6078,7 +5948,7 @@ static void transfer_update(struct work_struct *work)
 		msleep(100);
 
 		mutex_lock(&gspca_dev->usb_lock);
-		if (!gspca_dev->present || !gspca_dev->streaming)
+		if (gspca_dev->frozen || !gspca_dev->present || !gspca_dev->streaming)
 			goto err;
 
 		/* Bit 0 of register 11 indicates FIFO overflow */
@@ -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,12 +6326,85 @@ static int sd_config(struct gspca_dev *gspca_dev,
 	return 0;
 }
 
-/* this function is called at probe and resume time */
-static int sd_init(struct gspca_dev *gspca_dev)
+static int zcxx_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
 {
-	struct sd *sd = (struct sd *) gspca_dev;
-	struct cam *cam;
-	int sensor;
+	struct sd *sd = container_of(ctrl->handler, struct sd, ctrl_handler);
+	struct gspca_dev *gspca_dev = &sd->gspca_dev;
+
+	switch (ctrl->id) {
+	case V4L2_CID_AUTOGAIN:
+		gspca_dev->usb_err = 0;
+		if (ctrl->val && sd->exposure && gspca_dev->streaming)
+			sd->exposure->val = getexposure(gspca_dev);
+		return gspca_dev->usb_err;
+	}
+	return -EINVAL;
+}
+
+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;
+
+	gspca_dev->usb_err = 0;
+
+	if (ctrl->id == 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)
+			return -EBUSY;
+
+		sd->reg08 = (i << 1) | 1;
+		ctrl->val = jpeg_qual[i];
+	}
+
+	if (!gspca_dev->streaming)
+		return 0;
+
+	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:
+		setquality(gspca_dev);
+		break;
+	}
+	return gspca_dev->usb_err;
+}
+
+static const struct v4l2_ctrl_ops zcxx_ctrl_ops = {
+	.g_volatile_ctrl = zcxx_g_volatile_ctrl,
+	.s_ctrl = zcxx_s_ctrl,
+};
+
+static int sd_init_controls(struct gspca_dev *gspca_dev)
+{
+	struct sd *sd = (struct sd *)gspca_dev;
+	struct v4l2_ctrl_handler *hdl = &sd->ctrl_handler;
 	static const u8 gamma[SENSOR_MAX] = {
 		[SENSOR_ADCM2700] =	4,
 		[SENSOR_CS2102] =	4,
@@ -6484,6 +6426,48 @@ static int sd_init(struct gspca_dev *gspca_dev)
 		[SENSOR_PO2030] =	4,
 		[SENSOR_TAS5130C] =	3,
 	};
+
+	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;
+	}
+	v4l2_ctrl_cluster(3, &sd->gamma);
+	if (sd->sensor == SENSOR_HV7131R)
+		v4l2_ctrl_auto_cluster(2, &sd->autogain, 0, true);
+	return 0;
+}
+
+/* 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 cam *cam;
+	int sensor;
 	static const u8 mode_tb[SENSOR_MAX] = {
 		[SENSOR_ADCM2700] =	2,
 		[SENSOR_CS2102] =	1,
@@ -6613,7 +6597,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,26 +6638,6 @@ 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;
-	}
-#if AUTOGAIN_DEF
-	if (sd->ctrls[AUTOGAIN].val)
-		gspca_dev->ctrl_inac = (1 << EXPOSURE);
-#endif
-
 	/* switch off the led */
 	reg_w(gspca_dev, 0x01, 0x0000);
 	return gspca_dev->usb_err;
@@ -6791,7 +6754,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 +6763,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 +6779,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 +6791,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 +6815,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,79 +6876,17 @@ 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_set_jcomp(struct gspca_dev *gspca_dev,
 			struct v4l2_jpegcompression *jcomp)
 {
 	struct sd *sd = (struct sd *) gspca_dev;
+	int ret;
 
-	sd_setquality(gspca_dev, jcomp->quality);
-	jcomp->quality = sd->ctrls[QUALITY].val;
-	return gspca_dev->usb_err;
+	ret = v4l2_ctrl_s_ctrl(sd->jpegqual, jcomp->quality);
+	if (ret)
+		return ret;
+	jcomp->quality = v4l2_ctrl_g_ctrl(sd->jpegqual);
+	return 0;
 }
 
 static int sd_get_jcomp(struct gspca_dev *gspca_dev,
@@ -6991,7 +6895,7 @@ static int sd_get_jcomp(struct gspca_dev *gspca_dev,
 	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;
@@ -7015,15 +6919,13 @@ 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,
+	.init_controls = sd_init_controls,
 	.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)
@@ -7107,6 +7009,7 @@ static struct usb_driver sd_driver = {
 #ifdef CONFIG_PM
 	.suspend = gspca_suspend,
 	.resume = gspca_resume,
+	.reset_resume = gspca_resume,
 #endif
 };
 
-- 
1.7.10

[RFCv2 PATCH 15/17] gcpca-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 |  493 +++++++++++++++--------------------
 1 file changed, 210 insertions(+), 283 deletions(-)

diff --git a/drivers/media/video/gspca/sn9c20x.c b/drivers/media/video/gspca/sn9c20x.c
index 5285a51..81d75db 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;
@@ -167,175 +172,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,
@@ -1296,8 +1132,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;
 }
@@ -1312,9 +1146,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;
 }
@@ -1342,8 +1173,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;
 }
@@ -1380,9 +1209,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;
@@ -1424,8 +1250,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;
 }
@@ -1438,8 +1262,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;
 }
@@ -1472,8 +1294,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;
 }
@@ -1490,20 +1310,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;
@@ -1531,11 +1349,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);
@@ -1558,26 +1375,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) {
@@ -1640,17 +1452,16 @@ 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] = {sd->i2c_intf, sd->i2c_addr,
 				0x00, 0x00, 0x00, 0x00, 0x00, 0x10};
-	int expo, expo2;
+	int expo2;
 
 	if (gspca_dev->streaming)
 		exp[7] = 0x1e;
 
-	expo = sd->ctrls[EXPOSURE].val;
 	switch (sd->sensor) {
 	case SENSOR_OV7660:
 	case SENSOR_OV7670:
@@ -1697,17 +1508,15 @@ 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] = {sd->i2c_intf, sd->i2c_addr,
 				0x00, 0x00, 0x00, 0x00, 0x00, 0x10};
-	int g;
 
 	if (gspca_dev->streaming)
 		gain[7] = 0x15;		/* or 1d ? */
 
-	g = sd->ctrls[GAIN].val;
 	switch (sd->sensor) {
 	case SENSOR_OV7660:
 	case SENSOR_OV7670:
@@ -1746,11 +1555,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);
@@ -1879,13 +1688,129 @@ 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);
+	struct gspca_dev *gspca_dev = &sd->gspca_dev;
+
+	gspca_dev->usb_err = 0;
+
+	if (!gspca_dev->streaming)
+		return 0;
+
+	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);
+		break;
+	case V4L2_CID_GAMMA:
+		set_gamma(&sd->gspca_dev, ctrl->val);
+		break;
+	/* blue/red balance cluster */
+	case V4L2_CID_BLUE_BALANCE:
+		set_redblue(&sd->gspca_dev, sd->blue->val, sd->red->val);
+		break;
+	/* h/vflip cluster */
+	case V4L2_CID_HFLIP:
+		set_hvflip(&sd->gspca_dev, sd->hflip->val, sd->vflip->val);
+		break;
+	/* standalone exposure control */
+	case V4L2_CID_EXPOSURE:
+		set_exposure(&sd->gspca_dev, ctrl->val);
+		break;
+	/* standalone gain control */
+	case V4L2_CID_GAIN:
+		set_gain(&sd->gspca_dev, ctrl->val);
+		break;
+	/* 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);
+		break;
+	case V4L2_CID_JPEG_COMPRESSION_QUALITY:
+		set_quality(&sd->gspca_dev, ctrl->val);
+		break;
+	}
+	return gspca_dev->usb_err;
+}
+
+static const struct v4l2_ctrl_ops sd_ctrl_ops = {
+	.s_ctrl = sd_s_ctrl,
+};
+
+static int sd_init_controls(struct gspca_dev *gspca_dev)
+{
+	struct sd *sd = (struct sd *) gspca_dev;
+	struct v4l2_ctrl_handler *hdl = &sd->ctrl_handler;
+
+	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);
+	}
+
+	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 int sd_init(struct gspca_dev *gspca_dev)
 {
 	struct sd *sd = (struct sd *) gspca_dev;
@@ -1978,7 +1903,6 @@ static int sd_init(struct gspca_dev *gspca_dev)
 		pr_err("Unsupported sensor\n");
 		gspca_dev->usb_err = -ENODEV;
 	}
-
 	return gspca_dev->usb_err;
 }
 
@@ -2096,7 +2020,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;
@@ -2133,12 +2057,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);
@@ -2177,6 +2106,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;
 
 	/*
@@ -2185,16 +2117,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;
@@ -2205,15 +2136,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;
 
@@ -2227,19 +2157,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)
@@ -2247,7 +2170,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);
@@ -2263,10 +2186,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);
 }
 
@@ -2315,14 +2239,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);
 			}
 		}
@@ -2402,10 +2330,9 @@ 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,
+	.init_controls = sd_init_controls,
 	.isoc_init = sd_isoc_init,
 	.start = sd_start,
 	.stopN = sd_stopN,
-- 
1.7.10

[RFCv2 PATCH 16/17] gspca-stv06xx: 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/stv06xx/stv06xx.c        |   24 +-
 drivers/media/video/gspca/stv06xx/stv06xx.h        |    4 +
 drivers/media/video/gspca/stv06xx/stv06xx_hdcs.c   |  140 ++------
 drivers/media/video/gspca/stv06xx/stv06xx_hdcs.h   |    7 +-
 drivers/media/video/gspca/stv06xx/stv06xx_pb0100.c |  357 ++++++--------------
 drivers/media/video/gspca/stv06xx/stv06xx_pb0100.h |   12 +-
 drivers/media/video/gspca/stv06xx/stv06xx_sensor.h |    4 +-
 drivers/media/video/gspca/stv06xx/stv06xx_st6422.c |  234 ++++---------
 drivers/media/video/gspca/stv06xx/stv06xx_st6422.h |    4 +-
 drivers/media/video/gspca/stv06xx/stv06xx_vv6410.c |  197 +++--------
 drivers/media/video/gspca/stv06xx/stv06xx_vv6410.h |    8 +-
 11 files changed, 282 insertions(+), 709 deletions(-)

diff --git a/drivers/media/video/gspca/stv06xx/stv06xx.c b/drivers/media/video/gspca/stv06xx/stv06xx.c
index 91d99b4..ecac7d0 100644
--- a/drivers/media/video/gspca/stv06xx/stv06xx.c
+++ b/drivers/media/video/gspca/stv06xx/stv06xx.c
@@ -261,6 +261,23 @@ static int stv06xx_init(struct gspca_dev *gspca_dev)
 	return (err < 0) ? err : 0;
 }
 
+/* this function is called at probe time */
+static int stv06xx_init_controls(struct gspca_dev *gspca_dev)
+{
+	struct sd *sd = (struct sd *) gspca_dev;
+	int err;
+
+	PDEBUG(D_PROBE, "Initializing controls");
+
+	err = sd->sensor->init_controls(sd);
+	if (err) {
+		v4l2_ctrl_handler_free(&sd->ctrl_handler);
+		return err;
+	}
+	gspca_dev->vdev.ctrl_handler = &sd->ctrl_handler;
+	return 0;
+}
+
 /* Start the camera */
 static int stv06xx_start(struct gspca_dev *gspca_dev)
 {
@@ -512,6 +529,7 @@ static const struct sd_desc sd_desc = {
 	.name = MODULE_NAME,
 	.config = stv06xx_config,
 	.init = stv06xx_init,
+	.init_controls = stv06xx_init_controls,
 	.start = stv06xx_start,
 	.stopN = stv06xx_stopN,
 	.pkt_scan = stv06xx_pkt_scan,
@@ -594,11 +612,12 @@ static void sd_disconnect(struct usb_interface *intf)
 {
 	struct gspca_dev *gspca_dev = usb_get_intfdata(intf);
 	struct sd *sd = (struct sd *) gspca_dev;
+	void *priv = sd->sensor_priv;
 	PDEBUG(D_PROBE, "Disconnecting the stv06xx device");
 
-	if (sd->sensor->disconnect)
-		sd->sensor->disconnect(sd);
+	sd->sensor = NULL;
 	gspca_disconnect(intf);
+	kfree(priv);
 }
 
 static struct usb_driver sd_driver = {
@@ -609,6 +628,7 @@ static struct usb_driver sd_driver = {
 #ifdef CONFIG_PM
 	.suspend = gspca_suspend,
 	.resume = gspca_resume,
+	.reset_resume = gspca_resume,
 #endif
 };
 
diff --git a/drivers/media/video/gspca/stv06xx/stv06xx.h b/drivers/media/video/gspca/stv06xx/stv06xx.h
index d270a59..e04c862 100644
--- a/drivers/media/video/gspca/stv06xx/stv06xx.h
+++ b/drivers/media/video/gspca/stv06xx/stv06xx.h
@@ -31,6 +31,7 @@
 #define STV06XX_H_
 
 #include <linux/slab.h>
+#include <media/v4l2-ctrls.h>
 #include "gspca.h"
 
 #define MODULE_NAME "STV06xx"
@@ -89,6 +90,9 @@ struct sd {
 	/* A pointer to the currently connected sensor */
 	const struct stv06xx_sensor *sensor;
 
+	/* Control handler */
+	struct v4l2_ctrl_handler ctrl_handler;
+
 	/* A pointer to the sd_desc struct */
 	struct sd_desc desc;
 
diff --git a/drivers/media/video/gspca/stv06xx/stv06xx_hdcs.c b/drivers/media/video/gspca/stv06xx/stv06xx_hdcs.c
index a8698b7..b124ee7 100644
--- a/drivers/media/video/gspca/stv06xx/stv06xx_hdcs.c
+++ b/drivers/media/video/gspca/stv06xx/stv06xx_hdcs.c
@@ -32,36 +32,6 @@
 
 #include "stv06xx_hdcs.h"
 
-static const struct ctrl hdcs1x00_ctrl[] = {
-	{
-		{
-			.id		= V4L2_CID_EXPOSURE,
-			.type		= V4L2_CTRL_TYPE_INTEGER,
-			.name		= "exposure",
-			.minimum	= 0x00,
-			.maximum	= 0xff,
-			.step		= 0x1,
-			.default_value	= HDCS_DEFAULT_EXPOSURE,
-			.flags		= V4L2_CTRL_FLAG_SLIDER
-		},
-		.set = hdcs_set_exposure,
-		.get = hdcs_get_exposure
-	}, {
-		{
-			.id		= V4L2_CID_GAIN,
-			.type		= V4L2_CTRL_TYPE_INTEGER,
-			.name		= "gain",
-			.minimum	= 0x00,
-			.maximum	= 0xff,
-			.step		= 0x1,
-			.default_value	= HDCS_DEFAULT_GAIN,
-			.flags		= V4L2_CTRL_FLAG_SLIDER
-		},
-		.set = hdcs_set_gain,
-		.get = hdcs_get_gain
-	}
-};
-
 static struct v4l2_pix_format hdcs1x00_mode[] = {
 	{
 		HDCS_1X00_DEF_WIDTH,
@@ -76,36 +46,6 @@ static struct v4l2_pix_format hdcs1x00_mode[] = {
 	}
 };
 
-static const struct ctrl hdcs1020_ctrl[] = {
-	{
-		{
-			.id		= V4L2_CID_EXPOSURE,
-			.type		= V4L2_CTRL_TYPE_INTEGER,
-			.name		= "exposure",
-			.minimum	= 0x00,
-			.maximum	= 0xffff,
-			.step		= 0x1,
-			.default_value	= HDCS_DEFAULT_EXPOSURE,
-			.flags		= V4L2_CTRL_FLAG_SLIDER
-		},
-		.set = hdcs_set_exposure,
-		.get = hdcs_get_exposure
-	}, {
-		{
-			.id		= V4L2_CID_GAIN,
-			.type		= V4L2_CTRL_TYPE_INTEGER,
-			.name		= "gain",
-			.minimum	= 0x00,
-			.maximum	= 0xff,
-			.step		= 0x1,
-			.default_value	= HDCS_DEFAULT_GAIN,
-			.flags		= V4L2_CTRL_FLAG_SLIDER
-		},
-		.set = hdcs_set_gain,
-		.get = hdcs_get_gain
-	}
-};
-
 static struct v4l2_pix_format hdcs1020_mode[] = {
 	{
 		HDCS_1020_DEF_WIDTH,
@@ -150,7 +90,6 @@ struct hdcs {
 	} exp;
 
 	int psmp;
-	u8 exp_cache, gain_cache;
 };
 
 static int hdcs_reg_write_seq(struct sd *sd, u8 reg, u8 *vals, u8 len)
@@ -232,16 +171,6 @@ static int hdcs_reset(struct sd *sd)
 	return err;
 }
 
-static int hdcs_get_exposure(struct gspca_dev *gspca_dev, __s32 *val)
-{
-	struct sd *sd = (struct sd *) gspca_dev;
-	struct hdcs *hdcs = sd->sensor_priv;
-
-	*val = hdcs->exp_cache;
-
-	return 0;
-}
-
 static int hdcs_set_exposure(struct gspca_dev *gspca_dev, __s32 val)
 {
 	struct sd *sd = (struct sd *) gspca_dev;
@@ -261,7 +190,6 @@ static int hdcs_set_exposure(struct gspca_dev *gspca_dev, __s32 val)
 	u8 exp[14];
 
 	val &= 0xff;
-	hdcs->exp_cache = val;
 
 	cycles = val * HDCS_CLK_FREQ_MHZ * 257;
 
@@ -336,12 +264,9 @@ static int hdcs_set_exposure(struct gspca_dev *gspca_dev, __s32 val)
 
 static int hdcs_set_gains(struct sd *sd, u8 g)
 {
-	struct hdcs *hdcs = sd->sensor_priv;
 	int err;
 	u8 gains[4];
 
-	hdcs->gain_cache = g;
-
 	/* the voltage gain Av = (1 + 19 * val / 127) * (1 + bit7) */
 	if (g > 127)
 		g = 0x80 | (g / 2);
@@ -352,17 +277,7 @@ static int hdcs_set_gains(struct sd *sd, u8 g)
 	gains[3] = g;
 
 	err = hdcs_reg_write_seq(sd, HDCS_ERECPGA, gains, 4);
-		return err;
-}
-
-static int hdcs_get_gain(struct gspca_dev *gspca_dev, __s32 *val)
-{
-	struct sd *sd = (struct sd *) gspca_dev;
-	struct hdcs *hdcs = sd->sensor_priv;
-
-	*val = hdcs->gain_cache;
-
-	return 0;
+	return err;
 }
 
 static int hdcs_set_gain(struct gspca_dev *gspca_dev, __s32 val)
@@ -420,6 +335,38 @@ static int hdcs_set_size(struct sd *sd,
 	return err;
 }
 
+static int hdcs_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct sd *sd = container_of(ctrl->handler, struct sd, ctrl_handler);
+	int err = -EINVAL;
+
+	switch (ctrl->id) {
+	case V4L2_CID_GAIN:
+		err = hdcs_set_gain(&sd->gspca_dev, ctrl->val);
+		break;
+	case V4L2_CID_EXPOSURE:
+		err = hdcs_set_exposure(&sd->gspca_dev, ctrl->val);
+		break;
+	}
+	return err;
+}
+
+static const struct v4l2_ctrl_ops hdcs_ctrl_ops = {
+	.s_ctrl = hdcs_s_ctrl,
+};
+
+static int hdcs_init_controls(struct sd *sd)
+{
+	struct v4l2_ctrl_handler *hdl = &sd->ctrl_handler;
+
+	v4l2_ctrl_handler_init(hdl, 2);
+	v4l2_ctrl_new_std(hdl, &hdcs_ctrl_ops,
+			V4L2_CID_EXPOSURE, 0, 0xff, 1, HDCS_DEFAULT_EXPOSURE);
+	v4l2_ctrl_new_std(hdl, &hdcs_ctrl_ops,
+			V4L2_CID_GAIN, 0, 0xff, 1, HDCS_DEFAULT_GAIN);
+	return hdl->error;
+}
+
 static int hdcs_probe_1x00(struct sd *sd)
 {
 	struct hdcs *hdcs;
@@ -434,8 +381,6 @@ static int hdcs_probe_1x00(struct sd *sd)
 
 	sd->gspca_dev.cam.cam_mode = hdcs1x00_mode;
 	sd->gspca_dev.cam.nmodes = ARRAY_SIZE(hdcs1x00_mode);
-	sd->desc.ctrls = hdcs1x00_ctrl;
-	sd->desc.nctrls = ARRAY_SIZE(hdcs1x00_ctrl);
 
 	hdcs = kmalloc(sizeof(struct hdcs), GFP_KERNEL);
 	if (!hdcs)
@@ -493,8 +438,6 @@ static int hdcs_probe_1020(struct sd *sd)
 
 	sd->gspca_dev.cam.cam_mode = hdcs1020_mode;
 	sd->gspca_dev.cam.nmodes = ARRAY_SIZE(hdcs1020_mode);
-	sd->desc.ctrls = hdcs1020_ctrl;
-	sd->desc.nctrls = ARRAY_SIZE(hdcs1020_ctrl);
 
 	hdcs = kmalloc(sizeof(struct hdcs), GFP_KERNEL);
 	if (!hdcs)
@@ -537,12 +480,6 @@ static int hdcs_stop(struct sd *sd)
 	return hdcs_set_state(sd, HDCS_STATE_SLEEP);
 }
 
-static void hdcs_disconnect(struct sd *sd)
-{
-	PDEBUG(D_PROBE, "Disconnecting the sensor");
-	kfree(sd->sensor_priv);
-}
-
 static int hdcs_init(struct sd *sd)
 {
 	struct hdcs *hdcs = sd->sensor_priv;
@@ -587,16 +524,7 @@ static int hdcs_init(struct sd *sd)
 	if (err < 0)
 		return err;
 
-	err = hdcs_set_gains(sd, HDCS_DEFAULT_GAIN);
-	if (err < 0)
-		return err;
-
-	err = hdcs_set_size(sd, hdcs->array.width, hdcs->array.height);
-	if (err < 0)
-		return err;
-
-	err = hdcs_set_exposure(&sd->gspca_dev, HDCS_DEFAULT_EXPOSURE);
-	return err;
+	return hdcs_set_size(sd, hdcs->array.width, hdcs->array.height);
 }
 
 static int hdcs_dump(struct sd *sd)
diff --git a/drivers/media/video/gspca/stv06xx/stv06xx_hdcs.h b/drivers/media/video/gspca/stv06xx/stv06xx_hdcs.h
index a14a84a..1ba9158 100644
--- a/drivers/media/video/gspca/stv06xx/stv06xx_hdcs.h
+++ b/drivers/media/video/gspca/stv06xx/stv06xx_hdcs.h
@@ -131,14 +131,12 @@ static int hdcs_probe_1x00(struct sd *sd);
 static int hdcs_probe_1020(struct sd *sd);
 static int hdcs_start(struct sd *sd);
 static int hdcs_init(struct sd *sd);
+static int hdcs_init_controls(struct sd *sd);
 static int hdcs_stop(struct sd *sd);
 static int hdcs_dump(struct sd *sd);
-static void hdcs_disconnect(struct sd *sd);
 
-static int hdcs_get_exposure(struct gspca_dev *gspca_dev, __s32 *val);
 static int hdcs_set_exposure(struct gspca_dev *gspca_dev, __s32 val);
 static int hdcs_set_gain(struct gspca_dev *gspca_dev, __s32 val);
-static int hdcs_get_gain(struct gspca_dev *gspca_dev, __s32 *val);
 
 const struct stv06xx_sensor stv06xx_sensor_hdcs1x00 = {
 	.name = "HP HDCS-1000/1100",
@@ -152,10 +150,10 @@ const struct stv06xx_sensor stv06xx_sensor_hdcs1x00 = {
 	.max_packet_size = { 847 },
 
 	.init = hdcs_init,
+	.init_controls = hdcs_init_controls,
 	.probe = hdcs_probe_1x00,
 	.start = hdcs_start,
 	.stop = hdcs_stop,
-	.disconnect = hdcs_disconnect,
 	.dump = hdcs_dump,
 };
 
@@ -171,6 +169,7 @@ const struct stv06xx_sensor stv06xx_sensor_hdcs1020 = {
 	.max_packet_size = { 847 },
 
 	.init = hdcs_init,
+	.init_controls = hdcs_init_controls,
 	.probe = hdcs_probe_1020,
 	.start = hdcs_start,
 	.stop = hdcs_stop,
diff --git a/drivers/media/video/gspca/stv06xx/stv06xx_pb0100.c b/drivers/media/video/gspca/stv06xx/stv06xx_pb0100.c
index 26f14fc..c03b13e 100644
--- a/drivers/media/video/gspca/stv06xx/stv06xx_pb0100.c
+++ b/drivers/media/video/gspca/stv06xx/stv06xx_pb0100.c
@@ -48,105 +48,16 @@
 
 #include "stv06xx_pb0100.h"
 
-static const struct ctrl pb0100_ctrl[] = {
-#define GAIN_IDX 0
-	{
-		{
-			.id		= V4L2_CID_GAIN,
-			.type		= V4L2_CTRL_TYPE_INTEGER,
-			.name		= "Gain",
-			.minimum	= 0,
-			.maximum	= 255,
-			.step		= 1,
-			.default_value  = 128
-		},
-		.set = pb0100_set_gain,
-		.get = pb0100_get_gain
-	},
-#define RED_BALANCE_IDX 1
-	{
-		{
-			.id		= V4L2_CID_RED_BALANCE,
-			.type		= V4L2_CTRL_TYPE_INTEGER,
-			.name		= "Red Balance",
-			.minimum	= -255,
-			.maximum	= 255,
-			.step		= 1,
-			.default_value  = 0
-		},
-		.set = pb0100_set_red_balance,
-		.get = pb0100_get_red_balance
-	},
-#define BLUE_BALANCE_IDX 2
-	{
-		{
-			.id		= V4L2_CID_BLUE_BALANCE,
-			.type		= V4L2_CTRL_TYPE_INTEGER,
-			.name		= "Blue Balance",
-			.minimum	= -255,
-			.maximum	= 255,
-			.step		= 1,
-			.default_value  = 0
-		},
-		.set = pb0100_set_blue_balance,
-		.get = pb0100_get_blue_balance
-	},
-#define EXPOSURE_IDX 3
-	{
-		{
-			.id		= V4L2_CID_EXPOSURE,
-			.type		= V4L2_CTRL_TYPE_INTEGER,
-			.name		= "Exposure",
-			.minimum	= 0,
-			.maximum	= 511,
-			.step		= 1,
-			.default_value  = 12
-		},
-		.set = pb0100_set_exposure,
-		.get = pb0100_get_exposure
-	},
-#define AUTOGAIN_IDX 4
-	{
-		{
-			.id		= V4L2_CID_AUTOGAIN,
-			.type		= V4L2_CTRL_TYPE_BOOLEAN,
-			.name		= "Automatic Gain and Exposure",
-			.minimum	= 0,
-			.maximum	= 1,
-			.step		= 1,
-			.default_value  = 1
-		},
-		.set = pb0100_set_autogain,
-		.get = pb0100_get_autogain
-	},
-#define AUTOGAIN_TARGET_IDX 5
-	{
-		{
-			.id		= V4L2_CTRL_CLASS_USER + 0x1000,
-			.type		= V4L2_CTRL_TYPE_INTEGER,
-			.name		= "Automatic Gain Target",
-			.minimum	= 0,
-			.maximum	= 255,
-			.step		= 1,
-			.default_value  = 128
-		},
-		.set = pb0100_set_autogain_target,
-		.get = pb0100_get_autogain_target
-	},
-#define NATURAL_IDX 6
-	{
-		{
-			.id		= V4L2_CTRL_CLASS_USER + 0x1001,
-			.type		= V4L2_CTRL_TYPE_BOOLEAN,
-			.name		= "Natural Light Source",
-			.minimum	= 0,
-			.maximum	= 1,
-			.step		= 1,
-			.default_value  = 1
-		},
-		.set = pb0100_set_natural,
-		.get = pb0100_get_natural
-	}
+struct pb0100_ctrls {
+	struct { /* one big happy control cluster... */
+		struct v4l2_ctrl *autogain;
+		struct v4l2_ctrl *gain;
+		struct v4l2_ctrl *exposure;
+		struct v4l2_ctrl *red;
+		struct v4l2_ctrl *blue;
+		struct v4l2_ctrl *natural;
+	};
+	struct v4l2_ctrl *target;
 };
 
 static struct v4l2_pix_format pb0100_mode[] = {
@@ -174,38 +85,102 @@ static struct v4l2_pix_format pb0100_mode[] = {
 	}
 };
 
+static int pb0100_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct sd *sd = container_of(ctrl->handler, struct sd, ctrl_handler);
+	struct pb0100_ctrls *ctrls = sd->sensor_priv;
+	int err = -EINVAL;
+
+	switch (ctrl->id) {
+	case V4L2_CID_AUTOGAIN:
+		err = pb0100_set_autogain(&sd->gspca_dev, ctrl->val);
+		if (err)
+			break;
+		if (ctrl->val)
+			break;
+		err = pb0100_set_gain(&sd->gspca_dev, ctrls->gain->val);
+		if (err)
+			break;
+		err = pb0100_set_exposure(&sd->gspca_dev, ctrls->exposure->val);
+		break;
+	case V4L2_CTRL_CLASS_USER + 0x1001:
+		err = pb0100_set_autogain_target(&sd->gspca_dev, ctrl->val);
+		break;
+	}
+	return err;
+}
+
+static const struct v4l2_ctrl_ops pb0100_ctrl_ops = {
+	.s_ctrl = pb0100_s_ctrl,
+};
+
+static int pb0100_init_controls(struct sd *sd)
+{
+	struct v4l2_ctrl_handler *hdl = &sd->ctrl_handler;
+	struct pb0100_ctrls *ctrls;
+	static const struct v4l2_ctrl_config autogain_target = {
+		.ops = &pb0100_ctrl_ops,
+		.id = V4L2_CTRL_CLASS_USER + 0x1000,
+		.type = V4L2_CTRL_TYPE_INTEGER,
+		.name = "Automatic Gain Target",
+		.max = 255,
+		.step = 1,
+		.def = 128,
+	};
+	static const struct v4l2_ctrl_config natural_light = {
+		.ops = &pb0100_ctrl_ops,
+		.id = V4L2_CTRL_CLASS_USER + 0x1001,
+		.type = V4L2_CTRL_TYPE_BOOLEAN,
+		.name = "Natural Light Source",
+		.max = 1,
+		.step = 1,
+		.def = 1,
+	};
+
+	ctrls = kzalloc(sizeof(*ctrls), GFP_KERNEL);
+	if (!ctrls)
+		return -ENOMEM;
+
+	v4l2_ctrl_handler_init(hdl, 6);
+	ctrls->autogain = v4l2_ctrl_new_std(hdl, &pb0100_ctrl_ops,
+			V4L2_CID_AUTOGAIN, 0, 1, 1, 1);
+	ctrls->exposure = v4l2_ctrl_new_std(hdl, &pb0100_ctrl_ops,
+			V4L2_CID_EXPOSURE, 0, 511, 1, 12);
+	ctrls->gain = v4l2_ctrl_new_std(hdl, &pb0100_ctrl_ops,
+			V4L2_CID_GAIN, 0, 255, 1, 128);
+	ctrls->red = v4l2_ctrl_new_std(hdl, &pb0100_ctrl_ops,
+			V4L2_CID_RED_BALANCE, -255, 255, 1, 0);
+	ctrls->blue = v4l2_ctrl_new_std(hdl, &pb0100_ctrl_ops,
+			V4L2_CID_BLUE_BALANCE, -255, 255, 1, 0);
+	ctrls->natural = v4l2_ctrl_new_custom(hdl, &natural_light, NULL);
+	ctrls->target = v4l2_ctrl_new_custom(hdl, &autogain_target, NULL);
+	if (hdl->error) {
+		kfree(ctrls);
+		return hdl->error;
+	}
+	sd->sensor_priv = ctrls;
+	v4l2_ctrl_auto_cluster(5, &ctrls->autogain, 0, false);
+	return 0;
+}
+
 static int pb0100_probe(struct sd *sd)
 {
 	u16 sensor;
-	int i, err;
-	s32 *sensor_settings;
+	int err;
 
 	err = stv06xx_read_sensor(sd, PB_IDENT, &sensor);
 
 	if (err < 0)
 		return -ENODEV;
+	if ((sensor >> 8) != 0x64)
+		return -ENODEV;
 
-	if ((sensor >> 8) == 0x64) {
-		sensor_settings = kmalloc(
-				ARRAY_SIZE(pb0100_ctrl) * sizeof(s32),
-				GFP_KERNEL);
-		if (!sensor_settings)
-			return -ENOMEM;
-
-		pr_info("Photobit pb0100 sensor detected\n");
-
-		sd->gspca_dev.cam.cam_mode = pb0100_mode;
-		sd->gspca_dev.cam.nmodes = ARRAY_SIZE(pb0100_mode);
-		sd->desc.ctrls = pb0100_ctrl;
-		sd->desc.nctrls = ARRAY_SIZE(pb0100_ctrl);
-		for (i = 0; i < sd->desc.nctrls; i++)
-			sensor_settings[i] = pb0100_ctrl[i].qctrl.default_value;
-		sd->sensor_priv = sensor_settings;
+	pr_info("Photobit pb0100 sensor detected\n");
 
-		return 0;
-	}
+	sd->gspca_dev.cam.cam_mode = pb0100_mode;
+	sd->gspca_dev.cam.nmodes = ARRAY_SIZE(pb0100_mode);
 
-	return -ENODEV;
+	return 0;
 }
 
 static int pb0100_start(struct sd *sd)
@@ -214,7 +189,6 @@ static int pb0100_start(struct sd *sd)
 	struct usb_host_interface *alt;
 	struct usb_interface *intf;
 	struct cam *cam = &sd->gspca_dev.cam;
-	s32 *sensor_settings = sd->sensor_priv;
 	u32 mode = cam->cam_mode[sd->gspca_dev.curr_mode].priv;
 
 	intf = usb_ifnum_to_if(sd->gspca_dev.dev, sd->gspca_dev.iface);
@@ -255,13 +229,6 @@ static int pb0100_start(struct sd *sd)
 		stv06xx_write_bridge(sd, STV_SCAN_RATE, 0x20);
 	}
 
-	/* set_gain also sets red and blue balance */
-	pb0100_set_gain(&sd->gspca_dev, sensor_settings[GAIN_IDX]);
-	pb0100_set_exposure(&sd->gspca_dev, sensor_settings[EXPOSURE_IDX]);
-	pb0100_set_autogain_target(&sd->gspca_dev,
-				   sensor_settings[AUTOGAIN_TARGET_IDX]);
-	pb0100_set_autogain(&sd->gspca_dev, sensor_settings[AUTOGAIN_IDX]);
-
 	err = stv06xx_write_sensor(sd, PB_CONTROL, BIT(5)|BIT(3)|BIT(1));
 	PDEBUG(D_STREAM, "Started stream, status: %d", err);
 
@@ -285,12 +252,6 @@ out:
 	return (err < 0) ? err : 0;
 }
 
-static void pb0100_disconnect(struct sd *sd)
-{
-	sd->sensor = NULL;
-	kfree(sd->sensor_priv);
-}
-
 /* FIXME: Sort the init commands out and put them into tables,
 	  this is only for getting the camera to work */
 /* FIXME: No error handling for now,
@@ -362,62 +323,32 @@ static int pb0100_dump(struct sd *sd)
 	return 0;
 }
 
-static int pb0100_get_gain(struct gspca_dev *gspca_dev, __s32 *val)
-{
-	struct sd *sd = (struct sd *) gspca_dev;
-	s32 *sensor_settings = sd->sensor_priv;
-
-	*val = sensor_settings[GAIN_IDX];
-
-	return 0;
-}
-
 static int pb0100_set_gain(struct gspca_dev *gspca_dev, __s32 val)
 {
 	int err;
 	struct sd *sd = (struct sd *) gspca_dev;
-	s32 *sensor_settings = sd->sensor_priv;
+	struct pb0100_ctrls *ctrls = sd->sensor_priv;
 
-	if (sensor_settings[AUTOGAIN_IDX])
-		return -EBUSY;
-
-	sensor_settings[GAIN_IDX] = val;
 	err = stv06xx_write_sensor(sd, PB_G1GAIN, val);
 	if (!err)
 		err = stv06xx_write_sensor(sd, PB_G2GAIN, val);
 	PDEBUG(D_V4L2, "Set green gain to %d, status: %d", val, err);
 
 	if (!err)
-		err = pb0100_set_red_balance(gspca_dev,
-					     sensor_settings[RED_BALANCE_IDX]);
+		err = pb0100_set_red_balance(gspca_dev, ctrls->red->val);
 	if (!err)
-		err = pb0100_set_blue_balance(gspca_dev,
-					    sensor_settings[BLUE_BALANCE_IDX]);
+		err = pb0100_set_blue_balance(gspca_dev, ctrls->blue->val);
 
 	return err;
 }
 
-static int pb0100_get_red_balance(struct gspca_dev *gspca_dev, __s32 *val)
-{
-	struct sd *sd = (struct sd *) gspca_dev;
-	s32 *sensor_settings = sd->sensor_priv;
-
-	*val = sensor_settings[RED_BALANCE_IDX];
-
-	return 0;
-}
-
 static int pb0100_set_red_balance(struct gspca_dev *gspca_dev, __s32 val)
 {
 	int err;
 	struct sd *sd = (struct sd *) gspca_dev;
-	s32 *sensor_settings = sd->sensor_priv;
+	struct pb0100_ctrls *ctrls = sd->sensor_priv;
 
-	if (sensor_settings[AUTOGAIN_IDX])
-		return -EBUSY;
-
-	sensor_settings[RED_BALANCE_IDX] = val;
-	val += sensor_settings[GAIN_IDX];
+	val += ctrls->gain->val;
 	if (val < 0)
 		val = 0;
 	else if (val > 255)
@@ -429,27 +360,13 @@ static int pb0100_set_red_balance(struct gspca_dev *gspca_dev, __s32 val)
 	return err;
 }
 
-static int pb0100_get_blue_balance(struct gspca_dev *gspca_dev, __s32 *val)
-{
-	struct sd *sd = (struct sd *) gspca_dev;
-	s32 *sensor_settings = sd->sensor_priv;
-
-	*val = sensor_settings[BLUE_BALANCE_IDX];
-
-	return 0;
-}
-
 static int pb0100_set_blue_balance(struct gspca_dev *gspca_dev, __s32 val)
 {
 	int err;
 	struct sd *sd = (struct sd *) gspca_dev;
-	s32 *sensor_settings = sd->sensor_priv;
+	struct pb0100_ctrls *ctrls = sd->sensor_priv;
 
-	if (sensor_settings[AUTOGAIN_IDX])
-		return -EBUSY;
-
-	sensor_settings[BLUE_BALANCE_IDX] = val;
-	val += sensor_settings[GAIN_IDX];
+	val += ctrls->gain->val;
 	if (val < 0)
 		val = 0;
 	else if (val > 255)
@@ -461,51 +378,25 @@ static int pb0100_set_blue_balance(struct gspca_dev *gspca_dev, __s32 val)
 	return err;
 }
 
-static int pb0100_get_exposure(struct gspca_dev *gspca_dev, __s32 *val)
-{
-	struct sd *sd = (struct sd *) gspca_dev;
-	s32 *sensor_settings = sd->sensor_priv;
-
-	*val = sensor_settings[EXPOSURE_IDX];
-
-	return 0;
-}
-
 static int pb0100_set_exposure(struct gspca_dev *gspca_dev, __s32 val)
 {
-	int err;
 	struct sd *sd = (struct sd *) gspca_dev;
-	s32 *sensor_settings = sd->sensor_priv;
-
-	if (sensor_settings[AUTOGAIN_IDX])
-		return -EBUSY;
+	int err;
 
-	sensor_settings[EXPOSURE_IDX] = val;
 	err = stv06xx_write_sensor(sd, PB_RINTTIME, val);
 	PDEBUG(D_V4L2, "Set exposure to %d, status: %d", val, err);
 
 	return err;
 }
 
-static int pb0100_get_autogain(struct gspca_dev *gspca_dev, __s32 *val)
-{
-	struct sd *sd = (struct sd *) gspca_dev;
-	s32 *sensor_settings = sd->sensor_priv;
-
-	*val = sensor_settings[AUTOGAIN_IDX];
-
-	return 0;
-}
-
 static int pb0100_set_autogain(struct gspca_dev *gspca_dev, __s32 val)
 {
 	int err;
 	struct sd *sd = (struct sd *) gspca_dev;
-	s32 *sensor_settings = sd->sensor_priv;
+	struct pb0100_ctrls *ctrls = sd->sensor_priv;
 
-	sensor_settings[AUTOGAIN_IDX] = val;
-	if (sensor_settings[AUTOGAIN_IDX]) {
-		if (sensor_settings[NATURAL_IDX])
+	if (val) {
+		if (ctrls->natural->val)
 			val = BIT(6)|BIT(4)|BIT(0);
 		else
 			val = BIT(4)|BIT(0);
@@ -514,29 +405,15 @@ static int pb0100_set_autogain(struct gspca_dev *gspca_dev, __s32 val)
 
 	err = stv06xx_write_sensor(sd, PB_EXPGAIN, val);
 	PDEBUG(D_V4L2, "Set autogain to %d (natural: %d), status: %d",
-	       sensor_settings[AUTOGAIN_IDX], sensor_settings[NATURAL_IDX],
-	       err);
+	       val, ctrls->natural->val, err);
 
 	return err;
 }
 
-static int pb0100_get_autogain_target(struct gspca_dev *gspca_dev, __s32 *val)
-{
-	struct sd *sd = (struct sd *) gspca_dev;
-	s32 *sensor_settings = sd->sensor_priv;
-
-	*val = sensor_settings[AUTOGAIN_TARGET_IDX];
-
-	return 0;
-}
-
 static int pb0100_set_autogain_target(struct gspca_dev *gspca_dev, __s32 val)
 {
 	int err, totalpixels, brightpixels, darkpixels;
 	struct sd *sd = (struct sd *) gspca_dev;
-	s32 *sensor_settings = sd->sensor_priv;
-
-	sensor_settings[AUTOGAIN_TARGET_IDX] = val;
 
 	/* Number of pixels counted by the sensor when subsampling the pixels.
 	 * Slightly larger than the real value to avoid oscillation */
@@ -553,23 +430,3 @@ static int pb0100_set_autogain_target(struct gspca_dev *gspca_dev, __s32 val)
 
 	return err;
 }
-
-static int pb0100_get_natural(struct gspca_dev *gspca_dev, __s32 *val)
-{
-	struct sd *sd = (struct sd *) gspca_dev;
-	s32 *sensor_settings = sd->sensor_priv;
-
-	*val = sensor_settings[NATURAL_IDX];
-
-	return 0;
-}
-
-static int pb0100_set_natural(struct gspca_dev *gspca_dev, __s32 val)
-{
-	struct sd *sd = (struct sd *) gspca_dev;
-	s32 *sensor_settings = sd->sensor_priv;
-
-	sensor_settings[NATURAL_IDX] = val;
-
-	return pb0100_set_autogain(gspca_dev, sensor_settings[AUTOGAIN_IDX]);
-}
diff --git a/drivers/media/video/gspca/stv06xx/stv06xx_pb0100.h b/drivers/media/video/gspca/stv06xx/stv06xx_pb0100.h
index 757de24..5071e53 100644
--- a/drivers/media/video/gspca/stv06xx/stv06xx_pb0100.h
+++ b/drivers/media/video/gspca/stv06xx/stv06xx_pb0100.h
@@ -112,25 +112,17 @@
 static int pb0100_probe(struct sd *sd);
 static int pb0100_start(struct sd *sd);
 static int pb0100_init(struct sd *sd);
+static int pb0100_init_controls(struct sd *sd);
 static int pb0100_stop(struct sd *sd);
 static int pb0100_dump(struct sd *sd);
-static void pb0100_disconnect(struct sd *sd);
 
 /* V4L2 controls supported by the driver */
-static int pb0100_get_gain(struct gspca_dev *gspca_dev, __s32 *val);
 static int pb0100_set_gain(struct gspca_dev *gspca_dev, __s32 val);
-static int pb0100_get_red_balance(struct gspca_dev *gspca_dev, __s32 *val);
 static int pb0100_set_red_balance(struct gspca_dev *gspca_dev, __s32 val);
-static int pb0100_get_blue_balance(struct gspca_dev *gspca_dev, __s32 *val);
 static int pb0100_set_blue_balance(struct gspca_dev *gspca_dev, __s32 val);
-static int pb0100_get_exposure(struct gspca_dev *gspca_dev, __s32 *val);
 static int pb0100_set_exposure(struct gspca_dev *gspca_dev, __s32 val);
-static int pb0100_get_autogain(struct gspca_dev *gspca_dev, __s32 *val);
 static int pb0100_set_autogain(struct gspca_dev *gspca_dev, __s32 val);
-static int pb0100_get_autogain_target(struct gspca_dev *gspca_dev, __s32 *val);
 static int pb0100_set_autogain_target(struct gspca_dev *gspca_dev, __s32 val);
-static int pb0100_get_natural(struct gspca_dev *gspca_dev, __s32 *val);
-static int pb0100_set_natural(struct gspca_dev *gspca_dev, __s32 val);
 
 const struct stv06xx_sensor stv06xx_sensor_pb0100 = {
 	.name = "PB-0100",
@@ -142,11 +134,11 @@ const struct stv06xx_sensor stv06xx_sensor_pb0100 = {
 	.max_packet_size = { 847, 923 },
 
 	.init = pb0100_init,
+	.init_controls = pb0100_init_controls,
 	.probe = pb0100_probe,
 	.start = pb0100_start,
 	.stop = pb0100_stop,
 	.dump = pb0100_dump,
-	.disconnect = pb0100_disconnect,
 };
 
 #endif
diff --git a/drivers/media/video/gspca/stv06xx/stv06xx_sensor.h b/drivers/media/video/gspca/stv06xx/stv06xx_sensor.h
index fb229d8..3a498c2 100644
--- a/drivers/media/video/gspca/stv06xx/stv06xx_sensor.h
+++ b/drivers/media/video/gspca/stv06xx/stv06xx_sensor.h
@@ -63,8 +63,8 @@ struct stv06xx_sensor {
 	/* Performs a initialization sequence */
 	int (*init)(struct sd *sd);
 
-	/* Executed at device disconnect */
-	void (*disconnect)(struct sd *sd);
+	/* Initializes the controls */
+	int (*init_controls)(struct sd *sd);
 
 	/* Reads a sensor register */
 	int (*read_sensor)(struct sd *sd, const u8 address,
diff --git a/drivers/media/video/gspca/stv06xx/stv06xx_st6422.c b/drivers/media/video/gspca/stv06xx/stv06xx_st6422.c
index 9940e03..bbfe821 100644
--- a/drivers/media/video/gspca/stv06xx/stv06xx_st6422.c
+++ b/drivers/media/video/gspca/stv06xx/stv06xx_st6422.c
@@ -30,20 +30,6 @@
 
 #include "stv06xx_st6422.h"
 
-/* controls */
-enum e_ctrl {
-	BRIGHTNESS,
-	CONTRAST,
-	GAIN,
-	EXPOSURE,
-	NCTRLS		/* number of controls */
-};
-
-/* sensor settings */
-struct st6422_settings {
-	struct gspca_ctrl ctrls[NCTRLS];
-};
-
 static struct v4l2_pix_format st6422_mode[] = {
 	/* Note we actually get 124 lines of data, of which we skip the 4st
 	   4 as they are garbage */
@@ -74,83 +60,68 @@ static struct v4l2_pix_format st6422_mode[] = {
 };
 
 /* V4L2 controls supported by the driver */
-static void st6422_set_brightness(struct gspca_dev *gspca_dev);
-static void st6422_set_contrast(struct gspca_dev *gspca_dev);
-static void st6422_set_gain(struct gspca_dev *gspca_dev);
-static void st6422_set_exposure(struct gspca_dev *gspca_dev);
-
-static const struct ctrl st6422_ctrl[NCTRLS] = {
-[BRIGHTNESS] = {
-		{
-			.id		= V4L2_CID_BRIGHTNESS,
-			.type		= V4L2_CTRL_TYPE_INTEGER,
-			.name		= "Brightness",
-			.minimum	= 0,
-			.maximum	= 31,
-			.step		= 1,
-			.default_value  = 3
-		},
-		.set_control = st6422_set_brightness
-	},
-[CONTRAST] = {
-		{
-			.id		= V4L2_CID_CONTRAST,
-			.type		= V4L2_CTRL_TYPE_INTEGER,
-			.name		= "Contrast",
-			.minimum	= 0,
-			.maximum	= 15,
-			.step		= 1,
-			.default_value  = 11
-		},
-		.set_control = st6422_set_contrast
-	},
-[GAIN] = {
-		{
-			.id		= V4L2_CID_GAIN,
-			.type		= V4L2_CTRL_TYPE_INTEGER,
-			.name		= "Gain",
-			.minimum	= 0,
-			.maximum	= 255,
-			.step		= 1,
-			.default_value  = 64
-		},
-		.set_control = st6422_set_gain
-	},
-[EXPOSURE] = {
-		{
-			.id		= V4L2_CID_EXPOSURE,
-			.type		= V4L2_CTRL_TYPE_INTEGER,
-			.name		= "Exposure",
-			.minimum	= 0,
-#define EXPOSURE_MAX 1023
-			.maximum	= EXPOSURE_MAX,
-			.step		= 1,
-			.default_value  = 256
-		},
-		.set_control = st6422_set_exposure
-	},
+static int setbrightness(struct sd *sd, s32 val);
+static int setcontrast(struct sd *sd, s32 val);
+static int setgain(struct sd *sd, u8 gain);
+static int setexposure(struct sd *sd, s16 expo);
+
+static int st6422_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct sd *sd = container_of(ctrl->handler, struct sd, ctrl_handler);
+	int err = -EINVAL;
+
+	switch (ctrl->id) {
+	case V4L2_CID_BRIGHTNESS:
+		err = setbrightness(sd, ctrl->val);
+		break;
+	case V4L2_CID_CONTRAST:
+		err = setcontrast(sd, ctrl->val);
+		break;
+	case V4L2_CID_GAIN:
+		err = setgain(sd, ctrl->val);
+		break;
+	case V4L2_CID_EXPOSURE:
+		err = setexposure(sd, ctrl->val);
+		break;
+	}
+
+	/* commit settings */
+	if (err >= 0)
+		err = stv06xx_write_bridge(sd, 0x143f, 0x01);
+	sd->gspca_dev.usb_err = err;
+	return err;
+}
+
+static const struct v4l2_ctrl_ops st6422_ctrl_ops = {
+	.s_ctrl = st6422_s_ctrl,
 };
 
-static int st6422_probe(struct sd *sd)
+static int st6422_init_controls(struct sd *sd)
 {
-	struct st6422_settings *sensor_settings;
+	struct v4l2_ctrl_handler *hdl = &sd->ctrl_handler;
+
+	v4l2_ctrl_handler_init(hdl, 4);
+	v4l2_ctrl_new_std(hdl, &st6422_ctrl_ops,
+			V4L2_CID_BRIGHTNESS, 0, 31, 1, 3);
+	v4l2_ctrl_new_std(hdl, &st6422_ctrl_ops,
+			V4L2_CID_CONTRAST, 0, 15, 1, 11);
+	v4l2_ctrl_new_std(hdl, &st6422_ctrl_ops,
+			V4L2_CID_EXPOSURE, 0, 1023, 1, 256);
+	v4l2_ctrl_new_std(hdl, &st6422_ctrl_ops,
+			V4L2_CID_GAIN, 0, 255, 1, 64);
+
+	return hdl->error;
+}
 
+static int st6422_probe(struct sd *sd)
+{
 	if (sd->bridge != BRIDGE_ST6422)
 		return -ENODEV;
 
 	pr_info("st6422 sensor detected\n");
 
-	sensor_settings = kmalloc(sizeof *sensor_settings, GFP_KERNEL);
-	if (!sensor_settings)
-		return -ENOMEM;
-
 	sd->gspca_dev.cam.cam_mode = st6422_mode;
 	sd->gspca_dev.cam.nmodes = ARRAY_SIZE(st6422_mode);
-	sd->gspca_dev.cam.ctrls = sensor_settings->ctrls;
-	sd->desc.ctrls = st6422_ctrl;
-	sd->desc.nctrls = ARRAY_SIZE(st6422_ctrl);
-	sd->sensor_priv = sensor_settings;
-
 	return 0;
 }
 
@@ -239,38 +210,22 @@ static int st6422_init(struct sd *sd)
 	return err;
 }
 
-static void st6422_disconnect(struct sd *sd)
-{
-	sd->sensor = NULL;
-	kfree(sd->sensor_priv);
-}
-
-static int setbrightness(struct sd *sd)
+static int setbrightness(struct sd *sd, s32 val)
 {
-	struct st6422_settings *sensor_settings = sd->sensor_priv;
-
 	/* val goes from 0 -> 31 */
-	return stv06xx_write_bridge(sd, 0x1432,
-			sensor_settings->ctrls[BRIGHTNESS].val);
+	return stv06xx_write_bridge(sd, 0x1432, val);
 }
 
-static int setcontrast(struct sd *sd)
+static int setcontrast(struct sd *sd, s32 val)
 {
-	struct st6422_settings *sensor_settings = sd->sensor_priv;
-
 	/* Val goes from 0 -> 15 */
-	return stv06xx_write_bridge(sd, 0x143a,
-			sensor_settings->ctrls[CONTRAST].val | 0xf0);
+	return stv06xx_write_bridge(sd, 0x143a, val | 0xf0);
 }
 
-static int setgain(struct sd *sd)
+static int setgain(struct sd *sd, u8 gain)
 {
-	struct st6422_settings *sensor_settings = sd->sensor_priv;
-	u8 gain;
 	int err;
 
-	gain = sensor_settings->ctrls[GAIN].val;
-
 	/* Set red, green, blue, gain */
 	err = stv06xx_write_bridge(sd, 0x0509, gain);
 	if (err < 0)
@@ -292,13 +247,10 @@ static int setgain(struct sd *sd)
 	return stv06xx_write_bridge(sd, 0x050d, 0x01);
 }
 
-static int setexposure(struct sd *sd)
+static int setexposure(struct sd *sd, s16 expo)
 {
-	struct st6422_settings *sensor_settings = sd->sensor_priv;
-	u16 expo;
 	int err;
 
-	expo = sensor_settings->ctrls[EXPOSURE].val;
 	err = stv06xx_write_bridge(sd, 0x143d, expo & 0xff);
 	if (err < 0)
 		return err;
@@ -318,22 +270,6 @@ static int st6422_start(struct sd *sd)
 	if (err < 0)
 		return err;
 
-	err = setbrightness(sd);
-	if (err < 0)
-		return err;
-
-	err = setcontrast(sd);
-	if (err < 0)
-		return err;
-
-	err = setexposure(sd);
-	if (err < 0)
-		return err;
-
-	err = setgain(sd);
-	if (err < 0)
-		return err;
-
 	/* commit settings */
 	err = stv06xx_write_bridge(sd, 0x143f, 0x01);
 	return (err < 0) ? err : 0;
@@ -345,59 +281,3 @@ static int st6422_stop(struct sd *sd)
 
 	return 0;
 }
-
-static void st6422_set_brightness(struct gspca_dev *gspca_dev)
-{
-	int err;
-	struct sd *sd = (struct sd *) gspca_dev;
-
-	err = setbrightness(sd);
-
-	/* commit settings */
-	if (err >= 0)
-		err = stv06xx_write_bridge(sd, 0x143f, 0x01);
-
-	gspca_dev->usb_err = err;
-}
-
-static void st6422_set_contrast(struct gspca_dev *gspca_dev)
-{
-	int err;
-	struct sd *sd = (struct sd *) gspca_dev;
-
-	err = setcontrast(sd);
-
-	/* commit settings */
-	if (err >= 0)
-		err = stv06xx_write_bridge(sd, 0x143f, 0x01);
-
-	gspca_dev->usb_err = err;
-}
-
-static void st6422_set_gain(struct gspca_dev *gspca_dev)
-{
-	int err;
-	struct sd *sd = (struct sd *) gspca_dev;
-
-	err = setgain(sd);
-
-	/* commit settings */
-	if (err >= 0)
-		err = stv06xx_write_bridge(sd, 0x143f, 0x01);
-
-	gspca_dev->usb_err = err;
-}
-
-static void st6422_set_exposure(struct gspca_dev *gspca_dev)
-{
-	int err;
-	struct sd *sd = (struct sd *) gspca_dev;
-
-	err = setexposure(sd);
-
-	/* commit settings */
-	if (err >= 0)
-		err = stv06xx_write_bridge(sd, 0x143f, 0x01);
-
-	gspca_dev->usb_err = err;
-}
diff --git a/drivers/media/video/gspca/stv06xx/stv06xx_st6422.h b/drivers/media/video/gspca/stv06xx/stv06xx_st6422.h
index d7498e0..8f20fbf 100644
--- a/drivers/media/video/gspca/stv06xx/stv06xx_st6422.h
+++ b/drivers/media/video/gspca/stv06xx/stv06xx_st6422.h
@@ -34,8 +34,8 @@
 static int st6422_probe(struct sd *sd);
 static int st6422_start(struct sd *sd);
 static int st6422_init(struct sd *sd);
+static int st6422_init_controls(struct sd *sd);
 static int st6422_stop(struct sd *sd);
-static void st6422_disconnect(struct sd *sd);
 
 const struct stv06xx_sensor stv06xx_sensor_st6422 = {
 	.name = "ST6422",
@@ -43,10 +43,10 @@ const struct stv06xx_sensor stv06xx_sensor_st6422 = {
 	.min_packet_size = { 300, 847 },
 	.max_packet_size = { 300, 847 },
 	.init = st6422_init,
+	.init_controls = st6422_init_controls,
 	.probe = st6422_probe,
 	.start = st6422_start,
 	.stop = st6422_stop,
-	.disconnect = st6422_disconnect,
 };
 
 #endif
diff --git a/drivers/media/video/gspca/stv06xx/stv06xx_vv6410.c b/drivers/media/video/gspca/stv06xx/stv06xx_vv6410.c
index a5c69d9..1b7a68a 100644
--- a/drivers/media/video/gspca/stv06xx/stv06xx_vv6410.c
+++ b/drivers/media/video/gspca/stv06xx/stv06xx_vv6410.c
@@ -44,130 +44,82 @@ static struct v4l2_pix_format vv6410_mode[] = {
 	}
 };
 
-static const struct ctrl vv6410_ctrl[] = {
-#define HFLIP_IDX 0
-	{
-		{
-			.id		= V4L2_CID_HFLIP,
-			.type		= V4L2_CTRL_TYPE_BOOLEAN,
-			.name		= "horizontal flip",
-			.minimum	= 0,
-			.maximum	= 1,
-			.step		= 1,
-			.default_value	= 0
-		},
-		.set = vv6410_set_hflip,
-		.get = vv6410_get_hflip
-	},
-#define VFLIP_IDX 1
-	{
-		{
-			.id		= V4L2_CID_VFLIP,
-			.type		= V4L2_CTRL_TYPE_BOOLEAN,
-			.name		= "vertical flip",
-			.minimum	= 0,
-			.maximum	= 1,
-			.step		= 1,
-			.default_value	= 0
-		},
-		.set = vv6410_set_vflip,
-		.get = vv6410_get_vflip
-	},
-#define GAIN_IDX 2
-	{
-		{
-			.id		= V4L2_CID_GAIN,
-			.type		= V4L2_CTRL_TYPE_INTEGER,
-			.name		= "analog gain",
-			.minimum	= 0,
-			.maximum	= 15,
-			.step		= 1,
-			.default_value  = 10
-		},
-		.set = vv6410_set_analog_gain,
-		.get = vv6410_get_analog_gain
-	},
-#define EXPOSURE_IDX 3
-	{
-		{
-			.id		= V4L2_CID_EXPOSURE,
-			.type		= V4L2_CTRL_TYPE_INTEGER,
-			.name		= "exposure",
-			.minimum	= 0,
-			.maximum	= 32768,
-			.step		= 1,
-			.default_value  = 20000
-		},
-		.set = vv6410_set_exposure,
-		.get = vv6410_get_exposure
+static int vv6410_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct sd *sd = container_of(ctrl->handler, struct sd, ctrl_handler);
+	int err = -EINVAL;
+
+	switch (ctrl->id) {
+	case V4L2_CID_HFLIP:
+		err = vv6410_set_hflip(&sd->gspca_dev, ctrl->val);
+		break;
+	case V4L2_CID_VFLIP:
+		err = vv6410_set_vflip(&sd->gspca_dev, ctrl->val);
+		break;
+	case V4L2_CID_GAIN:
+		err = vv6410_set_analog_gain(&sd->gspca_dev, ctrl->val);
+		break;
+	case V4L2_CID_EXPOSURE:
+		err = vv6410_set_exposure(&sd->gspca_dev, ctrl->val);
+		break;
 	}
-	};
+	return err;
+}
+
+static const struct v4l2_ctrl_ops vv6410_ctrl_ops = {
+	.s_ctrl = vv6410_s_ctrl,
+};
 
 static int vv6410_probe(struct sd *sd)
 {
 	u16 data;
-	int err, i;
-	s32 *sensor_settings;
+	int err;
 
 	err = stv06xx_read_sensor(sd, VV6410_DEVICEH, &data);
 	if (err < 0)
 		return -ENODEV;
 
-	if (data == 0x19) {
-		pr_info("vv6410 sensor detected\n");
+	if (data != 0x19)
+		return -ENODEV;
 
-		sensor_settings = kmalloc(ARRAY_SIZE(vv6410_ctrl) * sizeof(s32),
-					  GFP_KERNEL);
-		if (!sensor_settings)
-			return -ENOMEM;
+	pr_info("vv6410 sensor detected\n");
 
-		sd->gspca_dev.cam.cam_mode = vv6410_mode;
-		sd->gspca_dev.cam.nmodes = ARRAY_SIZE(vv6410_mode);
-		sd->desc.ctrls = vv6410_ctrl;
-		sd->desc.nctrls = ARRAY_SIZE(vv6410_ctrl);
+	sd->gspca_dev.cam.cam_mode = vv6410_mode;
+	sd->gspca_dev.cam.nmodes = ARRAY_SIZE(vv6410_mode);
+	return 0;
+}
 
-		for (i = 0; i < sd->desc.nctrls; i++)
-			sensor_settings[i] = vv6410_ctrl[i].qctrl.default_value;
-		sd->sensor_priv = sensor_settings;
-		return 0;
-	}
-	return -ENODEV;
+static int vv6410_init_controls(struct sd *sd)
+{
+	struct v4l2_ctrl_handler *hdl = &sd->ctrl_handler;
+
+	v4l2_ctrl_handler_init(hdl, 4);
+	v4l2_ctrl_new_std(hdl, &vv6410_ctrl_ops,
+			V4L2_CID_HFLIP, 0, 1, 1, 0);
+	v4l2_ctrl_new_std(hdl, &vv6410_ctrl_ops,
+			V4L2_CID_VFLIP, 0, 1, 1, 0);
+	v4l2_ctrl_new_std(hdl, &vv6410_ctrl_ops,
+			V4L2_CID_EXPOSURE, 0, 32768, 1, 20000);
+	v4l2_ctrl_new_std(hdl, &vv6410_ctrl_ops,
+			V4L2_CID_GAIN, 0, 15, 1, 10);
+	return hdl->error;
 }
 
 static int vv6410_init(struct sd *sd)
 {
 	int err = 0, i;
-	s32 *sensor_settings = sd->sensor_priv;
 
-	for (i = 0; i < ARRAY_SIZE(stv_bridge_init); i++) {
+	for (i = 0; i < ARRAY_SIZE(stv_bridge_init); i++)
 		stv06xx_write_bridge(sd, stv_bridge_init[i].addr, stv_bridge_init[i].data);
-	}
 
 	if (err < 0)
 		return err;
 
 	err = stv06xx_write_sensor_bytes(sd, (u8 *) vv6410_sensor_init,
 					 ARRAY_SIZE(vv6410_sensor_init));
-	if (err < 0)
-		return err;
-
-	err = vv6410_set_exposure(&sd->gspca_dev,
-				   sensor_settings[EXPOSURE_IDX]);
-	if (err < 0)
-		return err;
-
-	err = vv6410_set_analog_gain(&sd->gspca_dev,
-				      sensor_settings[GAIN_IDX]);
-
 	return (err < 0) ? err : 0;
 }
 
-static void vv6410_disconnect(struct sd *sd)
-{
-	sd->sensor = NULL;
-	kfree(sd->sensor_priv);
-}
-
 static int vv6410_start(struct sd *sd)
 {
 	int err;
@@ -233,25 +185,12 @@ static int vv6410_dump(struct sd *sd)
 	return (err < 0) ? err : 0;
 }
 
-static int vv6410_get_hflip(struct gspca_dev *gspca_dev, __s32 *val)
-{
-	struct sd *sd = (struct sd *) gspca_dev;
-	s32 *sensor_settings = sd->sensor_priv;
-
-	*val = sensor_settings[HFLIP_IDX];
-	PDEBUG(D_V4L2, "Read horizontal flip %d", *val);
-
-	return 0;
-}
-
 static int vv6410_set_hflip(struct gspca_dev *gspca_dev, __s32 val)
 {
 	int err;
 	u16 i2c_data;
 	struct sd *sd = (struct sd *) gspca_dev;
-	s32 *sensor_settings = sd->sensor_priv;
 
-	sensor_settings[HFLIP_IDX] = val;
 	err = stv06xx_read_sensor(sd, VV6410_DATAFORMAT, &i2c_data);
 	if (err < 0)
 		return err;
@@ -267,25 +206,12 @@ static int vv6410_set_hflip(struct gspca_dev *gspca_dev, __s32 val)
 	return (err < 0) ? err : 0;
 }
 
-static int vv6410_get_vflip(struct gspca_dev *gspca_dev, __s32 *val)
-{
-	struct sd *sd = (struct sd *) gspca_dev;
-	s32 *sensor_settings = sd->sensor_priv;
-
-	*val = sensor_settings[VFLIP_IDX];
-	PDEBUG(D_V4L2, "Read vertical flip %d", *val);
-
-	return 0;
-}
-
 static int vv6410_set_vflip(struct gspca_dev *gspca_dev, __s32 val)
 {
 	int err;
 	u16 i2c_data;
 	struct sd *sd = (struct sd *) gspca_dev;
-	s32 *sensor_settings = sd->sensor_priv;
 
-	sensor_settings[VFLIP_IDX] = val;
 	err = stv06xx_read_sensor(sd, VV6410_DATAFORMAT, &i2c_data);
 	if (err < 0)
 		return err;
@@ -301,52 +227,23 @@ static int vv6410_set_vflip(struct gspca_dev *gspca_dev, __s32 val)
 	return (err < 0) ? err : 0;
 }
 
-static int vv6410_get_analog_gain(struct gspca_dev *gspca_dev, __s32 *val)
-{
-	struct sd *sd = (struct sd *) gspca_dev;
-	s32 *sensor_settings = sd->sensor_priv;
-
-	*val = sensor_settings[GAIN_IDX];
-
-	PDEBUG(D_V4L2, "Read analog gain %d", *val);
-
-	return 0;
-}
-
 static int vv6410_set_analog_gain(struct gspca_dev *gspca_dev, __s32 val)
 {
 	int err;
 	struct sd *sd = (struct sd *) gspca_dev;
-	s32 *sensor_settings = sd->sensor_priv;
 
-	sensor_settings[GAIN_IDX] = val;
 	PDEBUG(D_V4L2, "Set analog gain to %d", val);
 	err = stv06xx_write_sensor(sd, VV6410_ANALOGGAIN, 0xf0 | (val & 0xf));
 
 	return (err < 0) ? err : 0;
 }
 
-static int vv6410_get_exposure(struct gspca_dev *gspca_dev, __s32 *val)
-{
-	struct sd *sd = (struct sd *) gspca_dev;
-	s32 *sensor_settings = sd->sensor_priv;
-
-	*val = sensor_settings[EXPOSURE_IDX];
-
-	PDEBUG(D_V4L2, "Read exposure %d", *val);
-
-	return 0;
-}
-
 static int vv6410_set_exposure(struct gspca_dev *gspca_dev, __s32 val)
 {
 	int err;
 	struct sd *sd = (struct sd *) gspca_dev;
-	s32 *sensor_settings = sd->sensor_priv;
 	unsigned int fine, coarse;
 
-	sensor_settings[EXPOSURE_IDX] = val;
-
 	val = (val * val >> 14) + val / 4;
 
 	fine = val % VV6410_CIF_LINELENGTH;
diff --git a/drivers/media/video/gspca/stv06xx/stv06xx_vv6410.h b/drivers/media/video/gspca/stv06xx/stv06xx_vv6410.h
index a25b887..53e67b4 100644
--- a/drivers/media/video/gspca/stv06xx/stv06xx_vv6410.h
+++ b/drivers/media/video/gspca/stv06xx/stv06xx_vv6410.h
@@ -178,18 +178,14 @@
 static int vv6410_probe(struct sd *sd);
 static int vv6410_start(struct sd *sd);
 static int vv6410_init(struct sd *sd);
+static int vv6410_init_controls(struct sd *sd);
 static int vv6410_stop(struct sd *sd);
 static int vv6410_dump(struct sd *sd);
-static void vv6410_disconnect(struct sd *sd);
 
 /* V4L2 controls supported by the driver */
-static int vv6410_get_hflip(struct gspca_dev *gspca_dev, __s32 *val);
 static int vv6410_set_hflip(struct gspca_dev *gspca_dev, __s32 val);
-static int vv6410_get_vflip(struct gspca_dev *gspca_dev, __s32 *val);
 static int vv6410_set_vflip(struct gspca_dev *gspca_dev, __s32 val);
-static int vv6410_get_analog_gain(struct gspca_dev *gspca_dev, __s32 *val);
 static int vv6410_set_analog_gain(struct gspca_dev *gspca_dev, __s32 val);
-static int vv6410_get_exposure(struct gspca_dev *gspca_dev, __s32 *val);
 static int vv6410_set_exposure(struct gspca_dev *gspca_dev, __s32 val);
 
 const struct stv06xx_sensor stv06xx_sensor_vv6410 = {
@@ -202,11 +198,11 @@ const struct stv06xx_sensor stv06xx_sensor_vv6410 = {
 	.min_packet_size = { 1023 },
 	.max_packet_size = { 1023 },
 	.init = vv6410_init,
+	.init_controls = vv6410_init_controls,
 	.probe = vv6410_probe,
 	.start = vv6410_start,
 	.stop = vv6410_stop,
 	.dump = vv6410_dump,
-	.disconnect = vv6410_disconnect,
 };
 
 /* If NULL, only single value to write, stored in len */
-- 
1.7.10

[RFCv2 PATCH 17/17] gspca-mars: 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/mars.c |  292 ++++++++++++++++----------------------
 1 file changed, 126 insertions(+), 166 deletions(-)

diff --git a/drivers/media/video/gspca/mars.c b/drivers/media/video/gspca/mars.c
index b023146..b1a1fec 100644
--- a/drivers/media/video/gspca/mars.c
+++ b/drivers/media/video/gspca/mars.c
@@ -25,27 +25,26 @@
 
 #include "gspca.h"
 #include "jpeg.h"
+#include <media/v4l2-ctrls.h>
 
 MODULE_AUTHOR("Michel Xhaard <mxhaard@users.sourceforge.net>");
 MODULE_DESCRIPTION("GSPCA/Mars USB Camera Driver");
 MODULE_LICENSE("GPL");
 
-/* controls */
-enum e_ctrl {
-	BRIGHTNESS,
-	COLORS,
-	GAMMA,
-	SHARPNESS,
-	ILLUM_TOP,
-	ILLUM_BOT,
-	NCTRLS		/* number of controls */
-};
-
 /* 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 v4l2_ctrl *brightness;
+	struct v4l2_ctrl *saturation;
+	struct v4l2_ctrl *sharpness;
+	struct v4l2_ctrl *gamma;
+	struct { /* illuminator control cluster */
+		struct v4l2_ctrl *illum_top;
+		struct v4l2_ctrl *illum_bottom;
+	};
+	struct v4l2_ctrl *jpegqual;
 
 	u8 quality;
 #define QUALITY_MIN 40
@@ -56,89 +55,10 @@ struct sd {
 };
 
 /* V4L2 controls supported by the driver */
-static void setbrightness(struct gspca_dev *gspca_dev);
-static void setcolors(struct gspca_dev *gspca_dev);
-static void setgamma(struct gspca_dev *gspca_dev);
-static void setsharpness(struct gspca_dev *gspca_dev);
-static int sd_setilluminator1(struct gspca_dev *gspca_dev, __s32 val);
-static int sd_setilluminator2(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 = 30,
-		.step    = 1,
-		.default_value = 15,
-	    },
-	    .set_control = setbrightness
-	},
-[COLORS] = {
-	    {
-		.id      = V4L2_CID_SATURATION,
-		.type    = V4L2_CTRL_TYPE_INTEGER,
-		.name    = "Color",
-		.minimum = 1,
-		.maximum = 255,
-		.step    = 1,
-		.default_value = 200,
-	    },
-	    .set_control = setcolors
-	},
-[GAMMA] = {
-	    {
-		.id      = V4L2_CID_GAMMA,
-		.type    = V4L2_CTRL_TYPE_INTEGER,
-		.name    = "Gamma",
-		.minimum = 0,
-		.maximum = 3,
-		.step    = 1,
-		.default_value = 1,
-	    },
-	    .set_control = setgamma
-	},
-[SHARPNESS] = {
-	    {
-		.id	 = V4L2_CID_SHARPNESS,
-		.type    = V4L2_CTRL_TYPE_INTEGER,
-		.name    = "Sharpness",
-		.minimum = 0,
-		.maximum = 2,
-		.step    = 1,
-		.default_value = 1,
-	    },
-	    .set_control = setsharpness
-	},
-[ILLUM_TOP] = {
-	    {
-		.id	 = V4L2_CID_ILLUMINATORS_1,
-		.type    = V4L2_CTRL_TYPE_BOOLEAN,
-		.name    = "Top illuminator",
-		.minimum = 0,
-		.maximum = 1,
-		.step    = 1,
-		.default_value = 0,
-		.flags = V4L2_CTRL_FLAG_UPDATE,
-	    },
-	    .set = sd_setilluminator1
-	},
-[ILLUM_BOT] = {
-	    {
-		.id	 = V4L2_CID_ILLUMINATORS_2,
-		.type    = V4L2_CTRL_TYPE_BOOLEAN,
-		.name    = "Bottom illuminator",
-		.minimum = 0,
-		.maximum = 1,
-		.step    = 1,
-		.default_value = 0,
-		.flags = V4L2_CTRL_FLAG_UPDATE,
-	    },
-	    .set = sd_setilluminator2
-	},
-};
+static void setbrightness(struct gspca_dev *gspca_dev, s32 val);
+static void setcolors(struct gspca_dev *gspca_dev, s32 val);
+static void setgamma(struct gspca_dev *gspca_dev, s32 val);
+static void setsharpness(struct gspca_dev *gspca_dev, s32 val);
 
 static const struct v4l2_pix_format vga_mode[] = {
 	{320, 240, V4L2_PIX_FMT_JPEG, V4L2_FIELD_NONE,
@@ -198,59 +118,129 @@ static void mi_w(struct gspca_dev *gspca_dev,
 	reg_w(gspca_dev, 4);
 }
 
-static void setbrightness(struct gspca_dev *gspca_dev)
+static void setbrightness(struct gspca_dev *gspca_dev, s32 val)
 {
-	struct sd *sd = (struct sd *) gspca_dev;
-
 	gspca_dev->usb_buf[0] = 0x61;
-	gspca_dev->usb_buf[1] = sd->ctrls[BRIGHTNESS].val;
+	gspca_dev->usb_buf[1] = val;
 	reg_w(gspca_dev, 2);
 }
 
-static void setcolors(struct gspca_dev *gspca_dev)
+static void setcolors(struct gspca_dev *gspca_dev, s32 val)
 {
-	struct sd *sd = (struct sd *) gspca_dev;
-	s16 val;
-
-	val = sd->ctrls[COLORS].val;
 	gspca_dev->usb_buf[0] = 0x5f;
 	gspca_dev->usb_buf[1] = val << 3;
 	gspca_dev->usb_buf[2] = ((val >> 2) & 0xf8) | 0x04;
 	reg_w(gspca_dev, 3);
 }
 
-static void setgamma(struct gspca_dev *gspca_dev)
+static void setgamma(struct gspca_dev *gspca_dev, s32 val)
 {
-	struct sd *sd = (struct sd *) gspca_dev;
-
 	gspca_dev->usb_buf[0] = 0x06;
-	gspca_dev->usb_buf[1] = sd->ctrls[GAMMA].val * 0x40;
+	gspca_dev->usb_buf[1] = val * 0x40;
 	reg_w(gspca_dev, 2);
 }
 
-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;
-
 	gspca_dev->usb_buf[0] = 0x67;
-	gspca_dev->usb_buf[1] = sd->ctrls[SHARPNESS].val * 4 + 3;
+	gspca_dev->usb_buf[1] = val * 4 + 3;
 	reg_w(gspca_dev, 2);
 }
 
-static void setilluminators(struct gspca_dev *gspca_dev)
+static void setilluminators(struct gspca_dev *gspca_dev, bool top, bool bottom)
 {
-	struct sd *sd = (struct sd *) gspca_dev;
-
+	/* both are off if not streaming */
 	gspca_dev->usb_buf[0] = 0x22;
-	if (sd->ctrls[ILLUM_TOP].val)
+	if (top)
 		gspca_dev->usb_buf[1] = 0x76;
-	else if (sd->ctrls[ILLUM_BOT].val)
+	else if (bottom)
 		gspca_dev->usb_buf[1] = 0x7a;
 	else
 		gspca_dev->usb_buf[1] = 0x7e;
 	reg_w(gspca_dev, 2);
 }
 
+static int mars_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;
+
+	gspca_dev->usb_err = 0;
+
+	if (ctrl->id == V4L2_CID_ILLUMINATORS_1) {
+		/* only one can be on at a time */
+		if (ctrl->is_new && ctrl->val)
+			sd->illum_bottom->val = 0;
+		if (sd->illum_bottom->is_new && sd->illum_bottom->val)
+			sd->illum_top->val = 0;
+	}
+
+	if (!gspca_dev->streaming)
+		return 0;
+
+	switch (ctrl->id) {
+	case V4L2_CID_BRIGHTNESS:
+		setbrightness(&sd->gspca_dev, ctrl->val);
+		break;
+	case V4L2_CID_SATURATION:
+		setcolors(&sd->gspca_dev, ctrl->val);
+		break;
+	case V4L2_CID_GAMMA:
+		setgamma(&sd->gspca_dev, ctrl->val);
+		break;
+	case V4L2_CID_ILLUMINATORS_1:
+		setilluminators(&sd->gspca_dev, sd->illum_top->val,
+						sd->illum_bottom->val);
+		break;
+	case V4L2_CID_SHARPNESS:
+		setsharpness(&sd->gspca_dev, ctrl->val);
+		break;
+	case V4L2_CID_JPEG_COMPRESSION_QUALITY:
+		jpeg_set_qual(sd->jpeg_hdr, ctrl->val);
+		break;
+	default:
+		return -EINVAL;
+	}
+	return gspca_dev->usb_err;
+}
+
+static const struct v4l2_ctrl_ops mars_ctrl_ops = {
+	.s_ctrl = mars_s_ctrl,
+};
+
+/* this function is called at probe time */
+static int sd_init_controls(struct gspca_dev *gspca_dev)
+{
+	struct sd *sd = (struct sd *) gspca_dev;
+	struct v4l2_ctrl_handler *hdl = &sd->ctrl_handler;
+
+	gspca_dev->vdev.ctrl_handler = hdl;
+	v4l2_ctrl_handler_init(hdl, 7);
+	sd->brightness = v4l2_ctrl_new_std(hdl, &mars_ctrl_ops,
+			V4L2_CID_BRIGHTNESS, 0, 30, 1, 15);
+	sd->saturation = v4l2_ctrl_new_std(hdl, &mars_ctrl_ops,
+			V4L2_CID_SATURATION, 0, 255, 1, 200);
+	sd->gamma = v4l2_ctrl_new_std(hdl, &mars_ctrl_ops,
+			V4L2_CID_GAMMA, 0, 3, 1, 1);
+	sd->sharpness = v4l2_ctrl_new_std(hdl, &mars_ctrl_ops,
+			V4L2_CID_SHARPNESS, 0, 2, 1, 1);
+	sd->illum_top = v4l2_ctrl_new_std(hdl, &mars_ctrl_ops,
+			V4L2_CID_ILLUMINATORS_1, 0, 1, 1, 0);
+	sd->illum_top->flags |= V4L2_CTRL_FLAG_UPDATE;
+	sd->illum_bottom = v4l2_ctrl_new_std(hdl, &mars_ctrl_ops,
+			V4L2_CID_ILLUMINATORS_2, 0, 1, 1, 0);
+	sd->illum_bottom->flags |= V4L2_CTRL_FLAG_UPDATE;
+	sd->jpegqual = v4l2_ctrl_new_std(hdl, &mars_ctrl_ops,
+			V4L2_CID_JPEG_COMPRESSION_QUALITY,
+			QUALITY_MIN, QUALITY_MAX, 1, QUALITY_DEF);
+	if (hdl->error) {
+		pr_err("Could not initialize controls\n");
+		return hdl->error;
+	}
+	v4l2_ctrl_cluster(2, &sd->illum_top);
+	return 0;
+}
+
 /* this function is called at probe time */
 static int sd_config(struct gspca_dev *gspca_dev,
 			const struct usb_device_id *id)
@@ -261,7 +251,6 @@ static int sd_config(struct gspca_dev *gspca_dev,
 	cam = &gspca_dev->cam;
 	cam->cam_mode = vga_mode;
 	cam->nmodes = ARRAY_SIZE(vga_mode);
-	cam->ctrls = sd->ctrls;
 	sd->quality = QUALITY_DEF;
 	return 0;
 }
@@ -269,7 +258,6 @@ static int sd_config(struct gspca_dev *gspca_dev,
 /* this function is called at probe and resume time */
 static int sd_init(struct gspca_dev *gspca_dev)
 {
-	gspca_dev->ctrl_inac = (1 << ILLUM_TOP) | (1 << ILLUM_BOT);
 	return 0;
 }
 
@@ -282,7 +270,7 @@ static int sd_start(struct gspca_dev *gspca_dev)
 	/* create the JPEG header */
 	jpeg_define(sd->jpeg_hdr, gspca_dev->height, gspca_dev->width,
 			0x21);		/* JPEG 422 */
-	jpeg_set_qual(sd->jpeg_hdr, sd->quality);
+	jpeg_set_qual(sd->jpeg_hdr, v4l2_ctrl_g_ctrl(sd->jpegqual));
 
 	data = gspca_dev->usb_buf;
 
@@ -301,7 +289,7 @@ static int sd_start(struct gspca_dev *gspca_dev)
 	data[5] = 0x30;		/* reg 4, MI, PAS5101 :
 				 *	0x30 for 24mhz , 0x28 for 12mhz */
 	data[6] = 0x02;		/* reg 5, H start - was 0x04 */
-	data[7] = sd->ctrls[GAMMA].val * 0x40;	/* reg 0x06: gamma */
+	data[7] = v4l2_ctrl_g_ctrl(sd->gamma) * 0x40;	/* reg 0x06: gamma */
 	data[8] = 0x01;		/* reg 7, V start - was 0x03 */
 /*	if (h_size == 320 ) */
 /*		data[9]= 0x56;	 * reg 8, 24MHz, 2:1 scale down */
@@ -333,16 +321,16 @@ static int sd_start(struct gspca_dev *gspca_dev)
 				/* reg 0x5f/0x60 (LE) = saturation */
 				/* h (60): xxxx x100
 				 * l (5f): xxxx x000 */
-	data[2] = sd->ctrls[COLORS].val << 3;
-	data[3] = ((sd->ctrls[COLORS].val >> 2) & 0xf8) | 0x04;
-	data[4] = sd->ctrls[BRIGHTNESS].val; /* reg 0x61 = brightness */
+	data[2] = v4l2_ctrl_g_ctrl(sd->saturation) << 3;
+	data[3] = ((v4l2_ctrl_g_ctrl(sd->saturation) >> 2) & 0xf8) | 0x04;
+	data[4] = v4l2_ctrl_g_ctrl(sd->brightness); /* reg 0x61 = brightness */
 	data[5] = 0x00;
 
 	reg_w(gspca_dev, 6);
 
 	data[0] = 0x67;
 /*jfm: from win trace*/
-	data[1] = sd->ctrls[SHARPNESS].val * 4 + 3;
+	data[1] = v4l2_ctrl_g_ctrl(sd->sharpness) * 4 + 3;
 	data[2] = 0x14;
 	reg_w(gspca_dev, 3);
 
@@ -365,7 +353,9 @@ static int sd_start(struct gspca_dev *gspca_dev)
 	data[1] = 0x4d;		/* ISOC transferring enable... */
 	reg_w(gspca_dev, 2);
 
-	gspca_dev->ctrl_inac = 0; /* activate the illuminator controls */
+	setilluminators(gspca_dev, v4l2_ctrl_g_ctrl(sd->illum_top),
+				   v4l2_ctrl_g_ctrl(sd->illum_bottom));
+
 	return gspca_dev->usb_err;
 }
 
@@ -373,11 +363,8 @@ static void sd_stopN(struct gspca_dev *gspca_dev)
 {
 	struct sd *sd = (struct sd *) gspca_dev;
 
-	gspca_dev->ctrl_inac = (1 << ILLUM_TOP) | (1 << ILLUM_BOT);
-	if (sd->ctrls[ILLUM_TOP].val || sd->ctrls[ILLUM_BOT].val) {
-		sd->ctrls[ILLUM_TOP].val = 0;
-		sd->ctrls[ILLUM_BOT].val = 0;
-		setilluminators(gspca_dev);
+	if (v4l2_ctrl_g_ctrl(sd->illum_top) || v4l2_ctrl_g_ctrl(sd->illum_bottom)) {
+		setilluminators(gspca_dev, false, false);
 		msleep(20);
 	}
 
@@ -424,43 +411,16 @@ static void sd_pkt_scan(struct gspca_dev *gspca_dev,
 	gspca_frame_add(gspca_dev, INTER_PACKET, data, len);
 }
 
-static int sd_setilluminator1(struct gspca_dev *gspca_dev, __s32 val)
-{
-	struct sd *sd = (struct sd *) gspca_dev;
-
-	/* only one illuminator may be on */
-	sd->ctrls[ILLUM_TOP].val = val;
-	if (val)
-		sd->ctrls[ILLUM_BOT].val = 0;
-	setilluminators(gspca_dev);
-	return gspca_dev->usb_err;
-}
-
-static int sd_setilluminator2(struct gspca_dev *gspca_dev, __s32 val)
-{
-	struct sd *sd = (struct sd *) gspca_dev;
-
-	/* only one illuminator may be on */
-	sd->ctrls[ILLUM_BOT].val = val;
-	if (val)
-		sd->ctrls[ILLUM_TOP].val = 0;
-	setilluminators(gspca_dev);
-	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;
+	int ret;
 
-	if (jcomp->quality < QUALITY_MIN)
-		sd->quality = QUALITY_MIN;
-	else if (jcomp->quality > QUALITY_MAX)
-		sd->quality = QUALITY_MAX;
-	else
-		sd->quality = jcomp->quality;
-	if (gspca_dev->streaming)
-		jpeg_set_qual(sd->jpeg_hdr, sd->quality);
+	ret = v4l2_ctrl_s_ctrl(sd->jpegqual, jcomp->quality);
+	if (ret)
+		return ret;
+	jcomp->quality = v4l2_ctrl_g_ctrl(sd->jpegqual);
 	return 0;
 }
 
@@ -470,7 +430,7 @@ static int sd_get_jcomp(struct gspca_dev *gspca_dev,
 	struct sd *sd = (struct sd *) gspca_dev;
 
 	memset(jcomp, 0, sizeof *jcomp);
-	jcomp->quality = sd->quality;
+	jcomp->quality = v4l2_ctrl_g_ctrl(sd->jpegqual);
 	jcomp->jpeg_markers = V4L2_JPEG_MARKER_DHT
 			| V4L2_JPEG_MARKER_DQT;
 	return 0;
@@ -479,10 +439,9 @@ static int sd_get_jcomp(struct gspca_dev *gspca_dev,
 /* sub-driver description */
 static const struct sd_desc sd_desc = {
 	.name = MODULE_NAME,
-	.ctrls = sd_ctrls,
-	.nctrls = NCTRLS,
 	.config = sd_config,
 	.init = sd_init,
+	.init_controls = sd_init_controls,
 	.start = sd_start,
 	.stopN = sd_stopN,
 	.pkt_scan = sd_pkt_scan,
@@ -513,6 +472,7 @@ static struct usb_driver sd_driver = {
 #ifdef CONFIG_PM
 	.suspend = gspca_suspend,
 	.resume = gspca_resume,
+	.reset_resume = gspca_resume,
 #endif
 };
 
-- 
1.7.10

Re: [RFCv2 PATCH 13/17] gspca: switch to V4L2 core locking, except for the buffer queuing ioctls.

[Hans de Goede]

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

Hi Hans,

The entire series looks great, I do have a few remarks wrt this
patch, which I have fixed in my own tree (new version attached,
note untested sofar).

On 05/06/2012 02:28 PM, Hans Verkuil wrote:
> From: Hans Verkuil<hans.verkuil@cisco.com>
>
> Due to latency concerns the VIDIOC_QBUF, DQBUF and QUERYBUF do not use the
> core lock, instead they rely only on queue_lock.
>
> Signed-off-by: Hans Verkuil<hans.verkuil@cisco.com>
> ---
>   drivers/media/video/gspca/gspca.c |  203 ++++++++-----------------------------
>   1 file changed, 41 insertions(+), 162 deletions(-)
>
> diff --git a/drivers/media/video/gspca/gspca.c b/drivers/media/video/gspca/gspca.c
> index f840bed..edca4f3 100644
> --- a/drivers/media/video/gspca/gspca.c
> +++ b/drivers/media/video/gspca/gspca.c
> @@ -850,14 +850,6 @@ static int gspca_init_transfer(struct gspca_dev *gspca_dev)
>   	struct ep_tb_s ep_tb[MAX_ALT];
>   	int n, ret, xfer, alt, alt_idx;
>
> -	if (mutex_lock_interruptible(&gspca_dev->usb_lock))
> -		return -ERESTARTSYS;
> -
> -	if (!gspca_dev->present) {
> -		ret = -ENODEV;
> -		goto unlock;
> -	}
> -
>   	/* reset the streaming variables */
>   	gspca_dev->image = NULL;
>   	gspca_dev->image_len = 0;

You're removing a lot of checks for gspca_dev->present, relying on the
video_is_registered checks in v4l2-dev instead I assume, this is a good
idea, *but* it requires a small hack in disconnect to close a race.

Currently the end of gspca_disconnect looks like this:

         gspca_dev->dev = NULL;
         v4l2_device_disconnect(&gspca_dev->v4l2_dev);
         mutex_unlock(&gspca_dev->usb_lock);

         usb_set_intfdata(intf, NULL);

         /* release the device */
         /* (this will call gspca_release() immediately or on last close) */
         video_unregister_device(&gspca_dev->vdev);
}

Notice that usb_lock is unlocked before video_unregister_device gets called,
which means that any ioctl or other fops waiting for usb_lock can run
before video_unregister_device runs, and thus before they are protected
against being called on an disconnected device by the
video_is_registered checks in v4l2-dev.

Unfortunately simply moving the unlock down won't work, because if there
are no open file handles referencing the device, then the memory
referenced by gspca_dev will be free-ed after the video_unregister_device
call.

So I've changed disconnect to the following in my version, to allow the
present check removal you've did, as I quite like being able to
remove all those present checks :)   :

         /* The USB-interface device is freed at exit of this function */
         gspca_dev->dev = NULL;
         v4l2_device_disconnect(&gspca_dev->v4l2_dev);

         /* Ensure gspca_dev sticks around for the usb_lock unlock! */
         get_device(&gspca_dev->vdev.dev);
         video_unregister_device(&gspca_dev->vdev);
         mutex_unlock(&gspca_dev->usb_lock);
         /* (this will call gspca_release() immediately or on last close) */
         put_device(&gspca_dev->vdev.dev);

         usb_set_intfdata(intf, NULL);
}


<snip chunks on which I've no comments>

> @@ -1736,10 +1658,8 @@ static int vidioc_streamoff(struct file *file, void *priv,
>   	if (mutex_lock_interruptible(&gspca_dev->queue_lock))
>   		return -ERESTARTSYS;
>
> -	if (!gspca_dev->streaming) {
> -		ret = 0;
> -		goto out;
> -	}
> +	if (!gspca_dev->streaming)
> +		return 0;
>

BAD! queue_lock is held here, so you cannot just change this to a return!

>   	/* check the capture file */
>   	if (gspca_dev->capt_file != file) {

<snip chunks on which I've no comments>

> @@ -2009,11 +1883,9 @@ static int vidioc_dqbuf(struct file *file, void *priv,
>   	gspca_dev->fr_o = (i + 1) % GSPCA_MAX_FRAMES;
>
>   	if (gspca_dev->sd_desc->dq_callback) {
> -		mutex_lock(&gspca_dev->usb_lock);
>   		gspca_dev->usb_err = 0;
>   		if (gspca_dev->present)
>   			gspca_dev->sd_desc->dq_callback(gspca_dev);
> -		mutex_unlock(&gspca_dev->usb_lock);
>   	}
>
>   	frame->v4l2_buf.flags&= ~V4L2_BUF_FLAG_DONE;

You cannot remove the locking here, as dq_callback expects to be
called with the usb-lock locked.

Since usb-lock now is the device lock and thus gets locked before
the queue_lock, we cannot simply drop this chunk. Instead I've
moved the dq_callback to the end of vidioc_dqbuf, so after the
stream_lock has been released (there is no reason to have
the stream_lock hold when calling the dq_callback).

The dq_callback is used to do camera control adjustments which
need to be done after every X frames, and which cannot be done
from the isoc frame interrupts since they should not be done under
interrupt. When the drivers using dq_callback are converted to the
control framework, they will likely end up calling v4l2_ctrl_s_ctrl
from the dq_callback.

<snip chunks on which I've no comments>

Regards,

Hans


p.s.

I've yet to take a good look at the driver conversions other then
the zc3xx conversion.

[-- Attachment #2: 0013-gspca-switch-to-V4L2-core-locking-except-for-the-buf.patch --]
[-- Type: text/x-patch, Size: 15881 bytes --]

>From 46738bfafe910180d89d53cbe30a9fb4bd234a09 Mon Sep 17 00:00:00 2001
From: Hans Verkuil <hans.verkuil@cisco.com>
Date: Sun, 6 May 2012 14:28:27 +0200
Subject: [PATCH 13/17] gspca: switch to V4L2 core locking, except for the
 buffer queuing ioctls.

Due to latency concerns the VIDIOC_QBUF, DQBUF and QUERYBUF do not use the
core lock, instead they rely only on queue_lock.

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

diff --git a/drivers/media/video/gspca/gspca.c b/drivers/media/video/gspca/gspca.c
index f840bed..be31233 100644
--- a/drivers/media/video/gspca/gspca.c
+++ b/drivers/media/video/gspca/gspca.c
@@ -850,14 +850,6 @@ static int gspca_init_transfer(struct gspca_dev *gspca_dev)
 	struct ep_tb_s ep_tb[MAX_ALT];
 	int n, ret, xfer, alt, alt_idx;
 
-	if (mutex_lock_interruptible(&gspca_dev->usb_lock))
-		return -ERESTARTSYS;
-
-	if (!gspca_dev->present) {
-		ret = -ENODEV;
-		goto unlock;
-	}
-
 	/* reset the streaming variables */
 	gspca_dev->image = NULL;
 	gspca_dev->image_len = 0;
@@ -872,7 +864,7 @@ static int gspca_init_transfer(struct gspca_dev *gspca_dev)
 	if (gspca_dev->sd_desc->isoc_init) {
 		ret = gspca_dev->sd_desc->isoc_init(gspca_dev);
 		if (ret < 0)
-			goto unlock;
+			return ret;
 	}
 	xfer = gspca_dev->cam.bulk ? USB_ENDPOINT_XFER_BULK
 				   : USB_ENDPOINT_XFER_ISOC;
@@ -883,8 +875,7 @@ static int gspca_init_transfer(struct gspca_dev *gspca_dev)
 		ep = alt_xfer(&intf->altsetting[gspca_dev->alt], xfer);
 		if (ep == NULL) {
 			pr_err("bad altsetting %d\n", gspca_dev->alt);
-			ret = -EIO;
-			goto out;
+			return -EIO;
 		}
 		ep_tb[0].alt = gspca_dev->alt;
 		alt_idx = 1;
@@ -895,8 +886,7 @@ static int gspca_init_transfer(struct gspca_dev *gspca_dev)
 		alt_idx = build_isoc_ep_tb(gspca_dev, intf, ep_tb);
 		if (alt_idx <= 0) {
 			pr_err("no transfer endpoint found\n");
-			ret = -EIO;
-			goto unlock;
+			return -EIO;
 		}
 	}
 
@@ -991,8 +981,6 @@ retry:
 	}
 out:
 	gspca_input_create_urb(gspca_dev);
-unlock:
-	mutex_unlock(&gspca_dev->usb_lock);
 	return ret;
 }
 
@@ -1062,7 +1050,6 @@ static int gspca_get_mode(struct gspca_dev *gspca_dev,
 static int vidioc_g_register(struct file *file, void *priv,
 			struct v4l2_dbg_register *reg)
 {
-	int ret;
 	struct gspca_dev *gspca_dev = video_drvdata(file);
 
 	if (!gspca_dev->sd_desc->get_chip_ident)
@@ -1071,22 +1058,13 @@ static int vidioc_g_register(struct file *file, void *priv,
 	if (!gspca_dev->sd_desc->get_register)
 		return -ENOTTY;
 
-	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_register(gspca_dev, reg);
-	else
-		ret = -ENODEV;
-	mutex_unlock(&gspca_dev->usb_lock);
-
-	return ret;
+	return gspca_dev->sd_desc->get_register(gspca_dev, reg);
 }
 
 static int vidioc_s_register(struct file *file, void *priv,
 			struct v4l2_dbg_register *reg)
 {
-	int ret;
 	struct gspca_dev *gspca_dev = video_drvdata(file);
 
 	if (!gspca_dev->sd_desc->get_chip_ident)
@@ -1095,38 +1073,21 @@ static int vidioc_s_register(struct file *file, void *priv,
 	if (!gspca_dev->sd_desc->set_register)
 		return -ENOTTY;
 
-	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_register(gspca_dev, reg);
-	else
-		ret = -ENODEV;
-	mutex_unlock(&gspca_dev->usb_lock);
-
-	return ret;
+	return gspca_dev->sd_desc->set_register(gspca_dev, reg);
 }
 #endif
 
 static int vidioc_g_chip_ident(struct file *file, void *priv,
 			struct v4l2_dbg_chip_ident *chip)
 {
-	int ret;
 	struct gspca_dev *gspca_dev = video_drvdata(file);
 
 	if (!gspca_dev->sd_desc->get_chip_ident)
 		return -ENOTTY;
 
-	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_chip_ident(gspca_dev, chip);
-	else
-		ret = -ENODEV;
-	mutex_unlock(&gspca_dev->usb_lock);
-
-	return ret;
+	return gspca_dev->sd_desc->get_chip_ident(gspca_dev, chip);
 }
 
 static int vidioc_enum_fmt_vid_cap(struct file *file, void  *priv,
@@ -1339,8 +1300,6 @@ static int dev_open(struct file *file)
 	struct gspca_dev *gspca_dev = video_drvdata(file);
 
 	PDEBUG(D_STREAM, "[%s] open", current->comm);
-	if (!gspca_dev->present)
-		return -ENODEV;
 
 	/* protect the subdriver against rmmod */
 	if (!try_module_get(gspca_dev->module))
@@ -1369,10 +1328,8 @@ static int dev_close(struct file *file)
 	/* if the file did the capture, free the streaming resources */
 	if (gspca_dev->capt_file == file) {
 		if (gspca_dev->streaming) {
-			mutex_lock(&gspca_dev->usb_lock);
 			gspca_dev->usb_err = 0;
 			gspca_stream_off(gspca_dev);
-			mutex_unlock(&gspca_dev->usb_lock);
 		}
 		frame_free(gspca_dev);
 	}
@@ -1388,15 +1345,7 @@ static int vidioc_querycap(struct file *file, void  *priv,
 			   struct v4l2_capability *cap)
 {
 	struct gspca_dev *gspca_dev = video_drvdata(file);
-	int ret;
 
-	/* protect the access to the usb device */
-	if (mutex_lock_interruptible(&gspca_dev->usb_lock))
-		return -ERESTARTSYS;
-	if (!gspca_dev->present) {
-		ret = -ENODEV;
-		goto out;
-	}
 	strlcpy((char *) cap->driver, gspca_dev->sd_desc->name,
 			sizeof cap->driver);
 	if (gspca_dev->dev->product != NULL) {
@@ -1414,10 +1363,7 @@ static int vidioc_querycap(struct file *file, void  *priv,
 			  | V4L2_CAP_STREAMING
 			  | V4L2_CAP_READWRITE;
 	cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
-	ret = 0;
-out:
-	mutex_unlock(&gspca_dev->usb_lock);
-	return ret;
+	return 0;
 }
 
 static int get_ctrl(struct gspca_dev *gspca_dev,
@@ -1486,7 +1432,7 @@ static int vidioc_s_ctrl(struct file *file, void *priv,
 	struct gspca_dev *gspca_dev = video_drvdata(file);
 	const struct ctrl *ctrls;
 	struct gspca_ctrl *gspca_ctrl;
-	int idx, ret;
+	int idx;
 
 	idx = get_ctrl(gspca_dev, ctrl->id);
 	if (idx < 0)
@@ -1506,27 +1452,16 @@ static int vidioc_s_ctrl(struct file *file, void *priv,
 			return -ERANGE;
 	}
 	PDEBUG(D_CONF, "set ctrl [%08x] = %d", ctrl->id, ctrl->value);
-	if (mutex_lock_interruptible(&gspca_dev->usb_lock))
-		return -ERESTARTSYS;
-	if (!gspca_dev->present) {
-		ret = -ENODEV;
-		goto out;
-	}
 	gspca_dev->usb_err = 0;
-	if (ctrls->set != NULL) {
-		ret = ctrls->set(gspca_dev, ctrl->value);
-		goto out;
-	}
+	if (ctrls->set != NULL)
+		return ctrls->set(gspca_dev, ctrl->value);
 	if (gspca_ctrl != NULL) {
 		gspca_ctrl->val = ctrl->value;
 		if (ctrls->set_control != NULL
 		 && gspca_dev->streaming)
 			ctrls->set_control(gspca_dev);
 	}
-	ret = gspca_dev->usb_err;
-out:
-	mutex_unlock(&gspca_dev->usb_lock);
-	return ret;
+	return gspca_dev->usb_err;
 }
 
 static int vidioc_g_ctrl(struct file *file, void *priv,
@@ -1534,30 +1469,19 @@ static int vidioc_g_ctrl(struct file *file, void *priv,
 {
 	struct gspca_dev *gspca_dev = video_drvdata(file);
 	const struct ctrl *ctrls;
-	int idx, ret;
+	int idx;
 
 	idx = get_ctrl(gspca_dev, ctrl->id);
 	if (idx < 0)
 		return -EINVAL;
 	ctrls = &gspca_dev->sd_desc->ctrls[idx];
 
-	if (mutex_lock_interruptible(&gspca_dev->usb_lock))
-		return -ERESTARTSYS;
-	if (!gspca_dev->present) {
-		ret = -ENODEV;
-		goto out;
-	}
 	gspca_dev->usb_err = 0;
-	if (ctrls->get != NULL) {
-		ret = ctrls->get(gspca_dev, &ctrl->value);
-		goto out;
-	}
+	if (ctrls->get != NULL)
+		return ctrls->get(gspca_dev, &ctrl->value);
 	if (gspca_dev->cam.ctrls != NULL)
 		ctrl->value = gspca_dev->cam.ctrls[idx].val;
-	ret = 0;
-out:
-	mutex_unlock(&gspca_dev->usb_lock);
-	return ret;
+	return 0;
 }
 
 static int vidioc_querymenu(struct file *file, void *priv,
@@ -1640,10 +1564,8 @@ static int vidioc_reqbufs(struct file *file, void *priv,
 	/* stop streaming */
 	streaming = gspca_dev->streaming;
 	if (streaming) {
-		mutex_lock(&gspca_dev->usb_lock);
 		gspca_dev->usb_err = 0;
 		gspca_stream_off(gspca_dev);
-		mutex_unlock(&gspca_dev->usb_lock);
 
 		/* Don't restart the stream when switching from read
 		 * to mmap mode */
@@ -1748,13 +1670,8 @@ static int vidioc_streamoff(struct file *file, void *priv,
 	}
 
 	/* stop streaming */
-	if (mutex_lock_interruptible(&gspca_dev->usb_lock)) {
-		ret = -ERESTARTSYS;
-		goto out;
-	}
 	gspca_dev->usb_err = 0;
 	gspca_stream_off(gspca_dev);
-	mutex_unlock(&gspca_dev->usb_lock);
 	/* In case another thread is waiting in dqbuf */
 	wake_up_interruptible(&gspca_dev->wq);
 
@@ -1772,63 +1689,36 @@ static int vidioc_g_jpegcomp(struct file *file, void *priv,
 			struct v4l2_jpegcompression *jpegcomp)
 {
 	struct gspca_dev *gspca_dev = video_drvdata(file);
-	int ret;
 
 	if (!gspca_dev->sd_desc->get_jcomp)
 		return -ENOTTY;
-	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);
-	return ret;
+	return gspca_dev->sd_desc->get_jcomp(gspca_dev, jpegcomp);
 }
 
 static int vidioc_s_jpegcomp(struct file *file, void *priv,
 			struct v4l2_jpegcompression *jpegcomp)
 {
 	struct gspca_dev *gspca_dev = video_drvdata(file);
-	int ret;
 
 	if (!gspca_dev->sd_desc->set_jcomp)
 		return -ENOTTY;
-	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);
-	return ret;
+	return gspca_dev->sd_desc->set_jcomp(gspca_dev, jpegcomp);
 }
 
 static int vidioc_g_parm(struct file *filp, void *priv,
 			struct v4l2_streamparm *parm)
 {
-	struct gspca_dev *gspca_dev = video_drvdata(file);
+	struct gspca_dev *gspca_dev = video_drvdata(filp);
 
 	parm->parm.capture.readbuffers = gspca_dev->nbufread;
 
 	if (gspca_dev->sd_desc->get_streamparm) {
-		int ret;
-
-		if (mutex_lock_interruptible(&gspca_dev->usb_lock))
-			return -ERESTARTSYS;
-		if (gspca_dev->present) {
-			gspca_dev->usb_err = 0;
-			gspca_dev->sd_desc->get_streamparm(gspca_dev, parm);
-			ret = gspca_dev->usb_err;
-		} else {
-			ret = -ENODEV;
-		}
-		mutex_unlock(&gspca_dev->usb_lock);
-		return ret;
+		gspca_dev->usb_err = 0;
+		gspca_dev->sd_desc->get_streamparm(gspca_dev, parm);
+		return gspca_dev->usb_err;
 	}
-
 	return 0;
 }
 
@@ -1845,19 +1735,9 @@ static int vidioc_s_parm(struct file *filp, void *priv,
 		gspca_dev->nbufread = n;
 
 	if (gspca_dev->sd_desc->set_streamparm) {
-		int ret;
-
-		if (mutex_lock_interruptible(&gspca_dev->usb_lock))
-			return -ERESTARTSYS;
-		if (gspca_dev->present) {
-			gspca_dev->usb_err = 0;
-			gspca_dev->sd_desc->set_streamparm(gspca_dev, parm);
-			ret = gspca_dev->usb_err;
-		} else {
-			ret = -ENODEV;
-		}
-		mutex_unlock(&gspca_dev->usb_lock);
-		return ret;
+		gspca_dev->usb_err = 0;
+		gspca_dev->sd_desc->set_streamparm(gspca_dev, parm);
+		return gspca_dev->usb_err;
 	}
 
 	return 0;
@@ -1877,10 +1757,6 @@ static int dev_mmap(struct file *file, struct vm_area_struct *vma)
 
 	if (mutex_lock_interruptible(&gspca_dev->queue_lock))
 		return -ERESTARTSYS;
-	if (!gspca_dev->present) {
-		ret = -ENODEV;
-		goto out;
-	}
 	if (gspca_dev->capt_file != file) {
 		ret = -EINVAL;
 		goto out;
@@ -2008,14 +1884,6 @@ static int vidioc_dqbuf(struct file *file, void *priv,
 
 	gspca_dev->fr_o = (i + 1) % GSPCA_MAX_FRAMES;
 
-	if (gspca_dev->sd_desc->dq_callback) {
-		mutex_lock(&gspca_dev->usb_lock);
-		gspca_dev->usb_err = 0;
-		if (gspca_dev->present)
-			gspca_dev->sd_desc->dq_callback(gspca_dev);
-		mutex_unlock(&gspca_dev->usb_lock);
-	}
-
 	frame->v4l2_buf.flags &= ~V4L2_BUF_FLAG_DONE;
 	memcpy(v4l2_buf, &frame->v4l2_buf, sizeof *v4l2_buf);
 	PDEBUG(D_FRAM, "dqbuf %d", j);
@@ -2032,6 +1900,15 @@ static int vidioc_dqbuf(struct file *file, void *priv,
 	}
 out:
 	mutex_unlock(&gspca_dev->queue_lock);
+
+	if (ret == 0 && gspca_dev->sd_desc->dq_callback) {
+		mutex_lock(&gspca_dev->usb_lock);
+		gspca_dev->usb_err = 0;
+		if (gspca_dev->present)
+			gspca_dev->sd_desc->dq_callback(gspca_dev);
+		mutex_unlock(&gspca_dev->usb_lock);
+	}
+
 	return ret;
 }
 
@@ -2177,8 +2054,6 @@ static ssize_t dev_read(struct file *file, char __user *data,
 	int n, ret, ret2;
 
 	PDEBUG(D_FRAM, "read (%zd)", count);
-	if (!gspca_dev->present)
-		return -ENODEV;
 	if (gspca_dev->memory == GSPCA_MEMORY_NO) { /* first time ? */
 		ret = read_alloc(gspca_dev, file);
 		if (ret != 0)
@@ -2366,6 +2241,7 @@ int gspca_dev_probe2(struct usb_interface *intf,
 	gspca_dev->present = 1;
 
 	mutex_init(&gspca_dev->usb_lock);
+	gspca_dev->vdev.lock = &gspca_dev->usb_lock;
 	mutex_init(&gspca_dev->queue_lock);
 	init_waitqueue_head(&gspca_dev->wq);
 
@@ -2388,6 +2264,14 @@ int gspca_dev_probe2(struct usb_interface *intf,
 	if (ret)
 		goto out;
 
+	/* These ioctls use just queue_lock and not usb_lock.
+	   This improves latency if the usb lock is taken for a
+	   long time, e.g. when changing a control value, and a
+	   new frame is ready to be dequeued. */
+	v4l2_dont_use_lock(&gspca_dev->vdev, VIDIOC_DQBUF);
+	v4l2_dont_use_lock(&gspca_dev->vdev, VIDIOC_QBUF);
+	v4l2_dont_use_lock(&gspca_dev->vdev, VIDIOC_QUERYBUF);
+
 	/* init video stuff */
 	ret = video_register_device(&gspca_dev->vdev,
 				  VFL_TYPE_GRABBER,
@@ -2455,11 +2339,11 @@ void gspca_disconnect(struct usb_interface *intf)
 
 	PDEBUG(D_PROBE, "%s disconnect",
 		video_device_node_name(&gspca_dev->vdev));
+
 	mutex_lock(&gspca_dev->usb_lock);
 
 	gspca_dev->present = 0;
 	wake_up_interruptible(&gspca_dev->wq);
-
 	destroy_urbs(gspca_dev);
 
 #if defined(CONFIG_INPUT) || defined(CONFIG_INPUT_MODULE)
@@ -2471,18 +2355,18 @@ void gspca_disconnect(struct usb_interface *intf)
 	}
 #endif
 
-	/* the device is freed at exit of this function */
+	/* The USB-interface 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);
 
-	/* release the device */
-	/* (this will call gspca_release() immediately or on last close) */
+	/* Ensure gspca_dev sticks around for the usb_lock unlock! */
+	get_device(&gspca_dev->vdev.dev);
 	video_unregister_device(&gspca_dev->vdev);
+	mutex_unlock(&gspca_dev->usb_lock);
+	/* (this will call gspca_release() immediately or on last close) */
+	put_device(&gspca_dev->vdev.dev);
 
-/*	PDEBUG(D_PROBE, "disconnect complete"); */
+	usb_set_intfdata(intf, NULL);
 }
 EXPORT_SYMBOL(gspca_disconnect);
 
@@ -2493,17 +2377,16 @@ int gspca_suspend(struct usb_interface *intf, pm_message_t message)
 
 	if (!gspca_dev->streaming)
 		return 0;
+	mutex_lock(&gspca_dev->usb_lock);
 	gspca_dev->frozen = 1;		/* avoid urb error messages */
 	if (gspca_dev->sd_desc->stopN)
 		gspca_dev->sd_desc->stopN(gspca_dev);
 	destroy_urbs(gspca_dev);
 	gspca_input_destroy_urb(gspca_dev);
 	gspca_set_alt0(gspca_dev);
-	if (gspca_dev->sd_desc->stop0) {
-		mutex_lock(&gspca_dev->usb_lock);
+	if (gspca_dev->sd_desc->stop0)
 		gspca_dev->sd_desc->stop0(gspca_dev);
-		mutex_unlock(&gspca_dev->usb_lock);
-	}
+	mutex_unlock(&gspca_dev->usb_lock);
 	return 0;
 }
 EXPORT_SYMBOL(gspca_suspend);
@@ -2513,15 +2396,14 @@ int gspca_resume(struct usb_interface *intf)
 	struct gspca_dev *gspca_dev = usb_get_intfdata(intf);
 	int ret = 0;
 
+	mutex_lock(&gspca_dev->usb_lock);
 	gspca_dev->frozen = 0;
 	gspca_dev->sd_desc->init(gspca_dev);
 	gspca_set_default_mode(gspca_dev);
 	gspca_input_create_urb(gspca_dev);
-	if (gspca_dev->streaming) {
-		mutex_lock(&gspca_dev->queue_lock);
+	if (gspca_dev->streaming)
 		ret = gspca_init_transfer(gspca_dev);
-		mutex_unlock(&gspca_dev->queue_lock);
-	}
+	mutex_unlock(&gspca_dev->usb_lock);
 	return ret;
 }
 EXPORT_SYMBOL(gspca_resume);
-- 
1.7.10

Re: [RFCv2 PATCH 13/17] gspca: switch to V4L2 core locking, except for the buffer queuing ioctls.

[Hans Verkuil]

On Sun May 6 2012 17:25:55 Hans de Goede wrote:
> Hi Hans,
> 
> The entire series looks great, I do have a few remarks wrt this
> patch, which I have fixed in my own tree (new version attached,
> note untested sofar).
> 
> On 05/06/2012 02:28 PM, Hans Verkuil wrote:
> > From: Hans Verkuil<hans.verkuil@cisco.com>
> >
> > Due to latency concerns the VIDIOC_QBUF, DQBUF and QUERYBUF do not use the
> > core lock, instead they rely only on queue_lock.
> >
> > Signed-off-by: Hans Verkuil<hans.verkuil@cisco.com>
> > ---
> >   drivers/media/video/gspca/gspca.c |  203 ++++++++-----------------------------
> >   1 file changed, 41 insertions(+), 162 deletions(-)
> >
> > diff --git a/drivers/media/video/gspca/gspca.c b/drivers/media/video/gspca/gspca.c
> > index f840bed..edca4f3 100644
> > --- a/drivers/media/video/gspca/gspca.c
> > +++ b/drivers/media/video/gspca/gspca.c
> > @@ -850,14 +850,6 @@ static int gspca_init_transfer(struct gspca_dev *gspca_dev)
> >   	struct ep_tb_s ep_tb[MAX_ALT];
> >   	int n, ret, xfer, alt, alt_idx;
> >
> > -	if (mutex_lock_interruptible(&gspca_dev->usb_lock))
> > -		return -ERESTARTSYS;
> > -
> > -	if (!gspca_dev->present) {
> > -		ret = -ENODEV;
> > -		goto unlock;
> > -	}
> > -
> >   	/* reset the streaming variables */
> >   	gspca_dev->image = NULL;
> >   	gspca_dev->image_len = 0;
> 
> You're removing a lot of checks for gspca_dev->present, relying on the
> video_is_registered checks in v4l2-dev instead I assume,

Right. It's likely that all the present checks can be removed, but I just
remove the ones that I knew where unnecessary.

> this is a good
> idea, *but* it requires a small hack in disconnect to close a race.
> 
> Currently the end of gspca_disconnect looks like this:
> 
>          gspca_dev->dev = NULL;
>          v4l2_device_disconnect(&gspca_dev->v4l2_dev);
>          mutex_unlock(&gspca_dev->usb_lock);
> 
>          usb_set_intfdata(intf, NULL);
> 
>          /* release the device */
>          /* (this will call gspca_release() immediately or on last close) */
>          video_unregister_device(&gspca_dev->vdev);
> }
> 
> Notice that usb_lock is unlocked before video_unregister_device gets called,
> which means that any ioctl or other fops waiting for usb_lock can run
> before video_unregister_device runs, and thus before they are protected
> against being called on an disconnected device by the
> video_is_registered checks in v4l2-dev.

True, good catch, I missed that one.

> Unfortunately simply moving the unlock down won't work, because if there
> are no open file handles referencing the device, then the memory
> referenced by gspca_dev will be free-ed after the video_unregister_device
> call.

What you should do (refer to the disconnect implementation in radio/dsbr100.c)
is to use the release callback of struct v4l2_device instead. That way the
memory will be released after you call v4l2_device_put() as the last line in
the disconnect(). The advantage of using the v4l2_device release callback is
that it also works if you have more than one video/radio/vbi node. Only when
the very last user of the very last node exits will the release be called.

Actually, that was the main reason I added it since trying to keep track of
multiple device node references in a driver is doomed to fail.

> So I've changed disconnect to the following in my version, to allow the
> present check removal you've did, as I quite like being able to
> remove all those present checks :)   :
> 
>          /* The USB-interface device is freed at exit of this function */
>          gspca_dev->dev = NULL;
>          v4l2_device_disconnect(&gspca_dev->v4l2_dev);
> 
>          /* Ensure gspca_dev sticks around for the usb_lock unlock! */
>          get_device(&gspca_dev->vdev.dev);
>          video_unregister_device(&gspca_dev->vdev);
>          mutex_unlock(&gspca_dev->usb_lock);
>          /* (this will call gspca_release() immediately or on last close) */
>          put_device(&gspca_dev->vdev.dev);
> 
>          usb_set_intfdata(intf, NULL);
> }
> 
> 
> <snip chunks on which I've no comments>
> 
> > @@ -1736,10 +1658,8 @@ static int vidioc_streamoff(struct file *file, void *priv,
> >   	if (mutex_lock_interruptible(&gspca_dev->queue_lock))
> >   		return -ERESTARTSYS;
> >
> > -	if (!gspca_dev->streaming) {
> > -		ret = 0;
> > -		goto out;
> > -	}
> > +	if (!gspca_dev->streaming)
> > +		return 0;
> >
> 
> BAD! queue_lock is held here, so you cannot just change this to a return!

Good catch. I never tested calling STREAMOFF when not streaming :-)

> 
> >   	/* check the capture file */
> >   	if (gspca_dev->capt_file != file) {
> 
> <snip chunks on which I've no comments>
> 
> > @@ -2009,11 +1883,9 @@ static int vidioc_dqbuf(struct file *file, void *priv,
> >   	gspca_dev->fr_o = (i + 1) % GSPCA_MAX_FRAMES;
> >
> >   	if (gspca_dev->sd_desc->dq_callback) {
> > -		mutex_lock(&gspca_dev->usb_lock);
> >   		gspca_dev->usb_err = 0;
> >   		if (gspca_dev->present)
> >   			gspca_dev->sd_desc->dq_callback(gspca_dev);
> > -		mutex_unlock(&gspca_dev->usb_lock);
> >   	}
> >
> >   	frame->v4l2_buf.flags&= ~V4L2_BUF_FLAG_DONE;
> 
> You cannot remove the locking here, as dq_callback expects to be
> called with the usb-lock locked.
> 
> Since usb-lock now is the device lock and thus gets locked before
> the queue_lock, we cannot simply drop this chunk. Instead I've
> moved the dq_callback to the end of vidioc_dqbuf, so after the
> stream_lock has been released (there is no reason to have
> the stream_lock hold when calling the dq_callback).
> 
> The dq_callback is used to do camera control adjustments which
> need to be done after every X frames, and which cannot be done
> from the isoc frame interrupts since they should not be done under
> interrupt. When the drivers using dq_callback are converted to the
> control framework, they will likely end up calling v4l2_ctrl_s_ctrl
> from the dq_callback.

Can't dq_callback be called at the end of the function? After the
mutex_unlock(&queue_lock)? There we can take the usb_lock, call dq_callback
and unlock usb_lock again:

out:
		mutex_unlock(&gspca_dev->queue_lock);
        if (!ret && gspca_dev->sd_desc->dq_callback) {
		        if (mutex_lock_interruptible(&gspca_dev->usb_lock))
            		    return -ERESTARTSYS;
                gspca_dev->usb_err = 0;
                gspca_dev->sd_desc->dq_callback(gspca_dev);
				mutex_unlock(&gspca_dev->usb_lock);
        }
		return ret;

It seems reasonable, but I haven't looked in detail at what dq_callback
does.

Regards,

	Hans


> 
> <snip chunks on which I've no comments>
> 
> Regards,
> 
> Hans
> 
> 
> p.s.
> 
> I've yet to take a good look at the driver conversions other then
> the zc3xx conversion.
> 

Re: [RFCv2 PATCH 13/17] gspca: switch to V4L2 core locking, except for the buffer queuing ioctls.

[Hans de Goede]

Hi,

On 05/06/2012 05:51 PM, Hans Verkuil wrote:
> On Sun May 6 2012 17:25:55 Hans de Goede wrote:

<snip snip>

>> Notice that usb_lock is unlocked before video_unregister_device gets called,
>> which means that any ioctl or other fops waiting for usb_lock can run
>> before video_unregister_device runs, and thus before they are protected
>> against being called on an disconnected device by the
>> video_is_registered checks in v4l2-dev.
>
> True, good catch, I missed that one.
>
>> Unfortunately simply moving the unlock down won't work, because if there
>> are no open file handles referencing the device, then the memory
>> referenced by gspca_dev will be free-ed after the video_unregister_device
>> call.
>
> What you should do (refer to the disconnect implementation in radio/dsbr100.c)
> is to use the release callback of struct v4l2_device instead. That way the
> memory will be released after you call v4l2_device_put() as the last line in
> the disconnect(). The advantage of using the v4l2_device release callback is
> that it also works if you have more than one video/radio/vbi node. Only when
> the very last user of the very last node exits will the release be called.

Better solution, I'll adapt your patch to use this solution, merging in
the necessary changes.

<snip snip>

>>> @@ -2009,11 +1883,9 @@ static int vidioc_dqbuf(struct file *file, void *priv,
>>>    	gspca_dev->fr_o = (i + 1) % GSPCA_MAX_FRAMES;
>>>
>>>    	if (gspca_dev->sd_desc->dq_callback) {
>>> -		mutex_lock(&gspca_dev->usb_lock);
>>>    		gspca_dev->usb_err = 0;
>>>    		if (gspca_dev->present)
>>>    			gspca_dev->sd_desc->dq_callback(gspca_dev);
>>> -		mutex_unlock(&gspca_dev->usb_lock);
>>>    	}
>>>
>>>    	frame->v4l2_buf.flags&= ~V4L2_BUF_FLAG_DONE;
>>
>> You cannot remove the locking here, as dq_callback expects to be
>> called with the usb-lock locked.
>>
>> Since usb-lock now is the device lock and thus gets locked before
>> the queue_lock, we cannot simply drop this chunk. Instead I've
>> moved the dq_callback to the end of vidioc_dqbuf, so after the
>> stream_lock has been released (there is no reason to have
>> the stream_lock hold when calling the dq_callback).
>>
>> The dq_callback is used to do camera control adjustments which
>> need to be done after every X frames, and which cannot be done
>> from the isoc frame interrupts since they should not be done under
>> interrupt. When the drivers using dq_callback are converted to the
>> control framework, they will likely end up calling v4l2_ctrl_s_ctrl
>> from the dq_callback.
>
> Can't dq_callback be called at the end of the function?

Right, if you look at the new version of the patch I had attached,
you will see that is actually what I did, which is what I tried to
explain with: "Instead I've moved the dq_callback to the end of
vidioc_dqbuf, so after the stream_lock has been released (there
is no reason to have the stream_lock hold when calling the dq_callback)".


> After the
> mutex_unlock(&queue_lock)? There we can take the usb_lock, call dq_callback
> and unlock usb_lock again:
>
> out:
> 		mutex_unlock(&gspca_dev->queue_lock);
>          if (!ret&&  gspca_dev->sd_desc->dq_callback) {
> 		        if (mutex_lock_interruptible(&gspca_dev->usb_lock))
>              		    return -ERESTARTSYS;
>                  gspca_dev->usb_err = 0;
>                  gspca_dev->sd_desc->dq_callback(gspca_dev);
> 				mutex_unlock(&gspca_dev->usb_lock);
>          }
> 		return ret;
>

Right, almost exactly what I've except that you're missing a gscpa_dev->present
check after taking the lock.

Regards,

Hans