Re: [PATCH] hwmon: Add LTC2990 sensor driver

From: Mike Looijmans
Date: Thu Jan 07 2016 - 13:59:38 EST

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@xxxxxxxx>
---
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@xxxxxxxx>
+ *
+ * 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