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

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

[dinguyen at altera.com]

From: Dinh Nguyen <dinguyen@altera.com>

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 <dinguyen@altera.com>
Cc: Viresh Kumar <viresh.linux@gmail.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Arnd Bergmann <arnd@arndb.de>
CC: Russell King <linux@arm.linux.org.uk>
Cc: spear-devel at list.st.com
Cc: linux-arm-kernel at lists.infradead.org
---
 arch/arm/kernel/smp_twd.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

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)) {
-- 
1.7.9.5

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

[Linus Walleij]

On Sat, Aug 10, 2013 at 12:35 AM,  <dinguyen@altera.com> wrote:

> From: Dinh Nguyen <dinguyen@altera.com>
>
> 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 <dinguyen@altera.com>
(...)
>         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

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

[Dinh Nguyen]

On Wed, 2013-08-14 at 21:47 +0200, Linus Walleij wrote:
> On Sat, Aug 10, 2013 at 12:35 AM,  <dinguyen@altera.com> wrote:
> 
> > From: Dinh Nguyen <dinguyen@altera.com>
> >
> > 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 <dinguyen@altera.com>
> (...)
> >         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
>