Re: [PATCHv2 2/2] iio: adc: add driver for the ti-adc084s021 chip

From: Jonathan Cameron <hidden>
Date: 2017-05-04 16:31:10
Also in: linux-iio


On 3 May 2017 10:13:04 BST, "Mårten Lindahl" [off-list ref] wrote:

On Sun, 2017-04-30 at 16:51 +0100, Jonathan Cameron wrote:

quoted

On 30/04/17 13:53, Mårten Lindahl wrote:

quoted

From: Mårten Lindahl <redacted>

This adds support for the Texas Instruments ADC084S021 ADC chip.

Signed-off-by: Mårten Lindahl <redacted>

A few more bits inline.  Mostly stuff that has come up
in the V2 changes (and the inevitable bits I missed the
first time!)

Jonathan

Hi Jonathan! Please see my comments. I will send v3 today.
Thanks,
Mårten

quoted

---
Changes in v2:
- Removed most #defines in favor of inlines
- Corrected channel macro
- Removed configuration array with only one item
- Updated func adc084s021_adc_conversion to use be16_to_cpu
- Added IIO_CHAN_INFO_SCALE to func adc084s021_read_raw
- Use iio_device_claim_direct_mode in func adc084s021_read_raw
- Removed documentation for standard driver functions
- Changed retval to ret everywhere
- Removed dynamic alloc for data buffer in trigger handler
- Keeping mutex for all iterations in trigger handler
- Removed usage of events in this driver
- Removed info log in probe
- Use spi_message_init_with_transfers for spi message structs
- Use preenable and postdisable functions for regulator
- Inserted blank line before last return in all functions

 drivers/iio/adc/Kconfig         |  12 ++
 drivers/iio/adc/Makefile        |   1 +
 drivers/iio/adc/ti-adc084s021.c | 294

++++++++++++++++++++++++++++++++++++++++

quoted

 3 files changed, 307 insertions(+)
 create mode 100644 drivers/iio/adc/ti-adc084s021.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index dedae7a..13141e5 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig

@@ -560,6 +560,18 @@ config TI_ADC0832
 	  This driver can also be built as a module. If so, the module

will be

quoted

 	  called ti-adc0832.
 
+config TI_ADC084S021
+	tristate "Texas Instruments ADC084S021"
+	depends on SPI
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	help
+	  If you say yes here you get support for Texas Instruments

ADC084S021

quoted

+	  chips.
+
+	  This driver can also be built as a module. If so, the module

will be

quoted

+	  called ti-adc084s021.
+
 config TI_ADC12138
 	tristate "Texas Instruments ADC12130/ADC12132/ADC12138"
 	depends on SPI

diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index d001262..b1a6158 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile

@@ -51,6 +51,7 @@ obj-$(CONFIG_STM32_ADC_CORE) += stm32-adc-core.o
 obj-$(CONFIG_STM32_ADC) += stm32-adc.o
 obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
 obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o
+obj-$(CONFIG_TI_ADC084S021) += ti-adc084s021.o
 obj-$(CONFIG_TI_ADC12138) += ti-adc12138.o
 obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
 obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o

diff --git a/drivers/iio/adc/ti-adc084s021.c

b/drivers/iio/adc/ti-adc084s021.c

quoted

new file mode 100644
index 0000000..2dce257

--- /dev/null
+++ b/drivers/iio/adc/ti-adc084s021.c

@@ -0,0 +1,294 @@
+/**
+ * Copyright (C) 2017 Axis Communications AB
+ *
+ * Driver for Texas Instruments' ADC084S021 ADC chip.
+ * Datasheets can be found here:
+ * http://www.ti.com/lit/ds/symlink/adc084s021.pdf
+ *
+ * This program is free software; you can redistribute it and/or

modify

quoted

+ * it under the terms of the GNU General Public License version 2

as

quoted

+ * published by the Free Software Foundation.
+ */
+
+#include <linux/err.h>
+#include <linux/spi/spi.h>
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/regulator/consumer.h>
+
+#define ADC084S021_DRIVER_NAME "adc084s021"
+
+struct adc084s021 {
+	struct spi_device *spi;
+	struct spi_message message;
+	struct spi_transfer spi_trans[2];
+	struct regulator *reg;
+	struct mutex lock;
+	/*
+	 * DMA (thus cache coherency maintenance) requires the
+	 * transfer buffers to live in their own cache lines.
+	 */
+	union {
+		u16 tx_buf;
+		__be16 rx_buf;
+	} ____cacheline_aligned;
+};
+
+/**
+ * Channel specification
+ */

Comment doesn't add much so I'd drop it.

Fixed in v3.

quoted

+#define ADC084S021_VOLTAGE_CHANNEL(num)                  \
+	{                                                      \
+		.type = IIO_VOLTAGE,                                 \
+		.channel = (num),                                    \
+		.address = (num) << 3,                               \

This does feel a little pointless as you can just do the shift inline

and

quoted

use the channel value instead.  Doesn't really matter though.

Ok, fixed in v3.

quoted

+		.indexed = 1,                                        \
+		.scan_index = (num),                                 \
+		.scan_type = {                                       \
+			.sign = 'u',                                       \
+			.realbits = 8,                                     \
+			.storagebits = 16,                                 \
+			.shift = 4,                                        \
+			.endianness = IIO_BE,                              \
+		},                                                   \
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),        \
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),\
+	}
+
+static const struct iio_chan_spec adc084s021_channels[] = {
+	ADC084S021_VOLTAGE_CHANNEL(0),
+	ADC084S021_VOLTAGE_CHANNEL(1),
+	ADC084S021_VOLTAGE_CHANNEL(2),
+	ADC084S021_VOLTAGE_CHANNEL(3),
+	IIO_CHAN_SOFT_TIMESTAMP(4),
+};
+
+/**
+ * Read an ADC channel and return its value.
+ *
+ * @adc: The ADC SPI data.
+ * @channel: The IIO channel data structure.
+ */
+static int adc084s021_adc_conversion(struct adc084s021 *adc,
+			   struct iio_chan_spec const *channel)
+{
+	u8 value;
+	int ret;
+
+	adc->tx_buf = channel->address;
+
+	/* Do the transfer */
+	ret = spi_sync(adc->spi, &adc->message);
+	if (ret < 0)
+		return ret;
+
+	value = (be16_to_cpu(adc->rx_buf) >> channel->scan_type.shift) &

0xff;You want to do this for the read_raw sysfs calls, but not the
buffered ones

quoted

(where this shift and mask is made userspace's problem).

Fixed in v3.

quoted

+
+	dev_dbg(&adc->spi->dev, "value 0x%02X on channel %d\n",
+			     value, channel->channel);
+
+	return value;
+}
+
+static int adc084s021_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *channel, int *val,
+			   int *val2, long mask)
+{
+	struct adc084s021 *adc = iio_priv(indio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret < 0)
+			return ret;
+

Unless I'm dozing off this morning, you don't have any power..
So you'll need to enable the regulator first in this path.

Fixed in v3.

quoted

Note that the runtime power management autosuspend stuff can
be good for this as it stops the power cycling if a short burst
of readings is taken.

I don't have much experience of the pm_runtime system, so I looked
around to the other drivers but I see very few users of it. I can
absolutely look into it if you think it should be applied to this
driver.

I never insist on people implementing features they personally don't use as long as they don't
prevent them being added later! Hence DT bindings need to be easy to extend or complete...

So here it is entirely up to you. If someone else needs it down the line they can add it. Nice to have though.  If you want to do it I would suggest a follow up patch so it doesn't delay
the bit that matters to you.

In the grand scheme of things runtime pm is relatively new (a few years) and not necessary
for a driver to work.

quoted

+		ret = adc084s021_adc_conversion(adc, channel);
+		iio_device_release_direct_mode(indio_dev);
+		if (ret < 0)
+			return ret;
+
+		*val = ret;
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		ret = regulator_get_voltage(adc->reg);

Given the regulator is currently off as far as this device is

concerned

quoted

it's possible the answer will be 0...

Fixed in v3.

quoted

+		if (ret < 0)
+			return ret;
+
+		*val = ret / 1000;
+
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+/**
+ * Read enabled ADC channels and push data to the buffer.
+ *
+ * @irq: The interrupt number (not used).
+ * @pollfunc: Pointer to the poll func.
+ */
+static irqreturn_t adc084s021_buffer_trigger_handler(int irq, void

*pollfunc)

quoted

+{
+	struct iio_poll_func *pf = pollfunc;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct adc084s021 *adc = iio_priv(indio_dev);
+	__be16 data[8] = {0}; /* 4 * 16-bit words of data + 8 bytes

timestamp */

quoted

+	int scan_index;
+	int i = 0;
+
+	mutex_lock(&adc->lock);
+
+	for_each_set_bit(scan_index, indio_dev->active_scan_mask,
+			 indio_dev->masklength) {
+		const struct iio_chan_spec *channel =
+			&indio_dev->channels[scan_index];
+		data[i++] = adc084s021_adc_conversion(adc, channel);

This is clearly a rather simplistic way of handling the read outs.
Perfectly good for an initial version (which may never get improved
on!) but you could do the spi transfers for a set of channels much
more efficiently.

Figure 1 on the datasheet shows how the control register for the next
read can be written in parallel with the previous read. That would
mean each scan took N + 1 2 byte transfers rather than 2N as

currently.

quoted

I'm not particularly suggesting you do this, but thought I'd just
comment on the possibility as it is a common situation with spi
ADCs (sometimes you get a greater lag between setup and read out
making this even fiddlier!)

If you do want to do this, the trick is to do your transfer setup
and creation stuff in preenable so that it can be customised for
whatever channels are enabled.

Ok, Fixed in v3.
I have looked into this and I think I improved the design according to
your suggestion. Please note: According to the datasheet (section 8.3)
after each power up the ADC will read out last scanned/first channel,
which will result in a very first read of 16 bits that is ignored.
Also,
the readings are setup by only one transfer segment now.

Cool

quoted

+	}
+
+	iio_push_to_buffers_with_timestamp(indio_dev, data,
+					   iio_get_time_ns(indio_dev));
+	mutex_unlock(&adc->lock);
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static int adc084s021_buffer_preenable(struct iio_dev *indio_dev)
+{
+	struct adc084s021 *adc = iio_priv(indio_dev);
+
+	return regulator_enable(adc->reg);
+}
+
+static int adc084s021_buffer_postdisable(struct iio_dev

*indio_dev)

quoted

+{
+	struct adc084s021 *adc = iio_priv(indio_dev);
+
+	return regulator_disable(adc->reg);
+}
+
+static const struct iio_info adc084s021_info = {
+	.read_raw = adc084s021_read_raw,
+	.driver_module = THIS_MODULE,
+};
+
+static const struct iio_buffer_setup_ops

adc084s021_buffer_setup_ops = {

quoted

+	.preenable = adc084s021_buffer_preenable,
+	.postenable = iio_triggered_buffer_postenable,
+	.predisable = iio_triggered_buffer_predisable,
+	.postdisable = adc084s021_buffer_postdisable,
+};
+
+static int adc084s021_probe(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev;
+	struct adc084s021 *adc;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
+	if (!indio_dev) {
+		dev_err(&spi->dev, "Failed to allocate IIO device\n");
+		return -ENOMEM;
+	}
+
+	adc = iio_priv(indio_dev);
+	adc->spi = spi;
+	spi->bits_per_word = 8;
+
+	/* Update the SPI device with config and connect the iio dev */
+	ret = spi_setup(spi);
+	if (ret) {
+		dev_err(&spi->dev, "Failed to update SPI device\n");
+		return ret;
+	}
+	spi_set_drvdata(spi, indio_dev);
+
+	/* Initiate the Industrial I/O device */
+	indio_dev->dev.parent = &spi->dev;
+	indio_dev->dev.of_node = spi->dev.of_node;
+	indio_dev->name = spi_get_device_id(spi)->name;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &adc084s021_info;
+	indio_dev->channels = adc084s021_channels;
+	indio_dev->num_channels = ARRAY_SIZE(adc084s021_channels);
+
+	/* Create SPI transfer for channel reads */
+	adc->spi_trans[0].tx_buf = &adc->tx_buf;
+	adc->spi_trans[0].len = 2;
+	adc->spi_trans[0].speed_hz = spi->max_speed_hz;

You don't need to set these for individual transfers unless they
are different from how the spi bus has been set up...

It's handled in __spi_validate in drivers/spi/spi.c

	list_for_each_entry(xfer, &message->transfers, transfer_list) {
		message->frame_length += xfer->len;
		if (!xfer->bits_per_word)
			xfer->bits_per_word = spi->bits_per_word;

		if (!xfer->speed_hz)
			xfer->speed_hz = spi->max_speed_hz;
...

So it will set them spi->... in the core if you haven't overridden.

Fixed in v3.

quoted

+	adc->spi_trans[0].bits_per_word = spi->bits_per_word;
+	adc->spi_trans[1].rx_buf = &adc->rx_buf;
+	adc->spi_trans[1].len = 2;
+	adc->spi_trans[1].speed_hz = spi->max_speed_hz;
+	adc->spi_trans[1].bits_per_word = spi->bits_per_word;
+
+	spi_message_init_with_transfers(&adc->message, adc->spi_trans,

2);

quoted

+
+	adc->reg = devm_regulator_get(&spi->dev, "vref");
+	if (IS_ERR(adc->reg))
+		return PTR_ERR(adc->reg);
+
+	mutex_init(&adc->lock);
+
+	/* Setup triggered buffer with pollfunction */
+	ret = iio_triggered_buffer_setup(indio_dev, NULL,
+					    adc084s021_buffer_trigger_handler,
+					    &adc084s021_buffer_setup_ops);
+	if (ret) {
+		dev_err(&spi->dev, "Failed to setup triggered buffer\n");
+		return ret;
+	}
+
+	ret = iio_device_register(indio_dev);
+	if (ret) {
+		dev_err(&spi->dev, "Failed to register IIO device\n");
+		iio_triggered_buffer_cleanup(indio_dev);

With devm usage this won't be needed any more.

Fixed in v3.

quoted

Hmm. We should improve the error message from the core as it'll allow

us

quoted

to remove a lot of boiler plate in drivers. Ah well, a project for
another day.

I removed this message in v3.

quoted

+	}
+
+	return ret;
+}
+
+static int adc084s021_remove(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev = spi_get_drvdata(spi);
+
+	iio_device_unregister(indio_dev);
+	iio_triggered_buffer_cleanup(indio_dev);

Now you have moved to dynamically enabling the regulator, it makes
sense to use the devm_ versions of iio_device_register and 
iio_triggered_buffer setup.

With those, you'll be able to get rid of the remove callback entirely

as

quoted

there will be nothing to do.

Fixed in v3.

quoted

+
+	return 0;
+}
+
+static const struct of_device_id adc084s021_of_match[] = {
+	{ .compatible = "ti,adc084s021", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, adc084s021_of_match);
+
+static const struct spi_device_id adc084s021_id[] = {
+	{ ADC084S021_DRIVER_NAME, 0},
+	{}
+};
+MODULE_DEVICE_TABLE(spi, adc084s021_id);
+
+static struct spi_driver adc084s021_driver = {
+	.driver = {
+		.name = ADC084S021_DRIVER_NAME,
+		.of_match_table = of_match_ptr(adc084s021_of_match),
+	},
+	.probe = adc084s021_probe,
+	.remove = adc084s021_remove,
+	.id_table = adc084s021_id,
+};
+module_spi_driver(adc084s021_driver);
+
+MODULE_AUTHOR("Mårten Lindahl [off-list ref]");
+MODULE_DESCRIPTION("Texas Instruments ADC084S021");
+MODULE_LICENSE("GPL v2");
+MODULE_VERSION("1.0");


--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help