LinuxLists.cc - [PATCH 1/2] dt-bindings: iio: adc: ti-ads1298: Add driver

2023-12-13 09:48:00

Subject: [PATCH 1/2] dt-bindings: iio: adc: ti-ads1298: Add driver

Skeleton driver for the TI ADS1298 medical ADC. This device is
typically used for ECG and similar measurements. Supports data
acquisition at configurable scale and sampling frequency.

Signed-off-by: Mike Looijmans <[email protected]>

---

.../bindings/iio/adc/ti,ads1298.yaml | 80 +++++++++++++++++++
1 file changed, 80 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1298.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads1298.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads1298.yaml
new file mode 100644
index 000000000000..7a160ba721eb
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/ti,ads1298.yaml
@@ -0,0 +1,80 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/ti,ads1298.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Texas Instruments' ads1298 medical ADC chips
+
+maintainers:
+ - Mike Looijmans <[email protected]>
+
+properties:
+ compatible:
+ enum:
+ - ti,ads1298
+
+ reg:
+ maxItems: 1
+
+ spi-cpha: true
+
+ reset-gpios:
+ maxItems: 1
+
+ avdd-supply:
+ description:
+ Analog power supply, voltage between AVDD and AVSS. When providing a
+ symmetric +/- 2.5V, the regulator should report 5V.
+
+ vref-supply:
+ description:
+ Optional reference voltage. If omitted, internal reference is used,
+ depending on analog supply this is 2.4 or 4V.
+
+ clocks:
+ description: Optional 2.048 MHz external source clock on CLK pin
+ maxItems: 1
+
+ clock-names:
+ const: clk
+
+ interrupts:
+ description: Interrupt on DRDY pin, triggers on falling edge
+ maxItems: 1
+
+ label: true
+
+required:
+ - compatible
+ - reg
+ - avdd-supply
+ - interrupts
+
+allOf:
+ - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/interrupt-controller/irq.h>
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ adc@1 {
+ reg = <1>;
+ compatible = "ti,ads1298";
+ label = "ads1298-1-ecg";
+ avdd-supply = <&reg_iso_5v_a>;
+ clock-names = "clk";
+ clocks = <&clk_ads1298>;
+ interrupt-parent = <&gpio0>;
+ interrupts = <78 IRQ_TYPE_EDGE_FALLING>;
+ spi-max-frequency = <20000000>;
+ spi-cpha;
+ };
+ };
+...
--
2.34.1

Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert

TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: [email protected]
W: http://www.topic.nl

Please consider the environment before printing this e-mail

2023-12-13 09:48:17

by Mike Looijmans

[permalink] [raw]

Subject: [PATCH 2/2] iio: adc: ti-ads1298: Add driver

Skeleton driver for the TI ADS1298 medical ADC. This device is
typically used for ECG and similar measurements. Supports data
acquisition at configurable scale and sampling frequency.

Signed-off-by: Mike Looijmans <[email protected]>

---

drivers/iio/adc/Kconfig | 12 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/ti-ads1298.c | 740 +++++++++++++++++++++++++++++++++++
3 files changed, 753 insertions(+)
create mode 100644 drivers/iio/adc/ti-ads1298.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 35f9867da12c..57ccbf347828 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1318,6 +1318,18 @@ config TI_ADS8688
This driver can also be built as a module. If so, the module will be
called ti-ads8688.

