Hi Jeremy,
Your suggestion is better in the architecture view and it is clean and neat,
on the other side, mine is only concerning about the minimal changing
of previous code.
I will try your way and give your feedback.
Yong
On Mon, Nov 22, 2010 at 10:46 AM, Jeremy Kerr <jeremy.k...@canonical.com>wrote:
> Hi Yong,
>
> > diff --git a/arch/arm/common/clkdev.c b/arch/arm/common/clkdev.c
> > index 9e4c4d9..936684f 100644
> > --- a/arch/arm/common/clkdev.c
> > +++ b/arch/arm/common/clkdev.c
> > @@ -19,6 +19,9 @@
> > #include <linux/mutex.h>
> > #include <linux/clk.h>
> > #include <linux/slab.h>
> > +#ifdef CONFIG_CLK_DEBUG
> > +#include <linux/debugfs.h>
> > +#endif
>
> With changes suggested below, we won't need this
>
> >
> > #include <asm/clkdev.h>
> >
> > @@ -104,6 +107,10 @@ EXPORT_SYMBOL(clk_put);
> >
> > void clkdev_add(struct clk_lookup *cl)
> > {
> > +#ifdef CONFIG_CLK_DEBUG
> > + if (debugfs_initialized())
> > + clk_debug_register(cl->clk);
> > +#endif
>
> Don't make this conditional on CONFIG_CLK_DEBUG. the clk_debug_register
> should
> do this checking, and for the !CONFIG_CLK_DEBUG case, clk_debug_register
> should be an empty inline.
>
> > mutex_lock(&clocks_mutex);
> > list_add_tail(&cl->node, &clocks);
> > mutex_unlock(&clocks_mutex);
> > @@ -114,6 +121,10 @@ void __init clkdev_add_table(struct clk_lookup *cl,
> > size_t num)
> > {
> > mutex_lock(&clocks_mutex);
> > while (num--) {
> > +#ifdef CONFIG_CLK_DEBUG
> > + if (debugfs_initialized())
> > + clk_debug_register(cl->clk);
> > +#endif
>
> Same here.
>
> > list_add_tail(&cl->node, &clocks);
> > cl++;
> > }
> > @@ -186,3 +197,20 @@ void clkdev_drop(struct clk_lookup *cl)
> > kfree(cl);
> > }
> > EXPORT_SYMBOL(clkdev_drop);
> > +
> > +#ifdef CONFIG_CLK_DEBUG
> > +static int __init clk_debugfs_init(void)
> > +{
> > + struct clk_lookup *cl;
> > + int err;
> > +
> > + list_for_each_entry(cl, &clocks, node) {
> > + err = clk_debug_register(cl->clk);
> > + if (err)
> > + return err;
> > + }
> > + return 0;
> > +}
> > +late_initcall(clk_debugfs_init);
>
> Would be much better to contain all the debugging functionality in
> kernel/clk.c. I think we'd be better of exposing a single function:
>
> clk_debug_register(struct clk *);
>
> - and implementing the details in kernel/clk.c. The caller should not have
> to
> care about the internal implementation details of the debug code (ie, that
> debugfs is not initialised).
>
> So, this function must be able to register clocks at any time.
>
> > +
> > +#endif
> > diff --git a/include/linux/clk.h b/include/linux/clk.h
> > index 56416b7..5a0139c 100644
> > --- a/include/linux/clk.h
> > +++ b/include/linux/clk.h
> > @@ -44,16 +44,28 @@ struct device;
> > * registered with the arch-specific clock management code; the clock
> > driver
> > * code does not need to handle these.
> > */
> > +#define CLK_NAME_LEN 32
>
> I'd put this before the above comment, so the kerneldoc is properly
> located.
>
> > struct clk {
> > const struct clk_ops *ops;
> > unsigned int enable_count;
> > struct mutex mutex;
> > +#ifdef CONFIG_CLK_DEBUG
> > + char name[CLK_NAME_LEN];
> > + struct dentry *dentry;
> > +#endif
> > };
> >
> > +#ifdef CONFIG_CLK_DEBUG
> > +#define __INIT_CLK_DEBUG(n) .name = #n,
> > +#else
> > +#define __INIT_CLK_DEBUG(n)
> > +#endif
>
> looks good :)
>
> > +
> > #define INIT_CLK(name, o) { \
> > .ops = &o, \
> > .enable_count = 0, \
> > .mutex = __MUTEX_INITIALIZER(name.mutex), \
> > + __INIT_CLK_DEBUG(name) \
> > }
> >
> > struct clk_ops {
> > @@ -245,4 +257,5 @@ struct clk *clk_get_sys(const char *dev_id, const
> char
> > *con_id);
> > int clk_add_alias(const char *alias, const char *alias_dev_name, char
> *id,
> > struct device *dev);
> >
> > +int clk_debug_register(struct clk *clk);
>
> We need the inline here too:
>
> #ifdef CONFIG_CLK_DEBUG
> extern int clk_debug_register(struct clk *clk);
> #else
> static inline int clk_debug_register(struct clk *clk) { return 0; }
> #endif
>
> Also, we should probably make this return void; it's unlikely that callers
> are
> going to check (or care about) the return value here.
>
>
> > #endif
> > diff --git a/kernel/clk.c b/kernel/clk.c
> > index 32f25ef..df4da68 100644
> > --- a/kernel/clk.c
> > +++ b/kernel/clk.c
> > @@ -12,6 +12,10 @@
> > #include <linux/mutex.h>
> > #include <linux/module.h>
> >
> > +#ifdef CONFIG_CLK_DEBUG
> > +#include <linux/debugfs.h>
> > +#endif
>
> Just include unconditionally.
>
> > +
> > int clk_enable(struct clk *clk)
> > {
> > int ret = 0;
> > @@ -113,3 +117,89 @@ struct clk_ops clk_fixed_ops = {
> > .get_rate = clk_fixed_get_rate,
> > };
> > EXPORT_SYMBOL_GPL(clk_fixed_ops);
> > +
> > +#ifdef CONFIG_CLK_DEBUG
> > +/*
> > + * debugfs support to trace clock tree hierarchy and attributes
> > + */
> > +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,
> > + "%llu\n");
> > +
> > +
> > +static struct dentry *clk_root;
> > +static int clk_debug_register_one(struct clk *clk)
> > +{
> > + int err;
> > + struct dentry *d, *child, *child_tmp;
> > + struct clk *pa = clk_get_parent(clk);
> > +
> > + if (pa && !IS_ERR(pa))
> > + d = debugfs_create_dir(clk->name, pa->dentry);
> > + else {
> > + if (!clk_root)
> > + clk_root = debugfs_create_dir("clocks", NULL);
> > + if (!clk_root)
> > + return -ENOMEM;
> > + d = debugfs_create_dir(clk->name, clk_root);
> > + }
> > +
> > + 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,
> > + &clk_debug_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;
> > +}
> > +
> > +int clk_debug_register(struct clk *clk)
> > +{
> > + int err;
> > + struct clk *pa;
> > +
> > + pa = clk_get_parent(clk);
> > +
> > + if (pa && !IS_ERR(pa) && !pa->dentry) {
> > + err = clk_debug_register(pa);
> > + if (err)
> > + return err;
> > + }
> > +
> > + if (!clk->dentry) {
> > + err = clk_debug_register_one(clk);
> > + if (err)
> > + return err;
> > + }
> > + return 0;
> > +}
>
> How about something like:
>
> struct preinit_clk {
> struct list_head list;
> struct clk *clk;
> };
>
> static LIST_HEAD(preinit_clks);
> static DEFINE_MUTEX(preinit_lock);
> static int init_done;
>
> void clk_debug_register(struct clk *clk)
> {
> mutex_lock(&preinit_lock);
> if (init_done) {
> /* normal register path ... */
> } else {
> struct preinit_clk *p;
>
> p = kmalloc(sizeof(*p), GFP_KERNEL);
> if (!p)
> goto out_unlock;
> p->clk = clk;
> list_add(&p->list, &preinit_clks);
> }
>
> out_unlock:
> mutex_unlock(&preinit_lock);
> }
>
> and then in clk_debug_init, we add all the clocks from preinit_list, then
> set
> init_done = 1. Calls to clk_debug_register after clk_debug_init() will have
> the dentry set up directly.
>
> This way, the caller does not have to care about the state of the clk_debug
> code.
>
> How does this sound?
>
> > +#else /* defined CONFIG_CLK_DEBUG */
> > +inline int clk_debug_register(struct clk *clk) {}
>
> We'll define this in the header, so you can remove this definition.
>
> > +#endif
> > +EXPORT_SYMBOL_GPL(clk_debug_register);
>
> Cheers,
>
>
> Jeremy
>
_______________________________________________
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
