Michael Krufky wrote: > Oliver Endriss wrote: > > >Michael Krufky wrote: > > > > > >>Andreas Oberritter wrote: > >> > >>>Michael Krufky wrote: > >>> > >>>>I think that this was a good idea, although the same exact thing could > >>>>be done for all of the other drivers ... > >>>> > >>>I like this patch and I think it should be applied. It is a disadvantage > >>>to have all the code and arrays duplicated in several drivers if we > >>>could have it at a single place. > >>> > >>>The code can still be changed to use dvb-pll afterwards without > >>>unnecessary code duplication. > >>> > >>I can agree to that. The same can be done for lg-h06xf, and many > >>others. A lot of duplicated code could be removed, and I do agree that > >>this would be a step forward. The only problem I see with this is that > >>we'll end up with many tiny little header files just like this one, > >>bsbe1.h and bsru6.h ... This isn't necessarily a bad thing either. I > >>just didn't know if this is what we wanted to be doing. > >> > >Well, when we started with bsbe1.h and bsru6.h the patches were posted > >on this mailing list. Nobody complained... > > > Nobody complained because there is nothing wrong with it. (except for > one flaw -- see down below) I just had a few ideas I thought we could > talk about. Feel free to disagree with me -- IMHO, discussions like > this are good. > > >>Would it make sense to consolidate these small files into single source.[ch] files? > >>What do you think? > >> > >I don't like big files which contain lots of definitions which are not > >needed by most drivers. That's why I'm not very happy with dvb_pll.c. > > > OK. I agree to keep these separate, but dvb-pll is a good thing, and I > wouldn't want to see that split apart. When it comes to pll > programming, it makes sense to store the ranges in an array like we do > in dvb-pll, and to have a single function like dvb_pll_configure to > intepret it. I would hate to see that go away. Well, there is no reason to change that. I can live with it. > >When editing a file there is always a small chance to introduce a random > >bug somewhere in the file. In theory we have to re-test a driver > >whenever we change a file which the driver depends on... > > > This might be taking the anti- dvb-pll argument a bit too far... If I > add a new pll description to dvb-pll, the diff will clearly show that no > other descriptions were touched. This should not be a concern. In theory you are right, but Murphy's law applies, too. ;-) Btw, it was not meant to be an anti- dvb-pll argument. > >Small files will only affect those drivers which really need them. > > > >I vote for one header file per tuner. It's both easier to maintain and > >easier to understand for newbies. If you prefer we can move these files > >to a subdirectory to keep them separate from the frontend drivers. > > > >Later these definitions can be converted to use dvb_pll or whatever. > >For now they save us hundreds of lines of duplicated code. > > > >Imho this process should be continued step by step for all duplicated > >tuner descriptions. > > > This sounds good to me. I don't know if we need to actually move them > into a different directory, or if a filename prefix would suffice (see > my response to Andreas below). It seems to me that the decision has > already been made to use a filename prefix -- we should rename / move > those files sooner than later. > > Andreas Oberritter wrote: > > >I like small independent files. > > > Agreed. I retract my comment about consolidating them -- independent > files are most appropriate. > > >Btw. such a change has been proposed by me last summer and Johannes > >suggested using a common prefix like "fe-" although I'd vote for using > >"nim_". > > > Yes, now I remember... I second that vote. I would also prefer the > prefix "nim_". Please choose a prefix which can be understood by someone who is not a hardcore dvb developer. Virtuelly nobody knows what a 'nim' is. :-( > >http://thread.gmane.org/gmane.linux.drivers.dvb/19261/focus=19261 > > > >My old patch is still available but moved to a new URL. > >http://www.saftware.de/patches/frontend.diff > > > My last email was purely inquisitive. Now that you guys have responded, > I'd like to make another point: > > If we are putting these commonly used fuctions into nim-specific header > files, we STILL end up with duplicated binary code. Since all of the > code is now in these small header files, the c code is no longer > duplicated, however, these functions are still being compiled staticly > into each driver that uses them. Correct, see below. > I propose the following: > > We should apply Perceval's patch, but instead of creating "bsrv2.h" , > that we instead call it, "nim_bsrv2.c", and then we should create a > matching "nim_bsrv2.h" header, containing the following prototypes: > > int alps_bsrv2_pll_set(struct dvb_frontend* fe, struct i2c_adapter* i2c, > struct dvb_frontend_parameters* params); > struct ves1x93_config alps_bsrv2_config; > > ... Of course, the same should be applied to bsbe1.h and bsru6.h , and > all other newly-created nim modules. > > This way, instead of the same code being statically compiled into each > driver, we could compile them once as a module, and have that code > reused by the other drivers. > > IMHO, this would be the best and most portable solution, truly resulting > in the removal of such duplicated code in all senses. > > If we can all agree to this, then I will move forward and create such a > module for the lgh06xf. > > Comments? Well, I did it this way because I wanted to make the code easier to maintain while avoiding the module overhead (Kconfig modification and testing, module load/unload, module init/exit, usage counters etc). The size of a tuner description is just a few bytes, and it will be even smaller if it would be converted to use dvb_pll in the future... Imho it is not worth the trouble... CU Oliver -- -------------------------------------------------------- VDR Remote Plugin available at http://www.escape-edv.de/endriss/vdr/ -------------------------------------------------------- _______________________________________________ linux-dvb@xxxxxxxxxxx http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