+config TI_ADS1298
+ tristate "Texas Instruments ADS1298"
+ depends on SPI
+ select IIO_BUFFER
+ default y
+ help
+ If you say yes here you get support for Texas Instruments ADS1298
+ medical ADC chips
+
+ This driver can also be built as a module. If so, the module will be
+ called ti-ads1298.
+
config TI_ADS124S08
tristate "Texas Instruments ADS124S08"
depends on SPI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index bee11d442af4..ff0e3633eded 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -117,6 +117,7 @@ obj-$(CONFIG_TI_ADS7924) += ti-ads7924.o
obj-$(CONFIG_TI_ADS7950) += ti-ads7950.o
obj-$(CONFIG_TI_ADS8344) += ti-ads8344.o
obj-$(CONFIG_TI_ADS8688) += ti-ads8688.o
+obj-$(CONFIG_TI_ADS1298) += ti-ads1298.o
obj-$(CONFIG_TI_ADS124S08) += ti-ads124s08.o
obj-$(CONFIG_TI_ADS131E08) += ti-ads131e08.o
obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
diff --git a/drivers/iio/adc/ti-ads1298.c b/drivers/iio/adc/ti-ads1298.c
new file mode 100644
index 000000000000..54466285063f
--- /dev/null
+++ b/drivers/iio/adc/ti-ads1298.c
@@ -0,0 +1,740 @@
+// SPDX-License-Identifier: GPL-2.0
+/* TI ADS1298 chip family driver
+ * Copyright (C) 2023 Topic Embedded Products
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+
+#include <linux/gpio/consumer.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/kfifo_buf.h>
+#include <linux/iio/sysfs.h>
+
+#include <asm/unaligned.h>
+
+#define ADS1298_WAKEUP 0x02
+#define ADS1298_STANDBY 0x04
+#define ADS1298_RESET 0x06
+#define ADS1298_START 0x08
+#define ADS1298_STOP 0x0A
+#define ADS1298_RDATAC 0x10
+#define ADS1298_SDATAC 0x11
+#define ADS1298_RDATA 0x12
+
+/* Commands */
+#define ADS1298_CMD_WAKEUP 0x02
+#define ADS1298_CMD_STANDBY 0x04
+#define ADS1298_CMD_RESET 0x06
+#define ADS1298_CMD_START 0x08
+#define ADS1298_CMD_STOP 0x0a
+#define ADS1298_CMD_RDATAC 0x10
+#define ADS1298_CMD_SDATAC 0x11
+#define ADS1298_CMD_RDATA 0x12
+#define ADS1298_CMD_RREG 0x20
+#define ADS1298_CMD_WREG 0x40
+
+/* Registers */
+#define ADS1298_REG_ID 0x00
+#define ADS1298_REG_CONFIG1 0x01
+#define ADS1298_MASK_CONFIG1_HR BIT(7)
+#define ADS1298_MASK_CONFIG1_DR 0x07
+#define ADS1298_REG_CONFIG2 0x02
+#define ADS1298_MASK_CONFIG2_RESERVED BIT(6)
+#define ADS1298_MASK_CONFIG2_WCT_CHOP BIT(5)
+#define ADS1298_MASK_CONFIG2_INT_TEST BIT(4)
+#define ADS1298_MASK_CONFIG2_TEST_AMP BIT(2)
+#define ADS1298_MASK_CONFIG2_TEST_FREQ_DC GENMASK(1, 0)
+#define ADS1298_MASK_CONFIG2_TEST_FREQ_SLOW 0
+#define ADS1298_MASK_CONFIG2_TEST_FREQ_FAST BIT(0)
+#define ADS1298_REG_CONFIG3 0x03
+#define ADS1298_MASK_CONFIG3_PWR_REFBUF BIT(7)
+#define ADS1298_MASK_CONFIG3_RESERVED BIT(6)
+#define ADS1298_MASK_CONFIG3_VREF_4V BIT(5)
+#define ADS1298_REG_LOFF 0x04
+#define ADS1298_REG_CHnSET(n) (0x05 + n)
+#define ADS1298_MASK_CH_PD BIT(7)
+#define ADS1298_MASK_CH_PGA GENMASK(6, 4)
+#define ADS1298_MASK_CH_MUX GENMASK(2, 0)
+#define ADS1298_REG_LOFF_STATP 0x12
+#define ADS1298_REG_LOFF_STATN 0x13
+
+#define ADS1298_REG_CONFIG4 0x17
+#define ADS1298_MASK_CONFIG4_SINGLE_SHOT BIT(3)
+#define ADS1298_REG_WCT1 0x18
+#define ADS1298_REG_WCT2 0x19
+
+#define ADS1298_MAX_CHANNELS 8
+#define ADS1298_CLK_RATE 2048000
+/*
+ * Read/write register commands require 4 clocks to decode, for speeds above
+ * 2x the clock rate, this would require extra time between the command byte and
+ * the data. Much simpler is to just limit the SPI transfer speed while doing
+ * register access.
+ */
+#define ADS1298_SPI_BUS_SPEED_SLOW ADS1298_CLK_RATE
+/* For reading and writing registers, we need a 3-byte buffer */
+#define ADS1298_SPI_CMD_BUFFER_SIZE 3
+/* Outputs status word and 8 samples of 24 bits each, plus the command byte */
+#define ADS1298_SPI_RDATA_BUFFER_SIZE ((ADS1298_MAX_CHANNELS + 1) * 3 + 1)
+
+struct ads1298_private {
+ const struct ads1298_chip_info *chip_info;
+ struct spi_device *spi;
+ struct regulator *reg_avdd;
+ struct regulator *reg_vref;
+ struct clk *clk;
+ struct regmap *regmap;
+ struct completion completion;
+ struct iio_trigger *trig;
+ struct spi_transfer rdata_xfer;
+ struct spi_message rdata_msg;
+ spinlock_t irq_busy_lock; /* Handshake between SPI and DRDY irqs */
+ bool rdata_xfer_busy;
+
+ /* Temporary storage for demuxing data after SPI transfer */
+ u8 bounce_buffer[ADS1298_MAX_CHANNELS * 4];
+
+ /* For synchronous SPI exchanges (read/write registers) */
+ u8 cmd_buffer[ADS1298_SPI_CMD_BUFFER_SIZE] ____cacheline_aligned;
+
+ /* Buffer used for incoming SPI data */
+ u8 rx_buffer[ADS1298_SPI_RDATA_BUFFER_SIZE] ____cacheline_aligned;
+ /* Contains the RDATA command and zeroes to clock out */
+ u8 tx_buffer[ADS1298_SPI_RDATA_BUFFER_SIZE] ____cacheline_aligned;
+};
+
+#define ADS1298_CHAN(index) \
+{ \
+ .type = IIO_VOLTAGE, \
+ .indexed = 1, \
+ .channel = index, \
+ .address = 3 * index + 4, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+ BIT(IIO_CHAN_INFO_SCALE), \
+ .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
+ BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
+ .scan_index = index, \
+ .scan_type = { \
+ .sign = 's', \
+ .realbits = 24, \
+ .storagebits = 32, \
+ .endianness = IIO_BE, \
+ }, \
+}
+
+static const struct iio_chan_spec ads1298_channels[] = {
+ ADS1298_CHAN(0),
+ ADS1298_CHAN(1),
+ ADS1298_CHAN(2),
+ ADS1298_CHAN(3),
+ ADS1298_CHAN(4),
+ ADS1298_CHAN(5),
+ ADS1298_CHAN(6),
+ ADS1298_CHAN(7),
+};
+
+static int ads1298_write_cmd(struct ads1298_private *priv, u8 command)
+{
+ struct spi_transfer cmd_xfer = {
+ .tx_buf = priv->cmd_buffer,
+ .rx_buf = priv->cmd_buffer,
+ .len = 1,
+ .speed_hz = ADS1298_SPI_BUS_SPEED_SLOW,
+ .delay.value = 2,
+ .delay.unit = SPI_DELAY_UNIT_USECS,
+ };
+
+ priv->cmd_buffer[0] = command;
+
+ return spi_sync_transfer(priv->spi, &cmd_xfer, 1);
+}
+
+static int ads1298_read_one(struct ads1298_private *priv, int chan_index)
+{
+ int ret;
+
+ /* Enable the channel */
+ ret = regmap_update_bits(priv->regmap, ADS1298_REG_CHnSET(chan_index),
+ ADS1298_MASK_CH_PD, 0);
+ if (ret)
+ return ret;
+
+ /* Enable single-shot mode, so we don't need to send a STOP */
+ ret = regmap_update_bits(priv->regmap, ADS1298_REG_CONFIG4,
+ ADS1298_MASK_CONFIG4_SINGLE_SHOT,
+ ADS1298_MASK_CONFIG4_SINGLE_SHOT);
+ if (ret)
+ return ret;
+
+ reinit_completion(&priv->completion);
+
+ ret = ads1298_write_cmd(priv, ADS1298_CMD_START);
+ if (ret < 0) {
+ dev_err(&priv->spi->dev, "CMD_START error: %d\n", ret);
+ return ret;
+ }
+
+ /* Cannot take longer than 40ms (250Hz) */
+ ret = wait_for_completion_timeout(&priv->completion,
+ msecs_to_jiffies(50));
+ if (!ret)
+ return -ETIMEDOUT;
+
+ return 0;
+}
+
+static int ads1298_get_samp_freq(struct ads1298_private *priv, int *val)
+{
+ unsigned long rate;
+ int ret;
+ unsigned int cfg;
+
+ ret = regmap_read(priv->regmap, ADS1298_REG_CONFIG1, &cfg);
+ if (ret)
+ return ret;
+
+ if (priv->clk)
+ rate = clk_get_rate(priv->clk);
+ else
+ rate = ADS1298_CLK_RATE;
+ if (!rate)
+ return -EINVAL;
+
+ if (cfg & ADS1298_MASK_CONFIG1_HR)
+ rate >>= 6; /* HR mode */
+ else
+ rate >>= 7; /* LP mode */
+
+ *val = rate >> (cfg & ADS1298_MASK_CONFIG1_DR);
+
+ return IIO_VAL_INT;
+}
+
+static int ads1298_set_samp_freq(struct ads1298_private *priv, int val)
+{
+ unsigned long rate;
+ unsigned int factor;
+ unsigned int cfg;
+
+ if (priv->clk)
+ rate = clk_get_rate(priv->clk);
+ else
+ rate = ADS1298_CLK_RATE;
+ if (!rate)
+ return -EINVAL;
+
+ factor = (rate >> 6) / val;
+ if (factor >= 128) {
+ cfg = 0x06; /* Lowest frequency possible, must use LR mode */
+ } else if (factor <= 1) {
+ cfg = ADS1298_MASK_CONFIG1_HR; /* Fastest possible */
+ } else {
+ cfg = fls(factor) - 1;
+ cfg |= ADS1298_MASK_CONFIG1_HR; /* Use HR mode */
+ }
+
+ return regmap_update_bits(priv->regmap, ADS1298_REG_CONFIG1,
+ ADS1298_MASK_CONFIG1_HR | ADS1298_MASK_CONFIG1_DR,
+ cfg);
+}
+
+static const u8 ads1298_pga_settings[] = { 6, 1, 2, 3, 4, 8, 12 };
+
+static int ads1298_get_scale(struct ads1298_private *priv,
+ int channel, int *val, int *val2)
+{
+ int ret;
+ unsigned int regval;
+ u8 gain;
+
+ if (priv->reg_vref) {
+ ret = regulator_get_voltage(priv->reg_vref);
+ if (ret < 0)
+ return ret;
+
+ *val = ret / 1000; /* Convert to millivolts */
+ } else {
+ ret = regmap_read(priv->regmap, ADS1298_REG_CONFIG3, &regval);
+ if (ret)
+ return ret;
+
+ /* Refererence in millivolts */
+ *val = regval & ADS1298_MASK_CONFIG3_VREF_4V ? 4000 : 2400;
+ }
+
+ ret = regmap_read(priv->regmap, ADS1298_REG_CHnSET(channel), &regval);
+ if (ret)
+ return ret;
+
+ gain = ads1298_pga_settings[FIELD_GET(ADS1298_MASK_CH_PGA, regval)];
+ *val /= gain; /* Full scale is VREF / gain */
+
+ *val2 = 23; /* Signed 24-bit, max amplitude is 2^23 */
+
+ return IIO_VAL_FRACTIONAL_LOG2;
+}
+
+
+static int ads1298_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct ads1298_private *priv = iio_priv(indio_dev);
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ /* When busy, just peek in the buffer */
+ if (!iio_buffer_enabled(indio_dev)) {
+ ret = ads1298_read_one(priv, chan->scan_index);
+ if (ret)
+ return ret;
+ }
+ *val = sign_extend32(get_unaligned_be24(
+ priv->rx_buffer + chan->address), 23);
+ ret = IIO_VAL_INT;
+ break;
+ case IIO_CHAN_INFO_SCALE:
+ ret = ads1298_get_scale(priv, chan->channel, val, val2);
+ break;
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ ret = ads1298_get_samp_freq(priv, val);
+ break;
+ case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+ ret = regmap_read(priv->regmap, ADS1298_REG_CONFIG1, val);
+ if (!ret) {
+ ret = IIO_VAL_INT;
+ *val = (16 << (*val & ADS1298_MASK_CONFIG1_DR));
+ }
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+
+ return ret;
+}
+
+static int ads1298_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan, int val,
+ int val2, long mask)
+{
+ struct ads1298_private *priv = iio_priv(indio_dev);
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ ret = ads1298_set_samp_freq(priv, val);
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+
+ return ret;
+}
+
+static int ads1298_reg_write(void *context, unsigned int reg, unsigned int val)
+{
+ struct ads1298_private *priv = context;
+ struct spi_transfer reg_write_xfer = {
+ .tx_buf = priv->cmd_buffer,
+ .rx_buf = priv->cmd_buffer,
+ .len = 3,
+ .speed_hz = ADS1298_SPI_BUS_SPEED_SLOW,
+ .delay.value = 2,
+ .delay.unit = SPI_DELAY_UNIT_USECS,
+ };
+
+ priv->cmd_buffer[0] = ADS1298_CMD_WREG | reg;
+ priv->cmd_buffer[1] = 0x0;
+ priv->cmd_buffer[2] = val;
+
+ return spi_sync_transfer(priv->spi, &reg_write_xfer, 1);
+}
+
+static int ads1298_reg_read(void *context, unsigned int reg, unsigned int *val)
+{
+ struct ads1298_private *priv = context;
+ struct spi_transfer reg_read_xfer = {
+ .tx_buf = priv->cmd_buffer,
+ .rx_buf = priv->cmd_buffer,
+ .len = 3,
+ .speed_hz = ADS1298_SPI_BUS_SPEED_SLOW,
+ .delay.value = 2,
+ .delay.unit = SPI_DELAY_UNIT_USECS,
+ };
+ int ret;
+
+ priv->cmd_buffer[0] = ADS1298_CMD_RREG | reg;
+ priv->cmd_buffer[1] = 0x0;
+ priv->cmd_buffer[2] = 0;
+
+ ret = spi_sync_transfer(priv->spi, &reg_read_xfer, 1);
+ if (ret)
+ return ret;
+
+ *val = priv->cmd_buffer[2];
+
+ return 0;
+}
+
+static int ads1298_reg_access(struct iio_dev *indio_dev, unsigned int reg,
+ unsigned int writeval, unsigned int *readval)
+{
+ struct ads1298_private *priv = iio_priv(indio_dev);
+
+ if (!readval)
+ return regmap_write(priv->regmap, reg, writeval);
+
+ return regmap_read(priv->regmap, reg, readval);
+}
+
+static int ads1298_update_scan_mode(struct iio_dev *indio_dev,
+ const unsigned long *scan_mask)
+{
+ struct ads1298_private *priv = iio_priv(indio_dev);
+ int i;
+
+ /* Power down channels that aren't in use */
+ for (i = 0; i < ADS1298_MAX_CHANNELS; i++) {
+ regmap_update_bits(priv->regmap, ADS1298_REG_CHnSET(i),
+ ADS1298_MASK_CH_PD,
+ test_bit(i, scan_mask) ?
+ 0 : ADS1298_MASK_CH_PD);
+ }
+
+ return 0;
+}
+
+static const struct iio_info ads1298_info = {
+ .read_raw = &ads1298_read_raw,
+ .write_raw = &ads1298_write_raw,
+ .update_scan_mode = &ads1298_update_scan_mode,
+ .debugfs_reg_access = &ads1298_reg_access,
+};
+
+static void ads1298_rdata_unmark_busy(struct ads1298_private *priv)
+{
+ unsigned long flags;
+
+ /* Notify we're no longer waiting for the SPI transfer to complete */
+ spin_lock_irqsave(&priv->irq_busy_lock, flags);
+ priv->rdata_xfer_busy = false;
+ spin_unlock_irqrestore(&priv->irq_busy_lock, flags);
+}
+
+static void ads1298_rdata_complete(void *context)
+{
+ struct iio_dev *indio_dev = context;
+ struct ads1298_private *priv = iio_priv(indio_dev);
+ int scan_index;
+ u8 *bounce = priv->bounce_buffer;
+
+ if (!iio_buffer_enabled(indio_dev)) {
+ /* Happens when running in single transfer mode */
+ ads1298_rdata_unmark_busy(priv);
+ complete(&priv->completion);
+ return;
+ }
+
+ /* Demux the channel data into our bounce buffer */
+ for_each_set_bit(scan_index, indio_dev->active_scan_mask,
+ indio_dev->masklength) {
+ const struct iio_chan_spec *scan_chan =
+ &indio_dev->channels[scan_index];
+ const u8 *data = priv->rx_buffer + scan_chan->address;
+
+ /* Transfer 24-bit value into 32-bit array */
+ memcpy(bounce + 1, data, 3);
+ bounce += 4;
+ }
+
+ /* rx_buffer can be overwritten from this point on */
+ ads1298_rdata_unmark_busy(priv);
+
+ iio_push_to_buffers(indio_dev, priv->bounce_buffer);
+}
+
+static irqreturn_t ads1298_interrupt(int irq, void *dev_id)
+{
+ struct iio_dev *indio_dev = dev_id;
+ struct ads1298_private *priv = iio_priv(indio_dev);
+ unsigned long flags;
+ bool wasbusy;
+
+ /* Prevent that we submit a message that was still in progress */
+ spin_lock_irqsave(&priv->irq_busy_lock, flags);
+ wasbusy = priv->rdata_xfer_busy;
+ priv->rdata_xfer_busy = true;
+ spin_unlock_irqrestore(&priv->irq_busy_lock, flags);
+
+ if (!wasbusy)
+ spi_async(priv->spi, &priv->rdata_msg);
+
+ return IRQ_HANDLED;
+};
+
+static int ads1298_buffer_postenable(struct iio_dev *indio_dev)
+{
+ struct ads1298_private *priv = iio_priv(indio_dev);
+ int ret;
+
+ /* Disable single-shot mode */
+ ret = regmap_update_bits(priv->regmap, ADS1298_REG_CONFIG4,
+ ADS1298_MASK_CONFIG4_SINGLE_SHOT, 0);
+ if (ret)
+ return ret;
+
+ return ads1298_write_cmd(priv, ADS1298_CMD_START);
+}
+
+static int ads1298_buffer_predisable(struct iio_dev *indio_dev)
+{
+ struct ads1298_private *priv = iio_priv(indio_dev);
+
+ return ads1298_write_cmd(priv, ADS1298_CMD_STOP);
+}
+
+static const struct iio_buffer_setup_ops ads1298_setup_ops = {
+ .postenable = &ads1298_buffer_postenable,
+ .predisable = &ads1298_buffer_predisable,
+};
+
+static void ads1298_reg_disable(void *reg)
+{
+ regulator_disable(reg);
+}
+
+static const struct regmap_range ads1298_regmap_volatile_range[] = {
+ regmap_reg_range(ADS1298_REG_LOFF_STATP, ADS1298_REG_LOFF_STATN),
+};
+
+static const struct regmap_access_table ads1298_regmap_volatile = {
+ .yes_ranges = ads1298_regmap_volatile_range,
+ .n_yes_ranges = ARRAY_SIZE(ads1298_regmap_volatile_range),
+};
+
+static const struct regmap_config ads1298_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .reg_read = ads1298_reg_read,
+ .reg_write = ads1298_reg_write,
+ .max_register = ADS1298_REG_WCT2,
+ .volatile_table = &ads1298_regmap_volatile,
+ .cache_type = REGCACHE_RBTREE,
+};
+
+static const char *ads1298_family_name(unsigned int id)
+{
+ switch (id & 0xe0) {
+ case 0x80:
+ return "ADS129x";
+ case 0xc0:
+ return "ADS129xR";
+ default:
+ return "(unknown)";
+ }
+}
+
+static int ads1298_init(struct ads1298_private *priv)
+{
+ struct device *dev = &priv->spi->dev;
+ int ret;
+ unsigned int val;
+
+ /* Device initializes into RDATAC mode, which we don't want. */
+ ret = ads1298_write_cmd(priv, ADS1298_CMD_SDATAC);
+ if (ret)
+ return ret;
+
+ ret = regmap_read(priv->regmap, ADS1298_REG_ID, &val);
+ if (ret)
+ return ret;
+
+ if ((val & 0x18) == 0x10) {
+ dev_info(dev, "Found %s, %d channels\n",
+ ads1298_family_name(val),
+ 4 + 2 * (val & 0x07));
+ } else {
+ dev_err(dev, "Unknown ID: 0x%x\n", val);
+ return -ENODEV;
+ }
+
+ /* Enable internal test signal, double amplitude, double frequency */
+ regmap_write(priv->regmap, ADS1298_REG_CONFIG2,
+ ADS1298_MASK_CONFIG2_RESERVED |
+ ADS1298_MASK_CONFIG2_INT_TEST |
+ ADS1298_MASK_CONFIG2_TEST_AMP |
+ ADS1298_MASK_CONFIG2_TEST_FREQ_FAST);
+
+ val = ADS1298_MASK_CONFIG3_RESERVED; /* Must write 1 always */
+ if (!priv->reg_vref) {
+ /* Enable internal reference */
+ val |= ADS1298_MASK_CONFIG3_PWR_REFBUF;
+ /* Use 4V VREF when power supply is at least 4.4V */
+ if (regulator_get_voltage(priv->reg_avdd) >= 4400000)
+ val |= ADS1298_MASK_CONFIG3_VREF_4V;
+ }
+ regmap_write(priv->regmap, ADS1298_REG_CONFIG3, val);
+
+ for (val = 0; val < ADS1298_MAX_CHANNELS; val++) {
+ /* Set mux to analog input, PGA = 6x */
+ regmap_write(priv->regmap, ADS1298_REG_CHnSET(val), 0x00);
+ }
+
+ return 0;
+}
+
+static int ads1298_probe(struct spi_device *spi)
+{
+ struct ads1298_private *priv;
+ struct iio_dev *indio_dev;
+ struct device *dev = &spi->dev;
+ struct gpio_desc *reset_gpio;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(dev, sizeof(*priv));
+ if (indio_dev == NULL)
+ return -ENOMEM;
+
+ priv = iio_priv(indio_dev);
+
+ reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+ if (IS_ERR(reset_gpio))
+ return dev_err_probe(dev, ret, "Cannot get reset GPIO\n");
+
+ priv->reg_avdd = devm_regulator_get(dev, "avdd");
+ if (IS_ERR(priv->reg_avdd))
+ return dev_err_probe(dev, PTR_ERR(priv->reg_avdd),
+ "Failed to get avdd regulator\n");
+
+ /* VREF can be supplied externally. Otherwise use internal reference */
+ priv->reg_vref = devm_regulator_get_optional(dev, "vref");
+ if (IS_ERR(priv->reg_vref)) {
+ if (PTR_ERR(priv->reg_vref) == -ENODEV)
+ priv->reg_vref = NULL;
+ else
+ return dev_err_probe(dev, PTR_ERR(priv->reg_avdd),
+ "Failed to get vref regulator\n");
+ } else {
+ ret = regulator_enable(priv->reg_vref);
+ if (ret)
+ return ret;
+
+ ret = devm_add_action_or_reset(dev, ads1298_reg_disable,
+ priv->reg_vref);
+ if (ret)
+ return ret;
+ }
+
+ priv->clk = devm_clk_get_enabled(dev, "clk");
+ if (IS_ERR(priv->clk))
+ return dev_err_probe(dev, PTR_ERR(priv->clk),
+ "Failed to get clk\n");
+
+ ret = regulator_enable(priv->reg_avdd);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Failed to enable avdd regulator\n");
+
+ ret = devm_add_action_or_reset(dev, ads1298_reg_disable,
+ priv->reg_avdd);
+ if (ret)
+ return ret;
+
+ priv->spi = spi;
+ init_completion(&priv->completion);
+ spin_lock_init(&priv->irq_busy_lock);
+ priv->regmap = devm_regmap_init(dev, NULL, priv,
+ &ads1298_regmap_config);
+ if (IS_ERR(priv->regmap))
+ return PTR_ERR(priv->regmap);
+
+ priv->tx_buffer[0] = ADS1298_CMD_RDATA;
+ priv->rdata_xfer.tx_buf = priv->tx_buffer;
+ priv->rdata_xfer.rx_buf = priv->rx_buffer;
+ priv->rdata_xfer.len = ADS1298_SPI_RDATA_BUFFER_SIZE;
+ /* Must keep CS low for 4 clocks */
+ priv->rdata_xfer.delay.value = 2;
+ priv->rdata_xfer.delay.unit = SPI_DELAY_UNIT_USECS;
+ spi_message_init_with_transfers(&priv->rdata_msg, &priv->rdata_xfer, 1);
+ priv->rdata_msg.complete = &ads1298_rdata_complete;
+ priv->rdata_msg.context = indio_dev;
+
+ /* Avoid giving all the same name, iio scope doesn't handle that well */
+ indio_dev->name = devm_kasprintf(dev, GFP_KERNEL, "%s@%s",
+ spi_get_device_id(spi)->name,
+ dev_name(dev));
+ indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
+ indio_dev->channels = ads1298_channels;
+ indio_dev->num_channels = ADS1298_MAX_CHANNELS;
+ indio_dev->info = &ads1298_info;
+
+ if (reset_gpio) {
+ udelay(1); /* Minimum pulse width is 2 clocks at 2MHz */
+ gpiod_set_value(reset_gpio, 0);
+ } else {
+ ret = ads1298_write_cmd(priv, ADS1298_CMD_RESET);
+ if (ret)
+ return dev_err_probe(dev, ret, "RESET failed\n");
+ }
+ /* Wait 18 clock cycles for reset command to complete */
+ udelay(9);
+
+ ret = ads1298_init(priv);
+ if (ret)
+ return dev_err_probe(dev, ret, "Init failed\n");
+
+ ret = devm_request_irq(&spi->dev, spi->irq, &ads1298_interrupt,
+ IRQF_TRIGGER_FALLING, indio_dev->name,
+ indio_dev);
+ if (ret)
+ return ret;
+
+ ret = devm_iio_kfifo_buffer_setup(&spi->dev, indio_dev,
+ &ads1298_setup_ops);
+ if (ret)
+ return ret;
+
+ return devm_iio_device_register(dev, indio_dev);
+}
+
+static const struct spi_device_id ads1298_id[] = {
+ { "ads1298", },
+ { }
+};
+MODULE_DEVICE_TABLE(spi, ads1298_id);
+
+static const struct of_device_id ads1298_of_table[] = {
+ { .compatible = "ti,ads1298" },
+ { },
+};
+MODULE_DEVICE_TABLE(of, ads1298_of_table);
+
+static struct spi_driver ads1298_driver = {
+ .driver = {
+ .name = "ads1298",
+ .of_match_table = ads1298_of_table,
+ },
+ .probe = ads1298_probe,
+ .id_table = ads1298_id,
+};
+module_spi_driver(ads1298_driver);
+
+MODULE_AUTHOR("Mike Looijmans <[email protected]>");
+MODULE_DESCRIPTION("TI ADS1298 ADC");
+MODULE_LICENSE("GPL");
--
2.34.1

Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert

TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: [email protected]
W: http://www.topic.nl

Please consider the environment before printing this e-mail

2023-12-13 15:28:19

by Andy Shevchenko

[permalink] [raw]

Subject: Re: [PATCH 2/2] iio: adc: ti-ads1298: Add driver

On Wed, Dec 13, 2023 at 10:47:22AM +0100, Mike Looijmans wrote:
> Skeleton driver for the TI ADS1298 medical ADC. This device is
> typically used for ECG and similar measurements. Supports data
> acquisition at configurable scale and sampling frequency.

...

> +config TI_ADS1298
> + tristate "Texas Instruments ADS1298"
> + depends on SPI
> + select IIO_BUFFER

> + default y

Huh?!

> + help
> + If you say yes here you get support for Texas Instruments ADS1298
> + medical ADC chips
> +
> + This driver can also be built as a module. If so, the module will be
> + called ti-ads1298.

...

> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>

> +#include <linux/kernel.h>

Is it used as a proxy? (At least for array_size.h)
Please use real headers in such case.

> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>

> +#include <linux/gpio/consumer.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>

This is interesting grouping, but okay, I understand the point.

> +#include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/kfifo_buf.h>
> +#include <linux/iio/sysfs.h>

...

> +#define ADS1298_CLK_RATE 2048000

