[RFC/PATCH 0/6] V4L2 subdev userspace API

[RFC/PATCH 0/6] V4L2 subdev userspace API

[Laurent Pinchart]

Hi everybody,

Here are 6 patches that add a userspace API to the V4L2 subdevices. The API has
been developed to support the media controller and the OMAP3 ISP driver.

A few people have shown interest in the subdev userspace API already. As the
patches are not dependent on the media controller itself I'm submitting them
independently for review.

The API covers controls, events and generic ioctls. The controls and events
support reuse V4L2 ioctls, as explained in
Documentation/video4linux/v4l2-framework.txt. The subdev (and later media
controller) userspace API should probably be converted to DocBook format
eventually. The subdev API can be included in the V4L2 API document, but the
media controller should be kept separate. Comments on this will be appreciated.

While waiting for review I'll prepare the media controller core patches and
send them to the list.

Laurent Pinchart (5):
  v4l: subdev: Don't require core operations
  v4l: subdev: Add device node support
  v4l: subdev: Uninline the v4l2_subdev_init function
  v4l: subdev: Control ioctls support
  v4l: subdev: Generic ioctl support

Sakari Ailus (1):
  v4l: subdev: Events support

 Documentation/video4linux/v4l2-framework.txt |   47 +++++++
 drivers/media/video/Makefile                 |    2 +-
 drivers/media/video/v4l2-common.c            |    3 +
 drivers/media/video/v4l2-dev.c               |    5 +
 drivers/media/video/v4l2-device.c            |   27 ++++-
 drivers/media/video/v4l2-subdev.c            |  172 ++++++++++++++++++++++++++
 include/media/v4l2-dev.h                     |    3 +-
 include/media/v4l2-subdev.h                  |   33 +++--
 8 files changed, 276 insertions(+), 16 deletions(-)
 create mode 100644 drivers/media/video/v4l2-subdev.c

-- 
Regards,

Laurent Pinchart

[RFC/PATCH 1/6] v4l: subdev: Don't require core operations

[Laurent Pinchart]

There's no reason to require subdevices to implement the core
operations. Remove the check for non-NULL core operations when
initializing the subdev.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/media/v4l2-subdev.h |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 02c6f4d..6088316 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -437,8 +437,7 @@ static inline void v4l2_subdev_init(struct v4l2_subdev *sd,
 					const struct v4l2_subdev_ops *ops)
 {
 	INIT_LIST_HEAD(&sd->list);
-	/* ops->core MUST be set */
-	BUG_ON(!ops || !ops->core);
+	BUG_ON(!ops);
 	sd->ops = ops;
 	sd->v4l2_dev = NULL;
 	sd->flags = 0;
-- 
1.7.1

[RFC/PATCH 2/6] v4l: subdev: Add device node support

[Laurent Pinchart]

Create a device node named subdevX for every registered subdev.

As the device node is registered before the subdev core::s_config
function is called, return -EGAIN on open until initialization
completes.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Vimarsh Zutshi <vimarsh.zutshi@nokia.com>
---
 drivers/media/video/Makefile      |    2 +-
 drivers/media/video/v4l2-common.c |    3 ++
 drivers/media/video/v4l2-dev.c    |    5 +++
 drivers/media/video/v4l2-device.c |   27 +++++++++++++++-
 drivers/media/video/v4l2-subdev.c |   65 +++++++++++++++++++++++++++++++++++++
 include/media/v4l2-dev.h          |    3 +-
 include/media/v4l2-subdev.h       |   10 ++++++
 7 files changed, 112 insertions(+), 3 deletions(-)
 create mode 100644 drivers/media/video/v4l2-subdev.c

diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index cc93859..c9c07e5 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -11,7 +11,7 @@ stkwebcam-objs	:=	stk-webcam.o stk-sensor.o
 omap2cam-objs	:=	omap24xxcam.o omap24xxcam-dma.o
 
 videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
-			v4l2-event.o
+			v4l2-event.o v4l2-subdev.o
 
 # V4L2 core modules
 
diff --git a/drivers/media/video/v4l2-common.c b/drivers/media/video/v4l2-common.c
index 4e53b0b..3032aa3 100644
--- a/drivers/media/video/v4l2-common.c
+++ b/drivers/media/video/v4l2-common.c
@@ -871,6 +871,7 @@ struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device *v4l2_dev,
 
 	/* Register with the v4l2_device which increases the module's
 	   use count as well. */
+	sd->initialized = 0;
 	if (v4l2_device_register_subdev(v4l2_dev, sd))
 		sd = NULL;
 	/* Decrease the module use count to match the first try_module_get. */
@@ -885,6 +886,8 @@ struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device *v4l2_dev,
 		if (err && err != -ENOIOCTLCMD) {
 			v4l2_device_unregister_subdev(sd);
 			sd = NULL;
+		} else {
+			sd->initialized = 1;
 		}
 	}
 
diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
index 0ca7ec9..5a9e9df 100644
--- a/drivers/media/video/v4l2-dev.c
+++ b/drivers/media/video/v4l2-dev.c
@@ -401,6 +401,8 @@ static int get_index(struct video_device *vdev)
  *	%VFL_TYPE_VBI - Vertical blank data (undecoded)
  *
  *	%VFL_TYPE_RADIO - A radio card
+ *
+ *	%VFL_TYPE_SUBDEV - A subdevice
  */
 static int __video_register_device(struct video_device *vdev, int type, int nr,
 		int warn_if_nr_in_use)
@@ -439,6 +441,9 @@ static int __video_register_device(struct video_device *vdev, int type, int nr,
 	case VFL_TYPE_RADIO:
 		name_base = "radio";
 		break;
+	case VFL_TYPE_SUBDEV:
+		name_base = "subdev";
+		break;
 	default:
 		printk(KERN_ERR "%s called with unknown type: %d\n",
 		       __func__, type);
diff --git a/drivers/media/video/v4l2-device.c b/drivers/media/video/v4l2-device.c
index 5a7dc4a..685fa82 100644
--- a/drivers/media/video/v4l2-device.c
+++ b/drivers/media/video/v4l2-device.c
@@ -115,18 +115,40 @@ EXPORT_SYMBOL_GPL(v4l2_device_unregister);
 int v4l2_device_register_subdev(struct v4l2_device *v4l2_dev,
 						struct v4l2_subdev *sd)
 {
+	struct video_device *vdev;
+	int ret;
+
 	/* Check for valid input */
 	if (v4l2_dev == NULL || sd == NULL || !sd->name[0])
 		return -EINVAL;
+
 	/* Warn if we apparently re-register a subdev */
 	WARN_ON(sd->v4l2_dev != NULL);
+
 	if (!try_module_get(sd->owner))
 		return -ENODEV;
+
 	sd->v4l2_dev = v4l2_dev;
 	spin_lock(&v4l2_dev->lock);
 	list_add_tail(&sd->list, &v4l2_dev->subdevs);
 	spin_unlock(&v4l2_dev->lock);
-	return 0;
+
+	/* Register the device node. */
+	vdev = &sd->devnode;
+	snprintf(vdev->name, sizeof(vdev->name), "subdev");
+	vdev->parent = v4l2_dev->dev;
+	vdev->fops = &v4l2_subdev_fops;
+	vdev->release = video_device_release_empty;
+	ret = video_register_device(vdev, VFL_TYPE_SUBDEV, -1);
+	if (ret < 0) {
+		spin_lock(&v4l2_dev->lock);
+		list_del(&sd->list);
+		spin_unlock(&v4l2_dev->lock);
+		sd->v4l2_dev = NULL;
+		module_put(sd->owner);
+	}
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(v4l2_device_register_subdev);
 
@@ -135,10 +157,13 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev *sd)
 	/* return if it isn't registered */
 	if (sd == NULL || sd->v4l2_dev == NULL)
 		return;
+
 	spin_lock(&sd->v4l2_dev->lock);
 	list_del(&sd->list);
 	spin_unlock(&sd->v4l2_dev->lock);
 	sd->v4l2_dev = NULL;
+
 	module_put(sd->owner);
+	video_unregister_device(&sd->devnode);
 }
 EXPORT_SYMBOL_GPL(v4l2_device_unregister_subdev);
diff --git a/drivers/media/video/v4l2-subdev.c b/drivers/media/video/v4l2-subdev.c
new file mode 100644
index 0000000..a048161
--- /dev/null
+++ b/drivers/media/video/v4l2-subdev.c
@@ -0,0 +1,65 @@
+/*
+ *  V4L2 subdevice support.
+ *
+ *  Copyright (C) 2009  Laurent Pinchart <laurent.pinchart@ideasonboard.com>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include <linux/types.h>
+#include <linux/ioctl.h>
+#include <linux/videodev2.h>
+
+#include <media/v4l2-device.h>
+#include <media/v4l2-ioctl.h>
+
+static int subdev_open(struct file *file)
+{
+	struct video_device *vdev = video_devdata(file);
+	struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
+
+	if (!sd->initialized)
+		return -EAGAIN;
+
+	return 0;
+}
+
+static int subdev_close(struct file *file)
+{
+	return 0;
+}
+
+static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
+{
+	switch (cmd) {
+	default:
+		return -ENOIOCTLCMD;
+	}
+
+	return 0;
+}
+
+static long subdev_ioctl(struct file *file, unsigned int cmd,
+	unsigned long arg)
+{
+	return video_usercopy(file, cmd, arg, subdev_do_ioctl);
+}
+
+const struct v4l2_file_operations v4l2_subdev_fops = {
+	.owner = THIS_MODULE,
+	.open = subdev_open,
+	.unlocked_ioctl = subdev_ioctl,
+	.release = subdev_close,
+};
diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
index bebe44b..eee1e2b 100644
--- a/include/media/v4l2-dev.h
+++ b/include/media/v4l2-dev.h
@@ -22,7 +22,8 @@
 #define VFL_TYPE_VBI		1
 #define VFL_TYPE_RADIO		2
 #define VFL_TYPE_VTX		3
-#define VFL_TYPE_MAX		4
+#define VFL_TYPE_SUBDEV		4
+#define VFL_TYPE_MAX		5
 
 struct v4l2_ioctl_callbacks;
 struct video_device;
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 6088316..00010bd 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -22,6 +22,7 @@
 #define _V4L2_SUBDEV_H
 
 #include <media/v4l2-common.h>
+#include <media/v4l2-dev.h>
 #include <media/v4l2-mediabus.h>
 
 /* generic v4l2_device notify callback notification values */
@@ -421,8 +422,16 @@ struct v4l2_subdev {
 	u32 grp_id;
 	/* pointer to private data */
 	void *priv;
+	/* subdev device node */
+	struct video_device devnode;
+	unsigned int initialized;
 };
 
