On Mon, Apr 23, 2012 at 04:07:13PM -0700, Turquette, Mike wrote:

On Sat, Apr 21, 2012 at 8:57 AM, Shawn Guo [off-list ref] wrote:

quoted

Add imx23 clock support based on common clk framework.

Signed-off-by: Shawn Guo <redacted>
---

...

quoted

+int __init mx23_clocks_init(void)
+{
+ ? ? ? struct clk *clk;
+ ? ? ? int ret;
+
+ ? ? ? clk_misc_init();
+
+ ? ? ? mxs_clk_fixed("ref_xtal", 24000000);

There is no error checking at all?

Right now, the only possible reason that the registration function could
fail is -ENOMEM.  I thought it's not worth checking tens of function
returns for the same error which is unlikely to happen.

But if you're strong on this, I can add a check.

quoted

diff --git a/drivers/clk/mxs/clk.h b/drivers/clk/mxs/clk.h
index deb5c23..fb5937d 100644
--- a/drivers/clk/mxs/clk.h
+++ b/drivers/clk/mxs/clk.h

@@ -12,6 +12,8 @@

?#ifndef __MXS_CLK_H
?#define __MXS_CLK_H

+#include <linux/clk.h>
+#include <linux/clkdev.h>
?#include <linux/clk-provider.h>
?#include <linux/err.h>
?#include <linux/io.h>

@@ -72,4 +74,22 @@ static inline int mxs_clk_wait(void __iomem *reg, u8 shift)

? ? ? ?return 0;
?}

+static inline int mxs_clk_init_on(char **clks_init_on, int num)
+{
+ ? ? ? struct clk *clk;
+ ? ? ? int i;
+
+ ? ? ? for (i = 0; i < num; i++) {
+ ? ? ? ? ? ? ? clk = clk_get_sys(clks_init_on[i], NULL);
+ ? ? ? ? ? ? ? if (IS_ERR(clk)) {
+ ? ? ? ? ? ? ? ? ? ? ? pr_err("%s: failed to get clk %s", __func__,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?clks_init_on[i]);
+ ? ? ? ? ? ? ? ? ? ? ? return PTR_ERR(clk);
+ ? ? ? ? ? ? ? }
+ ? ? ? ? ? ? ? clk_prepare_enable(clk);
+ ? ? ? }
+
+ ? ? ? return 0;
+}

Any good reason for this code to live in the header as static inline?

It's a small function which is used by both clk-imx23.c and clk-imx28.c.

Do you call this code any time other than at boot?  Why not __init?

Eh, __init for inline function?

-- 
Regards,
Shawn