All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/6] clk: Initial feedback for off-SoC slow bus clocks
@ 2011-07-11  2:53 ` Mark Brown
  0 siblings, 0 replies; 72+ messages in thread
From: Mark Brown @ 2011-07-11  2:53 UTC (permalink / raw)
  To: linux-arm-kernel

I've just been having a go at implementing generic clk API support for
an off-SoC device on a slow bus, the clocking module in the wm831x/2x
series of PMICs.  Unfortunately as I don't currently have access to a
platform that has been converted to use the API I've not actually been
able to test the code but I'm reasonably optimistic that the code will
Just Work(tm) as the API seems fairly straightforward.

The biggest issue I ran into was that as the clocks are all registered
by name with the API if you've got two instances of the same off-SoC
device in the system you'll not be able to disambiguate between the
clocks it provides.  I've added a simple solution for this in the form
of a patch which takes a struct device as an argument and adds that to
the name of the registered clock.  This isn't ideal but should give
stable names which systems can use together with clkdev to match clocks
up with their users.  I believe that when we have device tree bindings
for clocks we should be able to use the device to access the bindings
and avoid this mangling - Grant, is that correct?

Otherwise everything seems to work pretty well from the driver side for
these devices, I've added some minor updates which came up naturally
while writing the driver code and seemed fairly obvious.  One of these
was a change to provide a user visible Kconfig option for building the
clock drivers, the idea being to make it easier to do build tests.  I
did also wonder if it's worth adding a patch to enable the API on x86
(which doesn't currently have a clock API) for coverage.

These patches (which include the two I posted yesterday) are against the
series you posted in May with the exception of the wm831x patch which
also has an additional dependency on "mfd: Add WM831x clock control
register definitions" which is due for merge in the next merge window.
If this stuff is all OK for you it would be good if you could include
the wm831x driver in your tree for this, especially given that there's
nothing in -next.

Mark Brown (6):
      clk: Prototype and document clk_register()
      clk: Provide a dummy clk_unregister()
      clk: Constify struct clk_hw_ops
      clk: Add Kconfig option to build all generic clk drivers
      clk: Support multiple instances of the same clock provider
      clk: Add initial WM831x clock driver

 MAINTAINERS              |    1 +
 drivers/clk/Kconfig      |   16 ++-
 drivers/clk/Makefile     |    1 +
 drivers/clk/clk-wm831x.c |  389 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/clk.c        |   38 ++++-
 include/linux/clk.h      |   33 ++++
 6 files changed, 472 insertions(+), 6 deletions(-)
 create mode 100644 drivers/clk/clk-wm831x.c

^ permalink raw reply	[flat|nested] 72+ messages in thread

* [PATCH 0/6] clk: Initial feedback for off-SoC slow bus clocks
@ 2011-07-11  2:53 ` Mark Brown
  0 siblings, 0 replies; 72+ messages in thread
From: Mark Brown @ 2011-07-11  2:53 UTC (permalink / raw)
  To: Jeremy Kerr
  Cc: linux-kernel, linux-arm-kernel, linux-sh, patches, Grant Likely

I've just been having a go at implementing generic clk API support for
an off-SoC device on a slow bus, the clocking module in the wm831x/2x
series of PMICs.  Unfortunately as I don't currently have access to a
platform that has been converted to use the API I've not actually been
able to test the code but I'm reasonably optimistic that the code will
Just Work(tm) as the API seems fairly straightforward.

The biggest issue I ran into was that as the clocks are all registered
by name with the API if you've got two instances of the same off-SoC
device in the system you'll not be able to disambiguate between the
clocks it provides.  I've added a simple solution for this in the form
of a patch which takes a struct device as an argument and adds that to
the name of the registered clock.  This isn't ideal but should give
stable names which systems can use together with clkdev to match clocks
up with their users.  I believe that when we have device tree bindings
for clocks we should be able to use the device to access the bindings
and avoid this mangling - Grant, is that correct?