+#define vdev_to_v4l2_subdev(vdev) \
+	container_of(vdev, struct v4l2_subdev, devnode)
+
+extern const struct v4l2_file_operations v4l2_subdev_fops;
+
 static inline void v4l2_set_subdevdata(struct v4l2_subdev *sd, void *p)
 {
 	sd->priv = p;
@@ -444,6 +453,7 @@ static inline void v4l2_subdev_init(struct v4l2_subdev *sd,
 	sd->name[0] = '\0';
 	sd->grp_id = 0;
 	sd->priv = NULL;
+	sd->initialized = 0;
 }
 
 /* Call an ops of a v4l2_subdev, doing the right checks against
-- 
1.7.1

[RFC/PATCH 3/6] v4l: subdev: Uninline the v4l2_subdev_init function

[Laurent Pinchart]

The function isn't small or performance sensitive enough to be inlined.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/v4l2-subdev.c |   14 ++++++++++++++
 include/media/v4l2-subdev.h       |   15 ++-------------
 2 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/media/video/v4l2-subdev.c b/drivers/media/video/v4l2-subdev.c
index a048161..a3672f0 100644
--- a/drivers/media/video/v4l2-subdev.c
+++ b/drivers/media/video/v4l2-subdev.c
@@ -63,3 +63,17 @@ const struct v4l2_file_operations v4l2_subdev_fops = {
 	.unlocked_ioctl = subdev_ioctl,
 	.release = subdev_close,
 };
+
+void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops)
+{
+	INIT_LIST_HEAD(&sd->list);
+	BUG_ON(!ops);
+	sd->ops = ops;
+	sd->v4l2_dev = NULL;
+	sd->flags = 0;
+	sd->name[0] = '\0';
+	sd->grp_id = 0;
+	sd->priv = NULL;
+	sd->initialized = 0;
+}
+EXPORT_SYMBOL(v4l2_subdev_init);
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 00010bd..7b6edcd 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -442,19 +442,8 @@ static inline void *v4l2_get_subdevdata(const struct v4l2_subdev *sd)
 	return sd->priv;
 }
 
-static inline void v4l2_subdev_init(struct v4l2_subdev *sd,
-					const struct v4l2_subdev_ops *ops)
-{
-	INIT_LIST_HEAD(&sd->list);
-	BUG_ON(!ops);
-	sd->ops = ops;
-	sd->v4l2_dev = NULL;
-	sd->flags = 0;
-	sd->name[0] = '\0';
-	sd->grp_id = 0;
-	sd->priv = NULL;
-	sd->initialized = 0;
-}
+void v4l2_subdev_init(struct v4l2_subdev *sd,
+		      const struct v4l2_subdev_ops *ops);
 
 /* Call an ops of a v4l2_subdev, doing the right checks against
    NULL pointers.
-- 
1.7.1

[RFC/PATCH 4/6] v4l: subdev: Control ioctls support

[Laurent Pinchart]

Pass the control-related ioctls to the subdev driver through the core
operations.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 Documentation/video4linux/v4l2-framework.txt |   24 ++++++++++++++++++++++++
 drivers/media/video/v4l2-subdev.c            |   24 ++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 0 deletions(-)

diff --git a/Documentation/video4linux/v4l2-framework.txt b/Documentation/video4linux/v4l2-framework.txt
index e831aac..f315858 100644
--- a/Documentation/video4linux/v4l2-framework.txt
+++ b/Documentation/video4linux/v4l2-framework.txt
@@ -314,6 +314,30 @@ controlled through GPIO pins. This distinction is only relevant when setting
 up the device, but once the subdev is registered it is completely transparent.
 
 
+V4L2 sub-device userspace API
+-----------------------------
+
+Beside exposing a kernel API through the v4l2_subdev_ops structure, V4L2
+sub-devices can also be controlled directly by userspace applications.
+
+When a sub-device is registered, a device node named subdevX is created in /dev.
+The device node handles a subset of the V4L2 API.
+
+VIDIOC_QUERYCTRL
+VIDIOC_QUERYMENU
+VIDIOC_G_CTRL
+VIDIOC_S_CTRL
+VIDIOC_G_EXT_CTRLS
+VIDIOC_S_EXT_CTRLS
+VIDIOC_TRY_EXT_CTRLS
+
+	The controls ioctls are identical to the ones defined in V4L2. They
+	behave identically, with the only exception that they deal only with
+	controls implemented in the sub-device. Depending on the driver, those
+	controls can be also be accessed through one (or several) V4L2 device
+	nodes.
+
+
 I2C sub-device drivers
 ----------------------
 
diff --git a/drivers/media/video/v4l2-subdev.c b/drivers/media/video/v4l2-subdev.c
index a3672f0..141098b 100644
--- a/drivers/media/video/v4l2-subdev.c
+++ b/drivers/media/video/v4l2-subdev.c
@@ -43,7 +43,31 @@ static int subdev_close(struct file *file)
 
 static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 {
+	struct video_device *vdev = video_devdata(file);
+	struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
+
 	switch (cmd) {
+	case VIDIOC_QUERYCTRL:
+		return v4l2_subdev_call(sd, core, queryctrl, arg);
+
+	case VIDIOC_QUERYMENU:
+		return v4l2_subdev_call(sd, core, querymenu, arg);
+
+	case VIDIOC_G_CTRL:
+		return v4l2_subdev_call(sd, core, g_ctrl, arg);
+
+	case VIDIOC_S_CTRL:
+		return v4l2_subdev_call(sd, core, s_ctrl, arg);
+
+	case VIDIOC_G_EXT_CTRLS:
+		return v4l2_subdev_call(sd, core, g_ext_ctrls, arg);
+
+	case VIDIOC_S_EXT_CTRLS:
+		return v4l2_subdev_call(sd, core, s_ext_ctrls, arg);
+
+	case VIDIOC_TRY_EXT_CTRLS:
+		return v4l2_subdev_call(sd, core, try_ext_ctrls, arg);
+
 	default:
 		return -ENOIOCTLCMD;
 	}
-- 
1.7.1

[RFC/PATCH 5/6] v4l: subdev: Events support

[Laurent Pinchart]

From: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>

Provide v4l2_subdevs with v4l2_event support. Subdev drivers only need very
little to support events.

Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
Signed-off-by: David Cohen <david.cohen@nokia.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 Documentation/video4linux/v4l2-framework.txt |   18 +++++++
 drivers/media/video/v4l2-subdev.c            |   71 +++++++++++++++++++++++++-
 include/media/v4l2-subdev.h                  |    9 +++
 3 files changed, 97 insertions(+), 1 deletions(-)

diff --git a/Documentation/video4linux/v4l2-framework.txt b/Documentation/video4linux/v4l2-framework.txt
index f315858..2f5162c 100644
--- a/Documentation/video4linux/v4l2-framework.txt
+++ b/Documentation/video4linux/v4l2-framework.txt
@@ -337,6 +337,24 @@ VIDIOC_TRY_EXT_CTRLS
 	controls can be also be accessed through one (or several) V4L2 device
 	nodes.
 
+VIDIOC_DQEVENT
+VIDIOC_SUBSCRIBE_EVENT
+VIDIOC_UNSUBSCRIBE_EVENT
+
+	The events ioctls are identical to the ones defined in V4L2. They
+	behave identically, with the only exception that they deal only with
+	events generated by the sub-device. Depending on the driver, those
+	events can also be reported by one (or several) V4L2 device nodes.
+
+	Sub-device drivers that want to use events need to set the
+	V4L2_SUBDEV_USES_EVENTS v4l2_subdev::flags and initialize
+	v4l2_subdev::nevents to events queue depth before registering the
+	sub-device. After registration events can be queued as usual on the
+	v4l2_subdev::devnode device node.
+
+	To properly support events, the poll() file operation is also
+	implemented.
+
 
 I2C sub-device drivers
 ----------------------
diff --git a/drivers/media/video/v4l2-subdev.c b/drivers/media/video/v4l2-subdev.c
index 141098b..7191a4b 100644
--- a/drivers/media/video/v4l2-subdev.c
+++ b/drivers/media/video/v4l2-subdev.c
@@ -18,26 +18,64 @@
  *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
 
-#include <linux/types.h>
 #include <linux/ioctl.h>
+#include <linux/slab.h>
+#include <linux/types.h>
 #include <linux/videodev2.h>
 
 #include <media/v4l2-device.h>
 #include <media/v4l2-ioctl.h>
+#include <media/v4l2-fh.h>
+#include <media/v4l2-event.h>
 
 static int subdev_open(struct file *file)
 {
 	struct video_device *vdev = video_devdata(file);
 	struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
+	struct v4l2_fh *vfh;
+	int ret;
 
 	if (!sd->initialized)
 		return -EAGAIN;
 
+	vfh = kzalloc(sizeof(*vfh), GFP_KERNEL);
+	if (vfh == NULL)
+		return -ENOMEM;
+
+	ret = v4l2_fh_init(vfh, vdev);
+	if (ret)
+		goto err;
+
+	if (sd->flags & V4L2_SUBDEV_USES_EVENTS) {
+		ret = v4l2_event_init(vfh);
+		if (ret)
+			goto err;
+
+		ret = v4l2_event_alloc(vfh, sd->nevents);
+		if (ret)
+			goto err;
+	}
+
+	v4l2_fh_add(vfh);
+	file->private_data = vfh;
+
 	return 0;
+
+err:
+	v4l2_fh_exit(vfh);
+	kfree(vfh);
+
+	return ret;
 }
 
 static int subdev_close(struct file *file)
 {
+	struct v4l2_fh *vfh = file->private_data;
+
+	v4l2_fh_del(vfh);
+	v4l2_fh_exit(vfh);
+	kfree(vfh);
+
 	return 0;
 }
 
@@ -45,6 +83,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 {
 	struct video_device *vdev = video_devdata(file);
 	struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
+	struct v4l2_fh *fh = file->private_data;
 
 	switch (cmd) {
 	case VIDIOC_QUERYCTRL:
@@ -68,6 +107,18 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 	case VIDIOC_TRY_EXT_CTRLS:
 		return v4l2_subdev_call(sd, core, try_ext_ctrls, arg);
 
+	case VIDIOC_DQEVENT:
+		if (!(sd->flags & V4L2_SUBDEV_USES_EVENTS))
+			return -ENOIOCTLCMD;
+
+		return v4l2_event_dequeue(fh, arg, file->f_flags & O_NONBLOCK);
+
+	case VIDIOC_SUBSCRIBE_EVENT:
+		return v4l2_subdev_call(sd, core, subscribe_event, fh, arg);
+
+	case VIDIOC_UNSUBSCRIBE_EVENT:
+		return v4l2_subdev_call(sd, core, unsubscribe_event, fh, arg);
+
 	default:
 		return -ENOIOCTLCMD;
 	}
@@ -81,11 +132,29 @@ static long subdev_ioctl(struct file *file, unsigned int cmd,
 	return video_usercopy(file, cmd, arg, subdev_do_ioctl);
 }
 
+static unsigned int subdev_poll(struct file *file, poll_table *wait)
+{
+	struct video_device *vdev = video_devdata(file);
+	struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
+	struct v4l2_fh *fh = file->private_data;
+
+	if (!(sd->flags & V4L2_SUBDEV_USES_EVENTS))
+		return POLLERR;
+
+	poll_wait(file, &fh->events->wait, wait);
+
+	if (v4l2_event_pending(fh))
+		return POLLPRI;
+
+	return 0;
+}
+
 const struct v4l2_file_operations v4l2_subdev_fops = {
 	.owner = THIS_MODULE,
 	.open = subdev_open,
 	.unlocked_ioctl = subdev_ioctl,
 	.release = subdev_close,
+	.poll = subdev_poll,
 };
 
 void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops)
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 7b6edcd..803d7bd 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -36,6 +36,8 @@
 #define V4L2_SUBDEV_IR_TX_FIFO_SERVICE_REQ	0x00000001
 
 struct v4l2_device;
+struct v4l2_event_subscription;
+struct v4l2_fh;
 struct v4l2_subdev;
 struct tuner_setup;
 
@@ -134,6 +136,10 @@ struct v4l2_subdev_core_ops {
 	int (*s_register)(struct v4l2_subdev *sd, struct v4l2_dbg_register *reg);
 #endif
 	int (*s_power)(struct v4l2_subdev *sd, int on);
+	int (*subscribe_event)(struct v4l2_subdev *sd, struct v4l2_fh *fh,
+			       struct v4l2_event_subscription *sub);
+	int (*unsubscribe_event)(struct v4l2_subdev *sd, struct v4l2_fh *fh,
+				 struct v4l2_event_subscription *sub);
 };
 
 /* s_mode: switch the tuner to a specific tuner mode. Replacement of s_radio.
@@ -406,6 +412,7 @@ struct v4l2_subdev_ops {
 #define V4L2_SUBDEV_FL_IS_I2C (1U << 0)
 /* Set this flag if this subdev is a spi device. */
 #define V4L2_SUBDEV_FL_IS_SPI (1U << 1)
+#define V4L2_SUBDEV_USES_EVENTS	(1U << 2)
 
 /* Each instance of a subdev driver should create this struct, either
    stand-alone or embedded in a larger struct.
@@ -425,6 +432,8 @@ struct v4l2_subdev {
 	/* subdev device node */
 	struct video_device devnode;
 	unsigned int initialized;
+	/* number of events to be allocated on open */
+	unsigned int nevents;
 };
 
 #define vdev_to_v4l2_subdev(vdev) \
-- 
1.7.1

[RFC/PATCH 6/6] v4l: subdev: Generic ioctl support

[Laurent Pinchart]

Instead of returning an error when receiving an ioctl call with an
unsupported command, forward the call to the subdev core::ioctl handler.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 Documentation/video4linux/v4l2-framework.txt |    5 +++++
 drivers/media/video/v4l2-subdev.c            |    2 +-
 2 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/Documentation/video4linux/v4l2-framework.txt b/Documentation/video4linux/v4l2-framework.txt
index 2f5162c..3a1d6b3 100644
--- a/Documentation/video4linux/v4l2-framework.txt
+++ b/Documentation/video4linux/v4l2-framework.txt
@@ -355,6 +355,11 @@ VIDIOC_UNSUBSCRIBE_EVENT
 	To properly support events, the poll() file operation is also
 	implemented.
 
+Private ioctls
+
+	All ioctls not in the above list are passed directly to the sub-device
+	driver through the core::ioctl operation.
+
 
 I2C sub-device drivers
 ----------------------
diff --git a/drivers/media/video/v4l2-subdev.c b/drivers/media/video/v4l2-subdev.c
index 7191a4b..c32b2c4 100644
--- a/drivers/media/video/v4l2-subdev.c
+++ b/drivers/media/video/v4l2-subdev.c
@@ -120,7 +120,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 		return v4l2_subdev_call(sd, core, unsubscribe_event, fh, arg);
 
 	default:
-		return -ENOIOCTLCMD;
+		return v4l2_subdev_call(sd, core, ioctl, cmd, arg);
 	}
 
 	return 0;
-- 
1.7.1

Re: [RFC/PATCH 1/6] v4l: subdev: Don't require core operations

[Hans Verkuil]

> There's no reason to require subdevices to implement the core
> operations. Remove the check for non-NULL core operations when
> initializing the subdev.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Hans Verkuil <hverkuil@xs4all.nl>

Yeah, that test was a bit overkill.

Regards,

         Hans

