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

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

[Hans Verkuil]

(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

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

[Andy Walls]

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

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

[Devin Heitmueller]

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

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

[Pekka Enberg]

Hi Devin,

On Sat, Nov 1, 2008 at 5:46 PM, Devin Heitmueller
<devin.heitmueller@gmail.com> 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

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

[Devin Heitmueller]

On Sat, Nov 1, 2008 at 12:08 PM, Pekka Enberg <penberg@cs.helsinki.fi> wrote:
> Hi Devin,
>
> On Sat, Nov 1, 2008 at 5:46 PM, Devin Heitmueller
> <devin.heitmueller@gmail.com> 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

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

[Hans Verkuil]

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

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

[Pekka Enberg]

On Sat, Nov 1, 2008 at 6:21 PM, Hans Verkuil <hverkuil@xs4all.nl> 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.

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

[Pekka Enberg]

On Sat, Nov 1, 2008 at 6:19 PM, Devin Heitmueller
<devin.heitmueller@gmail.com> 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...

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

[Devin Heitmueller]

Hello Hans,

Thanks for getting back to me.  Responses inline:

On Sat, Nov 1, 2008 at 12:21 PM, Hans Verkuil <hverkuil@xs4all.nl> 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

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

[Markus Rechberger]

On Sat, Nov 1, 2008 at 5:29 PM, Pekka Enberg <penberg@cs.helsinki.fi> wrote:
> On Sat, Nov 1, 2008 at 6:19 PM, Devin Heitmueller
> <devin.heitmueller@gmail.com> 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

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

[Markus Rechberger]

On Sat, Nov 1, 2008 at 5:51 PM, Devin Heitmueller
<devin.heitmueller@gmail.com> wrote:
> Hello Hans,
>
> Thanks for getting back to me.  Responses inline:
>
> On Sat, Nov 1, 2008 at 12:21 PM, Hans Verkuil <hverkuil@xs4all.nl> 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