[PATCH v2 0/2] IIO version of INA2xx

[PATCH v2 0/2] IIO version of INA2xx

[Marc Titinger]

Version 2 of https://lkml.org/lkml/2015/11/25/245

 * fix few typos, comments and return value type issues.
 * fix remove function: call order for iio_device_unregister
 * rename itb => int_time_vbus and its => int_time_vshunt for clarity
 * fix scale for integr. times (available values where in uS instead of s)
 * move static variable into device data to allow for multiple instances

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

Marc Titinger (2):
  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      |   9 +
 drivers/iio/adc/Makefile     |   1 +
 drivers/iio/adc/ina2xx-iio.c | 714 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 724 insertions(+)
 create mode 100644 drivers/iio/adc/ina2xx-iio.c

-- 
1.9.1

[PATCH v2 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      |   9 +
 drivers/iio/adc/Makefile     |   1 +
 drivers/iio/adc/ina2xx-iio.c | 678 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 688 insertions(+)
 create mode 100644 drivers/iio/adc/ina2xx-iio.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index eb0cd89..beea041 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -170,6 +170,15 @@ 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
+	select IIO_KFIFO_BUF
+	help
+	  Say yes here to build support for TI INA2xx family Power Monitors.
+
 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..99fb73c
--- /dev/null
+++ b/drivers/iio/adc/ina2xx-iio.c
@@ -0,0 +1,678 @@
+/*
+ * 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
+ */
+#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)
+
+/* Cosmetic macro giving the sampling period for a full P=UxI cycle */
+#define SAMPLING_PERIOD(c)	((c->int_time_vbus + c->int_time_vshunt) \
+				 * c->avg)
+
+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;
+	s64 prev_ns;	/* track buffer capture time check, for underruns*/
+	int int_time_vbus; /* Bus voltage integration time uS */
+	int int_time_vshunt; /* Shunt voltage integration time 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->int_time_vshunt;
+		else
+			*val2 = chip->int_time_vbus;
+
+		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, SAMPLING_PERIOD(chip));
+
+		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 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 int ina226_set_int_time_vbus(struct ina2xx_chip_info *chip,
+				    unsigned int val_us, unsigned int *config)
+{
+	int bits;
+
+	if (val_us > 8244 || val_us < 140)
+		return -EINVAL;
+
+	bits = find_closest(val_us, ina226_conv_time_tab,
+			ARRAY_SIZE(ina226_conv_time_tab));
+
+	chip->int_time_vbus = ina226_conv_time_tab[bits];
+
+	*config &= ~INA226_ITB_MASK;
+	*config |= INA226_SHIFT_ITB(bits) & INA226_ITB_MASK;
+
+	return 0;
+}
+
+static int ina226_set_int_time_vshunt(struct ina2xx_chip_info *chip,
+				      unsigned int val_us, unsigned int *config)
+{
+	int bits;
+
+	if (val_us > 8244 || val_us < 140)
+		return -EINVAL;
+
+	bits = find_closest(val_us, ina226_conv_time_tab,
+			ARRAY_SIZE(ina226_conv_time_tab));
+
+	chip->int_time_vshunt = 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_int_time_vshunt(chip, val2, &tmp);
+		else
+			ret = ina226_set_int_time_vbus(chip, val2, &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 Voltage 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 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 - chip->prev_ns) / 1000;
+
+	trace_printk("uS: elapsed: %lu, buf: %lu\n", elapsed_us, buffer_us);
+
+	chip->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 = SAMPLING_PERIOD(chip);
+	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 = SAMPLING_PERIOD(chip);
+
+	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);
+
+	chip->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);
+}
+
+/* Possible integration times for vshunt and vbus */
+static IIO_CONST_ATTR_INT_TIME_AVAIL \
+ ("0.000140 0.000204 0.000332 0.000588 0.001100 0.002116 0.004156 0.008244");
+
+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_int_time_vbus(chip, INA226_DEFAULT_IT, &val);
+		ina226_set_int_time_vshunt(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 iio_device_register(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);
+
+	iio_device_unregister(indio_dev);
+
+	/* Powerdown */
+	return regmap_update_bits(chip->regmap, INA2XX_CONFIG,
+				  INA2XX_MODE_MASK, 0);
+}
+
+
+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 v2 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).

some minor comments below
 
> 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      |   9 +
>  drivers/iio/adc/Makefile     |   1 +
>  drivers/iio/adc/ina2xx-iio.c | 678 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 688 insertions(+)
>  create mode 100644 drivers/iio/adc/ina2xx-iio.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index eb0cd89..beea041 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -170,6 +170,15 @@ 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
> +	select IIO_KFIFO_BUF
> +	help
> +	  Say yes here to build support for TI INA2xx family Power Monitors.

'of Power Monitors' probably

> +
>  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..99fb73c
> --- /dev/null
> +++ b/drivers/iio/adc/ina2xx-iio.c
> @@ -0,0 +1,678 @@
> +/*
> + * 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
> + */

a single-line comment would do as well

> +#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)
> +
> +/* Cosmetic macro giving the sampling period for a full P=UxI cycle */
> +#define SAMPLING_PERIOD(c)	((c->int_time_vbus + c->int_time_vshunt) \
> +				 * c->avg)
> +
> +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 = {

would be nice if this could be const; maybe add variants to chip_info

> +	.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;
> +	s64 prev_ns;	/* track buffer capture time check, for underruns*/
> +	int int_time_vbus; /* Bus voltage integration time uS */
> +	int int_time_vshunt; /* Shunt voltage integration time 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->int_time_vshunt;
> +		else
> +			*val2 = chip->int_time_vbus;
> +
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	/*
> +	 * Sample freq is read only, it is a consequence of
> +	 * 1/AVG*(CT_bus+CT_shunt).
> +	 */

I'd move the comment below the case

> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*val = DIV_ROUND_CLOSEST(1000000, SAMPLING_PERIOD(chip));
> +
> +		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 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 int ina226_set_int_time_vbus(struct ina2xx_chip_info *chip,
> +				    unsigned int val_us, unsigned int *config)
> +{
> +	int bits;
> +
> +	if (val_us > 8244 || val_us < 140)
> +		return -EINVAL;
> +
> +	bits = find_closest(val_us, ina226_conv_time_tab,
> +			ARRAY_SIZE(ina226_conv_time_tab));
> +
> +	chip->int_time_vbus = ina226_conv_time_tab[bits];
> +
> +	*config &= ~INA226_ITB_MASK;
> +	*config |= INA226_SHIFT_ITB(bits) & INA226_ITB_MASK;
> +
> +	return 0;
> +}
> +
> +static int ina226_set_int_time_vshunt(struct ina2xx_chip_info *chip,
> +				      unsigned int val_us, unsigned int *config)
> +{
> +	int bits;
> +
> +	if (val_us > 8244 || val_us < 140)
> +		return -EINVAL;
> +
> +	bits = find_closest(val_us, ina226_conv_time_tab,
> +			ARRAY_SIZE(ina226_conv_time_tab));
> +
> +	chip->int_time_vshunt = 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;

no need to init ret

> +	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_int_time_vshunt(chip, val2, &tmp);
> +		else
> +			ret = ina226_set_int_time_vbus(chip, val2, &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, \

maybe (_type), (_address) for consistency

> +	.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, \

indentation

> +	} \
> +}
> +
> +/*
> + * Sampling Freq is a consequence of the integration times of
> + * the Voltage channels.
> + */
> +#define INA2XX_CHAN_VOLTAGE(_index, _address) { \
> +	.type = IIO_VOLTAGE, \
> +	.address = _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 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;

no need to init ret

> +	unsigned long buffer_us = 0, elapsed_us = 0;

no need to init buffer_us, elapsed_us

> +	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 - chip->prev_ns) / 1000;
> +
> +	trace_printk("uS: elapsed: %lu, buf: %lu\n", elapsed_us, buffer_us);
> +
> +	chip->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 = SAMPLING_PERIOD(chip);
> +	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 = SAMPLING_PERIOD(chip);
> +
> +	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);
> +
> +	chip->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);
> +}
> +
> +/* Possible integration times for vshunt and vbus */
> +static IIO_CONST_ATTR_INT_TIME_AVAIL \
> + ("0.000140 0.000204 0.000332 0.000588 0.001100 0.002116 0.004156 0.008244");
> +
> +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_int_time_vbus(chip, INA226_DEFAULT_IT, &val);
> +		ina226_set_int_time_vshunt(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 iio_device_register(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);
> +
> +	iio_device_unregister(indio_dev);
> +
> +	/* Powerdown */
> +	return regmap_update_bits(chip->regmap, INA2XX_CONFIG,
> +				  INA2XX_MODE_MASK, 0);
> +}
> +
> +
> +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 v2 1/2] iio: ina2xx: add support for TI INA2xx Power Monitors

[Jonathan Cameron]

On 30/11/15 12:16, 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).
> 
> some minor comments below
other than the points Peter notes below, I'm basically happy with this
driver.  Will take a final look at v3 of course!

Jonathan
>  
>> 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      |   9 +
>>  drivers/iio/adc/Makefile     |   1 +
>>  drivers/iio/adc/ina2xx-iio.c | 678 +++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 688 insertions(+)
>>  create mode 100644 drivers/iio/adc/ina2xx-iio.c
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index eb0cd89..beea041 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -170,6 +170,15 @@ 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
>> +	select IIO_KFIFO_BUF
>> +	help
>> +	  Say yes here to build support for TI INA2xx family Power Monitors.
> 
> 'of Power Monitors' probably
> 
>> +
>>  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..99fb73c
>> --- /dev/null
>> +++ b/drivers/iio/adc/ina2xx-iio.c
>> @@ -0,0 +1,678 @@
>> +/*
>> + * 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
>> + */
> 
> a single-line comment would do as well
> 
>> +#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)
>> +
>> +/* Cosmetic macro giving the sampling period for a full P=UxI cycle */
>> +#define SAMPLING_PERIOD(c)	((c->int_time_vbus + c->int_time_vshunt) \
>> +				 * c->avg)
>> +
>> +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 = {
> 
> would be nice if this could be const; maybe add variants to chip_info
> 
>> +	.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;
>> +	s64 prev_ns;	/* track buffer capture time check, for underruns*/
>> +	int int_time_vbus; /* Bus voltage integration time uS */
>> +	int int_time_vshunt; /* Shunt voltage integration time 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->int_time_vshunt;
>> +		else
>> +			*val2 = chip->int_time_vbus;
>> +
>> +		return IIO_VAL_INT_PLUS_MICRO;
>> +	/*
>> +	 * Sample freq is read only, it is a consequence of
>> +	 * 1/AVG*(CT_bus+CT_shunt).
>> +	 */
> 
> I'd move the comment below the case
> 
>> +	case IIO_CHAN_INFO_SAMP_FREQ:
>> +		*val = DIV_ROUND_CLOSEST(1000000, SAMPLING_PERIOD(chip));
>> +
>> +		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 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 int ina226_set_int_time_vbus(struct ina2xx_chip_info *chip,
>> +				    unsigned int val_us, unsigned int *config)
>> +{
>> +	int bits;
>> +
>> +	if (val_us > 8244 || val_us < 140)
>> +		return -EINVAL;
>> +
>> +	bits = find_closest(val_us, ina226_conv_time_tab,
>> +			ARRAY_SIZE(ina226_conv_time_tab));
>> +
>> +	chip->int_time_vbus = ina226_conv_time_tab[bits];
>> +
>> +	*config &= ~INA226_ITB_MASK;
>> +	*config |= INA226_SHIFT_ITB(bits) & INA226_ITB_MASK;
>> +
>> +	return 0;
>> +}
>> +
>> +static int ina226_set_int_time_vshunt(struct ina2xx_chip_info *chip,
>> +				      unsigned int val_us, unsigned int *config)
>> +{
>> +	int bits;
>> +
>> +	if (val_us > 8244 || val_us < 140)
>> +		return -EINVAL;
>> +
>> +	bits = find_closest(val_us, ina226_conv_time_tab,
>> +			ARRAY_SIZE(ina226_conv_time_tab));
>> +
>> +	chip->int_time_vshunt = 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;
> 
> no need to init ret
> 
>> +	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_int_time_vshunt(chip, val2, &tmp);
>> +		else
>> +			ret = ina226_set_int_time_vbus(chip, val2, &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, \
> 
> maybe (_type), (_address) for consistency
> 
>> +	.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, \
> 
> indentation
> 
>> +	} \
>> +}
>> +
>> +/*
>> + * Sampling Freq is a consequence of the integration times of
>> + * the Voltage channels.
>> + */
>> +#define INA2XX_CHAN_VOLTAGE(_index, _address) { \
>> +	.type = IIO_VOLTAGE, \
>> +	.address = _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 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;
> 
> no need to init ret
> 
>> +	unsigned long buffer_us = 0, elapsed_us = 0;
> 
> no need to init buffer_us, elapsed_us
> 
>> +	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 - chip->prev_ns) / 1000;
>> +
>> +	trace_printk("uS: elapsed: %lu, buf: %lu\n", elapsed_us, buffer_us);
>> +
>> +	chip->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 = SAMPLING_PERIOD(chip);
>> +	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 = SAMPLING_PERIOD(chip);
>> +
>> +	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);
>> +
>> +	chip->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);
>> +}
>> +
>> +/* Possible integration times for vshunt and vbus */
>> +static IIO_CONST_ATTR_INT_TIME_AVAIL \
>> + ("0.000140 0.000204 0.000332 0.000588 0.001100 0.002116 0.004156 0.008244");
>> +
>> +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_int_time_vbus(chip, INA226_DEFAULT_IT, &val);
>> +		ina226_set_int_time_vshunt(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 iio_device_register(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);
>> +
>> +	iio_device_unregister(indio_dev);
>> +
>> +	/* Powerdown */
>> +	return regmap_update_bits(chip->regmap, INA2XX_CONFIG,
>> +				  INA2XX_MODE_MASK, 0);
>> +}
>> +
>> +
>> +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: [PATCH v2 1/2] iio: ina2xx: add support for TI INA2xx Power Monitors

[Guenter Roeck]

On Mon, Nov 30, 2015 at 12:49:14PM +0100, 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>
> ---
>  drivers/iio/adc/Kconfig      |   9 +
>  drivers/iio/adc/Makefile     |   1 +
>  drivers/iio/adc/ina2xx-iio.c | 678 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 688 insertions(+)
>  create mode 100644 drivers/iio/adc/ina2xx-iio.c
> +
[ ... ]
> +
> +static const struct i2c_device_id ina2xx_id[] = {
> +	{"ina219", ina219},
> +	{"ina220", ina219},
> +	{"ina226", ina226},
> +	{"ina230", ina226},
> +	{"ina231", ina226},
> +	{}
> +};

I wonder what is going to happen if both this driver and the hwmon
driver for the same chips are configured in a system which supports
devicetree (or any system, really). Unless I am missing something,
the result will be that both drivers will try to instantiate, and
one will fail with -EBUSY. Or the instantiated driver is more or less
random, depending on which one happens to be loaded. Not a good
situation to be in.

For the time being, it might make sense to add cross-dependencies
in Kconfig to only permit one of the two drivers to be configured.

Ultimately we may need a better solution for the iio-hwmon bridge,
one that makes the underlying driver transparent in both devicetree
properties and user space ABI. No idea how to do that, though.

Guenter

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

[Marc Titinger]

On 02/12/2015 03:14, Guenter Roeck wrote:
> On Mon, Nov 30, 2015 at 12:49:14PM +0100, 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>
>> ---
>>   drivers/iio/adc/Kconfig      |   9 +
>>   drivers/iio/adc/Makefile     |   1 +
>>   drivers/iio/adc/ina2xx-iio.c | 678 +++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 688 insertions(+)
>>   create mode 100644 drivers/iio/adc/ina2xx-iio.c
>> +
> [ ... ]
>> +
>> +static const struct i2c_device_id ina2xx_id[] = {
>> +	{"ina219", ina219},
>> +	{"ina220", ina219},
>> +	{"ina226", ina226},
>> +	{"ina230", ina226},
>> +	{"ina231", ina226},
>> +	{}
>> +};
>
> I wonder what is going to happen if both this driver and the hwmon
> driver for the same chips are configured in a system which supports
> devicetree (or any system, really). Unless I am missing something,
> the result will be that both drivers will try to instantiate, and
> one will fail with -EBUSY. Or the instantiated driver is more or less
> random, depending on which one happens to be loaded. Not a good
> situation to be in.

I agree, we should put a mutual exclusion in Kconfig, plus maybe a 
cross-reference in the help section.

>
> For the time being, it might make sense to add cross-dependencies
> in Kconfig to only permit one of the two drivers to be configured.
>
> Ultimately we may need a better solution for the iio-hwmon bridge,
> one that makes the underlying driver transparent in both devicetree
> properties and user space ABI. No idea how to do that, though.
>

IDK if ina2xx is a special case or if this matter of dual driver stacks 
for the same chip already occurred and requires specific plumbing. 
Making the user aware of the mutual of the exclusion sounds fine with me.

Marc.


> Guenter
>

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

[Guenter Roeck]

On 12/02/2015 02:20 AM, Marc Titinger wrote:
> On 02/12/2015 03:14, Guenter Roeck wrote:
>> On Mon, Nov 30, 2015 at 12:49:14PM +0100, 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>
>>> ---
>>>   drivers/iio/adc/Kconfig      |   9 +
>>>   drivers/iio/adc/Makefile     |   1 +
>>>   drivers/iio/adc/ina2xx-iio.c | 678 +++++++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 688 insertions(+)
>>>   create mode 100644 drivers/iio/adc/ina2xx-iio.c
>>> +
>> [ ... ]
>>> +
>>> +static const struct i2c_device_id ina2xx_id[] = {
>>> +    {"ina219", ina219},
>>> +    {"ina220", ina219},
>>> +    {"ina226", ina226},
>>> +    {"ina230", ina226},
>>> +    {"ina231", ina226},
>>> +    {}
>>> +};
>>
>> I wonder what is going to happen if both this driver and the hwmon
>> driver for the same chips are configured in a system which supports
>> devicetree (or any system, really). Unless I am missing something,
>> the result will be that both drivers will try to instantiate, and
>> one will fail with -EBUSY. Or the instantiated driver is more or less
>> random, depending on which one happens to be loaded. Not a good
>> situation to be in.
>
> I agree, we should put a mutual exclusion in Kconfig, plus maybe a cross-reference in the help section.
>
>>
>> For the time being, it might make sense to add cross-dependencies
>> in Kconfig to only permit one of the two drivers to be configured.
>>
>> Ultimately we may need a better solution for the iio-hwmon bridge,
>> one that makes the underlying driver transparent in both devicetree
>> properties and user space ABI. No idea how to do that, though.
>>
>
> IDK if ina2xx is a special case or if this matter of dual driver stacks for the same chip already occurred and requires specific plumbing. Making the user aware of the mutual of the exclusion sounds fine with me.
>
htu21. We'll drop that driver from hwmon with the 4.5 kernel.
We could just drop ina2xx as well, but it is more widely used
and referenced from dts files. The ABI changes, so I am not sure
if we can just do that.

Guenter

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

[Marc Titinger]

On 02/12/2015 17:04, Guenter Roeck wrote:
> On 12/02/2015 02:20 AM, Marc Titinger wrote:
>> On 02/12/2015 03:14, Guenter Roeck wrote:
>>> On Mon, Nov 30, 2015 at 12:49:14PM +0100, 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>
>>>> ---
>>>>   drivers/iio/adc/Kconfig      |   9 +
>>>>   drivers/iio/adc/Makefile     |   1 +
>>>>   drivers/iio/adc/ina2xx-iio.c | 678
>>>> +++++++++++++++++++++++++++++++++++++++++++
>>>>   3 files changed, 688 insertions(+)
>>>>   create mode 100644 drivers/iio/adc/ina2xx-iio.c
>>>> +
>>> [ ... ]
>>>> +
>>>> +static const struct i2c_device_id ina2xx_id[] = {
>>>> +    {"ina219", ina219},
>>>> +    {"ina220", ina219},
>>>> +    {"ina226", ina226},
>>>> +    {"ina230", ina226},
>>>> +    {"ina231", ina226},
>>>> +    {}
>>>> +};
>>>
>>> I wonder what is going to happen if both this driver and the hwmon
>>> driver for the same chips are configured in a system which supports
>>> devicetree (or any system, really). Unless I am missing something,
>>> the result will be that both drivers will try to instantiate, and
>>> one will fail with -EBUSY. Or the instantiated driver is more or less
>>> random, depending on which one happens to be loaded. Not a good
>>> situation to be in.
>>
>> I agree, we should put a mutual exclusion in Kconfig, plus maybe a
>> cross-reference in the help section.
>>
>>>
>>> For the time being, it might make sense to add cross-dependencies
>>> in Kconfig to only permit one of the two drivers to be configured.
>>>
>>> Ultimately we may need a better solution for the iio-hwmon bridge,
>>> one that makes the underlying driver transparent in both devicetree
>>> properties and user space ABI. No idea how to do that, though.
>>>
>>
>> IDK if ina2xx is a special case or if this matter of dual driver
>> stacks for the same chip already occurred and requires specific
>> plumbing. Making the user aware of the mutual of the exclusion sounds
>> fine with me.
>>
> htu21. We'll drop that driver from hwmon with the 4.5 kernel.
> We could just drop ina2xx as well, but it is more widely used
> and referenced from dts files. The ABI changes, so I am not sure
> if we can just do that.

I changed iio/adc/Kconfig to the following:

+config INA2XX_ADC
+       tristate "Texas Instruments INA2xx Power Monitors IIO driver"
+       depends on I2C && !SENSORS_INA2XX
+       select REGMAP_I2C
+       select IIO_BUFFER
+       select IIO_KFIFO_BUF
+       help
+         Say yes here to build support for TI INA2xx family of Power 
Monitors.
+         This driver is mutually exclusive with the HWMON version.
+


anything the patch should also add to hwmon/Kconfig (that will not lead 
to a cycling reference warning) ?


>
> Guenter
>

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

[Guenter Roeck]

On 12/02/2015 08:20 AM, Marc Titinger wrote:
> On 02/12/2015 17:04, Guenter Roeck wrote:
>> On 12/02/2015 02:20 AM, Marc Titinger wrote:
>>> On 02/12/2015 03:14, Guenter Roeck wrote:
>>>> On Mon, Nov 30, 2015 at 12:49:14PM +0100, 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>
>>>>> ---
>>>>>   drivers/iio/adc/Kconfig      |   9 +
>>>>>   drivers/iio/adc/Makefile     |   1 +
>>>>>   drivers/iio/adc/ina2xx-iio.c | 678
>>>>> +++++++++++++++++++++++++++++++++++++++++++
>>>>>   3 files changed, 688 insertions(+)
>>>>>   create mode 100644 drivers/iio/adc/ina2xx-iio.c
>>>>> +
>>>> [ ... ]
>>>>> +
>>>>> +static const struct i2c_device_id ina2xx_id[] = {
>>>>> +    {"ina219", ina219},
>>>>> +    {"ina220", ina219},
>>>>> +    {"ina226", ina226},
>>>>> +    {"ina230", ina226},
>>>>> +    {"ina231", ina226},
>>>>> +    {}
>>>>> +};
>>>>
>>>> I wonder what is going to happen if both this driver and the hwmon
>>>> driver for the same chips are configured in a system which supports
>>>> devicetree (or any system, really). Unless I am missing something,
>>>> the result will be that both drivers will try to instantiate, and
>>>> one will fail with -EBUSY. Or the instantiated driver is more or less
>>>> random, depending on which one happens to be loaded. Not a good
>>>> situation to be in.
>>>
>>> I agree, we should put a mutual exclusion in Kconfig, plus maybe a
>>> cross-reference in the help section.
>>>
>>>>
>>>> For the time being, it might make sense to add cross-dependencies
>>>> in Kconfig to only permit one of the two drivers to be configured.
>>>>
>>>> Ultimately we may need a better solution for the iio-hwmon bridge,
>>>> one that makes the underlying driver transparent in both devicetree
>>>> properties and user space ABI. No idea how to do that, though.
>>>>
>>>
>>> IDK if ina2xx is a special case or if this matter of dual driver
>>> stacks for the same chip already occurred and requires specific
>>> plumbing. Making the user aware of the mutual of the exclusion sounds
>>> fine with me.
>>>
>> htu21. We'll drop that driver from hwmon with the 4.5 kernel.
>> We could just drop ina2xx as well, but it is more widely used
>> and referenced from dts files. The ABI changes, so I am not sure
>> if we can just do that.
>
> I changed iio/adc/Kconfig to the following:
>
> +config INA2XX_ADC
> +       tristate "Texas Instruments INA2xx Power Monitors IIO driver"
> +       depends on I2C && !SENSORS_INA2XX
> +       select REGMAP_I2C
> +       select IIO_BUFFER
> +       select IIO_KFIFO_BUF
> +       help
> +         Say yes here to build support for TI INA2xx family of Power Monitors.
> +         This driver is mutually exclusive with the HWMON version.
> +
>
>
> anything the patch should also add to hwmon/Kconfig (that will not lead to a cycling reference warning) ?
>

You could try the opposite, but I don't know if that works.

	depends on I2C && !INA2XX_ADC

Guenter

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

[Marc Titinger]

On 02/12/2015 17:44, Guenter Roeck wrote:
> On 12/02/2015 08:20 AM, Marc Titinger wrote:
>> On 02/12/2015 17:04, Guenter Roeck wrote:
>>> On 12/02/2015 02:20 AM, Marc Titinger wrote:
>>>> On 02/12/2015 03:14, Guenter Roeck wrote:
>>>>> On Mon, Nov 30, 2015 at 12:49:14PM +0100, 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>
>>>>>> ---
>>>>>>   drivers/iio/adc/Kconfig      |   9 +
>>>>>>   drivers/iio/adc/Makefile     |   1 +
>>>>>>   drivers/iio/adc/ina2xx-iio.c | 678
>>>>>> +++++++++++++++++++++++++++++++++++++++++++
>>>>>>   3 files changed, 688 insertions(+)
>>>>>>   create mode 100644 drivers/iio/adc/ina2xx-iio.c
>>>>>> +
>>>>> [ ... ]
>>>>>> +
>>>>>> +static const struct i2c_device_id ina2xx_id[] = {
>>>>>> +    {"ina219", ina219},
>>>>>> +    {"ina220", ina219},
>>>>>> +    {"ina226", ina226},
>>>>>> +    {"ina230", ina226},
>>>>>> +    {"ina231", ina226},
>>>>>> +    {}
>>>>>> +};
>>>>>
>>>>> I wonder what is going to happen if both this driver and the hwmon
>>>>> driver for the same chips are configured in a system which supports
>>>>> devicetree (or any system, really). Unless I am missing something,
>>>>> the result will be that both drivers will try to instantiate, and
>>>>> one will fail with -EBUSY. Or the instantiated driver is more or less
>>>>> random, depending on which one happens to be loaded. Not a good
>>>>> situation to be in.
>>>>
>>>> I agree, we should put a mutual exclusion in Kconfig, plus maybe a
>>>> cross-reference in the help section.
>>>>
>>>>>
>>>>> For the time being, it might make sense to add cross-dependencies
>>>>> in Kconfig to only permit one of the two drivers to be configured.
>>>>>
>>>>> Ultimately we may need a better solution for the iio-hwmon bridge,
>>>>> one that makes the underlying driver transparent in both devicetree
>>>>> properties and user space ABI. No idea how to do that, though.
>>>>>
>>>>
>>>> IDK if ina2xx is a special case or if this matter of dual driver
>>>> stacks for the same chip already occurred and requires specific
>>>> plumbing. Making the user aware of the mutual of the exclusion sounds
>>>> fine with me.
>>>>
>>> htu21. We'll drop that driver from hwmon with the 4.5 kernel.
>>> We could just drop ina2xx as well, but it is more widely used
>>> and referenced from dts files. The ABI changes, so I am not sure
>>> if we can just do that.
>>
>> I changed iio/adc/Kconfig to the following:
>>
>> +config INA2XX_ADC
>> +       tristate "Texas Instruments INA2xx Power Monitors IIO driver"
>> +       depends on I2C && !SENSORS_INA2XX
>> +       select REGMAP_I2C
>> +       select IIO_BUFFER
>> +       select IIO_KFIFO_BUF
>> +       help
>> +         Say yes here to build support for TI INA2xx family of Power
>> Monitors.
>> +         This driver is mutually exclusive with the HWMON version.
>> +
>>
>>
>> anything the patch should also add to hwmon/Kconfig (that will not
>> lead to a cycling reference warning) ?
>>
>
> You could try the opposite, but I don't know if that works.
>
>      depends on I2C && !INA2XX_ADC

I tried: it's functional, but leads to a ugly warning:

drivers/iio/adc/Kconfig:173:error: recursive dependency detected!
drivers/iio/adc/Kconfig:173:	symbol INA2XX_ADC depends on SENSORS_INA2XX
drivers/hwmon/Kconfig:1453:	symbol SENSORS_INA2XX depends on INA2XX_ADC

Anyone knows how to implement a radio button in Kconfig ? ;)


>
> Guenter
>

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

[Guenter Roeck]

On 12/02/2015 09:09 AM, Marc Titinger wrote:
> On 02/12/2015 17:44, Guenter Roeck wrote:
>> On 12/02/2015 08:20 AM, Marc Titinger wrote:
>>> On 02/12/2015 17:04, Guenter Roeck wrote:
>>>> On 12/02/2015 02:20 AM, Marc Titinger wrote:
>>>>> On 02/12/2015 03:14, Guenter Roeck wrote:
>>>>>> On Mon, Nov 30, 2015 at 12:49:14PM +0100, 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>
>>>>>>> ---
>>>>>>>   drivers/iio/adc/Kconfig      |   9 +
>>>>>>>   drivers/iio/adc/Makefile     |   1 +
>>>>>>>   drivers/iio/adc/ina2xx-iio.c | 678
>>>>>>> +++++++++++++++++++++++++++++++++++++++++++
>>>>>>>   3 files changed, 688 insertions(+)
>>>>>>>   create mode 100644 drivers/iio/adc/ina2xx-iio.c
>>>>>>> +
>>>>>> [ ... ]
>>>>>>> +
>>>>>>> +static const struct i2c_device_id ina2xx_id[] = {
>>>>>>> +    {"ina219", ina219},
>>>>>>> +    {"ina220", ina219},
>>>>>>> +    {"ina226", ina226},
>>>>>>> +    {"ina230", ina226},
>>>>>>> +    {"ina231", ina226},
>>>>>>> +    {}
>>>>>>> +};
>>>>>>
>>>>>> I wonder what is going to happen if both this driver and the hwmon
>>>>>> driver for the same chips are configured in a system which supports
>>>>>> devicetree (or any system, really). Unless I am missing something,
>>>>>> the result will be that both drivers will try to instantiate, and
>>>>>> one will fail with -EBUSY. Or the instantiated driver is more or less
>>>>>> random, depending on which one happens to be loaded. Not a good
>>>>>> situation to be in.
>>>>>
>>>>> I agree, we should put a mutual exclusion in Kconfig, plus maybe a
>>>>> cross-reference in the help section.
>>>>>
>>>>>>
>>>>>> For the time being, it might make sense to add cross-dependencies
>>>>>> in Kconfig to only permit one of the two drivers to be configured.
>>>>>>
>>>>>> Ultimately we may need a better solution for the iio-hwmon bridge,
>>>>>> one that makes the underlying driver transparent in both devicetree
>>>>>> properties and user space ABI. No idea how to do that, though.
>>>>>>
>>>>>
>>>>> IDK if ina2xx is a special case or if this matter of dual driver
>>>>> stacks for the same chip already occurred and requires specific
>>>>> plumbing. Making the user aware of the mutual of the exclusion sounds
>>>>> fine with me.
>>>>>
>>>> htu21. We'll drop that driver from hwmon with the 4.5 kernel.
>>>> We could just drop ina2xx as well, but it is more widely used
>>>> and referenced from dts files. The ABI changes, so I am not sure
>>>> if we can just do that.
>>>
>>> I changed iio/adc/Kconfig to the following:
>>>
>>> +config INA2XX_ADC
>>> +       tristate "Texas Instruments INA2xx Power Monitors IIO driver"
>>> +       depends on I2C && !SENSORS_INA2XX
>>> +       select REGMAP_I2C
>>> +       select IIO_BUFFER
>>> +       select IIO_KFIFO_BUF
>>> +       help
>>> +         Say yes here to build support for TI INA2xx family of Power
>>> Monitors.
>>> +         This driver is mutually exclusive with the HWMON version.
>>> +
>>>
>>>
>>> anything the patch should also add to hwmon/Kconfig (that will not
>>> lead to a cycling reference warning) ?
>>>
>>
>> You could try the opposite, but I don't know if that works.
>>
>>      depends on I2C && !INA2XX_ADC
>
> I tried: it's functional, but leads to a ugly warning:
>
> drivers/iio/adc/Kconfig:173:error: recursive dependency detected!
> drivers/iio/adc/Kconfig:173:    symbol INA2XX_ADC depends on SENSORS_INA2XX
> drivers/hwmon/Kconfig:1453:    symbol SENSORS_INA2XX depends on INA2XX_ADC
>
Can't do that.

> Anyone knows how to implement a radio button in Kconfig ? ;)
>
That is possible, but only in a single menu. Look for choice/endchoice.

Guenter

[PATCH v2 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 99fb73c..7575a12 100644
--- a/drivers/iio/adc/ina2xx-iio.c
+++ b/drivers/iio/adc/ina2xx-iio.c
@@ -117,6 +117,7 @@ struct ina2xx_chip_info {
 	s64 prev_ns;	/* track buffer capture time check, for underruns*/
 	int int_time_vbus; /* Bus voltage integration time uS */
 	int int_time_vshunt; /* Shunt voltage integration time uS */
+	bool allow_async_readout;
 };
 
 static const struct ina2xx_config ina2xx_config[] = {
@@ -333,6 +334,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));
+	bool val;
+	int ret;
+
+	ret = strtobool((const char *) buf, &val);
+	if (ret)
+		return ret;
+
+	chip->allow_async_readout = val;
+
+	return len;
+}
+
+
 #define INA2XX_CHAN(_type, _index, _address) { \
 	.type = _type, \
 	.address = _address, \
@@ -399,11 +427,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);
@@ -454,7 +483,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);
@@ -477,6 +507,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);
 
 	chip->prev_ns = iio_get_time_ns();
 
@@ -518,7 +549,12 @@ static int ina2xx_debug_reg(struct iio_dev *indio_dev,
 static IIO_CONST_ATTR_INT_TIME_AVAIL \
  ("0.000140 0.000204 0.000332 0.000588 0.001100 0.002116 0.004156 0.008244");
 
+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 v2 2/2] iio: ina2xx: provide a sysfs parameter to allow async readout of the ADCs

[Jonathan Cameron]

On 30/11/15 11:49, 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
> 
> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
Missing one thing.
ABI Docs in Documentation/ABI/testing/sysfs-bus-iio-ina2xx-iio.txt

> ---
>  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 99fb73c..7575a12 100644
> --- a/drivers/iio/adc/ina2xx-iio.c
> +++ b/drivers/iio/adc/ina2xx-iio.c
> @@ -117,6 +117,7 @@ struct ina2xx_chip_info {
>  	s64 prev_ns;	/* track buffer capture time check, for underruns*/
>  	int int_time_vbus; /* Bus voltage integration time uS */
>  	int int_time_vshunt; /* Shunt voltage integration time uS */
> +	bool allow_async_readout;
>  };
>  
>  static const struct ina2xx_config ina2xx_config[] = {
> @@ -333,6 +334,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));
> +	bool val;
> +	int ret;
> +
> +	ret = strtobool((const char *) buf, &val);
> +	if (ret)
> +		return ret;
> +
> +	chip->allow_async_readout = val;
> +
> +	return len;
> +}
> +
> +
>  #define INA2XX_CHAN(_type, _index, _address) { \
>  	.type = _type, \
>  	.address = _address, \
> @@ -399,11 +427,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);
> @@ -454,7 +483,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);
> @@ -477,6 +507,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);
>  
>  	chip->prev_ns = iio_get_time_ns();
>  
> @@ -518,7 +549,12 @@ static int ina2xx_debug_reg(struct iio_dev *indio_dev,
>  static IIO_CONST_ATTR_INT_TIME_AVAIL \
>   ("0.000140 0.000204 0.000332 0.000588 0.001100 0.002116 0.004156 0.008244");
>  
> +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,
>  };
>