[PATCH 1/2] hwmon: (applesmc) Allow negative temperature values

[PATCH 1/2] hwmon: (applesmc) Allow negative temperature values

[Henrik Rydberg]

There are many userland reports of sensors with unreasonably small and
large temperatures. There seem to be several reasons for this:

Firstly, the major sensor type (sp78) is actually a signed number.
This explains why some sensors show very small or large values - they
are in fact all small, but of different sign.

Secondly, the other sensor type (1-hex) is not properly understood; it
may be that it is not a temperature after all.

Thirdly, some sensors are differential in nature, showing changes over
time rather than absolute numbers.  This explains why those values are
small and of varying sign.

This patch interprets the sp78 type as signed short, but keeps the
original scaling. For other types, -EINVAL is returned, since the
nature of those sensors is unknown.

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

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index f41585e..75f87f1 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -80,6 +80,8 @@
 #define FANS_MANUAL		"FS! " /* r-w ui16 */
 #define FAN_ID_FMT		"F%dID" /* r-o char[16] */
 
+#define TEMP_SENSOR_TYPE	"sp78"
+
 /* List of keys used to read/write fan speeds */
 static const char *const fan_speed_fmt[] = {
 	"F%dAc",		/* actual speed */
@@ -720,27 +722,22 @@ static ssize_t applesmc_show_temperature(struct device *dev,
 	int index = smcreg.temp_begin + to_index(devattr);
 	const struct applesmc_entry *entry;
 	int ret;
-	u8 buffer[2];
-	unsigned int temp;
+	s16 value;
+	int temp;
 
 	entry = applesmc_get_entry_by_index(index);
 	if (IS_ERR(entry))
 		return PTR_ERR(entry);
-	if (entry->len > 2)
+	if (strcmp(entry->type, TEMP_SENSOR_TYPE))
 		return -EINVAL;
 
-	ret = applesmc_read_entry(entry, buffer, entry->len);
+	ret = applesmc_read_s16(entry->key, &value);
 	if (ret)
 		return ret;
 
-	if (entry->len == 2) {
-		temp = buffer[0] * 1000;
-		temp += (buffer[1] >> 6) * 250;
-	} else {
-		temp = buffer[0] * 4000;
-	}
+	temp = 250 * (value >> 6);
 
-	return snprintf(sysfsbuf, PAGE_SIZE, "%u\n", temp);
+	return snprintf(sysfsbuf, PAGE_SIZE, "%d\n", temp);
 }
 
 static ssize_t applesmc_show_fan_speed(struct device *dev,
-- 
1.7.11.2

[PATCH 2/2] hwmon: (applesmc) Ignore some temperature registers

[Henrik Rydberg]

Not all sensors in the T range are useful temperatures.  This patch
creates a subset of sensors to be exported to userland, excluding the
unknown types.

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

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 75f87f1..4d937a1 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -133,11 +133,13 @@ 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 */
+	unsigned int index_count;	/* size of temperature index array */
 	int num_light_sensors;		/* number of light sensors */
 	bool has_accelerometer;		/* has motion sensor */
 	bool has_key_backlight;		/* has keyboard backlight */
 	bool init_complete;		/* true when fully initialized */
 	struct applesmc_entry *cache;	/* cached key entries */
+	const char **index;		/* temperature key index */
 } smcreg = {
 	.mutex = __MUTEX_INITIALIZER(smcreg.mutex),
 };
@@ -469,6 +471,30 @@ static void applesmc_device_init(void)
 	pr_warn("failed to init the device\n");
 }
 
+static int applesmc_init_index(struct applesmc_registers *s)
+{
+	const struct applesmc_entry *entry;
+	unsigned int i;
+
+	if (s->index)
+		return 0;
+
+	s->index = kcalloc(s->temp_count, sizeof(s->index[0]), GFP_KERNEL);
+	if (!s->index)
+		return -ENOMEM;
+
+	for (i = s->temp_begin; i < s->temp_end; i++) {
+		entry = applesmc_get_entry_by_index(i);
+		if (IS_ERR(entry))
+			continue;
+		if (strcmp(entry->type, TEMP_SENSOR_TYPE))
+			continue;
+		s->index[s->index_count++] = entry->key;
+	}
+
+	return 0;
+}
+
 /*
  * applesmc_init_smcreg_try - Try to initialize register cache. Idempotent.
  */
@@ -504,6 +530,10 @@ static int applesmc_init_smcreg_try(void)
 		return ret;
 	s->temp_count = s->temp_end - s->temp_begin;
 
+	ret = applesmc_init_index(s);
+	if (ret)
+		return ret;
+
 	ret = applesmc_has_key(LIGHT_SENSOR_LEFT_KEY, &left_light_sensor);
 	if (ret)
 		return ret;
@@ -520,8 +550,8 @@ static int applesmc_init_smcreg_try(void)
 	s->num_light_sensors = left_light_sensor + right_light_sensor;
 	s->init_complete = true;
 
-	pr_info("key=%d fan=%d temp=%d acc=%d lux=%d kbd=%d\n",
-	       s->key_count, s->fan_count, s->temp_count,
+	pr_info("key=%d fan=%d temp=%d index=%d acc=%d lux=%d kbd=%d\n",
+	       s->key_count, s->fan_count, s->temp_count, s->index_count,
 	       s->has_accelerometer,
 	       s->num_light_sensors,
 	       s->has_key_backlight);
@@ -529,6 +559,15 @@ static int applesmc_init_smcreg_try(void)
 	return 0;
 }
 
+static void applesmc_destroy_smcreg(void)
+{
+	kfree(smcreg.index);
+	smcreg.index = NULL;
+	kfree(smcreg.cache);
+	smcreg.cache = NULL;
+	smcreg.init_complete = false;
+}
+
 /*
  * applesmc_init_smcreg - Initialize register cache.
  *
@@ -549,19 +588,11 @@ static int applesmc_init_smcreg(void)
 		msleep(INIT_WAIT_MSECS);
 	}
 
-	kfree(smcreg.cache);
-	smcreg.cache = NULL;
+	applesmc_destroy_smcreg();
 
 	return ret;
 }
 
-static void applesmc_destroy_smcreg(void)
-{
-	kfree(smcreg.cache);
-	smcreg.cache = NULL;
-	smcreg.init_complete = false;
-}
-
 /* Device model stuff */
 static int applesmc_probe(struct platform_device *dev)
 {
@@ -705,33 +736,21 @@ out:
 static ssize_t applesmc_show_sensor_label(struct device *dev,
 			struct device_attribute *devattr, char *sysfsbuf)
 {
-	int index = smcreg.temp_begin + to_index(devattr);
-	const struct applesmc_entry *entry;
+	const char *key = smcreg.index[to_index(devattr)];
 
-	entry = applesmc_get_entry_by_index(index);
-	if (IS_ERR(entry))
-		return PTR_ERR(entry);
-
-	return snprintf(sysfsbuf, PAGE_SIZE, "%s\n", entry->key);
+	return snprintf(sysfsbuf, PAGE_SIZE, "%s\n", 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);
-	const struct applesmc_entry *entry;
+	const char *key = smcreg.index[to_index(devattr)];
 	int ret;
 	s16 value;
 	int temp;
 
-	entry = applesmc_get_entry_by_index(index);
-	if (IS_ERR(entry))
-		return PTR_ERR(entry);
-	if (strcmp(entry->type, TEMP_SENSOR_TYPE))
-		return -EINVAL;
-
-	ret = applesmc_read_s16(entry->key, &value);
+	ret = applesmc_read_s16(key, &value);
 	if (ret)
 		return ret;
 
@@ -1247,7 +1266,7 @@ static int __init applesmc_init(void)
 	if (ret)
 		goto out_info;
 
-	ret = applesmc_create_nodes(temp_group, smcreg.temp_count);
+	ret = applesmc_create_nodes(temp_group, smcreg.index_count);
 	if (ret)
 		goto out_fans;
 
-- 
1.7.11.2

Re: [lm-sensors] [PATCH 2/2] hwmon: (applesmc) Ignore some temperature registers

[Guenter Roeck]

On Mon, Jul 16, 2012 at 09:18:11AM +0200, Henrik Rydberg wrote:
> Not all sensors in the T range are useful temperatures.  This patch
> creates a subset of sensors to be exported to userland, excluding the
> unknown types.
> 
> Signed-off-by: Henrik Rydberg <rydberg@euromail.se>

Applied to -next.

Thanks,
Guenter

Re: [lm-sensors] [PATCH 1/2] hwmon: (applesmc) Allow negative temperature values

[Guenter Roeck]

On Mon, Jul 16, 2012 at 09:18:10AM +0200, Henrik Rydberg wrote:
> There are many userland reports of sensors with unreasonably small and
> large temperatures. There seem to be several reasons for this:
> 
> Firstly, the major sensor type (sp78) is actually a signed number.
> This explains why some sensors show very small or large values - they
> are in fact all small, but of different sign.
> 
> Secondly, the other sensor type (1-hex) is not properly understood; it
> may be that it is not a temperature after all.
> 
> Thirdly, some sensors are differential in nature, showing changes over
> time rather than absolute numbers.  This explains why those values are
> small and of varying sign.
> 
> This patch interprets the sp78 type as signed short, but keeps the
> original scaling. For other types, -EINVAL is returned, since the
> nature of those sensors is unknown.
> 
> Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
> ---

Applied to -next.

Thanks,
Guenter