[PATCH] windfarm: add PowerMac 12,1 support

[Arnd Bergmann]

On Monday 10 December 2007, Étienne Bersac wrote:
> From: Étienne Bersac <bersace03 at gmail.com>
> 
> Here is an updated patch that fix the potential bug, CPU param detection
> failure might not be detected. Please review it. 

I didn't find much interesting to complain about, just a few style
issues that could be improved. Good work!

> -static void wf_max6690_create(struct i2c_adapter *adapter, u8 addr)
> +static void wf_max6690_create(struct i2c_adapter *adapter, u8 addr,
> +			      const char *loc)
>  {
>  	struct wf_6690_sensor *max;
> -	char *name = "backside-temp";
> +	char *name = NULL;
>  

It's better not to initialize local variables like this one, instead of
assigning them to 0 or NULL. When it's not initialized, gcc is able to
warn about cases where you don't assign a meaningful value before using
it.

> +#undef	DEBUG
> +
> +#ifdef DEBUG
> +#define DBG(args...)	printk(args)
> +#else
> +#define DBG(args...)	do { } while (0)
> +#endif

Don't define your own macros like these, but instead use the ones that
are there. If possible, use 'dev_dbg()', otherwise 'pr_debug()'.

> +static int pm121_mach_model;	/* machine model id */
> +
> +/* Controls & sensors */
> +static struct wf_sensor	*sensor_cpu_power;
> +static struct wf_sensor	*sensor_cpu_temp;
> +static struct wf_sensor	*sensor_cpu_voltage;
> +static struct wf_sensor	*sensor_cpu_current;
> +static struct wf_sensor	*sensor_gpu_temp;
> +static struct wf_sensor	*sensor_north_bridge_temp;
> +static struct wf_sensor	*sensor_hard_drive_temp;
> +static struct wf_sensor	*sensor_optical_drive_temp;
> +static struct wf_sensor	*sensor_incoming_air_temp; /* unused ! */
> +static struct wf_control *controls[N_CONTROLS] = {};
> +
> +static unsigned int pm121_failure_state;
> +static int pm121_readjust, pm121_skipping;
> +static s32 average_power;

Having many global variables is typically a sign that you are doing
something wrong. It's better to attach all the data you have to your
device structure.

Have you checked that you are doing the right locking when accessing
these variables from concurrent threads?

> +struct pm121_correction {
> +	int	offset;
> +	int	slope;
> +};
> +
> +struct pm121_correction corrections[N_CONTROLS][PM121_NUM_CONFIGS] = {

This looks like it could be 'const'. 'static const' variables are better
because it becomes obvious that you don't need locking to access them.

> +static struct pm121_connection pm121_connections[] = {

same here.

> +static struct pm121_sys_param
> +pm121_sys_all_params[N_LOOPS][PM121_NUM_CONFIGS] = {

and here.


> +#ifdef HACKED_OVERTEMP
> +#define	MAX	0x4a0000
> +#else
> +#define	MAX	st->pid.param.tmax
> +#endif
> +	if (temp > MAX)
> +		pm121_failure_state |= FAILURE_OVERTEMP;
> +
> +#undef	MAX

You don't define HACKED_OVERTEMP anywhere, so you basicall have dead
code here. It's probably better to remove that for the upstream version,
even if it was helpful for your debugging.

> +
> +#define	pm121_register_control(control, match, id)			\
> +	if (controls[id] == NULL && !strcmp(control->name,match)) {	\
> +		if (wf_get_control(control) == 0)			\
> +			controls[id] = control;				\
> +	}								\
> +	all = all && controls[id];

Does this need to be a macro? If possible, use static functions
for this kind of stuff. They will normally be inlined, which gives
you the same object code in the end.

> +static int __init pm121_init(void)
> +{
> +	int rc = -ENODEV;
> +
> +	if (machine_is_compatible("PowerMac12,1"))
> +		rc = pm121_init_pm();
> +
> +	if (rc == 0) {
> +#ifdef MODULE
> +		request_module("windfarm_smu_controls");
> +		request_module("windfarm_smu_sensors");
> +		request_module("windfarm_smu_sat");
> +		request_module("windfarm_lm75_sensor");
> +		request_module("windfarm_max6690_sensor");
> +		request_module("windfarm_cpufreq_clamp");
> +
> +#endif /* MODULE */
> +		platform_driver_register(&pm121_driver);
> +	}
> +
> +	return rc;
> +}

Why the #ifdef MODULE here? It's normally better to have identical
code for modular and built-in cases.

	Arnd <><