On Fri, Jul 27, 2012 at 02:33:35PM +0800, Richard Zhao wrote:

quoted

+Generic cpufreq driver for CPU0

It's going to have generic name if it will become more generic.

I'm not in the position to say that it will become even more generic.

quoted

+- voltage-tolerance: Specify the CPU voltage tolerance in percentage.

Why do we have the same tolerance for all points?

Because I haven't seen any case that needs different tolerance for
different operating points.

I think you can
either remove tolerance or add it to opps.

I'm not going to remove it, because I have seen potential cpufreq-cpu0
candidates, e.g. omap-cpufreq, need it.  It's also improper to encode
it in operating-points, since OPP library does not have it.

quoted

+	ret = clk_set_rate(cpu_clk, freqs.new * 1000);

Check return value and fall back to previous point if it needs?

Right, the voltage should be reverted back if clk_set_rate fails.

quoted

+	cpu_dev->of_node = np;

hmm.. sys dev can not set of_node when populate it?

Since the sys dev is not populated from device tree, the of_node is
not set, and we have to do it here on our own.

And why not do it in module init? 

What's the advantage of doing it in module init over here?

static u32 max_freq = UINT_MAX / 1000; /* kHz */
module_param(max_freq, uint, 0);
MODULE_PARM_DESC(max_freq, "max cpu frequency in unit of kHz");

It's for debug. Make sense?

It does not look so useful to me, as it never came to me when I was
debugging the driver.

-- 
Regards,
Shawn