Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752723AbcLMXTu (ORCPT ); Tue, 13 Dec 2016 18:19:50 -0500 Received: from smtp.codeaurora.org ([198.145.29.96]:48196 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751171AbcLMXTs (ORCPT ); Tue, 13 Dec 2016 18:19:48 -0500 DMARC-Filter: OpenDMARC Filter v1.3.1 smtp.codeaurora.org 5DD55601CF Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=pass smtp.mailfrom=sboyd@codeaurora.org Date: Tue, 13 Dec 2016 15:19:00 -0800 From: Stephen Boyd To: Nikita Yushchenko Cc: Shawn Guo , Sascha Hauer , Fabio Estevam , Michael Turquette , linux-clk@vger.kernel.org, Chris Healy , linux-kernel@vger.kernel.org, Andrey Smirnov Subject: Re: [PATCH] clk: imx: pllv3: support fractional multiplier on vf610 PLL1/PLL2 Message-ID: <20161213231900.GP5423@codeaurora.org> References: <1481288408-16035-1-git-send-email-nikita.yoush@cogentembedded.com> <20161213073858.GO5423@codeaurora.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2063 Lines: 71 On 12/13, Nikita Yushchenko wrote: > >> diff --git a/drivers/clk/imx/clk-pllv3.c b/drivers/clk/imx/clk-pllv3.c > >> index 19f9b622981a..24a9e914e0d5 100644 > >> --- a/drivers/clk/imx/clk-pllv3.c > >> +++ b/drivers/clk/imx/clk-pllv3.c > > >> + > >> + temp64 *= mfn; > >> + do_div(temp64, mfd); > >> + > >> + return (parent_rate * div) + (u32)temp64; > >> +} > >> + > >> +static long clk_pllv3_vf610_round_rate(struct clk_hw *hw, unsigned long rate, > >> + unsigned long *prate) > >> +{ > >> + unsigned long parent_rate = *prate; > >> + unsigned int mfi = (rate >= 22 * parent_rate) ? 22 : 20; > > > > What is the importance of 22 and 20? Hint, at the least it needs > > a comment. > > These come directly from datasheet: > > Frequency multipler selection (MFI). > 0: Fout = Fref * 20 > 1: Fout = Fref * 22 > > These numbers (20 / 22) are common among flavours of pllv3 hardware. > In similar places in the same file (e.g. in clk_pllv3_recalc_rate(), in > clk_pllv3_set_rate() ,etc) there are no comments explaining them. > > Are you sure this place is special and comment is needed here? Probably an oversight when merging the code before. Please add a comment, or make some sort of function like rate_to_mfi(rate, parent_rate) And then add a comment above the function to describe what's special about 22 and 20. > > >> + u32 mfn, mfd = 0x3fffffff; > >> + u64 temp64; > >> + > >> + if (rate <= parent_rate * mfi) > >> + mfn = 0; > >> + else if (rate >= parent_rate * (mfi + 1)) > >> + mfn = mfd - 1; > >> + else { > >> + /* rate = parent_rate * (mfi + mfn/mfd) */ > >> + temp64 = rate - parent_rate * mfi; > >> + temp64 *= mfd; > >> + do_div(temp64, parent_rate); > >> + mfn = temp64; > >> + } > >> + > >> + temp64 = ((u64)mfd * mfi + mfn) * parent_rate; > >> + do_div(temp64, mfd); > >> + return (u32)temp64; > > > > Do we need the cast here for some reason? > > Just for readability, can remove if it hurts. Please do. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project