From: Michael Hennerich <michael.hennerich@analog.com>
To: Jonathan Cameron <jic23@cam.ac.uk>
Cc: "linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>,
"device-drivers-devel@blackfin.uclinux.org"
<device-drivers-devel@blackfin.uclinux.org>,
Drivers <Drivers@analog.com>
Subject: Re: [PATCH] IIO: ADC: New driver for AD7190/AD7192/AD7195 4 Channel SPI ADC
Date: Wed, 17 Aug 2011 15:47:51 +0200 [thread overview]
Message-ID: <4E4BC687.3050504@analog.com> (raw)
In-Reply-To: <4E452E51.5040203@cam.ac.uk>
On 08/12/2011 03:44 PM, Jonathan Cameron wrote:
> 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?
typo...
>> + * 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.
leftover from some debug test - 4 bytes are sufficient.
>> + 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) ?
needless !! only...
In unipolar operation the we have 2^N, in bipolar it's 2^(N-1)
>> + 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.
ok
>> +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.
ok
>> + 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?)
ok - lets protect it by mutex.
>> + 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!)
>
ok
>> + 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..
Anything bad here?
>> +
>> + 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?)
indeed needless
>> + .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.
ok
>> + 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_ */
>
Thanks for the review!
--
Greetings,
Michael
--
Analog Devices GmbH Wilhelm-Wagenfeld-Str. 6 80807 Muenchen
Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368;
Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin,
Margaret Seif
next prev parent reply other threads:[~2011-08-17 13:47 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-08-12 12:41 [PATCH] IIO: ADC: New driver for AD7190/AD7192/AD7195 4 Channel SPI ADC michael.hennerich
2011-08-12 13:44 ` Jonathan Cameron
2011-08-17 13:47 ` Michael Hennerich [this message]
2011-08-17 14:29 ` Jonathan Cameron
-- strict thread matches above, loose matches on Subject: below --
2011-08-17 15:04 michael.hennerich
2011-08-17 15:31 ` Jonathan Cameron
2011-08-17 15:57 ` Jonathan Cameron
2011-08-18 8:20 ` Hennerich, Michael
2011-08-17 16:05 ` Paul Thomas
2011-08-18 6:41 ` Hennerich, Michael
2011-08-18 17:26 ` Paul Thomas
2011-08-19 7:41 ` Hennerich, Michael
2011-08-26 6:41 ` Paul Thomas
2011-08-26 7:29 ` Hennerich, Michael
2011-08-17 15:29 michael.hennerich
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4E4BC687.3050504@analog.com \
--to=michael.hennerich@analog.com \
--cc=Drivers@analog.com \
--cc=device-drivers-devel@blackfin.uclinux.org \
--cc=jic23@cam.ac.uk \
--cc=linux-iio@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox