[linux-dvb] [PATCH] Moving ALPS BSRV2 tuner handling code to separate file.

[Oliver Endriss]

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/
--------------------------------------------------------