From: Hans Verkuil <hverkuil@xs4all.nl>
To: Laurent Pinchart <laurent.pinchart@skynet.be>
Subject: Re: v4l2_device/v4l2_subdev: please review (PATCH 1/3)
Date: Sun, 30 Nov 2008 22:40:58 +0100
User-Agent: KMail/1.9.9
Cc: video4linux-list@redhat.com, "Trilok Soni" <soni.trilok@gmail.com>,
       "linux-omap@vger.kernel.org" <linux-omap@vger.kernel.org>,
       "davinci-linux-open-source@linux.davincidsp.com" 
	<davinci-linux-open-source@linux.davincidsp.com>,
       linux-kernel@vger.kernel.org
References: <200811242309.37489.hverkuil@xs4all.nl> <200811291519.39549.hverkuil@xs4all.nl> <200811302202.22163.laurent.pinchart@skynet.be>
In-Reply-To: <200811302202.22163.laurent.pinchart@skynet.be>
MIME-Version: 1.0
Content-Type: text/plain;
  charset="iso-8859-1"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline
Message-Id: <200811302240.59076.hverkuil@xs4all.nl>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 26344
Lines: 615

Hi Laurent,

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

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

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

That's correct.

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

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

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

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

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

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

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/