[PATCH 1/2] clk: Fix error handling in fixed clock hardware type register fn

[PATCH 1/2] clk: Fix error handling in fixed clock hardware type register fn

[Saravana Kannan]

If memory allocation for the parents array or the parent string fails, then
fail the registration immediately instead of calling clk_register and
hoping it fails there.

Return -ENOMEM on failure.

Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
Cc: Mike Turquette <mturquette@linaro.org>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Jeremy Kerr <jeremy.kerr@canonical.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Arnd Bergman <arnd.bergmann@linaro.org>
Cc: Paul Walmsley <paul@pwsan.com>
Cc: Shawn Guo <shawn.guo@freescale.com>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Jamie Iles <jamie@jamieiles.com>
Cc: Richard Zhao <richard.zhao@linaro.org>
Cc: Saravana Kannan <skannan@codeaurora.org>
Cc: Magnus Damm <magnus.damm@gmail.com>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Linus Walleij <linus.walleij@stericsson.com>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: Amit Kucheria <amit.kucheria@linaro.org>
Cc: Deepak Saxena <dsaxena@linaro.org>
Cc: Grant Likely <grant.likely@secretlab.ca>
---
There are still some memory free issues when clk_register() fails, but I will
fix it when I fixed the other register() fns to return ENOMEM of alloc
failure instead of a NULL.

 drivers/clk/clk-fixed-rate.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/clk-fixed-rate.c b/drivers/clk/clk-fixed-rate.c
index 90c79fb..6423ae9 100644
--- a/drivers/clk/clk-fixed-rate.c
+++ b/drivers/clk/clk-fixed-rate.c
@@ -61,22 +61,26 @@ struct clk *clk_register_fixed_rate(struct device *dev, const char *name,
 		parent_names = kmalloc(sizeof(char *), GFP_KERNEL);
 
 		if (! parent_names)
-			goto out;
+			goto fail_ptr;
 
 		len = sizeof(char) * strlen(parent_name);
 
 		parent_names[0] = kmalloc(len, GFP_KERNEL);
 
 		if (!parent_names[0])
-			goto out;
+			goto fail_str;
 
 		strncpy(parent_names[0], parent_name, len);
 	}
 
-out:
 	return clk_register(dev, name,
 			&clk_fixed_rate_ops, &fixed->hw,
 			parent_names,
 			(parent_name ? 1 : 0),
 			flags);
+fail_str:
+	kfree(parent_names);
+fail_ptr:
+	kfree(fixed);
+	return ERR_PTR(-ENOMEM);
 }
-- 
1.7.8.3

[PATCH 2/2] clk: Move init fields from clk to clk_hw

[Saravana Kannan]

This has a couple of advantages:
* Completely hides struct clk from many clock platform drivers and static
  clock initialization code.
* Simplifies the generic clk_register() function and allows adding optional
  fields in the future without modifying the function signature.
* Allows for simpler static initialization of clocks on all platforms by
  removing the need for forward delcarations.
* Halves the number of symbols added for each static clock initialization.

Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
Cc: Mike Turquette <mturquette@linaro.org>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Jeremy Kerr <jeremy.kerr@canonical.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Arnd Bergman <arnd.bergmann@linaro.org>
Cc: Paul Walmsley <paul@pwsan.com>
Cc: Shawn Guo <shawn.guo@freescale.com>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Jamie Iles <jamie@jamieiles.com>
Cc: Richard Zhao <richard.zhao@linaro.org>
Cc: Saravana Kannan <skannan@codeaurora.org>
Cc: Magnus Damm <magnus.damm@gmail.com>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Linus Walleij <linus.walleij@stericsson.com>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: Amit Kucheria <amit.kucheria@linaro.org>
Cc: Deepak Saxena <dsaxena@linaro.org>
Cc: Grant Likely <grant.likely@secretlab.ca>
---
 drivers/clk/clk-divider.c    |   12 ++-
 drivers/clk/clk-fixed-rate.c |   12 ++-
 drivers/clk/clk-gate.c       |   12 ++-
 drivers/clk/clk-mux.c        |    9 ++-
 drivers/clk/clk.c            |  170 +++++++++++++++++++++---------------------
 include/linux/clk-private.h  |    5 -
 include/linux/clk-provider.h |    9 ++-
 7 files changed, 118 insertions(+), 111 deletions(-)

diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index d5ac6a7..68b69ed 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -184,11 +184,13 @@ struct clk *clk_register_divider(struct device *dev, const char *name,
 			goto out;
 	}
 
-	clk = clk_register(dev, name,
-			&clk_divider_ops, &div->hw,
-			div->parent,
-			(parent_name ? 1 : 0),
-			flags);
+	div->hw.name = name;
+	div->hw.ops = &clk_divider_ops;
+	div->hw.flags = flags;
+	div->hw.parent_names = div->parent;
+	div->hw.num_parents = (parent_name ? 1 : 0);
+
+	clk = clk_register(dev, &div->hw);
 	if (clk)
 		return clk;
 
diff --git a/drivers/clk/clk-fixed-rate.c b/drivers/clk/clk-fixed-rate.c
index 6423ae9..42a0156 100644
--- a/drivers/clk/clk-fixed-rate.c
+++ b/drivers/clk/clk-fixed-rate.c
@@ -73,11 +73,13 @@ struct clk *clk_register_fixed_rate(struct device *dev, const char *name,
 		strncpy(parent_names[0], parent_name, len);
 	}
 
-	return clk_register(dev, name,
-			&clk_fixed_rate_ops, &fixed->hw,
-			parent_names,
-			(parent_name ? 1 : 0),
-			flags);
+	fixed->hw.name = name;
+	fixed->hw.ops = &clk_fixed_rate_ops;
+	fixed->hw.flags = flags;
+	fixed->hw.parent_names = parent_names;
+	fixed->hw.num_parents = (parent_name ? 1 : 0);
+
+	return clk_register(dev, &fixed->hw);
 fail_str:
 	kfree(parent_names);
 fail_ptr:
diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c
index b5902e2..cb515c7 100644
--- a/drivers/clk/clk-gate.c
+++ b/drivers/clk/clk-gate.c
@@ -135,11 +135,13 @@ struct clk *clk_register_gate(struct device *dev, const char *name,
 			goto out;
 	}
 
-	clk = clk_register(dev, name,
-			&clk_gate_ops, &gate->hw,
-			gate->parent,
-			(parent_name ? 1 : 0),
-			flags);
+	gate->hw.name = name;
+	gate->hw.ops = &clk_gate_ops;
+	gate->hw.flags = flags;
+	gate->hw.parent_names = gate->parent;
+	gate->hw.num_parents = (parent_name ? 1 : 0);
+
+	clk = clk_register(dev, &gate->hw);
 	if (clk)
 		return clk;
 out:
diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c
index c71ad1f..1c88230 100644
--- a/drivers/clk/clk-mux.c
+++ b/drivers/clk/clk-mux.c
@@ -111,6 +111,11 @@ struct clk *clk_register_mux(struct device *dev, const char *name,
 	mux->flags = clk_mux_flags;
 	mux->lock = lock;
 
-	return clk_register(dev, name, &clk_mux_ops, &mux->hw,
-			parent_names, num_parents, flags);
+	mux->hw.name = name;
+	mux->hw.ops = &clk_mux_ops;
+	mux->hw.flags = flags;
+	mux->hw.parent_names = parent_names;
+	mux->hw.num_parents = num_parents;
+
+	return clk_register(dev, &mux->hw);
 }
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 9cf6f59..c448809 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -10,6 +10,7 @@
  */
 
 #include <linux/clk-private.h>
+#include <linux/clk-provider.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/spinlock.h>
@@ -44,7 +45,7 @@ static int clk_debug_create_one(struct clk *clk, struct dentry *pdentry)
 		goto out;
 	}
 
-	d = debugfs_create_dir(clk->name, pdentry);
+	d = debugfs_create_dir(clk->hw->name, pdentry);
 	if (!d)
 		goto out;
 
@@ -56,7 +57,7 @@ static int clk_debug_create_one(struct clk *clk, struct dentry *pdentry)
 		goto err_out;
 
 	d = debugfs_create_x32("clk_flags", S_IRUGO, clk->dentry,
-			(u32 *)&clk->flags);
+			(u32 *)&clk->hw->flags);
 	if (!d)
 		goto err_out;
 
@@ -134,7 +135,7 @@ static int clk_debug_register(struct clk *clk)
 	 * safe to add this clk to debugfs
 	 */
 	if (!parent)
-		if (clk->flags & CLK_IS_ROOT)
+		if (clk->hw->flags & CLK_IS_ROOT)
 			pdentry = rootdir;
 		else
 			pdentry = orphandir;
@@ -215,11 +216,11 @@ static void clk_disable_unused_subtree(struct clk *clk)
 	if (clk->enable_count)
 		goto unlock_out;
 
-	if (clk->flags & CLK_IGNORE_UNUSED)
+	if (clk->hw->flags & CLK_IGNORE_UNUSED)
 		goto unlock_out;
 
-	if (__clk_is_enabled(clk) && clk->ops->disable)
-		clk->ops->disable(clk->hw);
+	if (__clk_is_enabled(clk) && clk->hw->ops->disable)
+		clk->hw->ops->disable(clk->hw);
 
 unlock_out:
 	spin_unlock_irqrestore(&enable_lock, flags);
@@ -254,7 +255,7 @@ static inline int clk_disable_unused(struct clk *clk) { return 0; }
 
 inline const char *__clk_get_name(struct clk *clk)
 {
-	return !clk ? NULL : clk->name;
+	return !clk ? NULL : clk->hw->name;
 }
 
 inline struct clk_hw *__clk_get_hw(struct clk *clk)
@@ -264,7 +265,7 @@ inline struct clk_hw *__clk_get_hw(struct clk *clk)
 
 inline u8 __clk_get_num_parents(struct clk *clk)
 {
-	return !clk ? -EINVAL : clk->num_parents;
+	return !clk ? -EINVAL : clk->hw->num_parents;
 }
 
 inline struct clk *__clk_get_parent(struct clk *clk)
@@ -293,7 +294,7 @@ unsigned long __clk_get_rate(struct clk *clk)
 
 	ret = clk->rate;
 
-	if (clk->flags & CLK_IS_ROOT)
+	if (clk->hw->flags & CLK_IS_ROOT)
 		goto out;
 
 	if (!clk->parent)
@@ -305,7 +306,7 @@ out:
 
 inline unsigned long __clk_get_flags(struct clk *clk)
 {
-	return !clk ? -EINVAL : clk->flags;
+	return !clk ? -EINVAL : clk->hw->flags;
 }
 
 int __clk_is_enabled(struct clk *clk)
@@ -319,12 +320,12 @@ int __clk_is_enabled(struct clk *clk)
 	 * .is_enabled is only mandatory for clocks that gate
 	 * fall back to software usage counter if .is_enabled is missing
 	 */
-	if (!clk->ops->is_enabled) {
+	if (!clk->hw->ops->is_enabled) {
 		ret = clk->enable_count ? 1 : 0;
 		goto out;
 	}
 
-	ret = clk->ops->is_enabled(clk->hw);
+	ret = clk->hw->ops->is_enabled(clk->hw);
 out:
 	return ret;
 }
@@ -335,7 +336,7 @@ static struct clk *__clk_lookup_subtree(const char *name, struct clk *clk)
 	struct clk *ret;
 	struct hlist_node *tmp;
 
-	if (!strcmp(clk->name, name))
+	if (!strcmp(clk->hw->name, name))
 		return clk;
 
 	hlist_for_each_entry(child, tmp, &clk->children, child_node) {
@@ -388,8 +389,8 @@ void __clk_unprepare(struct clk *clk)
 
 	WARN_ON(clk->enable_count > 0);
 
-	if (clk->ops->unprepare)
-		clk->ops->unprepare(clk->hw);
+	if (clk->hw->ops->unprepare)
+		clk->hw->ops->unprepare(clk->hw);
 
 	__clk_unprepare(clk->parent);
 }
@@ -425,8 +426,8 @@ int __clk_prepare(struct clk *clk)
 		if (ret)
 			return ret;
 
-		if (clk->ops->prepare) {
-			ret = clk->ops->prepare(clk->hw);
+		if (clk->hw->ops->prepare) {
+			ret = clk->hw->ops->prepare(clk->hw);
 			if (ret) {
 				__clk_unprepare(clk->parent);
 				return ret;
@@ -474,8 +475,8 @@ static void __clk_disable(struct clk *clk)
 	if (--clk->enable_count > 0)
 		return;
 
-	if (clk->ops->disable)
-		clk->ops->disable(clk->hw);
+	if (clk->hw->ops->disable)
+		clk->hw->ops->disable(clk->hw);
 
 	__clk_disable(clk->parent);
 }
@@ -518,8 +519,8 @@ static int __clk_enable(struct clk *clk)
 		if (ret)
 			return ret;
 
-		if (clk->ops->enable) {
-			ret = clk->ops->enable(clk->hw);
+		if (clk->hw->ops->enable) {
+			ret = clk->hw->ops->enable(clk->hw);
 			if (ret) {
 				__clk_disable(clk->parent);
 				return ret;
@@ -589,13 +590,13 @@ unsigned long __clk_round_rate(struct clk *clk, unsigned long rate)
 	if (!clk)
 		return -EINVAL;
 
-	if (!clk->ops->round_rate)
+	if (!clk->hw->ops->round_rate)
 		return clk->rate;
 
-	if (clk->flags & CLK_SET_RATE_PARENT)
-		return clk->ops->round_rate(clk->hw, rate, &unused);
+	if (clk->hw->flags & CLK_SET_RATE_PARENT)
+		return clk->hw->ops->round_rate(clk->hw, rate, &unused);
 	else
-		return clk->ops->round_rate(clk->hw, rate, NULL);
+		return clk->hw->ops->round_rate(clk->hw, rate, NULL);
 }
 
 /**
@@ -681,8 +682,8 @@ static void __clk_recalc_rates(struct clk *clk, unsigned long msg)
 	if (clk->parent)
 		parent_rate = clk->parent->rate;
 
-	if (clk->ops->recalc_rate)
-		clk->rate = clk->ops->recalc_rate(clk->hw, parent_rate);
+	if (clk->hw->ops->recalc_rate)
+		clk->rate = clk->hw->ops->recalc_rate(clk->hw, parent_rate);
 	else
 		clk->rate = parent_rate;
 
@@ -720,8 +721,8 @@ static int __clk_speculate_rates(struct clk *clk, unsigned long parent_rate)
 	unsigned long new_rate;
 	int ret = NOTIFY_DONE;
 
-	if (clk->ops->recalc_rate)
-		new_rate = clk->ops->recalc_rate(clk->hw, parent_rate);
+	if (clk->hw->ops->recalc_rate)
+		new_rate = clk->hw->ops->recalc_rate(clk->hw, parent_rate);
 	else
 		new_rate = parent_rate;
 
@@ -750,8 +751,8 @@ static void clk_calc_subtree(struct clk *clk, unsigned long new_rate)
 	clk->new_rate = new_rate;
 
 	hlist_for_each_entry(child, tmp, &clk->children, child_node) {
-		if (child->ops->recalc_rate)
-			child->new_rate = child->ops->recalc_rate(child->hw, new_rate);
+		if (child->hw->ops->recalc_rate)
+			child->new_rate = child->hw->ops->recalc_rate(child->hw, new_rate);
 		else
 			child->new_rate = new_rate;
 		clk_calc_subtree(child, child->new_rate);
@@ -767,23 +768,24 @@ static struct clk *clk_calc_new_rates(struct clk *clk, unsigned long rate)
 	struct clk *top = clk;
 	unsigned long best_parent_rate = clk->parent->rate;
 	unsigned long new_rate;
+	unsigned long flags = clk->hw->flags;
 
-	if (!clk->ops->round_rate && !(clk->flags & CLK_SET_RATE_PARENT)) {
+	if (!clk->hw->ops->round_rate && !(flags & CLK_SET_RATE_PARENT)) {
 		clk->new_rate = clk->rate;
 		return NULL;
 	}
 
-	if (!clk->ops->round_rate && (clk->flags & CLK_SET_RATE_PARENT)) {
+	if (!clk->hw->ops->round_rate && (flags & CLK_SET_RATE_PARENT)) {
 		top = clk_calc_new_rates(clk->parent, rate);
 		new_rate = clk->new_rate = clk->parent->new_rate;
 
 		goto out;
 	}
 
-	if (clk->flags & CLK_SET_RATE_PARENT)
-		new_rate = clk->ops->round_rate(clk->hw, rate, &best_parent_rate);
+	if (flags & CLK_SET_RATE_PARENT)
+		new_rate = clk->hw->ops->round_rate(clk->hw, rate, &best_parent_rate);
 	else
-		new_rate = clk->ops->round_rate(clk->hw, rate, NULL);
+		new_rate = clk->hw->ops->round_rate(clk->hw, rate, NULL);
 
 	if (best_parent_rate != clk->parent->rate) {
 		top = clk_calc_new_rates(clk->parent, best_parent_rate);
@@ -838,11 +840,11 @@ static void clk_change_rate(struct clk *clk)
 
 	old_rate = clk->rate;
 
-	if (clk->ops->set_rate)
-		clk->ops->set_rate(clk->hw, clk->new_rate);
+	if (clk->hw->ops->set_rate)
+		clk->hw->ops->set_rate(clk->hw, clk->new_rate);
 
-	if (clk->ops->recalc_rate)
-		clk->rate = clk->ops->recalc_rate(clk->hw,
+	if (clk->hw->ops->recalc_rate)
+		clk->rate = clk->hw->ops->recalc_rate(clk->hw,
 				clk->parent->rate);
 	else
 		clk->rate = clk->parent->rate;
@@ -917,7 +919,7 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
 	fail_clk = clk_propagate_rate_change(top, PRE_RATE_CHANGE);
 	if (fail_clk) {
 		pr_warn("%s: failed to set %s rate\n", __func__,
-				fail_clk->name);
+				fail_clk->hw->name);
 		clk_propagate_rate_change(top, ABORT_RATE_CHANGE);
 		ret = -EBUSY;
 		goto out;
@@ -970,18 +972,19 @@ static struct clk *__clk_init_parent(struct clk *clk)
 
 	/* handle the trivial cases */
 
-	if (!clk->num_parents)
+	if (!clk->hw->num_parents)
 		goto out;
 
-	if (clk->num_parents == 1) {
+	if (clk->hw->num_parents == 1) {
 		if (IS_ERR_OR_NULL(clk->parent))
-			ret = clk->parent = __clk_lookup(clk->parent_names[0]);
+			ret = clk->parent =
+				__clk_lookup(clk->hw->parent_names[0]);
 		ret = clk->parent;
 		goto out;
 	}
 
-	if (!clk->ops->get_parent) {
-		WARN(!clk->ops->get_parent,
+	if (!clk->hw->ops->get_parent) {
+		WARN(!clk->hw->ops->get_parent,
 			"%s: multi-parent clocks must implement .get_parent\n",
 			__func__);
 		goto out;
@@ -993,18 +996,18 @@ static struct clk *__clk_init_parent(struct clk *clk)
 	 * clk->parent here; that is done by the calling function
 	 */
 
-	index = clk->ops->get_parent(clk->hw);
+	index = clk->hw->ops->get_parent(clk->hw);
 
 	if (!clk->parents)
 		clk->parents =
-			kmalloc((sizeof(struct clk*) * clk->num_parents),
+			kmalloc((sizeof(struct clk*) * clk->hw->num_parents),
 					GFP_KERNEL);
 
 	if (!clk->parents)
-		ret = __clk_lookup(clk->parent_names[index]);
+		ret = __clk_lookup(clk->hw->parent_names[index]);
 	else if (!clk->parents[index])
 		ret = clk->parents[index] =
-			__clk_lookup(clk->parent_names[index]);
+			__clk_lookup(clk->hw->parent_names[index]);
 	else
 		ret = clk->parents[index];
 
@@ -1039,12 +1042,12 @@ void __clk_reparent(struct clk *clk, struct clk *new_parent)
 		new_parent_d = orphandir;
 
 	d = debugfs_rename(clk->dentry->d_parent, clk->dentry,
-			new_parent_d, clk->name);
+			new_parent_d, clk->hw->name);
 	if (d)
 		clk->dentry = d;
 	else
 		pr_debug("%s: failed to rename debugfs entry for %s\n",
-				__func__, clk->name);
+				__func__, clk->hw->name);
 out:
 #endif
 
@@ -1063,7 +1066,7 @@ static int __clk_set_parent(struct clk *clk, struct clk *parent)
 	old_parent = clk->parent;
 
 	/* find index of new parent clock using cached parent ptrs */
-	for (i = 0; i < clk->num_parents; i++)
+	for (i = 0; i < clk->hw->num_parents; i++)
 		if (clk->parents[i] == parent)
 			break;
 
@@ -1071,16 +1074,17 @@ static int __clk_set_parent(struct clk *clk, struct clk *parent)
 	 * find index of new parent clock using string name comparison
 	 * also try to cache the parent to avoid future calls to __clk_lookup
 	 */
-	if (i == clk->num_parents)
-		for (i = 0; i < clk->num_parents; i++)
-			if (!strcmp(clk->parent_names[i], parent->name)) {
-				clk->parents[i] = __clk_lookup(parent->name);
+	if (i == clk->hw->num_parents)
+		for (i = 0; i < clk->hw->num_parents; i++)
+			if (!strcmp(clk->hw->parent_names[i],
+						parent->hw->name)) {
+				clk->parents[i] = __clk_lookup(parent->hw->name);
 				break;
 			}
 
-	if (i == clk->num_parents) {
+	if (i == clk->hw->num_parents) {
 		pr_debug("%s: clock %s is not a possible parent of clock %s\n",
-				__func__, parent->name, clk->name);
+				__func__, parent->hw->name, clk->hw->name);
 		goto out;
 	}
 
@@ -1095,7 +1099,7 @@ static int __clk_set_parent(struct clk *clk, struct clk *parent)
 	spin_unlock_irqrestore(&enable_lock, flags);
 
 	/* change clock input source */
-	ret = clk->ops->set_parent(clk->hw, i);
+	ret = clk->hw->ops->set_parent(clk->hw, i);
 
 	/* clean up old prepare and enable */
 	spin_lock_irqsave(&enable_lock, flags);
@@ -1126,10 +1130,10 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
 {
 	int ret = 0;
 
-	if (!clk || !clk->ops)
+	if (!clk || !clk->hw->ops)
 		return -EINVAL;
 
-	if (!clk->ops->set_parent)
+	if (!clk->hw->ops->set_parent)
 		return -ENOSYS;
 
 	/* prevent racing with updates to the clock topology */
@@ -1147,7 +1151,7 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
 		goto out;
 
 	/* only re-parent if the clock is not in use */
-	if ((clk->flags & CLK_SET_PARENT_GATE) && clk->prepare_count)
+	if ((clk->hw->flags & CLK_SET_PARENT_GATE) && clk->prepare_count)
 		ret = -EBUSY;
 	else
 		ret = __clk_set_parent(clk, parent);
@@ -1207,14 +1211,14 @@ void __clk_init(struct device *dev, struct clk *clk)
 	mutex_lock(&prepare_lock);
 
 	/* check to see if a clock with this name is already registered */
-	if (__clk_lookup(clk->name))
+	if (__clk_lookup(clk->hw->name))
 		goto out;
 
 	/* throw a WARN if any entries in parent_names are NULL */
-	for (i = 0; i < clk->num_parents; i++)
-		WARN(!clk->parent_names[i],
+	for (i = 0; i < clk->hw->num_parents; i++)
+		WARN(!clk->hw->parent_names[i],
 				"%s: invalid NULL in %s's .parent_names\n",
-				__func__, clk->name);
+				__func__, clk->hw->name);
 
 	/*
 	 * Allocate an array of struct clk *'s to avoid unnecessary string
@@ -1226,8 +1230,8 @@ void __clk_init(struct device *dev, struct clk *clk)
 	 * If clk->parents is not NULL we skip this entire block.  This allows
 	 * for clock drivers to statically initialize clk->parents.
 	 */
-	if (clk->num_parents && !clk->parents) {
-		clk->parents = kmalloc((sizeof(struct clk*) * clk->num_parents),
+	if (clk->hw->num_parents && !clk->parents) {
+		clk->parents = kmalloc((sizeof(struct clk*) * clk->hw->num_parents),
 				GFP_KERNEL);
 		/*
 		 * __clk_lookup returns NULL for parents that have not been
@@ -1236,9 +1240,9 @@ void __clk_init(struct device *dev, struct clk *clk)
 		 * missing parents later on.
 		 */
 		if (clk->parents)
-			for (i = 0; i < clk->num_parents; i++)
+			for (i = 0; i < clk->hw->num_parents; i++)
 				clk->parents[i] =
-					__clk_lookup(clk->parent_names[i]);
+					__clk_lookup(clk->hw->parent_names[i]);
 	}
 
 	clk->parent = __clk_init_parent(clk);
@@ -1256,7 +1260,7 @@ void __clk_init(struct device *dev, struct clk *clk)
 	if (clk->parent)
 		hlist_add_head(&clk->child_node,
 				&clk->parent->children);
-	else if (clk->flags & CLK_IS_ROOT)
+	else if (clk->hw->flags & CLK_IS_ROOT)
 		hlist_add_head(&clk->child_node, &clk_root_list);
 	else
 		hlist_add_head(&clk->child_node, &clk_orphan_list);
@@ -1267,8 +1271,8 @@ void __clk_init(struct device *dev, struct clk *clk)
 	 * parent's rate.  If a clock doesn't have a parent (or is orphaned)
 	 * then rate is set to zero.
 	 */
-	if (clk->ops->recalc_rate)
-		clk->rate = clk->ops->recalc_rate(clk->hw,
+	if (clk->hw->ops->recalc_rate)
+		clk->rate = clk->hw->ops->recalc_rate(clk->hw,
 				__clk_get_rate(clk->parent));
 	else if (clk->parent)
 		clk->rate = clk->parent->rate;
@@ -1280,8 +1284,9 @@ void __clk_init(struct device *dev, struct clk *clk)
 	 * this clock
 	 */
 	hlist_for_each_entry_safe(orphan, tmp, tmp2, &clk_orphan_list, child_node)
-		for (i = 0; i < orphan->num_parents; i++)
-			if (!strcmp(clk->name, orphan->parent_names[i])) {
+		for (i = 0; i < orphan->hw->num_parents; i++)
+			if (!strcmp(clk->hw->name,
+						orphan->hw->parent_names[i])) {
 				__clk_reparent(orphan, clk);
 				break;
 			}
@@ -1294,8 +1299,8 @@ void __clk_init(struct device *dev, struct clk *clk)
 	 * Please consider other ways of solving initialization problems before
 	 * using this callback, as it's use is discouraged.
 	 */
-	if (clk->ops->init)
-		clk->ops->init(clk->hw);
+	if (clk->hw->ops->init)
+		clk->hw->ops->init(clk->hw);
 
 	clk_debug_register(clk);
 
@@ -1320,9 +1325,7 @@ out:
  * cannot be dereferenced by driver code but may be used in conjuction with the
  * rest of the clock API.
  */
-struct clk *clk_register(struct device *dev, const char *name,
-		const struct clk_ops *ops, struct clk_hw *hw,
-		char **parent_names, u8 num_parents, unsigned long flags)
+struct clk *clk_register(struct device *dev, struct clk_hw *hw)
 {
 	struct clk *clk;
 
@@ -1330,12 +1333,7 @@ struct clk *clk_register(struct device *dev, const char *name,
 	if (!clk)
 		return NULL;
 
-	clk->name = name;
-	clk->ops = ops;
 	clk->hw = hw;
-	clk->flags = flags;
-	clk->parent_names = parent_names;
-	clk->num_parents = num_parents;
 	hw->clk = clk;
 
 	__clk_init(dev, clk);
diff --git a/include/linux/clk-private.h b/include/linux/clk-private.h
index 5e4312b..f70860d 100644
--- a/include/linux/clk-private.h
+++ b/include/linux/clk-private.h
@@ -26,16 +26,11 @@
 #ifdef CONFIG_COMMON_CLK
 
 struct clk {
-	const char		*name;
-	const struct clk_ops	*ops;
 	struct clk_hw		*hw;
 	struct clk		*parent;
-	char			**parent_names;
 	struct clk		**parents;
-	u8			num_parents;
 	unsigned long		rate;
 	unsigned long		new_rate;
-	unsigned long		flags;
 	unsigned int		enable_count;
 	unsigned int		prepare_count;
 	struct hlist_head	children;
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 5508897..f032e49 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -26,6 +26,11 @@
  */
 struct clk_hw {
 	struct clk *clk;
+	const char		*name;
+	const struct clk_ops	*ops;
+	char			**parent_names;
+	u8			num_parents;
+	unsigned long		flags;
 };
 
 /*
@@ -272,9 +277,7 @@ struct clk *clk_register_mux(struct device *dev, const char *name,
  * cannot be dereferenced by driver code but may be used in conjuction with the
  * rest of the clock API.
  */
-struct clk *clk_register(struct device *dev, const char *name,
-		const struct clk_ops *ops, struct clk_hw *hw,
-		char **parent_names, u8 num_parents, unsigned long flags);
+struct clk *clk_register(struct device *dev, struct clk_hw *hw);
 
 /* helper functions */
 const char *__clk_get_name(struct clk *clk);
-- 
1.7.8.3

Re: [PATCH 1/2] clk: Fix error handling in fixed clock hardware type register fn

[Sascha Hauer]

On Mon, Mar 19, 2012 at 08:38:25PM -0700, Saravana Kannan wrote:
> If memory allocation for the parents array or the parent string fails, then
> fail the registration immediately instead of calling clk_register and
> hoping it fails there.
> 
> Return -ENOMEM on failure.
> 
> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> Cc: Mike Turquette <mturquette@linaro.org>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Jeremy Kerr <jeremy.kerr@canonical.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Arnd Bergman <arnd.bergmann@linaro.org>
> Cc: Paul Walmsley <paul@pwsan.com>
> Cc: Shawn Guo <shawn.guo@freescale.com>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Jamie Iles <jamie@jamieiles.com>
> Cc: Richard Zhao <richard.zhao@linaro.org>
> Cc: Saravana Kannan <skannan@codeaurora.org>
> Cc: Magnus Damm <magnus.damm@gmail.com>
> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Cc: Linus Walleij <linus.walleij@stericsson.com>
> Cc: Stephen Boyd <sboyd@codeaurora.org>
> Cc: Amit Kucheria <amit.kucheria@linaro.org>
> Cc: Deepak Saxena <dsaxena@linaro.org>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> ---
> There are still some memory free issues when clk_register() fails, but I will
> fix it when I fixed the other register() fns to return ENOMEM of alloc
> failure instead of a NULL.
> 
>  drivers/clk/clk-fixed-rate.c |   10 +++++++---
>  1 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/clk-fixed-rate.c b/drivers/clk/clk-fixed-rate.c
> index 90c79fb..6423ae9 100644
> --- a/drivers/clk/clk-fixed-rate.c
> +++ b/drivers/clk/clk-fixed-rate.c
> @@ -61,22 +61,26 @@ struct clk *clk_register_fixed_rate(struct device *dev, const char *name,
>  		parent_names = kmalloc(sizeof(char *), GFP_KERNEL);
>  
>  		if (! parent_names)
> -			goto out;
> +			goto fail_ptr;
>  
>  		len = sizeof(char) * strlen(parent_name);
>  
>  		parent_names[0] = kmalloc(len, GFP_KERNEL);
>  
>  		if (!parent_names[0])
> -			goto out;
> +			goto fail_str;
>  
>  		strncpy(parent_names[0], parent_name, len);
>  	}

It's easier to add a char *parent to struct clk_fixed and pass it to
clk_register with &fixed->parent. This saves you a kmalloc call and
makes the error path simpler. It's the same way already done in the
divider.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/2] clk: Move init fields from clk to clk_hw

[Shawn Guo]

On Mon, Mar 19, 2012 at 08:38:26PM -0700, Saravana Kannan wrote:
> This has a couple of advantages:
> * Completely hides struct clk from many clock platform drivers and static
>   clock initialization code.
> * Simplifies the generic clk_register() function and allows adding optional
>   fields in the future without modifying the function signature.
> * Allows for simpler static initialization of clocks on all platforms by
>   removing the need for forward delcarations.
> * Halves the number of symbols added for each static clock initialization.
> 
> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>

I agree this is a reasonable move.  But while you simplify the interface
of clk_register(), why not making a further step to simplify the
following interfaces simple too?

struct clk *clk_register_fixed_rate(struct device *dev, const char *name,
                const char *parent_name, unsigned long flags,
                unsigned long fixed_rate);
struct clk *clk_register_gate(struct device *dev, const char *name,
                const char *parent_name, unsigned long flags,
                void __iomem *reg, u8 bit_idx,
                u8 clk_gate_flags, spinlock_t *lock);
struct clk *clk_register_divider(struct device *dev, const char *name,
                const char *parent_name, unsigned long flags,
                void __iomem *reg, u8 shift, u8 width,
                u8 clk_divider_flags, spinlock_t *lock);
struct clk *clk_register_mux(struct device *dev, const char *name,
                char **parent_names, u8 num_parents, unsigned long flags,
                void __iomem *reg, u8 shift, u8 width,
                u8 clk_mux_flags, spinlock_t *lock);

Otherwise,

Acked-by: Shawn Guo <shawn.guo@linaro.org>

-- 
Regards,
Shawn

Re: [PATCH 1/2] clk: Fix error handling in fixed clock hardware type register fn

[Saravana Kannan]

On Tue, March 20, 2012 12:19 am, Sascha Hauer wrote:
> On Mon, Mar 19, 2012 at 08:38:25PM -0700, Saravana Kannan wrote:
>> If memory allocation for the parents array or the parent string fails,
>> then
>> fail the registration immediately instead of calling clk_register and
>> hoping it fails there.
>>
>> Return -ENOMEM on failure.
>>
>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>> Cc: Mike Turquette <mturquette@linaro.org>
>> Cc: Andrew Lunn <andrew@lunn.ch>
>> Cc: Rob Herring <rob.herring@calxeda.com>
>> Cc: Russell King <linux@arm.linux.org.uk>
>> Cc: Jeremy Kerr <jeremy.kerr@canonical.com>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Arnd Bergman <arnd.bergmann@linaro.org>
>> Cc: Paul Walmsley <paul@pwsan.com>
>> Cc: Shawn Guo <shawn.guo@freescale.com>
>> Cc: Sascha Hauer <s.hauer@pengutronix.de>
>> Cc: Jamie Iles <jamie@jamieiles.com>
>> Cc: Richard Zhao <richard.zhao@linaro.org>
>> Cc: Saravana Kannan <skannan@codeaurora.org>
>> Cc: Magnus Damm <magnus.damm@gmail.com>
>> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
>> Cc: Linus Walleij <linus.walleij@stericsson.com>
>> Cc: Stephen Boyd <sboyd@codeaurora.org>
>> Cc: Amit Kucheria <amit.kucheria@linaro.org>
>> Cc: Deepak Saxena <dsaxena@linaro.org>
>> Cc: Grant Likely <grant.likely@secretlab.ca>
>> ---
>> There are still some memory free issues when clk_register() fails, but I
>> will
>> fix it when I fixed the other register() fns to return ENOMEM of alloc
>> failure instead of a NULL.
>>
>>  drivers/clk/clk-fixed-rate.c |   10 +++++++---
>>  1 files changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/clk/clk-fixed-rate.c b/drivers/clk/clk-fixed-rate.c
>> index 90c79fb..6423ae9 100644
>> --- a/drivers/clk/clk-fixed-rate.c
>> +++ b/drivers/clk/clk-fixed-rate.c
>> @@ -61,22 +61,26 @@ struct clk *clk_register_fixed_rate(struct device
>> *dev, const char *name,
>>  		parent_names = kmalloc(sizeof(char *), GFP_KERNEL);
>>
>>  		if (! parent_names)
>> -			goto out;
>> +			goto fail_ptr;
>>
>>  		len = sizeof(char) * strlen(parent_name);
>>
>>  		parent_names[0] = kmalloc(len, GFP_KERNEL);
>>
>>  		if (!parent_names[0])
>> -			goto out;
>> +			goto fail_str;
>>
>>  		strncpy(parent_names[0], parent_name, len);
>>  	}
>
> It's easier to add a char *parent to struct clk_fixed and pass it to
> clk_register with &fixed->parent. This saves you a kmalloc call and
> makes the error path simpler. It's the same way already done in the
> divider.

I thought about that since I saw the same was done for gated and divider
(I think). Here is my guess at Mike's reasoning for this:

Gated and divider clocks have to have a parent. There's nothing to gate
otherwise. But fixed rate clocks might not have a parent. It could be XO's
or PLLs running off of always on XOs not controlled by the SoC. So, it's
arguable to not have a parent. I don't have a strong opinion on this --
since Mike took the time to write it, it left it to his subjective
preference.

I sent this patch first since it was around the place I was cleaning up. I
didn't want to actually just shuffle around a bug. As I mentioned, this
patch still leaves a bug open -- what if clk_register() fails. I plan to
fix that once my two patches are picked up (hopefully).

Thanks,
Saravana

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 2/2] clk: Move init fields from clk to clk_hw

[Saravana Kannan]

On Tue, March 20, 2012 12:20 am, Shawn Guo wrote:
> On Mon, Mar 19, 2012 at 08:38:26PM -0700, Saravana Kannan wrote:
>> This has a couple of advantages:
>> * Completely hides struct clk from many clock platform drivers and
>> static
>>   clock initialization code.
>> * Simplifies the generic clk_register() function and allows adding
>> optional
>>   fields in the future without modifying the function signature.
>> * Allows for simpler static initialization of clocks on all platforms by
>>   removing the need for forward delcarations.
>> * Halves the number of symbols added for each static clock
>> initialization.
>>
>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>
> I agree this is a reasonable move.  But while you simplify the interface
> of clk_register(), why not making a further step to simplify the
> following interfaces simple too?
>
> struct clk *clk_register_fixed_rate(struct device *dev, const char *name,
>                 const char *parent_name, unsigned long flags,
>                 unsigned long fixed_rate);
> struct clk *clk_register_gate(struct device *dev, const char *name,
>                 const char *parent_name, unsigned long flags,
>                 void __iomem *reg, u8 bit_idx,
>                 u8 clk_gate_flags, spinlock_t *lock);
> struct clk *clk_register_divider(struct device *dev, const char *name,
>                 const char *parent_name, unsigned long flags,
>                 void __iomem *reg, u8 shift, u8 width,
>                 u8 clk_divider_flags, spinlock_t *lock);
> struct clk *clk_register_mux(struct device *dev, const char *name,
>                 char **parent_names, u8 num_parents, unsigned long flags,
>                 void __iomem *reg, u8 shift, u8 width,
>                 u8 clk_mux_flags, spinlock_t *lock);

If you simplify those functions further. They would just become
clk_register(). I'm not sure I see a value in them in at that point or
even in their current form. But if others see (I'm guessing since they
acked or didn't nack it), I'm not going to ask to remove them. If everyone
agrees that we should just remove them, I would be glad to.

It's arguable that these functions for the common hardware types saves the
need to deal with the kalloc in every platform driver. But it's not clear
to me where they would get these parameters in the first place. Most
likely form some sort of static array. At which point, it might as well be
a static array of pointers to clk_gated.hw, clk_fixed_rate.hw, etc instead
of a platform specific  struct to hold these initializers.

Btw, I need to fix the macros too. Or may be even delete them. Will see
the overall response before I send out v2 to do that.

>
> Otherwise,
>
> Acked-by: Shawn Guo <shawn.guo@linaro.org>
Thanks

-Saravana

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 2/2] clk: Move init fields from clk to clk_hw

[Shawn Guo]

On Tue, Mar 20, 2012 at 12:54:55AM -0700, Saravana Kannan wrote:
> 
> On Tue, March 20, 2012 12:20 am, Shawn Guo wrote:
...
> > struct clk *clk_register_fixed_rate(struct device *dev, const char *name,
> >                 const char *parent_name, unsigned long flags,
> >                 unsigned long fixed_rate);
> > struct clk *clk_register_gate(struct device *dev, const char *name,
> >                 const char *parent_name, unsigned long flags,
> >                 void __iomem *reg, u8 bit_idx,
> >                 u8 clk_gate_flags, spinlock_t *lock);
> > struct clk *clk_register_divider(struct device *dev, const char *name,
> >                 const char *parent_name, unsigned long flags,
> >                 void __iomem *reg, u8 shift, u8 width,
> >                 u8 clk_divider_flags, spinlock_t *lock);
> > struct clk *clk_register_mux(struct device *dev, const char *name,
> >                 char **parent_names, u8 num_parents, unsigned long flags,
> >                 void __iomem *reg, u8 shift, u8 width,
> >                 u8 clk_mux_flags, spinlock_t *lock);
> 
> If you simplify those functions further. They would just become
> clk_register(). I'm not sure I see a value in them in at that point or
> even in their current form. But if others see (I'm guessing since they
> acked or didn't nack it), I'm not going to ask to remove them. If everyone
> agrees that we should just remove them, I would be glad to.
> 
> It's arguable that these functions for the common hardware types saves the
> need to deal with the kalloc in every platform driver. But it's not clear
> to me where they would get these parameters in the first place. Most
> likely form some sort of static array. At which point, it might as well be
> a static array of pointers to clk_gated.hw, clk_fixed_rate.hw, etc instead
> of a platform specific  struct to hold these initializers.

I share the save view on this, so would vote to remove these
registration functions for basic clks completely.

Regards,
Shawn

Re: [PATCH 2/2] clk: Move init fields from clk to clk_hw

[Sascha Hauer]

On Tue, Mar 20, 2012 at 12:54:55AM -0700, Saravana Kannan wrote:
> 
> On Tue, March 20, 2012 12:20 am, Shawn Guo wrote:
> > On Mon, Mar 19, 2012 at 08:38:26PM -0700, Saravana Kannan wrote:
> >> This has a couple of advantages:
> >> * Completely hides struct clk from many clock platform drivers and
> >> static
> >>   clock initialization code.
> >> * Simplifies the generic clk_register() function and allows adding
> >> optional
> >>   fields in the future without modifying the function signature.
> >> * Allows for simpler static initialization of clocks on all platforms by
> >>   removing the need for forward delcarations.
> >> * Halves the number of symbols added for each static clock
> >> initialization.
> >>
> >> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
> >
> > I agree this is a reasonable move.  But while you simplify the interface
> > of clk_register(), why not making a further step to simplify the
> > following interfaces simple too?
> >
> > struct clk *clk_register_fixed_rate(struct device *dev, const char *name,
> >                 const char *parent_name, unsigned long flags,
> >                 unsigned long fixed_rate);
> > struct clk *clk_register_gate(struct device *dev, const char *name,
> >                 const char *parent_name, unsigned long flags,
> >                 void __iomem *reg, u8 bit_idx,
> >                 u8 clk_gate_flags, spinlock_t *lock);
> > struct clk *clk_register_divider(struct device *dev, const char *name,
> >                 const char *parent_name, unsigned long flags,
> >                 void __iomem *reg, u8 shift, u8 width,
> >                 u8 clk_divider_flags, spinlock_t *lock);
> > struct clk *clk_register_mux(struct device *dev, const char *name,
> >                 char **parent_names, u8 num_parents, unsigned long flags,
> >                 void __iomem *reg, u8 shift, u8 width,
> >                 u8 clk_mux_flags, spinlock_t *lock);
> 
> If you simplify those functions further. They would just become
> clk_register(). I'm not sure I see a value in them in at that point or
> even in their current form. But if others see (I'm guessing since they
> acked or didn't nack it), I'm not going to ask to remove them. If everyone
> agrees that we should just remove them, I would be glad to.
> 
> It's arguable that these functions for the common hardware types saves the
> need to deal with the kalloc in every platform driver. But it's not clear
> to me where they would get these parameters in the first place. Most
> likely form some sort of static array. At which point, it might as well be
> a static array of pointers to clk_gated.hw, clk_fixed_rate.hw, etc instead
> of a platform specific  struct to hold these initializers.

I am using these functions and don't need a static array, I just call
the functions with the desired parameters.

Overall the clock framework was written in a way that we have to expose
as little information about the internally used structs as necessary. It
seems your patches are pulling in the opposite direction now.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/2] clk: Move init fields from clk to clk_hw

[Saravana Kannan]

On Tue, March 20, 2012 2:40 am, Sascha Hauer wrote:
> On Tue, Mar 20, 2012 at 12:54:55AM -0700, Saravana Kannan wrote:
>>
>> On Tue, March 20, 2012 12:20 am, Shawn Guo wrote:
>> > On Mon, Mar 19, 2012 at 08:38:26PM -0700, Saravana Kannan wrote:
>> >> This has a couple of advantages:
>> >> * Completely hides struct clk from many clock platform drivers and
>> >> static
>> >>   clock initialization code.
>> >> * Simplifies the generic clk_register() function and allows adding
>> >> optional
>> >>   fields in the future without modifying the function signature.
>> >> * Allows for simpler static initialization of clocks on all platforms
>> by
>> >>   removing the need for forward delcarations.
>> >> * Halves the number of symbols added for each static clock
>> >> initialization.
>> >>
>> >> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>> >
>> > I agree this is a reasonable move.  But while you simplify the
>> interface
>> > of clk_register(), why not making a further step to simplify the
>> > following interfaces simple too?
>> >
>> > struct clk *clk_register_fixed_rate(struct device *dev, const char
>> *name,
>> >                 const char *parent_name, unsigned long flags,
>> >                 unsigned long fixed_rate);
>> > struct clk *clk_register_gate(struct device *dev, const char *name,
>> >                 const char *parent_name, unsigned long flags,
>> >                 void __iomem *reg, u8 bit_idx,
>> >                 u8 clk_gate_flags, spinlock_t *lock);
>> > struct clk *clk_register_divider(struct device *dev, const char *name,
>> >                 const char *parent_name, unsigned long flags,
>> >                 void __iomem *reg, u8 shift, u8 width,
>> >                 u8 clk_divider_flags, spinlock_t *lock);
>> > struct clk *clk_register_mux(struct device *dev, const char *name,
>> >                 char **parent_names, u8 num_parents, unsigned long
>> flags,
>> >                 void __iomem *reg, u8 shift, u8 width,
>> >                 u8 clk_mux_flags, spinlock_t *lock);
>>
>> If you simplify those functions further. They would just become
>> clk_register(). I'm not sure I see a value in them in at that point or
>> even in their current form. But if others see (I'm guessing since they
>> acked or didn't nack it), I'm not going to ask to remove them. If
>> everyone
>> agrees that we should just remove them, I would be glad to.
>>
>> It's arguable that these functions for the common hardware types saves
>> the
>> need to deal with the kalloc in every platform driver. But it's not
>> clear
>> to me where they would get these parameters in the first place. Most
>> likely form some sort of static array. At which point, it might as well
>> be
>> a static array of pointers to clk_gated.hw, clk_fixed_rate.hw, etc
>> instead
>> of a platform specific  struct to hold these initializers.
>
> I am using these functions and don't need a static array, I just call
> the functions with the desired parameters.

Sure, then let's leave it in. Curious, where do you get the desired
parameters from? Is it static date in code or is it from DT? You somehow
probe it?

> Overall the clock framework was written in a way that we have to expose
> as little information about the internally used structs as necessary. It
> seems your patches are pulling in the opposite direction now.

I'm not exposing anything that you don't already pass from the platform
driver. Also, you realize that this is very similar to what you suggested
with clk_initializer right? If there is a strong push, we can make a copy
of these inside the struct clk, but for these few init fields I don't see
a point (see earlier email).

-Saravana


-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 2/2] clk: Move init fields from clk to clk_hw

[Shawn Guo]

On Tue, Mar 20, 2012 at 10:40:31AM +0100, Sascha Hauer wrote:
...
> I am using these functions and don't need a static array, I just call
> the functions with the desired parameters.
> 
With this patch getting in, you do not have to use them then.  I feel
a static array will be much more readable than a long list of function
calls with a long list of hardcoded arguments to each.

-- 
Regards,
Shawn

Re: [PATCH 2/2] clk: Move init fields from clk to clk_hw

[Sascha Hauer]

On Tue, Mar 20, 2012 at 10:18:14PM +0800, Shawn Guo wrote:
> On Tue, Mar 20, 2012 at 10:40:31AM +0100, Sascha Hauer wrote:
> ...
> > I am using these functions and don't need a static array, I just call
> > the functions with the desired parameters.
> > 
> With this patch getting in, you do not have to use them then.  I feel
> a static array will be much more readable than a long list of function
> calls with a long list of hardcoded arguments to each.

I'm also not a fan of long argument lists; a divider like defined in my
tree takes 5 arguments, a gate 4 and a mux 6. While 6 is already at the
border I think it's still acceptable.

What I like in terms of readability is one line per clock which makes
for quite short clock files.

So when we use structs to initialize the clocks we'll probably have
something like this:

static struct clk_divider somediv = {
	.reg = CCM_BASE + 0x14,
	.width = 3,
	.shift = 17,
	.lock = &ccm_lock,
	.hw.parent = "someotherdiv",
	.hw.flags = CLK_SET_RATE_PARENT,
};

This will make a 4000 line file out of a 500 line file. Now when for
some reason struct clk_divider changes we end with big patches. If the
clk core gets a new fancy CLK_ flag which we want to have then again
we end up with big patches. Then there's also the possibility that
someone finds out that .lock and .hw.flags are common to all dividers
and comes up with a #define DEFINE_CLK_DIVIDER again to share common
fields.

All this can be solved when we introduce a small wrapper function and
use it in the clock files:

static inline struct clk *imx_clk_divider(const char *name, const char *parent,
		void __iomem *reg, u8 shift, u8 width)
{
	return clk_register_divider(NULL, name, parent, CLK_SET_RATE_PARENT,
			reg, shift, width, 0, &imx_ccm_lock);
}

It decouples us from the structs used by the clock framework, we can
add our preferred flags and still can share common fields like the lock.

While this was not the intention when I first converted from struct
initializers to function initializers I am confident that it will make
a good job.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/2] clk: Move init fields from clk to clk_hw

[Sascha Hauer]

On Tue, Mar 20, 2012 at 03:17:10AM -0700, Saravana Kannan wrote:
> 
> On Tue, March 20, 2012 2:40 am, Sascha Hauer wrote:
> > On Tue, Mar 20, 2012 at 12:54:55AM -0700, Saravana Kannan wrote:
> >>
> > I am using these functions and don't need a static array, I just call
> > the functions with the desired parameters.
> 
> Sure, then let's leave it in. Curious, where do you get the desired
> parameters from? Is it static date in code or is it from DT? You somehow
> probe it?

It's not from DT. See this thread:

http://www.spinics.net/lists/arm-kernel/msg165839.html

> 
> > Overall the clock framework was written in a way that we have to expose
> > as little information about the internally used structs as necessary. It
> > seems your patches are pulling in the opposite direction now.
> 
> I'm not exposing anything that you don't already pass from the platform
> driver. Also, you realize that this is very similar to what you suggested
> with clk_initializer right? If there is a strong push, we can make a copy
> of these inside the struct clk, but for these few init fields I don't see
> a point (see earlier email).

The difference is that a struct clk_initializer is only used to
initialize a clock and not actively used by the clock framework. But as
you already mentioned using a copy inside the clock framework has the
same effect.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/2] clk: Move init fields from clk to clk_hw

[Saravana Kannan]

On 03/20/2012 11:10 AM, Sascha Hauer wrote:
> On Tue, Mar 20, 2012 at 10:18:14PM +0800, Shawn Guo wrote:
>> On Tue, Mar 20, 2012 at 10:40:31AM +0100, Sascha Hauer wrote:
>> ...
>>> I am using these functions and don't need a static array, I just call
>>> the functions with the desired parameters.
>>>
>> With this patch getting in, you do not have to use them then.  I feel
>> a static array will be much more readable than a long list of function
>> calls with a long list of hardcoded arguments to each.
>
> I'm also not a fan of long argument lists; a divider like defined in my
> tree takes 5 arguments, a gate 4 and a mux 6. While 6 is already at the
> border I think it's still acceptable.
>
> What I like in terms of readability is one line per clock which makes
> for quite short clock files.

It certainly makes for short clock files, but it's definitely less 
readable that the expanded struct. For the original author the "one line 
per clock" looks readable since they wrote it. But for someone looking 
at the code to modify it, the expanded one would be much easier to read. 
Also, you can always declare your own macro if you really want to follow 
the one line approach.

> So when we use structs to initialize the clocks we'll probably have
> something like this:
>
> static struct clk_divider somediv = {
> 	.reg = CCM_BASE + 0x14,
> 	.width = 3,
> 	.shift = 17,
> 	.lock =&ccm_lock,
> 	.hw.parent = "someotherdiv",
> 	.hw.flags = CLK_SET_RATE_PARENT,
> };

Taken from your patches:

imx_clk_mux("spll_sel", CCM_CSCR, 17, 1, spll_sel_clks, 
ARRAY_SIZE(spll_sel_clks));

Compare the struct to the one line call. Now tell me, what does "1" 
represent? No clue. But in the struct, I can immediately tell what each 
one of the parameters are.

Anyway, my patch certainly isn't forcing you to use multiple lines. So, 
hopefully this won't be a point of contention.

> This will make a 4000 line file out of a 500 line file. Now when for
> some reason struct clk_divider changes we end with big patches. If the
> clk core gets a new fancy CLK_ flag which we want to have then again
> we end up with big patches. Then there's also the possibility that
> someone finds out that .lock and .hw.flags are common to all dividers
> and comes up with a #define DEFINE_CLK_DIVIDER again to share common
> fields.

This patch won't prevent you from doing any of that. You can still 
create macros for that (there are already one for that). Also, what you 
are pointing out is a bigger problem for the current clk_register() 
function since you might have to change the no. of params of all the 
callers even if a new field is optional.

> All this can be solved when we introduce a small wrapper function and
> use it in the clock files:
>
> static inline struct clk *imx_clk_divider(const char *name, const char *parent,
> 		void __iomem *reg, u8 shift, u8 width)
> {
> 	return clk_register_divider(NULL, name, parent, CLK_SET_RATE_PARENT,
> 			reg, shift, width, 0,&imx_ccm_lock);
> }
>
> It decouples us from the structs used by the clock framework, we can
> add our preferred flags and still can share common fields like the lock.
>
> While this was not the intention when I first converted from struct
> initializers to function initializers I am confident that it will make
> a good job.

Now I'm confused -- it's not clear if you are leaning towards my patch 
or away from it?

I also think we are talking about issues orthogonal to my patch. I don't 
think my patch prevents you from doing any of this as long as you are up 
for wrapper fns or macros.

-Saravana

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 2/2] clk: Move init fields from clk to clk_hw

[Saravana Kannan]

On 03/20/2012 11:14 AM, Sascha Hauer wrote:
> On Tue, Mar 20, 2012 at 03:17:10AM -0700, Saravana Kannan wrote:
>>
>> On Tue, March 20, 2012 2:40 am, Sascha Hauer wrote:
>>> On Tue, Mar 20, 2012 at 12:54:55AM -0700, Saravana Kannan wrote:
>>>>
>>> I am using these functions and don't need a static array, I just call
>>> the functions with the desired parameters.
>>
>> Sure, then let's leave it in. Curious, where do you get the desired
>> parameters from? Is it static date in code or is it from DT? You somehow
>> probe it?
>
> It's not from DT. See this thread:
>
> http://www.spinics.net/lists/arm-kernel/msg165839.html

Ah, I see. That's a lot of functions calls. I think it would be much 
more efficient if you just have an array and loop over it. With my 
patch, you can just call a single register function for all these clocks.

>>
>>> Overall the clock framework was written in a way that we have to expose
>>> as little information about the internally used structs as necessary. It
>>> seems your patches are pulling in the opposite direction now.
>>
>> I'm not exposing anything that you don't already pass from the platform
>> driver. Also, you realize that this is very similar to what you suggested
>> with clk_initializer right? If there is a strong push, we can make a copy
>> of these inside the struct clk, but for these few init fields I don't see
>> a point (see earlier email).
>
> The difference is that a struct clk_initializer is only used to
> initialize a clock and not actively used by the clock framework. But as
> you already mentioned using a copy inside the clock framework has the
> same effect.

My opinion on this is to follow a wait and watch approach on the "flags" 
field. I really can't think of a safe way anyone can misuse it. If 
people start doing that, we can just do the copy and let them deal with 
their broken code. Until that happens, I don't want to waste time/space 
by copying it. The rest of the fields aren't actively used by the common 
clock code after init.

Thanks,
Saravana

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 2/2] clk: Move init fields from clk to clk_hw

[Sascha Hauer]

On Tue, Mar 20, 2012 at 01:14:51PM -0700, Saravana Kannan wrote:
> On 03/20/2012 11:14 AM, Sascha Hauer wrote:
> >On Tue, Mar 20, 2012 at 03:17:10AM -0700, Saravana Kannan wrote:
> >>
> >>On Tue, March 20, 2012 2:40 am, Sascha Hauer wrote:
> >>>On Tue, Mar 20, 2012 at 12:54:55AM -0700, Saravana Kannan wrote:
> >>>>
> >>>I am using these functions and don't need a static array, I just call
> >>>the functions with the desired parameters.
> >>
> >>Sure, then let's leave it in. Curious, where do you get the desired
> >>parameters from? Is it static date in code or is it from DT? You somehow
> >>probe it?
> >
> >It's not from DT. See this thread:
> >
> >http://www.spinics.net/lists/arm-kernel/msg165839.html
> 
> Ah, I see. That's a lot of functions calls. I think it would be much
> more efficient if you just have an array and loop over it. With my
> patch, you can just call a single register function for all these
> clocks.

I was curious and gave it a try. I registered a fixed clock and 100
gates as child clocks. I tried both DEFINE_CLK_GATE and
clk_register_gate. It turned out there was no difference in speed.

> 
> >
> >The difference is that a struct clk_initializer is only used to
> >initialize a clock and not actively used by the clock framework. But as
> >you already mentioned using a copy inside the clock framework has the
> >same effect.
> 
> My opinion on this is to follow a wait and watch approach on the
> "flags" field. I really can't think of a safe way anyone can misuse
> it. If people start doing that, we can just do the copy and let them
> deal with their broken code. Until that happens, I don't want to
> waste time/space by copying it. The rest of the fields aren't
> actively used by the common clock code after init.

Actually you will save space by copying it because you can put the
initializers in __initdata and throw the initializers of unused SoCs
compiled into the kernel away during runtime.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/2] clk: Move init fields from clk to clk_hw

[Sascha Hauer]

On Tue, Mar 20, 2012 at 01:06:34PM -0700, Saravana Kannan wrote:
> On 03/20/2012 11:10 AM, Sascha Hauer wrote:
> >On Tue, Mar 20, 2012 at 10:18:14PM +0800, Shawn Guo wrote:
> >>On Tue, Mar 20, 2012 at 10:40:31AM +0100, Sascha Hauer wrote:
> >>...
> >>>I am using these functions and don't need a static array, I just call
> >>>the functions with the desired parameters.
> >>>
> >>With this patch getting in, you do not have to use them then.  I feel
> >>a static array will be much more readable than a long list of function
> >>calls with a long list of hardcoded arguments to each.
> >
> >I'm also not a fan of long argument lists; a divider like defined in my
> >tree takes 5 arguments, a gate 4 and a mux 6. While 6 is already at the
> >border I think it's still acceptable.
> >
> >What I like in terms of readability is one line per clock which makes
> >for quite short clock files.
> 
> It certainly makes for short clock files, but it's definitely less
> readable that the expanded struct. For the original author the "one
> line per clock" looks readable since they wrote it. But for someone
> looking at the code to modify it, the expanded one would be much
> easier to read. Also, you can always declare your own macro if you
> really want to follow the one line approach.
> 
> >So when we use structs to initialize the clocks we'll probably have
> >something like this:
> >
> >static struct clk_divider somediv = {
> >	.reg = CCM_BASE + 0x14,
> >	.width = 3,
> >	.shift = 17,
> >	.lock =&ccm_lock,
> >	.hw.parent = "someotherdiv",
> >	.hw.flags = CLK_SET_RATE_PARENT,
> >};
> 
> Taken from your patches:
> 
> imx_clk_mux("spll_sel", CCM_CSCR, 17, 1, spll_sel_clks,
> ARRAY_SIZE(spll_sel_clks));
> 
> Compare the struct to the one line call. Now tell me, what does "1"
> represent? No clue. But in the struct, I can immediately tell what
> each one of the parameters are.
> 
> Anyway, my patch certainly isn't forcing you to use multiple lines.
> So, hopefully this won't be a point of contention.
> 
> >This will make a 4000 line file out of a 500 line file. Now when for
> >some reason struct clk_divider changes we end with big patches. If the
> >clk core gets a new fancy CLK_ flag which we want to have then again
> >we end up with big patches. Then there's also the possibility that
> >someone finds out that .lock and .hw.flags are common to all dividers
> >and comes up with a #define DEFINE_CLK_DIVIDER again to share common
> >fields.
> 
> This patch won't prevent you from doing any of that. You can still
> create macros for that (there are already one for that). Also, what
> you are pointing out is a bigger problem for the current
> clk_register() function since you might have to change the no. of
> params of all the callers even if a new field is optional.
> 
> >All this can be solved when we introduce a small wrapper function and
> >use it in the clock files:
> >
> >static inline struct clk *imx_clk_divider(const char *name, const char *parent,
> >		void __iomem *reg, u8 shift, u8 width)
> >{
> >	return clk_register_divider(NULL, name, parent, CLK_SET_RATE_PARENT,
> >			reg, shift, width, 0,&imx_ccm_lock);
> >}
> >
> >It decouples us from the structs used by the clock framework, we can
> >add our preferred flags and still can share common fields like the lock.
> >
> >While this was not the intention when I first converted from struct
> >initializers to function initializers I am confident that it will make
> >a good job.
> 
> Now I'm confused -- it's not clear if you are leaning towards my
> patch or away from it?

There was a tendency to get rid of static initializers and I like the
idea of not exposing any of the internally used members outside the
clock framework.
Now people try their best to make themselves comfortable with the
static initializers and you even discussed the possibility of removing
the clk_register_* functions (which make it possible for users not
to use any of the internal struct members). That's what I don't like
about your patches. But if we go for static initializers anyway then your
patches probably change things for the better.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/2] clk: Move init fields from clk to clk_hw

[Paul Walmsley]

Hello Sascha

On Tue, 20 Mar 2012, Sascha Hauer wrote:

> [ C99 structure initializer elided ]
> 
> This will make a 4000 line file out of a 500 line file. Now when for
> some reason struct clk_divider changes we end with big patches. If the
> clk core gets a new fancy CLK_ flag which we want to have then again
> we end up with big patches. Then there's also the possibility that
> someone finds out that .lock and .hw.flags are common to all dividers
> and comes up with a #define DEFINE_CLK_DIVIDER again to share common
> fields.

At least we can understand easily what is being changed.  Readability, 
particularly by others not familiar with the clock data, is more important 
to me.

So like Saravana, I too prefer C99 structure initializers.  At least there 
should be a choice.

Quick quiz: in this line below:

imx_clk_divider("foo_clk", "bar_clk", CCM_BASE + 0x20, 0x5, 0x3);

which field is the bitfield shift and which is the bitfield width? :-)


- Paul

Re: [PATCH 1/2] clk: Fix error handling in fixed clock hardware type register fn

[Turquette, Mike]

On Tue, Mar 20, 2012 at 12:46 AM, Saravana Kannan
<skannan@codeaurora.org> wrote:
> On Tue, March 20, 2012 12:19 am, Sascha Hauer wrote:
>> On Mon, Mar 19, 2012 at 08:38:25PM -0700, Saravana Kannan wrote:
>>> If memory allocation for the parents array or the parent string fails,
>>> then
>>> fail the registration immediately instead of calling clk_register and
>>> hoping it fails there.
>>>
>>> Return -ENOMEM on failure.
>>>
>>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>>> Cc: Mike Turquette <mturquette@linaro.org>
>>> Cc: Andrew Lunn <andrew@lunn.ch>
>>> Cc: Rob Herring <rob.herring@calxeda.com>
>>> Cc: Russell King <linux@arm.linux.org.uk>
>>> Cc: Jeremy Kerr <jeremy.kerr@canonical.com>
>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>> Cc: Arnd Bergman <arnd.bergmann@linaro.org>
>>> Cc: Paul Walmsley <paul@pwsan.com>
>>> Cc: Shawn Guo <shawn.guo@freescale.com>
>>> Cc: Sascha Hauer <s.hauer@pengutronix.de>
>>> Cc: Jamie Iles <jamie@jamieiles.com>
>>> Cc: Richard Zhao <richard.zhao@linaro.org>
>>> Cc: Saravana Kannan <skannan@codeaurora.org>
>>> Cc: Magnus Damm <magnus.damm@gmail.com>
>>> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
>>> Cc: Linus Walleij <linus.walleij@stericsson.com>
>>> Cc: Stephen Boyd <sboyd@codeaurora.org>
>>> Cc: Amit Kucheria <amit.kucheria@linaro.org>
>>> Cc: Deepak Saxena <dsaxena@linaro.org>
>>> Cc: Grant Likely <grant.likely@secretlab.ca>
>>> ---
>>> There are still some memory free issues when clk_register() fails, but I
>>> will
>>> fix it when I fixed the other register() fns to return ENOMEM of alloc
>>> failure instead of a NULL.
>>>
>>>  drivers/clk/clk-fixed-rate.c |   10 +++++++---
>>>  1 files changed, 7 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/clk/clk-fixed-rate.c b/drivers/clk/clk-fixed-rate.c
>>> index 90c79fb..6423ae9 100644
>>> --- a/drivers/clk/clk-fixed-rate.c
>>> +++ b/drivers/clk/clk-fixed-rate.c
>>> @@ -61,22 +61,26 @@ struct clk *clk_register_fixed_rate(struct device
>>> *dev, const char *name,
>>>              parent_names = kmalloc(sizeof(char *), GFP_KERNEL);
>>>
>>>              if (! parent_names)
>>> -                    goto out;
>>> +                    goto fail_ptr;
>>>
>>>              len = sizeof(char) * strlen(parent_name);
>>>
>>>              parent_names[0] = kmalloc(len, GFP_KERNEL);
>>>
>>>              if (!parent_names[0])
>>> -                    goto out;
>>> +                    goto fail_str;
>>>
>>>              strncpy(parent_names[0], parent_name, len);
>>>      }
>>
>> It's easier to add a char *parent to struct clk_fixed and pass it to
>> clk_register with &fixed->parent. This saves you a kmalloc call and
>> makes the error path simpler. It's the same way already done in the
>> divider.

I thought I had done this for v7... hmm looks like one got left out.
I'll line up a patch to get it in sync with the others as part of my
fixes.

> I thought about that since I saw the same was done for gated and divider
> (I think). Here is my guess at Mike's reasoning for this:
>
> Gated and divider clocks have to have a parent. There's nothing to gate
> otherwise. But fixed rate clocks might not have a parent. It could be XO's
> or PLLs running off of always on XOs not controlled by the SoC. So, it's
> arguable to not have a parent. I don't have a strong opinion on this --
> since Mike took the time to write it, it left it to his subjective
> preference.

I appreciate the thoughtfulness.  Re-using the same type of mechanism
as the divider and gate clocks will still allow the fixed-rate clock
to be parentless, and it makes for cleaner code, one less allocation
and lines up with how the other single-parent basic clocks are done,
so I'll take that method in instead of your patch.

> I sent this patch first since it was around the place I was cleaning up. I
> didn't want to actually just shuffle around a bug. As I mentioned, this
> patch still leaves a bug open -- what if clk_register() fails. I plan to
> fix that once my two patches are picked up (hopefully).

Do you still find it useful to return -ENOMEM from the registration
function instead of a NULL clock?  I'm always worried that people
don't check for error codes on pointers in their platform code and
only check for NULL...

Regards,
Mike

> Thanks,
> Saravana

Re: [PATCH 2/2] clk: Move init fields from clk to clk_hw

[Turquette, Mike]

On Tue, Mar 20, 2012 at 4:12 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Tue, Mar 20, 2012 at 01:06:34PM -0700, Saravana Kannan wrote:
>> On 03/20/2012 11:10 AM, Sascha Hauer wrote:
>> >On Tue, Mar 20, 2012 at 10:18:14PM +0800, Shawn Guo wrote:
>> >>On Tue, Mar 20, 2012 at 10:40:31AM +0100, Sascha Hauer wrote:
>> >>...
>> >>>I am using these functions and don't need a static array, I just call
>> >>>the functions with the desired parameters.
>> >>>
>> >>With this patch getting in, you do not have to use them then.  I feel
>> >>a static array will be much more readable than a long list of function
>> >>calls with a long list of hardcoded arguments to each.
>> >
>> >I'm also not a fan of long argument lists; a divider like defined in my
>> >tree takes 5 arguments, a gate 4 and a mux 6. While 6 is already at the
>> >border I think it's still acceptable.
>> >
>> >What I like in terms of readability is one line per clock which makes
>> >for quite short clock files.
>>
>> It certainly makes for short clock files, but it's definitely less
>> readable that the expanded struct. For the original author the "one
>> line per clock" looks readable since they wrote it. But for someone
>> looking at the code to modify it, the expanded one would be much
>> easier to read. Also, you can always declare your own macro if you
>> really want to follow the one line approach.
>>
>> >So when we use structs to initialize the clocks we'll probably have
>> >something like this:
>> >
>> >static struct clk_divider somediv = {
>> >     .reg = CCM_BASE + 0x14,
>> >     .width = 3,
>> >     .shift = 17,
>> >     .lock =&ccm_lock,
>> >     .hw.parent = "someotherdiv",
>> >     .hw.flags = CLK_SET_RATE_PARENT,
>> >};
>>
>> Taken from your patches:
>>
>> imx_clk_mux("spll_sel", CCM_CSCR, 17, 1, spll_sel_clks,
>> ARRAY_SIZE(spll_sel_clks));
>>
>> Compare the struct to the one line call. Now tell me, what does "1"
>> represent? No clue. But in the struct, I can immediately tell what
>> each one of the parameters are.
>>
>> Anyway, my patch certainly isn't forcing you to use multiple lines.
>> So, hopefully this won't be a point of contention.
>>
>> >This will make a 4000 line file out of a 500 line file. Now when for
>> >some reason struct clk_divider changes we end with big patches. If the
>> >clk core gets a new fancy CLK_ flag which we want to have then again
>> >we end up with big patches. Then there's also the possibility that
>> >someone finds out that .lock and .hw.flags are common to all dividers
>> >and comes up with a #define DEFINE_CLK_DIVIDER again to share common
>> >fields.
>>
>> This patch won't prevent you from doing any of that. You can still
>> create macros for that (there are already one for that). Also, what
>> you are pointing out is a bigger problem for the current
>> clk_register() function since you might have to change the no. of
>> params of all the callers even if a new field is optional.
>>
>> >All this can be solved when we introduce a small wrapper function and
>> >use it in the clock files:
>> >
>> >static inline struct clk *imx_clk_divider(const char *name, const char *parent,
>> >             void __iomem *reg, u8 shift, u8 width)
>> >{
>> >     return clk_register_divider(NULL, name, parent, CLK_SET_RATE_PARENT,
>> >                     reg, shift, width, 0,&imx_ccm_lock);
>> >}
>> >
>> >It decouples us from the structs used by the clock framework, we can
>> >add our preferred flags and still can share common fields like the lock.
>> >
>> >While this was not the intention when I first converted from struct
>> >initializers to function initializers I am confident that it will make
>> >a good job.
>>
>> Now I'm confused -- it's not clear if you are leaning towards my
>> patch or away from it?
>
> There was a tendency to get rid of static initializers and I like the
> idea of not exposing any of the internally used members outside the
> clock framework.

I'm with Sascha on this.  I feel that dividing the interface strictly
into two halves is the best way.  I have an uneasy feeling about
exposing this stuff into struct clk_hw (or clk_initializer or
whatever).  This stretches the data out across three structures and
just doesn't feel right to me.

> Now people try their best to make themselves comfortable with the
> static initializers and you even discussed the possibility of removing
> the clk_register_* functions (which make it possible for users not
> to use any of the internal struct members). That's what I don't like
> about your patches. But if we go for static initializers anyway then your
> patches probably change things for the better.

Static initialization is something I have fought for; in fact the
original patches provided no way to do it, so clearly what we have
today is some progress for the folks desiring static init.  The patch
above doesn't actually prevent allocation from happening as it still
must call into clk_register and kmalloc struct clk, so besides
readability, I'm not sure what these patches buy us.

Assuming that C99 designated initializers (for the sole purpose of
readability) is the main draw of the above patch, please let me know
what you think about modifying the existing static init macros so that
your clock data looks like this:

DEFINE_CLK_DIVIDER(dpll_iva_m5x2_ck, &dpll_iva_x2_ck, "dpll_iva_x2_ck",
        .flags = 0x0,
        .reg = OMAP4430_CM_DIV_M5_DPLL_IVA,
        .shift = OMAP4430_HSDIVIDER_CLKOUT2_DIV_SHIFT,
        .width = OMAP4430_HSDIVIDER_CLKOUT2_DIV_WIDTH,
        .flags = CLK_DIVIDER_ONE_BASED,
        .lock = NULL
);

Note that the first argument is the name of this clock (and will be
properly stringified for .name = "whatever") and that the second and
third arguments are both the parent clock, used for initializing the
parent pointer and .parent_names, respectively.  If that aspect of the
macro is too ugly then those can even be broken out into a separate
macro since they are independent data structure (struct clk **parents,
and char **parent_names).  Or you could just open code those data
structures and only use a macro for struct clk and struct clk_foo.

This gives you the readability of C99 designated initializers while
keeping struct clk's members totally hidden from the rest of the
world.

Regards,
Mike

Re: [PATCH 1/2] clk: Fix error handling in fixed clock hardware type register fn

[Saravana Kannan]

On 03/20/2012 05:13 PM, Turquette, Mike wrote:
> On Tue, Mar 20, 2012 at 12:46 AM, Saravana Kannan
> <skannan@codeaurora.org>  wrote:
>> On Tue, March 20, 2012 12:19 am, Sascha Hauer wrote:
>>> On Mon, Mar 19, 2012 at 08:38:25PM -0700, Saravana Kannan wrote:
>>>> If memory allocation for the parents array or the parent string fails,
>>>> then
>>>> fail the registration immediately instead of calling clk_register and
>>>> hoping it fails there.
>>>>
>>>> Return -ENOMEM on failure.
>>>>
>>>> Signed-off-by: Saravana Kannan<skannan@codeaurora.org>
>>>> Cc: Mike Turquette<mturquette@linaro.org>
>>>> Cc: Andrew Lunn<andrew@lunn.ch>
>>>> Cc: Rob Herring<rob.herring@calxeda.com>
>>>> Cc: Russell King<linux@arm.linux.org.uk>
>>>> Cc: Jeremy Kerr<jeremy.kerr@canonical.com>
>>>> Cc: Thomas Gleixner<tglx@linutronix.de>
>>>> Cc: Arnd Bergman<arnd.bergmann@linaro.org>
>>>> Cc: Paul Walmsley<paul@pwsan.com>
>>>> Cc: Shawn Guo<shawn.guo@freescale.com>
>>>> Cc: Sascha Hauer<s.hauer@pengutronix.de>
>>>> Cc: Jamie Iles<jamie@jamieiles.com>
>>>> Cc: Richard Zhao<richard.zhao@linaro.org>
>>>> Cc: Saravana Kannan<skannan@codeaurora.org>
>>>> Cc: Magnus Damm<magnus.damm@gmail.com>
>>>> Cc: Mark Brown<broonie@opensource.wolfsonmicro.com>
>>>> Cc: Linus Walleij<linus.walleij@stericsson.com>
>>>> Cc: Stephen Boyd<sboyd@codeaurora.org>
>>>> Cc: Amit Kucheria<amit.kucheria@linaro.org>
>>>> Cc: Deepak Saxena<dsaxena@linaro.org>
>>>> Cc: Grant Likely<grant.likely@secretlab.ca>
>>>> ---
>>>> There are still some memory free issues when clk_register() fails, but I
>>>> will
>>>> fix it when I fixed the other register() fns to return ENOMEM of alloc
>>>> failure instead of a NULL.
>>>>
>>>>   drivers/clk/clk-fixed-rate.c |   10 +++++++---
>>>>   1 files changed, 7 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/clk/clk-fixed-rate.c b/drivers/clk/clk-fixed-rate.c
>>>> index 90c79fb..6423ae9 100644
>>>> --- a/drivers/clk/clk-fixed-rate.c
>>>> +++ b/drivers/clk/clk-fixed-rate.c
>>>> @@ -61,22 +61,26 @@ struct clk *clk_register_fixed_rate(struct device
>>>> *dev, const char *name,
>>>>               parent_names = kmalloc(sizeof(char *), GFP_KERNEL);
>>>>
>>>>               if (! parent_names)
>>>> -                    goto out;
>>>> +                    goto fail_ptr;
>>>>
>>>>               len = sizeof(char) * strlen(parent_name);
>>>>
>>>>               parent_names[0] = kmalloc(len, GFP_KERNEL);
>>>>
>>>>               if (!parent_names[0])
>>>> -                    goto out;
>>>> +                    goto fail_str;
>>>>
>>>>               strncpy(parent_names[0], parent_name, len);
>>>>       }
>>>
>>> It's easier to add a char *parent to struct clk_fixed and pass it to
>>> clk_register with&fixed->parent. This saves you a kmalloc call and
>>> makes the error path simpler. It's the same way already done in the
>>> divider.
>
> I thought I had done this for v7... hmm looks like one got left out.
> I'll line up a patch to get it in sync with the others as part of my
> fixes.
>
>> I thought about that since I saw the same was done for gated and divider
>> (I think). Here is my guess at Mike's reasoning for this:
>>
>> Gated and divider clocks have to have a parent. There's nothing to gate
>> otherwise. But fixed rate clocks might not have a parent. It could be XO's
>> or PLLs running off of always on XOs not controlled by the SoC. So, it's
>> arguable to not have a parent. I don't have a strong opinion on this --
>> since Mike took the time to write it, it left it to his subjective
>> preference.
>
> I appreciate the thoughtfulness.  Re-using the same type of mechanism
> as the divider and gate clocks will still allow the fixed-rate clock
> to be parentless, and it makes for cleaner code, one less allocation
> and lines up with how the other single-parent basic clocks are done,
> so I'll take that method in instead of your patch.

No problem, go for it.

>
>> I sent this patch first since it was around the place I was cleaning up. I
>> didn't want to actually just shuffle around a bug. As I mentioned, this
>> patch still leaves a bug open -- what if clk_register() fails. I plan to
>> fix that once my two patches are picked up (hopefully).
>
> Do you still find it useful to return -ENOMEM from the registration
> function instead of a NULL clock?  I'm always worried that people
> don't check for error codes on pointers in their platform code and
> only check for NULL...

The last discussion I remember, NULL was considered a valid clock. So, I 
think on error, we shouldn't ever return NULL when the return type is 
struct clk *.

Thanks,
Saravana

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 2/2] clk: Move init fields from clk to clk_hw

[Saravana Kannan]

On 03/20/2012 06:47 PM, Turquette, Mike wrote:
> On Tue, Mar 20, 2012 at 4:12 PM, Sascha Hauer<s.hauer@pengutronix.de>  wrote:
>> On Tue, Mar 20, 2012 at 01:06:34PM -0700, Saravana Kannan wrote:
>>> On 03/20/2012 11:10 AM, Sascha Hauer wrote:
>>>> On Tue, Mar 20, 2012 at 10:18:14PM +0800, Shawn Guo wrote:
>>>>> On Tue, Mar 20, 2012 at 10:40:31AM +0100, Sascha Hauer wrote:
>>>>> ...
>>>>>> I am using these functions and don't need a static array, I just call
>>>>>> the functions with the desired parameters.
>>>>>>
>>>>> With this patch getting in, you do not have to use them then.  I feel
>>>>> a static array will be much more readable than a long list of function
>>>>> calls with a long list of hardcoded arguments to each.
>>>>
>>>> I'm also not a fan of long argument lists; a divider like defined in my
>>>> tree takes 5 arguments, a gate 4 and a mux 6. While 6 is already at the
>>>> border I think it's still acceptable.
>>>>
>>>> What I like in terms of readability is one line per clock which makes
>>>> for quite short clock files.
>>>
>>> It certainly makes for short clock files, but it's definitely less
>>> readable that the expanded struct. For the original author the "one
>>> line per clock" looks readable since they wrote it. But for someone
>>> looking at the code to modify it, the expanded one would be much
>>> easier to read. Also, you can always declare your own macro if you
>>> really want to follow the one line approach.
>>>
>>>> So when we use structs to initialize the clocks we'll probably have
>>>> something like this:
>>>>
>>>> static struct clk_divider somediv = {
>>>>      .reg = CCM_BASE + 0x14,
>>>>      .width = 3,
>>>>      .shift = 17,
>>>>      .lock =&ccm_lock,
>>>>      .hw.parent = "someotherdiv",
>>>>      .hw.flags = CLK_SET_RATE_PARENT,
>>>> };
>>>
>>> Taken from your patches:
>>>
>>> imx_clk_mux("spll_sel", CCM_CSCR, 17, 1, spll_sel_clks,
>>> ARRAY_SIZE(spll_sel_clks));
>>>
>>> Compare the struct to the one line call. Now tell me, what does "1"
>>> represent? No clue. But in the struct, I can immediately tell what
>>> each one of the parameters are.
>>>
>>> Anyway, my patch certainly isn't forcing you to use multiple lines.
>>> So, hopefully this won't be a point of contention.
>>>
>>>> This will make a 4000 line file out of a 500 line file. Now when for
>>>> some reason struct clk_divider changes we end with big patches. If the
>>>> clk core gets a new fancy CLK_ flag which we want to have then again
>>>> we end up with big patches. Then there's also the possibility that
>>>> someone finds out that .lock and .hw.flags are common to all dividers
>>>> and comes up with a #define DEFINE_CLK_DIVIDER again to share common
>>>> fields.
>>>
>>> This patch won't prevent you from doing any of that. You can still
>>> create macros for that (there are already one for that). Also, what
>>> you are pointing out is a bigger problem for the current
>>> clk_register() function since you might have to change the no. of
>>> params of all the callers even if a new field is optional.
>>>
>>>> All this can be solved when we introduce a small wrapper function and
>>>> use it in the clock files:
>>>>
>>>> static inline struct clk *imx_clk_divider(const char *name, const char *parent,
>>>>              void __iomem *reg, u8 shift, u8 width)
>>>> {
>>>>      return clk_register_divider(NULL, name, parent, CLK_SET_RATE_PARENT,
>>>>                      reg, shift, width, 0,&imx_ccm_lock);
>>>> }
>>>>
>>>> It decouples us from the structs used by the clock framework, we can
>>>> add our preferred flags and still can share common fields like the lock.
>>>>
>>>> While this was not the intention when I first converted from struct
>>>> initializers to function initializers I am confident that it will make
>>>> a good job.
>>>
>>> Now I'm confused -- it's not clear if you are leaning towards my
>>> patch or away from it?
>>
>> There was a tendency to get rid of static initializers and I like the
>> idea of not exposing any of the internally used members outside the
>> clock framework.
>
> I'm with Sascha on this.  I feel that dividing the interface strictly
> into two halves is the best way.

I addressed this concern in my earlier comments. We can make a copy or 
we can agree the fields I moved to clk_hw aren't really useful wrt 
writing hacky code and call it a day. Can you please clarify why neither 
of these options are acceptable?

> I have an uneasy feeling about
> exposing this stuff into struct clk_hw (or clk_initializer or
> whatever).  This stretches the data out across three structures and
> just doesn't feel right to me.

Wrt this discussion, there are three distinct classes of data:
1) Those specific to the platform driver that the common code shouldn't 
care about.
2) Those specific to the common code that the platform driver shouldn't 
care about.
3) Stuff that's shared/passed between common code and the platform driver.

When we have three classes of data, I don't what's wrong with having 
three struct types to contain them. If anything, it's better than the 
current approach of exposing the common clock code specific data (struct 
clk) to code outside of common clock code just because we want to allow 
static initialization. The end goal should be to move struct clk inside 
clk.c.

I think this patch just takes us one step close to that since IMX and 
MSM won't have to include clk-private.h in any of our platform specific 
files while also allowing OMAP to include it for the near term.

>> Now people try their best to make themselves comfortable with the
>> static initializers and you even discussed the possibility of removing
>> the clk_register_* functions (which make it possible for users not
>> to use any of the internal struct members). That's what I don't like
>> about your patches. But if we go for static initializers anyway then your
>> patches probably change things for the better.
>
> Static initialization is something I have fought for; in fact the
> original patches provided no way to do it, so clearly what we have
> today is some progress for the folks desiring static init.

I too desire static init. Sorry if I was unclear and gave people the 
misconception that I wanted to remove static init.

> The patch
> above doesn't actually prevent allocation from happening as it still
> must call into clk_register and kmalloc struct clk,

Correct.

> so besides
> readability, I'm not sure what these patches buy us.

I think readability is very important and if this buys us nothing but 
readability, we should still take this patch. But there are other 
benefits too -- I mentioned them in the commit text.

> Assuming that C99 designated initializers (for the sole purpose of
> readability) is the main draw of the above patch, please let me know
> what you think about modifying the existing static init macros so that
> your clock data looks like this:
>
> DEFINE_CLK_DIVIDER(dpll_iva_m5x2_ck,&dpll_iva_x2_ck, "dpll_iva_x2_ck",
>          .flags = 0x0,
>          .reg = OMAP4430_CM_DIV_M5_DPLL_IVA,
>          .shift = OMAP4430_HSDIVIDER_CLKOUT2_DIV_SHIFT,
>          .width = OMAP4430_HSDIVIDER_CLKOUT2_DIV_WIDTH,
>          .flags = CLK_DIVIDER_ONE_BASED,
>          .lock = NULL
> );
>
> Note that the first argument is the name of this clock (and will be
> properly stringified for .name = "whatever") and that the second and
> third arguments are both the parent clock, used for initializing the
> parent pointer and .parent_names, respectively.  If that aspect of the
> macro is too ugly then those can even be broken out into a separate
> macro since they are independent data structure (struct clk **parents,
> and char **parent_names).  Or you could just open code those data
> structures and only use a macro for struct clk and struct clk_foo.
>
> This gives you the readability of C99 designated initializers while
> keeping struct clk's members totally hidden from the rest of the
> world.

But it still leaves the struct clk exposed to people who do static init 
of the clock tree. I think agreeing that the name, parent names, flags 
and ops are not used to hack with or just making a copy of all of them 
(and mark the originals as __init if that's doable). is a better 
solution than trying to go with macros and leave struct clk exposed to 
everyone who want to do static init of the clock tree.

At a later point when we are ready to move struct clk inside clk.c, with 
this patch applied right now, IMX and MSM won't have to churn their code.

Thanks,
Saravana

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 1/2] clk: Fix error handling in fixed clock hardware type register fn

[Turquette, Mike]

On Tue, Mar 20, 2012 at 7:32 PM, Saravana Kannan <skannan@codeaurora.org> wrote:
> On 03/20/2012 05:13 PM, Turquette, Mike wrote:
>>
>> On Tue, Mar 20, 2012 at 12:46 AM, Saravana Kannan
>> <skannan@codeaurora.org>  wrote:
>>>
>>> On Tue, March 20, 2012 12:19 am, Sascha Hauer wrote:
>>>>
>>>> On Mon, Mar 19, 2012 at 08:38:25PM -0700, Saravana Kannan wrote:
>>>>>
>>>>> If memory allocation for the parents array or the parent string fails,
>>>>> then
>>>>> fail the registration immediately instead of calling clk_register and
>>>>> hoping it fails there.
>>>>>
>>>>> Return -ENOMEM on failure.
>>>>>
>>>>> Signed-off-by: Saravana Kannan<skannan@codeaurora.org>
>>>>> Cc: Mike Turquette<mturquette@linaro.org>
>>>>> Cc: Andrew Lunn<andrew@lunn.ch>
>>>>> Cc: Rob Herring<rob.herring@calxeda.com>
>>>>> Cc: Russell King<linux@arm.linux.org.uk>
>>>>> Cc: Jeremy Kerr<jeremy.kerr@canonical.com>
>>>>> Cc: Thomas Gleixner<tglx@linutronix.de>
>>>>> Cc: Arnd Bergman<arnd.bergmann@linaro.org>
>>>>> Cc: Paul Walmsley<paul@pwsan.com>
>>>>> Cc: Shawn Guo<shawn.guo@freescale.com>
>>>>> Cc: Sascha Hauer<s.hauer@pengutronix.de>
>>>>> Cc: Jamie Iles<jamie@jamieiles.com>
>>>>> Cc: Richard Zhao<richard.zhao@linaro.org>
>>>>> Cc: Saravana Kannan<skannan@codeaurora.org>
>>>>> Cc: Magnus Damm<magnus.damm@gmail.com>
>>>>> Cc: Mark Brown<broonie@opensource.wolfsonmicro.com>
>>>>> Cc: Linus Walleij<linus.walleij@stericsson.com>
>>>>> Cc: Stephen Boyd<sboyd@codeaurora.org>
>>>>> Cc: Amit Kucheria<amit.kucheria@linaro.org>
>>>>> Cc: Deepak Saxena<dsaxena@linaro.org>
>>>>> Cc: Grant Likely<grant.likely@secretlab.ca>
>>>>> ---
>>>>> There are still some memory free issues when clk_register() fails, but
>>>>> I
>>>>> will
>>>>> fix it when I fixed the other register() fns to return ENOMEM of alloc
>>>>> failure instead of a NULL.
>>>>>
>>>>>  drivers/clk/clk-fixed-rate.c |   10 +++++++---
>>>>>  1 files changed, 7 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/clk/clk-fixed-rate.c
>>>>> b/drivers/clk/clk-fixed-rate.c
>>>>> index 90c79fb..6423ae9 100644
>>>>> --- a/drivers/clk/clk-fixed-rate.c
>>>>> +++ b/drivers/clk/clk-fixed-rate.c
>>>>> @@ -61,22 +61,26 @@ struct clk *clk_register_fixed_rate(struct device
>>>>> *dev, const char *name,
>>>>>              parent_names = kmalloc(sizeof(char *), GFP_KERNEL);
>>>>>
>>>>>              if (! parent_names)
>>>>> -                    goto out;
>>>>> +                    goto fail_ptr;
>>>>>
>>>>>              len = sizeof(char) * strlen(parent_name);
>>>>>
>>>>>              parent_names[0] = kmalloc(len, GFP_KERNEL);
>>>>>
>>>>>              if (!parent_names[0])
>>>>> -                    goto out;
>>>>> +                    goto fail_str;
>>>>>
>>>>>              strncpy(parent_names[0], parent_name, len);
>>>>>      }
>>>>
>>>>
>>>> It's easier to add a char *parent to struct clk_fixed and pass it to
>>>> clk_register with&fixed->parent. This saves you a kmalloc call and
>>>>
>>>> makes the error path simpler. It's the same way already done in the
>>>> divider.
>>
>>
>> I thought I had done this for v7... hmm looks like one got left out.
>> I'll line up a patch to get it in sync with the others as part of my
>> fixes.
>>
>>> I thought about that since I saw the same was done for gated and divider
>>> (I think). Here is my guess at Mike's reasoning for this:
>>>
>>> Gated and divider clocks have to have a parent. There's nothing to gate
>>> otherwise. But fixed rate clocks might not have a parent. It could be
>>> XO's
>>> or PLLs running off of always on XOs not controlled by the SoC. So, it's
>>> arguable to not have a parent. I don't have a strong opinion on this --
>>> since Mike took the time to write it, it left it to his subjective
>>> preference.
>>
>>
>> I appreciate the thoughtfulness.  Re-using the same type of mechanism
>> as the divider and gate clocks will still allow the fixed-rate clock
>> to be parentless, and it makes for cleaner code, one less allocation
>> and lines up with how the other single-parent basic clocks are done,
>> so I'll take that method in instead of your patch.
>
>
> No problem, go for it.
>
>
>>
>>> I sent this patch first since it was around the place I was cleaning up.
>>> I
>>> didn't want to actually just shuffle around a bug. As I mentioned, this
>>> patch still leaves a bug open -- what if clk_register() fails. I plan to
>>> fix that once my two patches are picked up (hopefully).
>>
>>
>> Do you still find it useful to return -ENOMEM from the registration
>> function instead of a NULL clock?  I'm always worried that people
>> don't check for error codes on pointers in their platform code and
>> only check for NULL...
>
>
> The last discussion I remember, NULL was considered a valid clock. So, I
> think on error, we shouldn't ever return NULL when the return type is struct
> clk *.

IIRC, that discussion was with respect to the .parent member of struct
clk.  It was decided that having .parent = NULL does not imply that a
clock is a root clock, but instead we rely on the CLK_IS_ROOT flag.  I
can't think of any other instance where a NULL clk when returned from
a registration function would be useful.

Regards,
Mike

> Thanks,
> Saravana

Re: [PATCH 1/2] clk: Fix error handling in fixed clock hardware type register fn

[Saravana Kannan]

On Tue, March 20, 2012 10:45 pm, Turquette, Mike wrote:
> IIRC, that discussion was with respect to the .parent member of struct
> clk.  It was decided that having .parent = NULL does not imply that a
> clock is a root clock, but instead we rely on the CLK_IS_ROOT flag.  I
> can't think of any other instance where a NULL clk when returned from
> a registration function would be useful.

I wasn't referring to a discussion with regards to this patch series. I'm
referring to what I believe was a discussion with Russell King on whether
a NULL from clk_get() should be considered a clock and whether it should
be usable as a "clock" with the clock APIs. That's the reason clk_get()
returns -ENOENT instead of NULL.

-Saravana

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 1/2] clk: Fix error handling in fixed clock hardware type register fn

[Russell King - ARM Linux]

On Tue, Mar 20, 2012 at 10:45:36PM -0700, Turquette, Mike wrote:
> IIRC, that discussion was with respect to the .parent member of struct
> clk.  It was decided that having .parent = NULL does not imply that a
> clock is a root clock, but instead we rely on the CLK_IS_ROOT flag.  I
> can't think of any other instance where a NULL clk when returned from
> a registration function would be useful.

No.  What I've been saying is that in _drivers_ which use clk_get(),
the error range is defined by IS_ERR(clk).  Every other value of
clk must be assumed by the driver to be valid, no ifs or buts.

What the clk API assumes about struct clk is its own business.  If it
wishes to interpret the NULL clk in a specific way, that's its business
to do so.  But it's still not the drivers business to then go and
interpret NULL as being any different from any other valid clock.

This is part of the clk API contract, which is as I said in the first
paragraph, and I keep on saying.  I'm not sure what's so difficult
about this.

Re: [PATCH 2/2] clk: Move init fields from clk to clk_hw

[Sascha Hauer]

On Tue, Mar 20, 2012 at 05:47:42PM -0600, Paul Walmsley wrote:
> Hello Sascha
> 
> On Tue, 20 Mar 2012, Sascha Hauer wrote:
> 
> > [ C99 structure initializer elided ]
> > 
> > This will make a 4000 line file out of a 500 line file. Now when for
> > some reason struct clk_divider changes we end with big patches. If the
> > clk core gets a new fancy CLK_ flag which we want to have then again
> > we end up with big patches. Then there's also the possibility that
> > someone finds out that .lock and .hw.flags are common to all dividers
> > and comes up with a #define DEFINE_CLK_DIVIDER again to share common
> > fields.
> 
> At least we can understand easily what is being changed.  Readability, 
> particularly by others not familiar with the clock data, is more important 
> to me.
> 
> So like Saravana, I too prefer C99 structure initializers.  At least there 
> should be a choice.
> 
> Quick quiz: in this line below:
> 
> imx_clk_divider("foo_clk", "bar_clk", CCM_BASE + 0x20, 0x5, 0x3);
> 
> which field is the bitfield shift and which is the bitfield width? :-)

I can't even remember the argument order in memset, but I never voted
for a struct memset_init ;)

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/2] clk: Fix error handling in fixed clock hardware type register fn

[Turquette, Mike]

On Wed, Mar 21, 2012 at 2:07 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Mar 20, 2012 at 10:45:36PM -0700, Turquette, Mike wrote:
>> IIRC, that discussion was with respect to the .parent member of struct
>> clk.  It was decided that having .parent = NULL does not imply that a
>> clock is a root clock, but instead we rely on the CLK_IS_ROOT flag.  I
>> can't think of any other instance where a NULL clk when returned from
>> a registration function would be useful.
>
> No.  What I've been saying is that in _drivers_ which use clk_get(),
> the error range is defined by IS_ERR(clk).  Every other value of
> clk must be assumed by the driver to be valid, no ifs or buts.

Drivers certainly might make calls to clk_register_foo, despite it
being outside of the traditional clock api.  For the sake of
consistency I'll convert the clk_register_foo functions to return
error codes.

Regards,
Mike

Re: [PATCH 2/2] clk: Move init fields from clk to clk_hw

[Shawn Guo]

On Tue, Mar 20, 2012 at 11:40:21PM +0100, Sascha Hauer wrote:
> On Tue, Mar 20, 2012 at 01:14:51PM -0700, Saravana Kannan wrote:
...
> > Ah, I see. That's a lot of functions calls. I think it would be much
> > more efficient if you just have an array and loop over it. With my
> > patch, you can just call a single register function for all these
> > clocks.
> 
> I was curious and gave it a try. I registered a fixed clock and 100
> gates as child clocks. I tried both DEFINE_CLK_GATE and
> clk_register_gate. It turned out there was no difference in speed.
> 
While there is no difference in speed efficiency, I think we will
gain space efficiency if we can have Saravana's patch to copy the
clk_hw fields and make the static array __initdata as you mentioned.

-- 
Regards,
Shawn

Re: [PATCH 2/2] clk: Move init fields from clk to clk_hw

[Saravana Kannan]

Mike,

(*nudge*) (*nudge*)

-Saravana


On 03/20/2012 08:01 PM, Saravana Kannan wrote:
> On 03/20/2012 06:47 PM, Turquette, Mike wrote:
>> On Tue, Mar 20, 2012 at 4:12 PM, Sascha Hauer<s.hauer@pengutronix.de>
>> wrote:
>>> On Tue, Mar 20, 2012 at 01:06:34PM -0700, Saravana Kannan wrote:
>>>> On 03/20/2012 11:10 AM, Sascha Hauer wrote:
>>>>> On Tue, Mar 20, 2012 at 10:18:14PM +0800, Shawn Guo wrote:
>>>>>> On Tue, Mar 20, 2012 at 10:40:31AM +0100, Sascha Hauer wrote:
>>>>>> ...
>>>>>>> I am using these functions and don't need a static array, I just
>>>>>>> call
>>>>>>> the functions with the desired parameters.
>>>>>>>
>>>>>> With this patch getting in, you do not have to use them then. I feel
>>>>>> a static array will be much more readable than a long list of
>>>>>> function
>>>>>> calls with a long list of hardcoded arguments to each.
>>>>>
>>>>> I'm also not a fan of long argument lists; a divider like defined
>>>>> in my
>>>>> tree takes 5 arguments, a gate 4 and a mux 6. While 6 is already at
>>>>> the
>>>>> border I think it's still acceptable.
>>>>>
>>>>> What I like in terms of readability is one line per clock which makes
>>>>> for quite short clock files.
>>>>
>>>> It certainly makes for short clock files, but it's definitely less
>>>> readable that the expanded struct. For the original author the "one
>>>> line per clock" looks readable since they wrote it. But for someone
>>>> looking at the code to modify it, the expanded one would be much
>>>> easier to read. Also, you can always declare your own macro if you
>>>> really want to follow the one line approach.
>>>>
>>>>> So when we use structs to initialize the clocks we'll probably have
>>>>> something like this:
>>>>>
>>>>> static struct clk_divider somediv = {
>>>>> .reg = CCM_BASE + 0x14,
>>>>> .width = 3,
>>>>> .shift = 17,
>>>>> .lock =&ccm_lock,
>>>>> .hw.parent = "someotherdiv",
>>>>> .hw.flags = CLK_SET_RATE_PARENT,
>>>>> };
>>>>
>>>> Taken from your patches:
>>>>
>>>> imx_clk_mux("spll_sel", CCM_CSCR, 17, 1, spll_sel_clks,
>>>> ARRAY_SIZE(spll_sel_clks));
>>>>
>>>> Compare the struct to the one line call. Now tell me, what does "1"
>>>> represent? No clue. But in the struct, I can immediately tell what
>>>> each one of the parameters are.
>>>>
>>>> Anyway, my patch certainly isn't forcing you to use multiple lines.
>>>> So, hopefully this won't be a point of contention.
>>>>
>>>>> This will make a 4000 line file out of a 500 line file. Now when for
>>>>> some reason struct clk_divider changes we end with big patches. If the
>>>>> clk core gets a new fancy CLK_ flag which we want to have then again
>>>>> we end up with big patches. Then there's also the possibility that
>>>>> someone finds out that .lock and .hw.flags are common to all dividers
>>>>> and comes up with a #define DEFINE_CLK_DIVIDER again to share common
>>>>> fields.
>>>>
>>>> This patch won't prevent you from doing any of that. You can still
>>>> create macros for that (there are already one for that). Also, what
>>>> you are pointing out is a bigger problem for the current
>>>> clk_register() function since you might have to change the no. of
>>>> params of all the callers even if a new field is optional.
>>>>
>>>>> All this can be solved when we introduce a small wrapper function and
>>>>> use it in the clock files:
>>>>>
>>>>> static inline struct clk *imx_clk_divider(const char *name, const
>>>>> char *parent,
>>>>> void __iomem *reg, u8 shift, u8 width)
>>>>> {
>>>>> return clk_register_divider(NULL, name, parent, CLK_SET_RATE_PARENT,
>>>>> reg, shift, width, 0,&imx_ccm_lock);
>>>>> }
>>>>>
>>>>> It decouples us from the structs used by the clock framework, we can
>>>>> add our preferred flags and still can share common fields like the
>>>>> lock.
>>>>>
>>>>> While this was not the intention when I first converted from struct
>>>>> initializers to function initializers I am confident that it will make
>>>>> a good job.
>>>>
>>>> Now I'm confused -- it's not clear if you are leaning towards my
>>>> patch or away from it?
>>>
>>> There was a tendency to get rid of static initializers and I like the
>>> idea of not exposing any of the internally used members outside the
>>> clock framework.
>>
>> I'm with Sascha on this. I feel that dividing the interface strictly
>> into two halves is the best way.
>
> I addressed this concern in my earlier comments. We can make a copy or
> we can agree the fields I moved to clk_hw aren't really useful wrt
> writing hacky code and call it a day. Can you please clarify why neither
> of these options are acceptable?
>
>> I have an uneasy feeling about
>> exposing this stuff into struct clk_hw (or clk_initializer or
>> whatever). This stretches the data out across three structures and
>> just doesn't feel right to me.
>
> Wrt this discussion, there are three distinct classes of data:
> 1) Those specific to the platform driver that the common code shouldn't
> care about.
> 2) Those specific to the common code that the platform driver shouldn't
> care about.
> 3) Stuff that's shared/passed between common code and the platform driver.
>
> When we have three classes of data, I don't what's wrong with having
> three struct types to contain them. If anything, it's better than the
> current approach of exposing the common clock code specific data (struct
> clk) to code outside of common clock code just because we want to allow
> static initialization. The end goal should be to move struct clk inside
> clk.c.
>
> I think this patch just takes us one step close to that since IMX and
> MSM won't have to include clk-private.h in any of our platform specific
> files while also allowing OMAP to include it for the near term.
>
>>> Now people try their best to make themselves comfortable with the
>>> static initializers and you even discussed the possibility of removing
>>> the clk_register_* functions (which make it possible for users not
>>> to use any of the internal struct members). That's what I don't like
>>> about your patches. But if we go for static initializers anyway then
>>> your
>>> patches probably change things for the better.
>>
>> Static initialization is something I have fought for; in fact the
>> original patches provided no way to do it, so clearly what we have
>> today is some progress for the folks desiring static init.
>
> I too desire static init. Sorry if I was unclear and gave people the
> misconception that I wanted to remove static init.
>
>> The patch
>> above doesn't actually prevent allocation from happening as it still
>> must call into clk_register and kmalloc struct clk,
>
> Correct.
>
>> so besides
>> readability, I'm not sure what these patches buy us.
>
> I think readability is very important and if this buys us nothing but
> readability, we should still take this patch. But there are other
> benefits too -- I mentioned them in the commit text.
>
>> Assuming that C99 designated initializers (for the sole purpose of
>> readability) is the main draw of the above patch, please let me know
>> what you think about modifying the existing static init macros so that
>> your clock data looks like this:
>>
>> DEFINE_CLK_DIVIDER(dpll_iva_m5x2_ck,&dpll_iva_x2_ck, "dpll_iva_x2_ck",
>> .flags = 0x0,
>> .reg = OMAP4430_CM_DIV_M5_DPLL_IVA,
>> .shift = OMAP4430_HSDIVIDER_CLKOUT2_DIV_SHIFT,
>> .width = OMAP4430_HSDIVIDER_CLKOUT2_DIV_WIDTH,
>> .flags = CLK_DIVIDER_ONE_BASED,
>> .lock = NULL
>> );
>>
>> Note that the first argument is the name of this clock (and will be
>> properly stringified for .name = "whatever") and that the second and
>> third arguments are both the parent clock, used for initializing the
>> parent pointer and .parent_names, respectively. If that aspect of the
>> macro is too ugly then those can even be broken out into a separate
>> macro since they are independent data structure (struct clk **parents,
>> and char **parent_names). Or you could just open code those data
>> structures and only use a macro for struct clk and struct clk_foo.
>>
>> This gives you the readability of C99 designated initializers while
>> keeping struct clk's members totally hidden from the rest of the
>> world.
>
> But it still leaves the struct clk exposed to people who do static init
> of the clock tree. I think agreeing that the name, parent names, flags
> and ops are not used to hack with or just making a copy of all of them
> (and mark the originals as __init if that's doable). is a better
> solution than trying to go with macros and leave struct clk exposed to
> everyone who want to do static init of the clock tree.
>
> At a later point when we are ready to move struct clk inside clk.c, with
> this patch applied right now, IMX and MSM won't have to churn their code.
>
> Thanks,
> Saravana
>


-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 2/2] clk: Move init fields from clk to clk_hw

