[RFC 0/4] IIO: add support for INA2xx power monitor

[RFC 0/4] IIO: add support for INA2xx power monitor

[Marc Titinger]

This chip has fair support in the hwmon stack already, this work is more
as a pathfinder for me, hence I post it as RFC to digg some more into 'does
and donts' with IIO.

Nevertheless, it provides a working streaming scheme for capturing
power/voltage/current with this chip. It works in local and remote
mode with iio_readdev and I did some sniffing tests with iio-oscilloscope
with promising results inspite of timeout issues for long temporal buffers
presumably due to the slow rates for this chip compared to expected high-speed
CoDecs (and maybe the lack of a proper plugin?).

The kthread I'm using does an active waiting to allow for sampling periods
shorter than a tick. I have not experienced performance issues with it on
the board (BeagleBoneBlack), IIOD could always schedule on time as far as I
could see, maybe with other i2c backends a schedule() could be mandatory ?

Many thanks,

Marc Titinger (4):
  iio: ina2xx: add direct IO support for TI INA2xx Power Monitors
  iio: ina2xx: add SAMP_FREQ attribute.
  iio: ina2xx: add debugfs reg access
  iio: ina2xx: add SOFTWARE buffer mode using an iio kfifo.

 drivers/iio/adc/Kconfig      |  11 +
 drivers/iio/adc/Makefile     |   1 +
 drivers/iio/adc/ina2xx-iio.c | 579 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 591 insertions(+)
 create mode 100644 drivers/iio/adc/ina2xx-iio.c

-- 
1.9.1

[RFC 1/4] iio: ina2xx: add direct IO support for TI INA2xx Power Monitors

[Marc Titinger]

Basic support or direct IO raw read, with averaging attribute.
Values are RAW, INT_PLUS_MICRO (Volt/Ampere/Watt).

IIO context has 1 devices:
     iio:device0: ina226
      4 channels found:
        power3:  (input)
        1 channel-specific attributes found:
                attr 0: raw value: 1.150000
        voltage0:  (input)
        1 channel-specific attributes found:
                attr 0: raw value: 0.000003
        voltage1:  (input)
        1 channel-specific attributes found:
                attr 0: raw value: 4.277500
        current2:  (input)
        1 channel-specific attributes found:
                attr 0: raw value: 0.268000
      2 device-specific attributes found:
                attr 0: in_calibscale value: 10000
                attr 1: in_mean_raw value: 4


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

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index eb0cd89..b648051 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -10,6 +10,15 @@ config AD_SIGMA_DELTA
 	select IIO_BUFFER
 	select IIO_TRIGGERED_BUFFER
 
+config INA2XX_IIO
+        tristate "Texas Instruments INA2xx Power Monitors IIO driver"
+	depends on I2C
+        help
+          Say yes here to build support for TI INA2xx familly Power Monitors.
+
+	  Note that this is not the existing hwmon interface for this device.
+
+
 config AD7266
 	tristate "Analog Devices AD7265/AD7266 ADC driver"
 	depends on SPI_MASTER
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index a096210..e6a844a 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -5,6 +5,7 @@
 # When adding new entries keep the list in alphabetical order
 obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
 obj-$(CONFIG_AD7266) += ad7266.o
+obj-$(CONFIG_INA2XX_IIO) += ina2xx-iio.o
 obj-$(CONFIG_AD7291) += ad7291.o
 obj-$(CONFIG_AD7298) += ad7298.o
 obj-$(CONFIG_AD7923) += ad7923.o
diff --git a/drivers/iio/adc/ina2xx-iio.c b/drivers/iio/adc/ina2xx-iio.c
new file mode 100644
index 0000000..257d8d5
--- /dev/null
+++ b/drivers/iio/adc/ina2xx-iio.c
@@ -0,0 +1,404 @@
+/*
+ * 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.
+ */
+#include <linux/module.h>
+#include <linux/iio/iio.h>
+#include <linux/i2c.h>
+#include <linux/regmap.h>
+#include <linux/platform_data/ina2xx.h>
+
+#include <linux/util_macros.h>
+
+/*
+ * INA2XX registers definition
+ */
+/* common register definitions */
+#define INA2XX_CONFIG                   0x00
+#define INA2XX_SHUNT_VOLTAGE            0x01	/* readonly */
+#define INA2XX_BUS_VOLTAGE              0x02	/* readonly */
+#define INA2XX_POWER                    0x03	/* readonly */
+#define INA2XX_CURRENT                  0x04	/* readonly */
+#define INA2XX_CALIBRATION              0x05
+
+/* 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 INA2XX_RSHUNT_DEFAULT           10000
+
+/* bit mask for reading the averaging setting in the configuration register */
+#define INA226_AVG_RD_MASK              0x0E00
+#define INA226_READ_AVG(reg)            (((reg) & INA226_AVG_RD_MASK) >> 9)
+#define INA226_SHIFT_AVG(val)           ((val) << 9)
+
+static struct regmap_config ina2xx_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 16,
+};
+
+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 iio_dev *indio_dev;
+	const struct ina2xx_config *config;
+	struct mutex state_lock;
+	long rshunt;
+	int avg;
+	int freq;
+	int period_us;
+	struct regmap *regmap;
+};
+
+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);
+		*val = *uval/1000000;
+		*uval = *uval - *val*1000000;
+		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 - *val*1000000;
+		return IIO_VAL_INT_PLUS_MICRO;
+
+	case INA2XX_POWER:
+		*uval = regval * chip->config->power_lsb;
+		*val = *uval/1000000;
+		*uval = *uval - *val*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;
+
+	case INA2XX_CALIBRATION:
+		*val = DIV_ROUND_CLOSEST(chip->config->calibration_factor,
+					regval);
+		return IIO_VAL_INT;
+
+	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_AVERAGE_RAW:
+		*val = chip->avg;
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_CALIBSCALE:
+		ret = regmap_read(chip->regmap, INA2XX_CALIBRATION, &regval);
+		if (ret < 0)
+			return ret;
+
+		return ina2xx_get_value(chip, INA2XX_CALIBRATION, regval,
+					val, val2);
+
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int ina2xx_calibrate(struct ina2xx_chip_info *chip)
+{
+	u16 val = DIV_ROUND_CLOSEST(chip->config->calibration_factor,
+				    chip->rshunt);
+
+	return regmap_write(chip->regmap, INA2XX_CALIBRATION, val);
+}
+
+
+/*
+ * Available averaging rates for ina226. The indices correspond with
+ * the bit values expected by the chip (according to the ina226 datasheet,
+ * table 3 AVG bit settings, found at
+ * http://www.ti.com/lit/ds/symlink/ina226.pdf.
+ */
+static const int ina226_avg_tab[] = { 1, 4, 16, 64, 128, 256, 512, 1024 };
+
+static unsigned int ina226_set_average(struct ina2xx_chip_info *chip,
+				       unsigned int val,
+				       unsigned int *config)
+{
+	int bits;
+
+	if (val > 1024 || val < 1)
+		return -EINVAL;
+
+	bits = find_closest(val, ina226_avg_tab,
+			    ARRAY_SIZE(ina226_avg_tab));
+
+	chip->avg = ina226_avg_tab[bits];
+
+	*config &= ~INA226_AVG_RD_MASK;
+	*config |= INA226_SHIFT_AVG(bits) & INA226_AVG_RD_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;
+
+	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_AVERAGE_RAW:
+
+		ret = ina226_set_average(chip, val, &tmp);
+		break;
+
+	default:
+		ret = -EINVAL;
+	}
+
+	if (!ret && (tmp != config))
+		ret = regmap_write(chip->regmap, INA2XX_CONFIG, config);
+_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_AVERAGE_RAW) | \
+				   BIT(IIO_CHAN_INFO_CALIBSCALE), \
+	.scan_index = (_index), \
+	.scan_type = { \
+		.sign = 'u', \
+		.realbits = 16, \
+		.storagebits = 16, \
+		.shift = 0, \
+	.endianness = IIO_BE, \
+	} \
+}
+
+static const struct iio_chan_spec ina2xx_channels[] = {
+	INA2XX_CHAN(IIO_VOLTAGE, 0, INA2XX_SHUNT_VOLTAGE),
+	INA2XX_CHAN(IIO_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 const struct iio_info ina2xx_info = {
+	.read_raw = &ina2xx_read_raw,
+	.write_raw = &ina2xx_write_raw,
+	.driver_module = THIS_MODULE,
+};
+
+/*
+* Initialize the configuration and calibration registers.
+*/
+static int ina2xx_init(struct ina2xx_chip_info *chip, unsigned int config)
+{
+	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).
+	 */
+	return ina2xx_calibrate(chip);
+}
+
+static int ina2xx_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	struct ina2xx_chip_info *chip;
+	struct iio_dev *indio_dev;
+	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->indio_dev = indio_dev;
+
+	/* set the device type */
+	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;
+
+	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);
+
+	ret = ina2xx_init(chip, val);
+	if (ret < 0) {
+		dev_err(&client->dev, "error configuring the device: %d\n",
+			ret);
+		return -ENODEV;
+	}
+
+	return iio_device_register(indio_dev);
+}
+
+static int ina2xx_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+
+	iio_device_unregister(indio_dev);
+
+	return 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

[RFC 2/4] iio: ina2xx: add SAMP_FREQ attribute.

[Marc Titinger]

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

diff --git a/drivers/iio/adc/ina2xx-iio.c b/drivers/iio/adc/ina2xx-iio.c
index 257d8d5..92169e1 100644
--- a/drivers/iio/adc/ina2xx-iio.c
+++ b/drivers/iio/adc/ina2xx-iio.c
@@ -42,7 +42,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_AVG              4
+#define INA226_DEFAULT_FREQ             454
 
 #define INA2XX_RSHUNT_DEFAULT           10000
 
@@ -51,6 +52,8 @@
 #define INA226_READ_AVG(reg)            (((reg) & INA226_AVG_RD_MASK) >> 9)
 #define INA226_SHIFT_AVG(val)           ((val) << 9)
 
+#define INA226_SFREQ_RD_MASK            0x01f8
+
 static struct regmap_config ina2xx_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 16,
@@ -172,6 +175,10 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
 		return ina2xx_get_value(chip, INA2XX_CALIBRATION, regval,
 					val, val2);
 
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*val = chip->freq;
+		return IIO_VAL_INT;
+
 	default:
 		return -EINVAL;
 	}
@@ -216,6 +223,38 @@ static unsigned int ina226_set_average(struct ina2xx_chip_info *chip,
 	return 0;
 }
 
+/*
+ * Conversion times in uS
+ */
+static const int ina226_conv_time_tab[] = { 140, 204, 332, 588, 1100,
+	2116, 4156, 8244};
+
+static unsigned int ina226_set_frequency(struct ina2xx_chip_info *chip,
+					 unsigned int val,
+					 unsigned int *config)
+{
+	int bits;
+
+	if (val > 3550  || val < 50)
+		return -EINVAL;
+
+	/* integration time in uS, for both voltage channels */
+	val = DIV_ROUND_CLOSEST(1000000, 2 * val);
+
+	bits = find_closest(val, ina226_conv_time_tab,
+			    ARRAY_SIZE(ina226_conv_time_tab));
+
+	chip->period_us = 2 * ina226_conv_time_tab[bits];
+
+	chip->freq = DIV_ROUND_CLOSEST(1000000, chip->period_us);
+
+	*config &= ~INA226_SFREQ_RD_MASK;
+	*config |= (bits << 3) | (bits << 6);
+
+	return 0;
+}
+
+
 static int ina2xx_write_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan,
 			    int val, int val2, long mask)
@@ -238,6 +277,11 @@ static int ina2xx_write_raw(struct iio_dev *indio_dev,
 		ret = ina226_set_average(chip, val, &tmp);
 		break;
 
+	case IIO_CHAN_INFO_SAMP_FREQ:
+
+		ret = ina226_set_frequency(chip, val, &tmp);
+		break;
+
 	default:
 		ret = -EINVAL;
 	}
@@ -257,6 +301,7 @@ _err:
 	.channel = (_index), \
 	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
 	.info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_AVERAGE_RAW) | \
+					BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
 				   BIT(IIO_CHAN_INFO_CALIBSCALE), \
 	.scan_index = (_index), \
 	.scan_type = { \
@@ -355,8 +400,10 @@ static int ina2xx_probe(struct i2c_client *client,
 
 	/* Patch the current config register with default. */
 	val = chip->config->config_default;
-	if (id->driver_data == ina226)
+	if (id->driver_data == ina226) {
 		ina226_set_average(chip, INA226_DEFAULT_AVG, &val);
+		ina226_set_frequency(chip, INA226_DEFAULT_FREQ, &val);
+	}
 
 	ret = ina2xx_init(chip, val);
 	if (ret < 0) {
-- 
1.9.1

[RFC 3/4] iio: ina2xx: add debugfs reg access

[Marc Titinger]

     iio:device0: ina226
...
      3 device-specific attributes found:
                attr 0: in_calibscale value: 10000
                attr 1: in_mean_raw value: 4
                attr 2: in_sampling_frequency value: 455

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

diff --git a/drivers/iio/adc/ina2xx-iio.c b/drivers/iio/adc/ina2xx-iio.c
index 92169e1..3e4a4b0 100644
--- a/drivers/iio/adc/ina2xx-iio.c
+++ b/drivers/iio/adc/ina2xx-iio.c
@@ -321,7 +321,19 @@ static const struct iio_chan_spec ina2xx_channels[] = {
 	IIO_CHAN_SOFT_TIMESTAMP(4),
 };
 
+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);
+}
+
 static const struct iio_info ina2xx_info = {
+	.debugfs_reg_access = &ina2xx_debug_reg,
 	.read_raw = &ina2xx_read_raw,
 	.write_raw = &ina2xx_write_raw,
 	.driver_module = THIS_MODULE,
-- 
1.9.1

[RFC 4/4] iio: ina2xx: add SOFTWARE buffer mode using an iio kfifo.

[Marc Titinger]

Capture the active scan_elements into a kfifo.
The capture thread will 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).

# iio_readdev ina226 |  od -x
WARNING: High-speed mode not enabled
0000000 042f 0d5a 002e 010c 4be8 4eb4 0013 0000
0000020 0430 0d5a 002e 010c a704 4f3e 0013 0000
0000040 0430 0d5a 002e 010c b477 4fc7 0013 0000
0000060 042f 0d5b 002e 010c 8052 5050 0013 0000
0000100 042f 0d5b 002e 010c 5d92 50d8 0013 0000
0000120 0430 0d5a 002e 010c fa59 515e 0013 0000
0000140 0430 0d5b 002e 010c 95d2 51e5 0013 0000

Signed-off-by: Marc Titinger <mtitinger@baylibre.com>

---
Ina2xx does not support auto-increment, hence the capture threads sticks
with single register reads instead of regmap_bulk_read.

The proper scales must be applied to those raw register
values, I'm in favor of doing the conversion in userland in a client plugin
for instance a sigrok, of iio-osciloscope plugin, rather than converting in
kernel.
---
 drivers/iio/adc/Kconfig      |   2 +
 drivers/iio/adc/ina2xx-iio.c | 120 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 120 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index b648051..6ef8ce5 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -13,6 +13,8 @@ config AD_SIGMA_DELTA
 config INA2XX_IIO
         tristate "Texas Instruments INA2xx Power Monitors IIO driver"
 	depends on I2C
+	select IIO_BUFFER
+	select IIO_KFIFO_BUF
         help
           Say yes here to build support for TI INA2xx familly Power Monitors.
 
diff --git a/drivers/iio/adc/ina2xx-iio.c b/drivers/iio/adc/ina2xx-iio.c
index 3e4a4b0..132b8a9 100644
--- a/drivers/iio/adc/ina2xx-iio.c
+++ b/drivers/iio/adc/ina2xx-iio.c
@@ -16,7 +16,10 @@
  * Licensed under the GPL-2 or later.
  */
 #include <linux/module.h>
-#include <linux/iio/iio.h>
+#include <linux/kthread.h>
+#include <linux/delay.h>
+#include <linux/iio/kfifo_buf.h>
+
 #include <linux/i2c.h>
 #include <linux/regmap.h>
 #include <linux/platform_data/ina2xx.h>
@@ -73,6 +76,7 @@ struct ina2xx_config {
 
 struct ina2xx_chip_info {
 	struct iio_dev *indio_dev;
+	struct task_struct *task;
 	const struct ina2xx_config *config;
 	struct mutex state_lock;
 	long rshunt;
@@ -263,6 +267,9 @@ static int ina2xx_write_raw(struct iio_dev *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);
@@ -321,6 +328,106 @@ static const struct iio_chan_spec ina2xx_channels[] = {
 	IIO_CHAN_SOFT_TIMESTAMP(4),
 };
 
+/*
+ * return uS until next due sampling.
+ */
+
+static s64 prev_ns;
+
+static int ina2xx_work_buffer(struct ina2xx_chip_info *chip)
+{
+	unsigned short data[8];
+	struct iio_dev *indio_dev = chip->indio_dev;
+	int bit, ret = 0, i = 0;
+	unsigned long buffer_us = 0, elapsed_us = 0;
+	s64 time_a, time_b;
+
+	time_a = iio_get_time_ns();
+
+	/* 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(chip->indio_dev,
+					   (unsigned int *)data, time_a);
+
+	buffer_us = (unsigned long)(time_b - time_a) / 1000;
+	elapsed_us = (unsigned long)(time_a - prev_ns) / 1000;
+
+	trace_printk("uS: elapsed: %lu, buf: %lu\n", elapsed_us, buffer_us);
+
+	prev_ns = time_a;
+_err:
+	return buffer_us;
+};
+
+static int ina2xx_capture_thread(void *data)
+{
+	struct ina2xx_chip_info *chip = (struct ina2xx_chip_info *)data;
+	unsigned int sampling_us = chip->period_us * chip->avg;
+	unsigned long buffer_us;
+
+	do {
+		buffer_us = ina2xx_work_buffer(chip);
+
+		if (sampling_us > buffer_us)
+			udelay(sampling_us - buffer_us);
+
+	} while (!kthread_should_stop());
+
+	chip->task = NULL;
+
+	return 0;
+}
+
+int ina2xx_buffer_enable(struct iio_dev *indio_dev)
+{
+	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
+	unsigned int sampling_us = chip->period_us * chip->avg;
+
+	trace_printk("Enabling buffer w/ scan_mask %02x, freq = %d, avg =%u\n",
+		     (unsigned int)(*indio_dev->active_scan_mask), chip->freq,
+		     chip->avg);
+
+	trace_printk("Expected work period is %u us\n", sampling_us);
+
+	prev_ns = iio_get_time_ns();
+
+	chip->task = kthread_run(ina2xx_capture_thread, (void *)chip,
+				 "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);
+
+	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)
 {
@@ -360,6 +467,7 @@ static int ina2xx_probe(struct i2c_client *client,
 {
 	struct ina2xx_chip_info *chip;
 	struct iio_dev *indio_dev;
+	struct iio_buffer *buffer;
 	int ret;
 	unsigned int val;
 
@@ -402,7 +510,7 @@ static int ina2xx_probe(struct i2c_client *client,
 
 	indio_dev->dev.parent = &client->dev;
 	indio_dev->info = &ina2xx_info;
-	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
 
 	chip->regmap = devm_regmap_init_i2c(client, &ina2xx_regmap_config);
 	if (IS_ERR(chip->regmap)) {
@@ -424,6 +532,14 @@ static int ina2xx_probe(struct i2c_client *client,
 		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);
 }
 
-- 
1.9.1

Re: [RFC 4/4] iio: ina2xx: add SOFTWARE buffer mode using an iio kfifo.

[Lars-Peter Clausen]

On 11/10/2015 05:07 PM, Marc Titinger wrote:
> Capture the active scan_elements into a kfifo.
> The capture thread will 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).
> 
> # iio_readdev ina226 |  od -x
> WARNING: High-speed mode not enabled
> 0000000 042f 0d5a 002e 010c 4be8 4eb4 0013 0000
> 0000020 0430 0d5a 002e 010c a704 4f3e 0013 0000
> 0000040 0430 0d5a 002e 010c b477 4fc7 0013 0000
> 0000060 042f 0d5b 002e 010c 8052 5050 0013 0000
> 0000100 042f 0d5b 002e 010c 5d92 50d8 0013 0000
> 0000120 0430 0d5a 002e 010c fa59 515e 0013 0000
> 0000140 0430 0d5b 002e 010c 95d2 51e5 0013 0000
> 
> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>

Interesting approach. I think if we are going to due this we want to make
this kind of emulation generic. Have you seen the software trigger and
configfs support patches[1] from Daniel? It kind of achieves the same as you
do, but using hrtimers.

> 
> ---
> Ina2xx does not support auto-increment, hence the capture threads sticks
> with single register reads instead of regmap_bulk_read.
> 
> The proper scales must be applied to those raw register
> values, I'm in favor of doing the conversion in userland in a client plugin

Yes, conversion should not be done in kernel space, we don't want to impose
the performance penalty on users which don't need it and you can typically
do it faster in userspace anyway where you have floats and SSE and what not.

> for instance a sigrok

Slightly OT, but do you already have some Sigrok IIO support? I have this
scheduled for end of the month, maybe we can align our strategies here and
avoid duplicated work.

- Lars

[1] https://lkml.org/lkml/2015/8/10/877

Re: [RFC 1/4] iio: ina2xx: add direct IO support for TI INA2xx Power Monitors

[Daniel Baluta]

On Tue, Nov 10, 2015 at 6:07 PM, Marc Titinger <mtitinger@baylibre.com> wrote:
> Basic support or direct IO raw read, with averaging attribute.
> Values are RAW, INT_PLUS_MICRO (Volt/Ampere/Watt).
>
> IIO context has 1 devices:
Is this from libiio right? Perhaps you should specify this:

"libiio context has 1 devices"

>      iio:device0: ina226
>       4 channels found:
>         power3:  (input)
>         1 channel-specific attributes found:
>                 attr 0: raw value: 1.150000
>         voltage0:  (input)
>         1 channel-specific attributes found:
>                 attr 0: raw value: 0.000003
>         voltage1:  (input)
>         1 channel-specific attributes found:
>                 attr 0: raw value: 4.277500
>         current2:  (input)
>         1 channel-specific attributes found:
>                 attr 0: raw value: 0.268000
>       2 device-specific attributes found:
>                 attr 0: in_calibscale value: 10000
>                 attr 1: in_mean_raw value: 4
>
>

Link to datasheet?

> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
> ---
>  drivers/iio/adc/Kconfig      |   9 +
>  drivers/iio/adc/Makefile     |   1 +
>  drivers/iio/adc/ina2xx-iio.c | 404 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 414 insertions(+)
>  create mode 100644 drivers/iio/adc/ina2xx-iio.c
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index eb0cd89..b648051 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -10,6 +10,15 @@ config AD_SIGMA_DELTA
>         select IIO_BUFFER
>         select IIO_TRIGGERED_BUFFER
>
> +config INA2XX_IIO
> +        tristate "Texas Instruments INA2xx Power Monitors IIO driver"
> +       depends on I2C

Since you are using regmap you should select it here.

> +        help
> +          Say yes here to build support for TI INA2xx familly Power Monitors.
> +
> +         Note that this is not the existing hwmon interface for this device.
> +
> +

Config symbol should be in alphabetical order.

>  config AD7266
>         tristate "Analog Devices AD7265/AD7266 ADC driver"
>         depends on SPI_MASTER
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index a096210..e6a844a 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -5,6 +5,7 @@
>  # When adding new entries keep the list in alphabetical order
>  obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
>  obj-$(CONFIG_AD7266) += ad7266.o
> +obj-$(CONFIG_INA2XX_IIO) += ina2xx-iio.o

The same here.

>  obj-$(CONFIG_AD7291) += ad7291.o
>  obj-$(CONFIG_AD7298) += ad7298.o
>  obj-$(CONFIG_AD7923) += ad7923.o
> diff --git a/drivers/iio/adc/ina2xx-iio.c b/drivers/iio/adc/ina2xx-iio.c
> new file mode 100644
> index 0000000..257d8d5
> --- /dev/null
> +++ b/drivers/iio/adc/ina2xx-iio.c
> @@ -0,0 +1,404 @@
> +/*
> + * 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.

If you know the I2C address its recommended to mention it here.

> + */
> +#include <linux/module.h>
> +#include <linux/iio/iio.h>
> +#include <linux/i2c.h>
> +#include <linux/regmap.h>
> +#include <linux/platform_data/ina2xx.h>
> +
> +#include <linux/util_macros.h>

Nice, didn't know about this :).

> +
> +/*
> + * INA2XX registers definition
> + */
> +/* common register definitions */
> +#define INA2XX_CONFIG                   0x00
> +#define INA2XX_SHUNT_VOLTAGE            0x01   /* readonly */
> +#define INA2XX_BUS_VOLTAGE              0x02   /* readonly */
> +#define INA2XX_POWER                    0x03   /* readonly */
> +#define INA2XX_CURRENT                  0x04   /* readonly */
> +#define INA2XX_CALIBRATION              0x05
> +
> +/* 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 INA2XX_RSHUNT_DEFAULT           10000
> +
> +/* bit mask for reading the averaging setting in the configuration register */
> +#define INA226_AVG_RD_MASK              0x0E00
> +#define INA226_READ_AVG(reg)            (((reg) & INA226_AVG_RD_MASK) >> 9)
> +#define INA226_SHIFT_AVG(val)           ((val) << 9)
> +
> +static struct regmap_config ina2xx_regmap_config = {
> +       .reg_bits = 8,
> +       .val_bits = 16,
Specify here which registers are cacheable, read/write.

> +};
> +
> +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 iio_dev *indio_dev;
> +       const struct ina2xx_config *config;
> +       struct mutex state_lock;
> +       long rshunt;
> +       int avg;
> +       int freq;
> +       int period_us;
> +       struct regmap *regmap;
> +};
> +
> +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);
> +               *val = *uval/1000000;
> +               *uval = *uval - *val*1000000;
> +               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 - *val*1000000;
> +               return IIO_VAL_INT_PLUS_MICRO;
> +
> +       case INA2XX_POWER:
> +               *uval = regval * chip->config->power_lsb;
> +               *val = *uval/1000000;
> +               *uval = *uval - *val*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;
> +
> +       case INA2XX_CALIBRATION:
> +               *val = DIV_ROUND_CLOSEST(chip->config->calibration_factor,
> +                                       regval);
> +               return IIO_VAL_INT;
> +
> +       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_AVERAGE_RAW:
> +               *val = chip->avg;
> +               return IIO_VAL_INT;
> +
> +       case IIO_CHAN_INFO_CALIBSCALE:
> +               ret = regmap_read(chip->regmap, INA2XX_CALIBRATION, &regval);
> +               if (ret < 0)
> +                       return ret;
> +
> +               return ina2xx_get_value(chip, INA2XX_CALIBRATION, regval,
> +                                       val, val2);
> +
> +       default:
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +
> +static int ina2xx_calibrate(struct ina2xx_chip_info *chip)
> +{
> +       u16 val = DIV_ROUND_CLOSEST(chip->config->calibration_factor,
> +                                   chip->rshunt);
> +
> +       return regmap_write(chip->regmap, INA2XX_CALIBRATION, val);
> +}
> +
> +
> +/*
> + * Available averaging rates for ina226. The indices correspond with
> + * the bit values expected by the chip (according to the ina226 datasheet,
> + * table 3 AVG bit settings, found at
> + * http://www.ti.com/lit/ds/symlink/ina226.pdf.
> + */
> +static const int ina226_avg_tab[] = { 1, 4, 16, 64, 128, 256, 512, 1024 };
> +
> +static unsigned int ina226_set_average(struct ina2xx_chip_info *chip,
> +                                      unsigned int val,
> +                                      unsigned int *config)
> +{
> +       int bits;
> +
> +       if (val > 1024 || val < 1)
> +               return -EINVAL;
> +
> +       bits = find_closest(val, ina226_avg_tab,
> +                           ARRAY_SIZE(ina226_avg_tab));
> +
> +       chip->avg = ina226_avg_tab[bits];
> +
> +       *config &= ~INA226_AVG_RD_MASK;
> +       *config |= INA226_SHIFT_AVG(bits) & INA226_AVG_RD_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;
> +
> +       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_AVERAGE_RAW:
> +
> +               ret = ina226_set_average(chip, val, &tmp);
> +               break;
> +
> +       default:
> +               ret = -EINVAL;
> +       }
> +
> +       if (!ret && (tmp != config))
> +               ret = regmap_write(chip->regmap, INA2XX_CONFIG, config);
> +_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_AVERAGE_RAW) | \
> +                                  BIT(IIO_CHAN_INFO_CALIBSCALE), \
> +       .scan_index = (_index), \
> +       .scan_type = { \
> +               .sign = 'u', \
> +               .realbits = 16, \
> +               .storagebits = 16, \
> +               .shift = 0, \
> +       .endianness = IIO_BE, \
> +       } \
> +}
> +
> +static const struct iio_chan_spec ina2xx_channels[] = {
> +       INA2XX_CHAN(IIO_VOLTAGE, 0, INA2XX_SHUNT_VOLTAGE),
> +       INA2XX_CHAN(IIO_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 const struct iio_info ina2xx_info = {
> +       .read_raw = &ina2xx_read_raw,
> +       .write_raw = &ina2xx_write_raw,
> +       .driver_module = THIS_MODULE,
> +};
> +
> +/*
> +* Initialize the configuration and calibration registers.
> +*/
> +static int ina2xx_init(struct ina2xx_chip_info *chip, unsigned int config)
> +{
> +       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).
> +        */
> +       return ina2xx_calibrate(chip);
> +}
> +
> +static int ina2xx_probe(struct i2c_client *client,
> +                       const struct i2c_device_id *id)
> +{
> +       struct ina2xx_chip_info *chip;
> +       struct iio_dev *indio_dev;
> +       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->indio_dev = indio_dev;
> +
> +       /* set the device type */
> +       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;
> +
> +       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);
> +
> +       ret = ina2xx_init(chip, val);
> +       if (ret < 0) {
> +               dev_err(&client->dev, "error configuring the device: %d\n",
> +                       ret);
> +               return -ENODEV;
> +       }
> +
> +       return iio_device_register(indio_dev);

If there is no reverse operation for ina2xx_init (e.g ina2xx_poweroff) then here
you can use devm_iio_device_register and completely remove
ina2xx_remove function.


> +}
> +
> +static int ina2xx_remove(struct i2c_client *client)
> +{
> +       struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +
> +       iio_device_unregister(indio_dev);
> +
> +       return 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: [RFC 2/4] iio: ina2xx: add SAMP_FREQ attribute.

[Daniel Baluta]

On Tue, Nov 10, 2015 at 6:07 PM, Marc Titinger <mtitinger@baylibre.com> wrote:
> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>

I'm in favor of small incremental patches, anyhow since this is the
initial submission
I can't see why this patch isn't squashed in the previous one?

> ---
>  drivers/iio/adc/ina2xx-iio.c | 51 ++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 49 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/adc/ina2xx-iio.c b/drivers/iio/adc/ina2xx-iio.c
> index 257d8d5..92169e1 100644
> --- a/drivers/iio/adc/ina2xx-iio.c
> +++ b/drivers/iio/adc/ina2xx-iio.c
> @@ -42,7 +42,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_AVG              4
> +#define INA226_DEFAULT_FREQ             454
>
>  #define INA2XX_RSHUNT_DEFAULT           10000
>
> @@ -51,6 +52,8 @@
>  #define INA226_READ_AVG(reg)            (((reg) & INA226_AVG_RD_MASK) >> 9)
>  #define INA226_SHIFT_AVG(val)           ((val) << 9)
>
> +#define INA226_SFREQ_RD_MASK            0x01f8
> +
>  static struct regmap_config ina2xx_regmap_config = {
>         .reg_bits = 8,
>         .val_bits = 16,
> @@ -172,6 +175,10 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
>                 return ina2xx_get_value(chip, INA2XX_CALIBRATION, regval,
>                                         val, val2);
>
> +       case IIO_CHAN_INFO_SAMP_FREQ:
> +               *val = chip->freq;
> +               return IIO_VAL_INT;
> +
>         default:
>                 return -EINVAL;
>         }
> @@ -216,6 +223,38 @@ static unsigned int ina226_set_average(struct ina2xx_chip_info *chip,
>         return 0;
>  }
>
> +/*
> + * Conversion times in uS
> + */
> +static const int ina226_conv_time_tab[] = { 140, 204, 332, 588, 1100,
> +       2116, 4156, 8244};
> +
> +static unsigned int ina226_set_frequency(struct ina2xx_chip_info *chip,
> +                                        unsigned int val,
> +                                        unsigned int *config)
> +{
> +       int bits;
> +
> +       if (val > 3550  || val < 50)
> +               return -EINVAL;
> +
> +       /* integration time in uS, for both voltage channels */
> +       val = DIV_ROUND_CLOSEST(1000000, 2 * val);
> +
> +       bits = find_closest(val, ina226_conv_time_tab,
> +                           ARRAY_SIZE(ina226_conv_time_tab));
> +
> +       chip->period_us = 2 * ina226_conv_time_tab[bits];
> +
> +       chip->freq = DIV_ROUND_CLOSEST(1000000, chip->period_us);
> +
> +       *config &= ~INA226_SFREQ_RD_MASK;
> +       *config |= (bits << 3) | (bits << 6);
> +
> +       return 0;
> +}
> +
> +
>  static int ina2xx_write_raw(struct iio_dev *indio_dev,
>                             struct iio_chan_spec const *chan,
>                             int val, int val2, long mask)
> @@ -238,6 +277,11 @@ static int ina2xx_write_raw(struct iio_dev *indio_dev,
>                 ret = ina226_set_average(chip, val, &tmp);
>                 break;
>
> +       case IIO_CHAN_INFO_SAMP_FREQ:
> +
> +               ret = ina226_set_frequency(chip, val, &tmp);
> +               break;
> +
>         default:
>                 ret = -EINVAL;
>         }
> @@ -257,6 +301,7 @@ _err:
>         .channel = (_index), \
>         .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
>         .info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_AVERAGE_RAW) | \
> +                                       BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
>                                    BIT(IIO_CHAN_INFO_CALIBSCALE), \
>         .scan_index = (_index), \
>         .scan_type = { \
> @@ -355,8 +400,10 @@ static int ina2xx_probe(struct i2c_client *client,
>
>         /* Patch the current config register with default. */
>         val = chip->config->config_default;
> -       if (id->driver_data == ina226)
> +       if (id->driver_data == ina226) {
>                 ina226_set_average(chip, INA226_DEFAULT_AVG, &val);
> +               ina226_set_frequency(chip, INA226_DEFAULT_FREQ, &val);
> +       }
>
>         ret = ina2xx_init(chip, val);
>         if (ret < 0) {
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 1/4] iio: ina2xx: add direct IO support for TI INA2xx Power Monitors

[Daniel Baluta]

<snip>

> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
> ---
>  drivers/iio/adc/Kconfig      |   9 +
>  drivers/iio/adc/Makefile     |   1 +
>  drivers/iio/adc/ina2xx-iio.c | 404 +++++++++++++++++++++++++++++++++++++++++++

One more thing. In IIO we do not prefer generic names for files.
Lets name this after the first device in the family.

Here I would say we should call it drivers/iio/adc/ina219.c. Skip the
-iio suffix
from file name, because it is already under the iio/ directory.

If there is a module name conflict with hwmon module we can name the
.ko resulted
from this file ina219-iio.ko.

Re: [RFC 1/4] iio: ina2xx: add direct IO support for TI INA2xx Power Monitors

[Marc Titinger]

On 11/11/2015 11:14, Daniel Baluta wrote:
> On Tue, Nov 10, 2015 at 6:07 PM, Marc Titinger <mtitinger@baylibre.com> wrote:
>> Basic support or direct IO raw read, with averaging attribute.
>> Values are RAW, INT_PLUS_MICRO (Volt/Ampere/Watt).
>>

Hi Daniel,

thanks for the review,
comments bellow,

Marc.


>> IIO context has 1 devices:
> Is this from libiio right? Perhaps you should specify this:
>
> "libiio context has 1 devices"

It's the raw output from iio_info to give a quick overview of the features.

>
>>       iio:device0: ina226
>>        4 channels found:
>>          power3:  (input)
>>          1 channel-specific attributes found:
>>                  attr 0: raw value: 1.150000
>>          voltage0:  (input)
>>          1 channel-specific attributes found:
>>                  attr 0: raw value: 0.000003
>>          voltage1:  (input)
>>          1 channel-specific attributes found:
>>                  attr 0: raw value: 4.277500
>>          current2:  (input)
>>          1 channel-specific attributes found:
>>                  attr 0: raw value: 0.268000
>>        2 device-specific attributes found:
>>                  attr 0: in_calibscale value: 10000
>>                  attr 1: in_mean_raw value: 4
>>
>>
>
> Link to datasheet?

Ok, will add it.

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


...


>>
>> +config INA2XX_IIO
>> +        tristate "Texas Instruments INA2xx Power Monitors IIO driver"
>> +       depends on I2C
>
> Since you are using regmap you should select it here.

Right.

>
>> +        help
>> +          Say yes here to build support for TI INA2xx familly Power Monitors.
>> +
>> +         Note that this is not the existing hwmon interface for this device.
>> +
>> +
>
> Config symbol should be in alphabetical order.

Ok
>
...


>>   obj-$(CONFIG_AD7266) += ad7266.o
>> +obj-$(CONFIG_INA2XX_IIO) += ina2xx-iio.o
>
> The same here.

Ok.

>
>>   obj-$(CONFIG_AD7291) += ad7291.o
>>   obj-$(CONFIG_AD7298) += ad7298.o
>>   obj-$(CONFIG_AD7923) += ad7923.o
>> diff --git a/drivers/iio/adc/ina2xx-iio.c b/drivers/iio/adc/ina2xx-iio.c
>> new file mode 100644
>> index 0000000..257d8d5
>> --- /dev/null
>> +++ b/drivers/iio/adc/ina2xx-iio.c
>> @@ -0,0 +1,404 @@
>> +/*
>> + * INA2XX Current and Power Monitors
>> + *

...

>> + *
>> + * Licensed under the GPL-2 or later.
>
> If you know the I2C address its recommended to mention it here.

ok.

...

>> +
>> +static struct regmap_config ina2xx_regmap_config = {
>> +       .reg_bits = 8,
>> +       .val_bits = 16,
> Specify here which registers are cacheable, read/write.

ok.

...

>> +
>> +       return iio_device_register(indio_dev);
>
> If there is no reverse operation for ina2xx_init (e.g ina2xx_poweroff) then here
> you can use devm_iio_device_register and completely remove
> ina2xx_remove function.

right, thanks!

Re: [RFC 1/4] iio: ina2xx: add direct IO support for TI INA2xx Power Monitors

[Marc Titinger]

On 11/11/2015 13:09, Daniel Baluta wrote:
> <snip>
>
>> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
>> ---
>>   drivers/iio/adc/Kconfig      |   9 +
>>   drivers/iio/adc/Makefile     |   1 +
>>   drivers/iio/adc/ina2xx-iio.c | 404 +++++++++++++++++++++++++++++++++++++++++++
>
> One more thing. In IIO we do not prefer generic names for files.
> Lets name this after the first device in the family.
>
> Here I would say we should call it drivers/iio/adc/ina219.c. Skip the
> -iio suffix
> from file name, because it is already under the iio/ directory.

I get your point, but in this specific case there is already support for 
this chip family in hwmon and both drivers make sense because both 
applications exist (hardware monitoring or power measurement equipement).

To avoid confusion it may be good to have at least a common radix for 
the CONFIG option, so that searching returns both options.

>
> If there is a module name conflict with hwmon module we can name the
> .ko resulted
> from this file ina219-iio.ko.
>

Re: [RFC 4/4] iio: ina2xx: add SOFTWARE buffer mode using an iio kfifo.

[Marc Titinger]

On 10/11/2015 19:23, Lars-Peter Clausen wrote:
> On 11/10/2015 05:07 PM, Marc Titinger wrote:
>> Capture the active scan_elements into a kfifo.
>> The capture thread will 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).
>>
>> # iio_readdev ina226 |  od -x
>> WARNING: High-speed mode not enabled
>> 0000000 042f 0d5a 002e 010c 4be8 4eb4 0013 0000
>> 0000020 0430 0d5a 002e 010c a704 4f3e 0013 0000
>> 0000040 0430 0d5a 002e 010c b477 4fc7 0013 0000
>> 0000060 042f 0d5b 002e 010c 8052 5050 0013 0000
>> 0000100 042f 0d5b 002e 010c 5d92 50d8 0013 0000
>> 0000120 0430 0d5a 002e 010c fa59 515e 0013 0000
>> 0000140 0430 0d5b 002e 010c 95d2 51e5 0013 0000
>>
>> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>

Hi Lars,

>
> Interesting approach. I think if we are going to due this we want to make
> this kind of emulation generic. Have you seen the software trigger and
> configfs support patches[1] from Daniel? It kind of achieves the same as you
> do, but using hrtimers.

I totally agree, let me have a look on those patches maybe I could add 
an active waiting scheme for platforms w/o hrtimers ?

>
>>
>> ---
>> Ina2xx does not support auto-increment, hence the capture threads sticks
>> with single register reads instead of regmap_bulk_read.
>>
>> The proper scales must be applied to those raw register
>> values, I'm in favor of doing the conversion in userland in a client plugin
>
> Yes, conversion should not be done in kernel space, we don't want to impose
> the performance penalty on users which don't need it and you can typically
> do it faster in userspace anyway where you have floats and SSE and what not.
>
>> for instance a sigrok
>
> Slightly OT, but do you already have some Sigrok IIO support? I have this
> scheduled for end of the month, maybe we can align our strategies here and
> avoid duplicated work.

How fortunate! I've started some preliminary work like cloning the demo 
driver into a skeletton for 'hardware/generic-iio/api.c', adding the 
build/ac plumbing, and linking to libiio with the idea of using iio_info 
to create a generic enumeration of the iio-context into sigrok channels.

Now, I'm not familiar with Glib and it might not be my prio until a 
couple of weeks, so I'd be super happy to wait for you if you are keen 
to do that part :)

What would be the best spot to chat about this ?


Marc.


>
> - Lars
>
> [1] https://lkml.org/lkml/2015/8/10/877
>

Re: [RFC 4/4] iio: ina2xx: add SOFTWARE buffer mode using an iio kfifo.

[Lars-Peter Clausen]

On 11/12/2015 11:18 AM, Marc Titinger wrote:
[...]
>> Slightly OT, but do you already have some Sigrok IIO support? I have this
>> scheduled for end of the month, maybe we can align our strategies here and
>> avoid duplicated work.
> 
> How fortunate! I've started some preliminary work like cloning the demo
> driver into a skeletton for 'hardware/generic-iio/api.c', adding the
> build/ac plumbing, and linking to libiio with the idea of using iio_info to
> create a generic enumeration of the iio-context into sigrok channels.
> 
> Now, I'm not familiar with Glib and it might not be my prio until a couple
> of weeks, so I'd be super happy to wait for you if you are keen to do that
> part :)
> 
> What would be the best spot to chat about this ?

#sigrok on irc.freenode.net

[RFC v2 1/2] iio: ina2xx: add direct IO support for TI INA2xx Power Monitors

[Marc Titinger]

Basic support or direct IO raw read, with averaging attribute.
Values are RAW, INT_PLUS_MICRO (Volt/Ampere/Watt).

Output of iio_info:

     iio:device0: ina226
      4 channels found:
        power3:  (input)
        1 channel-specific attributes found:
                attr 0: raw value: 1.150000
        voltage0:  (input)
        1 channel-specific attributes found:
                attr 0: raw value: 0.000003
        voltage1:  (input)
        1 channel-specific attributes found:
                attr 0: raw value: 4.277500
        current2:  (input)
        1 channel-specific attributes found:
                attr 0: raw value: 0.268000
      2 device-specific attributes found:
                attr 0: in_calibscale value: 10000
                attr 1: in_mean_raw value: 4
                attr 2: in_sampling_frequency value: 455

Tested with ina226, on BeagleBoneBlack.

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

Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
---

v2 of series https://lkml.org/lkml/2015/11/10/370 :

        - squash patches adding SAMPL_FREQ and debugfs interface
        - add regmap is_volatile and is_writeable callbacks
        - fix Kconfig deps and alphabetical sorting
        - simplification: use devm_iio_device_register

---

 drivers/iio/adc/Kconfig      |   9 +
 drivers/iio/adc/Makefile     |   1 +
 drivers/iio/adc/ina2xx-iio.c | 472 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 482 insertions(+)
 create mode 100644 drivers/iio/adc/ina2xx-iio.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index eb0cd89..087e5bd 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
+	help
+	  Say yes here to build support for TI INA2xx familly Power Monitors.
+
+	  Note that this is not the existing hwmon interface for this device.
+
 config LP8788_ADC
 	tristate "LP8788 ADC driver"
 	depends on MFD_LP8788
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index a096210..74e4341 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_BERLIN2_ADC) += berlin2-adc.o
 obj-$(CONFIG_DA9150_GPADC) += da9150-gpadc.o
 obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o
 obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
+obj-$(CONFIG_INA2XX_IIO) += ina2xx-iio.o
 obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
 obj-$(CONFIG_MAX1027) += max1027.o
 obj-$(CONFIG_MAX1363) += max1363.o
diff --git a/drivers/iio/adc/ina2xx-iio.c b/drivers/iio/adc/ina2xx-iio.c
new file mode 100644
index 0000000..28b7919
--- /dev/null
+++ b/drivers/iio/adc/ina2xx-iio.c
@@ -0,0 +1,472 @@
+/*
+ * 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/iio/iio.h>
+#include <linux/i2c.h>
+#include <linux/regmap.h>
+#include <linux/platform_data/ina2xx.h>
+
+#include <linux/util_macros.h>
+
+/*
+ * INA2XX registers definition
+ */
+/* common register definitions */
+#define INA2XX_CONFIG                   0x00
+#define INA2XX_SHUNT_VOLTAGE            0x01	/* readonly */
+#define INA2XX_BUS_VOLTAGE              0x02	/* readonly */
+#define INA2XX_POWER                    0x03	/* readonly */
+#define INA2XX_CURRENT                  0x04	/* readonly */
+#define INA2XX_CALIBRATION              0x05
+
+/* 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_FREQ             454
+
+#define INA2XX_RSHUNT_DEFAULT           10000
+
+/* bit mask for reading the averaging setting in the configuration register */
+#define INA226_AVG_RD_MASK              0x0E00
+#define INA226_READ_AVG(reg)            (((reg) & INA226_AVG_RD_MASK) >> 9)
+#define INA226_SHIFT_AVG(val)           ((val) << 9)
+
+#define INA226_SFREQ_RD_MASK            0x01f8
+
+
+static bool ina2xx_is_writeable_reg(struct device *dev, unsigned int reg)
+{
+	return (reg == INA2XX_CONFIG)
+	    || (reg == INA2XX_CALIBRATION);
+}
+
+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 iio_dev *indio_dev;
+	const struct ina2xx_config *config;
+	struct mutex state_lock;
+	long rshunt;
+	int avg;
+	int freq;
+	int period_us;
+	struct regmap *regmap;
+};
+
+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);
+		*val = *uval/1000000;
+		*uval = *uval - *val*1000000;
+		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 - *val*1000000;
+		return IIO_VAL_INT_PLUS_MICRO;
+
+	case INA2XX_POWER:
+		*uval = regval * chip->config->power_lsb;
+		*val = *uval/1000000;
+		*uval = *uval - *val*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;
+
+	case INA2XX_CALIBRATION:
+		*val = DIV_ROUND_CLOSEST(chip->config->calibration_factor,
+					regval);
+		return IIO_VAL_INT;
+
+	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_AVERAGE_RAW:
+		*val = chip->avg;
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_CALIBSCALE:
+		ret = regmap_read(chip->regmap, INA2XX_CALIBRATION, &regval);
+		if (ret < 0)
+			return ret;
+
+		return ina2xx_get_value(chip, INA2XX_CALIBRATION, regval,
+					val, val2);
+
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*val = chip->freq;
+		return IIO_VAL_INT;
+
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int ina2xx_calibrate(struct ina2xx_chip_info *chip)
+{
+	u16 val = DIV_ROUND_CLOSEST(chip->config->calibration_factor,
+				    chip->rshunt);
+
+	return regmap_write(chip->regmap, INA2XX_CALIBRATION, val);
+}
+
+
+/*
+ * Available averaging rates for ina226. The indices correspond with
+ * the bit values expected by the chip (according to the ina226 datasheet,
+ * table 3 AVG bit settings, found at
+ * http://www.ti.com/lit/ds/symlink/ina226.pdf.
+ */
+static const int ina226_avg_tab[] = { 1, 4, 16, 64, 128, 256, 512, 1024 };
+
+static unsigned int ina226_set_average(struct ina2xx_chip_info *chip,
+				       unsigned int val,
+				       unsigned int *config)
+{
+	int bits;
+
+	if (val > 1024 || val < 1)
+		return -EINVAL;
+
+	bits = find_closest(val, ina226_avg_tab,
+			    ARRAY_SIZE(ina226_avg_tab));
+
+	chip->avg = ina226_avg_tab[bits];
+
+	*config &= ~INA226_AVG_RD_MASK;
+	*config |= INA226_SHIFT_AVG(bits) & INA226_AVG_RD_MASK;
+
+	return 0;
+}
+
+/*
+ * Conversion times in uS
+ */
+static const int ina226_conv_time_tab[] = { 140, 204, 332, 588, 1100,
+	2116, 4156, 8244};
+
+static unsigned int ina226_set_frequency(struct ina2xx_chip_info *chip,
+					 unsigned int val,
+					 unsigned int *config)
+{
+	int bits;
+
+	if (val > 3550  || val < 50)
+		return -EINVAL;
+
+	/* integration time in uS, for both voltage channels */
+	val = DIV_ROUND_CLOSEST(1000000, 2 * val);
+
+	bits = find_closest(val, ina226_conv_time_tab,
+			    ARRAY_SIZE(ina226_conv_time_tab));
+
+	chip->period_us = 2 * ina226_conv_time_tab[bits];
+
+	chip->freq = DIV_ROUND_CLOSEST(1000000, chip->period_us);
+
+	*config &= ~INA226_SFREQ_RD_MASK;
+	*config |= (bits << 3) | (bits << 6);
+
+	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;
+
+	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_AVERAGE_RAW:
+
+		ret = ina226_set_average(chip, val, &tmp);
+		break;
+
+	case IIO_CHAN_INFO_SAMP_FREQ:
+
+		ret = ina226_set_frequency(chip, val, &tmp);
+		break;
+
+	default:
+		ret = -EINVAL;
+	}
+
+	if (!ret && (tmp != config))
+		ret = regmap_write(chip->regmap, INA2XX_CONFIG, config);
+_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_AVERAGE_RAW) | \
+					BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
+				   BIT(IIO_CHAN_INFO_CALIBSCALE), \
+	.scan_index = (_index), \
+	.scan_type = { \
+		.sign = 'u', \
+		.realbits = 16, \
+		.storagebits = 16, \
+		.shift = 0, \
+	.endianness = IIO_BE, \
+	} \
+}
+
+static const struct iio_chan_spec ina2xx_channels[] = {
+	INA2XX_CHAN(IIO_VOLTAGE, 0, INA2XX_SHUNT_VOLTAGE),
+	INA2XX_CHAN(IIO_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_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);
+}
+
+static const struct iio_info ina2xx_info = {
+	.debugfs_reg_access = &ina2xx_debug_reg,
+	.read_raw = &ina2xx_read_raw,
+	.write_raw = &ina2xx_write_raw,
+	.driver_module = THIS_MODULE,
+};
+
+/*
+* Initialize the configuration and calibration registers.
+*/
+static int ina2xx_init(struct ina2xx_chip_info *chip, unsigned int config)
+{
+	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).
+	 */
+	return ina2xx_calibrate(chip);
+}
+
+static int ina2xx_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	struct ina2xx_chip_info *chip;
+	struct iio_dev *indio_dev;
+	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->indio_dev = indio_dev;
+
+	/* set the device type */
+	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;
+
+	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_frequency(chip, INA226_DEFAULT_FREQ, &val);
+	}
+
+	ret = ina2xx_init(chip, val);
+	if (ret < 0) {
+		dev_err(&client->dev, "error configuring the device: %d\n",
+			ret);
+		return -ENODEV;
+	}
+
+	return devm_iio_device_register(&client->dev, indio_dev);
+}
+
+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,
+	.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

[RFC v2 2/2] iio: ina2xx: add SOFTWARE buffer mode using an iio kfifo.

[Marc Titinger]

Capture the active scan_elements into a kfifo.
The capture thread will 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).

# iio_readdev ina226 |  od -x
WARNING: High-speed mode not enabled
0000000 042f 0d5a 002e 010c 4be8 4eb4 0013 0000
0000020 0430 0d5a 002e 010c a704 4f3e 0013 0000
0000040 0430 0d5a 002e 010c b477 4fc7 0013 0000
0000060 042f 0d5b 002e 010c 8052 5050 0013 0000
0000100 042f 0d5b 002e 010c 5d92 50d8 0013 0000
0000120 0430 0d5a 002e 010c fa59 515e 0013 0000
0000140 0430 0d5b 002e 010c 95d2 51e5 0013 0000

Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
---
 drivers/iio/adc/Kconfig      |   1 +
 drivers/iio/adc/ina2xx-iio.c | 119 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 118 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 087e5bd..9b87372 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -174,6 +174,7 @@ config INA2XX_IIO
 	tristate "Texas Instruments INA2xx Power Monitors IIO driver"
 	depends on I2C
 	select REGMAP_I2C
+	select IIO_BUFFER
 	help
 	  Say yes here to build support for TI INA2xx familly Power Monitors.
 
diff --git a/drivers/iio/adc/ina2xx-iio.c b/drivers/iio/adc/ina2xx-iio.c
index 28b7919..4c11c80 100644
--- a/drivers/iio/adc/ina2xx-iio.c
+++ b/drivers/iio/adc/ina2xx-iio.c
@@ -20,7 +20,9 @@
  * Configurable 7-bit I2C slave address from 0x40 to 0x4F
  */
 #include <linux/module.h>
-#include <linux/iio/iio.h>
+#include <linux/kthread.h>
+#include <linux/delay.h>
+#include <linux/iio/kfifo_buf.h>
 #include <linux/i2c.h>
 #include <linux/regmap.h>
 #include <linux/platform_data/ina2xx.h>
@@ -92,6 +94,7 @@ struct ina2xx_config {
 
 struct ina2xx_chip_info {
 	struct iio_dev *indio_dev;
+	struct task_struct *task;
 	const struct ina2xx_config *config;
 	struct mutex state_lock;
 	long rshunt;
@@ -282,6 +285,9 @@ static int ina2xx_write_raw(struct iio_dev *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);
@@ -340,6 +346,106 @@ static const struct iio_chan_spec ina2xx_channels[] = {
 	IIO_CHAN_SOFT_TIMESTAMP(4),
 };
 
+/*
+ * return uS until next due sampling.
+ */
+
+static s64 prev_ns;
+
+static int ina2xx_work_buffer(struct ina2xx_chip_info *chip)
+{
+	unsigned short data[8];
+	struct iio_dev *indio_dev = chip->indio_dev;
+	int bit, ret = 0, i = 0;
+	unsigned long buffer_us = 0, elapsed_us = 0;
+	s64 time_a, time_b;
+
+	time_a = iio_get_time_ns();
+
+	/* 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(chip->indio_dev,
+					   (unsigned int *)data, time_a);
+
+	buffer_us = (unsigned long)(time_b - time_a) / 1000;
+	elapsed_us = (unsigned long)(time_a - prev_ns) / 1000;
+
+	trace_printk("uS: elapsed: %lu, buf: %lu\n", elapsed_us, buffer_us);
+
+	prev_ns = time_a;
+_err:
+	return buffer_us;
+};
+
+static int ina2xx_capture_thread(void *data)
+{
+	struct ina2xx_chip_info *chip = (struct ina2xx_chip_info *)data;
+	unsigned int sampling_us = chip->period_us * chip->avg;
+	unsigned long buffer_us;
+
+	do {
+		buffer_us = ina2xx_work_buffer(chip);
+
+		if (sampling_us > buffer_us)
+			udelay(sampling_us - buffer_us);
+
+	} while (!kthread_should_stop());
+
+	chip->task = NULL;
+
+	return 0;
+}
+
+int ina2xx_buffer_enable(struct iio_dev *indio_dev)
+{
+	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
+	unsigned int sampling_us = chip->period_us * chip->avg;
+
+	trace_printk("Enabling buffer w/ scan_mask %02x, freq = %d, avg =%u\n",
+		     (unsigned int)(*indio_dev->active_scan_mask), chip->freq,
+		     chip->avg);
+
+	trace_printk("Expected work period is %u us\n", sampling_us);
+
+	prev_ns = iio_get_time_ns();
+
+	chip->task = kthread_run(ina2xx_capture_thread, (void *)chip,
+				 "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);
+
+	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)
 {
@@ -379,6 +485,7 @@ static int ina2xx_probe(struct i2c_client *client,
 {
 	struct ina2xx_chip_info *chip;
 	struct iio_dev *indio_dev;
+	struct iio_buffer *buffer;
 	int ret;
 	unsigned int val;
 
@@ -421,7 +528,7 @@ static int ina2xx_probe(struct i2c_client *client,
 
 	indio_dev->dev.parent = &client->dev;
 	indio_dev->info = &ina2xx_info;
-	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
 
 	chip->regmap = devm_regmap_init_i2c(client, &ina2xx_regmap_config);
 	if (IS_ERR(chip->regmap)) {
@@ -443,6 +550,14 @@ static int ina2xx_probe(struct i2c_client *client,
 		return -ENODEV;
 	}
 
+	buffer = devm_iio_kfifo_allocate(&indio_dev->dev);
+	if (!buffer)
+		return -ENOMEM;
+
+	indio_dev->setup_ops = &ina2xx_setup_ops;
+
+	iio_device_attach_buffer(indio_dev, buffer);
+
 	return devm_iio_device_register(&client->dev, indio_dev);
 }
 
-- 
1.9.1

Re: [RFC 4/4] iio: ina2xx: add SOFTWARE buffer mode using an iio kfifo.

[Jonathan Cameron]

On 12/11/15 10:18, Marc Titinger wrote:
> On 10/11/2015 19:23, Lars-Peter Clausen wrote:
>> On 11/10/2015 05:07 PM, Marc Titinger wrote:
>>> Capture the active scan_elements into a kfifo.
>>> The capture thread will 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).
>>>
>>> # iio_readdev ina226 |  od -x
>>> WARNING: High-speed mode not enabled
>>> 0000000 042f 0d5a 002e 010c 4be8 4eb4 0013 0000
>>> 0000020 0430 0d5a 002e 010c a704 4f3e 0013 0000
>>> 0000040 0430 0d5a 002e 010c b477 4fc7 0013 0000
>>> 0000060 042f 0d5b 002e 010c 8052 5050 0013 0000
>>> 0000100 042f 0d5b 002e 010c 5d92 50d8 0013 0000
>>> 0000120 0430 0d5a 002e 010c fa59 515e 0013 0000
>>> 0000140 0430 0d5b 002e 010c 95d2 51e5 0013 0000
>>>
>>> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
> 
> Hi Lars,
> 
>>
>> Interesting approach. I think if we are going to due this we want to make
>> this kind of emulation generic. Have you seen the software trigger and
>> configfs support patches[1] from Daniel? It kind of achieves the same as you
>> do, but using hrtimers.
> 

> I totally agree, let me have a look on those patches maybe I could
> add an active waiting scheme for platforms w/o hrtimers ?
I've no idea if this is a remotely common case any more but in theory
I'd have no objection to such a patch though I would like it to be
a stand alone trigger similar to that used in the patch Daniel has
submitted.

>>
>>>
>>> ---
>>> Ina2xx does not support auto-increment, hence the capture threads sticks
>>> with single register reads instead of regmap_bulk_read.
>>>
>>> The proper scales must be applied to those raw register
>>> values, I'm in favor of doing the conversion in userland in a client plugin
>>
>> Yes, conversion should not be done in kernel space, we don't want to impose
>> the performance penalty on users which don't need it and you can typically
>> do it faster in userspace anyway where you have floats and SSE and what not.
>>
>>> for instance a sigrok
>>
>> Slightly OT, but do you already have some Sigrok IIO support? I have this
>> scheduled for end of the month, maybe we can align our strategies here and
>> avoid duplicated work.
> 

> How fortunate! I've started some preliminary work like cloning the
> demo driver into a skeletton for 'hardware/generic-iio/api.c', adding
> the build/ac plumbing, and linking to libiio with the idea of using
> iio_info to create a generic enumeration of the iio-context into
> sigrok channels.
> 
> Now, I'm not familiar with Glib and it might not be my prio until a
> couple of weeks, so I'd be super happy to wait for you if you are
> keen to do that part :)
> 
> What would be the best spot to chat about this ?
> 

> Marc.
> 
> 
>>
>> - Lars
>>
>> [1] https://lkml.org/lkml/2015/8/10/877
>>
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jonathan Cameron]

On 12/11/15 12:57, Marc Titinger wrote:
> Basic support or direct IO raw read, with averaging attribute.
> Values are RAW, INT_PLUS_MICRO (Volt/Ampere/Watt).
> 
> Output of iio_info:
> 
>      iio:device0: ina226
>       4 channels found:
>         power3:  (input)
>         1 channel-specific attributes found:
>                 attr 0: raw value: 1.150000
>         voltage0:  (input)
>         1 channel-specific attributes found:
>                 attr 0: raw value: 0.000003
>         voltage1:  (input)
>         1 channel-specific attributes found:
>                 attr 0: raw value: 4.277500
>         current2:  (input)
>         1 channel-specific attributes found:
>                 attr 0: raw value: 0.268000
>       2 device-specific attributes found:
>                 attr 0: in_calibscale value: 10000
>                 attr 1: in_mean_raw value: 4
>                 attr 2: in_sampling_frequency value: 455
> 
> Tested with ina226, on BeagleBoneBlack.
> 
> Datasheet: http://www.ti.com/lit/gpn/ina226
> 
> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
Hi Marc

To express a personal preference, please don't have series of patches as
replies to the original thread.  It gets really hard to follow after
a couple of revisions!

May seem a random question, but what do you want to gain from an IIO
driver over what the hwmon provides?

Various bits inline..

Mostly looks pretty good though.

Jonathan
> ---
> 
> v2 of series https://lkml.org/lkml/2015/11/10/370 :
> 
>         - squash patches adding SAMPL_FREQ and debugfs interface
>         - add regmap is_volatile and is_writeable callbacks
>         - fix Kconfig deps and alphabetical sorting
>         - simplification: use devm_iio_device_register
> 
> ---
> 
>  drivers/iio/adc/Kconfig      |   9 +
>  drivers/iio/adc/Makefile     |   1 +
>  drivers/iio/adc/ina2xx-iio.c | 472 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 482 insertions(+)
>  create mode 100644 drivers/iio/adc/ina2xx-iio.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index eb0cd89..087e5bd 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
> +	help
> +	  Say yes here to build support for TI INA2xx familly Power Monitors.
> +
> +	  Note that this is not the existing hwmon interface for this device.
> +
>  config LP8788_ADC
>  	tristate "LP8788 ADC driver"
>  	depends on MFD_LP8788
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index a096210..74e4341 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -19,6 +19,7 @@ obj-$(CONFIG_BERLIN2_ADC) += berlin2-adc.o
>  obj-$(CONFIG_DA9150_GPADC) += da9150-gpadc.o
>  obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o
>  obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
> +obj-$(CONFIG_INA2XX_IIO) += ina2xx-iio.o
>  obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>  obj-$(CONFIG_MAX1027) += max1027.o
>  obj-$(CONFIG_MAX1363) += max1363.o
> diff --git a/drivers/iio/adc/ina2xx-iio.c b/drivers/iio/adc/ina2xx-iio.c
> new file mode 100644
> index 0000000..28b7919
> --- /dev/null
> +++ b/drivers/iio/adc/ina2xx-iio.c
> @@ -0,0 +1,472 @@
> +/*
> + * 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/iio/iio.h>
> +#include <linux/i2c.h>
> +#include <linux/regmap.h>
> +#include <linux/platform_data/ina2xx.h>
> +
> +#include <linux/util_macros.h>
> +
> +/*
> + * INA2XX registers definition
> + */
> +/* common register definitions */
> +#define INA2XX_CONFIG                   0x00
> +#define INA2XX_SHUNT_VOLTAGE            0x01	/* readonly */
> +#define INA2XX_BUS_VOLTAGE              0x02	/* readonly */
> +#define INA2XX_POWER                    0x03	/* readonly */
> +#define INA2XX_CURRENT                  0x04	/* readonly */
> +#define INA2XX_CALIBRATION              0x05
> +
> +/* 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_FREQ             454
> +
> +#define INA2XX_RSHUNT_DEFAULT           10000
> +
> +/* bit mask for reading the averaging setting in the configuration register */
> +#define INA226_AVG_RD_MASK              0x0E00
genmask is always good for these.

> +#define INA226_READ_AVG(reg)            (((reg) & INA226_AVG_RD_MASK) >> 9)
> +#define INA226_SHIFT_AVG(val)           ((val) << 9)
> +
> +#define INA226_SFREQ_RD_MASK            0x01f8
> +
> +
> +static bool ina2xx_is_writeable_reg(struct device *dev, unsigned int reg)
> +{
> +	return (reg == INA2XX_CONFIG)
> +	    || (reg == INA2XX_CALIBRATION);
On one line I think this is still way less than 80 chars..
> +}
> +
> +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 iio_dev *indio_dev;
Having a pointer back to the indio_dev is usually a sign of
something 'unusual' going on...  Will be interesting to see what it is for ;)

Ah, that was easy, you don't use it that I can see.

> +	const struct ina2xx_config *config;
> +	struct mutex state_lock;
> +	long rshunt;
> +	int avg;
> +	int freq;
> +	int period_us;
> +	struct regmap *regmap;
> +};
> +
> +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);
> +		*val = *uval/1000000;
> +		*uval = *uval - *val*1000000;
> +		return IIO_VAL_INT_PLUS_MICRO;
I'm somewhat unconvinced that this wrapper is adding anything over
just doing this where you care about the result.
Also, this is a straight forward linear conversion.  Do it in userspace
by providing the relevant 'scale' element.
> +
> +	case INA2XX_BUS_VOLTAGE:
> +		*uval = (regval >> chip->config->bus_voltage_shift)
> +			* chip->config->bus_voltage_lsb;
> +		*val = *uval/1000000;
> +		*uval = *uval - *val*1000000;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	case INA2XX_POWER:
> +		*uval = regval * chip->config->power_lsb;
> +		*val = *uval/1000000;
> +		*uval = *uval - *val*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;
> +
> +	case INA2XX_CALIBRATION:
> +		*val = DIV_ROUND_CLOSEST(chip->config->calibration_factor,
> +					regval);
> +		return IIO_VAL_INT;
> +
> +	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_AVERAGE_RAW:
> +		*val = chip->avg;
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_CALIBSCALE:
> +		ret = regmap_read(chip->regmap, INA2XX_CALIBRATION, &regval);
> +		if (ret < 0)
> +			return ret;
> +
> +		return ina2xx_get_value(chip, INA2XX_CALIBRATION, regval,
> +					val, val2);
> +
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*val = chip->freq;
> +		return IIO_VAL_INT;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ina2xx_calibrate(struct ina2xx_chip_info *chip)
> +{
> +	u16 val = DIV_ROUND_CLOSEST(chip->config->calibration_factor,
> +				    chip->rshunt);
> +
> +	return regmap_write(chip->regmap, INA2XX_CALIBRATION, val);
> +}
> +
> +
> +/*
> + * Available averaging rates for ina226. The indices correspond with
> + * the bit values expected by the chip (according to the ina226 datasheet,
> + * table 3 AVG bit settings, found at
> + * http://www.ti.com/lit/ds/symlink/ina226.pdf.
> + */
> +static const int ina226_avg_tab[] = { 1, 4, 16, 64, 128, 256, 512, 1024 };
> +
> +static unsigned int ina226_set_average(struct ina2xx_chip_info *chip,
> +				       unsigned int val,
> +				       unsigned int *config)
> +{
> +	int bits;
> +
> +	if (val > 1024 || val < 1)
> +		return -EINVAL;
> +
> +	bits = find_closest(val, ina226_avg_tab,
> +			    ARRAY_SIZE(ina226_avg_tab));
> +
> +	chip->avg = ina226_avg_tab[bits];
> +
> +	*config &= ~INA226_AVG_RD_MASK;
> +	*config |= INA226_SHIFT_AVG(bits) & INA226_AVG_RD_MASK;
> +
> +	return 0;
> +}
> +
> +/*
> + * Conversion times in uS
> + */
> +static const int ina226_conv_time_tab[] = { 140, 204, 332, 588, 1100,
> +	2116, 4156, 8244};
> +
> +static unsigned int ina226_set_frequency(struct ina2xx_chip_info *chip,
> +					 unsigned int val,
> +					 unsigned int *config)
> +{
> +	int bits;
> +
> +	if (val > 3550  || val < 50)
> +		return -EINVAL;
> +
> +	/* integration time in uS, for both voltage channels */
> +	val = DIV_ROUND_CLOSEST(1000000, 2 * val);
> +
> +	bits = find_closest(val, ina226_conv_time_tab,
> +			    ARRAY_SIZE(ina226_conv_time_tab));
> +
> +	chip->period_us = 2 * ina226_conv_time_tab[bits];
> +
> +	chip->freq = DIV_ROUND_CLOSEST(1000000, chip->period_us);
> +
> +	*config &= ~INA226_SFREQ_RD_MASK;
> +	*config |= (bits << 3) | (bits << 6);
> +
> +	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;
> +
> +	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_AVERAGE_RAW:
> +
> +		ret = ina226_set_average(chip, val, &tmp);
This isn't what the ABI uses the info_average_raw attribute for - it's
for outputing the average of a channel rather than setting a mean
filter width (which is what you have here).  Probably need some new ABI
for this as our current filter description ABI is rather limited!

> +		break;
> +
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +
> +		ret = ina226_set_frequency(chip, val, &tmp);
> +		break;
> +
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +	if (!ret && (tmp != config))
> +		ret = regmap_write(chip->regmap, INA2XX_CONFIG, config);
> +_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_AVERAGE_RAW) | \
> +					BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
> +				   BIT(IIO_CHAN_INFO_CALIBSCALE), \
> +	.scan_index = (_index), \
> +	.scan_type = { \
> +		.sign = 'u', \
> +		.realbits = 16, \
> +		.storagebits = 16, \
> +		.shift = 0, \
> +	.endianness = IIO_BE, \
> +	} \
> +}
> +
> +static const struct iio_chan_spec ina2xx_channels[] = {
> +	INA2XX_CHAN(IIO_VOLTAGE, 0, INA2XX_SHUNT_VOLTAGE),
> +	INA2XX_CHAN(IIO_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_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);
> +}
> +
> +static const struct iio_info ina2xx_info = {
> +	.debugfs_reg_access = &ina2xx_debug_reg,
> +	.read_raw = &ina2xx_read_raw,
> +	.write_raw = &ina2xx_write_raw,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +/*
Single line comment, use single line comment syntax.
> +* Initialize the configuration and calibration registers.
> +*/
> +static int ina2xx_init(struct ina2xx_chip_info *chip, unsigned int config)
> +{
> +	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).
> +	 */
> +	return ina2xx_calibrate(chip);
> +}
> +
> +static int ina2xx_probe(struct i2c_client *client,
> +			const struct i2c_device_id *id)
> +{
> +	struct ina2xx_chip_info *chip;
> +	struct iio_dev *indio_dev;
> +	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->indio_dev = indio_dev;
> +
> +	/* set the device type */
> +	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;
> +
> +	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_frequency(chip, INA226_DEFAULT_FREQ, &val);
> +	}
> +
> +	ret = ina2xx_init(chip, val);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "error configuring the device: %d\n",
> +			ret);
> +		return -ENODEV;
> +	}
> +
> +	return devm_iio_device_register(&client->dev, indio_dev);
> +}
> +
> +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,
> +	.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: [RFC v2 1/2] iio: ina2xx: add direct IO support for TI INA2xx Power Monitors

[Marc Titinger]

On 14/11/2015 19:59, Jonathan Cameron wrote:
> On 12/11/15 12:57, Marc Titinger wrote:
>> Basic support or direct IO raw read, with averaging attribute.
>> Values are RAW, INT_PLUS_MICRO (Volt/Ampere/Watt).
>>
>> Output of iio_info:
>>
>>       iio:device0: ina226
>>        4 channels found:
>>          power3:  (input)
>>          1 channel-specific attributes found:
>>                  attr 0: raw value: 1.150000
>>          voltage0:  (input)
>>          1 channel-specific attributes found:
>>                  attr 0: raw value: 0.000003
>>          voltage1:  (input)
>>          1 channel-specific attributes found:
>>                  attr 0: raw value: 4.277500
>>          current2:  (input)
>>          1 channel-specific attributes found:
>>                  attr 0: raw value: 0.268000
>>        2 device-specific attributes found:
>>                  attr 0: in_calibscale value: 10000
>>                  attr 1: in_mean_raw value: 4
>>                  attr 2: in_sampling_frequency value: 455
>>
>> Tested with ina226, on BeagleBoneBlack.
>>
>> Datasheet: http://www.ti.com/lit/gpn/ina226
>>
>> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
> Hi Marc
>

Hi Jonathan,

> To express a personal preference, please don't have series of patches as
> replies to the original thread.  It gets really hard to follow after
> a couple of revisions!
>
Ok, sorry for that.

> May seem a random question, but what do you want to gain from an IIO
> driver over what the hwmon provides?

Good question. In the frame of Baylibre's ACME power and temperature 
monitoring demo based on Sigrock, we wish to add a remote mode for the 
GUI and processing front-end as well as explore other possibilities like 
using an on-board accelerator to capture the sensor data and stream it 
back to the front-end. This work is a pathfinder as IIO seems 
appropriate for what we want to do.

>
> Various bits inline..

Thanks for the review, further questions below!

Marc.

>
> Mostly looks pretty good though.
>
> Jonathan
>> ---
>>

...

>> +#define INA2XX_RSHUNT_DEFAULT           10000
>> +
>> +/* bit mask for reading the averaging setting in the configuration register */
>> +#define INA226_AVG_RD_MASK              0x0E00
> genmask is always good for these.
>

ok.

..

>> +
>> +static bool ina2xx_is_writeable_reg(struct device *dev, unsigned int reg)
>> +{
>> +	return (reg == INA2XX_CONFIG)
>> +	    || (reg == INA2XX_CALIBRATION);
> On one line I think this is still way less than 80 chars..
>> +}

ok

...


>> +struct ina2xx_chip_info {
>> +	struct iio_dev *indio_dev;
> Having a pointer back to the indio_dev is usually a sign of
> something 'unusual' going on...  Will be interesting to see what it is for ;)
>
> Ah, that was easy, you don't use it that I can see.
>

Ah, forgot to remove it when splitting into two patches, but yeah you're 
right, I shall pass the indio_dev ptr as data in the first place.

...

>> +
>> +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);
>> +		*val = *uval/1000000;
>> +		*uval = *uval - *val*1000000;
>> +		return IIO_VAL_INT_PLUS_MICRO;
> I'm somewhat unconvinced that this wrapper is adding anything over
> just doing this where you care about the result.
> Also, this is a straight forward linear conversion.  Do it in userspace
> by providing the relevant 'scale' element.

got it! A practical question: can you specify a divider rather than a 
multiplier as a scale so that userspace does the division?

>> +
>> +	case INA2XX_BUS_VOLTAGE:
>> +		*uval = (regval >> chip->config->bus_voltage_shift)
>> +			* chip->config->bus_voltage_lsb;
>> +		*val = *uval/1000000;
>> +		*uval = *uval - *val*1000000;
>> +		return IIO_VAL_INT_PLUS_MICRO;
...

>> +	tmp = config;
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_AVERAGE_RAW:
>> +
>> +		ret = ina226_set_average(chip, val, &tmp);
> This isn't what the ABI uses the info_average_raw attribute for - it's
> for outputing the average of a channel rather than setting a mean
> filter width (which is what you have here).  Probably need some new ABI
> for this as our current filter description ABI is rather limited!
>
Ah ok, should this go into a sysfs attribute then, until the ABI section 
is defined ? How about specifying the ABI section while we are at it ?

...

.driver_module = THIS_MODULE,
>> +};
>> +
>> +/*
> Single line comment, use single line comment syntax.

ok

Re: [RFC 4/4] iio: ina2xx: add SOFTWARE buffer mode using an iio kfifo.

[Marc Titinger]

On 14/11/2015 19:44, Jonathan Cameron wrote:
> On 12/11/15 10:18, Marc Titinger wrote:
>> On 10/11/2015 19:23, Lars-Peter Clausen wrote:
>>> On 11/10/2015 05:07 PM, Marc Titinger wrote:
>>>> Capture the active scan_elements into a kfifo.
>>>> The capture thread will 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).
>>>>
>>>> # iio_readdev ina226 |  od -x
>>>> WARNING: High-speed mode not enabled
>>>> 0000000 042f 0d5a 002e 010c 4be8 4eb4 0013 0000
>>>> 0000020 0430 0d5a 002e 010c a704 4f3e 0013 0000
>>>> 0000040 0430 0d5a 002e 010c b477 4fc7 0013 0000
>>>> 0000060 042f 0d5b 002e 010c 8052 5050 0013 0000
>>>> 0000100 042f 0d5b 002e 010c 5d92 50d8 0013 0000
>>>> 0000120 0430 0d5a 002e 010c fa59 515e 0013 0000
>>>> 0000140 0430 0d5b 002e 010c 95d2 51e5 0013 0000
>>>>
>>>> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
>>
>> Hi Lars,
>>
>>>
>>> Interesting approach. I think if we are going to due this we want to make
>>> this kind of emulation generic. Have you seen the software trigger and
>>> configfs support patches[1] from Daniel? It kind of achieves the same as you
>>> do, but using hrtimers.
>>
>
>> I totally agree, let me have a look on those patches maybe I could
>> add an active waiting scheme for platforms w/o hrtimers ?

> I've no idea if this is a remotely common case any more but in theory
> I'd have no objection to such a patch though I would like it to be
> a stand alone trigger similar to that used in the patch Daniel has
> submitted.
>

Yes, I've started looking into something like a 'polling' trigger class. 
I understand that I'd need to support triggered buffer mode in the 
driver in this case. The current patch is rather simple, but having a 
generic and documented solution seems a good idea.

>>>
>>>>
>>>> ---
>>>> Ina2xx does not support auto-increment, hence the capture threads sticks
>>>> with single register reads instead of regmap_bulk_read.
>>>>
>>>> The proper scales must be applied to those raw register
>>>> values, I'm in favor of doing the conversion in userland in a client plugin
>>>
>>> Yes, conversion should not be done in kernel space, we don't want to impose
>>> the performance penalty on users which don't need it and you can typically
>>> do it faster in userspace anyway where you have floats and SSE and what not.
>>>
>>>> for instance a sigrok
>>>
>>> Slightly OT, but do you already have some Sigrok IIO support? I have this
>>> scheduled for end of the month, maybe we can align our strategies here and
>>> avoid duplicated work.
>>
>
>> How fortunate! I've started some preliminary work like cloning the
>> demo driver into a skeletton for 'hardware/generic-iio/api.c', adding
>> the build/ac plumbing, and linking to libiio with the idea of using
>> iio_info to create a generic enumeration of the iio-context into
>> sigrok channels.
>>
>> Now, I'm not familiar with Glib and it might not be my prio until a
>> couple of weeks, so I'd be super happy to wait for you if you are
>> keen to do that part :)
>>
>> What would be the best spot to chat about this ?
>>
>
>> Marc.
>>
>>
>>>
>>> - Lars
>>>
>>> [1] https://lkml.org/lkml/2015/8/10/877
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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

[Jonathan Cameron]

On 16 November 2015 09:31:17 GMT+00:00, Marc Titinger <mtitinger@baylibre.com> wrote:
>On 14/11/2015 19:59, Jonathan Cameron wrote:
>> On 12/11/15 12:57, Marc Titinger wrote:
>>> Basic support or direct IO raw read, with averaging attribute.
>>> Values are RAW, INT_PLUS_MICRO (Volt/Ampere/Watt).
>>>
>>> Output of iio_info:
>>>
>>>       iio:device0: ina226
>>>        4 channels found:
>>>          power3:  (input)
>>>          1 channel-specific attributes found:
>>>                  attr 0: raw value: 1.150000
>>>          voltage0:  (input)
>>>          1 channel-specific attributes found:
>>>                  attr 0: raw value: 0.000003
>>>          voltage1:  (input)
>>>          1 channel-specific attributes found:
>>>                  attr 0: raw value: 4.277500
>>>          current2:  (input)
>>>          1 channel-specific attributes found:
>>>                  attr 0: raw value: 0.268000
>>>        2 device-specific attributes found:
>>>                  attr 0: in_calibscale value: 10000
>>>                  attr 1: in_mean_raw value: 4
>>>                  attr 2: in_sampling_frequency value: 455
>>>
>>> Tested with ina226, on BeagleBoneBlack.
>>>
>>> Datasheet: http://www.ti.com/lit/gpn/ina226
>>>
>>> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
>> Hi Marc
>>
>
>Hi Jonathan,
>
>> To express a personal preference, please don't have series of patches
>as
>> replies to the original thread.  It gets really hard to follow after
>> a couple of revisions!
>>
>Ok, sorry for that.
>
>> May seem a random question, but what do you want to gain from an IIO
>> driver over what the hwmon provides?
>
>Good question. In the frame of Baylibre's ACME power and temperature 
>monitoring demo based on Sigrock, we wish to add a remote mode for the 
>GUI and processing front-end as well as explore other possibilities
>like 
>using an on-board accelerator to capture the sensor data and stream it 
>back to the front-end. This work is a pathfinder as IIO seems 
>appropriate for what we want to do.
Fair enough I guess. Will need to check we aren't stepping on too many toes in
 hwmon if we merge a second driver...
>
>>
>> Various bits inline..
>
>Thanks for the review, further questions below!
>
>Marc.
>
>>
>> Mostly looks pretty good though.
>>
>> Jonathan
>>> ---
>>>
>
>...
>
>>> +#define INA2XX_RSHUNT_DEFAULT           10000
>>> +
>>> +/* bit mask for reading the averaging setting in the configuration
>register */
>>> +#define INA226_AVG_RD_MASK              0x0E00
>> genmask is always good for these.
>>
>
>ok.
>
>..
>
>>> +
>>> +static bool ina2xx_is_writeable_reg(struct device *dev, unsigned
>int reg)
>>> +{
>>> +	return (reg == INA2XX_CONFIG)
>>> +	    || (reg == INA2XX_CALIBRATION);
>> On one line I think this is still way less than 80 chars..
>>> +}
>
>ok
>
>...
>
>
>>> +struct ina2xx_chip_info {
>>> +	struct iio_dev *indio_dev;
>> Having a pointer back to the indio_dev is usually a sign of
>> something 'unusual' going on...  Will be interesting to see what it
>is for ;)
>>
>> Ah, that was easy, you don't use it that I can see.
>>
>
>Ah, forgot to remove it when splitting into two patches, but yeah
>you're 
>right, I shall pass the indio_dev ptr as data in the first place.
>
>...
>
>>> +
>>> +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);
>>> +		*val = *uval/1000000;
>>> +		*uval = *uval - *val*1000000;
>>> +		return IIO_VAL_INT_PLUS_MICRO;
>> I'm somewhat unconvinced that this wrapper is adding anything over
>> just doing this where you care about the result.
>> Also, this is a straight forward linear conversion.  Do it in
>userspace
>> by providing the relevant 'scale' element.
>
>got it! A practical question: can you specify a divider rather than a 
>multiplier as a scale so that userspace does the division?
Sort of see IIO_TYPE_FRACTIONAL

>
>>> +
>>> +	case INA2XX_BUS_VOLTAGE:
>>> +		*uval = (regval >> chip->config->bus_voltage_shift)
>>> +			* chip->config->bus_voltage_lsb;
>>> +		*val = *uval/1000000;
>>> +		*uval = *uval - *val*1000000;
>>> +		return IIO_VAL_INT_PLUS_MICRO;
>...
>
>>> +	tmp = config;
>>> +
>>> +	switch (mask) {
>>> +	case IIO_CHAN_INFO_AVERAGE_RAW:
>>> +
>>> +		ret = ina226_set_average(chip, val, &tmp);
>> This isn't what the ABI uses the info_average_raw attribute for -
>it's
>> for outputing the average of a channel rather than setting a mean
>> filter width (which is what you have here).  Probably need some new
>ABI
>> for this as our current filter description ABI is rather limited!
>>
>Ah ok, should this go into a sysfs attribute then, until the ABI
>section 
>is defined ? How about specifying the ABI section while we are at it ?

Go ahead and have a go at defining a suitable ABI :)

Post it as a separate RFC though as discussion might be rather in depth and
not really driver related. 
>
>...
>
>.driver_module = THIS_MODULE,
>>> +};
>>> +
>>> +/*
>> Single line comment, use single line comment syntax.
>
>ok

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.