[lm-sensors] [PATCH] W83627EHF driver update

[lm-sensors] [PATCH] W83627EHF driver update

[Rudolf Marek]

Hello

This patch adds long-awaited suport for automatic fan modes. Based on the work
of Yuan Mu from Winbond, I finished the support with the great help of David
Hubbard. The documenation update will follow.

Signed-Off-By: Yuan Mu <Ymu at Winbond.com.tw>
Signed-Off-By: Rudolf Marek <r.marek at sh.cvut.cz>
Signed-Off-By: David Hubbard <david.c.hubbard at gmail.com>

Some notes: The patch should be OK. It was checked/tested many times by me and
David. This patch is based on ehf_patch_rc1 and ehf_patch_fix_fan45. More than
80 columns were fixed and on 4 places mutex lock/unlock was placed.

David, please can you check/test this final patch before Jean takes it?
Please have a look to store_pwm_enable which has non-trivial locking. I belive 
it is OK and I tested briefly, but better to be sure.

Thanks,
regards
Rudolf

-------------- next part --------------
A non-text attachment was scrubbed...
Name: ehf_u.patch
Type: text/x-patch
Size: 18050 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060606/5822ece9/attachment-0001.bin 

[lm-sensors] [PATCH] W83627EHF driver update

[David Hubbard]

Hi Rudolf,

Okay, I will test this and get back to you tomorrow.

Thanks,
David

On 6/6/06, Rudolf Marek <r.marek at sh.cvut.cz> wrote:
> Hello
>
> This patch adds long-awaited suport for automatic fan modes. Based on the work
> of Yuan Mu from Winbond, I finished the support with the great help of David
> Hubbard. The documenation update will follow.
>
> Signed-Off-By: Yuan Mu <Ymu at Winbond.com.tw>
> Signed-Off-By: Rudolf Marek <r.marek at sh.cvut.cz>
> Signed-Off-By: David Hubbard <david.c.hubbard at gmail.com>
>
> Some notes: The patch should be OK. It was checked/tested many times by me and
> David. This patch is based on ehf_patch_rc1 and ehf_patch_fix_fan45. More than
> 80 columns were fixed and on 4 places mutex lock/unlock was placed.
>
> David, please can you check/test this final patch before Jean takes it?
> Please have a look to store_pwm_enable which has non-trivial locking. I belive
> it is OK and I tested briefly, but better to be sure.
>
> Thanks,
> regards
> Rudolf

[lm-sensors] [PATCH] W83627EHF driver update

[Jim Cromie]

Rudolf Marek wrote:
>
> Please have a look to store_pwm_enable which has non-trivial locking. 
> I belive it is OK and I tested briefly, but better to be sure.
>

> +static ssize_t
> +store_pwm_enable(struct device *dev, struct device_attribute *attr,
> +			const char *buf, size_t count)
> +{
>   
> +	if (data->pwm_enable[nr] != val) {
>   
I guess *here* is where you mean non-trivial.

why not take the lock here -> instead of in both branches of if ?

> +		if (!val) {
>   
you specifically and knowingly dont protect this call,
and advised us of it in your msg.  It certainly warrants a comment here too.

> +			store_pwm(dev, attr, "255", 3);
>   
and now take the lock.
> +			mutex_lock(&data->update_lock);
> +			data->pwm_enable[nr] = val;
> +		}
> +		else {
>   
repeating, this lock could be hoisted, but for the missing/unstated reason.

> +			mutex_lock(&data->update_lock);
> +			data->pwm_enable[nr] = val;
> +			val--;
> +		}
> +		reg = (reg & ~(11 << W83627EHF_PWM_ENABLE_SHIFT[nr]));
> +		reg |= val << W83627EHF_PWM_ENABLE_SHIFT[nr];
> +		w83627ehf_write_value(client, W83627EHF_REG_PWM_ENABLE[nr],
> +					reg);
> +		mutex_unlock(&data->update_lock);
> +	}
> +	return count;
> +}
> +
>   
:-)

[lm-sensors] [PATCH] W83627EHF driver update

[David Hubbard]

Hi Rudolf,

I like the added mutex locking. It's a good idea. It sounds like
you'll be modifying it a little?

Here is the output of the regression test on my machine:
# ./w83627ehf_regression.sh
* WARNING: This will run your system fans through many possible
           combinations. There is a possibility your system will
           overheat. Use this script at your own risk!
Mapping fans...setting all PWMs to 100%
    fan 1 0 RPM: ignored. (Fan not installed?)
    fan 2 0 RPM: ignored. (Fan not installed?)
    fan 3 3479 RPM:
        pwm1 3443 RPM
        pwm2 3443 RPM
        pwm3 1654 RPM: connected.
Testing fan 3 - pwm3 ... div OK, PWM/DC OK, enable OK
test_smartfan1 $1="3" $2="pwm3"
, SF1 OK

Obviously, I'm right in the middle of building SmartFan 1 testing. It
doesn't test that. Everything else works fine. I'm attaching the
regression test which is updated for this patch. So, sign me off on
it.

Thanks,
David
-------------- next part --------------
A non-text attachment was scrubbed...
Name: w83627ehf_regression.sh
Type: application/x-sh
Size: 15366 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060607/ba81aba3/attachment.sh 

[lm-sensors] [PATCH] W83627EHF driver update

[Rudolf Marek]

Hi Jim,

>> +static ssize_t
>> +store_pwm_enable(struct device *dev, struct device_attribute *attr,
>> +			const char *buf, size_t count)
>> +{
>>   
>> +	if (data->pwm_enable[nr] != val) {
>>   
> I guess *here* is where you mean non-trivial.
> 
> why not take the lock here -> instead of in both branches of if ?
> 
>> +		if (!val) {
>>   
> you specifically and knowingly dont protect this call,
> and advised us of it in your msg.  It certainly warrants a comment here too.

The call protect itself because it locks on its own. If the lock would be 
before, it tries to lock 2 times and will deadlock.

> 
>> +			store_pwm(dev, attr, "255", 3);
>>   
> and now take the lock.
>> +			mutex_lock(&data->update_lock);
>> +			data->pwm_enable[nr] = val;
>> +		}
>> +		else {
>>   
> repeating, this lock could be hoisted, but for the missing/unstated reason.

I know this code is not perfect, but how can it be done without more locking or 
more confusing temporary variables?

Thanks,

regards
Rudolf

[lm-sensors] [PATCH] W83627EHF driver update

[Rudolf Marek]

Hi David,

> I like the added mutex locking. It's a good idea. It sounds like
> you'll be modifying it a little?

I hope not.

There is just a place that Jim mentioned with the lock in both branches of if.
It is not elegant, but without more temporary variables or multiple lock/unlock 
it wont simply work as expected.

Thanks for the testing. If you have better idea how to handle the particular 
place it would be good.

Otherwise if Jim has not any ideas I would like Jean to accept it as it is.

Thanks,
Regards
Rudolf

[lm-sensors] [PATCH] W83627EHF driver update

[Sylvain Jeaugey]

Hi David,

I tested the module with your script. Here's the output :
* WARNING: This will run your system fans through many possible
            combinations. There is a possibility your system will
            overheat. Use this script at your own risk!
Mapping fans...setting all PWMs to 100%
     fan 1 2083 RPM:
         pwm1 RPM highly variable? ignored.
         pwm2 2136 RPM
         pwm3 2109 RPM
     fan 2 3668 RPM:
         pwm1 3668 RPM
         pwm2 1140 RPM: connected.
         pwm3 3668 RPM
     fan 3 0 RPM: ignored. (Fan not installed?)
Testing fan 2 - pwm2 ... div OK, PWM/DC OK
Failure: 50% 3479 RPM, disabled (100%) 3515 RPM.

Fan 1 is my PSU fan, reporting (sometimes crazy) values, but not powered 
my the motherboard. Thus, fan control won't affect it. It's perfectly 
normal.

pwm2 seem to perfectly control my CPU fan (fan2). I was very surprised 
when my CPU fan (normaly running at 20%) started to run at full power !

I'll receive another fan tomorrow, and will connect it to fan1 (and this 
time, it may be controlled by the w83627ehf).
I'll keep you informed. Thanks a lot for your work ! I was _really_ 
waiting for this feature !

Cheers,
Sylvain

Note : to pass the files verification phase, I had to change
ls -l | cut -c-28,42- | (
into
/bin/ls -l | cut -c-28,46- | cut -c-11,13- | (
to make it look like what you expected. Looks like ls hasn't a 
standard output among distros.

On Wed, 7 Jun 2006, David Hubbard wrote:

> Hi Rudolf,
>
> I like the added mutex locking. It's a good idea. It sounds like
> you'll be modifying it a little?
>
> Here is the output of the regression test on my machine:
> # ./w83627ehf_regression.sh
> * WARNING: This will run your system fans through many possible
>          combinations. There is a possibility your system will
>          overheat. Use this script at your own risk!
> Mapping fans...setting all PWMs to 100%
>   fan 1 0 RPM: ignored. (Fan not installed?)
>   fan 2 0 RPM: ignored. (Fan not installed?)
>   fan 3 3479 RPM:
>       pwm1 3443 RPM
>       pwm2 3443 RPM
>       pwm3 1654 RPM: connected.
> Testing fan 3 - pwm3 ... div OK, PWM/DC OK, enable OK
> test_smartfan1 $1="3" $2="pwm3"
> , SF1 OK
>
> Obviously, I'm right in the middle of building SmartFan 1 testing. It
> doesn't test that. Everything else works fine. I'm attaching the
> regression test which is updated for this patch. So, sign me off on
> it.
>
> Thanks,
> David
>

[lm-sensors] [PATCH] W83627EHF driver update

[David Hubbard]

Hi Sylvain,

Thanks for the feedback! The ls | cut stuff is broken, I know. But
it's a quick way to check that stuff. Maybe I should just remove
it--the sysfs files wouldn't ever have wrong permissions, would they?

> I tested the module with your script. Here's the output :
> * WARNING: This will run your system fans through many possible
>             combinations. There is a possibility your system will
>             overheat. Use this script at your own risk!
> Mapping fans...setting all PWMs to 100%
>      fan 1 2083 RPM:
>          pwm1 RPM highly variable? ignored.
>          pwm2 2136 RPM
>          pwm3 2109 RPM

The highly variable message and the fact that it didn't find anything
here just means that the fan1 input isn't connected to any of the
pwms. That's what you said here, right?

> Fan 1 is my PSU fan, reporting (sometimes crazy) values, but not powered
> my the motherboard. Thus, fan control won't affect it. It's perfectly
> normal.


Okay, so it starts testing the fan 2 - pwm2 stuff:

> Testing fan 2 - pwm2 ... div OK, PWM/DC OK
> Failure: 50% 3479 RPM, disabled (100%) 3515 RPM.

What that means is that it set pwm2 to 50, then measured the RPM (3479
RPM). Then it disabled pwm2, which should set pwm2 to 100. It got 3515
RPM. That's not a significant change. However, I wouldn't say the
disabled mode is broken. I'd say that the regression test script isn't
robust enough, and should maybe take more samples at 50% before
disabling and sampling at 100%. Anyway, I'll tweak it and see if the
problem goes away. One thing is--the more samples it takes, the longer
the script takes to run. :-(

I'm still thinking about the best way to test all this stuff. But I
like getting test results!

David

[lm-sensors] [PATCH] W83627EHF driver update

[Sylvain Jeaugey]

Hi David,

On Wed, 7 Jun 2006, David Hubbard wrote:

>> Mapping fans...setting all PWMs to 100%
>>      fan 1 2083 RPM:
>>          pwm1 RPM highly variable? ignored.
>>          pwm2 2136 RPM
>>          pwm3 2109 RPM
>
> The highly variable message and the fact that it didn't find anything
> here just means that the fan1 input isn't connected to any of the
> pwms. That's what you said here, right?
Not exactly. My PSU has got a fan connector, so that I can monitor my 
PSU's RPMs. But on this fan connector, only the RPM wire is connected (and 
the ground I believe). Plus, the reported RPMs are quite .. variable (some 
glitches at 20000-30000 RPMs sometimes). So maybe pwm1 does affect fan1, 
but I'll need using a standard fan to test that (I just received it !).

About fan3/ pwm3, I only have 2 fans.

> Okay, so it starts testing the fan 2 - pwm2 stuff:
>
>> Testing fan 2 - pwm2 ... div OK, PWM/DC OK
>> Failure: 50% 3479 RPM, disabled (100%) 3515 RPM.
>
> What that means is that it set pwm2 to 50, then measured the RPM (3479
> RPM). Then it disabled pwm2, which should set pwm2 to 100. It got 3515
> RPM. That's not a significant change. However, I wouldn't say the
> disabled mode is broken. I'd say that the regression test script isn't
> robust enough, and should maybe take more samples at 50% before
> disabling and sampling at 100%. Anyway, I'll tweak it and see if the
> problem goes away. One thing is--the more samples it takes, the longer
> the script takes to run. :-(
Mmm. When the script ended, it left my fan speed at 100% (huge noise). So 
I had to rapidly figure out how to set fan speed down to 20%. I tried 
doing
$ echo 27 > pwm2
It didn't work. I messed a little with pwm2_enable / pwm2_mode (setting 
their values to other ones, then setting them back to original) and 
retried setting pwm to 27. This time, the fan slowed down.
I'll need to do further testing to understand what's going on.

After it worked, I could set my fan speed from 40 to 255 and hear the fan 
accelerate slowly.

> I'm still thinking about the best way to test all this stuff. But I
> like getting test results!
No problem, I like controling my fan so precisely :)

Sylvain

[lm-sensors] [PATCH] W83627EHF driver update

[Jean Delvare]

Hi Rudolf,

Sorry for the late answer.

> This patch adds long-awaited suport for automatic fan modes. Based on the work
> of Yuan Mu from Winbond, I finished the support with the great help of David
> Hubbard. The documenation update will follow.
> 
> Signed-Off-By: Yuan Mu <Ymu at Winbond.com.tw>
> Signed-Off-By: Rudolf Marek <r.marek at sh.cvut.cz>
> Signed-Off-By: David Hubbard <david.c.hubbard at gmail.com>
> 
> Some notes: The patch should be OK. It was checked/tested many times by me and
> David. This patch is based on ehf_patch_rc1 and ehf_patch_fix_fan45. More than
> 80 columns were fixed and on 4 places mutex lock/unlock was placed.
> 
> David, please can you check/test this final patch before Jean takes it?
> Please have a look to store_pwm_enable which has non-trivial locking. I belive 
> it is OK and I tested briefly, but better to be sure.

Here's my review:

> +/* SmartFan registers */
> +/* DC or PWM output fan configuration */
> +static const u8 W83627EHF_REG_PWM_ENABLE[] = {
> +	0x04,			/* SYS FAN0 output mode and PWM mode */
> +	0x04,			/* CPU FAN0 output mode and PWM mode */
> +	0x12,			/* AUX FAN mode all fan MIN */
> +	0x62			/* CPU fan1 mode */
> +};

Missing comma after the last entry.

Also the comments lack consistency, in particular we don't care that
register 0x12 contains "fan min" bits here.

> +
> +/* maps the user modes to chip modes */

I don't see how this comment is related to the following data.

> +static const u8 W83627EHF_PWM_MODE_SHIFT[] = { 0, 1, 0, 6 };
> +static const u8 W83627EHF_PWM_ENABLE_SHIFT[] = { 2, 4, 1, 4 };
> +
> +/* FAN Duty Cycle, be used to control */
> +static const u8 W83627EHF_REG_PWM[] = { 0x01, 0x03, 0x11, 0x61 };
> +static const u8 W83627EHF_REG_TARGET_TEMP[] = { 0x05, 0x06, 0x13, 0x63 };

The name is not very well chosen, these registers can hold both a target
temperature or a target fan speed depending on the control mode (even
if we don't support the latter at the moment.) What about just
W83627EHF_REG_TARGET?

> +static const u8 W83627EHF_REG_TOLERANCE[] = { 0x07, 0x07, 0x14, 0x62 };
> +
> +
> +/* Advanced Fan control */
> +static const u8 W83627EHF_REG_FAN_MIN_OUTPUT[] = { 0x08, 0x09, 0x15, 0x64 };
> +static const u8 W83627EHF_REG_FAN_MAX_OUTPUT[] = { 0x67, 0x69 };
> +static const u8 W83627EHF_REG_FAN_STEP[] = { 0x68, 0x6A };
> +static const u8 W83627EHF_REG_FAN_STOP_TIME[] = { 0x0C, 0x0D, 0x17, 0x66 };
> +static const u8 W83627EHF_REG_FAN_STEP_TIME[] = { 0x0E, 0x0F };
> +

For arrays with four values it's quite clear that there's one value for
each fan, but for the arrays with only two values,  it's not so clear.
Some comments would be welcome.

>  /*
>   * Conversions
>   */
>  
> +/* 0 is PWM mode, output in ms */
> +static inline unsigned int step_time_from_reg(u8 reg, u8 mode)
> +{
> +	return mode ? 100 * reg : 400 * reg; 
> +}

Comment is wrong, PWM mode is 1 not 0.

> +
> +static inline u8 step_time_to_reg(unsigned int msec, u8 mode)
> +{
> +	return mode ? (msec + 50) / 100 : (msec + 200) / 400;
> +}
> +

Missing check for overflow!

>  static inline unsigned int
>  fan_from_reg(u8 reg, unsigned int div)
>  {
> @@ -223,6 +262,21 @@ struct w83627ehf_data {
>  	s16 temp_max[2];
>  	s16 temp_max_hyst[2];
>  	u32 alarms;
> + 
> +	u8 pwm_mode[4]; /* 0->DC variable voltage, 1->PWM variable duty cycle */
> +	u8 pwm_enable[4]; /* 0->disabled
> +			   * 1->manual
> +			   * 2->thermal cruise (also called SmartFan I)
> +			   */
> +	u8 pwm[4];
> +	u8 target_temp[4];
> +	u8 tolerance[4];
> +
> +	u8 fan_min_output[4]; /* minimum fan speed */
> +	u8 fan_max_output[2];
> +	u8 fan_step[2];
> +	u8 fan_stop_time[4];
> +	u8 fan_step_time[2]; /* 0 Down time, 1 up time */
>  };
>  
>  static inline int is_word_sized(u16 reg)
> @@ -350,6 +404,7 @@ static struct w83627ehf_data *w83627ehf_
>  	struct i2c_client *client = to_i2c_client(dev);
>  	struct w83627ehf_data *data = i2c_get_clientdata(client);
>  	int i;
> +	int tmp = 0;

Initialization not needed.

>  
>  	mutex_lock(&data->update_lock);
>  
> @@ -416,6 +471,43 @@ static struct w83627ehf_data *w83627ehf_
>  			}
>  		}
>  
> +		for (i = 0; i < 4; i++) {
> +			if (i != 1)
> +				tmp = w83627ehf_read_value(client,
> +						W83627EHF_REG_PWM_ENABLE[i]);
> +			data->pwm_mode[i] > +				((tmp >> W83627EHF_PWM_MODE_SHIFT[i]) & 1)
> +				? 0 : 1;
> +			if (data->pwm_enable[i] != 0 ||
> +			    ((tmp >> W83627EHF_PWM_ENABLE_SHIFT[i]) & 3) != 0)
> +				data->pwm_enable[i] = 
> +					((tmp >> W83627EHF_PWM_ENABLE_SHIFT[i])
> +						& 3) + 1;

I don't understand this test, can you please explain?

> +			data->pwm[i] = w83627ehf_read_value(client,
> +						W83627EHF_REG_PWM[i]);
> +			data->fan_min_output[i] = w83627ehf_read_value(client,
> +						W83627EHF_REG_FAN_MIN_OUTPUT[i]);
> +			data->fan_stop_time[i] = w83627ehf_read_value(client,
> +						W83627EHF_REG_FAN_STOP_TIME[i]);
> +			data->target_temp[i] > +				w83627ehf_read_value(client,
> +					W83627EHF_REG_TARGET_TEMP[i]) &
> +					(data->pwm_mode[i] = 1 ? 0x7f : 0xff);
> +			data->tolerance[i] > +				(w83627ehf_read_value(client,
> +					W83627EHF_REG_TOLERANCE[i]) >>
> +					(i = 1 ? 4 : 0)) & 0x0f;
> +		}

Register 0x07 is read twice, this could be avoided with the same trick
you use for register 0x04. I would also suggest that you make the
temporary variable(s) local as you only use it (them) here. What about
the following?

		for (i = 0; i < 4; i++) {
			int pwmcfg, tolerance;

			if (i != 1) {
				pwmcfg = w83627ehf_read_value(client,
						W83627EHF_REG_PWM_ENABLE[i]);
				tolerance = w83627ehf_read_value(client,
						W83627EHF_REG_TOLERANCE[i]);
			}

			data->pwm_mode[i] 				((pwmcfg >> W83627EHF_PWM_MODE_SHIFT[i]) & 1)
				? 0 : 1;

			(... other register reads ...)

			data->tolerance[i] = (tolerance >> (i = 1 ? 4 : 0)) & 0x0f;
		}

> +
> +		for (i = 0; i < 2; i++) {
> +			data->fan_max_output[i] = w83627ehf_read_value(client,
> +						W83627EHF_REG_FAN_MAX_OUTPUT[i]);
> +			data->fan_step[i] = w83627ehf_read_value(client,
> +						W83627EHF_REG_FAN_STEP[i]);
> +			data->fan_step_time[i] = w83627ehf_read_value(client,
> +						W83627EHF_REG_FAN_STEP_TIME[i]);
> +		}
> +
>  		/* Measured temperatures and limits */
>  		data->temp1 = w83627ehf_read_value(client,
>  			      W83627EHF_REG_TEMP1);
> @@ -777,6 +869,321 @@ static struct sensor_device_attribute sd
>  	SENSOR_ATTR(temp3_alarm, S_IRUGO, show_alarm, NULL, 13),
>  };
>  
> +#define show_pwm_reg(reg) \
> +static ssize_t show_##reg (struct device *dev, struct device_attribute *attr, \
> +				char *buf) \
> +{ \
> +	struct w83627ehf_data *data = w83627ehf_update_device(dev); \
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);\
> +	int nr = sensor_attr->index;\
> +	return sprintf(buf,"%d\n", data->reg[nr]); \

Missing space after comma. Your spaces before backslashes are also
inconsistent.

> +}
> +
> +show_pwm_reg(pwm_mode)
> +show_pwm_reg(pwm_enable)
> +show_pwm_reg(pwm)
> +
> +static ssize_t
> +store_pwm_mode(struct device *dev, struct device_attribute *attr,
> +			const char *buf, size_t count)
> +{
> +	struct w83627ehf_data *data = w83627ehf_update_device(dev);

No, you don't need to call update_device in a "store" sysfs callback.

> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> +	int nr = sensor_attr->index;
> +	u32 val = SENSORS_LIMIT(simple_strtoul(buf, NULL, 10), 0, 1);

Using SENSOR_LIMIT here doesn't make sense, as we're not dealing with a
range. Better return -EINVAL if value is neither 0 nor 1.

> +	u16 reg = w83627ehf_read_value(client, W83627EHF_REG_PWM_ENABLE[nr]);
> +
> +	if (data->pwm_mode[nr] != val) {
> +		mutex_lock(&data->update_lock);
> +		data->pwm_mode[nr] = val;
> +		reg = (reg & ~(1 << W83627EHF_PWM_MODE_SHIFT[nr]));

reg &= ..., or at least drop the extra pair of parentheses.

> +		if (!val)
> +			reg |= 1 << W83627EHF_PWM_MODE_SHIFT[nr];
> +		w83627ehf_write_value(client, W83627EHF_REG_PWM_ENABLE[nr],
> +					reg);
> +		mutex_unlock(&data->update_lock);
> +	}
> +	return count;
> +}

