Hi Krzysztof, On Wed, 4 Jul 2007 19:16:18 +0200, Krzysztof Helt wrote: > On Wed, 4 Jul 2007 11:36:07 +0200 Jean Delvare wrote: > > > +set(temp_max, THMC50_REG_TEMP_OS); > > > +set(temp_hyst, THMC50_REG_TEMP_HYST); > > > +set(remote_temp_max, THMC50_REG_REMOTE_TEMP_OS); > > > +set(remote_temp_hyst, THMC50_REG_REMOTE_TEMP_HYST); > > > +set(remote_temp2_max, ADM1022_REG_REMOTE_TEMP2_OS); > > > +set(remote_temp2_hyst, ADM1022_REG_REMOTE_TEMP2_HYST); > > > > Side note: all these macro-generated functions should replaced by nice > > dynamic sysfs callbacks. > > Should I rewrite it before posting a driver? No, your code works as is and I don't want to delay the inclusion of your work into the kernel. But once your driver is accepted upstream, you could work on a patch to make this part better. > > The detection code for these chips in sensors-detect additionally > > checks that the MSB of the configuration register isn't set. You may > > want to do the same in your driver. > > I thought about it but I have to know what this kind (data->type) value means. The > driver works with two chips (thmc50 & adm1022) and the adm1022 chips has two modes. > The adm1022 is identical to thmc50 in the first mode and different (additional remote > temp) in the second mode. > > If I use the MSB bit to detect adm1022 it means that it is the adm1022 chip in the > second mode. The adm1022 in the first mode would be detected as the thmc50. Oops, sorry. I'm now realizing that the sensors-detect code is wrong, and that confused me. The bit I was thinking of was not bit 7 but bit 4 of the configuration register (Soft Reset). This bit is essentially write-only so it should always read 0. This can be used to improve the detection for both chips. I just fixed sensors-detect so that it checks bit 4 instead of bit 7. Bit 7 can also be checked for the THMC50 (and the ADM1028) but not the ADM1022, as you found out. I guess that sensors-detect was only detecting one of your 2 ADM1022 chips? Can you please try the sensors-detect version from SVN and confirm that it detects your two ADM1022 chips properly now? > If I use the manufacturer code for recognition of the chip, the adm1022 is always > adm1022 regardless the MSB in the configuration register (so that bit can be completely > ignored). > > Should the data->type store the chip type or its working mode? Other drivers store the chip type in data->type (if needed), and add dedicated fields to the data structure if they need to remember working modes. This sounds sensible. If you prefer to store the working mode, than I suggest that you drop the "type" field and have for example a "has_temp3" field instead. > > What about user-space support? Looking at the libsensors/sensors code, > > I see that at least the 3rd temperature sensor of the ADM1022 is not > > supported in libsensors. And support is entirely missing from sensors? > > Do you have a patch to add support? At least the code in the > > lm-sensors-3.0.0 branch should support it right away. > > It works with sensors3 and after addition to the sensors.conf it works with the AP550 > MB (motherboard). Don't you want to submit support for lm-sensor 2.10.x too? We have no clear idea when lm-sensors 3.0.0 will be released. For sensors.conf, I suggest that you write a dedicated configuration file for your motherboard rather than adding to sensors.conf.eg. > I have problem how to name temperature values. There are 5 temperature values: > > 1 & 2 - inside CPUs (I have to find which one is CPU0) > 3 - in the chip which is mounted on the MB next to CPUs (corner of the MB - the air from > CPUs is ducted there) > 4 - in the chip which is mounted on the MB next to memory slot (which is in the half > length of the MB) > 5 - remote temperature from the second chip. I don't know where the sensor is located > but it is 5 degrees higher then the temperature 4 (maybe inside RIMM memory or next > to graphics card?). Sensor locations are sometimes mentioned on the motherboard manuals, so you may take a look. Otherwise you can "scan" you motherboard with a hair-dryer ;) The last remote temperature could be the north bridge or south bridge temperature. -- Jean Delvare
