2008-10-22 21:15:24

by Markus Rechberger

[permalink] [raw]
Subject: [PATCH 1/7] Adding empia base driver

em2880-dvb:
* supporting the digital part of Empia based devices, which
includes ATSC, ISDB-T and DVB-T

em28xx-aad.c:
* alternative audio driver, can be used instead of em28xx-audio if
alsa is not available
or not compiled into the kernel, it provides a raw interface to
the PCM samples

em28xx-audio.c:
* em28xx alsa driver and audio driver for FM radio

em28xx-audioep.c:
* em28xx alsa driver for devices which are set to vendor specific
audio on interface 1,
in that case snd-usb-audio will not attach to the interface and
em28xx-audioep will be needed

em28xx-cards.c:
* card definition and initial setup of devices.

em28xx-core.c:
* core videohandling and VBI frame slicing

em28xx-i2c.c:
* i2c setup and GPIO setup handling of the devices (including
em2888 based ones)

em28xx-input.c:
* currently mostly disabled since the linuxtv input handling is
broken by design and racy

em28xx-keymaps.c:
* keymap references of some remotes (could be merged into
ir-common, although as mentioned
this should be in userland done by lirc).

em28xx-video.c:
* inode handling for analog TV, radio and VBI, also some device probing

em28xx-webcam.c:
* videology webcam specific i2c commands


Attachments:
(No filename) (1.25 kB)
em28xx-01.diff (418.66 kB)
Download all attachments

2008-10-22 22:12:45

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH 1/7] Adding empia base driver

On Wed, Oct 22, 2008 at 11:14:36PM +0200, Markus Rechberger wrote:
> em2880-dvb:
> * supporting the digital part of Empia based devices, which
> includes ATSC, ISDB-T and DVB-T

<snip>

Doesn't this driver duplicate some of the existing devices we already
support with the current in-kernel driver? If so, why not just add the
new device support to the existing driver instead of duplicating
everything?

This is going to cause a big problem for distros as they will not know
which to enable, so they will probably just disable this one, which is
what I don't think you want to have happen :(

thanks,

greg k-h

2008-10-22 22:24:44

by Markus Rechberger

[permalink] [raw]
Subject: Re: [PATCH 1/7] Adding empia base driver

On Thu, Oct 23, 2008 at 12:09 AM, Greg KH <[email protected]> wrote:
> On Wed, Oct 22, 2008 at 11:14:36PM +0200, Markus Rechberger wrote:
>> em2880-dvb:
>> * supporting the digital part of Empia based devices, which
>> includes ATSC, ISDB-T and DVB-T
>
> <snip>
>
> Doesn't this driver duplicate some of the existing devices we already
> support with the current in-kernel driver? If so, why not just add the
> new device support to the existing driver instead of duplicating
> everything?
>
> This is going to cause a big problem for distros as they will not know
> which to enable, so they will probably just disable this one, which is
> what I don't think you want to have happen :(
>

the current driver doesn't support most devices which are in there,
also the alsa
audio driver can easily crash the whole system. (It's my code so I
know what was wrong there).

The core video code is already too much off, the VBI code added alot complexity
to it it does frame slicing on the fly.

Those devices ship VBI+VIDEO within 1 datastream, VBI and Video aren't
that different
in the system. both interfaces provide framebuffers through a mmap'ed interface.
If all the VBI buffers are filled the data has to be sliced off in any
case while providing
the same bottom data ot the Video interface

http://mcentral.de/hg/~mrec/em28xx-new/shortlog (there are more than
200 changelog entries
what happened in detail).

The development has been split off (first due limitations in the
kernel, afterwards due ..., and finally
due the restriction that all that has to work without a framework
upgrade on the eeePC).

diffing the 2 available drivers shows up that only the core is twice
as big as the one which is currently
in the driver (the result of 2-3 years asynchronous development).

The driver is currently also tested with signal generators (different
inputs, and different video standards).

Very likely the best would be to replace the available driver with it
but I don't care, alot people use and have been using the driver from
mcentral.de for a long time, development has always been opensource
there too.

regards,
Markus

2008-10-22 22:26:27

by Markus Rechberger

[permalink] [raw]
Subject: Re: [PATCH 1/7] Adding empia base driver

On Thu, Oct 23, 2008 at 12:24 AM, Markus Rechberger
<[email protected]> wrote:
> On Thu, Oct 23, 2008 at 12:09 AM, Greg KH <[email protected]> wrote:
>> On Wed, Oct 22, 2008 at 11:14:36PM +0200, Markus Rechberger wrote:
>>> em2880-dvb:
>>> * supporting the digital part of Empia based devices, which
>>> includes ATSC, ISDB-T and DVB-T
>>
>> <snip>
>>
>> Doesn't this driver duplicate some of the existing devices we already
>> support with the current in-kernel driver? If so, why not just add the
>> new device support to the existing driver instead of duplicating
>> everything?
>>
>> This is going to cause a big problem for distros as they will not know
>> which to enable, so they will probably just disable this one, which is
>> what I don't think you want to have happen :(
>>
>
> the current driver doesn't support most devices which are in there,
> also the alsa
> audio driver can easily crash the whole system. (It's my code so I
> know what was wrong there).
>

since the USB IDs were taken from the driver from mcentral.de without
testing them at all.

> The core video code is already too much off, the VBI code added alot complexity
> to it it does frame slicing on the fly.
>
> Those devices ship VBI+VIDEO within 1 datastream, VBI and Video aren't
> that different
> in the system. both interfaces provide framebuffers through a mmap'ed interface.
> If all the VBI buffers are filled the data has to be sliced off in any
> case while providing
> the same bottom data ot the Video interface
>
> http://mcentral.de/hg/~mrec/em28xx-new/shortlog (there are more than
> 200 changelog entries
> what happened in detail).
>
> The development has been split off (first due limitations in the
> kernel, afterwards due ..., and finally
> due the restriction that all that has to work without a framework
> upgrade on the eeePC).
>
> diffing the 2 available drivers shows up that only the core is twice
> as big as the one which is currently
> in the driver (the result of 2-3 years asynchronous development).
>
> The driver is currently also tested with signal generators (different
> inputs, and different video standards).
>
> Very likely the best would be to replace the available driver with it
> but I don't care, alot people use and have been using the driver from
> mcentral.de for a long time, development has always been opensource
> there too.
>
> regards,
> Markus
>

2008-10-22 22:31:42

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH 1/7] Adding empia base driver

On Thu, Oct 23, 2008 at 12:24:31AM +0200, Markus Rechberger wrote:
> On Thu, Oct 23, 2008 at 12:09 AM, Greg KH <[email protected]> wrote:
> > On Wed, Oct 22, 2008 at 11:14:36PM +0200, Markus Rechberger wrote:
> >> em2880-dvb:
> >> * supporting the digital part of Empia based devices, which
> >> includes ATSC, ISDB-T and DVB-T
> >
> > <snip>
> >
> > Doesn't this driver duplicate some of the existing devices we already
> > support with the current in-kernel driver? If so, why not just add the
> > new device support to the existing driver instead of duplicating
> > everything?
> >
> > This is going to cause a big problem for distros as they will not know
> > which to enable, so they will probably just disable this one, which is
> > what I don't think you want to have happen :(
> >
>
> the current driver doesn't support most devices which are in there,

Then why not just add new device support to the existing one?

> also the alsa audio driver can easily crash the whole system. (It's my
> code so I know what was wrong there).

Why not send patches to fix it?

> Very likely the best would be to replace the available driver with it
> but I don't care, alot people use and have been using the driver from
> mcentral.de for a long time, development has always been opensource
> there too.

Dropping existing code and replacing it entirely with a new base is not
how Linux kernel development works for the most part.

How about just sending patches in an incremental way, fixing problems in
the current driver that you know about, and adding support for all of
this goodness as well? That's what all 2000+ other kernel developers
do when they want to make changes like this.

thanks,

greg k-h

2008-10-22 22:36:11

by Markus Rechberger

[permalink] [raw]
Subject: Re: [PATCH 1/7] Adding empia base driver

On Thu, Oct 23, 2008 at 12:27 AM, Greg KH <[email protected]> wrote:
> On Thu, Oct 23, 2008 at 12:24:31AM +0200, Markus Rechberger wrote:
>> On Thu, Oct 23, 2008 at 12:09 AM, Greg KH <[email protected]> wrote:
>> > On Wed, Oct 22, 2008 at 11:14:36PM +0200, Markus Rechberger wrote:
>> >> em2880-dvb:
>> >> * supporting the digital part of Empia based devices, which
>> >> includes ATSC, ISDB-T and DVB-T
>> >
>> > <snip>
>> >
>> > Doesn't this driver duplicate some of the existing devices we already
>> > support with the current in-kernel driver? If so, why not just add the
>> > new device support to the existing driver instead of duplicating
>> > everything?
>> >
>> > This is going to cause a big problem for distros as they will not know
>> > which to enable, so they will probably just disable this one, which is
>> > what I don't think you want to have happen :(
>> >
>>
>> the current driver doesn't support most devices which are in there,
>
> Then why not just add new device support to the existing one?
>
>> also the alsa audio driver can easily crash the whole system. (It's my
>> code so I know what was wrong there).
>
> Why not send patches to fix it?
>

that's why I'm sending that request at the moment, development still goes on
on my side.

>> Very likely the best would be to replace the available driver with it
>> but I don't care, alot people use and have been using the driver from
>> mcentral.de for a long time, development has always been opensource
>> there too.
>
> Dropping existing code and replacing it entirely with a new base is not
> how Linux kernel development works for the most part.
>

the patch adds the driver from mcentral.de as alternative entry, not
the best way
but it currently supports almost all devices which have an entry in there.
2 or 3 entries definitely don't work yet due missing external drivers
which would
come in later.

> How about just sending patches in an incremental way, fixing problems in
> the current driver that you know about, and adding support for all of
> this goodness as well? That's what all 2000+ other kernel developers
> do when they want to make changes like this.
>

I understand what you mean although too many things went from from the
beginning on
and people were in general not participating at discussions, it
slightly changed now but
the codebase evolved over time.

I'll be happy to do so for upcoming patches.

Markus

2008-10-22 22:53:50

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH 1/7] Adding empia base driver

On Thu, Oct 23, 2008 at 12:35:54AM +0200, Markus Rechberger wrote:
> On Thu, Oct 23, 2008 at 12:27 AM, Greg KH <[email protected]> wrote:
> > On Thu, Oct 23, 2008 at 12:24:31AM +0200, Markus Rechberger wrote:
> >> On Thu, Oct 23, 2008 at 12:09 AM, Greg KH <[email protected]> wrote:
> >> > On Wed, Oct 22, 2008 at 11:14:36PM +0200, Markus Rechberger wrote:
> >> >> em2880-dvb:
> >> >> * supporting the digital part of Empia based devices, which
> >> >> includes ATSC, ISDB-T and DVB-T
> >> >
> >> > <snip>
> >> >
> >> > Doesn't this driver duplicate some of the existing devices we already
> >> > support with the current in-kernel driver? If so, why not just add the
> >> > new device support to the existing driver instead of duplicating
> >> > everything?
> >> >
> >> > This is going to cause a big problem for distros as they will not know
> >> > which to enable, so they will probably just disable this one, which is
> >> > what I don't think you want to have happen :(
> >> >
> >>
> >> the current driver doesn't support most devices which are in there,
> >
> > Then why not just add new device support to the existing one?
> >
> >> also the alsa audio driver can easily crash the whole system. (It's my
> >> code so I know what was wrong there).
> >
> > Why not send patches to fix it?
> >
>
> that's why I'm sending that request at the moment, development still goes on
> on my side.

I don't understand, I don't see patches here to fix the existing
problems in the existing driver. Am I missing something?

> >> Very likely the best would be to replace the available driver with it
> >> but I don't care, alot people use and have been using the driver from
> >> mcentral.de for a long time, development has always been opensource
> >> there too.
> >
> > Dropping existing code and replacing it entirely with a new base is not
> > how Linux kernel development works for the most part.
> >
>
> the patch adds the driver from mcentral.de as alternative entry, not
> the best way but it currently supports almost all devices which have
> an entry in there.

What do you mean by "alternative" entry? Our device/driver matching
system doesn't allow for "alternates" it is a "first one there wins"
type system, which, while isn't the nicest, is what it is. This driver
is going to cause problems for distros, and for users, as if you build
them both as modules, it is pseudo-random which one will end up being
loaded first and binding to the device.

Which is not something we generally want to do to our users at all.

> > How about just sending patches in an incremental way, fixing problems in
> > the current driver that you know about, and adding support for all of
> > this goodness as well? That's what all 2000+ other kernel developers
> > do when they want to make changes like this.
> >
>
> I understand what you mean although too many things went from from the
> beginning on and people were in general not participating at
> discussions, it slightly changed now but the codebase evolved over
> time.
>
> I'll be happy to do so for upcoming patches.

So we should drop our current rules for you, and trust that things from
here on out will be correct? That doesn't sound like a good idea for us
to do, would you recommend doing something like that for someone else?

Especially given the above mentioned problems for users, this really
isn't going to work out.

Please send patches that evolve the current drivers into supporting
these new devices so that our development model is followed, and no user
ends up in trouble.

thanks,

greg k-h

2008-10-23 08:53:20

by el es

[permalink] [raw]
Subject: Re: [PATCH 1/7] Adding empia base driver

Greg KH <greg <at> kroah.com> writes:


> Please send patches that evolve the current drivers into supporting
> these new devices so that our development model is followed, and no user
> ends up in trouble.
>
> thanks,
>
> greg k-h
>

(just my £.2, user's perspective warning)
Just to say this, the current in-tree driver doesn't support the device I own,
Markus' driver does.

Greg :
The code Markus is requesting to merge is a completely NEW driver comparing to
the one that is in the tree (has a long and painful history). I think you might
safely assume that it already is NOT based on the old driver, tough shares the
name, due to the changes (and history).

Markus : just an idea - maybe if you've renamed the driver, to reflect the fact,
that it is completely new code, and schedule the old one to be deprecated ?

Lukasz

2008-10-23 09:30:28

by Alan

[permalink] [raw]
Subject: Re: [PATCH 1/7] Adding empia base driver

> This is going to cause a big problem for distros as they will not know
> which to enable, so they will probably just disable this one, which is
> what I don't think you want to have happen :(

Before looking into any of this talk to the V4L and DVB maintainers,
browse video4linux list and have a happy read of stuff like:

http://thread.gmane.org/gmane.linux.drivers.dvb/35454


Now I wonder why Markus didn't cc Mauro on this despite Mauro being
maintainer.

Alan

2008-10-23 11:11:05

by Markus Rechberger

[permalink] [raw]
Subject: Re: [PATCH 1/7] Adding empia base driver

On Thu, Oct 23, 2008 at 5:29 AM, Alan Cox <[email protected]> wrote:
>> This is going to cause a big problem for distros as they will not know
>> which to enable, so they will probably just disable this one, which is
>> what I don't think you want to have happen :(
>
> Before looking into any of this talk to the V4L and DVB maintainers,
> browse video4linux list and have a happy read of stuff like:
>
> http://thread.gmane.org/gmane.linux.drivers.dvb/35454
>

There's a much much longer history about that, this can safely be ignored.
The history goes back 3 years .. this one only goes back a couple of months.
It will take at least a week for a single person to understand the
whole issue from the beginning on, although I'd say it's not relevant.

In case of Userspace tuners, see cx88 freeBSD driver the project is
growing, netBSD
is also using parts of that em28xx driver - there was no complaint at
all about it.
Some people don't like that idea in linux so it's ok and it has been
redone, no big deal anymore.

Markus

>
> Now I wonder why Markus didn't cc Mauro on this despite Mauro being
> maintainer.
>
> Alan
>

2008-10-24 20:16:19

by Markus Rechberger

[permalink] [raw]
Subject: Re: [PATCH 1/7] Adding empia base driver

On Fri, Oct 24, 2008 at 7:35 PM, Mauro Carvalho Chehab
<[email protected]> wrote:
> On Wed, 22 Oct 2008 22:59:00 +0200
> "Markus Rechberger" <[email protected]> wrote:
>
>> em2880-dvb:
>> * supporting the digital part of Empia based devices, which
>> includes ATSC, ISDB-T and DVB-T
>>
>> em28xx-aad.c:
>> * alternative audio driver, can be used instead of em28xx-audio if
>> alsa is not available
>> or not compiled into the kernel, it provides a raw interface to
>> the PCM samples
>>
>> em28xx-audio.c:
>> * em28xx alsa driver and audio driver for FM radio
>>
>> em28xx-audioep.c:
>> * em28xx alsa driver for devices which are set to vendor specific
>> audio on interface 1,
>> in that case snd-usb-audio will not attach to the interface and
>> em28xx-audioep will be needed
>>
>> em28xx-cards.c:
>> * card definition and initial setup of devices.
>>
>> em28xx-core.c:
>> * core videohandling and VBI frame slicing
>>
>> em28xx-i2c.c:
>> * i2c setup and GPIO setup handling of the devices (including
>> em2888 based ones)
>>
>> em28xx-input.c:
>> * currently mostly disabled since the linuxtv input handling is
>> broken by design and racy
>>
>> em28xx-keymaps.c:
>> * keymap references of some remotes (could be merged into
>> ir-common, although as mentioned
>> this should be in userland done by lirc).
>>
>> em28xx-video.c:
>> * inode handling for analog TV, radio and VBI, also some device probing
>>
>> em28xx-webcam.c:
>> * videology webcam specific i2c commands
>
> NACK.
>
> There's already a driver for em28xx. Be welcome sending incremental patches to
> improve, like other developers do. But another driver for the same chip would
> just create a mess.
>

the description right above shows up what the current driver is
missing, excluding the bugfixes.
Technical reasons why the source is basically kept in the structure as
it is, is eg. higher backward
compatibility without having to update the whole system. See eeePC
packages which are available
from some vendors. The driver seamlessly works with the rest which is
installed there, without
any framework upgrade.

Most users are currently using the drivers from mcentral.de since it's
more stable and very
well tested over the last 3 years. It was basically your decision to
not merge it back then
http://mcentral.de/v4l-dvb/

I pulled out the source and moved it together then and worked on
additional device support.

http://mcentral.de/hg/~mrec/em28xx-new/shortlog
There are more than 200 changesets pointing out how it evolved, if
someone wants to have an indepth
view about it. Bugreports and patches have been posted to the em28xx
mailinglist where people worked
on it, including enduser applications.

The xc3028 as it is in the kernel is based on leaked and partly
reverse engineered information.
I know that because I was also CC'ed with the leaked driver information.

The Xceive drivers which I submitted are the latest versions from
Xceive addressing several bugs,
you might not have access to their changelog.

Before continuing any discussion the sourcecode and every statement I
made for those patchsets
should be commented, otherwise a discussion won't go anywhere.

br,
Markus

2008-10-24 20:20:24

by Markus Rechberger

[permalink] [raw]
Subject: Re: [PATCH 1/7] Adding empia base driver

On Fri, Oct 24, 2008 at 10:15 PM, Markus Rechberger
<[email protected]> wrote:
> On Fri, Oct 24, 2008 at 7:35 PM, Mauro Carvalho Chehab
> <[email protected]> wrote:
>> On Wed, 22 Oct 2008 22:59:00 +0200
>> "Markus Rechberger" <[email protected]> wrote:
>>
>>> em2880-dvb:
>>> * supporting the digital part of Empia based devices, which
>>> includes ATSC, ISDB-T and DVB-T
>>>
>>> em28xx-aad.c:
>>> * alternative audio driver, can be used instead of em28xx-audio if
>>> alsa is not available
>>> or not compiled into the kernel, it provides a raw interface to
>>> the PCM samples
>>>
>>> em28xx-audio.c:
>>> * em28xx alsa driver and audio driver for FM radio
>>>
>>> em28xx-audioep.c:
>>> * em28xx alsa driver for devices which are set to vendor specific
>>> audio on interface 1,
>>> in that case snd-usb-audio will not attach to the interface and
>>> em28xx-audioep will be needed
>>>
>>> em28xx-cards.c:
>>> * card definition and initial setup of devices.
>>>
>>> em28xx-core.c:
>>> * core videohandling and VBI frame slicing
>>>
>>> em28xx-i2c.c:
>>> * i2c setup and GPIO setup handling of the devices (including
>>> em2888 based ones)
>>>
>>> em28xx-input.c:
>>> * currently mostly disabled since the linuxtv input handling is
>>> broken by design and racy
>>>
>>> em28xx-keymaps.c:
>>> * keymap references of some remotes (could be merged into
>>> ir-common, although as mentioned
>>> this should be in userland done by lirc).
>>>
>>> em28xx-video.c:
>>> * inode handling for analog TV, radio and VBI, also some device probing
>>>
>>> em28xx-webcam.c:
>>> * videology webcam specific i2c commands
>>
>> NACK.
>>
>> There's already a driver for em28xx. Be welcome sending incremental patches to
>> improve, like other developers do. But another driver for the same chip would
>> just create a mess.
>>
>
> the description right above shows up what the current driver is
> missing, excluding the bugfixes.
> Technical reasons why the source is basically kept in the structure as
> it is, is eg. higher backward
> compatibility without having to update the whole system. See eeePC
> packages which are available
> from some vendors. The driver seamlessly works with the rest which is
> installed there, without
> any framework upgrade.
>
> Most users are currently using the drivers from mcentral.de since it's
> more stable and very
> well tested over the last 3 years. It was basically your decision to
> not merge it back then
> http://mcentral.de/v4l-dvb/
>
> I pulled out the source and moved it together then and worked on
> additional device support.
>
> http://mcentral.de/hg/~mrec/em28xx-new/shortlog
> There are more than 200 changesets pointing out how it evolved, if
> someone wants to have an indepth
> view about it. Bugreports and patches have been posted to the em28xx
> mailinglist where people worked
> on it, including enduser applications.
>
> The xc3028 as it is in the kernel is based on leaked and partly
> reverse engineered information.
> I know that because I was also CC'ed with the leaked driver information.
>
> The Xceive drivers which I submitted are the latest versions from
> Xceive addressing several bugs,
> you might not have access to their changelog.
>
> Before continuing any discussion the sourcecode and every statement I
> made for those patchsets
> should be commented, otherwise a discussion won't go anywhere.
>

*to avoid any misunderstanding here, this is not about using
chipdrivers from userspace*

> br,
> Markus
>

2008-11-01 13:59:40

by Hans Verkuil

[permalink] [raw]
Subject: Re: [PATCH 1/7] Adding empia base driver

Hi Markus,

As promised I've done a review of your empia driver and looked at what
needs to be done to get it into the kernel.

First of all, I've no doubt that your empia driver is better and
supports more devices than the current em28xx driver. I also have no
problem adding your driver separate from the current driver. It's been
done before (certain networking drivers spring to mind) and while
obviously not ideal I expect that the older em28xx driver can probably
be removed after a year or something like that.

In my opinion it's pretty much hopeless trying to convert the current
em28xx driver into what you have. It's a huge amount of work that no
one wants to do and (in this case) with very little benefit. Of course,
Mauro has the final say in this.

While there is some cleaning up to do in your code (coding style, some
copyright/license clarifications), that is not a big deal. The coding
style cleanups are a fair amount of work, but I think we can probably
spread that out over several people and get that done quickly.

What I definitely would recommend is to use video_ioctl2 rather than
video_usercopy. The latter function will disappear in the future. I
think the policy for new drivers is to use video_ioctl2, so this is
probably a required task before it can be merged. Doing this will
improve maintenance of the code as well, so it's useful to do this
anyway. I think it's better if you do it, but I guess some volunteer
can probably be found if needed. It's not a difficult task.

Now the real problems are with three duplicate i2c drivers: cx25843,
xc3028 and xc5000.

To start with the easiest one: cx25843. There already is a driver for
this (cx25840) and the empia driver should use that one. Since I
maintain cx25840 the easiest approach for you is to see if you can get
me hardware (em28xx + cx25843) so that I can test and update cx25843 to
provide proper empia support. The alternative is that we work together
on this, but that's probably more work for both of us. I most
definitely do *not* want to duplicate this driver. Windows drivers
duplicate this stuff all the time, but we're not Windows and having one
driver ensures that fixes or new functionality become available to all
bridge drivers that use it.

The same reasoning is true for xc3028 and xc5000. In addition, the
licensing of these sources is very vague. Is it even allowed to
distribute them under a GPL license? There is no GPL license in the
sources, yet in the module you specify GPL. This needs to be clarified
first.

I see two ways forward when it comes to these drivers:

1) The empia driver switches to the current xceive drivers that are
already in the kernel. No doubt this means that xceive driver bugs will
surface with certain devices, but those are bugs that the xceive driver
maintainer will have to fix. Obviously assistance would be appreciated,
but the bottom line is that a) your driver is finally in the kernel,
and b) there is a lot more pressure to fix bugs in the xceive drivers
than is the case otherwise. Yes, some devices will not work initially
with the empia driver, but I expect that to be a temporary situation.