Otherwise everything seems to work pretty well from the driver side for
these devices, I've added some minor updates which came up naturally
while writing the driver code and seemed fairly obvious.  One of these
was a change to provide a user visible Kconfig option for building the
clock drivers, the idea being to make it easier to do build tests.  I
did also wonder if it's worth adding a patch to enable the API on x86
(which doesn't currently have a clock API) for coverage.

These patches (which include the two I posted yesterday) are against the
series you posted in May with the exception of the wm831x patch which
also has an additional dependency on "mfd: Add WM831x clock control
register definitions" which is due for merge in the next merge window.
If this stuff is all OK for you it would be good if you could include
the wm831x driver in your tree for this, especially given that there's
nothing in -next.

Mark Brown (6):
      clk: Prototype and document clk_register()
      clk: Provide a dummy clk_unregister()
      clk: Constify struct clk_hw_ops
      clk: Add Kconfig option to build all generic clk drivers
      clk: Support multiple instances of the same clock provider
      clk: Add initial WM831x clock driver

 MAINTAINERS              |    1 +
 drivers/clk/Kconfig      |   16 ++-
 drivers/clk/Makefile     |    1 +
 drivers/clk/clk-wm831x.c |  389 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/clk.c        |   38 ++++-
 include/linux/clk.h      |   33 ++++
 6 files changed, 472 insertions(+), 6 deletions(-)
 create mode 100644 drivers/clk/clk-wm831x.c

^ permalink raw reply	[flat|nested] 72+ messages in thread

* [PATCH 0/6] clk: Initial feedback for off-SoC slow bus clocks
@ 2011-07-11  2:53 ` Mark Brown
  0 siblings, 0 replies; 72+ messages in thread
From: Mark Brown @ 2011-07-11  2:53 UTC (permalink / raw)
  To: linux-arm-kernel

I've just been having a go at implementing generic clk API support for
an off-SoC device on a slow bus, the clocking module in the wm831x/2x
series of PMICs.  Unfortunately as I don't currently have access to a
platform that has been converted to use the API I've not actually been
able to test the code but I'm reasonably optimistic that the code will
Just Work(tm) as the API seems fairly straightforward.

The biggest issue I ran into was that as the clocks are all registered
by name with the API if you've got two instances of the same off-SoC
device in the system you'll not be able to disambiguate between the
clocks it provides.  I've added a simple solution for this in the form
of a patch which takes a struct device as an argument and adds that to
the name of the registered clock.  This isn't ideal but should give
stable names which systems can use together with clkdev to match clocks
up with their users.  I believe that when we have device tree bindings
for clocks we should be able to use the device to access the bindings
and avoid this mangling - Grant, is that correct?

Otherwise everything seems to work pretty well from the driver side for
these devices, I've added some minor updates which came up naturally
while writing the driver code and seemed fairly obvious.  One of these
was a change to provide a user visible Kconfig option for building the
clock drivers, the idea being to make it easier to do build tests.  I
did also wonder if it's worth adding a patch to enable the API on x86
(which doesn't currently have a clock API) for coverage.

These patches (which include the two I posted yesterday) are against the
series you posted in May with the exception of the wm831x patch which
also has an additional dependency on "mfd: Add WM831x clock control
register definitions" which is due for merge in the next merge window.
If this stuff is all OK for you it would be good if you could include
the wm831x driver in your tree for this, especially given that there's
nothing in -next.

Mark Brown (6):
      clk: Prototype and document clk_register()
      clk: Provide a dummy clk_unregister()
      clk: Constify struct clk_hw_ops
      clk: Add Kconfig option to build all generic clk drivers
      clk: Support multiple instances of the same clock provider
      clk: Add initial WM831x clock driver

 MAINTAINERS              |    1 +
 drivers/clk/Kconfig      |   16 ++-
 drivers/clk/Makefile     |    1 +
 drivers/clk/clk-wm831x.c |  389 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/clk.c        |   38 ++++-
 include/linux/clk.h      |   33 ++++
 6 files changed, 472 insertions(+), 6 deletions(-)
 create mode 100644 drivers/clk/clk-wm831x.c

^ permalink raw reply	[flat|nested] 72+ messages in thread

* [PATCH 1/6] clk: Prototype and document clk_register()
  2011-07-11  2:53 ` Mark Brown
  (?)
@ 2011-07-11  2:53   ` Mark Brown
  -1 siblings, 0 replies; 72+ messages in thread
From: Mark Brown @ 2011-07-11  2:53 UTC (permalink / raw)
  To: linux-arm-kernel

This allows the compiler to ensure drivers are using the correct prototype.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 include/linux/clk.h |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/include/linux/clk.h b/include/linux/clk.h
index 7c26135..2ca4f66 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -136,6 +136,18 @@ extern struct clk_hw_ops clk_gate_ops;
 
 #endif /* CONFIG_GENERIC_CLK_GATE */
 
+/**
+ * clk_register - register and initialize a new clock
+ *
+ * @ops: ops for the new clock
+ * @hw: struct clk_hw to be passed to the ops of the new clock
+ * @name: name to use for the new clock
+ *
+ * Register a new clock with the clk subsytem.  Returns either a
+ * struct clk for the new clock or a NULL pointer.
+ */
+struct clk *clk_register(struct clk_hw_ops *ops, struct clk_hw *hw,
+			 const char *name);
 
 #else /* !CONFIG_GENERIC_CLK */
 
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 72+ messages in thread

* [PATCH 1/6] clk: Prototype and document clk_register()
@ 2011-07-11  2:53   ` Mark Brown
  0 siblings, 0 replies; 72+ messages in thread
From: Mark Brown @ 2011-07-11  2:53 UTC (permalink / raw)
  To: Jeremy Kerr
  Cc: Grant Likely, linux-kernel, linux-arm-kernel, linux-sh, patches,
	Mark Brown

This allows the compiler to ensure drivers are using the correct prototype.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 include/linux/clk.h |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/include/linux/clk.h b/include/linux/clk.h
index 7c26135..2ca4f66 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -136,6 +136,18 @@ extern struct clk_hw_ops clk_gate_ops;
 
 #endif /* CONFIG_GENERIC_CLK_GATE */
 
+/**
+ * clk_register - register and initialize a new clock
+ *
+ * @ops: ops for the new clock
+ * @hw: struct clk_hw to be passed to the ops of the new clock
+ * @name: name to use for the new clock
+ *
+ * Register a new clock with the clk subsytem.  Returns either a
+ * struct clk for the new clock or a NULL pointer.
+ */
+struct clk *clk_register(struct clk_hw_ops *ops, struct clk_hw *hw,
+			 const char *name);
 
 #else /* !CONFIG_GENERIC_CLK */
 
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 72+ messages in thread

* [PATCH 1/6] clk: Prototype and document clk_register()
@ 2011-07-11  2:53   ` Mark Brown
  0 siblings, 0 replies; 72+ messages in thread
From: Mark Brown @ 2011-07-11  2:53 UTC (permalink / raw)
  To: linux-arm-kernel

This allows the compiler to ensure drivers are using the correct prototype.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 include/linux/clk.h |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/include/linux/clk.h b/include/linux/clk.h
index 7c26135..2ca4f66 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -136,6 +136,18 @@ extern struct clk_hw_ops clk_gate_ops;
 
 #endif /* CONFIG_GENERIC_CLK_GATE */
 
+/**
+ * clk_register - register and initialize a new clock
+ *
+ * @ops: ops for the new clock
+ * @hw: struct clk_hw to be passed to the ops of the new clock
+ * @name: name to use for the new clock
+ *
+ * Register a new clock with the clk subsytem.  Returns either a
+ * struct clk for the new clock or a NULL pointer.
+ */
+struct clk *clk_register(struct clk_hw_ops *ops, struct clk_hw *hw,
+			 const char *name);
 
 #else /* !CONFIG_GENERIC_CLK */
 
-- 
1.7.5.4

^ permalink raw reply related	[flat|nested] 72+ messages in thread

* [PATCH 2/6] clk: Provide a dummy clk_unregister()
  2011-07-11  2:53   ` Mark Brown
  (?)
@ 2011-07-11  2:53     ` Mark Brown
  -1 siblings, 0 replies; 72+ messages in thread
From: Mark Brown @ 2011-07-11  2:53 UTC (permalink / raw)
  To: linux-arm-kernel

Even though unregistration is not actually supported by the clk API it is
still useful to provide a clk_unregister() so that drivers can implement
their unregistration code. This saves having to go back later and update
them once unregistration is possible.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 include/linux/clk.h |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/include/linux/clk.h b/include/linux/clk.h
index 2ca4f66..df5c64f 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -149,6 +149,21 @@ extern struct clk_hw_ops clk_gate_ops;
 struct clk *clk_register(struct clk_hw_ops *ops, struct clk_hw *hw,
 			 const char *name);
 
+/**
+ * clk_unregister - remove a clock
+ *
+ * @clk: clock to unregister
+ *
+ * Remove a clock from the clk subsystem.  This is currently not
+ * implemented but is provided to allow unregistration code to be
+ * written in drivers ready for use when an implementation is
+ * provided.
+ */
+static inline int clk_unregister(struct clk *clk)
+{
+	return -ENOTSUPP;
+}
+
 #else /* !CONFIG_GENERIC_CLK */
 
 /*
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 72+ messages in thread

* [PATCH 2/6] clk: Provide a dummy clk_unregister()
@ 2011-07-11  2:53     ` Mark Brown
  0 siblings, 0 replies; 72+ messages in thread
From: Mark Brown @ 2011-07-11  2:53 UTC (permalink / raw)
  To: Jeremy Kerr
  Cc: Grant Likely, linux-kernel, linux-arm-kernel, linux-sh, patches,
	Mark Brown

Even though unregistration is not actually supported by the clk API it is
still useful to provide a clk_unregister() so that drivers can implement
their unregistration code. This saves having to go back later and update
them once unregistration is possible.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 include/linux/clk.h |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/include/linux/clk.h b/include/linux/clk.h
index 2ca4f66..df5c64f 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -149,6 +149,21 @@ extern struct clk_hw_ops clk_gate_ops;
 struct clk *clk_register(struct clk_hw_ops *ops, struct clk_hw *hw,
 			 const char *name);
 
+/**
+ * clk_unregister - remove a clock
+ *
+ * @clk: clock to unregister
+ *
+ * Remove a clock from the clk subsystem.  This is currently not
+ * implemented but is provided to allow unregistration code to be
+ * written in drivers ready for use when an implementation is
+ * provided.
+ */
+static inline int clk_unregister(struct clk *clk)
+{
+	return -ENOTSUPP;
+}
+
 #else /* !CONFIG_GENERIC_CLK */
 
 /*
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 72+ messages in thread

* [PATCH 2/6] clk: Provide a dummy clk_unregister()
@ 2011-07-11  2:53     ` Mark Brown
  0 siblings, 0 replies; 72+ messages in thread
From: Mark Brown @ 2011-07-11  2:53 UTC (permalink / raw)
  To: linux-arm-kernel

Even though unregistration is not actually supported by the clk API it is
still useful to provide a clk_unregister() so that drivers can implement
their unregistration code. This saves having to go back later and update
them once unregistration is possible.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 include/linux/clk.h |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/include/linux/clk.h b/include/linux/clk.h
index 2ca4f66..df5c64f 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -149,6 +149,21 @@ extern struct clk_hw_ops clk_gate_ops;
 struct clk *clk_register(struct clk_hw_ops *ops, struct clk_hw *hw,
 			 const char *name);
 
+/**
+ * clk_unregister - remove a clock
+ *
+ * @clk: clock to unregister
+ *
+ * Remove a clock from the clk subsystem.  This is currently not
+ * implemented but is provided to allow unregistration code to be
+ * written in drivers ready for use when an implementation is
+ * provided.
+ */
+static inline int clk_unregister(struct clk *clk)
+{
+	return -ENOTSUPP;
+}
+
 #else /* !CONFIG_GENERIC_CLK */
 
 /*
-- 
1.7.5.4

^ permalink raw reply related	[flat|nested] 72+ messages in thread

* [PATCH 3/6] clk: Constify struct clk_hw_ops
  2011-07-11  2:53   ` Mark Brown
  (?)
@ 2011-07-11  2:53     ` Mark Brown
  -1 siblings, 0 replies; 72+ messages in thread
From: Mark Brown @ 2011-07-11  2:53 UTC (permalink / raw)
  To: linux-arm-kernel

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/clk/clk.c   |    4 ++--
 include/linux/clk.h |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 3a4d70e..1df6e23 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -16,7 +16,7 @@
 
 struct clk {
 	const char		*name;
-	struct clk_hw_ops	*ops;
+	const struct clk_hw_ops	*ops;
 	struct clk_hw		*hw;
 	unsigned int		enable_count;
 	unsigned int		prepare_count;
@@ -252,7 +252,7 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
 }
 EXPORT_SYMBOL_GPL(clk_set_parent);
 
-struct clk *clk_register(struct clk_hw_ops *ops, struct clk_hw *hw,
+struct clk *clk_register(const struct clk_hw_ops *ops, struct clk_hw *hw,
 		const char *name)
 {
 	struct clk *clk;
diff --git a/include/linux/clk.h b/include/linux/clk.h
index df5c64f..fb5e435 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -146,7 +146,7 @@ extern struct clk_hw_ops clk_gate_ops;
  * Register a new clock with the clk subsytem.  Returns either a
  * struct clk for the new clock or a NULL pointer.
  */
-struct clk *clk_register(struct clk_hw_ops *ops, struct clk_hw *hw,
+struct clk *clk_register(const struct clk_hw_ops *ops, struct clk_hw *hw,
 			 const char *name);
 
 /**
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 72+ messages in thread

* [PATCH 3/6] clk: Constify struct clk_hw_ops
@ 2011-07-11  2:53     ` Mark Brown
  0 siblings, 0 replies; 72+ messages in thread
From: Mark Brown @ 2011-07-11  2:53 UTC (permalink / raw)
  To: Jeremy Kerr
  Cc: Grant Likely, linux-kernel, linux-arm-kernel, linux-sh, patches,
	Mark Brown

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/clk/clk.c   |    4 ++--
 include/linux/clk.h |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 3a4d70e..1df6e23 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -16,7 +16,7 @@
 
 struct clk {
 	const char		*name;
-	struct clk_hw_ops	*ops;
+	const struct clk_hw_ops	*ops;
 	struct clk_hw		*hw;
 	unsigned int		enable_count;
 	unsigned int		prepare_count;
@@ -252,7 +252,7 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
 }
 EXPORT_SYMBOL_GPL(clk_set_parent);
 
-struct clk *clk_register(struct clk_hw_ops *ops, struct clk_hw *hw,
+struct clk *clk_register(const struct clk_hw_ops *ops, struct clk_hw *hw,
 		const char *name)
 {
 	struct clk *clk;
diff --git a/include/linux/clk.h b/include/linux/clk.h
index df5c64f..fb5e435 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -146,7 +146,7 @@ extern struct clk_hw_ops clk_gate_ops;
  * Register a new clock with the clk subsytem.  Returns either a
  * struct clk for the new clock or a NULL pointer.
  */
-struct clk *clk_register(struct clk_hw_ops *ops, struct clk_hw *hw,
+struct clk *clk_register(const struct clk_hw_ops *ops, struct clk_hw *hw,
 			 const char *name);
 
 /**
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 72+ messages in thread

* [PATCH 3/6] clk: Constify struct clk_hw_ops
@ 2011-07-11  2:53     ` Mark Brown
  0 siblings, 0 replies; 72+ messages in thread
From: Mark Brown @ 2011-07-11  2:53 UTC (permalink / raw)
  To: linux-arm-kernel

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/clk/clk.c   |    4 ++--
 include/linux/clk.h |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 3a4d70e..1df6e23 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -16,7 +16,7 @@
 
 struct clk {
 	const char		*name;
-	struct clk_hw_ops	*ops;
+	const struct clk_hw_ops	*ops;
 	struct clk_hw		*hw;
 	unsigned int		enable_count;
 	unsigned int		prepare_count;
@@ -252,7 +252,7 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
 }
 EXPORT_SYMBOL_GPL(clk_set_parent);
 
-struct clk *clk_register(struct clk_hw_ops *ops, struct clk_hw *hw,
+struct clk *clk_register(const struct clk_hw_ops *ops, struct clk_hw *hw,
 		const char *name)
 {
 	struct clk *clk;
diff --git a/include/linux/clk.h b/include/linux/clk.h
index df5c64f..fb5e435 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -146,7 +146,7 @@ extern struct clk_hw_ops clk_gate_ops;
  * Register a new clock with the clk subsytem.  Returns either a
  * struct clk for the new clock or a NULL pointer.
  */
-struct clk *clk_register(struct clk_hw_ops *ops, struct clk_hw *hw,
+struct clk *clk_register(const struct clk_hw_ops *ops, struct clk_hw *hw,
 			 const char *name);
 
 /**
-- 
1.7.5.4

^ permalink raw reply related	[flat|nested] 72+ messages in thread

* [PATCH 4/6] clk: Add Kconfig option to build all generic clk drivers
  2011-07-11  2:53   ` Mark Brown
  (?)
@ 2011-07-11  2:53     ` Mark Brown
  -1 siblings, 0 replies; 72+ messages in thread
From: Mark Brown @ 2011-07-11  2:53 UTC (permalink / raw)
  To: linux-arm-kernel

Currently drivers for the generic clk subsystem must be selected by
platforms using them in order to enable build. When doing development on
the API or generic build time testing it is useful to be able to build
unused drivers in order to improve coverage so supply a Kconfig option
which allows this.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/clk/Kconfig |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 75d2902..1fd0070 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -1,4 +1,3 @@
-
 config CLKDEV_LOOKUP
 	bool
 	select HAVE_CLK
@@ -6,6 +5,16 @@ config CLKDEV_LOOKUP
 config GENERIC_CLK
 	bool
 
+config GENERIC_CLK_BUILD_TEST
+	bool "Build all generic clock drivers"
+	depends on EXPERIMENTAL && GENERIC_CLK
+	select GENERIC_CLK_FIXED
+	select GENERIC_CLK_GATE
+	help
+	   Enable all possible generic clock drivers.  This is only
+	   useful for improving build coverage, it is not useful for
+	   production kernel builds.
+
 config GENERIC_CLK_FIXED
 	bool
 	depends on GENERIC_CLK
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 72+ messages in thread

* [PATCH 4/6] clk: Add Kconfig option to build all generic clk drivers
@ 2011-07-11  2:53     ` Mark Brown
  0 siblings, 0 replies; 72+ messages in thread
From: Mark Brown @ 2011-07-11  2:53 UTC (permalink / raw)
  To: Jeremy Kerr
  Cc: Grant Likely, linux-kernel, linux-arm-kernel, linux-sh, patches,
	Mark Brown

Currently drivers for the generic clk subsystem must be selected by
platforms using them in order to enable build. When doing development on
the API or generic build time testing it is useful to be able to build
unused drivers in order to improve coverage so supply a Kconfig option
which allows this.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/clk/Kconfig |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 75d2902..1fd0070 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -1,4 +1,3 @@
-
 config CLKDEV_LOOKUP
 	bool
 	select HAVE_CLK
@@ -6,6 +5,16 @@ config CLKDEV_LOOKUP
 config GENERIC_CLK
 	bool
 
+config GENERIC_CLK_BUILD_TEST
+	bool "Build all generic clock drivers"
+	depends on EXPERIMENTAL && GENERIC_CLK
+	select GENERIC_CLK_FIXED
+	select GENERIC_CLK_GATE
+	help
+	   Enable all possible generic clock drivers.  This is only
+	   useful for improving build coverage, it is not useful for
+	   production kernel builds.
+
 config GENERIC_CLK_FIXED
 	bool
 	depends on GENERIC_CLK
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 72+ messages in thread

* [PATCH 4/6] clk: Add Kconfig option to build all generic clk drivers
@ 2011-07-11  2:53     ` Mark Brown
  0 siblings, 0 replies; 72+ messages in thread
From: Mark Brown @ 2011-07-11  2:53 UTC (permalink / raw)
  To: linux-arm-kernel

Currently drivers for the generic clk subsystem must be selected by
platforms using them in order to enable build. When doing development on
the API or generic build time testing it is useful to be able to build
unused drivers in order to improve coverage so supply a Kconfig option
which allows this.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/clk/Kconfig |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 75d2902..1fd0070 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -1,4 +1,3 @@
-
 config CLKDEV_LOOKUP
 	bool
 	select HAVE_CLK
@@ -6,6 +5,16 @@ config CLKDEV_LOOKUP
 config GENERIC_CLK
 	bool
 
+config GENERIC_CLK_BUILD_TEST
+	bool "Build all generic clock drivers"
+	depends on EXPERIMENTAL && GENERIC_CLK
+	select GENERIC_CLK_FIXED
+	select GENERIC_CLK_GATE
+	help
+	   Enable all possible generic clock drivers.  This is only
+	   useful for improving build coverage, it is not useful for
+	   production kernel builds.
+
 config GENERIC_CLK_FIXED
 	bool
 	depends on GENERIC_CLK
-- 
1.7.5.4

^ permalink raw reply related	[flat|nested] 72+ messages in thread

* [PATCH 5/6] clk: Support multiple instances of the same clock provider
  2011-07-11  2:53   ` Mark Brown
  (?)
@ 2011-07-11  2:53     ` Mark Brown
  -1 siblings, 0 replies; 72+ messages in thread
From: Mark Brown @ 2011-07-11  2:53 UTC (permalink / raw)
  To: linux-arm-kernel

Currently the generic clk API identifies clocks internally using the name
of the clock. This is OK for on-SoC clocks where we have enough control to
disambiguate but doesn't work well for clocks provided on external chips
where a system design may include more than one instance of the same chip
(the Wolfson Speyside system is an example of this) or may have namespace
collisions.

Address this by allowing the clock provider to supply a struct device for
the clock for use in disambiguation. As a first pass if it is provided we
prefix the clock name with the dev_name() of the device. With a device
tree binding for clocks it should be possible to remove this mangling and
instead use the struct device to provide access to the binding information.

In order to avoid needless noise in names and memory usage it is strongly
recommended that on-SoC clocks do not provide a struct device until the
implementation is improved.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/clk/clk.c   |   36 ++++++++++++++++++++++++++++++++----
 include/linux/clk.h |   14 ++++++++++----
 2 files changed, 42 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 1df6e23..f36f637 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -13,6 +13,7 @@
 #include <linux/mutex.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
+#include <linux/device.h>
 
 struct clk {
 	const char		*name;
@@ -252,20 +253,44 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
 }
 EXPORT_SYMBOL_GPL(clk_set_parent);
 
-struct clk *clk_register(const struct clk_hw_ops *ops, struct clk_hw *hw,
-		const char *name)
+struct clk *clk_register(struct device *dev, const struct clk_hw_ops *ops,
+			 struct clk_hw *hw, const char *name)
 {
 	struct clk *clk;
+	char *new_name;
+	size_t name_len;
 
 	clk = kzalloc(sizeof(*clk), GFP_KERNEL);
 	if (!clk)
 		return NULL;
 
-	clk->name = name;
 	clk->ops = ops;
 	clk->hw = hw;
 	hw->clk = clk;
 
+	/* Since we currently match clock providers on a purely string
+	 * based method add a prefix based on the device name if a
+	 * device is provided.  When we have support for device tree
+	 * based clock matching it should be possible to avoid this
+	 * mangling and instead use the struct device to hook into
+	 * the bindings.
+	 *
+	 * As we don't currently support unregistering clocks we don't
+	 * need to worry about cleanup as yet.
+	 */
+	if (dev) {
+		name_len = strlen(name) + strlen(dev_name(dev)) + 2;
+		new_name = kzalloc(name_len, GFP_KERNEL);
+		if (!new_name)
+			goto err;
+
+		snprintf(new_name, name_len, "%s-%s", dev_name(dev), name);
+
+		clk->name = new_name;
+	} else {
+		clk->name = name;
+	}
+
 	/* Query the hardware for parent and initial rate. We may alter
 	 * the clock topology, making this clock available from the parent's
 	 * children list. So, we need to protect against concurrent
@@ -285,7 +310,10 @@ struct clk *clk_register(const struct clk_hw_ops *ops, struct clk_hw *hw,
 
 	mutex_unlock(&prepare_lock);
 
-
 	return clk;
+
+err:
+	kfree(clk);
+	return NULL;
 }
 EXPORT_SYMBOL_GPL(clk_register);
diff --git a/include/linux/clk.h b/include/linux/clk.h
index fb5e435..cb1879b 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -139,15 +139,21 @@ extern struct clk_hw_ops clk_gate_ops;
 /**
  * clk_register - register and initialize a new clock
  *
+ * @dev: device providing the clock or NULL
  * @ops: ops for the new clock
  * @hw: struct clk_hw to be passed to the ops of the new clock
  * @name: name to use for the new clock
  *
- * Register a new clock with the clk subsytem.  Returns either a
- * struct clk for the new clock or a NULL pointer.
+ * Register a new clock with the clk subsytem.  If dev is provided
+ * then it will be used to disambiguate between multiple instances of
+ * the same device in the system, typically this should only be done
+ * for devices that are not part of the core SoC unless device tree is
+ * in use.
+ *
+ * Returns either a struct clk for the new clock or a NULL pointer.
  */
-struct clk *clk_register(const struct clk_hw_ops *ops, struct clk_hw *hw,
-			 const char *name);
+struct clk *clk_register(struct device *dev, const struct clk_hw_ops *ops,
+			 struct clk_hw *hw, const char *name);
 
 /**
  * clk_unregister - remove a clock
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 72+ messages in thread

* [PATCH 5/6] clk: Support multiple instances of the same clock provider
@ 2011-07-11  2:53     ` Mark Brown
  0 siblings, 0 replies; 72+ messages in thread
From: Mark Brown @ 2011-07-11  2:53 UTC (permalink / raw)
  To: Jeremy Kerr
  Cc: Grant Likely, linux-kernel, linux-arm-kernel, linux-sh, patches,
	Mark Brown

Currently the generic clk API identifies clocks internally using the name
of the clock. This is OK for on-SoC clocks where we have enough control to
disambiguate but doesn't work well for clocks provided on external chips
where a system design may include more than one instance of the same chip
(the Wolfson Speyside system is an example of this) or may have namespace
collisions.

Address this by allowing the clock provider to supply a struct device for
the clock for use in disambiguation. As a first pass if it is provided we
prefix the clock name with the dev_name() of the device. With a device
tree binding for clocks it should be possible to remove this mangling and
instead use the struct device to provide access to the binding information.

In order to avoid needless noise in names and memory usage it is strongly
recommended that on-SoC clocks do not provide a struct device until the
implementation is improved.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/clk/clk.c   |   36 ++++++++++++++++++++++++++++++++----
 include/linux/clk.h |   14 ++++++++++----
 2 files changed, 42 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 1df6e23..f36f637 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -13,6 +13,7 @@
 #include <linux/mutex.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
+#include <linux/device.h>
 
 struct clk {
 	const char		*name;
@@ -252,20 +253,44 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
 }
 EXPORT_SYMBOL_GPL(clk_set_parent);
 
-struct clk *clk_register(const struct clk_hw_ops *ops, struct clk_hw *hw,
-		const char *name)
+struct clk *clk_register(struct device *dev, const struct clk_hw_ops *ops,
+			 struct clk_hw *hw, const char *name)
 {
 	struct clk *clk;
+	char *new_name;
+	size_t name_len;
 
 	clk = kzalloc(sizeof(*clk), GFP_KERNEL);
 	if (!clk)
 		return NULL;
 
-	clk->name = name;
 	clk->ops = ops;
 	clk->hw = hw;
 	hw->clk = clk;
 
+	/* Since we currently match clock providers on a purely string
+	 * based method add a prefix based on the device name if a
+	 * device is provided.  When we have support for device tree
+	 * based clock matching it should be possible to avoid this
+	 * mangling and instead use the struct device to hook into
+	 * the bindings.
+	 *
+	 * As we don't currently support unregistering clocks we don't
+	 * need to worry about cleanup as yet.
+	 */
+	if (dev) {
+		name_len = strlen(name) + strlen(dev_name(dev)) + 2;
+		new_name = kzalloc(name_len, GFP_KERNEL);
+		if (!new_name)
+			goto err;
+
+		snprintf(new_name, name_len, "%s-%s", dev_name(dev), name);
+
+		clk->name = new_name;
+	} else {
+		clk->name = name;
+	}
+
 	/* Query the hardware for parent and initial rate. We may alter
 	 * the clock topology, making this clock available from the parent's
 	 * children list. So, we need to protect against concurrent
@@ -285,7 +310,10 @@ struct clk *clk_register(const struct clk_hw_ops *ops, struct clk_hw *hw,
 
 	mutex_unlock(&prepare_lock);
 
-
 	return clk;
+
+err:
+	kfree(clk);
+	return NULL;
 }
 EXPORT_SYMBOL_GPL(clk_register);
diff --git a/include/linux/clk.h b/include/linux/clk.h
index fb5e435..cb1879b 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -139,15 +139,21 @@ extern struct clk_hw_ops clk_gate_ops;
 /**
  * clk_register - register and initialize a new clock
  *
+ * @dev: device providing the clock or NULL
  * @ops: ops for the new clock
  * @hw: struct clk_hw to be passed to the ops of the new clock
  * @name: name to use for the new clock
  *
- * Register a new clock with the clk subsytem.  Returns either a
- * struct clk for the new clock or a NULL pointer.
+ * Register a new clock with the clk subsytem.  If dev is provided
+ * then it will be used to disambiguate between multiple instances of
+ * the same device in the system, typically this should only be done
+ * for devices that are not part of the core SoC unless device tree is
+ * in use.
+ *
+ * Returns either a struct clk for the new clock or a NULL pointer.
  */
-struct clk *clk_register(const struct clk_hw_ops *ops, struct clk_hw *hw,
-			 const char *name);
+struct clk *clk_register(struct device *dev, const struct clk_hw_ops *ops,
+			 struct clk_hw *hw, const char *name);
 
 /**
  * clk_unregister - remove a clock
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 72+ messages in thread

* [PATCH 5/6] clk: Support multiple instances of the same clock provider
@ 2011-07-11  2:53     ` Mark Brown
  0 siblings, 0 replies; 72+ messages in thread
From: Mark Brown @ 2011-07-11  2:53 UTC (permalink / raw)
  To: linux-arm-kernel

Currently the generic clk API identifies clocks internally using the name
of the clock. This is OK for on-SoC clocks where we have enough control to
disambiguate but doesn't work well for clocks provided on external chips
where a system design may include more than one instance of the same chip
(the Wolfson Speyside system is an example of this) or may have namespace
collisions.

Address this by allowing the clock provider to supply a struct device for
the clock for use in disambiguation. As a first pass if it is provided we
prefix the clock name with the dev_name() of the device. With a device
tree binding for clocks it should be possible to remove this mangling and
instead use the struct device to provide access to the binding information.

In order to avoid needless noise in names and memory usage it is strongly
recommended that on-SoC clocks do not provide a struct device until the
implementation is improved.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/clk/clk.c   |   36 ++++++++++++++++++++++++++++++++----
 include/linux/clk.h |   14 ++++++++++----
 2 files changed, 42 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 1df6e23..f36f637 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -13,6 +13,7 @@
 #include <linux/mutex.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
+#include <linux/device.h>
 
 struct clk {
 	const char		*name;
@@ -252,20 +253,44 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
 }
 EXPORT_SYMBOL_GPL(clk_set_parent);
 
-struct clk *clk_register(const struct clk_hw_ops *ops, struct clk_hw *hw,
-		const char *name)
+struct clk *clk_register(struct device *dev, const struct clk_hw_ops *ops,
+			 struct clk_hw *hw, const char *name)
 {
 	struct clk *clk;
+	char *new_name;
+	size_t name_len;
 
 	clk = kzalloc(sizeof(*clk), GFP_KERNEL);
 	if (!clk)
 		return NULL;
 
-	clk->name = name;
 	clk->ops = ops;
 	clk->hw = hw;
 	hw->clk = clk;
 
+	/* Since we currently match clock providers on a purely string
+	 * based method add a prefix based on the device name if a
+	 * device is provided.  When we have support for device tree
+	 * based clock matching it should be possible to avoid this
+	 * mangling and instead use the struct device to hook into
+	 * the bindings.
+	 *
+	 * As we don't currently support unregistering clocks we don't
+	 * need to worry about cleanup as yet.
+	 */
+	if (dev) {
+		name_len = strlen(name) + strlen(dev_name(dev)) + 2;
+		new_name = kzalloc(name_len, GFP_KERNEL);
+		if (!new_name)
+			goto err;
+
+		snprintf(new_name, name_len, "%s-%s", dev_name(dev), name);
+
+		clk->name = new_name;
+	} else {
+		clk->name = name;
+	}
+
 	/* Query the hardware for parent and initial rate. We may alter
 	 * the clock topology, making this clock available from the parent's
 	 * children list. So, we need to protect against concurrent
@@ -285,7 +310,10 @@ struct clk *clk_register(const struct clk_hw_ops *ops, struct clk_hw *hw,
 
 	mutex_unlock(&prepare_lock);
 
-
 	return clk;
+
+err:
+	kfree(clk);
+	return NULL;
 }
 EXPORT_SYMBOL_GPL(clk_register);
diff --git a/include/linux/clk.h b/include/linux/clk.h
index fb5e435..cb1879b 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -139,15 +139,21 @@ extern struct clk_hw_ops clk_gate_ops;
 /**
  * clk_register - register and initialize a new clock
  *
+ * @dev: device providing the clock or NULL
  * @ops: ops for the new clock
  * @hw: struct clk_hw to be passed to the ops of the new clock
  * @name: name to use for the new clock
  *
- * Register a new clock with the clk subsytem.  Returns either a
- * struct clk for the new clock or a NULL pointer.
+ * Register a new clock with the clk subsytem.  If dev is provided
+ * then it will be used to disambiguate between multiple instances of
+ * the same device in the system, typically this should only be done
+ * for devices that are not part of the core SoC unless device tree is
+ * in use.
+ *
+ * Returns either a struct clk for the new clock or a NULL pointer.
  */
-struct clk *clk_register(const struct clk_hw_ops *ops, struct clk_hw *hw,
-			 const char *name);
+struct clk *clk_register(struct device *dev, const struct clk_hw_ops *ops,
+			 struct clk_hw *hw, const char *name);
 
 /**
  * clk_unregister - remove a clock
-- 
1.7.5.4

^ permalink raw reply related	[flat|nested] 72+ messages in thread

* [PATCH 6/6] clk: Add initial WM831x clock driver
  2011-07-11  2:53   ` Mark Brown
  (?)
@ 2011-07-11  2:53     ` Mark Brown
  -1 siblings, 0 replies; 72+ messages in thread
From: Mark Brown @ 2011-07-11  2:53 UTC (permalink / raw)
  To: linux-arm-kernel

The WM831x and WM832x series of PMICs contain a flexible clocking
subsystem intended to provide always on and system core clocks.  It
features:

- A 32.768kHz crystal oscillator which can optionally be used to pass
  through an externally generated clock.
- A FLL which can be clocked from either the 32.768kHz oscillator or
  the CLKIN pin.
- A CLKOUT pin which can bring out either the oscillator or the FLL
  output.
- The 32.768kHz clock can also optionally be brought out on the GPIO
  pins of the device.

This driver fully supports the 32.768kHz oscillator and CLKOUT.  The FLL
is supported only in AUTO mode, the full flexibility of the FLL cannot
currently be used.  The use of clock references other than the internal
oscillator is not currently supported, and since clk_set_parent() is not
implemented in the generic clock API the clock tree configuration cannot
be changed at runtime.

Due to a lack of access to systems where the core SoC has been converted
to use the generic clock API this driver has been compile tested only.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 MAINTAINERS              |    1 +
 drivers/clk/Kconfig      |    5 +
 drivers/clk/Makefile     |    1 +
 drivers/clk/clk-wm831x.c |  389 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 396 insertions(+), 0 deletions(-)
 create mode 100644 drivers/clk/clk-wm831x.c

diff --git a/MAINTAINERS b/MAINTAINERS
index ae563fa..c234756 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6969,6 +6969,7 @@ T:	git git://opensource.wolfsonmicro.com/linux-2.6-audioplus
 W:	http://opensource.wolfsonmicro.com/content/linux-drivers-wolfson-devices
 S:	Supported
 F:	Documentation/hwmon/wm83??
+F:	drivers/clk/clk-wm83*.c
 F:	drivers/leds/leds-wm83*.c
 F:	drivers/mfd/wm8*.c
 F:	drivers/power/wm83*.c
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 1fd0070..7f6eec2 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -10,6 +10,7 @@ config GENERIC_CLK_BUILD_TEST
 	depends on EXPERIMENTAL && GENERIC_CLK
 	select GENERIC_CLK_FIXED
 	select GENERIC_CLK_GATE
+	select GENERIC_CLK_WM831X if MFD_WM831X=y
 	help
 	   Enable all possible generic clock drivers.  This is only
 	   useful for improving build coverage, it is not useful for
@@ -22,3 +23,7 @@ config GENERIC_CLK_FIXED
 config GENERIC_CLK_GATE
 	bool
 	depends on GENERIC_CLK
+
+config GENERIC_CLK_WM831X
+	tristate
+	depends on GENERIC_CLK && MFD_WM831X
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index d186446..6628ad5 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -3,3 +3,4 @@ obj-$(CONFIG_CLKDEV_LOOKUP)	+= clkdev.o
 obj-$(CONFIG_GENERIC_CLK)	+= clk.o
 obj-$(CONFIG_GENERIC_CLK_FIXED)	+= clk-fixed.o
 obj-$(CONFIG_GENERIC_CLK_GATE)	+= clk-gate.o
+obj-$(CONFIG_GENERIC_CLK_WM831X) += clk-wm831x.o
diff --git a/drivers/clk/clk-wm831x.c b/drivers/clk/clk-wm831x.c
new file mode 100644
index 0000000..82782f1
--- /dev/null
+++ b/drivers/clk/clk-wm831x.c
@@ -0,0 +1,389 @@
+/*
+ * WM831x clock control
+ *
+ * Copyright 2011 Wolfson Microelectronics PLC.
+ *
+ * Author: Mark Brown <broonie@opensource.wolfsonmicro.com>
+ *
+ *  This program is free software; you can redistribute  it and/or modify it
+ *  under  the terms of  the GNU General  Public License as published by the
+ *  Free Software Foundation;  either version 2 of the  License, or (at your
+ *  option) any later version.
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+#include <linux/mfd/wm831x/core.h>
+
+struct wm831x_clk {
+	struct wm831x *wm831x;
+	struct clk_hw xtal_hw;
+	struct clk_hw fll_hw;
+	struct clk_hw clkout_hw;
+	bool xtal_ena;
+};
+
+static int wm831x_xtal_enable(struct clk_hw *hw)
+{
+	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
+						  xtal_hw);
+
+	if (clkdata->xtal_ena)
+		return 0;
+	else
+		return -EPERM;
+}
+
+static unsigned long wm831x_xtal_recalc_rate(struct clk_hw *hw)
+{
+	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
+						  xtal_hw);
+
+	if (clkdata->xtal_ena)
+		return 32768;
+	else
+		return 0;
+}
+
+static long wm831x_xtal_round_rate(struct clk_hw *hw, unsigned long rate)
+{
+	return wm831x_xtal_recalc_rate(hw);
+}
+
+static const struct clk_hw_ops wm831x_xtal_ops = {
+	.enable = wm831x_xtal_enable,
+	.recalc_rate = wm831x_xtal_recalc_rate,
+	.round_rate = wm831x_xtal_round_rate,
+};
+
+static const unsigned long wm831x_fll_auto_rates[] = {
+	 2048000,
+	11289600,
+	12000000,
+	12288000,
+	19200000,
+	22579600,
+	24000000,
+	24576000,
+};
+
+static bool wm831x_fll_enabled(struct wm831x *wm831x)
+{
+	int ret;
+
+	ret = wm831x_reg_read(wm831x, WM831X_FLL_CONTROL_1);
+	if (ret < 0) {
+		dev_err(wm831x->dev, "Unable to read FLL_CONTROL_1: %d\n",
+			ret);
+		return true;
+	}
+
+	if (ret & WM831X_FLL_ENA)
+		return true;
+	else
+		return false;
+}
+
+static int wm831x_fll_prepare(struct clk_hw *hw)
+{
+	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
+						  fll_hw);
+	struct wm831x *wm831x = clkdata->wm831x;
+	int ret;
+
+	ret = wm831x_set_bits(wm831x, WM831X_FLL_CONTROL_2,
+			      WM831X_FLL_ENA, WM831X_FLL_ENA);
+	if (ret != 0)
+		dev_crit(wm831x->dev, "Failed to enable FLL: %d\n", ret);
+
+	return ret;
+}
+
+static void wm831x_fll_unprepare(struct clk_hw *hw)
+{
+	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
+						  fll_hw);
+	struct wm831x *wm831x = clkdata->wm831x;
+	int ret;
+
+	ret = wm831x_set_bits(wm831x, WM831X_FLL_CONTROL_2, WM831X_FLL_ENA, 0);
+	if (ret != 0)
+		dev_crit(wm831x->dev, "Failed to disaable FLL: %d\n", ret);
+}
+
+static unsigned long wm831x_fll_recalc_rate(struct clk_hw *hw)
+{
+	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
+						  fll_hw);
+	struct wm831x *wm831x = clkdata->wm831x;
+	int ret;
+
+	ret = wm831x_reg_read(wm831x, WM831X_CLOCK_CONTROL_2);
+	if (ret < 0) {
+		dev_err(wm831x->dev, "Unable to read CLOCK_CONTROL_2: %d\n",
+			ret);
+		return 0;
+	}
+
+	if (ret & WM831X_FLL_AUTO)
+		return wm831x_fll_auto_rates[ret & WM831X_FLL_AUTO_FREQ_MASK];
+
+	dev_err(wm831x->dev, "FLL only supported in AUTO mode\n");
+	return 0;
+}
+
+static int wm831x_fll_set_rate(struct clk_hw *hw, unsigned long rate,
+			       unsigned long *parent_rate)
+{
+	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
+						  fll_hw);
+	struct wm831x *wm831x = clkdata->wm831x;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(wm831x_fll_auto_rates); i++)
+		if (wm831x_fll_auto_rates[i] = rate)
+			break;
+	if (i = ARRAY_SIZE(wm831x_fll_auto_rates))
+		return -EINVAL;
+
+	if (wm831x_fll_enabled(wm831x))
+		return -EPERM;
+
+	return wm831x_set_bits(wm831x, WM831X_CLOCK_CONTROL_2,
+			       WM831X_FLL_AUTO_FREQ_MASK, i);
+}
+
+static struct clk *wm831x_fll_get_parent(struct clk_hw *hw)
+{
+	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
+						  fll_hw);
+	struct wm831x *wm831x = clkdata->wm831x;
+	int ret;
+
+	/* AUTO mode is always clocked from the crystal */
+	ret = wm831x_reg_read(wm831x, WM831X_CLOCK_CONTROL_2);
+	if (ret < 0) {
+		dev_err(wm831x->dev, "Unable to read CLOCK_CONTROL_2: %d\n",
+			ret);
+		return NULL;
+	}
+
+	if (ret & WM831X_FLL_AUTO)
+		return clkdata->xtal_hw.clk;
+
+	ret = wm831x_reg_read(wm831x, WM831X_FLL_CONTROL_5);
+	if (ret < 0) {
+		dev_err(wm831x->dev, "Unable to read FLL_CONTROL_5: %d\n",
+			ret);
+		return NULL;
+	}
+
+	switch (ret & WM831X_FLL_CLK_SRC_MASK) {
+	case 0:
+		return clkdata->xtal_hw.clk;
+	case 1:
+		dev_warn(wm831x->dev,
+			 "FLL clocked from CLKIN not yet supported\n");
+		return NULL;
+	default:
+		dev_err(wm831x->dev, "Unsupported FLL clock source %d\n",
+			ret & WM831X_FLL_CLK_SRC_MASK);
+		return NULL;
+	}
+}
+
+static const struct clk_hw_ops wm831x_fll_ops = {
+	.prepare = wm831x_fll_prepare,
+	.unprepare = wm831x_fll_unprepare,
+	.recalc_rate = wm831x_fll_recalc_rate,
+	.set_rate = wm831x_fll_set_rate,
+	.get_parent = wm831x_fll_get_parent,
+};
+
+static int wm831x_clkout_prepare(struct clk_hw *hw)
+{
+	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
+						  clkout_hw);
+	struct wm831x *wm831x = clkdata->wm831x;
+	int ret;
+
+	ret = wm831x_reg_unlock(wm831x);
+	if (ret != 0) {
+		dev_crit(wm831x->dev, "Failed to lock registers: %d\n", ret);
+		return ret;
+	}
+
+	ret = wm831x_set_bits(wm831x, WM831X_CLOCK_CONTROL_1,
+			      WM831X_CLKOUT_ENA, WM831X_CLKOUT_ENA);
+	if (ret != 0)
+		dev_crit(wm831x->dev, "Failed to enable CLKOUT: %d\n", ret);
+
+	wm831x_reg_lock(wm831x);
+
+	return ret;
+}
+
+static void wm831x_clkout_unprepare(struct clk_hw *hw)
+{
+	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
+						  clkout_hw);
+	struct wm831x *wm831x = clkdata->wm831x;
+	int ret;
+
+	ret = wm831x_reg_unlock(wm831x);
+	if (ret != 0) {
+		dev_crit(wm831x->dev, "Failed to lock registers: %d\n", ret);
+		return;
+	}
+
+	ret = wm831x_set_bits(wm831x, WM831X_CLOCK_CONTROL_1,
+			      WM831X_CLKOUT_ENA, 0);
+	if (ret != 0)
+		dev_crit(wm831x->dev, "Failed to disable CLKOUT: %d\n", ret);
+
+	wm831x_reg_lock(wm831x);
+}
+
+static unsigned long wm831x_clkout_recalc_rate(struct clk_hw *hw)
+{
+	return clk_get_rate(clk_get_parent(hw->clk));
+}
+
+static long wm831x_clkout_round_rate(struct clk_hw *hw, unsigned long rate)
+{
+	return clk_round_rate(clk_get_parent(hw->clk), rate);
+}
+
+static int wm831x_clkout_set_rate(struct clk_hw *hw, unsigned long rate,
+				  unsigned long *parent_rate)
+{
+	*parent_rate = rate;
+	return CLK_SET_RATE_PROPAGATE;
+}
+
+static struct clk *wm831x_clkout_get_parent(struct clk_hw *hw)
+{
+	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
+						  clkout_hw);
+	struct wm831x *wm831x = clkdata->wm831x;
+	int ret;
+
+	ret = wm831x_reg_read(wm831x, WM831X_CLOCK_CONTROL_1);
+	if (ret < 0) {
+		dev_err(wm831x->dev, "Unable to read CLOCK_CONTROL_1: %d\n",
+			ret);
+		return NULL;
+	}
+
+	if (ret & WM831X_CLKOUT_SRC)
+		return clkdata->xtal_hw.clk;
+	else
+		return clkdata->fll_hw.clk;
+}
+
+static const struct clk_hw_ops wm831x_clkout_ops = {
+	.prepare = wm831x_clkout_prepare,
+	.unprepare = wm831x_clkout_unprepare,
+	.recalc_rate = wm831x_clkout_recalc_rate,
+	.round_rate = wm831x_clkout_round_rate,
+	.set_rate = wm831x_clkout_set_rate,
+	.get_parent = wm831x_clkout_get_parent,
+};
+
+static __devinit int wm831x_clk_probe(struct platform_device *pdev)
+{
+	struct wm831x *wm831x = dev_get_drvdata(pdev->dev.parent);
+	struct wm831x_clk *clkdata;
+	int ret;
+
+	clkdata = kzalloc(sizeof(*clkdata), GFP_KERNEL);
+	if (!clkdata)
+		return -ENOMEM;
+
+	/* XTAL_ENA can only be set via OTP/InstantConfig so just read once */
+	ret = wm831x_reg_read(wm831x, WM831X_CLOCK_CONTROL_2);
+	if (ret < 0) {
+		dev_err(wm831x->dev, "Unable to read CLOCK_CONTROL_2: %d\n",
+			ret);
+		goto err_alloc;
+	}
+	clkdata->xtal_ena = ret & WM831X_XTAL_ENA;
+
+	if (!clk_register(wm831x->dev, &wm831x_xtal_ops, &clkdata->xtal_hw,
+			  "xtal")) {
+		ret = -EINVAL;
+		goto err_alloc;
+	}
+
+	if (!clk_register(wm831x->dev, &wm831x_fll_ops, &clkdata->fll_hw,
+			  "fll")) {
+		ret = -EINVAL;
+		goto err_xtal;
+	}
+
+	if (!clk_register(wm831x->dev, &wm831x_clkout_ops, &clkdata->clkout_hw,
+			  "clkout")) {
+		ret = -EINVAL;
+		goto err_fll;
+	}
+
+	dev_set_drvdata(&pdev->dev, clkdata);
+
+	return 0;
+
+err_fll:
+	clk_unregister(clkdata->fll_hw.clk);
+err_xtal:
+	clk_unregister(clkdata->xtal_hw.clk);
+err_alloc:
+	kfree(clkdata);
+	return ret;
+}
+
+static __devexit int wm831x_clk_remove(struct platform_device *pdev)
+{
+	struct wm831x_clk *clkdata = dev_get_drvdata(&pdev->dev);
+
+	clk_unregister(clkdata->clkout_hw.clk);
+	clk_unregister(clkdata->fll_hw.clk);
+	clk_unregister(clkdata->xtal_hw.clk);
+	kfree(clkdata);
+
+	return 0;
+}
+
+static struct platform_driver wm831x_clk_driver = {
+	.probe = wm831x_clk_probe,
+	.remove = __devexit_p(wm831x_clk_remove),
+	.driver		= {
+		.name	= "wm831x-clk",
+		.owner	= THIS_MODULE,
+	},
+};
+
+static int __init wm831x_clk_init(void)
+{
+	int ret;
+
+	ret = platform_driver_register(&wm831x_clk_driver);
+	if (ret != 0)
+		pr_err("Failed to register WM831x clock driver: %d\n", ret);
+
+	return ret;
+}
+module_init(wm831x_clk_init);
+
+static void __exit wm831x_clk_exit(void)
+{
+	platform_driver_unregister(&wm831x_clk_driver);
+}
+module_exit(wm831x_clk_exit);
+
+/* Module information */
+MODULE_AUTHOR("Mark Brown <broonie@opensource.wolfsonmicro.com>");
+MODULE_DESCRIPTION("WM831x clock driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:wm831x-clk");
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 72+ messages in thread

* [PATCH 6/6] clk: Add initial WM831x clock driver
@ 2011-07-11  2:53     ` Mark Brown
  0 siblings, 0 replies; 72+ messages in thread
From: Mark Brown @ 2011-07-11  2:53 UTC (permalink / raw)
  To: Jeremy Kerr
  Cc: Grant Likely, linux-kernel, linux-arm-kernel, linux-sh, patches,
	Mark Brown

The WM831x and WM832x series of PMICs contain a flexible clocking
subsystem intended to provide always on and system core clocks.  It
features:

- A 32.768kHz crystal oscillator which can optionally be used to pass
  through an externally generated clock.
- A FLL which can be clocked from either the 32.768kHz oscillator or
  the CLKIN pin.
- A CLKOUT pin which can bring out either the oscillator or the FLL
  output.
- The 32.768kHz clock can also optionally be brought out on the GPIO
  pins of the device.

This driver fully supports the 32.768kHz oscillator and CLKOUT.  The FLL
is supported only in AUTO mode, the full flexibility of the FLL cannot
currently be used.  The use of clock references other than the internal
oscillator is not currently supported, and since clk_set_parent() is not
implemented in the generic clock API the clock tree configuration cannot
be changed at runtime.

Due to a lack of access to systems where the core SoC has been converted
to use the generic clock API this driver has been compile tested only.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 MAINTAINERS              |    1 +
 drivers/clk/Kconfig      |    5 +
 drivers/clk/Makefile     |    1 +
 drivers/clk/clk-wm831x.c |  389 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 396 insertions(+), 0 deletions(-)
 create mode 100644 drivers/clk/clk-wm831x.c

diff --git a/MAINTAINERS b/MAINTAINERS
index ae563fa..c234756 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6969,6 +6969,7 @@ T:	git git://opensource.wolfsonmicro.com/linux-2.6-audioplus
 W:	http://opensource.wolfsonmicro.com/content/linux-drivers-wolfson-devices
 S:	Supported
 F:	Documentation/hwmon/wm83??
+F:	drivers/clk/clk-wm83*.c
 F:	drivers/leds/leds-wm83*.c
 F:	drivers/mfd/wm8*.c
 F:	drivers/power/wm83*.c
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 1fd0070..7f6eec2 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -10,6 +10,7 @@ config GENERIC_CLK_BUILD_TEST
 	depends on EXPERIMENTAL && GENERIC_CLK
 	select GENERIC_CLK_FIXED
 	select GENERIC_CLK_GATE
+	select GENERIC_CLK_WM831X if MFD_WM831X=y
 	help
 	   Enable all possible generic clock drivers.  This is only
 	   useful for improving build coverage, it is not useful for
@@ -22,3 +23,7 @@ config GENERIC_CLK_FIXED
 config GENERIC_CLK_GATE
 	bool
 	depends on GENERIC_CLK
+
+config GENERIC_CLK_WM831X
+	tristate
+	depends on GENERIC_CLK && MFD_WM831X
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index d186446..6628ad5 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -3,3 +3,4 @@ obj-$(CONFIG_CLKDEV_LOOKUP)	+= clkdev.o
 obj-$(CONFIG_GENERIC_CLK)	+= clk.o
 obj-$(CONFIG_GENERIC_CLK_FIXED)	+= clk-fixed.o
 obj-$(CONFIG_GENERIC_CLK_GATE)	+= clk-gate.o
+obj-$(CONFIG_GENERIC_CLK_WM831X) += clk-wm831x.o
diff --git a/drivers/clk/clk-wm831x.c b/drivers/clk/clk-wm831x.c
new file mode 100644
index 0000000..82782f1
--- /dev/null
+++ b/drivers/clk/clk-wm831x.c
@@ -0,0 +1,389 @@
+/*
+ * WM831x clock control
+ *
+ * Copyright 2011 Wolfson Microelectronics PLC.
+ *
+ * Author: Mark Brown <broonie@opensource.wolfsonmicro.com>
+ *
+ *  This program is free software; you can redistribute  it and/or modify it
+ *  under  the terms of  the GNU General  Public License as published by the
+ *  Free Software Foundation;  either version 2 of the  License, or (at your
+ *  option) any later version.
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+#include <linux/mfd/wm831x/core.h>
+
+struct wm831x_clk {
+	struct wm831x *wm831x;
+	struct clk_hw xtal_hw;
+	struct clk_hw fll_hw;
+	struct clk_hw clkout_hw;
+	bool xtal_ena;
+};
+
+static int wm831x_xtal_enable(struct clk_hw *hw)
+{
+	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
+						  xtal_hw);
+
+	if (clkdata->xtal_ena)
+		return 0;
+	else
+		return -EPERM;
+}
+
+static unsigned long wm831x_xtal_recalc_rate(struct clk_hw *hw)
+{
+	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
+						  xtal_hw);
+
+	if (clkdata->xtal_ena)
+		return 32768;
+	else
+		return 0;
+}
+
+static long wm831x_xtal_round_rate(struct clk_hw *hw, unsigned long rate)
+{
+	return wm831x_xtal_recalc_rate(hw);
+}
+
+static const struct clk_hw_ops wm831x_xtal_ops = {
+	.enable = wm831x_xtal_enable,
+	.recalc_rate = wm831x_xtal_recalc_rate,
+	.round_rate = wm831x_xtal_round_rate,
+};
+
+static const unsigned long wm831x_fll_auto_rates[] = {
+	 2048000,
+	11289600,
+	12000000,
+	12288000,
+	19200000,
+	22579600,
+	24000000,
+	24576000,
+};
+
+static bool wm831x_fll_enabled(struct wm831x *wm831x)
+{
+	int ret;
+
+	ret = wm831x_reg_read(wm831x, WM831X_FLL_CONTROL_1);
+	if (ret < 0) {
+		dev_err(wm831x->dev, "Unable to read FLL_CONTROL_1: %d\n",
+			ret);
+		return true;
+	}
+
+	if (ret & WM831X_FLL_ENA)
+		return true;
+	else
+		return false;
+}
+
+static int wm831x_fll_prepare(struct clk_hw *hw)
+{
+	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
+						  fll_hw);
+	struct wm831x *wm831x = clkdata->wm831x;
+	int ret;
+
+	ret = wm831x_set_bits(wm831x, WM831X_FLL_CONTROL_2,
+			      WM831X_FLL_ENA, WM831X_FLL_ENA);
+	if (ret != 0)
+		dev_crit(wm831x->dev, "Failed to enable FLL: %d\n", ret);
+
+	return ret;
+}
+
+static void wm831x_fll_unprepare(struct clk_hw *hw)
+{
+	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
+						  fll_hw);
+	struct wm831x *wm831x = clkdata->wm831x;
+	int ret;
+
+	ret = wm831x_set_bits(wm831x, WM831X_FLL_CONTROL_2, WM831X_FLL_ENA, 0);
+	if (ret != 0)
+		dev_crit(wm831x->dev, "Failed to disaable FLL: %d\n", ret);
+}
+
+static unsigned long wm831x_fll_recalc_rate(struct clk_hw *hw)
+{
+	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
+						  fll_hw);
+	struct wm831x *wm831x = clkdata->wm831x;
+	int ret;
+
+	ret = wm831x_reg_read(wm831x, WM831X_CLOCK_CONTROL_2);
+	if (ret < 0) {
+		dev_err(wm831x->dev, "Unable to read CLOCK_CONTROL_2: %d\n",
+			ret);
+		return 0;
+	}
+
+	if (ret & WM831X_FLL_AUTO)
+		return wm831x_fll_auto_rates[ret & WM831X_FLL_AUTO_FREQ_MASK];
+
+	dev_err(wm831x->dev, "FLL only supported in AUTO mode\n");
+	return 0;
+}
+
+static int wm831x_fll_set_rate(struct clk_hw *hw, unsigned long rate,
+			       unsigned long *parent_rate)
+{
+	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
+						  fll_hw);
+	struct wm831x *wm831x = clkdata->wm831x;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(wm831x_fll_auto_rates); i++)
+		if (wm831x_fll_auto_rates[i] == rate)
+			break;
+	if (i == ARRAY_SIZE(wm831x_fll_auto_rates))
+		return -EINVAL;
+
+	if (wm831x_fll_enabled(wm831x))
+		return -EPERM;
+
+	return wm831x_set_bits(wm831x, WM831X_CLOCK_CONTROL_2,
+			       WM831X_FLL_AUTO_FREQ_MASK, i);
+}
+
+static struct clk *wm831x_fll_get_parent(struct clk_hw *hw)
+{
+	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
+						  fll_hw);
+	struct wm831x *wm831x = clkdata->wm831x;
+	int ret;
+
+	/* AUTO mode is always clocked from the crystal */
+	ret = wm831x_reg_read(wm831x, WM831X_CLOCK_CONTROL_2);
+	if (ret < 0) {
+		dev_err(wm831x->dev, "Unable to read CLOCK_CONTROL_2: %d\n",
+			ret);
+		return NULL;
+	}
+
+	if (ret & WM831X_FLL_AUTO)
+		return clkdata->xtal_hw.clk;
+
+	ret = wm831x_reg_read(wm831x, WM831X_FLL_CONTROL_5);
+	if (ret < 0) {
+		dev_err(wm831x->dev, "Unable to read FLL_CONTROL_5: %d\n",
+			ret);
+		return NULL;
+	}
+
+	switch (ret & WM831X_FLL_CLK_SRC_MASK) {
+	case 0:
+		return clkdata->xtal_hw.clk;
+	case 1:
+		dev_warn(wm831x->dev,
+			 "FLL clocked from CLKIN not yet supported\n");
+		return NULL;
+	default:
+		dev_err(wm831x->dev, "Unsupported FLL clock source %d\n",
+			ret & WM831X_FLL_CLK_SRC_MASK);
+		return NULL;
+	}
+}
+
+static const struct clk_hw_ops wm831x_fll_ops = {
+	.prepare = wm831x_fll_prepare,
+	.unprepare = wm831x_fll_unprepare,
+	.recalc_rate = wm831x_fll_recalc_rate,
+	.set_rate = wm831x_fll_set_rate,
+	.get_parent = wm831x_fll_get_parent,
+};
+
+static int wm831x_clkout_prepare(struct clk_hw *hw)
+{
+	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
+						  clkout_hw);
+	struct wm831x *wm831x = clkdata->wm831x;
+	int ret;
+
+	ret = wm831x_reg_unlock(wm831x);
+	if (ret != 0) {
+		dev_crit(wm831x->dev, "Failed to lock registers: %d\n", ret);
+		return ret;
+	}
+
+	ret = wm831x_set_bits(wm831x, WM831X_CLOCK_CONTROL_1,
+			      WM831X_CLKOUT_ENA, WM831X_CLKOUT_ENA);
+	if (ret != 0)
+		dev_crit(wm831x->dev, "Failed to enable CLKOUT: %d\n", ret);
+
+	wm831x_reg_lock(wm831x);
+
+	return ret;
+}
+
+static void wm831x_clkout_unprepare(struct clk_hw *hw)
+{
+	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
+						  clkout_hw);
+	struct wm831x *wm831x = clkdata->wm831x;
+	int ret;
+
+	ret = wm831x_reg_unlock(wm831x);
+	if (ret != 0) {
+		dev_crit(wm831x->dev, "Failed to lock registers: %d\n", ret);
+		return;
+	}
+
+	ret = wm831x_set_bits(wm831x, WM831X_CLOCK_CONTROL_1,
+			      WM831X_CLKOUT_ENA, 0);
+	if (ret != 0)
+		dev_crit(wm831x->dev, "Failed to disable CLKOUT: %d\n", ret);
+
+	wm831x_reg_lock(wm831x);
+}
+
+static unsigned long wm831x_clkout_recalc_rate(struct clk_hw *hw)
+{
+	return clk_get_rate(clk_get_parent(hw->clk));
+}
+
+static long wm831x_clkout_round_rate(struct clk_hw *hw, unsigned long rate)
+{
+	return clk_round_rate(clk_get_parent(hw->clk), rate);
+}
+
+static int wm831x_clkout_set_rate(struct clk_hw *hw, unsigned long rate,
+				  unsigned long *parent_rate)
+{
+	*parent_rate = rate;
+	return CLK_SET_RATE_PROPAGATE;
+}
+
+static struct clk *wm831x_clkout_get_parent(struct clk_hw *hw)
+{
+	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
+						  clkout_hw);
+	struct wm831x *wm831x = clkdata->wm831x;
+	int ret;
+
+	ret = wm831x_reg_read(wm831x, WM831X_CLOCK_CONTROL_1);
+	if (ret < 0) {
+		dev_err(wm831x->dev, "Unable to read CLOCK_CONTROL_1: %d\n",
+			ret);
+		return NULL;
+	}
+
+	if (ret & WM831X_CLKOUT_SRC)
+		return clkdata->xtal_hw.clk;
+	else
+		return clkdata->fll_hw.clk;
+}
+
+static const struct clk_hw_ops wm831x_clkout_ops = {
+	.prepare = wm831x_clkout_prepare,
+	.unprepare = wm831x_clkout_unprepare,
+	.recalc_rate = wm831x_clkout_recalc_rate,
+	.round_rate = wm831x_clkout_round_rate,
+	.set_rate = wm831x_clkout_set_rate,
+	.get_parent = wm831x_clkout_get_parent,
+};
+
+static __devinit int wm831x_clk_probe(struct platform_device *pdev)
+{
+	struct wm831x *wm831x = dev_get_drvdata(pdev->dev.parent);
+	struct wm831x_clk *clkdata;
+	int ret;
+
+	clkdata = kzalloc(sizeof(*clkdata), GFP_KERNEL);
+	if (!clkdata)
+		return -ENOMEM;
+
+	/* XTAL_ENA can only be set via OTP/InstantConfig so just read once */
+	ret = wm831x_reg_read(wm831x, WM831X_CLOCK_CONTROL_2);
+	if (ret < 0) {
+		dev_err(wm831x->dev, "Unable to read CLOCK_CONTROL_2: %d\n",
+			ret);
+		goto err_alloc;
+	}
+	clkdata->xtal_ena = ret & WM831X_XTAL_ENA;
+
+	if (!clk_register(wm831x->dev, &wm831x_xtal_ops, &clkdata->xtal_hw,
+			  "xtal")) {
+		ret = -EINVAL;
+		goto err_alloc;
+	}
+
+	if (!clk_register(wm831x->dev, &wm831x_fll_ops, &clkdata->fll_hw,
+			  "fll")) {
+		ret = -EINVAL;
+		goto err_xtal;
+	}
+
+	if (!clk_register(wm831x->dev, &wm831x_clkout_ops, &clkdata->clkout_hw,
+			  "clkout")) {
+		ret = -EINVAL;
+		goto err_fll;
+	}
+
+	dev_set_drvdata(&pdev->dev, clkdata);
+
+	return 0;
+
+err_fll:
+	clk_unregister(clkdata->fll_hw.clk);
+err_xtal:
+	clk_unregister(clkdata->xtal_hw.clk);
+err_alloc:
+	kfree(clkdata);
+	return ret;
+}
+
+static __devexit int wm831x_clk_remove(struct platform_device *pdev)
+{
+	struct wm831x_clk *clkdata = dev_get_drvdata(&pdev->dev);
+
+	clk_unregister(clkdata->clkout_hw.clk);
+	clk_unregister(clkdata->fll_hw.clk);
+	clk_unregister(clkdata->xtal_hw.clk);
+	kfree(clkdata);
+
+	return 0;
+}
+
+static struct platform_driver wm831x_clk_driver = {
+	.probe = wm831x_clk_probe,
+	.remove = __devexit_p(wm831x_clk_remove),
+	.driver		= {
+		.name	= "wm831x-clk",
+		.owner	= THIS_MODULE,
+	},
+};
+
+static int __init wm831x_clk_init(void)
+{
+	int ret;
+
+	ret = platform_driver_register(&wm831x_clk_driver);
+	if (ret != 0)
+		pr_err("Failed to register WM831x clock driver: %d\n", ret);
+
+	return ret;
+}
+module_init(wm831x_clk_init);
+
+static void __exit wm831x_clk_exit(void)
+{
+	platform_driver_unregister(&wm831x_clk_driver);
+}
+module_exit(wm831x_clk_exit);
+
+/* Module information */
+MODULE_AUTHOR("Mark Brown <broonie@opensource.wolfsonmicro.com>");
+MODULE_DESCRIPTION("WM831x clock driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:wm831x-clk");
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 72+ messages in thread

* [PATCH 6/6] clk: Add initial WM831x clock driver
@ 2011-07-11  2:53     ` Mark Brown
  0 siblings, 0 replies; 72+ messages in thread
From: Mark Brown @ 2011-07-11  2:53 UTC (permalink / raw)
  To: linux-arm-kernel

The WM831x and WM832x series of PMICs contain a flexible clocking
subsystem intended to provide always on and system core clocks.  It
features:

- A 32.768kHz crystal oscillator which can optionally be used to pass
  through an externally generated clock.
- A FLL which can be clocked from either the 32.768kHz oscillator or
  the CLKIN pin.
- A CLKOUT pin which can bring out either the oscillator or the FLL
  output.
- The 32.768kHz clock can also optionally be brought out on the GPIO
  pins of the device.

This driver fully supports the 32.768kHz oscillator and CLKOUT.  The FLL
is supported only in AUTO mode, the full flexibility of the FLL cannot
currently be used.  The use of clock references other than the internal
oscillator is not currently supported, and since clk_set_parent() is not
implemented in the generic clock API the clock tree configuration cannot
be changed at runtime.

Due to a lack of access to systems where the core SoC has been converted
to use the generic clock API this driver has been compile tested only.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 MAINTAINERS              |    1 +
 drivers/clk/Kconfig      |    5 +
 drivers/clk/Makefile     |    1 +
 drivers/clk/clk-wm831x.c |  389 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 396 insertions(+), 0 deletions(-)
 create mode 100644 drivers/clk/clk-wm831x.c

diff --git a/MAINTAINERS b/MAINTAINERS
index ae563fa..c234756 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6969,6 +6969,7 @@ T:	git git://opensource.wolfsonmicro.com/linux-2.6-audioplus
 W:	http://opensource.wolfsonmicro.com/content/linux-drivers-wolfson-devices
 S:	Supported
 F:	Documentation/hwmon/wm83??
+F:	drivers/clk/clk-wm83*.c
 F:	drivers/leds/leds-wm83*.c
 F:	drivers/mfd/wm8*.c
 F:	drivers/power/wm83*.c
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 1fd0070..7f6eec2 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -10,6 +10,7 @@ config GENERIC_CLK_BUILD_TEST
 	depends on EXPERIMENTAL && GENERIC_CLK
 	select GENERIC_CLK_FIXED
 	select GENERIC_CLK_GATE
+	select GENERIC_CLK_WM831X if MFD_WM831X=y
 	help
 	   Enable all possible generic clock drivers.  This is only
 	   useful for improving build coverage, it is not useful for
@@ -22,3 +23,7 @@ config GENERIC_CLK_FIXED
 config GENERIC_CLK_GATE
 	bool
 	depends on GENERIC_CLK
+
+config GENERIC_CLK_WM831X
+	tristate
+	depends on GENERIC_CLK && MFD_WM831X
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index d186446..6628ad5 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -3,3 +3,4 @@ obj-$(CONFIG_CLKDEV_LOOKUP)	+= clkdev.o
 obj-$(CONFIG_GENERIC_CLK)	+= clk.o
 obj-$(CONFIG_GENERIC_CLK_FIXED)	+= clk-fixed.o
 obj-$(CONFIG_GENERIC_CLK_GATE)	+= clk-gate.o
+obj-$(CONFIG_GENERIC_CLK_WM831X) += clk-wm831x.o
diff --git a/drivers/clk/clk-wm831x.c b/drivers/clk/clk-wm831x.c
new file mode 100644
index 0000000..82782f1
--- /dev/null
+++ b/drivers/clk/clk-wm831x.c
@@ -0,0 +1,389 @@
+/*
+ * WM831x clock control
+ *
+ * Copyright 2011 Wolfson Microelectronics PLC.
+ *
+ * Author: Mark Brown <broonie@opensource.wolfsonmicro.com>
+ *
+ *  This program is free software; you can redistribute  it and/or modify it
+ *  under  the terms of  the GNU General  Public License as published by the
+ *  Free Software Foundation;  either version 2 of the  License, or (at your
+ *  option) any later version.
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+#include <linux/mfd/wm831x/core.h>
+
+struct wm831x_clk {
+	struct wm831x *wm831x;
+	struct clk_hw xtal_hw;
+	struct clk_hw fll_hw;
+	struct clk_hw clkout_hw;
+	bool xtal_ena;
+};
+
+static int wm831x_xtal_enable(struct clk_hw *hw)
+{
+	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
+						  xtal_hw);
+
+	if (clkdata->xtal_ena)
+		return 0;
+	else
+		return -EPERM;
+}
+
+static unsigned long wm831x_xtal_recalc_rate(struct clk_hw *hw)
+{
+	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
+						  xtal_hw);
+
+	if (clkdata->xtal_ena)
+		return 32768;
+	else
+		return 0;
+}
+
+static long wm831x_xtal_round_rate(struct clk_hw *hw, unsigned long rate)
+{
+	return wm831x_xtal_recalc_rate(hw);
+}
+
+static const struct clk_hw_ops wm831x_xtal_ops = {
+	.enable = wm831x_xtal_enable,
+	.recalc_rate = wm831x_xtal_recalc_rate,
+	.round_rate = wm831x_xtal_round_rate,
+};
+
+static const unsigned long wm831x_fll_auto_rates[] = {
+	 2048000,
+	11289600,
+	12000000,
+	12288000,
+	19200000,
+	22579600,
+	24000000,
+	24576000,
+};
+
+static bool wm831x_fll_enabled(struct wm831x *wm831x)
+{
+	int ret;
+
+	ret = wm831x_reg_read(wm831x, WM831X_FLL_CONTROL_1);
+	if (ret < 0) {
+		dev_err(wm831x->dev, "Unable to read FLL_CONTROL_1: %d\n",
+			ret);
+		return true;
+	}
+
+	if (ret & WM831X_FLL_ENA)
+		return true;
+	else
+		return false;
+}
+
+static int wm831x_fll_prepare(struct clk_hw *hw)
+{
+	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
+						  fll_hw);
+	struct wm831x *wm831x = clkdata->wm831x;
+	int ret;
+
+	ret = wm831x_set_bits(wm831x, WM831X_FLL_CONTROL_2,
+			      WM831X_FLL_ENA, WM831X_FLL_ENA);
+	if (ret != 0)
+		dev_crit(wm831x->dev, "Failed to enable FLL: %d\n", ret);
+
+	return ret;
+}
+
+static void wm831x_fll_unprepare(struct clk_hw *hw)
+{
+	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
+						  fll_hw);
+	struct wm831x *wm831x = clkdata->wm831x;
+	int ret;
+
+	ret = wm831x_set_bits(wm831x, WM831X_FLL_CONTROL_2, WM831X_FLL_ENA, 0);
+	if (ret != 0)
+		dev_crit(wm831x->dev, "Failed to disaable FLL: %d\n", ret);
+}
+
+static unsigned long wm831x_fll_recalc_rate(struct clk_hw *hw)
+{
+	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
+						  fll_hw);
+	struct wm831x *wm831x = clkdata->wm831x;
+	int ret;
+
+	ret = wm831x_reg_read(wm831x, WM831X_CLOCK_CONTROL_2);
+	if (ret < 0) {
+		dev_err(wm831x->dev, "Unable to read CLOCK_CONTROL_2: %d\n",
+			ret);
+		return 0;
+	}
+
+	if (ret & WM831X_FLL_AUTO)
+		return wm831x_fll_auto_rates[ret & WM831X_FLL_AUTO_FREQ_MASK];
+
+	dev_err(wm831x->dev, "FLL only supported in AUTO mode\n");
+	return 0;
+}
+
+static int wm831x_fll_set_rate(struct clk_hw *hw, unsigned long rate,
+			       unsigned long *parent_rate)
+{
+	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
+						  fll_hw);
+	struct wm831x *wm831x = clkdata->wm831x;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(wm831x_fll_auto_rates); i++)
+		if (wm831x_fll_auto_rates[i] == rate)
+			break;
+	if (i == ARRAY_SIZE(wm831x_fll_auto_rates))
+		return -EINVAL;
+
+	if (wm831x_fll_enabled(wm831x))
+		return -EPERM;
+
+	return wm831x_set_bits(wm831x, WM831X_CLOCK_CONTROL_2,
+			       WM831X_FLL_AUTO_FREQ_MASK, i);
+}
+
+static struct clk *wm831x_fll_get_parent(struct clk_hw *hw)
+{
+	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
+						  fll_hw);
+	struct wm831x *wm831x = clkdata->wm831x;
+	int ret;
+
+	/* AUTO mode is always clocked from the crystal */
+	ret = wm831x_reg_read(wm831x, WM831X_CLOCK_CONTROL_2);
+	if (ret < 0) {
+		dev_err(wm831x->dev, "Unable to read CLOCK_CONTROL_2: %d\n",
+			ret);
+		return NULL;
+	}
+
+	if (ret & WM831X_FLL_AUTO)
+		return clkdata->xtal_hw.clk;
+
+	ret = wm831x_reg_read(wm831x, WM831X_FLL_CONTROL_5);
+	if (ret < 0) {
+		dev_err(wm831x->dev, "Unable to read FLL_CONTROL_5: %d\n",
+			ret);
+		return NULL;
+	}
+
+	switch (ret & WM831X_FLL_CLK_SRC_MASK) {
+	case 0:
+		return clkdata->xtal_hw.clk;
+	case 1:
+		dev_warn(wm831x->dev,
+			 "FLL clocked from CLKIN not yet supported\n");
+		return NULL;
+	default:
+		dev_err(wm831x->dev, "Unsupported FLL clock source %d\n",
+			ret & WM831X_FLL_CLK_SRC_MASK);
+		return NULL;
+	}
+}
+
+static const struct clk_hw_ops wm831x_fll_ops = {
+	.prepare = wm831x_fll_prepare,
+	.unprepare = wm831x_fll_unprepare,
+	.recalc_rate = wm831x_fll_recalc_rate,
+	.set_rate = wm831x_fll_set_rate,
+	.get_parent = wm831x_fll_get_parent,
+};
+
+static int wm831x_clkout_prepare(struct clk_hw *hw)
+{
+	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
+						  clkout_hw);
+	struct wm831x *wm831x = clkdata->wm831x;
+	int ret;
+
+	ret = wm831x_reg_unlock(wm831x);
+	if (ret != 0) {
+		dev_crit(wm831x->dev, "Failed to lock registers: %d\n", ret);
+		return ret;
+	}
+
+	ret = wm831x_set_bits(wm831x, WM831X_CLOCK_CONTROL_1,
+			      WM831X_CLKOUT_ENA, WM831X_CLKOUT_ENA);
+	if (ret != 0)
+		dev_crit(wm831x->dev, "Failed to enable CLKOUT: %d\n", ret);
+
+	wm831x_reg_lock(wm831x);
+
+	return ret;
+}
+
+static void wm831x_clkout_unprepare(struct clk_hw *hw)
+{
+	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
+						  clkout_hw);
+	struct wm831x *wm831x = clkdata->wm831x;
+	int ret;
+
+	ret = wm831x_reg_unlock(wm831x);
+	if (ret != 0) {
+		dev_crit(wm831x->dev, "Failed to lock registers: %d\n", ret);
+		return;
+	}
+
+	ret = wm831x_set_bits(wm831x, WM831X_CLOCK_CONTROL_1,
+			      WM831X_CLKOUT_ENA, 0);
+	if (ret != 0)
+		dev_crit(wm831x->dev, "Failed to disable CLKOUT: %d\n", ret);
+
+	wm831x_reg_lock(wm831x);
+}
+
+static unsigned long wm831x_clkout_recalc_rate(struct clk_hw *hw)
+{
+	return clk_get_rate(clk_get_parent(hw->clk));
+}
+
+static long wm831x_clkout_round_rate(struct clk_hw *hw, unsigned long rate)
+{
+	return clk_round_rate(clk_get_parent(hw->clk), rate);
+}
+
+static int wm831x_clkout_set_rate(struct clk_hw *hw, unsigned long rate,
+				  unsigned long *parent_rate)
+{
+	*parent_rate = rate;
+	return CLK_SET_RATE_PROPAGATE;
+}
+
+static struct clk *wm831x_clkout_get_parent(struct clk_hw *hw)
+{
+	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
+						  clkout_hw);
+	struct wm831x *wm831x = clkdata->wm831x;
+	int ret;
+
+	ret = wm831x_reg_read(wm831x, WM831X_CLOCK_CONTROL_1);
+	if (ret < 0) {
+		dev_err(wm831x->dev, "Unable to read CLOCK_CONTROL_1: %d\n",
+			ret);
+		return NULL;
+	}
+
+	if (ret & WM831X_CLKOUT_SRC)
+		return clkdata->xtal_hw.clk;
+	else
+		return clkdata->fll_hw.clk;
+}
+
+static const struct clk_hw_ops wm831x_clkout_ops = {
+	.prepare = wm831x_clkout_prepare,
+	.unprepare = wm831x_clkout_unprepare,
+	.recalc_rate = wm831x_clkout_recalc_rate,
+	.round_rate = wm831x_clkout_round_rate,
+	.set_rate = wm831x_clkout_set_rate,
+	.get_parent = wm831x_clkout_get_parent,
+};
+
+static __devinit int wm831x_clk_probe(struct platform_device *pdev)
+{
+	struct wm831x *wm831x = dev_get_drvdata(pdev->dev.parent);
+	struct wm831x_clk *clkdata;
+	int ret;
+
+	clkdata = kzalloc(sizeof(*clkdata), GFP_KERNEL);
+	if (!clkdata)
+		return -ENOMEM;
+
+	/* XTAL_ENA can only be set via OTP/InstantConfig so just read once */
+	ret = wm831x_reg_read(wm831x, WM831X_CLOCK_CONTROL_2);
+	if (ret < 0) {
+		dev_err(wm831x->dev, "Unable to read CLOCK_CONTROL_2: %d\n",
+			ret);
+		goto err_alloc;
+	}
+	clkdata->xtal_ena = ret & WM831X_XTAL_ENA;
+
+	if (!clk_register(wm831x->dev, &wm831x_xtal_ops, &clkdata->xtal_hw,
+			  "xtal")) {
+		ret = -EINVAL;
+		goto err_alloc;
+	}
+
+	if (!clk_register(wm831x->dev, &wm831x_fll_ops, &clkdata->fll_hw,
+			  "fll")) {
+		ret = -EINVAL;
+		goto err_xtal;
+	}
+
+	if (!clk_register(wm831x->dev, &wm831x_clkout_ops, &clkdata->clkout_hw,
+			  "clkout")) {
+		ret = -EINVAL;
+		goto err_fll;
+	}
+
+	dev_set_drvdata(&pdev->dev, clkdata);
+
+	return 0;
+
+err_fll:
+	clk_unregister(clkdata->fll_hw.clk);
+err_xtal:
+	clk_unregister(clkdata->xtal_hw.clk);
+err_alloc:
+	kfree(clkdata);
+	return ret;
+}
+
+static __devexit int wm831x_clk_remove(struct platform_device *pdev)
+{
+	struct wm831x_clk *clkdata = dev_get_drvdata(&pdev->dev);
+
+	clk_unregister(clkdata->clkout_hw.clk);
+	clk_unregister(clkdata->fll_hw.clk);
+	clk_unregister(clkdata->xtal_hw.clk);
+	kfree(clkdata);
+
+	return 0;
+}
+
+static struct platform_driver wm831x_clk_driver = {
+	.probe = wm831x_clk_probe,
+	.remove = __devexit_p(wm831x_clk_remove),
+	.driver		= {
+		.name	= "wm831x-clk",
+		.owner	= THIS_MODULE,
+	},
+};
+
+static int __init wm831x_clk_init(void)
+{
+	int ret;
+
+	ret = platform_driver_register(&wm831x_clk_driver);
+	if (ret != 0)
+		pr_err("Failed to register WM831x clock driver: %d\n", ret);
+
+	return ret;
+}
+module_init(wm831x_clk_init);
+
+static void __exit wm831x_clk_exit(void)
+{
+	platform_driver_unregister(&wm831x_clk_driver);
+}
+module_exit(wm831x_clk_exit);
+
+/* Module information */
+MODULE_AUTHOR("Mark Brown <broonie@opensource.wolfsonmicro.com>");
+MODULE_DESCRIPTION("WM831x clock driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:wm831x-clk");
-- 
1.7.5.4

^ permalink raw reply related	[flat|nested] 72+ messages in thread

* Re: [PATCH 0/6] clk: Initial feedback for off-SoC slow bus clocks
  2011-07-11  2:53 ` Mark Brown
  (?)
@ 2011-07-11  3:57   ` Mark Brown
  -1 siblings, 0 replies; 72+ messages in thread
From: Mark Brown @ 2011-07-11  3:57 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Jul 11, 2011 at 11:53:44AM +0900, Mark Brown wrote:

Linus, CCing you in as apparently you're taking over the clock API work.
Do you need me to forward all the patches to you?

> I've just been having a go at implementing generic clk API support for
> an off-SoC device on a slow bus, the clocking module in the wm831x/2x
> series of PMICs.  Unfortunately as I don't currently have access to a
> platform that has been converted to use the API I've not actually been
> able to test the code but I'm reasonably optimistic that the code will
> Just Work(tm) as the API seems fairly straightforward.
> 
> The biggest issue I ran into was that as the clocks are all registered
> by name with the API if you've got two instances of the same off-SoC
> device in the system you'll not be able to disambiguate between the
> clocks it provides.  I've added a simple solution for this in the form
> of a patch which takes a struct device as an argument and adds that to
> the name of the registered clock.  This isn't ideal but should give
> stable names which systems can use together with clkdev to match clocks
> up with their users.  I believe that when we have device tree bindings
> for clocks we should be able to use the device to access the bindings
> and avoid this mangling - Grant, is that correct?
> 
> Otherwise everything seems to work pretty well from the driver side for
> these devices, I've added some minor updates which came up naturally
> while writing the driver code and seemed fairly obvious.  One of these
> was a change to provide a user visible Kconfig option for building the
> clock drivers, the idea being to make it easier to do build tests.  I
> did also wonder if it's worth adding a patch to enable the API on x86
> (which doesn't currently have a clock API) for coverage.
> 
> These patches (which include the two I posted yesterday) are against the
> series you posted in May with the exception of the wm831x patch which
> also has an additional dependency on "mfd: Add WM831x clock control
> register definitions" which is due for merge in the next merge window.
> If this stuff is all OK for you it would be good if you could include
> the wm831x driver in your tree for this, especially given that there's
> nothing in -next.
> 
> Mark Brown (6):
>       clk: Prototype and document clk_register()
>       clk: Provide a dummy clk_unregister()
>       clk: Constify struct clk_hw_ops
>       clk: Add Kconfig option to build all generic clk drivers
>       clk: Support multiple instances of the same clock provider
>       clk: Add initial WM831x clock driver
> 
>  MAINTAINERS              |    1 +
>  drivers/clk/Kconfig      |   16 ++-
>  drivers/clk/Makefile     |    1 +
>  drivers/clk/clk-wm831x.c |  389 ++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/clk.c        |   38 ++++-
>  include/linux/clk.h      |   33 ++++
>  6 files changed, 472 insertions(+), 6 deletions(-)
>  create mode 100644 drivers/clk/clk-wm831x.c

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH 0/6] clk: Initial feedback for off-SoC slow bus clocks
@ 2011-07-11  3:57   ` Mark Brown
  0 siblings, 0 replies; 72+ messages in thread
From: Mark Brown @ 2011-07-11  3:57 UTC (permalink / raw)
  To: Jeremy Kerr, Linus Walleij
  Cc: linux-kernel, linux-arm-kernel, linux-sh, patches, Grant Likely

On Mon, Jul 11, 2011 at 11:53:44AM +0900, Mark Brown wrote:

Linus, CCing you in as apparently you're taking over the clock API work.
Do you need me to forward all the patches to you?

> I've just been having a go at implementing generic clk API support for
> an off-SoC device on a slow bus, the clocking module in the wm831x/2x
> series of PMICs.  Unfortunately as I don't currently have access to a
> platform that has been converted to use the API I've not actually been
> able to test the code but I'm reasonably optimistic that the code will
> Just Work(tm) as the API seems fairly straightforward.
> 
> The biggest issue I ran into was that as the clocks are all registered
> by name with the API if you've got two instances of the same off-SoC
> device in the system you'll not be able to disambiguate between the
> clocks it provides.  I've added a simple solution for this in the form
> of a patch which takes a struct device as an argument and adds that to
> the name of the registered clock.  This isn't ideal but should give
> stable names which systems can use together with clkdev to match clocks
> up with their users.  I believe that when we have device tree bindings
> for clocks we should be able to use the device to access the bindings
> and avoid this mangling - Grant, is that correct?
> 
> Otherwise everything seems to work pretty well from the driver side for
> these devices, I've added some minor updates which came up naturally
> while writing the driver code and seemed fairly obvious.  One of these
> was a change to provide a user visible Kconfig option for building the
> clock drivers, the idea being to make it easier to do build tests.  I
> did also wonder if it's worth adding a patch to enable the API on x86
> (which doesn't currently have a clock API) for coverage.
> 
> These patches (which include the two I posted yesterday) are against the
> series you posted in May with the exception of the wm831x patch which
> also has an additional dependency on "mfd: Add WM831x clock control
> register definitions" which is due for merge in the next merge window.
> If this stuff is all OK for you it would be good if you could include
> the wm831x driver in your tree for this, especially given that there's
> nothing in -next.
> 
> Mark Brown (6):
>       clk: Prototype and document clk_register()
>       clk: Provide a dummy clk_unregister()
>       clk: Constify struct clk_hw_ops
>       clk: Add Kconfig option to build all generic clk drivers
>       clk: Support multiple instances of the same clock provider
>       clk: Add initial WM831x clock driver
> 
>  MAINTAINERS              |    1 +
>  drivers/clk/Kconfig      |   16 ++-
>  drivers/clk/Makefile     |    1 +
>  drivers/clk/clk-wm831x.c |  389 ++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/clk.c        |   38 ++++-
>  include/linux/clk.h      |   33 ++++
>  6 files changed, 472 insertions(+), 6 deletions(-)
>  create mode 100644 drivers/clk/clk-wm831x.c

^ permalink raw reply	[flat|nested] 72+ messages in thread

* [PATCH 0/6] clk: Initial feedback for off-SoC slow bus clocks
@ 2011-07-11  3:57   ` Mark Brown
  0 siblings, 0 replies; 72+ messages in thread
From: Mark Brown @ 2011-07-11  3:57 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Jul 11, 2011 at 11:53:44AM +0900, Mark Brown wrote:

Linus, CCing you in as apparently you're taking over the clock API work.
Do you need me to forward all the patches to you?

> I've just been having a go at implementing generic clk API support for
> an off-SoC device on a slow bus, the clocking module in the wm831x/2x
> series of PMICs.  Unfortunately as I don't currently have access to a
> platform that has been converted to use the API I've not actually been
> able to test the code but I'm reasonably optimistic that the code will
> Just Work(tm) as the API seems fairly straightforward.
> 
> The biggest issue I ran into was that as the clocks are all registered
> by name with the API if you've got two instances of the same off-SoC
> device in the system you'll not be able to disambiguate between the
> clocks it provides.  I've added a simple solution for this in the form
> of a patch which takes a struct device as an argument and adds that to
> the name of the registered clock.  This isn't ideal but should give
> stable names which systems can use together with clkdev to match clocks
> up with their users.  I believe that when we have device tree bindings
> for clocks we should be able to use the device to access the bindings
> and avoid this mangling - Grant, is that correct?
> 
> Otherwise everything seems to work pretty well from the driver side for
> these devices, I've added some minor updates which came up naturally
> while writing the driver code and seemed fairly obvious.  One of these
> was a change to provide a user visible Kconfig option for building the
> clock drivers, the idea being to make it easier to do build tests.  I
> did also wonder if it's worth adding a patch to enable the API on x86
> (which doesn't currently have a clock API) for coverage.
> 
> These patches (which include the two I posted yesterday) are against the
> series you posted in May with the exception of the wm831x patch which
> also has an additional dependency on "mfd: Add WM831x clock control
> register definitions" which is due for merge in the next merge window.
> If this stuff is all OK for you it would be good if you could include
> the wm831x driver in your tree for this, especially given that there's
> nothing in -next.
> 
> Mark Brown (6):
>       clk: Prototype and document clk_register()
>       clk: Provide a dummy clk_unregister()
>       clk: Constify struct clk_hw_ops
>       clk: Add Kconfig option to build all generic clk drivers
>       clk: Support multiple instances of the same clock provider
>       clk: Add initial WM831x clock driver
> 
>  MAINTAINERS              |    1 +
>  drivers/clk/Kconfig      |   16 ++-
>  drivers/clk/Makefile     |    1 +
>  drivers/clk/clk-wm831x.c |  389 ++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/clk.c        |   38 ++++-
>  include/linux/clk.h      |   33 ++++
>  6 files changed, 472 insertions(+), 6 deletions(-)
>  create mode 100644 drivers/clk/clk-wm831x.c

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH 0/6] clk: Initial feedback for off-SoC slow bus clocks
  2011-07-11  3:57   ` Mark Brown
  (?)
@ 2011-07-11  4:30     ` Mike Frysinger
  -1 siblings, 0 replies; 72+ messages in thread
From: Mike Frysinger @ 2011-07-11  4:30 UTC (permalink / raw)
  To: linux-arm-kernel

[-- Attachment #1: Type: Text/Plain, Size: 569 bytes --]

On Sunday, July 10, 2011 23:57:40 Mark Brown wrote:
> On Mon, Jul 11, 2011 at 11:53:44AM +0900, Mark Brown wrote:
> 
> Linus, CCing you in as apparently you're taking over the clock API work.
> Do you need me to forward all the patches to you?

along these lines, i dont think the new people on the cc noticed my earlier e-
mail:
for future series, could you cc uclinux-dist-devel@blackfin.uclinux.org ?  we 
dont do clock management on Blackfin parts atm, but it's something we would 
like to start doing.  our hardware can easily benefit from this.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH 0/6] clk: Initial feedback for off-SoC slow bus clocks
@ 2011-07-11  4:30     ` Mike Frysinger
  0 siblings, 0 replies; 72+ messages in thread
From: Mike Frysinger @ 2011-07-11  4:30 UTC (permalink / raw)
  To: Mark Brown
  Cc: Jeremy Kerr, Linus Walleij, linux-kernel, linux-arm-kernel,
	linux-sh, patches, Grant Likely

[-- Attachment #1: Type: Text/Plain, Size: 569 bytes --]

On Sunday, July 10, 2011 23:57:40 Mark Brown wrote:
> On Mon, Jul 11, 2011 at 11:53:44AM +0900, Mark Brown wrote:
> 
> Linus, CCing you in as apparently you're taking over the clock API work.
> Do you need me to forward all the patches to you?

along these lines, i dont think the new people on the cc noticed my earlier e-
mail:
for future series, could you cc uclinux-dist-devel@blackfin.uclinux.org ?  we 
dont do clock management on Blackfin parts atm, but it's something we would 
like to start doing.  our hardware can easily benefit from this.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 72+ messages in thread

* [PATCH 0/6] clk: Initial feedback for off-SoC slow bus clocks
@ 2011-07-11  4:30     ` Mike Frysinger
  0 siblings, 0 replies; 72+ messages in thread
From: Mike Frysinger @ 2011-07-11  4:30 UTC (permalink / raw)
  To: linux-arm-kernel

On Sunday, July 10, 2011 23:57:40 Mark Brown wrote:
> On Mon, Jul 11, 2011 at 11:53:44AM +0900, Mark Brown wrote:
> 
> Linus, CCing you in as apparently you're taking over the clock API work.
> Do you need me to forward all the patches to you?

along these lines, i dont think the new people on the cc noticed my earlier e-
mail:
for future series, could you cc uclinux-dist-devel at blackfin.uclinux.org ?  we 
dont do clock management on Blackfin parts atm, but it's something we would 
like to start doing.  our hardware can easily benefit from this.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110711/69b913bf/attachment.sig>

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH 0/6] clk: Initial feedback for off-SoC slow bus clocks
  2011-07-11  4:30     ` Mike Frysinger
  (?)
@ 2011-07-11  4:56       ` Barry Song
  -1 siblings, 0 replies; 72+ messages in thread
From: Barry Song @ 2011-07-11  4:56 UTC (permalink / raw)
  To: linux-arm-kernel

2011/7/11 Mike Frysinger <vapier@gentoo.org>:
> On Sunday, July 10, 2011 23:57:40 Mark Brown wrote:
>> On Mon, Jul 11, 2011 at 11:53:44AM +0900, Mark Brown wrote:
>>
>> Linus, CCing you in as apparently you're taking over the clock API work.
>> Do you need me to forward all the patches to you?
>
> along these lines, i dont think the new people on the cc noticed my earlier e-
> mail:
> for future series, could you cc uclinux-dist-devel@blackfin.uclinux.org ?  we
> dont do clock management on Blackfin parts atm, but it's something we would
> like to start doing.  our hardware can easily benefit from this.

Except that, AFAIK, arch/blackfin/mach-xxx is much similar with
arch/arm/mach-xxx with a lot of platform, i2c, spi board information.
it is probably we can also benefit from device tree as what ARM is
doing.
but not sure whether it is really worthwhile for the current situation
and long term plan of blackfin. Anyway, there are not so many blackfin
SoC and boards as ARM.

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

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH 0/6] clk: Initial feedback for off-SoC slow bus clocks
@ 2011-07-11  4:56       ` Barry Song
  0 siblings, 0 replies; 72+ messages in thread
From: Barry Song @ 2011-07-11  4:56 UTC (permalink / raw)
  To: Mike Frysinger
  Cc: Mark Brown, Grant Likely, Linus Walleij, linux-sh, patches,
	linux-kernel, Jeremy Kerr, linux-arm-kernel, uclinux-dist-devel

2011/7/11 Mike Frysinger <vapier@gentoo.org>:
> On Sunday, July 10, 2011 23:57:40 Mark Brown wrote:
>> On Mon, Jul 11, 2011 at 11:53:44AM +0900, Mark Brown wrote:
>>
>> Linus, CCing you in as apparently you're taking over the clock API work.
>> Do you need me to forward all the patches to you?
>
> along these lines, i dont think the new people on the cc noticed my earlier e-
> mail:
> for future series, could you cc uclinux-dist-devel@blackfin.uclinux.org ?  we
> dont do clock management on Blackfin parts atm, but it's something we would
> like to start doing.  our hardware can easily benefit from this.

Except that, AFAIK, arch/blackfin/mach-xxx is much similar with
arch/arm/mach-xxx with a lot of platform, i2c, spi board information.
it is probably we can also benefit from device tree as what ARM is
doing.
but not sure whether it is really worthwhile for the current situation
and long term plan of blackfin. Anyway, there are not so many blackfin
SoC and boards as ARM.

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

^ permalink raw reply	[flat|nested] 72+ messages in thread

* [PATCH 0/6] clk: Initial feedback for off-SoC slow bus clocks
@ 2011-07-11  4:56       ` Barry Song
  0 siblings, 0 replies; 72+ messages in thread
From: Barry Song @ 2011-07-11  4:56 UTC (permalink / raw)
  To: linux-arm-kernel

2011/7/11 Mike Frysinger <vapier@gentoo.org>:
> On Sunday, July 10, 2011 23:57:40 Mark Brown wrote:
>> On Mon, Jul 11, 2011 at 11:53:44AM +0900, Mark Brown wrote:
>>
>> Linus, CCing you in as apparently you're taking over the clock API work.
>> Do you need me to forward all the patches to you?
>
> along these lines, i dont think the new people on the cc noticed my earlier e-
> mail:
> for future series, could you cc uclinux-dist-devel at blackfin.uclinux.org ? ?we
> dont do clock management on Blackfin parts atm, but it's something we would
> like to start doing. ?our hardware can easily benefit from this.

Except that, AFAIK, arch/blackfin/mach-xxx is much similar with
arch/arm/mach-xxx with a lot of platform, i2c, spi board information.
it is probably we can also benefit from device tree as what ARM is
doing.
but not sure whether it is really worthwhile for the current situation
and long term plan of blackfin. Anyway, there are not so many blackfin
SoC and boards as ARM.

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

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [uclinux-dist-devel] [PATCH 0/6] clk: Initial feedback for
  2011-07-11  4:56       ` Barry Song
  (?)
@ 2011-07-11  5:01         ` Mike Frysinger
  -1 siblings, 0 replies; 72+ messages in thread
From: Mike Frysinger @ 2011-07-11  5:01 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Jul 11, 2011 at 00:56, Barry Song wrote:
> 2011/7/11 Mike Frysinger:
>> On Sunday, July 10, 2011 23:57:40 Mark Brown wrote:
>>> On Mon, Jul 11, 2011 at 11:53:44AM +0900, Mark Brown wrote:
>>> Linus, CCing you in as apparently you're taking over the clock API work.
>>> Do you need me to forward all the patches to you?
>>
>> along these lines, i dont think the new people on the cc noticed my earlier e-
>> mail:
>> for future series, could you cc uclinux-dist-devel@blackfin.uclinux.org ?  we
>> dont do clock management on Blackfin parts atm, but it's something we would
>> like to start doing.  our hardware can easily benefit from this.
>
> Except that, AFAIK, arch/blackfin/mach-xxx is much similar with
> arch/arm/mach-xxx with a lot of platform, i2c, spi board information.
> it is probably we can also benefit from device tree as what ARM is
> doing.

i dont think device trees are a requirement for the clock api

> but not sure whether it is really worthwhile for the current situation
> and long term plan of blackfin. Anyway, there are not so many blackfin
> SoC and boards as ARM.

there are not plans for device tree support.  no customers have asked
for it, and we arent in the arm situation where we have a distro (like
Ubuntu) riding us to have a single build boot on all the different
platforms.
-mike

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [uclinux-dist-devel] [PATCH 0/6] clk: Initial feedback for off-SoC slow bus clocks
@ 2011-07-11  5:01         ` Mike Frysinger
  0 siblings, 0 replies; 72+ messages in thread
From: Mike Frysinger @ 2011-07-11  5:01 UTC (permalink / raw)
  To: Barry Song
  Cc: Grant Likely, Mark Brown, Linus Walleij, linux-sh, patches,
	linux-kernel, uclinux-dist-devel, Jeremy Kerr, linux-arm-kernel

On Mon, Jul 11, 2011 at 00:56, Barry Song wrote:
> 2011/7/11 Mike Frysinger:
>> On Sunday, July 10, 2011 23:57:40 Mark Brown wrote:
>>> On Mon, Jul 11, 2011 at 11:53:44AM +0900, Mark Brown wrote:
>>> Linus, CCing you in as apparently you're taking over the clock API work.
>>> Do you need me to forward all the patches to you?
>>
>> along these lines, i dont think the new people on the cc noticed my earlier e-
>> mail:
>> for future series, could you cc uclinux-dist-devel@blackfin.uclinux.org ?  we
>> dont do clock management on Blackfin parts atm, but it's something we would
>> like to start doing.  our hardware can easily benefit from this.
>
> Except that, AFAIK, arch/blackfin/mach-xxx is much similar with
> arch/arm/mach-xxx with a lot of platform, i2c, spi board information.
> it is probably we can also benefit from device tree as what ARM is
> doing.

i dont think device trees are a requirement for the clock api

> but not sure whether it is really worthwhile for the current situation
> and long term plan of blackfin. Anyway, there are not so many blackfin
> SoC and boards as ARM.

there are not plans for device tree support.  no customers have asked
for it, and we arent in the arm situation where we have a distro (like
Ubuntu) riding us to have a single build boot on all the different
platforms.
-mike

^ permalink raw reply	[flat|nested] 72+ messages in thread

* [uclinux-dist-devel] [PATCH 0/6] clk: Initial feedback for off-SoC slow bus clocks
@ 2011-07-11  5:01         ` Mike Frysinger
  0 siblings, 0 replies; 72+ messages in thread
From: Mike Frysinger @ 2011-07-11  5:01 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Jul 11, 2011 at 00:56, Barry Song wrote:
> 2011/7/11 Mike Frysinger:
>> On Sunday, July 10, 2011 23:57:40 Mark Brown wrote:
>>> On Mon, Jul 11, 2011 at 11:53:44AM +0900, Mark Brown wrote:
>>> Linus, CCing you in as apparently you're taking over the clock API work.
>>> Do you need me to forward all the patches to you?
>>
>> along these lines, i dont think the new people on the cc noticed my earlier e-
>> mail:
>> for future series, could you cc uclinux-dist-devel at blackfin.uclinux.org ? ?we
>> dont do clock management on Blackfin parts atm, but it's something we would
>> like to start doing. ?our hardware can easily benefit from this.
>
> Except that, AFAIK, arch/blackfin/mach-xxx is much similar with
> arch/arm/mach-xxx with a lot of platform, i2c, spi board information.
> it is probably we can also benefit from device tree as what ARM is
> doing.

i dont think device trees are a requirement for the clock api

> but not sure whether it is really worthwhile for the current situation
> and long term plan of blackfin. Anyway, there are not so many blackfin
> SoC and boards as ARM.

there are not plans for device tree support.  no customers have asked
for it, and we arent in the arm situation where we have a distro (like
Ubuntu) riding us to have a single build boot on all the different
platforms.
-mike

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH 0/6] clk: Initial feedback for off-SoC slow bus clocks
  2011-07-11  2:53 ` Mark Brown
  (?)
@ 2011-07-11  9:31   ` Russell King - ARM Linux
  -1 siblings, 0 replies; 72+ messages in thread
From: Russell King - ARM Linux @ 2011-07-11  9:31 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Jul 11, 2011 at 11:53:44AM +0900, Mark Brown wrote:
> The biggest issue I ran into was that as the clocks are all registered
> by name with the API if you've got two instances of the same off-SoC
> device in the system you'll not be able to disambiguate between the
> clocks it provides.

Sigh.  That sounds like yet more trash.  Obviously whoever thought
up that doesn't actually understand clks.

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH 0/6] clk: Initial feedback for off-SoC slow bus clocks
@ 2011-07-11  9:31   ` Russell King - ARM Linux
  0 siblings, 0 replies; 72+ messages in thread
From: Russell King - ARM Linux @ 2011-07-11  9:31 UTC (permalink / raw)
  To: Mark Brown
  Cc: Jeremy Kerr, Grant Likely, patches, linux-kernel,
	linux-arm-kernel, linux-sh

On Mon, Jul 11, 2011 at 11:53:44AM +0900, Mark Brown wrote:
> The biggest issue I ran into was that as the clocks are all registered
> by name with the API if you've got two instances of the same off-SoC
> device in the system you'll not be able to disambiguate between the
> clocks it provides.

Sigh.  That sounds like yet more trash.  Obviously whoever thought
up that doesn't actually understand clks.

^ permalink raw reply	[flat|nested] 72+ messages in thread

* [PATCH 0/6] clk: Initial feedback for off-SoC slow bus clocks
@ 2011-07-11  9:31   ` Russell King - ARM Linux
  0 siblings, 0 replies; 72+ messages in thread
From: Russell King - ARM Linux @ 2011-07-11  9:31 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Jul 11, 2011 at 11:53:44AM +0900, Mark Brown wrote:
> The biggest issue I ran into was that as the clocks are all registered
> by name with the API if you've got two instances of the same off-SoC
> device in the system you'll not be able to disambiguate between the
> clocks it provides.

Sigh.  That sounds like yet more trash.  Obviously whoever thought
up that doesn't actually understand clks.

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH 5/6] clk: Support multiple instances of the same clock
  2011-07-11  2:53     ` Mark Brown
  (?)
@ 2011-07-11  9:34       ` Russell King - ARM Linux
  -1 siblings, 0 replies; 72+ messages in thread
From: Russell King - ARM Linux @ 2011-07-11  9:34 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Jul 11, 2011 at 11:53:56AM +0900, Mark Brown wrote:
> Currently the generic clk API identifies clocks internally using the name
> of the clock. This is OK for on-SoC clocks where we have enough control to
> disambiguate but doesn't work well for clocks provided on external chips
> where a system design may include more than one instance of the same chip
> (the Wolfson Speyside system is an example of this) or may have namespace
> collisions.
> 
> Address this by allowing the clock provider to supply a struct device for
> the clock for use in disambiguation. As a first pass if it is provided we
> prefix the clock name with the dev_name() of the device. With a device
> tree binding for clocks it should be possible to remove this mangling and
> instead use the struct device to provide access to the binding information.
> 
> In order to avoid needless noise in names and memory usage it is strongly
> recommended that on-SoC clocks do not provide a struct device until the
> implementation is improved.
> 
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> ---
>  drivers/clk/clk.c   |   36 ++++++++++++++++++++++++++++++++----
>  include/linux/clk.h |   14 ++++++++++----
>  2 files changed, 42 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 1df6e23..f36f637 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -13,6 +13,7 @@
>  #include <linux/mutex.h>
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
> +#include <linux/device.h>
>  
>  struct clk {
>  	const char		*name;
> @@ -252,20 +253,44 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
>  }
>  EXPORT_SYMBOL_GPL(clk_set_parent);
>  
> -struct clk *clk_register(const struct clk_hw_ops *ops, struct clk_hw *hw,
> -		const char *name)
> +struct clk *clk_register(struct device *dev, const struct clk_hw_ops *ops,
> +			 struct clk_hw *hw, const char *name)
>  {
>  	struct clk *clk;
> +	char *new_name;
> +	size_t name_len;
>  
>  	clk = kzalloc(sizeof(*clk), GFP_KERNEL);
>  	if (!clk)
>  		return NULL;
>  
> -	clk->name = name;
>  	clk->ops = ops;
>  	clk->hw = hw;
>  	hw->clk = clk;
>  
> +	/* Since we currently match clock providers on a purely string
> +	 * based method add a prefix based on the device name if a
> +	 * device is provided.  When we have support for device tree
> +	 * based clock matching it should be possible to avoid this
> +	 * mangling and instead use the struct device to hook into
> +	 * the bindings.
> +	 *
> +	 * As we don't currently support unregistering clocks we don't
> +	 * need to worry about cleanup as yet.
> +	 */
> +	if (dev) {
> +		name_len = strlen(name) + strlen(dev_name(dev)) + 2;
> +		new_name = kzalloc(name_len, GFP_KERNEL);
> +		if (!new_name)
> +			goto err;
> +
> +		snprintf(new_name, name_len, "%s-%s", dev_name(dev), name);
> +
> +		clk->name = new_name;
> +	} else {
> +		clk->name = name;
> +	}

This "clk consolidation" is really idiotic.  The clk matching mechanism
should have _nothing_ to do with the rest of the clk API, especially the
consolidation effort.

It should not matter whether clkdev is used, or an alternative method
to specify this stuff via DT.  Keep the clk_get()/clk_put() _separate_
from the consolidation of the rest.

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH 5/6] clk: Support multiple instances of the same clock provider
@ 2011-07-11  9:34       ` Russell King - ARM Linux
  0 siblings, 0 replies; 72+ messages in thread
From: Russell King - ARM Linux @ 2011-07-11  9:34 UTC (permalink / raw)
  To: Mark Brown
  Cc: Jeremy Kerr, Grant Likely, linux-sh, patches, linux-kernel,
	linux-arm-kernel

On Mon, Jul 11, 2011 at 11:53:56AM +0900, Mark Brown wrote:
> Currently the generic clk API identifies clocks internally using the name
> of the clock. This is OK for on-SoC clocks where we have enough control to
> disambiguate but doesn't work well for clocks provided on external chips
> where a system design may include more than one instance of the same chip
> (the Wolfson Speyside system is an example of this) or may have namespace
> collisions.
> 
> Address this by allowing the clock provider to supply a struct device for
> the clock for use in disambiguation. As a first pass if it is provided we
> prefix the clock name with the dev_name() of the device. With a device
> tree binding for clocks it should be possible to remove this mangling and
> instead use the struct device to provide access to the binding information.
> 
> In order to avoid needless noise in names and memory usage it is strongly
> recommended that on-SoC clocks do not provide a struct device until the
> implementation is improved.
> 
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> ---
>  drivers/clk/clk.c   |   36 ++++++++++++++++++++++++++++++++----
>  include/linux/clk.h |   14 ++++++++++----
>  2 files changed, 42 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 1df6e23..f36f637 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -13,6 +13,7 @@
>  #include <linux/mutex.h>
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
> +#include <linux/device.h>
>  
>  struct clk {
>  	const char		*name;
> @@ -252,20 +253,44 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
>  }
>  EXPORT_SYMBOL_GPL(clk_set_parent);
>  
> -struct clk *clk_register(const struct clk_hw_ops *ops, struct clk_hw *hw,
> -		const char *name)
> +struct clk *clk_register(struct device *dev, const struct clk_hw_ops *ops,
> +			 struct clk_hw *hw, const char *name)
>  {
>  	struct clk *clk;
> +	char *new_name;
> +	size_t name_len;
>  
>  	clk = kzalloc(sizeof(*clk), GFP_KERNEL);
>  	if (!clk)
>  		return NULL;
>  
> -	clk->name = name;
>  	clk->ops = ops;
>  	clk->hw = hw;
>  	hw->clk = clk;
>  
> +	/* Since we currently match clock providers on a purely string
> +	 * based method add a prefix based on the device name if a
> +	 * device is provided.  When we have support for device tree
> +	 * based clock matching it should be possible to avoid this
> +	 * mangling and instead use the struct device to hook into
> +	 * the bindings.
> +	 *
> +	 * As we don't currently support unregistering clocks we don't
> +	 * need to worry about cleanup as yet.
> +	 */
> +	if (dev) {
> +		name_len = strlen(name) + strlen(dev_name(dev)) + 2;
> +		new_name = kzalloc(name_len, GFP_KERNEL);
> +		if (!new_name)
> +			goto err;
> +
> +		snprintf(new_name, name_len, "%s-%s", dev_name(dev), name);
> +
> +		clk->name = new_name;
> +	} else {
> +		clk->name = name;
> +	}

This "clk consolidation" is really idiotic.  The clk matching mechanism
should have _nothing_ to do with the rest of the clk API, especially the
consolidation effort.

It should not matter whether clkdev is used, or an alternative method
to specify this stuff via DT.  Keep the clk_get()/clk_put() _separate_
from the consolidation of the rest.

^ permalink raw reply	[flat|nested] 72+ messages in thread

* [PATCH 5/6] clk: Support multiple instances of the same clock provider
@ 2011-07-11  9:34       ` Russell King - ARM Linux
  0 siblings, 0 replies; 72+ messages in thread
From: Russell King - ARM Linux @ 2011-07-11  9:34 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Jul 11, 2011 at 11:53:56AM +0900, Mark Brown wrote:
> Currently the generic clk API identifies clocks internally using the name
> of the clock. This is OK for on-SoC clocks where we have enough control to
> disambiguate but doesn't work well for clocks provided on external chips
> where a system design may include more than one instance of the same chip
> (the Wolfson Speyside system is an example of this) or may have namespace
> collisions.
> 
> Address this by allowing the clock provider to supply a struct device for
> the clock for use in disambiguation. As a first pass if it is provided we
> prefix the clock name with the dev_name() of the device. With a device
> tree binding for clocks it should be possible to remove this mangling and
> instead use the struct device to provide access to the binding information.
> 
> In order to avoid needless noise in names and memory usage it is strongly
> recommended that on-SoC clocks do not provide a struct device until the
> implementation is improved.
> 
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> ---
>  drivers/clk/clk.c   |   36 ++++++++++++++++++++++++++++++++----
>  include/linux/clk.h |   14 ++++++++++----
>  2 files changed, 42 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 1df6e23..f36f637 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -13,6 +13,7 @@
>  #include <linux/mutex.h>
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
> +#include <linux/device.h>
>  
>  struct clk {
>  	const char		*name;
> @@ -252,20 +253,44 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
>  }
>  EXPORT_SYMBOL_GPL(clk_set_parent);
>  
> -struct clk *clk_register(const struct clk_hw_ops *ops, struct clk_hw *hw,
> -		const char *name)
> +struct clk *clk_register(struct device *dev, const struct clk_hw_ops *ops,
> +			 struct clk_hw *hw, const char *name)
>  {
>  	struct clk *clk;
> +	char *new_name;
> +	size_t name_len;
>  
>  	clk = kzalloc(sizeof(*clk), GFP_KERNEL);
>  	if (!clk)
>  		return NULL;
>  
> -	clk->name = name;
>  	clk->ops = ops;
>  	clk->hw = hw;
>  	hw->clk = clk;
>  
> +	/* Since we currently match clock providers on a purely string
> +	 * based method add a prefix based on the device name if a
> +	 * device is provided.  When we have support for device tree
> +	 * based clock matching it should be possible to avoid this
> +	 * mangling and instead use the struct device to hook into
> +	 * the bindings.
> +	 *
> +	 * As we don't currently support unregistering clocks we don't
> +	 * need to worry about cleanup as yet.
> +	 */
> +	if (dev) {
> +		name_len = strlen(name) + strlen(dev_name(dev)) + 2;
> +		new_name = kzalloc(name_len, GFP_KERNEL);
> +		if (!new_name)
> +			goto err;
> +
> +		snprintf(new_name, name_len, "%s-%s", dev_name(dev), name);
> +
> +		clk->name = new_name;
> +	} else {
> +		clk->name = name;
> +	}

This "clk consolidation" is really idiotic.  The clk matching mechanism
should have _nothing_ to do with the rest of the clk API, especially the
consolidation effort.

It should not matter whether clkdev is used, or an alternative method
to specify this stuff via DT.  Keep the clk_get()/clk_put() _separate_
from the consolidation of the rest.

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH 0/6] clk: Initial feedback for off-SoC slow bus clocks
  2011-07-11  9:31   ` Russell King - ARM Linux
  (?)
@ 2011-07-11 10:07     ` Sascha Hauer
  -1 siblings, 0 replies; 72+ messages in thread
From: Sascha Hauer @ 2011-07-11 10:07 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Jul 11, 2011 at 10:31:24AM +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 11, 2011 at 11:53:44AM +0900, Mark Brown wrote:
> > The biggest issue I ran into was that as the clocks are all registered
> > by name with the API if you've got two instances of the same off-SoC
> > device in the system you'll not be able to disambiguate between the
> > clocks it provides.
> 
> Sigh.  That sounds like yet more trash.  Obviously whoever thought
> up that doesn't actually understand clks.

Nope. In the patches Jeremy posted clocks have a name, but this name
is not meant to be used with clk_get. clk_get is still implemented
in clkdev, so the matching between clocks and devices is independent
of the clock name.

In earlier versions of Jeremys patches the clock name was only present
when debugfs was compiled in and I think it can be changed back to this.

That said the debugfs support (which is not present in Jeremys latest
series) would break if two clocks with the same name have the same
parent, because the clock core would try to create to debugfs entries
with the same name.

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 |

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH 0/6] clk: Initial feedback for off-SoC slow bus clocks
@ 2011-07-11 10:07     ` Sascha Hauer
  0 siblings, 0 replies; 72+ messages in thread
From: Sascha Hauer @ 2011-07-11 10:07 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: Mark Brown, Grant Likely, linux-sh, patches, linux-kernel,
	Jeremy Kerr, linux-arm-kernel

On Mon, Jul 11, 2011 at 10:31:24AM +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 11, 2011 at 11:53:44AM +0900, Mark Brown wrote:
> > The biggest issue I ran into was that as the clocks are all registered
> > by name with the API if you've got two instances of the same off-SoC
> > device in the system you'll not be able to disambiguate between the
> > clocks it provides.
> 
> Sigh.  That sounds like yet more trash.  Obviously whoever thought
> up that doesn't actually understand clks.

Nope. In the patches Jeremy posted clocks have a name, but this name
is not meant to be used with clk_get. clk_get is still implemented
in clkdev, so the matching between clocks and devices is independent
of the clock name.

In earlier versions of Jeremys patches the clock name was only present
when debugfs was compiled in and I think it can be changed back to this.

That said the debugfs support (which is not present in Jeremys latest
series) would break if two clocks with the same name have the same
parent, because the clock core would try to create to debugfs entries
with the same name.

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 |

^ permalink raw reply	[flat|nested] 72+ messages in thread

* [PATCH 0/6] clk: Initial feedback for off-SoC slow bus clocks
@ 2011-07-11 10:07     ` Sascha Hauer
  0 siblings, 0 replies; 72+ messages in thread
From: Sascha Hauer @ 2011-07-11 10:07 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Jul 11, 2011 at 10:31:24AM +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 11, 2011 at 11:53:44AM +0900, Mark Brown wrote:
> > The biggest issue I ran into was that as the clocks are all registered
> > by name with the API if you've got two instances of the same off-SoC
> > device in the system you'll not be able to disambiguate between the
> > clocks it provides.
> 
> Sigh.  That sounds like yet more trash.  Obviously whoever thought
> up that doesn't actually understand clks.

Nope. In the patches Jeremy posted clocks have a name, but this name
is not meant to be used with clk_get. clk_get is still implemented
in clkdev, so the matching between clocks and devices is independent
of the clock name.

In earlier versions of Jeremys patches the clock name was only present
when debugfs was compiled in and I think it can be changed back to this.

That said the debugfs support (which is not present in Jeremys latest
series) would break if two clocks with the same name have the same
parent, because the clock core would try to create to debugfs entries
with the same name.

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 |

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH 0/6] clk: Initial feedback for off-SoC slow bus clocks
  2011-07-11 10:07     ` Sascha Hauer
  (?)
@ 2011-07-11 10:28       ` Russell King - ARM Linux
  -1 siblings, 0 replies; 72+ messages in thread
From: Russell King - ARM Linux @ 2011-07-11 10:28 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Jul 11, 2011 at 12:07:59PM +0200, Sascha Hauer wrote:
> On Mon, Jul 11, 2011 at 10:31:24AM +0100, Russell King - ARM Linux wrote:
> > On Mon, Jul 11, 2011 at 11:53:44AM +0900, Mark Brown wrote:
> > > The biggest issue I ran into was that as the clocks are all registered
> > > by name with the API if you've got two instances of the same off-SoC
> > > device in the system you'll not be able to disambiguate between the
> > > clocks it provides.
> > 
> > Sigh.  That sounds like yet more trash.  Obviously whoever thought
> > up that doesn't actually understand clks.
> 
> Nope. In the patches Jeremy posted clocks have a name, but this name
> is not meant to be used with clk_get. clk_get is still implemented
> in clkdev, so the matching between clocks and devices is independent
> of the clock name.
> 
> In earlier versions of Jeremys patches the clock name was only present
> when debugfs was compiled in and I think it can be changed back to this.
> 
> That said the debugfs support (which is not present in Jeremys latest
> series) would break if two clocks with the same name have the same
> parent, because the clock core would try to create to debugfs entries
> with the same name.

If that's all its for, then can't some other solution for debugfs names
(lets stop calling them clock names to avoid confusion) be found -
such as

	sprintf(debugfsname, "%s@%p", clk->debugfsprefix, clk);

Or how about using something like this:

$debugfs/clkdev/device/connection -> ../../clk/<name-of-clk>
$debugfs/clk/<name-of-clk>

where <name-of-clk> could just be the address of the struct clk (which
eliminates the need to pass any names in) or some prefix plus an
incrementing identifier, or just an incrementing number.

I did think about introducing such a scheme along with clkdev (and did
have some code but that's long gone), but without the $debugfs/clk/ bit
being standardized, it wouldn't have worked.

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH 0/6] clk: Initial feedback for off-SoC slow bus clocks
@ 2011-07-11 10:28       ` Russell King - ARM Linux
  0 siblings, 0 replies; 72+ messages in thread
From: Russell King - ARM Linux @ 2011-07-11 10:28 UTC (permalink / raw)
  To: Sascha Hauer
  Cc: Mark Brown, Grant Likely, linux-sh, patches, linux-kernel,
	Jeremy Kerr, linux-arm-kernel

On Mon, Jul 11, 2011 at 12:07:59PM +0200, Sascha Hauer wrote:
> On Mon, Jul 11, 2011 at 10:31:24AM +0100, Russell King - ARM Linux wrote:
> > On Mon, Jul 11, 2011 at 11:53:44AM +0900, Mark Brown wrote:
> > > The biggest issue I ran into was that as the clocks are all registered
> > > by name with the API if you've got two instances of the same off-SoC
> > > device in the system you'll not be able to disambiguate between the
> > > clocks it provides.
> > 
> > Sigh.  That sounds like yet more trash.  Obviously whoever thought
> > up that doesn't actually understand clks.
> 
> Nope. In the patches Jeremy posted clocks have a name, but this name
> is not meant to be used with clk_get. clk_get is still implemented
> in clkdev, so the matching between clocks and devices is independent
> of the clock name.
> 
> In earlier versions of Jeremys patches the clock name was only present
> when debugfs was compiled in and I think it can be changed back to this.
> 
> That said the debugfs support (which is not present in Jeremys latest
> series) would break if two clocks with the same name have the same
> parent, because the clock core would try to create to debugfs entries
> with the same name.

If that's all its for, then can't some other solution for debugfs names
(lets stop calling them clock names to avoid confusion) be found -
such as

	sprintf(debugfsname, "%s@%p", clk->debugfsprefix, clk);

Or how about using something like this:

$debugfs/clkdev/device/connection -> ../../clk/<name-of-clk>
$debugfs/clk/<name-of-clk>

where <name-of-clk> could just be the address of the struct clk (which
eliminates the need to pass any names in) or some prefix plus an
incrementing identifier, or just an incrementing number.

I did think about introducing such a scheme along with clkdev (and did
have some code but that's long gone), but without the $debugfs/clk/ bit
being standardized, it wouldn't have worked.

^ permalink raw reply	[flat|nested] 72+ messages in thread

* [PATCH 0/6] clk: Initial feedback for off-SoC slow bus clocks
@ 2011-07-11 10:28       ` Russell King - ARM Linux
  0 siblings, 0 replies; 72+ messages in thread
From: Russell King - ARM Linux @ 2011-07-11 10:28 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Jul 11, 2011 at 12:07:59PM +0200, Sascha Hauer wrote:
> On Mon, Jul 11, 2011 at 10:31:24AM +0100, Russell King - ARM Linux wrote:
> > On Mon, Jul 11, 2011 at 11:53:44AM +0900, Mark Brown wrote:
> > > The biggest issue I ran into was that as the clocks are all registered
> > > by name with the API if you've got two instances of the same off-SoC
> > > device in the system you'll not be able to disambiguate between the
> > > clocks it provides.
> > 
> > Sigh.  That sounds like yet more trash.  Obviously whoever thought
> > up that doesn't actually understand clks.
> 
> Nope. In the patches Jeremy posted clocks have a name, but this name
> is not meant to be used with clk_get. clk_get is still implemented
> in clkdev, so the matching between clocks and devices is independent
> of the clock name.
> 
> In earlier versions of Jeremys patches the clock name was only present
> when debugfs was compiled in and I think it can be changed back to this.
> 
> That said the debugfs support (which is not present in Jeremys latest
> series) would break if two clocks with the same name have the same
> parent, because the clock core would try to create to debugfs entries
> with the same name.

If that's all its for, then can't some other solution for debugfs names
(lets stop calling them clock names to avoid confusion) be found -
such as

	sprintf(debugfsname, "%s@%p", clk->debugfsprefix, clk);

Or how about using something like this:

$debugfs/clkdev/device/connection -> ../../clk/<name-of-clk>
$debugfs/clk/<name-of-clk>

where <name-of-clk> could just be the address of the struct clk (which
eliminates the need to pass any names in) or some prefix plus an
incrementing identifier, or just an incrementing number.

I did think about introducing such a scheme along with clkdev (and did
have some code but that's long gone), but without the $debugfs/clk/ bit
being standardized, it wouldn't have worked.

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH 0/6] clk: Initial feedback for off-SoC slow bus clocks
  2011-07-11 10:28       ` Russell King - ARM Linux
  (?)
@ 2011-07-11 10:46         ` Sascha Hauer
  -1 siblings, 0 replies; 72+ messages in thread
From: Sascha Hauer @ 2011-07-11 10:46 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Jul 11, 2011 at 11:28:56AM +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 11, 2011 at 12:07:59PM +0200, Sascha Hauer wrote:
> > On Mon, Jul 11, 2011 at 10:31:24AM +0100, Russell King - ARM Linux wrote:
> > > On Mon, Jul 11, 2011 at 11:53:44AM +0900, Mark Brown wrote:
> > > > The biggest issue I ran into was that as the clocks are all registered
> > > > by name with the API if you've got two instances of the same off-SoC
> > > > device in the system you'll not be able to disambiguate between the
> > > > clocks it provides.
> > > 
> > > Sigh.  That sounds like yet more trash.  Obviously whoever thought
> > > up that doesn't actually understand clks.
> > 
> > Nope. In the patches Jeremy posted clocks have a name, but this name
> > is not meant to be used with clk_get. clk_get is still implemented
> > in clkdev, so the matching between clocks and devices is independent
> > of the clock name.
> > 
> > In earlier versions of Jeremys patches the clock name was only present
> > when debugfs was compiled in and I think it can be changed back to this.
> > 
> > That said the debugfs support (which is not present in Jeremys latest
> > series) would break if two clocks with the same name have the same
> > parent, because the clock core would try to create to debugfs entries
> > with the same name.
> 
> If that's all its for, then can't some other solution for debugfs names
> (lets stop calling them clock names to avoid confusion) be found -
> such as
> 
> 	sprintf(debugfsname, "%s@%p", clk->debugfsprefix, clk);
> 

Sounds good. During my tests on i.MX I found it very convenient to have
speaking debugfs names, so I vote for this suggestion instead the
following ones.

> Or how about using something like this:
> 
> $debugfs/clkdev/device/connection -> ../../clk/<name-of-clk>
> $debugfs/clk/<name-of-clk>
> 
> where <name-of-clk> could just be the address of the struct clk (which
> eliminates the need to pass any names in) or some prefix plus an
> incrementing identifier, or just an incrementing number.
> 
> I did think about introducing such a scheme along with clkdev (and did
> have some code but that's long gone), but without the $debugfs/clk/ bit
> being standardized, it wouldn't have worked.
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

-- 
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 |

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH 0/6] clk: Initial feedback for off-SoC slow bus clocks
@ 2011-07-11 10:46         ` Sascha Hauer
  0 siblings, 0 replies; 72+ messages in thread
From: Sascha Hauer @ 2011-07-11 10:46 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: Grant Likely, patches, linux-sh, Mark Brown, linux-kernel,
	Jeremy Kerr, linux-arm-kernel

On Mon, Jul 11, 2011 at 11:28:56AM +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 11, 2011 at 12:07:59PM +0200, Sascha Hauer wrote:
> > On Mon, Jul 11, 2011 at 10:31:24AM +0100, Russell King - ARM Linux wrote:
> > > On Mon, Jul 11, 2011 at 11:53:44AM +0900, Mark Brown wrote:
> > > > The biggest issue I ran into was that as the clocks are all registered
> > > > by name with the API if you've got two instances of the same off-SoC
> > > > device in the system you'll not be able to disambiguate between the
> > > > clocks it provides.
> > > 
> > > Sigh.  That sounds like yet more trash.  Obviously whoever thought
> > > up that doesn't actually understand clks.
> > 
> > Nope. In the patches Jeremy posted clocks have a name, but this name
> > is not meant to be used with clk_get. clk_get is still implemented
> > in clkdev, so the matching between clocks and devices is independent
> > of the clock name.
> > 
> > In earlier versions of Jeremys patches the clock name was only present
> > when debugfs was compiled in and I think it can be changed back to this.
> > 
> > That said the debugfs support (which is not present in Jeremys latest
> > series) would break if two clocks with the same name have the same
> > parent, because the clock core would try to create to debugfs entries
> > with the same name.
> 
> If that's all its for, then can't some other solution for debugfs names
> (lets stop calling them clock names to avoid confusion) be found -
> such as
> 
> 	sprintf(debugfsname, "%s@%p", clk->debugfsprefix, clk);
> 

Sounds good. During my tests on i.MX I found it very convenient to have
speaking debugfs names, so I vote for this suggestion instead the
following ones.

> Or how about using something like this:
> 
> $debugfs/clkdev/device/connection -> ../../clk/<name-of-clk>
> $debugfs/clk/<name-of-clk>
> 
> where <name-of-clk> could just be the address of the struct clk (which
> eliminates the need to pass any names in) or some prefix plus an
> incrementing identifier, or just an incrementing number.
> 
> I did think about introducing such a scheme along with clkdev (and did
> have some code but that's long gone), but without the $debugfs/clk/ bit
> being standardized, it wouldn't have worked.
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

-- 
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 |

^ permalink raw reply	[flat|nested] 72+ messages in thread

* [PATCH 0/6] clk: Initial feedback for off-SoC slow bus clocks
@ 2011-07-11 10:46         ` Sascha Hauer
  0 siblings, 0 replies; 72+ messages in thread
From: Sascha Hauer @ 2011-07-11 10:46 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Jul 11, 2011 at 11:28:56AM +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 11, 2011 at 12:07:59PM +0200, Sascha Hauer wrote:
> > On Mon, Jul 11, 2011 at 10:31:24AM +0100, Russell King - ARM Linux wrote:
> > > On Mon, Jul 11, 2011 at 11:53:44AM +0900, Mark Brown wrote:
> > > > The biggest issue I ran into was that as the clocks are all registered
> > > > by name with the API if you've got two instances of the same off-SoC
> > > > device in the system you'll not be able to disambiguate between the
> > > > clocks it provides.
> > > 
> > > Sigh.  That sounds like yet more trash.  Obviously whoever thought
> > > up that doesn't actually understand clks.
> > 
> > Nope. In the patches Jeremy posted clocks have a name, but this name
> > is not meant to be used with clk_get. clk_get is still implemented
> > in clkdev, so the matching between clocks and devices is independent
> > of the clock name.
> > 
> > In earlier versions of Jeremys patches the clock name was only present
> > when debugfs was compiled in and I think it can be changed back to this.
> > 
> > That said the debugfs support (which is not present in Jeremys latest
> > series) would break if two clocks with the same name have the same
> > parent, because the clock core would try to create to debugfs entries
> > with the same name.
> 
> If that's all its for, then can't some other solution for debugfs names
> (lets stop calling them clock names to avoid confusion) be found -
> such as
> 
> 	sprintf(debugfsname, "%s@%p", clk->debugfsprefix, clk);
> 

Sounds good. During my tests on i.MX I found it very convenient to have
speaking debugfs names, so I vote for this suggestion instead the
following ones.

> Or how about using something like this:
> 
> $debugfs/clkdev/device/connection -> ../../clk/<name-of-clk>
> $debugfs/clk/<name-of-clk>
> 
> where <name-of-clk> could just be the address of the struct clk (which
> eliminates the need to pass any names in) or some prefix plus an
> incrementing identifier, or just an incrementing number.
> 
> I did think about introducing such a scheme along with clkdev (and did
> have some code but that's long gone), but without the $debugfs/clk/ bit
> being standardized, it wouldn't have worked.
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

-- 
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 |

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH 5/6] clk: Support multiple instances of the same clock
  2011-07-11  9:34       ` Russell King - ARM Linux
  (?)
@ 2011-07-11 10:53         ` Mark Brown
  -1 siblings, 0 replies; 72+ messages in thread
From: Mark Brown @ 2011-07-11 10:53 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Jul 11, 2011 at 10:34:39AM +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 11, 2011 at 11:53:56AM +0900, Mark Brown wrote:

> > +	/* Since we currently match clock providers on a purely string
> > +	 * based method add a prefix based on the device name if a
> > +	 * device is provided.  When we have support for device tree

> This "clk consolidation" is really idiotic.  The clk matching mechanism
> should have _nothing_ to do with the rest of the clk API, especially the
> consolidation effort.

It's not touching clkdev, the comment is somewhat misleading and is
mostly based on me thinking about how we'd deploy off-SoC clocks.
There's also the diagnostic issues Sacha mentioned, if we don't keep
some source information handy it's hard to tell what clock logging is
talking about.

> It should not matter whether clkdev is used, or an alternative method
> to specify this stuff via DT.  Keep the clk_get()/clk_put() _separate_
> from the consolidation of the rest.

We do need some way to have some idea which clocks we're talking about,
and for off-SoC stuff passing around struct clk pointers is rather
painful.  At some point some bit of code is going to have to get hold of
the actual struct clk and then map it onto the devices using it.

For device tree we should be able to do that fairly painlessly with just
the struct devices, without device tree you either have to have the
structs handy or use names.  At the minute the infrastructure is
somewhat lacking here.

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH 5/6] clk: Support multiple instances of the same clock provider
@ 2011-07-11 10:53         ` Mark Brown
  0 siblings, 0 replies; 72+ messages in thread
From: Mark Brown @ 2011-07-11 10:53 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: Jeremy Kerr, Grant Likely, linux-sh, patches, linux-kernel,
	linux-arm-kernel

On Mon, Jul 11, 2011 at 10:34:39AM +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 11, 2011 at 11:53:56AM +0900, Mark Brown wrote:

> > +	/* Since we currently match clock providers on a purely string
> > +	 * based method add a prefix based on the device name if a
> > +	 * device is provided.  When we have support for device tree

> This "clk consolidation" is really idiotic.  The clk matching mechanism
> should have _nothing_ to do with the rest of the clk API, especially the
> consolidation effort.

It's not touching clkdev, the comment is somewhat misleading and is
mostly based on me thinking about how we'd deploy off-SoC clocks.
There's also the diagnostic issues Sacha mentioned, if we don't keep
some source information handy it's hard to tell what clock logging is
talking about.

> It should not matter whether clkdev is used, or an alternative method
> to specify this stuff via DT.  Keep the clk_get()/clk_put() _separate_
> from the consolidation of the rest.

We do need some way to have some idea which clocks we're talking about,
and for off-SoC stuff passing around struct clk pointers is rather
painful.  At some point some bit of code is going to have to get hold of
the actual struct clk and then map it onto the devices using it.

For device tree we should be able to do that fairly painlessly with just
the struct devices, without device tree you either have to have the
structs handy or use names.  At the minute the infrastructure is
somewhat lacking here.

^ permalink raw reply	[flat|nested] 72+ messages in thread

* [PATCH 5/6] clk: Support multiple instances of the same clock provider
@ 2011-07-11 10:53         ` Mark Brown
  0 siblings, 0 replies; 72+ messages in thread
From: Mark Brown @ 2011-07-11 10:53 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Jul 11, 2011 at 10:34:39AM +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 11, 2011 at 11:53:56AM +0900, Mark Brown wrote:

> > +	/* Since we currently match clock providers on a purely string
> > +	 * based method add a prefix based on the device name if a
> > +	 * device is provided.  When we have support for device tree

> This "clk consolidation" is really idiotic.  The clk matching mechanism
> should have _nothing_ to do with the rest of the clk API, especially the
> consolidation effort.

It's not touching clkdev, the comment is somewhat misleading and is
mostly based on me thinking about how we'd deploy off-SoC clocks.
There's also the diagnostic issues Sacha mentioned, if we don't keep
some source information handy it's hard to tell what clock logging is
talking about.

> It should not matter whether clkdev is used, or an alternative method
> to specify this stuff via DT.  Keep the clk_get()/clk_put() _separate_
> from the consolidation of the rest.

We do need some way to have some idea which clocks we're talking about,
and for off-SoC stuff passing around struct clk pointers is rather
painful.  At some point some bit of code is going to have to get hold of
the actual struct clk and then map it onto the devices using it.

For device tree we should be able to do that fairly painlessly with just
the struct devices, without device tree you either have to have the
structs handy or use names.  At the minute the infrastructure is
somewhat lacking here.

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH 5/6] clk: Support multiple instances of the same clock
  2011-07-11 10:53         ` Mark Brown
  (?)
@ 2011-07-11 11:11           ` Russell King - ARM Linux
  -1 siblings, 0 replies; 72+ messages in thread
From: Russell King - ARM Linux @ 2011-07-11 11:11 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Jul 11, 2011 at 07:53:45PM +0900, Mark Brown wrote:
> We do need some way to have some idea which clocks we're talking about,
> and for off-SoC stuff passing around struct clk pointers is rather
> painful.  At some point some bit of code is going to have to get hold of
> the actual struct clk and then map it onto the devices using it.

As I haven't seen any of this "passing around struct clk pointers" crap
recently, I can only guess at what you're saying.  I thought all that
had been solved with _proper_ use of clkdev.

clkdev can provide you with a struct clk for _any_ device in the system
where its name is known at build time.

For devices where the name is not known at build time, a new cl_lookup
entry can be created at runtime with clkdev_alloc() or clk_add_alias(),
and dropped with clkdev_drop().

The problem comes when you aren't able to hook into a subsystem which
creates an unstable device name (eg, USB).  I suspect that's also a
problem for DT too because there has to be some way to go from a struct
device to something stable.

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH 5/6] clk: Support multiple instances of the same clock provider
@ 2011-07-11 11:11           ` Russell King - ARM Linux
  0 siblings, 0 replies; 72+ messages in thread
From: Russell King - ARM Linux @ 2011-07-11 11:11 UTC (permalink / raw)
  To: Mark Brown
  Cc: Jeremy Kerr, Grant Likely, linux-sh, patches, linux-kernel,
	linux-arm-kernel

On Mon, Jul 11, 2011 at 07:53:45PM +0900, Mark Brown wrote:
> We do need some way to have some idea which clocks we're talking about,
> and for off-SoC stuff passing around struct clk pointers is rather
> painful.  At some point some bit of code is going to have to get hold of
> the actual struct clk and then map it onto the devices using it.

As I haven't seen any of this "passing around struct clk pointers" crap
recently, I can only guess at what you're saying.  I thought all that
had been solved with _proper_ use of clkdev.

clkdev can provide you with a struct clk for _any_ device in the system
where its name is known at build time.

For devices where the name is not known at build time, a new cl_lookup
entry can be created at runtime with clkdev_alloc() or clk_add_alias(),
and dropped with clkdev_drop().

The problem comes when you aren't able to hook into a subsystem which
creates an unstable device name (eg, USB).  I suspect that's also a
problem for DT too because there has to be some way to go from a struct
device to something stable.

^ permalink raw reply	[flat|nested] 72+ messages in thread

* [PATCH 5/6] clk: Support multiple instances of the same clock provider
@ 2011-07-11 11:11           ` Russell King - ARM Linux
  0 siblings, 0 replies; 72+ messages in thread
From: Russell King - ARM Linux @ 2011-07-11 11:11 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Jul 11, 2011 at 07:53:45PM +0900, Mark Brown wrote:
> We do need some way to have some idea which clocks we're talking about,
> and for off-SoC stuff passing around struct clk pointers is rather
> painful.  At some point some bit of code is going to have to get hold of
> the actual struct clk and then map it onto the devices using it.

As I haven't seen any of this "passing around struct clk pointers" crap
recently, I can only guess at what you're saying.  I thought all that
had been solved with _proper_ use of clkdev.

clkdev can provide you with a struct clk for _any_ device in the system
where its name is known at build time.

For devices where the name is not known at build time, a new cl_lookup
entry can be created at runtime with clkdev_alloc() or clk_add_alias(),
and dropped with clkdev_drop().

The problem comes when you aren't able to hook into a subsystem which
creates an unstable device name (eg, USB).  I suspect that's also a
problem for DT too because there has to be some way to go from a struct
device to something stable.

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH 5/6] clk: Support multiple instances of the same clock
  2011-07-11 11:11           ` Russell King - ARM Linux
  (?)
@ 2011-07-11 11:41             ` Mark Brown
  -1 siblings, 0 replies; 72+ messages in thread
From: Mark Brown @ 2011-07-11 11:41 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Jul 11, 2011 at 12:11:53PM +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 11, 2011 at 07:53:45PM +0900, Mark Brown wrote:
> > We do need some way to have some idea which clocks we're talking about,
> > and for off-SoC stuff passing around struct clk pointers is rather
> > painful.  At some point some bit of code is going to have to get hold of
> > the actual struct clk and then map it onto the devices using it.

> As I haven't seen any of this "passing around struct clk pointers" crap
> recently, I can only guess at what you're saying.  I thought all that
> had been solved with _proper_ use of clkdev.

The problem is getting the data into clkdev in the first place for the
off-SoC devices.

> clkdev can provide you with a struct clk for _any_ device in the system
> where its name is known at build time.

Right, on the consumer side it's all sorted and works really well.  The
trick is on the provider side where you need to have the clock available
to be able to add the mappings for it.

Perhaps I'm just missing something about how this should all work -
actually, the regulator solution probably works fine here.  Pass an
array of mappings into either the driver or the clock subsystem and have
them add the mappings when the provider registers.  Whichever way around
should just be some hooks in the new subsystem.

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH 5/6] clk: Support multiple instances of the same clock provider
@ 2011-07-11 11:41             ` Mark Brown
  0 siblings, 0 replies; 72+ messages in thread
From: Mark Brown @ 2011-07-11 11:41 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: Jeremy Kerr, Grant Likely, linux-sh, patches, linux-kernel,
	linux-arm-kernel

On Mon, Jul 11, 2011 at 12:11:53PM +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 11, 2011 at 07:53:45PM +0900, Mark Brown wrote:
> > We do need some way to have some idea which clocks we're talking about,
> > and for off-SoC stuff passing around struct clk pointers is rather
> > painful.  At some point some bit of code is going to have to get hold of
> > the actual struct clk and then map it onto the devices using it.

> As I haven't seen any of this "passing around struct clk pointers" crap
> recently, I can only guess at what you're saying.  I thought all that
> had been solved with _proper_ use of clkdev.

The problem is getting the data into clkdev in the first place for the
off-SoC devices.

> clkdev can provide you with a struct clk for _any_ device in the system
> where its name is known at build time.

Right, on the consumer side it's all sorted and works really well.  The
trick is on the provider side where you need to have the clock available
to be able to add the mappings for it.

Perhaps I'm just missing something about how this should all work -
actually, the regulator solution probably works fine here.  Pass an
array of mappings into either the driver or the clock subsystem and have
them add the mappings when the provider registers.  Whichever way around
should just be some hooks in the new subsystem.

^ permalink raw reply	[flat|nested] 72+ messages in thread

* [PATCH 5/6] clk: Support multiple instances of the same clock provider
@ 2011-07-11 11:41             ` Mark Brown
  0 siblings, 0 replies; 72+ messages in thread
From: Mark Brown @ 2011-07-11 11:41 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Jul 11, 2011 at 12:11:53PM +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 11, 2011 at 07:53:45PM +0900, Mark Brown wrote:
> > We do need some way to have some idea which clocks we're talking about,
> > and for off-SoC stuff passing around struct clk pointers is rather
> > painful.  At some point some bit of code is going to have to get hold of
> > the actual struct clk and then map it onto the devices using it.

> As I haven't seen any of this "passing around struct clk pointers" crap
> recently, I can only guess at what you're saying.  I thought all that
> had been solved with _proper_ use of clkdev.

The problem is getting the data into clkdev in the first place for the
off-SoC devices.

> clkdev can provide you with a struct clk for _any_ device in the system
> where its name is known at build time.

Right, on the consumer side it's all sorted and works really well.  The
trick is on the provider side where you need to have the clock available
to be able to add the mappings for it.

Perhaps I'm just missing something about how this should all work -
actually, the regulator solution probably works fine here.  Pass an
array of mappings into either the driver or the clock subsystem and have
them add the mappings when the provider registers.  Whichever way around
should just be some hooks in the new subsystem.

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH 0/6] clk: Initial feedback for off-SoC slow bus clocks
  2011-07-11 10:46         ` Sascha Hauer
  (?)
@ 2011-07-11 11:43           ` Mark Brown
  -1 siblings, 0 replies; 72+ messages in thread
From: Mark Brown @ 2011-07-11 11:43 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Jul 11, 2011 at 12:46:25PM +0200, Sascha Hauer wrote:
> On Mon, Jul 11, 2011 at 11:28:56AM +0100, Russell King - ARM Linux wrote:

> > If that's all its for, then can't some other solution for debugfs names
> > (lets stop calling them clock names to avoid confusion) be found -
> > such as

> > 	sprintf(debugfsname, "%s@%p", clk->debugfsprefix, clk);

> Sounds good. During my tests on i.MX I found it very convenient to have
> speaking debugfs names, so I vote for this suggestion instead the
> following ones.

FWIW that's pretty much exactly what I added except I used a - instead
of @.

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH 0/6] clk: Initial feedback for off-SoC slow bus clocks
@ 2011-07-11 11:43           ` Mark Brown
  0 siblings, 0 replies; 72+ messages in thread
From: Mark Brown @ 2011-07-11 11:43 UTC (permalink / raw)
  To: Sascha Hauer
  Cc: Russell King - ARM Linux, Grant Likely, patches, linux-sh,
	linux-kernel, Jeremy Kerr, linux-arm-kernel

On Mon, Jul 11, 2011 at 12:46:25PM +0200, Sascha Hauer wrote:
> On Mon, Jul 11, 2011 at 11:28:56AM +0100, Russell King - ARM Linux wrote:

> > If that's all its for, then can't some other solution for debugfs names
> > (lets stop calling them clock names to avoid confusion) be found -
> > such as

> > 	sprintf(debugfsname, "%s@%p", clk->debugfsprefix, clk);

> Sounds good. During my tests on i.MX I found it very convenient to have
> speaking debugfs names, so I vote for this suggestion instead the
> following ones.

FWIW that's pretty much exactly what I added except I used a - instead
of @.

^ permalink raw reply	[flat|nested] 72+ messages in thread

* [PATCH 0/6] clk: Initial feedback for off-SoC slow bus clocks
@ 2011-07-11 11:43           ` Mark Brown
  0 siblings, 0 replies; 72+ messages in thread
From: Mark Brown @ 2011-07-11 11:43 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Jul 11, 2011 at 12:46:25PM +0200, Sascha Hauer wrote:
> On Mon, Jul 11, 2011 at 11:28:56AM +0100, Russell King - ARM Linux wrote:

> > If that's all its for, then can't some other solution for debugfs names
> > (lets stop calling them clock names to avoid confusion) be found -
> > such as

> > 	sprintf(debugfsname, "%s@%p", clk->debugfsprefix, clk);

> Sounds good. During my tests on i.MX I found it very convenient to have
> speaking debugfs names, so I vote for this suggestion instead the
> following ones.

FWIW that's pretty much exactly what I added except I used a - instead
of @.

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH 1/6] clk: Prototype and document clk_register()
  2011-07-11  2:53   ` Mark Brown
  (?)
@ 2011-07-15  2:53     ` Grant Likely
  -1 siblings, 0 replies; 72+ messages in thread
From: Grant Likely @ 2011-07-15  2:53 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Jul 11, 2011 at 11:53:52AM +0900, Mark Brown wrote:
> This allows the compiler to ensure drivers are using the correct prototype.
> 
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

Acked-by: Grant Likely <grant.likely@secretlab.ca>

> ---
>  include/linux/clk.h |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 7c26135..2ca4f66 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -136,6 +136,18 @@ extern struct clk_hw_ops clk_gate_ops;
>  
>  #endif /* CONFIG_GENERIC_CLK_GATE */
>  
> +/**
> + * clk_register - register and initialize a new clock
> + *
> + * @ops: ops for the new clock
> + * @hw: struct clk_hw to be passed to the ops of the new clock
> + * @name: name to use for the new clock
> + *
> + * Register a new clock with the clk subsytem.  Returns either a
> + * struct clk for the new clock or a NULL pointer.
> + */
> +struct clk *clk_register(struct clk_hw_ops *ops, struct clk_hw *hw,
> +			 const char *name);
>  
>  #else /* !CONFIG_GENERIC_CLK */
>  
> -- 
> 1.7.5.4
> 

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH 1/6] clk: Prototype and document clk_register()
@ 2011-07-15  2:53     ` Grant Likely
  0 siblings, 0 replies; 72+ messages in thread
From: Grant Likely @ 2011-07-15  2:53 UTC (permalink / raw)
  To: Mark Brown
  Cc: Jeremy Kerr, Grant Likely, linux-kernel, linux-arm-kernel,
	linux-sh, patches

On Mon, Jul 11, 2011 at 11:53:52AM +0900, Mark Brown wrote:
> This allows the compiler to ensure drivers are using the correct prototype.
> 
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

Acked-by: Grant Likely <grant.likely@secretlab.ca>

> ---
>  include/linux/clk.h |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 7c26135..2ca4f66 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -136,6 +136,18 @@ extern struct clk_hw_ops clk_gate_ops;
>  
>  #endif /* CONFIG_GENERIC_CLK_GATE */
>  
> +/**
> + * clk_register - register and initialize a new clock
> + *
> + * @ops: ops for the new clock
> + * @hw: struct clk_hw to be passed to the ops of the new clock
> + * @name: name to use for the new clock
> + *
> + * Register a new clock with the clk subsytem.  Returns either a
> + * struct clk for the new clock or a NULL pointer.
> + */
> +struct clk *clk_register(struct clk_hw_ops *ops, struct clk_hw *hw,
> +			 const char *name);
>  
>  #else /* !CONFIG_GENERIC_CLK */
>  
> -- 
> 1.7.5.4
> 

^ permalink raw reply	[flat|nested] 72+ messages in thread

* [PATCH 1/6] clk: Prototype and document clk_register()
@ 2011-07-15  2:53     ` Grant Likely
  0 siblings, 0 replies; 72+ messages in thread
From: Grant Likely @ 2011-07-15  2:53 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Jul 11, 2011 at 11:53:52AM +0900, Mark Brown wrote:
> This allows the compiler to ensure drivers are using the correct prototype.
> 
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

Acked-by: Grant Likely <grant.likely@secretlab.ca>

> ---
>  include/linux/clk.h |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 7c26135..2ca4f66 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -136,6 +136,18 @@ extern struct clk_hw_ops clk_gate_ops;
>  
>  #endif /* CONFIG_GENERIC_CLK_GATE */
>  
> +/**
> + * clk_register - register and initialize a new clock
> + *
> + * @ops: ops for the new clock
> + * @hw: struct clk_hw to be passed to the ops of the new clock
> + * @name: name to use for the new clock
> + *
> + * Register a new clock with the clk subsytem.  Returns either a
> + * struct clk for the new clock or a NULL pointer.
> + */
> +struct clk *clk_register(struct clk_hw_ops *ops, struct clk_hw *hw,
> +			 const char *name);
>  
>  #else /* !CONFIG_GENERIC_CLK */
>  
> -- 
> 1.7.5.4
> 

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH 6/6] clk: Add initial WM831x clock driver
  2011-07-11  2:53     ` Mark Brown
  (?)
@ 2011-07-15  2:53       ` Grant Likely
  -1 siblings, 0 replies; 72+ messages in thread
From: Grant Likely @ 2011-07-15  2:53 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Jul 11, 2011 at 11:53:57AM +0900, Mark Brown wrote:
> The WM831x and WM832x series of PMICs contain a flexible clocking
> subsystem intended to provide always on and system core clocks.  It
> features:
> 
> - A 32.768kHz crystal oscillator which can optionally be used to pass
>   through an externally generated clock.
> - A FLL which can be clocked from either the 32.768kHz oscillator or
>   the CLKIN pin.
> - A CLKOUT pin which can bring out either the oscillator or the FLL
>   output.
> - The 32.768kHz clock can also optionally be brought out on the GPIO
>   pins of the device.
> 
> This driver fully supports the 32.768kHz oscillator and CLKOUT.  The FLL
> is supported only in AUTO mode, the full flexibility of the FLL cannot
> currently be used.  The use of clock references other than the internal
> oscillator is not currently supported, and since clk_set_parent() is not
> implemented in the generic clock API the clock tree configuration cannot
> be changed at runtime.
> 
> Due to a lack of access to systems where the core SoC has been converted
> to use the generic clock API this driver has been compile tested only.

Generally seems okay.  Minor comments below.

g.

> 
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> ---
>  MAINTAINERS              |    1 +
>  drivers/clk/Kconfig      |    5 +
>  drivers/clk/Makefile     |    1 +
>  drivers/clk/clk-wm831x.c |  389 ++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 396 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/clk/clk-wm831x.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ae563fa..c234756 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6969,6 +6969,7 @@ T:	git git://opensource.wolfsonmicro.com/linux-2.6-audioplus
>  W:	http://opensource.wolfsonmicro.com/content/linux-drivers-wolfson-devices
>  S:	Supported
>  F:	Documentation/hwmon/wm83??
> +F:	drivers/clk/clk-wm83*.c
>  F:	drivers/leds/leds-wm83*.c
>  F:	drivers/mfd/wm8*.c
>  F:	drivers/power/wm83*.c
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 1fd0070..7f6eec2 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -10,6 +10,7 @@ config GENERIC_CLK_BUILD_TEST
>  	depends on EXPERIMENTAL && GENERIC_CLK
>  	select GENERIC_CLK_FIXED
>  	select GENERIC_CLK_GATE
> +	select GENERIC_CLK_WM831X if MFD_WM831X=y

Hmmm, this could get unwieldy in a hurry.

>  	help
>  	   Enable all possible generic clock drivers.  This is only
>  	   useful for improving build coverage, it is not useful for
> @@ -22,3 +23,7 @@ config GENERIC_CLK_FIXED
>  config GENERIC_CLK_GATE
>  	bool
>  	depends on GENERIC_CLK
> +
> +config GENERIC_CLK_WM831X
> +	tristate
> +	depends on GENERIC_CLK && MFD_WM831X
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index d186446..6628ad5 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -3,3 +3,4 @@ obj-$(CONFIG_CLKDEV_LOOKUP)	+= clkdev.o
>  obj-$(CONFIG_GENERIC_CLK)	+= clk.o
>  obj-$(CONFIG_GENERIC_CLK_FIXED)	+= clk-fixed.o
>  obj-$(CONFIG_GENERIC_CLK_GATE)	+= clk-gate.o
> +obj-$(CONFIG_GENERIC_CLK_WM831X) += clk-wm831x.o
> diff --git a/drivers/clk/clk-wm831x.c b/drivers/clk/clk-wm831x.c
> new file mode 100644
> index 0000000..82782f1
> --- /dev/null
> +++ b/drivers/clk/clk-wm831x.c
> @@ -0,0 +1,389 @@
> +/*
> + * WM831x clock control
> + *
> + * Copyright 2011 Wolfson Microelectronics PLC.
> + *
> + * Author: Mark Brown <broonie@opensource.wolfsonmicro.com>
> + *
> + *  This program is free software; you can redistribute  it and/or modify it
> + *  under  the terms of  the GNU General  Public License as published by the
> + *  Free Software Foundation;  either version 2 of the  License, or (at your
> + *  option) any later version.
> + *
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/platform_device.h>
> +#include <linux/mfd/wm831x/core.h>
> +
> +struct wm831x_clk {
> +	struct wm831x *wm831x;
> +	struct clk_hw xtal_hw;
> +	struct clk_hw fll_hw;
> +	struct clk_hw clkout_hw;
> +	bool xtal_ena;
> +};
> +
> +static int wm831x_xtal_enable(struct clk_hw *hw)
> +{
> +	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
> +						  xtal_hw);

This container of is used 10 times.  A static inline would be
reasonable.

> +
> +	if (clkdata->xtal_ena)
> +		return 0;
> +	else
> +		return -EPERM;
> +}

Nit: return clkdata->xtal_ena ? 0 : -EPERM;

Just makes for more concise code.

> +
> +static unsigned long wm831x_xtal_recalc_rate(struct clk_hw *hw)
> +{
> +	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
> +						  xtal_hw);
> +
> +	if (clkdata->xtal_ena)
> +		return 32768;
> +	else
> +		return 0;
> +}
> +
> +static long wm831x_xtal_round_rate(struct clk_hw *hw, unsigned long rate)
> +{
> +	return wm831x_xtal_recalc_rate(hw);
> +}
> +
> +static const struct clk_hw_ops wm831x_xtal_ops = {
> +	.enable = wm831x_xtal_enable,
> +	.recalc_rate = wm831x_xtal_recalc_rate,
> +	.round_rate = wm831x_xtal_round_rate,
> +};
> +
> +static const unsigned long wm831x_fll_auto_rates[] = {
> +	 2048000,
> +	11289600,
> +	12000000,
> +	12288000,
> +	19200000,
> +	22579600,
> +	24000000,
> +	24576000,
> +};
> +
> +static bool wm831x_fll_enabled(struct wm831x *wm831x)
> +{
> +	int ret;
> +
> +	ret = wm831x_reg_read(wm831x, WM831X_FLL_CONTROL_1);
> +	if (ret < 0) {
> +		dev_err(wm831x->dev, "Unable to read FLL_CONTROL_1: %d\n",
> +			ret);
> +		return true;
> +	}
> +
> +	if (ret & WM831X_FLL_ENA)
> +		return true;
> +	else
> +		return false;

similarly, return (ret & WM831X_FLL_ENA) != 0;

> +}
> +
> +static int wm831x_fll_prepare(struct clk_hw *hw)
> +{
> +	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
> +						  fll_hw);
> +	struct wm831x *wm831x = clkdata->wm831x;
> +	int ret;
> +
> +	ret = wm831x_set_bits(wm831x, WM831X_FLL_CONTROL_2,
> +			      WM831X_FLL_ENA, WM831X_FLL_ENA);
> +	if (ret != 0)
> +		dev_crit(wm831x->dev, "Failed to enable FLL: %d\n", ret);
> +
> +	return ret;
> +}
> +
> +static void wm831x_fll_unprepare(struct clk_hw *hw)
> +{
> +	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
> +						  fll_hw);
> +	struct wm831x *wm831x = clkdata->wm831x;
> +	int ret;
> +
> +	ret = wm831x_set_bits(wm831x, WM831X_FLL_CONTROL_2, WM831X_FLL_ENA, 0);
> +	if (ret != 0)
> +		dev_crit(wm831x->dev, "Failed to disaable FLL: %d\n", ret);
> +}
> +
> +static unsigned long wm831x_fll_recalc_rate(struct clk_hw *hw)
> +{
> +	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
> +						  fll_hw);
> +	struct wm831x *wm831x = clkdata->wm831x;
> +	int ret;
> +
> +	ret = wm831x_reg_read(wm831x, WM831X_CLOCK_CONTROL_2);
> +	if (ret < 0) {
> +		dev_err(wm831x->dev, "Unable to read CLOCK_CONTROL_2: %d\n",
> +			ret);
> +		return 0;
> +	}
> +
> +	if (ret & WM831X_FLL_AUTO)
> +		return wm831x_fll_auto_rates[ret & WM831X_FLL_AUTO_FREQ_MASK];
> +
> +	dev_err(wm831x->dev, "FLL only supported in AUTO mode\n");
> +	return 0;
> +}
> +
> +static int wm831x_fll_set_rate(struct clk_hw *hw, unsigned long rate,
> +			       unsigned long *parent_rate)
> +{
> +	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
> +						  fll_hw);
> +	struct wm831x *wm831x = clkdata->wm831x;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(wm831x_fll_auto_rates); i++)
> +		if (wm831x_fll_auto_rates[i] = rate)
> +			break;
> +	if (i = ARRAY_SIZE(wm831x_fll_auto_rates))
> +		return -EINVAL;
> +
> +	if (wm831x_fll_enabled(wm831x))
> +		return -EPERM;
> +
> +	return wm831x_set_bits(wm831x, WM831X_CLOCK_CONTROL_2,
> +			       WM831X_FLL_AUTO_FREQ_MASK, i);
> +}
> +
> +static struct clk *wm831x_fll_get_parent(struct clk_hw *hw)
> +{
> +	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
> +						  fll_hw);
> +	struct wm831x *wm831x = clkdata->wm831x;
> +	int ret;
> +
> +	/* AUTO mode is always clocked from the crystal */
> +	ret = wm831x_reg_read(wm831x, WM831X_CLOCK_CONTROL_2);
> +	if (ret < 0) {
> +		dev_err(wm831x->dev, "Unable to read CLOCK_CONTROL_2: %d\n",
> +			ret);
> +		return NULL;
> +	}
> +
> +	if (ret & WM831X_FLL_AUTO)
> +		return clkdata->xtal_hw.clk;
> +
> +	ret = wm831x_reg_read(wm831x, WM831X_FLL_CONTROL_5);
> +	if (ret < 0) {
> +		dev_err(wm831x->dev, "Unable to read FLL_CONTROL_5: %d\n",
> +			ret);
> +		return NULL;
> +	}
> +
> +	switch (ret & WM831X_FLL_CLK_SRC_MASK) {
> +	case 0:
> +		return clkdata->xtal_hw.clk;
> +	case 1:
> +		dev_warn(wm831x->dev,
> +			 "FLL clocked from CLKIN not yet supported\n");
> +		return NULL;
> +	default:
> +		dev_err(wm831x->dev, "Unsupported FLL clock source %d\n",
> +			ret & WM831X_FLL_CLK_SRC_MASK);
> +		return NULL;
> +	}
> +}
> +
> +static const struct clk_hw_ops wm831x_fll_ops = {
> +	.prepare = wm831x_fll_prepare,
> +	.unprepare = wm831x_fll_unprepare,
> +	.recalc_rate = wm831x_fll_recalc_rate,
> +	.set_rate = wm831x_fll_set_rate,
> +	.get_parent = wm831x_fll_get_parent,
> +};
> +
> +static int wm831x_clkout_prepare(struct clk_hw *hw)
> +{
> +	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
> +						  clkout_hw);
> +	struct wm831x *wm831x = clkdata->wm831x;
> +	int ret;
> +
> +	ret = wm831x_reg_unlock(wm831x);
> +	if (ret != 0) {
> +		dev_crit(wm831x->dev, "Failed to lock registers: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = wm831x_set_bits(wm831x, WM831X_CLOCK_CONTROL_1,
> +			      WM831X_CLKOUT_ENA, WM831X_CLKOUT_ENA);
> +	if (ret != 0)
> +		dev_crit(wm831x->dev, "Failed to enable CLKOUT: %d\n", ret);
> +
> +	wm831x_reg_lock(wm831x);
> +
> +	return ret;
> +}
> +
> +static void wm831x_clkout_unprepare(struct clk_hw *hw)
> +{
> +	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
> +						  clkout_hw);
> +	struct wm831x *wm831x = clkdata->wm831x;
> +	int ret;
> +
> +	ret = wm831x_reg_unlock(wm831x);
> +	if (ret != 0) {
> +		dev_crit(wm831x->dev, "Failed to lock registers: %d\n", ret);
> +		return;
> +	}
> +
> +	ret = wm831x_set_bits(wm831x, WM831X_CLOCK_CONTROL_1,
> +			      WM831X_CLKOUT_ENA, 0);
> +	if (ret != 0)
> +		dev_crit(wm831x->dev, "Failed to disable CLKOUT: %d\n", ret);
> +
> +	wm831x_reg_lock(wm831x);
> +}
> +
> +static unsigned long wm831x_clkout_recalc_rate(struct clk_hw *hw)
> +{
> +	return clk_get_rate(clk_get_parent(hw->clk));
> +}
> +
> +static long wm831x_clkout_round_rate(struct clk_hw *hw, unsigned long rate)
> +{
> +	return clk_round_rate(clk_get_parent(hw->clk), rate);
> +}
> +
> +static int wm831x_clkout_set_rate(struct clk_hw *hw, unsigned long rate,
> +				  unsigned long *parent_rate)
> +{
> +	*parent_rate = rate;
> +	return CLK_SET_RATE_PROPAGATE;
> +}
> +
> +static struct clk *wm831x_clkout_get_parent(struct clk_hw *hw)
> +{
> +	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
> +						  clkout_hw);
> +	struct wm831x *wm831x = clkdata->wm831x;
> +	int ret;
> +
> +	ret = wm831x_reg_read(wm831x, WM831X_CLOCK_CONTROL_1);
> +	if (ret < 0) {
> +		dev_err(wm831x->dev, "Unable to read CLOCK_CONTROL_1: %d\n",
> +			ret);
> +		return NULL;
> +	}
> +
> +	if (ret & WM831X_CLKOUT_SRC)
> +		return clkdata->xtal_hw.clk;
> +	else
> +		return clkdata->fll_hw.clk;
> +}
> +
> +static const struct clk_hw_ops wm831x_clkout_ops = {
> +	.prepare = wm831x_clkout_prepare,
> +	.unprepare = wm831x_clkout_unprepare,
> +	.recalc_rate = wm831x_clkout_recalc_rate,
> +	.round_rate = wm831x_clkout_round_rate,
> +	.set_rate = wm831x_clkout_set_rate,
> +	.get_parent = wm831x_clkout_get_parent,
> +};
> +
> +static __devinit int wm831x_clk_probe(struct platform_device *pdev)
> +{
> +	struct wm831x *wm831x = dev_get_drvdata(pdev->dev.parent);
> +	struct wm831x_clk *clkdata;
> +	int ret;
> +
> +	clkdata = kzalloc(sizeof(*clkdata), GFP_KERNEL);
> +	if (!clkdata)
> +		return -ENOMEM;
> +
> +	/* XTAL_ENA can only be set via OTP/InstantConfig so just read once */
> +	ret = wm831x_reg_read(wm831x, WM831X_CLOCK_CONTROL_2);
> +	if (ret < 0) {
> +		dev_err(wm831x->dev, "Unable to read CLOCK_CONTROL_2: %d\n",
> +			ret);
> +		goto err_alloc;
> +	}
> +	clkdata->xtal_ena = ret & WM831X_XTAL_ENA;
> +
> +	if (!clk_register(wm831x->dev, &wm831x_xtal_ops, &clkdata->xtal_hw,
> +			  "xtal")) {
> +		ret = -EINVAL;
> +		goto err_alloc;
> +	}
> +
> +	if (!clk_register(wm831x->dev, &wm831x_fll_ops, &clkdata->fll_hw,
> +			  "fll")) {
> +		ret = -EINVAL;
> +		goto err_xtal;
> +	}
> +
> +	if (!clk_register(wm831x->dev, &wm831x_clkout_ops, &clkdata->clkout_hw,
> +			  "clkout")) {
> +		ret = -EINVAL;
> +		goto err_fll;
> +	}

How common will this pattern be?  Is there need for a
clk_register_many() variant?

> +
> +	dev_set_drvdata(&pdev->dev, clkdata);
> +
> +	return 0;
> +
> +err_fll:
> +	clk_unregister(clkdata->fll_hw.clk);
> +err_xtal:
> +	clk_unregister(clkdata->xtal_hw.clk);
> +err_alloc:
> +	kfree(clkdata);
> +	return ret;
> +}
> +
> +static __devexit int wm831x_clk_remove(struct platform_device *pdev)
> +{
> +	struct wm831x_clk *clkdata = dev_get_drvdata(&pdev->dev);
> +
> +	clk_unregister(clkdata->clkout_hw.clk);
> +	clk_unregister(clkdata->fll_hw.clk);
> +	clk_unregister(clkdata->xtal_hw.clk);
> +	kfree(clkdata);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver wm831x_clk_driver = {
> +	.probe = wm831x_clk_probe,
> +	.remove = __devexit_p(wm831x_clk_remove),
> +	.driver		= {
> +		.name	= "wm831x-clk",
> +		.owner	= THIS_MODULE,
> +	},
> +};
> +
> +static int __init wm831x_clk_init(void)
> +{
> +	int ret;
> +
> +	ret = platform_driver_register(&wm831x_clk_driver);
> +	if (ret != 0)
> +		pr_err("Failed to register WM831x clock driver: %d\n", ret);
> +
> +	return ret;
> +}

Driver registration is very well implemented and debugged.  Just do
"return platform_driver_register();"

> +module_init(wm831x_clk_init);
> +
> +static void __exit wm831x_clk_exit(void)
> +{
> +	platform_driver_unregister(&wm831x_clk_driver);
> +}
> +module_exit(wm831x_clk_exit);
> +
> +/* Module information */
> +MODULE_AUTHOR("Mark Brown <broonie@opensource.wolfsonmicro.com>");
> +MODULE_DESCRIPTION("WM831x clock driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:wm831x-clk");
> -- 
> 1.7.5.4
> 

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH 6/6] clk: Add initial WM831x clock driver
@ 2011-07-15  2:53       ` Grant Likely
  0 siblings, 0 replies; 72+ messages in thread
From: Grant Likely @ 2011-07-15  2:53 UTC (permalink / raw)
  To: Mark Brown
  Cc: Jeremy Kerr, Grant Likely, linux-kernel, linux-arm-kernel,
	linux-sh, patches

On Mon, Jul 11, 2011 at 11:53:57AM +0900, Mark Brown wrote:
> The WM831x and WM832x series of PMICs contain a flexible clocking
> subsystem intended to provide always on and system core clocks.  It
> features:
> 
> - A 32.768kHz crystal oscillator which can optionally be used to pass
>   through an externally generated clock.
> - A FLL which can be clocked from either the 32.768kHz oscillator or
>   the CLKIN pin.
> - A CLKOUT pin which can bring out either the oscillator or the FLL
>   output.
> - The 32.768kHz clock can also optionally be brought out on the GPIO
>   pins of the device.
> 
> This driver fully supports the 32.768kHz oscillator and CLKOUT.  The FLL
> is supported only in AUTO mode, the full flexibility of the FLL cannot
> currently be used.  The use of clock references other than the internal
> oscillator is not currently supported, and since clk_set_parent() is not
> implemented in the generic clock API the clock tree configuration cannot
> be changed at runtime.
> 
> Due to a lack of access to systems where the core SoC has been converted
> to use the generic clock API this driver has been compile tested only.

Generally seems okay.  Minor comments below.

g.

> 
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> ---
>  MAINTAINERS              |    1 +
>  drivers/clk/Kconfig      |    5 +
>  drivers/clk/Makefile     |    1 +
>  drivers/clk/clk-wm831x.c |  389 ++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 396 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/clk/clk-wm831x.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ae563fa..c234756 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6969,6 +6969,7 @@ T:	git git://opensource.wolfsonmicro.com/linux-2.6-audioplus
>  W:	http://opensource.wolfsonmicro.com/content/linux-drivers-wolfson-devices
>  S:	Supported
>  F:	Documentation/hwmon/wm83??
> +F:	drivers/clk/clk-wm83*.c
>  F:	drivers/leds/leds-wm83*.c
>  F:	drivers/mfd/wm8*.c
>  F:	drivers/power/wm83*.c
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 1fd0070..7f6eec2 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -10,6 +10,7 @@ config GENERIC_CLK_BUILD_TEST
>  	depends on EXPERIMENTAL && GENERIC_CLK
>  	select GENERIC_CLK_FIXED
>  	select GENERIC_CLK_GATE
> +	select GENERIC_CLK_WM831X if MFD_WM831X=y

Hmmm, this could get unwieldy in a hurry.

>  	help
>  	   Enable all possible generic clock drivers.  This is only
>  	   useful for improving build coverage, it is not useful for
> @@ -22,3 +23,7 @@ config GENERIC_CLK_FIXED
>  config GENERIC_CLK_GATE
>  	bool
>  	depends on GENERIC_CLK
> +
> +config GENERIC_CLK_WM831X
> +	tristate
> +	depends on GENERIC_CLK && MFD_WM831X
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index d186446..6628ad5 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -3,3 +3,4 @@ obj-$(CONFIG_CLKDEV_LOOKUP)	+= clkdev.o
>  obj-$(CONFIG_GENERIC_CLK)	+= clk.o
>  obj-$(CONFIG_GENERIC_CLK_FIXED)	+= clk-fixed.o
>  obj-$(CONFIG_GENERIC_CLK_GATE)	+= clk-gate.o
> +obj-$(CONFIG_GENERIC_CLK_WM831X) += clk-wm831x.o
> diff --git a/drivers/clk/clk-wm831x.c b/drivers/clk/clk-wm831x.c
> new file mode 100644
> index 0000000..82782f1
> --- /dev/null
> +++ b/drivers/clk/clk-wm831x.c
> @@ -0,0 +1,389 @@
> +/*
> + * WM831x clock control
> + *
> + * Copyright 2011 Wolfson Microelectronics PLC.
> + *
> + * Author: Mark Brown <broonie@opensource.wolfsonmicro.com>
> + *
> + *  This program is free software; you can redistribute  it and/or modify it
> + *  under  the terms of  the GNU General  Public License as published by the
> + *  Free Software Foundation;  either version 2 of the  License, or (at your
> + *  option) any later version.
> + *
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/platform_device.h>
> +#include <linux/mfd/wm831x/core.h>
> +
> +struct wm831x_clk {
> +	struct wm831x *wm831x;
> +	struct clk_hw xtal_hw;
> +	struct clk_hw fll_hw;
> +	struct clk_hw clkout_hw;
> +	bool xtal_ena;
> +};
> +
> +static int wm831x_xtal_enable(struct clk_hw *hw)
> +{
> +	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
> +						  xtal_hw);

This container of is used 10 times.  A static inline would be
reasonable.

> +
> +	if (clkdata->xtal_ena)
> +		return 0;
> +	else
> +		return -EPERM;
> +}

Nit: return clkdata->xtal_ena ? 0 : -EPERM;

Just makes for more concise code.

> +
> +static unsigned long wm831x_xtal_recalc_rate(struct clk_hw *hw)
> +{
> +	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
> +						  xtal_hw);
> +
> +	if (clkdata->xtal_ena)
> +		return 32768;
> +	else
> +		return 0;
> +}
> +
> +static long wm831x_xtal_round_rate(struct clk_hw *hw, unsigned long rate)
> +{
> +	return wm831x_xtal_recalc_rate(hw);
> +}
> +
> +static const struct clk_hw_ops wm831x_xtal_ops = {
> +	.enable = wm831x_xtal_enable,
> +	.recalc_rate = wm831x_xtal_recalc_rate,
> +	.round_rate = wm831x_xtal_round_rate,
> +};
> +
> +static const unsigned long wm831x_fll_auto_rates[] = {
> +	 2048000,
> +	11289600,
> +	12000000,
> +	12288000,
> +	19200000,
> +	22579600,
> +	24000000,
> +	24576000,
> +};
> +
> +static bool wm831x_fll_enabled(struct wm831x *wm831x)
> +{
> +	int ret;
> +
> +	ret = wm831x_reg_read(wm831x, WM831X_FLL_CONTROL_1);
> +	if (ret < 0) {
> +		dev_err(wm831x->dev, "Unable to read FLL_CONTROL_1: %d\n",
> +			ret);
> +		return true;
> +	}
> +
> +	if (ret & WM831X_FLL_ENA)
> +		return true;
> +	else
> +		return false;

similarly, return (ret & WM831X_FLL_ENA) != 0;

> +}
> +
> +static int wm831x_fll_prepare(struct clk_hw *hw)
> +{
> +	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
> +						  fll_hw);
> +	struct wm831x *wm831x = clkdata->wm831x;
> +	int ret;
> +
> +	ret = wm831x_set_bits(wm831x, WM831X_FLL_CONTROL_2,
> +			      WM831X_FLL_ENA, WM831X_FLL_ENA);
> +	if (ret != 0)
> +		dev_crit(wm831x->dev, "Failed to enable FLL: %d\n", ret);
> +
> +	return ret;
> +}
> +
> +static void wm831x_fll_unprepare(struct clk_hw *hw)
> +{
> +	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
> +						  fll_hw);
> +	struct wm831x *wm831x = clkdata->wm831x;
> +	int ret;
> +
> +	ret = wm831x_set_bits(wm831x, WM831X_FLL_CONTROL_2, WM831X_FLL_ENA, 0);
> +	if (ret != 0)
> +		dev_crit(wm831x->dev, "Failed to disaable FLL: %d\n", ret);
> +}
> +
> +static unsigned long wm831x_fll_recalc_rate(struct clk_hw *hw)
> +{
> +	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
> +						  fll_hw);
> +	struct wm831x *wm831x = clkdata->wm831x;
> +	int ret;
> +
> +	ret = wm831x_reg_read(wm831x, WM831X_CLOCK_CONTROL_2);
> +	if (ret < 0) {
> +		dev_err(wm831x->dev, "Unable to read CLOCK_CONTROL_2: %d\n",
> +			ret);
> +		return 0;
> +	}
> +
> +	if (ret & WM831X_FLL_AUTO)
> +		return wm831x_fll_auto_rates[ret & WM831X_FLL_AUTO_FREQ_MASK];
> +
> +	dev_err(wm831x->dev, "FLL only supported in AUTO mode\n");
> +	return 0;
> +}
> +
> +static int wm831x_fll_set_rate(struct clk_hw *hw, unsigned long rate,
> +			       unsigned long *parent_rate)
> +{
> +	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
> +						  fll_hw);
> +	struct wm831x *wm831x = clkdata->wm831x;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(wm831x_fll_auto_rates); i++)
> +		if (wm831x_fll_auto_rates[i] == rate)
> +			break;
> +	if (i == ARRAY_SIZE(wm831x_fll_auto_rates))
> +		return -EINVAL;
> +
> +	if (wm831x_fll_enabled(wm831x))
> +		return -EPERM;
> +
> +	return wm831x_set_bits(wm831x, WM831X_CLOCK_CONTROL_2,
> +			       WM831X_FLL_AUTO_FREQ_MASK, i);
> +}
> +
> +static struct clk *wm831x_fll_get_parent(struct clk_hw *hw)
> +{
> +	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
> +						  fll_hw);
> +	struct wm831x *wm831x = clkdata->wm831x;
> +	int ret;
> +
> +	/* AUTO mode is always clocked from the crystal */
> +	ret = wm831x_reg_read(wm831x, WM831X_CLOCK_CONTROL_2);
> +	if (ret < 0) {
> +		dev_err(wm831x->dev, "Unable to read CLOCK_CONTROL_2: %d\n",
> +			ret);
> +		return NULL;
> +	}
> +
> +	if (ret & WM831X_FLL_AUTO)
> +		return clkdata->xtal_hw.clk;
> +
> +	ret = wm831x_reg_read(wm831x, WM831X_FLL_CONTROL_5);
> +	if (ret < 0) {
> +		dev_err(wm831x->dev, "Unable to read FLL_CONTROL_5: %d\n",
> +			ret);
> +		return NULL;
> +	}
> +
> +	switch (ret & WM831X_FLL_CLK_SRC_MASK) {
> +	case 0:
> +		return clkdata->xtal_hw.clk;
> +	case 1:
> +		dev_warn(wm831x->dev,
> +			 "FLL clocked from CLKIN not yet supported\n");
> +		return NULL;
> +	default:
> +		dev_err(wm831x->dev, "Unsupported FLL clock source %d\n",
> +			ret & WM831X_FLL_CLK_SRC_MASK);
> +		return NULL;
> +	}
> +}
> +
> +static const struct clk_hw_ops wm831x_fll_ops = {
> +	.prepare = wm831x_fll_prepare,
> +	.unprepare = wm831x_fll_unprepare,
> +	.recalc_rate = wm831x_fll_recalc_rate,
> +	.set_rate = wm831x_fll_set_rate,
> +	.get_parent = wm831x_fll_get_parent,
> +};
> +
> +static int wm831x_clkout_prepare(struct clk_hw *hw)
> +{
> +	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
> +						  clkout_hw);
> +	struct wm831x *wm831x = clkdata->wm831x;
> +	int ret;
> +
> +	ret = wm831x_reg_unlock(wm831x);
> +	if (ret != 0) {
> +		dev_crit(wm831x->dev, "Failed to lock registers: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = wm831x_set_bits(wm831x, WM831X_CLOCK_CONTROL_1,
> +			      WM831X_CLKOUT_ENA, WM831X_CLKOUT_ENA);
> +	if (ret != 0)
> +		dev_crit(wm831x->dev, "Failed to enable CLKOUT: %d\n", ret);
> +
> +	wm831x_reg_lock(wm831x);
> +
> +	return ret;
> +}
> +
> +static void wm831x_clkout_unprepare(struct clk_hw *hw)
> +{
> +	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
> +						  clkout_hw);
> +	struct wm831x *wm831x = clkdata->wm831x;
> +	int ret;
> +
> +	ret = wm831x_reg_unlock(wm831x);
> +	if (ret != 0) {
> +		dev_crit(wm831x->dev, "Failed to lock registers: %d\n", ret);
> +		return;
> +	}
> +
> +	ret = wm831x_set_bits(wm831x, WM831X_CLOCK_CONTROL_1,
> +			      WM831X_CLKOUT_ENA, 0);
> +	if (ret != 0)
> +		dev_crit(wm831x->dev, "Failed to disable CLKOUT: %d\n", ret);
> +
> +	wm831x_reg_lock(wm831x);
> +}
> +
> +static unsigned long wm831x_clkout_recalc_rate(struct clk_hw *hw)
> +{
> +	return clk_get_rate(clk_get_parent(hw->clk));
> +}
> +
> +static long wm831x_clkout_round_rate(struct clk_hw *hw, unsigned long rate)
> +{
> +	return clk_round_rate(clk_get_parent(hw->clk), rate);
> +}
> +
> +static int wm831x_clkout_set_rate(struct clk_hw *hw, unsigned long rate,
> +				  unsigned long *parent_rate)
> +{
> +	*parent_rate = rate;
> +	return CLK_SET_RATE_PROPAGATE;
> +}
> +
> +static struct clk *wm831x_clkout_get_parent(struct clk_hw *hw)
> +{
> +	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
> +						  clkout_hw);
> +	struct wm831x *wm831x = clkdata->wm831x;
> +	int ret;
> +
> +	ret = wm831x_reg_read(wm831x, WM831X_CLOCK_CONTROL_1);
> +	if (ret < 0) {
> +		dev_err(wm831x->dev, "Unable to read CLOCK_CONTROL_1: %d\n",
> +			ret);
> +		return NULL;
> +	}
> +
> +	if (ret & WM831X_CLKOUT_SRC)
> +		return clkdata->xtal_hw.clk;
> +	else
> +		return clkdata->fll_hw.clk;
> +}
> +
> +static const struct clk_hw_ops wm831x_clkout_ops = {
> +	.prepare = wm831x_clkout_prepare,
> +	.unprepare = wm831x_clkout_unprepare,
> +	.recalc_rate = wm831x_clkout_recalc_rate,
> +	.round_rate = wm831x_clkout_round_rate,
> +	.set_rate = wm831x_clkout_set_rate,
> +	.get_parent = wm831x_clkout_get_parent,
> +};
> +
> +static __devinit int wm831x_clk_probe(struct platform_device *pdev)
> +{
> +	struct wm831x *wm831x = dev_get_drvdata(pdev->dev.parent);
> +	struct wm831x_clk *clkdata;
> +	int ret;
> +
> +	clkdata = kzalloc(sizeof(*clkdata), GFP_KERNEL);
> +	if (!clkdata)
> +		return -ENOMEM;
> +
> +	/* XTAL_ENA can only be set via OTP/InstantConfig so just read once */
> +	ret = wm831x_reg_read(wm831x, WM831X_CLOCK_CONTROL_2);
> +	if (ret < 0) {
> +		dev_err(wm831x->dev, "Unable to read CLOCK_CONTROL_2: %d\n",
> +			ret);
> +		goto err_alloc;
> +	}
> +	clkdata->xtal_ena = ret & WM831X_XTAL_ENA;
> +
> +	if (!clk_register(wm831x->dev, &wm831x_xtal_ops, &clkdata->xtal_hw,
> +			  "xtal")) {
> +		ret = -EINVAL;
> +		goto err_alloc;
> +	}
> +
> +	if (!clk_register(wm831x->dev, &wm831x_fll_ops, &clkdata->fll_hw,
> +			  "fll")) {
> +		ret = -EINVAL;
> +		goto err_xtal;
> +	}
> +
> +	if (!clk_register(wm831x->dev, &wm831x_clkout_ops, &clkdata->clkout_hw,
> +			  "clkout")) {
> +		ret = -EINVAL;
> +		goto err_fll;
> +	}

How common will this pattern be?  Is there need for a
clk_register_many() variant?

> +
> +	dev_set_drvdata(&pdev->dev, clkdata);
> +
> +	return 0;
> +
> +err_fll:
> +	clk_unregister(clkdata->fll_hw.clk);
> +err_xtal:
> +	clk_unregister(clkdata->xtal_hw.clk);
> +err_alloc:
> +	kfree(clkdata);
> +	return ret;
> +}
> +
> +static __devexit int wm831x_clk_remove(struct platform_device *pdev)
> +{
> +	struct wm831x_clk *clkdata = dev_get_drvdata(&pdev->dev);
> +
> +	clk_unregister(clkdata->clkout_hw.clk);
> +	clk_unregister(clkdata->fll_hw.clk);
> +	clk_unregister(clkdata->xtal_hw.clk);
> +	kfree(clkdata);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver wm831x_clk_driver = {
> +	.probe = wm831x_clk_probe,
> +	.remove = __devexit_p(wm831x_clk_remove),
> +	.driver		= {
> +		.name	= "wm831x-clk",
> +		.owner	= THIS_MODULE,
> +	},
> +};
> +
> +static int __init wm831x_clk_init(void)
> +{
> +	int ret;
> +
> +	ret = platform_driver_register(&wm831x_clk_driver);
> +	if (ret != 0)
> +		pr_err("Failed to register WM831x clock driver: %d\n", ret);
> +
> +	return ret;
> +}

Driver registration is very well implemented and debugged.  Just do
"return platform_driver_register();"

> +module_init(wm831x_clk_init);
> +
> +static void __exit wm831x_clk_exit(void)
> +{
> +	platform_driver_unregister(&wm831x_clk_driver);
> +}
> +module_exit(wm831x_clk_exit);
> +
> +/* Module information */
> +MODULE_AUTHOR("Mark Brown <broonie@opensource.wolfsonmicro.com>");
> +MODULE_DESCRIPTION("WM831x clock driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:wm831x-clk");
> -- 
> 1.7.5.4
> 

^ permalink raw reply	[flat|nested] 72+ messages in thread

* [PATCH 6/6] clk: Add initial WM831x clock driver
@ 2011-07-15  2:53       ` Grant Likely
  0 siblings, 0 replies; 72+ messages in thread
From: Grant Likely @ 2011-07-15  2:53 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Jul 11, 2011 at 11:53:57AM +0900, Mark Brown wrote:
> The WM831x and WM832x series of PMICs contain a flexible clocking
> subsystem intended to provide always on and system core clocks.  It
> features:
> 
> - A 32.768kHz crystal oscillator which can optionally be used to pass
>   through an externally generated clock.
> - A FLL which can be clocked from either the 32.768kHz oscillator or
>   the CLKIN pin.
> - A CLKOUT pin which can bring out either the oscillator or the FLL
>   output.
> - The 32.768kHz clock can also optionally be brought out on the GPIO
>   pins of the device.
> 
> This driver fully supports the 32.768kHz oscillator and CLKOUT.  The FLL
> is supported only in AUTO mode, the full flexibility of the FLL cannot
> currently be used.  The use of clock references other than the internal
> oscillator is not currently supported, and since clk_set_parent() is not
> implemented in the generic clock API the clock tree configuration cannot
> be changed at runtime.
> 
> Due to a lack of access to systems where the core SoC has been converted
> to use the generic clock API this driver has been compile tested only.

Generally seems okay.  Minor comments below.

g.

> 
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> ---
>  MAINTAINERS              |    1 +
>  drivers/clk/Kconfig      |    5 +
>  drivers/clk/Makefile     |    1 +
>  drivers/clk/clk-wm831x.c |  389 ++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 396 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/clk/clk-wm831x.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ae563fa..c234756 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6969,6 +6969,7 @@ T:	git git://opensource.wolfsonmicro.com/linux-2.6-audioplus
>  W:	http://opensource.wolfsonmicro.com/content/linux-drivers-wolfson-devices
>  S:	Supported
>  F:	Documentation/hwmon/wm83??
> +F:	drivers/clk/clk-wm83*.c
>  F:	drivers/leds/leds-wm83*.c
>  F:	drivers/mfd/wm8*.c
>  F:	drivers/power/wm83*.c
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 1fd0070..7f6eec2 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -10,6 +10,7 @@ config GENERIC_CLK_BUILD_TEST
>  	depends on EXPERIMENTAL && GENERIC_CLK
>  	select GENERIC_CLK_FIXED
>  	select GENERIC_CLK_GATE
> +	select GENERIC_CLK_WM831X if MFD_WM831X=y

Hmmm, this could get unwieldy in a hurry.

