On 5/15/07, hermann pitton <hermann-pitton@xxxxxxxx> wrote:
Am Montag, den 14.05.2007, 20:22 -0400 schrieb Michael Krufky: > Markus Rechberger wrote: > > As from my side I don't have the time to do a major rewrite or doing > > some reordering again, please try to make the best out of what's > > available now. > > > > The patches include more than 9000 lines of codechanges through the > > v4l and dvb framework, these changes have been done during the last 1 > > 1/2 years of split development from the main v4l-dvb code, I know it's > > alot but it's worth to get it done now. It's not only about adding > > support for one new device only. > > I know we've been over this, but I need to state my feelings on the matter, otherwise I would have to simply keep quiet, and that doesn't help the situation for anybody. > > I _strongly_ feel that the core changes being proposed here will hurt the integrity of the dvb subsystem. The main arguments for accepting these changes is that it "adds support for over 60 devices" and that the author "doesn't have any more time to spend on this." > > The argument currently on the table, is that these changes get merged now, and we fix any problems later. I feel that this sets a bad president. Who is to stop the NEXT developer from changing around the subsystem core against the desires of those developers actively maintaining the project? > > We have an established internal API, and developers should keep subsystem codingstyle intact. If we merge this now, who is to say that we will _ever_ get around to 'fixing' it? We are having enough problems as it is trying to agree on a way to merge the xc3028 driver into the kernel -- how will we ever agree on the 'fixup' method later? The answer is simple -- we'll never agree. > > What does this mean? It means that we have to compromise. The ability to compromise is very important when more than one person is working on a project. Heck, the word 'compromise' always comes up when people talk about marriage, living situations, etc. It also applies to discussions such as this. > > Again, as I understand, the most significant argument for pushing this in, is to finally get driver support for these sixty some-odd devices into the kernel. Please keep in mind that we're talking about a single tuner ic (xc3028) and the sixty devices that depend on the presence of the xc3028 driver. We _are_ talking about a single device (ic). > > Even still, if the priority is to merge support for the xc3028 tuner ic into the kernel in the quickest way possible, then there is a _much_ lesser evil available as an option. > > I do frown upon code duplication, however, in this case it is a safer alternative to the one currently on the table. Earlier versions of the xc3028 tuner driver were bound to the video4linux tuner.ko module, for the sake of tuning analog television stations. There was a wrapper present inside the em2880-dvb driver that allowed the dvb subsystem to access the xc3028 via tuner.ko. I am not a big fan of this solution, however, it does not touch any core subsystem code. DVB-only devices can use a separate module in order to access the xc3028 without imposing dependencies on the v4l core. > > I have already written such a module, based on Markus' work, and it has already been proven in the field as a working solution. see xc3028-fe.c in: http://www.linuxtv.org/~mkrufky/xc-bluebird.patch > > The only argument against this method, is that it requires some code duplication -- two drivers for the xc3028. One for analog via the v4l subsystem, the other for dvb via the dvb subsystem. I repeat -- I am NOT promoting code duplication, however, given the circumstances, and the fact that the reason for all the controversy is that people want the xc3028 driver merged into the kernel asap, this solution is truly the lesser of two evils. We already have some code duplicated for simple pll's such as the fmd1216me and lgh06xf tuners -- the only difference is the size of the code. > > If we decide to go this route, it will truly be the best compromise -- It will allow us to merge in support for the sixty some-odd devices that Markus is talking about, and it will allow for easy development of newer devices that use this tuner ic. The major benefit of this method is that it _will_ allow for us developers to put our heads together after the fact, and find better ways of supporting hybrid tuners. At that point, the pressure of 'trying to merge support for sixty devices' will be gone. Developers will finally be able to discuss this issue without the pressure of the current pending issues, and we _will_ be able to find a better solution. > > As far as I can tell, it seems that this is the only way for us to push this through, while not upsetting other developers. However, I must repeat that following my proposal does create a maintenance problem -- It means that any changes to the xc3028-tuner module will have to be carried over to the xc3028-fe module. Once the two modules get merged, however, it will become more apparent to developers as to the reasons why we need a better solution. I do believe that this will be a catalyst to discovering a proper solution later on. If we decide to go this route, then I will volunteer to keep up the maintenance of the xc3028-fe module until that proper, better solution is agreed upon. > > Regards, > > Michael Krufky > Hi Michael, all good arguments. To force anything does only more bad, not to force anything causes always NACKs and nothing happens. That is the situation since ever on the hybrid stuff! We need proper locking between v4l and dvb. What we currently have is ridiculous.
this is what I took care about for the em28xx. But this is also very device dependent so each device has to find its own way for doing it the right way. For example some devices might be able to use composite in and DVB at the same time so why should we lock one framework in that case? In case of the em28xx/em2880-dvb driver this was easy I'm just checking the user counter (and I also modified the dvb framework to complete the usercounter implementation for a few devicenodes, I also read the comment in the sourcecode "now we can remove the usercounter", I still think it's useful to have it in there. Another thing that I noticed is that the usercounter initialization isn't consistent through the dvb framework, sometimes it's initialized with -1, another time with +1 which doesn't make much sense anymore since there's no special dependency on that but I didn't clean it up) In case of not going that way now I spent alot time on the hybrid tuner design, I proposed that we can go an easier way already but that caused much noise already because everyone had his own idea but noone seriously came up with something acceptable a few people just threw their incomplete ideas to the discussions and expected that everyone will fully understand what ideas they have about how it should look like. Noone ever sent me an email if I want to participate redoing it with someone or noone ever asked me if he can participate at what I'm doing - everyone wanted to do it alone here which seems to be the biggest problem. I really don't care about touching the dvb core to improve the whole issue, when merging the code I stumbled over a few changes which didn't look very sane anyway so there mustn't be any lock on development of that code (just to satisfy a few people who also have been with that project for ages). People should be invited to read through it and modify it if they come up with something that improves it. I also have pending patches from Geniatech [1] which would add full support for their cx88 based devices and which rely on these frameworkchanges too, I will not rebase any code anymore since I've done that already for nothing before. If there are any objections please state them out, there are still a few problems with that i2c check which need to be solved, this is what I'm really concerned about. And this also seems to be an existing bug in the Kernel at the moment. The problem with delaying all that work is that if someone applies changes to the main tree the patchsets have to be modified too again and this generates and already generated alot work. I spent quite a few weekends and nights on keeping the whole tree uptodate to at least beeing able to go on with development. Markus [1] b/xc3028_channelmaps.h | 2510 ++++++++++++++++++++++++++++ b/xc3028_scodes.h | 595 ++++++ linux/drivers/media/common/ir-keymaps.c | 47 linux/drivers/media/dvb/frontends/zl10353.c | 8 linux/drivers/media/tuners/xc3028-tuner.c | 532 +++-- linux/drivers/media/tuners/xc3028-tuner.h | 6 linux/drivers/media/video/cx88/cx88-cards.c | 93 - linux/drivers/media/video/cx88/cx88-dvb.c | 110 + linux/drivers/media/video/cx88/cx88-i2c.c | 106 + linux/drivers/media/video/cx88/cx88-input.c | 8 linux/drivers/media/video/cx88/cx88-mpeg.c | 5 linux/drivers/media/video/cx88/cx88.h | 9 linux/drivers/media/video/tuner-core.c | 3 linux/include/media/ir-common.h | 3 14 files changed, 3807 insertions(+), 228 deletions(-) _______________________________________________ linux-dvb mailing list linux-dvb@xxxxxxxxxxx http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
