[PATCH 0/8] hwmon: applesmc: Dynamic configuration rewrite

[PATCH 0/8] hwmon: applesmc: Dynamic configuration rewrite

[Henrik Rydberg]

The amount of machines supported by this driver has increased rapidly
over the past years, and the detailed driver support of the many
flavors is quickly becoming a problem.

This patchset introduces dynamic configuration by making the SMC
register set efficiently searchable, and fixes a number of outstanding
issues:

* Support for all new models since MacbookPro 6.
* Support for variable number of cpu sensors in the MacPros.
* Handles the odd temperature sensor types found on new models.
* Fixes minor read problems on many intermediate models.

The first patch is preparatory. The second patch adds the register
lookup cache. The third patch adds the dynamic temperature files, and
the fourth implements a new temperature register format. The fifth
patch makes the remaining features dynamically configured, and the
sixth patch sets up the fans dynamically. The seventh patch is purely
janitory, and the last patch updates the copyright.

As a side-effect, the file is reduced by almost five hundred lines.

Cheers,
Henrik

Henrik Rydberg (8):
  hwmon: applesmc: Relax the severity of device init failure
  hwmon: applesmc: Introduce a register lookup table
  hwmon: applesmc: Dynamic creation of temperature files
  hwmon: applesmc: Handle new temperature format
  hwmon: applesmc: Extract all features generically
  hwmon: applesmc: Dynamic creation of fan files
  hwmon: applesmc: Simplify feature sysfs handling
  hwmon: applesmc: Update copyright information

 drivers/hwmon/applesmc.c | 1542 ++++++++++++++++------------------------------
 1 files changed, 534 insertions(+), 1008 deletions(-)

[PATCH 1/8] hwmon: applesmc: Relax the severity of device init failure

[Henrik Rydberg]

The device init is used to reset the accelerometer. Failure to reset
is not severe enough to stop loading the module or to resume from
hibernation.  This patch relaxes failure to a warning and drops
output in case of success.

Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
---
 drivers/hwmon/applesmc.c |   24 ++----------------------
 1 files changed, 2 insertions(+), 22 deletions(-)

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index d7a5b75..375e464 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -458,21 +458,8 @@ static int applesmc_device_init(void)
 	mutex_lock(&applesmc_lock);
 
 	for (total = INIT_TIMEOUT_MSECS; total > 0; total -= INIT_WAIT_MSECS) {
-		if (debug)
-			printk(KERN_DEBUG "applesmc try %d\n", total);
 		if (!applesmc_read_key(MOTION_SENSOR_KEY, buffer, 2) &&
 				(buffer[0] != 0x00 || buffer[1] != 0x00)) {
-			if (total == INIT_TIMEOUT_MSECS) {
-				printk(KERN_DEBUG "applesmc: device has"
-						" already been initialized"
-						" (0x%02x, 0x%02x).\n",
-						buffer[0], buffer[1]);
-			} else {
-				printk(KERN_DEBUG "applesmc: device"
-						" successfully initialized"
-						" (0x%02x, 0x%02x).\n",
-						buffer[0], buffer[1]);
-			}
 			ret = 0;
 			goto out;
 		}
@@ -512,13 +499,8 @@ static int applesmc_get_fan_count(void)
 /* Device model stuff */
 static int applesmc_probe(struct platform_device *dev)
 {
-	int ret;
+	applesmc_device_init();
 
-	ret = applesmc_device_init();
-	if (ret)
-		return ret;
-
-	pr_info("device successfully initialized\n");
 	return 0;
 }
 
@@ -535,9 +517,7 @@ static int applesmc_pm_resume(struct device *dev)
 /* Reinitialize device on resume from hibernation */
 static int applesmc_pm_restore(struct device *dev)
 {
-	int ret = applesmc_device_init();
-	if (ret)
-		return ret;
+	applesmc_device_init();
 	return applesmc_pm_resume(dev);
 }
 
-- 
1.7.1

Re: [lm-sensors] [PATCH 1/8] hwmon: applesmc: Relax the severity of device init failure

[Guenter Roeck]

Hi Henrik,

On Sun, Oct 31, 2010 at 03:50:27AM -0400, Henrik Rydberg wrote:
> The device init is used to reset the accelerometer. Failure to reset
> is not severe enough to stop loading the module or to resume from
> hibernation.  This patch relaxes failure to a warning and drops
> output in case of success.
> 
> Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
> ---
>  drivers/hwmon/applesmc.c |   24 ++----------------------
>  1 files changed, 2 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index d7a5b75..375e464 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
> @@ -458,21 +458,8 @@ static int applesmc_device_init(void)
>  	mutex_lock(&applesmc_lock);
>  
Since the callers of applesmc_device_init() now all don't check the return value,
it might make sense to declare it as void and not bother returning an error.

Otherwise looks good.

Thanks,
Guenter

Re: [lm-sensors] [PATCH 1/8] hwmon: applesmc: Relax the severity of device init failure

[Henrik Rydberg]

On 11/02/2010 05:03 PM, Guenter Roeck wrote:

> Hi Henrik,
> 
> On Sun, Oct 31, 2010 at 03:50:27AM -0400, Henrik Rydberg wrote:
>> The device init is used to reset the accelerometer. Failure to reset
>> is not severe enough to stop loading the module or to resume from
>> hibernation.  This patch relaxes failure to a warning and drops
>> output in case of success.
>>
>> Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
>> ---
>>  drivers/hwmon/applesmc.c |   24 ++----------------------
>>  1 files changed, 2 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
>> index d7a5b75..375e464 100644
>> --- a/drivers/hwmon/applesmc.c
>> +++ b/drivers/hwmon/applesmc.c
>> @@ -458,21 +458,8 @@ static int applesmc_device_init(void)
>>  	mutex_lock(&applesmc_lock);
>>  
> Since the callers of applesmc_device_init() now all don't check the return value,
> it might make sense to declare it as void and not bother returning an error.
> 
> Otherwise looks good.


Sounds good, will change.

Henrik

[PATCH 2/8] hwmon: applesmc: Introduce a register lookup table

[Henrik Rydberg]

One main problem with the current driver is the inability to quickly
search for supported keys, resulting in detailed feature maps per
machine model which are cumbersome to maintain.

This patch adds a register lookup table, which enables binary search
for supported keys. The lookup also reduces the io frequency, so the
original mutex is replaced by locks around the actual io.

Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
---
 drivers/hwmon/applesmc.c |  515 +++++++++++++++++++++++++---------------------
 1 files changed, 281 insertions(+), 234 deletions(-)

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 375e464..7f030f0 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -32,6 +32,7 @@
 #include <linux/platform_device.h>
 #include <linux/input-polldev.h>
 #include <linux/kernel.h>
+#include <linux/slab.h>
 #include <linux/module.h>
 #include <linux/timer.h>
 #include <linux/dmi.h>
@@ -51,6 +52,7 @@
 
 #define APPLESMC_MAX_DATA_LENGTH 32
 
+/* wait up to 32 ms for a status change. */
 #define APPLESMC_MIN_WAIT	0x0040
 #define APPLESMC_MAX_WAIT	0x8000
 
@@ -196,6 +198,22 @@ struct dmi_match_data {
 	int temperature_set;
 };
 
+/* AppleSMC entry - cached register information */
+struct applesmc_entry {
+	char key[5];		/* four-letter key code */
+	u8 valid;		/* set when entry is successfully read once */
+	u8 len;			/* bounded by APPLESMC_MAX_DATA_LENGTH */
+	char type[5];		/* four-letter type code (padded) */
+};
+
+/* Register lookup and registers common to all SMCs */
+static struct applesmc_registers {
+	struct mutex mutex;		/* register read/write mutex */
+	unsigned int key_count;		/* number of SMC registers */
+	bool init_complete;		/* true when fully initialized */
+	struct applesmc_entry *entry;	/* key entries */
+} smcreg;
+
 static const int debug;
 static struct platform_device *pdev;
 static s16 rest_x;
@@ -217,14 +235,13 @@ static unsigned int fans_handled;
 /* Indicates which temperature sensors set to use. */
 static unsigned int applesmc_temperature_set;
 
-static DEFINE_MUTEX(applesmc_lock);
-
 /*
  * Last index written to key_at_index sysfs file, and value to use for all other
  * key_at_index_* sysfs files.
  */
 static unsigned int key_at_index;
 
+
 static struct workqueue_struct *applesmc_led_wq;
 
 /*
@@ -241,16 +258,10 @@ static int __wait_status(u8 val)
 	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
 		udelay(us);
 		if ((inb(APPLESMC_CMD_PORT) & APPLESMC_STATUS_MASK) == val) {
-			if (debug)
-				printk(KERN_DEBUG
-					"Waited %d us for status %x\n",
-					2 * us - APPLESMC_MIN_WAIT, val);
 			return 0;
 		}
 	}
 
-	pr_warn("wait status failed: %x != %x\n", val, inb(APPLESMC_CMD_PORT));
-
 	return -EIO;
 }
 
@@ -268,156 +279,228 @@ static int send_command(u8 cmd)
 		if ((inb(APPLESMC_CMD_PORT) & APPLESMC_STATUS_MASK) == 0x0c)
 			return 0;
 	}
-	pr_warn("command failed: %x -> %x\n", cmd, inb(APPLESMC_CMD_PORT));
 	return -EIO;
 }
 
-/*
- * applesmc_read_key - reads len bytes from a given key, and put them in buffer.
- * Returns zero on success or a negative error on failure. Callers must
- * hold applesmc_lock.
- */
-static int applesmc_read_key(const char* key, u8* buffer, u8 len)
+static int send_argument(const char *key)
 {
 	int i;
 
-	if (len > APPLESMC_MAX_DATA_LENGTH) {
-		pr_err("%s(): cannot read more than %d bytes\n",
-		       __func__, APPLESMC_MAX_DATA_LENGTH);
-		return -EINVAL;
-	}
-
-	if (send_command(APPLESMC_READ_CMD))
-		return -EIO;
-
 	for (i = 0; i < 4; i++) {
 		outb(key[i], APPLESMC_DATA_PORT);
 		if (__wait_status(0x04))
 			return -EIO;
 	}
-	if (debug)
-		printk(KERN_DEBUG "<%s", key);
+	return 0;
+}
+
+static int read_smc(u8 cmd, const char *key, u8 *buffer, u8 len)
+{
+	int i;
+
+	if (send_command(cmd) || send_argument(key)) {
+		pr_warn("%s: read arg fail\n", key);
+		return -EIO;
+	}
 
 	outb(len, APPLESMC_DATA_PORT);
-	if (debug)
-		printk(KERN_DEBUG ">%x", len);
 
 	for (i = 0; i < len; i++) {
-		if (__wait_status(0x05))
+		if (__wait_status(0x05)) {
+			pr_warn("%s: read data fail\n", key);
 			return -EIO;
+		}
 		buffer[i] = inb(APPLESMC_DATA_PORT);
-		if (debug)
-			printk(KERN_DEBUG "<%x", buffer[i]);
 	}
-	if (debug)
-		printk(KERN_DEBUG "\n");
 
 	return 0;
 }
 
-/*
- * applesmc_write_key - writes len bytes from buffer to a given key.
- * Returns zero on success or a negative error on failure. Callers must
- * hold applesmc_lock.
- */
-static int applesmc_write_key(const char* key, u8* buffer, u8 len)
+static int write_smc(u8 cmd, const char *key, const u8 *buffer, u8 len)
 {
 	int i;
 
-	if (len > APPLESMC_MAX_DATA_LENGTH) {
-		pr_err("%s(): cannot write more than %d bytes\n",
-		       __func__, APPLESMC_MAX_DATA_LENGTH);
-		return -EINVAL;
-	}
-
-	if (send_command(APPLESMC_WRITE_CMD))
+	if (send_command(cmd) || send_argument(key)) {
+		pr_warn("%s: write arg fail\n", key);
 		return -EIO;
-
-	for (i = 0; i < 4; i++) {
-		outb(key[i], APPLESMC_DATA_PORT);
-		if (__wait_status(0x04))
-			return -EIO;
 	}
 
 	outb(len, APPLESMC_DATA_PORT);
 
 	for (i = 0; i < len; i++) {
-		if (__wait_status(0x04))
+		if (__wait_status(0x04)) {
+			pr_warn("%s: write data fail\n", key);
 			return -EIO;
+		}
 		outb(buffer[i], APPLESMC_DATA_PORT);
 	}
 
 	return 0;
 }
 
+static int read_register_count(unsigned int *count)
+{
+	__be32 be;
+	int ret;
+
+	ret = read_smc(APPLESMC_READ_CMD, KEY_COUNT_KEY, (u8 *)&be, 4);
+	if (ret)
+		return ret;
+
+	*count = be32_to_cpu(be);
+	return 0;
+}
+
 /*
- * applesmc_get_key_at_index - get key at index, and put the result in key
- * (char[6]). Returns zero on success or a negative error on failure. Callers
- * must hold applesmc_lock.
+ * Serialized I/O
+ *
+ * Returns zero on success or a negative error on failure.
+ * All functions below are concurrency safe - callers should NOT hold lock.
  */
-static int applesmc_get_key_at_index(int index, char* key)
+
+static int applesmc_read_entry(const struct applesmc_entry *entry,
+			       u8 *buf, u8 len)
 {
-	int i;
-	u8 readkey[4];
-	readkey[0] = index >> 24;
-	readkey[1] = index >> 16;
-	readkey[2] = index >> 8;
-	readkey[3] = index;
+	int ret;
 
-	if (send_command(APPLESMC_GET_KEY_BY_INDEX_CMD))
-		return -EIO;
+	if (entry->len != len)
+		return -EINVAL;
+	mutex_lock(&smcreg.mutex);
+	ret = read_smc(APPLESMC_READ_CMD, entry->key, buf, len);
+	mutex_unlock(&smcreg.mutex);
 
-	for (i = 0; i < 4; i++) {
-		outb(readkey[i], APPLESMC_DATA_PORT);
-		if (__wait_status(0x04))
-			return -EIO;
-	}
+	return ret;
+}
+
+static int applesmc_write_entry(const struct applesmc_entry *entry,
+				const u8 *buf, u8 len)
+{
+	int ret;
 
-	outb(4, APPLESMC_DATA_PORT);
+	if (entry->len != len)
+		return -EINVAL;
+	mutex_lock(&smcreg.mutex);
+	ret = write_smc(APPLESMC_WRITE_CMD, entry->key, buf, len);
+	mutex_unlock(&smcreg.mutex);
+	return ret;
+}
 
-	for (i = 0; i < 4; i++) {
-		if (__wait_status(0x05))
-			return -EIO;
-		key[i] = inb(APPLESMC_DATA_PORT);
+static int applesmc_get_entry_by_index(int index, struct applesmc_entry *entry)
+{
+	struct applesmc_entry *cache = &smcreg.entry[index];
+	__be32 be;
+	int ret;
+
+	if (cache->valid) {
+		memcpy(entry, cache, sizeof(*entry));
+		return 0;
 	}
-	key[4] = 0;
 
-	return 0;
+	mutex_lock(&smcreg.mutex);
+
+	be = cpu_to_be32(index);
+	ret = read_smc(APPLESMC_GET_KEY_BY_INDEX_CMD, (u8 *)&be, cache->key, 4);
+	if (ret)
+		goto out;
+	ret = read_smc(APPLESMC_GET_KEY_TYPE_CMD, cache->key, &cache->len, 6);
+	if (ret)
+		goto out;
+
+	cache->type[4] = 0;
+	cache->valid = 1;
+	memcpy(entry, cache, sizeof(*entry));
+
+out:
+	mutex_unlock(&smcreg.mutex);
+	return ret;
 }
 
-/*
- * applesmc_get_key_type - get key type, and put the result in type (char[6]).
- * Returns zero on success or a negative error on failure. Callers must
- * hold applesmc_lock.
- */
-static int applesmc_get_key_type(char* key, char* type)
+static int applesmc_get_lower_bound(unsigned int *lo, const char *key)
 {
-	int i;
-
-	if (send_command(APPLESMC_GET_KEY_TYPE_CMD))
-		return -EIO;
+	int begin = 0, end = smcreg.key_count;
+	struct applesmc_entry entry;
+	int ret;
 
-	for (i = 0; i < 4; i++) {
-		outb(key[i], APPLESMC_DATA_PORT);
-		if (__wait_status(0x04))
-			return -EIO;
+	while (begin != end) {
+		int middle = begin + (end - begin) / 2;
+		ret = applesmc_get_entry_by_index(middle, &entry);
+		if (ret)
+			return ret;
+		if (strcmp(entry.key, key) < 0)
+			begin = middle + 1;
+		else
+			end = middle;
 	}
 
-	outb(6, APPLESMC_DATA_PORT);
+	*lo = begin;
+	return 0;
+}
 
-	for (i = 0; i < 6; i++) {
-		if (__wait_status(0x05))
-			return -EIO;
-		type[i] = inb(APPLESMC_DATA_PORT);
+static int applesmc_get_upper_bound(unsigned int *hi, const char *key)
+{
+	int begin = 0, end = smcreg.key_count;
+	struct applesmc_entry entry;
+	int ret;
+
+	while (begin != end) {
+		int middle = begin + (end - begin) / 2;
+		ret = applesmc_get_entry_by_index(middle, &entry);
+		if (ret)
+			return ret;
+		if (strcmp(key, entry.key) < 0)
+			end = middle;
+		else
+			begin = middle + 1;
 	}
-	type[5] = 0;
 
+	*hi = begin;
 	return 0;
 }
 
+static int applesmc_get_entry_by_key(const char *key,
+				     struct applesmc_entry *entry)
+{
+	int begin, end;
+	int ret;
+
+	ret = applesmc_get_lower_bound(&begin, key);
+	if (ret)
+		return ret;
+	ret = applesmc_get_upper_bound(&end, key);
+	if (ret)
+		return ret;
+	if (end - begin != 1)
+		return -EINVAL;
+
+	return applesmc_get_entry_by_index(begin, entry);
+}
+
+static int applesmc_read_key(const char *key, u8 *buffer, u8 len)
+{
+	struct applesmc_entry entry;
+	int ret;
+
+	ret = applesmc_get_entry_by_key(key, &entry);
+	if (ret)
+		return ret;
+
+	return applesmc_read_entry(&entry, buffer, len);
+}
+
+static int applesmc_write_key(const char *key, const u8 *buffer, u8 len)
+{
+	struct applesmc_entry entry;
+	int ret;
+
+	ret = applesmc_get_entry_by_key(key, &entry);
+	if (ret)
+		return ret;
+
+	return applesmc_write_entry(&entry, buffer, len);
+}
+
 /*
- * applesmc_read_motion_sensor - Read motion sensor (X, Y or Z). Callers must
- * hold applesmc_lock.
+ * applesmc_read_motion_sensor - Read motion sensor (X, Y or Z).
  */
 static int applesmc_read_motion_sensor(int index, s16* value)
 {
@@ -455,8 +538,6 @@ static int applesmc_device_init(void)
 	if (!applesmc_accelerometer)
 		return 0;
 
-	mutex_lock(&applesmc_lock);
-
 	for (total = INIT_TIMEOUT_MSECS; total > 0; total -= INIT_WAIT_MSECS) {
 		if (!applesmc_read_key(MOTION_SENSOR_KEY, buffer, 2) &&
 				(buffer[0] != 0x00 || buffer[1] != 0x00)) {
@@ -472,33 +553,90 @@ static int applesmc_device_init(void)
 	pr_warn("failed to init the device\n");
 
 out:
-	mutex_unlock(&applesmc_lock);
 	return ret;
 }
 
 /*
- * applesmc_get_fan_count - get the number of fans. Callers must NOT hold
- * applesmc_lock.
+ * applesmc_get_fan_count - get the number of fans.
  */
 static int applesmc_get_fan_count(void)
 {
 	int ret;
 	u8 buffer[1];
 
-	mutex_lock(&applesmc_lock);
-
 	ret = applesmc_read_key(FANS_COUNT, buffer, 1);
 
-	mutex_unlock(&applesmc_lock);
 	if (ret)
 		return ret;
 	else
 		return buffer[0];
 }
 
+/*
+ * applesmc_init_smcreg_try - Try to initialize register cache. Idempotent.
+ */
+static int applesmc_init_smcreg_try(void)
+{
+	struct applesmc_registers *s = &smcreg;
+	int ret;
+
+	if (s->init_complete)
+		return 0;
+
+	mutex_init(&s->mutex);
+
+	ret = read_register_count(&s->key_count);
+	if (ret)
+		return ret;
+
+	if (!s->entry)
+		s->entry = kcalloc(s->key_count, sizeof(*s->entry), GFP_KERNEL);
+	if (!s->entry)
+		return -ENOMEM;
+
+	s->init_complete = true;
+
+	pr_info("key=%d\n", s->key_count);
+
+	return 0;
+}
+
+/*
+ * applesmc_init_smcreg - Initialize register cache.
+ *
+ * Retries until initialization is successful, or the operation times out.
+ *
+ */
+static int applesmc_init_smcreg(void)
+{
+	int ms, ret;
+
+	for (ms = 0; ms < INIT_TIMEOUT_MSECS; ms += INIT_WAIT_MSECS) {
+		ret = applesmc_init_smcreg_try();
+		if (!ret)
+			return 0;
+		pr_warn("slow init, retrying\n");
+		msleep(INIT_WAIT_MSECS);
+	}
+
+	return ret;
+}
+
+static void applesmc_destroy_smcreg(void)
+{
+	kfree(smcreg.entry);
+	smcreg.entry = NULL;
+}
+
 /* Device model stuff */
 static int applesmc_probe(struct platform_device *dev)
 {
+	int ret;
+
+	ret = applesmc_init_smcreg();
+	if (ret)
+		return ret;
+
 	applesmc_device_init();
 
 	return 0;
@@ -507,10 +645,8 @@ static int applesmc_probe(struct platform_device *dev)
 /* Synchronize device with memorized backlight state */
 static int applesmc_pm_resume(struct device *dev)
 {
-	mutex_lock(&applesmc_lock);
 	if (applesmc_light)
 		applesmc_write_key(BACKLIGHT_KEY, backlight_state, 2);
-	mutex_unlock(&applesmc_lock);
 	return 0;
 }
 
@@ -551,20 +687,15 @@ static void applesmc_idev_poll(struct input_polled_dev *dev)
 	struct input_dev *idev = dev->input;
 	s16 x, y;
 
-	mutex_lock(&applesmc_lock);
-
 	if (applesmc_read_motion_sensor(SENSOR_X, &x))
-		goto out;
+		return;
 	if (applesmc_read_motion_sensor(SENSOR_Y, &y))
-		goto out;
+		return;
 
 	x = -x;
 	input_report_abs(idev, ABS_X, x - rest_x);
 	input_report_abs(idev, ABS_Y, y - rest_y);
 	input_sync(idev);
-
-out:
-	mutex_unlock(&applesmc_lock);
 }
 
 /* Sysfs Files */
@@ -581,8 +712,6 @@ static ssize_t applesmc_position_show(struct device *dev,
 	int ret;
 	s16 x, y, z;
 
-	mutex_lock(&applesmc_lock);
-
 	ret = applesmc_read_motion_sensor(SENSOR_X, &x);
 	if (ret)
 		goto out;
@@ -594,7 +723,6 @@ static ssize_t applesmc_position_show(struct device *dev,
 		goto out;
 
 out:
-	mutex_unlock(&applesmc_lock);
 	if (ret)
 		return ret;
 	else
@@ -604,18 +732,17 @@ out:
 static ssize_t applesmc_light_show(struct device *dev,
 				struct device_attribute *attr, char *sysfsbuf)
 {
+	struct applesmc_entry entry;
 	static int data_length;
 	int ret;
 	u8 left = 0, right = 0;
-	u8 buffer[10], query[6];
-
-	mutex_lock(&applesmc_lock);
+	u8 buffer[10];
 
 	if (!data_length) {
-		ret = applesmc_get_key_type(LIGHT_SENSOR_LEFT_KEY, query);
+		ret = applesmc_get_entry_by_key(LIGHT_SENSOR_LEFT_KEY, &entry);
 		if (ret)
 			goto out;
-		data_length = clamp_val(query[0], 0, 10);
+		data_length = clamp_val(entry.len, 0, 10);
 		pr_info("light sensor data length set to %d\n", data_length);
 	}
 
@@ -632,7 +759,6 @@ static ssize_t applesmc_light_show(struct device *dev,
 	right = buffer[2];
 
 out:
-	mutex_unlock(&applesmc_lock);
 	if (ret)
 		return ret;
 	else
@@ -661,14 +787,10 @@ static ssize_t applesmc_show_temperature(struct device *dev,
 	const char* key =
 		temperature_sensors_sets[applesmc_temperature_set][attr->index];
 
-	mutex_lock(&applesmc_lock);
-
 	ret = applesmc_read_key(key, buffer, 2);
 	temp = buffer[0]*1000;
 	temp += (buffer[1] >> 6) * 250;
 
-	mutex_unlock(&applesmc_lock);
-
 	if (ret)
 		return ret;
 	else
@@ -691,12 +813,9 @@ static ssize_t applesmc_show_fan_speed(struct device *dev,
 	newkey[3] = fan_speed_keys[sensor_attr->nr][3];
 	newkey[4] = 0;
 
-	mutex_lock(&applesmc_lock);
-
 	ret = applesmc_read_key(newkey, buffer, 2);
 	speed = ((buffer[0] << 8 | buffer[1]) >> 2);
 
-	mutex_unlock(&applesmc_lock);
 	if (ret)
 		return ret;
 	else
@@ -725,13 +844,10 @@ static ssize_t applesmc_store_fan_speed(struct device *dev,
 	newkey[3] = fan_speed_keys[sensor_attr->nr][3];
 	newkey[4] = 0;
 
-	mutex_lock(&applesmc_lock);
-
 	buffer[0] = (speed >> 6) & 0xff;
 	buffer[1] = (speed << 2) & 0xff;
 	ret = applesmc_write_key(newkey, buffer, 2);
 
-	mutex_unlock(&applesmc_lock);
 	if (ret)
 		return ret;
 	else
@@ -746,12 +862,9 @@ static ssize_t applesmc_show_fan_manual(struct device *dev,
 	u8 buffer[2];
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
 
-	mutex_lock(&applesmc_lock);
-
 	ret = applesmc_read_key(FANS_MANUAL, buffer, 2);
 	manual = ((buffer[0] << 8 | buffer[1]) >> attr->index) & 0x01;
 
-	mutex_unlock(&applesmc_lock);
 	if (ret)
 		return ret;
 	else
@@ -770,8 +883,6 @@ static ssize_t applesmc_store_fan_manual(struct device *dev,
 
 	input = simple_strtoul(sysfsbuf, NULL, 10);
 
-	mutex_lock(&applesmc_lock);
-
 	ret = applesmc_read_key(FANS_MANUAL, buffer, 2);
 	val = (buffer[0] << 8 | buffer[1]);
 	if (ret)
@@ -788,7 +899,6 @@ static ssize_t applesmc_store_fan_manual(struct device *dev,
 	ret = applesmc_write_key(FANS_MANUAL, buffer, 2);
 
 out:
-	mutex_unlock(&applesmc_lock);
 	if (ret)
 		return ret;
 	else
@@ -810,12 +920,9 @@ static ssize_t applesmc_show_fan_position(struct device *dev,
 	newkey[3] = FAN_POSITION[3];
 	newkey[4] = 0;
 
-	mutex_lock(&applesmc_lock);
-
 	ret = applesmc_read_key(newkey, buffer, 16);
 	buffer[16] = 0;
 
-	mutex_unlock(&applesmc_lock);
 	if (ret)
 		return ret;
 	else
@@ -831,18 +938,14 @@ static ssize_t applesmc_calibrate_show(struct device *dev,
 static ssize_t applesmc_calibrate_store(struct device *dev,
 	struct device_attribute *attr, const char *sysfsbuf, size_t count)
 {
-	mutex_lock(&applesmc_lock);
 	applesmc_calibrate();
-	mutex_unlock(&applesmc_lock);
 
 	return count;
 }
 
 static void applesmc_backlight_set(struct work_struct *work)
 {
-	mutex_lock(&applesmc_lock);
 	applesmc_write_key(BACKLIGHT_KEY, backlight_state, 2);
-	mutex_unlock(&applesmc_lock);
 }
 static DECLARE_WORK(backlight_work, &applesmc_backlight_set);
 
@@ -865,13 +968,10 @@ static ssize_t applesmc_key_count_show(struct device *dev,
 	u8 buffer[4];
 	u32 count;
 
-	mutex_lock(&applesmc_lock);
-
 	ret = applesmc_read_key(KEY_COUNT_KEY, buffer, 4);
 	count = ((u32)buffer[0]<<24) + ((u32)buffer[1]<<16) +
 						((u32)buffer[2]<<8) + buffer[3];
 
-	mutex_unlock(&applesmc_lock);
 	if (ret)
 		return ret;
 	else
@@ -881,113 +981,56 @@ static ssize_t applesmc_key_count_show(struct device *dev,
 static ssize_t applesmc_key_at_index_read_show(struct device *dev,
 				struct device_attribute *attr, char *sysfsbuf)
 {
-	char key[5];
-	char info[6];
+	struct applesmc_entry entry;
 	int ret;
 
-	mutex_lock(&applesmc_lock);
-
-	ret = applesmc_get_key_at_index(key_at_index, key);
-
-	if (ret || !key[0]) {
-		mutex_unlock(&applesmc_lock);
-
-		return -EINVAL;
-	}
-
-	ret = applesmc_get_key_type(key, info);
-
-	if (ret) {
-		mutex_unlock(&applesmc_lock);
-
+	ret = applesmc_get_entry_by_index(key_at_index, &entry);
+	if (ret)
 		return ret;
-	}
-
-	/*
-	 * info[0] maximum value (APPLESMC_MAX_DATA_LENGTH) is much lower than
-	 * PAGE_SIZE, so we don't need any checks before writing to sysfsbuf.
-	 */
-	ret = applesmc_read_key(key, sysfsbuf, info[0]);
-
-	mutex_unlock(&applesmc_lock);
-
-	if (!ret) {
-		return info[0];
-	} else {
+	ret = applesmc_read_entry(&entry, sysfsbuf, entry.len);
+	if (ret)
 		return ret;
-	}
+
+	return entry.len;
 }
 
 static ssize_t applesmc_key_at_index_data_length_show(struct device *dev,
 				struct device_attribute *attr, char *sysfsbuf)
 {
-	char key[5];
-	char info[6];
+	struct applesmc_entry entry;
 	int ret;
 
-	mutex_lock(&applesmc_lock);
-
-	ret = applesmc_get_key_at_index(key_at_index, key);
-
-	if (ret || !key[0]) {
-		mutex_unlock(&applesmc_lock);
-
-		return -EINVAL;
-	}
-
-	ret = applesmc_get_key_type(key, info);
-
-	mutex_unlock(&applesmc_lock);
-
-	if (!ret)
-		return snprintf(sysfsbuf, PAGE_SIZE, "%d\n", info[0]);
-	else
+	ret = applesmc_get_entry_by_index(key_at_index, &entry);
+	if (ret)
 		return ret;
+
+	return snprintf(sysfsbuf, PAGE_SIZE, "%d\n", entry.len);
 }
 
 static ssize_t applesmc_key_at_index_type_show(struct device *dev,
 				struct device_attribute *attr, char *sysfsbuf)
 {
-	char key[5];
-	char info[6];
+	struct applesmc_entry entry;
 	int ret;
 
-	mutex_lock(&applesmc_lock);
-
-	ret = applesmc_get_key_at_index(key_at_index, key);
-
-	if (ret || !key[0]) {
-		mutex_unlock(&applesmc_lock);
-
-		return -EINVAL;
-	}
-
-	ret = applesmc_get_key_type(key, info);
-
-	mutex_unlock(&applesmc_lock);
-
-	if (!ret)
-		return snprintf(sysfsbuf, PAGE_SIZE, "%s\n", info+1);
-	else
+	ret = applesmc_get_entry_by_index(key_at_index, &entry);
+	if (ret)
 		return ret;
+
+	return snprintf(sysfsbuf, PAGE_SIZE, "%s\n", entry.type);
 }
 
 static ssize_t applesmc_key_at_index_name_show(struct device *dev,
 				struct device_attribute *attr, char *sysfsbuf)
 {
-	char key[5];
+	struct applesmc_entry entry;
 	int ret;
 
-	mutex_lock(&applesmc_lock);
-
-	ret = applesmc_get_key_at_index(key_at_index, key);
-
-	mutex_unlock(&applesmc_lock);
+	ret = applesmc_get_entry_by_index(key_at_index, &entry);
+	if (ret)
+		return ret;
 
-	if (!ret && key[0])
-		return snprintf(sysfsbuf, PAGE_SIZE, "%s\n", key);
-	else
-		return -EINVAL;
+	return snprintf(sysfsbuf, PAGE_SIZE, "%s\n", entry.key);
 }
 
 static ssize_t applesmc_key_at_index_show(struct device *dev,
@@ -999,12 +1042,8 @@ static ssize_t applesmc_key_at_index_show(struct device *dev,
 static ssize_t applesmc_key_at_index_store(struct device *dev,
 	struct device_attribute *attr, const char *sysfsbuf, size_t count)
 {
-	mutex_lock(&applesmc_lock);
-
 	key_at_index = simple_strtoul(sysfsbuf, NULL, 10);
 
-	mutex_unlock(&applesmc_lock);
-
 	return count;
 }
 
@@ -1640,10 +1679,15 @@ static int __init applesmc_init(void)
 		goto out_driver;
 	}
 
-	ret = sysfs_create_file(&pdev->dev.kobj, &dev_attr_name.attr);
+	/* create register cache */
+	ret = applesmc_init_smcreg();
 	if (ret)
 		goto out_device;
 
+	ret = sysfs_create_file(&pdev->dev.kobj, &dev_attr_name.attr);
+	if (ret)
+		goto out_smcreg;
+
 	/* Create key enumeration sysfs files */
 	ret = sysfs_create_group(&pdev->dev.kobj, &key_enumeration_group);
 	if (ret)
@@ -1745,6 +1789,8 @@ out_fans:
 	sysfs_remove_group(&pdev->dev.kobj, &key_enumeration_group);
 out_name:
 	sysfs_remove_file(&pdev->dev.kobj, &dev_attr_name.attr);
+out_smcreg:
+	applesmc_destroy_smcreg();
 out_device:
 	platform_device_unregister(pdev);
 out_driver:
@@ -1773,6 +1819,7 @@ static void __exit applesmc_exit(void)
 				   &fan_attribute_groups[--fans_handled]);
 	sysfs_remove_group(&pdev->dev.kobj, &key_enumeration_group);
 	sysfs_remove_file(&pdev->dev.kobj, &dev_attr_name.attr);
+	applesmc_destroy_smcreg();
 	platform_device_unregister(pdev);
 	platform_driver_unregister(&applesmc_driver);
 	release_region(APPLESMC_DATA_PORT, APPLESMC_NR_PORTS);
-- 
1.7.1

Re: [lm-sensors] [PATCH 2/8] hwmon: applesmc: Introduce a register lookup table

[Guenter Roeck]

Hi Henrik,

On Sun, Oct 31, 2010 at 03:50:28AM -0400, Henrik Rydberg wrote:
> One main problem with the current driver is the inability to quickly
> search for supported keys, resulting in detailed feature maps per
> machine model which are cumbersome to maintain.
> 
> This patch adds a register lookup table, which enables binary search
> for supported keys. The lookup also reduces the io frequency, so the
> original mutex is replaced by locks around the actual io.
> 
> Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
> ---
>  drivers/hwmon/applesmc.c |  515 +++++++++++++++++++++++++---------------------
>  1 files changed, 281 insertions(+), 234 deletions(-)
> 
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index 375e464..7f030f0 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
> @@ -32,6 +32,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/input-polldev.h>
>  #include <linux/kernel.h>
> +#include <linux/slab.h>
>  #include <linux/module.h>
>  #include <linux/timer.h>
>  #include <linux/dmi.h>
> @@ -51,6 +52,7 @@
> 
>  #define APPLESMC_MAX_DATA_LENGTH 32
> 
> +/* wait up to 32 ms for a status change. */
>  #define APPLESMC_MIN_WAIT      0x0040
>  #define APPLESMC_MAX_WAIT      0x8000
> 
> @@ -196,6 +198,22 @@ struct dmi_match_data {
>         int temperature_set;
>  };
> 
> +/* AppleSMC entry - cached register information */
> +struct applesmc_entry {
> +       char key[5];            /* four-letter key code */
> +       u8 valid;               /* set when entry is successfully read once */
> +       u8 len;                 /* bounded by APPLESMC_MAX_DATA_LENGTH */
> +       char type[5];           /* four-letter type code (padded) */
> +};
> +
> +/* Register lookup and registers common to all SMCs */
> +static struct applesmc_registers {
> +       struct mutex mutex;             /* register read/write mutex */
> +       unsigned int key_count;         /* number of SMC registers */
> +       bool init_complete;             /* true when fully initialized */
> +       struct applesmc_entry *entry;   /* key entries */
> +} smcreg;
> +
>  static const int debug;
>  static struct platform_device *pdev;
>  static s16 rest_x;
> @@ -217,14 +235,13 @@ static unsigned int fans_handled;
>  /* Indicates which temperature sensors set to use. */
>  static unsigned int applesmc_temperature_set;
> 
> -static DEFINE_MUTEX(applesmc_lock);
> -
>  /*
>   * Last index written to key_at_index sysfs file, and value to use for all other
>   * key_at_index_* sysfs files.
>   */
>  static unsigned int key_at_index;
> 
> +

unnecessary blank line

>  static struct workqueue_struct *applesmc_led_wq;
> 
>  /*
> @@ -241,16 +258,10 @@ static int __wait_status(u8 val)
>         for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
>                 udelay(us);
>                 if ((inb(APPLESMC_CMD_PORT) & APPLESMC_STATUS_MASK) == val) {
> -                       if (debug)
> -                               printk(KERN_DEBUG
> -                                       "Waited %d us for status %x\n",
> -                                       2 * us - APPLESMC_MIN_WAIT, val);
>                         return 0;
>                 }
>         }
> 
> -       pr_warn("wait status failed: %x != %x\n", val, inb(APPLESMC_CMD_PORT));
> -
>         return -EIO;
>  }
> 
> @@ -268,156 +279,228 @@ static int send_command(u8 cmd)
>                 if ((inb(APPLESMC_CMD_PORT) & APPLESMC_STATUS_MASK) == 0x0c)
>                         return 0;
>         }
> -       pr_warn("command failed: %x -> %x\n", cmd, inb(APPLESMC_CMD_PORT));
>         return -EIO;
>  }
> 
> -/*
> - * applesmc_read_key - reads len bytes from a given key, and put them in buffer.
> - * Returns zero on success or a negative error on failure. Callers must
> - * hold applesmc_lock.
> - */
> -static int applesmc_read_key(const char* key, u8* buffer, u8 len)
> +static int send_argument(const char *key)
>  {
>         int i;
> 
> -       if (len > APPLESMC_MAX_DATA_LENGTH) {
> -               pr_err("%s(): cannot read more than %d bytes\n",
> -                      __func__, APPLESMC_MAX_DATA_LENGTH);
> -               return -EINVAL;
> -       }
> -
> -       if (send_command(APPLESMC_READ_CMD))
> -               return -EIO;
> -
>         for (i = 0; i < 4; i++) {
>                 outb(key[i], APPLESMC_DATA_PORT);
>                 if (__wait_status(0x04))
>                         return -EIO;
>         }
> -       if (debug)
> -               printk(KERN_DEBUG "<%s", key);
> +       return 0;
> +}
> +
> +static int read_smc(u8 cmd, const char *key, u8 *buffer, u8 len)
> +{
> +       int i;
> +
> +       if (send_command(cmd) || send_argument(key)) {
> +               pr_warn("%s: read arg fail\n", key);
> +               return -EIO;
> +       }
> 
>         outb(len, APPLESMC_DATA_PORT);
> -       if (debug)
> -               printk(KERN_DEBUG ">%x", len);
> 
>         for (i = 0; i < len; i++) {
> -               if (__wait_status(0x05))
> +               if (__wait_status(0x05)) {
> +                       pr_warn("%s: read data fail\n", key);
>                         return -EIO;
> +               }
>                 buffer[i] = inb(APPLESMC_DATA_PORT);
> -               if (debug)
> -                       printk(KERN_DEBUG "<%x", buffer[i]);
>         }
> -       if (debug)
> -               printk(KERN_DEBUG "\n");
> 
>         return 0;
>  }
> 
> -/*
> - * applesmc_write_key - writes len bytes from buffer to a given key.
> - * Returns zero on success or a negative error on failure. Callers must
> - * hold applesmc_lock.
> - */
> -static int applesmc_write_key(const char* key, u8* buffer, u8 len)
> +static int write_smc(u8 cmd, const char *key, const u8 *buffer, u8 len)
>  {
>         int i;
> 
> -       if (len > APPLESMC_MAX_DATA_LENGTH) {
> -               pr_err("%s(): cannot write more than %d bytes\n",
> -                      __func__, APPLESMC_MAX_DATA_LENGTH);
> -               return -EINVAL;
> -       }
> -
> -       if (send_command(APPLESMC_WRITE_CMD))
> +       if (send_command(cmd) || send_argument(key)) {
> +               pr_warn("%s: write arg fail\n", key);
>                 return -EIO;
> -
> -       for (i = 0; i < 4; i++) {
> -               outb(key[i], APPLESMC_DATA_PORT);
> -               if (__wait_status(0x04))
> -                       return -EIO;
>         }
> 
>         outb(len, APPLESMC_DATA_PORT);
> 
>         for (i = 0; i < len; i++) {
> -               if (__wait_status(0x04))
> +               if (__wait_status(0x04)) {
> +                       pr_warn("%s: write data fail\n", key);
>                         return -EIO;
> +               }
>                 outb(buffer[i], APPLESMC_DATA_PORT);
>         }
> 
>         return 0;
>  }
> 
> +static int read_register_count(unsigned int *count)
> +{
> +       __be32 be;
> +       int ret;
> +
> +       ret = read_smc(APPLESMC_READ_CMD, KEY_COUNT_KEY, (u8 *)&be, 4);
> +       if (ret)
> +               return ret;
> +
> +       *count = be32_to_cpu(be);
> +       return 0;
> +}
> +
>  /*
> - * applesmc_get_key_at_index - get key at index, and put the result in key
> - * (char[6]). Returns zero on success or a negative error on failure. Callers
> - * must hold applesmc_lock.
> + * Serialized I/O
> + *
> + * Returns zero on success or a negative error on failure.
> + * All functions below are concurrency safe - callers should NOT hold lock.
>   */
> -static int applesmc_get_key_at_index(int index, char* key)
> +
> +static int applesmc_read_entry(const struct applesmc_entry *entry,
> +                              u8 *buf, u8 len)
>  {
> -       int i;
> -       u8 readkey[4];
> -       readkey[0] = index >> 24;
> -       readkey[1] = index >> 16;
> -       readkey[2] = index >> 8;
> -       readkey[3] = index;
> +       int ret;
> 
> -       if (send_command(APPLESMC_GET_KEY_BY_INDEX_CMD))
> -               return -EIO;
> +       if (entry->len != len)
> +               return -EINVAL;
> +       mutex_lock(&smcreg.mutex);
> +       ret = read_smc(APPLESMC_READ_CMD, entry->key, buf, len);
> +       mutex_unlock(&smcreg.mutex);
> 
> -       for (i = 0; i < 4; i++) {
> -               outb(readkey[i], APPLESMC_DATA_PORT);
> -               if (__wait_status(0x04))
> -                       return -EIO;
> -       }
> +       return ret;
> +}
> +
> +static int applesmc_write_entry(const struct applesmc_entry *entry,
> +                               const u8 *buf, u8 len)
> +{
> +       int ret;
> 
> -       outb(4, APPLESMC_DATA_PORT);
> +       if (entry->len != len)
> +               return -EINVAL;
> +       mutex_lock(&smcreg.mutex);
> +       ret = write_smc(APPLESMC_WRITE_CMD, entry->key, buf, len);
> +       mutex_unlock(&smcreg.mutex);
> +       return ret;
> +}
> 
> -       for (i = 0; i < 4; i++) {
> -               if (__wait_status(0x05))
> -                       return -EIO;
> -               key[i] = inb(APPLESMC_DATA_PORT);
> +static int applesmc_get_entry_by_index(int index, struct applesmc_entry *entry)
> +{

One thing I don't understand about the whole caching scheme - why don't you just
return (and use) a pointer to the cached entry ? Seems to me that would be much simpler.
If you want to return error types, you could use ERR_PTR, PTR_ERR, and IS_ERR.

> +       struct applesmc_entry *cache = &smcreg.entry[index];
> +       __be32 be;
> +       int ret;
> +
> +       if (cache->valid) {
> +               memcpy(entry, cache, sizeof(*entry));
> +               return 0;
>         }
> -       key[4] = 0;
> 
> -       return 0;
> +       mutex_lock(&smcreg.mutex);
> +
> +       be = cpu_to_be32(index);
> +       ret = read_smc(APPLESMC_GET_KEY_BY_INDEX_CMD, (u8 *)&be, cache->key, 4);
> +       if (ret)
> +               goto out;
> +       ret = read_smc(APPLESMC_GET_KEY_TYPE_CMD, cache->key, &cache->len, 6);

This one is a bit odd. cache->len is an u8. You are reading 6 bytes into it.
I assume this is supposed to fill both cache->len and cache->type in a single operation.

Not sure if this is a good idea. Seems to depend a lot on the assumption that
fields are consecutive. Might be safer to read the data into a temporary
6 byte buffer and copy it into ->len and ->type afterwards.

If that is not acceptable, please add a comment describing what you are doing here
and why.

> +       if (ret)
> +               goto out;
> +
> +       cache->type[4] = 0;

Why read 6 bytes above if you overwrite the last byte anyway ?

> +       cache->valid = 1;
> +       memcpy(entry, cache, sizeof(*entry));
> +
> +out:
> +       mutex_unlock(&smcreg.mutex);
> +       return ret;
>  }
> 
> -/*
> - * applesmc_get_key_type - get key type, and put the result in type (char[6]).
> - * Returns zero on success or a negative error on failure. Callers must
> - * hold applesmc_lock.
> - */
> -static int applesmc_get_key_type(char* key, char* type)
> +static int applesmc_get_lower_bound(unsigned int *lo, const char *key)
>  {
> -       int i;
> -
> -       if (send_command(APPLESMC_GET_KEY_TYPE_CMD))
> -               return -EIO;
> +       int begin = 0, end = smcreg.key_count;
> +       struct applesmc_entry entry;
> +       int ret;
> 
> -       for (i = 0; i < 4; i++) {
> -               outb(key[i], APPLESMC_DATA_PORT);
> -               if (__wait_status(0x04))
> -                       return -EIO;
> +       while (begin != end) {
> +               int middle = begin + (end - begin) / 2;
> +               ret = applesmc_get_entry_by_index(middle, &entry);
> +               if (ret)
> +                       return ret;
> +               if (strcmp(entry.key, key) < 0)
> +                       begin = middle + 1;
> +               else
> +                       end = middle;
>         }
> 
> -       outb(6, APPLESMC_DATA_PORT);
> +       *lo = begin;
> +       return 0;
> +}
> 
> -       for (i = 0; i < 6; i++) {
> -               if (__wait_status(0x05))
> -                       return -EIO;
> -               type[i] = inb(APPLESMC_DATA_PORT);
> +static int applesmc_get_upper_bound(unsigned int *hi, const char *key)
> +{
> +       int begin = 0, end = smcreg.key_count;
> +       struct applesmc_entry entry;
> +       int ret;
> +
> +       while (begin != end) {
> +               int middle = begin + (end - begin) / 2;
> +               ret = applesmc_get_entry_by_index(middle, &entry);
> +               if (ret)
> +                       return ret;
> +               if (strcmp(key, entry.key) < 0)
> +                       end = middle;
> +               else
> +                       begin = middle + 1;
>         }
> -       type[5] = 0;
> 
> +       *hi = begin;
>         return 0;
>  }
> 
> +static int applesmc_get_entry_by_key(const char *key,
> +                                    struct applesmc_entry *entry)
> +{
> +       int begin, end;
> +       int ret;
> +
> +       ret = applesmc_get_lower_bound(&begin, key);
> +       if (ret)
> +               return ret;
> +       ret = applesmc_get_upper_bound(&end, key);
> +       if (ret)
> +               return ret;
> +       if (end - begin != 1)
> +               return -EINVAL;
> +
> +       return applesmc_get_entry_by_index(begin, entry);
> +}
> +
> +static int applesmc_read_key(const char *key, u8 *buffer, u8 len)
> +{
> +       struct applesmc_entry entry;
> +       int ret;
> +
> +       ret = applesmc_get_entry_by_key(key, &entry);
> +       if (ret)
> +               return ret;
> +
> +       return applesmc_read_entry(&entry, buffer, len);
> +}
> +
> +static int applesmc_write_key(const char *key, const u8 *buffer, u8 len)
> +{
> +       struct applesmc_entry entry;
> +       int ret;
> +
> +       ret = applesmc_get_entry_by_key(key, &entry);
> +       if (ret)
> +               return ret;
> +
> +       return applesmc_write_entry(&entry, buffer, len);
> +}
> +
>  /*
> - * applesmc_read_motion_sensor - Read motion sensor (X, Y or Z). Callers must
> - * hold applesmc_lock.
> + * applesmc_read_motion_sensor - Read motion sensor (X, Y or Z).
>   */
>  static int applesmc_read_motion_sensor(int index, s16* value)
>  {
> @@ -455,8 +538,6 @@ static int applesmc_device_init(void)
>         if (!applesmc_accelerometer)
>                 return 0;
> 
> -       mutex_lock(&applesmc_lock);
> -
>         for (total = INIT_TIMEOUT_MSECS; total > 0; total -= INIT_WAIT_MSECS) {
>                 if (!applesmc_read_key(MOTION_SENSOR_KEY, buffer, 2) &&
>                                 (buffer[0] != 0x00 || buffer[1] != 0x00)) {
> @@ -472,33 +553,90 @@ static int applesmc_device_init(void)
>         pr_warn("failed to init the device\n");
> 
>  out:
> -       mutex_unlock(&applesmc_lock);
>         return ret;
>  }
> 
>  /*
> - * applesmc_get_fan_count - get the number of fans. Callers must NOT hold
> - * applesmc_lock.
> + * applesmc_get_fan_count - get the number of fans.
>   */
>  static int applesmc_get_fan_count(void)
>  {
>         int ret;
>         u8 buffer[1];
> 
> -       mutex_lock(&applesmc_lock);
> -
>         ret = applesmc_read_key(FANS_COUNT, buffer, 1);
> 
> -       mutex_unlock(&applesmc_lock);
>         if (ret)
>                 return ret;
>         else
>                 return buffer[0];
>  }
> 
> +/*
> + * applesmc_init_smcreg_try - Try to initialize register cache. Idempotent.
> + */
> +static int applesmc_init_smcreg_try(void)
> +{
> +       struct applesmc_registers *s = &smcreg;
> +       int ret;
> +
> +       if (s->init_complete)
> +               return 0;
> +
> +       mutex_init(&s->mutex);
> +
> +       ret = read_register_count(&s->key_count);
> +       if (ret)
> +               return ret;
> +
> +       if (!s->entry)
> +               s->entry = kcalloc(s->key_count, sizeof(*s->entry), GFP_KERNEL);
> +       if (!s->entry)
> +               return -ENOMEM;
> +
> +       s->init_complete = true;
> +
> +       pr_info("key=%d\n", s->key_count);
> +
> +       return 0;
> +}
> +
> +/*
> + * applesmc_init_smcreg - Initialize register cache.
> + *
> + * Retries until initialization is successful, or the operation times out.
> + *
> + */
> +static int applesmc_init_smcreg(void)
> +{
> +       int ms, ret;
> +
> +       for (ms = 0; ms < INIT_TIMEOUT_MSECS; ms += INIT_WAIT_MSECS) {
> +               ret = applesmc_init_smcreg_try();
> +               if (!ret)
> +                       return 0;
> +               pr_warn("slow init, retrying\n");
> +               msleep(INIT_WAIT_MSECS);
> +       }
> +
> +       return ret;
> +}
> +
> +static void applesmc_destroy_smcreg(void)
> +{
> +       kfree(smcreg.entry);
> +       smcreg.entry = NULL;

Do you also have to reset init_complete ?

> +}
> +
>  /* Device model stuff */
>  static int applesmc_probe(struct platform_device *dev)
>  {
> +       int ret;
> +
> +       ret = applesmc_init_smcreg();
> +       if (ret)
> +               return ret;
> +
>         applesmc_device_init();
> 
>         return 0;
> @@ -507,10 +645,8 @@ static int applesmc_probe(struct platform_device *dev)
>  /* Synchronize device with memorized backlight state */
>  static int applesmc_pm_resume(struct device *dev)
>  {
> -       mutex_lock(&applesmc_lock);
>         if (applesmc_light)
>                 applesmc_write_key(BACKLIGHT_KEY, backlight_state, 2);
> -       mutex_unlock(&applesmc_lock);
>         return 0;
>  }
> 
> @@ -551,20 +687,15 @@ static void applesmc_idev_poll(struct input_polled_dev *dev)
>         struct input_dev *idev = dev->input;
>         s16 x, y;
> 
> -       mutex_lock(&applesmc_lock);
> -
>         if (applesmc_read_motion_sensor(SENSOR_X, &x))
> -               goto out;
> +               return;
>         if (applesmc_read_motion_sensor(SENSOR_Y, &y))
> -               goto out;
> +               return;
> 
>         x = -x;
>         input_report_abs(idev, ABS_X, x - rest_x);
>         input_report_abs(idev, ABS_Y, y - rest_y);
>         input_sync(idev);
> -
> -out:
> -       mutex_unlock(&applesmc_lock);
>  }
> 
>  /* Sysfs Files */
> @@ -581,8 +712,6 @@ static ssize_t applesmc_position_show(struct device *dev,
>         int ret;
>         s16 x, y, z;
> 
> -       mutex_lock(&applesmc_lock);
> -
>         ret = applesmc_read_motion_sensor(SENSOR_X, &x);
>         if (ret)
>                 goto out;
> @@ -594,7 +723,6 @@ static ssize_t applesmc_position_show(struct device *dev,
>                 goto out;
> 
>  out:
> -       mutex_unlock(&applesmc_lock);
>         if (ret)
>                 return ret;
>         else
> @@ -604,18 +732,17 @@ out:
>  static ssize_t applesmc_light_show(struct device *dev,
>                                 struct device_attribute *attr, char *sysfsbuf)
>  {
> +       struct applesmc_entry entry;
>         static int data_length;
>         int ret;
>         u8 left = 0, right = 0;
> -       u8 buffer[10], query[6];
> -
> -       mutex_lock(&applesmc_lock);
> +       u8 buffer[10];
> 
>         if (!data_length) {
> -               ret = applesmc_get_key_type(LIGHT_SENSOR_LEFT_KEY, query);
> +               ret = applesmc_get_entry_by_key(LIGHT_SENSOR_LEFT_KEY, &entry);
>                 if (ret)
>                         goto out;
> -               data_length = clamp_val(query[0], 0, 10);
> +               data_length = clamp_val(entry.len, 0, 10);
>                 pr_info("light sensor data length set to %d\n", data_length);
>         }
> 
> @@ -632,7 +759,6 @@ static ssize_t applesmc_light_show(struct device *dev,
>         right = buffer[2];
> 
>  out:
> -       mutex_unlock(&applesmc_lock);
>         if (ret)
>                 return ret;
>         else
> @@ -661,14 +787,10 @@ static ssize_t applesmc_show_temperature(struct device *dev,
>         const char* key =
>                 temperature_sensors_sets[applesmc_temperature_set][attr->index];
> 
> -       mutex_lock(&applesmc_lock);
> -
>         ret = applesmc_read_key(key, buffer, 2);
>         temp = buffer[0]*1000;
>         temp += (buffer[1] >> 6) * 250;
> 
> -       mutex_unlock(&applesmc_lock);
> -
>         if (ret)
>                 return ret;
>         else
> @@ -691,12 +813,9 @@ static ssize_t applesmc_show_fan_speed(struct device *dev,
>         newkey[3] = fan_speed_keys[sensor_attr->nr][3];
>         newkey[4] = 0;
> 
> -       mutex_lock(&applesmc_lock);
> -
>         ret = applesmc_read_key(newkey, buffer, 2);
>         speed = ((buffer[0] << 8 | buffer[1]) >> 2);
> 
> -       mutex_unlock(&applesmc_lock);
>         if (ret)
>                 return ret;
>         else
> @@ -725,13 +844,10 @@ static ssize_t applesmc_store_fan_speed(struct device *dev,
>         newkey[3] = fan_speed_keys[sensor_attr->nr][3];
>         newkey[4] = 0;
> 
> -       mutex_lock(&applesmc_lock);
> -
>         buffer[0] = (speed >> 6) & 0xff;
>         buffer[1] = (speed << 2) & 0xff;
>         ret = applesmc_write_key(newkey, buffer, 2);
> 
> -       mutex_unlock(&applesmc_lock);
>         if (ret)
>                 return ret;
>         else
> @@ -746,12 +862,9 @@ static ssize_t applesmc_show_fan_manual(struct device *dev,
>         u8 buffer[2];
>         struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> 
> -       mutex_lock(&applesmc_lock);
> -
>         ret = applesmc_read_key(FANS_MANUAL, buffer, 2);
>         manual = ((buffer[0] << 8 | buffer[1]) >> attr->index) & 0x01;
> 
> -       mutex_unlock(&applesmc_lock);
>         if (ret)
>                 return ret;
>         else
> @@ -770,8 +883,6 @@ static ssize_t applesmc_store_fan_manual(struct device *dev,
> 
>         input = simple_strtoul(sysfsbuf, NULL, 10);
> 
> -       mutex_lock(&applesmc_lock);
> -
>         ret = applesmc_read_key(FANS_MANUAL, buffer, 2);
>         val = (buffer[0] << 8 | buffer[1]);
>         if (ret)
> @@ -788,7 +899,6 @@ static ssize_t applesmc_store_fan_manual(struct device *dev,
>         ret = applesmc_write_key(FANS_MANUAL, buffer, 2);
> 
>  out:
> -       mutex_unlock(&applesmc_lock);
>         if (ret)
>                 return ret;
>         else
> @@ -810,12 +920,9 @@ static ssize_t applesmc_show_fan_position(struct device *dev,
>         newkey[3] = FAN_POSITION[3];
>         newkey[4] = 0;
> 
> -       mutex_lock(&applesmc_lock);
> -
>         ret = applesmc_read_key(newkey, buffer, 16);
>         buffer[16] = 0;
> 
> -       mutex_unlock(&applesmc_lock);
>         if (ret)
>                 return ret;
>         else
> @@ -831,18 +938,14 @@ static ssize_t applesmc_calibrate_show(struct device *dev,
>  static ssize_t applesmc_calibrate_store(struct device *dev,
>         struct device_attribute *attr, const char *sysfsbuf, size_t count)
>  {
> -       mutex_lock(&applesmc_lock);
>         applesmc_calibrate();
> -       mutex_unlock(&applesmc_lock);
> 
>         return count;
>  }
> 
>  static void applesmc_backlight_set(struct work_struct *work)
>  {
> -       mutex_lock(&applesmc_lock);
>         applesmc_write_key(BACKLIGHT_KEY, backlight_state, 2);
> -       mutex_unlock(&applesmc_lock);
>  }
>  static DECLARE_WORK(backlight_work, &applesmc_backlight_set);
> 
> @@ -865,13 +968,10 @@ static ssize_t applesmc_key_count_show(struct device *dev,
>         u8 buffer[4];
>         u32 count;
> 
> -       mutex_lock(&applesmc_lock);
> -
>         ret = applesmc_read_key(KEY_COUNT_KEY, buffer, 4);
>         count = ((u32)buffer[0]<<24) + ((u32)buffer[1]<<16) +
>                                                 ((u32)buffer[2]<<8) + buffer[3];
> 
> -       mutex_unlock(&applesmc_lock);
>         if (ret)
>                 return ret;
>         else
> @@ -881,113 +981,56 @@ static ssize_t applesmc_key_count_show(struct device *dev,
>  static ssize_t applesmc_key_at_index_read_show(struct device *dev,
>                                 struct device_attribute *attr, char *sysfsbuf)
>  {
> -       char key[5];
> -       char info[6];
> +       struct applesmc_entry entry;
>         int ret;
> 
> -       mutex_lock(&applesmc_lock);
> -
> -       ret = applesmc_get_key_at_index(key_at_index, key);
> -
> -       if (ret || !key[0]) {
> -               mutex_unlock(&applesmc_lock);
> -
> -               return -EINVAL;
> -       }
> -
> -       ret = applesmc_get_key_type(key, info);
> -
> -       if (ret) {
> -               mutex_unlock(&applesmc_lock);
> -
> +       ret = applesmc_get_entry_by_index(key_at_index, &entry);
> +       if (ret)
>                 return ret;
> -       }
> -
> -       /*
> -        * info[0] maximum value (APPLESMC_MAX_DATA_LENGTH) is much lower than
> -        * PAGE_SIZE, so we don't need any checks before writing to sysfsbuf.
> -        */
> -       ret = applesmc_read_key(key, sysfsbuf, info[0]);
> -
> -       mutex_unlock(&applesmc_lock);
> -
> -       if (!ret) {
> -               return info[0];
> -       } else {
> +       ret = applesmc_read_entry(&entry, sysfsbuf, entry.len);
> +       if (ret)
>                 return ret;
> -       }
> +
> +       return entry.len;
>  }
> 
>  static ssize_t applesmc_key_at_index_data_length_show(struct device *dev,
>                                 struct device_attribute *attr, char *sysfsbuf)
>  {
> -       char key[5];
> -       char info[6];
> +       struct applesmc_entry entry;
>         int ret;
> 
> -       mutex_lock(&applesmc_lock);
> -
> -       ret = applesmc_get_key_at_index(key_at_index, key);
> -
> -       if (ret || !key[0]) {
> -               mutex_unlock(&applesmc_lock);
> -
> -               return -EINVAL;
> -       }
> -
> -       ret = applesmc_get_key_type(key, info);
> -
> -       mutex_unlock(&applesmc_lock);
> -
> -       if (!ret)
> -               return snprintf(sysfsbuf, PAGE_SIZE, "%d\n", info[0]);
> -       else
> +       ret = applesmc_get_entry_by_index(key_at_index, &entry);
> +       if (ret)
>                 return ret;
> +
> +       return snprintf(sysfsbuf, PAGE_SIZE, "%d\n", entry.len);
>  }
> 
>  static ssize_t applesmc_key_at_index_type_show(struct device *dev,
>                                 struct device_attribute *attr, char *sysfsbuf)
>  {
> -       char key[5];
> -       char info[6];
> +       struct applesmc_entry entry;
>         int ret;
> 
> -       mutex_lock(&applesmc_lock);
> -
> -       ret = applesmc_get_key_at_index(key_at_index, key);
> -
> -       if (ret || !key[0]) {
> -               mutex_unlock(&applesmc_lock);
> -
> -               return -EINVAL;
> -       }
> -
> -       ret = applesmc_get_key_type(key, info);
> -
> -       mutex_unlock(&applesmc_lock);
> -
> -       if (!ret)
> -               return snprintf(sysfsbuf, PAGE_SIZE, "%s\n", info+1);
> -       else
> +       ret = applesmc_get_entry_by_index(key_at_index, &entry);
> +       if (ret)
>                 return ret;
> +
> +       return snprintf(sysfsbuf, PAGE_SIZE, "%s\n", entry.type);
>  }
> 
>  static ssize_t applesmc_key_at_index_name_show(struct device *dev,
>                                 struct device_attribute *attr, char *sysfsbuf)
>  {
> -       char key[5];
> +       struct applesmc_entry entry;
>         int ret;
> 
> -       mutex_lock(&applesmc_lock);
> -
> -       ret = applesmc_get_key_at_index(key_at_index, key);
> -
> -       mutex_unlock(&applesmc_lock);
> +       ret = applesmc_get_entry_by_index(key_at_index, &entry);
> +       if (ret)
> +               return ret;
> 
> -       if (!ret && key[0])
> -               return snprintf(sysfsbuf, PAGE_SIZE, "%s\n", key);
> -       else
> -               return -EINVAL;
> +       return snprintf(sysfsbuf, PAGE_SIZE, "%s\n", entry.key);
>  }
> 
>  static ssize_t applesmc_key_at_index_show(struct device *dev,
> @@ -999,12 +1042,8 @@ static ssize_t applesmc_key_at_index_show(struct device *dev,
>  static ssize_t applesmc_key_at_index_store(struct device *dev,
>         struct device_attribute *attr, const char *sysfsbuf, size_t count)
>  {
> -       mutex_lock(&applesmc_lock);
> -
>         key_at_index = simple_strtoul(sysfsbuf, NULL, 10);
> 
> -       mutex_unlock(&applesmc_lock);
> -
>         return count;
>  }
> 
> @@ -1640,10 +1679,15 @@ static int __init applesmc_init(void)
>                 goto out_driver;
>         }
> 
> -       ret = sysfs_create_file(&pdev->dev.kobj, &dev_attr_name.attr);
> +       /* create register cache */
> +       ret = applesmc_init_smcreg();
>         if (ret)
>                 goto out_device;
> 
> +       ret = sysfs_create_file(&pdev->dev.kobj, &dev_attr_name.attr);
> +       if (ret)
> +               goto out_smcreg;
> +
>         /* Create key enumeration sysfs files */
>         ret = sysfs_create_group(&pdev->dev.kobj, &key_enumeration_group);
>         if (ret)
> @@ -1745,6 +1789,8 @@ out_fans:
>         sysfs_remove_group(&pdev->dev.kobj, &key_enumeration_group);
>  out_name:
>         sysfs_remove_file(&pdev->dev.kobj, &dev_attr_name.attr);
> +out_smcreg:
> +       applesmc_destroy_smcreg();
>  out_device:
>         platform_device_unregister(pdev);
>  out_driver:
> @@ -1773,6 +1819,7 @@ static void __exit applesmc_exit(void)
>                                    &fan_attribute_groups[--fans_handled]);
>         sysfs_remove_group(&pdev->dev.kobj, &key_enumeration_group);
>         sysfs_remove_file(&pdev->dev.kobj, &dev_attr_name.attr);
> +       applesmc_destroy_smcreg();
>         platform_device_unregister(pdev);
>         platform_driver_unregister(&applesmc_driver);
>         release_region(APPLESMC_DATA_PORT, APPLESMC_NR_PORTS);
> --
> 1.7.1
> 
> 
> _______________________________________________
> lm-sensors mailing list
> lm-sensors@lm-sensors.org
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 2/8] hwmon: applesmc: Introduce a register lookup table

[Henrik Rydberg]

Hi Guenter,

>> @@ -217,14 +235,13 @@ static unsigned int fans_handled;

>>  /* Indicates which temperature sensors set to use. */
>>  static unsigned int applesmc_temperature_set;
>>
>> -static DEFINE_MUTEX(applesmc_lock);
>> -
>>  /*
>>   * Last index written to key_at_index sysfs file, and value to use for all other
>>   * key_at_index_* sysfs files.
>>   */
>>  static unsigned int key_at_index;
>>
>> +
> 
> unnecessary blank line


Ironically, I even tried to get rid out it two times. ;-)

>> +static int applesmc_get_entry_by_index(int index, struct applesmc_entry *entry)
>> +{
> 
> One thing I don't understand about the whole caching scheme - why don't you just
> return (and use) a pointer to the cached entry ? Seems to me that would be much simpler.
> If you want to return error types, you could use ERR_PTR, PTR_ERR, and IS_ERR.


Yes, the return types are important. I can change this, it will reduce the patch
by some lines.

>> +       struct applesmc_entry *cache = &smcreg.entry[index];
>> +       __be32 be;
>> +       int ret;
>> +
>> +       if (cache->valid) {
>> +               memcpy(entry, cache, sizeof(*entry));
>> +               return 0;
>>         }
>> -       key[4] = 0;
>>
>> -       return 0;
>> +       mutex_lock(&smcreg.mutex);


Note to self: it is possible to arrive here with a valid cache. If pointers are
used, exit to avoid incoherent reads.

>> +
>> +       be = cpu_to_be32(index);
>> +       ret = read_smc(APPLESMC_GET_KEY_BY_INDEX_CMD, (u8 *)&be, cache->key, 4);
>> +       if (ret)
>> +               goto out;
>> +       ret = read_smc(APPLESMC_GET_KEY_TYPE_CMD, cache->key, &cache->len, 6);
> 
> This one is a bit odd. cache->len is an u8. You are reading 6 bytes into it.
> I assume this is supposed to fill both cache->len and cache->type in a single operation.


True.


> Not sure if this is a good idea. Seems to depend a lot on the assumption that
> fields are consecutive. Might be safer to read the data into a temporary
> 6 byte buffer and copy it into ->len and ->type afterwards.


Right, there could be alignment problems here. Thanks.

> If that is not acceptable, please add a comment describing what you are doing here
> and why.


It was just about the extra stack space. There will be a natural place for the
buffer once the pointer are used.

>> +       if (ret)
>> +               goto out;
>> +
>> +       cache->type[4] = 0;
> 
> Why read 6 bytes above if you overwrite the last byte anyway ?


Backwards compatibility reasons. The register type names have been used as four
byte strings since the beginning. I do not know is the last value codes
something else - I can make that clearer by reserving a byte in the struct instead.

> 
>> +       cache->valid = 1;


Continued note to self: a switch here is not handled properly.

>> +       memcpy(entry, cache, sizeof(*entry));
>> +
>> +out:
>> +       mutex_unlock(&smcreg.mutex);
>> +       return ret;
>>  }



>> +/*
>> + * applesmc_init_smcreg_try - Try to initialize register cache. Idempotent.
>> + */
>> +static int applesmc_init_smcreg_try(void)
>> +{
>> +       struct applesmc_registers *s = &smcreg;
>> +       int ret;
>> +
>> +       if (s->init_complete)
>> +               return 0;
>> +
>> +       mutex_init(&s->mutex);
>> +
>> +       ret = read_register_count(&s->key_count);
>> +       if (ret)
>> +               return ret;
>> +
>> +       if (!s->entry)
>> +               s->entry = kcalloc(s->key_count, sizeof(*s->entry), GFP_KERNEL);
>> +       if (!s->entry)
>> +               return -ENOMEM;
>> +
>> +       s->init_complete = true;
>> +
>> +       pr_info("key=%d\n", s->key_count);
>> +
>> +       return 0;
>> +}
>> +
>> +/*
>> + * applesmc_init_smcreg - Initialize register cache.
>> + *
>> + * Retries until initialization is successful, or the operation times out.
>> + *
>> + */
>> +static int applesmc_init_smcreg(void)
>> +{
>> +       int ms, ret;
>> +
>> +       for (ms = 0; ms < INIT_TIMEOUT_MSECS; ms += INIT_WAIT_MSECS) {
>> +               ret = applesmc_init_smcreg_try();
>> +               if (!ret)
>> +                       return 0;
>> +               pr_warn("slow init, retrying\n");
>> +               msleep(INIT_WAIT_MSECS);
>> +       }
>> +
>> +       return ret;
>> +}
>> +
>> +static void applesmc_destroy_smcreg(void)
>> +{
>> +       kfree(smcreg.entry);
>> +       smcreg.entry = NULL;
> 
> Do you also have to reset init_complete ?


Yes!

Thanks,
Henrik

[PATCH 3/8] hwmon: applesmc: Dynamic creation of temperature files

[Henrik Rydberg]

The current driver creates temperature files based on a list
of temperature keys given per device. Apart from slow adaption
to new machine models, the number of sensors also depends on
the number of processors. This patch looks up the temperature
keys dynamically, thereby supporting all models.

Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
---
 drivers/hwmon/applesmc.c |  498 +++++++++++-----------------------------------
 1 files changed, 113 insertions(+), 385 deletions(-)

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 7f030f0..0207618 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -84,90 +84,6 @@
 #define FAN_TARGET_SPEED	"F0Tg" /* r-w fpe2 (2 bytes) */
 #define FAN_POSITION		"F0ID" /* r-o char[16] */
 
-/*
- * Temperature sensors keys (sp78 - 2 bytes).
- */
-static const char *temperature_sensors_sets[][41] = {
-/* Set 0: Macbook Pro */
-	{ "TA0P", "TB0T", "TC0D", "TC0P", "TG0H", "TG0P", "TG0T", "Th0H",
-	  "Th1H", "Tm0P", "Ts0P", "Ts1P", NULL },
-/* Set 1: Macbook2 set */
-	{ "TB0T", "TC0D", "TC0P", "TM0P", "TN0P", "TN1P", "TTF0", "Th0H",
-	  "Th0S", "Th1H", NULL },
-/* Set 2: Macbook set */
-	{ "TB0T", "TC0D", "TC0P", "TM0P", "TN0P", "TN1P", "Th0H", "Th0S",
-	  "Th1H", "Ts0P", NULL },
-/* Set 3: Macmini set */
-	{ "TC0D", "TC0P", NULL },
-/* Set 4: Mac Pro (2 x Quad-Core) */
-	{ "TA0P", "TCAG", "TCAH", "TCBG", "TCBH", "TC0C", "TC0D", "TC0P",
-	  "TC1C", "TC1D", "TC2C", "TC2D", "TC3C", "TC3D", "THTG", "TH0P",
-	  "TH1P", "TH2P", "TH3P", "TMAP", "TMAS", "TMBS", "TM0P", "TM0S",
-	  "TM1P", "TM1S", "TM2P", "TM2S", "TM3S", "TM8P", "TM8S", "TM9P",
-	  "TM9S", "TN0H", "TS0C", NULL },
-/* Set 5: iMac */
-	{ "TC0D", "TA0P", "TG0P", "TG0D", "TG0H", "TH0P", "Tm0P", "TO0P",
-	  "Tp0C", NULL },
-/* Set 6: Macbook3 set */
-	{ "TB0T", "TC0D", "TC0P", "TM0P", "TN0P", "TTF0", "TW0P", "Th0H",
-	  "Th0S", "Th1H", NULL },
-/* Set 7: Macbook Air */
-	{ "TB0T", "TB1S", "TB1T", "TB2S", "TB2T", "TC0D", "TC0P", "TCFP",
-	  "TTF0", "TW0P", "Th0H", "Tp0P", "TpFP", "Ts0P", "Ts0S", NULL },
-/* Set 8: Macbook Pro 4,1 (Penryn) */
-	{ "TB0T", "TC0D", "TC0P", "TG0D", "TG0H", "TTF0", "TW0P", "Th0H",
-	  "Th1H", "Th2H", "Tm0P", "Ts0P", NULL },
-/* Set 9: Macbook Pro 3,1 (Santa Rosa) */
-	{ "TALP", "TB0T", "TC0D", "TC0P", "TG0D", "TG0H", "TTF0", "TW0P",
-	  "Th0H", "Th1H", "Th2H", "Tm0P", "Ts0P", NULL },
-/* Set 10: iMac 5,1 */
-	{ "TA0P", "TC0D", "TC0P", "TG0D", "TH0P", "TO0P", "Tm0P", NULL },
-/* Set 11: Macbook 5,1 */
-	{ "TB0T", "TB1T", "TB2T", "TB3T", "TC0D", "TC0P", "TN0D", "TN0P",
-	  "TTF0", "Th0H", "Th1H", "ThFH", "Ts0P", "Ts0S", NULL },
-/* Set 12: Macbook Pro 5,1 */
-	{ "TB0T", "TB1T", "TB2T", "TB3T", "TC0D", "TC0F", "TC0P", "TG0D",
-	  "TG0F", "TG0H", "TG0P", "TG0T", "TG1H", "TN0D", "TN0P", "TTF0",
-	  "Th2H", "Tm0P", "Ts0P", "Ts0S", NULL },
-/* Set 13: iMac 8,1 */
-	{ "TA0P", "TC0D", "TC0H", "TC0P", "TG0D", "TG0H", "TG0P", "TH0P",
-	  "TL0P", "TO0P", "TW0P", "Tm0P", "Tp0P", NULL },
-/* Set 14: iMac 6,1 */
-	{ "TA0P", "TC0D", "TC0H", "TC0P", "TG0D", "TG0H", "TG0P", "TH0P",
-	  "TO0P", "Tp0P", NULL },
-/* Set 15: MacBook Air 2,1 */
-	{ "TB0T", "TB1S", "TB1T", "TB2S", "TB2T", "TC0D", "TN0D", "TTF0",
-	  "TV0P", "TVFP", "TW0P", "Th0P", "Tp0P", "Tp1P", "TpFP", "Ts0P",
-	  "Ts0S", NULL },
-/* Set 16: Mac Pro 3,1 (2 x Quad-Core) */
-	{ "TA0P", "TCAG", "TCAH", "TCBG", "TCBH", "TC0C", "TC0D", "TC0P",
-	  "TC1C", "TC1D", "TC2C", "TC2D", "TC3C", "TC3D", "TH0P", "TH1P",
-	  "TH2P", "TH3P", "TMAP", "TMAS", "TMBS", "TM0P", "TM0S", "TM1P",
-	  "TM1S", "TM2P", "TM2S", "TM3S", "TM8P", "TM8S", "TM9P", "TM9S",
-	  "TN0C", "TN0D", "TN0H", "TS0C", "Tp0C", "Tp1C", "Tv0S", "Tv1S",
-	  NULL },
-/* Set 17: iMac 9,1 */
-	{ "TA0P", "TC0D", "TC0H", "TC0P", "TG0D", "TG0H", "TH0P", "TL0P",
-	  "TN0D", "TN0H", "TN0P", "TO0P", "Tm0P", "Tp0P", NULL },
-/* Set 18: MacBook Pro 2,2 */
-	{ "TB0T", "TC0D", "TC0P", "TG0H", "TG0P", "TG0T", "TM0P", "TTF0",
-	  "Th0H", "Th1H", "Tm0P", "Ts0P", NULL },
-/* Set 19: Macbook Pro 5,3 */
-	{ "TB0T", "TB1T", "TB2T", "TB3T", "TC0D", "TC0F", "TC0P", "TG0D",
-	  "TG0F", "TG0H", "TG0P", "TG0T", "TN0D", "TN0P", "TTF0", "Th2H",
-	  "Tm0P", "Ts0P", "Ts0S", NULL },
-/* Set 20: MacBook Pro 5,4 */
-	{ "TB0T", "TB1T", "TB2T", "TB3T", "TC0D", "TC0F", "TC0P", "TN0D",
-	  "TN0P", "TTF0", "Th2H", "Ts0P", "Ts0S", NULL },
-/* Set 21: MacBook Pro 6,2 */
-	{ "TB0T", "TB1T", "TB2T", "TC0C", "TC0D", "TC0P", "TC1C", "TG0D",
-	  "TG0P", "TG0T", "TMCD", "TP0P", "TPCD", "Th1H", "Th2H", "Tm0P",
-	  "Ts0P", "Ts0S", NULL },
-/* Set 22: MacBook Pro 7,1 */
-	{ "TB0T", "TB1T", "TB2T", "TC0D", "TC0P", "TN0D", "TN0P", "TN0S",
-	  "TN1D", "TN1F", "TN1G", "TN1S", "Th1H", "Ts0P", "Ts0S", NULL },
-};
-
 /* List of keys used to read/write fan speeds */
 static const char* fan_speed_keys[] = {
 	FAN_ACTUAL_SPEED,
@@ -188,6 +104,8 @@ static const char* fan_speed_keys[] = {
 #define SENSOR_Y 1
 #define SENSOR_Z 2
 
+#define to_index(attr) (to_sensor_dev_attr(attr)->index)
+
 /* Structure to be passed to DMI_MATCH function */
 struct dmi_match_data {
 /* Indicates whether this computer has an accelerometer. */
@@ -198,6 +116,20 @@ struct dmi_match_data {
 	int temperature_set;
 };
 
+/* Dynamic device node attributes */
+struct applesmc_dev_attr {
+	struct sensor_device_attribute sda;	/* hwmon attributes */
+	char name[32];				/* room for node file name */
+};
+
+/* Dynamic device node group */
+struct applesmc_node_group {
+	char *format;				/* format string */
+	void *show;				/* show function */
+	void *store;				/* store function */
+	struct applesmc_dev_attr *nodes;	/* dynamic node array */
+};
+
 /* AppleSMC entry - cached register information */
 struct applesmc_entry {
 	char key[5];		/* four-letter key code */
@@ -210,6 +142,9 @@ struct applesmc_entry {
 static struct applesmc_registers {
 	struct mutex mutex;		/* register read/write mutex */
 	unsigned int key_count;		/* number of SMC registers */
+	unsigned int temp_count;	/* number of temperature registers */
+	unsigned int temp_begin;	/* temperature lower index bound */
+	unsigned int temp_end;		/* temperature upper index bound */
 	bool init_complete;		/* true when fully initialized */
 	struct applesmc_entry *entry;	/* key entries */
 } smcreg;
@@ -232,9 +167,6 @@ static unsigned int applesmc_light;
 /* The number of fans handled by the driver */
 static unsigned int fans_handled;
 
-/* Indicates which temperature sensors set to use. */
-static unsigned int applesmc_temperature_set;
-
 /*
  * Last index written to key_at_index sysfs file, and value to use for all other
  * key_at_index_* sysfs files.
@@ -594,9 +526,17 @@ static int applesmc_init_smcreg_try(void)
 	if (!s->entry)
 		return -ENOMEM;
 
+	ret = applesmc_get_lower_bound(&s->temp_begin, "T");
+	if (ret)
+		return ret;
+	ret = applesmc_get_lower_bound(&s->temp_end, "U");
+	if (ret)
+		return ret;
+	s->temp_count = s->temp_end - s->temp_begin;
+
 	s->init_complete = true;
 
-	pr_info("key=%d\n", s->key_count);
+	pr_info("key=%d temp=%d\n", s->key_count, s->temp_count);
 
 	return 0;
 }
@@ -769,32 +709,39 @@ out:
 static ssize_t applesmc_show_sensor_label(struct device *dev,
 			struct device_attribute *devattr, char *sysfsbuf)
 {
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	const char *key =
-		temperature_sensors_sets[applesmc_temperature_set][attr->index];
+	int index = smcreg.temp_begin + to_index(devattr);
+	struct applesmc_entry entry;
+	int ret;
 
-	return snprintf(sysfsbuf, PAGE_SIZE, "%s\n", key);
+	ret = applesmc_get_entry_by_index(index, &entry);
+	if (ret)
+		return ret;
+
+	return snprintf(sysfsbuf, PAGE_SIZE, "%s\n", entry.key);
 }
 
 /* Displays degree Celsius * 1000 */
 static ssize_t applesmc_show_temperature(struct device *dev,
 			struct device_attribute *devattr, char *sysfsbuf)
 {
+	int index = smcreg.temp_begin + to_index(devattr);
+	struct applesmc_entry entry;
 	int ret;
 	u8 buffer[2];
 	unsigned int temp;
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	const char* key =
-		temperature_sensors_sets[applesmc_temperature_set][attr->index];
 
-	ret = applesmc_read_key(key, buffer, 2);
-	temp = buffer[0]*1000;
-	temp += (buffer[1] >> 6) * 250;
+	ret = applesmc_get_entry_by_index(index, &entry);
+	if (ret)
+		return ret;
 
+	ret = applesmc_read_entry(&entry, buffer, 2);
 	if (ret)
 		return ret;
-	else
-		return snprintf(sysfsbuf, PAGE_SIZE, "%u\n", temp);
+
+	temp = buffer[0]*1000;
+	temp += (buffer[1] >> 6) * 250;
+
+	return snprintf(sysfsbuf, PAGE_SIZE, "%u\n", temp);
 }
 
 static ssize_t applesmc_show_fan_speed(struct device *dev,
@@ -1153,263 +1100,10 @@ static const struct attribute_group fan_attribute_groups[] = {
 	{ .attrs = fan4_attributes },
 };
 
-/*
- * Temperature sensors sysfs entries.
- */
-static SENSOR_DEVICE_ATTR(temp1_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 0);
-static SENSOR_DEVICE_ATTR(temp2_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 1);
-static SENSOR_DEVICE_ATTR(temp3_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 2);
-static SENSOR_DEVICE_ATTR(temp4_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 3);
-static SENSOR_DEVICE_ATTR(temp5_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 4);
-static SENSOR_DEVICE_ATTR(temp6_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 5);
-static SENSOR_DEVICE_ATTR(temp7_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 6);
-static SENSOR_DEVICE_ATTR(temp8_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 7);
-static SENSOR_DEVICE_ATTR(temp9_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 8);
-static SENSOR_DEVICE_ATTR(temp10_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 9);
-static SENSOR_DEVICE_ATTR(temp11_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 10);
-static SENSOR_DEVICE_ATTR(temp12_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 11);
-static SENSOR_DEVICE_ATTR(temp13_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 12);
-static SENSOR_DEVICE_ATTR(temp14_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 13);
-static SENSOR_DEVICE_ATTR(temp15_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 14);
-static SENSOR_DEVICE_ATTR(temp16_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 15);
-static SENSOR_DEVICE_ATTR(temp17_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 16);
-static SENSOR_DEVICE_ATTR(temp18_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 17);
-static SENSOR_DEVICE_ATTR(temp19_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 18);
-static SENSOR_DEVICE_ATTR(temp20_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 19);
-static SENSOR_DEVICE_ATTR(temp21_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 20);
-static SENSOR_DEVICE_ATTR(temp22_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 21);
-static SENSOR_DEVICE_ATTR(temp23_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 22);
-static SENSOR_DEVICE_ATTR(temp24_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 23);
-static SENSOR_DEVICE_ATTR(temp25_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 24);
-static SENSOR_DEVICE_ATTR(temp26_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 25);
-static SENSOR_DEVICE_ATTR(temp27_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 26);
-static SENSOR_DEVICE_ATTR(temp28_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 27);
-static SENSOR_DEVICE_ATTR(temp29_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 28);
-static SENSOR_DEVICE_ATTR(temp30_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 29);
-static SENSOR_DEVICE_ATTR(temp31_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 30);
-static SENSOR_DEVICE_ATTR(temp32_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 31);
-static SENSOR_DEVICE_ATTR(temp33_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 32);
-static SENSOR_DEVICE_ATTR(temp34_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 33);
-static SENSOR_DEVICE_ATTR(temp35_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 34);
-static SENSOR_DEVICE_ATTR(temp36_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 35);
-static SENSOR_DEVICE_ATTR(temp37_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 36);
-static SENSOR_DEVICE_ATTR(temp38_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 37);
-static SENSOR_DEVICE_ATTR(temp39_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 38);
-static SENSOR_DEVICE_ATTR(temp40_label, S_IRUGO,
-					applesmc_show_sensor_label, NULL, 39);
-static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 0);
-static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 1);
-static SENSOR_DEVICE_ATTR(temp3_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 2);
-static SENSOR_DEVICE_ATTR(temp4_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 3);
-static SENSOR_DEVICE_ATTR(temp5_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 4);
-static SENSOR_DEVICE_ATTR(temp6_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 5);
-static SENSOR_DEVICE_ATTR(temp7_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 6);
-static SENSOR_DEVICE_ATTR(temp8_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 7);
-static SENSOR_DEVICE_ATTR(temp9_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 8);
-static SENSOR_DEVICE_ATTR(temp10_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 9);
-static SENSOR_DEVICE_ATTR(temp11_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 10);
-static SENSOR_DEVICE_ATTR(temp12_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 11);
-static SENSOR_DEVICE_ATTR(temp13_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 12);
-static SENSOR_DEVICE_ATTR(temp14_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 13);
-static SENSOR_DEVICE_ATTR(temp15_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 14);
-static SENSOR_DEVICE_ATTR(temp16_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 15);
-static SENSOR_DEVICE_ATTR(temp17_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 16);
-static SENSOR_DEVICE_ATTR(temp18_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 17);
-static SENSOR_DEVICE_ATTR(temp19_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 18);
-static SENSOR_DEVICE_ATTR(temp20_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 19);
-static SENSOR_DEVICE_ATTR(temp21_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 20);
-static SENSOR_DEVICE_ATTR(temp22_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 21);
-static SENSOR_DEVICE_ATTR(temp23_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 22);
-static SENSOR_DEVICE_ATTR(temp24_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 23);
-static SENSOR_DEVICE_ATTR(temp25_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 24);
-static SENSOR_DEVICE_ATTR(temp26_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 25);
-static SENSOR_DEVICE_ATTR(temp27_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 26);
-static SENSOR_DEVICE_ATTR(temp28_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 27);
-static SENSOR_DEVICE_ATTR(temp29_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 28);
-static SENSOR_DEVICE_ATTR(temp30_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 29);
-static SENSOR_DEVICE_ATTR(temp31_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 30);
-static SENSOR_DEVICE_ATTR(temp32_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 31);
-static SENSOR_DEVICE_ATTR(temp33_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 32);
-static SENSOR_DEVICE_ATTR(temp34_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 33);
-static SENSOR_DEVICE_ATTR(temp35_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 34);
-static SENSOR_DEVICE_ATTR(temp36_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 35);
-static SENSOR_DEVICE_ATTR(temp37_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 36);
-static SENSOR_DEVICE_ATTR(temp38_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 37);
-static SENSOR_DEVICE_ATTR(temp39_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 38);
-static SENSOR_DEVICE_ATTR(temp40_input, S_IRUGO,
-					applesmc_show_temperature, NULL, 39);
-
-static struct attribute *label_attributes[] = {
-	&sensor_dev_attr_temp1_label.dev_attr.attr,
-	&sensor_dev_attr_temp2_label.dev_attr.attr,
-	&sensor_dev_attr_temp3_label.dev_attr.attr,
-	&sensor_dev_attr_temp4_label.dev_attr.attr,
-	&sensor_dev_attr_temp5_label.dev_attr.attr,
-	&sensor_dev_attr_temp6_label.dev_attr.attr,
-	&sensor_dev_attr_temp7_label.dev_attr.attr,
-	&sensor_dev_attr_temp8_label.dev_attr.attr,
-	&sensor_dev_attr_temp9_label.dev_attr.attr,
-	&sensor_dev_attr_temp10_label.dev_attr.attr,
-	&sensor_dev_attr_temp11_label.dev_attr.attr,
-	&sensor_dev_attr_temp12_label.dev_attr.attr,
-	&sensor_dev_attr_temp13_label.dev_attr.attr,
-	&sensor_dev_attr_temp14_label.dev_attr.attr,
-	&sensor_dev_attr_temp15_label.dev_attr.attr,
-	&sensor_dev_attr_temp16_label.dev_attr.attr,
-	&sensor_dev_attr_temp17_label.dev_attr.attr,
-	&sensor_dev_attr_temp18_label.dev_attr.attr,
-	&sensor_dev_attr_temp19_label.dev_attr.attr,
-	&sensor_dev_attr_temp20_label.dev_attr.attr,
-	&sensor_dev_attr_temp21_label.dev_attr.attr,
-	&sensor_dev_attr_temp22_label.dev_attr.attr,
-	&sensor_dev_attr_temp23_label.dev_attr.attr,
-	&sensor_dev_attr_temp24_label.dev_attr.attr,
-	&sensor_dev_attr_temp25_label.dev_attr.attr,
-	&sensor_dev_attr_temp26_label.dev_attr.attr,
-	&sensor_dev_attr_temp27_label.dev_attr.attr,
-	&sensor_dev_attr_temp28_label.dev_attr.attr,
-	&sensor_dev_attr_temp29_label.dev_attr.attr,
-	&sensor_dev_attr_temp30_label.dev_attr.attr,
-	&sensor_dev_attr_temp31_label.dev_attr.attr,
-	&sensor_dev_attr_temp32_label.dev_attr.attr,
-	&sensor_dev_attr_temp33_label.dev_attr.attr,
-	&sensor_dev_attr_temp34_label.dev_attr.attr,
-	&sensor_dev_attr_temp35_label.dev_attr.attr,
-	&sensor_dev_attr_temp36_label.dev_attr.attr,
-	&sensor_dev_attr_temp37_label.dev_attr.attr,
-	&sensor_dev_attr_temp38_label.dev_attr.attr,
-	&sensor_dev_attr_temp39_label.dev_attr.attr,
-	&sensor_dev_attr_temp40_label.dev_attr.attr,
-	NULL
-};
-
-static struct attribute *temperature_attributes[] = {
-	&sensor_dev_attr_temp1_input.dev_attr.attr,
-	&sensor_dev_attr_temp2_input.dev_attr.attr,
-	&sensor_dev_attr_temp3_input.dev_attr.attr,
-	&sensor_dev_attr_temp4_input.dev_attr.attr,
-	&sensor_dev_attr_temp5_input.dev_attr.attr,
-	&sensor_dev_attr_temp6_input.dev_attr.attr,
-	&sensor_dev_attr_temp7_input.dev_attr.attr,
-	&sensor_dev_attr_temp8_input.dev_attr.attr,
-	&sensor_dev_attr_temp9_input.dev_attr.attr,
-	&sensor_dev_attr_temp10_input.dev_attr.attr,
-	&sensor_dev_attr_temp11_input.dev_attr.attr,
-	&sensor_dev_attr_temp12_input.dev_attr.attr,
-	&sensor_dev_attr_temp13_input.dev_attr.attr,
-	&sensor_dev_attr_temp14_input.dev_attr.attr,
-	&sensor_dev_attr_temp15_input.dev_attr.attr,
-	&sensor_dev_attr_temp16_input.dev_attr.attr,
-	&sensor_dev_attr_temp17_input.dev_attr.attr,
-	&sensor_dev_attr_temp18_input.dev_attr.attr,
-	&sensor_dev_attr_temp19_input.dev_attr.attr,
-	&sensor_dev_attr_temp20_input.dev_attr.attr,
-	&sensor_dev_attr_temp21_input.dev_attr.attr,
-	&sensor_dev_attr_temp22_input.dev_attr.attr,
-	&sensor_dev_attr_temp23_input.dev_attr.attr,
-	&sensor_dev_attr_temp24_input.dev_attr.attr,
-	&sensor_dev_attr_temp25_input.dev_attr.attr,
-	&sensor_dev_attr_temp26_input.dev_attr.attr,
-	&sensor_dev_attr_temp27_input.dev_attr.attr,
-	&sensor_dev_attr_temp28_input.dev_attr.attr,
-	&sensor_dev_attr_temp29_input.dev_attr.attr,
-	&sensor_dev_attr_temp30_input.dev_attr.attr,
-	&sensor_dev_attr_temp31_input.dev_attr.attr,
-	&sensor_dev_attr_temp32_input.dev_attr.attr,
-	&sensor_dev_attr_temp33_input.dev_attr.attr,
-	&sensor_dev_attr_temp34_input.dev_attr.attr,
-	&sensor_dev_attr_temp35_input.dev_attr.attr,
-	&sensor_dev_attr_temp36_input.dev_attr.attr,
-	&sensor_dev_attr_temp37_input.dev_attr.attr,
-	&sensor_dev_attr_temp38_input.dev_attr.attr,
-	&sensor_dev_attr_temp39_input.dev_attr.attr,
-	&sensor_dev_attr_temp40_input.dev_attr.attr,
-	NULL
-};
-
-static const struct attribute_group temperature_attributes_group =
-	{ .attrs = temperature_attributes };
-
-static const struct attribute_group label_attributes_group = {
-	.attrs = label_attributes
+static struct applesmc_node_group temp_group[] = {
+	{ "temp%d_label", applesmc_show_sensor_label },
+	{ "temp%d_input", applesmc_show_temperature },
+	{ }
 };
 
 /* Module stuff */
@@ -1419,7 +1113,6 @@ static const struct attribute_group label_attributes_group = {
  */
 static int applesmc_dmi_match(const struct dmi_system_id *id)
 {
-	int i = 0;
 	struct dmi_match_data* dmi_data = id->driver_data;
 	pr_info("%s detected:\n", id->ident);
 	applesmc_accelerometer = dmi_data->accelerometer;
@@ -1429,13 +1122,66 @@ static int applesmc_dmi_match(const struct dmi_system_id *id)
 	pr_info(" - Model %s light sensors and backlight\n",
 		applesmc_light ? "with" : "without");
 
-	applesmc_temperature_set =  dmi_data->temperature_set;
-	while (temperature_sensors_sets[applesmc_temperature_set][i] != NULL)
-		i++;
-	pr_info(" - Model with %d temperature sensors\n", i);
+
 	return 1;
 }
 
+/*
+ * applesmc_destroy_nodes - remove files and free associated memory
+ */
+static void applesmc_destroy_nodes(struct applesmc_node_group *groups)
+{
+	struct applesmc_node_group *grp;
+	struct applesmc_dev_attr *node;
+
+	for (grp = groups; grp->nodes; grp++) {
+		for (node = grp->nodes; node->sda.dev_attr.attr.name; node++)
+			sysfs_remove_file(&pdev->dev.kobj,
+					  &node->sda.dev_attr.attr);
+		kfree(grp->nodes);
+		grp->nodes = NULL;
+	}
+}
+
+/*
+ * applesmc_create_nodes - create a two-dimensional group of sysfs files
+ */
+static int applesmc_create_nodes(struct applesmc_node_group *groups, int num)
+{
+	struct applesmc_node_group *grp;
+	struct applesmc_dev_attr *node;
+	struct attribute *attr;
+	int ret, i;
+
+	for (grp = groups; grp->format; grp++) {
+		grp->nodes = kcalloc(num + 1, sizeof(*node), GFP_KERNEL);
+		if (!grp->nodes) {
+			ret = -ENOMEM;
+			goto out;
+		}
+		for (i = 0; i < num; i++) {
+			node = &grp->nodes[i];
+			sprintf(node->name, grp->format, i + 1);
+			node->sda.index = i;
+			node->sda.dev_attr.show = grp->show;
+			node->sda.dev_attr.store = grp->store;
+			attr = &node->sda.dev_attr.attr;
+			attr->name = node->name;
+			attr->mode = S_IRUGO | (grp->store ? S_IWUSR : 0);
+			ret = sysfs_create_file(&pdev->dev.kobj, attr);
+			if (ret) {
+				attr->name = NULL;
+				goto out;
+			}
+		}
+	}
+
+	return 0;
+out:
+	applesmc_destroy_nodes(groups);
+	return ret;
+}
+
 /* Create accelerometer ressources */
 static int applesmc_create_accelerometer(void)
 {
@@ -1654,7 +1400,6 @@ static int __init applesmc_init(void)
 {
 	int ret;
 	int count;
-	int i;
 
 	if (!dmi_check_system(applesmc_whitelist)) {
 		pr_warn("supported laptop not found!\n");
@@ -1713,24 +1458,9 @@ static int __init applesmc_init(void)
 		fans_handled++;
 	}
 
-	for (i = 0;
-	     temperature_sensors_sets[applesmc_temperature_set][i] != NULL;
-	     i++) {
-		if (temperature_attributes[i] == NULL ||
-		    label_attributes[i] == NULL) {
-			pr_err("More temperature sensors in temperature_sensors_sets (at least %i) than available sysfs files in temperature_attributes (%i), please report this bug\n",
-			       i, i-1);
-			goto out_temperature;
-		}
-		ret = sysfs_create_file(&pdev->dev.kobj,
-						temperature_attributes[i]);
-		if (ret)
-			goto out_temperature;
-		ret = sysfs_create_file(&pdev->dev.kobj,
-						label_attributes[i]);
-		if (ret)
-			goto out_temperature;
-	}
+	ret = applesmc_create_nodes(temp_group, smcreg.temp_count);
+	if (ret)
+		goto out_fans;
 
 	if (applesmc_accelerometer) {
 		ret = applesmc_create_accelerometer();
@@ -1780,8 +1510,7 @@ out_accelerometer:
 	if (applesmc_accelerometer)
 		applesmc_release_accelerometer();
 out_temperature:
-	sysfs_remove_group(&pdev->dev.kobj, &label_attributes_group);
-	sysfs_remove_group(&pdev->dev.kobj, &temperature_attributes_group);
+	applesmc_destroy_nodes(temp_group);
 out_fans:
 	while (fans_handled)
 		sysfs_remove_group(&pdev->dev.kobj,
@@ -1812,8 +1541,7 @@ static void __exit applesmc_exit(void)
 	}
 	if (applesmc_accelerometer)
 		applesmc_release_accelerometer();
-	sysfs_remove_group(&pdev->dev.kobj, &label_attributes_group);
-	sysfs_remove_group(&pdev->dev.kobj, &temperature_attributes_group);
+	applesmc_destroy_nodes(temp_group);
 	while (fans_handled)
 		sysfs_remove_group(&pdev->dev.kobj,
 				   &fan_attribute_groups[--fans_handled]);
-- 
1.7.1

Re: [lm-sensors] [PATCH 3/8] hwmon: applesmc: Dynamic creation of temperature files

[Guenter Roeck]

On Sun, Oct 31, 2010 at 03:50:29AM -0400, Henrik Rydberg wrote:
> The current driver creates temperature files based on a list
> of temperature keys given per device. Apart from slow adaption
> to new machine models, the number of sensors also depends on
> the number of processors. This patch looks up the temperature
> keys dynamically, thereby supporting all models.
> 
> Signed-off-by: Henrik Rydberg <rydberg@euromail.se>

Hi Henrik,

Nice cleanup. Just a coule of minor comments.

> ---
>  drivers/hwmon/applesmc.c |  498 +++++++++++-----------------------------------
>  1 files changed, 113 insertions(+), 385 deletions(-)
> 
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index 7f030f0..0207618 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
[ ...]
> 
> -       pr_info("key=%d\n", s->key_count);
> +       pr_info("key=%d temp=%d\n", s->key_count, s->temp_count);

Is this useful or just noisy ? How about pr_debug instead ?

> 
>         return 0;
>  }

[ ... ] 

> @@ -1429,13 +1122,66 @@ static int applesmc_dmi_match(const struct dmi_system_id *id)
>         pr_info(" - Model %s light sensors and backlight\n",
>                 applesmc_light ? "with" : "without");
> 
> -       applesmc_temperature_set =  dmi_data->temperature_set;
> -       while (temperature_sensors_sets[applesmc_temperature_set][i] != NULL)
> -               i++;
> -       pr_info(" - Model with %d temperature sensors\n", i);
> +

This results in another double empty line.

Guenter

[PATCH 4/8] hwmon: applesmc: Handle new temperature format

[Henrik Rydberg]

The recent Macbooks have temperature registers of a new type.
This patch adds the logic to handle them.

Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
---
 drivers/hwmon/applesmc.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 0207618..ec92aff 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -733,13 +733,19 @@ static ssize_t applesmc_show_temperature(struct device *dev,
 	ret = applesmc_get_entry_by_index(index, &entry);
 	if (ret)
 		return ret;
+	if (entry.len > 2)
+		return -EINVAL;
 
-	ret = applesmc_read_entry(&entry, buffer, 2);
+	ret = applesmc_read_entry(&entry, buffer, entry.len);
 	if (ret)
 		return ret;
 
-	temp = buffer[0]*1000;
-	temp += (buffer[1] >> 6) * 250;
+	if (entry.len == 2) {
+		temp = buffer[0]*1000;
+		temp += (buffer[1] >> 6) * 250;
+	} else {
+		temp = buffer[0] * 4000;
+	}
 
 	return snprintf(sysfsbuf, PAGE_SIZE, "%u\n", temp);
 }
-- 
1.7.1

Re: [lm-sensors] [PATCH 4/8] hwmon: applesmc: Handle new temperature format

[Guenter Roeck]

On Sun, Oct 31, 2010 at 03:50:30AM -0400, Henrik Rydberg wrote:
> The recent Macbooks have temperature registers of a new type.
> This patch adds the logic to handle them.
> 
> Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
> ---
>  drivers/hwmon/applesmc.c |   12 +++++++++---
>  1 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index 0207618..ec92aff 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
> @@ -733,13 +733,19 @@ static ssize_t applesmc_show_temperature(struct device *dev,
>  	ret = applesmc_get_entry_by_index(index, &entry);
>  	if (ret)
>  		return ret;
> +	if (entry.len > 2)
> +		return -EINVAL;
>  
> -	ret = applesmc_read_entry(&entry, buffer, 2);
> +	ret = applesmc_read_entry(&entry, buffer, entry.len);
>  	if (ret)
>  		return ret;
>  
> -	temp = buffer[0]*1000;
> -	temp += (buffer[1] >> 6) * 250;
> +	if (entry.len == 2) {
> +		temp = buffer[0]*1000;

Should be 
		temp = buffer[0] * 1000;

Otherwise looks good.

Guenter

Re: [lm-sensors] [PATCH 4/8] hwmon: applesmc: Handle new temperature format

[Guenter Roeck]

On Sun, Oct 31, 2010 at 03:50:30AM -0400, Henrik Rydberg wrote:
> The recent Macbooks have temperature registers of a new type.
> This patch adds the logic to handle them.
> 
> Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
> ---
>  drivers/hwmon/applesmc.c |   12 +++++++++---
>  1 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index 0207618..ec92aff 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
> @@ -733,13 +733,19 @@ static ssize_t applesmc_show_temperature(struct device *dev,
>  	ret = applesmc_get_entry_by_index(index, &entry);
>  	if (ret)
>  		return ret;
> +	if (entry.len > 2)
> +		return -EINVAL;
>  
> -	ret = applesmc_read_entry(&entry, buffer, 2);
> +	ret = applesmc_read_entry(&entry, buffer, entry.len);
>  	if (ret)
>  		return ret;
>  
> -	temp = buffer[0]*1000;
> -	temp += (buffer[1] >> 6) * 250;
> +	if (entry.len == 2) {
> +		temp = buffer[0]*1000;
> +		temp += (buffer[1] >> 6) * 250;
> +	} else {
> +		temp = buffer[0] * 4000;
> +	}

Another comment - in the next patch, you check for entry.len == 0.
If that can happen, you would need to check it here as well.

Guenter

Re: [lm-sensors] [PATCH 4/8] hwmon: applesmc: Handle new temperature format

[Henrik Rydberg]

>>

>> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
>> index 0207618..ec92aff 100644
>> --- a/drivers/hwmon/applesmc.c
>> +++ b/drivers/hwmon/applesmc.c
>> @@ -733,13 +733,19 @@ static ssize_t applesmc_show_temperature(struct device *dev,
>>  	ret = applesmc_get_entry_by_index(index, &entry);
>>  	if (ret)
>>  		return ret;
>> +	if (entry.len > 2)
>> +		return -EINVAL;
>>  
>> -	ret = applesmc_read_entry(&entry, buffer, 2);
>> +	ret = applesmc_read_entry(&entry, buffer, entry.len);
>>  	if (ret)
>>  		return ret;
>>  
>> -	temp = buffer[0]*1000;
>> -	temp += (buffer[1] >> 6) * 250;
>> +	if (entry.len == 2) {
>> +		temp = buffer[0]*1000;
>> +		temp += (buffer[1] >> 6) * 250;
>> +	} else {
>> +		temp = buffer[0] * 4000;
>> +	}
> 
> Another comment - in the next patch, you check for entry.len == 0.
> If that can happen, you would need to check it here as well.


It cannot happen after a succesful return from get_entry(), but I will check it
again. Earlier versions used entry.len rather than entry.valid, which can still
be seen in a few places.

Thanks,
Henrik

[PATCH 5/8] hwmon: applesmc: Extract all features generically

[Henrik Rydberg]

With temperature keys being determined automatically, the dmi match
data is only used to assign features that can easily be detected from
the smc. This patch removes the dmi match data altogether, and reduces
the match table to the main machine models.

Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
---
 drivers/hwmon/applesmc.c |  236 ++++++++++++----------------------------------
 1 files changed, 60 insertions(+), 176 deletions(-)

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index ec92aff..abbff0b 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -106,16 +106,6 @@ static const char* fan_speed_keys[] = {
 
 #define to_index(attr) (to_sensor_dev_attr(attr)->index)
 
-/* Structure to be passed to DMI_MATCH function */
-struct dmi_match_data {
-/* Indicates whether this computer has an accelerometer. */
-	int accelerometer;
-/* Indicates whether this computer has light sensors and keyboard backlight. */
-	int light;
-/* Indicates which temperature sensors set to use. */
-	int temperature_set;
-};
-
 /* Dynamic device node attributes */
 struct applesmc_dev_attr {
 	struct sensor_device_attribute sda;	/* hwmon attributes */
@@ -145,6 +135,10 @@ static struct applesmc_registers {
 	unsigned int temp_count;	/* number of temperature registers */
 	unsigned int temp_begin;	/* temperature lower index bound */
 	unsigned int temp_end;		/* temperature upper index bound */
+	bool has_accelerometer;		/* has motion sensor */
+	bool has_left_light;		/* has left light sensor */
+	bool has_right_light;		/* has right light sensor */
+	bool has_key_light;		/* has keyboard backlight */
 	bool init_complete;		/* true when fully initialized */
 	struct applesmc_entry *entry;	/* key entries */
 } smcreg;
@@ -158,12 +152,6 @@ static u8 backlight_state[2];
 static struct device *hwmon_dev;
 static struct input_polled_dev *applesmc_idev;
 
-/* Indicates whether this computer has an accelerometer. */
-static unsigned int applesmc_accelerometer;
-
-/* Indicates whether this computer has light sensors and keyboard backlight. */
-static unsigned int applesmc_light;
-
 /* The number of fans handled by the driver */
 static unsigned int fans_handled;
 
@@ -431,6 +419,19 @@ static int applesmc_write_key(const char *key, const u8 *buffer, u8 len)
 	return applesmc_write_entry(&entry, buffer, len);
 }
 
+static int applesmc_has_key(const char *key, bool *value)
+{
+	struct applesmc_entry entry;
+	int ret;
+
+	ret = applesmc_get_entry_by_key(key, &entry);
+	if (ret && ret != -EINVAL)
+		return ret;
+
+	*value = ret == 0 && entry.len;
+	return 0;
+}
+
 /*
  * applesmc_read_motion_sensor - Read motion sensor (X, Y or Z).
  */
@@ -467,7 +468,7 @@ static int applesmc_device_init(void)
 	int total, ret = -ENXIO;
 	u8 buffer[2];
 
-	if (!applesmc_accelerometer)
+	if (!smcreg.has_accelerometer)
 		return 0;
 
 	for (total = INIT_TIMEOUT_MSECS; total > 0; total -= INIT_WAIT_MSECS) {
@@ -534,9 +535,26 @@ static int applesmc_init_smcreg_try(void)
 		return ret;
 	s->temp_count = s->temp_end - s->temp_begin;
 
+	ret = applesmc_has_key(LIGHT_SENSOR_LEFT_KEY, &s->has_left_light);
+	if (ret)
+		return ret;
+	ret = applesmc_has_key(LIGHT_SENSOR_RIGHT_KEY, &s->has_right_light);
+	if (ret)
+		return ret;
+	ret = applesmc_has_key(MOTION_SENSOR_KEY, &s->has_accelerometer);
+	if (ret)
+		return ret;
+	ret = applesmc_has_key(BACKLIGHT_KEY, &s->has_key_light);
+	if (ret)
+		return ret;
+
 	s->init_complete = true;
 
-	pr_info("key=%d temp=%d\n", s->key_count, s->temp_count);
+	pr_info("key=%d temp=%d acc=%d, light=%d, keyb=%d\n",
+	       s->key_count, s->temp_count,
+	       s->has_accelerometer,
+	       s->has_left_light + s->has_right_light,
+	       s->has_key_light);
 
 	return 0;
 }
@@ -585,7 +603,7 @@ static int applesmc_probe(struct platform_device *dev)
 /* Synchronize device with memorized backlight state */
 static int applesmc_pm_resume(struct device *dev)
 {
-	if (applesmc_light)
+	if (smcreg.has_key_light)
 		applesmc_write_key(BACKLIGHT_KEY, backlight_state, 2);
 	return 0;
 }
@@ -1115,24 +1133,6 @@ static struct applesmc_node_group temp_group[] = {
 /* Module stuff */
 
 /*
- * applesmc_dmi_match - found a match.  return one, short-circuiting the hunt.
- */
-static int applesmc_dmi_match(const struct dmi_system_id *id)
-{
-	struct dmi_match_data* dmi_data = id->driver_data;
-	pr_info("%s detected:\n", id->ident);
-	applesmc_accelerometer = dmi_data->accelerometer;
-	pr_info(" - Model %s accelerometer\n",
-		applesmc_accelerometer ? "with" : "without");
-	applesmc_light = dmi_data->light;
-	pr_info(" - Model %s light sensors and backlight\n",
-		applesmc_light ? "with" : "without");
-
-
-	return 1;
-}
-
-/*
  * applesmc_destroy_nodes - remove files and free associated memory
  */
 static void applesmc_destroy_nodes(struct applesmc_node_group *groups)
@@ -1247,158 +1247,39 @@ static void applesmc_release_accelerometer(void)
 	sysfs_remove_group(&pdev->dev.kobj, &accelerometer_attributes_group);
 }
 
-static __initdata struct dmi_match_data applesmc_dmi_data[] = {
-/* MacBook Pro: accelerometer, backlight and temperature set 0 */
-	{ .accelerometer = 1, .light = 1, .temperature_set = 0 },
-/* MacBook2: accelerometer and temperature set 1 */
-	{ .accelerometer = 1, .light = 0, .temperature_set = 1 },
-/* MacBook: accelerometer and temperature set 2 */
-	{ .accelerometer = 1, .light = 0, .temperature_set = 2 },
-/* MacMini: temperature set 3 */
-	{ .accelerometer = 0, .light = 0, .temperature_set = 3 },
-/* MacPro: temperature set 4 */
-	{ .accelerometer = 0, .light = 0, .temperature_set = 4 },
-/* iMac: temperature set 5 */
-	{ .accelerometer = 0, .light = 0, .temperature_set = 5 },
-/* MacBook3, MacBook4: accelerometer and temperature set 6 */
-	{ .accelerometer = 1, .light = 0, .temperature_set = 6 },
-/* MacBook Air: accelerometer, backlight and temperature set 7 */
-	{ .accelerometer = 1, .light = 1, .temperature_set = 7 },
-/* MacBook Pro 4: accelerometer, backlight and temperature set 8 */
-	{ .accelerometer = 1, .light = 1, .temperature_set = 8 },
-/* MacBook Pro 3: accelerometer, backlight and temperature set 9 */
-	{ .accelerometer = 1, .light = 1, .temperature_set = 9 },
-/* iMac 5: light sensor only, temperature set 10 */
-	{ .accelerometer = 0, .light = 0, .temperature_set = 10 },
-/* MacBook 5: accelerometer, backlight and temperature set 11 */
-	{ .accelerometer = 1, .light = 1, .temperature_set = 11 },
-/* MacBook Pro 5: accelerometer, backlight and temperature set 12 */
-	{ .accelerometer = 1, .light = 1, .temperature_set = 12 },
-/* iMac 8: light sensor only, temperature set 13 */
-	{ .accelerometer = 0, .light = 0, .temperature_set = 13 },
-/* iMac 6: light sensor only, temperature set 14 */
-	{ .accelerometer = 0, .light = 0, .temperature_set = 14 },
-/* MacBook Air 2,1: accelerometer, backlight and temperature set 15 */
-	{ .accelerometer = 1, .light = 1, .temperature_set = 15 },
-/* MacPro3,1: temperature set 16 */
-	{ .accelerometer = 0, .light = 0, .temperature_set = 16 },
-/* iMac 9,1: light sensor only, temperature set 17 */
-	{ .accelerometer = 0, .light = 0, .temperature_set = 17 },
-/* MacBook Pro 2,2: accelerometer, backlight and temperature set 18 */
-	{ .accelerometer = 1, .light = 1, .temperature_set = 18 },
-/* MacBook Pro 5,3: accelerometer, backlight and temperature set 19 */
-	{ .accelerometer = 1, .light = 1, .temperature_set = 19 },
-/* MacBook Pro 5,4: accelerometer, backlight and temperature set 20 */
-	{ .accelerometer = 1, .light = 1, .temperature_set = 20 },
-/* MacBook Pro 6,2: accelerometer, backlight and temperature set 21 */
-	{ .accelerometer = 1, .light = 1, .temperature_set = 21 },
-/* MacBook Pro 7,1: accelerometer, backlight and temperature set 22 */
-	{ .accelerometer = 1, .light = 1, .temperature_set = 22 },
-};
+static int applesmc_dmi_match(const struct dmi_system_id *id)
+{
+	pr_info("%s detected\n", id->ident);
+	return 1;
+}
 
 /* Note that DMI_MATCH(...,"MacBook") will match "MacBookPro1,1".
  * So we need to put "Apple MacBook Pro" before "Apple MacBook". */
 static __initdata struct dmi_system_id applesmc_whitelist[] = {
-	{ applesmc_dmi_match, "Apple MacBook Air 2", {
-	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
-	  DMI_MATCH(DMI_PRODUCT_NAME, "MacBookAir2") },
-		&applesmc_dmi_data[15]},
 	{ applesmc_dmi_match, "Apple MacBook Air", {
 	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
 	  DMI_MATCH(DMI_PRODUCT_NAME, "MacBookAir") },
-		&applesmc_dmi_data[7]},
-	{ applesmc_dmi_match, "Apple MacBook Pro 7", {
-	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
-	  DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro7") },
-		&applesmc_dmi_data[22]},
-	{ applesmc_dmi_match, "Apple MacBook Pro 5,4", {
-	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
-	  DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro5,4") },
-		&applesmc_dmi_data[20]},
-	{ applesmc_dmi_match, "Apple MacBook Pro 5,3", {
-	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
-	  DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro5,3") },
-		&applesmc_dmi_data[19]},
-	{ applesmc_dmi_match, "Apple MacBook Pro 6", {
-	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
-	  DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro6") },
-		&applesmc_dmi_data[21]},
-	{ applesmc_dmi_match, "Apple MacBook Pro 5", {
-	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
-	  DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro5") },
-		&applesmc_dmi_data[12]},
-	{ applesmc_dmi_match, "Apple MacBook Pro 4", {
-	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
-	  DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro4") },
-		&applesmc_dmi_data[8]},
-	{ applesmc_dmi_match, "Apple MacBook Pro 3", {
-	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
-	  DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro3") },
-		&applesmc_dmi_data[9]},
-	{ applesmc_dmi_match, "Apple MacBook Pro 2,2", {
-	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple Computer, Inc."),
-	  DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro2,2") },
-		&applesmc_dmi_data[18]},
+	},
 	{ applesmc_dmi_match, "Apple MacBook Pro", {
 	  DMI_MATCH(DMI_BOARD_VENDOR,"Apple"),
 	  DMI_MATCH(DMI_PRODUCT_NAME,"MacBookPro") },
-		&applesmc_dmi_data[0]},
-	{ applesmc_dmi_match, "Apple MacBook (v2)", {
-	  DMI_MATCH(DMI_BOARD_VENDOR,"Apple"),
-	  DMI_MATCH(DMI_PRODUCT_NAME,"MacBook2") },
-		&applesmc_dmi_data[1]},
-	{ applesmc_dmi_match, "Apple MacBook (v3)", {
-	  DMI_MATCH(DMI_BOARD_VENDOR,"Apple"),
-	  DMI_MATCH(DMI_PRODUCT_NAME,"MacBook3") },
-		&applesmc_dmi_data[6]},
-	{ applesmc_dmi_match, "Apple MacBook 4", {
-	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
-	  DMI_MATCH(DMI_PRODUCT_NAME, "MacBook4") },
-		&applesmc_dmi_data[6]},
-	{ applesmc_dmi_match, "Apple MacBook 5", {
-	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
-	  DMI_MATCH(DMI_PRODUCT_NAME, "MacBook5") },
-		&applesmc_dmi_data[11]},
+	},
 	{ applesmc_dmi_match, "Apple MacBook", {
 	  DMI_MATCH(DMI_BOARD_VENDOR,"Apple"),
 	  DMI_MATCH(DMI_PRODUCT_NAME,"MacBook") },
-		&applesmc_dmi_data[2]},
+	},
 	{ applesmc_dmi_match, "Apple Macmini", {
 	  DMI_MATCH(DMI_BOARD_VENDOR,"Apple"),
 	  DMI_MATCH(DMI_PRODUCT_NAME,"Macmini") },
-		&applesmc_dmi_data[3]},
-	{ applesmc_dmi_match, "Apple MacPro2", {
-	  DMI_MATCH(DMI_BOARD_VENDOR,"Apple"),
-	  DMI_MATCH(DMI_PRODUCT_NAME,"MacPro2") },
-		&applesmc_dmi_data[4]},
-	{ applesmc_dmi_match, "Apple MacPro3", {
-	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
-	  DMI_MATCH(DMI_PRODUCT_NAME, "MacPro3") },
-		&applesmc_dmi_data[16]},
+	},
 	{ applesmc_dmi_match, "Apple MacPro", {
 	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
 	  DMI_MATCH(DMI_PRODUCT_NAME, "MacPro") },
-		&applesmc_dmi_data[4]},
-	{ applesmc_dmi_match, "Apple iMac 9,1", {
-	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple Inc."),
-	  DMI_MATCH(DMI_PRODUCT_NAME, "iMac9,1") },
-		&applesmc_dmi_data[17]},
-	{ applesmc_dmi_match, "Apple iMac 8", {
-	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
-	  DMI_MATCH(DMI_PRODUCT_NAME, "iMac8") },
-		&applesmc_dmi_data[13]},
-	{ applesmc_dmi_match, "Apple iMac 6", {
-	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
-	  DMI_MATCH(DMI_PRODUCT_NAME, "iMac6") },
-		&applesmc_dmi_data[14]},
-	{ applesmc_dmi_match, "Apple iMac 5", {
-	  DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
-	  DMI_MATCH(DMI_PRODUCT_NAME, "iMac5") },
-		&applesmc_dmi_data[10]},
+	},
 	{ applesmc_dmi_match, "Apple iMac", {
 	  DMI_MATCH(DMI_BOARD_VENDOR,"Apple"),
 	  DMI_MATCH(DMI_PRODUCT_NAME,"iMac") },
-		&applesmc_dmi_data[5]},
+	},
 	{ .ident = NULL }
 };
 
@@ -1468,18 +1349,20 @@ static int __init applesmc_init(void)
 	if (ret)
 		goto out_fans;
 
-	if (applesmc_accelerometer) {
+	if (smcreg.has_accelerometer) {
 		ret = applesmc_create_accelerometer();
 		if (ret)
 			goto out_temperature;
 	}
 
-	if (applesmc_light) {
+	if (smcreg.has_left_light) {
 		/* Add light sensor file */
 		ret = sysfs_create_file(&pdev->dev.kobj, &dev_attr_light.attr);
 		if (ret)
 			goto out_accelerometer;
+	}
 
+	if (smcreg.has_key_light) {
 		/* Create the workqueue */
 		applesmc_led_wq = create_singlethread_workqueue("applesmc-led");
 		if (!applesmc_led_wq) {
@@ -1504,16 +1387,16 @@ static int __init applesmc_init(void)
 	return 0;
 
 out_light_ledclass:
-	if (applesmc_light)
+	if (smcreg.has_key_light)
 		led_classdev_unregister(&applesmc_backlight);
 out_light_wq:
-	if (applesmc_light)
+	if (smcreg.has_key_light)
 		destroy_workqueue(applesmc_led_wq);
 out_light_sysfs:
-	if (applesmc_light)
+	if (smcreg.has_left_light)
 		sysfs_remove_file(&pdev->dev.kobj, &dev_attr_light.attr);
 out_accelerometer:
-	if (applesmc_accelerometer)
+	if (smcreg.has_accelerometer)
 		applesmc_release_accelerometer();
 out_temperature:
 	applesmc_destroy_nodes(temp_group);
@@ -1540,12 +1423,13 @@ out:
 static void __exit applesmc_exit(void)
 {
 	hwmon_device_unregister(hwmon_dev);
-	if (applesmc_light) {
+	if (smcreg.has_key_light) {
 		led_classdev_unregister(&applesmc_backlight);
 		destroy_workqueue(applesmc_led_wq);
-		sysfs_remove_file(&pdev->dev.kobj, &dev_attr_light.attr);
 	}
-	if (applesmc_accelerometer)
+	if (smcreg.has_left_light)
+		sysfs_remove_file(&pdev->dev.kobj, &dev_attr_light.attr);
+	if (smcreg.has_accelerometer)
 		applesmc_release_accelerometer();
 	applesmc_destroy_nodes(temp_group);
 	while (fans_handled)
-- 
1.7.1

Re: [lm-sensors] [PATCH 5/8] hwmon: applesmc: Extract all features generically

[Guenter Roeck]

On Sun, Oct 31, 2010 at 03:50:31AM -0400, Henrik Rydberg wrote:
> With temperature keys being determined automatically, the dmi match
> data is only used to assign features that can easily be detected from
> the smc. This patch removes the dmi match data altogether, and reduces
> the match table to the main machine models.
> 
> Signed-off-by: Henrik Rydberg <rydberg@euromail.se>

Hi Henrik,

again, nice cleanup. Couple of comments below.

> ---
>  drivers/hwmon/applesmc.c |  236 ++++++++++++----------------------------------
>  1 files changed, 60 insertions(+), 176 deletions(-)
> 
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index ec92aff..abbff0b 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
> @@ -106,16 +106,6 @@ static const char* fan_speed_keys[] = {
> 
>  #define to_index(attr) (to_sensor_dev_attr(attr)->index)
> 
> -/* Structure to be passed to DMI_MATCH function */
> -struct dmi_match_data {
> -/* Indicates whether this computer has an accelerometer. */
> -       int accelerometer;
> -/* Indicates whether this computer has light sensors and keyboard backlight. */
> -       int light;
> -/* Indicates which temperature sensors set to use. */
> -       int temperature_set;
> -};
> -
>  /* Dynamic device node attributes */
>  struct applesmc_dev_attr {
>         struct sensor_device_attribute sda;     /* hwmon attributes */
> @@ -145,6 +135,10 @@ static struct applesmc_registers {
>         unsigned int temp_count;        /* number of temperature registers */
>         unsigned int temp_begin;        /* temperature lower index bound */
>         unsigned int temp_end;          /* temperature upper index bound */
> +       bool has_accelerometer;         /* has motion sensor */
> +       bool has_left_light;            /* has left light sensor */
> +       bool has_right_light;           /* has right light sensor */
> +       bool has_key_light;             /* has keyboard backlight */

Maybe has_key_backlight ? Might be a bit less confusing.

>         bool init_complete;             /* true when fully initialized */
>         struct applesmc_entry *entry;   /* key entries */
>  } smcreg;
> @@ -158,12 +152,6 @@ static u8 backlight_state[2];
>  static struct device *hwmon_dev;
>  static struct input_polled_dev *applesmc_idev;
> 
> -/* Indicates whether this computer has an accelerometer. */
> -static unsigned int applesmc_accelerometer;
> -
> -/* Indicates whether this computer has light sensors and keyboard backlight. */
> -static unsigned int applesmc_light;
> -
>  /* The number of fans handled by the driver */
>  static unsigned int fans_handled;
> 
> @@ -431,6 +419,19 @@ static int applesmc_write_key(const char *key, const u8 *buffer, u8 len)
>         return applesmc_write_entry(&entry, buffer, len);
>  }
> 
> +static int applesmc_has_key(const char *key, bool *value)
> +{
> +       struct applesmc_entry entry;
> +       int ret;
> +
> +       ret = applesmc_get_entry_by_key(key, &entry);
> +       if (ret && ret != -EINVAL)
> +               return ret;
> +
> +       *value = ret == 0 && entry.len;
> +       return 0;
> +}
> +
>  /*
>   * applesmc_read_motion_sensor - Read motion sensor (X, Y or Z).
>   */
> @@ -467,7 +468,7 @@ static int applesmc_device_init(void)
>         int total, ret = -ENXIO;
>         u8 buffer[2];
> 
> -       if (!applesmc_accelerometer)
> +       if (!smcreg.has_accelerometer)
>                 return 0;
> 
>         for (total = INIT_TIMEOUT_MSECS; total > 0; total -= INIT_WAIT_MSECS) {
> @@ -534,9 +535,26 @@ static int applesmc_init_smcreg_try(void)
>                 return ret;
>         s->temp_count = s->temp_end - s->temp_begin;
> 
> +       ret = applesmc_has_key(LIGHT_SENSOR_LEFT_KEY, &s->has_left_light);
> +       if (ret)
> +               return ret;
> +       ret = applesmc_has_key(LIGHT_SENSOR_RIGHT_KEY, &s->has_right_light);
> +       if (ret)
> +               return ret;
> +       ret = applesmc_has_key(MOTION_SENSOR_KEY, &s->has_accelerometer);
> +       if (ret)
> +               return ret;
> +       ret = applesmc_has_key(BACKLIGHT_KEY, &s->has_key_light);
> +       if (ret)
> +               return ret;
> +
>         s->init_complete = true;
> 
> -       pr_info("key=%d temp=%d\n", s->key_count, s->temp_count);
> +       pr_info("key=%d temp=%d acc=%d, light=%d, keyb=%d\n",

	light= and keyb= are a bit confusing. Maybe at least use backlight= ?

> +              s->key_count, s->temp_count,
> +              s->has_accelerometer,
> +              s->has_left_light + s->has_right_light,

Resulting output is 0, 1, or 3. And "1" can mean left or right.
Is this useful ?

> +              s->has_key_light);
> 
>         return 0;
>  }
> @@ -585,7 +603,7 @@ static int applesmc_probe(struct platform_device *dev)
>  /* Synchronize device with memorized backlight state */
>  static int applesmc_pm_resume(struct device *dev)
>  {
> -       if (applesmc_light)
> +       if (smcreg.has_key_light)
>                 applesmc_write_key(BACKLIGHT_KEY, backlight_state, 2);
>         return 0;
>  }
> @@ -1115,24 +1133,6 @@ static struct applesmc_node_group temp_group[] = {
>  /* Module stuff */
> 
>  /*
> - * applesmc_dmi_match - found a match.  return one, short-circuiting the hunt.
> - */
> -static int applesmc_dmi_match(const struct dmi_system_id *id)
> -{
> -       struct dmi_match_data* dmi_data = id->driver_data;
> -       pr_info("%s detected:\n", id->ident);
> -       applesmc_accelerometer = dmi_data->accelerometer;
> -       pr_info(" - Model %s accelerometer\n",
> -               applesmc_accelerometer ? "with" : "without");
> -       applesmc_light = dmi_data->light;
> -       pr_info(" - Model %s light sensors and backlight\n",
> -               applesmc_light ? "with" : "without");
> -
> -
> -       return 1;
> -}
> -
> -/*
>   * applesmc_destroy_nodes - remove files and free associated memory
>   */
>  static void applesmc_destroy_nodes(struct applesmc_node_group *groups)
> @@ -1247,158 +1247,39 @@ static void applesmc_release_accelerometer(void)
>         sysfs_remove_group(&pdev->dev.kobj, &accelerometer_attributes_group);
>  }
> 
> -static __initdata struct dmi_match_data applesmc_dmi_data[] = {
> -/* MacBook Pro: accelerometer, backlight and temperature set 0 */
> -       { .accelerometer = 1, .light = 1, .temperature_set = 0 },
> -/* MacBook2: accelerometer and temperature set 1 */
> -       { .accelerometer = 1, .light = 0, .temperature_set = 1 },
> -/* MacBook: accelerometer and temperature set 2 */
> -       { .accelerometer = 1, .light = 0, .temperature_set = 2 },
> -/* MacMini: temperature set 3 */
> -       { .accelerometer = 0, .light = 0, .temperature_set = 3 },
> -/* MacPro: temperature set 4 */
> -       { .accelerometer = 0, .light = 0, .temperature_set = 4 },
> -/* iMac: temperature set 5 */
> -       { .accelerometer = 0, .light = 0, .temperature_set = 5 },
> -/* MacBook3, MacBook4: accelerometer and temperature set 6 */
> -       { .accelerometer = 1, .light = 0, .temperature_set = 6 },
> -/* MacBook Air: accelerometer, backlight and temperature set 7 */
> -       { .accelerometer = 1, .light = 1, .temperature_set = 7 },
> -/* MacBook Pro 4: accelerometer, backlight and temperature set 8 */
> -       { .accelerometer = 1, .light = 1, .temperature_set = 8 },
> -/* MacBook Pro 3: accelerometer, backlight and temperature set 9 */
> -       { .accelerometer = 1, .light = 1, .temperature_set = 9 },
> -/* iMac 5: light sensor only, temperature set 10 */
> -       { .accelerometer = 0, .light = 0, .temperature_set = 10 },
> -/* MacBook 5: accelerometer, backlight and temperature set 11 */
> -       { .accelerometer = 1, .light = 1, .temperature_set = 11 },
> -/* MacBook Pro 5: accelerometer, backlight and temperature set 12 */
> -       { .accelerometer = 1, .light = 1, .temperature_set = 12 },
> -/* iMac 8: light sensor only, temperature set 13 */
> -       { .accelerometer = 0, .light = 0, .temperature_set = 13 },
> -/* iMac 6: light sensor only, temperature set 14 */
> -       { .accelerometer = 0, .light = 0, .temperature_set = 14 },
> -/* MacBook Air 2,1: accelerometer, backlight and temperature set 15 */
> -       { .accelerometer = 1, .light = 1, .temperature_set = 15 },
> -/* MacPro3,1: temperature set 16 */
> -       { .accelerometer = 0, .light = 0, .temperature_set = 16 },
> -/* iMac 9,1: light sensor only, temperature set 17 */
> -       { .accelerometer = 0, .light = 0, .temperature_set = 17 },
> -/* MacBook Pro 2,2: accelerometer, backlight and temperature set 18 */
> -       { .accelerometer = 1, .light = 1, .temperature_set = 18 },
> -/* MacBook Pro 5,3: accelerometer, backlight and temperature set 19 */
> -       { .accelerometer = 1, .light = 1, .temperature_set = 19 },
> -/* MacBook Pro 5,4: accelerometer, backlight and temperature set 20 */
> -       { .accelerometer = 1, .light = 1, .temperature_set = 20 },
> -/* MacBook Pro 6,2: accelerometer, backlight and temperature set 21 */
> -       { .accelerometer = 1, .light = 1, .temperature_set = 21 },
> -/* MacBook Pro 7,1: accelerometer, backlight and temperature set 22 */
> -       { .accelerometer = 1, .light = 1, .temperature_set = 22 },
> -};
> +static int applesmc_dmi_match(const struct dmi_system_id *id)
> +{
> +       pr_info("%s detected\n", id->ident);

Is this useful enough to be pr_info or just noisy ?

> +       return 1;
> +}
> 
>  /* Note that DMI_MATCH(...,"MacBook") will match "MacBookPro1,1".
>   * So we need to put "Apple MacBook Pro" before "Apple MacBook". */
>  static __initdata struct dmi_system_id applesmc_whitelist[] = {
> -       { applesmc_dmi_match, "Apple MacBook Air 2", {
> -         DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
> -         DMI_MATCH(DMI_PRODUCT_NAME, "MacBookAir2") },
> -               &applesmc_dmi_data[15]},
>         { applesmc_dmi_match, "Apple MacBook Air", {
>           DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
>           DMI_MATCH(DMI_PRODUCT_NAME, "MacBookAir") },
> -               &applesmc_dmi_data[7]},
> -       { applesmc_dmi_match, "Apple MacBook Pro 7", {
> -         DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
> -         DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro7") },
> -               &applesmc_dmi_data[22]},
> -       { applesmc_dmi_match, "Apple MacBook Pro 5,4", {
> -         DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
> -         DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro5,4") },
> -               &applesmc_dmi_data[20]},
> -       { applesmc_dmi_match, "Apple MacBook Pro 5,3", {
> -         DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
> -         DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro5,3") },
> -               &applesmc_dmi_data[19]},
> -       { applesmc_dmi_match, "Apple MacBook Pro 6", {
> -         DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
> -         DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro6") },
> -               &applesmc_dmi_data[21]},
> -       { applesmc_dmi_match, "Apple MacBook Pro 5", {
> -         DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
> -         DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro5") },
> -               &applesmc_dmi_data[12]},
> -       { applesmc_dmi_match, "Apple MacBook Pro 4", {
> -         DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
> -         DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro4") },
> -               &applesmc_dmi_data[8]},
> -       { applesmc_dmi_match, "Apple MacBook Pro 3", {
> -         DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
> -         DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro3") },
> -               &applesmc_dmi_data[9]},
> -       { applesmc_dmi_match, "Apple MacBook Pro 2,2", {
> -         DMI_MATCH(DMI_BOARD_VENDOR, "Apple Computer, Inc."),
> -         DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro2,2") },
> -               &applesmc_dmi_data[18]},
> +       },
>         { applesmc_dmi_match, "Apple MacBook Pro", {
>           DMI_MATCH(DMI_BOARD_VENDOR,"Apple"),
>           DMI_MATCH(DMI_PRODUCT_NAME,"MacBookPro") },
> -               &applesmc_dmi_data[0]},
> -       { applesmc_dmi_match, "Apple MacBook (v2)", {
> -         DMI_MATCH(DMI_BOARD_VENDOR,"Apple"),
> -         DMI_MATCH(DMI_PRODUCT_NAME,"MacBook2") },
> -               &applesmc_dmi_data[1]},
> -       { applesmc_dmi_match, "Apple MacBook (v3)", {
> -         DMI_MATCH(DMI_BOARD_VENDOR,"Apple"),
> -         DMI_MATCH(DMI_PRODUCT_NAME,"MacBook3") },
> -               &applesmc_dmi_data[6]},
> -       { applesmc_dmi_match, "Apple MacBook 4", {
> -         DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
> -         DMI_MATCH(DMI_PRODUCT_NAME, "MacBook4") },
> -               &applesmc_dmi_data[6]},
> -       { applesmc_dmi_match, "Apple MacBook 5", {
> -         DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
> -         DMI_MATCH(DMI_PRODUCT_NAME, "MacBook5") },
> -               &applesmc_dmi_data[11]},
> +       },
>         { applesmc_dmi_match, "Apple MacBook", {
>           DMI_MATCH(DMI_BOARD_VENDOR,"Apple"),
>           DMI_MATCH(DMI_PRODUCT_NAME,"MacBook") },
> -               &applesmc_dmi_data[2]},
> +       },
>         { applesmc_dmi_match, "Apple Macmini", {
>           DMI_MATCH(DMI_BOARD_VENDOR,"Apple"),
>           DMI_MATCH(DMI_PRODUCT_NAME,"Macmini") },
> -               &applesmc_dmi_data[3]},
> -       { applesmc_dmi_match, "Apple MacPro2", {
> -         DMI_MATCH(DMI_BOARD_VENDOR,"Apple"),
> -         DMI_MATCH(DMI_PRODUCT_NAME,"MacPro2") },
> -               &applesmc_dmi_data[4]},
> -       { applesmc_dmi_match, "Apple MacPro3", {
> -         DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
> -         DMI_MATCH(DMI_PRODUCT_NAME, "MacPro3") },
> -               &applesmc_dmi_data[16]},
> +       },
>         { applesmc_dmi_match, "Apple MacPro", {
>           DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
>           DMI_MATCH(DMI_PRODUCT_NAME, "MacPro") },
> -               &applesmc_dmi_data[4]},
> -       { applesmc_dmi_match, "Apple iMac 9,1", {
> -         DMI_MATCH(DMI_BOARD_VENDOR, "Apple Inc."),
> -         DMI_MATCH(DMI_PRODUCT_NAME, "iMac9,1") },
> -               &applesmc_dmi_data[17]},
> -       { applesmc_dmi_match, "Apple iMac 8", {
> -         DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
> -         DMI_MATCH(DMI_PRODUCT_NAME, "iMac8") },
> -               &applesmc_dmi_data[13]},
> -       { applesmc_dmi_match, "Apple iMac 6", {
> -         DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
> -         DMI_MATCH(DMI_PRODUCT_NAME, "iMac6") },
> -               &applesmc_dmi_data[14]},
> -       { applesmc_dmi_match, "Apple iMac 5", {
> -         DMI_MATCH(DMI_BOARD_VENDOR, "Apple"),
> -         DMI_MATCH(DMI_PRODUCT_NAME, "iMac5") },
> -               &applesmc_dmi_data[10]},
> +       },
>         { applesmc_dmi_match, "Apple iMac", {
>           DMI_MATCH(DMI_BOARD_VENDOR,"Apple"),
>           DMI_MATCH(DMI_PRODUCT_NAME,"iMac") },
> -               &applesmc_dmi_data[5]},
> +       },
>         { .ident = NULL }
>  };
> 
> @@ -1468,18 +1349,20 @@ static int __init applesmc_init(void)
>         if (ret)
>                 goto out_fans;
> 
> -       if (applesmc_accelerometer) {
> +       if (smcreg.has_accelerometer) {
>                 ret = applesmc_create_accelerometer();
>                 if (ret)
>                         goto out_temperature;
>         }
> 
> -       if (applesmc_light) {
> +       if (smcreg.has_left_light) {
>                 /* Add light sensor file */
>                 ret = sysfs_create_file(&pdev->dev.kobj, &dev_attr_light.attr);
>                 if (ret)
>                         goto out_accelerometer;
> +       }
> 
So "light" == left light sensor. How about the right light sensor ?

If (theoretically) a device as only a right light sensor, you would display
that a light sensor exists and then not create a sysfs file for it.

Maybe add a right_light sysfs attribute ? Or drop the knowledge about it entirely
since you don't use it anyway. Or merge the two into a single attribute file
(if that makes sense).

> +       if (smcreg.has_key_light) {
>                 /* Create the workqueue */
>                 applesmc_led_wq = create_singlethread_workqueue("applesmc-led");
>                 if (!applesmc_led_wq) {
> @@ -1504,16 +1387,16 @@ static int __init applesmc_init(void)
>         return 0;
> 
>  out_light_ledclass:
> -       if (applesmc_light)
> +       if (smcreg.has_key_light)
>                 led_classdev_unregister(&applesmc_backlight);
>  out_light_wq:
> -       if (applesmc_light)
> +       if (smcreg.has_key_light)
>                 destroy_workqueue(applesmc_led_wq);
>  out_light_sysfs:
> -       if (applesmc_light)
> +       if (smcreg.has_left_light)
>                 sysfs_remove_file(&pdev->dev.kobj, &dev_attr_light.attr);
>  out_accelerometer:
> -       if (applesmc_accelerometer)
> +       if (smcreg.has_accelerometer)
>                 applesmc_release_accelerometer();
>  out_temperature:
>         applesmc_destroy_nodes(temp_group);
> @@ -1540,12 +1423,13 @@ out:
>  static void __exit applesmc_exit(void)
>  {
>         hwmon_device_unregister(hwmon_dev);
> -       if (applesmc_light) {
> +       if (smcreg.has_key_light) {
>                 led_classdev_unregister(&applesmc_backlight);
>                 destroy_workqueue(applesmc_led_wq);
> -               sysfs_remove_file(&pdev->dev.kobj, &dev_attr_light.attr);
>         }
> -       if (applesmc_accelerometer)
> +       if (smcreg.has_left_light)
> +               sysfs_remove_file(&pdev->dev.kobj, &dev_attr_light.attr);
> +       if (smcreg.has_accelerometer)
>                 applesmc_release_accelerometer();
>         applesmc_destroy_nodes(temp_group);
>         while (fans_handled)
> --
> 1.7.1
> 
> 
> _______________________________________________
> lm-sensors mailing list
> lm-sensors@lm-sensors.org
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 5/8] hwmon: applesmc: Extract all features generically

[Henrik Rydberg]

>>  /* Dynamic device node attributes */

>>  struct applesmc_dev_attr {
>>         struct sensor_device_attribute sda;     /* hwmon attributes */
>> @@ -145,6 +135,10 @@ static struct applesmc_registers {
>>         unsigned int temp_count;        /* number of temperature registers */
>>         unsigned int temp_begin;        /* temperature lower index bound */
>>         unsigned int temp_end;          /* temperature upper index bound */
>> +       bool has_accelerometer;         /* has motion sensor */
>> +       bool has_left_light;            /* has left light sensor */
>> +       bool has_right_light;           /* has right light sensor */
>> +       bool has_key_light;             /* has keyboard backlight */
> 
> Maybe has_key_backlight ? Might be a bit less confusing.


Good point, thanks.

>> @@ -534,9 +535,26 @@ static int applesmc_init_smcreg_try(void)
>>                 return ret;
>>         s->temp_count = s->temp_end - s->temp_begin;
>>
>> +       ret = applesmc_has_key(LIGHT_SENSOR_LEFT_KEY, &s->has_left_light);
>> +       if (ret)
>> +               return ret;
>> +       ret = applesmc_has_key(LIGHT_SENSOR_RIGHT_KEY, &s->has_right_light);
>> +       if (ret)
>> +               return ret;
>> +       ret = applesmc_has_key(MOTION_SENSOR_KEY, &s->has_accelerometer);
>> +       if (ret)
>> +               return ret;
>> +       ret = applesmc_has_key(BACKLIGHT_KEY, &s->has_key_light);
>> +       if (ret)
>> +               return ret;
>> +
>>         s->init_complete = true;
>>
>> -       pr_info("key=%d temp=%d\n", s->key_count, s->temp_count);
>> +       pr_info("key=%d temp=%d acc=%d, light=%d, keyb=%d\n",
> 
> 	light= and keyb= are a bit confusing. Maybe at least use backlight= ?


I think "key" is important, since there are many confusions regarding lcd and
key backlight. And the dmesg line becomes awfully long quickly... I'll check.

> 
>> +              s->key_count, s->temp_count,
>> +              s->has_accelerometer,
>> +              s->has_left_light + s->has_right_light,
> 
> Resulting output is 0, 1, or 3. And "1" can mean left or right.
> Is this useful ?


Yes. Newer models seem to be moving away from the whole light sensor thing, but
a few machines had "stereo" light sensors, which later turned into a single
sensor. The "left and right" is less important than "stereo and mono", which
boils down to the number of sensors. Maybe I can change the names to make this
clearer.

>> +static int applesmc_dmi_match(const struct dmi_system_id *id)
>> +{
>> +       pr_info("%s detected\n", id->ident);
> 
> Is this useful enough to be pr_info or just noisy ?


Nah, noise. :-)

>> @@ -1468,18 +1349,20 @@ static int __init applesmc_init(void)
>>         if (ret)
>>                 goto out_fans;
>>
>> -       if (applesmc_accelerometer) {
>> +       if (smcreg.has_accelerometer) {
>>                 ret = applesmc_create_accelerometer();
>>                 if (ret)
>>                         goto out_temperature;
>>         }
>>
>> -       if (applesmc_light) {
>> +       if (smcreg.has_left_light) {
>>                 /* Add light sensor file */
>>                 ret = sysfs_create_file(&pdev->dev.kobj, &dev_attr_light.attr);
>>                 if (ret)
>>                         goto out_accelerometer;
>> +       }
>>
> So "light" == left light sensor. How about the right light sensor ?
> 
> If (theoretically) a device as only a right light sensor, you would display
> that a light sensor exists and then not create a sysfs file for it.


Yep. See comment above.

> Maybe add a right_light sysfs attribute ? Or drop the knowledge about it entirely
> since you don't use it anyway. Or merge the two into a single attribute file
> (if that makes sense).


Merging the has_left and has_right into a number might resolve the problems.

Thanks,
Henrik

[PATCH 6/8] hwmon: applesmc: Dynamic creation of fan files

[Henrik Rydberg]

With the dynamic temperature group in place, the setup of fans
can be simplified. This patch sets up the fans dynamically, removing
a hundred lines of code.

Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
---
 drivers/hwmon/applesmc.c |  188 +++++++++++-----------------------------------
 1 files changed, 43 insertions(+), 145 deletions(-)

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index abbff0b..4ba20ec 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -77,20 +77,15 @@
 
 #define FANS_COUNT		"FNum" /* r-o ui8 */
 #define FANS_MANUAL		"FS! " /* r-w ui16 */
-#define FAN_ACTUAL_SPEED	"F0Ac" /* r-o fpe2 (2 bytes) */
-#define FAN_MIN_SPEED		"F0Mn" /* r-o fpe2 (2 bytes) */
-#define FAN_MAX_SPEED		"F0Mx" /* r-o fpe2 (2 bytes) */
-#define FAN_SAFE_SPEED		"F0Sf" /* r-o fpe2 (2 bytes) */
-#define FAN_TARGET_SPEED	"F0Tg" /* r-w fpe2 (2 bytes) */
-#define FAN_POSITION		"F0ID" /* r-o char[16] */
+#define FAN_ID_FMT		"F%dID" /* r-o char[16] */
 
 /* List of keys used to read/write fan speeds */
-static const char* fan_speed_keys[] = {
-	FAN_ACTUAL_SPEED,
-	FAN_MIN_SPEED,
-	FAN_MAX_SPEED,
-	FAN_SAFE_SPEED,
-	FAN_TARGET_SPEED
+static const char *fan_speed_fmt[] = {
+	"F%dAc",		/* actual speed */
+	"F%dMn",		/* minimum speed (rw) */
+	"F%dMx",		/* maximum speed */
+	"F%dSf",		/* safe speed - not all models */
+	"F%dTg",		/* target speed (manual: rw) */
 };
 
 #define INIT_TIMEOUT_MSECS	5000	/* wait up to 5s for device init ... */
@@ -104,7 +99,8 @@ static const char* fan_speed_keys[] = {
 #define SENSOR_Y 1
 #define SENSOR_Z 2
 
-#define to_index(attr) (to_sensor_dev_attr(attr)->index)
+#define to_index(attr) (to_sensor_dev_attr(attr)->index & 0xffff)
+#define to_option(attr) (to_sensor_dev_attr(attr)->index >> 16)
 
 /* Dynamic device node attributes */
 struct applesmc_dev_attr {
@@ -117,6 +113,7 @@ struct applesmc_node_group {
 	char *format;				/* format string */
 	void *show;				/* show function */
 	void *store;				/* store function */
+	int option;				/* function argument */
 	struct applesmc_dev_attr *nodes;	/* dynamic node array */
 };
 
@@ -132,6 +129,7 @@ struct applesmc_entry {
 static struct applesmc_registers {
 	struct mutex mutex;		/* register read/write mutex */
 	unsigned int key_count;		/* number of SMC registers */
+	unsigned int fan_count;		/* number of fans */
 	unsigned int temp_count;	/* number of temperature registers */
 	unsigned int temp_begin;	/* temperature lower index bound */
 	unsigned int temp_end;		/* temperature upper index bound */
@@ -152,9 +150,6 @@ static u8 backlight_state[2];
 static struct device *hwmon_dev;
 static struct input_polled_dev *applesmc_idev;
 
-/* The number of fans handled by the driver */
-static unsigned int fans_handled;
-
 /*
  * Last index written to key_at_index sysfs file, and value to use for all other
  * key_at_index_* sysfs files.
@@ -490,27 +485,12 @@ out:
 }
 
 /*
- * applesmc_get_fan_count - get the number of fans.
- */
-static int applesmc_get_fan_count(void)
-{
-	int ret;
-	u8 buffer[1];
-
-	ret = applesmc_read_key(FANS_COUNT, buffer, 1);
-
-	if (ret)
-		return ret;
-	else
-		return buffer[0];
-}
-
-/*
  * applesmc_init_smcreg_try - Try to initialize register cache. Idempotent.
  */
 static int applesmc_init_smcreg_try(void)
 {
 	struct applesmc_registers *s = &smcreg;
+	u8 tmp[4];
 	int ret;
 
 	if (s->init_complete)
@@ -527,6 +507,11 @@ static int applesmc_init_smcreg_try(void)
 	if (!s->entry)
 		return -ENOMEM;
 
+	ret = applesmc_read_key(FANS_COUNT, tmp, 1);
+	if (ret)
+		return ret;
+	s->fan_count = tmp[0];
+
 	ret = applesmc_get_lower_bound(&s->temp_begin, "T");
 	if (ret)
 		return ret;
@@ -550,8 +535,8 @@ static int applesmc_init_smcreg_try(void)
 
 	s->init_complete = true;
 
-	pr_info("key=%d temp=%d acc=%d, light=%d, keyb=%d\n",
-	       s->key_count, s->temp_count,
+	pr_info("key=%d fan=%d temp=%d acc=%d, light=%d, keyb=%d\n",
+	       s->key_count, s->fan_count, s->temp_count,
 	       s->has_accelerometer,
 	       s->has_left_light + s->has_right_light,
 	       s->has_key_light);
@@ -775,14 +760,8 @@ static ssize_t applesmc_show_fan_speed(struct device *dev,
 	unsigned int speed = 0;
 	char newkey[5];
 	u8 buffer[2];
-	struct sensor_device_attribute_2 *sensor_attr =
-						to_sensor_dev_attr_2(attr);
 
-	newkey[0] = fan_speed_keys[sensor_attr->nr][0];
-	newkey[1] = '0' + sensor_attr->index;
-	newkey[2] = fan_speed_keys[sensor_attr->nr][2];
-	newkey[3] = fan_speed_keys[sensor_attr->nr][3];
-	newkey[4] = 0;
+	sprintf(newkey, fan_speed_fmt[to_option(attr)], to_index(attr));
 
 	ret = applesmc_read_key(newkey, buffer, 2);
 	speed = ((buffer[0] << 8 | buffer[1]) >> 2);
@@ -801,19 +780,13 @@ static ssize_t applesmc_store_fan_speed(struct device *dev,
 	u32 speed;
 	char newkey[5];
 	u8 buffer[2];
-	struct sensor_device_attribute_2 *sensor_attr =
-						to_sensor_dev_attr_2(attr);
 
 	speed = simple_strtoul(sysfsbuf, NULL, 10);
 
 	if (speed > 0x4000) /* Bigger than a 14-bit value */
 		return -EINVAL;
 
-	newkey[0] = fan_speed_keys[sensor_attr->nr][0];
-	newkey[1] = '0' + sensor_attr->index;
-	newkey[2] = fan_speed_keys[sensor_attr->nr][2];
-	newkey[3] = fan_speed_keys[sensor_attr->nr][3];
-	newkey[4] = 0;
+	sprintf(newkey, fan_speed_fmt[to_option(attr)], to_index(attr));
 
 	buffer[0] = (speed >> 6) & 0xff;
 	buffer[1] = (speed << 2) & 0xff;
@@ -826,15 +799,14 @@ static ssize_t applesmc_store_fan_speed(struct device *dev,
 }
 
 static ssize_t applesmc_show_fan_manual(struct device *dev,
-			struct device_attribute *devattr, char *sysfsbuf)
+			struct device_attribute *attr, char *sysfsbuf)
 {
 	int ret;
 	u16 manual = 0;
 	u8 buffer[2];
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
 
 	ret = applesmc_read_key(FANS_MANUAL, buffer, 2);
-	manual = ((buffer[0] << 8 | buffer[1]) >> attr->index) & 0x01;
+	manual = ((buffer[0] << 8 | buffer[1]) >> to_index(attr)) & 0x01;
 
 	if (ret)
 		return ret;
@@ -843,14 +815,13 @@ static ssize_t applesmc_show_fan_manual(struct device *dev,
 }
 
 static ssize_t applesmc_store_fan_manual(struct device *dev,
-					 struct device_attribute *devattr,
+					 struct device_attribute *attr,
 					 const char *sysfsbuf, size_t count)
 {
 	int ret;
 	u8 buffer[2];
 	u32 input;
 	u16 val;
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
 
 	input = simple_strtoul(sysfsbuf, NULL, 10);
 
@@ -860,9 +831,9 @@ static ssize_t applesmc_store_fan_manual(struct device *dev,
 		goto out;
 
 	if (input)
-		val = val | (0x01 << attr->index);
+		val = val | (0x01 << to_index(attr));
 	else
-		val = val & ~(0x01 << attr->index);
+		val = val & ~(0x01 << to_index(attr));
 
 	buffer[0] = (val >> 8) & 0xFF;
 	buffer[1] = val & 0xFF;
@@ -882,14 +853,8 @@ static ssize_t applesmc_show_fan_position(struct device *dev,
 	int ret;
 	char newkey[5];
 	u8 buffer[17];
-	struct sensor_device_attribute_2 *sensor_attr =
-						to_sensor_dev_attr_2(attr);
 
-	newkey[0] = FAN_POSITION[0];
-	newkey[1] = '0' + sensor_attr->index;
-	newkey[2] = FAN_POSITION[2];
-	newkey[3] = FAN_POSITION[3];
-	newkey[4] = 0;
+	sprintf(newkey, FAN_ID_FMT, to_index(attr));
 
 	ret = applesmc_read_key(newkey, buffer, 16);
 	buffer[16] = 0;
@@ -1066,62 +1031,15 @@ static struct attribute *key_enumeration_attributes[] = {
 static const struct attribute_group key_enumeration_group =
 	{ .attrs = key_enumeration_attributes };
 
-/*
- * Macro defining SENSOR_DEVICE_ATTR for a fan sysfs entries.
- *  - show actual speed
- *  - show/store minimum speed
- *  - show maximum speed
- *  - show safe speed
- *  - show/store target speed
- *  - show/store manual mode
- */
-#define sysfs_fan_speeds_offset(offset) \
-static SENSOR_DEVICE_ATTR_2(fan##offset##_input, S_IRUGO, \
-			applesmc_show_fan_speed, NULL, 0, offset-1); \
-\
-static SENSOR_DEVICE_ATTR_2(fan##offset##_min, S_IRUGO | S_IWUSR, \
-	applesmc_show_fan_speed, applesmc_store_fan_speed, 1, offset-1); \
-\
-static SENSOR_DEVICE_ATTR_2(fan##offset##_max, S_IRUGO, \
-			applesmc_show_fan_speed, NULL, 2, offset-1); \
-\
-static SENSOR_DEVICE_ATTR_2(fan##offset##_safe, S_IRUGO, \
-			applesmc_show_fan_speed, NULL, 3, offset-1); \
-\
-static SENSOR_DEVICE_ATTR_2(fan##offset##_output, S_IRUGO | S_IWUSR, \
-	applesmc_show_fan_speed, applesmc_store_fan_speed, 4, offset-1); \
-\
-static SENSOR_DEVICE_ATTR(fan##offset##_manual, S_IRUGO | S_IWUSR, \
-	applesmc_show_fan_manual, applesmc_store_fan_manual, offset-1); \
-\
-static SENSOR_DEVICE_ATTR(fan##offset##_label, S_IRUGO, \
-	applesmc_show_fan_position, NULL, offset-1); \
-\
-static struct attribute *fan##offset##_attributes[] = { \
-	&sensor_dev_attr_fan##offset##_input.dev_attr.attr, \
-	&sensor_dev_attr_fan##offset##_min.dev_attr.attr, \
-	&sensor_dev_attr_fan##offset##_max.dev_attr.attr, \
-	&sensor_dev_attr_fan##offset##_safe.dev_attr.attr, \
-	&sensor_dev_attr_fan##offset##_output.dev_attr.attr, \
-	&sensor_dev_attr_fan##offset##_manual.dev_attr.attr, \
-	&sensor_dev_attr_fan##offset##_label.dev_attr.attr, \
-	NULL \
-};
-
-/*
- * Create the needed functions for each fan using the macro defined above
- * (4 fans are supported)
- */
-sysfs_fan_speeds_offset(1);
-sysfs_fan_speeds_offset(2);
-sysfs_fan_speeds_offset(3);
-sysfs_fan_speeds_offset(4);
-
-static const struct attribute_group fan_attribute_groups[] = {
-	{ .attrs = fan1_attributes },
-	{ .attrs = fan2_attributes },
-	{ .attrs = fan3_attributes },
-	{ .attrs = fan4_attributes },
+static struct applesmc_node_group fan_group[] = {
+	{ "fan%d_label", applesmc_show_fan_position },
+	{ "fan%d_input", applesmc_show_fan_speed, NULL, 0 },
+	{ "fan%d_min", applesmc_show_fan_speed, applesmc_store_fan_speed, 1 },
+	{ "fan%d_max", applesmc_show_fan_speed, NULL, 2 },
+	{ "fan%d_safe", applesmc_show_fan_speed, NULL, 3 },
+	{ "fan%d_output", applesmc_show_fan_speed, applesmc_store_fan_speed, 4 },
+	{ "fan%d_manual", applesmc_show_fan_manual, applesmc_store_fan_manual },
+	{ }
 };
 
 static struct applesmc_node_group temp_group[] = {
@@ -1168,7 +1086,7 @@ static int applesmc_create_nodes(struct applesmc_node_group *groups, int num)
 		for (i = 0; i < num; i++) {
 			node = &grp->nodes[i];
 			sprintf(node->name, grp->format, i + 1);
-			node->sda.index = i;
+			node->sda.index = (grp->option << 16) | (i & 0xffff);
 			node->sda.dev_attr.show = grp->show;
 			node->sda.dev_attr.store = grp->store;
 			attr = &node->sda.dev_attr.attr;
@@ -1286,7 +1204,6 @@ static __initdata struct dmi_system_id applesmc_whitelist[] = {
 static int __init applesmc_init(void)
 {
 	int ret;
-	int count;
 
 	if (!dmi_check_system(applesmc_whitelist)) {
 		pr_warn("supported laptop not found!\n");
@@ -1325,25 +1242,9 @@ static int __init applesmc_init(void)
 	if (ret)
 		goto out_name;
 
-	/* create fan files */
-	count = applesmc_get_fan_count();
-	if (count < 0)
-		pr_err("Cannot get the number of fans\n");
-	else
-		pr_info("%d fans found\n", count);
-
-	if (count > 4) {
-		count = 4;
-		pr_warn("A maximum of 4 fans are supported by this driver\n");
-	}
-
-	while (fans_handled < count) {
-		ret = sysfs_create_group(&pdev->dev.kobj,
-					 &fan_attribute_groups[fans_handled]);
-		if (ret)
-			goto out_fans;
-		fans_handled++;
-	}
+	ret = applesmc_create_nodes(fan_group, smcreg.fan_count);
+	if (ret)
+		goto out_info;
 
 	ret = applesmc_create_nodes(temp_group, smcreg.temp_count);
 	if (ret)
@@ -1401,9 +1302,8 @@ out_accelerometer:
 out_temperature:
 	applesmc_destroy_nodes(temp_group);
 out_fans:
-	while (fans_handled)
-		sysfs_remove_group(&pdev->dev.kobj,
-				   &fan_attribute_groups[--fans_handled]);
+	applesmc_destroy_nodes(fan_group);
+out_info:
 	sysfs_remove_group(&pdev->dev.kobj, &key_enumeration_group);
 out_name:
 	sysfs_remove_file(&pdev->dev.kobj, &dev_attr_name.attr);
@@ -1432,9 +1332,7 @@ static void __exit applesmc_exit(void)
 	if (smcreg.has_accelerometer)
 		applesmc_release_accelerometer();
 	applesmc_destroy_nodes(temp_group);
-	while (fans_handled)
-		sysfs_remove_group(&pdev->dev.kobj,
-				   &fan_attribute_groups[--fans_handled]);
+	applesmc_destroy_nodes(fan_group);
 	sysfs_remove_group(&pdev->dev.kobj, &key_enumeration_group);
 	sysfs_remove_file(&pdev->dev.kobj, &dev_attr_name.attr);
 	applesmc_destroy_smcreg();
-- 
1.7.1

Re: [lm-sensors] [PATCH 6/8] hwmon: applesmc: Dynamic creation of fan files

[Guenter Roeck]

On Sun, Oct 31, 2010 at 03:50:32AM -0400, Henrik Rydberg wrote:
> With the dynamic temperature group in place, the setup of fans
> can be simplified. This patch sets up the fans dynamically, removing
> a hundred lines of code.
> 
> Signed-off-by: Henrik Rydberg <rydberg@euromail.se>

Hi Henrik,

just one minor comment.

> ---
>  drivers/hwmon/applesmc.c |  188 +++++++++++-----------------------------------
>  1 files changed, 43 insertions(+), 145 deletions(-)
> 
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index abbff0b..4ba20ec 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
[ ... ]

>  static int applesmc_init_smcreg_try(void)
>  {
>         struct applesmc_registers *s = &smcreg;
> +       u8 tmp[4];

Why tmp[4] if you only read one byte ?

Guenter

Re: [lm-sensors] [PATCH 6/8] hwmon: applesmc: Dynamic creation of fan files

[Henrik Rydberg]

>>  static int applesmc_init_smcreg_try(void)

>>  {
>>         struct applesmc_registers *s = &smcreg;
>> +       u8 tmp[4];
> 
> Why tmp[4] if you only read one byte ?


Alignment optimization, perhaps? :-) I'll change it.

Thanks,
Henrik

[PATCH 7/8] hwmon: applesmc: Simplify feature sysfs handling

[Henrik Rydberg]

Given the dynamic node construction method, the setup of the
accelerometer, light sensor and keyboard backlight sysfs nodes
can be simplified. This patch does not contain any logic changes.

Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
---
 drivers/hwmon/applesmc.c |  168 ++++++++++++++++++++-------------------------
 1 files changed, 75 insertions(+), 93 deletions(-)

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 4ba20ec..c09cd2e 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -989,47 +989,27 @@ static struct led_classdev applesmc_backlight = {
 	.brightness_set		= applesmc_brightness_set,
 };
 
-static DEVICE_ATTR(name, 0444, applesmc_name_show, NULL);
-
-static DEVICE_ATTR(position, 0444, applesmc_position_show, NULL);
-static DEVICE_ATTR(calibrate, 0644,
-			applesmc_calibrate_show, applesmc_calibrate_store);
-
-static struct attribute *accelerometer_attributes[] = {
-	&dev_attr_position.attr,
-	&dev_attr_calibrate.attr,
-	NULL
+static struct applesmc_node_group info_group[] = {
+	{ "name", applesmc_name_show },
+	{ "key_count", applesmc_key_count_show },
+	{ "key_at_index", applesmc_key_at_index_show, applesmc_key_at_index_store },
+	{ "key_at_index_name", applesmc_key_at_index_name_show },
+	{ "key_at_index_type", applesmc_key_at_index_type_show },
+	{ "key_at_index_data_length", applesmc_key_at_index_data_length_show },
+	{ "key_at_index_data", applesmc_key_at_index_read_show },
+	{ }
 };
 
-static const struct attribute_group accelerometer_attributes_group =
-	{ .attrs = accelerometer_attributes };
-
-static DEVICE_ATTR(light, 0444, applesmc_light_show, NULL);
-
-static DEVICE_ATTR(key_count, 0444, applesmc_key_count_show, NULL);
-static DEVICE_ATTR(key_at_index, 0644,
-		applesmc_key_at_index_show, applesmc_key_at_index_store);
-static DEVICE_ATTR(key_at_index_name, 0444,
-					applesmc_key_at_index_name_show, NULL);
-static DEVICE_ATTR(key_at_index_type, 0444,
-					applesmc_key_at_index_type_show, NULL);
-static DEVICE_ATTR(key_at_index_data_length, 0444,
-				applesmc_key_at_index_data_length_show, NULL);
-static DEVICE_ATTR(key_at_index_data, 0444,
-				applesmc_key_at_index_read_show, NULL);
-
-static struct attribute *key_enumeration_attributes[] = {
-	&dev_attr_key_count.attr,
-	&dev_attr_key_at_index.attr,
-	&dev_attr_key_at_index_name.attr,
-	&dev_attr_key_at_index_type.attr,
-	&dev_attr_key_at_index_data_length.attr,
-	&dev_attr_key_at_index_data.attr,
-	NULL
+static struct applesmc_node_group accelerometer_group[] = {
+	{ "position", applesmc_position_show },
+	{ "calibrate", applesmc_calibrate_show, applesmc_calibrate_store },
+	{ }
 };
 
-static const struct attribute_group key_enumeration_group =
-	{ .attrs = key_enumeration_attributes };
+static struct applesmc_node_group light_sensor_group[] = {
+	{ "light", applesmc_light_show },
+	{ }
+};
 
 static struct applesmc_node_group fan_group[] = {
 	{ "fan%d_label", applesmc_show_fan_position },
@@ -1112,8 +1092,10 @@ static int applesmc_create_accelerometer(void)
 	struct input_dev *idev;
 	int ret;
 
-	ret = sysfs_create_group(&pdev->dev.kobj,
-					&accelerometer_attributes_group);
+	if (!smcreg.has_accelerometer)
+		return 0;
+
+	ret = applesmc_create_nodes(accelerometer_group, 1);
 	if (ret)
 		goto out;
 
@@ -1150,7 +1132,7 @@ out_idev:
 	input_free_polled_device(applesmc_idev);
 
 out_sysfs:
-	sysfs_remove_group(&pdev->dev.kobj, &accelerometer_attributes_group);
+	applesmc_destroy_nodes(accelerometer_group);
 
 out:
 	pr_warn("driver init failed (ret=%d)!\n", ret);
@@ -1160,9 +1142,43 @@ out:
 /* Release all ressources used by the accelerometer */
 static void applesmc_release_accelerometer(void)
 {
+	if (!smcreg.has_accelerometer)
+		return;
 	input_unregister_polled_device(applesmc_idev);
 	input_free_polled_device(applesmc_idev);
-	sysfs_remove_group(&pdev->dev.kobj, &accelerometer_attributes_group);
+	applesmc_destroy_nodes(accelerometer_group);
+}
+
+static int applesmc_create_light_sensor(void)
+{
+	if (!smcreg.has_left_light)
+		return 0;
+	return applesmc_create_nodes(light_sensor_group, 1);
+}
+
+static void applesmc_release_light_sensor(void)
+{
+	if (!smcreg.has_left_light)
+		return;
+	applesmc_destroy_nodes(light_sensor_group);
+}
+
+static int applesmc_create_key_backlight(void)
+{
+	if (!smcreg.has_key_light)
+		return 0;
+	applesmc_led_wq = create_singlethread_workqueue("applesmc-led");
+	if (!applesmc_led_wq)
+		return -ENOMEM;
+	return led_classdev_register(&pdev->dev, &applesmc_backlight);
+}
+
+static void applesmc_release_key_backlight(void)
+{
+	if (!smcreg.has_key_light)
+		return;
+	led_classdev_unregister(&applesmc_backlight);
+	destroy_workqueue(applesmc_led_wq);
 }
 
 static int applesmc_dmi_match(const struct dmi_system_id *id)
@@ -1233,15 +1249,10 @@ static int __init applesmc_init(void)
 	if (ret)
 		goto out_device;
 
-	ret = sysfs_create_file(&pdev->dev.kobj, &dev_attr_name.attr);
+	ret = applesmc_create_nodes(info_group, 1);
 	if (ret)
 		goto out_smcreg;
 
-	/* Create key enumeration sysfs files */
-	ret = sysfs_create_group(&pdev->dev.kobj, &key_enumeration_group);
-	if (ret)
-		goto out_name;
-
 	ret = applesmc_create_nodes(fan_group, smcreg.fan_count);
 	if (ret)
 		goto out_info;
@@ -1250,32 +1261,17 @@ static int __init applesmc_init(void)
 	if (ret)
 		goto out_fans;
 
-	if (smcreg.has_accelerometer) {
-		ret = applesmc_create_accelerometer();
-		if (ret)
-			goto out_temperature;
-	}
-
-	if (smcreg.has_left_light) {
-		/* Add light sensor file */
-		ret = sysfs_create_file(&pdev->dev.kobj, &dev_attr_light.attr);
-		if (ret)
-			goto out_accelerometer;
-	}
+	ret = applesmc_create_accelerometer();
+	if (ret)
+		goto out_temperature;
 
-	if (smcreg.has_key_light) {
-		/* Create the workqueue */
-		applesmc_led_wq = create_singlethread_workqueue("applesmc-led");
-		if (!applesmc_led_wq) {
-			ret = -ENOMEM;
-			goto out_light_sysfs;
-		}
+	ret = applesmc_create_light_sensor();
+	if (ret)
+		goto out_accelerometer;
 
-		/* register as a led device */
-		ret = led_classdev_register(&pdev->dev, &applesmc_backlight);
-		if (ret < 0)
-			goto out_light_wq;
-	}
+	ret = applesmc_create_key_backlight();
+	if (ret)
+		goto out_light_sysfs;
 
 	hwmon_dev = hwmon_device_register(&pdev->dev);
 	if (IS_ERR(hwmon_dev)) {
@@ -1288,25 +1284,17 @@ static int __init applesmc_init(void)
 	return 0;
 
 out_light_ledclass:
-	if (smcreg.has_key_light)
-		led_classdev_unregister(&applesmc_backlight);
-out_light_wq:
-	if (smcreg.has_key_light)
-		destroy_workqueue(applesmc_led_wq);
+	applesmc_release_key_backlight();
 out_light_sysfs:
-	if (smcreg.has_left_light)
-		sysfs_remove_file(&pdev->dev.kobj, &dev_attr_light.attr);
+	applesmc_release_light_sensor();
 out_accelerometer:
-	if (smcreg.has_accelerometer)
-		applesmc_release_accelerometer();
+	applesmc_release_accelerometer();
 out_temperature:
 	applesmc_destroy_nodes(temp_group);
 out_fans:
 	applesmc_destroy_nodes(fan_group);
 out_info:
-	sysfs_remove_group(&pdev->dev.kobj, &key_enumeration_group);
-out_name:
-	sysfs_remove_file(&pdev->dev.kobj, &dev_attr_name.attr);
+	applesmc_destroy_nodes(info_group);
 out_smcreg:
 	applesmc_destroy_smcreg();
 out_device:
@@ -1323,18 +1311,12 @@ out:
 static void __exit applesmc_exit(void)
 {
 	hwmon_device_unregister(hwmon_dev);
-	if (smcreg.has_key_light) {
-		led_classdev_unregister(&applesmc_backlight);
-		destroy_workqueue(applesmc_led_wq);
-	}
-	if (smcreg.has_left_light)
-		sysfs_remove_file(&pdev->dev.kobj, &dev_attr_light.attr);
-	if (smcreg.has_accelerometer)
-		applesmc_release_accelerometer();
+	applesmc_release_key_backlight();
+	applesmc_release_light_sensor();
+	applesmc_release_accelerometer();
 	applesmc_destroy_nodes(temp_group);
 	applesmc_destroy_nodes(fan_group);
-	sysfs_remove_group(&pdev->dev.kobj, &key_enumeration_group);
-	sysfs_remove_file(&pdev->dev.kobj, &dev_attr_name.attr);
+	applesmc_destroy_nodes(info_group);
 	applesmc_destroy_smcreg();
 	platform_device_unregister(pdev);
 	platform_driver_unregister(&applesmc_driver);
-- 
1.7.1

Re: [lm-sensors] [PATCH 7/8] hwmon: applesmc: Simplify feature sysfs handling

[Guenter Roeck]

On Sun, Oct 31, 2010 at 03:50:33AM -0400, Henrik Rydberg wrote:
> Given the dynamic node construction method, the setup of the
> accelerometer, light sensor and keyboard backlight sysfs nodes
> can be simplified. This patch does not contain any logic changes.
> 
> Signed-off-by: Henrik Rydberg <rydberg@euromail.se>

Hi Henrik,

only one minor comment.

> ---
>  drivers/hwmon/applesmc.c |  168 ++++++++++++++++++++-------------------------
>  1 files changed, 75 insertions(+), 93 deletions(-)
> 
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index 4ba20ec..c09cd2e 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c

[ ... ]

> +static struct applesmc_node_group info_group[] = {
> +	{ "name", applesmc_name_show },
> +	{ "key_count", applesmc_key_count_show },
> +	{ "key_at_index", applesmc_key_at_index_show, applesmc_key_at_index_store },

Finally a checkpatch warning - line longer than 80 columns.

Guenter

[PATCH 8/8] hwmon: applesmc: Update copyright information

[Henrik Rydberg]

With the preceding patches, git blame assigns about half of
the file to the present author. Add a line to the copyright
to reflect this.

Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
---
 drivers/hwmon/applesmc.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index c09cd2e..7ae37eb 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -4,6 +4,7 @@
  * computers.
  *
  * Copyright (C) 2007 Nicolas Boichat <nicolas@boichat.ch>
+ * Copyright (C) 2010 Henrik Rydberg <rydberg@euromail.se>
  *
  * Based on hdaps.c driver:
  * Copyright (C) 2005 Robert Love <rml@novell.com>
-- 
1.7.1

Re: [lm-sensors] [PATCH 8/8] hwmon: applesmc: Update copyright information

[Guenter Roeck]

On Sun, Oct 31, 2010 at 03:50:34AM -0400, Henrik Rydberg wrote:
> With the preceding patches, git blame assigns about half of
> the file to the present author. Add a line to the copyright
> to reflect this.
> 
> Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
> ---
>  drivers/hwmon/applesmc.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index c09cd2e..7ae37eb 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
> @@ -4,6 +4,7 @@
>   * computers.
>   *
>   * Copyright (C) 2007 Nicolas Boichat <nicolas@boichat.ch>
> + * Copyright (C) 2010 Henrik Rydberg <rydberg@euromail.se>
>   *
>   * Based on hdaps.c driver:
>   * Copyright (C) 2005 Robert Love <rml@novell.com>
> -- 


Hi Henrik,

overall excellent cleanup. 

One suggestion - since you did that much rework, it would be nice if you could fix
the remaining few checkpatch errors in the file as well (and possibly the warnings).

Guenter

Re: [lm-sensors] [PATCH 8/8] hwmon: applesmc: Update copyright information

[Henrik Rydberg]

>

> One suggestion - since you did that much rework, it would be nice if you could fix
> the remaining few checkpatch errors in the file as well (and possibly the warnings).


Absolutely. Regarding the warnings, I was deliberately leaving the over-80 lines
since they appear in static array listings, and as such seem to be pretty common
in the kernel. I know these things have been discussed before. :-)

Cheers,
Henrik

Re: [lm-sensors] [PATCH 8/8] hwmon: applesmc: Update copyright information

[Guenter Roeck]

On Fri, Nov 05, 2010 at 05:00:05AM -0400, Henrik Rydberg wrote:
> >
> 
> > One suggestion - since you did that much rework, it would be nice if you could fix
> > the remaining few checkpatch errors in the file as well (and possibly the warnings).
> 
> 
> Absolutely. Regarding the warnings, I was deliberately leaving the over-80 lines
> since they appear in static array listings, and as such seem to be pretty common
> in the kernel. I know these things have been discussed before. :-)
> 
Ok, no problem with me.

Guenter

Re: [PATCH 0/8] hwmon: applesmc: Dynamic configuration rewrite

[Joe Perches]

On Sun, 2010-10-31 at 08:50 +0100, Henrik Rydberg wrote:
> This patchset introduces dynamic configuration by making the SMC
> register set efficiently searchable, and fixes a number of outstanding
> issues:

Hi Henrik.

Your patches are built on top of a patch I submitted
that wasn't accepted by Jean or Guenter.

Re: [PATCH 0/8] hwmon: applesmc: Dynamic configuration rewrite

[Henrik Rydberg]

On 10/31/2010 09:31 AM, Joe Perches wrote:

> On Sun, 2010-10-31 at 08:50 +0100, Henrik Rydberg wrote:
>> This patchset introduces dynamic configuration by making the SMC
>> register set efficiently searchable, and fixes a number of outstanding
>> issues:
> 
> Hi Henrik.
> 
> Your patches are built on top of a patch I submitted
> that wasn't accepted by Jean or Guenter.


Right. I imagine there will be new revisions of that patch. If we are all
targeting 2.6.38, I can certainly rebase this set back to the current head, if
that is preferrable. Jean?

Thanks,
Henrik

Re: [PATCH 0/8] hwmon: applesmc: Dynamic configuration rewrite

[Joe Perches]

On Sun, 2010-10-31 at 09:44 +0100, Henrik Rydberg wrote:
> On 10/31/2010 09:31 AM, Joe Perches wrote:
> > On Sun, 2010-10-31 at 08:50 +0100, Henrik Rydberg wrote:
> >> This patchset introduces dynamic configuration by making the SMC
> >> register set efficiently searchable, and fixes a number of outstanding
> >> issues:
> > Your patches are built on top of a patch I submitted
> > that wasn't accepted by Jean or Guenter.
> Right. I imagine there will be new revisions of that patch.

Maybe, but probably not by me.

Jean and I have different opinions on how quickly
what he views as a necessary kernel-wide precursor
patch might be applied.

Re: [PATCH 0/8] hwmon: applesmc: Dynamic configuration rewrite

[Jean Delvare]

On Sun, 31 Oct 2010 09:44:42 +0100, Henrik Rydberg wrote:
> On 10/31/2010 09:31 AM, Joe Perches wrote:
> 
> > On Sun, 2010-10-31 at 08:50 +0100, Henrik Rydberg wrote:
> >> This patchset introduces dynamic configuration by making the SMC
> >> register set efficiently searchable, and fixes a number of outstanding
> >> issues:
> > 
> > Hi Henrik.
> > 
> > Your patches are built on top of a patch I submitted
> > that wasn't accepted by Jean or Guenter.
> 
> 
> Right. I imagine there will be new revisions of that patch. If we are all
> targeting 2.6.38, I can certainly rebase this set back to the current head, if
> that is preferrable. Jean?

Hendrik, Joe, please give me a few days to finish urgent pending work,
and I'll get back to you regarding both issues. I'm sure we can sort it
all out :)


-- 
Jean Delvare

Re: [lm-sensors] [PATCH 0/8] hwmon: applesmc: Dynamic configuration rewrite

[Guenter Roeck]

On Sun, Oct 31, 2010 at 03:50:26AM -0400, Henrik Rydberg wrote:
> The amount of machines supported by this driver has increased rapidly
> over the past years, and the detailed driver support of the many
> flavors is quickly becoming a problem.
> 
> This patchset introduces dynamic configuration by making the SMC
> register set efficiently searchable, and fixes a number of outstanding
> issues:
> 
> * Support for all new models since MacbookPro 6.
> * Support for variable number of cpu sensors in the MacPros.
> * Handles the odd temperature sensor types found on new models.
> * Fixes minor read problems on many intermediate models.
> 
> The first patch is preparatory. The second patch adds the register
> lookup cache. The third patch adds the dynamic temperature files, and
> the fourth implements a new temperature register format. The fifth
> patch makes the remaining features dynamically configured, and the
> sixth patch sets up the fans dynamically. The seventh patch is purely
> janitory, and the last patch updates the copyright.
> 
> As a side-effect, the file is reduced by almost five hundred lines.
> 
Henrik,

since you applied your changes on top of Joe's pr_fmt patch, I assume
that you Ack that patch. Is that a correct assumption ?

I know you sent an Ack earlier, but it was "except for comments already made",
so this is just for clarification.

Thanks,
Guenter

Re: [lm-sensors] [PATCH 0/8] hwmon: applesmc: Dynamic configuration rewrite

[Henrik Rydberg]

On 11/03/2010 02:48 PM, Guenter Roeck wrote:

> On Sun, Oct 31, 2010 at 03:50:26AM -0400, Henrik Rydberg wrote:
>> The amount of machines supported by this driver has increased rapidly
>> over the past years, and the detailed driver support of the many
>> flavors is quickly becoming a problem.
>>
>> This patchset introduces dynamic configuration by making the SMC
>> register set efficiently searchable, and fixes a number of outstanding
>> issues:
>>
>> * Support for all new models since MacbookPro 6.
>> * Support for variable number of cpu sensors in the MacPros.
>> * Handles the odd temperature sensor types found on new models.
>> * Fixes minor read problems on many intermediate models.
>>
>> The first patch is preparatory. The second patch adds the register
>> lookup cache. The third patch adds the dynamic temperature files, and
>> the fourth implements a new temperature register format. The fifth
>> patch makes the remaining features dynamically configured, and the
>> sixth patch sets up the fans dynamically. The seventh patch is purely
>> janitory, and the last patch updates the copyright.
>>
>> As a side-effect, the file is reduced by almost five hundred lines.
>>
> Henrik,
> 
> since you applied your changes on top of Joe's pr_fmt patch, I assume
> that you Ack that patch. Is that a correct assumption ?
> 
> I know you sent an Ack earlier, but it was "except for comments already made",
> so this is just for clarification.


Yeah, I'm good with those changes. As far as I understand, the issue was
regarding the extra define at the top, not the changes per se. I can certainly
live with that extra line until something better comes along.

Thanks,
Henrik