Add support for AD4080 high-speed, low noise, low distortion,
20-bit, Easy Drive, successive approximation register (SAR)
analog-to-digital converter (ADC).

Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
changes in v2:
 - set num_lanes during probe.
 - rename bitslitp to iio_backend_data_alignment_enable
 - do ad4080_lvds_sync_write only once during probe.
 - use ETIMEDOUT
 - rename to `cnv` instead of `adc-clk`
 - update Kconfig help section
 - drop extra blank line.
 - replace `/**` with `/*` for comments
 - drop redundant return 0.
 - use `dev_dbg` during while(--timeout) procedure
 - use while (--timeout && !sync_en)
 - return -ETIME where applicable and check missing return codes.
 - regmap_update_bits used where applicable
 - use defines instead of GENMASK inline.
 - return FIELD_GET()
 - st->filter_enabled = mode and dropping the if else statement.
 - remove redundant brackets.
 - use OVERSAMPLING_RATIO attribute and drop custom ABI for it
 - use already existing filter_type attribute
 - fix indentation
 - remove comma on 'terminators'
 - use dev_err_probe instead of dev_err
 - check missing return values.
 - rework num_lanes property parse.
 - keep ad4080_chip_info since the driver will be extended for more parts
   in the future (also stated in cover letter).
 drivers/iio/adc/Kconfig  |  14 +
 drivers/iio/adc/Makefile |   1 +
 drivers/iio/adc/ad4080.c | 653 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 668 insertions(+)
 create mode 100644 drivers/iio/adc/ad4080.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 27413516216c..17df328f5322 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -47,6 +47,20 @@ config AD4030
 	  To compile this driver as a module, choose M here: the module will be
 	  called ad4030.
 
+config AD4080
+	tristate "Analog Devices AD4080 high speed ADC"
+	depends on SPI
+	select REGMAP_SPI
+	select IIO_BACKEND
+	help
+	  Say yes here to build support for Analog Devices AD4080
+	  high speed, low noise, low distortion, 20-bit, Easy Drive,
+	  successive approximation register (SAR) analog-to-digital
+	  converter (ADC). Supports iio_backended devices for AD4080.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called ad4080.
+
 config AD4130
 	tristate "Analog Device AD4130 ADC Driver"
 	depends on SPI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 9f26d5eca822..e6efed5b4e7a 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o
 obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
 obj-$(CONFIG_AD4000) += ad4000.o
 obj-$(CONFIG_AD4030) += ad4030.o
+obj-$(CONFIG_AD4080) += ad4080.o
 obj-$(CONFIG_AD4130) += ad4130.o
 obj-$(CONFIG_AD4695) += ad4695.o
 obj-$(CONFIG_AD4851) += ad4851.o
diff --git a/drivers/iio/adc/ad4080.c b/drivers/iio/adc/ad4080.c
new file mode 100644
index 000000000000..3a0b1ad13765
--- /dev/null
+++ b/drivers/iio/adc/ad4080.c
@@ -0,0 +1,653 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Analog Devices AD4080 SPI ADC driver
+ *
+ * Copyright 2025 Analog Devices Inc.
+ */
+
+#include <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+#include <linux/types.h>
+#include <linux/unaligned.h>
+#include <linux/units.h>
+
+#include <linux/iio/backend.h>
+#include <linux/iio/iio.h>
+
+#include <linux/clk.h>
+
+/* Register Definition */
+#define AD4080_REG_INTERFACE_CONFIG_A		0x00
+#define AD4080_REG_INTERFACE_CONFIG_B		0x01
+#define AD4080_REG_DEVICE_CONFIG		0x02
+#define AD4080_REG_CHIP_TYPE			0x03
+#define AD4080_REG_PRODUCT_ID_L			0x04
+#define AD4080_REG_PRODUCT_ID_H			0x05
+#define AD4080_REG_CHIP_GRADE			0x06
+#define AD4080_REG_SCRATCH_PAD			0x0A
+#define AD4080_REG_SPI_REVISION			0x0B
+#define AD4080_REG_VENDOR_L			0x0C
+#define AD4080_REG_VENDOR_H			0x0D
+#define AD4080_REG_STREAM_MODE			0x0E
+#define AD4080_REG_TRANSFER_CONFIG		0x0F
+#define AD4080_REG_INTERFACE_CONFIG_C		0x10
+#define AD4080_REG_INTERFACE_STATUS_A		0x11
+#define AD4080_REG_DEVICE_STATUS		0x14
+#define AD4080_REG_ADC_DATA_INTF_CONFIG_A	0x15
+#define AD4080_REG_ADC_DATA_INTF_CONFIG_B	0x16
+#define AD4080_REG_ADC_DATA_INTF_CONFIG_C	0x17
+#define AD4080_REG_PWR_CTRL			0x18
+#define AD4080_REG_GPIO_CONFIG_A		0x19
+#define AD4080_REG_GPIO_CONFIG_B		0x1A
+#define AD4080_REG_GPIO_CONFIG_C		0x1B
+#define AD4080_REG_GENERAL_CONFIG		0x1C
+#define AD4080_REG_FIFO_WATERMARK_LSB		0x1D
+#define AD4080_REG_FIFO_WATERMARK_MSB		0x1E
+#define AD4080_REG_EVENT_HYSTERESIS_LSB		0x1F
+#define AD4080_REG_EVENT_HYSTERESIS_MSB		0x20
+#define AD4080_REG_EVENT_DETECTION_HI_LSB	0x21
+#define AD4080_REG_EVENT_DETECTION_HI_MSB	0x22
+#define AD4080_REG_EVENT_DETECTION_LO_LSB	0x23
+#define AD4080_REG_EVENT_DETECTION_LO_MSB	0x24
+#define AD4080_REG_OFFSET_LSB			0x25
+#define AD4080_REG_OFFSET_MSB			0x26
+#define AD4080_REG_GAIN_LSB			0x27
+#define AD4080_REG_GAIN_MSB			0x28
+#define AD4080_REG_FILTER_CONFIG		0x29
+
+/* AD4080_REG_INTERFACE_CONFIG_A Bit Definition */
+#define AD4080_SW_RESET_MSK			(BIT(7) | BIT(0))
+#define AD4080_ADDR_ASC_MSK			BIT(5)
+#define AD4080_SDO_ENABLE_MSK			BIT(4)
+
+/* AD4080_REG_INTERFACE_CONFIG_B Bit Definition */
+#define AD4080_SINGLE_INST_MSK			BIT(7)
+#define AD4080_SHORT_INST_MSK			BIT(3)
+
+/* AD4080_REG_DEVICE_CONFIG Bit Definition */
+#define AD4080_OPERATING_MODES_MSK		GENMASK(1, 0)
+
+/* AD4080_REG_TRANSFER_CONFIG Bit Definition */
+#define AD4080_KEEP_STREAM_LENGTH_VAL_MSK	BIT(2)
+
+/* AD4080_REG_INTERFACE_CONFIG_C Bit Definition */
+#define AD4080_STRICT_REG_ACCESS_MSK		BIT(5)
+
+/* AD4080_REG_ADC_DATA_INTF_CONFIG_A Bit Definition */
+#define AD4080_RESERVED_CONFIG_A_MSK		BIT(6)
+#define AD4080_INTF_CHK_EN_MSK			BIT(4)
+#define AD4080_SPI_LVDS_LANES_MSK		BIT(2)
+#define AD4080_DATA_INTF_MODE_MSK		BIT(0)
+
+/* AD4080_REG_ADC_DATA_INTF_CONFIG_B Bit Definition */
+#define AD4080_LVDS_CNV_CLK_CNT_MSK		GENMASK(7, 4)
+#define AD4080_LVDS_SELF_CLK_MODE_MSK		BIT(3)
+#define AD4080_LVDS_CNV_EN_MSK			BIT(0)
+
+/* AD4080_REG_ADC_DATA_INTF_CONFIG_C Bit Definition */
+#define AD4080_LVDS_VOD_MSK			GENMASK(6, 4)
+
+/* AD4080_REG_PWR_CTRL Bit Definition */
+#define AD4080_ANA_DIG_LDO_PD_MSK		BIT(1)
+#define AD4080_INTF_LDO_PD_MSK			BIT(0)
+
+/* AD4080_REG_GPIO_CONFIG_A Bit Definition */
+#define AD4080_GPO_1_EN				BIT(1)
+#define AD4080_GPO_0_EN				BIT(0)
+
+/* AD4080_REG_GPIO_CONFIG_B Bit Definition */
+#define AD4080_GPIO_1_SEL			GENMASK(7, 4)
+#define AD4080_GPIO_0_SEL			GENMASK(3, 0)
+
+/* AD4080_REG_FIFO_CONFIG Bit Definition */
+#define AD4080_FIFO_MODE_MSK			GENMASK(1, 0)
+
+/* AD4080_REG_FILTER_CONFIG Bit Definition */
+#define AD4080_SINC_DEC_RATE_MSK		GENMASK(6, 3)
+#define AD4080_FILTER_SEL_MSK			GENMASK(1, 0)
+
+/* Miscellaneous Definitions */
+#define AD4080_SW_RESET				(BIT(7) | BIT(0))
+#define AD4080_SPI_READ				BIT(7)
+#define BYTE_ADDR_H				GENMASK(15, 8)
+#define BYTE_ADDR_L				GENMASK(7, 0)
+#define AD4080_CHIP_ID				GENMASK(2, 0)
+
+#define AD4080_MAX_SAMP_FREQ			40000000
+#define AD4080_MIN_SAMP_FREQ			1250000
+
+#define AXI_AD4080_ENABLE_FILTER_BIT		BIT(0)
+#define AXI_AD4080_SELF_SYNC_BIT		BIT(1)
+
+enum ad4080_filter_type {
+	FILTER_DISABLE,
+	SINC_1,
+	SINC_5,
+	SINC_5_COMP
+};
+
+static const unsigned int ad4080_scale_table[][2] = {
+	{6000, 0},
+};
+
+static const char *const ad4080_filter_type_iio_enum[] = {
+	[FILTER_DISABLE]   = "disabled",
+	[SINC_1]           = "sinc1",
+	[SINC_5]           = "sinc5",
+	[SINC_5_COMP]      = "sinc5_plus_compensation",
+};
+
+static const int ad4080_dec_rate_iio_enum[] = {
+	2, 4, 8, 16, 32, 64, 128, 256, 512, 1024
+};
+
+static const char * const ad4080_power_supplies[] = {
+	"vdd33", "vdd11", "vddldo", "iovdd", "vrefin",
+};
+
+struct ad4080_chip_info {
+	const char *name;
+	unsigned int product_id;
+	int num_scales;
+	const unsigned int (*scale_table)[2];
+	const struct iio_chan_spec *channels;
+	unsigned int num_channels;
+};
+
+struct ad4080_state {
+	struct spi_device		*spi;
+	struct regmap			*regmap;
+	struct clk			*clk;
+	struct iio_backend		*back;
+	const struct ad4080_chip_info	*info;
+	/*
+	 * Synchronize access to members the of driver state, and ensure
+	 * atomicity of consecutive regmap operations.
+	 */
+	struct mutex			lock;
+	unsigned int			num_lanes;
+	unsigned int			dec_rate;
+	enum ad4080_filter_type		filter_type;
+	bool				filter_en;
+	bool				lvds_cnv_en;
+};
+
+static const struct regmap_config ad4080_regmap_config = {
+	.reg_bits = 16,
+	.val_bits = 8,
+	.read_flag_mask = BIT(7),
+	.max_register = 0x29,
+};
+
+static int ad4080_reg_access(struct iio_dev *indio_dev, unsigned int reg,
+			     unsigned int writeval, unsigned int *readval)
+{
+	struct ad4080_state *st = iio_priv(indio_dev);
+
+	if (readval)
+		return regmap_read(st->regmap, reg, readval);
+
+	return regmap_write(st->regmap, reg, writeval);
+}
+
+static int ad4080_get_scale(struct ad4080_state *st, int *val, int *val2)
+{
+	unsigned int tmp;
+
+	tmp = (st->info->scale_table[0][0] * 1000000ULL) >>
+		    st->info->channels[0].scan_type.realbits;
+	*val = tmp / 1000000;
+	*val2 = tmp % 1000000;
+
+	return IIO_VAL_INT_PLUS_NANO;
+}
+
+static unsigned int ad4080_get_dec_rate(struct iio_dev *dev,
+					const struct iio_chan_spec *chan)
+{
+	struct ad4080_state *st = iio_priv(dev);
+	int ret;
+	unsigned int data;
+
+	ret = regmap_read(st->regmap, AD4080_REG_FILTER_CONFIG, &data);
+	if (ret)
+		return ret;
+
+	return (1 << (FIELD_GET(AD4080_SINC_DEC_RATE_MSK, data) + 1));
+}
+
+static int ad4080_set_dec_rate(struct iio_dev *dev,
+			       const struct iio_chan_spec *chan,
+			       unsigned int mode)
+{
+	struct ad4080_state *st = iio_priv(dev);
+	int ret;
+	unsigned int data;
+	unsigned int reg_val;
+
+	if (st->filter_type >= SINC_5 && mode >= 512)
+		return -EINVAL;
+
+	guard(mutex)(&st->lock);
+	ret = regmap_read(st->regmap, AD4080_REG_FILTER_CONFIG, &reg_val);
+	if (ret)
+		return ret;
+
+	data = ((ilog2(mode) - 1) << 3) | (reg_val & AD4080_FILTER_SEL_MSK);
+	ret = regmap_write(st->regmap, AD4080_REG_FILTER_CONFIG, data);
+	if (ret)
+		return ret;
+
+	st->dec_rate = mode;
+
+	return ret;
+}
+
+static int ad4080_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2, long m)
+{
+	struct ad4080_state *st = iio_priv(indio_dev);
+	int dec_rate;
+
+	switch (m) {
+	case IIO_CHAN_INFO_SCALE:
+		return ad4080_get_scale(st, val, val2);
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		if (st->filter_type == SINC_5_COMP)
+			dec_rate = st->dec_rate * 2;
+		else
+			dec_rate = st->dec_rate;
+		if (st->filter_en)
+			*val = DIV_ROUND_CLOSEST(clk_get_rate(st->clk), dec_rate);
+		else
+			*val = clk_get_rate(st->clk);
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		*val = ad4080_get_dec_rate(indio_dev, chan);
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad4080_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int val, int val2, long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		return -EINVAL;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		return -EINVAL;
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		return ad4080_set_dec_rate(indio_dev, chan, val);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad4080_lvds_sync_write(struct ad4080_state *st)
+{
+	unsigned int timeout = 100;
+	bool sync_en;
+	int ret;
+
+	guard(mutex)(&st->lock);
+	if (st->num_lanes == 1)
+		ret = regmap_write(st->regmap, AD4080_REG_ADC_DATA_INTF_CONFIG_A,
+				   AD4080_RESERVED_CONFIG_A_MSK |
+				   AD4080_INTF_CHK_EN_MSK);
+	else
+		ret = regmap_write(st->regmap, AD4080_REG_ADC_DATA_INTF_CONFIG_A,
+				   AD4080_RESERVED_CONFIG_A_MSK |
+				   AD4080_INTF_CHK_EN_MSK |
+				   AD4080_SPI_LVDS_LANES_MSK);
+	if (ret)
+		return ret;
+
+	ret = iio_backend_data_alignment_enable(st->back);
+	if (ret)
+		return ret;
+
+	do {
+		ret = iio_backend_sync_status_get(st->back, &sync_en);
+		if (ret)
+			return ret;
+
+		if (!sync_en)
+			dev_dbg(&st->spi->dev, "Not Locked: Running Bit Slip\n");
+	} while (--timeout && !sync_en);
+
+	if (timeout) {
+		dev_info(&st->spi->dev, "Success: Pattern correct and Locked!\n");
+		if (st->num_lanes == 1)
+			return regmap_write(st->regmap, AD4080_REG_ADC_DATA_INTF_CONFIG_A,
+					    AD4080_RESERVED_CONFIG_A_MSK);
+		else
+			return regmap_write(st->regmap, AD4080_REG_ADC_DATA_INTF_CONFIG_A,
+					    AD4080_RESERVED_CONFIG_A_MSK |
+					    AD4080_SPI_LVDS_LANES_MSK);
+	} else {
+		dev_info(&st->spi->dev, "LVDS Sync Timeout.\n");
+		if (st->num_lanes == 1) {
+			ret = regmap_write(st->regmap, AD4080_REG_ADC_DATA_INTF_CONFIG_A,
+					   AD4080_RESERVED_CONFIG_A_MSK);
+			if (ret)
+				return ret;
+		} else {
+			ret = regmap_write(st->regmap, AD4080_REG_ADC_DATA_INTF_CONFIG_A,
+					   AD4080_RESERVED_CONFIG_A_MSK |
+					   AD4080_SPI_LVDS_LANES_MSK);
+			if (ret)
+				return ret;
+		}
+
+		return -ETIMEDOUT;
+	}
+}
+
+static ssize_t ad4080_get_filter_type(struct iio_dev *dev,
+				      const struct iio_chan_spec *chan)
+{
+	struct ad4080_state *st = iio_priv(dev);
+	unsigned int data;
+	int ret;
+
+	ret = regmap_read(st->regmap, AD4080_REG_FILTER_CONFIG, &data);
+	if (ret)
+		return ret;
+
+	return FIELD_GET(AD4080_FILTER_SEL_MSK, data);
+}
+
+static int ad4080_set_filter_type(struct iio_dev *dev,
+				  const struct iio_chan_spec *chan,
+				  unsigned int mode)
+{
+	struct ad4080_state *st = iio_priv(dev);
+	int ret;
+	unsigned int data;
+	unsigned int reg_val;
+
+	if (mode >= SINC_5 && st->dec_rate >= 512)
+		return -EINVAL;
+
+	guard(mutex)(&st->lock);
+	if (mode)
+		ret = iio_backend_filter_enable(st->back);
+	else
+		ret = iio_backend_filter_disable(st->back);
+	if (ret)
+		return ret;
+
+	st->filter_en = mode;
+
+	ret = regmap_read(st->regmap, AD4080_REG_FILTER_CONFIG, &reg_val);
+	if (ret)
+		return ret;
+
+	data = (reg_val & AD4080_SINC_DEC_RATE_MSK) |
+	       (mode & AD4080_FILTER_SEL_MSK);
+
+	ret = regmap_write(st->regmap, AD4080_REG_FILTER_CONFIG, data);
+	if (ret)
+		return ret;
+
+	st->filter_type = mode;
+
+	return ret;
+}
+
+static int ad4080_read_avail(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     const int **vals, int *type, int *length,
+			     long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		*vals = ad4080_dec_rate_iio_enum;
+		*length = ARRAY_SIZE(ad4080_dec_rate_iio_enum);
+		*type = IIO_VAL_INT;
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info ad4080_iio_info = {
+	.debugfs_reg_access = ad4080_reg_access,
+	.read_raw = ad4080_read_raw,
+	.write_raw = ad4080_write_raw,
+	.read_avail = ad4080_read_avail,
+};
+
+static const struct iio_enum ad4080_filter_type_enum = {
+	.items = ad4080_filter_type_iio_enum,
+	.num_items = ARRAY_SIZE(ad4080_filter_type_iio_enum),
+	.set = ad4080_set_filter_type,
+	.get = ad4080_get_filter_type,
+};
+
+static struct iio_chan_spec_ext_info ad4080_ext_info[] = {
+	IIO_ENUM("filter_type",
+		 IIO_SHARED_BY_ALL,
+		 &ad4080_filter_type_enum),
+	IIO_ENUM_AVAILABLE("filter_type",
+			   IIO_SHARED_BY_ALL,
+			   &ad4080_filter_type_enum),
+	{}
+};
+
+#define AD4080_CHAN(_chan, _si, _bits, _sign, _shift)		\
+	{ .type = IIO_VOLTAGE,						\
+	  .indexed = 1,							\
+	  .channel = _chan,						\
+	  .info_mask_separate = BIT(IIO_CHAN_INFO_SCALE),		\
+	  .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) |	\
+		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),			\
+	  .info_mask_shared_by_all_available =				\
+		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),			\
+	  .ext_info = ad4080_ext_info,					\
+	  .scan_index = _si,						\
+	  .scan_type = {						\
+			.sign = _sign,					\
+			.realbits = _bits,				\
+			.storagebits = 32,				\
+			.shift = _shift,				\
+	  },								\
+	}
+
+static const struct iio_chan_spec ad4080_channels[] = {
+	AD4080_CHAN(0, 0, 20, 's', 0)
+};
+
+static const struct ad4080_chip_info ad4080_chip_info = {
+	.name = "AD4080",
+	.product_id = AD4080_CHIP_ID,
+	.scale_table = ad4080_scale_table,
+	.num_scales = ARRAY_SIZE(ad4080_scale_table),
+	.num_channels = 1,
+	.channels = ad4080_channels,
+};
+
+static int ad4080_setup(struct iio_dev *indio_dev)
+{
+	struct ad4080_state *st = iio_priv(indio_dev);
+	unsigned int id;
+	int ret;
+
+	ret = regmap_write(st->regmap, AD4080_REG_INTERFACE_CONFIG_A,
+			   AD4080_SW_RESET);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(st->regmap, AD4080_REG_INTERFACE_CONFIG_A,
+			   AD4080_SDO_ENABLE_MSK);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(st->regmap, AD4080_REG_CHIP_TYPE, &id);
+	if (ret)
+		return ret;
+
+	if (id != AD4080_CHIP_ID)
+		return dev_err_probe(&st->spi->dev, -EINVAL,
+				     "Unrecognized CHIP_ID 0x%X\n", id);
+
+	ret = regmap_set_bits(st->regmap, AD4080_REG_GPIO_CONFIG_A,
+			      AD4080_GPO_1_EN);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(st->regmap, AD4080_REG_GPIO_CONFIG_B,
+			   FIELD_PREP(AD4080_GPIO_1_SEL, 3));
+	if (ret)
+		return ret;
+
+	ret = iio_backend_num_lanes_set(st->back, st->num_lanes);
+	if (ret)
+		return ret;
+
+	ret = iio_backend_self_sync_enable(st->back);
+	if (ret)
+		return ret;
+
+	if (st->lvds_cnv_en) {
+		if (st->num_lanes) {
+			ret = regmap_update_bits(st->regmap,
+						 AD4080_REG_ADC_DATA_INTF_CONFIG_B,
+						 AD4080_LVDS_CNV_CLK_CNT_MSK,
+						 FIELD_PREP(AD4080_LVDS_CNV_CLK_CNT_MSK, 7));
+			if (ret)
+				return ret;
+		}
+
+		ret = regmap_set_bits(st->regmap,
+				      AD4080_REG_ADC_DATA_INTF_CONFIG_B,
+				      AD4080_LVDS_CNV_EN_MSK);
+		if (ret)
+			return ret;
+
+		return ad4080_lvds_sync_write(st);
+	}
+
+	return 0;
+}
+
+static void ad4080_properties_parse(struct ad4080_state *st)
+{
+	unsigned int val;
+	int ret;
+
+	st->lvds_cnv_en = device_property_read_bool(&st->spi->dev,
+						    "adi,lvds-cnv-enable");
+
+	st->num_lanes = 1;
+	ret = device_property_read_u32(&st->spi->dev, "adi,num_lanes", &val);
+	if (!ret)
+		st->num_lanes = val;
+}
+
+static int ad4080_probe(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev;
+	struct device *dev = &spi->dev;
+	struct ad4080_state *st;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+	st->spi = spi;
+
+	ret = devm_regulator_bulk_get_enable(dev,
+					     ARRAY_SIZE(ad4080_power_supplies),
+					     ad4080_power_supplies);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to get and enable supplies\n");
+
+	st->regmap = devm_regmap_init_spi(spi, &ad4080_regmap_config);
+	if (IS_ERR(st->regmap))
+		return PTR_ERR(st->regmap);
+
+	st->info = spi_get_device_match_data(spi);
+	if (!st->info)
+		return -ENODEV;
+
+	ret = devm_mutex_init(dev, &st->lock);
+	if (ret)
+		return ret;
+
+	st->info = spi_get_device_match_data(spi);
+	if (!st->info)
+		return -ENODEV;
+
+	indio_dev->name = st->info->name;
+	indio_dev->channels = st->info->channels;
+	indio_dev->num_channels = st->info->num_channels;
+	indio_dev->info = &ad4080_iio_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	ad4080_properties_parse(st);
+
+	st->clk = devm_clk_get_enabled(&spi->dev, "cnv");
+	if (IS_ERR(st->clk))
+		return PTR_ERR(st->clk);
+
+	st->back = devm_iio_backend_get(dev, NULL);
+	if (IS_ERR(st->back))
+		return PTR_ERR(st->back);
+
+	ret = devm_iio_backend_request_buffer(dev, st->back, indio_dev);
+	if (ret)
+		return ret;
+
+	ret = devm_iio_backend_enable(dev, st->back);
+	if (ret)
+		return ret;
+
+	ret = ad4080_setup(indio_dev);
+	if (ret)
+		return ret;
+
+	return devm_iio_device_register(&spi->dev, indio_dev);
+}
+
+static const struct spi_device_id ad4080_id[] = {
+	{ "ad4080", (kernel_ulong_t)&ad4080_chip_info },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, ad4080_id);
+
+static const struct of_device_id ad4080_of_match[] = {
+	{ .compatible = "adi,ad4080", &ad4080_chip_info },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ad4080_of_match);
+
+static struct spi_driver ad4080_driver = {
+	.driver = {
+		.name = "ad4080",
+		.of_match_table = ad4080_of_match,
+	},
+	.probe = ad4080_probe,
+	.id_table = ad4080_id,
+};
+module_spi_driver(ad4080_driver);
+
+MODULE_AUTHOR("Antoniu Miclaus <antoniu.miclaus@analog.com");
+MODULE_DESCRIPTION("Analog Devices AD4080");
+MODULE_LICENSE("GPL");
-- 
2.49.0

Hi Antoniu,

I still have doubts regarding the new backend API mostly because it's still
unclear how it should all work. I had some questions in the first version of the
series that were not clarified so I'll likely repeat myself... Please don't rush
into a v3 without having this sorted out.

Anyways, see below...
 
On Fri, 2025-04-11 at 15:36 +0300, Antoniu Miclaus wrote:
> Add support for AD4080 high-speed, low noise, low distortion,
> 20-bit, Easy Drive, successive approximation register (SAR)
> analog-to-digital converter (ADC).
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> ---
> changes in v2:
>  - set num_lanes during probe.
>  - rename bitslitp to iio_backend_data_alignment_enable
>  - do ad4080_lvds_sync_write only once during probe.
>  - use ETIMEDOUT
>  - rename to `cnv` instead of `adc-clk`
>  - update Kconfig help section
>  - drop extra blank line.
>  - replace `/**` with `/*` for comments
>  - drop redundant return 0.
>  - use `dev_dbg` during while(--timeout) procedure
>  - use while (--timeout && !sync_en)
>  - return -ETIME where applicable and check missing return codes.
>  - regmap_update_bits used where applicable
>  - use defines instead of GENMASK inline.
>  - return FIELD_GET()
>  - st->filter_enabled = mode and dropping the if else statement.
>  - remove redundant brackets.
>  - use OVERSAMPLING_RATIO attribute and drop custom ABI for it
>  - use already existing filter_type attribute
>  - fix indentation
>  - remove comma on 'terminators'
>  - use dev_err_probe instead of dev_err
>  - check missing return values.
>  - rework num_lanes property parse.
>  - keep ad4080_chip_info since the driver will be extended for more parts
>    in the future (also stated in cover letter).
>  drivers/iio/adc/Kconfig  |  14 +
>  drivers/iio/adc/Makefile |   1 +
>  drivers/iio/adc/ad4080.c | 653 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 668 insertions(+)
>  create mode 100644 drivers/iio/adc/ad4080.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 27413516216c..17df328f5322 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -47,6 +47,20 @@ config AD4030
>  	  To compile this driver as a module, choose M here: the module will
> be
>  	  called ad4030.
>  
> +config AD4080
> +	tristate "Analog Devices AD4080 high speed ADC"
> +	depends on SPI
> +	select REGMAP_SPI
> +	select IIO_BACKEND
> +	help
> +	  Say yes here to build support for Analog Devices AD4080
> +	  high speed, low noise, low distortion, 20-bit, Easy Drive,
> +	  successive approximation register (SAR) analog-to-digital
> +	  converter (ADC). Supports iio_backended devices for AD4080.
> +
> +	  To compile this driver as a module, choose M here: the module will
> be
> +	  called ad4080.
> +
>  config AD4130
>  	tristate "Analog Device AD4130 ADC Driver"
>  	depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 9f26d5eca822..e6efed5b4e7a 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o
>  obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
>  obj-$(CONFIG_AD4000) += ad4000.o
>  obj-$(CONFIG_AD4030) += ad4030.o
> +obj-$(CONFIG_AD4080) += ad4080.o
>  obj-$(CONFIG_AD4130) += ad4130.o
>  obj-$(CONFIG_AD4695) += ad4695.o
>  obj-$(CONFIG_AD4851) += ad4851.o
> diff --git a/drivers/iio/adc/ad4080.c b/drivers/iio/adc/ad4080.c
> new file mode 100644
> index 000000000000..3a0b1ad13765
> --- /dev/null
> +++ b/drivers/iio/adc/ad4080.c
> @@ -0,0 +1,653 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Analog Devices AD4080 SPI ADC driver
> + *
> + * Copyright 2025 Analog Devices Inc.
> + */
> +
> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/types.h>
> +#include <linux/unaligned.h>
> +#include <linux/units.h>
> +
> 

...

> +static int ad4080_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long m)
> +{
> +	struct ad4080_state *st = iio_priv(indio_dev);
> +	int dec_rate;
> +
> +	switch (m) {
> +	case IIO_CHAN_INFO_SCALE:
> +		return ad4080_get_scale(st, val, val2);
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		if (st->filter_type == SINC_5_COMP)
> +			dec_rate = st->dec_rate * 2;
> +		else
> +			dec_rate = st->dec_rate;
> +		if (st->filter_en)
> +			*val = DIV_ROUND_CLOSEST(clk_get_rate(st->clk),
> dec_rate);
> +		else
> +			*val = clk_get_rate(st->clk);

Are we expecting the clock rate to change at runtime. Typically, we can just
cache the rate during probe and do it "dynamically" if we ever need it.

> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		*val = ad4080_get_dec_rate(indio_dev, chan);
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad4080_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int val, int val2, long mask)
> +{
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		return -EINVAL;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		return -EINVAL;
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		return ad4080_set_dec_rate(indio_dev, chan, val);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad4080_lvds_sync_write(struct ad4080_state *st)
> +{

This whole function seems to depend on using the LVDS signal for cnv. Why? If we
have CMOS don't we also need some kind of interface alignment/calibration? If
not, that's sensible enough that should be stated in a comment.

> +	unsigned int timeout = 100;
> +	bool sync_en;
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +	if (st->num_lanes == 1)
> +		ret = regmap_write(st->regmap,
> AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> +				   AD4080_RESERVED_CONFIG_A_MSK |
> +				   AD4080_INTF_CHK_EN_MSK);
> +	else
> +		ret = regmap_write(st->regmap,
> AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> +				   AD4080_RESERVED_CONFIG_A_MSK |
> +				   AD4080_INTF_CHK_EN_MSK |
> +				   AD4080_SPI_LVDS_LANES_MSK);
> +	if (ret)
> +		return ret;
> +
> +	ret = iio_backend_data_alignment_enable(st->back);
> +	if (ret)
> +		return ret;
> +
> +	do {
> +		ret = iio_backend_sync_status_get(st->back, &sync_en);
> +		if (ret)
> +			return ret;
> +
> +		if (!sync_en)
> +			dev_dbg(&st->spi->dev, "Not Locked: Running Bit
> Slip\n");
> +	} while (--timeout && !sync_en);
> +
> +	if (timeout) {
> +		dev_info(&st->spi->dev, "Success: Pattern correct and
> Locked!\n");
> +		if (st->num_lanes == 1)
> +			return regmap_write(st->regmap,
> AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> +					    AD4080_RESERVED_CONFIG_A_MSK);
> +		else
> +			return regmap_write(st->regmap,
> AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> +					    AD4080_RESERVED_CONFIG_A_MSK |
> +					    AD4080_SPI_LVDS_LANES_MSK);
> +	} else {
> +		dev_info(&st->spi->dev, "LVDS Sync Timeout.\n");
> +		if (st->num_lanes == 1) {
> +			ret = regmap_write(st->regmap,
> AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> +					   AD4080_RESERVED_CONFIG_A_MSK);
> +			if (ret)
> +				return ret;
> +		} else {
> +			ret = regmap_write(st->regmap,
> AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> +					   AD4080_RESERVED_CONFIG_A_MSK |
> +					   AD4080_SPI_LVDS_LANES_MSK);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		return -ETIMEDOUT;

So, first the things that I don't really get (also the hdl docs need to be
improved FWIW):

* It seems that we can have data alignment or data capture synchronization
through an internal AMD FPGA technique called bit-slip or an external signal,
right? In here, it seems that we only use bit-slip and never disable it. Is that
really the goal?

* This whole process seems to be some kind of calibration at the interface
level, right?

* What's the point of the conv clock? Is it only used to get the sample rate? If
so, the hdl docs are misleading as it seems that it can be used instead of bit-
slip for data capture alignment?

Now, speaking about the backend API's, it looks like that
iio_backend_self_sync_enable() and iio_backend_data_alignment_enable() could be
merged into one API. From what I can tell, iio_backend_data_alignment_enable()
just enables the bit-slip process but that likely does nothing unless the
SELF_SYNC bit is also set to bit-slip, right? In fact, we could think about a
more generic (less flexible though) API like:

* iio_backend_intf_data_align(back, timeout_us), or
* iio_backend_intf_calib(back, timeout_us), or
* iio_backend_intf_data_capture_sync(back, timeout_us)

On the backend side, you could then enable bit-slip and do the status read (with
timeout) and return success or an error code.

The above seems to be pretty much what you're doing but in just one API that
makes sense to me.

Of course that the following questions still come to mind:

* Do we need to disable bit-slip after we're done (still fits into the one API
model)?
* Do we need a meaningful API to change between the syncing/alignment methods?
External signal vs bit-slip?

The above is key to better think of an API. Right now it feels that you're just
adding an API for every bit you want to control in this process...

If we end up needing more flexibility for this, we can also consider the
existing iio_backend_data_sample_trigger() API. I know is abusing a bit and a
stretch but in the end of the day the whole thing is related with aligning,
syncing, calibrating the interface for properly sampling data. Even bit-slip
(while not a traditional external trigger) looks like some kind of self-
adjusting, data-driven trigger mechanism to establish the correct starting point
for capturing data. So having two new enums like:

IIO_BACKEND_SAMPLE_TRIGGER_EXT_SIGNAL,
IIO_BACKEND_SAMPLE_TRIGGER_BIT_SLIP // or maybe a more generic name like
s/BIT_SLIP/INTERNAL

I do not think the above is that horrible :P... But I would really like to get
more opinions about this.

> +	}
> +}

...

> 
> +static const struct ad4080_chip_info ad4080_chip_info = {
> +	.name = "AD4080",
> +	.product_id = AD4080_CHIP_ID,
> +	.scale_table = ad4080_scale_table,
> +	.num_scales = ARRAY_SIZE(ad4080_scale_table),
> +	.num_channels = 1,
> +	.channels = ad4080_channels,
> +};
> +
> +static int ad4080_setup(struct iio_dev *indio_dev)
> +{
> +	struct ad4080_state *st = iio_priv(indio_dev);
> +	unsigned int id;
> +	int ret;
> +
> +	ret = regmap_write(st->regmap, AD4080_REG_INTERFACE_CONFIG_A,
> +			   AD4080_SW_RESET);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD4080_REG_INTERFACE_CONFIG_A,
> +			   AD4080_SDO_ENABLE_MSK);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(st->regmap, AD4080_REG_CHIP_TYPE, &id);
> +	if (ret)
> +		return ret;
> +
> +	if (id != AD4080_CHIP_ID)
> +		return dev_err_probe(&st->spi->dev, -EINVAL,
> +				     "Unrecognized CHIP_ID 0x%X\n", id);
> +
> +	ret = regmap_set_bits(st->regmap, AD4080_REG_GPIO_CONFIG_A,
> +			      AD4080_GPO_1_EN);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD4080_REG_GPIO_CONFIG_B,
> +			   FIELD_PREP(AD4080_GPIO_1_SEL, 3));
> +	if (ret)
> +		return ret;
> +
> +	ret = iio_backend_num_lanes_set(st->back, st->num_lanes);
> +	if (ret)
> +		return ret;
> +
> +	ret = iio_backend_self_sync_enable(st->back);
> +	if (ret)
> +		return ret;
> +

AFAIU, the above is enabling bit-slip?
 
> +	if (st->lvds_cnv_en) {
> +		if (st->num_lanes) {
> +			ret = regmap_update_bits(st->regmap,
> +						
> AD4080_REG_ADC_DATA_INTF_CONFIG_B,
> +						 AD4080_LVDS_CNV_CLK_CNT_MSK,
> +						
> FIELD_PREP(AD4080_LVDS_CNV_CLK_CNT_MSK, 7));
> +			if (ret)
> +				return ret;
> +		}
> +
> +		ret = regmap_set_bits(st->regmap,
> +				      AD4080_REG_ADC_DATA_INTF_CONFIG_B,
> +				      AD4080_LVDS_CNV_EN_MSK);
> +		if (ret)
> +			return ret;
> +
> +		return ad4080_lvds_sync_write(st);
> +	}
> +
> +	return 0;
> +}
> +
> +static void ad4080_properties_parse(struct ad4080_state *st)
> +{
> +	unsigned int val;
> +	int ret;
> +
> +	st->lvds_cnv_en = device_property_read_bool(&st->spi->dev,
> +						    "adi,lvds-cnv-enable");
> +

nit: I would probably drop the enable part. The property is about stating that
the signal is LVDS instead of CMOS. And IIUC, this should also depend on the
existence of `st->clk`
 
> +	st->num_lanes = 1;
> +	ret = device_property_read_u32(&st->spi->dev, "adi,num_lanes", &val);
> +	if (!ret)
> +		st->num_lanes = val;
> +}
> +
> +static int ad4080_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;
> +	struct device *dev = &spi->dev;
> +	struct ad4080_state *st;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->spi = spi;
> +
> +	ret = devm_regulator_bulk_get_enable(dev,
> +					    
> ARRAY_SIZE(ad4080_power_supplies),
> +					     ad4080_power_supplies);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to get and enable supplies\n");
> +
> +	st->regmap = devm_regmap_init_spi(spi, &ad4080_regmap_config);
> +	if (IS_ERR(st->regmap))
> +		return PTR_ERR(st->regmap);
> +
> +	st->info = spi_get_device_match_data(spi);
> +	if (!st->info)
> +		return -ENODEV;
> +
> +	ret = devm_mutex_init(dev, &st->lock);
> +	if (ret)
> +		return ret;
> +
> +	st->info = spi_get_device_match_data(spi);
> +	if (!st->info)
> +		return -ENODEV;
> +

The above is duplicated...

> +	indio_dev->name = st->info->name;
> +	indio_dev->channels = st->info->channels;
> +	indio_dev->num_channels = st->info->num_channels;
> +	indio_dev->info = &ad4080_iio_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	ad4080_properties_parse(st);
> +
> +	st->clk = devm_clk_get_enabled(&spi->dev, "cnv");
> +	if (IS_ERR(st->clk))
> +		return PTR_ERR(st->clk);
> +

From the datasheet it looks like this clock is optional? Moreover in the IP docs
we have the following:


"SELF_SYNC: Controls if the data capture synchronization is done through CNV
signal or bit-slip."

So I wonder, is the cnv clock meaning that we want to have capture
sync/alignment through that external signal instead of bit-slip?

- Nuno Sá

			   AD4080_SPI_LVDS_LANES_MSK);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = iio_backend_data_alignment_enable(st->back);
> > +	if (ret)
> > +		return ret;
> > +
> > +	do {
> > +		ret = iio_backend_sync_status_get(st->back, &sync_en);
> > +		if (ret)
> > +			return ret;
> > +
> > +		if (!sync_en)
> > +			dev_dbg(&st->spi->dev, "Not Locked: Running Bit
> > Slip\n");
> > +	} while (--timeout && !sync_en);
> > +
> > +	if (timeout) {
> > +		dev_info(&st->spi->dev, "Success: Pattern correct and
> > Locked!\n");
> > +		if (st->num_lanes == 1)
> > +			return regmap_write(st->regmap,
> > AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> > +					    AD4080_RESERVED_CONFIG_A_MSK);
> > +		else
> > +			return regmap_write(st->regmap,
> > AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> > +					    AD4080_RESERVED_CONFIG_A_MSK |
> > +					    AD4080_SPI_LVDS_LANES_MSK);
> > +	} else {
> > +		dev_info(&st->spi->dev, "LVDS Sync Timeout.\n");
> > +		if (st->num_lanes == 1) {
> > +			ret = regmap_write(st->regmap,
> > AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> > +					   AD4080_RESERVED_CONFIG_A_MSK);
> > +			if (ret)
> > +				return ret;
> > +		} else {
> > +			ret = regmap_write(st->regmap,
> > AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> > +					   AD4080_RESERVED_CONFIG_A_MSK |
> > +					   AD4080_SPI_LVDS_LANES_MSK);
> > +			if (ret)
> > +				return ret;
> > +		}
> > +
> > +		return -ETIMEDOUT;  
> 
> So, first the things that I don't really get (also the hdl docs need to be
> improved FWIW):
> 
> * It seems that we can have data alignment or data capture synchronization
> through an internal AMD FPGA technique called bit-slip or an external signal,
> right? In here, it seems that we only use bit-slip and never disable it. Is that
> really the goal?
> 
> * This whole process seems to be some kind of calibration at the interface
> level, right?
> 
> * What's the point of the conv clock? Is it only used to get the sample rate? If
> so, the hdl docs are misleading as it seems that it can be used instead of bit-
> slip for data capture alignment?
> 
> Now, speaking about the backend API's, it looks like that
> iio_backend_self_sync_enable() and iio_backend_data_alignment_enable() could be
> merged into one API. From what I can tell, iio_backend_data_alignment_enable()
> just enables the bit-slip process but that likely does nothing unless the
> SELF_SYNC bit is also set to bit-slip, right? In fact, we could think about a
> more generic (less flexible though) API like:
> 
> * iio_backend_intf_data_align(back, timeout_us), or
> * iio_backend_intf_calib(back, timeout_us), or
> * iio_backend_intf_data_capture_sync(back, timeout_us)
> 
> On the backend side, you could then enable bit-slip and do the status read (with
> timeout) and return success or an error code.
> 
> The above seems to be pretty much what you're doing but in just one API that
> makes sense to me.
> 
> Of course that the following questions still come to mind:
> 
> * Do we need to disable bit-slip after we're done (still fits into the one API
> model)?
> * Do we need a meaningful API to change between the syncing/alignment methods?
> External signal vs bit-slip?
> 
> The above is key to better think of an API. Right now it feels that you're just
> adding an API for every bit you want to control in this process...
> 
> If we end up needing more flexibility for this, we can also consider the
> existing iio_backend_data_sample_trigger() API. I know is abusing a bit and a
> stretch but in the end of the day the whole thing is related with aligning,
> syncing, calibrating the interface for properly sampling data. Even bit-slip
> (while not a traditional external trigger) looks like some kind of self-
> adjusting, data-driven trigger mechanism to establish the correct starting point
> for capturing data. So having two new enums like:
> 
> IIO_BACKEND_SAMPLE_TRIGGER_EXT_SIGNAL,
> IIO_BACKEND_SAMPLE_TRIGGER_BIT_SLIP // or maybe a more generic name like
> s/BIT_SLIP/INTERNAL
> 
> I do not think the above is that horrible :P... But I would really like to get
> more opinions about this.

Seems reasonable to me to combine the two as long as we have good documentation.
Agreed it is kind of deriving a trigger from the signal so turning it on with
a trigger type selection doesn't seem unreasonable.

Name tricky though.

Jonathan

> 
> > +	}
> > +}  
> 
> ...
> 
> > 
> > +static const struct ad4080_chip_info ad4080_chip_info = {
> > +	.name = "AD4080",
> > +	.product_id = AD4080_CHIP_ID,
> > +	.scale_table = ad4080_scale_table,
> > +	.num_scales = ARRAY_SIZE(ad4080_scale_table),
> > +	.num_channels = 1,
> > +	.channels = ad4080_channels,
> > +};
> > +
> > +static int ad4080_setup(struct iio_dev *indio_dev)
> > +{
> > +	struct ad4080_state *st = iio_priv(indio_dev);
> > +	unsigned int id;
> > +	int ret;
> > +
> > +	ret = regmap_write(st->regmap, AD4080_REG_INTERFACE_CONFIG_A,
> > +			   AD4080_SW_RESET);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_write(st->regmap, AD4080_REG_INTERFACE_CONFIG_A,
> > +			   AD4080_SDO_ENABLE_MSK);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_read(st->regmap, AD4080_REG_CHIP_TYPE, &id);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (id != AD4080_CHIP_ID)
> > +		return dev_err_probe(&st->spi->dev, -EINVAL,
> > +				     "Unrecognized CHIP_ID 0x%X\n", id);
> > +
> > +	ret = regmap_set_bits(st->regmap, AD4080_REG_GPIO_CONFIG_A,
> > +			      AD4080_GPO_1_EN);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_write(st->regmap, AD4080_REG_GPIO_CONFIG_B,
> > +			   FIELD_PREP(AD4080_GPIO_1_SEL, 3));
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = iio_backend_num_lanes_set(st->back, st->num_lanes);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = iio_backend_self_sync_enable(st->back);
> > +	if (ret)
> > +		return ret;
> > +  
> 
> AFAIU, the above is enabling bit-slip?
>  
> > +	if (st->lvds_cnv_en) {
> > +		if (st->num_lanes) {
> > +			ret = regmap_update_bits(st->regmap,
> > +						
> > AD4080_REG_ADC_DATA_INTF_CONFIG_B,
> > +						 AD4080_LVDS_CNV_CLK_CNT_MSK,
> > +						
> > FIELD_PREP(AD4080_LVDS_CNV_CLK_CNT_MSK, 7));
> > +			if (ret)
> > +				return ret;
> > +		}
> > +
> > +		ret = regmap_set_bits(st->regmap,
> > +				      AD4080_REG_ADC_DATA_INTF_CONFIG_B,
> > +				      AD4080_LVDS_CNV_EN_MSK);
> > +		if (ret)
> > +			return ret;
> > +
> > +		return ad4080_lvds_sync_write(st);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void ad4080_properties_parse(struct ad4080_state *st)
> > +{
> > +	unsigned int val;
> > +	int ret;
> > +
> > +	st->lvds_cnv_en = device_property_read_bool(&st->spi->dev,
> > +						    "adi,lvds-cnv-enable");
> > +  
> 
> nit: I would probably drop the enable part. The property is about stating that
> the signal is LVDS instead of CMOS. And IIUC, this should also depend on the
> existence of `st->clk`
>  
> > +	st->num_lanes = 1;
> > +	ret = device_property_read_u32(&st->spi->dev, "adi,num_lanes", &val);
> > +	if (!ret)
> > +		st->num_lanes = val;
> > +}
> > +
> > +static int ad4080_probe(struct spi_device *spi)
> > +{
> > +	struct iio_dev *indio_dev;
> > +	struct device *dev = &spi->dev;
> > +	struct ad4080_state *st;
> > +	int ret;
> > +
> > +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	st = iio_priv(indio_dev);
> > +	st->spi = spi;
> > +
> > +	ret = devm_regulator_bulk_get_enable(dev,
> > +					    
> > ARRAY_SIZE(ad4080_power_supplies),
> > +					     ad4080_power_supplies);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret,
> > +				     "failed to get and enable supplies\n");
> > +
> > +	st->regmap = devm_regmap_init_spi(spi, &ad4080_regmap_config);
> > +	if (IS_ERR(st->regmap))
> > +		return PTR_ERR(st->regmap);
> > +
> > +	st->info = spi_get_device_match_data(spi);
> > +	if (!st->info)
> > +		return -ENODEV;
> > +
> > +	ret = devm_mutex_init(dev, &st->lock);
> > +	if (ret)
> > +		return ret;
> > +
> > +	st->info = spi_get_device_match_data(spi);
> > +	if (!st->info)
> > +		return -ENODEV;
> > +  
> 
> The above is duplicated...
> 
> > +	indio_dev->name = st->info->name;
> > +	indio_dev->channels = st->info->channels;
> > +	indio_dev->num_channels = st->info->num_channels;
> > +	indio_dev->info = &ad4080_iio_info;
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > +	ad4080_properties_parse(st);
> > +
> > +	st->clk = devm_clk_get_enabled(&spi->dev, "cnv");
> > +	if (IS_ERR(st->clk))
> > +		return PTR_ERR(st->clk);
> > +  
> 
> From the datasheet it looks like this clock is optional? Moreover in the IP docs
> we have the following:
> 
> 
> "SELF_SYNC: Controls if the data capture synchronization is done through CNV
> signal or bit-slip."
> 
> So I wonder, is the cnv clock meaning that we want to have capture
> sync/alignment through that external signal instead of bit-slip?
> 
> - Nuno Sá

On Fri, 11 Apr 2025 15:36:27 +0300
Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:

> Add support for AD4080 high-speed, low noise, low distortion,
> 20-bit, Easy Drive, successive approximation register (SAR)
> analog-to-digital converter (ADC).
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
I'll leave the backend stuff to Nuno who has a better feel than
me for what fits in that interface.  So this is just a review
of the rest of this driver.

Various minor comments inline

Thanks,

Jonathan

> diff --git a/drivers/iio/adc/ad4080.c b/drivers/iio/adc/ad4080.c
> new file mode 100644
> index 000000000000..3a0b1ad13765
> --- /dev/null
> +++ b/drivers/iio/adc/ad4080.c

> +/* AD4080_REG_GPIO_CONFIG_B Bit Definition */
> +#define AD4080_GPIO_1_SEL			GENMASK(7, 4)
> +#define AD4080_GPIO_0_SEL			GENMASK(3, 0)
> +
> +/* AD4080_REG_FIFO_CONFIG Bit Definition */
> +#define AD4080_FIFO_MODE_MSK			GENMASK(1, 0)
> +
> +/* AD4080_REG_FILTER_CONFIG Bit Definition */
Better to name the defines to make that association explicit.
#define AD4080_FILTER_CONFIG_SINC_DEC_RATE_MSK etc

> +#define AD4080_SINC_DEC_RATE_MSK		GENMASK(6, 3)
> +#define AD4080_FILTER_SEL_MSK			GENMASK(1, 0)
> +
> +/* Miscellaneous Definitions */
> +#define AD4080_SW_RESET				(BIT(7) | BIT(0))
> +#define AD4080_SPI_READ				BIT(7)
> +#define BYTE_ADDR_H				GENMASK(15, 8)
> +#define BYTE_ADDR_L				GENMASK(7, 0)
Definitely not on those two!

If you are going this you are probably reading into the wrong data type.

> +static const unsigned int ad4080_scale_table[][2] = {
> +	{6000, 0},
	{ 6000, 0 },
> +};
> +
> +static const char *const ad4080_filter_type_iio_enum[] = {
> +	[FILTER_DISABLE]   = "disabled",
> +	[SINC_1]           = "sinc1",
> +	[SINC_5]           = "sinc5",
> +	[SINC_5_COMP]      = "sinc5_plus_compensation",
> +};
> +
> +static const int ad4080_dec_rate_iio_enum[] = {
> +	2, 4, 8, 16, 32, 64, 128, 256, 512, 1024
Convention is keep a trailing comma except when we have
an explicit terminating entry (NULL etc)

> +
> +static int ad4080_set_dec_rate(struct iio_dev *dev,
> +			       const struct iio_chan_spec *chan,
> +			       unsigned int mode)
> +{
> +	struct ad4080_state *st = iio_priv(dev);
> +	int ret;
> +	unsigned int data;
> +	unsigned int reg_val;
> +
> +	if (st->filter_type >= SINC_5 && mode >= 512)
> +		return -EINVAL;
> +
> +	guard(mutex)(&st->lock);
> +	ret = regmap_read(st->regmap, AD4080_REG_FILTER_CONFIG, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	data = ((ilog2(mode) - 1) << 3) | (reg_val & AD4080_FILTER_SEL_MSK);

As below. Odd to keep stuff with explicit mask like this rather than more
normal masking out what we are placing. &= ~SINC_RET_DATA_MSK; etc
 

> +	ret = regmap_write(st->regmap, AD4080_REG_FILTER_CONFIG, data);
> +	if (ret)
> +		return ret;
> +
> +	st->dec_rate = mode;
> +
> +	return ret;
> +}

> +static int ad4080_lvds_sync_write(struct ad4080_state *st)
> +{
> +	unsigned int timeout = 100;
> +	bool sync_en;
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +	if (st->num_lanes == 1)
> +		ret = regmap_write(st->regmap, AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> +				   AD4080_RESERVED_CONFIG_A_MSK |
> +				   AD4080_INTF_CHK_EN_MSK);
> +	else
> +		ret = regmap_write(st->regmap, AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> +				   AD4080_RESERVED_CONFIG_A_MSK |
> +				   AD4080_INTF_CHK_EN_MSK |
> +				   AD4080_SPI_LVDS_LANES_MSK);
> +	if (ret)
> +		return ret;
> +
> +	ret = iio_backend_data_alignment_enable(st->back);
> +	if (ret)
> +		return ret;
> +
> +	do {
> +		ret = iio_backend_sync_status_get(st->back, &sync_en);
> +		if (ret)
> +			return ret;
> +
> +		if (!sync_en)
> +			dev_dbg(&st->spi->dev, "Not Locked: Running Bit Slip\n");

Maybe sleep a bit before trying again?  Tight loops are very dependent on the
host CPU which is probably not what you want here.

> +	} while (--timeout && !sync_en);
> +
> +	if (timeout) {
> +		dev_info(&st->spi->dev, "Success: Pattern correct and Locked!\n");
> +		if (st->num_lanes == 1)
> +			return regmap_write(st->regmap, AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> +					    AD4080_RESERVED_CONFIG_A_MSK);
> +		else
> +			return regmap_write(st->regmap, AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> +					    AD4080_RESERVED_CONFIG_A_MSK |
> +					    AD4080_SPI_LVDS_LANES_MSK);
> +	} else {
> +		dev_info(&st->spi->dev, "LVDS Sync Timeout.\n");
> +		if (st->num_lanes == 1) {
> +			ret = regmap_write(st->regmap, AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> +					   AD4080_RESERVED_CONFIG_A_MSK);
> +			if (ret)
> +				return ret;
> +		} else {
> +			ret = regmap_write(st->regmap, AD4080_REG_ADC_DATA_INTF_CONFIG_A,
> +					   AD4080_RESERVED_CONFIG_A_MSK |
> +					   AD4080_SPI_LVDS_LANES_MSK);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		return -ETIMEDOUT;
> +	}
> +}

> +
> +static int ad4080_set_filter_type(struct iio_dev *dev,
> +				  const struct iio_chan_spec *chan,
> +				  unsigned int mode)
> +{
> +	struct ad4080_state *st = iio_priv(dev);
> +	int ret;
> +	unsigned int data;
> +	unsigned int reg_val;
> +
> +	if (mode >= SINC_5 && st->dec_rate >= 512)
> +		return -EINVAL;
> +
> +	guard(mutex)(&st->lock);
> +	if (mode)
> +		ret = iio_backend_filter_enable(st->back);
> +	else
> +		ret = iio_backend_filter_disable(st->back);
> +	if (ret)
> +		return ret;
> +
> +	st->filter_en = mode;
> +
> +	ret = regmap_read(st->regmap, AD4080_REG_FILTER_CONFIG, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	data = (reg_val & AD4080_SINC_DEC_RATE_MSK) |
> +	       (mode & AD4080_FILTER_SEL_MSK);

FIELD_PREP() for the second part.
The first is hanging on to one field.  Maybe just pull that out with
a FIELD_GET() and write it back again with FIELD_PREP?
Will be more code, but a little less subtle to read!


> +
> +	ret = regmap_write(st->regmap, AD4080_REG_FILTER_CONFIG, data);
> +	if (ret)
> +		return ret;
> +
> +	st->filter_type = mode;
> +
> +	return ret;
> +}

> +static struct iio_chan_spec_ext_info ad4080_ext_info[] = {
> +	IIO_ENUM("filter_type",
> +		 IIO_SHARED_BY_ALL,
> +		 &ad4080_filter_type_enum),
very short line wrap - aim for nearer 80 chars.

> +	IIO_ENUM_AVAILABLE("filter_type",
> +			   IIO_SHARED_BY_ALL,
> +			   &ad4080_filter_type_enum),
> +	{}
Trivial preference for 
	{ }

> +};
> +
> +#define AD4080_CHAN(_chan, _si, _bits, _sign, _shift)		\
> +	{ .type = IIO_VOLTAGE,	
Odd indent. Better perhaps as simpler
	{ \
		.indexed = 1, 
etc.					\
> +	  .indexed = 1,							\
> +	  .channel = _chan,						\
> +	  .info_mask_separate = BIT(IIO_CHAN_INFO_SCALE),		\
> +	  .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) |	\
> +		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),			\
> +	  .info_mask_shared_by_all_available =				\
> +		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),			\
> +	  .ext_info = ad4080_ext_info,					\
> +	  .scan_index = _si,						\
> +	  .scan_type = {						\
> +			.sign = _sign,					\
Current indent makes this look really wierd!

> +			.realbits = _bits,				\
> +			.storagebits = 32,				\
> +			.shift = _shift,				\
> +	  },								\
> +	}
> +
> +static const struct iio_chan_spec ad4080_channels[] = {
> +	AD4080_CHAN(0, 0, 20, 's', 0)
Don't bother with the macro as it doesn't add anything. Just put that
stuff all here. That will let you skip setting obvious defaults to 0
like the shift.

> +};
> +
> +static const struct ad4080_chip_info ad4080_chip_info = {
> +	.name = "AD4080",
> +	.product_id = AD4080_CHIP_ID,
> +	.scale_table = ad4080_scale_table,
> +	.num_scales = ARRAY_SIZE(ad4080_scale_table),
> +	.num_channels = 1,
> +	.channels = ad4080_channels,
> +};
> +
> +static int ad4080_setup(struct iio_dev *indio_dev)
> +{

> +	if (id != AD4080_CHIP_ID)
> +		return dev_err_probe(&st->spi->dev, -EINVAL,
> +				     "Unrecognized CHIP_ID 0x%X\n", id);
A mismatch on ID should not be treated as an error. That breaks the
use of fallback dt compatibles.  So convention on these is a dev_info()
and carry on anyway.  We've left breadcrumbs if things don't work but
not our role to make sure it is definitely the right hardware in the
firmware description.

> +
> +	ret = regmap_set_bits(st->regmap, AD4080_REG_GPIO_CONFIG_A,
> +			      AD4080_GPO_1_EN);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD4080_REG_GPIO_CONFIG_B,
> +			   FIELD_PREP(AD4080_GPIO_1_SEL, 3));
> +	if (ret)
> +		return ret;
> +
> +	ret = iio_backend_num_lanes_set(st->back, st->num_lanes);
> +	if (ret)
> +		return ret;
> +
> +	ret = iio_backend_self_sync_enable(st->back);
> +	if (ret)
> +		return ret;
> +
> +	if (st->lvds_cnv_en) {
I'd flip this unless you expect to shortly add more optional stuff after this
code

	if (!st->lvds_cnv_en)
		return 0;

	..

> +		if (st->num_lanes) {
> +			ret = regmap_update_bits(st->regmap,
> +						 AD4080_REG_ADC_DATA_INTF_CONFIG_B,
> +						 AD4080_LVDS_CNV_CLK_CNT_MSK,
> +						 FIELD_PREP(AD4080_LVDS_CNV_CLK_CNT_MSK, 7));
> +			if (ret)
> +				return ret;
> +		}
> +
> +		ret = regmap_set_bits(st->regmap,
> +				      AD4080_REG_ADC_DATA_INTF_CONFIG_B,
> +				      AD4080_LVDS_CNV_EN_MSK);
> +		if (ret)
> +			return ret;
> +
> +		return ad4080_lvds_sync_write(st);
> +	}
> +
> +	return 0;
> +}
> +
> +static void ad4080_properties_parse(struct ad4080_state *st)
> +{
> +	unsigned int val;
> +	int ret;
> +
> +	st->lvds_cnv_en = device_property_read_bool(&st->spi->dev,
> +						    "adi,lvds-cnv-enable");
> +
> +	st->num_lanes = 1;
> +	ret = device_property_read_u32(&st->spi->dev, "adi,num_lanes", &val);
> +	if (!ret)
> +		st->num_lanes = val;
Usual trick on these places were we have a default is to pick types correctly 
and do.

	st->num_lanes = 1;
	device_property_read_u32(&st->spi->dev, "adi,num_lanes", &st->num_lanes);

That is, rely on the call being side effect free on error.
 
> +}