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]>
---
Changes in v2:
Remove accidental "default y" in Kconfig
Indentation and similar cosmetic fixes
Magic numbers into constants
Short return paths in read_raw and write_raw
DMA buffer alignment
Bounce buffer is u32 instead of u8
Avoid races using claim_direct_mode
Check errors on all register accesses
Immediate SPI restart to reduce underruns
"name" is chip name, not unique
drivers/iio/adc/Kconfig | 11 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/ti-ads1298.c | 769 +++++++++++++++++++++++++++++++++++
3 files changed, 781 insertions(+)
create mode 100644 drivers/iio/adc/ti-ads1298.c
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 35f9867da12c..1d2d2eff15da 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1318,6 +1318,17 @@ 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
+ 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..539598b9f3fa
--- /dev/null
+++ b/drivers/iio/adc/ti-ads1298.c
@@ -0,0 +1,769 @@
+// SPDX-License-Identifier: GPL-2.0
+/* TI ADS1298 chip family driver
+ * Copyright (C) 2023 - 2024 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/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+#include <linux/spi/spi.h>
+#include <linux/sysfs.h>
+#include <linux/units.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_MASK_ID_FAMILY GENMASK(7, 3)
+#define ADS1298_MASK_ID_CHANNELS GENMASK(2, 0)
+#define ADS1298_ID_FAMILY_ADS129X 0x90
+#define ADS1298_ID_FAMILY_ADS129XR 0xd0
+#define ADS1298_REG_CONFIG1 0x01
+#define ADS1298_MASK_CONFIG1_HR BIT(7)
+#define ADS1298_MASK_CONFIG1_DR 0x07
+#define ADS1298_SHIFT_DR_HR 6
+#define ADS1298_SHIFT_DR_LP 7
+#define ADS1298_LOWEST_DR 0x06
+#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_HZ 2048000
+#define ADS1298_CLOCKS_TO_USECS(x) \
+ (DIV_ROUND_UP((x * MICROHZ_PER_HZ), ADS1298_CLK_RATE_HZ))
+/*
+ * 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_HZ
+/* For reading and writing registers, we need a 3-byte buffer */
+#define ADS1298_SPI_CMD_BUFFER_SIZE 3
+/* Outputs status word and 8 24-bit samples, 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 */
+ int rdata_xfer_busy;
+
+ /* Temporary storage for demuxing data after SPI transfer */
+ u32 bounce_buffer[ADS1298_MAX_CHANNELS];
+
+ /* For synchronous SPI exchanges (read/write registers) */
+ u8 cmd_buffer[ADS1298_SPI_CMD_BUFFER_SIZE] __aligned(IIO_DMA_MINALIGN);
+
+ /* Buffer used for incoming SPI data */
+ u8 rx_buffer[ADS1298_SPI_RDATA_BUFFER_SIZE];
+ /* Contains the RDATA command and zeroes to clock out */
+ u8 tx_buffer[ADS1298_SPI_RDATA_BUFFER_SIZE];
+};
+
+/* Three bytes per sample in RX buffer, starting at offset 4 */
+#define ADS1298_OFFSET_IN_RX_BUFFER(index) (3 * (index) + 4)
+
+#define ADS1298_CHAN(index) \
+{ \
+ .type = IIO_VOLTAGE, \
+ .indexed = 1, \
+ .channel = index, \
+ .address = ADS1298_OFFSET_IN_RX_BUFFER(index), \
+ .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_CPU, \
+ }, \
+}
+
+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 = sizeof(command),
+ .speed_hz = ADS1298_SPI_BUS_SPEED_SLOW,
+ .delay = {
+ .value = 2,
+ .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_HZ;
+ if (!rate)
+ return -EINVAL;
+
+ /* Data rate shift depends on HR/LP mode */
+ if (cfg & ADS1298_MASK_CONFIG1_HR)
+ rate >>= ADS1298_SHIFT_DR_HR;
+ else
+ rate >>= ADS1298_SHIFT_DR_LP;
+
+ *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_HZ;
+ if (!rate)
+ return -EINVAL;
+
+ factor = (rate >> ADS1298_SHIFT_DR_HR) / val;
+ if (factor >= 128) {
+ cfg = ADS1298_LOWEST_DR;
+ } 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, ®val);
+ 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), ®val);
+ 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:
+ ret = iio_device_claim_direct_mode(indio_dev);
+ if (ret)
+ return ret;
+
+ ret = ads1298_read_one(priv, chan->scan_index);
+
+ iio_device_release_direct_mode(indio_dev);
+
+ if (ret)
+ return ret;
+
+ *val = sign_extend32(get_unaligned_be24(priv->rx_buffer + chan->address),
+ 23);
+ return IIO_VAL_INT;
+ case IIO_CHAN_INFO_SCALE:
+ return ads1298_get_scale(priv, chan->channel, val, val2);
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ return ads1298_get_samp_freq(priv, val);
+ case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+ ret = regmap_read(priv->regmap, ADS1298_REG_CONFIG1, val);
+ if (ret)
+ return ret;
+
+ *val = (16 << (*val & ADS1298_MASK_CONFIG1_DR));
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+}
+
+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);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ return ads1298_set_samp_freq(priv, val);
+ default:
+ return -EINVAL;
+ }
+}
+
+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,
+ .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, ®_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,
+ .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, ®_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_read(priv->regmap, reg, readval);
+
+ return regmap_write(priv->regmap, reg, writeval);
+}
+
+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 = 0;
+ spin_unlock_irqrestore(&priv->irq_busy_lock, flags);
+}
+
+static int ads1298_update_scan_mode(struct iio_dev *indio_dev,
+ const unsigned long *scan_mask)
+{
+ struct ads1298_private *priv = iio_priv(indio_dev);
+ unsigned int val;
+ int ret;
+ int i;
+
+ /* Make the interrupt routines start with a clean slate */
+ ads1298_rdata_unmark_busy(priv);
+
+ /* Power down channels that aren't in use */
+ for (i = 0; i < ADS1298_MAX_CHANNELS; i++) {
+ val = test_bit(i, scan_mask) ? 0 : ADS1298_MASK_CH_PD;
+ ret = regmap_update_bits(priv->regmap, ADS1298_REG_CHnSET(i),
+ ADS1298_MASK_CH_PD, val);
+ if (ret)
+ return ret;
+ }
+
+ 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_release_busy_or_restart(struct ads1298_private *priv)
+{
+ unsigned long flags;
+ int wasbusy;
+
+ spin_lock_irqsave(&priv->irq_busy_lock, flags);
+
+ wasbusy = --priv->rdata_xfer_busy;
+ if (wasbusy) {
+ /*
+ * DRDY interrupt occurred before SPI completion. Start a new
+ * SPI transaction now to retrieve the data that wasn't latched
+ * into the ADS1298 chip's transfer buffer yet.
+ */
+ spi_async(priv->spi, &priv->rdata_msg);
+ /*
+ * If more than one DRDY took place, there was an overrun. Since
+ * the sample is already lost, reset the counter to 1 so that
+ * we will wait for a DRDY interrupt after this SPI transaction.
+ */
+ if (wasbusy > 1)
+ priv->rdata_xfer_busy = 1;
+ }
+
+ spin_unlock_irqrestore(&priv->irq_busy_lock, flags);
+}
+
+/* Called from SPI completion interrupt handler */
+static void ads1298_rdata_complete(void *context)
+{
+ struct iio_dev *indio_dev = context;
+ struct ads1298_private *priv = iio_priv(indio_dev);
+ int scan_index;
+ u32 *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;
+
+ *bounce++ = get_unaligned_be24(data);
+ }
+
+ /* rx_buffer can be overwritten from this point on */
+ ads1298_rdata_release_busy_or_restart(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;
+ int wasbusy;
+
+ spin_lock_irqsave(&priv->irq_busy_lock, flags);
+
+ wasbusy = priv->rdata_xfer_busy++;
+ /* When no SPI transfer in transit, start one now */
+ if (!wasbusy)
+ spi_async(priv->spi, &priv->rdata_msg);
+
+ spin_unlock_irqrestore(&priv->irq_busy_lock, flags);
+
+ 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 & ADS1298_MASK_ID_FAMILY) {
+ case ADS1298_ID_FAMILY_ADS129X:
+ return "ADS129x";
+ case ADS1298_ID_FAMILY_ADS129XR:
+ 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;
+
+ dev_info(dev, "Found %s, %u channels\n", ads1298_family_name(val),
+ 4 + 2 * (val & ADS1298_MASK_ID_CHANNELS));
+
+ /* Enable internal test signal, double amplitude, double frequency */
+ ret = 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);
+ if (ret)
+ return ret;
+
+ 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;
+ }
+ return regmap_write(priv->regmap, ADS1298_REG_CONFIG3, val);
+}
+
+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)
+ 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");
+
+ /* 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");
+
+ 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");
+
+ 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;
+
+ indio_dev->name = spi_get_device_id(spi)->name;
+ 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(ADS1298_CLOCKS_TO_USECS(2));
+ 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(ADS1298_CLOCKS_TO_USECS(18));
+
+ ret = ads1298_init(priv);
+ if (ret)
+ return dev_err_probe(dev, ret, "Init failed\n");
+
+ ret = devm_request_irq(dev, spi->irq, &ads1298_interrupt,
+ IRQF_TRIGGER_FALLING, indio_dev->name,
+ indio_dev);
+ if (ret)
+ return ret;
+
+ ret = devm_iio_kfifo_buffer_setup(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
On Fri, 2 Feb 2024 11:59:01 +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,
A few minor things I'd missed before.
I'm still interested in why more standard interrupt handling isn't
good enough here (see reply to v1 thread) but if we can't get to the bottom
of that (or do figure it out and we can't fix it) then this doesn't look
too bad so I'll accept the complex handling.
J
> diff --git a/drivers/iio/adc/ti-ads1298.c b/drivers/iio/adc/ti-ads1298.c
> new file mode 100644
> index 000000000000..539598b9f3fa
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads1298.c
> @@ -0,0 +1,769 @@
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/kfifo_buf.h>
> +#include <linux/iio/sysfs.h>
I don't see any custom ABI, so shouldn't need this.
> +
> +#include <asm/unaligned.h>
> +
> +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,
> + .unit = SPI_DELAY_UNIT_USECS,
> + },
> + };
> + int ret;
> +
> + priv->cmd_buffer[0] = ADS1298_CMD_RREG | reg;
> + priv->cmd_buffer[1] = 0x0;
Why mix of hex and decimal? Doesn't matter but looks odd
> + priv->cmd_buffer[2] = 0;
> +
> + ret = spi_sync_transfer(priv->spi, ®_read_xfer, 1);
> + if (ret)
> + return ret;
> +
> + *val = priv->cmd_buffer[2];
> +
> + return 0;
> +}
> +
> +/* Called from SPI completion interrupt handler */
> +static void ads1298_rdata_complete(void *context)
> +{
> + struct iio_dev *indio_dev = context;
> + struct ads1298_private *priv = iio_priv(indio_dev);
> + int scan_index;
> + u32 *bounce = priv->bounce_buffer;
> +
> + if (!iio_buffer_enabled(indio_dev)) {
Good to add a comment here on why this can't race as we are holding
the device in direct mode until after the completion.
iio_buffer_enabled() checks tend to expose races so I prefer people
to explicitly say why there isn't one.
> + /* 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;
> +
> + *bounce++ = get_unaligned_be24(data);
> + }
> +
> + /* rx_buffer can be overwritten from this point on */
> + ads1298_rdata_release_busy_or_restart(priv);
> +
> + iio_push_to_buffers(indio_dev, priv->bounce_buffer);
> +}
> +
> +static const char *ads1298_family_name(unsigned int id)
> +{
> + switch (id & ADS1298_MASK_ID_FAMILY) {
> + case ADS1298_ID_FAMILY_ADS129X:
> + return "ADS129x";
> + case ADS1298_ID_FAMILY_ADS129XR:
> + 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;
> +
> + dev_info(dev, "Found %s, %u channels\n", ads1298_family_name(val),
> + 4 + 2 * (val & ADS1298_MASK_ID_CHANNELS));
Noise for the log that is easy to figure out anyway (assuming successful
probe) Make that dev_dbg()
> +
> + /* Enable internal test signal, double amplitude, double frequency */
> + ret = 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);
Unless lines are very long, parameters should align just after (
> + if (ret)
> + return ret;
> +
> + 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;
> + }
> + return regmap_write(priv->regmap, ADS1298_REG_CONFIG3, val);
> +}
> +
> +static int ads1298_probe(struct spi_device *spi)
> +{
..
> + ret = devm_request_irq(dev, spi->irq, &ads1298_interrupt,
> + IRQF_TRIGGER_FALLING, indio_dev->name,
I missed this before (and we've gotten it wrong a bunch of times in the past
so plenty of bad examples to copy that we can't fix without possible
regressions) but we generally now leave irq direction to the firmware description.
People have an annoying habit of putting not gates and similar in the path
to interrupt pins. Fine to have the binding state the expected form though
(as you do). So basically not flags here.
I'm still curious to understand more of where the delays that lead to
needing to do this complex handling came from, but I guess it's not too bad
if we can't get to the bottom of that so I'll take the driver anyway
(after a little more time on list for others to review!)
> + indio_dev);
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
On 04-02-2024 16:54, Jonathan Cameron wrote:
> On Fri, 2 Feb 2024 11:59:01 +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,
>
> A few minor things I'd missed before.
>
> I'm still interested in why more standard interrupt handling isn't
> good enough here (see reply to v1 thread) but if we can't get to the bottom
> of that (or do figure it out and we can't fix it) then this doesn't look
> too bad so I'll accept the complex handling.
I think one of the key elements was the IRQF_ONESHOT usage. The DRDY
signal on this chip isn't a "level" signal as most chips have, it will
de-assert at any rising edge of the SPI clock, without regarding
chip-select. There's no other indication of "data ready", so the only
way is to keep the interrupt active on edge detect.
Keeping things in hard IRQ handlers reduces the number of context
switches, and the amount of work done is minimal. A worker thread would
wake at every DRDY signal, and after the corresponding SPI transaction.
This doesn't account for much overhead, but the interrupt rate is double
the sampling frequency. Most importantly, the device doesn't have to
compete with other threads in the system.
If I have time and hardware available, I try to get some timing info
with an oscilloscope...
Assume "yes" to all other suggestions...
>> + ret = devm_request_irq(dev, spi->irq, &ads1298_interrupt,
>> + IRQF_TRIGGER_FALLING, indio_dev->name,
> I missed this before (and we've gotten it wrong a bunch of times in the past
> so plenty of bad examples to copy that we can't fix without possible
> regressions) but we generally now leave irq direction to the firmware description.
> People have an annoying habit of putting not gates and similar in the path
> to interrupt pins. Fine to have the binding state the expected form though
> (as you do). So basically not flags here.
I'd be happy to leave the IRQ direction to firmware (indeed, inverters
happen...), but afaik that wasn't possible with interrupts. I'll dig
into the docs to see if that has changed recently.
> I'm still curious to understand more of where the delays that lead to
> needing to do this complex handling came from, but I guess it's not too bad
> if we can't get to the bottom of that so I'll take the driver anyway
> (after a little more time on list for others to review!)
>
>> + indio_dev);
(PS - sorry for sending HTML, consider me appropriately punisched for that)
--
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
On Fri, Feb 02, 2024 at 04:28:07PM +0200, Andy Shevchenko wrote:
> On Fri, Feb 02, 2024 at 11:59:01AM +0100, Mike Looijmans wrote:
Hit "Send" by a mistake, here is the full review.
..
Why this can't be the part of drivers/iio/adc/ti-ads124s08.c?
Seems to me the command list is the same, registers are different though.
Broadly the Q is have you checked other existing drivers if they can
be used as a base. If not, perhaps a word in the cover letter is good to have
(sorry if I asked this already).
..
> > +#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
>
> Leftovers.
..
> > +#define ADS1298_CLOCKS_TO_USECS(x) \
> > + (DIV_ROUND_UP((x * MICROHZ_PER_HZ), ADS1298_CLK_RATE_HZ))
>
> Wrong place of parentheses, should be "(x) * ...".
> Don't you need to include math.h?
..
> > +#define ADS1298_MASK_CONFIG1_DR 0x07
GENMASK() ?
..
> > +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;
Do you use this outside of the ->probe()? I just ask since I removed some
context already...
> > + spinlock_t irq_busy_lock; /* Handshake between SPI and DRDY irqs */
> > + int rdata_xfer_busy;
> > +
> > + /* Temporary storage for demuxing data after SPI transfer */
> > + u32 bounce_buffer[ADS1298_MAX_CHANNELS];
> > +
> > + /* For synchronous SPI exchanges (read/write registers) */
> > + u8 cmd_buffer[ADS1298_SPI_CMD_BUFFER_SIZE] __aligned(IIO_DMA_MINALIGN);
> > +
> > + /* Buffer used for incoming SPI data */
> > + u8 rx_buffer[ADS1298_SPI_RDATA_BUFFER_SIZE];
Cacheline aligned?
I see the cmd_buffer, but shouldn't this be also aligned?
> > + /* Contains the RDATA command and zeroes to clock out */
> > + u8 tx_buffer[ADS1298_SPI_RDATA_BUFFER_SIZE];
> > +};
..
> > +static int ads1298_write_cmd(struct ads1298_private *priv, u8 command)
> > +{
> > + struct spi_transfer cmd_xfer = {
I would use xfer[]...
> > + .tx_buf = priv->cmd_buffer,
> > + .rx_buf = priv->cmd_buffer,
> > + .len = sizeof(command),
In other cases you use plain number, perhaps 1 is okay here for consistency's
sake?
> > + .speed_hz = ADS1298_SPI_BUS_SPEED_SLOW,
> > + .delay = {
> > + .value = 2,
> > + .unit = SPI_DELAY_UNIT_USECS,
> > + },
> > + };
> > +
> > + priv->cmd_buffer[0] = command;
> > +
> > + return spi_sync_transfer(priv->spi, &cmd_xfer, 1);
..and ARRAY_SIZE(), but either way I'm fine, i.o.w. this is also okay.
> > +}
..
> > + /* Enable the channel */
Too many spaces before */.
..
> > + /* Cannot take longer than 40ms (250Hz) */
> > + ret = wait_for_completion_timeout(&priv->completion, msecs_to_jiffies(50));
> > + if (!ret)
> > + return -ETIMEDOUT;
> > +
> > + return 0;
Can be other way around
if (ret)
return 0;
return -ETIMEDOUT;
But the original is also okay.
> > +}
..
> > + if (priv->clk)
> > + rate = clk_get_rate(priv->clk);
> > + else
> > + rate = ADS1298_CLK_RATE_HZ;
Dead code (here and elsewhere). You probably wanted _optional clk APIs
in the probe.
> > + if (!rate)
> > + return -EINVAL;
..
> > + factor = (rate >> ADS1298_SHIFT_DR_HR) / val;
> > + if (factor >= 128) {
> > + cfg = ADS1298_LOWEST_DR;
> > + } else if (factor <= 1) {
> > + cfg = ADS1298_MASK_CONFIG1_HR; /* Fastest possible */
> > + } else {
> > + cfg = fls(factor) - 1;
> > + cfg |= ADS1298_MASK_CONFIG1_HR; /* Use HR mode */
> > + }
What about:
factor = (rate >> ADS1298_SHIFT_DR_HR) / val;
if (factor >= 128)
cfg = ADS1298_LOWEST_DR;
else if (factor)
cfg = ADS1298_MASK_CONFIG1_HR | ilog2(factor); /* Use HR mode */
else
cfg = ADS1298_MASK_CONFIG1_HR; /* Fastest possible */
(will need log2.h to be included)
..
> > + *val = ret / 1000; /* Convert to millivolts */
MILLI ?
..
> > + *val = sign_extend32(get_unaligned_be24(priv->rx_buffer + chan->address),
> > + 23);
Second time magic 23, can you define a constant and use it everywhere?
..
> > +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,
> > + .unit = SPI_DELAY_UNIT_USECS,
> > + },
> > + };
> > +
> > + priv->cmd_buffer[0] = ADS1298_CMD_WREG | reg;
> > + priv->cmd_buffer[1] = 0x0;
> > + priv->cmd_buffer[2] = val;
Sounds to me like put_unaligned_be16().
> > +
> > + return spi_sync_transfer(priv->spi, ®_write_xfer, 1);
> > +}
..
> > + priv->cmd_buffer[0] = ADS1298_CMD_RREG | reg;
> > + priv->cmd_buffer[1] = 0x0;
> > + priv->cmd_buffer[2] = 0;
Ditto.
> > + ret = spi_sync_transfer(priv->spi, ®_read_xfer, 1);
> > + if (ret)
> > + return ret;
..
> > + *val = priv->cmd_buffer[2];
Just wondering if the above is correct assumption, this probably needs to be
get_unaligned_be16().
..
> > + 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 = 0;
> > + spin_unlock_irqrestore(&priv->irq_busy_lock, flags);
Use cleanup.h?
..
> > +static int ads1298_update_scan_mode(struct iio_dev *indio_dev,
> > + const unsigned long *scan_mask)
> > +{
> > + struct ads1298_private *priv = iio_priv(indio_dev);
> > + unsigned int val;
> > + int ret;
> > + int i;
> > +
> > + /* Make the interrupt routines start with a clean slate */
> > + ads1298_rdata_unmark_busy(priv);
> > +
> > + /* Power down channels that aren't in use */
This comment does not describe why you need to write to _all_ channels.
> > + for (i = 0; i < ADS1298_MAX_CHANNELS; i++) {
> > + val = test_bit(i, scan_mask) ? 0 : ADS1298_MASK_CH_PD;
With above in mind, this perhaps needs to be one of for_each_set_bit(scan_mask) /
for_each_clear_bit(scan_mask).
> > + ret = regmap_update_bits(priv->regmap, ADS1298_REG_CHnSET(i),
> > + ADS1298_MASK_CH_PD, val);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
..
> > + unsigned long flags;
cleanup.h
> > + int wasbusy;
> > +
> > + spin_lock_irqsave(&priv->irq_busy_lock, flags);
> > +
> > + wasbusy = --priv->rdata_xfer_busy;
> > + if (wasbusy) {
> > + /*
> > + * DRDY interrupt occurred before SPI completion. Start a new
> > + * SPI transaction now to retrieve the data that wasn't latched
> > + * into the ADS1298 chip's transfer buffer yet.
> > + */
> > + spi_async(priv->spi, &priv->rdata_msg);
> > + /*
> > + * If more than one DRDY took place, there was an overrun. Since
> > + * the sample is already lost, reset the counter to 1 so that
> > + * we will wait for a DRDY interrupt after this SPI transaction.
> > + */
> > + if (wasbusy > 1)
> > + priv->rdata_xfer_busy = 1;
> > + }
> > +
> > + spin_unlock_irqrestore(&priv->irq_busy_lock, flags);
..
> > +static irqreturn_t ads1298_interrupt(int irq, void *dev_id)
Ditto.
..
> > + .cache_type = REGCACHE_RBTREE,
Why not MAPPLE TREE?
..
> > +static const char *ads1298_family_name(unsigned int id)
> > +{
> > + switch (id & ADS1298_MASK_ID_FAMILY) {
> > + case ADS1298_ID_FAMILY_ADS129X:
> > + return "ADS129x";
> > + case ADS1298_ID_FAMILY_ADS129XR:
> > + return "ADS129xR";
> > + default:
> > + return "(unknown)";
Hmm... Maybe "" is enough? What is the practice in IIO for this? Jonathan?
> > + }
> > +}
..
> > + dev_info(dev, "Found %s, %u channels\n", ads1298_family_name(val),
> > + 4 + 2 * (val & ADS1298_MASK_ID_CHANNELS));
How is this useful? Can't sysfs already give an answer to these Q:s?
..
> > + if (IS_ERR(priv->reg_vref)) {
> > + if (PTR_ERR(priv->reg_vref) == -ENODEV)
> > + priv->reg_vref = NULL;
> > + else
Redundant if you check for an error cases first.
> > + 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");
_optional? See above.
> > + if (IS_ERR(priv->clk))
> > + return dev_err_probe(dev, PTR_ERR(priv->clk),
> > + "Failed to get clk\n");
One line? (Even in strict, i.e. 80 limit, mode checkpatch won't complain on it)
..
> > + priv->regmap = devm_regmap_init(dev, NULL, priv,
> > + &ads1298_regmap_config);
One line? (It's 81, which we may very well tolerate)
> > + if (IS_ERR(priv->regmap))
> > + return PTR_ERR(priv->regmap);
..
> > + if (reset_gpio) {
> > + udelay(ADS1298_CLOCKS_TO_USECS(2));
Why this delay (the after one is explained, though)?
> > + 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(ADS1298_CLOCKS_TO_USECS(18));
--
With Best Regards,
Andy Shevchenko
Assume "yes" to all suggestions not mentioned below.
I'll send out a v3 later.
On 05-02-2024 14:55, Andy Shevchenko wrote:
> On Fri, Feb 02, 2024 at 04:28:07PM +0200, Andy Shevchenko wrote:
>> On Fri, Feb 02, 2024 at 11:59:01AM +0100, Mike Looijmans wrote:
> Hit "Send" by a mistake, here is the full review.
>
> ...
>
> Why this can't be the part of drivers/iio/adc/ti-ads124s08.c?
> Seems to me the command list is the same, registers are different though.
> Broadly the Q is have you checked other existing drivers if they can
> be used as a base. If not, perhaps a word in the cover letter is good to have
> (sorry if I asked this already).
>
> ...
The ads124s08 command list is the same, but that's about all that's similar.
>>> + struct spi_transfer rdata_xfer;
>>> + struct spi_message rdata_msg;
> Do you use this outside of the ->probe()? I just ask since I removed some
> context already...
Yes, probe() initializes the structs, they're used in the interrupt
routines.
>
>>> + spinlock_t irq_busy_lock; /* Handshake between SPI and DRDY irqs */
>>> + int rdata_xfer_busy;
>>> +
>>> + /* Temporary storage for demuxing data after SPI transfer */
>>> + u32 bounce_buffer[ADS1298_MAX_CHANNELS];
>>> +
>>> + /* For synchronous SPI exchanges (read/write registers) */
>>> + u8 cmd_buffer[ADS1298_SPI_CMD_BUFFER_SIZE] __aligned(IIO_DMA_MINALIGN);
>>> +
>>> + /* Buffer used for incoming SPI data */
>>> + u8 rx_buffer[ADS1298_SPI_RDATA_BUFFER_SIZE];
> Cacheline aligned?
> I see the cmd_buffer, but shouldn't this be also aligned?
I understood from Jonathan that that wasn't needed... "My" SPI
controller is rather dumb and doesn't even have DMA.
Will take a closer look though.
>>> + else
>>> + rate = ADS1298_CLK_RATE_HZ;
> Dead code (here and elsewhere). You probably wanted _optional clk APIs
> in the probe.
Yes "optional" was intended.
> ...
>
>>> +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,
>>> + .unit = SPI_DELAY_UNIT_USECS,
>>> + },
>>> + };
>>> +
>>> + priv->cmd_buffer[0] = ADS1298_CMD_WREG | reg;
>>> + priv->cmd_buffer[1] = 0x0;
>>> + priv->cmd_buffer[2] = val;
> Sounds to me like put_unaligned_be16().
Added comment to explain the first zero is the "number of registers to
read/write minus one".
> ...
>
>>> + 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 = 0;
>>> + spin_unlock_irqrestore(&priv->irq_busy_lock, flags);
> Use cleanup.h?
>
> ...
Will dive into it, is new to me... Looks like C++ "scoped_xxx" at a
first glance.
>
>>> +static int ads1298_update_scan_mode(struct iio_dev *indio_dev,
>>> + const unsigned long *scan_mask)
>>> +{
>>> + struct ads1298_private *priv = iio_priv(indio_dev);
>>> + unsigned int val;
>>> + int ret;
>>> + int i;
>>> +
>>> + /* Make the interrupt routines start with a clean slate */
>>> + ads1298_rdata_unmark_busy(priv);
>>> +
>>> + /* Power down channels that aren't in use */
> This comment does not describe why you need to write to _all_ channels.
Yeah, need to configure them all. Most common is that the cached value
is already okay and no actual write register will happen.
>
>>> + for (i = 0; i < ADS1298_MAX_CHANNELS; i++) {
>>> + val = test_bit(i, scan_mask) ? 0 : ADS1298_MASK_CH_PD;
> With above in mind, this perhaps needs to be one of for_each_set_bit(scan_mask) /
> for_each_clear_bit(scan_mask).
Probably more efficient to access the device registers in a natural order.
> ...
>
>>> + .cache_type = REGCACHE_RBTREE,
> Why not MAPPLE TREE?
Reading the description this driver isn't a good candidate - the map
isn't sparse and the hardware can do bulk read/write (though the driver
doesn't use it).
..
--
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
On Mon, Feb 05, 2024 at 04:25:19PM +0100, Mike Looijmans wrote:
> On 05-02-2024 14:55, Andy Shevchenko wrote:
> > > > + .cache_type = REGCACHE_RBTREE,
> > Why not MAPPLE TREE?
> Reading the description this driver isn't a good candidate - the map isn't
> sparse and the hardware can do bulk read/write (though the driver doesn't
> use it).
If your driver is a good candidate for rbtree it's a good candidate for
maple tree. There are very few specialist cases where there's an
advantage to sticking with rbtree. The maple cache has no meaningful
overhead compared to rbtree for non-sparse regmaps and will generate
bulk writes just fine.
>
> >
> >>> + spinlock_t irq_busy_lock; /* Handshake between SPI and DRDY irqs */
> >>> + int rdata_xfer_busy;
> >>> +
> >>> + /* Temporary storage for demuxing data after SPI transfer */
> >>> + u32 bounce_buffer[ADS1298_MAX_CHANNELS];
> >>> +
> >>> + /* For synchronous SPI exchanges (read/write registers) */
> >>> + u8 cmd_buffer[ADS1298_SPI_CMD_BUFFER_SIZE] __aligned(IIO_DMA_MINALIGN);
> >>> +
> >>> + /* Buffer used for incoming SPI data */
> >>> + u8 rx_buffer[ADS1298_SPI_RDATA_BUFFER_SIZE];
> > Cacheline aligned?
> > I see the cmd_buffer, but shouldn't this be also aligned?
>
> I understood from Jonathan that that wasn't needed... "My" SPI
> controller is rather dumb and doesn't even have DMA.
>
> Will take a closer look though.
It should be fine. The aim here is to ensure that nothing access data in
the same cacheline from the CPU side whilst DMA is ongoing (and unwanted
write backs of stale data can occur). As long at the handling isn't
very complex, a single marking of the first buffer used for DMA (and
being sure there is nothing other than other DMA buffers after it
the same cacheline (potentially multiple lines) is enough.
So the __aligend(IIO_DMA_MINALIGN) on cmd_buffer should work for allt hese
buffers.