Hi Jeremy,

Thanks for comments.
Please see my further discussion.

> 1. Create clock information based on common clock device, more specific,
> based on struct clk_lookup. Since platform drivers are supposed to register
> their clock
>  using 'void clkdev_add(struct clk_lookup *cl)', clk_lookup should contain
> enough information for clock display, like clock name and platform clk
> definition 'struck clk'. An extra field need to be added into clk_lookup is
> a 'struct dentry *' to store a pointer of the dentry node which will be
> exported in debug fs, like below:
> struct clk_lookup {
>         struct list_head        node;
>         const char              *dev_id;
>         const char              *con_id;
>         struct clk              *clk;
> +
> +#if defined(CONFIG_PM_DEBUG) && defined(CONFIG_DEBUG_FS)
> +       struct dentry           *dent;
> +#endif
> };

I think that struct clk_lookup is the wrong place to put this information; you
want to have debug information about the clock, so it should live in struct
clk. struct clk_lookup represents a relationship between a clock and
(potentially) a device using that clock, and this is why you're having the
naming issues.

True. I misunderstood the purpose of clk_lookup.
 
Unfortunately, that means modifying the 21-or-so different definitions of
struct clk. I'm guessing this is why you decided on using clk_lookup instead
:)
This is something I try to avoid. Originally, I thought I could write a unified interface for all the platform without caring about different clock definitions.
 

I'd see this better done as adding a 'const char name[CLK_NAME_LEN]' to struct
clk, and populating this in the relevant platform code. Using the common clk
API, you'd just have to change the one 'struct clk'. We already have a name
parameter to INIT_CLK, so we could trivially set the name:

This would give us:

#define CLK_NAME_LEN    16

struct clk {
       const struct clk_ops    *ops;
       unsigned int                    enable_count;
       struct mutex                    mutex;
       const char                      name[CLK_NAME_LEN];
};

#define INIT_CLK(name, o) {                                                                     \
       .ops                    = &o,                                                                   \
       .enable_count   = 0,                                                                            \
       .lock.mutex     = __MUTEX_INITIALIZER(name.lock.mutex),         \
       .name           = #name,                                                                        \
}

This means that you have already forced each SOC to have a unified 'struct clk', or at least force some fields inside the struct tp be unified, right? My understanding is that leaving 'struct clk' definition to SOC related code is to give freedom for 'struct clk' definition to SOC vendors and they can define their own 'struct clk' as they want. If 'struct clk' has this much in common, why cannot we define it or define some common fields in common code, say, <linux/clk.h> or <asm/clkdev.h>?

I think that allocating a dentry per clk_lookup is quite heavyweight; we could
just expose one debugfs file, with the whole set of clocks available through
this file, one clock per line. The seq_file interface makes this fairly
straightforward to do.

This is another way to expose information, but we plan to have a more vivid tree-like clock information, which had been adopted by omap already. The whole clock information is presented in a directory with many sub directories and sub nodes and powerdebug will walk through the directories to create tree-like output.
 
> 3. Currently, 'struct clk' is defined in platform related code, common
> clk  interface did not impose any assumption on how each platform define
> their own 'struct clk', therefore, clock debug interface can not make any
> assumption on how much information the platform defined clock contains as
> well. So far, only clk_get_rate is there as a common defined interface
> which I can use to export clock rate information, and I plan to add some
> optional interface like clk_get_count, clk_get_flag, so more useful
> information can be displayed.

You probably don't need to add the clk_get_count parameter, as this isn't
something we would want to expose to drivers through the clk API; instead,
just access clk->enable_count directly. With the common clk API, this will be
valid on all platforms, as struct clk always has the enable_count. I'm not
sure what information you want to expose with clk_get_flag, but the same might
apply there too.
About what can be exposed,  we can decide later, they don't have to be flag or count, but the goal is to provide useful information as we can. 

If you like, after I've finished some updates to the common clock code, I can
put together a patch to deomstrate what I mean. It shouldn't be too long :)

Can't wait to see that. Can I know a rough schedule of that? Since this is a high priority task for me on the blueprints, I am keen to know more. Maybe if you have draft code done, I can have test as soon as possible. 

Regards,


Jeremy