[PATCH] hwmon: Add LTC2990 sensor driver

[PATCH] hwmon: Add LTC2990 sensor driver

[Mike Looijmans]

This adds support for the Linear Technology LTC2990  I2C System Monitor.
The LTC2990 supports a combination of voltage, current and temperature
monitoring, but this driver currently only supports reading two currents
by measuring two differential voltages across series resistors.

This is sufficient to support the Topic Miami SOM which uses this chip
to monitor the currents flowing into the FPGA and the CPU parts.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
---
 drivers/hwmon/Kconfig   |  15 +++
 drivers/hwmon/Makefile  |   1 +
 drivers/hwmon/ltc2990.c | 273 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 289 insertions(+)
 create mode 100644 drivers/hwmon/ltc2990.c

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 80a73bf..b3eef31 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -685,6 +685,21 @@ config SENSORS_LTC2945
 	  This driver can also be built as a module. If so, the module will
 	  be called ltc2945.
 
+config SENSORS_LTC2990
+	tristate "Linear Technology LTC2990 (current monitoring mode only)"
+	depends on I2C
+	select REGMAP_I2C
+	default n
+	help
+	  If you say yes here you get support for Linear Technology LTC2990
+	  I2C System Monitor. The LTC2990 supports a combination of voltage,
+	  current and temperature monitoring, but this driver currently only
+	  supports reading two currents by measuring two differential voltages
+	  across series resistors.
+
+	  This driver can also be built as a module. If so, the module will
+	  be called ltc2990.
+
 config SENSORS_LTC4151
 	tristate "Linear Technology LTC4151"
 	depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 12a3239..e4bd15b 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -101,6 +101,7 @@ obj-$(CONFIG_SENSORS_LM95234)	+= lm95234.o
 obj-$(CONFIG_SENSORS_LM95241)	+= lm95241.o
 obj-$(CONFIG_SENSORS_LM95245)	+= lm95245.o
 obj-$(CONFIG_SENSORS_LTC2945)	+= ltc2945.o
+obj-$(CONFIG_SENSORS_LTC2990)	+= ltc2990.o
 obj-$(CONFIG_SENSORS_LTC4151)	+= ltc4151.o
 obj-$(CONFIG_SENSORS_LTC4215)	+= ltc4215.o
 obj-$(CONFIG_SENSORS_LTC4222)	+= ltc4222.o
diff --git a/drivers/hwmon/ltc2990.c b/drivers/hwmon/ltc2990.c
new file mode 100644
index 0000000..161d995
--- /dev/null
+++ b/drivers/hwmon/ltc2990.c
@@ -0,0 +1,273 @@
+/*
+ * driver for Linear Technology LTC2990 power monitor
+ *
+ * Copyright (C) 2014 Topic Embedded Products
+ * Author: Mike Looijmans <mike.looijmans@topic.nl>
+ *
+ * License: GPLv2
+ *
+ * This driver assumes the chip is wired as a dual current monitor, and
+ * reports the voltage drop across two series resistors.
+ */
+
+#include <linux/bug.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+
+#define LTC2990_STATUS	0x00
+#define LTC2990_CONTROL	0x01
+#define LTC2990_TRIGGER	0x02
+#define LTC2990_TINT_MSB	0x04
+#define LTC2990_TINT_LSB	0x05
+#define LTC2990_V1_MSB	0x06
+#define LTC2990_V1_LSB	0x07
+#define LTC2990_V2_MSB	0x08
+#define LTC2990_V2_LSB	0x09
+#define LTC2990_V3_MSB	0x0A
+#define LTC2990_V3_LSB	0x0B
+#define LTC2990_V4_MSB	0x0C
+#define LTC2990_V4_LSB	0x0D
+#define LTC2990_VCC_MSB	0x0E
+#define LTC2990_VCC_LSB	0x0F
+
+#define LTC2990_STATUS_BUSY	BIT(0)
+#define LTC2990_STATUS_TINT	BIT(1)
+#define LTC2990_STATUS_V1	BIT(2)
+#define LTC2990_STATUS_V2	BIT(3)
+#define LTC2990_STATUS_V3	BIT(4)
+#define LTC2990_STATUS_V4	BIT(5)
+#define LTC2990_STATUS_VCC	BIT(6)
+
+/* Only define control settings we actually use */
+#define LTC2990_CONTROL_KELVIN		BIT(7)
+#define LTC2990_CONTROL_SINGLE		BIT(6)
+#define LTC2990_CONTROL_MEASURE_ALL	(0x3 << 3)
+#define LTC2990_CONTROL_MODE_CURRENT	0x06
+#define LTC2990_CONTROL_MODE_VOLTAGE	0x07
+
+struct ltc2990_data {
+	struct device *hwmon_dev;
+	struct mutex update_lock;
+	unsigned long last_updated;
+	short values[6];
+	bool valid;
+	u8 update_counter;
+};
+
+static int ltc2990_write(struct i2c_client *i2c, u8 reg, u8 value)
+{
+	return i2c_smbus_write_byte_data(i2c, reg, value);
+}
+
+static int ltc2990_read_byte(struct i2c_client *i2c, u8 reg)
+{
+	return i2c_smbus_read_byte_data(i2c, reg);
+}
+
+static int ltc2990_read_word(struct i2c_client *i2c, u8 reg)
+{
+	int result = i2c_smbus_read_word_data(i2c, reg);
+	/* Result is MSB first, but smbus specs say LSB first, so swap the
+	 * result */
+	return result < 0 ? result : swab16(result);
+}
+
+static struct ltc2990_data *ltc2990_update_device(struct device *dev)
+{
+	struct i2c_client *i2c = to_i2c_client(dev);
+	struct ltc2990_data *data = i2c_get_clientdata(i2c);
+	struct ltc2990_data *ret = data;
+	unsigned int timeout;
+
+	mutex_lock(&data->update_lock);
+
+	/* Update about 4 times per second max */
+	if (time_after(jiffies, data->last_updated + HZ / 4) || !data->valid) {
+		int val;
+		int i;
+
+		/* Trigger ADC, any value will do */
+		val = ltc2990_write(i2c, LTC2990_TRIGGER, 1);
+		if (unlikely(val < 0)) {
+			ret = ERR_PTR(val);
+			goto abort;
+		}
+
+		/* Wait for conversion complete */
+		timeout = 200;
+		for (;;) {
+			usleep_range(2000, 4000);
+			val = ltc2990_read_byte(i2c, LTC2990_STATUS);
+			if (unlikely(val < 0)) {
+				ret = ERR_PTR(val);
+				goto abort;
+			}
+			/* Single-shot mode, wait for conversion to complete */
+			if ((val & LTC2990_STATUS_BUSY) == 0)
+				break;
+			if (--timeout == 0) {
+				ret = ERR_PTR(-ETIMEDOUT);
+				goto abort;
+			}
+		}
+
+		/* Read all registers */
+		for (i = 0; i < ARRAY_SIZE(data->values); ++i) {
+			val = ltc2990_read_word(i2c, (i<<1) + LTC2990_TINT_MSB);
+			if (unlikely(val < 0)) {
+				dev_dbg(dev,
+					"Failed to read ADC value: error %d\n",
+					val);
+				ret = ERR_PTR(val);
+				goto abort;
+			}
+			data->values[i] = val & 0x7FFF; /* Strip 'new' bit */
+		}
+		data->last_updated = jiffies;
+		data->valid = 1;
+
+		/*
+		 *  Quirk: Second trigger is ignored? After this, the BUSY will
+		 * still be set to "0" and no conversion performed.
+		 */
+		val = ltc2990_write(i2c, LTC2990_TRIGGER, 0);
+	}
+abort:
+	mutex_unlock(&data->update_lock);
+	return ret;
+}
+
+/* Return the converted value from the given register in uV or mC */
+static int ltc2990_get_value(struct ltc2990_data *data, u8 index)
+{
+	s32 result;
+	s16 v;
+
+	if (index == 0) { /* internal temp, 0.0625 degrees/LSB, 12-bit  */
+		v = data->values[index] << 3;
+		result = (s32)v * 1000 >> 7;
+	} else if (index < 5) { /* Vx-Vy, 19.42uV/LSB, 14-bit */
+		v = data->values[index] << 2;
+		result = (s32)v * 1942 / (4 * 100);
+	} else { /* Vcc, 305.18μV/LSB, 2.5V offset, 14-bit */
+		v = data->values[index] << 2;
+		result = (s32)v * 30518 / (4 * 100);
+		result += 2500000;
+	}
+	return result;
+}
+
+static ssize_t ltc2990_show_value(struct device *dev,
+				  struct device_attribute *da, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
+	struct ltc2990_data *data = ltc2990_update_device(dev);
+	int value;
+
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
+	value = ltc2990_get_value(data, attr->index);
+	return snprintf(buf, PAGE_SIZE, "%d\n", value);
+}
+
+static SENSOR_DEVICE_ATTR(temp_int, S_IRUGO, ltc2990_show_value, NULL, 0);
+static SENSOR_DEVICE_ATTR(v1v2_diff, S_IRUGO, ltc2990_show_value, NULL, 1);
+static SENSOR_DEVICE_ATTR(v3v4_diff, S_IRUGO, ltc2990_show_value, NULL, 3);
+static SENSOR_DEVICE_ATTR(vcc, S_IRUGO, ltc2990_show_value, NULL, 5);
+
+static struct attribute *ltc2990_attributes[] = {
+	&sensor_dev_attr_temp_int.dev_attr.attr,
+	&sensor_dev_attr_v1v2_diff.dev_attr.attr,
+	&sensor_dev_attr_v3v4_diff.dev_attr.attr,
+	&sensor_dev_attr_vcc.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group ltc2990_group = {
+	.attrs = ltc2990_attributes,
+};
+
+static int ltc2990_i2c_probe(
+	struct i2c_client *i2c, const struct i2c_device_id *id)
+{
+	int ret;
+	struct ltc2990_data *ltc2990;
+
+	if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
+		return -ENODEV;
+
+	ltc2990 = devm_kzalloc(&i2c->dev,
+		sizeof(struct ltc2990_data), GFP_KERNEL);
+	if (ltc2990 == NULL)
+		return -ENOMEM;
+
+	ret = ltc2990_read_byte(i2c, 0);
+	if (ret < 0) {
+		dev_err(&i2c->dev, "Could not read LTC2990 on i2c bus.\n");
+		return ret;
+	}
+	ret = ltc2990_write(i2c, LTC2990_CONTROL,
+		LTC2990_CONTROL_SINGLE | LTC2990_CONTROL_MEASURE_ALL |
+		LTC2990_CONTROL_MODE_CURRENT);
+	if (ret < 0) {
+		dev_err(&i2c->dev, "Error: Failed to set control mode.\n");
+		return ret;
+	}
+
+	mutex_init(&ltc2990->update_lock);
+	i2c_set_clientdata(i2c, ltc2990);
+
+	/* Register sysfs hooks */
+	ret = sysfs_create_group(&i2c->dev.kobj, &ltc2990_group);
+	if (ret)
+		return ret;
+
+	ltc2990->hwmon_dev = hwmon_device_register(&i2c->dev);
+	if (IS_ERR(ltc2990->hwmon_dev)) {
+		ret = PTR_ERR(ltc2990->hwmon_dev);
+		goto out_hwmon_device_register;
+	}
+
+	return 0;
+
+out_hwmon_device_register:
+	sysfs_remove_group(&i2c->dev.kobj, &ltc2990_group);
+	return ret;
+}
+
+static int ltc2990_i2c_remove(struct i2c_client *i2c)
+{
+	struct ltc2990_data *ltc2990 = i2c_get_clientdata(i2c);
+
+	hwmon_device_unregister(ltc2990->hwmon_dev);
+	sysfs_remove_group(&i2c->dev.kobj, &ltc2990_group);
+	return 0;
+}
+
+static const struct i2c_device_id ltc2990_i2c_id[] = {
+	{ "ltc2990", 0 },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, ltc2990_i2c_id);
+
+static struct i2c_driver ltc2990_i2c_driver = {
+	.driver = {
+		.name = "ltc2990",
+	},
+	.probe    = ltc2990_i2c_probe,
+	.remove   = ltc2990_i2c_remove,
+	.id_table = ltc2990_i2c_id,
+};
+
+module_i2c_driver(ltc2990_i2c_driver);
+
+MODULE_DESCRIPTION("LTC2990 Sensor Driver");
+MODULE_AUTHOR("Topic Embedded Products");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

Re: [PATCH] hwmon: Add LTC2990 sensor driver

[Guenter Roeck]

Hello Mike,

On 01/06/2016 12:07 AM, Mike Looijmans wrote:
> This adds support for the Linear Technology LTC2990  I2C System Monitor.

s/  / /

> The LTC2990 supports a combination of voltage, current and temperature
> monitoring, but this driver currently only supports reading two currents
> by measuring two differential voltages across series resistors.
>
Plus VCC, plus the internal temperature.

> This is sufficient to support the Topic Miami SOM which uses this chip
> to monitor the currents flowing into the FPGA and the CPU parts.
>
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> ---
>   drivers/hwmon/Kconfig   |  15 +++
>   drivers/hwmon/Makefile  |   1 +
>   drivers/hwmon/ltc2990.c | 273 ++++++++++++++++++++++++++++++++++++++++++++++++

Please also provide Documentation/hwmon/ltc2990.

Also, please read and follow Documentation/hwmon/submitting-patches.

>   3 files changed, 289 insertions(+)
>   create mode 100644 drivers/hwmon/ltc2990.c
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 80a73bf..b3eef31 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -685,6 +685,21 @@ config SENSORS_LTC2945
>   	  This driver can also be built as a module. If so, the module will
>   	  be called ltc2945.
>
> +config SENSORS_LTC2990
> +	tristate "Linear Technology LTC2990 (current monitoring mode only)"
> +	depends on I2C
> +	select REGMAP_I2C

Using regmap for the driver might be a good idea, but you don't.

> +	default n

Not necessary.

> +	help
> +	  If you say yes here you get support for Linear Technology LTC2990
> +	  I2C System Monitor. The LTC2990 supports a combination of voltage,
> +	  current and temperature monitoring, but this driver currently only
> +	  supports reading two currents by measuring two differential voltages
> +	  across series resistors.
> +
> +	  This driver can also be built as a module. If so, the module will
> +	  be called ltc2990.
> +
>   config SENSORS_LTC4151
>   	tristate "Linear Technology LTC4151"
>   	depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 12a3239..e4bd15b 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -101,6 +101,7 @@ obj-$(CONFIG_SENSORS_LM95234)	+= lm95234.o
>   obj-$(CONFIG_SENSORS_LM95241)	+= lm95241.o
>   obj-$(CONFIG_SENSORS_LM95245)	+= lm95245.o
>   obj-$(CONFIG_SENSORS_LTC2945)	+= ltc2945.o
> +obj-$(CONFIG_SENSORS_LTC2990)	+= ltc2990.o
>   obj-$(CONFIG_SENSORS_LTC4151)	+= ltc4151.o
>   obj-$(CONFIG_SENSORS_LTC4215)	+= ltc4215.o
>   obj-$(CONFIG_SENSORS_LTC4222)	+= ltc4222.o
> diff --git a/drivers/hwmon/ltc2990.c b/drivers/hwmon/ltc2990.c
> new file mode 100644
> index 0000000..161d995
> --- /dev/null
> +++ b/drivers/hwmon/ltc2990.c
> @@ -0,0 +1,273 @@
> +/*
> + * driver for Linear Technology LTC2990 power monitor

Driver

> + *
> + * Copyright (C) 2014 Topic Embedded Products

2015 ?

> + * Author: Mike Looijmans <mike.looijmans@topic.nl>
> + *
> + * License: GPLv2
> + *
> + * This driver assumes the chip is wired as a dual current monitor, and
> + * reports the voltage drop across two series resistors.

It also monitors the temperature and VCC.

> + */
> +
> +#include <linux/bug.h>

Is this used anywhere in the driver ?

> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +
> +#define LTC2990_STATUS	0x00
> +#define LTC2990_CONTROL	0x01
> +#define LTC2990_TRIGGER	0x02
> +#define LTC2990_TINT_MSB	0x04
> +#define LTC2990_TINT_LSB	0x05
> +#define LTC2990_V1_MSB	0x06
> +#define LTC2990_V1_LSB	0x07
> +#define LTC2990_V2_MSB	0x08
> +#define LTC2990_V2_LSB	0x09
> +#define LTC2990_V3_MSB	0x0A
> +#define LTC2990_V3_LSB	0x0B
> +#define LTC2990_V4_MSB	0x0C
> +#define LTC2990_V4_LSB	0x0D
> +#define LTC2990_VCC_MSB	0x0E
> +#define LTC2990_VCC_LSB	0x0F
> +
> +#define LTC2990_STATUS_BUSY	BIT(0)
> +#define LTC2990_STATUS_TINT	BIT(1)
> +#define LTC2990_STATUS_V1	BIT(2)
> +#define LTC2990_STATUS_V2	BIT(3)
> +#define LTC2990_STATUS_V3	BIT(4)
> +#define LTC2990_STATUS_V4	BIT(5)
> +#define LTC2990_STATUS_VCC	BIT(6)
> +
> +/* Only define control settings we actually use */
> +#define LTC2990_CONTROL_KELVIN		BIT(7)
> +#define LTC2990_CONTROL_SINGLE		BIT(6)
> +#define LTC2990_CONTROL_MEASURE_ALL	(0x3 << 3)
> +#define LTC2990_CONTROL_MODE_CURRENT	0x06
> +#define LTC2990_CONTROL_MODE_VOLTAGE	0x07
> +
> +struct ltc2990_data {
> +	struct device *hwmon_dev;
> +	struct mutex update_lock;
> +	unsigned long last_updated;
> +	short values[6];

u16 ?

> +	bool valid;
> +	u8 update_counter;

Not used anywhere.

> +};
> +
> +static int ltc2990_write(struct i2c_client *i2c, u8 reg, u8 value)
> +{
> +	return i2c_smbus_write_byte_data(i2c, reg, value);
> +}
> +
> +static int ltc2990_read_byte(struct i2c_client *i2c, u8 reg)
> +{
> +	return i2c_smbus_read_byte_data(i2c, reg);
> +}
> +

Useless shim functions.

> +static int ltc2990_read_word(struct i2c_client *i2c, u8 reg)
> +{
> +	int result = i2c_smbus_read_word_data(i2c, reg);
> +	/* Result is MSB first, but smbus specs say LSB first, so swap the
> +	 * result */

Bad multi-line comment.

> +	return result < 0 ? result : swab16(result);

Please use i2c_smbus_read_word_swapped() and drop the shim function.

> +}
> +
> +static struct ltc2990_data *ltc2990_update_device(struct device *dev)
> +{
> +	struct i2c_client *i2c = to_i2c_client(dev);
> +	struct ltc2990_data *data = i2c_get_clientdata(i2c);
> +	struct ltc2990_data *ret = data;
> +	unsigned int timeout;
> +
> +	mutex_lock(&data->update_lock);
> +
> +	/* Update about 4 times per second max */
> +	if (time_after(jiffies, data->last_updated + HZ / 4) || !data->valid) {
> +		int val;
> +		int i;
> +

Please consider using continuous conversion. This would simplify the code significantly
and reduce read delays.

> +		/* Trigger ADC, any value will do */
> +		val = ltc2990_write(i2c, LTC2990_TRIGGER, 1);
> +		if (unlikely(val < 0)) {
> +			ret = ERR_PTR(val);
> +			goto abort;
> +		}
> +
> +		/* Wait for conversion complete */
> +		timeout = 200;
> +		for (;;) {
> +			usleep_range(2000, 4000);
> +			val = ltc2990_read_byte(i2c, LTC2990_STATUS);
> +			if (unlikely(val < 0)) {
> +				ret = ERR_PTR(val);
> +				goto abort;
> +			}
> +			/* Single-shot mode, wait for conversion to complete */
> +			if ((val & LTC2990_STATUS_BUSY) == 0)

			if (!(...))

> +				break;
> +			if (--timeout == 0) {
> +				ret = ERR_PTR(-ETIMEDOUT);
> +				goto abort;
> +			}
> +		}

Again, please consider using continuous conversion mode.

If this is not feasible for some reason, you might as well just wait for the
minimum conversion time before trying to read for the first time. If so,
please use a fixed timeout by comparing the elapsed time instead of looping
for a maximum number of times. Not even counting the time for executing the
code, the maximum delay is between 400 ms and 800 ms, which is way too high
(chip spec says 167 ms worst case, if three temperature sensors are configured).

> +
> +		/* Read all registers */
> +		for (i = 0; i < ARRAY_SIZE(data->values); ++i) {
> +			val = ltc2990_read_word(i2c, (i<<1) + LTC2990_TINT_MSB);

Missing spaces around <<

> +			if (unlikely(val < 0)) {
> +				dev_dbg(dev,
> +					"Failed to read ADC value: error %d\n",
> +					val);
> +				ret = ERR_PTR(val);
> +				goto abort;
> +			}
> +			data->values[i] = val & 0x7FFF; /* Strip 'new' bit */

The bit is never evaluated, so you might as well store the raw value.

> +		}
> +		data->last_updated = jiffies;
> +		data->valid = 1;

	= true;

> +
> +		/*
> +		 *  Quirk: Second trigger is ignored? After this, the BUSY will
> +		 * still be set to "0" and no conversion performed.
> +		 */
> +		val = ltc2990_write(i2c, LTC2990_TRIGGER, 0);
> +	}
> +abort:
> +	mutex_unlock(&data->update_lock);
> +	return ret;
> +}
> +
> +/* Return the converted value from the given register in uV or mC */
> +static int ltc2990_get_value(struct ltc2990_data *data, u8 index)
> +{
> +	s32 result;
> +	s16 v;
> +
> +	if (index == 0) { /* internal temp, 0.0625 degrees/LSB, 12-bit  */
> +		v = data->values[index] << 3;
> +		result = (s32)v * 1000 >> 7;
> +	} else if (index < 5) { /* Vx-Vy, 19.42uV/LSB, 14-bit */
> +		v = data->values[index] << 2;

Datasheet says that the sign bit is in bit 14, so this drops the sign bit.

> +		result = (s32)v * 1942 / (4 * 100);
> +	} else { /* Vcc, 305.18μV/LSB, 2.5V offset, 14-bit */
> +		v = data->values[index] << 2;
> +		result = (s32)v * 30518 / (4 * 100);
> +		result += 2500000;
> +	}
> +	return result;
> +}
> +
> +static ssize_t ltc2990_show_value(struct device *dev,
> +				  struct device_attribute *da, char *buf)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> +	struct ltc2990_data *data = ltc2990_update_device(dev);
> +	int value;
> +
> +	if (IS_ERR(data))
> +		return PTR_ERR(data);
> +
> +	value = ltc2990_get_value(data, attr->index);
> +	return snprintf(buf, PAGE_SIZE, "%d\n", value);
> +}
> +
> +static SENSOR_DEVICE_ATTR(temp_int, S_IRUGO, ltc2990_show_value, NULL, 0);
> +static SENSOR_DEVICE_ATTR(v1v2_diff, S_IRUGO, ltc2990_show_value, NULL, 1);
> +static SENSOR_DEVICE_ATTR(v3v4_diff, S_IRUGO, ltc2990_show_value, NULL, 3);
> +static SENSOR_DEVICE_ATTR(vcc, S_IRUGO, ltc2990_show_value, NULL, 5);
> +

Please use standard attribute names (and units) as per Documentation/hwmon/sysfs-interface.

> +static struct attribute *ltc2990_attributes[] = {
> +	&sensor_dev_attr_temp_int.dev_attr.attr,
> +	&sensor_dev_attr_v1v2_diff.dev_attr.attr,
> +	&sensor_dev_attr_v3v4_diff.dev_attr.attr,
> +	&sensor_dev_attr_vcc.dev_attr.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group ltc2990_group = {
> +	.attrs = ltc2990_attributes,
> +};
> +

Please use the ATTRIBUTE_GROUPS() macro. Also see below.

> +static int ltc2990_i2c_probe(
> +	struct i2c_client *i2c, const struct i2c_device_id *id)

Please split as

static int ltc2990_i2c_probe(struct i2c_client *i2c,
			     const struct i2c_device_id *id)

> +{
> +	int ret;
> +	struct ltc2990_data *ltc2990;
> +
> +	if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> +		return -ENODEV;
> +
> +	ltc2990 = devm_kzalloc(&i2c->dev,
> +		sizeof(struct ltc2990_data), GFP_KERNEL);

Please align continuation lines with '('.

> +	if (ltc2990 == NULL)
> +		return -ENOMEM;
> +
> +	ret = ltc2990_read_byte(i2c, 0);
> +	if (ret < 0) {
> +		dev_err(&i2c->dev, "Could not read LTC2990 on i2c bus.\n");
> +		return ret;
> +	}

The write below would also return an error if the chip isn't there,
so this additional read does not provide any real value.

> +	ret = ltc2990_write(i2c, LTC2990_CONTROL,
> +		LTC2990_CONTROL_SINGLE | LTC2990_CONTROL_MEASURE_ALL |
> +		LTC2990_CONTROL_MODE_CURRENT);

I'll have to think about this. While it addresses your use case,
it limits the scope of this driver significantly. Looking into the board
specifications, you might as well do it right and determine (and set)
the correct configuration using devicetree data instead of forcing your
use case on everyone.

Sure, you may argue that you don't care, but we will be the ones who will have
to handle error reports that the driver unexpectedly changes the configuration
in other use cases (where, for example, the mode may have been pre-set by
the BIOS or ROMMON).

> +	if (ret < 0) {
> +		dev_err(&i2c->dev, "Error: Failed to set control mode.\n");
> +		return ret;
> +	}
> +
> +	mutex_init(&ltc2990->update_lock);
> +	i2c_set_clientdata(i2c, ltc2990);
> +
> +	/* Register sysfs hooks */
> +	ret = sysfs_create_group(&i2c->dev.kobj, &ltc2990_group);
> +	if (ret)
> +		return ret;
> +
> +	ltc2990->hwmon_dev = hwmon_device_register(&i2c->dev);

Please use devm_hwmon_device_register_with_groups().

> +	if (IS_ERR(ltc2990->hwmon_dev)) {
> +		ret = PTR_ERR(ltc2990->hwmon_dev);
> +		goto out_hwmon_device_register;
> +	}
> +
> +	return 0;
> +
> +out_hwmon_device_register:
> +	sysfs_remove_group(&i2c->dev.kobj, &ltc2990_group);
> +	return ret;
> +}
> +
> +static int ltc2990_i2c_remove(struct i2c_client *i2c)
> +{
> +	struct ltc2990_data *ltc2990 = i2c_get_clientdata(i2c);
> +
> +	hwmon_device_unregister(ltc2990->hwmon_dev);
> +	sysfs_remove_group(&i2c->dev.kobj, &ltc2990_group);
> +	return 0;
> +}
> +
> +static const struct i2c_device_id ltc2990_i2c_id[] = {
> +	{ "ltc2990", 0 },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, ltc2990_i2c_id);
> +
> +static struct i2c_driver ltc2990_i2c_driver = {
> +	.driver = {
> +		.name = "ltc2990",
> +	},
> +	.probe    = ltc2990_i2c_probe,
> +	.remove   = ltc2990_i2c_remove,
> +	.id_table = ltc2990_i2c_id,
> +};
> +
> +module_i2c_driver(ltc2990_i2c_driver);
> +
> +MODULE_DESCRIPTION("LTC2990 Sensor Driver");
> +MODULE_AUTHOR("Topic Embedded Products");
> +MODULE_LICENSE("GPL v2");
>

Re: [PATCH] hwmon: Add LTC2990 sensor driver

[Mike Looijmans]

Thank you very much for your review comments, I'll update the driver and 
post a v2 patch.

Inlined some replies below. Assume that I "will do" for all comments I 
didn't comment on inline...

On 06-01-16 16:22, Guenter Roeck wrote:
> Hello Mike,
>
> On 01/06/2016 12:07 AM, Mike Looijmans wrote:
>> This adds support for the Linear Technology LTC2990  I2C System Monitor.
>
> s/  / /
>
>> The LTC2990 supports a combination of voltage, current and temperature
>> monitoring, but this driver currently only supports reading two currents
>> by measuring two differential voltages across series resistors.
>>
> Plus VCC, plus the internal temperature.

Yeah, I should give myself more credit :) I'll add that in Kconfig too.

>> This is sufficient to support the Topic Miami SOM which uses this chip
>> to monitor the currents flowing into the FPGA and the CPU parts.
>>
>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
>> ---
>>   drivers/hwmon/Kconfig   |  15 +++
>>   drivers/hwmon/Makefile  |   1 +
>>   drivers/hwmon/ltc2990.c | 273
>> ++++++++++++++++++++++++++++++++++++++++++++++++
>
> Please also provide Documentation/hwmon/ltc2990.
>
> Also, please read and follow Documentation/hwmon/submitting-patches.
>
>>   3 files changed, 289 insertions(+)
>>   create mode 100644 drivers/hwmon/ltc2990.c
>>
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 80a73bf..b3eef31 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -685,6 +685,21 @@ config SENSORS_LTC2945
>>         This driver can also be built as a module. If so, the module will
>>         be called ltc2945.
>>
>> +config SENSORS_LTC2990
>> +    tristate "Linear Technology LTC2990 (current monitoring mode only)"
>> +    depends on I2C
>> +    select REGMAP_I2C
>
> Using regmap for the driver might be a good idea, but you don't.

Looking at the regmap, the driver only writes the one cachable register 
once, the "control". All the other registers cannot be cached, are 
either read-only or write-only. Don't think regmap will help, so I'll 
remove the "select" here.

>> +    default n
>
> Not necessary.
>
>> +    help
>> +      If you say yes here you get support for Linear Technology LTC2990
>> +      I2C System Monitor. The LTC2990 supports a combination of voltage,
>> +      current and temperature monitoring, but this driver currently only
>> +      supports reading two currents by measuring two differential
>> voltages
>> +      across series resistors.
>> +
>> +      This driver can also be built as a module. If so, the module will
>> +      be called ltc2990.
>> +
>>   config SENSORS_LTC4151
>>       tristate "Linear Technology LTC4151"
>>       depends on I2C
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index 12a3239..e4bd15b 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -101,6 +101,7 @@ obj-$(CONFIG_SENSORS_LM95234)    += lm95234.o
>>   obj-$(CONFIG_SENSORS_LM95241)    += lm95241.o
>>   obj-$(CONFIG_SENSORS_LM95245)    += lm95245.o
>>   obj-$(CONFIG_SENSORS_LTC2945)    += ltc2945.o
>> +obj-$(CONFIG_SENSORS_LTC2990)    += ltc2990.o
>>   obj-$(CONFIG_SENSORS_LTC4151)    += ltc4151.o
>>   obj-$(CONFIG_SENSORS_LTC4215)    += ltc4215.o
>>   obj-$(CONFIG_SENSORS_LTC4222)    += ltc4222.o
>> diff --git a/drivers/hwmon/ltc2990.c b/drivers/hwmon/ltc2990.c
>> new file mode 100644
>> index 0000000..161d995
>> --- /dev/null
>> +++ b/drivers/hwmon/ltc2990.c
>> @@ -0,0 +1,273 @@
>> +/*
>> + * driver for Linear Technology LTC2990 power monitor
>
> Driver
>
>> + *
>> + * Copyright (C) 2014 Topic Embedded Products
>
> 2015 ?

I wrote the driver early last year. I only did some cosmetic cleanup and 
rebase on master before submitting.

>
>> + * Author: Mike Looijmans <mike.looijmans@topic.nl>
>> + *
>> + * License: GPLv2
>> + *
>> + * This driver assumes the chip is wired as a dual current monitor, and
>> + * reports the voltage drop across two series resistors.
>
> It also monitors the temperature and VCC.
>
>> + */
>> +
>> +#include <linux/bug.h>
>
> Is this used anywhere in the driver ?

Not any more...

>
>> +#include <linux/delay.h>
>> +#include <linux/err.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/hwmon-sysfs.h>
>> +#include <linux/i2c.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/slab.h>
>> +
>> +#define LTC2990_STATUS    0x00
>> +#define LTC2990_CONTROL    0x01
>> +#define LTC2990_TRIGGER    0x02
>> +#define LTC2990_TINT_MSB    0x04
>> +#define LTC2990_TINT_LSB    0x05
>> +#define LTC2990_V1_MSB    0x06
>> +#define LTC2990_V1_LSB    0x07
>> +#define LTC2990_V2_MSB    0x08
>> +#define LTC2990_V2_LSB    0x09
>> +#define LTC2990_V3_MSB    0x0A
>> +#define LTC2990_V3_LSB    0x0B
>> +#define LTC2990_V4_MSB    0x0C
>> +#define LTC2990_V4_LSB    0x0D
>> +#define LTC2990_VCC_MSB    0x0E
>> +#define LTC2990_VCC_LSB    0x0F
>> +
>> +#define LTC2990_STATUS_BUSY    BIT(0)
>> +#define LTC2990_STATUS_TINT    BIT(1)
>> +#define LTC2990_STATUS_V1    BIT(2)
>> +#define LTC2990_STATUS_V2    BIT(3)
>> +#define LTC2990_STATUS_V3    BIT(4)
>> +#define LTC2990_STATUS_V4    BIT(5)
>> +#define LTC2990_STATUS_VCC    BIT(6)
>> +
>> +/* Only define control settings we actually use */
>> +#define LTC2990_CONTROL_KELVIN        BIT(7)
>> +#define LTC2990_CONTROL_SINGLE        BIT(6)
>> +#define LTC2990_CONTROL_MEASURE_ALL    (0x3 << 3)
>> +#define LTC2990_CONTROL_MODE_CURRENT    0x06
>> +#define LTC2990_CONTROL_MODE_VOLTAGE    0x07
>> +
>> +struct ltc2990_data {
>> +    struct device *hwmon_dev;
>> +    struct mutex update_lock;
>> +    unsigned long last_updated;
>> +    short values[6];
>
> u16 ?
>
>> +    bool valid;
>> +    u8 update_counter;
>
> Not used anywhere.
>
>> +};
>> +
>> +static int ltc2990_write(struct i2c_client *i2c, u8 reg, u8 value)
>> +{
>> +    return i2c_smbus_write_byte_data(i2c, reg, value);
>> +}
>> +
>> +static int ltc2990_read_byte(struct i2c_client *i2c, u8 reg)
>> +{
>> +    return i2c_smbus_read_byte_data(i2c, reg);
>> +}
>> +
>
> Useless shim functions.

True, should have removed them after refactoring them into one-liners.
They were convenient early on.

>
>> +static int ltc2990_read_word(struct i2c_client *i2c, u8 reg)
>> +{
>> +    int result = i2c_smbus_read_word_data(i2c, reg);
>> +    /* Result is MSB first, but smbus specs say LSB first, so swap the
>> +     * result */
>
> Bad multi-line comment.
>
>> +    return result < 0 ? result : swab16(result);
>
> Please use i2c_smbus_read_word_swapped() and drop the shim function.
>
>> +}
>> +
>> +static struct ltc2990_data *ltc2990_update_device(struct device *dev)
>> +{
>> +    struct i2c_client *i2c = to_i2c_client(dev);
>> +    struct ltc2990_data *data = i2c_get_clientdata(i2c);
>> +    struct ltc2990_data *ret = data;
>> +    unsigned int timeout;
>> +
>> +    mutex_lock(&data->update_lock);
>> +
>> +    /* Update about 4 times per second max */
>> +    if (time_after(jiffies, data->last_updated + HZ / 4) ||
>> !data->valid) {
>> +        int val;
>> +        int i;
>> +
>
> Please consider using continuous conversion. This would simplify the
> code significantly
> and reduce read delays.

It might increase power consumption though, as typically some user 
program would poll this every 10 seconds or so. I'll check the data sheet.

>> +        /* Trigger ADC, any value will do */
>> +        val = ltc2990_write(i2c, LTC2990_TRIGGER, 1);
>> +        if (unlikely(val < 0)) {
>> +            ret = ERR_PTR(val);
>> +            goto abort;
>> +        }
>> +
>> +        /* Wait for conversion complete */
>> +        timeout = 200;
>> +        for (;;) {
>> +            usleep_range(2000, 4000);
>> +            val = ltc2990_read_byte(i2c, LTC2990_STATUS);
>> +            if (unlikely(val < 0)) {
>> +                ret = ERR_PTR(val);
>> +                goto abort;
>> +            }
>> +            /* Single-shot mode, wait for conversion to complete */
>> +            if ((val & LTC2990_STATUS_BUSY) == 0)
>
>              if (!(...))
>
>> +                break;
>> +            if (--timeout == 0) {
>> +                ret = ERR_PTR(-ETIMEDOUT);
>> +                goto abort;
>> +            }
>> +        }
>
> Again, please consider using continuous conversion mode.
>
> If this is not feasible for some reason, you might as well just wait for
> the
> minimum conversion time before trying to read for the first time. If so,
> please use a fixed timeout by comparing the elapsed time instead of looping
> for a maximum number of times. Not even counting the time for executing the
> code, the maximum delay is between 400 ms and 800 ms, which is way too high
> (chip spec says 167 ms worst case, if three temperature sensors are
> configured).

Or maybe I should just sleep for 167ms and be done with it. Though I 
think I'l got with your minimal time first suggestion.

>> +
>> +        /* Read all registers */
>> +        for (i = 0; i < ARRAY_SIZE(data->values); ++i) {
>> +            val = ltc2990_read_word(i2c, (i<<1) + LTC2990_TINT_MSB);
>
> Missing spaces around <<
>
>> +            if (unlikely(val < 0)) {
>> +                dev_dbg(dev,
>> +                    "Failed to read ADC value: error %d\n",
>> +                    val);
>> +                ret = ERR_PTR(val);
>> +                goto abort;
>> +            }
>> +            data->values[i] = val & 0x7FFF; /* Strip 'new' bit */
>
> The bit is never evaluated, so you might as well store the raw value.
>
>> +        }
>> +        data->last_updated = jiffies;
>> +        data->valid = 1;
>
>      = true;
>
>> +
>> +        /*
>> +         *  Quirk: Second trigger is ignored? After this, the BUSY will
>> +         * still be set to "0" and no conversion performed.
>> +         */
>> +        val = ltc2990_write(i2c, LTC2990_TRIGGER, 0);
>> +    }
>> +abort:
>> +    mutex_unlock(&data->update_lock);
>> +    return ret;
>> +}
>> +
>> +/* Return the converted value from the given register in uV or mC */
>> +static int ltc2990_get_value(struct ltc2990_data *data, u8 index)
>> +{
>> +    s32 result;
>> +    s16 v;
>> +
>> +    if (index == 0) { /* internal temp, 0.0625 degrees/LSB, 12-bit  */
>> +        v = data->values[index] << 3;
>> +        result = (s32)v * 1000 >> 7;
>> +    } else if (index < 5) { /* Vx-Vy, 19.42uV/LSB, 14-bit */
>> +        v = data->values[index] << 2;
>
> Datasheet says that the sign bit is in bit 14, so this drops the sign bit.

I'll check the data sheet, would not have noticed this since current 
doesn't tend to run from the CPU back into the battery :)

>> +        result = (s32)v * 1942 / (4 * 100);
>> +    } else { /* Vcc, 305.18μV/LSB, 2.5V offset, 14-bit */
>> +        v = data->values[index] << 2;
>> +        result = (s32)v * 30518 / (4 * 100);
>> +        result += 2500000;
>> +    }
>> +    return result;
>> +}
>> +
>> +static ssize_t ltc2990_show_value(struct device *dev,
>> +                  struct device_attribute *da, char *buf)
>> +{
>> +    struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
>> +    struct ltc2990_data *data = ltc2990_update_device(dev);
>> +    int value;
>> +
>> +    if (IS_ERR(data))
>> +        return PTR_ERR(data);
>> +
>> +    value = ltc2990_get_value(data, attr->index);
>> +    return snprintf(buf, PAGE_SIZE, "%d\n", value);
>> +}
>> +
>> +static SENSOR_DEVICE_ATTR(temp_int, S_IRUGO, ltc2990_show_value,
>> NULL, 0);
>> +static SENSOR_DEVICE_ATTR(v1v2_diff, S_IRUGO, ltc2990_show_value,
>> NULL, 1);
>> +static SENSOR_DEVICE_ATTR(v3v4_diff, S_IRUGO, ltc2990_show_value,
>> NULL, 3);
>> +static SENSOR_DEVICE_ATTR(vcc, S_IRUGO, ltc2990_show_value, NULL, 5);
>> +
>
> Please use standard attribute names (and units) as per
> Documentation/hwmon/sysfs-interface.
>
>> +static struct attribute *ltc2990_attributes[] = {
>> +    &sensor_dev_attr_temp_int.dev_attr.attr,
>> +    &sensor_dev_attr_v1v2_diff.dev_attr.attr,
>> +    &sensor_dev_attr_v3v4_diff.dev_attr.attr,
>> +    &sensor_dev_attr_vcc.dev_attr.attr,
>> +    NULL,
>> +};
>> +
>> +static const struct attribute_group ltc2990_group = {
>> +    .attrs = ltc2990_attributes,
>> +};
>> +
>
> Please use the ATTRIBUTE_GROUPS() macro. Also see below.

Okay, will RTFM.

>
>> +static int ltc2990_i2c_probe(
>> +    struct i2c_client *i2c, const struct i2c_device_id *id)
>
> Please split as
>
> static int ltc2990_i2c_probe(struct i2c_client *i2c,
>                   const struct i2c_device_id *id)
>
>> +{
>> +    int ret;
>> +    struct ltc2990_data *ltc2990;
>> +
>> +    if (!i2c_check_functionality(i2c->adapter,
>> I2C_FUNC_SMBUS_BYTE_DATA))
>> +        return -ENODEV;
>> +
>> +    ltc2990 = devm_kzalloc(&i2c->dev,
>> +        sizeof(struct ltc2990_data), GFP_KERNEL);
>
> Please align continuation lines with '('.
>
>> +    if (ltc2990 == NULL)
>> +        return -ENOMEM;
>> +
>> +    ret = ltc2990_read_byte(i2c, 0);
>> +    if (ret < 0) {
>> +        dev_err(&i2c->dev, "Could not read LTC2990 on i2c bus.\n");
>> +        return ret;
>> +    }
>
> The write below would also return an error if the chip isn't there,
> so this additional read does not provide any real value.
>
>> +    ret = ltc2990_write(i2c, LTC2990_CONTROL,
>> +        LTC2990_CONTROL_SINGLE | LTC2990_CONTROL_MEASURE_ALL |
>> +        LTC2990_CONTROL_MODE_CURRENT);
>
> I'll have to think about this. While it addresses your use case,
> it limits the scope of this driver significantly. Looking into the board
> specifications, you might as well do it right and determine (and set)
> the correct configuration using devicetree data instead of forcing your
> use case on everyone.
>
> Sure, you may argue that you don't care, but we will be the ones who
> will have
> to handle error reports that the driver unexpectedly changes the
> configuration
> in other use cases (where, for example, the mode may have been pre-set by
> the BIOS or ROMMON).

The reason I didn't submit the driver last year was that I was still 
thinking about it. On the other hand, a limited driver is still much 
better than no driver at all, so that made me post it, with the added 
remarks about the driver being limited.

Adding the other functions it quite a bit more work than just writing 
this register. It also changes the properties to be exported to sysfs, 
and the calculation of the end values.

>
>> +    if (ret < 0) {
>> +        dev_err(&i2c->dev, "Error: Failed to set control mode.\n");
>> +        return ret;
>> +    }
>> +
>> +    mutex_init(&ltc2990->update_lock);
>> +    i2c_set_clientdata(i2c, ltc2990);
>> +
>> +    /* Register sysfs hooks */
>> +    ret = sysfs_create_group(&i2c->dev.kobj, &ltc2990_group);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ltc2990->hwmon_dev = hwmon_device_register(&i2c->dev);
>
> Please use devm_hwmon_device_register_with_groups().
>
>> +    if (IS_ERR(ltc2990->hwmon_dev)) {
>> +        ret = PTR_ERR(ltc2990->hwmon_dev);
>> +        goto out_hwmon_device_register;
>> +    }
>> +
>> +    return 0;
>> +
>> +out_hwmon_device_register:
>> +    sysfs_remove_group(&i2c->dev.kobj, &ltc2990_group);
>> +    return ret;
>> +}
>> +
>> +static int ltc2990_i2c_remove(struct i2c_client *i2c)
>> +{
>> +    struct ltc2990_data *ltc2990 = i2c_get_clientdata(i2c);
>> +
>> +    hwmon_device_unregister(ltc2990->hwmon_dev);
>> +    sysfs_remove_group(&i2c->dev.kobj, &ltc2990_group);
>> +    return 0;
>> +}
>> +
>> +static const struct i2c_device_id ltc2990_i2c_id[] = {
>> +    { "ltc2990", 0 },
>> +    {}
>> +};
>> +MODULE_DEVICE_TABLE(i2c, ltc2990_i2c_id);
>> +
>> +static struct i2c_driver ltc2990_i2c_driver = {
>> +    .driver = {
>> +        .name = "ltc2990",
>> +    },
>> +    .probe    = ltc2990_i2c_probe,
>> +    .remove   = ltc2990_i2c_remove,
>> +    .id_table = ltc2990_i2c_id,
>> +};
>> +
>> +module_i2c_driver(ltc2990_i2c_driver);
>> +
>> +MODULE_DESCRIPTION("LTC2990 Sensor Driver");
>> +MODULE_AUTHOR("Topic Embedded Products");
>> +MODULE_LICENSE("GPL v2");
>>
>


-- 
Mike Looijmans

Re: [PATCH] hwmon: Add LTC2990 sensor driver

[Guenter Roeck]

On 01/07/2016 10:59 AM, Mike Looijmans wrote:
> Thank you very much for your review comments, I'll update the driver and post a v2 patch.
>
> Inlined some replies below. Assume that I "will do" for all comments I didn't comment on inline...
>
> On 06-01-16 16:22, Guenter Roeck wrote:
>> Hello Mike,
>>
>> On 01/06/2016 12:07 AM, Mike Looijmans wrote:
>>> This adds support for the Linear Technology LTC2990  I2C System Monitor.
>>
>> s/  / /
>>
>>> The LTC2990 supports a combination of voltage, current and temperature
>>> monitoring, but this driver currently only supports reading two currents
>>> by measuring two differential voltages across series resistors.
>>>
>> Plus VCC, plus the internal temperature.
>
> Yeah, I should give myself more credit :) I'll add that in Kconfig too.
>
>>> This is sufficient to support the Topic Miami SOM which uses this chip
>>> to monitor the currents flowing into the FPGA and the CPU parts.
>>>
>>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
>>> ---
>>>   drivers/hwmon/Kconfig   |  15 +++
>>>   drivers/hwmon/Makefile  |   1 +
>>>   drivers/hwmon/ltc2990.c | 273
>>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>

[ ... ]

