[RFC] ARM: smp_twd: Fix twd_get_clock() to get clock from dts or clock framework

Commit Message

Dinh Nguyen Aug. 9, 2013, 10:35 p.m. UTC

From: Dinh Nguyen <[e-mail]>

Some platforms have a clock for the smp_twd that goes through a fixed
divider. Registering this smp_twd clocks is done by calling
clk_register_fixed_factor().

Fix up twd_get_clock() so that it can get the clock from the device
tree node or clk_register_fixed_factor().

Signed-off-by: Dinh Nguyen <[e-mail]>
Cc: Viresh Kumar <[e-mail]>
Cc: Linus Walleij <[e-mail]>
Cc: Rob Herring <[e-mail]>
Cc: Arnd Bergmann <[e-mail]>
CC: Russell King <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
---
 arch/arm/kernel/smp_twd.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Linus Walleij Aug. 14, 2013, 7:47 p.m. UTC | #1

On Sat, Aug 10, 2013 at 12:35 AM,  <[e-mail]> wrote:

> From: Dinh Nguyen <[e-mail]>
>
> Some platforms have a clock for the smp_twd that goes through a fixed
> divider. Registering this smp_twd clocks is done by calling
> clk_register_fixed_factor().
>
> Fix up twd_get_clock() so that it can get the clock from the device
> tree node or clk_register_fixed_factor().
>
> Signed-off-by: Dinh Nguyen <[e-mail]>
(...)
>         if (np)
>                 twd_clk = of_clk_get(np, 0);
> -       else
> +
> +       /* Some platforms do not register the smp_twd clock in the device */
> +       /* node of the twd timer. */
> +       if (IS_ERR(twd_clk))
>                 twd_clk = clk_get_sys("smp_twd", NULL);

NACK.

The patch commit message and the patch itself does not match.
This has nothing to do with using fixed factor or whatever.

What it wants to do is use clk_get_sys() in case of_clk_get() fails,
so DT clocks take precedence.

But this patch is buggy. If you have only platform data, and boot
without device tree, np will be NULL, and twd_clk will also be
NULL and thus clk_get_sys() will not execute and the system
is bugged up.

Yours,
Linus Walleij

Dinh Nguyen Aug. 21, 2013, 7:59 p.m. UTC | #2

On Wed, 2013-08-14 at 21:47 +0200, Linus Walleij wrote:
> On Sat, Aug 10, 2013 at 12:35 AM,  <[e-mail]> wrote:
> 
> > From: Dinh Nguyen <[e-mail]>
> >
> > Some platforms have a clock for the smp_twd that goes through a fixed
> > divider. Registering this smp_twd clocks is done by calling
> > clk_register_fixed_factor().
> >
> > Fix up twd_get_clock() so that it can get the clock from the device
> > tree node or clk_register_fixed_factor().
> >
> > Signed-off-by: Dinh Nguyen <[e-mail]>
> (...)
> >         if (np)
> >                 twd_clk = of_clk_get(np, 0);
> > -       else
> > +
> > +       /* Some platforms do not register the smp_twd clock in the device */
> > +       /* node of the twd timer. */
> > +       if (IS_ERR(twd_clk))
> >                 twd_clk = clk_get_sys("smp_twd", NULL);
> 
> NACK.
> 
> The patch commit message and the patch itself does not match.
> This has nothing to do with using fixed factor or whatever.
> 
> What it wants to do is use clk_get_sys() in case of_clk_get() fails,
> so DT clocks take precedence.
> 
> But this patch is buggy. If you have only platform data, and boot
> without device tree, np will be NULL, and twd_clk will also be
> NULL and thus clk_get_sys() will not execute and the system
> is bugged up.

Ah, I apologize. I did not even think about the none device tree case.

Dinh
> 
> Yours,
> Linus Walleij
>

Patch

diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
index 2595620..643b226 100644
--- a/arch/arm/kernel/smp_twd.c
+++ b/arch/arm/kernel/smp_twd.c
@@ -244,7 +244,10 @@  static void twd_get_clock(struct device_node *np)
 
 	if (np)
 		twd_clk = of_clk_get(np, 0);
-	else
+
+	/* Some platforms do not register the smp_twd clock in the device */
+	/* node of the twd timer. */
+	if (IS_ERR(twd_clk))	
 		twd_clk = clk_get_sys("smp_twd", NULL);
 
 	if (IS_ERR(twd_clk)) {