On Monday 22 October 2007 22:03:03 mkrufky@xxxxxxxxxxx wrote: > Mauro & others, > > After our conversation last week, I decided to move forward with > tuner-refactor-phase-2, so that you can have the pathway for your > tda9887 & tea5767 changes to go in without clashing with my pending > work. > > My code is now ready for review: > > http://linuxtv.org/hg/~mkrufky/tuner-refactor-phase-2 > > - Move all tda8275/8275a tuning code from tda8290 module into tda827x > module - tda827x: fix GPL export on attach function > - tda8290: add support for NXP TDA18271 tuner and TDA8295 analog > demod - tuner: move analog_tuner_ops into dvb_frontend_ops > - tuner: clear analog_demod_ops on release > - tuner: move analog_demod_priv into struct dvb_frontend > - dvb_frontend: codingstyle cleanups > - tuner: convert analog tuner demod sub-modules to dvb_frontend > interface - tuner: clean up ops checking in tuner_status function > - move std if setting from tda8290 to tda827x > - make tda9887 build selectable via Kconfig > > b/linux/drivers/media/dvb/frontends/tda18271.c | 1062 ++++++++ > b/linux/drivers/media/dvb/frontends/tda18271.h | 40 > b/linux/drivers/media/video/tda9887.h | 33 > linux/Documentation/video4linux/CARDLIST.tuner | 1 > linux/drivers/media/Kconfig | 14 > linux/drivers/media/dvb/dvb-core/dvb_frontend.h | 22 > linux/drivers/media/dvb/frontends/Kconfig | 7 > linux/drivers/media/dvb/frontends/Makefile | 1 > linux/drivers/media/dvb/frontends/tda827x.c | 370 ++ > linux/drivers/media/dvb/frontends/tda827x.h | 13 > linux/drivers/media/video/Makefile | 3 > linux/drivers/media/video/tda8290.c | 1297 ++++------ > linux/drivers/media/video/tda8290.h | 40 > linux/drivers/media/video/tda9887.c | 161 - > linux/drivers/media/video/tuner-core.c | 248 + > linux/drivers/media/video/tuner-driver.h | 25 > linux/drivers/media/video/tuner-types.c | 3 > linux/drivers/media/video/tveeprom.c | 2 > linux/include/media/tuner.h | 2 > v4l/versions.txt | 2 > 20 files changed, 2424 insertions(+), 922 deletions(-) > > The four major changes are: > > 1) move all tda827x tuning code from tda8290.c into tda827x.c > > 2) addition of tda8295 + tda18271 tuner + analog demod combo support. > > 3) conversion of tda9887 to a separate module > > 4) addition of analog_demod_ops & analog_demod_priv to struct > dvb_frontend > > After this is merged, the analog demods will be accessible via the > dvb_frontend interface. For the sake of simplicity, I've kept the > analog_tuner_ops untouched, and using this structure for the > demodulators within the dvb_frontend_ops structures. We can improve > on this in the future, if necessary. > > I've implemented this as a forward reference, so we can make any > changes to the analog_tuner_ops structure as we see fit, without > having any impact on dvb-only users of the dvb_frontend. > > This started off as a private email, but I believe that I've covered > all bases. Since I tend to be lazy with emails in general, I am just > going to cc the mailing lists and consider this an RFC. > > I've taken into account the concerns mentioned in the phase-1 RFC > thread -- I believe that all will be happy with the way that I've > done this. > > Please let me know if you have any questions or comments. > > Cheers, > > Mike Krufky Hi Mike, Looks good! Just some nitpick things: 1) tuner-core.c: things like: - if (t->ops.tuner_status) - t->ops.tuner_status(t); + if ((ops) && (ops->tuner_status)) + ops->tuner_status(t); A bit too many parenthesis, 'if (ops && ops->tuner_status)' works just as well and is more readable IMHO. 2) Please comment who IS responsible for freeing analog_demod_priv; also 'kfree(t->fe.' should be 'kfree(fe->' to keep the comment in sync with the code changes. +static void fe_release(struct dvb_frontend *fe) +{ + if (fe->ops.tuner_ops.release) + fe->ops.tuner_ops.release(fe); + + fe->ops.analog_demod_ops = NULL; + /* DO NOT kfree(t->fe.analog_demod_priv) */ + fe->analog_demod_priv = NULL; +} 3) Personally I don't see the need for changeset 6214 (clean up ops checking in tuner_status function): I thought the original was easier to read. Just remove the '{' '}' checkpatch complains about and it's fine. Reviewed-by: Hans Verkuil <hverkuil@xxxxxxxxx> Regards, Hans _______________________________________________ linux-dvb mailing list linux-dvb@xxxxxxxxxxx http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
