Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1753559AbbDHOAW (ORCPT <rfc822;w@1wt.eu>);
	Wed, 8 Apr 2015 10:00:22 -0400
Received: from mout.gmx.net ([212.227.15.19]:59774 "EHLO mout.gmx.net"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751376AbbDHOAQ (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Wed, 8 Apr 2015 10:00:16 -0400
MIME-Version: 1.0
Message-ID: <trinity-d1320d47-a158-4f77-8ddc-8dbeb74c6f8b-1428501608111@3capp-gmx-bs05>
From: "S. S." <ce3a@gmx.de>
To: "Sebastian Hesselbarth" <sebastian.hesselbarth@gmail.com>,
        mturquette@linaro.org, sboyd@codeaurora.org
Cc: linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Aw: Re: [PATCH v2] clk: si5351: fix .round_rate for multisynth 6-7
Content-Type: text/plain; charset=UTF-8
Date: Wed, 8 Apr 2015 16:00:08 +0200
Importance: normal
Sensitivity: Normal
In-Reply-To: <552523BC.7010500@gmail.com>
References: <1428433918-6339-1-git-send-email-ce3a@gmx.de>,
 <552523BC.7010500@gmail.com>
X-UI-Message-Type: mail
X-Priority: 3
X-Provags-ID: V03:K0:bge2ZLRhvNJXjdaptin+Kx4SV/v8mn5l0NEPIEmDNIX
 80yXqE7yArZD6QATOB1GNxusM/YXcMDJsws8j3aA+Ji0TW6rA7
 wxE9PUuUIZSs6w3vNt76dr5IqWCAiS23dVaQbnw8h0SXumsMz9
 SjKSruGKMYUC82ivwIwq0z1PMT80g7/5L3lSpwtiNZ9KZ+Y8d9
 6fEgzkpZLjN9xPJAcYpIk+u4SfX3nuUl9rGZNVhDLy3g3oYmhN
 7kVjoVtgCARoGmXjAs3tDSJud36/xQyc+x6F/JjuaWe6/SnO4t tCC1lY=
X-UI-Out-Filterresults: notjunk:1;
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 2376
Lines: 75

>> @@ -645,7 +646,7 @@ static long si5351_msynth_round_rate(struct clk_hw *hw, unsigned long rate,
>> struct si5351_hw_data *hwdata =
>> container_of(hw, struct si5351_hw_data, hw);
>> unsigned long long lltmp;
>> - unsigned long a, b, c;
>> + unsigned long a, b = 0, c = 1;
>
> Actually, moving b,c initialization up here is neither related
> to the patch itself nor is it mentioned in the commit log.
>
> Please do not mix different patches.

I agree. I just thought we could avoid repeating the b,c initialization
in the code, but You are right, that would be a different patch, if any 
at all.

>> *parent_rate = a * rate;
>
>> + if (rfrac)
>> + rational_best_approximation(rfrac, denom,
>> + SI5351_MULTISYNTH_B_MAX,
>> + SI5351_MULTISYNTH_C_MAX,
>> + &b, &c);
>> + }
>
> I am still not convinced that this is the right way to calculate the
> _best_ integer divider for ms6,7.
>
> The code above is written to (a) find the _largest_ integer "a" that
> will match
>
> a * rate <= parent_rate
>
> and then (b) determines the fractional part of the divider.
>
> As you correctly stated, ms6,7 do not support (b) but if we use (a) for
> those msynths, we will determine an "a" so that "a * rate" is always
> smaller than parent_rate.
> 
> What we actually want is the smallest error between generated and
> requested rate, so we have to find the closest integer with
> 
> a = DIV_ROUND_CLOSEST(parent_rate, rate)

Agreed, that's probably better.

> IMHO, the special divider calculation for ms6,7 is best dealt with
> in an extra else-if branch after the check for CLK_SET_RATE_PARENT
> flag.

Agreed.

> hwdata->params.p3 = 0;
> hwdata->params.p2 = 0;

Agreed.

> Out of curiosity, do you actually have a device that uses ms6,7 and can
> you measure the generated frequency - or did you just read the code as
> a bedtime story? ;)

:) I do have a device and have to use ms6,7, those are connected to pllb.
I have tested some integer pll dividers in a range from 30 to 130.
Measured frequencies and the frequencies shown in <debugfs>/clk/clk_summery
are the same.

I am going to send a patch v3.
Thanks for your feedback.

Sergej
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/