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

From: Guenter Roeck
Date: Thu Jan 14 2016 - 14:14:25 EST

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

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