[PATCH 0/2] fix tunning for r820t tunner

[PATCH 0/2] fix tunning for r820t tunner

[Jiří Pinkava]

Fixes range check for VCO parameters, simplifies calculation of divisor.

 drivers/media/tuners/r820t.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

[PATCH 1/2] [media] r820t: fix nint range check

[Jiří Pinkava]

Use full range of VCO parameters, fixes tunning for some frequencies.
---
 drivers/media/tuners/r820t.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/tuners/r820t.c b/drivers/media/tuners/r820t.c
index 1c23666..e25c720 100644
--- a/drivers/media/tuners/r820t.c
+++ b/drivers/media/tuners/r820t.c
@@ -637,7 +637,7 @@ static int r820t_set_pll(struct r820t_priv *priv,
enum v4l2_tuner_type type,
                vco_fra = pll_ref * 129 / 128;
        }
 
-       if (nint > 63) {
+       if (nint > 76) {
                tuner_info("No valid PLL values for %u kHz!\n", freq);
                return -EINVAL;
        }
-- 
1.8.3.2

Subject: [PATCH 2/2] [media] r820t: simplify divisor calculation

[Jiří Pinkava]

---
 drivers/media/tuners/r820t.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/media/tuners/r820t.c b/drivers/media/tuners/r820t.c
index e25c720..36dc63e 100644
--- a/drivers/media/tuners/r820t.c
+++ b/drivers/media/tuners/r820t.c
@@ -596,13 +596,9 @@ static int r820t_set_pll(struct r820t_priv *priv,
enum v4l2_tuner_type type,
        while (mix_div <= 64) {
                if (((freq * mix_div) >= vco_min) &&
                   ((freq * mix_div) < vco_max)) {
-                       div_buf = mix_div;
-                       while (div_buf > 2) {
-                               div_buf = div_buf >> 1;
-                               div_num++;
-                       }
                        break;
                }
+               ++div_num;
                mix_div = mix_div << 1;
        }
 
-- 
1.8.3.2

Re: Subject: [PATCH 2/2] [media] r820t: simplify divisor calculation

[Michael Krufky]

Jiří,

It's difficult to understand a patch like this, let alone merging it,
without any kind of explanation.  At a quick glance, the patch looks
wrong.  If you believe the patch is correct, then please resubmit with
some sort of description and explanation for the change.

Best regards,

Mike Krufky

On Sun, Sep 29, 2013 at 6:46 AM, Jiří Pinkava <j-pi@seznam.cz> wrote:
>
> ---
>  drivers/media/tuners/r820t.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/media/tuners/r820t.c b/drivers/media/tuners/r820t.c
> index e25c720..36dc63e 100644
> --- a/drivers/media/tuners/r820t.c
> +++ b/drivers/media/tuners/r820t.c
> @@ -596,13 +596,9 @@ static int r820t_set_pll(struct r820t_priv *priv,
> enum v4l2_tuner_type type,
>         while (mix_div <= 64) {
>                 if (((freq * mix_div) >= vco_min) &&
>                    ((freq * mix_div) < vco_max)) {
> -                       div_buf = mix_div;
> -                       while (div_buf > 2) {
> -                               div_buf = div_buf >> 1;
> -                               div_num++;
> -                       }
>                         break;
>                 }
> +               ++div_num;
>                 mix_div = mix_div << 1;
>         }
>
> --
> 1.8.3.2
>
>

Re: [PATCH 1/2] [media] r820t: fix nint range check

[Michael Krufky]

Jiří,

Do you have any documentation that supports this value change?
Changing this value affects the algorithm, and we'd be happier making
this change if the patch included some better description and perhaps
a reference explaining why the new value is correct.

Regards,

Mike Krufky

On Sun, Sep 29, 2013 at 6:45 AM, Jiří Pinkava <j-pi@seznam.cz> wrote:
>
>
> Use full range of VCO parameters, fixes tunning for some frequencies.
> ---
>  drivers/media/tuners/r820t.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/tuners/r820t.c b/drivers/media/tuners/r820t.c
> index 1c23666..e25c720 100644
> --- a/drivers/media/tuners/r820t.c
> +++ b/drivers/media/tuners/r820t.c
> @@ -637,7 +637,7 @@ static int r820t_set_pll(struct r820t_priv *priv,
> enum v4l2_tuner_type type,
>                 vco_fra = pll_ref * 129 / 128;
>         }
>
> -       if (nint > 63) {
> +       if (nint > 76) {
>                 tuner_info("No valid PLL values for %u kHz!\n", freq);
>                 return -EINVAL;
>         }
> --
> 1.8.3.2
>
>

Re: [PATCH 1/2] [media] r820t: fix nint range check

[Jiří Pinkava]

Mike,

unfortunately no documentation can be referenced except preliminary
version of
datasheet (1).This change is based on lucky guess and supported by lot of
testing on real hardware.

This change add support for devices with Xtal frequency bellow 28.8MHz.

>From Nint  are computed values of Ni and Si. For 28.8MHz crystal can
reach up to 12 / 3 (Ni / Si). Tuner supports crystals with frequencies
(1) 12, 16, 20, 20.48, 24, 27, 28.8, 32 MHz, but this kind of device is
rare to found.
Allowing Ni to go up to 15 instead of only 12 should be safe and for 15
/ 3 (Ni / Si)
we can compute limit for Nint = max(Ni) * 4 + max(Si) + 13 = 76.

If This is sufficient and acceptable explanation I can add some sort of
documentation into patch and resend it (both patches, I can prove I'm
right :)

(1)
http://rtl-sdr.com/wp-content/uploads/2013/04/R820T_datasheet-Non_R-20111130_unlocked.pdf

> Jiří,
>
> Do you have any documentation that supports this value change?
> Changing this value affects the algorithm, and we'd be happier making
> this change if the patch included some better description and perhaps
> a reference explaining why the new value is correct.
>
> Regards,
>
> Mike Krufky
>
> On Sun, Sep 29, 2013 at 6:45 AM, Jiří Pinkava <j-pi@seznam.cz> wrote:
>>
>> Use full range of VCO parameters, fixes tunning for some frequencies.
>> ---
>>  drivers/media/tuners/r820t.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/tuners/r820t.c b/drivers/media/tuners/r820t.c
>> index 1c23666..e25c720 100644
>> --- a/drivers/media/tuners/r820t.c
>> +++ b/drivers/media/tuners/r820t.c
>> @@ -637,7 +637,7 @@ static int r820t_set_pll(struct r820t_priv *priv,
>> enum v4l2_tuner_type type,
>>                 vco_fra = pll_ref * 129 / 128;
>>         }
>>
>> -       if (nint > 63) {
>> +       if (nint > 76) {
>>                 tuner_info("No valid PLL values for %u kHz!\n", freq);
>>                 return -EINVAL;
>>         }
>> --
>> 1.8.3.2
>>
>>

Re: [PATCH 1/2] [media] r820t: fix nint range check

[Michael Krufky]

Do you have any comments on this, Mauro?

Assuming that Mauro is OK with this change, (since he is the author of
this driver) then yes - please resubmit the patch with some
explanation within comments inline or within the commit message.

Best regards,

Mike Krufky

On Mon, Sep 30, 2013 at 12:42 PM, Jiří Pinkava <j-pi@seznam.cz> wrote:
> Mike,
>
> unfortunately no documentation can be referenced except preliminary
> version of
> datasheet (1).This change is based on lucky guess and supported by lot of
> testing on real hardware.
>
> This change add support for devices with Xtal frequency bellow 28.8MHz.
>
> From Nint  are computed values of Ni and Si. For 28.8MHz crystal can
> reach up to 12 / 3 (Ni / Si). Tuner supports crystals with frequencies
> (1) 12, 16, 20, 20.48, 24, 27, 28.8, 32 MHz, but this kind of device is
> rare to found.
> Allowing Ni to go up to 15 instead of only 12 should be safe and for 15
> / 3 (Ni / Si)
> we can compute limit for Nint = max(Ni) * 4 + max(Si) + 13 = 76.
>
> If This is sufficient and acceptable explanation I can add some sort of
> documentation into patch and resend it (both patches, I can prove I'm
> right :)
>
> (1)
> http://rtl-sdr.com/wp-content/uploads/2013/04/R820T_datasheet-Non_R-20111130_unlocked.pdf
>
>> Jiří,
>>
>> Do you have any documentation that supports this value change?
>> Changing this value affects the algorithm, and we'd be happier making
>> this change if the patch included some better description and perhaps
>> a reference explaining why the new value is correct.
>>
>> Regards,
>>
>> Mike Krufky
>>
>> On Sun, Sep 29, 2013 at 6:45 AM, Jiří Pinkava <j-pi@seznam.cz> wrote:
>>>
>>> Use full range of VCO parameters, fixes tunning for some frequencies.
>>> ---
>>>  drivers/media/tuners/r820t.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/tuners/r820t.c b/drivers/media/tuners/r820t.c
>>> index 1c23666..e25c720 100644
>>> --- a/drivers/media/tuners/r820t.c
>>> +++ b/drivers/media/tuners/r820t.c
>>> @@ -637,7 +637,7 @@ static int r820t_set_pll(struct r820t_priv *priv,
>>> enum v4l2_tuner_type type,
>>>                 vco_fra = pll_ref * 129 / 128;
>>>         }
>>>
>>> -       if (nint > 63) {
>>> +       if (nint > 76) {
>>>                 tuner_info("No valid PLL values for %u kHz!\n", freq);
>>>                 return -EINVAL;
>>>         }
>>> --
>>> 1.8.3.2
>>>
>>>
>

Re: [PATCH 1/2] [media] r820t: fix nint range check

[Mauro Carvalho Chehab]

Em Mon, 30 Sep 2013 12:46:52 -0400
Michael Krufky <mkrufky@linuxtv.org> escreveu:

> Do you have any comments on this, Mauro?
> 
> Assuming that Mauro is OK with this change, (since he is the author of
> this driver) then yes - please resubmit the patch with some
> explanation within comments inline or within the commit message.

Michael,

Please don't top post. My comments are below.

> 
> Best regards,
> 
> Mike Krufky
> 
> On Mon, Sep 30, 2013 at 12:42 PM, Jiří Pinkava <j-pi@seznam.cz> wrote:
> > Mike,
> >
> > unfortunately no documentation can be referenced except preliminary
> > version of
> > datasheet (1).This change is based on lucky guess and supported by lot of
> > testing on real hardware.
> >
> > This change add support for devices with Xtal frequency bellow 28.8MHz.

Hmm... I'm not seeing any test there checking if the frequency of the xtal
is below to 28.8 MHz. So, if this logic apples only in this case, the
patch needs to add a test for the xtal frequency.

Also, as the driver does some tests for other Raphael tuners, if this change
is known to work fine only on rt820t, please add a test there for the tuner
model too.

Except for that, I'm ok with the changes, provided that you add the
explanations about it.

> >
> > From Nint  are computed values of Ni and Si. For 28.8MHz crystal can
> > reach up to 12 / 3 (Ni / Si). Tuner supports crystals with frequencies
> > (1) 12, 16, 20, 20.48, 24, 27, 28.8, 32 MHz, but this kind of device is
> > rare to found.
> > Allowing Ni to go up to 15 instead of only 12 should be safe and for 15
> > / 3 (Ni / Si)
> > we can compute limit for Nint = max(Ni) * 4 + max(Si) + 13 = 76.
> >
> > If This is sufficient and acceptable explanation I can add some sort of
> > documentation into patch and resend it (both patches, I can prove I'm
> > right :)
> >
> > (1)
> > http://rtl-sdr.com/wp-content/uploads/2013/04/R820T_datasheet-Non_R-20111130_unlocked.pdf
> >
> >> Jiří,
> >>
> >> Do you have any documentation that supports this value change?
> >> Changing this value affects the algorithm, and we'd be happier making
> >> this change if the patch included some better description and perhaps
> >> a reference explaining why the new value is correct.
> >>
> >> Regards,
> >>
> >> Mike Krufky
> >>
> >> On Sun, Sep 29, 2013 at 6:45 AM, Jiří Pinkava <j-pi@seznam.cz> wrote:
> >>>
> >>> Use full range of VCO parameters, fixes tunning for some frequencies.
> >>> ---
> >>>  drivers/media/tuners/r820t.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/media/tuners/r820t.c b/drivers/media/tuners/r820t.c
> >>> index 1c23666..e25c720 100644
> >>> --- a/drivers/media/tuners/r820t.c
> >>> +++ b/drivers/media/tuners/r820t.c
> >>> @@ -637,7 +637,7 @@ static int r820t_set_pll(struct r820t_priv *priv,
> >>> enum v4l2_tuner_type type,
> >>>                 vco_fra = pll_ref * 129 / 128;
> >>>         }
> >>>
> >>> -       if (nint > 63) {
> >>> +       if (nint > 76) {
> >>>                 tuner_info("No valid PLL values for %u kHz!\n", freq);
> >>>                 return -EINVAL;
> >>>         }
> >>> --
> >>> 1.8.3.2
> >>>
> >>>
> >


-- 

Cheers,
Mauro