[Turquette, Mike]

On Mon, Mar 26, 2012 at 9:35 PM, Saravana Kannan <skannan@codeaurora.org> wrote:
> Mike,
>
> (*nudge*) (*nudge*)

Hi Saravana,

I spent some time mulling over the ideas this weekend and I agree that
we have 3 types of data, so having the three structs makes some sense.
 I'm checking this patch out more earnestly today.

Regards,
Mike

> -Saravana
>
>
>
> On 03/20/2012 08:01 PM, Saravana Kannan wrote:
>>
>> On 03/20/2012 06:47 PM, Turquette, Mike wrote:
>>>
>>> On Tue, Mar 20, 2012 at 4:12 PM, Sascha Hauer<s.hauer@pengutronix.de>
>>> wrote:
>>>>
>>>> On Tue, Mar 20, 2012 at 01:06:34PM -0700, Saravana Kannan wrote:
>>>>>
>>>>> On 03/20/2012 11:10 AM, Sascha Hauer wrote:
>>>>>>
>>>>>> On Tue, Mar 20, 2012 at 10:18:14PM +0800, Shawn Guo wrote:
>>>>>>>
>>>>>>> On Tue, Mar 20, 2012 at 10:40:31AM +0100, Sascha Hauer wrote:
>>>>>>> ...
>>>>>>>>
>>>>>>>> I am using these functions and don't need a static array, I just
>>>>>>>> call
>>>>>>>> the functions with the desired parameters.
>>>>>>>>
>>>>>>> With this patch getting in, you do not have to use them then. I feel
>>>>>>> a static array will be much more readable than a long list of
>>>>>>> function
>>>>>>> calls with a long list of hardcoded arguments to each.
>>>>>>
>>>>>>
>>>>>> I'm also not a fan of long argument lists; a divider like defined
>>>>>> in my
>>>>>> tree takes 5 arguments, a gate 4 and a mux 6. While 6 is already at
>>>>>> the
>>>>>> border I think it's still acceptable.
>>>>>>
>>>>>> What I like in terms of readability is one line per clock which makes
>>>>>> for quite short clock files.
>>>>>
>>>>>
>>>>> It certainly makes for short clock files, but it's definitely less
>>>>> readable that the expanded struct. For the original author the "one
>>>>> line per clock" looks readable since they wrote it. But for someone
>>>>> looking at the code to modify it, the expanded one would be much
>>>>> easier to read. Also, you can always declare your own macro if you
>>>>> really want to follow the one line approach.
>>>>>
>>>>>> So when we use structs to initialize the clocks we'll probably have
>>>>>> something like this:
>>>>>>
>>>>>> static struct clk_divider somediv = {
>>>>>> .reg = CCM_BASE + 0x14,
>>>>>> .width = 3,
>>>>>> .shift = 17,
>>>>>> .lock =&ccm_lock,
>>>>>> .hw.parent = "someotherdiv",
>>>>>> .hw.flags = CLK_SET_RATE_PARENT,
>>>>>> };
>>>>>
>>>>>
>>>>> Taken from your patches:
>>>>>
>>>>> imx_clk_mux("spll_sel", CCM_CSCR, 17, 1, spll_sel_clks,
>>>>> ARRAY_SIZE(spll_sel_clks));
>>>>>
>>>>> Compare the struct to the one line call. Now tell me, what does "1"
>>>>> represent? No clue. But in the struct, I can immediately tell what
>>>>> each one of the parameters are.
>>>>>
>>>>> Anyway, my patch certainly isn't forcing you to use multiple lines.
>>>>> So, hopefully this won't be a point of contention.
>>>>>
>>>>>> This will make a 4000 line file out of a 500 line file. Now when for
>>>>>> some reason struct clk_divider changes we end with big patches. If the
>>>>>> clk core gets a new fancy CLK_ flag which we want to have then again
>>>>>> we end up with big patches. Then there's also the possibility that
>>>>>> someone finds out that .lock and .hw.flags are common to all dividers
>>>>>> and comes up with a #define DEFINE_CLK_DIVIDER again to share common
>>>>>> fields.
>>>>>
>>>>>
>>>>> This patch won't prevent you from doing any of that. You can still
>>>>> create macros for that (there are already one for that). Also, what
>>>>> you are pointing out is a bigger problem for the current
>>>>> clk_register() function since you might have to change the no. of
>>>>> params of all the callers even if a new field is optional.
>>>>>
>>>>>> All this can be solved when we introduce a small wrapper function and
>>>>>> use it in the clock files:
>>>>>>
>>>>>> static inline struct clk *imx_clk_divider(const char *name, const
>>>>>> char *parent,
>>>>>> void __iomem *reg, u8 shift, u8 width)
>>>>>> {
>>>>>> return clk_register_divider(NULL, name, parent, CLK_SET_RATE_PARENT,
>>>>>> reg, shift, width, 0,&imx_ccm_lock);
>>>>>> }
>>>>>>
>>>>>> It decouples us from the structs used by the clock framework, we can
>>>>>> add our preferred flags and still can share common fields like the
>>>>>> lock.
>>>>>>
>>>>>> While this was not the intention when I first converted from struct
>>>>>> initializers to function initializers I am confident that it will make
>>>>>> a good job.
>>>>>
>>>>>
>>>>> Now I'm confused -- it's not clear if you are leaning towards my
>>>>> patch or away from it?
>>>>
>>>>
>>>> There was a tendency to get rid of static initializers and I like the
>>>> idea of not exposing any of the internally used members outside the
>>>> clock framework.
>>>
>>>
>>> I'm with Sascha on this. I feel that dividing the interface strictly
>>> into two halves is the best way.
>>
>>
>> I addressed this concern in my earlier comments. We can make a copy or
>> we can agree the fields I moved to clk_hw aren't really useful wrt
>> writing hacky code and call it a day. Can you please clarify why neither
>> of these options are acceptable?
>>
>>> I have an uneasy feeling about
>>> exposing this stuff into struct clk_hw (or clk_initializer or
>>> whatever). This stretches the data out across three structures and
>>> just doesn't feel right to me.
>>
>>
>> Wrt this discussion, there are three distinct classes of data:
>> 1) Those specific to the platform driver that the common code shouldn't
>> care about.
>> 2) Those specific to the common code that the platform driver shouldn't
>> care about.
>> 3) Stuff that's shared/passed between common code and the platform driver.
>>
>> When we have three classes of data, I don't what's wrong with having
>> three struct types to contain them. If anything, it's better than the
>> current approach of exposing the common clock code specific data (struct
>> clk) to code outside of common clock code just because we want to allow
>> static initialization. The end goal should be to move struct clk inside
>> clk.c.
>>
>> I think this patch just takes us one step close to that since IMX and
>> MSM won't have to include clk-private.h in any of our platform specific
>> files while also allowing OMAP to include it for the near term.
>>
>>>> Now people try their best to make themselves comfortable with the
>>>> static initializers and you even discussed the possibility of removing
>>>> the clk_register_* functions (which make it possible for users not
>>>> to use any of the internal struct members). That's what I don't like
>>>> about your patches. But if we go for static initializers anyway then
>>>> your
>>>> patches probably change things for the better.
>>>
>>>
>>> Static initialization is something I have fought for; in fact the
>>> original patches provided no way to do it, so clearly what we have
>>> today is some progress for the folks desiring static init.
>>
>>
>> I too desire static init. Sorry if I was unclear and gave people the
>> misconception that I wanted to remove static init.
>>
>>> The patch
>>> above doesn't actually prevent allocation from happening as it still
>>> must call into clk_register and kmalloc struct clk,
>>
>>
>> Correct.
>>
>>> so besides
>>> readability, I'm not sure what these patches buy us.
>>
>>
>> I think readability is very important and if this buys us nothing but
>> readability, we should still take this patch. But there are other
>> benefits too -- I mentioned them in the commit text.
>>
>>> Assuming that C99 designated initializers (for the sole purpose of
>>> readability) is the main draw of the above patch, please let me know
>>> what you think about modifying the existing static init macros so that
>>> your clock data looks like this:
>>>
>>> DEFINE_CLK_DIVIDER(dpll_iva_m5x2_ck,&dpll_iva_x2_ck, "dpll_iva_x2_ck",
>>> .flags = 0x0,
>>> .reg = OMAP4430_CM_DIV_M5_DPLL_IVA,
>>> .shift = OMAP4430_HSDIVIDER_CLKOUT2_DIV_SHIFT,
>>> .width = OMAP4430_HSDIVIDER_CLKOUT2_DIV_WIDTH,
>>> .flags = CLK_DIVIDER_ONE_BASED,
>>> .lock = NULL
>>> );
>>>
>>> Note that the first argument is the name of this clock (and will be
>>> properly stringified for .name = "whatever") and that the second and
>>> third arguments are both the parent clock, used for initializing the
>>> parent pointer and .parent_names, respectively. If that aspect of the
>>> macro is too ugly then those can even be broken out into a separate
>>> macro since they are independent data structure (struct clk **parents,
>>> and char **parent_names). Or you could just open code those data
>>> structures and only use a macro for struct clk and struct clk_foo.
>>>
>>> This gives you the readability of C99 designated initializers while
>>> keeping struct clk's members totally hidden from the rest of the
>>> world.
>>
>>
>> But it still leaves the struct clk exposed to people who do static init
>> of the clock tree. I think agreeing that the name, parent names, flags
>> and ops are not used to hack with or just making a copy of all of them
>> (and mark the originals as __init if that's doable). is a better
>> solution than trying to go with macros and leave struct clk exposed to
>> everyone who want to do static init of the clock tree.
>>
>> At a later point when we are ready to move struct clk inside clk.c, with
>> this patch applied right now, IMX and MSM won't have to churn their code.
>>
>> Thanks,
>> Saravana
>>
>
>
> --
> Sent by an employee of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 2/2] clk: Move init fields from clk to clk_hw

[Saravana Kannan]

On 03/27/2012 11:49 AM, Turquette, Mike wrote:
> On Mon, Mar 26, 2012 at 9:35 PM, Saravana Kannan<skannan@codeaurora.org>  wrote:
>> Mike,
>>
>> (*nudge*) (*nudge*)
>
> Hi Saravana,
>
> I spent some time mulling over the ideas this weekend and I agree that
> we have 3 types of data, so having the three structs makes some sense.
>   I'm checking this patch out more earnestly today.
>

Thanks Mike. The patch has a few things to clean up. I would be glad to 
send out a cleaned up patch once we have an approximate consensus on 
what to copy/not copy.

Thanks,
Saravana

Re: [PATCH 2/2] clk: Move init fields from clk to clk_hw

[Saravana Kannan]

On 03/27/2012 11:49 AM, Turquette, Mike wrote:
> On Mon, Mar 26, 2012 at 9:35 PM, Saravana Kannan<skannan@codeaurora.org>  wrote:
>> Mike,
>>
>> (*nudge*) (*nudge*)
>
> Hi Saravana,
>
> I spent some time mulling over the ideas this weekend and I agree that
> we have 3 types of data, so having the three structs makes some sense.
>   I'm checking this patch out more earnestly today.

(*nudge*) (*nudge*) again :-)

Sorry to keep nudging you on this often. The longer we wait for this, 
the more churn it will cause later on. That's why I'm being persistent 
on this. Also, once this goes in, I can start working on upstreaming MSM 
clock support.

Thanks,
Saravana

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 2/2] clk: Move init fields from clk to clk_hw

[Turquette, Mike]

On Thu, Apr 5, 2012 at 6:30 PM, Saravana Kannan <skannan@codeaurora.org> wrote:
> On 03/27/2012 11:49 AM, Turquette, Mike wrote:
>>
>> On Mon, Mar 26, 2012 at 9:35 PM, Saravana Kannan<skannan@codeaurora.org>
>>  wrote:
>>>
>>> Mike,
>>>
>>> (*nudge*) (*nudge*)
>>
>>
>> Hi Saravana,
>>
>> I spent some time mulling over the ideas this weekend and I agree that
>> we have 3 types of data, so having the three structs makes some sense.
>>  I'm checking this patch out more earnestly today.
>
>
> (*nudge*) (*nudge*) again :-)
>
> Sorry to keep nudging you on this often. The longer we wait for this, the
> more churn it will cause later on. That's why I'm being persistent on this.
> Also, once this goes in, I can start working on upstreaming MSM clock
> support.

Sorry for the long wait Saravana.  I've been swamped the past few days.

I thought about splitting this interface further (basically making the
statically initialized stuff visible to code that includes
clk-provider.h) and it seems like the benefits outweight the negative
stuff.

However I want to advance the cause of __initdata clock initializers,
so I think the final version of this change should have the core
perform a copy of the initializer data into struct clk_hw.  It's been
a while since I looked at Sascha's clk_initalizer patches but I think
that it is essentially the same idea.

TL;DR: I think a combination of your change to expose the
not-so-private parts of struct clk into struct clk_hw are OK and have
real benefits for the clk-provider.h code.  However we need to
implement it such that all the statically initialized data can be
__initdata.

My only concern with this series is that platform clock code will
access struct clk_hw members without the appropriate lock held (namely
prepare_lock).  I'm a bit worried that there might be a case where
some clock code access clk_hw->name when clk_hw has somehow been
destroyed/altered (e.g. if clk_put every finally gets implemented...).
 Besides clk_put are there any real instances where this might happen?

I'll be pushing my fixes branch out to the list soon.  Do you want to
rebase this change on top of it taking into account the __initdata
bits?

Regards,
Mike

Re: [PATCH 2/2] clk: Move init fields from clk to clk_hw

[Saravana Kannan]

On 04/11/2012 10:59 AM, Turquette, Mike wrote:
> On Thu, Apr 5, 2012 at 6:30 PM, Saravana Kannan<skannan@codeaurora.org>  wrote:
>> (*nudge*) (*nudge*) again :-)
>>
>> Sorry to keep nudging you on this often. The longer we wait for this, the
>> more churn it will cause later on. That's why I'm being persistent on this.
>> Also, once this goes in, I can start working on upstreaming MSM clock
>> support.

<SNIP>

> TL;DR: I think a combination of your change to expose the
> not-so-private parts of struct clk into struct clk_hw are OK and have
> real benefits for the clk-provider.h code.  However we need to
> implement it such that all the statically initialized data can be
> __initdata.

Sounds like a reasonable compromise.

> My only concern with this series is that platform clock code will
> access struct clk_hw members without the appropriate lock held (namely
> prepare_lock).  I'm a bit worried that there might be a case where
> some clock code access clk_hw->name when clk_hw has somehow been
> destroyed/altered (e.g. if clk_put every finally gets implemented...).
>   Besides clk_put are there any real instances where this might happen?

This problem is true for any struct/field marked as __init_data. So, I 
don't think this is a real problem. If someone is stupid enough to mark 
their data as __init_data and access them later, then there is not much 
we can do. Also, I believe the compiler throws out some warning when you 
try to access __init_data from non-init code.

> I'll be pushing my fixes branch out to the list soon.  Do you want to
> rebase this change on top of it taking into account the __initdata
> bits?

I thought it might be easier for you to base your changes on top of my 
patch. But I can try to rebase mine on top of your changes. Hopefully 
your fixes aren't crazy big/complex.

This is a busy week for me at work. I will try to send a patch in a day 
or two.

Thanks,
Saravana

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 2/2] clk: Move init fields from clk to clk_hw

[Turquette, Mike]

On Wed, Apr 11, 2012 at 12:57 PM, Saravana Kannan
<skannan@codeaurora.org> wrote:
> On 04/11/2012 10:59 AM, Turquette, Mike wrote:
>> On Thu, Apr 5, 2012 at 6:30 PM, Saravana Kannan<skannan@codeaurora.org>
>> My only concern with this series is that platform clock code will
>> access struct clk_hw members without the appropriate lock held (namely
>> prepare_lock).  I'm a bit worried that there might be a case where
>> some clock code access clk_hw->name when clk_hw has somehow been
>> destroyed/altered (e.g. if clk_put every finally gets implemented...).
>>  Besides clk_put are there any real instances where this might happen?
>
> This problem is true for any struct/field marked as __init_data. So, I don't
> think this is a real problem. If someone is stupid enough to mark their data
> as __init_data and access them later, then there is not much we can do.
> Also, I believe the compiler throws out some warning when you try to access
> __init_data from non-init code.

I'm referring to platform clock code accessing clk_hw->name or
clk_hw->parent_names AFTER the core code has copied the initialization
data.  So this isn't an __initdata problem as much as it is a design
issue with exposing some members of struct clk_hw.  I think that this
is basically OK since the benefits of not having to use so many
helpers functions in platform clock code is obvious and if a platform
gets bitten by this then they will learn the hard way to honor the
locking semantics outlined in Documentation/clk.txt

>> I'll be pushing my fixes branch out to the list soon.  Do you want to
>> rebase this change on top of it taking into account the __initdata
>> bits?
>
> I thought it might be easier for you to base your changes on top of my
> patch. But I can try to rebase mine on top of your changes. Hopefully your
> fixes aren't crazy big/complex.

I don't think the fixes are a big deal, especially in the core code.

> This is a busy week for me at work. I will try to send a patch in a day or
> two.

OK.  I might take a swing at it myself if I have time.

Regards,
Mike

Re: [PATCH 2/2] clk: Move init fields from clk to clk_hw

[Saravana Kannan]

On 04/11/2012 01:17 PM, Turquette, Mike wrote:
> On Wed, Apr 11, 2012 at 12:57 PM, Saravana Kannan
> <skannan@codeaurora.org>  wrote:
>> On 04/11/2012 10:59 AM, Turquette, Mike wrote:
>>> On Thu, Apr 5, 2012 at 6:30 PM, Saravana Kannan<skannan@codeaurora.org>
>>> My only concern with this series is that platform clock code will
>>> access struct clk_hw members without the appropriate lock held (namely
>>> prepare_lock).  I'm a bit worried that there might be a case where
>>> some clock code access clk_hw->name when clk_hw has somehow been
>>> destroyed/altered (e.g. if clk_put every finally gets implemented...).
>>>   Besides clk_put are there any real instances where this might happen?
>>
>> This problem is true for any struct/field marked as __init_data. So, I don't
>> think this is a real problem. If someone is stupid enough to mark their data
>> as __init_data and access them later, then there is not much we can do.
>> Also, I believe the compiler throws out some warning when you try to access
>> __init_data from non-init code.
>
> I'm referring to platform clock code accessing clk_hw->name or
> clk_hw->parent_names AFTER the core code has copied the initialization
> data.  So this isn't an __initdata problem as much as it is a design
> issue with exposing some members of struct clk_hw.  I think that this
> is basically OK since the benefits of not having to use so many
> helpers functions in platform clock code is obvious and if a platform
> gets bitten by this then they will learn the hard way to honor the
> locking semantics outlined in Documentation/clk.txt

That's what I meant too. The platform code and the static init data will 
most likely be written by the same dev. So, hopefully they know what 
they are doing.

>
>>> I'll be pushing my fixes branch out to the list soon.  Do you want to
>>> rebase this change on top of it taking into account the __initdata
>>> bits?
>>
>> I thought it might be easier for you to base your changes on top of my
>> patch. But I can try to rebase mine on top of your changes. Hopefully your
>> fixes aren't crazy big/complex.
>
> I don't think the fixes are a big deal, especially in the core code.
>
>> This is a busy week for me at work. I will try to send a patch in a day or
>> two.
>
> OK.  I might take a swing at it myself if I have time.

I still need to make the changes to add the copying part. So, don't 
bother with taking a swing at it. If you want to help out, help with 
rebasing it on top of your fixes would be appreciated. But based on what 
you said, it should be hard.

TLDR: Just wait for me to send out the patches. :)

-Saravana

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.