[lm-sensors] W83793 driver

[lm-sensors] W83793 driver

[Yuan Mu]

Hi Rudolf,
I have not send any news about w83793 driver because we met a problem -- the system always auto reboot even if
w83793 driver not loaded :(
Now they think the reason is the cpu fan is not fixed on the MB, and i will try to make the driver work on
other version of kernels...

Could you do me a favor to review the attached w83793 driver? i think i must fix the bugs before do the backport.
Thanks in advance :)
Any comments from others are greatly appreciated.

-- 
Best Regards
Yuan Mu


=============================================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: w83793.c
Type: application/octet
Size: 47444 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060609/a07c0b8f/attachment-0001.bin 

[lm-sensors] W83793 driver

[Rudolf Marek]

Hi Yuan,

> I have not send any news about w83793 driver because we met a problem --
> the system always auto reboot even if
> w83793 driver not loaded :(
> Now they think the reason is the cpu fan is not fixed on the MB, and i
> will try to make the driver work on
> other version of kernels...

Well you may try to compile Machine check excpetion support and thermal
throttling interrupt support. Then you will know if it has some thermal problems
or not.

Well maybe you can do that without a hardware. Check the i2c-stub driver.

If we would create more inteligent i2c-stub driver that supports banks, then we
can emulate this i2c chip with it. (This may work because registers are either
RO/RW no special behaviour mostly) With the emulation I mean that when you write
something to register it will be there.

The idea is following:
You need to dump all chip banks with i2cdump command. Then the driver i2c-stub
should be updated to support different banks. I could do it and test it on the
W83792d hardware machine if we are willing to go this direction.

Jean, any ideas about this? Do we have i2c-stub for 2.4 I cannot find it ...

> Could you do me a favor to review the attached w83793 driver? i think i
> must fix the bugs before do the backport.

I will just do a "quick" review later today and send you the results. I have in
my queue also VT1211 driver, hopefully I can do both today or in near future.

> Any comments from others are greatly appreciated.
Yes :)

Regards
Rudolf

[lm-sensors] W83793 driver

[Yuan Mu]

Hi Rudolf,
> 
>> I have not send any news about w83793 driver because we met a problem --
>> the system always auto reboot even if
>> w83793 driver not loaded :(
>> Now they think the reason is the cpu fan is not fixed on the MB, and i
>> will try to make the driver work on
>> other version of kernels...
> 
> Well you may try to compile Machine check excpetion support and thermal
> throttling interrupt support. Then you will know if it has some thermal problems
> or not.
> 
> Well maybe you can do that without a hardware. Check the i2c-stub driver.
> 
> If we would create more inteligent i2c-stub driver that supports banks, then we
> can emulate this i2c chip with it. (This may work because registers are either
> RO/RW no special behaviour mostly) With the emulation I mean that when you write
> something to register it will be there.
> 
> The idea is following:
> You need to dump all chip banks with i2cdump command. Then the driver i2c-stub
> should be updated to support different banks. I could do it and test it on the
> W83792d hardware machine if we are willing to go this direction.
> 
Sounds interesting, i think we do not need to do this now since the machine did not
reboot for 3 days, maybe the reason is they use a aeroengine as the cpufan...
I will dig what you said after finish the schedule about 793 :)
Now I got it work on FC5 and FC4, but FC3 met the same problem as i met before,
maybe i801 driver missing ICH7 PCI ID, and i'm trying to make it work :(

>> Could you do me a favor to review the attached w83793 driver? i think i
>> must fix the bugs before do the backport.
> 
> I will just do a "quick" review later today and send you the results. I have in
> my queue also VT1211 driver, hopefully I can do both today or in near future.
> 

Thanks, and later a 2.4 version :p

-- 
Best Regards
Yuan Mu


=============================================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.

[lm-sensors] W83793 driver

[Rudolf Marek]

Hi Yuan,

> Sounds interesting, i think we do not need to do this now since the 
> machine did not
> reboot for 3 days, maybe the reason is they use a aeroengine as the 
> cpufan...
> I will dig what you said after finish the schedule about 793 :)
> Now I got it work on FC5 and FC4, but FC3 met the same problem as i met 
> before,
> maybe i801 driver missing ICH7 PCI ID, and i'm trying to make it work :(

CHeck the PCI IDS, for new kernels you can check the history on each files:
http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=history;h†bc843a268058df558844b6bf64531617fbc698;f=drivers/i2c/busses/i2c-i801.c

And also through the trac.
http://www.lm-sensors.org/log/lm-sensors/trunk/kernel/busses/i2c-i801.c

>> I will just do a "quick" review later today and send you the results. 
>> I have in
>> my queue also VT1211 driver, hopefully I can do both today or in near 
>> future.
>>

I hope to do it this week, maybe in evening... Got a lot of work ;(

Regards
Rudolf

[lm-sensors] W83793 driver

[Yuan Mu]

Hi Rudolf,
>> Sounds interesting, i think we do not need to do this now since the 
>> machine did not
>> reboot for 3 days, maybe the reason is they use a aeroengine as the 
>> cpufan...
>> I will dig what you said after finish the schedule about 793 :)
>> Now I got it work on FC5 and FC4, but FC3 met the same problem as i 
>> met before,
>> maybe i801 driver missing ICH7 PCI ID, and i'm trying to make it work :(
> 
> CHeck the PCI IDS, for new kernels you can check the history on each files:
> http://www.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=history;h†bc843a268058df558844b6bf64531617fbc698;f=drivers/i2c/busses/i2c-i801.c 
> 
> 
> And also through the trac.
> http://www.lm-sensors.org/log/lm-sensors/trunk/kernel/busses/i2c-i801.c
> 

Thanks , it works now :)


-- 
Best Regards
Yuan Mu


=============================================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.

[lm-sensors] W83793 driver

[Rudolf Marek]

Hi Yuan,

Still reading the driver. It took already couple of evenings and two 120km
journeys in train and still I'm just passed the half of the driver.

I really want to do that well and this takes lot of time. I did not found
something really bad so far ;)

Please be patient, I have a lot of other work and I have other time consumpting
tasks to solve too.

Regards
Rudolf

[lm-sensors] W83793 driver

[Yuan Mu]

Hi Rudolf,

> Still reading the driver. It took already couple of evenings and two 120km
> journeys in train and still I'm just passed the half of the driver.
> 
Thanks. Do not watch the world-cup?
> I really want to do that well and this takes lot of time. I did not found
> something really bad so far ;)

It's really good to hear that, maybe not too bad ;)

> 
> Please be patient, I have a lot of other work and I have other time consumpting
> tasks to solve too.
> 

Never mind. I think we have enough time because the first MB use 793 delayed :(


-- 
Best Regards
Yuan Mu


=============================================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.

[lm-sensors] W83793 driver

[Rudolf Marek]

Hi Yuan,

After few hundreds kilometers in train and many hours outside of it I have
finally finished initial review. And I still dont know if I catched everything.

I'm attaching the driver with comments.

I would like to fix for sure:

1) step time/stop time unit to milliseconds
2) tolerance/range temperature to milidegrees of celsius, plus there is a little
problem with sysfs names. For EHF driver they are like this:
ruik at ruik:/sys/bus/i2c/devices/9191-0290$ ls pwm1*
pwm1  pwm1_enable  pwm1_min_output  pwm1_mode  pwm1_stop_time  pwm1_target
pwm1_tolerance

Because they are tight to pwm but your chip design has it tighten to temps... I
guess we cannot change it anyway...

Please can you provide some short documentation for new sysfs files that are
somewhat non-standard so we may discuss the names?

For example temp##index##_fan_map would be better name
temp[1-*]_auto_channels_pwm because we already define pwm[1-*]_auto_channels_temp

3)  WRITE_TO_REG
    is bad :) Please do not trust the cached values always write new one. ACPI
   might changed the value in your behind ;)

if (0 <= w83793_write_value(client, REG

Does not look nice too.

Plus there are many more comments and ideas in annotated driver. There are three
of my TODOs I will do that soon or later.

I'm offline starting tomorrow noon for two days so I will be not able to respond.

I think I will be able to respond you in morning if you have some questions to
my mail ;)

Regards
Rudolf
-------------- next part --------------
A non-text attachment was scrubbed...
Name: w83793.c
Type: application/octet
Size: 49177 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060705/4cc2553f/attachment-0001.bin 

[lm-sensors] W83793 driver

[Yuan Mu]

Hi Rudolf,
> 
> After few hundreds kilometers in train and many hours outside of it I have
> finally finished initial review. And I still dont know if I catched everything.
> 
Thanks :)

> I'm attaching the driver with comments.
> 
> I would like to fix for sure:
> 
> 1) step time/stop time unit to milliseconds
Of course ok.
> 2) tolerance/range temperature to milidegrees of celsius, plus there is a little
> problem with sysfs names. For EHF driver they are like this:
> ruik at ruik:/sys/bus/i2c/devices/9191-0290$ ls pwm1*
> pwm1  pwm1_enable  pwm1_min_output  pwm1_mode  pwm1_stop_time  pwm1_target
> pwm1_tolerance
> 
> Because they are tight to pwm but your chip design has it tighten to temps... I
> guess we cannot change it anyway...
> 
Yes, i guess so..
> Please can you provide some short documentation for new sysfs files that are
> somewhat non-standard so we may discuss the names?
> 
> For example temp##index##_fan_map would be better name
> temp[1-*]_auto_channels_pwm because we already define pwm[1-*]_auto_channels_temp
> 
OK.
> 3)  WRITE_TO_REG
>     is bad :) Please do not trust the cached values always write new one. ACPI
>    might changed the value in your behind ;)
> 
Ah, i hate ACPI :(
Almost all write function use this macro so to change it is simple :)
> if (0 <= w83793_write_value(client, REG
> 
> Does not look nice too.
> 
> Plus there are many more comments and ideas in annotated driver. There are three
> of my TODOs I will do that soon or later.
> 
I will fix what you said first.
> I'm offline starting tomorrow noon for two days so I will be not able to respond.
> 
> I think I will be able to respond you in morning if you have some questions to
> my mail ;)
> 

Thanks again for your review ;)

-- 
Best Regards
Yuan Mu


=============================================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.

[lm-sensors] W83793 driver

[Yuan Mu]

Hi Rudolf,

I revised the driver and make a patch against the file you send to me, please
can you check if it meet your review :)

And some note:
1.I removed some not used MACROs, and fix a wrong register definition.
2.tolerance registers, i stored the register value in cache, so i think no need to use index/2.
3. I leave the update_nonvolatile function there, because update too many registers every time
is just not worthy, if it can change in 300s interval, it also may change in 3s...


-- 
Best Regards
Yuan Mu


=============================================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 --------------
An embedded and charset-unspecified text was scrubbed...
Name: w83793_revise_1.patch
Url: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20060707/da56796a/attachment.pl