[PATCH v2] iio: adc: New driver for TI ADS7950 chips

[PATCH v2] iio: adc: New driver for TI ADS7950 chips

[David Lechner]

This adds a new driver for the TI ADS7950 family of ADC chips. These
communicate using SPI and come in 8/10/12-bit and 4/8/12/16 channel
varieties.

Signed-off-by: David Lechner <david@lechnology.com>
---

v2 changes:

* Got rid of XX wildcards - using ADS7950 everywhere
* Fixed some macro parentheses issues
* Added TI_ prefix to macros to match ti_ prefixes used elsewhere
* Added space in rx_buf for holding timestamp
* Use iio_device_claim_direct_mode() and spi_message_init_with_transfers()
  helper functions
* Don't use dev_info() at end of probe
* Minor spelling and code style fixes

 drivers/iio/adc/Kconfig      |  13 ++
 drivers/iio/adc/Makefile     |   1 +
 drivers/iio/adc/ti-ads7950.c | 488 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 502 insertions(+)
 create mode 100644 drivers/iio/adc/ti-ads7950.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 6bbee0b..26b32f0 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -527,6 +527,19 @@ config TI_ADS1015
 	  This driver can also be built as a module. If so, the module will be
 	  called ti-ads1015.
 
+config TI_ADS7950
+	tristate "Texas Instruments ADS7950 ADC driver"
+	depends on SPI
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	help
+	  Say yes here to build support for Texas Instruments ADS7950, ADS7951,
+	  ADS7952, ADS7953, ADS7954, ADS7955, ADS7956, ADS7957, ADS7958, ADS7959.
+	  ADS7960, ADS7961.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ti-ads7950.
+
 config TI_ADS8688
 	tristate "Texas Instruments ADS8688"
 	depends on SPI && OF
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 9391217..1966801 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -49,6 +49,7 @@ obj-$(CONFIG_TI_ADC12138) += ti-adc12138.o
 obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
 obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o
 obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
+obj-$(CONFIG_TI_ADS7950) += ti-ads7950.o
 obj-$(CONFIG_TI_ADS8688) += ti-ads8688.o
 obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
 obj-$(CONFIG_TWL4030_MADC) += twl4030-madc.o
diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
new file mode 100644
index 0000000..d0b76bd
--- /dev/null
+++ b/drivers/iio/adc/ti-ads7950.c
@@ -0,0 +1,488 @@
+/*
+ * Texas Instruments ADS7950 SPI ADC driver
+ *
+ * Copyright 2016 David Lechner <david@lechnology.com>
+ *
+ * Based on iio/ad7923.c:
+ * Copyright 2011 Analog Devices Inc
+ * Copyright 2012 CS Systemes d'Information
+ *
+ * And also on hwmon/ads79xx.c
+ * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com/
+ *	Nishanth Menon
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/bitops.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+#include <linux/spi/spi.h>
+
+#include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+#define TI_ADS7950_CR_MANUAL	BIT(12)
+#define TI_ADS7950_CR_WRITE	BIT(11)
+#define TI_ADS7950_CR_CHAN(ch)	((ch) << 7)
+#define TI_ADS7950_CR_RANGE_5V	BIT(6)
+
+#define TI_ADS7950_MAX_CHAN	16
+
+#define TI_ADS7950_TIMESTAMP_SIZE (sizeof(int64_t) / sizeof(__be16))
+
+/* val = value, dec = left shift, bits = number of bits of the mask */
+#define TI_ADS7950_EXTRACT(val, dec, bits) \
+	(((val) >> (dec)) & ((1 << (bits)) - 1))
+
+struct ti_ads7950_state {
+	struct spi_device	*spi;
+	struct spi_transfer	ring_xfer[TI_ADS7950_MAX_CHAN + 2];
+	struct spi_transfer	scan_single_xfer[3];
+	struct spi_message	ring_msg;
+	struct spi_message	scan_single_msg;
+
+	struct regulator	*reg;
+
+	unsigned int		settings;
+
+	/*
+	 * DMA (thus cache coherency maintenance) requires the
+	 * transfer buffers to live in their own cache lines.
+	 */
+	__be16	rx_buf[TI_ADS7950_MAX_CHAN + TI_ADS7950_TIMESTAMP_SIZE]
+							____cacheline_aligned;
+	__be16	tx_buf[TI_ADS7950_MAX_CHAN];
+};
+
+struct ti_ads7950_chip_info {
+	const struct iio_chan_spec *channels;
+	unsigned int num_channels;
+};
+
+enum ti_ads7950_id {
+	TI_ADS7950,
+	TI_ADS7951,
+	TI_ADS7952,
+	TI_ADS7953,
+	TI_ADS7954,
+	TI_ADS7955,
+	TI_ADS7956,
+	TI_ADS7957,
+	TI_ADS7958,
+	TI_ADS7959,
+	TI_ADS7960,
+	TI_ADS7961,
+};
+
+#define TI_ADS7950_V_CHAN(index, bits)				\
+{								\
+	.type = IIO_VOLTAGE,					\
+	.indexed = 1,						\
+	.channel = index,					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
+	.address = index,					\
+	.datasheet_name = "CH##index",				\
+	.scan_index = index,					\
+	.scan_type = {						\
+		.sign = 'u',					\
+		.realbits = bits,				\
+		.storagebits = 16,				\
+		.endianness = IIO_BE,				\
+	},							\
+}
+
+#define DECLARE_TI_ADS7950_4_CHANNELS(name, bits) \
+const struct iio_chan_spec name ## _channels[] = { \
+	TI_ADS7950_V_CHAN(0, bits), \
+	TI_ADS7950_V_CHAN(1, bits), \
+	TI_ADS7950_V_CHAN(2, bits), \
+	TI_ADS7950_V_CHAN(3, bits), \
+	IIO_CHAN_SOFT_TIMESTAMP(4), \
+}
+
+#define DECLARE_TI_ADS7950_8_CHANNELS(name, bits) \
+const struct iio_chan_spec name ## _channels[] = { \
+	TI_ADS7950_V_CHAN(0, bits), \
+	TI_ADS7950_V_CHAN(1, bits), \
+	TI_ADS7950_V_CHAN(2, bits), \
+	TI_ADS7950_V_CHAN(3, bits), \
+	TI_ADS7950_V_CHAN(4, bits), \
+	TI_ADS7950_V_CHAN(5, bits), \
+	TI_ADS7950_V_CHAN(6, bits), \
+	TI_ADS7950_V_CHAN(7, bits), \
+	IIO_CHAN_SOFT_TIMESTAMP(8), \
+}
+
+#define DECLARE_TI_ADS7950_12_CHANNELS(name, bits) \
+const struct iio_chan_spec name ## _channels[] = { \
+	TI_ADS7950_V_CHAN(0, bits), \
+	TI_ADS7950_V_CHAN(1, bits), \
+	TI_ADS7950_V_CHAN(2, bits), \
+	TI_ADS7950_V_CHAN(3, bits), \
+	TI_ADS7950_V_CHAN(4, bits), \
+	TI_ADS7950_V_CHAN(5, bits), \
+	TI_ADS7950_V_CHAN(6, bits), \
+	TI_ADS7950_V_CHAN(7, bits), \
+	TI_ADS7950_V_CHAN(8, bits), \
+	TI_ADS7950_V_CHAN(9, bits), \
+	TI_ADS7950_V_CHAN(10, bits), \
+	TI_ADS7950_V_CHAN(11, bits), \
+	IIO_CHAN_SOFT_TIMESTAMP(12), \
+}
+
+#define DECLARE_TI_ADS7950_16_CHANNELS(name, bits) \
+const struct iio_chan_spec name ## _channels[] = { \
+	TI_ADS7950_V_CHAN(0, bits), \
+	TI_ADS7950_V_CHAN(1, bits), \
+	TI_ADS7950_V_CHAN(2, bits), \
+	TI_ADS7950_V_CHAN(3, bits), \
+	TI_ADS7950_V_CHAN(4, bits), \
+	TI_ADS7950_V_CHAN(5, bits), \
+	TI_ADS7950_V_CHAN(6, bits), \
+	TI_ADS7950_V_CHAN(7, bits), \
+	TI_ADS7950_V_CHAN(8, bits), \
+	TI_ADS7950_V_CHAN(9, bits), \
+	TI_ADS7950_V_CHAN(10, bits), \
+	TI_ADS7950_V_CHAN(11, bits), \
+	TI_ADS7950_V_CHAN(12, bits), \
+	TI_ADS7950_V_CHAN(13, bits), \
+	TI_ADS7950_V_CHAN(14, bits), \
+	TI_ADS7950_V_CHAN(15, bits), \
+	IIO_CHAN_SOFT_TIMESTAMP(16), \
+}
+
+static DECLARE_TI_ADS7950_4_CHANNELS(ti_ads7950, 12);
+static DECLARE_TI_ADS7950_8_CHANNELS(ti_ads7951, 12);
+static DECLARE_TI_ADS7950_12_CHANNELS(ti_ads7952, 12);
+static DECLARE_TI_ADS7950_16_CHANNELS(ti_ads7953, 12);
+static DECLARE_TI_ADS7950_4_CHANNELS(ti_ads7954, 10);
+static DECLARE_TI_ADS7950_8_CHANNELS(ti_ads7955, 10);
+static DECLARE_TI_ADS7950_12_CHANNELS(ti_ads7956, 10);
+static DECLARE_TI_ADS7950_16_CHANNELS(ti_ads7957, 10);
+static DECLARE_TI_ADS7950_4_CHANNELS(ti_ads7958, 8);
+static DECLARE_TI_ADS7950_8_CHANNELS(ti_ads7959, 8);
+static DECLARE_TI_ADS7950_12_CHANNELS(ti_ads7960, 8);
+static DECLARE_TI_ADS7950_16_CHANNELS(ti_ads7961, 8);
+
+static const struct ti_ads7950_chip_info ti_ads7950_chip_info[] = {
+	[TI_ADS7950] = {
+		.channels	= ti_ads7950_channels,
+		.num_channels	= ARRAY_SIZE(ti_ads7950_channels),
+	},
+	[TI_ADS7951] = {
+		.channels	= ti_ads7951_channels,
+		.num_channels	= ARRAY_SIZE(ti_ads7951_channels),
+	},
+	[TI_ADS7952] = {
+		.channels	= ti_ads7952_channels,
+		.num_channels	= ARRAY_SIZE(ti_ads7952_channels),
+	},
+	[TI_ADS7953] = {
+		.channels	= ti_ads7953_channels,
+		.num_channels	= ARRAY_SIZE(ti_ads7953_channels),
+	},
+	[TI_ADS7954] = {
+		.channels	= ti_ads7954_channels,
+		.num_channels	= ARRAY_SIZE(ti_ads7954_channels),
+	},
+	[TI_ADS7955] = {
+		.channels	= ti_ads7955_channels,
+		.num_channels	= ARRAY_SIZE(ti_ads7955_channels),
+	},
+	[TI_ADS7956] = {
+		.channels	= ti_ads7956_channels,
+		.num_channels	= ARRAY_SIZE(ti_ads7956_channels),
+	},
+	[TI_ADS7957] = {
+		.channels	= ti_ads7957_channels,
+		.num_channels	= ARRAY_SIZE(ti_ads7957_channels),
+	},
+	[TI_ADS7958] = {
+		.channels	= ti_ads7958_channels,
+		.num_channels	= ARRAY_SIZE(ti_ads7958_channels),
+	},
+	[TI_ADS7959] = {
+		.channels	= ti_ads7959_channels,
+		.num_channels	= ARRAY_SIZE(ti_ads7959_channels),
+	},
+	[TI_ADS7960] = {
+		.channels	= ti_ads7960_channels,
+		.num_channels	= ARRAY_SIZE(ti_ads7960_channels),
+	},
+	[TI_ADS7961] = {
+		.channels	= ti_ads7961_channels,
+		.num_channels	= ARRAY_SIZE(ti_ads7961_channels),
+	},
+};
+
+/*
+ * ti_ads7950_update_scan_mode() setup the spi transfer buffer for the new
+ * scan mask
+ */
+static int ti_ads7950_update_scan_mode(struct iio_dev *indio_dev,
+				       const unsigned long *active_scan_mask)
+{
+	struct ti_ads7950_state *st = iio_priv(indio_dev);
+	int i, cmd, len;
+
+	len = 0;
+	for_each_set_bit(i, active_scan_mask, indio_dev->num_channels) {
+		cmd = TI_ADS7950_CR_WRITE | TI_ADS7950_CR_CHAN(i) | st->settings;
+		st->tx_buf[len++] = cpu_to_be16(cmd);
+	}
+
+	/* Data for the 1st channel is not returned until the 3rd transfer */
+	len += 2;
+	for (i = 0; i < len; i++) {
+		if ((i + 2) < len)
+			st->ring_xfer[i].tx_buf = &st->tx_buf[i];
+		if (i >= 2)
+			st->ring_xfer[i].rx_buf = &st->rx_buf[i - 2];
+		st->ring_xfer[i].len = 2;
+		st->ring_xfer[i].cs_change = 1;
+	}
+	/* make sure last transfer's cs_change is not set */
+	st->ring_xfer[len - 1].cs_change = 0;
+
+	spi_message_init_with_transfers(&st->ring_msg, st->ring_xfer, len);
+
+	return 0;
+}
+
+static irqreturn_t ti_ads7950_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct ti_ads7950_state *st = iio_priv(indio_dev);
+	int b_sent;
+
+	b_sent = spi_sync(st->spi, &st->ring_msg);
+	if (b_sent)
+		goto done;
+
+	iio_push_to_buffers_with_timestamp(indio_dev, st->rx_buf,
+					   iio_get_time_ns(indio_dev));
+
+done:
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static int ti_ads7950_scan_direct(struct ti_ads7950_state *st, unsigned int ch)
+{
+	int ret, cmd;
+
+	cmd = TI_ADS7950_CR_WRITE | TI_ADS7950_CR_CHAN(ch) | st->settings;
+	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 ti_ads7950_get_range(struct ti_ads7950_state *st)
+{
+	int vref;
+
+	vref = regulator_get_voltage(st->reg);
+	if (vref < 0)
+		return vref;
+
+	vref /= 1000;
+
+	if (st->settings & TI_ADS7950_CR_RANGE_5V)
+		vref *= 2;
+
+	return vref;
+}
+
+static int ti_ads7950_read_raw(struct iio_dev *indio_dev,
+			       struct iio_chan_spec const *chan,
+			       int *val, int *val2, long m)
+{
+	struct ti_ads7950_state *st = iio_priv(indio_dev);
+	int ret;
+
+	switch (m) {
+	case IIO_CHAN_INFO_RAW:
+
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret < 0)
+			return ret;
+
+		ret = ti_ads7950_scan_direct(st, chan->address);
+		iio_device_release_direct_mode(indio_dev);
+		if (ret < 0)
+			return ret;
+
+		if (chan->address != TI_ADS7950_EXTRACT(ret, 12, 4))
+			return -EIO;
+
+		*val = TI_ADS7950_EXTRACT(ret, 0, 12);
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		ret = ti_ads7950_get_range(st);
+		if (ret < 0)
+			return ret;
+
+		*val = ret;
+		*val2 = chan->scan_type.realbits;
+
+		return IIO_VAL_FRACTIONAL_LOG2;
+	}
+
+	return -EINVAL;
+}
+
+static const struct iio_info ti_ads7950_info = {
+	.read_raw		= &ti_ads7950_read_raw,
+	.update_scan_mode	= ti_ads7950_update_scan_mode,
+	.driver_module		= THIS_MODULE,
+};
+
+static int ti_ads7950_probe(struct spi_device *spi)
+{
+	struct ti_ads7950_state *st;
+	struct iio_dev *indio_dev;
+	const struct ti_ads7950_chip_info *info;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+
+	spi_set_drvdata(spi, indio_dev);
+
+	st->spi = spi;
+	st->settings = TI_ADS7950_CR_MANUAL | TI_ADS7950_CR_RANGE_5V;
+
+	info = &ti_ads7950_chip_info[spi_get_device_id(spi)->driver_data];
+
+	indio_dev->name = spi_get_device_id(spi)->name;
+	indio_dev->dev.parent = &spi->dev;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = info->channels;
+	indio_dev->num_channels = info->num_channels;
+	indio_dev->info = &ti_ads7950_info;
+
+	/*
+	 * Setup default message. The sample is read at the end of the first
+	 * transfer, then it takes one full cycle to convert the sample and one
+	 * more cycle to send the value. The conversion process is driven by
+	 * the SPI clock, which is why we have 3 transfers. The middle one is
+	 * just dummy data sent while the chip is converting the sample that
+	 * was read at the end of the first transfer.
+	 */
+
+	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].tx_buf = &st->tx_buf[0];
+	st->scan_single_xfer[1].len = 2;
+	st->scan_single_xfer[1].cs_change = 1;
+	st->scan_single_xfer[2].rx_buf = &st->rx_buf[0];
+	st->scan_single_xfer[2].len = 2;
+
+	spi_message_init_with_transfers(&st->scan_single_msg,
+					st->scan_single_xfer, 3);
+
+	st->reg = devm_regulator_get(&spi->dev, "refin");
+	if (IS_ERR(st->reg)) {
+		dev_err(&spi->dev, "Failed get get regulator \"refin\"\n");
+		return PTR_ERR(st->reg);
+	}
+
+	ret = regulator_enable(st->reg);
+	if (ret) {
+		dev_err(&spi->dev, "Failed to enable regulator \"refin\"\n");
+		return ret;
+	}
+
+	ret = iio_triggered_buffer_setup(indio_dev, NULL,
+					 &ti_ads7950_trigger_handler, NULL);
+	if (ret) {
+		dev_err(&spi->dev, "Failed to setup triggered buffer\n");
+		goto error_disable_reg;
+	}
+
+	ret = iio_device_register(indio_dev);
+	if (ret) {
+		dev_err(&spi->dev, "Failed to register iio device\n");
+		goto error_cleanup_ring;
+	}
+
+	return 0;
+
+error_cleanup_ring:
+	iio_triggered_buffer_cleanup(indio_dev);
+error_disable_reg:
+	regulator_disable(st->reg);
+
+	return ret;
+}
+
+static int ti_ads7950_remove(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev = spi_get_drvdata(spi);
+	struct ti_ads7950_state *st = iio_priv(indio_dev);
+
+	iio_device_unregister(indio_dev);
+	iio_triggered_buffer_cleanup(indio_dev);
+	regulator_disable(st->reg);
+
+	return 0;
+}
+
+static const struct spi_device_id ti_ads7950_id[] = {
+	{"ti-ads7950", TI_ADS7950},
+	{"ti-ads7951", TI_ADS7951},
+	{"ti-ads7952", TI_ADS7952},
+	{"ti-ads7953", TI_ADS7953},
+	{"ti-ads7954", TI_ADS7954},
+	{"ti-ads7955", TI_ADS7955},
+	{"ti-ads7956", TI_ADS7956},
+	{"ti-ads7957", TI_ADS7957},
+	{"ti-ads7958", TI_ADS7958},
+	{"ti-ads7959", TI_ADS7959},
+	{"ti-ads7960", TI_ADS7960},
+	{"ti-ads7961", TI_ADS7961},
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, ti_ads7950_id);
+
+static struct spi_driver ti_ads7950_driver = {
+	.driver = {
+		.name	= "ti-ads7950",
+	},
+	.probe		= ti_ads7950_probe,
+	.remove		= ti_ads7950_remove,
+	.id_table	= ti_ads7950_id,
+};
+module_spi_driver(ti_ads7950_driver);
+
+MODULE_AUTHOR("David Lechner <david@lechnology.com>");
+MODULE_DESCRIPTION("TI TI_ADS7950 ADC");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

Re: [PATCH v2] iio: adc: New driver for TI ADS7950 chips

[David Lechner]

On 11/20/2016 12:28 PM, David Lechner wrote:
> This adds a new driver for the TI ADS7950 family of ADC chips. These
> communicate using SPI and come in 8/10/12-bit and 4/8/12/16 channel
> varieties.
>
> Signed-off-by: David Lechner <david@lechnology.com>
> ---
>
> v2 changes:
>
> * Got rid of XX wildcards - using ADS7950 everywhere
> * Fixed some macro parentheses issues
> * Added TI_ prefix to macros to match ti_ prefixes used elsewhere
> * Added space in rx_buf for holding timestamp
> * Use iio_device_claim_direct_mode() and spi_message_init_with_transfers()
>   helper functions
> * Don't use dev_info() at end of probe
> * Minor spelling and code style fixes
>
>  drivers/iio/adc/Kconfig      |  13 ++
>  drivers/iio/adc/Makefile     |   1 +
>  drivers/iio/adc/ti-ads7950.c | 488 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 502 insertions(+)
>  create mode 100644 drivers/iio/adc/ti-ads7950.c
>

...

> diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
> new file mode 100644
> index 0000000..d0b76bd
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads7950.c

...

> +static irqreturn_t ti_ads7950_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct ti_ads7950_state *st = iio_priv(indio_dev);
> +	int b_sent;
> +
> +	b_sent = spi_sync(st->spi, &st->ring_msg);

hmm, I copied this from another driver, but spi_sync() in IRQ handler 
does not sound like a good idea (spi_sync() can sleep). I will replace 
it with spi_async().


> +	if (b_sent)
> +		goto done;
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, st->rx_buf,
> +					   iio_get_time_ns(indio_dev));
> +
> +done:
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}

...

> +static int ti_ads7950_read_raw(struct iio_dev *indio_dev,
> +			       struct iio_chan_spec const *chan,
> +			       int *val, int *val2, long m)
> +{
> +	struct ti_ads7950_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (m) {
> +	case IIO_CHAN_INFO_RAW:
> +
> +		ret = iio_device_claim_direct_mode(indio_dev);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = ti_ads7950_scan_direct(st, chan->address);
> +		iio_device_release_direct_mode(indio_dev);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (chan->address != TI_ADS7950_EXTRACT(ret, 12, 4))
> +			return -EIO;
> +
> +		*val = TI_ADS7950_EXTRACT(ret, 0, 12);

I'm not sure if I am doing this right. There are 8- 10- and 12-bit 
versions of this chip. The 8- and 10-bit versions still return a 12-bit 
number where the last 4 or 2 bits are always 0. Should I be shifting the 
12-bit value here based on the chip being used so that *val is 0-255 for 
8-bit and 0-1023 for 10-bit? Or should this be *really* raw and not even 
use TI_ADS7950_EXTRACT() to mask the channel address bits?

> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = ti_ads7950_get_range(st);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = ret;
> +		*val2 = chan->scan_type.realbits;
> +
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	}
> +
> +	return -EINVAL;
> +}

Re: [PATCH v2] iio: adc: New driver for TI ADS7950 chips

[Peter Meerwald-Stadler]

> > +static int ti_ads7950_read_raw(struct iio_dev *indio_dev,
> > +			       struct iio_chan_spec const *chan,
> > +			       int *val, int *val2, long m)
> > +{
> > +	struct ti_ads7950_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	switch (m) {
> > +	case IIO_CHAN_INFO_RAW:
> > +
> > +		ret = iio_device_claim_direct_mode(indio_dev);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		ret = ti_ads7950_scan_direct(st, chan->address);
> > +		iio_device_release_direct_mode(indio_dev);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		if (chan->address != TI_ADS7950_EXTRACT(ret, 12, 4))
> > +			return -EIO;
> > +
> > +		*val = TI_ADS7950_EXTRACT(ret, 0, 12);
> 
> I'm not sure if I am doing this right. There are 8- 10- and 12-bit versions of
> this chip. The 8- and 10-bit versions still return a 12-bit number where the
> last 4 or 2 bits are always 0. Should I be shifting the 12-bit value here
> based on the chip being used so that *val is 0-255 for 8-bit and 0-1023 for
> 10-bit? Or should this be *really* raw and not even use TI_ADS7950_EXTRACT()
> to mask the channel address bits?

I'd shift and adjust _SCALE so that *val * scale gives mV
 
> > +
> > +		return IIO_VAL_INT;
> > +	case IIO_CHAN_INFO_SCALE:
> > +		ret = ti_ads7950_get_range(st);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		*val = ret;
> > +		*val2 = chan->scan_type.realbits;
> > +
> > +		return IIO_VAL_FRACTIONAL_LOG2;
> > +	}
> > +
> > +	return -EINVAL;
> > +}

-- 

Peter Meerwald-Stadler
+43-664-2444418 (mobile)

Re: [PATCH v2] iio: adc: New driver for TI ADS7950 chips

[Jonathan Cameron]

On 21 November 2016 22:54:24 GMT+00:00, Peter Meerwald-Stadler <pmeerw@pmeerw.net> wrote:
>
>> > +static int ti_ads7950_read_raw(struct iio_dev *indio_dev,
>> > +			       struct iio_chan_spec const *chan,
>> > +			       int *val, int *val2, long m)
>> > +{
>> > +	struct ti_ads7950_state *st = iio_priv(indio_dev);
>> > +	int ret;
>> > +
>> > +	switch (m) {
>> > +	case IIO_CHAN_INFO_RAW:
>> > +
>> > +		ret = iio_device_claim_direct_mode(indio_dev);
>> > +		if (ret < 0)
>> > +			return ret;
>> > +
>> > +		ret = ti_ads7950_scan_direct(st, chan->address);
>> > +		iio_device_release_direct_mode(indio_dev);
>> > +		if (ret < 0)
>> > +			return ret;
>> > +
>> > +		if (chan->address != TI_ADS7950_EXTRACT(ret, 12, 4))
>> > +			return -EIO;
>> > +
>> > +		*val = TI_ADS7950_EXTRACT(ret, 0, 12);
>> 
>> I'm not sure if I am doing this right. There are 8- 10- and 12-bit
>versions of
>> this chip. The 8- and 10-bit versions still return a 12-bit number
>where the
>> last 4 or 2 bits are always 0. Should I be shifting the 12-bit value
>here
>> based on the chip being used so that *val is 0-255 for 8-bit and
>0-1023 for
>> 10-bit? Or should this be *really* raw and not even use
>TI_ADS7950_EXTRACT()
>> to mask the channel address bits?
>
>I'd shift and adjust _SCALE so that *val * scale gives mV
It would also be fine to not do anything and let userspace deal with shifting and masking for
buffered data.  Non buffered obviously still needs shifting and masking though!

I'd slightly prefer the doing nothing route but don't really care as both are valid uses of the ABI.

Jonathan
> 
>> > +
>> > +		return IIO_VAL_INT;
>> > +	case IIO_CHAN_INFO_SCALE:
>> > +		ret = ti_ads7950_get_range(st);
>> > +		if (ret < 0)
>> > +			return ret;
>> > +
>> > +		*val = ret;
>> > +		*val2 = chan->scan_type.realbits;
>> > +
>> > +		return IIO_VAL_FRACTIONAL_LOG2;
>> > +	}
>> > +
>> > +	return -EINVAL;
>> > +}

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH v2] iio: adc: New driver for TI ADS7950 chips

[David Lechner]

On 11/22/2016 01:23 AM, Jonathan Cameron wrote:
>
>
> On 21 November 2016 22:54:24 GMT+00:00, Peter Meerwald-Stadler <pmeerw@pmeerw.net> wrote:
>>
>>>> +static int ti_ads7950_read_raw(struct iio_dev *indio_dev,
>>>> +			       struct iio_chan_spec const *chan,
>>>> +			       int *val, int *val2, long m)
>>>> +{
>>>> +	struct ti_ads7950_state *st = iio_priv(indio_dev);
>>>> +	int ret;
>>>> +
>>>> +	switch (m) {
>>>> +	case IIO_CHAN_INFO_RAW:
>>>> +
>>>> +		ret = iio_device_claim_direct_mode(indio_dev);
>>>> +		if (ret < 0)
>>>> +			return ret;
>>>> +
>>>> +		ret = ti_ads7950_scan_direct(st, chan->address);
>>>> +		iio_device_release_direct_mode(indio_dev);
>>>> +		if (ret < 0)
>>>> +			return ret;
>>>> +
>>>> +		if (chan->address != TI_ADS7950_EXTRACT(ret, 12, 4))
>>>> +			return -EIO;
>>>> +
>>>> +		*val = TI_ADS7950_EXTRACT(ret, 0, 12);
>>>
>>> I'm not sure if I am doing this right. There are 8- 10- and 12-bit
>> versions of
>>> this chip. The 8- and 10-bit versions still return a 12-bit number
>> where the
>>> last 4 or 2 bits are always 0. Should I be shifting the 12-bit value
>> here
>>> based on the chip being used so that *val is 0-255 for 8-bit and
>> 0-1023 for
>>> 10-bit? Or should this be *really* raw and not even use
>> TI_ADS7950_EXTRACT()
>>> to mask the channel address bits?
>>
>> I'd shift and adjust _SCALE so that *val * scale gives mV
> It would also be fine to not do anything and let userspace deal with shifting and masking for
> buffered data.  Non buffered obviously still needs shifting and masking though!

I have sent a v3 patch already that does exactly this. Buffered data is 
untouched and unbuffered data is now correct with shifting and masking.

>
> I'd slightly prefer the doing nothing route but don't really care as both are valid uses of the ABI.
>
> Jonathan
>>
>>>> +
>>>> +		return IIO_VAL_INT;
>>>> +	case IIO_CHAN_INFO_SCALE:
>>>> +		ret = ti_ads7950_get_range(st);
>>>> +		if (ret < 0)
>>>> +			return ret;
>>>> +
>>>> +		*val = ret;
>>>> +		*val2 = chan->scan_type.realbits;
>>>> +
>>>> +		return IIO_VAL_FRACTIONAL_LOG2;
>>>> +	}
>>>> +
>>>> +	return -EINVAL;
>>>> +}
>

Re: [PATCH v2] iio: adc: New driver for TI ADS7950 chips

[Jonathan Cameron]

On 21/11/16 19:52, David Lechner wrote:
> On 11/20/2016 12:28 PM, David Lechner wrote:
>> This adds a new driver for the TI ADS7950 family of ADC chips. These
>> communicate using SPI and come in 8/10/12-bit and 4/8/12/16 channel
>> varieties.
>>
>> Signed-off-by: David Lechner <david@lechnology.com>
>> ---
>>
>> v2 changes:
>>
>> * Got rid of XX wildcards - using ADS7950 everywhere
>> * Fixed some macro parentheses issues
>> * Added TI_ prefix to macros to match ti_ prefixes used elsewhere
>> * Added space in rx_buf for holding timestamp
>> * Use iio_device_claim_direct_mode() and spi_message_init_with_transfers()
>>   helper functions
>> * Don't use dev_info() at end of probe
>> * Minor spelling and code style fixes
>>
>>  drivers/iio/adc/Kconfig      |  13 ++
>>  drivers/iio/adc/Makefile     |   1 +
>>  drivers/iio/adc/ti-ads7950.c | 488 +++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 502 insertions(+)
>>  create mode 100644 drivers/iio/adc/ti-ads7950.c
>>
> 
> ...
> 
>> diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
>> new file mode 100644
>> index 0000000..d0b76bd
>> --- /dev/null
>> +++ b/drivers/iio/adc/ti-ads7950.c
> 
> ...
> 
>> +static irqreturn_t ti_ads7950_trigger_handler(int irq, void *p)
>> +{
>> +    struct iio_poll_func *pf = p;
>> +    struct iio_dev *indio_dev = pf->indio_dev;
>> +    struct ti_ads7950_state *st = iio_priv(indio_dev);
>> +    int b_sent;
>> +
>> +    b_sent = spi_sync(st->spi, &st->ring_msg);
> 
> hmm, I copied this from another driver, but spi_sync() in IRQ handler does not sound like a good idea (spi_sync() can sleep). I will replace it with spi_async().
Umm.. spi_async doesn't make any sense here...

Key thing is that here you are in a threaded interrupt so sleeping is fine and
here I'd imagine it leads to simpler code.

It's a bit old but https://lwn.net/Articles/302043/ has a good description.

> 
> 
>> +    if (b_sent)
>> +        goto done;
>> +
>> +    iio_push_to_buffers_with_timestamp(indio_dev, st->rx_buf,
>> +                       iio_get_time_ns(indio_dev));
>> +
>> +done:
>> +    iio_trigger_notify_done(indio_dev->trig);
>> +
>> +    return IRQ_HANDLED;
>> +}
> 
> ...
> 
>> +static int ti_ads7950_read_raw(struct iio_dev *indio_dev,
>> +                   struct iio_chan_spec const *chan,
>> +                   int *val, int *val2, long m)
>> +{
>> +    struct ti_ads7950_state *st = iio_priv(indio_dev);
>> +    int ret;
>> +
>> +    switch (m) {
>> +    case IIO_CHAN_INFO_RAW:
>> +
>> +        ret = iio_device_claim_direct_mode(indio_dev);
>> +        if (ret < 0)
>> +            return ret;
>> +
>> +        ret = ti_ads7950_scan_direct(st, chan->address);
>> +        iio_device_release_direct_mode(indio_dev);
>> +        if (ret < 0)
>> +            return ret;
>> +
>> +        if (chan->address != TI_ADS7950_EXTRACT(ret, 12, 4))
>> +            return -EIO;
>> +
>> +        *val = TI_ADS7950_EXTRACT(ret, 0, 12);
> 
> I'm not sure if I am doing this right. There are 8- 10- and 12-bit
> versions of this chip. The 8- and 10-bit versions still return a
> 12-bit number where the last 4 or 2 bits are always 0. Should I be
> shifting the 12-bit value here based on the chip being used so that
> *val is 0-255 for 8-bit and 0-1023 for 10-bit? Or should this be
> *really* raw and not even use TI_ADS7950_EXTRACT() to mask the
> channel address bits?
>> +
>> +        return IIO_VAL_INT;
>> +    case IIO_CHAN_INFO_SCALE:
>> +        ret = ti_ads7950_get_range(st);
>> +        if (ret < 0)
>> +            return ret;
>> +
>> +        *val = ret;
>> +        *val2 = chan->scan_type.realbits;
>> +
>> +        return IIO_VAL_FRACTIONAL_LOG2;
>> +    }
>> +
>> +    return -EINVAL;
>> +}
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html