[lm-sensors] [PATCH] Fix the set pwm value will change fan mode bug

[lm-sensors] [PATCH] Fix the set pwm value will change fan mode bug

[Yuan Mu]

Hi Jean,

W83792D use pwm register low 4 bits to store fan PWM/DC value, bit 7 is used to store fan PWM/DC mode,
The store_pwm function use a wrong limit 0-255, so it may change the fan mode when set value is large than 127.

This fix the problem. Change the "index" value of pwm*_mode and pwm* SENSOR_ATTR to simplify code.

Signed-off-by: Yuan Mu <ymu at winbond.com.tw>
---
  w83792d.c |   59 +++++++++++++++++++++++++++--------------------------------
  1 file changed, 27 insertions(+), 32 deletions(-)

--- linux-2.6.17-rc2.orig/drivers/hwmon/w83792d.c	2006-04-19 18:04:04.000000000 +0800
+++ linux-2.6.17-rc2/drivers/hwmon/w83792d.c	2006-04-19 18:07:05.000000000 +0800
@@ -250,8 +250,6 @@ FAN_TO_REG(long rpm, int div)
  			: (val)) / 1000, 0, 0xff))
  #define TEMP_ADD_TO_REG_LOW(val)	((val%1000) ? 0x80 : 0x00)

-#define PWM_FROM_REG(val)		(val)
-#define PWM_TO_REG(val)			(SENSORS_LIMIT((val),0,255))
  #define DIV_FROM_REG(val)		(1 << (val))

  static inline u8
@@ -288,10 +286,8 @@ struct w83792d_data {
  	u8 temp1[3];		/* current, over, thyst */
  	u8 temp_add[2][6];	/* Register value */
  	u8 fan_div[7];		/* Register encoding, shifted right */
-	u8 pwm[7];		/* We only consider the first 3 set of pwm,
-				   although 792 chip has 7 set of pwm. */
+	u8 pwm[7];		/* Register value */
  	u8 pwmenable[3];
-	u8 pwm_mode[7];		/* indicates PWM or DC mode: 1->PWM; 0->DC */
  	u32 alarms;		/* realtime status register encoding,combined */
  	u8 chassis;		/* Chassis status */
  	u8 chassis_clear;	/* CLR_CHS, clear chassis intrusion detection */
@@ -627,7 +623,8 @@ show_pwm(struct device *dev, struct devi
  	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
  	int nr = sensor_attr->index;
  	struct w83792d_data *data = w83792d_update_device(dev);
-	return sprintf(buf, "%ld\n", (long) PWM_FROM_REG(data->pwm[nr-1]));
+
+	return sprintf(buf, "%d\n", data->pwm[nr] & 0x0f);
  }

  static ssize_t
@@ -659,14 +656,16 @@ store_pwm(struct device *dev, struct dev
  		const char *buf, size_t count)
  {
  	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
-	int nr = sensor_attr->index - 1;
+	int nr = sensor_attr->index;
  	struct i2c_client *client = to_i2c_client(dev);
  	struct w83792d_data *data = i2c_get_clientdata(client);
-	u32 val;
+	u8 val = data->pwm[nr] & 0xf0;

-	val = simple_strtoul(buf, NULL, 10);
-	data->pwm[nr] = PWM_TO_REG(val);
-	w83792d_write_value(client, W83792D_REG_PWM[nr], data->pwm[nr]);
+	val |= SENSORS_LIMIT(simple_strtoul(buf, NULL, 10), 0, 0x0f);
+	if (val != data->pwm[nr]) {
+		data->pwm[nr] = val;
+		w83792d_write_value(client, W83792D_REG_PWM[nr], val);
+	}

  	return count;
  }
@@ -707,9 +706,9 @@ store_pwmenable(struct device *dev, stru
  }

  static struct sensor_device_attribute sda_pwm[] = {
-	SENSOR_ATTR(pwm1, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 1),
-	SENSOR_ATTR(pwm2, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 2),
-	SENSOR_ATTR(pwm3, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 3),
+	SENSOR_ATTR(pwm1, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 0),
+	SENSOR_ATTR(pwm2, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 1),
+	SENSOR_ATTR(pwm3, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 2),
  };
  static struct sensor_device_attribute sda_pwm_enable[] = {
  	SENSOR_ATTR(pwm1_enable, S_IWUSR | S_IRUGO,
@@ -728,7 +727,7 @@ show_pwm_mode(struct device *dev, struct
  	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
  	int nr = sensor_attr->index;
  	struct w83792d_data *data = w83792d_update_device(dev);
-	return sprintf(buf, "%d\n", data->pwm_mode[nr-1]);
+	return sprintf(buf, "%d\n", data->pwm[nr] >> 7);
  }

  static ssize_t
@@ -736,29 +735,28 @@ store_pwm_mode(struct device *dev, struc
  			const char *buf, size_t count)
  {
  	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
-	int nr = sensor_attr->index - 1;
+	int nr = sensor_attr->index;
  	struct i2c_client *client = to_i2c_client(dev);
  	struct w83792d_data *data = i2c_get_clientdata(client);
-	u32 val;
-	u8 pwm_mode_mask = 0;
+	u8 val = data->pwm[nr] & 0x7f;

-	val = simple_strtoul(buf, NULL, 10);
-	data->pwm_mode[nr] = SENSORS_LIMIT(val, 0, 1);
-	pwm_mode_mask = w83792d_read_value(client,
-		W83792D_REG_PWM[nr]) & 0x7f;
-	w83792d_write_value(client, W83792D_REG_PWM[nr],
-		((data->pwm_mode[nr]) << 7) | pwm_mode_mask);
+	val |= SENSORS_LIMIT(simple_strtol(buf, NULL, 10), 0, 1) ? 0x80 : 0;
+
+	if (val != data->pwm[nr]) {
+		data->pwm[nr] = val;
+		w83792d_write_value(client, W83792D_REG_PWM[nr], val);
+	}

  	return count;
  }

  static struct sensor_device_attribute sda_pwm_mode[] = {
  	SENSOR_ATTR(pwm1_mode, S_IWUSR | S_IRUGO,
-		    show_pwm_mode, store_pwm_mode, 1),
+		    show_pwm_mode, store_pwm_mode, 0),
  	SENSOR_ATTR(pwm2_mode, S_IWUSR | S_IRUGO,
-		    show_pwm_mode, store_pwm_mode, 2),
+		    show_pwm_mode, store_pwm_mode, 1),
  	SENSOR_ATTR(pwm3_mode, S_IWUSR | S_IRUGO,
-		    show_pwm_mode, store_pwm_mode, 3),
+		    show_pwm_mode, store_pwm_mode, 2),
  };


@@ -1373,7 +1371,7 @@ static struct w83792d_data *w83792d_upda
  	struct i2c_client *client = to_i2c_client(dev);
  	struct w83792d_data *data = i2c_get_clientdata(client);
  	int i, j;
-	u8 reg_array_tmp[4], pwm_array_tmp[7], reg_tmp;
+	u8 reg_array_tmp[4], reg_tmp;

  	mutex_lock(&data->update_lock);

@@ -1402,10 +1400,8 @@ static struct w83792d_data *w83792d_upda
  			data->fan_min[i] = w83792d_read_value(client,
  						W83792D_REG_FAN_MIN[i]);
  			/* Update the PWM/DC Value and PWM/DC flag */
-			pwm_array_tmp[i] = w83792d_read_value(client,
+			data->pwm[i] = w83792d_read_value(client,
  						W83792D_REG_PWM[i]);
-			data->pwm[i] = pwm_array_tmp[i] & 0x0f;
-			data->pwm_mode[i] = pwm_array_tmp[i] >> 7;
  		}

  		reg_tmp = w83792d_read_value(client, W83792D_REG_FAN_CFG);
