[lm-sensors] [PATCH] hwmon: (asc7621) Bug fixes

[lm-sensors] [PATCH] hwmon: (asc7621) Bug fixes

[Ken Milmore]

[-- 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

Re: [lm-sensors] [PATCH] hwmon: (asc7621) Bug fixes

[Jean Delvare]

Hi Ken,

On Sat, 01 May 2010 14:08:20 +0100, Ken Milmore wrote:
> 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.

Thanks for the patch. Review below.

> 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...

Indeed, and this is serious enough that I would take a patch fixing
that right now. Especially pwm[1-3]_enable = 0 MUST switch fans to
full speed, pwmconfig and fancontrol rely on this.

> 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))

It would make sense to use strtoul here instead. Negative fan speeds
make no sense.

>  		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));

And then this becomes = 0.

>  
>  	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);
> +

Looks much better :)

>  	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);

Do you really need this? There's another call to SENSORS_LIMIT() below,
I thought it would be enough.

> +
> +	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
> 


-- 
Jean Delvare

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

Re: [lm-sensors] [PATCH] hwmon: (asc7621) Bug fixes

[Ken Milmore]

Hi Jean, thanks for the feedback.

Regarding the SENSORS_LIMIT(reqval, 0, 0xffff) in store_in8() which you 
have tagged as spurious, I believe it is necessary to prevent an 
overflow situation if an input value were provided such that reqval * 
0xc0 > LONG_MAX.  I should probably have added a comment to this effect. 
  The second SENSORS_LIMIT() then limits the result of the scaling 
calculation to the register range 0 - 0xff.  To get away with a single 
SENSORS_LIMIT(), it would be necessary guard to against the full-scale 
value mapping to 256, which won't fit in the register.  This could be 
accomplished like this:

SENSORS_LIMIT(reqval, 0, ((asc7621_in_scaling[nr] * 0x100) / 0xc0) - 1);

- but that just looks far too obscure to me, and not worth the bother. 
Please let me know if you think otherwise.

In general I've been very cautious in preparing the patch to only change 
lines of code that were demonstrably broken, rather than fixing up 
stylistic issues.  For example, the temperature scaling calculation is 
done in what is IMHO a rather bizarre fashion, but it doesn't actually 
get the wrong answers so I have left well alone.  The same goes for use 
of signed and unsigned types: personally, I'd strongly prefer to see 
long, simple_strtol() and sprintf("%ld") used throughout the driver for 
the scaled values, as I think consistency makes for more maintainable 
code.  But I'm happy to go with your suggested changes all the same.

Lastly, regarding the PWM control settings, I will take a look at what 
can be done to make these comply better with the sysfs-interface.  IMHO 
there is no completely clean solution here; several of the 
sysfs-interface settings map onto the same bits of the same registers in 
the device, so they cannot be completely orthogonalised.

On a note of more general discussion:  I don't want to sound negative, 
but maybe the PWM control interface has been a step too far for the 
hwmon drivers.  For a device like the asc7621, the programmer has to 
bend over backwards to make it fit the published interface, simply 
because the settings must be poked into the device one at a time as they 
are changed.  For this kind of device, it would be better to build up a 
complete register profile and then stuff the whole thing into the device 
in one go.  This could be better handled by a userland library than a 
kernel driver...  Opinionated rant over. ;-)

Best wishes,

-Ken.


On 03/05/10 11:58, Jean Delvare wrote:
> Hi Ken,
>
> On Sat, 01 May 2010 14:08:20 +0100, Ken Milmore wrote:
>> 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.
>
> Thanks for the patch. Review below.
>
>> 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...
>
> Indeed, and this is serious enough that I would take a patch fixing
> that right now. Especially pwm[1-3]_enable = 0 MUST switch fans to
> full speed, pwmconfig and fancontrol rely on this.
>
>>  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))
>
> It would make sense to use strtoul here instead. Negative fan speeds
> make no sense.
>
>>   		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));
>
> And then this becomes = 0.
>
>>
>>   	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);
>> +
>
> Looks much better :)
>
>>   	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);
>
> Do you really need this? There's another call to SENSORS_LIMIT() below,
> I thought it would be enough.
>
>> +
>> +	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
>>
>
>

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

