Date:	Wed, 26 Nov 2008 19:12:04 +0000
From:	"Aidan Thornton" <makosoft@...glemail.com>
To:	"Markus Rechberger" <mrechberger@...il.com>
Cc:	"Greg KH" <greg@...ah.com>,
	"Linux Kernel Mailing List" <linux-kernel@...r.kernel.org>,
	em28xx <em28xx@...ntral.de>, acano@...tmail.fm,
	"Andre Kelmanson" <akelmanson@...il.com>,
	"Bouwsma Barry" <freebeer.bouwsma@...il.com>,
	"Dan Kreiser" <kreiser@...ormatik.hu-berlin.de>,
	"Frank Neuber" <fn@...nelport.de>,
	"Jelle de Jong" <jelledejong@...ercraft.nl>,
	"John Stowers" <john.stowers.lists@...il.com>,
	"Lukas Kuna" <lukas.kuna@...anet.net>,
	"Stefan Vonolfen" <stefan.vonolfen@...il.com>,
	"Stephan Berberig" <s.berberig@...or.de>,
	"Thomas Giesecke" <thomas.giesecke@...mbh-naumburg.de>,
	"Vitaly Wool" <vwool@...mvista.com>,
	"Zhenyu Wang" <zhen78@...il.com>
Subject: Re: [PATCH 1/7] Adding empia base driver

On 10/22/08, Markus Rechberger <mrechberger@...il.com> wrote:
> On Thu, Oct 23, 2008 at 12:09 AM, Greg KH <greg@...ah.com> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists