[PATCH 0/2] IIO version of INA2xx (followup of related RFC)

[PATCH 0/2] IIO version of INA2xx (followup of related RFC)

[Marc Titinger]

following RFC started in https://lkml.org/lkml/2015/11/10/370
and feed back from https://lkml.org/lkml/2015/11/18/395

 * squash to a single patch that provides DIRECT and SOFTWARE buffer mode.

 * implement INFO_INT_TIME abi for each adc in order to better match how
   the chip works.  This also allows to compute the actual sample freq
   for INFO_SAMP_FREQ, that results from both the averaging ratio and the
   possible integration times.

 * Add an INT_TIME setting for each voltage ADC (default values are compa
   -tible for previous implementations).

 * provide the averaging feature of the chip using the OVERSAMPLING_RATIO
   abi.

 * by default, only issue a new sample value in the buffer when the
   Conversion Ready Flag indicates that a new value is available. The
   capture thread polls slightly faster than the chip-internal sampling
   clock to prevent re-read or skipping of samples.

 * Since this check for CVRF has its cost (i2c xfer), allow for a relaxed
   mode for when re-read or skipping or one sample is not big deal, but
   a faster sampling rate is wanted.

 * remove the calibration INFO, since the driver sets a hardcoded value
   for 'Current_LSB', only RShunt is available as a parameter. No use to
   expose the register to the user.

Why two drivers (hwmon and IIO) for this device ?
------------------------------------------------

 * Hwmon and IIO do not address exactly the same use-cases, while this chip
   can (and is) being used either as a power monitoring feature of a host
   device or as sensor to measure power properties of a target DUT.

 * In the second use-case (probing a DUT) we wish to plot measurements
   over time, display transients, peak values, compute derived metrics
   (like energy). A buffer streaming scheme and remote capabilities
   with libiio seems beneficial.


Marc Titinger (1):
  iio: ina2xx: add support for TI INA2xx Power Monitors
  iio: ina2xx: provide a sysfs parameter to allow async readout of the
    ADCs

 drivers/iio/adc/Kconfig      |  10 +
 drivers/iio/adc/Makefile     |   1 +
 drivers/iio/adc/ina2xx-iio.c | 720 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 731 insertions(+)
 create mode 100644 drivers/iio/adc/ina2xx-iio.c

-- 
1.9.1

[PATCH 1/2] iio: ina2xx: add support for TI INA2xx Power Monitors

[Marc Titinger]

in SOFTWARE buffer mode, a kthread will capture the active scan_elements
into a kfifo, then compute the remaining time until the next capture tick
and do an active wait (udelay).

This will produce a stream of up to fours channels plus a 64bits
timestamps (ns).

Tested with ina226, on BeagleBoneBlack.

Datasheet: http://www.ti.com/lit/gpn/ina226

Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
---
 drivers/iio/adc/Kconfig      |  10 +
 drivers/iio/adc/Makefile     |   1 +
 drivers/iio/adc/ina2xx-iio.c | 684 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 695 insertions(+)
 create mode 100644 drivers/iio/adc/ina2xx-iio.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index eb0cd89..9b87372 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -170,6 +170,16 @@ config EXYNOS_ADC
 	  of SoCs for drivers such as the touchscreen and hwmon to use to share
 	  this resource.
 
+config INA2XX_IIO
+	tristate "Texas Instruments INA2xx Power Monitors IIO driver"
+	depends on I2C
+	select REGMAP_I2C
+	select IIO_BUFFER
+	help
+	  Say yes here to build support for TI INA2xx familly Power Monitors.
+
+	  Note that this is not the existing hwmon interface for this device.
+
 config LP8788_ADC
 	tristate "LP8788 ADC driver"
 	depends on MFD_LP8788
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index a096210..74e4341 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_BERLIN2_ADC) += berlin2-adc.o
 obj-$(CONFIG_DA9150_GPADC) += da9150-gpadc.o
 obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o
 obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
+obj-$(CONFIG_INA2XX_IIO) += ina2xx-iio.o
 obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
 obj-$(CONFIG_MAX1027) += max1027.o
 obj-$(CONFIG_MAX1363) += max1363.o
diff --git a/drivers/iio/adc/ina2xx-iio.c b/drivers/iio/adc/ina2xx-iio.c
new file mode 100644
index 0000000..4a0026c
--- /dev/null
+++ b/drivers/iio/adc/ina2xx-iio.c
@@ -0,0 +1,684 @@
+/*
+ * INA2XX Current and Power Monitors
+ *
+ * Copyright 2015 Baylibre SAS.
+ *
+ * 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.
+ *
+ * Based on linux/drivers/iio/adc/ad7291.c
+ * Copyright 2010-2011 Analog Devices Inc.
+ *
+ * Based on linux/drivers/hwmon/ina2xx.c
+ * Copyright 2012 Lothar Felten <l-felten@ti.com>
+ *
+ * Licensed under the GPL-2 or later.
+ *
+ * IIO driver for INA219-220-226-230-231
+ *
+ * Configurable 7-bit I2C slave address from 0x40 to 0x4F
+ */
+#include <linux/module.h>
+#include <linux/kthread.h>
+#include <linux/delay.h>
+#include <linux/iio/kfifo_buf.h>
+#include <linux/iio/sysfs.h>
+#include <linux/i2c.h>
+#include <linux/regmap.h>
+#include <linux/platform_data/ina2xx.h>
+
+#include <linux/util_macros.h>
+
+/*
+ * INA2XX registers definition
+ */
+/* common register definitions */
+#define INA2XX_CONFIG                   0x00
+#define INA2XX_SHUNT_VOLTAGE            0x01	/* readonly */
+#define INA2XX_BUS_VOLTAGE              0x02	/* readonly */
+#define INA2XX_POWER                    0x03	/* readonly */
+#define INA2XX_CURRENT                  0x04	/* readonly */
+#define INA2XX_CALIBRATION              0x05
+
+#define INA226_ALERT_MASK		0x06
+#define INA266_CVRF			BIT(3)
+
+/* register count */
+#define INA219_REGISTERS                6
+#define INA226_REGISTERS                8
+#define INA2XX_MAX_REGISTERS            8
+
+/* settings - depend on use case */
+#define INA219_CONFIG_DEFAULT           0x399F	/* PGA=8 */
+#define INA226_CONFIG_DEFAULT           0x4327
+#define INA226_DEFAULT_AVG              4
+#define INA226_DEFAULT_IT		1110
+
+#define INA2XX_RSHUNT_DEFAULT           10000
+
+/*
+ * bit mask for reading the averaging setting in the configuration register
+ * FIXME: use regmap_fields.
+ */
+#define INA2XX_MODE_MASK	GENMASK(3, 0)
+
+#define INA226_AVG_MASK		GENMASK(11, 9)
+#define INA226_SHIFT_AVG(val)	((val) << 9)
+
+/* Integration time for VBus */
+#define INA226_ITB_MASK		GENMASK(8, 6)
+#define INA226_SHIFT_ITB(val)	((val) << 6)
+
+/*Integration Time for VShunt */
+#define INA226_ITS_MASK		GENMASK(5, 3)
+#define INA226_SHIFT_ITS(val)	((val) << 3)
+
+
+static bool ina2xx_is_writeable_reg(struct device *dev, unsigned int reg)
+{
+	return (reg == INA2XX_CONFIG) || (reg > INA2XX_CURRENT);
+}
+
+static bool ina2xx_is_volatile_reg(struct device *dev, unsigned int reg)
+{
+	return (reg != INA2XX_CONFIG);
+}
+
+static struct regmap_config ina2xx_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 16,
+
+	.writeable_reg = ina2xx_is_writeable_reg,
+	.volatile_reg = ina2xx_is_volatile_reg,
+};
+
+enum ina2xx_ids { ina219, ina226 };
+
+struct ina2xx_config {
+	u16 config_default;
+	int calibration_factor;
+	int registers;
+	int shunt_div;
+	int bus_voltage_shift;
+	int bus_voltage_lsb;	/* uV */
+	int power_lsb;		/* uW */
+};
+
+struct ina2xx_chip_info {
+	struct regmap *regmap;
+	struct task_struct *task;
+	const struct ina2xx_config *config;
+	struct mutex state_lock;
+	long rshunt;
+	int avg;
+	int itb; /* Bus voltage integration time uS */
+	int its; /* Shunt voltage integration tim uS */
+};
+
+static const struct ina2xx_config ina2xx_config[] = {
+	[ina219] = {
+		    .config_default = INA219_CONFIG_DEFAULT,
+		    .calibration_factor = 40960000,
+		    .registers = INA219_REGISTERS,
+		    .shunt_div = 100,
+		    .bus_voltage_shift = 3,
+		    .bus_voltage_lsb = 4000,
+		    .power_lsb = 20000,
+		    },
+	[ina226] = {
+		    .config_default = INA226_CONFIG_DEFAULT,
+		    .calibration_factor = 5120000,
+		    .registers = INA226_REGISTERS,
+		    .shunt_div = 400,
+		    .bus_voltage_shift = 0,
+		    .bus_voltage_lsb = 1250,
+		    .power_lsb = 25000,
+		    },
+};
+
+static int ina2xx_get_value(struct ina2xx_chip_info *chip, u8 reg,
+			    unsigned int regval, int *val, int *uval)
+{
+	*val = 0;
+
+	switch (reg) {
+	case INA2XX_SHUNT_VOLTAGE:
+		/* signed register */
+		*uval = DIV_ROUND_CLOSEST((s16) regval,
+					  chip->config->shunt_div);
+		return IIO_VAL_INT_PLUS_MICRO;
+
+	case INA2XX_BUS_VOLTAGE:
+		*uval = (regval >> chip->config->bus_voltage_shift)
+			* chip->config->bus_voltage_lsb;
+		*val = *uval/1000000;
+		*uval = *uval % 1000000;
+		return IIO_VAL_INT_PLUS_MICRO;
+
+	case INA2XX_POWER:
+		*uval = regval * chip->config->power_lsb;
+		*val = *uval/1000000;
+		*uval = *uval % 1000000;
+		return IIO_VAL_INT_PLUS_MICRO;
+
+	case INA2XX_CURRENT:
+		/* signed register, LSB=1mA (selected), in mA */
+		*uval = (s16) regval * 1000;
+		return IIO_VAL_INT_PLUS_MICRO;
+
+	default:
+		/* programmer goofed */
+		WARN_ON_ONCE(1);
+	}
+	return -EINVAL;
+}
+
+static int ina2xx_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2, long mask)
+{
+	int ret;
+	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
+	unsigned int regval;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = regmap_read(chip->regmap, chan->address, &regval);
+		if (ret < 0)
+			return ret;
+
+		return ina2xx_get_value(chip, chan->address, regval, val, val2);
+
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		*val = chip->avg;
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_INT_TIME:
+		*val = 0;
+		if (chan->address == INA2XX_SHUNT_VOLTAGE)
+			*val2 = chip->its;
+		else
+			*val2 = chip->itb;
+
+		return IIO_VAL_INT_PLUS_MICRO;
+	/*
+	 * sample freq is read only, it is a consequence of
+	 * 1/AVG*(CT_bus+CT_shunt)
+	 */
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*val = DIV_ROUND_CLOSEST(1000000,
+		       (chip->itb + chip->its) * chip->avg);
+
+		return IIO_VAL_INT;
+
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+/*
+ * Available averaging rates for ina226. The indices correspond with
+ * the bit values expected by the chip (according to the ina226 datasheet,
+ * table 3 AVG bit settings, found at
+ * http://www.ti.com/lit/ds/symlink/ina226.pdf.
+ */
+static const int ina226_avg_tab[] = { 1, 4, 16, 64, 128, 256, 512, 1024 };
+
+static unsigned int ina226_set_average(struct ina2xx_chip_info *chip,
+				       unsigned int val,
+				       unsigned int *config)
+{
+	int bits;
+
+	if (val > 1024 || val < 1)
+		return -EINVAL;
+
+	bits = find_closest(val, ina226_avg_tab,
+			    ARRAY_SIZE(ina226_avg_tab));
+
+	chip->avg = ina226_avg_tab[bits];
+
+	*config &= ~INA226_AVG_MASK;
+	*config |= INA226_SHIFT_AVG(bits) & INA226_AVG_MASK;
+
+	return 0;
+}
+
+/* Conversion times in uS */
+static const int ina226_conv_time_tab[] = { 140, 204, 332, 588, 1100,
+					    2116, 4156, 8244};
+
+static unsigned int ina226_set_itb(struct ina2xx_chip_info *chip,
+				   unsigned int val,
+				   unsigned int *config)
+{
+	int bits;
+
+	if (val > 8244 || val < 140)
+		return -EINVAL;
+
+	bits = find_closest(val, ina226_conv_time_tab,
+			ARRAY_SIZE(ina226_conv_time_tab));
+
+	chip->itb = ina226_conv_time_tab[bits];
+
+	*config &= ~INA226_ITB_MASK;
+	*config |= INA226_SHIFT_ITB(bits) & INA226_ITB_MASK;
+
+	return 0;
+}
+
+static unsigned int ina226_set_its(struct ina2xx_chip_info *chip,
+				   unsigned int val,
+				   unsigned int *config)
+{
+	int bits;
+
+	if (val > 8244 || val < 140)
+		return -EINVAL;
+
+	bits = find_closest(val, ina226_conv_time_tab,
+			ARRAY_SIZE(ina226_conv_time_tab));
+
+	chip->its = ina226_conv_time_tab[bits];
+
+	*config &= ~INA226_ITS_MASK;
+	*config |= INA226_SHIFT_ITS(bits) & INA226_ITS_MASK;
+
+	return 0;
+}
+
+
+static int ina2xx_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int val, int val2, long mask)
+{
+	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
+	int ret = 0;
+	unsigned int config, tmp;
+
+	if (iio_buffer_enabled(indio_dev))
+		return -EBUSY;
+
+	mutex_lock(&chip->state_lock);
+
+	ret = regmap_read(chip->regmap, INA2XX_CONFIG, &config);
+	if (ret < 0)
+		goto _err;
+
+	tmp = config;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		ret = ina226_set_average(chip, val, &tmp);
+		break;
+
+	case IIO_CHAN_INFO_INT_TIME:
+		if (chan->address == INA2XX_SHUNT_VOLTAGE)
+			ret = ina226_set_its(chip, val, &tmp);
+		else
+			ret = ina226_set_itb(chip, val, &tmp);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+	if (!ret && (tmp != config))
+		ret = regmap_write(chip->regmap, INA2XX_CONFIG, tmp);
+_err:
+	mutex_unlock(&chip->state_lock);
+
+	return ret;
+}
+
+
+#define INA2XX_CHAN(_type, _index, _address) { \
+	.type = _type, \
+	.address = _address, \
+	.indexed = 1, \
+	.channel = (_index), \
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+	.info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
+				   BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
+	.scan_index = (_index), \
+	.scan_type = { \
+		.sign = 'u', \
+		.realbits = 16, \
+		.storagebits = 16, \
+	.endianness = IIO_BE, \
+	} \
+}
+
+/*Sampling Freq is a consequence of the integration times of the V channels.*/
+#define INA2XX_CHAN_VOLTAGE(_index, _address) { \
+	.type = IIO_VOLTAGE, \
+	.address = _address, \
+	.indexed = 1, \
+	.channel = (_index), \
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+			      BIT(IIO_CHAN_INFO_INT_TIME), \
+	.scan_index = (_index), \
+	.scan_type = { \
+		.sign = 'u', \
+		.realbits = 16, \
+		.storagebits = 16, \
+	.endianness = IIO_BE, \
+	} \
+}
+
+
+static const struct iio_chan_spec ina2xx_channels[] = {
+	INA2XX_CHAN_VOLTAGE(0, INA2XX_SHUNT_VOLTAGE),
+	INA2XX_CHAN_VOLTAGE(1, INA2XX_BUS_VOLTAGE),
+	INA2XX_CHAN(IIO_CURRENT, 2, INA2XX_CURRENT),
+	INA2XX_CHAN(IIO_POWER, 3, INA2XX_POWER),
+	IIO_CHAN_SOFT_TIMESTAMP(4),
+};
+
+
+static s64 prev_ns;
+
+static int ina2xx_work_buffer(struct iio_dev *indio_dev)
+{
+	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
+	unsigned short data[8];
+	int bit, ret = 0, i = 0;
+	unsigned long buffer_us = 0, elapsed_us = 0;
+	s64 time_a, time_b;
+	unsigned int alert;
+
+	time_a = iio_get_time_ns();
+
+	/*
+	 * Because the timer thread and the chip conversion clock
+	 * are asynchronous, the period difference will eventually
+	 * result in reading V[k-1] again, or skip V[k] at time Tk.
+	 * In order to resync the timer with the conversion process
+	 * we check the ConVersionReadyFlag.
+	 * On hardware that supports using the ALERT pin to toggle a
+	 * GPIO a triggered buffer could be used instead.
+	 * For now, we pay for that extra read of the ALERT register
+	 */
+	do {
+		ret = regmap_read(chip->regmap, INA226_ALERT_MASK,
+				  &alert);
+		if (ret < 0)
+			goto _err;
+
+		alert &= INA266_CVRF;
+		trace_printk("Conversion ready: %d\n", !!alert);
+
+	} while (!alert);
+
+	/*
+	 * Single register reads: bulk_read will not work with ina226
+	 * as there is no auto-increment of the address register for
+	 * data length longer than 16bits.
+	 */
+	for_each_set_bit(bit, indio_dev->active_scan_mask,
+			 indio_dev->masklength) {
+		unsigned int val;
+
+		ret = regmap_read(chip->regmap,
+				  INA2XX_SHUNT_VOLTAGE + bit, &val);
+		if (ret < 0)
+			goto _err;
+
+		data[i++] = val;
+	}
+
+	time_b = iio_get_time_ns();
+
+	iio_push_to_buffers_with_timestamp(indio_dev,
+					   (unsigned int *)data, time_a);
+
+	buffer_us = (unsigned long)(time_b - time_a) / 1000;
+	elapsed_us = (unsigned long)(time_a - prev_ns) / 1000;
+
+	trace_printk("uS: elapsed: %lu, buf: %lu\n", elapsed_us, buffer_us);
+
+	prev_ns = time_a;
+
+_err:
+	return buffer_us;
+};
+
+static int ina2xx_capture_thread(void *data)
+{
+	struct iio_dev *indio_dev = (struct iio_dev *)data;
+	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
+	unsigned int sampling_us = (chip->itb + chip->its) * chip->avg;
+	unsigned long buffer_us;
+
+	/*
+	 * Poll a bit faster than the chip internal Fs, in case
+	 * we wish to sync with the conversion ready flag.
+	 */
+	sampling_us -= 200;
+
+	do {
+		buffer_us = ina2xx_work_buffer(indio_dev);
+
+		if (sampling_us > buffer_us)
+			udelay(sampling_us - buffer_us);
+
+	} while (!kthread_should_stop());
+
+	return 0;
+}
+
+int ina2xx_buffer_enable(struct iio_dev *indio_dev)
+{
+	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
+	unsigned int sampling_us = (chip->itb + chip->its) * chip->avg;
+
+	trace_printk("Enabling buffer w/ scan_mask %02x, freq = %d, avg =%u\n",
+		     (unsigned int)(*indio_dev->active_scan_mask),
+		     1000000/sampling_us, chip->avg);
+
+	trace_printk("Expected work period: %u us\n", sampling_us);
+
+	prev_ns = iio_get_time_ns();
+
+	chip->task = kthread_run(ina2xx_capture_thread, (void *)indio_dev,
+				 "ina2xx-%uus", sampling_us);
+
+	return PTR_ERR_OR_ZERO(chip->task);
+}
+
+int ina2xx_buffer_disable(struct iio_dev *indio_dev)
+{
+	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
+
+	if (chip->task) {
+		kthread_stop(chip->task);
+		chip->task = NULL;
+	}
+	return 0;
+}
+
+static const struct iio_buffer_setup_ops ina2xx_setup_ops = {
+	.postenable = &ina2xx_buffer_enable,
+	.postdisable = &ina2xx_buffer_disable,
+};
+
+static int ina2xx_debug_reg(struct iio_dev *indio_dev,
+			    unsigned reg, unsigned writeval, unsigned *readval)
+{
+	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
+
+	if (!readval)
+		return regmap_write(chip->regmap, reg, writeval);
+
+	return regmap_read(chip->regmap, reg, readval);
+}
+
+/* frequencies matching the cummulated integration times for vshunt and vbus */
+static IIO_CONST_ATTR_INT_TIME_AVAIL("140 204 332 588 1100 2116 4156 8244");
+
+static struct attribute *ina2xx_attributes[] = {
+	&iio_const_attr_integration_time_available.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group ina2xx_attribute_group = {
+	.attrs = ina2xx_attributes,
+};
+
+static const struct iio_info ina2xx_info = {
+	.debugfs_reg_access = &ina2xx_debug_reg,
+	.read_raw = &ina2xx_read_raw,
+	.write_raw = &ina2xx_write_raw,
+	.attrs = &ina2xx_attribute_group,
+	.driver_module = THIS_MODULE,
+};
+
+/* Initialize the configuration and calibration registers. */
+static int ina2xx_init(struct ina2xx_chip_info *chip, unsigned int config)
+{
+	u16 regval;
+	int ret = regmap_write(chip->regmap, INA2XX_CONFIG, config);
+
+	if (ret < 0)
+		return ret;
+	/*
+	 * Set current LSB to 1mA, shunt is in uOhms
+	 * (equation 13 in datasheet). We hardcode a Current_LSB
+	 * of 1.0 x10-6. The only remaining parameter is RShunt
+	 * There is no need to expose the CALIBRATION register
+	 * to the user for now.
+	 */
+	regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor,
+			    chip->rshunt);
+
+	return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval);
+}
+
+static int ina2xx_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	struct ina2xx_chip_info *chip;
+	struct iio_dev *indio_dev;
+	struct iio_buffer *buffer;
+	int ret;
+	unsigned int val;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	chip = iio_priv(indio_dev);
+
+	chip->config = &ina2xx_config[id->driver_data];
+
+	if (of_property_read_u32(client->dev.of_node,
+				 "shunt-resistor", &val) < 0) {
+		struct ina2xx_platform_data *pdata =
+		    dev_get_platdata(&client->dev);
+
+		if (pdata)
+			val = pdata->shunt_uohms;
+		else
+			val = INA2XX_RSHUNT_DEFAULT;
+	}
+
+	if (val <= 0 || val > chip->config->calibration_factor)
+		return -ENODEV;
+
+	chip->rshunt = val;
+
+	ina2xx_regmap_config.max_register = chip->config->registers;
+
+	mutex_init(&chip->state_lock);
+
+	/* this is only used for device removal purposes */
+	i2c_set_clientdata(client, indio_dev);
+
+	indio_dev->name = id->name;
+	indio_dev->channels = ina2xx_channels;
+	indio_dev->num_channels = ARRAY_SIZE(ina2xx_channels);
+
+	indio_dev->dev.parent = &client->dev;
+	indio_dev->info = &ina2xx_info;
+	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
+
+	chip->regmap = devm_regmap_init_i2c(client, &ina2xx_regmap_config);
+	if (IS_ERR(chip->regmap)) {
+		dev_err(&client->dev, "failed to allocate register map\n");
+		return PTR_ERR(chip->regmap);
+	}
+
+	/* Patch the current config register with default. */
+	val = chip->config->config_default;
+
+	if (id->driver_data == ina226) {
+		ina226_set_average(chip, INA226_DEFAULT_AVG, &val);
+		ina226_set_itb(chip, INA226_DEFAULT_IT, &val);
+		ina226_set_its(chip, INA226_DEFAULT_IT, &val);
+	}
+
+	ret = ina2xx_init(chip, val);
+	if (ret < 0) {
+		dev_err(&client->dev, "error configuring the device: %d\n",
+			ret);
+		return -ENODEV;
+	}
+
+	buffer = devm_iio_kfifo_allocate(&indio_dev->dev);
+	if (!buffer)
+		return -ENOMEM;
+
+	indio_dev->setup_ops = &ina2xx_setup_ops;
+
+	iio_device_attach_buffer(indio_dev, buffer);
+
+	return devm_iio_device_register(&client->dev, indio_dev);
+}
+
+
+static int ina2xx_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
+	int ret;
+
+	mutex_destroy(&chip->state_lock);
+
+	/* Powerdown */
+	ret = regmap_update_bits(chip->regmap, INA2XX_CONFIG,
+				  INA2XX_MODE_MASK, 0);
+
+	iio_device_unregister(indio_dev);
+
+	return ret;
+}
+
+
+static const struct i2c_device_id ina2xx_id[] = {
+	{"ina219", ina219},
+	{"ina220", ina219},
+	{"ina226", ina226},
+	{"ina230", ina226},
+	{"ina231", ina226},
+	{}
+};
+
+MODULE_DEVICE_TABLE(i2c, ina2xx_id);
+
+static struct i2c_driver ina2xx_driver = {
+	.driver = {
+		   .name = KBUILD_MODNAME,
+	},
+	.probe = ina2xx_probe,
+	.remove = ina2xx_remove,
+	.id_table = ina2xx_id,
+};
+
+module_i2c_driver(ina2xx_driver);
+
+MODULE_AUTHOR("Marc Titinger <marc.titinger@baylibre.com>");
+MODULE_DESCRIPTION("Texas Instruments INA2XX ADC driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

Re: [PATCH 1/2] iio: ina2xx: add support for TI INA2xx Power Monitors

[Peter Meerwald-Stadler]

> in SOFTWARE buffer mode, a kthread will capture the active scan_elements
> into a kfifo, then compute the remaining time until the next capture tick
> and do an active wait (udelay).

minor comments below 

> 
> Datasheet: http://www.ti.com/lit/gpn/ina226
> 
> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
> ---
>  drivers/iio/adc/Kconfig      |  10 +
>  drivers/iio/adc/Makefile     |   1 +
>  drivers/iio/adc/ina2xx-iio.c | 684 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 695 insertions(+)
>  create mode 100644 drivers/iio/adc/ina2xx-iio.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index eb0cd89..9b87372 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -170,6 +170,16 @@ config EXYNOS_ADC
>  	  of SoCs for drivers such as the touchscreen and hwmon to use to share
>  	  this resource.
>  
> +config INA2XX_IIO
> +	tristate "Texas Instruments INA2xx Power Monitors IIO driver"
> +	depends on I2C
> +	select REGMAP_I2C
> +	select IIO_BUFFER
> +	help
> +	  Say yes here to build support for TI INA2xx familly Power Monitors.

family

> +
> +	  Note that this is not the existing hwmon interface for this device.

this message not very clear

> +
>  config LP8788_ADC
>  	tristate "LP8788 ADC driver"
>  	depends on MFD_LP8788
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index a096210..74e4341 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -19,6 +19,7 @@ obj-$(CONFIG_BERLIN2_ADC) += berlin2-adc.o
>  obj-$(CONFIG_DA9150_GPADC) += da9150-gpadc.o
>  obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o
>  obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
> +obj-$(CONFIG_INA2XX_IIO) += ina2xx-iio.o
>  obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>  obj-$(CONFIG_MAX1027) += max1027.o
>  obj-$(CONFIG_MAX1363) += max1363.o
> diff --git a/drivers/iio/adc/ina2xx-iio.c b/drivers/iio/adc/ina2xx-iio.c
> new file mode 100644
> index 0000000..4a0026c
> --- /dev/null
> +++ b/drivers/iio/adc/ina2xx-iio.c
> @@ -0,0 +1,684 @@
> +/*
> + * INA2XX Current and Power Monitors
> + *
> + * Copyright 2015 Baylibre SAS.
> + *
> + * 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.
> + *
> + * Based on linux/drivers/iio/adc/ad7291.c
> + * Copyright 2010-2011 Analog Devices Inc.
> + *
> + * Based on linux/drivers/hwmon/ina2xx.c
> + * Copyright 2012 Lothar Felten <l-felten@ti.com>
> + *
> + * Licensed under the GPL-2 or later.
> + *
> + * IIO driver for INA219-220-226-230-231
> + *
> + * Configurable 7-bit I2C slave address from 0x40 to 0x4F
> + */
> +#include <linux/module.h>
> +#include <linux/kthread.h>
> +#include <linux/delay.h>
> +#include <linux/iio/kfifo_buf.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/i2c.h>
> +#include <linux/regmap.h>
> +#include <linux/platform_data/ina2xx.h>
> +
> +#include <linux/util_macros.h>
> +
> +/*
> + * INA2XX registers definition
> + */
> +/* common register definitions */

comment style; do we need both?

> +#define INA2XX_CONFIG                   0x00
> +#define INA2XX_SHUNT_VOLTAGE            0x01	/* readonly */
> +#define INA2XX_BUS_VOLTAGE              0x02	/* readonly */
> +#define INA2XX_POWER                    0x03	/* readonly */
> +#define INA2XX_CURRENT                  0x04	/* readonly */
> +#define INA2XX_CALIBRATION              0x05
> +
> +#define INA226_ALERT_MASK		0x06
> +#define INA266_CVRF			BIT(3)
> +
> +/* register count */
> +#define INA219_REGISTERS                6
> +#define INA226_REGISTERS                8
> +#define INA2XX_MAX_REGISTERS            8
> +
> +/* settings - depend on use case */
> +#define INA219_CONFIG_DEFAULT           0x399F	/* PGA=8 */
> +#define INA226_CONFIG_DEFAULT           0x4327
> +#define INA226_DEFAULT_AVG              4
> +#define INA226_DEFAULT_IT		1110
> +
> +#define INA2XX_RSHUNT_DEFAULT           10000
> +
> +/*
> + * bit mask for reading the averaging setting in the configuration register
> + * FIXME: use regmap_fields.
> + */
> +#define INA2XX_MODE_MASK	GENMASK(3, 0)
> +
> +#define INA226_AVG_MASK		GENMASK(11, 9)
> +#define INA226_SHIFT_AVG(val)	((val) << 9)
> +
> +/* Integration time for VBus */
> +#define INA226_ITB_MASK		GENMASK(8, 6)
> +#define INA226_SHIFT_ITB(val)	((val) << 6)
> +
> +/*Integration Time for VShunt */

time

> +#define INA226_ITS_MASK		GENMASK(5, 3)
> +#define INA226_SHIFT_ITS(val)	((val) << 3)
> +
> +
> +static bool ina2xx_is_writeable_reg(struct device *dev, unsigned int reg)
> +{
> +	return (reg == INA2XX_CONFIG) || (reg > INA2XX_CURRENT);
> +}
> +
> +static bool ina2xx_is_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	return (reg != INA2XX_CONFIG);
> +}
> +
> +static struct regmap_config ina2xx_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 16,
> +
> +	.writeable_reg = ina2xx_is_writeable_reg,
> +	.volatile_reg = ina2xx_is_volatile_reg,
> +};
> +
> +enum ina2xx_ids { ina219, ina226 };
> +
> +struct ina2xx_config {
> +	u16 config_default;
> +	int calibration_factor;
> +	int registers;
> +	int shunt_div;
> +	int bus_voltage_shift;
> +	int bus_voltage_lsb;	/* uV */
> +	int power_lsb;		/* uW */
> +};
> +
> +struct ina2xx_chip_info {
> +	struct regmap *regmap;
> +	struct task_struct *task;
> +	const struct ina2xx_config *config;
> +	struct mutex state_lock;
> +	long rshunt;
> +	int avg;
> +	int itb; /* Bus voltage integration time uS */
> +	int its; /* Shunt voltage integration tim uS */

time

> +};
> +
> +static const struct ina2xx_config ina2xx_config[] = {
> +	[ina219] = {
> +		    .config_default = INA219_CONFIG_DEFAULT,
> +		    .calibration_factor = 40960000,
> +		    .registers = INA219_REGISTERS,
> +		    .shunt_div = 100,
> +		    .bus_voltage_shift = 3,
> +		    .bus_voltage_lsb = 4000,
> +		    .power_lsb = 20000,
> +		    },
> +	[ina226] = {
> +		    .config_default = INA226_CONFIG_DEFAULT,
> +		    .calibration_factor = 5120000,
> +		    .registers = INA226_REGISTERS,
> +		    .shunt_div = 400,
> +		    .bus_voltage_shift = 0,
> +		    .bus_voltage_lsb = 1250,
> +		    .power_lsb = 25000,
> +		    },
> +};
> +
> +static int ina2xx_get_value(struct ina2xx_chip_info *chip, u8 reg,
> +			    unsigned int regval, int *val, int *uval)
> +{
> +	*val = 0;
> +
> +	switch (reg) {
> +	case INA2XX_SHUNT_VOLTAGE:
> +		/* signed register */
> +		*uval = DIV_ROUND_CLOSEST((s16) regval,
> +					  chip->config->shunt_div);
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	case INA2XX_BUS_VOLTAGE:
> +		*uval = (regval >> chip->config->bus_voltage_shift)
> +			* chip->config->bus_voltage_lsb;
> +		*val = *uval/1000000;

whitespace around / please

> +		*uval = *uval % 1000000;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	case INA2XX_POWER:
> +		*uval = regval * chip->config->power_lsb;
> +		*val = *uval/1000000;
> +		*uval = *uval % 1000000;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	case INA2XX_CURRENT:
> +		/* signed register, LSB=1mA (selected), in mA */
> +		*uval = (s16) regval * 1000;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	default:
> +		/* programmer goofed */
> +		WARN_ON_ONCE(1);
> +	}
> +	return -EINVAL;
> +}
> +
> +static int ina2xx_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long mask)
> +{
> +	int ret;
> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> +	unsigned int regval;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = regmap_read(chip->regmap, chan->address, &regval);
> +		if (ret < 0)
> +			return ret;
> +
> +		return ina2xx_get_value(chip, chan->address, regval, val, val2);
> +
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		*val = chip->avg;
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_INT_TIME:
> +		*val = 0;
> +		if (chan->address == INA2XX_SHUNT_VOLTAGE)
> +			*val2 = chip->its;
> +		else
> +			*val2 = chip->itb;
> +
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	/*
> +	 * sample freq is read only, it is a consequence of
> +	 * 1/AVG*(CT_bus+CT_shunt)
> +	 */
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*val = DIV_ROUND_CLOSEST(1000000,
> +		       (chip->itb + chip->its) * chip->avg);
> +
> +		return IIO_VAL_INT;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * Available averaging rates for ina226. The indices correspond with
> + * the bit values expected by the chip (according to the ina226 datasheet,
> + * table 3 AVG bit settings, found at
> + * http://www.ti.com/lit/ds/symlink/ina226.pdf.
> + */
> +static const int ina226_avg_tab[] = { 1, 4, 16, 64, 128, 256, 512, 1024 };
> +
> +static unsigned int ina226_set_average(struct ina2xx_chip_info *chip,

retval should have type int

> +				       unsigned int val,
> +				       unsigned int *config)
> +{
> +	int bits;
> +
> +	if (val > 1024 || val < 1)
> +		return -EINVAL;
> +
> +	bits = find_closest(val, ina226_avg_tab,
> +			    ARRAY_SIZE(ina226_avg_tab));
> +
> +	chip->avg = ina226_avg_tab[bits];
> +
> +	*config &= ~INA226_AVG_MASK;
> +	*config |= INA226_SHIFT_AVG(bits) & INA226_AVG_MASK;
> +
> +	return 0;
> +}
> +
> +/* Conversion times in uS */
> +static const int ina226_conv_time_tab[] = { 140, 204, 332, 588, 1100,
> +					    2116, 4156, 8244};

maybe whitespace before }

> +
> +static unsigned int ina226_set_itb(struct ina2xx_chip_info *chip,
> +				   unsigned int val,
> +				   unsigned int *config)
> +{
> +	int bits;
> +
> +	if (val > 8244 || val < 140)
> +		return -EINVAL;
> +
> +	bits = find_closest(val, ina226_conv_time_tab,
> +			ARRAY_SIZE(ina226_conv_time_tab));
> +
> +	chip->itb = ina226_conv_time_tab[bits];
> +
> +	*config &= ~INA226_ITB_MASK;
> +	*config |= INA226_SHIFT_ITB(bits) & INA226_ITB_MASK;
> +
> +	return 0;
> +}
> +
> +static unsigned int ina226_set_its(struct ina2xx_chip_info *chip,

retval should have type int

> +				   unsigned int val,
> +				   unsigned int *config)
> +{
> +	int bits;
> +
> +	if (val > 8244 || val < 140)
> +		return -EINVAL;
> +
> +	bits = find_closest(val, ina226_conv_time_tab,
> +			ARRAY_SIZE(ina226_conv_time_tab));
> +
> +	chip->its = ina226_conv_time_tab[bits];
> +
> +	*config &= ~INA226_ITS_MASK;
> +	*config |= INA226_SHIFT_ITS(bits) & INA226_ITS_MASK;
> +
> +	return 0;
> +}
> +

drop dup newline

> +
> +static int ina2xx_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int val, int val2, long mask)
> +{
> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> +	int ret = 0;
> +	unsigned int config, tmp;
> +
> +	if (iio_buffer_enabled(indio_dev))
> +		return -EBUSY;
> +
> +	mutex_lock(&chip->state_lock);
> +
> +	ret = regmap_read(chip->regmap, INA2XX_CONFIG, &config);
> +	if (ret < 0)
> +		goto _err;
> +
> +	tmp = config;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		ret = ina226_set_average(chip, val, &tmp);
> +		break;
> +
> +	case IIO_CHAN_INFO_INT_TIME:
> +		if (chan->address == INA2XX_SHUNT_VOLTAGE)
> +			ret = ina226_set_its(chip, val, &tmp);
> +		else
> +			ret = ina226_set_itb(chip, val, &tmp);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +	if (!ret && (tmp != config))
> +		ret = regmap_write(chip->regmap, INA2XX_CONFIG, tmp);
> +_err:
> +	mutex_unlock(&chip->state_lock);
> +
> +	return ret;
> +}
> +
> +
> +#define INA2XX_CHAN(_type, _index, _address) { \
> +	.type = _type, \
> +	.address = _address, \
> +	.indexed = 1, \
> +	.channel = (_index), \
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> +	.info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
> +				   BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
> +	.scan_index = (_index), \
> +	.scan_type = { \
> +		.sign = 'u', \
> +		.realbits = 16, \
> +		.storagebits = 16, \
> +	.endianness = IIO_BE, \
> +	} \
> +}
> +
> +/*Sampling Freq is a consequence of the integration times of the V channels.*/

