From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757301AbcAOJzj (ORCPT ); Fri, 15 Jan 2016 04:55:39 -0500 Received: from mx6-11.smtp.antispamcloud.com ([95.211.2.202]:51435 "EHLO mx6-11.smtp.antispamcloud.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757278AbcAOJzd convert rfc822-to-8bit (ORCPT ); Fri, 15 Jan 2016 04:55:33 -0500 Subject: Re: [PATCH v3] hwmon: Add LTC2990 sensor driver To: Guenter Roeck References: <1452683150-27747-1-git-send-email-mike.looijmans@topic.nl> <1452696301-28761-1-git-send-email-mike.looijmans@topic.nl> <20160114191415.GA30413@roeck-us.net> CC: , , From: Mike Looijmans Organization: TOPIC Message-ID: <5698C1DD.6000607@topic.nl> Date: Fri, 15 Jan 2016 10:54:37 +0100 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: <20160114191415.GA30413@roeck-us.net> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8BIT X-Originating-IP: [192.168.80.121] X-EXCLAIMER-MD-CONFIG: 9833cda7-5b21-4d34-9a38-8d025ddc3664 X-EXCLAIMER-MD-BIFURCATION-INSTANCE: 0 X-Filter-ID: s0sct1PQhAABKnZB5plbIbbvfIHzQjPVmPLZeVYSu3xU9luQrU+8/8qthi+0Jd/W6KAUC/fjyuDn NXFr4uarwyNwZSpVdakq5X66Q8VYQRca9vh+1iGKuQPOclYnYikQ0/2nHbqxWziKbKPiSpdwVJeh 6oj6+vlVI7HORWkrem3u4ONX+wVRdCv4IOs1C4geKx6e82Qhrl5Lh95/VjZqVHB+NvhiSkt9FGXX bXB10m9VPlSGMfU9Lzo0uxWN7kMhLncc/qbbv57GdnyRbm8yEsXNGmth+d+8KgKLd5cInbndzoq/ +Z/VaBbRH8ukCyAUIbtf63VNbf0lrvssY+k7ACgUUZ6G78uMfV2DLUDoJYSPCwPTzw22wYdX31FM YXOOFl5Bgdd0xwbfE0yjjQqqICERbInMiTBIUBbQ/Dy6Ip5J/ASe1B0hWvEIK5cPBacYKufgB7Ba elLkNTG3g1L+53OYYq89G8LTxEq5+I242X/3Y80OmAux3oN13+ztUzneK7xP+LD+FavI7Ol7e9pH /Pe4Uvw3tUqH8Ib6WPRGPWZ1++6w8PqYuLgGb7u7tZKksfrTSy315T+eQnDC8Q/96JFVgpT1b21u ZVckGp0ccOYWZk7PZs7jDt+VcZIxsBahwbRYKWMNtROYKfbuaAYREuN+dnGw834OHhrth+DryhAC LW1+1+M5yHGOoRTZGur+36K7vl8zpZeTek+zeKr5aP212Tjb53F+WW+V66zQ4+PJmA9TOeyOD4YF ZaxXY3oj4JtjPJ4R6ft//lif1yYpLLiLFZJhYijeIOrG82fwxtlvEjSGiKhzEHocL7YhFpj+ X-Report-Abuse-To: spam@mx99.antispamcloud.com X-Filter-Fingerprint: IFrWXGses7OKB5S5G8/dJXhXyDRoOQM5J3kcUr0HrMvJUWjZ8+qhjyB23tbDuyLOYL8Ff78gYsez 4Rl08xudmXi4esCQ0R1MchVjt7wblGlvhFgW0MjUMRkF5sMCDfftTXNFDzN17hnrWeZYOJvLq0Ic WjZ+XcEjj/7Pkld0zkmvziDInX9WdMov2kn2yXjdwv61T+KDYyYtREgszdyFwv8IxCB3p/oCKvxr eyISh3JGb7OS5oVgiO+kDxZrVPLz3MmEGC2PrUKqLq5WmHK+Nw== X-Originating-IP: 88.159.208.100 X-Spampanel-Domain: topic.nl X-Spampanel-Username: 88.159.208.100 Authentication-Results: antispamcloud.com; auth=pass smtp.auth=88.159.208.100@topic.nl X-Spampanel-Outgoing-Class: ham X-Spampanel-Outgoing-Evidence: SB/global_tokens (0.00222658862305) X-Recommended-Action: accept Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > > 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 >> + >> + >> +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 >> + * >> + * 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 >> + >> +#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