2) Your xceive drivers replace the current drivers. This will require
that a) the license situation is clarified, b) the drivers are modified
to fit the current v4l-dvb tuner architecture. This option will mean a
lot of work for you since you are the maintainer of these drivers. In
addition, I forsee a lot of flaming if we go this way.

BTW, I noticed that the current xc5000 driver is very similar to yours
but with proper copyrights/license notices and coding style. So for
this driver option 1 is definitely the way to go.

To be honest, I don't see option 2 as viable. I forsee too many
inter-personal problems that will appear if we go that way. Option 1
has the big advantage that all you need to do is to file a bug report
if it doesn't work with a certain device. And in the meantime users can
fallback to your stand-alone driver until it's fixed in the kernel.
Obviously, if you can state in the bug report what the precise problem
is, so much the better.

So my recommendation would be to:

1) Switch to using the current xceive drivers in your empia driver. This
is something you have to do, I'm afraid. Unless someone would
volunteer? I personally do not have enough experience with this to do
it.

2) Switch to using the cx25840 driver. If I can get hardware, then I can
do this, otherwise we have to do this together. Initially we probably
have to disable devices using the cx25840 until the cx25840 driver has
been fixed for the empia driver.

3) Switch to video_ioctl2 in the empia driver. You can do that, but we
can probably find a volunteer as well.

4) Conform the code to the coding style. If several people can help with
this we can get it done pretty quickly.

5) Merge the empia driver alongside the current em28xx driver.

There are no doubt some things I missed, but I don't think I missed
anything major. I've put up a hg tree here:

http://linuxtv.org/hg/~hverkuil/v4l-dvb-em28xx/

This allows you to compile the empia module alongside the em28xx module.
Note that I've removed the empia cx25843 module in the last changeset
in order to test which dependencies the driver had on that module. So
if you want to test this tree with an empia+cx25843 device, then don't
get the latest changeset, but the one before that.

My tree does contain the empia xceive drivers, though. Perhaps someone
more knowledgeable with DVB can take a look to see how much work it is
to convert to the kernel xceive drivers? And to see if I overlooked any
DVB-related major obstacles?

I think this is a reasonable roadmap to finally get this in. If everyone
can pitch in then it really shouldn't take that much work to get it
into v4l-dvb and from there to 2.6.29.

Regards,

Hans Verkuil

2008-11-01 14:06:31

by Hans Verkuil

[permalink] [raw]
Subject: Re: [PATCH 1/7] Adding empia base driver

(Note: I'm reposting this since it bounced on several mailinglists due
to "Too many recipients". I'm now only sending it to the mailinglists
as I assume that most of the recipients are on at least one of these
mailinglists. Apologies if this is not the case.)


Hi Markus,

As promised I've done a review of your empia driver and looked at what
needs to be done to get it into the kernel.

First of all, I've no doubt that your empia driver is better and
supports more devices than the current em28xx driver. I also have no
problem adding your driver separate from the current driver. It's been
done before (certain networking drivers spring to mind) and while
obviously not ideal I expect that the older em28xx driver can probably
be removed after a year or something like that.

In my opinion it's pretty much hopeless trying to convert the current
em28xx driver into what you have. It's a huge amount of work that no
one wants to do and (in this case) with very little benefit. Of course,
Mauro has the final say in this.

While there is some cleaning up to do in your code (coding style, some
copyright/license clarifications), that is not a big deal. The coding
style cleanups are a fair amount of work, but I think we can probably
spread that out over several people and get that done quickly.

What I definitely would recommend is to use video_ioctl2 rather than
video_usercopy. The latter function will disappear in the future. I
think the policy for new drivers is to use video_ioctl2, so this is
probably a required task before it can be merged. Doing this will
improve maintenance of the code as well, so it's useful to do this
anyway. I think it's better if you do it, but I guess some volunteer
can probably be found if needed. It's not a difficult task.

Now the real problems are with three duplicate i2c drivers: cx25843,
xc3028 and xc5000.

To start with the easiest one: cx25843. There already is a driver for
this (cx25840) and the empia driver should use that one. Since I
maintain cx25840 the easiest approach for you is to see if you can get
me hardware (em28xx + cx25843) so that I can test and update cx25843 to
provide proper empia support. The alternative is that we work together
on this, but that's probably more work for both of us. I most
definitely do *not* want to duplicate this driver. Windows drivers
duplicate this stuff all the time, but we're not Windows and having one
driver ensures that fixes or new functionality become available to all
bridge drivers that use it.

The same reasoning is true for xc3028 and xc5000. In addition, the
licensing of these sources is very vague. Is it even allowed to
distribute them under a GPL license? There is no GPL license in the
sources, yet in the module you specify GPL. This needs to be clarified
first.

I see two ways forward when it comes to these drivers:

1) The empia driver switches to the current xceive drivers that are
already in the kernel. No doubt this means that xceive driver bugs will
surface with certain devices, but those are bugs that the xceive driver
maintainer will have to fix. Obviously assistance would be appreciated,
but the bottom line is that a) your driver is finally in the kernel,
and b) there is a lot more pressure to fix bugs in the xceive drivers
than is the case otherwise. Yes, some devices will not work initially
with the empia driver, but I expect that to be a temporary situation.

2) Your xceive drivers replace the current drivers. This will require
that a) the license situation is clarified, b) the drivers are modified
to fit the current v4l-dvb tuner architecture. This option will mean a
lot of work for you since you are the maintainer of these drivers. In
addition, I forsee a lot of flaming if we go this way.

BTW, I noticed that the current xc5000 driver is very similar to yours
but with proper copyrights/license notices and coding style. So for
this driver option 1 is definitely the way to go.

To be honest, I don't see option 2 as viable. I forsee too many
inter-personal problems that will appear if we go that way. Option 1
has the big advantage that all you need to do is to file a bug report
if it doesn't work with a certain device. And in the meantime users can
fallback to your stand-alone driver until it's fixed in the kernel.
Obviously, if you can state in the bug report what the precise problem
is, so much the better.

So my recommendation would be to:

1) Switch to using the current xceive drivers in your empia driver. This
is something you have to do, I'm afraid. Unless someone would
volunteer? I personally do not have enough experience with this to do
it.

2) Switch to using the cx25840 driver. If I can get hardware, then I can
do this, otherwise we have to do this together. Initially we probably
have to disable devices using the cx25840 until the cx25840 driver has
been fixed for the empia driver.

3) Switch to video_ioctl2 in the empia driver. You can do that, but we
can probably find a volunteer as well.

4) Conform the code to the coding style. If several people can help with
this we can get it done pretty quickly.

5) Merge the empia driver alongside the current em28xx driver.

There are no doubt some things I missed, but I don't think I missed
anything major. I've put up a hg tree here:

http://linuxtv.org/hg/~hverkuil/v4l-dvb-em28xx/

This allows you to compile the empia module alongside the em28xx module.
Note that I've removed the empia cx25843 module in the last changeset
in order to test which dependencies the driver had on that module. So
if you want to test this tree with an empia+cx25843 device, then don't
get the latest changeset, but the one before that.

My tree does contain the empia xceive drivers, though. Perhaps someone
more knowledgeable with DVB can take a look to see how much work it is
to convert to the kernel xceive drivers? And to see if I overlooked any
DVB-related major obstacles?

I think this is a reasonable roadmap to finally get this in. If everyone
can pitch in then it really shouldn't take that much work to get it
into v4l-dvb and from there to 2.6.29.

Regards,

Hans Verkuil

2008-11-01 15:04:26

by Andy Walls

[permalink] [raw]
Subject: Re: [linux-dvb] [PATCH 1/7] Adding empia base driver

On Sat, 2008-11-01 at 15:05 +0100, Hans Verkuil wrote:

> Hi Markus,
>
> As promised I've done a review of your empia driver and looked at what
> needs to be done to get it into the kernel.
>
> First of all, I've no doubt that your empia driver is better and
> supports more devices than the current em28xx driver. I also have no
> problem adding your driver separate from the current driver. It's been
> done before (certain networking drivers spring to mind) and while
> obviously not ideal I expect that the older em28xx driver can probably
> be removed after a year or something like that.

[snip]

> So my recommendation would be to:

[snip]

> 3) Switch to video_ioctl2 in the empia driver. You can do that, but we
> can probably find a volunteer as well.
>
> 4) Conform the code to the coding style. If several people can help with
> this we can get it done pretty quickly.

I can support these two portions of the effort, if what Hans' proposes
is the agreed plan.

Point me to the target directories in the repo, and suggest desired
completion dates for whatever tasks.

Standing by...

Regards,
Andy

2008-11-01 15:46:19

by Devin Heitmueller

[permalink] [raw]
Subject: Re: [linux-dvb] [PATCH 1/7] Adding empia base driver

Hello,

I have held off on offering an opinion this to see what others
thought, but I think now may be the time to speak up.

First off, a disclaimer: I am a contributor to the existing in-kernel
em28xx driver. I have spent many months working on that codebase,
adding device support and fixing bugs. I also have a large series of
patches queuing up that significantly improve both the codebase's
functionality and maintainability (having recently been given access
to some of the datasheets thanks to Empia and Pinnacle).

As one of the half dozen people who are working on the linux-dvb
version of em28xx, I am against the wholesale replacement of the
current version with Markus's codebase.

Why? I've got a list of reasons, but in the interest of fairness,
let's start with what I see to be the good things:

# Significantly better device support - Markus has access to the
actual hardware for many of these devices, spends time adding support
for new devices, and since he works for the chipset vendor can in many
cases just call the manufacturer of the product and ask them
questions.

# Proper tuner locking - tuner locking was one of the big issues that
caused infighting before Markus forked off his code. Let's face it -
it's been over a year and most of the other devices don't do any
locking at all. His scheme, while not unified across drivers, is
better than nothing.

# Based on the chipset datasheets - He had the benefit of just being
able to look up what the registers mean

# VBI support for analog - a recent addition in the mrec driver, but
none at all in the V4L driver

# Support for other demods not currently in V4L - I don't think we
have any devices yet that use the LGDT3304, but Markus's driver has
support for devices that do.

# More thoroughly debugged - He's working on this full time. He's
working bugs, dealing with issues, and putting in proper fixes based
on reliable information instead of reverse engineering.

========

Now, the not so good things:

# Doesn't leverage common infrastructure such as videobuf (resulting
in duplicate functionality and more difficult for those who have to
maintain multiple drivers)

# Firmware blobs embedded in source - While it's easier for the user,
many distributions do not allow firmware blobs in the kernel due to
the belief that this is not GPL compatible. We would need to get
permission from the vendor to redistribute the firmware as a file (in
the V4L driver, we extract it from the Windows driver binary)

# Ambigious licensing - some of the files have headers from companies
other than Empiatech which are very clearly not GPL compatible (like
the Micronas drx3973d driver). Also, it's not clear that even the
firmware blobs mentioned above are authorized to be redistributed by
their rightful owners (Xceive and Micronas). While Empiatech may be ok
with making a GPL driver, these parties have not consented to having
their intellectual property in the kernel (they may have consented but
the header files say just the opposite).

# It has its own xc3028 and xc5000 tuner driver. I don't know whether
his driver is better than the one in V4L. Presumably he has the
datasheets for those parts, but on the other hand the V4L driver
allows loading of the firmware externally. The V4L drivers are also
used by devices beyond the em28xx and may have functionality required
by other companies products.

# What I'll call "Black magic" - lots of arbitrary code without any
explanation as to what it is doing or why. Why does the DVB init
routine write 0x77 to register 0x12? What does that do? A combination
of poor use of constants and commented code combined with a lack of
access to the datasheets leaves this a mystery. You just have to
"trust that it's doing the right thing because it works"

# He's the only one who has access to the datasheets, so there is
limited opportunity for peer review. The community driver is based on
reverse engineering, and we can pass around USB traces we collect to
justify/explain design decisions. How do you question a design when
the basis of answers is essentially "because the secret document that
I can't show you says so"?