whitespace after /* and before */ please

> +#define INA2XX_CHAN_VOLTAGE(_index, _address) { \
> +	.type = IIO_VOLTAGE, \
> +	.address = _address, \
> +	.indexed = 1, \
> +	.channel = (_index), \
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> +			      BIT(IIO_CHAN_INFO_INT_TIME), \
> +	.scan_index = (_index), \
> +	.scan_type = { \
> +		.sign = 'u', \
> +		.realbits = 16, \
> +		.storagebits = 16, \
> +	.endianness = IIO_BE, \
> +	} \
> +}
> +

drop dup newline

> +
> +static const struct iio_chan_spec ina2xx_channels[] = {
> +	INA2XX_CHAN_VOLTAGE(0, INA2XX_SHUNT_VOLTAGE),
> +	INA2XX_CHAN_VOLTAGE(1, INA2XX_BUS_VOLTAGE),
> +	INA2XX_CHAN(IIO_CURRENT, 2, INA2XX_CURRENT),
> +	INA2XX_CHAN(IIO_POWER, 3, INA2XX_POWER),
> +	IIO_CHAN_SOFT_TIMESTAMP(4),
> +};
> +
> +
> +static s64 prev_ns;
> +
> +static int ina2xx_work_buffer(struct iio_dev *indio_dev)
> +{
> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> +	unsigned short data[8];
> +	int bit, ret = 0, i = 0;
> +	unsigned long buffer_us = 0, elapsed_us = 0;
> +	s64 time_a, time_b;
> +	unsigned int alert;
> +
> +	time_a = iio_get_time_ns();
> +
> +	/*
> +	 * Because the timer thread and the chip conversion clock
> +	 * are asynchronous, the period difference will eventually
> +	 * result in reading V[k-1] again, or skip V[k] at time Tk.
> +	 * In order to resync the timer with the conversion process
> +	 * we check the ConVersionReadyFlag.
> +	 * On hardware that supports using the ALERT pin to toggle a
> +	 * GPIO a triggered buffer could be used instead.
> +	 * For now, we pay for that extra read of the ALERT register
> +	 */
> +	do {
> +		ret = regmap_read(chip->regmap, INA226_ALERT_MASK,
> +				  &alert);
> +		if (ret < 0)
> +			goto _err;
> +
> +		alert &= INA266_CVRF;
> +		trace_printk("Conversion ready: %d\n", !!alert);
> +
> +	} while (!alert);
> +
> +	/*
> +	 * Single register reads: bulk_read will not work with ina226
> +	 * as there is no auto-increment of the address register for
> +	 * data length longer than 16bits.
> +	 */
> +	for_each_set_bit(bit, indio_dev->active_scan_mask,
> +			 indio_dev->masklength) {
> +		unsigned int val;
> +
> +		ret = regmap_read(chip->regmap,
> +				  INA2XX_SHUNT_VOLTAGE + bit, &val);
> +		if (ret < 0)
> +			goto _err;
> +
> +		data[i++] = val;
> +	}
> +
> +	time_b = iio_get_time_ns();
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev,
> +					   (unsigned int *)data, time_a);
> +
> +	buffer_us = (unsigned long)(time_b - time_a) / 1000;
> +	elapsed_us = (unsigned long)(time_a - prev_ns) / 1000;
> +
> +	trace_printk("uS: elapsed: %lu, buf: %lu\n", elapsed_us, buffer_us);
> +
> +	prev_ns = time_a;
> +
> +_err:
> +	return buffer_us;
> +};
> +
> +static int ina2xx_capture_thread(void *data)
> +{
> +	struct iio_dev *indio_dev = (struct iio_dev *)data;
> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> +	unsigned int sampling_us = (chip->itb + chip->its) * chip->avg;
> +	unsigned long buffer_us;
> +
> +	/*
> +	 * Poll a bit faster than the chip internal Fs, in case
> +	 * we wish to sync with the conversion ready flag.
> +	 */
> +	sampling_us -= 200;
> +
> +	do {
> +		buffer_us = ina2xx_work_buffer(indio_dev);
> +
> +		if (sampling_us > buffer_us)
> +			udelay(sampling_us - buffer_us);
> +
> +	} while (!kthread_should_stop());
> +
> +	return 0;
> +}
> +
> +int ina2xx_buffer_enable(struct iio_dev *indio_dev)
> +{
> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> +	unsigned int sampling_us = (chip->itb + chip->its) * chip->avg;
> +
> +	trace_printk("Enabling buffer w/ scan_mask %02x, freq = %d, avg =%u\n",
> +		     (unsigned int)(*indio_dev->active_scan_mask),
> +		     1000000/sampling_us, chip->avg);
> +
> +	trace_printk("Expected work period: %u us\n", sampling_us);
> +
> +	prev_ns = iio_get_time_ns();
> +
> +	chip->task = kthread_run(ina2xx_capture_thread, (void *)indio_dev,
> +				 "ina2xx-%uus", sampling_us);
> +
> +	return PTR_ERR_OR_ZERO(chip->task);
> +}
> +
> +int ina2xx_buffer_disable(struct iio_dev *indio_dev)
> +{
> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> +
> +	if (chip->task) {
> +		kthread_stop(chip->task);
> +		chip->task = NULL;
> +	}
> +	return 0;
> +}
> +
> +static const struct iio_buffer_setup_ops ina2xx_setup_ops = {
> +	.postenable = &ina2xx_buffer_enable,
> +	.postdisable = &ina2xx_buffer_disable,
> +};
> +
> +static int ina2xx_debug_reg(struct iio_dev *indio_dev,
> +			    unsigned reg, unsigned writeval, unsigned *readval)
> +{
> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> +
> +	if (!readval)
> +		return regmap_write(chip->regmap, reg, writeval);
> +
> +	return regmap_read(chip->regmap, reg, readval);
> +}
> +
> +/* frequencies matching the cummulated integration times for vshunt and vbus */

cumulated

> +static IIO_CONST_ATTR_INT_TIME_AVAIL("140 204 332 588 1100 2116 4156 8244");
> +
> +static struct attribute *ina2xx_attributes[] = {
> +	&iio_const_attr_integration_time_available.dev_attr.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group ina2xx_attribute_group = {
> +	.attrs = ina2xx_attributes,
> +};
> +
> +static const struct iio_info ina2xx_info = {
> +	.debugfs_reg_access = &ina2xx_debug_reg,
> +	.read_raw = &ina2xx_read_raw,
> +	.write_raw = &ina2xx_write_raw,
> +	.attrs = &ina2xx_attribute_group,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +/* Initialize the configuration and calibration registers. */
> +static int ina2xx_init(struct ina2xx_chip_info *chip, unsigned int config)
> +{
> +	u16 regval;
> +	int ret = regmap_write(chip->regmap, INA2XX_CONFIG, config);
> +
> +	if (ret < 0)
> +		return ret;
> +	/*
> +	 * Set current LSB to 1mA, shunt is in uOhms
> +	 * (equation 13 in datasheet). We hardcode a Current_LSB
> +	 * of 1.0 x10-6. The only remaining parameter is RShunt

full stop

> +	 * There is no need to expose the CALIBRATION register
> +	 * to the user for now.
> +	 */
> +	regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor,
> +			    chip->rshunt);
> +
> +	return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval);
> +}
> +
> +static int ina2xx_probe(struct i2c_client *client,
> +			const struct i2c_device_id *id)
> +{
> +	struct ina2xx_chip_info *chip;
> +	struct iio_dev *indio_dev;
> +	struct iio_buffer *buffer;
> +	int ret;
> +	unsigned int val;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	chip = iio_priv(indio_dev);
> +
> +	chip->config = &ina2xx_config[id->driver_data];
> +
> +	if (of_property_read_u32(client->dev.of_node,
> +				 "shunt-resistor", &val) < 0) {
> +		struct ina2xx_platform_data *pdata =
> +		    dev_get_platdata(&client->dev);
> +
> +		if (pdata)
> +			val = pdata->shunt_uohms;
> +		else
> +			val = INA2XX_RSHUNT_DEFAULT;
> +	}
> +
> +	if (val <= 0 || val > chip->config->calibration_factor)
> +		return -ENODEV;
> +
> +	chip->rshunt = val;
> +
> +	ina2xx_regmap_config.max_register = chip->config->registers;
> +
> +	mutex_init(&chip->state_lock);
> +
> +	/* this is only used for device removal purposes */
> +	i2c_set_clientdata(client, indio_dev);
> +
> +	indio_dev->name = id->name;
> +	indio_dev->channels = ina2xx_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(ina2xx_channels);
> +
> +	indio_dev->dev.parent = &client->dev;
> +	indio_dev->info = &ina2xx_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
> +
> +	chip->regmap = devm_regmap_init_i2c(client, &ina2xx_regmap_config);
> +	if (IS_ERR(chip->regmap)) {
> +		dev_err(&client->dev, "failed to allocate register map\n");
> +		return PTR_ERR(chip->regmap);
> +	}
> +
> +	/* Patch the current config register with default. */
> +	val = chip->config->config_default;
> +
> +	if (id->driver_data == ina226) {
> +		ina226_set_average(chip, INA226_DEFAULT_AVG, &val);
> +		ina226_set_itb(chip, INA226_DEFAULT_IT, &val);
> +		ina226_set_its(chip, INA226_DEFAULT_IT, &val);
> +	}
> +
> +	ret = ina2xx_init(chip, val);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "error configuring the device: %d\n",
> +			ret);
> +		return -ENODEV;
> +	}
> +
> +	buffer = devm_iio_kfifo_allocate(&indio_dev->dev);
> +	if (!buffer)
> +		return -ENOMEM;
> +
> +	indio_dev->setup_ops = &ina2xx_setup_ops;
> +
> +	iio_device_attach_buffer(indio_dev, buffer);
> +
> +	return devm_iio_device_register(&client->dev, indio_dev);
> +}
> +
> +
> +static int ina2xx_remove(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> +	int ret;
> +
> +	mutex_destroy(&chip->state_lock);

needed?

> +
> +	/* Powerdown */
> +	ret = regmap_update_bits(chip->regmap, INA2XX_CONFIG,
> +				  INA2XX_MODE_MASK, 0);
> +
> +	iio_device_unregister(indio_dev);

not needed since devm_iio_device_register() is used

> +
> +	return ret;
> +}
> +
> +
> +static const struct i2c_device_id ina2xx_id[] = {
> +	{"ina219", ina219},
> +	{"ina220", ina219},
> +	{"ina226", ina226},
> +	{"ina230", ina226},
> +	{"ina231", ina226},
> +	{}
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, ina2xx_id);
> +
> +static struct i2c_driver ina2xx_driver = {
> +	.driver = {
> +		   .name = KBUILD_MODNAME,
> +	},
> +	.probe = ina2xx_probe,
> +	.remove = ina2xx_remove,
> +	.id_table = ina2xx_id,
> +};
> +
> +module_i2c_driver(ina2xx_driver);
> +
> +MODULE_AUTHOR("Marc Titinger <marc.titinger@baylibre.com>");
> +MODULE_DESCRIPTION("Texas Instruments INA2XX ADC driver");
> +MODULE_LICENSE("GPL v2");
> 

-- 

Peter Meerwald-Stadler
+43-664-2444418 (mobile)

Re: [PATCH 1/2] iio: ina2xx: add support for TI INA2xx Power Monitors

[Marc Titinger]

On 25/11/2015 13:20, Peter Meerwald-Stadler wrote:
>
>> in SOFTWARE buffer mode, a kthread will capture the active scan_elements
>> into a kfifo, then compute the remaining time until the next capture tick
>> and do an active wait (udelay).
>
> minor comments below

Thanks Peter! All fixed in next iteration.

M.
...

>> +config INA2XX_IIO
>> +	tristate "Texas Instruments INA2xx Power Monitors IIO driver"
>> +	depends on I2C
>> +	select REGMAP_I2C
>> +	select IIO_BUFFER
>> +	help
>> +	  Say yes here to build support for TI INA2xx familly Power Monitors.
>
> family
>
>> +
>> +	  Note that this is not the existing hwmon interface for this device.
>
> this message not very clear

removed. This was fuel to discuss the RFC.
...

>> +
>> +/*
>> + * INA2XX registers definition
>> + */
>> +/* common register definitions */
>
> comment style; do we need both?

removed one.

>> +
>> +/*Integration Time for VShunt */
>
> time
>

ok


>> +	int itb; /* Bus voltage integration time uS */
>> +	int its; /* Shunt voltage integration tim uS */
>
> time
>

ok

ge_shift)
>> +			* chip->config->bus_voltage_lsb;
>> +		*val = *uval/1000000;
>
> whitespace around / please

ok



>> +
>> +static unsigned int ina226_set_average(struct ina2xx_chip_info *chip,
>
> retval should have type int

indeed!


>> +static const int ina226_conv_time_tab[] = { 140, 204, 332, 588, 1100,
>> +					    2116, 4156, 8244};
>
> maybe whitespace before }

ok

>> +}
>> +
>> +static unsigned int ina226_set_its(struct ina2xx_chip_info *chip,
>
> retval should have type int

ok


>> +	return 0;
>> +}
>> +
>
> drop dup newline
>

ok


>> +
>> +/*Sampling Freq is a consequence of the integration times of the V channels.*/
>
> whitespace after /* and before */ please
>

ok

>> +#define INA2XX_CHAN_VOLTAGE(_index, _address) { \
>> +	.type = IIO_VOLTAGE, \
>> +	.address = _address, \
>> +	.indexed = 1, \
>> +	.channel = (_index), \
>> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
>> +			      BIT(IIO_CHAN_INFO_INT_TIME), \
>> +	.scan_index = (_index), \
>> +	.scan_type = { \
>> +		.sign = 'u', \
>> +		.realbits = 16, \
>> +		.storagebits = 16, \
>> +	.endianness = IIO_BE, \
>> +	} \
>> +}
>> +
>
> drop dup newline
>

ok


>> +}
>> +
>> +/* frequencies matching the cummulated integration times for vshunt and vbus */
>
> cumulated

wrong comment anyway, fixed.

>> +	 * Set current LSB to 1mA, shunt is in uOhms
>> +	 * (equation 13 in datasheet). We hardcode a Current_LSB
>> +	 * of 1.0 x10-6. The only remaining parameter is RShunt
>
> full stop

ok

>> +	mutex_destroy(&chip->state_lock);
>
> needed?

removed.

>> +
>> +	iio_device_unregister(indio_dev);
>
> not needed since devm_iio_device_register() is used

ok

Re: [PATCH 1/2] iio: ina2xx: add support for TI INA2xx Power Monitors

[Jonathan Cameron]

On 25/11/15 11:28, Marc Titinger wrote:
> in SOFTWARE buffer mode, a kthread will capture the active scan_elements
> into a kfifo, then compute the remaining time until the next capture tick
> and do an active wait (udelay).
> 
> This will produce a stream of up to fours channels plus a 64bits
> timestamps (ns).
> 
> Tested with ina226, on BeagleBoneBlack.
> 
> Datasheet: http://www.ti.com/lit/gpn/ina226
> 
> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
A few more bits from me, but basically looking good.

We do however, need to make the hwmon guys aware this is going on.
Please cc their list on the next version.

Last thing we want is for this to turn up as a surprise!

