* [PATCH 1/4] tda18271_set_analog_params major bugfix
@ 2009-09-22 21:05 spam
2009-09-24 18:46 ` Michael Krufky
0 siblings, 1 reply; 7+ messages in thread
From: spam @ 2009-09-22 21:05 UTC (permalink / raw
To: linux-media; +Cc: mkrufky
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);
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/4] tda18271_set_analog_params major bugfix
2009-09-22 21:05 [PATCH 1/4] tda18271_set_analog_params major bugfix spam
@ 2009-09-24 18:46 ` Michael Krufky
2009-09-24 21:42 ` spam
0 siblings, 1 reply; 7+ messages in thread
From: Michael Krufky @ 2009-09-24 18:46 UTC (permalink / raw
To: Henk.Vergonet; +Cc: linux-media
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/4] tda18271_set_analog_params major bugfix
2009-09-24 18:46 ` Michael Krufky
@ 2009-09-24 21:42 ` spam
2009-09-27 16:25 ` Michael Krufky
0 siblings, 1 reply; 7+ messages in thread
From: spam @ 2009-09-24 21:42 UTC (permalink / raw
To: Michael Krufky; +Cc: linux-media
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/4] tda18271_set_analog_params major bugfix
2009-09-24 21:42 ` spam
@ 2009-09-27 16:25 ` Michael Krufky
2009-09-27 16:35 ` Michael Krufky
0 siblings, 1 reply; 7+ messages in thread
From: Michael Krufky @ 2009-09-27 16:25 UTC (permalink / raw
To: Henk.Vergonet; +Cc: linux-media
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/4] tda18271_set_analog_params major bugfix
2009-09-27 16:25 ` Michael Krufky
@ 2009-09-27 16:35 ` Michael Krufky
2009-09-27 20:24 ` spam
0 siblings, 1 reply; 7+ messages in thread
From: Michael Krufky @ 2009-09-27 16:35 UTC (permalink / raw
To: Henk.Vergonet; +Cc: linux-media
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/4] tda18271_set_analog_params major bugfix
2009-09-27 16:35 ` Michael Krufky
@ 2009-09-27 20:24 ` spam
2009-09-27 20:37 ` Michael Krufky
0 siblings, 1 reply; 7+ messages in thread
From: spam @ 2009-09-27 20:24 UTC (permalink / raw
To: Michael Krufky; +Cc: linux-media
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/4] tda18271_set_analog_params major bugfix
2009-09-27 20:24 ` spam
@ 2009-09-27 20:37 ` Michael Krufky
0 siblings, 0 replies; 7+ messages in thread
From: Michael Krufky @ 2009-09-27 20:37 UTC (permalink / raw
To: Henk.Vergonet; +Cc: linux-media
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2009-09-27 20:37 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-09-22 21:05 [PATCH 1/4] tda18271_set_analog_params major bugfix spam
2009-09-24 18:46 ` Michael Krufky
2009-09-24 21:42 ` spam
2009-09-27 16:25 ` Michael Krufky
2009-09-27 16:35 ` Michael Krufky
2009-09-27 20:24 ` spam
2009-09-27 20:37 ` Michael Krufky
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.