[RFC PATCH 0/5] iio: adc: Add initial support for TI ADS1262

[RFC PATCH 0/5] iio: adc: Add initial support for TI ADS1262

[Sayyad Abid]

The ADS1262 is a 32-bit, high-resolution delta-sigma ADC communicating
over SPI. It's designed for precision measurements.

This initial driver provides the basic functionality needed to:
 - Probe and register the device via SPI.
 - Expose standard IIO channels for reading raw voltage samples.

Basic testing was performed on a Raspberry Pi Zero 2W using the hardware
SPI0 interface. The connections used were:

+-----------------+            +-----------------+
| RPi Zero 2W     |            | TI ADS1262      |
| (SPI0 Pins)     |            |                 |
|-----------------|            |-----------------|
| MOSI            |----------->| DIN             |
| MISO            |<-----------| DOUT/DRDY       |
| SCLK            |----------->| SCLK            |
| CE0             |----------->| /CS             |
| 5V              |----------->| DVDD, AVDD      |
| GND             |----------->| DGND, AGND      |
+-----------------+            +-----------------+

I would greatly appreciate any feedback on the driver structure,
IIO integration, SPI communication handling, or any potential issues
or areas for improvement you might spot.

This series is broken down as follows:
 Patch 1: Adds the core driver code (ti-ads1262.c).
 Patch 2: Adds the Kconfig option.
 Patch 3: Adds the Makefile entry for compilation.
 Patch 4: Adds the MAINTAINERS entry.

Thanks for your time and consideration.

Sayyad Abid (5):
  iio: adc: ti-ads1262.c: add initial driver for TI ADS1262 ADC
  iio: adc: Kconfig: add Kconfig entry for TI ADS1262 driver
  iio: adc: Makefile: add compile support for TI ADS1262 driver
  MAINTAINERS: add entry for TI ADS1262 ADC driver
  dt-bindings: iio: adc: add bindings for TI ADS1262

 .../bindings/iio/adc/ti,ads1262.yaml          | 189 ++++++++
 MAINTAINERS                                   |   7 +
 drivers/iio/adc/Kconfig                       |  12 +
 drivers/iio/adc/Makefile                      |   1 +
 drivers/iio/adc/ti-ads1262.c                  | 438 ++++++++++++++++++
 5 files changed, 647 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1262.yaml
 create mode 100644 drivers/iio/adc/ti-ads1262.c

--
2.39.5

[RFC PATCH 1/5] iio: adc: ti-ads1262.c: add initial driver for TI ADS1262 ADC

[Sayyad Abid]

Add the core driver file `ti-ads1262.c` for the TI ADS1262 ADC.
This initial version implements basic IIO functionality for device
probe via SPI and reading raw voltage samples from input channels.

Signed-off-by: Sayyad Abid <sayyad.abid16@gmail.com>
---
 drivers/iio/adc/ti-ads1262.c | 438 +++++++++++++++++++++++++++++++++++
 1 file changed, 438 insertions(+)
 create mode 100644 drivers/iio/adc/ti-ads1262.c

diff --git a/drivers/iio/adc/ti-ads1262.c b/drivers/iio/adc/ti-ads1262.c
new file mode 100644
index 000000000000..ef34c528ffeb
--- /dev/null
+++ b/drivers/iio/adc/ti-ads1262.c
@@ -0,0 +1,438 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * IIO driver for Texas Instruments ADS1662 32-bit ADC
+ *
+ * Datasheet: https://www.ti.com/product/ADS1262
+ */
+
+#include <linux/kernel.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/delay.h>
+#include <linux/spi/spi.h>
+#include <linux/unaligned.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/triggered_buffer.h>
+
+/* Commands */
+#define ADS1262_CMD_RESET		0x06
+#define ADS1262_CMD_START1		0x08
+#define ADS1262_CMD_STOP1		0x0A
+#define ADS1262_CMD_RDATA1		0x12
+#define ADS1262_CMD_RREG		0x20
+#define ADS1262_CMD_WREG		0x40
+
+/* Registers */
+#define ADS1262_REG_ID			0x00
+#define ADS1262_REG_POWER		0x01
+#define ADS1262_REG_INTERFACE		0x02
+#define ADS1262_REG_MODE0		0x03
+#define ADS1262_REG_MODE1		0x04
+#define ADS1262_REG_MODE2		0x05
+#define ADS1262_REG_INPMUX		0x06
+
+/* Configurations */
+#define ADS1262_INTREF_ENABLE		0x01
+#define ADS1262_MODE0_ONE_SHOT		0x40
+#define ADS1262_MODE2_PGA_EN		0x00
+#define ADS1262_MODE2_PGA_BYPASS	BIT(7)
+
+/* Masks */
+#define ADS1262_MASK_MODE2_DR		GENMASK(4, 0)
+
+/* ADS1262_SPECS */
+#define ADS1262_MAX_CHANNELS		11
+#define ADS1262_BITS_PER_SAMPLE		32
+#define ADS1262_CLK_RATE_HZ		7372800
+#define ADS1262_CLOCKS_TO_USECS(x)	\
+	(DIV_ROUND_UP((x) * MICROHZ_PER_HZ, ADS1262_CLK_RATE_HZ))
+#define ADS1262_VOLTAGE_INT_REF_uV	2500000
+#define ADS1262_TEMP_SENSITIVITY_uV_per_C 420
+
+#define ADS1262_SETTLE_TIME_USECS	10000
+
+/* The Read/Write commands require 4 tCLK to encode and decode, for speeds
+ * 2x the clock rate, these commands would require extra time between the
+ * command byte and the data. A simple way to tacke this issue is by
+ * limiting the SPI bus transfer speed while accessing registers.
+ */
+#define ADS1262_SPI_BUS_SPEED_SLOW	ADS1262_CLK_RATE_HZ
+
+/* For reading and writing we need a buffer of size 3bytes*/
+#define ADS1262_SPI_CMD_BUFFER_SIZE	3
+
+/* Read data buffer size for
+ * 1 status byte - 4 byte data (32 bit) - 1 byte checksum / CRC
+ */
+#define ADS1262_SPI_RDATA_BUFFER_SIZE	6
+
+#define MILLI				1000
+
+/**
+ * struct ads1262_private - ADS1262 ADC private data structure
+ * @spi: SPI device structure
+ * @reset_gpio: GPIO descriptor for reset pin
+ * @prev_channel: Previously selected channel for MUX configuration
+ * @cmd_buffer: Buffer for SPI command transfers
+ * @rx_buffer: Buffer for SPI data reception
+ */
+struct ads1262_private {
+	struct spi_device *spi;
+	struct gpio_desc *reset_gpio;
+	u8 prev_channel;
+	u8 cmd_buffer[ADS1262_SPI_CMD_BUFFER_SIZE];
+	u8 rx_buffer[ADS1262_SPI_RDATA_BUFFER_SIZE] __aligned(IIO_DMA_MINALIGN);
+};
+
+#define ADS1262_CHAN(index)						\
+{									\
+	.type = IIO_VOLTAGE,						\
+	.indexed = 1,							\
+	.channel = index,						\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
+	.scan_index = index,						\
+	.scan_type = {							\
+		.sign = 's',						\
+		.realbits = ADS1262_BITS_PER_SAMPLE,			\
+		.storagebits = 32,					\
+		.endianness = IIO_CPU					\
+	},								\
+}
+
+#define ADS1262_DIFF_CHAN(index, pos_channel, neg_channel)		\
+{									\
+	.type = IIO_VOLTAGE,						\
+	.indexed = 1,							\
+	.channel = pos_channel,						\
+	.channel2 = neg_channel,					\
+	.differential = 1,						\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
+	.scan_index = index,						\
+	.scan_type = {							\
+		.sign = 's',						\
+		.realbits = ADS1262_BITS_PER_SAMPLE,			\
+		.storagebits = 32,					\
+		.endianness = IIO_CPU					\
+	},								\
+}
+
+#define ADS1262_TEMP_CHAN(index)					\
+{									\
+	.type = IIO_TEMP,						\
+	.indexed = 1,							\
+	.channel = index,						\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
+			     BIT(IIO_CHAN_INFO_SCALE) |			\
+			     BIT(IIO_CHAN_INFO_SAMP_FREQ),		\
+	.scan_index = index,						\
+	.scan_type = {							\
+		.sign = 's',						\
+		.realbits = ADS1262_BITS_PER_SAMPLE,			\
+		.storagebits = 32,					\
+		.endianness = IIO_BE,					\
+	},								\
+}
+
+static const struct iio_chan_spec ads1262_channels[] = {
+	/* Single ended channels*/
+	ADS1262_CHAN(0),
+	ADS1262_CHAN(1),
+	ADS1262_CHAN(2),
+	ADS1262_CHAN(3),
+	ADS1262_CHAN(4),
+	ADS1262_CHAN(5),
+	ADS1262_CHAN(6),
+	ADS1262_CHAN(7),
+	ADS1262_CHAN(8),
+	ADS1262_CHAN(9),
+	/* The channel at index 10 is AINCOM, which is the common ground
+	 * of the ADC. It is not a valid channel for the user.
+	 */
+
+	/* Temperature and Monitor channels */
+	ADS1262_TEMP_CHAN(11),	/* TEMP SENSOR */
+	ADS1262_CHAN(12),	/* AVDD MON */
+	ADS1262_CHAN(13),	/* DVDD MON */
+	ADS1262_CHAN(14),	/* TDAC TEST */
+	/* Differential channels */
+	ADS1262_DIFF_CHAN(15, 0, 1),	/* AIN0 - AIN1 */
+	ADS1262_DIFF_CHAN(16, 2, 3),	/* AIN2 - AIN3 */
+	ADS1262_DIFF_CHAN(17, 4, 5),	/* AIN4 - AIN5 */
+	ADS1262_DIFF_CHAN(18, 6, 7),	/* AIN6 - AIN7 */
+	ADS1262_DIFF_CHAN(19, 8, 9),	/* AIN8 - AIN9 */
+};
+
+static int ads1262_write_cmd(struct ads1262_private *priv, u8 command)
+{
+	struct spi_transfer xfer = {
+		.tx_buf = priv->cmd_buffer,
+		.rx_buf = priv->rx_buffer,
+		.len = ADS1262_SPI_RDATA_BUFFER_SIZE,
+		.speed_hz = ADS1262_CLK_RATE_HZ,
+	};
+
+	priv->cmd_buffer[0] = command;
+
+	return spi_sync_transfer(priv->spi, &xfer, 1);
+}
+
+static int ads1262_reg_write(void *context, unsigned int reg, unsigned int val)
+{
+	struct ads1262_private *priv = context;
+
+	priv->cmd_buffer[0] = ADS1262_CMD_WREG | reg;
+	priv->cmd_buffer[1] = 0;
+	priv->cmd_buffer[2] = val;
+
+	return spi_write(priv->spi, &priv->cmd_buffer[0], 3);
+}
+
+static int ads1262_reg_read(void *context, unsigned int reg)
+{
+	struct ads1262_private *priv = context;
+	struct spi_transfer reg_read_xfer = {
+		.tx_buf = priv->cmd_buffer,
+		.rx_buf = priv->cmd_buffer,
+		.len = 3,
+		.speed_hz = ADS1262_CLK_RATE_HZ,
+	};
+	int ret;
+
+	priv->cmd_buffer[0] = ADS1262_CMD_RREG | reg;
+	priv->cmd_buffer[1] = 0;
+	priv->cmd_buffer[2] = 0;
+
+	ret = spi_sync_transfer(priv->spi, &reg_read_xfer, 1);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int ads1262_reset(struct iio_dev *indio_dev)
+{
+	struct ads1262_private *priv = iio_priv(indio_dev);
+
+	if (priv->reset_gpio) {
+		gpiod_set_value(priv->reset_gpio, 0);
+		usleep_range(200, 300);
+		gpiod_set_value(priv->reset_gpio, 1);
+	} else {
+		return ads1262_write_cmd(priv, ADS1262_CMD_RESET);
+	}
+	return 0;
+}
+
+static int ads1262_init(struct iio_dev *indio_dev)
+{
+	struct ads1262_private *priv = iio_priv(indio_dev);
+	int ret = 0;
+
+	ret = ads1262_reset(indio_dev);
+	if (ret)
+		return ret;
+
+	/* 10 milliseconds settling time for the ADC to stabilize */
+	fsleep(ADS1262_SETTLE_TIME_USECS);
+
+	/* Clearing the RESET bit in the power register to detect ADC reset */
+	ret = ads1262_reg_write(priv, ADS1262_REG_POWER, ADS1262_INTREF_ENABLE);
+	if (ret)
+		return ret;
+
+	/* Setting the ADC to one-shot conversion mode */
+	ret = ads1262_reg_write(priv, ADS1262_REG_MODE0, ADS1262_MODE0_ONE_SHOT);
+	if (ret)
+		return ret;
+
+	ret = ads1262_reg_read(priv, ADS1262_REG_INPMUX);
+	if (ret)
+		return ret;
+
+	priv->prev_channel = priv->cmd_buffer[2];
+
+	return ret;
+}
+
+static int ads1262_get_samp_freq(struct ads1262_private *priv, int *val)
+{
+	unsigned long samp_freq;
+	int ret;
+
+	ret = ads1262_reg_read(priv, ADS1262_REG_MODE2);
+	if (ret)
+		return ret;
+
+	samp_freq = priv->cmd_buffer[2];
+
+	*val = (samp_freq & ADS1262_MASK_MODE2_DR);
+
+	return IIO_VAL_INT;
+}
+
+/**
+ * ads1262_read - Read a single sample from the ADC
+ * @priv: Pointer to the ADS1262 private data structure
+ * @chan: Pointer to the IIO channel specification
+ * @val: Pointer to store the read value
+ *
+ * Reads a single sample from the specified ADC channel. For differential
+ * channels, it sets up the MUX with both channels. For single-ended channels,
+ * it uses the channel number and AINCOM (0x0A).
+ *
+ * Return: 0 on success, negative error code on failure
+ */
+static int ads1262_read(struct ads1262_private *priv,
+			struct iio_chan_spec const *chan, int *val)
+{
+	u8 mux_value;
+	int ret;
+
+	if (chan->differential) {
+		mux_value = (chan->channel << 4) | chan->channel2;
+	} else {
+		/* For single-ended channels, use the channel number on one end
+		 * and AINCOM (0x0A) on the other end
+		 */
+		mux_value = (chan->channel << 4) | 0x0A;
+	}
+
+	if (mux_value != priv->prev_channel) {
+		ret = ads1262_write_cmd(priv, ADS1262_CMD_STOP1);
+		if (ret)
+			return ret;
+
+		ret = ads1262_reg_write(priv, ADS1262_REG_INPMUX, mux_value);
+		if (ret)
+			return ret;
+
+		priv->prev_channel = mux_value;
+	}
+
+	ret = ads1262_write_cmd(priv, ADS1262_CMD_START1);
+	if (ret)
+		return ret;
+
+	usleep_range(1000, 2000);
+
+	*val = sign_extend64(get_unaligned_be32(priv->rx_buffer + 1),
+			     ADS1262_BITS_PER_SAMPLE - 1);
+
+	return 0;
+}
+
+static int ads1262_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int *val, int *val2, long mask)
+{
+	struct ads1262_private *spi = iio_priv(indio_dev);
+	s64 temp;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ads1262_read(spi, chan, val);
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_VOLTAGE:
+			*val = ADS1262_VOLTAGE_INT_REF_uV;
+			*val2 = chan->scan_type.realbits;
+			return IIO_VAL_FRACTIONAL_LOG2;
+		case IIO_TEMP:
+			temp = (s64)ADS1262_VOLTAGE_INT_REF_uV * MILLI;
+			temp /= ADS1262_TEMP_SENSITIVITY_uV_per_C;
+			*val = temp;
+			*val2 = chan->scan_type.realbits;
+			return IIO_VAL_FRACTIONAL_LOG2;
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		return ads1262_get_samp_freq(spi, val);
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info ads1262_info = {
+	.read_raw = ads1262_read_raw,
+};
+
+static void ads1262_stop(void *ptr)
+{
+	struct ads1262_private *adc = ptr;
+
+	ads1262_write_cmd(adc, ADS1262_CMD_STOP1);
+}
+
+static int ads1262_probe(struct spi_device *spi)
+{
+	struct ads1262_private *adc;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	adc = iio_priv(indio_dev);
+	adc->spi = spi;
+
+	spi->mode = SPI_MODE_1;
+	spi->max_speed_hz = ADS1262_SPI_BUS_SPEED_SLOW;
+	spi_set_drvdata(spi, indio_dev);
+
+	indio_dev->dev.parent = &spi->dev;
+	indio_dev->name = spi_get_device_id(spi)->name;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = ads1262_channels;
+	indio_dev->num_channels = ARRAY_SIZE(ads1262_channels);
+	indio_dev->info = &ads1262_info;
+
+	ret = ads1262_reg_read(adc, ADS1262_REG_ID);
+	if (ret)
+		return ret;
+
+	if (adc->rx_buffer[2] != ADS1262_REG_ID)
+		dev_err_probe(&spi->dev, -EINVAL, "Wrong device ID 0x%x\n",
+			      adc->rx_buffer[2]);
+
+	ret = devm_add_action_or_reset(&spi->dev, ads1262_stop, adc);
+	if (ret)
+		return ret;
+
+	ret = ads1262_init(indio_dev);
+	if (ret)
+		return ret;
+
+	return devm_iio_device_register(&spi->dev, indio_dev);
+}
+
+static struct spi_device_id ads1262_id_table[] = {
+	{ "ads1262" },
+	{}
+};
+MODULE_DEVICE_TABLE(spi, ads1262_id_table);
+
+static const struct of_device_id ads1262_of_match[] = {
+	{ .compatible = "ti,ads1262" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, ads1262_of_match);
+
+static struct spi_driver ads1262_driver = {
+	.driver = {
+		.name = "ads1262",
+		.of_match_table = ads1262_of_match,
+	},
+	.probe = ads1262_probe,
+	.id_table = ads1262_id_table,
+};
+module_spi_driver(ads1262_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Sayyad Abid <sayyad.abid16@gmail.com>");
+MODULE_DESCRIPTION("TI ADS1262 ADC");
-- 
2.39.5

Re: [RFC PATCH 1/5] iio: adc: ti-ads1262.c: add initial driver for TI ADS1262 ADC

[David Lechner]

On 5/1/25 5:00 AM, Sayyad Abid wrote:
> Add the core driver file `ti-ads1262.c` for the TI ADS1262 ADC.
> This initial version implements basic IIO functionality for device
> probe via SPI and reading raw voltage samples from input channels.
> 
> Signed-off-by: Sayyad Abid <sayyad.abid16@gmail.com>
> ---
>  drivers/iio/adc/ti-ads1262.c | 438 +++++++++++++++++++++++++++++++++++
>  1 file changed, 438 insertions(+)
>  create mode 100644 drivers/iio/adc/ti-ads1262.c
> 
> diff --git a/drivers/iio/adc/ti-ads1262.c b/drivers/iio/adc/ti-ads1262.c
> new file mode 100644
> index 000000000000..ef34c528ffeb
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads1262.c
> @@ -0,0 +1,438 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * IIO driver for Texas Instruments ADS1662 32-bit ADC
> + *
> + * Datasheet: https://www.ti.com/product/ADS1262
> + */
> +
> +#include <linux/kernel.h>

This header includes too much, please use more specific headers.

> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/delay.h>

Alphabetical order is preferred.

> +#include <linux/spi/spi.h>
> +#include <linux/unaligned.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/triggered_buffer.h>
> +
> +/* Commands */
> +#define ADS1262_CMD_RESET		0x06
> +#define ADS1262_CMD_START1		0x08
> +#define ADS1262_CMD_STOP1		0x0A
> +#define ADS1262_CMD_RDATA1		0x12
> +#define ADS1262_CMD_RREG		0x20
> +#define ADS1262_CMD_WREG		0x40
> +
> +/* Registers */
> +#define ADS1262_REG_ID			0x00
> +#define ADS1262_REG_POWER		0x01
> +#define ADS1262_REG_INTERFACE		0x02
> +#define ADS1262_REG_MODE0		0x03
> +#define ADS1262_REG_MODE1		0x04
> +#define ADS1262_REG_MODE2		0x05
> +#define ADS1262_REG_INPMUX		0x06
> +
> +/* Configurations */
> +#define ADS1262_INTREF_ENABLE		0x01
> +#define ADS1262_MODE0_ONE_SHOT		0x40
> +#define ADS1262_MODE2_PGA_EN		0x00
> +#define ADS1262_MODE2_PGA_BYPASS	BIT(7)
> +
> +/* Masks */
> +#define ADS1262_MASK_MODE2_DR		GENMASK(4, 0)
> +
> +/* ADS1262_SPECS */
> +#define ADS1262_MAX_CHANNELS		11
> +#define ADS1262_BITS_PER_SAMPLE		32
> +#define ADS1262_CLK_RATE_HZ		7372800
> +#define ADS1262_CLOCKS_TO_USECS(x)	\
> +	(DIV_ROUND_UP((x) * MICROHZ_PER_HZ, ADS1262_CLK_RATE_HZ))

This is only for the internal clock, but external clock is also possible so
better to just do this inline rather than a macro.

> +#define ADS1262_VOLTAGE_INT_REF_uV	2500000
> +#define ADS1262_TEMP_SENSITIVITY_uV_per_C 420
> +
> +#define ADS1262_SETTLE_TIME_USECS	10000
> +
> +/* The Read/Write commands require 4 tCLK to encode and decode, for speeds
> + * 2x the clock rate, these commands would require extra time between the
> + * command byte and the data. A simple way to tacke this issue is by
> + * limiting the SPI bus transfer speed while accessing registers.
> + */
> +#define ADS1262_SPI_BUS_SPEED_SLOW	ADS1262_CLK_RATE_HZ
> +
> +/* For reading and writing we need a buffer of size 3bytes*/
> +#define ADS1262_SPI_CMD_BUFFER_SIZE	3
> +
> +/* Read data buffer size for
> + * 1 status byte - 4 byte data (32 bit) - 1 byte checksum / CRC
> + */
> +#define ADS1262_SPI_RDATA_BUFFER_SIZE	6
> +
> +#define MILLI				1000

There is already linux/units.h for this.

> +
> +/**
> + * struct ads1262_private - ADS1262 ADC private data structure
> + * @spi: SPI device structure
> + * @reset_gpio: GPIO descriptor for reset pin
> + * @prev_channel: Previously selected channel for MUX configuration
> + * @cmd_buffer: Buffer for SPI command transfers
> + * @rx_buffer: Buffer for SPI data reception
> + */
> +struct ads1262_private {
> +	struct spi_device *spi;
> +	struct gpio_desc *reset_gpio;
> +	u8 prev_channel;
> +	u8 cmd_buffer[ADS1262_SPI_CMD_BUFFER_SIZE];
> +	u8 rx_buffer[ADS1262_SPI_RDATA_BUFFER_SIZE] __aligned(IIO_DMA_MINALIGN);

cmd_buffer is also used with SPI, so __aligned(IIO_DMA_MINALIGN); needs to go
there instead.

> +};
> +
> +#define ADS1262_CHAN(index)						\
> +{									\
> +	.type = IIO_VOLTAGE,						\
> +	.indexed = 1,							\
> +	.channel = index,						\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
> +	.scan_index = index,						\
> +	.scan_type = {							\
> +		.sign = 's',						\
> +		.realbits = ADS1262_BITS_PER_SAMPLE,			\
> +		.storagebits = 32,					\
> +		.endianness = IIO_CPU					\
> +	},								\
> +}
> +
> +#define ADS1262_DIFF_CHAN(index, pos_channel, neg_channel)		\
> +{									\
> +	.type = IIO_VOLTAGE,						\
> +	.indexed = 1,							\
> +	.channel = pos_channel,						\
> +	.channel2 = neg_channel,					\
> +	.differential = 1,						\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
> +	.scan_index = index,						\
> +	.scan_type = {							\
> +		.sign = 's',						\
> +		.realbits = ADS1262_BITS_PER_SAMPLE,			\
> +		.storagebits = 32,					\
> +		.endianness = IIO_CPU					\
> +	},								\
> +}
> +
> +#define ADS1262_TEMP_CHAN(index)					\
> +{									\
> +	.type = IIO_TEMP,						\
> +	.indexed = 1,							\
> +	.channel = index,						\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
> +			     BIT(IIO_CHAN_INFO_SCALE) |			\
> +			     BIT(IIO_CHAN_INFO_SAMP_FREQ),		\
> +	.scan_index = index,						\
> +	.scan_type = {							\
> +		.sign = 's',						\
> +		.realbits = ADS1262_BITS_PER_SAMPLE,			\
> +		.storagebits = 32,					\
> +		.endianness = IIO_BE,					\
> +	},								\
> +}
> +
> +static const struct iio_chan_spec ads1262_channels[] = {
> +	/* Single ended channels*/
> +	ADS1262_CHAN(0),
> +	ADS1262_CHAN(1),
> +	ADS1262_CHAN(2),
> +	ADS1262_CHAN(3),
> +	ADS1262_CHAN(4),
> +	ADS1262_CHAN(5),
> +	ADS1262_CHAN(6),
> +	ADS1262_CHAN(7),
> +	ADS1262_CHAN(8),
> +	ADS1262_CHAN(9),

Hard-coding the channels here means there would be no point in allowing the
devicetree to specify the channels. However, since the same pins can be used
for many other purposes, I don't think hard-coding channels here makes sense
and we should always dynamically create the channel specs for channels connected
to AIN pins based on the devicetree.

> +	/* The channel at index 10 is AINCOM, which is the common ground
> +	 * of the ADC. It is not a valid channel for the user.
> +	 */

If this bit about AINCOM is true, then the DT bindings need to reflect that.
The datasheet makes it looks like any other input though.



> +
> +	/* Temperature and Monitor channels */
> +	ADS1262_TEMP_CHAN(11),	/* TEMP SENSOR */
> +	ADS1262_CHAN(12),	/* AVDD MON */
> +	ADS1262_CHAN(13),	/* DVDD MON */
> +	ADS1262_CHAN(14),	/* TDAC TEST */

It will be fine to always add these diagnotic channels though without having to
specify them in the devicetree.

> +	/* Differential channels */
> +	ADS1262_DIFF_CHAN(15, 0, 1),	/* AIN0 - AIN1 */
> +	ADS1262_DIFF_CHAN(16, 2, 3),	/* AIN2 - AIN3 */
> +	ADS1262_DIFF_CHAN(17, 4, 5),	/* AIN4 - AIN5 */
> +	ADS1262_DIFF_CHAN(18, 6, 7),	/* AIN6 - AIN7 */
> +	ADS1262_DIFF_CHAN(19, 8, 9),	/* AIN8 - AIN9 */
> +};
> +
> +static int ads1262_write_cmd(struct ads1262_private *priv, u8 command)
> +{
> +	struct spi_transfer xfer = {
> +		.tx_buf = priv->cmd_buffer,
> +		.rx_buf = priv->rx_buffer,
> +		.len = ADS1262_SPI_RDATA_BUFFER_SIZE,
> +		.speed_hz = ADS1262_CLK_RATE_HZ,
> +	};
> +
> +	priv->cmd_buffer[0] = command;
> +
> +	return spi_sync_transfer(priv->spi, &xfer, 1);
> +}
> +
> +static int ads1262_reg_write(void *context, unsigned int reg, unsigned int val)
> +{
> +	struct ads1262_private *priv = context;
> +
> +	priv->cmd_buffer[0] = ADS1262_CMD_WREG | reg;
> +	priv->cmd_buffer[1] = 0;
> +	priv->cmd_buffer[2] = val;
> +
> +	return spi_write(priv->spi, &priv->cmd_buffer[0], 3);
> +}
> +
> +static int ads1262_reg_read(void *context, unsigned int reg)
> +{
> +	struct ads1262_private *priv = context;
> +	struct spi_transfer reg_read_xfer = {
> +		.tx_buf = priv->cmd_buffer,
> +		.rx_buf = priv->cmd_buffer,
> +		.len = 3,
> +		.speed_hz = ADS1262_CLK_RATE_HZ,
> +	};
> +	int ret;
> +
> +	priv->cmd_buffer[0] = ADS1262_CMD_RREG | reg;
> +	priv->cmd_buffer[1] = 0;
> +	priv->cmd_buffer[2] = 0;
> +
> +	ret = spi_sync_transfer(priv->spi, &reg_read_xfer, 1);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}

Why not use regmap? You will still custom read/write functions similar to these
because of needing the lower SCLK rate, but it will give you a bunch of other
nice features for free.

> +
> +static int ads1262_reset(struct iio_dev *indio_dev)
> +{
> +	struct ads1262_private *priv = iio_priv(indio_dev);
> +
> +	if (priv->reset_gpio) {
> +		gpiod_set_value(priv->reset_gpio, 0);
> +		usleep_range(200, 300);

Use fsleep(). Also, could make this clear that it is 4 tCLK cycles (the hard-
coded value would have to be changed if external clock support was added).

> +		gpiod_set_value(priv->reset_gpio, 1);

The DT bindings will take care of active low, so this looks backwards. Also
st->reset_gpio is never assigned, so this is dead code.

> +	} else {
> +		return ads1262_write_cmd(priv, ADS1262_CMD_RESET);
> +	}
> +	return 0;
> +}
> +
> +static int ads1262_init(struct iio_dev *indio_dev)
> +{
> +	struct ads1262_private *priv = iio_priv(indio_dev);
> +	int ret = 0;
> +
> +	ret = ads1262_reset(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	/* 10 milliseconds settling time for the ADC to stabilize */
> +	fsleep(ADS1262_SETTLE_TIME_USECS);

Woud make more sense to have this inside the reset function.

> +
> +	/* Clearing the RESET bit in the power register to detect ADC reset */
> +	ret = ads1262_reg_write(priv, ADS1262_REG_POWER, ADS1262_INTREF_ENABLE);

This is where regmap_clear_bits() would come in handy. Then you don't have to
care about other defaults like INTREF being 1.

> +	if (ret)
> +		return ret;
> +
> +	/* Setting the ADC to one-shot conversion mode */
> +	ret = ads1262_reg_write(priv, ADS1262_REG_MODE0, ADS1262_MODE0_ONE_SHOT);
> +	if (ret)
> +		return ret;
> +
> +	ret = ads1262_reg_read(priv, ADS1262_REG_INPMUX);
> +	if (ret)
> +		return ret;
> +
> +	priv->prev_channel = priv->cmd_buffer[2];

Regmap has features to give all of the default values of registers and mark
which registers are volatile. Then you don't have to manage stuff like this
yourself.

> +
> +	return ret;
> +}
> +
> +static int ads1262_get_samp_freq(struct ads1262_private *priv, int *val)
> +{
> +	unsigned long samp_freq;
> +	int ret;
> +
> +	ret = ads1262_reg_read(priv, ADS1262_REG_MODE2);
> +	if (ret)
> +		return ret;
> +
> +	samp_freq = priv->cmd_buffer[2];
> +
> +	*val = (samp_freq & ADS1262_MASK_MODE2_DR);

Use FIELD_GET().

> +
> +	return IIO_VAL_INT;
> +}
> +
> +/**
> + * ads1262_read - Read a single sample from the ADC
> + * @priv: Pointer to the ADS1262 private data structure
> + * @chan: Pointer to the IIO channel specification
> + * @val: Pointer to store the read value
> + *
> + * Reads a single sample from the specified ADC channel. For differential
> + * channels, it sets up the MUX with both channels. For single-ended channels,
> + * it uses the channel number and AINCOM (0x0A).
> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +static int ads1262_read(struct ads1262_private *priv,
> +			struct iio_chan_spec const *chan, int *val)
> +{
> +	u8 mux_value;
> +	int ret;
> +
> +	if (chan->differential) {
> +		mux_value = (chan->channel << 4) | chan->channel2;
> +	} else {
> +		/* For single-ended channels, use the channel number on one end
> +		 * and AINCOM (0x0A) on the other end
> +		 */
> +		mux_value = (chan->channel << 4) | 0x0A;
> +	}
> +
> +	if (mux_value != priv->prev_channel) {
> +		ret = ads1262_write_cmd(priv, ADS1262_CMD_STOP1);
> +		if (ret)
> +			return ret;
> +
> +		ret = ads1262_reg_write(priv, ADS1262_REG_INPMUX, mux_value);
> +		if (ret)
> +			return ret;
> +
> +		priv->prev_channel = mux_value;
> +	}
> +
> +	ret = ads1262_write_cmd(priv, ADS1262_CMD_START1);
> +	if (ret)
> +		return ret;
> +
> +	usleep_range(1000, 2000);

fsleep().

Also, doesn't this depend on the sample rate (and filter type, but that isn't
implemented yet).

Another alternative would be to use the /DRDY interrupt instead so that we
don't have to try to guess the right amout of time to wait for the conversion
to complete.

> +
> +	*val = sign_extend64(get_unaligned_be32(priv->rx_buffer + 1),

val is not 64 bit. Since ADS1262_BITS_PER_SAMPLE == 32, we don't need to sign-
extend.

> +			     ADS1262_BITS_PER_SAMPLE - 1);
> +
> +	return 0;
> +}
> +
> +static int ads1262_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	struct ads1262_private *spi = iio_priv(indio_dev);
> +	s64 temp;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ads1262_read(spi, chan, val);

Need to check return value.

> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:
> +			*val = ADS1262_VOLTAGE_INT_REF_uV;
> +			*val2 = chan->scan_type.realbits;

Do we need realbits - 1 since this is signed?

> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		case IIO_TEMP:
> +			temp = (s64)ADS1262_VOLTAGE_INT_REF_uV * MILLI;
> +			temp /= ADS1262_TEMP_SENSITIVITY_uV_per_C;
> +			*val = temp;
> +			*val2 = chan->scan_type.realbits;
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		return ads1262_get_samp_freq(spi, val);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info ads1262_info = {
> +	.read_raw = ads1262_read_raw,
> +};
> +
> +static void ads1262_stop(void *ptr)
> +{
> +	struct ads1262_private *adc = ptr;
> +
> +	ads1262_write_cmd(adc, ADS1262_CMD_STOP1);
> +}
> +
> +static int ads1262_probe(struct spi_device *spi)
> +{
> +	struct ads1262_private *adc;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	adc = iio_priv(indio_dev);
> +	adc->spi = spi;
> +
> +	spi->mode = SPI_MODE_1;

We can leave this out and just rely on the devicetree to set it correctly.

> +	spi->max_speed_hz = ADS1262_SPI_BUS_SPEED_SLOW;

I found out the hard way, this isn't a good idea. You did it right by having
the register read/write functions specify the speed per xfer. Don't set the
speed here so that reading sample data can use the actual max rate from the
devicetree.

> +	spi_set_drvdata(spi, indio_dev);

There isn't spi_get_drvdata() so this isn't needed.

> +
> +	indio_dev->dev.parent = &spi->dev;
> +	indio_dev->name = spi_get_device_id(spi)->name;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = ads1262_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(ads1262_channels);
> +	indio_dev->info = &ads1262_info;
> +
> +	ret = ads1262_reg_read(adc, ADS1262_REG_ID);
> +	if (ret)
> +		return ret;
> +
> +	if (adc->rx_buffer[2] != ADS1262_REG_ID)
> +		dev_err_probe(&spi->dev, -EINVAL, "Wrong device ID 0x%x\n",
> +			      adc->rx_buffer[2]);

Proably don't want to error here. It tends to cause problems if there is ever
a new compatibile chip with different ID.

> +
> +	ret = devm_add_action_or_reset(&spi->dev, ads1262_stop, adc);

We are using single pulse mode, so it doesn't seem like we would need to do
this at driver exit.

> +	if (ret)
> +		return ret;
> +
> +	ret = ads1262_init(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(&spi->dev, indio_dev);
> +}
> +
> +static struct spi_device_id ads1262_id_table[] = {
> +	{ "ads1262" },
> +	{}

	{ }

> +};
> +MODULE_DEVICE_TABLE(spi, ads1262_id_table);
> +
> +static const struct of_device_id ads1262_of_match[] = {
> +	{ .compatible = "ti,ads1262" },
> +	{},

	{ }

(It's the style we use in the IIO subsystem)

> +};
> +MODULE_DEVICE_TABLE(of, ads1262_of_match);
> +
> +static struct spi_driver ads1262_driver = {
> +	.driver = {
> +		.name = "ads1262",
> +		.of_match_table = ads1262_of_match,
> +	},
> +	.probe = ads1262_probe,
> +	.id_table = ads1262_id_table,
> +};
> +module_spi_driver(ads1262_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Sayyad Abid <sayyad.abid16@gmail.com>");
> +MODULE_DESCRIPTION("TI ADS1262 ADC");

Re: [RFC PATCH 1/5] iio: adc: ti-ads1262.c: add initial driver for TI ADS1262 ADC

[Andy Shevchenko]

On Thu, May 01, 2025 at 12:37:30PM -0500, David Lechner wrote:
> On 5/1/25 5:00 AM, Sayyad Abid wrote:
> > Add the core driver file `ti-ads1262.c` for the TI ADS1262 ADC.
> > This initial version implements basic IIO functionality for device
> > probe via SPI and reading raw voltage samples from input channels.

...

> > +#include <linux/kernel.h>
> 
> This header includes too much, please use more specific headers.
> 
> > +#include <linux/device.h>

Ditto for this one. Include it only if really required, otherwise we have often
used dev_printk.h, device/devres.h.

> > +#include <linux/module.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/delay.h>
> 
> Alphabetical order is preferred.

> > +#include <linux/spi/spi.h>
> > +#include <linux/unaligned.h>

Also many headers are missing (probably due to inclusion of kernel.h).

...

> > +#define ADS1262_SETTLE_TIME_USECS	10000

_US is fine (no need to have longer _USECS, which is not so standard).

Also
(10 * USEC_PER_MSEC)

...

> > +/* The Read/Write commands require 4 tCLK to encode and decode, for speeds
> > + * 2x the clock rate, these commands would require extra time between the
> > + * command byte and the data. A simple way to tacke this issue is by
> > + * limiting the SPI bus transfer speed while accessing registers.
> > + */

/*
 * Wrong style for multi-line comments, please use
 * this as an example. Fix all comments in the file
 * accordingly.
 */

...

> > +/* For reading and writing we need a buffer of size 3bytes*/

Missing space.

...

> > +/**
> > + * struct ads1262_private - ADS1262 ADC private data structure
> > + * @spi: SPI device structure
> > + * @reset_gpio: GPIO descriptor for reset pin
> > + * @prev_channel: Previously selected channel for MUX configuration
> > + * @cmd_buffer: Buffer for SPI command transfers
> > + * @rx_buffer: Buffer for SPI data reception
> > + */
> > +struct ads1262_private {
> > +	struct spi_device *spi;

Is it really used? Or is struct device *dev just enough?

> > +	struct gpio_desc *reset_gpio;
> > +	u8 prev_channel;
> > +	u8 cmd_buffer[ADS1262_SPI_CMD_BUFFER_SIZE];
> > +	u8 rx_buffer[ADS1262_SPI_RDATA_BUFFER_SIZE] __aligned(IIO_DMA_MINALIGN);
> 
> cmd_buffer is also used with SPI, so __aligned(IIO_DMA_MINALIGN); needs to go
> there instead.
> 
> > +};

...

> > +#define ADS1262_CHAN(index)						\
> > +{									\
> > +	.type = IIO_VOLTAGE,						\
> > +	.indexed = 1,							\
> > +	.channel = index,						\
> > +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
> > +	.scan_index = index,						\
> > +	.scan_type = {							\
> > +		.sign = 's',						\
> > +		.realbits = ADS1262_BITS_PER_SAMPLE,			\
> > +		.storagebits = 32,					\
> > +		.endianness = IIO_CPU					\

Leave trailing comma here and in the similar cases (when it's not a clear
terminator entry).

> > +	},								\
> > +}

...

> > +static int ads1262_write_cmd(struct ads1262_private *priv, u8 command)
> > +{
> > +	struct spi_transfer xfer = {
> > +		.tx_buf = priv->cmd_buffer,
> > +		.rx_buf = priv->rx_buffer,
> > +		.len = ADS1262_SPI_RDATA_BUFFER_SIZE,
> > +		.speed_hz = ADS1262_CLK_RATE_HZ,
> > +	};
> > +
> > +	priv->cmd_buffer[0] = command;
> > +
> > +	return spi_sync_transfer(priv->spi, &xfer, 1);
> > +}
> > +
> > +static int ads1262_reg_write(void *context, unsigned int reg, unsigned int val)
> > +{
> > +	struct ads1262_private *priv = context;
> > +
> > +	priv->cmd_buffer[0] = ADS1262_CMD_WREG | reg;
> > +	priv->cmd_buffer[1] = 0;
> > +	priv->cmd_buffer[2] = val;
> > +
> > +	return spi_write(priv->spi, &priv->cmd_buffer[0], 3);
> > +}

Can't you use regmap SPI instead?

...

> > +static int ads1262_reg_read(void *context, unsigned int reg)
> > +{
> > +	struct ads1262_private *priv = context;
> > +	struct spi_transfer reg_read_xfer = {
> > +		.tx_buf = priv->cmd_buffer,
> > +		.rx_buf = priv->cmd_buffer,
> > +		.len = 3,
> > +		.speed_hz = ADS1262_CLK_RATE_HZ,
> > +	};
> > +	int ret;
> > +
> > +	priv->cmd_buffer[0] = ADS1262_CMD_RREG | reg;
> > +	priv->cmd_buffer[1] = 0;
> > +	priv->cmd_buffer[2] = 0;
> > +
> > +	ret = spi_sync_transfer(priv->spi, &reg_read_xfer, 1);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> 
> Why not use regmap? You will still custom read/write functions similar to these
> because of needing the lower SCLK rate, but it will give you a bunch of other
> nice features for free.

Ah, same comment above :-)

...

> > +static int ads1262_reset(struct iio_dev *indio_dev)
> > +{
> > +	struct ads1262_private *priv = iio_priv(indio_dev);
> > +
> > +	if (priv->reset_gpio) {
> > +		gpiod_set_value(priv->reset_gpio, 0);
> > +		usleep_range(200, 300);
> 
> Use fsleep(). Also, could make this clear that it is 4 tCLK cycles (the hard-
> coded value would have to be changed if external clock support was added).
> 
> > +		gpiod_set_value(priv->reset_gpio, 1);
> 
> The DT bindings will take care of active low, so this looks backwards. Also
> st->reset_gpio is never assigned, so this is dead code.
> 
> > +	} else {

Redundant else. Just return from the conditional and have the below outside of
it.

> > +		return ads1262_write_cmd(priv, ADS1262_CMD_RESET);
> > +	}

Missing blank line, but see above.

> > +	return 0;
> > +}

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 1/5] iio: adc: ti-ads1262.c: add initial driver for TI ADS1262 ADC

[Jonathan Cameron]

On Thu,  1 May 2025 15:30:39 +0530
Sayyad Abid <sayyad.abid16@gmail.com> wrote:

> Add the core driver file `ti-ads1262.c` for the TI ADS1262 ADC.
> This initial version implements basic IIO functionality for device
> probe via SPI and reading raw voltage samples from input channels.
> 
> Signed-off-by: Sayyad Abid <sayyad.abid16@gmail.com>
A few additional comments from me (some probably overlap!)

Jonathan

> ---
>  drivers/iio/adc/ti-ads1262.c | 438 +++++++++++++++++++++++++++++++++++
>  1 file changed, 438 insertions(+)
>  create mode 100644 drivers/iio/adc/ti-ads1262.c
> 
> diff --git a/drivers/iio/adc/ti-ads1262.c b/drivers/iio/adc/ti-ads1262.c
> new file mode 100644
> index 000000000000..ef34c528ffeb
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads1262.c
> @@ -0,0 +1,438 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * IIO driver for Texas Instruments ADS1662 32-bit ADC
> + *
> + * Datasheet: https://www.ti.com/product/ADS1262
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/delay.h>
> +#include <linux/spi/spi.h>
> +#include <linux/unaligned.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/triggered_buffer.h>
I'm not seeing these being used yet.

> +/**
> + * struct ads1262_private - ADS1262 ADC private data structure
> + * @spi: SPI device structure
> + * @reset_gpio: GPIO descriptor for reset pin
> + * @prev_channel: Previously selected channel for MUX configuration
> + * @cmd_buffer: Buffer for SPI command transfers
> + * @rx_buffer: Buffer for SPI data reception
> + */
> +struct ads1262_private {
> +	struct spi_device *spi;
> +	struct gpio_desc *reset_gpio;
> +	u8 prev_channel;
> +	u8 cmd_buffer[ADS1262_SPI_CMD_BUFFER_SIZE];

You seem to use cmd_buffer directly in spi calls so that needs the __aligned(IIO_DMA_MINALIGN)
marking. You can probably just have one on that as long as you don't ever edit that
whilst rx_buffer is still in use.

> +	u8 rx_buffer[ADS1262_SPI_RDATA_BUFFER_SIZE] __aligned(IIO_DMA_MINALIGN);
> +};
> +
> +#define ADS1262_CHAN(index)						\
> +{									\
> +	.type = IIO_VOLTAGE,						\
> +	.indexed = 1,							\
> +	.channel = index,						\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
> +	.scan_index = index,						\
> +	.scan_type = {							\
> +		.sign = 's',						\
> +		.realbits = ADS1262_BITS_PER_SAMPLE,			\
> +		.storagebits = 32,					\
> +		.endianness = IIO_CPU					\
> +	},								\
> +}
> +
> +#define ADS1262_DIFF_CHAN(index, pos_channel, neg_channel)		\
> +{									\
> +	.type = IIO_VOLTAGE,						\
> +	.indexed = 1,							\
> +	.channel = pos_channel,						\
> +	.channel2 = neg_channel,					\
> +	.differential = 1,						\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
> +	.scan_index = index,						\
> +	.scan_type = {							\
> +		.sign = 's',						\
> +		.realbits = ADS1262_BITS_PER_SAMPLE,			\
> +		.storagebits = 32,					\
> +		.endianness = IIO_CPU					\
> +	},								\
> +}
> +
> +#define ADS1262_TEMP_CHAN(index)					\
> +{									\
> +	.type = IIO_TEMP,						\
> +	.indexed = 1,							\
> +	.channel = index,						\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
> +			     BIT(IIO_CHAN_INFO_SCALE) |			\
> +			     BIT(IIO_CHAN_INFO_SAMP_FREQ),		\

Is the sampling frequency independent for each channel?
Or is this a case where we want them separate as the overall sampling frequency
is directly related to how many channels are enabled?  Odd to just have
this for the temp channel.

> +	.scan_index = index,						\
> +	.scan_type = {							\
> +		.sign = 's',						\
> +		.realbits = ADS1262_BITS_PER_SAMPLE,			\
> +		.storagebits = 32,					\
> +		.endianness = IIO_BE,					\
> +	},								\
> +}
> +
> +static const struct iio_chan_spec ads1262_channels[] = {
> +	/* Single ended channels*/
> +	ADS1262_CHAN(0),
> +	ADS1262_CHAN(1),
> +	ADS1262_CHAN(2),
> +	ADS1262_CHAN(3),
> +	ADS1262_CHAN(4),
> +	ADS1262_CHAN(5),
> +	ADS1262_CHAN(6),
> +	ADS1262_CHAN(7),
> +	ADS1262_CHAN(8),
> +	ADS1262_CHAN(9),
> +	/* The channel at index 10 is AINCOM, which is the common ground
Comment syntax
	/*
	 * The channel

> +	 * of the ADC. It is not a valid channel for the user.
> +	 */
> +
> +	/* Temperature and Monitor channels */
> +	ADS1262_TEMP_CHAN(11),	/* TEMP SENSOR */
> +	ADS1262_CHAN(12),	/* AVDD MON */
> +	ADS1262_CHAN(13),	/* DVDD MON */
> +	ADS1262_CHAN(14),	/* TDAC TEST */
> +	/* Differential channels */
> +	ADS1262_DIFF_CHAN(15, 0, 1),	/* AIN0 - AIN1 */
> +	ADS1262_DIFF_CHAN(16, 2, 3),	/* AIN2 - AIN3 */
> +	ADS1262_DIFF_CHAN(17, 4, 5),	/* AIN4 - AIN5 */
> +	ADS1262_DIFF_CHAN(18, 6, 7),	/* AIN6 - AIN7 */
> +	ADS1262_DIFF_CHAN(19, 8, 9),	/* AIN8 - AIN9 */
> +};
> +
> +static int ads1262_write_cmd(struct ads1262_private *priv, u8 command)
> +{
> +	struct spi_transfer xfer = {
> +		.tx_buf = priv->cmd_buffer,
> +		.rx_buf = priv->rx_buffer,
> +		.len = ADS1262_SPI_RDATA_BUFFER_SIZE,
> +		.speed_hz = ADS1262_CLK_RATE_HZ,
> +	};
> +
> +	priv->cmd_buffer[0] = command;
> +
> +	return spi_sync_transfer(priv->spi, &xfer, 1);
> +}
> +
> +static int ads1262_reg_write(void *context, unsigned int reg, unsigned int val)
> +{
> +	struct ads1262_private *priv = context;
> +
> +	priv->cmd_buffer[0] = ADS1262_CMD_WREG | reg;
> +	priv->cmd_buffer[1] = 0;
> +	priv->cmd_buffer[2] = val;
> +
> +	return spi_write(priv->spi, &priv->cmd_buffer[0], 3);
> +}
> +
> +static int ads1262_reg_read(void *context, unsigned int reg)
> +{
> +	struct ads1262_private *priv = context;
> +	struct spi_transfer reg_read_xfer = {
> +		.tx_buf = priv->cmd_buffer,
> +		.rx_buf = priv->cmd_buffer,
> +		.len = 3,
> +		.speed_hz = ADS1262_CLK_RATE_HZ,
> +	};
> +	int ret;
> +
> +	priv->cmd_buffer[0] = ADS1262_CMD_RREG | reg;
> +	priv->cmd_buffer[1] = 0;
> +	priv->cmd_buffer[2] = 0;
> +
> +	ret = spi_sync_transfer(priv->spi, &reg_read_xfer, 1);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
	return spi_sync_transfer()
> +}
> +
> +static int ads1262_reset(struct iio_dev *indio_dev)
> +{
> +	struct ads1262_private *priv = iio_priv(indio_dev);
> +
> +	if (priv->reset_gpio) {
> +		gpiod_set_value(priv->reset_gpio, 0);
> +		usleep_range(200, 300);
> +		gpiod_set_value(priv->reset_gpio, 1);
		return 0;
	}

	return ads1262_write_cmd();


> +	} else {
> +		return ads1262_write_cmd(priv, ADS1262_CMD_RESET);
> +	}
> +	return 0;
> +}
> +
> +static int ads1262_init(struct iio_dev *indio_dev)
> +{
> +	struct ads1262_private *priv = iio_priv(indio_dev);
> +	int ret = 0;
> +
> +	ret = ads1262_reset(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	/* 10 milliseconds settling time for the ADC to stabilize */
> +	fsleep(ADS1262_SETTLE_TIME_USECS);

This is probably better placed in the reset function as it is part
of that overall action.  I added a comment asking why there wasn't one there
before seeing this.

> +
> +	/* Clearing the RESET bit in the power register to detect ADC reset */
> +	ret = ads1262_reg_write(priv, ADS1262_REG_POWER, ADS1262_INTREF_ENABLE);

I'd be tempted to do this in the _reset() function as well given
it is really just another part of that.

> +	if (ret)
> +		return ret;
> +
> +	/* Setting the ADC to one-shot conversion mode */
> +	ret = ads1262_reg_write(priv, ADS1262_REG_MODE0, ADS1262_MODE0_ONE_SHOT);
> +	if (ret)
> +		return ret;
> +
> +	ret = ads1262_reg_read(priv, ADS1262_REG_INPMUX);
> +	if (ret)
> +		return ret;
> +
> +	priv->prev_channel = priv->cmd_buffer[2];
> +
> +	return ret;
> +}
> +
> +static int ads1262_get_samp_freq(struct ads1262_private *priv, int *val)
> +{
> +	unsigned long samp_freq;
> +	int ret;
> +
> +	ret = ads1262_reg_read(priv, ADS1262_REG_MODE2);
> +	if (ret)
> +		return ret;
> +
> +	samp_freq = priv->cmd_buffer[2];
There are multiple calls that use this structure. I'd kind of
expect to see some locking to prevent them racing against each other.
Note sysfs etc provide no prevention against such races.
> +
> +	*val = (samp_freq & ADS1262_MASK_MODE2_DR);

No brackets needed.

> +
> +	return IIO_VAL_INT;
> +}
> +
> +/**
> + * ads1262_read - Read a single sample from the ADC
> + * @priv: Pointer to the ADS1262 private data structure
> + * @chan: Pointer to the IIO channel specification
> + * @val: Pointer to store the read value
> + *
> + * Reads a single sample from the specified ADC channel. For differential
> + * channels, it sets up the MUX with both channels. For single-ended channels,
> + * it uses the channel number and AINCOM (0x0A).
> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +static int ads1262_read(struct ads1262_private *priv,
> +			struct iio_chan_spec const *chan, int *val)
> +{
> +	u8 mux_value;
> +	int ret;
> +
> +	if (chan->differential) {
> +		mux_value = (chan->channel << 4) | chan->channel2;

FIELD_PREP() and appropriate masks would make it easier to see what is
going on here.

> +	} else {
> +		/* For single-ended channels, use the channel number on one end
		/*
		 * For single-ended channels...

> +		 * and AINCOM (0x0A) on the other end
> +		 */
> +		mux_value = (chan->channel << 4) | 0x0A;
> +	}
> +
> +	if (mux_value != priv->prev_channel) {
Noting stops this racing with another read of a different channel.
Given you are trying to keep state consistent you need
a mutex in your priv structure to serialize them.

> +		ret = ads1262_write_cmd(priv, ADS1262_CMD_STOP1);
> +		if (ret)
> +			return ret;
> +
> +		ret = ads1262_reg_write(priv, ADS1262_REG_INPMUX, mux_value);
> +		if (ret)
> +			return ret;
> +
> +		priv->prev_channel = mux_value;
> +	}
> +
> +	ret = ads1262_write_cmd(priv, ADS1262_CMD_START1);
> +	if (ret)
> +		return ret;
> +
> +	usleep_range(1000, 2000);
> +
> +	*val = sign_extend64(get_unaligned_be32(priv->rx_buffer + 1),
> +			     ADS1262_BITS_PER_SAMPLE - 1);
> +
> +	return 0;
> +}
> +
> +static int ads1262_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	struct ads1262_private *spi = iio_priv(indio_dev);
> +	s64 temp;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ads1262_read(spi, chan, val);
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:
> +			*val = ADS1262_VOLTAGE_INT_REF_uV;
> +			*val2 = chan->scan_type.realbits;
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		case IIO_TEMP:
> +			temp = (s64)ADS1262_VOLTAGE_INT_REF_uV * MILLI;
> +			temp /= ADS1262_TEMP_SENSITIVITY_uV_per_C;
> +			*val = temp;
> +			*val2 = chan->scan_type.realbits;
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		return ads1262_get_samp_freq(spi, val);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info ads1262_info = {
> +	.read_raw = ads1262_read_raw,
> +};
> +
> +static void ads1262_stop(void *ptr)
> +{
> +	struct ads1262_private *adc = ptr;
> +
> +	ads1262_write_cmd(adc, ADS1262_CMD_STOP1);
> +}
> +
> +static int ads1262_probe(struct spi_device *spi)
> +{
> +	struct ads1262_private *adc;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	adc = iio_priv(indio_dev);
> +	adc->spi = spi;
> +
> +	spi->mode = SPI_MODE_1;
> +	spi->max_speed_hz = ADS1262_SPI_BUS_SPEED_SLOW;

David covered these...

> +	spi_set_drvdata(spi, indio_dev);
> +
> +	indio_dev->dev.parent = &spi->dev;
> +	indio_dev->name = spi_get_device_id(spi)->name;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = ads1262_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(ads1262_channels);
> +	indio_dev->info = &ads1262_info;
> +
> +	ret = ads1262_reg_read(adc, ADS1262_REG_ID);
> +	if (ret)
> +		return ret;
> +
> +	if (adc->rx_buffer[2] != ADS1262_REG_ID)
> +		dev_err_probe(&spi->dev, -EINVAL, "Wrong device ID 0x%x\n",
> +			      adc->rx_buffer[2]);
> +
> +	ret = devm_add_action_or_reset(&spi->dev, ads1262_stop, adc);

Which call is this 'undoing'.  Generally I'd expect to only see
a devm callback register after what it is setting up.
Here I'm assuming that stop is undoing something in init?
If so register this after ads1262_init()


> +	if (ret)
> +		return ret;
> +
> +	ret = ads1262_init(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(&spi->dev, indio_dev);
> +}
> +
> +static struct spi_device_id ads1262_id_table[] = {
> +	{ "ads1262" },
> +	{}
	
	{ }

> +};
> +MODULE_DEVICE_TABLE(spi, ads1262_id_table);
> +
> +static const struct of_device_id ads1262_of_match[] = {
> +	{ .compatible = "ti,ads1262" },
> +	{},

	{ }

Random choice I made for IIO as I'd like consistency.
> +};
> +MODULE_DEVICE_TABLE(of, ads1262_of_match);

[RFC PATCH 2/5] iio: adc: Kconfig: add Kconfig entry for TI ADS1262 driver

[Sayyad Abid]

Adds the Kconfig option `CONFIG_TI_ADS1262` under the IIO ADC menu.
This allows users to select the TI ADS1262 driver for compilation
during kernel configuration.

Signed-off-by: Sayyad Abid <sayyad.abid16@gmail.com>
---
 drivers/iio/adc/Kconfig | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 6529df1a498c..f3f8a8cf5f89 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1542,6 +1542,18 @@ config TI_ADS1100
 	  This driver can also be built as a module. If so, the module will be
 	  called ti-ads1100.
 
+config TI_ADS1262
+	tristate "Texas Instruments ADS1262"
+	depends on SPI
+	select IIO_BUFFER
+	help
+	  If you say yes here you get support for Texas Instruments ADS1262
+	  32-bit precision ADC with programmable gain amplifier and internal
+	  voltage reference.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called ti-ads1262.
+
 config TI_ADS1298
 	tristate "Texas Instruments ADS1298"
 	depends on SPI
-- 
2.39.5

Re: [RFC PATCH 2/5] iio: adc: Kconfig: add Kconfig entry for TI ADS1262 driver

[David Lechner]

On 5/1/25 5:00 AM, Sayyad Abid wrote:
> Adds the Kconfig option `CONFIG_TI_ADS1262` under the IIO ADC menu.
> This allows users to select the TI ADS1262 driver for compilation
> during kernel configuration.
> 
> Signed-off-by: Sayyad Abid <sayyad.abid16@gmail.com>
> ---
This can be in the same patch as the driver. Same with the makefile - it doesn't
need to be a separate patch.

Re: [RFC PATCH 2/5] iio: adc: Kconfig: add Kconfig entry for TI ADS1262 driver

[Jonathan Cameron]

On Thu, 1 May 2025 12:37:35 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 5/1/25 5:00 AM, Sayyad Abid wrote:
> > Adds the Kconfig option `CONFIG_TI_ADS1262` under the IIO ADC menu.
> > This allows users to select the TI ADS1262 driver for compilation
> > during kernel configuration.
> > 
> > Signed-off-by: Sayyad Abid <sayyad.abid16@gmail.com>
> > ---  
> This can be in the same patch as the driver. Same with the makefile - it doesn't
> need to be a separate patch.

Change that from can to should / must.  There are some bits of the kernel
that run a different approach for complex multi patch drivers but for
IIO we want everything to be building at each step of the series.
Personally I see that as good practice and I don't know of any part of the
kernel where the maintainers reject that approach, just some where they
don't mind as much if a complex driver is built up and only compiled in
the final patch

Thanks,

Jonathan

[RFC PATCH 3/5] iio: adc: Makefile: add compile support for TI ADS1262 driver

[Sayyad Abid]

This ensures the driver object file (`ti-ads1262.o`) is built and
linked into the kernel when the corresponding Kconfig option
(`CONFIG_TI_ADS1262`) is enabled.

Signed-off-by: Sayyad Abid <sayyad.abid16@gmail.com>
---
 drivers/iio/adc/Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 3e918c3eec69..14192944c225 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -134,6 +134,7 @@ obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
 obj-$(CONFIG_TI_ADS1100) += ti-ads1100.o
 obj-$(CONFIG_TI_ADS1119) += ti-ads1119.o
 obj-$(CONFIG_TI_ADS124S08) += ti-ads124s08.o
+obj-$(CONFIG_TI_ADS1262) += ti-ads1262.o
 obj-$(CONFIG_TI_ADS1298) += ti-ads1298.o
 obj-$(CONFIG_TI_ADS131E08) += ti-ads131e08.o
 obj-$(CONFIG_TI_ADS7138) += ti-ads7138.o
-- 
2.39.5

[RFC PATCH 4/5] MAINTAINERS: add entry for TI ADS1262 ADC driver

[Sayyad Abid]

Add a new MAINTAINERS section for the TI ADS1262 ADC driver, which includes
the main source file and the device tree binding documentation.

Signed-off-by: Sayyad Abid <sayyad.abid16@gmail.com>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 3cbf9ac0d83f..10b2e9293a99 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -24187,6 +24187,13 @@ S:	Supported
 F:	Documentation/devicetree/bindings/iio/adc/ti,ads7924.yaml
 F:	drivers/iio/adc/ti-ads7924.c
 
+TI ADS1262 ADC DRIVER
+M:	Sayyad Abid <sayyad.abid16@gmail.com>
+L:	linux-iio@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/adc/ti,ads1262.yaml
+F:	drivers/iio/adc/ads1262.c
+
 TI AM437X VPFE DRIVER
 M:	"Lad, Prabhakar" <prabhakar.csengg@gmail.com>
 L:	linux-media@vger.kernel.org
-- 
2.39.5

Re: [RFC PATCH 4/5] MAINTAINERS: add entry for TI ADS1262 ADC driver

[Jonathan Cameron]

On Thu,  1 May 2025 15:30:42 +0530
Sayyad Abid <sayyad.abid16@gmail.com> wrote:

> Add a new MAINTAINERS section for the TI ADS1262 ADC driver, which includes
> the main source file and the device tree binding documentation.
> 
> Signed-off-by: Sayyad Abid <sayyad.abid16@gmail.com>
Better to add this (and build it up in stages) as part of the patch
that introduces the 1st file which should be the DT binding.  

That way checkpatch won't moan ;)

Jonathan


> ---
>  MAINTAINERS | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3cbf9ac0d83f..10b2e9293a99 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -24187,6 +24187,13 @@ S:	Supported
>  F:	Documentation/devicetree/bindings/iio/adc/ti,ads7924.yaml
>  F:	drivers/iio/adc/ti-ads7924.c
>  
> +TI ADS1262 ADC DRIVER
> +M:	Sayyad Abid <sayyad.abid16@gmail.com>
> +L:	linux-iio@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/iio/adc/ti,ads1262.yaml
> +F:	drivers/iio/adc/ads1262.c
> +
>  TI AM437X VPFE DRIVER
>  M:	"Lad, Prabhakar" <prabhakar.csengg@gmail.com>
>  L:	linux-media@vger.kernel.org

[RFC PATCH 5/5] dt-bindings: iio: adc: add bindings for TI ADS1262

[Sayyad Abid]

Add the Device Tree binding documentation for the Texas Instruments ADS1262 ADC.

Signed-off-by: Sayyad Abid <sayyad.abid16@gmail.com>
---
 .../bindings/iio/adc/ti,ads1262.yaml          | 189 ++++++++++++++++++
 1 file changed, 189 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1262.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads1262.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads1262.yaml
new file mode 100644
index 000000000000..8c4cc2cf6467
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/ti,ads1262.yaml
@@ -0,0 +1,189 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/ti,ads1262.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Texas Instruments' ADS1262 32-Bit Analog to Digital Converter
+
+maintainers:
+  - Sayyad Abid <sayyad.abid16@gmail.com>
+
+description: |
+  Texas Instruments ADS1262 32-Bit Analog to Digital Converter with,
+  internal temperature sensor, GPIOs and PGAs
+
+  The ADS1262 is a 32-bit, 38-kSPS, precision ADC with a programmable gain
+  amplifier (PGA) and internal voltage reference. It features:
+  - 11 single-ended or 5 differential input channels
+  - Internal temperature sensor
+  - Programmable gain amplifier (PGA) with gains from 1 to 32
+  - Internal voltage reference
+  - GPIO pins for control and monitoring
+  - SPI interface
+
+  Specifications about the part can be found at:
+  https://www.ti.com/product/ADS1262
+
+properties:
+  compatible:
+    enum:
+      - ti,ads1262
+
+  reg:
+    maxItems: 1
+    description: SPI chip select number
+
+  spi-max-frequency:
+    maximum: 7372800
+    description: Maximum SPI clock frequency in Hz (7.3728 MHz)
+
+  spi-cpha:
+    type: boolean
+    description: Required for SPI mode 1 operation
+
+  reset-gpios:
+    maxItems: 1
+    description: GPIO specifier for the reset pin (active low)
+
+  vref-supply:
+    description: |
+      The regulator supply for ADC reference voltage. If not specified,
+      the internal 2.5V reference will be used.
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+  '#io-channel-cells':
+    const: 1
+
+  ti,pga-bypass:
+    type: boolean
+    description: |
+      If true, bypass the PGA. If false or not specified, PGA is enabled.
+
+  ti,data-rate:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 0
+    maximum: 15
+    description: |
+      Data acquisition rate in samples per second
+      0: 2.5
+      1: 5
+      2: 10
+      3: 16.6
+      4: 20
+      5: 50
+      6: 60
+      7: 100
+      8: 400
+      9: 1200
+      10: 2400
+      11: 4800
+      12: 7200
+      13: 14400
+      14: 19200
+      15: 38400
+
+required:
+  - compatible
+  - reg
+  - spi-cpha
+  - '#address-cells'
+  - '#size-cells'
+  - '#io-channel-cells'
+
+additionalProperties: false
+
+patternProperties:
+  "^channel@([0-9]|1[0-1])$":
+    type: object
+    additionalProperties: false
+    description: |
+      Represents the external channels which are connected to the ADC.
+      Channels 0-9 are available for external signals, channel 10 is AINCOM,
+      and channel 11 is the internal temperature sensor.
+
+    properties:
+      reg:
+        description: |
+          Channel number. It can have up to 10 channels numbered from 0 to 9,
+          channel 10 is AINCOM, and channel 11 is the internal temperature sensor.
+        items:
+          - minimum: 0
+            maximum: 11
+
+      diff-channels:
+        description: |
+          List of two channel numbers for differential measurement.
+          First number is positive input, second is negative input.
+          Not applicable for temperature sensor (channel 11).
+        items:
+          - minimum: 0
+            maximum: 9
+          - minimum: 0
+            maximum: 9
+
+      ti,gain:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        minimum: 0
+        maximum: 5
+        description: |
+          PGA gain setting. Not applicable for temperature sensor (channel 11).
+          0: 1 (default)
+          1: 2
+          2: 4
+          3: 8
+          4: 16
+          5: 32
+
+    required:
+      - reg
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    spi {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      ads1262: adc@0 {
+        compatible = "ti,ads1262";
+        reg = <0>;
+        spi-max-frequency = <7372800>;
+        vref-supply = <&adc_vref>;
+        spi-cpha;
+        reset-gpios = <&gpio1 16 GPIO_ACTIVE_LOW>;
+        ti,pga-bypass;
+        ti,data-rate = <15>; /* 38400 SPS */
+
+        #address-cells = <1>;
+        #size-cells = <0>;
+        #io-channel-cells = <1>;
+
+        /* Single-ended channel */
+        channel@0 {
+          reg = <0>;
+        };
+
+        /* Differential channel */
+        channel@1 {
+          reg = <1>;
+          diff-channels = <1 2>;
+          ti,gain = <2>; /* Gain of 4 */
+        };
+
+        /* Temperature sensor */
+        channel@11 {
+          reg = <11>;
+        };
+      };
+    };
+...
-- 
2.39.5

Re: [RFC PATCH 5/5] dt-bindings: iio: adc: add bindings for TI ADS1262

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3102 bytes --]

On Thu, May 01, 2025 at 03:30:43PM +0530, Sayyad Abid wrote:

> +properties:
> +  compatible:
> +    enum:
> +      - ti,ads1262

How different is the ads1263? Do we get support for both "for free"?

> +  spi-cpha:
> +    type: boolean
> +    description: Required for SPI mode 1 operation

This should just collapse to "spi-cpha: true", cos the definition of it
comes from spi-peripheral-props.yaml.

> +
> +  reset-gpios:
> +    maxItems: 1
> +    description: GPIO specifier for the reset pin (active low)
> +
> +  vref-supply:
> +    description: |
> +      The regulator supply for ADC reference voltage. If not specified,
> +      the internal 2.5V reference will be used.

I looked this device up, I don't see an input pin called "vref" and
there appear to be multiple reference inputs. All supplies should be
documented.

> +      ti,gain:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        minimum: 0
> +        maximum: 5
> +        description: |
> +          PGA gain setting. Not applicable for temperature sensor (channel 11).
> +          0: 1 (default)
> +          1: 2
> +          2: 4
> +          3: 8
> +          4: 16
> +          5: 32

Why can't the gain be in it's actual unit, rather than what I am
guessing is a register value?

> +  ti,data-rate:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0
> +    maximum: 15
> +    description: |
> +      Data acquisition rate in samples per second
> +      0: 2.5
> +      1: 5
> +      2: 10
> +      3: 16.6
> +      4: 20
> +      5: 50
> +      6: 60
> +      7: 100
> +      8: 400
> +      9: 1200
> +      10: 2400
> +      11: 4800
> +      12: 7200
> +      13: 14400
> +      14: 19200
> +      15: 38400

Same applies here really, except that the fractional per second rate
would only work if the base unit was samples-per-<something sub second>,
which might or might not be worth it.

> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    spi {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      ads1262: adc@0 {

The label here is unused AFAICT and should be dropped.

Cheers,
Conor.

> +        compatible = "ti,ads1262";
> +        reg = <0>;
> +        spi-max-frequency = <7372800>;
> +        vref-supply = <&adc_vref>;
> +        spi-cpha;
> +        reset-gpios = <&gpio1 16 GPIO_ACTIVE_LOW>;
> +        ti,pga-bypass;
> +        ti,data-rate = <15>; /* 38400 SPS */
> +
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        #io-channel-cells = <1>;
> +
> +        /* Single-ended channel */
> +        channel@0 {
> +          reg = <0>;
> +        };
> +
> +        /* Differential channel */
> +        channel@1 {
> +          reg = <1>;
> +          diff-channels = <1 2>;
> +          ti,gain = <2>; /* Gain of 4 */
> +        };
> +
> +        /* Temperature sensor */
> +        channel@11 {
> +          reg = <11>;
> +        };
> +      };
> +    };
> +...
> -- 
> 2.39.5
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [RFC PATCH 5/5] dt-bindings: iio: adc: add bindings for TI ADS1262

[David Lechner]

On 5/1/25 5:00 AM, Sayyad Abid wrote:
> Add the Device Tree binding documentation for the Texas Instruments ADS1262 ADC.
> 
> Signed-off-by: Sayyad Abid <sayyad.abid16@gmail.com>
> ---

For readers, it logically makes more sense to have this be the first patch in
the series.

>  .../bindings/iio/adc/ti,ads1262.yaml          | 189 ++++++++++++++++++
>  1 file changed, 189 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1262.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads1262.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads1262.yaml
> new file mode 100644
> index 000000000000..8c4cc2cf6467
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/ti,ads1262.yaml
> @@ -0,0 +1,189 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/ti,ads1262.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Texas Instruments' ADS1262 32-Bit Analog to Digital Converter
> +
> +maintainers:
> +  - Sayyad Abid <sayyad.abid16@gmail.com>
> +
> +description: |
> +  Texas Instruments ADS1262 32-Bit Analog to Digital Converter with,
> +  internal temperature sensor, GPIOs and PGAs
> +
> +  The ADS1262 is a 32-bit, 38-kSPS, precision ADC with a programmable gain
> +  amplifier (PGA) and internal voltage reference. It features:
> +  - 11 single-ended or 5 differential input channels
> +  - Internal temperature sensor
> +  - Programmable gain amplifier (PGA) with gains from 1 to 32
> +  - Internal voltage reference
> +  - GPIO pins for control and monitoring
> +  - SPI interface

Normally, I would say that we want to have the devicetree as complete as
possible, but this chip has so many ways to wire it up, it would take days
to do a through review, so not sure where to draw the line. But I think there
are a few obvious ones we could still add, even if the driver doesn't use them
yet.

interrupts: for the /DRDY output

clks: for the clock input (see adi,ad7173 and adi,ad7192 for inspiration)

gpio-controller and #gpio-cells: for AIN pins used as GPIO

And for later...
* voltage supply outputs
* DAC outputs
* common mode voltage inputs
* REF{P.N}{1,2,3} supplies
* per-channel reference selection


> +
> +  Specifications about the part can be found at:
> +  https://www.ti.com/product/ADS1262
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ti,ads1262
> +
> +  reg:
> +    maxItems: 1
> +    description: SPI chip select number
> +
> +  spi-max-frequency:
> +    maximum: 7372800
> +    description: Maximum SPI clock frequency in Hz (7.3728 MHz)

Datasheet says minimum period is 125 ns, so that would make the max 8 MHz.

> +
> +  spi-cpha:
> +    type: boolean
> +    description: Required for SPI mode 1 operation
> +
> +  reset-gpios:
> +    maxItems: 1
> +    description: GPIO specifier for the reset pin (active low)
> +
> +  vref-supply:
> +    description: |
> +      The regulator supply for ADC reference voltage. If not specified,
> +      the internal 2.5V reference will be used.
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +  '#io-channel-cells':
> +    const: 1
> +
> +  ti,pga-bypass:
> +    type: boolean
> +    description: |
> +      If true, bypass the PGA. If false or not specified, PGA is enabled.

Why is this one global but the actual gain per-channel?

> +
> +  ti,data-rate:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0
> +    maximum: 15
> +    description: |
> +      Data acquisition rate in samples per second
> +      0: 2.5
> +      1: 5
> +      2: 10
> +      3: 16.6
> +      4: 20
> +      5: 50
> +      6: 60
> +      7: 100
> +      8: 400
> +      9: 1200
> +      10: 2400
> +      11: 4800
> +      12: 7200
> +      13: 14400
> +      14: 19200
> +      15: 38400

This is something that should be implemented with a sampling_frequency attribute
so that it can be changed at runtime rather than be hard-coded in the devicetree.

> +
> +required:
> +  - compatible
> +  - reg
> +  - spi-cpha
> +  - '#address-cells'
> +  - '#size-cells'
> +  - '#io-channel-cells'
> +
> +additionalProperties: false
> +
> +patternProperties:
> +  "^channel@([0-9]|1[0-1])$":
> +    type: object
> +    additionalProperties: false
> +    description: |
> +      Represents the external channels which are connected to the ADC.
> +      Channels 0-9 are available for external signals, channel 10 is AINCOM,
> +      and channel 11 is the internal temperature sensor.
> +
> +    properties:
> +      reg:
> +        description: |
> +          Channel number. It can have up to 10 channels numbered from 0 to 9,
> +          channel 10 is AINCOM, and channel 11 is the internal temperature sensor.
> +        items:
> +          - minimum: 0
> +            maximum: 11

Why allow the temperature input here but not other diagnostic input? Would make
more sense to me to omit temperature from the devicetree since it doesn't have
different wiring possibilities like the AIN pins.

> +
> +      diff-channels:
> +        description: |
> +          List of two channel numbers for differential measurement.
> +          First number is positive input, second is negative input.
> +          Not applicable for temperature sensor (channel 11).
> +        items:
> +          - minimum: 0
> +            maximum: 9
> +          - minimum: 0
> +            maximum: 9

Shouldn't this have max of 10 to include AINCOM?

> +
> +      ti,gain:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        minimum: 0
> +        maximum: 5
> +        description: |
> +          PGA gain setting. Not applicable for temperature sensor (channel 11).
> +          0: 1 (default)
> +          1: 2
> +          2: 4
> +          3: 8
> +          4: 16
> +          5: 32
> +
> +    required:
> +      - reg
> +
> +allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    spi {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      ads1262: adc@0 {
> +        compatible = "ti,ads1262";
> +        reg = <0>;
> +        spi-max-frequency = <7372800>;
> +        vref-supply = <&adc_vref>;
> +        spi-cpha;
> +        reset-gpios = <&gpio1 16 GPIO_ACTIVE_LOW>;
> +        ti,pga-bypass;

If PGA is bypassed...

> +        ti,data-rate = <15>; /* 38400 SPS */
> +
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        #io-channel-cells = <1>;
> +
> +        /* Single-ended channel */
> +        channel@0 {
> +          reg = <0>;
> +        };
> +
> +        /* Differential channel */
> +        channel@1 {
> +          reg = <1>;
> +          diff-channels = <1 2>;
> +          ti,gain = <2>; /* Gain of 4 */

...then gain doesn't make sense here.

> +        };
> +
> +        /* Temperature sensor */
> +        channel@11 {
> +          reg = <11>;
> +        };
> +      };
> +    };
> +...

Re: [RFC PATCH 5/5] dt-bindings: iio: adc: add bindings for TI ADS1262

[Jonathan Cameron]

On Thu,  1 May 2025 15:30:43 +0530
Sayyad Abid <sayyad.abid16@gmail.com> wrote:

> Add the Device Tree binding documentation for the Texas Instruments ADS1262 ADC.
> 
> Signed-off-by: Sayyad Abid <sayyad.abid16@gmail.com>
Hi Sayyad,

A few additional comments from me.

Jonathan

> ---
>  .../bindings/iio/adc/ti,ads1262.yaml          | 189 ++++++++++++++++++
>  1 file changed, 189 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1262.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads1262.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads1262.yaml
> new file mode 100644
> index 000000000000..8c4cc2cf6467
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/ti,ads1262.yaml
> @@ -0,0 +1,189 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/ti,ads1262.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Texas Instruments' ADS1262 32-Bit Analog to Digital Converter
> +
> +maintainers:
> +  - Sayyad Abid <sayyad.abid16@gmail.com>
> +
> +description: |
> +  Texas Instruments ADS1262 32-Bit Analog to Digital Converter with,
> +  internal temperature sensor, GPIOs and PGAs
> +
> +  The ADS1262 is a 32-bit, 38-kSPS, precision ADC with a programmable gain
> +  amplifier (PGA) and internal voltage reference. It features:
> +  - 11 single-ended or 5 differential input channels
> +  - Internal temperature sensor
> +  - Programmable gain amplifier (PGA) with gains from 1 to 32
> +  - Internal voltage reference
> +  - GPIO pins for control and monitoring
> +  - SPI interface
> +
> +  Specifications about the part can be found at:
> +  https://www.ti.com/product/ADS1262
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ti,ads1262
> +
> +  reg:
> +    maxItems: 1
> +    description: SPI chip select number
> +
> +  spi-max-frequency:
> +    maximum: 7372800
> +    description: Maximum SPI clock frequency in Hz (7.3728 MHz)
> +
> +  spi-cpha:
> +    type: boolean
> +    description: Required for SPI mode 1 operation
> +
> +  reset-gpios:
> +    maxItems: 1
> +    description: GPIO specifier for the reset pin (active low)
> +
> +  vref-supply:
> +    description: |
> +      The regulator supply for ADC reference voltage. If not specified,
> +      the internal 2.5V reference will be used.
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +  '#io-channel-cells':
> +    const: 1
> +
> +  ti,pga-bypass:
> +    type: boolean
> +    description: |
> +      If true, bypass the PGA. If false or not specified, PGA is enabled.

Why do we want to support this?  If we do, why is it a wiring thing (so suitable
for DT) rather than a runtime configuration thing. e.g. If I set the scale to
1 does that mean I can bypass the PGA?

> +
> +  ti,data-rate:
As has been commented, this is a userspace thing to control, not
something we should get from DT.

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0
> +    maximum: 15
> +    description: |
> +      Data acquisition rate in samples per second
> +      0: 2.5
...

> +
> +patternProperties:
> +  "^channel@([0-9]|1[0-1])$":
> +    type: object
> +    additionalProperties: false
> +    description: |
> +      Represents the external channels which are connected to the ADC.
> +      Channels 0-9 are available for external signals, channel 10 is AINCOM,
> +      and channel 11 is the internal temperature sensor.
> +
> +    properties:

Should reference adc.yaml which will provide much of the basic per channel stuff
and ensure standard formats etc.

> +      reg:
> +        description: |
> +          Channel number. It can have up to 10 channels numbered from 0 to 9,
> +          channel 10 is AINCOM, and channel 11 is the internal temperature sensor.
> +        items:
> +          - minimum: 0
> +            maximum: 11
> +
> +      diff-channels:
> +        description: |
> +          List of two channel numbers for differential measurement.
> +          First number is positive input, second is negative input.
> +          Not applicable for temperature sensor (channel 11).
> +        items:
> +          - minimum: 0
> +            maximum: 9
> +          - minimum: 0
> +            maximum: 9
> +
> +      ti,gain:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        minimum: 0
> +        maximum: 5
> +        description: |
> +          PGA gain setting. Not applicable for temperature sensor (channel 11).

Looks like a userspace thing rather than a DT one.

> +          0: 1 (default)
> +          1: 2
> +          2: 4
> +          3: 8
> +          4: 16
> +          5: 32
> +
> +    required:
> +      - reg
> +
> +allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    spi {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      ads1262: adc@0 {
> +        compatible = "ti,ads1262";
> +        reg = <0>;
> +        spi-max-frequency = <7372800>;
> +        vref-supply = <&adc_vref>;
> +        spi-cpha;
> +        reset-gpios = <&gpio1 16 GPIO_ACTIVE_LOW>;
> +        ti,pga-bypass;
> +        ti,data-rate = <15>; /* 38400 SPS */
> +
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        #io-channel-cells = <1>;
> +
> +        /* Single-ended channel */
> +        channel@0 {
> +          reg = <0>;
> +        };
> +
> +        /* Differential channel */
> +        channel@1 {
> +          reg = <1>;
> +          diff-channels = <1 2>;
> +          ti,gain = <2>; /* Gain of 4 */
> +        };
> +
> +        /* Temperature sensor */
> +        channel@11 {
> +          reg = <11>;
> +        };
> +      };
> +    };
> +...

Re: [RFC PATCH 0/5] iio: adc: Add initial support for TI ADS1262

[David Lechner]

On 5/1/25 5:00 AM, Sayyad Abid wrote:
> The ADS1262 is a 32-bit, high-resolution delta-sigma ADC communicating
> over SPI. It's designed for precision measurements.
> 
> This initial driver provides the basic functionality needed to:
>  - Probe and register the device via SPI.
>  - Expose standard IIO channels for reading raw voltage samples.
> 
> Basic testing was performed on a Raspberry Pi Zero 2W using the hardware
> SPI0 interface. The connections used were:
> 
> +-----------------+            +-----------------+
> | RPi Zero 2W     |            | TI ADS1262      |
> | (SPI0 Pins)     |            |                 |
> |-----------------|            |-----------------|
> | MOSI            |----------->| DIN             |
> | MISO            |<-----------| DOUT/DRDY       |
> | SCLK            |----------->| SCLK            |
> | CE0             |----------->| /CS             |
> | 5V              |----------->| DVDD, AVDD      |
> | GND             |----------->| DGND, AGND      |
> +-----------------+            +-----------------+
> 
> I would greatly appreciate any feedback on the driver structure,
> IIO integration, SPI communication handling, or any potential issues
> or areas for improvement you might spot.
> 
> This series is broken down as follows:
>  Patch 1: Adds the core driver code (ti-ads1262.c).
>  Patch 2: Adds the Kconfig option.
>  Patch 3: Adds the Makefile entry for compilation.
>  Patch 4: Adds the MAINTAINERS entry.
> 
> Thanks for your time and consideration.
> 
> Sayyad Abid (5):
>   iio: adc: ti-ads1262.c: add initial driver for TI ADS1262 ADC
>   iio: adc: Kconfig: add Kconfig entry for TI ADS1262 driver
>   iio: adc: Makefile: add compile support for TI ADS1262 driver
>   MAINTAINERS: add entry for TI ADS1262 ADC driver
>   dt-bindings: iio: adc: add bindings for TI ADS1262
> 
>  .../bindings/iio/adc/ti,ads1262.yaml          | 189 ++++++++
>  MAINTAINERS                                   |   7 +
>  drivers/iio/adc/Kconfig                       |  12 +
>  drivers/iio/adc/Makefile                      |   1 +
>  drivers/iio/adc/ti-ads1262.c                  | 438 ++++++++++++++++++
>  5 files changed, 647 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1262.yaml
>  create mode 100644 drivers/iio/adc/ti-ads1262.c
> 
> --
> 2.39.5
> 

It looks like you managed to CC everyone who ever touched the IIO ADC makefile.
On v2, you don't need to include quite so many. :-) Just the people listed in
MAINTAINERS.

And you can drop the RFC on v2, there doesn't seem to be anything unusual that
needs more than regular review. v1 didn't really need the RFC either and is
quite good for a first IIO driver. ;-)

Re: [RFC PATCH 0/5] iio: adc: Add initial support for TI ADS1262

[Andy Shevchenko]

On Thu, May 01, 2025 at 01:20:51PM -0500, David Lechner wrote:
> On 5/1/25 5:00 AM, Sayyad Abid wrote:

> It looks like you managed to CC everyone who ever touched the IIO ADC makefile.
> On v2, you don't need to include quite so many. :-) Just the people listed in
> MAINTAINERS.

FWIW, one may use my "smart" script [1] that has some heuristics which appears to
be quite robust (rarely gives false positives).

[1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh


-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 0/5] iio: adc: Add initial support for TI ADS1262

[Jonathan Cameron]

On Thu,  1 May 2025 15:30:38 +0530
Sayyad Abid <sayyad.abid16@gmail.com> wrote:

> The ADS1262 is a 32-bit, high-resolution delta-sigma ADC communicating
> over SPI. It's designed for precision measurements.
> 
> This initial driver provides the basic functionality needed to:
>  - Probe and register the device via SPI.
>  - Expose standard IIO channels for reading raw voltage samples.
> 
> Basic testing was performed on a Raspberry Pi Zero 2W using the hardware
> SPI0 interface. The connections used were:
> 
> +-----------------+            +-----------------+
> | RPi Zero 2W     |            | TI ADS1262      |
> | (SPI0 Pins)     |            |                 |
> |-----------------|            |-----------------|
> | MOSI            |----------->| DIN             |
> | MISO            |<-----------| DOUT/DRDY       |
> | SCLK            |----------->| SCLK            |
> | CE0             |----------->| /CS             |
> | 5V              |----------->| DVDD, AVDD      |
> | GND             |----------->| DGND, AGND      |
> +-----------------+            +-----------------+
> 
> I would greatly appreciate any feedback on the driver structure,
> IIO integration, SPI communication handling, or any potential issues
> or areas for improvement you might spot.
> 
> This series is broken down as follows:
>  Patch 1: Adds the core driver code (ti-ads1262.c).
>  Patch 2: Adds the Kconfig option.
>  Patch 3: Adds the Makefile entry for compilation.
>  Patch 4: Adds the MAINTAINERS entry.
> 
> Thanks for your time and consideration.

