On 01/22/2015 02:01 AM, Stephen Boyd wrote:

On 01/21, Tomeu Vizoso wrote:

quoted

@@ -2075,10 +2210,12 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
 		}
 	}
 
-	ret = __clk_init(dev, clk);
+	hw->clk = __clk_create_clk(hw, NULL, NULL);
+	ret = __clk_init(dev, hw->clk);
 	if (!ret)
-		return clk;
+		return hw->clk;
 
+	kfree(hw->clk);
 fail_parent_names_copy:
 	while (--i >= 0)
 		kfree(clk->parent_names[i]);

Sigh, this patch is so huge I keep finding more things. Sorry. It
looks like __clk_create_clk() can return an error pointer, which
we then send directly to __clk_init. First off, we shouldn't
kfree() that pointer if it's an error pointer. Second, we
shouldn't crash in __clk_init() in such a situation so there
needs to be some sort of check somewhere.

Oops, done. I have reused the fail_parent_names_copy label as the
less-bad possibility. Probably the error labels should be named after
the target code and not after what the source code does, as per the
latest CodingStyle additions.

BTW, please try and fixup checkpatch warnings.

What were you thinking of specifically? I'm running it with
--max-line-length=106 and the other warnings are in clk-test.c that I
still have to polish when I get some time.

quoted

diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index da4bda8..fac3244 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c

@@ -69,20 +70,22 @@ struct clk *of_clk_get(struct device_node *np, int index)

[...]

quoted

-struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
+static struct clk *__of_clk_get_by_name(struct device_node *np, const char *name)

It would be nice if this returned an already __clk_create_clk()ed
pointer.

quoted

 {
 	struct clk *clk = ERR_PTR(-ENOENT);
 

@@ -119,7 +122,33 @@ struct clk *of_clk_get_by_name(struct device_node *np, const char *name)

[...]

quoted

+struct clk *of_clk_get_by_name(struct device_node *np, const char *name)
+{
+	struct clk *clk = __of_clk_get_by_name(np, name);
+
+	if (!IS_ERR(clk))
+		clk = __clk_create_clk(__clk_get_hw(clk), np->full_name, name);

Because we do it here where we know we're CONFIG_COMMON_CLK=y.

quoted

+
+	return clk;
+}
 EXPORT_SYMBOL(of_clk_get_by_name);
+
+#else /* defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK) */
+
+static struct clk *__of_clk_get_by_name(struct device_node *np, const char *name)
+{
+	return ERR_PTR(-ENOENT);
+}
 #endif
 
 /*

@@ -185,9 +229,13 @@ struct clk *clk_get(struct device *dev, const char *con_id)
 	struct clk *clk;
 
 	if (dev) {
-		clk = of_clk_get_by_name(dev->of_node, con_id);
-		if (!IS_ERR(clk))
+		clk = __of_clk_get_by_name(dev->of_node, con_id);
+		if (!IS_ERR(clk)) {
+#if defined(CONFIG_COMMON_CLK)
+			clk = __clk_create_clk(__clk_get_hw(clk), dev_id, con_id);
+#endif

And we do it here where we could remove the #ifdef.

Yeah, I tried to reduce the ifdefing back then and this is the simplest
I could come up with. The reason for clk_get() to call
__clk_create_clk() directly is that it has more relevant information
with which to tag the per-user clk.

of_clk_get_by_name() has the name of the node but not the dev_id, which
in my testing looked as much less useful when debugging who did what to
a clock.

Thanks,

Tomeu

quoted

 			return clk;
+		}
 		if (PTR_ERR(clk) == -EPROBE_DEFER)
 			return clk;
 	}