@@ -1513,7 +1509,6 @@ static void w83792d_print_debug(struct w
  		dev_dbg(dev, "fan[%d] is: 0x%x\n", i, data->fan[i]);
  		dev_dbg(dev, "fan[%d] min is: 0x%x\n", i, data->fan_min[i]);
  		dev_dbg(dev, "pwm[%d]     is: 0x%x\n", i, data->pwm[i]);
-		dev_dbg(dev, "pwm_mode[%d] is: 0x%x\n", i, data->pwm_mode[i]);
  	}
  	dev_dbg(dev, "3 set of Temperatures: ===>\n");
  	for (i=0; i<3; i++) {




=============================================The privileged confidential information contained in this email is intended for use only by the addressees as indicated by the original sender of this email. If you are not the addressee indicated in this email or are not responsible for delivery of the email to such  a person, please kindly reply to the sender indicating this fact and delete all copies of it from your computer and network server immediately. Your cooperation is highly appreciated. It is advised that any unauthorized use of confidential information of Winbond is strictly prohibited; and any information in this email irrelevant to the official business of Winbond shall be deemed as neither given nor endorsed by Winbond.

[lm-sensors] [PATCH] Fix the set pwm value will change fan mode

[Rudolf Marek]

Hi all,

Jean please can you push it forward? Looks good to me and I think Yuan  test it
before sending to us ;)

Regards
Rudolf

[lm-sensors] [PATCH] Fix the set pwm value will change fan mode

[Jean Delvare]

Hi Yuan,

> W83792D use pwm register low 4 bits to store fan PWM/DC
> value, bit 7 is used to store fan PWM/DC mode. The store_pwm
> function use a wrong limit 0-255, so it may change the fan
> mode when set value is large than 127.

Good catch, thanks for working on this (and sorry for the late answer.)

> This fix the problem. Change the "index" value of
> pwm*_mode and pwm* SENSOR_ATTR to simplify code.

That's not totally correct, unfortunately.

