Hi Pauline,
[CC: to kernel, since it matters a bit there...]
On Thu, 2003-09-25 at 12:26, Pauline Middelink wrote:
> Regarding the patch: would be nice.
It was easier than I thought (all code is in zr36120.c, the i2c
(zr36120_i2c.c) was a piece of cake and zr36120_mem.c is untouched). See
attachment. I have no clue whether it works, but this is how my driver
does it.
Changes:
* move to new i2c
* move to new videodev
* implement a zoran_vdev_release() function to free memory after it's no
longer in use (this is a hack imo, but it's the suggested way of doing
it... There was an article somewhere about it and it's also been
discussed on the v4l mailinglist... Can't find a link, though)
* proper module use-counting
* changed a __put_user() call somewhere because it failed to compile
here - not sure whether this is a real issue or just a local error
(RH90)... Probably some macro change in 2.6.x. It still does the same
thing, but in a different way.
* add zr36120 i2c HW id
* it compiles in my local 2.6.x tree
It accepts I2C_DRIVERID_TUNER as tuner (tuner.ko) and
I2C_DRIVERID_SAA7110 as decoder (saa7110.ko). If there's any others that
need adding, simply add them to the case: list.
Things left to do (for you ;) ): v4l2 (sounds like a good thing, though
it'll take some time), multiple opens (if you want), and (IIRC, Gerd?)
videodev + vbidev fops should be the same. This wasn't the case in
2.4.x, but it's the case in 2.6.x, I think. I'm actually not totally
sure, I think Gerd knows more about this, and can give you some pointers
to how others have done this.
Please let me know the results. :).
Ronald
--
Ronald Bultje <[email protected]>
Linux Video/Multimedia developer
On Thu, Sep 25, 2003 at 05:59:44PM +0200, Ronald Bultje wrote:
> + if (!try_module_get(THIS_MODULE)) {
> + printk(KERN_ERR "failed to acquire lock on myself\n");
> + return -EIO;
> + }
This is broken, you need an owner outside the open routine.
> +
> + /* find the device */
> + for (i = 0; i < zoran_cards; i++) {
> + if (zorans[i].video_dev->minor == minor) {
> + ztv = &zorans[i];
> + break;
> + }
> + }
What serializes this?
> + if (ztv->have_decoder &&
> + !try_module_get(ztv->decoder->driver->owner)) {
> + printk(KERN_ERR "Failed to acquire lock on TV decoder\n");
> + module_put(THIS_MODULE);
> + return -EIO;
> + }
> + if (ztv->have_tuner &&
> + !try_module_get(ztv->tuner->driver->owner)) {
> + printk(KERN_ERR "Failed to acquire lock on TV tuner\n");
> + if (ztv->have_decoder)
> + module_put(ztv->decoder->driver->owner);
> + module_put(THIS_MODULE);
> + return -EIO;
> + }
You probably want to add a few gotos to unwind at the end of the
function with less code deuplication..
> * implement a zoran_vdev_release() function to free memory after it's no
> longer in use (this is a hack imo, but it's the suggested way of doing
> it... There was an article somewhere about it and it's also been
> discussed on the v4l mailinglist... Can't find a link, though)
If you just kfree() there you don't need your own bur can simply use
video_device_release() ...
> Things left to do (for you ;) ): v4l2 (sounds like a good thing, though
> it'll take some time), multiple opens (if you want), and (IIRC, Gerd?)
> videodev + vbidev fops should be the same. This wasn't the case in
> 2.4.x, but it's the case in 2.6.x, I think.
That's a v4l vs. v4l2 API thing. The v4l2 API allows applications to
switch a device handle into vbi mode via S_FMT, thus you don't need
separate devices any more. For v4l1 backward comparibility this is
still needed through. For v4l2 drivers /dev/video and /dev/vbi almost
the same, /dev/vbi has just some other default settings after opening
the device.
Gerd
Hi,
On Thu, 2003-09-25 at 18:10, Christoph Hellwig wrote:
> On Thu, Sep 25, 2003 at 05:59:44PM +0200, Ronald Bultje wrote:
> > + if (!try_module_get(THIS_MODULE)) {
> > + printk(KERN_ERR "failed to acquire lock on myself\n");
> > + return -EIO;
> > + }
>
> This is broken, you need an owner outside the open routine.
Eh? could you explain, please?
> > +
> > + /* find the device */
> > + for (i = 0; i < zoran_cards; i++) {
> > + if (zorans[i].video_dev->minor == minor) {
> > + ztv = &zorans[i];
> > + break;
> > + }
> > + }
>
> What serializes this?
We have an array of devices, and in order to find the right one, we go
through the array until the inode belonging to the device that's being
opened matches the one in the video device that we reigstered during
module loading. I don't know of a way to get that information in any
direct (non-loop) way, but that might be because I never cared to. I
admit it's ugly, but it's correct and used in other drivers, too.
Ronald
--
Ronald Bultje <[email protected]>
Linux Video/Multimedia developer