====

I shared this list with Markus a few months ago and, the licensing
issues aside, it was his contention that "nobody cares" about most of
the things above. As a maintainer that wants to continue contributing
to the codebase, *I* care. And I'm sure that anybody other than
Markus who wants to understand the em28xx codebase and be able to fix
bugs would also care. I'm also concerned with consistency between
drivers. Having one driver do things differently than all the others
is just a maintenance headache for those who have to support multiple
drivers.

A number of people have suggested that nobody was willing to
incorporate Markus's changes incrementally to improve the in-kernel
driver. This couldn't be further from the truth. I appealed to
Markus on multiple occasions trying to find some compromise where his
changes could be merged into the mainline em28xx driver. He outright
refused. It was his contention that his driver was/is better than the
in-kernel driver in every possible way, and that the existing code has
no redeeming value. In fact, I was accused of taking his GPL'd code
without his consent and incorporating it into the linux-dvb codebase.
It's this "all or nothing" attitude that has prevented his work thus
far from being incorporated, not the unwillingness of people like
myself to do the work to merge his changes in a sane matter.

I *really* want to see this resolved, because I recognize that I could
be better served working on other things than duplicating efforts to
debug issues that Markus may have already fixed in his codebase. But
just throwing away the work of half a dozen other developers on an
actively maintained driver is not really the sort of compromise I
think would be best for the community.

I'm sorry if the sharing of my views on this matter create more
animosity within the community, as that is the exact opposite of what
I want.

Devin

--
Devin J. Heitmueller
http://www.devinheitmueller.com
AIM: devinheitmueller

2008-11-01 16:08:20

by Pekka Enberg

[permalink] [raw]
Subject: Re: [linux-dvb] [PATCH 1/7] Adding empia base driver

Hi Devin,

On Sat, Nov 1, 2008 at 5:46 PM, Devin Heitmueller
<[email protected]> wrote:
> A number of people have suggested that nobody was willing to
> incorporate Markus's changes incrementally to improve the in-kernel
> driver. This couldn't be further from the truth. I appealed to
> Markus on multiple occasions trying to find some compromise where his
> changes could be merged into the mainline em28xx driver. He outright
> refused. It was his contention that his driver was/is better than the
> in-kernel driver in every possible way, and that the existing code has
> no redeeming value. In fact, I was accused of taking his GPL'd code
> without his consent and incorporating it into the linux-dvb codebase.
> It's this "all or nothing" attitude that has prevented his work thus
> far from being incorporated, not the unwillingness of people like
> myself to do the work to merge his changes in a sane matter.

I'm not sure I understand how he can refuse such a thing. If the code
is released under the GPLv2 and the author refuses to play by the well
known rules of the kernel community, then I don't see any problem with
taking the code and improving the current driver (as long as the
copyright is properly attributed, of course).

I think it's already pretty well established that we don't just take
in shiny new drivers and trust a new maintainer to do the right thing
because that has gotten us in such a mess so many times before. Being
part of the community is not so much the code you write but the way
you interact with other kernel developers.

So, if I were you, I'd just do it.

Pekka

2008-11-01 16:19:25

by Devin Heitmueller

[permalink] [raw]
Subject: Re: [linux-dvb] [PATCH 1/7] Adding empia base driver

On Sat, Nov 1, 2008 at 12:08 PM, Pekka Enberg <[email protected]> wrote:
> Hi Devin,
>
> On Sat, Nov 1, 2008 at 5:46 PM, Devin Heitmueller
> <[email protected]> wrote:
>> A number of people have suggested that nobody was willing to
>> incorporate Markus's changes incrementally to improve the in-kernel
>> driver. This couldn't be further from the truth. I appealed to
>> Markus on multiple occasions trying to find some compromise where his
>> changes could be merged into the mainline em28xx driver. He outright
>> refused. It was his contention that his driver was/is better than the
>> in-kernel driver in every possible way, and that the existing code has
>> no redeeming value. In fact, I was accused of taking his GPL'd code
>> without his consent and incorporating it into the linux-dvb codebase.
>> It's this "all or nothing" attitude that has prevented his work thus
>> far from being incorporated, not the unwillingness of people like
>> myself to do the work to merge his changes in a sane matter.
>
> I'm not sure I understand how he can refuse such a thing. If the code
> is released under the GPLv2 and the author refuses to play by the well
> known rules of the kernel community, then I don't see any problem with
> taking the code and improving the current driver (as long as the
> copyright is properly attributed, of course).
>
> I think it's already pretty well established that we don't just take
> in shiny new drivers and trust a new maintainer to do the right thing
> because that has gotten us in such a mess so many times before. Being
> part of the community is not so much the code you write but the way
> you interact with other kernel developers.
>
> So, if I were you, I'd just do it.

Hello Pekka,

I do not believe that he had any legal standing to prevent me from
taking the code and incorporating it if that was my desire, given that
he released it under the GPL. However, taking someone's code when
they specifically said they don't want you to is kind of a crappy
thing to do in my humble opinion, and it definitely doesn't improve
relations with the developer.

The reality is that from a technical standpoint I really want Markus
to be the maintainer. He knows more about the devices than anyone,
works for the vendor, and has access to all the datasheets. That
said, I don't want a situation where he is the *only* one who can do
work on the codebase because it is poorly commented and structured in
a way where only he can understand why it works the way it does.
Also, I believe certain design decisions should be made as a result of
consensus with the other maintainers, taking into consideration
consistency across drivers.

Regards,

Devin

--
Devin J. Heitmueller
http://www.devinheitmueller.com
AIM: devinheitmueller

2008-11-01 16:22:44

by Hans Verkuil

[permalink] [raw]
Subject: Re: [linux-dvb] [PATCH 1/7] Adding empia base driver

Hi Devin,

Just a few quick notes:

On Saturday 01 November 2008 16:46:03 Devin Heitmueller wrote:
> Hello,
>
> I have held off on offering an opinion this to see what others
> thought, but I think now may be the time to speak up.
>
> First off, a disclaimer: I am a contributor to the existing in-kernel
> em28xx driver. I have spent many months working on that codebase,
> adding device support and fixing bugs. I also have a large series of
> patches queuing up that significantly improve both the codebase's
> functionality and maintainability (having recently been given access
> to some of the datasheets thanks to Empia and Pinnacle).
>
> As one of the half dozen people who are working on the linux-dvb
> version of em28xx, I am against the wholesale replacement of the
> current version with Markus's codebase.

At this time I do not advocate replacing the current em28xx driver. But
when they are both in the kernel, then I expect and hope that the best
features of the em28xx driver are merged into the empia driver and that
the current em28xx driver can eventually be dropped.

> Why? I've got a list of reasons, but in the interest of fairness,
> let's start with what I see to be the good things:
>
> # Significantly better device support - Markus has access to the
> actual hardware for many of these devices, spends time adding support
> for new devices, and since he works for the chipset vendor can in
> many cases just call the manufacturer of the product and ask them
> questions.
>
> # Proper tuner locking - tuner locking was one of the big issues that
> caused infighting before Markus forked off his code. Let's face it -
> it's been over a year and most of the other devices don't do any
> locking at all. His scheme, while not unified across drivers, is
> better than nothing.
>
> # Based on the chipset datasheets - He had the benefit of just being
> able to look up what the registers mean
>
> # VBI support for analog - a recent addition in the mrec driver, but
> none at all in the V4L driver
>
> # Support for other demods not currently in V4L - I don't think we
> have any devices yet that use the LGDT3304, but Markus's driver has
> support for devices that do.
>
> # More thoroughly debugged - He's working on this full time. He's
> working bugs, dealing with issues, and putting in proper fixes based
> on reliable information instead of reverse engineering.
>
> ========
>
> Now, the not so good things:
>
> # Doesn't leverage common infrastructure such as videobuf (resulting
> in duplicate functionality and more difficult for those who have to
> maintain multiple drivers)

Definitely a candidate to merge into Markus' driver eventually. There
are more drivers that do not use videobuf (my own ivtv and cx18 drivers
spring to mind).

> # Firmware blobs embedded in source - While it's easier for the user,
> many distributions do not allow firmware blobs in the kernel due to
> the belief that this is not GPL compatible. We would need to get
> permission from the vendor to redistribute the firmware as a file (in
> the V4L driver, we extract it from the Windows driver binary)

>From what I saw firmware blobs were only present in the xceive drivers,
and it is my opinion that it is not a good idea to merge these into the
kernel. Much better to fix the existing drivers. Having the empia
driver into the kernel will actually force those fixes to be made.

> # Ambigious licensing - some of the files have headers from companies
> other than Empiatech which are very clearly not GPL compatible (like
> the Micronas drx3973d driver). Also, it's not clear that even the
> firmware blobs mentioned above are authorized to be redistributed by
> their rightful owners (Xceive and Micronas). While Empiatech may be
> ok with making a GPL driver, these parties have not consented to
> having their intellectual property in the kernel (they may have
> consented but the header files say just the opposite).

Licensing should obviously be addressed. But such drivers (except for
the xceive ones) are currently not used by the empia sources as
submitted by Markus.

> # It has its own xc3028 and xc5000 tuner driver. I don't know whether
> his driver is better than the one in V4L. Presumably he has the
> datasheets for those parts, but on the other hand the V4L driver
> allows loading of the firmware externally. The V4L drivers are also
> used by devices beyond the em28xx and may have functionality required
> by other companies products.

For the record: other devs have datasheets and sources as well for these
devices.

> # What I'll call "Black magic" - lots of arbitrary code without any
> explanation as to what it is doing or why. Why does the DVB init
> routine write 0x77 to register 0x12? What does that do? A combination
> of poor use of constants and commented code combined with a lack of
> access to the datasheets leaves this a mystery. You just have to
> "trust that it's doing the right thing because it works"

This is not an uncommon occurence when datasheets are not public.
Hopefully Markus can address such problems when the driver is in the
kernel. It's IMHO not a blocking issue.

> # He's the only one who has access to the datasheets, so there is
> limited opportunity for peer review. The community driver is based on
> reverse engineering, and we can pass around USB traces we collect to
> justify/explain design decisions. How do you question a design when
> the basis of answers is essentially "because the secret document that
> I can't show you says so"?

There are lots of drivers that are based on NDAs (e.g. my cx18 driver).
The code is public, but the datasheets aren't. That's actually much
better than to rely on reverse engineering. Of course, you get the best
results if the datasheets are also public, but that's sadly not always
possible. Often active developers can all get NDAs, so that multiple
devs have access to datasheets (again, that's the case for the cx18
driver).

I see this as an advantage, not a disadvantage.

>
> ====
>
> I shared this list with Markus a few months ago and, the licensing
> issues aside, it was his contention that "nobody cares" about most of
> the things above. As a maintainer that wants to continue
> contributing to the codebase, *I* care. And I'm sure that anybody
> other than Markus who wants to understand the em28xx codebase and be
> able to fix bugs would also care. I'm also concerned with
> consistency between drivers. Having one driver do things differently
> than all the others is just a maintenance headache for those who have
> to support multiple drivers.
>
> A number of people have suggested that nobody was willing to
> incorporate Markus's changes incrementally to improve the in-kernel
> driver. This couldn't be further from the truth. I appealed to
> Markus on multiple occasions trying to find some compromise where his
> changes could be merged into the mainline em28xx driver. He outright
> refused. It was his contention that his driver was/is better than
> the in-kernel driver in every possible way, and that the existing
> code has no redeeming value. In fact, I was accused of taking his
> GPL'd code without his consent and incorporating it into the
> linux-dvb codebase. It's this "all or nothing" attitude that has
> prevented his work thus far from being incorporated, not the
> unwillingness of people like myself to do the work to merge his
> changes in a sane matter.
>
> I *really* want to see this resolved, because I recognize that I
> could be better served working on other things than duplicating
> efforts to debug issues that Markus may have already fixed in his
> codebase. But just throwing away the work of half a dozen other
> developers on an actively maintained driver is not really the sort of
> compromise I think would be best for the community.
>
> I'm sorry if the sharing of my views on this matter create more
> animosity within the community, as that is the exact opposite of what
> I want.

This is I think the last chance to get Markus' driver into the kernel.
If this fails again, then there is no other choice but to fork it all.
But for the end-users it's so much better if Markus would maintain the
empia driver since he has the datasheets and hardware.

Forget the history, and see this as a new driver. I think I presented a
reasonable roadmap for it to be merged.

Regards,

Hans

2008-11-01 16:27:27

by Pekka Enberg

[permalink] [raw]
Subject: Re: [linux-dvb] [PATCH 1/7] Adding empia base driver

On Sat, Nov 1, 2008 at 6:21 PM, Hans Verkuil <[email protected]> wrote:
> At this time I do not advocate replacing the current em28xx driver. But
> when they are both in the kernel, then I expect and hope that the best
> features of the em28xx driver are merged into the empia driver and that
> the current em28xx driver can eventually be dropped.

We've done that many times and it usually ends up with us dragging the
old driver along for a very long time (look at the eepro100 driver
removal as an example). So you really want to just incrementally fix
up em28xx driver to avoid a mess.

2008-11-01 16:30:14

by Pekka Enberg

[permalink] [raw]
Subject: Re: [linux-dvb] [PATCH 1/7] Adding empia base driver

On Sat, Nov 1, 2008 at 6:19 PM, Devin Heitmueller
<[email protected]> wrote:
> The reality is that from a technical standpoint I really want Markus
> to be the maintainer. He knows more about the devices than anyone,
> works for the vendor, and has access to all the datasheets. That
> said, I don't want a situation where he is the *only* one who can do
> work on the codebase because it is poorly commented and structured in
> a way where only he can understand why it works the way it does.
> Also, I believe certain design decisions should be made as a result of
> consensus with the other maintainers, taking into consideration
> consistency across drivers.

I can understand that you want him to be the maintainer. But so far he
hasn't really shown to be interested in doing that. It's just bit sad
that the we don't have a proper driver given that the code exists and
that there's a person who's willing to do the work to get it merged...

2008-11-01 16:51:36

by Devin Heitmueller

[permalink] [raw]
Subject: Re: [linux-dvb] [PATCH 1/7] Adding empia base driver

Hello Hans,

Thanks for getting back to me. Responses inline:

On Sat, Nov 1, 2008 at 12:21 PM, Hans Verkuil <[email protected]> wrote:
>> As one of the half dozen people who are working on the linux-dvb
>> version of em28xx, I am against the wholesale replacement of the
>> current version with Markus's codebase.
>
> At this time I do not advocate replacing the current em28xx driver. But
> when they are both in the kernel, then I expect and hope that the best
> features of the em28xx driver are merged into the empia driver and that
> the current em28xx driver can eventually be dropped.

I'm certainly not against this approach, and having it in the mainline
will make it easier for others to contribute and improve the codebase.
We would however have to deal with how to handle all the overlapping
product support and the increased confusion that results from users
reporting problems and figuring out which driver they are talking
about (this is already an issue today though as most users don't
understand that there are two drivers).

>> # Doesn't leverage common infrastructure such as videobuf (resulting
>> in duplicate functionality and more difficult for those who have to
>> maintain multiple drivers)
>
> Definitely a candidate to merge into Markus' driver eventually. There
> are more drivers that do not use videobuf (my own ivtv and cx18 drivers
> spring to mind).

Agreed. If both drivers are used in parallel than this is less of an
issue. I was just against the wholesale replacement, which would
result in moving backwards in these areas since the work was already
done in the mainline driver.

>> # Firmware blobs embedded in source - While it's easier for the user,
>> many distributions do not allow firmware blobs in the kernel due to
>> the belief that this is not GPL compatible. We would need to get
>> permission from the vendor to redistribute the firmware as a file (in
>> the V4L driver, we extract it from the Windows driver binary)
>
> From what I saw firmware blobs were only present in the xceive drivers,
> and it is my opinion that it is not a good idea to merge these into the
> kernel. Much better to fix the existing drivers. Having the empia
> driver into the kernel will actually force those fixes to be made.

Yes, I was referring to the Xceive drivers. I agree with what you
are saying, as long as we can agree that we should not have parallel
tuner implementations in the kernel and the changes to use the
mainline tuners should be made *before* it gets imported.

>> # Ambigious licensing - some of the files have headers from companies
>> other than Empiatech which are very clearly not GPL compatible (like
>> the Micronas drx3973d driver). Also, it's not clear that even the
>> firmware blobs mentioned above are authorized to be redistributed by
>> their rightful owners (Xceive and Micronas). While Empiatech may be
>> ok with making a GPL driver, these parties have not consented to
>> having their intellectual property in the kernel (they may have
>> consented but the header files say just the opposite).
>
> Licensing should obviously be addressed. But such drivers (except for
> the xceive ones) are currently not used by the empia sources as
> submitted by Markus.

I do not believe they should be included into the codebase until the
licensing issues are addressed. Having the code in the kernel is a
liability risk, even if it is not used by anything right now.

>> # It has its own xc3028 and xc5000 tuner driver. I don't know whether
>> his driver is better than the one in V4L. Presumably he has the
>> datasheets for those parts, but on the other hand the V4L driver
>> allows loading of the firmware externally. The V4L drivers are also
>> used by devices beyond the em28xx and may have functionality required
>> by other companies products.
>
> For the record: other devs have datasheets and sources as well for these
> devices.

Yes, I know. Markus has suggested that his versions of the drivers
are better because they are based on the reference code. The xc5000
driver aside (where the mainline driver is also based on the Xceive
reference code with proper licensing and attribution), I do not
believe he has ever offered any technical basis for his assertion.

>> # What I'll call "Black magic" - lots of arbitrary code without any
>> explanation as to what it is doing or why. Why does the DVB init
>> routine write 0x77 to register 0x12? What does that do? A combination
>> of poor use of constants and commented code combined with a lack of
>> access to the datasheets leaves this a mystery. You just have to
>> "trust that it's doing the right thing because it works"
>
> This is not an uncommon occurence when datasheets are not public.
> Hopefully Markus can address such problems when the driver is in the
> kernel. It's IMHO not a blocking issue.

He has had the opportunity to do this in his own tree, and has thus
far not done it because, as he put it in email to me "nobody cares
about this". I disagree with this assertion personally as someone who
has had to fix bugs in the mainline driver and it would have been very
helpful to at least have commented what some of the code is doing. He
has the datasheets, and has made a conscious decision to not describe
what the code is doing.

>> # He's the only one who has access to the datasheets, so there is
>> limited opportunity for peer review. The community driver is based on
>> reverse engineering, and we can pass around USB traces we collect to
>> justify/explain design decisions. How do you question a design when
>> the basis of answers is essentially "because the secret document that
>> I can't show you says so"?
>
> There are lots of drivers that are based on NDAs (e.g. my cx18 driver).
> The code is public, but the datasheets aren't. That's actually much
> better than to rely on reverse engineering. Of course, you get the best
> results if the datasheets are also public, but that's sadly not always
> possible. Often active developers can all get NDAs, so that multiple
> devs have access to datasheets (again, that's the case for the cx18
> driver).
>
> I see this as an advantage, not a disadvantage.

I understand the value of datasheets, as I am in this situation myself
with several devices. However, in many cases a well written driver
will have good comments as to what it is doing (super secret
algorithms aside). In fact, now that I have access to some of the
Empia datasheets, I have some patches for the mainline driver that
better document some of these cases.

>> I'm sorry if the sharing of my views on this matter create more
>> animosity within the community, as that is the exact opposite of what
>> I want.
>
> This is I think the last chance to get Markus' driver into the kernel.
> If this fails again, then there is no other choice but to fork it all.
> But for the end-users it's so much better if Markus would maintain the
> empia driver since he has the datasheets and hardware.
>
> Forget the history, and see this as a new driver. I think I presented a
> reasonable roadmap for it to be merged.

Sure, and if Markus is willing to compromise on things like the tuner
drivers, then this would be good for everybody. Past experience has
suggested that he was unwilling to compromise on anything (based on my
attempts in the past), so if things have changed then I would be
thrilled to work with him.

Devin

--
Devin J. Heitmueller
http://www.devinheitmueller.com
AIM: devinheitmueller

2008-11-01 17:55:49

by Markus Rechberger

[permalink] [raw]
Subject: Re: [linux-dvb] [PATCH 1/7] Adding empia base driver

On Sat, Nov 1, 2008 at 5:29 PM, Pekka Enberg <[email protected]> wrote:
> On Sat, Nov 1, 2008 at 6:19 PM, Devin Heitmueller
> <[email protected]> wrote:
>> The reality is that from a technical standpoint I really want Markus
>> to be the maintainer. He knows more about the devices than anyone,
>> works for the vendor, and has access to all the datasheets. That
>> said, I don't want a situation where he is the *only* one who can do
>> work on the codebase because it is poorly commented and structured in
>> a way where only he can understand why it works the way it does.
>> Also, I believe certain design decisions should be made as a result of
>> consensus with the other maintainers, taking into consideration
>> consistency across drivers.
>
> I can understand that you want him to be the maintainer. But so far he
> hasn't really shown to be interested in doing that. It's just bit sad
> that the we don't have a proper driver given that the code exists and
> that there's a person who's willing to do the work to get it merged...
>

There's alot discussion and I haven't followed it yet although, the
best way to go
seen from my side is to take the em28xx-new code and merge the usable kernelcode
into that one (note that only ~10% of the devices in the kernel em28xx
driver are actually
tested).

I read about the License, I can say this is no issue overall, all of
the code is available under GPL,
and some of it is also under BSD license.

I need to catch up the other mails before continuing...

Markus

2008-11-02 04:02:31

by Markus Rechberger

[permalink] [raw]
Subject: Re: [linux-dvb] [PATCH 1/7] Adding empia base driver

On Sat, Nov 1, 2008 at 5:51 PM, Devin Heitmueller
<[email protected]> wrote:
> Hello Hans,
>
> Thanks for getting back to me. Responses inline:
>
> On Sat, Nov 1, 2008 at 12:21 PM, Hans Verkuil <[email protected]> wrote:
>>> As one of the half dozen people who are working on the linux-dvb
>>> version of em28xx, I am against the wholesale replacement of the
>>> current version with Markus's codebase.
>>
>> At this time I do not advocate replacing the current em28xx driver. But
>> when they are both in the kernel, then I expect and hope that the best
>> features of the em28xx driver are merged into the empia driver and that
>> the current em28xx driver can eventually be dropped.
>
> I'm certainly not against this approach, and having it in the mainline
> will make it easier for others to contribute and improve the codebase.
> We would however have to deal with how to handle all the overlapping
> product support and the increased confusion that results from users
> reporting problems and figuring out which driver they are talking
> about (this is already an issue today though as most users don't
> understand that there are two drivers).
>
>>> # Doesn't leverage common infrastructure such as videobuf (resulting
>>> in duplicate functionality and more difficult for those who have to
>>> maintain multiple drivers)
>>
>> Definitely a candidate to merge into Markus' driver eventually. There
>> are more drivers that do not use videobuf (my own ivtv and cx18 drivers
>> spring to mind).
>
> Agreed. If both drivers are used in parallel than this is less of an
> issue. I was just against the wholesale replacement, which would
> result in moving backwards in these areas since the work was already
> done in the mainline driver.
>
>>> # Firmware blobs embedded in source - While it's easier for the user,
>>> many distributions do not allow firmware blobs in the kernel due to
>>> the belief that this is not GPL compatible. We would need to get
>>> permission from the vendor to redistribute the firmware as a file (in
>>> the V4L driver, we extract it from the Windows driver binary)
>>
>> From what I saw firmware blobs were only present in the xceive drivers,
>> and it is my opinion that it is not a good idea to merge these into the
>> kernel. Much better to fix the existing drivers. Having the empia
>> driver into the kernel will actually force those fixes to be made.
>
> Yes, I was referring to the Xceive drivers. I agree with what you
> are saying, as long as we can agree that we should not have parallel
> tuner implementations in the kernel and the changes to use the
> mainline tuners should be made *before* it gets imported.
>
>>> # Ambigious licensing - some of the files have headers from companies
>>> other than Empiatech which are very clearly not GPL compatible (like
>>> the Micronas drx3973d driver). Also, it's not clear that even the
>>> firmware blobs mentioned above are authorized to be redistributed by
>>> their rightful owners (Xceive and Micronas). While Empiatech may be
>>> ok with making a GPL driver, these parties have not consented to
>>> having their intellectual property in the kernel (they may have
>>> consented but the header files say just the opposite).
>>
>> Licensing should obviously be addressed. But such drivers (except for
>> the xceive ones) are currently not used by the empia sources as
>> submitted by Markus.
>
> I do not believe they should be included into the codebase until the
> licensing issues are addressed. Having the code in the kernel is a
> liability risk, even if it is not used by anything right now.
>
>>> # It has its own xc3028 and xc5000 tuner driver. I don't know whether
>>> his driver is better than the one in V4L. Presumably he has the
>>> datasheets for those parts, but on the other hand the V4L driver
>>> allows loading of the firmware externally. The V4L drivers are also
>>> used by devices beyond the em28xx and may have functionality required
>>> by other companies products.
>>
>> For the record: other devs have datasheets and sources as well for these
>> devices.
>
> Yes, I know. Markus has suggested that his versions of the drivers
> are better because they are based on the reference code. The xc5000
> driver aside (where the mainline driver is also based on the Xceive
> reference code with proper licensing and attribution), I do not
> believe he has ever offered any technical basis for his assertion.
>
>>> # What I'll call "Black magic" - lots of arbitrary code without any
>>> explanation as to what it is doing or why. Why does the DVB init
>>> routine write 0x77 to register 0x12? What does that do? A combination
>>> of poor use of constants and commented code combined with a lack of
>>> access to the datasheets leaves this a mystery. You just have to
>>> "trust that it's doing the right thing because it works"
>>
>> This is not an uncommon occurence when datasheets are not public.
>> Hopefully Markus can address such problems when the driver is in the
>> kernel. It's IMHO not a blocking issue.
>
> He has had the opportunity to do this in his own tree, and has thus
> far not done it because, as he put it in email to me "nobody cares
> about this". I disagree with this assertion personally as someone who
> has had to fix bugs in the mainline driver and it would have been very
> helpful to at least have commented what some of the code is doing. He
> has the datasheets, and has made a conscious decision to not describe
> what the code is doing.
>

