[PATCH 0/6] Clocklib: generic clocks framework

[PATCH 0/6] Clocklib: generic clocks framework

[Dmitry Baryshkov]

Hi,

Here is an updated version of the clocklib patchset.

Changes since last repost:

* fix an clks_register error path, as noted by Pavel Machek

-- 
With best wishes
Dmitry

[PATCH 1/6] Clocklib: add generic framework for managing clocks.

[Dmitry Baryshkov]

Provide a generic framework that platform may choose
to support clocks api. In particular this provides
platform-independant struct clk definition, a full
implementation of clocks api and a set of functions
for registering and unregistering clocks in a safe way.

Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
---
 include/linux/clklib.h |  145 +++++++++++++++++++++
 init/Kconfig           |    7 +
 kernel/Makefile        |    1 +
 kernel/clklib.c        |  326 ++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 479 insertions(+), 0 deletions(-)
 create mode 100644 include/linux/clklib.h
 create mode 100644 kernel/clklib.c

diff --git a/include/linux/clklib.h b/include/linux/clklib.h
new file mode 100644
index 0000000..3d0ffaf
--- /dev/null
+++ b/include/linux/clklib.h
@@ -0,0 +1,145 @@
+/*
+ * Copyright (C) 2008 Dmitry Baryshkov
+ *
+ * This file is released under the GPL v2.
+ */
+
+#ifndef CLKLIB_H
+#define CLKLIB_H
+
+#include <linux/list.h>
+
+struct seq_file;
+
+/**
+ * struct clk_ops - generic clock management operations
+ * @can_get: checks whether passed device can get this clock
+ * @set_parent: reconfigures the clock to use specified parent
+ * @set_mode: enable or disable specified clock
+ * @get_rate: obtain the current clock rate of a specified clock
+ * @set_rate: set the clock rate for a specified clock
+ * @round_rate: adjust a reate to the exact rate a clock can provide
+ *
+ * This structure specifies clock operations that are used to configure
+ * specific clock.
+ */
+struct clk_ops {
+	int (*can_get)(struct clk *clk, struct device *dev);
+	int (*set_parent)(struct clk *clk, struct clk *parent);
+	int (*set_mode) (struct clk *clk, bool enable);
+	unsigned long (*get_rate) (struct clk *clk);
+	int (*set_rate)  (struct clk *, unsigned long);
+	long (*round_rate) (struct clk *, unsigned long);
+};
+
+/**
+ * struct clk - generic struct clk implementation used in the clocklib
+ * @node: used to place all clocks in a list
+ * @parent: the parent clock
+ * @owner: module holding all the functions
+ * @name: the name of this clock
+ * @users: count how many users have enabled this clock
+ * @ops: a pointer to clock management operations
+ */
+struct clk {
+	struct list_head node;
+	struct clk	*parent;
+	struct module *owner;
+
+	const char	*name;
+	int		users;
+
+	const struct clk_ops *ops;
+};
+
+int __must_check clk_register(struct clk *clk);
+void clk_unregister(struct clk *clk);
+static void __maybe_unused clks_unregister(struct clk **clks, size_t num)
+{
+	int i;
+	for (i = num - 1; i >= 0;  i++) {
+		clk_unregister(clks[i]);
+	}
+}
+
+static int __must_check __maybe_unused clks_register(struct clk **clks, size_t num)
+{
+	int i;
+	int ret;
+	for (i = 0; i < num; i++) {
+		ret = clk_register(clks[i]);
+		if (ret != 0)
+			goto cleanup;
+	}
+
+	return 0;
+
+cleanup:
+	clks_unregister(clks, i);
+
+	return ret;
+}
+
+int __must_check clk_alloc_function(const char *parent, struct clk *clk);
+
+/*
+ * struct clk_function - helper that allows easy registration of clock "functions"
+ * @parent: the name of the parent clock
+ * @clk: the clock that will be set up and installed
+ *
+ * Sometimes single clock will have multiple users or several clocks
+ * will be bound to similar devices. This allows one to register
+ * simple wrapper clocks that serve only naming purposes.
+ */
+struct clk_function {
+	const char *parent;
+	void *priv;
+	struct clk clk;
+};
+
+#define CLK_FUNC(_clock, _function, _ops, _data)	\
+	{						\
+		.parent = _clock,			\
+		.priv	= _data,			\
+		.clk = {				\
+			.name= _function,		\
+			.owner = THIS_MODULE,		\
+			.ops = _ops,			\
+		},					\
+	}
+#define FUNC_TO_CLK(func)	&(&(struct clk_function) func)->clk
+
+static void __maybe_unused clk_free_functions(
+		struct clk_function *funcs,
+		int num)
+{
+	int i;
+
+	for (i = num - 1; i >= 0; i--) {
+		clk_unregister(&funcs[i].clk);
+	}
+}
+
+static int __must_check __maybe_unused clk_alloc_functions(
+		struct clk_function *funcs,
+		int num)
+{
+	int i;
+	int rc;
+
+	for (i = 0; i < num; i++) {
+		rc = clk_alloc_function(funcs[i].parent, &funcs[i].clk);
+
+		if (rc) {
+			printk(KERN_ERR "Error allocating %s.%s function.\n",
+					funcs[i].parent,
+					funcs[i].clk.name);
+			clk_free_functions(funcs, i);
+			return rc;
+		}
+	}
+
+	return 0;
+}
+
+#endif
diff --git a/init/Kconfig b/init/Kconfig
index a97924b..1dd9ce2 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -504,6 +504,13 @@ config CC_OPTIMIZE_FOR_SIZE
 config SYSCTL
 	bool
 
+config HAVE_CLOCK_LIB
+	bool
+	help
+	  Platforms select clocklib if they use this infrastructure
+	  for managing their clocks both built into SoC and provided
+	  by external devices.
+
 menuconfig EMBEDDED
 	bool "Configure standard kernel features (for small systems)"
 	help
diff --git a/kernel/Makefile b/kernel/Makefile
index 6c584c5..afaed51 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -68,6 +68,7 @@ obj-$(CONFIG_TASK_DELAY_ACCT) += delayacct.o
 obj-$(CONFIG_TASKSTATS) += taskstats.o tsacct.o
 obj-$(CONFIG_MARKERS) += marker.o
 obj-$(CONFIG_LATENCYTOP) += latencytop.o
+obj-$(CONFIG_HAVE_CLOCK_LIB) += clklib.o
 
 ifneq ($(CONFIG_SCHED_NO_NO_OMIT_FRAME_POINTER),y)
 # According to Alan Modra <alan@linuxcare.com.au>, the -fno-omit-frame-pointer is
diff --git a/kernel/clklib.c b/kernel/clklib.c
new file mode 100644
index 0000000..012f845
--- /dev/null
+++ b/kernel/clklib.c
@@ -0,0 +1,326 @@
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/clk.h>
+#include <linux/clklib.h>
+#include <linux/spinlock.h>
+#include <linux/err.h>
+#include <linux/delay.h>
+
+static LIST_HEAD(clocks);
+static DEFINE_SPINLOCK(clocks_lock);
+
+static int __clk_register(struct clk *clk)
+{
+	if (clk->parent &&
+	    !try_module_get(clk->parent->owner))
+		return -EINVAL;
+
+	list_add_tail(&clk->node, &clocks);
+
+	return 0;
+}
+
+int __must_check clk_register(struct clk *clk)
+{
+	unsigned long flags;
+	int rc;
+
+	spin_lock_irqsave(&clocks_lock, flags);
+
+	rc = __clk_register(clk);
+
+	spin_unlock_irqrestore(&clocks_lock, flags);
+
+	return rc;
+}
+EXPORT_SYMBOL(clk_register);
+
+void clk_unregister(struct clk *clk)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&clocks_lock, flags);
+	list_del(&clk->node);
+	if (clk->parent)
+		module_put(clk->parent->owner);
+	spin_unlock_irqrestore(&clocks_lock, flags);
+}
+EXPORT_SYMBOL(clk_unregister);
+
+struct clk *clk_get(struct device *dev, const char *id)
+{
+	struct clk *p, *clk = ERR_PTR(-ENOENT);
+	unsigned long flags;
+
+	spin_lock_irqsave(&clocks_lock, flags);
+
+	list_for_each_entry(p, &clocks, node) {
+		if (strcmp(id, p->name) == 0 &&
+		    (p->ops && p->ops->can_get && p->ops->can_get(p, dev)) &&
+		    try_module_get(p->owner)) {
+			clk = p;
+			break;
+		}
+	}
+
+	list_for_each_entry(p, &clocks, node) {
+		if (strcmp(id, p->name) == 0 &&
+		    (!p->ops || !p->ops->can_get) &&
+		    try_module_get(p->owner)) {
+			clk = p;
+			break;
+		}
+	}
+
+	spin_unlock_irqrestore(&clocks_lock, flags);
+
+	return clk;
+}
+EXPORT_SYMBOL(clk_get);
+
+void clk_put(struct clk *clk)
+{
+	unsigned long flags;
+
+	if (!clk || IS_ERR(clk))
+		return;
+
+	spin_lock_irqsave(&clocks_lock, flags);
+
+	module_put(clk->owner);
+
+	spin_unlock_irqrestore(&clocks_lock, flags);
+}
+EXPORT_SYMBOL(clk_put);
+
+int clk_set_parent(struct clk *clk, struct clk *parent)
+{
+	int rc;
+	unsigned long flags;
+
+	if (!clk || IS_ERR(clk) || !clk->ops || !clk->ops->set_parent)
+		return -EINVAL;
+
+	spin_lock_irqsave(&clocks_lock, flags);
+
+	rc = clk->ops->set_parent(clk, parent);
+	if (!rc)
+		clk->parent = parent;
+
+	spin_unlock_irqrestore(&clocks_lock, flags);
+
+	return rc;
+}
+EXPORT_SYMBOL(clk_set_parent);
+
+struct clk *clk_get_parent(struct clk *clk)
+{
+	struct clk* parent;
+	unsigned long flags;
+
+	spin_lock_irqsave(&clocks_lock, flags);
+
+	parent = clk->parent;
+
+	spin_unlock_irqrestore(&clocks_lock, flags);
+
+	return parent;
+
+}
+
+static void __clk_disable(struct clk *clk)
+{
+	if (clk->users <= 0) {
+		WARN_ON(1);
+		return;
+	}
+
+	if (--clk->users == 0)
+		if (clk->ops && clk->ops->set_mode)
+			clk->ops->set_mode(clk, false);
+
+	if (clk->parent)
+		__clk_disable(clk->parent);
+}
+
+void clk_disable(struct clk *clk)
+{
+	unsigned long flags;
+
+	if (!clk || IS_ERR(clk))
+		return;
+
+	spin_lock_irqsave(&clocks_lock, flags);
+
+	__clk_disable(clk);
+
+	spin_unlock_irqrestore(&clocks_lock, flags);
+}
+EXPORT_SYMBOL(clk_disable);
+
+static int __clk_enable(struct clk *clk)
+{
+	int rc = 0;
+
+	if (clk->parent) {
+		rc = __clk_enable(clk->parent);
+
+		if (rc)
+			return rc;
+	}
+
+	if (clk->users++ != 0) {
+		return 0;
+	}
+
+	if (clk->ops && clk->ops->set_mode) {
+		rc = clk->ops->set_mode(clk, true);
+		if (rc) {
+			if (clk->parent)
+				__clk_disable(clk->parent);
+
+			return rc;
+		}
+	}
+
+	return rc;
+}
+
+int clk_enable(struct clk *clk)
+{
+	unsigned long flags;
+	int rc;
+
+	if (!clk || IS_ERR(clk))
+		return -EINVAL;
+
+	spin_lock_irqsave(&clocks_lock, flags);
+
+	rc = __clk_enable(clk);
+
+	spin_unlock_irqrestore(&clocks_lock, flags);
+
+	return rc;
+}
+EXPORT_SYMBOL(clk_enable);
+
+static unsigned long __clk_get_rate(struct clk *clk)
+{
+	unsigned long rate = 0;
+
+	while (clk) {
+		if (clk->ops && clk->ops->get_rate) {
+			rate = clk->ops->get_rate(clk);
+			break;
+		} else
+			clk = clk->parent;
+	}
+
+	return rate;
+}
+
+unsigned long clk_get_rate(struct clk *clk)
+{
+	unsigned long rate = 0;
+	unsigned long flags;
+
+	if (!clk || IS_ERR(clk))
+		return -EINVAL;
+
+	spin_lock_irqsave(&clocks_lock, flags);
+
+	rate = __clk_get_rate(clk);
+
+	spin_unlock_irqrestore(&clocks_lock, flags);
+
+	return rate;
+}
+EXPORT_SYMBOL(clk_get_rate);
+
+long clk_round_rate(struct clk *clk, unsigned long rate)
+{
+	long res = 0;
+	unsigned long flags;
+	struct clk *pclk;
+
+	if (!clk || IS_ERR(clk))
+		return -EINVAL;
+
+	spin_lock_irqsave(&clocks_lock, flags);
+
+	for (pclk = clk; !res && pclk ; pclk = pclk->parent) {
+		if (pclk->ops && pclk->ops->round_rate)
+			res = pclk->ops->round_rate(clk, rate);
+	}
+
+	if (!res)
+		res = __clk_get_rate(clk);
+
+	spin_unlock_irqrestore(&clocks_lock, flags);
+
+	return res;
+}
+EXPORT_SYMBOL(clk_round_rate);
+
+int clk_set_rate(struct clk *clk, unsigned long rate)
+{
+	int rc = -EINVAL;
+	unsigned long flags;
+
+	spin_lock_irqsave(&clocks_lock, flags);
+
+	while (clk && !IS_ERR(clk)) {
+		if (clk->ops && clk->ops->set_rate) {
+			rc = clk->ops->set_rate(clk, rate);
+			break;
+		}
+
+		clk = clk->parent;
+	}
+
+	spin_unlock_irqrestore(&clocks_lock, flags);
+
+	return rc;
+}
+EXPORT_SYMBOL(clk_set_rate);
+
+int clk_alloc_function(const char *parent, struct clk *clk)
+{
+	int rc = 0;
+	unsigned long flags;
+	struct clk *pclk;
+	bool found = false;
+
+	spin_lock_irqsave(&clocks_lock, flags);
+
+	list_for_each_entry(pclk, &clocks, node) {
+		if (strcmp(parent, pclk->name) == 0 &&
+		    try_module_get(pclk->owner)) {
+			found = true;
+			break;
+		}
+	}
+
+	if (!found) {
+		rc = -ENODEV;
+		goto out;
+	}
+
+	clk->parent = pclk;
+
+	__clk_register(clk);
+	/*
+	 * We locked parent owner during search
+	 * and also in __clk_register. Free one reference
+	 */
+	module_put(pclk->owner);
+
+out:
+	if (rc) {
+		kfree(clk);
+	}
+	spin_unlock_irqrestore(&clocks_lock, flags);
+
+	return rc;
+}
+EXPORT_SYMBOL(clk_alloc_function);
-- 
1.5.4.4


-- 
With best wishes
Dmitry

Re: [PATCH 1/6] Clocklib: add generic framework for managing clocks.

[Andrew Morton]

On Thu, 3 Apr 2008 17:23:29 +0400
Dmitry Baryshkov <dbaryshkov@gmail.com> wrote:

> Provide a generic framework that platform may choose
> to support clocks api. In particular this provides
> platform-independant struct clk definition, a full
> implementation of clocks api and a set of functions
> for registering and unregistering clocks in a safe way.
> 
> +static void __maybe_unused clks_unregister(struct clk **clks, size_t num)
> +static int __must_check __maybe_unused clks_register(struct clk **clks, size_t num)
> +static void __maybe_unused clk_free_functions(
> +		struct clk_function *funcs,
> +		int num)
> +static int __must_check __maybe_unused clk_alloc_functions(
> +		struct clk_function *funcs,
> +		int num)

What are all these __maybe_unused markers doing here?

> ...
>
> +int __must_check clk_alloc_function(const char *parent, struct clk *clk);

This should be `static int'.  I'm surprised the compiler doesn't get
upset.

> +	for (i = num - 1; i >= 0; i--) {
> +		clk_unregister(&funcs[i].clk);
> +	}
>
> ...
>
> +	if (rc) {
> +		kfree(clk);
> +	}

There are a few coding-style glitches in here.  Please run
scripts/checkpatch.pl and consider its reportage.

[PATCH 2/6] Clocklib: debugfs support

[Dmitry Baryshkov]

Provide /sys/kernel/debug/clock to ease debugging.

Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
---
 include/linux/clklib.h |    2 +
 kernel/clklib.c        |   74 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+), 0 deletions(-)

diff --git a/include/linux/clklib.h b/include/linux/clklib.h
index 3d0ffaf..77514cc 100644
--- a/include/linux/clklib.h
+++ b/include/linux/clklib.h
@@ -19,6 +19,7 @@ struct seq_file;
  * @get_rate: obtain the current clock rate of a specified clock
  * @set_rate: set the clock rate for a specified clock
  * @round_rate: adjust a reate to the exact rate a clock can provide
+ * @format: output any additional information for a clock
  *
  * This structure specifies clock operations that are used to configure
  * specific clock.
@@ -30,6 +31,7 @@ struct clk_ops {
 	unsigned long (*get_rate) (struct clk *clk);
 	int (*set_rate)  (struct clk *, unsigned long);
 	long (*round_rate) (struct clk *, unsigned long);
+	int (*format)	(struct clk *, struct seq_file *);
 };
 
 /**
diff --git a/kernel/clklib.c b/kernel/clklib.c
index 012f845..8d872e1 100644
--- a/kernel/clklib.c
+++ b/kernel/clklib.c
@@ -324,3 +324,77 @@ out:
 	return rc;
 }
 EXPORT_SYMBOL(clk_alloc_function);
+
+#ifdef CONFIG_DEBUG_FS
+
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
+
+#define NAME_FIELD_LEN	20
+
+static void dump_clocks(struct seq_file *s, struct clk *parent, int nest)
+{
+	struct clk *clk;
+	int i;
+
+	list_for_each_entry(clk, &clocks, node) {
+		if (clk->parent == parent) {
+			for (i = 0; i < nest; i++) {
+				seq_putc(s, ' ');
+				seq_putc(s, ' ');
+			}
+			seq_puts(s, clk->name);
+
+			i = 2 * nest + strlen(clk->name);
+			if (i >= NAME_FIELD_LEN)
+				i = NAME_FIELD_LEN - 1;
+			for (; i < NAME_FIELD_LEN; i++) {
+				seq_putc(s, ' ');
+			}
+			seq_printf(s, "%c use=%d rate=%10lu Hz",
+				clk->ops && clk->ops->set_parent ? '*' : ' ',
+				clk->users,
+				__clk_get_rate(clk));
+			if (clk->ops && clk->ops->format)
+				clk->ops->format(clk, s);
+			seq_putc(s, '\n');
+
+			dump_clocks(s, clk, nest + 1);
+		}
+	}
+}
+
+static int clocklib_show(struct seq_file *s, void *unused)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&clocks_lock, flags);
+
+	dump_clocks(s, NULL, 0);
+
+	spin_unlock_irqrestore(&clocks_lock, flags);
+
+	return 0;
+}
+
+static int clocklib_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, clocklib_show, NULL);
+}
+
+static struct file_operations clocklib_operations = {
+	.open		= clocklib_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
+
+static int __init clocklib_debugfs_init(void)
+{
+	debugfs_create_file("clock", S_IFREG | S_IRUGO,
+				NULL, NULL, &clocklib_operations);
+	return 0;
+}
+subsys_initcall(clocklib_debugfs_init);
+
+#endif /* DEBUG_FS */
-- 
1.5.4.4


-- 
With best wishes
Dmitry

Re: [PATCH 2/6] Clocklib: debugfs support

[Andrew Morton]

On Thu, 3 Apr 2008 17:23:37 +0400
Dmitry Baryshkov <dbaryshkov@gmail.com> wrote:

> Provide /sys/kernel/debug/clock to ease debugging.
> 

Plese fully document the proposed kernel->userspace interface in the
changelog so that we can review it design.

> diff --git a/include/linux/clklib.h b/include/linux/clklib.h
> index 3d0ffaf..77514cc 100644
> --- a/include/linux/clklib.h
> +++ b/include/linux/clklib.h
> @@ -19,6 +19,7 @@ struct seq_file;
>   * @get_rate: obtain the current clock rate of a specified clock
>   * @set_rate: set the clock rate for a specified clock
>   * @round_rate: adjust a reate to the exact rate a clock can provide
> + * @format: output any additional information for a clock
>   *
>   * This structure specifies clock operations that are used to configure
>   * specific clock.
> @@ -30,6 +31,7 @@ struct clk_ops {
>  	unsigned long (*get_rate) (struct clk *clk);
>  	int (*set_rate)  (struct clk *, unsigned long);
>  	long (*round_rate) (struct clk *, unsigned long);
> +	int (*format)	(struct clk *, struct seq_file *);

It's conventional to leave no space between the ) and the ( here.  Using a
tab is quite unconventional.

>  };
>  
>  /**
> diff --git a/kernel/clklib.c b/kernel/clklib.c
> index 012f845..8d872e1 100644
> --- a/kernel/clklib.c
> +++ b/kernel/clklib.c
> @@ -324,3 +324,77 @@ out:
>  	return rc;
>  }
>  EXPORT_SYMBOL(clk_alloc_function);
> +
> +#ifdef CONFIG_DEBUG_FS
> +
> +#include <linux/debugfs.h>
> +#include <linux/seq_file.h>
> +
> +#define NAME_FIELD_LEN	20
> +
> +static void dump_clocks(struct seq_file *s, struct clk *parent, int nest)
> +{
> +	struct clk *clk;
> +	int i;
> +
> +	list_for_each_entry(clk, &clocks, node) {
> +		if (clk->parent == parent) {
> +			for (i = 0; i < nest; i++) {
> +				seq_putc(s, ' ');
> +				seq_putc(s, ' ');
> +			}
> +			seq_puts(s, clk->name);
> +
> +			i = 2 * nest + strlen(clk->name);
> +			if (i >= NAME_FIELD_LEN)
> +				i = NAME_FIELD_LEN - 1;
> +			for (; i < NAME_FIELD_LEN; i++) {
> +				seq_putc(s, ' ');
> +			}
> +			seq_printf(s, "%c use=%d rate=%10lu Hz",
> +				clk->ops && clk->ops->set_parent ? '*' : ' ',
> +				clk->users,
> +				__clk_get_rate(clk));
> +			if (clk->ops && clk->ops->format)
> +				clk->ops->format(clk, s);
> +			seq_putc(s, '\n');
> +
> +			dump_clocks(s, clk, nest + 1);
> +		}
> +	}
> +}

hm, this would break the one-value-per-file rule, except that applies to
sysfs, but this interface is implemented via debug fs, but it is proposed
that it appear under /sys.  Tricky.

Re: [PATCH 2/6] Clocklib: debugfs support

[Greg KH]

On Mon, Apr 07, 2008 at 03:59:01PM -0700, Andrew Morton wrote:
> On Thu, 3 Apr 2008 17:23:37 +0400
> Dmitry Baryshkov <dbaryshkov@gmail.com> wrote:
> 
> > Provide /sys/kernel/debug/clock to ease debugging.
> > 
> 
> Plese fully document the proposed kernel->userspace interface in the
> changelog so that we can review it design.

You can do this easily by providing a file for Documentation/ABI/
describing your addition to the kernel ABI.

> >  /**
> > diff --git a/kernel/clklib.c b/kernel/clklib.c
> > index 012f845..8d872e1 100644
> > --- a/kernel/clklib.c
> > +++ b/kernel/clklib.c
> > @@ -324,3 +324,77 @@ out:
> >  	return rc;
> >  }
> >  EXPORT_SYMBOL(clk_alloc_function);
> > +
> > +#ifdef CONFIG_DEBUG_FS
> > +
> > +#include <linux/debugfs.h>
> > +#include <linux/seq_file.h>
> > +
> > +#define NAME_FIELD_LEN	20
> > +
> > +static void dump_clocks(struct seq_file *s, struct clk *parent, int nest)
> > +{
> > +	struct clk *clk;
> > +	int i;
> > +
> > +	list_for_each_entry(clk, &clocks, node) {
> > +		if (clk->parent == parent) {
> > +			for (i = 0; i < nest; i++) {
> > +				seq_putc(s, ' ');
> > +				seq_putc(s, ' ');
> > +			}
> > +			seq_puts(s, clk->name);
> > +
> > +			i = 2 * nest + strlen(clk->name);
> > +			if (i >= NAME_FIELD_LEN)
> > +				i = NAME_FIELD_LEN - 1;
> > +			for (; i < NAME_FIELD_LEN; i++) {
> > +				seq_putc(s, ' ');
> > +			}
> > +			seq_printf(s, "%c use=%d rate=%10lu Hz",
> > +				clk->ops && clk->ops->set_parent ? '*' : ' ',
> > +				clk->users,
> > +				__clk_get_rate(clk));
> > +			if (clk->ops && clk->ops->format)
> > +				clk->ops->format(clk, s);
> > +			seq_putc(s, '\n');
> > +
> > +			dump_clocks(s, clk, nest + 1);
> > +		}
> > +	}
> > +}
> 
> hm, this would break the one-value-per-file rule, except that applies to
> sysfs, but this interface is implemented via debug fs, but it is proposed
> that it appear under /sys.  Tricky.

Huh?

How can a seq_file entry show up in sysfs?

Or do you mean /sys/kernel/debug/  ?
That's where debugfs is supposed to be mounted so that's fine, it's
really a debugfs file, not a sysfs one.

thanks,

greg k-h

[PATCH 3/6] Clocklib: support sa1100 sub-arch.

[Dmitry Baryshkov]

Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
---
 arch/arm/Kconfig             |    1 +
 arch/arm/mach-sa1100/clock.c |  131 ++++++++++-------------------------------
 2 files changed, 33 insertions(+), 99 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 4039a13..6d78a27 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -426,6 +426,7 @@ config ARCH_SA1100
 	select GENERIC_GPIO
 	select GENERIC_TIME
 	select HAVE_IDE
+	select HAVE_CLOCK_LIB
 	help
 	  Support for StrongARM 11x0 based boards.
 
diff --git a/arch/arm/mach-sa1100/clock.c b/arch/arm/mach-sa1100/clock.c
index fc97fe5..8f9b777 100644
--- a/arch/arm/mach-sa1100/clock.c
+++ b/arch/arm/mach-sa1100/clock.c
@@ -8,127 +8,60 @@
 #include <linux/err.h>
 #include <linux/string.h>
 #include <linux/clk.h>
+#include <linux/clklib.h>
 #include <linux/spinlock.h>
 #include <linux/mutex.h>
 
 #include <asm/hardware.h>
 
-/*
- * Very simple clock implementation - we only have one clock to
- * deal with at the moment, so we only match using the "name".
- */
-struct clk {
-	struct list_head	node;
-	unsigned long		rate;
-	const char		*name;
-	unsigned int		enabled;
-	void			(*enable)(void);
-	void			(*disable)(void);
-};
-
-static LIST_HEAD(clocks);
-static DEFINE_MUTEX(clocks_mutex);
-static DEFINE_SPINLOCK(clocks_lock);
-
-struct clk *clk_get(struct device *dev, const char *id)
-{
-	struct clk *p, *clk = ERR_PTR(-ENOENT);
-
-	mutex_lock(&clocks_mutex);
-	list_for_each_entry(p, &clocks, node) {
-		if (strcmp(id, p->name) == 0) {
-			clk = p;
-			break;
-		}
-	}
-	mutex_unlock(&clocks_mutex);
-
-	return clk;
-}
-EXPORT_SYMBOL(clk_get);
-
-void clk_put(struct clk *clk)
-{
-}
-EXPORT_SYMBOL(clk_put);
-
-int clk_enable(struct clk *clk)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(&clocks_lock, flags);
-	if (clk->enabled++ == 0)
-		clk->enable();
-	spin_unlock_irqrestore(&clocks_lock, flags);
-	return 0;
-}
-EXPORT_SYMBOL(clk_enable);
-
-void clk_disable(struct clk *clk)
-{
-	unsigned long flags;
-
-	WARN_ON(clk->enabled == 0);
-
-	spin_lock_irqsave(&clocks_lock, flags);
-	if (--clk->enabled == 0)
-		clk->disable();
-	spin_unlock_irqrestore(&clocks_lock, flags);
-}
-EXPORT_SYMBOL(clk_disable);
-
-unsigned long clk_get_rate(struct clk *clk)
-{
-	return clk->rate;
-}
-EXPORT_SYMBOL(clk_get_rate);
-
-
-static void clk_gpio27_enable(void)
+static int clk_gpio27_set_enable(struct clk *clk, bool enable)
 {
 	/*
 	 * First, set up the 3.6864MHz clock on GPIO 27 for the SA-1111:
 	 * (SA-1110 Developer's Manual, section 9.1.2.1)
 	 */
-	GAFR |= GPIO_32_768kHz;
-	GPDR |= GPIO_32_768kHz;
-	TUCR = TUCR_3_6864MHz;
+	if (enable) {
+		GAFR |= GPIO_32_768kHz;
+		GPDR |= GPIO_32_768kHz;
+		TUCR = TUCR_3_6864MHz;
+	} else {
+		TUCR = 0;
+		GPDR &= ~GPIO_32_768kHz;
+		GAFR &= ~GPIO_32_768kHz;
+	}
+
+	return 0;
 }
 
-static void clk_gpio27_disable(void)
+static unsigned long clk_gpio27_get_rate(struct clk *clk)
 {
-	TUCR = 0;
-	GPDR &= ~GPIO_32_768kHz;
-	GAFR &= ~GPIO_32_768kHz;
+	return 3686400;
 }
 
+static struct clk_ops clk_gpio27_ops = {
+	.get_rate	= clk_gpio27_get_rate,
+	.set_mode	= clk_gpio27_set_enable,
+};
+
 static struct clk clk_gpio27 = {
 	.name		= "GPIO27_CLK",
-	.rate		= 3686400,
-	.enable		= clk_gpio27_enable,
-	.disable	= clk_gpio27_disable,
+	.ops		= &clk_gpio27_ops,
+	.owner		= THIS_MODULE,
 };
 
-int clk_register(struct clk *clk)
-{
-	mutex_lock(&clocks_mutex);
-	list_add(&clk->node, &clocks);
-	mutex_unlock(&clocks_mutex);
-	return 0;
-}
-EXPORT_SYMBOL(clk_register);
+static struct clk clk_3_6MHz= {
+	.name		= "3_6MHz_CLK",
+	.parent		= &clk_gpio27,
+	.owner		= THIS_MODULE,
+};
 
-void clk_unregister(struct clk *clk)
-{
-	mutex_lock(&clocks_mutex);
-	list_del(&clk->node);
-	mutex_unlock(&clocks_mutex);
-}
-EXPORT_SYMBOL(clk_unregister);
+static struct clk* clks[] = {
+	&clk_gpio27,
+	&clk_3_6MHz,
+};
 
 static int __init clk_init(void)
 {
-	clk_register(&clk_gpio27);
-	return 0;
+	return clks_register(clks, ARRAY_SIZE(clks));
 }
 arch_initcall(clk_init);
-- 
1.5.4.4


-- 
With best wishes
Dmitry

[PATCH 4/6] Clocklib: support ARM pxa sub-arch.

[Dmitry Baryshkov]

Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
---
 arch/arm/Kconfig           |    1 +
 arch/arm/mach-pxa/clock.c  |  152 ++++++++++++++++----------------------------
 arch/arm/mach-pxa/clock.h  |   64 ++++++++----------
 arch/arm/mach-pxa/pxa25x.c |   74 +++++++++++++--------
 arch/arm/mach-pxa/pxa27x.c |   67 +++++++++++--------
 arch/arm/mach-pxa/pxa3xx.c |  132 +++++++++++++++++++++-----------------
 6 files changed, 244 insertions(+), 246 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 6d78a27..ce2ffe0 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -402,6 +402,7 @@ config ARCH_PXA
 	select GENERIC_TIME
 	select GENERIC_CLOCKEVENTS
 	select TICK_ONESHOT
+	select HAVE_CLOCK_LIB
 	help
 	  Support for Intel/Marvell's PXA2xx/PXA3xx processor line.
 
diff --git a/arch/arm/mach-pxa/clock.c b/arch/arm/mach-pxa/clock.c
index df5ae27..ba45ec0 100644
--- a/arch/arm/mach-pxa/clock.c
+++ b/arch/arm/mach-pxa/clock.c
@@ -8,6 +8,7 @@
 #include <linux/err.h>
 #include <linux/string.h>
 #include <linux/clk.h>
+#include <linux/clklib.h>
 #include <linux/spinlock.h>
 #include <linux/platform_device.h>
 #include <linux/delay.h>
@@ -19,135 +20,92 @@
 #include "generic.h"
 #include "clock.h"
 
-static LIST_HEAD(clocks);
-static DEFINE_MUTEX(clocks_mutex);
-static DEFINE_SPINLOCK(clocks_lock);
-
-static struct clk *clk_lookup(struct device *dev, const char *id)
+static int clk_gpio11_set_mode(struct clk *clk, bool enable)
 {
-	struct clk *p;
-
-	list_for_each_entry(p, &clocks, node)
-		if (strcmp(id, p->name) == 0 && p->dev == dev)
-			return p;
+	if (enable)
+		pxa_gpio_mode(GPIO11_3_6MHz_MD);
 
-	return NULL;
+	return 0;
 }
 
-struct clk *clk_get(struct device *dev, const char *id)
+static unsigned long clk_gpio11_get_rate(struct clk *clk)
 {
-	struct clk *p, *clk = ERR_PTR(-ENOENT);
+	return 3686400;
+}
 
-	mutex_lock(&clocks_mutex);
-	p = clk_lookup(dev, id);
-	if (!p)
-		p = clk_lookup(NULL, id);
-	if (p)
-		clk = p;
-	mutex_unlock(&clocks_mutex);
+static const struct clk_ops clk_gpio11_ops = {
+	.get_rate	= clk_gpio11_get_rate,
+	.set_mode	= clk_gpio11_set_mode,
+};
 
-	return clk;
-}
-EXPORT_SYMBOL(clk_get);
+static struct clk clk_gpio11 = {
+	.name		= "GPIO11_CLK",
+	.ops		= &clk_gpio11_ops,
+	.owner		= THIS_MODULE,
+};
 
-void clk_put(struct clk *clk)
-{
-}
-EXPORT_SYMBOL(clk_put);
+static struct clk clk_3_6MHz= {
+	.name		= "3_6MHz_CLK",
+	.parent		= &clk_gpio11,
+	.owner		= THIS_MODULE,
+};
 
-int clk_enable(struct clk *clk)
+int clk_cken_set_mode(struct clk *clk, bool enable)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(&clocks_lock, flags);
-	if (clk->enabled++ == 0)
-		clk->ops->enable(clk);
-	spin_unlock_irqrestore(&clocks_lock, flags);
+	struct clk_cken *cclk = container_of(clk, struct clk_cken, clk);
+	int cken = cclk->cken;
 
-	if (clk->delay)
-		udelay(clk->delay);
+	if (enable) {
+		CKEN |= 1 << cken;
+		if (cclk->delay)
+			udelay(cclk->delay);
+	} else
+		CKEN &= ~(1 << cken);
 
 	return 0;
 }
-EXPORT_SYMBOL(clk_enable);
-
-void clk_disable(struct clk *clk)
-{
-	unsigned long flags;
 
-	WARN_ON(clk->enabled == 0);
+unsigned long clk_cken_get_rate(struct clk *clk) {
+	struct clk_cken *cclk = container_of(clk, struct clk_cken, clk);
 
-	spin_lock_irqsave(&clocks_lock, flags);
-	if (--clk->enabled == 0)
-		clk->ops->disable(clk);
-	spin_unlock_irqrestore(&clocks_lock, flags);
+	return cclk->rate;
 }
-EXPORT_SYMBOL(clk_disable);
-
-unsigned long clk_get_rate(struct clk *clk)
-{
-	unsigned long rate;
-
-	rate = clk->rate;
-	if (clk->ops->getrate)
-		rate = clk->ops->getrate(clk);
-
-	return rate;
-}
-EXPORT_SYMBOL(clk_get_rate);
 
+const struct clk_ops clk_cken_ops = {
+	.set_mode = clk_cken_set_mode,
+	.get_rate = clk_cken_get_rate,
+};
 
-static void clk_gpio27_enable(struct clk *clk)
+static int clk_dev_can_get(struct clk *clk, struct device *dev)
 {
-	pxa_gpio_mode(GPIO11_3_6MHz_MD);
-}
+	struct clk_function *cfunc = container_of(clk, struct clk_function, clk);
 
-static void clk_gpio27_disable(struct clk *clk)
-{
+	return (dev == cfunc->priv);
 }
 
-static const struct clkops clk_gpio27_ops = {
-	.enable		= clk_gpio27_enable,
-	.disable	= clk_gpio27_disable,
-};
-
-
-void clk_cken_enable(struct clk *clk)
+static int clk_dev_format(struct clk *clk, struct seq_file *s)
 {
-	CKEN |= 1 << clk->cken;
-}
+	struct clk_function *cfunc = container_of(clk, struct clk_function, clk);
 
-void clk_cken_disable(struct clk *clk)
-{
-	CKEN &= ~(1 << clk->cken);
+	BUG_ON(!cfunc->priv);
+
+	seq_puts(s, " for device ");
+	seq_puts(s, ((struct device *)cfunc->priv)->bus_id);
+	return 0;
 }
 
-const struct clkops clk_cken_ops = {
-	.enable		= clk_cken_enable,
-	.disable	= clk_cken_disable,
+const struct clk_ops clk_dev_ops = {
+	.can_get = clk_dev_can_get,
+	.format = clk_dev_format,
 };
 
-static struct clk common_clks[] = {
-	{
-		.name		= "GPIO27_CLK",
-		.ops		= &clk_gpio27_ops,
-		.rate		= 3686400,
-	},
+static struct clk * common_clks[] = {
+	&clk_gpio11,
+	&clk_3_6MHz,
 };
 
-void clks_register(struct clk *clks, size_t num)
-{
-	int i;
-
-	mutex_lock(&clocks_mutex);
-	for (i = 0; i < num; i++)
-		list_add(&clks[i].node, &clocks);
-	mutex_unlock(&clocks_mutex);
-}
-
 static int __init clk_init(void)
 {
-	clks_register(common_clks, ARRAY_SIZE(common_clks));
-	return 0;
+	return clks_register(common_clks, ARRAY_SIZE(common_clks));
 }
 arch_initcall(clk_init);
diff --git a/arch/arm/mach-pxa/clock.h b/arch/arm/mach-pxa/clock.h
index bc6b77e..0b11d13 100644
--- a/arch/arm/mach-pxa/clock.h
+++ b/arch/arm/mach-pxa/clock.h
@@ -1,43 +1,37 @@
-struct clk;
+#include <linux/clk.h>
+#include <linux/clklib.h>
+#include <linux/seq_file.h>
 
-struct clkops {
-	void			(*enable)(struct clk *);
-	void			(*disable)(struct clk *);
-	unsigned long		(*getrate)(struct clk *);
-};
+extern int clk_cken_set_mode(struct clk *clk, bool enable);
+extern unsigned long clk_cken_get_rate(struct clk *clk);
+extern const struct clk_ops clk_cken_ops;
 
-struct clk {
-	struct list_head	node;
-	const char		*name;
-	struct device		*dev;
-	const struct clkops	*ops;
-	unsigned long		rate;
-	unsigned int		cken;
-	unsigned int		delay;
-	unsigned int		enabled;
+struct clk_cken {
+	unsigned int	cken;
+	unsigned long	rate;
+	int		delay;
+	struct clk	clk;
 };
 
-#define INIT_CKEN(_name, _cken, _rate, _delay, _dev)	\
-	{						\
-		.name	= _name,			\
-		.dev	= _dev,				\
-		.ops	= &clk_cken_ops,		\
+#define INIT_CKEN(_name, _cken, _rate, _delay)		\
+	&(&(struct clk_cken) {				\
+		.cken = CKEN_##_cken,			\
 		.rate	= _rate,			\
-		.cken	= CKEN_##_cken,			\
 		.delay	= _delay,			\
-	}
-
-#define INIT_CK(_name, _cken, _ops, _dev)		\
-	{						\
-		.name	= _name,			\
-		.dev	= _dev,				\
-		.ops	= _ops,				\
-		.cken	= CKEN_##_cken,			\
-	}
-
-extern const struct clkops clk_cken_ops;
+		.clk = {				\
+			.name	= _name,		\
+			.ops	= &clk_cken_ops,	\
+		},					\
+	})->clk
 
-void clk_cken_enable(struct clk *clk);
-void clk_cken_disable(struct clk *clk);
+#define INIT_CK(_name, _cken, _ops)			\
+	&(&(struct clk_cken) {				\
+		.cken = CKEN_##_cken,			\
+		.clk = {				\
+			.name	= _name,		\
+			.ops	= _ops,			\
+		},					\
+	})->clk
 
-void clks_register(struct clk *clks, size_t num);
+extern const struct clk_ops clk_dev_ops;
+extern const struct clk_ops clk_pxa3xx_cken_ops;
diff --git a/arch/arm/mach-pxa/pxa25x.c b/arch/arm/mach-pxa/pxa25x.c
index 599e53f..5924ef1 100644
--- a/arch/arm/mach-pxa/pxa25x.c
+++ b/arch/arm/mach-pxa/pxa25x.c
@@ -101,10 +101,9 @@ static unsigned long clk_pxa25x_lcd_getrate(struct clk *clk)
 	return pxa25x_get_memclk_frequency_10khz() * 10000;
 }
 
-static const struct clkops clk_pxa25x_lcd_ops = {
-	.enable		= clk_cken_enable,
-	.disable	= clk_cken_disable,
-	.getrate	= clk_pxa25x_lcd_getrate,
+static const struct clk_ops clk_pxa25x_lcd_ops = {
+	.set_mode = clk_cken_set_mode,
+	.get_rate = clk_pxa25x_lcd_getrate,
 };
 
 /*
@@ -112,29 +111,43 @@ static const struct clkops clk_pxa25x_lcd_ops = {
  * 95.842MHz -> MMC 19.169MHz, I2C 31.949MHz, FICP 47.923MHz, USB 47.923MHz
  * 147.456MHz -> UART 14.7456MHz, AC97 12.288MHz, I2S 5.672MHz (allegedly)
  */
-static struct clk pxa25x_hwuart_clk =
-	INIT_CKEN("UARTCLK", HWUART, 14745600, 1, &pxa_device_hwuart.dev)
-;
-
-static struct clk pxa25x_clks[] = {
-	INIT_CK("LCDCLK", LCD, &clk_pxa25x_lcd_ops, &pxa_device_fb.dev),
-	INIT_CKEN("UARTCLK", FFUART, 14745600, 1, &pxa_device_ffuart.dev),
-	INIT_CKEN("UARTCLK", BTUART, 14745600, 1, &pxa_device_btuart.dev),
-	INIT_CKEN("UARTCLK", STUART, 14745600, 1, NULL),
-	INIT_CKEN("UDCCLK", USB, 47923000, 5, &pxa_device_udc.dev),
-	INIT_CKEN("MMCCLK", MMC, 19169000, 0, &pxa_device_mci.dev),
-	INIT_CKEN("I2CCLK", I2C, 31949000, 0, &pxa_device_i2c.dev),
-
-	INIT_CKEN("SSPCLK",  SSP, 3686400, 0, &pxa25x_device_ssp.dev),
-	INIT_CKEN("SSPCLK", NSSP, 3686400, 0, &pxa25x_device_nssp.dev),
-	INIT_CKEN("SSPCLK", ASSP, 3686400, 0, &pxa25x_device_assp.dev),
+static struct clk *pxa25x_hwuart_cken_clk =
+	INIT_CKEN("HWUARTCLK", HWUART, 14745600, 1);
+
+static struct clk_function pxa25x_hwuart_func =
+	CLK_FUNC("HWUARTCLK", "UARTCLK", &clk_dev_ops, &pxa_device_hwuart.dev);
+
+
+static struct clk *pxa25x_clks[] = {
+	INIT_CK("LCDCLK", LCD, &clk_pxa25x_lcd_ops),
+	INIT_CKEN("FFUARTCLK", FFUART, 14745600, 1),
+	INIT_CKEN("BTUARTCLK", BTUART, 14745600, 1),
+	INIT_CKEN("STUARTCLK", STUART, 14745600, 1),
+	INIT_CKEN("UDCCLK", USB, 47923000, 5),
+	INIT_CKEN("PXAMMCCLK", MMC, 19169000, 0),
+	INIT_CKEN("I2CCLK", I2C, 31949000, 0),
+
+	INIT_CKEN("SSP_CLK",  SSP, 3686400, 0),
+	INIT_CKEN("NSSPCLK", NSSP, 3686400, 0),
+	INIT_CKEN("ASSPCLK", ASSP, 3686400, 0),
 
 	/*
-	INIT_CKEN("PWMCLK",  PWM0, 3686400,  0, NULL),
-	INIT_CKEN("PWMCLK",  PWM0, 3686400,  0, NULL),
-	INIT_CKEN("I2SCLK",  I2S,  14745600, 0, NULL),
+	INIT_CKEN("PWMCLK",  PWM0, 3686400,  0),
+	INIT_CKEN("PWMCLK",  PWM0, 3686400,  0),
+	INIT_CKEN("I2SCLK",  I2S,  14745600, 0),
 	*/
-	INIT_CKEN("FICPCLK", FICP, 47923000, 0, NULL),
+	INIT_CKEN("FICPCLK", FICP, 47923000, 0),
+};
+
+static struct clk_function pxa25x_clk_funcs[] = {
+	CLK_FUNC("FFUARTCLK", "UARTCLK", &clk_dev_ops, &pxa_device_ffuart.dev),
+	CLK_FUNC("BTUARTCLK", "UARTCLK", &clk_dev_ops, &pxa_device_btuart.dev),
+	CLK_FUNC("STUARTCLK", "UARTCLK", &clk_dev_ops, &pxa_device_stuart.dev),
+	CLK_FUNC("STUARTCLK", "SIRCLK", NULL, NULL),
+	CLK_FUNC("PXAMMCCLK", "MMCCLK", &clk_dev_ops, &pxa_device_mci.dev),
+	CLK_FUNC("SSP_CLK", "SSPCLK", &clk_dev_ops, &pxa25x_device_ssp.dev),
+	CLK_FUNC("NSSPCLK", "SSPCLK", &clk_dev_ops, &pxa25x_device_nssp.dev),
+	CLK_FUNC("ASSPCLK", "SSPCLK", &clk_dev_ops, &pxa25x_device_assp.dev),
 };
 
 #ifdef CONFIG_PM
@@ -289,17 +302,22 @@ static struct sys_device pxa25x_sysdev[] = {
 		.cls	= &pxa_gpio_sysclass,
 	},
 };
-
 static int __init pxa25x_init(void)
 {
 	int i, ret = 0;
 
 	/* Only add HWUART for PXA255/26x; PXA210/250/27x do not have it. */
-	if (cpu_is_pxa25x())
-		clks_register(&pxa25x_hwuart_clk, 1);
+	if (cpu_is_pxa25x()) {
+		ret = clk_register(pxa25x_hwuart_cken_clk);
+		if (!ret)
+			ret = clk_alloc_function(
+					pxa25x_hwuart_func.parent,
+					&pxa25x_hwuart_func.clk);
+	}
 
 	if (cpu_is_pxa21x() || cpu_is_pxa25x()) {
-		clks_register(pxa25x_clks, ARRAY_SIZE(pxa25x_clks));
+		ret = clks_register(pxa25x_clks, ARRAY_SIZE(pxa25x_clks));
+		ret = clk_alloc_functions(pxa25x_clk_funcs, ARRAY_SIZE(pxa25x_clk_funcs));
 
 		if ((ret = pxa_init_dma(16)))
 			return ret;
diff --git a/arch/arm/mach-pxa/pxa27x.c b/arch/arm/mach-pxa/pxa27x.c
index 46a951c..11a15a6 100644
--- a/arch/arm/mach-pxa/pxa27x.c
+++ b/arch/arm/mach-pxa/pxa27x.c
@@ -129,44 +129,54 @@ static unsigned long clk_pxa27x_lcd_getrate(struct clk *clk)
 	return pxa27x_get_lcdclk_frequency_10khz() * 10000;
 }
 
-static const struct clkops clk_pxa27x_lcd_ops = {
-	.enable		= clk_cken_enable,
-	.disable	= clk_cken_disable,
-	.getrate	= clk_pxa27x_lcd_getrate,
+static const struct clk_ops clk_pxa27x_lcd_ops = {
+	.set_mode = clk_cken_set_mode,
+	.get_rate = clk_pxa27x_lcd_getrate,
 };
 
-static struct clk pxa27x_clks[] = {
-	INIT_CK("LCDCLK", LCD,    &clk_pxa27x_lcd_ops, &pxa_device_fb.dev),
-	INIT_CK("CAMCLK", CAMERA, &clk_pxa27x_lcd_ops, NULL),
+static struct clk *pxa27x_clks[] = {
+	INIT_CK("LCDCLK", LCD,    &clk_pxa27x_lcd_ops),
+	INIT_CK("CAMCLK", CAMERA, &clk_pxa27x_lcd_ops),
 
-	INIT_CKEN("UARTCLK", FFUART, 14857000, 1, &pxa_device_ffuart.dev),
-	INIT_CKEN("UARTCLK", BTUART, 14857000, 1, &pxa_device_btuart.dev),
-	INIT_CKEN("UARTCLK", STUART, 14857000, 1, NULL),
+	INIT_CKEN("FFUARTCLK", FFUART, 14857000, 1),
+	INIT_CKEN("BTUARTCLK", BTUART, 14857000, 1),
+	INIT_CKEN("STUARTCLK", STUART, 14857000, 1),
 
-	INIT_CKEN("I2SCLK",  I2S,  14682000, 0, &pxa_device_i2s.dev),
-	INIT_CKEN("I2CCLK",  I2C,  32842000, 0, &pxa_device_i2c.dev),
-	INIT_CKEN("UDCCLK",  USB,  48000000, 5, &pxa_device_udc.dev),
-	INIT_CKEN("MMCCLK",  MMC,  19500000, 0, &pxa_device_mci.dev),
-	INIT_CKEN("FICPCLK", FICP, 48000000, 0, &pxa_device_ficp.dev),
+	INIT_CKEN("I2SCLK",  I2S,  14682000, 0),
+	INIT_CKEN("I2CCLK",  I2C,  32842000, 0),
+	INIT_CKEN("UDCCLK",  USB,  48000000, 5),
+	INIT_CKEN("PXAMMCCLK",  MMC,  19500000, 0),
+	INIT_CKEN("FICPCLK", FICP, 48000000, 0),
 
-	INIT_CKEN("USBCLK", USBHOST, 48000000, 0, &pxa27x_device_ohci.dev),
-	INIT_CKEN("I2CCLK", PWRI2C, 13000000, 0, &pxa27x_device_i2c_power.dev),
-	INIT_CKEN("KBDCLK", KEYPAD, 32768, 0, NULL),
+	INIT_CKEN("USBCLK", USBHOST, 48000000, 0),
+	INIT_CKEN("I2CCLK", PWRI2C, 13000000, 0),
+	INIT_CKEN("KBDCLK", KEYPAD, 32768, 0),
 
-	INIT_CKEN("SSPCLK", SSP1, 13000000, 0, &pxa27x_device_ssp1.dev),
-	INIT_CKEN("SSPCLK", SSP2, 13000000, 0, &pxa27x_device_ssp2.dev),
-	INIT_CKEN("SSPCLK", SSP3, 13000000, 0, &pxa27x_device_ssp3.dev),
+	INIT_CKEN("SSP1CLK", SSP1, 13000000, 0),
+	INIT_CKEN("SSP2CLK", SSP2, 13000000, 0),
+	INIT_CKEN("SSP3CLK", SSP3, 13000000, 0),
 
 	/*
-	INIT_CKEN("PWMCLK",  PWM0, 13000000, 0, NULL),
-	INIT_CKEN("MSLCLK",  MSL,  48000000, 0, NULL),
-	INIT_CKEN("USIMCLK", USIM, 48000000, 0, NULL),
-	INIT_CKEN("MSTKCLK", MEMSTK, 19500000, 0, NULL),
-	INIT_CKEN("IMCLK",   IM,   0, 0, NULL),
-	INIT_CKEN("MEMCLK",  MEMC, 0, 0, NULL),
+	INIT_CKEN("PWMCLK",  PWM0, 13000000, 0),
+	INIT_CKEN("MSLCLK",  MSL,  48000000, 0),
+	INIT_CKEN("USIMCLK", USIM, 48000000, 0),
+	INIT_CKEN("MSTKCLK", MEMSTK, 19500000, 0),
+	INIT_CKEN("IMCLK",   IM,   0, 0),
+	INIT_CKEN("MEMCLK",  MEMC, 0, 0),
 	*/
 };
 
+static struct clk_function pxa27x_clk_funcs[] = {
+	CLK_FUNC("FFUARTCLK", "UARTCLK", &clk_dev_ops, &pxa_device_ffuart.dev),
+	CLK_FUNC("BTUARTCLK", "UARTCLK", &clk_dev_ops, &pxa_device_btuart.dev),
+	CLK_FUNC("STUARTCLK", "UARTCLK", &clk_dev_ops, &pxa_device_stuart.dev),
+	CLK_FUNC("STUARTCLK", "SIRCLK", NULL, NULL),
+	CLK_FUNC("PXAMMCCLK", "MMCCLK", &clk_dev_ops, &pxa_device_mci.dev),
+	CLK_FUNC("SSP1CLK", "SSPCLK", &clk_dev_ops, &pxa27x_device_ssp1.dev),
+	CLK_FUNC("SSP2CLK", "SSPCLK", &clk_dev_ops, &pxa27x_device_ssp2.dev),
+	CLK_FUNC("SSP3CLK", "SSPCLK", &clk_dev_ops, &pxa27x_device_ssp3.dev),
+};
+
 #ifdef CONFIG_PM
 
 #define SAVE(x)		sleep_save[SLEEP_SAVE_##x] = x
@@ -401,7 +411,8 @@ static int __init pxa27x_init(void)
 	int i, ret = 0;
 
 	if (cpu_is_pxa27x()) {
-		clks_register(pxa27x_clks, ARRAY_SIZE(pxa27x_clks));
+		ret = clks_register(pxa27x_clks, ARRAY_SIZE(pxa27x_clks));
+		ret = clk_alloc_functions(pxa27x_clk_funcs, ARRAY_SIZE(pxa27x_clk_funcs));
 
 		if ((ret = pxa_init_dma(32)))
 			return ret;
diff --git a/arch/arm/mach-pxa/pxa3xx.c b/arch/arm/mach-pxa/pxa3xx.c
index 35f25fd..7a7cd3c 100644
--- a/arch/arm/mach-pxa/pxa3xx.c
+++ b/arch/arm/mach-pxa/pxa3xx.c
@@ -125,75 +125,90 @@ static unsigned long clk_pxa3xx_hsio_getrate(struct clk *clk)
 	return hsio_clk;
 }
 
-static void clk_pxa3xx_cken_enable(struct clk *clk)
+static int clk_pxa3xx_cken_set_mode(struct clk *clk, bool enable)
 {
-	unsigned long mask = 1ul << (clk->cken & 0x1f);
-
-	if (clk->cken < 32)
-		CKENA |= mask;
+	struct clk_cken *cclk = container_of(clk, struct clk_cken, clk);
+	int cken = cclk->cken;
+	unsigned long mask = 1ul << (cken & 0x1f);
+
+	if (cken < 32)
+		if (enable)
+			CKENA |= mask;
+		else
+			CKENA &= ~mask;
 	else
-		CKENB |= mask;
-}
-
-static void clk_pxa3xx_cken_disable(struct clk *clk)
-{
-	unsigned long mask = 1ul << (clk->cken & 0x1f);
+		if (enable)
+			CKENB |= mask;
+		else
+			CKENB &= ~mask;
 
-	if (clk->cken < 32)
-		CKENA &= ~mask;
-	else
-		CKENB &= ~mask;
+	return 0;
 }
 
-static const struct clkops clk_pxa3xx_cken_ops = {
-	.enable		= clk_pxa3xx_cken_enable,
-	.disable	= clk_pxa3xx_cken_disable,
+const struct clk_ops clk_pxa3xx_cken_ops = {
+	.set_mode = clk_pxa3xx_cken_set_mode,
+	.get_rate = clk_cken_get_rate,
 };
 
-static const struct clkops clk_pxa3xx_hsio_ops = {
-	.enable		= clk_pxa3xx_cken_enable,
-	.disable	= clk_pxa3xx_cken_disable,
-	.getrate	= clk_pxa3xx_hsio_getrate,
+static const struct clk_ops clk_pxa3xx_hsio_ops = {
+	.set_mode = clk_pxa3xx_cken_set_mode,
+	.get_rate = clk_pxa3xx_hsio_getrate,
 };
 
-#define PXA3xx_CKEN(_name, _cken, _rate, _delay, _dev)	\
-	{						\
-		.name	= _name,			\
-		.dev	= _dev,				\
-		.ops	= &clk_pxa3xx_cken_ops,		\
+#define PXA3xx_CKEN(_name, _cken, _rate, _delay)	\
+	&(&(struct clk_cken) {				\
+		.cken = CKEN_##_cken,			\
 		.rate	= _rate,			\
-		.cken	= CKEN_##_cken,			\
 		.delay	= _delay,			\
-	}
-
-#define PXA3xx_CK(_name, _cken, _ops, _dev)		\
-	{						\
-		.name	= _name,			\
-		.dev	= _dev,				\
-		.ops	= _ops,				\
-		.cken	= CKEN_##_cken,			\
-	}
-
-static struct clk pxa3xx_clks[] = {
-	PXA3xx_CK("LCDCLK", LCD,    &clk_pxa3xx_hsio_ops, &pxa_device_fb.dev),
-	PXA3xx_CK("CAMCLK", CAMERA, &clk_pxa3xx_hsio_ops, NULL),
-
-	PXA3xx_CKEN("UARTCLK", FFUART, 14857000, 1, &pxa_device_ffuart.dev),
-	PXA3xx_CKEN("UARTCLK", BTUART, 14857000, 1, &pxa_device_btuart.dev),
-	PXA3xx_CKEN("UARTCLK", STUART, 14857000, 1, NULL),
-
-	PXA3xx_CKEN("I2CCLK", I2C,  32842000, 0, &pxa_device_i2c.dev),
-	PXA3xx_CKEN("UDCCLK", UDC,  48000000, 5, &pxa_device_udc.dev),
-	PXA3xx_CKEN("USBCLK", USBH, 48000000, 0, &pxa27x_device_ohci.dev),
-
-	PXA3xx_CKEN("SSPCLK", SSP1, 13000000, 0, &pxa27x_device_ssp1.dev),
-	PXA3xx_CKEN("SSPCLK", SSP2, 13000000, 0, &pxa27x_device_ssp2.dev),
-	PXA3xx_CKEN("SSPCLK", SSP3, 13000000, 0, &pxa27x_device_ssp3.dev),
-	PXA3xx_CKEN("SSPCLK", SSP4, 13000000, 0, &pxa3xx_device_ssp4.dev),
+		.clk = {				\
+			.name	= _name,		\
+			.ops	= &clk_pxa3xx_cken_ops,	\
+		},					\
+	})->clk
+
+#define PXA3xx_CK(_name, _cken, _ops)			\
+	&(&(struct clk_cken) {				\
+		.cken = CKEN_##_cken,			\
+		.clk = {				\
+			.name	= _name,		\
+			.ops	= _ops,			\
+		},					\
+	})->clk
+
+static struct clk *pxa3xx_clks[] = {
+	PXA3xx_CK("LCDCLK", LCD,    &clk_pxa3xx_hsio_ops),
+	PXA3xx_CK("CAMCLK", CAMERA, &clk_pxa3xx_hsio_ops),
+
+	PXA3xx_CKEN("FFUARTCLK", FFUART, 14857000, 1),
+	PXA3xx_CKEN("BTUARTCLK", BTUART, 14857000, 1),
+	PXA3xx_CKEN("STUARTCLK", STUART, 14857000, 1),
+
+	PXA3xx_CKEN("I2CCLK", I2C,  32842000, 0),
+	PXA3xx_CKEN("UDCCLK", UDC,  48000000, 5),
+	PXA3xx_CKEN("USBCLK", USBH, 48000000, 0),
+
+	PXA3xx_CKEN("SSP1CLK", SSP1, 13000000, 0),
+	PXA3xx_CKEN("SSP2CLK", SSP2, 13000000, 0),
+	PXA3xx_CKEN("SSP3CLK", SSP3, 13000000, 0),
+	PXA3xx_CKEN("SSP4CLK", SSP4, 13000000, 0),
+
+	PXA3xx_CKEN("MMC1CLK", MMC1, 19500000, 0),
+	PXA3xx_CKEN("MMC2CLK", MMC2, 19500000, 0),
+	PXA3xx_CKEN("MMC3CLK", MMC3, 19500000, 0),
+};
 
-	PXA3xx_CKEN("MMCCLK", MMC1, 19500000, 0, &pxa_device_mci.dev),
-	PXA3xx_CKEN("MMCCLK", MMC2, 19500000, 0, &pxa3xx_device_mci2.dev),
-	PXA3xx_CKEN("MMCCLK", MMC3, 19500000, 0, &pxa3xx_device_mci3.dev),
+static struct clk_function pxa3xx_clk_funcs[] = {
+	CLK_FUNC("FFUARTCLK", "UARTCLK", &clk_dev_ops, &pxa_device_ffuart.dev),
+	CLK_FUNC("BTUARTCLK", "UARTCLK", &clk_dev_ops, &pxa_device_btuart.dev),
+	CLK_FUNC("STUARTCLK", "UARTCLK", &clk_dev_ops, &pxa_device_stuart.dev),
+	CLK_FUNC("STUARTCLK", "SIRCLK", NULL, NULL),
+	CLK_FUNC("SSP1CLK", "SSPCLK", &clk_dev_ops, &pxa27x_device_ssp1.dev),
+	CLK_FUNC("SSP2CLK", "SSPCLK", &clk_dev_ops, &pxa27x_device_ssp2.dev),
+	CLK_FUNC("SSP3CLK", "SSPCLK", &clk_dev_ops, &pxa27x_device_ssp3.dev),
+	CLK_FUNC("SSP4CLK", "SSPCLK", &clk_dev_ops, &pxa3xx_device_ssp4.dev),
+	CLK_FUNC("MMC1CLK", "MMCCLK", &clk_dev_ops, &pxa_device_mci.dev),
+	CLK_FUNC("MMC2CLK", "MMCCLK", &clk_dev_ops, &pxa3xx_device_mci2.dev),
+	CLK_FUNC("MMC3CLK", "MMCCLK", &clk_dev_ops, &pxa3xx_device_mci3.dev),
 };
 
 #ifdef CONFIG_PM
@@ -513,7 +528,8 @@ static int __init pxa3xx_init(void)
 		 */
 		ASCR &= ~(ASCR_RDH | ASCR_D1S | ASCR_D2S | ASCR_D3S);
 
-		clks_register(pxa3xx_clks, ARRAY_SIZE(pxa3xx_clks));
+		ret = clks_register(pxa3xx_clks, ARRAY_SIZE(pxa3xx_clks));
+		ret = clk_alloc_functions(pxa3xx_clk_funcs, ARRAY_SIZE(pxa3xx_clk_funcs));
 
 		if ((ret = pxa_init_dma(32)))
 			return ret;
-- 
1.5.4.4


-- 
With best wishes
Dmitry

[PATCH 5/6] Clocklib: Use correct clock for IrDA on pxa

[Dmitry Baryshkov]

Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
---
 drivers/net/irda/pxaficp_ir.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/irda/pxaficp_ir.c b/drivers/net/irda/pxaficp_ir.c
index 8c09344..36d2ec0 100644
--- a/drivers/net/irda/pxaficp_ir.c
+++ b/drivers/net/irda/pxaficp_ir.c
@@ -814,7 +814,7 @@ static int pxa_irda_probe(struct platform_device *pdev)
 	si->dev = &pdev->dev;
 	si->pdata = pdev->dev.platform_data;
 
-	si->sir_clk = clk_get(&pdev->dev, "UARTCLK");
+	si->sir_clk = clk_get(&pdev->dev, "SIRCLK");
 	si->fir_clk = clk_get(&pdev->dev, "FICPCLK");
 	if (IS_ERR(si->sir_clk) || IS_ERR(si->fir_clk)) {
 		err = PTR_ERR(IS_ERR(si->sir_clk) ? si->sir_clk : si->fir_clk);
-- 
1.5.4.4


-- 
With best wishes
Dmitry

Re: [PATCH 5/6] Clocklib: Use correct clock for IrDA on pxa

[Andrew Morton]

On Thu, 3 Apr 2008 17:24:02 +0400
Dmitry Baryshkov <dbaryshkov@gmail.com> wrote:

> Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
> ---
>  drivers/net/irda/pxaficp_ir.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/irda/pxaficp_ir.c b/drivers/net/irda/pxaficp_ir.c
> index 8c09344..36d2ec0 100644
> --- a/drivers/net/irda/pxaficp_ir.c
> +++ b/drivers/net/irda/pxaficp_ir.c
> @@ -814,7 +814,7 @@ static int pxa_irda_probe(struct platform_device *pdev)
>  	si->dev = &pdev->dev;
>  	si->pdata = pdev->dev.platform_data;
>  
> -	si->sir_clk = clk_get(&pdev->dev, "UARTCLK");
> +	si->sir_clk = clk_get(&pdev->dev, "SIRCLK");
>  	si->fir_clk = clk_get(&pdev->dev, "FICPCLK");
>  	if (IS_ERR(si->sir_clk) || IS_ERR(si->fir_clk)) {
>  		err = PTR_ERR(IS_ERR(si->sir_clk) ? si->sir_clk : si->fir_clk);

When fixing a bug, please describe what the bug is, and how the patch fixes
it.

A patch needs to be very very obvious to be able to get away with no
changelog, and this one is not very very obvious.

Re: [PATCH 5/6] Clocklib: Use correct clock for IrDA on pxa

[Russell King]

On Mon, Apr 07, 2008 at 04:00:29PM -0700, Andrew Morton wrote:
> On Thu, 3 Apr 2008 17:24:02 +0400
> Dmitry Baryshkov <dbaryshkov@gmail.com> wrote:
> 
> > Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
> > ---
> >  drivers/net/irda/pxaficp_ir.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/net/irda/pxaficp_ir.c b/drivers/net/irda/pxaficp_ir.c
> > index 8c09344..36d2ec0 100644
> > --- a/drivers/net/irda/pxaficp_ir.c
> > +++ b/drivers/net/irda/pxaficp_ir.c
> > @@ -814,7 +814,7 @@ static int pxa_irda_probe(struct platform_device *pdev)
> >  	si->dev = &pdev->dev;
> >  	si->pdata = pdev->dev.platform_data;
> >  
> > -	si->sir_clk = clk_get(&pdev->dev, "UARTCLK");
> > +	si->sir_clk = clk_get(&pdev->dev, "SIRCLK");
> >  	si->fir_clk = clk_get(&pdev->dev, "FICPCLK");
> >  	if (IS_ERR(si->sir_clk) || IS_ERR(si->fir_clk)) {
> >  		err = PTR_ERR(IS_ERR(si->sir_clk) ? si->sir_clk : si->fir_clk);
> 
> When fixing a bug, please describe what the bug is, and how the patch fixes
> it.
> 
> A patch needs to be very very obvious to be able to get away with no
> changelog, and this one is not very very obvious.

It's not a bug fix - it's a stupidity of the clock lib that the API
doesn't really require.

I've not made up my mind what to do with this stuff, but if it requires
drivers to work around its short comings, then I'm not in favour of it.

(We have the above code working as is.)

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH 5/6] Clocklib: Use correct clock for IrDA on pxa

[Dmitry]

Hi,

2008/4/8, Russell King <rmk+lkml@arm.linux.org.uk>:
> On Mon, Apr 07, 2008 at 04:00:29PM -0700, Andrew Morton wrote:
>  > On Thu, 3 Apr 2008 17:24:02 +0400
>  > Dmitry Baryshkov <dbaryshkov@gmail.com> wrote:
>  >
>  > > Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
>  > > ---
>  > >  drivers/net/irda/pxaficp_ir.c |    2 +-
>  > >  1 files changed, 1 insertions(+), 1 deletions(-)
>  > >
>  > > diff --git a/drivers/net/irda/pxaficp_ir.c b/drivers/net/irda/pxaficp_ir.c
>  > > index 8c09344..36d2ec0 100644
>  > > --- a/drivers/net/irda/pxaficp_ir.c
>  > > +++ b/drivers/net/irda/pxaficp_ir.c
>  > > @@ -814,7 +814,7 @@ static int pxa_irda_probe(struct platform_device *pdev)
>  > >     si->dev = &pdev->dev;
>  > >     si->pdata = pdev->dev.platform_data;
>  > >
>  > > -   si->sir_clk = clk_get(&pdev->dev, "UARTCLK");
>  > > +   si->sir_clk = clk_get(&pdev->dev, "SIRCLK");
>  > >     si->fir_clk = clk_get(&pdev->dev, "FICPCLK");
>  > >     if (IS_ERR(si->sir_clk) || IS_ERR(si->fir_clk)) {
>  > >             err = PTR_ERR(IS_ERR(si->sir_clk) ? si->sir_clk : si->fir_clk);
>  >
>  > When fixing a bug, please describe what the bug is, and how the patch fixes
>  > it.
>  >
>  > A patch needs to be very very obvious to be able to get away with no
>  > changelog, and this one is not very very obvious.
>
>
> It's not a bug fix - it's a stupidity of the clock lib that the API
>  doesn't really require.
>
>  I've not made up my mind what to do with this stuff, but if it requires
>  drivers to work around its short comings, then I'm not in favour of it.
>
>  (We have the above code working as is.)

Yes, it works currently. But there are a few problems: we declare
STUART's UARTCLK with dev=NULL (all other UARTCLKs are declared with
proper devices).
Therefore,  I consider it as a hack and would like to remove it. As
the clocklib allows
to register multiple children clocks with different names, I'd propose
to separate UARTCLK
and SIRCLK functions of STUART clock. The UARTCLK will be used (as
usual) by the pxa uart driver. And IrDA will use SIRCLK.

-- 
With best wishes
Dmitry

Re: [PATCH 5/6] Clocklib: Use correct clock for IrDA on pxa

[Russell King]

On Tue, Apr 08, 2008 at 01:47:35PM +0400, Dmitry wrote:
> Yes, it works currently. But there are a few problems: we declare
> STUART's UARTCLK with dev=NULL (all other UARTCLKs are declared with
> proper devices).  Therefore,  I consider it as a hack and would like
> to remove it.

I don't consider it a hack at all - it's a work around for the fact
that the PXA FIR driver shares the UART, but the FIR driver doesn't
bind to the UART itself.

The _real_ issue is with IrDA itself, and is larger than just the
clock library.  Any serial port which supports IrDA, even on x86,
has to be shared between the serial driver and the IrDA driver -
there's no way for them to quietly co-exist and "just work" as
requested.

So, let's not work around the short comings of Serial/IrDA interactions
by adding additional complexity to random other layers which _shouldn't_
even be seeing the issue.

In addition, the point of the clock framework is that you ask for the
device plus clock NAME on _that_ device.  Inventing random other names
for the same physical clock on the same physical device is just nonsense -
even more so than the existing workaround.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH 5/6] Clocklib: Use correct clock for IrDA on pxa

[Dmitry]

Hi,

2008/4/8, Russell King <rmk+lkml@arm.linux.org.uk>:
> On Tue, Apr 08, 2008 at 01:47:35PM +0400, Dmitry wrote:
>  > Yes, it works currently. But there are a few problems: we declare
>  > STUART's UARTCLK with dev=NULL (all other UARTCLKs are declared with
>  > proper devices).  Therefore,  I consider it as a hack and would like
>  > to remove it.
>
>
> I don't consider it a hack at all - it's a work around for the fact
>  that the PXA FIR driver shares the UART, but the FIR driver doesn't
>  bind to the UART itself.

Would you then accept the patch that still contains UARTCLK bound to
pxa uart device, and IrDA requesting clock STUARTCLK?

>  The _real_ issue is with IrDA itself, and is larger than just the
>  clock library.  Any serial port which supports IrDA, even on x86,
>  has to be shared between the serial driver and the IrDA driver -
>  there's no way for them to quietly co-exist and "just work" as
>  requested.

Yes. I wonder how this is solved in other platforms.
>
>  So, let's not work around the short comings of Serial/IrDA interactions
>  by adding additional complexity to random other layers which _shouldn't_
>  even be seeing the issue.
>
>  In addition, the point of the clock framework is that you ask for the
>  device plus clock NAME on _that_ device.  Inventing random other names
>  for the same physical clock on the same physical device is just nonsense -
>  even more so than the existing workaround.

See my proposition above. I highly dislike the UARTCLK w/o device declared.
Once it has already lead me to (small) problems due to messed other
UARTCLKs  declarations on pxa25x.

-- 
With best wishes
Dmitry

Re: [PATCH 5/6] Clocklib: Use correct clock for IrDA on pxa

[Russell King]

On Wed, Apr 09, 2008 at 11:15:51AM +0400, Dmitry wrote:
> 2008/4/8, Russell King <rmk+lkml@arm.linux.org.uk>:
> > On Tue, Apr 08, 2008 at 01:47:35PM +0400, Dmitry wrote:
> >  > Yes, it works currently. But there are a few problems: we declare
> >  > STUART's UARTCLK with dev=NULL (all other UARTCLKs are declared with
> >  > proper devices).  Therefore,  I consider it as a hack and would like
> >  > to remove it.
> >
> > I don't consider it a hack at all - it's a work around for the fact
> >  that the PXA FIR driver shares the UART, but the FIR driver doesn't
> >  bind to the UART itself.
> 
> Would you then accept the patch that still contains UARTCLK bound to
> pxa uart device, and IrDA requesting clock STUARTCLK?

See below - fixing the underlying issue makes this problem vanish.

> >  The _real_ issue is with IrDA itself, and is larger than just the
> >  clock library.  Any serial port which supports IrDA, even on x86,
> >  has to be shared between the serial driver and the IrDA driver -
> >  there's no way for them to quietly co-exist and "just work" as
> >  requested.
> 
> Yes. I wonder how this is solved in other platforms.

As I've suggested, it isn't.  On x86, for instance, you have to use
setserial to deconfigure the serial port from the serial driver (by
setserial /dev/ttyS1 uart none) and then load the IrDA driver.

Unfortunately, there's currently no way to properly hand off a real
serial device to the IrDA layer when the UART is connected to an IrDA
interface.

TBH, I'm not sure what the status of the kernel-side IrDA drivers are -
maybe this is an issue which Samuel Ortiz could tackle if he has time.
That would solve this issue on both 8250-based ports as well as SA11x0
and PXA platforms.

Basically what I'm thinking is a serial_core function which could be
called to say "I'm an IrDA driver, and I think I should be using the
serial port located <here>, please give me control of it" and it'd
pass over the various parameters including the struct device for it.
Or something like that.

> See my proposition above. I highly dislike the UARTCLK w/o device declared.
> Once it has already lead me to (small) problems due to messed other
> UARTCLKs  declarations on pxa25x.

If we get the underlying issue fixed, that issue magically goes away.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH 5/6] Clocklib: Use correct clock for IrDA on pxa

[Alan Cox]

> TBH, I'm not sure what the status of the kernel-side IrDA drivers are -
> maybe this is an issue which Samuel Ortiz could tackle if he has time.
> That would solve this issue on both 8250-based ports as well as SA11x0
> and PXA platforms.

I think "in need of much love" would be a polite way to put it and some
of them are probably going to break soon as they pretend to be tty
devices but fake it badly.

> Basically what I'm thinking is a serial_core function which could be
> called to say "I'm an IrDA driver, and I think I should be using the
> serial port located <here>, please give me control of it" and it'd
> pass over the various parameters including the struct device for it.
> Or something like that.

It's called a line discipline, we've had them for many years. We may need
a way for ldiscs and drivers to co-operate a bit more but these days we
support proper buffering and arbitary baud rates (except on a few
platforms whose maintainers are not paying attention ;)).

So I don't actually see why the FIR layer can't become a good citizen.

Alan

Re: [PATCH 5/6] Clocklib: Use correct clock for IrDA on pxa

[Russell King]

On Wed, Apr 09, 2008 at 08:09:16PM +0100, Alan Cox wrote:
> > Basically what I'm thinking is a serial_core function which could be
> > called to say "I'm an IrDA driver, and I think I should be using the
> > serial port located <here>, please give me control of it" and it'd
> > pass over the various parameters including the struct device for it.
> > Or something like that.
> 
> It's called a line discipline, we've had them for many years. We may need
> a way for ldiscs and drivers to co-operate a bit more but these days we
> support proper buffering and arbitary baud rates (except on a few
> platforms whose maintainers are not paying attention ;)).

I feel that's a "it would be nice if" solution - and something worth
aiming for, but the amount of work required to get there is not going
to be insignificant.

Maybe Dmitry can bite his nails on the NULL device clklib issue for a
while as this transition is happening ?  (Though we need to find someone
to make it happen... we need volunteers!)

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH 5/6] Clocklib: Use correct clock for IrDA on pxa

[Dmitry]

Hi, Russell,

2008/4/9, Russell King <rmk+lkml@arm.linux.org.uk>:
> On Wed, Apr 09, 2008 at 08:09:16PM +0100, Alan Cox wrote:
>  > > Basically what I'm thinking is a serial_core function which could be
>  > > called to say "I'm an IrDA driver, and I think I should be using the
>  > > serial port located <here>, please give me control of it" and it'd
>  > > pass over the various parameters including the struct device for it.
>  > > Or something like that.
>  >
>  > It's called a line discipline, we've had them for many years. We may need
>  > a way for ldiscs and drivers to co-operate a bit more but these days we
>  > support proper buffering and arbitary baud rates (except on a few
>  > platforms whose maintainers are not paying attention ;)).

That would be a good thing to have.

> I feel that's a "it would be nice if" solution - and something worth
>  aiming for, but the amount of work required to get there is not going
>  to be insignificant.
>
>  Maybe Dmitry can bite his nails on the NULL device clklib issue for a
>  while as this transition is happening ?  (Though we need to find someone
>  to make it happen... we need volunteers!)

Russell, I'd propose to agree to bind PXA's IrDA to STUARTCLK and
leave UARTCLK function to real pxa uart driver. If you would oppose, I
won't have any choises other that to agree with it.

-- 
With best wishes
Dmitry

Re: [PATCH 5/6] Clocklib: Use correct clock for IrDA on pxa

[Alan Cox]

> > It's called a line discipline, we've had them for many years. We may need
> > a way for ldiscs and drivers to co-operate a bit more but these days we
> > support proper buffering and arbitary baud rates (except on a few
> > platforms whose maintainers are not paying attention ;)).
> 
> I feel that's a "it would be nice if" solution - and something worth
> aiming for, but the amount of work required to get there is not going
> to be insignificant.

The work required to fix up the existing FIR hacks is not insignifcant
either. Also right now the tty layer is getting a major rework so now is
actually the time to sort out anything extra that is needed.

If you want 4MBit please just use an ldisc and do

	struct ktermios tmp;
	mutex_lock(&tty->termios_mutex);
	tmp = *tty->termios;
	tty_encode_baud_rate(tty, 4000000, 4000000);
	tty->driver->set_termios(tty, &tmp);
	mutex_unlock(&tty->termios_mutex);

Alan

Re: [PATCH 5/6] Clocklib: Use correct clock for IrDA on pxa

[Russell King]

On Wed, Apr 09, 2008 at 09:52:33PM +0100, Alan Cox wrote:
> > > It's called a line discipline, we've had them for many years. We may need
> > > a way for ldiscs and drivers to co-operate a bit more but these days we
> > > support proper buffering and arbitary baud rates (except on a few
> > > platforms whose maintainers are not paying attention ;)).
> > 
> > I feel that's a "it would be nice if" solution - and something worth
> > aiming for, but the amount of work required to get there is not going
> > to be insignificant.
> 
> The work required to fix up the existing FIR hacks is not insignifcant
> either. Also right now the tty layer is getting a major rework so now is
> actually the time to sort out anything extra that is needed.
> 
> If you want 4MBit please just use an ldisc and do
> 
> 	struct ktermios tmp;
> 	mutex_lock(&tty->termios_mutex);
> 	tmp = *tty->termios;
> 	tty_encode_baud_rate(tty, 4000000, 4000000);
> 	tty->driver->set_termios(tty, &tmp);
> 	mutex_unlock(&tty->termios_mutex);

There's more to FIR than just a baud rate change.  On PXA for instance,
SIR is implemented using the standard UART device in "SIR" mode, but FIR
is a completely separate hardware block with its own IRQs and clocks -
you need to switch the pin muxing from the UART to the FIR device.

So it's not just a matter of setting the baud rate to 4Mbps.

However, you can't just say "have two separate drivers and only use
one or the other" - all IrDA link negotiation is done at SIR at 9600
baud and only when negotiation is complete will the selected rate
become effective - be that SIR or FIR based.

So yes, using a ldisc for SIR (with a hook into the driver to tell the
driver to setup the port for IR) sounds ideal, but we're still going to
need to deal with the FIR device and switch IrDA between that and the
SIR UART ldisc.

Note - there are other reasons for finally sorting this out as well -
there are systems with IR which want to support both IrDA up to FIR
and other uart-based IR applications.  IIRC this came up on the iPAQs.
I forget exactly which applications but I believe lirc might fall into
the "want a serial port not the FIR network device interface" class.
Having SIR always go via ldiscs should sort that out nicely as well.
(I'm thinking maybe we want some control to set a port into IR
transmission/reception mode independent of selecting the IrDA ldisc -
but I'd suggest further research first - I may be just handwaving
about an already solved problem.)

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

[PATCH 6/6] Clocklib: use correct name for 3,6MHz clock

[Dmitry Baryshkov]

Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
---
 arch/arm/common/sa1111.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/common/sa1111.c b/arch/arm/common/sa1111.c
index eb06d0b..282a4d9 100644
--- a/arch/arm/common/sa1111.c
+++ b/arch/arm/common/sa1111.c
@@ -627,7 +627,7 @@ __sa1111_probe(struct device *me, struct resource *mem, int irq)
 	if (!sachip)
 		return -ENOMEM;
 
-	sachip->clk = clk_get(me, "GPIO27_CLK");
+	sachip->clk = clk_get(me, "3_6MHz_CLK");
 	if (!sachip->clk) {
 		ret = PTR_ERR(sachip->clk);
 		goto err_free;
-- 
1.5.4.4


-- 
With best wishes
Dmitry

Re: [PATCH 6/6] Clocklib: use correct name for 3,6MHz clock

[Andrew Morton]

On Thu, 3 Apr 2008 17:24:11 +0400
Dmitry Baryshkov <dbaryshkov@gmail.com> wrote:

> Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
> ---
>  arch/arm/common/sa1111.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/common/sa1111.c b/arch/arm/common/sa1111.c
> index eb06d0b..282a4d9 100644
> --- a/arch/arm/common/sa1111.c
> +++ b/arch/arm/common/sa1111.c
> @@ -627,7 +627,7 @@ __sa1111_probe(struct device *me, struct resource *mem, int irq)
>  	if (!sachip)
>  		return -ENOMEM;
>  
> -	sachip->clk = clk_get(me, "GPIO27_CLK");
> +	sachip->clk = clk_get(me, "3_6MHz_CLK");
>  	if (!sachip->clk) {
>  		ret = PTR_ERR(sachip->clk);
>  		goto err_free;

Again, there's just not enough information for us (well: me) to be able to
evaluate this patch.

For example, if the current name is "incorrect" then why shouldn't we fix
it in 2.6.25?  2.6.24?  etc.

Re: [PATCH 6/6] Clocklib: use correct name for 3,6MHz clock

[Russell King]

On Mon, Apr 07, 2008 at 04:01:55PM -0700, Andrew Morton wrote:
> On Thu, 3 Apr 2008 17:24:11 +0400
> Dmitry Baryshkov <dbaryshkov@gmail.com> wrote:
> > Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
> > ---
> >  arch/arm/common/sa1111.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/arch/arm/common/sa1111.c b/arch/arm/common/sa1111.c
> > index eb06d0b..282a4d9 100644
> > --- a/arch/arm/common/sa1111.c
> > +++ b/arch/arm/common/sa1111.c
> > @@ -627,7 +627,7 @@ __sa1111_probe(struct device *me, struct resource *mem, int irq)
> >  	if (!sachip)
> >  		return -ENOMEM;
> >  
> > -	sachip->clk = clk_get(me, "GPIO27_CLK");
> > +	sachip->clk = clk_get(me, "3_6MHz_CLK");
> >  	if (!sachip->clk) {
> >  		ret = PTR_ERR(sachip->clk);
> >  		goto err_free;
> 
> Again, there's just not enough information for us (well: me) to be able to
> evaluate this patch.
> 
> For example, if the current name is "incorrect" then why shouldn't we fix
> it in 2.6.25?  2.6.24?  etc.

I don't see any reason for this change.  Except maybe someone wanted
a nicer name to be exposed to userland.  I don't see the point of
exposing what's supposed to be a kernel _internal_ API to userland
and then having an issue with those names being exported there.

To put it another way: I don't ever want to have to think about
userland issues when dealing with device clocking interfaces.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH 6/6] Clocklib: use correct name for 3,6MHz clock

[Dmitry]

2008/4/8, Andrew Morton <akpm@linux-foundation.org>:
> On Thu, 3 Apr 2008 17:24:11 +0400
>  Dmitry Baryshkov <dbaryshkov@gmail.com> wrote:
>
>  > Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
>  > ---
>  >  arch/arm/common/sa1111.c |    2 +-
>  >  1 files changed, 1 insertions(+), 1 deletions(-)
>  >
>  > diff --git a/arch/arm/common/sa1111.c b/arch/arm/common/sa1111.c
>  > index eb06d0b..282a4d9 100644
>  > --- a/arch/arm/common/sa1111.c
>  > +++ b/arch/arm/common/sa1111.c
>  > @@ -627,7 +627,7 @@ __sa1111_probe(struct device *me, struct resource *mem, int irq)
>  >       if (!sachip)
>  >               return -ENOMEM;
>  >
>  > -     sachip->clk = clk_get(me, "GPIO27_CLK");
>  > +     sachip->clk = clk_get(me, "3_6MHz_CLK");
>  >       if (!sachip->clk) {
>  >               ret = PTR_ERR(sachip->clk);
>  >               goto err_free;
>
>
> Again, there's just not enough information for us (well: me) to be able to
>  evaluate this patch.
>
>  For example, if the current name is "incorrect" then why shouldn't we fix
>  it in 2.6.25?  2.6.24?  etc.
>
>

The name GPIO27_CLK came from sa1100 arm sub-arch. There the 3.6 MHz
clock was provided via GPIO 27. The PXA clocks code have copied the
name for the clock (as it's used by sa1111 companion chip that can be
used with both sa1100 and pxa). However on pxa the 3.6MHz clock is
provided by different PIN. So the name GPIO27_CLK is misleading and
incorrect for PXA.

In my patchset I've changed the name of the clock to correct
GPIO11_CLK and have made both sa1100 and pxa provide common clock name
("3_6MHz_CLK").

This isn't a matter of userspace interfaces or communication. It's a
matter of logic and self-correctness.

If you wish I can put this information in the patch header.

-- 
With best wishes
Dmitry

Re: [PATCH 6/6] Clocklib: use correct name for 3,6MHz clock

[Russell King]

On Tue, Apr 08, 2008 at 01:52:41PM +0400, Dmitry wrote:
> 2008/4/8, Andrew Morton <akpm@linux-foundation.org>:
> > On Thu, 3 Apr 2008 17:24:11 +0400
> >  Dmitry Baryshkov <dbaryshkov@gmail.com> wrote:
> >
> >  > Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
> >  > ---
> >  >  arch/arm/common/sa1111.c |    2 +-
> >  >  1 files changed, 1 insertions(+), 1 deletions(-)
> >  >
> >  > diff --git a/arch/arm/common/sa1111.c b/arch/arm/common/sa1111.c
> >  > index eb06d0b..282a4d9 100644
> >  > --- a/arch/arm/common/sa1111.c
> >  > +++ b/arch/arm/common/sa1111.c
> >  > @@ -627,7 +627,7 @@ __sa1111_probe(struct device *me, struct resource *mem, int irq)
> >  >       if (!sachip)
> >  >               return -ENOMEM;
> >  >
> >  > -     sachip->clk = clk_get(me, "GPIO27_CLK");
> >  > +     sachip->clk = clk_get(me, "3_6MHz_CLK");
> >  >       if (!sachip->clk) {
> >  >               ret = PTR_ERR(sachip->clk);
> >  >               goto err_free;
> >
> >
> > Again, there's just not enough information for us (well: me) to be able to
> >  evaluate this patch.
> >
> >  For example, if the current name is "incorrect" then why shouldn't we fix
> >  it in 2.6.25?  2.6.24?  etc.
> >
> >
> 
> The name GPIO27_CLK came from sa1100 arm sub-arch. There the 3.6 MHz
> clock was provided via GPIO 27. The PXA clocks code have copied the
> name for the clock (as it's used by sa1111 companion chip that can be
> used with both sa1100 and pxa). However on pxa the 3.6MHz clock is
> provided by different PIN. So the name GPIO27_CLK is misleading and
> incorrect for PXA.

So... what is the correct name.  Bear in mind what I said in the previous
reply this evening - which says that it should be the name used by the
SA1111.  Look in the data sheet - the pin itself to which the 3.6MHz
clock is supplised will have a name.  That's the name which should be
used.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH 6/6] Clocklib: use correct name for 3,6MHz clock

[Dmitry]

Hi,

2008/4/8, Russell King <rmk+lkml@arm.linux.org.uk>:
> On Tue, Apr 08, 2008 at 01:52:41PM +0400, Dmitry wrote:
>  > 2008/4/8, Andrew Morton <akpm@linux-foundation.org>:
>  > > On Thu, 3 Apr 2008 17:24:11 +0400
>  > >  Dmitry Baryshkov <dbaryshkov@gmail.com> wrote:
>  > >
>  > >  > Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
>  > >  > ---
>  > >  >  arch/arm/common/sa1111.c |    2 +-
>  > >  >  1 files changed, 1 insertions(+), 1 deletions(-)
>  > >  >
>  > >  > diff --git a/arch/arm/common/sa1111.c b/arch/arm/common/sa1111.c
>  > >  > index eb06d0b..282a4d9 100644
>  > >  > --- a/arch/arm/common/sa1111.c
>  > >  > +++ b/arch/arm/common/sa1111.c
>  > >  > @@ -627,7 +627,7 @@ __sa1111_probe(struct device *me, struct resource *mem, int irq)
>  > >  >       if (!sachip)
>  > >  >               return -ENOMEM;
>  > >  >
>  > >  > -     sachip->clk = clk_get(me, "GPIO27_CLK");
>  > >  > +     sachip->clk = clk_get(me, "3_6MHz_CLK");
>  > >  >       if (!sachip->clk) {
>  > >  >               ret = PTR_ERR(sachip->clk);
>  > >  >               goto err_free;
>  > >
>  > >
>  > > Again, there's just not enough information for us (well: me) to be able to
>  > >  evaluate this patch.
>  > >
>  > >  For example, if the current name is "incorrect" then why shouldn't we fix
>  > >  it in 2.6.25?  2.6.24?  etc.
>  > >
>  > >
>  >
>  > The name GPIO27_CLK came from sa1100 arm sub-arch. There the 3.6 MHz
>  > clock was provided via GPIO 27. The PXA clocks code have copied the
>  > name for the clock (as it's used by sa1111 companion chip that can be
>  > used with both sa1100 and pxa). However on pxa the 3.6MHz clock is
>  > provided by different PIN. So the name GPIO27_CLK is misleading and
>  > incorrect for PXA.
>
>
> So... what is the correct name.  Bear in mind what I said in the previous
>  reply this evening - which says that it should be the name used by the
>  SA1111.  Look in the data sheet - the pin itself to which the 3.6MHz
>  clock is supplised will have a name.  That's the name which should be
>  used.

I use the same pin/clock for the tc6393xb driver. And I'm pretty sure
the datasheets won't agree on the name of the pin. Which name should I
use?


-- 
With best wishes
Dmitry

Re: [PATCH 6/6] Clocklib: use correct name for 3,6MHz clock

[Russell King]

On Tue, Apr 08, 2008 at 11:58:11PM +0400, Dmitry wrote:
> 2008/4/8, Russell King <rmk+lkml@arm.linux.org.uk>:
> > So... what is the correct name.  Bear in mind what I said in the previous
> >  reply this evening - which says that it should be the name used by the
> >  SA1111.  Look in the data sheet - the pin itself to which the 3.6MHz
> >  clock is supplised will have a name.  That's the name which should be
> >  used.
> 
> I use the same pin/clock for the tc6393xb driver. And I'm pretty sure
> the datasheets won't agree on the name of the pin. Which name should I
> use?

You missed the fundamental issue about the clock API - the _name_ is
not the clock name defined by the host.  It's the _device_ clock name.

So, you shouldn't be using the SA1111 clock name for the tc6393xb driver.
You should be using its own name.  The platform specific bit of the clock
API is then supposed to return you the struct clk corresponding with
that input, by using the platform knowledge that it's connected to GPIO27
or whatever.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH 6/6] Clocklib: use correct name for 3,6MHz clock

[Dmitry]

Hi,

2008/4/9, Russell King <rmk+lkml@arm.linux.org.uk>:
> On Tue, Apr 08, 2008 at 11:58:11PM +0400, Dmitry wrote:
>  > 2008/4/8, Russell King <rmk+lkml@arm.linux.org.uk>:
>
> > > So... what is the correct name.  Bear in mind what I said in the previous
>  > >  reply this evening - which says that it should be the name used by the
>  > >  SA1111.  Look in the data sheet - the pin itself to which the 3.6MHz
>  > >  clock is supplised will have a name.  That's the name which should be
>  > >  used.
>  >
>  > I use the same pin/clock for the tc6393xb driver. And I'm pretty sure
>  > the datasheets won't agree on the name of the pin. Which name should I
>  > use?
>
>
> You missed the fundamental issue about the clock API - the _name_ is
>  not the clock name defined by the host.  It's the _device_ clock name.
>
>  So, you shouldn't be using the SA1111 clock name for the tc6393xb driver.
>  You should be using its own name.  The platform specific bit of the clock
>  API is then supposed to return you the struct clk corresponding with
>  that input, by using the platform knowledge that it's connected to GPIO27
>  or whatever.

I think I understood your idea. I'll repost updated patchset later.

-- 
With best wishes
Dmitry

Re: [PATCH 6/6] Clocklib: use correct name for 3,6MHz clock

[Dmitry Baryshkov]

Hi,

On Tue, Apr 08, 2008 at 09:07:21PM +0100, Russell King wrote:
> On Tue, Apr 08, 2008 at 11:58:11PM +0400, Dmitry wrote:
> > 2008/4/8, Russell King <rmk+lkml@arm.linux.org.uk>:
> > > So... what is the correct name.  Bear in mind what I said in the previous
> > >  reply this evening - which says that it should be the name used by the
> > >  SA1111.  Look in the data sheet - the pin itself to which the 3.6MHz
> > >  clock is supplised will have a name.  That's the name which should be
> > >  used.
> > 
> > I use the same pin/clock for the tc6393xb driver. And I'm pretty sure
> > the datasheets won't agree on the name of the pin. Which name should I
> > use?
> 
> You missed the fundamental issue about the clock API - the _name_ is
> not the clock name defined by the host.  It's the _device_ clock name.
> 
> So, you shouldn't be using the SA1111 clock name for the tc6393xb driver.
> You should be using its own name.  The platform specific bit of the clock
> API is then supposed to return you the struct clk corresponding with
> that input, by using the platform knowledge that it's connected to GPIO27
> or whatever.

BTW: Who should know that there should be a clock for a device?
E.g. on pxa, 3.6MHz clock is registered in clock.c, various CKEN-based
clocks are registered from pxa*.c. Which file should contain
host_name -> device_name allocation board config?


And BTW2: The sa1111 spec names the clock simply as "CLK". Should we use
this name?


-- 
With best wishes
Dmitry

[PATCH 0/6] Clocklib: generic clocks framework

[Dmitry Baryshkov]

Hi,

Here is a new version based upon suggestions from Haavard and Russell.

Changes since last post:
* Split struct clk into clk and clk_ops
* Hide most non-generic things into clock type specific structs wrapping
  struct clk.
* Provide at least partial documentation for structures and fields.
* Better /sys/kernel/debug/clock formatting
* Incorporates sa1100 and PXA patches.

-- 
With best wishes
Dmitry

[PATCH 2/6] Clocklib: debugfs support

[Dmitry Baryshkov]

Provide /sys/kernel/debug/clock to ease debugging.

Signed-off-by: Dmitry Baryshkov <dbaryshkov@gmail.com>
---
 include/linux/clklib.h |    2 +
 kernel/clklib.c        |   74 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+), 0 deletions(-)

diff --git a/include/linux/clklib.h b/include/linux/clklib.h
index 88ab2c1..9f1c636 100644
--- a/include/linux/clklib.h
+++ b/include/linux/clklib.h
@@ -19,6 +19,7 @@ struct seq_file;
  * @get_rate: obtain the current clock rate of a specified clock
  * @set_rate: set the clock rate for a specified clock
  * @round_rate: adjust a reate to the exact rate a clock can provide
+ * @format: output any additional information for a clock
  *
  * This structure specifies clock operations that are used to configure
  * specific clock.
@@ -30,6 +31,7 @@ struct clk_ops {
 	unsigned long (*get_rate) (struct clk *clk);
 	int (*set_rate)  (struct clk *, unsigned long);
 	long (*round_rate) (struct clk *, unsigned long);
+	int (*format)	(struct clk *, struct seq_file *);
 };
 
 /**
diff --git a/kernel/clklib.c b/kernel/clklib.c
index 012f845..8d872e1 100644
--- a/kernel/clklib.c
+++ b/kernel/clklib.c
@@ -324,3 +324,77 @@ out:
 	return rc;
 }
 EXPORT_SYMBOL(clk_alloc_function);
+
+#ifdef CONFIG_DEBUG_FS
+
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
+
+#define NAME_FIELD_LEN	20
+
+static void dump_clocks(struct seq_file *s, struct clk *parent, int nest)
+{
+	struct clk *clk;
+	int i;
+
+	list_for_each_entry(clk, &clocks, node) {
+		if (clk->parent == parent) {
+			for (i = 0; i < nest; i++) {
+				seq_putc(s, ' ');
+				seq_putc(s, ' ');
+			}
+			seq_puts(s, clk->name);
+
+			i = 2 * nest + strlen(clk->name);
+			if (i >= NAME_FIELD_LEN)
+				i = NAME_FIELD_LEN - 1;
+			for (; i < NAME_FIELD_LEN; i++) {
+				seq_putc(s, ' ');
+			}
+			seq_printf(s, "%c use=%d rate=%10lu Hz",
+				clk->ops && clk->ops->set_parent ? '*' : ' ',
+				clk->users,
+				__clk_get_rate(clk));
+			if (clk->ops && clk->ops->format)
+				clk->ops->format(clk, s);
+			seq_putc(s, '\n');
+
+			dump_clocks(s, clk, nest + 1);
+		}
+	}
+}
+
+static int clocklib_show(struct seq_file *s, void *unused)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&clocks_lock, flags);
+
+	dump_clocks(s, NULL, 0);
+
+	spin_unlock_irqrestore(&clocks_lock, flags);
+
+	return 0;
+}
+
+static int clocklib_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, clocklib_show, NULL);
+}
+
+static struct file_operations clocklib_operations = {
+	.open		= clocklib_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
+
+static int __init clocklib_debugfs_init(void)
+{
+	debugfs_create_file("clock", S_IFREG | S_IRUGO,
+				NULL, NULL, &clocklib_operations);
+	return 0;
+}
+subsys_initcall(clocklib_debugfs_init);
+
+#endif /* DEBUG_FS */
-- 
1.5.4.4


-- 
With best wishes
Dmitry