| 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: <57164869.8000004@ti.com>
Date: Tue, 19 Apr 2016 10:02:01 -0500
From: "Andrew F. Davis" <afd@...com>
To: Guenter Roeck <linux@...ck-us.net>,
Jean Delvare <jdelvare@...e.com>,
Jonathan Corbet <corbet@....net>
CC: <linux-hwmon@...r.kernel.org>, <linux-doc@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/1] hwmon: Add support for INA3221 Triple Current/Voltage
Monitors
On 04/19/2016 09:09 AM, Guenter Roeck wrote:
> On 04/11/2016 01:50 PM, Andrew F. Davis wrote:
>> Add support for the the INA3221 26v capable, Triple channel,
>> Bi-Directional, Zero-Drift, Low-/High-Side, Current/Voltage Monitor
>> with I2C interface.
>>
>> Signed-off-by: Andrew F. Davis <afd@...com>
>> ---
>> Documentation/hwmon/ina3221 | 32 ++++
>> drivers/hwmon/Kconfig | 11 ++
>> drivers/hwmon/Makefile | 1 +
>> drivers/hwmon/ina3221.c | 398
>> ++++++++++++++++++++++++++++++++++++++++++++
>> 4 files changed, 442 insertions(+)
>> create mode 100644 Documentation/hwmon/ina3221
>> create mode 100644 drivers/hwmon/ina3221.c
>>
>> diff --git a/Documentation/hwmon/ina3221 b/Documentation/hwmon/ina3221
>> new file mode 100644
>> index 0000000..3de3814
>> --- /dev/null
>> +++ b/Documentation/hwmon/ina3221
>> @@ -0,0 +1,32 @@
>> +Kernel driver ina3221
>> +=====================
>> +
>> +Supported chips:
>> + * Texas Instruments INA3221
>> + Prefix: 'ina3221'
>> + Addresses: I2C 0x40 - 0x43
>> + Datasheet: Publicly available at the Texas Instruments website
>> + http://www.ti.com/
>> +
>> +Author: Andrew F. Davis <afd@...com>
>> +
>> +Description
>> +-----------
>> +
>> +The Texas Instruments INA3221 monitors voltage, current, and power on
>> the high
>> +side of up to three D.C. power supplies. The INA3221 monitors both
>> shunt drop
>> +and supply voltage, with programmable conversion times and averaging,
>> current
>> +and power are calculated host-side from these.
>> +
>> +Sysfs entries
>> +-------------
>> +
>> +in[012]_input Bus voltages(mV) for channels 1, 2, and 3
>> respectively
>> +in[345]_input Shunt voltages(mV) for channels 1, 2, and 3
>> respectively
>> +curr[012]_input Current(mA) measurement for channels 1, 2,
>> and 3 respectively
>
> Numbering for currents is [123], not [012].
>
Okay, I'm guessing I should then change in to match?
>> +power[012]_input Power(uW) measurement for channels 1, 2, and
>> 3 respectively
>
> hwmon only reports what is there, and doesn't do calculations. The chip
> doesn't report
> the power, so please drop the power attributes.
>
Sure
>> +shunt[012]_resistor Shunt resistance(uOhm) for channels 1, 2, and
>> 3 respectively
>
> This should be a backup, if neither devicetree nor platform data are
> provided,
> not a primary means to provide the shunt resistor value. Also, there
> should be a default,
> to make sure the driver provides useful data even if nothing is configured.
> See ina2xx.c for an example.
>
I'll add DT support for this.
>> +critical[012]_alarm Critical alert voltage(mV) setting, activated
>> when the
>> + respective shunt voltage is above this value.
>> +warning[012]_alarm Warning alert voltage(mV) setting, activated
>> when the
>> + respective shunt voltage average is above
>> this value.
>
> Alarm attributes report the alarm status, not voltages. Use in[012]_max and
> in[012]_crit attributes to set the limits. Report the alarm status with
> in[012]_max_alarm and in[012]_crit_alarm (from register 0x0f, bit 9-7 and
> 5-3).
>
Will fix.
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 5c2d13a..de08242 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -1503,6 +1503,17 @@ config SENSORS_INA2XX
>> This driver can also be built as a module. If so, the module
>> will be called ina2xx.
>>
>> +config SENSORS_INA3221
>> + tristate "Texas Instruments INA3221 Triple Power Monitor"
>> + depends on I2C
>> + select REGMAP_I2C
>> + help
>> + If you say yes here you get support for the TI INA3221 Triple
>> Power
>> + Monitor.
>> +
>> + This driver can also be built as a module. If so, the module
>> + will be called ina3221.
>> +
>> config SENSORS_TC74
>> tristate "Microchip TC74"
>> depends on I2C
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index 58cc3ac..83e8ab0 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -77,6 +77,7 @@ obj-$(CONFIG_SENSORS_IBMPOWERNV)+= ibmpowernv.o
>> obj-$(CONFIG_SENSORS_IIO_HWMON) += iio_hwmon.o
>> obj-$(CONFIG_SENSORS_INA209) += ina209.o
>> obj-$(CONFIG_SENSORS_INA2XX) += ina2xx.o
>> +obj-$(CONFIG_SENSORS_INA3221) += ina3221.o
>> obj-$(CONFIG_SENSORS_IT87) += it87.o
>> obj-$(CONFIG_SENSORS_JC42) += jc42.o
>> obj-$(CONFIG_SENSORS_JZ4740) += jz4740-hwmon.o
>> diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
>> new file mode 100644
>> index 0000000..763e23b
>> --- /dev/null
>> +++ b/drivers/hwmon/ina3221.c
>> @@ -0,0 +1,398 @@
>> +/*
>> + * INA3221 Triple Current/Voltage Monitor
>> + *
>> + * Copyright (C) 2016 Texas Instruments Incorporated -
>> http://www.ti.com/
>> + * Andrew F. Davis <afd@...com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful, but
>> + * WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>> + * General Public License for more details.
>> + */
>> +
>> +#include <linux/i2c.h>
>> +#include <linux/module.h>
>> +#include <linux/regmap.h>
>> +
>> +#include <linux/hwmon.h>
>> +#include <linux/hwmon-sysfs.h>
>> +
>> +#define INA3221_DRIVER_NAME "ina3221"
>> +
>> +#define INA3221_CONFIG 0x00
>> +#define INA3221_SHUNT1 0x01
>> +#define INA3221_BUS1 0x02
>> +#define INA3221_SHUNT2 0x03
>> +#define INA3221_BUS2 0x04
>> +#define INA3221_SHUNT3 0x05
>> +#define INA3221_BUS3 0x06
>> +#define INA3221_CRIT1 0x07
>> +#define INA3221_WARN1 0x08
>> +#define INA3221_CRIT2 0x09
>> +#define INA3221_WARN2 0x0a
>> +#define INA3221_CRIT3 0x0b
>> +#define INA3221_WARN3 0x0c
>> +#define INA3221_SHUNT_SUM 0x0d
>> +#define INA3221_SHUNT_SUM_LIMIT 0x0e
>> +#define INA3221_MASK_ENABLE 0x0f
>> +#define INA3221_POWERV_HLIMIT 0x10
>> +#define INA3221_POWERV_LLIMIT 0x11
>> +
>> +#define INA3221_CONFIG_MODE_SHUNT BIT(1)
>> +#define INA3221_CONFIG_MODE_BUS BIT(2)
>> +#define INA3221_CONFIG_MODE_CONTINUOUS BIT(3)
>> +
>> +enum ina3221_fields {
>> + /* Configuration */
>> + F_MODE, F_SHUNT_CT, F_BUS_CT, F_AVG,
>> + F_CHAN3_EN, F_CHAN2_EN, F_CHAN1_EN, F_RST,
>> +
>> + /* sentinel */
>> + F_MAX_FIELDS
>> +};
>> +
>> +static const struct reg_field ina3221_reg_fields[] = {
>> + [F_MODE] = REG_FIELD(INA3221_CONFIG, 0, 2),
>> + [F_SHUNT_CT] = REG_FIELD(INA3221_CONFIG, 3, 5),
>> + [F_BUS_CT] = REG_FIELD(INA3221_CONFIG, 6, 8),
>> + [F_AVG] = REG_FIELD(INA3221_CONFIG, 9, 11),
>> + [F_CHAN3_EN] = REG_FIELD(INA3221_CONFIG, 12, 12),
>> + [F_CHAN2_EN] = REG_FIELD(INA3221_CONFIG, 13, 13),
>> + [F_CHAN1_EN] = REG_FIELD(INA3221_CONFIG, 14, 14),
>> + [F_RST] = REG_FIELD(INA3221_CONFIG, 15, 15),
>> +};
>> +
>> +enum ina3221_channels {
>> + INA3221_CHANNEL1,
>> + INA3221_CHANNEL2,
>> + INA3221_CHANNEL3,
>> + INA3221_NUM_CHANNELS
>> +};
>> +
>> +static const int shunt_registers[] = {
>> + [INA3221_CHANNEL1] = INA3221_SHUNT1,
>> + [INA3221_CHANNEL2] = INA3221_SHUNT2,
>> + [INA3221_CHANNEL3] = INA3221_SHUNT3,
>> +};
>> +
>> +static const int bus_registers[] = {
>> + [INA3221_CHANNEL1] = INA3221_BUS1,
>> + [INA3221_CHANNEL2] = INA3221_BUS2,
>> + [INA3221_CHANNEL3] = INA3221_BUS3,
>> +};
>> +
>> +/**
>> + * struct ina3221_data - device specific information
>> + * @dev: Device structure
>> + * @regmap: Register map of the device
>> + * @fields: Register fields of the device
>> + */
>> +struct ina3221_data {
>> + struct device *dev;
>> + struct regmap *regmap;
>> + struct regmap_field *fields[F_MAX_FIELDS];
>> + unsigned int shunt_resistors[INA3221_NUM_CHANNELS];
>> +};
>> +
>> +static int ina3221_read_value(struct ina3221_data *ina, unsigned int
>> reg,
>> + unsigned int *val)
>> +{
>> + unsigned int regval;
>> + int ret;
>> +
>> + ret = regmap_read(ina->regmap, reg, ®val);
>> + if (ret)
>> + return ret;
>> +
>> + *val = sign_extend32(regval >> 3, 12);
>> +
>> + return 0;
>> +}
>> +
>> +static ssize_t ina3221_show_voltage(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
>> + struct ina3221_data *ina = dev_get_drvdata(dev);
>> + unsigned int reg = sd_attr->index;
>> + int val, voltage_mv, ret;
>> +
>> + ret = ina3221_read_value(ina, reg, &val);
>> + if (ret)
>> + return ret;
>> +
>> + if (reg == INA3221_BUS1 ||
>> + reg == INA3221_BUS2 ||
>> + reg == INA3221_BUS3)
>> + voltage_mv = val * 8;
>> + else
>> + voltage_mv = DIV_ROUND_CLOSEST(val * 40, 1000);
>> +
>> + return snprintf(buf, PAGE_SIZE, "%d\n", voltage_mv);
>> +}
>> +
>> +static ssize_t ina3221_set_voltage(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
>> + struct ina3221_data *ina = dev_get_drvdata(dev);
>> + unsigned int reg = sd_attr->index;
>> + int val, ret;
>> +
>> + ret = kstrtoint(buf, 10, &val);
>> + if (ret)
>> + return ret;
>> +
> Please clamp the value to valid voltage range.
>
ACK
>> + val = DIV_ROUND_CLOSEST(val, 40000) << 3;
>> +
>> + ret = regmap_write(ina->regmap, reg, val);
>> + if (ret)
>> + return ret;
>> +
>> + return count;
>> +}
>> +
>> +static ssize_t ina3221_show_current(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
>> + struct ina3221_data *ina = dev_get_drvdata(dev);
>> + unsigned int channel = sd_attr->index;
>> + unsigned int shunt_reg, resistance_uo;
>> + int val, current_ma, shunt_voltage_mv, ret;
>> +
>> + shunt_reg = shunt_registers[channel];
>> + resistance_uo = ina->shunt_resistors[channel];
>> + if (resistance_uo == 0) {
>> + current_ma = 0;
>> + goto resistance_is_futile;
>
> Please no funny label names.
http://i.imgur.com/2cZhiM4.jpg :)
> Also, just make sure that shunt_resistors[]
> is never 0 to avoid having to check the value here.
>
That works, will fix.
>> + }
>> +
>> + ret = ina3221_read_value(ina, shunt_reg, &val);
>> + if (ret)
>> + return ret;
>> + shunt_voltage_mv = val * 40000;
>> +
>> + current_ma = DIV_ROUND_CLOSEST(shunt_voltage_mv, resistance_uo);
>> +resistance_is_futile:
>> + return snprintf(buf, PAGE_SIZE, "%d\n", current_ma);
>> +}
>> +
>> +static ssize_t ina3221_show_power(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
>> + struct ina3221_data *ina = dev_get_drvdata(dev);
>> + unsigned int channel = sd_attr->index;
>> + unsigned int shunt_reg, bus_reg, resistance_uo;
>> + int val, power_uw, current_ma, shunt_voltage_mv, bus_voltage_mv,
>> ret;
>> +
>> + shunt_reg = shunt_registers[channel];
>> + bus_reg = bus_registers[channel];
>> + resistance_uo = ina->shunt_resistors[channel];
>> + if (resistance_uo == 0) {
>> + power_uw = 0;
>> + goto sorry_i_couldnt_resist;
>> + }
>> +
>> + ret = ina3221_read_value(ina, shunt_reg, &val);
>> + if (ret)
>> + return ret;
>> + shunt_voltage_mv = val * 40000;
>> +
>> + current_ma = DIV_ROUND_CLOSEST(shunt_voltage_mv, resistance_uo);
>> +
>> + ret = ina3221_read_value(ina, bus_reg, &val);
>> + if (ret)
>> + return ret;
>> + bus_voltage_mv = val * 8;
>> +
>> + power_uw = current_ma * bus_voltage_mv;
>> +sorry_i_couldnt_resist:
>> + return snprintf(buf, PAGE_SIZE, "%d\n", power_uw);
>> +}
>> +
>> +static ssize_t ina3221_show_shunt(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
>> + struct ina3221_data *ina = dev_get_drvdata(dev);
>> + unsigned int channel = sd_attr->index;
>> + unsigned int resistance_uo;
>> +
>> + resistance_uo = ina->shunt_resistors[channel];
>> +
>> + return snprintf(buf, PAGE_SIZE, "%d\n", resistance_uo);
>> +}
>> +
>> +static ssize_t ina3221_set_shunt(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct sensor_device_attribute *sd_attr = to_sensor_dev_attr(attr);
>> + struct ina3221_data *ina = dev_get_drvdata(dev);
>> + unsigned int channel = sd_attr->index;
>> + int ret;
>> +
>> + ret = kstrtouint(buf, 10, &ina->shunt_resistors[channel]);
>> + if (ret)
>> + return ret;
>> +
> Please don't blindly accept values here. Wrong values should be rejected.
> See ina2xx.c for an example.
>
I'll fix that and post v2 here shortly.
Thanks,
Andrew
Powered by blists - more mailing lists