Skip to contentSkip to conversation

I2C: New hardware monitor driver: adm9240

PATCH New Grant Coady 3 participants 15 messages May 19, 2005

lore
Share & tools
lore

Grant Coady

author
#0 lore raw

PATCH 1/1 I2C: New hardware monitor driver: adm9240

Hi Greg, All,

Completion of Michiel Rook's port of adm9240 to 2.6.

This driver also has the new auto fan clock divider and is my test driver for proving the auto fan_div algorithm.

Development on 2.6.11.7 and compile tested on 2.6.12-rc3.

Changes since last submission: remove unused includes, rename sysfs 'ci_clear' to 'control' as another driver uses 'control', rename 'analog_out' to 'aout_output' to match sysfs naming style, change scaling macros to static inlines, update auto fan_div comments.

Thanks,
--Grant.

Signed-off-by Grant Coady <[email protected]>
---
 Kconfig   |   11 
 Makefile  |    1 
 adm9240.c |  766 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 778 insertions(+)
drivers/i2c/chips/Kconfig +11 -0
diff -X dontdiff -Nru linux-2.6.12-rc3/drivers/i2c/chips/Kconfig linux-2.6.12-rc3a/drivers/i2c/chips/Kconfig
@@ -51,6 +51,17 @@
 	  This driver can also be built as a module.  If so, the module
 	  will be called adm1031.
 
+config SENSORS_ADM9240
+	tristate "Analog Devices ADM9240 and compatibles"
+	depends on I2C && EXPERIMENTAL
+	select I2C_SENSOR
+	help
+	  If you say yes here you get support for Analog Devices ADM9240,
+	  Dallas DS1780, National Semiconductor LM81 sensor chips.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called adm9240.
+
 config SENSORS_ASB100
 	tristate "Asus ASB100 Bach"
 	depends on I2C && EXPERIMENTAL
drivers/i2c/chips/Makefile +1 -0
diff -X dontdiff -Nru linux-2.6.12-rc3/drivers/i2c/chips/Makefile linux-2.6.12-rc3a/drivers/i2c/chips/Makefile
@@ -11,6 +11,7 @@
 obj-$(CONFIG_SENSORS_ADM1025)	+= adm1025.o
 obj-$(CONFIG_SENSORS_ADM1026)	+= adm1026.o
 obj-$(CONFIG_SENSORS_ADM1031)	+= adm1031.o
+obj-$(CONFIG_SENSORS_ADM9240)	+= adm9240.o
 obj-$(CONFIG_SENSORS_DS1337)	+= ds1337.o
 obj-$(CONFIG_SENSORS_DS1621)	+= ds1621.o
 obj-$(CONFIG_SENSORS_EEPROM)	+= eeprom.o
drivers/i2c/chips/adm9240.c +766 -0

766 lines not on this page · raw message

#1 lore raw

PATCH 1/1 I2C: New hardware monitor driver: adm9240 (2nd try)

Hi Grant,

Quoted 5 lines
Quoted 3 lines
	reg = adm9240_read_value(client, ADM9240_REG_CONFIG);
	adm9240_write_value(client, ADM9240_REG_CONFIG,
		(reg | 0x01) & 0x7f);

That would fail to start the measurement cycle (...)

How that? Bit 0 will be set to 1, which starts the measurement cycle, exactly like your previous code did.

Quoted 1 line

and leave user-space able to assert a hardware interrupt signal,

I don't understand. What do you mean exactly?

Quoted 1 line

(...) plus it writes zero to bit 7 for what reason?

Because writing 1 instead would reset the chip, and you don't want to do that, do you?

Thanks,
--
Jean Delvare

#2 lore raw

PATCH 1/1 I2C: New hardware monitor driver: adm9240 (2nd try)

In reply to Grant Coady
Quoted 2 lines · Grant Coady

Maybe I should wait until after 2.6.11.8 before resending patch again, we're just clogging the works?

Doesn't bother me, they are all just being queued up in my TODO mailbox :)

thanks,

greg k-h

#3 lore raw

PATCH 1/1 I2C: New hardware monitor driver: adm9240 (2nd try)

In reply to Grant Coady

Hi Grant,

Quoted 7 lines

Need to clear bit 3, powerup = 1, as well as set bit 0, but:

control reg (40h)
7..4 writing zero harmless
3 INT_clear, power-up default = 1 stops measurement, writing zero
required 2,1 writing zero harmless
0 start measurement cycle, write '1' to start measurement cycle

Oh well, if you say so, fine with me. After all, you are supposed to know that device better than I do.

Signature · 1 line
Jean Delvare
author
#4 lore raw

PATCH 1/1 I2C: New hardware monitor driver: adm9240 (3rd try)

In reply to Grant Coady

Hi Greg, Khali,

Completion of Michiel Rook's port of adm9240 to 2.6 with addition of auto fan clock divider and replace scaling macros with static inlines.

This patch replaces earlier versions and fixes default limit set 'for' loop found by Jean Delvare, minor reporting and comment changes.

--Grant.

Signed-off-by Grant Coady <[email protected]>
---
 Kconfig   |   11 
 Makefile  |    1 
 adm9240.c |  768 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 780 insertions(+)
drivers/i2c/chips/Kconfig +11 -0
diff -X dontdiff -Nru linux-2.6.12-rc3/drivers/i2c/chips/Kconfig linux-2.6.12-rc3b/drivers/i2c/chips/Kconfig
@@ -51,6 +51,17 @@
 	  This driver can also be built as a module.  If so, the module
 	  will be called adm1031.
 
+config SENSORS_ADM9240
+	tristate "Analog Devices ADM9240 and compatibles"
+	depends on I2C && EXPERIMENTAL
+	select I2C_SENSOR
+	help
+	  If you say yes here you get support for Analog Devices ADM9240,
+	  Dallas DS1780, National Semiconductor LM81 sensor chips.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called adm9240.
+
 config SENSORS_ASB100
 	tristate "Asus ASB100 Bach"
 	depends on I2C && EXPERIMENTAL
drivers/i2c/chips/Makefile +1 -0
diff -X dontdiff -Nru linux-2.6.12-rc3/drivers/i2c/chips/Makefile linux-2.6.12-rc3b/drivers/i2c/chips/Makefile
@@ -11,6 +11,7 @@
 obj-$(CONFIG_SENSORS_ADM1025)	+= adm1025.o
 obj-$(CONFIG_SENSORS_ADM1026)	+= adm1026.o
 obj-$(CONFIG_SENSORS_ADM1031)	+= adm1031.o
+obj-$(CONFIG_SENSORS_ADM9240)	+= adm9240.o
 obj-$(CONFIG_SENSORS_DS1337)	+= ds1337.o
 obj-$(CONFIG_SENSORS_DS1621)	+= ds1621.o
 obj-$(CONFIG_SENSORS_EEPROM)	+= eeprom.o
drivers/i2c/chips/adm9240.c +768 -0

768 lines not on this page · raw message

author
#5 lore raw

PATCH 1/1 I2C: New hardware monitor driver: adm9240 (2nd try)

In reply to Grant Coady

Hi Khali, Greg,

Completion of Michiel Rook's port of adm9240 to 2.6 with addition of auto fan clock divider and replace scaling macros with static inlines.

Changes to address Jean Delvare's comments: sysfs 'control' now 'chassis_clear', replace removed includes, whitespace fixes, removed cold start mode 0 set, commented 'start measurement cycle' command.

Added: prevent auto fan adjustment on first measurement, reducing gratuitous fan clock divider changes.

--Grant.

Signed-off-by Grant Coady <[email protected]>
---
 Kconfig   |   11 
 Makefile  |    1 
 adm9240.c |  769 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 781 insertions(+)
drivers/i2c/chips/Kconfig +11 -0
diff -X dontdiff -Nru linux-2.6.12-rc3/drivers/i2c/chips/Kconfig linux-2.6.12-rc3b/drivers/i2c/chips/Kconfig
@@ -51,6 +51,17 @@
 	  This driver can also be built as a module.  If so, the module
 	  will be called adm1031.
 
+config SENSORS_ADM9240
+	tristate "Analog Devices ADM9240 and compatibles"
+	depends on I2C && EXPERIMENTAL
+	select I2C_SENSOR
+	help
+	  If you say yes here you get support for Analog Devices ADM9240,
+	  Dallas DS1780, National Semiconductor LM81 sensor chips.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called adm9240.
+
 config SENSORS_ASB100
 	tristate "Asus ASB100 Bach"
 	depends on I2C && EXPERIMENTAL
drivers/i2c/chips/Makefile +1 -0
diff -X dontdiff -Nru linux-2.6.12-rc3/drivers/i2c/chips/Makefile linux-2.6.12-rc3b/drivers/i2c/chips/Makefile
@@ -11,6 +11,7 @@
 obj-$(CONFIG_SENSORS_ADM1025)	+= adm1025.o
 obj-$(CONFIG_SENSORS_ADM1026)	+= adm1026.o
 obj-$(CONFIG_SENSORS_ADM1031)	+= adm1031.o
+obj-$(CONFIG_SENSORS_ADM9240)	+= adm9240.o
 obj-$(CONFIG_SENSORS_DS1337)	+= ds1337.o
 obj-$(CONFIG_SENSORS_DS1621)	+= ds1621.o
 obj-$(CONFIG_SENSORS_EEPROM)	+= eeprom.o
drivers/i2c/chips/adm9240.c +769 -0

769 lines not on this page · raw message

#6 lore raw

PATCH 1/1 I2C: New hardware monitor driver: adm9240

In reply to Grant Coady

Hi Grant,

Quoted 5 lines

Changes since last submission: remove unused includes, rename sysfs 'ci_clear' to 'control' as another driver uses 'control', rename 'analog_out' to 'aout_output' to match sysfs naming style, change scaling macros to static inlines, update auto fan_div comments.

The "control" name isn't a good one IMHO. It could hardly be less descriptive. The fact that another driver uses it is not relevant, as it is not part of our sysfs standard. "chassis_clear" maybe?

My comments about your code:

drivers/i2c/chips/adm9240.c line 46

Quoted 3 lines
drivers/i2c/chips/adm9240.c +3 -0
+#include <linux/i2c.h>
+#include <linux/i2c-sensor.h>
+#include <linux/i2c-vid.h>

I think you have been a bit agressive in your includes removal. Please at least include linux/init.h, linux/module.h and linux/slab.h. The driver needs them.

Quoted 2 lines

+static unsigned short normal_i2c[] = { 0x2c,0x2d,0x2e,0x2f, I2C_CLIENT_END };

Space after the commas please. There are a couple other places in the code where they are missing, please add them where needed.

drivers/i2c/chips/adm9240.c line 653

Quoted 2 lines
drivers/i2c/chips/adm9240.c +2 -0
+		adm9240_write_value(client, ADM9240_REG_TEMP_CONF, 0);
+		adm9240_write_value(client, ADM9240_REG_CONFIG, 1);

What an agressive initialization. Please only set the bits which really need to be. Arbitrary reconfiguration of hardware monitoring chips is not wise.

Otherwise I am fine with your code.

Thanks,
--
Jean Delvare

author
#7 lore raw

PATCH 1/1 I2C: New hardware monitor driver: adm9240

In reply to Grant Coady

Hi Khali, Greg,

Quoted 1 line · Jean Delvare

is not part of our sysfs standard. "chassis_clear" maybe?

done

Quoted 3 lines

I think you have been a bit agressive in your includes removal. Please at least include linux/init.h, linux/module.h and linux/slab.h. The driver needs them.

back in

Quoted 5 lines
Quoted 2 lines

+static unsigned short normal_i2c[] = { 0x2c,0x2d,0x2e,0x2f, I2C_CLIENT_END };

Space after the commas please. There are a couple other places in the code where they are missing, please add them where needed.

done

Quoted 1 line

drivers/i2c/chips/adm9240.c line 652

Quoted 1 line
drivers/i2c/chips/adm9240.c +1 -0
+		adm9240_write_value(client, ADM9240_REG_TEMP_CONF, 0);

removed, leave adm9240 in power-up one-shot interrupt mode, as alarm status remains asserted over multiple reads and driver does not enable interrupt pin output.

Quoted 1 line

drivers/i2c/chips/adm9240.c line 653

Quoted 1 line
drivers/i2c/chips/adm9240.c +1 -0
+		adm9240_write_value(client, ADM9240_REG_CONFIG, 1);

this one starts measurement cycle, added a comment to that effect

Quoted 2 lines

Otherwise I am fine with your code.

Thank you. New patch soonish.

--Grant.

#8 lore raw

PATCH 1/1 I2C: New hardware monitor driver: adm9240 (2nd try)

In reply to Grant Coady

Hi Grant,

drivers/i2c/chips/adm9240.c line 647

Quoted 6 lines
drivers/i2c/chips/adm9240.c +6 -0
+		for (i = 1; i < 6; i++) {
+			adm9240_write_value(client,
+					ADM9240_REG_IN_MIN(i), 0);
+			adm9240_write_value(client,
+					ADM9240_REG_IN_MAX(i), 255);
+		}

Shouldn't i start from 0 here?

Review

Quoted 2 lines
(quoted diff) +2 -0
+		/* start measurement cycle */
+		adm9240_write_value(client, ADM9240_REG_CONFIG, 1);

I'm sorry but I'm still not quite happy with this. You want to change one bit and end up writing 8. What about something along the lines of:

	reg = adm9240_read_value(client, ADM9240_REG_CONFIG);
	adm9240_write_value(client, ADM9240_REG_CONFIG,
		(reg | 0x01) & 0x7f);

That way you really only set the bits which need be.

Thanks,
--
Jean Delvare

author
#9 lore raw

PATCH 1/1 I2C: New hardware monitor driver: adm9240

In reply to Grant Coady

Hi Greg,

Completion of Michiel Rook's port of adm9240 to 2.6 with addition of auto fan clock divider based on Jean Delvare's algorithm, and replaces scaling macros with static inlines.

This patch is updated against 2.6.12-rc3-mm1, no other changes.

Development on 2.6.11.7 and compile tested on 2.6.12-rc3-mm1.

--Grant.

Signed-off-by Grant Coady <[email protected]>
---
 Kconfig   |   11 
 Makefile  |    1 
 adm9240.c |  768 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 780 insertions(+)
drivers/i2c/chips/Kconfig +11 -0
diff -X dontdiff -Nru linux-2.6.12-rc3-mm1/drivers/i2c/chips/Kconfig linux-2.6.12-rc3-mm1b/drivers/i2c/chips/Kconfig
@@ -51,6 +51,17 @@
 	  This driver can also be built as a module.  If so, the module
 	  will be called adm1031.
 
+config SENSORS_ADM9240
+	tristate "Analog Devices ADM9240 and compatibles"
+	depends on I2C && EXPERIMENTAL
+	select I2C_SENSOR
+	help
+	  If you say yes here you get support for Analog Devices ADM9240,
+	  Dallas DS1780, National Semiconductor LM81 sensor chips.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called adm9240.
+
 config SENSORS_ASB100
 	tristate "Asus ASB100 Bach"
 	depends on I2C && EXPERIMENTAL
drivers/i2c/chips/Makefile +1 -0
diff -X dontdiff -Nru linux-2.6.12-rc3-mm1/drivers/i2c/chips/Makefile linux-2.6.12-rc3-mm1b/drivers/i2c/chips/Makefile
@@ -11,6 +11,7 @@
 obj-$(CONFIG_SENSORS_ADM1025)	+= adm1025.o
 obj-$(CONFIG_SENSORS_ADM1026)	+= adm1026.o
 obj-$(CONFIG_SENSORS_ADM1031)	+= adm1031.o
+obj-$(CONFIG_SENSORS_ADM9240)	+= adm9240.o
 obj-$(CONFIG_SENSORS_DS1337)	+= ds1337.o
 obj-$(CONFIG_SENSORS_DS1621)	+= ds1621.o
 obj-$(CONFIG_SENSORS_EEPROM)	+= eeprom.o
drivers/i2c/chips/adm9240.c +768 -0

768 lines not on this page · raw message

author
#10 lore raw

PATCH 1/1 I2C: New hardware monitor driver: adm9240 (2nd try)

In reply to Grant Coady
Quoted 8 lines · Jean Delvare

Hi Grant,

Quoted 5 lines
Quoted 3 lines
	reg = adm9240_read_value(client, ADM9240_REG_CONFIG);
	adm9240_write_value(client, ADM9240_REG_CONFIG,
		(reg | 0x01) & 0x7f);

That would fail to start the measurement cycle (...)

Besides lm_sensors 2.9.1 does:

/* Called when we have found a new ADM9240. */ static void adm9240_init_client(struct i2c_client *client)

{
        /* Start monitoring */
        adm9240_write_value(client, ADM9240_REG_CONFIG, 0x01);
}

--Grant.

author
#11 lore raw

PATCH 1/1 I2C: New hardware monitor driver: adm9240 (2nd try)

In reply to Grant Coady

Hi Khali,
On Thu, 28 Apr 2005 10:10:27 +0200 (CEST), "Jean Delvare" <[email protected]> wrote:

Please forgive my grumpiness in last reply, I was annoyed at myself for missing that 1 instead of 0 in the cold start code limits clear loop.

Quoted 9 lines
Quoted 6 lines
Quoted 4 lines
	reg = adm9240_read_value(client, ADM9240_REG_CONFIG);
	adm9240_write_value(client, ADM9240_REG_CONFIG,
		(reg | 0x01) & 0x7f);

That would fail to start the measurement cycle (...)

How that? Bit 0 will be set to 1, which starts the measurement cycle, exactly like your previous code did.

Need to clear bit 3, powerup = 1, as well as set bit 0, but:

control reg (40h)
7..4 writing zero harmless
3 INT_clear, power-up default = 1 stops measurement, writing zero required
2,1 writing zero harmless
0 start measurement cycle, write '1' to start measurement cycle

powerup value = 0x08: bit 3 set stops measuring cycle, waiting for interrupt acknowledge, writing 0x01 to start is therefore correct, however, I can change "cold start" test to (control = 0x08), which is much more correct as the observed and datasheet power-up value than (control & 1) NZ indicating measurement cycle is running.

temp mode reg (4bh)
7 ro lsb chip temperature, writing zero harmless
6..2 reserved, = 0, writing zero harmless
1,0 0,3 'default' interrupt mode :)

	1	power up = 1 --> one shot mode (so why 0,3 called default?)
	2	comparator mode, does not assert temp alarm flags

Therefore writing 0 sets mode zero and is harmless. Leaving it at powerup default is also harmless as we not using interrupts and alarm status is correct, comparator mode does not set alarm status for over-temperature. Since we do not provide an accessor for this value the driver should set it to 0 or 1 to enable over-temp alarm status.

If the driver finds the chip running no change at all is made to control or mode.

Maybe I should wait until after 2.6.11.8 before resending patch again, we're just clogging the works?

--Grant.

author
#12 lore raw

PATCH 1/1 I2C: New hardware monitor driver: adm9240 (2nd try)

In reply to Grant Coady

Hi Khali,

Quoted 11 lines · Jean Delvare

Hi Grant,

drivers/i2c/chips/adm9240.c line 647

Quoted 6 lines
drivers/i2c/chips/adm9240.c +6 -0
+		for (i = 1; i < 6; i++) {
+			adm9240_write_value(client,
+					ADM9240_REG_IN_MIN(i), 0);
+			adm9240_write_value(client,
+					ADM9240_REG_IN_MAX(i), 255);
+		}

Shouldn't i start from 0 here?

Yep, missed that :)

Quoted 9 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+		/* start measurement cycle */
+		adm9240_write_value(client, ADM9240_REG_CONFIG, 1);

I'm sorry but I'm still not quite happy with this. You want to change one bit and end up writing 8. What about something along the lines of:

	reg = adm9240_read_value(client, ADM9240_REG_CONFIG);
	adm9240_write_value(client, ADM9240_REG_CONFIG,
		(reg | 0x01) & 0x7f);

That would fail to start the measurement cycle and leave user-space able to assert a hardware interrupt signal, plus it writes zero to bit 7 for what reason?

Thanks,
--Grant.

#13 lore raw

PATCH 1/1 I2C: New hardware monitor driver: adm9240

In reply to Grant Coady

Hi Grant,

Quoted 5 lines

Completion of Michiel Rook's port of adm9240 to 2.6 with addition of auto fan clock divider based on Jean Delvare's algorithm, and replaces scaling macros with static inlines.

This patch is updated against 2.6.12-rc3-mm1, no other changes.

Thanks for the good work! I applied this version to my local tree. Greg, please pick it.

Signature · 1 line
Jean Delvare
Applied
#14 lore raw

PATCH 1/1 I2C: New hardware monitor driver: adm9240

In reply to Grant Coady
Quoted 9 lines · Grant Coady

Hi Greg,

Completion of Michiel Rook's port of adm9240 to 2.6 with addition of auto fan clock divider based on Jean Delvare's algorithm, and replaces scaling macros with static inlines.

This patch is updated against 2.6.12-rc3-mm1, no other changes.

Development on 2.6.11.7 and compile tested on 2.6.12-rc3-mm1.

Applied, thanks.

greg k-h