Re: [PATCH] IIO ADC support for AD7923

From: christophe leroy
Date: Sat Jan 19 2013 - 10:05:05 EST

Next message: Cong Ding: "[PATCH] btrfs: fix potential null pointer dereference bug"
Previous message: Greg Kroah-Hartman: "Re: [RFC PATCH v5 7/8] PCI/PCIe: add "pci=nopciehp" to disable PCIenative hotplug"
In reply to: Alessandro Rubini: "Re: [PATCH] IIO ADC support for AD7923"
Next in thread: Lars-Peter Clausen: "Re: [PATCH] IIO ADC support for AD7923"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxx>
Signed-off-by: Christophe Leroy <christophe.leroy@xxxxxx>

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

+ 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.

+ 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@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

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;
+}
+

[...]

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Cong Ding: "[PATCH] btrfs: fix potential null pointer dereference bug"
Previous message: Greg Kroah-Hartman: "Re: [RFC PATCH v5 7/8] PCI/PCIe: add "pci=nopciehp" to disable PCIenative hotplug"
In reply to: Alessandro Rubini: "Re: [PATCH] IIO ADC support for AD7923"
Next in thread: Lars-Peter Clausen: "Re: [PATCH] IIO ADC support for AD7923"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]