>  	help
>  	   Enable all possible generic clock drivers.  This is only
>  	   useful for improving build coverage, it is not useful for
> @@ -22,3 +23,7 @@ config GENERIC_CLK_FIXED
>  config GENERIC_CLK_GATE
>  	bool
>  	depends on GENERIC_CLK
> +
> +config GENERIC_CLK_WM831X
> +	tristate
> +	depends on GENERIC_CLK && MFD_WM831X
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index d186446..6628ad5 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -3,3 +3,4 @@ obj-$(CONFIG_CLKDEV_LOOKUP)	+= clkdev.o
>  obj-$(CONFIG_GENERIC_CLK)	+= clk.o
>  obj-$(CONFIG_GENERIC_CLK_FIXED)	+= clk-fixed.o
>  obj-$(CONFIG_GENERIC_CLK_GATE)	+= clk-gate.o
> +obj-$(CONFIG_GENERIC_CLK_WM831X) += clk-wm831x.o
> diff --git a/drivers/clk/clk-wm831x.c b/drivers/clk/clk-wm831x.c
> new file mode 100644
> index 0000000..82782f1
> --- /dev/null
> +++ b/drivers/clk/clk-wm831x.c
> @@ -0,0 +1,389 @@
> +/*
> + * WM831x clock control
> + *
> + * Copyright 2011 Wolfson Microelectronics PLC.
> + *
> + * Author: Mark Brown <broonie@opensource.wolfsonmicro.com>
> + *
> + *  This program is free software; you can redistribute  it and/or modify it
> + *  under  the terms of  the GNU General  Public License as published by the
> + *  Free Software Foundation;  either version 2 of the  License, or (at your
> + *  option) any later version.
> + *
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/platform_device.h>
> +#include <linux/mfd/wm831x/core.h>
> +
> +struct wm831x_clk {
> +	struct wm831x *wm831x;
> +	struct clk_hw xtal_hw;
> +	struct clk_hw fll_hw;
> +	struct clk_hw clkout_hw;
> +	bool xtal_ena;
> +};
> +
> +static int wm831x_xtal_enable(struct clk_hw *hw)
> +{
> +	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
> +						  xtal_hw);

This container of is used 10 times.  A static inline would be
reasonable.

> +
> +	if (clkdata->xtal_ena)
> +		return 0;
> +	else
> +		return -EPERM;
> +}

Nit: return clkdata->xtal_ena ? 0 : -EPERM;

Just makes for more concise code.

> +
> +static unsigned long wm831x_xtal_recalc_rate(struct clk_hw *hw)
> +{
> +	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
> +						  xtal_hw);
> +
> +	if (clkdata->xtal_ena)
> +		return 32768;
> +	else
> +		return 0;
> +}
> +
> +static long wm831x_xtal_round_rate(struct clk_hw *hw, unsigned long rate)
> +{
> +	return wm831x_xtal_recalc_rate(hw);
> +}
> +
> +static const struct clk_hw_ops wm831x_xtal_ops = {
> +	.enable = wm831x_xtal_enable,
> +	.recalc_rate = wm831x_xtal_recalc_rate,
> +	.round_rate = wm831x_xtal_round_rate,
> +};
> +
> +static const unsigned long wm831x_fll_auto_rates[] = {
> +	 2048000,
> +	11289600,
> +	12000000,
> +	12288000,
> +	19200000,
> +	22579600,
> +	24000000,
> +	24576000,
> +};
> +
> +static bool wm831x_fll_enabled(struct wm831x *wm831x)
> +{
> +	int ret;
> +
> +	ret = wm831x_reg_read(wm831x, WM831X_FLL_CONTROL_1);
> +	if (ret < 0) {
> +		dev_err(wm831x->dev, "Unable to read FLL_CONTROL_1: %d\n",
> +			ret);
> +		return true;
> +	}
> +
> +	if (ret & WM831X_FLL_ENA)
> +		return true;
> +	else
> +		return false;

similarly, return (ret & WM831X_FLL_ENA) != 0;

> +}
> +
> +static int wm831x_fll_prepare(struct clk_hw *hw)
> +{
> +	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
> +						  fll_hw);
> +	struct wm831x *wm831x = clkdata->wm831x;
> +	int ret;
> +
> +	ret = wm831x_set_bits(wm831x, WM831X_FLL_CONTROL_2,
> +			      WM831X_FLL_ENA, WM831X_FLL_ENA);
> +	if (ret != 0)
> +		dev_crit(wm831x->dev, "Failed to enable FLL: %d\n", ret);
> +
> +	return ret;
> +}
> +
> +static void wm831x_fll_unprepare(struct clk_hw *hw)
> +{
> +	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
> +						  fll_hw);
> +	struct wm831x *wm831x = clkdata->wm831x;
> +	int ret;
> +
> +	ret = wm831x_set_bits(wm831x, WM831X_FLL_CONTROL_2, WM831X_FLL_ENA, 0);
> +	if (ret != 0)
> +		dev_crit(wm831x->dev, "Failed to disaable FLL: %d\n", ret);
> +}
> +
> +static unsigned long wm831x_fll_recalc_rate(struct clk_hw *hw)
> +{
> +	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
> +						  fll_hw);
> +	struct wm831x *wm831x = clkdata->wm831x;
> +	int ret;
> +
> +	ret = wm831x_reg_read(wm831x, WM831X_CLOCK_CONTROL_2);
> +	if (ret < 0) {
> +		dev_err(wm831x->dev, "Unable to read CLOCK_CONTROL_2: %d\n",
> +			ret);
> +		return 0;
> +	}
> +
> +	if (ret & WM831X_FLL_AUTO)
> +		return wm831x_fll_auto_rates[ret & WM831X_FLL_AUTO_FREQ_MASK];
> +
> +	dev_err(wm831x->dev, "FLL only supported in AUTO mode\n");
> +	return 0;
> +}
> +
> +static int wm831x_fll_set_rate(struct clk_hw *hw, unsigned long rate,
> +			       unsigned long *parent_rate)
> +{
> +	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
> +						  fll_hw);
> +	struct wm831x *wm831x = clkdata->wm831x;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(wm831x_fll_auto_rates); i++)
> +		if (wm831x_fll_auto_rates[i] == rate)
> +			break;
> +	if (i == ARRAY_SIZE(wm831x_fll_auto_rates))
> +		return -EINVAL;
> +
> +	if (wm831x_fll_enabled(wm831x))
> +		return -EPERM;
> +
> +	return wm831x_set_bits(wm831x, WM831X_CLOCK_CONTROL_2,
> +			       WM831X_FLL_AUTO_FREQ_MASK, i);
> +}
> +
> +static struct clk *wm831x_fll_get_parent(struct clk_hw *hw)
> +{
> +	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
> +						  fll_hw);
> +	struct wm831x *wm831x = clkdata->wm831x;
> +	int ret;
> +
> +	/* AUTO mode is always clocked from the crystal */
> +	ret = wm831x_reg_read(wm831x, WM831X_CLOCK_CONTROL_2);
> +	if (ret < 0) {
> +		dev_err(wm831x->dev, "Unable to read CLOCK_CONTROL_2: %d\n",
> +			ret);
> +		return NULL;
> +	}
> +
> +	if (ret & WM831X_FLL_AUTO)
> +		return clkdata->xtal_hw.clk;
> +
> +	ret = wm831x_reg_read(wm831x, WM831X_FLL_CONTROL_5);
> +	if (ret < 0) {
> +		dev_err(wm831x->dev, "Unable to read FLL_CONTROL_5: %d\n",
> +			ret);
> +		return NULL;
> +	}
> +
> +	switch (ret & WM831X_FLL_CLK_SRC_MASK) {
> +	case 0:
> +		return clkdata->xtal_hw.clk;
> +	case 1:
> +		dev_warn(wm831x->dev,
> +			 "FLL clocked from CLKIN not yet supported\n");
> +		return NULL;
> +	default:
> +		dev_err(wm831x->dev, "Unsupported FLL clock source %d\n",
> +			ret & WM831X_FLL_CLK_SRC_MASK);
> +		return NULL;
> +	}
> +}
> +
> +static const struct clk_hw_ops wm831x_fll_ops = {
> +	.prepare = wm831x_fll_prepare,
> +	.unprepare = wm831x_fll_unprepare,
> +	.recalc_rate = wm831x_fll_recalc_rate,
> +	.set_rate = wm831x_fll_set_rate,
> +	.get_parent = wm831x_fll_get_parent,
> +};
> +
> +static int wm831x_clkout_prepare(struct clk_hw *hw)
> +{
> +	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
> +						  clkout_hw);
> +	struct wm831x *wm831x = clkdata->wm831x;
> +	int ret;
> +
> +	ret = wm831x_reg_unlock(wm831x);
> +	if (ret != 0) {
> +		dev_crit(wm831x->dev, "Failed to lock registers: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = wm831x_set_bits(wm831x, WM831X_CLOCK_CONTROL_1,
> +			      WM831X_CLKOUT_ENA, WM831X_CLKOUT_ENA);
> +	if (ret != 0)
> +		dev_crit(wm831x->dev, "Failed to enable CLKOUT: %d\n", ret);
> +
> +	wm831x_reg_lock(wm831x);
> +
> +	return ret;
> +}
> +
> +static void wm831x_clkout_unprepare(struct clk_hw *hw)
> +{
> +	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
> +						  clkout_hw);
> +	struct wm831x *wm831x = clkdata->wm831x;
> +	int ret;
> +
> +	ret = wm831x_reg_unlock(wm831x);
> +	if (ret != 0) {
> +		dev_crit(wm831x->dev, "Failed to lock registers: %d\n", ret);
> +		return;
> +	}
> +
> +	ret = wm831x_set_bits(wm831x, WM831X_CLOCK_CONTROL_1,
> +			      WM831X_CLKOUT_ENA, 0);
> +	if (ret != 0)
> +		dev_crit(wm831x->dev, "Failed to disable CLKOUT: %d\n", ret);
> +
> +	wm831x_reg_lock(wm831x);
> +}
> +
> +static unsigned long wm831x_clkout_recalc_rate(struct clk_hw *hw)
> +{
> +	return clk_get_rate(clk_get_parent(hw->clk));
> +}
> +
> +static long wm831x_clkout_round_rate(struct clk_hw *hw, unsigned long rate)
> +{
> +	return clk_round_rate(clk_get_parent(hw->clk), rate);
> +}
> +
> +static int wm831x_clkout_set_rate(struct clk_hw *hw, unsigned long rate,
> +				  unsigned long *parent_rate)
> +{
> +	*parent_rate = rate;
> +	return CLK_SET_RATE_PROPAGATE;
> +}
> +
> +static struct clk *wm831x_clkout_get_parent(struct clk_hw *hw)
> +{
> +	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
> +						  clkout_hw);
> +	struct wm831x *wm831x = clkdata->wm831x;
> +	int ret;
> +
> +	ret = wm831x_reg_read(wm831x, WM831X_CLOCK_CONTROL_1);
> +	if (ret < 0) {
> +		dev_err(wm831x->dev, "Unable to read CLOCK_CONTROL_1: %d\n",
> +			ret);
> +		return NULL;
> +	}
> +
> +	if (ret & WM831X_CLKOUT_SRC)
> +		return clkdata->xtal_hw.clk;
> +	else
> +		return clkdata->fll_hw.clk;
> +}
> +
> +static const struct clk_hw_ops wm831x_clkout_ops = {
> +	.prepare = wm831x_clkout_prepare,
> +	.unprepare = wm831x_clkout_unprepare,
> +	.recalc_rate = wm831x_clkout_recalc_rate,
> +	.round_rate = wm831x_clkout_round_rate,
> +	.set_rate = wm831x_clkout_set_rate,
> +	.get_parent = wm831x_clkout_get_parent,
> +};
> +
> +static __devinit int wm831x_clk_probe(struct platform_device *pdev)
> +{
> +	struct wm831x *wm831x = dev_get_drvdata(pdev->dev.parent);
> +	struct wm831x_clk *clkdata;
> +	int ret;
> +
> +	clkdata = kzalloc(sizeof(*clkdata), GFP_KERNEL);
> +	if (!clkdata)
> +		return -ENOMEM;
> +
> +	/* XTAL_ENA can only be set via OTP/InstantConfig so just read once */
> +	ret = wm831x_reg_read(wm831x, WM831X_CLOCK_CONTROL_2);
> +	if (ret < 0) {
> +		dev_err(wm831x->dev, "Unable to read CLOCK_CONTROL_2: %d\n",
> +			ret);
> +		goto err_alloc;
> +	}
> +	clkdata->xtal_ena = ret & WM831X_XTAL_ENA;
> +
> +	if (!clk_register(wm831x->dev, &wm831x_xtal_ops, &clkdata->xtal_hw,
> +			  "xtal")) {
> +		ret = -EINVAL;
> +		goto err_alloc;
> +	}
> +
> +	if (!clk_register(wm831x->dev, &wm831x_fll_ops, &clkdata->fll_hw,
> +			  "fll")) {
> +		ret = -EINVAL;
> +		goto err_xtal;
> +	}
> +
> +	if (!clk_register(wm831x->dev, &wm831x_clkout_ops, &clkdata->clkout_hw,
> +			  "clkout")) {
> +		ret = -EINVAL;
> +		goto err_fll;
> +	}

How common will this pattern be?  Is there need for a
clk_register_many() variant?

> +
> +	dev_set_drvdata(&pdev->dev, clkdata);
> +
> +	return 0;
> +
> +err_fll:
> +	clk_unregister(clkdata->fll_hw.clk);
> +err_xtal:
> +	clk_unregister(clkdata->xtal_hw.clk);
> +err_alloc:
> +	kfree(clkdata);
> +	return ret;
> +}
> +
> +static __devexit int wm831x_clk_remove(struct platform_device *pdev)
> +{
> +	struct wm831x_clk *clkdata = dev_get_drvdata(&pdev->dev);
> +
> +	clk_unregister(clkdata->clkout_hw.clk);
> +	clk_unregister(clkdata->fll_hw.clk);
> +	clk_unregister(clkdata->xtal_hw.clk);
> +	kfree(clkdata);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver wm831x_clk_driver = {
> +	.probe = wm831x_clk_probe,
> +	.remove = __devexit_p(wm831x_clk_remove),
> +	.driver		= {
> +		.name	= "wm831x-clk",
> +		.owner	= THIS_MODULE,
> +	},
> +};
> +
> +static int __init wm831x_clk_init(void)
> +{
> +	int ret;
> +
> +	ret = platform_driver_register(&wm831x_clk_driver);
> +	if (ret != 0)
> +		pr_err("Failed to register WM831x clock driver: %d\n", ret);
> +
> +	return ret;
> +}

Driver registration is very well implemented and debugged.  Just do
"return platform_driver_register();"

> +module_init(wm831x_clk_init);
> +
> +static void __exit wm831x_clk_exit(void)
> +{
> +	platform_driver_unregister(&wm831x_clk_driver);
> +}
> +module_exit(wm831x_clk_exit);
> +
> +/* Module information */
> +MODULE_AUTHOR("Mark Brown <broonie@opensource.wolfsonmicro.com>");
> +MODULE_DESCRIPTION("WM831x clock driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:wm831x-clk");
> -- 
> 1.7.5.4
> 

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH 6/6] clk: Add initial WM831x clock driver
  2011-07-15  2:53       ` Grant Likely
  (?)
@ 2011-07-15  5:05         ` Mark Brown
  -1 siblings, 0 replies; 72+ messages in thread
From: Mark Brown @ 2011-07-15  5:05 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Jul 14, 2011 at 08:53:39PM -0600, Grant Likely wrote:
> On Mon, Jul 11, 2011 at 11:53:57AM +0900, Mark Brown wrote:

> > @@ -10,6 +10,7 @@ config GENERIC_CLK_BUILD_TEST
> >  	depends on EXPERIMENTAL && GENERIC_CLK
> >  	select GENERIC_CLK_FIXED
> >  	select GENERIC_CLK_GATE
> > +	select GENERIC_CLK_WM831X if MFD_WM831X=y

> Hmmm, this could get unwieldy in a hurry.

It's not really any hassle, we've got a one of these in ASoC.  The list
gets long but if you keep it sorted it's not an issue for merges and
otherwise it's just long not complicated.  The ability to get build
coverage is *really* useful.

> > +static int wm831x_xtal_enable(struct clk_hw *hw)
> > +{
> > +	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
> > +						  xtal_hw);

> This container of is used 10 times.  A static inline would be
> reasonable.

Not quite - it's used in three different variants (one for each of the
clocks).

> > +	if (clkdata->xtal_ena)
> > +		return 0;
> > +	else
> > +		return -EPERM;
> > +}

> Nit: return clkdata->xtal_ena ? 0 : -EPERM;

> Just makes for more concise code.

I have an extremely strong dislike of the ternery operator, I find it
does nothing for legibility.

> > +	if (!clk_register(wm831x->dev, &wm831x_clkout_ops, &clkdata->clkout_hw,
> > +			  "clkout")) {
> > +		ret = -EINVAL;
> > +		goto err_fll;
> > +	}

> How common will this pattern be?  Is there need for a
> clk_register_many() variant?

I dunno, I think a lot of the SoCs may be doing one clk per device.  But
equally well it's not like it'd be hard if someone starts working on the
API again.

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH 6/6] clk: Add initial WM831x clock driver
@ 2011-07-15  5:05         ` Mark Brown
  0 siblings, 0 replies; 72+ messages in thread
From: Mark Brown @ 2011-07-15  5:05 UTC (permalink / raw)
  To: Grant Likely
  Cc: Jeremy Kerr, Grant Likely, linux-kernel, linux-arm-kernel,
	linux-sh, patches

On Thu, Jul 14, 2011 at 08:53:39PM -0600, Grant Likely wrote:
> On Mon, Jul 11, 2011 at 11:53:57AM +0900, Mark Brown wrote:

> > @@ -10,6 +10,7 @@ config GENERIC_CLK_BUILD_TEST
> >  	depends on EXPERIMENTAL && GENERIC_CLK
> >  	select GENERIC_CLK_FIXED
> >  	select GENERIC_CLK_GATE
> > +	select GENERIC_CLK_WM831X if MFD_WM831X=y

> Hmmm, this could get unwieldy in a hurry.

It's not really any hassle, we've got a one of these in ASoC.  The list
gets long but if you keep it sorted it's not an issue for merges and
otherwise it's just long not complicated.  The ability to get build
coverage is *really* useful.

> > +static int wm831x_xtal_enable(struct clk_hw *hw)
> > +{
> > +	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
> > +						  xtal_hw);

> This container of is used 10 times.  A static inline would be
> reasonable.

Not quite - it's used in three different variants (one for each of the
clocks).

> > +	if (clkdata->xtal_ena)
> > +		return 0;
> > +	else
> > +		return -EPERM;
> > +}

> Nit: return clkdata->xtal_ena ? 0 : -EPERM;

> Just makes for more concise code.

I have an extremely strong dislike of the ternery operator, I find it
does nothing for legibility.

> > +	if (!clk_register(wm831x->dev, &wm831x_clkout_ops, &clkdata->clkout_hw,
> > +			  "clkout")) {
> > +		ret = -EINVAL;
> > +		goto err_fll;
> > +	}

> How common will this pattern be?  Is there need for a
> clk_register_many() variant?

I dunno, I think a lot of the SoCs may be doing one clk per device.  But
equally well it's not like it'd be hard if someone starts working on the
API again.

^ permalink raw reply	[flat|nested] 72+ messages in thread

* [PATCH 6/6] clk: Add initial WM831x clock driver
@ 2011-07-15  5:05         ` Mark Brown
  0 siblings, 0 replies; 72+ messages in thread
From: Mark Brown @ 2011-07-15  5:05 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Jul 14, 2011 at 08:53:39PM -0600, Grant Likely wrote:
> On Mon, Jul 11, 2011 at 11:53:57AM +0900, Mark Brown wrote:

> > @@ -10,6 +10,7 @@ config GENERIC_CLK_BUILD_TEST
> >  	depends on EXPERIMENTAL && GENERIC_CLK
> >  	select GENERIC_CLK_FIXED
> >  	select GENERIC_CLK_GATE
> > +	select GENERIC_CLK_WM831X if MFD_WM831X=y

> Hmmm, this could get unwieldy in a hurry.

It's not really any hassle, we've got a one of these in ASoC.  The list
gets long but if you keep it sorted it's not an issue for merges and
otherwise it's just long not complicated.  The ability to get build
coverage is *really* useful.

> > +static int wm831x_xtal_enable(struct clk_hw *hw)
> > +{
> > +	struct wm831x_clk *clkdata = container_of(hw, struct wm831x_clk,
> > +						  xtal_hw);

> This container of is used 10 times.  A static inline would be
> reasonable.

Not quite - it's used in three different variants (one for each of the
clocks).

> > +	if (clkdata->xtal_ena)
> > +		return 0;
> > +	else
> > +		return -EPERM;
> > +}

> Nit: return clkdata->xtal_ena ? 0 : -EPERM;

> Just makes for more concise code.

I have an extremely strong dislike of the ternery operator, I find it
does nothing for legibility.

> > +	if (!clk_register(wm831x->dev, &wm831x_clkout_ops, &clkdata->clkout_hw,
> > +			  "clkout")) {
> > +		ret = -EINVAL;
> > +		goto err_fll;
> > +	}

> How common will this pattern be?  Is there need for a
> clk_register_many() variant?

I dunno, I think a lot of the SoCs may be doing one clk per device.  But
equally well it's not like it'd be hard if someone starts working on the
API again.

^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH 6/6] clk: Add initial WM831x clock driver
  2011-07-15  5:05         ` Mark Brown
  (?)
@ 2011-07-15  5:14           ` Ryan Mallon
  -1 siblings, 0 replies; 72+ messages in thread
From: Ryan Mallon @ 2011-07-15  5:14 UTC (permalink / raw)
  To: linux-arm-kernel

On 15/07/11 15:05, Mark Brown wrote:
> On Thu, Jul 14, 2011 at 08:53:39PM -0600, Grant Likely wrote:
>> On Mon, Jul 11, 2011 at 11:53:57AM +0900, Mark Brown wrote:
>>> +	if (clkdata->xtal_ena)
>>> +		return 0;
>>> +	else
>>> +		return -EPERM;
>>> +}
>> Nit: return clkdata->xtal_ena ? 0 : -EPERM;
>> Just makes for more concise code.
> I have an extremely strong dislike of the ternery operator, I find it
> does nothing for legibility.
>
I prefer this:

     if (!clkdata->xtal_ena)
         return -EPERM;

     return 0;
}

Which makes the error check clear and makes the success case visible 
right at the bottom of the function.

~Ryan


^ permalink raw reply	[flat|nested] 72+ messages in thread

* Re: [PATCH 6/6] clk: Add initial WM831x clock driver
@ 2011-07-15  5:14           ` Ryan Mallon
  0 siblings, 0 replies; 72+ messages in thread
From: Ryan Mallon @ 2011-07-15  5:14 UTC (permalink / raw)
  To: Mark Brown
  Cc: Grant Likely, Jeremy Kerr, Grant Likely, linux-kernel,
	linux-arm-kernel, linux-sh, patches

On 15/07/11 15:05, Mark Brown wrote:
> On Thu, Jul 14, 2011 at 08:53:39PM -0600, Grant Likely wrote:
>> On Mon, Jul 11, 2011 at 11:53:57AM +0900, Mark Brown wrote:
>>> +	if (clkdata->xtal_ena)
>>> +		return 0;
>>> +	else
>>> +		return -EPERM;
>>> +}
>> Nit: return clkdata->xtal_ena ? 0 : -EPERM;
>> Just makes for more concise code.
> I have an extremely strong dislike of the ternery operator, I find it
> does nothing for legibility.
>
I prefer this:

     if (!clkdata->xtal_ena)
         return -EPERM;

     return 0;
}

Which makes the error check clear and makes the success case visible 
right at the bottom of the function.

~Ryan


^ permalink raw reply	[flat|nested] 72+ messages in thread

* [PATCH 6/6] clk: Add initial WM831x clock driver
@ 2011-07-15  5:14           ` Ryan Mallon
  0 siblings, 0 replies; 72+ messages in thread
From: Ryan Mallon @ 2011-07-15  5:14 UTC (permalink / raw)
  To: linux-arm-kernel

On 15/07/11 15:05, Mark Brown wrote:
> On Thu, Jul 14, 2011 at 08:53:39PM -0600, Grant Likely wrote:
>> On Mon, Jul 11, 2011 at 11:53:57AM +0900, Mark Brown wrote:
>>> +	if (clkdata->xtal_ena)
>>> +		return 0;
>>> +	else
>>> +		return -EPERM;
>>> +}
>> Nit: return clkdata->xtal_ena ? 0 : -EPERM;
>> Just makes for more concise code.
> I have an extremely strong dislike of the ternery operator, I find it
> does nothing for legibility.
>
I prefer this:

     if (!clkdata->xtal_ena)
         return -EPERM;

     return 0;
}

Which makes the error check clear and makes the success case visible 
right at the bottom of the function.

~Ryan

^ permalink raw reply	[flat|nested] 72+ messages in thread

end of thread, other threads:[~2011-07-15  5:14 UTC | newest]

Thread overview: 72+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-07-11  2:53 [PATCH 0/6] clk: Initial feedback for off-SoC slow bus clocks Mark Brown
2011-07-11  2:53 ` Mark Brown
2011-07-11  2:53 ` Mark Brown
2011-07-11  2:53 ` [PATCH 1/6] clk: Prototype and document clk_register() Mark Brown
2011-07-11  2:53   ` Mark Brown
2011-07-11  2:53   ` Mark Brown
2011-07-11  2:53   ` [PATCH 2/6] clk: Provide a dummy clk_unregister() Mark Brown
2011-07-11  2:53     ` Mark Brown
2011-07-11  2:53     ` Mark Brown
2011-07-11  2:53   ` [PATCH 3/6] clk: Constify struct clk_hw_ops Mark Brown
2011-07-11  2:53     ` Mark Brown
2011-07-11  2:53     ` Mark Brown
2011-07-11  2:53   ` [PATCH 4/6] clk: Add Kconfig option to build all generic clk drivers Mark Brown
2011-07-11  2:53     ` Mark Brown
2011-07-11  2:53     ` Mark Brown
2011-07-11  2:53   ` [PATCH 5/6] clk: Support multiple instances of the same clock provider Mark Brown
2011-07-11  2:53     ` Mark Brown
2011-07-11  2:53     ` Mark Brown
2011-07-11  9:34     ` [PATCH 5/6] clk: Support multiple instances of the same clock Russell King - ARM Linux
2011-07-11  9:34       ` [PATCH 5/6] clk: Support multiple instances of the same clock provider Russell King - ARM Linux
2011-07-11  9:34       ` Russell King - ARM Linux
2011-07-11 10:53       ` [PATCH 5/6] clk: Support multiple instances of the same clock Mark Brown
2011-07-11 10:53         ` [PATCH 5/6] clk: Support multiple instances of the same clock provider Mark Brown
2011-07-11 10:53         ` Mark Brown
2011-07-11 11:11         ` [PATCH 5/6] clk: Support multiple instances of the same clock Russell King - ARM Linux
2011-07-11 11:11           ` [PATCH 5/6] clk: Support multiple instances of the same clock provider Russell King - ARM Linux
2011-07-11 11:11           ` Russell King - ARM Linux
2011-07-11 11:41           ` [PATCH 5/6] clk: Support multiple instances of the same clock Mark Brown
2011-07-11 11:41             ` [PATCH 5/6] clk: Support multiple instances of the same clock provider Mark Brown
2011-07-11 11:41             ` Mark Brown
2011-07-11  2:53   ` [PATCH 6/6] clk: Add initial WM831x clock driver Mark Brown
2011-07-11  2:53     ` Mark Brown
2011-07-11  2:53     ` Mark Brown
2011-07-15  2:53     ` Grant Likely
2011-07-15  2:53       ` Grant Likely
2011-07-15  2:53       ` Grant Likely
2011-07-15  5:05       ` Mark Brown
2011-07-15  5:05         ` Mark Brown
2011-07-15  5:05         ` Mark Brown
2011-07-15  5:14         ` Ryan Mallon
2011-07-15  5:14           ` Ryan Mallon
2011-07-15  5:14           ` Ryan Mallon
2011-07-15  2:53   ` [PATCH 1/6] clk: Prototype and document clk_register() Grant Likely
2011-07-15  2:53     ` Grant Likely
2011-07-15  2:53     ` Grant Likely
2011-07-11  3:57 ` [PATCH 0/6] clk: Initial feedback for off-SoC slow bus clocks Mark Brown
2011-07-11  3:57   ` Mark Brown
2011-07-11  3:57   ` Mark Brown
2011-07-11  4:30   ` Mike Frysinger
2011-07-11  4:30     ` Mike Frysinger
2011-07-11  4:30     ` Mike Frysinger
2011-07-11  4:56     ` Barry Song
2011-07-11  4:56       ` Barry Song
2011-07-11  4:56       ` Barry Song
2011-07-11  5:01       ` [uclinux-dist-devel] [PATCH 0/6] clk: Initial feedback for Mike Frysinger
2011-07-11  5:01         ` [uclinux-dist-devel] [PATCH 0/6] clk: Initial feedback for off-SoC slow bus clocks Mike Frysinger
2011-07-11  5:01         ` Mike Frysinger
2011-07-11  9:31 ` Russell King - ARM Linux
2011-07-11  9:31   ` Russell King - ARM Linux
2011-07-11  9:31   ` Russell King - ARM Linux
2011-07-11 10:07   ` Sascha Hauer
2011-07-11 10:07     ` Sascha Hauer
2011-07-11 10:07     ` Sascha Hauer
2011-07-11 10:28     ` Russell King - ARM Linux
2011-07-11 10:28       ` Russell King - ARM Linux
2011-07-11 10:28       ` Russell King - ARM Linux
2011-07-11 10:46       ` Sascha Hauer
2011-07-11 10:46         ` Sascha Hauer
2011-07-11 10:46         ` Sascha Hauer
2011-07-11 11:43         ` Mark Brown
2011-07-11 11:43           ` Mark Brown
2011-07-11 11:43           ` Mark Brown

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.