Locking is broken. You're doing a read-modify-write cycle, you must
hold the lock before the read. You also must hold the lock when testing
data->pwm_mode[nr] - even though I don't think you should test it at
all, I see no reason to make the register write conditional.

Same comments apply to all your store callback functions, it seems.

> +
> +static ssize_t
> +store_pwm(struct device *dev, struct device_attribute *attr,
> +			const char *buf, size_t count)
> +{
> +	struct w83627ehf_data *data = w83627ehf_update_device(dev);
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> +	int nr = sensor_attr->index;
> +	u32 val = SENSORS_LIMIT(simple_strtoul(buf, NULL, 10), 0, 255);
> +
> +	if (data->pwm_enable[nr] != 0 && data->pwm[nr] != val) {
> +		mutex_lock(&data->update_lock);
> +		data->pwm[nr] = val;
> +		w83627ehf_write_value(client, W83627EHF_REG_PWM[nr],
> +					val);
> +		mutex_unlock(&data->update_lock);
> +	}
> +	return count;
> +}
> +
> +static ssize_t
> +store_pwm_enable(struct device *dev, struct device_attribute *attr,
> +			const char *buf, size_t count)
> +{
> +	struct w83627ehf_data *data = w83627ehf_update_device(dev);
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> +	int nr = sensor_attr->index;
> +	u32 val = SENSORS_LIMIT(simple_strtoul(buf, NULL, 10), 0, 3);
> +	u16 reg = w83627ehf_read_value(client, W83627EHF_REG_PWM_ENABLE[nr]);
> +
> +	if (data->pwm_enable[nr] != val) {
> +		if (!val) {
> +			store_pwm(dev, attr, "255", 3);
> +			mutex_lock(&data->update_lock);
> +			data->pwm_enable[nr] = val;
> +		}
> +		else {
> +			mutex_lock(&data->update_lock);
> +			data->pwm_enable[nr] = val;
> +			val--;
> +		}
> +		reg = (reg & ~(11 << W83627EHF_PWM_ENABLE_SHIFT[nr]));

What is this "11" falling from the sky?

> +		reg |= val << W83627EHF_PWM_ENABLE_SHIFT[nr];
> +		w83627ehf_write_value(client, W83627EHF_REG_PWM_ENABLE[nr],
> +					reg);
> +		mutex_unlock(&data->update_lock);
> +	}
> +	return count;
> +}

I just realized that you are emulating pwm_enable = 0. I guess the chip
doesn't support it? Then you don't want to implement it in the driver.
This emulation makes your code much more complex with no gain at all.
Drivers must implement what the chip support, they must NOT emulate
features.

I'm stopping my review here. Please fix:
* use of w83627ehf_update_device
* use of SENSORS_LIMIT
* locking
* conditional register writes
all around the place, then test the patch again, and then I'll review
it again.

> +
> +
> +#define show_tol_temp(reg) \
> +static ssize_t show_##reg(struct device *dev, struct device_attribute *attr, \
> +				char *buf) \
> +{ \
> +	struct w83627ehf_data *data = w83627ehf_update_device(dev); \
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr); \
> +	int nr = sensor_attr->index; \
> +	return sprintf(buf,"%d\n", temp1_from_reg(data->reg[nr])); \
> +}
> +
> +show_tol_temp(tolerance)
> +show_tol_temp(target_temp)
> +
> +static ssize_t
> +store_target_temp(struct device *dev, struct device_attribute *attr,
> +			const char *buf, size_t count)
> +{
> +	struct w83627ehf_data *data = w83627ehf_update_device(dev);
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> +	int nr = sensor_attr->index;
> +	u8 val = temp1_to_reg(
> +			SENSORS_LIMIT(simple_strtoul(buf, NULL, 10), 0, 127000));
> +
> +	if (data->target_temp[nr] != val) {
> +		mutex_lock(&data->update_lock);
> +		data->target_temp[nr] = val;
> +		w83627ehf_write_value(client, W83627EHF_REG_TARGET_TEMP[nr],
> +					val);
> +		mutex_unlock(&data->update_lock);
> +	}
> +	return count;
> +}
> +
> +static ssize_t
> +store_tolerance(struct device *dev, struct device_attribute *attr,
> +			const char *buf, size_t count)
> +{
> +	struct w83627ehf_data *data = w83627ehf_update_device(dev);
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> +	int nr = sensor_attr->index;
> +	/* Limit the temp to 0C - 15C */
> +	u8 val = temp1_to_reg(
> +			SENSORS_LIMIT(simple_strtoul(buf, NULL, 10), 0, 15000));
> +
> +	u16 reg = w83627ehf_read_value(client, W83627EHF_REG_TOLERANCE[nr]);
> +
> +	if (data->tolerance[nr] != val) {
> +		mutex_lock(&data->update_lock);
> +		data->tolerance[nr] = val;
> +		if (nr = 1)
> +			reg = (reg & 0x0f) | (val << 4);
> +		else
> +			reg = (reg & 0xf0) | val;
> +		w83627ehf_write_value(client,
> +				W83627EHF_REG_TOLERANCE[nr], reg);
> +		mutex_unlock(&data->update_lock);
> +	}
> +	return count;
> +}
> +
> +static struct sensor_device_attribute sda_pwm[] = {
> +	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),
> +	SENSOR_ATTR(pwm4, S_IWUSR | S_IRUGO, show_pwm, store_pwm, 3),
> +};
> +
> +static struct sensor_device_attribute sda_pwm_mode[] = {
> +	SENSOR_ATTR(pwm1_mode, S_IWUSR | S_IRUGO, show_pwm_mode,
> +		    store_pwm_mode, 0),
> +	SENSOR_ATTR(pwm2_mode, S_IWUSR | S_IRUGO, show_pwm_mode,
> +		    store_pwm_mode, 1),
> +	SENSOR_ATTR(pwm3_mode, S_IWUSR | S_IRUGO, show_pwm_mode,
> +		    store_pwm_mode, 2),
> +	SENSOR_ATTR(pwm4_mode, S_IWUSR | S_IRUGO, show_pwm_mode,
> +		    store_pwm_mode, 3),
> +};
> +
> +static struct sensor_device_attribute sda_pwm_enable[] = {
> +	SENSOR_ATTR(pwm1_enable, S_IWUSR | S_IRUGO, show_pwm_enable,
> +		    store_pwm_enable, 0),
> +	SENSOR_ATTR(pwm2_enable, S_IWUSR | S_IRUGO, show_pwm_enable,
> +		    store_pwm_enable, 1),
> +	SENSOR_ATTR(pwm3_enable, S_IWUSR | S_IRUGO, show_pwm_enable,
> +		    store_pwm_enable, 2),
> +	SENSOR_ATTR(pwm4_enable, S_IWUSR | S_IRUGO, show_pwm_enable,
> +		    store_pwm_enable, 3),
> +};
> +
> +static struct sensor_device_attribute sda_target_temp[] = {
> +	SENSOR_ATTR(temp1_target, S_IWUSR | S_IRUGO, show_target_temp,
> +		    store_target_temp, 0),
> +	SENSOR_ATTR(temp2_target, S_IWUSR | S_IRUGO, show_target_temp,
> +		    store_target_temp, 1),
> +	SENSOR_ATTR(temp3_target, S_IWUSR | S_IRUGO, show_target_temp,
> +		    store_target_temp, 2),
> +	SENSOR_ATTR(temp4_target, S_IWUSR | S_IRUGO, show_target_temp,
> +		    store_target_temp, 3),
> +};
> +
> +static struct sensor_device_attribute sda_tolerance[] = {
> +	SENSOR_ATTR(temp1_tolerance, S_IWUSR | S_IRUGO, show_tolerance,
> +		    store_tolerance, 0),
> +	SENSOR_ATTR(temp2_tolerance, S_IWUSR | S_IRUGO, show_tolerance,
> +		    store_tolerance, 1),
> +	SENSOR_ATTR(temp3_tolerance, S_IWUSR | S_IRUGO, show_tolerance,
> +		    store_tolerance, 2),
> +	SENSOR_ATTR(temp4_tolerance, S_IWUSR | S_IRUGO, show_tolerance,
> +		    store_tolerance, 3),
> +};
> +
> +static void device_create_file_pwm(struct device *dev, int i)
> +{
> +	device_create_file(dev, &sda_pwm[i].dev_attr);
> +	device_create_file(dev, &sda_pwm_mode[i].dev_attr);
> +	device_create_file(dev, &sda_pwm_enable[i].dev_attr);
> +	device_create_file(dev, &sda_target_temp[i].dev_attr);
> +	device_create_file(dev, &sda_tolerance[i].dev_attr);
> +}
> +
> +/* Smart Fan registers */
> +
> +#define fan_functions(reg, REG) \
> +static ssize_t show_##reg(struct device *dev, struct device_attribute *attr, \
> +		       char *buf) \
> +{ \
> +	struct w83627ehf_data *data = w83627ehf_update_device(dev); \
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr); \
> +	int nr = sensor_attr->index; \
> +	return sprintf(buf,"%d\n", data->reg[nr]); \
> +}\
> +static ssize_t \
> +store_##reg(struct device *dev, struct device_attribute *attr, \
> +			    const char *buf, size_t count) \
> +{\
> +	struct i2c_client *client = to_i2c_client(dev); \
> +	struct w83627ehf_data *data = i2c_get_clientdata(client); \
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr); \
> +	int nr = sensor_attr->index; \
> +	u32 val = SENSORS_LIMIT(simple_strtoul(buf, NULL, 10), 0, 255);\
> +	if ( data->reg[nr] != val ) { \
> +		mutex_lock(&data->update_lock); \
> +		data->reg[nr] = val; \
> +		w83627ehf_write_value(client, W83627EHF_REG_##REG[nr], \
> +					 val); \
> +		mutex_unlock(&data->update_lock); \
> +	} \
> +	return count; \
> +}
> +
> +fan_functions(fan_min_output, FAN_MIN_OUTPUT)
> +fan_functions(fan_max_output, FAN_MAX_OUTPUT)
> +fan_functions(fan_step, FAN_STEP)
> +
> +#define fan_time_functions(reg, REG) \
> +static ssize_t show_##reg(struct device *dev, struct device_attribute *attr, \
> +				char *buf) \
> +{ \
> +	struct w83627ehf_data *data = w83627ehf_update_device(dev); \
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr); \
> +	int nr = sensor_attr->index; \
> +	return sprintf(buf,"%d\n", \
> +			step_time_from_reg(data->reg[nr], data->pwm_mode[nr])); \
> +} \
> +\
> +static ssize_t \
> +store_##reg(struct device *dev, struct device_attribute *attr, \
> +			const char *buf, size_t count) \
> +{ \
> +	struct w83627ehf_data *data = w83627ehf_update_device(dev); \
> +	struct i2c_client *client = to_i2c_client(dev); \
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr); \
> +	int nr = sensor_attr->index; \
> +	u8 val = step_time_to_reg(simple_strtoul(buf, NULL, 10), \
> +					data->pwm_mode[nr]); \
> +	if ( data->reg[nr] != val ) { \
> +		mutex_lock(&data->update_lock); \
> +		data->reg[nr] = val; \
> +		w83627ehf_write_value(client, W83627EHF_REG_##REG[nr], \
> +					 val); \
> +		mutex_unlock(&data->update_lock); \
> +	} \
> +	return count; \
> +} \
> +
> +fan_time_functions(fan_step_time, FAN_STEP_TIME)
> +fan_time_functions(fan_stop_time, FAN_STOP_TIME)
> +
> +
> +static struct sensor_device_attribute sda_sf3_arrays_fan4[] = {
> +	SENSOR_ATTR(fan4_stop_time, S_IWUSR | S_IRUGO, show_fan_stop_time,
> +		    store_fan_stop_time, 3),
> +	SENSOR_ATTR(fan4_min_output, S_IWUSR | S_IRUGO, show_fan_min_output,
> +		    store_fan_min_output, 3),
> +	SENSOR_ATTR(fan4_max_output, S_IWUSR | S_IRUGO, show_fan_max_output,
> +		    store_fan_max_output, 1),
> +	SENSOR_ATTR(fan4_step, S_IWUSR | S_IRUGO, show_fan_step,
> +		    store_fan_step, 1),
> +};
> +
> +static struct sensor_device_attribute sda_sf3_arrays[] = {
> +	SENSOR_ATTR(fan_step_up_time, S_IWUSR | S_IRUGO, show_fan_step_time,
> +		    store_fan_step_time, 1),
> +	SENSOR_ATTR(fan_step_down_time, S_IWUSR | S_IRUGO, show_fan_step_time,
> +		    store_fan_step_time, 0),
> +	SENSOR_ATTR(fan1_stop_time, S_IWUSR | S_IRUGO, show_fan_stop_time,
> +		    store_fan_stop_time, 0),
> +	SENSOR_ATTR(fan2_stop_time, S_IWUSR | S_IRUGO, show_fan_stop_time,
> +		    store_fan_stop_time, 1),
> +	SENSOR_ATTR(fan3_stop_time, S_IWUSR | S_IRUGO, show_fan_stop_time,
> +		    store_fan_stop_time, 2),
> +	SENSOR_ATTR(fan1_min_output, S_IWUSR | S_IRUGO, show_fan_min_output,
> +		    store_fan_min_output, 0),
> +	SENSOR_ATTR(fan2_min_output, S_IWUSR | S_IRUGO, show_fan_min_output,
> +		    store_fan_min_output, 1),
> +	SENSOR_ATTR(fan3_min_output, S_IWUSR | S_IRUGO, show_fan_min_output,
> +		    store_fan_min_output, 2),
> +	SENSOR_ATTR(fan2_max_output, S_IWUSR | S_IRUGO, show_fan_max_output,
> +		    store_fan_max_output, 0),
> +	SENSOR_ATTR(fan2_step, S_IWUSR | S_IRUGO, show_fan_step,
> +		    store_fan_step, 0),
> +};
> +
>  /*
>   * Driver and client management
>   */
> @@ -810,6 +1217,7 @@ static int w83627ehf_detect(struct i2c_a
>  	struct i2c_client *client;
>  	struct w83627ehf_data *data;
>  	struct device *dev;
> +	u8 cr24, cr29;
>  	int i, err = 0;
>  
>  	if (!request_region(address + REGION_OFFSET, REGION_LENGTH,
> @@ -848,13 +1256,22 @@ static int w83627ehf_detect(struct i2c_a
>  		data->fan_min[i] = w83627ehf_read_value(client,
>  				   W83627EHF_REG_FAN_MIN[i]);
>  
> +	/* fan4 and fan5 share some pins with the GPIO and serial flash */
> +
> +	superio_enter();
> +	/* flash needs to be disabled for AUXFANIN1/fan5 -> 00 */
> +	cr24 = superio_inb(0x24) & 0x2; 
> +	cr29 = superio_inb(0x29) & 0x6; /* CPUFANIN1/fan4 ->00 */
> +	superio_exit();
> +	
>  	/* It looks like fan4 and fan5 pins can be alternatively used
>  	   as fan on/off switches */
> +	
>  	data->has_fan = 0x07; /* fan1, fan2 and fan3 */
>  	i = w83627ehf_read_value(client, W83627EHF_REG_FANDIV1);
> -	if (i & (1 << 2))
> +	if ((i & (1 << 2)) && (!cr29))
>  		data->has_fan |= (1 << 3);
> -	if (i & (1 << 0))
> +	if ((i & (1 << 0)) && (!cr24))
>  		data->has_fan |= (1 << 4);
>  
>  	/* Register sysfs hooks */
> @@ -864,13 +1281,28 @@ static int w83627ehf_detect(struct i2c_a
>  		goto exit_detach;
>  	}
>  
> +  	for (i = 0; i < ARRAY_SIZE(sda_sf3_arrays); i++)
> +  		device_create_file(dev, &sda_sf3_arrays[i].dev_attr);
> +
> +	/* if fan4 is enabled create the sf3 files for it */
> +	if (data->has_fan & (1 << 3))
> +		for (i = 0; i < ARRAY_SIZE(sda_sf3_arrays_fan4); i++)
> +			device_create_file(dev, &sda_sf3_arrays_fan4[i].dev_attr);
> +
>  	for (i = 0; i < 10; i++)
>  		device_create_file_in(dev, i);
>  
> -	for (i = 0; i < 5; i++) {
> +	for (i = 0; i < 5; i++)
>  		if (data->has_fan & (1 << i))
>  			device_create_file_fan(dev, i);
> +
> +	for (i = 0; i < 4; i++) {
> +		if (data->has_fan & (1 << i)) {
> +			device_create_file_pwm(dev, i);
> +			data->pwm_mode[i] = 1; /* initialize in PWM mode */
> +		}
>  	}
> +
>  	for (i = 0; i < ARRAY_SIZE(sda_temp); i++)
>  		device_create_file(dev, &sda_temp[i].dev_attr);
>  
> 


-- 
Jean Delvare

[lm-sensors] [PATCH] W83627EHF driver update

[David Hubbard]

Hi Jean, Rudolf,

> > +
> > +/* maps the user modes to chip modes */
>
> I don't see how this comment is related to the following data.
>
> > +static const u8 W83627EHF_PWM_MODE_SHIFT[] = { 0, 1, 0, 6 };
> > +static const u8 W83627EHF_PWM_ENABLE_SHIFT[] = { 2, 4, 1, 4 };

The PWM modes are located at different bit positions in the registers.
the PWM_MODE_SHIFT values give the bit positions. Remember that the
pwmN_mode sets PWM or DC.

The PWM_ENABLE_SHIFT values give the bit positions from the PWM enable
settings. After the emulated mode 0 (disabled) is accounted for, then
they go into the register.

So I guess we should replace the comment with a better explanation. I
think that comment is a remnant from some code that was deleted.

> > +/* FAN Duty Cycle, be used to control */
> > +static const u8 W83627EHF_REG_PWM[] = { 0x01, 0x03, 0x11, 0x61 };
> > +static const u8 W83627EHF_REG_TARGET_TEMP[] = { 0x05, 0x06, 0x13, 0x63 };
>
> The name is not very well chosen, these registers can hold both a target
> temperature or a target fan speed depending on the control mode (even
> if we don't support the latter at the moment.) What about just
> W83627EHF_REG_TARGET?

This is a good suggestion. In the near future, the registers will hold
one or the other value, and the driver will support both modes. Rudolf
and I talked about storing both values in the driver, with separate
sysfs files so libsensors could read either value, the one on the chip
or the one that will replace the value on the chip if the mode
switched. Advantages to doing it this way: we can set a reasonable
default for the value that's not on the chip, so when the mdoe
switches, the value in the register is not left unchanged (and
probably not reasonable); also, from userspace, it looks like there
are two registers, even though in hardware there's only one.

Disadvantages: well, emulation is bad, isn't it? But here I would
argue for emulating separate registers in the driver because the
register sharing in the hardware is impossible to map directly to a
sysfs file. If we're going to have two sysfs files with different
meanings, then can we just emulate separate registers?

> I just realized that you are emulating pwm_enable = 0. I guess the chip
> doesn't support it? Then you don't want to implement it in the driver.
> This emulation makes your code much more complex with no gain at all.
> Drivers must implement what the chip support, they must NOT emulate
> features.
>
> I'm stopping my review here. Please fix:
> * use of w83627ehf_update_device
> * use of SENSORS_LIMIT
> * locking
> * conditional register writes
> all around the place, then test the patch again, and then I'll review
> it again.

So it looks like there are some comments to fix, a few minor things,
plus what you've listed here. Rudolf, I'll be able to spend some time
on this over the weekend. That's a way off, if you feel like working
on it before then.

Thanks for the review, Jean.

David

[lm-sensors] [PATCH] W83627EHF driver update

[Jean Delvare]

Hi David,

> > The name is not very well chosen, these registers can hold both a target
> > temperature or a target fan speed depending on the control mode (even
> > if we don't support the latter at the moment.) What about just
> > W83627EHF_REG_TARGET?
> 
> This is a good suggestion. In the near future, the registers will hold
> one or the other value, and the driver will support both modes. Rudolf
> and I talked about storing both values in the driver, with separate
> sysfs files so libsensors could read either value, the one on the chip
> or the one that will replace the value on the chip if the mode
> switched. Advantages to doing it this way: we can set a reasonable
> default for the value that's not on the chip, so when the mdoe
> switches, the value in the register is not left unchanged (and
> probably not reasonable); also, from userspace, it looks like there
> are two registers, even though in hardware there's only one.
> 
> Disadvantages: well, emulation is bad, isn't it? But here I would
> argue for emulating separate registers in the driver because the
> register sharing in the hardware is impossible to map directly to a
> sysfs file. If we're going to have two sysfs files with different
> meanings, then can we just emulate separate registers?

I'm fine with having two sets of internal variables, the reasons you
give in that sense are good ones. I don't think doing so qualifies as
"emulation". The hardware designers took an unfortunate shortcut by
using the same registers for different functions, we need to deal with
this. 

Thanks,
-- 
Jean Delvare

[lm-sensors] [PATCH] W83627EHF driver update

[David Hubbard]

Hi Rudolf,

On 6/13/06, Jean Delvare <khali at linux-fr.org> wrote:
> Hi David,
>
> > > The name is not very well chosen, these registers can hold both a target
> > > temperature or a target fan speed depending on the control mode (even
> > > if we don't support the latter at the moment.) What about just
> > > W83627EHF_REG_TARGET?
> >
> > This is a good suggestion. In the near future, the registers will hold
> > one or the other value, and the driver will support both modes. Rudolf
> > and I talked about storing both values in the driver, with separate
> > sysfs files so libsensors could read either value, the one on the chip
> > or the one that will replace the value on the chip if the mode
> > switched. Advantages to doing it this way: we can set a reasonable
> > default for the value that's not on the chip, so when the mdoe
> > switches, the value in the register is not left unchanged (and
> > probably not reasonable); also, from userspace, it looks like there
> > are two registers, even though in hardware there's only one.
> >
> > Disadvantages: well, emulation is bad, isn't it? But here I would
> > argue for emulating separate registers in the driver because the
> > register sharing in the hardware is impossible to map directly to a
> > sysfs file. If we're going to have two sysfs files with different
> > meanings, then can we just emulate separate registers?
>
> I'm fine with having two sets of internal variables, the reasons you
> give in that sense are good ones. I don't think doing so qualifies as
> "emulation". The hardware designers took an unfortunate shortcut by
> using the same registers for different functions, we need to deal with
> this.

Okay, that won't show up in the code this week. I'd like to see the
new features we have working signed off. I think that's Rudolf opinion
too. Then we'll add the other features, including that one.

David

[lm-sensors] [PATCH] W83627EHF driver update

[Rudolf Marek]

Hi all,
> 
> The name is not very well chosen, these registers can hold both a target
> temperature or a target fan speed depending on the control mode (even
> if we don't support the latter at the moment.) What about just
> W83627EHF_REG_TARGET?

I think this is in the plan for future driver upgrade.

> 
>> +static const u8 W83627EHF_REG_TOLERANCE[] = { 0x07, 0x07, 0x14, 0x62 };
>> +
>> +
>> +/* Advanced Fan control */
>> +static const u8 W83627EHF_REG_FAN_MIN_OUTPUT[] = { 0x08, 0x09, 0x15, 0x64 };
>> +static const u8 W83627EHF_REG_FAN_MAX_OUTPUT[] = { 0x67, 0x69 };
>> +static const u8 W83627EHF_REG_FAN_STEP[] = { 0x68, 0x6A };
>> +static const u8 W83627EHF_REG_FAN_STOP_TIME[] = { 0x0C, 0x0D, 0x17, 0x66 };
>> +static const u8 W83627EHF_REG_FAN_STEP_TIME[] = { 0x0E, 0x0F };
>> +
> 
> For arrays with four values it's quite clear that there's one value for
> each fan, but for the arrays with only two values,  it's not so clear.
> Some comments would be welcome.

OK

> 
>>  /*
>>   * Conversions
>>   */
>>  
>> +/* 0 is PWM mode, output in ms */
>> +static inline unsigned int step_time_from_reg(u8 reg, u8 mode)
>> +{
>> +	return mode ? 100 * reg : 400 * reg; 
>> +}
> 
> Comment is wrong, PWM mode is 1 not 0.

Good catch.

> 
>> +
>> +static inline u8 step_time_to_reg(unsigned int msec, u8 mode)
>> +{
>> +	return mode ? (msec + 50) / 100 : (msec + 200) / 400;
>> +}
>> +
> 
> Missing check for overflow!

When it is bigger than 255... Will fix...

=>
> Initialization not needed.

OK

> 
>>  
>>  	mutex_lock(&data->update_lock);
>>  
>> @@ -416,6 +471,43 @@ static struct w83627ehf_data *w83627ehf_
>>  			}
>>  		}
>>  
>> +		for (i = 0; i < 4; i++) {
>> +			if (i != 1)
>> +				tmp = w83627ehf_read_value(client,
>> +						W83627EHF_REG_PWM_ENABLE[i]);
>> +			data->pwm_mode[i] >> +				((tmp >> W83627EHF_PWM_MODE_SHIFT[i]) & 1)
>> +				? 0 : 1;
>> +			if (data->pwm_enable[i] != 0 ||
>> +			    ((tmp >> W83627EHF_PWM_ENABLE_SHIFT[i]) & 3) != 0)
>> +				data->pwm_enable[i] = 
>> +					((tmp >> W83627EHF_PWM_ENABLE_SHIFT[i])
>> +						& 3) + 1;
> 
> I don't understand this test, can you please explain?

Well this is for that MODE 0 emulation. I think we agreed we can ignore the mode.

> 
>> +			data->pwm[i] = w83627ehf_read_value(client,
>> +						W83627EHF_REG_PWM[i]);
>> +			data->fan_min_output[i] = w83627ehf_read_value(client,
>> +						W83627EHF_REG_FAN_MIN_OUTPUT[i]);
>> +			data->fan_stop_time[i] = w83627ehf_read_value(client,
>> +						W83627EHF_REG_FAN_STOP_TIME[i]);
>> +			data->target_temp[i] >> +				w83627ehf_read_value(client,
>> +					W83627EHF_REG_TARGET_TEMP[i]) &
>> +					(data->pwm_mode[i] = 1 ? 0x7f : 0xff);
>> +			data->tolerance[i] >> +				(w83627ehf_read_value(client,
>> +					W83627EHF_REG_TOLERANCE[i]) >>
>> +					(i = 1 ? 4 : 0)) & 0x0f;
>> +		}
> 
> Register 0x07 is read twice, this could be avoided with the same trick
> you use for register 0x04. I would also suggest that you make the
> temporary variable(s) local as you only use it (them) here. What about
> the following?
> 
> 		for (i = 0; i < 4; i++) {
> 			int pwmcfg, tolerance;
> 
> 			if (i != 1) {
> 				pwmcfg = w83627ehf_read_value(client,
> 						W83627EHF_REG_PWM_ENABLE[i]);
> 				tolerance = w83627ehf_read_value(client,
> 						W83627EHF_REG_TOLERANCE[i]);
> 			}
> 


Ok looks good.

> 			data->pwm_mode[i] > 				((pwmcfg >> W83627EHF_PWM_MODE_SHIFT[i]) & 1)
> 				? 0 : 1;
> 
> 			(... other register reads ...)
> 
> 			data->tolerance[i] = (tolerance >> (i = 1 ? 4 : 0)) & 0x0f;
> 		}
> 
>> +
>> +		for (i = 0; i < 2; i++) {
>> +			data->fan_max_output[i] = w83627ehf_read_value(client,
>> +						W83627EHF_REG_FAN_MAX_OUTPUT[i]);
>> +			data->fan_step[i] = w83627ehf_read_value(client,
>> +						W83627EHF_REG_FAN_STEP[i]);
>> +			data->fan_step_time[i] = w83627ehf_read_value(client,
>> +						W83627EHF_REG_FAN_STEP_TIME[i]);
>> +		}
>> +
>>  		/* Measured temperatures and limits */
>>  		data->temp1 = w83627ehf_read_value(client,
>>  			      W83627EHF_REG_TEMP1);
>> @@ -777,6 +869,321 @@ static struct sensor_device_attribute sd
>>  	SENSOR_ATTR(temp3_alarm, S_IRUGO, show_alarm, NULL, 13),
>>  };
>>  
>> +#define show_pwm_reg(reg) \
>> +static ssize_t show_##reg (struct device *dev, struct device_attribute *attr, \
>> +				char *buf) \
>> +{ \
>> +	struct w83627ehf_data *data = w83627ehf_update_device(dev); \
>> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);\
>> +	int nr = sensor_attr->index;\
>> +	return sprintf(buf,"%d\n", data->reg[nr]); \
> 
> Missing space after comma. Your spaces before backslashes are also
> inconsistent.

Ok I will try to unify this. There are three authors of this patch so sometimes
it is quite hard to get all stuff right.


> 
>> +}
>> +
>> +show_pwm_reg(pwm_mode)
>> +show_pwm_reg(pwm_enable)
>> +show_pwm_reg(pwm)
>> +
>> +static ssize_t
>> +store_pwm_mode(struct device *dev, struct device_attribute *attr,
>> +			const char *buf, size_t count)
>> +{
>> +	struct w83627ehf_data *data = w83627ehf_update_device(dev);
> 
> No, you don't need to call update_device in a "store" sysfs callback.

Yes good catch.


>> +	struct i2c_client *client = to_i2c_client(dev);
>> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
>> +	int nr = sensor_attr->index;
>> +	u32 val = SENSORS_LIMIT(simple_strtoul(buf, NULL, 10), 0, 1);
> 
> Using SENSOR_LIMIT here doesn't make sense, as we're not dealing with a
> range. Better return -EINVAL if value is neither 0 nor 1.

Well yes why not, maybe we should write a note to a docs?

> 
>> +	u16 reg = w83627ehf_read_value(client, W83627EHF_REG_PWM_ENABLE[nr]);
>> +
>> +	if (data->pwm_mode[nr] != val) {
>> +		mutex_lock(&data->update_lock);
>> +		data->pwm_mode[nr] = val;
>> +		reg = (reg & ~(1 << W83627EHF_PWM_MODE_SHIFT[nr]));
> 
> reg &= ..., or at least drop the extra pair of parentheses.

oOK

> 
>> +		if (!val)
>> +			reg |= 1 << W83627EHF_PWM_MODE_SHIFT[nr];
>> +		w83627ehf_write_value(client, W83627EHF_REG_PWM_ENABLE[nr],
>> +					reg);
>> +		mutex_unlock(&data->update_lock);
>> +	}
>> +	return count;
>> +}
> 
> Locking is broken. You're doing a read-modify-write cycle, you must
> hold the lock before the read. 
> You also must hold the lock when testing
> data->pwm_mode[nr] - even though I don't think you should test it at
> all, I see no reason to make the register write conditional.

 The register writes are just time "savers" I think it Yuan did it also for the
new driver...

> 
> Same comments apply to all your store callback functions, it seems.

I'm not a author of this code, I just did not catch it ...

>> +
>> +static ssize_t
>> +store_pwm(struct device *dev, struct device_attribute *attr,
>> +			const char *buf, size_t count)
>> +{
>> +	struct w83627ehf_data *data = w83627ehf_update_device(dev);
>> +	struct i2c_client *client = to_i2c_client(dev);
>> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
>> +	int nr = sensor_attr->index;
>> +	u32 val = SENSORS_LIMIT(simple_strtoul(buf, NULL, 10), 0, 255);
>> +
>> +	if (data->pwm_enable[nr] != 0 && data->pwm[nr] != val) {
>> +		mutex_lock(&data->update_lock);
>> +		data->pwm[nr] = val;
>> +		w83627ehf_write_value(client, W83627EHF_REG_PWM[nr],
>> +					val);
>> +		mutex_unlock(&data->update_lock);
>> +	}
>> +	return count;
>> +}
>> +
>> +static ssize_t
>> +store_pwm_enable(struct device *dev, struct device_attribute *attr,
>> +			const char *buf, size_t count)
>> +{
>> +	struct w83627ehf_data *data = w83627ehf_update_device(dev);
>> +	struct i2c_client *client = to_i2c_client(dev);
>> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
>> +	int nr = sensor_attr->index;
>> +	u32 val = SENSORS_LIMIT(simple_strtoul(buf, NULL, 10), 0, 3);
>> +	u16 reg = w83627ehf_read_value(client, W83627EHF_REG_PWM_ENABLE[nr]);
>> +
>> +	if (data->pwm_enable[nr] != val) {
>> +		if (!val) {
>> +			store_pwm(dev, attr, "255", 3);
>> +			mutex_lock(&data->update_lock);
>> +			data->pwm_enable[nr] = val;
>> +		}
>> +		else {
>> +			mutex_lock(&data->update_lock);
>> +			data->pwm_enable[nr] = val;
>> +			val--;
>> +		}
>> +		reg = (reg & ~(11 << W83627EHF_PWM_ENABLE_SHIFT[nr]));
> 
> What is this "11" falling from the sky?
> 
>> +		reg |= val << W83627EHF_PWM_ENABLE_SHIFT[nr];
>> +		w83627ehf_write_value(client, W83627EHF_REG_PWM_ENABLE[nr],
>> +					reg);
>> +		mutex_unlock(&data->update_lock);
>> +	}
>> +	return count;
>> +}
> 
> I just realized that you are emulating pwm_enable = 0. I guess the chip
> doesn't support it? Then you don't want to implement it in the driver.
> This emulation makes your code much more complex with no gain at all.
> Drivers must implement what the chip support, they must NOT emulate
> features.

On the other hand we want consistent interface... I thought that either the file
could be present or not, but if present it MUST support 0 and 1 at least.

I think you told me that that -EINVAL should do it and I can revert the
emulation stuff back. But I think we should FIX this in docs so it cannot be a
matter of interpretation.

I will fix  the minor stuff too.

> * use of w83627ehf_update_device
> * use of SENSORS_LIMIT
> * locking
> * conditional register writes

As for the conditional register writes. You just told me to save one read and
you dont want the conditional writes? hmmm?

> all around the place, then test the patch again, and then I'll review
> it again.
OK

Many thanks for the review. As I already told you we need some kind of wiki page
describing all those pitfalls so I can easily check the stuff and dont forget
about anything..

Anyone will help me?

Regards
Rudolf

[lm-sensors] [PATCH] W83627EHF driver update

[Jean Delvare]

Hi Rudolf,

> > The name is not very well chosen, these registers can hold both a target
> > temperature or a target fan speed depending on the control mode (even
> > if we don't support the latter at the moment.) What about just
> > W83627EHF_REG_TARGET?
> 
> I think this is in the plan for future driver upgrade.

Let's get the name right now, so that this future driver upgrade
doesn't have to change it everywhere.

> > > +	struct i2c_client *client = to_i2c_client(dev);
> > > +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> > > +	int nr = sensor_attr->index;
> > > +	u32 val = SENSORS_LIMIT(simple_strtoul(buf, NULL, 10), 0, 1);
> > 
> > Using SENSOR_LIMIT here doesn't make sense, as we're not dealing with a
> > range. Better return -EINVAL if value is neither 0 nor 1.
> 
> Well yes why not, maybe we should write a note to a docs?

I think the best we can do is to make it right in our drivers, because
this is where people look when writing new drivers. If you want to
extend the sysfs-interface documentation file to describe how invalid
values should be handled (on write), feel free to do so.

> > I just realized that you are emulating pwm_enable = 0. I guess the chip
> > doesn't support it? Then you don't want to implement it in the driver.
> > This emulation makes your code much more complex with no gain at all.
> > Drivers must implement what the chip support, they must NOT emulate
> > features.
> 
> On the other hand we want consistent interface... I thought that either the file
> could be present or not, but if present it MUST support 0 and 1 at least.
> 
> I think you told me that that -EINVAL should do it and I can revert the
> emulation stuff back. But I think we should FIX this in docs so it cannot be a
> matter of interpretation.

Patch welcome :)

> As for the conditional register writes. You just told me to save one read and
> you dont want the conditional writes? hmmm?

Saving one read in a safe way, in a function which will be called
hundreds of times in the driver lifetime, is worth it. Saving a write
in a function which will be called only a few times in the driver
lifetime, in an unsafe way, is not good.

> Many thanks for the review. As I already told you we need some kind of wiki page
> describing all those pitfalls so I can easily check the stuff and dont forget
> about anything..
> 
> Anyone will help me?

I think I already helped much by reviewing so much code in the past.
Everything I said there is a candidate for your page if you want to
write it.

-- 
Jean Delvare

[lm-sensors] [PATCH] W83627EHF driver update

[Rudolf Marek]

Hi,

> Let's get the name right now, so that this future driver upgrade
> doesn't have to change it everywhere.

OK I changed it. Three places total ;)

>>>> +	struct i2c_client *client = to_i2c_client(dev);
>>>> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
>>>> +	int nr = sensor_attr->index;
>>>> +	u32 val = SENSORS_LIMIT(simple_strtoul(buf, NULL, 10), 0, 1);
>>> Using SENSOR_LIMIT here doesn't make sense, as we're not dealing with a
>>> range. Better return -EINVAL if value is neither 0 nor 1.
>> Well yes why not, maybe we should write a note to a docs?
> 
> I think the best we can do is to make it right in our drivers, because
> this is where people look when writing new drivers. If you want to
> extend the sysfs-interface documentation file to describe how invalid
> values should be handled (on write), feel free to do so.

Ok

Well they are handled at least with two different ways.

1) -EINVAL
2) if out of bounds - nearest bound is written

>>> I just realized that you are emulating pwm_enable = 0. I guess the chip
>>> doesn't support it? Then you don't want to implement it in the driver.
>>> This emulation makes your code much more complex with no gain at all.
>>> Drivers must implement what the chip support, they must NOT emulate
>>> features.
>> On the other hand we want consistent interface... I thought that either the file
>> could be present or not, but if present it MUST support 0 and 1 at least.
>>
>> I think you told me that that -EINVAL should do it and I can revert the
>> emulation stuff back. But I think we should FIX this in docs so it cannot be a
>> matter of interpretation.
> 
> Patch welcome :)

OK

>> Anyone will help me?
> 
> I think I already helped much by reviewing so much code in the past.
> Everything I said there is a candidate for your page if you want to
> write it.

Yes I had it in mind. But this was not addressed to you. But to others round here...

Regards
Rudolf

[lm-sensors] [PATCH] W83627EHF driver update

[Jean Delvare]

Rudolf,

> > > > Using SENSOR_LIMIT here doesn't make sense, as we're not dealing with a
> > > > range. Better return -EINVAL if value is neither 0 nor 1.
> > >
> > > Well yes why not, maybe we should write a note to a docs?
> > 
> > I think the best we can do is to make it right in our drivers, because
> > this is where people look when writing new drivers. If you want to
> > extend the sysfs-interface documentation file to describe how invalid
> > values should be handled (on write), feel free to do so.
> 
> Ok
> 
> Well they are handled at least with two different ways.
> 
> 1) -EINVAL
> 2) if out of bounds - nearest bound is written

First for discrete values, second for ranges. This makes sense, and is
still simple enough so everyone should be able to get it.

-- 
Jean Delvare

[lm-sensors] [PATCH] W83627EHF driver update

[Rudolf Marek]

Hi all,

Here is some updated version for the test. It fixes the stuff Jean found. I want 
to read it once again I'm not falling asleep while reading it. It works for me 
and I think it should work for you all too.

