Add .determine_rate to composite clk

Add .determine_rate to composite clk

[Lemon Dai]

Hi Mike,

As .determine_rate was added to struct clk and clk_mux_ops, maybe we
should also copy determine_rate operation from mux_ops to
clk_composite_ops in clk_register_composite( ),
to make composite clk implementation more generic.

Patch:
diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
index a33f46f..d224d6e 100644
--- a/drivers/clk/clk-composite.c
+++ b/drivers/clk/clk-composite.c
@@ -43,6 +43,20 @@ static int clk_composite_set_parent(struct clk_hw
*hw, u8 index)
        return mux_ops->set_parent(mux_hw, index);
 }

+static long clk_composite_determine_rate(struct clk_hw *hw, unsigned long rate,
+                               unsigned long *best_parent_rate,
+                               struct clk **best_parent_p)
+{
+       struct clk_composite *composite = to_clk_composite(hw);
+       const struct clk_ops *mux_ops = composite->mux_ops;
+       struct clk_hw *mux_hw = composite->mux_hw;
+
+       mux_hw->clk = hw->clk;
+
+       return mux_ops->determine_rate(mux_hw, rate, best_parent_rate,
+                               best_parent_p);
+}
+
 static unsigned long clk_composite_recalc_rate(struct clk_hw *hw,
                                            unsigned long parent_rate)
 {
@@ -147,6 +161,11 @@ struct clk *clk_register_composite(struct device
*dev, const char *name,
                composite->mux_ops = mux_ops;
                clk_composite_ops->get_parent = clk_composite_get_parent;
                clk_composite_ops->set_parent = clk_composite_set_parent;
+
+               if (mux_ops->determine_rate) {
+                       clk_composite_ops->determine_rate =
+                                       clk_composite_determine_rate;
+               }
        }

        if (rate_hw && rate_ops) {


--
Best regards,

Lemon Dai

Add .determine_rate to composite clk

[Maxime Ripard]

Hi Lemon,

On Sat, Jan 11, 2014 at 05:15:58PM +0800, Lemon Dai wrote:
> Hi Mike,
> 
> As .determine_rate was added to struct clk and clk_mux_ops, maybe we
> should also copy determine_rate operation from mux_ops to
> clk_composite_ops in clk_register_composite( ),
> to make composite clk implementation more generic.

A similar patch is on its way to 3.14 already (commit 107f3198 in
Mike's clk-next branch).

Also, for future contributions, I'd suggest to read the
Documentation/SubmittingPatches file. Your patch had several style
issues that are covered in this file.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140111/9c6994f3/attachment.sig>

Add .determine_rate to composite clk

[Lemon Dai]

Hi Maxime,

The commit 107f3198 in Mike's clk-next branch shows that:
(about line 72 in drivers/clk/clk-composite.c)

        } else if (mux_hw && mux_ops && mux_ops->determine_rate) {
                mux_hw->clk = hw->clk;
                return mux_ops->determine_rate(rate_hw, rate, best_parent_rate,
                                               best_parent_p);


 It should be fixed as follows, at least for reason that  NULL rate_hw
may be used in the old code,

        } else if (mux_hw && mux_ops && mux_ops->determine_rate) {
                mux_hw->clk = hw->clk;
 -              return mux_ops->determine_rate(rate_hw, rate, best_parent_rate,
 +             return mux_ops->determine_rate(mux_hw, rate, best_parent_rate,
                                               best_parent_p);

--
Best regards,
Lemon Dai

On Sun, Jan 12, 2014 at 11:56 AM, Lemon Dai <dailemon.gl@gmail.com> wrote:
> Hi  Maxime,
>
> Thank you for your reply and suggestion.
>  I am sorry for sending  a mail with such a lot of style issues before
> reading Documentation/SubmittingPatches file.
>
> Best wishes,
> Lemon
>
> On Sat, Jan 11, 2014 at 6:28 PM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
>> Hi Lemon,
>>
>> On Sat, Jan 11, 2014 at 05:15:58PM +0800, Lemon Dai wrote:
>>> Hi Mike,
>>>
>>> As .determine_rate was added to struct clk and clk_mux_ops, maybe we
>>> should also copy determine_rate operation from mux_ops to
>>> clk_composite_ops in clk_register_composite( ),
>>> to make composite clk implementation more generic.
>>
>> A similar patch is on its way to 3.14 already (commit 107f3198 in
>> Mike's clk-next branch).
>>
>> Also, for future contributions, I'd suggest to read the
>> Documentation/SubmittingPatches file. Your patch had several style
>> issues that are covered in this file.
>>
>> Thanks!
>> Maxime
>>
>> --
>> Maxime Ripard, Free Electrons
>> Embedded Linux, Kernel and Android engineering
>> http://free-electrons.com

Add .determine_rate to composite clk

[Mike Turquette]

Quoting Lemon Dai (2014-01-14 06:08:44)
> Hi Maxime,
> 
> The commit 107f3198 in Mike's clk-next branch shows that:
> (about line 72 in drivers/clk/clk-composite.c)
> 
>         } else if (mux_hw && mux_ops && mux_ops->determine_rate) {
>                 mux_hw->clk = hw->clk;
>                 return mux_ops->determine_rate(rate_hw, rate, best_parent_rate,
>                                                best_parent_p);
> 
> 
>  It should be fixed as follows, at least for reason that  NULL rate_hw
> may be used in the old code,
> 
>         } else if (mux_hw && mux_ops && mux_ops->determine_rate) {
>                 mux_hw->clk = hw->clk;
>  -              return mux_ops->determine_rate(rate_hw, rate, best_parent_rate,
>  +             return mux_ops->determine_rate(mux_hw, rate, best_parent_rate,
>                                                best_parent_p);

Hi Lemon Dai,

I have applied this fix to clk-next. The patch is below:



>From ca01f3f60aa864d7ee8124ca597d4010378926b5 Mon Sep 17 00:00:00 2001
From: Mike Turquette <mturquette@linaro.org>
Date: Tue, 14 Jan 2014 12:56:01 -0800
Subject: [PATCH] clk: composite: pass mux_hw into determine_rate

The composite clock's .determine_rate implementation can call the
underyling .determine_rate callback corresponding to rate_hw or the
underlying .determine_rate callback corresponding to mux_hw. In both
cases we pass in rate_hw, which is wrong. Fixed by passing mux_hw into
the correct callback.

Reported-by: Lemon Dai <dailemon.gl@gmail.com>
Signed-off-by: Mike Turquette <mturquette@linaro.org>
---
 drivers/clk/clk-composite.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
index 753d0b7..57a078e 100644
--- a/drivers/clk/clk-composite.c
+++ b/drivers/clk/clk-composite.c
@@ -71,7 +71,7 @@ static long clk_composite_determine_rate(struct clk_hw *hw, unsigned long rate,
 						best_parent_p);
 	} else if (mux_hw && mux_ops && mux_ops->determine_rate) {
 		mux_hw->clk = hw->clk;
-		return mux_ops->determine_rate(rate_hw, rate, best_parent_rate,
+		return mux_ops->determine_rate(mux_hw, rate, best_parent_rate,
 					       best_parent_p);
 	} else {
 		pr_err("clk: clk_composite_determine_rate function called, but no mux or rate callback set!\n");
-- 
1.8.3.2