[PATCH] clk: Use a separate struct for holding init data.

[PATCH] clk: Use a separate struct for holding init data.

[Saravana Kannan]

Create a struct clk_init_data to hold all data that needs to be passed from
the platfrom specific driver to the common clock framework during clock
registration. Add a pointer to this struct inside clk_hw.

This has several advantages:
* Completely hides struct clk from many clock platform drivers and static
  clock initialization code that don't care for static initialization of
  the struct clks.
* For platforms that want to do complete static initialization, it removed
  the need to directly mess with the struct clk's fields while still
  allowing to statically allocate struct clk. This keeps the code more
  future proof even if they include clk-private.h.
* Simplifies the generic clk_register() function and allows adding optional
  fields in the future without modifying the function signature.
* Simplifies the static initialization of clocks on all platforms by
  removing the need for forward delcarations or convoluted macros.

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    |   14 +++--
 drivers/clk/clk-fixed-rate.c |   14 +++--
 drivers/clk/clk-gate.c       |   15 +++--
 drivers/clk/clk-mux.c        |   10 +++-
 drivers/clk/clk.c            |   91 +++++++++++++++++++------------
 include/linux/clk-private.h  |  121 +-----------------------------------------
 include/linux/clk-provider.h |   59 +++++++++++++-------
 7 files changed, 129 insertions(+), 195 deletions(-)

diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index 90627e4..8ea11b4 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -167,6 +167,7 @@ struct clk *clk_register_divider(struct device *dev, const char *name,
 {
 	struct clk_divider *div;
 	struct clk *clk;
+	struct clk_init_data init;
 
 	/* allocate the divider */
 	div = kzalloc(sizeof(struct clk_divider), GFP_KERNEL);
@@ -175,19 +176,22 @@ struct clk *clk_register_divider(struct device *dev, const char *name,
 		return ERR_PTR(-ENOMEM);
 	}
 
+	init.name = name;
+	init.ops = &clk_divider_ops;
+	init.flags = flags;
+	init.parent_names = (parent_name ? &parent_name: NULL);
+	init.num_parents = (parent_name ? 1 : 0);
+
 	/* struct clk_divider assignments */
 	div->reg = reg;
 	div->shift = shift;
 	div->width = width;
 	div->flags = clk_divider_flags;
 	div->lock = lock;
+	div->hw.init = &init;
 
 	/* register the clock */
-	clk = clk_register(dev, name,
-			&clk_divider_ops, &div->hw,
-			(parent_name ? &parent_name: NULL),
-			(parent_name ? 1 : 0),
-			flags);
+	clk = clk_register(dev, &div->hw);
 
 	if (IS_ERR(clk))
 		kfree(div);
diff --git a/drivers/clk/clk-fixed-rate.c b/drivers/clk/clk-fixed-rate.c
index b555a04..cbd2462 100644
--- a/drivers/clk/clk-fixed-rate.c
+++ b/drivers/clk/clk-fixed-rate.c
@@ -52,6 +52,7 @@ struct clk *clk_register_fixed_rate(struct device *dev, const char *name,
 {
 	struct clk_fixed_rate *fixed;
 	struct clk *clk;
+	struct clk_init_data init;
 
 	/* allocate fixed-rate clock */
 	fixed = kzalloc(sizeof(struct clk_fixed_rate), GFP_KERNEL);
@@ -60,15 +61,18 @@ struct clk *clk_register_fixed_rate(struct device *dev, const char *name,
 		return ERR_PTR(-ENOMEM);
 	}
 
+	init.name = name;
+	init.ops = &clk_fixed_rate_ops;
+	init.flags = flags;
+	init.parent_names = (parent_name ? &parent_name: NULL);
+	init.num_parents = (parent_name ? 1 : 0);
+
 	/* struct clk_fixed_rate assignments */
 	fixed->fixed_rate = fixed_rate;
+	fixed->hw.init = &init;
 
 	/* register the clock */
-	clk = clk_register(dev, name,
-			&clk_fixed_rate_ops, &fixed->hw,
-			(parent_name ? &parent_name : NULL),
-			(parent_name ? 1 : 0),
-			flags);
+	clk = clk_register(dev, &fixed->hw);
 
 	if (IS_ERR(clk))
 		kfree(fixed);
diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c
index 0021616..578465e 100644
--- a/drivers/clk/clk-gate.c
+++ b/drivers/clk/clk-gate.c
@@ -119,6 +119,7 @@ struct clk *clk_register_gate(struct device *dev, const char *name,
 {
 	struct clk_gate *gate;
 	struct clk *clk;
+	struct clk_init_data init;
 
 	/* allocate the gate */
 	gate = kzalloc(sizeof(struct clk_gate), GFP_KERNEL);
@@ -127,18 +128,20 @@ struct clk *clk_register_gate(struct device *dev, const char *name,
 		return ERR_PTR(-ENOMEM);
 	}
 
+	init.name = name;
+	init.ops = &clk_gate_ops;
+	init.flags = flags;
+	init.parent_names = (parent_name ? &parent_name: NULL);
+	init.num_parents = (parent_name ? 1 : 0);
+
 	/* struct clk_gate assignments */
 	gate->reg = reg;
 	gate->bit_idx = bit_idx;
 	gate->flags = clk_gate_flags;
 	gate->lock = lock;
+	gate->hw.init = &init;
 
-	/* register the clock */
-	clk = clk_register(dev, name,
-			&clk_gate_ops, &gate->hw,
-			(parent_name ? &parent_name : NULL),
-			(parent_name ? 1 : 0),
-			flags);
+	clk = clk_register(dev, &gate->hw);
 
 	if (IS_ERR(clk))
 		kfree(gate);
diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c
index 6e58f11..8e97491 100644
--- a/drivers/clk/clk-mux.c
+++ b/drivers/clk/clk-mux.c
@@ -95,6 +95,7 @@ struct clk *clk_register_mux(struct device *dev, const char *name,
 {
 	struct clk_mux *mux;
 	struct clk *clk;
+	struct clk_init_data init;
 
 	/* allocate the mux */
 	mux = kzalloc(sizeof(struct clk_mux), GFP_KERNEL);
@@ -103,6 +104,12 @@ struct clk *clk_register_mux(struct device *dev, const char *name,
 		return ERR_PTR(-ENOMEM);
 	}
 
+	init.name = name;
+	init.ops = &clk_mux_ops;
+	init.flags = flags;
+	init.parent_names = parent_names;
+	init.num_parents = num_parents;
+
 	/* struct clk_mux assignments */
 	mux->reg = reg;
 	mux->shift = shift;
@@ -110,8 +117,7 @@ struct clk *clk_register_mux(struct device *dev, const char *name,
 	mux->flags = clk_mux_flags;
 	mux->lock = lock;
 
-	clk = clk_register(dev, name, &clk_mux_ops, &mux->hw,
-			parent_names, num_parents, flags);
+	clk = clk_register(dev, &mux->hw);
 
 	if (IS_ERR(clk))
 		kfree(mux);
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 2dd20c0..97a2c91 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1169,28 +1169,8 @@ EXPORT_SYMBOL_GPL(clk_set_parent);
  *
  * Initializes the lists in struct clk, queries the hardware for the
  * parent and rate and sets them both.
- *
- * Any struct clk passed into __clk_init must have the following members
- * populated:
- * 	.name
- * 	.ops
- * 	.hw
- * 	.parent_names
- * 	.num_parents
- * 	.flags
- *
- * Essentially, everything that would normally be passed into clk_register is
- * assumed to be initialized already in __clk_init.  The other members may be
- * populated, but are optional.
- *
- * __clk_init is only exposed via clk-private.h and is intended for use with
- * very large numbers of clocks that need to be statically initialized.  It is
- * a layering violation to include clk-private.h from any code which implements
- * a clock's .ops; as such any statically initialized clock data MUST be in a
- * separate C file from the logic that implements it's operations.  Returns 0
- * on success, otherwise an error code.
  */
-int __clk_init(struct device *dev, struct clk *clk)
+static int __clk_init(struct device *dev, struct clk *clk)
 {
 	int i, ret = 0;
 	struct clk *orphan;
@@ -1321,14 +1301,47 @@ out:
 }
 
 /**
+ * __clk_register - register a clock and return a cookie.
+ *
+ * Same as clk_register, except that the .clk field inside hw shall point to a
+ * preallocated (generally statically allocated) struct clk. None of the fields
+ * of the struct clk need to be initialized.
+ *
+ * The data pointed to by .init and .clk field shall NOT be marked as init
+ * data.
+ *
+ * __clk_register is only exposed via clk-private.h and is intended for use with
+ * very large numbers of clocks that need to be statically initialized.  It is
+ * a layering violation to include clk-private.h from any code which implements
+ * a clock's .ops; as such any statically initialized clock data MUST be in a
+ * separate C file from the logic that implements it's operations.  Returns 0
+ * on success, otherwise an error code.
+ */
+struct clk *__clk_register(struct device *dev, struct clk_hw *hw)
+{
+	int ret;
+	struct clk *clk;
+
+	clk = hw->clk;
+	clk->name = hw->init->name;
+	clk->ops = hw->init->ops;
+	clk->hw = hw;
+	clk->flags = hw->init->flags;
+	clk->parent_names = hw->init->parent_names;
+	clk->num_parents = hw->init->num_parents;
+
+	ret = __clk_init(dev, clk);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return clk;
+}
+EXPORT_SYMBOL_GPL(__clk_register);
+
+/**
  * clk_register - allocate a new clock, register it and return an opaque cookie
  * @dev: device that is registering this clock
- * @name: clock name
- * @ops: operations this clock supports
  * @hw: link to hardware-specific clock data
- * @parent_names: array of string names for all possible parents
- * @num_parents: number of possible parents
- * @flags: framework-level hints and quirks
  *
  * clk_register is the primary interface for populating the clock tree with new
  * clock nodes.  It returns a pointer to the newly allocated struct clk which
@@ -1336,9 +1349,7 @@ out:
  * rest of the clock API.  In the event of an error clk_register will return an
  * error code; drivers must test for an error code after calling clk_register.
  */
-struct clk *clk_register(struct device *dev, const char *name,
-		const struct clk_ops *ops, struct clk_hw *hw,
-		const char **parent_names, u8 num_parents, unsigned long flags)
+struct clk *clk_register(struct device *dev, struct clk_hw *hw)
 {
 	int i, ret;
 	struct clk *clk;
@@ -1350,15 +1361,20 @@ struct clk *clk_register(struct device *dev, const char *name,
 		goto fail_out;
 	}
 
-	clk->name = name;
-	clk->ops = ops;
+	clk->name = kstrdup(hw->init->name, GFP_KERNEL);
+	if (!clk->name) {
+		pr_err("%s: could not allocate clk->name\n", __func__);
+		ret = -ENOMEM;
+		goto fail_name;
+	}
+	clk->ops = hw->init->ops;
 	clk->hw = hw;
-	clk->flags = flags;
-	clk->num_parents = num_parents;
+	clk->flags = hw->init->flags;
+	clk->num_parents = hw->init->num_parents;
 	hw->clk = clk;
 
 	/* allocate local copy in case parent_names is __initdata */
-	clk->parent_names = kzalloc((sizeof(char*) * num_parents),
+	clk->parent_names = kzalloc((sizeof(char*) * clk->num_parents),
 			GFP_KERNEL);
 
 	if (!clk->parent_names) {
@@ -1369,8 +1385,9 @@ struct clk *clk_register(struct device *dev, const char *name,
 
 
 	/* copy each string name in case parent_names is __initdata */
-	for (i = 0; i < num_parents; i++) {
-		clk->parent_names[i] = kstrdup(parent_names[i], GFP_KERNEL);
+	for (i = 0; i < clk->num_parents; i++) {
+		clk->parent_names[i] = kstrdup(hw->init->parent_names[i],
+						GFP_KERNEL);
 		if (!clk->parent_names[i]) {
 			pr_err("%s: could not copy parent_names\n", __func__);
 			ret = -ENOMEM;
@@ -1387,6 +1404,8 @@ fail_parent_names_copy:
 		kfree(clk->parent_names[i]);
 	kfree(clk->parent_names);
 fail_parent_names:
+	kfree(clk->name);
+fail_name:
 	kfree(clk);
 fail_out:
 	return ERR_PTR(ret);
diff --git a/include/linux/clk-private.h b/include/linux/clk-private.h
index eeae7a3..316bad7 100644
--- a/include/linux/clk-private.h
+++ b/include/linux/clk-private.h
@@ -46,126 +46,7 @@ struct clk {
 #endif
 };
 
-/*
- * DOC: Basic clock implementations common to many platforms
- *
- * Each basic clock hardware type is comprised of a structure describing the
- * clock hardware, implementations of the relevant callbacks in struct clk_ops,
- * unique flags for that hardware type, a registration function and an
- * alternative macro for static initialization
- */
-
-#define DEFINE_CLK(_name, _ops, _flags, _parent_names,		\
-		_parents)					\
-	static struct clk _name = {				\
-		.name = #_name,					\
-		.ops = &_ops,					\
-		.hw = &_name##_hw.hw,				\
-		.parent_names = _parent_names,			\
-		.num_parents = ARRAY_SIZE(_parent_names),	\
-		.parents = _parents,				\
-		.flags = _flags,				\
-	}
-
-#define DEFINE_CLK_FIXED_RATE(_name, _flags, _rate,		\
-				_fixed_rate_flags)		\
-	static struct clk _name;				\
-	static char *_name##_parent_names[] = {};		\
-	static struct clk_fixed_rate _name##_hw = {		\
-		.hw = {						\
-			.clk = &_name,				\
-		},						\
-		.fixed_rate = _rate,				\
-		.flags = _fixed_rate_flags,			\
-	};							\
-	DEFINE_CLK(_name, clk_fixed_rate_ops, _flags,		\
-			_name##_parent_names, NULL);
-
-#define DEFINE_CLK_GATE(_name, _parent_name, _parent_ptr,	\
-				_flags, _reg, _bit_idx,		\
-				_gate_flags, _lock)		\
-	static struct clk _name;				\
-	static char *_name##_parent_names[] = {			\
-		_parent_name,					\
-	};							\
-	static struct clk *_name##_parents[] = {		\
-		_parent_ptr,					\
-	};							\
-	static struct clk_gate _name##_hw = {			\
-		.hw = {						\
-			.clk = &_name,				\
-		},						\
-		.reg = _reg,					\
-		.bit_idx = _bit_idx,				\
-		.flags = _gate_flags,				\
-		.lock = _lock,					\
-	};							\
-	DEFINE_CLK(_name, clk_gate_ops, _flags,			\
-			_name##_parent_names, _name##_parents);
-
-#define DEFINE_CLK_DIVIDER(_name, _parent_name, _parent_ptr,	\
-				_flags, _reg, _shift, _width,	\
-				_divider_flags, _lock)		\
-	static struct clk _name;				\
-	static char *_name##_parent_names[] = {			\
-		_parent_name,					\
-	};							\
-	static struct clk *_name##_parents[] = {		\
-		_parent_ptr,					\
-	};							\
-	static struct clk_divider _name##_hw = {		\
-		.hw = {						\
-			.clk = &_name,				\
-		},						\
-		.reg = _reg,					\
-		.shift = _shift,				\
-		.width = _width,				\
-		.flags = _divider_flags,			\
-		.lock = _lock,					\
-	};							\
-	DEFINE_CLK(_name, clk_divider_ops, _flags,		\
-			_name##_parent_names, _name##_parents);
-
-#define DEFINE_CLK_MUX(_name, _parent_names, _parents, _flags,	\
-				_reg, _shift, _width,		\
-				_mux_flags, _lock)		\
-	static struct clk _name;				\
-	static struct clk_mux _name##_hw = {			\
-		.hw = {						\
-			.clk = &_name,				\
-		},						\
-		.reg = _reg,					\
-		.shift = _shift,				\
-		.width = _width,				\
-		.flags = _mux_flags,				\
-		.lock = _lock,					\
-	};							\
-	DEFINE_CLK(_name, clk_mux_ops, _flags, _parent_names,	\
-			_parents);
-
-/**
- * __clk_init - initialize the data structures in a struct clk
- * @dev:	device initializing this clk, placeholder for now
- * @clk:	clk being initialized
- *
- * Initializes the lists in struct clk, queries the hardware for the
- * parent and rate and sets them both.
- *
- * Any struct clk passed into __clk_init must have the following members
- * populated:
- * 	.name
- * 	.ops
- * 	.hw
- * 	.parent_names
- * 	.num_parents
- * 	.flags
- *
- * It is not necessary to call clk_register if __clk_init is used directly with
- * statically initialized clock data.
- *
- * Returns 0 on success, otherwise an error code.
- */
-int __clk_init(struct device *dev, struct clk *clk);
+struct clk *__clk_register(struct device *dev, struct clk_hw *hw);
 
 #endif /* CONFIG_COMMON_CLK */
 #endif /* CLK_PRIVATE_H */
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 8f21489..5db3412 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -15,19 +15,6 @@
 
 #ifdef CONFIG_COMMON_CLK
 
-/**
- * struct clk_hw - handle for traversing from a struct clk to its corresponding
- * hardware-specific structure.  struct clk_hw should be declared within struct
- * clk_foo and then referenced by the struct clk instance that uses struct
- * clk_foo's clk_ops
- *
- * clk: pointer to the struct clk instance that points back to this struct
- * clk_hw instance
- */
-struct clk_hw {
-	struct clk *clk;
-};
-
 /*
  * flags used across common struct clk.  these flags should only affect the
  * top-level framework.  custom flags for dealing with hardware specifics
@@ -39,6 +26,8 @@ struct clk_hw {
 #define CLK_IGNORE_UNUSED	BIT(3) /* do not gate even if unused */
 #define CLK_IS_ROOT		BIT(4) /* root clk, has no parent */
 
+struct clk_hw;
+
 /**
  * struct clk_ops -  Callback operations for hardware clocks; these are to
  * be provided by the clock implementation, and will be called by drivers
@@ -122,6 +111,41 @@ struct clk_ops {
 	void		(*init)(struct clk_hw *hw);
 };
 
+/**
+ * struct clk_init_data - holds init data that's common to all clocks and is
+ * shared between the clock provider and the common clock framework.
+ *
+ * @name: clock name
+ * @ops: operations this clock supports
+ * @parent_names: array of string names for all possible parents
+ * @num_parents: number of possible parents
+ * @flags: framework-level hints and quirks
+ */
+struct clk_init_data {
+	const char		*name;
+	const struct clk_ops	*ops;
+	const char		**parent_names;
+	u8			num_parents;
+	unsigned long		flags;
+};
+
+/**
+ * struct clk_hw - handle for traversing from a struct clk to its corresponding
+ * hardware-specific structure.  struct clk_hw should be declared within struct
+ * clk_foo and then referenced by the struct clk instance that uses struct
+ * clk_foo's clk_ops
+ *
+ * @clk: pointer to the struct clk instance that points back to this struct
+ * clk_hw instance
+ *
+ * @init: pointer to struct clk_init_data that contains the init data shared
+ * with the common clock framework.
+ */
+struct clk_hw {
+	struct clk *clk;
+	struct clk_init_data *init;
+};
+
 /*
  * DOC: Basic clock implementations common to many platforms
  *
@@ -255,12 +279,7 @@ struct clk *clk_register_mux(struct device *dev, const char *name,
 /**
  * clk_register - allocate a new clock, register it and return an opaque cookie
  * @dev: device that is registering this clock
- * @name: clock name
- * @ops: operations this clock supports
  * @hw: link to hardware-specific clock data
- * @parent_names: array of string names for all possible parents
- * @num_parents: number of possible parents
- * @flags: framework-level hints and quirks
  *
  * clk_register is the primary interface for populating the clock tree with new
  * clock nodes.  It returns a pointer to the newly allocated struct clk which
@@ -268,9 +287,7 @@ struct clk *clk_register_mux(struct device *dev, const char *name,
  * rest of the clock API.  In the event of an error clk_register will return an
  * error code; drivers must test for an error code after calling clk_register.
  */
-struct clk *clk_register(struct device *dev, const char *name,
-		const struct clk_ops *ops, struct clk_hw *hw,
-		const 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

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] clk: Use a separate struct for holding init data.

[Saravana Kannan]

On 04/25/2012 10:58 PM, Saravana Kannan wrote:
> Create a struct clk_init_data to hold all data that needs to be passed from
> the platfrom specific driver to the common clock framework during clock
> registration. Add a pointer to this struct inside clk_hw.
>
> This has several advantages:
> * Completely hides struct clk from many clock platform drivers and static
>    clock initialization code that don't care for static initialization of
>    the struct clks.
> * For platforms that want to do complete static initialization, it removed
>    the need to directly mess with the struct clk's fields while still
>    allowing to statically allocate struct clk. This keeps the code more
>    future proof even if they include clk-private.h.
> * Simplifies the generic clk_register() function and allows adding optional
>    fields in the future without modifying the function signature.
> * Simplifies the static initialization of clocks on all platforms by
>    removing the need for forward delcarations or convoluted macros.

<SNIP>

> diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
> index 90627e4..8ea11b4 100644
> --- a/drivers/clk/clk-divider.c
> +++ b/drivers/clk/clk-divider.c
> @@ -167,6 +167,7 @@ struct clk *clk_register_divider(struct device *dev, const char *name,
>   {

<SNIP>

> diff --git a/drivers/clk/clk-fixed-rate.c b/drivers/clk/clk-fixed-rate.c
> index b555a04..cbd2462 100644
> --- a/drivers/clk/clk-fixed-rate.c
> +++ b/drivers/clk/clk-fixed-rate.c
> @@ -52,6 +52,7 @@ struct clk *clk_register_fixed_rate(struct device *dev, const char *name,
>   {

<SNIP>

> diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c
> index 0021616..578465e 100644
> --- a/drivers/clk/clk-gate.c
> +++ b/drivers/clk/clk-gate.c
> @@ -119,6 +119,7 @@ struct clk *clk_register_gate(struct device *dev, const char *name,
>   {

<SNIP>

> diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c
> index 6e58f11..8e97491 100644
> --- a/drivers/clk/clk-mux.c
> +++ b/drivers/clk/clk-mux.c
> @@ -95,6 +95,7 @@ struct clk *clk_register_mux(struct device *dev, const char *name,
>   {

I would really like to remove these functions. At least until we add 
device tree support where each clock is listed in device tree.

At present, these functions seem to be abused more than actually being 
used appropriately. IMHO, these should not be used to register clocks in 
your probe function by calling these functions one at a time. Just 
declare the clocks statically and call clk_register in a loop on them.

The only time I see these functions as being appropriate is when the 
list of clocks in your device is detected by actually probing (reading 
registers) HW or by parsing device tree (haven't followed it closely, 
but still seems to be work in progress).

Regards,
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] clk: Use a separate struct for holding init data.

[Sascha Hauer]

On Wed, Apr 25, 2012 at 10:58:56PM -0700, Saravana Kannan wrote:
> Create a struct clk_init_data to hold all data that needs to be passed from
> the platfrom specific driver to the common clock framework during clock
> registration. Add a pointer to this struct inside clk_hw.
> 
> This has several advantages:
> * Completely hides struct clk from many clock platform drivers and static
>   clock initialization code that don't care for static initialization of
>   the struct clks.
> * For platforms that want to do complete static initialization, it removed
>   the need to directly mess with the struct clk's fields while still
>   allowing to statically allocate struct clk. This keeps the code more
>   future proof even if they include clk-private.h.
> * Simplifies the generic clk_register() function and allows adding optional
>   fields in the future without modifying the function signature.
> * Simplifies the static initialization of clocks on all platforms by
>   removing the need for forward delcarations or convoluted macros.

Can we please stop messing with the function prototypes? So you prefer
passing a struct to clk_register which is fine and yes, it may have
advantages. But do we really need to change the prototype? Why can't we
just add a new function?

I am generally open to do these changes, but we have come to the point
where people actually want to *use* the clock framework instead of
rebasing their stuff onto the latest patches.

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] clk: Use a separate struct for holding init data.

[Sascha Hauer]

On Wed, Apr 25, 2012 at 11:28:32PM -0700, Saravana Kannan wrote:
> 
> >diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c
> >index 6e58f11..8e97491 100644
> >--- a/drivers/clk/clk-mux.c
> >+++ b/drivers/clk/clk-mux.c
> >@@ -95,6 +95,7 @@ struct clk *clk_register_mux(struct device *dev, const char *name,
> >  {
> 
> I would really like to remove these functions. At least until we add
> device tree support where each clock is listed in device tree.
> 
> At present, these functions seem to be abused more than actually
> being used appropriately.

I think this goes in my direction. Still exactly this abuse how you call
it makes me relatively independent of for example the changes you
introduce with your patch. Had I static initializers I would now have
to start a rebase orgy.

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] clk: Use a separate struct for holding init data.

[Saravana Kannan]

On Thu, April 26, 2012 1:39 am, Sascha Hauer wrote:
> On Wed, Apr 25, 2012 at 10:58:56PM -0700, Saravana Kannan wrote:
>> Create a struct clk_init_data to hold all data that needs to be passed
>> from
>> the platfrom specific driver to the common clock framework during clock
>> registration. Add a pointer to this struct inside clk_hw.
>>
>> This has several advantages:
>> * Completely hides struct clk from many clock platform drivers and
>> static
>>   clock initialization code that don't care for static initialization of
>>   the struct clks.
>> * For platforms that want to do complete static initialization, it
>> removed
>>   the need to directly mess with the struct clk's fields while still
>>   allowing to statically allocate struct clk. This keeps the code more
>>   future proof even if they include clk-private.h.
>> * Simplifies the generic clk_register() function and allows adding
>> optional
>>   fields in the future without modifying the function signature.
>> * Simplifies the static initialization of clocks on all platforms by
>>   removing the need for forward delcarations or convoluted macros.
>
> Can we please stop messing with the function prototypes? So you prefer
> passing a struct to clk_register which is fine and yes, it may have
> advantages. But do we really need to change the prototype? Why can't we
> just add a new function?

I thought you were using functions that are specific to the clock type and
not the clk_register function. That's pretty much the only reason I left
in the other functions. I was trying to reduce the first level of churn
for people where had already started using the common clock framework.

> I am generally open to do these changes, but we have come to the point
> where people actually want to *use* the clock framework instead of
> rebasing their stuff onto the latest patches.

This is pretty early on in the life of the common clock framework. So, I
don't think this clean up is unjustified. Again, I left the other
functions as is because people might be using it.

-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] clk: Use a separate struct for holding init data.

[Saravana Kannan]

On Thu, April 26, 2012 1:42 am, Sascha Hauer wrote:
> On Wed, Apr 25, 2012 at 11:28:32PM -0700, Saravana Kannan wrote:
>>
>> >diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c
>> >index 6e58f11..8e97491 100644
>> >--- a/drivers/clk/clk-mux.c
>> >+++ b/drivers/clk/clk-mux.c
>> >@@ -95,6 +95,7 @@ struct clk *clk_register_mux(struct device *dev,
>> const char *name,
>> >  {
>>
>> I would really like to remove these functions. At least until we add
>> device tree support where each clock is listed in device tree.
>>
>> At present, these functions seem to be abused more than actually
>> being used appropriately.
>
> I think this goes in my direction. Still exactly this abuse how you call
> it makes me relatively independent of for example the changes you
> introduce with your patch. Had I static initializers I would now have
> to start a rebase orgy.

In the other email you say you have to change. Here you say, you don't
have to change. Hopefully, you didn't have to change much -- I was aiming
for that. If there was agreement about removing these functions, I was
planning on helping move the current users after this patch merged.

I think in the long run this will result in less changes for you and more
readable code. If clk_register() adds another optional param, you can't
get around that without having to write more wrapper functions or changing
any existing ones you might have. But with this struct, the common clock
code can be written in a way so that the a value of 0 for the new param
defaults to the behavior that was there before the param was added.

Something to think about: With these wrapper calls, one would do a lot of
kalloc and copying of small items when one knows at compile time what the
clocks are going to be.

Anyway, I understand that some people see value in this. That's why I'm
bringing it up for discussion instead of just doing it in my patch.

-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] clk: Use a separate struct for holding init data.

[Mark Brown]

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

On Thu, Apr 26, 2012 at 10:39:24AM +0200, Sascha Hauer wrote:

> Can we please stop messing with the function prototypes? So you prefer
> passing a struct to clk_register which is fine and yes, it may have
> advantages. But do we really need to change the prototype? Why can't we
> just add a new function?

> I am generally open to do these changes, but we have come to the point
> where people actually want to *use* the clock framework instead of
> rebasing their stuff onto the latest patches.

Or at least wait until we've got somewhere with applying drivers so that
whoever is changing the APIs is responsible for updating at least the
in-tree drivers.  This would minimise the pain for people who've been
sitting waiting to get their stuff in which seems helpful.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] clk: Use a separate struct for holding init data.

[Sascha Hauer]

On Thu, Apr 26, 2012 at 02:36:37AM -0700, Saravana Kannan wrote:
> 
> On Thu, April 26, 2012 1:42 am, Sascha Hauer wrote:
> > On Wed, Apr 25, 2012 at 11:28:32PM -0700, Saravana Kannan wrote:
> >>
> >> >diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c
> >> >index 6e58f11..8e97491 100644
> >> >--- a/drivers/clk/clk-mux.c
> >> >+++ b/drivers/clk/clk-mux.c
> >> >@@ -95,6 +95,7 @@ struct clk *clk_register_mux(struct device *dev,
> >> const char *name,
> >> >  {
> >>
> >> I would really like to remove these functions. At least until we add
> >> device tree support where each clock is listed in device tree.
> >>
> >> At present, these functions seem to be abused more than actually
> >> being used appropriately.
> >
> > I think this goes in my direction. Still exactly this abuse how you call
> > it makes me relatively independent of for example the changes you
> > introduce with your patch. Had I static initializers I would now have
> > to start a rebase orgy.
> 
> In the other email you say you have to change. Here you say, you don't
> have to change. Hopefully, you didn't have to change much

I don't have to change much, still there are changes since I also use
clk_register. I will do the changes if we agree on your patch, but I
really hope this is the last of such changes.

> -- I was aiming for that.

Thank you for this.

> If there was agreement about removing these functions, I was
> planning on helping move the current users after this patch merged.

Please understand that I begin to get frustrated. I was really happy to
see the clock framework merged in the last window. Now it's -rc4 already
and there are still patches flowing that delay the merge of SoC support.

> 
> I think in the long run this will result in less changes for you and more
> readable code. If clk_register() adds another optional param, you can't
> get around that without having to write more wrapper functions or changing
> any existing ones you might have. But with this struct, the common clock
> code can be written in a way so that the a value of 0 for the new param
> defaults to the behavior that was there before the param was added.
> 
> Something to think about: With these wrapper calls, one would do a lot of
> kalloc and copying of small items when one knows at compile time what the
> clocks are going to be.

We do not know during compile what clocks will be since we do not know
at compile time which SoC we are going to run on. Statically allocated
clocks (which are going to be used directly instead of making copies)
may be fine for someone do a build for one SoC only, but I think our
goal is to build a multi SoC and eventually even a multi architecture
kernel.

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] clk: Use a separate struct for holding init data.

[Saravana Kannan]

On 04/26/2012 02:51 AM, Sascha Hauer wrote:
> On Thu, Apr 26, 2012 at 02:36:37AM -0700, Saravana Kannan wrote:
>>
>> On Thu, April 26, 2012 1:42 am, Sascha Hauer wrote:
>>> On Wed, Apr 25, 2012 at 11:28:32PM -0700, Saravana Kannan wrote:
>>>>
>>>>> diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c
>>>>> index 6e58f11..8e97491 100644
>>>>> --- a/drivers/clk/clk-mux.c
>>>>> +++ b/drivers/clk/clk-mux.c
>>>>> @@ -95,6 +95,7 @@ struct clk *clk_register_mux(struct device *dev,
>>>> const char *name,
>>>>>   {
>>>>
>>>> I would really like to remove these functions. At least until we add
>>>> device tree support where each clock is listed in device tree.
>>>>
>>>> At present, these functions seem to be abused more than actually
>>>> being used appropriately.
>>>
>>> I think this goes in my direction. Still exactly this abuse how you call
>>> it makes me relatively independent of for example the changes you
>>> introduce with your patch. Had I static initializers I would now have
>>> to start a rebase orgy.
>>
>> In the other email you say you have to change. Here you say, you don't
>> have to change. Hopefully, you didn't have to change much
>
> I don't have to change much, still there are changes since I also use
> clk_register. I will do the changes if we agree on your patch, but I
> really hope this is the last of such changes.
>
>> -- I was aiming for that.
>
> Thank you for this.
>
>> If there was agreement about removing these functions, I was
>> planning on helping move the current users after this patch merged.
>
> Please understand that I begin to get frustrated. I was really happy to
> see the clock framework merged in the last window. Now it's -rc4 already
> and there are still patches flowing that delay the merge of SoC support.

Mike, Shawn, Paul, Rob,

Comments? Can we pull this in?

-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] clk: Use a separate struct for holding init data.

[Turquette, Mike]

On Mon, Apr 30, 2012 at 12:30 PM, Saravana Kannan
<skannan@codeaurora.org> wrote:
> Mike, Shawn, Paul, Rob,
>
> Comments? Can we pull this in?

Saravana,

I will test this a bit more thoroughly tomorrow and get back to you.

Thanks,
Mike

> -Saravana
>
> --
> Sent by an employee of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] clk: Use a separate struct for holding init data.

[Saravana Kannan]

On 04/30/2012 03:19 PM, Turquette, Mike wrote:
> On Mon, Apr 30, 2012 at 12:30 PM, Saravana Kannan
> <skannan@codeaurora.org>  wrote:
>> Mike, Shawn, Paul, Rob,
>>
>> Comments? Can we pull this in?
>
> Saravana,
>
> I will test this a bit more thoroughly tomorrow and get back to you.
>
> Thanks,
> Mike

Thanks Mike!

I'm still hoping a Ack/Nack for the general idea from the others.

-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] clk: Use a separate struct for holding init data.

[Shawn Guo]

On Mon, Apr 30, 2012 at 03:46:49PM -0700, Saravana Kannan wrote:
> I'm still hoping a Ack/Nack for the general idea from the others.
> 
I believe that I have Acked the idea when you proposed it at the first
time.  What I really hoped is you can post the patch at least 1 week
earlier.  Basically I share the same frustration that Sascha has, the
platform porting have been delayed by flowing changes on the core code.
It's been -rc5, but we have not got a stable core base to have platform
porting expose on linux-next.

Anyway, since I agree this is the right direction for the long run, I
will revisit my platform porting one more time once Mike applies the
patch.

-- 
Regards,
Shawn

Re: [PATCH] clk: Use a separate struct for holding init data.

[Andrew Lunn]

On Tue, May 01, 2012 at 04:11:05PM +0800, Shawn Guo wrote:
> On Mon, Apr 30, 2012 at 03:46:49PM -0700, Saravana Kannan wrote:
> > I'm still hoping a Ack/Nack for the general idea from the others.
> > 
> I believe that I have Acked the idea when you proposed it at the first
> time.  What I really hoped is you can post the patch at least 1 week
> earlier.  Basically I share the same frustration that Sascha has, the
> platform porting have been delayed by flowing changes on the core code.
> It's been -rc5, but we have not got a stable core base to have platform
> porting expose on linux-next.

Hi folks

I agree with you as well, it is frustrating. Could we agree, that once
this patch is in, we freeze the core until the start of the next
cycle. We use the remainder of this cycle for porting platforms to the
generic clock framework.

	  Andrew

Re: [PATCH] clk: Use a separate struct for holding init data.

[Mark Brown]

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

On Tue, May 01, 2012 at 11:13:34AM +0200, Andrew Lunn wrote:

> I agree with you as well, it is frustrating. Could we agree, that once
> this patch is in, we freeze the core until the start of the next
> cycle. We use the remainder of this cycle for porting platforms to the
> generic clock framework.

Or merge the platforms then do framework changes incrementally, updating
the platforms as we go (which is the normal pattern for maintaining a
framework...).  I see we've already got SPEAr merged.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] clk: Use a separate struct for holding init data.

[Saravana Kannan]

On 05/01/2012 10:00 AM, Mark Brown wrote:
> On Tue, May 01, 2012 at 11:13:34AM +0200, Andrew Lunn wrote:
>
>> I agree with you as well, it is frustrating. Could we agree, that once
>> this patch is in, we freeze the core until the start of the next
>> cycle. We use the remainder of this cycle for porting platforms to the
>> generic clock framework.
>
> Or merge the platforms then do framework changes incrementally, updating
> the platforms as we go (which is the normal pattern for maintaining a
> framework...).  I see we've already got SPEAr merged.

Sorry for the annoyance I seem to have caused. I too have been trying to 
get this in for a while before the other platforms started using the new 
framework. Not everyone was free at the same time and it's taken longer 
that I would have wished for.

I did my best to limit the changes that would be needed without making 
my patch useless. Appreciate your understanding.

Regards,
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] clk: Use a separate struct for holding init data.

[Andrew Lunn]

> Or merge the platforms then do framework changes incrementally, updating
> the platforms as we go (which is the normal pattern for maintaining a
> framework...).  I see we've already got SPEAr merged.

I'm not too sure SPEAr has been really merged. As far as i understand,
its dependencies have not been fulfilled, so it does not compile. This
to me is another indication of the problems we have at the moment...

   Andrew

Re: [PATCH] clk: Use a separate struct for holding init data.

[Mark Brown]

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

On Tue, May 01, 2012 at 11:03:57AM -0700, Saravana Kannan wrote:

> Sorry for the annoyance I seem to have caused. I too have been
> trying to get this in for a while before the other platforms started
> using the new framework. Not everyone was free at the same time and
> it's taken longer that I would have wished for.

> I did my best to limit the changes that would be needed without
> making my patch useless. Appreciate your understanding.

To be honest it doesn't look like your patch is a particular issue here
- there's wider process problems, for example we've managed to go
through most of the release cycle and so far the only changes showing up
in -next are:

Viresh Kumar (6):
      SPEAr: clk: Add VCO-PLL Synthesizer clock
      SPEAr: clk: Add Auxiliary Synthesizer clock
      SPEAr: clk: Add Fractional Synthesizer clock
      SPEAr: clk: Add General Purpose Timer Synthesizer clock
      SPEAr: Switch to common clock framework
      SPEAr13xx: Add common clock framework support

Mark Brown (1):
      ARM: 7376/1: clkdev: Implement managed clk_get()

Sascha Hauer (1):
      clk: add a fixed factor clock

viresh kumar (1):
      ARM: 7392/1: CLKDEV: Optimize clk_find()

and obviously there's quite a bit more work which has been going on.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] clk: Use a separate struct for holding init data.

[Mike Turquette]

On 20120501-19:19, Mark Brown wrote:
> On Tue, May 01, 2012 at 11:03:57AM -0700, Saravana Kannan wrote:
> 
> > Sorry for the annoyance I seem to have caused. I too have been
> > trying to get this in for a while before the other platforms started
> > using the new framework. Not everyone was free at the same time and
> > it's taken longer that I would have wished for.
> 
> > I did my best to limit the changes that would be needed without
> > making my patch useless. Appreciate your understanding.
> 
> To be honest it doesn't look like your patch is a particular issue here
> - there's wider process problems, for example we've managed to go
> through most of the release cycle and so far the only changes showing up
> in -next are:

I think that "wider process problems" is probably a euphemism, and I'll
take responsibility for that.  This has been a learning process for me
and I underestimated the percentage of my time that would be consumed by
common clk maintenance.  I'm trying to rectify that problem now.

> 
> Viresh Kumar (6):
>       SPEAr: clk: Add VCO-PLL Synthesizer clock
>       SPEAr: clk: Add Auxiliary Synthesizer clock
>       SPEAr: clk: Add Fractional Synthesizer clock
>       SPEAr: clk: Add General Purpose Timer Synthesizer clock
>       SPEAr: Switch to common clock framework
>       SPEAr13xx: Add common clock framework support
> 
> Mark Brown (1):
>       ARM: 7376/1: clkdev: Implement managed clk_get()
> 
> Sascha Hauer (1):
>       clk: add a fixed factor clock
> 
> viresh kumar (1):
>       ARM: 7392/1: CLKDEV: Optimize clk_find()
> 
> and obviously there's quite a bit more work which has been going on.

I could use some suggestions on the best way to resolve the merge issues
we have currently.  It appears that we have three bases that platforms
need to port over the common clk framework:

Russell's clkdev
Arnd's arm-soc
My clk-next branch

I was happy to push my changes to Linus directly (as discussed in
previous mails) but I'm starting to think that maybe having Arnd absorb
the clk-next branch as part of arm-soc would be the fastest way to
assist platforms that are porting over.

Do the platform folks agree?  Is this suggestion sane?

Thanks,
Mike

Re: [PATCH] clk: Use a separate struct for holding init data.

[Mike Turquette]

On 20120425-22:58, Saravana Kannan wrote:
> Create a struct clk_init_data to hold all data that needs to be passed from
> the platfrom specific driver to the common clock framework during clock
> registration. Add a pointer to this struct inside clk_hw.
> 
> This has several advantages:
> * Completely hides struct clk from many clock platform drivers and static
>   clock initialization code that don't care for static initialization of
>   the struct clks.
> * For platforms that want to do complete static initialization, it removed
>   the need to directly mess with the struct clk's fields while still
>   allowing to statically allocate struct clk. This keeps the code more
>   future proof even if they include clk-private.h.
> * Simplifies the generic clk_register() function and allows adding optional
>   fields in the future without modifying the function signature.
> * Simplifies the static initialization of clocks on all platforms by
>   removing the need for forward delcarations or convoluted macros.
> 
> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>

Hi Saravana,

Thanks for the patch.  I've taken it into my clk-next but I have two
points:

1) I'm surprised that you abandoned the approach of exposing the
less-private members of struct clk via struct clk_hw.  Your original
patch did just that, but did not account for static initialization.
This patch seems to have gone in the opposite direction and only
accounts for static initialization.

I'm happy to take the patch as-is, but I did think that there were
merits to your original approach.

2) I did make a modification to your patch where I kept the
DEFINE_CLK_* macros and continued to declare __clk_init in
clk-private.h.  I do want to get rid of both of these in the future but
currently my platform relies on static initialization before the
allocator is available.  Please let me know if this causes a problem for
you.

Platform folks should rebase on top of this if needed.  This should be
the last change to the driver/platform-facing API before 3.5.

Sascha,

Can you resubmit your fixed-factor clock?  I think the registration
function collides with these changes.

Thanks,
Mike

Re: [PATCH] clk: Use a separate struct for holding init data.

[Shawn Guo]

On Tue, May 01, 2012 at 06:56:50PM -0700, Mike Turquette wrote:
> I could use some suggestions on the best way to resolve the merge issues
> we have currently.  It appears that we have three bases that platforms
> need to port over the common clk framework:
> 
> Russell's clkdev
> Arnd's arm-soc
> My clk-next branch
> 
> I was happy to push my changes to Linus directly (as discussed in
> previous mails) but I'm starting to think that maybe having Arnd absorb
> the clk-next branch as part of arm-soc would be the fastest way to
> assist platforms that are porting over.
> 
> Do the platform folks agree?  Is this suggestion sane?
> 
As one of the people who are working on platform porting, I'm not
concerned about the path that clk core goes to Linus, but the time
when we have a stable clk core branch appears on arm-soc either as
a dependency or a downstream tree.  Once we have stable branches for
both rmk's clkdev and clk core appear on arm-soc, we can start asking
Arnd to pull platform porting.

-- 
Regards,
Shawn

Re: [PATCH] clk: Use a separate struct for holding init data.

[Saravana Kannan]

On 05/01/2012 07:04 PM, Mike Turquette wrote:
> On 20120425-22:58, Saravana Kannan wrote:
>> Create a struct clk_init_data to hold all data that needs to be passed from
>> the platfrom specific driver to the common clock framework during clock
>> registration. Add a pointer to this struct inside clk_hw.
>>
>> This has several advantages:
>> * Completely hides struct clk from many clock platform drivers and static
>>    clock initialization code that don't care for static initialization of
>>    the struct clks.
>> * For platforms that want to do complete static initialization, it removed
>>    the need to directly mess with the struct clk's fields while still
>>    allowing to statically allocate struct clk. This keeps the code more
>>    future proof even if they include clk-private.h.
>> * Simplifies the generic clk_register() function and allows adding optional
>>    fields in the future without modifying the function signature.
>> * Simplifies the static initialization of clocks on all platforms by
>>    removing the need for forward delcarations or convoluted macros.
>>
>> Signed-off-by: Saravana Kannan<skannan@codeaurora.org>
>
> Hi Saravana,
>
> Thanks for the patch.  I've taken it into my clk-next but I have two
> points:

Yayyy!! Finally I can get rid of having to know about struct clk.

> 1) I'm surprised that you abandoned the approach of exposing the
> less-private members of struct clk via struct clk_hw.  Your original
> patch did just that, but did not account for static initialization.
> This patch seems to have gone in the opposite direction and only
> accounts for static initialization.

I think there might be some misunderstanding on what can/can't be done 
with my patch. Or may be I'm not understanding your question.

I used to expose the "shared" info through clk_hw. I just put them in a 
struct and make clk_hw point to it. This would allow for easily marking 
this shared info as __init data. It would have a been a pain to do (or 
not even possible) if I had put the fields directly into clk_hw.

I'm not sure why you say my patch only accounts for static 
initialization. The entire clk specific struct (say, struct fixed_clk), 
the clk_init_data can be dynamically allocated and registered using 
clk_register.

For completely static init, you can just do:

#include <linux/clk-private.h>

static struct clk __my_clk;

static struct clk_init_data __my_clki = {
	<fill in shared fields>
};

static struct fixed_clk my_clk = {
	.blah = 10,
	.hw = {
		.i = &__my_clki;
		.c = &__my_clk;
	},
};

__clk_register(&my_clk.hw);

>
> I'm happy to take the patch as-is, but I did think that there were
> merits to your original approach.

Is there anything the first patch could do that this one couldn't?

The only small demerit of this patch that I know is that we could be 
doing some copying of the shared data when we do clk_register() (this 
prevents us from having one copy of parent list, etc).

>
> 2) I did make a modification to your patch where I kept the
> DEFINE_CLK_* macros and continued to declare __clk_init in
> clk-private.h.  I do want to get rid of both of these in the future but
> currently my platform relies on static initialization before the
> allocator is available.  Please let me know if this causes a problem for
> you.

I definitely had your requirements in mind too when I made the changes.

You really shouldn't need __clk_init. That's why I added __clk_register. 
With __clk_register (and the example I gave above), you should be able 
to do fully static init. Is there something I missed?

The DEFINE_CLK_* marcos aren't really very useful since there is no 
cyclic referencing going on.

You also don't really need to define variables for struct clk or struct 
clk_init_data. You can create anonymous struct pointers if that's your 
style. Something like:


static struct fixed_clk my_clk = {
	.blah = 10,
	.hw = {
		.i = &(struct clk_init_data) {
			<fill in shared fields>
		},
		.c = &(struct clk){};
	},
};

So, with one of the above approaches, DEFINE_CLK_* macros just end up 
obfuscating the definition of a clock and its fields.

With __clk_register() the only real difference between fully static and 
partly dynamic clock registration is that you don't mark the 
clk_init_data struct as __init and you call __clk_register() instead of 
clk_register(). I believe I documented it next to __clk_register() in clk.c.

> Platform folks should rebase on top of this if needed.  This should be
> the last change to the driver/platform-facing API before 3.5.

I really wish we discussed your changes before it was made, pulled in 
and announced since clk_init isn't really needed. But since you just 
added more APIs and didn't remove the ones I added, I guess it's not 
very bad.

Since people were already frustrated with the API change I made at this 
point, can we recommend people to not use __clk_init() when sending 
patches for your clk-next? And make it static after the next kernel release?

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] clk: Use a separate struct for holding init data.

[Andrew Lunn]

> I could use some suggestions on the best way to resolve the merge issues
> we have currently.  It appears that we have three bases that platforms
> need to port over the common clk framework:
> 
> Russell's clkdev
> Arnd's arm-soc
> My clk-next branch

Hi Mike

The Orion code only depends on clk-next. I've been more conservative
with the changes, knowing that once they are merged i can add more
patches to make use of devm_get_clk() etc.

So for my, as well as going in via arm-soc, i could also imaging
giving you a pull request and becoming part of clk-next. However, i
don't care what route they take.

      Andrew

Re: [PATCH] clk: Use a separate struct for holding init data.

[Sascha Hauer]

On Wed, Apr 25, 2012 at 10:58:56PM -0700, Saravana Kannan wrote:
> Create a struct clk_init_data to hold all data that needs to be passed from
> the platfrom specific driver to the common clock framework during clock
> registration. Add a pointer to this struct inside clk_hw.
> 
> This has several advantages:
> * Completely hides struct clk from many clock platform drivers and static
>   clock initialization code that don't care for static initialization of
>   the struct clks.
> * For platforms that want to do complete static initialization, it removed
>   the need to directly mess with the struct clk's fields while still
>   allowing to statically allocate struct clk. This keeps the code more
>   future proof even if they include clk-private.h.
> * Simplifies the generic clk_register() function and allows adding optional
>   fields in the future without modifying the function signature.
> * Simplifies the static initialization of clocks on all platforms by
>   removing the need for forward delcarations or convoluted macros.
> 
> 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    |   14 +++--
>  drivers/clk/clk-fixed-rate.c |   14 +++--
>  drivers/clk/clk-gate.c       |   15 +++--
>  drivers/clk/clk-mux.c        |   10 +++-
>  drivers/clk/clk.c            |   91 +++++++++++++++++++------------
>  include/linux/clk-private.h  |  121 +-----------------------------------------
>  include/linux/clk-provider.h |   59 +++++++++++++-------
>  7 files changed, 129 insertions(+), 195 deletions(-)
> 
> diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c
> index 6e58f11..8e97491 100644
> --- a/drivers/clk/clk-mux.c
> +++ b/drivers/clk/clk-mux.c
> @@ -95,6 +95,7 @@ struct clk *clk_register_mux(struct device *dev, const char *name,
>  {
>  	struct clk_mux *mux;
>  	struct clk *clk;
> +	struct clk_init_data init;
>  
>  	/* allocate the mux */
>  	mux = kzalloc(sizeof(struct clk_mux), GFP_KERNEL);
> @@ -103,6 +104,12 @@ struct clk *clk_register_mux(struct device *dev, const char *name,
>  		return ERR_PTR(-ENOMEM);
>  	}
>  
> +	init.name = name;
> +	init.ops = &clk_mux_ops;
> +	init.flags = flags;
> +	init.parent_names = parent_names;
> +	init.num_parents = num_parents;
> +
>  	/* struct clk_mux assignments */
>  	mux->reg = reg;
>  	mux->shift = shift;
> @@ -110,8 +117,7 @@ struct clk *clk_register_mux(struct device *dev, const char *name,
>  	mux->flags = clk_mux_flags;
>  	mux->lock = lock;

There is a mux->hw.init = &init missing here.

Sascha

>  
> -	clk = clk_register(dev, name, &clk_mux_ops, &mux->hw,
> -			parent_names, num_parents, flags);
> +	clk = clk_register(dev, &mux->hw);
>  
>  	if (IS_ERR(clk))
>  		kfree(mux);

-- 
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] clk: Use a separate struct for holding init data.

[Russell King - ARM Linux]

On Wed, May 02, 2012 at 11:58:16AM +0200, Sascha Hauer wrote:
> > diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c
> > index 6e58f11..8e97491 100644
> > --- a/drivers/clk/clk-mux.c
> > +++ b/drivers/clk/clk-mux.c
> > @@ -95,6 +95,7 @@ struct clk *clk_register_mux(struct device *dev, const char *name,
> >  {
> >  	struct clk_mux *mux;
> >  	struct clk *clk;
> > +	struct clk_init_data init;
> >  
> >  	/* allocate the mux */
> >  	mux = kzalloc(sizeof(struct clk_mux), GFP_KERNEL);
> > @@ -103,6 +104,12 @@ struct clk *clk_register_mux(struct device *dev, const char *name,
> >  		return ERR_PTR(-ENOMEM);
> >  	}
> >  
> > +	init.name = name;
> > +	init.ops = &clk_mux_ops;
> > +	init.flags = flags;
> > +	init.parent_names = parent_names;
> > +	init.num_parents = num_parents;
> > +
> >  	/* struct clk_mux assignments */
> >  	mux->reg = reg;
> >  	mux->shift = shift;
> > @@ -110,8 +117,7 @@ struct clk *clk_register_mux(struct device *dev, const char *name,
> >  	mux->flags = clk_mux_flags;
> >  	mux->lock = lock;
> 
> There is a mux->hw.init = &init missing here.

What happens to mux->hw.init long term?  Because once this function
returns, that pointer will no longer be valid.  It would be a good
idea to NULL it out in clk_register() once its done with, to ensure
that no one comes up with the idea of using it later.

Re: [PATCH] clk: Use a separate struct for holding init data.

[Sascha Hauer]

On Wed, May 02, 2012 at 11:02:25AM +0100, Russell King - ARM Linux wrote:
> > 
> > There is a mux->hw.init = &init missing here.
> 
> What happens to mux->hw.init long term?  Because once this function
> returns, that pointer will no longer be valid.

It's not used after clk_register, so everything should be fine.

> It would be a good
> idea to NULL it out in clk_register() once its done with, to ensure
> that no one comes up with the idea of using it later.

Enforcing this is a good idea.

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] clk: Use a separate struct for holding init data.

[Arnd Bergmann]

On Wednesday 02 May 2012, Mike Turquette wrote:
> I was happy to push my changes to Linus directly (as discussed in
> previous mails) but I'm starting to think that maybe having Arnd absorb
> the clk-next branch as part of arm-soc would be the fastest way to
> assist platforms that are porting over.
> 
> Do the platform folks agree?  Is this suggestion sane?

I guess that makes sense while there are still nontrivial
interdependencies between work going the clk-next branch and into
arm-soc.

	Arnd

Re: [PATCH] clk: Use a separate struct for holding init data.

[Mark Brown]

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

On Tue, May 01, 2012 at 06:56:50PM -0700, Mike Turquette wrote:
> On 20120501-19:19, Mark Brown wrote:

> > To be honest it doesn't look like your patch is a particular issue here
> > - there's wider process problems, for example we've managed to go
> > through most of the release cycle and so far the only changes showing up
> > in -next are:

> I think that "wider process problems" is probably a euphemism, and I'll
> take responsibility for that.  This has been a learning process for me
> and I underestimated the percentage of my time that would be consumed by
> common clk maintenance.  I'm trying to rectify that problem now.

It's not really a euphamism - it really does seem like we've got all the
technical stuff proceeding reasonably well but we're just struggling
with the mechanics of actually getting the code into -next and on its
way to mainline.

> I was happy to push my changes to Linus directly (as discussed in
> previous mails) but I'm starting to think that maybe having Arnd absorb
> the clk-next branch as part of arm-soc would be the fastest way to
> assist platforms that are porting over.

> Do the platform folks agree?  Is this suggestion sane?

Seems to make sense to me; if there's some bits that are less clear you
could always keep them on a branch separate to the one that the
platforms use.  It's probably also worth getting things into -next
directly, that way integration testing of bleeding edge stuff can happen
before it's been merged into other trees and it's hard to change.

What I've tried do with regmap when it's been possible (it's not this
time around because the stride changes touch everything) is to have
topic branches so that people can pull in only the specific bits they
need.  I still end upn sending a pull request to Linus even if chunks of
it have also gone via other trees.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] clk: Use a separate struct for holding init data.

[Mike Turquette]

On 20120501-21:42, Saravana Kannan wrote:
> On 05/01/2012 07:04 PM, Mike Turquette wrote:
> >1) I'm surprised that you abandoned the approach of exposing the
> >less-private members of struct clk via struct clk_hw.  Your original
> >patch did just that, but did not account for static initialization.
> >This patch seems to have gone in the opposite direction and only
> >accounts for static initialization.
> 
> I think there might be some misunderstanding on what can/can't be
> done with my patch. Or may be I'm not understanding your question.
> 

I believe I am reading the code correctly.  I'll try reiterating below.

> I used to expose the "shared" info through clk_hw. I just put them
> in a struct and make clk_hw point to it. This would allow for easily
> marking this shared info as __init data.

So platforms must choose between marking clk->hw->init as __initdata, OR
they can keep it around and reference it later from clock code that
includes clk-provder.h.  Is this a fair statement?

> It would have a been a pain
> to do (or not even possible) if I had put the fields directly into
> clk_hw.
> 

Not true.  Combining Sascha's original static initializer idea with your
struct clk_hw patch would be easy.  A separate struct for static init
would be marked as __initdata.  When passed into a registration function
that data would be copied to fields in struct clk_hw.  You've done this
exact same thing, with the exception of copying the data to struct clk
instead.

It is not a big deal, and I'm fine with the direction this patch has
taken, but I wanted to point it out.  If you look at Russell's and
Sascha's replies in this thread you'll see that they regard
clk->hw->init as purely __initdata, going so far as to mark it NULL in
clk_register.  This sort of change at the framework level would
eliminate the ability for your clock code to reference these members
directly.

> I'm not sure why you say my patch only accounts for static
> initialization. The entire clk specific struct (say, struct
> fixed_clk), the clk_init_data can be dynamically allocated and
> registered using clk_register.
> 

This was a miscommunication on my part and can be disregarded.  Of
course your patch allows for dynamic registration.

My point was that of the two previous approaches on this list (Sascha's
static initializers and your own struct clk_hw modifications), this
patch only represents the functionality of the former.  I should have
been more clear.

> >2) I did make a modification to your patch where I kept the
> >DEFINE_CLK_* macros and continued to declare __clk_init in
> >clk-private.h.  I do want to get rid of both of these in the future but
> >currently my platform relies on static initialization before the
> >allocator is available.  Please let me know if this causes a problem for
> >you.
> 
> I definitely had your requirements in mind too when I made the changes.
> 
> You really shouldn't need __clk_init. That's why I added
> __clk_register.

I completely missed __clk_register.  I'm not sure I see the point of it
however.  struct clk is still exposed to folks that are using this new
function; compared to simply calling __clk_init it has added a few loads
& stores.  Regardless these interfaces will hopefully die off completely
once OMAP's clock code uses an initcall.

> >Platform folks should rebase on top of this if needed.  This should be
> >the last change to the driver/platform-facing API before 3.5.
> 
> I really wish we discussed your changes before it was made, pulled
> in and announced since clk_init isn't really needed. But since you
> just added more APIs and didn't remove the ones I added, I guess
> it's not very bad.
> 

I probably announced it too soon, but everything you added to the API is
still there, just with some extra stuff that should go away in the
future.

> Since people were already frustrated with the API change I made at
> this point, can we recommend people to not use __clk_init() when
> sending patches for your clk-next? And make it static after the next
> kernel release?

Yes, I completely agree.  It would be good to get rid of __clk_register
and __clk_init down the road.  Marking the interfaces as deprecated is
one solution; however I agree with your suggestion and just catching it
in review is probably the best route.

Thanks,
Mike

Re: [PATCH] clk: Use a separate struct for holding init data.

[Mike Turquette]

On 20120502-07:16, Andrew Lunn wrote:
> > I could use some suggestions on the best way to resolve the merge issues
> > we have currently.  It appears that we have three bases that platforms
> > need to port over the common clk framework:
> > 
> > Russell's clkdev
> > Arnd's arm-soc
> > My clk-next branch
> 
> Hi Mike
> 
> The Orion code only depends on clk-next. I've been more conservative
> with the changes, knowing that once they are merged i can add more
> patches to make use of devm_get_clk() etc.
> 
> So for my, as well as going in via arm-soc, i could also imaging
> giving you a pull request and becoming part of clk-next. However, i
> don't care what route they take.
> 

Hi Andrew,

The choice is yours.

Regards,
Mike

>       Andrew

Re: [PATCH] clk: Use a separate struct for holding init data.

[Domenico Andreoli]

On Wed, Apr 25, 2012 at 10:58:56PM -0700, Saravana Kannan wrote:
>
> diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
> index 90627e4..8ea11b4 100644
> --- a/drivers/clk/clk-divider.c
> +++ b/drivers/clk/clk-divider.c
> @@ -167,6 +167,7 @@ struct clk *clk_register_divider(struct device *dev, const char *name,
>  {
>  	struct clk_divider *div;
>  	struct clk *clk;
> +	struct clk_init_data init;
>  
>  	/* allocate the divider */
>  	div = kzalloc(sizeof(struct clk_divider), GFP_KERNEL);
> @@ -175,19 +176,22 @@ struct clk *clk_register_divider(struct device *dev, const char *name,
>  		return ERR_PTR(-ENOMEM);
>  	}
>  
> +	init.name = name;
> +	init.ops = &clk_divider_ops;
> +	init.flags = flags;
> +	init.parent_names = (parent_name ? &parent_name: NULL);
> +	init.num_parents = (parent_name ? 1 : 0);
> +
>  	/* struct clk_divider assignments */
>  	div->reg = reg;
>  	div->shift = shift;
>  	div->width = width;
>  	div->flags = clk_divider_flags;
>  	div->lock = lock;
> +	div->hw.init = &init;
>  
>  	/* register the clock */
> -	clk = clk_register(dev, name,
> -			&clk_divider_ops, &div->hw,
> -			(parent_name ? &parent_name: NULL),
> -			(parent_name ? 1 : 0),
> -			flags);
> +	clk = clk_register(dev, &div->hw);
>  
>  	if (IS_ERR(clk))
>  		kfree(div);

I would prefer to rip the parent _settings_ configuration out of
clk_register(). It's optional right? And passing a single parent is a
common case.

Three cases:

  1) one parent:
     __clk_register_parent(clk, parent_name);
     clk_register(dev, name, &ops, flags);

  2) many parents:
     __clk_register_parents(clk, parent_names, num_parents);
     clk_register(dev, name, &ops, flags);

  3) no parents:
     clk_register(dev, name, &ops, flags);

You may also want to move the whole parent initialization into
__clk_register_parents() and call it after clk_register(), it would
simplify some error paths.

This pattern could be used also with other common clocks registration
functions (fixed rate, divider, mux, etc) that may have complex
initializations and/or optional parameters that cannot go all on the
same function call.

cheers,
Domenico

Re: [PATCH] clk: Use a separate struct for holding init data.

[Saravana Kannan]

On 05/03/2012 04:03 PM, Domenico Andreoli wrote:
> On Wed, Apr 25, 2012 at 10:58:56PM -0700, Saravana Kannan wrote:
>>
>> diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
>> index 90627e4..8ea11b4 100644
>> --- a/drivers/clk/clk-divider.c
>> +++ b/drivers/clk/clk-divider.c
>> @@ -167,6 +167,7 @@ struct clk *clk_register_divider(struct device *dev, const char *name,
>>   {
>>   	struct clk_divider *div;
>>   	struct clk *clk;
>> +	struct clk_init_data init;
>>
>>   	/* allocate the divider */
>>   	div = kzalloc(sizeof(struct clk_divider), GFP_KERNEL);
>> @@ -175,19 +176,22 @@ struct clk *clk_register_divider(struct device *dev, const char *name,
>>   		return ERR_PTR(-ENOMEM);
>>   	}
>>
>> +	init.name = name;
>> +	init.ops =&clk_divider_ops;
>> +	init.flags = flags;
>> +	init.parent_names = (parent_name ?&parent_name: NULL);
>> +	init.num_parents = (parent_name ? 1 : 0);
>> +
>>   	/* struct clk_divider assignments */
>>   	div->reg = reg;
>>   	div->shift = shift;
>>   	div->width = width;
>>   	div->flags = clk_divider_flags;
>>   	div->lock = lock;
>> +	div->hw.init =&init;
>>
>>   	/* register the clock */
>> -	clk = clk_register(dev, name,
>> -			&clk_divider_ops,&div->hw,
>> -			(parent_name ?&parent_name: NULL),
>> -			(parent_name ? 1 : 0),
>> -			flags);
>> +	clk = clk_register(dev,&div->hw);
>>
>>   	if (IS_ERR(clk))
>>   		kfree(div);
>
> I would prefer to rip the parent _settings_ configuration out of
> clk_register(). It's optional right? And passing a single parent is a
> common case.
>
> Three cases:
>
>    1) one parent:
>       __clk_register_parent(clk, parent_name);
>       clk_register(dev, name,&ops, flags);
>
>    2) many parents:
>       __clk_register_parents(clk, parent_names, num_parents);
>       clk_register(dev, name,&ops, flags);
>
>    3) no parents:
>       clk_register(dev, name,&ops, flags);
>
> You may also want to move the whole parent initialization into
> __clk_register_parents() and call it after clk_register(), it would
> simplify some error paths.
>
> This pattern could be used also with other common clocks registration
> functions (fixed rate, divider, mux, etc) that may have complex
> initializations and/or optional parameters that cannot go all on the
> same function call.

Please no. If anything, make those other register functions go in the 
direction of clk_register(). Have a long list of params to a function 
and then having it fill up a structure just makes the code less 
readable. Why would that be any better than having the whole structure 
statically declared or the whole structure dynamically populated (by 
device tree) and then calling clk_register()?

Take about 50 clocks with 3 parents each and try to register them in the 
way you suggested and in a way how clk_register() in this patch will 
need you to declare them statically. Compare the two and see which would 
be more readable.

-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] clk: Use a separate struct for holding init data.

[Domenico Andreoli]

On Thu, May 03, 2012 at 06:11:56PM -0700, Saravana Kannan wrote:
> On 05/03/2012 04:03 PM, Domenico Andreoli wrote:
> >On Wed, Apr 25, 2012 at 10:58:56PM -0700, Saravana Kannan wrote:
> >>
> >>diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
> >>index 90627e4..8ea11b4 100644
> >>--- a/drivers/clk/clk-divider.c
> >>+++ b/drivers/clk/clk-divider.c
> >>@@ -167,6 +167,7 @@ struct clk *clk_register_divider(struct device *dev, const char *name,
> >>  {
> >>  	struct clk_divider *div;
> >>  	struct clk *clk;
> >>+	struct clk_init_data init;
> >>
> >>  	/* allocate the divider */
> >>  	div = kzalloc(sizeof(struct clk_divider), GFP_KERNEL);
> >>@@ -175,19 +176,22 @@ struct clk *clk_register_divider(struct device *dev, const char *name,
> >>  		return ERR_PTR(-ENOMEM);
> >>  	}
> >>
> >>+	init.name = name;
> >>+	init.ops =&clk_divider_ops;
> >>+	init.flags = flags;
> >>+	init.parent_names = (parent_name ?&parent_name: NULL);
> >>+	init.num_parents = (parent_name ? 1 : 0);
> >>+
> >>  	/* struct clk_divider assignments */
> >>  	div->reg = reg;
> >>  	div->shift = shift;
> >>  	div->width = width;
> >>  	div->flags = clk_divider_flags;
> >>  	div->lock = lock;
> >>+	div->hw.init =&init;
> >>
> >>  	/* register the clock */
> >>-	clk = clk_register(dev, name,
> >>-			&clk_divider_ops,&div->hw,
> >>-			(parent_name ?&parent_name: NULL),
> >>-			(parent_name ? 1 : 0),
> >>-			flags);
> >>+	clk = clk_register(dev,&div->hw);
> >>
> >>  	if (IS_ERR(clk))
> >>  		kfree(div);
> >
> >I would prefer to rip the parent _settings_ configuration out of
> >clk_register(). It's optional right? And passing a single parent is a
> >common case.
> >
> >Three cases:
> >
> >   1) one parent:
> >      __clk_register_parent(clk, parent_name);
> >      clk_register(dev, name,&ops, flags);
> >
> >   2) many parents:
> >      __clk_register_parents(clk, parent_names, num_parents);
> >      clk_register(dev, name,&ops, flags);
> >
> >   3) no parents:
> >      clk_register(dev, name,&ops, flags);
> >
> >You may also want to move the whole parent initialization into
> >__clk_register_parents() and call it after clk_register(), it would
> >simplify some error paths.
> >
> >This pattern could be used also with other common clocks registration
> >functions (fixed rate, divider, mux, etc) that may have complex
> >initializations and/or optional parameters that cannot go all on the
> >same function call.
> 
> Please no. If anything, make those other register functions go in
> the direction of clk_register(). Have a long list of params to a
> function and then having it fill up a structure just makes the code
> less readable. Why would that be any better than having the whole
> structure statically declared or the whole structure dynamically
> populated (by device tree) and then calling clk_register()?
> 
> Take about 50 clocks with 3 parents each and try to register them in
> the way you suggested and in a way how clk_register() in this patch
> will need you to declare them statically. Compare the two and see
> which would be more readable.

I was not thinking at the static initialization at all (but I was
forgetting that clk does not yet exist before the invocation of
clk_register).

For a few hours I was convinced that moving the parent initialization
stuff in a separate function would have allowed also to ditch the (IMHO)
horrible whole name caching (whose purpose is... allowing to register
clock with a parent not yet known to the clock subsystem?)

Unfortunately the whole idea is quite invasive and the benefits
debatable, it's simply too late to speak.

Thanks anyway.

Domenico