Re: [PATCH] CPUFreq Support for PXA255

[Eric Piel <Eric.Piel@lifl.fr>]

14.07.2005 13:40, Ian Campbell wrote/a écrit:
> Hi, 
Hello,

I don't know so much about cpufreq, so only very few comments:
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ 2.6/arch/arm/mach-pxa/cpu-pxa.c	2005-07-14 10:57:04.000000000 +0100
> +
> +static void pxa_select_freq_table(struct cpufreq_policy *policy,
> +				  pxa_freqs_t ** settings,
> +				  struct cpufreq_frequency_table **table)
> +{
> +	if (strcmp(policy->governor->name, "performance") == 0) {
> +		if (settings)
> +			*settings = pxa255_run_freqs;
> +		if (table)
> +			*table = pxa255_run_freq_table;
> +	} else {
> +		if (settings)
> +			*settings = pxa255_turbo_freqs;
> +		if (table)
> +			*table = pxa255_turbo_freq_table;
> +	}
> +}
So your driver depends on a hard-coded name of a governor ? It seems 
suspicious...


> +
> +static int pxa_set_target(struct cpufreq_policy *policy,
> +			  unsigned int target_freq, unsigned int relation)
> +{
:
> +
> +	/* Get the current policy */
> +	pxa_select_freq_table(policy, &pxa_freq_settings, &pxa_freqs_table);
> +
> +	/* Lookup the next frequency */
> +	if (cpufreq_frequency_table_target(policy, pxa_freqs_table,
> +					   target_freq, relation, &idx))
> +		return -EINVAL;
> +
> +	freqs.old = get_clk_frequency_khz(0);
> +	freqs.new = pxa_freq_settings[idx].khz;
It's probably a good idea to include such a code:
	if (freqs.new == freqs.old)
		return 0;

This avoid to do lots of things while it would change nothing.


> +static int pxa_cpufreq_init(struct cpufreq_policy *policy)
> +{
:
> +	policy->cpuinfo.min_freq = PXA25x_MIN_FREQ;
> +	policy->cpuinfo.transition_latency = 1000;	/* FIXME: 1 ms, assumed */
If you want to say 1ms, you have to write 1000000 (it's in nanosecond). 
This value is quite important for the ondemand governor so if you have 
better knowledge of the latency, don't hesitate to fix it!

> +
> +static struct cpufreq_driver pxa_cpufreq_driver = {
> +	.verify	= pxa_verify_policy,
> +	.target	= pxa_set_target,
> +	.init	= pxa_cpufreq_init,
> +	.get	= pxa_cpufreq_get,
> +	.name	= "PXA25x",
> +};
Not sure it's a good idea to put the name in uppercase.

Eric