ATXP1 kernel patch

[se.witt@gmx.net (Sebastian Witt)]

Jean Delvare wrote:
 >>I've updated the module:
 >>http://www.hasw.net/linux/atxp1-0.3.patch
 >
 >
 > Comments:
 >
 > 1* Please respect the alphabetical order in i2c/chips/Makefile.
 >
 > 2* You include linux/config.h but don't seem to use it.
1-2 Ok.

 > 3* What is this "low_voltage" module parameter?
It allows setting of 1075mV which is not available on all platforms. 
Think this is removeable when using i2c-sensor-vid?

 > 4* I am really suspicious that the atxp1 has 0x37 and 0x4e as possible
 > addresses. How do you know?
On all mainboards (full list under http://www.hasw.net) I've seen, the 
ATXP1 is using 0x37 or 0x4e. If someone has a board with an ATXP1 which 
is using a different address, I think it can easily be added.

 > 5* normal_i2c_range is gone in Linux 2.6.10.
 >
 > 6* Don't use .id in i2c_driver atxp1_driver, you don't need it.
 >
 > 7* Other drivers don't call I2C_CLIENT_INSMOD directly.
Ok.


 > 8* In the current implementation, atxp1_data.valid is usless (value is
 > always 0). I also think that you should use restore and use
 > atxp1_data.last_updated.
Already fixed that, now using in atxp1_update_device:

# Update register data if needed
for(count = 0; count <= 0x0a; count++) {
     if((data->valid >> count & 1) = 0) {
         data->reg[count] = i2c_smbus_read_byte_data(client, count);
         data->valid |= 1 << count;
     }
}

 > 9* What's the difference between VID and CPU VID?
VID is the VID output control, enables or disables VID output control.
CPU VID are the original VID pins from the CPU.

 > 10* You hardcode the VID <-> Vcore translation formula. Depending on the
 > VRM version, it might or might not be correct. You should use the
 > functions in i2c-sensor-vid.
Ok.

 > 11* atxp1_write is quite complex. What makes you believe that the write
 > could possibly fail? Other i2c drivers don't do that kind of check.
Ported code. I'll remove it.

 > 12* Why does vcore default to 1.650V in atxp1_storevcore? Why is vcore
 > arbitrarily kept between 1.075V/1.100V and 1.850V? If trying to write a
 > value outside of this range is an error, then atxp1_storevcore should
 > return an error (negative value) when it happens.
1.075V-1.85V is the VRM range that is available on my 8RDA+ board. 
Greater range isn't possible.

 > 13* Use dev_info/dev_dbg etc... instead of printk wherever possible.
Ok.

 > 14* Care to comment/document the final loops in atxp1_storevcore? Since
 > atxp1_write is complex, I fear that these loops will be damn slow. Why
 > are they needed at all?
Trying to set CPU voltage in greater steps will freeze the CPU 
immediately (>+/-125mV, less under high CPU load). By in-/decrease the 
voltage in 25mV step the stability is increased. On my PC a script 
controls the CPU frequency and voltage and I do not recongize if it 
switches multiple times between 1125mV and 1650mV in a minute for 
testing. Ok that's subjective but could you define "slow" a little bit 
more please?

 > 15* Why does atxp1_storevcore set data->valid to 0?
So it only updates the local data if there was an write.
Is already replaced.

 > 16* Sysfs file name should probably be cpu0_vid, not cpu_vid, although I
 > admit I don't know how to properly handle the case where more than one
 > device is present.
Not thought on this, but is there a MP system which uses two of theses 
chips to
let the user independently control the CPU core voltages? But it should 
be modified.

 > 17* Don't set new_client->id to 0 in atxp1_detect, it's useless.
Ok.

 > 18* You detection method is too weak and will most likely result in
 > frequent misdetections. You have to improve it. I know that the device
 > doesn't have a decidated identification register, nor does it have many
 > registers to test, but we can certainly do something. Which registers
 > does the device have exactly? What happens when reading from
 > non-existent registers? The output of "i2cdump" may help.
Yes, that's right. I've attached the dump. Maybe using 0x0b-0x0f (shows 
all zero), 0x10-0x1a showing same value as 0x00 and >0x20 all zero.

 > 19* atxp1_detach_client frees the client data even if the deregistration
 > failed. This is probably dangerous.
Ok, fixed.

 > 20* init and exit functions are not properly marked __init and __exit.
also.

 > Good work nonetheless, this device is completely different from what we
 > have seen before so it's quite normal that some things need to be
 > discussed and tweaked before you/we get them right.

Thanks for your help.

Regards,
Sebastian
-------------- next part --------------
No size specified (using byte-data access)
  WARNING! This program can confuse your I2C bus, cause data loss and worse!
  I will probe file /dev/i2c-0, address 0x37, mode byte
  You have five seconds to reconsider and press CTRL-C!

     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 29 0a 1c 0c 0c 81 96 03 00 00 fb 00 00 00 00 00    )???????..?.....
10: 29 29 29 29 29 29 29 29 29 29 29 09 09 08 08 08    )))))))))))?????
20: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................
f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00    ................