Re: [PATCH 1/1] iio: adc: Add driver for the TI INA3221 Triple Current/Voltage Monitor

Re: [PATCH 1/1] iio: adc: Add driver for the TI INA3221 Triple Current/Voltage Monitor

[Jonathan Cameron]

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@ti.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'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?

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@ti.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...
> +
> +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@ti.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@ti.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@ti.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?

> +};
> +
> +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),
> +	},
> +	.probe = ina3221_probe,
> +	.id_table = ina3221_ids,
> +};
> +module_i2c_driver(ina3221_i2c_driver);
> +
> +MODULE_AUTHOR("Andrew F. Davis <afd@ti.com>");
> +MODULE_DESCRIPTION("Texas Instruments INA3221 Driver");
> +MODULE_LICENSE("GPL v2");
> 

Re: [PATCH 1/1] iio: adc: Add driver for the TI INA3221 Triple Current/Voltage Monitor

[Guenter Roeck]

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@ti.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@ti.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.

>> +
>> +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@ti.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@ti.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@ti.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 ?

>> +};
>> +
>> +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.

>> +	},
>> +	.probe = ina3221_probe,
>> +	.id_table = ina3221_ids,
>> +};
>> +module_i2c_driver(ina3221_i2c_driver);
>> +
>> +MODULE_AUTHOR("Andrew F. Davis <afd@ti.com>");
>> +MODULE_DESCRIPTION("Texas Instruments INA3221 Driver");
>> +MODULE_LICENSE("GPL v2");
>>
>
>

Re: [PATCH 1/1] iio: adc: Add driver for the TI INA3221 Triple Current/Voltage Monitor

[Andrew F. Davis]

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@ti.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@ti.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@ti.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@ti.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@ti.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@ti.com>");
>>> +MODULE_DESCRIPTION("Texas Instruments INA3221 Driver");
>>> +MODULE_LICENSE("GPL v2");
>>>
>>
>>
> 

Re: [PATCH 1/1] iio: adc: Add driver for the TI INA3221 Triple Current/Voltage Monitor

[Guenter Roeck]

On Mon, Apr 11, 2016 at 10:59:10AM -0500, Andrew F. Davis wrote:
> 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@ti.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@ti.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.
> 
That is why one would specify it in devicetree. The board designers (those
who hopefully help writing the devicetree description of a board) would know.

Guenter

Re: [PATCH 1/1] iio: adc: Add driver for the TI INA3221 Triple Current/Voltage Monitor

[Andrew F. Davis]

On 04/10/2016 06: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@ti.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.
> 

I was on the fence with this also, I was leaning towards hwmod until I
looked onto how the ina2xx driver has been ported to iio. This is almost
the same part but the ina3x has three monitors vs one. In addition it
looks like NVIDIA has written a hwmod driver for this part
(https://github.com/Bogdacutu/STLinux-Kernel/blob/master/drivers/hwmon/ina3221.c)
but then also ported it over to IIO (although doesn't appear to be
upstream ready or ever has been submitted for such)
(https://github.com/SuperPichu/shield-tablet-kernel/blob/master/drivers/staging/iio/meter/ina3221.c)
So I figured this was the way things are moving, but I have no problem
working this as a hwmod driver. The IIO work is already done here, I'll
write the hwmod version also but keep working this, I see no reason we
cant have both if needed. (unless using this and just using iio_hwmod.c
if needed is more acceptable?)

> 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?
> 
> 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@ti.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...

I feel this may become confusing to some, but I have no real objection
to this.

>> +
>> +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@ti.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.

The other option is to have each channel have an integration_time, but
this may give the false impression that they are individually adjustable
when they are actually all linked together, change one, they all change
(of the same type (bus/shunt)).

>> +
>> +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@ti.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.
> 

Agree, we already have threshold events, perhaps a way to set the
threshold for devices like this that have adjustable ones could be
useful, but I see what you mean, why would regular ADCs need an
adjustable threshold?

> 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.
> 

I was thinking about leaving these to the user to do something with,
they may want them tie them to an alert event somehow. Or I could
probably tie them to channel events?

>> 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.

Will add.

[...]

>> +	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?
> 

They just trigger a pin when reached, these can then be hooked to
anything the designer would like, I wasn't sure how to best handle this
so I left them un-wired to anything in this driver, but I can see how
this would make these channels seem out of place. :/



A quick look though hwmod seems to reveal a lot of basic
Voltage/Current/Temp parts, what seems to separates them is hwmon
drivers expose more alerts, perhaps the IIO framework could be expanded
to handle some hwmon like alerts, then the frameworks could move towards
being merged, as opposed to parts having a driver for each framework
depending on use ( not volunteering, just suggesting :) ). I guess there
is little keeping me from simply registering with both frameworks in one
driver...

Andrew

Re: [PATCH 1/1] iio: adc: Add driver for the TI INA3221 Triple Current/Voltage Monitor

[Guenter Roeck]

On Mon, Apr 11, 2016 at 10:48:27AM -0500, Andrew F. Davis wrote:
> On 04/10/2016 06: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@ti.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.
> > 
> 
> I was on the fence with this also, I was leaning towards hwmod until I
> looked onto how the ina2xx driver has been ported to iio. This is almost
> the same part but the ina3x has three monitors vs one. In addition it
> looks like NVIDIA has written a hwmod driver for this part
> (https://github.com/Bogdacutu/STLinux-Kernel/blob/master/drivers/hwmon/ina3221.c)
> but then also ported it over to IIO (although doesn't appear to be
> upstream ready or ever has been submitted for such)
> (https://github.com/SuperPichu/shield-tablet-kernel/blob/master/drivers/staging/iio/meter/ina3221.c)
> So I figured this was the way things are moving, but I have no problem
> working this as a hwmod driver. The IIO work is already done here, I'll
> write the hwmod version also but keep working this, I see no reason we
> cant have both if needed. (unless using this and just using iio_hwmod.c
> if needed is more acceptable?)
> 

You can not have both since they would conflict with each other.
ina2xx has possibly created a bad precedent. I am not inclined to accept
a hwmon driver if an iio driver already exists. If you want an iio driver,
fine with me, but then you (and its users) will have to live with its
limitations when it comes to hardware monitoring.

I don't really mind if things are going all towards iio if that is where
the community wants to go. However, if that is the case, I would suggest
that someone should spend the time to define a clear sense of 'limits'
as well as alert handling in iio, in a way that is exportable to other
subsystems (hwmon and thermal come into mind).

Guenter

Re: [PATCH 1/1] iio: adc: Add driver for the TI INA3221 Triple Current/Voltage Monitor

[Andrew F. Davis]

On 04/11/2016 11:38 AM, Guenter Roeck wrote:
> On Mon, Apr 11, 2016 at 10:48:27AM -0500, Andrew F. Davis wrote:
>> On 04/10/2016 06: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@ti.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.
>>>
>>
>> I was on the fence with this also, I was leaning towards hwmod until I
>> looked onto how the ina2xx driver has been ported to iio. This is almost
>> the same part but the ina3x has three monitors vs one. In addition it
>> looks like NVIDIA has written a hwmod driver for this part
>> (https://github.com/Bogdacutu/STLinux-Kernel/blob/master/drivers/hwmon/ina3221.c)
>> but then also ported it over to IIO (although doesn't appear to be
>> upstream ready or ever has been submitted for such)
>> (https://github.com/SuperPichu/shield-tablet-kernel/blob/master/drivers/staging/iio/meter/ina3221.c)
>> So I figured this was the way things are moving, but I have no problem
>> working this as a hwmod driver. The IIO work is already done here, I'll
>> write the hwmod version also but keep working this, I see no reason we
>> cant have both if needed. (unless using this and just using iio_hwmod.c
>> if needed is more acceptable?)
>>
> 
> You can not have both since they would conflict with each other.
> ina2xx has possibly created a bad precedent. I am not inclined to accept
> a hwmon driver if an iio driver already exists. If you want an iio driver,
> fine with me, but then you (and its users) will have to live with its
> limitations when it comes to hardware monitoring.
> 

Hmm, my plan was an MFD device core, that then could mediate the two
sub-drivers, but I'm not sure about this yet.

> I don't really mind if things are going all towards iio if that is where
> the community wants to go. However, if that is the case, I would suggest
> that someone should spend the time to define a clear sense of 'limits'
> as well as alert handling in iio, in a way that is exportable to other
> subsystems (hwmon and thermal come into mind).
> 

I agree. I think both frameworks offer useful interfaces, so I would
hate to limit users to one, but for now I'll just post the hwmon version
here in a bit and live with that until someone requests the IIO support.

Andrew

Re: [PATCH 1/1] iio: adc: Add driver for the TI INA3221 Triple Current/Voltage Monitor

[Guenter Roeck]

On Mon, Apr 11, 2016 at 11:47:44AM -0500, Andrew F. Davis wrote:
> On 04/11/2016 11:38 AM, Guenter Roeck wrote:
> > On Mon, Apr 11, 2016 at 10:48:27AM -0500, Andrew F. Davis wrote:
> >> On 04/10/2016 06: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@ti.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.
> >>>
> >>
> >> I was on the fence with this also, I was leaning towards hwmod until I
> >> looked onto how the ina2xx driver has been ported to iio. This is almost
> >> the same part but the ina3x has three monitors vs one. In addition it
> >> looks like NVIDIA has written a hwmod driver for this part
> >> (https://github.com/Bogdacutu/STLinux-Kernel/blob/master/drivers/hwmon/ina3221.c)
> >> but then also ported it over to IIO (although doesn't appear to be
> >> upstream ready or ever has been submitted for such)
> >> (https://github.com/SuperPichu/shield-tablet-kernel/blob/master/drivers/staging/iio/meter/ina3221.c)
> >> So I figured this was the way things are moving, but I have no problem
> >> working this as a hwmod driver. The IIO work is already done here, I'll
> >> write the hwmod version also but keep working this, I see no reason we
> >> cant have both if needed. (unless using this and just using iio_hwmod.c
> >> if needed is more acceptable?)
> >>
> > 
> > You can not have both since they would conflict with each other.
> > ina2xx has possibly created a bad precedent. I am not inclined to accept
> > a hwmon driver if an iio driver already exists. If you want an iio driver,
> > fine with me, but then you (and its users) will have to live with its
> > limitations when it comes to hardware monitoring.
> > 
> 
> Hmm, my plan was an MFD device core, that then could mediate the two
> sub-drivers, but I'm not sure about this yet.
> 
mfd is intended to separate multi-function devices, not multiple
linux subsystems accessing the same logical functionality in a chip just
because the subsystems don't have a means to communicate with each other.
I don't think using mfd for this purpose is a good idea; someone would
have to work hard to convince me that it is.

> > I don't really mind if things are going all towards iio if that is where
> > the community wants to go. However, if that is the case, I would suggest
> > that someone should spend the time to define a clear sense of 'limits'
> > as well as alert handling in iio, in a way that is exportable to other
> > subsystems (hwmon and thermal come into mind).
> > 
> 
> I agree. I think both frameworks offer useful interfaces, so I would
> hate to limit users to one, but for now I'll just post the hwmon version
> here in a bit and live with that until someone requests the IIO support.
> 
Ultimately it might make sense to create a hwmon->iio bridge in the hwmon
core (just like it would make sense to create a hwmon->thermal bridge).
This way drivers could be implemented whereever it is more convenient,
and where the primary use case fits best. The basic idea would be for a
hwmon driver to register with a flag such as "register with the iio
subsystem as well".

However, that would require substantial structural changes (or call it
modernization) of the hwmon driver core API. Unfortunately, that is unlikely
to happen anytime soon - I don't have the time, and no one else seems to
show much interest in hwmon lately. So instead of doing that, people end up
writing drivers for the same chip in multiple subsystems, and end up with
limitations one way or another. ina2xx is a perfect example.

Guenter

Re: [PATCH 1/1] iio: adc: Add driver for the TI INA3221 Triple Current/Voltage Monitor

[Marc Titinger]

[-- Attachment #1: Type: text/plain, Size: 4649 bytes --]

Le 11 avr. 2016 8:08 PM, "Guenter Roeck" <linux@roeck-us.net> a écrit :
>
> On Mon, Apr 11, 2016 at 11:47:44AM -0500, Andrew F. Davis wrote:
> > On 04/11/2016 11:38 AM, Guenter Roeck wrote:
> > > On Mon, Apr 11, 2016 at 10:48:27AM -0500, Andrew F. Davis wrote:
> > >> On 04/10/2016 06: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@ti.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.
> > >>>
> > >>
> > >> I was on the fence with this also, I was leaning towards hwmod until
I
> > >> looked onto how the ina2xx driver has been ported to iio. This is
almost
> > >> the same part but the ina3x has three monitors vs one. In addition it
> > >> looks like NVIDIA has written a hwmod driver for this part
> > >> (
https://github.com/Bogdacutu/STLinux-Kernel/blob/master/drivers/hwmon/ina3221.c
)
> > >> but then also ported it over to IIO (although doesn't appear to be
> > >> upstream ready or ever has been submitted for such)
> > >> (
https://github.com/SuperPichu/shield-tablet-kernel/blob/master/drivers/staging/iio/meter/ina3221.c
)
> > >> So I figured this was the way things are moving, but I have no
problem
> > >> working this as a hwmod driver. The IIO work is already done here,
I'll
> > >> write the hwmod version also but keep working this, I see no reason
we
> > >> cant have both if needed. (unless using this and just using
iio_hwmod.c
> > >> if needed is more acceptable?)
> > >>
> > >
> > > You can not have both since they would conflict with each other.
> > > ina2xx has possibly created a bad precedent. I am not inclined to
accept
> > > a hwmon driver if an iio driver already exists. If you want an iio
driver,
> > > fine with me, but then you (and its users) will have to live with its
> > > limitations when it comes to hardware monitoring.
> > >
> >
> > Hmm, my plan was an MFD device core, that then could mediate the two
> > sub-drivers, but I'm not sure about this yet.
> >
> mfd is intended to separate multi-function devices, not multiple
> linux subsystems accessing the same logical functionality in a chip just
> because the subsystems don't have a means to communicate with each other.
> I don't think using mfd for this purpose is a good idea; someone would
> have to work hard to convince me that it is.
>
> > > I don't really mind if things are going all towards iio if that is
where
> > > the community wants to go. However, if that is the case, I would
suggest
> > > that someone should spend the time to define a clear sense of 'limits'
> > > as well as alert handling in iio, in a way that is exportable to other
> > > subsystems (hwmon and thermal come into mind).
> > >
> >
> > I agree. I think both frameworks offer useful interfaces, so I would
> > hate to limit users to one, but for now I'll just post the hwmon version
> > here in a bit and live with that until someone requests the IIO support.
> >
> Ultimately it might make sense to create a hwmon->iio bridge in the hwmon
> core (just like it would make sense to create a hwmon->thermal bridge).
> This way drivers could be implemented whereever it is more convenient,
> and where the primary use case fits best. The basic idea would be for a
> hwmon driver to register with a flag such as "register with the iio
> subsystem as well".
>
> However, that would require substantial structural changes (or call it
> modernization) of the hwmon driver core API. Unfortunately, that is
unlikely
> to happen anytime soon - I don't have the time, and no one else seems to
> show much interest in hwmon lately. So instead of doing that, people end
up
> writing drivers for the same chip in multiple subsystems, and end up with
> limitations one way or another. ina2xx is a perfect example.

Neither versions of the ina2xx support threshold/alert yet so it is not
really a limitation trade-off that motivates the coexistence of both
drivers but the respective userland ecosystem over IIO or hwmon that have
unique features and use cases. IMHO it would make more sense to put effort
in merging into a common driver/lib with a united capability set rather
than bridging, since it is mostly about keeping things tidy.

BR,
Marc

>
> Guenter

[-- Attachment #2: Type: text/html, Size: 5985 bytes --]

Re: [PATCH 1/1] iio: adc: Add driver for the TI INA3221 Triple Current/Voltage Monitor

[jic23]

On 11.04.2016 19:08, Guenter Roeck wrote:
> On Mon, Apr 11, 2016 at 11:47:44AM -0500, Andrew F. Davis wrote:
>> On 04/11/2016 11:38 AM, Guenter Roeck wrote:
>> > On Mon, Apr 11, 2016 at 10:48:27AM -0500, Andrew F. Davis wrote:
>> >> On 04/10/2016 06: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@ti.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.
>> >>>
>> >>
>> >> I was on the fence with this also, I was leaning towards hwmod until I
>> >> looked onto how the ina2xx driver has been ported to iio. This is almost
>> >> the same part but the ina3x has three monitors vs one. In addition it
>> >> looks like NVIDIA has written a hwmod driver for this part
>> >> (https://github.com/Bogdacutu/STLinux-Kernel/blob/master/drivers/hwmon/ina3221.c)
>> >> but then also ported it over to IIO (although doesn't appear to be
>> >> upstream ready or ever has been submitted for such)
>> >> (https://github.com/SuperPichu/shield-tablet-kernel/blob/master/drivers/staging/iio/meter/ina3221.c)
>> >> So I figured this was the way things are moving, but I have no problem
>> >> working this as a hwmod driver. The IIO work is already done here, I'll
>> >> write the hwmod version also but keep working this, I see no reason we
>> >> cant have both if needed. (unless using this and just using iio_hwmod.c
>> >> if needed is more acceptable?)
>> >>
>> >
>> > You can not have both since they would conflict with each other.
>> > ina2xx has possibly created a bad precedent. I am not inclined to accept
>> > a hwmon driver if an iio driver already exists. If you want an iio driver,
>> > fine with me, but then you (and its users) will have to live with its
>> > limitations when it comes to hardware monitoring.
>> >
>> 
>> Hmm, my plan was an MFD device core, that then could mediate the two
>> sub-drivers, but I'm not sure about this yet.
>> 
> mfd is intended to separate multi-function devices, not multiple
> linux subsystems accessing the same logical functionality in a chip 
> just
> because the subsystems don't have a means to communicate with each 
> other.
> I don't think using mfd for this purpose is a good idea; someone would
> have to work hard to convince me that it is.
> 
>> > I don't really mind if things are going all towards iio if that is where
>> > the community wants to go. However, if that is the case, I would suggest
>> > that someone should spend the time to define a clear sense of 'limits'
>> > as well as alert handling in iio, in a way that is exportable to other
>> > subsystems (hwmon and thermal come into mind).
>> >
>> 
>> I agree. I think both frameworks offer useful interfaces, so I would
>> hate to limit users to one, but for now I'll just post the hwmon 
>> version
>> here in a bit and live with that until someone requests the IIO 
>> support.
>> 
> Ultimately it might make sense to create a hwmon->iio bridge in the 
> hwmon
> core (just like it would make sense to create a hwmon->thermal bridge).
> This way drivers could be implemented whereever it is more convenient,
> and where the primary use case fits best. The basic idea would be for a
> hwmon driver to register with a flag such as "register with the iio
> subsystem as well".
> 
> However, that would require substantial structural changes (or call it
> modernization) of the hwmon driver core API. Unfortunately, that is 
> unlikely
> to happen anytime soon - I don't have the time, and no one else seems 
> to
> show much interest in hwmon lately. So instead of doing that, people 
> end up
> writing drivers for the same chip in multiple subsystems, and end up 
> with
> limitations one way or another. ina2xx is a perfect example.
The aim from the IIO side was to take a kind of different approach to 
this.
Original suggest was Mark Brown's (I like to blame him whenever possible 
;).
His usecase was SoC ADCs where one single ADC is commonly used to do a 
whole
load of parallel tasks.  It's not uncommon to have one logic block 
doing,
touchscreen, hwmon and general purpose ADC channels.

Anyhow, what he brought up is that, as we were adding the ability to 
have
other consumers of IIO data in the kernel we can separate the backend 
from
the front end.

The backend :
* Handles the hardware providing any of:

> 
> Guenter

Re: [PATCH 1/1] iio: adc: Add driver for the TI INA3221 Triple Current/Voltage Monitor

[jic23]

On 12.04.2016 09:34, jic23@jic23.retrosnub.co.uk wrote:
> On 11.04.2016 19:08, Guenter Roeck wrote:
>> On Mon, Apr 11, 2016 at 11:47:44AM -0500, Andrew F. Davis wrote:
>>> On 04/11/2016 11:38 AM, Guenter Roeck wrote:
>>> > On Mon, Apr 11, 2016 at 10:48:27AM -0500, Andrew F. Davis wrote:
>>> >> On 04/10/2016 06: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@ti.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.
>>> >>>
>>> >>
>>> >> I was on the fence with this also, I was leaning towards hwmod until I
>>> >> looked onto how the ina2xx driver has been ported to iio. This is almost
>>> >> the same part but the ina3x has three monitors vs one. In addition it
>>> >> looks like NVIDIA has written a hwmod driver for this part
>>> >> (https://github.com/Bogdacutu/STLinux-Kernel/blob/master/drivers/hwmon/ina3221.c)
>>> >> but then also ported it over to IIO (although doesn't appear to be
>>> >> upstream ready or ever has been submitted for such)
>>> >> (https://github.com/SuperPichu/shield-tablet-kernel/blob/master/drivers/staging/iio/meter/ina3221.c)
>>> >> So I figured this was the way things are moving, but I have no problem
>>> >> working this as a hwmod driver. The IIO work is already done here, I'll
>>> >> write the hwmod version also but keep working this, I see no reason we
>>> >> cant have both if needed. (unless using this and just using iio_hwmod.c
>>> >> if needed is more acceptable?)
>>> >>
>>> >
>>> > You can not have both since they would conflict with each other.
>>> > ina2xx has possibly created a bad precedent. I am not inclined to accept
>>> > a hwmon driver if an iio driver already exists. If you want an iio driver,
>>> > fine with me, but then you (and its users) will have to live with its
>>> > limitations when it comes to hardware monitoring.
>>> >
>>> 
>>> Hmm, my plan was an MFD device core, that then could mediate the two
>>> sub-drivers, but I'm not sure about this yet.
>>> 
>> mfd is intended to separate multi-function devices, not multiple
>> linux subsystems accessing the same logical functionality in a chip 
>> just
>> because the subsystems don't have a means to communicate with each 
>> other.
>> I don't think using mfd for this purpose is a good idea; someone would
>> have to work hard to convince me that it is.
>> 
>>> > I don't really mind if things are going all towards iio if that is where
>>> > the community wants to go. However, if that is the case, I would suggest
>>> > that someone should spend the time to define a clear sense of 'limits'
>>> > as well as alert handling in iio, in a way that is exportable to other
>>> > subsystems (hwmon and thermal come into mind).
>>> >
>>> 
>>> I agree. I think both frameworks offer useful interfaces, so I would
>>> hate to limit users to one, but for now I'll just post the hwmon 
>>> version
>>> here in a bit and live with that until someone requests the IIO 
>>> support.
>>> 
>> Ultimately it might make sense to create a hwmon->iio bridge in the 
>> hwmon
>> core (just like it would make sense to create a hwmon->thermal 
>> bridge).
>> This way drivers could be implemented whereever it is more convenient,
>> and where the primary use case fits best. The basic idea would be for 
>> a
>> hwmon driver to register with a flag such as "register with the iio
>> subsystem as well".
>> 
>> However, that would require substantial structural changes (or call it
>> modernization) of the hwmon driver core API. Unfortunately, that is 
>> unlikely
>> to happen anytime soon - I don't have the time, and no one else seems 
>> to
>> show much interest in hwmon lately. So instead of doing that, people 
>> end up
>> writing drivers for the same chip in multiple subsystems, and end up 
>> with
>> limitations one way or another. ina2xx is a perfect example.
> The aim from the IIO side was to take a kind of different approach to 
> this.
> Original suggest was Mark Brown's (I like to blame him whenever 
> possible ;).
> His usecase was SoC ADCs where one single ADC is commonly used to do a 
> whole
> load of parallel tasks.  It's not uncommon to have one logic block 
> doing,
> touchscreen, hwmon and general purpose ADC channels.
> 
> Anyhow, what he brought up is that, as we were adding the ability to 
> have
> other consumers of IIO data in the kernel we can separate the backend 
> from
> the front end.
> 
> The backend :
> * Handles the hardware providing any of:
<sorry, managed to hit a keypress I didn't know about in my webmail>
- polled data access (possibly including faking this when the driver is 
in an
   exclusive push data mode - which is common)
- pushed data access - demuxing to consumer drivers so they only see the 
channels
   they want.
- events (not yet done) - demuxing included.
  * Doesn't provide any userspace interface at all - other than tools to 
create
    soft mappings of channel sets to client drivers.

The frontend (IIO one)
* Userspace side of things
- including say kfifo's for pushing out data.
- sysfs interfaces for polled reads
- event chardevs and buffers etc.

There are more complex cases where the split gets really tricky such as 
devices
with block based user transfers.  Those may never fit into this 
framework and
may effectively always have to have an IIO front end. We can probably 
work out
how to do this in kernel if there is ever a usecase but right now none 
of our other
likely consumers are going to want these data flows!

Thus the IIO front end is at the same level as iio-hwmon, iio-input 
perhaps
iio-thermal etc.

There are clearly several steps to do this. The current iio->hwmon 
bridge
driver was step one.

1) Event support.
2) Bindings that the device tree guys actually like (tricky!) or
3) Userspace configuration of such mappings (configfs probably? - just 
possibly
the media framework stuff...

So to take an extreme case of an typical SoC ADC we'd have data flows 
such as:
* IIO backend receiving channel setup data from IIO front end (which 
channels, events, other stuff)
* IIO backend pushing data to IIO front end
* IIO backend being pulled by IIO front end
* IIO backend pushing out of band events to IIO front end.

* IIO backend receiving channel setup data from INPUT front end (which 
channels
    + configuration - note that we'll need to work out arbitration rules 
if
    incompatible requests are made)
* IIO backend pushing data to the INPUT Front end
* IIO backend being polled by INPUT front end (unusual case probably).
* IIO backend pushing out of band events to INPUT front end (note that 
in input front
   end these will be merged back into the data stream)

* IIO backend receiving channel setup data form HWMON front end (which 
channels etc)
* IIO backend polled by the INPUT front end (usual case here)
* IIO Backend pushing out of band events to HWMON front end (which will 
typically
   cash them or fire off the relevant poll event)

If we keep it the rules simple initially this should be too hard.  Later 
one we have
stuff like different pushed data rates to different drivers to handle.

Making the IIO front end explicitly request which channels it is using 
also allows
for useful info on what is actually being used - thus we can work out if 
a channel
needs to be cached when in push mode (as it has been requested in a form 
that allows
pull and that consumer hasn't currently set up for push).

What fun,

Any volunteers? This was kind of my main target, but these days I get 
very little
actual kernel code written unfortunately...

Jonathan
> 
>> 
>> Guenter

Re: [PATCH 1/1] iio: adc: Add driver for the TI INA3221 Triple Current/Voltage Monitor

[jic23]

On 11.04.2016 16:48, Andrew F. Davis wrote:
> On 04/10/2016 06: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@ti.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.
>> 
> 
> I was on the fence with this also, I was leaning towards hwmod until I
> looked onto how the ina2xx driver has been ported to iio. This is 
> almost
> the same part but the ina3x has three monitors vs one. In addition it
> looks like NVIDIA has written a hwmod driver for this part
> (https://github.com/Bogdacutu/STLinux-Kernel/blob/master/drivers/hwmon/ina3221.c)
> but then also ported it over to IIO (although doesn't appear to be
> upstream ready or ever has been submitted for such)
> (https://github.com/SuperPichu/shield-tablet-kernel/blob/master/drivers/staging/iio/meter/ina3221.c)
> So I figured this was the way things are moving, but I have no problem
> working this as a hwmod driver. The IIO work is already done here, I'll
> write the hwmod version also but keep working this, I see no reason we
> cant have both if needed. (unless using this and just using iio_hwmod.c
> if needed is more acceptable?)
> 
>> 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?
>> 
>> 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@ti.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...
> 
> I feel this may become confusing to some, but I have no real objection
> to this.
Guenter's point that the shunt measurements are really current measures 
may mean it makes
more sense to handle these by allowing say device tree to provide the 
resistance values and
then converting them to a direct current measure.
> 
>>> +
>>> +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@ti.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.
> 
> The other option is to have each channel have an integration_time, but
> this may give the false impression that they are individually 
> adjustable
> when they are actually all linked together, change one, they all change
> (of the same type (bus/shunt)).
Hmm. It is a little fiddly as we don't support grouping by extended name 
like this.
It's far from uncommon to have a set of channel parameters tied together 
and the ABI
allows for this.  Any parameter can change any other.  I think I'd 
rather stay within
the standard abi if at all possible.  Perhaps this will all be cleaned 
up anyway if we
move to having the shunt voltages output as currents?


> 
>>> +
>>> +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@ti.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.
>> 
> 
> Agree, we already have threshold events, perhaps a way to set the
> threshold for devices like this that have adjustable ones could be
> useful, but I see what you mean, why would regular ADCs need an
> adjustable threshold?
> 
I'm a little confused as all the thresholds are adjustable... Issue is 
we
only currently allow one event of a given type per channel - here you 
effectively
had two.

>> 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.
>> 
> 
> I was thinking about leaving these to the user to do something with,
> they may want them tie them to an alert event somehow. Or I could
> probably tie them to channel events?
Channel event I think, but to support this properly we need the ability 
to
have two such thresholds on one channel.
> 
>>> 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.
> 
> Will add.
> 
> [...]
> 
>>> +	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?
>> 
> 
> They just trigger a pin when reached, these can then be hooked to
> anything the designer would like, I wasn't sure how to best handle this
> so I left them un-wired to anything in this driver, but I can see how
> this would make these channels seem out of place. :/
Interesting use case - I wonder if we need a way of making it explicit 
that
an event is controlled, but not received by an IIO driver... hmm.  Maybe
just a case of detecting that we didn't specify the relevant itnerrupt 
line.
> 
> 
> 
> A quick look though hwmod seems to reveal a lot of basic
> Voltage/Current/Temp parts, what seems to separates them is hwmon
> drivers expose more alerts, perhaps the IIO framework could be expanded
> to handle some hwmon like alerts, then the frameworks could move 
> towards
> being merged, as opposed to parts having a driver for each framework
> depending on use ( not volunteering, just suggesting :) ). I guess 
> there
> is little keeping me from simply registering with both frameworks in 
> one
> driver...
I'll expand on this later in the thread (as it seems to have come up)
but the 'plan' with IIO which has gotten rather stalled was to work 
towards
what I've always termed IIO on IIO,  that is IIO userspace side as just
another client of an IIO backend.  Thus you can use the IIO backend with
a bridge (or consumer if you prefer) providing any of the likely front 
ends
without having to have the IIO userspace as well.


> 
> Andrew

Re: [PATCH 1/1] iio: adc: Add driver for the TI INA3221 Triple Current/Voltage Monitor

[Andrew F. Davis]

On 04/12/2016 03:29 AM, jic23@jic23.retrosnub.co.uk wrote:
> On 11.04.2016 16:48, Andrew F. Davis wrote:
>> On 04/10/2016 06: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@ti.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.
>>>
>>
>> I was on the fence with this also, I was leaning towards hwmod until I
>> looked onto how the ina2xx driver has been ported to iio. This is almost
>> the same part but the ina3x has three monitors vs one. In addition it
>> looks like NVIDIA has written a hwmod driver for this part
>> (https://github.com/Bogdacutu/STLinux-Kernel/blob/master/drivers/hwmon/ina3221.c)
>>
>> but then also ported it over to IIO (although doesn't appear to be
>> upstream ready or ever has been submitted for such)
>> (https://github.com/SuperPichu/shield-tablet-kernel/blob/master/drivers/staging/iio/meter/ina3221.c)
>>
>> So I figured this was the way things are moving, but I have no problem
>> working this as a hwmod driver. The IIO work is already done here, I'll
>> write the hwmod version also but keep working this, I see no reason we
>> cant have both if needed. (unless using this and just using iio_hwmod.c
>> if needed is more acceptable?)
>>
>>> 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?
>>>
>>> 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@ti.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...
>>
>> I feel this may become confusing to some, but I have no real objection
>> to this.
> Guenter's point that the shunt measurements are really current measures
> may mean it makes
> more sense to handle these by allowing say device tree to provide the
> resistance values and
> then converting them to a direct current measure.

I think so as well, and yesterday I converted the driver to hwmod with
this in mind: http://www.spinics.net/lists/kernel/msg2231424.html
Here we do the calculation in software, but I feel there may be a reason
it was not performed in hardware to begin with, I'm not sure if there
are use-cases where this math will not be correct (low/high-side wiring
changes or something).

>>
>>>> +
>>>> +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@ti.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.
>>
>> The other option is to have each channel have an integration_time, but
>> this may give the false impression that they are individually adjustable
>> when they are actually all linked together, change one, they all change
>> (of the same type (bus/shunt)).
> Hmm. It is a little fiddly as we don't support grouping by extended name
> like this.
> It's far from uncommon to have a set of channel parameters tied together
> and the ABI
> allows for this.  Any parameter can change any other.  I think I'd
> rather stay within
> the standard abi if at all possible.  Perhaps this will all be cleaned
> up anyway if we
> move to having the shunt voltages output as currents?
> 

That may work.

> 
>>
>>>> +
>>>> +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@ti.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.
>>>
>>
>> Agree, we already have threshold events, perhaps a way to set the
>> threshold for devices like this that have adjustable ones could be
>> useful, but I see what you mean, why would regular ADCs need an
>> adjustable threshold?
>>
> I'm a little confused as all the thresholds are adjustable... Issue is we
> only currently allow one event of a given type per channel - here you
> effectively
> had two.
> 
>>> 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.
>>>
>>
>> I was thinking about leaving these to the user to do something with,
>> they may want them tie them to an alert event somehow. Or I could
>> probably tie them to channel events?
> Channel event I think, but to support this properly we need the ability to
> have two such thresholds on one channel.

This is why I have not attempted interrupt event handling yet, I believe
expanding channel events may be what is needed to start merging hwmon
and iio. Again not volunteering just yet :)

Andrew