lkml.org 
[lkml]   [2008]   [Nov]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: v4l2_device/v4l2_subdev: please review (PATCH 1/3)
    Date
    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


    \
     
     \ /
      Last update: 2008-11-30 22:43    [W:0.090 / U:30.388 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site