| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <56965661.2010307@topic.nl>
Date: Wed, 13 Jan 2016 14:51:29 +0100
From: Mike Looijmans <mike.looijmans@...ic.nl>
To: Guenter Roeck <linux@...ck-us.net>, <lm-sensors@...sensors.org>
CC: <jdelvare@...e.com>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] hwmon: Add LTC2990 sensor driver
On 13-01-16 14:24, Guenter Roeck wrote:
> 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.
I'm kinda interested into how that would work.
I'll have to remove the driver first to get i2cdump to work on the chip. I
cannot force a device removal from user space while running, can I?
And i suppose you want a dump of a chip in running status? (On boot, all are
registers are simply set to zero)
> 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>
Not needed, will be removed in v3
>> +#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>
Not needed
>> +
>> +#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.
Agree. Will remove them, it's clutter.
>
>> +#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 ?
No need for status read when in continuous mode, so they have no point being here.
>
>> +/* Only define control settings we actually use */
>
> Hmmm ... but not all are used.
I think it's better to remove that comment :)
>> +#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.
Agree, good point. Also using the register names helps readability.
>> + 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);
Agree, as above.
>> +
>> +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.
Guess so...
>
>> + 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");
>>
>
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