W83793: ignore the temperature readings when
PATCH 1/1 W83793: ignore the temperature readings when
Hello
w83793: ignore the temperature readings when its channel is disabled.
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
PATCH 1/1 W83793: ignore the temperature readings
Hi Rudolf, Gong,
Quoted 8 lines · Rudolf Marek
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_enabletohas_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:
Review line 1329
Quoted 6 lines
(quoted diff) +2
-0@@ -1320,6 +1325,30 @@ data->has_pwm |= 0x80; } + /* check the temp1-6 mode */ + data->has_temp = 0;
Not needed, kzalloc() zeroed it for you.
Quoted 4 lines
(quoted diff) +4
-0+ + tmp = w83793_read_value(client,W83793_REG_TEMP_MODE[0]); + + if (tmp & 0x03)
Review line 1343
Quoted 10 lines
(quoted diff) +10
-0+ 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.
Review line 1354
Quoted 11 lines
(quoted diff) +8
-0+ + 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,
Signature · 1 line
Jean Delvare
PATCH 1/1 W83793: ignore the temperature readings
In reply to Rudolf MarekQuoted 3 lines · Jean Delvare
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.
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
PATCH 1/1 W83793: ignore the temperature readings
In reply to Rudolf MarekHi, Jean
Sorry for the delay.
Quoted 5 lines
Quoted 4 lines
Jean Delvare wrote:
Hi Rudolf, Gong,Hm, the patch is incomplete. The following changes are missing:
Quoted 8 lines
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
PATCH 1/1 W83793: ignore the temperature readings
In reply to Rudolf MarekHi Gong,
Quoted 9 lines · JGong at winbond.com
Quoted 5 lines
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:
Review static void w83793_update_nonvolatile(st · line 1501
Quoted 13 lines
(quoted diff) +4
-1@@ -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,
Signature · 1 line
Jean Delvare