From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756562AbcAMNYn (ORCPT ); Wed, 13 Jan 2016 08:24:43 -0500 Received: from bh-25.webhostbox.net ([208.91.199.152]:50963 "EHLO bh-25.webhostbox.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755790AbcAMNYk (ORCPT ); Wed, 13 Jan 2016 08:24:40 -0500 Subject: Re: [PATCH v2] hwmon: Add LTC2990 sensor driver To: Mike Looijmans , lm-sensors@lm-sensors.org References: <568FD131.4080308@roeck-us.net> <1452067627-24939-1-git-send-email-mike.looijmans@topic.nl> <1452683150-27747-1-git-send-email-mike.looijmans@topic.nl> Cc: jdelvare@suse.com, linux-kernel@vger.kernel.org From: Guenter Roeck Message-ID: <5696500E.4090905@roeck-us.net> Date: Wed, 13 Jan 2016 05:24:30 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: <1452683150-27747-1-git-send-email-mike.looijmans@topic.nl> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-Authenticated_sender: linux@roeck-us.net X-OutGoing-Spam-Status: No, score=-1.0 X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - bh-25.webhostbox.net X-AntiAbuse: Original Domain - vger.kernel.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - roeck-us.net X-Get-Message-Sender-Via: bh-25.webhostbox.net: authenticated_id: linux@roeck-us.net X-Authenticated-Sender: bh-25.webhostbox.net: linux@roeck-us.net X-Source: X-Source-Args: X-Source-Dir: Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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, 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 > + > + > +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 > + * > + * 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 > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#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"); >