> --- linux-2.6.17-rc2.orig/drivers/hwmon/w83792d.c	2006-04-19 18:04:04.000000000 +0800
> +++ linux-2.6.17-rc2/drivers/hwmon/w83792d.c	2006-04-19 18:07:05.000000000 +0800
> @@ -250,8 +250,6 @@ FAN_TO_REG(long rpm, int div)
>   			: (val)) / 1000, 0, 0xff))
>   #define TEMP_ADD_TO_REG_LOW(val)	((val%1000) ? 0x80 : 0x00)
> 
> -#define PWM_FROM_REG(val)		(val)
> -#define PWM_TO_REG(val)			(SENSORS_LIMIT((val),0,255))
>   #define DIV_FROM_REG(val)		(1 << (val))
> 
>   static inline u8
> @@ -288,10 +286,8 @@ struct w83792d_data {
>   	u8 temp1[3];		/* current, over, thyst */
>   	u8 temp_add[2][6];	/* Register value */
>   	u8 fan_div[7];		/* Register encoding, shifted right */
> -	u8 pwm[7];		/* We only consider the first 3 set of pwm,
> -				   although 792 chip has 7 set of pwm. */
> +	u8 pwm[7];		/* Register value */

The comment still seems valid to me, pwm4-7 aren't handled by the
driver, right?

>   	u8 pwmenable[3];
> -	u8 pwm_mode[7];		/* indicates PWM or DC mode: 1->PWM; 0->DC */
>   	u32 alarms;		/* realtime status register encoding,combined */
>   	u8 chassis;		/* Chassis status */
>   	u8 chassis_clear;	/* CLR_CHS, clear chassis intrusion detection */
> @@ -627,7 +623,8 @@ show_pwm(struct device *dev, struct devi
>   	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
>   	int nr = sensor_attr->index;
>   	struct w83792d_data *data = w83792d_update_device(dev);
> -	return sprintf(buf, "%ld\n", (long) PWM_FROM_REG(data->pwm[nr-1]));
> +
> +	return sprintf(buf, "%d\n", data->pwm[nr] & 0x0f);
>   }

No, Documentation/hwmon/sysfs-interface says that the PWM values must
range from 0 (fan stopped) to 255 (fan at full speed). Here you range
from 0 to 15 only. You must scale the result so that it fits the
standard range.

> 
>   static ssize_t
> @@ -659,14 +656,16 @@ store_pwm(struct device *dev, struct dev
>   		const char *buf, size_t count)
>   {
>   	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> -	int nr = sensor_attr->index - 1;
> +	int nr = sensor_attr->index;
>   	struct i2c_client *client = to_i2c_client(dev);
>   	struct w83792d_data *data = i2c_get_clientdata(client);
> -	u32 val;
> +	u8 val = data->pwm[nr] & 0xf0;
> 
> -	val = simple_strtoul(buf, NULL, 10);
> -	data->pwm[nr] = PWM_TO_REG(val);
> -	w83792d_write_value(client, W83792D_REG_PWM[nr], data->pwm[nr]);
> +	val |= SENSORS_LIMIT(simple_strtoul(buf, NULL, 10), 0, 0x0f);
> +	if (val != data->pwm[nr]) {
> +		data->pwm[nr] = val;
> +		w83792d_write_value(client, W83792D_REG_PWM[nr], val);
> +	}
> 
>   	return count;
>   }

The write shouldn't be conditional. You base your condition on a cached
value, which can be very old if the user didn't read any register
lately. And you should be holding the lock here, else data->pwm[nr] may
change in your back.

Please also keep in mind that the allowed input range IS 0 to 255, as
per interface specification. It is the driver's job to scale it to what
the chip expects.

> @@ -707,9 +706,9 @@ store_pwmenable(struct device *dev, stru
>   }
> 
>   static struct sensor_device_attribute sda_pwm[] = {
> -	SENSOR_ATTR(pwm1, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 1),
> -	SENSOR_ATTR(pwm2, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 2),
> -	SENSOR_ATTR(pwm3, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 3),
> +	SENSOR_ATTR(pwm1, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 0),
> +	SENSOR_ATTR(pwm2, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 1),
> +	SENSOR_ATTR(pwm3, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 2),
>   };
>   static struct sensor_device_attribute sda_pwm_enable[] = {
>   	SENSOR_ATTR(pwm1_enable, S_IWUSR | S_IRUGO,
> @@ -728,7 +727,7 @@ show_pwm_mode(struct device *dev, struct
>   	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
>   	int nr = sensor_attr->index;
>   	struct w83792d_data *data = w83792d_update_device(dev);
> -	return sprintf(buf, "%d\n", data->pwm_mode[nr-1]);
> +	return sprintf(buf, "%d\n", data->pwm[nr] >> 7);
>   }
> 
>   static ssize_t
> @@ -736,29 +735,28 @@ store_pwm_mode(struct device *dev, struc
>   			const char *buf, size_t count)
>   {
>   	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> -	int nr = sensor_attr->index - 1;
> +	int nr = sensor_attr->index;
>   	struct i2c_client *client = to_i2c_client(dev);
>   	struct w83792d_data *data = i2c_get_clientdata(client);
> -	u32 val;
> -	u8 pwm_mode_mask = 0;
> +	u8 val = data->pwm[nr] & 0x7f;
> 
> -	val = simple_strtoul(buf, NULL, 10);
> -	data->pwm_mode[nr] = SENSORS_LIMIT(val, 0, 1);
> -	pwm_mode_mask = w83792d_read_value(client,
> -		W83792D_REG_PWM[nr]) & 0x7f;
> -	w83792d_write_value(client, W83792D_REG_PWM[nr],
> -		((data->pwm_mode[nr]) << 7) | pwm_mode_mask);
> +	val |= SENSORS_LIMIT(simple_strtol(buf, NULL, 10), 0, 1) ? 0x80 : 0;
> +
> +	if (val != data->pwm[nr]) {
> +		data->pwm[nr] = val;
> +		w83792d_write_value(client, W83792D_REG_PWM[nr], val);
> +	}
> 
>   	return count;
>   }

Same here, write should be unconditional, and you should be holding the
lock. Also, you don't want to use SENSORS_LIMIT here. Instead, reject
invalid values (return -EINVAL).

> 
>   static struct sensor_device_attribute sda_pwm_mode[] = {
>   	SENSOR_ATTR(pwm1_mode, S_IWUSR | S_IRUGO,
> -		    show_pwm_mode, store_pwm_mode, 1),
> +		    show_pwm_mode, store_pwm_mode, 0),
>   	SENSOR_ATTR(pwm2_mode, S_IWUSR | S_IRUGO,
> -		    show_pwm_mode, store_pwm_mode, 2),
> +		    show_pwm_mode, store_pwm_mode, 1),
>   	SENSOR_ATTR(pwm3_mode, S_IWUSR | S_IRUGO,
> -		    show_pwm_mode, store_pwm_mode, 3),
> +		    show_pwm_mode, store_pwm_mode, 2),
>   };
> 
> 
> @@ -1373,7 +1371,7 @@ static struct w83792d_data *w83792d_upda
>   	struct i2c_client *client = to_i2c_client(dev);
>   	struct w83792d_data *data = i2c_get_clientdata(client);
>   	int i, j;
> -	u8 reg_array_tmp[4], pwm_array_tmp[7], reg_tmp;
> +	u8 reg_array_tmp[4], reg_tmp;
> 
>   	mutex_lock(&data->update_lock);
> 
> @@ -1402,10 +1400,8 @@ static struct w83792d_data *w83792d_upda
>   			data->fan_min[i] = w83792d_read_value(client,
>   						W83792D_REG_FAN_MIN[i]);
>   			/* Update the PWM/DC Value and PWM/DC flag */
> -			pwm_array_tmp[i] = w83792d_read_value(client,
> +			data->pwm[i] = w83792d_read_value(client,
>   						W83792D_REG_PWM[i]);
> -			data->pwm[i] = pwm_array_tmp[i] & 0x0f;
> -			data->pwm_mode[i] = pwm_array_tmp[i] >> 7;
>   		}
> 
>   		reg_tmp = w83792d_read_value(client, W83792D_REG_FAN_CFG);
> @@ -1513,7 +1509,6 @@ static void w83792d_print_debug(struct w
>   		dev_dbg(dev, "fan[%d] is: 0x%x\n", i, data->fan[i]);
>   		dev_dbg(dev, "fan[%d] min is: 0x%x\n", i, data->fan_min[i]);
>   		dev_dbg(dev, "pwm[%d]     is: 0x%x\n", i, data->pwm[i]);
> -		dev_dbg(dev, "pwm_mode[%d] is: 0x%x\n", i, data->pwm_mode[i]);
>   	}
>   	dev_dbg(dev, "3 set of Temperatures: ===>\n");
>   	for (i=0; i<3; i++) {

I'm fine with all the other changes. Care to update this patch, test it
and submit it again?

Thanks,
-- 
Jean Delvare

[lm-sensors] [PATCH] Fix the set pwm value will change fan mode

[Rudolf Marek]

Hi all,

Sorry, I did not catch the interface violation. No bell rang in my head. I will
pay more attention to this next time.

Regards
Rudolf

[lm-sensors] [PATCH] Fix the set pwm value will change fan mode

[Yuan Mu]

Hi Jean,

Here comes the patch, please can you check, i add the lock and allow fan
pwm input range 0-255 here, the write also unconditional, please can you check :)



