| Message ID | 20090922210500.GA8661@systol-ng.god.lan (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, Sep 22, 2009 at 5:05 PM, <spam@systol-ng.god.lan> wrote: > > Multiplication by 62500 causes an overflow in the 32 bits "freq" register when > using radio. FM radio reception on a Zolid Hybrid PCI is now working. Other > tda18271 configurations may also benefit from this change ;) > > Signed-off-by: Henk.Vergonet@gmail.com > > diff -r 29e4ba1a09bc linux/drivers/media/common/tuners/tda18271-fe.c > --- a/linux/drivers/media/common/tuners/tda18271-fe.c  Sat Sep 19 09:45:22 2009 -0300 > +++ b/linux/drivers/media/common/tuners/tda18271-fe.c  Tue Sep 22 22:06:31 2009 +0200 > @@ -1001,38 +1020,43 @@ >     struct tda18271_std_map_item *map; >     char *mode; >     int ret; > -    u32 freq = params->frequency * 62500; > +    u32 freq; > >     priv->mode = TDA18271_ANALOG; > >     if (params->mode == V4L2_TUNER_RADIO) { > -        freq = freq / 1000; > +        freq = params->frequency * 625; > +        freq = freq / 10; >         map = &std_map->fm_radio; >         mode = "fm"; > -    } else if (params->std & V4L2_STD_MN) { > -        map = &std_map->atv_mn; > -        mode = "MN"; > -    } else if (params->std & V4L2_STD_B) { > -        map = &std_map->atv_b; > -        mode = "B"; > -    } else if (params->std & V4L2_STD_GH) { > -        map = &std_map->atv_gh; > -        mode = "GH"; > -    } else if (params->std & V4L2_STD_PAL_I) { > -        map = &std_map->atv_i; > -        mode = "I"; > -    } else if (params->std & V4L2_STD_DK) { > -        map = &std_map->atv_dk; > -        mode = "DK"; > -    } else if (params->std & V4L2_STD_SECAM_L) { > -        map = &std_map->atv_l; > -        mode = "L"; > -    } else if (params->std & V4L2_STD_SECAM_LC) { > -        map = &std_map->atv_lc; > -        mode = "L'"; >     } else { > -        map = &std_map->atv_i; > -        mode = "xx"; > +        freq = params->frequency * 62500; > + > +        if (params->std & V4L2_STD_MN) { > +            map = &std_map->atv_mn; > +            mode = "MN"; > +        } else if (params->std & V4L2_STD_B) { > +            map = &std_map->atv_b; > +            mode = "B"; > +        } else if (params->std & V4L2_STD_GH) { > +            map = &std_map->atv_gh; > +            mode = "GH"; > +        } else if (params->std & V4L2_STD_PAL_I) { > +            map = &std_map->atv_i; > +            mode = "I"; > +        } else if (params->std & V4L2_STD_DK) { > +            map = &std_map->atv_dk; > +            mode = "DK"; > +        } else if (params->std & V4L2_STD_SECAM_L) { > +            map = &std_map->atv_l; > +            mode = "L"; > +        } else if (params->std & V4L2_STD_SECAM_LC) { > +            map = &std_map->atv_lc; > +            mode = "L'"; > +        } else { > +            map = &std_map->atv_i; > +            mode = "xx"; > +        } >     } > >     tda_dbg("setting tda18271 to system %s\n", mode); > Nice catch, Henk -- Thank you for this fix... I will have this one merged as soon as I can. Signed-off-by: Michael Krufky <mkrufky@kernellabs.com> Mauro, please do not merge the tda18271 / tda829x patches until I send you a pull request -- there is a patch-order dependency with other pending changes and I would prefer to send this to you in the proper order. Thanks, Mike Krufky -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Sep 24, 2009 at 02:46:06PM -0400, Michael Krufky wrote: > On Tue, Sep 22, 2009 at 5:05 PM, <spam@systol-ng.god.lan> wrote: > > > > Multiplication by 62500 causes an overflow in the 32 bits "freq" register when > > using radio. FM radio reception on a Zolid Hybrid PCI is now working. Other > > tda18271 configurations may also benefit from this change ;) > > > > Signed-off-by: Henk.Vergonet@gmail.com > > > > diff -r 29e4ba1a09bc linux/drivers/media/common/tuners/tda18271-fe.c ... > > - freq = freq / 1000; > > + freq = params->frequency * 625; > > + freq = freq / 10; Hmm now that I review my own patch: - freq = freq / 1000; + freq = params->frequency * 125; + freq = freq / 2; might even be better... regards henk -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Sep 24, 2009 at 5:42 PM, <spam@systol-ng.god.lan> wrote: > On Thu, Sep 24, 2009 at 02:46:06PM -0400, Michael Krufky wrote: >> On Tue, Sep 22, 2009 at 5:05 PM, Â <spam@systol-ng.god.lan> wrote: >> > >> > Multiplication by 62500 causes an overflow in the 32 bits "freq" register when >> > using radio. FM radio reception on a Zolid Hybrid PCI is now working. Other >> > tda18271 configurations may also benefit from this change ;) >> > >> > Signed-off-by: Henk.Vergonet@gmail.com >> > >> > diff -r 29e4ba1a09bc linux/drivers/media/common/tuners/tda18271-fe.c > ... >> > - Â Â Â Â Â Â Â freq = freq / 1000; >> > + Â Â Â Â Â Â Â freq = params->frequency * 625; >> > + Â Â Â Â Â Â Â freq = freq / 10; > > Hmm now that I review my own patch: > > - Â Â Â Â Â Â Â freq = freq / 1000; > + Â Â Â Â Â Â Â freq = params->frequency * 125; > + Â Â Â Â Â Â Â freq = freq / 2; > > might even be better... Henk, That certainly is better, but I am going to go with an even simpler & cleaner approach: diff -r f52640ced9e8 linux/drivers/media/common/tuners/tda18271-fe.c --- a/linux/drivers/media/common/tuners/tda18271-fe.c Tue Sep 15 01:25:35 2009 -0400 +++ b/linux/drivers/media/common/tuners/tda18271-fe.c Sun Sep 27 12:21:37 2009 -0400 @@ -1001,12 +1001,12 @@ struct tda18271_std_map_item *map; char *mode; int ret; - u32 freq = params->frequency * 62500; + u32 freq = params->frequency * + ((params->mode == V4L2_TUNER_RADIO) ? 125 / 2 : 62500); priv->mode = TDA18271_ANALOG; if (params->mode == V4L2_TUNER_RADIO) { - freq = freq / 1000; map = &std_map->fm_radio; mode = "fm"; } else if (params->std & V4L2_STD_MN) { You still get the credit for spotting the problem & providing the original fix -- thanks again! I'm going to push this along with the others today. Cheers, Mike -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Sep 27, 2009 at 12:25 PM, Michael Krufky <mkrufky@kernellabs.com> wrote: > On Thu, Sep 24, 2009 at 5:42 PM,  <spam@systol-ng.god.lan> wrote: >> On Thu, Sep 24, 2009 at 02:46:06PM -0400, Michael Krufky wrote: >>> On Tue, Sep 22, 2009 at 5:05 PM,  <spam@systol-ng.god.lan> wrote: >>> > >>> > Multiplication by 62500 causes an overflow in the 32 bits "freq" register when >>> > using radio. FM radio reception on a Zolid Hybrid PCI is now working. Other >>> > tda18271 configurations may also benefit from this change ;) >>> > >>> > Signed-off-by: Henk.Vergonet@gmail.com >>> > >>> > diff -r 29e4ba1a09bc linux/drivers/media/common/tuners/tda18271-fe.c >> ... >>> > -        freq = freq / 1000; >>> > +        freq = params->frequency * 625; >>> > +        freq = freq / 10; >> >> Hmm now that I review my own patch: >> >> -        freq = freq / 1000; >> +        freq = params->frequency * 125; >> +        freq = freq / 2; >> >> might even be better... > > Henk, > > That certainly is better, but I am going to go with an even simpler & > cleaner approach: > > diff -r f52640ced9e8 linux/drivers/media/common/tuners/tda18271-fe.c > --- a/linux/drivers/media/common/tuners/tda18271-fe.c  Tue Sep 15 > 01:25:35 2009 -0400 > +++ b/linux/drivers/media/common/tuners/tda18271-fe.c  Sun Sep 27 > 12:21:37 2009 -0400 > @@ -1001,12 +1001,12 @@ >     struct tda18271_std_map_item *map; >     char *mode; >     int ret; > -    u32 freq = params->frequency * 62500; > +    u32 freq = params->frequency * > +        ((params->mode == V4L2_TUNER_RADIO) ? 125 / 2 : 62500); > >     priv->mode = TDA18271_ANALOG; > >     if (params->mode == V4L2_TUNER_RADIO) { > -        freq = freq / 1000; >         map = &std_map->fm_radio; >         mode = "fm"; >     } else if (params->std & V4L2_STD_MN) { > > > You still get the credit for spotting the problem & providing the > original fix -- thanks again!  I'm going to push this along with the > others today. On a second thought, I see that my above patch loses some precision ... this is even better: diff -r f52640ced9e8 linux/drivers/media/common/tuners/tda18271-fe.c --- a/linux/drivers/media/common/tuners/tda18271-fe.c Tue Sep 15 01:25:35 2009 -0400 +++ b/linux/drivers/media/common/tuners/tda18271-fe.c Sun Sep 27 12:33:20 2009 -0400 @@ -1001,12 +1001,12 @@ struct tda18271_std_map_item *map; char *mode; int ret; - u32 freq = params->frequency * 62500; + u32 freq = params->frequency * 125 * + ((params->mode == V4L2_TUNER_RADIO) ? 1 : 1000) / 2; priv->mode = TDA18271_ANALOG; if (params->mode == V4L2_TUNER_RADIO) { - freq = freq / 1000; map = &std_map->fm_radio; mode = "fm"; } else if (params->std & V4L2_STD_MN) { Cheers, Mike -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Sep 27, 2009 at 12:35:00PM -0400, Michael Krufky wrote: > On Sun, Sep 27, 2009 at 12:25 PM, Michael Krufky <mkrufky@kernellabs.com> wrote: > > On a second thought, I see that my above patch loses some precision > ... this is even better: > > diff -r f52640ced9e8 linux/drivers/media/common/tuners/tda18271-fe.c > --- a/linux/drivers/media/common/tuners/tda18271-fe.c Tue Sep 15 > 01:25:35 2009 -0400 > +++ b/linux/drivers/media/common/tuners/tda18271-fe.c Sun Sep 27 > 12:33:20 2009 -0400 > @@ -1001,12 +1001,12 @@ > struct tda18271_std_map_item *map; > char *mode; > int ret; > - u32 freq = params->frequency * 62500; > + u32 freq = params->frequency * 125 * > + ((params->mode == V4L2_TUNER_RADIO) ? 1 : 1000) / 2; > > priv->mode = TDA18271_ANALOG; > > if (params->mode == V4L2_TUNER_RADIO) { > - freq = freq / 1000; > map = &std_map->fm_radio; > mode = "fm"; > } else if (params->std & V4L2_STD_MN) { > > Cheers, > > Mike Much better! Btw. It seems that the tuner is capable of tuning in 1000 Hz steps, is there a reason why we are using 62500 Hz steps? Regards, Henk -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Sep 27, 2009 at 4:24 PM, <spam@systol-ng.god.lan> wrote: > On Sun, Sep 27, 2009 at 12:35:00PM -0400, Michael Krufky wrote: >> On Sun, Sep 27, 2009 at 12:25 PM, Michael Krufky <mkrufky@kernellabs.com> wrote: >> >> On a second thought, I see that my above patch loses some precision >> ... Â this is even better: >> >> diff -r f52640ced9e8 linux/drivers/media/common/tuners/tda18271-fe.c >> --- a/linux/drivers/media/common/tuners/tda18271-fe.c Tue Sep 15 >> 01:25:35 2009 -0400 >> +++ b/linux/drivers/media/common/tuners/tda18271-fe.c Sun Sep 27 >> 12:33:20 2009 -0400 >> @@ -1001,12 +1001,12 @@ >> Â Â Â struct tda18271_std_map_item *map; >> Â Â Â char *mode; >> Â Â Â int ret; >> - Â Â u32 freq = params->frequency * 62500; >> + Â Â u32 freq = params->frequency * 125 * >> + Â Â Â Â Â Â ((params->mode == V4L2_TUNER_RADIO) ? 1 : 1000) / 2; >> >> Â Â Â priv->mode = TDA18271_ANALOG; >> >> Â Â Â if (params->mode == V4L2_TUNER_RADIO) { >> - Â Â Â Â Â Â freq = freq / 1000; >> Â Â Â Â Â Â Â map = &std_map->fm_radio; >> Â Â Â Â Â Â Â mode = "fm"; >> Â Â Â } else if (params->std & V4L2_STD_MN) { >> >> Cheers, >> >> Mike > > Much better! > > Btw. It seems that the tuner is capable of tuning in 1000 Hz steps, is > there a reason why we are using 62500 Hz steps? That's the v4l2 analog tuner API. It has nothing to do with the tda18271 driver internals. If you look on the digital set_params function, you'll see that this doesnt happen there. -Mike -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff -r 29e4ba1a09bc linux/drivers/media/common/tuners/tda18271-fe.c --- a/linux/drivers/media/common/tuners/tda18271-fe.c Sat Sep 19 09:45:22 2009 -0300 +++ b/linux/drivers/media/common/tuners/tda18271-fe.c Tue Sep 22 22:06:31 2009 +0200 @@ -1001,38 +1020,43 @@ struct tda18271_std_map_item *map; char *mode; int ret; - u32 freq = params->frequency * 62500; + u32 freq; priv->mode = TDA18271_ANALOG; if (params->mode == V4L2_TUNER_RADIO) { - freq = freq / 1000; + freq = params->frequency * 625; + freq = freq / 10; map = &std_map->fm_radio; mode = "fm"; - } else if (params->std & V4L2_STD_MN) { - map = &std_map->atv_mn; - mode = "MN"; - } else if (params->std & V4L2_STD_B) { - map = &std_map->atv_b; - mode = "B"; - } else if (params->std & V4L2_STD_GH) { - map = &std_map->atv_gh; - mode = "GH"; - } else if (params->std & V4L2_STD_PAL_I) { - map = &std_map->atv_i; - mode = "I"; - } else if (params->std & V4L2_STD_DK) { - map = &std_map->atv_dk; - mode = "DK"; - } else if (params->std & V4L2_STD_SECAM_L) { - map = &std_map->atv_l; - mode = "L"; - } else if (params->std & V4L2_STD_SECAM_LC) { - map = &std_map->atv_lc; - mode = "L'"; } else { - map = &std_map->atv_i; - mode = "xx"; + freq = params->frequency * 62500; + + if (params->std & V4L2_STD_MN) { + map = &std_map->atv_mn; + mode = "MN"; + } else if (params->std & V4L2_STD_B) { + map = &std_map->atv_b; + mode = "B"; + } else if (params->std & V4L2_STD_GH) { + map = &std_map->atv_gh; + mode = "GH"; + } else if (params->std & V4L2_STD_PAL_I) { + map = &std_map->atv_i; + mode = "I"; + } else if (params->std & V4L2_STD_DK) { + map = &std_map->atv_dk; + mode = "DK"; + } else if (params->std & V4L2_STD_SECAM_L) { + map = &std_map->atv_l; + mode = "L"; + } else if (params->std & V4L2_STD_SECAM_LC) { + map = &std_map->atv_lc; + mode = "L'"; + } else { + map = &std_map->atv_i; + mode = "xx"; + } } tda_dbg("setting tda18271 to system %s\n", mode);
Multiplication by 62500 causes an overflow in the 32 bits "freq" register when using radio. FM radio reception on a Zolid Hybrid PCI is now working. Other tda18271 configurations may also benefit from this change ;) Signed-off-by: Henk.Vergonet@gmail.com -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html