On 01/08/2013 09:42 AM, Christophe Leroy wrote:Our driver is based on AD7298 driver, which is today in staging. Should we put ours in drivers/iio/ directly anyway ?This patch adds support for Analog Devices AD7923 ADC in the IIO Subsystem.Hi,
Signed-off-by: Patrick Vasseur <patrick.vasseur@xxxxxx>
Signed-off-by: Christophe Leroy <christophe.leroy@xxxxxx>
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/KconfigNew IIO drivers should not be added to staging, unless there is a very good
--- 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
reason. Since this driver does not have any major issues it should directly go
into drivers/iio/
This part is copied unmodified from AD7298. Should we modify it ?
[...]
diff -uN linux-3.7.1/drivers/staging/iio/adc/ad7923.h linux/drivers/staging/iio/adc/ad7923.hboth buffers should of type __be16
--- 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];
Copied from AD7298. Do we change it anyway ?
+};static const struct
+
+#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[] = {
I don't understand what you mean.
+ AD7923_V_CHAN(0),[...]
+ AD7923_V_CHAN(1),
+ AD7923_V_CHAN(2),
+ AD7923_V_CHAN(3),
+ IIO_CHAN_SOFT_TIMESTAMP(4),
+};
+
+val2 is never used in the upper layers in this case.
+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);
ok
+ }If the address does not match you should probably return an error
Ok, we'll try and add it.
+ return IIO_VAL_INT;How about also reporting the scale attribute?
+ }
Ok, but still is AD7298.
+ return -EINVAL;__devinit/__devexit is being phased out in upstream, it should not be used in
+}
+
+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)
new drivers anymore.
Ok
+{If the regulator could not be requested return an error.
+ 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)) {
Ok
+ ret = regulator_enable(st->reg);This line is just noise, please remove it.
+ 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");
Ok, why not, but once again this is from AD7298. We have tried to keep things homogeneous. Should we do differently ?
+Considering the overall size of the driver I think it makes sense to put it all
+ 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@xxxxxx>");
+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
in just one file.
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.
@@ -0,0 +1,113 @@Hm, ok this looks a bit strange. You lookup the first channel which is selected
+/*
+ * 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);
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, why not, but again this comes from AD7298 driver.
+ cmd <<= AD7923_SHIFT_REGISTER;No need to perform the endianness conversion here. Just mark the channel as IIO_BE.
+ 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;
+}
+