Skip to contentSkip to conversation

iio: adc: add TI ADS1299

PATCH In review Stephan Costa Barros 6 participants 10 messages Oct 19 – Oct 24

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover iio: adc: add TI ADS1299

This series adds a driver for the TI ADS1299 family (ADS1299, ADS1299-6 and ADS1299-4): 24-bit, simultaneous-sampling biopotential front-ends for EEG, on SPI.

This first version keeps to the generic IIO ABI: raw reads, the gain of each channel's PGA through its scale, the sampling frequency, and a kfifo buffer fed from the DRDY interrupt with a software timestamp.

The part has more, which I would like to add in follow-up series, each with its own ABI discussion: SRB1 and SRB2 reference routing, the bias drive amplifier, lead-off detection (through IIO_EV_TYPE_FAULT and IIO_EV_DIR_FAULT_OPENWIRE), the internal test signal, the temperature sensor and supply monitors, GPIO1 to GPIO4 as a gpiochip, the CLK pin as a clock output, and daisy chains. The binding already describes the hardware for those.

A question on structure. The driver started from ti-ads1298.c, and in this subset the two are close. What differs here is the PGA gain table (1 to 24 against 1 to 12 with 3), the data rates (no high-resolution mode), the fixed 4.5 V internal reference, the ID register layout and the 4 and 6 channel variants; above 0x14 the register maps also part (MISC1 and MISC2 here, PACE, RESP and WCT on the ADS1298). Most of what would follow, SRB1 and SRB2, the bias drive and lead-off on every channel, is specific to the ADS1299, so I kept a separate driver. If you would rather have the ADS1299 as a variant in ti-ads1298, I can rework it that way.

Tested on a Raspberry Pi 4B running 6.18.50 (Raspberry Pi OS), with one ADS1299 and DRDY on a GPIO edge interrupt, the driver built out of tree from this patch:
- every value in in_voltage_scale_available and sampling_frequency_available written and read back, and values outside the lists refused;
- the internal DC test signal, routed through debugfs register access, read at gain 24 against shorted inputs: -0.08 % to -0.41 % from VREF / 2400 across the eight channels, as with the out-of-tree driver on the same board;
- a 10 s capture at 250 SPS: 2497 samples for 2497 DRDY interrupts, no gaps in the timestamps, and buffered values matching direct reads.
So on 6.18 the driver was run and tested as above; on the IIO tree below, the same source, unchanged, was compiled but not run.

The series is based on the testing branch of Jonathan's IIO tree (git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git).

Stephan Costa Barros (2):
dt-bindings: iio: adc: add TI ADS1299
iio: adc: ti-ads1299: add driver for TI ADS1299

 .../bindings/iio/adc/ti,ads1299.yaml          | 221 +++++
 MAINTAINERS                                   |   7 +
 drivers/iio/adc/Kconfig                       |  14 +
 drivers/iio/adc/Makefile                      |   1 +
 drivers/iio/adc/ti-ads1299.c                  | 897 ++++++++++++++++++
 5 files changed, 1140 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1299.yaml
 create mode 100644 drivers/iio/adc/ti-ads1299.c

base-commit: 19c5db3507633edc21b4172d99f573c2dfbf42b1

Signature · 1 line
2.43.0
#1 lore raw

PATCH 1/2 dt-bindings: iio: adc: add TI ADS1299

The ADS1299 is an 8-channel, 24-bit, simultaneous-sampling biopotential (EEG) analog front-end with an SPI interface. The ADS1299-6 and ADS1299-4 are the same part with six and four channels.

One converter can drive its clock out of its CLK pin to the others (#clock-cells), and converters sharing a chip select in daisy-chain mode are described as one node with #daisy-chained-devices.

---
 .../bindings/iio/adc/ti,ads1299.yaml          | 221 ++++++++++++++++++
 MAINTAINERS                                   |   6 +
 2 files changed, 227 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1299.yaml
Documentation/devicetree/bindings/iio/adc/ti,ads1299.yaml new file +221 -0
new file mode 100644
index 000000000..a8966cd07
@@ -0,0 +1,221 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/ti,ads1299.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Texas Instruments ADS1299 biopotential analog front-end
+
+maintainers:
+  - Stephan Costa Barros <[email protected]>
+
+description: |
+  Texas Instruments ADS1299 family: low-noise, 24-bit, simultaneous-sampling
+  biopotential (EEG) analog front-ends with an SPI interface. The ADS1299 has
+  eight channels, the ADS1299-6 six and the ADS1299-4 four.
+
+  Each channel has its own programmable gain amplifier and a multiplexer that
+  can select the electrode inputs, a shorted input, the internal test signal,
+  the supply or the temperature sensor. The part also provides lead-off
+  detection and a bias drive amplifier for the driven reference electrode.
+
+  The converter signals a completed conversion on its DRDY output and is read
+  over SPI in mode 1 (CPOL=0, CPHA=1).
+
+  Several converters can share one SPI bus in two ways. In cascaded mode each
+  has its own chip select and its own node; one of them can drive its internal
+  oscillator out of its CLK pin (#clock-cells) for the others to use as their
+  external clock. In daisy-chain mode they share the chip select and DIN, the
+  DOUT of each one feeds the DAISY_IN of the one before it, and the chain is a
+  single node with #daisy-chained-devices. A daisy chain shares DIN, so every
+  register write reaches every device: the devices run with one configuration
+  and from one external clock, and the bias drive amplifier cannot be used.
+
+  https://www.ti.com/lit/gpn/ads1299
+
+properties:
+  compatible:
+    enum:
+      - ti,ads1299
+      - ti,ads1299-4
+      - ti,ads1299-6
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+    description:
+      The DRDY output of the converter, asserted low when a conversion is
+      ready to be read.
+
+  reset-gpios:
+    maxItems: 1
+    description: The active-low RESET input of the converter.
+
+  powerdown-gpios:
+    maxItems: 1
+    description:
+      The active-low PWDN input.
+
+  clocks:
+    maxItems: 1
+    description:
+      External clock on the CLK pin, used when the CLKSEL pin selects an
+      external clock. Omit when the part runs from its internal oscillator.
+
+  clock-names:
+    const: clk
+
+  '#clock-cells':
+    const: 0
+    description:
+      The CLK pin as a clock output, for a part whose CLKSEL pin selects the
+      internal oscillator.
+
+  clock-output-names:
+    maxItems: 1
+
+  gpio-controller:
+    description:
+      GPIO1 to GPIO4 are wired for use as general-purpose pins. Omit when they
+      are tied to DGND, as the datasheet asks of unused ones, so that no pin
+      tied to ground can be driven as an output.
+
+  '#gpio-cells':
+    const: 2
+
+  gpio-line-names:
+    minItems: 1
+    maxItems: 4
+
+  '#daisy-chained-devices':
+    description:
+      Number of converters in the daisy chain, all of the type the compatible
+      names. Their channels are reported in chain order, the device whose DOUT
+      reaches the host first.
+    minimum: 1
+    maximum: 8
+    default: 1
+
+  avdd-supply:
+    description: Analog supply.
+
+  dvdd-supply:
+    description: Digital supply.
+
+  vref-supply:
+    description:
+      External voltage reference on VREFP. Omit to use the internal 4.5 V
+      reference.
+
+  biasref-supply:
+    description:
+      External voltage on the BIASREF pin, the reference of the bias drive
+      amplifier. Omit to have the part generate (AVDD + AVSS) / 2.
+
+  spi-cpha: true
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - avdd-supply
+  - spi-cpha
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+  - if:
+      required:
+        - '#clock-cells'
+    then:
+      properties:
+        clocks: false
+  - if:
+      required:
+        - '#daisy-chained-devices'
+      properties:
+        '#daisy-chained-devices':
+          minimum: 2
+    then:
+      required:
+        - clocks
+      properties:
+        '#clock-cells': false
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@0 {
+            compatible = "ti,ads1299";
+            reg = <0>;
+            spi-max-frequency = <4000000>;
+            spi-cpha;
+            interrupt-parent = <&gpio1>;
+            interrupts = <5 IRQ_TYPE_EDGE_FALLING>;
+            reset-gpios = <&gpio1 2 GPIO_ACTIVE_LOW>;
+            powerdown-gpios = <&gpio1 3 GPIO_ACTIVE_LOW>;
+            avdd-supply = <&reg_avdd>;
+            dvdd-supply = <&reg_dvdd>;
+            gpio-controller;
+            #gpio-cells = <2>;
+            gpio-line-names = "ADC_GPIO1", "ADC_GPIO2", "", "STATUS_LED";
+        };
+    };
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    /* Cascaded: the first converter clocks the second */
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc0: adc@0 {
+            compatible = "ti,ads1299";
+            reg = <0>;
+            spi-cpha;
+            interrupt-parent = <&gpio1>;
+            interrupts = <5 IRQ_TYPE_EDGE_FALLING>;
+            avdd-supply = <&reg_avdd>;
+            #clock-cells = <0>;
+        };
+
+        adc@1 {
+            compatible = "ti,ads1299";
+            reg = <1>;
+            spi-cpha;
+            interrupt-parent = <&gpio1>;
+            interrupts = <6 IRQ_TYPE_EDGE_FALLING>;
+            avdd-supply = <&reg_avdd>;
+            clocks = <&adc0>;
+            clock-names = "clk";
+        };
+    };
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    /* Daisy chain: three converters, 24 channels, one chip select */
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@0 {
+            compatible = "ti,ads1299";
+            reg = <0>;
+            spi-cpha;
+            interrupt-parent = <&gpio1>;
+            interrupts = <5 IRQ_TYPE_EDGE_FALLING>;
+            avdd-supply = <&reg_avdd>;
+            clocks = <&osc_2048k>;
+            clock-names = "clk";
+            #daisy-chained-devices = <3>;
+        };
+    };
MAINTAINERS +6 -0
index 52024397d..b4a8e4573 100644
@@ -27370,6 +27370,12 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/iio/adc/ti,ads1018.yaml
 F:	drivers/iio/adc/ti-ads1018.c
 
