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

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

+
+       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.

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170121/d293a654/attachment.sig>