[lm-sensors] [PATCH] THMC50 support for 2.6 (2nd revision)

[Mark M. Hoffman]

Hi Krzysztof:

* Krzysztof Helt <krzysztof.h1 at wp.pl> [2007-07-04 19:16:18 +0200]:
> On Wed, 4 Jul 2007 11:36:07 +0200
> Jean Delvare <khali at linux-fr.org> wrote:
> 
> > Hi Krzysztof,
> > 
> > On Mon, 2 Jul 2007 19:54:48 +0200, Krzysztof Helt wrote:
> > > From: Krzysztof Helt <krzysztof.h1 at wp.pl>
> > > +
> > > +The ADM1022 works the same as THMC50 but it is faster (5 Hz instead of
> > > +1 Hz for THMC50). It can be also put in a new mode to handle additional
> > > +remote temperature sensor. Some pins change their meaning in this mode.
> > > +If the sensor is wired to work in this mode but is not set with adm1022_temp3
> > > +parameter a fan is forced to full output.
> > 
> > This last sentence makes no sense to me. Can you try to reformulate it?
> > 
> 
> I can remove it completely. It was thought as a hint for someone with the fan settings
> after loading the driver.
> Is it better: "
> +If the sensor is supposed to work in this mode but is not set with adm1022_temp3
> +parameter a fan can be forced to full speed.
> 
> > BTW, I would expect the mode to be set properly by the BIOS. Wasn't it
> > the case for you?
> > 
> 
> I will read the configuration register in the probe function, so it will be preserved.
> Good hint.
> 
> > > +#define THMC50_REG_INTER			0x41
> > > +#define THMC50_REG_INTER_MIRROR			0x4C
> > > +#define THMC50_REG_INTER_MASK			0x43
> > 
> > You don't make any use of these THMC50_REG_INTER* defines, so why
> > define them? (What is this "inter" thing, BTW?)
> > 
> 
> They are interrupt registers (mask, status and status mirror).
> I'll remove them as THMC50's datasheet is still available.

I personally do not mind seeing macros for unused registers.

> > > +		.name		= "thmc50 sensor chip driver",
> > 
> > Should be just "thmc50", as Mark told you already?
> 
> I misunderstood him and changed only letters from capital ones.
>  
> > > +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?
> 
> > > +	client->driver = &thmc50_driver;
> > > +	client->flags = 0;
> > 
> > Note that this initialization to 0 isn't actually needed: the kzalloc
> > above did it for you.
> > 
> 
> I wonder if it is worth to go for kmalloc instead because most of fields (if not all)
> are set anyway.

You still want kzalloc because struct i2c_client contains a whole struct device.

> > > +	if (company == 0x49) {
> > > +		kind = thmc50;
> > > +	} else if (company == 0x41) {
> > > +		kind = adm1022;
> > > +	} else {
> > > +		pr_debug("thmc50.o: Detection of THMC50/ADM1022 failed\n");
> > > +		goto exit_free;
> > > +	}
> > 
> > [...]
> > 
> > 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.

I think that would be OK, actually.

> 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?
> 
> 
> > 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). 
> 
> 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?).
> 
> Any help appreciated.

Regards,

-- 
Mark M. Hoffman
mhoffman at lightlink.com