Hi Jean, > > Good idea. It's better not to register anything if we are giving it all > up in the end. However, you forgot to free the memory you allocated. Fixed. > > You are right that what addresses 0x10 and 0x11 return depend on what was > previously read from the chip, because these address don't map to > physical registers. However this is precisely a very good way to detect > exotic chips such as the ATXP1. Please do some tests to understand what > exactly determines the values being returned. Usually this is the last > "real" value read, but in your case this seems to be different. > > At the very least you can check that both addresses (0x10 and 0x11) > return the same value, whatever this is, right? > > I really think we need to use this, or the detection will be possibly > unreliable (although we are lucky that the ATXP1 uses uncommon > addresses). > > I've done some tests and it seems to work now if I first check the vendor ID registers and than after that 0,0x10,0x11. Also working after some random read/writes and reloading the module. > > We don't much care about future versions at this point. There might > never be any future version. If there is, you'll be able to do all the > required changes at that time. The code you will submit for the first > version of your driver has to make sense per se. Please only read the > registers you need. Done. > The reason why i2c clients have an update function is to centralize and > control i2c traffic that can be triggered by regular users. Remember > that regular users can read from sysfs files your driver exports. In > order to prevent possible bus saturation by regular users, all client > drivers have a timer in their update function, which prevent register > updates more frequent than (typically) one second. Your driver doesn't. > Please see in e.g. lm90.c how this has to be done, and do the same. > > Having individual validity bits then becomes useless. The only driver > which has that is eeprom.c, but only because it has a very large number > of "registers" (256 as opposed to less than 30 for most drivers) and a > very long cache timeout (5 minutes instead of 1 second). For your driver > it doesn't make sense. You should NOT suppose that register values > never change, this is dangerous. You're right, it's possible that the register values are reset after suspend-to-disk. And checking the validity bits doesn't recognize this. > So please: > 1* Add a timer check in your update function. 1 second (HZ) should be > fine. > 2* Only read registers you need. > 3* Have a single valid bit for them all. Done. > > Random comments on your code now: > .... All fixed, I think. Thanks for your help, Sebastian -------------- next part -------------- An embedded and charset-unspecified text was scrubbed... Name: atxp1.c Url: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20050120/e56440b4/attachment.c
