Hi David, On Sun, 24 Dec 2006 20:58:48 -0800, David Hubbard wrote: > > The 4 LSB of the Super-I/O chip ID (4 LSB of LPC register 0x21) are > > subject to change in the future, which is why we mask them out when > > checking for device ID. > > I updated the comment and replaced the 4 LSB with zeros. There isn't > room to note that the 4 LSB are ignored in the comment, but the > implementation uses SIO_ID_MASK = 0xFFF0, so it should be correct. In > w83627ehf_find(), I thought about masking val when printing the > KERN_WARNING, but I prefer more information rather than less > information. Also, I think the it is more readable to mask val in > switch (val & SIO_ID_MASK). Agreed, I'm fine with your implementation. > > Strange choice. You store one specific difference between both chips > > first, then deduce the chip name again based on this difference. A > > saner approach would be to store the chip id (or kind) as a global, and > > then in w83627ehf_detect(), use the value to set the number of voltage > > inputs as a data structure member. It would be much more flexible that > > the current approach. This will also make your job easier when > > converting the driver to a platform driver. > > I agree. I would prefer to store specific values in w83627ehf_data and > detect the chips only once. However, I went with a compromise and so > in w83627ehf_find() when I detect the 627ehf, 627ehg, or 627dhg, I > store one value (w83627ehf_num_in) when I should be storing both > num_in and name. I consider the creation of a global variable a poor > solution, and I will change the implementation as quickly as possible. > I don't want to detect things twice (once in w83627ehf_find() and > later in w83627ehf_detect()). Well, you do detect things twice in the current implementation too. There's no way around it as long as the driver is based on i2c-isa. > > You can take a look at the it87 driver, it does exactly this. > > I will, in a future patch. OK, I'm looking forward to it :) If you do it at the same time you convert the driver to a platform driver, f71805f will be a better example though. > > This last paragraph isn't particularly helpful, "test" and "reserved" > > really mean the same. > > Yes, but I feel the information is still worth adding, for > completeness. (It could be argued that all the information in this > section isn't particularly helpful; one should just read the driver. I > would argue that it is still worth having.) I have left it in the > patch, but if you feel strongly about it, I'll delete it. No, I don't. It's your patch after all ;) So, your patch is applied, and will be in the next -mm kernel, and will then go in kernel 2.6.21: http://khali.linux-fr.org/devel/linux-2.6/jdelvare-hwmon/hwmon-w83627ehf-add-w83627dhg-support.patch Thanks for your contribution. -- Jean Delvare
