Messages in this thread | | | Date | Tue, 13 Dec 2016 15:19:00 -0800 | From | Stephen Boyd <> | Subject | Re: [PATCH] clk: imx: pllv3: support fractional multiplier on vf610 PLL1/PLL2 |
| |
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
| |