Re: [PATCH] CPUFreq Support for PXA255

[Dominik Brodowski <linux@dominikbrodowski.net>]

Hi,

On Thu, Jul 14, 2005 at 12:40:56PM +0100, Ian Campbell wrote:
> +#define DEBUG  0
> +
> +#ifdef DEBUG
> +static unsigned int freq_debug = DEBUG;
> +module_param(freq_debug, int, 0);
> +MODULE_PARM_DESC(freq_debug, "Set the debug messages to on=1/off=0");
> +#else
> +#define freq_debug  0
> +#endif

Please use the cpufreq debug infrastructure for this:

#define dprintk(msg...) cpufreq_debug_printk(CPUFREQ_DEBUG_DRIVER, "pxa", msg)

> +typedef struct {
> +	unsigned int khz;
> +	unsigned int membus;
> +	unsigned int cccr;
> +	unsigned int div2;
> +} pxa_freqs_t;

Don't use typedefs, please. Just name it "struct pxa_freqs"

> +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;
> +	}
> +}

As noted by Eric Piel already, please use a module parameter combined with a
sysfs file instead.

> +	if (freq_debug) {
> +		printk("Verified CPU policy: %dKhz min to %dKhz max\n",
> +		       policy->min, policy->max);
> +	}

	dprintk("Verified ...);

> +	cpumask_t cpus_allowed;

Is this a SMP-capable platform?

> +	policy->cpuinfo.transition_latency = 1000;	/* FIXME: 1 ms, assumed */

1000000

> +1.1 ARM, PXA
> +------------
>  
>  The following ARM processors are supported by cpufreq:
>  
>  ARM Integrator
>  ARM-SA1100
>  ARM-SA1110
> +Intel PXA

Is the PXA just a sub-architecture of ARM?

>  choice
>  	prompt "Default CPUFreq governor"
> -	default CPU_FREQ_DEFAULT_GOV_USERSPACE if CPU_FREQ_SA1100 || CPU_FREQ_SA1110
> +	default CPU_FREQ_DEFAULT_GOV_USERSPACE if CPU_FREQ_SA1100 || CPU_FREQ_SA1110 || CPU_FREQ_PXA

Why is that?


Except the two-tables-issue I'd like to see solved differently, it's good
code IMHO.

	Dominik