Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver

[Richard Zhao <richard.zhao@linaro.org>]

On Tue, Dec 20, 2011 at 02:59:04PM +0000, Mark Brown wrote:
> On Mon, Dec 19, 2011 at 11:21:40AM +0800, Richard Zhao wrote:
> > It support single core and multi-core ARM SoCs. But currently it assume
> > all cores share the same frequency and voltage.
> 
> My comments on the previous version of the patch still apply:
> 
>  - The voltage ranges being set need to be specified as ranges.
cpu normally need strict voltages. and only proved operating opints
are allowed to set in dts. If the voltage changes slightly especially
for high frequency, it's easy to cause unstable.
>  - Frequencies that can't be supported due to limitations of the
>    available supplies shouldn't be exposed to users.
As I said, only proved operation points are allowed.
>  - The driver needs to handle errors.
Yes.
> 
> > +Required properties in /cpus/cpu@0:
> > +- compatible : "generic-cpufreq"
> > +- cpu-freqs : cpu frequency points it support
> > +- cpu-volts : cpu voltages required by the frequency point at the same index
> > +- trans-latency :  transition_latency
> 
> You need to define units for all of these, and for the transition
> latency you need to be clear about what's being measured (it looks like
> the CPU time only, not any voltage ramping).
right.
> 
> You also need to define how the core supplies get looked up.
It's pure software. platform uses this driver have to define "cpu" consumer.
> 
> > +	/* Manual states, that PLL stabilizes in two CLK32 periods */
> > +	policy->cpuinfo.transition_latency = trans_latency;
> 
> I guess this comment is a cut'n'paste error.
right, thanks.
> 
> > +
> > +	ret = cpufreq_frequency_table_cpuinfo(policy, freq_table);
> > +
> > +	if (ret < 0) {
> > +		pr_err("%s: invalid frequency table for cpu %d\n",
> > +		       __func__, policy->cpu);
> 
> You should define pr_fmt to always include __func__ in the messages
> rather than open coding - ensures consistency and is less noisy in the
> code.
I'll check it.
> 
> > +	pr_info("Generic CPU frequency driver\n");
> 
> This seems noisy...
Why? Do you think only errors and warnings can print out?

Thanks
Richard