W83792D use pwm register low 4 bits to store PWM/DC value, bit 7 is used to store fan PWM/DC mode,
The store_pwm function did not convert the pwm input correctly, so it may change the fan mode when new value is set.

This fix the problem. Change the "index" value of pwm*_mode and pwm* SENSOR_ATTR to simplify code.


Signed-off-by: Yuan Mu <ymu at winbond.com.tw>
---
 w83792d.c |   59 ++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 30 insertions(+), 29 deletions(-)

--- linux-2.6.17-rc5.orig/drivers/hwmon/w83792d.c	2006-05-29 14:43:09.000000000 +0800
+++ linux-2.6.17-rc5/drivers/hwmon/w83792d.c	2006-05-30 10:00:42.000000000 +0800
@@ -250,8 +250,6 @@ FAN_TO_REG(long rpm, int div)
 			: (val)) / 1000, 0, 0xff))
 #define TEMP_ADD_TO_REG_LOW(val)	((val%1000) ? 0x80 : 0x00)

-#define PWM_FROM_REG(val)		(val)
-#define PWM_TO_REG(val)			(SENSORS_LIMIT((val),0,255))
 #define DIV_FROM_REG(val)		(1 << (val))

 static inline u8
@@ -291,7 +289,6 @@ struct w83792d_data {
 	u8 pwm[7];		/* We only consider the first 3 set of pwm,
 				   although 792 chip has 7 set of pwm. */
 	u8 pwmenable[3];
-	u8 pwm_mode[7];		/* indicates PWM or DC mode: 1->PWM; 0->DC */
 	u32 alarms;		/* realtime status register encoding,combined */
 	u8 chassis;		/* Chassis status */
 	u8 chassis_clear;	/* CLR_CHS, clear chassis intrusion detection */
@@ -627,7 +624,7 @@ show_pwm(struct device *dev, struct devi
 	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
 	int nr = sensor_attr->index;
 	struct w83792d_data *data = w83792d_update_device(dev);
-	return sprintf(buf, "%ld\n", (long) PWM_FROM_REG(data->pwm[nr-1]));
+	return sprintf(buf, "%d\n", (data->pwm[nr] & 0x0f) << 4);
 }

 static ssize_t
@@ -659,14 +656,16 @@ store_pwm(struct device *dev, struct dev
 		const char *buf, size_t count)
 {
 	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
-	int nr = sensor_attr->index - 1;
+	int nr = sensor_attr->index;
 	struct i2c_client *client = to_i2c_client(dev);
 	struct w83792d_data *data = i2c_get_clientdata(client);
-	u32 val;
+	u8 val = SENSORS_LIMIT(simple_strtoul(buf, NULL, 10), 0, 255) >> 4;

-	val = simple_strtoul(buf, NULL, 10);
-	data->pwm[nr] = PWM_TO_REG(val);
+	mutex_lock(&data->update_lock);
+	val |= w83792d_read_value(client, W83792D_REG_PWM[nr]) & 0xf0;
+	data->pwm[nr] = val;
 	w83792d_write_value(client, W83792D_REG_PWM[nr], data->pwm[nr]);
+	mutex_unlock(&data->update_lock);

 	return count;
 }
@@ -707,9 +706,9 @@ store_pwmenable(struct device *dev, stru
 }

 static struct sensor_device_attribute sda_pwm[] = {
-	SENSOR_ATTR(pwm1, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 1),
-	SENSOR_ATTR(pwm2, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 2),
-	SENSOR_ATTR(pwm3, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 3),
+	SENSOR_ATTR(pwm1, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 0),
+	SENSOR_ATTR(pwm2, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 1),
+	SENSOR_ATTR(pwm3, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 2),
 };
 static struct sensor_device_attribute sda_pwm_enable[] = {
 	SENSOR_ATTR(pwm1_enable, S_IWUSR | S_IRUGO,
@@ -728,7 +727,7 @@ show_pwm_mode(struct device *dev, struct
 	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
 	int nr = sensor_attr->index;
 	struct w83792d_data *data = w83792d_update_device(dev);
-	return sprintf(buf, "%d\n", data->pwm_mode[nr-1]);
+	return sprintf(buf, "%d\n", data->pwm[nr] >> 7);
 }

 static ssize_t
@@ -736,29 +735,34 @@ store_pwm_mode(struct device *dev, struc
 			const char *buf, size_t count)
 {
 	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
-	int nr = sensor_attr->index - 1;
+	int nr = sensor_attr->index;
 	struct i2c_client *client = to_i2c_client(dev);
 	struct w83792d_data *data = i2c_get_clientdata(client);
-	u32 val;
-	u8 pwm_mode_mask = 0;
+	u32 val = simple_strtoul(buf, NULL, 10);

-	val = simple_strtoul(buf, NULL, 10);
-	data->pwm_mode[nr] = SENSORS_LIMIT(val, 0, 1);
-	pwm_mode_mask = w83792d_read_value(client,
-		W83792D_REG_PWM[nr]) & 0x7f;
-	w83792d_write_value(client, W83792D_REG_PWM[nr],
-		((data->pwm_mode[nr]) << 7) | pwm_mode_mask);
+	mutex_lock(&data->update_lock);
+	data->pwm[nr] = w83792d_read_value(client, W83792D_REG_PWM[nr]);
+	if (0 = val) {			/* DC mode */
+		data->pwm[nr] &= 0x7f;
+	} else if (1 = val) {		/* PWM mode */
+		data->pwm[nr] = (data->pwm[nr] & 0x7f) | 0x80;
+	} else {
+		return -EINVAL;
+	}
+
+	w83792d_write_value(client, W83792D_REG_PWM[nr], data->pwm[nr]);
+	mutex_unlock(&data->update_lock);

 	return count;
 }

 static struct sensor_device_attribute sda_pwm_mode[] = {
 	SENSOR_ATTR(pwm1_mode, S_IWUSR | S_IRUGO,
-		    show_pwm_mode, store_pwm_mode, 1),
+		    show_pwm_mode, store_pwm_mode, 0),
 	SENSOR_ATTR(pwm2_mode, S_IWUSR | S_IRUGO,
-		    show_pwm_mode, store_pwm_mode, 2),
+		    show_pwm_mode, store_pwm_mode, 1),
 	SENSOR_ATTR(pwm3_mode, S_IWUSR | S_IRUGO,
-		    show_pwm_mode, store_pwm_mode, 3),
+		    show_pwm_mode, store_pwm_mode, 2),
 };


@@ -1373,7 +1377,7 @@ static struct w83792d_data *w83792d_upda
 	struct i2c_client *client = to_i2c_client(dev);
 	struct w83792d_data *data = i2c_get_clientdata(client);
 	int i, j;
-	u8 reg_array_tmp[4], pwm_array_tmp[7], reg_tmp;
+	u8 reg_array_tmp[4], reg_tmp;

 	mutex_lock(&data->update_lock);

@@ -1402,10 +1406,8 @@ static struct w83792d_data *w83792d_upda
 			data->fan_min[i] = w83792d_read_value(client,
 						W83792D_REG_FAN_MIN[i]);
 			/* Update the PWM/DC Value and PWM/DC flag */
-			pwm_array_tmp[i] = w83792d_read_value(client,
+			data->pwm[i] = w83792d_read_value(client,
 						W83792D_REG_PWM[i]);
-			data->pwm[i] = pwm_array_tmp[i] & 0x0f;
-			data->pwm_mode[i] = pwm_array_tmp[i] >> 7;
 		}

 		reg_tmp = w83792d_read_value(client, W83792D_REG_FAN_CFG);
@@ -1513,7 +1515,6 @@ static void w83792d_print_debug(struct w
 		dev_dbg(dev, "fan[%d] is: 0x%x\n", i, data->fan[i]);
 		dev_dbg(dev, "fan[%d] min is: 0x%x\n", i, data->fan_min[i]);
 		dev_dbg(dev, "pwm[%d]     is: 0x%x\n", i, data->pwm[i]);
-		dev_dbg(dev, "pwm_mode[%d] is: 0x%x\n", i, data->pwm_mode[i]);
 	}
 	dev_dbg(dev, "3 set of Temperatures: ===>\n");
 	for (i=0; i<3; i++) {


=============================================The privileged confidential information contained in this email is intended for use only by the addressees as indicated by the original sender of this email. If you are not the addressee indicated in this email or are not responsible for delivery of the email to such  a person, please kindly reply to the sender indicating this fact and delete all copies of it from your computer and network server immediately. Your cooperation is highly appreciated. It is advised that any unauthorized use of confidential information of Winbond is strictly prohibited; and any information in this email irrelevant to the official business of Winbond shall be deemed as neither given nor endorsed by Winbond.
If your computer is unable to decode Chinese font, please ignore the following message.It essentially repeats the statement in English given above.
???????????????????, ?????????????????. ?????????????????????????????????, ????????????????????????????. ??????, ??????. ????, ?????????????????????????????. ??????????????,??????????????.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ymu.vcf
Type: text/x-vcard
Size: 61 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060530/8324c56a/ymu.vcf

[lm-sensors] [PATCH] Fix the set pwm value will change fan mode

[Jean Delvare]

Hi Yuan,

> Here comes the patch, please can you check, i add the lock and allow fan
> pwm input range 0-255 here, the write also unconditional, please can you check :)

