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
