Hi Yong,
> To do so, I need to make somethings done, and some of them are related to
> common clock code, for which I am more than happy to hear your comments.
Definitely, I'm happy to help out.
> 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.
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
:)
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,
\
}
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.
> 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.
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 :)
Regards,
Jeremy
_______________________________________________
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
