hwmon: Add LTC4245 driver

RFC PATCH In review Ira Snyder 4 participants 5 messages Oct 15, 2008 – Oct 17, 2008

Subsystems (2)

Documentation Hardware monitoring

lore

Share & tools

lore

Ira Snyder

author

2008-10-15 21:29 UTC #0 lore raw

RFC PATCH v3 1/1 hwmon: Add LTC4245 driver

Add Linux support for the Linear Technology LTC4245 Multiple Supply Hot Swap controller I2C monitoring interface.

Signed-off-by Ira W. Snyder <[email protected]>

---

I've incorporated the suggestions from the latest posting of this driver.

I didn't see an easy way to combine the LTC4245_VOLTAGE and LTC4245_ALARM defines into a single #define. The in[1-4]_min_alarm bits are in the FAULT1 register, while the in[5-8]_min_alarm are in the FAULT2 register. I could do it with one more macro parameter, but I thought that was getting ugly and confusing. Comments?

Also, I used a local variable named "current" in a few places. Checking the driver with sparse, I get a warning that I'm shadowing the "current" variable in arch/powerpc/include/asm/current.h. Should I change the name of my variable, or ignore the warning? There isn't any harm in it...

Thanks again for reviewing the driver.

Changes v2 -> v3:
* fix units (power?_input in uW, curr?_input in mA)
* combine all alarm functions
* rename power[1-4]_alarm to in[5-8]_min_alarm, per suggestions

Changes v1 -> v2:
* fixed checkpatch warnings
* removed raw register access, per suggestions
* changed sysfs interface, per suggestions (current, power, alarms)

 Documentation/hwmon/ltc4245 |   69 +++++
 drivers/hwmon/Kconfig       |   11 +
 drivers/hwmon/Makefile      |    1 +
 drivers/hwmon/ltc4245.c     |  577 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 658 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/hwmon/ltc4245
 create mode 100644 drivers/hwmon/ltc4245.c

Documentation/hwmon/ltc4245 new file +68 -0

new file mode 100644
index 0000000..d2f411a
@@ -0,0 +1,69 @@
+Kernel driver ltc4245
+==========+
+Supported chips:
+  * Linear Technology LTC4245
+    Prefix: 'ltc4245'
+    Addresses scanned: 0x20-0x3f
+    Datasheet:
+        http://www.linear.com/pc/downloadDocument.do?navId=H0,C1,C1003,C1006,C1140,P19392,D13517
+
+Author: Ira W. Snyder <[email protected]>
+
+
+Description
+-----------
+
+The LTC4245 controller allows a board to be safely inserted and removed
+from a live backplane in multiple supply systems such as CompactPCI and
+PCI Express.
+
+
+Sysfs entries
+-------------
+
+The LTC4245 has built-in limits for over and under current warnings. This
+makes it very likely that the reference circuit will be used.
+
+This driver uses the values in the datasheet to change the register values
+into the values specified in the sysfs-interface document. The current readings
+rely on the sense resistors listed in Table 2: "Sense Resistor Values".
+
+in1_input		12v input voltage (mV)
+in2_input		5v  input voltage (mV)
+in3_input		3v  input voltage (mV)
+in4_input		Vee (-12v) input voltage (mV)
+
+in1_min_alarm		12v input undervoltage alarm
+in2_min_alarm		5v  input undervoltage alarm
+in3_min_alarm		3v  input undervoltage alarm
+in4_min_alarm		Vee (-12v) input undervoltage alarm
+
+curr1_input		12v current (mA)
+curr2_input		5v  current (mA)
+curr3_input		3v  current (mA)
+curr4_input		Vee (-12v) current (mA)
+
+curr1_max_alarm		12v overcurrent alarm
+curr2_max_alarm		5v  overcurrent alarm
+curr3_max_alarm		3v  overcurrent alarm
+curr4_max_alarm		Vee (-12v) overcurrent alarm
+
+in5_input		12v output voltage (mV)
+in6_input		5v  output voltage (mV)
+in7_input		3v  output voltage (mV)
+in8_input		Vee (-12v) output voltage (mV)
+
+in5_min_alarm		12v output undervoltage alarm
+in6_min_alarm		5v  output undervoltage alarm
+in7_min_alarm		3v  output undervoltage alarm
+in8_min_alarm		Vee (-12v) output undervoltage alarm
+
+in9_input		GPIO #1 voltage data
+in10_input		GPIO #2 voltage data
+in11_input		GPIO #3 voltage data
+
+power1_input		12v power usage (mW)
+power2_input		5v  power usage (mW)
+power3_input		3v  power usage (mW)
+power4_input		Vee (-12v) power usage (mW)

drivers/hwmon/Kconfig +11 -0

index d402e8d..91bec8a 100644
@@ -836,6 +836,17 @@ config SENSORS_APPLESMC
 	  Say Y here if you have an applicable laptop and want to experience
 	  the awesome power of applesmc.
 
+config SENSORS_LTC4245
+	tristate "Linear Technology LTC4245"
+	depends on I2C && EXPERIMENTAL
+	default n
+	help
+	  If you say yes here you get support for Linear Technology LTC4245
+	  Multiple Supply Hot Swap Controller I2C interface.
+
+	  This driver can also be built as a module. If so, the module will
+	  be called ltc4245.
+
 config HWMON_DEBUG_CHIP
 	bool "Hardware Monitoring Chip debugging messages"
 	default n

drivers/hwmon/Makefile +1 -0

index 950134a..0ed56ac 100644
@@ -59,6 +59,7 @@ obj-$(CONFIG_SENSORS_LM87)	+= lm87.o
 obj-$(CONFIG_SENSORS_LM90)	+= lm90.o
 obj-$(CONFIG_SENSORS_LM92)	+= lm92.o
 obj-$(CONFIG_SENSORS_LM93)	+= lm93.o
+obj-$(CONFIG_SENSORS_LTC4245)	+= ltc4245.o
 obj-$(CONFIG_SENSORS_MAX1619)	+= max1619.o
 obj-$(CONFIG_SENSORS_MAX6650)	+= max6650.o
 obj-$(CONFIG_SENSORS_PC87360)	+= pc87360.o

drivers/hwmon/ltc4245.c new file +577 -0

577 lines not on this page · raw message

Signature · 7 lines

1.5.4.3


_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Hans de Goede

2008-10-16 20:38 UTC #1 lore raw

Quoted 6 lines · Ira Snyder

Add Linux support for the Linear Technology LTC4245 Multiple Supply Hot Swap controller I2C monitoring interface.
Signed-off-by Ira W. Snyder <[email protected]>
---

Quoted 9 lines

I've incorporated the suggestions from the latest posting of this driver.
I didn't see an easy way to combine the LTC4245_VOLTAGE and LTC4245_ALARM defines into a single #define. The in[1-4]_min_alarm bits are in the FAULT1 register, while the in[5-8]_min_alarm are in the FAULT2 register. I could do it with one more macro parameter, but I thought that was getting ugly and confusing. Comments?

Looks good now, I've only got one minor issue left (which I'm afraid I missed in my reviews before).

Quoted 5 lines

Also, I used a local variable named "current" in a few places. Checking the driver with sparse, I get a warning that I'm shadowing the "current" variable in arch/powerpc/include/asm/current.h. Should I change the name of my variable, or ignore the warning? There isn't any harm in it...

Just ignore the warning.

<snip>

drivers/hwmon/ltc4245.c line 121

Quoted 3 lines

drivers/hwmon/ltc4245.c new file +3 -0

