Re: [PATCH v4 2/2] drm: rcar-du: calculate DPLLCR to be more small jitter
From: Laurent Pinchart
Date: Mon Dec 18 2017 - 03:39:59 EST
On Monday, 18 December 2017 10:38:19 EET Kuninori Morimoto wrote:
> Hi Laurent
> Thank you for your feedback
> >> + * To be small jitter,
> > Nitpicking, I would write this "to minimize the jitter".
> >> + * This code is assuming "used" from 64bit CPU only,
> >> + * not from 32bit CPU. But both can compile correctly
> > Nitpicking again, I would write this "This code only runs on 64-bit
> > architectures, the unsigned long type can thus be used for 64-bit
> > computation. It will still compile without any warning on 32-bit
> > architectures."
> I will follow your English ;)
> >> + /*
> >> + * fvco = fin * P * N / M
> >> + * fclkout = fin * N / M / FDPLL
> >> + *
> >> + * To avoid duplicate calculation, let's use below
> >> + *
> >> + * finnm = fin * N / M
> > This is called fout in your diagram above, I would use the same name here.
> Oops indeed. I didn't notice
> >> + unsigned long finnm = input * (n + 1) / (m + 1);
> >> + unsigned long fvco = finnm * 2;
> >> +
> >> + if (fvco < 2000 || fvco > 4096 * 1000 * 1000U)
> >> + continue;
> > How about
> > if (fvco < 1000 || fvco > 2048 * 1000 * 1000)
> > to avoid computing the intermediate fvco variable ?
> I think you want to say
> - if (fvco < 1000 || fvco > 2048 * 1000 * 1000)
> + if (fout < 1000 || fout > 2048 * 1000 * 1000)
Yes, sorry, that's what I meant.
> Actually I notcied about this, but I thought it makes
> user confuse. Thus, I kept original number.
> I'm happy if compiler can adjust it automatically,
> if not, I have no objection to modify it but we want to have such comment ?
> Because above comment/explain mentions about "fvco", not "fout".
Sure, I'll add a comment, it's a good point.
> > If you agree with these small changes there's no need to resubmit the
> > patch, I'll modify it when applying, and
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Thank you for your help
Thank you for the code :-)