> ---
>  include/media/v4l2-subdev.h |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 02c6f4d..6088316 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -437,8 +437,7 @@ static inline void v4l2_subdev_init(struct v4l2_subdev
> *sd,
>  					const struct v4l2_subdev_ops *ops)
>  {
>  	INIT_LIST_HEAD(&sd->list);
> -	/* ops->core MUST be set */
> -	BUG_ON(!ops || !ops->core);
> +	BUG_ON(!ops);
>  	sd->ops = ops;
>  	sd->v4l2_dev = NULL;
>  	sd->flags = 0;
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: [RFC/PATCH 2/6] v4l: subdev: Add device node support

[Hans Verkuil]

> Create a device node named subdevX for every registered subdev.
>
> As the device node is registered before the subdev core::s_config
> function is called, return -EGAIN on open until initialization
> completes.

The only reason we have s_config is for old i2c drivers that need to be
supported in pre-2.6.26 kernels in the mercurial repository.

I'm thinking that we should get rid of that legacy support in the git
tree. It hurts my eyes every time I see that code.

Not a blocker for this patch series, but if others agree that we should
get rid of the legacy support then I can work on that.

>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Vimarsh Zutshi <vimarsh.zutshi@nokia.com>
> ---
>  drivers/media/video/Makefile      |    2 +-
>  drivers/media/video/v4l2-common.c |    3 ++
>  drivers/media/video/v4l2-dev.c    |    5 +++
>  drivers/media/video/v4l2-device.c |   27 +++++++++++++++-
>  drivers/media/video/v4l2-subdev.c |   65
> +++++++++++++++++++++++++++++++++++++
>  include/media/v4l2-dev.h          |    3 +-
>  include/media/v4l2-subdev.h       |   10 ++++++
>  7 files changed, 112 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/media/video/v4l2-subdev.c
>
> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> index cc93859..c9c07e5 100644
> --- a/drivers/media/video/Makefile
> +++ b/drivers/media/video/Makefile
> @@ -11,7 +11,7 @@ stkwebcam-objs	:=	stk-webcam.o stk-sensor.o
>  omap2cam-objs	:=	omap24xxcam.o omap24xxcam-dma.o
>
>  videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
> -			v4l2-event.o
> +			v4l2-event.o v4l2-subdev.o
>
>  # V4L2 core modules
>
> diff --git a/drivers/media/video/v4l2-common.c
> b/drivers/media/video/v4l2-common.c
> index 4e53b0b..3032aa3 100644
> --- a/drivers/media/video/v4l2-common.c
> +++ b/drivers/media/video/v4l2-common.c
> @@ -871,6 +871,7 @@ struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct
> v4l2_device *v4l2_dev,
>
>  	/* Register with the v4l2_device which increases the module's
>  	   use count as well. */
> +	sd->initialized = 0;
>  	if (v4l2_device_register_subdev(v4l2_dev, sd))
>  		sd = NULL;
>  	/* Decrease the module use count to match the first try_module_get. */
> @@ -885,6 +886,8 @@ struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct
> v4l2_device *v4l2_dev,
>  		if (err && err != -ENOIOCTLCMD) {
>  			v4l2_device_unregister_subdev(sd);
>  			sd = NULL;
> +		} else {
> +			sd->initialized = 1;
>  		}
>  	}
>
> diff --git a/drivers/media/video/v4l2-dev.c
> b/drivers/media/video/v4l2-dev.c
> index 0ca7ec9..5a9e9df 100644
> --- a/drivers/media/video/v4l2-dev.c
> +++ b/drivers/media/video/v4l2-dev.c
> @@ -401,6 +401,8 @@ static int get_index(struct video_device *vdev)
>   *	%VFL_TYPE_VBI - Vertical blank data (undecoded)
>   *
>   *	%VFL_TYPE_RADIO - A radio card
> + *
> + *	%VFL_TYPE_SUBDEV - A subdevice
>   */
>  static int __video_register_device(struct video_device *vdev, int type,
> int nr,
>  		int warn_if_nr_in_use)
> @@ -439,6 +441,9 @@ static int __video_register_device(struct video_device
> *vdev, int type, int nr,
>  	case VFL_TYPE_RADIO:
>  		name_base = "radio";
>  		break;
> +	case VFL_TYPE_SUBDEV:
> +		name_base = "subdev";
> +		break;
>  	default:
>  		printk(KERN_ERR "%s called with unknown type: %d\n",
>  		       __func__, type);
> diff --git a/drivers/media/video/v4l2-device.c
> b/drivers/media/video/v4l2-device.c
> index 5a7dc4a..685fa82 100644
> --- a/drivers/media/video/v4l2-device.c
> +++ b/drivers/media/video/v4l2-device.c
> @@ -115,18 +115,40 @@ EXPORT_SYMBOL_GPL(v4l2_device_unregister);
>  int v4l2_device_register_subdev(struct v4l2_device *v4l2_dev,
>  						struct v4l2_subdev *sd)
>  {
> +	struct video_device *vdev;
> +	int ret;
> +
>  	/* Check for valid input */
>  	if (v4l2_dev == NULL || sd == NULL || !sd->name[0])
>  		return -EINVAL;
> +
>  	/* Warn if we apparently re-register a subdev */
>  	WARN_ON(sd->v4l2_dev != NULL);
> +
>  	if (!try_module_get(sd->owner))
>  		return -ENODEV;
> +
>  	sd->v4l2_dev = v4l2_dev;
>  	spin_lock(&v4l2_dev->lock);
>  	list_add_tail(&sd->list, &v4l2_dev->subdevs);
>  	spin_unlock(&v4l2_dev->lock);
> -	return 0;
> +
> +	/* Register the device node. */
> +	vdev = &sd->devnode;
> +	snprintf(vdev->name, sizeof(vdev->name), "subdev");

Hmm, perhaps we should be more creative here. For example:

snprintf(vdev->name, sizeof(vdev->name), "subdev %s", sd->name);

> +	vdev->parent = v4l2_dev->dev;
> +	vdev->fops = &v4l2_subdev_fops;
> +	vdev->release = video_device_release_empty;
> +	ret = video_register_device(vdev, VFL_TYPE_SUBDEV, -1);
> +	if (ret < 0) {
> +		spin_lock(&v4l2_dev->lock);
> +		list_del(&sd->list);
> +		spin_unlock(&v4l2_dev->lock);
> +		sd->v4l2_dev = NULL;
> +		module_put(sd->owner);

You can just call v4l2_device_unregister_subdev(sd) here. The call
to video_unregister_device will know that the registration failed and will
just return.

> +	}
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(v4l2_device_register_subdev);
>
> @@ -135,10 +157,13 @@ void v4l2_device_unregister_subdev(struct
> v4l2_subdev *sd)
>  	/* return if it isn't registered */
>  	if (sd == NULL || sd->v4l2_dev == NULL)
>  		return;
> +
>  	spin_lock(&sd->v4l2_dev->lock);
>  	list_del(&sd->list);
>  	spin_unlock(&sd->v4l2_dev->lock);
>  	sd->v4l2_dev = NULL;
> +
>  	module_put(sd->owner);
> +	video_unregister_device(&sd->devnode);
>  }
>  EXPORT_SYMBOL_GPL(v4l2_device_unregister_subdev);
> diff --git a/drivers/media/video/v4l2-subdev.c
> b/drivers/media/video/v4l2-subdev.c
> new file mode 100644
> index 0000000..a048161
> --- /dev/null
> +++ b/drivers/media/video/v4l2-subdev.c
> @@ -0,0 +1,65 @@
> +/*
> + *  V4L2 subdevice support.
> + *
> + *  Copyright (C) 2009  Laurent Pinchart

2009 -> 2010

Might be wrong elsewhere as well.

> <laurent.pinchart@ideasonboard.com>
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; either version 2 of the License, or
> + *  (at your option) any later version.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; if not, write to the Free Software
> + *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307
> USA
> + */
> +
> +#include <linux/types.h>
> +#include <linux/ioctl.h>
> +#include <linux/videodev2.h>
> +
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-ioctl.h>
> +
> +static int subdev_open(struct file *file)
> +{
> +	struct video_device *vdev = video_devdata(file);
> +	struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
> +
> +	if (!sd->initialized)
> +		return -EAGAIN;
> +
> +	return 0;
> +}
> +
> +static int subdev_close(struct file *file)
> +{
> +	return 0;
> +}
> +
> +static long subdev_do_ioctl(struct file *file, unsigned int cmd, void
> *arg)
> +{
> +	switch (cmd) {
> +	default:
> +		return -ENOIOCTLCMD;
> +	}
> +
> +	return 0;
> +}
> +
> +static long subdev_ioctl(struct file *file, unsigned int cmd,
> +	unsigned long arg)
> +{
> +	return video_usercopy(file, cmd, arg, subdev_do_ioctl);
> +}
> +
> +const struct v4l2_file_operations v4l2_subdev_fops = {
> +	.owner = THIS_MODULE,
> +	.open = subdev_open,
> +	.unlocked_ioctl = subdev_ioctl,
> +	.release = subdev_close,
> +};
> diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
> index bebe44b..eee1e2b 100644
> --- a/include/media/v4l2-dev.h
> +++ b/include/media/v4l2-dev.h
> @@ -22,7 +22,8 @@
>  #define VFL_TYPE_VBI		1
>  #define VFL_TYPE_RADIO		2
>  #define VFL_TYPE_VTX		3
> -#define VFL_TYPE_MAX		4
> +#define VFL_TYPE_SUBDEV		4
> +#define VFL_TYPE_MAX		5
>
>  struct v4l2_ioctl_callbacks;
>  struct video_device;
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 6088316..00010bd 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -22,6 +22,7 @@
>  #define _V4L2_SUBDEV_H
>
>  #include <media/v4l2-common.h>
> +#include <media/v4l2-dev.h>
>  #include <media/v4l2-mediabus.h>
>
>  /* generic v4l2_device notify callback notification values */
> @@ -421,8 +422,16 @@ struct v4l2_subdev {
>  	u32 grp_id;
>  	/* pointer to private data */
>  	void *priv;
> +	/* subdev device node */
> +	struct video_device devnode;
> +	unsigned int initialized;
>  };
>
> +#define vdev_to_v4l2_subdev(vdev) \
> +	container_of(vdev, struct v4l2_subdev, devnode)
> +
> +extern const struct v4l2_file_operations v4l2_subdev_fops;
> +
>  static inline void v4l2_set_subdevdata(struct v4l2_subdev *sd, void *p)
>  {
>  	sd->priv = p;
> @@ -444,6 +453,7 @@ static inline void v4l2_subdev_init(struct v4l2_subdev
> *sd,
>  	sd->name[0] = '\0';
>  	sd->grp_id = 0;
>  	sd->priv = NULL;
> +	sd->initialized = 0;
>  }
>
>  /* Call an ops of a v4l2_subdev, doing the right checks against
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Reviewed-by: Hans Verkuil <hverkuil@xs4all.nl>

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: [RFC/PATCH 3/6] v4l: subdev: Uninline the v4l2_subdev_init function

[Hans Verkuil]

> The function isn't small or performance sensitive enough to be inlined.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Hans Verkuil <hverkuil@xs4all.nl>

Looks good!

       Hans

> ---
>  drivers/media/video/v4l2-subdev.c |   14 ++++++++++++++
>  include/media/v4l2-subdev.h       |   15 ++-------------
>  2 files changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/media/video/v4l2-subdev.c
> b/drivers/media/video/v4l2-subdev.c
> index a048161..a3672f0 100644
> --- a/drivers/media/video/v4l2-subdev.c
> +++ b/drivers/media/video/v4l2-subdev.c
> @@ -63,3 +63,17 @@ const struct v4l2_file_operations v4l2_subdev_fops = {
>  	.unlocked_ioctl = subdev_ioctl,
>  	.release = subdev_close,
>  };
> +
> +void v4l2_subdev_init(struct v4l2_subdev *sd, const struct
> v4l2_subdev_ops *ops)
> +{
> +	INIT_LIST_HEAD(&sd->list);
> +	BUG_ON(!ops);
> +	sd->ops = ops;
> +	sd->v4l2_dev = NULL;
> +	sd->flags = 0;
> +	sd->name[0] = '\0';
> +	sd->grp_id = 0;
> +	sd->priv = NULL;
> +	sd->initialized = 0;
> +}
> +EXPORT_SYMBOL(v4l2_subdev_init);
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 00010bd..7b6edcd 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -442,19 +442,8 @@ static inline void *v4l2_get_subdevdata(const struct
> v4l2_subdev *sd)
>  	return sd->priv;
>  }
>
> -static inline void v4l2_subdev_init(struct v4l2_subdev *sd,
> -					const struct v4l2_subdev_ops *ops)
> -{
> -	INIT_LIST_HEAD(&sd->list);
> -	BUG_ON(!ops);
> -	sd->ops = ops;
> -	sd->v4l2_dev = NULL;
> -	sd->flags = 0;
> -	sd->name[0] = '\0';
> -	sd->grp_id = 0;
> -	sd->priv = NULL;
> -	sd->initialized = 0;
> -}
> +void v4l2_subdev_init(struct v4l2_subdev *sd,
> +		      const struct v4l2_subdev_ops *ops);
>
>  /* Call an ops of a v4l2_subdev, doing the right checks against
>     NULL pointers.
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: [RFC/PATCH 4/6] v4l: subdev: Control ioctls support

[Hans Verkuil]

> Pass the control-related ioctls to the subdev driver through the core
> operations.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  Documentation/video4linux/v4l2-framework.txt |   24
> ++++++++++++++++++++++++
>  drivers/media/video/v4l2-subdev.c            |   24
> ++++++++++++++++++++++++
>  2 files changed, 48 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/video4linux/v4l2-framework.txt
> b/Documentation/video4linux/v4l2-framework.txt
> index e831aac..f315858 100644
> --- a/Documentation/video4linux/v4l2-framework.txt
> +++ b/Documentation/video4linux/v4l2-framework.txt
> @@ -314,6 +314,30 @@ controlled through GPIO pins. This distinction is
> only relevant when setting
>  up the device, but once the subdev is registered it is completely
> transparent.
>
>
> +V4L2 sub-device userspace API
> +-----------------------------
> +
> +Beside exposing a kernel API through the v4l2_subdev_ops structure, V4L2
> +sub-devices can also be controlled directly by userspace applications.
> +
> +When a sub-device is registered, a device node named subdevX is created
> in /dev.
> +The device node handles a subset of the V4L2 API.
> +
> +VIDIOC_QUERYCTRL
> +VIDIOC_QUERYMENU
> +VIDIOC_G_CTRL
> +VIDIOC_S_CTRL
> +VIDIOC_G_EXT_CTRLS
> +VIDIOC_S_EXT_CTRLS
> +VIDIOC_TRY_EXT_CTRLS
> +
> +	The controls ioctls are identical to the ones defined in V4L2. They
> +	behave identically, with the only exception that they deal only with
> +	controls implemented in the sub-device. Depending on the driver, those
> +	controls can be also be accessed through one (or several) V4L2 device
> +	nodes.
> +
> +
>  I2C sub-device drivers
>  ----------------------
>
> diff --git a/drivers/media/video/v4l2-subdev.c
> b/drivers/media/video/v4l2-subdev.c
> index a3672f0..141098b 100644
> --- a/drivers/media/video/v4l2-subdev.c
> +++ b/drivers/media/video/v4l2-subdev.c
> @@ -43,7 +43,31 @@ static int subdev_close(struct file *file)
>
>  static long subdev_do_ioctl(struct file *file, unsigned int cmd, void
> *arg)
>  {
> +	struct video_device *vdev = video_devdata(file);
> +	struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
> +
>  	switch (cmd) {
> +	case VIDIOC_QUERYCTRL:
> +		return v4l2_subdev_call(sd, core, queryctrl, arg);
> +
> +	case VIDIOC_QUERYMENU:
> +		return v4l2_subdev_call(sd, core, querymenu, arg);
> +
> +	case VIDIOC_G_CTRL:
> +		return v4l2_subdev_call(sd, core, g_ctrl, arg);
> +
> +	case VIDIOC_S_CTRL:
> +		return v4l2_subdev_call(sd, core, s_ctrl, arg);
> +
> +	case VIDIOC_G_EXT_CTRLS:
> +		return v4l2_subdev_call(sd, core, g_ext_ctrls, arg);
> +
> +	case VIDIOC_S_EXT_CTRLS:
> +		return v4l2_subdev_call(sd, core, s_ext_ctrls, arg);
> +
> +	case VIDIOC_TRY_EXT_CTRLS:
> +		return v4l2_subdev_call(sd, core, try_ext_ctrls, arg);
> +
>  	default:
>  		return -ENOIOCTLCMD;
>  	}
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

This should simplify substantially once the control framework is in place.

IMHO the control framework should go in first, then this code, updated for
the control framework.

Reviewed-by: Hans Verkuil <hverkuil@xs4all.nl>

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: [RFC/PATCH 4/6] v4l: subdev: Control ioctls support

[Laurent Pinchart]

Hi Hans,

Thanks for the quick review.

On Wednesday 07 July 2010 14:33:52 Hans Verkuil wrote:
> > Pass the control-related ioctls to the subdev driver through the core
> > operations.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

[snip]

> This should simplify substantially once the control framework is in place.

Definitely.

> IMHO the control framework should go in first, then this code, updated for
> the control framework.

I'm fine with either way.

-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH 5/6] v4l: subdev: Events support

[Hans Verkuil]

> From: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
>
> Provide v4l2_subdevs with v4l2_event support. Subdev drivers only need
> very
> little to support events.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> Signed-off-by: David Cohen <david.cohen@nokia.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  Documentation/video4linux/v4l2-framework.txt |   18 +++++++
>  drivers/media/video/v4l2-subdev.c            |   71
> +++++++++++++++++++++++++-
>  include/media/v4l2-subdev.h                  |    9 +++
>  3 files changed, 97 insertions(+), 1 deletions(-)
>
> diff --git a/Documentation/video4linux/v4l2-framework.txt
> b/Documentation/video4linux/v4l2-framework.txt
> index f315858..2f5162c 100644
> --- a/Documentation/video4linux/v4l2-framework.txt
> +++ b/Documentation/video4linux/v4l2-framework.txt
> @@ -337,6 +337,24 @@ VIDIOC_TRY_EXT_CTRLS
>  	controls can be also be accessed through one (or several) V4L2 device
>  	nodes.
>
> +VIDIOC_DQEVENT
> +VIDIOC_SUBSCRIBE_EVENT
> +VIDIOC_UNSUBSCRIBE_EVENT
> +
> +	The events ioctls are identical to the ones defined in V4L2. They
> +	behave identically, with the only exception that they deal only with
> +	events generated by the sub-device. Depending on the driver, those
> +	events can also be reported by one (or several) V4L2 device nodes.
> +
> +	Sub-device drivers that want to use events need to set the
> +	V4L2_SUBDEV_USES_EVENTS v4l2_subdev::flags and initialize
> +	v4l2_subdev::nevents to events queue depth before registering the
> +	sub-device. After registration events can be queued as usual on the
> +	v4l2_subdev::devnode device node.
> +
> +	To properly support events, the poll() file operation is also
> +	implemented.
> +
>
>  I2C sub-device drivers
>  ----------------------
> diff --git a/drivers/media/video/v4l2-subdev.c
> b/drivers/media/video/v4l2-subdev.c
> index 141098b..7191a4b 100644
> --- a/drivers/media/video/v4l2-subdev.c
> +++ b/drivers/media/video/v4l2-subdev.c
> @@ -18,26 +18,64 @@
>   *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307
> USA
>   */
>
> -#include <linux/types.h>
>  #include <linux/ioctl.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
>  #include <linux/videodev2.h>
>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-ioctl.h>
> +#include <media/v4l2-fh.h>
> +#include <media/v4l2-event.h>
>
>  static int subdev_open(struct file *file)
>  {
>  	struct video_device *vdev = video_devdata(file);
>  	struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
> +	struct v4l2_fh *vfh;
> +	int ret;
>
>  	if (!sd->initialized)
>  		return -EAGAIN;
>
> +	vfh = kzalloc(sizeof(*vfh), GFP_KERNEL);
> +	if (vfh == NULL)
> +		return -ENOMEM;
> +
> +	ret = v4l2_fh_init(vfh, vdev);
> +	if (ret)
> +		goto err;
> +
> +	if (sd->flags & V4L2_SUBDEV_USES_EVENTS) {
> +		ret = v4l2_event_init(vfh);
> +		if (ret)
> +			goto err;
> +
> +		ret = v4l2_event_alloc(vfh, sd->nevents);
> +		if (ret)
> +			goto err;
> +	}
> +
> +	v4l2_fh_add(vfh);
> +	file->private_data = vfh;
> +
>  	return 0;
> +
> +err:
> +	v4l2_fh_exit(vfh);
> +	kfree(vfh);
> +
> +	return ret;
>  }
>
>  static int subdev_close(struct file *file)
>  {
> +	struct v4l2_fh *vfh = file->private_data;
> +
> +	v4l2_fh_del(vfh);
> +	v4l2_fh_exit(vfh);
> +	kfree(vfh);
> +
>  	return 0;
>  }
>
> @@ -45,6 +83,7 @@ static long subdev_do_ioctl(struct file *file, unsigned
> int cmd, void *arg)
>  {
>  	struct video_device *vdev = video_devdata(file);
>  	struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
> +	struct v4l2_fh *fh = file->private_data;
>
>  	switch (cmd) {
>  	case VIDIOC_QUERYCTRL:
> @@ -68,6 +107,18 @@ static long subdev_do_ioctl(struct file *file,
> unsigned int cmd, void *arg)
>  	case VIDIOC_TRY_EXT_CTRLS:
>  		return v4l2_subdev_call(sd, core, try_ext_ctrls, arg);
>
> +	case VIDIOC_DQEVENT:
> +		if (!(sd->flags & V4L2_SUBDEV_USES_EVENTS))
> +			return -ENOIOCTLCMD;
> +
> +		return v4l2_event_dequeue(fh, arg, file->f_flags & O_NONBLOCK);
> +
> +	case VIDIOC_SUBSCRIBE_EVENT:
> +		return v4l2_subdev_call(sd, core, subscribe_event, fh, arg);
> +
> +	case VIDIOC_UNSUBSCRIBE_EVENT:
> +		return v4l2_subdev_call(sd, core, unsubscribe_event, fh, arg);
> +
>  	default:
>  		return -ENOIOCTLCMD;
>  	}
> @@ -81,11 +132,29 @@ static long subdev_ioctl(struct file *file, unsigned
> int cmd,
>  	return video_usercopy(file, cmd, arg, subdev_do_ioctl);
>  }
>
> +static unsigned int subdev_poll(struct file *file, poll_table *wait)
> +{
> +	struct video_device *vdev = video_devdata(file);
> +	struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
> +	struct v4l2_fh *fh = file->private_data;
> +
> +	if (!(sd->flags & V4L2_SUBDEV_USES_EVENTS))
> +		return POLLERR;
> +
> +	poll_wait(file, &fh->events->wait, wait);
> +
> +	if (v4l2_event_pending(fh))
> +		return POLLPRI;
> +
> +	return 0;
> +}
> +
>  const struct v4l2_file_operations v4l2_subdev_fops = {
>  	.owner = THIS_MODULE,
>  	.open = subdev_open,
>  	.unlocked_ioctl = subdev_ioctl,
>  	.release = subdev_close,
> +	.poll = subdev_poll,
>  };
>
>  void v4l2_subdev_init(struct v4l2_subdev *sd, const struct
> v4l2_subdev_ops *ops)
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 7b6edcd..803d7bd 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -36,6 +36,8 @@
>  #define V4L2_SUBDEV_IR_TX_FIFO_SERVICE_REQ	0x00000001
>
>  struct v4l2_device;
> +struct v4l2_event_subscription;
> +struct v4l2_fh;
>  struct v4l2_subdev;
>  struct tuner_setup;
>
> @@ -134,6 +136,10 @@ struct v4l2_subdev_core_ops {
>  	int (*s_register)(struct v4l2_subdev *sd, struct v4l2_dbg_register
> *reg);
>  #endif
>  	int (*s_power)(struct v4l2_subdev *sd, int on);
> +	int (*subscribe_event)(struct v4l2_subdev *sd, struct v4l2_fh *fh,
> +			       struct v4l2_event_subscription *sub);
> +	int (*unsubscribe_event)(struct v4l2_subdev *sd, struct v4l2_fh *fh,
> +				 struct v4l2_event_subscription *sub);
>  };
>
>  /* s_mode: switch the tuner to a specific tuner mode. Replacement of
> s_radio.
> @@ -406,6 +412,7 @@ struct v4l2_subdev_ops {
>  #define V4L2_SUBDEV_FL_IS_I2C (1U << 0)
>  /* Set this flag if this subdev is a spi device. */
>  #define V4L2_SUBDEV_FL_IS_SPI (1U << 1)
> +#define V4L2_SUBDEV_USES_EVENTS	(1U << 2)

Recommend renaming to V4L2_SUBDEV_FL_HAS_EVENTS.

The prefix is consistent with the other flags and 'has events' feels more
natural to me.

>
>  /* Each instance of a subdev driver should create this struct, either
>     stand-alone or embedded in a larger struct.
> @@ -425,6 +432,8 @@ struct v4l2_subdev {
>  	/* subdev device node */
>  	struct video_device devnode;
>  	unsigned int initialized;
> +	/* number of events to be allocated on open */
> +	unsigned int nevents;
>  };
>
>  #define vdev_to_v4l2_subdev(vdev) \
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Reviewed-by: Hans Verkuil <hverkuil@xs4all.nl>

Regards,

        Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: [RFC/PATCH 6/6] v4l: subdev: Generic ioctl support

[Hans Verkuil]

> Instead of returning an error when receiving an ioctl call with an
> unsupported command, forward the call to the subdev core::ioctl handler.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  Documentation/video4linux/v4l2-framework.txt |    5 +++++
>  drivers/media/video/v4l2-subdev.c            |    2 +-
>  2 files changed, 6 insertions(+), 1 deletions(-)
>
> diff --git a/Documentation/video4linux/v4l2-framework.txt
> b/Documentation/video4linux/v4l2-framework.txt
> index 2f5162c..3a1d6b3 100644
> --- a/Documentation/video4linux/v4l2-framework.txt
> +++ b/Documentation/video4linux/v4l2-framework.txt
> @@ -355,6 +355,11 @@ VIDIOC_UNSUBSCRIBE_EVENT
>  	To properly support events, the poll() file operation is also
>  	implemented.
>
> +Private ioctls
> +
> +	All ioctls not in the above list are passed directly to the sub-device
> +	driver through the core::ioctl operation.
> +
>
>  I2C sub-device drivers
>  ----------------------
> diff --git a/drivers/media/video/v4l2-subdev.c
> b/drivers/media/video/v4l2-subdev.c
> index 7191a4b..c32b2c4 100644
> --- a/drivers/media/video/v4l2-subdev.c
> +++ b/drivers/media/video/v4l2-subdev.c
> @@ -120,7 +120,7 @@ static long subdev_do_ioctl(struct file *file,
> unsigned int cmd, void *arg)
>  		return v4l2_subdev_call(sd, core, unsubscribe_event, fh, arg);
>
>  	default:
> -		return -ENOIOCTLCMD;
> +		return v4l2_subdev_call(sd, core, ioctl, cmd, arg);
>  	}
>
>  	return 0;
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Reviewed-by: Hans Verkuil <hverkuil@xs4all.nl>

Nice to see how everything fits together :-)

