Hi Morimoto-san, 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". > > (snip) > > >> + * 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 :-) -- Regards, Laurent Pinchart