Jonathan
> ---
>  drivers/iio/adc/Kconfig      |  10 +
>  drivers/iio/adc/Makefile     |   1 +
>  drivers/iio/adc/ina2xx-iio.c | 684 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 695 insertions(+)
>  create mode 100644 drivers/iio/adc/ina2xx-iio.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index eb0cd89..9b87372 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -170,6 +170,16 @@ config EXYNOS_ADC
>  	  of SoCs for drivers such as the touchscreen and hwmon to use to share
>  	  this resource.
>  
> +config INA2XX_IIO
> +	tristate "Texas Instruments INA2xx Power Monitors IIO driver"
> +	depends on I2C
> +	select REGMAP_I2C
> +	select IIO_BUFFER
> +	help
> +	  Say yes here to build support for TI INA2xx familly Power Monitors.
> +
> +	  Note that this is not the existing hwmon interface for this device.
> +
>  config LP8788_ADC
>  	tristate "LP8788 ADC driver"
>  	depends on MFD_LP8788
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index a096210..74e4341 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -19,6 +19,7 @@ obj-$(CONFIG_BERLIN2_ADC) += berlin2-adc.o
>  obj-$(CONFIG_DA9150_GPADC) += da9150-gpadc.o
>  obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o
>  obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
> +obj-$(CONFIG_INA2XX_IIO) += ina2xx-iio.o
>  obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>  obj-$(CONFIG_MAX1027) += max1027.o
>  obj-$(CONFIG_MAX1363) += max1363.o
> diff --git a/drivers/iio/adc/ina2xx-iio.c b/drivers/iio/adc/ina2xx-iio.c
> new file mode 100644
> index 0000000..4a0026c
> --- /dev/null
> +++ b/drivers/iio/adc/ina2xx-iio.c
> @@ -0,0 +1,684 @@
> +/*
> + * INA2XX Current and Power Monitors
> + *
> + * Copyright 2015 Baylibre SAS.
> + *
> + * 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.
> + *
> + * Based on linux/drivers/iio/adc/ad7291.c
> + * Copyright 2010-2011 Analog Devices Inc.
> + *
> + * Based on linux/drivers/hwmon/ina2xx.c
> + * Copyright 2012 Lothar Felten <l-felten@ti.com>
> + *
> + * Licensed under the GPL-2 or later.
> + *
> + * IIO driver for INA219-220-226-230-231
> + *
> + * Configurable 7-bit I2C slave address from 0x40 to 0x4F
> + */
> +#include <linux/module.h>
> +#include <linux/kthread.h>
> +#include <linux/delay.h>
> +#include <linux/iio/kfifo_buf.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/i2c.h>
> +#include <linux/regmap.h>
> +#include <linux/platform_data/ina2xx.h>
> +
> +#include <linux/util_macros.h>
> +
> +/*
> + * INA2XX registers definition
> + */
> +/* common register definitions */
> +#define INA2XX_CONFIG                   0x00
> +#define INA2XX_SHUNT_VOLTAGE            0x01	/* readonly */
> +#define INA2XX_BUS_VOLTAGE              0x02	/* readonly */
> +#define INA2XX_POWER                    0x03	/* readonly */
> +#define INA2XX_CURRENT                  0x04	/* readonly */
> +#define INA2XX_CALIBRATION              0x05
> +
> +#define INA226_ALERT_MASK		0x06
> +#define INA266_CVRF			BIT(3)
> +
> +/* register count */
> +#define INA219_REGISTERS                6
> +#define INA226_REGISTERS                8
> +#define INA2XX_MAX_REGISTERS            8
> +
> +/* settings - depend on use case */
> +#define INA219_CONFIG_DEFAULT           0x399F	/* PGA=8 */
> +#define INA226_CONFIG_DEFAULT           0x4327
> +#define INA226_DEFAULT_AVG              4
> +#define INA226_DEFAULT_IT		1110
> +
> +#define INA2XX_RSHUNT_DEFAULT           10000
> +
> +/*
> + * bit mask for reading the averaging setting in the configuration register
> + * FIXME: use regmap_fields.
> + */
> +#define INA2XX_MODE_MASK	GENMASK(3, 0)
> +
> +#define INA226_AVG_MASK		GENMASK(11, 9)
> +#define INA226_SHIFT_AVG(val)	((val) << 9)
> +
> +/* Integration time for VBus */
> +#define INA226_ITB_MASK		GENMASK(8, 6)
> +#define INA226_SHIFT_ITB(val)	((val) << 6)
> +
> +/*Integration Time for VShunt */
> +#define INA226_ITS_MASK		GENMASK(5, 3)
> +#define INA226_SHIFT_ITS(val)	((val) << 3)
> +
> +
> +static bool ina2xx_is_writeable_reg(struct device *dev, unsigned int reg)
> +{
> +	return (reg == INA2XX_CONFIG) || (reg > INA2XX_CURRENT);
> +}
> +
> +static bool ina2xx_is_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	return (reg != INA2XX_CONFIG);
> +}
> +
> +static struct regmap_config ina2xx_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 16,
> +
> +	.writeable_reg = ina2xx_is_writeable_reg,
> +	.volatile_reg = ina2xx_is_volatile_reg,
> +};
> +
> +enum ina2xx_ids { ina219, ina226 };
> +
> +struct ina2xx_config {
> +	u16 config_default;
> +	int calibration_factor;
> +	int registers;
> +	int shunt_div;
> +	int bus_voltage_shift;
> +	int bus_voltage_lsb;	/* uV */
> +	int power_lsb;		/* uW */
> +};
> +
> +struct ina2xx_chip_info {
> +	struct regmap *regmap;
> +	struct task_struct *task;
> +	const struct ina2xx_config *config;
> +	struct mutex state_lock;
> +	long rshunt;
> +	int avg;
> +	int itb; /* Bus voltage integration time uS */
> +	int its; /* Shunt voltage integration tim uS */
> +};
> +
> +static const struct ina2xx_config ina2xx_config[] = {
> +	[ina219] = {
> +		    .config_default = INA219_CONFIG_DEFAULT,
> +		    .calibration_factor = 40960000,
> +		    .registers = INA219_REGISTERS,
> +		    .shunt_div = 100,
> +		    .bus_voltage_shift = 3,
> +		    .bus_voltage_lsb = 4000,
> +		    .power_lsb = 20000,
> +		    },
> +	[ina226] = {
> +		    .config_default = INA226_CONFIG_DEFAULT,
> +		    .calibration_factor = 5120000,
> +		    .registers = INA226_REGISTERS,
> +		    .shunt_div = 400,
> +		    .bus_voltage_shift = 0,
> +		    .bus_voltage_lsb = 1250,
> +		    .power_lsb = 25000,
> +		    },
> +};
> +
> +static int ina2xx_get_value(struct ina2xx_chip_info *chip, u8 reg,
> +			    unsigned int regval, int *val, int *uval)
> +{
> +	*val = 0;
> +
> +	switch (reg) {
> +	case INA2XX_SHUNT_VOLTAGE:
> +		/* signed register */
> +		*uval = DIV_ROUND_CLOSEST((s16) regval,
> +					  chip->config->shunt_div);
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	case INA2XX_BUS_VOLTAGE:
> +		*uval = (regval >> chip->config->bus_voltage_shift)
> +			* chip->config->bus_voltage_lsb;
> +		*val = *uval/1000000;
> +		*uval = *uval % 1000000;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	case INA2XX_POWER:
> +		*uval = regval * chip->config->power_lsb;
> +		*val = *uval/1000000;
> +		*uval = *uval % 1000000;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	case INA2XX_CURRENT:
> +		/* signed register, LSB=1mA (selected), in mA */
> +		*uval = (s16) regval * 1000;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	default:
> +		/* programmer goofed */
> +		WARN_ON_ONCE(1);
> +	}
> +	return -EINVAL;
> +}
> +
> +static int ina2xx_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long mask)
> +{
> +	int ret;
> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> +	unsigned int regval;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = regmap_read(chip->regmap, chan->address, &regval);
> +		if (ret < 0)
> +			return ret;
> +
> +		return ina2xx_get_value(chip, chan->address, regval, val, val2);
> +
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		*val = chip->avg;
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_INT_TIME:
> +		*val = 0;
> +		if (chan->address == INA2XX_SHUNT_VOLTAGE)
> +			*val2 = chip->its;
> +		else
> +			*val2 = chip->itb;
> +
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	/*
> +	 * sample freq is read only, it is a consequence of
> +	 * 1/AVG*(CT_bus+CT_shunt)
> +	 */
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*val = DIV_ROUND_CLOSEST(1000000,
> +		       (chip->itb + chip->its) * chip->avg);
> +
> +		return IIO_VAL_INT;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * Available averaging rates for ina226. The indices correspond with
> + * the bit values expected by the chip (according to the ina226 datasheet,
> + * table 3 AVG bit settings, found at
> + * http://www.ti.com/lit/ds/symlink/ina226.pdf.
> + */
> +static const int ina226_avg_tab[] = { 1, 4, 16, 64, 128, 256, 512, 1024 };
> +
> +static unsigned int ina226_set_average(struct ina2xx_chip_info *chip,
> +				       unsigned int val,
> +				       unsigned int *config)
> +{
> +	int bits;
> +
> +	if (val > 1024 || val < 1)
> +		return -EINVAL;
> +
> +	bits = find_closest(val, ina226_avg_tab,
> +			    ARRAY_SIZE(ina226_avg_tab));
> +
> +	chip->avg = ina226_avg_tab[bits];
> +
> +	*config &= ~INA226_AVG_MASK;
> +	*config |= INA226_SHIFT_AVG(bits) & INA226_AVG_MASK;
> +
> +	return 0;
> +}
> +
> +/* Conversion times in uS */
> +static const int ina226_conv_time_tab[] = { 140, 204, 332, 588, 1100,
> +					    2116, 4156, 8244};
> +
> +static unsigned int ina226_set_itb(struct ina2xx_chip_info *chip,
> +				   unsigned int val,
> +				   unsigned int *config)
> +{
> +	int bits;
> +
> +	if (val > 8244 || val < 140)
> +		return -EINVAL;
> +
> +	bits = find_closest(val, ina226_conv_time_tab,
> +			ARRAY_SIZE(ina226_conv_time_tab));
> +
> +	chip->itb = ina226_conv_time_tab[bits];
> +
> +	*config &= ~INA226_ITB_MASK;
> +	*config |= INA226_SHIFT_ITB(bits) & INA226_ITB_MASK;
> +
> +	return 0;
> +}
> +
Could do with a slightly more informative name than its vs itb.
Or at least some docs explaining what it does.
> +static unsigned int ina226_set_its(struct ina2xx_chip_info *chip,
> +				   unsigned int val,
> +				   unsigned int *config)
> +{
> +	int bits;
> +
> +	if (val > 8244 || val < 140)
> +		return -EINVAL;
> +
> +	bits = find_closest(val, ina226_conv_time_tab,
> +			ARRAY_SIZE(ina226_conv_time_tab));
> +
> +	chip->its = ina226_conv_time_tab[bits];
> +
> +	*config &= ~INA226_ITS_MASK;
> +	*config |= INA226_SHIFT_ITS(bits) & INA226_ITS_MASK;
> +
> +	return 0;
> +}
> +
> +
> +static int ina2xx_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int val, int val2, long mask)
> +{
> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> +	int ret = 0;
> +	unsigned int config, tmp;
> +
> +	if (iio_buffer_enabled(indio_dev))
> +		return -EBUSY;
> +
> +	mutex_lock(&chip->state_lock);
> +
> +	ret = regmap_read(chip->regmap, INA2XX_CONFIG, &config);
> +	if (ret < 0)
> +		goto _err;
> +
> +	tmp = config;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		ret = ina226_set_average(chip, val, &tmp);
> +		break;
> +
> +	case IIO_CHAN_INFO_INT_TIME:
> +		if (chan->address == INA2XX_SHUNT_VOLTAGE)
> +			ret = ina226_set_its(chip, val, &tmp);
> +		else
> +			ret = ina226_set_itb(chip, val, &tmp);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +	if (!ret && (tmp != config))
> +		ret = regmap_write(chip->regmap, INA2XX_CONFIG, tmp);
> +_err:
> +	mutex_unlock(&chip->state_lock);
> +
> +	return ret;
> +}
> +
> +
> +#define INA2XX_CHAN(_type, _index, _address) { \
> +	.type = _type, \
> +	.address = _address, \
> +	.indexed = 1, \
> +	.channel = (_index), \
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> +	.info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
> +				   BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
> +	.scan_index = (_index), \
> +	.scan_type = { \
> +		.sign = 'u', \
> +		.realbits = 16, \
> +		.storagebits = 16, \
> +	.endianness = IIO_BE, \
> +	} \
> +}
> +
> +/*Sampling Freq is a consequence of the integration times of the V channels.*/
> +#define INA2XX_CHAN_VOLTAGE(_index, _address) { \
> +	.type = IIO_VOLTAGE, \
> +	.address = _address, \
> +	.indexed = 1, \
> +	.channel = (_index), \
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> +			      BIT(IIO_CHAN_INFO_INT_TIME), \
> +	.scan_index = (_index), \
> +	.scan_type = { \
> +		.sign = 'u', \
> +		.realbits = 16, \
> +		.storagebits = 16, \
> +	.endianness = IIO_BE, \
> +	} \
> +}
> +
> +
> +static const struct iio_chan_spec ina2xx_channels[] = {
> +	INA2XX_CHAN_VOLTAGE(0, INA2XX_SHUNT_VOLTAGE),
> +	INA2XX_CHAN_VOLTAGE(1, INA2XX_BUS_VOLTAGE),
> +	INA2XX_CHAN(IIO_CURRENT, 2, INA2XX_CURRENT),
> +	INA2XX_CHAN(IIO_POWER, 3, INA2XX_POWER),
> +	IIO_CHAN_SOFT_TIMESTAMP(4),
> +};
> +
> +
> +static s64 prev_ns;
This prevents you having more than one instance of the device - move it into
your chip_info.
> +
> +static int ina2xx_work_buffer(struct iio_dev *indio_dev)
> +{
> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> +	unsigned short data[8];
> +	int bit, ret = 0, i = 0;
> +	unsigned long buffer_us = 0, elapsed_us = 0;
> +	s64 time_a, time_b;
> +	unsigned int alert;
> +
> +	time_a = iio_get_time_ns();
> +
> +	/*
> +	 * Because the timer thread and the chip conversion clock
> +	 * are asynchronous, the period difference will eventually
> +	 * result in reading V[k-1] again, or skip V[k] at time Tk.
> +	 * In order to resync the timer with the conversion process
> +	 * we check the ConVersionReadyFlag.
> +	 * On hardware that supports using the ALERT pin to toggle a
> +	 * GPIO a triggered buffer could be used instead.
> +	 * For now, we pay for that extra read of the ALERT register
> +	 */
> +	do {
> +		ret = regmap_read(chip->regmap, INA226_ALERT_MASK,
> +				  &alert);
> +		if (ret < 0)
> +			goto _err;
> +
> +		alert &= INA266_CVRF;
> +		trace_printk("Conversion ready: %d\n", !!alert);
> +
> +	} while (!alert);
> +
> +	/*
> +	 * Single register reads: bulk_read will not work with ina226
> +	 * as there is no auto-increment of the address register for
> +	 * data length longer than 16bits.
> +	 */
> +	for_each_set_bit(bit, indio_dev->active_scan_mask,
> +			 indio_dev->masklength) {
> +		unsigned int val;
> +
> +		ret = regmap_read(chip->regmap,
> +				  INA2XX_SHUNT_VOLTAGE + bit, &val);
> +		if (ret < 0)
> +			goto _err;
> +
> +		data[i++] = val;
> +	}
> +
> +	time_b = iio_get_time_ns();
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev,
> +					   (unsigned int *)data, time_a);
> +
> +	buffer_us = (unsigned long)(time_b - time_a) / 1000;
> +	elapsed_us = (unsigned long)(time_a - prev_ns) / 1000;
> +
> +	trace_printk("uS: elapsed: %lu, buf: %lu\n", elapsed_us, buffer_us);
> +
> +	prev_ns = time_a;
> +
> +_err:
> +	return buffer_us;
> +};
> +
> +static int ina2xx_capture_thread(void *data)
> +{
> +	struct iio_dev *indio_dev = (struct iio_dev *)data;
> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> +	unsigned int sampling_us = (chip->itb + chip->its) * chip->avg;
> +	unsigned long buffer_us;
> +
> +	/*
> +	 * Poll a bit faster than the chip internal Fs, in case
> +	 * we wish to sync with the conversion ready flag.
> +	 */
> +	sampling_us -= 200;
> +
> +	do {
> +		buffer_us = ina2xx_work_buffer(indio_dev);
> +
> +		if (sampling_us > buffer_us)
> +			udelay(sampling_us - buffer_us);
> +
> +	} while (!kthread_should_stop());
> +
> +	return 0;
> +}
> +
> +int ina2xx_buffer_enable(struct iio_dev *indio_dev)
> +{
> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> +	unsigned int sampling_us = (chip->itb + chip->its) * chip->avg;
> +
> +	trace_printk("Enabling buffer w/ scan_mask %02x, freq = %d, avg =%u\n",
> +		     (unsigned int)(*indio_dev->active_scan_mask),
> +		     1000000/sampling_us, chip->avg);
> +
> +	trace_printk("Expected work period: %u us\n", sampling_us);
> +
> +	prev_ns = iio_get_time_ns();
> +
> +	chip->task = kthread_run(ina2xx_capture_thread, (void *)indio_dev,
> +				 "ina2xx-%uus", sampling_us);
> +
> +	return PTR_ERR_OR_ZERO(chip->task);
> +}
> +
> +int ina2xx_buffer_disable(struct iio_dev *indio_dev)
> +{
> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> +
> +	if (chip->task) {
> +		kthread_stop(chip->task);
> +		chip->task = NULL;
> +	}
nitpick: blank line here.
> +	return 0;
> +}
> +
> +static const struct iio_buffer_setup_ops ina2xx_setup_ops = {
> +	.postenable = &ina2xx_buffer_enable,
> +	.postdisable = &ina2xx_buffer_disable,
> +};
> +
> +static int ina2xx_debug_reg(struct iio_dev *indio_dev,
> +			    unsigned reg, unsigned writeval, unsigned *readval)
> +{
> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> +
> +	if (!readval)
> +		return regmap_write(chip->regmap, reg, writeval);
> +
> +	return regmap_read(chip->regmap, reg, readval);
> +}
> +
> +/* frequencies matching the cummulated integration times for vshunt and vbus */
huh?  Integration time is in seconds... These seem unlikely to be valid
settings.
> +static IIO_CONST_ATTR_INT_TIME_AVAIL("140 204 332 588 1100 2116 4156 8244");
> +
> +static struct attribute *ina2xx_attributes[] = {
> +	&iio_const_attr_integration_time_available.dev_attr.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group ina2xx_attribute_group = {
> +	.attrs = ina2xx_attributes,
> +};
> +
> +static const struct iio_info ina2xx_info = {
> +	.debugfs_reg_access = &ina2xx_debug_reg,
> +	.read_raw = &ina2xx_read_raw,
> +	.write_raw = &ina2xx_write_raw,
> +	.attrs = &ina2xx_attribute_group,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +/* Initialize the configuration and calibration registers. */
> +static int ina2xx_init(struct ina2xx_chip_info *chip, unsigned int config)
> +{
> +	u16 regval;
> +	int ret = regmap_write(chip->regmap, INA2XX_CONFIG, config);
> +
> +	if (ret < 0)
> +		return ret;
> +	/*
> +	 * Set current LSB to 1mA, shunt is in uOhms
> +	 * (equation 13 in datasheet). We hardcode a Current_LSB
> +	 * of 1.0 x10-6. The only remaining parameter is RShunt
> +	 * There is no need to expose the CALIBRATION register
> +	 * to the user for now.
> +	 */
> +	regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor,
> +			    chip->rshunt);
> +
> +	return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval);
> +}
> +
> +static int ina2xx_probe(struct i2c_client *client,
> +			const struct i2c_device_id *id)
> +{
> +	struct ina2xx_chip_info *chip;
> +	struct iio_dev *indio_dev;
> +	struct iio_buffer *buffer;
> +	int ret;
> +	unsigned int val;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	chip = iio_priv(indio_dev);
> +
> +	chip->config = &ina2xx_config[id->driver_data];
> +
> +	if (of_property_read_u32(client->dev.of_node,
> +				 "shunt-resistor", &val) < 0) {
> +		struct ina2xx_platform_data *pdata =
> +		    dev_get_platdata(&client->dev);
> +
> +		if (pdata)
> +			val = pdata->shunt_uohms;
> +		else
> +			val = INA2XX_RSHUNT_DEFAULT;
> +	}
> +
> +	if (val <= 0 || val > chip->config->calibration_factor)
> +		return -ENODEV;
> +
> +	chip->rshunt = val;
> +
> +	ina2xx_regmap_config.max_register = chip->config->registers;
> +
> +	mutex_init(&chip->state_lock);
> +
> +	/* this is only used for device removal purposes */
> +	i2c_set_clientdata(client, indio_dev);
> +
> +	indio_dev->name = id->name;
> +	indio_dev->channels = ina2xx_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(ina2xx_channels);
> +
> +	indio_dev->dev.parent = &client->dev;
> +	indio_dev->info = &ina2xx_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
> +
> +	chip->regmap = devm_regmap_init_i2c(client, &ina2xx_regmap_config);
> +	if (IS_ERR(chip->regmap)) {
> +		dev_err(&client->dev, "failed to allocate register map\n");
> +		return PTR_ERR(chip->regmap);
> +	}
> +
> +	/* Patch the current config register with default. */
> +	val = chip->config->config_default;
> +
> +	if (id->driver_data == ina226) {
> +		ina226_set_average(chip, INA226_DEFAULT_AVG, &val);
> +		ina226_set_itb(chip, INA226_DEFAULT_IT, &val);
> +		ina226_set_its(chip, INA226_DEFAULT_IT, &val);
> +	}
> +
> +	ret = ina2xx_init(chip, val);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "error configuring the device: %d\n",
> +			ret);
> +		return -ENODEV;
> +	}
> +
> +	buffer = devm_iio_kfifo_allocate(&indio_dev->dev);
> +	if (!buffer)
> +		return -ENOMEM;
> +
> +	indio_dev->setup_ops = &ina2xx_setup_ops;
> +
> +	iio_device_attach_buffer(indio_dev, buffer);
> +
> +	return devm_iio_device_register(&client->dev, indio_dev);
> +}
> +
> +
> +static int ina2xx_remove(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> +	int ret;
> +
> +	mutex_destroy(&chip->state_lock);
> +
> +	/* Powerdown */
> +	ret = regmap_update_bits(chip->regmap, INA2XX_CONFIG,
> +				  INA2XX_MODE_MASK, 0);
> +
> +	iio_device_unregister(indio_dev);
Peter already covered this to a degree.

iio_device_unregister removes the userspace interface - hence it
must be the first thing done in remove.

If you use devm_ version of register it will occur at the end of the
remove.  Hence if the remove has anything in it, you pretty much
can't use the devm_iio_device_register without introducing a nasty
race condition.  Hence you don't want the devm version here and you
want to move iio_device_unregister to the beginning of this function.
> +
> +	return ret;
> +}
> +
> +
> +static const struct i2c_device_id ina2xx_id[] = {
> +	{"ina219", ina219},
> +	{"ina220", ina219},
> +	{"ina226", ina226},
> +	{"ina230", ina226},
> +	{"ina231", ina226},
> +	{}
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, ina2xx_id);
> +
> +static struct i2c_driver ina2xx_driver = {
> +	.driver = {
> +		   .name = KBUILD_MODNAME,
> +	},
> +	.probe = ina2xx_probe,
> +	.remove = ina2xx_remove,
> +	.id_table = ina2xx_id,
> +};
> +
> +module_i2c_driver(ina2xx_driver);
> +
> +MODULE_AUTHOR("Marc Titinger <marc.titinger@baylibre.com>");
> +MODULE_DESCRIPTION("Texas Instruments INA2XX ADC driver");
> +MODULE_LICENSE("GPL v2");
> 

Re: [lm-sensors] [PATCH 1/2] iio: ina2xx: add support for TI INA2xx Power Monitors

[Jonathan Cameron]

On 25/11/15 11:28, Marc Titinger wrote:
> in SOFTWARE buffer mode, a kthread will capture the active scan_elements
> into a kfifo, then compute the remaining time until the next capture tick
> and do an active wait (udelay).
> 
> This will produce a stream of up to fours channels plus a 64bits
> timestamps (ns).
> 
> Tested with ina226, on BeagleBoneBlack.
> 
> Datasheet: http://www.ti.com/lit/gpn/ina226
> 
> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
A few more bits from me, but basically looking good.

We do however, need to make the hwmon guys aware this is going on.
Please cc their list on the next version.

Last thing we want is for this to turn up as a surprise!

Jonathan
> ---
>  drivers/iio/adc/Kconfig      |  10 +
>  drivers/iio/adc/Makefile     |   1 +
>  drivers/iio/adc/ina2xx-iio.c | 684 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 695 insertions(+)
>  create mode 100644 drivers/iio/adc/ina2xx-iio.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index eb0cd89..9b87372 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -170,6 +170,16 @@ config EXYNOS_ADC
>  	  of SoCs for drivers such as the touchscreen and hwmon to use to share
>  	  this resource.
>  
> +config INA2XX_IIO
> +	tristate "Texas Instruments INA2xx Power Monitors IIO driver"
> +	depends on I2C
> +	select REGMAP_I2C
> +	select IIO_BUFFER
> +	help
> +	  Say yes here to build support for TI INA2xx familly Power Monitors.
> +
> +	  Note that this is not the existing hwmon interface for this device.
> +
>  config LP8788_ADC
>  	tristate "LP8788 ADC driver"
>  	depends on MFD_LP8788
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index a096210..74e4341 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -19,6 +19,7 @@ obj-$(CONFIG_BERLIN2_ADC) += berlin2-adc.o
>  obj-$(CONFIG_DA9150_GPADC) += da9150-gpadc.o
>  obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o
>  obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
> +obj-$(CONFIG_INA2XX_IIO) += ina2xx-iio.o
>  obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>  obj-$(CONFIG_MAX1027) += max1027.o
>  obj-$(CONFIG_MAX1363) += max1363.o
> diff --git a/drivers/iio/adc/ina2xx-iio.c b/drivers/iio/adc/ina2xx-iio.c
> new file mode 100644
> index 0000000..4a0026c
> --- /dev/null
> +++ b/drivers/iio/adc/ina2xx-iio.c
> @@ -0,0 +1,684 @@
> +/*
> + * INA2XX Current and Power Monitors
> + *
> + * Copyright 2015 Baylibre SAS.
> + *
> + * 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.
> + *
> + * Based on linux/drivers/iio/adc/ad7291.c
> + * Copyright 2010-2011 Analog Devices Inc.
> + *
> + * Based on linux/drivers/hwmon/ina2xx.c
> + * Copyright 2012 Lothar Felten <l-felten@ti.com>
> + *
> + * Licensed under the GPL-2 or later.
> + *
> + * IIO driver for INA219-220-226-230-231
> + *
> + * Configurable 7-bit I2C slave address from 0x40 to 0x4F
> + */
> +#include <linux/module.h>
> +#include <linux/kthread.h>
> +#include <linux/delay.h>
> +#include <linux/iio/kfifo_buf.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/i2c.h>
> +#include <linux/regmap.h>
> +#include <linux/platform_data/ina2xx.h>
> +
> +#include <linux/util_macros.h>
> +
> +/*
> + * INA2XX registers definition
> + */
> +/* common register definitions */
> +#define INA2XX_CONFIG                   0x00
> +#define INA2XX_SHUNT_VOLTAGE            0x01	/* readonly */
> +#define INA2XX_BUS_VOLTAGE              0x02	/* readonly */
> +#define INA2XX_POWER                    0x03	/* readonly */
> +#define INA2XX_CURRENT                  0x04	/* readonly */
> +#define INA2XX_CALIBRATION              0x05
> +
> +#define INA226_ALERT_MASK		0x06
> +#define INA266_CVRF			BIT(3)
> +
> +/* register count */
> +#define INA219_REGISTERS                6
> +#define INA226_REGISTERS                8
> +#define INA2XX_MAX_REGISTERS            8
> +
> +/* settings - depend on use case */
> +#define INA219_CONFIG_DEFAULT           0x399F	/* PGA=8 */
> +#define INA226_CONFIG_DEFAULT           0x4327
> +#define INA226_DEFAULT_AVG              4
> +#define INA226_DEFAULT_IT		1110
> +
> +#define INA2XX_RSHUNT_DEFAULT           10000
> +
> +/*
> + * bit mask for reading the averaging setting in the configuration register
> + * FIXME: use regmap_fields.
> + */
> +#define INA2XX_MODE_MASK	GENMASK(3, 0)
> +
> +#define INA226_AVG_MASK		GENMASK(11, 9)
> +#define INA226_SHIFT_AVG(val)	((val) << 9)
> +
> +/* Integration time for VBus */
> +#define INA226_ITB_MASK		GENMASK(8, 6)
> +#define INA226_SHIFT_ITB(val)	((val) << 6)
> +
> +/*Integration Time for VShunt */
> +#define INA226_ITS_MASK		GENMASK(5, 3)
> +#define INA226_SHIFT_ITS(val)	((val) << 3)
> +
> +
> +static bool ina2xx_is_writeable_reg(struct device *dev, unsigned int reg)
> +{
> +	return (reg = INA2XX_CONFIG) || (reg > INA2XX_CURRENT);
> +}
> +
> +static bool ina2xx_is_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	return (reg != INA2XX_CONFIG);
> +}
> +
> +static struct regmap_config ina2xx_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 16,
> +
> +	.writeable_reg = ina2xx_is_writeable_reg,
> +	.volatile_reg = ina2xx_is_volatile_reg,
> +};
> +
> +enum ina2xx_ids { ina219, ina226 };
> +
> +struct ina2xx_config {
> +	u16 config_default;
> +	int calibration_factor;
> +	int registers;
> +	int shunt_div;
> +	int bus_voltage_shift;
> +	int bus_voltage_lsb;	/* uV */
> +	int power_lsb;		/* uW */
> +};
> +
> +struct ina2xx_chip_info {
> +	struct regmap *regmap;
> +	struct task_struct *task;
> +	const struct ina2xx_config *config;
> +	struct mutex state_lock;
> +	long rshunt;
> +	int avg;
> +	int itb; /* Bus voltage integration time uS */
> +	int its; /* Shunt voltage integration tim uS */
> +};
> +
> +static const struct ina2xx_config ina2xx_config[] = {
> +	[ina219] = {
> +		    .config_default = INA219_CONFIG_DEFAULT,
> +		    .calibration_factor = 40960000,
> +		    .registers = INA219_REGISTERS,
> +		    .shunt_div = 100,
> +		    .bus_voltage_shift = 3,
> +		    .bus_voltage_lsb = 4000,
> +		    .power_lsb = 20000,
> +		    },
> +	[ina226] = {
> +		    .config_default = INA226_CONFIG_DEFAULT,
> +		    .calibration_factor = 5120000,
> +		    .registers = INA226_REGISTERS,
> +		    .shunt_div = 400,
> +		    .bus_voltage_shift = 0,
> +		    .bus_voltage_lsb = 1250,
> +		    .power_lsb = 25000,
> +		    },
> +};
> +
> +static int ina2xx_get_value(struct ina2xx_chip_info *chip, u8 reg,
> +			    unsigned int regval, int *val, int *uval)
> +{
> +	*val = 0;
> +
> +	switch (reg) {
> +	case INA2XX_SHUNT_VOLTAGE:
> +		/* signed register */
> +		*uval = DIV_ROUND_CLOSEST((s16) regval,
> +					  chip->config->shunt_div);
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	case INA2XX_BUS_VOLTAGE:
> +		*uval = (regval >> chip->config->bus_voltage_shift)
> +			* chip->config->bus_voltage_lsb;
> +		*val = *uval/1000000;
> +		*uval = *uval % 1000000;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	case INA2XX_POWER:
> +		*uval = regval * chip->config->power_lsb;
> +		*val = *uval/1000000;
> +		*uval = *uval % 1000000;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	case INA2XX_CURRENT:
> +		/* signed register, LSB=1mA (selected), in mA */
> +		*uval = (s16) regval * 1000;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	default:
> +		/* programmer goofed */
> +		WARN_ON_ONCE(1);
> +	}
> +	return -EINVAL;
> +}
> +
> +static int ina2xx_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long mask)
> +{
> +	int ret;
> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> +	unsigned int regval;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = regmap_read(chip->regmap, chan->address, &regval);
> +		if (ret < 0)
> +			return ret;
> +
> +		return ina2xx_get_value(chip, chan->address, regval, val, val2);
> +
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		*val = chip->avg;
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_INT_TIME:
> +		*val = 0;
> +		if (chan->address = INA2XX_SHUNT_VOLTAGE)
> +			*val2 = chip->its;
> +		else
> +			*val2 = chip->itb;
> +
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	/*
> +	 * sample freq is read only, it is a consequence of
> +	 * 1/AVG*(CT_bus+CT_shunt)
> +	 */
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*val = DIV_ROUND_CLOSEST(1000000,
> +		       (chip->itb + chip->its) * chip->avg);
> +
> +		return IIO_VAL_INT;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * Available averaging rates for ina226. The indices correspond with
> + * the bit values expected by the chip (according to the ina226 datasheet,
> + * table 3 AVG bit settings, found at
> + * http://www.ti.com/lit/ds/symlink/ina226.pdf.
> + */
> +static const int ina226_avg_tab[] = { 1, 4, 16, 64, 128, 256, 512, 1024 };
> +
> +static unsigned int ina226_set_average(struct ina2xx_chip_info *chip,
> +				       unsigned int val,
> +				       unsigned int *config)
> +{
> +	int bits;
> +
> +	if (val > 1024 || val < 1)
> +		return -EINVAL;
> +
> +	bits = find_closest(val, ina226_avg_tab,
> +			    ARRAY_SIZE(ina226_avg_tab));
> +
> +	chip->avg = ina226_avg_tab[bits];
> +
> +	*config &= ~INA226_AVG_MASK;
> +	*config |= INA226_SHIFT_AVG(bits) & INA226_AVG_MASK;
> +
> +	return 0;
> +}
> +
> +/* Conversion times in uS */
> +static const int ina226_conv_time_tab[] = { 140, 204, 332, 588, 1100,
> +					    2116, 4156, 8244};
> +
> +static unsigned int ina226_set_itb(struct ina2xx_chip_info *chip,
> +				   unsigned int val,
> +				   unsigned int *config)
> +{
> +	int bits;
> +
> +	if (val > 8244 || val < 140)
> +		return -EINVAL;
> +
> +	bits = find_closest(val, ina226_conv_time_tab,
> +			ARRAY_SIZE(ina226_conv_time_tab));
> +
> +	chip->itb = ina226_conv_time_tab[bits];
> +
> +	*config &= ~INA226_ITB_MASK;
> +	*config |= INA226_SHIFT_ITB(bits) & INA226_ITB_MASK;
> +
> +	return 0;
> +}
> +
Could do with a slightly more informative name than its vs itb.
Or at least some docs explaining what it does.
> +static unsigned int ina226_set_its(struct ina2xx_chip_info *chip,
> +				   unsigned int val,
> +				   unsigned int *config)
> +{
> +	int bits;
> +
> +	if (val > 8244 || val < 140)
> +		return -EINVAL;
> +
> +	bits = find_closest(val, ina226_conv_time_tab,
> +			ARRAY_SIZE(ina226_conv_time_tab));
> +
> +	chip->its = ina226_conv_time_tab[bits];
> +
> +	*config &= ~INA226_ITS_MASK;
> +	*config |= INA226_SHIFT_ITS(bits) & INA226_ITS_MASK;
> +
> +	return 0;
> +}
> +
> +
> +static int ina2xx_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int val, int val2, long mask)
> +{
> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> +	int ret = 0;
> +	unsigned int config, tmp;
> +
> +	if (iio_buffer_enabled(indio_dev))
> +		return -EBUSY;
> +
> +	mutex_lock(&chip->state_lock);
> +
> +	ret = regmap_read(chip->regmap, INA2XX_CONFIG, &config);
> +	if (ret < 0)
> +		goto _err;
> +
> +	tmp = config;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		ret = ina226_set_average(chip, val, &tmp);
> +		break;
> +
> +	case IIO_CHAN_INFO_INT_TIME:
> +		if (chan->address = INA2XX_SHUNT_VOLTAGE)
> +			ret = ina226_set_its(chip, val, &tmp);
> +		else
> +			ret = ina226_set_itb(chip, val, &tmp);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +	if (!ret && (tmp != config))
> +		ret = regmap_write(chip->regmap, INA2XX_CONFIG, tmp);
> +_err:
> +	mutex_unlock(&chip->state_lock);
> +
> +	return ret;
> +}
> +
> +
> +#define INA2XX_CHAN(_type, _index, _address) { \
> +	.type = _type, \
> +	.address = _address, \
> +	.indexed = 1, \
> +	.channel = (_index), \
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> +	.info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
> +				   BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
> +	.scan_index = (_index), \
> +	.scan_type = { \
> +		.sign = 'u', \
> +		.realbits = 16, \
> +		.storagebits = 16, \
> +	.endianness = IIO_BE, \
> +	} \
> +}
> +
> +/*Sampling Freq is a consequence of the integration times of the V channels.*/
> +#define INA2XX_CHAN_VOLTAGE(_index, _address) { \
> +	.type = IIO_VOLTAGE, \
> +	.address = _address, \
> +	.indexed = 1, \
> +	.channel = (_index), \
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> +			      BIT(IIO_CHAN_INFO_INT_TIME), \
> +	.scan_index = (_index), \
> +	.scan_type = { \
> +		.sign = 'u', \
> +		.realbits = 16, \
> +		.storagebits = 16, \
> +	.endianness = IIO_BE, \
> +	} \
> +}
> +
> +
> +static const struct iio_chan_spec ina2xx_channels[] = {
> +	INA2XX_CHAN_VOLTAGE(0, INA2XX_SHUNT_VOLTAGE),
> +	INA2XX_CHAN_VOLTAGE(1, INA2XX_BUS_VOLTAGE),
> +	INA2XX_CHAN(IIO_CURRENT, 2, INA2XX_CURRENT),
> +	INA2XX_CHAN(IIO_POWER, 3, INA2XX_POWER),
> +	IIO_CHAN_SOFT_TIMESTAMP(4),
> +};
> +
> +
> +static s64 prev_ns;
This prevents you having more than one instance of the device - move it into
your chip_info.
> +
> +static int ina2xx_work_buffer(struct iio_dev *indio_dev)
> +{
> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> +	unsigned short data[8];
> +	int bit, ret = 0, i = 0;
> +	unsigned long buffer_us = 0, elapsed_us = 0;
> +	s64 time_a, time_b;
> +	unsigned int alert;
> +
> +	time_a = iio_get_time_ns();
> +
> +	/*
> +	 * Because the timer thread and the chip conversion clock
> +	 * are asynchronous, the period difference will eventually
> +	 * result in reading V[k-1] again, or skip V[k] at time Tk.
> +	 * In order to resync the timer with the conversion process
> +	 * we check the ConVersionReadyFlag.
> +	 * On hardware that supports using the ALERT pin to toggle a
> +	 * GPIO a triggered buffer could be used instead.
> +	 * For now, we pay for that extra read of the ALERT register
> +	 */
> +	do {
> +		ret = regmap_read(chip->regmap, INA226_ALERT_MASK,
> +				  &alert);
> +		if (ret < 0)
> +			goto _err;
> +
> +		alert &= INA266_CVRF;
> +		trace_printk("Conversion ready: %d\n", !!alert);
> +
> +	} while (!alert);
> +
> +	/*
> +	 * Single register reads: bulk_read will not work with ina226
> +	 * as there is no auto-increment of the address register for
> +	 * data length longer than 16bits.
> +	 */
> +	for_each_set_bit(bit, indio_dev->active_scan_mask,
> +			 indio_dev->masklength) {
> +		unsigned int val;
> +
> +		ret = regmap_read(chip->regmap,
> +				  INA2XX_SHUNT_VOLTAGE + bit, &val);
> +		if (ret < 0)
> +			goto _err;
> +
> +		data[i++] = val;
> +	}
> +
> +	time_b = iio_get_time_ns();
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev,
> +					   (unsigned int *)data, time_a);
> +
> +	buffer_us = (unsigned long)(time_b - time_a) / 1000;
> +	elapsed_us = (unsigned long)(time_a - prev_ns) / 1000;
> +
> +	trace_printk("uS: elapsed: %lu, buf: %lu\n", elapsed_us, buffer_us);
> +
> +	prev_ns = time_a;
> +
> +_err:
> +	return buffer_us;
> +};
> +
> +static int ina2xx_capture_thread(void *data)
> +{
> +	struct iio_dev *indio_dev = (struct iio_dev *)data;
> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> +	unsigned int sampling_us = (chip->itb + chip->its) * chip->avg;
> +	unsigned long buffer_us;
> +
> +	/*
> +	 * Poll a bit faster than the chip internal Fs, in case
> +	 * we wish to sync with the conversion ready flag.
> +	 */
> +	sampling_us -= 200;
> +
> +	do {
> +		buffer_us = ina2xx_work_buffer(indio_dev);
> +
> +		if (sampling_us > buffer_us)
> +			udelay(sampling_us - buffer_us);
> +
> +	} while (!kthread_should_stop());
> +
> +	return 0;
> +}
> +
> +int ina2xx_buffer_enable(struct iio_dev *indio_dev)
> +{
> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> +	unsigned int sampling_us = (chip->itb + chip->its) * chip->avg;
> +
> +	trace_printk("Enabling buffer w/ scan_mask %02x, freq = %d, avg =%u\n",
> +		     (unsigned int)(*indio_dev->active_scan_mask),
> +		     1000000/sampling_us, chip->avg);
> +
> +	trace_printk("Expected work period: %u us\n", sampling_us);
> +
> +	prev_ns = iio_get_time_ns();
> +
> +	chip->task = kthread_run(ina2xx_capture_thread, (void *)indio_dev,
> +				 "ina2xx-%uus", sampling_us);
> +
> +	return PTR_ERR_OR_ZERO(chip->task);
> +}
> +
> +int ina2xx_buffer_disable(struct iio_dev *indio_dev)
> +{
> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> +
> +	if (chip->task) {
> +		kthread_stop(chip->task);
> +		chip->task = NULL;
> +	}
nitpick: blank line here.
> +	return 0;
> +}
> +
> +static const struct iio_buffer_setup_ops ina2xx_setup_ops = {
> +	.postenable = &ina2xx_buffer_enable,
> +	.postdisable = &ina2xx_buffer_disable,
> +};
> +
> +static int ina2xx_debug_reg(struct iio_dev *indio_dev,
> +			    unsigned reg, unsigned writeval, unsigned *readval)
> +{
> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> +
> +	if (!readval)
> +		return regmap_write(chip->regmap, reg, writeval);
> +
> +	return regmap_read(chip->regmap, reg, readval);
> +}
> +
> +/* frequencies matching the cummulated integration times for vshunt and vbus */
huh?  Integration time is in seconds... These seem unlikely to be valid
settings.
> +static IIO_CONST_ATTR_INT_TIME_AVAIL("140 204 332 588 1100 2116 4156 8244");
> +
> +static struct attribute *ina2xx_attributes[] = {
> +	&iio_const_attr_integration_time_available.dev_attr.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group ina2xx_attribute_group = {
> +	.attrs = ina2xx_attributes,
> +};
> +
> +static const struct iio_info ina2xx_info = {
> +	.debugfs_reg_access = &ina2xx_debug_reg,
> +	.read_raw = &ina2xx_read_raw,
> +	.write_raw = &ina2xx_write_raw,
> +	.attrs = &ina2xx_attribute_group,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +/* Initialize the configuration and calibration registers. */
> +static int ina2xx_init(struct ina2xx_chip_info *chip, unsigned int config)
> +{
> +	u16 regval;
> +	int ret = regmap_write(chip->regmap, INA2XX_CONFIG, config);
> +
> +	if (ret < 0)
> +		return ret;
> +	/*
> +	 * Set current LSB to 1mA, shunt is in uOhms
> +	 * (equation 13 in datasheet). We hardcode a Current_LSB
> +	 * of 1.0 x10-6. The only remaining parameter is RShunt
> +	 * There is no need to expose the CALIBRATION register
> +	 * to the user for now.
> +	 */
> +	regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor,
> +			    chip->rshunt);
> +
> +	return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval);
> +}
> +
> +static int ina2xx_probe(struct i2c_client *client,
> +			const struct i2c_device_id *id)
> +{
> +	struct ina2xx_chip_info *chip;
> +	struct iio_dev *indio_dev;
> +	struct iio_buffer *buffer;
> +	int ret;
> +	unsigned int val;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	chip = iio_priv(indio_dev);
> +
> +	chip->config = &ina2xx_config[id->driver_data];
> +
> +	if (of_property_read_u32(client->dev.of_node,
> +				 "shunt-resistor", &val) < 0) {
> +		struct ina2xx_platform_data *pdata > +		    dev_get_platdata(&client->dev);
> +
> +		if (pdata)
> +			val = pdata->shunt_uohms;
> +		else
> +			val = INA2XX_RSHUNT_DEFAULT;
> +	}
> +
> +	if (val <= 0 || val > chip->config->calibration_factor)
> +		return -ENODEV;
> +
> +	chip->rshunt = val;
> +
> +	ina2xx_regmap_config.max_register = chip->config->registers;
> +
> +	mutex_init(&chip->state_lock);
> +
> +	/* this is only used for device removal purposes */
> +	i2c_set_clientdata(client, indio_dev);
> +
> +	indio_dev->name = id->name;
> +	indio_dev->channels = ina2xx_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(ina2xx_channels);
> +
> +	indio_dev->dev.parent = &client->dev;
> +	indio_dev->info = &ina2xx_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
> +
> +	chip->regmap = devm_regmap_init_i2c(client, &ina2xx_regmap_config);
> +	if (IS_ERR(chip->regmap)) {
> +		dev_err(&client->dev, "failed to allocate register map\n");
> +		return PTR_ERR(chip->regmap);
> +	}
> +
> +	/* Patch the current config register with default. */
> +	val = chip->config->config_default;
> +
> +	if (id->driver_data = ina226) {
> +		ina226_set_average(chip, INA226_DEFAULT_AVG, &val);
> +		ina226_set_itb(chip, INA226_DEFAULT_IT, &val);
> +		ina226_set_its(chip, INA226_DEFAULT_IT, &val);
> +	}
> +
> +	ret = ina2xx_init(chip, val);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "error configuring the device: %d\n",
> +			ret);
> +		return -ENODEV;
> +	}
> +
> +	buffer = devm_iio_kfifo_allocate(&indio_dev->dev);
> +	if (!buffer)
> +		return -ENOMEM;
> +
> +	indio_dev->setup_ops = &ina2xx_setup_ops;
> +
> +	iio_device_attach_buffer(indio_dev, buffer);
> +
> +	return devm_iio_device_register(&client->dev, indio_dev);
> +}
> +
> +
> +static int ina2xx_remove(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> +	int ret;
> +
> +	mutex_destroy(&chip->state_lock);
> +
> +	/* Powerdown */
> +	ret = regmap_update_bits(chip->regmap, INA2XX_CONFIG,
> +				  INA2XX_MODE_MASK, 0);
> +
> +	iio_device_unregister(indio_dev);
Peter already covered this to a degree.

iio_device_unregister removes the userspace interface - hence it
must be the first thing done in remove.

If you use devm_ version of register it will occur at the end of the
remove.  Hence if the remove has anything in it, you pretty much
can't use the devm_iio_device_register without introducing a nasty
race condition.  Hence you don't want the devm version here and you
want to move iio_device_unregister to the beginning of this function.
> +
> +	return ret;
> +}
> +
> +
> +static const struct i2c_device_id ina2xx_id[] = {
> +	{"ina219", ina219},
> +	{"ina220", ina219},
> +	{"ina226", ina226},
> +	{"ina230", ina226},
> +	{"ina231", ina226},
> +	{}
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, ina2xx_id);
> +
> +static struct i2c_driver ina2xx_driver = {
> +	.driver = {
> +		   .name = KBUILD_MODNAME,
> +	},
> +	.probe = ina2xx_probe,
> +	.remove = ina2xx_remove,
> +	.id_table = ina2xx_id,
> +};
> +
> +module_i2c_driver(ina2xx_driver);
> +
> +MODULE_AUTHOR("Marc Titinger <marc.titinger@baylibre.com>");
> +MODULE_DESCRIPTION("Texas Instruments INA2XX ADC driver");
> +MODULE_LICENSE("GPL v2");
> 


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [PATCH 1/2] iio: ina2xx: add support for TI INA2xx Power Monitors

[Guenter Roeck]

On 11/29/2015 08:31 AM, Jonathan Cameron wrote:
> On 25/11/15 11:28, Marc Titinger wrote:
>> in SOFTWARE buffer mode, a kthread will capture the active scan_elements
>> into a kfifo, then compute the remaining time until the next capture tick
>> and do an active wait (udelay).
>>
>> This will produce a stream of up to fours channels plus a 64bits
>> timestamps (ns).
>>
>> Tested with ina226, on BeagleBoneBlack.
>>
>> Datasheet: http://www.ti.com/lit/gpn/ina226
>>
>> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
> A few more bits from me, but basically looking good.
>
> We do however, need to make the hwmon guys aware this is going on.
> Please cc their list on the next version.
>
> Last thing we want is for this to turn up as a surprise!
>

I have seen the original patch, so no surprise here. Just not sure
if we should remove the hwmon driver after the iio driver is accepted.
Even though the stated goal is different, it seems to me that having
two drivers really does not make sense. Any thoughts ?

Guenter

> Jonathan
>> ---
>>   drivers/iio/adc/Kconfig      |  10 +
>>   drivers/iio/adc/Makefile     |   1 +
>>   drivers/iio/adc/ina2xx-iio.c | 684 +++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 695 insertions(+)
>>   create mode 100644 drivers/iio/adc/ina2xx-iio.c
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index eb0cd89..9b87372 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -170,6 +170,16 @@ config EXYNOS_ADC
>>   	  of SoCs for drivers such as the touchscreen and hwmon to use to share
>>   	  this resource.
>>
>> +config INA2XX_IIO
>> +	tristate "Texas Instruments INA2xx Power Monitors IIO driver"
>> +	depends on I2C
>> +	select REGMAP_I2C
>> +	select IIO_BUFFER
>> +	help
>> +	  Say yes here to build support for TI INA2xx familly Power Monitors.
>> +
>> +	  Note that this is not the existing hwmon interface for this device.
>> +
>>   config LP8788_ADC
>>   	tristate "LP8788 ADC driver"
>>   	depends on MFD_LP8788
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index a096210..74e4341 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -19,6 +19,7 @@ obj-$(CONFIG_BERLIN2_ADC) += berlin2-adc.o
>>   obj-$(CONFIG_DA9150_GPADC) += da9150-gpadc.o
>>   obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o
>>   obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
>> +obj-$(CONFIG_INA2XX_IIO) += ina2xx-iio.o
>>   obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>>   obj-$(CONFIG_MAX1027) += max1027.o
>>   obj-$(CONFIG_MAX1363) += max1363.o
>> diff --git a/drivers/iio/adc/ina2xx-iio.c b/drivers/iio/adc/ina2xx-iio.c
>> new file mode 100644
>> index 0000000..4a0026c
>> --- /dev/null
>> +++ b/drivers/iio/adc/ina2xx-iio.c
>> @@ -0,0 +1,684 @@
>> +/*
>> + * INA2XX Current and Power Monitors
>> + *
>> + * Copyright 2015 Baylibre SAS.
>> + *
>> + * 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.
>> + *
>> + * Based on linux/drivers/iio/adc/ad7291.c
>> + * Copyright 2010-2011 Analog Devices Inc.
>> + *
>> + * Based on linux/drivers/hwmon/ina2xx.c
>> + * Copyright 2012 Lothar Felten <l-felten@ti.com>
>> + *
>> + * Licensed under the GPL-2 or later.
>> + *
>> + * IIO driver for INA219-220-226-230-231
>> + *
>> + * Configurable 7-bit I2C slave address from 0x40 to 0x4F
>> + */
>> +#include <linux/module.h>
>> +#include <linux/kthread.h>
>> +#include <linux/delay.h>
>> +#include <linux/iio/kfifo_buf.h>
>> +#include <linux/iio/sysfs.h>
>> +#include <linux/i2c.h>
>> +#include <linux/regmap.h>
>> +#include <linux/platform_data/ina2xx.h>
>> +
>> +#include <linux/util_macros.h>
>> +
>> +/*
>> + * INA2XX registers definition
>> + */
>> +/* common register definitions */
>> +#define INA2XX_CONFIG                   0x00
>> +#define INA2XX_SHUNT_VOLTAGE            0x01	/* readonly */
>> +#define INA2XX_BUS_VOLTAGE              0x02	/* readonly */
>> +#define INA2XX_POWER                    0x03	/* readonly */
>> +#define INA2XX_CURRENT                  0x04	/* readonly */
>> +#define INA2XX_CALIBRATION              0x05
>> +
>> +#define INA226_ALERT_MASK		0x06
>> +#define INA266_CVRF			BIT(3)
>> +
>> +/* register count */
>> +#define INA219_REGISTERS                6
>> +#define INA226_REGISTERS                8
>> +#define INA2XX_MAX_REGISTERS            8
>> +
>> +/* settings - depend on use case */
>> +#define INA219_CONFIG_DEFAULT           0x399F	/* PGA=8 */
>> +#define INA226_CONFIG_DEFAULT           0x4327
>> +#define INA226_DEFAULT_AVG              4
>> +#define INA226_DEFAULT_IT		1110
>> +
>> +#define INA2XX_RSHUNT_DEFAULT           10000
>> +
>> +/*
>> + * bit mask for reading the averaging setting in the configuration register
>> + * FIXME: use regmap_fields.
>> + */
>> +#define INA2XX_MODE_MASK	GENMASK(3, 0)
>> +
>> +#define INA226_AVG_MASK		GENMASK(11, 9)
>> +#define INA226_SHIFT_AVG(val)	((val) << 9)
>> +
>> +/* Integration time for VBus */
>> +#define INA226_ITB_MASK		GENMASK(8, 6)
>> +#define INA226_SHIFT_ITB(val)	((val) << 6)
>> +
>> +/*Integration Time for VShunt */
>> +#define INA226_ITS_MASK		GENMASK(5, 3)
>> +#define INA226_SHIFT_ITS(val)	((val) << 3)
>> +
>> +
>> +static bool ina2xx_is_writeable_reg(struct device *dev, unsigned int reg)
>> +{
>> +	return (reg == INA2XX_CONFIG) || (reg > INA2XX_CURRENT);
>> +}
>> +
>> +static bool ina2xx_is_volatile_reg(struct device *dev, unsigned int reg)
>> +{
>> +	return (reg != INA2XX_CONFIG);
>> +}
>> +
>> +static struct regmap_config ina2xx_regmap_config = {
>> +	.reg_bits = 8,
>> +	.val_bits = 16,
>> +
>> +	.writeable_reg = ina2xx_is_writeable_reg,
>> +	.volatile_reg = ina2xx_is_volatile_reg,
>> +};
>> +
>> +enum ina2xx_ids { ina219, ina226 };
>> +
>> +struct ina2xx_config {
>> +	u16 config_default;
>> +	int calibration_factor;
>> +	int registers;
>> +	int shunt_div;
>> +	int bus_voltage_shift;
>> +	int bus_voltage_lsb;	/* uV */
>> +	int power_lsb;		/* uW */
>> +};
>> +
>> +struct ina2xx_chip_info {
>> +	struct regmap *regmap;
>> +	struct task_struct *task;
>> +	const struct ina2xx_config *config;
>> +	struct mutex state_lock;
>> +	long rshunt;
>> +	int avg;
>> +	int itb; /* Bus voltage integration time uS */
>> +	int its; /* Shunt voltage integration tim uS */
>> +};
>> +
>> +static const struct ina2xx_config ina2xx_config[] = {
>> +	[ina219] = {
>> +		    .config_default = INA219_CONFIG_DEFAULT,
>> +		    .calibration_factor = 40960000,
>> +		    .registers = INA219_REGISTERS,
>> +		    .shunt_div = 100,
>> +		    .bus_voltage_shift = 3,
>> +		    .bus_voltage_lsb = 4000,
>> +		    .power_lsb = 20000,
>> +		    },
>> +	[ina226] = {
>> +		    .config_default = INA226_CONFIG_DEFAULT,
>> +		    .calibration_factor = 5120000,
>> +		    .registers = INA226_REGISTERS,
>> +		    .shunt_div = 400,
>> +		    .bus_voltage_shift = 0,
>> +		    .bus_voltage_lsb = 1250,
>> +		    .power_lsb = 25000,
>> +		    },
>> +};
>> +
>> +static int ina2xx_get_value(struct ina2xx_chip_info *chip, u8 reg,
>> +			    unsigned int regval, int *val, int *uval)
>> +{
>> +	*val = 0;
>> +
>> +	switch (reg) {
>> +	case INA2XX_SHUNT_VOLTAGE:
>> +		/* signed register */
>> +		*uval = DIV_ROUND_CLOSEST((s16) regval,
>> +					  chip->config->shunt_div);
>> +		return IIO_VAL_INT_PLUS_MICRO;
>> +
>> +	case INA2XX_BUS_VOLTAGE:
>> +		*uval = (regval >> chip->config->bus_voltage_shift)
>> +			* chip->config->bus_voltage_lsb;
>> +		*val = *uval/1000000;
>> +		*uval = *uval % 1000000;
>> +		return IIO_VAL_INT_PLUS_MICRO;
>> +
>> +	case INA2XX_POWER:
>> +		*uval = regval * chip->config->power_lsb;
>> +		*val = *uval/1000000;
>> +		*uval = *uval % 1000000;
>> +		return IIO_VAL_INT_PLUS_MICRO;
>> +
>> +	case INA2XX_CURRENT:
>> +		/* signed register, LSB=1mA (selected), in mA */
>> +		*uval = (s16) regval * 1000;
>> +		return IIO_VAL_INT_PLUS_MICRO;
>> +
>> +	default:
>> +		/* programmer goofed */
>> +		WARN_ON_ONCE(1);
>> +	}
>> +	return -EINVAL;
>> +}
>> +
>> +static int ina2xx_read_raw(struct iio_dev *indio_dev,
>> +			   struct iio_chan_spec const *chan,
>> +			   int *val, int *val2, long mask)
>> +{
>> +	int ret;
>> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>> +	unsigned int regval;
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		ret = regmap_read(chip->regmap, chan->address, &regval);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		return ina2xx_get_value(chip, chan->address, regval, val, val2);
>> +
>> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>> +		*val = chip->avg;
>> +		return IIO_VAL_INT;
>> +
>> +	case IIO_CHAN_INFO_INT_TIME:
>> +		*val = 0;
>> +		if (chan->address == INA2XX_SHUNT_VOLTAGE)
>> +			*val2 = chip->its;
>> +		else
>> +			*val2 = chip->itb;
>> +
>> +		return IIO_VAL_INT_PLUS_MICRO;
>> +	/*
>> +	 * sample freq is read only, it is a consequence of
>> +	 * 1/AVG*(CT_bus+CT_shunt)
>> +	 */
>> +	case IIO_CHAN_INFO_SAMP_FREQ:
>> +		*val = DIV_ROUND_CLOSEST(1000000,
>> +		       (chip->itb + chip->its) * chip->avg);
>> +
>> +		return IIO_VAL_INT;
>> +
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>> + * Available averaging rates for ina226. The indices correspond with
>> + * the bit values expected by the chip (according to the ina226 datasheet,
>> + * table 3 AVG bit settings, found at
>> + * http://www.ti.com/lit/ds/symlink/ina226.pdf.
>> + */
>> +static const int ina226_avg_tab[] = { 1, 4, 16, 64, 128, 256, 512, 1024 };
>> +
>> +static unsigned int ina226_set_average(struct ina2xx_chip_info *chip,
>> +				       unsigned int val,
>> +				       unsigned int *config)
>> +{
>> +	int bits;
>> +
>> +	if (val > 1024 || val < 1)
>> +		return -EINVAL;
>> +
>> +	bits = find_closest(val, ina226_avg_tab,
>> +			    ARRAY_SIZE(ina226_avg_tab));
>> +
>> +	chip->avg = ina226_avg_tab[bits];
>> +
>> +	*config &= ~INA226_AVG_MASK;
>> +	*config |= INA226_SHIFT_AVG(bits) & INA226_AVG_MASK;
>> +
>> +	return 0;
>> +}
>> +
>> +/* Conversion times in uS */
>> +static const int ina226_conv_time_tab[] = { 140, 204, 332, 588, 1100,
>> +					    2116, 4156, 8244};
>> +
>> +static unsigned int ina226_set_itb(struct ina2xx_chip_info *chip,
>> +				   unsigned int val,
>> +				   unsigned int *config)
>> +{
>> +	int bits;
>> +
>> +	if (val > 8244 || val < 140)
>> +		return -EINVAL;
>> +
>> +	bits = find_closest(val, ina226_conv_time_tab,
>> +			ARRAY_SIZE(ina226_conv_time_tab));
>> +
>> +	chip->itb = ina226_conv_time_tab[bits];
>> +
>> +	*config &= ~INA226_ITB_MASK;
>> +	*config |= INA226_SHIFT_ITB(bits) & INA226_ITB_MASK;
>> +
>> +	return 0;
>> +}
>> +
> Could do with a slightly more informative name than its vs itb.
> Or at least some docs explaining what it does.
>> +static unsigned int ina226_set_its(struct ina2xx_chip_info *chip,
>> +				   unsigned int val,
>> +				   unsigned int *config)
>> +{
>> +	int bits;
>> +
>> +	if (val > 8244 || val < 140)
>> +		return -EINVAL;
>> +
>> +	bits = find_closest(val, ina226_conv_time_tab,
>> +			ARRAY_SIZE(ina226_conv_time_tab));
>> +
>> +	chip->its = ina226_conv_time_tab[bits];
>> +
>> +	*config &= ~INA226_ITS_MASK;
>> +	*config |= INA226_SHIFT_ITS(bits) & INA226_ITS_MASK;
>> +
>> +	return 0;
>> +}
>> +
>> +
>> +static int ina2xx_write_raw(struct iio_dev *indio_dev,
>> +			    struct iio_chan_spec const *chan,
>> +			    int val, int val2, long mask)
>> +{
>> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>> +	int ret = 0;
>> +	unsigned int config, tmp;
>> +
>> +	if (iio_buffer_enabled(indio_dev))
>> +		return -EBUSY;
>> +
>> +	mutex_lock(&chip->state_lock);
>> +
>> +	ret = regmap_read(chip->regmap, INA2XX_CONFIG, &config);
>> +	if (ret < 0)
>> +		goto _err;
>> +
>> +	tmp = config;
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>> +		ret = ina226_set_average(chip, val, &tmp);
>> +		break;
>> +
>> +	case IIO_CHAN_INFO_INT_TIME:
>> +		if (chan->address == INA2XX_SHUNT_VOLTAGE)
>> +			ret = ina226_set_its(chip, val, &tmp);
>> +		else
>> +			ret = ina226_set_itb(chip, val, &tmp);
>> +		break;
>> +	default:
>> +		ret = -EINVAL;
>> +	}
>> +
>> +	if (!ret && (tmp != config))
>> +		ret = regmap_write(chip->regmap, INA2XX_CONFIG, tmp);
>> +_err:
>> +	mutex_unlock(&chip->state_lock);
>> +
>> +	return ret;
>> +}
>> +
>> +
>> +#define INA2XX_CHAN(_type, _index, _address) { \
>> +	.type = _type, \
>> +	.address = _address, \
>> +	.indexed = 1, \
>> +	.channel = (_index), \
>> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
>> +	.info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
>> +				   BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
>> +	.scan_index = (_index), \
>> +	.scan_type = { \
>> +		.sign = 'u', \
>> +		.realbits = 16, \
>> +		.storagebits = 16, \
>> +	.endianness = IIO_BE, \
>> +	} \
>> +}
>> +
>> +/*Sampling Freq is a consequence of the integration times of the V channels.*/
>> +#define INA2XX_CHAN_VOLTAGE(_index, _address) { \
>> +	.type = IIO_VOLTAGE, \
>> +	.address = _address, \
>> +	.indexed = 1, \
>> +	.channel = (_index), \
>> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
>> +			      BIT(IIO_CHAN_INFO_INT_TIME), \
>> +	.scan_index = (_index), \
>> +	.scan_type = { \
>> +		.sign = 'u', \
>> +		.realbits = 16, \
>> +		.storagebits = 16, \
>> +	.endianness = IIO_BE, \
>> +	} \
>> +}
>> +
>> +
>> +static const struct iio_chan_spec ina2xx_channels[] = {
>> +	INA2XX_CHAN_VOLTAGE(0, INA2XX_SHUNT_VOLTAGE),
>> +	INA2XX_CHAN_VOLTAGE(1, INA2XX_BUS_VOLTAGE),
>> +	INA2XX_CHAN(IIO_CURRENT, 2, INA2XX_CURRENT),
>> +	INA2XX_CHAN(IIO_POWER, 3, INA2XX_POWER),
>> +	IIO_CHAN_SOFT_TIMESTAMP(4),
>> +};
>> +
>> +
>> +static s64 prev_ns;
> This prevents you having more than one instance of the device - move it into
> your chip_info.
>> +
>> +static int ina2xx_work_buffer(struct iio_dev *indio_dev)
>> +{
>> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>> +	unsigned short data[8];
>> +	int bit, ret = 0, i = 0;
>> +	unsigned long buffer_us = 0, elapsed_us = 0;
>> +	s64 time_a, time_b;
>> +	unsigned int alert;
>> +
>> +	time_a = iio_get_time_ns();
>> +
>> +	/*
>> +	 * Because the timer thread and the chip conversion clock
>> +	 * are asynchronous, the period difference will eventually
>> +	 * result in reading V[k-1] again, or skip V[k] at time Tk.
>> +	 * In order to resync the timer with the conversion process
>> +	 * we check the ConVersionReadyFlag.
>> +	 * On hardware that supports using the ALERT pin to toggle a
>> +	 * GPIO a triggered buffer could be used instead.
>> +	 * For now, we pay for that extra read of the ALERT register
>> +	 */
>> +	do {
>> +		ret = regmap_read(chip->regmap, INA226_ALERT_MASK,
>> +				  &alert);
>> +		if (ret < 0)
>> +			goto _err;
>> +
>> +		alert &= INA266_CVRF;
>> +		trace_printk("Conversion ready: %d\n", !!alert);
>> +
>> +	} while (!alert);
>> +
>> +	/*
>> +	 * Single register reads: bulk_read will not work with ina226
>> +	 * as there is no auto-increment of the address register for
>> +	 * data length longer than 16bits.
>> +	 */
>> +	for_each_set_bit(bit, indio_dev->active_scan_mask,
>> +			 indio_dev->masklength) {
>> +		unsigned int val;
>> +
>> +		ret = regmap_read(chip->regmap,
>> +				  INA2XX_SHUNT_VOLTAGE + bit, &val);
>> +		if (ret < 0)
>> +			goto _err;
>> +
>> +		data[i++] = val;
>> +	}
>> +
>> +	time_b = iio_get_time_ns();
>> +
>> +	iio_push_to_buffers_with_timestamp(indio_dev,
>> +					   (unsigned int *)data, time_a);
>> +
>> +	buffer_us = (unsigned long)(time_b - time_a) / 1000;
>> +	elapsed_us = (unsigned long)(time_a - prev_ns) / 1000;
>> +
>> +	trace_printk("uS: elapsed: %lu, buf: %lu\n", elapsed_us, buffer_us);
>> +
>> +	prev_ns = time_a;
>> +
>> +_err:
>> +	return buffer_us;
>> +};
>> +
>> +static int ina2xx_capture_thread(void *data)
>> +{
>> +	struct iio_dev *indio_dev = (struct iio_dev *)data;
>> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>> +	unsigned int sampling_us = (chip->itb + chip->its) * chip->avg;
>> +	unsigned long buffer_us;
>> +
>> +	/*
>> +	 * Poll a bit faster than the chip internal Fs, in case
>> +	 * we wish to sync with the conversion ready flag.
>> +	 */
>> +	sampling_us -= 200;
>> +
>> +	do {
>> +		buffer_us = ina2xx_work_buffer(indio_dev);
>> +
>> +		if (sampling_us > buffer_us)
>> +			udelay(sampling_us - buffer_us);
>> +
>> +	} while (!kthread_should_stop());
>> +
>> +	return 0;
>> +}
>> +
>> +int ina2xx_buffer_enable(struct iio_dev *indio_dev)
>> +{
>> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>> +	unsigned int sampling_us = (chip->itb + chip->its) * chip->avg;
>> +
>> +	trace_printk("Enabling buffer w/ scan_mask %02x, freq = %d, avg =%u\n",
>> +		     (unsigned int)(*indio_dev->active_scan_mask),
>> +		     1000000/sampling_us, chip->avg);
>> +
>> +	trace_printk("Expected work period: %u us\n", sampling_us);
>> +
>> +	prev_ns = iio_get_time_ns();
>> +
>> +	chip->task = kthread_run(ina2xx_capture_thread, (void *)indio_dev,
>> +				 "ina2xx-%uus", sampling_us);
>> +
>> +	return PTR_ERR_OR_ZERO(chip->task);
>> +}
>> +
>> +int ina2xx_buffer_disable(struct iio_dev *indio_dev)
>> +{
>> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>> +
>> +	if (chip->task) {
>> +		kthread_stop(chip->task);
>> +		chip->task = NULL;
>> +	}
> nitpick: blank line here.
>> +	return 0;
>> +}
>> +
>> +static const struct iio_buffer_setup_ops ina2xx_setup_ops = {
>> +	.postenable = &ina2xx_buffer_enable,
>> +	.postdisable = &ina2xx_buffer_disable,
>> +};
>> +
>> +static int ina2xx_debug_reg(struct iio_dev *indio_dev,
>> +			    unsigned reg, unsigned writeval, unsigned *readval)
>> +{
>> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>> +
>> +	if (!readval)
>> +		return regmap_write(chip->regmap, reg, writeval);
>> +
>> +	return regmap_read(chip->regmap, reg, readval);
>> +}
>> +
>> +/* frequencies matching the cummulated integration times for vshunt and vbus */
> huh?  Integration time is in seconds... These seem unlikely to be valid
> settings.
>> +static IIO_CONST_ATTR_INT_TIME_AVAIL("140 204 332 588 1100 2116 4156 8244");
>> +
>> +static struct attribute *ina2xx_attributes[] = {
>> +	&iio_const_attr_integration_time_available.dev_attr.attr,
>> +	NULL,
>> +};
>> +
>> +static const struct attribute_group ina2xx_attribute_group = {
>> +	.attrs = ina2xx_attributes,
>> +};
>> +
>> +static const struct iio_info ina2xx_info = {
>> +	.debugfs_reg_access = &ina2xx_debug_reg,
>> +	.read_raw = &ina2xx_read_raw,
>> +	.write_raw = &ina2xx_write_raw,
>> +	.attrs = &ina2xx_attribute_group,
>> +	.driver_module = THIS_MODULE,
>> +};
>> +
>> +/* Initialize the configuration and calibration registers. */
>> +static int ina2xx_init(struct ina2xx_chip_info *chip, unsigned int config)
>> +{
>> +	u16 regval;
>> +	int ret = regmap_write(chip->regmap, INA2XX_CONFIG, config);
>> +
>> +	if (ret < 0)
>> +		return ret;
>> +	/*
>> +	 * Set current LSB to 1mA, shunt is in uOhms
>> +	 * (equation 13 in datasheet). We hardcode a Current_LSB
>> +	 * of 1.0 x10-6. The only remaining parameter is RShunt
>> +	 * There is no need to expose the CALIBRATION register
>> +	 * to the user for now.
>> +	 */
>> +	regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor,
>> +			    chip->rshunt);
>> +
>> +	return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval);
>> +}
>> +
>> +static int ina2xx_probe(struct i2c_client *client,
>> +			const struct i2c_device_id *id)
>> +{
>> +	struct ina2xx_chip_info *chip;
>> +	struct iio_dev *indio_dev;
>> +	struct iio_buffer *buffer;
>> +	int ret;
>> +	unsigned int val;
>> +
>> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
>> +	if (!indio_dev)
>> +		return -ENOMEM;
>> +
>> +	chip = iio_priv(indio_dev);
>> +
>> +	chip->config = &ina2xx_config[id->driver_data];
>> +
>> +	if (of_property_read_u32(client->dev.of_node,
>> +				 "shunt-resistor", &val) < 0) {
>> +		struct ina2xx_platform_data *pdata =
>> +		    dev_get_platdata(&client->dev);
>> +
>> +		if (pdata)
>> +			val = pdata->shunt_uohms;
>> +		else
>> +			val = INA2XX_RSHUNT_DEFAULT;
>> +	}
>> +
>> +	if (val <= 0 || val > chip->config->calibration_factor)
>> +		return -ENODEV;
>> +
>> +	chip->rshunt = val;
>> +
>> +	ina2xx_regmap_config.max_register = chip->config->registers;
>> +
>> +	mutex_init(&chip->state_lock);
>> +
>> +	/* this is only used for device removal purposes */
>> +	i2c_set_clientdata(client, indio_dev);
>> +
>> +	indio_dev->name = id->name;
>> +	indio_dev->channels = ina2xx_channels;
>> +	indio_dev->num_channels = ARRAY_SIZE(ina2xx_channels);
>> +
>> +	indio_dev->dev.parent = &client->dev;
>> +	indio_dev->info = &ina2xx_info;
>> +	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
>> +
>> +	chip->regmap = devm_regmap_init_i2c(client, &ina2xx_regmap_config);
>> +	if (IS_ERR(chip->regmap)) {
>> +		dev_err(&client->dev, "failed to allocate register map\n");
>> +		return PTR_ERR(chip->regmap);
>> +	}
>> +
>> +	/* Patch the current config register with default. */
>> +	val = chip->config->config_default;
>> +
>> +	if (id->driver_data == ina226) {
>> +		ina226_set_average(chip, INA226_DEFAULT_AVG, &val);
>> +		ina226_set_itb(chip, INA226_DEFAULT_IT, &val);
>> +		ina226_set_its(chip, INA226_DEFAULT_IT, &val);
>> +	}
>> +
>> +	ret = ina2xx_init(chip, val);
>> +	if (ret < 0) {
>> +		dev_err(&client->dev, "error configuring the device: %d\n",
>> +			ret);
>> +		return -ENODEV;
>> +	}
>> +
>> +	buffer = devm_iio_kfifo_allocate(&indio_dev->dev);
>> +	if (!buffer)
>> +		return -ENOMEM;
>> +
>> +	indio_dev->setup_ops = &ina2xx_setup_ops;
>> +
>> +	iio_device_attach_buffer(indio_dev, buffer);
>> +
>> +	return devm_iio_device_register(&client->dev, indio_dev);
>> +}
>> +
>> +
>> +static int ina2xx_remove(struct i2c_client *client)
>> +{
>> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
>> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	mutex_destroy(&chip->state_lock);
>> +
>> +	/* Powerdown */
>> +	ret = regmap_update_bits(chip->regmap, INA2XX_CONFIG,
>> +				  INA2XX_MODE_MASK, 0);
>> +
>> +	iio_device_unregister(indio_dev);
> Peter already covered this to a degree.
>
> iio_device_unregister removes the userspace interface - hence it
> must be the first thing done in remove.
>
> If you use devm_ version of register it will occur at the end of the
> remove.  Hence if the remove has anything in it, you pretty much
> can't use the devm_iio_device_register without introducing a nasty
> race condition.  Hence you don't want the devm version here and you
> want to move iio_device_unregister to the beginning of this function.
>> +
>> +	return ret;
>> +}
>> +
>> +
>> +static const struct i2c_device_id ina2xx_id[] = {
>> +	{"ina219", ina219},
>> +	{"ina220", ina219},
>> +	{"ina226", ina226},
>> +	{"ina230", ina226},
>> +	{"ina231", ina226},
>> +	{}
>> +};
>> +
>> +MODULE_DEVICE_TABLE(i2c, ina2xx_id);
>> +
>> +static struct i2c_driver ina2xx_driver = {
>> +	.driver = {
>> +		   .name = KBUILD_MODNAME,
>> +	},
>> +	.probe = ina2xx_probe,
>> +	.remove = ina2xx_remove,
>> +	.id_table = ina2xx_id,
>> +};
>> +
>> +module_i2c_driver(ina2xx_driver);
>> +
>> +MODULE_AUTHOR("Marc Titinger <marc.titinger@baylibre.com>");
>> +MODULE_DESCRIPTION("Texas Instruments INA2XX ADC driver");
>> +MODULE_LICENSE("GPL v2");
>>
>
>

Re: [lm-sensors] [PATCH 1/2] iio: ina2xx: add support for TI INA2xx Power Monitors

[Guenter Roeck]

On 11/29/2015 08:31 AM, Jonathan Cameron wrote:
> On 25/11/15 11:28, Marc Titinger wrote:
>> in SOFTWARE buffer mode, a kthread will capture the active scan_elements
>> into a kfifo, then compute the remaining time until the next capture tick
>> and do an active wait (udelay).
>>
>> This will produce a stream of up to fours channels plus a 64bits
>> timestamps (ns).
>>
>> Tested with ina226, on BeagleBoneBlack.
>>
>> Datasheet: http://www.ti.com/lit/gpn/ina226
>>
>> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
> A few more bits from me, but basically looking good.
>
> We do however, need to make the hwmon guys aware this is going on.
> Please cc their list on the next version.
>
> Last thing we want is for this to turn up as a surprise!
>

I have seen the original patch, so no surprise here. Just not sure
if we should remove the hwmon driver after the iio driver is accepted.
Even though the stated goal is different, it seems to me that having
two drivers really does not make sense. Any thoughts ?

Guenter

> Jonathan
>> ---
>>   drivers/iio/adc/Kconfig      |  10 +
>>   drivers/iio/adc/Makefile     |   1 +
>>   drivers/iio/adc/ina2xx-iio.c | 684 +++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 695 insertions(+)
>>   create mode 100644 drivers/iio/adc/ina2xx-iio.c
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index eb0cd89..9b87372 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -170,6 +170,16 @@ config EXYNOS_ADC
>>   	  of SoCs for drivers such as the touchscreen and hwmon to use to share
>>   	  this resource.
>>
>> +config INA2XX_IIO
>> +	tristate "Texas Instruments INA2xx Power Monitors IIO driver"
>> +	depends on I2C
>> +	select REGMAP_I2C
>> +	select IIO_BUFFER
>> +	help
>> +	  Say yes here to build support for TI INA2xx familly Power Monitors.
>> +
>> +	  Note that this is not the existing hwmon interface for this device.
>> +
>>   config LP8788_ADC
>>   	tristate "LP8788 ADC driver"
>>   	depends on MFD_LP8788
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index a096210..74e4341 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -19,6 +19,7 @@ obj-$(CONFIG_BERLIN2_ADC) += berlin2-adc.o
>>   obj-$(CONFIG_DA9150_GPADC) += da9150-gpadc.o
>>   obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o
>>   obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
>> +obj-$(CONFIG_INA2XX_IIO) += ina2xx-iio.o
>>   obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>>   obj-$(CONFIG_MAX1027) += max1027.o
>>   obj-$(CONFIG_MAX1363) += max1363.o
>> diff --git a/drivers/iio/adc/ina2xx-iio.c b/drivers/iio/adc/ina2xx-iio.c
>> new file mode 100644
>> index 0000000..4a0026c
>> --- /dev/null
>> +++ b/drivers/iio/adc/ina2xx-iio.c
>> @@ -0,0 +1,684 @@
>> +/*
>> + * INA2XX Current and Power Monitors
>> + *
>> + * Copyright 2015 Baylibre SAS.
>> + *
>> + * 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.
>> + *
>> + * Based on linux/drivers/iio/adc/ad7291.c
>> + * Copyright 2010-2011 Analog Devices Inc.
>> + *
>> + * Based on linux/drivers/hwmon/ina2xx.c
>> + * Copyright 2012 Lothar Felten <l-felten@ti.com>
>> + *
>> + * Licensed under the GPL-2 or later.
>> + *
>> + * IIO driver for INA219-220-226-230-231
>> + *
>> + * Configurable 7-bit I2C slave address from 0x40 to 0x4F
>> + */
>> +#include <linux/module.h>
>> +#include <linux/kthread.h>
>> +#include <linux/delay.h>
>> +#include <linux/iio/kfifo_buf.h>
>> +#include <linux/iio/sysfs.h>
>> +#include <linux/i2c.h>
>> +#include <linux/regmap.h>
>> +#include <linux/platform_data/ina2xx.h>
>> +
>> +#include <linux/util_macros.h>
>> +
>> +/*
>> + * INA2XX registers definition
>> + */
>> +/* common register definitions */
>> +#define INA2XX_CONFIG                   0x00
>> +#define INA2XX_SHUNT_VOLTAGE            0x01	/* readonly */
>> +#define INA2XX_BUS_VOLTAGE              0x02	/* readonly */
>> +#define INA2XX_POWER                    0x03	/* readonly */
>> +#define INA2XX_CURRENT                  0x04	/* readonly */
>> +#define INA2XX_CALIBRATION              0x05
>> +
>> +#define INA226_ALERT_MASK		0x06
>> +#define INA266_CVRF			BIT(3)
>> +
>> +/* register count */
>> +#define INA219_REGISTERS                6
>> +#define INA226_REGISTERS                8
>> +#define INA2XX_MAX_REGISTERS            8
>> +
>> +/* settings - depend on use case */
>> +#define INA219_CONFIG_DEFAULT           0x399F	/* PGA=8 */
>> +#define INA226_CONFIG_DEFAULT           0x4327
>> +#define INA226_DEFAULT_AVG              4
>> +#define INA226_DEFAULT_IT		1110
>> +
>> +#define INA2XX_RSHUNT_DEFAULT           10000
>> +
>> +/*
>> + * bit mask for reading the averaging setting in the configuration register
>> + * FIXME: use regmap_fields.
>> + */
>> +#define INA2XX_MODE_MASK	GENMASK(3, 0)
>> +
>> +#define INA226_AVG_MASK		GENMASK(11, 9)
>> +#define INA226_SHIFT_AVG(val)	((val) << 9)
>> +
>> +/* Integration time for VBus */
>> +#define INA226_ITB_MASK		GENMASK(8, 6)
>> +#define INA226_SHIFT_ITB(val)	((val) << 6)
>> +
>> +/*Integration Time for VShunt */
>> +#define INA226_ITS_MASK		GENMASK(5, 3)
>> +#define INA226_SHIFT_ITS(val)	((val) << 3)
>> +
>> +
>> +static bool ina2xx_is_writeable_reg(struct device *dev, unsigned int reg)
>> +{
>> +	return (reg = INA2XX_CONFIG) || (reg > INA2XX_CURRENT);
>> +}
>> +
>> +static bool ina2xx_is_volatile_reg(struct device *dev, unsigned int reg)
>> +{
>> +	return (reg != INA2XX_CONFIG);
>> +}
>> +
>> +static struct regmap_config ina2xx_regmap_config = {
>> +	.reg_bits = 8,
>> +	.val_bits = 16,
>> +
>> +	.writeable_reg = ina2xx_is_writeable_reg,
>> +	.volatile_reg = ina2xx_is_volatile_reg,
>> +};
>> +
>> +enum ina2xx_ids { ina219, ina226 };
>> +
>> +struct ina2xx_config {
>> +	u16 config_default;
>> +	int calibration_factor;
>> +	int registers;
>> +	int shunt_div;
>> +	int bus_voltage_shift;
>> +	int bus_voltage_lsb;	/* uV */
>> +	int power_lsb;		/* uW */
>> +};
>> +
>> +struct ina2xx_chip_info {
>> +	struct regmap *regmap;
>> +	struct task_struct *task;
>> +	const struct ina2xx_config *config;
>> +	struct mutex state_lock;
>> +	long rshunt;
>> +	int avg;
>> +	int itb; /* Bus voltage integration time uS */
>> +	int its; /* Shunt voltage integration tim uS */
>> +};
>> +
>> +static const struct ina2xx_config ina2xx_config[] = {
>> +	[ina219] = {
>> +		    .config_default = INA219_CONFIG_DEFAULT,
>> +		    .calibration_factor = 40960000,
>> +		    .registers = INA219_REGISTERS,
>> +		    .shunt_div = 100,
>> +		    .bus_voltage_shift = 3,
>> +		    .bus_voltage_lsb = 4000,
>> +		    .power_lsb = 20000,
>> +		    },
>> +	[ina226] = {
>> +		    .config_default = INA226_CONFIG_DEFAULT,
>> +		    .calibration_factor = 5120000,
>> +		    .registers = INA226_REGISTERS,
>> +		    .shunt_div = 400,
>> +		    .bus_voltage_shift = 0,
>> +		    .bus_voltage_lsb = 1250,
>> +		    .power_lsb = 25000,
>> +		    },
>> +};
>> +
>> +static int ina2xx_get_value(struct ina2xx_chip_info *chip, u8 reg,
>> +			    unsigned int regval, int *val, int *uval)
>> +{
>> +	*val = 0;
>> +
>> +	switch (reg) {
>> +	case INA2XX_SHUNT_VOLTAGE:
>> +		/* signed register */
>> +		*uval = DIV_ROUND_CLOSEST((s16) regval,
>> +					  chip->config->shunt_div);
>> +		return IIO_VAL_INT_PLUS_MICRO;
>> +
>> +	case INA2XX_BUS_VOLTAGE:
>> +		*uval = (regval >> chip->config->bus_voltage_shift)
>> +			* chip->config->bus_voltage_lsb;
>> +		*val = *uval/1000000;
>> +		*uval = *uval % 1000000;
>> +		return IIO_VAL_INT_PLUS_MICRO;
>> +
>> +	case INA2XX_POWER:
>> +		*uval = regval * chip->config->power_lsb;
>> +		*val = *uval/1000000;
>> +		*uval = *uval % 1000000;
>> +		return IIO_VAL_INT_PLUS_MICRO;
>> +
>> +	case INA2XX_CURRENT:
>> +		/* signed register, LSB=1mA (selected), in mA */
>> +		*uval = (s16) regval * 1000;
>> +		return IIO_VAL_INT_PLUS_MICRO;
>> +
>> +	default:
>> +		/* programmer goofed */
>> +		WARN_ON_ONCE(1);
>> +	}
>> +	return -EINVAL;
>> +}
>> +
>> +static int ina2xx_read_raw(struct iio_dev *indio_dev,
>> +			   struct iio_chan_spec const *chan,
>> +			   int *val, int *val2, long mask)
>> +{
>> +	int ret;
>> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>> +	unsigned int regval;
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		ret = regmap_read(chip->regmap, chan->address, &regval);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		return ina2xx_get_value(chip, chan->address, regval, val, val2);
>> +
>> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>> +		*val = chip->avg;
>> +		return IIO_VAL_INT;
>> +
>> +	case IIO_CHAN_INFO_INT_TIME:
>> +		*val = 0;
>> +		if (chan->address = INA2XX_SHUNT_VOLTAGE)
>> +			*val2 = chip->its;
>> +		else
>> +			*val2 = chip->itb;
>> +
>> +		return IIO_VAL_INT_PLUS_MICRO;
>> +	/*
>> +	 * sample freq is read only, it is a consequence of
>> +	 * 1/AVG*(CT_bus+CT_shunt)
>> +	 */
>> +	case IIO_CHAN_INFO_SAMP_FREQ:
>> +		*val = DIV_ROUND_CLOSEST(1000000,
>> +		       (chip->itb + chip->its) * chip->avg);
>> +
>> +		return IIO_VAL_INT;
>> +
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>> + * Available averaging rates for ina226. The indices correspond with
>> + * the bit values expected by the chip (according to the ina226 datasheet,
>> + * table 3 AVG bit settings, found at
>> + * http://www.ti.com/lit/ds/symlink/ina226.pdf.
>> + */
>> +static const int ina226_avg_tab[] = { 1, 4, 16, 64, 128, 256, 512, 1024 };
>> +
>> +static unsigned int ina226_set_average(struct ina2xx_chip_info *chip,
>> +				       unsigned int val,
>> +				       unsigned int *config)
>> +{
>> +	int bits;
>> +
>> +	if (val > 1024 || val < 1)
>> +		return -EINVAL;
>> +
>> +	bits = find_closest(val, ina226_avg_tab,
>> +			    ARRAY_SIZE(ina226_avg_tab));
>> +
>> +	chip->avg = ina226_avg_tab[bits];
>> +
>> +	*config &= ~INA226_AVG_MASK;
>> +	*config |= INA226_SHIFT_AVG(bits) & INA226_AVG_MASK;
>> +
>> +	return 0;
>> +}
>> +
>> +/* Conversion times in uS */
>> +static const int ina226_conv_time_tab[] = { 140, 204, 332, 588, 1100,
>> +					    2116, 4156, 8244};
>> +
>> +static unsigned int ina226_set_itb(struct ina2xx_chip_info *chip,
>> +				   unsigned int val,
>> +				   unsigned int *config)
>> +{
>> +	int bits;
>> +
>> +	if (val > 8244 || val < 140)
>> +		return -EINVAL;
>> +
>> +	bits = find_closest(val, ina226_conv_time_tab,
>> +			ARRAY_SIZE(ina226_conv_time_tab));
>> +
>> +	chip->itb = ina226_conv_time_tab[bits];
>> +
>> +	*config &= ~INA226_ITB_MASK;
>> +	*config |= INA226_SHIFT_ITB(bits) & INA226_ITB_MASK;
>> +
>> +	return 0;
>> +}
>> +
> Could do with a slightly more informative name than its vs itb.
> Or at least some docs explaining what it does.
>> +static unsigned int ina226_set_its(struct ina2xx_chip_info *chip,
>> +				   unsigned int val,
>> +				   unsigned int *config)
>> +{
>> +	int bits;
>> +
>> +	if (val > 8244 || val < 140)
>> +		return -EINVAL;
>> +
>> +	bits = find_closest(val, ina226_conv_time_tab,
>> +			ARRAY_SIZE(ina226_conv_time_tab));
>> +
>> +	chip->its = ina226_conv_time_tab[bits];
>> +
>> +	*config &= ~INA226_ITS_MASK;
>> +	*config |= INA226_SHIFT_ITS(bits) & INA226_ITS_MASK;
>> +
>> +	return 0;
>> +}
>> +
>> +
>> +static int ina2xx_write_raw(struct iio_dev *indio_dev,
>> +			    struct iio_chan_spec const *chan,
>> +			    int val, int val2, long mask)
>> +{
>> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>> +	int ret = 0;
>> +	unsigned int config, tmp;
>> +
>> +	if (iio_buffer_enabled(indio_dev))
>> +		return -EBUSY;
>> +
>> +	mutex_lock(&chip->state_lock);
>> +
>> +	ret = regmap_read(chip->regmap, INA2XX_CONFIG, &config);
>> +	if (ret < 0)
>> +		goto _err;
>> +
>> +	tmp = config;
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>> +		ret = ina226_set_average(chip, val, &tmp);
>> +		break;
>> +
>> +	case IIO_CHAN_INFO_INT_TIME:
>> +		if (chan->address = INA2XX_SHUNT_VOLTAGE)
>> +			ret = ina226_set_its(chip, val, &tmp);
>> +		else
>> +			ret = ina226_set_itb(chip, val, &tmp);
>> +		break;
>> +	default:
>> +		ret = -EINVAL;
>> +	}
>> +
>> +	if (!ret && (tmp != config))
>> +		ret = regmap_write(chip->regmap, INA2XX_CONFIG, tmp);
>> +_err:
>> +	mutex_unlock(&chip->state_lock);
>> +
>> +	return ret;
>> +}
>> +
>> +
>> +#define INA2XX_CHAN(_type, _index, _address) { \
>> +	.type = _type, \
>> +	.address = _address, \
>> +	.indexed = 1, \
>> +	.channel = (_index), \
>> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
>> +	.info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
>> +				   BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
>> +	.scan_index = (_index), \
>> +	.scan_type = { \
>> +		.sign = 'u', \
>> +		.realbits = 16, \
>> +		.storagebits = 16, \
>> +	.endianness = IIO_BE, \
>> +	} \
>> +}
>> +
>> +/*Sampling Freq is a consequence of the integration times of the V channels.*/
>> +#define INA2XX_CHAN_VOLTAGE(_index, _address) { \
>> +	.type = IIO_VOLTAGE, \
>> +	.address = _address, \
>> +	.indexed = 1, \
>> +	.channel = (_index), \
>> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
>> +			      BIT(IIO_CHAN_INFO_INT_TIME), \
>> +	.scan_index = (_index), \
>> +	.scan_type = { \
>> +		.sign = 'u', \
>> +		.realbits = 16, \
>> +		.storagebits = 16, \
>> +	.endianness = IIO_BE, \
>> +	} \
>> +}
>> +
>> +
>> +static const struct iio_chan_spec ina2xx_channels[] = {
>> +	INA2XX_CHAN_VOLTAGE(0, INA2XX_SHUNT_VOLTAGE),
>> +	INA2XX_CHAN_VOLTAGE(1, INA2XX_BUS_VOLTAGE),
>> +	INA2XX_CHAN(IIO_CURRENT, 2, INA2XX_CURRENT),
>> +	INA2XX_CHAN(IIO_POWER, 3, INA2XX_POWER),
>> +	IIO_CHAN_SOFT_TIMESTAMP(4),
>> +};
>> +
>> +
>> +static s64 prev_ns;
> This prevents you having more than one instance of the device - move it into
> your chip_info.
>> +
>> +static int ina2xx_work_buffer(struct iio_dev *indio_dev)
>> +{
>> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>> +	unsigned short data[8];
>> +	int bit, ret = 0, i = 0;
>> +	unsigned long buffer_us = 0, elapsed_us = 0;
>> +	s64 time_a, time_b;
>> +	unsigned int alert;
>> +
>> +	time_a = iio_get_time_ns();
>> +
>> +	/*
>> +	 * Because the timer thread and the chip conversion clock
>> +	 * are asynchronous, the period difference will eventually
>> +	 * result in reading V[k-1] again, or skip V[k] at time Tk.
>> +	 * In order to resync the timer with the conversion process
>> +	 * we check the ConVersionReadyFlag.
>> +	 * On hardware that supports using the ALERT pin to toggle a
>> +	 * GPIO a triggered buffer could be used instead.
>> +	 * For now, we pay for that extra read of the ALERT register
>> +	 */
>> +	do {
>> +		ret = regmap_read(chip->regmap, INA226_ALERT_MASK,
>> +				  &alert);
>> +		if (ret < 0)
>> +			goto _err;
>> +
>> +		alert &= INA266_CVRF;
>> +		trace_printk("Conversion ready: %d\n", !!alert);
>> +
>> +	} while (!alert);
>> +
>> +	/*
>> +	 * Single register reads: bulk_read will not work with ina226
>> +	 * as there is no auto-increment of the address register for
>> +	 * data length longer than 16bits.
>> +	 */
>> +	for_each_set_bit(bit, indio_dev->active_scan_mask,
>> +			 indio_dev->masklength) {
>> +		unsigned int val;
>> +
>> +		ret = regmap_read(chip->regmap,
>> +				  INA2XX_SHUNT_VOLTAGE + bit, &val);
>> +		if (ret < 0)
>> +			goto _err;
>> +
>> +		data[i++] = val;
>> +	}
>> +
>> +	time_b = iio_get_time_ns();
>> +
>> +	iio_push_to_buffers_with_timestamp(indio_dev,
>> +					   (unsigned int *)data, time_a);
>> +
>> +	buffer_us = (unsigned long)(time_b - time_a) / 1000;
>> +	elapsed_us = (unsigned long)(time_a - prev_ns) / 1000;
>> +
>> +	trace_printk("uS: elapsed: %lu, buf: %lu\n", elapsed_us, buffer_us);
>> +
>> +	prev_ns = time_a;
>> +
>> +_err:
>> +	return buffer_us;
>> +};
>> +
>> +static int ina2xx_capture_thread(void *data)
>> +{
>> +	struct iio_dev *indio_dev = (struct iio_dev *)data;
>> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>> +	unsigned int sampling_us = (chip->itb + chip->its) * chip->avg;
>> +	unsigned long buffer_us;
>> +
>> +	/*
>> +	 * Poll a bit faster than the chip internal Fs, in case
>> +	 * we wish to sync with the conversion ready flag.
>> +	 */
>> +	sampling_us -= 200;
>> +
>> +	do {
>> +		buffer_us = ina2xx_work_buffer(indio_dev);
>> +
>> +		if (sampling_us > buffer_us)
>> +			udelay(sampling_us - buffer_us);
>> +
>> +	} while (!kthread_should_stop());
>> +
>> +	return 0;
>> +}
>> +
>> +int ina2xx_buffer_enable(struct iio_dev *indio_dev)
>> +{
>> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>> +	unsigned int sampling_us = (chip->itb + chip->its) * chip->avg;
>> +
>> +	trace_printk("Enabling buffer w/ scan_mask %02x, freq = %d, avg =%u\n",
>> +		     (unsigned int)(*indio_dev->active_scan_mask),
>> +		     1000000/sampling_us, chip->avg);
>> +
>> +	trace_printk("Expected work period: %u us\n", sampling_us);
>> +
>> +	prev_ns = iio_get_time_ns();
>> +
>> +	chip->task = kthread_run(ina2xx_capture_thread, (void *)indio_dev,
>> +				 "ina2xx-%uus", sampling_us);
>> +
>> +	return PTR_ERR_OR_ZERO(chip->task);
>> +}
>> +
>> +int ina2xx_buffer_disable(struct iio_dev *indio_dev)
>> +{
>> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>> +
>> +	if (chip->task) {
>> +		kthread_stop(chip->task);
>> +		chip->task = NULL;
>> +	}
> nitpick: blank line here.
>> +	return 0;
>> +}
>> +
>> +static const struct iio_buffer_setup_ops ina2xx_setup_ops = {
>> +	.postenable = &ina2xx_buffer_enable,
>> +	.postdisable = &ina2xx_buffer_disable,
>> +};
>> +
>> +static int ina2xx_debug_reg(struct iio_dev *indio_dev,
>> +			    unsigned reg, unsigned writeval, unsigned *readval)
>> +{
>> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>> +
>> +	if (!readval)
>> +		return regmap_write(chip->regmap, reg, writeval);
>> +
>> +	return regmap_read(chip->regmap, reg, readval);
>> +}
>> +
>> +/* frequencies matching the cummulated integration times for vshunt and vbus */
> huh?  Integration time is in seconds... These seem unlikely to be valid
> settings.
>> +static IIO_CONST_ATTR_INT_TIME_AVAIL("140 204 332 588 1100 2116 4156 8244");
>> +
>> +static struct attribute *ina2xx_attributes[] = {
>> +	&iio_const_attr_integration_time_available.dev_attr.attr,
>> +	NULL,
>> +};
>> +
>> +static const struct attribute_group ina2xx_attribute_group = {
>> +	.attrs = ina2xx_attributes,
>> +};
>> +
>> +static const struct iio_info ina2xx_info = {
>> +	.debugfs_reg_access = &ina2xx_debug_reg,
>> +	.read_raw = &ina2xx_read_raw,
>> +	.write_raw = &ina2xx_write_raw,
>> +	.attrs = &ina2xx_attribute_group,
>> +	.driver_module = THIS_MODULE,
>> +};
>> +
>> +/* Initialize the configuration and calibration registers. */
>> +static int ina2xx_init(struct ina2xx_chip_info *chip, unsigned int config)
>> +{
>> +	u16 regval;
>> +	int ret = regmap_write(chip->regmap, INA2XX_CONFIG, config);
>> +
>> +	if (ret < 0)
>> +		return ret;
>> +	/*
>> +	 * Set current LSB to 1mA, shunt is in uOhms
>> +	 * (equation 13 in datasheet). We hardcode a Current_LSB
>> +	 * of 1.0 x10-6. The only remaining parameter is RShunt
>> +	 * There is no need to expose the CALIBRATION register
>> +	 * to the user for now.
>> +	 */
>> +	regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor,
>> +			    chip->rshunt);
>> +
>> +	return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval);
>> +}
>> +
>> +static int ina2xx_probe(struct i2c_client *client,
>> +			const struct i2c_device_id *id)
>> +{
>> +	struct ina2xx_chip_info *chip;
>> +	struct iio_dev *indio_dev;
>> +	struct iio_buffer *buffer;
>> +	int ret;
>> +	unsigned int val;
>> +
>> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
>> +	if (!indio_dev)
>> +		return -ENOMEM;
>> +
>> +	chip = iio_priv(indio_dev);
>> +
>> +	chip->config = &ina2xx_config[id->driver_data];
>> +
>> +	if (of_property_read_u32(client->dev.of_node,
>> +				 "shunt-resistor", &val) < 0) {
>> +		struct ina2xx_platform_data *pdata >> +		    dev_get_platdata(&client->dev);
>> +
>> +		if (pdata)
>> +			val = pdata->shunt_uohms;
>> +		else
>> +			val = INA2XX_RSHUNT_DEFAULT;
>> +	}
>> +
>> +	if (val <= 0 || val > chip->config->calibration_factor)
>> +		return -ENODEV;
>> +
>> +	chip->rshunt = val;
>> +
>> +	ina2xx_regmap_config.max_register = chip->config->registers;
>> +
>> +	mutex_init(&chip->state_lock);
>> +
>> +	/* this is only used for device removal purposes */
>> +	i2c_set_clientdata(client, indio_dev);
>> +
>> +	indio_dev->name = id->name;
>> +	indio_dev->channels = ina2xx_channels;
>> +	indio_dev->num_channels = ARRAY_SIZE(ina2xx_channels);
>> +
>> +	indio_dev->dev.parent = &client->dev;
>> +	indio_dev->info = &ina2xx_info;
>> +	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
>> +
>> +	chip->regmap = devm_regmap_init_i2c(client, &ina2xx_regmap_config);
>> +	if (IS_ERR(chip->regmap)) {
>> +		dev_err(&client->dev, "failed to allocate register map\n");
>> +		return PTR_ERR(chip->regmap);
>> +	}
>> +
>> +	/* Patch the current config register with default. */
>> +	val = chip->config->config_default;
>> +
>> +	if (id->driver_data = ina226) {
>> +		ina226_set_average(chip, INA226_DEFAULT_AVG, &val);
>> +		ina226_set_itb(chip, INA226_DEFAULT_IT, &val);
>> +		ina226_set_its(chip, INA226_DEFAULT_IT, &val);
>> +	}
>> +
>> +	ret = ina2xx_init(chip, val);
>> +	if (ret < 0) {
>> +		dev_err(&client->dev, "error configuring the device: %d\n",
>> +			ret);
>> +		return -ENODEV;
>> +	}
>> +
>> +	buffer = devm_iio_kfifo_allocate(&indio_dev->dev);
>> +	if (!buffer)
>> +		return -ENOMEM;
>> +
>> +	indio_dev->setup_ops = &ina2xx_setup_ops;
>> +
>> +	iio_device_attach_buffer(indio_dev, buffer);
>> +
>> +	return devm_iio_device_register(&client->dev, indio_dev);
>> +}
>> +
>> +
>> +static int ina2xx_remove(struct i2c_client *client)
>> +{
>> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
>> +	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	mutex_destroy(&chip->state_lock);
>> +
>> +	/* Powerdown */
>> +	ret = regmap_update_bits(chip->regmap, INA2XX_CONFIG,
>> +				  INA2XX_MODE_MASK, 0);
>> +
>> +	iio_device_unregister(indio_dev);
> Peter already covered this to a degree.
>
> iio_device_unregister removes the userspace interface - hence it
> must be the first thing done in remove.
>
> If you use devm_ version of register it will occur at the end of the
> remove.  Hence if the remove has anything in it, you pretty much
> can't use the devm_iio_device_register without introducing a nasty
> race condition.  Hence you don't want the devm version here and you
> want to move iio_device_unregister to the beginning of this function.
>> +
>> +	return ret;
>> +}
>> +
>> +
>> +static const struct i2c_device_id ina2xx_id[] = {
>> +	{"ina219", ina219},
>> +	{"ina220", ina219},
>> +	{"ina226", ina226},
>> +	{"ina230", ina226},
>> +	{"ina231", ina226},
>> +	{}
>> +};
>> +
>> +MODULE_DEVICE_TABLE(i2c, ina2xx_id);
>> +
>> +static struct i2c_driver ina2xx_driver = {
>> +	.driver = {
>> +		   .name = KBUILD_MODNAME,
>> +	},
>> +	.probe = ina2xx_probe,
>> +	.remove = ina2xx_remove,
>> +	.id_table = ina2xx_id,
>> +};
>> +
>> +module_i2c_driver(ina2xx_driver);
>> +
>> +MODULE_AUTHOR("Marc Titinger <marc.titinger@baylibre.com>");
>> +MODULE_DESCRIPTION("Texas Instruments INA2XX ADC driver");
>> +MODULE_LICENSE("GPL v2");
>>
>
>


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [PATCH 1/2] iio: ina2xx: add support for TI INA2xx Power Monitors

