[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5696500E.4090905@roeck-us.net>
Date: Wed, 13 Jan 2016 05:24:30 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Mike Looijmans <mike.looijmans@...ic.nl>, lm-sensors@...sensors.org
Cc: jdelvare@...e.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] hwmon: Add LTC2990 sensor driver
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@...ic.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@...ic.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@...ic.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");
>
Powered by blists - more mailing lists