Message-ID: <570BC9CE.9010905@ti.com>
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, &regval);
>>> +        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], &regval);
>>> +        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],
>>> &reg_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