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 -------------- An embedded and charset-unspecified text was scrubbed... Name: dump Url: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20050104/231af8a0/attachment.pl
