[lm-sensors] [PATCH] Use fan divisor bits from vbat register of w83791d

[Jean Delvare]

Hi Marc,

On Thu, 15 May 2008 00:52:13 +0200, Marc Hulsman wrote:
> I will take over maintenance of the w83791d driver. Thanks Charles for you 
> work on it until now, I found it really useful!
> 
> Also thanks for your help with finalizing the following patch. It takes into 
> account an extra bit for the fan divisor of fans 1-3, enabling them to have 
> fan divisors higher than 8. This also solves a bug where the reported fan 
> speed is too high if the chip is initialized with a fan divisor larger than 
> 8. 

Good catch. I didn't know that the w83791d driver had this bug. Oddly
enough, the Linux 2.4 driver had this right.

> ---
> Update w83791d with fan bits in vbat mon register (7.48 of the
> datasheet). This change allows all fans to have a divisor of 128, 
> and fixes a problem with incorrectly reported fan speeds. 
> 
> Signed-off-by: Marc Hulsman <m.hulsman at tudelft.nl>

You patch looks good, I have a few comments though:

> 
> ---
>  drivers/hwmon/w83791d.c     |   26 ++++++++++++++++++++++----
>  Documentation/hwmon/w83791d |    6 +++---
>  2 files changed, 25 insertions(+), 7 deletions(-)
> ---
> diff -uNrp linux-2.6.26-rc1-git6/drivers/hwmon/w83791d.c 
> linux-2.6.26-rc1-git6-modified/drivers/hwmon/w83791d.c
> --- linux-2.6.26-rc1-git6/drivers/hwmon/w83791d.c	2008-05-03 
> 20:59:44.000000000 +0200
> +++ linux-2.6.26-rc1-git6-modified/drivers/hwmon/w83791d.c	2008-05-08 
> 19:08:55.000000000 +0200
> @@ -233,11 +233,9 @@ static u8 fan_to_reg(long rpm, int div)
>  static u8 div_to_reg(int nr, long val)
>  {
>  	int i;
> -	int max;
>  
> -	/* first three fan's divisor max out at 8, rest max out at 128 */
> -	max = (nr < 3) ? 8 : 128;
> -	val = SENSORS_LIMIT(val, 1, max) >> 1;
> +	/* fan divisors max out at 128 */
> +	val = SENSORS_LIMIT(val, 1, 128) >> 1;
>  	for (i = 0; i < 7; i++) {
>  		if (val == 0)
>  			break;
> @@ -519,6 +517,7 @@ static ssize_t store_fan_div(struct devi
>  	unsigned long min;
>  	u8 tmp_fan_div;
>  	u8 fan_div_reg;
> +	u8 vbat_reg;
>  	int indx = 0;
>  	u8 keep_mask = 0;
>  	u8 new_shift = 0;
> @@ -570,6 +569,16 @@ static ssize_t store_fan_div(struct devi
>  	w83791d_write(client, W83791D_REG_FAN_DIV[indx],
>  				fan_div_reg | tmp_fan_div);
>  
> +	/* Bit 2 of fans 0-2 is stored in the vbat register (bits 5-7) */
> +	if(nr < 3) {

Coding style: please add a space between "if" and "(". Note that you
can (and should) run scripts/checkpatch.pl on your patches before you
submit them, it'll catch most of the coding style mistakes.

> +		keep_mask = ~(1 << (nr + 5));
> +		vbat_reg = w83791d_read(client, W83791D_REG_VBAT) 

There's a trailing space at the end of this line, please remove it.

> +				& keep_mask;
> +		tmp_fan_div = (data->fan_div[nr] << (3 + nr)) & ~keep_mask;
> +		w83791d_write(client, W83791D_REG_VBAT,
> +				vbat_reg | tmp_fan_div);
> +	}
> +
>  	/* Restore fan_min */
>  	data->fan_min[nr] = fan_to_reg(min, DIV_FROM_REG(data->fan_div[nr]));
>  	w83791d_write(client, W83791D_REG_FAN_MIN[nr], data->fan_min[nr]);
> @@ -1245,6 +1254,7 @@ static struct w83791d_data *w83791d_upda
>  	struct w83791d_data *data = i2c_get_clientdata(client);
>  	int i, j;
>  	u8 reg_array_tmp[3];
> +	u8 vbat_reg;
>  
>  	mutex_lock(&data->update_lock);
>  
> @@ -1288,6 +1298,14 @@ static struct w83791d_data *w83791d_upda
>  						W83791D_REG_TEMP1[i]);
>  		}
>  
> +		/* The fan divisor for fans 0-2 get bit 2 from
> +		   bits 5-7 respectively of vbat register */
> +		vbat_reg = w83791d_read(client, W83791D_REG_VBAT);
> +		for (i = 0; i < 3; i++) {
> +			data->fan_div[i] |= ((vbat_reg &
> +						(1 << (5 + i))) >> (3 + i));

Don't you think that:

			data->fan_div[i] |= (vbat_reg >> (3 + i)) & 0x04;

would be more readable?

> +		}
> +

I would also suggest that you move this block a bit earlier in the
function. You inserted it in the middle of the temperature register
reading, while I think it would be more logical to have it right after
the rest of the code filling the fan_div values.

>  		/* Update the rest of the temperature sensors */
>  		for (i = 0; i < 2; i++) {
>  			for (j = 0; j < 3; j++) {
> diff -uNrp linux-2.6.26-rc1-git6/Documentation/hwmon/w83791d 
> linux-2.6.26-rc1-git6-modified/Documentation/hwmon/w83791d
> --- linux-2.6.26-rc1-git6/Documentation/hwmon/w83791d	2008-05-03 
> 20:59:44.000000000 +0200
> +++ linux-2.6.26-rc1-git6-modified/Documentation/hwmon/w83791d	2008-05-08 
> 18:52:46.000000000 +0200
> @@ -22,6 +22,7 @@ Credits:
>  
>  Additional contributors:
>      Sven Anders <anders at anduras.de>
> +    Marc Hulsman <m.hulsman at tudelft.nl>
>  
>  Module Parameters
>  -----------------
> @@ -67,9 +68,8 @@ on until the temperature falls below the
>  
>  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 for fan 1/2/3
> -and 1, 2, 4, 8, 16, 32, 64 or 128 for fan 4/5) to give the readings more
> -range or accuracy.
> +readings can be divided by a programmable divider (1, 2, 4, 8, 16,
> +32, 64 or 128 for all fans) to give the readings more range or accuracy.
>  
>  Voltage sensors (also known as IN sensors) report their values in millivolts.
>  An alarm is triggered if the voltage has crossed a programmable minimum

The rest looks fine, nice patch.

Thanks,
-- 
Jean Delvare