On Monday 22 October 2007 22:03:03 [EMAIL PROTECTED] 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 <[EMAIL PROTECTED]>
Regards,
Hans
_______________________________________________
linux-dvb mailing list
linux-dvb@linuxtv.org
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linux-dvb
