clk: cleanup on basic clks

PATCH New Shawn Guo 4 participants 24 messages Mar 27, 2012 – Apr 9, 2012 Common clk

lore

Share & tools

lore

Shawn Guo

author

2012-03-27 07:23 UTC #0 lore raw

cover clk: cleanup on basic clks

The series makes some cleanup on basic clks. Two patches from Mark are folded here as they are doing the same thing.

Regards,
Shawn

---
Mark Brown (2):
clk: fixed-rate: Don't open code kstrdup()
clk: Constify parent name arrays

Shawn Guo (8):
clk: use kzalloc in clk_register_mux
clk: remove unnecessary EXPORT_SYMBOL_GPL
clk: add "const" for clk_ops of basic clks
clk: declare clk_ops of basic clks in clk-provider.h
clk: align error return of basic clks registeration
clk: fixed-rate: registration error handling fixup
clk: dynamically allocate parent_names basic clk registration
clk: copy parent_names in clk-mux registration

 drivers/clk/clk-divider.c    |   19 +++++++++++--------
 drivers/clk/clk-fixed-rate.c |   33 +++++++++++++++++----------------
 drivers/clk/clk-gate.c       |   20 ++++++++++++--------
 drivers/clk/clk-mux.c        |   39 +++++++++++++++++++++++++++++++--------
 drivers/clk/clk.c            |    2 +-
 include/linux/clk-private.h  |   10 +---------
 include/linux/clk-provider.h |   10 ++++++----
 7 files changed, 79 insertions(+), 54 deletions(-)

Shawn Guo

author

2012-03-27 07:23 UTC #1 lore raw

PATCH 1/10 clk: use kzalloc in clk_register_mux

Change clk_register_mux to use kzalloc, just like what all other basic clk registration functions do.

Signed-off-by Shawn Guo <[email protected]>

---
 drivers/clk/clk-mux.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/clk/clk-mux.c +1 -1

