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

[Michael Krufky]

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.

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

>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_".

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

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?

Cheers,

Michael Krufky