Message-ID: <87bmiwqw1k.wl%kuninori.morimoto.gx@renesas.com>
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>,
        David Airlie <airlied@linux.ie>,
        Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
        Linux-Kernel <linux-kernel@vger.kernel.org>,
        dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v4 2/2] drm: rcar-du: calculate DPLLCR to be more small jitter
In-Reply-To: <2409831.iIlgTMgqFv@avalon>
References: <87r2rsrifu.wl%kuninori.morimoto.gx@renesas.com>
        <87o9mwridk.wl%kuninori.morimoto.gx@renesas.com>
        <2409831.iIlgTMgqFv@avalon>
User-Agent: Wanderlust/2.15.9 Emacs/24.3 Mule/6.0
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Date: Mon, 18 Dec 2017 08:38:19 +0000
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Dec 2017 08:38:19.4615 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: f27d8c60-00b8-4275-8683-08d545f2b12b
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 53d82571-da19-47e4-9cb4-625a166a4a2a
X-MS-Exchange-Transport-CrossTenantHeadersStamped: HK2PR0601MB1874
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 1811
Lines: 67


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)

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".

> 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@ideasonboard.com>

Thank you for your help


Best regards
---
Kuninori Morimoto