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
