Hi all,
This is hopefully the final version. All earlier comments have been
incorporated into this patch. I also made a new change: the mutex has been
replaced by a spinlock and I no longer lock when walking the list of subdevs.
So the assumption is that subdevs only added during initialization of the
device and removed during the destruction of the device, and not in between.
I cannot think of any reason why this you would want to do this, but should
this ever happen then the list should be replaced by a klist. I consider
this overkill, esp. since walking the subdev list should be as fast as
possible.
The full tree which includes the i2c module and ivtv conversion as well can
be accessed here: http://linuxtv.org/hg/~hverkuil/v4l-dvb-ng
Regards,
Hans
# HG changeset patch
# User Hans Verkuil <[email protected]>
# Date 1227979357 -3600
# Node ID 719584d5989d93b7ccb3de54155cbab9e6b06526
# Parent 7100e78482d70ee9acf90f2c15848f6002f89b7d
v4l2: add v4l2_device and v4l2_subdev structs to the v4l2 framework.
From: Hans Verkuil <[email protected]>
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 <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
Reviewed-by: Guennadi Liakhovetski <[email protected]>
Reviewed-by: Andy Walls <[email protected]>
diff -r 7100e78482d7 -r 719584d5989d linux/Documentation/video4linux/v4l2-framework.txt
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/linux/Documentation/video4linux/v4l2-framework.txt Sat Nov 29 18:22:37 2008 +0100
@@ -0,0 +1,362 @@
+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 (if any).
+
+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 in the future a v4l2_fh struct will keep track of filehandle instances
+(this is 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->driver_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_get_drvdata(dev);
+
+ /* 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.
+
+A typical state struct would look like this (where 'chipname' is replaced by
+the name of the chip):
+
+struct chipname_state {
+ struct v4l2_subdev sd;
+ ... /* additional state fields */
+};
+
+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 a chipname_state struct:
+
+static inline struct chipname_state *to_state(struct v4l2_subdev *sd)
+{
+ return container_of(sd, struct chipname_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
+-------------------
+
+Not yet documented.
diff -r 7100e78482d7 -r 719584d5989d linux/drivers/media/video/Makefile
--- a/linux/drivers/media/video/Makefile Sat Nov 29 00:46:43 2008 -0200
+++ b/linux/drivers/media/video/Makefile Sat Nov 29 18:22:37 2008 +0100
@@ -8,7 +8,7 @@
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
diff -r 7100e78482d7 -r 719584d5989d linux/drivers/media/video/v4l2-device.c
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/linux/drivers/media/video/v4l2-device.c Sat Nov 29 18:22:37 2008 +0100
@@ -0,0 +1,80 @@
+/*
+ V4L2 device support.
+
+ Copyright (C) 2008 Hans Verkuil <[email protected]>
+
+ 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_get_drvdata(dev));
+ INIT_LIST_HEAD(&v4l2_dev->subdevs);
+ spin_lock_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_set_drvdata(dev, 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 || !dev_get_drvdata(v4l2_dev->dev));
+ dev_set_drvdata(v4l2_dev->dev, 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;
+ spin_lock(&dev->lock);
+ list_add_tail(&sd->list, &dev->subdevs);
+ spin_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;
+ spin_lock(&sd->dev->lock);
+ list_del(&sd->list);
+ spin_unlock(&sd->dev->lock);
+ sd->dev = NULL;
+ module_put(sd->owner);
+}
+EXPORT_SYMBOL(v4l2_device_unregister_subdev);
diff -r 7100e78482d7 -r 719584d5989d linux/drivers/media/video/v4l2-subdev.c
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/linux/drivers/media/video/v4l2-subdev.c Sat Nov 29 18:22:37 2008 +0100
@@ -0,0 +1,108 @@
+/*
+ V4L2 sub-device support.
+
+ Copyright (C) 2008 Hans Verkuil <[email protected]>
+
+ 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);
diff -r 7100e78482d7 -r 719584d5989d linux/include/media/v4l2-device.h
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/linux/include/media/v4l2-device.h Sat Nov 29 18:22:37 2008 +0100
@@ -0,0 +1,109 @@
+/*
+ V4L2 device support header.
+
+ Copyright (C) 2008 Hans Verkuil <[email protected]>
+
+ 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.
+ */
+
+#define V4L2_DEVICE_NAME_SIZE (BUS_ID_SIZE + 16)
+
+struct v4l2_device {
+ /* dev->driver_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. */
+ spinlock_t lock;
+ /* unique device name, by default the driver name + bus ID */
+ char name[V4L2_DEVICE_NAME_SIZE];
+};
+
+/* Initialize v4l2_dev and make dev->driver_data point to v4l2_dev */
+void v4l2_device_register(struct device *dev, struct v4l2_device *v4l2_dev);
+/* Set v4l2_dev->dev->driver_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. 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); \
+ 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; \
+ \
+ list_for_each_entry(sd, &(dev)->subdevs, list) \
+ if ((cond) && sd->ops->o && sd->ops->o->f) \
+ sd->ops->o->f(sd , ##args); \
+ } 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; \
+ \
+ 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; \
+ } \
+ (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
diff -r 7100e78482d7 -r 719584d5989d linux/include/media/v4l2-subdev.h
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/linux/include/media/v4l2-subdev.h Sat Nov 29 18:22:37 2008 +0100
@@ -0,0 +1,188 @@
+/*
+ V4L2 sub-device support header.
+
+ Copyright (C) 2008 Hans Verkuil <[email protected]>
+
+ 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
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG
On Saturday 29 November 2008, Hans Verkuil wrote:
> +void v4l2_device_register(struct device *dev, struct v4l2_device *v4l2_dev)
> +{
> +???????BUG_ON(!dev || !v4l2_dev || dev_get_drvdata(dev));
>
Ouch. Better to return -EINVAL, like most register() calls,
than *ever* use a BUG_ON() for bad parameters. Same applies
every other place you use BUG_ON, from a quick scan ...
For the unregister() paths a WARN() would be fair. Again,
any time you're tempted to use BUG() or BUG_ON(), you need
to re-think. It's hardly ever the right thing to do. Just
report the error and continue; callers should check, and
clean up if something went wrong.
> +EXPORT_SYMBOL(v4l2_device_register);
This may be a nit, but I wonder why not EXPORT_SYMBOL_GPL,
which seems to be more correct in this case.
Another quasi-style point: v4l2-device.s and v4l-subdev.c
are so small, and conceptually related, that I'd be tempted
to have one file not two. Ditto their headers.
- Dave
On Saturday 29 November 2008 21:20:47 David Brownell wrote:
> On Saturday 29 November 2008, Hans Verkuil wrote:
> > +void v4l2_device_register(struct device *dev, struct v4l2_device
> > *v4l2_dev) +{
> > +???????BUG_ON(!dev || !v4l2_dev || dev_get_drvdata(dev));
>
> Ouch. Better to return -EINVAL, like most register() calls,
> than *ever* use a BUG_ON() for bad parameters. Same applies
> every other place you use BUG_ON, from a quick scan ...
Are there some documented guidelines on when to use BUG_ON? I see it
used in other places in this way. My reasoning was that returning an
error makes sense if external causes can result in an error, but this
test is more the equivalent of an assert(), i.e. catching a programming
bug early.
> For the unregister() paths a WARN() would be fair. Again,
> any time you're tempted to use BUG() or BUG_ON(), you need
> to re-think. It's hardly ever the right thing to do. Just
> report the error and continue; callers should check, and
> clean up if something went wrong.
>
> > +EXPORT_SYMBOL(v4l2_device_register);
>
> This may be a nit, but I wonder why not EXPORT_SYMBOL_GPL,
> which seems to be more correct in this case.
I was under the impression that the v4l core was all EXPORT_SYMBOL, but
I took another look and a lot is now EXPORT_SYMBOL_GPL, so I guess I
should use that as well.
> Another quasi-style point: v4l2-device.s and v4l-subdev.c
> are so small, and conceptually related, that I'd be tempted
> to have one file not two. Ditto their headers.
They are small now, but they will become a lot larger in the future.
There is a lot of common code in most if not all of the v4l drivers and
my intention is to gradually expand the framework and move some of the
common code into these headers/sources. This is just the start.
>
> - Dave
>
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-kernel" in the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG
On Saturday 29 November 2008, Hans Verkuil wrote:
> > > +void v4l2_device_register(struct device *dev, struct v4l2_device *v4l2_dev)
> > > +{
> > > +???????BUG_ON(!dev || !v4l2_dev || dev_get_drvdata(dev));
> >
> > Ouch. ?Better to return -EINVAL, like most register() calls,
> > than *ever* use a BUG_ON() for bad parameters. ?Same applies
> > every other place you use BUG_ON, from a quick scan ...
>
> Are there some documented guidelines on when to use BUG_ON?
Maybe there should be. I know I've seen flames from Linus on
the topic. Basically, treat it like a panic() where the system
must stop operation lest it catch fire or scribble all over the
(not-backed-up) disk ... if the system can keep running sanely,
then BUG() and friends are inappropriate.
> I see it used in other places in this way.
I tend to submit patches fixing bugs like that, when I have time.
> My reasoning was that returning an
> error makes sense if external causes can result in an error, but this
> test is more the equivalent of an assert(), i.e. catching a programming
> bug early.
In which case a WARN() is better. But in most cases I wouldn't
even do that. The kernel's design center is closer to "run
robustly" than "make developers' lives easier". Programmers
who don't check return values for critical operations like
registering core resources deserve what they get. And if you
want to nudge them, the __must_check annotation helps catch
such goofage even earlier: compile time, not run time.
- Dave
On Saturday 29 November 2008 23:22:19 David Brownell wrote:
> On Saturday 29 November 2008, Hans Verkuil wrote:
> > > > +void v4l2_device_register(struct device *dev, struct
> > > > v4l2_device *v4l2_dev) +{
> > > > +???????BUG_ON(!dev || !v4l2_dev || dev_get_drvdata(dev));
> > >
> > > Ouch. ?Better to return -EINVAL, like most register() calls,
> > > than *ever* use a BUG_ON() for bad parameters. ?Same applies
> > > every other place you use BUG_ON, from a quick scan ...
> >
> > Are there some documented guidelines on when to use BUG_ON?
>
> Maybe there should be. I know I've seen flames from Linus on
> the topic. Basically, treat it like a panic() where the system
> must stop operation lest it catch fire or scribble all over the
> (not-backed-up) disk ... if the system can keep running sanely,
> then BUG() and friends are inappropriate.
I think it would be good to have some document about this, since
from what I've seen from a quick scan I'm not the only one who uses
it incorrectly. There is no documentation in the asm-generic/bug.h
header and there is also no documentation on this in the Documentation
directory.
> > I see it used in other places in this way.
>
> I tend to submit patches fixing bugs like that, when I have time.
>
> > My reasoning was that returning an
> > error makes sense if external causes can result in an error, but
> > this test is more the equivalent of an assert(), i.e. catching a
> > programming bug early.
>
> In which case a WARN() is better. But in most cases I wouldn't
> even do that. The kernel's design center is closer to "run
> robustly" than "make developers' lives easier". Programmers
> who don't check return values for critical operations like
> registering core resources deserve what they get. And if you
> want to nudge them, the __must_check annotation helps catch
> such goofage even earlier: compile time, not run time.
I've replaced it as follows (and with __must_check in the header):
int v4l2_device_register(struct device *dev, struct v4l2_device *v4l2_dev)
{
if (dev == NULL || v4l2_dev == NULL)
return -EINVAL;
/* Warn if we apparently re-register a device */
WARN_ON(dev_get_drvdata(dev));
INIT_LIST_HEAD(&v4l2_dev->subdevs);
spin_lock_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_set_drvdata(dev, v4l2_dev);
return 0;
}
EXPORT_SYMBOL_GPL(v4l2_device_register);
void v4l2_device_unregister(struct v4l2_device *v4l2_dev)
{
struct v4l2_subdev *sd, *next;
if (v4l2_dev == NULL || v4l2_dev->dev == NULL)
return;
dev_set_drvdata(v4l2_dev->dev, 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_GPL(v4l2_device_unregister);
int v4l2_device_register_subdev(struct v4l2_device *dev, struct v4l2_subdev *sd)
{
/* Check for valid input */
if (dev == NULL || sd == NULL || !sd->name[0])
return -EINVAL;
/* Warn if we apparently re-register a subdev */
WARN_ON(sd->dev);
if (!try_module_get(sd->owner))
return -ENODEV;
sd->dev = dev;
spin_lock(&dev->lock);
list_add_tail(&sd->list, &dev->subdevs);
spin_unlock(&dev->lock);
return 0;
}
EXPORT_SYMBOL_GPL(v4l2_device_register_subdev);
void v4l2_device_unregister_subdev(struct v4l2_subdev *sd)
{
/* return if it isn't registered */
if (sd == NULL || sd->dev == NULL)
return;
spin_lock(&sd->dev->lock);
list_del(&sd->list);
spin_unlock(&sd->dev->lock);
sd->dev = NULL;
module_put(sd->owner);
}
EXPORT_SYMBOL_GPL(v4l2_device_unregister_subdev);
Thanks for the review!
Hans
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG
On Sat, 2008-11-29 at 18:52 +0100, Hans Verkuil wrote:
> Hi all,
>
> This is hopefully the final version. All earlier comments have been
> incorporated into this patch. I also made a new change: the mutex has been
> replaced by a spinlock and I no longer lock when walking the list of subdevs.
>
> So the assumption is that subdevs only added during initialization of the
> device and removed during the destruction of the device, and not in between.
> I cannot think of any reason why this you would want to do this, but should
> this ever happen then the list should be replaced by a klist. I consider
> this overkill, esp. since walking the subdev list should be as fast as
> possible.
I don't have a problem with not locking during a list walk as long as
you can ensure the register and unregister calls can't happen in the
middle of a walk. I haven't taken a hard look to see if this is the
case. I'd imagine a walk while registration is going on is the only
case that has a remote chance of happening.
Although I think I've just answered my own next question I'll ask
anyway: why are register & unregister such time critical operations that
we have to spin instead of risk sleeping in a mutex? To greatly reduce
the probability a walk happens while registering? If that's the case it
sounds like we're knowingly building in a race.
This comment kind of bugged me too:
>/* Iterate over all subdevs. The next item is prefetched, so you can
> + delete the current item if necessary. */
> +#define v4l2_device_for_each_subdev(sd, dev)
It implies it's safe to remove things from the list that we're not
locking.
I can appreciate the desire for being able to walk the list and issue
commands to various subdevs with high concurrency. I know spinlocks are
optimized for the common case of the lock being available (well at least
the underlying __raw_spin_lock() is optimized, if preemption is enabled
there's a little overhead added). So they give the safety with a minor
penalty at the micro level, but kill concurrency of common operations at
the macro level.
So how about a rwlock_t and using read_lock() and write_lock(), locks
that provide safety and allow high concurrency at the macro level? I
don't know if there's an analog of a read/write mutex.
Did I totally miss the concept?
Regards,
Andy
>
> The full tree which includes the i2c module and ivtv conversion as well can
> be accessed here: http://linuxtv.org/hg/~hverkuil/v4l-dvb-ng
>
> Regards,
>
> Hans
>
> # HG changeset patch
> # User Hans Verkuil <[email protected]>
> # Date 1227979357 -3600
> # Node ID 719584d5989d93b7ccb3de54155cbab9e6b06526
> # Parent 7100e78482d70ee9acf90f2c15848f6002f89b7d
> v4l2: add v4l2_device and v4l2_subdev structs to the v4l2 framework.
>
> From: Hans Verkuil <[email protected]>
>
> 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 <[email protected]>
> Reviewed-by: Laurent Pinchart <[email protected]>
> Reviewed-by: Guennadi Liakhovetski <[email protected]>
> Reviewed-by: Andy Walls <[email protected]>
>
> diff -r 7100e78482d7 -r 719584d5989d linux/Documentation/video4linux/v4l2-framework.txt
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/linux/Documentation/video4linux/v4l2-framework.txt Sat Nov 29 18:22:37 2008 +0100
> @@ -0,0 +1,362 @@
> +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 (if any).
> +
> +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 in the future a v4l2_fh struct will keep track of filehandle instances
> +(this is 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->driver_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_get_drvdata(dev);
> +
> + /* 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.
> +
> +A typical state struct would look like this (where 'chipname' is replaced by
> +the name of the chip):
> +
> +struct chipname_state {
> + struct v4l2_subdev sd;
> + ... /* additional state fields */
> +};
> +
> +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 a chipname_state struct:
> +
> +static inline struct chipname_state *to_state(struct v4l2_subdev *sd)
> +{
> + return container_of(sd, struct chipname_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
> +-------------------
> +
> +Not yet documented.
> diff -r 7100e78482d7 -r 719584d5989d linux/drivers/media/video/Makefile
> --- a/linux/drivers/media/video/Makefile Sat Nov 29 00:46:43 2008 -0200
> +++ b/linux/drivers/media/video/Makefile Sat Nov 29 18:22:37 2008 +0100
> @@ -8,7 +8,7 @@
>
> 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
>
> diff -r 7100e78482d7 -r 719584d5989d linux/drivers/media/video/v4l2-device.c
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/linux/drivers/media/video/v4l2-device.c Sat Nov 29 18:22:37 2008 +0100
> @@ -0,0 +1,80 @@
> +/*
> + V4L2 device support.
> +
> + Copyright (C) 2008 Hans Verkuil <[email protected]>
> +
> + 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_get_drvdata(dev));
> + INIT_LIST_HEAD(&v4l2_dev->subdevs);
> + spin_lock_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_set_drvdata(dev, 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 || !dev_get_drvdata(v4l2_dev->dev));
> + dev_set_drvdata(v4l2_dev->dev, 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;
> + spin_lock(&dev->lock);
> + list_add_tail(&sd->list, &dev->subdevs);
> + spin_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;
> + spin_lock(&sd->dev->lock);
> + list_del(&sd->list);
> + spin_unlock(&sd->dev->lock);
> + sd->dev = NULL;
> + module_put(sd->owner);
> +}
> +EXPORT_SYMBOL(v4l2_device_unregister_subdev);
> diff -r 7100e78482d7 -r 719584d5989d linux/drivers/media/video/v4l2-subdev.c
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/linux/drivers/media/video/v4l2-subdev.c Sat Nov 29 18:22:37 2008 +0100
> @@ -0,0 +1,108 @@
> +/*
> + V4L2 sub-device support.
> +
> + Copyright (C) 2008 Hans Verkuil <[email protected]>
> +
> + 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);
> diff -r 7100e78482d7 -r 719584d5989d linux/include/media/v4l2-device.h
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/linux/include/media/v4l2-device.h Sat Nov 29 18:22:37 2008 +0100
> @@ -0,0 +1,109 @@
> +/*
> + V4L2 device support header.
> +
> + Copyright (C) 2008 Hans Verkuil <[email protected]>
> +
> + 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.
> + */
> +
> +#define V4L2_DEVICE_NAME_SIZE (BUS_ID_SIZE + 16)
> +
> +struct v4l2_device {
> + /* dev->driver_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. */
> + spinlock_t lock;
> + /* unique device name, by default the driver name + bus ID */
> + char name[V4L2_DEVICE_NAME_SIZE];
> +};
> +
> +/* Initialize v4l2_dev and make dev->driver_data point to v4l2_dev */
> +void v4l2_device_register(struct device *dev, struct v4l2_device *v4l2_dev);
> +/* Set v4l2_dev->dev->driver_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. 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); \
> + 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; \
> + \
> + list_for_each_entry(sd, &(dev)->subdevs, list) \
> + if ((cond) && sd->ops->o && sd->ops->o->f) \
> + sd->ops->o->f(sd , ##args); \
> + } 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; \
> + \
> + 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; \
> + } \
> + (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
> diff -r 7100e78482d7 -r 719584d5989d linux/include/media/v4l2-subdev.h
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/linux/include/media/v4l2-subdev.h Sat Nov 29 18:22:37 2008 +0100
> @@ -0,0 +1,188 @@
> +/*
> + V4L2 sub-device support header.
> +
> + Copyright (C) 2008 Hans Verkuil <[email protected]>
> +
> + 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
>
>
On Sunday 30 November 2008 00:31:38 Andy Walls wrote:
> On Sat, 2008-11-29 at 18:52 +0100, Hans Verkuil wrote:
> > Hi all,
> >
> > This is hopefully the final version. All earlier comments have been
> > incorporated into this patch. I also made a new change: the mutex
> > has been replaced by a spinlock and I no longer lock when walking
> > the list of subdevs.
> >
> > So the assumption is that subdevs only added during initialization
> > of the device and removed during the destruction of the device, and
> > not in between. I cannot think of any reason why this you would
> > want to do this, but should this ever happen then the list should
> > be replaced by a klist. I consider this overkill, esp. since
> > walking the subdev list should be as fast as possible.
>
> I don't have a problem with not locking during a list walk as long as
> you can ensure the register and unregister calls can't happen in the
> middle of a walk. I haven't taken a hard look to see if this is the
> case. I'd imagine a walk while registration is going on is the only
> case that has a remote chance of happening.
A driver that would do register or unregister calls in the middle of a
walk is very badly written. I can't even imagine how that can happen.
That said, I'll add a comment in the header making it explicit that no
locking is taking place.
> Although I think I've just answered my own next question I'll ask
> anyway: why are register & unregister such time critical operations
> that we have to spin instead of risk sleeping in a mutex? To greatly
> reduce the probability a walk happens while registering? If that's
> the case it sounds like we're knowingly building in a race.
In the register function it only needs to lock the list momentarily in
case two registrations happen at the same time. A mutex is overkill for
that. Perhaps having locking at all is overkill as well for this, but
for now I feel safer with the lock. In addition, I might well change
the way subdevices are registered. Right now the bridge driver loads
and registers a subdevice, but an alternative approach would be that
the i2c driver can register itself with the bridge driver when loaded.
There are actually some advantages to that approach, but if I do that,
then the lock is definitely needed.
> This comment kind of bugged me too:
> >/* Iterate over all subdevs. The next item is prefetched, so you can
> > + delete the current item if necessary. */
> > +#define v4l2_device_for_each_subdev(sd, dev)
>
> It implies it's safe to remove things from the list that we're not
> locking.
Oops, that was a bogus comment. Actually, the whole macro is a bit bogus
and can simply be replaced by this:
/* Iterate over all subdevs. */
#define v4l2_device_for_each_subdev(sd, dev) \
list_for_each_entry(sd, &(dev)->subdevs, list)
> I can appreciate the desire for being able to walk the list and issue
> commands to various subdevs with high concurrency. I know spinlocks
> are optimized for the common case of the lock being available (well
> at least the underlying __raw_spin_lock() is optimized, if preemption
> is enabled there's a little overhead added). So they give the safety
> with a minor penalty at the micro level, but kill concurrency of
> common operations at the macro level.
>
> So how about a rwlock_t and using read_lock() and write_lock(), locks
> that provide safety and allow high concurrency at the macro level? I
> don't know if there's an analog of a read/write mutex.
Note that I can't use spinlocks while walking the subdevs list since the
subdev ops that you call can sleep which is not allowed with spinlocks
(as I very quickly found out when I tried it :-) ).
The only way to safely walk over it is to switch to a klist, but I
really like to keep __v4l2_device_call_subdevs as fast as possible.
Since the registration/unregistration of subdevs is fully controlled by
the driver it is trivial for the driver to ensure that there are no
(un)registrations while the subdev list is being walked. There are no
external events that can cause this to happen.
In addition, there is also no point for a driver to begin issuing
commands to subdevs while some of them are not yet registered. So I
feel confident about my approach.
But if it turns out to be a problem after all in the future, then it
will be easy to replace the list with a klist.
> Did I totally miss the concept?
Not at all, and once again thanks for the comments.
I've updated my tree with the changes mentioned above.
Regards,
Hans
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG
On Sun, 2008-11-30 at 01:40 +0100, Hans Verkuil wrote:
> On Sunday 30 November 2008 00:31:38 Andy Walls wrote:
> > On Sat, 2008-11-29 at 18:52 +0100, Hans Verkuil wrote:
> > > Hi all,
> > >
> > > This is hopefully the final version. All earlier comments have been
> > > incorporated into this patch. I also made a new change: the mutex
> > > has been replaced by a spinlock and I no longer lock when walking
> > > the list of subdevs.
> > >
> > > So the assumption is that subdevs only added during initialization
> > > of the device and removed during the destruction of the device, and
> > > not in between. I cannot think of any reason why this you would
> > > want to do this, but should this ever happen then the list should
> > > be replaced by a klist. I consider this overkill, esp. since
> > > walking the subdev list should be as fast as possible.
> >
> > I don't have a problem with not locking during a list walk as long as
> > you can ensure the register and unregister calls can't happen in the
> > middle of a walk. I haven't taken a hard look to see if this is the
> > case. I'd imagine a walk while registration is going on is the only
> > case that has a remote chance of happening.
>
> A driver that would do register or unregister calls in the middle of a
> walk is very badly written. I can't even imagine how that can happen.
>
> That said, I'll add a comment in the header making it explicit that no
> locking is taking place.
Ah.
> > Although I think I've just answered my own next question I'll ask
> > anyway: why are register & unregister such time critical operations
> > that we have to spin instead of risk sleeping in a mutex? To greatly
> > reduce the probability a walk happens while registering? If that's
> > the case it sounds like we're knowingly building in a race.
>
> In the register function it only needs to lock the list momentarily in
> case two registrations happen at the same time. A mutex is overkill for
> that. Perhaps having locking at all is overkill as well for this, but
> for now I feel safer with the lock. In addition, I might well change
> the way subdevices are registered. Right now the bridge driver loads
> and registers a subdevice, but an alternative approach would be that
> the i2c driver can register itself with the bridge driver when loaded.
> There are actually some advantages to that approach, but if I do that,
> then the lock is definitely needed.
>
> > This comment kind of bugged me too:
> > >/* Iterate over all subdevs. The next item is prefetched, so you can
> > > + delete the current item if necessary. */
> > > +#define v4l2_device_for_each_subdev(sd, dev)
> >
> > It implies it's safe to remove things from the list that we're not
> > locking.
>
> Oops, that was a bogus comment. Actually, the whole macro is a bit bogus
> and can simply be replaced by this:
>
> /* Iterate over all subdevs. */
> #define v4l2_device_for_each_subdev(sd, dev) \
> list_for_each_entry(sd, &(dev)->subdevs, list)
>
> > I can appreciate the desire for being able to walk the list and issue
> > commands to various subdevs with high concurrency. I know spinlocks
> > are optimized for the common case of the lock being available (well
> > at least the underlying __raw_spin_lock() is optimized, if preemption
> > is enabled there's a little overhead added). So they give the safety
> > with a minor penalty at the micro level, but kill concurrency of
> > common operations at the macro level.
> >
> > So how about a rwlock_t and using read_lock() and write_lock(), locks
> > that provide safety and allow high concurrency at the macro level? I
> > don't know if there's an analog of a read/write mutex.
>
> Note that I can't use spinlocks while walking the subdevs list since the
> subdev ops that you call can sleep which is not allowed with spinlocks
> (as I very quickly found out when I tried it :-) ).
Ah.
> The only way to safely walk over it is to switch to a klist, but I
> really like to keep __v4l2_device_call_subdevs as fast as possible.
> Since the registration/unregistration of subdevs is fully controlled by
> the driver it is trivial for the driver to ensure that there are no
> (un)registrations while the subdev list is being walked. There are no
> external events that can cause this to happen.
>
> In addition, there is also no point for a driver to begin issuing
> commands to subdevs while some of them are not yet registered. So I
> feel confident about my approach.
Just playing a little more devil's advocate:
I have a recollection that the lirc_pvr150 module calls an ivtv function
directly to reset the IR blaster chip on PVR-150's. I'd imagine in the
future, the GPIO subdev in ivtv might implement some sort of reset_ops
or ir_ops for a cleaner interface for an updated lirc_pvr150 module to
call in on. There's my one contrived example, contingent on assumed
changes, of a bridge chip driver not being strictly in control of when a
subdev could be called.
(It's moot in this case though as lirc_pvr150 would need ivtv to init
i2c before gpio, which ivtv doesn't do, before lirc_pvr150 would attempt
a reset of the IR blaster.)
I think you're safe for now. :)
On a more philosophical note is GPIO really a single subdev or a
collection of independent serial & discrete buses to a collection of
subdevs? In cx18 depending on the board, GPIO can reset chips, change
audio mux paths, change the state of LED's, and in the future be used as
a serial line (if I ever get that soft-UART to the IR blaster
implemented).
> But if it turns out to be a problem after all in the future, then it
> will be easy to replace the list with a klist.
Well my one contrived example is between two modules who usage is
tightly coupled at least in one direction, even if source code changes
are not tightly coordinated. In that case I'd predict problems will
simply be avoided due to the tight coupling.
Regards,
Andy
> > Did I totally miss the concept?
>
> Not at all, and once again thanks for the comments.
>
> I've updated my tree with the changes mentioned above.
>
> Regards,
>
> Hans
>
> --
> Hans Verkuil - video4linux developer - sponsored by TANDBERG
>
On Saturday 29 November 2008, Hans Verkuil wrote:
> > > Are there some documented guidelines on when to use BUG_ON?
> >
> > Maybe there should be. ?I know I've seen flames from Linus on
> > the topic. ?Basically, treat it like a panic() where the system
> > must stop operation lest it catch fire or scribble all over the
> > (not-backed-up) disk ... if the system can keep running sanely,
> > then BUG() and friends are inappropriate.
>
> I think it would be good to have some document about this, since
> from what I've seen from a quick scan I'm not the only one who uses
> it incorrectly. There is no documentation in the asm-generic/bug.h
> header and there is also no documentation on this in the Documentation
> directory.
Fair enough ... but the basic advice is still going to be "strongly
avoid ever calling BUG()".
I'll send a patch along doing that.
- Dave
On Sunday 30 November 2008 03:49:45 Andy Walls wrote:
> On Sun, 2008-11-30 at 01:40 +0100, Hans Verkuil wrote:
> > On Sunday 30 November 2008 00:31:38 Andy Walls wrote:
> > > On Sat, 2008-11-29 at 18:52 +0100, Hans Verkuil wrote:
> > > > Hi all,
> > > >
> > > > This is hopefully the final version. All earlier comments have
> > > > been incorporated into this patch. I also made a new change:
> > > > the mutex has been replaced by a spinlock and I no longer lock
> > > > when walking the list of subdevs.
> > > >
> > > > So the assumption is that subdevs only added during
> > > > initialization of the device and removed during the destruction
> > > > of the device, and not in between. I cannot think of any reason
> > > > why this you would want to do this, but should this ever happen
> > > > then the list should be replaced by a klist. I consider this
> > > > overkill, esp. since walking the subdev list should be as fast
> > > > as possible.
> > >
> > > I don't have a problem with not locking during a list walk as
> > > long as you can ensure the register and unregister calls can't
> > > happen in the middle of a walk. I haven't taken a hard look to
> > > see if this is the case. I'd imagine a walk while registration
> > > is going on is the only case that has a remote chance of
> > > happening.
> >
> > A driver that would do register or unregister calls in the middle
> > of a walk is very badly written. I can't even imagine how that can
> > happen.
> >
> > That said, I'll add a comment in the header making it explicit that
> > no locking is taking place.
>
> Ah.
>
> > > Although I think I've just answered my own next question I'll ask
> > > anyway: why are register & unregister such time critical
> > > operations that we have to spin instead of risk sleeping in a
> > > mutex? To greatly reduce the probability a walk happens while
> > > registering? If that's the case it sounds like we're knowingly
> > > building in a race.
> >
> > In the register function it only needs to lock the list momentarily
> > in case two registrations happen at the same time. A mutex is
> > overkill for that. Perhaps having locking at all is overkill as
> > well for this, but for now I feel safer with the lock. In addition,
> > I might well change the way subdevices are registered. Right now
> > the bridge driver loads and registers a subdevice, but an
> > alternative approach would be that the i2c driver can register
> > itself with the bridge driver when loaded. There are actually some
> > advantages to that approach, but if I do that, then the lock is
> > definitely needed.
> >
> > > This comment kind of bugged me too:
> > > >/* Iterate over all subdevs. The next item is prefetched, so you
> > > > can + delete the current item if necessary. */
> > > > +#define v4l2_device_for_each_subdev(sd, dev)
> > >
> > > It implies it's safe to remove things from the list that we're
> > > not locking.
> >
> > Oops, that was a bogus comment. Actually, the whole macro is a bit
> > bogus and can simply be replaced by this:
> >
> > /* Iterate over all subdevs. */
> > #define v4l2_device_for_each_subdev(sd, dev) \
> > list_for_each_entry(sd, &(dev)->subdevs, list)
> >
> > > I can appreciate the desire for being able to walk the list and
> > > issue commands to various subdevs with high concurrency. I know
> > > spinlocks are optimized for the common case of the lock being
> > > available (well at least the underlying __raw_spin_lock() is
> > > optimized, if preemption is enabled there's a little overhead
> > > added). So they give the safety with a minor penalty at the
> > > micro level, but kill concurrency of common operations at the
> > > macro level.
> > >
> > > So how about a rwlock_t and using read_lock() and write_lock(),
> > > locks that provide safety and allow high concurrency at the macro
> > > level? I don't know if there's an analog of a read/write mutex.
> >
> > Note that I can't use spinlocks while walking the subdevs list
> > since the subdev ops that you call can sleep which is not allowed
> > with spinlocks (as I very quickly found out when I tried it :-) ).
>
> Ah.
>
> > The only way to safely walk over it is to switch to a klist, but I
> > really like to keep __v4l2_device_call_subdevs as fast as possible.
> > Since the registration/unregistration of subdevs is fully
> > controlled by the driver it is trivial for the driver to ensure
> > that there are no (un)registrations while the subdev list is being
> > walked. There are no external events that can cause this to happen.
> >
> > In addition, there is also no point for a driver to begin issuing
> > commands to subdevs while some of them are not yet registered. So I
> > feel confident about my approach.
>
> Just playing a little more devil's advocate:
>
> I have a recollection that the lirc_pvr150 module calls an ivtv
> function directly to reset the IR blaster chip on PVR-150's. I'd
> imagine in the future, the GPIO subdev in ivtv might implement some
> sort of reset_ops or ir_ops for a cleaner interface for an updated
> lirc_pvr150 module to call in on. There's my one contrived example,
> contingent on assumed changes, of a bridge chip driver not being
> strictly in control of when a subdev could be called.
>
> (It's moot in this case though as lirc_pvr150 would need ivtv to init
> i2c before gpio, which ivtv doesn't do, before lirc_pvr150 would
> attempt a reset of the IR blaster.)
>
> I think you're safe for now. :)
I think so too :-) Anyway, it's highly debatable whether that reset
thing for ivtv is still needed. I suspect it isn't.
> On a more philosophical note is GPIO really a single subdev or a
> collection of independent serial & discrete buses to a collection of
> subdevs? In cx18 depending on the board, GPIO can reset chips,
> change audio mux paths, change the state of LED's, and in the future
> be used as a serial line (if I ever get that soft-UART to the IR
> blaster implemented).
Definitely a collection of subdevs. Usually each chip driven by GPIO
would have its own subdev. So ivtv implements an audio muxer subdev,
but it might also have additional subdevs for other chips connected to
the GPIO lines. I haven't explored all the possibilities yet, but I
suspect that this can be a quite powerful solution.
Regards,
Hans
> > But if it turns out to be a problem after all in the future, then
> > it will be easy to replace the list with a klist.
>
> Well my one contrived example is between two modules who usage is
> tightly coupled at least in one direction, even if source code
> changes are not tightly coordinated. In that case I'd predict
> problems will simply be avoided due to the tight coupling.
>
> Regards,
> Andy
>
> > > Did I totally miss the concept?
> >
> > Not at all, and once again thanks for the comments.
> >
> > I've updated my tree with the changes mentioned above.
> >
> > Regards,
> >
> > Hans
> >
> > --
> > Hans Verkuil - video4linux developer - sponsored by TANDBERG
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG
On Sunday 30 November 2008, Hans Verkuil wrote:
> > On a more philosophical note is GPIO really a single subdev or a
> > collection of independent serial & discrete buses to a collection of
> > subdevs? ?In cx18 depending on the board, GPIO can reset chips,
> > change audio mux paths, change the state of LED's, and in the future
> > be used as a serial line (if I ever get that soft-UART to the IR
> > blaster implemented).
>
> Definitely a collection of subdevs. Usually each chip driven by GPIO
> would have its own subdev. So ivtv implements an audio muxer subdev,
> but it might also have additional subdevs for other chips connected to
> the GPIO lines. I haven't explored all the possibilities yet, but I
> suspect that this can be a quite powerful solution.
Right. GPIO is basically a glue tech ... the focus should be what's
being glued (the various subdevs, and their parents), not on the glue.
I think a standardized set of GPIO calls, and gpiolib, makes that a
lot easier nowadays; though maybe not always painless.
Keeping to the V4L2 topic: I figure there will be some issues there
to sort out. For example, it might be important to collect key
resources like GPIOs and regulators before setting up the various I2C
clients that rely on those for proper operation, or other parts of a
complex V4L2 "assembly" (to coin a term). I understand that in some
cases those GPIOs are provided through I2C-based expanders... such
sequencing issues will become clearer as the $SUBJECT work matures.
(I'd expect the v4l2_device would manage such stuff.)
One widely accessible OMAP example: the beagleboard.org hardware uses
a GPIO to enable its DVI output, and a regulator that must be enabled
for S-Video output. Both should be disabled when their output channel
is not in use -- software doesn't handle that yet though! -- and I'd
expect an OMAP3 v4l2_device would understand such board-specific setup
issues. (Support for that isn't yet in mainline though.)
- Dave