Regards,

        Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: [RFC/PATCH 2/6] v4l: subdev: Add device node support

[Hans Verkuil]

> Create a device node named subdevX for every registered subdev.
>
> As the device node is registered before the subdev core::s_config
> function is called, return -EGAIN on open until initialization
> completes.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Vimarsh Zutshi <vimarsh.zutshi@nokia.com>
> ---
>  drivers/media/video/Makefile      |    2 +-
>  drivers/media/video/v4l2-common.c |    3 ++
>  drivers/media/video/v4l2-dev.c    |    5 +++
>  drivers/media/video/v4l2-device.c |   27 +++++++++++++++-
>  drivers/media/video/v4l2-subdev.c |   65
> +++++++++++++++++++++++++++++++++++++
>  include/media/v4l2-dev.h          |    3 +-
>  include/media/v4l2-subdev.h       |   10 ++++++
>  7 files changed, 112 insertions(+), 3 deletions(-)

...

> diff --git a/drivers/media/video/v4l2-device.c
> b/drivers/media/video/v4l2-device.c
> index 5a7dc4a..685fa82 100644
> --- a/drivers/media/video/v4l2-device.c
> +++ b/drivers/media/video/v4l2-device.c
> @@ -115,18 +115,40 @@ EXPORT_SYMBOL_GPL(v4l2_device_unregister);
>  int v4l2_device_register_subdev(struct v4l2_device *v4l2_dev,
>  						struct v4l2_subdev *sd)
>  {
> +	struct video_device *vdev;
> +	int ret;
> +
>  	/* Check for valid input */
>  	if (v4l2_dev == NULL || sd == NULL || !sd->name[0])
>  		return -EINVAL;
> +
>  	/* Warn if we apparently re-register a subdev */
>  	WARN_ON(sd->v4l2_dev != NULL);
> +
>  	if (!try_module_get(sd->owner))
>  		return -ENODEV;
> +
>  	sd->v4l2_dev = v4l2_dev;
>  	spin_lock(&v4l2_dev->lock);
>  	list_add_tail(&sd->list, &v4l2_dev->subdevs);
>  	spin_unlock(&v4l2_dev->lock);
> -	return 0;
> +
> +	/* Register the device node. */
> +	vdev = &sd->devnode;
> +	snprintf(vdev->name, sizeof(vdev->name), "subdev");
> +	vdev->parent = v4l2_dev->dev;
> +	vdev->fops = &v4l2_subdev_fops;
> +	vdev->release = video_device_release_empty;
> +	ret = video_register_device(vdev, VFL_TYPE_SUBDEV, -1);
> +	if (ret < 0) {
> +		spin_lock(&v4l2_dev->lock);
> +		list_del(&sd->list);
> +		spin_unlock(&v4l2_dev->lock);
> +		sd->v4l2_dev = NULL;
> +		module_put(sd->owner);
> +	}
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(v4l2_device_register_subdev);
>

I'm missing one thing here: in this code the subdev device node is always
registered. But for most subdev drivers there is no need for a device
node. This should really be explicitly turned on by the subdev driver
itself.

Regards,

       Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: [RFC/PATCH 2/6] v4l: subdev: Add device node support

[Laurent Pinchart]

Hi Hans,

Thanks for the quick review.

On Wednesday 07 July 2010 14:30:45 Hans Verkuil wrote:
> > Create a device node named subdevX for every registered subdev.
> > 
> > As the device node is registered before the subdev core::s_config
> > function is called, return -EGAIN on open until initialization
> > completes.
> 
> The only reason we have s_config is for old i2c drivers that need to be
> supported in pre-2.6.26 kernels in the mercurial repository.
> 
> I'm thinking that we should get rid of that legacy support in the git
> tree. It hurts my eyes every time I see that code.
> 
> Not a blocker for this patch series, but if others agree that we should
> get rid of the legacy support then I can work on that.

Some (most ?) I2C sensors need to be accessed during probe. This involves 
powering the sensor up, which is handled by a board code function called 
through a platform data function pointer.

On the OMAP3 ISP the sensor clock can be generated by the ISP. This means that 
board code needs to call an ISP (exported) function to turn the clock on. To 
do so we currently retrieve a pointer to the ISP device through the subdev's 
parent v4l2_device, embedded in the ISP device structure. This requires the 
subdev to be registered, otherwise its parent will be NULL. For that reason 
we're still using the core::s_config operation.

This is obviously not an ideal situation, and I'm open to suggestions on how 
to solve it without core::s_config. One possibility would be to use a global 
ISP device pointer in the board code, as there's only one ISP device in the 
system. It feels a bit hackish though.

[snip]

> > diff --git a/drivers/media/video/v4l2-device.c
> > b/drivers/media/video/v4l2-device.c
> > index 5a7dc4a..685fa82 100644
> > --- a/drivers/media/video/v4l2-device.c
> > +++ b/drivers/media/video/v4l2-device.c

[snip]

> > @@ -115,18 +115,40 @@ EXPORT_SYMBOL_GPL(v4l2_device_unregister);
> > 
> >  int v4l2_device_register_subdev(struct v4l2_device *v4l2_dev,
> >  
> >  						struct v4l2_subdev *sd)
> >  
> >  {
> > 
> > +	struct video_device *vdev;
> > +	int ret;
> > +
> >  	/* Check for valid input */
> >  	if (v4l2_dev == NULL || sd == NULL || !sd->name[0])
> >  		return -EINVAL;
> > +
> >  	/* Warn if we apparently re-register a subdev */
> >  	WARN_ON(sd->v4l2_dev != NULL);
> > +
> >  	if (!try_module_get(sd->owner))
> >  		return -ENODEV;
> > +
> >  	sd->v4l2_dev = v4l2_dev;
> >  	spin_lock(&v4l2_dev->lock);
> >  	list_add_tail(&sd->list, &v4l2_dev->subdevs);
> >  	spin_unlock(&v4l2_dev->lock);
> > 
> > -	return 0;
> > +
> > +	/* Register the device node. */
> > +	vdev = &sd->devnode;
> > +	snprintf(vdev->name, sizeof(vdev->name), "subdev");
> 
> Hmm, perhaps we should be more creative here. For example:
> 
> snprintf(vdev->name, sizeof(vdev->name), "subdev %s", sd->name);

I'm definitely open to alternative name suggestions. For instance, I'm not 
sure to be happy with /dev/subdevX. /dev/v4l2-subdevX might be better.

As for vdev->name, your suggestion sounds good.

> > +	vdev->parent = v4l2_dev->dev;
> > +	vdev->fops = &v4l2_subdev_fops;
> > +	vdev->release = video_device_release_empty;
> > +	ret = video_register_device(vdev, VFL_TYPE_SUBDEV, -1);
> > +	if (ret < 0) {
> > +		spin_lock(&v4l2_dev->lock);
> > +		list_del(&sd->list);
> > +		spin_unlock(&v4l2_dev->lock);
> > +		sd->v4l2_dev = NULL;
> > +		module_put(sd->owner);
> 
> You can just call v4l2_device_unregister_subdev(sd) here. The call
> to video_unregister_device will know that the registration failed and will
> just return.

Good point, thanks.

> > +	}
> > +
> > +	return ret;
> > 
> >  }
> >  EXPORT_SYMBOL_GPL(v4l2_device_register_subdev);
> > 

[snip]

> > diff --git a/drivers/media/video/v4l2-subdev.c
> > b/drivers/media/video/v4l2-subdev.c
> > new file mode 100644
> > index 0000000..a048161
> > --- /dev/null
> > +++ b/drivers/media/video/v4l2-subdev.c
> > @@ -0,0 +1,65 @@
> > +/*
> > + *  V4L2 subdevice support.
> > + *
> > + *  Copyright (C) 2009  Laurent Pinchart
> 
> 2009 -> 2010
> 
> Might be wrong elsewhere as well.

I'll fix that.

-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH 2/6] v4l: subdev: Add device node support

[Hans Verkuil]

> Hi Hans,
>
> Thanks for the quick review.
>
> On Wednesday 07 July 2010 14:30:45 Hans Verkuil wrote:
>> > Create a device node named subdevX for every registered subdev.
>> >
>> > As the device node is registered before the subdev core::s_config
>> > function is called, return -EGAIN on open until initialization
>> > completes.
>>
>> The only reason we have s_config is for old i2c drivers that need to be
>> supported in pre-2.6.26 kernels in the mercurial repository.
>>
>> I'm thinking that we should get rid of that legacy support in the git
>> tree. It hurts my eyes every time I see that code.
>>
>> Not a blocker for this patch series, but if others agree that we should
>> get rid of the legacy support then I can work on that.
>
> Some (most ?) I2C sensors need to be accessed during probe. This involves
> powering the sensor up, which is handled by a board code function called
> through a platform data function pointer.
>
> On the OMAP3 ISP the sensor clock can be generated by the ISP. This means
> that
> board code needs to call an ISP (exported) function to turn the clock on.
> To
> do so we currently retrieve a pointer to the ISP device through the
> subdev's
> parent v4l2_device, embedded in the ISP device structure. This requires
> the
> subdev to be registered, otherwise its parent will be NULL. For that
> reason
> we're still using the core::s_config operation.
>
> This is obviously not an ideal situation, and I'm open to suggestions on
> how
> to solve it without core::s_config. One possibility would be to use a
> global
> ISP device pointer in the board code, as there's only one ISP device in
> the
> system. It feels a bit hackish though.

Or make the v4l2_device pointer part of the platform data? That way the
subdev driver has access to the v4l2_device pointer in a fairly clean
manner.

>
> [snip]
>
>> > diff --git a/drivers/media/video/v4l2-device.c
>> > b/drivers/media/video/v4l2-device.c
>> > index 5a7dc4a..685fa82 100644
>> > --- a/drivers/media/video/v4l2-device.c
>> > +++ b/drivers/media/video/v4l2-device.c
>
> [snip]
>
>> > @@ -115,18 +115,40 @@ EXPORT_SYMBOL_GPL(v4l2_device_unregister);
>> >
>> >  int v4l2_device_register_subdev(struct v4l2_device *v4l2_dev,
>> >
>> >  						struct v4l2_subdev *sd)
>> >
>> >  {
>> >
>> > +	struct video_device *vdev;
>> > +	int ret;
>> > +
>> >  	/* Check for valid input */
>> >  	if (v4l2_dev == NULL || sd == NULL || !sd->name[0])
>> >  		return -EINVAL;
>> > +
>> >  	/* Warn if we apparently re-register a subdev */
>> >  	WARN_ON(sd->v4l2_dev != NULL);
>> > +
>> >  	if (!try_module_get(sd->owner))
>> >  		return -ENODEV;
>> > +
>> >  	sd->v4l2_dev = v4l2_dev;
>> >  	spin_lock(&v4l2_dev->lock);
>> >  	list_add_tail(&sd->list, &v4l2_dev->subdevs);
>> >  	spin_unlock(&v4l2_dev->lock);
>> >
>> > -	return 0;
>> > +
>> > +	/* Register the device node. */
>> > +	vdev = &sd->devnode;
>> > +	snprintf(vdev->name, sizeof(vdev->name), "subdev");
>>
>> Hmm, perhaps we should be more creative here. For example:
>>
>> snprintf(vdev->name, sizeof(vdev->name), "subdev %s", sd->name);
>
> I'm definitely open to alternative name suggestions. For instance, I'm not
> sure to be happy with /dev/subdevX. /dev/v4l2-subdevX might be better.

I think I would go with v4l-subdevX. I agree, that's better than the
overly generic 'subdevX'.

>
> As for vdev->name, your suggestion sounds good.
>
>> > +	vdev->parent = v4l2_dev->dev;
>> > +	vdev->fops = &v4l2_subdev_fops;
>> > +	vdev->release = video_device_release_empty;
>> > +	ret = video_register_device(vdev, VFL_TYPE_SUBDEV, -1);
>> > +	if (ret < 0) {
>> > +		spin_lock(&v4l2_dev->lock);
>> > +		list_del(&sd->list);
>> > +		spin_unlock(&v4l2_dev->lock);
>> > +		sd->v4l2_dev = NULL;
>> > +		module_put(sd->owner);
>>
>> You can just call v4l2_device_unregister_subdev(sd) here. The call
>> to video_unregister_device will know that the registration failed and
>> will
>> just return.
>
> Good point, thanks.
>
>> > +	}
>> > +
>> > +	return ret;
>> >
>> >  }
>> >  EXPORT_SYMBOL_GPL(v4l2_device_register_subdev);
>> >
>
> [snip]
>
>> > diff --git a/drivers/media/video/v4l2-subdev.c
>> > b/drivers/media/video/v4l2-subdev.c
>> > new file mode 100644
>> > index 0000000..a048161
>> > --- /dev/null
>> > +++ b/drivers/media/video/v4l2-subdev.c
>> > @@ -0,0 +1,65 @@
>> > +/*
>> > + *  V4L2 subdevice support.
>> > + *
>> > + *  Copyright (C) 2009  Laurent Pinchart
>>
>> 2009 -> 2010
>>
>> Might be wrong elsewhere as well.
>
> I'll fix that.
>
> --
> Regards,
>
> Laurent Pinchart
>

Regards,

        Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: [RFC/PATCH 2/6] v4l: subdev: Add device node support

[Laurent Pinchart]

Hi Hans,

On Wednesday 07 July 2010 14:43:08 Hans Verkuil wrote:
> > Create a device node named subdevX for every registered subdev.
> > 
> > As the device node is registered before the subdev core::s_config
> > function is called, return -EGAIN on open until initialization
> > completes.

[snip]

> I'm missing one thing here: in this code the subdev device node is always
> registered. But for most subdev drivers there is no need for a device
> node. This should really be explicitly turned on by the subdev driver
> itself.

I'll fix that with a subdev flag.

-- 
Regards,

Laurent Pinchart

RE: [RFC/PATCH 1/6] v4l: subdev: Don't require core operations

[Karicheri, Muralidharan]

Laurent,

This is a good fix. For example, soc based sub devices may not
have core ops implemented. 

Murali Karicheri
Software Design Engineer
Texas Instruments Inc.
Germantown, MD 20874
email: m-karicheri2@ti.com

>-----Original Message-----
>From: linux-media-owner@vger.kernel.org [mailto:linux-media-
>owner@vger.kernel.org] On Behalf Of Laurent Pinchart
>Sent: Wednesday, July 07, 2010 7:53 AM
>To: linux-media@vger.kernel.org
>Cc: sakari.ailus@maxwell.research.nokia.com
>Subject: [RFC/PATCH 1/6] v4l: subdev: Don't require core operations
>
>There's no reason to require subdevices to implement the core
>operations. Remove the check for non-NULL core operations when
>initializing the subdev.
>
>Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>---
> include/media/v4l2-subdev.h |    3 +--
> 1 files changed, 1 insertions(+), 2 deletions(-)
>
>diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>index 02c6f4d..6088316 100644
>--- a/include/media/v4l2-subdev.h
>+++ b/include/media/v4l2-subdev.h
>@@ -437,8 +437,7 @@ static inline void v4l2_subdev_init(struct v4l2_subdev
>*sd,
> 					const struct v4l2_subdev_ops *ops)
> {
> 	INIT_LIST_HEAD(&sd->list);
>-	/* ops->core MUST be set */
>-	BUG_ON(!ops || !ops->core);
>+	BUG_ON(!ops);
> 	sd->ops = ops;
> 	sd->v4l2_dev = NULL;
> 	sd->flags = 0;
>--
>1.7.1
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-media" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [RFC/PATCH 2/6] v4l: subdev: Add device node support