Regards
Rudolf


-------------- next part --------------
A non-text attachment was scrubbed...
Name: ehf_next_try4.patch
Type: text/x-patch
Size: 16859 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060614/cc694273/attachment-0001.bin 

[lm-sensors] [PATCH] W83627EHF driver update

[David Hubbard]

Hi Rudolf,

+		for (i = 0; i < 4; i++) {
+			int pwmcfg, tolerance;
+			if (i != 1) {
+				pwmcfg = w83627ehf_read_value(client,
+						W83627EHF_REG_PWM_ENABLE[i]);

Can we put a comment here explaining the if condition? Suggestion:
/* W83627EHF_REG_PWM_ENABLE[1] = W83627EHF_REG_PWM_ENABLE[0]. The
values of pwmcfg and tolerance are not changed from i=0 to i=1 */


+store_pwm_enable(struct device *dev, struct device_attribute *attr,
+			const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct w83627ehf_data *data = i2c_get_clientdata(client);
+	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+	int nr = sensor_attr->index;
+	u32 val = SENSORS_LIMIT(simple_strtoul(buf, NULL, 10), 0, 3);
+	u16 reg;
+
+	if (!val)	/* pwm off not supported */
+		return -EINVAL;

We return -EINVAL in store_pwm_mode() if val is invalid. We shouldn't
use SENSORS_LIMIT here either. Can we return -EINVAL instead? Also,
these are the PWM modes the driver currently supports:
1 - manual PWM control
2 - thermal cruise

These PWM modes will soon be supported:
3 - RPM cruise
4 - Smart Fan III

So should val be limited to 1, 2, 3, or should val be limited to 1, 2?

Suggested change:
	u32 val = simple_strtoul(buf, NULL, 10);
	if (!val || val > 3)	/* mode 0 (disabled) not supported */
		return -EINVAL;

I'm testing it on my system right now. I will let you know if there
are problems.

Thanks,
David

On 6/14/06, Rudolf Marek <r.marek at sh.cvut.cz> wrote:
> Hi all,
>
> Here is some updated version for the test. It fixes the stuff Jean found. I want
> to read it once again I'm not falling asleep while reading it. It works for me
> and I think it should work for you all too.

[lm-sensors] [PATCH] W83627EHF driver update

[Sylvain Jeaugey]

On Thu, 15 Jun 2006, David Hubbard wrote:
> I'm testing it on my system right now. I will let you know if there
> are problems.
Everything's OK here.

Manual fan control works fine.
David, is there anything else I should test ?
[ pwm2 is controling fan2. Other pwm don't have any effect (since the 
beginning). ]

Sylvain

> On 6/14/06, Rudolf Marek <r.marek at sh.cvut.cz> wrote:
>> Hi all,
>>
>> Here is some updated version for the test. It fixes the stuff Jean found. I want
>> to read it once again I'm not falling asleep while reading it. It works for me
>> and I think it should work for you all too.
>
> _______________________________________________
> lm-sensors mailing list
> lm-sensors at lm-sensors.org
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
>

[lm-sensors] [PATCH] W83627EHF driver update

[David Hubbard]

Hi Sylvain,

> Everything's OK here.
>
> Manual fan control works fine.
> David, is there anything else I should test ?

I think that's all.

> [ pwm2 is controling fan2. Other pwm don't have any effect (since the
> beginning). ]
>
> Sylvain

David

[lm-sensors] [PATCH] W83627EHF driver update

[Rudolf Marek]

Hi all,

Here comes the updated patch

This patch adds long-awaited suport for automatic fan modes. Based on the work
of Yuan Mu from Winbond, I finished the support with the great help of David
Hubbard. The documenation update will follow.

Signed-Off-By: Yuan Mu <Ymu at Winbond.com.tw>
Signed-Off-By: Rudolf Marek <r.marek at sh.cvut.cz>
Signed-Off-By: David Hubbard <david.c.hubbard at gmail.com>

David, I fix the comment you had, thanks! Also I ripped out two more register
exports (sf3 fan_max and fan_step). This will be neccessary to put back next round.

Regards
Rudolf
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ehf_next_try5.patch
Type: text/x-patch
Size: 16043 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060616/f0e6212b/attachment.bin 

[lm-sensors] [PATCH] W83627EHF driver update

[David Hubbard]

Hi Rudolf,

On 6/16/06, Rudolf Marek <r.marek at sh.cvut.cz> wrote:
> Hi all,
>
> Here comes the updated patch
>
> This patch adds long-awaited suport for automatic fan modes. Based on the work
> of Yuan Mu from Winbond, I finished the support with the great help of David
> Hubbard. The documenation update will follow.
>
> Signed-Off-By: Yuan Mu <Ymu at Winbond.com.tw>
> Signed-Off-By: Rudolf Marek <r.marek at sh.cvut.cz>
> Signed-Off-By: David Hubbard <david.c.hubbard at gmail.com>
>
> David, I fix the comment you had, thanks! Also I ripped out two more register
> exports (sf3 fan_max and fan_step). This will be neccessary to put back next round.
>
> Regards
> Rudolf

Just signing off. It looks fine to me.

David

[lm-sensors] [PATCH] W83627EHF driver update

[Jean Delvare]

Hi Rudolf,

> Here comes the updated patch
> 
> This patch adds long-awaited suport for automatic fan modes. Based on the work
> of Yuan Mu from Winbond, I finished the support with the great help of David
> Hubbard. The documenation update will follow.

I'd rather take this and the documentation patch together. We all know
what happens to patches which "will follow" ;)