Units? _HZ ?

...

> +/* Outputs status word and 8 samples of 24 bits each, plus the command byte */

/* Outputs status word and 8 24-bit samples, plus the command byte */

a bit shorter.

> +#define ADS1298_SPI_RDATA_BUFFER_SIZE ((ADS1298_MAX_CHANNELS + 1) * 3 + 1)

...

> +#define ADS1298_CHAN(index) \
> +{ \
> + .type = IIO_VOLTAGE, \
> + .indexed = 1, \
> + .channel = index, \
> + .address = 3 * index + 4, \

Hmm... does this 3 have a distinct definition?

> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_SCALE), \

Can be written as below

.info_mask_separate = \
BIT(IIO_CHAN_INFO_RAW) | \
BIT(IIO_CHAN_INFO_SCALE), \

> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
> + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \

.info_mask_shared_by_all = \
BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \

> + .scan_index = index, \
> + .scan_type = { \
> + .sign = 's', \
> + .realbits = 24, \
> + .storagebits = 32, \
> + .endianness = IIO_BE, \
> + }, \
> +}

...

> +static int ads1298_write_cmd(struct ads1298_private *priv, u8 command)
> +{
> + struct spi_transfer cmd_xfer = {
> + .tx_buf = priv->cmd_buffer,
> + .rx_buf = priv->cmd_buffer,
> + .len = 1,

sizeof(command) ?

> + .speed_hz = ADS1298_SPI_BUS_SPEED_SLOW,

> + .delay.value = 2,
> + .delay.unit = SPI_DELAY_UNIT_USECS,

.delay = {
.value = ...
.unit = ...
},

> + };

> + priv->cmd_buffer[0] = command;
> +
> + return spi_sync_transfer(priv->spi, &cmd_xfer, 1);
> +}

...

> + /* Cannot take longer than 40ms (250Hz) */
> + ret = wait_for_completion_timeout(&priv->completion,
> + msecs_to_jiffies(50));

One line?

> + if (!ret)
> + return -ETIMEDOUT;

...

> + if (cfg & ADS1298_MASK_CONFIG1_HR)
> + rate >>= 6; /* HR mode */
> + else
> + rate >>= 7; /* LP mode */

Are those magic numbers defined?

...

> + factor = (rate >> 6) / val;

Is it the same 6 semantically as above?

...

> + return IIO_VAL_FRACTIONAL_LOG2;
> +}
> +
> +

One blank line is enough.

> + *val = sign_extend32(get_unaligned_be24(
> + priv->rx_buffer + chan->address), 23);

Strange indentation.

*val = sign_extend32(get_unaligned_be24(priv->rx_buffer + chan->address),
23);

...

> + case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> + ret = regmap_read(priv->regmap, ADS1298_REG_CONFIG1, val);
> + if (!ret) {

Why not using standard pattern?

if (ret)
return ret;

(see below)

> + ret = IIO_VAL_INT;
> + *val = (16 << (*val & ADS1298_MASK_CONFIG1_DR));

Outer parentheses are redundant.

> + }
> + break;

return IIO_VAL_INT;

> + default:
> + ret = -EINVAL;
> + break;

return directly.

> + }

> + return ret;

It will gone.

...

> +static int ads1298_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int val,
> + int val2, long mask)
> +{
> + struct ads1298_private *priv = iio_priv(indio_dev);

> + int ret;

No need, just return directly.

> + switch (mask) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + ret = ads1298_set_samp_freq(priv, val);
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> + return ret;
> +}

...

> +static int ads1298_reg_access(struct iio_dev *indio_dev, unsigned int reg,
> + unsigned int writeval, unsigned int *readval)
> +{
> + struct ads1298_private *priv = iio_priv(indio_dev);

> + if (!readval)

Perhaps positive conditional?

if (readval)
return readval;
return writeval;

> + return regmap_write(priv->regmap, reg, writeval);
> +
> + return regmap_read(priv->regmap, reg, readval);
> +}

...

> + /* Power down channels that aren't in use */
> + for (i = 0; i < ADS1298_MAX_CHANNELS; i++) {
> + regmap_update_bits(priv->regmap, ADS1298_REG_CHnSET(i),
> + ADS1298_MASK_CH_PD,
> + test_bit(i, scan_mask) ?
> + 0 : ADS1298_MASK_CH_PD);

Broken indentation.

> + }

...

> +static void ads1298_rdata_unmark_busy(struct ads1298_private *priv)
> +{
> + unsigned long flags;
> +
> + /* Notify we're no longer waiting for the SPI transfer to complete */
> + spin_lock_irqsave(&priv->irq_busy_lock, flags);
> + priv->rdata_xfer_busy = false;
> + spin_unlock_irqrestore(&priv->irq_busy_lock, flags);

Perhaps switch to use guard()?
(And scoped_guard() where it makes sense.)

> +}

> + /* Transfer 24-bit value into 32-bit array */
> + memcpy(bounce + 1, data, 3);

Hmm... Wouldn't get_unaligned_..24() work here better?

> + bounce += 4;

If so, you can iterate over u32 members directly without this += 4.

...

> +static const char *ads1298_family_name(unsigned int id)
> +{
> + switch (id & 0xe0) {

GENMASK() ?

> + case 0x80:
> + return "ADS129x";
> + case 0xc0:
> + return "ADS129xR";

Can we have these all magics be defined?

> + default:
> + return "(unknown)";
> + }
> +}

...

> + if ((val & 0x18) == 0x10) {

Ditto.

> + dev_info(dev, "Found %s, %d channels\n",
> + ads1298_family_name(val),
> + 4 + 2 * (val & 0x07));

Ditto for 0x07.

> + } else {
> + dev_err(dev, "Unknown ID: 0x%x\n", val);
> + return -ENODEV;
> + }

...

> + indio_dev = devm_iio_device_alloc(dev, sizeof(*priv));
> + if (indio_dev == NULL)

We do !indio_dev in this case.

> + return -ENOMEM;

...

> + ret = devm_add_action_or_reset(dev, ads1298_reg_disable,
> + priv->reg_vref);

One line?

> + if (ret)
> + return ret;