[Marc Titinger]

On 29/11/2015 18:38, Guenter Roeck wrote:
> On 11/29/2015 08:31 AM, Jonathan Cameron wrote:
>> On 25/11/15 11:28, Marc Titinger wrote:
>>> in SOFTWARE buffer mode, a kthread will capture the active scan_elements
>>> into a kfifo, then compute the remaining time until the next capture
>>> tick
>>> and do an active wait (udelay).
>>>
>>> This will produce a stream of up to fours channels plus a 64bits
>>> timestamps (ns).
>>>
>>> Tested with ina226, on BeagleBoneBlack.
>>>
>>> Datasheet: http://www.ti.com/lit/gpn/ina226
>>>
>>> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
>> A few more bits from me, but basically looking good.
>>
>> We do however, need to make the hwmon guys aware this is going on.
>> Please cc their list on the next version.
>>
>> Last thing we want is for this to turn up as a surprise!
>>
>
> I have seen the original patch, so no surprise here. Just not sure
> if we should remove the hwmon driver after the iio driver is accepted.
> Even though the stated goal is different, it seems to me that having
> two drivers really does not make sense. Any thoughts ?

Hi Guenter,

we for instance have two completely unrelated projects both using this 
chip, on one side we wish to measure power consumption on the host board 
and hwmon has the apis and tool ecosystem for it, while on the "ACME" 
system we needed IIO because it's buffer streaming scheme and remote 
features saves the day to create a lab instrument. But really both have 
their use, I'd recommend keeping the hwmon driver for now. I doubt those 
drivers will generate much maintenance work in the future either.

Kind Regards,
Marc.



>
> Guenter
>
>> Jonathan
>>> ---
>>>   drivers/iio/adc/Kconfig      |  10 +
>>>   drivers/iio/adc/Makefile     |   1 +
>>>   drivers/iio/adc/ina2xx-iio.c | 684
>>> +++++++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 695 insertions(+)
>>>   create mode 100644 drivers/iio/adc/ina2xx-iio.c
>>>
>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>> index eb0cd89..9b87372 100644
>>> --- a/drivers/iio/adc/Kconfig
>>> +++ b/drivers/iio/adc/Kconfig
>>> @@ -170,6 +170,16 @@ config EXYNOS_ADC
>>>         of SoCs for drivers such as the touchscreen and hwmon to use
>>> to share
>>>         this resource.
>>>
>>> +config INA2XX_IIO
>>> +    tristate "Texas Instruments INA2xx Power Monitors IIO driver"
>>> +    depends on I2C
>>> +    select REGMAP_I2C
>>> +    select IIO_BUFFER
>>> +    help
>>> +      Say yes here to build support for TI INA2xx familly Power
>>> Monitors.
>>> +
>>> +      Note that this is not the existing hwmon interface for this
>>> device.
>>> +
>>>   config LP8788_ADC
>>>       tristate "LP8788 ADC driver"
>>>       depends on MFD_LP8788
>>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>>> index a096210..74e4341 100644
>>> --- a/drivers/iio/adc/Makefile
>>> +++ b/drivers/iio/adc/Makefile
>>> @@ -19,6 +19,7 @@ obj-$(CONFIG_BERLIN2_ADC) += berlin2-adc.o
>>>   obj-$(CONFIG_DA9150_GPADC) += da9150-gpadc.o
>>>   obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o
>>>   obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
>>> +obj-$(CONFIG_INA2XX_IIO) += ina2xx-iio.o
>>>   obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>>>   obj-$(CONFIG_MAX1027) += max1027.o
>>>   obj-$(CONFIG_MAX1363) += max1363.o
>>> diff --git a/drivers/iio/adc/ina2xx-iio.c b/drivers/iio/adc/ina2xx-iio.c
>>> new file mode 100644
>>> index 0000000..4a0026c
>>> --- /dev/null
>>> +++ b/drivers/iio/adc/ina2xx-iio.c
>>> @@ -0,0 +1,684 @@
>>> +/*
>>> + * INA2XX Current and Power Monitors
>>> + *
>>> + * Copyright 2015 Baylibre SAS.
>>> + *
>>> + * 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.
>>> + *
>>> + * Based on linux/drivers/iio/adc/ad7291.c
>>> + * Copyright 2010-2011 Analog Devices Inc.
>>> + *
>>> + * Based on linux/drivers/hwmon/ina2xx.c
>>> + * Copyright 2012 Lothar Felten <l-felten@ti.com>
>>> + *
>>> + * Licensed under the GPL-2 or later.
>>> + *
>>> + * IIO driver for INA219-220-226-230-231
>>> + *
>>> + * Configurable 7-bit I2C slave address from 0x40 to 0x4F
>>> + */
>>> +#include <linux/module.h>
>>> +#include <linux/kthread.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/iio/kfifo_buf.h>
>>> +#include <linux/iio/sysfs.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/platform_data/ina2xx.h>
>>> +
>>> +#include <linux/util_macros.h>
>>> +
>>> +/*
>>> + * INA2XX registers definition
>>> + */
>>> +/* common register definitions */
>>> +#define INA2XX_CONFIG                   0x00
>>> +#define INA2XX_SHUNT_VOLTAGE            0x01    /* readonly */
>>> +#define INA2XX_BUS_VOLTAGE              0x02    /* readonly */
>>> +#define INA2XX_POWER                    0x03    /* readonly */
>>> +#define INA2XX_CURRENT                  0x04    /* readonly */
>>> +#define INA2XX_CALIBRATION              0x05
>>> +
>>> +#define INA226_ALERT_MASK        0x06
>>> +#define INA266_CVRF            BIT(3)
>>> +
>>> +/* register count */
>>> +#define INA219_REGISTERS                6
>>> +#define INA226_REGISTERS                8
>>> +#define INA2XX_MAX_REGISTERS            8
>>> +
>>> +/* settings - depend on use case */
>>> +#define INA219_CONFIG_DEFAULT           0x399F    /* PGA=8 */
>>> +#define INA226_CONFIG_DEFAULT           0x4327
>>> +#define INA226_DEFAULT_AVG              4
>>> +#define INA226_DEFAULT_IT        1110
>>> +
>>> +#define INA2XX_RSHUNT_DEFAULT           10000
>>> +
>>> +/*
>>> + * bit mask for reading the averaging setting in the configuration
>>> register
>>> + * FIXME: use regmap_fields.
>>> + */
>>> +#define INA2XX_MODE_MASK    GENMASK(3, 0)
>>> +
>>> +#define INA226_AVG_MASK        GENMASK(11, 9)
>>> +#define INA226_SHIFT_AVG(val)    ((val) << 9)
>>> +
>>> +/* Integration time for VBus */
>>> +#define INA226_ITB_MASK        GENMASK(8, 6)
>>> +#define INA226_SHIFT_ITB(val)    ((val) << 6)
>>> +
>>> +/*Integration Time for VShunt */
>>> +#define INA226_ITS_MASK        GENMASK(5, 3)
>>> +#define INA226_SHIFT_ITS(val)    ((val) << 3)
>>> +
>>> +
>>> +static bool ina2xx_is_writeable_reg(struct device *dev, unsigned int
>>> reg)
>>> +{
>>> +    return (reg == INA2XX_CONFIG) || (reg > INA2XX_CURRENT);
>>> +}
>>> +
>>> +static bool ina2xx_is_volatile_reg(struct device *dev, unsigned int
>>> reg)
>>> +{
>>> +    return (reg != INA2XX_CONFIG);
>>> +}
>>> +
>>> +static struct regmap_config ina2xx_regmap_config = {
>>> +    .reg_bits = 8,
>>> +    .val_bits = 16,
>>> +
>>> +    .writeable_reg = ina2xx_is_writeable_reg,
>>> +    .volatile_reg = ina2xx_is_volatile_reg,
>>> +};
>>> +
>>> +enum ina2xx_ids { ina219, ina226 };
>>> +
>>> +struct ina2xx_config {
>>> +    u16 config_default;
>>> +    int calibration_factor;
>>> +    int registers;
>>> +    int shunt_div;
>>> +    int bus_voltage_shift;
>>> +    int bus_voltage_lsb;    /* uV */
>>> +    int power_lsb;        /* uW */
>>> +};
>>> +
>>> +struct ina2xx_chip_info {
>>> +    struct regmap *regmap;
>>> +    struct task_struct *task;
>>> +    const struct ina2xx_config *config;
>>> +    struct mutex state_lock;
>>> +    long rshunt;
>>> +    int avg;
>>> +    int itb; /* Bus voltage integration time uS */
>>> +    int its; /* Shunt voltage integration tim uS */
>>> +};
>>> +
>>> +static const struct ina2xx_config ina2xx_config[] = {
>>> +    [ina219] = {
>>> +            .config_default = INA219_CONFIG_DEFAULT,
>>> +            .calibration_factor = 40960000,
>>> +            .registers = INA219_REGISTERS,
>>> +            .shunt_div = 100,
>>> +            .bus_voltage_shift = 3,
>>> +            .bus_voltage_lsb = 4000,
>>> +            .power_lsb = 20000,
>>> +            },
>>> +    [ina226] = {
>>> +            .config_default = INA226_CONFIG_DEFAULT,
>>> +            .calibration_factor = 5120000,
>>> +            .registers = INA226_REGISTERS,
>>> +            .shunt_div = 400,
>>> +            .bus_voltage_shift = 0,
>>> +            .bus_voltage_lsb = 1250,
>>> +            .power_lsb = 25000,
>>> +            },
>>> +};
>>> +
>>> +static int ina2xx_get_value(struct ina2xx_chip_info *chip, u8 reg,
>>> +                unsigned int regval, int *val, int *uval)
>>> +{
>>> +    *val = 0;
>>> +
>>> +    switch (reg) {
>>> +    case INA2XX_SHUNT_VOLTAGE:
>>> +        /* signed register */
>>> +        *uval = DIV_ROUND_CLOSEST((s16) regval,
>>> +                      chip->config->shunt_div);
>>> +        return IIO_VAL_INT_PLUS_MICRO;
>>> +
>>> +    case INA2XX_BUS_VOLTAGE:
>>> +        *uval = (regval >> chip->config->bus_voltage_shift)
>>> +            * chip->config->bus_voltage_lsb;
>>> +        *val = *uval/1000000;
>>> +        *uval = *uval % 1000000;
>>> +        return IIO_VAL_INT_PLUS_MICRO;
>>> +
>>> +    case INA2XX_POWER:
>>> +        *uval = regval * chip->config->power_lsb;
>>> +        *val = *uval/1000000;
>>> +        *uval = *uval % 1000000;
>>> +        return IIO_VAL_INT_PLUS_MICRO;
>>> +
>>> +    case INA2XX_CURRENT:
>>> +        /* signed register, LSB=1mA (selected), in mA */
>>> +        *uval = (s16) regval * 1000;
>>> +        return IIO_VAL_INT_PLUS_MICRO;
>>> +
>>> +    default:
>>> +        /* programmer goofed */
>>> +        WARN_ON_ONCE(1);
>>> +    }
>>> +    return -EINVAL;
>>> +}
>>> +
>>> +static int ina2xx_read_raw(struct iio_dev *indio_dev,
>>> +               struct iio_chan_spec const *chan,
>>> +               int *val, int *val2, long mask)
>>> +{
>>> +    int ret;
>>> +    struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>>> +    unsigned int regval;
>>> +
>>> +    switch (mask) {
>>> +    case IIO_CHAN_INFO_RAW:
>>> +        ret = regmap_read(chip->regmap, chan->address, &regval);
>>> +        if (ret < 0)
>>> +            return ret;
>>> +
>>> +        return ina2xx_get_value(chip, chan->address, regval, val,
>>> val2);
>>> +
>>> +    case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>>> +        *val = chip->avg;
>>> +        return IIO_VAL_INT;
>>> +
>>> +    case IIO_CHAN_INFO_INT_TIME:
>>> +        *val = 0;
>>> +        if (chan->address == INA2XX_SHUNT_VOLTAGE)
>>> +            *val2 = chip->its;
>>> +        else
>>> +            *val2 = chip->itb;
>>> +
>>> +        return IIO_VAL_INT_PLUS_MICRO;
>>> +    /*
>>> +     * sample freq is read only, it is a consequence of
>>> +     * 1/AVG*(CT_bus+CT_shunt)
>>> +     */
>>> +    case IIO_CHAN_INFO_SAMP_FREQ:
>>> +        *val = DIV_ROUND_CLOSEST(1000000,
>>> +               (chip->itb + chip->its) * chip->avg);
>>> +
>>> +        return IIO_VAL_INT;
>>> +
>>> +    default:
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +/*
>>> + * Available averaging rates for ina226. The indices correspond with
>>> + * the bit values expected by the chip (according to the ina226
>>> datasheet,
>>> + * table 3 AVG bit settings, found at
>>> + * http://www.ti.com/lit/ds/symlink/ina226.pdf.
>>> + */
>>> +static const int ina226_avg_tab[] = { 1, 4, 16, 64, 128, 256, 512,
>>> 1024 };
>>> +
>>> +static unsigned int ina226_set_average(struct ina2xx_chip_info *chip,
>>> +                       unsigned int val,
>>> +                       unsigned int *config)
>>> +{
>>> +    int bits;
>>> +
>>> +    if (val > 1024 || val < 1)
>>> +        return -EINVAL;
>>> +
>>> +    bits = find_closest(val, ina226_avg_tab,
>>> +                ARRAY_SIZE(ina226_avg_tab));
>>> +
>>> +    chip->avg = ina226_avg_tab[bits];
>>> +
>>> +    *config &= ~INA226_AVG_MASK;
>>> +    *config |= INA226_SHIFT_AVG(bits) & INA226_AVG_MASK;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +/* Conversion times in uS */
>>> +static const int ina226_conv_time_tab[] = { 140, 204, 332, 588, 1100,
>>> +                        2116, 4156, 8244};
>>> +
>>> +static unsigned int ina226_set_itb(struct ina2xx_chip_info *chip,
>>> +                   unsigned int val,
>>> +                   unsigned int *config)
>>> +{
>>> +    int bits;
>>> +
>>> +    if (val > 8244 || val < 140)
>>> +        return -EINVAL;
>>> +
>>> +    bits = find_closest(val, ina226_conv_time_tab,
>>> +            ARRAY_SIZE(ina226_conv_time_tab));
>>> +
>>> +    chip->itb = ina226_conv_time_tab[bits];
>>> +
>>> +    *config &= ~INA226_ITB_MASK;
>>> +    *config |= INA226_SHIFT_ITB(bits) & INA226_ITB_MASK;
>>> +
>>> +    return 0;
>>> +}
>>> +
>> Could do with a slightly more informative name than its vs itb.
>> Or at least some docs explaining what it does.
>>> +static unsigned int ina226_set_its(struct ina2xx_chip_info *chip,
>>> +                   unsigned int val,
>>> +                   unsigned int *config)
>>> +{
>>> +    int bits;
>>> +
>>> +    if (val > 8244 || val < 140)
>>> +        return -EINVAL;
>>> +
>>> +    bits = find_closest(val, ina226_conv_time_tab,
>>> +            ARRAY_SIZE(ina226_conv_time_tab));
>>> +
>>> +    chip->its = ina226_conv_time_tab[bits];
>>> +
>>> +    *config &= ~INA226_ITS_MASK;
>>> +    *config |= INA226_SHIFT_ITS(bits) & INA226_ITS_MASK;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +
>>> +static int ina2xx_write_raw(struct iio_dev *indio_dev,
>>> +                struct iio_chan_spec const *chan,
>>> +                int val, int val2, long mask)
>>> +{
>>> +    struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>>> +    int ret = 0;
>>> +    unsigned int config, tmp;
>>> +
>>> +    if (iio_buffer_enabled(indio_dev))
>>> +        return -EBUSY;
>>> +
>>> +    mutex_lock(&chip->state_lock);
>>> +
>>> +    ret = regmap_read(chip->regmap, INA2XX_CONFIG, &config);
>>> +    if (ret < 0)
>>> +        goto _err;
>>> +
>>> +    tmp = config;
>>> +
>>> +    switch (mask) {
>>> +    case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>>> +        ret = ina226_set_average(chip, val, &tmp);
>>> +        break;
>>> +
>>> +    case IIO_CHAN_INFO_INT_TIME:
>>> +        if (chan->address == INA2XX_SHUNT_VOLTAGE)
>>> +            ret = ina226_set_its(chip, val, &tmp);
>>> +        else
>>> +            ret = ina226_set_itb(chip, val, &tmp);
>>> +        break;
>>> +    default:
>>> +        ret = -EINVAL;
>>> +    }
>>> +
>>> +    if (!ret && (tmp != config))
>>> +        ret = regmap_write(chip->regmap, INA2XX_CONFIG, tmp);
>>> +_err:
>>> +    mutex_unlock(&chip->state_lock);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +
>>> +#define INA2XX_CHAN(_type, _index, _address) { \
>>> +    .type = _type, \
>>> +    .address = _address, \
>>> +    .indexed = 1, \
>>> +    .channel = (_index), \
>>> +    .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
>>> +    .info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
>>> +                   BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
>>> +    .scan_index = (_index), \
>>> +    .scan_type = { \
>>> +        .sign = 'u', \
>>> +        .realbits = 16, \
>>> +        .storagebits = 16, \
>>> +    .endianness = IIO_BE, \
>>> +    } \
>>> +}
>>> +
>>> +/*Sampling Freq is a consequence of the integration times of the V
>>> channels.*/
>>> +#define INA2XX_CHAN_VOLTAGE(_index, _address) { \
>>> +    .type = IIO_VOLTAGE, \
>>> +    .address = _address, \
>>> +    .indexed = 1, \
>>> +    .channel = (_index), \
>>> +    .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
>>> +                  BIT(IIO_CHAN_INFO_INT_TIME), \
>>> +    .scan_index = (_index), \
>>> +    .scan_type = { \
>>> +        .sign = 'u', \
>>> +        .realbits = 16, \
>>> +        .storagebits = 16, \
>>> +    .endianness = IIO_BE, \
>>> +    } \
>>> +}
>>> +
>>> +
>>> +static const struct iio_chan_spec ina2xx_channels[] = {
>>> +    INA2XX_CHAN_VOLTAGE(0, INA2XX_SHUNT_VOLTAGE),
>>> +    INA2XX_CHAN_VOLTAGE(1, INA2XX_BUS_VOLTAGE),
>>> +    INA2XX_CHAN(IIO_CURRENT, 2, INA2XX_CURRENT),
>>> +    INA2XX_CHAN(IIO_POWER, 3, INA2XX_POWER),
>>> +    IIO_CHAN_SOFT_TIMESTAMP(4),
>>> +};
>>> +
>>> +
>>> +static s64 prev_ns;
>> This prevents you having more than one instance of the device - move
>> it into
>> your chip_info.
>>> +
>>> +static int ina2xx_work_buffer(struct iio_dev *indio_dev)
>>> +{
>>> +    struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>>> +    unsigned short data[8];
>>> +    int bit, ret = 0, i = 0;
>>> +    unsigned long buffer_us = 0, elapsed_us = 0;
>>> +    s64 time_a, time_b;
>>> +    unsigned int alert;
>>> +
>>> +    time_a = iio_get_time_ns();
>>> +
>>> +    /*
>>> +     * Because the timer thread and the chip conversion clock
>>> +     * are asynchronous, the period difference will eventually
>>> +     * result in reading V[k-1] again, or skip V[k] at time Tk.
>>> +     * In order to resync the timer with the conversion process
>>> +     * we check the ConVersionReadyFlag.
>>> +     * On hardware that supports using the ALERT pin to toggle a
>>> +     * GPIO a triggered buffer could be used instead.
>>> +     * For now, we pay for that extra read of the ALERT register
>>> +     */
>>> +    do {
>>> +        ret = regmap_read(chip->regmap, INA226_ALERT_MASK,
>>> +                  &alert);
>>> +        if (ret < 0)
>>> +            goto _err;
>>> +
>>> +        alert &= INA266_CVRF;
>>> +        trace_printk("Conversion ready: %d\n", !!alert);
>>> +
>>> +    } while (!alert);
>>> +
>>> +    /*
>>> +     * Single register reads: bulk_read will not work with ina226
>>> +     * as there is no auto-increment of the address register for
>>> +     * data length longer than 16bits.
>>> +     */
>>> +    for_each_set_bit(bit, indio_dev->active_scan_mask,
>>> +             indio_dev->masklength) {
>>> +        unsigned int val;
>>> +
>>> +        ret = regmap_read(chip->regmap,
>>> +                  INA2XX_SHUNT_VOLTAGE + bit, &val);
>>> +        if (ret < 0)
>>> +            goto _err;
>>> +
>>> +        data[i++] = val;
>>> +    }
>>> +
>>> +    time_b = iio_get_time_ns();
>>> +
>>> +    iio_push_to_buffers_with_timestamp(indio_dev,
>>> +                       (unsigned int *)data, time_a);
>>> +
>>> +    buffer_us = (unsigned long)(time_b - time_a) / 1000;
>>> +    elapsed_us = (unsigned long)(time_a - prev_ns) / 1000;
>>> +
>>> +    trace_printk("uS: elapsed: %lu, buf: %lu\n", elapsed_us,
>>> buffer_us);
>>> +
>>> +    prev_ns = time_a;
>>> +
>>> +_err:
>>> +    return buffer_us;
>>> +};
>>> +
>>> +static int ina2xx_capture_thread(void *data)
>>> +{
>>> +    struct iio_dev *indio_dev = (struct iio_dev *)data;
>>> +    struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>>> +    unsigned int sampling_us = (chip->itb + chip->its) * chip->avg;
>>> +    unsigned long buffer_us;
>>> +
>>> +    /*
>>> +     * Poll a bit faster than the chip internal Fs, in case
>>> +     * we wish to sync with the conversion ready flag.
>>> +     */
>>> +    sampling_us -= 200;
>>> +
>>> +    do {
>>> +        buffer_us = ina2xx_work_buffer(indio_dev);
>>> +
>>> +        if (sampling_us > buffer_us)
>>> +            udelay(sampling_us - buffer_us);
>>> +
>>> +    } while (!kthread_should_stop());
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +int ina2xx_buffer_enable(struct iio_dev *indio_dev)
>>> +{
>>> +    struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>>> +    unsigned int sampling_us = (chip->itb + chip->its) * chip->avg;
>>> +
>>> +    trace_printk("Enabling buffer w/ scan_mask %02x, freq = %d, avg
>>> =%u\n",
>>> +             (unsigned int)(*indio_dev->active_scan_mask),
>>> +             1000000/sampling_us, chip->avg);
>>> +
>>> +    trace_printk("Expected work period: %u us\n", sampling_us);
>>> +
>>> +    prev_ns = iio_get_time_ns();
>>> +
>>> +    chip->task = kthread_run(ina2xx_capture_thread, (void *)indio_dev,
>>> +                 "ina2xx-%uus", sampling_us);
>>> +
>>> +    return PTR_ERR_OR_ZERO(chip->task);
>>> +}
>>> +
>>> +int ina2xx_buffer_disable(struct iio_dev *indio_dev)
>>> +{
>>> +    struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>>> +
>>> +    if (chip->task) {
>>> +        kthread_stop(chip->task);
>>> +        chip->task = NULL;
>>> +    }
>> nitpick: blank line here.
>>> +    return 0;
>>> +}
>>> +
>>> +static const struct iio_buffer_setup_ops ina2xx_setup_ops = {
>>> +    .postenable = &ina2xx_buffer_enable,
>>> +    .postdisable = &ina2xx_buffer_disable,
>>> +};
>>> +
>>> +static int ina2xx_debug_reg(struct iio_dev *indio_dev,
>>> +                unsigned reg, unsigned writeval, unsigned *readval)
>>> +{
>>> +    struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>>> +
>>> +    if (!readval)
>>> +        return regmap_write(chip->regmap, reg, writeval);
>>> +
>>> +    return regmap_read(chip->regmap, reg, readval);
>>> +}
>>> +
>>> +/* frequencies matching the cummulated integration times for vshunt
>>> and vbus */
>> huh?  Integration time is in seconds... These seem unlikely to be valid
>> settings.
>>> +static IIO_CONST_ATTR_INT_TIME_AVAIL("140 204 332 588 1100 2116 4156
>>> 8244");
>>> +
>>> +static struct attribute *ina2xx_attributes[] = {
>>> +    &iio_const_attr_integration_time_available.dev_attr.attr,
>>> +    NULL,
>>> +};
>>> +
>>> +static const struct attribute_group ina2xx_attribute_group = {
>>> +    .attrs = ina2xx_attributes,
>>> +};
>>> +
>>> +static const struct iio_info ina2xx_info = {
>>> +    .debugfs_reg_access = &ina2xx_debug_reg,
>>> +    .read_raw = &ina2xx_read_raw,
>>> +    .write_raw = &ina2xx_write_raw,
>>> +    .attrs = &ina2xx_attribute_group,
>>> +    .driver_module = THIS_MODULE,
>>> +};
>>> +
>>> +/* Initialize the configuration and calibration registers. */
>>> +static int ina2xx_init(struct ina2xx_chip_info *chip, unsigned int
>>> config)
>>> +{
>>> +    u16 regval;
>>> +    int ret = regmap_write(chip->regmap, INA2XX_CONFIG, config);
>>> +
>>> +    if (ret < 0)
>>> +        return ret;
>>> +    /*
>>> +     * Set current LSB to 1mA, shunt is in uOhms
>>> +     * (equation 13 in datasheet). We hardcode a Current_LSB
>>> +     * of 1.0 x10-6. The only remaining parameter is RShunt
>>> +     * There is no need to expose the CALIBRATION register
>>> +     * to the user for now.
>>> +     */
>>> +    regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor,
>>> +                chip->rshunt);
>>> +
>>> +    return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval);
>>> +}
>>> +
>>> +static int ina2xx_probe(struct i2c_client *client,
>>> +            const struct i2c_device_id *id)
>>> +{
>>> +    struct ina2xx_chip_info *chip;
>>> +    struct iio_dev *indio_dev;
>>> +    struct iio_buffer *buffer;
>>> +    int ret;
>>> +    unsigned int val;
>>> +
>>> +    indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
>>> +    if (!indio_dev)
>>> +        return -ENOMEM;
>>> +
>>> +    chip = iio_priv(indio_dev);
>>> +
>>> +    chip->config = &ina2xx_config[id->driver_data];
>>> +
>>> +    if (of_property_read_u32(client->dev.of_node,
>>> +                 "shunt-resistor", &val) < 0) {
>>> +        struct ina2xx_platform_data *pdata =
>>> +            dev_get_platdata(&client->dev);
>>> +
>>> +        if (pdata)
>>> +            val = pdata->shunt_uohms;
>>> +        else
>>> +            val = INA2XX_RSHUNT_DEFAULT;
>>> +    }
>>> +
>>> +    if (val <= 0 || val > chip->config->calibration_factor)
>>> +        return -ENODEV;
>>> +
>>> +    chip->rshunt = val;
>>> +
>>> +    ina2xx_regmap_config.max_register = chip->config->registers;
>>> +
>>> +    mutex_init(&chip->state_lock);
>>> +
>>> +    /* this is only used for device removal purposes */
>>> +    i2c_set_clientdata(client, indio_dev);
>>> +
>>> +    indio_dev->name = id->name;
>>> +    indio_dev->channels = ina2xx_channels;
>>> +    indio_dev->num_channels = ARRAY_SIZE(ina2xx_channels);
>>> +
>>> +    indio_dev->dev.parent = &client->dev;
>>> +    indio_dev->info = &ina2xx_info;
>>> +    indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
>>> +
>>> +    chip->regmap = devm_regmap_init_i2c(client, &ina2xx_regmap_config);
>>> +    if (IS_ERR(chip->regmap)) {
>>> +        dev_err(&client->dev, "failed to allocate register map\n");
>>> +        return PTR_ERR(chip->regmap);
>>> +    }
>>> +
>>> +    /* Patch the current config register with default. */
>>> +    val = chip->config->config_default;
>>> +
>>> +    if (id->driver_data == ina226) {
>>> +        ina226_set_average(chip, INA226_DEFAULT_AVG, &val);
>>> +        ina226_set_itb(chip, INA226_DEFAULT_IT, &val);
>>> +        ina226_set_its(chip, INA226_DEFAULT_IT, &val);
>>> +    }
>>> +
>>> +    ret = ina2xx_init(chip, val);
>>> +    if (ret < 0) {
>>> +        dev_err(&client->dev, "error configuring the device: %d\n",
>>> +            ret);
>>> +        return -ENODEV;
>>> +    }
>>> +
>>> +    buffer = devm_iio_kfifo_allocate(&indio_dev->dev);
>>> +    if (!buffer)
>>> +        return -ENOMEM;
>>> +
>>> +    indio_dev->setup_ops = &ina2xx_setup_ops;
>>> +
>>> +    iio_device_attach_buffer(indio_dev, buffer);
>>> +
>>> +    return devm_iio_device_register(&client->dev, indio_dev);
>>> +}
>>> +
>>> +
>>> +static int ina2xx_remove(struct i2c_client *client)
>>> +{
>>> +    struct iio_dev *indio_dev = i2c_get_clientdata(client);
>>> +    struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>>> +    int ret;
>>> +
>>> +    mutex_destroy(&chip->state_lock);
>>> +
>>> +    /* Powerdown */
>>> +    ret = regmap_update_bits(chip->regmap, INA2XX_CONFIG,
>>> +                  INA2XX_MODE_MASK, 0);
>>> +
>>> +    iio_device_unregister(indio_dev);
>> Peter already covered this to a degree.
>>
>> iio_device_unregister removes the userspace interface - hence it
>> must be the first thing done in remove.
>>
>> If you use devm_ version of register it will occur at the end of the
>> remove.  Hence if the remove has anything in it, you pretty much
>> can't use the devm_iio_device_register without introducing a nasty
>> race condition.  Hence you don't want the devm version here and you
>> want to move iio_device_unregister to the beginning of this function.
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +
>>> +static const struct i2c_device_id ina2xx_id[] = {
>>> +    {"ina219", ina219},
>>> +    {"ina220", ina219},
>>> +    {"ina226", ina226},
>>> +    {"ina230", ina226},
>>> +    {"ina231", ina226},
>>> +    {}
>>> +};
>>> +
>>> +MODULE_DEVICE_TABLE(i2c, ina2xx_id);
>>> +
>>> +static struct i2c_driver ina2xx_driver = {
>>> +    .driver = {
>>> +           .name = KBUILD_MODNAME,
>>> +    },
>>> +    .probe = ina2xx_probe,
>>> +    .remove = ina2xx_remove,
>>> +    .id_table = ina2xx_id,
>>> +};
>>> +
>>> +module_i2c_driver(ina2xx_driver);
>>> +
>>> +MODULE_AUTHOR("Marc Titinger <marc.titinger@baylibre.com>");
>>> +MODULE_DESCRIPTION("Texas Instruments INA2XX ADC driver");
>>> +MODULE_LICENSE("GPL v2");
>>>
>>
>>
>

Re: [lm-sensors] [PATCH 1/2] iio: ina2xx: add support for TI INA2xx Power Monitors

[Marc Titinger]

On 29/11/2015 18:38, Guenter Roeck wrote:
> On 11/29/2015 08:31 AM, Jonathan Cameron wrote:
>> On 25/11/15 11:28, Marc Titinger wrote:
>>> in SOFTWARE buffer mode, a kthread will capture the active scan_elements
>>> into a kfifo, then compute the remaining time until the next capture
>>> tick
>>> and do an active wait (udelay).
>>>
>>> This will produce a stream of up to fours channels plus a 64bits
>>> timestamps (ns).
>>>
>>> Tested with ina226, on BeagleBoneBlack.
>>>
>>> Datasheet: http://www.ti.com/lit/gpn/ina226
>>>
>>> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
>> A few more bits from me, but basically looking good.
>>
>> We do however, need to make the hwmon guys aware this is going on.
>> Please cc their list on the next version.
>>
>> Last thing we want is for this to turn up as a surprise!
>>
>
> I have seen the original patch, so no surprise here. Just not sure
> if we should remove the hwmon driver after the iio driver is accepted.
> Even though the stated goal is different, it seems to me that having
> two drivers really does not make sense. Any thoughts ?

Hi Guenter,

we for instance have two completely unrelated projects both using this 
chip, on one side we wish to measure power consumption on the host board 
and hwmon has the apis and tool ecosystem for it, while on the "ACME" 
system we needed IIO because it's buffer streaming scheme and remote 
features saves the day to create a lab instrument. But really both have 
their use, I'd recommend keeping the hwmon driver for now. I doubt those 
drivers will generate much maintenance work in the future either.

Kind Regards,
Marc.



>
> Guenter
>
>> Jonathan
>>> ---
>>>   drivers/iio/adc/Kconfig      |  10 +
>>>   drivers/iio/adc/Makefile     |   1 +
>>>   drivers/iio/adc/ina2xx-iio.c | 684
>>> +++++++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 695 insertions(+)
>>>   create mode 100644 drivers/iio/adc/ina2xx-iio.c
>>>
>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>> index eb0cd89..9b87372 100644
>>> --- a/drivers/iio/adc/Kconfig
>>> +++ b/drivers/iio/adc/Kconfig
>>> @@ -170,6 +170,16 @@ config EXYNOS_ADC
>>>         of SoCs for drivers such as the touchscreen and hwmon to use
>>> to share
>>>         this resource.
>>>
>>> +config INA2XX_IIO
>>> +    tristate "Texas Instruments INA2xx Power Monitors IIO driver"
>>> +    depends on I2C
>>> +    select REGMAP_I2C
>>> +    select IIO_BUFFER
>>> +    help
>>> +      Say yes here to build support for TI INA2xx familly Power
>>> Monitors.
>>> +
>>> +      Note that this is not the existing hwmon interface for this
>>> device.
>>> +
>>>   config LP8788_ADC
>>>       tristate "LP8788 ADC driver"
>>>       depends on MFD_LP8788
>>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>>> index a096210..74e4341 100644
>>> --- a/drivers/iio/adc/Makefile
>>> +++ b/drivers/iio/adc/Makefile
>>> @@ -19,6 +19,7 @@ obj-$(CONFIG_BERLIN2_ADC) += berlin2-adc.o
>>>   obj-$(CONFIG_DA9150_GPADC) += da9150-gpadc.o
>>>   obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o
>>>   obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
>>> +obj-$(CONFIG_INA2XX_IIO) += ina2xx-iio.o
>>>   obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>>>   obj-$(CONFIG_MAX1027) += max1027.o
>>>   obj-$(CONFIG_MAX1363) += max1363.o
>>> diff --git a/drivers/iio/adc/ina2xx-iio.c b/drivers/iio/adc/ina2xx-iio.c
>>> new file mode 100644
>>> index 0000000..4a0026c
>>> --- /dev/null
>>> +++ b/drivers/iio/adc/ina2xx-iio.c
>>> @@ -0,0 +1,684 @@
>>> +/*
>>> + * INA2XX Current and Power Monitors
>>> + *
>>> + * Copyright 2015 Baylibre SAS.
>>> + *
>>> + * 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.
>>> + *
>>> + * Based on linux/drivers/iio/adc/ad7291.c
>>> + * Copyright 2010-2011 Analog Devices Inc.
>>> + *
>>> + * Based on linux/drivers/hwmon/ina2xx.c
>>> + * Copyright 2012 Lothar Felten <l-felten@ti.com>
>>> + *
>>> + * Licensed under the GPL-2 or later.
>>> + *
>>> + * IIO driver for INA219-220-226-230-231
>>> + *
>>> + * Configurable 7-bit I2C slave address from 0x40 to 0x4F
>>> + */
>>> +#include <linux/module.h>
>>> +#include <linux/kthread.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/iio/kfifo_buf.h>
>>> +#include <linux/iio/sysfs.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/platform_data/ina2xx.h>
>>> +
>>> +#include <linux/util_macros.h>
>>> +
>>> +/*
>>> + * INA2XX registers definition
>>> + */
>>> +/* common register definitions */
>>> +#define INA2XX_CONFIG                   0x00
>>> +#define INA2XX_SHUNT_VOLTAGE            0x01    /* readonly */
>>> +#define INA2XX_BUS_VOLTAGE              0x02    /* readonly */
>>> +#define INA2XX_POWER                    0x03    /* readonly */
>>> +#define INA2XX_CURRENT                  0x04    /* readonly */
>>> +#define INA2XX_CALIBRATION              0x05
>>> +
>>> +#define INA226_ALERT_MASK        0x06
>>> +#define INA266_CVRF            BIT(3)
>>> +
>>> +/* register count */
>>> +#define INA219_REGISTERS                6
>>> +#define INA226_REGISTERS                8
>>> +#define INA2XX_MAX_REGISTERS            8
>>> +
>>> +/* settings - depend on use case */
>>> +#define INA219_CONFIG_DEFAULT           0x399F    /* PGA=8 */
>>> +#define INA226_CONFIG_DEFAULT           0x4327
>>> +#define INA226_DEFAULT_AVG              4
>>> +#define INA226_DEFAULT_IT        1110
>>> +
>>> +#define INA2XX_RSHUNT_DEFAULT           10000
>>> +
>>> +/*
>>> + * bit mask for reading the averaging setting in the configuration
>>> register
>>> + * FIXME: use regmap_fields.
>>> + */
>>> +#define INA2XX_MODE_MASK    GENMASK(3, 0)
>>> +
>>> +#define INA226_AVG_MASK        GENMASK(11, 9)
>>> +#define INA226_SHIFT_AVG(val)    ((val) << 9)
>>> +
>>> +/* Integration time for VBus */
>>> +#define INA226_ITB_MASK        GENMASK(8, 6)
>>> +#define INA226_SHIFT_ITB(val)    ((val) << 6)
>>> +
>>> +/*Integration Time for VShunt */
>>> +#define INA226_ITS_MASK        GENMASK(5, 3)
>>> +#define INA226_SHIFT_ITS(val)    ((val) << 3)
>>> +
>>> +
>>> +static bool ina2xx_is_writeable_reg(struct device *dev, unsigned int
>>> reg)
>>> +{
>>> +    return (reg = INA2XX_CONFIG) || (reg > INA2XX_CURRENT);
>>> +}
>>> +
>>> +static bool ina2xx_is_volatile_reg(struct device *dev, unsigned int
>>> reg)
>>> +{
>>> +    return (reg != INA2XX_CONFIG);
>>> +}
>>> +
>>> +static struct regmap_config ina2xx_regmap_config = {
>>> +    .reg_bits = 8,
>>> +    .val_bits = 16,
>>> +
>>> +    .writeable_reg = ina2xx_is_writeable_reg,
>>> +    .volatile_reg = ina2xx_is_volatile_reg,
>>> +};
>>> +
>>> +enum ina2xx_ids { ina219, ina226 };
>>> +
>>> +struct ina2xx_config {
>>> +    u16 config_default;
>>> +    int calibration_factor;
>>> +    int registers;
>>> +    int shunt_div;
>>> +    int bus_voltage_shift;
>>> +    int bus_voltage_lsb;    /* uV */
>>> +    int power_lsb;        /* uW */
>>> +};
>>> +
>>> +struct ina2xx_chip_info {
>>> +    struct regmap *regmap;
>>> +    struct task_struct *task;
>>> +    const struct ina2xx_config *config;
>>> +    struct mutex state_lock;
>>> +    long rshunt;
>>> +    int avg;
>>> +    int itb; /* Bus voltage integration time uS */
>>> +    int its; /* Shunt voltage integration tim uS */
>>> +};
>>> +
>>> +static const struct ina2xx_config ina2xx_config[] = {
>>> +    [ina219] = {
>>> +            .config_default = INA219_CONFIG_DEFAULT,
>>> +            .calibration_factor = 40960000,
>>> +            .registers = INA219_REGISTERS,
>>> +            .shunt_div = 100,
>>> +            .bus_voltage_shift = 3,
>>> +            .bus_voltage_lsb = 4000,
>>> +            .power_lsb = 20000,
>>> +            },
>>> +    [ina226] = {
>>> +            .config_default = INA226_CONFIG_DEFAULT,
>>> +            .calibration_factor = 5120000,
>>> +            .registers = INA226_REGISTERS,
>>> +            .shunt_div = 400,
>>> +            .bus_voltage_shift = 0,
>>> +            .bus_voltage_lsb = 1250,
>>> +            .power_lsb = 25000,
>>> +            },
>>> +};
>>> +
>>> +static int ina2xx_get_value(struct ina2xx_chip_info *chip, u8 reg,
>>> +                unsigned int regval, int *val, int *uval)
>>> +{
>>> +    *val = 0;
>>> +
>>> +    switch (reg) {
>>> +    case INA2XX_SHUNT_VOLTAGE:
>>> +        /* signed register */
>>> +        *uval = DIV_ROUND_CLOSEST((s16) regval,
>>> +                      chip->config->shunt_div);
>>> +        return IIO_VAL_INT_PLUS_MICRO;
>>> +
>>> +    case INA2XX_BUS_VOLTAGE:
>>> +        *uval = (regval >> chip->config->bus_voltage_shift)
>>> +            * chip->config->bus_voltage_lsb;
>>> +        *val = *uval/1000000;
>>> +        *uval = *uval % 1000000;
>>> +        return IIO_VAL_INT_PLUS_MICRO;
>>> +
>>> +    case INA2XX_POWER:
>>> +        *uval = regval * chip->config->power_lsb;
>>> +        *val = *uval/1000000;
>>> +        *uval = *uval % 1000000;
>>> +        return IIO_VAL_INT_PLUS_MICRO;
>>> +
>>> +    case INA2XX_CURRENT:
>>> +        /* signed register, LSB=1mA (selected), in mA */
>>> +        *uval = (s16) regval * 1000;
>>> +        return IIO_VAL_INT_PLUS_MICRO;
>>> +
>>> +    default:
>>> +        /* programmer goofed */
>>> +        WARN_ON_ONCE(1);
>>> +    }
>>> +    return -EINVAL;
>>> +}
>>> +
>>> +static int ina2xx_read_raw(struct iio_dev *indio_dev,
>>> +               struct iio_chan_spec const *chan,
>>> +               int *val, int *val2, long mask)
>>> +{
>>> +    int ret;
>>> +    struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>>> +    unsigned int regval;
>>> +
>>> +    switch (mask) {
>>> +    case IIO_CHAN_INFO_RAW:
>>> +        ret = regmap_read(chip->regmap, chan->address, &regval);
>>> +        if (ret < 0)
>>> +            return ret;
>>> +
>>> +        return ina2xx_get_value(chip, chan->address, regval, val,
>>> val2);
>>> +
>>> +    case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>>> +        *val = chip->avg;
>>> +        return IIO_VAL_INT;
>>> +
>>> +    case IIO_CHAN_INFO_INT_TIME:
>>> +        *val = 0;
>>> +        if (chan->address = INA2XX_SHUNT_VOLTAGE)
>>> +            *val2 = chip->its;
>>> +        else
>>> +            *val2 = chip->itb;
>>> +
>>> +        return IIO_VAL_INT_PLUS_MICRO;
>>> +    /*
>>> +     * sample freq is read only, it is a consequence of
>>> +     * 1/AVG*(CT_bus+CT_shunt)
>>> +     */
>>> +    case IIO_CHAN_INFO_SAMP_FREQ:
>>> +        *val = DIV_ROUND_CLOSEST(1000000,
>>> +               (chip->itb + chip->its) * chip->avg);
>>> +
>>> +        return IIO_VAL_INT;
>>> +
>>> +    default:
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +/*
>>> + * Available averaging rates for ina226. The indices correspond with
>>> + * the bit values expected by the chip (according to the ina226
>>> datasheet,
>>> + * table 3 AVG bit settings, found at
>>> + * http://www.ti.com/lit/ds/symlink/ina226.pdf.
>>> + */
>>> +static const int ina226_avg_tab[] = { 1, 4, 16, 64, 128, 256, 512,
>>> 1024 };
>>> +
>>> +static unsigned int ina226_set_average(struct ina2xx_chip_info *chip,
>>> +                       unsigned int val,
>>> +                       unsigned int *config)
>>> +{
>>> +    int bits;
>>> +
>>> +    if (val > 1024 || val < 1)
>>> +        return -EINVAL;
>>> +
>>> +    bits = find_closest(val, ina226_avg_tab,
>>> +                ARRAY_SIZE(ina226_avg_tab));
>>> +
>>> +    chip->avg = ina226_avg_tab[bits];
>>> +
>>> +    *config &= ~INA226_AVG_MASK;
>>> +    *config |= INA226_SHIFT_AVG(bits) & INA226_AVG_MASK;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +/* Conversion times in uS */
>>> +static const int ina226_conv_time_tab[] = { 140, 204, 332, 588, 1100,
>>> +                        2116, 4156, 8244};
>>> +
>>> +static unsigned int ina226_set_itb(struct ina2xx_chip_info *chip,
>>> +                   unsigned int val,
>>> +                   unsigned int *config)
>>> +{
>>> +    int bits;
>>> +
>>> +    if (val > 8244 || val < 140)
>>> +        return -EINVAL;
>>> +
>>> +    bits = find_closest(val, ina226_conv_time_tab,
>>> +            ARRAY_SIZE(ina226_conv_time_tab));
>>> +
>>> +    chip->itb = ina226_conv_time_tab[bits];
>>> +
>>> +    *config &= ~INA226_ITB_MASK;
>>> +    *config |= INA226_SHIFT_ITB(bits) & INA226_ITB_MASK;
>>> +
>>> +    return 0;
>>> +}
>>> +
>> Could do with a slightly more informative name than its vs itb.
>> Or at least some docs explaining what it does.
>>> +static unsigned int ina226_set_its(struct ina2xx_chip_info *chip,
>>> +                   unsigned int val,
>>> +                   unsigned int *config)
>>> +{
>>> +    int bits;
>>> +
>>> +    if (val > 8244 || val < 140)
>>> +        return -EINVAL;
>>> +
>>> +    bits = find_closest(val, ina226_conv_time_tab,
>>> +            ARRAY_SIZE(ina226_conv_time_tab));
>>> +
>>> +    chip->its = ina226_conv_time_tab[bits];
>>> +
>>> +    *config &= ~INA226_ITS_MASK;
>>> +    *config |= INA226_SHIFT_ITS(bits) & INA226_ITS_MASK;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +
>>> +static int ina2xx_write_raw(struct iio_dev *indio_dev,
>>> +                struct iio_chan_spec const *chan,
>>> +                int val, int val2, long mask)
>>> +{
>>> +    struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>>> +    int ret = 0;
>>> +    unsigned int config, tmp;
>>> +
>>> +    if (iio_buffer_enabled(indio_dev))
>>> +        return -EBUSY;
>>> +
>>> +    mutex_lock(&chip->state_lock);
>>> +
>>> +    ret = regmap_read(chip->regmap, INA2XX_CONFIG, &config);
>>> +    if (ret < 0)
>>> +        goto _err;
>>> +
>>> +    tmp = config;
>>> +
>>> +    switch (mask) {
>>> +    case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>>> +        ret = ina226_set_average(chip, val, &tmp);
>>> +        break;
>>> +
>>> +    case IIO_CHAN_INFO_INT_TIME:
>>> +        if (chan->address = INA2XX_SHUNT_VOLTAGE)
>>> +            ret = ina226_set_its(chip, val, &tmp);
>>> +        else
>>> +            ret = ina226_set_itb(chip, val, &tmp);
>>> +        break;
>>> +    default:
>>> +        ret = -EINVAL;
>>> +    }
>>> +
>>> +    if (!ret && (tmp != config))
>>> +        ret = regmap_write(chip->regmap, INA2XX_CONFIG, tmp);
>>> +_err:
>>> +    mutex_unlock(&chip->state_lock);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +
>>> +#define INA2XX_CHAN(_type, _index, _address) { \
>>> +    .type = _type, \
>>> +    .address = _address, \
>>> +    .indexed = 1, \
>>> +    .channel = (_index), \
>>> +    .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
>>> +    .info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
>>> +                   BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
>>> +    .scan_index = (_index), \
>>> +    .scan_type = { \
>>> +        .sign = 'u', \
>>> +        .realbits = 16, \
>>> +        .storagebits = 16, \
>>> +    .endianness = IIO_BE, \
>>> +    } \
>>> +}
>>> +
>>> +/*Sampling Freq is a consequence of the integration times of the V
>>> channels.*/
>>> +#define INA2XX_CHAN_VOLTAGE(_index, _address) { \
>>> +    .type = IIO_VOLTAGE, \
>>> +    .address = _address, \
>>> +    .indexed = 1, \
>>> +    .channel = (_index), \
>>> +    .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
>>> +                  BIT(IIO_CHAN_INFO_INT_TIME), \
>>> +    .scan_index = (_index), \
>>> +    .scan_type = { \
>>> +        .sign = 'u', \
>>> +        .realbits = 16, \
>>> +        .storagebits = 16, \
>>> +    .endianness = IIO_BE, \
>>> +    } \
>>> +}
>>> +
>>> +
>>> +static const struct iio_chan_spec ina2xx_channels[] = {
>>> +    INA2XX_CHAN_VOLTAGE(0, INA2XX_SHUNT_VOLTAGE),
>>> +    INA2XX_CHAN_VOLTAGE(1, INA2XX_BUS_VOLTAGE),
>>> +    INA2XX_CHAN(IIO_CURRENT, 2, INA2XX_CURRENT),
>>> +    INA2XX_CHAN(IIO_POWER, 3, INA2XX_POWER),
>>> +    IIO_CHAN_SOFT_TIMESTAMP(4),
>>> +};
>>> +
>>> +
>>> +static s64 prev_ns;
>> This prevents you having more than one instance of the device - move
>> it into
>> your chip_info.
>>> +
>>> +static int ina2xx_work_buffer(struct iio_dev *indio_dev)
>>> +{
>>> +    struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>>> +    unsigned short data[8];
>>> +    int bit, ret = 0, i = 0;
>>> +    unsigned long buffer_us = 0, elapsed_us = 0;
>>> +    s64 time_a, time_b;
>>> +    unsigned int alert;
>>> +
>>> +    time_a = iio_get_time_ns();
>>> +
>>> +    /*
>>> +     * Because the timer thread and the chip conversion clock
>>> +     * are asynchronous, the period difference will eventually
>>> +     * result in reading V[k-1] again, or skip V[k] at time Tk.
>>> +     * In order to resync the timer with the conversion process
>>> +     * we check the ConVersionReadyFlag.
>>> +     * On hardware that supports using the ALERT pin to toggle a
>>> +     * GPIO a triggered buffer could be used instead.
>>> +     * For now, we pay for that extra read of the ALERT register
>>> +     */
>>> +    do {
>>> +        ret = regmap_read(chip->regmap, INA226_ALERT_MASK,
>>> +                  &alert);
>>> +        if (ret < 0)
>>> +            goto _err;
>>> +
>>> +        alert &= INA266_CVRF;
>>> +        trace_printk("Conversion ready: %d\n", !!alert);
>>> +
>>> +    } while (!alert);
>>> +
>>> +    /*
>>> +     * Single register reads: bulk_read will not work with ina226
>>> +     * as there is no auto-increment of the address register for
>>> +     * data length longer than 16bits.
>>> +     */
>>> +    for_each_set_bit(bit, indio_dev->active_scan_mask,
>>> +             indio_dev->masklength) {
>>> +        unsigned int val;
>>> +
>>> +        ret = regmap_read(chip->regmap,
>>> +                  INA2XX_SHUNT_VOLTAGE + bit, &val);
>>> +        if (ret < 0)
>>> +            goto _err;
>>> +
>>> +        data[i++] = val;
>>> +    }
>>> +
>>> +    time_b = iio_get_time_ns();
>>> +
>>> +    iio_push_to_buffers_with_timestamp(indio_dev,
>>> +                       (unsigned int *)data, time_a);
>>> +
>>> +    buffer_us = (unsigned long)(time_b - time_a) / 1000;
>>> +    elapsed_us = (unsigned long)(time_a - prev_ns) / 1000;
>>> +
>>> +    trace_printk("uS: elapsed: %lu, buf: %lu\n", elapsed_us,
>>> buffer_us);
>>> +
>>> +    prev_ns = time_a;
>>> +
>>> +_err:
>>> +    return buffer_us;
>>> +};
>>> +
>>> +static int ina2xx_capture_thread(void *data)
>>> +{
>>> +    struct iio_dev *indio_dev = (struct iio_dev *)data;
>>> +    struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>>> +    unsigned int sampling_us = (chip->itb + chip->its) * chip->avg;
>>> +    unsigned long buffer_us;
>>> +
>>> +    /*
>>> +     * Poll a bit faster than the chip internal Fs, in case
>>> +     * we wish to sync with the conversion ready flag.
>>> +     */
>>> +    sampling_us -= 200;
>>> +
>>> +    do {
>>> +        buffer_us = ina2xx_work_buffer(indio_dev);
>>> +
>>> +        if (sampling_us > buffer_us)
>>> +            udelay(sampling_us - buffer_us);
>>> +
>>> +    } while (!kthread_should_stop());
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +int ina2xx_buffer_enable(struct iio_dev *indio_dev)
>>> +{
>>> +    struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>>> +    unsigned int sampling_us = (chip->itb + chip->its) * chip->avg;
>>> +
>>> +    trace_printk("Enabling buffer w/ scan_mask %02x, freq = %d, avg
>>> =%u\n",
>>> +             (unsigned int)(*indio_dev->active_scan_mask),
>>> +             1000000/sampling_us, chip->avg);
>>> +
>>> +    trace_printk("Expected work period: %u us\n", sampling_us);
>>> +
>>> +    prev_ns = iio_get_time_ns();
>>> +
>>> +    chip->task = kthread_run(ina2xx_capture_thread, (void *)indio_dev,
>>> +                 "ina2xx-%uus", sampling_us);
>>> +
>>> +    return PTR_ERR_OR_ZERO(chip->task);
>>> +}
>>> +
>>> +int ina2xx_buffer_disable(struct iio_dev *indio_dev)
>>> +{
>>> +    struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>>> +
>>> +    if (chip->task) {
>>> +        kthread_stop(chip->task);
>>> +        chip->task = NULL;
>>> +    }
>> nitpick: blank line here.
>>> +    return 0;
>>> +}
>>> +
>>> +static const struct iio_buffer_setup_ops ina2xx_setup_ops = {
>>> +    .postenable = &ina2xx_buffer_enable,
>>> +    .postdisable = &ina2xx_buffer_disable,
>>> +};
>>> +
>>> +static int ina2xx_debug_reg(struct iio_dev *indio_dev,
>>> +                unsigned reg, unsigned writeval, unsigned *readval)
>>> +{
>>> +    struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>>> +
>>> +    if (!readval)
>>> +        return regmap_write(chip->regmap, reg, writeval);
>>> +
>>> +    return regmap_read(chip->regmap, reg, readval);
>>> +}
>>> +
>>> +/* frequencies matching the cummulated integration times for vshunt
>>> and vbus */
>> huh?  Integration time is in seconds... These seem unlikely to be valid
>> settings.
>>> +static IIO_CONST_ATTR_INT_TIME_AVAIL("140 204 332 588 1100 2116 4156
>>> 8244");
>>> +
>>> +static struct attribute *ina2xx_attributes[] = {
>>> +    &iio_const_attr_integration_time_available.dev_attr.attr,
>>> +    NULL,
>>> +};
>>> +
>>> +static const struct attribute_group ina2xx_attribute_group = {
>>> +    .attrs = ina2xx_attributes,
>>> +};
>>> +
>>> +static const struct iio_info ina2xx_info = {
>>> +    .debugfs_reg_access = &ina2xx_debug_reg,
>>> +    .read_raw = &ina2xx_read_raw,
>>> +    .write_raw = &ina2xx_write_raw,
>>> +    .attrs = &ina2xx_attribute_group,
>>> +    .driver_module = THIS_MODULE,
>>> +};
>>> +
>>> +/* Initialize the configuration and calibration registers. */
>>> +static int ina2xx_init(struct ina2xx_chip_info *chip, unsigned int
>>> config)
>>> +{
>>> +    u16 regval;
>>> +    int ret = regmap_write(chip->regmap, INA2XX_CONFIG, config);
>>> +
>>> +    if (ret < 0)
>>> +        return ret;
>>> +    /*
>>> +     * Set current LSB to 1mA, shunt is in uOhms
>>> +     * (equation 13 in datasheet). We hardcode a Current_LSB
>>> +     * of 1.0 x10-6. The only remaining parameter is RShunt
>>> +     * There is no need to expose the CALIBRATION register
>>> +     * to the user for now.
>>> +     */
>>> +    regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor,
>>> +                chip->rshunt);
>>> +
>>> +    return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval);
>>> +}
>>> +
>>> +static int ina2xx_probe(struct i2c_client *client,
>>> +            const struct i2c_device_id *id)
>>> +{
>>> +    struct ina2xx_chip_info *chip;
>>> +    struct iio_dev *indio_dev;
>>> +    struct iio_buffer *buffer;
>>> +    int ret;
>>> +    unsigned int val;
>>> +
>>> +    indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
>>> +    if (!indio_dev)
>>> +        return -ENOMEM;
>>> +
>>> +    chip = iio_priv(indio_dev);
>>> +
>>> +    chip->config = &ina2xx_config[id->driver_data];
>>> +
>>> +    if (of_property_read_u32(client->dev.of_node,
>>> +                 "shunt-resistor", &val) < 0) {
>>> +        struct ina2xx_platform_data *pdata >>> +            dev_get_platdata(&client->dev);
>>> +
>>> +        if (pdata)
>>> +            val = pdata->shunt_uohms;
>>> +        else
>>> +            val = INA2XX_RSHUNT_DEFAULT;
>>> +    }
>>> +
>>> +    if (val <= 0 || val > chip->config->calibration_factor)
>>> +        return -ENODEV;
>>> +
>>> +    chip->rshunt = val;
>>> +
>>> +    ina2xx_regmap_config.max_register = chip->config->registers;
>>> +
>>> +    mutex_init(&chip->state_lock);
>>> +
>>> +    /* this is only used for device removal purposes */
>>> +    i2c_set_clientdata(client, indio_dev);
>>> +
>>> +    indio_dev->name = id->name;
>>> +    indio_dev->channels = ina2xx_channels;
>>> +    indio_dev->num_channels = ARRAY_SIZE(ina2xx_channels);
>>> +
>>> +    indio_dev->dev.parent = &client->dev;
>>> +    indio_dev->info = &ina2xx_info;
>>> +    indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
>>> +
>>> +    chip->regmap = devm_regmap_init_i2c(client, &ina2xx_regmap_config);
>>> +    if (IS_ERR(chip->regmap)) {
>>> +        dev_err(&client->dev, "failed to allocate register map\n");
>>> +        return PTR_ERR(chip->regmap);
>>> +    }
>>> +
>>> +    /* Patch the current config register with default. */
>>> +    val = chip->config->config_default;
>>> +
>>> +    if (id->driver_data = ina226) {
>>> +        ina226_set_average(chip, INA226_DEFAULT_AVG, &val);
>>> +        ina226_set_itb(chip, INA226_DEFAULT_IT, &val);
>>> +        ina226_set_its(chip, INA226_DEFAULT_IT, &val);
>>> +    }
>>> +
>>> +    ret = ina2xx_init(chip, val);
>>> +    if (ret < 0) {
>>> +        dev_err(&client->dev, "error configuring the device: %d\n",
>>> +            ret);
>>> +        return -ENODEV;
>>> +    }
>>> +
>>> +    buffer = devm_iio_kfifo_allocate(&indio_dev->dev);
>>> +    if (!buffer)
>>> +        return -ENOMEM;
>>> +
>>> +    indio_dev->setup_ops = &ina2xx_setup_ops;
>>> +
>>> +    iio_device_attach_buffer(indio_dev, buffer);
>>> +
>>> +    return devm_iio_device_register(&client->dev, indio_dev);
>>> +}
>>> +
>>> +
>>> +static int ina2xx_remove(struct i2c_client *client)
>>> +{
>>> +    struct iio_dev *indio_dev = i2c_get_clientdata(client);
>>> +    struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>>> +    int ret;
>>> +
>>> +    mutex_destroy(&chip->state_lock);
>>> +
>>> +    /* Powerdown */
>>> +    ret = regmap_update_bits(chip->regmap, INA2XX_CONFIG,
>>> +                  INA2XX_MODE_MASK, 0);
>>> +
>>> +    iio_device_unregister(indio_dev);
>> Peter already covered this to a degree.
>>
>> iio_device_unregister removes the userspace interface - hence it
>> must be the first thing done in remove.
>>
>> If you use devm_ version of register it will occur at the end of the
>> remove.  Hence if the remove has anything in it, you pretty much
>> can't use the devm_iio_device_register without introducing a nasty
>> race condition.  Hence you don't want the devm version here and you
>> want to move iio_device_unregister to the beginning of this function.
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +
>>> +static const struct i2c_device_id ina2xx_id[] = {
>>> +    {"ina219", ina219},
>>> +    {"ina220", ina219},
>>> +    {"ina226", ina226},
>>> +    {"ina230", ina226},
>>> +    {"ina231", ina226},
>>> +    {}
>>> +};
>>> +
>>> +MODULE_DEVICE_TABLE(i2c, ina2xx_id);
>>> +
>>> +static struct i2c_driver ina2xx_driver = {
>>> +    .driver = {
>>> +           .name = KBUILD_MODNAME,
>>> +    },
>>> +    .probe = ina2xx_probe,
>>> +    .remove = ina2xx_remove,
>>> +    .id_table = ina2xx_id,
>>> +};
>>> +
>>> +module_i2c_driver(ina2xx_driver);
>>> +
>>> +MODULE_AUTHOR("Marc Titinger <marc.titinger@baylibre.com>");
>>> +MODULE_DESCRIPTION("Texas Instruments INA2XX ADC driver");
>>> +MODULE_LICENSE("GPL v2");
>>>
>>
>>
>


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [PATCH 1/2] iio: ina2xx: add support for TI INA2xx Power Monitors

[Guenter Roeck]

On 12/01/2015 02:01 AM, Marc Titinger wrote:
> On 29/11/2015 18:38, Guenter Roeck wrote:
>> On 11/29/2015 08:31 AM, Jonathan Cameron wrote:
>>> On 25/11/15 11:28, Marc Titinger wrote:
>>>> in SOFTWARE buffer mode, a kthread will capture the active scan_elements
>>>> into a kfifo, then compute the remaining time until the next capture
>>>> tick
>>>> and do an active wait (udelay).
>>>>
>>>> This will produce a stream of up to fours channels plus a 64bits
>>>> timestamps (ns).
>>>>
>>>> Tested with ina226, on BeagleBoneBlack.
>>>>
>>>> Datasheet: http://www.ti.com/lit/gpn/ina226
>>>>
>>>> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
>>> A few more bits from me, but basically looking good.
>>>
>>> We do however, need to make the hwmon guys aware this is going on.
>>> Please cc their list on the next version.
>>>
>>> Last thing we want is for this to turn up as a surprise!
>>>
>>
>> I have seen the original patch, so no surprise here. Just not sure
>> if we should remove the hwmon driver after the iio driver is accepted.
>> Even though the stated goal is different, it seems to me that having
>> two drivers really does not make sense. Any thoughts ?
>
> Hi Guenter,
>
> we for instance have two completely unrelated projects both using this chip, on one side we wish to measure power consumption on the host board and hwmon has the apis and tool ecosystem for it, while on the "ACME" system we needed IIO because it's buffer streaming scheme and remote features saves the day to create a lab instrument. But really both have their use, I'd recommend keeping the hwmon driver for now. I doubt those drivers will generate much maintenance work in the future either.
>

Marc,

You can use the iio-hwmon bridge to get the hwmon ABI from the iio driver.
There is no need to keep the hwmon driver as a separate entity for that.

Guenter

Re: [lm-sensors] [PATCH 1/2] iio: ina2xx: add support for TI INA2xx Power Monitors

[Guenter Roeck]

On 12/01/2015 02:01 AM, Marc Titinger wrote:
> On 29/11/2015 18:38, Guenter Roeck wrote:
>> On 11/29/2015 08:31 AM, Jonathan Cameron wrote:
>>> On 25/11/15 11:28, Marc Titinger wrote:
>>>> in SOFTWARE buffer mode, a kthread will capture the active scan_elements
>>>> into a kfifo, then compute the remaining time until the next capture
>>>> tick
>>>> and do an active wait (udelay).
>>>>
>>>> This will produce a stream of up to fours channels plus a 64bits
>>>> timestamps (ns).
>>>>
>>>> Tested with ina226, on BeagleBoneBlack.
>>>>
>>>> Datasheet: http://www.ti.com/lit/gpn/ina226
>>>>
>>>> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
>>> A few more bits from me, but basically looking good.
>>>
>>> We do however, need to make the hwmon guys aware this is going on.
>>> Please cc their list on the next version.
>>>
>>> Last thing we want is for this to turn up as a surprise!
>>>
>>
>> I have seen the original patch, so no surprise here. Just not sure
>> if we should remove the hwmon driver after the iio driver is accepted.
>> Even though the stated goal is different, it seems to me that having
>> two drivers really does not make sense. Any thoughts ?
>
> Hi Guenter,
>
> we for instance have two completely unrelated projects both using this chip, on one side we wish to measure power consumption on the host board and hwmon has the apis and tool ecosystem for it, while on the "ACME" system we needed IIO because it's buffer streaming scheme and remote features saves the day to create a lab instrument. But really both have their use, I'd recommend keeping the hwmon driver for now. I doubt those drivers will generate much maintenance work in the future either.
>

Marc,

You can use the iio-hwmon bridge to get the hwmon ABI from the iio driver.
There is no need to keep the hwmon driver as a separate entity for that.

Guenter


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

[PATCH 2/2] iio: ina2xx: provide a sysfs parameter to allow async readout of the ADCs

[Marc Titinger]

This can lead to repeated or skipped samples depending on the clock beat
between the capture thread and the chip sampling clock, but will also spare
reading/waiting for the Capture Ready Flag and improve the available i2c
bandwidth for reading measurements.

Output of iio_info:
...snip...
4 device-specific attributes found:
  attr 0: in_oversampling_ratio value: 4
  attr 1: in_allow_async_readout value: 0
  attr 2: integration_time_available value: 140 204 332 588 1100 2116 4156 8244
  attr 3: in_sampling_frequency value: 114

Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
---
 drivers/iio/adc/ina2xx-iio.c | 48 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 42 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/adc/ina2xx-iio.c b/drivers/iio/adc/ina2xx-iio.c
index 4a0026c..c153164 100644
--- a/drivers/iio/adc/ina2xx-iio.c
+++ b/drivers/iio/adc/ina2xx-iio.c
@@ -114,6 +114,7 @@ struct ina2xx_chip_info {
 	int avg;
 	int itb; /* Bus voltage integration time uS */
 	int its; /* Shunt voltage integration tim uS */
+	bool allow_async_readout;
 };
 
 static const struct ina2xx_config ina2xx_config[] = {
@@ -335,6 +336,33 @@ _err:
 }
 
 
+static ssize_t ina2xx_allow_async_readout_show(struct device *dev,
+					   struct device_attribute *attr,
+					   char *buf)
+{
+	struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
+
+	return sprintf(buf, "%d\n", chip->allow_async_readout);
+}
+
+static ssize_t ina2xx_allow_async_readout_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t len)
+{
+	struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
+	unsigned long val;
+	int ret;
+
+	ret = kstrtoul((const char *) buf, 10, &val);
+	if (ret)
+		return -EINVAL;
+
+	chip->allow_async_readout = !!val;
+
+	return len;
+}
+
+
 #define INA2XX_CHAN(_type, _index, _address) { \
 	.type = _type, \
 	.address = _address, \
@@ -402,11 +430,12 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
 	 * GPIO a triggered buffer could be used instead.
 	 * For now, we pay for that extra read of the ALERT register
 	 */
-	do {
-		ret = regmap_read(chip->regmap, INA226_ALERT_MASK,
-				  &alert);
-		if (ret < 0)
-			goto _err;
+	if (!chip->allow_async_readout)
+		do {
+			ret = regmap_read(chip->regmap, INA226_ALERT_MASK,
+					  &alert);
+			if (ret < 0)
+				goto _err;
 
 		alert &= INA266_CVRF;
 		trace_printk("Conversion ready: %d\n", !!alert);
@@ -457,7 +486,8 @@ static int ina2xx_capture_thread(void *data)
 	 * Poll a bit faster than the chip internal Fs, in case
 	 * we wish to sync with the conversion ready flag.
 	 */
-	sampling_us -= 200;
+	if (!chip->allow_async_readout)
+		sampling_us -= 200;
 
 	do {
 		buffer_us = ina2xx_work_buffer(indio_dev);
@@ -480,6 +510,7 @@ int ina2xx_buffer_enable(struct iio_dev *indio_dev)
 		     1000000/sampling_us, chip->avg);
 
 	trace_printk("Expected work period: %u us\n", sampling_us);
+	trace_printk("Async readout mode: %d\n", chip->allow_async_readout);
 
 	prev_ns = iio_get_time_ns();
 
@@ -519,7 +550,12 @@ static int ina2xx_debug_reg(struct iio_dev *indio_dev,
 /* frequencies matching the cummulated integration times for vshunt and vbus */
 static IIO_CONST_ATTR_INT_TIME_AVAIL("140 204 332 588 1100 2116 4156 8244");
 
+static IIO_DEVICE_ATTR(in_allow_async_readout, S_IRUGO | S_IWUSR,
+		       ina2xx_allow_async_readout_show,
+		       ina2xx_allow_async_readout_store, 0);
+
 static struct attribute *ina2xx_attributes[] = {
+	&iio_dev_attr_in_allow_async_readout.dev_attr.attr,
 	&iio_const_attr_integration_time_available.dev_attr.attr,
 	NULL,
 };
-- 
1.9.1

Re: [PATCH 2/2] iio: ina2xx: provide a sysfs parameter to allow async readout of the ADCs

[Jonathan Cameron]

On 25/11/15 11:28, Marc Titinger wrote:
> This can lead to repeated or skipped samples depending on the clock beat
> between the capture thread and the chip sampling clock, but will also spare
> reading/waiting for the Capture Ready Flag and improve the available i2c
> bandwidth for reading measurements.
> 
> Output of iio_info:
> ...snip...
> 4 device-specific attributes found:
>   attr 0: in_oversampling_ratio value: 4
>   attr 1: in_allow_async_readout value: 0
>   attr 2: integration_time_available value: 140 204 332 588 1100 2116 4156 8244
>   attr 3: in_sampling_frequency value: 114
>
This is a good compromise if it is needed to get the rates as high as
you need.

One suggestion inline.

Jonathan

> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
> ---
>  drivers/iio/adc/ina2xx-iio.c | 48 ++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 42 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/adc/ina2xx-iio.c b/drivers/iio/adc/ina2xx-iio.c
> index 4a0026c..c153164 100644
> --- a/drivers/iio/adc/ina2xx-iio.c
> +++ b/drivers/iio/adc/ina2xx-iio.c
> @@ -114,6 +114,7 @@ struct ina2xx_chip_info {
>  	int avg;
>  	int itb; /* Bus voltage integration time uS */
>  	int its; /* Shunt voltage integration tim uS */
> +	bool allow_async_readout;
>  };
>  
>  static const struct ina2xx_config ina2xx_config[] = {
> @@ -335,6 +336,33 @@ _err:
>  }
>  
>  
> +static ssize_t ina2xx_allow_async_readout_show(struct device *dev,
> +					   struct device_attribute *attr,
> +					   char *buf)
> +{
> +	struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
> +
> +	return sprintf(buf, "%d\n", chip->allow_async_readout);
> +}
> +
> +static ssize_t ina2xx_allow_async_readout_store(struct device *dev,
> +				struct device_attribute *attr,
> +				const char *buf, size_t len)
> +{
> +	struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
> +	unsigned long val;
> +	int ret;
> +
strtobool would simplify this.
> +	ret = kstrtoul((const char *) buf, 10, &val);
> +	if (ret)
> +		return -EINVAL;
> +
> +	chip->allow_async_readout = !!val;
> +
> +	return len;
> +}
> +
> +
>  #define INA2XX_CHAN(_type, _index, _address) { \
>  	.type = _type, \
>  	.address = _address, \
> @@ -402,11 +430,12 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
>  	 * GPIO a triggered buffer could be used instead.
>  	 * For now, we pay for that extra read of the ALERT register
>  	 */
> -	do {
> -		ret = regmap_read(chip->regmap, INA226_ALERT_MASK,
> -				  &alert);
> -		if (ret < 0)
> -			goto _err;
> +	if (!chip->allow_async_readout)
> +		do {
> +			ret = regmap_read(chip->regmap, INA226_ALERT_MASK,
> +					  &alert);
> +			if (ret < 0)
> +				goto _err;
>  
>  		alert &= INA266_CVRF;
>  		trace_printk("Conversion ready: %d\n", !!alert);
> @@ -457,7 +486,8 @@ static int ina2xx_capture_thread(void *data)
>  	 * Poll a bit faster than the chip internal Fs, in case
>  	 * we wish to sync with the conversion ready flag.
>  	 */
> -	sampling_us -= 200;
> +	if (!chip->allow_async_readout)
> +		sampling_us -= 200;
>  
>  	do {
>  		buffer_us = ina2xx_work_buffer(indio_dev);
> @@ -480,6 +510,7 @@ int ina2xx_buffer_enable(struct iio_dev *indio_dev)
>  		     1000000/sampling_us, chip->avg);
>  
>  	trace_printk("Expected work period: %u us\n", sampling_us);
> +	trace_printk("Async readout mode: %d\n", chip->allow_async_readout);
>  
>  	prev_ns = iio_get_time_ns();
>  
> @@ -519,7 +550,12 @@ static int ina2xx_debug_reg(struct iio_dev *indio_dev,
>  /* frequencies matching the cummulated integration times for vshunt and vbus */
>  static IIO_CONST_ATTR_INT_TIME_AVAIL("140 204 332 588 1100 2116 4156 8244");
>  
> +static IIO_DEVICE_ATTR(in_allow_async_readout, S_IRUGO | S_IWUSR,
> +		       ina2xx_allow_async_readout_show,
> +		       ina2xx_allow_async_readout_store, 0);
> +
>  static struct attribute *ina2xx_attributes[] = {
> +	&iio_dev_attr_in_allow_async_readout.dev_attr.attr,
>  	&iio_const_attr_integration_time_available.dev_attr.attr,
>  	NULL,
>  };
> 

Re: [lm-sensors] [PATCH 2/2] iio: ina2xx: provide a sysfs parameter to allow async readout of the AD

[Jonathan Cameron]

On 25/11/15 11:28, Marc Titinger wrote:
> This can lead to repeated or skipped samples depending on the clock beat
> between the capture thread and the chip sampling clock, but will also spare
> reading/waiting for the Capture Ready Flag and improve the available i2c
> bandwidth for reading measurements.
> 
> Output of iio_info:
> ...snip...
> 4 device-specific attributes found:
>   attr 0: in_oversampling_ratio value: 4
>   attr 1: in_allow_async_readout value: 0
>   attr 2: integration_time_available value: 140 204 332 588 1100 2116 4156 8244
>   attr 3: in_sampling_frequency value: 114
>
This is a good compromise if it is needed to get the rates as high as
you need.

One suggestion inline.

Jonathan

> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
> ---
>  drivers/iio/adc/ina2xx-iio.c | 48 ++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 42 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/adc/ina2xx-iio.c b/drivers/iio/adc/ina2xx-iio.c
> index 4a0026c..c153164 100644
> --- a/drivers/iio/adc/ina2xx-iio.c
> +++ b/drivers/iio/adc/ina2xx-iio.c
> @@ -114,6 +114,7 @@ struct ina2xx_chip_info {
>  	int avg;
>  	int itb; /* Bus voltage integration time uS */
>  	int its; /* Shunt voltage integration tim uS */
> +	bool allow_async_readout;
>  };
>  
>  static const struct ina2xx_config ina2xx_config[] = {
> @@ -335,6 +336,33 @@ _err:
>  }
>  
>  
> +static ssize_t ina2xx_allow_async_readout_show(struct device *dev,
> +					   struct device_attribute *attr,
> +					   char *buf)
> +{
> +	struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
> +
> +	return sprintf(buf, "%d\n", chip->allow_async_readout);
> +}
> +
> +static ssize_t ina2xx_allow_async_readout_store(struct device *dev,
> +				struct device_attribute *attr,
> +				const char *buf, size_t len)
> +{
> +	struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
> +	unsigned long val;
> +	int ret;
> +
strtobool would simplify this.
> +	ret = kstrtoul((const char *) buf, 10, &val);
> +	if (ret)
> +		return -EINVAL;
> +
> +	chip->allow_async_readout = !!val;
> +
> +	return len;
> +}
> +
> +
>  #define INA2XX_CHAN(_type, _index, _address) { \
>  	.type = _type, \
>  	.address = _address, \
> @@ -402,11 +430,12 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
>  	 * GPIO a triggered buffer could be used instead.
>  	 * For now, we pay for that extra read of the ALERT register
>  	 */
> -	do {
> -		ret = regmap_read(chip->regmap, INA226_ALERT_MASK,
> -				  &alert);
> -		if (ret < 0)
> -			goto _err;
> +	if (!chip->allow_async_readout)
> +		do {
> +			ret = regmap_read(chip->regmap, INA226_ALERT_MASK,
> +					  &alert);
> +			if (ret < 0)
> +				goto _err;
>  
>  		alert &= INA266_CVRF;
>  		trace_printk("Conversion ready: %d\n", !!alert);
> @@ -457,7 +486,8 @@ static int ina2xx_capture_thread(void *data)
>  	 * Poll a bit faster than the chip internal Fs, in case
>  	 * we wish to sync with the conversion ready flag.
>  	 */
> -	sampling_us -= 200;
> +	if (!chip->allow_async_readout)
> +		sampling_us -= 200;
>  
>  	do {
>  		buffer_us = ina2xx_work_buffer(indio_dev);
> @@ -480,6 +510,7 @@ int ina2xx_buffer_enable(struct iio_dev *indio_dev)
>  		     1000000/sampling_us, chip->avg);
>  
>  	trace_printk("Expected work period: %u us\n", sampling_us);
> +	trace_printk("Async readout mode: %d\n", chip->allow_async_readout);
>  
>  	prev_ns = iio_get_time_ns();
>  
> @@ -519,7 +550,12 @@ static int ina2xx_debug_reg(struct iio_dev *indio_dev,
>  /* frequencies matching the cummulated integration times for vshunt and vbus */
>  static IIO_CONST_ATTR_INT_TIME_AVAIL("140 204 332 588 1100 2116 4156 8244");
>  
> +static IIO_DEVICE_ATTR(in_allow_async_readout, S_IRUGO | S_IWUSR,
> +		       ina2xx_allow_async_readout_show,
> +		       ina2xx_allow_async_readout_store, 0);
> +
>  static struct attribute *ina2xx_attributes[] = {
> +	&iio_dev_attr_in_allow_async_readout.dev_attr.attr,
>  	&iio_const_attr_integration_time_available.dev_attr.attr,
>  	NULL,
>  };
> 


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors