[PATCH] IIO ADC support for AD7923

[PATCH] IIO ADC support for AD7923

[Christophe Leroy]

This patch adds support for Analog Devices AD7923 ADC in the IIO Subsystem.

Signed-off-by: Patrick Vasseur <patrick.vasseur@c-s.fr>
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>

diff -uN linux-3.7.1/drivers/staging/iio/adc/Kconfig linux/drivers/staging/iio/adc/Kconfig
--- linux-3.7.1/drivers/staging/iio/adc/Kconfig	2012-12-17 20:14:54.000000000 +0100
+++ linux/drivers/staging/iio/adc/Kconfig	2012-12-13 19:52:10.000000000 +0100
@@ -21,6 +21,17 @@
 	  To compile this driver as a module, choose M here: the
 	  module will be called ad7298.
 
+config AD7923
+	tristate "Analog Devices AD7923 ADC driver"
+	depends on SPI
+	select IIO_TRIGGERED_BUFFER if IIO_BUFFER
+	help
+	  Say yes here to build support for Analog Devices AD7923
+	  4 Channel ADC.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ad7923.
+
 config AD7606
 	tristate "Analog Devices AD7606 ADC driver"
 	depends on GPIOLIB
diff -uN linux-3.7.1/drivers/staging/iio/adc/Makefile linux/drivers/staging/iio/adc/Makefile
--- linux-3.7.1/drivers/staging/iio/adc/Makefile	2012-12-17 20:14:54.000000000 +0100
+++ linux/drivers/staging/iio/adc/Makefile	2012-12-13 19:52:10.000000000 +0100
@@ -25,6 +25,10 @@
 ad7298-$(CONFIG_IIO_BUFFER) += ad7298_ring.o
 obj-$(CONFIG_AD7298) += ad7298.o
 
+ad7923-y := ad7923_core.o
+ad7923-$(CONFIG_IIO_BUFFER) += ad7923_ring.o
+obj-$(CONFIG_AD7923) += ad7923.o
+
 obj-$(CONFIG_AD7291) += ad7291.o
 obj-$(CONFIG_AD7780) += ad7780.o
 obj-$(CONFIG_AD7793) += ad7793.o
diff -uN linux-3.7.1/drivers/staging/iio/adc/ad7923.h linux/drivers/staging/iio/adc/ad7923.h
--- linux-3.7.1/drivers/staging/iio/adc/ad7923.h	1970-01-01 01:00:00.000000000 +0100
+++ linux/drivers/staging/iio/adc/ad7923.h	2013-01-05 17:53:53.000000000 +0100
@@ -0,0 +1,72 @@
+/*
+ * AD7923 SPI ADC driver
+ *
+ * Copyright 2011 Analog Devices Inc (from AD7298 Driver)
+ * Copyright 2012 CS Systemes d'Information
+ *
+ * Licensed under the GPL-2 or later.
+ */
+#ifndef IIO_ADC_AD7923_H_
+#define IIO_ADC_AD7923_H_
+
+#define AD7923_WRITE_CR		(1 << 11)	/* write control register */
+#define AD7923_RANGE		(1 << 1)	/* range to REFin */
+#define AD7923_CODING		(1 << 0)	/* coding is straight binary */
+#define AD7923_PM_MODE_AS	(1)		/* auto shutdown */
+#define AD7923_PM_MODE_FS	(2)		/* full shutdown */
+#define AD7923_PM_MODE_OPS	(3)		/* normal operation */
+#define AD7923_CHANNEL_0	(0)		/* analog input 0 */
+#define AD7923_CHANNEL_1	(1)		/* analog input 1 */
+#define AD7923_CHANNEL_2	(2)		/* analog input 2 */
+#define AD7923_CHANNEL_3	(3)		/* analog input 3 */
+#define AD7923_SEQUENCE_OFF	(0)		/* no sequence fonction */
+#define AD7923_SEQUENCE_PROTECT	(2)		/* no interrupt write cycle */
+#define AD7923_SEQUENCE_ON	(3)		/* continuous sequence */
+
+#define AD7923_MAX_CHAN		4
+
+#define AD7923_PM_MODE_WRITE(mode)	(mode << 4)	/* write mode */
+#define AD7923_CHANNEL_WRITE(channel)	(channel << 6)	/* write channel */
+#define AD7923_SEQUENCE_WRITE(sequence)	(((sequence & 1) << 3) \
+					+ ((sequence & 2) << 9))
+						/* write sequence fonction */
+/* left shift for CR : bit 11 transmit in first */
+#define AD7923_SHIFT_REGISTER	4
+
+/* val = value, dec = left shift, bits = number of bits of the mask */
+#define EXTRACT(val, dec, bits)		((val >> dec) & ((1 << bits) - 1))
+/* val = value, bits = number of bits of the original value */
+#define EXTRACT_PERCENT(val, bits)	(((val + 1) * 100) >> bits)
+
+struct ad7923_state {
+	struct spi_device		*spi;
+	struct regulator		*reg;
+	struct spi_transfer		ring_xfer[6];
+	struct spi_transfer		scan_single_xfer[2];
+	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[4] ____cacheline_aligned;
+	unsigned short			tx_buf[2];
+};
+
+#ifdef CONFIG_IIO_BUFFER
+int ad7923_register_ring_funcs_and_init(struct iio_dev *indio_dev);
+void ad7923_ring_cleanup(struct iio_dev *indio_dev);
+int ad7923_update_scan_mode(struct iio_dev *indio_dev,
+	const unsigned long *active_scan_mask);
+#else /* CONFIG_IIO_BUFFER */
+static inline int
+ad7923_register_ring_funcs_and_init(struct iio_dev *indio_dev)
+{
+	return 0;
+}
+static inline void ad7923_ring_cleanup(struct iio_dev *indio_dev)
+{
+}
+#define ad7923_update_scan_mode NULL
+#endif /* CONFIG_IIO_BUFFER */
+#endif /* IIO_ADC_AD7923_H_ */
diff -uN linux-3.7.1/drivers/staging/iio/adc/ad7923_core.c linux/drivers/staging/iio/adc/ad7923_core.c
--- linux-3.7.1/drivers/staging/iio/adc/ad7923_core.c	1970-01-01 01:00:00.000000000 +0100
+++ linux/drivers/staging/iio/adc/ad7923_core.c	2013-01-05 17:54:11.000000000 +0100
@@ -0,0 +1,202 @@
+/*
+ * AD7923 SPI ADC driver
+ *
+ * Copyright 2011 Analog Devices Inc (from AD7298 Driver)
+ * Copyright 2012 CS Systemes d'Information
+ *
+ * Licensed under the GPL-2.
+ */
+
+#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/module.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/buffer.h>
+
+#include "ad7923.h"
+
+#define AD7923_V_CHAN(index)						\
+	{								\
+		.type = IIO_VOLTAGE,					\
+		.indexed = 1,						\
+		.channel = index,					\
+		.info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT |		\
+		IIO_CHAN_INFO_SCALE_SHARED_BIT,				\
+		.address = index,					\
+		.scan_index = index,					\
+		.scan_type = {						\
+			.sign = 'u',					\
+			.realbits = 12,					\
+			.storagebits = 16,				\
+		},							\
+	}
+
+static struct iio_chan_spec ad7923_channels[] = {
+	AD7923_V_CHAN(0),
+	AD7923_V_CHAN(1),
+	AD7923_V_CHAN(2),
+	AD7923_V_CHAN(3),
+	IIO_CHAN_SOFT_TIMESTAMP(4),
+};
+
+static int ad7923_scan_direct(struct ad7923_state *st, unsigned ch)
+{
+	int ret, cmd;
+
+	cmd = AD7923_WRITE_CR | AD7923_PM_MODE_WRITE(AD7923_PM_MODE_OPS) |
+		AD7923_SEQUENCE_WRITE(AD7923_SEQUENCE_OFF) | AD7923_CODING |
+		AD7923_CHANNEL_WRITE(ch) | AD7923_RANGE;
+	cmd <<= AD7923_SHIFT_REGISTER;
+	st->tx_buf[0] = cpu_to_be16(cmd);
+
+	ret = spi_sync(st->spi, &st->scan_single_msg);
+	if (ret)
+		return ret;
+
+	return be16_to_cpu(st->rx_buf[0]);
+}
+
+static int ad7923_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int *val,
+			   int *val2,
+			   long m)
+{
+	int ret;
+	struct ad7923_state *st = iio_priv(indio_dev);
+
+	switch (m) {
+	case IIO_CHAN_INFO_RAW:
+		mutex_lock(&indio_dev->mlock);
+		if (iio_buffer_enabled(indio_dev))
+			ret = -EBUSY;
+		else
+			ret = ad7923_scan_direct(st, chan->address);
+		mutex_unlock(&indio_dev->mlock);
+
+		if (ret < 0)
+			return ret;
+		if (chan->address == EXTRACT(ret, 12, 4)) {
+			*val = EXTRACT(ret, 0, 12);
+			*val2 = EXTRACT_PERCENT(*val, 12);
+		}
+		return IIO_VAL_INT;
+	}
+	return -EINVAL;
+}
+
+static const struct iio_info ad7923_info = {
+	.read_raw = &ad7923_read_raw,
+	.update_scan_mode = ad7923_update_scan_mode,
+	.driver_module = THIS_MODULE,
+};
+
+static int __devinit ad7923_probe(struct spi_device *spi)
+{
+	struct ad7923_state *st;
+	struct device *dev = &spi->dev;
+	int ret;
+	struct iio_dev *indio_dev = iio_device_alloc(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;
+	}
+
+	spi_set_drvdata(spi, indio_dev);
+
+	st->spi = spi;
+
+	indio_dev->name = spi_get_device_id(spi)->name;
+	indio_dev->dev.parent = &spi->dev;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = ad7923_channels;
+	indio_dev->num_channels = ARRAY_SIZE(ad7923_channels);
+	indio_dev->info = &ad7923_info;
+
+	/* 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].rx_buf = &st->rx_buf[0];
+	st->scan_single_xfer[1].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);
+
+	ret = ad7923_register_ring_funcs_and_init(indio_dev);
+	if (ret)
+		goto error_disable_reg;
+
+	ret = iio_device_register(indio_dev);
+	if (ret)
+		goto error_cleanup_ring;
+
+	dev_info(dev, "Driver AD7923 is added.\n");
+
+	return 0;
+
+error_cleanup_ring:
+	ad7923_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);
+	iio_device_free(indio_dev);
+
+	return ret;
+}
+
+static int __devexit ad7923_remove(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev = spi_get_drvdata(spi);
+	struct ad7923_state *st = iio_priv(indio_dev);
+
+	iio_device_unregister(indio_dev);
+	ad7923_ring_cleanup(indio_dev);
+	if (!IS_ERR(st->reg)) {
+		regulator_disable(st->reg);
+		regulator_put(st->reg);
+	}
+	iio_device_free(indio_dev);
+
+	return 0;
+}
+
+static const struct spi_device_id ad7923_id[] = {
+	{"ad7923", 0},
+	{}
+};
+MODULE_DEVICE_TABLE(spi, ad7923_id);
+
+static struct spi_driver ad7923_driver = {
+	.driver = {
+		.name	= "ad7923",
+		.owner	= THIS_MODULE,
+	},
+	.probe		= ad7923_probe,
+	.remove		= __devexit_p(ad7923_remove),
+	.id_table	= ad7923_id,
+};
+module_spi_driver(ad7923_driver);
+
+MODULE_AUTHOR("Patrick Vasseur <patrick.vasseur@c-s.fr>");
+MODULE_DESCRIPTION("Analog Devices AD7923 ADC");
+MODULE_LICENSE("GPL v2");
diff -uN linux-3.7.1/drivers/staging/iio/adc/ad7923_ring.c linux/drivers/staging/iio/adc/ad7923_ring.c
--- linux-3.7.1/drivers/staging/iio/adc/ad7923_ring.c	1970-01-01 01:00:00.000000000 +0100
+++ linux/drivers/staging/iio/adc/ad7923_ring.c	2013-01-05 17:51:47.000000000 +0100
@@ -0,0 +1,113 @@
+/*
+ * AD7923 SPI ADC driver
+ *
+ * Copyright 2011 Analog Devices Inc (from AD7298 Driver)
+ * Copyright 2012 CS Systemes d'Information
+ *
+ * Licensed under the GPL-2.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/spi/spi.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+#include "ad7923.h"
+
+/**
+ * ad7923_update_scan_mode() setup the spi transfer buffer for the new scan mask
+ **/
+int ad7923_update_scan_mode(struct iio_dev *indio_dev,
+	const unsigned long *active_scan_mask)
+{
+	struct ad7923_state *st = iio_priv(indio_dev);
+	int i, cmd, channel;
+	int scan_count;
+
+	/* Now compute overall size */
+	for (i = 0, channel = 0; i < AD7923_MAX_CHAN; i++)
+		if (test_bit(i, active_scan_mask))
+			channel = i;
+
+	cmd = AD7923_WRITE_CR | AD7923_CODING | AD7923_RANGE |
+		AD7923_PM_MODE_WRITE(AD7923_PM_MODE_OPS) |
+		AD7923_SEQUENCE_WRITE(AD7923_SEQUENCE_ON) |
+		AD7923_CHANNEL_WRITE(channel);
+	cmd <<= AD7923_SHIFT_REGISTER;
+	st->tx_buf[0] = cpu_to_be16(cmd);
+
+	/* 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;
+
+	spi_message_init(&st->ring_msg);
+	spi_message_add_tail(&st->ring_xfer[0], &st->ring_msg);
+
+	for (i = 0; i < (channel + 1); i++) {
+		st->ring_xfer[i + 1].rx_buf = &st->rx_buf[i];
+		st->ring_xfer[i + 1].len = 2;
+		st->ring_xfer[i + 1].cs_change = 1;
+		spi_message_add_tail(&st->ring_xfer[i + 1], &st->ring_msg);
+	}
+	/* make sure last transfer cs_change is not set */
+	st->ring_xfer[i + 1].cs_change = 0;
+
+	return 0;
+}
+
+/**
+ * ad7923_trigger_handler() bh of trigger launched polling to ring buffer
+ *
+ * Currently there is no option in this driver to disable the saving of
+ * timestamps within the ring.
+ **/
+static irqreturn_t ad7923_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct ad7923_state *st = iio_priv(indio_dev);
+	s64 time_ns = 0;
+	__u16 buf[16];
+	int b_sent, i, channel;
+
+	b_sent = spi_sync(st->spi, &st->ring_msg);
+	if (b_sent)
+		goto done;
+
+	if (indio_dev->scan_timestamp) {
+		time_ns = iio_get_time_ns();
+		memcpy((u8 *)buf + indio_dev->scan_bytes - sizeof(s64),
+			&time_ns, sizeof(time_ns));
+	}
+
+	for (i = 0, channel = 0; i < AD7923_MAX_CHAN; i++)
+		if (test_bit(i, indio_dev->active_scan_mask))
+			channel = i;
+
+	for (i = 0; i < (channel + 1); i++)
+		buf[i] = be16_to_cpu(st->rx_buf[i]);
+
+	iio_push_to_buffer(indio_dev->buffer, (u8 *)buf);
+
+done:
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+int ad7923_register_ring_funcs_and_init(struct iio_dev *indio_dev)
+{
+	return iio_triggered_buffer_setup(indio_dev, NULL,
+			&ad7923_trigger_handler, NULL);
+}
+
+void ad7923_ring_cleanup(struct iio_dev *indio_dev)
+{
+	iio_triggered_buffer_cleanup(indio_dev);
+}

Re: [PATCH] IIO ADC support for AD7923

[Alessandro Rubini]

> +config AD7923

I wonder if IIO config symbols should have IIO_ in their name, so
people looking at config files knows what they actually are.
Actually, all USB drivers have USB in their config name, which is
useful even if e.g. "PL2303" cannot be but USB.

On the other hand, AD7923 and all the others can well be driven by
something else than IIO, even if currently this is the only mainstream
option. Here comedi made the right choice, and all their symbols are
in the CONFIG_COMEDI_ name space.

Thanks
/alessandro

Re: [PATCH] IIO ADC support for AD7923

[Lars-Peter Clausen]

On 01/08/2013 09:42 AM, Christophe Leroy wrote:
> This patch adds support for Analog Devices AD7923 ADC in the IIO Subsystem.
> 
> Signed-off-by: Patrick Vasseur <patrick.vasseur@c-s.fr>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>

Hi,

Thanks for the driver, looks pretty good. Some comments inline.

- Lars

> 
> diff -uN linux-3.7.1/drivers/staging/iio/adc/Kconfig linux/drivers/staging/iio/adc/Kconfig
> --- linux-3.7.1/drivers/staging/iio/adc/Kconfig	2012-12-17 20:14:54.000000000 +0100
> +++ linux/drivers/staging/iio/adc/Kconfig	2012-12-13 19:52:10.000000000 +0100

New IIO drivers should not be added to staging, unless there is a very good
reason. Since this driver does not have any major issues it should directly go
into drivers/iio/

[...]

> diff -uN linux-3.7.1/drivers/staging/iio/adc/ad7923.h linux/drivers/staging/iio/adc/ad7923.h
> --- linux-3.7.1/drivers/staging/iio/adc/ad7923.h	1970-01-01 01:00:00.000000000 +0100
> +++ linux/drivers/staging/iio/adc/ad7923.h	2013-01-05 17:53:53.000000000 +0100
> @@ -0,0 +1,72 @@
> +/*
> + * AD7923 SPI ADC driver
> + *
> + * Copyright 2011 Analog Devices Inc (from AD7298 Driver)
> + * Copyright 2012 CS Systemes d'Information
> + *
> + * Licensed under the GPL-2 or later.
> + */
> +#ifndef IIO_ADC_AD7923_H_
> +#define IIO_ADC_AD7923_H_
> +
> +#define AD7923_WRITE_CR		(1 << 11)	/* write control register */
> +#define AD7923_RANGE		(1 << 1)	/* range to REFin */
> +#define AD7923_CODING		(1 << 0)	/* coding is straight binary */
> +#define AD7923_PM_MODE_AS	(1)		/* auto shutdown */
> +#define AD7923_PM_MODE_FS	(2)		/* full shutdown */
> +#define AD7923_PM_MODE_OPS	(3)		/* normal operation */
> +#define AD7923_CHANNEL_0	(0)		/* analog input 0 */
> +#define AD7923_CHANNEL_1	(1)		/* analog input 1 */
> +#define AD7923_CHANNEL_2	(2)		/* analog input 2 */
> +#define AD7923_CHANNEL_3	(3)		/* analog input 3 */
> +#define AD7923_SEQUENCE_OFF	(0)		/* no sequence fonction */
> +#define AD7923_SEQUENCE_PROTECT	(2)		/* no interrupt write cycle */
> +#define AD7923_SEQUENCE_ON	(3)		/* continuous sequence */
> +
> +#define AD7923_MAX_CHAN		4
> +
> +#define AD7923_PM_MODE_WRITE(mode)	(mode << 4)	/* write mode */
> +#define AD7923_CHANNEL_WRITE(channel)	(channel << 6)	/* write channel */
> +#define AD7923_SEQUENCE_WRITE(sequence)	(((sequence & 1) << 3) \
> +					+ ((sequence & 2) << 9))
> +						/* write sequence fonction */
> +/* left shift for CR : bit 11 transmit in first */
> +#define AD7923_SHIFT_REGISTER	4
> +
> +/* val = value, dec = left shift, bits = number of bits of the mask */
> +#define EXTRACT(val, dec, bits)		((val >> dec) & ((1 << bits) - 1))
> +/* val = value, bits = number of bits of the original value */
> +#define EXTRACT_PERCENT(val, bits)	(((val + 1) * 100) >> bits)
> +
> +struct ad7923_state {
> +	struct spi_device		*spi;
> +	struct regulator		*reg;
> +	struct spi_transfer		ring_xfer[6];
> +	struct spi_transfer		scan_single_xfer[2];
> +	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[4] ____cacheline_aligned;
> +	unsigned short			tx_buf[2];

both buffers should of type __be16

> +};
> +
> +#ifdef CONFIG_IIO_BUFFER
> +int ad7923_register_ring_funcs_and_init(struct iio_dev *indio_dev);
> +void ad7923_ring_cleanup(struct iio_dev *indio_dev);
> +int ad7923_update_scan_mode(struct iio_dev *indio_dev,
> +	const unsigned long *active_scan_mask);
> +#else /* CONFIG_IIO_BUFFER */
> +static inline int
> +ad7923_register_ring_funcs_and_init(struct iio_dev *indio_dev)
> +{
> +	return 0;
> +}
> +static inline void ad7923_ring_cleanup(struct iio_dev *indio_dev)
> +{
> +}
> +#define ad7923_update_scan_mode NULL
> +#endif /* CONFIG_IIO_BUFFER */
> +#endif /* IIO_ADC_AD7923_H_ */
> diff -uN linux-3.7.1/drivers/staging/iio/adc/ad7923_core.c linux/drivers/staging/iio/adc/ad7923_core.c
> --- linux-3.7.1/drivers/staging/iio/adc/ad7923_core.c	1970-01-01 01:00:00.000000000 +0100
> +++ linux/drivers/staging/iio/adc/ad7923_core.c	2013-01-05 17:54:11.000000000 +0100
> @@ -0,0 +1,202 @@
> +/*
> + * AD7923 SPI ADC driver
> + *
> + * Copyright 2011 Analog Devices Inc (from AD7298 Driver)
> + * Copyright 2012 CS Systemes d'Information
> + *
> + * Licensed under the GPL-2.
> + */
> +
> +#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/module.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/buffer.h>
> +
> +#include "ad7923.h"
> +
> +#define AD7923_V_CHAN(index)						\
> +	{								\
> +		.type = IIO_VOLTAGE,					\
> +		.indexed = 1,						\
> +		.channel = index,					\
> +		.info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT |		\
> +		IIO_CHAN_INFO_SCALE_SHARED_BIT,				\
> +		.address = index,					\
> +		.scan_index = index,					\
> +		.scan_type = {						\
> +			.sign = 'u',					\
> +			.realbits = 12,					\
> +			.storagebits = 16,				\
> +		},							\
> +	}
> +
> +static struct iio_chan_spec ad7923_channels[] = {

static const struct

> +	AD7923_V_CHAN(0),
> +	AD7923_V_CHAN(1),
> +	AD7923_V_CHAN(2),
> +	AD7923_V_CHAN(3),
> +	IIO_CHAN_SOFT_TIMESTAMP(4),
> +};
> +
[...]
> +
> +static int ad7923_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val,
> +			   int *val2,
> +			   long m)
> +{
> +	int ret;
> +	struct ad7923_state *st = iio_priv(indio_dev);
> +
> +	switch (m) {
> +	case IIO_CHAN_INFO_RAW:
> +		mutex_lock(&indio_dev->mlock);
> +		if (iio_buffer_enabled(indio_dev))
> +			ret = -EBUSY;
> +		else
> +			ret = ad7923_scan_direct(st, chan->address);
> +		mutex_unlock(&indio_dev->mlock);
> +
> +		if (ret < 0)
> +			return ret;
> +		if (chan->address == EXTRACT(ret, 12, 4)) {
> +			*val = EXTRACT(ret, 0, 12);
> +			*val2 = EXTRACT_PERCENT(*val, 12);

val2 is never used in the upper layers in this case.

> +		}

If the address does not match you should probably return an error

> +		return IIO_VAL_INT;
> +	}

How about also reporting the scale attribute?

> +	return -EINVAL;
> +}
> +
> +static const struct iio_info ad7923_info = {
> +	.read_raw = &ad7923_read_raw,
> +	.update_scan_mode = ad7923_update_scan_mode,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +static int __devinit ad7923_probe(struct spi_device *spi)

__devinit/__devexit is being phased out in upstream, it should not be used in
new drivers anymore.

> +{
> +	struct ad7923_state *st;
> +	struct device *dev = &spi->dev;
> +	int ret;
> +	struct iio_dev *indio_dev = iio_device_alloc(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)) {

If the regulator could not be requested return an error.

> +		ret = regulator_enable(st->reg);
> +		if (ret)
> +			goto error_put_reg;
> +	}
> +
> +	spi_set_drvdata(spi, indio_dev);
> +
> +	st->spi = spi;
> +
> +	indio_dev->name = spi_get_device_id(spi)->name;
> +	indio_dev->dev.parent = &spi->dev;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = ad7923_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(ad7923_channels);
> +	indio_dev->info = &ad7923_info;
> +
> +	/* 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].rx_buf = &st->rx_buf[0];
> +	st->scan_single_xfer[1].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);
> +
> +	ret = ad7923_register_ring_funcs_and_init(indio_dev);
> +	if (ret)
> +		goto error_disable_reg;
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret)
> +		goto error_cleanup_ring;
> +
> +	dev_info(dev, "Driver AD7923 is added.\n");

This line is just noise, please remove it.

> +
> +	return 0;
> +
> +error_cleanup_ring:
> +	ad7923_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);
> +	iio_device_free(indio_dev);
> +
> +	return ret;
> +}
> +
> +static int __devexit ad7923_remove(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> +	struct ad7923_state *st = iio_priv(indio_dev);
> +
> +	iio_device_unregister(indio_dev);
> +	ad7923_ring_cleanup(indio_dev);
> +	if (!IS_ERR(st->reg)) {
> +		regulator_disable(st->reg);
> +		regulator_put(st->reg);
> +	}
> +	iio_device_free(indio_dev);
> +
> +	return 0;
> +}
> +
> +static const struct spi_device_id ad7923_id[] = {
> +	{"ad7923", 0},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(spi, ad7923_id);
> +
> +static struct spi_driver ad7923_driver = {
> +	.driver = {
> +		.name	= "ad7923",
> +		.owner	= THIS_MODULE,
> +	},
> +	.probe		= ad7923_probe,
> +	.remove		= __devexit_p(ad7923_remove),
> +	.id_table	= ad7923_id,
> +};
> +module_spi_driver(ad7923_driver);
> +
> +MODULE_AUTHOR("Patrick Vasseur <patrick.vasseur@c-s.fr>");
> +MODULE_DESCRIPTION("Analog Devices AD7923 ADC");
> +MODULE_LICENSE("GPL v2");
> diff -uN linux-3.7.1/drivers/staging/iio/adc/ad7923_ring.c linux/drivers/staging/iio/adc/ad7923_ring.c
> --- linux-3.7.1/drivers/staging/iio/adc/ad7923_ring.c	1970-01-01 01:00:00.000000000 +0100
> +++ linux/drivers/staging/iio/adc/ad7923_ring.c	2013-01-05 17:51:47.000000000 +0100

Considering the overall size of the driver I think it makes sense to put it all
in just one file.

> @@ -0,0 +1,113 @@
> +/*
> + * AD7923 SPI ADC driver
> + *
> + * Copyright 2011 Analog Devices Inc (from AD7298 Driver)
> + * Copyright 2012 CS Systemes d'Information
> + *
> + * Licensed under the GPL-2.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/spi/spi.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +
> +#include "ad7923.h"
> +
> +/**
> + * ad7923_update_scan_mode() setup the spi transfer buffer for the new scan mask
> + **/
> +int ad7923_update_scan_mode(struct iio_dev *indio_dev,
> +	const unsigned long *active_scan_mask)
> +{
> +	struct ad7923_state *st = iio_priv(indio_dev);
> +	int i, cmd, channel;
> +	int scan_count;
> +
> +	/* Now compute overall size */
> +	for (i = 0, channel = 0; i < AD7923_MAX_CHAN; i++)
> +		if (test_bit(i, active_scan_mask))
> +			channel = i;
> +
> +	cmd = AD7923_WRITE_CR | AD7923_CODING | AD7923_RANGE |
> +		AD7923_PM_MODE_WRITE(AD7923_PM_MODE_OPS) |
> +		AD7923_SEQUENCE_WRITE(AD7923_SEQUENCE_ON) |
> +		AD7923_CHANNEL_WRITE(channel);

Hm, ok this looks a bit strange. You lookup the first channel which is selected
and then also sample all channels which come before it. E.g. if only channel 2
is selected you sample channel 0, 1 and 2. In the trigger handler you push the
buffer to the IIO core. But in your buffer you have the result of channel 0 in
the position where IIO would expect channel 2.
I think it is better if you send a cmd for each channel that needs to be
samples. E.g.:

	if (for_each_set_bit(i, active_scan_mask, AD7923_MAX_CHAN)) {
		cmd = AD7923_WRITE_CR | AD7923_CODING | AD7923_RANGE |
			AD7923_PM_MODE_WRITE(AD7923_PM_MODE_OPS) |
			AD7923_SEQUENCE_WRITE(AD7923_SEQUENCE_OFF) |
			AD7923_CHANNEL_WRITE(i);
		cmd <<= AD7923_SHIFT_REGISTER;
		st->tx_buf[j++] = cpu_to_be16(cmd);
	}


> +	cmd <<= AD7923_SHIFT_REGISTER;
> +	st->tx_buf[0] = cpu_to_be16(cmd);
> +
> +	/* 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;
> +
> +	spi_message_init(&st->ring_msg);
> +	spi_message_add_tail(&st->ring_xfer[0], &st->ring_msg);
> +
> +	for (i = 0; i < (channel + 1); i++) {
> +		st->ring_xfer[i + 1].rx_buf = &st->rx_buf[i];
> +		st->ring_xfer[i + 1].len = 2;
> +		st->ring_xfer[i + 1].cs_change = 1;
> +		spi_message_add_tail(&st->ring_xfer[i + 1], &st->ring_msg);
> +	}
> +	/* make sure last transfer cs_change is not set */
> +	st->ring_xfer[i + 1].cs_change = 0;
> +
> +	return 0;
> +}
> +
> +/**
> + * ad7923_trigger_handler() bh of trigger launched polling to ring buffer
> + *
> + * Currently there is no option in this driver to disable the saving of
> + * timestamps within the ring.
> + **/
> +static irqreturn_t ad7923_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct ad7923_state *st = iio_priv(indio_dev);
> +	s64 time_ns = 0;
> +	__u16 buf[16];
> +	int b_sent, i, channel;
> +
> +	b_sent = spi_sync(st->spi, &st->ring_msg);
> +	if (b_sent)
> +		goto done;
> +
> +	if (indio_dev->scan_timestamp) {
> +		time_ns = iio_get_time_ns();
> +		memcpy((u8 *)buf + indio_dev->scan_bytes - sizeof(s64),
> +			&time_ns, sizeof(time_ns));
> +	}
> +
> +	for (i = 0, channel = 0; i < AD7923_MAX_CHAN; i++)
> +		if (test_bit(i, indio_dev->active_scan_mask))
> +			channel = i;
> +
> +	for (i = 0; i < (channel + 1); i++)
> +		buf[i] = be16_to_cpu(st->rx_buf[i]);

No need to perform the endianness conversion here. Just mark the channel as IIO_BE.

> +
> +	iio_push_to_buffer(indio_dev->buffer, (u8 *)buf);
> +
> +done:
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
[...]

Re: [PATCH] IIO ADC support for AD7923

[Jonathan Cameron]

On 01/08/2013 09:14 AM, Alessandro Rubini wrote:
>> +config AD7923
> 
> I wonder if IIO config symbols should have IIO_ in their name, so
> people looking at config files knows what they actually are.
> Actually, all USB drivers have USB in their config name, which is
> useful even if e.g. "PL2303" cannot be but USB.
> 
> On the other hand, AD7923 and all the others can well be driven by
> something else than IIO, even if currently this is the only mainstream
> option. Here comedi made the right choice, and all their symbols are
> in the CONFIG_COMEDI_ name space.
> 
> Thanks
> /alessandro
I have no particular problem with this for new drivers, but
obviously it is an interface change for older ones that may
cause some issues.  Anyone else care either way?

Various patches to at least kill off missleading config symbol
names (SENSOR_ which is used hwmon) have been kicking around but
I admit I keep putting them on the back burner on the basis there
are better things to be getting on with and it will cause a fair bit
of churn.

Re: [PATCH] IIO ADC support for AD7923

[Lars-Peter Clausen]

On 01/08/2013 10:14 AM, Alessandro Rubini wrote:
>> +config AD7923
> 
> I wonder if IIO config symbols should have IIO_ in their name, so
> people looking at config files knows what they actually are.
> Actually, all USB drivers have USB in their config name, which is
> useful even if e.g. "PL2303" cannot be but USB.
> 
> On the other hand, AD7923 and all the others can well be driven by
> something else than IIO, even if currently this is the only mainstream
> option.

No, IIO is and should be the only option for these devices. We really don't
want to drivers for the same device in different subsystems of the kernel.

The other argument is still valid though.

> Here comedi made the right choice, and all their symbols are
> in the CONFIG_COMEDI_ name space.

- lars

Re: [PATCH] IIO ADC support for AD7923

[Alessandro Rubini]

Thank you Jonathan for replying, I apologize for the delay.

>> I wonder if IIO config symbols should have IIO_ in their name, [...]

> I have no particular problem with this for new drivers, but
> obviously it is an interface change for older ones that may
> cause some issues.

Yes, that's right. I just raised a problem, though a minor one.

thanks
/alessandro

Re: [PATCH] IIO ADC support for AD7923

[Alessandro Rubini]

>> I wonder if IIO config symbols should have IIO_ in their name, so
>> people looking at config files knows what they actually are.
>> Actually, all USB drivers have USB in their config name, which is
>> useful even if e.g. "PL2303" cannot be but USB.
>> 
>> On the other hand, AD7923 and all the others can well be driven by
>> something else than IIO, even if currently this is the only mainstream
>> option.
> 
> No, IIO is and should be the only option for these devices. We really don't
> want to drivers for the same device in different subsystems of the kernel.

We really don't want a non-USB driver for a USB device, but still the
name is in the config symbol and is useful. And this is an hardware
constraint.

And your argument may well have applied to comedi. But they wisely
made the right choice and allowed iio to take over and possibly drive
the same devices.

But sure your code is perfect and no other subsystem will ever be needed.


To explain, this is the problem I had: IIO was still in staging, and I
had to use the ADC in the at91sam9g20.  The platform data there was
protected by "IS_ENABLED(CONFIG_AT91_ADC)", which is actually only
selectable if IIO is configured in.

What should I do if I wanted to drive directly the hardware with a
simple custom thing, because, for example, my target board is very
small?  Currently I run a patched kernel, because otherwise I can't
load my custom module and use the already-available platform data.
No, it's not only a name change, it's that the description of the
hardware should not depend on which subsystems are selected.


But yes, you are right. I'm working on another I/O subsystem. We are
gong to release zio-1.0 in a few days, because the thing is mature
and used in production.

I hope to meet you in person at fosdem and be able to talk over a beer
or two.

regards
/alessandro

Re: [PATCH] IIO ADC support for AD7923

[Lars-Peter Clausen]

On 01/17/2013 06:11 PM, Alessandro Rubini wrote:
>>> I wonder if IIO config symbols should have IIO_ in their name, so
>>> people looking at config files knows what they actually are.
>>> Actually, all USB drivers have USB in their config name, which is
>>> useful even if e.g. "PL2303" cannot be but USB.
>>>
>>> On the other hand, AD7923 and all the others can well be driven by
>>> something else than IIO, even if currently this is the only mainstream
>>> option.
>>
>> No, IIO is and should be the only option for these devices. We really don't
>> want to drivers for the same device in different subsystems of the kernel.
> 
> We really don't want a non-USB driver for a USB device, but still the
> name is in the config symbol and is useful. And this is an hardware
> constraint.
> 
> And your argument may well have applied to comedi. But they wisely
> made the right choice and allowed iio to take over and possibly drive
> the same devices.
> 
> But sure your code is perfect and no other subsystem will ever be needed.
> 

I'm not implying IIO is perfect. IIO has certain shortcomings and some of
these shortcomings have been addressed in ZIO.

Still it's a very bad idea to have two subsystem which have a huge overlap
in both functionality and targeted devices. It will gives us all lots of
headaches later on. As IIO continues to evolve it will get support for some
of the features that only ZIO supports at the moment and as ZIO grows it
will get support for features currently only supported by IIO. So in the end
we have two frameworks for the very same purpose.

Btw. I'd be arguing the same way if ZIO was currently inside the kernel and
Jonathan came along and proposed to merge IIO.

> 
> [...]
>
> But yes, you are right. I'm working on another I/O subsystem. We are
> gong to release zio-1.0 in a few days, because the thing is mature
> and used in production.
> 
> I hope to meet you in person at fosdem and be able to talk over a beer
> or two.
> 

Looking forward to meeting you :)

