Re: [PATCH] IIO: ADC: New driver for AD7190/AD7192/AD7195 4 Channel SPI ADC

[Jonathan Cameron <jic23@cam.ac.uk>]

On 08/12/11 13:41, michael.hennerich@analog.com wrote:
> From: Michael Hennerich <michael.hennerich@analog.com>
> 
> New driver for AD7190/AD7192/AD7195 4.8 kHz, Ultralow Noise, 24-Bit
> Sigma-Delta ADC with PGA
> 
> These devices features a dual use data out ready DOUT/RDY output.
> In order to avoid contentions on the SPI bus, it's necessary to use
> spi bus locking. The DOUT/RDY output must also be wired to an
> interrupt capable GPIO.
> 
> In INDIO_RING_TRIGGERED mode, this driver may block its SPI bus segment
> for an extended period of time.

Couple of nit picks and some missing docs.  Otherwise looks good.
> 
> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> ---
>  drivers/staging/iio/adc/Kconfig  |   14 +
>  drivers/staging/iio/adc/Makefile |    1 +
>  drivers/staging/iio/adc/ad7192.c | 1178 ++++++++++++++++++++++++++++++++++++++
>  drivers/staging/iio/adc/ad7192.h |   47 ++
>  4 files changed, 1240 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/staging/iio/adc/ad7192.c
>  create mode 100644 drivers/staging/iio/adc/ad7192.h
> 
> diff --git a/drivers/staging/iio/adc/Kconfig b/drivers/staging/iio/adc/Kconfig
> index b39f2e1..975a9a5 100644
> --- a/drivers/staging/iio/adc/Kconfig
> +++ b/drivers/staging/iio/adc/Kconfig
> @@ -161,6 +161,20 @@ config AD7816
>  	  Say yes here to build support for Analog Devices AD7816/7/8
>  	  temperature sensors and ADC.
>  
> +config AD7192
> +	tristate "Analog Devices AD7190 AD7192 AD7195 ADC driver"
> +	depends on SPI
> +	select IIO_RING_BUFFER
> +	select IIO_SW_RING
> +	select IIO_TRIGGER
> +	help
> +	  Say yes here to build support for Analog Devices AD7190,
> +	  AD7192 or AD7195 SPI analog to digital convertors (ADC).
> +	  If unsure, say N (but it's safe to say "Y").
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called ad7192.
> +
>  config ADT75
>  	tristate "Analog Devices ADT75 temperature sensor driver"
>  	depends on I2C
> diff --git a/drivers/staging/iio/adc/Makefile b/drivers/staging/iio/adc/Makefile
> index f020351..b72dcd9 100644
> --- a/drivers/staging/iio/adc/Makefile
> +++ b/drivers/staging/iio/adc/Makefile
> @@ -37,6 +37,7 @@ obj-$(CONFIG_AD7745) += ad7745.o
>  obj-$(CONFIG_AD7780) += ad7780.o
>  obj-$(CONFIG_AD7793) += ad7793.o
>  obj-$(CONFIG_AD7816) += ad7816.o
> +obj-$(CONFIG_AD7192) += ad7192.o
>  obj-$(CONFIG_ADT75) += adt75.o
>  obj-$(CONFIG_ADT7310) += adt7310.o
>  obj-$(CONFIG_ADT7410) += adt7410.o
> diff --git a/drivers/staging/iio/adc/ad7192.c b/drivers/staging/iio/adc/ad7192.c
> new file mode 100644
> index 0000000..e7e53d2
> --- /dev/null
> +++ b/drivers/staging/iio/adc/ad7192.c
> @@ -0,0 +1,1178 @@
> +/*
> + * AD7190 AD7192 AD7195 SPI ADC driver
> + *
> + * Copyright 2011 Analog Devices Inc.
> + *
> + * Licensed under the GPL-2.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>
> +#include <linux/spi/spi.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/err.h>
> +#include <linux/sched.h>
> +#include <linux/delay.h>
> +
> +#include "../iio.h"
> +#include "../sysfs.h"
> +#include "../ring_generic.h"
> +#include "../ring_sw.h"
> +#include "../trigger.h"
> +#include "adc.h"
Going... and unused anyway.
> +
> +#include "ad7192.h"
> +
> +/* Registers */
> +#define AD7192_REG_COMM		0 /* Communications Register (WO, 8-bit) */
> +#define AD7192_REG_STAT		0 /* Status Register	     (RO, 8-bit) */
> +#define AD7192_REG_MODE		1 /* Mode Register	     (RW, 24-bit */
> +#define AD7192_REG_CONF		2 /* Configuration Register  (RW, 24-bit) */
> +#define AD7192_REG_DATA		3 /* Data Register	     (RO, 24/32-bit) */
> +#define AD7192_REG_ID		4 /* ID Register	     (RO, 8-bit) */
> +#define AD7192_REG_GPOCON	5 /* GPOCON Register	     (RO, 8-bit) */
> +#define AD7192_REG_OFFSET	6 /* Offset Register	     (RW, 16-bit
> +				   * (AD7792)/24-bit (AD7192)) */
> +#define AD7192_REG_FULLSALE	7 /* Full-Scale Register
> +				   * (RW, 16-bit (AD7792)/24-bit (AD7192)) */
> +
> +/* Communications Register Bit Designations (AD7192_REG_COMM) */
> +#define AD7192_COMM_WEN		(1 << 7) /* Write Enable */
> +#define AD7192_COMM_WRITE	(0 << 6) /* Write Operation */
> +#define AD7192_COMM_READ	(1 << 6) /* Read Operation */
> +#define AD7192_COMM_ADDR(x)	(((x) & 0x7) << 3) /* Register Address */
> +#define AD7192_COMM_CREAD	(1 << 2) /* Continuous Read of Data Register */
> +
> +/* Status Register Bit Designations (AD7192_REG_STAT) */
> +#define AD7192_STAT_RDY		(1 << 7) /* Ready */
> +#define AD7192_STAT_ERR		(1 << 6) /* Error (Overrange, Underrange) */
> +#define AD7192_STAT_NOREF	(1 << 5) /* Error no external reference */
> +#define AD7192_STAT_PARITY	(1 << 4) /* Parity */
> +#define AD7192_STAT_CH3		(1 << 2) /* Channel 3 */
> +#define AD7192_STAT_CH2		(1 << 1) /* Channel 2 */
> +#define AD7192_STAT_CH1		(1 << 0) /* Channel 1 */
> +
> +/* Mode Register Bit Designations (AD7192_REG_MODE) */
> +#define AD7192_MODE_SEL(x)	(((x) & 0x7) << 21) /* Operation Mode Select */
> +#define AD7192_MODE_DAT_STA	(1 << 20) /* Status Register transmission */
> +#define AD7192_MODE_CLKSRC(x)	(((x) & 0x3) << 18) /* Clock Source Select */
> +#define AD7192_MODE_SINC3	(1 << 15) /* SINC3 Filter Select */
> +#define AD7192_MODE_ACX		(1 << 14) /* AC excitation enable(AD7195 only)*/
> +#define AD7192_MODE_ENPAR	(1 << 13) /* Parity Enable */
> +#define AD7192_MODE_CLKDIV	(1 << 12) /* Clock divide by 2 (AD7190/2 only)*/
> +#define AD7192_MODE_SCYCLE	(1 << 11) /* Single cycle conversion */
> +#define AD7192_MODE_REJ60	(1 << 10) /* 50/60Hz notch filter */
> +#define AD7192_MODE_RATE(x)	((x) & 0x3FF) /* Filter Update Rate Select */
> +
> +/* Mode Register: AD7192_MODE_SEL options */
> +#define AD7192_MODE_CONT		0 /* Continuous Conversion Mode */
> +#define AD7192_MODE_SINGLE		1 /* Single Conversion Mode */
> +#define AD7192_MODE_IDLE		2 /* Idle Mode */
> +#define AD7192_MODE_PWRDN		3 /* Power-Down Mode */
> +#define AD7192_MODE_CAL_INT_ZERO	4 /* Internal Zero-Scale Calibration */
> +#define AD7192_MODE_CAL_INT_FULL	5 /* Internal Full-Scale Calibration */
> +#define AD7192_MODE_CAL_SYS_ZERO	6 /* System Zero-Scale Calibration */
> +#define AD7192_MODE_CAL_SYS_FULL	7 /* System Full-Scale Calibration */
> +
> +/* Mode Register: AD7192_MODE_CLKSRC options */
> +#define AD7192_CLK_EXT_MCLK1_2		0 /* External 4.92 MHz Clock connected
> +					   * from MCLK1 to MCLK2 */
> +#define AD7192_CLK_EXT_MCLK2		1 /* External Clock applied to MCLK2 */
> +#define AD7192_CLK_INT			2 /* Internal 4.92 MHz Clock not
> +					   * available at the MCLK2 pin */
> +#define AD7192_CLK_INT_CO		3 /* Internal 4.92 MHz Clock available
> +					   * at the MCLK2 pin */
> +
> +
> +/* Configuration Register Bit Designations (AD7192_REG_CONF) */
> +
> +#define AD7192_CONF_CHOP	(1 << 23) /* CHOP enable */
> +#define AD7192_CONF_REFSEL	(1 << 20) /* REFIN1/REFIN2 Reference Select */
> +#define AD7192_CONF_CHAN(x)	(((x) & 0xFF) << 8) /* Channel select */
> +#define AD7192_CONF_BURN	(1 << 7) /* Burnout current enable */
> +#define AD7192_CONF_REFDET	(1 << 6) /* Reference detect enable */
> +#define AD7192_CONF_BUF		(1 << 4) /* Buffered Mode Enable */
> +#define AD7192_CONF_UNIPOLAR	(1 << 3) /* Unipolar/Bipolar Enable */
> +#define AD7192_CONF_GAIN(x)	((x) & 0x7) /* Gain Select */
> +
> +#define AD7192_CH_AIN1P_AIN2M	0 /* AIN1(+) - AIN2(-) */
> +#define AD7192_CH_AIN3P_AIN4M	1 /* AIN3(+) - AIN4(-) */
> +#define AD7192_CH_TEMP		2 /* Temp Sensor */
> +#define AD7192_CH_AIN2P_AIN2M	3 /* AIN2(+) - AIN2(-) */
> +#define AD7192_CH_AIN1		4 /* AIN1 - AINCOM */
> +#define AD7192_CH_AIN2		5 /* AIN2 - AINCOM */
> +#define AD7192_CH_AIN3		6 /* AIN3 - AINCOM */
> +#define AD7192_CH_AIN4		7 /* AIN4 - AINCOM */
> +
> +/* ID Register Bit Designations (AD7192_REG_ID) */
> +#define ID_AD7190		0x4
> +#define ID_AD7192		0x0
> +#define ID_AD7195		0x6
> +#define AD7192_ID_MASK		0x0F
> +
> +/* GPOCON Register Bit Designations (AD7192_REG_GPOCON) */
> +#define AD7192_GPOCON_BPDSW	(1 << 6) /* Bridge power-down switch enable */
> +#define AD7192_GPOCON_GP32EN	(1 << 5) /* Digital Output P3 and P2 enable */
> +#define AD7192_GPOCON_GP10EN	(1 << 4) /* Digital Output P1 and P0 enable */
> +#define AD7192_GPOCON_P3DAT	(1 << 3) /* P3 state */
> +#define AD7192_GPOCON_P2DAT	(1 << 2) /* P2 state */
> +#define AD7192_GPOCON_P1DAT	(1 << 1) /* P1 state */
> +#define AD7192_GPOCON_P0DAT	(1 << 0) /* P0 state */
> +
> +#define AD7192_INT_FREQ_MHz	4915200
> +
> +/* NOTE:
> + * The AD7190/2/5sold features a dual use data out ready DOUT/RDY output.
sold?
> + * In order to avoid contentions on the SPI bus, it's therefore necessary
> + * to use spi bus locking.
> + *
> + * The DOUT/RDY output must also be wired to an interrupt capable GPIO.
> + */
> +
> +struct ad7192_state {
> +	struct spi_device		*spi;
> +	struct iio_trigger		*trig;
> +	struct regulator		*reg;
> +	struct ad7192_platform_data	*pdata;
> +	wait_queue_head_t		wq_data_avail;
> +	bool				done;
> +	bool				irq_dis;
> +	u16				int_vref_mv;
> +	u32				mclk;
> +	u32				f_order;
> +	u32				mode;
> +	u32				conf;
> +	u32				scale_avail[8][2];
> +	u32				available_scan_masks[9];
> +	u8				gpocon;
> +	u8				devid;
> +	/*
> +	 * DMA (thus cache coherency maintenance) requires the
> +	 * transfer buffers to live in their own cache lines.
> +	 */
I may be missing something but this seems somewhat large.
> +	u8				data[32] ____cacheline_aligned;
> +};
> +
> +static int __ad7192_write_reg(struct ad7192_state *st, bool locked,
> +			      bool cs_change, unsigned char reg,
> +			      unsigned size, unsigned val)
> +{
> +	u8 *data = st->data;
> +	struct spi_transfer t = {
> +		.tx_buf		= data,
> +		.len		= size + 1,
> +		.cs_change	= cs_change,
> +	};
> +	struct spi_message m;
> +
> +	data[0] = AD7192_COMM_WRITE | AD7192_COMM_ADDR(reg);
> +
> +	switch (size) {
> +	case 3:
> +		data[1] = val >> 16;
> +		data[2] = val >> 8;
> +		data[3] = val;
> +		break;
> +	case 2:
> +		data[1] = val >> 8;
> +		data[2] = val;
> +		break;
> +	case 1:
> +		data[1] = val;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	spi_message_init(&m);
> +	spi_message_add_tail(&t, &m);
> +
> +	if (locked)
> +		return spi_sync_locked(st->spi, &m);
> +	else
> +		return spi_sync(st->spi, &m);
> +}
> +
> +static int ad7192_write_reg(struct ad7192_state *st,
> +			    unsigned reg, unsigned size, unsigned val)
> +{
> +	return __ad7192_write_reg(st, false, false, reg, size, val);
> +}
> +
> +static int __ad7192_read_reg(struct ad7192_state *st, bool locked,
> +			     bool cs_change, unsigned char reg,
> +			     int *val, unsigned size)
> +{
> +	u8 *data = st->data;
> +	int ret;
> +	struct spi_transfer t[] = {
> +		{
> +			.tx_buf = data,
> +			.len = 1,
> +		}, {
> +			.rx_buf = data,
> +			.len = size,
> +			.cs_change = cs_change,
> +		},
> +	};
> +	struct spi_message m;
> +
> +	data[0] = AD7192_COMM_READ | AD7192_COMM_ADDR(reg);
> +
> +	spi_message_init(&m);
> +	spi_message_add_tail(&t[0], &m);
> +	spi_message_add_tail(&t[1], &m);
> +
> +	if (locked)
> +		ret = spi_sync_locked(st->spi, &m);
> +	else
> +		ret = spi_sync(st->spi, &m);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (size) {
> +	case 3:
> +		*val = data[0] << 16 | data[1] << 8 | data[2];
> +		break;
> +	case 2:
> +		*val = data[0] << 8 | data[1];
> +		break;
> +	case 1:
> +		*val = data[0];
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ad7192_read_reg(struct ad7192_state *st,
> +			   unsigned reg, int *val, unsigned size)
> +{
> +	return __ad7192_read_reg(st, 0, 0, reg, val, size);
> +}
> +
> +static int ad7192_read(struct ad7192_state *st, unsigned ch,
> +		       unsigned len, int *val)
> +{
> +	int ret;
> +	st->conf = (st->conf & ~AD7192_CONF_CHAN(-1)) |
> +		AD7192_CONF_CHAN(1 << ch);
> +	st->mode = (st->mode & ~AD7192_MODE_SEL(-1)) |
> +		AD7192_MODE_SEL(AD7192_MODE_SINGLE);
> +
> +	ad7192_write_reg(st, AD7192_REG_CONF, 3, st->conf);
> +
> +	spi_bus_lock(st->spi->master);
> +	st->done = false;
> +
> +	ret = __ad7192_write_reg(st, 1, 1, AD7192_REG_MODE, 3, st->mode);
> +	if (ret < 0)
> +		goto out;
> +
> +	st->irq_dis = false;
> +	enable_irq(st->spi->irq);
> +	wait_event_interruptible(st->wq_data_avail, st->done);
> +
> +	ret = __ad7192_read_reg(st, 1, 0, AD7192_REG_DATA, val, len);
> +out:
> +	spi_bus_unlock(st->spi->master);
> +
> +	return ret;
> +}
> +
> +static int ad7192_calibrate(struct ad7192_state *st, unsigned mode, unsigned ch)
> +{
> +	int ret;
> +
> +	st->conf = (st->conf & ~AD7192_CONF_CHAN(-1)) |
> +		AD7192_CONF_CHAN(1 << ch);
> +	st->mode = (st->mode & ~AD7192_MODE_SEL(-1)) | AD7192_MODE_SEL(mode);
> +
> +	ad7192_write_reg(st, AD7192_REG_CONF, 3, st->conf);
> +
> +	spi_bus_lock(st->spi->master);
> +	st->done = false;
> +
> +	ret = __ad7192_write_reg(st, 1, 1, AD7192_REG_MODE, 3,
> +				 (st->devid != ID_AD7195) ?
> +				 st->mode | AD7192_MODE_CLKDIV :
> +				 st->mode);
> +	if (ret < 0)
> +		goto out;
> +
> +	st->irq_dis = false;
> +	enable_irq(st->spi->irq);
> +	wait_event_interruptible(st->wq_data_avail, st->done);
> +
> +	st->mode = (st->mode & ~AD7192_MODE_SEL(-1)) |
> +		AD7192_MODE_SEL(AD7192_MODE_IDLE);
> +
> +	ret = __ad7192_write_reg(st, 1, 0, AD7192_REG_MODE, 3, st->mode);
> +out:
> +	spi_bus_unlock(st->spi->master);
> +
> +	return ret;
> +}
> +
> +static const u8 ad7192_calib_arr[8][2] = {
> +	{AD7192_MODE_CAL_INT_ZERO, AD7192_CH_AIN1},
> +	{AD7192_MODE_CAL_INT_FULL, AD7192_CH_AIN1},
> +	{AD7192_MODE_CAL_INT_ZERO, AD7192_CH_AIN2},
> +	{AD7192_MODE_CAL_INT_FULL, AD7192_CH_AIN2},
> +	{AD7192_MODE_CAL_INT_ZERO, AD7192_CH_AIN3},
> +	{AD7192_MODE_CAL_INT_FULL, AD7192_CH_AIN3},
> +	{AD7192_MODE_CAL_INT_ZERO, AD7192_CH_AIN4},
> +	{AD7192_MODE_CAL_INT_FULL, AD7192_CH_AIN4}
> +};
> +
> +static int ad7192_calibrate_all(struct ad7192_state *st)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(ad7192_calib_arr); i++) {
> +		ret = ad7192_calibrate(st, ad7192_calib_arr[i][0],
> +				       ad7192_calib_arr[i][1]);
> +		if (ret)
> +			goto out;
> +	}
> +
> +	return 0;
> +out:
> +	dev_err(&st->spi->dev, "Calibration failed\n");
> +	return ret;
> +}
> +
> +static int ad7192_setup(struct ad7192_state *st)
> +{
> +	struct iio_dev *indio_dev = spi_get_drvdata(st->spi);
> +	struct ad7192_platform_data *pdata = st->pdata;
> +	unsigned long long scale_uv;
> +	int i, ret, id;
> +	u8 ones[6];
> +
> +	/* reset the serial interface */
> +	memset(&ones, 0xFF, 6);
> +	ret = spi_write(st->spi, &ones, 6);
> +	if (ret < 0)
> +		goto out;
> +	msleep(1); /* Wait for at least 500us */
> +
> +	/* write/read test for device presence */
> +	ret = ad7192_read_reg(st, AD7192_REG_ID, &id, 1);
> +	if (ret)
> +		goto out;
> +
> +	id &= AD7192_ID_MASK;
> +
> +	if (id != st->devid)
> +		dev_warn(&st->spi->dev, "device ID query failed (0x%X)\n", id);
> +
> +	switch (pdata->clock_source_sel) {
> +	case AD7192_CLK_EXT_MCLK1_2:
> +	case AD7192_CLK_EXT_MCLK2:
> +		st->mclk = AD7192_INT_FREQ_MHz;
> +		break;
> +	case AD7192_CLK_INT:
> +	case AD7192_CLK_INT_CO:
> +		if (pdata->ext_clk_Hz)
> +			st->mclk = pdata->ext_clk_Hz;
> +		else
> +			st->mclk = AD7192_INT_FREQ_MHz;
> +			break;
> +	default:
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	st->mode = AD7192_MODE_SEL(AD7192_MODE_IDLE) |
> +		AD7192_MODE_CLKSRC(pdata->clock_source_sel) |
> +		AD7192_MODE_RATE(480);
> +
> +	st->conf = AD7192_CONF_GAIN(0);
> +
> +	if (pdata->rej60_en)
> +		st->mode |= AD7192_MODE_REJ60;
> +
> +	if (pdata->sinc3_en)
> +		st->mode |= AD7192_MODE_SINC3;
> +
> +	if (pdata->refin2_en && (st->devid != ID_AD7195))
> +		st->conf |= AD7192_CONF_REFSEL;
> +
> +	if (pdata->chop_en) {
> +		st->conf |= AD7192_CONF_CHOP;
> +		if (pdata->sinc3_en)
> +			st->f_order = 3; /* SINC 3rd order */
> +		else
Inconsistent spacing in comments.
> +			st->f_order = 4; /*SINC 4th order*/
> +	} else {
> +		st->f_order = 1;
> +	}
> +
> +	if (pdata->buf_en)
> +		st->conf |= AD7192_CONF_BUF;
> +
> +	if (pdata->unipolar_en)
> +		st->conf |= AD7192_CONF_UNIPOLAR;
> +
> +	if (pdata->burnout_curr_en)
> +		st->conf |= AD7192_CONF_BURN;
> +
> +	ret = ad7192_write_reg(st, AD7192_REG_MODE, 3, st->mode);
> +	if (ret)
> +		goto out;
> +
> +	ret = ad7192_write_reg(st, AD7192_REG_CONF, 3, st->conf);
> +	if (ret)
> +		goto out;
> +
> +	ret = ad7192_calibrate_all(st);
> +	if (ret)
> +		goto out;
> +
> +	/* Populate available ADC input ranges */
> +	for (i = 0; i < ARRAY_SIZE(st->scale_avail); i++) {
> +		scale_uv = ((u64)st->int_vref_mv * 100000000)
> +			>> (indio_dev->channels[0].scan_type.realbits -
> +			(!!(st->conf & AD7192_CONF_UNIPOLAR) ? 0 : 1));
  That ternary operator looks weird.  is that not just !(st->conf & AD7192_UNIPOLAR) ?
> +		scale_uv >>= i;
> +
> +		st->scale_avail[i][1] = do_div(scale_uv, 100000000) * 10;
> +		st->scale_avail[i][0] = scale_uv;
> +	}
> +
> +	return 0;
> +out:
> +	dev_err(&st->spi->dev, "setup failed\n");
> +	return ret;
> +}
> +
> +static int ad7192_scan_from_ring(struct ad7192_state *st, unsigned ch, int *val)
> +{
> +	struct iio_ring_buffer *ring = iio_priv_to_dev(st)->ring;
> +	int ret;
> +	s64 dat64[2];
> +	u32 *dat32 = (u32 *)dat64;
> +
> +	if (!(ring->scan_mask & (1 << ch)))
> +		return  -EBUSY;
> +
> +	ret = ring->access->read_last(ring, (u8 *) &dat64);
> +	if (ret)
> +		return ret;
> +
> +	*val = *dat32;
> +
> +	return 0;
> +}
> +
> +static int ad7192_ring_preenable(struct iio_dev *indio_dev)
> +{
> +	struct ad7192_state *st = iio_priv(indio_dev);
> +	struct iio_ring_buffer *ring = indio_dev->ring;
> +	size_t d_size;
> +	unsigned channel;
> +
> +	if (!ring->scan_count)
> +		return -EINVAL;
> +
> +	channel = __ffs(ring->scan_mask);
> +
> +	d_size = ring->scan_count *
> +		 indio_dev->channels[0].scan_type.storagebits / 8;
> +
> +	if (ring->scan_timestamp) {
> +		d_size += sizeof(s64);
> +
> +		if (d_size % sizeof(s64))
> +			d_size += sizeof(s64) - (d_size % sizeof(s64));
> +	}
> +
> +	if (indio_dev->ring->access->set_bytes_per_datum)
> +		indio_dev->ring->access->set_bytes_per_datum(indio_dev->ring,
> +							     d_size);
> +
> +	st->mode  = (st->mode & ~AD7192_MODE_SEL(-1)) |
> +		    AD7192_MODE_SEL(AD7192_MODE_CONT);
> +	st->conf  = (st->conf & ~AD7192_CONF_CHAN(-1)) |
> +		    AD7192_CONF_CHAN(1 << indio_dev->channels[channel].address);
> +
> +	ad7192_write_reg(st, AD7192_REG_CONF, 3, st->conf);
> +
> +	spi_bus_lock(st->spi->master);
> +	__ad7192_write_reg(st, 1, 1, AD7192_REG_MODE, 3, st->mode);
> +
> +	st->irq_dis = false;
> +	enable_irq(st->spi->irq);
> +
> +	return 0;
> +}
> +
> +static int ad7192_ring_postdisable(struct iio_dev *indio_dev)
> +{
> +	struct ad7192_state *st = iio_priv(indio_dev);
> +
> +	st->mode  = (st->mode & ~AD7192_MODE_SEL(-1)) |
> +		    AD7192_MODE_SEL(AD7192_MODE_IDLE);
> +
> +	st->done = false;
> +	wait_event_interruptible(st->wq_data_avail, st->done);
> +
> +	if (!st->irq_dis)
> +		disable_irq_nosync(st->spi->irq);
> +
> +	__ad7192_write_reg(st, 1, 0, AD7192_REG_MODE, 3, st->mode);
> +
> +	return spi_bus_unlock(st->spi->master);
> +}
> +
> +/**
> + * ad7192_trigger_handler() bh of trigger launched polling to ring buffer
> + **/
Unwanted blank line?
> +
> +static irqreturn_t ad7192_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->private_data;
> +	struct iio_ring_buffer *ring = indio_dev->ring;
> +	struct ad7192_state *st = iio_priv(indio_dev);
> +	s64 dat64[2];
> +	s32 *dat32 = (s32 *)dat64;
> +
> +	if (ring->scan_count)
> +		__ad7192_read_reg(st, 1, 1, AD7192_REG_DATA,
> +				  dat32,
> +				  indio_dev->channels[0].scan_type.realbits/8);
> +
> +	/* Guaranteed to be aligned with 8 byte boundary */
> +	if (ring->scan_timestamp)
> +		dat64[1] = pf->timestamp;
> +
> +	ring->access->store_to(ring, (u8 *)dat64, pf->timestamp);
> +
> +	iio_trigger_notify_done(indio_dev->trig);
> +	st->irq_dis = false;
> +	enable_irq(st->spi->irq);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static const struct iio_ring_setup_ops ad7192_ring_setup_ops = {
> +	.preenable = &ad7192_ring_preenable,
> +	.postenable = &iio_triggered_ring_postenable,
> +	.predisable = &iio_triggered_ring_predisable,
> +	.postdisable = &ad7192_ring_postdisable,
> +};
> +
> +static int ad7192_register_ring_funcs_and_init(struct iio_dev *indio_dev)
> +{
> +	int ret;
> +
> +	indio_dev->ring = iio_sw_rb_allocate(indio_dev);
> +	if (!indio_dev->ring) {
> +		ret = -ENOMEM;
> +		goto error_ret;
> +	}
> +	/* Effectively select the ring buffer implementation */
> +	indio_dev->ring->access = &ring_sw_access_funcs;
> +	indio_dev->pollfunc = iio_alloc_pollfunc(&iio_pollfunc_store_time,
> +						 &ad7192_trigger_handler,
> +						 IRQF_ONESHOT,
> +						 indio_dev,
> +						 "ad7192_consumer%d",
> +						 indio_dev->id);
> +	if (indio_dev->pollfunc == NULL) {
> +		ret = -ENOMEM;
> +		goto error_deallocate_sw_rb;
> +	}
> +
> +	/* Ring buffer functions - here trigger setup related */
> +	indio_dev->ring->setup_ops = &ad7192_ring_setup_ops;
> +
> +	/* Flag that polled ring buffering is possible */
> +	indio_dev->modes |= INDIO_RING_TRIGGERED;
> +	return 0;
> +
> +error_deallocate_sw_rb:
> +	iio_sw_rb_free(indio_dev->ring);
> +error_ret:
> +	return ret;
> +}
> +
> +static void ad7192_ring_cleanup(struct iio_dev *indio_dev)
> +{
> +	/* ensure that the trigger has been detached */
> +	if (indio_dev->trig) {
> +		iio_put_trigger(indio_dev->trig);
> +		iio_trigger_dettach_poll_func(indio_dev->trig,
> +					      indio_dev->pollfunc);
> +	}
> +	iio_dealloc_pollfunc(indio_dev->pollfunc);
> +	iio_sw_rb_free(indio_dev->ring);
> +}
> +
> +/**
> + * ad7192_data_rdy_trig_poll() the event handler for the data rdy trig
> + **/
> +static irqreturn_t ad7192_data_rdy_trig_poll(int irq, void *private)
> +{
> +	struct ad7192_state *st = iio_priv(private);
> +
> +	st->done = true;
> +	wake_up_interruptible(&st->wq_data_avail);
> +	disable_irq_nosync(irq);
> +	st->irq_dis = true;
> +	iio_trigger_poll(st->trig, iio_get_time_ns());
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int ad7192_probe_trigger(struct iio_dev *indio_dev)
> +{
> +	struct ad7192_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	st->trig = iio_allocate_trigger("%s-dev%d",
> +					spi_get_device_id(st->spi)->name,
> +					indio_dev->id);
> +	if (st->trig == NULL) {
> +		ret = -ENOMEM;
> +		goto error_ret;
> +	}
> +
> +	ret = request_irq(st->spi->irq,
> +			  ad7192_data_rdy_trig_poll,
> +			  IRQF_TRIGGER_LOW,
> +			  spi_get_device_id(st->spi)->name,
> +			  indio_dev);
> +	if (ret)
> +		goto error_free_trig;
> +
> +	disable_irq_nosync(st->spi->irq);
> +	st->irq_dis = true;
> +	st->trig->dev.parent = &st->spi->dev;
> +	st->trig->owner = THIS_MODULE;
> +	st->trig->private_data = indio_dev;
> +
> +	ret = iio_trigger_register(st->trig);
> +
> +	/* select default trigger */
> +	indio_dev->trig = st->trig;
> +	if (ret)
> +		goto error_free_irq;
> +
> +	return 0;
> +
> +error_free_irq:
> +	free_irq(st->spi->irq, indio_dev);
> +error_free_trig:
> +	iio_free_trigger(st->trig);
> +error_ret:
> +	return ret;
> +}
> +
> +static void ad7192_remove_trigger(struct iio_dev *indio_dev)
> +{
> +	struct ad7192_state *st = iio_priv(indio_dev);
> +
> +	iio_trigger_unregister(st->trig);
> +	free_irq(st->spi->irq, indio_dev);
> +	iio_free_trigger(st->trig);
> +}
> +
> +static ssize_t ad7192_read_frequency(struct device *dev,
> +		struct device_attribute *attr,
> +		char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct ad7192_state *st = iio_priv(indio_dev);
> +
> +	return sprintf(buf, "%d\n", st->mclk /
> +			(st->f_order * 1024 * AD7192_MODE_RATE(st->mode)));
> +}
> +
> +static ssize_t ad7192_write_frequency(struct device *dev,
> +		struct device_attribute *attr,
> +		const char *buf,
> +		size_t len)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct ad7192_state *st = iio_priv(indio_dev);
> +	unsigned long lval;
> +	int div, ret;
> +
> +	ret = strict_strtoul(buf, 10, &lval);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&indio_dev->mlock);
> +	if (iio_ring_enabled(indio_dev)) {
> +		mutex_unlock(&indio_dev->mlock);
> +		return -EBUSY;
> +	}
> +
> +	div = st->mclk / (lval * st->f_order * 1024);
> +	if (div < 1 || div > 1023) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	st->mode &= ~AD7192_MODE_RATE(-1);
> +	st->mode |= AD7192_MODE_RATE(div);
> +	ad7192_write_reg(st, AD7192_REG_MODE, 3, st->mode);
> +
> +out:
> +	mutex_unlock(&indio_dev->mlock);
> +
> +	return ret ? ret : len;
> +}
> +
> +static IIO_DEV_ATTR_SAMP_FREQ(S_IWUSR | S_IRUGO,
> +		ad7192_read_frequency,
> +		ad7192_write_frequency);
> +
> +
> +static ssize_t ad7192_show_scale_available(struct device *dev,
> +			struct device_attribute *attr, char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct ad7192_state *st = iio_priv(indio_dev);
> +	int i, len = 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(st->scale_avail); i++)
> +		len += sprintf(buf + len, "%d.%09u ", st->scale_avail[i][0],
> +			       st->scale_avail[i][1]);
> +
> +	len += sprintf(buf + len, "\n");
> +
> +	return len;
> +}
> +
> +static IIO_DEVICE_ATTR_NAMED(in_m_in_scale_available, in-in_scale_available,
> +			     S_IRUGO, ad7192_show_scale_available, NULL, 0);
> +
> +static IIO_DEVICE_ATTR(in_scale_available, S_IRUGO,
> +		       ad7192_show_scale_available, NULL, 0);
> +

For readability I'd just break this into two functions.

> +static ssize_t ad7192_show(struct device *dev,
> +		struct device_attribute *attr,
> +		char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct ad7192_state *st = iio_priv(indio_dev);
> +	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> +
> +	return sprintf(buf, "%d\n", (this_attr->address == AD7192_REG_GPOCON) ?
> +		!!(st->gpocon & AD7192_GPOCON_BPDSW) :
> +		!!(st->mode & AD7192_MODE_ACX));
> +}
> +
> +static ssize_t ad7192_set(struct device *dev,
> +		struct device_attribute *attr,
> +		const char *buf,
> +		size_t len)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct ad7192_state *st = iio_priv(indio_dev);
> +	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> +	int ret;
> +	bool val;
> +
> +	ret = strtobool(buf, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	mutex_lock(&indio_dev->mlock);
> +	if (iio_ring_enabled(indio_dev)) {
> +		mutex_unlock(&indio_dev->mlock);
> +		return -EBUSY;
> +	}
> +
> +	switch (this_attr->address) {
> +	case AD7192_REG_GPOCON:
> +		if (val)
> +			st->gpocon |= AD7192_GPOCON_BPDSW;
> +		else
> +			st->gpocon &= ~AD7192_GPOCON_BPDSW;
> +
> +		ad7192_write_reg(st, AD7192_REG_GPOCON, 1, st->gpocon);
> +		break;
> +	case AD7192_REG_MODE:
> +		if (val)
> +			st->mode |= AD7192_MODE_ACX;
> +		else
> +			st->mode &= ~AD7192_MODE_ACX;
> +
> +		ad7192_write_reg(st, AD7192_REG_GPOCON, 3, st->mode);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +	mutex_unlock(&indio_dev->mlock);
> +
> +	return ret ? ret : len;
> +}
> +
> +static IIO_DEVICE_ATTR(bridge_switch_en, S_IRUGO | S_IWUSR,
> +		       ad7192_show, ad7192_set,
> +		       AD7192_REG_GPOCON);
> +
> +static IIO_DEVICE_ATTR(ac_excitation_en, S_IRUGO | S_IWUSR,
> +		       ad7192_show, ad7192_set,
> +		       AD7192_REG_MODE);
> +
> +static struct attribute *ad7192_attributes[] = {
> +	&iio_dev_attr_sampling_frequency.dev_attr.attr,
> +	&iio_dev_attr_in_m_in_scale_available.dev_attr.attr,
> +	&iio_dev_attr_in_scale_available.dev_attr.attr,
> +	&iio_dev_attr_bridge_switch_en.dev_attr.attr,
Docs please.
> +	&iio_dev_attr_ac_excitation_en.dev_attr.attr,
Docs please.
> +	NULL
> +};
> +
> +static mode_t ad7192_attr_is_visible(struct kobject *kobj,
> +				     struct attribute *attr, int n)
> +{
> +	struct device *dev = container_of(kobj, struct device, kobj);
> +	struct iio_dev *dev_info = dev_get_drvdata(dev);
> +	struct ad7192_state *st = iio_priv(dev_info);
> +
> +	mode_t mode = attr->mode;
> +
> +	if ((st->devid != ID_AD7195) &&
> +		(attr == &iio_dev_attr_ac_excitation_en.dev_attr.attr))
> +		mode = 0;
> +
> +	return mode;
> +}
> +
> +static const struct attribute_group ad7192_attribute_group = {
> +	.attrs = ad7192_attributes,
> +	.is_visible = ad7192_attr_is_visible,
> +};
> +
> +static int ad7192_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val,
> +			   int *val2,
> +			   long m)
> +{
> +	struct ad7192_state *st = iio_priv(indio_dev);
> +	int ret, smpl = 0;
> +	bool unipolar = !!(st->conf & AD7192_CONF_UNIPOLAR);
> +
> +	switch (m) {
> +	case 0:
> +		mutex_lock(&indio_dev->mlock);
> +		if (iio_ring_enabled(indio_dev))
> +			ret = ad7192_scan_from_ring(st,
> +					chan->scan_index, &smpl);
> +		else
> +			ret = ad7192_read(st, chan->address,
> +					chan->scan_type.realbits / 8, &smpl);
> +		mutex_unlock(&indio_dev->mlock);
> +
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = (smpl >> chan->scan_type.shift) &
> +			((1 << (chan->scan_type.realbits)) - 1);
> +
> +		switch (chan->type) {
> +		case IIO_IN:
> +		case IIO_IN_DIFF:
> +			if (!unipolar)
> +				*val -= (1 << (chan->scan_type.realbits - 1));
> +			break;
> +		case IIO_TEMP:
> +			*val -= 0x800000;
> +			*val /= 2815; /* temp Kelvin */
> +			*val -= 273; /* temp Celsius */
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		return IIO_VAL_INT;
> +
Could in theory hit a race between reading and writing this (and get inconsistent
pair of val and val2?)
> +	case (1 << IIO_CHAN_INFO_SCALE_SHARED):
> +		*val = st->scale_avail[AD7192_CONF_GAIN(st->conf)][0];
> +		*val2 = st->scale_avail[AD7192_CONF_GAIN(st->conf)][1];
> +
> +		return IIO_VAL_INT_PLUS_NANO;
> +
> +	case (1 << IIO_CHAN_INFO_SCALE_SEPARATE):
> +		*val =  1000;
> +
> +		return IIO_VAL_INT;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int ad7192_write_raw(struct iio_dev *indio_dev,
> +			       struct iio_chan_spec const *chan,
> +			       int val,
> +			       int val2,
> +			       long mask)
> +{
> +	struct ad7192_state *st = iio_priv(indio_dev);
> +	int ret, i;
> +	unsigned int tmp;
> +
> +	mutex_lock(&indio_dev->mlock);
> +	if (iio_ring_enabled(indio_dev)) {
> +		mutex_unlock(&indio_dev->mlock);
> +		return -EBUSY;
> +	}
> +
> +	switch (mask) {
> +	case (1 << IIO_CHAN_INFO_SCALE_SHARED):
> +		ret = -EINVAL;
> +		for (i = 0; i < ARRAY_SIZE(st->scale_avail); i++)
> +			if (val2 == st->scale_avail[i][1]) {
> +				tmp = st->conf;
> +				st->conf &= ~AD7192_CONF_GAIN(-1);
> +				st->conf |= AD7192_CONF_GAIN(i);
> +
> +				if (tmp != st->conf) {
> +					ad7192_write_reg(st, AD7192_REG_CONF,
> +							 3, st->conf);
> +					ad7192_calibrate_all(st);
> +				}
> +				ret = 0;
> +			}
> +
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +	mutex_unlock(&indio_dev->mlock);
blank line here.
> +	return ret;
> +}
> +
> +static int ad7192_validate_trigger(struct iio_dev *indio_dev,
> +				   struct iio_trigger *trig)
> +{
> +	if (indio_dev->trig != trig)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int ad7192_write_raw_get_fmt(struct iio_dev *indio_dev,
> +			       struct iio_chan_spec const *chan,
> +			       long mask)
> +{
> +	return IIO_VAL_INT_PLUS_NANO;
> +}
> +
> +static const struct iio_info ad7192_info = {
> +	.read_raw = &ad7192_read_raw,
> +	.write_raw = &ad7192_write_raw,
> +	.write_raw_get_fmt = &ad7192_write_raw_get_fmt,
> +	.attrs = &ad7192_attribute_group,
> +	.validate_trigger = ad7192_validate_trigger,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +static struct iio_chan_spec ad7192_channels[] = {
> +
Peronsally I'm going more and more against this macro, so
if you are bored, please define a local one setting up
exactly those bits of the structure needed.  Its a little
more code, but a heck of a lot more readable. (basically
I was wrong to introduce this in the first place and will
not be taking it out of staging if I can help it!)

> +	IIO_CHAN(IIO_IN_DIFF, 0, 1, 0, NULL, 1, 2,
> +		    (1 << IIO_CHAN_INFO_SCALE_SHARED),
> +		    AD7192_CH_AIN1P_AIN2M,
> +		    0, IIO_ST('s', 24, 32, 0), 0),
> +	IIO_CHAN(IIO_IN_DIFF, 0, 1, 0, NULL, 3, 4,
> +		    (1 << IIO_CHAN_INFO_SCALE_SHARED),
> +		    AD7192_CH_AIN3P_AIN4M,
> +		    1, IIO_ST('s', 24, 32, 0), 0),
> +	IIO_CHAN(IIO_TEMP, 0, 1, 0, NULL, 0, 0,
> +		    (1 << IIO_CHAN_INFO_SCALE_SEPARATE),
> +		    AD7192_CH_TEMP,
> +		    2, IIO_ST('s', 24, 32, 0), 0),
> +	IIO_CHAN(IIO_IN_DIFF, 0, 1, 0, "shorted", 2, 2,
> +		    (1 << IIO_CHAN_INFO_SCALE_SHARED),
> +		    AD7192_CH_AIN2P_AIN2M,
> +		    3, IIO_ST('s', 24, 32, 0), 0),
> +	IIO_CHAN(IIO_IN, 0, 1, 0, NULL, 1, 0,
> +		    (1 << IIO_CHAN_INFO_SCALE_SHARED),
> +		    AD7192_CH_AIN1,
> +		    4, IIO_ST('s', 24, 32, 0), 0),
> +	IIO_CHAN(IIO_IN, 0, 1, 0, NULL, 2, 0,
> +		    (1 << IIO_CHAN_INFO_SCALE_SHARED),
> +		    AD7192_CH_AIN2,
> +		    5, IIO_ST('s', 24, 32, 0), 0),
> +	IIO_CHAN(IIO_IN, 0, 1, 0, NULL, 3, 0,
> +		    (1 << IIO_CHAN_INFO_SCALE_SHARED),
> +		    AD7192_CH_AIN3,
> +		    6, IIO_ST('s', 24, 32, 0), 0),
> +	IIO_CHAN(IIO_IN, 0, 1, 0, NULL, 4, 0,
> +		    (1 << IIO_CHAN_INFO_SCALE_SHARED),
> +		    AD7192_CH_AIN4,
> +		    7, IIO_ST('s', 24, 32, 0), 0),
> +	IIO_CHAN_SOFT_TIMESTAMP(8),
> +};
> +
> +static int __devinit ad7192_probe(struct spi_device *spi)
> +{
> +	struct ad7192_platform_data *pdata = spi->dev.platform_data;
> +	struct ad7192_state *st;
> +	struct iio_dev *indio_dev;
> +	int ret, i , voltage_uv = 0, regdone = 0;
> +
> +	if (!pdata) {
> +		dev_err(&spi->dev, "no platform data?\n");
> +		return -ENODEV;
> +	}
> +
> +	if (!spi->irq) {
> +		dev_err(&spi->dev, "no IRQ?\n");
> +		return -ENODEV;
> +	}
> +
> +	indio_dev = iio_allocate_device(sizeof(*st));
> +	if (indio_dev == NULL)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +
> +	st->reg = regulator_get(&spi->dev, "vcc");
> +	if (!IS_ERR(st->reg)) {
> +		ret = regulator_enable(st->reg);
> +		if (ret)
> +			goto error_put_reg;
> +
> +		voltage_uv = regulator_get_voltage(st->reg);
> +	}
> +
> +	st->pdata = pdata;
> +
> +	if (pdata && pdata->vref_mv)
> +		st->int_vref_mv = pdata->vref_mv;
> +	else if (voltage_uv)
> +		st->int_vref_mv = voltage_uv / 1000;
> +	else
> +		dev_warn(&spi->dev, "reference voltage undefined\n");
> +
> +	spi_set_drvdata(spi, indio_dev);
> +	st->spi = spi;
> +	st->devid = spi_get_device_id(spi)->driver_data;
> +	indio_dev->dev.parent = &spi->dev;
> +	indio_dev->name = spi_get_device_id(spi)->name;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = ad7192_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(ad7192_channels);
> +	indio_dev->available_scan_masks = st->available_scan_masks;
> +	indio_dev->info = &ad7192_info;
> +
> +	for (i = 0; i < indio_dev->num_channels; i++)
> +		st->available_scan_masks[i] = (1 << i) | (1 <<
> +			indio_dev->channels[indio_dev->num_channels - 1].
> +			scan_index);
> +
> +	init_waitqueue_head(&st->wq_data_avail);
> +
> +	ret = ad7192_register_ring_funcs_and_init(indio_dev);
> +	if (ret)
> +		goto error_disable_reg;
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret)
> +		goto error_unreg_ring;
> +	regdone = 1;
Hmm..

> +
> +	ret = ad7192_probe_trigger(indio_dev);
> +	if (ret)
> +		goto error_unreg_ring;
> +
> +	ret = iio_ring_buffer_register_ex(indio_dev->ring, 0,
> +					  indio_dev->channels,
> +					  indio_dev->num_channels);
> +	if (ret)
> +		goto error_remove_trigger;
> +
> +	ret = ad7192_setup(st);
> +	if (ret)
> +		goto error_uninitialize_ring;
> +
> +	return 0;
> +
> +error_uninitialize_ring:
> +	iio_ring_buffer_unregister(indio_dev->ring);
> +error_remove_trigger:
> +	ad7192_remove_trigger(indio_dev);
> +error_unreg_ring:
> +	ad7192_ring_cleanup(indio_dev);
> +error_disable_reg:
> +	if (!IS_ERR(st->reg))
> +		regulator_disable(st->reg);
> +error_put_reg:
> +	if (!IS_ERR(st->reg))
> +		regulator_put(st->reg);
> +
> +	if (regdone)
> +		iio_device_unregister(indio_dev);
> +	else
> +		iio_free_device(indio_dev);
> +
> +	return ret;
> +}
> +
> +static int ad7192_remove(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> +	struct ad7192_state *st = iio_priv(indio_dev);
> +
> +	iio_ring_buffer_unregister(indio_dev->ring);
> +	ad7192_remove_trigger(indio_dev);
> +	ad7192_ring_cleanup(indio_dev);
> +
> +	if (!IS_ERR(st->reg)) {
> +		regulator_disable(st->reg);
> +		regulator_put(st->reg);
> +	}
> +
> +	iio_device_unregister(indio_dev);
> +
> +	return 0;
> +}
> +
> +static const struct spi_device_id ad7192_id[] = {
> +	{"ad7190", ID_AD7190},
> +	{"ad7192", ID_AD7192},
> +	{"ad7195", ID_AD7195},
tab in next line?
> +{}
> +};
> +
> +static struct spi_driver ad7192_driver = {
> +	.driver = {
> +		.name	= "ad7192",
> +		.bus	= &spi_bus_type,

This is set in spi_register_driver, so why set it here? (basically
have I missed an upcoming change in the spi core?)
> +		.owner	= THIS_MODULE,
> +	},
> +	.probe		= ad7192_probe,
> +	.remove		= __devexit_p(ad7192_remove),
> +	.id_table	= ad7192_id,
> +};
> +
> +static int __init ad7192_init(void)
> +{
> +	return spi_register_driver(&ad7192_driver);
> +}
> +module_init(ad7192_init);
> +
> +static void __exit ad7192_exit(void)
> +{
> +	spi_unregister_driver(&ad7192_driver);
> +}
> +module_exit(ad7192_exit);
> +
> +MODULE_AUTHOR("Michael Hennerich <hennerich@blackfin.uclinux.org>");
> +MODULE_DESCRIPTION("Analog Devices AD7190, AD7192, AD7195 ADC");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/staging/iio/adc/ad7192.h b/drivers/staging/iio/adc/ad7192.h
> new file mode 100644
> index 0000000..2e0bd5e
> --- /dev/null
> +++ b/drivers/staging/iio/adc/ad7192.h
> @@ -0,0 +1,47 @@
> +/*
> + * AD7190 AD7192 AD7195 SPI ADC driver
> + *
> + * Copyright 2011 Analog Devices Inc.
> + *
> + * Licensed under the GPL-2.
> + */
> +#ifndef IIO_ADC_AD7192_H_
> +#define IIO_ADC_AD7192_H_
> +
> +/*
> + * TODO: struct ad7192_platform_data needs to go into include/linux/iio
> + */
> +
> +/**
> + * struct ad7192_platform_data - platform/board specific information
> + * @vref_mv:		the external reference voltage in millivolt
> + * @ext_clk_Hz:		the external clock frequency in Hz, if not set
> + *			the driver uses the internal clock (16.776 MHz)
> + * @clock_source_sel:	[0..3]
> + *			0 External 4.92 MHz clock connected from MCLK1 to MCLK2
> + *			1 External Clock applied to MCLK2
> + *			2 Internal 4.92 MHz Clock not available at the MCLK2 pin
> + *			3 Internal 4.92 MHz Clock available at the MCLK2 pin
> + * @refin2_en:		REFIN1/REFIN2 Reference Select (AD7190/2 only)
> + * @rej60_en:		50/60Hz notch filter enable
> + * @sinc3_en:		SINC3 filter enable (default SINC4)
> + * @chop_en:		CHOP mode enable
> + * @buf_en:		buffered input mode enable
> + * @unipolar_en:	unipolar mode enable
> + * @burnout_curr_en:	constant current generators on AIN(+|-) enable
> + */
> +
> +struct ad7192_platform_data {
> +	u16		vref_mv;
> +	u32		ext_clk_Hz;
Trivial, but reorder this to the top to save 2 bytes.
> +	u8		clock_source_sel;
> +	bool		refin2_en;
> +	bool		rej60_en;
> +	bool		sinc3_en;
> +	bool		chop_en;
> +	bool		buf_en;
> +	bool		unipolar_en;
> +	bool		burnout_curr_en;
> +};
> +
> +#endif /* IIO_ADC_AD7192_H_ */