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
_______________________________________________
linux-dvb mailing list
linux-dvb@xxxxxxxxxxx
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