Re: [lm-sensors] [PATCH] hwmon: (asc7621) Bug fixes

[Jean Delvare]

Hi Ken,

If you could stop top-posting...

On Mon, 03 May 2010 13:47:30 +0100, Ken Milmore wrote:
> Regarding the SENSORS_LIMIT(reqval, 0, 0xffff) in store_in8() which you 
> have tagged as spurious, I believe it is necessary to prevent an 
> overflow situation if an input value were provided such that reqval * 
> 0xc0 > LONG_MAX.  I should probably have added a comment to this effect. 
>   The second SENSORS_LIMIT() then limits the result of the scaling 
> calculation to the register range 0 - 0xff.  To get away with a single 
> SENSORS_LIMIT(), it would be necessary guard to against the full-scale 
> value mapping to 256, which won't fit in the register.  This could be 
> accomplished like this:
> 
> SENSORS_LIMIT(reqval, 0, ((asc7621_in_scaling[nr] * 0x100) / 0xc0) - 1);
> 
> - but that just looks far too obscure to me, and not worth the bother. 
> Please let me know if you think otherwise.

OK, if this is done on purpose, then fine with me.

> In general I've been very cautious in preparing the patch to only change 
> lines of code that were demonstrably broken, rather than fixing up 
> stylistic issues.  For example, the temperature scaling calculation is 
> done in what is IMHO a rather bizarre fashion, but it doesn't actually 
> get the wrong answers so I have left well alone.  The same goes for use 
> of signed and unsigned types: personally, I'd strongly prefer to see 
> long, simple_strtol() and sprintf("%ld") used throughout the driver for 
> the scaled values, as I think consistency makes for more maintainable 
> code.  But I'm happy to go with your suggested changes all the same.

I'm very happy with the approach you took. The most important thing
right now is to get the driver in good shape for 2.6.34-final. If more
important changes or cleanups are desired, they can happen later.

BTW, if you feel like taking care of the asc7621 driver in the future,
feel tree to add your name to the MAINTAINERS entry for that driver. I
tend to accept more changes from driver maintainers than from other
contributors, because I know they will care if something breaks.

So I've applied your bugfixes patch, and will send it to Linus soon.

> Lastly, regarding the PWM control settings, I will take a look at what 
> can be done to make these comply better with the sysfs-interface.  IMHO 
> there is no completely clean solution here; several of the 
> sysfs-interface settings map onto the same bits of the same registers in 
> the device, so they cannot be completely orthogonalised.

I didn't look at the details (and certainly won't have the time to do
it) but this is something relatively frequent. It can require some
thinking to make all chips fit in the interface, but in most cases it
is manageable. Feel free to discuss specific issues you have.

> On a note of more general discussion:  I don't want to sound negative, 
> but maybe the PWM control interface has been a step too far for the 
> hwmon drivers.  For a device like the asc7621, the programmer has to 
> bend over backwards to make it fit the published interface, simply 
> because the settings must be poked into the device one at a time as they 
> are changed.  For this kind of device, it would be better to build up a 
> complete register profile and then stuff the whole thing into the device 
> in one go.  This could be better handled by a userland library than a 
> kernel driver...  Opinionated rant over. ;-)

You opinion on this is very welcome. I guess you refer to the automatic
fan speed control interface (the manual control interface is simple
enough that there's probably not much to discuss.) Originally there was
no standard interface for this. Several days ago I made a proposal to
standardize it, which received mixed reviews. I finally implemented it
because no better alternative came out, but there are still cases where
the chip won't fit.

As for the one-value-per-file approach, that's mandatory for sysfs
interface, and hwmon drivers have been using sysfs since kernel
2.5.something. That being said, the automatic fan speed control is
indeed very different from the monitoring part of the drivers, and I
would have no objection to a brand new interface being developed, if it
has significant advantages.

-- 
Jean Delvare

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