[Proposal][PATCH] sh: clkfw: Moved the .init callback in the clk_register

[Proposal][PATCH] sh: clkfw: Moved the .init callback in the clk_register

[Francesco VIRLINZI]

[-- Attachment #1: Type: text/plain, Size: 255 bytes --]

Hi Magnus
Here there is the patch to move the .init callback during the clock 
registration.
Currently the __clk_init returns always zero...
 in the next step if the __clk_init fails the clock registration will be 
rejected.

Let me know.
Ciao
 Francesco

[-- Attachment #2: .0001-sh-clkfw-Moved-the-.init-callback-in-the-clk_regis.patch.swp --]
[-- Type: application/octet-stream, Size: 12288 bytes --]

Re: [Proposal][PATCH] sh: clkfw: Moved the .init callback in the

[Francesco VIRLINZI]

[-- Attachment #1: Type: text/plain, Size: 376 bytes --]

Sorry... THe attached was broken.
Here the right patch,
Ciao
 Francesco
Francesco VIRLINZI ha scritto:
> Hi Magnus
> Here there is the patch to move the .init callback during the clock 
> registration.
> Currently the __clk_init returns always zero...
> in the next step if the __clk_init fails the clock registration will 
> be rejected.
>
> Let me know.
> Ciao
> Francesco


[-- Attachment #2: 0001-sh-clkfw-Moved-the-.init-callback-in-the-clk_regis.patch --]
[-- Type: text/x-patch, Size: 2848 bytes --]

From 8b11a6b0fac5d0bbd85b43c1f5e51c006e32ffbb Mon Sep 17 00:00:00 2001
From: Francesco Virlinzi <francesco.virlinzi@st.com>
Date: Mon, 11 May 2009 11:04:11 +0200
Subject: [PATCH] sh: clkfw: Moved the .init callback in the clk_register function

This patch moves the .init callback function in the clk_register

Signed-off-by: Francesco Virlinzi <francesco.virlinzi@st.com>
---
 arch/sh/include/asm/clock.h |    3 +--
 arch/sh/kernel/cpu/clock.c  |   20 ++++++++++----------
 2 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/arch/sh/include/asm/clock.h b/arch/sh/include/asm/clock.h
index b1f2919..f8d253b 100644
--- a/arch/sh/include/asm/clock.h
+++ b/arch/sh/include/asm/clock.h
@@ -9,7 +9,7 @@
 struct clk;
 
 struct clk_ops {
-	void (*init)(struct clk *clk);
+	int (*init)(struct clk *clk);
 	void (*enable)(struct clk *clk);
 	void (*disable)(struct clk *clk);
 	void (*recalc)(struct clk *clk);
@@ -36,7 +36,6 @@ struct clk {
 
 #define CLK_ALWAYS_ENABLED	(1 << 0)
 #define CLK_RATE_PROPAGATES	(1 << 1)
-#define CLK_NEEDS_INIT		(1 << 2)
 
 /* Should be defined by processor-specific code */
 void arch_init_clk_ops(struct clk_ops **, int type);
diff --git a/arch/sh/kernel/cpu/clock.c b/arch/sh/kernel/cpu/clock.c
index 133dbe4..16b5cca 100644
--- a/arch/sh/kernel/cpu/clock.c
+++ b/arch/sh/kernel/cpu/clock.c
@@ -89,8 +89,9 @@ static void propagate_rate(struct clk *clk)
 	}
 }
 
-static void __clk_init(struct clk *clk)
+static int __clk_init(struct clk *clk)
 {
+	int ret = 0;
 	/*
 	 * See if this is the first time we're enabling the clock, some
 	 * clocks that are always enabled still require "special"
@@ -99,12 +100,11 @@ static void __clk_init(struct clk *clk)
 	 * divisors to use before it can effectively recalc.
 	 */
 
-	if (clk->flags & CLK_NEEDS_INIT) {
-		if (clk->ops && clk->ops->init)
-			clk->ops->init(clk);
+	if (clk->ops && clk->ops->init)
+		clk->ops->init(clk);	/* First step... */
+/*		ret = clk->ops->init(clk); ... Next step...*/
 
-		clk->flags &= ~CLK_NEEDS_INIT;
-	}
+	return ret;
 }
 
 static int __clk_enable(struct clk *clk)
@@ -119,8 +119,6 @@ static int __clk_enable(struct clk *clk)
 		return 0;
 
 	if (clk->usecount == 1) {
-		__clk_init(clk);
-
 		__clk_enable(clk->parent);
 
 		if (clk->ops && clk->ops->enable)
@@ -175,16 +173,18 @@ EXPORT_SYMBOL_GPL(clk_disable);
 
 int clk_register(struct clk *clk)
 {
+
+	if (__clk_init(clk))	/* it does every hardware initialization */
+		return -EPERM;	/* and checks they were ok */
+
 	mutex_lock(&clock_list_sem);
 
 	list_add(&clk->node, &clock_list);
 	clk->usecount = 0;
-	clk->flags |= CLK_NEEDS_INIT;
 
 	mutex_unlock(&clock_list_sem);
 
 	if (clk->flags & CLK_ALWAYS_ENABLED) {
-		__clk_init(clk);
 		pr_debug( "Clock '%s' is ALWAYS_ENABLED\n", clk->name);
 		if (clk->ops && clk->ops->enable)
 			clk->ops->enable(clk);
-- 
1.6.0.6

Re: [Proposal][PATCH] sh: clkfw: Moved the .init callback in the

[Magnus Damm]

Hi Francesco,

I had a look at your patch. The idea behind the patch is fine with me
- still untested - but I'd be happy to test the next version if you
can improve it a bit. Here's some feedback:

Changing init() to return an int is ok with me. Not sure what Paul
thinks about this. Anyway, to do so you need to fix your patch so all
init() callbacks are changed. Applying this patch as-is will break
many things.

No need for the __clk_init() function any more. And please use the
return value from the init() callback.

You probably want to call init() from clk_register() after adding the
clock to the list. Also, return the value from the init() callback, no
need to always return -EPERM.

And if possible, please send the patch inline. That makes reviewing much easier.

Cheers,

/ magnus - waiting for a V3

On Mon, May 11, 2009 at 6:26 PM, Francesco VIRLINZI
<francesco.virlinzi@st.com> wrote:
> Sorry... THe attached was broken.
> Here the right patch,
> Ciao
> Francesco
> Francesco VIRLINZI ha scritto:
>>
>> Hi Magnus
>> Here there is the patch to move the .init callback during the clock
>> registration.
>> Currently the __clk_init returns always zero...
>> in the next step if the __clk_init fails the clock registration will be
>> rejected.
>>
>> Let me know.
>> Ciao
>> Francesco
>
>

Re: [Proposal][PATCH] sh: clkfw: Moved the .init callback in the

[Francesco VIRLINZI]

Hi Magnus
> Hi Francesco,
>
> I had a look at your patch. The idea behind the patch is fine with me
> - still untested - but I'd be happy to test the next version if you
> can improve it a bit. 
No problems I can do that... but.

Paul: I remember you had some issue on this design.

Do you agree now? Do you think the v3 is commit-able?

Let me know.

Regards
 Francesco

> Here's some feedback:
>
> Changing init() to return an int is ok with me. Not sure what Paul
> thinks about this. Anyway, to do so you need to fix your patch so all
> init() callbacks are changed. Applying this patch as-is will break
> many things.
>
> No need for the __clk_init() function any more. And please use the
> return value from the init() callback.
>
> You probably want to call init() from clk_register() after adding the
> clock to the list. Also, return the value from the init() callback, no
> need to always return -EPERM.
>
> And if possible, please send the patch inline. That makes reviewing much easier.
>
> Cheers,
>
> / magnus - waiting for a V3
>
> On Mon, May 11, 2009 at 6:26 PM, Francesco VIRLINZI
> <francesco.virlinzi@st.com> wrote:
>   
>> Sorry... THe attached was broken.
>> Here the right patch,
>> Ciao
>> Francesco
>> Francesco VIRLINZI ha scritto:
>>     
>>> Hi Magnus
>>> Here there is the patch to move the .init callback during the clock
>>> registration.
>>> Currently the __clk_init returns always zero...
>>> in the next step if the __clk_init fails the clock registration will be
>>> rejected.
>>>
>>> Let me know.
>>> Ciao
>>> Francesco
>>>       
>>     
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>