>>> +}
>>> +
>>> +static struct ltc2990_data *ltc2990_update_device(struct device *dev)
>>> +{
>>> +    struct i2c_client *i2c = to_i2c_client(dev);
>>> +    struct ltc2990_data *data = i2c_get_clientdata(i2c);
>>> +    struct ltc2990_data *ret = data;
>>> +    unsigned int timeout;
>>> +
>>> +    mutex_lock(&data->update_lock);
>>> +
>>> +    /* Update about 4 times per second max */
>>> +    if (time_after(jiffies, data->last_updated + HZ / 4) ||
>>> !data->valid) {
>>> +        int val;
>>> +        int i;
>>> +
>>
>> Please consider using continuous conversion. This would simplify the
>> code significantly
>> and reduce read delays.
>
> It might increase power consumption though, as typically some user program would poll this every 10 seconds or so. I'll check the data sheet.
>

I suspect that the power savings will be less than the added power
consumed by the CPU due to the more complex code.

Really, unless you have an application where a few mW power savings
are essential and warrant the additional code complexity, this is
the wrong approach.

>>> +        /* Trigger ADC, any value will do */
>>> +        val = ltc2990_write(i2c, LTC2990_TRIGGER, 1);
>>> +        if (unlikely(val < 0)) {
>>> +            ret = ERR_PTR(val);
>>> +            goto abort;
>>> +        }
>>> +
>>> +        /* Wait for conversion complete */
>>> +        timeout = 200;
>>> +        for (;;) {
>>> +            usleep_range(2000, 4000);
>>> +            val = ltc2990_read_byte(i2c, LTC2990_STATUS);
>>> +            if (unlikely(val < 0)) {
>>> +                ret = ERR_PTR(val);
>>> +                goto abort;
>>> +            }
>>> +            /* Single-shot mode, wait for conversion to complete */
>>> +            if ((val & LTC2990_STATUS_BUSY) == 0)
>>
>>              if (!(...))
>>
>>> +                break;
>>> +            if (--timeout == 0) {
>>> +                ret = ERR_PTR(-ETIMEDOUT);
>>> +                goto abort;
>>> +            }
>>> +        }
>>
>> Again, please consider using continuous conversion mode.
>>
>> If this is not feasible for some reason, you might as well just wait for
>> the
>> minimum conversion time before trying to read for the first time. If so,
>> please use a fixed timeout by comparing the elapsed time instead of looping
>> for a maximum number of times. Not even counting the time for executing the
>> code, the maximum delay is between 400 ms and 800 ms, which is way too high
>> (chip spec says 167 ms worst case, if three temperature sensors are
>> configured).
>
> Or maybe I should just sleep for 167ms and be done with it. Though I think I'l got with your minimal time first suggestion.
>

Keep in mind that any user space program using this driver would effectively
go to sleep for the wait time. Maybe that doesn't matter in your application,
but it may be a problem for others. A fixed large delay would make the situation
even worse.

Guenter

[PATCH v2] hwmon: Add LTC2990 sensor driver

[Mike Looijmans]

This adds support for the Linear Technology LTC2990  I2C System Monitor.
The LTC2990 supports a combination of voltage, current and temperature
monitoring. This driver currently only supports reading two currents
by measuring two differential voltages across series resistors, in
addition to the Vcc supply voltage and internal temperature.

This is sufficient to support the Topic Miami SOM which uses this chip
to monitor the currents flowing into the FPGA and the CPU parts.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
---
v2: Processed all review comments.
    Put chip into continuous measurement mode.
    Added ducumentation.
    Use standard hwmon interfaces and macros.

 Documentation/hwmon/ltc2990 |  44 ++++++++++++
 drivers/hwmon/Kconfig       |  14 ++++
 drivers/hwmon/Makefile      |   1 +
 drivers/hwmon/ltc2990.c     | 160 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 219 insertions(+)
 create mode 100644 Documentation/hwmon/ltc2990
 create mode 100644 drivers/hwmon/ltc2990.c

diff --git a/Documentation/hwmon/ltc2990 b/Documentation/hwmon/ltc2990
new file mode 100644
index 0000000..838b74e
--- /dev/null
+++ b/Documentation/hwmon/ltc2990
@@ -0,0 +1,44 @@
+Kernel driver ltc2990
+=====================
+
+Supported chips:
+  * Linear Technology LTC2990
+    Prefix: 'ltc2990'
+    Addresses scanned: -
+    Datasheet: http://www.linear.com/product/ltc2990
+
+Author: Mike Looijmans <mike.looijmans@topic.nl>
+
+
+Description
+-----------
+
+LTC2990 is a Quad I2C Voltage, Current and Temperature Monitor.
+The chip's inputs can measure 4 voltages, or two inputs together (1+2 and 3+4)
+can be combined to measure a differential voltage, which is typically used to
+measure current through a series resistor, or a temperature.
+
+This driver currently uses the 2x differential mode only. In order to support
+other modes, the driver will need to be expanded.
+
+
+Usage Notes
+-----------
+
+This driver does not probe for PMBus devices. You will have to instantiate
+devices explicitly.
+
+
+Sysfs attributes
+----------------
+
+The "curr*_input" measurements actually report the voltage drop across the
+input pins in microvolts. This is equivalent to the current through a 1mOhm
+sense resistor. Divide the reported value by the actual sense resistor value
+in mOhm to get the actual value.
+
+in0_input     Voltage at Vcc pin in millivolt (range 2.5V to 5V)
+temp1_input   Internal chip temperature in millidegrees Celcius
+curr1_input   Current in mA across v1-v2 assuming a 1mOhm sense resistor.
+curr2_input   Current in mA across v3-v4 assuming a 1mOhm sense resistor.
+
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 80a73bf..8a31d64 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -685,6 +685,20 @@ config SENSORS_LTC2945
 	  This driver can also be built as a module. If so, the module will
 	  be called ltc2945.
 
+config SENSORS_LTC2990
+	tristate "Linear Technology LTC2990 (current monitoring mode only)"
+	depends on I2C
+	help
+	  If you say yes here you get support for Linear Technology LTC2990
+	  I2C System Monitor. The LTC2990 supports a combination of voltage,
+	  current and temperature monitoring, but in addition to the Vcc supply
+	  voltage and chip temperature, this driver currently only supports
+	  reading two currents by measuring two differential voltages across
+	  series resistors.
+
+	  This driver can also be built as a module. If so, the module will
+	  be called ltc2990.
+
 config SENSORS_LTC4151
 	tristate "Linear Technology LTC4151"
 	depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 12a3239..e4bd15b 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -101,6 +101,7 @@ obj-$(CONFIG_SENSORS_LM95234)	+= lm95234.o
 obj-$(CONFIG_SENSORS_LM95241)	+= lm95241.o
 obj-$(CONFIG_SENSORS_LM95245)	+= lm95245.o
 obj-$(CONFIG_SENSORS_LTC2945)	+= ltc2945.o
+obj-$(CONFIG_SENSORS_LTC2990)	+= ltc2990.o
 obj-$(CONFIG_SENSORS_LTC4151)	+= ltc4151.o
 obj-$(CONFIG_SENSORS_LTC4215)	+= ltc4215.o
 obj-$(CONFIG_SENSORS_LTC4222)	+= ltc4222.o
diff --git a/drivers/hwmon/ltc2990.c b/drivers/hwmon/ltc2990.c
new file mode 100644
index 0000000..3720ff7
--- /dev/null
+++ b/drivers/hwmon/ltc2990.c
@@ -0,0 +1,160 @@
+/*
+ * Driver for Linear Technology LTC2990 power monitor
+ *
+ * Copyright (C) 2014 Topic Embedded Products
+ * Author: Mike Looijmans <mike.looijmans@topic.nl>
+ *
+ * License: GPLv2
+ *
+ * This driver assumes the chip is wired as a dual current monitor, and
+ * reports the voltage drop across two series resistors. It also reports
+ * the chip's internal temperature and Vcc power supply voltage.
+ */
+
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+
+#define LTC2990_STATUS	0x00
+#define LTC2990_CONTROL	0x01
+#define LTC2990_TRIGGER	0x02
+#define LTC2990_TINT_MSB	0x04
+#define LTC2990_TINT_LSB	0x05
+#define LTC2990_V1_MSB	0x06
+#define LTC2990_V1_LSB	0x07
+#define LTC2990_V2_MSB	0x08
+#define LTC2990_V2_LSB	0x09
+#define LTC2990_V3_MSB	0x0A
+#define LTC2990_V3_LSB	0x0B
+#define LTC2990_V4_MSB	0x0C
+#define LTC2990_V4_LSB	0x0D
+#define LTC2990_VCC_MSB	0x0E
+#define LTC2990_VCC_LSB	0x0F
+
+#define LTC2990_STATUS_BUSY	BIT(0)
+#define LTC2990_STATUS_TINT	BIT(1)
+#define LTC2990_STATUS_V1	BIT(2)
+#define LTC2990_STATUS_V2	BIT(3)
+#define LTC2990_STATUS_V3	BIT(4)
+#define LTC2990_STATUS_V4	BIT(5)
+#define LTC2990_STATUS_VCC	BIT(6)
+
+/* Only define control settings we actually use */
+#define LTC2990_CONTROL_KELVIN		BIT(7)
+#define LTC2990_CONTROL_SINGLE		BIT(6)
+#define LTC2990_CONTROL_MEASURE_ALL	(0x3 << 3)
+#define LTC2990_CONTROL_MODE_CURRENT	0x06
+#define LTC2990_CONTROL_MODE_VOLTAGE	0x07
+
+/* convert raw register value to sign-extended integer in 16-bit range */
+static int ltc2990_voltage_to_int(int raw)
+{
+	if (raw & BIT(14)) {
+		return -(0x4000 - (raw & 0x3FFF)) << 2;
+	} else {
+		return (raw & 0x3FFF) << 2;
+	}
+}
+
+/* Return the converted value from the given register in uV or mC */
+static int ltc2990_get_value(struct i2c_client *i2c, u8 index)
+{
+	int val;
+	int result;
+
+	val = i2c_smbus_read_word_swapped(i2c, (index << 1) + LTC2990_TINT_MSB);
+	if (unlikely(val < 0))
+		return val;
+
+	if (index == 0) { /* internal temp, 0.0625 degrees/LSB, 13-bit  */
+		val = (val & 0x1FFF) << 3;
+		result = (val * 1000) >> 7;
+	} else if (index < 5) { /* Vx-Vy, 19.42uV/LSB */
+		result = ltc2990_voltage_to_int(val) * 1942 / (4 * 100);
+	} else { /* Vcc, 305.18μV/LSB, 2.5V offset */
+		result = ltc2990_voltage_to_int(val) * 30518 / (4 * 100 * 1000);
+		result += 2500;
+	}
+
+	return result;
+}
+
+static ssize_t ltc2990_show_value(struct device *dev,
+				  struct device_attribute *da, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
+	int value;
+
+	value = ltc2990_get_value(dev_get_drvdata(dev), attr->index);
+	return snprintf(buf, PAGE_SIZE, "%d\n", value);
+}
+
+static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, ltc2990_show_value, NULL, 0);
+static SENSOR_DEVICE_ATTR(curr1_input, S_IRUGO, ltc2990_show_value, NULL, 1);
+static SENSOR_DEVICE_ATTR(curr2_input, S_IRUGO, ltc2990_show_value, NULL, 3);
+static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ltc2990_show_value, NULL, 5);
+
+static struct attribute *ltc2990_attrs[] = {
+	&sensor_dev_attr_temp1_input.dev_attr.attr,
+	&sensor_dev_attr_curr1_input.dev_attr.attr,
+	&sensor_dev_attr_curr2_input.dev_attr.attr,
+	&sensor_dev_attr_in0_input.dev_attr.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(ltc2990);
+
+static int ltc2990_i2c_probe(struct i2c_client *i2c,
+	const struct i2c_device_id *id)
+{
+	int ret;
+	struct device *hwmon_dev;
+
+	if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
+		return -ENODEV;
+
+	/* Setup continuous mode, current monitor */
+	ret = i2c_smbus_write_byte_data(i2c, LTC2990_CONTROL,
+		LTC2990_CONTROL_MEASURE_ALL | LTC2990_CONTROL_MODE_CURRENT);
+	if (ret < 0) {
+		dev_err(&i2c->dev, "Error: Failed to set control mode.\n");
+		return ret;
+	}
+	/* Trigger once to start continuous conversion */
+	ret = i2c_smbus_write_byte_data(i2c, LTC2990_TRIGGER, 1);
+	if (ret < 0) {
+		dev_err(&i2c->dev, "Error: Failed to start aquisition.\n");
+		return ret;
+	}
+
+	hwmon_dev = devm_hwmon_device_register_with_groups(&i2c->dev,
+							   i2c->name,
+							   i2c,
+							   ltc2990_groups);
+
+	return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static const struct i2c_device_id ltc2990_i2c_id[] = {
+	{ "ltc2990", 0 },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, ltc2990_i2c_id);
+
+static struct i2c_driver ltc2990_i2c_driver = {
+	.driver = {
+		.name = "ltc2990",
+	},
+	.probe    = ltc2990_i2c_probe,
+	.id_table = ltc2990_i2c_id,
+};
+
+module_i2c_driver(ltc2990_i2c_driver);
+
+MODULE_DESCRIPTION("LTC2990 Sensor Driver");
+MODULE_AUTHOR("Topic Embedded Products");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

Re: [PATCH] hwmon: Add LTC2990 sensor driver

[Mike Looijmans]

On 08-01-16 16:09, Guenter Roeck wrote:
> On 01/07/2016 10:59 AM, Mike Looijmans wrote:
>> Thank you very much for your review comments, I'll update the driver and
>> post a v2 patch.
>>
>> Inlined some replies below. Assume that I "will do" for all comments I
>> didn't comment on inline...
>>
>> On 06-01-16 16:22, Guenter Roeck wrote:
>>> Hello Mike,
>>>
>>> On 01/06/2016 12:07 AM, Mike Looijmans wrote:
>>>> This adds support for the Linear Technology LTC2990  I2C System Monitor.
>>>
>>> s/  / /
>>>
>>>> The LTC2990 supports a combination of voltage, current and temperature
>>>> monitoring, but this driver currently only supports reading two currents
>>>> by measuring two differential voltages across series resistors.
>>>>
>>> Plus VCC, plus the internal temperature.
>>
>> Yeah, I should give myself more credit :) I'll add that in Kconfig too.
>>
>>>> This is sufficient to support the Topic Miami SOM which uses this chip
>>>> to monitor the currents flowing into the FPGA and the CPU parts.
>>>>
>>>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
>>>> ---
>>>>   drivers/hwmon/Kconfig   |  15 +++
>>>>   drivers/hwmon/Makefile  |   1 +
>>>>   drivers/hwmon/ltc2990.c | 273
>>>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>>
>
> [ ... ]
>
>>>> +}
>>>> +
>>>> +static struct ltc2990_data *ltc2990_update_device(struct device *dev)
>>>> +{
>>>> +    struct i2c_client *i2c = to_i2c_client(dev);
>>>> +    struct ltc2990_data *data = i2c_get_clientdata(i2c);
>>>> +    struct ltc2990_data *ret = data;
>>>> +    unsigned int timeout;
>>>> +
>>>> +    mutex_lock(&data->update_lock);
>>>> +
>>>> +    /* Update about 4 times per second max */
>>>> +    if (time_after(jiffies, data->last_updated + HZ / 4) ||
>>>> !data->valid) {
>>>> +        int val;
>>>> +        int i;
>>>> +
>>>
>>> Please consider using continuous conversion. This would simplify the
>>> code significantly
>>> and reduce read delays.
>>
>> It might increase power consumption though, as typically some user program
>> would poll this every 10 seconds or so. I'll check the data sheet.
>>
>
> I suspect that the power savings will be less than the added power
> consumed by the CPU due to the more complex code.
>
> Really, unless you have an application where a few mW power savings
> are essential and warrant the additional code complexity, this is
> the wrong approach.

Yeah, you're right, checked the datasheet and it reports a 1mA typical power 
usage when converting. Not worth the trouble.

Continuous conversion made the driver a LOT simpler, could even completely 
drop the private data structure.

I sent v2 already, but just noticed that some of the #includes were no longer
needed, so I'll send a v3 to fix that.

...


Kind regards,

Mike Looijmans
System Expert

TOPIC Embedded Products
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
E-mail: mike.looijmans@topicproducts.com
Website: www.topicproducts.com

Please consider the environment before printing this e-mail

Re: [PATCH v2] hwmon: Add LTC2990 sensor driver

[Guenter Roeck]

On 01/13/2016 03:05 AM, Mike Looijmans wrote:
> This adds support for the Linear Technology LTC2990  I2C System Monitor.
> The LTC2990 supports a combination of voltage, current and temperature
> monitoring. This driver currently only supports reading two currents
> by measuring two differential voltages across series resistors, in
> addition to the Vcc supply voltage and internal temperature.
>
> This is sufficient to support the Topic Miami SOM which uses this chip
> to monitor the currents flowing into the FPGA and the CPU parts.
>
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>

Mike,

That looks much better. Can you send me the output of i2cdump for the chip ?
That would help me writing module test code for it.

Thanks,
Guenter

> ---
> v2: Processed all review comments.
>      Put chip into continuous measurement mode.
>      Added ducumentation.
>      Use standard hwmon interfaces and macros.
>
>   Documentation/hwmon/ltc2990 |  44 ++++++++++++
>   drivers/hwmon/Kconfig       |  14 ++++
>   drivers/hwmon/Makefile      |   1 +
>   drivers/hwmon/ltc2990.c     | 160 ++++++++++++++++++++++++++++++++++++++++++++
>   4 files changed, 219 insertions(+)
>   create mode 100644 Documentation/hwmon/ltc2990
>   create mode 100644 drivers/hwmon/ltc2990.c
>
> diff --git a/Documentation/hwmon/ltc2990 b/Documentation/hwmon/ltc2990
> new file mode 100644
> index 0000000..838b74e
> --- /dev/null
> +++ b/Documentation/hwmon/ltc2990
> @@ -0,0 +1,44 @@
> +Kernel driver ltc2990
> +=====================
> +
> +Supported chips:
> +  * Linear Technology LTC2990
> +    Prefix: 'ltc2990'
> +    Addresses scanned: -
> +    Datasheet: http://www.linear.com/product/ltc2990
> +
> +Author: Mike Looijmans <mike.looijmans@topic.nl>
> +
> +
> +Description
> +-----------
> +
> +LTC2990 is a Quad I2C Voltage, Current and Temperature Monitor.
> +The chip's inputs can measure 4 voltages, or two inputs together (1+2 and 3+4)
> +can be combined to measure a differential voltage, which is typically used to
> +measure current through a series resistor, or a temperature.
> +
> +This driver currently uses the 2x differential mode only. In order to support
> +other modes, the driver will need to be expanded.
> +
> +
> +Usage Notes
> +-----------
> +
> +This driver does not probe for PMBus devices. You will have to instantiate
> +devices explicitly.
> +
> +
> +Sysfs attributes
> +----------------
> +
> +The "curr*_input" measurements actually report the voltage drop across the
> +input pins in microvolts. This is equivalent to the current through a 1mOhm
> +sense resistor. Divide the reported value by the actual sense resistor value
> +in mOhm to get the actual value.
> +
> +in0_input     Voltage at Vcc pin in millivolt (range 2.5V to 5V)
> +temp1_input   Internal chip temperature in millidegrees Celcius
> +curr1_input   Current in mA across v1-v2 assuming a 1mOhm sense resistor.
> +curr2_input   Current in mA across v3-v4 assuming a 1mOhm sense resistor.
> +
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 80a73bf..8a31d64 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -685,6 +685,20 @@ config SENSORS_LTC2945
>   	  This driver can also be built as a module. If so, the module will
>   	  be called ltc2945.
>
> +config SENSORS_LTC2990
> +	tristate "Linear Technology LTC2990 (current monitoring mode only)"
> +	depends on I2C
> +	help
> +	  If you say yes here you get support for Linear Technology LTC2990
> +	  I2C System Monitor. The LTC2990 supports a combination of voltage,
> +	  current and temperature monitoring, but in addition to the Vcc supply
> +	  voltage and chip temperature, this driver currently only supports
> +	  reading two currents by measuring two differential voltages across
> +	  series resistors.
> +
> +	  This driver can also be built as a module. If so, the module will
> +	  be called ltc2990.
> +
>   config SENSORS_LTC4151
>   	tristate "Linear Technology LTC4151"
>   	depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 12a3239..e4bd15b 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -101,6 +101,7 @@ obj-$(CONFIG_SENSORS_LM95234)	+= lm95234.o
>   obj-$(CONFIG_SENSORS_LM95241)	+= lm95241.o
>   obj-$(CONFIG_SENSORS_LM95245)	+= lm95245.o
>   obj-$(CONFIG_SENSORS_LTC2945)	+= ltc2945.o
> +obj-$(CONFIG_SENSORS_LTC2990)	+= ltc2990.o
>   obj-$(CONFIG_SENSORS_LTC4151)	+= ltc4151.o
>   obj-$(CONFIG_SENSORS_LTC4215)	+= ltc4215.o
>   obj-$(CONFIG_SENSORS_LTC4222)	+= ltc4222.o
> diff --git a/drivers/hwmon/ltc2990.c b/drivers/hwmon/ltc2990.c
> new file mode 100644
> index 0000000..3720ff7
> --- /dev/null
> +++ b/drivers/hwmon/ltc2990.c
> @@ -0,0 +1,160 @@
> +/*
> + * Driver for Linear Technology LTC2990 power monitor
> + *
> + * Copyright (C) 2014 Topic Embedded Products
> + * Author: Mike Looijmans <mike.looijmans@topic.nl>
> + *
> + * License: GPLv2
> + *
> + * This driver assumes the chip is wired as a dual current monitor, and
> + * reports the voltage drop across two series resistors. It also reports
> + * the chip's internal temperature and Vcc power supply voltage.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +
> +#define LTC2990_STATUS	0x00
> +#define LTC2990_CONTROL	0x01
> +#define LTC2990_TRIGGER	0x02
> +#define LTC2990_TINT_MSB	0x04
> +#define LTC2990_TINT_LSB	0x05
> +#define LTC2990_V1_MSB	0x06
> +#define LTC2990_V1_LSB	0x07
> +#define LTC2990_V2_MSB	0x08
> +#define LTC2990_V2_LSB	0x09
> +#define LTC2990_V3_MSB	0x0A
> +#define LTC2990_V3_LSB	0x0B
> +#define LTC2990_V4_MSB	0x0C
> +#define LTC2990_V4_LSB	0x0D
> +#define LTC2990_VCC_MSB	0x0E
> +#define LTC2990_VCC_LSB	0x0F
> +

LSB not used.

> +#define LTC2990_STATUS_BUSY	BIT(0)
> +#define LTC2990_STATUS_TINT	BIT(1)
> +#define LTC2990_STATUS_V1	BIT(2)
> +#define LTC2990_STATUS_V2	BIT(3)
> +#define LTC2990_STATUS_V3	BIT(4)
> +#define LTC2990_STATUS_V4	BIT(5)
> +#define LTC2990_STATUS_VCC	BIT(6)
> +
No longer used ?

> +/* Only define control settings we actually use */

Hmmm ... but not all are used.

> +#define LTC2990_CONTROL_KELVIN		BIT(7)
> +#define LTC2990_CONTROL_SINGLE		BIT(6)
> +#define LTC2990_CONTROL_MEASURE_ALL	(0x3 << 3)
> +#define LTC2990_CONTROL_MODE_CURRENT	0x06
> +#define LTC2990_CONTROL_MODE_VOLTAGE	0x07
> +
> +/* convert raw register value to sign-extended integer in 16-bit range */
> +static int ltc2990_voltage_to_int(int raw)
> +{
> +	if (raw & BIT(14)) {
> +		return -(0x4000 - (raw & 0x3FFF)) << 2;
> +	} else {
> +		return (raw & 0x3FFF) << 2;
> +	}
> +}
> +
> +/* Return the converted value from the given register in uV or mC */
> +static int ltc2990_get_value(struct i2c_client *i2c, u8 index)
> +{
> +	int val;
> +	int result;
> +
> +	val = i2c_smbus_read_word_swapped(i2c, (index << 1) + LTC2990_TINT_MSB);
> +	if (unlikely(val < 0))
> +		return val;
> +
> +	if (index == 0) { /* internal temp, 0.0625 degrees/LSB, 13-bit  */
> +		val = (val & 0x1FFF) << 3;
> +		result = (val * 1000) >> 7;
> +	} else if (index < 5) { /* Vx-Vy, 19.42uV/LSB */
> +		result = ltc2990_voltage_to_int(val) * 1942 / (4 * 100);
> +	} else { /* Vcc, 305.18μV/LSB, 2.5V offset */
> +		result = ltc2990_voltage_to_int(val) * 30518 / (4 * 100 * 1000);
> +		result += 2500;
> +	}
> +
With the register in index (see below) this could be

	val = i2c_smbus_read_word_swapped(i2c, index);

	switch (index) {
	case LTC2990_TINT_MSB:
		val = (val & 0x1FFF) << 3;
		result = (val * 1000) >> 7;
		break;
	case LTC2990_V1_MSB:
	case LTC2990_V2_MSB:
		...
		result = ltc2990_voltage_to_int(val) * 1942 / (4 * 100);
		break;
	case LTC2990_VCC_MSB:
		result = ltc2990_voltage_to_int(val) * 30518 / (4 * 100 * 1000);
		result += 2500;
		break;
	default:
		result = 0;	/* won't happen, makes compiler happy */
		break;
	}

which I think would be easier to understand.
	
> +	return result;
> +}
> +
> +static ssize_t ltc2990_show_value(struct device *dev,
> +				  struct device_attribute *da, char *buf)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> +	int value;
> +
> +	value = ltc2990_get_value(dev_get_drvdata(dev), attr->index);
> +	return snprintf(buf, PAGE_SIZE, "%d\n", value);
> +}
> +
> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, ltc2990_show_value, NULL, 0);
> +static SENSOR_DEVICE_ATTR(curr1_input, S_IRUGO, ltc2990_show_value, NULL, 1);
> +static SENSOR_DEVICE_ATTR(curr2_input, S_IRUGO, ltc2990_show_value, NULL, 3);
> +static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ltc2990_show_value, NULL, 5);

Consider providing the register MSB in index.

Example:
	static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, ltc2990_show_value, NULL, LTC2990_TINT_MSB);




> +
> +static struct attribute *ltc2990_attrs[] = {
> +	&sensor_dev_attr_temp1_input.dev_attr.attr,
> +	&sensor_dev_attr_curr1_input.dev_attr.attr,
> +	&sensor_dev_attr_curr2_input.dev_attr.attr,
> +	&sensor_dev_attr_in0_input.dev_attr.attr,
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(ltc2990);
> +
> +static int ltc2990_i2c_probe(struct i2c_client *i2c,
> +	const struct i2c_device_id *id)

Please align continuation lines with '('.

> +{
> +	int ret;
> +	struct device *hwmon_dev;
> +
> +	if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_SMBUS_BYTE_DATA))

Also need capability to read words.

> +		return -ENODEV;
> +
> +	/* Setup continuous mode, current monitor */
> +	ret = i2c_smbus_write_byte_data(i2c, LTC2990_CONTROL,
> +		LTC2990_CONTROL_MEASURE_ALL | LTC2990_CONTROL_MODE_CURRENT);
> +	if (ret < 0) {
> +		dev_err(&i2c->dev, "Error: Failed to set control mode.\n");
> +		return ret;
> +	}
> +	/* Trigger once to start continuous conversion */
> +	ret = i2c_smbus_write_byte_data(i2c, LTC2990_TRIGGER, 1);
> +	if (ret < 0) {
> +		dev_err(&i2c->dev, "Error: Failed to start aquisition.\n");

s/aquisition/acquisition/

> +		return ret;
> +	}
> +
> +	hwmon_dev = devm_hwmon_device_register_with_groups(&i2c->dev,
> +							   i2c->name,
> +							   i2c,
> +							   ltc2990_groups);
> +
> +	return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static const struct i2c_device_id ltc2990_i2c_id[] = {
> +	{ "ltc2990", 0 },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, ltc2990_i2c_id);
> +
> +static struct i2c_driver ltc2990_i2c_driver = {
> +	.driver = {
> +		.name = "ltc2990",
> +	},
> +	.probe    = ltc2990_i2c_probe,
> +	.id_table = ltc2990_i2c_id,
> +};
> +
> +module_i2c_driver(ltc2990_i2c_driver);
> +
> +MODULE_DESCRIPTION("LTC2990 Sensor Driver");
> +MODULE_AUTHOR("Topic Embedded Products");
> +MODULE_LICENSE("GPL v2");
>

Re: [PATCH v2] hwmon: Add LTC2990 sensor driver

[Mike Looijmans]

On 13-01-16 14:24, Guenter Roeck wrote:
> On 01/13/2016 03:05 AM, Mike Looijmans wrote:
>> This adds support for the Linear Technology LTC2990  I2C System Monitor.
>> The LTC2990 supports a combination of voltage, current and temperature
>> monitoring. This driver currently only supports reading two currents
>> by measuring two differential voltages across series resistors, in
>> addition to the Vcc supply voltage and internal temperature.
>>
>> This is sufficient to support the Topic Miami SOM which uses this chip
>> to monitor the currents flowing into the FPGA and the CPU parts.
>>
>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
>
> Mike,
>
> That looks much better. Can you send me the output of i2cdump for the chip ?
> That would help me writing module test code for it.

I'm kinda interested into how that would work.

I'll have to remove the driver first to get i2cdump to work on the chip. I 
cannot force a device removal from user space while running, can I?
And i suppose you want a dump of a chip in running status? (On boot, all are 
registers are simply set to zero)


> Thanks,
> Guenter
>
>> ---
>> v2: Processed all review comments.
>>      Put chip into continuous measurement mode.
>>      Added ducumentation.
>>      Use standard hwmon interfaces and macros.
>>
>>   Documentation/hwmon/ltc2990 |  44 ++++++++++++
>>   drivers/hwmon/Kconfig       |  14 ++++
>>   drivers/hwmon/Makefile      |   1 +
>>   drivers/hwmon/ltc2990.c     | 160
>> ++++++++++++++++++++++++++++++++++++++++++++
>>   4 files changed, 219 insertions(+)
>>   create mode 100644 Documentation/hwmon/ltc2990
>>   create mode 100644 drivers/hwmon/ltc2990.c
>>
>> diff --git a/Documentation/hwmon/ltc2990 b/Documentation/hwmon/ltc2990
>> new file mode 100644
>> index 0000000..838b74e
>> --- /dev/null
>> +++ b/Documentation/hwmon/ltc2990
>> @@ -0,0 +1,44 @@
>> +Kernel driver ltc2990
>> +=====================
>> +
>> +Supported chips:
>> +  * Linear Technology LTC2990
>> +    Prefix: 'ltc2990'
>> +    Addresses scanned: -
>> +    Datasheet: http://www.linear.com/product/ltc2990
>> +
>> +Author: Mike Looijmans <mike.looijmans@topic.nl>
>> +
>> +
>> +Description
>> +-----------
>> +
>> +LTC2990 is a Quad I2C Voltage, Current and Temperature Monitor.
>> +The chip's inputs can measure 4 voltages, or two inputs together (1+2 and 3+4)
>> +can be combined to measure a differential voltage, which is typically used to
>> +measure current through a series resistor, or a temperature.
>> +
>> +This driver currently uses the 2x differential mode only. In order to support
>> +other modes, the driver will need to be expanded.
>> +
>> +
>> +Usage Notes
>> +-----------
>> +
>> +This driver does not probe for PMBus devices. You will have to instantiate
>> +devices explicitly.
>> +
>> +
>> +Sysfs attributes
>> +----------------
>> +
>> +The "curr*_input" measurements actually report the voltage drop across the
>> +input pins in microvolts. This is equivalent to the current through a 1mOhm
>> +sense resistor. Divide the reported value by the actual sense resistor value
>> +in mOhm to get the actual value.
>> +
>> +in0_input     Voltage at Vcc pin in millivolt (range 2.5V to 5V)
>> +temp1_input   Internal chip temperature in millidegrees Celcius
>> +curr1_input   Current in mA across v1-v2 assuming a 1mOhm sense resistor.
>> +curr2_input   Current in mA across v3-v4 assuming a 1mOhm sense resistor.
>> +
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 80a73bf..8a31d64 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -685,6 +685,20 @@ config SENSORS_LTC2945
>>         This driver can also be built as a module. If so, the module will
>>         be called ltc2945.
>>
>> +config SENSORS_LTC2990
>> +    tristate "Linear Technology LTC2990 (current monitoring mode only)"
>> +    depends on I2C
>> +    help
>> +      If you say yes here you get support for Linear Technology LTC2990
>> +      I2C System Monitor. The LTC2990 supports a combination of voltage,
>> +      current and temperature monitoring, but in addition to the Vcc supply
>> +      voltage and chip temperature, this driver currently only supports
>> +      reading two currents by measuring two differential voltages across
>> +      series resistors.
>> +
>> +      This driver can also be built as a module. If so, the module will
>> +      be called ltc2990.
>> +
>>   config SENSORS_LTC4151
>>       tristate "Linear Technology LTC4151"
>>       depends on I2C
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index 12a3239..e4bd15b 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -101,6 +101,7 @@ obj-$(CONFIG_SENSORS_LM95234)    += lm95234.o
>>   obj-$(CONFIG_SENSORS_LM95241)    += lm95241.o
>>   obj-$(CONFIG_SENSORS_LM95245)    += lm95245.o
>>   obj-$(CONFIG_SENSORS_LTC2945)    += ltc2945.o
>> +obj-$(CONFIG_SENSORS_LTC2990)    += ltc2990.o
>>   obj-$(CONFIG_SENSORS_LTC4151)    += ltc4151.o
>>   obj-$(CONFIG_SENSORS_LTC4215)    += ltc4215.o
>>   obj-$(CONFIG_SENSORS_LTC4222)    += ltc4222.o
>> diff --git a/drivers/hwmon/ltc2990.c b/drivers/hwmon/ltc2990.c
>> new file mode 100644
>> index 0000000..3720ff7
>> --- /dev/null
>> +++ b/drivers/hwmon/ltc2990.c
>> @@ -0,0 +1,160 @@
>> +/*
>> + * Driver for Linear Technology LTC2990 power monitor
>> + *
>> + * Copyright (C) 2014 Topic Embedded Products
>> + * Author: Mike Looijmans <mike.looijmans@topic.nl>
>> + *
>> + * License: GPLv2
>> + *
>> + * This driver assumes the chip is wired as a dual current monitor, and
>> + * reports the voltage drop across two series resistors. It also reports
>> + * the chip's internal temperature and Vcc power supply voltage.
>> + */
>> +
>> +#include <linux/delay.h>

Not needed, will be removed in v3

>> +#include <linux/err.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/hwmon-sysfs.h>
>> +#include <linux/i2c.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/slab.h>

Not needed

>> +
>> +#define LTC2990_STATUS    0x00
>> +#define LTC2990_CONTROL    0x01
>> +#define LTC2990_TRIGGER    0x02
>> +#define LTC2990_TINT_MSB    0x04
>> +#define LTC2990_TINT_LSB    0x05
>> +#define LTC2990_V1_MSB    0x06
>> +#define LTC2990_V1_LSB    0x07
>> +#define LTC2990_V2_MSB    0x08
>> +#define LTC2990_V2_LSB    0x09
>> +#define LTC2990_V3_MSB    0x0A
>> +#define LTC2990_V3_LSB    0x0B
>> +#define LTC2990_V4_MSB    0x0C
>> +#define LTC2990_V4_LSB    0x0D
>> +#define LTC2990_VCC_MSB    0x0E
>> +#define LTC2990_VCC_LSB    0x0F
>> +
>
> LSB not used.

Agree. Will remove them, it's clutter.

>
>> +#define LTC2990_STATUS_BUSY    BIT(0)
>> +#define LTC2990_STATUS_TINT    BIT(1)
>> +#define LTC2990_STATUS_V1    BIT(2)
>> +#define LTC2990_STATUS_V2    BIT(3)
>> +#define LTC2990_STATUS_V3    BIT(4)
>> +#define LTC2990_STATUS_V4    BIT(5)
>> +#define LTC2990_STATUS_VCC    BIT(6)
>> +
> No longer used ?

No need for status read when in continuous mode, so they have no point being here.

>
>> +/* Only define control settings we actually use */
>
> Hmmm ... but not all are used.

I think it's better to remove that comment :)

>> +#define LTC2990_CONTROL_KELVIN        BIT(7)
>> +#define LTC2990_CONTROL_SINGLE        BIT(6)
>> +#define LTC2990_CONTROL_MEASURE_ALL    (0x3 << 3)
>> +#define LTC2990_CONTROL_MODE_CURRENT    0x06
>> +#define LTC2990_CONTROL_MODE_VOLTAGE    0x07
>> +
>> +/* convert raw register value to sign-extended integer in 16-bit range */
>> +static int ltc2990_voltage_to_int(int raw)
>> +{
>> +    if (raw & BIT(14)) {
>> +        return -(0x4000 - (raw & 0x3FFF)) << 2;
>> +    } else {
>> +        return (raw & 0x3FFF) << 2;
>> +    }
>> +}
>> +
>> +/* Return the converted value from the given register in uV or mC */
>> +static int ltc2990_get_value(struct i2c_client *i2c, u8 index)
>> +{
>> +    int val;
>> +    int result;
>> +
>> +    val = i2c_smbus_read_word_swapped(i2c, (index << 1) + LTC2990_TINT_MSB);
>> +    if (unlikely(val < 0))
>> +        return val;
>> +
>> +    if (index == 0) { /* internal temp, 0.0625 degrees/LSB, 13-bit  */
>> +        val = (val & 0x1FFF) << 3;
>> +        result = (val * 1000) >> 7;
>> +    } else if (index < 5) { /* Vx-Vy, 19.42uV/LSB */
>> +        result = ltc2990_voltage_to_int(val) * 1942 / (4 * 100);
>> +    } else { /* Vcc, 305.18μV/LSB, 2.5V offset */
>> +        result = ltc2990_voltage_to_int(val) * 30518 / (4 * 100 * 1000);
>> +        result += 2500;
>> +    }
>> +
> With the register in index (see below) this could be
>
>      val = i2c_smbus_read_word_swapped(i2c, index);
>
>      switch (index) {
>      case LTC2990_TINT_MSB:
>          val = (val & 0x1FFF) << 3;
>          result = (val * 1000) >> 7;
>          break;
>      case LTC2990_V1_MSB:
>      case LTC2990_V2_MSB:
>          ...
>          result = ltc2990_voltage_to_int(val) * 1942 / (4 * 100);
>          break;
>      case LTC2990_VCC_MSB:
>          result = ltc2990_voltage_to_int(val) * 30518 / (4 * 100 * 1000);
>          result += 2500;
>          break;
>      default:
>          result = 0;    /* won't happen, makes compiler happy */
>          break;
>      }
>
> which I think would be easier to understand.

Agree, good point. Also using the register names helps readability.

>> +    return result;
>> +}
>> +
>> +static ssize_t ltc2990_show_value(struct device *dev,
>> +                  struct device_attribute *da, char *buf)
>> +{
>> +    struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
>> +    int value;
>> +
>> +    value = ltc2990_get_value(dev_get_drvdata(dev), attr->index);
>> +    return snprintf(buf, PAGE_SIZE, "%d\n", value);
>> +}
>> +
>> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, ltc2990_show_value, NULL, 0);
>> +static SENSOR_DEVICE_ATTR(curr1_input, S_IRUGO, ltc2990_show_value, NULL, 1);
>> +static SENSOR_DEVICE_ATTR(curr2_input, S_IRUGO, ltc2990_show_value, NULL, 3);
>> +static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ltc2990_show_value, NULL, 5);
>
> Consider providing the register MSB in index.
>
> Example:
>      static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, ltc2990_show_value, NULL,
> LTC2990_TINT_MSB);

Agree, as above.

>> +
>> +static struct attribute *ltc2990_attrs[] = {
>> +    &sensor_dev_attr_temp1_input.dev_attr.attr,
>> +    &sensor_dev_attr_curr1_input.dev_attr.attr,
>> +    &sensor_dev_attr_curr2_input.dev_attr.attr,
>> +    &sensor_dev_attr_in0_input.dev_attr.attr,
>> +    NULL,
>> +};
>> +ATTRIBUTE_GROUPS(ltc2990);
>> +
>> +static int ltc2990_i2c_probe(struct i2c_client *i2c,
>> +    const struct i2c_device_id *id)
>
> Please align continuation lines with '('.
>
>> +{
>> +    int ret;
>> +    struct device *hwmon_dev;
>> +
>> +    if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
>
> Also need capability to read words.

Guess so...

>
>> +        return -ENODEV;
>> +
>> +    /* Setup continuous mode, current monitor */
>> +    ret = i2c_smbus_write_byte_data(i2c, LTC2990_CONTROL,
>> +        LTC2990_CONTROL_MEASURE_ALL | LTC2990_CONTROL_MODE_CURRENT);
>> +    if (ret < 0) {
>> +        dev_err(&i2c->dev, "Error: Failed to set control mode.\n");
>> +        return ret;
>> +    }
>> +    /* Trigger once to start continuous conversion */
>> +    ret = i2c_smbus_write_byte_data(i2c, LTC2990_TRIGGER, 1);
>> +    if (ret < 0) {
>> +        dev_err(&i2c->dev, "Error: Failed to start aquisition.\n");
>
> s/aquisition/acquisition/
>
>> +        return ret;
>> +    }
>> +
>> +    hwmon_dev = devm_hwmon_device_register_with_groups(&i2c->dev,
>> +                               i2c->name,
>> +                               i2c,
>> +                               ltc2990_groups);
>> +
>> +    return PTR_ERR_OR_ZERO(hwmon_dev);
>> +}
>> +
>> +static const struct i2c_device_id ltc2990_i2c_id[] = {
>> +    { "ltc2990", 0 },
>> +    {}
>> +};
>> +MODULE_DEVICE_TABLE(i2c, ltc2990_i2c_id);
>> +
>> +static struct i2c_driver ltc2990_i2c_driver = {
>> +    .driver = {
>> +        .name = "ltc2990",
>> +    },
>> +    .probe    = ltc2990_i2c_probe,
>> +    .id_table = ltc2990_i2c_id,
>> +};
>> +
>> +module_i2c_driver(ltc2990_i2c_driver);
>> +
>> +MODULE_DESCRIPTION("LTC2990 Sensor Driver");
>> +MODULE_AUTHOR("Topic Embedded Products");
>> +MODULE_LICENSE("GPL v2");
>>
>



Kind regards,

Mike Looijmans
System Expert

TOPIC Embedded Products
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
E-mail: mike.looijmans@topicproducts.com
Website: www.topicproducts.com

Please consider the environment before printing this e-mail

Re: [PATCH v2] hwmon: Add LTC2990 sensor driver

[Guenter Roeck]

On 01/13/2016 05:51 AM, Mike Looijmans wrote:
> On 13-01-16 14:24, Guenter Roeck wrote:
>> On 01/13/2016 03:05 AM, Mike Looijmans wrote:
>>> This adds support for the Linear Technology LTC2990  I2C System Monitor.
>>> The LTC2990 supports a combination of voltage, current and temperature
>>> monitoring. This driver currently only supports reading two currents
>>> by measuring two differential voltages across series resistors, in
>>> addition to the Vcc supply voltage and internal temperature.
>>>
>>> This is sufficient to support the Topic Miami SOM which uses this chip
>>> to monitor the currents flowing into the FPGA and the CPU parts.
>>>
>>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
>>
>> Mike,
>>
>> That looks much better. Can you send me the output of i2cdump for the chip ?
>> That would help me writing module test code for it.
>
> I'm kinda interested into how that would work.
>
https://github.com/groeck/module-tests

> I'll have to remove the driver first to get i2cdump to work on the chip. I cannot force a device removal from user space while running, can I?
> And i suppose you want a dump of a chip in running status? (On boot, all are registers are simply set to zero)
>

Just use i2cdump -f while the driver is active.

Thanks,
Guenter

Re: [PATCH v2] hwmon: Add LTC2990 sensor driver

[Mike Looijmans]

On 13-01-16 14:57, Guenter Roeck wrote:
> On 01/13/2016 05:51 AM, Mike Looijmans wrote:
>> On 13-01-16 14:24, Guenter Roeck wrote:
>>> On 01/13/2016 03:05 AM, Mike Looijmans wrote:
>>>> This adds support for the Linear Technology LTC2990  I2C System Monitor.
>>>> The LTC2990 supports a combination of voltage, current and temperature
>>>> monitoring. This driver currently only supports reading two currents
>>>> by measuring two differential voltages across series resistors, in
>>>> addition to the Vcc supply voltage and internal temperature.
>>>>
>>>> This is sufficient to support the Topic Miami SOM which uses this chip
>>>> to monitor the currents flowing into the FPGA and the CPU parts.
>>>>
>>>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
>>>
>>> Mike,
>>>
>>> That looks much better. Can you send me the output of i2cdump for the chip ?
>>> That would help me writing module test code for it.
>>
>> I'm kinda interested into how that would work.
>>
> https://github.com/groeck/module-tests
>
>> I'll have to remove the driver first to get i2cdump to work on the chip. I
>> cannot force a device removal from user space while running, can I?
>> And i suppose you want a dump of a chip in running status? (On boot, all are
>> registers are simply set to zero)
>>
>
> Just use i2cdump -f while the driver is active.

Here's the dump (twice so you can see the A/D converter is actually running):

root@topic-miami-florida-pci-xc7z015:~# i2cdump -y -f -r 0-0xf 1 0x4c b
      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 7f 1e 7f 7f 82 5f 80 44 80 44 80 3f 80 3f 89 b5    ?????_?D?D??????
root@topic-miami-florida-pci-xc7z015:~# i2cdump -y -f -r 0-0xf 1 0x4c b
      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 7f 1e 7f 7f 82 65 80 42 80 42 80 3d 80 3d 89 b4    ?????e?B?B?=?=??




Kind regards,

Mike Looijmans
System Expert

TOPIC Embedded Products
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
E-mail: mike.looijmans@topicproducts.com
Website: www.topicproducts.com

Please consider the environment before printing this e-mail

[PATCH v3] hwmon: Add LTC2990 sensor driver

[Mike Looijmans]

This adds support for the Linear Technology LTC2990  I2C System Monitor.
The LTC2990 supports a combination of voltage, current and temperature
monitoring. This driver currently only supports reading two currents
by measuring two differential voltages across series resistors, in
addition to the Vcc supply voltage and internal temperature.

This is sufficient to support the Topic Miami SOM which uses this chip
to monitor the currents flowing into the FPGA and the CPU parts.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
---
v3: Remove unused includes.
    Remove (most) unused register defines.
    Also check on SMBUS WORD capability.
    Use register defines as value indices.
    Alignment fixups with "(".
v2: Processed all review comments.
     Put chip into continuous measurement mode.
     Added ducumentation.
     Use standard hwmon interfaces and macros.

 Documentation/hwmon/ltc2990 |  44 ++++++++++++
 drivers/hwmon/Kconfig       |  14 ++++
 drivers/hwmon/Makefile      |   1 +
 drivers/hwmon/ltc2990.c     | 159 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 218 insertions(+)
 create mode 100644 Documentation/hwmon/ltc2990
 create mode 100644 drivers/hwmon/ltc2990.c

diff --git a/Documentation/hwmon/ltc2990 b/Documentation/hwmon/ltc2990
new file mode 100644
index 0000000..838b74e
--- /dev/null
+++ b/Documentation/hwmon/ltc2990
@@ -0,0 +1,44 @@
+Kernel driver ltc2990
+=====================
+
+Supported chips:
+  * Linear Technology LTC2990
+    Prefix: 'ltc2990'
+    Addresses scanned: -
+    Datasheet: http://www.linear.com/product/ltc2990
+
+Author: Mike Looijmans <mike.looijmans@topic.nl>
+
+
+Description
+-----------
+
+LTC2990 is a Quad I2C Voltage, Current and Temperature Monitor.
+The chip's inputs can measure 4 voltages, or two inputs together (1+2 and 3+4)
+can be combined to measure a differential voltage, which is typically used to
+measure current through a series resistor, or a temperature.
+
+This driver currently uses the 2x differential mode only. In order to support
+other modes, the driver will need to be expanded.
+
+
+Usage Notes
+-----------
+
+This driver does not probe for PMBus devices. You will have to instantiate
+devices explicitly.
+
+
+Sysfs attributes
+----------------
+
+The "curr*_input" measurements actually report the voltage drop across the
+input pins in microvolts. This is equivalent to the current through a 1mOhm
+sense resistor. Divide the reported value by the actual sense resistor value
+in mOhm to get the actual value.
+
+in0_input     Voltage at Vcc pin in millivolt (range 2.5V to 5V)
+temp1_input   Internal chip temperature in millidegrees Celcius
+curr1_input   Current in mA across v1-v2 assuming a 1mOhm sense resistor.
+curr2_input   Current in mA across v3-v4 assuming a 1mOhm sense resistor.
+
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 80a73bf..8a31d64 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -685,6 +685,20 @@ config SENSORS_LTC2945
 	  This driver can also be built as a module. If so, the module will
 	  be called ltc2945.
 
+config SENSORS_LTC2990
+	tristate "Linear Technology LTC2990 (current monitoring mode only)"
+	depends on I2C
+	help
+	  If you say yes here you get support for Linear Technology LTC2990
+	  I2C System Monitor. The LTC2990 supports a combination of voltage,
+	  current and temperature monitoring, but in addition to the Vcc supply
+	  voltage and chip temperature, this driver currently only supports
+	  reading two currents by measuring two differential voltages across
+	  series resistors.
+
+	  This driver can also be built as a module. If so, the module will
+	  be called ltc2990.
+
 config SENSORS_LTC4151
 	tristate "Linear Technology LTC4151"
 	depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 12a3239..e4bd15b 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -101,6 +101,7 @@ obj-$(CONFIG_SENSORS_LM95234)	+= lm95234.o
 obj-$(CONFIG_SENSORS_LM95241)	+= lm95241.o
 obj-$(CONFIG_SENSORS_LM95245)	+= lm95245.o
 obj-$(CONFIG_SENSORS_LTC2945)	+= ltc2945.o
+obj-$(CONFIG_SENSORS_LTC2990)	+= ltc2990.o
 obj-$(CONFIG_SENSORS_LTC4151)	+= ltc4151.o
 obj-$(CONFIG_SENSORS_LTC4215)	+= ltc4215.o
 obj-$(CONFIG_SENSORS_LTC4222)	+= ltc4222.o
diff --git a/drivers/hwmon/ltc2990.c b/drivers/hwmon/ltc2990.c
new file mode 100644
index 0000000..37ca5f4
--- /dev/null
+++ b/drivers/hwmon/ltc2990.c
@@ -0,0 +1,159 @@
+/*
+ * Driver for Linear Technology LTC2990 power monitor
+ *
+ * Copyright (C) 2014 Topic Embedded Products
+ * Author: Mike Looijmans <mike.looijmans@topic.nl>
+ *
+ * License: GPLv2
+ *
+ * This driver assumes the chip is wired as a dual current monitor, and
+ * reports the voltage drop across two series resistors. It also reports
+ * the chip's internal temperature and Vcc power supply voltage.
+ */
+
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+#define LTC2990_STATUS	0x00
+#define LTC2990_CONTROL	0x01
+#define LTC2990_TRIGGER	0x02
+#define LTC2990_TINT_MSB	0x04
+#define LTC2990_V1_MSB	0x06
+#define LTC2990_V2_MSB	0x08
+#define LTC2990_V3_MSB	0x0A
+#define LTC2990_V4_MSB	0x0C
+#define LTC2990_VCC_MSB	0x0E
+
+#define LTC2990_CONTROL_KELVIN		BIT(7)
+#define LTC2990_CONTROL_SINGLE		BIT(6)
+#define LTC2990_CONTROL_MEASURE_ALL	(0x3 << 3)
+#define LTC2990_CONTROL_MODE_CURRENT	0x06
+#define LTC2990_CONTROL_MODE_VOLTAGE	0x07
+
+/* convert raw register value to sign-extended integer in 16-bit range */
+static int ltc2990_voltage_to_int(int raw)
+{
+	if (raw & BIT(14)) {
+		return -(0x4000 - (raw & 0x3FFF)) << 2;
+	} else {
+		return (raw & 0x3FFF) << 2;
+	}
+}
+
+/* Return the converted value from the given register in uV or mC */
+static int ltc2990_get_value(struct i2c_client *i2c, u8 reg)
+{
+	int val;
+	int result;
+
+	val = i2c_smbus_read_word_swapped(i2c, reg);
+	if (unlikely(val < 0))
+		return val;
+	switch (reg) {
+	case LTC2990_TINT_MSB:
+		/* internal temp, 0.0625 degrees/LSB, 13-bit  */
+		val = (val & 0x1FFF) << 3;
+		result = (val * 1000) >> 7;
+		break;
+	case LTC2990_V1_MSB:
+	case LTC2990_V3_MSB:
+		 /* Vx-Vy, 19.42uV/LSB. Depends on mode. */
+		result = ltc2990_voltage_to_int(val) * 1942 / (4 * 100);
+		break;
+	case LTC2990_VCC_MSB:
+		/* Vcc, 305.18μV/LSB, 2.5V offset */
+		result = ltc2990_voltage_to_int(val) * 30518 / (4 * 100 * 1000);
+		result += 2500;
+		break;
+	default:
+		result = 0; /* won't happen, keep compiler happy */
+		break;
+	}
+
+	return result;
+}
+
+static ssize_t ltc2990_show_value(struct device *dev,
+				  struct device_attribute *da, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
+	int value;
+
+	value = ltc2990_get_value(dev_get_drvdata(dev), attr->index);
+	return snprintf(buf, PAGE_SIZE, "%d\n", value);
+}
+
+static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, ltc2990_show_value, NULL,
+			  LTC2990_TINT_MSB);
+static SENSOR_DEVICE_ATTR(curr1_input, S_IRUGO, ltc2990_show_value, NULL,
+			  LTC2990_V1_MSB);
+static SENSOR_DEVICE_ATTR(curr2_input, S_IRUGO, ltc2990_show_value, NULL,
+			  LTC2990_V3_MSB);
+static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ltc2990_show_value, NULL,
+			  LTC2990_VCC_MSB);
+
+static struct attribute *ltc2990_attrs[] = {
+	&sensor_dev_attr_temp1_input.dev_attr.attr,
+	&sensor_dev_attr_curr1_input.dev_attr.attr,
+	&sensor_dev_attr_curr2_input.dev_attr.attr,
+	&sensor_dev_attr_in0_input.dev_attr.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(ltc2990);
+
+static int ltc2990_i2c_probe(struct i2c_client *i2c,
+			     const struct i2c_device_id *id)
+{
+	int ret;
+	struct device *hwmon_dev;
+
+	if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_SMBUS_BYTE_DATA |
+				     I2C_FUNC_SMBUS_WORD_DATA))
+		return -ENODEV;
+
+	/* Setup continuous mode, current monitor */
+	ret = i2c_smbus_write_byte_data(i2c, LTC2990_CONTROL,
+					LTC2990_CONTROL_MEASURE_ALL |
+					LTC2990_CONTROL_MODE_CURRENT);
+	if (ret < 0) {
+		dev_err(&i2c->dev, "Error: Failed to set control mode.\n");
+		return ret;
+	}
+	/* Trigger once to start continuous conversion */
+	ret = i2c_smbus_write_byte_data(i2c, LTC2990_TRIGGER, 1);
+	if (ret < 0) {
+		dev_err(&i2c->dev, "Error: Failed to start acquisition.\n");
+		return ret;
+	}
+
+	hwmon_dev = devm_hwmon_device_register_with_groups(&i2c->dev,
+							   i2c->name,
+							   i2c,
+							   ltc2990_groups);
+
+	return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static const struct i2c_device_id ltc2990_i2c_id[] = {
+	{ "ltc2990", 0 },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, ltc2990_i2c_id);
+
+static struct i2c_driver ltc2990_i2c_driver = {
+	.driver = {
+		.name = "ltc2990",
+	},
+	.probe    = ltc2990_i2c_probe,
+	.id_table = ltc2990_i2c_id,
+};
+
+module_i2c_driver(ltc2990_i2c_driver);
+
+MODULE_DESCRIPTION("LTC2990 Sensor Driver");
+MODULE_AUTHOR("Topic Embedded Products");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

Re: [PATCH v3] hwmon: Add LTC2990 sensor driver

[Guenter Roeck]

On Wed, Jan 13, 2016 at 03:45:01PM +0100, Mike Looijmans wrote:
> This adds support for the Linear Technology LTC2990  I2C System Monitor.
> The LTC2990 supports a combination of voltage, current and temperature
> monitoring. This driver currently only supports reading two currents
> by measuring two differential voltages across series resistors, in
> addition to the Vcc supply voltage and internal temperature.
> 
> This is sufficient to support the Topic Miami SOM which uses this chip
> to monitor the currents flowing into the FPGA and the CPU parts.
> 
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>

Hi Mike,

almost there. Please see inline.

Thanks,
Guenter

> ---
> v3: Remove unused includes.
>     Remove (most) unused register defines.
>     Also check on SMBUS WORD capability.
>     Use register defines as value indices.
>     Alignment fixups with "(".
> v2: Processed all review comments.
>      Put chip into continuous measurement mode.
>      Added ducumentation.
>      Use standard hwmon interfaces and macros.
> 
>  Documentation/hwmon/ltc2990 |  44 ++++++++++++
>  drivers/hwmon/Kconfig       |  14 ++++
>  drivers/hwmon/Makefile      |   1 +
>  drivers/hwmon/ltc2990.c     | 159 ++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 218 insertions(+)
>  create mode 100644 Documentation/hwmon/ltc2990
>  create mode 100644 drivers/hwmon/ltc2990.c
> 
> diff --git a/Documentation/hwmon/ltc2990 b/Documentation/hwmon/ltc2990
> new file mode 100644
> index 0000000..838b74e
> --- /dev/null
> +++ b/Documentation/hwmon/ltc2990
> @@ -0,0 +1,44 @@
> +Kernel driver ltc2990
> +=====================
> +
> +Supported chips:
> +  * Linear Technology LTC2990
> +    Prefix: 'ltc2990'
> +    Addresses scanned: -
> +    Datasheet: http://www.linear.com/product/ltc2990
> +
> +Author: Mike Looijmans <mike.looijmans@topic.nl>
> +
> +
> +Description
> +-----------
> +
> +LTC2990 is a Quad I2C Voltage, Current and Temperature Monitor.
> +The chip's inputs can measure 4 voltages, or two inputs together (1+2 and 3+4)
> +can be combined to measure a differential voltage, which is typically used to
> +measure current through a series resistor, or a temperature.
> +
> +This driver currently uses the 2x differential mode only. In order to support
> +other modes, the driver will need to be expanded.
> +
> +
> +Usage Notes
> +-----------
> +
> +This driver does not probe for PMBus devices. You will have to instantiate
> +devices explicitly.
> +
> +
> +Sysfs attributes
> +----------------
> +
> +The "curr*_input" measurements actually report the voltage drop across the
> +input pins in microvolts. This is equivalent to the current through a 1mOhm
> +sense resistor. Divide the reported value by the actual sense resistor value
> +in mOhm to get the actual value.
> +
> +in0_input     Voltage at Vcc pin in millivolt (range 2.5V to 5V)
> +temp1_input   Internal chip temperature in millidegrees Celcius
> +curr1_input   Current in mA across v1-v2 assuming a 1mOhm sense resistor.
> +curr2_input   Current in mA across v3-v4 assuming a 1mOhm sense resistor.
> +
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 80a73bf..8a31d64 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -685,6 +685,20 @@ config SENSORS_LTC2945
>  	  This driver can also be built as a module. If so, the module will
>  	  be called ltc2945.
>  
> +config SENSORS_LTC2990
> +	tristate "Linear Technology LTC2990 (current monitoring mode only)"
> +	depends on I2C
> +	help
> +	  If you say yes here you get support for Linear Technology LTC2990
> +	  I2C System Monitor. The LTC2990 supports a combination of voltage,
> +	  current and temperature monitoring, but in addition to the Vcc supply
> +	  voltage and chip temperature, this driver currently only supports
> +	  reading two currents by measuring two differential voltages across
> +	  series resistors.
> +
> +	  This driver can also be built as a module. If so, the module will
> +	  be called ltc2990.
> +
>  config SENSORS_LTC4151
>  	tristate "Linear Technology LTC4151"
>  	depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 12a3239..e4bd15b 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -101,6 +101,7 @@ obj-$(CONFIG_SENSORS_LM95234)	+= lm95234.o
>  obj-$(CONFIG_SENSORS_LM95241)	+= lm95241.o
>  obj-$(CONFIG_SENSORS_LM95245)	+= lm95245.o
>  obj-$(CONFIG_SENSORS_LTC2945)	+= ltc2945.o
> +obj-$(CONFIG_SENSORS_LTC2990)	+= ltc2990.o
>  obj-$(CONFIG_SENSORS_LTC4151)	+= ltc4151.o
>  obj-$(CONFIG_SENSORS_LTC4215)	+= ltc4215.o
>  obj-$(CONFIG_SENSORS_LTC4222)	+= ltc4222.o
> diff --git a/drivers/hwmon/ltc2990.c b/drivers/hwmon/ltc2990.c
> new file mode 100644
> index 0000000..37ca5f4
> --- /dev/null
> +++ b/drivers/hwmon/ltc2990.c
> @@ -0,0 +1,159 @@
> +/*
> + * Driver for Linear Technology LTC2990 power monitor
> + *
> + * Copyright (C) 2014 Topic Embedded Products
> + * Author: Mike Looijmans <mike.looijmans@topic.nl>
> + *
> + * License: GPLv2
> + *
> + * This driver assumes the chip is wired as a dual current monitor, and
> + * reports the voltage drop across two series resistors. It also reports
> + * the chip's internal temperature and Vcc power supply voltage.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +
> +#define LTC2990_STATUS	0x00
> +#define LTC2990_CONTROL	0x01
> +#define LTC2990_TRIGGER	0x02
> +#define LTC2990_TINT_MSB	0x04
> +#define LTC2990_V1_MSB	0x06
> +#define LTC2990_V2_MSB	0x08
> +#define LTC2990_V3_MSB	0x0A
> +#define LTC2990_V4_MSB	0x0C
> +#define LTC2990_VCC_MSB	0x0E
> +
> +#define LTC2990_CONTROL_KELVIN		BIT(7)
> +#define LTC2990_CONTROL_SINGLE		BIT(6)
> +#define LTC2990_CONTROL_MEASURE_ALL	(0x3 << 3)
> +#define LTC2990_CONTROL_MODE_CURRENT	0x06
> +#define LTC2990_CONTROL_MODE_VOLTAGE	0x07
> +
> +/* convert raw register value to sign-extended integer in 16-bit range */
> +static int ltc2990_voltage_to_int(int raw)
> +{
> +	if (raw & BIT(14)) {
> +		return -(0x4000 - (raw & 0x3FFF)) << 2;
> +	} else {
> +		return (raw & 0x3FFF) << 2;
> +	}

Unnecessary { }. Please see checkpatch warning.

> +}
> +
> +/* Return the converted value from the given register in uV or mC */
> +static int ltc2990_get_value(struct i2c_client *i2c, u8 reg)
> +{
> +	int val;
> +	int result;
> +
> +	val = i2c_smbus_read_word_swapped(i2c, reg);
> +	if (unlikely(val < 0))
> +		return val;

This suggests the function returns a value < 0 on error ...

> +	switch (reg) {
> +	case LTC2990_TINT_MSB:
> +		/* internal temp, 0.0625 degrees/LSB, 13-bit  */
> +		val = (val & 0x1FFF) << 3;
> +		result = (val * 1000) >> 7;
> +		break;
> +	case LTC2990_V1_MSB:
> +	case LTC2990_V3_MSB:
> +		 /* Vx-Vy, 19.42uV/LSB. Depends on mode. */
> +		result = ltc2990_voltage_to_int(val) * 1942 / (4 * 100);
> +		break;
> +	case LTC2990_VCC_MSB:
> +		/* Vcc, 305.18μV/LSB, 2.5V offset */
> +		result = ltc2990_voltage_to_int(val) * 30518 / (4 * 100 * 1000);
> +		result += 2500;
> +		break;
> +	default:
> +		result = 0; /* won't happen, keep compiler happy */

Given that, we can return some error here, and ...

> +		break;
> +	}
> +
> +	return result;
> +}
> +
> +static ssize_t ltc2990_show_value(struct device *dev,
> +				  struct device_attribute *da, char *buf)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> +	int value;
> +
> +	value = ltc2990_get_value(dev_get_drvdata(dev), attr->index);

... we should actually check the error return here.

	if (value < 0)
		return value;

> +	return snprintf(buf, PAGE_SIZE, "%d\n", value);
> +}
> +
> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, ltc2990_show_value, NULL,
> +			  LTC2990_TINT_MSB);
> +static SENSOR_DEVICE_ATTR(curr1_input, S_IRUGO, ltc2990_show_value, NULL,
> +			  LTC2990_V1_MSB);
> +static SENSOR_DEVICE_ATTR(curr2_input, S_IRUGO, ltc2990_show_value, NULL,
> +			  LTC2990_V3_MSB);
> +static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ltc2990_show_value, NULL,
> +			  LTC2990_VCC_MSB);
> +
> +static struct attribute *ltc2990_attrs[] = {
> +	&sensor_dev_attr_temp1_input.dev_attr.attr,
> +	&sensor_dev_attr_curr1_input.dev_attr.attr,
> +	&sensor_dev_attr_curr2_input.dev_attr.attr,
> +	&sensor_dev_attr_in0_input.dev_attr.attr,
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(ltc2990);
> +
> +static int ltc2990_i2c_probe(struct i2c_client *i2c,
> +			     const struct i2c_device_id *id)
> +{
> +	int ret;
> +	struct device *hwmon_dev;
> +
> +	if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_SMBUS_BYTE_DATA |
> +				     I2C_FUNC_SMBUS_WORD_DATA))
> +		return -ENODEV;
> +
> +	/* Setup continuous mode, current monitor */
> +	ret = i2c_smbus_write_byte_data(i2c, LTC2990_CONTROL,
> +					LTC2990_CONTROL_MEASURE_ALL |
> +					LTC2990_CONTROL_MODE_CURRENT);
> +	if (ret < 0) {
> +		dev_err(&i2c->dev, "Error: Failed to set control mode.\n");
> +		return ret;
> +	}
> +	/* Trigger once to start continuous conversion */
> +	ret = i2c_smbus_write_byte_data(i2c, LTC2990_TRIGGER, 1);
> +	if (ret < 0) {
> +		dev_err(&i2c->dev, "Error: Failed to start acquisition.\n");
> +		return ret;
> +	}
> +
> +	hwmon_dev = devm_hwmon_device_register_with_groups(&i2c->dev,
> +							   i2c->name,
> +							   i2c,
> +							   ltc2990_groups);
> +
> +	return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static const struct i2c_device_id ltc2990_i2c_id[] = {
> +	{ "ltc2990", 0 },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, ltc2990_i2c_id);
> +
> +static struct i2c_driver ltc2990_i2c_driver = {
> +	.driver = {
> +		.name = "ltc2990",
> +	},
> +	.probe    = ltc2990_i2c_probe,
> +	.id_table = ltc2990_i2c_id,
> +};
> +
> +module_i2c_driver(ltc2990_i2c_driver);
> +
> +MODULE_DESCRIPTION("LTC2990 Sensor Driver");
> +MODULE_AUTHOR("Topic Embedded Products");
> +MODULE_LICENSE("GPL v2");
> -- 
> 1.9.1
> 

Re: [PATCH v3] hwmon: Add LTC2990 sensor driver

[Mike Looijmans]

On 14-01-16 20:14, Guenter Roeck wrote:
> On Wed, Jan 13, 2016 at 03:45:01PM +0100, Mike Looijmans wrote:
>> This adds support for the Linear Technology LTC2990  I2C System Monitor.
>> The LTC2990 supports a combination of voltage, current and temperature
>> monitoring. This driver currently only supports reading two currents
>> by measuring two differential voltages across series resistors, in
>> addition to the Vcc supply voltage and internal temperature.
>>
>> This is sufficient to support the Topic Miami SOM which uses this chip
>> to monitor the currents flowing into the FPGA and the CPU parts.
>>
>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
>
> Hi Mike,
>
> almost there. Please see inline.

Great, I'll order some cake :)

Expect v4 in a few minutes, I'm testing it on the hardware now.

> Thanks,
> Guenter
>
>> ---
>> v3: Remove unused includes.
>>      Remove (most) unused register defines.
>>      Also check on SMBUS WORD capability.
>>      Use register defines as value indices.
>>      Alignment fixups with "(".
>> v2: Processed all review comments.
>>       Put chip into continuous measurement mode.
>>       Added ducumentation.
>>       Use standard hwmon interfaces and macros.
>>
>>   Documentation/hwmon/ltc2990 |  44 ++++++++++++
>>   drivers/hwmon/Kconfig       |  14 ++++
>>   drivers/hwmon/Makefile      |   1 +
>>   drivers/hwmon/ltc2990.c     | 159 ++++++++++++++++++++++++++++++++++++++++++++
>>   4 files changed, 218 insertions(+)
>>   create mode 100644 Documentation/hwmon/ltc2990
>>   create mode 100644 drivers/hwmon/ltc2990.c
>>
>> diff --git a/Documentation/hwmon/ltc2990 b/Documentation/hwmon/ltc2990
>> new file mode 100644
>> index 0000000..838b74e
>> --- /dev/null
>> +++ b/Documentation/hwmon/ltc2990
>> @@ -0,0 +1,44 @@
>> +Kernel driver ltc2990
>> +=====================
>> +
>> +Supported chips:
>> +  * Linear Technology LTC2990
>> +    Prefix: 'ltc2990'
>> +    Addresses scanned: -
>> +    Datasheet: http://www.linear.com/product/ltc2990
>> +
>> +Author: Mike Looijmans <mike.looijmans@topic.nl>
>> +
>> +
>> +Description
>> +-----------
>> +
>> +LTC2990 is a Quad I2C Voltage, Current and Temperature Monitor.
>> +The chip's inputs can measure 4 voltages, or two inputs together (1+2 and 3+4)
>> +can be combined to measure a differential voltage, which is typically used to
>> +measure current through a series resistor, or a temperature.
>> +
>> +This driver currently uses the 2x differential mode only. In order to support
>> +other modes, the driver will need to be expanded.
>> +
>> +
>> +Usage Notes
>> +-----------
>> +
>> +This driver does not probe for PMBus devices. You will have to instantiate
>> +devices explicitly.
>> +
>> +
>> +Sysfs attributes
>> +----------------
>> +
>> +The "curr*_input" measurements actually report the voltage drop across the
>> +input pins in microvolts. This is equivalent to the current through a 1mOhm
>> +sense resistor. Divide the reported value by the actual sense resistor value
>> +in mOhm to get the actual value.
>> +
>> +in0_input     Voltage at Vcc pin in millivolt (range 2.5V to 5V)
>> +temp1_input   Internal chip temperature in millidegrees Celcius
>> +curr1_input   Current in mA across v1-v2 assuming a 1mOhm sense resistor.
>> +curr2_input   Current in mA across v3-v4 assuming a 1mOhm sense resistor.
>> +
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 80a73bf..8a31d64 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -685,6 +685,20 @@ config SENSORS_LTC2945
>>   	  This driver can also be built as a module. If so, the module will
>>   	  be called ltc2945.
>>
>> +config SENSORS_LTC2990
>> +	tristate "Linear Technology LTC2990 (current monitoring mode only)"
>> +	depends on I2C
>> +	help
>> +	  If you say yes here you get support for Linear Technology LTC2990
>> +	  I2C System Monitor. The LTC2990 supports a combination of voltage,
>> +	  current and temperature monitoring, but in addition to the Vcc supply
>> +	  voltage and chip temperature, this driver currently only supports
>> +	  reading two currents by measuring two differential voltages across
>> +	  series resistors.
>> +
>> +	  This driver can also be built as a module. If so, the module will
>> +	  be called ltc2990.
>> +
>>   config SENSORS_LTC4151
>>   	tristate "Linear Technology LTC4151"
>>   	depends on I2C
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index 12a3239..e4bd15b 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -101,6 +101,7 @@ obj-$(CONFIG_SENSORS_LM95234)	+= lm95234.o
>>   obj-$(CONFIG_SENSORS_LM95241)	+= lm95241.o
>>   obj-$(CONFIG_SENSORS_LM95245)	+= lm95245.o
>>   obj-$(CONFIG_SENSORS_LTC2945)	+= ltc2945.o
>> +obj-$(CONFIG_SENSORS_LTC2990)	+= ltc2990.o
>>   obj-$(CONFIG_SENSORS_LTC4151)	+= ltc4151.o
>>   obj-$(CONFIG_SENSORS_LTC4215)	+= ltc4215.o
>>   obj-$(CONFIG_SENSORS_LTC4222)	+= ltc4222.o
>> diff --git a/drivers/hwmon/ltc2990.c b/drivers/hwmon/ltc2990.c
>> new file mode 100644
>> index 0000000..37ca5f4
>> --- /dev/null
>> +++ b/drivers/hwmon/ltc2990.c
>> @@ -0,0 +1,159 @@
>> +/*
>> + * Driver for Linear Technology LTC2990 power monitor
>> + *
>> + * Copyright (C) 2014 Topic Embedded Products
>> + * Author: Mike Looijmans <mike.looijmans@topic.nl>
>> + *
>> + * License: GPLv2
>> + *
>> + * This driver assumes the chip is wired as a dual current monitor, and
>> + * reports the voltage drop across two series resistors. It also reports
>> + * the chip's internal temperature and Vcc power supply voltage.
>> + */
>> +
>> +#include <linux/err.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/hwmon-sysfs.h>
>> +#include <linux/i2c.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +
>> +#define LTC2990_STATUS	0x00
>> +#define LTC2990_CONTROL	0x01
>> +#define LTC2990_TRIGGER	0x02
>> +#define LTC2990_TINT_MSB	0x04
>> +#define LTC2990_V1_MSB	0x06
>> +#define LTC2990_V2_MSB	0x08
>> +#define LTC2990_V3_MSB	0x0A
>> +#define LTC2990_V4_MSB	0x0C
>> +#define LTC2990_VCC_MSB	0x0E
>> +
>> +#define LTC2990_CONTROL_KELVIN		BIT(7)
>> +#define LTC2990_CONTROL_SINGLE		BIT(6)
>> +#define LTC2990_CONTROL_MEASURE_ALL	(0x3 << 3)
>> +#define LTC2990_CONTROL_MODE_CURRENT	0x06
>> +#define LTC2990_CONTROL_MODE_VOLTAGE	0x07
>> +
>> +/* convert raw register value to sign-extended integer in 16-bit range */
>> +static int ltc2990_voltage_to_int(int raw)
>> +{
>> +	if (raw & BIT(14)) {
>> +		return -(0x4000 - (raw & 0x3FFF)) << 2;
>> +	} else {
>> +		return (raw & 0x3FFF) << 2;
>> +	}
>
> Unnecessary { }. Please see checkpatch warning.

I'll make sure to run checkpatch again. Its only remaining warning is about 
MAINTAINERS now.

>> +}
>> +
>> +/* Return the converted value from the given register in uV or mC */
>> +static int ltc2990_get_value(struct i2c_client *i2c, u8 reg)
>> +{
>> +	int val;
>> +	int result;
>> +
>> +	val = i2c_smbus_read_word_swapped(i2c, reg);
>> +	if (unlikely(val < 0))
>> +		return val;
>
> This suggests the function returns a value < 0 on error ...

Misleading, since most of the valid results can be negative. I'll change the 
method to return a result code and take a result pointer instead.

>
>> +	switch (reg) {
>> +	case LTC2990_TINT_MSB:
>> +		/* internal temp, 0.0625 degrees/LSB, 13-bit  */
>> +		val = (val & 0x1FFF) << 3;
>> +		result = (val * 1000) >> 7;
>> +		break;
>> +	case LTC2990_V1_MSB:
>> +	case LTC2990_V3_MSB:
>> +		 /* Vx-Vy, 19.42uV/LSB. Depends on mode. */
>> +		result = ltc2990_voltage_to_int(val) * 1942 / (4 * 100);
>> +		break;
>> +	case LTC2990_VCC_MSB:
>> +		/* Vcc, 305.18μV/LSB, 2.5V offset */
>> +		result = ltc2990_voltage_to_int(val) * 30518 / (4 * 100 * 1000);
>> +		result += 2500;
>> +		break;
>> +	default:
>> +		result = 0; /* won't happen, keep compiler happy */
>
> Given that, we can return some error here, and ...
>
>> +		break;
>> +	}
>> +
>> +	return result;
>> +}
>> +
>> +static ssize_t ltc2990_show_value(struct device *dev,
>> +				  struct device_attribute *da, char *buf)
>> +{
>> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
>> +	int value;
>> +
>> +	value = ltc2990_get_value(dev_get_drvdata(dev), attr->index);
>
> ... we should actually check the error return here.
>
> 	if (value < 0)
> 		return value;

Yes, with the change above, it can properly return an error code if the I2C 
transaction failed.

>> +	return snprintf(buf, PAGE_SIZE, "%d\n", value);
>> +}
>> +
>> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, ltc2990_show_value, NULL,
>> +			  LTC2990_TINT_MSB);
>> +static SENSOR_DEVICE_ATTR(curr1_input, S_IRUGO, ltc2990_show_value, NULL,
>> +			  LTC2990_V1_MSB);
>> +static SENSOR_DEVICE_ATTR(curr2_input, S_IRUGO, ltc2990_show_value, NULL,
>> +			  LTC2990_V3_MSB);
>> +static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ltc2990_show_value, NULL,
>> +			  LTC2990_VCC_MSB);
>> +
>> +static struct attribute *ltc2990_attrs[] = {
>> +	&sensor_dev_attr_temp1_input.dev_attr.attr,
>> +	&sensor_dev_attr_curr1_input.dev_attr.attr,
>> +	&sensor_dev_attr_curr2_input.dev_attr.attr,
>> +	&sensor_dev_attr_in0_input.dev_attr.attr,
>> +	NULL,
>> +};
>> +ATTRIBUTE_GROUPS(ltc2990);
>> +
>> +static int ltc2990_i2c_probe(struct i2c_client *i2c,
>> +			     const struct i2c_device_id *id)
>> +{
>> +	int ret;
>> +	struct device *hwmon_dev;
>> +
>> +	if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_SMBUS_BYTE_DATA |
>> +				     I2C_FUNC_SMBUS_WORD_DATA))
>> +		return -ENODEV;
>> +
>> +	/* Setup continuous mode, current monitor */
>> +	ret = i2c_smbus_write_byte_data(i2c, LTC2990_CONTROL,
>> +					LTC2990_CONTROL_MEASURE_ALL |
>> +					LTC2990_CONTROL_MODE_CURRENT);
>> +	if (ret < 0) {
>> +		dev_err(&i2c->dev, "Error: Failed to set control mode.\n");
>> +		return ret;
>> +	}
>> +	/* Trigger once to start continuous conversion */
>> +	ret = i2c_smbus_write_byte_data(i2c, LTC2990_TRIGGER, 1);
>> +	if (ret < 0) {
>> +		dev_err(&i2c->dev, "Error: Failed to start acquisition.\n");
>> +		return ret;
>> +	}
>> +
>> +	hwmon_dev = devm_hwmon_device_register_with_groups(&i2c->dev,
>> +							   i2c->name,
>> +							   i2c,
>> +							   ltc2990_groups);
>> +
>> +	return PTR_ERR_OR_ZERO(hwmon_dev);
>> +}
>> +
>> +static const struct i2c_device_id ltc2990_i2c_id[] = {
>> +	{ "ltc2990", 0 },
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(i2c, ltc2990_i2c_id);
>> +
>> +static struct i2c_driver ltc2990_i2c_driver = {
>> +	.driver = {
>> +		.name = "ltc2990",
>> +	},
>> +	.probe    = ltc2990_i2c_probe,
>> +	.id_table = ltc2990_i2c_id,
>> +};
>> +
>> +module_i2c_driver(ltc2990_i2c_driver);
>> +
>> +MODULE_DESCRIPTION("LTC2990 Sensor Driver");
>> +MODULE_AUTHOR("Topic Embedded Products");
>> +MODULE_LICENSE("GPL v2");
>> --
>> 1.9.1
>>



Kind regards,

Mike Looijmans
System Expert

TOPIC Embedded Products
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
E-mail: mike.looijmans@topicproducts.com
Website: www.topicproducts.com

Please consider the environment before printing this e-mail

[PATCH v4] hwmon: Add LTC2990 sensor driver

[Mike Looijmans]

This adds support for the Linear Technology LTC2990  I2C System Monitor.
The LTC2990 supports a combination of voltage, current and temperature
monitoring. This driver currently only supports reading two currents
by measuring two differential voltages across series resistors, in
addition to the Vcc supply voltage and internal temperature.

This is sufficient to support the Topic Miami SOM which uses this chip
to monitor the currents flowing into the FPGA and the CPU parts.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
---
v4: Fix checkpatch warnings
    ltc2990_get_value can return error code
v3: Remove unused includes.
    Remove (most) unused register defines.
    Also check on SMBUS WORD capability.
    Use register defines as value indices.
    Alignment fixups with "(".
v2: Processed all review comments.
     Put chip into continuous measurement mode.
     Added ducumentation.
     Use standard hwmon interfaces and macros.
 Documentation/hwmon/ltc2990 |  44 ++++++++++++
 drivers/hwmon/Kconfig       |  14 ++++
 drivers/hwmon/Makefile      |   1 +
 drivers/hwmon/ltc2990.c     | 161 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 220 insertions(+)
 create mode 100644 Documentation/hwmon/ltc2990
 create mode 100644 drivers/hwmon/ltc2990.c

diff --git a/Documentation/hwmon/ltc2990 b/Documentation/hwmon/ltc2990
new file mode 100644
index 0000000..838b74e
--- /dev/null
+++ b/Documentation/hwmon/ltc2990
@@ -0,0 +1,44 @@
+Kernel driver ltc2990
+=====================
+
+Supported chips:
+  * Linear Technology LTC2990
+    Prefix: 'ltc2990'
+    Addresses scanned: -
+    Datasheet: http://www.linear.com/product/ltc2990
+
+Author: Mike Looijmans <mike.looijmans@topic.nl>
+
+
+Description
+-----------
+
+LTC2990 is a Quad I2C Voltage, Current and Temperature Monitor.
+The chip's inputs can measure 4 voltages, or two inputs together (1+2 and 3+4)
+can be combined to measure a differential voltage, which is typically used to
+measure current through a series resistor, or a temperature.
+
+This driver currently uses the 2x differential mode only. In order to support
+other modes, the driver will need to be expanded.
+
+
+Usage Notes
+-----------
+
+This driver does not probe for PMBus devices. You will have to instantiate
+devices explicitly.
+
+
+Sysfs attributes
+----------------
+
+The "curr*_input" measurements actually report the voltage drop across the
+input pins in microvolts. This is equivalent to the current through a 1mOhm
+sense resistor. Divide the reported value by the actual sense resistor value
+in mOhm to get the actual value.
+
+in0_input     Voltage at Vcc pin in millivolt (range 2.5V to 5V)
+temp1_input   Internal chip temperature in millidegrees Celcius
+curr1_input   Current in mA across v1-v2 assuming a 1mOhm sense resistor.
+curr2_input   Current in mA across v3-v4 assuming a 1mOhm sense resistor.
+
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 80a73bf..8a31d64 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -685,6 +685,20 @@ config SENSORS_LTC2945
 	  This driver can also be built as a module. If so, the module will
 	  be called ltc2945.
 
+config SENSORS_LTC2990
+	tristate "Linear Technology LTC2990 (current monitoring mode only)"
+	depends on I2C
+	help
+	  If you say yes here you get support for Linear Technology LTC2990
+	  I2C System Monitor. The LTC2990 supports a combination of voltage,
+	  current and temperature monitoring, but in addition to the Vcc supply
+	  voltage and chip temperature, this driver currently only supports
+	  reading two currents by measuring two differential voltages across
+	  series resistors.
+
+	  This driver can also be built as a module. If so, the module will
+	  be called ltc2990.
+
 config SENSORS_LTC4151
 	tristate "Linear Technology LTC4151"
 	depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 12a3239..e4bd15b 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -101,6 +101,7 @@ obj-$(CONFIG_SENSORS_LM95234)	+= lm95234.o
 obj-$(CONFIG_SENSORS_LM95241)	+= lm95241.o
 obj-$(CONFIG_SENSORS_LM95245)	+= lm95245.o
 obj-$(CONFIG_SENSORS_LTC2945)	+= ltc2945.o
+obj-$(CONFIG_SENSORS_LTC2990)	+= ltc2990.o
 obj-$(CONFIG_SENSORS_LTC4151)	+= ltc4151.o
 obj-$(CONFIG_SENSORS_LTC4215)	+= ltc4215.o
 obj-$(CONFIG_SENSORS_LTC4222)	+= ltc4222.o
diff --git a/drivers/hwmon/ltc2990.c b/drivers/hwmon/ltc2990.c
new file mode 100644
index 0000000..8f8fe05
--- /dev/null
+++ b/drivers/hwmon/ltc2990.c
@@ -0,0 +1,161 @@
+/*
+ * Driver for Linear Technology LTC2990 power monitor
+ *
+ * Copyright (C) 2014 Topic Embedded Products
+ * Author: Mike Looijmans <mike.looijmans@topic.nl>
+ *
+ * License: GPLv2
+ *
+ * This driver assumes the chip is wired as a dual current monitor, and
+ * reports the voltage drop across two series resistors. It also reports
+ * the chip's internal temperature and Vcc power supply voltage.
+ */
+
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+#define LTC2990_STATUS	0x00
+#define LTC2990_CONTROL	0x01
+#define LTC2990_TRIGGER	0x02
+#define LTC2990_TINT_MSB	0x04
+#define LTC2990_V1_MSB	0x06
+#define LTC2990_V2_MSB	0x08
+#define LTC2990_V3_MSB	0x0A
+#define LTC2990_V4_MSB	0x0C
+#define LTC2990_VCC_MSB	0x0E
+
+#define LTC2990_CONTROL_KELVIN		BIT(7)
+#define LTC2990_CONTROL_SINGLE		BIT(6)
+#define LTC2990_CONTROL_MEASURE_ALL	(0x3 << 3)
+#define LTC2990_CONTROL_MODE_CURRENT	0x06
+#define LTC2990_CONTROL_MODE_VOLTAGE	0x07
+
+/* convert raw register value to sign-extended integer in 16-bit range */
+static int ltc2990_voltage_to_int(int raw)
+{
+	if (raw & BIT(14))
+		return -(0x4000 - (raw & 0x3FFF)) << 2;
+	else
+		return (raw & 0x3FFF) << 2;
+}
+
+/* Return the converted value from the given register in uV or mC */
+static int ltc2990_get_value(struct i2c_client *i2c, u8 reg, int *result)
+{
+	int val;
+
+	val = i2c_smbus_read_word_swapped(i2c, reg);
+	if (unlikely(val < 0))
+		return val;
+
+	switch (reg) {
+	case LTC2990_TINT_MSB:
+		/* internal temp, 0.0625 degrees/LSB, 13-bit  */
+		val = (val & 0x1FFF) << 3;
+		*result = (val * 1000) >> 7;
+		break;
+	case LTC2990_V1_MSB:
+	case LTC2990_V3_MSB:
+		 /* Vx-Vy, 19.42uV/LSB. Depends on mode. */
+		*result = ltc2990_voltage_to_int(val) * 1942 / (4 * 100);
+		break;
+	case LTC2990_VCC_MSB:
+		/* Vcc, 305.18μV/LSB, 2.5V offset */
+		*result = (ltc2990_voltage_to_int(val) * 30518 /
+			   (4 * 100 * 1000)) + 2500;
+		break;
+	default:
+		return -EINVAL; /* won't happen, keep compiler happy */
+	}
+
+	return 0;
+}
+
+static ssize_t ltc2990_show_value(struct device *dev,
+				  struct device_attribute *da, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
+	int value;
+	int ret;
+
+	ret = ltc2990_get_value(dev_get_drvdata(dev), attr->index, &value);
+	if (unlikely(ret < 0))
+		return ret;
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", value);
+}
+
+static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, ltc2990_show_value, NULL,
+			  LTC2990_TINT_MSB);
+static SENSOR_DEVICE_ATTR(curr1_input, S_IRUGO, ltc2990_show_value, NULL,
+			  LTC2990_V1_MSB);
+static SENSOR_DEVICE_ATTR(curr2_input, S_IRUGO, ltc2990_show_value, NULL,
+			  LTC2990_V3_MSB);
+static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ltc2990_show_value, NULL,
+			  LTC2990_VCC_MSB);
+
+static struct attribute *ltc2990_attrs[] = {
+	&sensor_dev_attr_temp1_input.dev_attr.attr,
+	&sensor_dev_attr_curr1_input.dev_attr.attr,
+	&sensor_dev_attr_curr2_input.dev_attr.attr,
+	&sensor_dev_attr_in0_input.dev_attr.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(ltc2990);
+
+static int ltc2990_i2c_probe(struct i2c_client *i2c,
+			     const struct i2c_device_id *id)
+{
+	int ret;
+	struct device *hwmon_dev;
+
+	if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_SMBUS_BYTE_DATA |
+				     I2C_FUNC_SMBUS_WORD_DATA))
+		return -ENODEV;
+
+	/* Setup continuous mode, current monitor */
+	ret = i2c_smbus_write_byte_data(i2c, LTC2990_CONTROL,
+					LTC2990_CONTROL_MEASURE_ALL |
+					LTC2990_CONTROL_MODE_CURRENT);
+	if (ret < 0) {
+		dev_err(&i2c->dev, "Error: Failed to set control mode.\n");
+		return ret;
+	}
+	/* Trigger once to start continuous conversion */
+	ret = i2c_smbus_write_byte_data(i2c, LTC2990_TRIGGER, 1);
+	if (ret < 0) {
+		dev_err(&i2c->dev, "Error: Failed to start acquisition.\n");
+		return ret;
+	}
+
+	hwmon_dev = devm_hwmon_device_register_with_groups(&i2c->dev,
+							   i2c->name,
+							   i2c,
+							   ltc2990_groups);
+
+	return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static const struct i2c_device_id ltc2990_i2c_id[] = {
+	{ "ltc2990", 0 },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, ltc2990_i2c_id);
+
+static struct i2c_driver ltc2990_i2c_driver = {
+	.driver = {
+		.name = "ltc2990",
+	},
+	.probe    = ltc2990_i2c_probe,
+	.id_table = ltc2990_i2c_id,
+};
+
+module_i2c_driver(ltc2990_i2c_driver);
+
+MODULE_DESCRIPTION("LTC2990 Sensor Driver");
+MODULE_AUTHOR("Topic Embedded Products");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

