Re: [lm-sensors] THMC50 and ADM1022 support for 2.6 kernel

Re: [lm-sensors] THMC50 and ADM1022 support for 2.6 kernel

[Jean Delvare]

Hi Mark, hi Krzysztof,

A couple comments on top of what Mark already said:

On Sun, 1 Jul 2007 10:03:25 -0400, Mark M. Hoffman wrote:
> * Krzysztof Helt <krzysztof.h1@wp.pl> [2007-06-25 17:02:10 +0200]:
> > diff -urNp linux-2.6.21/Documentation/hwmon/thmc50 linux-2.6.22/Documentation/hwmon/thmc50
> > --- linux-2.6.21/Documentation/hwmon/thmc50	1970-01-01 01:00:00.000000000 +0100
> > +++ linux-2.6.22/Documentation/hwmon/thmc50	2007-06-24 22:22:56.852806945 +0200
> > @@ -0,0 +1,82 @@
> > +Kernel driver `thmc50.o'
> > +==========
Please check the other driver documentation files under
Documentation/hwmon for the correct header style.

> > +
> > +Status: Complete and some-what tested

We no longer document the status in documentation files. It gets out of
date too easily, and non-working drivers don't go into the kernel tree
anyway.

> > +
> > +Supported chips: 
> > +  * Analog Devices ADM1022
> > +    Prefix: 'adm1022'
> > +    Addresses scanned: I2C 0x2D - 0x2F
> > +    Datasheet: Publicly available at the Analog Devices website
> 
> Please provide a link, if possible.
> 
> > +  * Texas Instruments THMC50
> > +    Prefix: 'thmc50'
> > +    Addresses scanned: I2C 0x2D - 0x2F
> > +    Datasheet: Publicly available at the Texas Instruments' website
> 
> Ditto.

These address ranges don't match what the driver actually does.

> > diff -urNp linux-2.6.21/drivers/hwmon/thmc50.c linux-2.6.22/drivers/hwmon/thmc50.c
> > --- linux-2.6.21/drivers/hwmon/thmc50.c	1970-01-01 01:00:00.000000000 +0100
> > +++ linux-2.6.22/drivers/hwmon/thmc50.c	2007-06-24 22:22:56.852806945 +0200
> > @@ -0,0 +1,438 @@
> (...)
> > +/*
> > +/* Conversions. Rounding and limit checking is only done on the TO_REG

Comment is started twice.

> > +   variants. Note that you should be a bit careful with which arguments
> > +   these macros are called: arguments may be evaluated more than once.
> > +   Fixing this is just not worth it. */

Fixing it is actually very worth it, and I suggest that you turn these
macros into inline functions.

> > +#define TEMP_FROM_REG(val) ((val>127)?(val - 0x0100)*1000:val*1000)
> > +#define TEMP_TO_REG(val)   ((val<0)?0x0100+val/1000:val/1000)

Note that using signed variables would save you these ugly 0x100
offsets. See the lm90 driver for an example.

> > +
> > +/* Each client has this additional data */
> > +struct thmc50_data {
> > +	struct i2c_client client;
> > +	struct class_device *class_dev;
> > +
> > +	struct mutex update_lock;
> > +	char valid;		/* !=0 if following fields are valid */
> > +	enum chips type;
> > +	unsigned long last_updated;	/* In jiffies */
> > +
> > +	/* Register values */
> > +	u16 temp_input;
> > +	u16 temp_max;
> > +	u16 temp_hyst;
> > +	u16 remote_temp_input;
> > +	u16 remote_temp_max;
> > +	u16 remote_temp_hyst;
> > +	u16 remote_temp2_input;
> > +	u16 remote_temp2_max;
> > +	u16 remote_temp2_hyst;
> > +	u16 analog_out;
> > +	u16 config_reg;
> > +};

What's the point of using 16-bit members to store only 8-bit register
values?

> > +/* This is the driver that will be inserted */

This is a not very interesting comment ;)

> > +static struct i2c_driver thmc50_driver = {
> > +	.driver = {
> > +		.name		= "THMC50 sensor chip driver",
> 
> Should be one word, lowercase... "thmc50".
> 
> > +	},
> > +	.id		= I2C_DRIVERID_THMC50,

These IDs are unused and will be deleted someday, so you might as well
not define it.

> > +	.attach_adapter	= thmc50_attach_adapter,
> > +	.detach_client	= thmc50_detach_client,
> > +};

> > +/* This function is called by i2c_detect */

Unlikely, given that i2c_detect() no longer exists.

> > +int thmc50_detect(struct i2c_adapter *adapter, int address, int kind)
> > +{
> > +	int company;
> > +	int revision;
> > +	struct i2c_client *new_client;

I would be grateful if you could rename this to just "client".

> > +	struct thmc50_data *data;
> > +	struct device *dev;
> > +	int err = 0;
> > +	const char *type_name = "";
> > +
> > +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> 
> Some type of warning would be nice here, instead of silently failing.
> 
> > +		goto exit;

I disagree. There may be several I2C buses on the system, for example
one with no THMC50 chip and which doesn't support this functionality,
and one with a THMC50 chip and which does support this functionality.
Printing a warning when the first bus is being probed would be quite
misleading.

As a matter of fact, none of the other i2c-based hardware monitoring
drivers print error messages in this case. One driver (max6650) prints
a debugging message.

> > +/* All registers are word-sized, except for the configuration register.
> > +   THMC50 uses a high-byte first convention, which is exactly opposite to
> > +   the usual practice. */

This comment was seemingly stolen from a different driver, and doesn't
apply to the THMC50 at all!

> > +static int thmc50_read_value(struct i2c_client *client, u8 reg)
> > +{
> > +	return i2c_smbus_read_byte_data(client, reg);
> > +}
> > +
> > +/* All registers are word-sized, except for the configuration register.
> > +   THMC50 uses a high-byte first convention, which is exactly opposite to
> > +   the usual practice. */
> > +static int thmc50_write_value(struct i2c_client *client, u8 reg, u16 value)
> > +{
> > +	return i2c_smbus_write_byte_data(client, reg, value);
> > +}
> 
> You don't use this return value anywhere.  Why not make it void?

Why define these functions in the first place? You could call the
i2c_smbus_*() functions directly. If you really want to keep these
wrappers, please at least mark them inline.

> > +static void thmc50_init_client(struct i2c_client *client)
> > +{
> > +	struct thmc50_data *data = i2c_get_clientdata(client);
> > +
> > +	data->config_reg |= 0x23;
> > +	thmc50_write_value(client, THMC50_REG_CONF, data->config_reg);
> > +	data->analog_out = thmc50_read_value(client, THMC50_REG_ANALOG_OUT);
> > +	/* set up to at least 1 */
> > +	if (data->analog_out = 0 ) {
> > +		data->analog_out = 1;
> > +		thmc50_write_value(client, THMC50_REG_ANALOG_OUT, data->analog_out);
> > +	}
> > +}

Why do you change the value of the analog output? Loading a hardware
monitoring driver should NOT change the state of the system.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] THMC50 and ADM1022 support for 2.6 kernel

[Krzysztof Helt]

On Sun, 1 Jul 2007 20:50:03 +0200

Thank you for valuable input. I removed wrappers and changed data types

Jean Delvare <khali@linux-fr.org> wrote:
> 
> > > +	struct thmc50_data *data;
> > > +	struct device *dev;
> > > +	int err = 0;
> > > +	const char *type_name = "";
> > > +
> > > +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> > 
> > Some type of warning would be nice here, instead of silently failing.
> > 
> > > +		goto exit;
> 
> I disagree. There may be several I2C buses on the system, for example
> one with no THMC50 chip and which doesn't support this functionality,
> and one with a THMC50 chip and which does support this functionality.
> Printing a warning when the first bus is being probed would be quite
> misleading.
> 
> As a matter of fact, none of the other i2c-based hardware monitoring
> drivers print error messages in this case. One driver (max6650) prints
> a debugging message.
> 

The driver will print debugging message to keep you and Mark Hoffman happy.
> 
> > > +static void thmc50_init_client(struct i2c_client *client)
> > > +{
> > > +	struct thmc50_data *data = i2c_get_clientdata(client);
> > > +
> > > +	data->config_reg |= 0x23;
> > > +	thmc50_write_value(client, THMC50_REG_CONF, data->config_reg);
> > > +	data->analog_out = thmc50_read_value(client, THMC50_REG_ANALOG_OUT);
> > > +	/* set up to at least 1 */
> > > +	if (data->analog_out = 0 ) {
> > > +		data->analog_out = 1;
> > > +		thmc50_write_value(client, THMC50_REG_ANALOG_OUT, data->analog_out);
> > > +	}
> > > +}
> 
> Why do you change the value of the analog output? Loading a hardware
> monitoring driver should NOT change the state of the system.
> 

The analog output has range from 0 to 255. The 0 value is the least value possible but
it is not 0 voltage on output pin. It simply must be adjusted to the range 1-255
to conform with sysfs. I choose this way as the simplest. Another possibility is
to scale this value when set and displayed from 1-255 (sysfs) to 0-255 (chip) range.

The sysfs 0 value for pwm1 is translated to setting a special bit which cuts off the
fan power completely (this works on my motherboard and it is given in reference
design, but I cannot guarantee it is connected this way on all boards). 

Regards,
Krzysztof

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] THMC50 and ADM1022 support for 2.6 kernel

[Jean Delvare]

Hi Krzysztof,

On Sun, 1 Jul 2007 23:37:33 +0200, Krzysztof Helt wrote:
> > > > +static void thmc50_init_client(struct i2c_client *client)
> > > > +{
> > > > +	struct thmc50_data *data = i2c_get_clientdata(client);
> > > > +
> > > > +	data->config_reg |= 0x23;
> > > > +	thmc50_write_value(client, THMC50_REG_CONF, data->config_reg);
> > > > +	data->analog_out = thmc50_read_value(client, THMC50_REG_ANALOG_OUT);
> > > > +	/* set up to at least 1 */
> > > > +	if (data->analog_out = 0 ) {
> > > > +		data->analog_out = 1;
> > > > +		thmc50_write_value(client, THMC50_REG_ANALOG_OUT, data->analog_out);
> > > > +	}
> > > > +}
> > 
> > Why do you change the value of the analog output? Loading a hardware
> > monitoring driver should NOT change the state of the system.
> 
> The analog output has range from 0 to 255. The 0 value is the least value possible but
> it is not 0 voltage on output pin. It simply must be adjusted to the range 1-255
> to conform with sysfs. I choose this way as the simplest. Another possibility is
> to scale this value when set and displayed from 1-255 (sysfs) to 0-255 (chip) range.
>
> The sysfs 0 value for pwm1 is translated to setting a special bit which cuts off the
> fan power completely (this works on my motherboard and it is given in reference
> design, but I cannot guarantee it is connected this way on all boards). 

I see. Scaling from 1-255 to 0-255 would be counterproductive, but is
it really what you want to do? You say that a register value doesn't
stop the fan, suggesting that the analog output isn't 0V? Do you know
how much it is? For example, if the full analog output is 2.5 V and a
register value of 0 means an output of 1.0 V, then you want to map the
register values 0 to 255 to sysfs values 102 to 255. 0 would stop the
fan, and values from 1 to 101 would behave the same as 102 (i.e.
minimum non-stop speed). See what I mean?

This is just a proposal anyway, your current implementation is
probably acceptable as well.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] THMC50 and ADM1022 support for 2.6 kernel(improved)

[Krzysztof Helt]

Dnia 3-07-2007 o godz. 23:17 Jean Delvare napisa³(a):
> Hi Krzysztof,
> 
> On Sun, 1 Jul 2007 23:37:33 +0200, Krzysztof Helt wrote:

> > The analog output has range from 0 to 255. The 0 value is the
least value possible but
> > it is not 0 voltage on output pin. It simply must be adjusted
to the range 1-255
> > to conform with sysfs. I choose this way as the simplest.
Another possibility is
> > to scale this value when set and displayed from 1-255 (sysfs)
to 0-255 (chip) range.
> >
> > The sysfs 0 value for pwm1 is translated to setting a special
bit which cuts off the
> > fan power completely (this works on my motherboard and it is
given in reference
> > design, but I cannot guarantee it is connected this way on
all boards). 
> 
> I see. Scaling from 1-255 to 0-255 would be counterproductive,
but is
> it really what you want to do? You say that a register value
doesn't
> stop the fan, suggesting that the analog output isn't 0V? Do
you know
> how much it is? For example, if the full analog output is 2.5 V
and a
> register value of 0 means an output of 1.0 V, then you want to
map the
> register values 0 to 255 to sysfs values 102 to 255. 0 would
stop the
> fan, and values from 1 to 101 would behave the same as 102 (i.e.
> minimum non-stop speed). See what I mean?
> 

I see, but my explanation was not quite clear/true.

According to docs, the THMC50 produces range between 0 and 2.5V.
The residual voltage (0 value != 0 V) comes from the external
power amplifier. There are 6 example designs of external power
amplifier given in the THMC50's datasheet. Most of the are
powered by 0 - 12V so they are usually unable to produce the full
0-12V swing (as amplifiers and transistors cannot give output up
to power voltage limits). One design (the cheapest) is said to
not give full voltage swing by design.  That's why they have the
fan off bit - to be able to stop the fan.

One of designs is powered from +/-12V and this one certainly can
produce true 0V output - in such case the 0 value would stop fan
and the fan off pin may be left unconnected. As you can see, my
driver writes 0 value into the fan speed register along with
setting of the fan off bit  - to cover both cases.

The residual voltage is simple imperfection of analog parts
outside the chip and the value of this voltage is as random as
the choice of analog elements mounted on  motherboards (will vary
with production batch change and type of elements e.g. change of
supplier).

Summary: one knows that 0 value produces 0V at the chip pin and
at the same time one knows it is not 0V at the fan pins in most
of reference designs (hence the separate fan off bit). 

Regards,
Krzysztof

----------------------------------------------------
Legenda Hip-Hopu w pelnym 9 osobowym skladzie, na jedynym koncercie 
w Polsce WU-TANG CLAN. 
19 lipca 2007 r, godz. 20.00 - Warszawa, klub Stodo³a. 
http://klik.wp.pl/?adr=http%3A%2F%2Fadv.reklama.wp.pl%2Fas%2Fwutangclan.html&sid\x1210



_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] THMC50 and ADM1022 support for 2.6 kernel(improved)

[Jean Delvare]

Hi Krzysztof,

On Wed, 04 Jul 2007 09:25:05 +0200, Krzysztof Helt wrote:
> According to docs, the THMC50 produces range between 0 and 2.5V.
> The residual voltage (0 value != 0 V) comes from the external
> power amplifier. There are 6 example designs of external power
> amplifier given in the THMC50's datasheet. Most of the are
> powered by 0 - 12V so they are usually unable to produce the full
> 0-12V swing (as amplifiers and transistors cannot give output up
> to power voltage limits). One design (the cheapest) is said to
> not give full voltage swing by design.  That's why they have the
> fan off bit - to be able to stop the fan.
> 
> One of designs is powered from +/-12V and this one certainly can
> produce true 0V output - in such case the 0 value would stop fan
> and the fan off pin may be left unconnected. As you can see, my
> driver writes 0 value into the fan speed register along with
> setting of the fan off bit  - to cover both cases.
> 
> The residual voltage is simple imperfection of analog parts
> outside the chip and the value of this voltage is as random as
> the choice of analog elements mounted on  motherboards (will vary
> with production batch change and type of elements e.g. change of
> supplier).
> 
> Summary: one knows that 0 value produces 0V at the chip pin and
> at the same time one knows it is not 0V at the fan pins in most
> of reference designs (hence the separate fan off bit). 

I see, thanks for the detailed explanation. Then I'm fine with your
implementation.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors