I did some testing on my Aspire One machine here and it looks pretty
nice: while compiling a kernel watched the fan going on when the
temperature reaches 67-68 (I don't think this is Celsius though, no?)
and then turning itself off when temp goes below 60.
+config ACERHDF
+ tristate "Acer Aspire One temperature and fan driver"
+ depends on THERMAL
+ depends on THERMAL_HWMON
depends on THERMAL && THERMAL_HWMON
+/* module parameters */
+module_param(interval, int, 0600);
+MODULE_PARM_DESC(interval, "Polling interval of temperature check");
+module_param(fanon, int, 0600);
This allows for the user to potentially melt his CPU by entering a too high
value. You should check that in the acerhdf_init() against the max allowed
according to spec, I gather it is 67?
+struct bios_settings_t {
+ const char *vendor;
+ const char *version;
+ unsigned char fanreg;
+ unsigned char tempreg;
+ unsigned char cmd_off;
+ unsigned char cmd_auto;
+ unsigned char state_off;
obviously cmd_off and state_off are the same values so remove one of them.
let's put a prefix to all those function names, otherwise their names
are all too generic:
get_temp -> acerhdf_get_temp
change_fanstate -> acerhdf_change_fanstate
you can wrap all those "if (verbose)"-checks in a macro making the code more
readable:
+ ec_write(bios_settings[bios_version].fanreg,
+ (state) ? bios_settings[bios_version].cmd_auto :
+ bios_settings[bios_version].cmd_off);
too unreadable.
how about:
u8 cmd = (state) ? bios_settings[bios_version].cmd_auto
: bios_settings[bios_version].cmd_off;
ec_write(bios_settings[bios_version].fanreg, cmd);
+/* return temperature */
no need for stating the obvious.
+/* set operation mode;
+ * kernel: a kernel thread takes care about managing the
+ * fan (see acerhdf_thread)
where is that acerhdf_thread? maybe stale comment from the kthread bits?
+ * user: kernel thread is stopped and a userspace tool
+ * should take care about managing the fan
+ */
+static int set_mode(struct thermal_zone_device *thermal,
+ enum thermal_device_mode mode)
+{
+ if (mode == THERMAL_DEVICE_DISABLED) {
+ if (verbose)
+ printk(KERN_NOTICE "acerhdf: kernelmode OFF\n");
those printk's shouldn't depend on verbose since this is important info
IMHO and I want to know that I've changed modes successfully and that my
CPU doesn't get fried.
+ *temperature = 89;
#define ACERHDF_TEMP_CRIT 89
+/* print current fan state */
+static int get_cur_state(struct thermal_cooling_device *cdev,
+ unsigned long *state)
+{
+ *state = get_fanstate();
you need error handling here:
if (*state < 0) {
*state = 0xffff;
return 1;
}
return 0;
or some other invalid value similar to how it's done in get_temp()
above.
+ if (state && !old_state)
+ change_fanstate(1);
let's have defines for those fan states
#define ACERHDF_FAN_OFF 0
#define ACERHDF_FAN_AUTO 1
and then do
change_fanstate(ACERHDF_FAN_AUTO);
+ /* if started in user mode, prevent the kernel from switching
+ * off the fan */
+ if (!kernelmode) {
+ recently_changed = 1;
+ printk(KERN_NOTICE
+ "acerhdf: kernelmode disabled\n");
+ printk(KERN_NOTICE
+ "acerhdf: to enable kernelmode:\n");
+ printk(KERN_NOTICE
+ "acerhdf: echo -n \"enabled\" > "
+ "/sys/class/thermal/thermal_zone0/mode\n");
maybe I'm missing something but shouldn't this be enabled by default and
only when the user wants to have acerfand or some other uspace tool do
the controlling, only then turn it off. I'd rather trust this is done
in the kernel instead of some flaky uspace thread which could maybe
segfault and we fry our nice little netbook :).
+ /* register platform device */
don't need those comments, function name is enough
+ if (platform_driver_register(&acerhdf_driver)) {