+TI ADS1299 ADC DRIVER
+M:	Stephan Costa Barros <[email protected]>
+L:	[email protected]
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/adc/ti,ads1299.yaml
+
 TI ADS7924 ADC DRIVER
 M:	Hugo Villeneuve <[email protected]>
 L:	[email protected]
Signature · 1 line
2.43.0
#2 lore raw

Re: [PATCH 1/2] dt-bindings: iio: adc: add TI ADS1299

Quoted 50 lines · Stephan Costa Barros

The ADS1299 is an 8-channel, 24-bit, simultaneous-sampling biopotential (EEG) analog front-end with an SPI interface. The ADS1299-6 and ADS1299-4 are the same part with six and four channels.

One converter can drive its clock out of its CLK pin to the others (#clock-cells), and converters sharing a chip select in daisy-chain mode are described as one node with #daisy-chained-devices.

---
 .../bindings/iio/adc/ti,ads1299.yaml          | 221 ++++++++++++++++++
 MAINTAINERS                                   |   6 +
 2 files changed, 227 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1299.yaml
Documentation/devicetree/bindings/iio/adc/ti,ads1299.yaml new file +29 -0
new file mode 100644
index 000000000..a8966cd07
@@ -0,0 +1,221 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/ti,ads1299.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Texas Instruments ADS1299 biopotential analog front-end
+
+maintainers:
+  - Stephan Costa Barros <[email protected]>
+
+description: |
+  Texas Instruments ADS1299 family: low-noise, 24-bit, simultaneous-sampling
+  biopotential (EEG) analog front-ends with an SPI interface. The ADS1299 has
+  eight channels, the ADS1299-6 six and the ADS1299-4 four.
+
+  Each channel has its own programmable gain amplifier and a multiplexer that
+  can select the electrode inputs, a shorted input, the internal test signal,
+  the supply or the temperature sensor. The part also provides lead-off
+  detection and a bias drive amplifier for the driven reference electrode.
+
+  The converter signals a completed conversion on its DRDY output and is read
+  over SPI in mode 1 (CPOL=0, CPHA=1).
+
+  Several converters can share one SPI bus in two ways. In cascaded mode each
+  has its own chip select and its own node; one of them can drive its internal
+  oscillator out of its CLK pin (#clock-cells) for the others to use as their
+  external clock. In daisy-chain mode they share the chip select and DIN, the
+  DOUT of each one feeds the DAISY_IN of the one before it, and the chain is a

Documentation/devicetree/bindings/iio/adc/ti,ads1299.yaml line 39

Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/ti,ads1299.yaml new file +10 -0
+  single node with #daisy-chained-devices. A daisy chain shares DIN, so every
+  register write reaches every device: the devices run with one configuration
+  and from one external clock, and the bias drive amplifier cannot be used.
+
+  https://www.ti.com/lit/gpn/ads1299
+
+properties:
+  compatible:
+    enum:
+      - ti,ads1299

This should be then "-8" variant?

Quoted 19 lines
Documentation/devicetree/bindings/iio/adc/ti,ads1299.yaml new file +19 -0
+      - ti,ads1299-4
+      - ti,ads1299-6
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+    description:
+      The DRDY output of the converter, asserted low when a conversion is
+      ready to be read.
+
+  reset-gpios:
+    maxItems: 1
+    description: The active-low RESET input of the converter.
+
+  powerdown-gpios:
+    maxItems: 1
+    description:

Documentation/devicetree/bindings/iio/adc/ti,ads1299.yaml line 68

Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/ti,ads1299.yaml new file +10 -0
+      The active-low PWDN input.
+
+  clocks:
+    maxItems: 1
+    description:
+      External clock on the CLK pin, used when the CLKSEL pin selects an
+      external clock. Omit when the part runs from its internal oscillator.
+
+  clock-names:
+    const: clk

Drop clock-names

Documentation/devicetree/bindings/iio/adc/ti,ads1299.yaml line 77

Quoted 9 lines
Documentation/devicetree/bindings/iio/adc/ti,ads1299.yaml new file +9 -0
+
+  '#clock-cells':
+    const: 0
+    description:
+      The CLK pin as a clock output, for a part whose CLKSEL pin selects the
+      internal oscillator.
+
+  clock-output-names:
+    maxItems: 1

Best regards,
Krzysztof

#3 lore raw

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

In reply to Stephan Costa Barros

Add a driver for the TI ADS1299, ADS1299-6 and ADS1299-4: low-noise 24-bit biopotential (EEG) analog front-ends with eight, six and four channels, on SPI. It is derived from ti-ads1298.c.

The driver reads each channel on demand, or all of them through a kfifo buffer fed from the DRDY interrupt. The gain of each channel's PGA is set through its scale, and the data rate through sampling_frequency.

The channel count is read from the ID register. It sets the length of each data read, so when it differs from the compatible the part found is used, with a warning.

The reference is the internal 4.5 V one unless a vref supply is given, and the clock the internal oscillator unless an external one is given. Daisy-chained converters are refused.

---
 MAINTAINERS                  |   1 +
 drivers/iio/adc/Kconfig      |  14 +
 drivers/iio/adc/Makefile     |   1 +
 drivers/iio/adc/ti-ads1299.c | 897 +++++++++++++++++++++++++++++++++++
 4 files changed, 913 insertions(+)
 create mode 100644 drivers/iio/adc/ti-ads1299.c
MAINTAINERS +1 -0
index b4a8e4573..29dcc2c2b 100644
@@ -27375,6 +27375,7 @@ M:	Stephan Costa Barros <[email protected]>
 L:	[email protected]
 S:	Maintained
 F:	Documentation/devicetree/bindings/iio/adc/ti,ads1299.yaml
+F:	drivers/iio/adc/ti-ads1299.c
 
 TI ADS7924 ADC DRIVER
 M:	Hugo Villeneuve <[email protected]>
drivers/iio/adc/Kconfig +14 -0
index 02de4809a..b9f2d3d6e 100644
@@ -1913,6 +1913,20 @@ config TI_ADS1298
 	  This driver can also be built as a module. If so, the module will be
 	  called ti-ads1298.
 
+config TI_ADS1299
+	tristate "Texas Instruments ADS1299"
+	depends on SPI
+	select IIO_BUFFER
+	select IIO_KFIFO_BUF
+	select REGMAP
+	help
+	  Say yes here to build support for Texas Instruments ADS1299,
+	  ADS1299-6 and ADS1299-4 low-noise 24-bit biopotential (EEG)
+	  analog front-ends.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called ti-ads1299.
+
 config TI_ADS131E08
 	tristate "Texas Instruments ADS131E08"
 	depends on SPI
drivers/iio/adc/Makefile +1 -0
index 9e82c21db..4bb543b66 100644
@@ -162,6 +162,7 @@ obj-$(CONFIG_TI_ADS112C04) += ti-ads112c04.o
 obj-$(CONFIG_TI_ADS112C14) += ti-ads112c14.o
 obj-$(CONFIG_TI_ADS124S08) += ti-ads124s08.o
 obj-$(CONFIG_TI_ADS1298) += ti-ads1298.o
+obj-$(CONFIG_TI_ADS1299) += ti-ads1299.o
 obj-$(CONFIG_TI_ADS131E08) += ti-ads131e08.o
 obj-$(CONFIG_TI_ADS131M02) += ti-ads131m02.o
 obj-$(CONFIG_TI_ADS7138) += ti-ads7138.o
drivers/iio/adc/ti-ads1299.c new file +897 -0

897 lines not on this page · raw message

Signature · 1 line
2.43.0
#4 lore raw

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

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…

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

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.

Quoted 6 lines
drivers/iio/adc/ti-ads1299.c new file +0 -0
new file mode 100644
index 0000000000000..f817a86cca5c9
@@ -0,0 +1,897 @@

[ ... ]

Quoted 28 lines
drivers/iio/adc/ti-ads1299.c new file +28 -0
+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];
+};

[ ... ]

Quoted 7 lines
drivers/iio/adc/ti-ads1299.c new file +7 -0
+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,

drivers/iio/adc/ti-ads1299.c line 208

Quoted 10 lines
drivers/iio/adc/ti-ads1299.c new file +10 -0
+		.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?

Quoted 33 lines
drivers/iio/adc/ti-ads1299.c new file +33 -0
+
+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;
+}

[ ... ]

Quoted 54 lines
drivers/iio/adc/ti-ads1299.c new file +54 -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;

drivers/iio/adc/ti-ads1299.c line 573

Quoted 10 lines
drivers/iio/adc/ti-ads1299.c new file +10 -0
+
+	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?

Quoted 11 lines
drivers/iio/adc/ti-ads1299.c new file +11 -0
+
+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;

drivers/iio/adc/ti-ads1299.c line 594

Quoted 10 lines
drivers/iio/adc/ti-ads1299.c new file +10 -0
+
+	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.

[ ... ]

Quoted 2 lines
drivers/iio/adc/ti-ads1299.c new file +2 -0
+static int ads1299_init(struct iio_dev *indio_dev, bool internal_vref)
+{

[ ... ]

Quoted 20 lines
drivers/iio/adc/ti-ads1299.c new file +20 -0
+	/* 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;
+}

[ ... ]

Quoted 2 lines
drivers/iio/adc/ti-ads1299.c new file +2 -0
+static int ads1299_probe(struct spi_device *spi)
+{

[ ... ]

Quoted 24 lines
drivers/iio/adc/ti-ads1299.c new file +24 -0
+	/* 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");

[ ... ]

Quoted 7 lines
drivers/iio/adc/ti-ads1299.c new file +7 -0
+	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");

[ ... ]

Quoted 9 lines
drivers/iio/adc/ti-ads1299.c new file +9 -0
+	/* 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 {

drivers/iio/adc/ti-ads1299.c line 821

Quoted 10 lines
drivers/iio/adc/ti-ads1299.c new file +10 -0
+		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

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
#5 lore raw

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

In reply to Stephan Costa Barros

Hello,

drivers/iio/adc/ti-ads1299.c line 31

Quoted 3 lines · Stephan Costa Barros
drivers/iio/adc/ti-ads1299.c new file +2 -0
+#include <linux/mod_devicetable.h>
[...]
+#include <linux/spi/spi.h>

<linux/spi/spi.h> provides both of_device_id and spi_device_id. So there is no need to also include <linux/mod_devicetable.h>. The latter pulls in a lot of useless dependencies, so there is actually some harm in using it.

Best regards
Uwe

#6 lore raw

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

Quoted 4 lines · Uwe Kleine-König

<linux/spi/spi.h> provides both of_device_id and spi_device_id. So there is no need to also include <linux/mod_devicetable.h>. The latter pulls in a lot of useless dependencies, so there is actually some harm in using it.

Thanks, I'll drop it in v2.

Stephan

#7 lore raw
In reply to Stephan Costa Barros
Quoted 3 lines · Stephan Costa Barros

This series adds a driver for the TI ADS1299 family (ADS1299, ADS1299-6 and ADS1299-4): 24-bit, simultaneous-sampling biopotential front-ends for EEG, on SPI.

Why a brand new driver?

Quoted 12 lines

This first version keeps to the generic IIO ABI: raw reads, the gain of each channel's PGA through its scale, the sampling frequency, and a kfifo buffer fed from the DRDY interrupt with a software timestamp.

The part has more, which I would like to add in follow-up series, each with its own ABI discussion: SRB1 and SRB2 reference routing, the bias drive amplifier, lead-off detection (through IIO_EV_TYPE_FAULT and IIO_EV_DIR_FAULT_OPENWIRE), the internal test signal, the temperature sensor and supply monitors, GPIO1 to GPIO4 as a gpiochip, the CLK pin as a clock output, and daisy chains. The binding already describes the hardware for those.

Quoted 10 lines

A question on structure. The driver started from ti-ads1298.c, and in this subset the two are close. What differs here is the PGA gain table (1 to 24 against 1 to 12 with 3), the data rates (no high-resolution mode), the fixed 4.5 V internal reference, the ID register layout and the 4 and 6 channel variants; above 0x14 the register maps also part (MISC1 and MISC2 here, PACE, RESP and WCT on the ADS1298). Most of what would follow, SRB1 and SRB2, the bias drive and lead-off on every channel, is specific to the ADS1299, so I kept a separate driver. If you would rather have the ADS1299 as a variant in ti-ads1298, I can rework it that way.

Okay, I think you can try to rework this as a series on top of the existing code. I can't easily see if the above difference warrants a brand new driver.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#8 lore raw
Quoted 1 line · Andy Shevchenko

On Sun, Oct 18, 2026 at 09:12:45PM -0300, Stephan Costa Barros wrote:

[...]

Quoted 2 lines
Quoted 2 lines

A question on structure. The driver started from ti-ads1298.c, and in this subset the two are close.

[...]

Quoted 2 lines

Okay, I think you can try to rework this as a series on top of the existing code. I can't easily see if the above difference warrants a brand new driver.

Thanks, Andy. I'll do that for v2: first prepare ti-ads1298 for more than one family (chip info for the gain table, the data rates and the reference), then add the ADS1299 to it, with its compatibles in ti,ads1298.yaml instead of a new binding.

Some background: this comes from my undergraduate thesis. The project first planned a driver written from scratch; I then found ti-ads1298 and built on it instead. Beyond what ti-ads1298 does today, the out-of-tree driver that came out of it sets the gain of each channel, timestamps each buffered sample, reports lead-off as IIO_EV_TYPE_FAULT / IIO_EV_DIR_FAULT_OPENWIRE events, routes the internal test signal and shorted inputs, reads the supply monitors and the temperature sensor, switches SRB1 and SRB2, and runs a self test at probe; all of that is tested on an ADS1299. The bias drive, GPIO1-4 as a gpiochip, the CLK output, PWDN and suspend are written but not tested yet.

For mainline I cut this down to the generic ABI. The rest can follow as separate series on top of ti-ads1298, where most of it applies to the ADS1298 as well; only SRB1 and SRB2 are ADS1299-specific.

The automated review of this version also pointed at issues that the new driver inherited from ti-ads1298 too.

I had missed Md Shofiqul Islam's series when I sent this one, and found it only now: "[PATCH v2 0/2] iio: adc: ti-ads1298: add ADS1298/ADS1299 driver", sent on 6 July 2026 and reviewed by Jonathan on 11 July. Md, are you still working on it? If not, I would like to take it on, following Jonathan's review there (per-chip match data, the reference voltage and the ID register), and I can test it on hardware: an ADS1299 with DRDY as an interrupt.

Jonathan, if you would rather keep a separate driver, please let me know before I start.

Stephan

maintainer
#9 lore raw
Quoted 44 lines · On Mon, 19 Oct 2026 21:00:31 -0300 Stephan Costa Barros…
Quoted 1 line · Andy Shevchenko

On Sun, Oct 18, 2026 at 09:12:45PM -0300, Stephan Costa Barros wrote:

[...]

Quoted 2 lines
Quoted 2 lines

A question on structure. The driver started from ti-ads1298.c, and in this subset the two are close.

[...]

Quoted 2 lines

Okay, I think you can try to rework this as a series on top of the existing code. I can't easily see if the above difference warrants a brand new driver.

Thanks, Andy. I'll do that for v2: first prepare ti-ads1298 for more than one family (chip info for the gain table, the data rates and the reference), then add the ADS1299 to it, with its compatibles in ti,ads1298.yaml instead of a new binding.

Some background: this comes from my undergraduate thesis. The project first planned a driver written from scratch; I then found ti-ads1298 and built on it instead. Beyond what ti-ads1298 does today, the out-of-tree driver that came out of it sets the gain of each channel, timestamps each buffered sample, reports lead-off as IIO_EV_TYPE_FAULT / IIO_EV_DIR_FAULT_OPENWIRE events, routes the internal test signal and shorted inputs, reads the supply monitors and the temperature sensor, switches SRB1 and SRB2, and runs a self test at probe; all of that is tested on an ADS1299. The bias drive, GPIO1-4 as a gpiochip, the CLK output, PWDN and suspend are written but not tested yet.

For mainline I cut this down to the generic ABI. The rest can follow as separate series on top of ti-ads1298, where most of it applies to the ADS1298 as well; only SRB1 and SRB2 are ADS1299-specific.

The automated review of this version also pointed at issues that the new driver inherited from ti-ads1298 too.

I had missed Md Shofiqul Islam's series when I sent this one, and found it only now: "[PATCH v2 0/2] iio: adc: ti-ads1298: add ADS1298/ADS1299 driver", sent on 6 July 2026 and reviewed by Jonathan on 11 July. Md, are you still working on it? If not, I would like to take it on, following Jonathan's review there (per-chip match data, the reference voltage and the ID register), and I can test it on hardware: an ADS1299 with DRDY as an interrupt.

Quoted 3 lines

Jonathan, if you would rather keep a separate driver, please let me know before I start.

Definitely worth having a go to extend the existing driver. If you run into major problems or unexpected complexity then possibly come back to the separate one.

Jonathan

Quoted 1 line

Stephan