Re: [PATCH] iio: adc: add driver for the ti-adc084s021 chip

["Mårten Lindahl" <marten.lindahl-VrBV9hrLPhE@public.gmane.org>]

On Sun, 2017-04-23 at 20:13 +0100, Jonathan Cameron wrote:
> On 21/04/17 21:19, Peter Meerwald-Stadler wrote:
> > 
> >> From: Mårten Lindahl <martenli-VrBV9hrLPhE@public.gmane.org>
> > 
> > comments below
> A few more from me. Laptop battery gone flat so not so thorough on top few lines!
> 
> Jonathan
Hi Jonathan! Thanks for the comments! Please see my reply below.
Thanks,
Mårten
> >  
> >> This adds support for the Texas Instruments ADC084S021 ADC chip.
> >>
> >> Signed-off-by: Mårten Lindahl <martenli-VrBV9hrLPhE@public.gmane.org>
> >> ---
> >>  .../devicetree/bindings/iio/adc/ti-adc084s021.txt  |  25 ++
> >>  drivers/iio/adc/Kconfig                            |  12 +
> >>  drivers/iio/adc/Makefile                           |   1 +
> >>  drivers/iio/adc/ti-adc084s021.c                    | 342 +++++++++++++++++++++
> >>  4 files changed, 380 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-adc084s021.txt
> >>  create mode 100644 drivers/iio/adc/ti-adc084s021.c
> >>
> >> diff --git a/Documentation/devicetree/bindings/iio/adc/ti-adc084s021.txt b/Documentation/devicetree/bindings/iio/adc/ti-adc084s021.txt
> >> new file mode 100644
> >> index 0000000..921eb46
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/iio/adc/ti-adc084s021.txt
> >> @@ -0,0 +1,25 @@
> >> +* Texas Instruments' ADC084S021
> >> +
> >> +Required properties:
> >> + - compatible        : Must be "ti,adc084s021"
> >> + - reg               : SPI chip select number for the device
> >> + - vref-supply       : The regulator supply for ADC reference voltage
> >> + - spi-max-frequency : Definition as per Documentation/devicetree/bindings/spi/spi-bus.txt
> >> +
> >> +Optional properties:
> >> + - spi-cpol          : SPI inverse clock polarity, as per spi-bus bindings
> >> + - spi-cpha          : SPI shifted clock phase (CPHA), as per spi-bus bindings
> >> + - spi-cs-high       : SPI chip select active high, as per spi-bus bindings
> >> +
> >> +
> >> +Example:
> >> +adc@0 {
> >> +	compatible = "ti,adc084s021";
> >> +	reg = <0>;
> >> +	vref-supply = <&adc_vref>;
> >> +	spi-cpol;
> >> +	spi-cpha;
> >> +	spi-cs-high;
> >> +	spi-max-frequency = <16000000>;
> >> +	pl022,com-mode = <0x2>; /* DMA */
> > 
> > what is this?
> > 
> >> +};
> >> 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
> >>  	  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
> >> +	  chips.
> >> +
> >> +	  This driver can also be built as a module. If so, the module will be
> >> +	  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
> >> new file mode 100644
> >> index 0000000..4f33b91
> >> --- /dev/null
> >> +++ b/drivers/iio/adc/ti-adc084s021.c
> >> @@ -0,0 +1,342 @@
> >> +/**
> >> + * 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
> >> + * it under the terms of the GNU General Public License version 2 as
> >> + * 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/events.h>
> >> +#include <linux/iio/triggered_buffer.h>
> >> +#include <linux/iio/trigger_consumer.h>
> >> +#include <linux/regulator/consumer.h>
> >> +
> >> +#define MODULE_NAME     "adc084s021"
> >> +#define DRIVER_VERSION  "1.0"
> > 
> > is only used once at the very end...
> > 
> >> +#define ADC_RESOLUTION  8
> Put it inline...
Fixed in v2.
> >> +#define ADC_N_CHANNELS  4
> Belongs inline. No additional info provided by having it here.
Fixed in v2.
> > 
> > we want a consistent prefix, such as ADC084S021_
> > 
> >> +
> >> +struct adc084s021_configuration {
> >> +	const struct iio_chan_spec *channels;
> >> +	u8 num_channels;
> > 
> > no need for u8, perhaps unsigned?
> > 
> >> +};
> >> +
> >> +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 {
> >> +		u8 tx_buf[2];
> >> +		u8 rx_buf[2];
> >> +	} ____cacheline_aligned;
> >> +	u8 cur_adc_values[ADC_N_CHANNELS];
> >> +};
> >> +
> >> +/**
> >> + * Event triggered when value changes on a channel
> >> + */
> >> +static const struct iio_event_spec adc084s021_event = {
> >> +	.type = IIO_EV_TYPE_CHANGE,
> >> +	.dir = IIO_EV_DIR_NONE,
> >> +};
> Not the intent of that type of event at all.
I removed using events in v2.
> >> +
> >> +/**
> >> + * Channel specification
> >> + */
> >> +#define ADC084S021_VOLTAGE_CHANNEL(num)                  \
> >> +	{                                                      \
> >> +		.type = IIO_VOLTAGE,                                 \
> >> +		.channel = (num),                                    \
> >> +		.address = (num << 3),                               \
> > 
> > parenthesis should be around (num)
> > 
> >> +		.indexed = 1,                                        \
> >> +		.scan_index = num,                                   \
> > 
> > parenthesis?
> > 
> >> +		.scan_type = {                                       \
> >> +			.sign = 'u',                                       \
> >> +			.realbits = 8,                                     \
> >> +			.storagebits = 32,                                 \
> >> +			.shift = 24 - ((num << 3)),                        \
> > 
> > no need for (( )) around the expression, parenthesis should be around num
> > 
> > the shift doesn't make sense, you are shifting in 
> > 
> > adc084s021_adc_conversion() already and return just 8 bits (so storagebits 
> > should be 8)?
> > 
> > you could/should use realbits = 8, storagebits = 16, shift = 4 and 
> > endianness = IIO_BE if I read Figure 1 of the datasheet correctly
> > 
> >> +		},                                                   \
> >> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),        \
> > 
> > likely this is missing _SCALE
> > IIO expects data to be returned in millivolt, see 
> > Documentation/ABI/testing/sysfs-bus-iio
> > 
> >> +		.event_spec = &adc084s021_event,                     \
> >> +		.num_event_specs = 1,                                \
> > 
> > ARRAY_SIZE(&adc084s021_event) to be extensible?
> > 
> >> +	}
> >> +
> >> +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),
> >> +};
> >> +
> >> +static const struct adc084s021_configuration adc084s021_config[] = {
> >> +	{ adc084s021_channels, ARRAY_SIZE(adc084s021_channels) },
> > 
> > for just one configuration, this is not really needed; so you plan/forsee 
> > more chips being added soonish?
> > 
> >> +};
> >> +
> >> +/**
> >> + * 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)
> >> +{
> >> +	u16 value;
> > 
> > value should be u8, but is not really needed
> > 
> >> +	int ret;
> >> +
> >> +	mutex_lock(&adc->lock);
> >> +	adc->tx_buf[0] = channel->address;
> >> +
> >> +	/* Do the transfer */
> >> +	ret = spi_sync(adc->spi, &adc->message);
> >> +
> > 
> > no newline here please
> > 
> >> +	if (ret < 0) {
> >> +		mutex_unlock(&adc->lock);
> >> +		return ret;
> >> +	}
> >> +
> >> +	value = (adc->rx_buf[0] << 4) | (adc->rx_buf[1] >> 4);
> > 
> > I recommend using __be16 for rx_buf and
> > ret = (be16_to_cpu(adc->rx_buf) >> 4) & 0xff;
> > 
> >> +	mutex_unlock(&adc->lock);
> >> +
> >> +	dev_dbg(&adc->spi->dev, "value 0x%02X on channel %d\n",
> >> +			     value, channel->channel);
> >> +	return value;
> >> +}
> >> +
> >> +/**
> >> + * Make a readout of requested IIO channel info.
> > 
> > no need to document this, it is found in every IIO driver...
> > 
> >> + *
> >> + * @indio_dev: The industrial I/O device.
> >> + * @channel: The IIO channel data structure.
> >> + * @val: First element of value (integer).
> >> + * @val2: Second element of value (fractional).
> >> + * @mask: The info_mask to read.
> >> + */
> >> +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 retval;
> > 
> > how about using ret everywhere?
> Agreed.
Fixed in v2.
> > 
> >> +
> >> +	switch (mask) {
> >> +	case IIO_CHAN_INFO_RAW:
> > 
> > probably want iio_device_claim_direct_mode() so to not interfere with 
> > buffered accessBorderline case as all you will do is queue up additional spi transfers.
> At least after you have dropped the unusual events stuff.
> 
> Arguably this will mean sysfs reads could make the buffered data flow
> less deterministic though so maybe on claiming direct mode (which
> prevents it running concurrently with buffered capture.
Fixed in v2. And no events anymore.
> > 
> >> +		retval = adc084s021_adc_conversion(adc, channel);
> >> +		if (retval < 0)
> >> +			return retval;
> >> +
> >> +		*val = retval;
> >> +		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_trigger_handler(int irq, void *pollfunc)
> >> +{
> >> +	struct iio_poll_func *pf = pollfunc;
> >> +	struct iio_dev *indio_dev = pf->indio_dev;
> >> +	struct adc084s021 *adc = iio_priv(indio_dev);
> >> +	u8 *data;
> >> +	s64 timestamp;
> >> +	int value, scan_index;
> >> +
> >> +	data = kzalloc(indio_dev->scan_bytes, GFP_KERNEL);
> > 
> > pre-allocate buffer with maximum size statically; allocation is 
> > potentially expensive; don't forget space for the timestamp and padding...
> > 
> >> +	if (!data) {
> >> +		iio_trigger_notify_done(indio_dev->trig);
> >> +		return IRQ_NONE;
> >> +	}
> >> +
> >> +	timestamp = iio_get_time_ns(indio_dev);
> >> +
> >> +	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];
> >> +		value = adc084s021_adc_conversion(adc, channel);
> > 
> > lock is taken and released for each channel, probably want to do it just 
> > once?
> > 
> >> +		data[scan_index] = value;
> >> +
> >> +		/*
> >> +		 * Compare read data to previous read. If it differs send
> >> +		 * event notification for affected channel.
> >> +		 */
> >> +		if (adc->cur_adc_values[scan_index] != (u8)value) {
> > 
> > cur_adc_values is not initialized (probably set to 0);
> > so on first read, should the notification be sent?
> ?  This is 'unusual' to say the least. Why the events given the data
> is available via the buffer.  If you really want to do this stuff, it
> ought to be in userspace.
> 
> Are you trying to emulate the filtering input does?
I removed the check for previous value and sending events in v2.
> > 
> >> +			adc->cur_adc_values[scan_index] = (u8)value;
> >> +			iio_push_event(indio_dev,
> >> +						  IIO_EVENT_CODE(IIO_VOLTAGE, 0,
> >> +						    IIO_NO_MOD, IIO_EV_DIR_NONE,
> >> +						    IIO_EV_TYPE_CHANGE,
> >> +						    channel->channel, 0, 0),
> >> +						  timestamp);
> >> +			dev_dbg(&indio_dev->dev,
> >> +					    "new value on ch%d: 0x%02X (ts %llu)\n",
> >> +					    channel->channel, value, timestamp);
> >> +		}
> >> +	}
> >> +
> >> +	iio_push_to_buffers_with_timestamp(indio_dev, data, timestamp);
> >> +	iio_trigger_notify_done(indio_dev->trig);
> >> +	kfree(data);
> blank line here please.
Fixed in v2.
> >> +	return IRQ_HANDLED;
> >> +}
> >> +
> >> +static const struct iio_info adc084s021_info = {
> >> +	.read_raw = adc084s021_read_raw,
> >> +	.driver_module = THIS_MODULE,
> >> +};
> >> +
> >> +/**
> >> + * Create and register ADC IIO device for SPI.
> > 
> > comment is rather pointless
> > 
> >> + */
> >> +static int adc084s021_probe(struct spi_device *spi)
> >> +{
> >> +	struct iio_dev *indio_dev;
> >> +	struct adc084s021 *adc;
> >> +	int config = spi_get_device_id(spi)->driver_data;
> >> +	int retval;
> > 
> > ret maybe
> > 
> >> +
> >> +	/* Allocate an Industrial I/O device */
> > 
> > obviously...
> :)  Yeah, clear out any comments that don't tell us much.
Fixed in v2.
> > 
> >> +	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 = ADC_RESOLUTION;
> This surprised me somewhat as I was expecting an odd value for this
> (and was going to ask if standard power of 2 options would work).
> If it had been inline, this would have required slightly less review
> time which I always like (particularly when crammed into an airline seat!)
Yes, I am using inline value in v2.
> >> +
> >> +	/* Update the SPI device with config and connect the iio dev */
> >> +	retval = spi_setup(spi);
> >> +	if (retval) {
> >> +		dev_err(&spi->dev, "Failed to update SPI device\n");
> >> +		return retval;
> >> +	}
> >> +	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_config[config].channels;
> >> +	indio_dev->num_channels = adc084s021_config[config].num_channels;
> > 
> > could do it directly as long there is just one configuration
> That would certainly be preferable unless V2 is going to show up
> with multiple options!
Fixed. No more options supported in v2.
> > 
> >> +
> >> +	/* Create SPI transfer for channel reads */
> >> +	adc->spi_trans[0].tx_buf = &adc->tx_buf[0];
> >> +	adc->spi_trans[0].len = 2;
> >> +	adc->spi_trans[0].speed_hz = spi->max_speed_hz;
> >> +	adc->spi_trans[0].bits_per_word = spi->bits_per_word;
> >> +	adc->spi_trans[1].rx_buf = &adc->rx_buf[0];
> >> +	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;
> >> +
> >> +	/* Setup SPI message for channel reads */
> >> +	spi_message_init(&adc->message);
> >> +	spi_message_add_tail(&adc->spi_trans[0], &adc->message);
> >> +	spi_message_add_tail(&adc->spi_trans[1], &adc->message);
> spi_init_with_transfers to save a bit of boilerplate.
Fixed in v2.
> >> +
> >> +	adc->reg = devm_regulator_get(&spi->dev, "vref");
> >> +	if (IS_ERR(adc->reg))
> >> +		return PTR_ERR(adc->reg);
> >> +
> >> +	retval = regulator_enable(adc->reg);
> >> +	if (retval < 0)
> >> +		return retval;
> Given we either have slow sysfs accesses or know we have buffered
> access on going. Have  you considered enabling and disabling
> the regulator dynamically? The fact that this often makes sense is
> why Mark and co from the regulators side of things haven't provided
> a devm_regulator_enable... You would need a preenable and postdisable
> to make sure it was on for buffered access.
Yes, I am using preenable and postdisable functions for regulator in v2.
> >> +
> >> +	mutex_init(&adc->lock);
> >> +
> >> +	/* Setup triggered buffer with pollfunction */
> >> +	retval = iio_triggered_buffer_setup(indio_dev, NULL,
> > 
> > devm_()
> I disagree. It would change the ordering wrt to the regulator_enable.
> Now, obviously it won't actually matter, but it will make the code
> less obviously correct and for the small  burden of extra unwind
> code I'd keep using the non devm version.
I did not change this in v2. So I kept the non devm_ functions.
> > 
> >> +					    adc084s021_trigger_handler, NULL);
> >> +	if (retval) {
> >> +		dev_err(&spi->dev, "Failed to setup triggered buffer\n");
> >> +		goto buffer_setup_failed;
> >> +	}
> >> +
> >> +	retval = iio_device_register(indio_dev);
> >> +	if (retval) {
> >> +		dev_err(&spi->dev, "Failed to register IIO device\n");
> Hmm. If we were to flesh out some error messages for the few
> cases where there is no info provided in iio_device_register we could
> probably clean out a fair bit of boiler plate reporting in drivers.
> Ah well, one for the future!
If you insist I will remove it :)
> >> +		goto device_register_failed;
> >> +	}
> >> +
> >> +	dev_info(&spi->dev, "probed!\n");
> > 
> > no logging please, just outputs clutter
> > 
> >> +	return 0;
> >> +
> >> +device_register_failed:
> >> +	iio_triggered_buffer_cleanup(indio_dev);
> >> +buffer_setup_failed:
> >> +	regulator_disable(adc->reg);
> >> +	return retval;
> >> +}
> >> +
> >> +/**
> >> + * Unregister ADC IIO device for SPI.
> >> + */
> >> +static int adc084s021_remove(struct spi_device *spi)
> >> +{
> >> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> >> +	struct adc084s021 *adc = iio_priv(indio_dev);
> >> +
> >> +	iio_device_unregister(indio_dev);
> >> +	iio_triggered_buffer_cleanup(indio_dev);
> >> +	regulator_disable(adc->reg);
> blank line here preferred (slightly!)
Fixed in v2.
> >> +	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[] = {
> >> +	{ MODULE_NAME, 0},
> >> +	{}
> >> +};
> >> +
> >> +MODULE_DEVICE_TABLE(spi, adc084s021_id);
> >> +
> >> +static struct spi_driver adc084s021_driver = {
> >> +	.driver = {
> >> +		.name = MODULE_NAME,
> >> +		.of_match_table = of_match_ptr(adc084s021_of_match),
> >> +	},
> >> +	.probe = adc084s021_probe,
> >> +	.remove = adc084s021_remove,
> >> +	.id_table = adc084s021_id,
> >> +};
> >> +
> Convention often says to not bother with a blank line here or before
> the MODULE_DEVICE_TABLE above.  Gives a visual indication that these macros
> are being passed the structures.
> (really minor point!)
Fixed in v2.
> >> +module_spi_driver(adc084s021_driver);
> >> +
> >> +MODULE_AUTHOR("Mårten Lindahl <martenli-VrBV9hrLPhE@public.gmane.org>");
> >> +MODULE_DESCRIPTION("Texas Instruments ADC084S021");
> >> +MODULE_LICENSE("GPL v2");
> >> +MODULE_VERSION(DRIVER_VERSION);
> >>
> > 
>