+/* Return the voltage from the given register in millivolts */
+static u32 ltc4245_get_voltage(struct device *dev, u8 reg)
+{

Why dont you make this an s32 (or just an int) and then ..

Quoted 14 lines

drivers/hwmon/ltc4245.c new file +14 -0

+	struct ltc4245_data *data = ltc4245_update_device(dev);
+	const s32 val = data->vregs[reg - 0x10];
+	const u8 regval = val;
+	u32 voltage = 0;
+
+	if (unlikely(val < 0))
+		return 0;
+
+	switch (reg) {
+	case LTC4245_12VIN:
+	case LTC4245_12VOUT:
+		voltage = regval * 55;
+		break;
+	case LTC4245_5VIN:

drivers/hwmon/ltc4245.c line 145

Quoted 10 lines

drivers/hwmon/ltc4245.c new file +10 -0

+	case LTC4245_5VOUT:
+		voltage = regval * 22;
+		break;
+	case LTC4245_3VIN:
+	case LTC4245_3VOUT:
+		voltage = regval * 15;
+		break;
+	case LTC4245_VEEIN:
+	case LTC4245_VEEOUT:
+		voltage = regval * 55;

Make this * -55, and ...

<snip>

drivers/hwmon/ltc4245.c line 225

Quoted 13 lines

drivers/hwmon/ltc4245.c new file +13 -0

+
+static ssize_t ltc4245_show_voltage(struct device *dev,
+				    struct device_attribute *da,
+				    char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
+	const u32 voltage = ltc4245_get_voltage(dev, attr->index);
+
+	/* The VEEIN and VEEOUT registers are for -12V, so
+	 * we add the negative sign on the output */
+	if (attr->index = LTC4245_VEEIN || attr->index = LTC4245_VEEOUT)
+		return snprintf(buf, PAGE_SIZE, "%d\n", voltage * -1);
+

Remove this special case, and make the %u below %d ?

drivers/hwmon/ltc4245.c line 228

Quoted 3 lines

drivers/hwmon/ltc4245.c new file +3 -0
+	return snprintf(buf, PAGE_SIZE, "%u\n", voltage);
+}
+

<snip>

Regards,

Hans

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Ira Snyder

author

2008-10-16 20:50 UTC #2 lore raw

In reply to Ira Snyder

On Thu, Oct 16, 2008 at 10:38:10PM +0200, Hans de Goede wrote:

snip

Quoted 6 lines

drivers/hwmon/ltc4245.c line 121
Quoted 3 lines
drivers/hwmon/ltc4245.c new file +3 -0
+/* Return the voltage from the given register in millivolts */
+static u32 ltc4245_get_voltage(struct device *dev, u8 reg)
+{
Why dont you make this an s32 (or just an int) and then ..

snip

Quoted 6 lines

drivers/hwmon/ltc4245.c line 145
Quoted 3 lines
drivers/hwmon/ltc4245.c new file +3 -0
+	case LTC4245_VEEIN:
+	case LTC4245_VEEOUT:
+		voltage = regval * 55;
Make this * -55, and ...

Great idea. I didn't even think of that. :) I'll post up a final patch after I hear back on the stuff below.

Is it ok to have a negative power? The current value gets used to calculate the (virtual) power in ltc4245_show_power(), but that's it.

Thanks again for the review, it has really improved this driver a lot. Ira

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Jean Delvare

2008-10-16 20:56 UTC #3 lore raw

In reply to Ira Snyder

Quoted 20 lines · Ira Snyder

On Thu, Oct 16, 2008 at 10:38:10PM +0200, Hans de Goede wrote:
snip
Quoted 6 lines
drivers/hwmon/ltc4245.c line 121
Quoted 3 lines
drivers/hwmon/ltc4245.c new file +3 -0
+/* Return the voltage from the given register in millivolts */
+static u32 ltc4245_get_voltage(struct device *dev, u8 reg)
+{
Why dont you make this an s32 (or just an int) and then ..
snip
Quoted 6 lines
drivers/hwmon/ltc4245.c line 145
Quoted 3 lines
drivers/hwmon/ltc4245.c new file +3 -0
+	case LTC4245_VEEIN:
+	case LTC4245_VEEOUT:
+		voltage = regval * 55;
Make this * -55, and ...

Quoted 5 lines

Great idea. I didn't even think of that. :) I'll post up a final patch after I hear back on the stuff below.
Is it ok to have a negative power? The current value gets used to calculate the (virtual) power in ltc4245_show_power(), but that's it.

I'd use the absolute value for power. A negative power sounds weird.

Signature · 6 lines

Jean Delvare

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Hans de Goede

2008-10-17 06:27 UTC #4 lore raw

In reply to Ira Snyder

Quoted 17 lines · Jean Delvare

Quoted 16 lines · Ira Snyder
On Thu, Oct 16, 2008 at 10:38:10PM +0200, Hans de Goede wrote:
snip
Quoted 5 lines
drivers/hwmon/ltc4245.c line 121
Quoted 3 lines
drivers/hwmon/ltc4245.c new file +3 -0
+/* Return the voltage from the given register in millivolts */
+static u32 ltc4245_get_voltage(struct device *dev, u8 reg)
+{
Why dont you make this an s32 (or just an int) and then ..
snip
Quoted 5 lines
drivers/hwmon/ltc4245.c line 145
Quoted 3 lines
drivers/hwmon/ltc4245.c new file +3 -0
+	case LTC4245_VEEIN:
+	case LTC4245_VEEOUT:
+		voltage = regval * 55;
Make this * -55, and ...

Quoted 8 lines

Quoted 5 lines
Great idea. I didn't even think of that. :) I'll post up a final patch after I hear back on the stuff below.
Is it ok to have a negative power? The current value gets used to calculate the (virtual) power in ltc4245_show_power(), but that's it.
I'd use the absolute value for power. A negative power sounds weird.

Regards,

Hans

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors