Re: [RFC] tuner-refactor-phase-2

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

[Index of Archives]     [Linux Media]     [Video 4 Linux]     [Asterisk]     [Samba]     [Xorg]     [Xfree86]     [Linux USB]

  Powered by Linux