diff mbox

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

Message ID 1278503608-9126-3-git-send-email-laurent.pinchart@ideasonboard.com (mailing list archive)
State RFC
Headers show

Commit Message

Laurent Pinchart July 7, 2010, 11:53 a.m. UTC
None

Comments

Laurent Pinchart July 9, 2010, 8:57 a.m. UTC | #1
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.
diff mbox

Patch

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