[PATCH v2 0/6] iio: adc: ti-ads1298: fixes, and ADS1299 support

[PATCH v2 0/6] iio: adc: ti-ads1298: fixes, and ADS1299 support

[Stephan Costa Barros]

This series adds the TI ADS1299 to the ti-ads1298 driver. On the way it
fixes twelve issues in ti-ads1298 itself. Five of them come from the
automated review of v1, which was a separate driver derived from
ti-ads1298; the others came up while reworking and testing.

Changes in v2:
- Extend ti-ads1298 instead of adding a new driver (Andy, Jonathan).
- Bindings: the ADS1299 goes into ti,ads1298.yaml, with no clock-names
  (Krzysztof). ti,ads1299 names the 8-channel part, as TI sells it as
  plain ADS1299 with no "-8"; ti,ads1299-4 and ti,ads1299-6 fall back to
  it, since the channel count is read from the ID register. Each ADS129x
  part also gets a compatible, with ti,ads1298 as the fallback.
- The issues the automated review found in v1 were inherited from
  ti-ads1298, so they are now fixed there, in patches 1 and 2.
- No <linux/mod_devicetable.h> is added (Uwe).
- The ABI that v1 added beyond ti-ads1298 (writable scale, _available
  lists, timestamp) is left for a follow-up; see the end.
- v1: https://lore.kernel.org/all/20261019001247.483005-1-stephanbrrs8@gmail.com/

The series:
- Patches 1 to 3 fix ti-ads1298 as it is in mainline today, grouped by
  area: the data read path, power-up and the device clock, and smaller
  issues (scale, ID check, regmap, Kconfig). Each has a Fixes: tag and
  none depends on the rest of the series, so they can be applied ahead
  of it.
- Patch 4 prepares the driver for more than one chip family, with a
  chip info structure, given as match data, that holds what differs
  between families. The ADS129x behaves as before.
- Patches 5 and 6 add the ADS1299 to the binding and to the driver,
  with a compatible for each ADS129x part and the optional DVDD supply
  and PWDN pin.

The ADS1299 shares the commands, the data format and most of the
register map of the ADS129x. It differs in:
- the PGA gains, 1 to 24;
- the data rates: no HR bit, and bit 7 of CONFIG1 is reserved;
- a fixed 4.5 V internal reference, and the reserved bits of CONFIG3;
- the ID layout (bit 3 alone tells the two families apart);
- the register map, which ends at CONFIG4;
- inputs that are shorted out of reset.
These are fields of the chip info and an ID decoding callback; the code
paths are the same for both families.

Testing:
- On real hardware: an ADS1299 with a Raspberry Pi 4, running the
  driver from the tip of the series. Probe, scale, data rates,
  single-shot reads, buffered capture, and buffer start and stop under
  stress all behave as expected. The clock fix in patch 2 was checked on
  the same board.
- With a KUnit test, kept out of this series, that runs the driver
  against an emulated ADS129x and ADS1299. It fails when most of the
  fixes are undone, and it found the command timing issue fixed in
  patch 2. I can post it if it is of interest.
- Three of the fixes in patch 1 were found by review and could not be
  reproduced; the commit message says which.
- Not tested: an ADS1298 or other ADS129x part. Mike, if you still have
  an ADS1298 board, a test of the series would be welcome.

Proposal for a follow-up: once this series is in, I would like to add
generic IIO ABI that serves both families in the same way: the PGA gain
of each channel through a writable scale, with
in_voltage_scale_available, sampling_frequency_available, and a
timestamp channel. Would that be acceptable as a follow-up series on
top of this one?

Stephan Costa Barros (6):
  iio: adc: ti-ads1298: fix the data read path
  iio: adc: ti-ads1298: fix power-up and clock handling
  iio: adc: ti-ads1298: fix scale, ID check, regmap and Kconfig
  iio: adc: ti-ads1298: prepare for more chip families
  dt-bindings: iio: adc: ti,ads1298: add ADS1299
  iio: adc: ti-ads1298: add ADS1299 support

 .../bindings/iio/adc/ti,ads1298.yaml          |  41 +-
 drivers/iio/adc/Kconfig                       |   7 +-
 drivers/iio/adc/ti-ads1298.c                  | 513 ++++++++++++++----
 3 files changed, 448 insertions(+), 113 deletions(-)


base-commit: c9dbe2a47f635465c9985b7af74d2ae310e98125
-- 
2.43.0

[PATCH v2 1/6] iio: adc: ti-ads1298: fix the data read path

[Stephan Costa Barros]

Fix five issues in how the driver reads samples:

- cmd_buffer serves both the register accessors, under the regmap lock,
  and the commands, under the IIO device lock, and shares a cacheline
  with rx_buffer and tx_buffer. A register access can then rewrite a
  command on the bus, or corrupt an RDATA transfer in flight on a
  platform without coherent DMA. Give each path its own buffer, each on
  its own cacheline.

- After STOP, the conversion in progress still raises DRDY, so an RDATA
  transfer can be in flight when the core frees the active scan mask.
  Track in the driver whether samples go to the buffer, wait in
  predisable for the last DRDY and its transfer, and push the sample
  before marking the transfer done. update_scan_mode reset the transfer
  counter, which let a new transfer be queued over one in flight; wait
  for it instead.

- When a single-shot read times out, its transfer may still be in
  flight, and the next read then wakes up with the old sample. On
  unbind, nothing waits for the DRDY interrupt or its transfer. Wait for
  the transfer before a single-shot read, and on teardown disable the
  interrupt and wait for the last transfer.

- The return value of spi_async() and the status of the RDATA message
  are ignored: a failure leaves the transfer counter stuck, and a failed
  transfer is reported as a sample. Handle both.

- read_raw takes the sample out of rx_buffer after releasing direct
  mode, so a buffer enabled in between can overwrite it. Take it while
  direct mode is still held.

The first, second and last were found by review and not reproduced.
The third and fourth were found by review and reproduced on an emulated
device only.

Fixes: 00ef7708fa60 ("iio: adc: ti-ads1298: Add driver")
Signed-off-by: Stephan Costa Barros <stephanbrrs8@gmail.com>
---
 drivers/iio/adc/ti-ads1298.c | 183 +++++++++++++++++++++++++++--------
 1 file changed, 144 insertions(+), 39 deletions(-)

diff --git a/drivers/iio/adc/ti-ads1298.c b/drivers/iio/adc/ti-ads1298.c
index 06c6353aa..1d32b1f47 100644
--- a/drivers/iio/adc/ti-ads1298.c
+++ b/drivers/iio/adc/ti-ads1298.c
@@ -10,6 +10,7 @@
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/gpio/consumer.h>
+#include <linux/interrupt.h>
 #include <linux/log2.h>
 #include <linux/math.h>
 #include <linux/module.h>
@@ -18,6 +19,7 @@
 #include <linux/slab.h>
 #include <linux/spi/spi.h>
 #include <linux/units.h>
+#include <linux/wait.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/buffer.h>
@@ -119,15 +121,26 @@ struct ads1298_private {
 	 * >2 = Multiple DRDY during transfer, lost rdata_xfer_busy - 2 samples
 	 */
 	unsigned int rdata_xfer_busy;
+	/* Samples go to the buffer; under irq_busy_lock */
+	bool buffered;
+	/* Woken when rdata_xfer_busy drops to 0 */
+	wait_queue_head_t idle_wq;
+	/* Status of the last single-shot read */
+	int rdata_status;
 
 	/* 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);
-
+	/*
+	 * Each buffer below is used by one path at a time and has its own
+	 * cacheline, so that no two transfers in flight share one.
+	 */
+	/* Register access, under the regmap lock */
+	u8 reg_buffer[ADS1298_SPI_CMD_BUFFER_SIZE] __aligned(IIO_DMA_MINALIGN);
+	/* Commands, under the IIO device lock or in probe */
+	u8 cmd_buffer[1] __aligned(IIO_DMA_MINALIGN);
 	/* Buffer used for incoming SPI data */
-	u8 rx_buffer[ADS1298_SPI_RDATA_BUFFER_SIZE_MAX];
+	u8 rx_buffer[ADS1298_SPI_RDATA_BUFFER_SIZE_MAX] __aligned(IIO_DMA_MINALIGN);
 	/* Contains the RDATA command and zeroes to clock out */
 	u8 tx_buffer[ADS1298_SPI_RDATA_BUFFER_SIZE_MAX];
 };
@@ -185,10 +198,49 @@ static int ads1298_write_cmd(struct ads1298_private *priv, u8 command)
 	return spi_sync_transfer(priv->spi, &xfer, 1);
 }
 
+/* Whether no RDATA transfer is queued or in flight */
+static bool ads1298_rdata_idle(struct ads1298_private *priv)
+{
+	guard(spinlock_irqsave)(&priv->irq_busy_lock);
+	return !priv->rdata_xfer_busy;
+}
+
+static int ads1298_wait_rdata_idle(struct ads1298_private *priv)
+{
+	if (!wait_event_timeout(priv->idle_wq, ads1298_rdata_idle(priv),
+				msecs_to_jiffies(100)))
+		return -ETIMEDOUT;
+
+	return 0;
+}
+
+/* Called with irq_busy_lock held */
+static void ads1298_rdata_start(struct ads1298_private *priv)
+{
+	int ret;
+
+	ret = spi_async(priv->spi, &priv->rdata_msg);
+	if (!ret)
+		return;
+
+	/* No completion will run for this transfer */
+	priv->rdata_xfer_busy = 0;
+	if (!priv->buffered) {
+		priv->rdata_status = ret;
+		complete(&priv->completion);
+	}
+	wake_up(&priv->idle_wq);
+}
+
 static int ads1298_read_one(struct ads1298_private *priv, int chan_index)
 {
 	int ret;
 
+	/* A transfer left over from an earlier timeout must finish first */
+	ret = ads1298_wait_rdata_idle(priv);
+	if (ret)
+		return ret;
+
 	/* Enable the channel */
 	ret = regmap_update_bits(priv->regmap, ADS1298_REG_CHnSET(chan_index),
 				 ADS1298_MASK_CH_PD, 0);
@@ -217,7 +269,7 @@ static int ads1298_read_one(struct ads1298_private *priv, int chan_index)
 	if (!wait_for_completion_timeout(&priv->completion, msecs_to_jiffies(50)))
 		return -ETIMEDOUT;
 
-	return 0;
+	return priv->rdata_status;
 }
 
 static int ads1298_get_samp_freq(struct ads1298_private *priv, int *val)
@@ -330,14 +382,17 @@ static int ads1298_read_raw(struct iio_dev *indio_dev,
 			return -EBUSY;
 
 		ret = ads1298_read_one(priv, chan->scan_index);
+		/* Once direct mode is released, a buffer may overwrite rx_buffer */
+		if (!ret)
+			*val = sign_extend32(get_unaligned_be24(priv->rx_buffer +
+								chan->address),
+					     ADS1298_BITS_PER_SAMPLE - 1);
 
 		iio_device_release_direct(indio_dev);
 
 		if (ret)
 			return ret;
 
-		*val = sign_extend32(get_unaligned_be24(priv->rx_buffer + chan->address),
-				     ADS1298_BITS_PER_SAMPLE - 1);
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
 		return ads1298_get_scale(priv, chan->channel, val, val2);
@@ -373,8 +428,8 @@ 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,
+		.tx_buf = priv->reg_buffer,
+		.rx_buf = priv->reg_buffer,
 		.len = 3,
 		.speed_hz = ADS1298_SPI_BUS_SPEED_SLOW,
 		.delay = {
@@ -383,9 +438,9 @@ static int ads1298_reg_write(void *context, unsigned int reg, unsigned int val)
 		},
 	};
 
-	priv->cmd_buffer[0] = ADS1298_CMD_WREG | reg;
-	priv->cmd_buffer[1] = 0; /* Number of registers to be written - 1 */
-	priv->cmd_buffer[2] = val;
+	priv->reg_buffer[0] = ADS1298_CMD_WREG | reg;
+	priv->reg_buffer[1] = 0; /* Number of registers to be written - 1 */
+	priv->reg_buffer[2] = val;
 
 	return spi_sync_transfer(priv->spi, &reg_write_xfer, 1);
 }
@@ -394,8 +449,8 @@ 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,
+		.tx_buf = priv->reg_buffer,
+		.rx_buf = priv->reg_buffer,
 		.len = 3,
 		.speed_hz = ADS1298_SPI_BUS_SPEED_SLOW,
 		.delay = {
@@ -405,15 +460,15 @@ static int ads1298_reg_read(void *context, unsigned int reg, unsigned int *val)
 	};
 	int ret;
 
-	priv->cmd_buffer[0] = ADS1298_CMD_RREG | reg;
-	priv->cmd_buffer[1] = 0; /* Number of registers to be read - 1 */
-	priv->cmd_buffer[2] = 0;
+	priv->reg_buffer[0] = ADS1298_CMD_RREG | reg;
+	priv->reg_buffer[1] = 0; /* Number of registers to be read - 1 */
+	priv->reg_buffer[2] = 0;
 
 	ret = spi_sync_transfer(priv->spi, &reg_read_xfer, 1);
 	if (ret)
 		return ret;
 
-	*val = priv->cmd_buffer[2];
+	*val = priv->reg_buffer[2];
 
 	return 0;
 }
@@ -429,13 +484,6 @@ static int ads1298_reg_access(struct iio_dev *indio_dev, unsigned int reg,
 	return regmap_write(priv->regmap, reg, writeval);
 }
 
-static void ads1298_rdata_unmark_busy(struct ads1298_private *priv)
-{
-	/* Notify we're no longer waiting for the SPI transfer to complete */
-	guard(spinlock_irqsave)(&priv->irq_busy_lock);
-	priv->rdata_xfer_busy = 0;
-}
-
 static int ads1298_update_scan_mode(struct iio_dev *indio_dev,
 				    const unsigned long *scan_mask)
 {
@@ -444,8 +492,10 @@ static int ads1298_update_scan_mode(struct iio_dev *indio_dev,
 	int ret;
 	int i;
 
-	/* Make the interrupt routines start with a clean slate */
-	ads1298_rdata_unmark_busy(priv);
+	/* Nothing may be in flight when the buffer starts */
+	ret = ads1298_wait_rdata_idle(priv);
+	if (ret)
+		return ret;
 
 	/* Configure power-down bits to match scan mask */
 	for (i = 0; i < indio_dev->num_channels; i++) {
@@ -470,22 +520,22 @@ static void ads1298_rdata_release_busy_or_restart(struct ads1298_private *priv)
 {
 	guard(spinlock_irqsave)(&priv->irq_busy_lock);
 
-	if (priv->rdata_xfer_busy > 1) {
+	if (priv->buffered && priv->rdata_xfer_busy > 1) {
 		/*
 		 * 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.
 		 */
 		priv->rdata_xfer_busy = 1;
+		ads1298_rdata_start(priv);
 	} else {
 		/* No pending data, wait for DRDY */
 		priv->rdata_xfer_busy = 0;
+		wake_up(&priv->idle_wq);
 	}
 }
 
@@ -494,19 +544,32 @@ static void ads1298_rdata_complete(void *context)
 {
 	struct iio_dev *indio_dev = context;
 	struct ads1298_private *priv = iio_priv(indio_dev);
+	int status = priv->rdata_msg.status;
 	int scan_index;
 	u32 *bounce = priv->bounce_buffer;
+	bool buffered;
 
-	if (!iio_buffer_enabled(indio_dev)) {
+	/* Only changes while no transfer is in flight */
+	scoped_guard(spinlock_irqsave, &priv->irq_busy_lock)
+		buffered = priv->buffered;
+
+	if (!buffered) {
 		/*
 		 * for a single transfer mode we're kept in direct_mode until
 		 * completion, avoiding a race with buffered IO.
 		 */
-		ads1298_rdata_unmark_busy(priv);
+		priv->rdata_status = status;
+		ads1298_rdata_release_busy_or_restart(priv);
 		complete(&priv->completion);
 		return;
 	}
 
+	/* A failed transfer leaves no sample to push */
+	if (status) {
+		ads1298_rdata_release_busy_or_restart(priv);
+		return;
+	}
+
 	/* Demux the channel data into our bounce buffer */
 	iio_for_each_active_channel(indio_dev, scan_index) {
 		const struct iio_chan_spec *scan_chan =
@@ -516,10 +579,13 @@ static void ads1298_rdata_complete(void *context)
 		*bounce++ = get_unaligned_be24(data);
 	}
 
-	/* rx_buffer can be overwritten from this point on */
-	ads1298_rdata_release_busy_or_restart(priv);
-
+	/*
+	 * Push before marking the transfer done: predisable waits for that,
+	 * and the scan must not be torn down while it is used here.
+	 */
 	iio_push_to_buffers(indio_dev, priv->bounce_buffer);
+
+	ads1298_rdata_release_busy_or_restart(priv);
 }
 
 static irqreturn_t ads1298_interrupt(int irq, void *dev_id)
@@ -533,7 +599,7 @@ static irqreturn_t ads1298_interrupt(int irq, void *dev_id)
 	wasbusy = priv->rdata_xfer_busy++;
 	/* When no SPI transfer in transit, start one now */
 	if (!wasbusy)
-		spi_async(priv->spi, &priv->rdata_msg);
+		ads1298_rdata_start(priv);
 
 	return IRQ_HANDLED;
 };
@@ -549,14 +615,39 @@ static int ads1298_buffer_postenable(struct iio_dev *indio_dev)
 	if (ret)
 		return ret;
 
-	return ads1298_write_cmd(priv, ADS1298_CMD_START);
+	scoped_guard(spinlock_irqsave, &priv->irq_busy_lock)
+		priv->buffered = true;
+
+	ret = ads1298_write_cmd(priv, ADS1298_CMD_START);
+	if (ret) {
+		scoped_guard(spinlock_irqsave, &priv->irq_busy_lock)
+			priv->buffered = false;
+	}
+
+	return ret;
 }
 
 static int ads1298_buffer_predisable(struct iio_dev *indio_dev)
 {
 	struct ads1298_private *priv = iio_priv(indio_dev);
+	int ret, ret2;
+	int rate;
+
+	ret = ads1298_write_cmd(priv, ADS1298_CMD_STOP);
+
+	/*
+	 * The conversion in progress completes and raises DRDY once more.
+	 * Wait for it and for its transfer before the core tears down the
+	 * scan.
+	 */
+	if (ads1298_get_samp_freq(priv, &rate) >= 0 && rate > 0)
+		fsleep(2 * DIV_ROUND_UP(USEC_PER_SEC, rate));
+	ret2 = ads1298_wait_rdata_idle(priv);
 
-	return ads1298_write_cmd(priv, ADS1298_CMD_STOP);
+	scoped_guard(spinlock_irqsave, &priv->irq_busy_lock)
+		priv->buffered = false;
+
+	return ret ?: ret2;
 }
 
 static const struct iio_buffer_setup_ops ads1298_setup_ops = {
@@ -569,6 +660,15 @@ static void ads1298_reg_disable(void *reg)
 	regulator_disable(reg);
 }
 
+static void ads1298_quiesce(void *data)
+{
+	struct ads1298_private *priv = data;
+
+	/* No new transfer may start, and the last one must end, before teardown */
+	disable_irq(priv->spi->irq);
+	ads1298_wait_rdata_idle(priv);
+}
+
 static const struct regmap_range ads1298_regmap_volatile_range[] = {
 	regmap_reg_range(ADS1298_REG_LOFF_STATP, ADS1298_REG_LOFF_STATN),
 };
@@ -690,6 +790,7 @@ static int ads1298_probe(struct spi_device *spi)
 
 	priv->spi = spi;
 	init_completion(&priv->completion);
+	init_waitqueue_head(&priv->idle_wq);
 	spin_lock_init(&priv->irq_busy_lock);
 	priv->regmap = devm_regmap_init(dev, NULL, priv, &ads1298_regmap_config);
 	if (IS_ERR(priv->regmap))
@@ -735,6 +836,10 @@ static int ads1298_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
+	ret = devm_add_action_or_reset(dev, ads1298_quiesce, priv);
+	if (ret)
+		return ret;
+
 	ret = devm_iio_kfifo_buffer_setup(dev, indio_dev, &ads1298_setup_ops);
 	if (ret)
 		return ret;
-- 
2.43.0

[PATCH v2 2/6] iio: adc: ti-ads1298: fix power-up and clock handling

[Stephan Costa Barros]

Fix three issues with the power-up sequence and the device clock:

- The datasheet powers up the supplies first, then the clock, then
  waits t_POR = 2^18 clock cycles (128 ms at 2.048 MHz) before the
  reset. The driver enabled an external clock and the vref supply
  before AVDD, reset right away, and did not wait for the 150 ms
  start-up of the internal reference. Follow the datasheet. Probe takes
  about 280 ms longer.

- The binding describes one optional clock and no clock-names, but the
  driver asked for the clock named "clk". With a device tree that
  follows the binding, the lookup fails, the optional getter returns
  NULL, and a clock given in the device tree is never enabled, while the
  sampling frequency is computed from the nominal 2.048 MHz. Ask for the
  first clock of the node; a device tree with clock-names = "clk" still
  gets the same clock.

- A command takes 4 cycles of the device clock to decode, the minimum
  reset pulse is 2 cycles, and the reset takes 18 more. These were
  converted with the nominal 2.048 MHz, so a command ends after a fixed
  2 us, short of 4 cycles with an external clock below 2 MHz, which the
  ADS129x allows down to 1.94 MHz. Convert them with the rate of the
  clock in use. At 2.048 MHz the delays do not change.

The clock lookup was seen to fail on a Raspberry Pi 4, with a
fixed-clock in the device tree. The power-up order was found by review
and reproduced on an emulated device only. The decode time was found
with an emulated device; it is not seen on hardware running at
2.048 MHz.

Fixes: 00ef7708fa60 ("iio: adc: ti-ads1298: Add driver")
Signed-off-by: Stephan Costa Barros <stephanbrrs8@gmail.com>
---
 drivers/iio/adc/ti-ads1298.c | 73 +++++++++++++++++++++++++-----------
 1 file changed, 51 insertions(+), 22 deletions(-)

diff --git a/drivers/iio/adc/ti-ads1298.c b/drivers/iio/adc/ti-ads1298.c
index 1d32b1f47..f5965b994 100644
--- a/drivers/iio/adc/ti-ads1298.c
+++ b/drivers/iio/adc/ti-ads1298.c
@@ -84,8 +84,12 @@
 #define ADS1298_MAX_CHANNELS	8
 #define ADS1298_BITS_PER_SAMPLE	24
 #define ADS1298_CLK_RATE_HZ	2048000
-#define ADS1298_CLOCKS_TO_USECS(x) \
-		(DIV_ROUND_UP((x) * MICROHZ_PER_HZ, ADS1298_CLK_RATE_HZ))
+/* A command takes 4 clock cycles to decode, before the next one may start */
+#define ADS1298_DECODE_CLOCKS	4
+/* Power-on reset: wait 2^18 clock cycles before the reset */
+#define ADS1298_TPOR_CLOCKS	BIT(18)
+/* Start-up time of the internal reference */
+#define ADS1298_REF_START_UP_MS	150
 /*
  * 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
@@ -106,6 +110,7 @@ struct ads1298_private {
 	struct regulator *reg_avdd;
 	struct regulator *reg_vref;
 	struct clk *clk;
+	unsigned long clk_rate;
 	struct regmap *regmap;
 	struct completion completion;
 	struct iio_trigger *trig;
@@ -180,6 +185,12 @@ static const struct iio_chan_spec ads1298_channels[] = {
 	ADS1298_CHAN(7),
 };
 
+static unsigned int ads1298_clocks_to_usecs(struct ads1298_private *priv,
+					    unsigned int clocks)
+{
+	return DIV_ROUND_UP(clocks * USEC_PER_SEC, priv->clk_rate);
+}
+
 static int ads1298_write_cmd(struct ads1298_private *priv, u8 command)
 {
 	struct spi_transfer xfer = {
@@ -188,7 +199,7 @@ static int ads1298_write_cmd(struct ads1298_private *priv, u8 command)
 		.len = 1,
 		.speed_hz = ADS1298_SPI_BUS_SPEED_SLOW,
 		.delay = {
-			.value = 2,
+			.value = ads1298_clocks_to_usecs(priv, ADS1298_DECODE_CLOCKS),
 			.unit = SPI_DELAY_UNIT_USECS,
 		},
 	};
@@ -433,7 +444,7 @@ static int ads1298_reg_write(void *context, unsigned int reg, unsigned int val)
 		.len = 3,
 		.speed_hz = ADS1298_SPI_BUS_SPEED_SLOW,
 		.delay = {
-			.value = 2,
+			.value = ads1298_clocks_to_usecs(priv, ADS1298_DECODE_CLOCKS),
 			.unit = SPI_DELAY_UNIT_USECS,
 		},
 	};
@@ -454,7 +465,7 @@ static int ads1298_reg_read(void *context, unsigned int reg, unsigned int *val)
 		.len = 3,
 		.speed_hz = ADS1298_SPI_BUS_SPEED_SLOW,
 		.delay = {
-			.value = 2,
+			.value = ads1298_clocks_to_usecs(priv, ADS1298_DECODE_CLOCKS),
 			.unit = SPI_DELAY_UNIT_USECS,
 		},
 	};
@@ -730,7 +741,14 @@ static int ads1298_init(struct iio_dev *indio_dev)
 		if (regulator_get_voltage(priv->reg_avdd) >= 4400000)
 			val |= ADS1298_MASK_CONFIG3_VREF_4V;
 	}
-	return regmap_write(priv->regmap, ADS1298_REG_CONFIG3, val);
+	ret = regmap_write(priv->regmap, ADS1298_REG_CONFIG3, val);
+	if (ret)
+		return ret;
+
+	if (!priv->reg_vref)
+		msleep(ADS1298_REF_START_UP_MS);
+
+	return 0;
 }
 
 static int ads1298_probe(struct spi_device *spi)
@@ -739,6 +757,7 @@ static int ads1298_probe(struct spi_device *spi)
 	struct iio_dev *indio_dev;
 	struct device *dev = &spi->dev;
 	struct gpio_desc *reset_gpio;
+	unsigned long clk_rate;
 	int ret;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*priv));
@@ -753,6 +772,19 @@ static int ads1298_probe(struct spi_device *spi)
 		return dev_err_probe(dev, PTR_ERR(reset_gpio),
 				     "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");
+
+	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;
+
 	/* VREF can be supplied externally, otherwise use internal reference */
 	priv->reg_vref = devm_regulator_get_optional(dev, "vref");
 	if (IS_ERR(priv->reg_vref)) {
@@ -771,22 +803,15 @@ static int ads1298_probe(struct spi_device *spi)
 			return ret;
 	}
 
-	priv->clk = devm_clk_get_optional_enabled(dev, "clk");
+	/* The clock follows the supplies */
+	priv->clk = devm_clk_get_optional_enabled(dev, NULL);
 	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;
+	clk_rate = priv->clk ? clk_get_rate(priv->clk) : 0;
+	if (!clk_rate)
+		clk_rate = ADS1298_CLK_RATE_HZ;
+	priv->clk_rate = clk_rate;
 
 	priv->spi = spi;
 	init_completion(&priv->completion);
@@ -800,12 +825,15 @@ static int ads1298_probe(struct spi_device *spi)
 	indio_dev->channels = ads1298_channels;
 	indio_dev->info = &ads1298_info;
 
+	/* Power-on reset time, counted from when the clock is valid */
+	msleep(DIV_ROUND_UP(ADS1298_TPOR_CLOCKS * MILLI, clk_rate));
+
 	if (reset_gpio) {
 		/*
 		 * Deassert reset now that clock and power are active.
 		 * Minimum reset pulsewidth is 2 clock cycles.
 		 */
-		fsleep(ADS1298_CLOCKS_TO_USECS(2));
+		fsleep(ads1298_clocks_to_usecs(priv, 2));
 		gpiod_set_value_cansleep(reset_gpio, 0);
 	} else {
 		ret = ads1298_write_cmd(priv, ADS1298_CMD_RESET);
@@ -813,7 +841,7 @@ static int ads1298_probe(struct spi_device *spi)
 			return dev_err_probe(dev, ret, "RESET failed\n");
 	}
 	/* Wait 18 clock cycles for reset command to complete */
-	fsleep(ADS1298_CLOCKS_TO_USECS(18));
+	fsleep(ads1298_clocks_to_usecs(priv, 18));
 
 	ret = ads1298_init(indio_dev);
 	if (ret)
@@ -824,7 +852,8 @@ static int ads1298_probe(struct spi_device *spi)
 	priv->rdata_xfer.rx_buf = priv->rx_buffer;
 	priv->rdata_xfer.len = ADS1298_SPI_RDATA_BUFFER_SIZE(indio_dev->num_channels);
 	/* Must keep CS low for 4 clocks */
-	priv->rdata_xfer.delay.value = 2;
+	priv->rdata_xfer.delay.value = ads1298_clocks_to_usecs(priv,
+							       ADS1298_DECODE_CLOCKS);
 	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;
-- 
2.43.0

[PATCH v2 3/6] iio: adc: ti-ads1298: fix scale, ID check, regmap and Kconfig

[Stephan Costa Barros]

Fix four smaller issues:

- The scale divides the reference, in whole millivolts, by the gain
  before it is reported. With the internal 4 V reference, it is 0.099 %
  low at gain 6, 0.101 % at gain 12 and 0.025 % at gain 3. Report
  VREF / (gain * 2^23) as an IIO_VAL_FRACTIONAL; sysfs still shows it
  with nine decimals, now without the error.

- Only the channel codes 0, 1 and 2 of the ID register are defined; an
  ID of 0x93, for example, gives 10 channels. The IIO core then reads
  past ads1298_channels, and the RDATA transfer writes past rx_buffer.
  Refuse the reserved codes as an unknown ID.

- The driver uses regmap but does not select REGMAP, and fails to build
  where nothing else selects it, such as tinyconfig with SPI, IIO and
  TI_ADS1298 on x86_64.

- Bit 0 of CONFIG3 is a read-only status bit and the GPIO register
  reads the pins, but both are cached, so a read through debugfs returns
  a stale value; debugfs can also write ID, LOFF_STATP and LOFF_STATN.
  Mark CONFIG3 and GPIO volatile, and the other three not writeable.

The ID check was found by review and reproduced on an emulated device
only, where KASAN reports the read past ads1298_channels.

Fixes: 00ef7708fa60 ("iio: adc: ti-ads1298: Add driver")
Signed-off-by: Stephan Costa Barros <stephanbrrs8@gmail.com>
---
 drivers/iio/adc/Kconfig      |  1 +
 drivers/iio/adc/ti-ads1298.c | 29 ++++++++++++++++++++++++-----
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index f32f678f7..a29ed8f45 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1924,6 +1924,7 @@ config TI_ADS1298
 	depends on SPI
 	select IIO_BUFFER
 	select IIO_KFIFO_BUF
+	select REGMAP
 	help
 	  If you say yes here you get support for Texas Instruments ADS1298
 	  medical ADC chips
diff --git a/drivers/iio/adc/ti-ads1298.c b/drivers/iio/adc/ti-ads1298.c
index f5965b994..b390c13ae 100644
--- a/drivers/iio/adc/ti-ads1298.c
+++ b/drivers/iio/adc/ti-ads1298.c
@@ -75,6 +75,7 @@
 
 #define ADS1298_REG_LOFF_STATP	0x12
 #define ADS1298_REG_LOFF_STATN	0x13
+#define ADS1298_REG_GPIO	0x14
 #define ADS1298_REG_CONFIG4	0x17
 #define ADS1298_MASK_CONFIG4_SINGLE_SHOT	BIT(3)
 
@@ -373,11 +374,10 @@ static int ads1298_get_scale(struct ads1298_private *priv,
 		return -EINVAL;
 
 	gain = ads1298_pga_settings[pga_idx];
-	*val /= gain; /* Full scale is VREF / gain */
+	/* Full scale is VREF / gain, signed, hence the -1 */
+	*val2 = gain << (ADS1298_BITS_PER_SAMPLE - 1);
 
-	*val2 = ADS1298_BITS_PER_SAMPLE - 1; /* Signed, hence the -1 */
-
-	return IIO_VAL_FRACTIONAL_LOG2;
+	return IIO_VAL_FRACTIONAL;
 }
 
 static int ads1298_read_raw(struct iio_dev *indio_dev,
@@ -680,8 +680,11 @@ static void ads1298_quiesce(void *data)
 	ads1298_wait_rdata_idle(priv);
 }
 
+/* CONFIG3 holds the read-only RLD_STAT; GPIO reads the pins */
 static const struct regmap_range ads1298_regmap_volatile_range[] = {
+	regmap_reg_range(ADS1298_REG_CONFIG3, ADS1298_REG_CONFIG3),
 	regmap_reg_range(ADS1298_REG_LOFF_STATP, ADS1298_REG_LOFF_STATN),
+	regmap_reg_range(ADS1298_REG_GPIO, ADS1298_REG_GPIO),
 };
 
 static const struct regmap_access_table ads1298_regmap_volatile = {
@@ -689,12 +692,23 @@ static const struct regmap_access_table ads1298_regmap_volatile = {
 	.n_yes_ranges = ARRAY_SIZE(ads1298_regmap_volatile_range),
 };
 
+static const struct regmap_range ads1298_regmap_read_only_range[] = {
+	regmap_reg_range(ADS1298_REG_ID, ADS1298_REG_ID),
+	regmap_reg_range(ADS1298_REG_LOFF_STATP, ADS1298_REG_LOFF_STATN),
+};
+
+static const struct regmap_access_table ads1298_regmap_writeable = {
+	.no_ranges = ads1298_regmap_read_only_range,
+	.n_no_ranges = ARRAY_SIZE(ads1298_regmap_read_only_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,
+	.wr_table = &ads1298_regmap_writeable,
 	.volatile_table = &ads1298_regmap_volatile,
 	.cache_type = REGCACHE_MAPLE,
 };
@@ -704,6 +718,7 @@ static int ads1298_init(struct iio_dev *indio_dev)
 	struct ads1298_private *priv = iio_priv(indio_dev);
 	struct device *dev = &priv->spi->dev;
 	const char *suffix;
+	unsigned int nu_ch;
 	unsigned int val;
 	int ret;
 
@@ -717,7 +732,11 @@ static int ads1298_init(struct iio_dev *indio_dev)
 		return ret;
 
 	/* Fill in name and channel count based on what the chip told us */
-	indio_dev->num_channels = 4 + 2 * (val & ADS1298_MASK_ID_CHANNELS);
+	nu_ch = FIELD_GET(ADS1298_MASK_ID_CHANNELS, val);
+	/* 0, 1 and 2 are 4, 6 and 8 channels; the other codes are reserved */
+	if (nu_ch > 2)
+		return dev_err_probe(dev, -ENODEV, "Unknown ID: 0x%x\n", val);
+	indio_dev->num_channels = 4 + 2 * nu_ch;
 	switch (val & ADS1298_MASK_ID_FAMILY) {
 	case ADS1298_ID_FAMILY_ADS129X:
 		suffix = "";
-- 
2.43.0

[PATCH v2 4/6] iio: adc: ti-ads1298: prepare for more chip families

[Stephan Costa Barros]

The ADS1299 has the register map of the ADS129x, with other gains, data
rates, reference and ID layout. To support it in this driver, collect
what differs between families in a chip info structure, given as match
data. The private structure has carried an unused chip_info pointer
since the driver was added.

The chip info holds:
- the regmap configuration and the PGA gain table;
- the data rate rules: the base shift, whether CONFIG1 has an HR bit,
  and the oversampling ratio at DR = 0;
- the internal reference, whether CONFIG3 has the VREF_4V bit, and the
  reserved bits of CONFIG3 that must be written as 1;
- a callback that checks the ID register and sets the channel count and
  the name, as the ID layouts differ; bit 3 alone tells them apart.

No functional change.

Signed-off-by: Stephan Costa Barros <stephanbrrs8@gmail.com>
---
 drivers/iio/adc/ti-ads1298.c | 132 +++++++++++++++++++++++++----------
 1 file changed, 96 insertions(+), 36 deletions(-)

diff --git a/drivers/iio/adc/ti-ads1298.c b/drivers/iio/adc/ti-ads1298.c
index b390c13ae..5ad543bdf 100644
--- a/drivers/iio/adc/ti-ads1298.c
+++ b/drivers/iio/adc/ti-ads1298.c
@@ -49,8 +49,6 @@
 #define ADS1298_REG_CONFIG1	0x01
 #define ADS1298_MASK_CONFIG1_HR			BIT(7)
 #define ADS1298_MASK_CONFIG1_DR			GENMASK(2, 0)
-#define ADS1298_SHIFT_DR_HR			6
-#define ADS1298_SHIFT_DR_LP			7
 #define ADS1298_LOWEST_DR			0x06
 
 #define ADS1298_REG_CONFIG2	0x02
@@ -66,6 +64,7 @@
 #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_VREF_4V_MV			4000
 
 #define ADS1298_REG_LOFF	0x04
 #define ADS1298_REG_CHnSET(n)	(0x05 + (n))
@@ -105,6 +104,31 @@
 #define ADS1298_SPI_RDATA_BUFFER_SIZE_MAX \
 		ADS1298_SPI_RDATA_BUFFER_SIZE(ADS1298_MAX_CHANNELS)
 
+/* What differs between the chip families the driver supports */
+struct ads1298_chip_info {
+	/* Checks the ID register, and sets the channel count and the name */
+	int (*parse_id)(struct iio_dev *indio_dev, unsigned int id);
+	const struct regmap_config *regmap_config;
+	/* PGA gain for each value of the GAIN field of CHnSET */
+	const u8 *gains;
+	unsigned int num_gains;
+	/*
+	 * The data rate is f_CLK >> (dr_shift + DR). With an HR bit in
+	 * CONFIG1, dr_shift is that of the high-resolution mode, and the
+	 * low-power mode shifts one more.
+	 */
+	unsigned int dr_shift;
+	bool has_hr;
+	/* Oversampling ratio at DR = 0 */
+	unsigned int osr_base;
+	/* Internal reference, in millivolts */
+	unsigned int vref_mv;
+	/* CONFIG3 has the VREF_4V bit, which selects a 4 V internal reference */
+	bool has_vref_4v;
+	/* Reserved bits of CONFIG3 that must be written as 1 */
+	unsigned int config3_reserved;
+};
+
 struct ads1298_private {
 	const struct ads1298_chip_info *chip_info;
 	struct spi_device *spi;
@@ -286,6 +310,7 @@ static int ads1298_read_one(struct ads1298_private *priv, int chan_index)
 
 static int ads1298_get_samp_freq(struct ads1298_private *priv, int *val)
 {
+	const struct ads1298_chip_info *info = priv->chip_info;
 	unsigned long rate;
 	unsigned int cfg;
 	int ret;
@@ -302,10 +327,10 @@ static int ads1298_get_samp_freq(struct ads1298_private *priv, int *val)
 		return -EINVAL;
 
 	/* Data rate shift depends on HR/LP mode */
-	if (cfg & ADS1298_MASK_CONFIG1_HR)
-		rate >>= ADS1298_SHIFT_DR_HR;
+	if (info->has_hr && !(cfg & ADS1298_MASK_CONFIG1_HR))
+		rate >>= info->dr_shift + 1;
 	else
-		rate >>= ADS1298_SHIFT_DR_LP;
+		rate >>= info->dr_shift;
 
 	*val = rate >> (cfg & ADS1298_MASK_CONFIG1_DR);
 
@@ -314,6 +339,7 @@ static int ads1298_get_samp_freq(struct ads1298_private *priv, int *val)
 
 static int ads1298_set_samp_freq(struct ads1298_private *priv, int val)
 {
+	const struct ads1298_chip_info *info = priv->chip_info;
 	unsigned long rate;
 	unsigned int factor;
 	unsigned int cfg;
@@ -327,9 +353,9 @@ static int ads1298_set_samp_freq(struct ads1298_private *priv, int val)
 	if (val <= 0)
 		return -EINVAL;
 
-	factor = (rate >> ADS1298_SHIFT_DR_HR) / val;
-	if (factor >= BIT(ADS1298_SHIFT_DR_LP))
-		cfg = ADS1298_LOWEST_DR;
+	factor = (rate >> info->dr_shift) / val;
+	if (factor >= BIT(ADS1298_LOWEST_DR + 1))
+		cfg = ADS1298_LOWEST_DR; /* Lowest rate, in LP mode */
 	else if (factor)
 		cfg = ADS1298_MASK_CONFIG1_HR | ilog2(factor); /* Use HR mode */
 	else
@@ -340,11 +366,10 @@ static int ads1298_set_samp_freq(struct ads1298_private *priv, int val)
 				  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)
 {
+	const struct ads1298_chip_info *info = priv->chip_info;
 	unsigned int pga_idx;
 	int ret;
 	unsigned int regval;
@@ -356,13 +381,16 @@ static int ads1298_get_scale(struct ads1298_private *priv,
 			return ret;
 
 		*val = ret / MILLI; /* Convert to millivolts */
-	} else {
+	} else if (info->has_vref_4v) {
 		ret = regmap_read(priv->regmap, ADS1298_REG_CONFIG3, &regval);
 		if (ret)
 			return ret;
 
 		/* Reference in millivolts */
-		*val = regval & ADS1298_MASK_CONFIG3_VREF_4V ? 4000 : 2400;
+		*val = regval & ADS1298_MASK_CONFIG3_VREF_4V ?
+		       ADS1298_VREF_4V_MV : info->vref_mv;
+	} else {
+		*val = info->vref_mv;
 	}
 
 	ret = regmap_read(priv->regmap, ADS1298_REG_CHnSET(channel), &regval);
@@ -370,10 +398,10 @@ static int ads1298_get_scale(struct ads1298_private *priv,
 		return ret;
 
 	pga_idx = FIELD_GET(ADS1298_MASK_CH_PGA, regval);
-	if (pga_idx >= ARRAY_SIZE(ads1298_pga_settings))
+	if (pga_idx >= info->num_gains)
 		return -EINVAL;
 
-	gain = ads1298_pga_settings[pga_idx];
+	gain = info->gains[pga_idx];
 	/* Full scale is VREF / gain, signed, hence the -1 */
 	*val2 = gain << (ADS1298_BITS_PER_SAMPLE - 1);
 
@@ -414,7 +442,7 @@ static int ads1298_read_raw(struct iio_dev *indio_dev,
 		if (ret)
 			return ret;
 
-		*val = 16 << (*val & ADS1298_MASK_CONFIG1_DR);
+		*val = priv->chip_info->osr_base << (*val & ADS1298_MASK_CONFIG1_DR);
 		return IIO_VAL_INT;
 	default:
 		return -EINVAL;
@@ -713,31 +741,20 @@ static const struct regmap_config ads1298_regmap_config = {
 	.cache_type = REGCACHE_MAPLE,
 };
 
-static int ads1298_init(struct iio_dev *indio_dev)
+static int ads1298_parse_id(struct iio_dev *indio_dev, unsigned int id)
 {
 	struct ads1298_private *priv = iio_priv(indio_dev);
 	struct device *dev = &priv->spi->dev;
 	const char *suffix;
 	unsigned int nu_ch;
-	unsigned int val;
-	int ret;
-
-	/* 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;
 
 	/* Fill in name and channel count based on what the chip told us */
-	nu_ch = FIELD_GET(ADS1298_MASK_ID_CHANNELS, val);
+	nu_ch = FIELD_GET(ADS1298_MASK_ID_CHANNELS, id);
 	/* 0, 1 and 2 are 4, 6 and 8 channels; the other codes are reserved */
 	if (nu_ch > 2)
-		return dev_err_probe(dev, -ENODEV, "Unknown ID: 0x%x\n", val);
+		return dev_err_probe(dev, -ENODEV, "Unknown ID: 0x%x\n", id);
 	indio_dev->num_channels = 4 + 2 * nu_ch;
-	switch (val & ADS1298_MASK_ID_FAMILY) {
+	switch (id & ADS1298_MASK_ID_FAMILY) {
 	case ADS1298_ID_FAMILY_ADS129X:
 		suffix = "";
 		break;
@@ -745,19 +762,57 @@ static int ads1298_init(struct iio_dev *indio_dev)
 		suffix = "r";
 		break;
 	default:
-		return dev_err_probe(dev, -ENODEV, "Unknown ID: 0x%x\n", val);
+		return dev_err_probe(dev, -ENODEV, "Unknown ID: 0x%x\n", id);
 	}
 	indio_dev->name = devm_kasprintf(dev, GFP_KERNEL, "ads129%u%s",
 					 indio_dev->num_channels, suffix);
 	if (!indio_dev->name)
 		return -ENOMEM;
 
-	val = ADS1298_MASK_CONFIG3_RESERVED; /* Must write 1 always */
+	return 0;
+}
+
+static const u8 ads1298_gains[] = { 6, 1, 2, 3, 4, 8, 12 };
+
+static const struct ads1298_chip_info ads1298_chip_info = {
+	.parse_id = ads1298_parse_id,
+	.regmap_config = &ads1298_regmap_config,
+	.gains = ads1298_gains,
+	.num_gains = ARRAY_SIZE(ads1298_gains),
+	.dr_shift = 6,
+	.has_hr = true,
+	.osr_base = 16,
+	.vref_mv = 2400,
+	.has_vref_4v = true,
+	.config3_reserved = ADS1298_MASK_CONFIG3_RESERVED,
+};
+
+static int ads1298_init(struct iio_dev *indio_dev)
+{
+	struct ads1298_private *priv = iio_priv(indio_dev);
+	unsigned int val;
+	int ret;
+
+	/* 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;
+
+	ret = priv->chip_info->parse_id(indio_dev, val);
+	if (ret)
+		return ret;
+
+	val = priv->chip_info->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)
+		if (priv->chip_info->has_vref_4v &&
+		    regulator_get_voltage(priv->reg_avdd) >= 4400000)
 			val |= ADS1298_MASK_CONFIG3_VREF_4V;
 	}
 	ret = regmap_write(priv->regmap, ADS1298_REG_CONFIG3, val);
@@ -785,6 +840,10 @@ static int ads1298_probe(struct spi_device *spi)
 
 	priv = iio_priv(indio_dev);
 
+	priv->chip_info = spi_get_device_match_data(spi);
+	if (!priv->chip_info)
+		return -ENODEV;
+
 	/* Reset to be asserted before enabling clock and power */
 	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
 	if (IS_ERR(reset_gpio))
@@ -836,7 +895,8 @@ static int ads1298_probe(struct spi_device *spi)
 	init_completion(&priv->completion);
 	init_waitqueue_head(&priv->idle_wq);
 	spin_lock_init(&priv->irq_busy_lock);
-	priv->regmap = devm_regmap_init(dev, NULL, priv, &ads1298_regmap_config);
+	priv->regmap = devm_regmap_init(dev, NULL, priv,
+					priv->chip_info->regmap_config);
 	if (IS_ERR(priv->regmap))
 		return PTR_ERR(priv->regmap);
 
@@ -896,13 +956,13 @@ static int ads1298_probe(struct spi_device *spi)
 }
 
 static const struct spi_device_id ads1298_id[] = {
-	{ .name = "ads1298" },
+	{ .name = "ads1298", .driver_data = (kernel_ulong_t)&ads1298_chip_info },
 	{ }
 };
 MODULE_DEVICE_TABLE(spi, ads1298_id);
 
 static const struct of_device_id ads1298_of_table[] = {
-	{ .compatible = "ti,ads1298" },
+	{ .compatible = "ti,ads1298", .data = &ads1298_chip_info },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, ads1298_of_table);
-- 
2.43.0

[PATCH v2 5/6] dt-bindings: iio: adc: ti,ads1298: add ADS1299

[Stephan Costa Barros]

The ADS1299 is an 8-channel, 24-bit biopotential ADC for EEG with the
register map of the ADS129x. It differs in its gains, data rates and
ID layout, and has a fixed 4.5 V internal reference. The ADS1299-4 and
ADS1299-6 are the same part with four and six channels.

TI sells the 8-channel part as plain ADS1299, so ti,ads1299 names it,
and it is the fallback for ti,ads1299-4 and ti,ads1299-6: the channel
count can be read from the ID register. The ADS1299 is not compatible
with the ADS1298, so ti,ads1298 is not a fallback for it.

In the same way, give each ADS129x part a compatible, with ti,ads1298 as
the fallback: the ID register tells the parts apart, by family and
channel count. ti,ads1298 alone remains valid, so existing device trees
are not affected.

Both families have a digital supply, DVDD, and an active-low power-down
pin, PWDN, which the binding did not describe. Add them, both optional.

Add myself as a maintainer of the binding, next to Mike.

Signed-off-by: Stephan Costa Barros <stephanbrrs8@gmail.com>
---
 .../bindings/iio/adc/ti,ads1298.yaml          | 41 +++++++++++++++----
 1 file changed, 34 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads1298.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads1298.yaml
index afcc683db..c88f0dd95 100644
--- a/Documentation/devicetree/bindings/iio/adc/ti,ads1298.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/ti,ads1298.yaml
@@ -4,19 +4,36 @@
 $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
+title: Texas Instruments' ads1298 and ads1299 medical ADC chips
 
 description: |
-  Datasheet at: https://www.ti.com/product/ADS1298
-  Bindings for this chip aren't complete.
+  Datasheets at: https://www.ti.com/product/ADS1298
+  and https://www.ti.com/product/ADS1299
+  Bindings for these chips aren't complete.
 
 maintainers:
   - Mike Looijmans <mike.looijmans@topic.nl>
+  - Stephan Costa Barros <stephanbrrs8@gmail.com>
 
 properties:
   compatible:
-    enum:
-      - ti,ads1298
+    oneOf:
+      - enum:
+          - ti,ads1298
+          - ti,ads1299
+      - items:
+          - enum:
+              - ti,ads1294
+              - ti,ads1294r
+              - ti,ads1296
+              - ti,ads1296r
+              - ti,ads1298r
+          - const: ti,ads1298
+      - items:
+          - enum:
+              - ti,ads1299-4
+              - ti,ads1299-6
+          - const: ti,ads1299
 
   reg:
     maxItems: 1
@@ -26,15 +43,25 @@ properties:
   reset-gpios:
     maxItems: 1
 
+  powerdown-gpios:
+    description:
+      Active low GPIO connected to the PWDN pin. While it is asserted, the
+      device is powered down.
+    maxItems: 1
+
   avdd-supply:
     description:
       Analog power supply, voltage between AVDD and AVSS. When providing a
       symmetric +/- 2.5V, the regulator should report 5V.
 
+  dvdd-supply:
+    description: Digital power supply, voltage between DVDD and DGND.
+
   vref-supply:
     description:
-      Optional reference voltage. If omitted, internal reference is used,
-      which is 2.4V when analog supply is below 4.4V, 4V otherwise.
+      Optional reference voltage. If omitted, internal reference is used.
+      On the ads129x, it is 2.4V when analog supply is below 4.4V, 4V
+      otherwise. On the ads1299, it is 4.5V.
 
   clocks:
     description: Optional 2.048 MHz external source clock on CLK pin
-- 
2.43.0

[PATCH v2 6/6] iio: adc: ti-ads1298: add ADS1299 support

[Stephan Costa Barros]

The ADS1299, ADS1299-6 and ADS1299-4 are 8, 6 and 4-channel, 24-bit
biopotential ADCs for EEG. They share the register map, the commands
and the data format of the ADS129x, and differ in:

- the PGA gains, 1, 2, 4, 6, 8, 12 and 24;
- the data rates, f_CLK / 2^(7 + DR) with no HR bit, from 16 kSPS to
  250 SPS, and the oversampling ratio, 64 * 2^DR;
- the internal reference, a fixed 4.5 V, and the reserved bits 6:5 of
  CONFIG3;
- the ID register, which holds the channel count in bits 1:0;
- the register map, which ends at CONFIG4;
- the inputs, which are shorted out of reset.

Add the chip info for the ADS1299, and connect the inputs of every
channel at init, which changes nothing on the ADS129x, whose inputs are
already connected out of reset. The channel count is read from the ID
register, so ti,ads1299 also serves the ADS1299-4 and ADS1299-6 through
their fallback.

For both families, enable the DVDD supply when the device tree gives
one, and hold PWDN asserted, like RESET, until the supplies and the
clock are up. Nothing changes for a device tree that has neither.

Tested on an ADS1299 with a Raspberry Pi 4.

Signed-off-by: Stephan Costa Barros <stephanbrrs8@gmail.com>
---
 drivers/iio/adc/Kconfig      |   6 +--
 drivers/iio/adc/ti-ads1298.c | 100 +++++++++++++++++++++++++++++++++--
 2 files changed, 100 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index a29ed8f45..724ae6123 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1920,14 +1920,14 @@ config TI_ADS1262
 	  called ti-ads1262.
 
 config TI_ADS1298
-	tristate "Texas Instruments ADS1298"
+	tristate "Texas Instruments ADS1298 and ADS1299"
 	depends on SPI
 	select IIO_BUFFER
 	select IIO_KFIFO_BUF
 	select REGMAP
 	help
-	  If you say yes here you get support for Texas Instruments ADS1298
-	  medical ADC chips
+	  If you say yes here you get support for Texas Instruments ADS1294,
+	  ADS1296, ADS1298 and ADS1299 medical ADC chips, and their variants
 
 	  This driver can also be built as a module. If so, the module will be
 	  called ti-ads1298.
diff --git a/drivers/iio/adc/ti-ads1298.c b/drivers/iio/adc/ti-ads1298.c
index 5ad543bdf..e0cea23b1 100644
--- a/drivers/iio/adc/ti-ads1298.c
+++ b/drivers/iio/adc/ti-ads1298.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
-/* TI ADS1298 chip family driver
+/* TI ADS1298 and ADS1299 chip family driver
  * Copyright (C) 2023 - 2024 Topic Embedded Products
+ * Copyright (C) 2026 Stephan Costa Barros <stephanbrrs8@gmail.com>
  */
 
 #include <linux/bitfield.h>
@@ -45,6 +46,10 @@
 #define ADS1298_MASK_ID_CHANNELS		GENMASK(2, 0)
 #define ADS1298_ID_FAMILY_ADS129X		0x90
 #define ADS1298_ID_FAMILY_ADS129XR		0xd0
+/* ADS1299: bit 4 reads 1 and DEV_ID[3:2] is 11; bits 7:5 are the revision */
+#define ADS1299_MASK_ID_DEVICE			GENMASK(4, 2)
+#define ADS1299_MASK_ID_CHANNELS		GENMASK(1, 0)
+#define ADS1299_ID_DEVICE			0x7
 
 #define ADS1298_REG_CONFIG1	0x01
 #define ADS1298_MASK_CONFIG1_HR			BIT(7)
@@ -65,12 +70,14 @@
 #define ADS1298_MASK_CONFIG3_RESERVED		BIT(6)
 #define ADS1298_MASK_CONFIG3_VREF_4V		BIT(5)
 #define ADS1298_VREF_4V_MV			4000
+#define ADS1299_MASK_CONFIG3_RESERVED		GENMASK(6, 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_MUX_NORMAL		0
 
 #define ADS1298_REG_LOFF_STATP	0x12
 #define ADS1298_REG_LOFF_STATN	0x13
@@ -354,6 +361,15 @@ static int ads1298_set_samp_freq(struct ads1298_private *priv, int val)
 		return -EINVAL;
 
 	factor = (rate >> info->dr_shift) / val;
+
+	if (!info->has_hr) {
+		/* The slowest rate at or above val, or the lowest of all */
+		cfg = factor ? min(ilog2(factor), ADS1298_LOWEST_DR) : 0;
+
+		return regmap_update_bits(priv->regmap, ADS1298_REG_CONFIG1,
+					  ADS1298_MASK_CONFIG1_DR, cfg);
+	}
+
 	if (factor >= BIT(ADS1298_LOWEST_DR + 1))
 		cfg = ADS1298_LOWEST_DR; /* Lowest rate, in LP mode */
 	else if (factor)
@@ -772,6 +788,43 @@ static int ads1298_parse_id(struct iio_dev *indio_dev, unsigned int id)
 	return 0;
 }
 
+/* The ADS1299 has no WCT registers */
+static const struct regmap_config ads1299_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.reg_read = ads1298_reg_read,
+	.reg_write = ads1298_reg_write,
+	.max_register = ADS1298_REG_CONFIG4,
+	.wr_table = &ads1298_regmap_writeable,
+	.volatile_table = &ads1298_regmap_volatile,
+	.cache_type = REGCACHE_MAPLE,
+};
+
+static int ads1299_parse_id(struct iio_dev *indio_dev, unsigned int id)
+{
+	struct ads1298_private *priv = iio_priv(indio_dev);
+	struct device *dev = &priv->spi->dev;
+	unsigned int nu_ch;
+
+	nu_ch = FIELD_GET(ADS1299_MASK_ID_CHANNELS, id);
+	/* 0, 1 and 2 are 4, 6 and 8 channels; 3 is reserved */
+	if (FIELD_GET(ADS1299_MASK_ID_DEVICE, id) != ADS1299_ID_DEVICE ||
+	    nu_ch > 2)
+		return dev_err_probe(dev, -ENODEV, "Unknown ID: 0x%x\n", id);
+	indio_dev->num_channels = 4 + 2 * nu_ch;
+
+	/* TI sells the 8-channel part as plain ADS1299 */
+	if (indio_dev->num_channels == ADS1298_MAX_CHANNELS)
+		indio_dev->name = "ads1299";
+	else
+		indio_dev->name = devm_kasprintf(dev, GFP_KERNEL, "ads1299-%u",
+						 indio_dev->num_channels);
+	if (!indio_dev->name)
+		return -ENOMEM;
+
+	return 0;
+}
+
 static const u8 ads1298_gains[] = { 6, 1, 2, 3, 4, 8, 12 };
 
 static const struct ads1298_chip_info ads1298_chip_info = {
@@ -787,10 +840,24 @@ static const struct ads1298_chip_info ads1298_chip_info = {
 	.config3_reserved = ADS1298_MASK_CONFIG3_RESERVED,
 };
 
+static const u8 ads1299_gains[] = { 1, 2, 4, 6, 8, 12, 24 };
+
+static const struct ads1298_chip_info ads1299_chip_info = {
+	.parse_id = ads1299_parse_id,
+	.regmap_config = &ads1299_regmap_config,
+	.gains = ads1299_gains,
+	.num_gains = ARRAY_SIZE(ads1299_gains),
+	.dr_shift = 7,
+	.osr_base = 64,
+	.vref_mv = 4500,
+	.config3_reserved = ADS1299_MASK_CONFIG3_RESERVED,
+};
+
 static int ads1298_init(struct iio_dev *indio_dev)
 {
 	struct ads1298_private *priv = iio_priv(indio_dev);
 	unsigned int val;
+	unsigned int i;
 	int ret;
 
 	/* Device initializes into RDATAC mode, which we don't want */
@@ -822,6 +889,16 @@ static int ads1298_init(struct iio_dev *indio_dev)
 	if (!priv->reg_vref)
 		msleep(ADS1298_REF_START_UP_MS);
 
+	/* The ADS1299 comes out of reset with its inputs shorted */
+	for (i = 0; i < indio_dev->num_channels; i++) {
+		ret = regmap_update_bits(priv->regmap, ADS1298_REG_CHnSET(i),
+					 ADS1298_MASK_CH_MUX,
+					 FIELD_PREP(ADS1298_MASK_CH_MUX,
+						    ADS1298_MUX_NORMAL));
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }
 
@@ -830,6 +907,7 @@ 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 *powerdown_gpio;
 	struct gpio_desc *reset_gpio;
 	unsigned long clk_rate;
 	int ret;
@@ -844,12 +922,18 @@ static int ads1298_probe(struct spi_device *spi)
 	if (!priv->chip_info)
 		return -ENODEV;
 
-	/* Reset to be asserted before enabling clock and power */
+	/* Reset and power-down to be asserted before enabling clock and power */
 	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
 	if (IS_ERR(reset_gpio))
 		return dev_err_probe(dev, PTR_ERR(reset_gpio),
 				     "Cannot get reset GPIO\n");
 
+	powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
+						 GPIOD_OUT_HIGH);
+	if (IS_ERR(powerdown_gpio))
+		return dev_err_probe(dev, PTR_ERR(powerdown_gpio),
+				     "Cannot get powerdown 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),
@@ -863,6 +947,10 @@ static int ads1298_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
+	ret = devm_regulator_get_enable_optional(dev, "dvdd");
+	if (ret && ret != -ENODEV)
+		return dev_err_probe(dev, ret, "Failed to enable dvdd 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)) {
@@ -904,6 +992,9 @@ static int ads1298_probe(struct spi_device *spi)
 	indio_dev->channels = ads1298_channels;
 	indio_dev->info = &ads1298_info;
 
+	/* Out of power-down, now that the supplies and the clock are up */
+	gpiod_set_value_cansleep(powerdown_gpio, 0);
+
 	/* Power-on reset time, counted from when the clock is valid */
 	msleep(DIV_ROUND_UP(ADS1298_TPOR_CLOCKS * MILLI, clk_rate));
 
@@ -957,12 +1048,14 @@ static int ads1298_probe(struct spi_device *spi)
 
 static const struct spi_device_id ads1298_id[] = {
 	{ .name = "ads1298", .driver_data = (kernel_ulong_t)&ads1298_chip_info },
+	{ .name = "ads1299", .driver_data = (kernel_ulong_t)&ads1299_chip_info },
 	{ }
 };
 MODULE_DEVICE_TABLE(spi, ads1298_id);
 
 static const struct of_device_id ads1298_of_table[] = {
 	{ .compatible = "ti,ads1298", .data = &ads1298_chip_info },
+	{ .compatible = "ti,ads1299", .data = &ads1299_chip_info },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, ads1298_of_table);
@@ -978,5 +1071,6 @@ static struct spi_driver ads1298_driver = {
 module_spi_driver(ads1298_driver);
 
 MODULE_AUTHOR("Mike Looijmans <mike.looijmans@topic.nl>");
-MODULE_DESCRIPTION("TI ADS1298 ADC");
+MODULE_AUTHOR("Stephan Costa Barros <stephanbrrs8@gmail.com>");
+MODULE_DESCRIPTION("TI ADS1298 and ADS1299 ADC");
 MODULE_LICENSE("GPL");
-- 
2.43.0

Re: [PATCH v2 1/6] iio: adc: ti-ads1298: fix the data read path

[Andy Shevchenko]

On Sat, Oct 24, 2026 at 08:13:30PM -0300, Stephan Costa Barros wrote:
> Fix five issues in how the driver reads samples:
> 
> - cmd_buffer serves both the register accessors, under the regmap lock,
>   and the commands, under the IIO device lock, and shares a cacheline
>   with rx_buffer and tx_buffer. A register access can then rewrite a
>   command on the bus, or corrupt an RDATA transfer in flight on a
>   platform without coherent DMA. Give each path its own buffer, each on
>   its own cacheline.
> 
> - After STOP, the conversion in progress still raises DRDY, so an RDATA
>   transfer can be in flight when the core frees the active scan mask.
>   Track in the driver whether samples go to the buffer, wait in
>   predisable for the last DRDY and its transfer, and push the sample
>   before marking the transfer done. update_scan_mode reset the transfer
>   counter, which let a new transfer be queued over one in flight; wait
>   for it instead.
> 
> - When a single-shot read times out, its transfer may still be in
>   flight, and the next read then wakes up with the old sample. On
>   unbind, nothing waits for the DRDY interrupt or its transfer. Wait for
>   the transfer before a single-shot read, and on teardown disable the
>   interrupt and wait for the last transfer.
> 
> - The return value of spi_async() and the status of the RDATA message
>   are ignored: a failure leaves the transfer counter stuck, and a failed
>   transfer is reported as a sample. Handle both.
> 
> - read_raw takes the sample out of rx_buffer after releasing direct
>   mode, so a buffer enabled in between can overwrite it. Take it while
>   direct mode is still held.
> 
> The first, second and last were found by review and not reproduced.
> The third and fourth were found by review and reproduced on an emulated
> device only.

...

> static void ads1298_rdata_complete(void *context)
>  {
>  	struct iio_dev *indio_dev = context;
>  	struct ads1298_private *priv = iio_priv(indio_dev);
> +	int status = priv->rdata_msg.status;
>  	int scan_index;
>  	u32 *bounce = priv->bounce_buffer;
> +	bool buffered;
>  
> -	if (!iio_buffer_enabled(indio_dev)) {

> +	/* Only changes while no transfer is in flight */
> +	scoped_guard(spinlock_irqsave, &priv->irq_busy_lock)
> +		buffered = priv->buffered;
> +
> +	if (!buffered) {

Strictly speaking you need to assign buffered to make any compiler not being
confused by the scoped_guard(), which cases IIRC we used to have the past.

...

> static int ads1298_buffer_postenable(struct iio_dev *indio_dev)

> -	return ads1298_write_cmd(priv, ADS1298_CMD_START);
> +	scoped_guard(spinlock_irqsave, &priv->irq_busy_lock)
> +		priv->buffered = true;
> +
> +	ret = ads1298_write_cmd(priv, ADS1298_CMD_START);
> +	if (ret) {
> +		scoped_guard(spinlock_irqsave, &priv->irq_busy_lock)
> +			priv->buffered = false;
> +	}
> +
> +	return ret;

Before write_cmd is called the buffered might have been triggered. Is it okay?

...

>  static int ads1298_buffer_predisable(struct iio_dev *indio_dev)
>  {
>  	struct ads1298_private *priv = iio_priv(indio_dev);
> +	int ret, ret2;
> +	int rate;
> +
> +	ret = ads1298_write_cmd(priv, ADS1298_CMD_STOP);
> +
> +	/*
> +	 * The conversion in progress completes and raises DRDY once more.
> +	 * Wait for it and for its transfer before the core tears down the
> +	 * scan.
> +	 */
> +	if (ads1298_get_samp_freq(priv, &rate) >= 0 && rate > 0)
> +		fsleep(2 * DIV_ROUND_UP(USEC_PER_SEC, rate));
> +	ret2 = ads1298_wait_rdata_idle(priv);
>  
> -	return ads1298_write_cmd(priv, ADS1298_CMD_STOP);
> +	scoped_guard(spinlock_irqsave, &priv->irq_busy_lock)
> +		priv->buffered = false;
> +
> +	return ret ?: ret2;
>  }

The code lacks of comments on why in all error cases (either first one, or
second, or both are triggered) it's fine to continue. And what will be the
meaning of the returned value in all three cases?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/6] iio: adc: ti-ads1298: fix power-up and clock handling

[Andy Shevchenko]

On Sat, Oct 24, 2026 at 08:13:31PM -0300, Stephan Costa Barros wrote:
> Fix three issues with the power-up sequence and the device clock:
> 
> - The datasheet powers up the supplies first, then the clock, then
>   waits t_POR = 2^18 clock cycles (128 ms at 2.048 MHz) before the
>   reset. The driver enabled an external clock and the vref supply
>   before AVDD, reset right away, and did not wait for the 150 ms
>   start-up of the internal reference. Follow the datasheet. Probe takes
>   about 280 ms longer.
> 
> - The binding describes one optional clock and no clock-names, but the
>   driver asked for the clock named "clk". With a device tree that
>   follows the binding, the lookup fails, the optional getter returns
>   NULL, and a clock given in the device tree is never enabled, while the
>   sampling frequency is computed from the nominal 2.048 MHz. Ask for the
>   first clock of the node; a device tree with clock-names = "clk" still
>   gets the same clock.
> 
> - A command takes 4 cycles of the device clock to decode, the minimum
>   reset pulse is 2 cycles, and the reset takes 18 more. These were
>   converted with the nominal 2.048 MHz, so a command ends after a fixed
>   2 us, short of 4 cycles with an external clock below 2 MHz, which the
>   ADS129x allows down to 1.94 MHz. Convert them with the rate of the
>   clock in use. At 2.048 MHz the delays do not change.
> 
> The clock lookup was seen to fail on a Raspberry Pi 4, with a
> fixed-clock in the device tree. The power-up order was found by review
> and reproduced on an emulated device only. The decode time was found
> with an emulated device; it is not seen on hardware running at
> 2.048 MHz.

...

> +	clk_rate = priv->clk ? clk_get_rate(priv->clk) : 0;

Why this conditional?

> +	if (!clk_rate)
> +		clk_rate = ADS1298_CLK_RATE_HZ;
> +	priv->clk_rate = clk_rate;

...

> +	/* Power-on reset time, counted from when the clock is valid */
> +	msleep(DIV_ROUND_UP(ADS1298_TPOR_CLOCKS * MILLI, clk_rate));

Do you mean MSEC_PER_SEC?

...

> +	priv->rdata_xfer.delay.value = ads1298_clocks_to_usecs(priv,
> +							       ADS1298_DECODE_CLOCKS);

Indent it as

	priv->rdata_xfer.delay.value =
		ads1298_clocks_to_usecs(priv, ADS1298_DECODE_CLOCKS);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/6] iio: adc: ti-ads1298: prepare for more chip families

[Andy Shevchenko]

On Sat, Oct 24, 2026 at 08:13:33PM -0300, Stephan Costa Barros wrote:
> The ADS1299 has the register map of the ADS129x, with other gains, data
> rates, reference and ID layout. To support it in this driver, collect
> what differs between families in a chip info structure, given as match
> data. The private structure has carried an unused chip_info pointer
> since the driver was added.
> 
> The chip info holds:
> - the regmap configuration and the PGA gain table;
> - the data rate rules: the base shift, whether CONFIG1 has an HR bit,
>   and the oversampling ratio at DR = 0;
> - the internal reference, whether CONFIG3 has the VREF_4V bit, and the
>   reserved bits of CONFIG3 that must be written as 1;
> - a callback that checks the ID register and sets the channel count and
>   the name, as the ID layouts differ; bit 3 alone tells them apart.

...

> +#define ADS1298_VREF_4V_MV			4000

_mV

...

> +/* What differs between the chip families the driver supports */
> +struct ads1298_chip_info {
> +	/* Checks the ID register, and sets the channel count and the name */
> +	int (*parse_id)(struct iio_dev *indio_dev, unsigned int id);
> +	const struct regmap_config *regmap_config;
> +	/* PGA gain for each value of the GAIN field of CHnSET */
> +	const u8 *gains;
> +	unsigned int num_gains;
> +	/*
> +	 * The data rate is f_CLK >> (dr_shift + DR). With an HR bit in
> +	 * CONFIG1, dr_shift is that of the high-resolution mode, and the
> +	 * low-power mode shifts one more.
> +	 */
> +	unsigned int dr_shift;
> +	bool has_hr;
> +	/* Oversampling ratio at DR = 0 */
> +	unsigned int osr_base;
> +	/* Internal reference, in millivolts */
> +	unsigned int vref_mv;

_mV

> +	/* CONFIG3 has the VREF_4V bit, which selects a 4 V internal reference */
> +	bool has_vref_4v;
> +	/* Reserved bits of CONFIG3 that must be written as 1 */
> +	unsigned int config3_reserved;
> +};

Run `pahole` and act accordingly.

...

>  		*val = ret / MILLI; /* Convert to millivolts */

At some point this should use (MICRO / MILLI) instead of MILLI.

...

>  		ret = regmap_read(priv->regmap, ADS1298_REG_CONFIG3, &regval);
>  		if (ret)
>  			return ret;
>  
>  		/* Reference in millivolts */
> -		*val = regval & ADS1298_MASK_CONFIG3_VREF_4V ? 4000 : 2400;
> +		*val = regval & ADS1298_MASK_CONFIG3_VREF_4V ?
> +		       ADS1298_VREF_4V_MV : info->vref_mv;

Can't we assign the default and always use info->vref_mV here?

...

> +	priv->chip_info = spi_get_device_match_data(spi);
> +	if (!priv->chip_info)
> +		return -ENODEV;

-ENODATA

...

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

Leave it a single line.

>  	if (IS_ERR(priv->regmap))

...

>  static const struct spi_device_id ads1298_id[] = {
> -	{ .name = "ads1298" },
> +	{ .name = "ads1298", .driver_data = (kernel_ulong_t)&ads1298_chip_info },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(spi, ads1298_id);
>  
>  static const struct of_device_id ads1298_of_table[] = {
> -	{ .compatible = "ti,ads1298" },
> +	{ .compatible = "ti,ads1298", .data = &ads1298_chip_info },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, ads1298_of_table);

I think this patch is overloaded. You need to split it to a series (to make
chip_info a separate preparatory change, for example).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 6/6] iio: adc: ti-ads1298: add ADS1299 support

[Andy Shevchenko]

On Sat, Oct 24, 2026 at 08:13:35PM -0300, Stephan Costa Barros wrote:
> The ADS1299, ADS1299-6 and ADS1299-4 are 8, 6 and 4-channel, 24-bit
> biopotential ADCs for EEG. They share the register map, the commands
> and the data format of the ADS129x, and differ in:
> 
> - the PGA gains, 1, 2, 4, 6, 8, 12 and 24;
> - the data rates, f_CLK / 2^(7 + DR) with no HR bit, from 16 kSPS to
>   250 SPS, and the oversampling ratio, 64 * 2^DR;
> - the internal reference, a fixed 4.5 V, and the reserved bits 6:5 of
>   CONFIG3;
> - the ID register, which holds the channel count in bits 1:0;
> - the register map, which ends at CONFIG4;
> - the inputs, which are shorted out of reset.
> 
> Add the chip info for the ADS1299, and connect the inputs of every
> channel at init, which changes nothing on the ADS129x, whose inputs are
> already connected out of reset. The channel count is read from the ID
> register, so ti,ads1299 also serves the ADS1299-4 and ADS1299-6 through
> their fallback.
> 
> For both families, enable the DVDD supply when the device tree gives
> one, and hold PWDN asserted, like RESET, until the supplies and the
> clock are up. Nothing changes for a device tree that has neither.

...

>  config TI_ADS1298
> -	tristate "Texas Instruments ADS1298"
> +	tristate "Texas Instruments ADS1298 and ADS1299"
>  	depends on SPI
>  	select IIO_BUFFER
>  	select IIO_KFIFO_BUF
>  	select REGMAP
>  	help
> -	  If you say yes here you get support for Texas Instruments ADS1298
> -	  medical ADC chips
> +	  If you say yes here you get support for Texas Instruments ADS1294,
> +	  ADS1296, ADS1298 and ADS1299 medical ADC chips, and their variants

Make this a list, see how other drivers (recently updated to support more HW)
do it.


>  	  This driver can also be built as a module. If so, the module will be
>  	  called ti-ads1298.

...

>  // SPDX-License-Identifier: GPL-2.0
> -/* TI ADS1298 chip family driver
> +/* TI ADS1298 and ADS1299 chip family driver
>   * Copyright (C) 2023 - 2024 Topic Embedded Products
> + * Copyright (C) 2026 Stephan Costa Barros <stephanbrrs8@gmail.com>
>   */

Fix multi-line comment style, make the text of tristate in Kconfig, this one,
and in the MODULE_DESCRIPTION() the same.

...

> +	/* TI sells the 8-channel part as plain ADS1299 */
> +	if (indio_dev->num_channels == ADS1298_MAX_CHANNELS)
> +		indio_dev->name = "ads1299";
> +	else
> +		indio_dev->name = devm_kasprintf(dev, GFP_KERNEL, "ads1299-%u",
> +						 indio_dev->num_channels);

Why do we need this? Can't we derive it from IIO ABI?

> +	if (!indio_dev->name)
> +		return -ENOMEM;

...

>  static int ads1298_init(struct iio_dev *indio_dev)
>  {
>  	struct ads1298_private *priv = iio_priv(indio_dev);
>  	unsigned int val;
> +	unsigned int i;

It's not used outside of the loop, make it local to that for-loop.

>  	int ret;
>  
>  	/* Device initializes into RDATAC mode, which we don't want */

>  	if (!priv->reg_vref)
>  		msleep(ADS1298_REF_START_UP_MS);
>  
> +	/* The ADS1299 comes out of reset with its inputs shorted */
> +	for (i = 0; i < indio_dev->num_channels; i++) {
> +		ret = regmap_update_bits(priv->regmap, ADS1298_REG_CHnSET(i),
> +					 ADS1298_MASK_CH_MUX,
> +					 FIELD_PREP(ADS1298_MASK_CH_MUX,
> +						    ADS1298_MUX_NORMAL));
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return 0;
>  }

-- 
With Best Regards,
Andy Shevchenko