...

> + return dev_err_probe(dev, PTR_ERR(priv->clk),
> + "Failed to get clk\n");

Ditto.

...

> + return dev_err_probe(dev, ret,
> + "Failed to enable avdd regulator\n");

Ditto.

...

> + ret = devm_add_action_or_reset(dev, ads1298_reg_disable,
> + priv->reg_avdd);

Ditto.

...

> + priv->regmap = devm_regmap_init(dev, NULL, priv,
> + &ads1298_regmap_config);

Ditto.

...

> + /* Avoid giving all the same name, iio scope doesn't handle that well */

IIO

> + indio_dev->name = devm_kasprintf(dev, GFP_KERNEL, "%s@%s",
> + spi_get_device_id(spi)->name,
> + dev_name(dev));

No error check?

> + if (reset_gpio) {
> + udelay(1); /* Minimum pulse width is 2 clocks at 2MHz */

How do you know it's 2MHz? Is it always the same on all platforms / setups?

> + gpiod_set_value(reset_gpio, 0);
> + } else {
> + ret = ads1298_write_cmd(priv, ADS1298_CMD_RESET);
> + if (ret)
> + return dev_err_probe(dev, ret, "RESET failed\n");
> + }
> + /* Wait 18 clock cycles for reset command to complete */

Ditto.

> + udelay(9);

...

> + ret = devm_request_irq(&spi->dev, spi->irq, &ads1298_interrupt,

&spi->dev is different to dev?

> + IRQF_TRIGGER_FALLING, indio_dev->name,
> + indio_dev);

...

> + ret = devm_iio_kfifo_buffer_setup(&spi->dev, indio_dev,
> + &ads1298_setup_ops);

Ditto.

...

> +static const struct spi_device_id ads1298_id[] = {
> + { "ads1298", },

Inner comma is not needed.

> + { }
> +};

...

> +static const struct of_device_id ads1298_of_table[] = {
> + { .compatible = "ti,ads1298" },
> + { },

Drop the comma in terminator entry.

> +};

--
With Best Regards,
Andy Shevchenko

2023-12-13 16:24:04

On Wed, 31 Jan 2024 17:24:18 +0100
Mike Looijmans <[email protected]> wrote:

> On 14-12-2023 12:06, Jonathan Cameron wrote:
> > On Wed, 13 Dec 2023 10:47:22 +0100
> > Mike Looijmans <[email protected]> wrote:
> >
> >> Skeleton driver for the TI ADS1298 medical ADC. This device is
> >> typically used for ECG and similar measurements. Supports data
> >> acquisition at configurable scale and sampling frequency.
> >>
> >> Signed-off-by: Mike Looijmans <[email protected]>
> >>
> > Hi Mike,
> >
> > Various small things inline - some of which probably overlap with Andy's
> > comments.
>
>
> Working on it - Assume "yes to all" as my response on all suggestions,
> except for the IRQ handling...
>
> >
> > The larger one is I don't follow why this does manual protection against
> > concurrent handling of the result of an IRQ. Much easier to just use
> > an IRQ threaded handler and IRQF_ONESHOT to mask the irq entirely until
> > after the initial handling is done. If that doesn't work for some reason
> > then more comments needed to explain why!
>
> Yeah, definitely needs comments, and a bit of code too...
>
> This chip doesn't have a buffer, but it does "latch" the sample data
> when it receives a RDATA command (hence I use that in favor of RDATAC,
> which does not latch and might return corrupted data).
>
> To keep the latency as low as possible, I want to immediately start the
> SPI transfer when the DRDY interrupt arrives. My experiments have shown
> a big difference there, when using a ONESHOT trigger, it failed to meet
> the deadline at even the lowest frequencies.

That's interesting to hear. I wonder why - the overhead should be small.

Potentially it kicks the interrupt thread which then might kick an spi
controller thread rather than kicking the spi controller directly.
I think that depends on the SPI controller driver implementation choices
assuming things aren't contended - the defaults in the spi core
will take a fast path in current context if no contention - so it'll
happen in the interrupt thread. So in general case there should be
very little difference in the timing needed to kick off the actual
SPI transfer starting via the two paths. The bit you mention below
on overlapping is a gain. One trick we do to try and grab that back
if it's occasional is to poll the device in the trigger reenable
callback (only works if there is a register to say there is new data).

Maybe something odd going on in the interrupt controller driver...
It might not be useable for the higher frequencies, but should work
at reasonably low ones.

>
> The next SPI transfer can start immediately after the data has been
> copied into the intermediate buffer for IIO, the handler need not wait
> for IIO to process the data.

I'd be surprised if the processing time was significant (compared to the SPI
transfer times) - so this 'feels' like a bit of over the top micro
optimization.

>
> When the DRDY interrupt arrives, and there's an SPI transfer still in
> progress, it's not too late yet, the "latch" may save us still. Once the
> SPI transfer completes and the data is in the intermediate buffer, we
> should immediately start a new SPI transaction to latch and fetch the
> next set. (This code is still missing in the current version)
>
> Only when DRDY arrives a second time during an SPI transaction we missed
> the deadline and sample data was lost.

If you are running anywhere near speeds where this happens, things aren't
going to be reliable anyway. Whether that is a problem depends on your usecase.

So in conclusion, I'm surprised you are seeing such a difference in the
rates you can capture at. Might be worth trying to grab some debug info
to understand this a bit more given you are proposing an unusual structure
with maintenance costs etc.

>
> No further comments below from me, just kept the history for reference
Don't bother :) Better to crop as much as possible. We have archives for
history.

Jonathan
>