[lm-sensors] [PATCH] W83793: ignore the temperature readings when

[lm-sensors] [PATCH] W83793: ignore the temperature readings when

[Rudolf Marek]

Hello

w83793: ignore the temperature readings when its channel is disabled.

Signed-off-by: Gong Jun <jgong at winbond.com>
Signed-off-by: Rudolf Marek <r.marek at assembler.cz>

Looks OK to me, Jean. I just fixed the the indentation, strip version to p1, and
renamed the temp_enable to has_temp, to be coherent with what he have until now.
Please apply.

Thanks,
Rudolf
-------------- next part --------------
A non-text attachment was scrubbed...
Name: hwmon-patch-w83793-temp-display.patch
Type: text/x-diff
Size: 2361 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20070112/fc3294ae/attachment.bin 

[lm-sensors] [PATCH] W83793: ignore the temperature readings

[Jean Delvare]

Hi Rudolf, Gong,

On Fri, 12 Jan 2007 13:30:27 +0100, Rudolf Marek wrote:
> w83793: ignore the temperature readings when its channel is disabled.
> 
> Signed-off-by: Gong Jun <jgong at winbond.com>
> Signed-off-by: Rudolf Marek <r.marek at assembler.cz>
> 
> Looks OK to me, Jean. I just fixed the the indentation, strip version to p1, and
> renamed the temp_enable to has_temp, to be coherent with what he have until now.
> Please apply.

Hm, the patch is incomplete. The following changes are missing:
* The temp files are no longer deleted if an error occurs in
  w83793_detect().
* The temp files are no longer deleted when the driver in unloaded.
* All the temperature registers are still read in w83793_update_device()
  and w83793_update_nonvolatile(). It would be better to skip the
  unused temperature registers to speed up the functions.
* The user still has the possibility to change the temperature sensor
  type. This is confusing because the driver will not react properly
  if the user disables a temperature channel that was previously
  enabled. So we should no longer let the user set the temperature
  sensor types to 0.

I also have minor comments on the code:

> @@ -1320,6 +1325,30 @@
>  		data->has_pwm |= 0x80;
>  	}
>  
> +	/* check the temp1-6 mode */
> +	data->has_temp = 0;

Not needed, kzalloc() zeroed it for you.

> +
> +	tmp = w83793_read_value(client,W83793_REG_TEMP_MODE[0]);
> +
> +	if (tmp & 0x03)
> +		data->has_temp |= 0x01;
> +
> +	if (tmp & 0x0c)
> +		data->has_temp |= 0x02;
> +
> +	if (tmp & 0x30)
> +		data->has_temp |= 0x04;
> +
> +	if (tmp & 0xc0)
> +		data->has_temp |= 0x08;

A few less blank lines would be welcome, as all these instructions are
related.

> +
> +	tmp = w83793_read_value(client,W83793_REG_TEMP_MODE[1]);
> +
> +	if (tmp & 0x01)
> +		data->has_temp |= 0x10;
> +	if (tmp & 0x02)
> +		data->has_temp |= 0x20;
> +
>  	/* Register sysfs hooks */
>  	for (i = 0; i < ARRAY_SIZE(w83793_sensor_attr_2); i++) {
>  		err = device_create_file(dev,

Please add the missing parts, test again and send the updated patch to
me.

Thanks,
-- 
Jean Delvare

[lm-sensors] [PATCH] W83793: ignore the temperature readings

[Rudolf Marek]

Jean Delvare wrote:
> Hi Rudolf, Gong,
> 
> Hm, the patch is incomplete. The following changes are missing:

Hi again, sorry I forgot to think in wider scale completely :/
I'm attaching updated patch.

w83793: ignore the temperature readings when its channel is disabled, ignore 
AMDSI readings.

Signed-off-by: Gong Jun <jgong at winbond.com>
Signed-off-by: Rudolf Marek <r.marek at assembler.cz>

regards
Rudolf
-------------- next part --------------
A non-text attachment was scrubbed...
Name: hwmon-patch-w83793-temp-display.patch
Type: text/x-patch
Size: 4573 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20070114/3df49b68/attachment.bin 

[lm-sensors] [PATCH] W83793: ignore the temperature readings

[JGong at winbond.com]

Hi, Jean 

Sorry for the delay.

>>Jean Delvare wrote:
>> Hi Rudolf, Gong,
>> 
>> Hm, the patch is incomplete. The following changes are missing:

>Hi again, sorry I forgot to think in wider scale completely :/
>I'm attaching updated patch.

>w83793: ignore the temperature readings when its channel is disabled,
>ignore AMDSI readings.

>Signed-off-by: Gong Jun <jgong at winbond.com>
>Signed-off-by: Rudolf Marek <r.marek at assembler.cz>

In addition I have added the code to skip the unused temperature
registers to speed up the update functions. Also it is tested on my
motherboard.

Best regards,
Gong Jun



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

[lm-sensors] [PATCH] W83793: ignore the temperature readings

[Jean Delvare]

Hi Gong,

On Tue, 16 Jan 2007 18:25:38 +0800, JGong at winbond.com wrote:
> >w83793: ignore the temperature readings when its channel is disabled,
> >ignore AMDSI readings.
> 
> >Signed-off-by: Gong Jun <jgong at winbond.com>
> >Signed-off-by: Rudolf Marek <r.marek at assembler.cz>
> 
> In addition I have added the code to skip the unused temperature
> registers to speed up the update functions. Also it is tested on my
> motherboard.

You've been too generous in your changes:

> @@ -1445,9 +1491,12 @@ static void w83793_update_nonvolatile(st
>  		}
>  	}
>  
> -	for (i = 0; i < ARRAY_SIZE(data->temp_mode); i++)
> +	for (i = 0; i < ARRAY_SIZE(data->temp_mode); i++) {
> +		if (!(data->has_temp & (1 << i)))
> +                        continue;
>  		data->temp_mode[i] >  		    w83793_read_value(client, W83793_REG_TEMP_MODE[i]);
> +	}
>  
>  	for (i = 0; i < ARRAY_SIZE(data->tolerance); i++) {
>  		data->tolerance[i] 
This change isn't correct, as temp_mode is a two-byte array, not 6. In
fact we can assume that at least a couple temperature channels will be
in use on every board so the test isn't really worth the effort.

Patch applied, with Rudolf's cleanups and documentation update.

Thanks,
-- 
Jean Delvare