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

[Mauro Carvalho Chehab <m.chehab@samsung.com>]

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