* Re: [lm-sensors] [PATCH 1/2] iio: ina2xx: add support for TI INA2xx Power Monitors [not found] ` <1448450896-24387-2-git-send-email-mtitinger@baylibre.com> @ 2015-11-29 16:31 ` Jonathan Cameron 2015-11-29 17:38 ` Guenter Roeck 0 siblings, 1 reply; 5+ messages in thread From: Jonathan Cameron @ 2015-11-29 16:31 UTC (permalink / raw) To: Marc Titinger, knaack.h, lars, pmeerw, linux, jdelvare Cc: linux-iio, linux-kernel, lm-sensors On 25/11/15 11:28, Marc Titinger wrote: > in SOFTWARE buffer mode, a kthread will capture the active scan_elements > into a kfifo, then compute the remaining time until the next capture tick > and do an active wait (udelay). > > This will produce a stream of up to fours channels plus a 64bits > timestamps (ns). > > Tested with ina226, on BeagleBoneBlack. > > Datasheet: http://www.ti.com/lit/gpn/ina226 > > Signed-off-by: Marc Titinger <mtitinger@baylibre.com> A few more bits from me, but basically looking good. We do however, need to make the hwmon guys aware this is going on. Please cc their list on the next version. Last thing we want is for this to turn up as a surprise! Jonathan > --- > drivers/iio/adc/Kconfig | 10 + > drivers/iio/adc/Makefile | 1 + > drivers/iio/adc/ina2xx-iio.c | 684 +++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 695 insertions(+) > create mode 100644 drivers/iio/adc/ina2xx-iio.c > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > index eb0cd89..9b87372 100644 > --- a/drivers/iio/adc/Kconfig > +++ b/drivers/iio/adc/Kconfig > @@ -170,6 +170,16 @@ config EXYNOS_ADC > of SoCs for drivers such as the touchscreen and hwmon to use to share > this resource. > > +config INA2XX_IIO > + tristate "Texas Instruments INA2xx Power Monitors IIO driver" > + depends on I2C > + select REGMAP_I2C > + select IIO_BUFFER > + help > + Say yes here to build support for TI INA2xx familly Power Monitors. > + > + Note that this is not the existing hwmon interface for this device. > + > config LP8788_ADC > tristate "LP8788 ADC driver" > depends on MFD_LP8788 > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile > index a096210..74e4341 100644 > --- a/drivers/iio/adc/Makefile > +++ b/drivers/iio/adc/Makefile > @@ -19,6 +19,7 @@ obj-$(CONFIG_BERLIN2_ADC) += berlin2-adc.o > obj-$(CONFIG_DA9150_GPADC) += da9150-gpadc.o > obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o > obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o > +obj-$(CONFIG_INA2XX_IIO) += ina2xx-iio.o > obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o > obj-$(CONFIG_MAX1027) += max1027.o > obj-$(CONFIG_MAX1363) += max1363.o > diff --git a/drivers/iio/adc/ina2xx-iio.c b/drivers/iio/adc/ina2xx-iio.c > new file mode 100644 > index 0000000..4a0026c > --- /dev/null > +++ b/drivers/iio/adc/ina2xx-iio.c > @@ -0,0 +1,684 @@ > +/* > + * INA2XX Current and Power Monitors > + * > + * Copyright 2015 Baylibre SAS. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * Based on linux/drivers/iio/adc/ad7291.c > + * Copyright 2010-2011 Analog Devices Inc. > + * > + * Based on linux/drivers/hwmon/ina2xx.c > + * Copyright 2012 Lothar Felten <l-felten@ti.com> > + * > + * Licensed under the GPL-2 or later. > + * > + * IIO driver for INA219-220-226-230-231 > + * > + * Configurable 7-bit I2C slave address from 0x40 to 0x4F > + */ > +#include <linux/module.h> > +#include <linux/kthread.h> > +#include <linux/delay.h> > +#include <linux/iio/kfifo_buf.h> > +#include <linux/iio/sysfs.h> > +#include <linux/i2c.h> > +#include <linux/regmap.h> > +#include <linux/platform_data/ina2xx.h> > + > +#include <linux/util_macros.h> > + > +/* > + * INA2XX registers definition > + */ > +/* common register definitions */ > +#define INA2XX_CONFIG 0x00 > +#define INA2XX_SHUNT_VOLTAGE 0x01 /* readonly */ > +#define INA2XX_BUS_VOLTAGE 0x02 /* readonly */ > +#define INA2XX_POWER 0x03 /* readonly */ > +#define INA2XX_CURRENT 0x04 /* readonly */ > +#define INA2XX_CALIBRATION 0x05 > + > +#define INA226_ALERT_MASK 0x06 > +#define INA266_CVRF BIT(3) > + > +/* register count */ > +#define INA219_REGISTERS 6 > +#define INA226_REGISTERS 8 > +#define INA2XX_MAX_REGISTERS 8 > + > +/* settings - depend on use case */ > +#define INA219_CONFIG_DEFAULT 0x399F /* PGA=8 */ > +#define INA226_CONFIG_DEFAULT 0x4327 > +#define INA226_DEFAULT_AVG 4 > +#define INA226_DEFAULT_IT 1110 > + > +#define INA2XX_RSHUNT_DEFAULT 10000 > + > +/* > + * bit mask for reading the averaging setting in the configuration register > + * FIXME: use regmap_fields. > + */ > +#define INA2XX_MODE_MASK GENMASK(3, 0) > + > +#define INA226_AVG_MASK GENMASK(11, 9) > +#define INA226_SHIFT_AVG(val) ((val) << 9) > + > +/* Integration time for VBus */ > +#define INA226_ITB_MASK GENMASK(8, 6) > +#define INA226_SHIFT_ITB(val) ((val) << 6) > + > +/*Integration Time for VShunt */ > +#define INA226_ITS_MASK GENMASK(5, 3) > +#define INA226_SHIFT_ITS(val) ((val) << 3) > + > + > +static bool ina2xx_is_writeable_reg(struct device *dev, unsigned int reg) > +{ > + return (reg = INA2XX_CONFIG) || (reg > INA2XX_CURRENT); > +} > + > +static bool ina2xx_is_volatile_reg(struct device *dev, unsigned int reg) > +{ > + return (reg != INA2XX_CONFIG); > +} > + > +static struct regmap_config ina2xx_regmap_config = { > + .reg_bits = 8, > + .val_bits = 16, > + > + .writeable_reg = ina2xx_is_writeable_reg, > + .volatile_reg = ina2xx_is_volatile_reg, > +}; > + > +enum ina2xx_ids { ina219, ina226 }; > + > +struct ina2xx_config { > + u16 config_default; > + int calibration_factor; > + int registers; > + int shunt_div; > + int bus_voltage_shift; > + int bus_voltage_lsb; /* uV */ > + int power_lsb; /* uW */ > +}; > + > +struct ina2xx_chip_info { > + struct regmap *regmap; > + struct task_struct *task; > + const struct ina2xx_config *config; > + struct mutex state_lock; > + long rshunt; > + int avg; > + int itb; /* Bus voltage integration time uS */ > + int its; /* Shunt voltage integration tim uS */ > +}; > + > +static const struct ina2xx_config ina2xx_config[] = { > + [ina219] = { > + .config_default = INA219_CONFIG_DEFAULT, > + .calibration_factor = 40960000, > + .registers = INA219_REGISTERS, > + .shunt_div = 100, > + .bus_voltage_shift = 3, > + .bus_voltage_lsb = 4000, > + .power_lsb = 20000, > + }, > + [ina226] = { > + .config_default = INA226_CONFIG_DEFAULT, > + .calibration_factor = 5120000, > + .registers = INA226_REGISTERS, > + .shunt_div = 400, > + .bus_voltage_shift = 0, > + .bus_voltage_lsb = 1250, > + .power_lsb = 25000, > + }, > +}; > + > +static int ina2xx_get_value(struct ina2xx_chip_info *chip, u8 reg, > + unsigned int regval, int *val, int *uval) > +{ > + *val = 0; > + > + switch (reg) { > + case INA2XX_SHUNT_VOLTAGE: > + /* signed register */ > + *uval = DIV_ROUND_CLOSEST((s16) regval, > + chip->config->shunt_div); > + return IIO_VAL_INT_PLUS_MICRO; > + > + case INA2XX_BUS_VOLTAGE: > + *uval = (regval >> chip->config->bus_voltage_shift) > + * chip->config->bus_voltage_lsb; > + *val = *uval/1000000; > + *uval = *uval % 1000000; > + return IIO_VAL_INT_PLUS_MICRO; > + > + case INA2XX_POWER: > + *uval = regval * chip->config->power_lsb; > + *val = *uval/1000000; > + *uval = *uval % 1000000; > + return IIO_VAL_INT_PLUS_MICRO; > + > + case INA2XX_CURRENT: > + /* signed register, LSB=1mA (selected), in mA */ > + *uval = (s16) regval * 1000; > + return IIO_VAL_INT_PLUS_MICRO; > + > + default: > + /* programmer goofed */ > + WARN_ON_ONCE(1); > + } > + return -EINVAL; > +} > + > +static int ina2xx_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, int *val2, long mask) > +{ > + int ret; > + struct ina2xx_chip_info *chip = iio_priv(indio_dev); > + unsigned int regval; > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + ret = regmap_read(chip->regmap, chan->address, ®val); > + if (ret < 0) > + return ret; > + > + return ina2xx_get_value(chip, chan->address, regval, val, val2); > + > + case IIO_CHAN_INFO_OVERSAMPLING_RATIO: > + *val = chip->avg; > + return IIO_VAL_INT; > + > + case IIO_CHAN_INFO_INT_TIME: > + *val = 0; > + if (chan->address = INA2XX_SHUNT_VOLTAGE) > + *val2 = chip->its; > + else > + *val2 = chip->itb; > + > + return IIO_VAL_INT_PLUS_MICRO; > + /* > + * sample freq is read only, it is a consequence of > + * 1/AVG*(CT_bus+CT_shunt) > + */ > + case IIO_CHAN_INFO_SAMP_FREQ: > + *val = DIV_ROUND_CLOSEST(1000000, > + (chip->itb + chip->its) * chip->avg); > + > + return IIO_VAL_INT; > + > + default: > + return -EINVAL; > + } > + > + return 0; > +} > + > +/* > + * Available averaging rates for ina226. The indices correspond with > + * the bit values expected by the chip (according to the ina226 datasheet, > + * table 3 AVG bit settings, found at > + * http://www.ti.com/lit/ds/symlink/ina226.pdf. > + */ > +static const int ina226_avg_tab[] = { 1, 4, 16, 64, 128, 256, 512, 1024 }; > + > +static unsigned int ina226_set_average(struct ina2xx_chip_info *chip, > + unsigned int val, > + unsigned int *config) > +{ > + int bits; > + > + if (val > 1024 || val < 1) > + return -EINVAL; > + > + bits = find_closest(val, ina226_avg_tab, > + ARRAY_SIZE(ina226_avg_tab)); > + > + chip->avg = ina226_avg_tab[bits]; > + > + *config &= ~INA226_AVG_MASK; > + *config |= INA226_SHIFT_AVG(bits) & INA226_AVG_MASK; > + > + return 0; > +} > + > +/* Conversion times in uS */ > +static const int ina226_conv_time_tab[] = { 140, 204, 332, 588, 1100, > + 2116, 4156, 8244}; > + > +static unsigned int ina226_set_itb(struct ina2xx_chip_info *chip, > + unsigned int val, > + unsigned int *config) > +{ > + int bits; > + > + if (val > 8244 || val < 140) > + return -EINVAL; > + > + bits = find_closest(val, ina226_conv_time_tab, > + ARRAY_SIZE(ina226_conv_time_tab)); > + > + chip->itb = ina226_conv_time_tab[bits]; > + > + *config &= ~INA226_ITB_MASK; > + *config |= INA226_SHIFT_ITB(bits) & INA226_ITB_MASK; > + > + return 0; > +} > + Could do with a slightly more informative name than its vs itb. Or at least some docs explaining what it does. > +static unsigned int ina226_set_its(struct ina2xx_chip_info *chip, > + unsigned int val, > + unsigned int *config) > +{ > + int bits; > + > + if (val > 8244 || val < 140) > + return -EINVAL; > + > + bits = find_closest(val, ina226_conv_time_tab, > + ARRAY_SIZE(ina226_conv_time_tab)); > + > + chip->its = ina226_conv_time_tab[bits]; > + > + *config &= ~INA226_ITS_MASK; > + *config |= INA226_SHIFT_ITS(bits) & INA226_ITS_MASK; > + > + return 0; > +} > + > + > +static int ina2xx_write_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int val, int val2, long mask) > +{ > + struct ina2xx_chip_info *chip = iio_priv(indio_dev); > + int ret = 0; > + unsigned int config, tmp; > + > + if (iio_buffer_enabled(indio_dev)) > + return -EBUSY; > + > + mutex_lock(&chip->state_lock); > + > + ret = regmap_read(chip->regmap, INA2XX_CONFIG, &config); > + if (ret < 0) > + goto _err; > + > + tmp = config; > + > + switch (mask) { > + case IIO_CHAN_INFO_OVERSAMPLING_RATIO: > + ret = ina226_set_average(chip, val, &tmp); > + break; > + > + case IIO_CHAN_INFO_INT_TIME: > + if (chan->address = INA2XX_SHUNT_VOLTAGE) > + ret = ina226_set_its(chip, val, &tmp); > + else > + ret = ina226_set_itb(chip, val, &tmp); > + break; > + default: > + ret = -EINVAL; > + } > + > + if (!ret && (tmp != config)) > + ret = regmap_write(chip->regmap, INA2XX_CONFIG, tmp); > +_err: > + mutex_unlock(&chip->state_lock); > + > + return ret; > +} > + > + > +#define INA2XX_CHAN(_type, _index, _address) { \ > + .type = _type, \ > + .address = _address, \ > + .indexed = 1, \ > + .channel = (_index), \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > + .info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \ > + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \ > + .scan_index = (_index), \ > + .scan_type = { \ > + .sign = 'u', \ > + .realbits = 16, \ > + .storagebits = 16, \ > + .endianness = IIO_BE, \ > + } \ > +} > + > +/*Sampling Freq is a consequence of the integration times of the V channels.*/ > +#define INA2XX_CHAN_VOLTAGE(_index, _address) { \ > + .type = IIO_VOLTAGE, \ > + .address = _address, \ > + .indexed = 1, \ > + .channel = (_index), \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ > + BIT(IIO_CHAN_INFO_INT_TIME), \ > + .scan_index = (_index), \ > + .scan_type = { \ > + .sign = 'u', \ > + .realbits = 16, \ > + .storagebits = 16, \ > + .endianness = IIO_BE, \ > + } \ > +} > + > + > +static const struct iio_chan_spec ina2xx_channels[] = { > + INA2XX_CHAN_VOLTAGE(0, INA2XX_SHUNT_VOLTAGE), > + INA2XX_CHAN_VOLTAGE(1, INA2XX_BUS_VOLTAGE), > + INA2XX_CHAN(IIO_CURRENT, 2, INA2XX_CURRENT), > + INA2XX_CHAN(IIO_POWER, 3, INA2XX_POWER), > + IIO_CHAN_SOFT_TIMESTAMP(4), > +}; > + > + > +static s64 prev_ns; This prevents you having more than one instance of the device - move it into your chip_info. > + > +static int ina2xx_work_buffer(struct iio_dev *indio_dev) > +{ > + struct ina2xx_chip_info *chip = iio_priv(indio_dev); > + unsigned short data[8]; > + int bit, ret = 0, i = 0; > + unsigned long buffer_us = 0, elapsed_us = 0; > + s64 time_a, time_b; > + unsigned int alert; > + > + time_a = iio_get_time_ns(); > + > + /* > + * Because the timer thread and the chip conversion clock > + * are asynchronous, the period difference will eventually > + * result in reading V[k-1] again, or skip V[k] at time Tk. > + * In order to resync the timer with the conversion process > + * we check the ConVersionReadyFlag. > + * On hardware that supports using the ALERT pin to toggle a > + * GPIO a triggered buffer could be used instead. > + * For now, we pay for that extra read of the ALERT register > + */ > + do { > + ret = regmap_read(chip->regmap, INA226_ALERT_MASK, > + &alert); > + if (ret < 0) > + goto _err; > + > + alert &= INA266_CVRF; > + trace_printk("Conversion ready: %d\n", !!alert); > + > + } while (!alert); > + > + /* > + * Single register reads: bulk_read will not work with ina226 > + * as there is no auto-increment of the address register for > + * data length longer than 16bits. > + */ > + for_each_set_bit(bit, indio_dev->active_scan_mask, > + indio_dev->masklength) { > + unsigned int val; > + > + ret = regmap_read(chip->regmap, > + INA2XX_SHUNT_VOLTAGE + bit, &val); > + if (ret < 0) > + goto _err; > + > + data[i++] = val; > + } > + > + time_b = iio_get_time_ns(); > + > + iio_push_to_buffers_with_timestamp(indio_dev, > + (unsigned int *)data, time_a); > + > + buffer_us = (unsigned long)(time_b - time_a) / 1000; > + elapsed_us = (unsigned long)(time_a - prev_ns) / 1000; > + > + trace_printk("uS: elapsed: %lu, buf: %lu\n", elapsed_us, buffer_us); > + > + prev_ns = time_a; > + > +_err: > + return buffer_us; > +}; > + > +static int ina2xx_capture_thread(void *data) > +{ > + struct iio_dev *indio_dev = (struct iio_dev *)data; > + struct ina2xx_chip_info *chip = iio_priv(indio_dev); > + unsigned int sampling_us = (chip->itb + chip->its) * chip->avg; > + unsigned long buffer_us; > + > + /* > + * Poll a bit faster than the chip internal Fs, in case > + * we wish to sync with the conversion ready flag. > + */ > + sampling_us -= 200; > + > + do { > + buffer_us = ina2xx_work_buffer(indio_dev); > + > + if (sampling_us > buffer_us) > + udelay(sampling_us - buffer_us); > + > + } while (!kthread_should_stop()); > + > + return 0; > +} > + > +int ina2xx_buffer_enable(struct iio_dev *indio_dev) > +{ > + struct ina2xx_chip_info *chip = iio_priv(indio_dev); > + unsigned int sampling_us = (chip->itb + chip->its) * chip->avg; > + > + trace_printk("Enabling buffer w/ scan_mask %02x, freq = %d, avg =%u\n", > + (unsigned int)(*indio_dev->active_scan_mask), > + 1000000/sampling_us, chip->avg); > + > + trace_printk("Expected work period: %u us\n", sampling_us); > + > + prev_ns = iio_get_time_ns(); > + > + chip->task = kthread_run(ina2xx_capture_thread, (void *)indio_dev, > + "ina2xx-%uus", sampling_us); > + > + return PTR_ERR_OR_ZERO(chip->task); > +} > + > +int ina2xx_buffer_disable(struct iio_dev *indio_dev) > +{ > + struct ina2xx_chip_info *chip = iio_priv(indio_dev); > + > + if (chip->task) { > + kthread_stop(chip->task); > + chip->task = NULL; > + } nitpick: blank line here. > + return 0; > +} > + > +static const struct iio_buffer_setup_ops ina2xx_setup_ops = { > + .postenable = &ina2xx_buffer_enable, > + .postdisable = &ina2xx_buffer_disable, > +}; > + > +static int ina2xx_debug_reg(struct iio_dev *indio_dev, > + unsigned reg, unsigned writeval, unsigned *readval) > +{ > + struct ina2xx_chip_info *chip = iio_priv(indio_dev); > + > + if (!readval) > + return regmap_write(chip->regmap, reg, writeval); > + > + return regmap_read(chip->regmap, reg, readval); > +} > + > +/* frequencies matching the cummulated integration times for vshunt and vbus */ huh? Integration time is in seconds... These seem unlikely to be valid settings. > +static IIO_CONST_ATTR_INT_TIME_AVAIL("140 204 332 588 1100 2116 4156 8244"); > + > +static struct attribute *ina2xx_attributes[] = { > + &iio_const_attr_integration_time_available.dev_attr.attr, > + NULL, > +}; > + > +static const struct attribute_group ina2xx_attribute_group = { > + .attrs = ina2xx_attributes, > +}; > + > +static const struct iio_info ina2xx_info = { > + .debugfs_reg_access = &ina2xx_debug_reg, > + .read_raw = &ina2xx_read_raw, > + .write_raw = &ina2xx_write_raw, > + .attrs = &ina2xx_attribute_group, > + .driver_module = THIS_MODULE, > +}; > + > +/* Initialize the configuration and calibration registers. */ > +static int ina2xx_init(struct ina2xx_chip_info *chip, unsigned int config) > +{ > + u16 regval; > + int ret = regmap_write(chip->regmap, INA2XX_CONFIG, config); > + > + if (ret < 0) > + return ret; > + /* > + * Set current LSB to 1mA, shunt is in uOhms > + * (equation 13 in datasheet). We hardcode a Current_LSB > + * of 1.0 x10-6. The only remaining parameter is RShunt > + * There is no need to expose the CALIBRATION register > + * to the user for now. > + */ > + regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor, > + chip->rshunt); > + > + return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval); > +} > + > +static int ina2xx_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct ina2xx_chip_info *chip; > + struct iio_dev *indio_dev; > + struct iio_buffer *buffer; > + int ret; > + unsigned int val; > + > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip)); > + if (!indio_dev) > + return -ENOMEM; > + > + chip = iio_priv(indio_dev); > + > + chip->config = &ina2xx_config[id->driver_data]; > + > + if (of_property_read_u32(client->dev.of_node, > + "shunt-resistor", &val) < 0) { > + struct ina2xx_platform_data *pdata > + dev_get_platdata(&client->dev); > + > + if (pdata) > + val = pdata->shunt_uohms; > + else > + val = INA2XX_RSHUNT_DEFAULT; > + } > + > + if (val <= 0 || val > chip->config->calibration_factor) > + return -ENODEV; > + > + chip->rshunt = val; > + > + ina2xx_regmap_config.max_register = chip->config->registers; > + > + mutex_init(&chip->state_lock); > + > + /* this is only used for device removal purposes */ > + i2c_set_clientdata(client, indio_dev); > + > + indio_dev->name = id->name; > + indio_dev->channels = ina2xx_channels; > + indio_dev->num_channels = ARRAY_SIZE(ina2xx_channels); > + > + indio_dev->dev.parent = &client->dev; > + indio_dev->info = &ina2xx_info; > + indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE; > + > + chip->regmap = devm_regmap_init_i2c(client, &ina2xx_regmap_config); > + if (IS_ERR(chip->regmap)) { > + dev_err(&client->dev, "failed to allocate register map\n"); > + return PTR_ERR(chip->regmap); > + } > + > + /* Patch the current config register with default. */ > + val = chip->config->config_default; > + > + if (id->driver_data = ina226) { > + ina226_set_average(chip, INA226_DEFAULT_AVG, &val); > + ina226_set_itb(chip, INA226_DEFAULT_IT, &val); > + ina226_set_its(chip, INA226_DEFAULT_IT, &val); > + } > + > + ret = ina2xx_init(chip, val); > + if (ret < 0) { > + dev_err(&client->dev, "error configuring the device: %d\n", > + ret); > + return -ENODEV; > + } > + > + buffer = devm_iio_kfifo_allocate(&indio_dev->dev); > + if (!buffer) > + return -ENOMEM; > + > + indio_dev->setup_ops = &ina2xx_setup_ops; > + > + iio_device_attach_buffer(indio_dev, buffer); > + > + return devm_iio_device_register(&client->dev, indio_dev); > +} > + > + > +static int ina2xx_remove(struct i2c_client *client) > +{ > + struct iio_dev *indio_dev = i2c_get_clientdata(client); > + struct ina2xx_chip_info *chip = iio_priv(indio_dev); > + int ret; > + > + mutex_destroy(&chip->state_lock); > + > + /* Powerdown */ > + ret = regmap_update_bits(chip->regmap, INA2XX_CONFIG, > + INA2XX_MODE_MASK, 0); > + > + iio_device_unregister(indio_dev); Peter already covered this to a degree. iio_device_unregister removes the userspace interface - hence it must be the first thing done in remove. If you use devm_ version of register it will occur at the end of the remove. Hence if the remove has anything in it, you pretty much can't use the devm_iio_device_register without introducing a nasty race condition. Hence you don't want the devm version here and you want to move iio_device_unregister to the beginning of this function. > + > + return ret; > +} > + > + > +static const struct i2c_device_id ina2xx_id[] = { > + {"ina219", ina219}, > + {"ina220", ina219}, > + {"ina226", ina226}, > + {"ina230", ina226}, > + {"ina231", ina226}, > + {} > +}; > + > +MODULE_DEVICE_TABLE(i2c, ina2xx_id); > + > +static struct i2c_driver ina2xx_driver = { > + .driver = { > + .name = KBUILD_MODNAME, > + }, > + .probe = ina2xx_probe, > + .remove = ina2xx_remove, > + .id_table = ina2xx_id, > +}; > + > +module_i2c_driver(ina2xx_driver); > + > +MODULE_AUTHOR("Marc Titinger <marc.titinger@baylibre.com>"); > +MODULE_DESCRIPTION("Texas Instruments INA2XX ADC driver"); > +MODULE_LICENSE("GPL v2"); > _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] iio: ina2xx: add support for TI INA2xx Power Monitors 2015-11-29 16:31 ` [lm-sensors] [PATCH 1/2] iio: ina2xx: add support for TI INA2xx Power Monitors Jonathan Cameron @ 2015-11-29 17:38 ` Guenter Roeck 2015-12-01 10:01 ` Marc Titinger 0 siblings, 1 reply; 5+ messages in thread From: Guenter Roeck @ 2015-11-29 17:38 UTC (permalink / raw) To: Jonathan Cameron, Marc Titinger, knaack.h, lars, pmeerw, jdelvare Cc: linux-iio, linux-kernel, lm-sensors On 11/29/2015 08:31 AM, Jonathan Cameron wrote: > On 25/11/15 11:28, Marc Titinger wrote: >> in SOFTWARE buffer mode, a kthread will capture the active scan_elements >> into a kfifo, then compute the remaining time until the next capture tick >> and do an active wait (udelay). >> >> This will produce a stream of up to fours channels plus a 64bits >> timestamps (ns). >> >> Tested with ina226, on BeagleBoneBlack. >> >> Datasheet: http://www.ti.com/lit/gpn/ina226 >> >> Signed-off-by: Marc Titinger <mtitinger@baylibre.com> > A few more bits from me, but basically looking good. > > We do however, need to make the hwmon guys aware this is going on. > Please cc their list on the next version. > > Last thing we want is for this to turn up as a surprise! > I have seen the original patch, so no surprise here. Just not sure if we should remove the hwmon driver after the iio driver is accepted. Even though the stated goal is different, it seems to me that having two drivers really does not make sense. Any thoughts ? Guenter > Jonathan >> --- >> drivers/iio/adc/Kconfig | 10 + >> drivers/iio/adc/Makefile | 1 + >> drivers/iio/adc/ina2xx-iio.c | 684 +++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 695 insertions(+) >> create mode 100644 drivers/iio/adc/ina2xx-iio.c >> >> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig >> index eb0cd89..9b87372 100644 >> --- a/drivers/iio/adc/Kconfig >> +++ b/drivers/iio/adc/Kconfig >> @@ -170,6 +170,16 @@ config EXYNOS_ADC >> of SoCs for drivers such as the touchscreen and hwmon to use to share >> this resource. >> >> +config INA2XX_IIO >> + tristate "Texas Instruments INA2xx Power Monitors IIO driver" >> + depends on I2C >> + select REGMAP_I2C >> + select IIO_BUFFER >> + help >> + Say yes here to build support for TI INA2xx familly Power Monitors. >> + >> + Note that this is not the existing hwmon interface for this device. >> + >> config LP8788_ADC >> tristate "LP8788 ADC driver" >> depends on MFD_LP8788 >> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile >> index a096210..74e4341 100644 >> --- a/drivers/iio/adc/Makefile >> +++ b/drivers/iio/adc/Makefile >> @@ -19,6 +19,7 @@ obj-$(CONFIG_BERLIN2_ADC) += berlin2-adc.o >> obj-$(CONFIG_DA9150_GPADC) += da9150-gpadc.o >> obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o >> obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o >> +obj-$(CONFIG_INA2XX_IIO) += ina2xx-iio.o >> obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o >> obj-$(CONFIG_MAX1027) += max1027.o >> obj-$(CONFIG_MAX1363) += max1363.o >> diff --git a/drivers/iio/adc/ina2xx-iio.c b/drivers/iio/adc/ina2xx-iio.c >> new file mode 100644 >> index 0000000..4a0026c >> --- /dev/null >> +++ b/drivers/iio/adc/ina2xx-iio.c >> @@ -0,0 +1,684 @@ >> +/* >> + * INA2XX Current and Power Monitors >> + * >> + * Copyright 2015 Baylibre SAS. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + * >> + * Based on linux/drivers/iio/adc/ad7291.c >> + * Copyright 2010-2011 Analog Devices Inc. >> + * >> + * Based on linux/drivers/hwmon/ina2xx.c >> + * Copyright 2012 Lothar Felten <l-felten@ti.com> >> + * >> + * Licensed under the GPL-2 or later. >> + * >> + * IIO driver for INA219-220-226-230-231 >> + * >> + * Configurable 7-bit I2C slave address from 0x40 to 0x4F >> + */ >> +#include <linux/module.h> >> +#include <linux/kthread.h> >> +#include <linux/delay.h> >> +#include <linux/iio/kfifo_buf.h> >> +#include <linux/iio/sysfs.h> >> +#include <linux/i2c.h> >> +#include <linux/regmap.h> >> +#include <linux/platform_data/ina2xx.h> >> + >> +#include <linux/util_macros.h> >> + >> +/* >> + * INA2XX registers definition >> + */ >> +/* common register definitions */ >> +#define INA2XX_CONFIG 0x00 >> +#define INA2XX_SHUNT_VOLTAGE 0x01 /* readonly */ >> +#define INA2XX_BUS_VOLTAGE 0x02 /* readonly */ >> +#define INA2XX_POWER 0x03 /* readonly */ >> +#define INA2XX_CURRENT 0x04 /* readonly */ >> +#define INA2XX_CALIBRATION 0x05 >> + >> +#define INA226_ALERT_MASK 0x06 >> +#define INA266_CVRF BIT(3) >> + >> +/* register count */ >> +#define INA219_REGISTERS 6 >> +#define INA226_REGISTERS 8 >> +#define INA2XX_MAX_REGISTERS 8 >> + >> +/* settings - depend on use case */ >> +#define INA219_CONFIG_DEFAULT 0x399F /* PGA=8 */ >> +#define INA226_CONFIG_DEFAULT 0x4327 >> +#define INA226_DEFAULT_AVG 4 >> +#define INA226_DEFAULT_IT 1110 >> + >> +#define INA2XX_RSHUNT_DEFAULT 10000 >> + >> +/* >> + * bit mask for reading the averaging setting in the configuration register >> + * FIXME: use regmap_fields. >> + */ >> +#define INA2XX_MODE_MASK GENMASK(3, 0) >> + >> +#define INA226_AVG_MASK GENMASK(11, 9) >> +#define INA226_SHIFT_AVG(val) ((val) << 9) >> + >> +/* Integration time for VBus */ >> +#define INA226_ITB_MASK GENMASK(8, 6) >> +#define INA226_SHIFT_ITB(val) ((val) << 6) >> + >> +/*Integration Time for VShunt */ >> +#define INA226_ITS_MASK GENMASK(5, 3) >> +#define INA226_SHIFT_ITS(val) ((val) << 3) >> + >> + >> +static bool ina2xx_is_writeable_reg(struct device *dev, unsigned int reg) >> +{ >> + return (reg = INA2XX_CONFIG) || (reg > INA2XX_CURRENT); >> +} >> + >> +static bool ina2xx_is_volatile_reg(struct device *dev, unsigned int reg) >> +{ >> + return (reg != INA2XX_CONFIG); >> +} >> + >> +static struct regmap_config ina2xx_regmap_config = { >> + .reg_bits = 8, >> + .val_bits = 16, >> + >> + .writeable_reg = ina2xx_is_writeable_reg, >> + .volatile_reg = ina2xx_is_volatile_reg, >> +}; >> + >> +enum ina2xx_ids { ina219, ina226 }; >> + >> +struct ina2xx_config { >> + u16 config_default; >> + int calibration_factor; >> + int registers; >> + int shunt_div; >> + int bus_voltage_shift; >> + int bus_voltage_lsb; /* uV */ >> + int power_lsb; /* uW */ >> +}; >> + >> +struct ina2xx_chip_info { >> + struct regmap *regmap; >> + struct task_struct *task; >> + const struct ina2xx_config *config; >> + struct mutex state_lock; >> + long rshunt; >> + int avg; >> + int itb; /* Bus voltage integration time uS */ >> + int its; /* Shunt voltage integration tim uS */ >> +}; >> + >> +static const struct ina2xx_config ina2xx_config[] = { >> + [ina219] = { >> + .config_default = INA219_CONFIG_DEFAULT, >> + .calibration_factor = 40960000, >> + .registers = INA219_REGISTERS, >> + .shunt_div = 100, >> + .bus_voltage_shift = 3, >> + .bus_voltage_lsb = 4000, >> + .power_lsb = 20000, >> + }, >> + [ina226] = { >> + .config_default = INA226_CONFIG_DEFAULT, >> + .calibration_factor = 5120000, >> + .registers = INA226_REGISTERS, >> + .shunt_div = 400, >> + .bus_voltage_shift = 0, >> + .bus_voltage_lsb = 1250, >> + .power_lsb = 25000, >> + }, >> +}; >> + >> +static int ina2xx_get_value(struct ina2xx_chip_info *chip, u8 reg, >> + unsigned int regval, int *val, int *uval) >> +{ >> + *val = 0; >> + >> + switch (reg) { >> + case INA2XX_SHUNT_VOLTAGE: >> + /* signed register */ >> + *uval = DIV_ROUND_CLOSEST((s16) regval, >> + chip->config->shunt_div); >> + return IIO_VAL_INT_PLUS_MICRO; >> + >> + case INA2XX_BUS_VOLTAGE: >> + *uval = (regval >> chip->config->bus_voltage_shift) >> + * chip->config->bus_voltage_lsb; >> + *val = *uval/1000000; >> + *uval = *uval % 1000000; >> + return IIO_VAL_INT_PLUS_MICRO; >> + >> + case INA2XX_POWER: >> + *uval = regval * chip->config->power_lsb; >> + *val = *uval/1000000; >> + *uval = *uval % 1000000; >> + return IIO_VAL_INT_PLUS_MICRO; >> + >> + case INA2XX_CURRENT: >> + /* signed register, LSB=1mA (selected), in mA */ >> + *uval = (s16) regval * 1000; >> + return IIO_VAL_INT_PLUS_MICRO; >> + >> + default: >> + /* programmer goofed */ >> + WARN_ON_ONCE(1); >> + } >> + return -EINVAL; >> +} >> + >> +static int ina2xx_read_raw(struct iio_dev *indio_dev, >> + struct iio_chan_spec const *chan, >> + int *val, int *val2, long mask) >> +{ >> + int ret; >> + struct ina2xx_chip_info *chip = iio_priv(indio_dev); >> + unsigned int regval; >> + >> + switch (mask) { >> + case IIO_CHAN_INFO_RAW: >> + ret = regmap_read(chip->regmap, chan->address, ®val); >> + if (ret < 0) >> + return ret; >> + >> + return ina2xx_get_value(chip, chan->address, regval, val, val2); >> + >> + case IIO_CHAN_INFO_OVERSAMPLING_RATIO: >> + *val = chip->avg; >> + return IIO_VAL_INT; >> + >> + case IIO_CHAN_INFO_INT_TIME: >> + *val = 0; >> + if (chan->address = INA2XX_SHUNT_VOLTAGE) >> + *val2 = chip->its; >> + else >> + *val2 = chip->itb; >> + >> + return IIO_VAL_INT_PLUS_MICRO; >> + /* >> + * sample freq is read only, it is a consequence of >> + * 1/AVG*(CT_bus+CT_shunt) >> + */ >> + case IIO_CHAN_INFO_SAMP_FREQ: >> + *val = DIV_ROUND_CLOSEST(1000000, >> + (chip->itb + chip->its) * chip->avg); >> + >> + return IIO_VAL_INT; >> + >> + default: >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + >> +/* >> + * Available averaging rates for ina226. The indices correspond with >> + * the bit values expected by the chip (according to the ina226 datasheet, >> + * table 3 AVG bit settings, found at >> + * http://www.ti.com/lit/ds/symlink/ina226.pdf. >> + */ >> +static const int ina226_avg_tab[] = { 1, 4, 16, 64, 128, 256, 512, 1024 }; >> + >> +static unsigned int ina226_set_average(struct ina2xx_chip_info *chip, >> + unsigned int val, >> + unsigned int *config) >> +{ >> + int bits; >> + >> + if (val > 1024 || val < 1) >> + return -EINVAL; >> + >> + bits = find_closest(val, ina226_avg_tab, >> + ARRAY_SIZE(ina226_avg_tab)); >> + >> + chip->avg = ina226_avg_tab[bits]; >> + >> + *config &= ~INA226_AVG_MASK; >> + *config |= INA226_SHIFT_AVG(bits) & INA226_AVG_MASK; >> + >> + return 0; >> +} >> + >> +/* Conversion times in uS */ >> +static const int ina226_conv_time_tab[] = { 140, 204, 332, 588, 1100, >> + 2116, 4156, 8244}; >> + >> +static unsigned int ina226_set_itb(struct ina2xx_chip_info *chip, >> + unsigned int val, >> + unsigned int *config) >> +{ >> + int bits; >> + >> + if (val > 8244 || val < 140) >> + return -EINVAL; >> + >> + bits = find_closest(val, ina226_conv_time_tab, >> + ARRAY_SIZE(ina226_conv_time_tab)); >> + >> + chip->itb = ina226_conv_time_tab[bits]; >> + >> + *config &= ~INA226_ITB_MASK; >> + *config |= INA226_SHIFT_ITB(bits) & INA226_ITB_MASK; >> + >> + return 0; >> +} >> + > Could do with a slightly more informative name than its vs itb. > Or at least some docs explaining what it does. >> +static unsigned int ina226_set_its(struct ina2xx_chip_info *chip, >> + unsigned int val, >> + unsigned int *config) >> +{ >> + int bits; >> + >> + if (val > 8244 || val < 140) >> + return -EINVAL; >> + >> + bits = find_closest(val, ina226_conv_time_tab, >> + ARRAY_SIZE(ina226_conv_time_tab)); >> + >> + chip->its = ina226_conv_time_tab[bits]; >> + >> + *config &= ~INA226_ITS_MASK; >> + *config |= INA226_SHIFT_ITS(bits) & INA226_ITS_MASK; >> + >> + return 0; >> +} >> + >> + >> +static int ina2xx_write_raw(struct iio_dev *indio_dev, >> + struct iio_chan_spec const *chan, >> + int val, int val2, long mask) >> +{ >> + struct ina2xx_chip_info *chip = iio_priv(indio_dev); >> + int ret = 0; >> + unsigned int config, tmp; >> + >> + if (iio_buffer_enabled(indio_dev)) >> + return -EBUSY; >> + >> + mutex_lock(&chip->state_lock); >> + >> + ret = regmap_read(chip->regmap, INA2XX_CONFIG, &config); >> + if (ret < 0) >> + goto _err; >> + >> + tmp = config; >> + >> + switch (mask) { >> + case IIO_CHAN_INFO_OVERSAMPLING_RATIO: >> + ret = ina226_set_average(chip, val, &tmp); >> + break; >> + >> + case IIO_CHAN_INFO_INT_TIME: >> + if (chan->address = INA2XX_SHUNT_VOLTAGE) >> + ret = ina226_set_its(chip, val, &tmp); >> + else >> + ret = ina226_set_itb(chip, val, &tmp); >> + break; >> + default: >> + ret = -EINVAL; >> + } >> + >> + if (!ret && (tmp != config)) >> + ret = regmap_write(chip->regmap, INA2XX_CONFIG, tmp); >> +_err: >> + mutex_unlock(&chip->state_lock); >> + >> + return ret; >> +} >> + >> + >> +#define INA2XX_CHAN(_type, _index, _address) { \ >> + .type = _type, \ >> + .address = _address, \ >> + .indexed = 1, \ >> + .channel = (_index), \ >> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ >> + .info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \ >> + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \ >> + .scan_index = (_index), \ >> + .scan_type = { \ >> + .sign = 'u', \ >> + .realbits = 16, \ >> + .storagebits = 16, \ >> + .endianness = IIO_BE, \ >> + } \ >> +} >> + >> +/*Sampling Freq is a consequence of the integration times of the V channels.*/ >> +#define INA2XX_CHAN_VOLTAGE(_index, _address) { \ >> + .type = IIO_VOLTAGE, \ >> + .address = _address, \ >> + .indexed = 1, \ >> + .channel = (_index), \ >> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ >> + BIT(IIO_CHAN_INFO_INT_TIME), \ >> + .scan_index = (_index), \ >> + .scan_type = { \ >> + .sign = 'u', \ >> + .realbits = 16, \ >> + .storagebits = 16, \ >> + .endianness = IIO_BE, \ >> + } \ >> +} >> + >> + >> +static const struct iio_chan_spec ina2xx_channels[] = { >> + INA2XX_CHAN_VOLTAGE(0, INA2XX_SHUNT_VOLTAGE), >> + INA2XX_CHAN_VOLTAGE(1, INA2XX_BUS_VOLTAGE), >> + INA2XX_CHAN(IIO_CURRENT, 2, INA2XX_CURRENT), >> + INA2XX_CHAN(IIO_POWER, 3, INA2XX_POWER), >> + IIO_CHAN_SOFT_TIMESTAMP(4), >> +}; >> + >> + >> +static s64 prev_ns; > This prevents you having more than one instance of the device - move it into > your chip_info. >> + >> +static int ina2xx_work_buffer(struct iio_dev *indio_dev) >> +{ >> + struct ina2xx_chip_info *chip = iio_priv(indio_dev); >> + unsigned short data[8]; >> + int bit, ret = 0, i = 0; >> + unsigned long buffer_us = 0, elapsed_us = 0; >> + s64 time_a, time_b; >> + unsigned int alert; >> + >> + time_a = iio_get_time_ns(); >> + >> + /* >> + * Because the timer thread and the chip conversion clock >> + * are asynchronous, the period difference will eventually >> + * result in reading V[k-1] again, or skip V[k] at time Tk. >> + * In order to resync the timer with the conversion process >> + * we check the ConVersionReadyFlag. >> + * On hardware that supports using the ALERT pin to toggle a >> + * GPIO a triggered buffer could be used instead. >> + * For now, we pay for that extra read of the ALERT register >> + */ >> + do { >> + ret = regmap_read(chip->regmap, INA226_ALERT_MASK, >> + &alert); >> + if (ret < 0) >> + goto _err; >> + >> + alert &= INA266_CVRF; >> + trace_printk("Conversion ready: %d\n", !!alert); >> + >> + } while (!alert); >> + >> + /* >> + * Single register reads: bulk_read will not work with ina226 >> + * as there is no auto-increment of the address register for >> + * data length longer than 16bits. >> + */ >> + for_each_set_bit(bit, indio_dev->active_scan_mask, >> + indio_dev->masklength) { >> + unsigned int val; >> + >> + ret = regmap_read(chip->regmap, >> + INA2XX_SHUNT_VOLTAGE + bit, &val); >> + if (ret < 0) >> + goto _err; >> + >> + data[i++] = val; >> + } >> + >> + time_b = iio_get_time_ns(); >> + >> + iio_push_to_buffers_with_timestamp(indio_dev, >> + (unsigned int *)data, time_a); >> + >> + buffer_us = (unsigned long)(time_b - time_a) / 1000; >> + elapsed_us = (unsigned long)(time_a - prev_ns) / 1000; >> + >> + trace_printk("uS: elapsed: %lu, buf: %lu\n", elapsed_us, buffer_us); >> + >> + prev_ns = time_a; >> + >> +_err: >> + return buffer_us; >> +}; >> + >> +static int ina2xx_capture_thread(void *data) >> +{ >> + struct iio_dev *indio_dev = (struct iio_dev *)data; >> + struct ina2xx_chip_info *chip = iio_priv(indio_dev); >> + unsigned int sampling_us = (chip->itb + chip->its) * chip->avg; >> + unsigned long buffer_us; >> + >> + /* >> + * Poll a bit faster than the chip internal Fs, in case >> + * we wish to sync with the conversion ready flag. >> + */ >> + sampling_us -= 200; >> + >> + do { >> + buffer_us = ina2xx_work_buffer(indio_dev); >> + >> + if (sampling_us > buffer_us) >> + udelay(sampling_us - buffer_us); >> + >> + } while (!kthread_should_stop()); >> + >> + return 0; >> +} >> + >> +int ina2xx_buffer_enable(struct iio_dev *indio_dev) >> +{ >> + struct ina2xx_chip_info *chip = iio_priv(indio_dev); >> + unsigned int sampling_us = (chip->itb + chip->its) * chip->avg; >> + >> + trace_printk("Enabling buffer w/ scan_mask %02x, freq = %d, avg =%u\n", >> + (unsigned int)(*indio_dev->active_scan_mask), >> + 1000000/sampling_us, chip->avg); >> + >> + trace_printk("Expected work period: %u us\n", sampling_us); >> + >> + prev_ns = iio_get_time_ns(); >> + >> + chip->task = kthread_run(ina2xx_capture_thread, (void *)indio_dev, >> + "ina2xx-%uus", sampling_us); >> + >> + return PTR_ERR_OR_ZERO(chip->task); >> +} >> + >> +int ina2xx_buffer_disable(struct iio_dev *indio_dev) >> +{ >> + struct ina2xx_chip_info *chip = iio_priv(indio_dev); >> + >> + if (chip->task) { >> + kthread_stop(chip->task); >> + chip->task = NULL; >> + } > nitpick: blank line here. >> + return 0; >> +} >> + >> +static const struct iio_buffer_setup_ops ina2xx_setup_ops = { >> + .postenable = &ina2xx_buffer_enable, >> + .postdisable = &ina2xx_buffer_disable, >> +}; >> + >> +static int ina2xx_debug_reg(struct iio_dev *indio_dev, >> + unsigned reg, unsigned writeval, unsigned *readval) >> +{ >> + struct ina2xx_chip_info *chip = iio_priv(indio_dev); >> + >> + if (!readval) >> + return regmap_write(chip->regmap, reg, writeval); >> + >> + return regmap_read(chip->regmap, reg, readval); >> +} >> + >> +/* frequencies matching the cummulated integration times for vshunt and vbus */ > huh? Integration time is in seconds... These seem unlikely to be valid > settings. >> +static IIO_CONST_ATTR_INT_TIME_AVAIL("140 204 332 588 1100 2116 4156 8244"); >> + >> +static struct attribute *ina2xx_attributes[] = { >> + &iio_const_attr_integration_time_available.dev_attr.attr, >> + NULL, >> +}; >> + >> +static const struct attribute_group ina2xx_attribute_group = { >> + .attrs = ina2xx_attributes, >> +}; >> + >> +static const struct iio_info ina2xx_info = { >> + .debugfs_reg_access = &ina2xx_debug_reg, >> + .read_raw = &ina2xx_read_raw, >> + .write_raw = &ina2xx_write_raw, >> + .attrs = &ina2xx_attribute_group, >> + .driver_module = THIS_MODULE, >> +}; >> + >> +/* Initialize the configuration and calibration registers. */ >> +static int ina2xx_init(struct ina2xx_chip_info *chip, unsigned int config) >> +{ >> + u16 regval; >> + int ret = regmap_write(chip->regmap, INA2XX_CONFIG, config); >> + >> + if (ret < 0) >> + return ret; >> + /* >> + * Set current LSB to 1mA, shunt is in uOhms >> + * (equation 13 in datasheet). We hardcode a Current_LSB >> + * of 1.0 x10-6. The only remaining parameter is RShunt >> + * There is no need to expose the CALIBRATION register >> + * to the user for now. >> + */ >> + regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor, >> + chip->rshunt); >> + >> + return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval); >> +} >> + >> +static int ina2xx_probe(struct i2c_client *client, >> + const struct i2c_device_id *id) >> +{ >> + struct ina2xx_chip_info *chip; >> + struct iio_dev *indio_dev; >> + struct iio_buffer *buffer; >> + int ret; >> + unsigned int val; >> + >> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip)); >> + if (!indio_dev) >> + return -ENOMEM; >> + >> + chip = iio_priv(indio_dev); >> + >> + chip->config = &ina2xx_config[id->driver_data]; >> + >> + if (of_property_read_u32(client->dev.of_node, >> + "shunt-resistor", &val) < 0) { >> + struct ina2xx_platform_data *pdata >> + dev_get_platdata(&client->dev); >> + >> + if (pdata) >> + val = pdata->shunt_uohms; >> + else >> + val = INA2XX_RSHUNT_DEFAULT; >> + } >> + >> + if (val <= 0 || val > chip->config->calibration_factor) >> + return -ENODEV; >> + >> + chip->rshunt = val; >> + >> + ina2xx_regmap_config.max_register = chip->config->registers; >> + >> + mutex_init(&chip->state_lock); >> + >> + /* this is only used for device removal purposes */ >> + i2c_set_clientdata(client, indio_dev); >> + >> + indio_dev->name = id->name; >> + indio_dev->channels = ina2xx_channels; >> + indio_dev->num_channels = ARRAY_SIZE(ina2xx_channels); >> + >> + indio_dev->dev.parent = &client->dev; >> + indio_dev->info = &ina2xx_info; >> + indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE; >> + >> + chip->regmap = devm_regmap_init_i2c(client, &ina2xx_regmap_config); >> + if (IS_ERR(chip->regmap)) { >> + dev_err(&client->dev, "failed to allocate register map\n"); >> + return PTR_ERR(chip->regmap); >> + } >> + >> + /* Patch the current config register with default. */ >> + val = chip->config->config_default; >> + >> + if (id->driver_data = ina226) { >> + ina226_set_average(chip, INA226_DEFAULT_AVG, &val); >> + ina226_set_itb(chip, INA226_DEFAULT_IT, &val); >> + ina226_set_its(chip, INA226_DEFAULT_IT, &val); >> + } >> + >> + ret = ina2xx_init(chip, val); >> + if (ret < 0) { >> + dev_err(&client->dev, "error configuring the device: %d\n", >> + ret); >> + return -ENODEV; >> + } >> + >> + buffer = devm_iio_kfifo_allocate(&indio_dev->dev); >> + if (!buffer) >> + return -ENOMEM; >> + >> + indio_dev->setup_ops = &ina2xx_setup_ops; >> + >> + iio_device_attach_buffer(indio_dev, buffer); >> + >> + return devm_iio_device_register(&client->dev, indio_dev); >> +} >> + >> + >> +static int ina2xx_remove(struct i2c_client *client) >> +{ >> + struct iio_dev *indio_dev = i2c_get_clientdata(client); >> + struct ina2xx_chip_info *chip = iio_priv(indio_dev); >> + int ret; >> + >> + mutex_destroy(&chip->state_lock); >> + >> + /* Powerdown */ >> + ret = regmap_update_bits(chip->regmap, INA2XX_CONFIG, >> + INA2XX_MODE_MASK, 0); >> + >> + iio_device_unregister(indio_dev); > Peter already covered this to a degree. > > iio_device_unregister removes the userspace interface - hence it > must be the first thing done in remove. > > If you use devm_ version of register it will occur at the end of the > remove. Hence if the remove has anything in it, you pretty much > can't use the devm_iio_device_register without introducing a nasty > race condition. Hence you don't want the devm version here and you > want to move iio_device_unregister to the beginning of this function. >> + >> + return ret; >> +} >> + >> + >> +static const struct i2c_device_id ina2xx_id[] = { >> + {"ina219", ina219}, >> + {"ina220", ina219}, >> + {"ina226", ina226}, >> + {"ina230", ina226}, >> + {"ina231", ina226}, >> + {} >> +}; >> + >> +MODULE_DEVICE_TABLE(i2c, ina2xx_id); >> + >> +static struct i2c_driver ina2xx_driver = { >> + .driver = { >> + .name = KBUILD_MODNAME, >> + }, >> + .probe = ina2xx_probe, >> + .remove = ina2xx_remove, >> + .id_table = ina2xx_id, >> +}; >> + >> +module_i2c_driver(ina2xx_driver); >> + >> +MODULE_AUTHOR("Marc Titinger <marc.titinger@baylibre.com>"); >> +MODULE_DESCRIPTION("Texas Instruments INA2XX ADC driver"); >> +MODULE_LICENSE("GPL v2"); >> > > _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] iio: ina2xx: add support for TI INA2xx Power Monitors 2015-11-29 17:38 ` Guenter Roeck @ 2015-12-01 10:01 ` Marc Titinger 2015-12-01 15:02 ` Guenter Roeck 0 siblings, 1 reply; 5+ messages in thread From: Marc Titinger @ 2015-12-01 10:01 UTC (permalink / raw) To: Guenter Roeck, Jonathan Cameron, knaack.h, lars, pmeerw, jdelvare Cc: linux-iio, linux-kernel, lm-sensors On 29/11/2015 18:38, Guenter Roeck wrote: > On 11/29/2015 08:31 AM, Jonathan Cameron wrote: >> On 25/11/15 11:28, Marc Titinger wrote: >>> in SOFTWARE buffer mode, a kthread will capture the active scan_elements >>> into a kfifo, then compute the remaining time until the next capture >>> tick >>> and do an active wait (udelay). >>> >>> This will produce a stream of up to fours channels plus a 64bits >>> timestamps (ns). >>> >>> Tested with ina226, on BeagleBoneBlack. >>> >>> Datasheet: http://www.ti.com/lit/gpn/ina226 >>> >>> Signed-off-by: Marc Titinger <mtitinger@baylibre.com> >> A few more bits from me, but basically looking good. >> >> We do however, need to make the hwmon guys aware this is going on. >> Please cc their list on the next version. >> >> Last thing we want is for this to turn up as a surprise! >> > > I have seen the original patch, so no surprise here. Just not sure > if we should remove the hwmon driver after the iio driver is accepted. > Even though the stated goal is different, it seems to me that having > two drivers really does not make sense. Any thoughts ? Hi Guenter, we for instance have two completely unrelated projects both using this chip, on one side we wish to measure power consumption on the host board and hwmon has the apis and tool ecosystem for it, while on the "ACME" system we needed IIO because it's buffer streaming scheme and remote features saves the day to create a lab instrument. But really both have their use, I'd recommend keeping the hwmon driver for now. I doubt those drivers will generate much maintenance work in the future either. Kind Regards, Marc. > > Guenter > >> Jonathan >>> --- >>> drivers/iio/adc/Kconfig | 10 + >>> drivers/iio/adc/Makefile | 1 + >>> drivers/iio/adc/ina2xx-iio.c | 684 >>> +++++++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 695 insertions(+) >>> create mode 100644 drivers/iio/adc/ina2xx-iio.c >>> >>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig >>> index eb0cd89..9b87372 100644 >>> --- a/drivers/iio/adc/Kconfig >>> +++ b/drivers/iio/adc/Kconfig >>> @@ -170,6 +170,16 @@ config EXYNOS_ADC >>> of SoCs for drivers such as the touchscreen and hwmon to use >>> to share >>> this resource. >>> >>> +config INA2XX_IIO >>> + tristate "Texas Instruments INA2xx Power Monitors IIO driver" >>> + depends on I2C >>> + select REGMAP_I2C >>> + select IIO_BUFFER >>> + help >>> + Say yes here to build support for TI INA2xx familly Power >>> Monitors. >>> + >>> + Note that this is not the existing hwmon interface for this >>> device. >>> + >>> config LP8788_ADC >>> tristate "LP8788 ADC driver" >>> depends on MFD_LP8788 >>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile >>> index a096210..74e4341 100644 >>> --- a/drivers/iio/adc/Makefile >>> +++ b/drivers/iio/adc/Makefile >>> @@ -19,6 +19,7 @@ obj-$(CONFIG_BERLIN2_ADC) += berlin2-adc.o >>> obj-$(CONFIG_DA9150_GPADC) += da9150-gpadc.o >>> obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o >>> obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o >>> +obj-$(CONFIG_INA2XX_IIO) += ina2xx-iio.o >>> obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o >>> obj-$(CONFIG_MAX1027) += max1027.o >>> obj-$(CONFIG_MAX1363) += max1363.o >>> diff --git a/drivers/iio/adc/ina2xx-iio.c b/drivers/iio/adc/ina2xx-iio.c >>> new file mode 100644 >>> index 0000000..4a0026c >>> --- /dev/null >>> +++ b/drivers/iio/adc/ina2xx-iio.c >>> @@ -0,0 +1,684 @@ >>> +/* >>> + * INA2XX Current and Power Monitors >>> + * >>> + * Copyright 2015 Baylibre SAS. >>> + * >>> + * This program is free software; you can redistribute it and/or modify >>> + * it under the terms of the GNU General Public License version 2 as >>> + * published by the Free Software Foundation. >>> + * >>> + * Based on linux/drivers/iio/adc/ad7291.c >>> + * Copyright 2010-2011 Analog Devices Inc. >>> + * >>> + * Based on linux/drivers/hwmon/ina2xx.c >>> + * Copyright 2012 Lothar Felten <l-felten@ti.com> >>> + * >>> + * Licensed under the GPL-2 or later. >>> + * >>> + * IIO driver for INA219-220-226-230-231 >>> + * >>> + * Configurable 7-bit I2C slave address from 0x40 to 0x4F >>> + */ >>> +#include <linux/module.h> >>> +#include <linux/kthread.h> >>> +#include <linux/delay.h> >>> +#include <linux/iio/kfifo_buf.h> >>> +#include <linux/iio/sysfs.h> >>> +#include <linux/i2c.h> >>> +#include <linux/regmap.h> >>> +#include <linux/platform_data/ina2xx.h> >>> + >>> +#include <linux/util_macros.h> >>> + >>> +/* >>> + * INA2XX registers definition >>> + */ >>> +/* common register definitions */ >>> +#define INA2XX_CONFIG 0x00 >>> +#define INA2XX_SHUNT_VOLTAGE 0x01 /* readonly */ >>> +#define INA2XX_BUS_VOLTAGE 0x02 /* readonly */ >>> +#define INA2XX_POWER 0x03 /* readonly */ >>> +#define INA2XX_CURRENT 0x04 /* readonly */ >>> +#define INA2XX_CALIBRATION 0x05 >>> + >>> +#define INA226_ALERT_MASK 0x06 >>> +#define INA266_CVRF BIT(3) >>> + >>> +/* register count */ >>> +#define INA219_REGISTERS 6 >>> +#define INA226_REGISTERS 8 >>> +#define INA2XX_MAX_REGISTERS 8 >>> + >>> +/* settings - depend on use case */ >>> +#define INA219_CONFIG_DEFAULT 0x399F /* PGA=8 */ >>> +#define INA226_CONFIG_DEFAULT 0x4327 >>> +#define INA226_DEFAULT_AVG 4 >>> +#define INA226_DEFAULT_IT 1110 >>> + >>> +#define INA2XX_RSHUNT_DEFAULT 10000 >>> + >>> +/* >>> + * bit mask for reading the averaging setting in the configuration >>> register >>> + * FIXME: use regmap_fields. >>> + */ >>> +#define INA2XX_MODE_MASK GENMASK(3, 0) >>> + >>> +#define INA226_AVG_MASK GENMASK(11, 9) >>> +#define INA226_SHIFT_AVG(val) ((val) << 9) >>> + >>> +/* Integration time for VBus */ >>> +#define INA226_ITB_MASK GENMASK(8, 6) >>> +#define INA226_SHIFT_ITB(val) ((val) << 6) >>> + >>> +/*Integration Time for VShunt */ >>> +#define INA226_ITS_MASK GENMASK(5, 3) >>> +#define INA226_SHIFT_ITS(val) ((val) << 3) >>> + >>> + >>> +static bool ina2xx_is_writeable_reg(struct device *dev, unsigned int >>> reg) >>> +{ >>> + return (reg = INA2XX_CONFIG) || (reg > INA2XX_CURRENT); >>> +} >>> + >>> +static bool ina2xx_is_volatile_reg(struct device *dev, unsigned int >>> reg) >>> +{ >>> + return (reg != INA2XX_CONFIG); >>> +} >>> + >>> +static struct regmap_config ina2xx_regmap_config = { >>> + .reg_bits = 8, >>> + .val_bits = 16, >>> + >>> + .writeable_reg = ina2xx_is_writeable_reg, >>> + .volatile_reg = ina2xx_is_volatile_reg, >>> +}; >>> + >>> +enum ina2xx_ids { ina219, ina226 }; >>> + >>> +struct ina2xx_config { >>> + u16 config_default; >>> + int calibration_factor; >>> + int registers; >>> + int shunt_div; >>> + int bus_voltage_shift; >>> + int bus_voltage_lsb; /* uV */ >>> + int power_lsb; /* uW */ >>> +}; >>> + >>> +struct ina2xx_chip_info { >>> + struct regmap *regmap; >>> + struct task_struct *task; >>> + const struct ina2xx_config *config; >>> + struct mutex state_lock; >>> + long rshunt; >>> + int avg; >>> + int itb; /* Bus voltage integration time uS */ >>> + int its; /* Shunt voltage integration tim uS */ >>> +}; >>> + >>> +static const struct ina2xx_config ina2xx_config[] = { >>> + [ina219] = { >>> + .config_default = INA219_CONFIG_DEFAULT, >>> + .calibration_factor = 40960000, >>> + .registers = INA219_REGISTERS, >>> + .shunt_div = 100, >>> + .bus_voltage_shift = 3, >>> + .bus_voltage_lsb = 4000, >>> + .power_lsb = 20000, >>> + }, >>> + [ina226] = { >>> + .config_default = INA226_CONFIG_DEFAULT, >>> + .calibration_factor = 5120000, >>> + .registers = INA226_REGISTERS, >>> + .shunt_div = 400, >>> + .bus_voltage_shift = 0, >>> + .bus_voltage_lsb = 1250, >>> + .power_lsb = 25000, >>> + }, >>> +}; >>> + >>> +static int ina2xx_get_value(struct ina2xx_chip_info *chip, u8 reg, >>> + unsigned int regval, int *val, int *uval) >>> +{ >>> + *val = 0; >>> + >>> + switch (reg) { >>> + case INA2XX_SHUNT_VOLTAGE: >>> + /* signed register */ >>> + *uval = DIV_ROUND_CLOSEST((s16) regval, >>> + chip->config->shunt_div); >>> + return IIO_VAL_INT_PLUS_MICRO; >>> + >>> + case INA2XX_BUS_VOLTAGE: >>> + *uval = (regval >> chip->config->bus_voltage_shift) >>> + * chip->config->bus_voltage_lsb; >>> + *val = *uval/1000000; >>> + *uval = *uval % 1000000; >>> + return IIO_VAL_INT_PLUS_MICRO; >>> + >>> + case INA2XX_POWER: >>> + *uval = regval * chip->config->power_lsb; >>> + *val = *uval/1000000; >>> + *uval = *uval % 1000000; >>> + return IIO_VAL_INT_PLUS_MICRO; >>> + >>> + case INA2XX_CURRENT: >>> + /* signed register, LSB=1mA (selected), in mA */ >>> + *uval = (s16) regval * 1000; >>> + return IIO_VAL_INT_PLUS_MICRO; >>> + >>> + default: >>> + /* programmer goofed */ >>> + WARN_ON_ONCE(1); >>> + } >>> + return -EINVAL; >>> +} >>> + >>> +static int ina2xx_read_raw(struct iio_dev *indio_dev, >>> + struct iio_chan_spec const *chan, >>> + int *val, int *val2, long mask) >>> +{ >>> + int ret; >>> + struct ina2xx_chip_info *chip = iio_priv(indio_dev); >>> + unsigned int regval; >>> + >>> + switch (mask) { >>> + case IIO_CHAN_INFO_RAW: >>> + ret = regmap_read(chip->regmap, chan->address, ®val); >>> + if (ret < 0) >>> + return ret; >>> + >>> + return ina2xx_get_value(chip, chan->address, regval, val, >>> val2); >>> + >>> + case IIO_CHAN_INFO_OVERSAMPLING_RATIO: >>> + *val = chip->avg; >>> + return IIO_VAL_INT; >>> + >>> + case IIO_CHAN_INFO_INT_TIME: >>> + *val = 0; >>> + if (chan->address = INA2XX_SHUNT_VOLTAGE) >>> + *val2 = chip->its; >>> + else >>> + *val2 = chip->itb; >>> + >>> + return IIO_VAL_INT_PLUS_MICRO; >>> + /* >>> + * sample freq is read only, it is a consequence of >>> + * 1/AVG*(CT_bus+CT_shunt) >>> + */ >>> + case IIO_CHAN_INFO_SAMP_FREQ: >>> + *val = DIV_ROUND_CLOSEST(1000000, >>> + (chip->itb + chip->its) * chip->avg); >>> + >>> + return IIO_VAL_INT; >>> + >>> + default: >>> + return -EINVAL; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +/* >>> + * Available averaging rates for ina226. The indices correspond with >>> + * the bit values expected by the chip (according to the ina226 >>> datasheet, >>> + * table 3 AVG bit settings, found at >>> + * http://www.ti.com/lit/ds/symlink/ina226.pdf. >>> + */ >>> +static const int ina226_avg_tab[] = { 1, 4, 16, 64, 128, 256, 512, >>> 1024 }; >>> + >>> +static unsigned int ina226_set_average(struct ina2xx_chip_info *chip, >>> + unsigned int val, >>> + unsigned int *config) >>> +{ >>> + int bits; >>> + >>> + if (val > 1024 || val < 1) >>> + return -EINVAL; >>> + >>> + bits = find_closest(val, ina226_avg_tab, >>> + ARRAY_SIZE(ina226_avg_tab)); >>> + >>> + chip->avg = ina226_avg_tab[bits]; >>> + >>> + *config &= ~INA226_AVG_MASK; >>> + *config |= INA226_SHIFT_AVG(bits) & INA226_AVG_MASK; >>> + >>> + return 0; >>> +} >>> + >>> +/* Conversion times in uS */ >>> +static const int ina226_conv_time_tab[] = { 140, 204, 332, 588, 1100, >>> + 2116, 4156, 8244}; >>> + >>> +static unsigned int ina226_set_itb(struct ina2xx_chip_info *chip, >>> + unsigned int val, >>> + unsigned int *config) >>> +{ >>> + int bits; >>> + >>> + if (val > 8244 || val < 140) >>> + return -EINVAL; >>> + >>> + bits = find_closest(val, ina226_conv_time_tab, >>> + ARRAY_SIZE(ina226_conv_time_tab)); >>> + >>> + chip->itb = ina226_conv_time_tab[bits]; >>> + >>> + *config &= ~INA226_ITB_MASK; >>> + *config |= INA226_SHIFT_ITB(bits) & INA226_ITB_MASK; >>> + >>> + return 0; >>> +} >>> + >> Could do with a slightly more informative name than its vs itb. >> Or at least some docs explaining what it does. >>> +static unsigned int ina226_set_its(struct ina2xx_chip_info *chip, >>> + unsigned int val, >>> + unsigned int *config) >>> +{ >>> + int bits; >>> + >>> + if (val > 8244 || val < 140) >>> + return -EINVAL; >>> + >>> + bits = find_closest(val, ina226_conv_time_tab, >>> + ARRAY_SIZE(ina226_conv_time_tab)); >>> + >>> + chip->its = ina226_conv_time_tab[bits]; >>> + >>> + *config &= ~INA226_ITS_MASK; >>> + *config |= INA226_SHIFT_ITS(bits) & INA226_ITS_MASK; >>> + >>> + return 0; >>> +} >>> + >>> + >>> +static int ina2xx_write_raw(struct iio_dev *indio_dev, >>> + struct iio_chan_spec const *chan, >>> + int val, int val2, long mask) >>> +{ >>> + struct ina2xx_chip_info *chip = iio_priv(indio_dev); >>> + int ret = 0; >>> + unsigned int config, tmp; >>> + >>> + if (iio_buffer_enabled(indio_dev)) >>> + return -EBUSY; >>> + >>> + mutex_lock(&chip->state_lock); >>> + >>> + ret = regmap_read(chip->regmap, INA2XX_CONFIG, &config); >>> + if (ret < 0) >>> + goto _err; >>> + >>> + tmp = config; >>> + >>> + switch (mask) { >>> + case IIO_CHAN_INFO_OVERSAMPLING_RATIO: >>> + ret = ina226_set_average(chip, val, &tmp); >>> + break; >>> + >>> + case IIO_CHAN_INFO_INT_TIME: >>> + if (chan->address = INA2XX_SHUNT_VOLTAGE) >>> + ret = ina226_set_its(chip, val, &tmp); >>> + else >>> + ret = ina226_set_itb(chip, val, &tmp); >>> + break; >>> + default: >>> + ret = -EINVAL; >>> + } >>> + >>> + if (!ret && (tmp != config)) >>> + ret = regmap_write(chip->regmap, INA2XX_CONFIG, tmp); >>> +_err: >>> + mutex_unlock(&chip->state_lock); >>> + >>> + return ret; >>> +} >>> + >>> + >>> +#define INA2XX_CHAN(_type, _index, _address) { \ >>> + .type = _type, \ >>> + .address = _address, \ >>> + .indexed = 1, \ >>> + .channel = (_index), \ >>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ >>> + .info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \ >>> + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \ >>> + .scan_index = (_index), \ >>> + .scan_type = { \ >>> + .sign = 'u', \ >>> + .realbits = 16, \ >>> + .storagebits = 16, \ >>> + .endianness = IIO_BE, \ >>> + } \ >>> +} >>> + >>> +/*Sampling Freq is a consequence of the integration times of the V >>> channels.*/ >>> +#define INA2XX_CHAN_VOLTAGE(_index, _address) { \ >>> + .type = IIO_VOLTAGE, \ >>> + .address = _address, \ >>> + .indexed = 1, \ >>> + .channel = (_index), \ >>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ >>> + BIT(IIO_CHAN_INFO_INT_TIME), \ >>> + .scan_index = (_index), \ >>> + .scan_type = { \ >>> + .sign = 'u', \ >>> + .realbits = 16, \ >>> + .storagebits = 16, \ >>> + .endianness = IIO_BE, \ >>> + } \ >>> +} >>> + >>> + >>> +static const struct iio_chan_spec ina2xx_channels[] = { >>> + INA2XX_CHAN_VOLTAGE(0, INA2XX_SHUNT_VOLTAGE), >>> + INA2XX_CHAN_VOLTAGE(1, INA2XX_BUS_VOLTAGE), >>> + INA2XX_CHAN(IIO_CURRENT, 2, INA2XX_CURRENT), >>> + INA2XX_CHAN(IIO_POWER, 3, INA2XX_POWER), >>> + IIO_CHAN_SOFT_TIMESTAMP(4), >>> +}; >>> + >>> + >>> +static s64 prev_ns; >> This prevents you having more than one instance of the device - move >> it into >> your chip_info. >>> + >>> +static int ina2xx_work_buffer(struct iio_dev *indio_dev) >>> +{ >>> + struct ina2xx_chip_info *chip = iio_priv(indio_dev); >>> + unsigned short data[8]; >>> + int bit, ret = 0, i = 0; >>> + unsigned long buffer_us = 0, elapsed_us = 0; >>> + s64 time_a, time_b; >>> + unsigned int alert; >>> + >>> + time_a = iio_get_time_ns(); >>> + >>> + /* >>> + * Because the timer thread and the chip conversion clock >>> + * are asynchronous, the period difference will eventually >>> + * result in reading V[k-1] again, or skip V[k] at time Tk. >>> + * In order to resync the timer with the conversion process >>> + * we check the ConVersionReadyFlag. >>> + * On hardware that supports using the ALERT pin to toggle a >>> + * GPIO a triggered buffer could be used instead. >>> + * For now, we pay for that extra read of the ALERT register >>> + */ >>> + do { >>> + ret = regmap_read(chip->regmap, INA226_ALERT_MASK, >>> + &alert); >>> + if (ret < 0) >>> + goto _err; >>> + >>> + alert &= INA266_CVRF; >>> + trace_printk("Conversion ready: %d\n", !!alert); >>> + >>> + } while (!alert); >>> + >>> + /* >>> + * Single register reads: bulk_read will not work with ina226 >>> + * as there is no auto-increment of the address register for >>> + * data length longer than 16bits. >>> + */ >>> + for_each_set_bit(bit, indio_dev->active_scan_mask, >>> + indio_dev->masklength) { >>> + unsigned int val; >>> + >>> + ret = regmap_read(chip->regmap, >>> + INA2XX_SHUNT_VOLTAGE + bit, &val); >>> + if (ret < 0) >>> + goto _err; >>> + >>> + data[i++] = val; >>> + } >>> + >>> + time_b = iio_get_time_ns(); >>> + >>> + iio_push_to_buffers_with_timestamp(indio_dev, >>> + (unsigned int *)data, time_a); >>> + >>> + buffer_us = (unsigned long)(time_b - time_a) / 1000; >>> + elapsed_us = (unsigned long)(time_a - prev_ns) / 1000; >>> + >>> + trace_printk("uS: elapsed: %lu, buf: %lu\n", elapsed_us, >>> buffer_us); >>> + >>> + prev_ns = time_a; >>> + >>> +_err: >>> + return buffer_us; >>> +}; >>> + >>> +static int ina2xx_capture_thread(void *data) >>> +{ >>> + struct iio_dev *indio_dev = (struct iio_dev *)data; >>> + struct ina2xx_chip_info *chip = iio_priv(indio_dev); >>> + unsigned int sampling_us = (chip->itb + chip->its) * chip->avg; >>> + unsigned long buffer_us; >>> + >>> + /* >>> + * Poll a bit faster than the chip internal Fs, in case >>> + * we wish to sync with the conversion ready flag. >>> + */ >>> + sampling_us -= 200; >>> + >>> + do { >>> + buffer_us = ina2xx_work_buffer(indio_dev); >>> + >>> + if (sampling_us > buffer_us) >>> + udelay(sampling_us - buffer_us); >>> + >>> + } while (!kthread_should_stop()); >>> + >>> + return 0; >>> +} >>> + >>> +int ina2xx_buffer_enable(struct iio_dev *indio_dev) >>> +{ >>> + struct ina2xx_chip_info *chip = iio_priv(indio_dev); >>> + unsigned int sampling_us = (chip->itb + chip->its) * chip->avg; >>> + >>> + trace_printk("Enabling buffer w/ scan_mask %02x, freq = %d, avg >>> =%u\n", >>> + (unsigned int)(*indio_dev->active_scan_mask), >>> + 1000000/sampling_us, chip->avg); >>> + >>> + trace_printk("Expected work period: %u us\n", sampling_us); >>> + >>> + prev_ns = iio_get_time_ns(); >>> + >>> + chip->task = kthread_run(ina2xx_capture_thread, (void *)indio_dev, >>> + "ina2xx-%uus", sampling_us); >>> + >>> + return PTR_ERR_OR_ZERO(chip->task); >>> +} >>> + >>> +int ina2xx_buffer_disable(struct iio_dev *indio_dev) >>> +{ >>> + struct ina2xx_chip_info *chip = iio_priv(indio_dev); >>> + >>> + if (chip->task) { >>> + kthread_stop(chip->task); >>> + chip->task = NULL; >>> + } >> nitpick: blank line here. >>> + return 0; >>> +} >>> + >>> +static const struct iio_buffer_setup_ops ina2xx_setup_ops = { >>> + .postenable = &ina2xx_buffer_enable, >>> + .postdisable = &ina2xx_buffer_disable, >>> +}; >>> + >>> +static int ina2xx_debug_reg(struct iio_dev *indio_dev, >>> + unsigned reg, unsigned writeval, unsigned *readval) >>> +{ >>> + struct ina2xx_chip_info *chip = iio_priv(indio_dev); >>> + >>> + if (!readval) >>> + return regmap_write(chip->regmap, reg, writeval); >>> + >>> + return regmap_read(chip->regmap, reg, readval); >>> +} >>> + >>> +/* frequencies matching the cummulated integration times for vshunt >>> and vbus */ >> huh? Integration time is in seconds... These seem unlikely to be valid >> settings. >>> +static IIO_CONST_ATTR_INT_TIME_AVAIL("140 204 332 588 1100 2116 4156 >>> 8244"); >>> + >>> +static struct attribute *ina2xx_attributes[] = { >>> + &iio_const_attr_integration_time_available.dev_attr.attr, >>> + NULL, >>> +}; >>> + >>> +static const struct attribute_group ina2xx_attribute_group = { >>> + .attrs = ina2xx_attributes, >>> +}; >>> + >>> +static const struct iio_info ina2xx_info = { >>> + .debugfs_reg_access = &ina2xx_debug_reg, >>> + .read_raw = &ina2xx_read_raw, >>> + .write_raw = &ina2xx_write_raw, >>> + .attrs = &ina2xx_attribute_group, >>> + .driver_module = THIS_MODULE, >>> +}; >>> + >>> +/* Initialize the configuration and calibration registers. */ >>> +static int ina2xx_init(struct ina2xx_chip_info *chip, unsigned int >>> config) >>> +{ >>> + u16 regval; >>> + int ret = regmap_write(chip->regmap, INA2XX_CONFIG, config); >>> + >>> + if (ret < 0) >>> + return ret; >>> + /* >>> + * Set current LSB to 1mA, shunt is in uOhms >>> + * (equation 13 in datasheet). We hardcode a Current_LSB >>> + * of 1.0 x10-6. The only remaining parameter is RShunt >>> + * There is no need to expose the CALIBRATION register >>> + * to the user for now. >>> + */ >>> + regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor, >>> + chip->rshunt); >>> + >>> + return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval); >>> +} >>> + >>> +static int ina2xx_probe(struct i2c_client *client, >>> + const struct i2c_device_id *id) >>> +{ >>> + struct ina2xx_chip_info *chip; >>> + struct iio_dev *indio_dev; >>> + struct iio_buffer *buffer; >>> + int ret; >>> + unsigned int val; >>> + >>> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip)); >>> + if (!indio_dev) >>> + return -ENOMEM; >>> + >>> + chip = iio_priv(indio_dev); >>> + >>> + chip->config = &ina2xx_config[id->driver_data]; >>> + >>> + if (of_property_read_u32(client->dev.of_node, >>> + "shunt-resistor", &val) < 0) { >>> + struct ina2xx_platform_data *pdata >>> + dev_get_platdata(&client->dev); >>> + >>> + if (pdata) >>> + val = pdata->shunt_uohms; >>> + else >>> + val = INA2XX_RSHUNT_DEFAULT; >>> + } >>> + >>> + if (val <= 0 || val > chip->config->calibration_factor) >>> + return -ENODEV; >>> + >>> + chip->rshunt = val; >>> + >>> + ina2xx_regmap_config.max_register = chip->config->registers; >>> + >>> + mutex_init(&chip->state_lock); >>> + >>> + /* this is only used for device removal purposes */ >>> + i2c_set_clientdata(client, indio_dev); >>> + >>> + indio_dev->name = id->name; >>> + indio_dev->channels = ina2xx_channels; >>> + indio_dev->num_channels = ARRAY_SIZE(ina2xx_channels); >>> + >>> + indio_dev->dev.parent = &client->dev; >>> + indio_dev->info = &ina2xx_info; >>> + indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE; >>> + >>> + chip->regmap = devm_regmap_init_i2c(client, &ina2xx_regmap_config); >>> + if (IS_ERR(chip->regmap)) { >>> + dev_err(&client->dev, "failed to allocate register map\n"); >>> + return PTR_ERR(chip->regmap); >>> + } >>> + >>> + /* Patch the current config register with default. */ >>> + val = chip->config->config_default; >>> + >>> + if (id->driver_data = ina226) { >>> + ina226_set_average(chip, INA226_DEFAULT_AVG, &val); >>> + ina226_set_itb(chip, INA226_DEFAULT_IT, &val); >>> + ina226_set_its(chip, INA226_DEFAULT_IT, &val); >>> + } >>> + >>> + ret = ina2xx_init(chip, val); >>> + if (ret < 0) { >>> + dev_err(&client->dev, "error configuring the device: %d\n", >>> + ret); >>> + return -ENODEV; >>> + } >>> + >>> + buffer = devm_iio_kfifo_allocate(&indio_dev->dev); >>> + if (!buffer) >>> + return -ENOMEM; >>> + >>> + indio_dev->setup_ops = &ina2xx_setup_ops; >>> + >>> + iio_device_attach_buffer(indio_dev, buffer); >>> + >>> + return devm_iio_device_register(&client->dev, indio_dev); >>> +} >>> + >>> + >>> +static int ina2xx_remove(struct i2c_client *client) >>> +{ >>> + struct iio_dev *indio_dev = i2c_get_clientdata(client); >>> + struct ina2xx_chip_info *chip = iio_priv(indio_dev); >>> + int ret; >>> + >>> + mutex_destroy(&chip->state_lock); >>> + >>> + /* Powerdown */ >>> + ret = regmap_update_bits(chip->regmap, INA2XX_CONFIG, >>> + INA2XX_MODE_MASK, 0); >>> + >>> + iio_device_unregister(indio_dev); >> Peter already covered this to a degree. >> >> iio_device_unregister removes the userspace interface - hence it >> must be the first thing done in remove. >> >> If you use devm_ version of register it will occur at the end of the >> remove. Hence if the remove has anything in it, you pretty much >> can't use the devm_iio_device_register without introducing a nasty >> race condition. Hence you don't want the devm version here and you >> want to move iio_device_unregister to the beginning of this function. >>> + >>> + return ret; >>> +} >>> + >>> + >>> +static const struct i2c_device_id ina2xx_id[] = { >>> + {"ina219", ina219}, >>> + {"ina220", ina219}, >>> + {"ina226", ina226}, >>> + {"ina230", ina226}, >>> + {"ina231", ina226}, >>> + {} >>> +}; >>> + >>> +MODULE_DEVICE_TABLE(i2c, ina2xx_id); >>> + >>> +static struct i2c_driver ina2xx_driver = { >>> + .driver = { >>> + .name = KBUILD_MODNAME, >>> + }, >>> + .probe = ina2xx_probe, >>> + .remove = ina2xx_remove, >>> + .id_table = ina2xx_id, >>> +}; >>> + >>> +module_i2c_driver(ina2xx_driver); >>> + >>> +MODULE_AUTHOR("Marc Titinger <marc.titinger@baylibre.com>"); >>> +MODULE_DESCRIPTION("Texas Instruments INA2XX ADC driver"); >>> +MODULE_LICENSE("GPL v2"); >>> >> >> > _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] iio: ina2xx: add support for TI INA2xx Power Monitors 2015-12-01 10:01 ` Marc Titinger @ 2015-12-01 15:02 ` Guenter Roeck 0 siblings, 0 replies; 5+ messages in thread From: Guenter Roeck @ 2015-12-01 15:02 UTC (permalink / raw) To: Marc Titinger, Jonathan Cameron, knaack.h, lars, pmeerw, jdelvare Cc: linux-iio, linux-kernel, lm-sensors On 12/01/2015 02:01 AM, Marc Titinger wrote: > On 29/11/2015 18:38, Guenter Roeck wrote: >> On 11/29/2015 08:31 AM, Jonathan Cameron wrote: >>> On 25/11/15 11:28, Marc Titinger wrote: >>>> in SOFTWARE buffer mode, a kthread will capture the active scan_elements >>>> into a kfifo, then compute the remaining time until the next capture >>>> tick >>>> and do an active wait (udelay). >>>> >>>> This will produce a stream of up to fours channels plus a 64bits >>>> timestamps (ns). >>>> >>>> Tested with ina226, on BeagleBoneBlack. >>>> >>>> Datasheet: http://www.ti.com/lit/gpn/ina226 >>>> >>>> Signed-off-by: Marc Titinger <mtitinger@baylibre.com> >>> A few more bits from me, but basically looking good. >>> >>> We do however, need to make the hwmon guys aware this is going on. >>> Please cc their list on the next version. >>> >>> Last thing we want is for this to turn up as a surprise! >>> >> >> I have seen the original patch, so no surprise here. Just not sure >> if we should remove the hwmon driver after the iio driver is accepted. >> Even though the stated goal is different, it seems to me that having >> two drivers really does not make sense. Any thoughts ? > > Hi Guenter, > > we for instance have two completely unrelated projects both using this chip, on one side we wish to measure power consumption on the host board and hwmon has the apis and tool ecosystem for it, while on the "ACME" system we needed IIO because it's buffer streaming scheme and remote features saves the day to create a lab instrument. But really both have their use, I'd recommend keeping the hwmon driver for now. I doubt those drivers will generate much maintenance work in the future either. > Marc, You can use the iio-hwmon bridge to get the hwmon ABI from the iio driver. There is no need to keep the hwmon driver as a separate entity for that. Guenter _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply [flat|nested] 5+ messages in thread
[parent not found: <1448450896-24387-3-git-send-email-mtitinger@baylibre.com>]
* Re: [lm-sensors] [PATCH 2/2] iio: ina2xx: provide a sysfs parameter to allow async readout of the AD [not found] ` <1448450896-24387-3-git-send-email-mtitinger@baylibre.com> @ 2015-11-29 16:33 ` Jonathan Cameron 0 siblings, 0 replies; 5+ messages in thread From: Jonathan Cameron @ 2015-11-29 16:33 UTC (permalink / raw) To: Marc Titinger, knaack.h, lars, pmeerw, linux, jdelvare Cc: linux-iio, linux-kernel, lm-sensors On 25/11/15 11:28, Marc Titinger wrote: > This can lead to repeated or skipped samples depending on the clock beat > between the capture thread and the chip sampling clock, but will also spare > reading/waiting for the Capture Ready Flag and improve the available i2c > bandwidth for reading measurements. > > Output of iio_info: > ...snip... > 4 device-specific attributes found: > attr 0: in_oversampling_ratio value: 4 > attr 1: in_allow_async_readout value: 0 > attr 2: integration_time_available value: 140 204 332 588 1100 2116 4156 8244 > attr 3: in_sampling_frequency value: 114 > This is a good compromise if it is needed to get the rates as high as you need. One suggestion inline. Jonathan > Signed-off-by: Marc Titinger <mtitinger@baylibre.com> > --- > drivers/iio/adc/ina2xx-iio.c | 48 ++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 42 insertions(+), 6 deletions(-) > > diff --git a/drivers/iio/adc/ina2xx-iio.c b/drivers/iio/adc/ina2xx-iio.c > index 4a0026c..c153164 100644 > --- a/drivers/iio/adc/ina2xx-iio.c > +++ b/drivers/iio/adc/ina2xx-iio.c > @@ -114,6 +114,7 @@ struct ina2xx_chip_info { > int avg; > int itb; /* Bus voltage integration time uS */ > int its; /* Shunt voltage integration tim uS */ > + bool allow_async_readout; > }; > > static const struct ina2xx_config ina2xx_config[] = { > @@ -335,6 +336,33 @@ _err: > } > > > +static ssize_t ina2xx_allow_async_readout_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev)); > + > + return sprintf(buf, "%d\n", chip->allow_async_readout); > +} > + > +static ssize_t ina2xx_allow_async_readout_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t len) > +{ > + struct ina2xx_chip_info *chip = iio_priv(dev_to_iio_dev(dev)); > + unsigned long val; > + int ret; > + strtobool would simplify this. > + ret = kstrtoul((const char *) buf, 10, &val); > + if (ret) > + return -EINVAL; > + > + chip->allow_async_readout = !!val; > + > + return len; > +} > + > + > #define INA2XX_CHAN(_type, _index, _address) { \ > .type = _type, \ > .address = _address, \ > @@ -402,11 +430,12 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev) > * GPIO a triggered buffer could be used instead. > * For now, we pay for that extra read of the ALERT register > */ > - do { > - ret = regmap_read(chip->regmap, INA226_ALERT_MASK, > - &alert); > - if (ret < 0) > - goto _err; > + if (!chip->allow_async_readout) > + do { > + ret = regmap_read(chip->regmap, INA226_ALERT_MASK, > + &alert); > + if (ret < 0) > + goto _err; > > alert &= INA266_CVRF; > trace_printk("Conversion ready: %d\n", !!alert); > @@ -457,7 +486,8 @@ static int ina2xx_capture_thread(void *data) > * Poll a bit faster than the chip internal Fs, in case > * we wish to sync with the conversion ready flag. > */ > - sampling_us -= 200; > + if (!chip->allow_async_readout) > + sampling_us -= 200; > > do { > buffer_us = ina2xx_work_buffer(indio_dev); > @@ -480,6 +510,7 @@ int ina2xx_buffer_enable(struct iio_dev *indio_dev) > 1000000/sampling_us, chip->avg); > > trace_printk("Expected work period: %u us\n", sampling_us); > + trace_printk("Async readout mode: %d\n", chip->allow_async_readout); > > prev_ns = iio_get_time_ns(); > > @@ -519,7 +550,12 @@ static int ina2xx_debug_reg(struct iio_dev *indio_dev, > /* frequencies matching the cummulated integration times for vshunt and vbus */ > static IIO_CONST_ATTR_INT_TIME_AVAIL("140 204 332 588 1100 2116 4156 8244"); > > +static IIO_DEVICE_ATTR(in_allow_async_readout, S_IRUGO | S_IWUSR, > + ina2xx_allow_async_readout_show, > + ina2xx_allow_async_readout_store, 0); > + > static struct attribute *ina2xx_attributes[] = { > + &iio_dev_attr_in_allow_async_readout.dev_attr.attr, > &iio_const_attr_integration_time_available.dev_attr.attr, > NULL, > }; > _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-12-01 15:02 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1448450896-24387-1-git-send-email-mtitinger@baylibre.com>
[not found] ` <1448450896-24387-2-git-send-email-mtitinger@baylibre.com>
2015-11-29 16:31 ` [lm-sensors] [PATCH 1/2] iio: ina2xx: add support for TI INA2xx Power Monitors Jonathan Cameron
2015-11-29 17:38 ` Guenter Roeck
2015-12-01 10:01 ` Marc Titinger
2015-12-01 15:02 ` Guenter Roeck
[not found] ` <1448450896-24387-3-git-send-email-mtitinger@baylibre.com>
2015-11-29 16:33 ` [lm-sensors] [PATCH 2/2] iio: ina2xx: provide a sysfs parameter to allow async readout of the AD Jonathan Cameron
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox