Message-ID: <5698C1DD.6000607@topic.nl>
Date:	Fri, 15 Jan 2016 10:54:37 +0100
From:	Mike Looijmans <mike.looijmans@...ic.nl>
To:	Guenter Roeck <linux@...ck-us.net>
CC:	<lm-sensors@...sensors.org>, <jdelvare@...e.com>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] hwmon: Add LTC2990 sensor driver

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 <mike.looijmans@...ic.nl>
>
> 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 <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..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@...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/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.

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@...icproducts.com
Website: www.topicproducts.com

Please consider the environment before printing this e-mail

Powered by blists - more mailing lists