[PATCH] 2.6.0-test3 zoran driver update

[PATCH] 2.6.0-test3 zoran driver update

[Ronald Bultje]

Hi Linus/Andrew,

This is a patch for the video4linux unified zoran driver that has been
in the kernel since 2.4.5 or so. I've send it in a few times before but
it didn't go in yet (Alan promised to look into it, but he's on
sabbatical now, so I'll have to bug someone else :p). Could this patch
be considered please?
It fixes compile issues in the current 2.6.0-test3 unified zoran driver
(current one doesn't compile at all), and also updates its version to
what we have in CVS. This adds support for new cards (e.g. LML33R10 from
LinuxMediaLabs and DC30+ from Pinnacle), fixes bugs in cards that were
already supported and generally improves capture reliability. Changes
per file (in detail) are given below.

http://mjpeg.sf.net/driver-zoran/linux-zoran-driver.patch.gz contains
the patch. Uncompressed size is 740kB, compressed size is 164 kB (so no
attaching here). MD5SUM is 96e4c566c9786f07b26707f2155145ee.

Please consider this. Waiting longer won't fix the broken driver that's
currently in your tree. :).

Thanks,
Ronald

----- detailed changes -----

Changes for each relevant part:
i2c-id.h:
add some new IDs for new i2c drivers

pci_ids.h:
add PCI IDs for each of the supported cards if it has any

saa7111.c, saa7110.c, adv7175.c, bt819.c, saa7185.c, bt856.c:
update to whatever we've got in our CVS. For most, these are just
"easiness" fixes that either add some better debug output, or that make
maintainance for both 2.6.x and 2.4.x simpler for me. There's also some
specific changes. E.g., in saa7110.c, we enable the VCR mode bits so we
get a better image from VCR input. In all of them, we make debugging an
insmod option rather than a compile-time option (this makes debugging
for users a *lot* easier). Point is that I just want our latest CVS in
here. Maintainance is going to be a personal horror-story if it's not.

vpx3220.c, saa7114.c, adv7170.c:
new i2c ones (for respectively DC10/DC30, LML33R10 and again LML33R10)

zr36067.c:
removed, the driver is now spread over multiple source files.

zoran*.[ch], zr36057.h
spread-out source files. Also fixes lots of bugs, can't even start
naming them all, you don't want that, neither do I. Just assume that it
works better than it used to - it does.
Nice things that aren't in the old driver: much more stable, supports
DC30+, supports LML33R10, has proper locking/semaphores, supports
multiple opens without races now, adapted to new i2c subsystem, v4l2
support, Xv (hardware-scaled) overlay support, and a lot more.
Oh, and this one actually compiles.

videocodec.[ch], zr360{16,50,60}.[ch]:
new sublayer for the driver to unify how we handle the zr36060 chip
(DC10(+)/LML33/LML33R10/Buz) and the zr36050/zr36016 (DC30(+)).

MAINTAINERS:
add me as maintainer (I am).

Kconfig:
add new cards, plus improve the descriptions.

Makefile:
d'oh.

Zoran:
documentation update.

Others:
I don't think there are any.

Greg has gone over the i2c changes a long time ago, he agreed on all of
them. Gerd is supposed to take care of the v4l part, he has never
complained about any of them. Stability is good, we've fixed most issues
(there's still some out there, but nothing serious), lots better than in
the old driver. Also, the cards work much better than with the old
driver.

In short, I think this is worth applying, even though it's a huge beast.

-- 
Ronald Bultje <rbultje@ronald.bitfreak.net>

Re: [PATCH] 2.6.0-test3 zoran driver update

[Francois Romieu]

Ronald Bultje <rbultje@ronald.bitfreak.net> :
[...]

- {adv7170/adv7175/bt819/saa7110/saa7185}_detect_client()
  for each of these functions, two error exit path leak on locally allocated
  variable "channel".

- {adv7170/adv7175/bt819/saa7111/saa7185}_write_block()
  The code duplication could surely be avoided.

- always put a blank line between variables declaration and code pleae

- find_zr36057():
  what about replacing pci_find_device() by the modern pci insertion/removal
  api (which has been standing there for ~3 years)

- pci_enable_device() in find_zr36057() isn't balanced by pci_disable_device()
  in zoran_release()

- +irqreturn_t
  +zoran_irq (int             irq,
[...]
  +                                               for (i = 0; i < 4; i++) {
  +                                                       if (zr->
  +                                                           stat_com[i] &
  +                                                           1)
  +                                                               sv[i] =
  +                                                                   '1';
  +                                               }

  Post-modernism ?


It looks interesting.

--
Ueimor

Re: [PATCH] 2.6.0-test3 zoran driver update

[Ronald Bultje]

Hi Francois,

thanks for the comments, I'll definately look at them. One thing is
unclear:

On Thu, 2003-08-21 at 01:08, Francois Romieu wrote:
> - {adv7170/adv7175/bt819/saa7110/saa7185}_detect_client()
>   for each of these functions, two error exit path leak on locally allocated
>   variable "channel".

There is no variable 'channel' in these functions. I've double checked
these functions, too, and can't find any obvious leaks in them at all.
Could you please re-check?

Thanks again,
Ronald

-- 
Ronald Bultje <rbultje@ronald.bitfreak.net>

Re: [PATCH] 2.6.0-test3 zoran driver update

[Francois Romieu]

Ronald Bultje <rbultje@ronald.bitfreak.net> :
[...]
> There is no variable 'channel' in these functions. I've double checked
> these functions, too, and can't find any obvious leaks in them at all.
> Could you please re-check?

Oops, cerebral traffic jam.

It should have read "client", not "channel".

--
Ueimor

Re: [PATCH] 2.6.0-test3 zoran driver update

[Ronald Bultje]

Hi Francois,

On Thu, 2003-08-21 at 13:01, Francois Romieu wrote:
> Ronald Bultje <rbultje@ronald.bitfreak.net> :
> > There is no variable 'channel' in these functions. I've double checked
> > these functions, too, and can't find any obvious leaks in them at all.
> > Could you please re-check?
> 
> Oops, cerebral traffic jam.
> 
> It should have read "client", not "channel".

Got it, thanks. Patches will show up in a few days.

Ronald

-- 
Ronald Bultje <rbultje@ronald.bitfreak.net>
Linux Video/Multimedia developer

Re: [PATCH] 2.6.0-test3 zoran driver update

[Ronald Bultje]

Hey Francois,

coming back to your other points now. (This means I've worked on them.
;) ).

On Thu, 2003-08-21 at 01:08, Francois Romieu wrote:
> - {adv7170/adv7175/bt819/saa7110/saa7185}_detect_client()
>   for each of these functions, two error exit path leak on locally allocated
>   variable "channel".

That one was fixed already, patch coming up.

> - {adv7170/adv7175/bt819/saa7111/saa7185}_write_block()
>   The code duplication could surely be avoided.

I'm wondering how. I could make an inline function that each of them
includes (but that doesn't decrease binary size, code is still
duplicated), or I could make a parent module (and I don't want to do
that). The only bad thing of the current way is the maintainance, but I
don't really mind.

Does it matter if I just keep it the way it is right now? I don't really
mind at all.

> - always put a blank line between variables declaration and code pleae

Patch coming up. It fixes most of them. I might have missed one or two.

> - find_zr36057():
>   what about replacing pci_find_device() by the modern pci insertion/removal
>   api (which has been standing there for ~3 years)

That's planned (also, we aren't conforming to the latest DMA API yet,
that's planned for future fixage, too). I can't really fix that
short-term, though, I'm affraid...

> - pci_enable_device() in find_zr36057() isn't balanced by pci_disable_device()
>   in zoran_release()

Patch coming up...

> - +irqreturn_t
>   +zoran_irq (int             irq,
> [...]
>   +                                               for (i = 0; i < 4; i++) {
>   +                                                       if (zr->
>   +                                                           stat_com[i] &
>   +                                                           1)
>   +                                                               sv[i] =
>   +                                                                   '1';
>   +                                               }
> 
>   Post-modernism ?

Uh yes, that's an indent artifact, we occassionaly find that in some
places. I started with a codebase that hadn't been maintained for a year
or so, and it used 3 coding styles, so I fixed that by just running
indent -kr over it and fixing the most obvious style bugs as I
encountered them. However, I'm sure I missed some, like this. If you
find more, please let me know. Patch coming up. :).

> It looks interesting.

Thanks, and thanks for the comments, too.

I'm moving some things explicitely to the long-term list, btw, that's on
purpose. It'd be cool to handle the new DMA/PCI subsystem, but we've got
many things to work on, some of which are more important than this.
Also, kernel modules isn't all I'm working on, and this isn't my job. So
it's no deinterest or anything, it's simply a matter of (lack of) time
in combination with a bit of realism. I'll work on it, just not today.
:).

Ronald

-- 
Ronald Bultje <rbultje@ronald.bitfreak.net>

Re: [PATCH] 2.6.0-test3 zoran driver update

[Francois Romieu]

Ronald Bultje <rbultje@ronald.bitfreak.net> :
[avoid code duplication in {adv7170/adv7175/bt819/saa7111/saa7185}_write_block]
> I'm wondering how. I could make an inline function that each of them
> includes (but that doesn't decrease binary size, code is still
> duplicated), or I could make a parent module (and I don't want to do
> that). The only bad thing of the current way is the maintainance, but I
> don't really mind.

These functions differ only in:

struct XXX *encoder = i2c_get_clientdata(client);
       ^^^
[...]
                        do {
                                block_data[msg.len++] =
                                    encoder->reg[reg++] = data[1];
                                    ^^^^^^^^^ 
[...]
                        if ((ret =
                             XXX_write(client, reg, *data++)) < 0)
                             ^^^^^^^^^

Make i2c_get_clientdata() return a generic container struct which
provides instance specific methods to retrieve the "reg" member and
the XXX_write() function. Init these in XXX_detect_client() just
before i2c_set_clientdata() and it is done.

Patches that remove code used to make Penguin #0 happy. We want an happy
Penguin #0, don't we ? :o)

> Does it matter if I just keep it the way it is right now? I don't really
> mind at all.

Your choice. As a maintainer, you can say "It would be great. Volunteer
anyone ?" and see voluteer appearing.

[...]
> I'm moving some things explicitely to the long-term list, btw, that's on
> purpose. It'd be cool to handle the new DMA/PCI subsystem, but we've got
> many things to work on, some of which are more important than this.

Ok, I'll check http://mjpeg.sourceforge.net/driver-zoran/development.php and
try to kill one or two simple, self-contained, tasks.

[part-time kernel involvement]

Same thing here.

Regards

--
Ueimor