Thanks for the updated patch. There are a few new issues in
store_pwm_mode:

> @@ -736,29 +735,34 @@ store_pwm_mode(struct device *dev, struc
>  			const char *buf, size_t count)
>  {
>  	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> -	int nr = sensor_attr->index - 1;
> +	int nr = sensor_attr->index;
>  	struct i2c_client *client = to_i2c_client(dev);
>  	struct w83792d_data *data = i2c_get_clientdata(client);
> -	u32 val;
> -	u8 pwm_mode_mask = 0;
> +	u32 val = simple_strtoul(buf, NULL, 10);
> 
> -	val = simple_strtoul(buf, NULL, 10);
> -	data->pwm_mode[nr] = SENSORS_LIMIT(val, 0, 1);
> -	pwm_mode_mask = w83792d_read_value(client,
> -		W83792D_REG_PWM[nr]) & 0x7f;
> -	w83792d_write_value(client, W83792D_REG_PWM[nr],
> -		((data->pwm_mode[nr]) << 7) | pwm_mode_mask);
> +	mutex_lock(&data->update_lock);
> +	data->pwm[nr] = w83792d_read_value(client, W83792D_REG_PWM[nr]);
> +	if (0 = val) {			/* DC mode */
> +		data->pwm[nr] &= 0x7f;
> +	} else if (1 = val) {		/* PWM mode */
> +		data->pwm[nr] = (data->pwm[nr] & 0x7f) | 0x80;

Equivalent to:
		data->pwm[nr] |= 0x80;

> +	} else {
> +		return -EINVAL;

You return with the update lock held! It's easier to test the input
value for validity before taking the lock, this avoids that kind of
trap and is also more efficient.

> +	}
> +
> +	w83792d_write_value(client, W83792D_REG_PWM[nr], data->pwm[nr]);
> +	mutex_unlock(&data->update_lock);
> 
>  	return count;
>  }

I've fixed both issues and applied your patch:
http://khali.linux-fr.org/devel/i2c/linux-2.6/hwmon-w83792d-pwm-set-fix.patch

Rudolf, parts of this patch should be ported back to the Linux 2.4
version of the driver. Please take care of this once our new Subversion
repository is usable.

Thanks,
-- 
Jean Delvare

[lm-sensors] [PATCH] Fix the set pwm value will change fan mode

[Yuan Mu]

Hi Jean,

[snipped]
>> +		data->pwm[nr] = (data->pwm[nr] & 0x7f) | 0x80;
> 
> Equivalent to:
> 		data->pwm[nr] |= 0x80;
> 
Oh, thank you , i will beat my head :(
>> +	} else {
>> +		return -EINVAL;
> 
> You return with the update lock held! It's easier to test the input
> value for validity before taking the lock, this avoids that kind of
> trap and is also more efficient.
> 

Beat my head once more ;)

And do we need to add lock in other store functions in w83792d ?


Best Regards
Yuan Mu







=============================================The privileged confidential information contained in this email is intended for use only by the addressees as indicated by the original sender of this email. If you are not the addressee indicated in this email or are not responsible for delivery of the email to such  a person, please kindly reply to the sender indicating this fact and delete all copies of it from your computer and network server immediately. Your cooperation is highly appreciated. It is advised that any unauthorized use of confidential information of Winbond is strictly prohibited; and any information in this email irrelevant to the official business of Winbond shall be deemed as neither given nor endorsed by Winbond.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ymu.vcf
Type: text/x-vcard
Size: 56 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060530/a715b008/ymu-0001.vcf

[lm-sensors] [PATCH] Fix the set pwm value will change fan mode

[Jean Delvare]

Hi Yuan,

> And do we need to add lock in other store functions in w83792d ?

Yes, we do. I'm surprised that it isn't the case. All drivers were
supposedly fixed at the same time. Maybe the w83792d driver was
integrated at that time and slipped through the checks.

-- 
Jean Delvare