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
_______________________________________________
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
