2015-12-07 09:09:48

by Marc Titinger

[permalink] [raw]
Subject: [PATCH v3 0/2] IIO version of INA2xx

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

* rename ina2xx-iio to ian2xx-adc for consistency with other chips
(ina2xx being the hwmon driver).
* add an exclusion in the iio/adc/Kconfig: depends on !SENSORS_INA2XX
* more typos and style fixes
* make regmap config const with .max_register = INA2XX_MAX_REGISTERS

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

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

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

--
1.9.1


2015-12-07 09:11:43

by Marc Titinger

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

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

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

Tested with ina226, on BeagleBoneBlack.

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

Signed-off-by: Marc Titinger <[email protected]>
---
drivers/iio/adc/Kconfig | 10 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/ina2xx-adc.c | 670 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 681 insertions(+)
create mode 100644 drivers/iio/adc/ina2xx-adc.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index eb0cd89..e5fdc7d 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -170,6 +170,16 @@ config EXYNOS_ADC
of SoCs for drivers such as the touchscreen and hwmon to use to share
this resource.

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

2015-12-07 09:09:59

by Marc Titinger

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

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

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

Signed-off-by: Marc Titinger <[email protected]>
---
drivers/iio/adc/ina2xx-adc.c | 54 ++++++++++++++++++++++++++++++++++++--------
1 file changed, 45 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 0e7c474..615c203 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -111,6 +111,7 @@ struct ina2xx_chip_info {
s64 prev_ns; /* track buffer capture time, check for underruns*/
int int_time_vbus; /* Bus voltage integration time uS */
int int_time_vshunt; /* Shunt voltage integration time uS */
+ bool allow_async_readout;
};

static const struct ina2xx_config ina2xx_config[] = {
@@ -326,6 +327,33 @@ _err:
}


+static ssize_t ina2xx_allow_async_readout_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
+
+ return sprintf(buf, "%d\n", chip->allow_async_readout);
+}
+
+static ssize_t ina2xx_allow_async_readout_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t len)
+{
+ struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
+ bool val;
+ int ret;
+
+ ret = strtobool((const char *) buf, &val);
+ if (ret)
+ return ret;
+
+ chip->allow_async_readout = val;
+
+ return len;
+}
+
+
#define INA2XX_CHAN(_type, _index, _address) { \
.type = (_type), \
.address = (_address), \
@@ -392,16 +420,17 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
* GPIO a triggered buffer could be used instead.
* For now, we pay for that extra read of the ALERT register
*/
- do {
- ret = regmap_read(chip->regmap, INA226_ALERT_MASK,
- &alert);
- if (ret < 0)
- return ret;
+ if (!chip->allow_async_readout)
+ do {
+ ret = regmap_read(chip->regmap, INA226_ALERT_MASK,
+ &alert);
+ if (ret < 0)
+ return ret;

- alert &= INA266_CVRF;
- trace_printk("Conversion ready: %d\n", !!alert);
+ alert &= INA266_CVRF;
+ trace_printk("Conversion ready: %d\n", !!alert);

- } while (!alert);
+ } while (!alert);

/*
* Single register reads: bulk_read will not work with ina226
@@ -446,7 +475,8 @@ static int ina2xx_capture_thread(void *data)
* Poll a bit faster than the chip internal Fs, in case
* we wish to sync with the conversion ready flag.
*/
- sampling_us -= 200;
+ if (!chip->allow_async_readout)
+ sampling_us -= 200;

do {
buffer_us = ina2xx_work_buffer(indio_dev);
@@ -471,6 +501,7 @@ int ina2xx_buffer_enable(struct iio_dev *indio_dev)
1000000/sampling_us, chip->avg);

trace_printk("Expected work period: %u us\n", sampling_us);
+ trace_printk("Async readout mode: %d\n", chip->allow_async_readout);

chip->prev_ns = iio_get_time_ns();

@@ -512,7 +543,12 @@ static int ina2xx_debug_reg(struct iio_dev *indio_dev,
static IIO_CONST_ATTR_INT_TIME_AVAIL \
("0.000140 0.000204 0.000332 0.000588 0.001100 0.002116 0.004156 0.008244");

+static IIO_DEVICE_ATTR(in_allow_async_readout, S_IRUGO | S_IWUSR,
+ ina2xx_allow_async_readout_show,
+ ina2xx_allow_async_readout_store, 0);
+
static struct attribute *ina2xx_attributes[] = {
+ &iio_dev_attr_in_allow_async_readout.dev_attr.attr,
&iio_const_attr_integration_time_available.dev_attr.attr,
NULL,
};
--
1.9.1

2015-12-12 15:56:19

by Jonathan Cameron

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

On 07/12/15 09:09, Marc Titinger wrote:
> This can lead to repeated or skipped samples depending on the clock beat
> between the capture thread and the chip sampling clock, but will also spare
> reading/waiting for the Capture Ready Flag and improve the available i2c
> bandwidth for reading measurements.
>
> Output of iio_info:
> ...snip...
> 4 device-specific attributes found:
> attr 0: in_oversampling_ratio value: 4
> attr 1: in_allow_async_readout value: 0
> attr 2: integration_time_available value: 140 204 332 588 1100 2116...
> attr 3: in_sampling_frequency value: 114
>
> Signed-off-by: Marc Titinger <[email protected]>
Applied to the togreg branch of iio.git - initially pushed out as testing for the autobuilders
to play with it.

Could you prepare a follow up patch adding abi docs to
Documentation/ABI/testing/sysfs-bus-iio-ina2xx-adc
please.

I didn't want to hold this up by insisting on docs now, but it does need
doing asap.

Jonathan
> ---
> drivers/iio/adc/ina2xx-adc.c | 54 ++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 45 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index 0e7c474..615c203 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -111,6 +111,7 @@ struct ina2xx_chip_info {
> s64 prev_ns; /* track buffer capture time, check for underruns*/
> int int_time_vbus; /* Bus voltage integration time uS */
> int int_time_vshunt; /* Shunt voltage integration time uS */
> + bool allow_async_readout;
> };
>
> static const struct ina2xx_config ina2xx_config[] = {
> @@ -326,6 +327,33 @@ _err:
> }
>
>
> +static ssize_t ina2xx_allow_async_readout_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
> +
> + return sprintf(buf, "%d\n", chip->allow_async_readout);
> +}
> +
> +static ssize_t ina2xx_allow_async_readout_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev));
> + bool val;
> + int ret;
> +
> + ret = strtobool((const char *) buf, &val);
> + if (ret)
> + return ret;
> +
> + chip->allow_async_readout = val;
> +
> + return len;
> +}
> +
> +
> #define INA2XX_CHAN(_type, _index, _address) { \
> .type = (_type), \
> .address = (_address), \
> @@ -392,16 +420,17 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
> * GPIO a triggered buffer could be used instead.
> * For now, we pay for that extra read of the ALERT register
> */
> - do {
> - ret = regmap_read(chip->regmap, INA226_ALERT_MASK,
> - &alert);
> - if (ret < 0)
> - return ret;
> + if (!chip->allow_async_readout)
> + do {
> + ret = regmap_read(chip->regmap, INA226_ALERT_MASK,
> + &alert);
> + if (ret < 0)
> + return ret;
>
> - alert &= INA266_CVRF;
> - trace_printk("Conversion ready: %d\n", !!alert);
> + alert &= INA266_CVRF;
> + trace_printk("Conversion ready: %d\n", !!alert);
>
> - } while (!alert);
> + } while (!alert);
>
> /*
> * Single register reads: bulk_read will not work with ina226
> @@ -446,7 +475,8 @@ static int ina2xx_capture_thread(void *data)
> * Poll a bit faster than the chip internal Fs, in case
> * we wish to sync with the conversion ready flag.
> */
> - sampling_us -= 200;
> + if (!chip->allow_async_readout)
> + sampling_us -= 200;
>
> do {
> buffer_us = ina2xx_work_buffer(indio_dev);
> @@ -471,6 +501,7 @@ int ina2xx_buffer_enable(struct iio_dev *indio_dev)
> 1000000/sampling_us, chip->avg);
>
> trace_printk("Expected work period: %u us\n", sampling_us);
> + trace_printk("Async readout mode: %d\n", chip->allow_async_readout);
>
> chip->prev_ns = iio_get_time_ns();
>
> @@ -512,7 +543,12 @@ static int ina2xx_debug_reg(struct iio_dev *indio_dev,
> static IIO_CONST_ATTR_INT_TIME_AVAIL \
> ("0.000140 0.000204 0.000332 0.000588 0.001100 0.002116 0.004156 0.008244");
>
> +static IIO_DEVICE_ATTR(in_allow_async_readout, S_IRUGO | S_IWUSR,
> + ina2xx_allow_async_readout_show,
> + ina2xx_allow_async_readout_store, 0);
> +
> static struct attribute *ina2xx_attributes[] = {
> + &iio_dev_attr_in_allow_async_readout.dev_attr.attr,
> &iio_const_attr_integration_time_available.dev_attr.attr,
> NULL,
> };
>

2015-12-12 17:15:04

by Jonathan Cameron

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

On 07/12/15 09:09, Marc Titinger wrote:
> in SOFTWARE buffer mode, a kthread will capture the active scan_elements
> into a kfifo, then compute the remaining time until the next capture tick
> and do an active wait (udelay).
>
> This will produce a stream of up to fours channels plus a 64bits
> timestamps (ns).
>
> Tested with ina226, on BeagleBoneBlack.
>
> Datasheet: http://www.ti.com/lit/gpn/ina226
>
> Signed-off-by: Marc Titinger <[email protected]>
One small (largely semantic) issue with balanced post enable / pre disable
(rather than post disable) for the buffer.

Otherwise fine so I'll apply this and fix that up in the process.

Applied to the togreg branch of iio.git - pushed out as testing.
Was a bit of additional fuzz but I think the merge was straightforward.

Long term - might move this out to a meter directory along with the
various power monitors that are in staging/iio/meter.

Thanks,

Jonathan
> ---
> drivers/iio/adc/Kconfig | 10 +
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/ina2xx-adc.c | 670 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 681 insertions(+)
> create mode 100644 drivers/iio/adc/ina2xx-adc.c
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index eb0cd89..e5fdc7d 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -170,6 +170,16 @@ config EXYNOS_ADC
> of SoCs for drivers such as the touchscreen and hwmon to use to share
> this resource.
>
> +config INA2XX_ADC
> + tristate "Texas Instruments INA2xx Power Monitors IIO driver"
> + depends on I2C && !SENSORS_INA2XX
> + select REGMAP_I2C
> + select IIO_BUFFER
> + select IIO_KFIFO_BUF
> + help
> + Say yes here to build support for TI INA2xx family of Power Monitors.
> + This driver is mutually exclusive with the HWMON version.
> +
> 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..2869b0ef 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_ADC) += ina2xx-adc.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-adc.c b/drivers/iio/adc/ina2xx-adc.c
> new file mode 100644
> index 0000000..0e7c474
> --- /dev/null
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -0,0 +1,670 @@
> +/*
> + * 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 <[email protected]>
> + *
> + * Licensed under the GPL-2 or later.
> + *
> + * IIO driver for INA219-220-226-230-231
> + *
> + * Configurable 7-bit I2C slave address from 0x40 to 0x4F
> + */
> +#include <linux/module.h>
> +#include <linux/kthread.h>
> +#include <linux/delay.h>
> +#include <linux/iio/kfifo_buf.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/i2c.h>
> +#include <linux/regmap.h>
> +#include <linux/platform_data/ina2xx.h>
> +
> +#include <linux/util_macros.h>
> +
> +/* INA2XX registers definition */
> +#define INA2XX_CONFIG 0x00
> +#define INA2XX_SHUNT_VOLTAGE 0x01 /* readonly */
> +#define INA2XX_BUS_VOLTAGE 0x02 /* readonly */
> +#define INA2XX_POWER 0x03 /* readonly */
> +#define INA2XX_CURRENT 0x04 /* readonly */
> +#define INA2XX_CALIBRATION 0x05
> +
> +#define INA226_ALERT_MASK 0x06
> +#define INA266_CVRF BIT(3)
> +
> +#define INA2XX_MAX_REGISTERS 8
> +
> +/* settings - depend on use case */
> +#define INA219_CONFIG_DEFAULT 0x399F /* PGA=8 */
> +#define INA226_CONFIG_DEFAULT 0x4327
> +#define INA226_DEFAULT_AVG 4
> +#define INA226_DEFAULT_IT 1110
> +
> +#define INA2XX_RSHUNT_DEFAULT 10000
> +
> +/*
> + * bit mask for reading the averaging setting in the configuration register
> + * FIXME: use regmap_fields.
> + */
> +#define INA2XX_MODE_MASK GENMASK(3, 0)
> +
> +#define INA226_AVG_MASK GENMASK(11, 9)
> +#define INA226_SHIFT_AVG(val) ((val) << 9)
> +
> +/* Integration time for VBus */
> +#define INA226_ITB_MASK GENMASK(8, 6)
> +#define INA226_SHIFT_ITB(val) ((val) << 6)
> +
> +/* Integration time for VShunt */
> +#define INA226_ITS_MASK GENMASK(5, 3)
> +#define INA226_SHIFT_ITS(val) ((val) << 3)
> +
> +/* Cosmetic macro giving the sampling period for a full P=UxI cycle */
> +#define SAMPLING_PERIOD(c) ((c->int_time_vbus + c->int_time_vshunt) \
> + * c->avg)
> +
> +static bool ina2xx_is_writeable_reg(struct device *dev, unsigned int reg)
> +{
> + return (reg == INA2XX_CONFIG) || (reg > INA2XX_CURRENT);
> +}
> +
> +static bool ina2xx_is_volatile_reg(struct device *dev, unsigned int reg)
> +{
> + return (reg != INA2XX_CONFIG);
> +}
> +
> +static const struct regmap_config ina2xx_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 16,
> + .max_register = INA2XX_MAX_REGISTERS,
> + .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 shunt_div;
> + int bus_voltage_shift;
> + int bus_voltage_lsb; /* uV */
> + int power_lsb; /* uW */
> +};
> +
> +struct ina2xx_chip_info {
> + struct regmap *regmap;
> + struct task_struct *task;
> + const struct ina2xx_config *config;
> + struct mutex state_lock;
> + long rshunt;
> + int avg;
> + s64 prev_ns; /* track buffer capture time, check for underruns*/
> + int int_time_vbus; /* Bus voltage integration time uS */
> + int int_time_vshunt; /* Shunt voltage integration time uS */
> +};
> +
> +static const struct ina2xx_config ina2xx_config[] = {
> + [ina219] = {
> + .config_default = INA219_CONFIG_DEFAULT,
> + .calibration_factor = 40960000,
> + .shunt_div = 100,
> + .bus_voltage_shift = 3,
> + .bus_voltage_lsb = 4000,
> + .power_lsb = 20000,
> + },
> + [ina226] = {
> + .config_default = INA226_CONFIG_DEFAULT,
> + .calibration_factor = 5120000,
> + .shunt_div = 400,
> + .bus_voltage_shift = 0,
> + .bus_voltage_lsb = 1250,
> + .power_lsb = 25000,
> + },
> +};
> +
> +static int ina2xx_get_value(struct ina2xx_chip_info *chip, u8 reg,
> + unsigned int regval, int *val, int *uval)
> +{
> + *val = 0;
> +
> + switch (reg) {
> + case INA2XX_SHUNT_VOLTAGE:
> + /* signed register */
> + *uval = DIV_ROUND_CLOSEST((s16) regval,
> + chip->config->shunt_div);
> + return IIO_VAL_INT_PLUS_MICRO;
> +
> + case INA2XX_BUS_VOLTAGE:
> + *uval = (regval >> chip->config->bus_voltage_shift)
> + * chip->config->bus_voltage_lsb;
> + *val = *uval / 1000000;
> + *uval = *uval % 1000000;
> + return IIO_VAL_INT_PLUS_MICRO;
> +
> + case INA2XX_POWER:
> + *uval = regval * chip->config->power_lsb;
> + *val = *uval / 1000000;
> + *uval = *uval % 1000000;
> + return IIO_VAL_INT_PLUS_MICRO;
> +
> + case INA2XX_CURRENT:
> + /* signed register, LSB=1mA (selected), in mA */
> + *uval = (s16) regval * 1000;
> + return IIO_VAL_INT_PLUS_MICRO;
> +
> + default:
> + /* programmer goofed */
> + WARN_ON_ONCE(1);
> + }
> + return -EINVAL;
> +}
> +
> +static int ina2xx_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + int ret;
> + struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> + unsigned int regval;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + ret = regmap_read(chip->regmap, chan->address, &regval);
> + if (ret < 0)
> + return ret;
> +
> + return ina2xx_get_value(chip, chan->address, regval, val, val2);
> +
> + case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> + *val = chip->avg;
> + return IIO_VAL_INT;
> +
> + case IIO_CHAN_INFO_INT_TIME:
> + *val = 0;
> + if (chan->address == INA2XX_SHUNT_VOLTAGE)
> + *val2 = chip->int_time_vshunt;
> + else
> + *val2 = chip->int_time_vbus;
> +
> + return IIO_VAL_INT_PLUS_MICRO;
> +
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + /*
> + * Sample freq is read only, it is a consequence of
> + * 1/AVG*(CT_bus+CT_shunt).
> + */
> + *val = DIV_ROUND_CLOSEST(1000000, SAMPLING_PERIOD(chip));
> +
> + return IIO_VAL_INT;
> +
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * Available averaging rates for ina226. The indices correspond with
> + * the bit values expected by the chip (according to the ina226 datasheet,
> + * table 3 AVG bit settings, found at
> + * http://www.ti.com/lit/ds/symlink/ina226.pdf.
> + */
> +static const int ina226_avg_tab[] = { 1, 4, 16, 64, 128, 256, 512, 1024 };
> +
> +static int ina226_set_average(struct ina2xx_chip_info *chip, unsigned int val,
> + unsigned int *config)
> +{
> + int bits;
> +
> + if (val > 1024 || val < 1)
> + return -EINVAL;
> +
> + bits = find_closest(val, ina226_avg_tab,
> + ARRAY_SIZE(ina226_avg_tab));
> +
> + chip->avg = ina226_avg_tab[bits];
> +
> + *config &= ~INA226_AVG_MASK;
> + *config |= INA226_SHIFT_AVG(bits) & INA226_AVG_MASK;
> +
> + return 0;
> +}
> +
> +/* Conversion times in uS */
> +static const int ina226_conv_time_tab[] = { 140, 204, 332, 588, 1100,
> + 2116, 4156, 8244 };
> +
> +static int ina226_set_int_time_vbus(struct ina2xx_chip_info *chip,
> + unsigned int val_us, unsigned int *config)
> +{
> + int bits;
> +
> + if (val_us > 8244 || val_us < 140)
> + return -EINVAL;
> +
> + bits = find_closest(val_us, ina226_conv_time_tab,
> + ARRAY_SIZE(ina226_conv_time_tab));
> +
> + chip->int_time_vbus = ina226_conv_time_tab[bits];
> +
> + *config &= ~INA226_ITB_MASK;
> + *config |= INA226_SHIFT_ITB(bits) & INA226_ITB_MASK;
> +
> + return 0;
> +}
> +
> +static int ina226_set_int_time_vshunt(struct ina2xx_chip_info *chip,
> + unsigned int val_us, unsigned int *config)
> +{
> + int bits;
> +
> + if (val_us > 8244 || val_us < 140)
> + return -EINVAL;
> +
> + bits = find_closest(val_us, ina226_conv_time_tab,
> + ARRAY_SIZE(ina226_conv_time_tab));
> +
> + chip->int_time_vshunt = ina226_conv_time_tab[bits];
> +
> + *config &= ~INA226_ITS_MASK;
> + *config |= INA226_SHIFT_ITS(bits) & INA226_ITS_MASK;
> +
> + return 0;
> +}
> +
> +static int ina2xx_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> + int ret;
> + unsigned int config, tmp;
> +
> + if (iio_buffer_enabled(indio_dev))
> + return -EBUSY;
> +
> + mutex_lock(&chip->state_lock);
> +
> + ret = regmap_read(chip->regmap, INA2XX_CONFIG, &config);
> + if (ret < 0)
> + goto _err;
> +
> + tmp = config;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> + ret = ina226_set_average(chip, val, &tmp);
> + break;
> +
> + case IIO_CHAN_INFO_INT_TIME:
> + if (chan->address == INA2XX_SHUNT_VOLTAGE)
> + ret = ina226_set_int_time_vshunt(chip, val2, &tmp);
> + else
> + ret = ina226_set_int_time_vbus(chip, val2, &tmp);
> + break;
> + default:
> + ret = -EINVAL;
> + }
> +
> + if (!ret && (tmp != config))
> + ret = regmap_write(chip->regmap, INA2XX_CONFIG, tmp);
> +_err:
> + mutex_unlock(&chip->state_lock);
> +
> + return ret;
> +}
> +
> +
> +#define INA2XX_CHAN(_type, _index, _address) { \
> + .type = (_type), \
> + .address = (_address), \
> + .indexed = 1, \
> + .channel = (_index), \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
> + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
> + .scan_index = (_index), \
> + .scan_type = { \
> + .sign = 'u', \
> + .realbits = 16, \
> + .storagebits = 16, \
> + .endianness = IIO_BE, \
> + } \
> +}
> +
> +/*
> + * Sampling Freq is a consequence of the integration times of
> + * the Voltage channels.
> + */
> +#define INA2XX_CHAN_VOLTAGE(_index, _address) { \
> + .type = IIO_VOLTAGE, \
> + .address = (_address), \
> + .indexed = 1, \
> + .channel = (_index), \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_INT_TIME), \
> + .scan_index = (_index), \
> + .scan_type = { \
> + .sign = 'u', \
> + .realbits = 16, \
> + .storagebits = 16, \
> + .endianness = IIO_BE, \
> + } \
> +}
> +
> +static const struct iio_chan_spec ina2xx_channels[] = {
> + INA2XX_CHAN_VOLTAGE(0, INA2XX_SHUNT_VOLTAGE),
> + INA2XX_CHAN_VOLTAGE(1, INA2XX_BUS_VOLTAGE),
> + INA2XX_CHAN(IIO_CURRENT, 2, INA2XX_CURRENT),
> + INA2XX_CHAN(IIO_POWER, 3, INA2XX_POWER),
> + IIO_CHAN_SOFT_TIMESTAMP(4),
> +};
> +
> +static int ina2xx_work_buffer(struct iio_dev *indio_dev)
> +{
> + struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> + unsigned short data[8];
> + int bit, ret, i = 0;
> + unsigned long buffer_us, elapsed_us;
> + s64 time_a, time_b;
> + unsigned int alert;
> +
> + time_a = iio_get_time_ns();
> +
> + /*
> + * Because the timer thread and the chip conversion clock
> + * are asynchronous, the period difference will eventually
> + * result in reading V[k-1] again, or skip V[k] at time Tk.
> + * In order to resync the timer with the conversion process
> + * we check the ConVersionReadyFlag.
> + * On hardware that supports using the ALERT pin to toggle a
> + * GPIO a triggered buffer could be used instead.
> + * For now, we pay for that extra read of the ALERT register
> + */
> + do {
> + ret = regmap_read(chip->regmap, INA226_ALERT_MASK,
> + &alert);
> + if (ret < 0)
> + return ret;
> +
> + alert &= INA266_CVRF;
> + trace_printk("Conversion ready: %d\n", !!alert);
> +
> + } while (!alert);
> +
> + /*
> + * Single register reads: bulk_read will not work with ina226
> + * as there is no auto-increment of the address register for
> + * data length longer than 16bits.
> + */
> + for_each_set_bit(bit, indio_dev->active_scan_mask,
> + indio_dev->masklength) {
> + unsigned int val;
> +
> + ret = regmap_read(chip->regmap,
> + INA2XX_SHUNT_VOLTAGE + bit, &val);
> + if (ret < 0)
> + return ret;
> +
> + data[i++] = val;
> + }
> +
> + time_b = iio_get_time_ns();
> +
> + iio_push_to_buffers_with_timestamp(indio_dev,
> + (unsigned int *)data, time_a);
> +
> + buffer_us = (unsigned long)(time_b - time_a) / 1000;
> + elapsed_us = (unsigned long)(time_a - chip->prev_ns) / 1000;
> +
> + trace_printk("uS: elapsed: %lu, buf: %lu\n", elapsed_us, buffer_us);
> +
> + chip->prev_ns = time_a;
> +
> + return buffer_us;
> +};
> +
> +static int ina2xx_capture_thread(void *data)
> +{
> + struct iio_dev *indio_dev = (struct iio_dev *)data;
> + struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> + unsigned int sampling_us = SAMPLING_PERIOD(chip);
> + int buffer_us;
> +
> + /*
> + * Poll a bit faster than the chip internal Fs, in case
> + * we wish to sync with the conversion ready flag.
> + */
> + sampling_us -= 200;
> +
> + do {
> + buffer_us = ina2xx_work_buffer(indio_dev);
> + if (buffer_us < 0)
> + return buffer_us;
> +
> + if (sampling_us > buffer_us)
> + udelay(sampling_us - buffer_us);
> +
> + } while (!kthread_should_stop());
> +
> + return 0;
> +}
> +
> +int ina2xx_buffer_enable(struct iio_dev *indio_dev)
> +{
> + struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> + unsigned int sampling_us = SAMPLING_PERIOD(chip);
> +
> + trace_printk("Enabling buffer w/ scan_mask %02x, freq = %d, avg =%u\n",
> + (unsigned int)(*indio_dev->active_scan_mask),
> + 1000000/sampling_us, chip->avg);
> +
> + trace_printk("Expected work period: %u us\n", sampling_us);
> +
> + chip->prev_ns = iio_get_time_ns();
> +
> + chip->task = kthread_run(ina2xx_capture_thread, (void *)indio_dev,
> + "ina2xx-%uus", sampling_us);
> +
> + return PTR_ERR_OR_ZERO(chip->task);
> +}
> +
> +int ina2xx_buffer_disable(struct iio_dev *indio_dev)
> +{
> + struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> +
> + if (chip->task) {
> + kthread_stop(chip->task);
> + chip->task = NULL;
> + }
> +
> + return 0;
> +}
> +
> +static const struct iio_buffer_setup_ops ina2xx_setup_ops = {
> + .postenable = &ina2xx_buffer_enable,
> + .postdisable = &ina2xx_buffer_disable,
Sorry, only just noticed this. The postenable pairs with predisable
(inside and outside the state change for the two pairs). I might just
fix this up when applying (should be predisable)

> +};
> +
> +static int ina2xx_debug_reg(struct iio_dev *indio_dev,
> + unsigned reg, unsigned writeval, unsigned *readval)
> +{
> + struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> +
> + if (!readval)
> + return regmap_write(chip->regmap, reg, writeval);
> +
> + return regmap_read(chip->regmap, reg, readval);
> +}
> +
> +/* Possible integration times for vshunt and vbus */
> +static IIO_CONST_ATTR_INT_TIME_AVAIL \
> + ("0.000140 0.000204 0.000332 0.000588 0.001100 0.002116 0.004156 0.008244");
> +
> +static struct attribute *ina2xx_attributes[] = {
> + &iio_const_attr_integration_time_available.dev_attr.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group ina2xx_attribute_group = {
> + .attrs = ina2xx_attributes,
> +};
> +
> +static const struct iio_info ina2xx_info = {
> + .debugfs_reg_access = &ina2xx_debug_reg,
> + .read_raw = &ina2xx_read_raw,
> + .write_raw = &ina2xx_write_raw,
> + .attrs = &ina2xx_attribute_group,
> + .driver_module = THIS_MODULE,
> +};
> +
> +/* Initialize the configuration and calibration registers. */
> +static int ina2xx_init(struct ina2xx_chip_info *chip, unsigned int config)
> +{
> + u16 regval;
> + int ret = regmap_write(chip->regmap, INA2XX_CONFIG, config);
> +
> + if (ret < 0)
> + return ret;
> + /*
> + * Set current LSB to 1mA, shunt is in uOhms
> + * (equation 13 in datasheet). We hardcode a Current_LSB
> + * of 1.0 x10-6. The only remaining parameter is RShunt.
> + * There is no need to expose the CALIBRATION register
> + * to the user for now.
> + */
> + regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor,
> + chip->rshunt);
> +
> + return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval);
> +}
> +
> +static int ina2xx_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct ina2xx_chip_info *chip;
> + struct iio_dev *indio_dev;
> + struct iio_buffer *buffer;
> + int ret;
> + unsigned int val;
> +
> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + chip = iio_priv(indio_dev);
> +
> + chip->config = &ina2xx_config[id->driver_data];
> +
> + if (of_property_read_u32(client->dev.of_node,
> + "shunt-resistor", &val) < 0) {
> + struct ina2xx_platform_data *pdata =
> + dev_get_platdata(&client->dev);
> +
> + if (pdata)
> + val = pdata->shunt_uohms;
> + else
> + val = INA2XX_RSHUNT_DEFAULT;
> + }
> +
> + if (val <= 0 || val > chip->config->calibration_factor)
> + return -ENODEV;
> +
> + chip->rshunt = val;
> +
> + mutex_init(&chip->state_lock);
> +
> + /* This is only used for device removal purposes. */
> + i2c_set_clientdata(client, indio_dev);
> +
> + indio_dev->name = id->name;
> + indio_dev->channels = ina2xx_channels;
> + indio_dev->num_channels = ARRAY_SIZE(ina2xx_channels);
> +
> + indio_dev->dev.parent = &client->dev;
> + indio_dev->info = &ina2xx_info;
> + indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
> +
> + chip->regmap = devm_regmap_init_i2c(client, &ina2xx_regmap_config);
> + if (IS_ERR(chip->regmap)) {
> + dev_err(&client->dev, "failed to allocate register map\n");
> + return PTR_ERR(chip->regmap);
> + }
> +
> + /* Patch the current config register with default. */
> + val = chip->config->config_default;
> +
> + if (id->driver_data == ina226) {
> + ina226_set_average(chip, INA226_DEFAULT_AVG, &val);
> + ina226_set_int_time_vbus(chip, INA226_DEFAULT_IT, &val);
> + ina226_set_int_time_vshunt(chip, INA226_DEFAULT_IT, &val);
> + }
> +
> + ret = ina2xx_init(chip, val);
> + if (ret < 0) {
> + dev_err(&client->dev, "error configuring the device: %d\n",
> + ret);
> + return -ENODEV;
> + }
> +
> + buffer = devm_iio_kfifo_allocate(&indio_dev->dev);
> + if (!buffer)
> + return -ENOMEM;
> +
> + indio_dev->setup_ops = &ina2xx_setup_ops;
> +
> + iio_device_attach_buffer(indio_dev, buffer);
> +
> + return iio_device_register(indio_dev);
> +}
> +
> +
> +static int ina2xx_remove(struct i2c_client *client)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> + struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> +
> + iio_device_unregister(indio_dev);
> +
> + /* Powerdown */
> + return regmap_update_bits(chip->regmap, INA2XX_CONFIG,
> + INA2XX_MODE_MASK, 0);
> +}
> +
> +
> +static const struct i2c_device_id ina2xx_id[] = {
> + {"ina219", ina219},
> + {"ina220", ina219},
> + {"ina226", ina226},
> + {"ina230", ina226},
> + {"ina231", ina226},
> + {}
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, ina2xx_id);
> +
> +static struct i2c_driver ina2xx_driver = {
> + .driver = {
> + .name = KBUILD_MODNAME,
> + },
> + .probe = ina2xx_probe,
> + .remove = ina2xx_remove,
> + .id_table = ina2xx_id,
> +};
> +
> +module_i2c_driver(ina2xx_driver);
> +
> +MODULE_AUTHOR("Marc Titinger <[email protected]>");
> +MODULE_DESCRIPTION("Texas Instruments INA2XX ADC driver");
> +MODULE_LICENSE("GPL v2");
>

2015-12-14 11:01:25

by Marc Titinger

[permalink] [raw]
Subject: [PATCH] iio: ina2xx: add ABI documentation entry sysfs-bus-iio-ina2xx-adc

Documentation for attributes:

* in_allow_async_readout
* in_shunt_resistor

Signed-off-by: Marc Titinger <[email protected]>
---
Documentation/ABI/testing/sysfs-bus-iio-ina2xx-adc | 24 ++++++++++++++++++++++
1 file changed, 24 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-ina2xx-adc

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-ina2xx-adc b/Documentation/ABI/testing/sysfs-bus-iio-ina2xx-adc
new file mode 100644
index 0000000..8916f7e
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-ina2xx-adc
@@ -0,0 +1,24 @@
+What: /sys/bus/iio/devices/iio:deviceX/in_allow_async_readout
+Date: December 2015
+KernelVersion: 4.4
+Contact: [email protected]
+Description:
+ By default (value '0'), the capture thread checks for the Conversion
+ Ready Flag to being set prior to committing a new value to the sample
+ buffer. This synchronizes the in-chip conversion rate with the
+ in-driver readout rate at the cost of an additional register read.
+
+ Writing '1' will remove the polling for the Conversion Ready Flags to
+ save the additional i2c transaction, which will improve the bandwidth
+ available for reading data. However, samples can be occasionally skipped
+ or repeated, depending on the beat between the capture and conversion
+ rates.
+
+What: /sys/bus/iio/devices/iio:deviceX/in_shunt_resistor
+Date: December 2015
+KernelVersion: 4.4
+Contact: [email protected]
+Description:
+ The value of the shunt resistor may be known only at runtime fom an
+ eeprom content read by a client application. This attribute allows to
+ set its value in ohms.
--
1.9.1

2015-12-15 15:26:30

by Marc Titinger

[permalink] [raw]
Subject: [PATCH] iio: ina2xx: fix channel order in software buffer

POWER and CURRENT were swapped out in the buffer:
was current2 and power3, correct order is power2 and current3.

Signed-off-by: Marc Titinger <[email protected]>

---

Hi Jon,

we just found this while testing with the buffered mode.
My apologies for missing this bug ealier.

Cheers,
Marc.

---

drivers/iio/adc/ina2xx-adc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 98939ba..91e1f2b 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -427,8 +427,8 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
static const struct iio_chan_spec ina2xx_channels[] = {
INA2XX_CHAN_VOLTAGE(0, INA2XX_SHUNT_VOLTAGE),
INA2XX_CHAN_VOLTAGE(1, INA2XX_BUS_VOLTAGE),
- INA2XX_CHAN(IIO_CURRENT, 2, INA2XX_CURRENT),
- INA2XX_CHAN(IIO_POWER, 3, INA2XX_POWER),
+ INA2XX_CHAN(IIO_POWER, 2, INA2XX_POWER),
+ INA2XX_CHAN(IIO_CURRENT, 3, INA2XX_CURRENT),
IIO_CHAN_SOFT_TIMESTAMP(4),
};

--
1.9.1

2015-12-16 17:46:15

by Andrew Davis

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

On 12/07/2015 03:09 AM, Marc Titinger wrote:
> +#define INA2XX_CHAN(_type, _index, _address) { \
> + .type = (_type), \
> + .address = (_address), \
> + .indexed = 1, \
> + .channel = (_index), \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
> + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
> + .scan_index = (_index), \
> + .scan_type = { \
> + .sign = 'u', \
> + .realbits = 16, \
> + .storagebits = 16, \
> + .endianness = IIO_BE, \

^^^^ See below.

> + } \
> +}
> +
> +/*
> + * Sampling Freq is a consequence of the integration times of
> + * the Voltage channels.
> + */
> +#define INA2XX_CHAN_VOLTAGE(_index, _address) { \
> + .type = IIO_VOLTAGE, \
> + .address = (_address), \
> + .indexed = 1, \
> + .channel = (_index), \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_INT_TIME), \
> + .scan_index = (_index), \
> + .scan_type = { \
> + .sign = 'u', \
> + .realbits = 16, \
> + .storagebits = 16, \
> + .endianness = IIO_BE, \
> + } \
> +}
> +
> +static const struct iio_chan_spec ina2xx_channels[] = {
> + INA2XX_CHAN_VOLTAGE(0, INA2XX_SHUNT_VOLTAGE),
> + INA2XX_CHAN_VOLTAGE(1, INA2XX_BUS_VOLTAGE),
> + INA2XX_CHAN(IIO_CURRENT, 2, INA2XX_CURRENT),
> + INA2XX_CHAN(IIO_POWER, 3, INA2XX_POWER),
> + IIO_CHAN_SOFT_TIMESTAMP(4),
> +};
> +
> +static int ina2xx_work_buffer(struct iio_dev *indio_dev)
> +{
> + struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> + unsigned short data[8];
> + int bit, ret, i = 0;
> + unsigned long buffer_us, elapsed_us;
> + s64 time_a, time_b;
> + unsigned int alert;
> +
> + time_a = iio_get_time_ns();
> +
> + /*
> + * Because the timer thread and the chip conversion clock
> + * are asynchronous, the period difference will eventually
> + * result in reading V[k-1] again, or skip V[k] at time Tk.
> + * In order to resync the timer with the conversion process
> + * we check the ConVersionReadyFlag.
> + * On hardware that supports using the ALERT pin to toggle a
> + * GPIO a triggered buffer could be used instead.
> + * For now, we pay for that extra read of the ALERT register
> + */
> + do {
> + ret = regmap_read(chip->regmap, INA226_ALERT_MASK,
> + &alert);
> + if (ret < 0)
> + return ret;
> +
> + alert &= INA266_CVRF;
> + trace_printk("Conversion ready: %d\n", !!alert);
> +
> + } while (!alert);
> +
> + /*
> + * Single register reads: bulk_read will not work with ina226
> + * as there is no auto-increment of the address register for
> + * data length longer than 16bits.
> + */
> + for_each_set_bit(bit, indio_dev->active_scan_mask,
> + indio_dev->masklength) {
> + unsigned int val;
> +
> + ret = regmap_read(chip->regmap,
> + INA2XX_SHUNT_VOLTAGE + bit, &val);
> + if (ret < 0)
> + return ret;
> +
> + data[i++] = val;

The read above seems to fill the buffer in CPU order, but above
IIO_BE is specified, this should probably be IIO_CPU to avoid
confusion for programs that check for IIO buffer endianness.

I'll push this in a fixup patch later. Just wanted to give a
heads up if you run into problems with some program.

2015-12-16 17:54:52

by Marc Titinger

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

On 16/12/2015 18:45, Andrew F. Davis wrote:
> On 12/07/2015 03:09 AM, Marc Titinger wrote:
>> +#define INA2XX_CHAN(_type, _index, _address) { \
>> + .type = (_type), \
>> + .address = (_address), \
>> + .indexed = 1, \
>> + .channel = (_index), \
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
>> + .info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
>> + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
>> + .scan_index = (_index), \
>> + .scan_type = { \
>> + .sign = 'u', \
>> + .realbits = 16, \
>> + .storagebits = 16, \
>> + .endianness = IIO_BE, \
>
> ^^^^ See below.
>
>> + } \
>> +}
>> +
>> +/*
>> + * Sampling Freq is a consequence of the integration times of
>> + * the Voltage channels.
>> + */
>> +#define INA2XX_CHAN_VOLTAGE(_index, _address) { \
>> + .type = IIO_VOLTAGE, \
>> + .address = (_address), \
>> + .indexed = 1, \
>> + .channel = (_index), \
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
>> + BIT(IIO_CHAN_INFO_INT_TIME), \
>> + .scan_index = (_index), \
>> + .scan_type = { \
>> + .sign = 'u', \
>> + .realbits = 16, \
>> + .storagebits = 16, \
>> + .endianness = IIO_BE, \
>> + } \
>> +}
>> +
>> +static const struct iio_chan_spec ina2xx_channels[] = {
>> + INA2XX_CHAN_VOLTAGE(0, INA2XX_SHUNT_VOLTAGE),
>> + INA2XX_CHAN_VOLTAGE(1, INA2XX_BUS_VOLTAGE),
>> + INA2XX_CHAN(IIO_CURRENT, 2, INA2XX_CURRENT),
>> + INA2XX_CHAN(IIO_POWER, 3, INA2XX_POWER),
>> + IIO_CHAN_SOFT_TIMESTAMP(4),
>> +};
>> +
>> +static int ina2xx_work_buffer(struct iio_dev *indio_dev)
>> +{
>> + struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>> + unsigned short data[8];
>> + int bit, ret, i = 0;
>> + unsigned long buffer_us, elapsed_us;
>> + s64 time_a, time_b;
>> + unsigned int alert;
>> +
>> + time_a = iio_get_time_ns();
>> +
>> + /*
>> + * Because the timer thread and the chip conversion clock
>> + * are asynchronous, the period difference will eventually
>> + * result in reading V[k-1] again, or skip V[k] at time Tk.
>> + * In order to resync the timer with the conversion process
>> + * we check the ConVersionReadyFlag.
>> + * On hardware that supports using the ALERT pin to toggle a
>> + * GPIO a triggered buffer could be used instead.
>> + * For now, we pay for that extra read of the ALERT register
>> + */
>> + do {
>> + ret = regmap_read(chip->regmap, INA226_ALERT_MASK,
>> + &alert);
>> + if (ret < 0)
>> + return ret;
>> +
>> + alert &= INA266_CVRF;
>> + trace_printk("Conversion ready: %d\n", !!alert);
>> +
>> + } while (!alert);
>> +
>> + /*
>> + * Single register reads: bulk_read will not work with ina226
>> + * as there is no auto-increment of the address register for
>> + * data length longer than 16bits.
>> + */
>> + for_each_set_bit(bit, indio_dev->active_scan_mask,
>> + indio_dev->masklength) {
>> + unsigned int val;
>> +
>> + ret = regmap_read(chip->regmap,
>> + INA2XX_SHUNT_VOLTAGE + bit, &val);
>> + if (ret < 0)
>> + return ret;
>> +
>> + data[i++] = val;
>
> The read above seems to fill the buffer in CPU order, but above
> IIO_BE is specified, this should probably be IIO_CPU to avoid
> confusion for programs that check for IIO buffer endianness.

Hi Andrew,

Well spotted. I'm sorry it is getting a bit messy because I posted a fix
for this in a separate patch (see https://lkml.org/lkml/2015/12/12/131)

Using IIO_CPU seems an even better option, I did not know about this
value and used IIO_LE.

Thanks!
Marc.




>
> I'll push this in a fixup patch later. Just wanted to give a
> heads up if you run into problems with some program.

2015-12-16 18:11:26

by Andrew Davis

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

On 12/16/2015 11:54 AM, Marc Titinger wrote:
> On 16/12/2015 18:45, Andrew F. Davis wrote:
>> On 12/07/2015 03:09 AM, Marc Titinger wrote:
>>> +#define INA2XX_CHAN(_type, _index, _address) { \
>>> + .type = (_type), \
>>> + .address = (_address), \
>>> + .indexed = 1, \
>>> + .channel = (_index), \
>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
>>> + .info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
>>> + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
>>> + .scan_index = (_index), \
>>> + .scan_type = { \
>>> + .sign = 'u', \
>>> + .realbits = 16, \
>>> + .storagebits = 16, \
>>> + .endianness = IIO_BE, \
>>
>> ^^^^ See below.
>>
>>> + } \
>>> +}
>>> +
>>> +/*
>>> + * Sampling Freq is a consequence of the integration times of
>>> + * the Voltage channels.
>>> + */
>>> +#define INA2XX_CHAN_VOLTAGE(_index, _address) { \
>>> + .type = IIO_VOLTAGE, \
>>> + .address = (_address), \
>>> + .indexed = 1, \
>>> + .channel = (_index), \
>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
>>> + BIT(IIO_CHAN_INFO_INT_TIME), \
>>> + .scan_index = (_index), \
>>> + .scan_type = { \
>>> + .sign = 'u', \
>>> + .realbits = 16, \
>>> + .storagebits = 16, \
>>> + .endianness = IIO_BE, \
>>> + } \
>>> +}
>>> +
>>> +static const struct iio_chan_spec ina2xx_channels[] = {
>>> + INA2XX_CHAN_VOLTAGE(0, INA2XX_SHUNT_VOLTAGE),
>>> + INA2XX_CHAN_VOLTAGE(1, INA2XX_BUS_VOLTAGE),
>>> + INA2XX_CHAN(IIO_CURRENT, 2, INA2XX_CURRENT),
>>> + INA2XX_CHAN(IIO_POWER, 3, INA2XX_POWER),
>>> + IIO_CHAN_SOFT_TIMESTAMP(4),
>>> +};
>>> +
>>> +static int ina2xx_work_buffer(struct iio_dev *indio_dev)
>>> +{
>>> + struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>>> + unsigned short data[8];
>>> + int bit, ret, i = 0;
>>> + unsigned long buffer_us, elapsed_us;
>>> + s64 time_a, time_b;
>>> + unsigned int alert;
>>> +
>>> + time_a = iio_get_time_ns();
>>> +
>>> + /*
>>> + * Because the timer thread and the chip conversion clock
>>> + * are asynchronous, the period difference will eventually
>>> + * result in reading V[k-1] again, or skip V[k] at time Tk.
>>> + * In order to resync the timer with the conversion process
>>> + * we check the ConVersionReadyFlag.
>>> + * On hardware that supports using the ALERT pin to toggle a
>>> + * GPIO a triggered buffer could be used instead.
>>> + * For now, we pay for that extra read of the ALERT register
>>> + */
>>> + do {
>>> + ret = regmap_read(chip->regmap, INA226_ALERT_MASK,
>>> + &alert);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + alert &= INA266_CVRF;
>>> + trace_printk("Conversion ready: %d\n", !!alert);
>>> +
>>> + } while (!alert);
>>> +
>>> + /*
>>> + * Single register reads: bulk_read will not work with ina226
>>> + * as there is no auto-increment of the address register for
>>> + * data length longer than 16bits.
>>> + */
>>> + for_each_set_bit(bit, indio_dev->active_scan_mask,
>>> + indio_dev->masklength) {
>>> + unsigned int val;
>>> +
>>> + ret = regmap_read(chip->regmap,
>>> + INA2XX_SHUNT_VOLTAGE + bit, &val);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + data[i++] = val;
>>
>> The read above seems to fill the buffer in CPU order, but above
>> IIO_BE is specified, this should probably be IIO_CPU to avoid
>> confusion for programs that check for IIO buffer endianness.
>
> Hi Andrew,
>
> Well spotted. I'm sorry it is getting a bit messy because I posted a fix for this in a separate patch (see https://lkml.org/lkml/2015/12/12/131)
>

Ah, wasn't following that.

> Using IIO_CPU seems an even better option, I did not know about this value and used IIO_LE.
>

Which will give incorrect results on BE systems, I'll wait till the dust settles for this
driver and push some more small fixups a bit later.

> Thanks!
> Marc.
>

2015-12-19 15:07:06

by Jonathan Cameron

[permalink] [raw]
Subject: Re: [PATCH] iio: ina2xx: fix channel order in software buffer

On 15/12/15 15:26, Marc Titinger wrote:
> POWER and CURRENT were swapped out in the buffer:
> was current2 and power3, correct order is power2 and current3.
>
> Signed-off-by: Marc Titinger <[email protected]>
>
> ---
>
> Hi Jon,
>
> we just found this while testing with the buffered mode.
> My apologies for missing this bug ealier.
>
> Cheers,
> Marc.
>
> ---
>
Applied this one to the togreg branch of iio.git which will get
initially pushed out as testing for the autobuilders to play with it.

> drivers/iio/adc/ina2xx-adc.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index 98939ba..91e1f2b 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -427,8 +427,8 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
> static const struct iio_chan_spec ina2xx_channels[] = {
> INA2XX_CHAN_VOLTAGE(0, INA2XX_SHUNT_VOLTAGE),
> INA2XX_CHAN_VOLTAGE(1, INA2XX_BUS_VOLTAGE),
> - INA2XX_CHAN(IIO_CURRENT, 2, INA2XX_CURRENT),
> - INA2XX_CHAN(IIO_POWER, 3, INA2XX_POWER),
> + INA2XX_CHAN(IIO_POWER, 2, INA2XX_POWER),
> + INA2XX_CHAN(IIO_CURRENT, 3, INA2XX_CURRENT),
> IIO_CHAN_SOFT_TIMESTAMP(4),
> };
>
>