Hi Yong,
Yes, it's also nice to have a file containing all the clock information which you have implemented in the email. Since we expect more features like enable/disable clocks in the debugfs, we also like to have tree-like debugfs for clock information.
Sure; if you want the extended functionality, I think your tree approach is best.
Below is my draft implementation. Original implementation is from omap platform clock code, and I adopted to common clock device.
A couple of comments inline...
diff --git a/arch/arm/common/clkdev.c b/arch/arm/common/clkdev.c index 9e4c4d9..e7a629a 100644 --- a/arch/arm/common/clkdev.c +++ b/arch/arm/common/clkdev.c @@ -19,6 +19,7 @@
#include <linux/mutex.h> #include <linux/clk.h> #include <linux/slab.h>
+#include <linux/debugfs.h>
Minor nitpick: no need to put this #inlude on its own.
#include <asm/clkdev.h>
@@ -186,3 +187,128 @@ void clkdev_drop(struct clk_lookup *cl)
kfree(cl); } EXPORT_SYMBOL(clkdev_drop);
+#if defined(CONFIG_PM_DEBUG) && defined(CONFIG_DEBUG_FS)
Might be good to add CONFIG_CLK_DEBUG (which would depend on CONFIG_PM_DEBUG and CONFIG_DEBUG_FS), so we can enable/disable it separately.
+/*
- debugfs support to trace clock tree hierarchy and attributes
- */
+static int open_file(struct inode *inode, struct file *file)
I think you can replace this (and read_file, and rate_fops) with something like:
static int clk_debug_rate_get(void *data, u64 *val) { struct clk *clk = data *val = (u64)clk_get_rate(clk); return 0; } DEFINE_SIMPLE_ATTRIBUTE(clk_debug_rate_fops, clk_debug_rate_get, NULL, "%lu\n");
and then in clk_debugfs_register_one:
debugfs_create_file("rate", S_IRUGO, clk->dentry, clk, &clk_debug_rate_fops);
+static struct dentry *clk_debugfs_root;
+static int clk_debugfs_register_one(struct clk *clk) +{
- int err;
- struct dentry *d, *child, *child_tmp;
- struct clk *pa = clk_get_parent(clk);
- if ((pa != ERR_PTR(-ENOSYS)) && pa)
I'd suggest:
if (pa && !IS_ERR(pa))
- d = debugfs_create_dir(clk ? clk->name : "null", pa->dentry);
- else
- d = debugfs_create_dir(clk ? clk->name : "null", clk_debugfs_root);
Why do you need to check for clk == NULL here? We probably don't want 'null' directories hanging around.
- if (!d) {
- return -ENOMEM;
- }
- clk->dentry = d;
- d = debugfs_create_u32("enable_count", S_IRUGO, clk->dentry, (u32
*)&clk->enable_count);
- if (!d) {
- err = -ENOMEM;
- goto err_out;
- }
- d = debugfs_create_file("rate", S_IRUGO, clk->dentry, (void *)clk,
&rate_fops);
- if (!d) {
- err = -ENOMEM;
- goto err_out;
- }
- return 0;
+err_out:
- d = clk->dentry;
- list_for_each_entry_safe(child, child_tmp, &d->d_subdirs, d_u.d_child)
- debugfs_remove(child);
- debugfs_remove(clk->dentry);
- return err;
+}
+static int clk_debugfs_register(struct clk *clk) +{
- int err;
- struct clk *pa;
- if (clk == ERR_PTR(-ENOSYS))
- return -1;
- if (!clk)
- return -1;
Do we need these checks?
- pa = clk_get_parent(clk);
- if ((pa != ERR_PTR(-ENOSYS)) && pa && !pa->dentry) {
Again, this might be better:
if (pa && !IS_ERR(pa) && !pa->dentry)
.. but this makes me think we should probably drop the -ENOSYS return code from clk_get_parent, but that's a patch for another time.. :)
- err = clk_debugfs_register(pa);
- if (err)
- return err;
- }
- if (!clk->dentry) {
- err = clk_debugfs_register_one(clk);
- if (err)
- return err;
- }
- return 0;
+}
+static int __init clk_debugfs_init(void) +{
- struct clk_lookup *cl;
- struct dentry *d;
- int err;
- d = debugfs_create_dir("clock", NULL);
I'd suggest "clocks" instead of "clock", but this is fairly minor.
diff --git a/arch/arm/plat-mxc/clock-common.c b/arch/arm/plat-mxc/clock-common.c index 8911813..96e19a4 100644 --- a/arch/arm/plat-mxc/clock-common.c +++ b/arch/arm/plat-mxc/clock-common.c @@ -93,11 +93,14 @@ void clk_mxc_disable(struct clk *_clk)
unsigned long clk_mxc_get_rate(struct clk *_clk) { struct clk_mxc *clk = to_clk_mxc(_clk);
if (clk->get_rate) return clk->get_rate(clk);
- return clk_get_rate(clk->parent);
- if (clk->parent)
- return clk_get_rate(clk->parent);
- return 0;
I see you found this oops too :)
diff --git a/include/linux/clk.h b/include/linux/clk.h index 56416b7..0dc3443 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -44,16 +44,22 @@ struct device;
- registered with the arch-specific clock management code; the clock
driver
- code does not need to handle these.
*/
+#define CLK_NAME_LEN 16
struct clk { const struct clk_ops *ops; unsigned int enable_count; struct mutex mutex;
- char name[CLK_NAME_LEN];
+#if defined(CONFIG_PM_DEBUG) && defined(CONFIG_DEBUG_FS)
struct dentry *dentry;+#endif
Probably best to put 'name' into the CLK_DEBUG case; then you'll just need to use the __INIT_CLK_DEBUG macros in my previous patch.
BTW, the spacing in the patch seems to be a little off; not sure if this is due to your mailer or the patch itself. If it's the latter, you might want to clean this up before submitting.
Cheers,
Jeremy