You are the first person I ever saw asking for that in a driver. A
short mail asking
what a specific register does is the usual way how register
information can be revealed.

eg.:
http://linuxtv.org/hg/v4l-dvb/file/55f8fcf70843/linux/drivers/media/video/sn9c102/sn9c102_ov7630.c
line 36 and ongoing.


>>> # He's the only one who has access to the datasheets, so there is
>>> limited opportunity for peer review. The community driver is based on
>>> reverse engineering, and we can pass around USB traces we collect to
>>> justify/explain design decisions. How do you question a design when
>>> the basis of answers is essentially "because the secret document that
>>> I can't show you says so"?
>>
>> There are lots of drivers that are based on NDAs (e.g. my cx18 driver).
>> The code is public, but the datasheets aren't. That's actually much
>> better than to rely on reverse engineering. Of course, you get the best
>> results if the datasheets are also public, but that's sadly not always
>> possible. Often active developers can all get NDAs, so that multiple
>> devs have access to datasheets (again, that's the case for the cx18
>> driver).
>>
>> I see this as an advantage, not a disadvantage.
>
> I understand the value of datasheets, as I am in this situation myself
> with several devices. However, in many cases a well written driver
> will have good comments as to what it is doing (super secret
> algorithms aside). In fact, now that I have access to some of the
> Empia datasheets, I have some patches for the mainline driver that
> better document some of these cases.
>

Take care that you get the official agreement to publish documentation about it.

>>> I'm sorry if the sharing of my views on this matter create more
>>> animosity within the community, as that is the exact opposite of what
>>> I want.
>>
>> This is I think the last chance to get Markus' driver into the kernel.
>> If this fails again, then there is no other choice but to fork it all.
>> But for the end-users it's so much better if Markus would maintain the
>> empia driver since he has the datasheets and hardware.
>>
>> Forget the history, and see this as a new driver. I think I presented a
>> reasonable roadmap for it to be merged.
>
> Sure, and if Markus is willing to compromise on things like the tuner
> drivers, then this would be good for everybody. Past experience has
> suggested that he was unwilling to compromise on anything (based on my
> attempts in the past), so if things have changed then I would be
> thrilled to work with him.
>

In case of the tuners I'd like to keep them the way they are *for now*
- it might be
changed lateron. Those things are still in progress. It doesn't
interfere with other
tuners in the system.
The driver explicitly tells the tuner-core to not attach anything when
those 2 chips are
used for analog and digital TV. It's backward compatible without
adding any problems.
The xc5000 driver from Steven is based on reference drivers as far as
I know, there have
been a few updates to it and especially the xc5000 part of that device
is still not in a
frozen state (there are issues with the cx25843 - xc5000)
All the firmware parts are moved out of the driver, frequency tables
are kept inside.

Markus

2008-11-02 04:27:53

by Mauro Carvalho Chehab

[permalink] [raw]
Subject: Re: [PATCH 1/7] Adding empia base driver

On Sat, 1 Nov 2008 14:59:17 +0100
Hans Verkuil <[email protected]> wrote:

> Hi Markus,
>
> As promised I've done a review of your empia driver and looked at what
> needs to be done to get it into the kernel.
>
> First of all, I've no doubt that your empia driver is better and
> supports more devices than the current em28xx driver. I also have no
> problem adding your driver separate from the current driver. It's been
> done before (certain networking drivers spring to mind) and while
> obviously not ideal I expect that the older em28xx driver can probably
> be removed after a year or something like that.
>
> In my opinion it's pretty much hopeless trying to convert the current
> em28xx driver into what you have. It's a huge amount of work that no
> one wants to do and (in this case) with very little benefit. Of course,
> Mauro has the final say in this.
>

Both upstream and the 4 duplicated drivers have similar functionality. Also,
the upstream driver is actively maintained. So, there's no sense on accepting
those duplicated drivers.

Also, just replacing one existing driver by a newer one will cause regressions
on some already fixed bugs and remove some improvements that the upstream driver
suffered.

If there's a bug or a lack of functionality on em28xx, cx25843, xc5000 or
tuner-xc2028, it is just a matter of submitting patches fixing those bugs or
adding newer features.

Cheers,
Mauro

2008-11-08 10:15:24

by Mauro Carvalho Chehab

[permalink] [raw]
Subject: Re: [PATCH 1/7] Adding empia base driver

On Sat, 8 Nov 2008 03:50:20 -0200
"Andre Kelmanson" <[email protected]> wrote:

> Dears,
>
> I'm using this version of em28xx for a long time and it's working fine. It
> has three very important features for me. The first one is Kaiomy device,
> the second one is the new em28xx-audoep module and the third one is PAL-M
> support. Kaiomy and PAL-M support were developed based on my support on the
> em28xx mailinglist.
>
> Now I can use my device (Kaiomy) outside Windows with sound (em28xx-audioep)
> and colors (PAL-M)! I'm using this version everyday with no problems.
>
> Because of this, it will be nice if this work could be included in the
> kernel code. What do you (other users) think about having that driver in
> kernel?

Andr?,

First of all, the big issue why we aren't merging em28xx improvements from
Markus is that he is not following the rules.

For example, you said that you've contributed with Markus tree.

However, on Markus pull request, I see no patch from you on his series of patches.

The correct procedure would be just to forward your patch as-is, adding with his SOB bellow yours.

Not doing this, he would be considered as the author of your patch. IANAL, but
this doesn't seem to be right, from GPL's perspective. Probably, there are
other patches there not authored by Markus that are just merged inside his big patch.

About PAL-M, this always worked at the upstream driver. I have myself lots of
em28xx devices, all working in colors with PAL-M, and with audio. I live in
Brazil, so I always check if PAL-M is ok ;)

If you want to have your device supported, just send us a patch against the
upstream driver.

Cheers,
Mauro

2008-11-08 10:22:29

by Markus Rechberger

[permalink] [raw]
Subject: Re: [PATCH 1/7] Adding empia base driver

On Sat, Nov 8, 2008 at 11:15 AM, Mauro Carvalho Chehab
<[email protected]> wrote:
> On Sat, 8 Nov 2008 03:50:20 -0200
> "Andre Kelmanson" <[email protected]> wrote:
>
>> Dears,
>>
>> I'm using this version of em28xx for a long time and it's working fine. It
>> has three very important features for me. The first one is Kaiomy device,
>> the second one is the new em28xx-audoep module and the third one is PAL-M
>> support. Kaiomy and PAL-M support were developed based on my support on the
>> em28xx mailinglist.
>>
>> Now I can use my device (Kaiomy) outside Windows with sound (em28xx-audioep)
>> and colors (PAL-M)! I'm using this version everyday with no problems.
>>
>> Because of this, it will be nice if this work could be included in the
>> kernel code. What do you (other users) think about having that driver in
>> kernel?
>
> Andr?,
>
> First of all, the big issue why we aren't merging em28xx improvements from
> Markus is that he is not following the rules.
>
> For example, you said that you've contributed with Markus tree.
>
> However, on Markus pull request, I see no patch from you on his series of patches.
>
> The correct procedure would be just to forward your patch as-is, adding with his SOB bellow yours.
>
> Not doing this, he would be considered as the author of your patch. IANAL, but
> this doesn't seem to be right, from GPL's perspective. Probably, there are
> other patches there not authored by Markus that are just merged inside his big patch.
>
> About PAL-M, this always worked at the upstream driver. I have myself lots of
> em28xx devices, all working in colors with PAL-M, and with audio. I live in
> Brazil, so I always check if PAL-M is ok ;)
>
> If you want to have your device supported, just send us a patch against the
> upstream driver.
>

As written earlier already I don't think that I didn't follow any
rules here since I provided single
patches at the very first beginning

http://mcentral.de/v4l-dvb/
(this is all kernel code and only kernel code).

That work didn't get attention and due a different decision of
framework changes (which that codebase relied
on) it all had to be rebased, I doubt that anyone
would have reworked all that patch for patch. Instead it went into one
repository and finally got modified to work again
with the available framework rather than relying onto any such modifications.

The modification which I received from Andr? suggested to use the NTSC
VBI offset lines for PAL-M, and the
rest for audio comes from my side.

Markus

2008-11-08 10:37:59

by Mauro Carvalho Chehab

[permalink] [raw]
Subject: Re: [PATCH 1/7] Adding empia base driver

On Sat, 8 Nov 2008, Markus Rechberger wrote:

> As written earlier already I don't think that I didn't follow any
> rules here since I provided single
> patches at the very first beginning
>
> http://mcentral.de/v4l-dvb/
> (this is all kernel code and only kernel code).
>
> That work didn't get attention and due a different decision of
> framework changes (which that codebase relied
> on) it all had to be rebased, I doubt that anyone
> would have reworked all that patch for patch. Instead it went into one
> repository and finally got modified to work again
> with the available framework rather than relying onto any such modifications.

One thing is to rebase a tree, another is to merge all patches into a big
one, not preserving the original authorships.

Development trees sometimes need rebase. This is done by popping all newer
patches from the tree, applying the upstream patches, and then pushing
again every individual patches, fixing the ones that don't apply well, but
preserving their authorships.

The modified patches should receive a special tag before the
maintainer's SOB, like:

[me@mymail: I did this to apply this patch]

as stated at the kernel docs.

This method will reduce a lot the risk of breaking improvements and other
fixes that happened upstream, and will properly preserve authorship of
individual patches.

If you were doing a rebase, your patches would likely be accepted.

--
Cheers,
Mauro

2008-11-08 10:43:17

by Markus Rechberger

[permalink] [raw]
Subject: Re: [PATCH 1/7] Adding empia base driver

On Sat, Nov 8, 2008 at 11:37 AM, Mauro Carvalho Chehab
<[email protected]> wrote:
> On Sat, 8 Nov 2008, Markus Rechberger wrote:
>
>> As written earlier already I don't think that I didn't follow any
>> rules here since I provided single
>> patches at the very first beginning
>>
>> http://mcentral.de/v4l-dvb/
>> (this is all kernel code and only kernel code).
>>
>> That work didn't get attention and due a different decision of
>> framework changes (which that codebase relied
>> on) it all had to be rebased, I doubt that anyone
>> would have reworked all that patch for patch. Instead it went into one
>> repository and finally got modified to work again
>> with the available framework rather than relying onto any such
>> modifications.
>
> One thing is to rebase a tree, another is to merge all patches into a big
> one, not preserving the original authorships.
>
> Development trees sometimes need rebase. This is done by popping all newer
> patches from the tree, applying the upstream patches, and then pushing again
> every individual patches, fixing the ones that don't apply well, but
> preserving their authorships.
>
> The modified patches should receive a special tag before the maintainer's
> SOB, like:
>
> [me@mymail: I did this to apply this patch]
>
> as stated at the kernel docs.
>
> This method will reduce a lot the risk of breaking improvements and other
> fixes that happened upstream, and will properly preserve authorship of
> individual patches.
>
> If you were doing a rebase, your patches would likely be accepted.
>

Should I start picking patches from the linuxtv.org tree where patches
from my tree are taken
and where the Sign off is not provided?

Just recently one patch went into not even stating that it comes from
my codebase, although not
worth the time making an elephant out of it.

Markus

2008-11-08 10:46:19

by Markus Rechberger

[permalink] [raw]
Subject: Re: [PATCH 1/7] Adding empia base driver

On Sat, Nov 8, 2008 at 11:42 AM, Markus Rechberger
<[email protected]> wrote:
> On Sat, Nov 8, 2008 at 11:37 AM, Mauro Carvalho Chehab
> <[email protected]> wrote:
>> On Sat, 8 Nov 2008, Markus Rechberger wrote:
>>
>>> As written earlier already I don't think that I didn't follow any
>>> rules here since I provided single
>>> patches at the very first beginning
>>>
>>> http://mcentral.de/v4l-dvb/
>>> (this is all kernel code and only kernel code).
>>>
>>> That work didn't get attention and due a different decision of
>>> framework changes (which that codebase relied
>>> on) it all had to be rebased, I doubt that anyone
>>> would have reworked all that patch for patch. Instead it went into one
>>> repository and finally got modified to work again
>>> with the available framework rather than relying onto any such
>>> modifications.
>>
>> One thing is to rebase a tree, another is to merge all patches into a big
>> one, not preserving the original authorships.
>>
>> Development trees sometimes need rebase. This is done by popping all newer
>> patches from the tree, applying the upstream patches, and then pushing again
>> every individual patches, fixing the ones that don't apply well, but
>> preserving their authorships.
>>
>> The modified patches should receive a special tag before the maintainer's
>> SOB, like:
>>
>> [me@mymail: I did this to apply this patch]
>>
>> as stated at the kernel docs.
>>
>> This method will reduce a lot the risk of breaking improvements and other
>> fixes that happened upstream, and will properly preserve authorship of
>> individual patches.
>>
>> If you were doing a rebase, your patches would likely be accepted.
>>
>

let's link back to the only review done:
http://linux.derkeiler.com/Mailing-Lists/Kernel/2008-11/msg00060.html

<snip>
In my opinion it's pretty much hopeless trying to convert the current
em28xx driver into what you have. It's a huge amount of work that no
one wants to do and (in this case) with very little benefit.
</snip>

Markus

2008-11-08 10:56:58

by Mauro Carvalho Chehab

[permalink] [raw]
Subject: Re: [PATCH 1/7] Adding empia base driver

On Sat, 8 Nov 2008, Markus Rechberger wrote:

> Should I start picking patches from the linuxtv.org tree where patches
> from my tree are taken
> and where the Sign off is not provided?

SOB's are just meant to testify that the code is GPL. It has nothing to do
with authorship.

On all cases I'm aware, when some code from your tree were merged
upstream, your authorship were marked inside the patch's description.
Also, your authorship are explicit at the em28xx files you've contributed.

--
Cheers,
Mauro Carvalho Chehab
http://linuxtv.org
[email protected]

2008-11-08 11:02:36

by Markus Rechberger

[permalink] [raw]
Subject: Re: [PATCH 1/7] Adding empia base driver

On Sat, Nov 8, 2008 at 11:56 AM, Mauro Carvalho Chehab
<[email protected]> wrote:
> On Sat, 8 Nov 2008, Markus Rechberger wrote:
>
>> Should I start picking patches from the linuxtv.org tree where patches
>> from my tree are taken
>> and where the Sign off is not provided?
>
> SOB's are just meant to testify that the code is GPL. It has nothing to do
> with authorship.
>
> On all cases I'm aware, when some code from your tree were merged upstream,
> your authorship were marked inside the patch's description. Also, your
> authorship are explicit at the em28xx files you've contributed.
>

really?
http://linuxtv.org/hg/v4l-dvb/rev/5f3c3af9c1f9

http://article.gmane.org/gmane.linux.drivers.dvb/44726

nevermind.

Markus

2008-11-26 19:12:19

by Aidan Thornton

[permalink] [raw]
Subject: Re: [PATCH 1/7] Adding empia base driver

On 10/22/08, Markus Rechberger <[email protected]> wrote:
> On Thu, Oct 23, 2008 at 12:09 AM, Greg KH <[email protected]> wrote:
>> On Wed, Oct 22, 2008 at 11:14:36PM +0200, Markus Rechberger wrote:
>>> em2880-dvb:
>>> * supporting the digital part of Empia based devices, which
>>> includes ATSC, ISDB-T and DVB-T
>>
>> <snip>
>>
>> Doesn't this driver duplicate some of the existing devices we already
>> support with the current in-kernel driver? If so, why not just add the
>> new device support to the existing driver instead of duplicating
>> everything?
>>
>> This is going to cause a big problem for distros as they will not know
>> which to enable, so they will probably just disable this one, which is
>> what I don't think you want to have happen :(
>>
>
> the current driver doesn't support most devices which are in there,
> also the alsa
> audio driver can easily crash the whole system. (It's my code so I
> know what was wrong there).
>
> The core video code is already too much off, the VBI code added alot
> complexity
> to it it does frame slicing on the fly.
>
> Those devices ship VBI+VIDEO within 1 datastream, VBI and Video aren't
> that different
> in the system. both interfaces provide framebuffers through a mmap'ed
> interface.
> If all the VBI buffers are filled the data has to be sliced off in any
> case while providing
> the same bottom data ot the Video interface

Hi,

Just to check, your driver does handle this correctly now, right? Last
time I checked, it was a buggy mess with severely broken locking that
caused a kernel panic half the time when recording whith MythTV (and
indeed, had been for years). It looks, at a glance, like this may have
been fixed, but since I never quite managed to pin down the exact
cause, I can't say for sure.

(The code is, alas, still somewhat ugly compared to the existing
videobuf-based code in the in-kernel driver - it should be possible to
add VBI support to that fairly cleanly and easily. I actually
attempted this a while back, but ran into issues due to not having
access to the datasheets. Of course, the fact that Markus works there
and is actively aggressive to any Linux driver development that
doesn't go through him.)

> http://mcentral.de/hg/~mrec/em28xx-new/shortlog (there are more than
> 200 changelog entries
> what happened in detail).
>
> The development has been split off (first due limitations in the
> kernel, afterwards due ..., and finally
> due the restriction that all that has to work without a framework
> upgrade on the eeePC).

This doesn't exactly inspire confidence; it seems to basically mean
you forked or rewrote existing drivers rather than just using code
that was already there.

> diffing the 2 available drivers shows up that only the core is twice
> as big as the one which is currently
> in the driver (the result of 2-3 years asynchronous development).

Actually, part of the reason for this is probably that there's a
massive bunch of video buffer handling code in your driver that mostly
repeats stuff handled by the videobuf module in the in-kernel driver.
(Admittedly, your does slightly more in that it handles raw VBI, but
as I've said it should be possible to do that cleanly and in a smaller
amount of code using videobuf.)

> The driver is currently also tested with signal generators (different
> inputs, and different video standards).
>
> Very likely the best would be to replace the available driver with it
> but I don't care, alot people use and have been using the driver from
> mcentral.de for a long time, development has always been opensource
> there too.

Just not necessarily open source under a GPL compliant license, as I recall.

As I've said before, it'd be really nice if you'd make an effort to
actually integrate your changes, but this looks like more of the same
old mess.

Aidan

2008-11-26 20:36:39

by Aidan Thornton

[permalink] [raw]
Subject: Re: [PATCH 1/7] Adding empia base driver

Hi,

On 11/1/08, Hans Verkuil <[email protected]> wrote:
> Hi Markus,
>
> As promised I've done a review of your empia driver and looked at what
> needs to be done to get it into the kernel.
>
> First of all, I've no doubt that your empia driver is better and
> supports more devices than the current em28xx driver. I also have no
> problem adding your driver separate from the current driver. It's been
> done before (certain networking drivers spring to mind) and while
> obviously not ideal I expect that the older em28xx driver can probably
> be removed after a year or something like that.
>
> In my opinion it's pretty much hopeless trying to convert the current
> em28xx driver into what you have. It's a huge amount of work that no
> one wants to do and (in this case) with very little benefit. Of course,
> Mauro has the final say in this.

The main reason no-one wants to do it is partly because it's a large
amount of work, and partly because Markus is the only one able to test
a decent subset of the available hardware and he has objections to the
idea. To be honest, the diffs look somewhat worse than they actually
are - there's a lot of noise and a lot of changes that will just be
dumped if this approach is used.

Incidentally, the new code has some odd quirks. For example, why does
opening an ALSA device switch the card from digital to radio mode,
even when the card doesn't support radio? In particular, unless I'm
missing something, this would mean that any app that opened the ALSA
device before the video device would get stuck in radio mode. (Also,
why is there tuner-specific code in the ALSA driver?) Why the bizarre
indirection through em28xx_cmd, which only has one possible value for
cmd?

> While there is some cleaning up to do in your code (coding style, some
> copyright/license clarifications), that is not a big deal. The coding
> style cleanups are a fair amount of work, but I think we can probably
> spread that out over several people and get that done quickly.
>
> What I definitely would recommend is to use video_ioctl2 rather than
> video_usercopy. The latter function will disappear in the future. I
> think the policy for new drivers is to use video_ioctl2, so this is
> probably a required task before it can be merged. Doing this will
> improve maintenance of the code as well, so it's useful to do this
> anyway. I think it's better if you do it, but I guess some volunteer
> can probably be found if needed. It's not a difficult task.
>
> Now the real problems are with three duplicate i2c drivers: cx25843,
> xc3028 and xc5000.
>
> To start with the easiest one: cx25843. There already is a driver for
> this (cx25840) and the empia driver should use that one. Since I
> maintain cx25840 the easiest approach for you is to see if you can get
> me hardware (em28xx + cx25843) so that I can test and update cx25843 to
> provide proper empia support. The alternative is that we work together
> on this, but that's probably more work for both of us. I most
> definitely do *not* want to duplicate this driver. Windows drivers
> duplicate this stuff all the time, but we're not Windows and having one
> driver ensures that fixes or new functionality become available to all
> bridge drivers that use it.
>
> The same reasoning is true for xc3028 and xc5000. In addition, the
> licensing of these sources is very vague. Is it even allowed to
> distribute them under a GPL license? There is no GPL license in the
> sources, yet in the module you specify GPL. This needs to be clarified
> first.
>
> I see two ways forward when it comes to these drivers:
>
> 1) The empia driver switches to the current xceive drivers that are
> already in the kernel. No doubt this means that xceive driver bugs will
> surface with certain devices, but those are bugs that the xceive driver
> maintainer will have to fix. Obviously assistance would be appreciated,
> but the bottom line is that a) your driver is finally in the kernel,
> and b) there is a lot more pressure to fix bugs in the xceive drivers
> than is the case otherwise. Yes, some devices will not work initially
> with the empia driver, but I expect that to be a temporary situation.

The trouble is, it looks like the core empia code is pretty tightly
integrated with the xc3028 driver - in particular, there's lots of
xc3028-specific code in empia/empia-cards.c. (This probably isn't a
good thing in itself.) Ideally, Markus would've used the in-kernel
driver for at least xc3028 - it was developed and mostly working at
that point - but I suppose that was fairly unlikely to happen.
(There's also a bunch of tuner-specific code in empia-dvb.c that
wouldn't be needed if the proper framework was used.)

Plus, there are (as usual) some direct writes to demodulators by
empia-dvb.c that need to be cleaned up.

> 2) Your xceive drivers replace the current drivers. This will require
> that a) the license situation is clarified, b) the drivers are modified
> to fit the current v4l-dvb tuner architecture. This option will mean a
> lot of work for you since you are the maintainer of these drivers. In
> addition, I forsee a lot of flaming if we go this way.

Of course, the same issue applies to this option - the empia and
xc3028 code appear to be fairly closely entangled.

In fact, since most of the work is, I suspect, going to be dealing
with this issue, just merging the required changes into the existing
em28xx driver starts to look like quite a plausible option. It
basically boils down to:

(a) audio changes, some of which are undesirable, and the rest of
which can be done by a (hopefully relatively simple) patch to the
current driver - I can just about see most of the changes in the diff.
(b) VBI, which can't all be ported directly but should be fairly easy
to add - plus, doing it that way is cleaner.
(c) some modifications to the card initialisation functions and glue
code, which ought to be simple to port but will require someone with
access to the hardware to test and debug them. (This means Markus,
basically).
(b) a bunch of formatting changes, noise and, now-redundant code with
the occasional useful change to trap the unwary.

(Interestingly, aside from VBI, the now-redundant buffer management
code, duplicate drivers, init/glue code for new cards, and misplaced
tuner-specific stuff, most of the significant changes seem to be code
that's in the existing driver but not the new. This is probably not a
good sign. Unfortunately, there's a lot of diff noise, partly due to
some of the functions being reordered at some point, partly due to
changes in stuff like indentation and format of register #defines, and
partly due to diff sucking, so it's a bit hard to see what exactly
changed in places.)

Of course, since Markus probably won't go for this, it's not really an
option. That's a shame, because it'd probably work out best.

Sorry about the rather late mail; I don't generally pay attention to
em28xx development anymore. I used to be more involved, but my card
died and I moved to hardware with a less messy driver situation.

Aidan Thornton

2008-11-27 04:25:49

by Markus Rechberger

[permalink] [raw]
Subject: Re: [PATCH 1/7] Adding empia base driver

On Wed, Nov 26, 2008 at 8:12 PM, Aidan Thornton <[email protected]> wrote:
> On 10/22/08, Markus Rechberger <[email protected]> wrote:
>> On Thu, Oct 23, 2008 at 12:09 AM, Greg KH <[email protected]> wrote:
>>> On Wed, Oct 22, 2008 at 11:14:36PM +0200, Markus Rechberger wrote:
>>>> em2880-dvb:
>>>> * supporting the digital part of Empia based devices, which
>>>> includes ATSC, ISDB-T and DVB-T
>>>
>>> <snip>
>>>
>>> Doesn't this driver duplicate some of the existing devices we already
>>> support with the current in-kernel driver? If so, why not just add the
>>> new device support to the existing driver instead of duplicating
>>> everything?
>>>
>>> This is going to cause a big problem for distros as they will not know
>>> which to enable, so they will probably just disable this one, which is
>>> what I don't think you want to have happen :(
>>>
>>
>> the current driver doesn't support most devices which are in there,
>> also the alsa
>> audio driver can easily crash the whole system. (It's my code so I
>> know what was wrong there).
>>
>> The core video code is already too much off, the VBI code added alot
>> complexity
>> to it it does frame slicing on the fly.
>>
>> Those devices ship VBI+VIDEO within 1 datastream, VBI and Video aren't
>> that different
>> in the system. both interfaces provide framebuffers through a mmap'ed
>> interface.
>> If all the VBI buffers are filled the data has to be sliced off in any
>> case while providing
>> the same bottom data ot the Video interface
>
> Hi,
>
> Just to check, your driver does handle this correctly now, right? Last
> time I checked, it was a buggy mess with severely broken locking that
> caused a kernel panic half the time when recording whith MythTV (and
> indeed, had been for years). It looks, at a glance, like this may have
> been fixed, but since I never quite managed to pin down the exact
> cause, I can't say for sure.
>

the exact cause was that the vbi buffer handling was connected with
the video buffer handling
back then. It crashed at certain scenarios.
VBI has been reworked completely since then, also the offsets. It's a
hardcoded em28xx - videodecoder setup.

> (The code is, alas, still somewhat ugly compared to the existing
> videobuf-based code in the in-kernel driver - it should be possible to
> add VBI support to that fairly cleanly and easily. I actually
> attempted this a while back, but ran into issues due to not having
> access to the datasheets. Of course, the fact that Markus works there
> and is actively aggressive to any Linux driver development that
> doesn't go through him.)
>
>> http://mcentral.de/hg/~mrec/em28xx-new/shortlog (there are more than
>> 200 changelog entries
>> what happened in detail).
>>
>> The development has been split off (first due limitations in the
>> kernel, afterwards due ..., and finally
>> due the restriction that all that has to work without a framework
>> upgrade on the eeePC).
>
> This doesn't exactly inspire confidence; it seems to basically mean
> you forked or rewrote existing drivers rather than just using code
> that was already there.
>

well I initially worked on that driver since 2005(!) and at least
helped more than
thousand users to get their devices work.

>> diffing the 2 available drivers shows up that only the core is twice
>> as big as the one which is currently
>> in the driver (the result of 2-3 years asynchronous development).
>
> Actually, part of the reason for this is probably that there's a
> massive bunch of video buffer handling code in your driver that mostly
> repeats stuff handled by the videobuf module in the in-kernel driver.
> (Admittedly, your does slightly more in that it handles raw VBI, but
> as I've said it should be possible to do that cleanly and in a smaller
> amount of code using videobuf.)
>
>> The driver is currently also tested with signal generators (different
>> inputs, and different video standards).
>>
>> Very likely the best would be to replace the available driver with it
>> but I don't care, alot people use and have been using the driver from
>> mcentral.de for a long time, development has always been opensource
>> there too.
>
> Just not necessarily open source under a GPL compliant license, as I recall.
>

I pointed out a couple of times that the license is no issue with the
exported sources.

> As I've said before, it'd be really nice if you'd make an effort to
> actually integrate your changes, but this looks like more of the same
> old mess.
>

The driver has been exported so far, that's all I'll ever offer. In
future to avoid those clashes I'll
simply use the i2c-dev framework which is commonly available and
allows to control
those chips from userland without any extra module. We already have a
player which
is optimized for those devices and also properly supports that interface.

I simply don't have the time for those games anymore, and those people
here who have personal
issues with me and/or my work don't pay my monthly bills, neither do
they have any clue how that
driver is used with some systems of certain customers. I constantly
deliver full solutions, and not
driver code only. Neither am I limited to Linux only.

Markus

2008-11-27 09:33:22

by Pekka Enberg

[permalink] [raw]
Subject: Re: [PATCH 1/7] Adding empia base driver

On Thu, Nov 27, 2008 at 6:25 AM, Markus Rechberger
<[email protected]> wrote:
> I simply don't have the time for those games anymore, and those people here who have personal
> issues with me and/or my work don't pay my monthly bills, neither do they have any clue how that
> driver is used with some systems of certain customers. I constantly deliver full solutions, and not
> driver code only. Neither am I limited to Linux only.

Again, it's not personal, it's the way we do things here. But you
should already know that by now so whatever.

What puzzles me the most here is why isn't Empia simply providing
datasheets to other developers as well. I mean, even the GPU and
wireless vendors know how to play well with the community now, so
what's the problem? Being locked to just one developer who's unwilling
to cooperate doesn't sound like a winning strategy to me.

Greg, is there any reason we don't have Markus' out-of-tree driver
listed here, btw:

http://www.linuxdriverproject.org/twiki/bin/view/Main/OutOfTreeDrivers

Maybe there are people in the "Linux Driver Project" that could be
interested in working on this but simply don't know about it?

Pekka

2008-11-28 15:48:18

by Devin Heitmueller

[permalink] [raw]
Subject: Re: [PATCH 1/7] Adding empia base driver

On Thu, Nov 27, 2008 at 4:33 AM, Pekka Enberg <[email protected]> wrote:
> On Thu, Nov 27, 2008 at 6:25 AM, Markus Rechberger
> <[email protected]> wrote:
>> I simply don't have the time for those games anymore, and those people here who have personal
>> issues with me and/or my work don't pay my monthly bills, neither do they have any clue how that
>> driver is used with some systems of certain customers. I constantly deliver full solutions, and not
>> driver code only. Neither am I limited to Linux only.
>
> Again, it's not personal, it's the way we do things here. But you
> should already know that by now so whatever.
>
> What puzzles me the most here is why isn't Empia simply providing
> datasheets to other developers as well. I mean, even the GPU and
> wireless vendors know how to play well with the community now, so
> what's the problem? Being locked to just one developer who's unwilling
> to cooperate doesn't sound like a winning strategy to me.
>
> Greg, is there any reason we don't have Markus' out-of-tree driver
> listed here, btw:
>
> http://www.linuxdriverproject.org/twiki/bin/view/Main/OutOfTreeDrivers
>
> Maybe there are people in the "Linux Driver Project" that could be
> interested in working on this but simply don't know about it?
>
> Pekka
> --

Hello Pekka,

In Empia's defense, they actually have recently provided the
datasheets to others under NDA, such as myself. This is one of the
reasons the in-kernel em28xx driver has improved significantly in the
last month (including new support for the em2874 and fixes to
em2860/em2880 support).

The in-kernel driver will continue to improve, device support will
continue to increase as I work with the people who have devices that
people care about.

Markus has made it very clear that he does not want any of his code
merged into the existing in-kernel em28xx driver. Depsite the fact
that the license specifically allows doing exactly that, I have made
an effort to respect his feelings in this manner.

Regards,

Devin

--
Devin J. Heitmueller
http://www.devinheitmueller.com
AIM: devinheitmueller

2008-11-28 20:23:59

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH 1/7] Adding empia base driver

On Thu, Nov 27, 2008 at 11:33:03AM +0200, Pekka Enberg wrote:
> On Thu, Nov 27, 2008 at 6:25 AM, Markus Rechberger
> <[email protected]> wrote:
> > I simply don't have the time for those games anymore, and those people here who have personal
> > issues with me and/or my work don't pay my monthly bills, neither do they have any clue how that
> > driver is used with some systems of certain customers. I constantly deliver full solutions, and not
> > driver code only. Neither am I limited to Linux only.
>
> Again, it's not personal, it's the way we do things here. But you
> should already know that by now so whatever.
>
> What puzzles me the most here is why isn't Empia simply providing
> datasheets to other developers as well. I mean, even the GPU and
> wireless vendors know how to play well with the community now, so
> what's the problem? Being locked to just one developer who's unwilling
> to cooperate doesn't sound like a winning strategy to me.
>
> Greg, is there any reason we don't have Markus' out-of-tree driver
> listed here, btw:
>
> http://www.linuxdriverproject.org/twiki/bin/view/Main/OutOfTreeDrivers

It's a wiki, feel free to add it yourself :)

thanks,

greg k-h

2008-11-30 06:23:45

by Markus Rechberger

[permalink] [raw]
Subject: Re: [PATCH 1/7] Adding empia base driver

On Fri, Nov 28, 2008 at 9:09 PM, Greg KH <[email protected]> wrote:
> On Thu, Nov 27, 2008 at 11:33:03AM +0200, Pekka Enberg wrote:
>> On Thu, Nov 27, 2008 at 6:25 AM, Markus Rechberger
>> <[email protected]> wrote:
>> > I simply don't have the time for those games anymore, and those people here who have personal
>> > issues with me and/or my work don't pay my monthly bills, neither do they have any clue how that
>> > driver is used with some systems of certain customers. I constantly deliver full solutions, and not
>> > driver code only. Neither am I limited to Linux only.
>>
>> Again, it's not personal, it's the way we do things here. But you
>> should already know that by now so whatever.
>>
>> What puzzles me the most here is why isn't Empia simply providing
>> datasheets to other developers as well. I mean, even the GPU and
>> wireless vendors know how to play well with the community now, so
>> what's the problem? Being locked to just one developer who's unwilling
>> to cooperate doesn't sound like a winning strategy to me.
>>
>> Greg, is there any reason we don't have Markus' out-of-tree driver
>> listed here, btw:
>>
>> http://www.linuxdriverproject.org/twiki/bin/view/Main/OutOfTreeDrivers
>
> It's a wiki, feel free to add it yourself :)
>
> thanks,
>
> greg k-h
>

hah I fully understand now. I've been blind. There's no better way to
go than the current one for
someone here who recently got hired at Redhat even if he doesn't
deserve it for real.

I hope some other people are able to convince me about OSS again. I
have alot respect of
ffmpeg people actually. Writing v4l drivers is damn easy actually
callbacks here callbacks there
learning some structs and dumping data 1:1 to userland and someone is
done already.

This is the second project which goes that way, the first one was
polite enough to just take my
code and remove the copyrights .. after noticing this - after a year-
I got a nice reply that the work
was inspired by even when containing the same comments...

this is my last email to LKML, even when continuing to support Linux I
won't loose focus at other
operating systems. Thanks for teaching me about GPL stuff, I now have
more faith into the BSD
license if at all in the OSS area.

Markus