Re: [PATCH 0/5] I8K driver facelift

[Frank Sorenson <frank@tuxrocks.com>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Dmitry Torokhov wrote:
| Hi,
|
| here are some changes that freshen I8K driver (Dell Inspiron/Latitude
| platform driver). The patches have been tested on Inspiron 8100.
<snip>
| Please consider for inclusion.
|
| Thanks!

These patches look pretty good.  A few comments (with a patch--tested on
my Inspiron 9200):

- - The "return i8k_smm(&regs) < 0 ? : regs.eax;" construction is nice and
tidy, but it isn't passing on the return value of the called function,
and is returning TRUE or 1 on failure.  This makes it difficult to check
the return value for valid data.  Old behavior returned negative, so
I'll return -1.

- - The I8K_VERSION string should probably be changed to something more
unique.  The version maintained outside the kernel tree by Massimo Dal
Zotto (http://www.debian.org/~dz/i8k/) is up to 1.25, so 1.14 isn't very
distinguishing.  Maybe just use the date?  Not sure here...

- - Also, some newer Dell laptops require a different function to get the
Dell signature, so the i8k_get_dell_signature test should check both
(borrowing some code from the out-of-kernel version).

- - Some newer Dell laptops report DMI_SYS_VENDOR as "Dell Inc." rather
than "Dell Computer"

- - Some of the Dell motherboards provide more than 1 temperature sensor.
~ How about a generic i8k_get_temp function, and i8k_get_cpu_temp just
calls that with sensor 0.

- - Also, I've added detection of the number of temperature sensors and
fans at init time.  This way, we aren't hardcoded to 1 sensor and 2
fans.  I couldn't figure out how to set up the sysfs entries
dynamically, but that probably should happen too.

- - Some boards don't need the I8K_FAN_MULT to get the right fan RPM (I
don't think my fans are rotating at over 90,000 RPM)!  I guess we'll
need to address this sometime, but I have not done so.

Patch follows:

Signed-off-by: Frank Sorenson <frank@tuxrocks.com>

- --- 2.6.11-a/drivers/char/i8k.c	2005-03-12 18:47:55.000000000 -0700
+++ 2.6.11-b/drivers/char/i8k.c	2005-03-12 20:29:01.000000000 -0700
@@ -28,7 +28,7 @@

~ #include <linux/i8k.h>

- -#define I8K_VERSION		"1.14 21/02/2005"
+#define I8K_VERSION		"2005-03-12"

~ #define I8K_SMM_FN_STATUS	0x0025
~ #define I8K_SMM_POWER_STATUS	0x0069
@@ -36,7 +36,8 @@
~ #define I8K_SMM_GET_FAN		0x00a3
~ #define I8K_SMM_GET_SPEED	0x02a3
~ #define I8K_SMM_GET_TEMP	0x10a3
- -#define I8K_SMM_GET_DELL_SIG	0xffa3
+#define I8K_SMM_GET_DELL_SIG1	0xfea3
+#define I8K_SMM_GET_DELL_SIG2	0xffa3
~ #define I8K_SMM_BIOS_VERSION	0x00a6

~ #define I8K_FAN_MULT		30
@@ -55,6 +56,8 @@
~ #define I8K_TEMPERATURE_BUG	1

~ static char bios_version[4];
+static int num_temps = 0;
+static int num_fans = 0;

~ MODULE_AUTHOR("Massimo Dal Zotto (dz@debian.org)");
~ MODULE_DESCRIPTION("Driver for accessing SMM BIOS on Dell laptops");
@@ -201,10 +204,10 @@
~  */
~ static int i8k_get_fan_status(int fan)
~ {
- -	struct smm_regs regs = { .eax = I8K_SMM_GET_FAN, };
+	struct smm_regs regs = { .eax = I8K_SMM_GET_FAN, .ebx = (fan & 0xff)};

~ 	regs.ebx = fan & 0xff;
- -	return i8k_smm(&regs) < 0 ? : regs.eax & 0xff;
+	return i8k_smm(&regs) < 0 ? -1 : regs.eax & 0xff;
~ }

~ /*
@@ -215,7 +218,7 @@
~ 	struct smm_regs regs = { .eax = I8K_SMM_GET_SPEED, };

~ 	regs.ebx = fan & 0xff;
- -	return i8k_smm(&regs) < 0 ? : (regs.eax & 0xffff) * I8K_FAN_MULT;
+	return i8k_smm(&regs) < 0 ? -1 : (regs.eax & 0xffff) * I8K_FAN_MULT;
~ }

~ /*
@@ -228,15 +231,15 @@
~ 	speed = (speed < 0) ? 0 : ((speed > I8K_FAN_MAX) ? I8K_FAN_MAX : speed);
~ 	regs.ebx = (fan & 0xff) | (speed << 8);

- -	return i8k_smm(&regs) < 0 ? : i8k_get_fan_status(fan);
+	return i8k_smm(&regs) < 0 ? -1 : i8k_get_fan_status(fan);
~ }

~ /*
~  * Read the cpu temperature.
~  */
- -static int i8k_get_cpu_temp(void)
+static int i8k_get_temp(int sensor)
~ {
- -	struct smm_regs regs = { .eax = I8K_SMM_GET_TEMP, };
+	struct smm_regs regs = { .eax = I8K_SMM_GET_TEMP, .ebx = sensor };
~ 	int rc;
~ 	int temp;

@@ -268,9 +271,14 @@
~ 	return temp;
~ }

- -static int i8k_get_dell_signature(void)
+static int i8k_get_cpu_temp(void)
+{
+	return i8k_get_temp(0);
+}
+
+static int i8k_get_dell_sig_aux(int fn)
~ {
- -	struct smm_regs regs = { .eax = I8K_SMM_GET_DELL_SIG, };
+	struct smm_regs regs = { .eax = fn, };
~ 	int rc;

~ 	if ((rc = i8k_smm(&regs)) < 0)
@@ -279,6 +287,17 @@
~ 	return regs.eax == 1145651527 && regs.edx == 1145392204 ? 0 : -1;
~ }

+static int i8k_get_dell_signature(void)
+{
+	int rc;
+
+	if (((rc=i8k_get_dell_sig_aux(I8K_SMM_GET_DELL_SIG1)) < 0) &&
+	    ((rc=i8k_get_dell_sig_aux(I8K_SMM_GET_DELL_SIG2)) < 0)) {
+		return rc;
+	}
+	return 0;
+}
+
~ static int i8k_ioctl(struct inode *ip, struct file *fp, unsigned int cmd,
~ 		     unsigned long arg)
~ {
@@ -506,6 +525,13 @@
~ 		},
~ 	},
~ 	{
+		.ident = "Dell Inspiron",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Inspiron"),
+		},
+	},
+	{
~ 		.ident = "Dell Latitude",
~ 		.matches = {
~ 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Computer"),
@@ -587,6 +613,11 @@
~ 	if (i8k_probe())
~ 		return -ENODEV;

+	while (i8k_get_temp(num_temps) >= 0) num_temps ++;
+	printk(KERN_INFO "i8k: found %d temperature sensors\n", num_temps);
+	while (i8k_get_fan_status(num_fans) >= 0) num_fans ++;
+	printk(KERN_INFO "i8k: found %d fans\n", num_fans);
+
~ 	/* Register the proc entry */
~ 	proc_i8k = create_proc_entry("i8k", 0, NULL);
~ 	if (!proc_i8k)



Frank
- --
Frank Sorenson - KD7TZK
Systems Manager, Computer Science Department
Brigham Young University
frank@tuxrocks.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.6 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFCM7ZZaI0dwg4A47wRAok1AKDLYIrMXLphYAeAq98OXYqxk6U1vACfQpld
qczdJm2992BjeQ/iU9RP+k4=
=nNut
-----END PGP SIGNATURE-----