[Karicheri, Muralidharan]

>v4l2_device *v4l2_dev,
> 		if (err && err != -ENOIOCTLCMD) {
> 			v4l2_device_unregister_subdev(sd);
> 			sd = NULL;
>+		} else {
>+			sd->initialized = 1;
> 		}

Wouldn't checkpatch.pl script complain about { } on the else part since
there is only one statement?  
> 	}
>

RE: [RFC/PATCH 2/6] v4l: subdev: Add device node support

[Sylwester Nawrocki]

Isn't it like there need to be {} for both "if" and "else" when
there is more than one line in either block?

Regards,
--
Sylwester Nawrocki

> -----Original Message-----
> From: linux-media-owner@vger.kernel.org [mailto:linux-media-
> owner@vger.kernel.org] On Behalf Of Karicheri, Muralidharan
> Sent: Wednesday, July 07, 2010 4:15 PM
> To: Laurent Pinchart; linux-media@vger.kernel.org
> Cc: sakari.ailus@maxwell.research.nokia.com
> Subject: RE: [RFC/PATCH 2/6] v4l: subdev: Add device node support
> 
> 
> 
> >v4l2_device *v4l2_dev,
> > 		if (err && err != -ENOIOCTLCMD) {
> > 			v4l2_device_unregister_subdev(sd);
> > 			sd = NULL;
> >+		} else {
> >+			sd->initialized = 1;
> > 		}
> 
> Wouldn't checkpatch.pl script complain about { } on the else part since
> there is only one statement?
> > 	}
> >
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media"
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH 2/6] v4l: subdev: Add device node support