> David, I fix the comment you had, thanks! Also I ripped out two more register
> exports (sf3 fan_max and fan_step). This will be neccessary to put back next round.

It's in a much better shape now, thanks to both of you for the work. I
would still have some comments though:

> @@ -416,6 +462,35 @@ static struct w83627ehf_data *w83627ehf_
>  			}
>  		}
>  
> +		for (i = 0; i < 4; i++) {
> +			int pwmcfg, tolerance;
> +			/* pwmcfg, tolarance mapped for i=0, i=1 to same reg */
> +			if (i != 1) {
> +				pwmcfg = w83627ehf_read_value(client,
> +						W83627EHF_REG_PWM_ENABLE[i]);
> +				tolerance = w83627ehf_read_value(client,
> +						W83627EHF_REG_TOLERANCE[i]);
> +			}

Unfortunately my compiler (gcc 3.3.6) complains:

drivers/hwmon/w83627ehf.c: In function `w83627ehf_update_device':
drivers/hwmon/w83627ehf.c:466: warning: `pwmcfg' might be used uninitialized in this function
drivers/hwmon/w83627ehf.c:466: warning: `tolerance' might be used uninitialized in this function

The compiler is wrong, but we can't leave warnings in the code. I guess
this is the reason why you where initializing your temporary variable
in the first place, I'm sorry for asking you to change that.

At this point I'm not too sure what is worst, two redundant inb_p, or
two initializations and four comparisons. I let you decide, as long as
there is no warning. If you go for initialization, please add a comment
explaining it's there to prevent a compiler warning.

> +static ssize_t
> +store_target_temp(struct device *dev, struct device_attribute *attr,
> +			const char *buf, size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct w83627ehf_data *data = i2c_get_clientdata(client);
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> +	int nr = sensor_attr->index;
> +	u8 val = temp1_to_reg(
> +			SENSORS_LIMIT(simple_strtoul(buf, NULL, 10), 0, 127000));
> +
> +	mutex_lock(&data->update_lock);
> +	data->target_temp[nr] = val;
> +	w83627ehf_write_value(client, W83627EHF_REG_TARGET[nr], val);
> +	mutex_unlock(&data->update_lock);
> +	return count;
> +}
> +
> +static ssize_t
> +store_tolerance(struct device *dev, struct device_attribute *attr,
> +			const char *buf, size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct w83627ehf_data *data = i2c_get_clientdata(client);
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> +	int nr = sensor_attr->index;
> +	u16 reg;
> +	/* Limit the temp to 0C - 15C */
> +	u8 val = temp1_to_reg(
> +			SENSORS_LIMIT(simple_strtoul(buf, NULL, 10), 0, 15000));
> +
> +	mutex_lock(&data->update_lock);
> +	reg = w83627ehf_read_value(client, W83627EHF_REG_TOLERANCE[nr]);
> +	data->tolerance[nr] = val;
> +	if (nr = 1)
> +		reg = (reg & 0x0f) | (val << 4);
> +	else
> +		reg = (reg & 0xf0) | val;
> +	w83627ehf_write_value(client, W83627EHF_REG_TOLERANCE[nr], reg);
> +	mutex_unlock(&data->update_lock);
> +	return count;
> +}

These combinations of temp1_to_reg and SENSORS_LIMIT are inefficient. I
suggest that you instead add two parameters (min and max values) to
temp1_to_reg(). As it is inlined there will be no performance penalty
and the code will look better too.

> +/* Smart Fan registers */
> +
> +#define fan_functions(reg, REG) \
> +static ssize_t show_##reg(struct device *dev, struct device_attribute *attr, \
> +		       char *buf) \
> +{ \
> +	struct w83627ehf_data *data = w83627ehf_update_device(dev); \
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr); \
> +	int nr = sensor_attr->index; \
> +	return sprintf(buf, "%d\n", data->reg[nr]); \
> +}\
> +static ssize_t \
> +store_##reg(struct device *dev, struct device_attribute *attr, \
> +			    const char *buf, size_t count) \
> +{\
> +	struct i2c_client *client = to_i2c_client(dev); \
> +	struct w83627ehf_data *data = i2c_get_clientdata(client); \
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr); \
> +	int nr = sensor_attr->index; \
> +	u32 val = SENSORS_LIMIT(simple_strtoul(buf, NULL, 10), 0, 255); \
> +	mutex_lock(&data->update_lock); \
> +	data->reg[nr] = val; \
> +	w83627ehf_write_value(client, W83627EHF_REG_##REG[nr],  val); \
> +	mutex_unlock(&data->update_lock); \
> +	return count; \
> +}
> +
> +fan_functions(fan_min_output, FAN_MIN_OUTPUT)
> +
> +#define fan_time_functions(reg, REG) \
> +static ssize_t show_##reg(struct device *dev, struct device_attribute *attr, \
> +				char *buf) \
> +{ \
> +	struct w83627ehf_data *data = w83627ehf_update_device(dev); \
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr); \
> +	int nr = sensor_attr->index; \
> +	return sprintf(buf, "%d\n", \
> +			step_time_from_reg(data->reg[nr], data->pwm_mode[nr])); \
> +} \
> +\
> +static ssize_t \
> +store_##reg(struct device *dev, struct device_attribute *attr, \
> +			const char *buf, size_t count) \
> +{ \
> +	struct i2c_client *client = to_i2c_client(dev); \
> +	struct w83627ehf_data *data = i2c_get_clientdata(client); \
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr); \
> +	int nr = sensor_attr->index; \
> +	u8 val = step_time_to_reg(simple_strtoul(buf, NULL, 10), \
> +					data->pwm_mode[nr]); \
> +	mutex_lock(&data->update_lock); \
> +	data->reg[nr] = val; \
> +	w83627ehf_write_value(client, W83627EHF_REG_##REG[nr], val); \
> +	mutex_unlock(&data->update_lock); \
> +	return count; \
> +} \
> +
> +fan_time_functions(fan_stop_time, FAN_STOP_TIME)

Why do you define these two macros, if you then use them only once?

>  /*
>   * Driver and client management
>   */
> @@ -810,6 +1162,7 @@ static int w83627ehf_detect(struct i2c_a
>  	struct i2c_client *client;
>  	struct w83627ehf_data *data;
>  	struct device *dev;
> +	u8 cr24, cr29;
>  	int i, err = 0;
>  
>  	if (!request_region(address + REGION_OFFSET, REGION_LENGTH,
> @@ -848,13 +1201,22 @@ static int w83627ehf_detect(struct i2c_a
>  		data->fan_min[i] = w83627ehf_read_value(client,
>  				   W83627EHF_REG_FAN_MIN[i]);
>  
> +	/* fan4 and fan5 share some pins with the GPIO and serial flash */
> +
> +	superio_enter();
> +	/* flash needs to be disabled for AUXFANIN1/fan5 -> 00 */
> +	cr24 = superio_inb(0x24) & 0x2; 
> +	cr29 = superio_inb(0x29) & 0x6; /* CPUFANIN1/fan4 ->00 */
> +	superio_exit();

What does "-> 00" mean? These comments could be improved (maybe merged
with the one right below?) I would also prefer that the masking is done
when testing, rather than when reading the registers, else your
variable names don't make much sense.

> +	
>  	/* It looks like fan4 and fan5 pins can be alternatively used
>  	   as fan on/off switches */
> +	
>  	data->has_fan = 0x07; /* fan1, fan2 and fan3 */
>  	i = w83627ehf_read_value(client, W83627EHF_REG_FANDIV1);
> -	if (i & (1 << 2))
> +	if ((i & (1 << 2)) && (!cr29))
>  		data->has_fan |= (1 << 3);
> -	if (i & (1 << 0))
> +	if ((i & (1 << 0)) && (!cr24))
>  		data->has_fan |= (1 << 4);

>  	for (i = 0; i < 10; i++)
>  		device_create_file_in(dev, i);
>  
> -	for (i = 0; i < 5; i++) {
> +	for (i = 0; i < 5; i++)
>  		if (data->has_fan & (1 << i))
>  			device_create_file_fan(dev, i);

Please don't change this.

> +
> +	for (i = 0; i < 4; i++) {
> +		if (data->has_fan & (1 << i)) {
> +			device_create_file_pwm(dev, i);
> +			data->pwm_mode[i] = 1; /* initialize in PWM mode */
> +		}
>  	}

This initialization doesn't make much sense, it is arbitrary and the
value will be overwritten on first update anyway. What's the purpose?

Thanks,
-- 
Jean Delvare

[lm-sensors] [PATCH] W83627EHF driver update

[David Hubbard]

Hi Jean, Rudolf,

On 6/20/06, Jean Delvare <khali at linux-fr.org> wrote:
> This initialization doesn't make much sense, it is arbitrary and the
> value will be overwritten on first update anyway. What's the purpose?

The value can't be zero, so I initialized it to 1. This is also a
holdover from emulating 0 (disabled). Let's take it out and test it
that way. What do you think, Rudolf?

David

[lm-sensors] [PATCH] W83627EHF driver update

[Rudolf Marek]

Hi all,

Here comes fixed patch, I left the macro there because it will be used in future
update. Rest was fixed.

I'm attaching the documentation patch too. David, please can you read it and
check it? (I will sign that off when we know that there are no mistakes)

This patch adds long-awaited support for automatic fan modes. Based on the work
of Yuan Mu from Winbond, I finished the support with the great help of David
Hubbard. The documentation update will follow.

Signed-Off-By: Yuan Mu <Ymu at Winbond.com.tw>
Signed-Off-By: Rudolf Marek <r.marek at sh.cvut.cz>
Signed-Off-By: David Hubbard <david.c.hubbard at gmail.com>

Regards
Rudolf
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ehf_docs.patch
Type: text/x-patch
Size: 3393 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060624/922b5044/attachment.bin 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ehf_next_try7.patch
Type: text/x-patch
Size: 16868 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060624/922b5044/attachment-0001.bin 

[lm-sensors] [PATCH] W83627EHF driver update

[Jean Delvare]

Hi Rudolf,

> Here comes fixed patch, I left the macro there because it will be used in future
> update. Rest was fixed.

I am happy with the code now, but there is a problem with the sysfs file
names, see below.

> I'm attaching the documentation patch too. David, please can you read it and
> check it? (I will sign that off when we know that there are no mistakes)

My review of it:

> diff -uprN a/Documentation/hwmon/w83627ehf linux-2.6.17-rc6/Documentation/hwmon/w83627ehf
> --- a/Documentation/hwmon/w83627ehf	1970-01-01 01:00:00.000000000 +0100
> +++ linux-2.6.17-rc6/Documentation/hwmon/w83627ehf	2006-06-24 18:33:31.839223250 +0200
> @@ -0,0 +1,75 @@
> +Kernel driver w83627ehf
> +===========> +
> +Supported chips:
> +  * Winbond W83627EHF/EHG/EF/EG (ISA accesses ONLY)

s/accesses/access/

EF and EG verions are not actually supported, see below.

> +    Prefix: 'w83627ehf'
> +    Addresses scanned: ISA address retrieved from Super I/O registers
> +    Datasheet: http://www.winbond-usa.com/products/winbond_products/pdfs/
> +                      /PCIC/W83627EHF_%20W83627EHGb.pdf

Please don't split the URL.

Weird file name, BTW :(

> +
> +Authors:
> +        Jean Delvare <khali at linux-fr.org>,
> +        Yuan Mu <Ymu at Winbond.com.tw>,
> +        Rudolf Marek <r.marek at sh.cvut.cz>
> +
> +Description
> +-----------
> +
> +This driver implements support for the Winbond W83627EF, W83627EHF,
> +W83627EHG and W83627EG super I/O chips. We will refer to them
> +collectively as Winbond chips.

Not correct. The W83627EF and W83627EG do not have the hardware
monitoring features so the driver is useless for them.

> +
> +The chips implement three temperature sensors, five fan rotation 
> +speed sensors, ten analog voltage sensors, alarms with beep warnings (control
> +unimplemented), and some automatic fan regulation strategies.

And manual fan speed control too.

> +
> +Temperatures are measured in degrees Celsius and measurement resolution is 1
> +degC for temp1 and 0.5 degC for temp2 and temp3. An alarm is triggered when
> +the temperature gets higher than the Overtemperature Shutdown value; it stays
> +on until the temperature falls below the Hysteresis value.

"Overtemperature Shutdown" is an old term, I think it was used in the
LM75 datasheet, but doesn't make much sense here. The system is usually
NOT going to shutdown if this limit is crossed.

> +
> +Fan rotation speeds are reported in RPM (rotations per minute). An alarm is
> +triggered if the rotation speed has dropped below a programmable limit. Fan
> +readings can be divided by a programmable divider (1, 2, 4, 8, 16, 32, 64 or
> +128) to give the readings more range or accuracy. Some fans might not
> +be present because they share pins with other funtions.

This driver implements automatic fan clock divider switching, so the
divider values are hidden from the user.

> +
> +Voltage sensors (also known as IN sensors) report their values in millivolts.
> +An alarm is triggered if the voltage has crossed a programmable minimum
> +or maximum limit.
> +
> +The driver supports automatic fan control mode known as Thermal Cruise.
> +This mode works for fan1-fan4. Programmable mapping of the temperatures
> +to fans is not implemented, BIOS should have done it.

We should at least have read-only files to let the user know the
mapping.

It would be nice to describe what is the "thermal cruise" mode.

> +
> +/sys files
> +----------
> +
> +pwm[1-4] - this file stores PWM duty cycle or DC value (fan speed) in range:
> +	0 (stop) to 255 (full)
> +
> +pwm[1-4]_enable - this file controls mode of fan/temperature control:
> +	* 1 Manual Mode, write to pwm file any value 0-255 (fullspeed)

s/fullspeed/full speed/

> +        * 2 Thermal Cruise

Broken indentation.

> +
> +Thermal Cruise mode
> +--------------------
> +
> +If the temperature is in its range, defined with:

s/its/the/

> +
> +temp[1-4]_target    - set target temperature, unit miliC (range 0 - 127000)
> +temp[1-4]_tolerance - tolerance, unit miliC (range 0 - 15000)

Unknown unit "miliC".

I'm just realizing that these names are quite wrong. For example
temp1_target may not be related to temp1 but to a different temperature
sensor depending on the temp/fan mapping. The "1" here really means
pwm1, not temp1. So these files should be named pwm[1-4]_target_temp
and pwm[1-4]_tolerance_temp, respectively.

BTW, these file names should also be added to sysfs-interface.

> +
> +There are no changes to fan speed. Once the temperature leaves the interval,
> +fan speed increases (temp is higher) or decreases if lower than desired. 
> +There are defined steps and times, but not exported by the driver yet.
> +
> +fan[1-4]_min_output - minimum fan speed (range 1 - 255), when the temperature
> +                      is bellow defined range.

Should be named pwm[1-4]_min.

s/bellow/below/

> +fan[1-4]_stop_time  - how many milliseconds [ms] must elapse to switch
> +                      corresponding fan off. (when the temperature was bellow
> +                      defined range).
> +

Should be named pwm[1-4]_stop_time.

> +It seems last two funtions are exclusive, and are controlled with register 0x12.
> +(Should be programed by BIOS)
> 

But the driver uncondionally creates both files? This needs to be
investigated, and fixed (either the driver or the documentation.)

Thanks,
-- 
Jean Delvare

[lm-sensors] [PATCH] W83627EHF driver update

[Rudolf Marek]

Hi all,

> I am happy with the code now, but there is a problem with the sysfs file
> names, see below.

Ok. I'm attaching fixed versions of both.

> And manual fan speed control too.

Ok I will mention, although I considered this a special case of automatic regulation


> We should at least have read-only files to let the user know the
> mapping.

The mapping is trivial and programmable for fan4 only.
I fixed the docs.

> I'm just realizing that these names are quite wrong. For example
> temp1_target may not be related to temp1 but to a different temperature
> sensor depending on the temp/fan mapping. The "1" here really means
> pwm1, not temp1. So these files should be named pwm[1-4]_target_temp
> and pwm[1-4]_tolerance_temp, respectively.

Yes correct. Fixed.


> BTW, these file names should also be added to sysfs-interface.

If we consider this as standard for some chips. We would need some mapping
standard too (Yuan did some file for w83793)

> But the driver uncondionally creates both files? This needs to be
> investigated, and fixed (either the driver or the documentation.)

I fixed the note. The files are used in SF3 iirc, so we might have them.

This patch adds long-awaited support for automatic fan modes. Based on the work
of Yuan Mu from Winbond, I finished the support with the great help of David
Hubbard. Many thanks goes to Jean Delvare, for his reviews and patience.
The documentation update will follow.

Signed-Off-By: Yuan Mu <Ymu at Winbond.com.tw>
Signed-Off-By: Rudolf Marek <r.marek at sh.cvut.cz>
Signed-Off-By: David Hubbard <david.c.hubbard at gmail.com>

Regards
Rudolf
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ehf_next_try8.patch
Type: text/x-patch
Size: 16860 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060625/878aa562/attachment.bin 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ehf_docs1.patch
Type: text/x-patch
Size: 3706 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060625/878aa562/attachment-0001.bin 

[lm-sensors] [PATCH] W83627EHF driver update

[Jean Delvare]

Hi Rudolf,	

> > I am happy with the code now, but there is a problem with the sysfs file
> > names, see below.
> 
> Ok. I'm attaching fixed versions of both.
> 
> > And manual fan speed control too.
> 
> Ok I will mention, although I considered this a special case
> of automatic regulation
> 
> > We should at least have read-only files to let the user know the
> > mapping.
> 
> The mapping is trivial and programmable for fan4 only.
> I fixed the docs.
> 
> > I'm just realizing that these names are quite wrong. For example
> > temp1_target may not be related to temp1 but to a different temperature
> > sensor depending on the temp/fan mapping. The "1" here really means
> > pwm1, not temp1. So these files should be named pwm[1-4]_target_temp
> > and pwm[1-4]_tolerance_temp, respectively.
> 
> Yes correct. Fixed.

Thanks.

> If we consider this as standard for some chips. We would need some mapping
> standard too (Yuan did some file for w83793)

The temp <-> pwm mapping standard already exists.

> > But the driver uncondionally creates both files? This needs to be
> > investigated, and fixed (either the driver or the documentation.)
> 
> I fixed the note. The files are used in SF3 iirc, so we might have them.

I'm still not exactly happy. Documenting that we implemented things in
a possibly non-working way is better than not documenting it, agreed,
but even better would be to have things working in the first place.

Also on the to do list is the export of the fan <-> temperature
mapping, at least read only, as I asked before already. For now pwm4 is
useless in Temperature Cruise mode, as you don't know what temperature
channel it is working with.

Anyway, I will apply these patches now, as we have gone trough many
review cycles already. I made some fixes to the documentation patch to
fix typos and reword one sentence. Any future change to the w83627ehf
driver or its documentation should be incremental on top of these
patches.

Thanks,
-- 
Jean Delvare

[lm-sensors] [PATCH] W83627EHF driver update

[Rudolf Marek]

Hi Jean,

> The temp <-> pwm mapping standard already exists.

Aha ok. pwm[1-*]_auto_channels_temp I guess.

>>> But the driver uncondionally creates both files? This needs to be
>>> investigated, and fixed (either the driver or the documentation.)
>> I fixed the note. The files are used in SF3 iirc, so we might have them.
> 
> I'm still not exactly happy. Documenting that we implemented things in
> a possibly non-working way is better than not documenting it, agreed,
> but even better would be to have things working in the first place.

This stuff is so complex that even if you think you got it right, then some bits 
will make even bigger mess than it was.

> Also on the to do list is the export of the fan <-> temperature
> mapping, at least read only, as I asked before already. For now pwm4 is
> useless in Temperature Cruise mode, as you don't know what temperature
> channel it is working with.

pwm4 is disabled on my board so I cannot tell which is which. I remmber that on 
david board it is disabled too.

I will put this into my queue, do it soon.

> Anyway, I will apply these patches now, as we have gone trough many
> review cycles already. I made some fixes to the documentation patch to
> fix typos and reword one sentence. Any future change to the w83627ehf
> driver or its documentation should be incremental on top of these
> patches.

Thank you very much. As you know I'm not in exactly good situation so I hope I 
will do better next time when things are sorted out.

Regards
Rudolf