Trent, Trent Piepho wrote: > On Tue, 12 Jun 2007, Michael Krufky wrote: > >> I have finalized the 'removal of static dependencies on the dvb-pll >> module' changesets, but had to change some things inside the dvb-pll >> framework in the process. See my "dvb-pll" tree for the topmost changesets: >> >> http://linuxtv.org/hg/~mkrufky/dvb-pll >> >> > Looks good. I'd use a enum for the tuner id, but that's not a big deal. > Thank you for the review. I'd like to stick with the integer #define, for debugging purposes.... I plan to add a patch in the future, that will allow forcing a particular dvb_pll_desc_id as a module option, for the sake of testing new card variations without the need for writing code / recompiling..... This will be especially handy for cases where a vendor may release a card using a different tuner, while not changing the devices subsystem id, as we have seen numerous times before. When I do this, I intend to include the text "(for debugging purposes only)" , as users should not use this regularly -- it is only to handle debugging / testing for the above situation. > There is one thing that should be changed: > > +unsigned const int pll_count = ARRAY_SIZE(pll_list); > > This should at least be made static, otherwise there really will be a > global variable in memory with pll_count in it. Some other source file > could use it, so gcc can't delete it. I'd probably make it a macro, or > just change: > I meant to mark this static -- my mistake. However, instead of marking it static, I will apply the change that you've suggested below -- this is much cleaner, IMHO. > + BUG_ON(pll_desc_id < 1 || pll_desc_id > pll_count); > to > + BUG_ON(pll_desc_id < 1 || pll_desc_id > ARRAY_SIZE(pll_list)); > > Also, is there a reason for DVB_PLL_UNDEFINED? It doesn't seem like there > any way to make use of it. > I don't know if it is really needed, but I just wanted to reserve the '0' as a default, uninitialized setting. I did this for the sake of future-compatible cleanliness....... We have an undefined setting for tuner.ko, and all the v4lfoo-cards.c arrays, I only thought it natural to do the same here....... Should I remove it? I think it's safe to leave as-is, but if it's really just a waste of space then it can go....... Anyhow, I altered the topmost changeset as per your suggestion, above, and pushed it here: http://linuxtv.org/hg/~mkrufky/dvb-pll I'd like to include your S-O-B on the changeset before requesting the pull to Mauro. If you're OK with the changeset as-is, please respond to this email with your sign-off. Thanks again for the review. Regards, Mike _______________________________________________ linux-dvb mailing list linux-dvb@xxxxxxxxxxx http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