Re: [PATCH v4] hwmon: Add LTC2990 sensor driver

[Guenter Roeck]

On 01/15/2016 01:54 AM, Mike Looijmans wrote:
> This adds support for the Linear Technology LTC2990  I2C System Monitor.
> The LTC2990 supports a combination of voltage, current and temperature
> monitoring. This driver currently only supports reading two currents
> by measuring two differential voltages across series resistors, in
> addition to the Vcc supply voltage and internal temperature.
>
> This is sufficient to support the Topic Miami SOM which uses this chip
> to monitor the currents flowing into the FPGA and the CPU parts.
>
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>

Applied to -next.

Thanks,
Guenter

> ---
> v4: Fix checkpatch warnings
>      ltc2990_get_value can return error code
> v3: Remove unused includes.
>      Remove (most) unused register defines.
>      Also check on SMBUS WORD capability.
>      Use register defines as value indices.
>      Alignment fixups with "(".
> v2: Processed all review comments.
>       Put chip into continuous measurement mode.
>       Added ducumentation.
>       Use standard hwmon interfaces and macros.
>   Documentation/hwmon/ltc2990 |  44 ++++++++++++
>   drivers/hwmon/Kconfig       |  14 ++++
>   drivers/hwmon/Makefile      |   1 +
>   drivers/hwmon/ltc2990.c     | 161 ++++++++++++++++++++++++++++++++++++++++++++
>   4 files changed, 220 insertions(+)
>   create mode 100644 Documentation/hwmon/ltc2990
>   create mode 100644 drivers/hwmon/ltc2990.c
>
> diff --git a/Documentation/hwmon/ltc2990 b/Documentation/hwmon/ltc2990
> new file mode 100644
> index 0000000..838b74e
> --- /dev/null
> +++ b/Documentation/hwmon/ltc2990
> @@ -0,0 +1,44 @@
> +Kernel driver ltc2990
> +=====================
> +
> +Supported chips:
> +  * Linear Technology LTC2990
> +    Prefix: 'ltc2990'
> +    Addresses scanned: -
> +    Datasheet: http://www.linear.com/product/ltc2990
> +
> +Author: Mike Looijmans <mike.looijmans@topic.nl>
> +
> +
> +Description
> +-----------
> +
> +LTC2990 is a Quad I2C Voltage, Current and Temperature Monitor.
> +The chip's inputs can measure 4 voltages, or two inputs together (1+2 and 3+4)
> +can be combined to measure a differential voltage, which is typically used to
> +measure current through a series resistor, or a temperature.
> +
> +This driver currently uses the 2x differential mode only. In order to support
> +other modes, the driver will need to be expanded.
> +
> +
> +Usage Notes
> +-----------
> +
> +This driver does not probe for PMBus devices. You will have to instantiate
> +devices explicitly.
> +
> +
> +Sysfs attributes
> +----------------
> +
> +The "curr*_input" measurements actually report the voltage drop across the
> +input pins in microvolts. This is equivalent to the current through a 1mOhm
> +sense resistor. Divide the reported value by the actual sense resistor value
> +in mOhm to get the actual value.
> +
> +in0_input     Voltage at Vcc pin in millivolt (range 2.5V to 5V)
> +temp1_input   Internal chip temperature in millidegrees Celcius
> +curr1_input   Current in mA across v1-v2 assuming a 1mOhm sense resistor.
> +curr2_input   Current in mA across v3-v4 assuming a 1mOhm sense resistor.
> +
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 80a73bf..8a31d64 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -685,6 +685,20 @@ config SENSORS_LTC2945
>   	  This driver can also be built as a module. If so, the module will
>   	  be called ltc2945.
>
> +config SENSORS_LTC2990
> +	tristate "Linear Technology LTC2990 (current monitoring mode only)"
> +	depends on I2C
> +	help
> +	  If you say yes here you get support for Linear Technology LTC2990
> +	  I2C System Monitor. The LTC2990 supports a combination of voltage,
> +	  current and temperature monitoring, but in addition to the Vcc supply
> +	  voltage and chip temperature, this driver currently only supports
> +	  reading two currents by measuring two differential voltages across
> +	  series resistors.
> +
> +	  This driver can also be built as a module. If so, the module will
> +	  be called ltc2990.
> +
>   config SENSORS_LTC4151
>   	tristate "Linear Technology LTC4151"
>   	depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 12a3239..e4bd15b 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -101,6 +101,7 @@ obj-$(CONFIG_SENSORS_LM95234)	+= lm95234.o
>   obj-$(CONFIG_SENSORS_LM95241)	+= lm95241.o
>   obj-$(CONFIG_SENSORS_LM95245)	+= lm95245.o
>   obj-$(CONFIG_SENSORS_LTC2945)	+= ltc2945.o
> +obj-$(CONFIG_SENSORS_LTC2990)	+= ltc2990.o
>   obj-$(CONFIG_SENSORS_LTC4151)	+= ltc4151.o
>   obj-$(CONFIG_SENSORS_LTC4215)	+= ltc4215.o
>   obj-$(CONFIG_SENSORS_LTC4222)	+= ltc4222.o
> diff --git a/drivers/hwmon/ltc2990.c b/drivers/hwmon/ltc2990.c
> new file mode 100644
> index 0000000..8f8fe05
> --- /dev/null
> +++ b/drivers/hwmon/ltc2990.c
> @@ -0,0 +1,161 @@
> +/*
> + * Driver for Linear Technology LTC2990 power monitor
> + *
> + * Copyright (C) 2014 Topic Embedded Products
> + * Author: Mike Looijmans <mike.looijmans@topic.nl>
> + *
> + * License: GPLv2
> + *
> + * This driver assumes the chip is wired as a dual current monitor, and
> + * reports the voltage drop across two series resistors. It also reports
> + * the chip's internal temperature and Vcc power supply voltage.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +
> +#define LTC2990_STATUS	0x00
> +#define LTC2990_CONTROL	0x01
> +#define LTC2990_TRIGGER	0x02
> +#define LTC2990_TINT_MSB	0x04
> +#define LTC2990_V1_MSB	0x06
> +#define LTC2990_V2_MSB	0x08
> +#define LTC2990_V3_MSB	0x0A
> +#define LTC2990_V4_MSB	0x0C
> +#define LTC2990_VCC_MSB	0x0E
> +
> +#define LTC2990_CONTROL_KELVIN		BIT(7)
> +#define LTC2990_CONTROL_SINGLE		BIT(6)
> +#define LTC2990_CONTROL_MEASURE_ALL	(0x3 << 3)
> +#define LTC2990_CONTROL_MODE_CURRENT	0x06
> +#define LTC2990_CONTROL_MODE_VOLTAGE	0x07
> +
> +/* convert raw register value to sign-extended integer in 16-bit range */
> +static int ltc2990_voltage_to_int(int raw)
> +{
> +	if (raw & BIT(14))
> +		return -(0x4000 - (raw & 0x3FFF)) << 2;
> +	else
> +		return (raw & 0x3FFF) << 2;
> +}
> +
> +/* Return the converted value from the given register in uV or mC */
> +static int ltc2990_get_value(struct i2c_client *i2c, u8 reg, int *result)
> +{
> +	int val;
> +
> +	val = i2c_smbus_read_word_swapped(i2c, reg);
> +	if (unlikely(val < 0))
> +		return val;
> +
> +	switch (reg) {
> +	case LTC2990_TINT_MSB:
> +		/* internal temp, 0.0625 degrees/LSB, 13-bit  */
> +		val = (val & 0x1FFF) << 3;
> +		*result = (val * 1000) >> 7;
> +		break;
> +	case LTC2990_V1_MSB:
> +	case LTC2990_V3_MSB:
> +		 /* Vx-Vy, 19.42uV/LSB. Depends on mode. */
> +		*result = ltc2990_voltage_to_int(val) * 1942 / (4 * 100);
> +		break;
> +	case LTC2990_VCC_MSB:
> +		/* Vcc, 305.18μV/LSB, 2.5V offset */
> +		*result = (ltc2990_voltage_to_int(val) * 30518 /
> +			   (4 * 100 * 1000)) + 2500;
> +		break;
> +	default:
> +		return -EINVAL; /* won't happen, keep compiler happy */
> +	}
> +
> +	return 0;
> +}
> +
> +static ssize_t ltc2990_show_value(struct device *dev,
> +				  struct device_attribute *da, char *buf)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> +	int value;
> +	int ret;
> +
> +	ret = ltc2990_get_value(dev_get_drvdata(dev), attr->index, &value);
> +	if (unlikely(ret < 0))
> +		return ret;
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", value);
> +}
> +
> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, ltc2990_show_value, NULL,
> +			  LTC2990_TINT_MSB);
> +static SENSOR_DEVICE_ATTR(curr1_input, S_IRUGO, ltc2990_show_value, NULL,
> +			  LTC2990_V1_MSB);
> +static SENSOR_DEVICE_ATTR(curr2_input, S_IRUGO, ltc2990_show_value, NULL,
> +			  LTC2990_V3_MSB);
> +static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ltc2990_show_value, NULL,
> +			  LTC2990_VCC_MSB);
> +
> +static struct attribute *ltc2990_attrs[] = {
> +	&sensor_dev_attr_temp1_input.dev_attr.attr,
> +	&sensor_dev_attr_curr1_input.dev_attr.attr,
> +	&sensor_dev_attr_curr2_input.dev_attr.attr,
> +	&sensor_dev_attr_in0_input.dev_attr.attr,
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(ltc2990);
> +
> +static int ltc2990_i2c_probe(struct i2c_client *i2c,
> +			     const struct i2c_device_id *id)
> +{
> +	int ret;
> +	struct device *hwmon_dev;
> +
> +	if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_SMBUS_BYTE_DATA |
> +				     I2C_FUNC_SMBUS_WORD_DATA))
> +		return -ENODEV;
> +
> +	/* Setup continuous mode, current monitor */
> +	ret = i2c_smbus_write_byte_data(i2c, LTC2990_CONTROL,
> +					LTC2990_CONTROL_MEASURE_ALL |
> +					LTC2990_CONTROL_MODE_CURRENT);
> +	if (ret < 0) {
> +		dev_err(&i2c->dev, "Error: Failed to set control mode.\n");
> +		return ret;
> +	}
> +	/* Trigger once to start continuous conversion */
> +	ret = i2c_smbus_write_byte_data(i2c, LTC2990_TRIGGER, 1);
> +	if (ret < 0) {
> +		dev_err(&i2c->dev, "Error: Failed to start acquisition.\n");
> +		return ret;
> +	}
> +
> +	hwmon_dev = devm_hwmon_device_register_with_groups(&i2c->dev,
> +							   i2c->name,
> +							   i2c,
> +							   ltc2990_groups);
> +
> +	return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static const struct i2c_device_id ltc2990_i2c_id[] = {
> +	{ "ltc2990", 0 },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, ltc2990_i2c_id);
> +
> +static struct i2c_driver ltc2990_i2c_driver = {
> +	.driver = {
> +		.name = "ltc2990",
> +	},
> +	.probe    = ltc2990_i2c_probe,
> +	.id_table = ltc2990_i2c_id,
> +};
> +
> +module_i2c_driver(ltc2990_i2c_driver);
> +
> +MODULE_DESCRIPTION("LTC2990 Sensor Driver");
> +MODULE_AUTHOR("Topic Embedded Products");
> +MODULE_LICENSE("GPL v2");
>