Re: [PATCH 2/2] iio: adc: ti-ads1299: add driver for TI ADS1299

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] iio: adc: ti-ads1299: unsynchronized async rdata_msg on stop and timeout
- [High] iio: adc: ti-ads1299: concurrent cmd_buffer access and shared DMA cacheline
- [Medium] iio: adc: ti-ads1299: incorrect power-up and reference start-up sequence in ads1299_probe()
- [Medium] iio: adc: ti-ads1299: unchecked spi_async() and rdata_msg.status errors
--

commit 3f1066cf1b7c3e1e9933416e0c037a37dcbe64ed
Author: Stephan Costa Barros <stephanbrrs8@gmail.com>

iio: adc: ti-ads1299: add driver for TI ADS1299

This patch adds an IIO ADC driver for the Texas Instruments ADS1299,
ADS1299-6, and ADS1299-4 24-bit biopotential analog front-ends over SPI.
It supports on-demand single-shot channel reads and buffered capture driven
by the DRDY interrupt.

> diff --git a/drivers/iio/adc/ti-ads1299.c b/drivers/iio/adc/ti-ads1299.c
> new file mode 100644
> index 0000000000000..f817a86cca5c9
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads1299.c
> @@ -0,0 +1,897 @@

[ ... ]

> +struct ads1299_private {
> +	struct spi_device *spi;
> +	struct regmap *regmap;
> +	struct completion completion;
> +	struct spi_transfer rdata_xfer;
> +	struct spi_message rdata_msg;
> +	spinlock_t irq_busy_lock; /* Handshake between SPI and DRDY irqs */
> +	/*
> +	 * 0 = idle, 1 = transfer in progress, 2 = a sample became due during
> +	 * it; any more were lost.
> +	 */
> +	unsigned int rdata_xfer_busy;
> +	unsigned int num_adc_channels;
> +	int samp_freq_avail[ADS1299_NUM_DR];
> +	int scale_avail[ARRAY_SIZE(ads1299_pga_gain)][2];
> +
> +	struct {
> +		u32 chan[ADS1299_MAX_CHANNELS];
> +		aligned_s64 timestamp;
> +	} scan;
> +
> +	/* For synchronous SPI exchanges (read/write registers) */
> +	u8 cmd_buffer[ADS1299_SPI_CMD_BUFFER_SIZE] __aligned(IIO_DMA_MINALIGN);
> +
> +	u8 rx_buffer[ADS1299_SPI_RDATA_BUFFER_SIZE_MAX];
> +	/* Contains the RDATA command and zeroes to clock out */
> +	u8 tx_buffer[ADS1299_SPI_RDATA_BUFFER_SIZE_MAX];
> +};

[ ... ]

> +static int ads1299_write_cmd(struct ads1299_private *priv, u8 command)
> +{
> +	struct spi_transfer xfer = {
> +		.tx_buf = priv->cmd_buffer,
> +		.rx_buf = priv->cmd_buffer,
> +		.len = 1,
> +		.speed_hz = ADS1299_SPI_BUS_SPEED_SLOW,
> +		.delay = {
> +			.value = 2,
> +			.unit = SPI_DELAY_UNIT_USECS,
> +		},
> +	};
> +
> +	priv->cmd_buffer[0] = command;
> +
> +	return spi_sync_transfer(priv->spi, &xfer, 1);
> +}

[Severity: High]
Can priv->cmd_buffer be corrupted by concurrent accesses, or share a DMA
cacheline with priv->rx_buffer and priv->tx_buffer?

In ads1299_reg_read() and ads1299_reg_write(), priv->cmd_buffer is used as
both tx_buf and rx_buf under priv->regmap's internal lock. However,
ads1299_write_cmd() mutates and transfers priv->cmd_buffer under the IIO
device mode lock (via ads1299_read_one(), ads1299_buffer_postenable(), and
ads1299_buffer_predisable()) without acquiring the regmap lock.

Because ads1299_read_raw() (for IIO_CHAN_INFO_SCALE and
IIO_CHAN_INFO_SAMP_FREQ, where ADS1299_REG_CONFIG1 is uncached on first
read) and ads1299_reg_access() invoke regmap_read() and regmap_write()
without iio_device_claim_direct(), those paths can run concurrently with
ads1299_write_cmd() under disjoint locks and overwrite priv->cmd_buffer
during an active SPI transfer.

In addition, in struct ads1299_private, only cmd_buffer has
__aligned(IIO_DMA_MINALIGN), while rx_buffer and tx_buffer immediately
follow it in the same cacheline. On non-coherent platforms, can CPU or DMA
access to cmd_buffer (for example, during ads1299_buffer_predisable() or
sysfs/debugfs register reads) corrupt an in-flight rdata_msg DMA transfer
using rx_buffer and tx_buffer?

> +
> +static int ads1299_read_one(struct ads1299_private *priv, int chan_index)
> +{
> +	int ret;
> +
> +	/* Enable the channel */
> +	ret = regmap_update_bits(priv->regmap, ADS1299_REG_CHnSET(chan_index),
> +				 ADS1299_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, ADS1299_REG_CONFIG4,
> +				 ADS1299_MASK_CONFIG4_SINGLE_SHOT,
> +				 ADS1299_MASK_CONFIG4_SINGLE_SHOT);
> +	if (ret)
> +		return ret;
> +
> +	reinit_completion(&priv->completion);
> +
> +	ret = ads1299_write_cmd(priv, ADS1299_CMD_START);
> +	if (ret < 0) {
> +		dev_err(&priv->spi->dev, "CMD_START error: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* A conversion takes 4 ms at the lowest rate, plus bus latency */
> +	ret = wait_for_completion_timeout(&priv->completion, msecs_to_jiffies(100));
> +	if (!ret)
> +		return -ETIMEDOUT;
> +
> +	return 0;
> +}

[ ... ]

> +static void ads1299_rdata_release_busy_or_restart(struct ads1299_private *priv)
> +{
> +	guard(spinlock_irqsave)(&priv->irq_busy_lock);
> +
> +	if (priv->rdata_xfer_busy > 1) {
> +		/* A sample became due during the transfer: fetch it now */
> +		spi_async(priv->spi, &priv->rdata_msg);
> +		priv->rdata_xfer_busy = 1;
> +	} else {
> +		priv->rdata_xfer_busy = 0;
> +	}
> +}
> +
> +/* Called from SPI completion interrupt handler */
> +static void ads1299_rdata_complete(void *context)
> +{
> +	struct iio_dev *indio_dev = context;
> +	struct ads1299_private *priv = iio_priv(indio_dev);
> +	u32 *bounce = priv->scan.chan;
> +	int scan_index;
> +
> +	if (!iio_buffer_enabled(indio_dev)) {
> +		/*
> +		 * For a single transfer we are kept in direct mode until
> +		 * completion, avoiding a race with buffered IO.
> +		 */
> +		ads1299_rdata_unmark_busy(priv);
> +		complete(&priv->completion);
> +		return;
> +	}
> +
> +	iio_for_each_active_channel(indio_dev, scan_index) {
> +		const struct iio_chan_spec *scan_chan =
> +					&indio_dev->channels[scan_index];
> +
> +		if (scan_chan->type == IIO_TIMESTAMP)
> +			continue;
> +
> +		*bounce++ = get_unaligned_be24(priv->rx_buffer +
> +					       scan_chan->address);
> +	}
> +
> +	/* rx_buffer can be overwritten from this point on */
> +	ads1299_rdata_release_busy_or_restart(priv);
> +
> +	iio_push_to_buffers_with_ts(indio_dev, &priv->scan, sizeof(priv->scan),
> +				    iio_get_time_ns(indio_dev));
> +}
> +
> +static irqreturn_t ads1299_interrupt(int irq, void *dev_id)
> +{
> +	struct iio_dev *indio_dev = dev_id;
> +	struct ads1299_private *priv = iio_priv(indio_dev);
> +	unsigned int wasbusy;
> +
> +	guard(spinlock_irqsave)(&priv->irq_busy_lock);
> +
> +	wasbusy = priv->rdata_xfer_busy++;
> +	/* When no SPI transfer is in transit, start one now */
> +	if (!wasbusy)
> +		spi_async(priv->spi, &priv->rdata_msg);
> +
> +	return IRQ_HANDLED;
> +}

[Severity: Medium]
What happens if spi_async() fails in ads1299_interrupt() or
ads1299_rdata_release_busy_or_restart(), or if priv->rdata_msg completes
with an error?

In both functions, priv->rdata_xfer_busy is set to a non-zero value without
checking the return value of spi_async(). If spi_async() returns an error,
ads1299_rdata_complete() will never run, leaving priv->rdata_xfer_busy
non-zero so subsequent DRDY interrupts see wasbusy != 0 and stop starting
transfers.

Also, in ads1299_rdata_complete(), priv->rdata_msg.status is not checked
before completing priv->completion or pushing priv->rx_buffer data via
iio_push_to_buffers_with_ts(). Would a failed async SPI transfer report
stale or partial rx_buffer contents as valid ADC samples?

> +
> +static int ads1299_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct ads1299_private *priv = iio_priv(indio_dev);
> +	int ret;
> +
> +	/* Disable single-shot mode */
> +	ret = regmap_update_bits(priv->regmap, ADS1299_REG_CONFIG4,
> +				 ADS1299_MASK_CONFIG4_SINGLE_SHOT, 0);
> +	if (ret)
> +		return ret;
> +
> +	return ads1299_write_cmd(priv, ADS1299_CMD_START);
> +}
> +
> +static int ads1299_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	struct ads1299_private *priv = iio_priv(indio_dev);
> +
> +	return ads1299_write_cmd(priv, ADS1299_CMD_STOP);
> +}

[Severity: High]
Does ads1299_buffer_predisable() race with in-flight DRDY interrupts and
asynchronous priv->rdata_msg transfers?

When disabling the buffer, iio_disable_buffers() calls
ads1299_buffer_predisable(), which sends ADS1299_CMD_STOP and returns
immediately without synchronizing spi->irq, preventing
ads1299_rdata_release_busy_or_restart() from re-submitting priv->rdata_msg,
or waiting for in-flight priv->rdata_msg transfers to complete.

Right after predisable returns, iio_disable_buffers() frees
indio_dev->active_scan_mask and sets it to NULL before updating currentmode
to INDIO_DIRECT_MODE. If ads1299_rdata_complete() runs concurrently during
this window, iio_buffer_enabled(indio_dev) is still true while
iio_for_each_active_channel() dereferences indio_dev->active_scan_mask. If
it completes slightly later after currentmode becomes INDIO_DIRECT_MODE, it
will signal priv->completion and leave stale completion state for a
subsequent ads1299_read_one().

A similar issue exists if wait_for_completion_timeout() times out in
ads1299_read_one() while priv->rdata_msg is still in flight:
ads1299_read_raw() releases direct mode without waiting for priv->rdata_msg,
and a subsequent ads1299_update_scan_mode() calls
ads1299_rdata_unmark_busy() to reset priv->rdata_xfer_busy = 0, allowing the
next DRDY interrupt to pass an already-queued priv->rdata_msg to
spi_async(). Likewise, on driver unbind, devm tears down indio_dev without
quiescing the IRQ and flushing in-flight priv->rdata_msg transfers first.

[ ... ]

> +static int ads1299_init(struct iio_dev *indio_dev, bool internal_vref)
> +{

[ ... ]

> +	/* Without the reference buffer every channel reads zero */
> +	val = ADS1299_CONFIG3_RESERVED_VALUE;
> +	if (internal_vref)
> +		val |= ADS1299_MASK_CONFIG3_PWR_REFBUF;
> +	ret = regmap_write(priv->regmap, ADS1299_REG_CONFIG3, val);
> +	if (ret)
> +		return ret;
> +
> +	/* Out of reset the inputs are shorted: connect them */
> +	for (i = 0; i < priv->num_adc_channels; i++) {
> +		ret = regmap_update_bits(priv->regmap, ADS1299_REG_CHnSET(i),
> +					 ADS1299_MASK_CH_MUX,
> +					 FIELD_PREP(ADS1299_MASK_CH_MUX,
> +						    ADS1299_MUX_NORMAL));
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

[ ... ]

> +static int ads1299_probe(struct spi_device *spi)
> +{

[ ... ]

> +	/* 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))
> +		return dev_err_probe(dev, PTR_ERR(reset_gpio),
> +				     "Cannot get reset GPIO\n");
> +
> +	powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> +						 GPIOD_OUT_LOW);
> +	if (IS_ERR(powerdown_gpio))
> +		return dev_err_probe(dev, PTR_ERR(powerdown_gpio),
> +				     "Cannot get powerdown GPIO\n");
> +
> +	/* VREF can be supplied externally, otherwise use the internal 4.5 V */
> +	vref_uv = devm_regulator_get_enable_read_voltage(dev, "vref");
> +	if (vref_uv < 0 && vref_uv != -ENODEV)
> +		return dev_err_probe(dev, vref_uv, "Failed to get vref voltage\n");
> +
> +	internal_vref = vref_uv == -ENODEV;
> +	if (internal_vref)
> +		vref_uv = ADS1299_INTERNAL_VREF_UV;
> +
> +	clk = devm_clk_get_optional_enabled(dev, "clk");
> +	if (IS_ERR(clk))
> +		return dev_err_probe(dev, PTR_ERR(clk), "Failed to get clk\n");

[ ... ]

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

[ ... ]

> +	/* Out of power-down: wait for the oscillator and the reference */
> +	if (powerdown_gpio)
> +		msleep(ADS1299_START_UP_MS);
> +
> +	if (reset_gpio) {
> +		/* Minimum reset pulse width is 2 clock cycles */
> +		fsleep(ADS1299_CLOCKS_TO_USECS(2));
> +		gpiod_set_value_cansleep(reset_gpio, 0);
> +	} else {
> +		ret = ads1299_write_cmd(priv, ADS1299_CMD_RESET);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "RESET failed\n");
> +	}
> +	/* Wait 18 clock cycles for the reset to complete */
> +	fsleep(ADS1299_CLOCKS_TO_USECS(18));
> +
> +	ret = ads1299_init(indio_dev, internal_vref);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Init failed\n");

[Severity: Medium]
Is the power-up and reference start-up sequence in ads1299_probe() and
ads1299_init() ordered as intended?

First, powerdown_gpio is requested with GPIOD_OUT_LOW (deasserted) and vref
and clk are enabled before the avdd and dvdd regulators are enabled.

Second, msleep(ADS1299_START_UP_MS) is only executed when powerdown_gpio is
present. When the optional powerdown GPIO is omitted, the driver skips the
150 ms power-on start-up delay after enabling avdd and dvdd before releasing
reset or issuing ADS1299_CMD_RESET.

Third, out of reset, ADS1299_REG_CONFIG3 defaults to having the internal
reference buffer powered down, and ADS1299_MASK_CONFIG3_PWR_REFBUF is only
written later in ads1299_init(). Because msleep(ADS1299_START_UP_MS) runs
before reset is released (or before ADS1299_CMD_RESET) and before
ADS1299_MASK_CONFIG3_PWR_REFBUF is enabled, the internal reference buffer is
not given time to settle after being powered up.

[ ... ]

—
🤖 Generated by Linux Kernel Patch Reviewer (Model: gemini-3.1-pro-preview)
Prompt Version: 969354535492578f3c622ebd4ebddaf43de71bd4
Tools: git-2.53.0-1.fc43.x86_64
Date: 2026-10-19 06:58:26 UTC
Duration (ms): 229238
Tokens (in/out): 167213 / 11103
Prompts Used: build.md, callstack.md, cleanup.md, gpio.md, inline-template.md, kconfig.md, mm-alloc.md, of.md, subsystem/locking.md, technical-patterns.md

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019001247.483005-1-stephanbrrs8@gmail.com?part=2