Re: [PATCH 13/15] - xc2028 bugfix for firmware 3.6 -> Zarlink use without shift in DTV8 or DTV78

2010-02-03 Thread Devin Heitmueller
On Wed, Feb 3, 2010 at 3:36 PM, Stefan Ringel  wrote:
> signed-off-by: Stefan Ringel 
> --- a/drivers/media/common/tuners/tuner-xc2028.c
> +++ b/drivers/media/common/tuners/tuner-xc2028.c
> @@ -1114,7 +1122,11 @@ static int xc2028_set_params(struct dvb_frontend *fe,
>
>     /* All S-code tables need a 200kHz shift */
>     if (priv->ctrl.demod) {
> -        demod = priv->ctrl.demod + 200;
> +        if ((priv->ctrl.fname == "xc3028L-v36.fw") && (priv->ctrl.demod
> == XC3028_FE_ZARLINK456) && ((type & DTV78) | (type & DTV8)) ) {
> +            demod = priv->ctrl.demod;
> +        } else {
> +            demod = priv->ctrl.demod + 200;
> +        }
>         /*
>          * The DTV7 S-code table needs a 700 kHz shift.
>          * Thanks to Terry Wu  for reporting this
> @@ -1123,8 +1135,8 @@ static int xc2028_set_params(struct dvb_frontend *fe,
>          * use this firmware after initialization, but a tune to a UHF
>          * channel should then cause DTV78 to be used.
>          */
> -        if (type & DTV7)
> -            demod += 500;
> +        if (type & DTV7)
> +            demod += 500;
>     }

Independent of the validity of this patch, you should not be
submitting patches that have a mix of whitespace changes and actual
changes.  In the above case (the if type & DTV7 part), it looks like
these shouldn't have been included at all since it makes no functional
change.

It sounds like a nit-pick, but the reality is that its inclusion had
me staring at it for 30 seconds trying to figure out whether there was
an *actual* difference there or if it was purely whitespace.

>
>     return generic_set_freq(fe, p->frequency,
> @@ -1240,6 +1252,10 @@ static const struct dvb_tuner_ops
> xc2028_dvb_tuner_ops = {
>     .get_rf_strength   = xc2028_signal,
>     .set_params        = xc2028_set_params,
>     .sleep             = xc2028_sleep,
> +#if 0
> +    int (*get_bandwidth)(struct dvb_frontend *fe, u32 *bandwidth);
> +    int (*get_status)(struct dvb_frontend *fe, u32 *status);
> +#endif
>  };

Likewise, you should not be including unrelated changes in patches -
the above "#if 0" section not only is never compiled in to the code
(presumably it is debug code), but it has nothing to do with the fix
this patch is claiming to address.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 13/15] - xc2028 bugfix for firmware 3.6 -> Zarlink use without shift in DTV8 or DTV78

2010-02-03 Thread Stefan Ringel
signed-off-by: Stefan Ringel 
--- a/drivers/media/common/tuners/tuner-xc2028.c
+++ b/drivers/media/common/tuners/tuner-xc2028.c
@@ -1114,7 +1122,11 @@ static int xc2028_set_params(struct dvb_frontend *fe,
 
 /* All S-code tables need a 200kHz shift */
 if (priv->ctrl.demod) {
-demod = priv->ctrl.demod + 200;
+if ((priv->ctrl.fname == "xc3028L-v36.fw") && (priv->ctrl.demod
== XC3028_FE_ZARLINK456) && ((type & DTV78) | (type & DTV8)) ) {
+demod = priv->ctrl.demod;
+} else {
+demod = priv->ctrl.demod + 200;
+}
 /*
  * The DTV7 S-code table needs a 700 kHz shift.
  * Thanks to Terry Wu  for reporting this
@@ -1123,8 +1135,8 @@ static int xc2028_set_params(struct dvb_frontend *fe,
  * use this firmware after initialization, but a tune to a UHF
  * channel should then cause DTV78 to be used.
  */
-if (type & DTV7)
-demod += 500;
+if (type & DTV7)
+demod += 500;
 }
 
 return generic_set_freq(fe, p->frequency,
@@ -1240,6 +1252,10 @@ static const struct dvb_tuner_ops
xc2028_dvb_tuner_ops = {
 .get_rf_strength   = xc2028_signal,
 .set_params= xc2028_set_params,
 .sleep = xc2028_sleep,
+#if 0
+int (*get_bandwidth)(struct dvb_frontend *fe, u32 *bandwidth);
+int (*get_status)(struct dvb_frontend *fe, u32 *status);
+#endif
 };
 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html