index c71ad1f..50e0595 100644
@@ -97,7 +97,7 @@ struct clk *clk_register_mux(struct device *dev, const char *name,
 {
 	struct clk_mux *mux;
 
-	mux = kmalloc(sizeof(struct clk_mux), GFP_KERNEL);
+	mux = kzalloc(sizeof(struct clk_mux), GFP_KERNEL);
 
 	if (!mux) {
 		pr_err("%s: could not allocate mux clk\n", __func__);

Signature · 1 line

1.7.5.4

Turquette, Mike

2012-03-27 23:31 UTC #2 lore raw

PATCH 1/10 clk: use kzalloc in clk_register_mux

Quoted 4 lines · Shawn Guo

Change clk_register_mux to use kzalloc, just like what all other basic clk registration functions do.
Signed-off-by Shawn Guo <[email protected]>

Thanks for the patch. Will take into my fixes branch.

Regards,
Mike

Quoted 25 lines

---
?drivers/clk/clk-mux.c | ? ?2 +-
?1 files changed, 1 insertions(+), 1 deletions(-)
drivers/clk/clk-mux.c +1 -2
index c71ad1f..50e0595 100644
@@ -97,7 +97,7 @@ struct clk *clk_register_mux(struct device *dev, const char *name,
 ?{
 ? ? ? ?struct clk_mux *mux;
 
- ? ? ? mux = kmalloc(sizeof(struct clk_mux), GFP_KERNEL);
+ ? ? ? mux = kzalloc(sizeof(struct clk_mux), GFP_KERNEL);
 
 ? ? ? ?if (!mux) {
 ? ? ? ? ? ? ? ?pr_err("%s: could not allocate mux clk\n", __func__);
--
1.7.5.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Shawn Guo

author

2012-03-27 07:23 UTC #3 lore raw

PATCH 2/10 clk: remove unnecessary EXPORT_SYMBOL_GPL

In reply to Shawn Guo

It makes no sense to have EXPORT_SYMBOL_GPL on static functions.

Signed-off-by Shawn Guo <[email protected]>

---
 drivers/clk/clk-divider.c    |    3 ---
 drivers/clk/clk-fixed-rate.c |    1 -
 drivers/clk/clk-gate.c       |    3 ---
 drivers/clk/clk-mux.c        |    2 --
 4 files changed, 0 insertions(+), 9 deletions(-)

drivers/clk/clk-divider.c +0 -3

index d5ac6a7..231cd6e 100644
@@ -45,7 +45,6 @@ static unsigned long clk_divider_recalc_rate(struct clk_hw *hw,
 
 	return parent_rate / div;
 }
-EXPORT_SYMBOL_GPL(clk_divider_recalc_rate);
 
 /*
  * The reverse of DIV_ROUND_UP: The maximum number which

@@ -117,7 +116,6 @@ static long clk_divider_round_rate(struct clk_hw *hw, unsigned long rate,
 		return r / div;
 	}
 }
-EXPORT_SYMBOL_GPL(clk_divider_round_rate);
 
 static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate)
 {

@@ -147,7 +145,6 @@ static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate)
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(clk_divider_set_rate);
 
 struct clk_ops clk_divider_ops = {
 	.recalc_rate = clk_divider_recalc_rate,

drivers/clk/clk-fixed-rate.c +0 -1

index 90c79fb..651b06f 100644
@@ -32,7 +32,6 @@ static unsigned long clk_fixed_rate_recalc_rate(struct clk_hw *hw,
 {
 	return to_clk_fixed_rate(hw)->fixed_rate;
 }
-EXPORT_SYMBOL_GPL(clk_fixed_rate_recalc_rate);
 
 struct clk_ops clk_fixed_rate_ops = {
 	.recalc_rate = clk_fixed_rate_recalc_rate,

drivers/clk/clk-gate.c +0 -3

index b5902e2..b688f47 100644
@@ -71,7 +71,6 @@ static int clk_gate_enable(struct clk_hw *hw)
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(clk_gate_enable);
 
 static void clk_gate_disable(struct clk_hw *hw)
 {

@@ -82,7 +81,6 @@ static void clk_gate_disable(struct clk_hw *hw)
 	else
 		clk_gate_clear_bit(gate);
 }
-EXPORT_SYMBOL_GPL(clk_gate_disable);
 
 static int clk_gate_is_enabled(struct clk_hw *hw)
 {

@@ -99,7 +97,6 @@ static int clk_gate_is_enabled(struct clk_hw *hw)
 
 	return reg ? 1 : 0;
 }
-EXPORT_SYMBOL_GPL(clk_gate_is_enabled);
 
 struct clk_ops clk_gate_ops = {
 	.enable = clk_gate_enable,

drivers/clk/clk-mux.c +0 -2

index 50e0595..45cad61 100644
@@ -55,7 +55,6 @@ static u8 clk_mux_get_parent(struct clk_hw *hw)
 
 	return val;
 }
-EXPORT_SYMBOL_GPL(clk_mux_get_parent);
 
 static int clk_mux_set_parent(struct clk_hw *hw, u8 index)
 {

@@ -82,7 +81,6 @@ static int clk_mux_set_parent(struct clk_hw *hw, u8 index)
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(clk_mux_set_parent);
 
 struct clk_ops clk_mux_ops = {
 	.get_parent = clk_mux_get_parent,

Signature · 1 line

1.7.5.4

Shawn Guo

author

2012-03-27 07:23 UTC #4 lore raw

PATCH 3/10 clk: add "const" for clk_ops of basic clks

In reply to Shawn Guo

The clk_ops of basic clks should have "const" to match the definition in "struct clk" and clk_register prototype.

Signed-off-by Shawn Guo <[email protected]>

---
 drivers/clk/clk-divider.c    |    2 +-
 drivers/clk/clk-fixed-rate.c |    2 +-
 drivers/clk/clk-gate.c       |    2 +-
 drivers/clk/clk-mux.c        |    2 +-
 include/linux/clk-private.h  |    8 ++++----
 5 files changed, 8 insertions(+), 8 deletions(-)

drivers/clk/clk-divider.c +1 -1

index 231cd6e..b1c4b02 100644
@@ -146,7 +146,7 @@ static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate)
 	return 0;
 }
 
-struct clk_ops clk_divider_ops = {
+const struct clk_ops clk_divider_ops = {
 	.recalc_rate = clk_divider_recalc_rate,
 	.round_rate = clk_divider_round_rate,
 	.set_rate = clk_divider_set_rate,

drivers/clk/clk-fixed-rate.c +1 -1

index 651b06f..027e477 100644
@@ -33,7 +33,7 @@ static unsigned long clk_fixed_rate_recalc_rate(struct clk_hw *hw,
 	return to_clk_fixed_rate(hw)->fixed_rate;
 }
 
-struct clk_ops clk_fixed_rate_ops = {
+const struct clk_ops clk_fixed_rate_ops = {
 	.recalc_rate = clk_fixed_rate_recalc_rate,
 };
 EXPORT_SYMBOL_GPL(clk_fixed_rate_ops);

drivers/clk/clk-gate.c +1 -1

index b688f47..fe2ff9e 100644
@@ -98,7 +98,7 @@ static int clk_gate_is_enabled(struct clk_hw *hw)
 	return reg ? 1 : 0;
 }
 
-struct clk_ops clk_gate_ops = {
+const struct clk_ops clk_gate_ops = {
 	.enable = clk_gate_enable,
 	.disable = clk_gate_disable,
 	.is_enabled = clk_gate_is_enabled,

drivers/clk/clk-mux.c +1 -1

index 45cad61..5424488 100644
@@ -82,7 +82,7 @@ static int clk_mux_set_parent(struct clk_hw *hw, u8 index)
 	return 0;
 }
 
-struct clk_ops clk_mux_ops = {
+const struct clk_ops clk_mux_ops = {
 	.get_parent = clk_mux_get_parent,
 	.set_parent = clk_mux_set_parent,
 };

include/linux/clk-private.h +4 -4

index 5e4312b..5f4ccd7 100644
@@ -55,7 +55,7 @@ struct clk {
  * alternative macro for static initialization
  */
 
-extern struct clk_ops clk_fixed_rate_ops;
+extern const struct clk_ops clk_fixed_rate_ops;
 
 #define DEFINE_CLK_FIXED_RATE(_name, _flags, _rate,		\
 				_fixed_rate_flags)		\

@@ -78,7 +78,7 @@ extern struct clk_ops clk_fixed_rate_ops;
 		.flags = _flags,				\
 	};
 
-extern struct clk_ops clk_gate_ops;
+extern const struct clk_ops clk_gate_ops;
 
 #define DEFINE_CLK_GATE(_name, _parent_name, _parent_ptr,	\
 				_flags, _reg, _bit_idx,		\

@@ -110,7 +110,7 @@ extern struct clk_ops clk_gate_ops;
 		.flags = _flags,				\
 	};
 
-extern struct clk_ops clk_divider_ops;
+extern const struct clk_ops clk_divider_ops;
 
 #define DEFINE_CLK_DIVIDER(_name, _parent_name, _parent_ptr,	\
 				_flags, _reg, _shift, _width,	\

@@ -143,7 +143,7 @@ extern struct clk_ops clk_divider_ops;
 		.flags = _flags,				\
 	};
 
-extern struct clk_ops clk_mux_ops;
+extern const struct clk_ops clk_mux_ops;
 
 #define DEFINE_CLK_MUX(_name, _parent_names, _parents, _flags,	\
 				_reg, _shift, _width,		\

Signature · 1 line

1.7.5.4

Turquette, Mike

2012-03-27 23:29 UTC #5 lore raw

PATCH 3/10 clk: add "const" for clk_ops of basic clks

Quoted 4 lines · Shawn Guo

The clk_ops of basic clks should have "const" to match the definition in "struct clk" and clk_register prototype.
Signed-off-by Shawn Guo <[email protected]>

Thanks for the patch. Will take into my fixes branch.

Regards,
Mike

Quoted 108 lines

---

?drivers/clk/clk-divider.c ? ?| ? ?2 +-
?drivers/clk/clk-fixed-rate.c | ? ?2 +-
?drivers/clk/clk-gate.c ? ? ? | ? ?2 +-
?drivers/clk/clk-mux.c ? ? ? ?| ? ?2 +-
?include/linux/clk-private.h ?| ? ?8 ++++----

?5 files changed, 8 insertions(+), 8 deletions(-)

drivers/clk/clk-divider.c +1 -1

index 231cd6e..b1c4b02 100644
@@ -146,7 +146,7 @@ static int clk_divider_set_rate(struct clk_hw *hw, unsigned long rate)
 ? ? ? ?return 0;
 ?}
 
-struct clk_ops clk_divider_ops = {
+const struct clk_ops clk_divider_ops = {
 ? ? ? ?.recalc_rate = clk_divider_recalc_rate,
 ? ? ? ?.round_rate = clk_divider_round_rate,
 ? ? ? ?.set_rate = clk_divider_set_rate,

drivers/clk/clk-fixed-rate.c +1 -1

index 651b06f..027e477 100644
@@ -33,7 +33,7 @@ static unsigned long clk_fixed_rate_recalc_rate(struct clk_hw *hw,
 ? ? ? ?return to_clk_fixed_rate(hw)->fixed_rate;
 ?}
 
-struct clk_ops clk_fixed_rate_ops = {
+const struct clk_ops clk_fixed_rate_ops = {
 ? ? ? ?.recalc_rate = clk_fixed_rate_recalc_rate,
 ?};
 ?EXPORT_SYMBOL_GPL(clk_fixed_rate_ops);

drivers/clk/clk-gate.c +1 -1

index b688f47..fe2ff9e 100644
@@ -98,7 +98,7 @@ static int clk_gate_is_enabled(struct clk_hw *hw)
 ? ? ? ?return reg ? 1 : 0;
 ?}
 
-struct clk_ops clk_gate_ops = {
+const struct clk_ops clk_gate_ops = {
 ? ? ? ?.enable = clk_gate_enable,
 ? ? ? ?.disable = clk_gate_disable,
 ? ? ? ?.is_enabled = clk_gate_is_enabled,

drivers/clk/clk-mux.c +1 -1

index 45cad61..5424488 100644
@@ -82,7 +82,7 @@ static int clk_mux_set_parent(struct clk_hw *hw, u8 index)
 ? ? ? ?return 0;
 ?}
 
-struct clk_ops clk_mux_ops = {
+const struct clk_ops clk_mux_ops = {
 ? ? ? ?.get_parent = clk_mux_get_parent,
 ? ? ? ?.set_parent = clk_mux_set_parent,
 ?};

include/linux/clk-private.h +0 -0

index 5e4312b..5f4ccd7 100644
@@ -55,7 +55,7 @@ struct clk {

?* alternative macro for static initialization
?*/

include/linux/clk-private.h +1 -1

-extern struct clk_ops clk_fixed_rate_ops;
+extern const struct clk_ops clk_fixed_rate_ops;

?#define DEFINE_CLK_FIXED_RATE(_name, _flags, _rate, ? ? ? ? ? ?\
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?_fixed_rate_flags) ? ? ? ? ? ? ?\

include/linux/clk-private.h +3 -4

@@ -78,7 +78,7 @@ extern struct clk_ops clk_fixed_rate_ops;
 ? ? ? ? ? ? ? ?.flags = _flags, ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
 ? ? ? ?};
 
-extern struct clk_ops clk_gate_ops;
+extern const struct clk_ops clk_gate_ops;
 
 ?#define DEFINE_CLK_GATE(_name, _parent_name, _parent_ptr, ? ? ?\
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?_flags, _reg, _bit_idx, ? ? ? ? \

@@ -110,7 +110,7 @@ extern struct clk_ops clk_gate_ops;
 ? ? ? ? ? ? ? ?.flags = _flags, ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
 ? ? ? ?};
 
-extern struct clk_ops clk_divider_ops;
+extern const struct clk_ops clk_divider_ops;
 
 ?#define DEFINE_CLK_DIVIDER(_name, _parent_name, _parent_ptr, ? \
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?_flags, _reg, _shift, _width, ? \

@@ -143,7 +143,7 @@ extern struct clk_ops clk_divider_ops;
 ? ? ? ? ? ? ? ?.flags = _flags, ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
 ? ? ? ?};
 
-extern struct clk_ops clk_mux_ops;
+extern const struct clk_ops clk_mux_ops;
 
 ?#define DEFINE_CLK_MUX(_name, _parent_names, _parents, _flags, \
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?_reg, _shift, _width, ? ? ? ? ? \
--

1.7.5.4

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Shawn Guo

author

2012-03-27 07:23 UTC #6 lore raw

PATCH 4/10 clk: declare clk_ops of basic clks in clk-provider.h

In reply to Shawn Guo

Besides the static initialization, the clk_ops of basic clks could also be used by particular clk type being subclass of the basic clks.

For example, clk_busy_divider has the same clk_ops as clk_divider, except it has to wait for a busy bit before return success with .set_rate. clk_busy_divider will somehow reuse clk_ops of clk_divider.

Since clk-provider.h is included by clk-private.h, it's safe to move those clk_ops declaration of basic clks form clk-private.h into clk-provider.h, so that implementation of clks like clk_busy_divider above do not need to include clk-private.h to access those clk_ops.

Signed-off-by Shawn Guo <[email protected]>

---
 include/linux/clk-private.h  |    8 --------
 include/linux/clk-provider.h |    4 ++++
 2 files changed, 4 insertions(+), 8 deletions(-)

include/linux/clk-private.h +0 -8

index 5f4ccd7..f19fee0 100644
@@ -55,8 +55,6 @@ struct clk {
  * alternative macro for static initialization
  */
 
-extern const struct clk_ops clk_fixed_rate_ops;
-
 #define DEFINE_CLK_FIXED_RATE(_name, _flags, _rate,		\
 				_fixed_rate_flags)		\
 	static struct clk _name;				\

@@ -78,8 +76,6 @@ extern const struct clk_ops clk_fixed_rate_ops;
 		.flags = _flags,				\
 	};
 
-extern const struct clk_ops clk_gate_ops;
-
 #define DEFINE_CLK_GATE(_name, _parent_name, _parent_ptr,	\
 				_flags, _reg, _bit_idx,		\
 				_gate_flags, _lock)		\

@@ -110,8 +106,6 @@ extern const struct clk_ops clk_gate_ops;
 		.flags = _flags,				\
 	};
 
-extern const struct clk_ops clk_divider_ops;
-
 #define DEFINE_CLK_DIVIDER(_name, _parent_name, _parent_ptr,	\
 				_flags, _reg, _shift, _width,	\
 				_divider_flags, _lock)		\

@@ -143,8 +137,6 @@ extern const struct clk_ops clk_divider_ops;
 		.flags = _flags,				\
 	};
 
-extern const struct clk_ops clk_mux_ops;
-
 #define DEFINE_CLK_MUX(_name, _parent_names, _parents, _flags,	\
 				_reg, _shift, _width,		\
 				_mux_flags, _lock)		\

include/linux/clk-provider.h +4 -0

index 5508897..6eb8e5d 100644
@@ -149,6 +149,7 @@ struct clk_fixed_rate {
 	u8		flags;
 };
 
+extern const struct clk_ops clk_fixed_rate_ops;
 struct clk *clk_register_fixed_rate(struct device *dev, const char *name,
 		const char *parent_name, unsigned long flags,
 		unsigned long fixed_rate);

@@ -180,6 +181,7 @@ struct clk_gate {
 
 #define CLK_GATE_SET_TO_DISABLE		BIT(0)
 
+extern const struct clk_ops clk_gate_ops;
 struct clk *clk_register_gate(struct device *dev, const char *name,
 		const char *parent_name, unsigned long flags,
 		void __iomem *reg, u8 bit_idx,

@@ -218,6 +220,7 @@ struct clk_divider {
 #define CLK_DIVIDER_ONE_BASED		BIT(0)
 #define CLK_DIVIDER_POWER_OF_TWO	BIT(1)
 
+extern const struct clk_ops clk_divider_ops;
 struct clk *clk_register_divider(struct device *dev, const char *name,
 		const char *parent_name, unsigned long flags,
 		void __iomem *reg, u8 shift, u8 width,

@@ -252,6 +255,7 @@ struct clk_mux {
 #define CLK_MUX_INDEX_ONE		BIT(0)
 #define CLK_MUX_INDEX_BIT		BIT(1)
 
+extern const struct clk_ops clk_mux_ops;
 struct clk *clk_register_mux(struct device *dev, const char *name,
 		char **parent_names, u8 num_parents, unsigned long flags,
 		void __iomem *reg, u8 shift, u8 width,

Signature · 1 line

1.7.5.4

Turquette, Mike

2012-03-28 21:20 UTC #7 lore raw

PATCH 4/10 clk: declare clk_ops of basic clks in clk-provider.h

Quoted 7 lines · Shawn Guo

Besides the static initialization, the clk_ops of basic clks could also be used by particular clk type being subclass of the basic clks.
For example, clk_busy_divider has the same clk_ops as clk_divider, except it has to wait for a busy bit before return success with .set_rate. ?clk_busy_divider will somehow reuse clk_ops of clk_divider.

Quoted 6 lines

Since clk-provider.h is included by clk-private.h, it's safe to move those clk_ops declaration of basic clks form ?clk-private.h into clk-provider.h, so that implementation of clks like clk_busy_divider above do not need to include clk-private.h to access those clk_ops.
Signed-off-by Shawn Guo <[email protected]>

Hi Shawn,

Thanks for the patch. After Andrew's response on using clk_ops over the EXPORT'd functions I'll take this in my fixes branch.

Mike

Quoted 89 lines

---
?include/linux/clk-private.h ?| ? ?8 --------
?include/linux/clk-provider.h | ? ?4 ++++
?2 files changed, 4 insertions(+), 8 deletions(-)

include/linux/clk-private.h +0 -0

index 5f4ccd7..f19fee0 100644
@@ -55,8 +55,6 @@ struct clk {

?* alternative macro for static initialization
?*/

include/linux/clk-private.h +0 -2

-extern const struct clk_ops clk_fixed_rate_ops;
-

?#define DEFINE_CLK_FIXED_RATE(_name, _flags, _rate, ? ? ? ? ? ?\
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?_fixed_rate_flags) ? ? ? ? ? ? ?\
? ? ? ?static struct clk _name; ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\

include/linux/clk-private.h +0 -6

@@ -78,8 +76,6 @@ extern const struct clk_ops clk_fixed_rate_ops;
 ? ? ? ? ? ? ? ?.flags = _flags, ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
 ? ? ? ?};
 
-extern const struct clk_ops clk_gate_ops;
-
 ?#define DEFINE_CLK_GATE(_name, _parent_name, _parent_ptr, ? ? ?\
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?_flags, _reg, _bit_idx, ? ? ? ? \
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?_gate_flags, _lock) ? ? ? ? ? ? \

@@ -110,8 +106,6 @@ extern const struct clk_ops clk_gate_ops;
 ? ? ? ? ? ? ? ?.flags = _flags, ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
 ? ? ? ?};
 
-extern const struct clk_ops clk_divider_ops;
-
 ?#define DEFINE_CLK_DIVIDER(_name, _parent_name, _parent_ptr, ? \
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?_flags, _reg, _shift, _width, ? \
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?_divider_flags, _lock) ? ? ? ? ?\

@@ -143,8 +137,6 @@ extern const struct clk_ops clk_divider_ops;
 ? ? ? ? ? ? ? ?.flags = _flags, ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
 ? ? ? ?};
 
-extern const struct clk_ops clk_mux_ops;
-
 ?#define DEFINE_CLK_MUX(_name, _parent_names, _parents, _flags, \
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?_reg, _shift, _width, ? ? ? ? ? \
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?_mux_flags, _lock) ? ? ? ? ? ? ?\

include/linux/clk-provider.h +4 -1

index 5508897..6eb8e5d 100644
@@ -149,6 +149,7 @@ struct clk_fixed_rate {
 ? ? ? ?u8 ? ? ? ? ? ? ?flags;
 ?};
 
+extern const struct clk_ops clk_fixed_rate_ops;
 ?struct clk *clk_register_fixed_rate(struct device *dev, const char *name,
 ? ? ? ? ? ? ? ?const char *parent_name, unsigned long flags,
 ? ? ? ? ? ? ? ?unsigned long fixed_rate);

@@ -180,6 +181,7 @@ struct clk_gate {
 
 ?#define CLK_GATE_SET_TO_DISABLE ? ? ? ? ? ? ? ?BIT(0)
 
+extern const struct clk_ops clk_gate_ops;
 ?struct clk *clk_register_gate(struct device *dev, const char *name,
 ? ? ? ? ? ? ? ?const char *parent_name, unsigned long flags,
 ? ? ? ? ? ? ? ?void __iomem *reg, u8 bit_idx,

@@ -218,6 +220,7 @@ struct clk_divider {
 ?#define CLK_DIVIDER_ONE_BASED ? ? ? ? ?BIT(0)
 ?#define CLK_DIVIDER_POWER_OF_TWO ? ? ? BIT(1)
 
+extern const struct clk_ops clk_divider_ops;
 ?struct clk *clk_register_divider(struct device *dev, const char *name,
 ? ? ? ? ? ? ? ?const char *parent_name, unsigned long flags,
 ? ? ? ? ? ? ? ?void __iomem *reg, u8 shift, u8 width,

@@ -252,6 +255,7 @@ struct clk_mux {
 ?#define CLK_MUX_INDEX_ONE ? ? ? ? ? ? ?BIT(0)
 ?#define CLK_MUX_INDEX_BIT ? ? ? ? ? ? ?BIT(1)
 
+extern const struct clk_ops clk_mux_ops;
 ?struct clk *clk_register_mux(struct device *dev, const char *name,
 ? ? ? ? ? ? ? ?char **parent_names, u8 num_parents, unsigned long flags,
 ? ? ? ? ? ? ? ?void __iomem *reg, u8 shift, u8 width,
--

1.7.5.4

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Shawn Guo

author

2012-03-27 07:23 UTC #8 lore raw

PATCH 5/10 clk: align error return of basic clks registeration

In reply to Shawn Guo

clk_register_gate and clk_register_divider return NULL for error, which is aligned with clk_register function. But clk_register_mux and clk_register_fixed_rate return ERR_PTR. Change them to return NULL for better aligning with other registeration functions.

Signed-off-by Shawn Guo <[email protected]>

---
 drivers/clk/clk-fixed-rate.c |    2 +-
 drivers/clk/clk-mux.c        |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

drivers/clk/clk-fixed-rate.c +1 -1

index 027e477..d268439 100644
@@ -50,7 +50,7 @@ struct clk *clk_register_fixed_rate(struct device *dev, const char *name,
 
 	if (!fixed) {
 		pr_err("%s: could not allocate fixed clk\n", __func__);
-		return ERR_PTR(-ENOMEM);
+		return NULL;
 	}
 
 	/* struct clk_fixed_rate assignments */

drivers/clk/clk-mux.c +1 -1

index 5424488..19f3116 100644
@@ -99,7 +99,7 @@ struct clk *clk_register_mux(struct device *dev, const char *name,
 
 	if (!mux) {
 		pr_err("%s: could not allocate mux clk\n", __func__);
-		return ERR_PTR(-ENOMEM);
+		return NULL;
 	}
 
 	/* struct clk_mux assignments */

Signature · 1 line

1.7.5.4

Turquette, Mike

2012-03-27 23:28 UTC #9 lore raw

PATCH 5/10 clk: align error return of basic clks registeration

Quoted 6 lines · Shawn Guo

clk_register_gate and clk_register_divider return NULL for error, which is aligned with clk_register function. ?But clk_register_mux and clk_register_fixed_rate return ERR_PTR. ?Change them to return NULL for better aligning with other registeration functions.
Signed-off-by Shawn Guo <[email protected]>

Thanks for the patch Shawn, but this is actually the opposite direction that I want to go. In light of the other thread with Saravana and Russell on a similar topic, I'd like to make sure that the driver-facing functions clearly return -EERROR during failure. I've lined up a patch for the same on my side.

Regards,
Mike

Quoted 39 lines

---
?drivers/clk/clk-fixed-rate.c | ? ?2 +-
?drivers/clk/clk-mux.c ? ? ? ?| ? ?2 +-
?2 files changed, 2 insertions(+), 2 deletions(-)
drivers/clk/clk-fixed-rate.c +1 -1
index 027e477..d268439 100644
@@ -50,7 +50,7 @@ struct clk *clk_register_fixed_rate(struct device *dev, const char *name,
 
 ? ? ? ?if (!fixed) {
 ? ? ? ? ? ? ? ?pr_err("%s: could not allocate fixed clk\n", __func__);
- ? ? ? ? ? ? ? return ERR_PTR(-ENOMEM);
+ ? ? ? ? ? ? ? return NULL;
 ? ? ? ?}
 
? ? ? ?/* struct clk_fixed_rate assignments */
drivers/clk/clk-mux.c +1 -1
index 5424488..19f3116 100644
@@ -99,7 +99,7 @@ struct clk *clk_register_mux(struct device *dev, const char *name,
 
 ? ? ? ?if (!mux) {
 ? ? ? ? ? ? ? ?pr_err("%s: could not allocate mux clk\n", __func__);
- ? ? ? ? ? ? ? return ERR_PTR(-ENOMEM);
+ ? ? ? ? ? ? ? return NULL;
 ? ? ? ?}
 
? ? ? ?/* struct clk_mux assignments */
--
1.7.5.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Shawn Guo

author

2012-03-27 07:23 UTC #10 lore raw

PATCH 6/10 clk: fixed-rate: Don't open code kstrdup()

In reply to Shawn Guo

From: Mark Brown <[email protected]>

Save a little code by using the library function.

Signed-off-by Mark Brown <[email protected]>

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

drivers/clk/clk-fixed-rate.c +1 -6

index d268439..1f87a03 100644
@@ -44,7 +44,6 @@ struct clk *clk_register_fixed_rate(struct device *dev, const char *name,
 {
 	struct clk_fixed_rate *fixed;
 	char **parent_names = NULL;
-	u8 len;
 
 	fixed = kzalloc(sizeof(struct clk_fixed_rate), GFP_KERNEL);

@@ -62,14 +61,10 @@ struct clk *clk_register_fixed_rate(struct device *dev, const char *name,
 		if (! parent_names)
 			goto out;
 
-		len = sizeof(char) * strlen(parent_name);
-
-		parent_names[0] = kmalloc(len, GFP_KERNEL);
+		parent_names[0] = kstrdup(parent_name, GFP_KERNEL);
 
 		if (!parent_names[0])
 			goto out;
-
-		strncpy(parent_names[0], parent_name, len);
 	}
 
 out:

Signature · 1 line

1.7.5.4

Turquette, Mike

2012-03-27 23:36 UTC #11 lore raw

PATCH 6/10 clk: fixed-rate: Don't open code kstrdup()

Quoted 5 lines · Shawn Guo

From: Mark Brown <[email protected]>
Save a little code by using the library function.
Signed-off-by Mark Brown <[email protected]>

Shawn, thanks for reposting.

Mark, thanks for the patch. I wrote a near-identical patch in my branch, but it returns -EERROR instead of just returning NULL on failure so I'll stick with my version.

Thanks,
Mike

Quoted 40 lines

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

drivers/clk/clk-fixed-rate.c +1 -6

index d268439..1f87a03 100644
@@ -44,7 +44,6 @@ struct clk *clk_register_fixed_rate(struct device *dev, const char *name,
 ?{
 ? ? ? ?struct clk_fixed_rate *fixed;
 ? ? ? ?char **parent_names = NULL;
- ? ? ? u8 len;
 
 ? ? ? ?fixed = kzalloc(sizeof(struct clk_fixed_rate), GFP_KERNEL);

@@ -62,14 +61,10 @@ struct clk *clk_register_fixed_rate(struct device *dev, const char *name,
 ? ? ? ? ? ? ? ?if (! parent_names)
 ? ? ? ? ? ? ? ? ? ? ? ?goto out;
 
- ? ? ? ? ? ? ? len = sizeof(char) * strlen(parent_name);
-
- ? ? ? ? ? ? ? parent_names[0] = kmalloc(len, GFP_KERNEL);
+ ? ? ? ? ? ? ? parent_names[0] = kstrdup(parent_name, GFP_KERNEL);
 
 ? ? ? ? ? ? ? ?if (!parent_names[0])
 ? ? ? ? ? ? ? ? ? ? ? ?goto out;
-
- ? ? ? ? ? ? ? strncpy(parent_names[0], parent_name, len);
 ? ? ? ?}

?out:
--
1.7.5.4

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Shawn Guo

author

2012-03-27 07:23 UTC #12 lore raw

PATCH 7/10 clk: Constify parent name arrays

In reply to Shawn Guo

From: Mark Brown <[email protected]>

Drivers should be able to declare their arrays of parent names as const so the APIs need to accept const arguments.

Signed-off-by Mark Brown <[email protected]>

---
 drivers/clk/clk-fixed-rate.c |    2 +-
 drivers/clk/clk-mux.c        |    2 +-
 drivers/clk/clk.c            |    2 +-
 include/linux/clk-private.h  |    2 +-
 include/linux/clk-provider.h |    6 +++---
 5 files changed, 7 insertions(+), 7 deletions(-)

drivers/clk/clk-fixed-rate.c +1 -1

index 1f87a03..295f0d5 100644
@@ -43,7 +43,7 @@ struct clk *clk_register_fixed_rate(struct device *dev, const char *name,
 		unsigned long fixed_rate)
 {
 	struct clk_fixed_rate *fixed;
-	char **parent_names = NULL;
+	const char **parent_names = NULL;
 
 	fixed = kzalloc(sizeof(struct clk_fixed_rate), GFP_KERNEL);

drivers/clk/clk-mux.c +1 -1

index 19f3116..1ff90a6 100644
@@ -89,7 +89,7 @@ const struct clk_ops clk_mux_ops = {
 EXPORT_SYMBOL_GPL(clk_mux_ops);
 
 struct clk *clk_register_mux(struct device *dev, const char *name,
-		char **parent_names, u8 num_parents, unsigned long flags,
+		const char **parent_names, u8 num_parents, unsigned long flags,
 		void __iomem *reg, u8 shift, u8 width,
 		u8 clk_mux_flags, spinlock_t *lock)
 {

drivers/clk/clk.c +1 -1

index 9cf6f59..33ef0df 100644
@@ -1322,7 +1322,7 @@ out:
  */
 struct clk *clk_register(struct device *dev, const char *name,
 		const struct clk_ops *ops, struct clk_hw *hw,
-		char **parent_names, u8 num_parents, unsigned long flags)
+		const char **parent_names, u8 num_parents, unsigned long flags)
 {
 	struct clk *clk;

include/linux/clk-private.h +1 -1

index f19fee0..e9c8b98 100644
@@ -30,7 +30,7 @@ struct clk {
 	const struct clk_ops	*ops;
 	struct clk_hw		*hw;
 	struct clk		*parent;
-	char			**parent_names;
+	const char		**parent_names;
 	struct clk		**parents;
 	u8			num_parents;
 	unsigned long		rate;

include/linux/clk-provider.h +3 -3

index 6eb8e5d..24c5013 100644
@@ -214,7 +214,7 @@ struct clk_divider {
 	u8		width;
 	u8		flags;
 	spinlock_t	*lock;
-	char		*parent[1];
+	const char	*parent[1];
 };
 
 #define CLK_DIVIDER_ONE_BASED		BIT(0)

@@ -257,7 +257,7 @@ struct clk_mux {
 
 extern const struct clk_ops clk_mux_ops;
 struct clk *clk_register_mux(struct device *dev, const char *name,
-		char **parent_names, u8 num_parents, unsigned long flags,
+		const char **parent_names, u8 num_parents, unsigned long flags,
 		void __iomem *reg, u8 shift, u8 width,
 		u8 clk_mux_flags, spinlock_t *lock);

@@ -278,7 +278,7 @@ struct clk *clk_register_mux(struct device *dev, const char *name,
  */
 struct clk *clk_register(struct device *dev, const char *name,
 		const struct clk_ops *ops, struct clk_hw *hw,
-		char **parent_names, u8 num_parents, unsigned long flags);
+		const char **parent_names, u8 num_parents, unsigned long flags);
 
 /* helper functions */
 const char *__clk_get_name(struct clk *clk);

Signature · 1 line

1.7.5.4

Turquette, Mike

2012-03-28 01:03 UTC #13 lore raw

PATCH 7/10 clk: Constify parent name arrays

Quoted 6 lines · Shawn Guo

From: Mark Brown <[email protected]>
Drivers should be able to declare their arrays of parent names as const so the APIs need to accept const arguments.
Signed-off-by Mark Brown <[email protected]>

Thanks for reposting Shawn.

Mark, I've taken this in to my fixes branch, but I dropped the change in drivers/clk/clk-fixed-rate.c (since that code has shuffled) and I also marked the char *parent[1] in fixed-rate and gate clocks as const, in addition to the divider you did in your patch.

Thanks,
Mike

Mark Brown

2012-03-28 10:07 UTC #14 lore raw

PATCH 7/10 clk: Constify parent name arrays

Quoted 4 lines · Mike

Mark, I've taken this in to my fixes branch, but I dropped the change in drivers/clk/clk-fixed-rate.c (since that code has shuffled) and I also marked the char *parent[1] in fixed-rate and gate clocks as const, in addition to the divider you did in your patch.

That one's more fun as you should really be declaring them as something
like a const array of pointers to const char which is very cumbersome to
actually do and looks ugly as hell.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120328/ae45a8ac/attachment.sig>

Shawn Guo

author

2012-03-27 07:23 UTC #15 lore raw

PATCH 8/10 clk: fixed-rate: registration error handling fixup

In reply to Shawn Guo

The clk_register_fixed_rate should not call clk_register on "out:" path.

Signed-off-by Shawn Guo <[email protected]>

---
 drivers/clk/clk-fixed-rate.c |   19 +++++++++++++------
 1 files changed, 13 insertions(+), 6 deletions(-)

drivers/clk/clk-fixed-rate.c +13 -6

index 295f0d5..0d80827 100644
@@ -43,6 +43,7 @@ struct clk *clk_register_fixed_rate(struct device *dev, const char *name,
 		unsigned long fixed_rate)
 {
 	struct clk_fixed_rate *fixed;
+	struct clk *clk;
 	const char **parent_names = NULL;
 
 	fixed = kzalloc(sizeof(struct clk_fixed_rate), GFP_KERNEL);

@@ -56,21 +57,27 @@ struct clk *clk_register_fixed_rate(struct device *dev, const char *name,
 	fixed->fixed_rate = fixed_rate;
 
 	if (parent_name) {
-		parent_names = kmalloc(sizeof(char *), GFP_KERNEL);
-
-		if (! parent_names)
+		parent_names = kzalloc(sizeof(char *), GFP_KERNEL);
+		if (!parent_names)
 			goto out;
 
 		parent_names[0] = kstrdup(parent_name, GFP_KERNEL);
-
 		if (!parent_names[0])
 			goto out;
 	}
 
-out:
-	return clk_register(dev, name,
+	clk = clk_register(dev, name,
 			&clk_fixed_rate_ops, &fixed->hw,
 			parent_names,
 			(parent_name ? 1 : 0),
 			flags);
+	if (clk)
+		return clk;
+
+out:
+	kfree(parent_names[0]);
+	kfree(parent_names);
+	kfree(fixed);
+
+	return NULL;
 }

Signature · 1 line

1.7.5.4

Turquette, Mike

2012-03-28 00:31 UTC #16 lore raw

PATCH 8/10 clk: fixed-rate: registration error handling fixup

Quoted 4 lines · Shawn Guo

The clk_register_fixed_rate should not call clk_register on "out:" path.
Signed-off-by Shawn Guo <[email protected]>

Hi Shawn,

I already had a patch for this sitting in my fixes branch and it properly returns -EERROR for failures, so I'll stick with that one.

Thanks,
Mike

Quoted 58 lines

---
?drivers/clk/clk-fixed-rate.c | ? 19 +++++++++++++------ ?1 files changed, 13 insertions(+), 6 deletions(-)

drivers/clk/clk-fixed-rate.c +13 -7

index 295f0d5..0d80827 100644
@@ -43,6 +43,7 @@ struct clk *clk_register_fixed_rate(struct device *dev, const char *name,
 ? ? ? ? ? ? ? ?unsigned long fixed_rate)
 ?{
 ? ? ? ?struct clk_fixed_rate *fixed;
+ ? ? ? struct clk *clk;
 ? ? ? ?const char **parent_names = NULL;
 
 ? ? ? ?fixed = kzalloc(sizeof(struct clk_fixed_rate), GFP_KERNEL);

@@ -56,21 +57,27 @@ struct clk *clk_register_fixed_rate(struct device *dev, const char *name,
 ? ? ? ?fixed->fixed_rate = fixed_rate;
 
 ? ? ? ?if (parent_name) {
- ? ? ? ? ? ? ? parent_names = kmalloc(sizeof(char *), GFP_KERNEL);
-
- ? ? ? ? ? ? ? if (! parent_names)
+ ? ? ? ? ? ? ? parent_names = kzalloc(sizeof(char *), GFP_KERNEL);
+ ? ? ? ? ? ? ? if (!parent_names)
 ? ? ? ? ? ? ? ? ? ? ? ?goto out;
 
 ? ? ? ? ? ? ? ?parent_names[0] = kstrdup(parent_name, GFP_KERNEL);
-
 ? ? ? ? ? ? ? ?if (!parent_names[0])
 ? ? ? ? ? ? ? ? ? ? ? ?goto out;
 ? ? ? ?}
 
-out:
- ? ? ? return clk_register(dev, name,
+ ? ? ? clk = clk_register(dev, name,
 ? ? ? ? ? ? ? ? ? ? ? ?&clk_fixed_rate_ops, &fixed->hw,
 ? ? ? ? ? ? ? ? ? ? ? ?parent_names,
 ? ? ? ? ? ? ? ? ? ? ? ?(parent_name ? 1 : 0),
 ? ? ? ? ? ? ? ? ? ? ? ?flags);
+ ? ? ? if (clk)
+ ? ? ? ? ? ? ? return clk;
+
+out:
+ ? ? ? kfree(parent_names[0]);
+ ? ? ? kfree(parent_names);
+ ? ? ? kfree(fixed);
+
+ ? ? ? return NULL;
 ?}
--

1.7.5.4

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Shawn Guo

author

2012-03-27 07:23 UTC #17 lore raw

PATCH 9/10 clk: dynamically allocate parent_names basic clk registration

In reply to Shawn Guo

It's not necessary to have "char *parent[1]" carried in struct clk_gate and clk_divider. Instead, it can be dynamically allocated in registration function, just like what clk_register_fixed_rate does.

Signed-off-by Shawn Guo <[email protected]>

---
 drivers/clk/clk-divider.c    |   14 ++++++++++----
 drivers/clk/clk-gate.c       |   15 +++++++++++----
 include/linux/clk-provider.h |    2 --
 3 files changed, 21 insertions(+), 10 deletions(-)

drivers/clk/clk-divider.c +10 -4

index b1c4b02..df03a61 100644
@@ -160,6 +160,7 @@ struct clk *clk_register_divider(struct device *dev, const char *name,
 {
 	struct clk_divider *div;
 	struct clk *clk;
+	const char **parent_names = NULL;
 
 	div = kzalloc(sizeof(struct clk_divider), GFP_KERNEL);

@@ -176,21 +177,26 @@ struct clk *clk_register_divider(struct device *dev, const char *name,
 	div->lock = lock;
 
 	if (parent_name) {
-		div->parent[0] = kstrdup(parent_name, GFP_KERNEL);
-		if (!div->parent[0])
+		parent_names = kzalloc(sizeof(char *), GFP_KERNEL);
+		if (!parent_names)
+			goto out;
+
+		parent_names[0] = kstrdup(parent_name, GFP_KERNEL);
+		if (!parent_names[0])
 			goto out;
 	}
 
 	clk = clk_register(dev, name,
 			&clk_divider_ops, &div->hw,
-			div->parent,
+			parent_names,
 			(parent_name ? 1 : 0),
 			flags);
 	if (clk)
 		return clk;
 
 out:
-	kfree(div->parent[0]);
+	kfree(parent_names[0]);
+	kfree(parent_names);
 	kfree(div);
 
 	return NULL;

drivers/clk/clk-gate.c +11 -4

index fe2ff9e..51c56cb 100644
@@ -112,6 +112,7 @@ struct clk *clk_register_gate(struct device *dev, const char *name,
 {
 	struct clk_gate *gate;
 	struct clk *clk;
+	const char **parent_names = NULL;
 
 	gate = kzalloc(sizeof(struct clk_gate), GFP_KERNEL);

@@ -127,20 +128,26 @@ struct clk *clk_register_gate(struct device *dev, const char *name,
 	gate->lock = lock;
 
 	if (parent_name) {
-		gate->parent[0] = kstrdup(parent_name, GFP_KERNEL);
-		if (!gate->parent[0])
+		parent_names = kzalloc(sizeof(char *), GFP_KERNEL);
+		if (!parent_names)
+			goto out;
+
+		parent_names[0] = kstrdup(parent_name, GFP_KERNEL);
+		if (parent_names[0])
 			goto out;
 	}
 
 	clk = clk_register(dev, name,
 			&clk_gate_ops, &gate->hw,
-			gate->parent,
+			parent_names,
 			(parent_name ? 1 : 0),
 			flags);
 	if (clk)
 		return clk;
+
 out:
-	kfree(gate->parent[0]);
+	kfree(parent_names[0]);
+	kfree(parent_names);
 	kfree(gate);
 
 	return NULL;

include/linux/clk-provider.h +0 -2

index 24c5013..7948640 100644
@@ -176,7 +176,6 @@ struct clk_gate {
 	u8		bit_idx;
 	u8		flags;
 	spinlock_t	*lock;
-	char		*parent[1];
 };
 
 #define CLK_GATE_SET_TO_DISABLE		BIT(0)

@@ -214,7 +213,6 @@ struct clk_divider {
 	u8		width;
 	u8		flags;
 	spinlock_t	*lock;
-	const char	*parent[1];
 };
 
 #define CLK_DIVIDER_ONE_BASED		BIT(0)

Signature · 1 line

1.7.5.4

Sascha Hauer

2012-03-27 08:55 UTC #18 lore raw

PATCH 9/10 clk: dynamically allocate parent_names basic clk registration

Quoted 3 lines · Shawn Guo

It's not necessary to have "char *parent[1]" carried in struct clk_gate and clk_divider. Instead, it can be dynamically allocated in registration function, just like what clk_register_fixed_rate does.

I patched it the way it is to get rid of the additional malloc which seemed unnecessary. Maybe the real question is whether the core should duplicate the parent strings instead of the basic clock types.

Sascha

Quoted 123 lines

Signed-off-by Shawn Guo <[email protected]>

---
 drivers/clk/clk-divider.c    |   14 ++++++++++----
 drivers/clk/clk-gate.c       |   15 +++++++++++----
 include/linux/clk-provider.h |    2 --
 3 files changed, 21 insertions(+), 10 deletions(-)

drivers/clk/clk-divider.c +10 -4

index b1c4b02..df03a61 100644
@@ -160,6 +160,7 @@ struct clk *clk_register_divider(struct device *dev, const char *name,
 {
 	struct clk_divider *div;
 	struct clk *clk;
+	const char **parent_names = NULL;
 
 	div = kzalloc(sizeof(struct clk_divider), GFP_KERNEL);

@@ -176,21 +177,26 @@ struct clk *clk_register_divider(struct device *dev, const char *name,
 	div->lock = lock;
 
 	if (parent_name) {
-		div->parent[0] = kstrdup(parent_name, GFP_KERNEL);
-		if (!div->parent[0])
+		parent_names = kzalloc(sizeof(char *), GFP_KERNEL);
+		if (!parent_names)
+			goto out;
+
+		parent_names[0] = kstrdup(parent_name, GFP_KERNEL);
+		if (!parent_names[0])
 			goto out;
 	}
 
 	clk = clk_register(dev, name,
 			&clk_divider_ops, &div->hw,
-			div->parent,
+			parent_names,
 			(parent_name ? 1 : 0),
 			flags);
 	if (clk)
 		return clk;
 
 out:
-	kfree(div->parent[0]);
+	kfree(parent_names[0]);
+	kfree(parent_names);
 	kfree(div);
 
 	return NULL;

drivers/clk/clk-gate.c +11 -4

index fe2ff9e..51c56cb 100644
@@ -112,6 +112,7 @@ struct clk *clk_register_gate(struct device *dev, const char *name,
 {
 	struct clk_gate *gate;
 	struct clk *clk;
+	const char **parent_names = NULL;
 
 	gate = kzalloc(sizeof(struct clk_gate), GFP_KERNEL);

@@ -127,20 +128,26 @@ struct clk *clk_register_gate(struct device *dev, const char *name,
 	gate->lock = lock;
 
 	if (parent_name) {
-		gate->parent[0] = kstrdup(parent_name, GFP_KERNEL);
-		if (!gate->parent[0])
+		parent_names = kzalloc(sizeof(char *), GFP_KERNEL);
+		if (!parent_names)
+			goto out;
+
+		parent_names[0] = kstrdup(parent_name, GFP_KERNEL);
+		if (parent_names[0])
 			goto out;
 	}
 
 	clk = clk_register(dev, name,
 			&clk_gate_ops, &gate->hw,
-			gate->parent,
+			parent_names,
 			(parent_name ? 1 : 0),
 			flags);
 	if (clk)
 		return clk;
+
 out:
-	kfree(gate->parent[0]);
+	kfree(parent_names[0]);
+	kfree(parent_names);
 	kfree(gate);
 
 	return NULL;

include/linux/clk-provider.h +0 -2

index 24c5013..7948640 100644
@@ -176,7 +176,6 @@ struct clk_gate {
 	u8		bit_idx;
 	u8		flags;
 	spinlock_t	*lock;
-	char		*parent[1];
 };
 
 #define CLK_GATE_SET_TO_DISABLE		BIT(0)

@@ -214,7 +213,6 @@ struct clk_divider {
 	u8		width;
 	u8		flags;
 	spinlock_t	*lock;
-	const char	*parent[1];
 };
 
 #define CLK_DIVIDER_ONE_BASED		BIT(0)

Signature · 7 lines

1.7.5.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Signature · 4 lines

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 |

Shawn Guo

author

2012-03-27 12:51 UTC #19 lore raw

PATCH 9/10 clk: dynamically allocate parent_names basic clk registration

Quoted 9 lines · Sascha Hauer

Quoted 3 lines · Shawn Guo
It's not necessary to have "char *parent[1]" carried in struct clk_gate and clk_divider. Instead, it can be dynamically allocated in registration function, just like what clk_register_fixed_rate does.
I patched it the way it is to get rid of the additional malloc which seemed unnecessary. Maybe the real question is whether the core should duplicate the parent strings instead of the basic clock types.

If we can ensure the parent strings will only be used during clk_register call, we do not need to duplicate the parent strings.

But right now the parent strings is used not only in clk_register call but also __clk_set_parent for finding parent index in case the search on cached parent points fails. I guess Mike has a good reason for that.

Signature · 2 lines

Regards,
Shawn

Sascha Hauer

2012-03-27 19:40 UTC #20 lore raw

PATCH 9/10 clk: dynamically allocate parent_names basic clk registration

Quoted 10 lines · Shawn Guo

Quoted 9 lines · Sascha Hauer
Quoted 3 lines · Shawn Guo
It's not necessary to have "char *parent[1]" carried in struct clk_gate and clk_divider. Instead, it can be dynamically allocated in registration function, just like what clk_register_fixed_rate does.
I patched it the way it is to get rid of the additional malloc which seemed unnecessary. Maybe the real question is whether the core should duplicate the parent strings instead of the basic clock types.

Quoted 6 lines

If we can ensure the parent strings will only be used during clk_register call, we do not need to duplicate the parent strings.
But right now the parent strings is used not only in clk_register call but also __clk_set_parent for finding parent index in case the search on cached parent points fails. I guess Mike has a good reason for that.

Probably. That's why I suggest to copy the strings in the framework. Then we do not have to duplicate the code to copy the strings and also make sure that the core works on its own strings.

Sascha

Signature · 4 lines

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 |

Turquette, Mike

2012-03-27 23:17 UTC #21 lore raw

PATCH 9/10 clk: dynamically allocate parent_names basic clk registration

Quoted 11 lines · Sascha Hauer

Quoted 10 lines · Shawn Guo
Quoted 9 lines · Sascha Hauer
Quoted 3 lines · Shawn Guo
It's not necessary to have "char *parent[1]" carried in struct clk_gate and clk_divider. ?Instead, it can be dynamically allocated in registration function, just like what clk_register_fixed_rate does.
I patched it the way it is to get rid of the additional malloc which seemed unnecessary. Maybe the real question is whether the core should duplicate the parent strings instead of the basic clock types.

Quoted 6 lines

Quoted 6 lines
If we can ensure the parent strings will only be used during clk_register call, we do not need to duplicate the parent strings.
But right now the parent strings is used not only in clk_register call but also __clk_set_parent for finding parent index in case the search on cached parent points fails. ?I guess Mike has a good reason for that.

I'd like to think my reasons are good ;-)

The common clk framework pulls together clock data from many sources, and we can never assume that the tree of struct clk's is "complete". For example an on-SoC multiplexer clock might have one input from an external PMIC. It is conceivable that clock data / common clk support for that PMIC isn't yet merged. Should that prevent the on-SoC mux clock from using that source via clk_set_parent? Absolutely not. That is why the parent_names arrays are necessary. They represent "complete" data even in the absence of actual clock data. Using only the index of the array the .set_rate callback should be able to do the [register write | i2c command | whatever] in order to switch to the PMIC source. The rate will be unknown, but at least the operation is possible.

Quoted 3 lines

Probably. That's why I suggest to copy the strings in the framework. Then we do not have to duplicate the code to copy the strings and also make sure that the core works on its own strings.

Copying the strings in the fwk is sensible. I'll throw together a patch which takes in the const char *parent_name and passes that to clk_register directly, which builds up the char **parent_names itself.

Regards,
Mike

Shawn Guo

author

2012-03-27 07:23 UTC #22 lore raw

PATCH 10/10 clk: copy parent_names in clk-mux registration

In reply to Shawn Guo

Just like what all other basic clks do, copy parent_names in clk-mux registration, so that clock driver can mark the parent_names __initconst.

Signed-off-by Shawn Guo <[email protected]>

---
 drivers/clk/clk-mux.c |   29 +++++++++++++++++++++++++++--
 1 files changed, 27 insertions(+), 2 deletions(-)

drivers/clk/clk-mux.c +27 -2

index 1ff90a6..353fcb7 100644
@@ -94,6 +94,9 @@ struct clk *clk_register_mux(struct device *dev, const char *name,
 		u8 clk_mux_flags, spinlock_t *lock)
 {
 	struct clk_mux *mux;
+	struct clk *clk;
+	const char **parents = NULL;
+	int i = 0;
 
 	mux = kzalloc(sizeof(struct clk_mux), GFP_KERNEL);

@@ -109,6 +112,28 @@ struct clk *clk_register_mux(struct device *dev, const char *name,
 	mux->flags = clk_mux_flags;
 	mux->lock = lock;
 
-	return clk_register(dev, name, &clk_mux_ops, &mux->hw,
-			parent_names, num_parents, flags);
+	if (num_parents) {
+		parents = kzalloc(sizeof(char *) * num_parents, GFP_KERNEL);
+		if (!parents)
+			goto out;
+
+		for (i = 0; i < num_parents; i++) {
+			parents[i] = kstrdup(parent_names[i], GFP_KERNEL);
+			if (!parents[i])
+				goto out;
+		}
+	}
+
+	clk =  clk_register(dev, name, &clk_mux_ops, &mux->hw,
+			parents, num_parents, flags);
+	if (clk)
+		return clk;
+
+out:
+	for (i--; i >= 0; i--)
+		kfree(parents[i]);
+	kfree(parents);
+	kfree(mux);
+
+	return NULL;
 }

Signature · 1 line

1.7.5.4

Turquette, Mike

2012-04-09 22:00 UTC #23 lore raw

PATCH 10/10 clk: copy parent_names in clk-mux registration

Quoted 5 lines · Shawn Guo

Just like what all other basic clks do, copy parent_names in clk-mux registration, so that clock driver can mark the parent_names __initconst.
Signed-off-by Shawn Guo <[email protected]>

Hi Shawn,

As discussed in the other thread, I've moved the parent string copies into clk_register; that makes this patch unnecessary (and you can still mark the parent_names as __initdata).

I'll publish my fixes either today or tomorrow.

Regards,
Mike

Quoted 57 lines

---
?drivers/clk/clk-mux.c | ? 29 +++++++++++++++++++++++++++-- ?1 files changed, 27 insertions(+), 2 deletions(-)

drivers/clk/clk-mux.c +27 -3

index 1ff90a6..353fcb7 100644
@@ -94,6 +94,9 @@ struct clk *clk_register_mux(struct device *dev, const char *name,
 ? ? ? ? ? ? ? ?u8 clk_mux_flags, spinlock_t *lock)
 ?{
 ? ? ? ?struct clk_mux *mux;
+ ? ? ? struct clk *clk;
+ ? ? ? const char **parents = NULL;
+ ? ? ? int i = 0;
 
 ? ? ? ?mux = kzalloc(sizeof(struct clk_mux), GFP_KERNEL);

@@ -109,6 +112,28 @@ struct clk *clk_register_mux(struct device *dev, const char *name,
 ? ? ? ?mux->flags = clk_mux_flags;
 ? ? ? ?mux->lock = lock;
 
- ? ? ? return clk_register(dev, name, &clk_mux_ops, &mux->hw,
- ? ? ? ? ? ? ? ? ? ? ? parent_names, num_parents, flags);
+ ? ? ? if (num_parents) {
+ ? ? ? ? ? ? ? parents = kzalloc(sizeof(char *) * num_parents, GFP_KERNEL);
+ ? ? ? ? ? ? ? if (!parents)
+ ? ? ? ? ? ? ? ? ? ? ? goto out;
+
+ ? ? ? ? ? ? ? for (i = 0; i < num_parents; i++) {
+ ? ? ? ? ? ? ? ? ? ? ? parents[i] = kstrdup(parent_names[i], GFP_KERNEL);
+ ? ? ? ? ? ? ? ? ? ? ? if (!parents[i])
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? goto out;
+ ? ? ? ? ? ? ? }
+ ? ? ? }
+
+ ? ? ? clk = ?clk_register(dev, name, &clk_mux_ops, &mux->hw,
+ ? ? ? ? ? ? ? ? ? ? ? parents, num_parents, flags);
+ ? ? ? if (clk)
+ ? ? ? ? ? ? ? return clk;
+
+out:
+ ? ? ? for (i--; i >= 0; i--)
+ ? ? ? ? ? ? ? kfree(parents[i]);
+ ? ? ? kfree(parents);
+ ? ? ? kfree(mux);
+
+ ? ? ? return NULL;
 ?}
--

1.7.5.4

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel