Re: [PATCH] IIO: ADC: New driver for the AD7298 8-channel SPI ADC

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

On 02/14/11 14:03, michael.hennerich@analog.com wrote:
> From: Michael Hennerich <michael.hennerich@analog.com>
> 
> This patch adds support for the
> AD7298:  8-Channel, 1MSPS, 12-Bit SAR ADC with Temperature Sensor
> via SPI bus.
> 
> This patch replaces the existing ad7298.c driver completely.
> It was necessary since, the old driver did not comply with the
> IIO ABI for such devices.
Guess that's one approach to fixing up a driver!
Anyhow, it's nice and clean now.

Couple of trivial points inline.  You may need some locking
in the temperature read function, fix that or explain what
I'm missing before sending on to Greg, the other bits are up
to you.

I see the original driver used a busy pin. For the record, could you
also explain any disadvantages in this new one not doing that?

Thanks,

Jonathan

> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
> ---
>  drivers/staging/iio/adc/Kconfig       |    7 +-
>  drivers/staging/iio/adc/Makefile      |    5 +-
>  drivers/staging/iio/adc/ad7298.c      |  501 ---------------------------------
>  drivers/staging/iio/adc/ad7298.h      |   87 ++++++
>  drivers/staging/iio/adc/ad7298_core.c |  287 +++++++++++++++++++
>  drivers/staging/iio/adc/ad7298_ring.c |  244 ++++++++++++++++
>  6 files changed, 627 insertions(+), 504 deletions(-)
>  delete mode 100644 drivers/staging/iio/adc/ad7298.c
>  create mode 100644 drivers/staging/iio/adc/ad7298.h
>  create mode 100644 drivers/staging/iio/adc/ad7298_core.c
>  create mode 100644 drivers/staging/iio/adc/ad7298_ring.c
> 
> diff --git a/drivers/staging/iio/adc/Kconfig b/drivers/staging/iio/adc/Kconfig
> index 86869cd..34daf58 100644
> --- a/drivers/staging/iio/adc/Kconfig
> +++ b/drivers/staging/iio/adc/Kconfig
> @@ -49,11 +49,14 @@ config AD7291
>  	  temperature sensors.
>  
>  config AD7298
> -	tristate "Analog Devices AD7298 temperature sensor and ADC driver"
> +	tristate "Analog Devices AD7298 ADC driver"
>  	depends on SPI
>  	help
>  	  Say yes here to build support for Analog Devices AD7298
> -	  temperature sensors and ADC.
> +	  8 Channel ADC with temperature sensor.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called ad7298.
Good. That description kind of emphasises the adc side more and explains what
this is doing in IIO rather than hwmon.
>  
>  config AD7314
>  	tristate "Analog Devices AD7314 temperature sensor driver"
> diff --git a/drivers/staging/iio/adc/Makefile b/drivers/staging/iio/adc/Makefile
> index 6f231a2..662e56f 100644
> --- a/drivers/staging/iio/adc/Makefile
> +++ b/drivers/staging/iio/adc/Makefile
> @@ -19,10 +19,13 @@ ad7887-y := ad7887_core.o
>  ad7887-$(CONFIG_IIO_RING_BUFFER) += ad7887_ring.o
>  obj-$(CONFIG_AD7887) += ad7887.o
>  
> +ad7298-y := ad7298_core.o
> +ad7298-$(CONFIG_IIO_RING_BUFFER) += ad7298_ring.o
> +obj-$(CONFIG_AD7298) += ad7298.o
> +
>  obj-$(CONFIG_AD7150) += ad7150.o
>  obj-$(CONFIG_AD7152) += ad7152.o
>  obj-$(CONFIG_AD7291) += ad7291.o
> -obj-$(CONFIG_AD7298) += ad7298.o
>  obj-$(CONFIG_AD7314) += ad7314.o
>  obj-$(CONFIG_AD7745) += ad7745.o
>  obj-$(CONFIG_AD7816) += ad7816.o
> diff --git a/drivers/staging/iio/adc/ad7298.c b/drivers/staging/iio/adc/ad7298.c
> deleted file mode 100644
> index 1a080c9..0000000
....
> diff --git a/drivers/staging/iio/adc/ad7298.h b/drivers/staging/iio/adc/ad7298.h
> new file mode 100644
> index 0000000..3e11865
> --- /dev/null
> +++ b/drivers/staging/iio/adc/ad7298.h
> @@ -0,0 +1,87 @@
> +/*
> + * AD7298 SPI ADC driver
> + *
> + * Copyright 2011 Analog Devices Inc.
> + *
> + * Licensed under the GPL-2.
> + */
> +
> +#ifndef IIO_ADC_AD7298_H_
> +#define IIO_ADC_AD7298_H_
> +
> +#define AD7298_WRITE	(1 << 15) /* write to the control register */
> +#define AD7298_REPEAT	(1 << 14) /* repeated conversion enable */

Perhaps a single macro covering them all?
#define AD7298_CH(x) (1 << (13 - x))
where x is then 0 through 7 
> +#define AD7298_CH0	(1 << 13) /* channel select */
> +#define AD7298_CH1	(1 << 12) /* channel select */
> +#define AD7298_CH2	(1 << 11) /* channel select */
> +#define AD7298_CH3	(1 << 10) /* channel select */
> +#define AD7298_CH4	(1 << 9) /* channel select */
> +#define AD7298_CH5	(1 << 8) /* channel select */
> +#define AD7298_CH6	(1 << 7) /* channel select */
> +#define AD7298_CH7	(1 << 6) /* channel select */
> +#define AD7298_TSENSE	(1 << 5) /* temperature conversion enable */
> +#define AD7298_EXTREF	(1 << 2) /* external reference enable */
> +#define AD7298_TAVG	(1 << 1) /* temperature sensor averaging enable */
> +#define AD7298_PDD	(1 << 0) /* partial power down enable */
> +
> +#define AD7298_CH_MASK	(AD7298_CH0 | AD7298_CH1 | AD7298_CH2 | AD7298_CH3 | \
> +			AD7298_CH4 | AD7298_CH5 | AD7298_CH6 | AD7298_CH7)
> +
> +#define AD7298_MAX_CHAN		8
> +#define AD7298_BITS		12
> +#define AD7298_STORAGE_BITS	16
> +#define AD7298_INTREF_mV	2500
> +
> +#define RES_MASK(bits)	((1 << (bits)) - 1)
> +
> +/*
> + * TODO: struct ad7298_platform_data needs to go into include/linux/iio
> + */
> +
> +struct ad7298_platform_data {
> +	/* External Vref voltage applied */
> +	u16				vref_mv;
> +};
> +
> +struct ad7298_state {
> +	struct iio_dev			*indio_dev;
> +	struct spi_device		*spi;
> +	struct regulator		*reg;
> +	struct work_struct		poll_work;
> +	atomic_t			protect_ring;
> +	size_t				d_size;
> +	u16				int_vref_mv;
> +	unsigned			ext_ref;
Could put #ifdefs round the bits only used when the ring is enabled.
Potential tiny saving in space, but more importantly makes it clear
what is used where. Could put the other ring based bits in there as
well I guess (and yes, we could do this in numerous drivers).  Maybe
it isn't worth the hassle.
> +	struct spi_transfer		ring_xfer[10];
> +	struct spi_transfer		scan_single_xfer[3];
> +	struct spi_message		ring_msg;
> +	struct spi_message		scan_single_msg;
> +	/*
> +	 * DMA (thus cache coherency maintenance) requires the
> +	 * transfer buffers to live in their own cache lines.
> +	 */
> +	unsigned short			rx_buf[8] ____cacheline_aligned;
> +	unsigned short			tx_buf[2];
> +};
> +
> +#ifdef CONFIG_IIO_RING_BUFFER
> +int ad7298_scan_from_ring(struct ad7298_state *st, long ch);
> +int ad7298_register_ring_funcs_and_init(struct iio_dev *indio_dev);
> +void ad7298_ring_cleanup(struct iio_dev *indio_dev);
> +#else /* CONFIG_IIO_RING_BUFFER */
> +static inline int ad7298_scan_from_ring(struct ad7298_state *st, long ch)
> +{
> +	return 0;
> +}
> +
> +static inline int
> +ad7298_register_ring_funcs_and_init(struct iio_dev *indio_dev)
> +{
> +	return 0;
> +}
> +
> +static inline void ad7298_ring_cleanup(struct iio_dev *indio_dev)
> +{
> +}
> +#endif /* CONFIG_IIO_RING_BUFFER */
> +#endif /* IIO_ADC_AD7298_H_ */
> diff --git a/drivers/staging/iio/adc/ad7298_core.c b/drivers/staging/iio/adc/ad7298_core.c
> new file mode 100644
> index 0000000..4d63bd2
> --- /dev/null
> +++ b/drivers/staging/iio/adc/ad7298_core.c
> @@ -0,0 +1,287 @@
> +/*
> + * AD7298 SPI ADC driver
> + *
> + * Copyright 2011 Analog Devices Inc.
> + *
> + * Licensed under the GPL-2.
> + */
> +
> +#include <linux/workqueue.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/delay.h>
> +
> +#include "../iio.h"
> +#include "../sysfs.h"
> +#include "../ring_generic.h"
> +#include "adc.h"
> +
> +#include "ad7298.h"
> +
> +static int ad7298_scan_direct(struct ad7298_state *st, unsigned ch)
> +{
> +	int ret;
> +	st->tx_buf[0] = cpu_to_be16(AD7298_WRITE | st->ext_ref |
> +				   (AD7298_CH0 >> ch));
> +
> +	ret = spi_sync(st->spi, &st->scan_single_msg);
> +	if (ret)
> +		return ret;
> +
> +	return be16_to_cpu(st->rx_buf[0]);
> +}
> +
> +static ssize_t ad7298_scan(struct device *dev,
> +			    struct device_attribute *attr,
> +			    char *buf)
> +{
> +	struct iio_dev *dev_info = dev_get_drvdata(dev);
> +	struct ad7298_state *st = dev_info->dev_data;
> +	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> +	int ret;
> +
> +	mutex_lock(&dev_info->mlock);
> +	if (iio_ring_enabled(dev_info))
> +		ret = ad7298_scan_from_ring(st, this_attr->address);
> +	else
> +		ret = ad7298_scan_direct(st, this_attr->address);
> +	mutex_unlock(&dev_info->mlock);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	return sprintf(buf, "%d\n", ret & RES_MASK(AD7298_BITS));
> +}
> +
> +static IIO_DEV_ATTR_IN_RAW(0, ad7298_scan, 0);
> +static IIO_DEV_ATTR_IN_RAW(1, ad7298_scan, 1);
> +static IIO_DEV_ATTR_IN_RAW(2, ad7298_scan, 2);
> +static IIO_DEV_ATTR_IN_RAW(3, ad7298_scan, 3);
> +static IIO_DEV_ATTR_IN_RAW(4, ad7298_scan, 4);
> +static IIO_DEV_ATTR_IN_RAW(5, ad7298_scan, 5);
> +static IIO_DEV_ATTR_IN_RAW(6, ad7298_scan, 6);
> +static IIO_DEV_ATTR_IN_RAW(7, ad7298_scan, 7);
> +
> +static ssize_t ad7298_show_temp(struct device *dev,
> +				struct device_attribute *attr,
> +				char *buf)
> +{
> +	/* Driver currently only support internal vref */
> +	struct iio_dev *dev_info = dev_get_drvdata(dev);
> +	struct ad7298_state *st = iio_dev_get_devdata(dev_info);
> +	unsigned short tmp;
> +	char sign;
> +
I guess we might want to make the TAVG bit controllable? As ever
though, whoever wants this can add it later.
> +	tmp = cpu_to_be16(AD7298_WRITE | AD7298_TSENSE |
> +			  AD7298_TAVG | st->ext_ref);
> +

Should you hold a lock here to prevent other reads being attempted?
> +	spi_write(st->spi, (u8 *)&tmp, 2);
> +	tmp = 0;
> +	spi_write(st->spi, (u8 *)&tmp, 2);
> +	usleep_range(101, 1000); /* sleep > 100us */
> +	spi_read(st->spi, (u8 *)&tmp, 2);
> +
> +	tmp = be16_to_cpu(tmp) & RES_MASK(AD7298_BITS);
> +
> +	/*
> +	 * One LSB of the ADC corresponds to 0.25 deg C.
> +	 * The temperature reading is in 12-bit twos complement format
> +	 */
> +
> +	if (tmp & (1 << (AD7298_BITS - 1))) {
> +		tmp = (1 << AD7298_BITS) - tmp;
> +		sign = '-';
> +	} else {
> +		sign = '+';
> +	}
> +
> +	return sprintf(buf, "%c%d.%.2d\n", sign, tmp / 4, (tmp & 3) * 25);
> +}
> +
> +static IIO_DEVICE_ATTR(temp, S_IRUGO, ad7298_show_temp, NULL, 0);
should be temp0_input and in milli degrees Celcius.
Sorry, need to match hwmon for this.  Also needs documentation.  Actually
I hadn't previously noticed that hwmon uses milli degrees. Dratt, we may
have to change our all the _scale parameters for _raw temp attributes
to take that into account + update docs obviously.
> +
> +static ssize_t ad7298_show_scale(struct device *dev,
> +				struct device_attribute *attr,
> +				char *buf)
> +{
> +	/* Driver currently only support internal vref */
> +	struct iio_dev *dev_info = dev_get_drvdata(dev);
> +	struct ad7298_state *st = iio_dev_get_devdata(dev_info);
> +	/* Corresponds to Vref / 2^(bits) */
> +	unsigned int scale_uv = (st->int_vref_mv * 1000) >> AD7298_BITS;
> +
> +	return sprintf(buf, "%d.%03d\n", scale_uv / 1000, scale_uv % 1000);
> +}
> +static IIO_DEVICE_ATTR(in_scale, S_IRUGO, ad7298_show_scale, NULL, 0);
> +
> +static ssize_t ad7298_show_name(struct device *dev,
> +				 struct device_attribute *attr,
> +				 char *buf)
> +{
> +	struct iio_dev *dev_info = dev_get_drvdata(dev);
> +	struct ad7298_state *st = iio_dev_get_devdata(dev_info);
> +
> +	return sprintf(buf, "%s\n", spi_get_device_id(st->spi)->name);
> +}
> +static IIO_DEVICE_ATTR(name, S_IRUGO, ad7298_show_name, NULL, 0);
I'm guessing more devices are following using this driver. If not I'd
advocate use of a const attr for this.
> +
> +static struct attribute *ad7298_attributes[] = {
> +	&iio_dev_attr_in0_raw.dev_attr.attr,
> +	&iio_dev_attr_in1_raw.dev_attr.attr,
> +	&iio_dev_attr_in2_raw.dev_attr.attr,
> +	&iio_dev_attr_in3_raw.dev_attr.attr,
> +	&iio_dev_attr_in4_raw.dev_attr.attr,
> +	&iio_dev_attr_in5_raw.dev_attr.attr,
> +	&iio_dev_attr_in6_raw.dev_attr.attr,
> +	&iio_dev_attr_in7_raw.dev_attr.attr,
> +	&iio_dev_attr_in_scale.dev_attr.attr,
> +	&iio_dev_attr_temp.dev_attr.attr,
> +	&iio_dev_attr_name.dev_attr.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group ad7298_attribute_group = {
> +	.attrs = ad7298_attributes,
> +};
> +
> +static int __devinit ad7298_probe(struct spi_device *spi)
> +{
> +	struct ad7298_platform_data *pdata = spi->dev.platform_data;
> +	struct ad7298_state *st;
> +	int ret;
> +
> +	st = kzalloc(sizeof(*st), GFP_KERNEL);
> +	if (st == NULL) {
> +		ret = -ENOMEM;
> +		goto error_ret;
> +	}
> +
> +	st->reg = regulator_get(&spi->dev, "vcc");
> +	if (!IS_ERR(st->reg)) {
> +		ret = regulator_enable(st->reg);
> +		if (ret)
> +			goto error_put_reg;
> +	}
> +
> +	spi_set_drvdata(spi, st);
> +
> +	atomic_set(&st->protect_ring, 0);
> +	st->spi = spi;
> +
> +	st->indio_dev = iio_allocate_device();
> +	if (st->indio_dev == NULL) {
> +		ret = -ENOMEM;
> +		goto error_disable_reg;
> +	}
> +
> +	st->indio_dev->dev.parent = &spi->dev;
> +	st->indio_dev->attrs = &ad7298_attribute_group;
> +	st->indio_dev->dev_data = (void *)(st);
> +	st->indio_dev->driver_module = THIS_MODULE;
> +	st->indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	/* Setup default message */
> +
> +	st->scan_single_xfer[0].tx_buf = &st->tx_buf[0];
> +	st->scan_single_xfer[0].len = 2;
> +	st->scan_single_xfer[0].cs_change = 1;
> +	st->scan_single_xfer[1].tx_buf = &st->tx_buf[1];
> +	st->scan_single_xfer[1].len = 2;
> +	st->scan_single_xfer[1].cs_change = 1;
> +	st->scan_single_xfer[2].rx_buf = &st->rx_buf[0];
> +	st->scan_single_xfer[2].len = 2;
> +
> +	spi_message_init(&st->scan_single_msg);
> +	spi_message_add_tail(&st->scan_single_xfer[0], &st->scan_single_msg);
> +	spi_message_add_tail(&st->scan_single_xfer[1], &st->scan_single_msg);
> +	spi_message_add_tail(&st->scan_single_xfer[2], &st->scan_single_msg);
> +
> +	if (pdata && pdata->vref_mv) {
> +		st->int_vref_mv = pdata->vref_mv;
> +		st->ext_ref = AD7298_EXTREF;
> +	} else {
> +		st->int_vref_mv = AD7298_INTREF_mV;
> +	}
> +
> +	ret = ad7298_register_ring_funcs_and_init(st->indio_dev);
> +	if (ret)
> +		goto error_free_device;
> +
> +	ret = iio_device_register(st->indio_dev);
> +	if (ret)
> +		goto error_free_device;
> +
> +	ret = iio_ring_buffer_register(st->indio_dev->ring, 0);
> +	if (ret)
> +		goto error_cleanup_ring;
> +	return 0;
> +
> +error_cleanup_ring:
> +	ad7298_ring_cleanup(st->indio_dev);
> +	iio_device_unregister(st->indio_dev);
> +error_free_device:
> +	iio_free_device(st->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);
> +	kfree(st);
> +error_ret:
> +	return ret;
> +}
> +
> +static int ad7298_remove(struct spi_device *spi)
> +{
> +	struct ad7298_state *st = spi_get_drvdata(spi);
> +	struct iio_dev *indio_dev = st->indio_dev;
> +
> +	iio_ring_buffer_unregister(indio_dev->ring);
Might be my editor, but I think you have a weird spacing
issue on the next line.
> +		ad7298_ring_cleanup(indio_dev);
> +	iio_device_unregister(indio_dev);
> +	if (!IS_ERR(st->reg)) {
> +		regulator_disable(st->reg);
> +		regulator_put(st->reg);
> +	}
> +	kfree(st);
> +	return 0;
> +}
> +
> +static const struct spi_device_id ad7298_id[] = {
> +	{"ad7298", 0},
> +	{}
> +};
> +
> +static struct spi_driver ad7298_driver = {
> +	.driver = {
> +		.name	= "ad7298",
> +		.bus	= &spi_bus_type,
> +		.owner	= THIS_MODULE,
> +	},
> +	.probe		= ad7298_probe,
> +	.remove		= __devexit_p(ad7298_remove),
> +	.id_table	= ad7298_id,
> +};
> +
> +static int __init ad7298_init(void)
> +{
> +	return spi_register_driver(&ad7298_driver);
> +}
> +module_init(ad7298_init);
> +
> +static void __exit ad7298_exit(void)
> +{
> +	spi_unregister_driver(&ad7298_driver);
> +}
> +module_exit(ad7298_exit);
> +
> +MODULE_AUTHOR("Michael Hennerich <hennerich@blackfin.uclinux.org>");
> +MODULE_DESCRIPTION("Analog Devices AD7298 ADC");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("spi:ad7298");
> diff --git a/drivers/staging/iio/adc/ad7298_ring.c b/drivers/staging/iio/adc/ad7298_ring.c
> new file mode 100644
> index 0000000..15e0640
> --- /dev/null
> +++ b/drivers/staging/iio/adc/ad7298_ring.c
> @@ -0,0 +1,244 @@
> +/*
> + * AD7298 SPI ADC driver
> + *
> + * Copyright 2011 Analog Devices Inc.
> + *
> + * Licensed under the GPL-2.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/workqueue.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>
> +#include <linux/spi/spi.h>
> +
> +#include "../iio.h"
> +#include "../ring_generic.h"
> +#include "../ring_sw.h"
> +#include "../trigger.h"
> +#include "../sysfs.h"
> +
> +#include "ad7298.h"
> +
> +static IIO_SCAN_EL_C(in0, 0, 0, NULL);
> +static IIO_SCAN_EL_C(in1, 1, 0, NULL);
> +static IIO_SCAN_EL_C(in2, 2, 0, NULL);
> +static IIO_SCAN_EL_C(in3, 3, 0, NULL);
> +static IIO_SCAN_EL_C(in4, 4, 0, NULL);
> +static IIO_SCAN_EL_C(in5, 5, 0, NULL);
> +static IIO_SCAN_EL_C(in6, 6, 0, NULL);
> +static IIO_SCAN_EL_C(in7, 7, 0, NULL);
> +
> +static IIO_CONST_ATTR(in_type, "u12/16") ;
> +
> +static struct attribute *ad7298_scan_el_attrs[] = {
> +	&iio_scan_el_in0.dev_attr.attr,
> +	&iio_const_attr_in0_index.dev_attr.attr,
> +	&iio_scan_el_in1.dev_attr.attr,
> +	&iio_const_attr_in1_index.dev_attr.attr,
> +	&iio_scan_el_in2.dev_attr.attr,
> +	&iio_const_attr_in2_index.dev_attr.attr,
> +	&iio_scan_el_in3.dev_attr.attr,
> +	&iio_const_attr_in3_index.dev_attr.attr,
> +	&iio_scan_el_in4.dev_attr.attr,
> +	&iio_const_attr_in4_index.dev_attr.attr,
> +	&iio_scan_el_in5.dev_attr.attr,
> +	&iio_const_attr_in5_index.dev_attr.attr,
> +	&iio_scan_el_in6.dev_attr.attr,
> +	&iio_const_attr_in6_index.dev_attr.attr,
> +	&iio_scan_el_in7.dev_attr.attr,
> +	&iio_const_attr_in7_index.dev_attr.attr,
> +	&iio_const_attr_in_type.dev_attr.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group ad7298_scan_el_group = {
> +	.name = "scan_elements",
> +	.attrs = ad7298_scan_el_attrs,
> +};
> +
> +int ad7298_scan_from_ring(struct ad7298_state *st, long ch)
> +{
> +	struct iio_ring_buffer *ring = st->indio_dev->ring;
> +	int ret;
> +	u16 *ring_data;
> +
> +	if (!(ring->scan_mask & (1 << ch))) {
> +		ret = -EBUSY;
> +		goto error_ret;
> +	}
> +
> +	ring_data = kmalloc(ring->access.get_bytes_per_datum(ring), GFP_KERNEL);
> +	if (ring_data == NULL) {
> +		ret = -ENOMEM;
> +		goto error_ret;
> +	}
> +	ret = ring->access.read_last(ring, (u8 *) ring_data);
> +	if (ret)
> +		goto error_free_ring_data;
> +
> +	ret = be16_to_cpu(ring_data[ch]);
> +
> +error_free_ring_data:
> +	kfree(ring_data);
> +error_ret:
> +	return ret;
> +}
> +
> +/**
> + * ad7298_ring_preenable() setup the parameters of the ring before enabling
> + *
> + * The complex nature of the setting of the nuber of bytes per datum is due
> + * to this driver currently ensuring that the timestamp is stored at an 8
> + * byte boundary.
> + **/
> +static int ad7298_ring_preenable(struct iio_dev *indio_dev)
> +{
> +	struct ad7298_state *st = indio_dev->dev_data;
> +	struct iio_ring_buffer *ring = indio_dev->ring;
> +	size_t d_size;
> +	int i, m;
> +	unsigned short command;
> +
> +	d_size = ring->scan_count *
> +		 (AD7298_STORAGE_BITS / 8) + sizeof(s64);
> +
> +	if (d_size % sizeof(s64))
> +		d_size += sizeof(s64) - (d_size % sizeof(s64));
> +
> +	if (ring->access.set_bytes_per_datum)
> +		ring->access.set_bytes_per_datum(ring, d_size);
> +
> +	st->d_size = d_size;
> +
> +	command = AD7298_WRITE | st->ext_ref;
> +
> +	for (i = 0, m = AD7298_CH0; i < AD7298_MAX_CHAN; i++, m >>= 1)
> +		if (ring->scan_mask & (1 << i))
> +			command |= m;
> +
> +	st->tx_buf[0] = cpu_to_be16(command);
> +
> +	/* build spi ring message */
> +	st->ring_xfer[0].tx_buf = &st->tx_buf[0];
> +	st->ring_xfer[0].len = 2;
> +	st->ring_xfer[0].cs_change = 1;
> +	st->ring_xfer[1].tx_buf = &st->tx_buf[1];
> +	st->ring_xfer[1].len = 2;
> +	st->ring_xfer[1].cs_change = 1;
> +
> +	spi_message_init(&st->ring_msg);
> +	spi_message_add_tail(&st->ring_xfer[0], &st->ring_msg);
> +	spi_message_add_tail(&st->ring_xfer[1], &st->ring_msg);
> +
> +	for (i = 0; i < ring->scan_count; i++) {
> +		st->ring_xfer[i + 2].rx_buf = &st->rx_buf[i];
> +		st->ring_xfer[i + 2].len = 2;
> +		st->ring_xfer[i + 2].cs_change = 1;
> +		spi_message_add_tail(&st->ring_xfer[i + 2], &st->ring_msg);
> +	}
> +	/* make sure last transfer cs_change is not set */
> +	st->ring_xfer[i + 1].cs_change = 0;
> +
> +	return 0;
> +}
> +
> +/**
> + * ad7298_poll_func_th() th of trigger launched polling to ring buffer
> + *
> + * As sampling only occurs on spi comms occuring, leave timestamping until
> + * then.  Some triggers will generate their own time stamp.  Currently
> + * there is no way of notifying them when no one cares.
> + **/
> +static void ad7298_poll_func_th(struct iio_dev *indio_dev, s64 time)
> +{
> +	struct ad7298_state *st = indio_dev->dev_data;
> +
> +	schedule_work(&st->poll_work);
> +	return;
> +}
New line here please.
> +/**
> + * ad7298_poll_bh_to_ring() bh of trigger launched polling to ring buffer
> + * @work_s:	the work struct through which this was scheduled
> + *
> + * Currently there is no option in this driver to disable the saving of
> + * timestamps within the ring.
> + * I think the one copy of this at a time was to avoid problems if the
> + * trigger was set far too high and the reads then locked up the computer.
> + **/
> +static void ad7298_poll_bh_to_ring(struct work_struct *work_s)
> +{
> +	struct ad7298_state *st = container_of(work_s, struct ad7298_state,
> +						  poll_work);
> +	struct iio_dev *indio_dev = st->indio_dev;
> +	struct iio_sw_ring_buffer *sw_ring = iio_to_sw_ring(indio_dev->ring);
> +	struct iio_ring_buffer *ring = indio_dev->ring;
> +	s64 time_ns;
> +	__u16 buf[16];
> +	int b_sent, i;
> +
> +	/* Ensure only one copy of this function running at a time */
> +	if (atomic_inc_return(&st->protect_ring) > 1)
> +		return;
> +
> +	b_sent = spi_sync(st->spi, &st->ring_msg);
> +	if (b_sent)
> +		goto done;
> +
> +	time_ns = iio_get_time_ns();
> +
> +	for (i = 0; i < ring->scan_count; i++)
> +		buf[i] = be16_to_cpu(st->rx_buf[i]);
> +
> +	memcpy((u8 *)buf + st->d_size - sizeof(s64), &time_ns, sizeof(time_ns));
> +	indio_dev->ring->access.store_to(&sw_ring->buf, (u8 *)buf, time_ns);
> +done:
> +	atomic_dec(&st->protect_ring);
> +}
> +
> +int ad7298_register_ring_funcs_and_init(struct iio_dev *indio_dev)
> +{
> +	struct ad7298_state *st = indio_dev->dev_data;
> +	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 */
> +	iio_ring_sw_register_funcs(&indio_dev->ring->access);
> +	ret = iio_alloc_pollfunc(indio_dev, NULL, &ad7298_poll_func_th);
> +	if (ret)
> +		goto error_deallocate_sw_rb;
> +
> +	/* Ring buffer functions - here trigger setup related */
> +
> +	indio_dev->ring->preenable = &ad7298_ring_preenable;
> +	indio_dev->ring->postenable = &iio_triggered_ring_postenable;
> +	indio_dev->ring->predisable = &iio_triggered_ring_predisable;
> +	indio_dev->ring->scan_el_attrs = &ad7298_scan_el_group;
> +
> +	INIT_WORK(&st->poll_work, &ad7298_poll_bh_to_ring);
> +
> +	/* 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;
> +}
> +
> +void ad7298_ring_cleanup(struct iio_dev *indio_dev)
> +{
> +	if (indio_dev->trig) {
> +		iio_put_trigger(indio_dev->trig);
> +		iio_trigger_dettach_poll_func(indio_dev->trig,
> +					      indio_dev->pollfunc);
> +	}
> +	kfree(indio_dev->pollfunc);
> +	iio_sw_rb_free(indio_dev->ring);
> +}