Re: v4l2_device/v4l2_subdev: please review

Re: v4l2_device/v4l2_subdev: please review

[Trilok Soni]

Hi Hans,

On Tue, Nov 25, 2008 at 3:39 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> Hi all,
>
> I've finally tracked down the last oops so I could make a new tree with
> all the latest changes.
>

Please send these patches to mailing list (git-send-email?) for easy
review. Also CCing LKML for wider view is also good, as we are doing
some core changes right?

-- 
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni

Re: v4l2_device/v4l2_subdev: please review

[Hans Verkuil]

On Tuesday 25 November 2008 07:51:25 Trilok Soni wrote:
> Hi Hans,
>
> On Tue, Nov 25, 2008 at 3:39 AM, Hans Verkuil <hverkuil@xs4all.nl> 
wrote:
> > Hi all,
> >
> > I've finally tracked down the last oops so I could make a new tree
> > with all the latest changes.
>
> Please send these patches to mailing list (git-send-email?) for easy
> review. Also CCing LKML for wider view is also good, as we are doing
> some core changes right?

I'm not going to spam the list with these quite big patches. Just go to 
http://linuxtv.org/hg/~hverkuil/v4l-dvb-ng/ and click on the 'raw' link 
after each change to see the patch. Most of these changes are just 
boring i2c driver conversions.

We are adding to the v4l core, but the changes do not affect existing 
v4l drivers let alone other subsystems. Although I should probably have 
added the omap list.

Regards,

	Hans

Re: v4l2_device/v4l2_subdev: please review

[Trilok Soni]

Hi Hans,

>
> I'm not going to spam the list with these quite big patches. Just go to
> http://linuxtv.org/hg/~hverkuil/v4l-dvb-ng/ and click on the 'raw' link
> after each change to see the patch. Most of these changes are just
> boring i2c driver conversions.

It is hard to review these patches from this link, as if you submit
the patch to ML then someone can just give inline comments to your
patch, otherwise reviewer has to copy that code which he/she wants to
comment while replying and not easy to track too. I don't know size
limit of this v4l2 ML, but linux-kernel ML can receive somewhat big
patches I believe.

>
> We are adding to the v4l core, but the changes do not affect existing
> v4l drivers let alone other subsystems. Although I should probably have
> added the omap list.

OMAP + soc-camera + v4l2-int-* community would be more interested to
see these patches as they need to change their sensor/controller
drivers to adapt your changes.

-- 
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni

Re: v4l2_device/v4l2_subdev: please review (PATCH 1/3)

[Hans Verkuil]

On Tuesday 25 November 2008 08:27:29 Trilok Soni wrote:
> Hi Hans,
>
> > I'm not going to spam the list with these quite big patches. Just
> > go to http://linuxtv.org/hg/~hverkuil/v4l-dvb-ng/ and click on the
> > 'raw' link after each change to see the patch. Most of these
> > changes are just boring i2c driver conversions.
>
> It is hard to review these patches from this link, as if you submit
> the patch to ML then someone can just give inline comments to your
> patch, otherwise reviewer has to copy that code which he/she wants to
> comment while replying and not easy to track too. I don't know size
> limit of this v4l2 ML, but linux-kernel ML can receive somewhat big
> patches I believe.
>
> > We are adding to the v4l core, but the changes do not affect
> > existing v4l drivers let alone other subsystems. Although I should
> > probably have added the omap list.
>
> OMAP + soc-camera + v4l2-int-* community would be more interested to
> see these patches as they need to change their sensor/controller
> drivers to adapt your changes.

As requested, the patches as separate posts for review.

	Hans

# HG changeset patch
# User Hans Verkuil <hverkuil@xs4all.nl>
# Date 1227560990 -3600
# Node ID d9ec70c0b0c55e18813f91218c6da6212ca9b7e6
# Parent b63737bf9eef1ff8494cb7fbc2e818e6aff7a34f
v4l2: add v4l2_device and v4l2_subdev structs to the v4l2 framework.

From: Hans Verkuil <hverkuil@xs4all.nl>

Start implementing a proper v4l2 framework as discussed during the
Linux Plumbers Conference 2008.

Introduces v4l2_device (for device instances) and v4l2_subdev (representing
sub-device instances).

Priority: normal

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

--- a/linux/drivers/media/video/Makefile	Mon Nov 24 10:51:20 2008 -0200
+++ b/linux/drivers/media/video/Makefile	Mon Nov 24 22:09:50 2008 +0100
@@ -8,7 +8,7 @@ msp3400-objs	:=	msp3400-driver.o msp3400
 
 stkwebcam-objs	:=	stk-webcam.o stk-sensor.o
 
-videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o
+videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-subdev.o
 
 obj-$(CONFIG_VIDEO_DEV) += videodev.o v4l2-compat-ioctl32.o v4l2-int-device.o
 
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/linux/Documentation/video4linux/v4l2-framework.txt	Mon Nov 24 22:09:50 2008 +0100
@@ -0,0 +1,360 @@
+Overview of the V4L2 driver framework
+=====================================
+
+This text documents the various structures provided by the V4L2 framework and
+their relationships.
+
+
+Introduction
+------------
+
+The V4L2 drivers tend to be very complex due to the complexity of the
+hardware: most devices have multiple ICs, export multiple device nodes in
+/dev, and create also non-V4L2 devices such as DVB, ALSA, FB, I2C and input
+(IR) devices.
+
+Especially the fact that V4L2 drivers have to setup supporting ICs to
+do audio/video muxing/encoding/decoding makes it more complex than most.
+Usually these ICs are connected to the main bridge driver through one or
+more I2C busses, but other busses can also be used. Such devices are
+called 'sub-devices'.
+
+For a long time the framework was limited to the video_device struct for
+creating V4L device nodes and video_buf for handling the video buffers
+(note that this document does not discuss the video_buf framework).
+
+This meant that all drivers had to do the setup of device instances and
+connecting to sub-devices themselves. Some of this is quite complicated
+to do right and many drivers never did do it correctly.
+
+There is also a lot of common code that could never be refactored due to
+the lack of a framework.
+
+So this framework sets up the basic building blocks that all drivers
+need and this same framework should make it much easier to refactor
+common code into utility functions shared by all drivers.
+
+
+Structure of a driver
+---------------------
+
+All drivers have the following structure:
+
+1) A struct for each device instance containing the device state.
+
+2) A way of initializing and commanding sub-devices.
+
+3) Creating V4L2 device nodes (/dev/videoX, /dev/vbiX, /dev/radioX and
+   /dev/vtxX) and keeping track of device-node specific data.
+
+4) Filehandle-specific structs containing per-filehandle data.
+
+This is a rough schematic of how it all relates:
+
+    device instances
+      |
+      +-sub-device instances
+      |
+      \-V4L2 device nodes
+	  |
+	  \-filehandle instances
+
+
+Structure of the framework
+--------------------------
+
+The framework closely resembles the driver structure: it has a v4l2_device
+struct for the device instance data, a v4l2_subdev struct to refer to
+sub-device instances, the video_device struct stores V4L2 device node data
+and a v4l2_fh struct keeps track of filehandle instances (TODO: not yet
+implemented).
+
+
+struct v4l2_device
+------------------
+
+Each device instance is represented by a struct v4l2_device (v4l2-device.h).
+Very simple devices can just allocate this struct, but most of the time you
+would embed this struct inside a larger struct.
+
+You must register the device instance:
+
+	v4l2_device_register(struct device *dev, struct v4l2_device *v4l2_dev);
+
+Registration will initialize the v4l2_device struct and link dev->platform_data
+to v4l2_dev. Registration will also set v4l2_dev->name to a value derived from
+dev (driver name followed by the bus_id, to be precise). You may change the
+name after registration if you want.
+
+You unregister with:
+
+	v4l2_device_unregister(struct v4l2_device *v4l2_dev);
+
+Unregistering will also automatically unregister all subdevs from the device.
+
+Sometimes you need to iterate over all devices registered by a specific
+driver. This is usually the case if multiple device drivers use the same
+hardware. E.g. the ivtvfb driver is a framebuffer driver that uses the ivtv
+hardware. The same is true for alsa drivers for example.
+
+You can iterate over all registered devices as follows:
+
+static int callback(struct device *dev, void *p)
+{
+	struct v4l2_device *v4l2_dev = dev->platform_data;
+
+	/* test if this device was inited */
+	if (v4l2_dev == NULL)
+		return 0;
+	...
+	return 0;
+}
+
+int iterate(void *p)
+{
+	struct device_driver *drv;
+	int err;
+
+	/* Find driver 'ivtv' on the PCI bus.
+	   pci_bus_type is a global. For USB busses use usb_bus_type. */
+	drv = driver_find("ivtv", &pci_bus_type);
+	/* iterate over all ivtv device instances */
+	err = driver_for_each_device(drv, NULL, p, callback);
+	put_driver(drv);
+	return err;
+}
+
+Sometimes you need to keep a running counter of the device instance. This is
+commonly used to map a device instance to an index of a module option array.
+
+The recommended approach is as follows:
+
+static atomic_t drv_instance = ATOMIC_INIT(0);
+
+static int __devinit drv_probe(struct pci_dev *dev,
+				const struct pci_device_id *pci_id)
+{
+	...
+	state->instance = atomic_inc_return(&drv_instance) - 1;
+}
+
+
+struct v4l2_subdev
+------------------
+
+Many drivers need to communicate with sub-devices. These devices can do all
+sort of tasks, but most commonly they handle audio and/or video muxing,
+encoding or decoding. For webcams common sub-devices are sensors and camera
+controllers.
+
+Usually these are I2C devices, but not necessarily. In order to provide the
+driver with a consistent interface to these sub-devices the v4l2_subdev struct
+(v4l2-subdev.h) was created.
+
+Each sub-device driver must have a v4l2_subdev struct. This struct can be
+stand-alone for simple sub-devices or it might be embedded in a larger struct
+if more state information needs to be stored. Usually there is a low-level
+device struct (e.g. i2c_client) that contains the device data as setup
+by the kernel. It is recommended to store that pointer in the private
+data of v4l2_subdev using v4l2_set_subdevdata(). That makes it easy to go
+from a v4l2_subdev to the actual low-level bus-specific device data.
+
+You also need a way to go from the low-level struct to v4l2_subdev. For the
+common i2c_client struct the i2c_set_clientdata() call is used to store a
+v4l2_subdev pointer, for other busses you may have to use other methods.
+
+From the bridge driver perspective you load the sub-device module and somehow
+obtain the v4l2_subdev pointer. For i2c devices this is easy: you call
+i2c_get_clientdata(). For other busses something similar needs to be done.
+Helper functions exists for sub-devices on an I2C bus that do most of this
+tricky work for you.
+
+Each v4l2_subdev contains function pointers that sub-device drivers can
+implement (or leave NULL if it is not applicable). Since sub-devices can do
+so many different things and you do not want to end up with a huge ops struct
+of which only a handful of ops are commonly implemented, the function pointers
+are sorted according to category and each category has its own ops struct.
+
+The top-level ops struct contains pointers to the category ops structs, which
+may be NULL if the subdev driver does not support anything from that category.
+
+It looks like this:
+
+struct v4l2_subdev_core_ops {
+	int (*g_chip_ident)(struct v4l2_subdev *sd, struct v4l2_chip_ident *chip);
+	int (*log_status)(struct v4l2_subdev *sd);
+	int (*init)(struct v4l2_subdev *sd, u32 val);
+	...
+};
+
+struct v4l2_subdev_tuner_ops {
+	...
+};
+
+struct v4l2_subdev_audio_ops {
+	...
+};
+
+struct v4l2_subdev_video_ops {
+	...
+};
+
+struct v4l2_subdev_ops {
+	const struct v4l2_subdev_core_ops  *core;
+	const struct v4l2_subdev_tuner_ops *tuner;
+	const struct v4l2_subdev_audio_ops *audio;
+	const struct v4l2_subdev_video_ops *video;
+};
+
+The core ops are common to all subdevs, the other categories are implemented
+depending on the sub-device. E.g. a video device is unlikely to support the
+audio ops and vice versa.
+
+This setup limits the number of function pointers while still making it easy
+to add new ops and categories.
+
+A sub-device driver initializes the v4l2_subdev struct using:
+
+	v4l2_subdev_init(subdev, &ops);
+
+Afterwards you need to initialize subdev->name with a unique name and set the
+module owner. This is done for you if you use the i2c helper functions.
+
+A device (bridge) driver needs to register the v4l2_subdev with the
+v4l2_device:
+
+	int err = v4l2_device_register_subdev(device, subdev);
+
+This can fail if the subdev module disappeared before it could be registered.
+After this function was called successfully the subdev->dev field points to
+the v4l2_device.
+
+You can unregister a sub-device using:
+
+	v4l2_device_unregister_subdev(subdev);
+
+Afterwards the subdev module can be unloaded and subdev->dev == NULL.
+
+You can call an ops function either directly:
+
+	err = subdev->ops->core->g_chip_ident(subdev, &chip);
+
+but it is better and easier to use this macro:
+
+	err = v4l2_subdev_call(subdev, core, g_chip_ident, &chip);
+
+The macro will to the right NULL pointer checks and returns -ENODEV if subdev
+is NULL, -ENOIOCTLCMD if either subdev->core or subdev->core->g_chip_ident is
+NULL, or the actual result of the subdev->ops->core->g_chip_ident ops.
+
+It is also possible to call all or a subset of the sub-devices:
+
+	v4l2_device_call_all(dev, 0, core, g_chip_ident, &chip);
+
+Any subdev that does not support this ops is skipped and error results are
+ignored. If you want to check for errors use this:
+
+	err = v4l2_device_call_until_err(dev, 0, core, g_chip_ident, &chip);
+
+Any error except -ENOIOCTLCMD will exit the loop with that error. If no
+errors (except -ENOIOCTLCMD) occured, then 0 is returned.
+
+The second argument to both calls is a group ID. If 0, then all subdevs are
+called. If non-zero, then only those whose group ID match that value will
+be called. Before a bridge driver registers a subdev it can set subdev->grp_id
+to whatever value it wants (it's 0 by default). This value is owned by the
+bridge driver and the sub-device driver will never modify or use it.
+
+The group ID gives the bridge driver more control how callbacks are called.
+For example, there may be multiple audio chips on a board, each capable of
+changing the volume. But usually only one will actually be used when the
+user want to change the volume. You can set the group ID for that subdev to
+e.g. AUDIO_CONTROLLER and specify that as the group ID value when calling
+v4l2_device_call_all(). That ensures that it will only go to the subdev
+that needs it.
+
+The advantage of using v4l2_subdev is that it is a generic struct and does
+not contain any knowledge about the underlying hardware. So a driver might
+contain several subdevs that use an I2C bus, but also a subdev that is
+controlled through GPIO pins. This distinction is only relevant when setting
+up the device, but once the subdev is registered it is completely transparent.
+
+
+I2C sub-device drivers
+----------------------
+
+Since these drivers are so common, special helper functions are available to
+ease the use of these drivers (v4l2-common.h).
+
+The recommended method of adding v4l2_subdev support to an I2C driver is to
+embed the v4l2_subdev struct into the state struct that is created for each
+I2C device instance. Very simple devices have no state struct and in that case
+you can just create a v4l2_subdev directly.
+
+Initialize the v4l2_subdev struct as follows:
+
+	v4l2_i2c_subdev_init(&state->sd, client, subdev_ops);
+
+This function will fill in all the fields of v4l2_subdev and ensure that the
+v4l2_subdev and i2c_client both point to one another.
+
+You should also add a helper inline function to go from a v4l2_subdev pointer
+to the state struct:
+
+static inline struct subdev_state *to_state(struct v4l2_subdev *sd)
+{
+	return container_of(sd, struct subdev_state, sd);
+}
+
+Use this to go from the v4l2_subdev struct to the i2c_client struct:
+
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+And this to go from an i2c_client to a v4l2_subdev struct:
+
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+
+Finally you need to make a command function to make driver->command()
+call the right subdev_ops functions:
+
+static int subdev_command(struct i2c_client *client, unsigned cmd, void *arg)
+{
+	return v4l2_subdev_command(i2c_get_clientdata(client), cmd, arg);
+}
+
+If driver->command is never used then you can leave this out. Eventually the
+driver->command usage should be removed from v4l.
+
+Make sure to call v4l2_device_unregister_subdev(sd) when the remove() callback
+is called. This will unregister the sub-device from the bridge driver. It is
+safe to call this even if the sub-device was never registered.
+
+
+The bridge driver also has some helper functions it can use:
+
+struct v4l2_subdev *sd = v4l2_i2c_new_subdev(adapter, "module_foo", "chipid", 0x36);
+
+This loads the given module (can be NULL if no module needs to be loaded) and
+calls i2c_new_device() with the given i2c_adapter and chip/address arguments.
+If all goes well, then it registers the subdev with the v4l2_device. It gets
+the v4l2_device by calling i2c_get_adapdata(adapter), so you should make sure
+that adapdata is set to v4l2_device when you setup the i2c_adapter in your
+driver.
+
+You can also use v4l2_i2c_new_probed_subdev() which is very similar to
+v4l2_i2c_new_subdev(), except that it has an array of possible I2C addresses
+that it should probe. Internally it calls i2c_new_probed_device().
+
+Both functions return NULL if something went wrong.
+
+
+struct video_device
+-------------------
+
+TODO: document.
+
+
+struct v4l2_fh
+--------------
+
+TODO: document.
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/linux/drivers/media/video/v4l2-device.c	Mon Nov 24 22:09:50 2008 +0100
@@ -0,0 +1,80 @@
+/*
+    V4L2 device support.
+
+    Copyright (C) 2008  Hans Verkuil <hverkuil@xs4all.nl>
+
+    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/i2c.h>
+#include <linux/videodev2.h>
+#include <media/v4l2-device.h>
+
+void v4l2_device_register(struct device *dev, struct v4l2_device *v4l2_dev)
+{
+	BUG_ON(!dev || !v4l2_dev || dev->platform_data);
+	INIT_LIST_HEAD(&v4l2_dev->subdevs);
+	mutex_init(&v4l2_dev->lock);
+	v4l2_dev->dev = dev;
+	snprintf(v4l2_dev->name, sizeof(v4l2_dev->name), "%s %s",
+			dev->driver->name, dev->bus_id);
+	dev->platform_data = v4l2_dev;
+}
+EXPORT_SYMBOL(v4l2_device_register);
+
+void v4l2_device_unregister(struct v4l2_device *v4l2_dev)
+{
+	struct v4l2_subdev *sd, *next;
+
+	BUG_ON(!v4l2_dev || !v4l2_dev->dev || !v4l2_dev->dev->platform_data);
+	v4l2_dev->dev->platform_data = NULL;
+	/* unregister subdevs */
+	list_for_each_entry_safe(sd, next, &v4l2_dev->subdevs, list)
+		v4l2_device_unregister_subdev(sd);
+
+	v4l2_dev->dev = NULL;
+}
+EXPORT_SYMBOL(v4l2_device_unregister);
+
+int v4l2_device_register_subdev(struct v4l2_device *dev, struct v4l2_subdev *sd)
+{
+	/* Check that sd->dev is not set and that the subdev's name is
+	   filled in. */
+	BUG_ON(!dev || !sd || sd->dev || !sd->name[0]);
+	if (!try_module_get(sd->owner))
+		return -ENODEV;
+	sd->dev = dev;
+	mutex_lock(&dev->lock);
+	list_add_tail(&sd->list, &dev->subdevs);
+	mutex_unlock(&dev->lock);
+	return 0;
+}
+EXPORT_SYMBOL(v4l2_device_register_subdev);
+
+void v4l2_device_unregister_subdev(struct v4l2_subdev *sd)
+{
+	BUG_ON(!sd);
+	/* return if it isn't registered */
+	if (sd->dev == NULL)
+		return;
+	mutex_lock(&sd->dev->lock);
+	list_del(&sd->list);
+	mutex_unlock(&sd->dev->lock);
+	sd->dev = NULL;
+	module_put(sd->owner);
+}
+EXPORT_SYMBOL(v4l2_device_unregister_subdev);
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/linux/drivers/media/video/v4l2-subdev.c	Mon Nov 24 22:09:50 2008 +0100
@@ -0,0 +1,108 @@
+/*
+    V4L2 sub-device support.
+
+    Copyright (C) 2008  Hans Verkuil <hverkuil@xs4all.nl>
+
+    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/i2c.h>
+#include <linux/videodev2.h>
+#include <media/v4l2-subdev.h>
+
+int v4l2_subdev_command(struct v4l2_subdev *sd, unsigned cmd, void *arg)
+{
+	switch (cmd) {
+	case VIDIOC_QUERYCTRL:
+		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_QUERYMENU:
+		return v4l2_subdev_call(sd, core, queryctrl, arg);
+	case VIDIOC_LOG_STATUS:
+		return v4l2_subdev_call(sd, core, log_status);
+	case VIDIOC_G_CHIP_IDENT:
+		return v4l2_subdev_call(sd, core, g_chip_ident, arg);
+	case VIDIOC_INT_S_STANDBY:
+		return v4l2_subdev_call(sd, core, s_standby, *(u32 *)arg);
+	case VIDIOC_INT_RESET:
+		return v4l2_subdev_call(sd, core, reset, *(u32 *)arg);
+	case VIDIOC_INT_S_GPIO:
+		return v4l2_subdev_call(sd, core, s_gpio, *(u32 *)arg);
+	case VIDIOC_INT_INIT:
+		return v4l2_subdev_call(sd, core, init, *(u32 *)arg);
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+	case VIDIOC_DBG_G_REGISTER:
+		return v4l2_subdev_call(sd, core, g_register, arg);
+	case VIDIOC_DBG_S_REGISTER:
+		return v4l2_subdev_call(sd, core, s_register, arg);
+#endif
+
+	case VIDIOC_INT_S_TUNER_MODE:
+		return v4l2_subdev_call(sd, tuner, s_mode, *(enum v4l2_tuner_type *)arg);
+	case AUDC_SET_RADIO:
+		return v4l2_subdev_call(sd, tuner, s_radio);
+	case VIDIOC_S_TUNER:
+		return v4l2_subdev_call(sd, tuner, s_tuner, arg);
+	case VIDIOC_G_TUNER:
+		return v4l2_subdev_call(sd, tuner, g_tuner, arg);
+	case VIDIOC_S_STD:
+		return v4l2_subdev_call(sd, tuner, s_std, *(v4l2_std_id *)arg);
+	case VIDIOC_S_FREQUENCY:
+		return v4l2_subdev_call(sd, tuner, s_frequency, arg);
+	case VIDIOC_G_FREQUENCY:
+		return v4l2_subdev_call(sd, tuner, g_frequency, arg);
+	case TUNER_SET_TYPE_ADDR:
+		return v4l2_subdev_call(sd, tuner, s_type_addr, arg);
+	case TUNER_SET_CONFIG:
+		return v4l2_subdev_call(sd, tuner, s_config, arg);
+
+	case VIDIOC_INT_AUDIO_CLOCK_FREQ:
+		return v4l2_subdev_call(sd, audio, s_clock_freq, *(u32 *)arg);
+	case VIDIOC_INT_S_AUDIO_ROUTING:
+		return v4l2_subdev_call(sd, audio, s_routing, arg);
+	case VIDIOC_INT_I2S_CLOCK_FREQ:
+		return v4l2_subdev_call(sd, audio, s_i2s_clock_freq, *(u32 *)arg);
+
+	case VIDIOC_INT_S_VIDEO_ROUTING:
+		return v4l2_subdev_call(sd, video, s_routing, arg);
+	case VIDIOC_INT_S_CRYSTAL_FREQ:
+		return v4l2_subdev_call(sd, video, s_crystal_freq, arg);
+	case VIDIOC_INT_DECODE_VBI_LINE:
+		return v4l2_subdev_call(sd, video, decode_vbi_line, arg);
+	case VIDIOC_INT_S_VBI_DATA:
+		return v4l2_subdev_call(sd, video, s_vbi_data, arg);
+	case VIDIOC_INT_G_VBI_DATA:
+		return v4l2_subdev_call(sd, video, g_vbi_data, arg);
+	case VIDIOC_S_FMT:
+		return v4l2_subdev_call(sd, video, s_fmt, arg);
+	case VIDIOC_G_FMT:
+		return v4l2_subdev_call(sd, video, g_fmt, arg);
+	case VIDIOC_INT_S_STD_OUTPUT:
+		return v4l2_subdev_call(sd, video, s_std_output, *(v4l2_std_id *)arg);
+	case VIDIOC_STREAMON:
+		return v4l2_subdev_call(sd, video, s_stream, 1);
+	case VIDIOC_STREAMOFF:
+		return v4l2_subdev_call(sd, video, s_stream, 0);
+
+	default:
+		return v4l2_subdev_call(sd, core, ioctl, cmd, arg);
+	}
+}
+EXPORT_SYMBOL(v4l2_subdev_command);
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/linux/include/media/v4l2-device.h	Mon Nov 24 22:09:50 2008 +0100
@@ -0,0 +1,124 @@
+/*
+    V4L2 device support header.
+
+    Copyright (C) 2008  Hans Verkuil <hverkuil@xs4all.nl>
+
+    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
+ */
+
+#ifndef _V4L2_DEVICE_H
+#define _V4L2_DEVICE_H
+
+#include <media/v4l2-subdev.h>
+
+/* Each instance of a V4L2 device should create the v4l2_device struct,
+   either stand-alone or embedded in a larger struct.
+
+   It allows easy access to sub-devices (see v4l2-subdev.h) and provides
+   basic V4L2 device-level support.
+
+   Each driver should set a unique driver ID and instance number (usually
+   0 for the first instance and counting upwards from there).
+
+   It is recommended to set the name as follows:
+
+   <drivername> + instance number
+
+   If the driver name ends with a digit, then put a '-' between the driver
+   name and the instance number.
+ */
+
+#define V4L2_DEVICE_NAME_SIZE 32
+
+struct v4l2_device {
+	/* dev->platform_data points to this struct */
+	struct device *dev;
+	/* used to keep track of the registered subdevs */
+	struct list_head subdevs;
+	/* lock this struct; can be used by the driver as well if this
+	   struct is embedded into a larger struct. */
+	struct mutex lock;
+	/* unique device name */
+	char name[V4L2_DEVICE_NAME_SIZE];
+};
+
+/* Initialize v4l2_dev and make dev->platform_data point to v4l2_dev */
+void v4l2_device_register(struct device *dev, struct v4l2_device *v4l2_dev);
+/* Set v4l2_dev->dev->platform_data to NULL and unregister all sub-devices */
+void v4l2_device_unregister(struct v4l2_device *v4l2_dev);
+
+/* Register a subdev with a v4l2 device. While registered the subdev module
+   is marked as in-use. An error is returned if the module is no longer
+   loaded when you attempt to register it. */
+int v4l2_device_register_subdev(struct v4l2_device *dev, struct v4l2_subdev *sd);
+/* Unregister a subdev with a v4l2 device. Can also be called if the subdev
+   wasn't registered. In that case it will do nothing. */
+void v4l2_device_unregister_subdev(struct v4l2_subdev *sd);
+
+/* Iterate over all subdevs. Warn if you iterate over the subdevs without
+   the device struct being locked. The next item is prefetched, so you can
+   delete the current item if necessary. */
+#define v4l2_device_for_each_subdev(sd, dev)				\
+	for (sd = list_entry((dev)->subdevs.next, typeof(*sd), list), 	\
+	     ({ WARN_ON(!mutex_is_locked(&(dev)->lock)); 0; });		\
+		prefetch(sd->list.next), &sd->list != &(dev)->subdevs; 	\
+	     sd = list_entry(sd->list.next, typeof(*sd), list))
+
+/* Call the specified callback for all subdevs matching the condition.
+   Ignore any errors. */
+#define __v4l2_device_call_subdevs(dev, cond, o, f, args...) 		\
+	do { 								\
+		struct v4l2_subdev *sd; 				\
+									\
+		mutex_lock(&(dev)->lock); 				\
+		list_for_each_entry(sd, &(dev)->subdevs, list)   	\
+			if ((cond) && sd->ops->o && sd->ops->o->f) 	\
+				sd->ops->o->f(sd , ##args); 		\
+		mutex_unlock(&(dev)->lock); 				\
+	} while (0)
+
+/* Call the specified callback for all subdevs matching the condition.
+   If the callback returns an error other than 0 or -ENOIOCTLCMD, then
+   return with that error code. */
+#define __v4l2_device_call_subdevs_until_err(dev, cond, o, f, args...)  \
+({ 									\
+	struct v4l2_subdev *sd; 					\
+	int err = 0; 							\
+									\
+	mutex_lock(&(dev)->lock); 					\
+	list_for_each_entry(sd, &(dev)->subdevs, list) { 		\
+		if ((cond) && sd->ops->o && sd->ops->o->f) 		\
+			err = sd->ops->o->f(sd , ##args); 		\
+		if (err && err != -ENOIOCTLCMD)				\
+			break; 						\
+	} 								\
+	mutex_unlock(&(dev)->lock); 					\
+	(err == -ENOIOCTLCMD) ? 0 : err; 				\
+})
+
+/* Call the specified callback for all subdevs matching grp_id (if 0, then
+   match them all). Ignore any errors. */
+#define v4l2_device_call_all(dev, grp_id, o, f, args...) 		\
+	__v4l2_device_call_subdevs(dev, 				\
+			!(grp_id) || sd->grp_id == (grp_id), o, f , ##args)
+
+/* Call the specified callback for all subdevs matching grp_id (if 0, then
+   match them all). If the callback returns an error other than 0 or
+   -ENOIOCTLCMD, then return with that error code. */
+#define v4l2_device_call_until_err(dev, grp_id, o, f, args...) 		\
+	__v4l2_device_call_subdevs_until_err(dev,			\
+		       !(grp_id) || sd->grp_id == (grp_id), o, f , ##args)
+
+#endif
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/linux/include/media/v4l2-subdev.h	Mon Nov 24 22:09:50 2008 +0100
@@ -0,0 +1,188 @@
+/*
+    V4L2 sub-device support header.
+
+    Copyright (C) 2008  Hans Verkuil <hverkuil@xs4all.nl>
+
+    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
+ */
+
+#ifndef _V4L2_SUBDEV_H
+#define _V4L2_SUBDEV_H
+
+#include <media/v4l2-common.h>
+
+struct v4l2_device;
+struct v4l2_subdev;
+struct tuner_setup;
+
+/* Sub-devices are devices that are connected somehow to the main bridge
+   device. These devices are usually audio/video muxers/encoders/decoders or
+   sensors and webcam controllers.
+
+   Usually these devices are controlled through an i2c bus, but other busses
+   may also be used.
+
+   The v4l2_subdev struct provides a way of accessing these devices in a
+   generic manner. Most operations that these sub-devices support fall in
+   a few categories: core ops, audio ops, video ops and tuner ops.
+
+   More categories can be added if needed, although this should remain a
+   limited set (no more than approx. 8 categories).
+
+   Each category has its own set of ops that subdev drivers can implement.
+
+   A subdev driver can leave the pointer to the category ops NULL if
+   it does not implement them (e.g. an audio subdev will generally not
+   implement the video category ops). The exception is the core category:
+   this must always be present.
+
+   These ops are all used internally so it is no problem to change, remove
+   or add ops or move ops from one to another category. Currently these
+   ops are based on the original ioctls, but since ops are not limited to
+   one argument there is room for improvement here once all i2c subdev
+   drivers are converted to use these ops.
+ */
+
+/* Core ops: it is highly recommended to implement at least these ops:
+
+   g_chip_ident
+   log_status
+   g_register
+   s_register
+
+   This provides basic debugging support.
+
+   The ioctl ops is meant for generic ioctl-like commands. Depending on
+   the use-case it might be better to use subdev-specific ops (currently
+   not yet implemented) since ops provide proper type-checking.
+ */
+struct v4l2_subdev_core_ops {
+	int (*g_chip_ident)(struct v4l2_subdev *sd, struct v4l2_chip_ident *chip);
+	int (*log_status)(struct v4l2_subdev *sd);
+	int (*init)(struct v4l2_subdev *sd, u32 val);
+	int (*s_standby)(struct v4l2_subdev *sd, u32 standby);
+	int (*reset)(struct v4l2_subdev *sd, u32 val);
+	int (*s_gpio)(struct v4l2_subdev *sd, u32 val);
+	int (*queryctrl)(struct v4l2_subdev *sd, struct v4l2_queryctrl *qc);
+	int (*g_ctrl)(struct v4l2_subdev *sd, struct v4l2_control *ctrl);
+	int (*s_ctrl)(struct v4l2_subdev *sd, struct v4l2_control *ctrl);
+	int (*querymenu)(struct v4l2_subdev *sd, struct v4l2_querymenu *qm);
+	int (*ioctl)(struct v4l2_subdev *sd, int cmd, void *arg);
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+	int (*g_register)(struct v4l2_subdev *sd, struct v4l2_register *reg);
+	int (*s_register)(struct v4l2_subdev *sd, struct v4l2_register *reg);
+#endif
+};
+
+struct v4l2_subdev_tuner_ops {
+	int (*s_mode)(struct v4l2_subdev *sd, enum v4l2_tuner_type);
+	int (*s_radio)(struct v4l2_subdev *sd);
+	int (*s_frequency)(struct v4l2_subdev *sd, struct v4l2_frequency *freq);
+	int (*g_frequency)(struct v4l2_subdev *sd, struct v4l2_frequency *freq);
+	int (*g_tuner)(struct v4l2_subdev *sd, struct v4l2_tuner *vt);
+	int (*s_tuner)(struct v4l2_subdev *sd, struct v4l2_tuner *vt);
+	int (*s_std)(struct v4l2_subdev *sd, v4l2_std_id norm);
+	int (*s_type_addr)(struct v4l2_subdev *sd, struct tuner_setup *type);
+	int (*s_config)(struct v4l2_subdev *sd, const struct v4l2_priv_tun_config *config);
+};
+
+struct v4l2_subdev_audio_ops {
+	int (*s_clock_freq)(struct v4l2_subdev *sd, u32 freq);
+	int (*s_i2s_clock_freq)(struct v4l2_subdev *sd, u32 freq);
+	int (*s_routing)(struct v4l2_subdev *sd, const struct v4l2_routing *route);
+};
+
+struct v4l2_subdev_video_ops {
+	int (*s_routing)(struct v4l2_subdev *sd, const struct v4l2_routing *route);
+	int (*s_crystal_freq)(struct v4l2_subdev *sd, struct v4l2_crystal_freq *freq);
+	int (*decode_vbi_line)(struct v4l2_subdev *sd, struct v4l2_decode_vbi_line *vbi_line);
+	int (*s_vbi_data)(struct v4l2_subdev *sd, const struct v4l2_sliced_vbi_data *vbi_data);
+	int (*g_vbi_data)(struct v4l2_subdev *sd, struct v4l2_sliced_vbi_data *vbi_data);
+	int (*s_std_output)(struct v4l2_subdev *sd, v4l2_std_id std);
+	int (*s_stream)(struct v4l2_subdev *sd, int enable);
+	int (*s_fmt)(struct v4l2_subdev *sd, struct v4l2_format *fmt);
+	int (*g_fmt)(struct v4l2_subdev *sd, struct v4l2_format *fmt);
+};
+
+struct v4l2_subdev_ops {
+	const struct v4l2_subdev_core_ops  *core;
+	const struct v4l2_subdev_tuner_ops *tuner;
+	const struct v4l2_subdev_audio_ops *audio;
+	const struct v4l2_subdev_video_ops *video;
+};
+
+#define V4L2_SUBDEV_NAME_SIZE 32
+
+/* Each instance of a subdev driver should create this struct, either
+   stand-alone or embedded in a larger struct.
+ */
+struct v4l2_subdev {
+	struct list_head list;
+	struct module *owner;
+	struct v4l2_device *dev;
+	const struct v4l2_subdev_ops *ops;
+	/* name must be unique */
+	char name[V4L2_SUBDEV_NAME_SIZE];
+	/* can be used to group similar subdevs, value is driver-specific */
+	u32 grp_id;
+	/* pointer to private data */
+	void *priv;
+};
+
+static inline void v4l2_set_subdevdata(struct v4l2_subdev *sd, void *p)
+{
+	sd->priv = p;
+}
+
+static inline void *v4l2_get_subdevdata(const struct v4l2_subdev *sd)
+{
+	return sd->priv;
+}
+
+/* Convert an ioctl-type command to the proper v4l2_subdev_ops function call.
+   This is used by subdev modules that can be called by both old-style ioctl
+   commands and through the v4l2_subdev_ops.
+
+   The ioctl API of the subdev driver can call this function to call the
+   right ops based on the ioctl cmd and arg.
+
+   Once all subdev drivers have been converted and all drivers no longer
+   use the ioctl interface, then this function can be removed.
+ */
+int v4l2_subdev_command(struct v4l2_subdev *sd, unsigned cmd, void *arg);
+
+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);
+	sd->ops = ops;
+	sd->dev = NULL;
+	sd->name[0] = '\0';
+	sd->grp_id = 0;
+	sd->priv = NULL;
+}
+
+/* Call an ops of a v4l2_subdev, doing the right checks against
+   NULL pointers.
+
+   Example: err = v4l2_subdev_call(sd, core, g_chip_ident, &chip);
+ */
+#define v4l2_subdev_call(sd, o, f, args...)				\
+	(!(sd) ? -ENODEV : (((sd) && (sd)->ops->o && (sd)->ops->o->f) ?	\
+		(sd)->ops->o->f((sd) , ##args) : -ENOIOCTLCMD))
+
+#endif

Re: v4l2_device/v4l2_subdev: please review (PATCH 1/3)

[Laurent Pinchart]

Hi Hans,

On Tuesday 25 November 2008, Hans Verkuil wrote:
> As requested, the patches as separate posts for review.
>
> 	Hans
>
> # HG changeset patch
> # User Hans Verkuil <hverkuil@xs4all.nl>
> # Date 1227560990 -3600
> # Node ID d9ec70c0b0c55e18813f91218c6da6212ca9b7e6
> # Parent b63737bf9eef1ff8494cb7fbc2e818e6aff7a34f
> v4l2: add v4l2_device and v4l2_subdev structs to the v4l2 framework.
>
> From: Hans Verkuil <hverkuil@xs4all.nl>
>
> Start implementing a proper v4l2 framework as discussed during the
> Linux Plumbers Conference 2008.
>
> Introduces v4l2_device (for device instances) and v4l2_subdev (representing
> sub-device instances).
>
> Priority: normal
>
> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>

Comments inlined. I've reviewed the general approach only, so there might be 
some small issues at the code level that I haven't noticed.

Would you mind holding the push request until we're done discussing the points 
I mention throughout this e-mail ?

> --- a/linux/drivers/media/video/Makefile	Mon Nov 24 10:51:20 2008 -0200
> +++ b/linux/drivers/media/video/Makefile	Mon Nov 24 22:09:50 2008 +0100
> @@ -8,7 +8,7 @@ msp3400-objs	:=	msp3400-driver.o msp3400
>
>  stkwebcam-objs	:=	stk-webcam.o stk-sensor.o
>
> -videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o
> +videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-subdev.o
>
>  obj-$(CONFIG_VIDEO_DEV) += videodev.o v4l2-compat-ioctl32.o
> v4l2-int-device.o
>
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/linux/Documentation/video4linux/v4l2-framework.txt	Mon Nov 24
> 22:09:50 2008 +0100 @@ -0,0 +1,360 @@
> +Overview of the V4L2 driver framework
> +=====================================
> +
> +This text documents the various structures provided by the V4L2 framework
> and +their relationships.
> +
> +
> +Introduction
> +------------
> +
> +The V4L2 drivers tend to be very complex due to the complexity of the
> +hardware: most devices have multiple ICs, export multiple device nodes in
> +/dev, and create also non-V4L2 devices such as DVB, ALSA, FB, I2C and
> input +(IR) devices.
> +
> +Especially the fact that V4L2 drivers have to setup supporting ICs to
> +do audio/video muxing/encoding/decoding makes it more complex than most.
> +Usually these ICs are connected to the main bridge driver through one or
> +more I2C busses, but other busses can also be used. Such devices are
> +called 'sub-devices'.

Do you know of other busses being used in (Linux supported) real video 
hardware, or is it currently theoretical only ?

> +For a long time the framework was limited to the video_device struct for
> +creating V4L device nodes and video_buf for handling the video buffers
> +(note that this document does not discuss the video_buf framework).
> +
> +This meant that all drivers had to do the setup of device instances and
> +connecting to sub-devices themselves. Some of this is quite complicated
> +to do right and many drivers never did do it correctly.
> +
> +There is also a lot of common code that could never be refactored due to
> +the lack of a framework.
> +
> +So this framework sets up the basic building blocks that all drivers
> +need and this same framework should make it much easier to refactor
> +common code into utility functions shared by all drivers.
> +
> +
> +Structure of a driver
> +---------------------
> +
> +All drivers have the following structure:
> +
> +1) A struct for each device instance containing the device state.
> +
> +2) A way of initializing and commanding sub-devices.

This only applies to drivers handling "composite devices" (systems including 
sub-devices). Let's make sure the proposed framework doesn't make "simple 
devices" more complex to handle that they are now.

> +3) Creating V4L2 device nodes (/dev/videoX, /dev/vbiX, /dev/radioX and
> +   /dev/vtxX) and keeping track of device-node specific data.
> +
> +4) Filehandle-specific structs containing per-filehandle data.
> +
> +This is a rough schematic of how it all relates:
> +
> +    device instances
> +      |
> +      +-sub-device instances
> +      |
> +      \-V4L2 device nodes
> +	  |
> +	  \-filehandle instances
> +
> +
> +Structure of the framework
> +--------------------------
> +
> +The framework closely resembles the driver structure: it has a v4l2_device
> +struct for the device instance data, a v4l2_subdev struct to refer to
> +sub-device instances, the video_device struct stores V4L2 device node data
> +and a v4l2_fh struct keeps track of filehandle instances (TODO: not yet
> +implemented).
> +
> +
> +struct v4l2_device
> +------------------
> +
> +Each device instance is represented by a struct v4l2_device
> (v4l2-device.h). +Very simple devices can just allocate this struct, but
> most of the time you +would embed this struct inside a larger struct.
> +
> +You must register the device instance:
> +
> +	v4l2_device_register(struct device *dev, struct v4l2_device *v4l2_dev);
> +
> +Registration will initialize the v4l2_device struct and link
> dev->platform_data +to v4l2_dev.

That's an abuse of platform_data, I don't think it was ever meant that way.

> Registration will also set v4l2_dev->name 
> to a value derived from +dev (driver name followed by the bus_id, to be
> precise). You may change the +name after registration if you want.
> +
> +You unregister with:
> +
> +	v4l2_device_unregister(struct v4l2_device *v4l2_dev);
> +
> +Unregistering will also automatically unregister all subdevs from the
> device. +
> +Sometimes you need to iterate over all devices registered by a specific
> +driver. This is usually the case if multiple device drivers use the same
> +hardware. E.g. the ivtvfb driver is a framebuffer driver that uses the
> ivtv +hardware. The same is true for alsa drivers for example.
> +
> +You can iterate over all registered devices as follows:
> +
> +static int callback(struct device *dev, void *p)
> +{
> +	struct v4l2_device *v4l2_dev = dev->platform_data;
> +
> +	/* test if this device was inited */
> +	if (v4l2_dev == NULL)
> +		return 0;
> +	...
> +	return 0;
> +}
> +
> +int iterate(void *p)
> +{
> +	struct device_driver *drv;
> +	int err;
> +
> +	/* Find driver 'ivtv' on the PCI bus.
> +	   pci_bus_type is a global. For USB busses use usb_bus_type. */
> +	drv = driver_find("ivtv", &pci_bus_type);
> +	/* iterate over all ivtv device instances */
> +	err = driver_for_each_device(drv, NULL, p, callback);
> +	put_driver(drv);
> +	return err;
> +}

I'm not sure to really see what use cases you're talking about. The above code 
looks good, but iterating over device bound to specific drivers should be 
done with care as it might be the sign of badly designed code. Every new 
instance of the above code should be reviewed with care.

> +Sometimes you need to keep a running counter of the device instance. This
> is +commonly used to map a device instance to an index of a module option
> array. +
> +The recommended approach is as follows:
> +
> +static atomic_t drv_instance = ATOMIC_INIT(0);
> +
> +static int __devinit drv_probe(struct pci_dev *dev,
> +				const struct pci_device_id *pci_id)
> +{
> +	...
> +	state->instance = atomic_inc_return(&drv_instance) - 1;
> +}
> +
> +
> +struct v4l2_subdev
> +------------------
> +
> +Many drivers need to communicate with sub-devices. These devices can do
> all +sort of tasks, but most commonly they handle audio and/or video
> muxing, +encoding or decoding. For webcams common sub-devices are sensors
> and camera +controllers.
> +
> +Usually these are I2C devices, but not necessarily. In order to provide
> the +driver with a consistent interface to these sub-devices the
> v4l2_subdev struct +(v4l2-subdev.h) was created.
> +
> +Each sub-device driver must have a v4l2_subdev struct. This struct can be
> +stand-alone for simple sub-devices or it might be embedded in a larger
> struct +if more state information needs to be stored. Usually there is a
> low-level +device struct (e.g. i2c_client) that contains the device data as
> setup +by the kernel. It is recommended to store that pointer in the
> private +data of v4l2_subdev using v4l2_set_subdevdata(). That makes it
> easy to go +from a v4l2_subdev to the actual low-level bus-specific device
> data. +
> +You also need a way to go from the low-level struct to v4l2_subdev. For
> the +common i2c_client struct the i2c_set_clientdata() call is used to
> store a +v4l2_subdev pointer, for other busses you may have to use other
> methods. +
> +From the bridge driver perspective you load the sub-device module and
> somehow +obtain the v4l2_subdev pointer. For i2c devices this is easy: you
> call +i2c_get_clientdata(). For other busses something similar needs to be
> done. +Helper functions exists for sub-devices on an I2C bus that do most
> of this +tricky work for you.
> +
> +Each v4l2_subdev contains function pointers that sub-device drivers can
> +implement (or leave NULL if it is not applicable). Since sub-devices can
> do +so many different things and you do not want to end up with a huge ops
> struct +of which only a handful of ops are commonly implemented, the
> function pointers +are sorted according to category and each category has
> its own ops struct. +

I like that.

> +The top-level ops struct contains pointers to the category ops structs,
> which +may be NULL if the subdev driver does not support anything from that
> category. +
> +It looks like this:
> +
> +struct v4l2_subdev_core_ops {
> +	int (*g_chip_ident)(struct v4l2_subdev *sd, struct v4l2_chip_ident
> *chip); +	int (*log_status)(struct v4l2_subdev *sd);
> +	int (*init)(struct v4l2_subdev *sd, u32 val);
> +	...
> +};
> +
> +struct v4l2_subdev_tuner_ops {
> +	...
> +};
> +
> +struct v4l2_subdev_audio_ops {
> +	...
> +};
> +
> +struct v4l2_subdev_video_ops {
> +	...
> +};
> +
> +struct v4l2_subdev_ops {
> +	const struct v4l2_subdev_core_ops  *core;
> +	const struct v4l2_subdev_tuner_ops *tuner;
> +	const struct v4l2_subdev_audio_ops *audio;
> +	const struct v4l2_subdev_video_ops *video;
> +};
> +
> +The core ops are common to all subdevs, the other categories are
> implemented +depending on the sub-device. E.g. a video device is unlikely
> to support the +audio ops and vice versa.
> +
> +This setup limits the number of function pointers while still making it
> easy +to add new ops and categories.
> +
> +A sub-device driver initializes the v4l2_subdev struct using:
> +
> +	v4l2_subdev_init(subdev, &ops);
> +
> +Afterwards you need to initialize subdev->name with a unique name and set
> the +module owner. This is done for you if you use the i2c helper
> functions. +
> +A device (bridge) driver needs to register the v4l2_subdev with the
> +v4l2_device:
> +
> +	int err = v4l2_device_register_subdev(device, subdev);
> +
> +This can fail if the subdev module disappeared before it could be
> registered. +After this function was called successfully the subdev->dev
> field points to +the v4l2_device.
> +
> +You can unregister a sub-device using:
> +
> +	v4l2_device_unregister_subdev(subdev);
> +
> +Afterwards the subdev module can be unloaded and subdev->dev == NULL.
> +
> +You can call an ops function either directly:
> +
> +	err = subdev->ops->core->g_chip_ident(subdev, &chip);
> +
> +but it is better and easier to use this macro:
> +
> +	err = v4l2_subdev_call(subdev, core, g_chip_ident, &chip);
> +
> +The macro will to the right NULL pointer checks and returns -ENODEV if
> subdev +is NULL,

This should probably be checked when registering the sub-device instead of at 
all calls to sub-device operations.

> -ENOIOCTLCMD if either subdev->core or 
> subdev->core->g_chip_ident is +NULL, or the actual result of the
> subdev->ops->core->g_chip_ident ops. +
> +It is also possible to call all or a subset of the sub-devices:
> +
> +	v4l2_device_call_all(dev, 0, core, g_chip_ident, &chip);
> +
> +Any subdev that does not support this ops is skipped and error results are
> +ignored. If you want to check for errors use this:
> +
> +	err = v4l2_device_call_until_err(dev, 0, core, g_chip_ident, &chip);
> +
> +Any error except -ENOIOCTLCMD will exit the loop with that error. If no
> +errors (except -ENOIOCTLCMD) occured, then 0 is returned.
> +
> +The second argument to both calls is a group ID. If 0, then all subdevs
> are +called. If non-zero, then only those whose group ID match that value
> will +be called. Before a bridge driver registers a subdev it can set
> subdev->grp_id +to whatever value it wants (it's 0 by default). This value
> is owned by the +bridge driver and the sub-device driver will never modify
> or use it. +
> +The group ID gives the bridge driver more control how callbacks are
> called. +For example, there may be multiple audio chips on a board, each
> capable of +changing the volume. But usually only one will actually be used
> when the +user want to change the volume. You can set the group ID for that
> subdev to +e.g. AUDIO_CONTROLLER and specify that as the group ID value
> when calling +v4l2_device_call_all(). That ensures that it will only go to
> the subdev +that needs it.

This is interesting as well.

> +The advantage of using v4l2_subdev is that it is a generic struct and does
> +not contain any knowledge about the underlying hardware. So a driver might
> +contain several subdevs that use an I2C bus, but also a subdev that is
> +controlled through GPIO pins. This distinction is only relevant when
> setting +up the device, but once the subdev is registered it is completely
> transparent. +

The bridge driver won't have to care about how sub-devices are accessed, but 
sub-devices drivers will have to be written specifically for the V4L2 
subsystem. Do we have I2C devices shared between V4L(2) and DVB (and possible 
other subsystems) ? How would those be handled ? If your goal is to share 
v4l2_subdev drivers between V4L(2) and DVB I don't think the v4l2_ prefix is 
right.

> +I2C sub-device drivers
> +----------------------
> +
> +Since these drivers are so common, special helper functions are available
> to +ease the use of these drivers (v4l2-common.h).
> +
> +The recommended method of adding v4l2_subdev support to an I2C driver is
> to +embed the v4l2_subdev struct into the state struct that is created for
> each +I2C device instance. Very simple devices have no state struct and in
> that case +you can just create a v4l2_subdev directly.

The only I2C video-related chips I've dealt with were the ones used by 
DC10/DC10+ boards and those have no state struct. What is the state struct in 
your description ? Is it the driver-specific data allocated when the device 
is probed ?

> +Initialize the v4l2_subdev struct as follows:
> +
> +	v4l2_i2c_subdev_init(&state->sd, client, subdev_ops);
> +
> +This function will fill in all the fields of v4l2_subdev and ensure that
> the +v4l2_subdev and i2c_client both point to one another.
> +
> +You should also add a helper inline function to go from a v4l2_subdev
> pointer +to the state struct:
> +
> +static inline struct subdev_state *to_state(struct v4l2_subdev *sd)
> +{
> +	return container_of(sd, struct subdev_state, sd);
> +}

<chipname>_state might be a better name, it would avoid namespace clashes (I'm 
aware the state structure is supposed to be private to the I2C device driver, 
but we never know what might happen).

> +Use this to go from the v4l2_subdev struct to the i2c_client struct:
> +
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +
> +And this to go from an i2c_client to a v4l2_subdev struct:
> +
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +
> +Finally you need to make a command function to make driver->command()
> +call the right subdev_ops functions:
> +
> +static int subdev_command(struct i2c_client *client, unsigned cmd, void
> *arg) +{
> +	return v4l2_subdev_command(i2c_get_clientdata(client), cmd, arg);
> +}
> +
> +If driver->command is never used then you can leave this out. Eventually
> the +driver->command usage should be removed from v4l.

Should it then be added to the list of features scheduled for removal ?

> +Make sure to call v4l2_device_unregister_subdev(sd) when the remove()
> callback +is called. This will unregister the sub-device from the bridge
> driver. It is +safe to call this even if the sub-device was never
> registered.
> +
> +
> +The bridge driver also has some helper functions it can use:
> +
> +struct v4l2_subdev *sd = v4l2_i2c_new_subdev(adapter, "module_foo",
> "chipid", 0x36); +
> +This loads the given module (can be NULL if no module needs to be loaded)
> and +calls i2c_new_device() with the given i2c_adapter and chip/address
> arguments. +If all goes well, then it registers the subdev with the
> v4l2_device. It gets +the v4l2_device by calling i2c_get_adapdata(adapter),
> so you should make sure +that adapdata is set to v4l2_device when you setup
> the i2c_adapter in your +driver.
> +
> +You can also use v4l2_i2c_new_probed_subdev() which is very similar to
> +v4l2_i2c_new_subdev(), except that it has an array of possible I2C
> addresses +that it should probe. Internally it calls
> i2c_new_probed_device(). +
> +Both functions return NULL if something went wrong.
> +
> +
> +struct video_device
> +-------------------
> +
> +TODO: document.

Do you plan to change the video_device structure ?

> +
> +struct v4l2_fh
> +--------------
> +
> +TODO: document.

I can't find that structure anywhere in your patches. Shouldn't it be removed 
from the document and added back when you introduce it ?

> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/linux/drivers/media/video/v4l2-device.c	Mon Nov 24 22:09:50 2008
> +0100 @@ -0,0 +1,80 @@
> +/*
> +    V4L2 device support.
> +
> +    Copyright (C) 2008  Hans Verkuil <hverkuil@xs4all.nl>
> +
> +    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/i2c.h>
> +#include <linux/videodev2.h>
> +#include <media/v4l2-device.h>
> +
> +void v4l2_device_register(struct device *dev, struct v4l2_device
> *v4l2_dev) +{
> +	BUG_ON(!dev || !v4l2_dev || dev->platform_data);
> +	INIT_LIST_HEAD(&v4l2_dev->subdevs);
> +	mutex_init(&v4l2_dev->lock);
> +	v4l2_dev->dev = dev;
> +	snprintf(v4l2_dev->name, sizeof(v4l2_dev->name), "%s %s",
> +			dev->driver->name, dev->bus_id);
> +	dev->platform_data = v4l2_dev;
> +}
> +EXPORT_SYMBOL(v4l2_device_register);
> +
> +void v4l2_device_unregister(struct v4l2_device *v4l2_dev)
> +{
> +	struct v4l2_subdev *sd, *next;
> +
> +	BUG_ON(!v4l2_dev || !v4l2_dev->dev || !v4l2_dev->dev->platform_data);
> +	v4l2_dev->dev->platform_data = NULL;
> +	/* unregister subdevs */
> +	list_for_each_entry_safe(sd, next, &v4l2_dev->subdevs, list)
> +		v4l2_device_unregister_subdev(sd);
> +
> +	v4l2_dev->dev = NULL;
> +}
> +EXPORT_SYMBOL(v4l2_device_unregister);
> +
> +int v4l2_device_register_subdev(struct v4l2_device *dev, struct
> v4l2_subdev *sd) +{
> +	/* Check that sd->dev is not set and that the subdev's name is
> +	   filled in. */
> +	BUG_ON(!dev || !sd || sd->dev || !sd->name[0]);
> +	if (!try_module_get(sd->owner))
> +		return -ENODEV;
> +	sd->dev = dev;
> +	mutex_lock(&dev->lock);
> +	list_add_tail(&sd->list, &dev->subdevs);
> +	mutex_unlock(&dev->lock);
> +	return 0;
> +}
> +EXPORT_SYMBOL(v4l2_device_register_subdev);
> +
> +void v4l2_device_unregister_subdev(struct v4l2_subdev *sd)
> +{
> +	BUG_ON(!sd);
> +	/* return if it isn't registered */
> +	if (sd->dev == NULL)
> +		return;
> +	mutex_lock(&sd->dev->lock);
> +	list_del(&sd->list);
> +	mutex_unlock(&sd->dev->lock);
> +	sd->dev = NULL;
> +	module_put(sd->owner);
> +}
> +EXPORT_SYMBOL(v4l2_device_unregister_subdev);
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/linux/drivers/media/video/v4l2-subdev.c	Mon Nov 24 22:09:50 2008
> +0100 @@ -0,0 +1,108 @@
> +/*
> +    V4L2 sub-device support.
> +
> +    Copyright (C) 2008  Hans Verkuil <hverkuil@xs4all.nl>
> +
> +    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/i2c.h>
> +#include <linux/videodev2.h>
> +#include <media/v4l2-subdev.h>
> +
> +int v4l2_subdev_command(struct v4l2_subdev *sd, unsigned cmd, void *arg)
> +{
> +	switch (cmd) {
> +	case VIDIOC_QUERYCTRL:
> +		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_QUERYMENU:
> +		return v4l2_subdev_call(sd, core, queryctrl, arg);
> +	case VIDIOC_LOG_STATUS:
> +		return v4l2_subdev_call(sd, core, log_status);
> +	case VIDIOC_G_CHIP_IDENT:
> +		return v4l2_subdev_call(sd, core, g_chip_ident, arg);
> +	case VIDIOC_INT_S_STANDBY:
> +		return v4l2_subdev_call(sd, core, s_standby, *(u32 *)arg);
> +	case VIDIOC_INT_RESET:
> +		return v4l2_subdev_call(sd, core, reset, *(u32 *)arg);
> +	case VIDIOC_INT_S_GPIO:
> +		return v4l2_subdev_call(sd, core, s_gpio, *(u32 *)arg);
> +	case VIDIOC_INT_INIT:
> +		return v4l2_subdev_call(sd, core, init, *(u32 *)arg);
> +#ifdef CONFIG_VIDEO_ADV_DEBUG
> +	case VIDIOC_DBG_G_REGISTER:
> +		return v4l2_subdev_call(sd, core, g_register, arg);
> +	case VIDIOC_DBG_S_REGISTER:
> +		return v4l2_subdev_call(sd, core, s_register, arg);
> +#endif
> +
> +	case VIDIOC_INT_S_TUNER_MODE:
> +		return v4l2_subdev_call(sd, tuner, s_mode, *(enum v4l2_tuner_type
> *)arg); +	case AUDC_SET_RADIO:
> +		return v4l2_subdev_call(sd, tuner, s_radio);
> +	case VIDIOC_S_TUNER:
> +		return v4l2_subdev_call(sd, tuner, s_tuner, arg);
> +	case VIDIOC_G_TUNER:
> +		return v4l2_subdev_call(sd, tuner, g_tuner, arg);
> +	case VIDIOC_S_STD:
> +		return v4l2_subdev_call(sd, tuner, s_std, *(v4l2_std_id *)arg);
> +	case VIDIOC_S_FREQUENCY:
> +		return v4l2_subdev_call(sd, tuner, s_frequency, arg);
> +	case VIDIOC_G_FREQUENCY:
> +		return v4l2_subdev_call(sd, tuner, g_frequency, arg);
> +	case TUNER_SET_TYPE_ADDR:
> +		return v4l2_subdev_call(sd, tuner, s_type_addr, arg);
> +	case TUNER_SET_CONFIG:
> +		return v4l2_subdev_call(sd, tuner, s_config, arg);
> +
> +	case VIDIOC_INT_AUDIO_CLOCK_FREQ:
> +		return v4l2_subdev_call(sd, audio, s_clock_freq, *(u32 *)arg);
> +	case VIDIOC_INT_S_AUDIO_ROUTING:
> +		return v4l2_subdev_call(sd, audio, s_routing, arg);
> +	case VIDIOC_INT_I2S_CLOCK_FREQ:
> +		return v4l2_subdev_call(sd, audio, s_i2s_clock_freq, *(u32 *)arg);
> +
> +	case VIDIOC_INT_S_VIDEO_ROUTING:
> +		return v4l2_subdev_call(sd, video, s_routing, arg);
> +	case VIDIOC_INT_S_CRYSTAL_FREQ:
> +		return v4l2_subdev_call(sd, video, s_crystal_freq, arg);
> +	case VIDIOC_INT_DECODE_VBI_LINE:
> +		return v4l2_subdev_call(sd, video, decode_vbi_line, arg);
> +	case VIDIOC_INT_S_VBI_DATA:
> +		return v4l2_subdev_call(sd, video, s_vbi_data, arg);
> +	case VIDIOC_INT_G_VBI_DATA:
> +		return v4l2_subdev_call(sd, video, g_vbi_data, arg);
> +	case VIDIOC_S_FMT:
> +		return v4l2_subdev_call(sd, video, s_fmt, arg);
> +	case VIDIOC_G_FMT:
> +		return v4l2_subdev_call(sd, video, g_fmt, arg);
> +	case VIDIOC_INT_S_STD_OUTPUT:
> +		return v4l2_subdev_call(sd, video, s_std_output, *(v4l2_std_id *)arg);
> +	case VIDIOC_STREAMON:
> +		return v4l2_subdev_call(sd, video, s_stream, 1);
> +	case VIDIOC_STREAMOFF:
> +		return v4l2_subdev_call(sd, video, s_stream, 0);
> +
> +	default:
> +		return v4l2_subdev_call(sd, core, ioctl, cmd, arg);
> +	}
> +}
> +EXPORT_SYMBOL(v4l2_subdev_command);
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/linux/include/media/v4l2-device.h	Mon Nov 24 22:09:50 2008 +0100
> @@ -0,0 +1,124 @@
> +/*
> +    V4L2 device support header.
> +
> +    Copyright (C) 2008  Hans Verkuil <hverkuil@xs4all.nl>
> +
> +    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 + */
> +
> +#ifndef _V4L2_DEVICE_H
> +#define _V4L2_DEVICE_H
> +
> +#include <media/v4l2-subdev.h>
> +
> +/* Each instance of a V4L2 device should create the v4l2_device struct,
> +   either stand-alone or embedded in a larger struct.

Is it required for all V4L2 devices, or only those that use subdevices ? 
What's the added value of v4l2_device for "simple" devices such as UVC 
webcams ?

> +   It allows easy access to sub-devices (see v4l2-subdev.h) and provides
> +   basic V4L2 device-level support.
> +
> +   Each driver should set a unique driver ID and instance number (usually
> +   0 for the first instance and counting upwards from there).
> +
> +   It is recommended to set the name as follows:
> +
> +   <drivername> + instance number
> +
> +   If the driver name ends with a digit, then put a '-' between the driver
> +   name and the instance number.

The name field is automatically filled by v4l2_device_register. I think the 
comment is outdated.

> + */
> +
> +#define V4L2_DEVICE_NAME_SIZE 32

The device name is made of the driver name and the bus id. The driver name has 
no upper bound, but the bus id is limited to BUS_ID_SIZE (currently 20) 
bytes. Shouldn't V4L2_DEVICE_NAME_SIZE be defined as (BUS_ID_SIZE + 
reasonable constant for the driver name) ?

> +struct v4l2_device {
> +	/* dev->platform_data points to this struct */
> +	struct device *dev;
> +	/* used to keep track of the registered subdevs */
> +	struct list_head subdevs;
> +	/* lock this struct; can be used by the driver as well if this
> +	   struct is embedded into a larger struct. */
> +	struct mutex lock;
> +	/* unique device name */
> +	char name[V4L2_DEVICE_NAME_SIZE];
> +};
> +
> +/* Initialize v4l2_dev and make dev->platform_data point to v4l2_dev */
> +void v4l2_device_register(struct device *dev, struct v4l2_device
> *v4l2_dev); +/* Set v4l2_dev->dev->platform_data to NULL and unregister all
> sub-devices */ +void v4l2_device_unregister(struct v4l2_device *v4l2_dev);
> +
> +/* Register a subdev with a v4l2 device. While registered the subdev
> module +   is marked as in-use. An error is returned if the module is no
> longer +   loaded when you attempt to register it. */
> +int v4l2_device_register_subdev(struct v4l2_device *dev, struct
> v4l2_subdev *sd); +/* Unregister a subdev with a v4l2 device. Can also be
> called if the subdev +   wasn't registered. In that case it will do
> nothing. */
> +void v4l2_device_unregister_subdev(struct v4l2_subdev *sd);
> +
> +/* Iterate over all subdevs. Warn if you iterate over the subdevs without
> +   the device struct being locked. The next item is prefetched, so you can
> +   delete the current item if necessary. */
> +#define v4l2_device_for_each_subdev(sd, dev)				\
> +	for (sd = list_entry((dev)->subdevs.next, typeof(*sd), list), 	\
> +	     ({ WARN_ON(!mutex_is_locked(&(dev)->lock)); 0; });		\
> +		prefetch(sd->list.next), &sd->list != &(dev)->subdevs; 	\
> +	     sd = list_entry(sd->list.next, typeof(*sd), list))
> +
> +/* Call the specified callback for all subdevs matching the condition.
> +   Ignore any errors. */
> +#define __v4l2_device_call_subdevs(dev, cond, o, f, args...) 		\
> +	do { 								\
> +		struct v4l2_subdev *sd; 				\
> +									\
> +		mutex_lock(&(dev)->lock); 				\
> +		list_for_each_entry(sd, &(dev)->subdevs, list)   	\
> +			if ((cond) && sd->ops->o && sd->ops->o->f) 	\
> +				sd->ops->o->f(sd , ##args); 		\
> +		mutex_unlock(&(dev)->lock); 				\
> +	} while (0)
> +
> +/* Call the specified callback for all subdevs matching the condition.
> +   If the callback returns an error other than 0 or -ENOIOCTLCMD, then
> +   return with that error code. */
> +#define __v4l2_device_call_subdevs_until_err(dev, cond, o, f, args...)  \
> +({ 									\
> +	struct v4l2_subdev *sd; 					\
> +	int err = 0; 							\
> +									\
> +	mutex_lock(&(dev)->lock); 					\
> +	list_for_each_entry(sd, &(dev)->subdevs, list) { 		\
> +		if ((cond) && sd->ops->o && sd->ops->o->f) 		\
> +			err = sd->ops->o->f(sd , ##args); 		\
> +		if (err && err != -ENOIOCTLCMD)				\
> +			break; 						\
> +	} 								\
> +	mutex_unlock(&(dev)->lock); 					\
> +	(err == -ENOIOCTLCMD) ? 0 : err; 				\
> +})
> +
> +/* Call the specified callback for all subdevs matching grp_id (if 0, then
> +   match them all). Ignore any errors. */
> +#define v4l2_device_call_all(dev, grp_id, o, f, args...) 		\
> +	__v4l2_device_call_subdevs(dev, 				\
> +			!(grp_id) || sd->grp_id == (grp_id), o, f , ##args)
> +
> +/* Call the specified callback for all subdevs matching grp_id (if 0, then
> +   match them all). If the callback returns an error other than 0 or
> +   -ENOIOCTLCMD, then return with that error code. */
> +#define v4l2_device_call_until_err(dev, grp_id, o, f, args...) 		\
> +	__v4l2_device_call_subdevs_until_err(dev,			\
> +		       !(grp_id) || sd->grp_id == (grp_id), o, f , ##args)
> +
> +#endif
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/linux/include/media/v4l2-subdev.h	Mon Nov 24 22:09:50 2008 +0100
> @@ -0,0 +1,188 @@
> +/*
> +    V4L2 sub-device support header.
> +
> +    Copyright (C) 2008  Hans Verkuil <hverkuil@xs4all.nl>
> +
> +    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 + */
> +
> +#ifndef _V4L2_SUBDEV_H
> +#define _V4L2_SUBDEV_H
> +
> +#include <media/v4l2-common.h>
> +
> +struct v4l2_device;
> +struct v4l2_subdev;
> +struct tuner_setup;
> +
> +/* Sub-devices are devices that are connected somehow to the main bridge
> +   device. These devices are usually audio/video muxers/encoders/decoders
> or +   sensors and webcam controllers.
> +
> +   Usually these devices are controlled through an i2c bus, but other
> busses +   may also be used.
> +
> +   The v4l2_subdev struct provides a way of accessing these devices in a
> +   generic manner. Most operations that these sub-devices support fall in
> +   a few categories: core ops, audio ops, video ops and tuner ops.
> +
> +   More categories can be added if needed, although this should remain a
> +   limited set (no more than approx. 8 categories).
> +
> +   Each category has its own set of ops that subdev drivers can implement.
> +
> +   A subdev driver can leave the pointer to the category ops NULL if
> +   it does not implement them (e.g. an audio subdev will generally not
> +   implement the video category ops). The exception is the core category:
> +   this must always be present.
> +
> +   These ops are all used internally so it is no problem to change, remove
> +   or add ops or move ops from one to another category. Currently these
> +   ops are based on the original ioctls, but since ops are not limited to
> +   one argument there is room for improvement here once all i2c subdev
> +   drivers are converted to use these ops.
> + */
> +
> +/* Core ops: it is highly recommended to implement at least these ops:
> +
> +   g_chip_ident
> +   log_status
> +   g_register
> +   s_register
> +
> +   This provides basic debugging support.
> +
> +   The ioctl ops is meant for generic ioctl-like commands. Depending on
> +   the use-case it might be better to use subdev-specific ops (currently
> +   not yet implemented) since ops provide proper type-checking.
> + */
> +struct v4l2_subdev_core_ops {
> +	int (*g_chip_ident)(struct v4l2_subdev *sd, struct v4l2_chip_ident
> *chip); +	int (*log_status)(struct v4l2_subdev *sd);
> +	int (*init)(struct v4l2_subdev *sd, u32 val);
> +	int (*s_standby)(struct v4l2_subdev *sd, u32 standby);
> +	int (*reset)(struct v4l2_subdev *sd, u32 val);
> +	int (*s_gpio)(struct v4l2_subdev *sd, u32 val);
> +	int (*queryctrl)(struct v4l2_subdev *sd, struct v4l2_queryctrl *qc);
> +	int (*g_ctrl)(struct v4l2_subdev *sd, struct v4l2_control *ctrl);
> +	int (*s_ctrl)(struct v4l2_subdev *sd, struct v4l2_control *ctrl);
> +	int (*querymenu)(struct v4l2_subdev *sd, struct v4l2_querymenu *qm);
> +	int (*ioctl)(struct v4l2_subdev *sd, int cmd, void *arg);
> +#ifdef CONFIG_VIDEO_ADV_DEBUG
> +	int (*g_register)(struct v4l2_subdev *sd, struct v4l2_register *reg);
> +	int (*s_register)(struct v4l2_subdev *sd, struct v4l2_register *reg);
> +#endif
> +};
> +
> +struct v4l2_subdev_tuner_ops {
> +	int (*s_mode)(struct v4l2_subdev *sd, enum v4l2_tuner_type);
> +	int (*s_radio)(struct v4l2_subdev *sd);
> +	int (*s_frequency)(struct v4l2_subdev *sd, struct v4l2_frequency *freq);
> +	int (*g_frequency)(struct v4l2_subdev *sd, struct v4l2_frequency *freq);
> +	int (*g_tuner)(struct v4l2_subdev *sd, struct v4l2_tuner *vt);
> +	int (*s_tuner)(struct v4l2_subdev *sd, struct v4l2_tuner *vt);
> +	int (*s_std)(struct v4l2_subdev *sd, v4l2_std_id norm);
> +	int (*s_type_addr)(struct v4l2_subdev *sd, struct tuner_setup *type);
> +	int (*s_config)(struct v4l2_subdev *sd, const struct v4l2_priv_tun_config
> *config); +};
> +
> +struct v4l2_subdev_audio_ops {
> +	int (*s_clock_freq)(struct v4l2_subdev *sd, u32 freq);
> +	int (*s_i2s_clock_freq)(struct v4l2_subdev *sd, u32 freq);
> +	int (*s_routing)(struct v4l2_subdev *sd, const struct v4l2_routing
> *route); +};
> +
> +struct v4l2_subdev_video_ops {
> +	int (*s_routing)(struct v4l2_subdev *sd, const struct v4l2_routing
> *route); +	int (*s_crystal_freq)(struct v4l2_subdev *sd, struct
> v4l2_crystal_freq *freq); +	int (*decode_vbi_line)(struct v4l2_subdev *sd,
> struct v4l2_decode_vbi_line *vbi_line); +	int (*s_vbi_data)(struct
> v4l2_subdev *sd, const struct v4l2_sliced_vbi_data *vbi_data); +	int
> (*g_vbi_data)(struct v4l2_subdev *sd, struct v4l2_sliced_vbi_data
> *vbi_data); +	int (*s_std_output)(struct v4l2_subdev *sd, v4l2_std_id std);
> +	int (*s_stream)(struct v4l2_subdev *sd, int enable);
> +	int (*s_fmt)(struct v4l2_subdev *sd, struct v4l2_format *fmt);
> +	int (*g_fmt)(struct v4l2_subdev *sd, struct v4l2_format *fmt);
> +};
> +
> +struct v4l2_subdev_ops {
> +	const struct v4l2_subdev_core_ops  *core;
> +	const struct v4l2_subdev_tuner_ops *tuner;
> +	const struct v4l2_subdev_audio_ops *audio;
> +	const struct v4l2_subdev_video_ops *video;
> +};
> +
> +#define V4L2_SUBDEV_NAME_SIZE 32
> +
> +/* Each instance of a subdev driver should create this struct, either
> +   stand-alone or embedded in a larger struct.
> + */
> +struct v4l2_subdev {
> +	struct list_head list;
> +	struct module *owner;
> +	struct v4l2_device *dev;
> +	const struct v4l2_subdev_ops *ops;
> +	/* name must be unique */
> +	char name[V4L2_SUBDEV_NAME_SIZE];
> +	/* can be used to group similar subdevs, value is driver-specific */
> +	u32 grp_id;
> +	/* pointer to private data */
> +	void *priv;
> +};
> +
> +static inline void v4l2_set_subdevdata(struct v4l2_subdev *sd, void *p)
> +{
> +	sd->priv = p;
> +}
> +
> +static inline void *v4l2_get_subdevdata(const struct v4l2_subdev *sd)
> +{
> +	return sd->priv;
> +}
> +
> +/* Convert an ioctl-type command to the proper v4l2_subdev_ops function
> call. +   This is used by subdev modules that can be called by both
> old-style ioctl +   commands and through the v4l2_subdev_ops.
> +
> +   The ioctl API of the subdev driver can call this function to call the
> +   right ops based on the ioctl cmd and arg.
> +
> +   Once all subdev drivers have been converted and all drivers no longer
> +   use the ioctl interface, then this function can be removed.
> + */
> +int v4l2_subdev_command(struct v4l2_subdev *sd, unsigned cmd, void *arg);
> +
> +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);
> +	sd->ops = ops;
> +	sd->dev = NULL;
> +	sd->name[0] = '\0';
> +	sd->grp_id = 0;
> +	sd->priv = NULL;
> +}
> +
> +/* Call an ops of a v4l2_subdev, doing the right checks against
> +   NULL pointers.
> +
> +   Example: err = v4l2_subdev_call(sd, core, g_chip_ident, &chip);
> + */
> +#define v4l2_subdev_call(sd, o, f, args...)				\
> +	(!(sd) ? -ENODEV : (((sd) && (sd)->ops->o && (sd)->ops->o->f) ?	\
> +		(sd)->ops->o->f((sd) , ##args) : -ENOIOCTLCMD))
> +
> +#endif
>
> --
> video4linux-list mailing list
> Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
> https://www.redhat.com/mailman/listinfo/video4linux-list

Re: v4l2_device/v4l2_subdev: please review (PATCH 1/3)

[Hans Verkuil]

Hi Laurent,

Let me start by thanking you for reviewing this! Much appreciated.

On Saturday 29 November 2008 00:34:44 Laurent Pinchart wrote:
> Hi Hans,
>
> On Tuesday 25 November 2008, Hans Verkuil wrote:
> > As requested, the patches as separate posts for review.
> >
> > 	Hans
> >
> > # HG changeset patch
> > # User Hans Verkuil <hverkuil@xs4all.nl>
> > # Date 1227560990 -3600
> > # Node ID d9ec70c0b0c55e18813f91218c6da6212ca9b7e6
> > # Parent b63737bf9eef1ff8494cb7fbc2e818e6aff7a34f
> > v4l2: add v4l2_device and v4l2_subdev structs to the v4l2
> > framework.
> >
> > From: Hans Verkuil <hverkuil@xs4all.nl>
> >
> > Start implementing a proper v4l2 framework as discussed during the
> > Linux Plumbers Conference 2008.
> >
> > Introduces v4l2_device (for device instances) and v4l2_subdev
> > (representing sub-device instances).
> >
> > Priority: normal
> >
> > Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
>
> Comments inlined. I've reviewed the general approach only, so there
> might be some small issues at the code level that I haven't noticed.
>
> Would you mind holding the push request until we're done discussing
> the points I mention throughout this e-mail ?

Sure, no problem.

>
> > --- a/linux/drivers/media/video/Makefile	Mon Nov 24 10:51:20 2008
> > -0200 +++ b/linux/drivers/media/video/Makefile	Mon Nov 24 22:09:50
> > 2008 +0100 @@ -8,7 +8,7 @@ msp3400-objs	:=	msp3400-driver.o msp3400
> >
> >  stkwebcam-objs	:=	stk-webcam.o stk-sensor.o
> >
> > -videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o
> > +videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o
> > v4l2-subdev.o
> >
> >  obj-$(CONFIG_VIDEO_DEV) += videodev.o v4l2-compat-ioctl32.o
> > v4l2-int-device.o
> >
> > --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> > +++ b/linux/Documentation/video4linux/v4l2-framework.txt	Mon Nov 24
> > 22:09:50 2008 +0100 @@ -0,0 +1,360 @@
> > +Overview of the V4L2 driver framework
> > +=====================================
> > +
> > +This text documents the various structures provided by the V4L2
> > framework and +their relationships.
> > +
> > +
> > +Introduction
> > +------------
> > +
> > +The V4L2 drivers tend to be very complex due to the complexity of
> > the +hardware: most devices have multiple ICs, export multiple
> > device nodes in +/dev, and create also non-V4L2 devices such as
> > DVB, ALSA, FB, I2C and input +(IR) devices.
> > +
> > +Especially the fact that V4L2 drivers have to setup supporting ICs
> > to +do audio/video muxing/encoding/decoding makes it more complex
> > than most. +Usually these ICs are connected to the main bridge
> > driver through one or +more I2C busses, but other busses can also
> > be used. Such devices are +called 'sub-devices'.
>
> Do you know of other busses being used in (Linux supported) real
> video hardware, or is it currently theoretical only ?

The pxa_camera driver is one example of that. Also devices driven by 
GPIO pins can be implemented this way. I did that in ivtv for example: 
most cards use i2c audio muxers, but some have audio muxers that are 
commanded through GPIO so I created a v4l2_subdev that uses GPIO to 
drive these chips. Works very well indeed.

> > +For a long time the framework was limited to the video_device
> > struct for +creating V4L device nodes and video_buf for handling
> > the video buffers +(note that this document does not discuss the
> > video_buf framework). +
> > +This meant that all drivers had to do the setup of device
> > instances and +connecting to sub-devices themselves. Some of this
> > is quite complicated +to do right and many drivers never did do it
> > correctly.
> > +
> > +There is also a lot of common code that could never be refactored
> > due to +the lack of a framework.
> > +
> > +So this framework sets up the basic building blocks that all
> > drivers +need and this same framework should make it much easier to
> > refactor +common code into utility functions shared by all drivers.
> > +
> > +
> > +Structure of a driver
> > +---------------------
> > +
> > +All drivers have the following structure:
> > +
> > +1) A struct for each device instance containing the device state.
> > +
> > +2) A way of initializing and commanding sub-devices.
>
> This only applies to drivers handling "composite devices" (systems
> including sub-devices). Let's make sure the proposed framework
> doesn't make "simple devices" more complex to handle that they are
> now.

I will make a note of this.

> > +3) Creating V4L2 device nodes (/dev/videoX, /dev/vbiX, /dev/radioX
> > and +   /dev/vtxX) and keeping track of device-node specific data.
> > +
> > +4) Filehandle-specific structs containing per-filehandle data.
> > +
> > +This is a rough schematic of how it all relates:
> > +
> > +    device instances
> > +      |
> > +      +-sub-device instances
> > +      |
> > +      \-V4L2 device nodes
> > +	  |
> > +	  \-filehandle instances
> > +
> > +
> > +Structure of the framework
> > +--------------------------
> > +
> > +The framework closely resembles the driver structure: it has a
> > v4l2_device +struct for the device instance data, a v4l2_subdev
> > struct to refer to +sub-device instances, the video_device struct
> > stores V4L2 device node data +and a v4l2_fh struct keeps track of
> > filehandle instances (TODO: not yet +implemented).
> > +
> > +
> > +struct v4l2_device
> > +------------------
> > +
> > +Each device instance is represented by a struct v4l2_device
> > (v4l2-device.h). +Very simple devices can just allocate this
> > struct, but most of the time you +would embed this struct inside a
> > larger struct. +
> > +You must register the device instance:
> > +
> > +	v4l2_device_register(struct device *dev, struct v4l2_device
> > *v4l2_dev); +
> > +Registration will initialize the v4l2_device struct and link
> > dev->platform_data +to v4l2_dev.
>
> That's an abuse of platform_data, I don't think it was ever meant
> that way.

I took another look at this, and dev_set_drvdata() is a better solution. 
I'll change it.

> > Registration will also set v4l2_dev->name
> > to a value derived from +dev (driver name followed by the bus_id,
> > to be precise). You may change the +name after registration if you
> > want. +
> > +You unregister with:
> > +
> > +	v4l2_device_unregister(struct v4l2_device *v4l2_dev);
> > +
> > +Unregistering will also automatically unregister all subdevs from
> > the device. +
> > +Sometimes you need to iterate over all devices registered by a
> > specific +driver. This is usually the case if multiple device
> > drivers use the same +hardware. E.g. the ivtvfb driver is a
> > framebuffer driver that uses the ivtv +hardware. The same is true
> > for alsa drivers for example. +
> > +You can iterate over all registered devices as follows:
> > +
> > +static int callback(struct device *dev, void *p)
> > +{
> > +	struct v4l2_device *v4l2_dev = dev->platform_data;
> > +
> > +	/* test if this device was inited */
> > +	if (v4l2_dev == NULL)
> > +		return 0;
> > +	...
> > +	return 0;
> > +}
> > +
> > +int iterate(void *p)
> > +{
> > +	struct device_driver *drv;
> > +	int err;
> > +
> > +	/* Find driver 'ivtv' on the PCI bus.
> > +	   pci_bus_type is a global. For USB busses use usb_bus_type. */
> > +	drv = driver_find("ivtv", &pci_bus_type);
> > +	/* iterate over all ivtv device instances */
> > +	err = driver_for_each_device(drv, NULL, p, callback);
> > +	put_driver(drv);
> > +	return err;
> > +}
>
> I'm not sure to really see what use cases you're talking about. The
> above code looks good, but iterating over device bound to specific
> drivers should be done with care as it might be the sign of badly
> designed code. Every new instance of the above code should be
> reviewed with care.

This is only relevant when you have two separate drivers that both use 
the same hardware. This is the case for e.g. ivtv and ivtvfb (the 
framebuffer driver) and for e.g. cx88 and cx88-alsa (the alsa driver). 
In both cases the second driver needs to find the core data structures 
of the main driver. Right now this is implemented by having lists of 
device instances in the main driver which the secondary driver walks to 
find the devices. However, this information is already present in the 
kernel so rather than duplicating this information it is much better to 
use the kernel API to get hold of it. I put this code in the 
documentation since it not trivial to figure this out, which is 
probably the reason everyone keeps their own list.

> > +Sometimes you need to keep a running counter of the device
> > instance. This is +commonly used to map a device instance to an
> > index of a module option array. +
> > +The recommended approach is as follows:
> > +
> > +static atomic_t drv_instance = ATOMIC_INIT(0);
> > +
> > +static int __devinit drv_probe(struct pci_dev *dev,
> > +				const struct pci_device_id *pci_id)
> > +{
> > +	...
> > +	state->instance = atomic_inc_return(&drv_instance) - 1;
> > +}
> > +
> > +
> > +struct v4l2_subdev
> > +------------------
> > +
> > +Many drivers need to communicate with sub-devices. These devices
> > can do all +sort of tasks, but most commonly they handle audio
> > and/or video muxing, +encoding or decoding. For webcams common
> > sub-devices are sensors and camera +controllers.
> > +
> > +Usually these are I2C devices, but not necessarily. In order to
> > provide the +driver with a consistent interface to these
> > sub-devices the v4l2_subdev struct +(v4l2-subdev.h) was created.
> > +
> > +Each sub-device driver must have a v4l2_subdev struct. This struct
> > can be +stand-alone for simple sub-devices or it might be embedded
> > in a larger struct +if more state information needs to be stored.
> > Usually there is a low-level +device struct (e.g. i2c_client) that
> > contains the device data as setup +by the kernel. It is recommended
> > to store that pointer in the private +data of v4l2_subdev using
> > v4l2_set_subdevdata(). That makes it easy to go +from a v4l2_subdev
> > to the actual low-level bus-specific device data. +
> > +You also need a way to go from the low-level struct to
> > v4l2_subdev. For the +common i2c_client struct the
> > i2c_set_clientdata() call is used to store a +v4l2_subdev pointer,
> > for other busses you may have to use other methods. +
> > +From the bridge driver perspective you load the sub-device module
> > and somehow +obtain the v4l2_subdev pointer. For i2c devices this
> > is easy: you call +i2c_get_clientdata(). For other busses something
> > similar needs to be done. +Helper functions exists for sub-devices
> > on an I2C bus that do most of this +tricky work for you.
> > +
> > +Each v4l2_subdev contains function pointers that sub-device
> > drivers can +implement (or leave NULL if it is not applicable).
> > Since sub-devices can do +so many different things and you do not
> > want to end up with a huge ops struct +of which only a handful of
> > ops are commonly implemented, the function pointers +are sorted
> > according to category and each category has its own ops struct. +
>
> I like that.

Thank you :-)

I've done my best to sort everything into sensible categories, but once 
all i2c drivers are converted I'll probably do another review of this 
and move some ops around if appropriate.

> > +The top-level ops struct contains pointers to the category ops
> > structs, which +may be NULL if the subdev driver does not support
> > anything from that category. +
> > +It looks like this:
> > +
> > +struct v4l2_subdev_core_ops {
> > +	int (*g_chip_ident)(struct v4l2_subdev *sd, struct
> > v4l2_chip_ident *chip); +	int (*log_status)(struct v4l2_subdev
> > *sd);
> > +	int (*init)(struct v4l2_subdev *sd, u32 val);
> > +	...
> > +};
> > +
> > +struct v4l2_subdev_tuner_ops {
> > +	...
> > +};
> > +
> > +struct v4l2_subdev_audio_ops {
> > +	...
> > +};
> > +
> > +struct v4l2_subdev_video_ops {
> > +	...
> > +};
> > +
> > +struct v4l2_subdev_ops {
> > +	const struct v4l2_subdev_core_ops  *core;
> > +	const struct v4l2_subdev_tuner_ops *tuner;
> > +	const struct v4l2_subdev_audio_ops *audio;
> > +	const struct v4l2_subdev_video_ops *video;
> > +};
> > +
> > +The core ops are common to all subdevs, the other categories are
> > implemented +depending on the sub-device. E.g. a video device is
> > unlikely to support the +audio ops and vice versa.
> > +
> > +This setup limits the number of function pointers while still
> > making it easy +to add new ops and categories.
> > +
> > +A sub-device driver initializes the v4l2_subdev struct using:
> > +
> > +	v4l2_subdev_init(subdev, &ops);
> > +
> > +Afterwards you need to initialize subdev->name with a unique name
> > and set the +module owner. This is done for you if you use the i2c
> > helper functions. +
> > +A device (bridge) driver needs to register the v4l2_subdev with
> > the +v4l2_device:
> > +
> > +	int err = v4l2_device_register_subdev(device, subdev);
> > +
> > +This can fail if the subdev module disappeared before it could be
> > registered. +After this function was called successfully the
> > subdev->dev field points to +the v4l2_device.
> > +
> > +You can unregister a sub-device using:
> > +
> > +	v4l2_device_unregister_subdev(subdev);
> > +
> > +Afterwards the subdev module can be unloaded and subdev->dev ==
> > NULL. +
> > +You can call an ops function either directly:
> > +
> > +	err = subdev->ops->core->g_chip_ident(subdev, &chip);
> > +
> > +but it is better and easier to use this macro:
> > +
> > +	err = v4l2_subdev_call(subdev, core, g_chip_ident, &chip);
> > +
> > +The macro will to the right NULL pointer checks and returns
> > -ENODEV if subdev +is NULL,
>
> This should probably be checked when registering the sub-device
> instead of at all calls to sub-device operations.

That was my initial approach as well, but it is actually quite handy to 
have. For example in the ivtv driver there are some cards that can do 
video output. What I can do with this is to use a v4l2_subdev pointer 
which is NULL if the card doesn't support this feature. At the moment I 
have to check for this feature every time. The overhead is minimal, but 
it makes life easier and it allows for cleaner driver code.

Actually this idea came from Sakari Ailus and his v4l2-int-device work.

> > -ENOIOCTLCMD if either subdev->core or
> > subdev->core->g_chip_ident is +NULL, or the actual result of the
> > subdev->ops->core->g_chip_ident ops. +
> > +It is also possible to call all or a subset of the sub-devices:
> > +
> > +	v4l2_device_call_all(dev, 0, core, g_chip_ident, &chip);
> > +
> > +Any subdev that does not support this ops is skipped and error
> > results are +ignored. If you want to check for errors use this:
> > +
> > +	err = v4l2_device_call_until_err(dev, 0, core, g_chip_ident,
> > &chip); +
> > +Any error except -ENOIOCTLCMD will exit the loop with that error.
> > If no +errors (except -ENOIOCTLCMD) occured, then 0 is returned. +
> > +The second argument to both calls is a group ID. If 0, then all
> > subdevs are +called. If non-zero, then only those whose group ID
> > match that value will +be called. Before a bridge driver registers
> > a subdev it can set subdev->grp_id +to whatever value it wants
> > (it's 0 by default). This value is owned by the +bridge driver and
> > the sub-device driver will never modify or use it. +
> > +The group ID gives the bridge driver more control how callbacks
> > are called. +For example, there may be multiple audio chips on a
> > board, each capable of +changing the volume. But usually only one
> > will actually be used when the +user want to change the volume. You
> > can set the group ID for that subdev to +e.g. AUDIO_CONTROLLER and
> > specify that as the group ID value when calling
> > +v4l2_device_call_all(). That ensures that it will only go to the
> > subdev +that needs it.
>
> This is interesting as well.

Thanks to the brainstorm we had at the Linux Plumbers Conference. I 
think it was either Steve Toth or Mike Krufky who came up with this 
idea.

> > +The advantage of using v4l2_subdev is that it is a generic struct
> > and does +not contain any knowledge about the underlying hardware.
> > So a driver might +contain several subdevs that use an I2C bus, but
> > also a subdev that is +controlled through GPIO pins. This
> > distinction is only relevant when setting +up the device, but once
> > the subdev is registered it is completely transparent. +
>
> The bridge driver won't have to care about how sub-devices are
> accessed, but sub-devices drivers will have to be written
> specifically for the V4L2 subsystem. Do we have I2C devices shared
> between V4L(2) and DVB (and possible other subsystems) ? How would
> those be handled ? If your goal is to share v4l2_subdev drivers
> between V4L(2) and DVB I don't think the v4l2_ prefix is right.

I've gone back and forth between a v4l2_ and a media_ prefix a few times 
now and I've settled on v4l2_ (for now at least). First of all, at the 
moment it is a purely v4l2 interface. Secondly, I'm not entirely sure 
what sort of modifications I need to make to make it possible for DVB 
to start using this. It's a separate (but interesting) project and 
based on results of that it might well turn out that the prefix will be 
changed to something like media_.

However, starting to use the media_ prefix at this stage suggests that 
it is something that it definitely isn't at the moment.

> > +I2C sub-device drivers
> > +----------------------
> > +
> > +Since these drivers are so common, special helper functions are
> > available to +ease the use of these drivers (v4l2-common.h).
> > +
> > +The recommended method of adding v4l2_subdev support to an I2C
> > driver is to +embed the v4l2_subdev struct into the state struct
> > that is created for each +I2C device instance. Very simple devices
> > have no state struct and in that case +you can just create a
> > v4l2_subdev directly.
>
> The only I2C video-related chips I've dealt with were the ones used
> by DC10/DC10+ boards and those have no state struct. What is the
> state struct in your description ? Is it the driver-specific data
> allocated when the device is probed ?

Hmm, I need to clarify this: most i2c drivers (but not all) have a 
struct containing state information. The exact contents depends of 
course on the driver.

> > +Initialize the v4l2_subdev struct as follows:
> > +
> > +	v4l2_i2c_subdev_init(&state->sd, client, subdev_ops);
> > +
> > +This function will fill in all the fields of v4l2_subdev and
> > ensure that the +v4l2_subdev and i2c_client both point to one
> > another. +
> > +You should also add a helper inline function to go from a
> > v4l2_subdev pointer +to the state struct:
> > +
> > +static inline struct subdev_state *to_state(struct v4l2_subdev
> > *sd) +{
> > +	return container_of(sd, struct subdev_state, sd);
> > +}
>
> <chipname>_state might be a better name, it would avoid namespace
> clashes (I'm aware the state structure is supposed to be private to
> the I2C device driver, but we never know what might happen).

OK.

> > +Use this to go from the v4l2_subdev struct to the i2c_client
> > struct: +
> > +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> > +
> > +And this to go from an i2c_client to a v4l2_subdev struct:
> > +
> > +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > +
> > +Finally you need to make a command function to make
> > driver->command() +call the right subdev_ops functions:
> > +
> > +static int subdev_command(struct i2c_client *client, unsigned cmd,
> > void *arg) +{
> > +	return v4l2_subdev_command(i2c_get_clientdata(client), cmd, arg);
> > +}
> > +
> > +If driver->command is never used then you can leave this out.
> > Eventually the +driver->command usage should be removed from v4l.
>
> Should it then be added to the list of features scheduled for removal
> ?

No, driver->command is part of the i2c subsystem and we are not the only 
users, so it can't be scheduled for removal. But it is probably 
possible to add some check on this command callback at some low-level 
v4l2 core function to ensure it isn't used. But this is quite some time 
in the future for now.

> > +Make sure to call v4l2_device_unregister_subdev(sd) when the
> > remove() callback +is called. This will unregister the sub-device
> > from the bridge driver. It is +safe to call this even if the
> > sub-device was never registered.
> > +
> > +
> > +The bridge driver also has some helper functions it can use:
> > +
> > +struct v4l2_subdev *sd = v4l2_i2c_new_subdev(adapter,
> > "module_foo", "chipid", 0x36); +
> > +This loads the given module (can be NULL if no module needs to be
> > loaded) and +calls i2c_new_device() with the given i2c_adapter and
> > chip/address arguments. +If all goes well, then it registers the
> > subdev with the v4l2_device. It gets +the v4l2_device by calling
> > i2c_get_adapdata(adapter), so you should make sure +that adapdata
> > is set to v4l2_device when you setup the i2c_adapter in your
> > +driver.
> > +
> > +You can also use v4l2_i2c_new_probed_subdev() which is very
> > similar to +v4l2_i2c_new_subdev(), except that it has an array of
> > possible I2C addresses +that it should probe. Internally it calls
> > i2c_new_probed_device(). +
> > +Both functions return NULL if something went wrong.
> > +
> > +
> > +struct video_device
> > +-------------------
> > +
> > +TODO: document.
>
> Do you plan to change the video_device structure ?

I want to add a v4l2_device pointer to it, but that's all for now.

> > +
> > +struct v4l2_fh
> > +--------------
> > +
> > +TODO: document.
>
> I can't find that structure anywhere in your patches. Shouldn't it be
> removed from the document and added back when you introduce it ?

Yes, I think so too.

>
> > --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> > +++ b/linux/drivers/media/video/v4l2-device.c	Mon Nov 24 22:09:50
> > 2008 +0100 @@ -0,0 +1,80 @@
> > +/*
> > +    V4L2 device support.
> > +
> > +    Copyright (C) 2008  Hans Verkuil <hverkuil@xs4all.nl>
> > +
> > +    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/i2c.h>
> > +#include <linux/videodev2.h>
> > +#include <media/v4l2-device.h>
> > +
> > +void v4l2_device_register(struct device *dev, struct v4l2_device
> > *v4l2_dev) +{
> > +	BUG_ON(!dev || !v4l2_dev || dev->platform_data);
> > +	INIT_LIST_HEAD(&v4l2_dev->subdevs);
> > +	mutex_init(&v4l2_dev->lock);
> > +	v4l2_dev->dev = dev;
> > +	snprintf(v4l2_dev->name, sizeof(v4l2_dev->name), "%s %s",
> > +			dev->driver->name, dev->bus_id);
> > +	dev->platform_data = v4l2_dev;
> > +}
> > +EXPORT_SYMBOL(v4l2_device_register);
> > +
> > +void v4l2_device_unregister(struct v4l2_device *v4l2_dev)
> > +{
> > +	struct v4l2_subdev *sd, *next;
> > +
> > +	BUG_ON(!v4l2_dev || !v4l2_dev->dev ||
> > !v4l2_dev->dev->platform_data); +	v4l2_dev->dev->platform_data =
> > NULL;
> > +	/* unregister subdevs */
> > +	list_for_each_entry_safe(sd, next, &v4l2_dev->subdevs, list)
> > +		v4l2_device_unregister_subdev(sd);
> > +
> > +	v4l2_dev->dev = NULL;
> > +}
> > +EXPORT_SYMBOL(v4l2_device_unregister);
> > +
> > +int v4l2_device_register_subdev(struct v4l2_device *dev, struct
> > v4l2_subdev *sd) +{
> > +	/* Check that sd->dev is not set and that the subdev's name is
> > +	   filled in. */
> > +	BUG_ON(!dev || !sd || sd->dev || !sd->name[0]);
> > +	if (!try_module_get(sd->owner))
> > +		return -ENODEV;
> > +	sd->dev = dev;
> > +	mutex_lock(&dev->lock);
> > +	list_add_tail(&sd->list, &dev->subdevs);
> > +	mutex_unlock(&dev->lock);
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(v4l2_device_register_subdev);
> > +
> > +void v4l2_device_unregister_subdev(struct v4l2_subdev *sd)
> > +{
> > +	BUG_ON(!sd);
> > +	/* return if it isn't registered */
> > +	if (sd->dev == NULL)
> > +		return;
> > +	mutex_lock(&sd->dev->lock);
> > +	list_del(&sd->list);
> > +	mutex_unlock(&sd->dev->lock);
> > +	sd->dev = NULL;
> > +	module_put(sd->owner);
> > +}
> > +EXPORT_SYMBOL(v4l2_device_unregister_subdev);
> > --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> > +++ b/linux/drivers/media/video/v4l2-subdev.c	Mon Nov 24 22:09:50
> > 2008 +0100 @@ -0,0 +1,108 @@
> > +/*
> > +    V4L2 sub-device support.
> > +
> > +    Copyright (C) 2008  Hans Verkuil <hverkuil@xs4all.nl>
> > +
> > +    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/i2c.h>
> > +#include <linux/videodev2.h>
> > +#include <media/v4l2-subdev.h>
> > +
> > +int v4l2_subdev_command(struct v4l2_subdev *sd, unsigned cmd, void
> > *arg) +{
> > +	switch (cmd) {
> > +	case VIDIOC_QUERYCTRL:
> > +		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_QUERYMENU:
> > +		return v4l2_subdev_call(sd, core, queryctrl, arg);
> > +	case VIDIOC_LOG_STATUS:
> > +		return v4l2_subdev_call(sd, core, log_status);
> > +	case VIDIOC_G_CHIP_IDENT:
> > +		return v4l2_subdev_call(sd, core, g_chip_ident, arg);
> > +	case VIDIOC_INT_S_STANDBY:
> > +		return v4l2_subdev_call(sd, core, s_standby, *(u32 *)arg);
> > +	case VIDIOC_INT_RESET:
> > +		return v4l2_subdev_call(sd, core, reset, *(u32 *)arg);
> > +	case VIDIOC_INT_S_GPIO:
> > +		return v4l2_subdev_call(sd, core, s_gpio, *(u32 *)arg);
> > +	case VIDIOC_INT_INIT:
> > +		return v4l2_subdev_call(sd, core, init, *(u32 *)arg);
> > +#ifdef CONFIG_VIDEO_ADV_DEBUG
> > +	case VIDIOC_DBG_G_REGISTER:
> > +		return v4l2_subdev_call(sd, core, g_register, arg);
> > +	case VIDIOC_DBG_S_REGISTER:
> > +		return v4l2_subdev_call(sd, core, s_register, arg);
> > +#endif
> > +
> > +	case VIDIOC_INT_S_TUNER_MODE:
> > +		return v4l2_subdev_call(sd, tuner, s_mode, *(enum
> > v4l2_tuner_type *)arg); +	case AUDC_SET_RADIO:
> > +		return v4l2_subdev_call(sd, tuner, s_radio);
> > +	case VIDIOC_S_TUNER:
> > +		return v4l2_subdev_call(sd, tuner, s_tuner, arg);
> > +	case VIDIOC_G_TUNER:
> > +		return v4l2_subdev_call(sd, tuner, g_tuner, arg);
> > +	case VIDIOC_S_STD:
> > +		return v4l2_subdev_call(sd, tuner, s_std, *(v4l2_std_id *)arg);
> > +	case VIDIOC_S_FREQUENCY:
> > +		return v4l2_subdev_call(sd, tuner, s_frequency, arg);
> > +	case VIDIOC_G_FREQUENCY:
> > +		return v4l2_subdev_call(sd, tuner, g_frequency, arg);
> > +	case TUNER_SET_TYPE_ADDR:
> > +		return v4l2_subdev_call(sd, tuner, s_type_addr, arg);
> > +	case TUNER_SET_CONFIG:
> > +		return v4l2_subdev_call(sd, tuner, s_config, arg);
> > +
> > +	case VIDIOC_INT_AUDIO_CLOCK_FREQ:
> > +		return v4l2_subdev_call(sd, audio, s_clock_freq, *(u32 *)arg);
> > +	case VIDIOC_INT_S_AUDIO_ROUTING:
> > +		return v4l2_subdev_call(sd, audio, s_routing, arg);
> > +	case VIDIOC_INT_I2S_CLOCK_FREQ:
> > +		return v4l2_subdev_call(sd, audio, s_i2s_clock_freq, *(u32
> > *)arg); +
> > +	case VIDIOC_INT_S_VIDEO_ROUTING:
> > +		return v4l2_subdev_call(sd, video, s_routing, arg);
> > +	case VIDIOC_INT_S_CRYSTAL_FREQ:
> > +		return v4l2_subdev_call(sd, video, s_crystal_freq, arg);
> > +	case VIDIOC_INT_DECODE_VBI_LINE:
> > +		return v4l2_subdev_call(sd, video, decode_vbi_line, arg);
> > +	case VIDIOC_INT_S_VBI_DATA:
> > +		return v4l2_subdev_call(sd, video, s_vbi_data, arg);
> > +	case VIDIOC_INT_G_VBI_DATA:
> > +		return v4l2_subdev_call(sd, video, g_vbi_data, arg);
> > +	case VIDIOC_S_FMT:
> > +		return v4l2_subdev_call(sd, video, s_fmt, arg);
> > +	case VIDIOC_G_FMT:
> > +		return v4l2_subdev_call(sd, video, g_fmt, arg);
> > +	case VIDIOC_INT_S_STD_OUTPUT:
> > +		return v4l2_subdev_call(sd, video, s_std_output, *(v4l2_std_id
> > *)arg); +	case VIDIOC_STREAMON:
> > +		return v4l2_subdev_call(sd, video, s_stream, 1);
> > +	case VIDIOC_STREAMOFF:
> > +		return v4l2_subdev_call(sd, video, s_stream, 0);
> > +
> > +	default:
> > +		return v4l2_subdev_call(sd, core, ioctl, cmd, arg);
> > +	}
> > +}
> > +EXPORT_SYMBOL(v4l2_subdev_command);
> > --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> > +++ b/linux/include/media/v4l2-device.h	Mon Nov 24 22:09:50 2008
> > +0100 @@ -0,0 +1,124 @@
> > +/*
> > +    V4L2 device support header.
> > +
> > +    Copyright (C) 2008  Hans Verkuil <hverkuil@xs4all.nl>
> > +
> > +    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 + */
> > +
> > +#ifndef _V4L2_DEVICE_H
> > +#define _V4L2_DEVICE_H
> > +
> > +#include <media/v4l2-subdev.h>
> > +
> > +/* Each instance of a V4L2 device should create the v4l2_device
> > struct, +   either stand-alone or embedded in a larger struct.
>
> Is it required for all V4L2 devices, or only those that use
> subdevices ? What's the added value of v4l2_device for "simple"
> devices such as UVC webcams ?
>
> > +   It allows easy access to sub-devices (see v4l2-subdev.h) and
> > provides +   basic V4L2 device-level support.
> > +
> > +   Each driver should set a unique driver ID and instance number
> > (usually +   0 for the first instance and counting upwards from
> > there). +
> > +   It is recommended to set the name as follows:
> > +
> > +   <drivername> + instance number
> > +
> > +   If the driver name ends with a digit, then put a '-' between
> > the driver +   name and the instance number.
>
> The name field is automatically filled by v4l2_device_register. I
> think the comment is outdated.

Oops, you are right.

> > + */
> > +
> > +#define V4L2_DEVICE_NAME_SIZE 32
>
> The device name is made of the driver name and the bus id. The driver
> name has no upper bound, but the bus id is limited to BUS_ID_SIZE
> (currently 20) bytes. Shouldn't V4L2_DEVICE_NAME_SIZE be defined as
> (BUS_ID_SIZE + reasonable constant for the driver name) ?

Agreed.

> > +struct v4l2_device {
> > +	/* dev->platform_data points to this struct */
> > +	struct device *dev;
> > +	/* used to keep track of the registered subdevs */
> > +	struct list_head subdevs;
> > +	/* lock this struct; can be used by the driver as well if this
> > +	   struct is embedded into a larger struct. */
> > +	struct mutex lock;
> > +	/* unique device name */
> > +	char name[V4L2_DEVICE_NAME_SIZE];
> > +};
> > +
> > +/* Initialize v4l2_dev and make dev->platform_data point to
> > v4l2_dev */ +void v4l2_device_register(struct device *dev, struct
> > v4l2_device *v4l2_dev); +/* Set v4l2_dev->dev->platform_data to
> > NULL and unregister all sub-devices */ +void
> > v4l2_device_unregister(struct v4l2_device *v4l2_dev); +
> > +/* Register a subdev with a v4l2 device. While registered the
> > subdev module +   is marked as in-use. An error is returned if the
> > module is no longer +   loaded when you attempt to register it. */
> > +int v4l2_device_register_subdev(struct v4l2_device *dev, struct
> > v4l2_subdev *sd); +/* Unregister a subdev with a v4l2 device. Can
> > also be called if the subdev +   wasn't registered. In that case it
> > will do nothing. */
> > +void v4l2_device_unregister_subdev(struct v4l2_subdev *sd);
> > +
> > +/* Iterate over all subdevs. Warn if you iterate over the subdevs
> > without +   the device struct being locked. The next item is
> > prefetched, so you can +   delete the current item if necessary. */
> > +#define v4l2_device_for_each_subdev(sd, dev)				\
> > +	for (sd = list_entry((dev)->subdevs.next, typeof(*sd), list), 	\
> > +	     ({ WARN_ON(!mutex_is_locked(&(dev)->lock)); 0; });		\
> > +		prefetch(sd->list.next), &sd->list != &(dev)->subdevs; 	\
> > +	     sd = list_entry(sd->list.next, typeof(*sd), list))
> > +
> > +/* Call the specified callback for all subdevs matching the
> > condition. +   Ignore any errors. */
> > +#define __v4l2_device_call_subdevs(dev, cond, o, f, args...) 		\
> > +	do { 								\
> > +		struct v4l2_subdev *sd; 				\
> > +									\
> > +		mutex_lock(&(dev)->lock); 				\
> > +		list_for_each_entry(sd, &(dev)->subdevs, list)   	\
> > +			if ((cond) && sd->ops->o && sd->ops->o->f) 	\
> > +				sd->ops->o->f(sd , ##args); 		\
> > +		mutex_unlock(&(dev)->lock); 				\
> > +	} while (0)
> > +
> > +/* Call the specified callback for all subdevs matching the
> > condition. +   If the callback returns an error other than 0 or
> > -ENOIOCTLCMD, then +   return with that error code. */
> > +#define __v4l2_device_call_subdevs_until_err(dev, cond, o, f,
> > args...)  \ +({ 									\
> > +	struct v4l2_subdev *sd; 					\
> > +	int err = 0; 							\
> > +									\
> > +	mutex_lock(&(dev)->lock); 					\
> > +	list_for_each_entry(sd, &(dev)->subdevs, list) { 		\
> > +		if ((cond) && sd->ops->o && sd->ops->o->f) 		\
> > +			err = sd->ops->o->f(sd , ##args); 		\
> > +		if (err && err != -ENOIOCTLCMD)				\
> > +			break; 						\
> > +	} 								\
> > +	mutex_unlock(&(dev)->lock); 					\
> > +	(err == -ENOIOCTLCMD) ? 0 : err; 				\
> > +})
> > +
> > +/* Call the specified callback for all subdevs matching grp_id (if
> > 0, then +   match them all). Ignore any errors. */
> > +#define v4l2_device_call_all(dev, grp_id, o, f, args...) 		\
> > +	__v4l2_device_call_subdevs(dev, 				\
> > +			!(grp_id) || sd->grp_id == (grp_id), o, f , ##args)
> > +
> > +/* Call the specified callback for all subdevs matching grp_id (if
> > 0, then +   match them all). If the callback returns an error other
> > than 0 or +   -ENOIOCTLCMD, then return with that error code. */
> > +#define v4l2_device_call_until_err(dev, grp_id, o, f, args...) 		\
> > +	__v4l2_device_call_subdevs_until_err(dev,			\
> > +		       !(grp_id) || sd->grp_id == (grp_id), o, f , ##args)
> > +
> > +#endif
> > --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> > +++ b/linux/include/media/v4l2-subdev.h	Mon Nov 24 22:09:50 2008
> > +0100 @@ -0,0 +1,188 @@
> > +/*
> > +    V4L2 sub-device support header.
> > +
> > +    Copyright (C) 2008  Hans Verkuil <hverkuil@xs4all.nl>
> > +
> > +    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 + */
> > +
> > +#ifndef _V4L2_SUBDEV_H
> > +#define _V4L2_SUBDEV_H
> > +
> > +#include <media/v4l2-common.h>
> > +
> > +struct v4l2_device;
> > +struct v4l2_subdev;
> > +struct tuner_setup;
> > +
> > +/* Sub-devices are devices that are connected somehow to the main
> > bridge +   device. These devices are usually audio/video
> > muxers/encoders/decoders or +   sensors and webcam controllers.
> > +
> > +   Usually these devices are controlled through an i2c bus, but
> > other busses +   may also be used.
> > +
> > +   The v4l2_subdev struct provides a way of accessing these
> > devices in a +   generic manner. Most operations that these
> > sub-devices support fall in +   a few categories: core ops, audio
> > ops, video ops and tuner ops. +
> > +   More categories can be added if needed, although this should
> > remain a +   limited set (no more than approx. 8 categories).
> > +
> > +   Each category has its own set of ops that subdev drivers can
> > implement. +
> > +   A subdev driver can leave the pointer to the category ops NULL
> > if +   it does not implement them (e.g. an audio subdev will
> > generally not +   implement the video category ops). The exception
> > is the core category: +   this must always be present.
> > +
> > +   These ops are all used internally so it is no problem to
> > change, remove +   or add ops or move ops from one to another
> > category. Currently these +   ops are based on the original ioctls,
> > but since ops are not limited to +   one argument there is room for
> > improvement here once all i2c subdev +   drivers are converted to
> > use these ops.
> > + */
> > +
> > +/* Core ops: it is highly recommended to implement at least these
> > ops: +
> > +   g_chip_ident
> > +   log_status
> > +   g_register
> > +   s_register
> > +
> > +   This provides basic debugging support.
> > +
> > +   The ioctl ops is meant for generic ioctl-like commands.
> > Depending on +   the use-case it might be better to use
> > subdev-specific ops (currently +   not yet implemented) since ops
> > provide proper type-checking. + */
> > +struct v4l2_subdev_core_ops {
> > +	int (*g_chip_ident)(struct v4l2_subdev *sd, struct
> > v4l2_chip_ident *chip); +	int (*log_status)(struct v4l2_subdev
> > *sd);
> > +	int (*init)(struct v4l2_subdev *sd, u32 val);
> > +	int (*s_standby)(struct v4l2_subdev *sd, u32 standby);
> > +	int (*reset)(struct v4l2_subdev *sd, u32 val);
> > +	int (*s_gpio)(struct v4l2_subdev *sd, u32 val);
> > +	int (*queryctrl)(struct v4l2_subdev *sd, struct v4l2_queryctrl
> > *qc); +	int (*g_ctrl)(struct v4l2_subdev *sd, struct v4l2_control
> > *ctrl); +	int (*s_ctrl)(struct v4l2_subdev *sd, struct v4l2_control
> > *ctrl); +	int (*querymenu)(struct v4l2_subdev *sd, struct
> > v4l2_querymenu *qm); +	int (*ioctl)(struct v4l2_subdev *sd, int
> > cmd, void *arg); +#ifdef CONFIG_VIDEO_ADV_DEBUG
> > +	int (*g_register)(struct v4l2_subdev *sd, struct v4l2_register
> > *reg); +	int (*s_register)(struct v4l2_subdev *sd, struct
> > v4l2_register *reg); +#endif
> > +};
> > +
> > +struct v4l2_subdev_tuner_ops {
> > +	int (*s_mode)(struct v4l2_subdev *sd, enum v4l2_tuner_type);
> > +	int (*s_radio)(struct v4l2_subdev *sd);
> > +	int (*s_frequency)(struct v4l2_subdev *sd, struct v4l2_frequency
> > *freq); +	int (*g_frequency)(struct v4l2_subdev *sd, struct
> > v4l2_frequency *freq); +	int (*g_tuner)(struct v4l2_subdev *sd,
> > struct v4l2_tuner *vt); +	int (*s_tuner)(struct v4l2_subdev *sd,
> > struct v4l2_tuner *vt); +	int (*s_std)(struct v4l2_subdev *sd,
> > v4l2_std_id norm);
> > +	int (*s_type_addr)(struct v4l2_subdev *sd, struct tuner_setup
> > *type); +	int (*s_config)(struct v4l2_subdev *sd, const struct
> > v4l2_priv_tun_config *config); +};
> > +
> > +struct v4l2_subdev_audio_ops {
> > +	int (*s_clock_freq)(struct v4l2_subdev *sd, u32 freq);
> > +	int (*s_i2s_clock_freq)(struct v4l2_subdev *sd, u32 freq);
> > +	int (*s_routing)(struct v4l2_subdev *sd, const struct
> > v4l2_routing *route); +};
> > +
> > +struct v4l2_subdev_video_ops {
> > +	int (*s_routing)(struct v4l2_subdev *sd, const struct
> > v4l2_routing *route); +	int (*s_crystal_freq)(struct v4l2_subdev
> > *sd, struct v4l2_crystal_freq *freq); +	int
> > (*decode_vbi_line)(struct v4l2_subdev *sd, struct
> > v4l2_decode_vbi_line *vbi_line); +	int (*s_vbi_data)(struct
> > v4l2_subdev *sd, const struct v4l2_sliced_vbi_data *vbi_data);
> > +	int (*g_vbi_data)(struct v4l2_subdev *sd, struct
> > v4l2_sliced_vbi_data *vbi_data); +	int (*s_std_output)(struct
> > v4l2_subdev *sd, v4l2_std_id std); +	int (*s_stream)(struct
> > v4l2_subdev *sd, int enable);
> > +	int (*s_fmt)(struct v4l2_subdev *sd, struct v4l2_format *fmt);
> > +	int (*g_fmt)(struct v4l2_subdev *sd, struct v4l2_format *fmt);
> > +};
> > +
> > +struct v4l2_subdev_ops {
> > +	const struct v4l2_subdev_core_ops  *core;
> > +	const struct v4l2_subdev_tuner_ops *tuner;
> > +	const struct v4l2_subdev_audio_ops *audio;
> > +	const struct v4l2_subdev_video_ops *video;
> > +};
> > +
> > +#define V4L2_SUBDEV_NAME_SIZE 32
> > +
> > +/* Each instance of a subdev driver should create this struct,
> > either +   stand-alone or embedded in a larger struct.
> > + */
> > +struct v4l2_subdev {
> > +	struct list_head list;
> > +	struct module *owner;
> > +	struct v4l2_device *dev;
> > +	const struct v4l2_subdev_ops *ops;
> > +	/* name must be unique */
> > +	char name[V4L2_SUBDEV_NAME_SIZE];
> > +	/* can be used to group similar subdevs, value is driver-specific
> > */ +	u32 grp_id;
> > +	/* pointer to private data */
> > +	void *priv;
> > +};
> > +
> > +static inline void v4l2_set_subdevdata(struct v4l2_subdev *sd,
> > void *p) +{
> > +	sd->priv = p;
> > +}
> > +
> > +static inline void *v4l2_get_subdevdata(const struct v4l2_subdev
> > *sd) +{
> > +	return sd->priv;
> > +}
> > +
> > +/* Convert an ioctl-type command to the proper v4l2_subdev_ops
> > function call. +   This is used by subdev modules that can be
> > called by both old-style ioctl +   commands and through the
> > v4l2_subdev_ops. +
> > +   The ioctl API of the subdev driver can call this function to
> > call the +   right ops based on the ioctl cmd and arg.
> > +
> > +   Once all subdev drivers have been converted and all drivers no
> > longer +   use the ioctl interface, then this function can be
> > removed. + */
> > +int v4l2_subdev_command(struct v4l2_subdev *sd, unsigned cmd, void
> > *arg); +
> > +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);
> > +	sd->ops = ops;
> > +	sd->dev = NULL;
> > +	sd->name[0] = '\0';
> > +	sd->grp_id = 0;
> > +	sd->priv = NULL;
> > +}
> > +
> > +/* Call an ops of a v4l2_subdev, doing the right checks against
> > +   NULL pointers.
> > +
> > +   Example: err = v4l2_subdev_call(sd, core, g_chip_ident, &chip);
> > + */
> > +#define v4l2_subdev_call(sd, o, f, args...)				\
> > +	(!(sd) ? -ENODEV : (((sd) && (sd)->ops->o && (sd)->ops->o->f) ?	\
> > +		(sd)->ops->o->f((sd) , ##args) : -ENOIOCTLCMD))
> > +
> > +#endif
> >
> > --
> > video4linux-list mailing list
> > Unsubscribe
> > mailto:video4linux-list-request@redhat.com?subject=unsubscribe
> > https://www.redhat.com/mailman/listinfo/video4linux-list

Once again, thank you for the review. I'll update my repository 
accordingly.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: v4l2_device/v4l2_subdev: please review (PATCH 1/3)

[Guennadi Liakhovetski]

On Sat, 29 Nov 2008, Hans Verkuil wrote:

> > > +Introduction
> > > +------------
> > > +
> > > +The V4L2 drivers tend to be very complex due to the complexity of
> > > the +hardware: most devices have multiple ICs, export multiple
> > > device nodes in +/dev, and create also non-V4L2 devices such as
> > > DVB, ALSA, FB, I2C and input +(IR) devices.
> > > +
> > > +Especially the fact that V4L2 drivers have to setup supporting ICs
> > > to +do audio/video muxing/encoding/decoding makes it more complex
> > > than most. +Usually these ICs are connected to the main bridge
> > > driver through one or +more I2C busses, but other busses can also
> > > be used. Such devices are +called 'sub-devices'.
> >
> > Do you know of other busses being used in (Linux supported) real
> > video hardware, or is it currently theoretical only ?
> 
> The pxa_camera driver is one example of that. Also devices driven by 
> GPIO pins can be implemented this way. I did that in ivtv for example: 
> most cards use i2c audio muxers, but some have audio muxers that are 
> commanded through GPIO so I created a v4l2_subdev that uses GPIO to 
> drive these chips. Works very well indeed.

I think pxa-camera (as well as sh-mobile-ceu and other soc-camera host 
drivers in the works) is not a very good example here. Sensors connected 
to embedded controllers like PXA indeed use a dedicated camera bus but 
only for data exchange. This bus comprises of data and synchronisation 
lines only. Sensors are still connected over an i2c bus for control and 
configuration, also been open to other busses, I haven't seen such 
examples as yet. I might have misunderstood what has been discussed here 
though.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: v4l2_device/v4l2_subdev: please review (PATCH 1/3)

[Hans Verkuil]

On Saturday 29 November 2008 20:08:44 Guennadi Liakhovetski wrote:
> On Sat, 29 Nov 2008, Hans Verkuil wrote:
> > > > +Introduction
> > > > +------------
> > > > +
> > > > +The V4L2 drivers tend to be very complex due to the complexity
> > > > of the +hardware: most devices have multiple ICs, export
> > > > multiple device nodes in +/dev, and create also non-V4L2
> > > > devices such as DVB, ALSA, FB, I2C and input +(IR) devices.
> > > > +
> > > > +Especially the fact that V4L2 drivers have to setup supporting
> > > > ICs to +do audio/video muxing/encoding/decoding makes it more
> > > > complex than most. +Usually these ICs are connected to the main
> > > > bridge driver through one or +more I2C busses, but other busses
> > > > can also be used. Such devices are +called 'sub-devices'.
> > >
> > > Do you know of other busses being used in (Linux supported) real
> > > video hardware, or is it currently theoretical only ?
> >
> > The pxa_camera driver is one example of that. Also devices driven
> > by GPIO pins can be implemented this way. I did that in ivtv for
> > example: most cards use i2c audio muxers, but some have audio
> > muxers that are commanded through GPIO so I created a v4l2_subdev
> > that uses GPIO to drive these chips. Works very well indeed.
>
> I think pxa-camera (as well as sh-mobile-ceu and other soc-camera
> host drivers in the works) is not a very good example here. Sensors
> connected to embedded controllers like PXA indeed use a dedicated
> camera bus but only for data exchange. This bus comprises of data and
> synchronisation lines only. Sensors are still connected over an i2c
> bus for control and configuration, also been open to other busses, I
> haven't seen such examples as yet. I might have misunderstood what
> has been discussed here though.

I agree that it not the best example, although it is perfectly possible 
to see this as a controller sub-device. Having the same mechanism to 
talk to any type of hardware involved in video capture and display has 
definite advantages.

Once these patches are in I would definitely recommend that people start 
experimenting with them. Also be aware that this is just the first 
step. I'm going to improve on these two fundamental structs 
(v4l2_device and v4l2_subdev) to add much improved support for 
controls. Currently drivers have to spend way too much effort on 
implementing all the control handling code.

And there are many more things one can do with these structures. I'll 
just take it step by step.

Regards,

         Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: v4l2_device/v4l2_subdev: please review (PATCH 1/3)

[David Brownell]

On Saturday 29 November 2008, Hans Verkuil wrote:
> > > +And this to go from an i2c_client to a v4l2_subdev struct:
> > > +
> > > +   struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > > +
> > > +Finally you need to make a command function to make
> > > +driver->command() call the right subdev_ops functions:
> > > +
> > > +static int
> > > +subdev_command(struct i2c_client *client, unsigned cmd, void *arg)
> > > +{ 
> > > +   return v4l2_subdev_command(i2c_get_clientdata(client), cmd, arg);
> > > +}
> > > +
> > > +If driver->command is never used then you can leave this out.
> > > +Eventually the driver->command usage should be removed from v4l.
> >
> > Should it then be added to the list of features scheduled for removal?
> 
> No, driver->command is part of the i2c subsystem and we are not the only 
> users, so it can't be scheduled for removal.

ISTR that the only use of the i2c_driver.command() mechanism
is for video/DVB driver support.  It's kind of an ugly wart,
and it'd be good to see such ioctl-ish interfaces vanish.

This came up a while back as part of a "how can we clean up
the I2C stack" discussion.  So it would be nice if V4L2 wasn't
effectively adding new reasons (and advice) to use this.

- Dave

Re: v4l2_device/v4l2_subdev: please review (PATCH 1/3)

[Hans Verkuil]

On Saturday 29 November 2008 20:46:43 David Brownell wrote:
> On Saturday 29 November 2008, Hans Verkuil wrote:
> > > > +And this to go from an i2c_client to a v4l2_subdev struct:
> > > > +
> > > > +   struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > > > +
> > > > +Finally you need to make a command function to make
> > > > +driver->command() call the right subdev_ops functions:
> > > > +
> > > > +static int
> > > > +subdev_command(struct i2c_client *client, unsigned cmd, void
> > > > *arg) +{
> > > > +   return v4l2_subdev_command(i2c_get_clientdata(client), cmd,
> > > > arg); +}
> > > > +
> > > > +If driver->command is never used then you can leave this out.
> > > > +Eventually the driver->command usage should be removed from
> > > > v4l.
> > >
> > > Should it then be added to the list of features scheduled for
> > > removal?
> >
> > No, driver->command is part of the i2c subsystem and we are not the
> > only users, so it can't be scheduled for removal.
>
> ISTR that the only use of the i2c_driver.command() mechanism
> is for video/DVB driver support.  It's kind of an ugly wart,
> and it'd be good to see such ioctl-ish interfaces vanish.
>
> This came up a while back as part of a "how can we clean up
> the I2C stack" discussion.  So it would be nice if V4L2 wasn't
> effectively adding new reasons (and advice) to use this.

If v4l is indeed the only user, then once all i2c drivers are converted 
to this new sub device API, then command can indeed go away.

Would be nice, but it will probably take a few kernel cycles before we 
are there. It's a lot of work, but very worthwhile in my opinion.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: v4l2_device/v4l2_subdev: please review (PATCH 1/3)

[Laurent Pinchart]

Hi Hans,

On Saturday 29 November 2008, Hans Verkuil wrote:
> Hi Laurent,
>
> Let me start by thanking you for reviewing this! Much appreciated.

You're welcome.

> On Saturday 29 November 2008 00:34:44 Laurent Pinchart wrote:
> > Hi Hans,
> >
> > On Tuesday 25 November 2008, Hans Verkuil wrote:
> > > As requested, the patches as separate posts for review.
> > >
> > > 	Hans
> > >
> > > # HG changeset patch
> > > # User Hans Verkuil <hverkuil@xs4all.nl>
> > > # Date 1227560990 -3600
> > > # Node ID d9ec70c0b0c55e18813f91218c6da6212ca9b7e6
> > > # Parent b63737bf9eef1ff8494cb7fbc2e818e6aff7a34f
> > > v4l2: add v4l2_device and v4l2_subdev structs to the v4l2
> > > framework.
> > >
> > > From: Hans Verkuil <hverkuil@xs4all.nl>
> > >
> > > Start implementing a proper v4l2 framework as discussed during the
> > > Linux Plumbers Conference 2008.
> > >
> > > Introduces v4l2_device (for device instances) and v4l2_subdev
> > > (representing sub-device instances).
> > >
> > > Priority: normal
> > >
> > > Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
> >
> > Comments inlined. I've reviewed the general approach only, so there
> > might be some small issues at the code level that I haven't noticed.
> >
> > Would you mind holding the push request until we're done discussing
> > the points I mention throughout this e-mail ?
>
> Sure, no problem.
>
> > > --- a/linux/drivers/media/video/Makefile	Mon Nov 24 10:51:20 2008
> > > -0200 +++ b/linux/drivers/media/video/Makefile	Mon Nov 24 22:09:50
> > > 2008 +0100 @@ -8,7 +8,7 @@ msp3400-objs	:=	msp3400-driver.o msp3400
> > >
> > >  stkwebcam-objs	:=	stk-webcam.o stk-sensor.o
> > >
> > > -videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o
> > > +videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o
> > > v4l2-subdev.o
> > >
> > >  obj-$(CONFIG_VIDEO_DEV) += videodev.o v4l2-compat-ioctl32.o
> > > v4l2-int-device.o
> > >
> > > --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> > > +++ b/linux/Documentation/video4linux/v4l2-framework.txt	Mon Nov 24
> > > 22:09:50 2008 +0100 @@ -0,0 +1,360 @@
> > > +Overview of the V4L2 driver framework
> > > +=====================================
> > > +
> > > +This text documents the various structures provided by the V4L2
> > > framework and +their relationships.
> > > +
> > > +
> > > +Introduction
> > > +------------
> > > +
> > > +The V4L2 drivers tend to be very complex due to the complexity of
> > > the +hardware: most devices have multiple ICs, export multiple
> > > device nodes in +/dev, and create also non-V4L2 devices such as
> > > DVB, ALSA, FB, I2C and input +(IR) devices.
> > > +
> > > +Especially the fact that V4L2 drivers have to setup supporting ICs
> > > to +do audio/video muxing/encoding/decoding makes it more complex
> > > than most. +Usually these ICs are connected to the main bridge
> > > driver through one or +more I2C busses, but other busses can also
> > > be used. Such devices are +called 'sub-devices'.
> >
> > Do you know of other busses being used in (Linux supported) real
> > video hardware, or is it currently theoretical only ?
>
> The pxa_camera driver is one example of that. Also devices driven by
> GPIO pins can be implemented this way. I did that in ivtv for example:
> most cards use i2c audio muxers, but some have audio muxers that are
> commanded through GPIO so I created a v4l2_subdev that uses GPIO to
> drive these chips. Works very well indeed.
>
> > > +For a long time the framework was limited to the video_device
> > > struct for +creating V4L device nodes and video_buf for handling
> > > the video buffers +(note that this document does not discuss the
> > > video_buf framework). +
> > > +This meant that all drivers had to do the setup of device
> > > instances and +connecting to sub-devices themselves. Some of this
> > > is quite complicated +to do right and many drivers never did do it
> > > correctly.
> > > +
> > > +There is also a lot of common code that could never be refactored
> > > due to +the lack of a framework.
> > > +
> > > +So this framework sets up the basic building blocks that all
> > > drivers +need and this same framework should make it much easier to
> > > refactor +common code into utility functions shared by all drivers.
> > > +
> > > +
> > > +Structure of a driver
> > > +---------------------
> > > +
> > > +All drivers have the following structure:
> > > +
> > > +1) A struct for each device instance containing the device state.
> > > +
> > > +2) A way of initializing and commanding sub-devices.
> >
> > This only applies to drivers handling "composite devices" (systems
> > including sub-devices). Let's make sure the proposed framework
> > doesn't make "simple devices" more complex to handle that they are
> > now.
>
> I will make a note of this.
>
> > > +3) Creating V4L2 device nodes (/dev/videoX, /dev/vbiX, /dev/radioX
> > > and +   /dev/vtxX) and keeping track of device-node specific data.
> > > +
> > > +4) Filehandle-specific structs containing per-filehandle data.
> > > +
> > > +This is a rough schematic of how it all relates:
> > > +
> > > +    device instances
> > > +      |
> > > +      +-sub-device instances
> > > +      |
> > > +      \-V4L2 device nodes
> > > +	  |
> > > +	  \-filehandle instances
> > > +
> > > +
> > > +Structure of the framework
> > > +--------------------------
> > > +
> > > +The framework closely resembles the driver structure: it has a
> > > v4l2_device +struct for the device instance data, a v4l2_subdev
> > > struct to refer to +sub-device instances, the video_device struct
> > > stores V4L2 device node data +and a v4l2_fh struct keeps track of
> > > filehandle instances (TODO: not yet +implemented).
> > > +
> > > +
> > > +struct v4l2_device
> > > +------------------
> > > +
> > > +Each device instance is represented by a struct v4l2_device
> > > (v4l2-device.h). +Very simple devices can just allocate this
> > > struct, but most of the time you +would embed this struct inside a
> > > larger struct. +
> > > +You must register the device instance:
> > > +
> > > +	v4l2_device_register(struct device *dev, struct v4l2_device
> > > *v4l2_dev); +
> > > +Registration will initialize the v4l2_device struct and link
> > > dev->platform_data +to v4l2_dev.
> >
> > That's an abuse of platform_data, I don't think it was ever meant
> > that way.
>
> I took another look at this, and dev_set_drvdata() is a better solution.
> I'll change it.
>
> > > Registration will also set v4l2_dev->name
> > > to a value derived from +dev (driver name followed by the bus_id,
> > > to be precise). You may change the +name after registration if you
> > > want. +
> > > +You unregister with:
> > > +
> > > +	v4l2_device_unregister(struct v4l2_device *v4l2_dev);
> > > +
> > > +Unregistering will also automatically unregister all subdevs from
> > > the device. +
> > > +Sometimes you need to iterate over all devices registered by a
> > > specific +driver. This is usually the case if multiple device
> > > drivers use the same +hardware. E.g. the ivtvfb driver is a
> > > framebuffer driver that uses the ivtv +hardware. The same is true
> > > for alsa drivers for example. +
> > > +You can iterate over all registered devices as follows:
> > > +
> > > +static int callback(struct device *dev, void *p)
> > > +{
> > > +	struct v4l2_device *v4l2_dev = dev->platform_data;
> > > +
> > > +	/* test if this device was inited */
> > > +	if (v4l2_dev == NULL)
> > > +		return 0;
> > > +	...
> > > +	return 0;
> > > +}
> > > +
> > > +int iterate(void *p)
> > > +{
> > > +	struct device_driver *drv;
> > > +	int err;
> > > +
> > > +	/* Find driver 'ivtv' on the PCI bus.
> > > +	   pci_bus_type is a global. For USB busses use usb_bus_type. */
> > > +	drv = driver_find("ivtv", &pci_bus_type);
> > > +	/* iterate over all ivtv device instances */
> > > +	err = driver_for_each_device(drv, NULL, p, callback);
> > > +	put_driver(drv);
> > > +	return err;
> > > +}
> >
> > I'm not sure to really see what use cases you're talking about. The
> > above code looks good, but iterating over device bound to specific
> > drivers should be done with care as it might be the sign of badly
> > designed code. Every new instance of the above code should be
> > reviewed with care.
>
> This is only relevant when you have two separate drivers that both use
> the same hardware. This is the case for e.g. ivtv and ivtvfb (the
> framebuffer driver) and for e.g. cx88 and cx88-alsa (the alsa driver).
> In both cases the second driver needs to find the core data structures
> of the main driver. Right now this is implemented by having lists of
> device instances in the main driver which the secondary driver walks to
> find the devices. However, this information is already present in the
> kernel so rather than duplicating this information it is much better to
> use the kernel API to get hold of it. I put this code in the
> documentation since it not trivial to figure this out, which is
> probably the reason everyone keeps their own list.

That's a very interesting use case that might benefit other subsystems as 
well. I've had a quick look at the cx88 driver(s), it seems cx88 hardware 
expose separate video and audio PCI functions. Is this right ? In that case 
the two drivers (cx88 and cx88-alsa) bind to two separate devices. Do you 
know if other subsystems need to share driver data between separate devices ? 
If the cx88-alsa driver needs data from the "main" driver, how do you handle 
the cx88-alsa module being loaded before cx88 ?

As for the ivtv driver, this is quite different. I suppose the ivtvfb driver 
wouldn't need to walk the devices list if it wasn't a separate module, 
right ?

> > > +Sometimes you need to keep a running counter of the device
> > > instance. This is +commonly used to map a device instance to an
> > > index of a module option array. +
> > > +The recommended approach is as follows:
> > > +
> > > +static atomic_t drv_instance = ATOMIC_INIT(0);
> > > +
> > > +static int __devinit drv_probe(struct pci_dev *dev,
> > > +				const struct pci_device_id *pci_id)
> > > +{
> > > +	...
> > > +	state->instance = atomic_inc_return(&drv_instance) - 1;
> > > +}
> > > +
> > > +
> > > +struct v4l2_subdev
> > > +------------------
> > > +
> > > +Many drivers need to communicate with sub-devices. These devices
> > > can do all +sort of tasks, but most commonly they handle audio
> > > and/or video muxing, +encoding or decoding. For webcams common
> > > sub-devices are sensors and camera +controllers.
> > > +
> > > +Usually these are I2C devices, but not necessarily. In order to
> > > provide the +driver with a consistent interface to these
> > > sub-devices the v4l2_subdev struct +(v4l2-subdev.h) was created.
> > > +
> > > +Each sub-device driver must have a v4l2_subdev struct. This struct
> > > can be +stand-alone for simple sub-devices or it might be embedded
> > > in a larger struct +if more state information needs to be stored.
> > > Usually there is a low-level +device struct (e.g. i2c_client) that
> > > contains the device data as setup +by the kernel. It is recommended
> > > to store that pointer in the private +data of v4l2_subdev using
> > > v4l2_set_subdevdata(). That makes it easy to go +from a v4l2_subdev
> > > to the actual low-level bus-specific device data. +
> > > +You also need a way to go from the low-level struct to
> > > v4l2_subdev. For the +common i2c_client struct the
> > > i2c_set_clientdata() call is used to store a +v4l2_subdev pointer,
> > > for other busses you may have to use other methods. +
> > > +From the bridge driver perspective you load the sub-device module
> > > and somehow +obtain the v4l2_subdev pointer. For i2c devices this
> > > is easy: you call +i2c_get_clientdata(). For other busses something
> > > similar needs to be done. +Helper functions exists for sub-devices
> > > on an I2C bus that do most of this +tricky work for you.
> > > +
> > > +Each v4l2_subdev contains function pointers that sub-device
> > > drivers can +implement (or leave NULL if it is not applicable).
> > > Since sub-devices can do +so many different things and you do not
> > > want to end up with a huge ops struct +of which only a handful of
> > > ops are commonly implemented, the function pointers +are sorted
> > > according to category and each category has its own ops struct. +
> >
> > I like that.
>
> Thank you :-)
>
> I've done my best to sort everything into sensible categories, but once
> all i2c drivers are converted I'll probably do another review of this
> and move some ops around if appropriate.

I haven't double-checked each operation, but that doesn't have to be sorted 
now.

> > > +The top-level ops struct contains pointers to the category ops
> > > structs, which +may be NULL if the subdev driver does not support
> > > anything from that category. +
> > > +It looks like this:
> > > +
> > > +struct v4l2_subdev_core_ops {
> > > +	int (*g_chip_ident)(struct v4l2_subdev *sd, struct
> > > v4l2_chip_ident *chip); +	int (*log_status)(struct v4l2_subdev
> > > *sd);
> > > +	int (*init)(struct v4l2_subdev *sd, u32 val);
> > > +	...
> > > +};
> > > +
> > > +struct v4l2_subdev_tuner_ops {
> > > +	...
> > > +};
> > > +
> > > +struct v4l2_subdev_audio_ops {
> > > +	...
> > > +};
> > > +
> > > +struct v4l2_subdev_video_ops {
> > > +	...
> > > +};
> > > +
> > > +struct v4l2_subdev_ops {
> > > +	const struct v4l2_subdev_core_ops  *core;
> > > +	const struct v4l2_subdev_tuner_ops *tuner;
> > > +	const struct v4l2_subdev_audio_ops *audio;
> > > +	const struct v4l2_subdev_video_ops *video;
> > > +};
> > > +
> > > +The core ops are common to all subdevs, the other categories are
> > > implemented +depending on the sub-device. E.g. a video device is
> > > unlikely to support the +audio ops and vice versa.
> > > +
> > > +This setup limits the number of function pointers while still
> > > making it easy +to add new ops and categories.
> > > +
> > > +A sub-device driver initializes the v4l2_subdev struct using:
> > > +
> > > +	v4l2_subdev_init(subdev, &ops);
> > > +
> > > +Afterwards you need to initialize subdev->name with a unique name
> > > and set the +module owner. This is done for you if you use the i2c
> > > helper functions. +
> > > +A device (bridge) driver needs to register the v4l2_subdev with
> > > the +v4l2_device:
> > > +
> > > +	int err = v4l2_device_register_subdev(device, subdev);
> > > +
> > > +This can fail if the subdev module disappeared before it could be
> > > registered. +After this function was called successfully the
> > > subdev->dev field points to +the v4l2_device.
> > > +
> > > +You can unregister a sub-device using:
> > > +
> > > +	v4l2_device_unregister_subdev(subdev);
> > > +
> > > +Afterwards the subdev module can be unloaded and subdev->dev ==
> > > NULL. +
> > > +You can call an ops function either directly:
> > > +
> > > +	err = subdev->ops->core->g_chip_ident(subdev, &chip);
> > > +
> > > +but it is better and easier to use this macro:
> > > +
> > > +	err = v4l2_subdev_call(subdev, core, g_chip_ident, &chip);
> > > +
> > > +The macro will to the right NULL pointer checks and returns
> > > -ENODEV if subdev +is NULL,
> >
> > This should probably be checked when registering the sub-device
> > instead of at all calls to sub-device operations.
>
> That was my initial approach as well, but it is actually quite handy to
> have. For example in the ivtv driver there are some cards that can do
> video output. What I can do with this is to use a v4l2_subdev pointer
> which is NULL if the card doesn't support this feature. At the moment I
> have to check for this feature every time. The overhead is minimal, but
> it makes life easier and it allows for cleaner driver code.

If the card can't do video output the driver should probably block video 
output operations at a higher lever, before reaching calls to the sub-device. 
I'm not concerned much about performances here (calls to sub-devices are 
probably not performance critical) but more about code cleanness. It's quite 
easy to remove important checks if you know that lower level code will 
probably catch your mistakes.

> Actually this idea came from Sakari Ailus and his v4l2-int-device work.
>
> > > -ENOIOCTLCMD if either subdev->core or
> > > subdev->core->g_chip_ident is +NULL, or the actual result of the
> > > subdev->ops->core->g_chip_ident ops. +
> > > +It is also possible to call all or a subset of the sub-devices:
> > > +
> > > +	v4l2_device_call_all(dev, 0, core, g_chip_ident, &chip);
> > > +
> > > +Any subdev that does not support this ops is skipped and error
> > > results are +ignored. If you want to check for errors use this:
> > > +
> > > +	err = v4l2_device_call_until_err(dev, 0, core, g_chip_ident,
> > > &chip); +
> > > +Any error except -ENOIOCTLCMD will exit the loop with that error.
> > > If no +errors (except -ENOIOCTLCMD) occured, then 0 is returned. +
> > > +The second argument to both calls is a group ID. If 0, then all
> > > subdevs are +called. If non-zero, then only those whose group ID
> > > match that value will +be called. Before a bridge driver registers
> > > a subdev it can set subdev->grp_id +to whatever value it wants
> > > (it's 0 by default). This value is owned by the +bridge driver and
> > > the sub-device driver will never modify or use it. +
> > > +The group ID gives the bridge driver more control how callbacks
> > > are called. +For example, there may be multiple audio chips on a
> > > board, each capable of +changing the volume. But usually only one
> > > will actually be used when the +user want to change the volume. You
> > > can set the group ID for that subdev to +e.g. AUDIO_CONTROLLER and
> > > specify that as the group ID value when calling
> > > +v4l2_device_call_all(). That ensures that it will only go to the
> > > subdev +that needs it.
> >
> > This is interesting as well.
>
> Thanks to the brainstorm we had at the Linux Plumbers Conference. I
> think it was either Steve Toth or Mike Krufky who came up with this
> idea.
>
> > > +The advantage of using v4l2_subdev is that it is a generic struct
> > > and does +not contain any knowledge about the underlying hardware.
> > > So a driver might +contain several subdevs that use an I2C bus, but
> > > also a subdev that is +controlled through GPIO pins. This
> > > distinction is only relevant when setting +up the device, but once
> > > the subdev is registered it is completely transparent. +
> >
> > The bridge driver won't have to care about how sub-devices are
> > accessed, but sub-devices drivers will have to be written
> > specifically for the V4L2 subsystem. Do we have I2C devices shared
> > between V4L(2) and DVB (and possible other subsystems) ? How would
> > those be handled ? If your goal is to share v4l2_subdev drivers
> > between V4L(2) and DVB I don't think the v4l2_ prefix is right.
>
> I've gone back and forth between a v4l2_ and a media_ prefix a few times
> now and I've settled on v4l2_ (for now at least). First of all, at the
> moment it is a purely v4l2 interface. Secondly, I'm not entirely sure
> what sort of modifications I need to make to make it possible for DVB
> to start using this. It's a separate (but interesting) project and
> based on results of that it might well turn out that the prefix will be
> changed to something like media_.
>
> However, starting to use the media_ prefix at this stage suggests that
> it is something that it definitely isn't at the moment.

Ok, so your proposal only targets video input devices (with optional 
framebuffer, sound or hid support) that implement the v4l2 interfaces and 
that use sub-devices, right ?

> > > +I2C sub-device drivers
> > > +----------------------
> > > +
> > > +Since these drivers are so common, special helper functions are
> > > available to +ease the use of these drivers (v4l2-common.h).
> > > +
> > > +The recommended method of adding v4l2_subdev support to an I2C
> > > driver is to +embed the v4l2_subdev struct into the state struct
> > > that is created for each +I2C device instance. Very simple devices
> > > have no state struct and in that case +you can just create a
> > > v4l2_subdev directly.
> >
> > The only I2C video-related chips I've dealt with were the ones used
> > by DC10/DC10+ boards and those have no state struct. What is the
> > state struct in your description ? Is it the driver-specific data
> > allocated when the device is probed ?
>
> Hmm, I need to clarify this: most i2c drivers (but not all) have a
> struct containing state information. The exact contents depends of
> course on the driver.
>
> > > +Initialize the v4l2_subdev struct as follows:
> > > +
> > > +	v4l2_i2c_subdev_init(&state->sd, client, subdev_ops);
> > > +
> > > +This function will fill in all the fields of v4l2_subdev and
> > > ensure that the +v4l2_subdev and i2c_client both point to one
> > > another. +
> > > +You should also add a helper inline function to go from a
> > > v4l2_subdev pointer +to the state struct:
> > > +
> > > +static inline struct subdev_state *to_state(struct v4l2_subdev
> > > *sd) +{
> > > +	return container_of(sd, struct subdev_state, sd);
> > > +}
> >
> > <chipname>_state might be a better name, it would avoid namespace
> > clashes (I'm aware the state structure is supposed to be private to
> > the I2C device driver, but we never know what might happen).
>
> OK.
>
> > > +Use this to go from the v4l2_subdev struct to the i2c_client
> > > struct: +
> > > +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> > > +
> > > +And this to go from an i2c_client to a v4l2_subdev struct:
> > > +
> > > +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > > +
> > > +Finally you need to make a command function to make
> > > driver->command() +call the right subdev_ops functions:
> > > +
> > > +static int subdev_command(struct i2c_client *client, unsigned cmd,
> > > void *arg) +{
> > > +	return v4l2_subdev_command(i2c_get_clientdata(client), cmd, arg);
> > > +}
> > > +
> > > +If driver->command is never used then you can leave this out.
> > > Eventually the +driver->command usage should be removed from v4l.
> >
> > Should it then be added to the list of features scheduled for removal
> > ?
>
> No, driver->command is part of the i2c subsystem and we are not the only
> users, so it can't be scheduled for removal. But it is probably
> possible to add some check on this command callback at some low-level
> v4l2 core function to ensure it isn't used. But this is quite some time
> in the future for now.

Ok.

> > > +Make sure to call v4l2_device_unregister_subdev(sd) when the
> > > remove() callback +is called. This will unregister the sub-device
> > > from the bridge driver. It is +safe to call this even if the
> > > sub-device was never registered.
> > > +
> > > +
> > > +The bridge driver also has some helper functions it can use:
> > > +
> > > +struct v4l2_subdev *sd = v4l2_i2c_new_subdev(adapter,
> > > "module_foo", "chipid", 0x36); +
> > > +This loads the given module (can be NULL if no module needs to be
> > > loaded) and +calls i2c_new_device() with the given i2c_adapter and
> > > chip/address arguments. +If all goes well, then it registers the
> > > subdev with the v4l2_device. It gets +the v4l2_device by calling
> > > i2c_get_adapdata(adapter), so you should make sure +that adapdata
> > > is set to v4l2_device when you setup the i2c_adapter in your
> > > +driver.
> > > +
> > > +You can also use v4l2_i2c_new_probed_subdev() which is very
> > > similar to +v4l2_i2c_new_subdev(), except that it has an array of
> > > possible I2C addresses +that it should probe. Internally it calls
> > > i2c_new_probed_device(). +
> > > +Both functions return NULL if something went wrong.
> > > +
> > > +
> > > +struct video_device
> > > +-------------------
> > > +
> > > +TODO: document.
> >
> > Do you plan to change the video_device structure ?
>
> I want to add a v4l2_device pointer to it, but that's all for now.

Just to make things clear, v4l2_device is the parent structure, right ?

> > > +
> > > +struct v4l2_fh
> > > +--------------
> > > +
> > > +TODO: document.
> >
> > I can't find that structure anywhere in your patches. Shouldn't it be
> > removed from the document and added back when you introduce it ?
>
> Yes, I think so too.
>
> > > --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> > > +++ b/linux/drivers/media/video/v4l2-device.c	Mon Nov 24 22:09:50
> > > 2008 +0100 @@ -0,0 +1,80 @@
> > > +/*
> > > +    V4L2 device support.
> > > +
> > > +    Copyright (C) 2008  Hans Verkuil <hverkuil@xs4all.nl>
> > > +
> > > +    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/i2c.h>
> > > +#include <linux/videodev2.h>
> > > +#include <media/v4l2-device.h>
> > > +
> > > +void v4l2_device_register(struct device *dev, struct v4l2_device
> > > *v4l2_dev) +{
> > > +	BUG_ON(!dev || !v4l2_dev || dev->platform_data);
> > > +	INIT_LIST_HEAD(&v4l2_dev->subdevs);
> > > +	mutex_init(&v4l2_dev->lock);
> > > +	v4l2_dev->dev = dev;
> > > +	snprintf(v4l2_dev->name, sizeof(v4l2_dev->name), "%s %s",
> > > +			dev->driver->name, dev->bus_id);
> > > +	dev->platform_data = v4l2_dev;
> > > +}
> > > +EXPORT_SYMBOL(v4l2_device_register);
> > > +
> > > +void v4l2_device_unregister(struct v4l2_device *v4l2_dev)
> > > +{
> > > +	struct v4l2_subdev *sd, *next;
> > > +
> > > +	BUG_ON(!v4l2_dev || !v4l2_dev->dev ||
> > > !v4l2_dev->dev->platform_data); +	v4l2_dev->dev->platform_data =
> > > NULL;
> > > +	/* unregister subdevs */
> > > +	list_for_each_entry_safe(sd, next, &v4l2_dev->subdevs, list)
> > > +		v4l2_device_unregister_subdev(sd);
> > > +
> > > +	v4l2_dev->dev = NULL;
> > > +}
> > > +EXPORT_SYMBOL(v4l2_device_unregister);
> > > +
> > > +int v4l2_device_register_subdev(struct v4l2_device *dev, struct
> > > v4l2_subdev *sd) +{
> > > +	/* Check that sd->dev is not set and that the subdev's name is
> > > +	   filled in. */
> > > +	BUG_ON(!dev || !sd || sd->dev || !sd->name[0]);
> > > +	if (!try_module_get(sd->owner))
> > > +		return -ENODEV;
> > > +	sd->dev = dev;
> > > +	mutex_lock(&dev->lock);
> > > +	list_add_tail(&sd->list, &dev->subdevs);
> > > +	mutex_unlock(&dev->lock);
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL(v4l2_device_register_subdev);
> > > +
> > > +void v4l2_device_unregister_subdev(struct v4l2_subdev *sd)
> > > +{
> > > +	BUG_ON(!sd);
> > > +	/* return if it isn't registered */
> > > +	if (sd->dev == NULL)
> > > +		return;
> > > +	mutex_lock(&sd->dev->lock);
> > > +	list_del(&sd->list);
> > > +	mutex_unlock(&sd->dev->lock);
> > > +	sd->dev = NULL;
> > > +	module_put(sd->owner);
> > > +}
> > > +EXPORT_SYMBOL(v4l2_device_unregister_subdev);
> > > --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> > > +++ b/linux/drivers/media/video/v4l2-subdev.c	Mon Nov 24 22:09:50
> > > 2008 +0100 @@ -0,0 +1,108 @@
> > > +/*
> > > +    V4L2 sub-device support.
> > > +
> > > +    Copyright (C) 2008  Hans Verkuil <hverkuil@xs4all.nl>
> > > +
> > > +    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/i2c.h>
> > > +#include <linux/videodev2.h>
> > > +#include <media/v4l2-subdev.h>
> > > +
> > > +int v4l2_subdev_command(struct v4l2_subdev *sd, unsigned cmd, void
> > > *arg) +{
> > > +	switch (cmd) {
> > > +	case VIDIOC_QUERYCTRL:
> > > +		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_QUERYMENU:
> > > +		return v4l2_subdev_call(sd, core, queryctrl, arg);
> > > +	case VIDIOC_LOG_STATUS:
> > > +		return v4l2_subdev_call(sd, core, log_status);
> > > +	case VIDIOC_G_CHIP_IDENT:
> > > +		return v4l2_subdev_call(sd, core, g_chip_ident, arg);
> > > +	case VIDIOC_INT_S_STANDBY:
> > > +		return v4l2_subdev_call(sd, core, s_standby, *(u32 *)arg);
> > > +	case VIDIOC_INT_RESET:
> > > +		return v4l2_subdev_call(sd, core, reset, *(u32 *)arg);
> > > +	case VIDIOC_INT_S_GPIO:
> > > +		return v4l2_subdev_call(sd, core, s_gpio, *(u32 *)arg);
> > > +	case VIDIOC_INT_INIT:
> > > +		return v4l2_subdev_call(sd, core, init, *(u32 *)arg);
> > > +#ifdef CONFIG_VIDEO_ADV_DEBUG
> > > +	case VIDIOC_DBG_G_REGISTER:
> > > +		return v4l2_subdev_call(sd, core, g_register, arg);
> > > +	case VIDIOC_DBG_S_REGISTER:
> > > +		return v4l2_subdev_call(sd, core, s_register, arg);
> > > +#endif
> > > +
> > > +	case VIDIOC_INT_S_TUNER_MODE:
> > > +		return v4l2_subdev_call(sd, tuner, s_mode, *(enum
> > > v4l2_tuner_type *)arg); +	case AUDC_SET_RADIO:
> > > +		return v4l2_subdev_call(sd, tuner, s_radio);
> > > +	case VIDIOC_S_TUNER:
> > > +		return v4l2_subdev_call(sd, tuner, s_tuner, arg);
> > > +	case VIDIOC_G_TUNER:
> > > +		return v4l2_subdev_call(sd, tuner, g_tuner, arg);
> > > +	case VIDIOC_S_STD:
> > > +		return v4l2_subdev_call(sd, tuner, s_std, *(v4l2_std_id *)arg);
> > > +	case VIDIOC_S_FREQUENCY:
> > > +		return v4l2_subdev_call(sd, tuner, s_frequency, arg);
> > > +	case VIDIOC_G_FREQUENCY:
> > > +		return v4l2_subdev_call(sd, tuner, g_frequency, arg);
> > > +	case TUNER_SET_TYPE_ADDR:
> > > +		return v4l2_subdev_call(sd, tuner, s_type_addr, arg);
> > > +	case TUNER_SET_CONFIG:
> > > +		return v4l2_subdev_call(sd, tuner, s_config, arg);
> > > +
> > > +	case VIDIOC_INT_AUDIO_CLOCK_FREQ:
> > > +		return v4l2_subdev_call(sd, audio, s_clock_freq, *(u32 *)arg);
> > > +	case VIDIOC_INT_S_AUDIO_ROUTING:
> > > +		return v4l2_subdev_call(sd, audio, s_routing, arg);
> > > +	case VIDIOC_INT_I2S_CLOCK_FREQ:
> > > +		return v4l2_subdev_call(sd, audio, s_i2s_clock_freq, *(u32
> > > *)arg); +
> > > +	case VIDIOC_INT_S_VIDEO_ROUTING:
> > > +		return v4l2_subdev_call(sd, video, s_routing, arg);
> > > +	case VIDIOC_INT_S_CRYSTAL_FREQ:
> > > +		return v4l2_subdev_call(sd, video, s_crystal_freq, arg);
> > > +	case VIDIOC_INT_DECODE_VBI_LINE:
> > > +		return v4l2_subdev_call(sd, video, decode_vbi_line, arg);
> > > +	case VIDIOC_INT_S_VBI_DATA:
> > > +		return v4l2_subdev_call(sd, video, s_vbi_data, arg);
> > > +	case VIDIOC_INT_G_VBI_DATA:
> > > +		return v4l2_subdev_call(sd, video, g_vbi_data, arg);
> > > +	case VIDIOC_S_FMT:
> > > +		return v4l2_subdev_call(sd, video, s_fmt, arg);
> > > +	case VIDIOC_G_FMT:
> > > +		return v4l2_subdev_call(sd, video, g_fmt, arg);
> > > +	case VIDIOC_INT_S_STD_OUTPUT:
> > > +		return v4l2_subdev_call(sd, video, s_std_output, *(v4l2_std_id
> > > *)arg); +	case VIDIOC_STREAMON:
> > > +		return v4l2_subdev_call(sd, video, s_stream, 1);
> > > +	case VIDIOC_STREAMOFF:
> > > +		return v4l2_subdev_call(sd, video, s_stream, 0);
> > > +
> > > +	default:
> > > +		return v4l2_subdev_call(sd, core, ioctl, cmd, arg);
> > > +	}
> > > +}
> > > +EXPORT_SYMBOL(v4l2_subdev_command);
> > > --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> > > +++ b/linux/include/media/v4l2-device.h	Mon Nov 24 22:09:50 2008
> > > +0100 @@ -0,0 +1,124 @@
> > > +/*
> > > +    V4L2 device support header.
> > > +
> > > +    Copyright (C) 2008  Hans Verkuil <hverkuil@xs4all.nl>
> > > +
> > > +    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 + */
> > > +
> > > +#ifndef _V4L2_DEVICE_H
> > > +#define _V4L2_DEVICE_H
> > > +
> > > +#include <media/v4l2-subdev.h>
> > > +
> > > +/* Each instance of a V4L2 device should create the v4l2_device
> > > struct, +   either stand-alone or embedded in a larger struct.
> >
> > Is it required for all V4L2 devices, or only those that use
> > subdevices ? What's the added value of v4l2_device for "simple"
> > devices such as UVC webcams ?
> >
> > > +   It allows easy access to sub-devices (see v4l2-subdev.h) and
> > > provides +   basic V4L2 device-level support.
> > > +
> > > +   Each driver should set a unique driver ID and instance number
> > > (usually +   0 for the first instance and counting upwards from
> > > there). +
> > > +   It is recommended to set the name as follows:
> > > +
> > > +   <drivername> + instance number
> > > +
> > > +   If the driver name ends with a digit, then put a '-' between
> > > the driver +   name and the instance number.
> >
> > The name field is automatically filled by v4l2_device_register. I
> > think the comment is outdated.
>
> Oops, you are right.
>
> > > + */
> > > +
> > > +#define V4L2_DEVICE_NAME_SIZE 32
> >
> > The device name is made of the driver name and the bus id. The driver
> > name has no upper bound, but the bus id is limited to BUS_ID_SIZE
> > (currently 20) bytes. Shouldn't V4L2_DEVICE_NAME_SIZE be defined as
> > (BUS_ID_SIZE + reasonable constant for the driver name) ?
>
> Agreed.
>
> > > +struct v4l2_device {
> > > +	/* dev->platform_data points to this struct */
> > > +	struct device *dev;
> > > +	/* used to keep track of the registered subdevs */
> > > +	struct list_head subdevs;
> > > +	/* lock this struct; can be used by the driver as well if this
> > > +	   struct is embedded into a larger struct. */
> > > +	struct mutex lock;
> > > +	/* unique device name */
> > > +	char name[V4L2_DEVICE_NAME_SIZE];
> > > +};
> > > +
> > > +/* Initialize v4l2_dev and make dev->platform_data point to
> > > v4l2_dev */ +void v4l2_device_register(struct device *dev, struct
> > > v4l2_device *v4l2_dev); +/* Set v4l2_dev->dev->platform_data to
> > > NULL and unregister all sub-devices */ +void
> > > v4l2_device_unregister(struct v4l2_device *v4l2_dev); +
> > > +/* Register a subdev with a v4l2 device. While registered the
> > > subdev module +   is marked as in-use. An error is returned if the
> > > module is no longer +   loaded when you attempt to register it. */
> > > +int v4l2_device_register_subdev(struct v4l2_device *dev, struct
> > > v4l2_subdev *sd); +/* Unregister a subdev with a v4l2 device. Can
> > > also be called if the subdev +   wasn't registered. In that case it
> > > will do nothing. */
> > > +void v4l2_device_unregister_subdev(struct v4l2_subdev *sd);
> > > +
> > > +/* Iterate over all subdevs. Warn if you iterate over the subdevs
> > > without +   the device struct being locked. The next item is
> > > prefetched, so you can +   delete the current item if necessary. */
> > > +#define v4l2_device_for_each_subdev(sd, dev)				\
> > > +	for (sd = list_entry((dev)->subdevs.next, typeof(*sd), list), 	\
> > > +	     ({ WARN_ON(!mutex_is_locked(&(dev)->lock)); 0; });		\
> > > +		prefetch(sd->list.next), &sd->list != &(dev)->subdevs; 	\
> > > +	     sd = list_entry(sd->list.next, typeof(*sd), list))
> > > +
> > > +/* Call the specified callback for all subdevs matching the
> > > condition. +   Ignore any errors. */
> > > +#define __v4l2_device_call_subdevs(dev, cond, o, f, args...) 		\
> > > +	do { 								\
> > > +		struct v4l2_subdev *sd; 				\
> > > +									\
> > > +		mutex_lock(&(dev)->lock); 				\
> > > +		list_for_each_entry(sd, &(dev)->subdevs, list)   	\
> > > +			if ((cond) && sd->ops->o && sd->ops->o->f) 	\
> > > +				sd->ops->o->f(sd , ##args); 		\
> > > +		mutex_unlock(&(dev)->lock); 				\
> > > +	} while (0)
> > > +
> > > +/* Call the specified callback for all subdevs matching the
> > > condition. +   If the callback returns an error other than 0 or
> > > -ENOIOCTLCMD, then +   return with that error code. */
> > > +#define __v4l2_device_call_subdevs_until_err(dev, cond, o, f,
> > > args...)  \ +({ 									\
> > > +	struct v4l2_subdev *sd; 					\
> > > +	int err = 0; 							\
> > > +									\
> > > +	mutex_lock(&(dev)->lock); 					\
> > > +	list_for_each_entry(sd, &(dev)->subdevs, list) { 		\
> > > +		if ((cond) && sd->ops->o && sd->ops->o->f) 		\
> > > +			err = sd->ops->o->f(sd , ##args); 		\
> > > +		if (err && err != -ENOIOCTLCMD)				\
> > > +			break; 						\
> > > +	} 								\
> > > +	mutex_unlock(&(dev)->lock); 					\
> > > +	(err == -ENOIOCTLCMD) ? 0 : err; 				\
> > > +})
> > > +
> > > +/* Call the specified callback for all subdevs matching grp_id (if
> > > 0, then +   match them all). Ignore any errors. */
> > > +#define v4l2_device_call_all(dev, grp_id, o, f, args...) 		\
> > > +	__v4l2_device_call_subdevs(dev, 				\
> > > +			!(grp_id) || sd->grp_id == (grp_id), o, f , ##args)
> > > +
> > > +/* Call the specified callback for all subdevs matching grp_id (if
> > > 0, then +   match them all). If the callback returns an error other
> > > than 0 or +   -ENOIOCTLCMD, then return with that error code. */
> > > +#define v4l2_device_call_until_err(dev, grp_id, o, f, args...) 		\
> > > +	__v4l2_device_call_subdevs_until_err(dev,			\
> > > +		       !(grp_id) || sd->grp_id == (grp_id), o, f , ##args)
> > > +
> > > +#endif
> > > --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> > > +++ b/linux/include/media/v4l2-subdev.h	Mon Nov 24 22:09:50 2008
> > > +0100 @@ -0,0 +1,188 @@
> > > +/*
> > > +    V4L2 sub-device support header.
> > > +
> > > +    Copyright (C) 2008  Hans Verkuil <hverkuil@xs4all.nl>
> > > +
> > > +    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 + */
> > > +
> > > +#ifndef _V4L2_SUBDEV_H
> > > +#define _V4L2_SUBDEV_H
> > > +
> > > +#include <media/v4l2-common.h>
> > > +
> > > +struct v4l2_device;
> > > +struct v4l2_subdev;
> > > +struct tuner_setup;
> > > +
> > > +/* Sub-devices are devices that are connected somehow to the main
> > > bridge +   device. These devices are usually audio/video
> > > muxers/encoders/decoders or +   sensors and webcam controllers.
> > > +
> > > +   Usually these devices are controlled through an i2c bus, but
> > > other busses +   may also be used.
> > > +
> > > +   The v4l2_subdev struct provides a way of accessing these
> > > devices in a +   generic manner. Most operations that these
> > > sub-devices support fall in +   a few categories: core ops, audio
> > > ops, video ops and tuner ops. +
> > > +   More categories can be added if needed, although this should
> > > remain a +   limited set (no more than approx. 8 categories).
> > > +
> > > +   Each category has its own set of ops that subdev drivers can
> > > implement. +
> > > +   A subdev driver can leave the pointer to the category ops NULL
> > > if +   it does not implement them (e.g. an audio subdev will
> > > generally not +   implement the video category ops). The exception
> > > is the core category: +   this must always be present.
> > > +
> > > +   These ops are all used internally so it is no problem to
> > > change, remove +   or add ops or move ops from one to another
> > > category. Currently these +   ops are based on the original ioctls,
> > > but since ops are not limited to +   one argument there is room for
> > > improvement here once all i2c subdev +   drivers are converted to
> > > use these ops.
> > > + */
> > > +
> > > +/* Core ops: it is highly recommended to implement at least these
> > > ops: +
> > > +   g_chip_ident
> > > +   log_status
> > > +   g_register
> > > +   s_register
> > > +
> > > +   This provides basic debugging support.
> > > +
> > > +   The ioctl ops is meant for generic ioctl-like commands.
> > > Depending on +   the use-case it might be better to use
> > > subdev-specific ops (currently +   not yet implemented) since ops
> > > provide proper type-checking. + */
> > > +struct v4l2_subdev_core_ops {
> > > +	int (*g_chip_ident)(struct v4l2_subdev *sd, struct
> > > v4l2_chip_ident *chip); +	int (*log_status)(struct v4l2_subdev
> > > *sd);
> > > +	int (*init)(struct v4l2_subdev *sd, u32 val);
> > > +	int (*s_standby)(struct v4l2_subdev *sd, u32 standby);
> > > +	int (*reset)(struct v4l2_subdev *sd, u32 val);
> > > +	int (*s_gpio)(struct v4l2_subdev *sd, u32 val);
> > > +	int (*queryctrl)(struct v4l2_subdev *sd, struct v4l2_queryctrl
> > > *qc); +	int (*g_ctrl)(struct v4l2_subdev *sd, struct v4l2_control
> > > *ctrl); +	int (*s_ctrl)(struct v4l2_subdev *sd, struct v4l2_control
> > > *ctrl); +	int (*querymenu)(struct v4l2_subdev *sd, struct
> > > v4l2_querymenu *qm); +	int (*ioctl)(struct v4l2_subdev *sd, int
> > > cmd, void *arg); +#ifdef CONFIG_VIDEO_ADV_DEBUG
> > > +	int (*g_register)(struct v4l2_subdev *sd, struct v4l2_register
> > > *reg); +	int (*s_register)(struct v4l2_subdev *sd, struct
> > > v4l2_register *reg); +#endif
> > > +};
> > > +
> > > +struct v4l2_subdev_tuner_ops {
> > > +	int (*s_mode)(struct v4l2_subdev *sd, enum v4l2_tuner_type);
> > > +	int (*s_radio)(struct v4l2_subdev *sd);
> > > +	int (*s_frequency)(struct v4l2_subdev *sd, struct v4l2_frequency
> > > *freq); +	int (*g_frequency)(struct v4l2_subdev *sd, struct
> > > v4l2_frequency *freq); +	int (*g_tuner)(struct v4l2_subdev *sd,
> > > struct v4l2_tuner *vt); +	int (*s_tuner)(struct v4l2_subdev *sd,
> > > struct v4l2_tuner *vt); +	int (*s_std)(struct v4l2_subdev *sd,
> > > v4l2_std_id norm);
> > > +	int (*s_type_addr)(struct v4l2_subdev *sd, struct tuner_setup
> > > *type); +	int (*s_config)(struct v4l2_subdev *sd, const struct
> > > v4l2_priv_tun_config *config); +};
> > > +
> > > +struct v4l2_subdev_audio_ops {
> > > +	int (*s_clock_freq)(struct v4l2_subdev *sd, u32 freq);
> > > +	int (*s_i2s_clock_freq)(struct v4l2_subdev *sd, u32 freq);
> > > +	int (*s_routing)(struct v4l2_subdev *sd, const struct
> > > v4l2_routing *route); +};
> > > +
> > > +struct v4l2_subdev_video_ops {
> > > +	int (*s_routing)(struct v4l2_subdev *sd, const struct
> > > v4l2_routing *route); +	int (*s_crystal_freq)(struct v4l2_subdev
> > > *sd, struct v4l2_crystal_freq *freq); +	int
> > > (*decode_vbi_line)(struct v4l2_subdev *sd, struct
> > > v4l2_decode_vbi_line *vbi_line); +	int (*s_vbi_data)(struct
> > > v4l2_subdev *sd, const struct v4l2_sliced_vbi_data *vbi_data);
> > > +	int (*g_vbi_data)(struct v4l2_subdev *sd, struct
> > > v4l2_sliced_vbi_data *vbi_data); +	int (*s_std_output)(struct
> > > v4l2_subdev *sd, v4l2_std_id std); +	int (*s_stream)(struct
> > > v4l2_subdev *sd, int enable);
> > > +	int (*s_fmt)(struct v4l2_subdev *sd, struct v4l2_format *fmt);
> > > +	int (*g_fmt)(struct v4l2_subdev *sd, struct v4l2_format *fmt);
> > > +};
> > > +
> > > +struct v4l2_subdev_ops {
> > > +	const struct v4l2_subdev_core_ops  *core;
> > > +	const struct v4l2_subdev_tuner_ops *tuner;
> > > +	const struct v4l2_subdev_audio_ops *audio;
> > > +	const struct v4l2_subdev_video_ops *video;
> > > +};
> > > +
> > > +#define V4L2_SUBDEV_NAME_SIZE 32
> > > +
> > > +/* Each instance of a subdev driver should create this struct,
> > > either +   stand-alone or embedded in a larger struct.
> > > + */
> > > +struct v4l2_subdev {
> > > +	struct list_head list;
> > > +	struct module *owner;
> > > +	struct v4l2_device *dev;
> > > +	const struct v4l2_subdev_ops *ops;
> > > +	/* name must be unique */
> > > +	char name[V4L2_SUBDEV_NAME_SIZE];
> > > +	/* can be used to group similar subdevs, value is driver-specific
> > > */ +	u32 grp_id;
> > > +	/* pointer to private data */
> > > +	void *priv;
> > > +};
> > > +
> > > +static inline void v4l2_set_subdevdata(struct v4l2_subdev *sd,
> > > void *p) +{
> > > +	sd->priv = p;
> > > +}
> > > +
> > > +static inline void *v4l2_get_subdevdata(const struct v4l2_subdev
> > > *sd) +{
> > > +	return sd->priv;
> > > +}
> > > +
> > > +/* Convert an ioctl-type command to the proper v4l2_subdev_ops
> > > function call. +   This is used by subdev modules that can be
> > > called by both old-style ioctl +   commands and through the
> > > v4l2_subdev_ops. +
> > > +   The ioctl API of the subdev driver can call this function to
> > > call the +   right ops based on the ioctl cmd and arg.
> > > +
> > > +   Once all subdev drivers have been converted and all drivers no
> > > longer +   use the ioctl interface, then this function can be
> > > removed. + */
> > > +int v4l2_subdev_command(struct v4l2_subdev *sd, unsigned cmd, void
> > > *arg); +
> > > +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);
> > > +	sd->ops = ops;
> > > +	sd->dev = NULL;
> > > +	sd->name[0] = '\0';
> > > +	sd->grp_id = 0;
> > > +	sd->priv = NULL;
> > > +}
> > > +
> > > +/* Call an ops of a v4l2_subdev, doing the right checks against
> > > +   NULL pointers.
> > > +
> > > +   Example: err = v4l2_subdev_call(sd, core, g_chip_ident, &chip);
> > > + */
> > > +#define v4l2_subdev_call(sd, o, f, args...)				\
> > > +	(!(sd) ? -ENODEV : (((sd) && (sd)->ops->o && (sd)->ops->o->f) ?	\
> > > +		(sd)->ops->o->f((sd) , ##args) : -ENOIOCTLCMD))
> > > +
> > > +#endif
> > >
> > > --
> > > video4linux-list mailing list
> > > Unsubscribe
> > > mailto:video4linux-list-request@redhat.com?subject=unsubscribe
> > > https://www.redhat.com/mailman/listinfo/video4linux-list
>
> Once again, thank you for the review. I'll update my repository
> accordingly.
>
> Regards,
>
> 	Hans

Re: v4l2_device/v4l2_subdev: please review (PATCH 1/3)

[Hans Verkuil]

Hi Laurent,

On Sunday 30 November 2008 22:02:21 Laurent Pinchart wrote:
> Hi Hans,
>
> On Saturday 29 November 2008, Hans Verkuil wrote:
> > Hi Laurent,
> >
> > Let me start by thanking you for reviewing this! Much appreciated.
>
> You're welcome.
>
> > On Saturday 29 November 2008 00:34:44 Laurent Pinchart wrote:
> > > Hi Hans,
> > >
> > > On Tuesday 25 November 2008, Hans Verkuil wrote:
> > > > As requested, the patches as separate posts for review.
> > > >
> > > > 	Hans
> > > >
> > > > # HG changeset patch
> > > > # User Hans Verkuil <hverkuil@xs4all.nl>
> > > > # Date 1227560990 -3600
> > > > # Node ID d9ec70c0b0c55e18813f91218c6da6212ca9b7e6
> > > > # Parent b63737bf9eef1ff8494cb7fbc2e818e6aff7a34f
> > > > v4l2: add v4l2_device and v4l2_subdev structs to the v4l2
> > > > framework.
> > > >
> > > > From: Hans Verkuil <hverkuil@xs4all.nl>
> > > >
> > > > Start implementing a proper v4l2 framework as discussed during
> > > > the Linux Plumbers Conference 2008.
> > > >
> > > > Introduces v4l2_device (for device instances) and v4l2_subdev
> > > > (representing sub-device instances).
> > > >
> > > > Priority: normal
> > > >
> > > > Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
> > >
> > > Comments inlined. I've reviewed the general approach only, so
> > > there might be some small issues at the code level that I haven't
> > > noticed.
> > >
> > > Would you mind holding the push request until we're done
> > > discussing the points I mention throughout this e-mail ?
> >
> > Sure, no problem.
> >
> > > > --- a/linux/drivers/media/video/Makefile	Mon Nov 24 10:51:20
> > > > 2008 -0200 +++ b/linux/drivers/media/video/Makefile	Mon Nov 24
> > > > 22:09:50 2008 +0100 @@ -8,7 +8,7 @@
> > > > msp3400-objs	:=	msp3400-driver.o msp3400
> > > >
> > > >  stkwebcam-objs	:=	stk-webcam.o stk-sensor.o
> > > >
> > > > -videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o
> > > > +videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o
> > > > v4l2-subdev.o
> > > >
> > > >  obj-$(CONFIG_VIDEO_DEV) += videodev.o v4l2-compat-ioctl32.o
> > > > v4l2-int-device.o
> > > >
> > > > --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> > > > +++ b/linux/Documentation/video4linux/v4l2-framework.txt	Mon
> > > > Nov 24 22:09:50 2008 +0100 @@ -0,0 +1,360 @@
> > > > +Overview of the V4L2 driver framework
> > > > +=====================================
> > > > +
> > > > +This text documents the various structures provided by the
> > > > V4L2 framework and +their relationships.
> > > > +
> > > > +
> > > > +Introduction
> > > > +------------
> > > > +
> > > > +The V4L2 drivers tend to be very complex due to the complexity
> > > > of the +hardware: most devices have multiple ICs, export
> > > > multiple device nodes in +/dev, and create also non-V4L2
> > > > devices such as DVB, ALSA, FB, I2C and input +(IR) devices.
> > > > +
> > > > +Especially the fact that V4L2 drivers have to setup supporting
> > > > ICs to +do audio/video muxing/encoding/decoding makes it more
> > > > complex than most. +Usually these ICs are connected to the main
> > > > bridge driver through one or +more I2C busses, but other busses
> > > > can also be used. Such devices are +called 'sub-devices'.
> > >
> > > Do you know of other busses being used in (Linux supported) real
> > > video hardware, or is it currently theoretical only ?
> >
> > The pxa_camera driver is one example of that. Also devices driven
> > by GPIO pins can be implemented this way. I did that in ivtv for
> > example: most cards use i2c audio muxers, but some have audio
> > muxers that are commanded through GPIO so I created a v4l2_subdev
> > that uses GPIO to drive these chips. Works very well indeed.
> >
> > > > +For a long time the framework was limited to the video_device
> > > > struct for +creating V4L device nodes and video_buf for
> > > > handling the video buffers +(note that this document does not
> > > > discuss the video_buf framework). +
> > > > +This meant that all drivers had to do the setup of device
> > > > instances and +connecting to sub-devices themselves. Some of
> > > > this is quite complicated +to do right and many drivers never
> > > > did do it correctly.
> > > > +
> > > > +There is also a lot of common code that could never be
> > > > refactored due to +the lack of a framework.
> > > > +
> > > > +So this framework sets up the basic building blocks that all
> > > > drivers +need and this same framework should make it much
> > > > easier to refactor +common code into utility functions shared
> > > > by all drivers. +
> > > > +
> > > > +Structure of a driver
> > > > +---------------------
> > > > +
> > > > +All drivers have the following structure:
> > > > +
> > > > +1) A struct for each device instance containing the device
> > > > state. +
> > > > +2) A way of initializing and commanding sub-devices.
> > >
> > > This only applies to drivers handling "composite devices"
> > > (systems including sub-devices). Let's make sure the proposed
> > > framework doesn't make "simple devices" more complex to handle
> > > that they are now.
> >
> > I will make a note of this.
> >
> > > > +3) Creating V4L2 device nodes (/dev/videoX, /dev/vbiX,
> > > > /dev/radioX and +   /dev/vtxX) and keeping track of device-node
> > > > specific data. +
> > > > +4) Filehandle-specific structs containing per-filehandle data.
> > > > +
> > > > +This is a rough schematic of how it all relates:
> > > > +
> > > > +    device instances
> > > > +      |
> > > > +      +-sub-device instances
> > > > +      |
> > > > +      \-V4L2 device nodes
> > > > +	  |
> > > > +	  \-filehandle instances
> > > > +
> > > > +
> > > > +Structure of the framework
> > > > +--------------------------
> > > > +
> > > > +The framework closely resembles the driver structure: it has a
> > > > v4l2_device +struct for the device instance data, a v4l2_subdev
> > > > struct to refer to +sub-device instances, the video_device
> > > > struct stores V4L2 device node data +and a v4l2_fh struct keeps
> > > > track of filehandle instances (TODO: not yet +implemented).
> > > > +
> > > > +
> > > > +struct v4l2_device
> > > > +------------------
> > > > +
> > > > +Each device instance is represented by a struct v4l2_device
> > > > (v4l2-device.h). +Very simple devices can just allocate this
> > > > struct, but most of the time you +would embed this struct
> > > > inside a larger struct. +
> > > > +You must register the device instance:
> > > > +
> > > > +	v4l2_device_register(struct device *dev, struct v4l2_device
> > > > *v4l2_dev); +
> > > > +Registration will initialize the v4l2_device struct and link
> > > > dev->platform_data +to v4l2_dev.
> > >
> > > That's an abuse of platform_data, I don't think it was ever meant
> > > that way.
> >
> > I took another look at this, and dev_set_drvdata() is a better
> > solution. I'll change it.
> >
> > > > Registration will also set v4l2_dev->name
> > > > to a value derived from +dev (driver name followed by the
> > > > bus_id, to be precise). You may change the +name after
> > > > registration if you want. +
> > > > +You unregister with:
> > > > +
> > > > +	v4l2_device_unregister(struct v4l2_device *v4l2_dev);
> > > > +
> > > > +Unregistering will also automatically unregister all subdevs
> > > > from the device. +
> > > > +Sometimes you need to iterate over all devices registered by a
> > > > specific +driver. This is usually the case if multiple device
> > > > drivers use the same +hardware. E.g. the ivtvfb driver is a
> > > > framebuffer driver that uses the ivtv +hardware. The same is
> > > > true for alsa drivers for example. +
> > > > +You can iterate over all registered devices as follows:
> > > > +
> > > > +static int callback(struct device *dev, void *p)
> > > > +{
> > > > +	struct v4l2_device *v4l2_dev = dev->platform_data;
> > > > +
> > > > +	/* test if this device was inited */
> > > > +	if (v4l2_dev == NULL)
> > > > +		return 0;
> > > > +	...
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +int iterate(void *p)
> > > > +{
> > > > +	struct device_driver *drv;
> > > > +	int err;
> > > > +
> > > > +	/* Find driver 'ivtv' on the PCI bus.
> > > > +	   pci_bus_type is a global. For USB busses use usb_bus_type.
> > > > */ +	drv = driver_find("ivtv", &pci_bus_type);
> > > > +	/* iterate over all ivtv device instances */
> > > > +	err = driver_for_each_device(drv, NULL, p, callback);
> > > > +	put_driver(drv);
> > > > +	return err;
> > > > +}
> > >
> > > I'm not sure to really see what use cases you're talking about.
> > > The above code looks good, but iterating over device bound to
> > > specific drivers should be done with care as it might be the sign
> > > of badly designed code. Every new instance of the above code
> > > should be reviewed with care.
> >
> > This is only relevant when you have two separate drivers that both
> > use the same hardware. This is the case for e.g. ivtv and ivtvfb
> > (the framebuffer driver) and for e.g. cx88 and cx88-alsa (the alsa
> > driver). In both cases the second driver needs to find the core
> > data structures of the main driver. Right now this is implemented
> > by having lists of device instances in the main driver which the
> > secondary driver walks to find the devices. However, this
> > information is already present in the kernel so rather than
> > duplicating this information it is much better to use the kernel
> > API to get hold of it. I put this code in the documentation since
> > it not trivial to figure this out, which is probably the reason
> > everyone keeps their own list.
>
> That's a very interesting use case that might benefit other
> subsystems as well. I've had a quick look at the cx88 driver(s), it
> seems cx88 hardware expose separate video and audio PCI functions. Is
> this right ? In that case the two drivers (cx88 and cx88-alsa) bind
> to two separate devices. Do you know if other subsystems need to
> share driver data between separate devices ? If the cx88-alsa driver
> needs data from the "main" driver, how do you handle the cx88-alsa
> module being loaded before cx88 ?

Loading the cx88-alsa driver will force a load of cx88 as well since 
cx88-alsa uses exported cx88 functions. An alternative would be for the 
cx88-alsa module to load the cx88 module explicitly. Note that I am no 
authority on the cx88 driver, so I do not really know if this method 
would work equally well with cx88. I suspect it does, though.

> As for the ivtv driver, this is quite different. I suppose the ivtvfb
> driver wouldn't need to walk the devices list if it wasn't a separate
> module, right ?

That's correct.

>
> > > > +Sometimes you need to keep a running counter of the device
> > > > instance. This is +commonly used to map a device instance to an
> > > > index of a module option array. +
> > > > +The recommended approach is as follows:
> > > > +
> > > > +static atomic_t drv_instance = ATOMIC_INIT(0);
> > > > +
> > > > +static int __devinit drv_probe(struct pci_dev *dev,
> > > > +				const struct pci_device_id *pci_id)
> > > > +{
> > > > +	...
> > > > +	state->instance = atomic_inc_return(&drv_instance) - 1;
> > > > +}
> > > > +
> > > > +
> > > > +struct v4l2_subdev
> > > > +------------------
> > > > +
> > > > +Many drivers need to communicate with sub-devices. These
> > > > devices can do all +sort of tasks, but most commonly they
> > > > handle audio and/or video muxing, +encoding or decoding. For
> > > > webcams common sub-devices are sensors and camera +controllers.
> > > > +
> > > > +Usually these are I2C devices, but not necessarily. In order
> > > > to provide the +driver with a consistent interface to these
> > > > sub-devices the v4l2_subdev struct +(v4l2-subdev.h) was
> > > > created. +
> > > > +Each sub-device driver must have a v4l2_subdev struct. This
> > > > struct can be +stand-alone for simple sub-devices or it might
> > > > be embedded in a larger struct +if more state information needs
> > > > to be stored. Usually there is a low-level +device struct (e.g.
> > > > i2c_client) that contains the device data as setup +by the
> > > > kernel. It is recommended to store that pointer in the private
> > > > +data of v4l2_subdev using v4l2_set_subdevdata(). That makes it
> > > > easy to go +from a v4l2_subdev to the actual low-level
> > > > bus-specific device data. +
> > > > +You also need a way to go from the low-level struct to
> > > > v4l2_subdev. For the +common i2c_client struct the
> > > > i2c_set_clientdata() call is used to store a +v4l2_subdev
> > > > pointer, for other busses you may have to use other methods. +
> > > > +From the bridge driver perspective you load the sub-device
> > > > module and somehow +obtain the v4l2_subdev pointer. For i2c
> > > > devices this is easy: you call +i2c_get_clientdata(). For other
> > > > busses something similar needs to be done. +Helper functions
> > > > exists for sub-devices on an I2C bus that do most of this
> > > > +tricky work for you. +
> > > > +Each v4l2_subdev contains function pointers that sub-device
> > > > drivers can +implement (or leave NULL if it is not applicable).
> > > > Since sub-devices can do +so many different things and you do
> > > > not want to end up with a huge ops struct +of which only a
> > > > handful of ops are commonly implemented, the function pointers
> > > > +are sorted according to category and each category has its own
> > > > ops struct. +
> > >
> > > I like that.
> >
> > Thank you :-)
> >
> > I've done my best to sort everything into sensible categories, but
> > once all i2c drivers are converted I'll probably do another review
> > of this and move some ops around if appropriate.
>
> I haven't double-checked each operation, but that doesn't have to be
> sorted now.
>
> > > > +The top-level ops struct contains pointers to the category ops
> > > > structs, which +may be NULL if the subdev driver does not
> > > > support anything from that category. +
> > > > +It looks like this:
> > > > +
> > > > +struct v4l2_subdev_core_ops {
> > > > +	int (*g_chip_ident)(struct v4l2_subdev *sd, struct
> > > > v4l2_chip_ident *chip); +	int (*log_status)(struct v4l2_subdev
> > > > *sd);
> > > > +	int (*init)(struct v4l2_subdev *sd, u32 val);
> > > > +	...
> > > > +};
> > > > +
> > > > +struct v4l2_subdev_tuner_ops {
> > > > +	...
> > > > +};
> > > > +
> > > > +struct v4l2_subdev_audio_ops {
> > > > +	...
> > > > +};
> > > > +
> > > > +struct v4l2_subdev_video_ops {
> > > > +	...
> > > > +};
> > > > +
> > > > +struct v4l2_subdev_ops {
> > > > +	const struct v4l2_subdev_core_ops  *core;
> > > > +	const struct v4l2_subdev_tuner_ops *tuner;
> > > > +	const struct v4l2_subdev_audio_ops *audio;
> > > > +	const struct v4l2_subdev_video_ops *video;
> > > > +};
> > > > +
> > > > +The core ops are common to all subdevs, the other categories
> > > > are implemented +depending on the sub-device. E.g. a video
> > > > device is unlikely to support the +audio ops and vice versa.
> > > > +
> > > > +This setup limits the number of function pointers while still
> > > > making it easy +to add new ops and categories.
> > > > +
> > > > +A sub-device driver initializes the v4l2_subdev struct using:
> > > > +
> > > > +	v4l2_subdev_init(subdev, &ops);
> > > > +
> > > > +Afterwards you need to initialize subdev->name with a unique
> > > > name and set the +module owner. This is done for you if you use
> > > > the i2c helper functions. +
> > > > +A device (bridge) driver needs to register the v4l2_subdev
> > > > with the +v4l2_device:
> > > > +
> > > > +	int err = v4l2_device_register_subdev(device, subdev);
> > > > +
> > > > +This can fail if the subdev module disappeared before it could
> > > > be registered. +After this function was called successfully the
> > > > subdev->dev field points to +the v4l2_device.
> > > > +
> > > > +You can unregister a sub-device using:
> > > > +
> > > > +	v4l2_device_unregister_subdev(subdev);
> > > > +
> > > > +Afterwards the subdev module can be unloaded and subdev->dev
> > > > == NULL. +
> > > > +You can call an ops function either directly:
> > > > +
> > > > +	err = subdev->ops->core->g_chip_ident(subdev, &chip);
> > > > +
> > > > +but it is better and easier to use this macro:
> > > > +
> > > > +	err = v4l2_subdev_call(subdev, core, g_chip_ident, &chip);
> > > > +
> > > > +The macro will to the right NULL pointer checks and returns
> > > > -ENODEV if subdev +is NULL,
> > >
> > > This should probably be checked when registering the sub-device
> > > instead of at all calls to sub-device operations.
> >
> > That was my initial approach as well, but it is actually quite
> > handy to have. For example in the ivtv driver there are some cards
> > that can do video output. What I can do with this is to use a
> > v4l2_subdev pointer which is NULL if the card doesn't support this
> > feature. At the moment I have to check for this feature every time.
> > The overhead is minimal, but it makes life easier and it allows for
> > cleaner driver code.
>
> If the card can't do video output the driver should probably block
> video output operations at a higher lever, before reaching calls to
> the sub-device. I'm not concerned much about performances here (calls
> to sub-devices are probably not performance critical) but more about
> code cleanness. It's quite easy to remove important checks if you
> know that lower level code will probably catch your mistakes.

In my view being able to handle a NULL v4l2_subdev pointer is similar to 
allowing kfree(NULL). Yes, you could handle it at a higher level, but 
this saves you a lot of extra tests which are often not needed.

> > Actually this idea came from Sakari Ailus and his v4l2-int-device
> > work.
> >
> > > > -ENOIOCTLCMD if either subdev->core or
> > > > subdev->core->g_chip_ident is +NULL, or the actual result of
> > > > the subdev->ops->core->g_chip_ident ops. +
> > > > +It is also possible to call all or a subset of the
> > > > sub-devices: +
> > > > +	v4l2_device_call_all(dev, 0, core, g_chip_ident, &chip);
> > > > +
> > > > +Any subdev that does not support this ops is skipped and error
> > > > results are +ignored. If you want to check for errors use this:
> > > > +
> > > > +	err = v4l2_device_call_until_err(dev, 0, core, g_chip_ident,
> > > > &chip); +
> > > > +Any error except -ENOIOCTLCMD will exit the loop with that
> > > > error. If no +errors (except -ENOIOCTLCMD) occured, then 0 is
> > > > returned. + +The second argument to both calls is a group ID.
> > > > If 0, then all subdevs are +called. If non-zero, then only
> > > > those whose group ID match that value will +be called. Before a
> > > > bridge driver registers a subdev it can set subdev->grp_id +to
> > > > whatever value it wants (it's 0 by default). This value is
> > > > owned by the +bridge driver and the sub-device driver will
> > > > never modify or use it. +
> > > > +The group ID gives the bridge driver more control how
> > > > callbacks are called. +For example, there may be multiple audio
> > > > chips on a board, each capable of +changing the volume. But
> > > > usually only one will actually be used when the +user want to
> > > > change the volume. You can set the group ID for that subdev to
> > > > +e.g. AUDIO_CONTROLLER and specify that as the group ID value
> > > > when calling
> > > > +v4l2_device_call_all(). That ensures that it will only go to
> > > > the subdev +that needs it.
> > >
> > > This is interesting as well.
> >
> > Thanks to the brainstorm we had at the Linux Plumbers Conference. I
> > think it was either Steve Toth or Mike Krufky who came up with this
> > idea.
> >
> > > > +The advantage of using v4l2_subdev is that it is a generic
> > > > struct and does +not contain any knowledge about the underlying
> > > > hardware. So a driver might +contain several subdevs that use
> > > > an I2C bus, but also a subdev that is +controlled through GPIO
> > > > pins. This distinction is only relevant when setting +up the
> > > > device, but once the subdev is registered it is completely
> > > > transparent. +
> > >
> > > The bridge driver won't have to care about how sub-devices are
> > > accessed, but sub-devices drivers will have to be written
> > > specifically for the V4L2 subsystem. Do we have I2C devices
> > > shared between V4L(2) and DVB (and possible other subsystems) ?
> > > How would those be handled ? If your goal is to share v4l2_subdev
> > > drivers between V4L(2) and DVB I don't think the v4l2_ prefix is
> > > right.
> >
> > I've gone back and forth between a v4l2_ and a media_ prefix a few
> > times now and I've settled on v4l2_ (for now at least). First of
> > all, at the moment it is a purely v4l2 interface. Secondly, I'm not
> > entirely sure what sort of modifications I need to make to make it
> > possible for DVB to start using this. It's a separate (but
> > interesting) project and based on results of that it might well
> > turn out that the prefix will be changed to something like media_.
> >
> > However, starting to use the media_ prefix at this stage suggests
> > that it is something that it definitely isn't at the moment.
>
> Ok, so your proposal only targets video input devices (with optional
> framebuffer, sound or hid support) that implement the v4l2 interfaces
> and that use sub-devices, right ?

Yes. That's by far the largest user of sub-devices. DVB is of course an 
obvious future candidate due to the overlap with v4l2.

> > > > +I2C sub-device drivers
> > > > +----------------------
> > > > +
> > > > +Since these drivers are so common, special helper functions
> > > > are available to +ease the use of these drivers
> > > > (v4l2-common.h). +
> > > > +The recommended method of adding v4l2_subdev support to an I2C
> > > > driver is to +embed the v4l2_subdev struct into the state
> > > > struct that is created for each +I2C device instance. Very
> > > > simple devices have no state struct and in that case +you can
> > > > just create a v4l2_subdev directly.
> > >
> > > The only I2C video-related chips I've dealt with were the ones
> > > used by DC10/DC10+ boards and those have no state struct. What is
> > > the state struct in your description ? Is it the driver-specific
> > > data allocated when the device is probed ?
> >
> > Hmm, I need to clarify this: most i2c drivers (but not all) have a
> > struct containing state information. The exact contents depends of
> > course on the driver.
> >
> > > > +Initialize the v4l2_subdev struct as follows:
> > > > +
> > > > +	v4l2_i2c_subdev_init(&state->sd, client, subdev_ops);
> > > > +
> > > > +This function will fill in all the fields of v4l2_subdev and
> > > > ensure that the +v4l2_subdev and i2c_client both point to one
> > > > another. +
> > > > +You should also add a helper inline function to go from a
> > > > v4l2_subdev pointer +to the state struct:
> > > > +
> > > > +static inline struct subdev_state *to_state(struct v4l2_subdev
> > > > *sd) +{
> > > > +	return container_of(sd, struct subdev_state, sd);
> > > > +}
> > >
> > > <chipname>_state might be a better name, it would avoid namespace
> > > clashes (I'm aware the state structure is supposed to be private
> > > to the I2C device driver, but we never know what might happen).
> >
> > OK.
> >
> > > > +Use this to go from the v4l2_subdev struct to the i2c_client
> > > > struct: +
> > > > +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> > > > +
> > > > +And this to go from an i2c_client to a v4l2_subdev struct:
> > > > +
> > > > +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > > > +
> > > > +Finally you need to make a command function to make
> > > > driver->command() +call the right subdev_ops functions:
> > > > +
> > > > +static int subdev_command(struct i2c_client *client, unsigned
> > > > cmd, void *arg) +{
> > > > +	return v4l2_subdev_command(i2c_get_clientdata(client), cmd,
> > > > arg); +}
> > > > +
> > > > +If driver->command is never used then you can leave this out.
> > > > Eventually the +driver->command usage should be removed from
> > > > v4l.
> > >
> > > Should it then be added to the list of features scheduled for
> > > removal ?
> >
> > No, driver->command is part of the i2c subsystem and we are not the
> > only users, so it can't be scheduled for removal. But it is
> > probably possible to add some check on this command callback at
> > some low-level v4l2 core function to ensure it isn't used. But this
> > is quite some time in the future for now.
>
> Ok.
>
> > > > +Make sure to call v4l2_device_unregister_subdev(sd) when the
> > > > remove() callback +is called. This will unregister the
> > > > sub-device from the bridge driver. It is +safe to call this
> > > > even if the sub-device was never registered.
> > > > +
> > > > +
> > > > +The bridge driver also has some helper functions it can use:
> > > > +
> > > > +struct v4l2_subdev *sd = v4l2_i2c_new_subdev(adapter,
> > > > "module_foo", "chipid", 0x36); +
> > > > +This loads the given module (can be NULL if no module needs to
> > > > be loaded) and +calls i2c_new_device() with the given
> > > > i2c_adapter and chip/address arguments. +If all goes well, then
> > > > it registers the subdev with the v4l2_device. It gets +the
> > > > v4l2_device by calling i2c_get_adapdata(adapter), so you should
> > > > make sure +that adapdata is set to v4l2_device when you setup
> > > > the i2c_adapter in your +driver.
> > > > +
> > > > +You can also use v4l2_i2c_new_probed_subdev() which is very
> > > > similar to +v4l2_i2c_new_subdev(), except that it has an array
> > > > of possible I2C addresses +that it should probe. Internally it
> > > > calls i2c_new_probed_device(). +
> > > > +Both functions return NULL if something went wrong.
> > > > +
> > > > +
> > > > +struct video_device
> > > > +-------------------
> > > > +
> > > > +TODO: document.
> > >
> > > Do you plan to change the video_device structure ?
> >
> > I want to add a v4l2_device pointer to it, but that's all for now.
>
> Just to make things clear, v4l2_device is the parent structure, right
> ?

Yes. Currently there is no link back from the video_device to the 
v4l2_device and that should of course be added.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: v4l2_device/v4l2_subdev: please review (PATCH 2/3)

[Hans Verkuil]

# HG changeset patch
# User Hans Verkuil <hverkuil@xs4all.nl>
# Date 1227453585 -3600
# Node ID aaf944b194f95c0cc47603c1fc5105cca073c7db
# Parent d9ec70c0b0c55e18813f91218c6da6212ca9b7e6
v4l2-common: add i2c helper functions

From: Hans Verkuil <hverkuil@xs4all.nl>

Add helper functions to load i2c sub-devices, integrating them
into the v4l2-framework.

Priority: normal

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

--- a/linux/drivers/media/video/v4l2-common.c	Mon Nov 24 22:09:50 2008 
+0100
+++ b/linux/drivers/media/video/v4l2-common.c	Sun Nov 23 16:19:45 2008 
+0100
@@ -58,6 +58,7 @@
 #include <asm/div64.h>
 #define __OLD_VIDIOC_ /* To allow fixing old calls*/
 #include <media/v4l2-common.h>
+#include <media/v4l2-device.h>
 #include <media/v4l2-chip-ident.h>
 
 #include <linux/videodev2.h>
@@ -802,4 +803,173 @@ int v4l2_i2c_attach(struct i2c_adapter *
 	return err != -ENOMEM ? 0 : err;
 }
 EXPORT_SYMBOL(v4l2_i2c_attach);
-#endif
+
+void v4l2_i2c_subdev_init(struct v4l2_subdev *sd, struct i2c_client 
*client,
+		const struct v4l2_subdev_ops *ops)
+{
+	v4l2_subdev_init(sd, ops);
+	/* the owner is the same as the i2c_client's driver owner */
+	sd->owner = client->driver->driver.owner;
+	/* i2c_client and v4l2_subdev point to one another */
+	v4l2_set_subdevdata(sd, client);
+	i2c_set_clientdata(client, sd);
+	/* initialize name */
+	snprintf(sd->name, sizeof(sd->name), "%s %d-%04x",
+		client->driver->driver.name, i2c_adapter_id(client->adapter),
+		client->addr);
+}
+EXPORT_SYMBOL(v4l2_i2c_subdev_init);
+
+#if LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 22)
+/* Supporting function to find a client on a specific address on the
+   given adapter. Used for legacy i2c drivers. */
+static struct i2c_client *v4l2_i2c_legacy_find_client(struct 
i2c_adapter *adap, u8 addr)
+{
+	struct i2c_client *result = NULL;
+	struct i2c_client *client;
+	struct list_head  *item;
+
+#if LINUX_VERSION_CODE <= KERNEL_VERSION(2, 6, 16)
+	down(&adap->clist_lock);
+#else
+	mutex_lock(&adap->clist_lock);
+#endif
+	list_for_each(item, &adap->clients) {
+		client = list_entry(item, struct i2c_client, list);
+		if (client->addr == addr) {
+			result = client;
+			break;
+		}
+	}
+#if LINUX_VERSION_CODE <= KERNEL_VERSION(2, 6, 16)
+	up(&adap->clist_lock);
+#else
+	mutex_unlock(&adap->clist_lock);
+#endif
+	return result;
+}
+#endif
+
+
+/* Load an i2c sub-device. It assumes that i2c_get_adapdata(adapter)
+   returns the v4l2_device and that i2c_get_clientdata(client)
+   returns the v4l2_subdev. */
+struct v4l2_subdev *v4l2_i2c_new_subdev(struct i2c_adapter *adapter,
+		const char *module_name, const char *client_type, u8 addr)
+{
+	struct v4l2_device *dev = i2c_get_adapdata(adapter);
+	struct v4l2_subdev *sd = NULL;
+	struct i2c_client *client;
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 22)
+	struct i2c_board_info info;
+#endif
+
+	BUG_ON(!dev);
+#ifdef MODULE
+	if (module_name)
+		request_module(module_name);
+#endif
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 22)
+	/* Setup the i2c board info with the device type and
+	   the device address. */
+	memset(&info, 0, sizeof(info));
+#if LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 26)
+	strlcpy(info.driver_name, client_type, sizeof(info.driver_name));
+#else
+	strlcpy(info.type, client_type, sizeof(info.type));
+#endif
+	info.addr = addr;
+
+	/* Create the i2c client */
+	client = i2c_new_device(adapter, &info);
+#else
+	/* Legacy code: loading the module automatically
+	   probes and creates the i2c_client on the adapter.
+	   Try to find the client by walking the adapter's client list. */
+	client = v4l2_i2c_legacy_find_client(adapter, addr);
+#endif
+	/* Note: it is possible in the future that
+	   c->driver is NULL if the driver is still being loaded.
+	   We need better support from the kernel so that we
+	   can easily wait for the load to finish. */
+	if (client == NULL || client->driver == NULL)
+		return NULL;
+
+	/* Lock the module so we can safely get the v4l2_subdev pointer */
+	if (!try_module_get(client->driver->driver.owner))
+		return NULL;
+	sd = i2c_get_clientdata(client);
+
+	/* Register with the v4l2_device which increases the module's
+	   use count as well. */
+	if (v4l2_device_register_subdev(dev, sd))
+		sd = NULL;
+	/* Decrease the module use count to match the first try_module_get. */
+	module_put(client->driver->driver.owner);
+	return sd;
+
+}
+EXPORT_SYMBOL(v4l2_i2c_new_subdev);
+
+/* Probe and load an i2c sub-device. It assumes that 
i2c_get_adapdata(adapter)
+   returns the v4l2_device and that i2c_get_clientdata(client)
+   returns the v4l2_subdev. */
+struct v4l2_subdev *v4l2_i2c_new_probed_subdev(struct i2c_adapter 
*adapter,
+	const char *module_name, const char *client_type,
+	const unsigned short *addrs)
+{
+	struct v4l2_device *dev = i2c_get_adapdata(adapter);
+	struct v4l2_subdev *sd = NULL;
+	struct i2c_client *client = NULL;
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 22)
+	struct i2c_board_info info;
+#endif
+
+	BUG_ON(!dev);
+#ifdef MODULE
+	if (module_name)
+		request_module(module_name);
+#endif
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 22)
+	/* Setup the i2c board info with the device type and
+	   the device address. */
+	memset(&info, 0, sizeof(info));
+#if LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 26)
+	strlcpy(info.driver_name, client_type, sizeof(info.driver_name));
+#else
+	strlcpy(info.type, client_type, sizeof(info.type));
+#endif
+
+	/* Probe and create the i2c client */
+	client = i2c_new_probed_device(adapter, &info, addrs);
+#else
+	/* Legacy code: loading the module should automatically
+	   probe and create the i2c_client on the adapter.
+	   Try to find the client by walking the adapter's client list
+	   for each of the possible addresses. */
+	while (!client && *addrs != I2C_CLIENT_END)
+		client = v4l2_i2c_legacy_find_client(adapter, *addrs++);
+#endif
+	/* Note: it is possible in the future that
+	   c->driver is NULL if the driver is still being loaded.
+	   We need better support from the kernel so that we
+	   can easily wait for the load to finish. */
+	if (client == NULL || client->driver == NULL)
+		return NULL;
+
+	/* Lock the module so we can safely get the v4l2_subdev pointer */
+	if (!try_module_get(client->driver->driver.owner))
+		return NULL;
+	sd = i2c_get_clientdata(client);
+
+	/* Register with the v4l2_device which increases the module's
+	   use count as well. */
+	if (v4l2_device_register_subdev(dev, sd))
+		sd = NULL;
+	/* Decrease the module use count to match the first try_module_get. */
+	module_put(client->driver->driver.owner);
+	return sd;
+}
+EXPORT_SYMBOL(v4l2_i2c_new_probed_subdev);
+
+#endif
--- a/linux/include/media/v4l2-common.h	Mon Nov 24 22:09:50 2008 +0100
+++ b/linux/include/media/v4l2-common.h	Sun Nov 23 16:19:45 2008 +0100
@@ -53,6 +53,29 @@
 	do { 								     \
 		if (debug >= (level))					     \
 			v4l_client_printk(KERN_DEBUG, client, fmt , ## arg); \
+	} while (0)
+
+/* ------------------------------------------------------------------------- 
*/
+
+/* These printk constructs can be used with v4l2_device and v4l2_subdev 
*/
+#define v4l2_printk(level, dev, fmt, arg...) \
+	printk(level "%s: " fmt, (dev)->name , ## arg)
+
+#define v4l2_err(dev, fmt, arg...) \
+	v4l2_printk(KERN_ERR, dev, fmt , ## arg)
+
+#define v4l2_warn(dev, fmt, arg...) \
+	v4l2_printk(KERN_WARNING, dev, fmt , ## arg)
+
+#define v4l2_info(dev, fmt, arg...) \
+	v4l2_printk(KERN_INFO, dev, fmt , ## arg)
+
+/* These three macros assume that the debug level is set with a module
+   parameter called 'debug'. */
+#define v4l2_dbg(level, debug, dev, fmt, arg...)			\
+	do { 								\
+		if (debug >= (level))					\
+			v4l2_printk(KERN_DEBUG, dev, fmt , ## arg); 	\
 	} while (0)
 
 /* ------------------------------------------------------------------------- 
*/
@@ -104,10 +127,28 @@ struct i2c_adapter;
 struct i2c_adapter;
 struct i2c_client;
 struct i2c_device_id;
+struct v4l2_device;
+struct v4l2_subdev;
+struct v4l2_subdev_ops;
 
 int v4l2_i2c_attach(struct i2c_adapter *adapter, int address, struct 
i2c_driver *driver,
 		const char *name,
 		int (*probe)(struct i2c_client *, const struct i2c_device_id *));
+
+/* 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(struct i2c_adapter *adapter,
+		const char *module_name, const char *client_type, u8 addr);
+/* Probe and 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_probed_subdev(struct i2c_adapter 
*adapter,
+		const char *module_name, const char *client_type,
+		const unsigned short *addrs);
+/* Initialize an v4l2_subdev with data from an i2c_client struct */
+void v4l2_i2c_subdev_init(struct v4l2_subdev *sd, struct i2c_client 
*client,
+		const struct v4l2_subdev_ops *ops);
 
 /* ------------------------------------------------------------------------- 
*/
 

Re: v4l2_device/v4l2_subdev: please review (PATCH 3/3)

[Hans Verkuil]

# HG changeset patch
# User Hans Verkuil <hverkuil@xs4all.nl>
# Date 1227561257 -3600
# Node ID d5c3c3f0b53c549b53d9596c9a7e827ec7521c57
# Parent 3a957c63323e35f5862259814b728d4c08584963
cx25840: convert to v4l2_subdev.

From: Hans Verkuil <hverkuil@xs4all.nl>

Priority: normal

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

--- a/linux/drivers/media/video/cx25840/cx25840-audio.c	Mon Nov 24 
22:12:55 2008 +0100
+++ b/linux/drivers/media/video/cx25840/cx25840-audio.c	Mon Nov 24 
22:14:17 2008 +0100
@@ -26,7 +26,7 @@
 
 static int set_audclk_freq(struct i2c_client *client, u32 freq)
 {
-	struct cx25840_state *state = i2c_get_clientdata(client);
+	struct subdev_state *state = to_state(i2c_get_clientdata(client));
 
 	if (freq != 32000 && freq != 44100 && freq != 48000)
 		return -EINVAL;
@@ -194,7 +194,7 @@ static int set_audclk_freq(struct i2c_cl
 
 void cx25840_audio_set_path(struct i2c_client *client)
 {
-	struct cx25840_state *state = i2c_get_clientdata(client);
+	struct subdev_state *state = to_state(i2c_get_clientdata(client));
 
 	/* assert soft reset */
 	cx25840_and_or(client, 0x810, ~0x1, 0x01);
@@ -236,7 +236,7 @@ void cx25840_audio_set_path(struct i2c_c
 
 static int get_volume(struct i2c_client *client)
 {
-	struct cx25840_state *state = i2c_get_clientdata(client);
+	struct subdev_state *state = to_state(i2c_get_clientdata(client));
 	int vol;
 
 	if (state->unmute_volume >= 0)
@@ -253,7 +253,7 @@ static int get_volume(struct i2c_client 
 
 static void set_volume(struct i2c_client *client, int volume)
 {
-	struct cx25840_state *state = i2c_get_clientdata(client);
+	struct subdev_state *state = to_state(i2c_get_clientdata(client));
 	int vol;
 
 	if (state->unmute_volume >= 0) {
@@ -341,14 +341,14 @@ static void set_balance(struct i2c_clien
 
 static int get_mute(struct i2c_client *client)
 {
-	struct cx25840_state *state = i2c_get_clientdata(client);
+	struct subdev_state *state = to_state(i2c_get_clientdata(client));
 
 	return state->unmute_volume >= 0;
 }
 
 static void set_mute(struct i2c_client *client, int mute)
 {
-	struct cx25840_state *state = i2c_get_clientdata(client);
+	struct subdev_state *state = to_state(i2c_get_clientdata(client));
 
 	if (mute && state->unmute_volume == -1) {
 		int vol = get_volume(client);
@@ -366,7 +366,7 @@ static void set_mute(struct i2c_client *
 
 int cx25840_audio(struct i2c_client *client, unsigned int cmd, void 
*arg)
 {
-	struct cx25840_state *state = i2c_get_clientdata(client);
+	struct subdev_state *state = to_state(i2c_get_clientdata(client));
 	struct v4l2_control *ctrl = arg;
 	int retval;
 
--- a/linux/drivers/media/video/cx25840/cx25840-core.c	Mon Nov 24 
22:12:55 2008 +0100
+++ b/linux/drivers/media/video/cx25840/cx25840-core.c	Mon Nov 24 
22:14:17 2008 +0100
@@ -185,11 +185,11 @@ static void cx25836_initialize(struct i2
 #if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 20)
 static void cx25840_work_handler(struct work_struct *work)
 {
-	struct cx25840_state *state = container_of(work, struct cx25840_state, 
fw_work);
+	struct subdev_state *state = container_of(work, struct subdev_state, 
fw_work);
 #else
 void cx25840_work_handler(void *arg)
 {
-	struct cx25840_state *state = arg;
+	struct subdev_state *state = arg;
 #endif
 	cx25840_loadfw(state->c);
 	wake_up(&state->fw_wait);
@@ -198,7 +198,7 @@ static void cx25840_initialize(struct i2
 static void cx25840_initialize(struct i2c_client *client)
 {
 	DEFINE_WAIT(wait);
-	struct cx25840_state *state = i2c_get_clientdata(client);
+	struct subdev_state *state = to_state(i2c_get_clientdata(client));
 	struct workqueue_struct *q;
 
 	/* datasheet startup in numbered steps, refer to page 3-77 */
@@ -270,7 +270,7 @@ static void cx23885_initialize(struct i2
 static void cx23885_initialize(struct i2c_client *client)
 {
 	DEFINE_WAIT(wait);
-	struct cx25840_state *state = i2c_get_clientdata(client);
+	struct subdev_state *state = to_state(i2c_get_clientdata(client));
 	struct workqueue_struct *q;
 
 	/* Internal Reset */
@@ -369,7 +369,7 @@ static void cx23885_initialize(struct i2
 
 void cx25840_std_setup(struct i2c_client *client)
 {
-	struct cx25840_state *state = i2c_get_clientdata(client);
+	struct subdev_state *state = to_state(i2c_get_clientdata(client));
 	v4l2_std_id std = state->std;
 	int hblank, hactive, burst, vblank, vactive, sc;
 	int vblank656, src_decimation;
@@ -516,7 +516,7 @@ void cx25840_std_setup(struct i2c_client
 
 static void input_change(struct i2c_client *client)
 {
-	struct cx25840_state *state = i2c_get_clientdata(client);
+	struct subdev_state *state = to_state(i2c_get_clientdata(client));
 	v4l2_std_id std = state->std;
 
 	/* Follow step 8c and 8d of section 3.16 in the cx25840 datasheet */
@@ -570,7 +570,7 @@ static int set_input(struct i2c_client *
 static int set_input(struct i2c_client *client, enum 
cx25840_video_input vid_input,
 						enum cx25840_audio_input aud_input)
 {
-	struct cx25840_state *state = i2c_get_clientdata(client);
+	struct subdev_state *state = to_state(i2c_get_clientdata(client));
 	u8 is_composite = (vid_input >= CX25840_COMPOSITE1 &&
 			   vid_input <= CX25840_COMPOSITE8);
 	u8 reg;
@@ -690,7 +690,7 @@ static int set_input(struct i2c_client *
 
 static int set_v4lstd(struct i2c_client *client)
 {
-	struct cx25840_state *state = i2c_get_clientdata(client);
+	struct subdev_state *state = to_state(i2c_get_clientdata(client));
 	u8 fmt = 0; 	/* zero is autodetect */
 	u8 pal_m = 0;
 
@@ -739,9 +739,10 @@ static int set_v4lstd(struct i2c_client 
 
 /* ----------------------------------------------------------------------- 
*/
 
-static int set_v4lctrl(struct i2c_client *client, struct v4l2_control 
*ctrl)
-{
-	struct cx25840_state *state = i2c_get_clientdata(client);
+static int subdev_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control 
*ctrl)
+{
+	struct subdev_state *state = to_state(sd);
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
 
 	switch (ctrl->id) {
 	case CX25840_CID_ENABLE_PVR150_WORKAROUND:
@@ -805,9 +806,10 @@ static int set_v4lctrl(struct i2c_client
 	return 0;
 }
 
-static int get_v4lctrl(struct i2c_client *client, struct v4l2_control 
*ctrl)
-{
-	struct cx25840_state *state = i2c_get_clientdata(client);
+static int subdev_g_ctrl(struct v4l2_subdev *sd, struct v4l2_control 
*ctrl)
+{
+	struct subdev_state *state = to_state(sd);
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
 
 	switch (ctrl->id) {
 	case CX25840_CID_ENABLE_PVR150_WORKAROUND:
@@ -842,21 +844,23 @@ static int get_v4lctrl(struct i2c_client
 
 /* ----------------------------------------------------------------------- 
*/
 
-static int get_v4lfmt(struct i2c_client *client, struct v4l2_format 
*fmt)
-{
+static int subdev_g_fmt(struct v4l2_subdev *sd, struct v4l2_format 
*fmt)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
 	switch (fmt->type) {
 	case V4L2_BUF_TYPE_SLICED_VBI_CAPTURE:
 		return cx25840_vbi(client, VIDIOC_G_FMT, fmt);
 	default:
 		return -EINVAL;
 	}
-
-	return 0;
-}
-
-static int set_v4lfmt(struct i2c_client *client, struct v4l2_format 
*fmt)
-{
-	struct cx25840_state *state = i2c_get_clientdata(client);
+	return 0;
+}
+
+static int subdev_s_fmt(struct v4l2_subdev *sd, struct v4l2_format 
*fmt)
+{
+	struct subdev_state *state = to_state(sd);
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	struct v4l2_pix_format *pix;
 	int HSC, VSC, Vsrc, Hsrc, filter, Vlines;
 	int is_50Hz = !(state->std & V4L2_STD_525_60);
@@ -933,7 +937,7 @@ static void log_video_status(struct i2c_
 		"0xD", "0xE", "0xF"
 	};
 
-	struct cx25840_state *state = i2c_get_clientdata(client);
+	struct subdev_state *state = to_state(i2c_get_clientdata(client));
 	u8 vidfmt_sel = cx25840_read(client, 0x400) & 0xf;
 	u8 gen_stat1 = cx25840_read(client, 0x40d);
 	u8 gen_stat2 = cx25840_read(client, 0x40e);
@@ -963,7 +967,7 @@ static void log_video_status(struct i2c_
 
 static void log_audio_status(struct i2c_client *client)
 {
-	struct cx25840_state *state = i2c_get_clientdata(client);
+	struct subdev_state *state = to_state(i2c_get_clientdata(client));
 	u8 download_ctl = cx25840_read(client, 0x803);
 	u8 mod_det_stat0 = cx25840_read(client, 0x804);
 	u8 mod_det_stat1 = cx25840_read(client, 0x805);
@@ -1116,21 +1120,12 @@ static void log_audio_status(struct i2c_
 
 /* ----------------------------------------------------------------------- 
*/
 
-static int cx25840_command(struct i2c_client *client, unsigned int cmd,
-			   void *arg)
-{
-	struct cx25840_state *state = i2c_get_clientdata(client);
-	struct v4l2_tuner *vt = arg;
-	struct v4l2_routing *route = arg;
-
-	/* ignore these commands */
-	switch (cmd) {
-		case TUNER_SET_TYPE_ADDR:
-			return 0;
-	}
+static int subdev_init(struct v4l2_subdev *sd, u32 val)
+{
+	struct subdev_state *state = to_state(sd);
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
 
 	if (!state->is_initialized) {
-		v4l_dbg(1, cx25840_debug, client, "cmd %08x triggered fw load\n", 
cmd);
 		/* initialize on first use */
 		state->is_initialized = 1;
 		if (state->is_cx25836)
@@ -1140,50 +1135,69 @@ static int cx25840_command(struct i2c_cl
 		else
 			cx25840_initialize(client);
 	}
-
-	switch (cmd) {
+	return 0;
+}
+
 #ifdef CONFIG_VIDEO_ADV_DEBUG
-	/* ioctls to allow direct access to the
-	 * cx25840 registers for testing */
-	case VIDIOC_DBG_G_REGISTER:
-	case VIDIOC_DBG_S_REGISTER:
-	{
-		struct v4l2_register *reg = arg;
-
-		if (!v4l2_chip_match_i2c_client(client, reg->match_type, 
reg->match_chip))
-			return -EINVAL;
-		if (!capable(CAP_SYS_ADMIN))
-			return -EPERM;
-
-		if (cmd == VIDIOC_DBG_G_REGISTER)
-			reg->val = cx25840_read(client, reg->reg & 0x0fff);
-		else
-			cx25840_write(client, reg->reg & 0x0fff, reg->val & 0xff);
-		break;
-	}
+static int subdev_g_register(struct v4l2_subdev *sd, struct 
v4l2_register *reg)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+	if (!v4l2_chip_match_i2c_client(client,
+				reg->match_type, reg->match_chip))
+		return -EINVAL;
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+	reg->val = cx25840_read(client, reg->reg & 0x0fff);
+	return 0;
+}
+
+static int subdev_s_register(struct v4l2_subdev *sd, struct 
v4l2_register *reg)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+	if (!v4l2_chip_match_i2c_client(client,
+				reg->match_type, reg->match_chip))
+		return -EINVAL;
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+	cx25840_write(client, reg->reg & 0x0fff, reg->val & 0xff);
+	return 0;
+}
 #endif
 
-	case VIDIOC_INT_DECODE_VBI_LINE:
-		return cx25840_vbi(client, cmd, arg);
-
-	case VIDIOC_INT_AUDIO_CLOCK_FREQ:
-		return cx25840_audio(client, cmd, arg);
-
-	case VIDIOC_STREAMON:
-		v4l_dbg(1, cx25840_debug, client, "enable output\n");
+static int subdev_decode_vbi_line(struct v4l2_subdev *sd, struct 
v4l2_decode_vbi_line *vbi)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+	return cx25840_vbi(client, VIDIOC_INT_DECODE_VBI_LINE, vbi);
+}
+
+static int subdev_s_clock_freq(struct v4l2_subdev *sd, u32 freq)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+	return cx25840_audio(client, VIDIOC_INT_AUDIO_CLOCK_FREQ, &freq);
+}
+
+static int subdev_s_stream(struct v4l2_subdev *sd, int enable)
+{
+	struct subdev_state *state = to_state(sd);
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+	v4l_dbg(1, cx25840_debug, client, "%s output\n",
+			enable ? "enable" : "disable");
+	if (enable) {
 		if (state->is_cx23885) {
 			u8 v = (cx25840_read(client, 0x421) | 0x0b);
 			cx25840_write(client, 0x421, v);
 		} else {
 			cx25840_write(client, 0x115,
-				state->is_cx25836 ? 0x0c : 0x8c);
+					state->is_cx25836 ? 0x0c : 0x8c);
 			cx25840_write(client, 0x116,
-				state->is_cx25836 ? 0x04 : 0x07);
-		}
-		break;
-
-	case VIDIOC_STREAMOFF:
-		v4l_dbg(1, cx25840_debug, client, "disable output\n");
+					state->is_cx25836 ? 0x04 : 0x07);
+		}
+	} else {
 		if (state->is_cx23885) {
 			u8 v = cx25840_read(client, 0x421) & ~(0x0b);
 			cx25840_write(client, 0x421, v);
@@ -1191,133 +1205,136 @@ static int cx25840_command(struct i2c_cl
 			cx25840_write(client, 0x115, 0x00);
 			cx25840_write(client, 0x116, 0x00);
 		}
+	}
+	return 0;
+}
+
+static int subdev_queryctrl(struct v4l2_subdev *sd, struct 
v4l2_queryctrl *qc)
+{
+	struct subdev_state *state = to_state(sd);
+
+	switch (qc->id) {
+	case V4L2_CID_BRIGHTNESS:
+	case V4L2_CID_CONTRAST:
+	case V4L2_CID_SATURATION:
+	case V4L2_CID_HUE:
+		return v4l2_ctrl_query_fill_std(qc);
+	default:
 		break;
-
-	case VIDIOC_LOG_STATUS:
-		log_video_status(client);
-		if (!state->is_cx25836)
-			log_audio_status(client);
-		break;
-
-	case VIDIOC_G_CTRL:
-		return get_v4lctrl(client, (struct v4l2_control *)arg);
-
-	case VIDIOC_S_CTRL:
-		return set_v4lctrl(client, (struct v4l2_control *)arg);
-
-	case VIDIOC_QUERYCTRL:
-	{
-		struct v4l2_queryctrl *qc = arg;
-
-		switch (qc->id) {
-			case V4L2_CID_BRIGHTNESS:
-			case V4L2_CID_CONTRAST:
-			case V4L2_CID_SATURATION:
-			case V4L2_CID_HUE:
-				return v4l2_ctrl_query_fill_std(qc);
-			default:
-				break;
-		}
-		if (state->is_cx25836)
-			return -EINVAL;
-
-		switch (qc->id) {
-			case V4L2_CID_AUDIO_VOLUME:
-				return v4l2_ctrl_query_fill(qc, 0, 65535,
-					65535 / 100, state->default_volume);
-			case V4L2_CID_AUDIO_MUTE:
-			case V4L2_CID_AUDIO_BALANCE:
-			case V4L2_CID_AUDIO_BASS:
-			case V4L2_CID_AUDIO_TREBLE:
-				return v4l2_ctrl_query_fill_std(qc);
-			default:
-				return -EINVAL;
-		}
+	}
+	if (state->is_cx25836)
 		return -EINVAL;
-	}
-
-	case VIDIOC_G_STD:
-		*(v4l2_std_id *)arg = state->std;
-		break;
-
-	case VIDIOC_S_STD:
-		if (state->radio == 0 && state->std == *(v4l2_std_id *)arg)
-			return 0;
-		state->radio = 0;
-		state->std = *(v4l2_std_id *)arg;
-		return set_v4lstd(client);
-
-	case AUDC_SET_RADIO:
-		state->radio = 1;
-		break;
-
-	case VIDIOC_INT_G_VIDEO_ROUTING:
-		route->input = state->vid_input;
-		route->output = 0;
-		break;
-
-	case VIDIOC_INT_S_VIDEO_ROUTING:
-		return set_input(client, route->input, state->aud_input);
-
-	case VIDIOC_INT_G_AUDIO_ROUTING:
-		if (state->is_cx25836)
-			return -EINVAL;
-		route->input = state->aud_input;
-		route->output = 0;
-		break;
-
-	case VIDIOC_INT_S_AUDIO_ROUTING:
-		if (state->is_cx25836)
-			return -EINVAL;
-		return set_input(client, state->vid_input, route->input);
-
-	case VIDIOC_S_FREQUENCY:
-		if (!state->is_cx25836) {
-			input_change(client);
-		}
-		break;
-
-	case VIDIOC_G_TUNER:
-	{
-		u8 vpres = cx25840_read(client, 0x40e) & 0x20;
-		u8 mode;
-		int val = 0;
-
-		if (state->radio)
-			break;
-
-		vt->signal = vpres ? 0xffff : 0x0;
-		if (state->is_cx25836)
-			break;
-
-		vt->capability |=
-		    V4L2_TUNER_CAP_STEREO | V4L2_TUNER_CAP_LANG1 |
-		    V4L2_TUNER_CAP_LANG2 | V4L2_TUNER_CAP_SAP;
-
-		mode = cx25840_read(client, 0x804);
-
-		/* get rxsubchans and audmode */
-		if ((mode & 0xf) == 1)
-			val |= V4L2_TUNER_SUB_STEREO;
-		else
-			val |= V4L2_TUNER_SUB_MONO;
-
-		if (mode == 2 || mode == 4)
-			val = V4L2_TUNER_SUB_LANG1 | V4L2_TUNER_SUB_LANG2;
-
-		if (mode & 0x10)
-			val |= V4L2_TUNER_SUB_SAP;
-
-		vt->rxsubchans = val;
-		vt->audmode = state->audmode;
-		break;
-	}
-
-	case VIDIOC_S_TUNER:
-		if (state->radio || state->is_cx25836)
-			break;
-
-		switch (vt->audmode) {
+
+	switch (qc->id) {
+	case V4L2_CID_AUDIO_VOLUME:
+		return v4l2_ctrl_query_fill(qc, 0, 65535,
+				65535 / 100, state->default_volume);
+	case V4L2_CID_AUDIO_MUTE:
+	case V4L2_CID_AUDIO_BALANCE:
+	case V4L2_CID_AUDIO_BASS:
+	case V4L2_CID_AUDIO_TREBLE:
+		return v4l2_ctrl_query_fill_std(qc);
+	default:
+		return -EINVAL;
+	}
+	return -EINVAL;
+}
+
+static int subdev_s_std(struct v4l2_subdev *sd, v4l2_std_id std)
+{
+	struct subdev_state *state = to_state(sd);
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+	if (state->radio == 0 && state->std == std)
+		return 0;
+	state->radio = 0;
+	state->std = std;
+	return set_v4lstd(client);
+}
+
+static int subdev_s_radio(struct v4l2_subdev *sd)
+{
+	struct subdev_state *state = to_state(sd);
+
+	state->radio = 1;
+	return 0;
+}
+
+static int subdev_s_video_routing(struct v4l2_subdev *sd, const struct 
v4l2_routing *route)
+{
+	struct subdev_state *state = to_state(sd);
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+	return set_input(client, route->input, state->aud_input);
+}
+
+static int subdev_s_audio_routing(struct v4l2_subdev *sd, const struct 
v4l2_routing *route)
+{
+	struct subdev_state *state = to_state(sd);
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+	if (state->is_cx25836)
+		return -EINVAL;
+	return set_input(client, state->vid_input, route->input);
+}
+
+static int subdev_s_frequency(struct v4l2_subdev *sd, struct 
v4l2_frequency *freq)
+{
+	struct subdev_state *state = to_state(sd);
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+	if (!state->is_cx25836)
+		input_change(client);
+	return 0;
+}
+
+static int subdev_g_tuner(struct v4l2_subdev *sd, struct v4l2_tuner 
*vt)
+{
+	struct subdev_state *state = to_state(sd);
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	u8 vpres = cx25840_read(client, 0x40e) & 0x20;
+	u8 mode;
+	int val = 0;
+
+	if (state->radio)
+		return 0;
+
+	vt->signal = vpres ? 0xffff : 0x0;
+	if (state->is_cx25836)
+		return 0;
+
+	vt->capability |=
+		V4L2_TUNER_CAP_STEREO | V4L2_TUNER_CAP_LANG1 |
+		V4L2_TUNER_CAP_LANG2 | V4L2_TUNER_CAP_SAP;
+
+	mode = cx25840_read(client, 0x804);
+
+	/* get rxsubchans and audmode */
+	if ((mode & 0xf) == 1)
+		val |= V4L2_TUNER_SUB_STEREO;
+	else
+		val |= V4L2_TUNER_SUB_MONO;
+
+	if (mode == 2 || mode == 4)
+		val = V4L2_TUNER_SUB_LANG1 | V4L2_TUNER_SUB_LANG2;
+
+	if (mode & 0x10)
+		val |= V4L2_TUNER_SUB_SAP;
+
+	vt->rxsubchans = val;
+	vt->audmode = state->audmode;
+	return 0;
+}
+
+static int subdev_s_tuner(struct v4l2_subdev *sd, struct v4l2_tuner 
*vt)
+{
+	struct subdev_state *state = to_state(sd);
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+	if (state->radio || state->is_cx25836)
+		return 0;
+
+	switch (vt->audmode) {
 		case V4L2_TUNER_MODE_MONO:
 			/* mono      -> mono
 			   stereo    -> mono
@@ -1345,41 +1362,100 @@ static int cx25840_command(struct i2c_cl
 			break;
 		default:
 			return -EINVAL;
-		}
-		state->audmode = vt->audmode;
-		break;
-
-	case VIDIOC_G_FMT:
-		return get_v4lfmt(client, (struct v4l2_format *)arg);
-
-	case VIDIOC_S_FMT:
-		return set_v4lfmt(client, (struct v4l2_format *)arg);
-
-	case VIDIOC_INT_RESET:
-		if (state->is_cx25836)
-			cx25836_initialize(client);
-		else if (state->is_cx23885)
-			cx23885_initialize(client);
-		else
-			cx25840_initialize(client);
-		break;
-
-	case VIDIOC_G_CHIP_IDENT:
-		return v4l2_chip_ident_i2c_client(client, arg, state->id, 
state->rev);
-
-	default:
-		return -EINVAL;
-	}
-
-	return 0;
+	}
+	state->audmode = vt->audmode;
+	return 0;
+}
+
+static int subdev_reset(struct v4l2_subdev *sd, u32 val)
+{
+	struct subdev_state *state = to_state(sd);
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+	if (state->is_cx25836)
+		cx25836_initialize(client);
+	else if (state->is_cx23885)
+		cx23885_initialize(client);
+	else
+		cx25840_initialize(client);
+	return 0;
+}
+
+static int subdev_g_chip_ident(struct v4l2_subdev *sd, struct 
v4l2_chip_ident *chip)
+{
+	struct subdev_state *state = to_state(sd);
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+	return v4l2_chip_ident_i2c_client(client, chip, state->id, 
state->rev);
+}
+
+static int subdev_log_status(struct v4l2_subdev *sd)
+{
+	struct subdev_state *state = to_state(sd);
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+	log_video_status(client);
+	if (!state->is_cx25836)
+		log_audio_status(client);
+	return 0;
+}
+
+static int subdev_command(struct i2c_client *client, unsigned cmd, void 
*arg)
+{
+	return v4l2_subdev_command(i2c_get_clientdata(client), cmd, arg);
 }
 
 /* ----------------------------------------------------------------------- 
*/
 
-static int cx25840_probe(struct i2c_client *client,
+static const struct v4l2_subdev_core_ops subdev_core_ops = {
+	.log_status = subdev_log_status,
+	.g_chip_ident = subdev_g_chip_ident,
+	.g_ctrl = subdev_g_ctrl,
+	.s_ctrl = subdev_s_ctrl,
+	.queryctrl = subdev_queryctrl,
+	.reset = subdev_reset,
+	.init = subdev_init,
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+	.g_register = subdev_g_register,
+	.s_register = subdev_s_register,
+#endif
+};
+
+static const struct v4l2_subdev_tuner_ops subdev_tuner_ops = {
+	.s_frequency = subdev_s_frequency,
+	.s_std = subdev_s_std,
+	.s_radio = subdev_s_radio,
+	.g_tuner = subdev_g_tuner,
+	.s_tuner = subdev_s_tuner,
+};
+
+static const struct v4l2_subdev_audio_ops subdev_audio_ops = {
+	.s_clock_freq = subdev_s_clock_freq,
+	.s_routing = subdev_s_audio_routing,
+};
+
+static const struct v4l2_subdev_video_ops subdev_video_ops = {
+	.s_routing = subdev_s_video_routing,
+	.g_fmt = subdev_g_fmt,
+	.s_fmt = subdev_s_fmt,
+	.decode_vbi_line = subdev_decode_vbi_line,
+	.s_stream = subdev_s_stream,
+};
+
+static const struct v4l2_subdev_ops subdev_ops = {
+	.core = &subdev_core_ops,
+	.tuner = &subdev_tuner_ops,
+	.audio = &subdev_audio_ops,
+	.video = &subdev_video_ops,
+};
+
+/* ----------------------------------------------------------------------- 
*/
+
+static int subdev_probe(struct i2c_client *client,
 			 const struct i2c_device_id *did)
 {
-	struct cx25840_state *state;
+	struct subdev_state *state;
+	struct v4l2_subdev *sd;
 	u32 id;
 	u16 device_id;
 
@@ -1410,11 +1486,12 @@ static int cx25840_probe(struct i2c_clie
 		return -ENODEV;
 	}
 
-	state = kzalloc(sizeof(struct cx25840_state), GFP_KERNEL);
-	if (state == NULL) {
+	state = kzalloc(sizeof(struct subdev_state), GFP_KERNEL);
+	if (state == NULL)
 		return -ENOMEM;
-	}
-
+
+	sd = &state->sd;
+	v4l2_i2c_subdev_init(sd, client, &subdev_ops);
 	/* Note: revision '(device_id & 0x0f) == 2' was never built. The
 	   marking skips from 0x1 == 22 to 0x3 == 23. */
 	v4l_info(client, "cx25%3x-2%x found @ 0x%x (%s)\n",
@@ -1422,7 +1499,6 @@ static int cx25840_probe(struct i2c_clie
 		    (device_id & 0x0f) < 3 ? (device_id & 0x0f) + 1 : (device_id & 
0x0f),
 		    client->addr << 1, client->adapter->name);
 
-	i2c_set_clientdata(client, state);
 	state->c = client;
 	state->is_cx25836 = ((device_id & 0xff00) == 0x8300);
 	state->is_cx23885 = (device_id == 0x0000) || (device_id == 0x1313);
@@ -1447,9 +1523,12 @@ static int cx25840_probe(struct i2c_clie
 	return 0;
 }
 
-static int cx25840_remove(struct i2c_client *client)
-{
-	kfree(i2c_get_clientdata(client));
+static int subdev_remove(struct i2c_client *client)
+{
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+
+	v4l2_device_unregister_subdev(sd);
+	kfree(to_state(sd));
 	return 0;
 }
 
@@ -1464,9 +1543,9 @@ static struct v4l2_i2c_driver_data v4l2_
 static struct v4l2_i2c_driver_data v4l2_i2c_data = {
 	.name = "cx25840",
 	.driverid = I2C_DRIVERID_CX25840,
-	.command = cx25840_command,
-	.probe = cx25840_probe,
-	.remove = cx25840_remove,
+	.command = subdev_command,
+	.probe = subdev_probe,
+	.remove = subdev_remove,
 #if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 26)
 	.id_table = cx25840_id,
 #endif
--- a/linux/drivers/media/video/cx25840/cx25840-core.h	Mon Nov 24 
22:12:55 2008 +0100
+++ b/linux/drivers/media/video/cx25840/cx25840-core.h	Mon Nov 24 
22:14:17 2008 +0100
@@ -23,6 +23,7 @@
 #include "compat.h"
 
 #include <linux/videodev2.h>
+#include <media/v4l2-device.h>
 #include <linux/i2c.h>
 
 /* ENABLE_PVR150_WORKAROUND activates a workaround for a hardware bug 
that is
@@ -33,8 +34,9 @@
    providing this information. */
 #define CX25840_CID_ENABLE_PVR150_WORKAROUND (V4L2_CID_PRIVATE_BASE+0)
 
-struct cx25840_state {
+struct subdev_state {
 	struct i2c_client *c;
+	struct v4l2_subdev sd;
 	int pvr150_workaround;
 	int radio;
 	v4l2_std_id std;
@@ -53,6 +55,11 @@ struct cx25840_state {
 	wait_queue_head_t fw_wait;    /* wake up when the fw load is finished 
*/
 	struct work_struct fw_work;   /* work entry for fw load */
 };
+
+static inline struct subdev_state *to_state(struct v4l2_subdev *sd)
+{
+	return container_of(sd, struct subdev_state, sd);
+}
 
 /* ----------------------------------------------------------------------- 
*/
 /* cx25850-core.c 							   */
--- a/linux/drivers/media/video/cx25840/cx25840-firmware.c	Mon Nov 24 
22:12:55 2008 +0100
+++ b/linux/drivers/media/video/cx25840/cx25840-firmware.c	Mon Nov 24 
22:14:17 2008 +0100
@@ -92,7 +92,7 @@ static int fw_write(struct i2c_client *c
 
 int cx25840_loadfw(struct i2c_client *client)
 {
-	struct cx25840_state *state = i2c_get_clientdata(client);
+	struct subdev_state *state = to_state(i2c_get_clientdata(client));
 	const struct firmware *fw = NULL;
 	u8 buffer[FWSEND];
 	const u8 *ptr;
--- a/linux/drivers/media/video/cx25840/cx25840-vbi.c	Mon Nov 24 
22:12:55 2008 +0100
+++ b/linux/drivers/media/video/cx25840/cx25840-vbi.c	Mon Nov 24 
22:14:17 2008 +0100
@@ -85,7 +85,7 @@ static int decode_vps(u8 * dst, u8 * p)
 
 int cx25840_vbi(struct i2c_client *client, unsigned int cmd, void *arg)
 {
-	struct cx25840_state *state = i2c_get_clientdata(client);
+	struct subdev_state *state = to_state(i2c_get_clientdata(client));
 	struct v4l2_format *fmt;
 	struct v4l2_sliced_vbi_format *svbi;
 

Re: v4l2_device/v4l2_subdev: please review

[Hans Verkuil]

> Hi Hans,
>
>>
>> I'm not going to spam the list with these quite big patches. Just go to
>> http://linuxtv.org/hg/~hverkuil/v4l-dvb-ng/ and click on the 'raw' link
>> after each change to see the patch. Most of these changes are just
>> boring i2c driver conversions.
>
> It is hard to review these patches from this link, as if you submit
> the patch to ML then someone can just give inline comments to your
> patch, otherwise reviewer has to copy that code which he/she wants to
> comment while replying and not easy to track too. I don't know size
> limit of this v4l2 ML, but linux-kernel ML can receive somewhat big
> patches I believe.

The V4L2 ML is fairly limited. I'm pretty sure it can't handle the ivtv
change (70+ kB). I can do this for the two changes that actually add the
new code and for one of the i2c conversion tonight (this webmail client
messes up the layout).

Personally I dislike reviewing large patches that are part of an email.
Smaller ones are OK, but large patches are hard to read. Much easier to
review with a decent editor.

>
>>
>> We are adding to the v4l core, but the changes do not affect existing
>> v4l drivers let alone other subsystems. Although I should probably have
>> added the omap list.
>
> OMAP + soc-camera + v4l2-int-* community would be more interested to
> see these patches as they need to change their sensor/controller
> drivers to adapt your changes.

I'll add the linux-omap mailinglist when I repost tonight.

Regards,

          Hans