[Mauro Carvalho Chehab]

Em 07-07-2010 08:53, Laurent Pinchart escreveu:
> Create a device node named subdevX for every registered subdev.
> As the device node is registered before the subdev core::s_config
> function is called, return -EGAIN on open until initialization
> completes.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Vimarsh Zutshi <vimarsh.zutshi@nokia.com>
> ---
>  drivers/media/video/Makefile      |    2 +-
>  drivers/media/video/v4l2-common.c |    3 ++
>  drivers/media/video/v4l2-dev.c    |    5 +++
>  drivers/media/video/v4l2-device.c |   27 +++++++++++++++-
>  drivers/media/video/v4l2-subdev.c |   65 +++++++++++++++++++++++++++++++++++++
>  include/media/v4l2-dev.h          |    3 +-
>  include/media/v4l2-subdev.h       |   10 ++++++
>  7 files changed, 112 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/media/video/v4l2-subdev.c
> 
> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> index cc93859..c9c07e5 100644
> --- a/drivers/media/video/Makefile
> +++ b/drivers/media/video/Makefile
> @@ -11,7 +11,7 @@ stkwebcam-objs	:=	stk-webcam.o stk-sensor.o
>  omap2cam-objs	:=	omap24xxcam.o omap24xxcam-dma.o
>  
>  videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
> -			v4l2-event.o
> +			v4l2-event.o v4l2-subdev.o
>  
>  # V4L2 core modules
>  
> diff --git a/drivers/media/video/v4l2-common.c b/drivers/media/video/v4l2-common.c
> index 4e53b0b..3032aa3 100644
> --- a/drivers/media/video/v4l2-common.c
> +++ b/drivers/media/video/v4l2-common.c
> @@ -871,6 +871,7 @@ struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device *v4l2_dev,
>  
>  	/* Register with the v4l2_device which increases the module's
>  	   use count as well. */
> +	sd->initialized = 0;
>  	if (v4l2_device_register_subdev(v4l2_dev, sd))
>  		sd = NULL;
>  	/* Decrease the module use count to match the first try_module_get. */
> @@ -885,6 +886,8 @@ struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device *v4l2_dev,
>  		if (err && err != -ENOIOCTLCMD) {
>  			v4l2_device_unregister_subdev(sd);
>  			sd = NULL;
> +		} else {
> +			sd->initialized = 1;
>  		}
>  	}
>  
> diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
> index 0ca7ec9..5a9e9df 100644
> --- a/drivers/media/video/v4l2-dev.c
> +++ b/drivers/media/video/v4l2-dev.c
> @@ -401,6 +401,8 @@ static int get_index(struct video_device *vdev)
>   *	%VFL_TYPE_VBI - Vertical blank data (undecoded)
>   *
>   *	%VFL_TYPE_RADIO - A radio card
> + *
> + *	%VFL_TYPE_SUBDEV - A subdevice
>   */
>  static int __video_register_device(struct video_device *vdev, int type, int nr,
>  		int warn_if_nr_in_use)
> @@ -439,6 +441,9 @@ static int __video_register_device(struct video_device *vdev, int type, int nr,
>  	case VFL_TYPE_RADIO:
>  		name_base = "radio";
>  		break;
> +	case VFL_TYPE_SUBDEV:
> +		name_base = "subdev";
> +		break;
>  	default:
>  		printk(KERN_ERR "%s called with unknown type: %d\n",
>  		       __func__, type);
> diff --git a/drivers/media/video/v4l2-device.c b/drivers/media/video/v4l2-device.c
> index 5a7dc4a..685fa82 100644
> --- a/drivers/media/video/v4l2-device.c
> +++ b/drivers/media/video/v4l2-device.c
> @@ -115,18 +115,40 @@ EXPORT_SYMBOL_GPL(v4l2_device_unregister);
>  int v4l2_device_register_subdev(struct v4l2_device *v4l2_dev,
>  						struct v4l2_subdev *sd)
>  {
> +	struct video_device *vdev;
> +	int ret;
> +
>  	/* Check for valid input */
>  	if (v4l2_dev == NULL || sd == NULL || !sd->name[0])
>  		return -EINVAL;
> +
>  	/* Warn if we apparently re-register a subdev */
>  	WARN_ON(sd->v4l2_dev != NULL);
> +
>  	if (!try_module_get(sd->owner))
>  		return -ENODEV;
> +
>  	sd->v4l2_dev = v4l2_dev;
>  	spin_lock(&v4l2_dev->lock);
>  	list_add_tail(&sd->list, &v4l2_dev->subdevs);
>  	spin_unlock(&v4l2_dev->lock);
> -	return 0;
> +
> +	/* Register the device node. */
> +	vdev = &sd->devnode;
> +	snprintf(vdev->name, sizeof(vdev->name), "subdev");
> +	vdev->parent = v4l2_dev->dev;
> +	vdev->fops = &v4l2_subdev_fops;
> +	vdev->release = video_device_release_empty;
> +	ret = video_register_device(vdev, VFL_TYPE_SUBDEV, -1);
> +	if (ret < 0) {
> +		spin_lock(&v4l2_dev->lock);
> +		list_del(&sd->list);
> +		spin_unlock(&v4l2_dev->lock);
> +		sd->v4l2_dev = NULL;
> +		module_put(sd->owner);
> +	}
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(v4l2_device_register_subdev);
>  
> @@ -135,10 +157,13 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev *sd)
>  	/* return if it isn't registered */
>  	if (sd == NULL || sd->v4l2_dev == NULL)
>  		return;
> +
>  	spin_lock(&sd->v4l2_dev->lock);
>  	list_del(&sd->list);
>  	spin_unlock(&sd->v4l2_dev->lock);
>  	sd->v4l2_dev = NULL;
> +
>  	module_put(sd->owner);
> +	video_unregister_device(&sd->devnode);
>  }
>  EXPORT_SYMBOL_GPL(v4l2_device_unregister_subdev);
> diff --git a/drivers/media/video/v4l2-subdev.c b/drivers/media/video/v4l2-subdev.c
> new file mode 100644
> index 0000000..a048161
> --- /dev/null
> +++ b/drivers/media/video/v4l2-subdev.c
> @@ -0,0 +1,65 @@
> +/*
> + *  V4L2 subdevice support.
> + *
> + *  Copyright (C) 2009  Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; either version 2 of the License, or
> + *  (at your option) any later version.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; if not, write to the Free Software
> + *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + */
> +
> +#include <linux/types.h>
> +#include <linux/ioctl.h>
> +#include <linux/videodev2.h>
> +
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-ioctl.h>
> +
> +static int subdev_open(struct file *file)
> +{
> +	struct video_device *vdev = video_devdata(file);
> +	struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
> +
> +	if (!sd->initialized)
> +		return -EAGAIN;

Those internal interfaces should not be used on normal devices/applications, as none of
the existing drivers are tested or supposed to properly work if an external program is
touching on its internal interfaces. So, please add:

	if (!capable(CAP_SYS_ADMIN))
		return -EPERM;

> +
> +	return 0;
> +}
> +
> +static int subdev_close(struct file *file)
> +{
> +	return 0;
> +}
> +
> +static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> +{
> +	switch (cmd) {
> +	default:
> +		return -ENOIOCTLCMD;
> +	}
> +
> +	return 0;
> +}
> +
> +static long subdev_ioctl(struct file *file, unsigned int cmd,
> +	unsigned long arg)
> +{
> +	return video_usercopy(file, cmd, arg, subdev_do_ioctl);

This is a legacy call. Please, don't use it. 

Also, while the API doc says that only certain ioctls are supported on subdev, there's
no code here preventing the usage of invalid ioctls. So, it is possible to do bad things,
like changing the video standard format individually on each subdev, creating all sorts of
problems.

> +}
> +
> +const struct v4l2_file_operations v4l2_subdev_fops = {
> +	.owner = THIS_MODULE,
> +	.open = subdev_open,
> +	.unlocked_ioctl = subdev_ioctl,
> +	.release = subdev_close,
> +};
> diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
> index bebe44b..eee1e2b 100644
> --- a/include/media/v4l2-dev.h
> +++ b/include/media/v4l2-dev.h
> @@ -22,7 +22,8 @@
>  #define VFL_TYPE_VBI		1
>  #define VFL_TYPE_RADIO		2
>  #define VFL_TYPE_VTX		3
> -#define VFL_TYPE_MAX		4
> +#define VFL_TYPE_SUBDEV		4
> +#define VFL_TYPE_MAX		5
>  
>  struct v4l2_ioctl_callbacks;
>  struct video_device;
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 6088316..00010bd 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -22,6 +22,7 @@
>  #define _V4L2_SUBDEV_H
>  
>  #include <media/v4l2-common.h>
> +#include <media/v4l2-dev.h>
>  #include <media/v4l2-mediabus.h>
>  
>  /* generic v4l2_device notify callback notification values */
> @@ -421,8 +422,16 @@ struct v4l2_subdev {
>  	u32 grp_id;
>  	/* pointer to private data */
>  	void *priv;
> +	/* subdev device node */
> +	struct video_device devnode;
> +	unsigned int initialized;
>  };
>  
> +#define vdev_to_v4l2_subdev(vdev) \
> +	container_of(vdev, struct v4l2_subdev, devnode)
> +
> +extern const struct v4l2_file_operations v4l2_subdev_fops;
> +
>  static inline void v4l2_set_subdevdata(struct v4l2_subdev *sd, void *p)
>  {
>  	sd->priv = p;
> @@ -444,6 +453,7 @@ static inline void v4l2_subdev_init(struct v4l2_subdev *sd,
>  	sd->name[0] = '\0';
>  	sd->grp_id = 0;
>  	sd->priv = NULL;
> +	sd->initialized = 0;
>  }
>  
>  /* Call an ops of a v4l2_subdev, doing the right checks against

Re: [RFC/PATCH 2/6] v4l: subdev: Add device node support

[Mauro Carvalho Chehab]

Em 07-07-2010 11:14, Karicheri, Muralidharan escreveu:
> 
> 
>> v4l2_device *v4l2_dev,
>> 		if (err && err != -ENOIOCTLCMD) {
>> 			v4l2_device_unregister_subdev(sd);
>> 			sd = NULL;
>> +		} else {
>> +			sd->initialized = 1;
>> 		}
> 
> Wouldn't checkpatch.pl script complain about { } on the else part since
> there is only one statement?  

IMO, it is because it analyzes the entire if clause. As the first part of the if has two
statements, CodingStyle accepts the usage of braces at the second part 
(although this is not a common practice).

>> 	}
>>
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH 2/6] v4l: subdev: Add device node support

[Laurent Pinchart]

Hi Mauro,

Thanks for the review.

On Wednesday 07 July 2010 16:58:01 Mauro Carvalho Chehab wrote:
> Em 07-07-2010 08:53, Laurent Pinchart escreveu:
> > Create a device node named subdevX for every registered subdev.
> > As the device node is registered before the subdev core::s_config
> > function is called, return -EGAIN on open until initialization
> > completes.

[snip]

> > diff --git a/drivers/media/video/v4l2-subdev.c
> > b/drivers/media/video/v4l2-subdev.c new file mode 100644
> > index 0000000..a048161
> > --- /dev/null
> > +++ b/drivers/media/video/v4l2-subdev.c
> > @@ -0,0 +1,65 @@

[snip]

> > +static int subdev_open(struct file *file)
> > +{
> > +	struct video_device *vdev = video_devdata(file);
> > +	struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
> > +
> > +	if (!sd->initialized)
> > +		return -EAGAIN;
> 
> Those internal interfaces should not be used on normal
> devices/applications, as none of the existing drivers are tested or
> supposed to properly work if an external program is touching on its
> internal interfaces. So, please add:
> 
> 	if (!capable(CAP_SYS_ADMIN))
> 		return -EPERM;

As Hans pointed out, subdev device nodes should only be created if the subdev 
request it explicitly. I'll fix the patch accordingly. Existing subdevs will 
not have a device node by default anymore, so the CAP_SYS_ADMIN capability 
won't be required (new subdevs that explicitly ask for a device node are 
supposed to handle the calls properly, otherwise it's a bit pointless :-)).

> > +
> > +	return 0;
> > +}

[snip]

> > +static long subdev_ioctl(struct file *file, unsigned int cmd,
> > +	unsigned long arg)
> > +{
> > +	return video_usercopy(file, cmd, arg, subdev_do_ioctl);
> 
> This is a legacy call. Please, don't use it.

What should I use instead then ? I need the functionality of video_usercopy. I 
could copy it to v4l2-subdev.c verbatim. As video_ioctl2 shares lots of code 
with video_usercopy I think video_usercopy should stay, and video_ioctl2 
should use it.

> Also, while the API doc says that only certain ioctls are supported on
> subdev, there's no code here preventing the usage of invalid ioctls. So,
> it is possible to do bad things, like changing the video standard format
> individually on each subdev, creating all sorts of problems.

Invalid (or rather unsupported) ioctls will be routed to the subdev 
core::ioctl operation. Formats will not be changed automagically just because 
a userspace application issues a VIDIOC_S_FMT ioctl.

As the device node creation will need to be requested explicitly this won't be 
an issue anyway.

-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH 2/6] v4l: subdev: Add device node support

[Mauro Carvalho Chehab]

Em 07-07-2010 16:44, Laurent Pinchart escreveu:
> Hi Mauro,
> 
> Thanks for the review.
>
> On Wednesday 07 July 2010 16:58:01 Mauro Carvalho Chehab wrote:
>> Em 07-07-2010 08:53, Laurent Pinchart escreveu:
>>> Create a device node named subdevX for every registered subdev.
>>> As the device node is registered before the subdev core::s_config
>>> function is called, return -EGAIN on open until initialization
>>> completes.
> 
> [snip]
> 
>>> diff --git a/drivers/media/video/v4l2-subdev.c
>>> b/drivers/media/video/v4l2-subdev.c new file mode 100644
>>> index 0000000..a048161
>>> --- /dev/null
>>> +++ b/drivers/media/video/v4l2-subdev.c
>>> @@ -0,0 +1,65 @@
> 
> [snip]
> 
>>> +static int subdev_open(struct file *file)
>>> +{
>>> +	struct video_device *vdev = video_devdata(file);
>>> +	struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
>>> +
>>> +	if (!sd->initialized)
>>> +		return -EAGAIN;
>>
>> Those internal interfaces should not be used on normal
>> devices/applications, as none of the existing drivers are tested or
>> supposed to properly work if an external program is touching on its
>> internal interfaces. So, please add:
>>
>> 	if (!capable(CAP_SYS_ADMIN))
>> 		return -EPERM;
> 
> As Hans pointed out, subdev device nodes should only be created if the subdev 
> request it explicitly. I'll fix the patch accordingly. Existing subdevs will 
> not have a device node by default anymore, so the CAP_SYS_ADMIN capability 
> won't be required (new subdevs that explicitly ask for a device node are 
> supposed to handle the calls properly, otherwise it's a bit pointless :-)).

It should be not requested by the subdev, but by the bridge driver (or maybe
by both).

On several drivers, the bridge is connected to more than one subdev, and some
changes need to go to both subdevs, in order to work. As the glue logic is at
the bridge driver, creating subdev interfaces will not make sense on those devices.

>>> +
>>> +	return 0;
>>> +}
> 
> [snip]
> 
>>> +static long subdev_ioctl(struct file *file, unsigned int cmd,
>>> +	unsigned long arg)
>>> +{
>>> +	return video_usercopy(file, cmd, arg, subdev_do_ioctl);
>>
>> This is a legacy call. Please, don't use it.
> 
> What should I use instead then ? I need the functionality of video_usercopy. I 
> could copy it to v4l2-subdev.c verbatim. As video_ioctl2 shares lots of code 
> with video_usercopy I think video_usercopy should stay, and video_ioctl2 
> should use it.

The bad thing of video_usercopy() is that it has a "compat" code, to fix broken
definitions of some iotls (see video_fix_command()), and that a few drivers still
use it, instead of video_ioctl2. For sure, we don't need the "compat" code for
subdev interface. Also, as time goes by, we'll eventually have different needs at
the subdev interface, as some types of ioctl's may be specific to subdevs and may
require different copy logic.

IMO, the better is to use the same logic inside the subdev stuff, of course
removing that "old ioctl" fix logic:

#ifdef __OLD_VIDIOC_
	cmd = video_fix_command(cmd);
#endif

And replacing the call to:
	err = func(file, cmd, parg);

By the proper subdev handling.

>> Also, while the API doc says that only certain ioctls are supported on
>> subdev, there's no code here preventing the usage of invalid ioctls. So,
>> it is possible to do bad things, like changing the video standard format
>> individually on each subdev, creating all sorts of problems.
> 
> Invalid (or rather unsupported) ioctls will be routed to the subdev 
> core::ioctl operation. Formats will not be changed automagically just because 
> a userspace application issues a VIDIOC_S_FMT ioctl.
> 
> As the device node creation will need to be requested explicitly this won't be 
> an issue anyway.
> 

Ok. It helps if you add the proper ioctl logic, instead of a stub.

Cheers,
Mauro

RE: [RFC/PATCH 2/6] v4l: subdev: Add device node support

[Pawel Osciak]

Hi,

>>v4l2_device *v4l2_dev,
>> 		if (err && err != -ENOIOCTLCMD) {
>> 			v4l2_device_unregister_subdev(sd);
>> 			sd = NULL;
>>+		} else {
>>+			sd->initialized = 1;
>> 		}
>
>Wouldn't checkpatch.pl script complain about { } on the else part since
>there is only one statement?
>> 	}
>>


CodingStyle is 100% clear on this:


Do not unnecessarily use braces where a single statement will do.

if (condition)
        action();

This does not apply if one branch of a conditional statement is a single
statement. Use braces in both branches.

if (condition) {
        do_this();
        do_that();
} else {
        otherwise();
}

Best regards
--
Pawel Osciak
Linux Platform Group
Samsung Poland R&D Center

Re: [RFC/PATCH 2/6] v4l: subdev: Add device node support

[Laurent Pinchart]

Hi Hans,

On Wednesday 07 July 2010 15:04:17 Hans Verkuil wrote:
> > On Wednesday 07 July 2010 14:30:45 Hans Verkuil wrote:

[snip]

> > Some (most ?) I2C sensors need to be accessed during probe. This involves
> > powering the sensor up, which is handled by a board code function called
> > through a platform data function pointer.
> > 
> > On the OMAP3 ISP the sensor clock can be generated by the ISP. This means
> > that board code needs to call an ISP (exported) function to turn the clock
> > on. To do so we currently retrieve a pointer to the ISP device through the
> > subdev's parent v4l2_device, embedded in the ISP device structure. This
> > requires the subdev to be registered, otherwise its parent will be NULL.
> > For that reason we're still using the core::s_config operation.
> > 
> > This is obviously not an ideal situation, and I'm open to suggestions on
> > how to solve it without core::s_config. One possibility would be to use a
> > global ISP device pointer in the board code, as there's only one ISP
> > device in the system. It feels a bit hackish though.
> 
> Or make the v4l2_device pointer part of the platform data? That way the
> subdev driver has access to the v4l2_device pointer in a fairly clean
> manner.

As we've discussed on IRC, the platform data should really store hardware 
properties, not software/driver information. Beside, storing the v4l2_device 
pointer in the platform data would be quite difficult, as 
v4l2_device_register_subdev only sees a void * pointer for platform_data. It 
has no knowledge of the device-specific structure.

-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH 2/6] v4l: subdev: Add device node support

[Laurent Pinchart]

Hi Mauro,

On Wednesday 07 July 2010 22:53:40 Mauro Carvalho Chehab wrote:
> Em 07-07-2010 16:44, Laurent Pinchart escreveu:
> > On Wednesday 07 July 2010 16:58:01 Mauro Carvalho Chehab wrote:
> >> Em 07-07-2010 08:53, Laurent Pinchart escreveu:
> >>> Create a device node named subdevX for every registered subdev.
> >>> As the device node is registered before the subdev core::s_config
> >>> function is called, return -EGAIN on open until initialization
> >>> completes.
> > 
> > [snip]
> > 
> >>> +static int subdev_open(struct file *file)
> >>> +{
> >>> +	struct video_device *vdev = video_devdata(file);
> >>> +	struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
> >>> +
> >>> +	if (!sd->initialized)
> >>> +		return -EAGAIN;
> >> 
> >> Those internal interfaces should not be used on normal
> >> devices/applications, as none of the existing drivers are tested or
> >> supposed to properly work if an external program is touching on its
> >> 
> >> internal interfaces. So, please add:
> >> 	if (!capable(CAP_SYS_ADMIN))
> >> 	
> >> 		return -EPERM;
> > 
> > As Hans pointed out, subdev device nodes should only be created if the
> > subdev request it explicitly. I'll fix the patch accordingly. Existing
> > subdevs will not have a device node by default anymore, so the
> > CAP_SYS_ADMIN capability won't be required (new subdevs that explicitly
> > ask for a device node are supposed to handle the calls properly,
> > otherwise it's a bit pointless :-)).
> 
> It should be not requested by the subdev, but by the bridge driver (or
> maybe by both).
> 
> On several drivers, the bridge is connected to more than one subdev, and
> some changes need to go to both subdevs, in order to work. As the glue
> logic is at the bridge driver, creating subdev interfaces will not make
> sense on those devices.

Agreed. I've added a flag that subdev drivers can set to request creation of a 
device node explicitly, and an argument to to v4l2_i2c_new_subdev_board and 
v4l2_spi_new_subdev to overwrite the flag. A device node will only be created 
if the flag is set by the subdev (meaning "I can support device nodes") and 
the flag is not forced to 0 by the bridge driver (meaning "I allow userspace 
to access the subdev directly).

[snip]

> >>> +static long subdev_ioctl(struct file *file, unsigned int cmd,
> >>> +	unsigned long arg)
> >>> +{
> >>> +	return video_usercopy(file, cmd, arg, subdev_do_ioctl);
> >> 
> >> This is a legacy call. Please, don't use it.
> > 
> > What should I use instead then ? I need the functionality of
> > video_usercopy. I could copy it to v4l2-subdev.c verbatim. As
> > video_ioctl2 shares lots of code with video_usercopy I think
> > video_usercopy should stay, and video_ioctl2 should use it.
> 
> The bad thing of video_usercopy() is that it has a "compat" code, to fix
> broken definitions of some iotls (see video_fix_command()), and that a few
> drivers still use it, instead of video_ioctl2.

video_ioctl2 has the same compat code.

> For sure, we don't need the "compat" code for subdev interface. Also, as
> time goes by, we'll eventually have different needs at the subdev interface,
> as some types of ioctl's may be specific to subdevs and may require
> different copy logic.

We can change the logic then :-)

> IMO, the better is to use the same logic inside the subdev stuff, of course
> removing that "old ioctl" fix logic:
> 
> #ifdef __OLD_VIDIOC_
> 	cmd = video_fix_command(cmd);
> #endif
> 
> And replacing the call to:
> 	err = func(file, cmd, parg);
> 
> By the proper subdev handling.

What about renaming video_usercopy to __video_usercopy, adding an argument to 
enable/disable the compat code, create a new video_usercopy that calls 
__video_usercopy with compat code enabled, have video_ioctl2 call 
__video_usercopy with compat code enabled, and having subdev_ioctl call 
__video_usercopy with compat code disabled ?

-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH 2/6] v4l: subdev: Add device node support

[Hans Verkuil]

> Hi Hans,
>
> On Wednesday 07 July 2010 15:04:17 Hans Verkuil wrote:
>> > On Wednesday 07 July 2010 14:30:45 Hans Verkuil wrote:
>
> [snip]
>
>> > Some (most ?) I2C sensors need to be accessed during probe. This
>> involves
>> > powering the sensor up, which is handled by a board code function
>> called
>> > through a platform data function pointer.
>> >
>> > On the OMAP3 ISP the sensor clock can be generated by the ISP. This
>> means
>> > that board code needs to call an ISP (exported) function to turn the
>> clock
>> > on. To do so we currently retrieve a pointer to the ISP device through
>> the
>> > subdev's parent v4l2_device, embedded in the ISP device structure.
>> This
>> > requires the subdev to be registered, otherwise its parent will be
>> NULL.
>> > For that reason we're still using the core::s_config operation.
>> >
>> > This is obviously not an ideal situation, and I'm open to suggestions
>> on
>> > how to solve it without core::s_config. One possibility would be to
>> use a
>> > global ISP device pointer in the board code, as there's only one ISP
>> > device in the system. It feels a bit hackish though.
>>
>> Or make the v4l2_device pointer part of the platform data? That way the
>> subdev driver has access to the v4l2_device pointer in a fairly clean
>> manner.
>
> As we've discussed on IRC, the platform data should really store hardware
> properties, not software/driver information. Beside, storing the
> v4l2_device
> pointer in the platform data would be quite difficult, as
> v4l2_device_register_subdev only sees a void * pointer for platform_data.
> It
> has no knowledge of the device-specific structure.

I think my main problem is the use of s_config. This op was designed to
pass irq and platform data to i2c drivers in pre-2.6.26 kernels. Looking
at the current tree I see that the only driver using it is mt9v011.c. I
think s_config should be removed and mt9v011.c/em28xx-cards.c be changed
to use the platform_data directly. Currently s_config is used there to set
the sensor xtal, which seems very much hardware specific to me.

Instead of s_config I would probably add a 'registered' op that is called
automatically by v4l2_device_register_subdev once the subdev has been
registered successfully. Subdev drivers can then execute code that can
only be run when registered. Later we might add an 'unregistered' op as
well should we need it.

This seems much more reasonable to me.

Comments?

         Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: [RFC/PATCH 2/6] v4l: subdev: Add device node support

[Mauro Carvalho Chehab]

Em 08-07-2010 09:08, Laurent Pinchart escreveu:
> Hi Mauro,
> 
> On Wednesday 07 July 2010 22:53:40 Mauro Carvalho Chehab wrote:
>> Em 07-07-2010 16:44, Laurent Pinchart escreveu:
>>> On Wednesday 07 July 2010 16:58:01 Mauro Carvalho Chehab wrote:
>>>> Em 07-07-2010 08:53, Laurent Pinchart escreveu:
>>>>> Create a device node named subdevX for every registered subdev.
>>>>> As the device node is registered before the subdev core::s_config
>>>>> function is called, return -EGAIN on open until initialization
>>>>> completes.
>>>
>>> [snip]
>>>
>>>>> +static int subdev_open(struct file *file)
>>>>> +{
>>>>> +	struct video_device *vdev = video_devdata(file);
>>>>> +	struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
>>>>> +
>>>>> +	if (!sd->initialized)
>>>>> +		return -EAGAIN;
>>>>
>>>> Those internal interfaces should not be used on normal
>>>> devices/applications, as none of the existing drivers are tested or
>>>> supposed to properly work if an external program is touching on its
>>>>
>>>> internal interfaces. So, please add:
>>>> 	if (!capable(CAP_SYS_ADMIN))
>>>> 	
>>>> 		return -EPERM;
>>>
>>> As Hans pointed out, subdev device nodes should only be created if the
>>> subdev request it explicitly. I'll fix the patch accordingly. Existing
>>> subdevs will not have a device node by default anymore, so the
>>> CAP_SYS_ADMIN capability won't be required (new subdevs that explicitly
>>> ask for a device node are supposed to handle the calls properly,
>>> otherwise it's a bit pointless :-)).
>>
>> It should be not requested by the subdev, but by the bridge driver (or
>> maybe by both).
>>
>> On several drivers, the bridge is connected to more than one subdev, and
>> some changes need to go to both subdevs, in order to work. As the glue
>> logic is at the bridge driver, creating subdev interfaces will not make
>> sense on those devices.
> 
> Agreed. I've added a flag that subdev drivers can set to request creation of a 
> device node explicitly, and an argument to to v4l2_i2c_new_subdev_board and 
> v4l2_spi_new_subdev to overwrite the flag. A device node will only be created 
> if the flag is set by the subdev (meaning "I can support device nodes") and 
> the flag is not forced to 0 by the bridge driver (meaning "I allow userspace 
> to access the subdev directly).

OK.

> [snip]
> 
>>>>> +static long subdev_ioctl(struct file *file, unsigned int cmd,
>>>>> +	unsigned long arg)
>>>>> +{
>>>>> +	return video_usercopy(file, cmd, arg, subdev_do_ioctl);
>>>>
>>>> This is a legacy call. Please, don't use it.
>>>
>>> What should I use instead then ? I need the functionality of
>>> video_usercopy. I could copy it to v4l2-subdev.c verbatim. As
>>> video_ioctl2 shares lots of code with video_usercopy I think
>>> video_usercopy should stay, and video_ioctl2 should use it.
>>
>> The bad thing of video_usercopy() is that it has a "compat" code, to fix
>> broken definitions of some iotls (see video_fix_command()), and that a few
>> drivers still use it, instead of video_ioctl2.
> 
> video_ioctl2 has the same compat code.

Yes, in order to avoid breaking binary compatibility with files compiled against
the old videodev2.h header that used to declare some ioctls as:

#define VIDIOC_OVERLAY     	_IOWR('V', 14, int)
#define VIDIOC_S_PARM      	 _IOW('V', 22, struct v4l2_streamparm)
#define VIDIOC_S_CTRL      	 _IOW('V', 28, struct v4l2_control)
#define VIDIOC_G_AUDIO     	_IOWR('V', 33, struct v4l2_audio)
#define VIDIOC_G_AUDOUT    	_IOWR('V', 49, struct v4l2_audioout)
#define VIDIOC_CROPCAP     	 _IOR('V', 58, struct v4l2_cropcap)

instead of:

#define VIDIOC_OVERLAY		 _IOW('V', 14, int)
#define VIDIOC_S_PARM		_IOWR('V', 22, struct v4l2_streamparm)
#define VIDIOC_S_CTRL		_IOWR('V', 28, struct v4l2_control)
#define VIDIOC_G_AUDIO		 _IOR('V', 33, struct v4l2_audio)
#define VIDIOC_G_AUDOUT		 _IOR('V', 49, struct v4l2_audioout)
#define VIDIOC_CROPCAP		_IOWR('V', 58, struct v4l2_cropcap)

(basically, the old ioctl's were using the wrong _IO macros, preventing a generic
copy_from_user/copy_to_user code to work)

This doesn't make sense for subdev, as old binary-only applications will never
use the legacy ioctl definitions.

Probably, we can mark this legacy support for removal at 
Documentation/feature-removal-schedule.txt, and remove
it on a latter version of the kernel. It seems that the old ioctl definitions are
before 2005 (before 2.6.12):

^1da177e (Linus Torvalds        2005-04-16 15:20:36 -0700 1461) #define VIDIOC_OVERLAY_OLD      _IOWR ('V', 14, int)
^1da177e (Linus Torvalds        2005-04-16 15:20:36 -0700 1462) #define VIDIOC_S_PARM_OLD       _IOW  ('V', 22, struct v4l2_streamparm)
^1da177e (Linus Torvalds        2005-04-16 15:20:36 -0700 1463) #define VIDIOC_S_CTRL_OLD       _IOW  ('V', 28, struct v4l2_control)
^1da177e (Linus Torvalds        2005-04-16 15:20:36 -0700 1464) #define VIDIOC_G_AUDIO_OLD      _IOWR ('V', 33, struct v4l2_audio)
^1da177e (Linus Torvalds        2005-04-16 15:20:36 -0700 1465) #define VIDIOC_G_AUDOUT_OLD     _IOWR ('V', 49, struct v4l2_audioout)
^1da177e (Linus Torvalds        2005-04-16 15:20:36 -0700 1466) #define VIDIOC_CROPCAP_OLD      _IOR  ('V', 58, struct v4l2_cropcap)

>> For sure, we don't need the "compat" code for subdev interface. Also, as
>> time goes by, we'll eventually have different needs at the subdev interface,
>> as some types of ioctl's may be specific to subdevs and may require
>> different copy logic.
> 
> We can change the logic then :-)
> 
>> IMO, the better is to use the same logic inside the subdev stuff, of course
>> removing that "old ioctl" fix logic:
>>
>> #ifdef __OLD_VIDIOC_
>> 	cmd = video_fix_command(cmd);
>> #endif
>>
>> And replacing the call to:
>> 	err = func(file, cmd, parg);
>>
>> By the proper subdev handling.
> 
> What about renaming video_usercopy to __video_usercopy, adding an argument to 
> enable/disable the compat code, create a new video_usercopy that calls 
> __video_usercopy with compat code enabled, have video_ioctl2 call 
> __video_usercopy with compat code enabled, and having subdev_ioctl call 
> __video_usercopy with compat code disabled ?

Seems good, but maybe the better is to put the call to video_fix_command() outside
the common function.

We may add also a printk for the video_usercopy wrapper printing that the
driver is using a legacy API call, and that this will be removed on a next kernel version.
Maybe this way, people will finally submit patches porting the last remaining drivers to
video_ioctl2.

I suspect, however, that we'll end by needing a subdev-specific version of __video_usercopy,
as we add new ioctls to subdev.

Cheers,
Mauro.

Re: [RFC/PATCH 2/6] v4l: subdev: Add device node support

[Laurent Pinchart]

Hi Mauro,

On Thursday 08 July 2010 15:51:33 Mauro Carvalho Chehab wrote:
> Em 08-07-2010 09:08, Laurent Pinchart escreveu:
> > On Wednesday 07 July 2010 22:53:40 Mauro Carvalho Chehab wrote:
> >> Em 07-07-2010 16:44, Laurent Pinchart escreveu:
> >>> On Wednesday 07 July 2010 16:58:01 Mauro Carvalho Chehab wrote:
> >>>> Em 07-07-2010 08:53, Laurent Pinchart escreveu:

[snip]

> >>>>> +static long subdev_ioctl(struct file *file, unsigned int cmd,
> >>>>> +	unsigned long arg)
> >>>>> +{
> >>>>> +	return video_usercopy(file, cmd, arg, subdev_do_ioctl);
> >>>> 
> >>>> This is a legacy call. Please, don't use it.
> >>> 
> >>> What should I use instead then ? I need the functionality of
> >>> video_usercopy. I could copy it to v4l2-subdev.c verbatim. As
> >>> video_ioctl2 shares lots of code with video_usercopy I think
> >>> video_usercopy should stay, and video_ioctl2 should use it.
> >> 
> >> The bad thing of video_usercopy() is that it has a "compat" code, to fix
> >> broken definitions of some iotls (see video_fix_command()), and that a
> >> few drivers still use it, instead of video_ioctl2.
> > 
> > video_ioctl2 has the same compat code.
> 
> Yes, in order to avoid breaking binary compatibility with files compiled
> against the old videodev2.h header that used to declare some ioctls as:

[snip]

> This doesn't make sense for subdev, as old binary-only applications will
> never use the legacy ioctl definitions.
> 
> Probably, we can mark this legacy support for removal at
> Documentation/feature-removal-schedule.txt, and remove
> it on a latter version of the kernel. It seems that the old ioctl
> definitions are before 2005 (before 2.6.12):

Good idea.

[snip]

> > What about renaming video_usercopy to __video_usercopy, adding an
> > argument to enable/disable the compat code, create a new video_usercopy
> > that calls __video_usercopy with compat code enabled, have video_ioctl2
> > call __video_usercopy with compat code enabled, and having subdev_ioctl
> > call __video_usercopy with compat code disabled ?
> 
> Seems good, but maybe the better is to put the call to video_fix_command()
> outside the common function.

Agreed. It belongs to video_usercopy and video_ioctl2.

> We may add also a printk for the video_usercopy wrapper printing that the
> driver is using a legacy API call, and that this will be removed on a next
> kernel version. Maybe this way, people will finally submit patches porting
> the last remaining drivers to video_ioctl2.
> 
> I suspect, however, that we'll end by needing a subdev-specific version of
> __video_usercopy, as we add new ioctls to subdev.

If we need one then I'll create one in v4l2-subdev.c, but as long as we don't 
there's little point in duplicating the code.

I've had a look at video_usercopy and video_ioctl2, and the functions are 
mostly identical. The differences are

- video_ioctl2 passes the original arg argument to __video_do_ioctl for _IO 
ioctls. video_usercopy passes NULL. This seems to be used by the ivtv driver 
to handle DVB _IO ioctls (VIDEO_SELECT_SOURCE, AUDIO_SET_MUTE, 
AUDIO_CHANNEL_SELECT and AUDIO_BILINGUAL_CHANNEL_SELECT) that pass an integer 
through the ioctl argument. Any objection against passing the original 
argument to the ioctl handler in video_usercopy as well ? I've quickly checked 
the video_usercopy users and none of them seem to check that arg is NULL for 
_IO ioctls. This should thus be harmless.

- For argument of known ioctls that have a few input fields at the beginning 
of the structure followed by output fields, video_ioctl2 only copies the input 
fields from userspace and zeroes out the rest. video_usercopy doesn't. Do we 
have video_usercopy drivers that abuse the output fields to pass information 
to the driver, or could the video_ioctl2 behaviour be generalized to 
video_usercopy ?

If the answer to the two questions is yes, video_usercopy and video_ioctl2 
could use the same code. Otherwise they can't, and in that case I should 
probably use video_usercopy in v4l2-subdev.c, replacing it with a private copy 
when it will disappear.

-- 
Regards,

Laurent Pinchart