Hi, On 11/1/08, Hans Verkuil <hverkuil@xxxxxxxxx> 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 _______________________________________________ linux-dvb mailing list linux-dvb@xxxxxxxxxxx http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
