Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: Linux: Kernel

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

 

 

First page Previous page 1 2 Next page Last page  View All Linux kernel RSS feed   Index | Next | Previous | View Threaded


skannan at codeaurora

Apr 25, 2012, 10:58 PM

Post #1 of 32 (249 views)
Permalink
[PATCH] clk: Use a separate struct for holding init data.

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 [at] codeaurora>
Cc: Mike Turquette <mturquette [at] linaro>
Cc: Andrew Lunn <andrew [at] lunn>
Cc: Rob Herring <rob.herring [at] calxeda>
Cc: Russell King <linux [at] arm>
Cc: Jeremy Kerr <jeremy.kerr [at] canonical>
Cc: Thomas Gleixner <tglx [at] linutronix>
Cc: Arnd Bergman <arnd.bergmann [at] linaro>
Cc: Paul Walmsley <paul [at] pwsan>
Cc: Shawn Guo <shawn.guo [at] freescale>
Cc: Sascha Hauer <s.hauer [at] pengutronix>
Cc: Jamie Iles <jamie [at] jamieiles>
Cc: Richard Zhao <richard.zhao [at] linaro>
Cc: Saravana Kannan <skannan [at] codeaurora>
Cc: Magnus Damm <magnus.damm [at] gmail>
Cc: Mark Brown <broonie [at] opensource>
Cc: Linus Walleij <linus.walleij [at] stericsson>
Cc: Stephen Boyd <sboyd [at] codeaurora>
Cc: Amit Kucheria <amit.kucheria [at] linaro>
Cc: Deepak Saxena <dsaxena [at] linaro>
Cc: Grant Likely <grant.likely [at] secretlab>
---
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


skannan at codeaurora

Apr 25, 2012, 11:28 PM

Post #2 of 32 (253 views)
Permalink
Re: [PATCH] clk: Use a separate struct for holding init data. [In reply to]

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


s.hauer at pengutronix

Apr 26, 2012, 1:39 AM

Post #3 of 32 (249 views)
Permalink
Re: [PATCH] clk: Use a separate struct for holding init data. [In reply to]

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 |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


s.hauer at pengutronix

Apr 26, 2012, 1:42 AM

Post #4 of 32 (249 views)
Permalink
Re: [PATCH] clk: Use a separate struct for holding init data. [In reply to]

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 |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


skannan at codeaurora

Apr 26, 2012, 2:15 AM

Post #5 of 32 (251 views)
Permalink
Re: [PATCH] clk: Use a separate struct for holding init data. [In reply to]

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.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


skannan at codeaurora

Apr 26, 2012, 2:36 AM

Post #6 of 32 (249 views)
Permalink
Re: [PATCH] clk: Use a separate struct for holding init data. [In reply to]

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


broonie at opensource

Apr 26, 2012, 2:49 AM

Post #7 of 32 (249 views)
Permalink
Re: [PATCH] clk: Use a separate struct for holding init data. [In reply to]

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.
Attachments: signature.asc (0.82 KB)


s.hauer at pengutronix

Apr 26, 2012, 2:51 AM

Post #8 of 32 (248 views)
Permalink
Re: [PATCH] clk: Use a separate struct for holding init data. [In reply to]

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 |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


skannan at codeaurora

Apr 30, 2012, 12:30 PM

Post #9 of 32 (236 views)
Permalink
Re: [PATCH] clk: Use a separate struct for holding init data. [In reply to]

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


mturquette at ti

Apr 30, 2012, 3:19 PM

Post #10 of 32 (234 views)
Permalink
Re: [PATCH] clk: Use a separate struct for holding init data. [In reply to]

On Mon, Apr 30, 2012 at 12:30 PM, Saravana Kannan
<skannan [at] codeaurora> 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 [at] lists
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


skannan at codeaurora

Apr 30, 2012, 3:46 PM

Post #11 of 32 (240 views)
Permalink
Re: [PATCH] clk: Use a separate struct for holding init data. [In reply to]

On 04/30/2012 03:19 PM, Turquette, Mike wrote:
> On Mon, Apr 30, 2012 at 12:30 PM, Saravana Kannan
> <skannan [at] codeaurora> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


shawn.guo at linaro

May 1, 2012, 1:11 AM

Post #12 of 32 (236 views)
Permalink
Re: [PATCH] clk: Use a separate struct for holding init data. [In reply to]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


andrew at lunn

May 1, 2012, 2:13 AM

Post #13 of 32 (237 views)
Permalink
Re: [PATCH] clk: Use a separate struct for holding init data. [In reply to]

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


broonie at opensource

May 1, 2012, 10:00 AM

Post #14 of 32 (230 views)
Permalink
Re: [PATCH] clk: Use a separate struct for holding init data. [In reply to]

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.
Attachments: signature.asc (0.82 KB)


skannan at codeaurora

May 1, 2012, 11:03 AM

Post #15 of 32 (230 views)
Permalink
Re: [PATCH] clk: Use a separate struct for holding init data. [In reply to]

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


andrew at lunn

May 1, 2012, 11:04 AM

Post #16 of 32 (232 views)
Permalink
Re: [PATCH] clk: Use a separate struct for holding init data. [In reply to]

> 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


broonie at opensource

May 1, 2012, 11:19 AM

Post #17 of 32 (231 views)
Permalink
Re: [PATCH] clk: Use a separate struct for holding init data. [In reply to]

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.
Attachments: signature.asc (0.82 KB)


mturquette at ti

May 1, 2012, 6:56 PM

Post #18 of 32 (226 views)
Permalink
Re: [PATCH] clk: Use a separate struct for holding init data. [In reply to]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


mturquette at ti

May 1, 2012, 7:04 PM

Post #19 of 32 (226 views)
Permalink
Re: [PATCH] clk: Use a separate struct for holding init data. [In reply to]

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 [at] codeaurora>

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


shawn.guo at linaro

May 1, 2012, 7:14 PM

Post #20 of 32 (226 views)
Permalink
Re: [PATCH] clk: Use a separate struct for holding init data. [In reply to]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


skannan at codeaurora

May 1, 2012, 9:42 PM

Post #21 of 32 (226 views)
Permalink
Re: [PATCH] clk: Use a separate struct for holding init data. [In reply to]

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 [at] codeaurora>
>
> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


andrew at lunn

May 1, 2012, 10:16 PM

Post #22 of 32 (227 views)
Permalink
Re: [PATCH] clk: Use a separate struct for holding init data. [In reply to]

> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


s.hauer at pengutronix

May 2, 2012, 2:58 AM

Post #23 of 32 (224 views)
Permalink
Re: [PATCH] clk: Use a separate struct for holding init data. [In reply to]

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 [at] codeaurora>
> Cc: Mike Turquette <mturquette [at] linaro>
> Cc: Andrew Lunn <andrew [at] lunn>
> Cc: Rob Herring <rob.herring [at] calxeda>
> Cc: Russell King <linux [at] arm>
> Cc: Jeremy Kerr <jeremy.kerr [at] canonical>
> Cc: Thomas Gleixner <tglx [at] linutronix>
> Cc: Arnd Bergman <arnd.bergmann [at] linaro>
> Cc: Paul Walmsley <paul [at] pwsan>
> Cc: Shawn Guo <shawn.guo [at] freescale>
> Cc: Sascha Hauer <s.hauer [at] pengutronix>
> Cc: Jamie Iles <jamie [at] jamieiles>
> Cc: Richard Zhao <richard.zhao [at] linaro>
> Cc: Saravana Kannan <skannan [at] codeaurora>
> Cc: Magnus Damm <magnus.damm [at] gmail>
> Cc: Mark Brown <broonie [at] opensource>
> Cc: Linus Walleij <linus.walleij [at] stericsson>
> Cc: Stephen Boyd <sboyd [at] codeaurora>
> Cc: Amit Kucheria <amit.kucheria [at] linaro>
> Cc: Deepak Saxena <dsaxena [at] linaro>
> Cc: Grant Likely <grant.likely [at] secretlab>
> ---
> 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 |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


linux at arm

May 2, 2012, 3:02 AM

Post #24 of 32 (229 views)
Permalink
Re: [PATCH] clk: Use a separate struct for holding init data. [In reply to]

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


s.hauer at pengutronix

May 2, 2012, 3:11 AM

Post #25 of 32 (226 views)
Permalink
Re: [PATCH] clk: Use a separate struct for holding init data. [In reply to]

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 |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

First page Previous page 1 2 Next page Last page  View All Linux kernel RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.