[linux-dvb] [RFC] remove static dependencies on dvb-pll

Michael Krufky mkrufky at linuxtv.org
Sat Jun 2 00:31:02 CEST 2007


Trent Piepho wrote:
> On Fri, 1 Jun 2007, Michael Krufky wrote:
>   
>> I haven't tested this yet, but it should work just fine...
>>
>> I will test it when I get home.
>>
>>
>> This patch removes all static dependencies on the dvb-pll module, without _any_
>> harmful side effects.
>>
>> Any comments?
>>     
>
> I think the handling inside dvb-pll might be better a little differently.
>
> Keep the pll_desc inside dvb_pll_priv as a pointer, instead of making it the
> pll id number.  Look up the pointer from the id once in dvb_pll_attach(), so
> it doesn't have to be done for each time a pll function is called.
>
> Instead of making the pll ID an int and using #defines, I'd make the id an
> enum.  You don't have to manually assign the id numbers that way and it's
> clear what supposed to go there.
>
> Instead of using a big switch statement to turn the pll id into a description
> pointer, I'd use an array:
>
> dvb-pll.h:
> enum dvb_pll_id = {
> 	DVB_PLL_THOMSON_DTT7579,
> 	...
> 	DVB_PLL_LAST
> };
>
> dvb-pll.c:
> static struct dvb_pll_desc* pll_list[] = {
> 	[DVB_PLL_THOMSON_DTT7579] = &dvb_pll_thomson_dtt7579,
> 	...  // order doesn't actually have to match the enum
> };
>
> struct dvb_frontend *dvb_pll_attach(..., enum dvb_pll_id id)
> {
>         if (id >= DVB_PLL_LAST) { some error? BUG()? };
> 	priv->pll_desc = pll_list[id];
> }
>   
Thank you, Trent.

I agree with your suggestions, and the array method would probably be a
bit quicker.  I had intended on creating an array as you have shown
above, but used the switch..case only for this proof of concept patch. 
I will convert the switch..case to the array, though, as I do believe it
will be more efficient.  As far as storing the dvb_pll_desc pointer
inside the dvb_pll_priv struct, that is what I wanted to do at first,
but I could not, because there are still external callers of
dvb_pll_configure.

Once we eliminate that last caller of dvb_pll_configure (and unexport
dvb_pll_configure), then we can store the pointer instead of the integer
/ enum.

Thanks for the review.

Cheers,

Mike




More information about the linux-dvb mailing list