Am Samstag, 2. Juni 2007 00:31 schrieb Michael Krufky:
> 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
OK, Mike: I am just adjusting your latest approach due to Email issues of
wordwrapping. Could you please be kind enough to supply another patch
including Trent's changes?
Will test them then.
Thank you
Regards
Uwe
P. S.: Could you, Trent, please be kind enough to help pulling your original
stuff into mainline? (i. e. dst customization patchset). Just please add your
SOB again.
Thank you everybody
>
>
> _______________________________________________
> linux-dvb mailing list
> linux-dvb@xxxxxxxxxxx
> http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
_______________________________________________
linux-dvb mailing list
linux-dvb@xxxxxxxxxxx
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
