2016-11-20 18:28:29

by David Lechner

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

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 <[email protected]>
---

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 <[email protected]>
+ *
+ * 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 <[email protected]>");
+MODULE_DESCRIPTION("TI TI_ADS7950 ADC");
+MODULE_LICENSE("GPL v2");
--
2.7.4


2016-11-21 19:52:56

by David Lechner

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

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 <[email protected]>
> ---
>
> 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;
> +}

2016-11-21 22:54:28

by Peter Meerwald-Stadler

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


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

2016-11-22 07:24:01

by Jonathan Cameron

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



On 21 November 2016 22:54:24 GMT+00:00, Peter Meerwald-Stadler <[email protected]> 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.

2016-11-22 18:23:51

by David Lechner

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

On 11/22/2016 01:23 AM, Jonathan Cameron wrote:
>
>
> On 21 November 2016 22:54:24 GMT+00:00, Peter Meerwald-Stadler <[email protected]> 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;
>>>> +}
>

2016-11-24 20:35:22

by Jonathan Cameron

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

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 <[email protected]>
>> ---
>>
>> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html