- Lars

Re: [PATCH] IIO ADC support for AD7923

[Alessandro Rubini]

Thank you Lars for you constructive attitude.

See you at fosdem

/alessandro

Re: [PATCH] IIO ADC support for AD7923

[Getz, Robin]

On Thu 17 Jan 2013 12:36, Lars-Peter Clausen pondered:
> On 01/17/2013 06:11 PM, Alessandro Rubini wrote:
> > [...]
> >
> > But yes, you are right. I'm working on another I/O subsystem. We are
> > gong to release zio-1.0 in a few days, because the thing is mature
> > and used in production.

Neither means it's a good idea for upstream :)

> Still it's a very bad idea to have two subsystem which have a huge overlap
> in both functionality and targeted devices. It will gives us all lots of
> headaches later on. As IIO continues to evolve it will get support for some
> of the features that only ZIO supports at the moment and as ZIO grows it
> will get support for features currently only supported by IIO. So in the
> end we have two frameworks for the very same purpose.

I want to strongly agree with Lars-Peter. Lets work together on one thing - 
which tries to solve all the our system level issues. As an end user - I 
don't want to re-write userspace for multiple interfaces to the same 
underlying ADC/DACs.

I don't know how Greg feels about another subsystem in the kernel which 
duplicates existing functionality/targetted devices - but it doesn't sound 
like a good idea to me.

> > I hope to meet you in person at fosdem and be able to talk over a beer
> > or two.
>
> Looking forward to meeting you :)

Hopefully you can come to some logical conclusions over a friendly beverage. 
Even if you can't decide on how to merge things (plan for adding missing 
features from one to the other), maybe it's just deciding on how to get as 
much reuse as possible (duplication of device register and bit definitions?)

-Robin

Re: [PATCH] IIO ADC support for AD7923

[Jonathan Cameron]

On 01/18/2013 11:02 PM, Getz, Robin wrote:
> On Thu 17 Jan 2013 12:36, Lars-Peter Clausen pondered:
>> On 01/17/2013 06:11 PM, Alessandro Rubini wrote:
>>> [...]
>>>
>>> But yes, you are right. I'm working on another I/O subsystem. We are
>>> gong to release zio-1.0 in a few days, because the thing is mature
>>> and used in production.
> 
> Neither means it's a good idea for upstream :)
> 
>> Still it's a very bad idea to have two subsystem which have a huge overlap
>> in both functionality and targeted devices. It will gives us all lots of
>> headaches later on. As IIO continues to evolve it will get support for some
>> of the features that only ZIO supports at the moment and as ZIO grows it
>> will get support for features currently only supported by IIO. So in the
>> end we have two frameworks for the very same purpose.
> 
> I want to strongly agree with Lars-Peter. Lets work together on one thing - 
> which tries to solve all the our system level issues. As an end user - I 
> don't want to re-write userspace for multiple interfaces to the same 
> underlying ADC/DACs.
> 
> I don't know how Greg feels about another subsystem in the kernel which 
> duplicates existing functionality/targetted devices - but it doesn't sound 
> like a good idea to me.
> 
>>> I hope to meet you in person at fosdem and be able to talk over a beer
>>> or two.
>>
>> Looking forward to meeting you :)
> 
> Hopefully you can come to some logical conclusions over a friendly beverage. 
> Even if you can't decide on how to merge things (plan for adding missing 
> features from one to the other), maybe it's just deciding on how to get as 
> much reuse as possible (duplication of device register and bit definitions?)
> 

I second these comments from Lars and Robin (or third I suppose ;).
Lets get the best possible result. The source of an idea or code
really doesn't matter in the long run, what matters is that we
get a solution that works well for all users.

Enjoy those beverages (and fosdem of course!)

Jonathan

Re: [PATCH] IIO ADC support for AD7923

[christophe leroy]

Hi Lars,

Sorry to respond so late, I've been very busy lately.
Please see answers/questions below

Main point is that our driver is mainly copied and adapted from AD7298 
driver, and your comments are on things that we have not modified from 
AD7298, so what should we do really ?

We are a bit new comers in Kernel Development, so thanks for your help.

Christophe

Le 08/01/2013 11:27, Lars-Peter Clausen a écrit :
> On 01/08/2013 09:42 AM, Christophe Leroy wrote:
>> This patch adds support for Analog Devices AD7923 ADC in the IIO Subsystem.
>>
>> Signed-off-by: Patrick Vasseur <patrick.vasseur@c-s.fr>
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> Hi,
>
> Thanks for the driver, looks pretty good. Some comments inline.
>
> - Lars
>
>> diff -uN linux-3.7.1/drivers/staging/iio/adc/Kconfig linux/drivers/staging/iio/adc/Kconfig
>> --- linux-3.7.1/drivers/staging/iio/adc/Kconfig	2012-12-17 20:14:54.000000000 +0100
>> +++ linux/drivers/staging/iio/adc/Kconfig	2012-12-13 19:52:10.000000000 +0100
> New IIO drivers should not be added to staging, unless there is a very good
> reason. Since this driver does not have any major issues it should directly go
> into drivers/iio/
Our driver is based on AD7298 driver, which is today in staging. Should 
we put ours in drivers/iio/ directly anyway ?
>
> [...]
>
>> diff -uN linux-3.7.1/drivers/staging/iio/adc/ad7923.h linux/drivers/staging/iio/adc/ad7923.h
>> --- linux-3.7.1/drivers/staging/iio/adc/ad7923.h	1970-01-01 01:00:00.000000000 +0100
>> +++ linux/drivers/staging/iio/adc/ad7923.h	2013-01-05 17:53:53.000000000 +0100
>> @@ -0,0 +1,72 @@
>> +/*
>> + * AD7923 SPI ADC driver
>> + *
>> + * Copyright 2011 Analog Devices Inc (from AD7298 Driver)
>> + * Copyright 2012 CS Systemes d'Information
>> + *
>> + * Licensed under the GPL-2 or later.
>> + */
>> +#ifndef IIO_ADC_AD7923_H_
>> +#define IIO_ADC_AD7923_H_
>> +
>> +#define AD7923_WRITE_CR		(1 << 11)	/* write control register */
>> +#define AD7923_RANGE		(1 << 1)	/* range to REFin */
>> +#define AD7923_CODING		(1 << 0)	/* coding is straight binary */
>> +#define AD7923_PM_MODE_AS	(1)		/* auto shutdown */
>> +#define AD7923_PM_MODE_FS	(2)		/* full shutdown */
>> +#define AD7923_PM_MODE_OPS	(3)		/* normal operation */
>> +#define AD7923_CHANNEL_0	(0)		/* analog input 0 */
>> +#define AD7923_CHANNEL_1	(1)		/* analog input 1 */
>> +#define AD7923_CHANNEL_2	(2)		/* analog input 2 */
>> +#define AD7923_CHANNEL_3	(3)		/* analog input 3 */
>> +#define AD7923_SEQUENCE_OFF	(0)		/* no sequence fonction */
>> +#define AD7923_SEQUENCE_PROTECT	(2)		/* no interrupt write cycle */
>> +#define AD7923_SEQUENCE_ON	(3)		/* continuous sequence */
>> +
>> +#define AD7923_MAX_CHAN		4
>> +
>> +#define AD7923_PM_MODE_WRITE(mode)	(mode << 4)	/* write mode */
>> +#define AD7923_CHANNEL_WRITE(channel)	(channel << 6)	/* write channel */
>> +#define AD7923_SEQUENCE_WRITE(sequence)	(((sequence & 1) << 3) \
>> +					+ ((sequence & 2) << 9))
>> +						/* write sequence fonction */
>> +/* left shift for CR : bit 11 transmit in first */
>> +#define AD7923_SHIFT_REGISTER	4
>> +
>> +/* val = value, dec = left shift, bits = number of bits of the mask */
>> +#define EXTRACT(val, dec, bits)		((val >> dec) & ((1 << bits) - 1))
>> +/* val = value, bits = number of bits of the original value */
>> +#define EXTRACT_PERCENT(val, bits)	(((val + 1) * 100) >> bits)
>> +
>> +struct ad7923_state {
>> +	struct spi_device		*spi;
>> +	struct regulator		*reg;
>> +	struct spi_transfer		ring_xfer[6];
>> +	struct spi_transfer		scan_single_xfer[2];
>> +	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[4] ____cacheline_aligned;
>> +	unsigned short			tx_buf[2];
> both buffers should of type __be16
This part is copied unmodified from AD7298. Should we modify it ?
>
>> +};
>> +
>> +#ifdef CONFIG_IIO_BUFFER
>> +int ad7923_register_ring_funcs_and_init(struct iio_dev *indio_dev);
>> +void ad7923_ring_cleanup(struct iio_dev *indio_dev);
>> +int ad7923_update_scan_mode(struct iio_dev *indio_dev,
>> +	const unsigned long *active_scan_mask);
>> +#else /* CONFIG_IIO_BUFFER */
>> +static inline int
>> +ad7923_register_ring_funcs_and_init(struct iio_dev *indio_dev)
>> +{
>> +	return 0;
>> +}
>> +static inline void ad7923_ring_cleanup(struct iio_dev *indio_dev)
>> +{
>> +}
>> +#define ad7923_update_scan_mode NULL
>> +#endif /* CONFIG_IIO_BUFFER */
>> +#endif /* IIO_ADC_AD7923_H_ */
>> diff -uN linux-3.7.1/drivers/staging/iio/adc/ad7923_core.c linux/drivers/staging/iio/adc/ad7923_core.c
>> --- linux-3.7.1/drivers/staging/iio/adc/ad7923_core.c	1970-01-01 01:00:00.000000000 +0100
>> +++ linux/drivers/staging/iio/adc/ad7923_core.c	2013-01-05 17:54:11.000000000 +0100
>> @@ -0,0 +1,202 @@
>> +/*
>> + * AD7923 SPI ADC driver
>> + *
>> + * Copyright 2011 Analog Devices Inc (from AD7298 Driver)
>> + * Copyright 2012 CS Systemes d'Information
>> + *
>> + * Licensed under the GPL-2.
>> + */
>> +
>> +#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/module.h>
>> +
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/sysfs.h>
>> +#include <linux/iio/buffer.h>
>> +
>> +#include "ad7923.h"
>> +
>> +#define AD7923_V_CHAN(index)						\
>> +	{								\
>> +		.type = IIO_VOLTAGE,					\
>> +		.indexed = 1,						\
>> +		.channel = index,					\
>> +		.info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT |		\
>> +		IIO_CHAN_INFO_SCALE_SHARED_BIT,				\
>> +		.address = index,					\
>> +		.scan_index = index,					\
>> +		.scan_type = {						\
>> +			.sign = 'u',					\
>> +			.realbits = 12,					\
>> +			.storagebits = 16,				\
>> +		},							\
>> +	}
>> +
>> +static struct iio_chan_spec ad7923_channels[] = {
> static const struct
Copied from AD7298. Do we change it anyway ?
>
>> +	AD7923_V_CHAN(0),
>> +	AD7923_V_CHAN(1),
>> +	AD7923_V_CHAN(2),
>> +	AD7923_V_CHAN(3),
>> +	IIO_CHAN_SOFT_TIMESTAMP(4),
>> +};
>> +
> [...]
>> +
>> +static int ad7923_read_raw(struct iio_dev *indio_dev,
>> +			   struct iio_chan_spec const *chan,
>> +			   int *val,
>> +			   int *val2,
>> +			   long m)
>> +{
>> +	int ret;
>> +	struct ad7923_state *st = iio_priv(indio_dev);
>> +
>> +	switch (m) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		mutex_lock(&indio_dev->mlock);
>> +		if (iio_buffer_enabled(indio_dev))
>> +			ret = -EBUSY;
>> +		else
>> +			ret = ad7923_scan_direct(st, chan->address);
>> +		mutex_unlock(&indio_dev->mlock);
>> +
>> +		if (ret < 0)
>> +			return ret;
>> +		if (chan->address == EXTRACT(ret, 12, 4)) {
>> +			*val = EXTRACT(ret, 0, 12);
>> +			*val2 = EXTRACT_PERCENT(*val, 12);
> val2 is never used in the upper layers in this case.
I don't understand what you mean.
Again, this is copied from AD7298
>
>> +		}
> If the address does not match you should probably return an error
ok
>
>> +		return IIO_VAL_INT;
>> +	}
> How about also reporting the scale attribute?
Ok, we'll try and add it.
>
>> +	return -EINVAL;
>> +}
>> +
>> +static const struct iio_info ad7923_info = {
>> +	.read_raw = &ad7923_read_raw,
>> +	.update_scan_mode = ad7923_update_scan_mode,
>> +	.driver_module = THIS_MODULE,
>> +};
>> +
>> +static int __devinit ad7923_probe(struct spi_device *spi)
> __devinit/__devexit is being phased out in upstream, it should not be used in
> new drivers anymore.
Ok, but still is AD7298.
>
>> +{
>> +	struct ad7923_state *st;
>> +	struct device *dev = &spi->dev;
>> +	int ret;
>> +	struct iio_dev *indio_dev = iio_device_alloc(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)) {
> If the regulator could not be requested return an error.
Ok
>
>> +		ret = regulator_enable(st->reg);
>> +		if (ret)
>> +			goto error_put_reg;
>> +	}
>> +
>> +	spi_set_drvdata(spi, indio_dev);
>> +
>> +	st->spi = spi;
>> +
>> +	indio_dev->name = spi_get_device_id(spi)->name;
>> +	indio_dev->dev.parent = &spi->dev;
>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>> +	indio_dev->channels = ad7923_channels;
>> +	indio_dev->num_channels = ARRAY_SIZE(ad7923_channels);
>> +	indio_dev->info = &ad7923_info;
>> +
>> +	/* 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].rx_buf = &st->rx_buf[0];
>> +	st->scan_single_xfer[1].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);
>> +
>> +	ret = ad7923_register_ring_funcs_and_init(indio_dev);
>> +	if (ret)
>> +		goto error_disable_reg;
>> +
>> +	ret = iio_device_register(indio_dev);
>> +	if (ret)
>> +		goto error_cleanup_ring;
>> +
>> +	dev_info(dev, "Driver AD7923 is added.\n");
> This line is just noise, please remove it.
Ok
>
>> +
>> +	return 0;
>> +
>> +error_cleanup_ring:
>> +	ad7923_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);
>> +	iio_device_free(indio_dev);
>> +
>> +	return ret;
>> +}
>> +
>> +static int __devexit ad7923_remove(struct spi_device *spi)
>> +{
>> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
>> +	struct ad7923_state *st = iio_priv(indio_dev);
>> +
>> +	iio_device_unregister(indio_dev);
>> +	ad7923_ring_cleanup(indio_dev);
>> +	if (!IS_ERR(st->reg)) {
>> +		regulator_disable(st->reg);
>> +		regulator_put(st->reg);
>> +	}
>> +	iio_device_free(indio_dev);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct spi_device_id ad7923_id[] = {
>> +	{"ad7923", 0},
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(spi, ad7923_id);
>> +
>> +static struct spi_driver ad7923_driver = {
>> +	.driver = {
>> +		.name	= "ad7923",
>> +		.owner	= THIS_MODULE,
>> +	},
>> +	.probe		= ad7923_probe,
>> +	.remove		= __devexit_p(ad7923_remove),
>> +	.id_table	= ad7923_id,
>> +};
>> +module_spi_driver(ad7923_driver);
>> +
>> +MODULE_AUTHOR("Patrick Vasseur <patrick.vasseur@c-s.fr>");
>> +MODULE_DESCRIPTION("Analog Devices AD7923 ADC");
>> +MODULE_LICENSE("GPL v2");
>> diff -uN linux-3.7.1/drivers/staging/iio/adc/ad7923_ring.c linux/drivers/staging/iio/adc/ad7923_ring.c
>> --- linux-3.7.1/drivers/staging/iio/adc/ad7923_ring.c	1970-01-01 01:00:00.000000000 +0100
>> +++ linux/drivers/staging/iio/adc/ad7923_ring.c	2013-01-05 17:51:47.000000000 +0100
> Considering the overall size of the driver I think it makes sense to put it all
> in just one file.
Ok, why not, but once again this is from AD7298. We have tried to keep 
things homogeneous. Should we do differently ?
>
>> @@ -0,0 +1,113 @@
>> +/*
>> + * AD7923 SPI ADC driver
>> + *
>> + * Copyright 2011 Analog Devices Inc (from AD7298 Driver)
>> + * Copyright 2012 CS Systemes d'Information
>> + *
>> + * Licensed under the GPL-2.
>> + */
>> +
>> +#include <linux/interrupt.h>
>> +#include <linux/kernel.h>
>> +#include <linux/slab.h>
>> +#include <linux/spi/spi.h>
>> +
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/buffer.h>
>> +#include <linux/iio/trigger_consumer.h>
>> +#include <linux/iio/triggered_buffer.h>
>> +
>> +#include "ad7923.h"
>> +
>> +/**
>> + * ad7923_update_scan_mode() setup the spi transfer buffer for the new scan mask
>> + **/
>> +int ad7923_update_scan_mode(struct iio_dev *indio_dev,
>> +	const unsigned long *active_scan_mask)
>> +{
>> +	struct ad7923_state *st = iio_priv(indio_dev);
>> +	int i, cmd, channel;
>> +	int scan_count;
>> +
>> +	/* Now compute overall size */
>> +	for (i = 0, channel = 0; i < AD7923_MAX_CHAN; i++)
>> +		if (test_bit(i, active_scan_mask))
>> +			channel = i;
>> +
>> +	cmd = AD7923_WRITE_CR | AD7923_CODING | AD7923_RANGE |
>> +		AD7923_PM_MODE_WRITE(AD7923_PM_MODE_OPS) |
>> +		AD7923_SEQUENCE_WRITE(AD7923_SEQUENCE_ON) |
>> +		AD7923_CHANNEL_WRITE(channel);
> Hm, ok this looks a bit strange. You lookup the first channel which is selected
> and then also sample all channels which come before it. E.g. if only channel 2
> is selected you sample channel 0, 1 and 2. In the trigger handler you push the
> buffer to the IIO core. But in your buffer you have the result of channel 0 in
> the position where IIO would expect channel 2.
> I think it is better if you send a cmd for each channel that needs to be
> samples. E.g.:
>
> 	if (for_each_set_bit(i, active_scan_mask, AD7923_MAX_CHAN)) {
> 		cmd = AD7923_WRITE_CR | AD7923_CODING | AD7923_RANGE |
> 			AD7923_PM_MODE_WRITE(AD7923_PM_MODE_OPS) |
> 			AD7923_SEQUENCE_WRITE(AD7923_SEQUENCE_OFF) |
> 			AD7923_CHANNEL_WRITE(i);
> 		cmd <<= AD7923_SHIFT_REGISTER;
> 		st->tx_buf[j++] = cpu_to_be16(cmd);
> 	}
Ok, here we are trying to use the sequence mode. But unlike the AD7298, 
here sequence mode is only able to go from channel 0 to a given channel. 
Hence the reason why we try to identify the highest requested channel, 
then we do a sequential read of all from 0 to this one.

Wouldn't the use on non sequential mode be less performant ?

>
>> +	cmd <<= AD7923_SHIFT_REGISTER;
>> +	st->tx_buf[0] = cpu_to_be16(cmd);
>> +
>> +	/* 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;
>> +
>> +	spi_message_init(&st->ring_msg);
>> +	spi_message_add_tail(&st->ring_xfer[0], &st->ring_msg);
>> +
>> +	for (i = 0; i < (channel + 1); i++) {
>> +		st->ring_xfer[i + 1].rx_buf = &st->rx_buf[i];
>> +		st->ring_xfer[i + 1].len = 2;
>> +		st->ring_xfer[i + 1].cs_change = 1;
>> +		spi_message_add_tail(&st->ring_xfer[i + 1], &st->ring_msg);
>> +	}
>> +	/* make sure last transfer cs_change is not set */
>> +	st->ring_xfer[i + 1].cs_change = 0;
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * ad7923_trigger_handler() bh of trigger launched polling to ring buffer
>> + *
>> + * Currently there is no option in this driver to disable the saving of
>> + * timestamps within the ring.
>> + **/
>> +static irqreturn_t ad7923_trigger_handler(int irq, void *p)
>> +{
>> +	struct iio_poll_func *pf = p;
>> +	struct iio_dev *indio_dev = pf->indio_dev;
>> +	struct ad7923_state *st = iio_priv(indio_dev);
>> +	s64 time_ns = 0;
>> +	__u16 buf[16];
>> +	int b_sent, i, channel;
>> +
>> +	b_sent = spi_sync(st->spi, &st->ring_msg);
>> +	if (b_sent)
>> +		goto done;
>> +
>> +	if (indio_dev->scan_timestamp) {
>> +		time_ns = iio_get_time_ns();
>> +		memcpy((u8 *)buf + indio_dev->scan_bytes - sizeof(s64),
>> +			&time_ns, sizeof(time_ns));
>> +	}
>> +
>> +	for (i = 0, channel = 0; i < AD7923_MAX_CHAN; i++)
>> +		if (test_bit(i, indio_dev->active_scan_mask))
>> +			channel = i;
>> +
>> +	for (i = 0; i < (channel + 1); i++)
>> +		buf[i] = be16_to_cpu(st->rx_buf[i]);
> No need to perform the endianness conversion here. Just mark the channel as IIO_BE.
Ok, why not, but again this comes from AD7298 driver.
>
>> +
>> +	iio_push_to_buffer(indio_dev->buffer, (u8 *)buf);
>> +
>> +done:
>> +	iio_trigger_notify_done(indio_dev->trig);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
> [...]
>

Re: [PATCH] IIO ADC support for AD7923

[Lars-Peter Clausen]

Hi,

On 01/19/2013 04:05 PM, christophe leroy wrote:
> Hi Lars,
> 
> Sorry to respond so late, I've been very busy lately.
> Please see answers/questions below
> 
> Main point is that our driver is mainly copied and adapted from AD7298
> driver, and your comments are on things that we have not modified from
> AD7298, so what should we do really ?

The AD7288 driver has recently seen some updates. The issues which your driver
inherited from the AD7288 driver have been fixed in the original driver. See:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=drivers/iio/adc/ad7298.c;h=b34d754994d50d53f60fee694440658ba0b137dd;hb=HEAD

> 
> We are a bit new comers in Kernel Development, so thanks for your help.
> 
> Christophe
> 
> Le 08/01/2013 11:27, Lars-Peter Clausen a écrit :
>> On 01/08/2013 09:42 AM, Christophe Leroy wrote:
>>> This patch adds support for Analog Devices AD7923 ADC in the IIO
>>> Subsystem.
>>>
>>> Signed-off-by: Patrick Vasseur <patrick.vasseur@c-s.fr>
>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> Hi,
>>
>> Thanks for the driver, looks pretty good. Some comments inline.
>>
>> - Lars
>>
>>> diff -uN linux-3.7.1/drivers/staging/iio/adc/Kconfig
>>> linux/drivers/staging/iio/adc/Kconfig
>>> --- linux-3.7.1/drivers/staging/iio/adc/Kconfig    2012-12-17
>>> 20:14:54.000000000 +0100
>>> +++ linux/drivers/staging/iio/adc/Kconfig    2012-12-13
>>> 19:52:10.000000000 +0100
>> New IIO drivers should not be added to staging, unless there is a very
>> good
>> reason. Since this driver does not have any major issues it should
>> directly go
>> into drivers/iio/
> Our driver is based on AD7298 driver, which is today in staging. Should
> we put ours in drivers/iio/ directly anyway ?
>>
>> [...]
>>
[...]
>>> diff -uN linux-3.7.1/drivers/staging/iio/adc/ad7923_core.c
>>> linux/drivers/staging/iio/adc/ad7923_core.c
>>> --- linux-3.7.1/drivers/staging/iio/adc/ad7923_core.c    1970-01-01
>>> 01:00:00.000000000 +0100
>>> +++ linux/drivers/staging/iio/adc/ad7923_core.c    2013-01-05
>> [...]
>>> +
>>> +static int ad7923_read_raw(struct iio_dev *indio_dev,
>>> +               struct iio_chan_spec const *chan,
>>> +               int *val,
>>> +               int *val2,
>>> +               long m)
>>> +{
>>> +    int ret;
>>> +    struct ad7923_state *st = iio_priv(indio_dev);
>>> +
>>> +    switch (m) {
>>> +    case IIO_CHAN_INFO_RAW:
>>> +        mutex_lock(&indio_dev->mlock);
>>> +        if (iio_buffer_enabled(indio_dev))
>>> +            ret = -EBUSY;
>>> +        else
>>> +            ret = ad7923_scan_direct(st, chan->address);
>>> +        mutex_unlock(&indio_dev->mlock);
>>> +
>>> +        if (ret < 0)
>>> +            return ret;
>>> +        if (chan->address == EXTRACT(ret, 12, 4)) {
>>> +            *val = EXTRACT(ret, 0, 12);
>>> +            *val2 = EXTRACT_PERCENT(*val, 12);
>> val2 is never used in the upper layers in this case.
> I don't understand what you mean.

Just drop the *val2 = ... line. It doesn't make any sense in this context. Also
make sure to remove the EXTRACT_PERCENT macro since it wont be used anymore.

> Again, this is copied from AD7298
>>
[...]
>>> +/**
>>> + * ad7923_update_scan_mode() setup the spi transfer buffer for the
>>> new scan mask
>>> + **/
>>> +int ad7923_update_scan_mode(struct iio_dev *indio_dev,
>>> +    const unsigned long *active_scan_mask)
>>> +{
>>> +    struct ad7923_state *st = iio_priv(indio_dev);
>>> +    int i, cmd, channel;
>>> +    int scan_count;
>>> +
>>> +    /* Now compute overall size */
>>> +    for (i = 0, channel = 0; i < AD7923_MAX_CHAN; i++)
>>> +        if (test_bit(i, active_scan_mask))
>>> +            channel = i;
>>> +
>>> +    cmd = AD7923_WRITE_CR | AD7923_CODING | AD7923_RANGE |
>>> +        AD7923_PM_MODE_WRITE(AD7923_PM_MODE_OPS) |
>>> +        AD7923_SEQUENCE_WRITE(AD7923_SEQUENCE_ON) |
>>> +        AD7923_CHANNEL_WRITE(channel);
>> Hm, ok this looks a bit strange. You lookup the first channel which is
>> selected
>> and then also sample all channels which come before it. E.g. if only
>> channel 2
>> is selected you sample channel 0, 1 and 2. In the trigger handler you
>> push the
>> buffer to the IIO core. But in your buffer you have the result of
>> channel 0 in
>> the position where IIO would expect channel 2.
>> I think it is better if you send a cmd for each channel that needs to be
>> samples. E.g.:
>>
>>     if (for_each_set_bit(i, active_scan_mask, AD7923_MAX_CHAN)) {
>>         cmd = AD7923_WRITE_CR | AD7923_CODING | AD7923_RANGE |
>>             AD7923_PM_MODE_WRITE(AD7923_PM_MODE_OPS) |
>>             AD7923_SEQUENCE_WRITE(AD7923_SEQUENCE_OFF) |
>>             AD7923_CHANNEL_WRITE(i);
>>         cmd <<= AD7923_SHIFT_REGISTER;
>>         st->tx_buf[j++] = cpu_to_be16(cmd);
>>     }
> Ok, here we are trying to use the sequence mode. But unlike the AD7298,
> here sequence mode is only able to go from channel 0 to a given channel.
> Hence the reason why we try to identify the highest requested channel,
> then we do a sequential read of all from 0 to this one.
> 

The issue is that this doesn't work for all configurations. E.g. imagine
channel 0 is not selected and channel 1 is selected. You are now going to
sample both channel 0 and channel 1. Although you should only sample channel 1.

> Wouldn't the use on non sequential mode be less performant ?

I'm not sure whether the sequential mode actually gives you better performance
or whether it's just for convenience. But you can add a special case to the
sample function where it will use the sequential mode when possible.

- Lars