All of lore.kernel.org
 help / color / mirror / Atom feed
From: Ken Milmore <ken@kenm.demon.co.uk>
To: lm-sensors@vger.kernel.org
Subject: [lm-sensors] [PATCH] hwmon: (asc7621) Bug fixes
Date: Sat, 01 May 2010 13:08:20 +0000	[thread overview]
Message-ID: <4BDC27C4.2000205@kenm.demon.co.uk> (raw)

[-- Attachment #1: Type: text/plain, Size: 1090 bytes --]

Jean et al,

Find patch attached; this should apply against Linus' 2.6 tree. The 
issues fixed here have all been discussed before on the lm_sensors list 
(see 07/2008 and 04/2010), so hopefully they are uncontroversial.

Having taken a brief look at the PWM control inteface in the driver, I 
can see there are a few rough edges there but it is probably too late to 
address them now.  In particular the pwm[1-3]_enable and 
pwm[1-3]_auto_channels settings tend to interpret "Fan control disabled" 
as "Fan off", which doesn't accord with the principle of least surprise, 
or with the sysfs-interface documentation.  IMHO "Fan full on" would 
have been a much safer default for these settings...

Kind regards,

Ken.


On 28/04/10 13:32, Jean Delvare wrote:
> On Wed, 17 Mar 2010 08:39:17 +0100, Jean Delvare wrote:
>> Hi Ken,
>>
>> On Sun, 07 Mar 2010 10:20:13 +0000, Ken Milmore wrote:
>>> I will try and get a patch together as soon as time permits.
>>
>> Do you have a patch by now?
>
> Ping Ken. Would be nice to have a patch ready soon so that it can make
> it into kernel 2.6.35.
>

[-- Attachment #2: 0001-hwmon-asc7621-Bug-fixes.patch --]
[-- Type: text/plain, Size: 6975 bytes --]

From 5a9cc351ca11375a81082a06212ac45ac900b213 Mon Sep 17 00:00:00 2001
From: Ken Milmore <ken.milmore@googlemail.com>
Date: Sat, 1 May 2010 13:26:41 +0100
Subject: [PATCH] hwmon: (asc7621) Bug fixes

* Allow fan minimum RPM to be set to zero without triggering alarms.
* Fix voltage scaling arithmetic and correct scale factors.
* Correct fan1-fan4 alarm bit shifts.
* Correct register address for temp3_smoothing_enable.
* Read the alarm registers with high priority.

Signed-off-by: Ken Milmore <ken.milmore@googlemail.com>
---
 drivers/hwmon/asc7621.c |   63 +++++++++++++++++++++++------------------------
 1 files changed, 31 insertions(+), 32 deletions(-)

diff --git a/drivers/hwmon/asc7621.c b/drivers/hwmon/asc7621.c
index 7f94810..0f388ad 100644
--- a/drivers/hwmon/asc7621.c
+++ b/drivers/hwmon/asc7621.c
@@ -268,8 +268,11 @@ static ssize_t store_fan16(struct device *dev,
 	if (strict_strtol(buf, 10, &reqval))
 		return -EINVAL;
 
+	/* If a minimum RPM of zero is requested, then we set the register to
+	   0xffff. This value allows the fan to be stopped completely without
+	   generating an alarm. */
 	reqval =
-	    (SENSORS_LIMIT((reqval) <= 0 ? 0 : 5400000 / (reqval), 0, 65534));
+	    (reqval <= 0 ? 0xffff : SENSORS_LIMIT(5400000 / reqval, 0, 0xfffe));
 
 	mutex_lock(&data->update_lock);
 	data->reg[param->msb[0]] = (reqval >> 8) & 0xff;
@@ -285,8 +288,9 @@ static ssize_t store_fan16(struct device *dev,
  * Voltages are scaled in the device so that the nominal voltage
  * is 3/4ths of the 0-255 range (i.e. 192).
  * If all voltages are 'normal' then all voltage registers will
- * read 0xC0.  This doesn't help us if we don't have a point of refernce.
- * The data sheet however provides us with the full scale value for each
+ * read 0xC0.
+ *
+ * The data sheet provides us with the 3/4 scale value for each voltage
  * which is stored in in_scaling.  The sda->index parameter value provides
  * the index into in_scaling.
  *
@@ -295,7 +299,7 @@ static ssize_t store_fan16(struct device *dev,
  */
 
 static int asc7621_in_scaling[] = {
-	3320, 3000, 4380, 6640, 16000
+	2500, 2250, 3300, 5000, 12000
 };
 
 static ssize_t show_in10(struct device *dev, struct device_attribute *attr,
@@ -306,19 +310,12 @@ static ssize_t show_in10(struct device *dev, struct device_attribute *attr,
 	u8 nr = sda->index;
 
 	mutex_lock(&data->update_lock);
-	regval = (data->reg[param->msb[0]] * asc7621_in_scaling[nr]) / 256;
-
-	/* The LSB value is a 2-bit scaling of the MSB's LSbit value.
-	 * I.E.  If the maximim voltage for this input is 6640 millivolts then
-	 * a MSB register value of 0 = 0mv and 255 = 6640mv.
-	 * A 1 step change therefore represents 25.9mv (6640 / 256).
-	 * The extra 2-bits therefore represent increments of 6.48mv.
-	 */
-	regval += ((asc7621_in_scaling[nr] / 256) / 4) *
-	    (data->reg[param->lsb[0]] >> 6);
-
+	regval = (data->reg[param->msb[0]] << 8) | (data->reg[param->lsb[0]]);
 	mutex_unlock(&data->update_lock);
 
+	/* The LSB value is a 2-bit scaling of the MSB's LSbit value. */
+	regval = (regval >> 6) * asc7621_in_scaling[nr] / (0xc0 << 2);
+
 	return sprintf(buf, "%u\n", regval);
 }
 
@@ -331,7 +328,7 @@ static ssize_t show_in8(struct device *dev, struct device_attribute *attr,
 
 	return sprintf(buf, "%u\n",
 		       ((data->reg[param->msb[0]] *
-			 asc7621_in_scaling[nr]) / 256));
+			 asc7621_in_scaling[nr]) / 0xc0));
 }
 
 static ssize_t store_in8(struct device *dev, struct device_attribute *attr,
@@ -344,9 +341,11 @@ static ssize_t store_in8(struct device *dev, struct device_attribute *attr,
 	if (strict_strtol(buf, 10, &reqval))
 		return -EINVAL;
 
-	reqval = SENSORS_LIMIT(reqval, 0, asc7621_in_scaling[nr]);
+	reqval = SENSORS_LIMIT(reqval, 0, 0xffff);
+
+	reqval = reqval * 0xc0 / asc7621_in_scaling[nr];
 
-	reqval = (reqval * 255 + 128) / asc7621_in_scaling[nr];
+	reqval = SENSORS_LIMIT(reqval, 0, 0xff);
 
 	mutex_lock(&data->update_lock);
 	data->reg[param->msb[0]] = reqval;
@@ -846,11 +845,11 @@ static struct asc7621_param asc7621_params[] = {
 	PWRITE(in3_max, 3, PRI_LOW, 0x4b, 0, 0, 0, in8),
 	PWRITE(in4_max, 4, PRI_LOW, 0x4d, 0, 0, 0, in8),
 
-	PREAD(in0_alarm, 0, PRI_LOW, 0x41, 0, 0x01, 0, bitmask),
-	PREAD(in1_alarm, 1, PRI_LOW, 0x41, 0, 0x01, 1, bitmask),
-	PREAD(in2_alarm, 2, PRI_LOW, 0x41, 0, 0x01, 2, bitmask),
-	PREAD(in3_alarm, 3, PRI_LOW, 0x41, 0, 0x01, 3, bitmask),
-	PREAD(in4_alarm, 4, PRI_LOW, 0x42, 0, 0x01, 0, bitmask),
+	PREAD(in0_alarm, 0, PRI_HIGH, 0x41, 0, 0x01, 0, bitmask),
+	PREAD(in1_alarm, 1, PRI_HIGH, 0x41, 0, 0x01, 1, bitmask),
+	PREAD(in2_alarm, 2, PRI_HIGH, 0x41, 0, 0x01, 2, bitmask),
+	PREAD(in3_alarm, 3, PRI_HIGH, 0x41, 0, 0x01, 3, bitmask),
+	PREAD(in4_alarm, 4, PRI_HIGH, 0x42, 0, 0x01, 0, bitmask),
 
 	PREAD(fan1_input, 0, PRI_HIGH, 0x29, 0x28, 0, 0, fan16),
 	PREAD(fan2_input, 1, PRI_HIGH, 0x2b, 0x2a, 0, 0, fan16),
@@ -862,10 +861,10 @@ static struct asc7621_param asc7621_params[] = {
 	PWRITE(fan3_min, 2, PRI_LOW, 0x59, 0x58, 0, 0, fan16),
 	PWRITE(fan4_min, 3, PRI_LOW, 0x5b, 0x5a, 0, 0, fan16),
 
-	PREAD(fan1_alarm, 0, PRI_LOW, 0x42, 0, 0x01, 0, bitmask),
-	PREAD(fan2_alarm, 1, PRI_LOW, 0x42, 0, 0x01, 1, bitmask),
-	PREAD(fan3_alarm, 2, PRI_LOW, 0x42, 0, 0x01, 2, bitmask),
-	PREAD(fan4_alarm, 3, PRI_LOW, 0x42, 0, 0x01, 3, bitmask),
+	PREAD(fan1_alarm, 0, PRI_HIGH, 0x42, 0, 0x01, 2, bitmask),
+	PREAD(fan2_alarm, 1, PRI_HIGH, 0x42, 0, 0x01, 3, bitmask),
+	PREAD(fan3_alarm, 2, PRI_HIGH, 0x42, 0, 0x01, 4, bitmask),
+	PREAD(fan4_alarm, 3, PRI_HIGH, 0x42, 0, 0x01, 5, bitmask),
 
 	PREAD(temp1_input, 0, PRI_HIGH, 0x25, 0x10, 0, 0, temp10),
 	PREAD(temp2_input, 1, PRI_HIGH, 0x26, 0x15, 0, 0, temp10),
@@ -886,10 +885,10 @@ static struct asc7621_param asc7621_params[] = {
 	PWRITE(temp3_max, 2, PRI_LOW, 0x53, 0, 0, 0, temp8),
 	PWRITE(temp4_max, 3, PRI_LOW, 0x35, 0, 0, 0, temp8),
 
-	PREAD(temp1_alarm, 0, PRI_LOW, 0x41, 0, 0x01, 4, bitmask),
-	PREAD(temp2_alarm, 1, PRI_LOW, 0x41, 0, 0x01, 5, bitmask),
-	PREAD(temp3_alarm, 2, PRI_LOW, 0x41, 0, 0x01, 6, bitmask),
-	PREAD(temp4_alarm, 3, PRI_LOW, 0x43, 0, 0x01, 0, bitmask),
+	PREAD(temp1_alarm, 0, PRI_HIGH, 0x41, 0, 0x01, 4, bitmask),
+	PREAD(temp2_alarm, 1, PRI_HIGH, 0x41, 0, 0x01, 5, bitmask),
+	PREAD(temp3_alarm, 2, PRI_HIGH, 0x41, 0, 0x01, 6, bitmask),
+	PREAD(temp4_alarm, 3, PRI_HIGH, 0x43, 0, 0x01, 0, bitmask),
 
 	PWRITE(temp1_source, 0, PRI_LOW, 0x02, 0, 0x07, 4, bitmask),
 	PWRITE(temp2_source, 1, PRI_LOW, 0x02, 0, 0x07, 0, bitmask),
@@ -898,7 +897,7 @@ static struct asc7621_param asc7621_params[] = {
 
 	PWRITE(temp1_smoothing_enable, 0, PRI_LOW, 0x62, 0, 0x01, 3, bitmask),
 	PWRITE(temp2_smoothing_enable, 1, PRI_LOW, 0x63, 0, 0x01, 7, bitmask),
-	PWRITE(temp3_smoothing_enable, 2, PRI_LOW, 0x64, 0, 0x01, 3, bitmask),
+	PWRITE(temp3_smoothing_enable, 2, PRI_LOW, 0x63, 0, 0x01, 3, bitmask),
 	PWRITE(temp4_smoothing_enable, 3, PRI_LOW, 0x3c, 0, 0x01, 3, bitmask),
 
 	PWRITE(temp1_smoothing_time, 0, PRI_LOW, 0x62, 0, 0x07, 0, temp_st),
-- 
1.7.1


[-- Attachment #3: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

             reply	other threads:[~2010-05-01 13:08 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-05-01 13:08 Ken Milmore [this message]
2010-05-03 10:58 ` [lm-sensors] [PATCH] hwmon: (asc7621) Bug fixes Jean Delvare
2010-05-03 12:47 ` Ken Milmore
2010-05-03 13:14 ` Jean Delvare

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4BDC27C4.2000205@kenm.demon.co.uk \
    --to=ken@kenm.demon.co.uk \
    --cc=lm-sensors@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.