From: Arnd Bergmann <arnd@arndb•de>
To: linuxppc-dev@ozlabs•org
Subject: Re: [PATCH] windfarm: add PowerMac 12,1 support
Date: Mon, 10 Dec 2007 16:39:52 +0100 [thread overview]
Message-ID: <200712101639.52599.arnd@arndb.de> (raw)
In-Reply-To: <1197299517.6822.2.camel@thilivren>
On Monday 10 December 2007, =C3=89tienne Bersac wrote:
> From: =C3=89tienne Bersac <bersace03@gmail•com>
>=20
> Here is an updated patch that fix the potential bug, CPU param detection
> failure might not be detected. Please review it.=20
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 =3D "backside-temp";
> + char *name =3D NULL;
> =20
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] =3D {};
> +
> +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] =3D {
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[] =3D {
same here.
> +static struct pm121_sys_param
> +pm121_sys_all_params[N_LOOPS][PM121_NUM_CONFIGS] =3D {
and here.
> +#ifdef HACKED_OVERTEMP
> +#define MAX 0x4a0000
> +#else
> +#define MAX st->pid.param.tmax
> +#endif
> + if (temp > MAX)
> + pm121_failure_state |=3D 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] =3D=3D NULL && !strcmp(control->name,match)) { \
> + if (wf_get_control(control) =3D=3D 0) \
> + controls[id] =3D control; \
> + } \
> + all =3D 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 =3D -ENODEV;
> +
> + if (machine_is_compatible("PowerMac12,1"))
> + rc =3D pm121_init_pm();
> +
> + if (rc =3D=3D 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 <><
next prev parent reply other threads:[~2007-12-10 15:40 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2007-12-06 1:20 [PATCH] windfarm: add PowerMac 12,1 support Étienne Bersac
2007-12-06 15:30 ` Étienne Bersac
2007-12-10 15:11 ` Étienne Bersac
2007-12-10 15:39 ` Arnd Bergmann [this message]
2007-12-10 17:17 ` Étienne Bersac
2007-12-10 17:22 ` Étienne Bersac
-- strict thread matches above, loose matches on Subject: below --
2008-01-26 11:55 Étienne Bersac
2008-03-22 19:35 ` David Woodhouse
2008-03-22 22:13 ` Benjamin Herrenschmidt
2008-03-22 23:02 ` David Woodhouse
2008-03-22 23:14 ` David Woodhouse
2008-03-22 23:25 ` Benjamin Herrenschmidt
2008-03-22 23:25 ` Stephen Rothwell
2008-03-22 23:31 ` David Woodhouse
2008-03-23 1:01 ` Andreas Schwab
2008-03-23 5:38 ` Stephen Rothwell
2008-04-29 5:39 Benjamin Herrenschmidt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=200712101639.52599.arnd@arndb.de \
--to=arnd@arndb$(echo .)de \
--cc=linuxppc-dev@ozlabs$(echo .)org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox