| 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
| ||
|
Date: Mon, 11 Apr 2016 10:59:10 -0500
From: "Andrew F. Davis" <afd@...com>
To: Guenter Roeck <linux@...ck-us.net>,
Jonathan Cameron <jic23@...nel.org>,
Hartmut Knaack <knaack.h@....de>,
Lars-Peter Clausen <lars@...afoo.de>,
Peter Meerwald <pmeerw@...erw.net>,
Marc Titinger <mtitinger@...libre.com>
CC: <linux-iio@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
Jean Delvare <jdelvare@...e.de>, <linux-hwmon@...r.kernel.org>
Subject: Re: [PATCH 1/1] iio: adc: Add driver for the TI INA3221 Triple
Current/Voltage Monitor
On 04/10/2016 10:16 AM, Guenter Roeck wrote:
> On 04/10/2016 04:57 AM, Jonathan Cameron wrote:
>> On 08/04/16 19:19, Andrew F. Davis wrote:
>>> The INA3221 is a three-channel, high-side current and bus voltage
>>> monitor
>>> with an I2C and SMBUS compatible interface.
>>>
>>> Signed-off-by: Andrew F. Davis <afd@...com>
>> Hi Andrew,
>>
>> My immediate thought on this one is whether it would be better off in
>> hwmon.
>> I'm not convinced either way from a quick glance at the data sheet,
>> but the
>> question needs to be addressed.
>>
>
> It looks like a typical hardware monitoring device to me.
>
>> It's not exactly a clean fit for the IIO ABI either at the moment though
>> I think some elements of that could be easily sorted out.
>> The key think in my mind is to look at what is actually being measured
>> rather than assume all the external components will be as the datasheet
>> assumes them to be...
>>
>> + where do the alert lines actually go?
>>
> In a typical application they would connect to interrupts or to gpio pins.
> They could also trigger some direct action, such as turning on a fan,
> though that is unlikely nowadays. The 'critical' pin might sometimes
> trigger a system reset.
>
> Some more comments inline.
>
> Guenter
>
>> Jonathan
>>> ---
>>> .../ABI/testing/sysfs-bus-iio-adc-ina3221 | 23 ++
>>> drivers/iio/adc/Kconfig | 7 +
>>> drivers/iio/adc/Makefile | 1 +
>>> drivers/iio/adc/ina3221.c | 417
>>> +++++++++++++++++++++
>>> 4 files changed, 448 insertions(+)
>>> create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-ina3221
>>> create mode 100644 drivers/iio/adc/ina3221.c
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-ina3221
>>> b/Documentation/ABI/testing/sysfs-bus-iio-adc-ina3221
>>> new file mode 100644
>>> index 0000000..bbe4f8c
>>> --- /dev/null
>>> +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-ina3221
>>> @@ -0,0 +1,23 @@
>>> +What:
>>> /sys/bus/iio/devices/iio:deviceX/in_voltageY_shunt_(raw|scale)
>>> +What:
>>> /sys/bus/iio/devices/iio:deviceX/in_voltageY_bus_(raw|scale)
>>> +Date: April 2016
>>> +KernelVersion: 4.7
>>> +Contact: Andrew F. Davis <afd@...com>
>>> +Description:
>>> + Shunt and Bus voltage for each channel.
>> Units etc need specifying or at least a reference to the main
>> in_voltageY_raw
>> docs etc. These are really completely separate measurements be it
>> differential measurements where the + on one is the - on the other
>> (second is really a
>> unipolar measurement as it's relative to 0). I wonder if we are
>> better off supporting them as such so define this device as having the
>> channels.
>> in_voltage1-voltage0_raw (shunt voltage 1)
>> in_voltage0_raw (bus voltage 1)
>> in_voltage3-voltage2_raw (shunt voltage 2)
>> in_voltage2_raw (bus voltage 2)
>> in_voltage5-voltage4_raw (shunt voltage 3)
>> in_voltage4_raw
>>
>> That I think reflects the actual measuring options, without applying
>> assumptions on what is connected to them. Yes the datasheet says you
>> should
>> put a shunt between the first two connections and the load between the
>> second connection and the 0 volt line, but I can't see anything
>> preventing
>> this being used differently...
>
> Shunt voltage (or voltage difference) has a LSB of 40uV. Using it for
> anything else but current measurement doesn't really make much sense.
> I would report it not as voltage but as current, but that is from
> my filtered hwmon point of view, so maybe it doesn't really apply here.
>
I would need to know the shunt resistance, I could get this from the
user somehow (DT/sysfs) but I decided to report directly from the ADC
and let the reader do the math so I don't have to make any use assumptions.
>>> +
>>> +What:
>>> /sys/bus/iio/devices/iio:deviceX/shunt_integration_time[_available]
>>> +What:
>>> /sys/bus/iio/devices/iio:deviceX/bus_integration_time[_available]
>>> +Date: April 2016
>>> +KernelVersion: 4.7
>>> +Contact: Andrew F. Davis <afd@...com>
>>> +Description:
>>> + Shunt and Bus integration time for each channel.
>> I'd keep these tightly associated with the channels as then they become
>> standard abi elements, just for channels with extended names.
>>> +
>>> +What:
>>> /sys/bus/iio/devices/iio:deviceX/in_voltageY_shunt_critical_(raw|scale)
>>> +What:
>>> /sys/bus/iio/devices/iio:deviceX/in_voltageY_shunt_warning_(raw|scale)
>>> +Date: April 2016
>>> +KernelVersion: 4.7
>>> +Contact: Andrew F. Davis <afd@...com>
>>> +Description:
>>> + Shunt voltage critical and warning trigger levels.
>> This is why I think this may really belong in hwmon.
>> The way you currently have this done is a bodge on the relevant IIO
>> interfaces.
>> If there is good reason to look at multiple equivalent event types on a
>> given channel in IIO we can look at adding that support to the core.
>> This is a lot more common in explicit hardware monitoring chips than in
>> more general ADCs.
>>
>> Also I see nothing in the driver that is actually detecting if these
>> conditions have been passed? If that is assumed to be a problem for
>> external
>> hardware then it should be clearly documented as such.
>>
>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>> index af4aea7..d713c9c 100644
>>> --- a/drivers/iio/adc/Kconfig
>>> +++ b/drivers/iio/adc/Kconfig
>>> @@ -232,6 +232,13 @@ config IMX7D_ADC
>>> This driver can also be built as a module. If so, the module
>>> will be
>>> called imx7d_adc.
>>>
>>> +config INA3221
>>> + tristate "Texas Instruments INA3221 Triple Power Monitor"
>>> + depends on I2C
>>> + select REGMAP_I2C
>>> + help
>>> + Say yes here to build support for TI INA3221 Triple Power
>>> Monitor.
>> Need more detail here really. Something about what it provides and
>> the name
>> of the module would be conventional.
>>> +
>>> config LP8788_ADC
>>> tristate "LP8788 ADC driver"
>>> depends on MFD_LP8788
>>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>>> index 0cb7921..eebe0c6 100644
>>> --- a/drivers/iio/adc/Makefile
>>> +++ b/drivers/iio/adc/Makefile
>>> @@ -24,6 +24,7 @@ obj-$(CONFIG_FSL_MX25_ADC) += fsl-imx25-gcq.o
>>> obj-$(CONFIG_HI8435) += hi8435.o
>>> obj-$(CONFIG_IMX7D_ADC) += imx7d_adc.o
>>> obj-$(CONFIG_INA2XX_ADC) += ina2xx-adc.o
>>> +obj-$(CONFIG_INA3221) += ina3221.o
>>> obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>>> obj-$(CONFIG_MAX1027) += max1027.o
>>> obj-$(CONFIG_MAX1363) += max1363.o
>>> diff --git a/drivers/iio/adc/ina3221.c b/drivers/iio/adc/ina3221.c
>>> new file mode 100644
>>> index 0000000..e5b9df97
>>> --- /dev/null
>>> +++ b/drivers/iio/adc/ina3221.c
>>> @@ -0,0 +1,417 @@
>>> +/*
>>> + * 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/iio/iio.h>
>>> +#include <linux/iio/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),
>>> +};
>>> +
>>> +#define is_bus_reg(_reg) \
>>> + (_reg == INA3221_BUS1 || \
>>> + _reg == INA3221_BUS2 || \
>>> + _reg == 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];
>>> +};
>>> +
>>> +/**
>>> + * struct ina3221_reg_lookup - value element in iio lookup table map
>>> + * @integer: Integer component of value
>>> + * @fract: Fractional component of value
>>> + */
>>> +struct ina3221_reg_lookup {
>>> + int integer;
>>> + int fract;
>>> +};
>>> +
>>> +static const struct ina3221_reg_lookup ina3221_conv_time_table[] = {
>>> + {.fract = 140}, {.fract = 204}, {.fract = 332}, {.fract = 588},
>>> + {.fract = 1100}, {.fract = 2116}, {.fract = 4156}, {.fract = 8244},
>>> +};
>>> +
>>> +static const int ina3221_avg_table[] = { 1, 4, 16, 64, 128, 256,
>>> 512, 1024 };
>>> +static IIO_CONST_ATTR(oversampling_ratio_available, "1 4 16 64 128
>>> 256 512 1024");
>>> +
>>> +static int ina3221_read_raw(struct iio_dev *indio_dev,
>>> + struct iio_chan_spec const *chan,
>>> + int *val, int *val2, long mask)
>>> +{
>>> + struct ina3221_data *ina = iio_priv(indio_dev);
>>> + unsigned int regval;
>>> + int ret;
>>> +
>>> + switch (mask) {
>>> + case IIO_CHAN_INFO_RAW:
>>> + ret = regmap_read(ina->regmap, chan->address, ®val);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + *val = (s16)sign_extend32(regval >> 3, 12);
>>> +
>>> + return IIO_VAL_INT;
>>> +
>>> + case IIO_CHAN_INFO_SCALE:
>>> + if (is_bus_reg(chan->address)) {
>>> + *val = 8;
>>> + *val2 = 0;
>>> + } else {
>>> + *val = 0;
>>> + *val2 = 40000;
>>> + }
>>> +
>>> + return IIO_VAL_INT_PLUS_MICRO;
>>> +
>>> + case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>>> + ret = regmap_field_read(ina->fields[F_AVG], ®val);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + *val = ina3221_avg_table[regval];
>>> +
>>> + return IIO_VAL_INT;
>>> + }
>>> +
>>> + return -EINVAL;
>>> +}
>>> +
>>> +static int ina3221_write_raw(struct iio_dev *indio_dev,
>>> + struct iio_chan_spec const *chan,
>>> + int val, int val2, long mask)
>>> +{
>>> + struct ina3221_data *ina = iio_priv(indio_dev);
>>> + int i;
>>> +
>>> + switch (mask) {
>>> + case IIO_CHAN_INFO_RAW:
>>> + return regmap_write(ina->regmap, chan->address, val << 3);
>>> +
>>> + case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>>> + if (val2)
>>> + return -EINVAL;
>>> + for (i = 0; i < ARRAY_SIZE(ina3221_avg_table); i++)
>>> + if (ina3221_avg_table[i] == val)
>>> + break;
>>> + if (i == ARRAY_SIZE(ina3221_avg_table))
>>> + return -EINVAL;
>>> +
>>> + return regmap_field_write(ina->fields[F_AVG], i);
>>> + }
>>> +
>>> + return -EINVAL;
>>> +}
>>> +
>>> +#define INA3221_CHAN(_channel, _address, _name) { \
>>> + .type = IIO_VOLTAGE, \
>>> + .channel = (_channel), \
>>> + .address = (_address), \
>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
>>> + BIT(IIO_CHAN_INFO_SCALE), \
>>> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
>>> + .extend_name = _name, \
>>> + .indexed = true, \
>>> +}
>>> +
>>> +static const struct iio_chan_spec ina3221_channels[] = {
>>> + INA3221_CHAN(1, INA3221_SHUNT1, "shunt"),
>>> + INA3221_CHAN(1, INA3221_BUS1, "bus"),
>>> + INA3221_CHAN(1, INA3221_CRIT1, "shunt_critical"),
>>> + INA3221_CHAN(1, INA3221_WARN1, "shunt_warning"),
>>> +
>>> + INA3221_CHAN(2, INA3221_SHUNT2, "shunt"),
>>> + INA3221_CHAN(2, INA3221_BUS2, "bus"),
>>> + INA3221_CHAN(2, INA3221_CRIT2, "shunt_critical"),
>>> + INA3221_CHAN(2, INA3221_WARN2, "shunt_warning"),
>>> +
>>> + INA3221_CHAN(3, INA3221_SHUNT3, "shunt"),
>>> + INA3221_CHAN(3, INA3221_BUS3, "bus"),
>>> + INA3221_CHAN(3, INA3221_CRIT3, "shunt_critical"),
>>> + INA3221_CHAN(3, INA3221_WARN3, "shunt_warning"),
>> I'm not really sure how these work at all... You can set the thresholds
>> but how does the driver know if they have been tripped?
>> Or are we dealing with the assumption that it is a problem for external
>> hardware?
>>
> 'shunt' is really the current reported as voltage. 'bus' is the actual
> voltage. Unless I am missing it, the driver won't know if the thresholds
> have tripped. It would need an interrupt handler and read the status
> register to know that. But that isn't really relevant for an iio driver,
> or is it ? What would it do if the limits are tripped ?
>
I agree, this is really the issue that makes me want to go hwmod side
with this part, although the interrupts could be made to trigger an
IIO_EVENT.
>>> +};
>>> +
>>> +struct ina3221_attr {
>>> + struct device_attribute dev_attr;
>>> + struct device_attribute dev_attr_available;
>>> + unsigned int field;
>>> + const struct ina3221_reg_lookup *table;
>>> + unsigned int table_size;
>>> +};
>>> +
>>> +#define to_ina3221_attr(_dev_attr) \
>>> + container_of(_dev_attr, struct ina3221_attr, dev_attr)
>>> +
>>> +#define to_ina3221_attr_available(_dev_attr) \
>>> + container_of(_dev_attr, struct ina3221_attr, dev_attr_available)
>>> +
>>> +static ssize_t ina3221_show_register(struct device *dev,
>>> + struct device_attribute *attr,
>>> + char *buf)
>>> +{
>>> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>> + struct ina3221_data *ina = iio_priv(indio_dev);
>>> + struct ina3221_attr *ina3221_attr = to_ina3221_attr(attr);
>>> + unsigned int reg_val;
>>> + int vals[2];
>>> + int ret;
>>> +
>>> + ret = regmap_field_read(ina->fields[ina3221_attr->field],
>>> ®_val);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + vals[0] = ina3221_attr->table[reg_val].integer;
>>> + vals[1] = ina3221_attr->table[reg_val].fract;
>>> +
>>> + return iio_format_value(buf, IIO_VAL_INT_PLUS_MICRO, 2, vals);
>>> +}
>>> +
>>> +static ssize_t ina3221_store_register(struct device *dev,
>>> + struct device_attribute *attr,
>>> + const char *buf, size_t count)
>>> +{
>>> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>> + struct ina3221_data *ina = iio_priv(indio_dev);
>>> + struct ina3221_attr *ina3221_attr = to_ina3221_attr(attr);
>>> + long val;
>>> + int integer, fract, ret;
>>> +
>>> + ret = iio_str_to_fixpoint(buf, 100000, &integer, &fract);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + if (integer < 0)
>>> + return -EINVAL;
>>> +
>>> + for (val = 0; val < ina3221_attr->table_size; val++)
>>> + if (ina3221_attr->table[val].integer == integer &&
>>> + ina3221_attr->table[val].fract == fract) {
>>> + ret =
>>> regmap_field_write(ina->fields[ina3221_attr->field], val);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + return count;
>>> + }
>>> +
>>> + return -EINVAL;
>>> +}
>>> +
>>> +static ssize_t ina3221_show_available(struct device *dev,
>>> + struct device_attribute *attr,
>>> + char *buf)
>>> +{
>>> + struct ina3221_attr *ina3221_attr =
>>> to_ina3221_attr_available(attr);
>>> + ssize_t len = 0;
>>> + int i;
>>> +
>>> + for (i = 0; i < ina3221_attr->table_size; i++)
>>> + len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%06u ",
>>> + ina3221_attr->table[i].integer,
>>> + ina3221_attr->table[i].fract);
>>> +
>>> + if (len > 0)
>>> + buf[len - 1] = '\n';
>>> +
>>> + return len;
>>> +}
>>> +
>>> +#define INA3221_ATTR(_name, _field, _table) \
>>> + struct ina3221_attr ina3221_attr_##_name = { \
>>> + .dev_attr = __ATTR(_name, (S_IRUGO | S_IWUSR), \
>>> + ina3221_show_register, \
>>> + ina3221_store_register), \
>>> + .dev_attr_available = __ATTR(_name##_available, S_IRUGO, \
>>> + ina3221_show_available, NULL), \
>>> + .field = _field, \
>>> + .table = _table, \
>>> + .table_size = ARRAY_SIZE(_table), \
>>> + }
>>> +
>>> +static INA3221_ATTR(shunt_integration_time, F_SHUNT_CT,
>>> ina3221_conv_time_table);
>>> +static INA3221_ATTR(bus_integration_time, F_BUS_CT,
>>> ina3221_conv_time_table);
>>> +
>>> +static struct attribute *ina3221_attributes[] = {
>>> + &ina3221_attr_shunt_integration_time.dev_attr.attr,
>>> + &ina3221_attr_shunt_integration_time.dev_attr_available.attr,
>>> + &ina3221_attr_bus_integration_time.dev_attr.attr,
>>> + &ina3221_attr_bus_integration_time.dev_attr_available.attr,
>>> + &iio_const_attr_oversampling_ratio_available.dev_attr.attr,
>>> + NULL,
>>> +};
>>> +
>>> +static const struct attribute_group ina3221_attribute_group = {
>>> + .attrs = ina3221_attributes,
>>> +};
>>> +
>>> +static const struct iio_info ina3221_iio_info = {
>>> + .driver_module = THIS_MODULE,
>>> + .attrs = &ina3221_attribute_group,
>>> + .read_raw = ina3221_read_raw,
>>> + .write_raw = ina3221_write_raw,
>>> +};
>>> +
>>> +static const struct regmap_range ina3221_yes_ranges[] = {
>>> + regmap_reg_range(INA3221_SHUNT1, INA3221_BUS3),
>>> + regmap_reg_range(INA3221_MASK_ENABLE, INA3221_MASK_ENABLE),
>>> +};
>>> +
>>> +static const struct regmap_access_table ina3221_volatile_table = {
>>> + .yes_ranges = ina3221_yes_ranges,
>>> + .n_yes_ranges = ARRAY_SIZE(ina3221_yes_ranges),
>>> +};
>>> +
>>> +static const struct regmap_config ina3221_regmap_config = {
>>> + .reg_bits = 8,
>>> + .val_bits = 16,
>>> +
>>> + .cache_type = REGCACHE_RBTREE,
>>> + .volatile_table = &ina3221_volatile_table,
>>> +};
>>> +
>>> +#ifdef CONFIG_OF
>>> +static const struct of_device_id ina3221_of_match[] = {
>>> + { .compatible = "ti,ina3221", },
>>> + { /* sentinel */ }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, ina3221_of_match);
>>> +#endif
>>> +
>>> +static int ina3221_probe(struct i2c_client *client,
>>> + const struct i2c_device_id *id)
>>> +{
>>> + struct iio_dev *indio_dev;
>>> + struct ina3221_data *ina;
>>> + int i, ret;
>>> +
>>> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*ina));
>>> + if (!indio_dev)
>>> + return -ENOMEM;
>>> + i2c_set_clientdata(client, indio_dev);
>>> +
>>> + ina = iio_priv(indio_dev);
>>> +
>>> + ina->dev = &client->dev;
>>> +
>>> + ina->regmap = devm_regmap_init_i2c(client, &ina3221_regmap_config);
>>> + if (IS_ERR(ina->regmap)) {
>>> + dev_err(ina->dev, "Unable to allocate register map\n");
>>> + return PTR_ERR(ina->regmap);
>>> + }
>>> +
>>> + for (i = 0; i < F_MAX_FIELDS; i++) {
>>> + ina->fields[i] = devm_regmap_field_alloc(ina->dev,
>>> + ina->regmap,
>>> + ina3221_reg_fields[i]);
>>> + if (IS_ERR(ina->fields[i])) {
>>> + dev_err(ina->dev, "Unable to allocate regmap fields\n");
>>> + return PTR_ERR(ina->fields[i]);
>>> + }
>>> + }
>>> +
>>> + ret = regmap_field_write(ina->fields[F_RST], true);
>>> + if (ret) {
>>> + dev_err(ina->dev, "Unable to reset device\n");
>>> + return ret;
>>> + }
>>> +
>>> + indio_dev->modes = INDIO_DIRECT_MODE;
>>> + indio_dev->dev.parent = ina->dev;
>>> + indio_dev->channels = ina3221_channels;
>>> + indio_dev->num_channels = ARRAY_SIZE(ina3221_channels);
>>> + indio_dev->name = INA3221_DRIVER_NAME;
>>> + indio_dev->info = &ina3221_iio_info;
>>> +
>>> + ret = devm_iio_device_register(ina->dev, indio_dev);
>>> + if (ret) {
>>> + dev_err(ina->dev, "Unable to register IIO device\n");
>>> + return ret;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct i2c_device_id ina3221_ids[] = {
>>> + { "ina3221", 0 },
>>> + { /* sentinel */ }
>>> +};
>>> +MODULE_DEVICE_TABLE(i2c, ina3221_ids);
>>> +
>>> +static struct i2c_driver ina3221_i2c_driver = {
>>> + .driver = {
>>> + .name = INA3221_DRIVER_NAME,
>>> + .of_match_table = of_match_ptr(ina3221_of_match),
>
> Is that really necessary ? I don't see any chip specific properties
> Having said that, specifying shunt resistor values might be useful,
> but since the driver reports it as voltage it would not really
> add any value.
>
Not necessary, I just left it in incase I did want to add a shunt
resistor value to DT like the ina2xx driver. I'll remove this until then.
Andrew
>>> + },
>>> + .probe = ina3221_probe,
>>> + .id_table = ina3221_ids,
>>> +};
>>> +module_i2c_driver(ina3221_i2c_driver);
>>> +
>>> +MODULE_AUTHOR("Andrew F. Davis <afd@...com>");
>>> +MODULE_DESCRIPTION("Texas Instruments INA3221 Driver");
>>> +MODULE_LICENSE("GPL v2");
>>>
>>
>>
>
Powered by blists - more mailing lists