On Sun, Jan 22, 2017 at 6:13 AM, Maxime Ripard
[off-list ref] wrote:

On Sat, Jan 21, 2017 at 09:27:42AM +0800, Chen-Yu Tsai wrote:

quoted

On Fri, Jan 20, 2017 at 3:29 PM, Maxime Ripard
[off-list ref] wrote:

quoted

Signed-off-by: Maxime Ripard <redacted>
---
 drivers/clk/sunxi-ng/ccu_mult.c | 16 +++++++++++++---
 drivers/clk/sunxi-ng/ccu_mult.h | 10 ++++++----
 drivers/clk/sunxi-ng/ccu_nk.c   |  8 ++++----
 drivers/clk/sunxi-ng/ccu_nkm.c  |  8 ++++----
 drivers/clk/sunxi-ng/ccu_nkmp.c |  8 ++++----
 drivers/clk/sunxi-ng/ccu_nm.c   |  4 ++--
 6 files changed, 33 insertions(+), 21 deletions(-)

diff --git a/drivers/clk/sunxi-ng/ccu_mult.c b/drivers/clk/sunxi-ng/ccu_mult.c
index 8b7ee7baa85b..8724c01171b1 100644
--- a/drivers/clk/sunxi-ng/ccu_mult.c
+++ b/drivers/clk/sunxi-ng/ccu_mult.c

@@ -40,8 +40,13 @@ static unsigned long ccu_mult_round_rate(struct ccu_mux_internal *mux,
        struct ccu_mult *cm = data;
        struct _ccu_mult _cm;

-       _cm.min = 1;
-       _cm.max = 1 << cm->mult.width;
+       _cm.min = cm->mult.min;

This particular line should probably be in a separate patch fixing
commit 2beaa601c849 ("clk: sunxi-ng: Implement minimum for multipliers")?
It kind of sticks out, and doesn't match the commit message.

Indeed (also because there's no commit message, I'll fix that.)

quoted

quoted

+
+       if (cm->mult.max)
+               _cm.max = cm->mult.max;
+       else
+               _cm.max = (1 << cm->mult.width) + cm->mult.offset - 1;
+
        ccu_mult_find_best(parent_rate, rate, &_cm);

        return parent_rate * _cm.mult;

@@ -114,7 +119,12 @@ static int ccu_mult_set_rate(struct clk_hw *hw, unsigned long rate,
                                                &parent_rate);

        _cm.min = cm->mult.min;
-       _cm.max = 1 << cm->mult.width;
+
+       if (cm->mult.max)
+               _cm.max = cm->mult.max;
+       else
+               _cm.max = (1 << cm->mult.width) + cm->mult.offset - 1;

The changes look good. Thinking about this more, you might need to
adjust the default minimum to account for the offset as well? At
the moment it doesn't really affect us, as the offset is either
1 or 0, which means a minimum of 1 is equally good. But leaving
a potential error in there doesn't feel right.

I'm not sure we should really care, I'm not aware of any SoC that
would have such a clock, either in the "old" or "new" ones. I'm not
sure we should fix that isn't broken.

Indeed. Here's hoping Allwinner doesn't try anything "smart".
What I'm concerned about is that we are leaving gaps in the clk
driver that will make it harder to understand for newcomers.

ChenYu

quoted

Said change would be against patch 2, so

Acked-by: Chen-Yu Tsai <redacted>

for this patch once the first comment is addressed.

I'll do that change and respin the serie, thanks!
Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com