Hi Sayyad

As a general rule, an RFC is usually meant to contain a list of open
questions or some clear statement on why it is not ready for consideration
as a final driver submissions. Often that is something like some other
ongoing discussion that needs to conclude.

I'm not seeing such questions, so I'm guessing this was just a bit of
a lack of confidence?  In general it looks pretty good so drop the RFC on
v2 :)

Jonathan

> 
> Sayyad Abid (5):
>   iio: adc: ti-ads1262.c: add initial driver for TI ADS1262 ADC
>   iio: adc: Kconfig: add Kconfig entry for TI ADS1262 driver
>   iio: adc: Makefile: add compile support for TI ADS1262 driver
>   MAINTAINERS: add entry for TI ADS1262 ADC driver
>   dt-bindings: iio: adc: add bindings for TI ADS1262
> 
>  .../bindings/iio/adc/ti,ads1262.yaml          | 189 ++++++++
>  MAINTAINERS                                   |   7 +
>  drivers/iio/adc/Kconfig                       |  12 +
>  drivers/iio/adc/Makefile                      |   1 +
>  drivers/iio/adc/ti-ads1262.c                  | 438 ++++++++++++++++++
>  5 files changed, 647 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1262.yaml
>  create mode 100644 drivers/iio/adc/ti-ads1262.c
> 
> --
> 2.39.5
> 
>