From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753004AbbDFQoB (ORCPT ); Mon, 6 Apr 2015 12:44:01 -0400 Received: from mail-wi0-f170.google.com ([209.85.212.170]:37583 "EHLO mail-wi0-f170.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750891AbbDFQn7 (ORCPT ); Mon, 6 Apr 2015 12:43:59 -0400 Message-ID: <5522B7CB.40507@gmail.com> Date: Mon, 06 Apr 2015 18:43:55 +0200 From: Sebastian Hesselbarth User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 To: Sergej Sawazki , mturquette@linaro.org, sboyd@codeaurora.org CC: linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] clk: si5351: fix .round_rate for multisynth 6-7 References: <1428330321-25209-1-git-send-email-ce3a@gmx.de> In-Reply-To: <1428330321-25209-1-git-send-email-ce3a@gmx.de> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 06.04.2015 16:25, Sergej Sawazki wrote: > The divider calculation for multisynth 6 and 7 differs from the > calculation for multisynth 0-5. > > For MS6 and MS7, set MSx_P1 directly, MSx_P1=divide value > [AN619, p. 6]. > > Referenced document: > [AN619] Manually Generating an Si5351 Register Map, Rev. 0.4 > > Signed-off-by: Sergej Sawazki Sergej, thanks for the patch, I do have some remarks though. > --- > drivers/clk/clk-si5351.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/clk/clk-si5351.c b/drivers/clk/clk-si5351.c > index 44ea107..310078d 100644 > --- a/drivers/clk/clk-si5351.c > +++ b/drivers/clk/clk-si5351.c > @@ -552,7 +552,8 @@ static const struct clk_ops si5351_pll_ops = { > * MSx_P2[19:0] = 128 * b - c * floor(128 * b/c) = (128*b) mod c > * MSx_P3[19:0] = c > * > - * MS[6,7] are integer (P1) divide only, P2 = 0, P3 = 0 > + * MS[6,7] are integer (P1) divide only, P1 = divide value, > + * P2 and P3 are not applicable > * > * for 150MHz < fOUT <= 160MHz: > * > @@ -718,11 +719,18 @@ static long si5351_msynth_round_rate(struct clk_hw *hw, unsigned long rate, > do_div(lltmp, a * c + b); > rate = (unsigned long)lltmp; > > - /* calculate parameters */ > + /* > + * calculate parameters > + * for multisync6-7 set p1 directly, fOUT = fIN / p1 I'd say the comment about the differences of MS6,7 in the function's comment block is enough, no need to repeat it again here. > + */ > if (divby4) { > hwdata->params.p3 = 1; > hwdata->params.p2 = 0; > hwdata->params.p1 = 0; > + } else if (hwdata->num >= 6) { > + hwdata->params.p3 = 1; > + hwdata->params.p2 = 0; > + hwdata->params.p1 = a; Hmm. Just overruling what will be written in the registers is definitely not enough. If you look at the first two lines of this hunk, you'll admit that the visible representation (rate) of the MS frequency isn't affected by your patch at all. That basically means that real frequency and frequency in the kernel will differ, so we have to put this change in the param calculation above this hunk instead. The extra else-if-path here is ok though. Sebastian > } else { > hwdata->params.p3 = c; > hwdata->params.p2 = (128 * b) % c;