[RFC/PATCH v2 0/7] V4L2 subdev userspace API

[RFC/PATCH v2 0/7] V4L2 subdev userspace API

[Laurent Pinchart]

Hi everybody,

Here's the second version of the V4L2 subdev userspace API patches. Comments
received on the first version have been incorporated, with the exception of the
video_usercopy usage as this is still under discussion.

A problem with module reference counting has also been solved by setting the
owner of the subdev device node to the subdev driver, not videodev.ko. This
required a change to the v4l2_i2c_new_subdev_board function prototype, and thus
a modification to the radio-si4713 driver.

Due to the fast review (thanks everybody) I haven't had time to finish the media
controller core patches, but they will arrive soon.

Laurent Pinchart (6):
  v4l: subdev: Don't require core operations
  v4l: Merge v4l2_i2c_new_subdev_cfg and v4l2_i2c_new_subdev
  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 |   57 +++++++++
 drivers/media/radio/radio-si4713.c           |    2 +-
 drivers/media/video/Makefile                 |    2 +-
 drivers/media/video/soc_camera.c             |    2 +-
 drivers/media/video/v4l2-common.c            |   22 ++--
 drivers/media/video/v4l2-dev.c               |   27 ++---
 drivers/media/video/v4l2-device.c            |   25 ++++-
 drivers/media/video/v4l2-subdev.c            |  172 ++++++++++++++++++++++++++
 include/media/v4l2-common.h                  |   21 +---
 include/media/v4l2-dev.h                     |   18 +++-
 include/media/v4l2-subdev.h                  |   40 ++++---
 11 files changed, 326 insertions(+), 62 deletions(-)
 create mode 100644 drivers/media/video/v4l2-subdev.c

[RFC/PATCH v2 1/7] 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 v2 2/7] v4l: Merge v4l2_i2c_new_subdev_cfg and v4l2_i2c_new_subdev

[Laurent Pinchart]

v4l2_i2c_new_subdev_cfg is called by v4l2_i2c_new_subdev only. Merge the
two functions into v4l2_i2c_new_subdev.

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

diff --git a/drivers/media/video/v4l2-common.c b/drivers/media/video/v4l2-common.c
index 4e53b0b..bbda421 100644
--- a/drivers/media/video/v4l2-common.c
+++ b/drivers/media/video/v4l2-common.c
@@ -897,10 +897,9 @@ error:
 }
 EXPORT_SYMBOL_GPL(v4l2_i2c_new_subdev_board);
 
-struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct v4l2_device *v4l2_dev,
+struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device *v4l2_dev,
 		struct i2c_adapter *adapter,
 		const char *module_name, const char *client_type,
-		int irq, void *platform_data,
 		u8 addr, const unsigned short *probe_addrs)
 {
 	struct i2c_board_info info;
@@ -910,13 +909,11 @@ struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct v4l2_device *v4l2_dev,
 	memset(&info, 0, sizeof(info));
 	strlcpy(info.type, client_type, sizeof(info.type));
 	info.addr = addr;
-	info.irq = irq;
-	info.platform_data = platform_data;
 
 	return v4l2_i2c_new_subdev_board(v4l2_dev, adapter, module_name,
 			&info, probe_addrs);
 }
-EXPORT_SYMBOL_GPL(v4l2_i2c_new_subdev_cfg);
+EXPORT_SYMBOL_GPL(v4l2_i2c_new_subdev);
 
 /* Return i2c client address of v4l2_subdev. */
 unsigned short v4l2_i2c_subdev_addr(struct v4l2_subdev *sd)
diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
index 98b3264..6fc3d7a 100644
--- a/include/media/v4l2-common.h
+++ b/include/media/v4l2-common.h
@@ -139,24 +139,11 @@ struct v4l2_subdev_ops;
 /* Load an i2c module and return an initialized v4l2_subdev struct.
    Only call request_module if module_name != NULL.
    The client_type argument is the name of the chip that's on the adapter. */
-struct v4l2_subdev *v4l2_i2c_new_subdev_cfg(struct v4l2_device *v4l2_dev,
+struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device *v4l2_dev,
 		struct i2c_adapter *adapter,
 		const char *module_name, const char *client_type,
-		int irq, void *platform_data,
 		u8 addr, const unsigned short *probe_addrs);
 
-/* Load an i2c module and return an initialized v4l2_subdev struct.
-   Only call request_module if module_name != NULL.
-   The client_type argument is the name of the chip that's on the adapter. */
-static inline struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device *v4l2_dev,
-		struct i2c_adapter *adapter,
-		const char *module_name, const char *client_type,
-		u8 addr, const unsigned short *probe_addrs)
-{
-	return v4l2_i2c_new_subdev_cfg(v4l2_dev, adapter, module_name,
-				client_type, 0, NULL, addr, probe_addrs);
-}
-
 struct i2c_board_info;
 
 struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device *v4l2_dev,
-- 
1.7.1

[RFC/PATCH v2 3/7] 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>
---
 Documentation/video4linux/v4l2-framework.txt |   18 +++++++
 drivers/media/radio/radio-si4713.c           |    2 +-
 drivers/media/video/Makefile                 |    2 +-
 drivers/media/video/soc_camera.c             |    2 +-
 drivers/media/video/v4l2-common.c            |   15 +++++-
 drivers/media/video/v4l2-dev.c               |   27 ++++------
 drivers/media/video/v4l2-device.c            |   25 +++++++++-
 drivers/media/video/v4l2-subdev.c            |   65 ++++++++++++++++++++++++++
 include/media/v4l2-common.h                  |    6 ++-
 include/media/v4l2-dev.h                     |   18 ++++++-
 include/media/v4l2-subdev.h                  |   16 ++++++-
 11 files changed, 166 insertions(+), 30 deletions(-)
 create mode 100644 drivers/media/video/v4l2-subdev.c

diff --git a/Documentation/video4linux/v4l2-framework.txt b/Documentation/video4linux/v4l2-framework.txt
index e831aac..164bb0f 100644
--- a/Documentation/video4linux/v4l2-framework.txt
+++ b/Documentation/video4linux/v4l2-framework.txt
@@ -314,6 +314,24 @@ 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 v4l-subdevX can be created
+in /dev. If the sub-device supports direct userspace configuration it must set
+the V4L2_SUBDEV_FL_HAS_DEVNODE flag before being registered.
+
+For I2C and SPI sub-devices, the v4l2_device driver can disable registration of
+the device node if it wants to control the sub-device on its own. In that case
+it must set the v4l2_i2c_new_subdev_board or v4l2_spi_new_subdev enable_devnode
+argument to 0. Setting the argument to 1 will only enable device node
+registration if the sub-device driver has set the V4L2_SUBDEV_FL_HAS_DEVNODE
+flag.
+
+
 I2C sub-device drivers
 ----------------------
 
diff --git a/drivers/media/radio/radio-si4713.c b/drivers/media/radio/radio-si4713.c
index 13554ab..58dd199 100644
--- a/drivers/media/radio/radio-si4713.c
+++ b/drivers/media/radio/radio-si4713.c
@@ -292,7 +292,7 @@ static int radio_si4713_pdriver_probe(struct platform_device *pdev)
 	}
 
 	sd = v4l2_i2c_new_subdev_board(&rsdev->v4l2_dev, adapter, "si4713_i2c",
-					pdata->subdev_board_info, NULL);
+					pdata->subdev_board_info, NULL, 0);
 	if (!sd) {
 		dev_err(&pdev->dev, "Cannot get v4l2 subdevice\n");
 		rval = -ENODEV;
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/soc_camera.c b/drivers/media/video/soc_camera.c
index 475757b..10fae27 100644
--- a/drivers/media/video/soc_camera.c
+++ b/drivers/media/video/soc_camera.c
@@ -895,7 +895,7 @@ static int soc_camera_init_i2c(struct soc_camera_device *icd,
 	icl->board_info->platform_data = icd;
 
 	subdev = v4l2_i2c_new_subdev_board(&ici->v4l2_dev, adap,
-				icl->module_name, icl->board_info, NULL);
+				icl->module_name, icl->board_info, NULL, 0);
 	if (!subdev)
 		goto ei2cnd;
 
diff --git a/drivers/media/video/v4l2-common.c b/drivers/media/video/v4l2-common.c
index bbda421..4265562 100644
--- a/drivers/media/video/v4l2-common.c
+++ b/drivers/media/video/v4l2-common.c
@@ -838,7 +838,8 @@ EXPORT_SYMBOL_GPL(v4l2_i2c_subdev_init);
 /* Load an i2c sub-device. */
 struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device *v4l2_dev,
 		struct i2c_adapter *adapter, const char *module_name,
-		struct i2c_board_info *info, const unsigned short *probe_addrs)
+		struct i2c_board_info *info, const unsigned short *probe_addrs,
+		int enable_devnode)
 {
 	struct v4l2_subdev *sd = NULL;
 	struct i2c_client *client;
@@ -868,9 +869,12 @@ struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device *v4l2_dev,
 	if (!try_module_get(client->driver->driver.owner))
 		goto error;
 	sd = i2c_get_clientdata(client);
+	if (!enable_devnode)
+		sd->flags &= ~V4L2_SUBDEV_FL_HAS_DEVNODE;
 
 	/* 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 +889,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;
 		}
 	}
 
@@ -911,7 +917,7 @@ struct v4l2_subdev *v4l2_i2c_new_subdev(struct v4l2_device *v4l2_dev,
 	info.addr = addr;
 
 	return v4l2_i2c_new_subdev_board(v4l2_dev, adapter, module_name,
-			&info, probe_addrs);
+			&info, probe_addrs, 0);
 }
 EXPORT_SYMBOL_GPL(v4l2_i2c_new_subdev);
 
@@ -981,7 +987,8 @@ void v4l2_spi_subdev_init(struct v4l2_subdev *sd, struct spi_device *spi,
 EXPORT_SYMBOL_GPL(v4l2_spi_subdev_init);
 
 struct v4l2_subdev *v4l2_spi_new_subdev(struct v4l2_device *v4l2_dev,
-		struct spi_master *master, struct spi_board_info *info)
+		struct spi_master *master, struct spi_board_info *info,
+		int enable_devnode)
 {
 	struct v4l2_subdev *sd = NULL;
 	struct spi_device *spi = NULL;
@@ -1000,6 +1007,8 @@ struct v4l2_subdev *v4l2_spi_new_subdev(struct v4l2_device *v4l2_dev,
 		goto error;
 
 	sd = spi_get_drvdata(spi);
+	if (!enable_devnode)
+		sd->flags &= ~V4L2_SUBDEV_FL_HAS_DEVNODE;
 
 	/* Register with the v4l2_device which increases the module's
 	   use count as well. */
diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
index 0ca7ec9..bcd47a0 100644
--- a/drivers/media/video/v4l2-dev.c
+++ b/drivers/media/video/v4l2-dev.c
@@ -376,13 +376,14 @@ static int get_index(struct video_device *vdev)
 }
 
 /**
- *	video_register_device - register video4linux devices
+ *	__video_register_device - register video4linux devices
  *	@vdev: video device structure we want to register
  *	@type: type of device to register
  *	@nr:   which device node number (0 == /dev/video0, 1 == /dev/video1, ...
  *             -1 == first free)
  *	@warn_if_nr_in_use: warn if the desired device node number
  *	       was already in use and another number was chosen instead.
+ *	@owner: module that owns the video device node
  *
  *	The registration code assigns minor numbers and device node numbers
  *	based on the requested type and registers the new device node with
@@ -401,9 +402,11 @@ 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)
+int __video_register_device(struct video_device *vdev, int type, int nr,
+		int warn_if_nr_in_use, struct module *owner)
 {
 	int i = 0;
 	int ret;
@@ -439,6 +442,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 = "v4l-subdev";
+		break;
 	default:
 		printk(KERN_ERR "%s called with unknown type: %d\n",
 		       __func__, type);
@@ -525,7 +531,7 @@ static int __video_register_device(struct video_device *vdev, int type, int nr,
 		vdev->cdev->ops = &v4l2_unlocked_fops;
 	else
 		vdev->cdev->ops = &v4l2_fops;
-	vdev->cdev->owner = vdev->fops->owner;
+	vdev->cdev->owner = owner;
 	ret = cdev_add(vdev->cdev, MKDEV(VIDEO_MAJOR, vdev->minor), 1);
 	if (ret < 0) {
 		printk(KERN_ERR "%s: cdev_add failed\n", __func__);
@@ -574,18 +580,7 @@ cleanup:
 	vdev->minor = -1;
 	return ret;
 }
-
-int video_register_device(struct video_device *vdev, int type, int nr)
-{
-	return __video_register_device(vdev, type, nr, 1);
-}
-EXPORT_SYMBOL(video_register_device);
-
-int video_register_device_no_warn(struct video_device *vdev, int type, int nr)
-{
-	return __video_register_device(vdev, type, nr, 0);
-}
-EXPORT_SYMBOL(video_register_device_no_warn);
+EXPORT_SYMBOL(__video_register_device);
 
 /**
  *	video_unregister_device - unregister a video4linux device
diff --git a/drivers/media/video/v4l2-device.c b/drivers/media/video/v4l2-device.c
index 5a7dc4a..b287aaa 100644
--- a/drivers/media/video/v4l2-device.c
+++ b/drivers/media/video/v4l2-device.c
@@ -115,18 +115,38 @@ 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 = 0;
+
 	/* 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;
+	strlcpy(vdev->name, sd->name, sizeof(vdev->name));
+	vdev->parent = v4l2_dev->dev;
+	vdev->fops = &v4l2_subdev_fops;
+	vdev->release = video_device_release_empty;
+	if (sd->flags & V4L2_SUBDEV_FL_HAS_DEVNODE) {
+		ret = __video_register_device(vdev, VFL_TYPE_SUBDEV, -1, 1,
+					      sd->owner);
+		if (ret < 0)
+			v4l2_device_unregister_subdev(sd);
+	}
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(v4l2_device_register_subdev);
 
@@ -135,10 +155,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..f016376
--- /dev/null
+++ b/drivers/media/video/v4l2-subdev.c
@@ -0,0 +1,65 @@
+/*
+ *  V4L2 subdevice support.
+ *
+ *  Copyright (C) 2010  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-common.h b/include/media/v4l2-common.h
index 6fc3d7a..496d158 100644
--- a/include/media/v4l2-common.h
+++ b/include/media/v4l2-common.h
@@ -148,7 +148,8 @@ struct i2c_board_info;
 
 struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device *v4l2_dev,
 		struct i2c_adapter *adapter, const char *module_name,
-		struct i2c_board_info *info, const unsigned short *probe_addrs);
+		struct i2c_board_info *info, const unsigned short *probe_addrs,
+		int enable_devnode);
 
 /* Initialize an v4l2_subdev with data from an i2c_client struct */
 void v4l2_i2c_subdev_init(struct v4l2_subdev *sd, struct i2c_client *client,
@@ -181,7 +182,8 @@ struct spi_device;
 /* Load an spi module and return an initialized v4l2_subdev struct.
    The client_type argument is the name of the chip that's on the adapter. */
 struct v4l2_subdev *v4l2_spi_new_subdev(struct v4l2_device *v4l2_dev,
-		struct spi_master *master, struct spi_board_info *info);
+		struct spi_master *master, struct spi_board_info *info,
+		int enable_devnode);
 
 /* Initialize an v4l2_subdev with data from an spi_device struct */
 void v4l2_spi_subdev_init(struct v4l2_subdev *sd, struct spi_device *spi,
diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
index bebe44b..195fa56 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;
@@ -98,15 +99,26 @@ struct video_device
 /* dev to video-device */
 #define to_video_device(cd) container_of(cd, struct video_device, dev)
 
+int __must_check __video_register_device(struct video_device *vdev, int type,
+		int nr, int warn_if_nr_in_use, struct module *owner);
+
 /* Register video devices. Note that if video_register_device fails,
    the release() callback of the video_device structure is *not* called, so
    the caller is responsible for freeing any data. Usually that means that
    you call video_device_release() on failure. */
-int __must_check video_register_device(struct video_device *vdev, int type, int nr);
+static inline int __must_check video_register_device(struct video_device *vdev,
+		int type, int nr)
+{
+	return __video_register_device(vdev, type, nr, 1, vdev->fops->owner);
+}
 
 /* Same as video_register_device, but no warning is issued if the desired
    device node number was already in use. */
-int __must_check video_register_device_no_warn(struct video_device *vdev, int type, int nr);
+static inline int __must_check video_register_device_no_warn(
+		struct video_device *vdev, int type, int nr)
+{
+	return __video_register_device(vdev, type, nr, 0, vdev->fops->owner);
+}
 
 /* Unregister video devices. Will do nothing if vdev == NULL or
    video_is_registered() returns false. */
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 6088316..dc0ccd3 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 */
@@ -402,9 +403,11 @@ struct v4l2_subdev_ops {
 #define V4L2_SUBDEV_NAME_SIZE 32
 
 /* Set this flag if this subdev is a i2c device. */
-#define V4L2_SUBDEV_FL_IS_I2C (1U << 0)
+#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_FL_IS_SPI			(1U << 1)
+/* Set this flag if this subdev needs a device node. */
+#define V4L2_SUBDEV_FL_HAS_DEVNODE		(1U << 2)
 
 /* Each instance of a subdev driver should create this struct, either
    stand-alone or embedded in a larger struct.
@@ -421,8 +424,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 +455,7 @@ static inline void v4l2_subdev_init(struct v4l2_subdev *sd,
 	sd->name[0] = '\0';
 	sd->grp_id = 0;
 	sd->priv = NULL;
+	sd->initialized = 1;
 }
 
 /* Call an ops of a v4l2_subdev, doing the right checks against
-- 
1.7.1

[RFC/PATCH v2 4/7] 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 f016376..37142ae 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 = 1;
+}
+EXPORT_SYMBOL(v4l2_subdev_init);
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index dc0ccd3..9ee45c8 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -444,19 +444,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 = 1;
-}
+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 v2 5/7] 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 |   16 ++++++++++++++++
 drivers/media/video/v4l2-subdev.c            |   24 ++++++++++++++++++++++++
 2 files changed, 40 insertions(+), 0 deletions(-)

diff --git a/Documentation/video4linux/v4l2-framework.txt b/Documentation/video4linux/v4l2-framework.txt
index 164bb0f..9c3f33c 100644
--- a/Documentation/video4linux/v4l2-framework.txt
+++ b/Documentation/video4linux/v4l2-framework.txt
@@ -331,6 +331,22 @@ argument to 0. Setting the argument to 1 will only enable device node
 registration if the sub-device driver has set the V4L2_SUBDEV_FL_HAS_DEVNODE
 flag.
 
+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 37142ae..0ebd760 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 v2 6/7] 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                  |   10 ++++
 3 files changed, 98 insertions(+), 1 deletions(-)

diff --git a/Documentation/video4linux/v4l2-framework.txt b/Documentation/video4linux/v4l2-framework.txt
index 9c3f33c..89bd881 100644
--- a/Documentation/video4linux/v4l2-framework.txt
+++ b/Documentation/video4linux/v4l2-framework.txt
@@ -347,6 +347,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 0ebd760..31bec67 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_FL_HAS_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_FL_HAS_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_FL_HAS_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 9ee45c8..55a8c93 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.
@@ -408,6 +414,8 @@ struct v4l2_subdev_ops {
 #define V4L2_SUBDEV_FL_IS_SPI			(1U << 1)
 /* Set this flag if this subdev needs a device node. */
 #define V4L2_SUBDEV_FL_HAS_DEVNODE		(1U << 2)
+/* Set this flag if this subdev generates events. */
+#define V4L2_SUBDEV_FL_HAS_EVENTS		(1U << 3)
 
 /* Each instance of a subdev driver should create this struct, either
    stand-alone or embedded in a larger struct.
@@ -427,6 +435,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 v2 7/7] 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 89bd881..581e7db 100644
--- a/Documentation/video4linux/v4l2-framework.txt
+++ b/Documentation/video4linux/v4l2-framework.txt
@@ -365,6 +365,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 31bec67..ce47772 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 v2 6/7] v4l: subdev: Events support

[Mauro Carvalho Chehab]

Em 09-07-2010 12:31, Laurent Pinchart escreveu:
> 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                  |   10 ++++
>  3 files changed, 98 insertions(+), 1 deletions(-)
> 
> diff --git a/Documentation/video4linux/v4l2-framework.txt b/Documentation/video4linux/v4l2-framework.txt
> index 9c3f33c..89bd881 100644
> --- a/Documentation/video4linux/v4l2-framework.txt
> +++ b/Documentation/video4linux/v4l2-framework.txt
> @@ -347,6 +347,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 0ebd760..31bec67 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;

Why to allocate/init the file handler for devices that don't need it?
I would just move the above logic to happen only if V4L2_SUBDEV_FL_HAS_EVENTS.

> +
> +	if (sd->flags & V4L2_SUBDEV_FL_HAS_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_FL_HAS_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);

Hmm... shouldn't it test also for V4L2_SUBDEV_FL_HAS_EVENTS?

I would do, instead:

	if (fh) {
		switch(cmd) {
			/* FH events logic */
		}
	}


> +
>  	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_FL_HAS_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 9ee45c8..55a8c93 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.
> @@ -408,6 +414,8 @@ struct v4l2_subdev_ops {
>  #define V4L2_SUBDEV_FL_IS_SPI			(1U << 1)
>  /* Set this flag if this subdev needs a device node. */
>  #define V4L2_SUBDEV_FL_HAS_DEVNODE		(1U << 2)
> +/* Set this flag if this subdev generates events. */
> +#define V4L2_SUBDEV_FL_HAS_EVENTS		(1U << 3)
>  
>  /* Each instance of a subdev driver should create this struct, either
>     stand-alone or embedded in a larger struct.
> @@ -427,6 +435,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) \

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

[Mauro Carvalho Chehab]

Em 09-07-2010 12:31, Laurent Pinchart escreveu:
> 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 89bd881..581e7db 100644
> --- a/Documentation/video4linux/v4l2-framework.txt
> +++ b/Documentation/video4linux/v4l2-framework.txt
> @@ -365,6 +365,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 31bec67..ce47772 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;

Hmm... private ioctls at subdev... I'm not sure if I like this idea. I prefer to merge this patch
only after having a driver actually needing it, after discussing why not using a standard ioctl
for that driver.

Cheers,
Mauro.

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

[Hans Verkuil]

On Saturday 10 July 2010 16:08:46 Mauro Carvalho Chehab wrote:
> Em 09-07-2010 12:31, Laurent Pinchart escreveu:
> > 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 89bd881..581e7db 100644
> > --- a/Documentation/video4linux/v4l2-framework.txt
> > +++ b/Documentation/video4linux/v4l2-framework.txt
> > @@ -365,6 +365,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 31bec67..ce47772 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;
> 
> Hmm... private ioctls at subdev... I'm not sure if I like this idea. I prefer to merge this patch
> only after having a driver actually needing it, after discussing why not using a standard ioctl
> for that driver.

Part of the reason for making these subdev device nodes is to actually allow
private ioctls (after properly discussing it and with documentation). SoCs tend
to have a lot of very hardware specific features that do not translate to generic
ioctls. Until now these are either ignored or handled through custom drivers, but
but it is much better to handle them in a 'controlled' fashion.

Regards,

	Hans

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

Re: [RFC/PATCH v2 0/7] V4L2 subdev userspace API

[Hans Verkuil]

Looks good, I have no comments for this patch series.

Regards,

	Hans

On Friday 09 July 2010 17:31:45 Laurent Pinchart wrote:
> Hi everybody,
> 
> Here's the second version of the V4L2 subdev userspace API patches. Comments
> received on the first version have been incorporated, with the exception of the
> video_usercopy usage as this is still under discussion.
> 
> A problem with module reference counting has also been solved by setting the
> owner of the subdev device node to the subdev driver, not videodev.ko. This
> required a change to the v4l2_i2c_new_subdev_board function prototype, and thus
> a modification to the radio-si4713 driver.
> 
> Due to the fast review (thanks everybody) I haven't had time to finish the media
> controller core patches, but they will arrive soon.
> 
> Laurent Pinchart (6):
>   v4l: subdev: Don't require core operations
>   v4l: Merge v4l2_i2c_new_subdev_cfg and v4l2_i2c_new_subdev
>   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 |   57 +++++++++
>  drivers/media/radio/radio-si4713.c           |    2 +-
>  drivers/media/video/Makefile                 |    2 +-
>  drivers/media/video/soc_camera.c             |    2 +-
>  drivers/media/video/v4l2-common.c            |   22 ++--
>  drivers/media/video/v4l2-dev.c               |   27 ++---
>  drivers/media/video/v4l2-device.c            |   25 ++++-
>  drivers/media/video/v4l2-subdev.c            |  172 ++++++++++++++++++++++++++
>  include/media/v4l2-common.h                  |   21 +---
>  include/media/v4l2-dev.h                     |   18 +++-
>  include/media/v4l2-subdev.h                  |   40 ++++---
>  11 files changed, 326 insertions(+), 62 deletions(-)
>  create mode 100644 drivers/media/video/v4l2-subdev.c
> 
> --
> 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 v2 7/7] v4l: subdev: Generic ioctl support

[Mauro Carvalho Chehab]

Em 10-07-2010 13:31, Hans Verkuil escreveu:
> On Saturday 10 July 2010 16:08:46 Mauro Carvalho Chehab wrote:
>> Em 09-07-2010 12:31, Laurent Pinchart escreveu:

>> Hmm... private ioctls at subdev... I'm not sure if I like this idea. I prefer to merge this patch
>> only after having a driver actually needing it, after discussing why not using a standard ioctl
>> for that driver.
> 
> Part of the reason for making these subdev device nodes is to actually allow
> private ioctls (after properly discussing it and with documentation). SoCs tend
> to have a lot of very hardware specific features that do not translate to generic
> ioctls. Until now these are either ignored or handled through custom drivers, but
> but it is much better to handle them in a 'controlled' fashion.

I understand that SoC's may have lots of features that are currently not being exposed,
but if they'll be either be shown as CTRL or as new ioctls need further discussions. That's
why I prefer to receive this patch in a patch series where such needed is required.

Cheers,
Mauro.

RE: [RFC/PATCH v2 7/7] v4l: subdev: Generic ioctl support

[Pawel Osciak]

>Laurent Pinchart wrote:
>Sent: Friday, July 09, 2010 5:32 PM
>To: linux-media@vger.kernel.org
>Cc: sakari.ailus@maxwell.research.nokia.com
>Subject: [RFC/PATCH v2 7/7] v4l: subdev: Generic ioctl support
>
>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 89bd881..581e7db 100644
>--- a/Documentation/video4linux/v4l2-framework.txt
>+++ b/Documentation/video4linux/v4l2-framework.txt
>@@ -365,6 +365,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 31bec67..ce47772 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;

Hi,
one remark about this:

(@Laurent: this is what've been discussing on Saturday on IRC)

It looks to me that with this, a subdev will only be able to have either
kernel or userspace-callable ioctls, not both. Note, that I don't mean one
ioctl callable from both, but 2 ioctls, where one is kernel-callable and
the other is userspace-callable. Technically that would work, but would
become a security risk. Malicious userspace would be able to call the
kernel-only ioctl.

So a driver has to be careful not to expose a node to the userspace if
it has kernel-callable subdev ioctls.

As long as drivers obey that rule and we do not require both types of
ioctls in one subdev, there is no problem.

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

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

[Sakari Ailus]

Hi Mauro,

Mauro Carvalho Chehab wrote:
> Em 09-07-2010 12:31, Laurent Pinchart escreveu:
>> 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                  |   10 ++++
>>  3 files changed, 98 insertions(+), 1 deletions(-)
>>
>> diff --git a/Documentation/video4linux/v4l2-framework.txt b/Documentation/video4linux/v4l2-framework.txt
>> index 9c3f33c..89bd881 100644
>> --- a/Documentation/video4linux/v4l2-framework.txt
>> +++ b/Documentation/video4linux/v4l2-framework.txt
>> @@ -347,6 +347,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 0ebd760..31bec67 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;
> 
> Why to allocate/init the file handler for devices that don't need it?
> I would just move the above logic to happen only if V4L2_SUBDEV_FL_HAS_EVENTS.

This patch actually adds subdev support for V4L2 file handles AND
events. v4l2_fh is also used to support probe formats on subdevs (not
contained in this patchset).

That v4l2_fh_init() function just initialises a few fields, there is no
allocation being done. The v4l2_fh structure will be part of v4l2_subdev_fh:

struct v4l2_subdev_fh {
        struct v4l2_fh vfh;
        struct v4l2_mbus_framefmt *probe_fmt;
        struct v4l2_rect *probe_crop;
};

(Again not yet in this patchset.) The probe formats are a new concept to
allow trying formats across the whole pipeline for which the old try_fmt
wasn't suitable for: memory vs. bus format and pads matter.

>> +
>> +	if (sd->flags & V4L2_SUBDEV_FL_HAS_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_FL_HAS_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);
> 
> Hmm... shouldn't it test also for V4L2_SUBDEV_FL_HAS_EVENTS?
> 
> I would do, instead:
> 
> 	if (fh) {
> 		switch(cmd) {
> 			/* FH events logic */
> 		}
> 	}

No need to. If the subdev doesn't support events it likely should not
define handlers for event specific calls, right? v4l2_subdev_call()
behaves well if the handler is NULL.

Both would work equally well, I guess.

Best regards,

-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com

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

[Laurent Pinchart]

Hi Sakari,

On Monday 12 July 2010 13:06:34 Sakari Ailus wrote:
> Mauro Carvalho Chehab wrote:
> > Em 09-07-2010 12:31, Laurent Pinchart escreveu:

[snip]

> >> diff --git a/drivers/media/video/v4l2-subdev.c
> >> b/drivers/media/video/v4l2-subdev.c index 0ebd760..31bec67 100644
> >> --- a/drivers/media/video/v4l2-subdev.c
> >> +++ b/drivers/media/video/v4l2-subdev.c
> >> @@ -18,26 +18,64 @@

[snip]

> >> 
> >>  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;
> > 
> > Why to allocate/init the file handler for devices that don't need it?
> > I would just move the above logic to happen only if
> > V4L2_SUBDEV_FL_HAS_EVENTS.
> 
> This patch actually adds subdev support for V4L2 file handles AND
> events. v4l2_fh is also used to support probe formats on subdevs (not
> contained in this patchset).
> 
> That v4l2_fh_init() function just initialises a few fields, there is no
> allocation being done. The v4l2_fh structure will be part of
> v4l2_subdev_fh:
> 
> struct v4l2_subdev_fh {
>         struct v4l2_fh vfh;
>         struct v4l2_mbus_framefmt *probe_fmt;
>         struct v4l2_rect *probe_crop;
> };
> 
> (Again not yet in this patchset.) The probe formats are a new concept to
> allow trying formats across the whole pipeline for which the old try_fmt
> wasn't suitable for: memory vs. bus format and pads matter.

Mauro's point was that v4l2_fh isn't needed if the subdev doesn't support 
events. subdev_fh will likely be mandatory, but that's for a future patch, so 
I can make the v4l2_fh allocation optional for now.

[snip]

> >> @@ -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_FL_HAS_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);
> > 
> > Hmm... shouldn't it test also for V4L2_SUBDEV_FL_HAS_EVENTS?
> > 
> > I would do, instead:
> > 	if (fh) {
> > 	
> > 		switch(cmd) {
> > 		
> > 			/* FH events logic */
> > 		
> > 		}
> > 	
> > 	}
> 
> No need to. If the subdev doesn't support events it likely should not
> define handlers for event specific calls, right? v4l2_subdev_call()
> behaves well if the handler is NULL.
> 
> Both would work equally well, I guess.

The check in v4l2_subdev_call is fine, I don't think we need another check.

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Pawel,

On Monday 12 July 2010 11:33:52 Pawel Osciak wrote:
> > On Friday 9 July 2010 17:32:00 Laurent Pinchart wrote:

[snip]

> >diff --git a/drivers/media/video/v4l2-subdev.c b/drivers/media/video/v4l2-
> >subdev.c
> >index 31bec67..ce47772 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;
> 
> Hi,
> one remark about this:
> 
> (@Laurent: this is what've been discussing on Saturday on IRC)
> 
> It looks to me that with this, a subdev will only be able to have either
> kernel or userspace-callable ioctls, not both. Note, that I don't mean one
> ioctl callable from both, but 2 ioctls, where one is kernel-callable and
> the other is userspace-callable. Technically that would work, but would
> become a security risk. Malicious userspace would be able to call the
> kernel-only ioctl.
> 
> So a driver has to be careful not to expose a node to the userspace if
> it has kernel-callable subdev ioctls.
> 
> As long as drivers obey that rule and we do not require both types of
> ioctls in one subdev, there is no problem.

I'll drop this patch for now, as Mauro would like it to go in with a driver 
that uses it. I'll resubmit it with the OMAP3 ISP driver.

-- 
Regards,

Laurent Pinchart