[PATCH v7 8/8] iio: adc: ad4851: add ad485x driver

Antoniu Miclaus posted 8 patches 3 weeks, 5 days ago
[PATCH v7 8/8] iio: adc: ad4851: add ad485x driver
Posted by Antoniu Miclaus 3 weeks, 5 days ago
Add support for the AD485X a fully buffered, 8-channel simultaneous
sampling, 16/20-bit, 1 MSPS data acquisition system (DAS) with
differential, wide common-mode range inputs.

Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
changes in v7:
 - use new iio backend os enable/disable functions
 - implement separate scan_type for both signed and unsigned.
 - drop ext_scan_type for 16-bit chips
 - rework scan_index ordering.
 - add separate scales for diff/single-ended channels
 - parse iio channels via dts properties.
 drivers/iio/adc/Kconfig  |   13 +
 drivers/iio/adc/Makefile |    1 +
 drivers/iio/adc/ad4851.c | 1346 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 1360 insertions(+)
 create mode 100644 drivers/iio/adc/ad4851.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 6c4e74420fd2..0d97cd760d90 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -61,6 +61,19 @@ config AD4695
 	  To compile this driver as a module, choose M here: the module will be
 	  called ad4695.
 
+config AD4851
+	tristate "Analog Device AD4851 DAS Driver"
+	depends on SPI
+	select REGMAP_SPI
+	select IIO_BACKEND
+	help
+	  Say yes here to build support for Analog Devices AD4851, AD4852,
+	  AD4853, AD4854, AD4855, AD4856, AD4857, AD4858, AD4858I high speed
+	  data acquisition system (DAS).
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called ad4851.
+
 config AD7091R
 	tristate
 
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 7b91cd98c0e0..d83df8b5925d 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
 obj-$(CONFIG_AD4000) += ad4000.o
 obj-$(CONFIG_AD4130) += ad4130.o
 obj-$(CONFIG_AD4695) += ad4695.o
+obj-$(CONFIG_AD4851) += ad4851.o
 obj-$(CONFIG_AD7091R) += ad7091r-base.o
 obj-$(CONFIG_AD7091R5) += ad7091r5.o
 obj-$(CONFIG_AD7091R8) += ad7091r8.o
diff --git a/drivers/iio/adc/ad4851.c b/drivers/iio/adc/ad4851.c
new file mode 100644
index 000000000000..e8e5c0def29e
--- /dev/null
+++ b/drivers/iio/adc/ad4851.c
@@ -0,0 +1,1346 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Analog Devices AD4851 DAS driver
+ *
+ * Copyright 2024 Analog Devices Inc.
+ */
+
+#include <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/minmax.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/pwm.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>
+
+#define AD4851_REG_INTERFACE_CONFIG_A	0x00
+#define AD4851_REG_INTERFACE_CONFIG_B	0x01
+#define AD4851_REG_PRODUCT_ID_L		0x04
+#define AD4851_REG_PRODUCT_ID_H		0x05
+#define AD4851_REG_DEVICE_CTRL		0x25
+#define AD4851_REG_PACKET		0x26
+#define AD4851_REG_OVERSAMPLE		0x27
+
+#define AD4851_REG_CH_CONFIG_BASE	0x2A
+#define AD4851_REG_CHX_SOFTSPAN(ch)	((0x12 * (ch)) + AD4851_REG_CH_CONFIG_BASE)
+#define AD4851_REG_CHX_OFFSET(ch)	(AD4851_REG_CHX_SOFTSPAN(ch) + 0x01)
+#define AD4851_REG_CHX_OFFSET_LSB(ch)	AD4851_REG_CHX_OFFSET(ch)
+#define AD4851_REG_CHX_OFFSET_MID(ch)	(AD4851_REG_CHX_OFFSET_LSB(ch) + 0x01)
+#define AD4851_REG_CHX_OFFSET_MSB(ch)	(AD4851_REG_CHX_OFFSET_MID(ch) + 0x01)
+#define AD4851_REG_CHX_GAIN(ch)		(AD4851_REG_CHX_OFFSET(ch) + 0x03)
+#define AD4851_REG_CHX_GAIN_LSB(ch)	AD4851_REG_CHX_GAIN(ch)
+#define AD4851_REG_CHX_GAIN_MSB(ch)	(AD4851_REG_CHX_GAIN(ch) + 0x01)
+#define AD4851_REG_CHX_PHASE(ch)	(AD4851_REG_CHX_GAIN(ch) + 0x02)
+#define AD4851_REG_CHX_PHASE_LSB(ch)	AD4851_REG_CHX_PHASE(ch)
+#define AD4851_REG_CHX_PHASE_MSB(ch)	(AD4851_REG_CHX_PHASE_LSB(ch) + 0x01)
+
+#define AD4851_REG_TESTPAT_0(c)		(0x38 + (c) * 0x12)
+#define AD4851_REG_TESTPAT_1(c)		(0x39 + (c) * 0x12)
+#define AD4851_REG_TESTPAT_2(c)		(0x3A + (c) * 0x12)
+#define AD4851_REG_TESTPAT_3(c)		(0x3B + (c) * 0x12)
+
+#define AD4851_SW_RESET			(BIT(7) | BIT(0))
+#define AD4851_SDO_ENABLE		BIT(4)
+#define AD4851_SINGLE_INSTRUCTION	BIT(7)
+#define AD4851_REFBUF_PD		BIT(2)
+#define AD4851_REFSEL_PD		BIT(1)
+#define AD4851_ECHO_CLOCK_MODE		BIT(0)
+
+#define AD4851_PACKET_FORMAT_0		0
+#define AD4851_PACKET_FORMAT_1		1
+#define AD4851_PACKET_FORMAT_MASK	GENMASK(1, 0)
+
+#define AD4851_OS_EN_MSK		BIT(7)
+#define AD4851_OS_RATIO_MSK		GENMASK(3, 0)
+
+#define AD4851_TEST_PAT			BIT(2)
+
+#define AD4858_PACKET_SIZE_20		0
+#define AD4858_PACKET_SIZE_24		1
+#define AD4858_PACKET_SIZE_32		2
+
+#define AD4857_PACKET_SIZE_16		0
+#define AD4857_PACKET_SIZE_24		1
+
+#define AD4851_TESTPAT_0_DEFAULT	0x2A
+#define AD4851_TESTPAT_1_DEFAULT	0x3C
+#define AD4851_TESTPAT_2_DEFAULT	0xCE
+#define AD4851_TESTPAT_3_DEFAULT(c)	(0x0A + (0x10 * (c)))
+
+#define AD4851_SOFTSPAN_0V_2V5		0
+#define AD4851_SOFTSPAN_N2V5_2V5	1
+#define AD4851_SOFTSPAN_0V_5V		2
+#define AD4851_SOFTSPAN_N5V_5V		3
+#define AD4851_SOFTSPAN_0V_6V25		4
+#define AD4851_SOFTSPAN_N6V25_6V25	5
+#define AD4851_SOFTSPAN_0V_10V		6
+#define AD4851_SOFTSPAN_N10V_10V	7
+#define AD4851_SOFTSPAN_0V_12V5		8
+#define AD4851_SOFTSPAN_N12V5_12V5	9
+#define AD4851_SOFTSPAN_0V_20V		10
+#define AD4851_SOFTSPAN_N20V_20V	11
+#define AD4851_SOFTSPAN_0V_25V		12
+#define AD4851_SOFTSPAN_N25V_25V	13
+#define AD4851_SOFTSPAN_0V_40V		14
+#define AD4851_SOFTSPAN_N40V_40V	15
+
+#define AD4851_MAX_LANES		8
+#define AD4851_MAX_IODELAY		32
+
+#define AD4851_T_CNVH_NS		40
+
+#define AD4841_MAX_SCALE_AVAIL		8
+
+#define AD4851_MAX_CH_NR		8
+#define AD4851_CH_START			0
+
+struct ad4851_scale {
+	unsigned int scale_val;
+	u8 reg_val;
+};
+
+static const struct ad4851_scale ad4851_scale_table_se[] = {
+	{ 2500, 0x0 },
+	{ 5000, 0x2 },
+	{ 6250, 0x4 },
+	{ 10000, 0x6 },
+	{ 12500, 0x8 },
+	{ 20000, 0xA },
+	{ 25000, 0xC },
+	{ 40000, 0xE },
+};
+
+static const struct ad4851_scale ad4851_scale_table_diff[] = {
+	{ 5000, 0x1 },
+	{ 10000, 0x3 },
+	{ 12500, 0x5 },
+	{ 20000, 0x7 },
+	{ 25000, 0x9 },
+	{ 40000, 0xB },
+	{ 50000, 0xD },
+	{ 80000, 0xF },
+};
+
+static const unsigned int ad4851_scale_avail_se[] = {
+	2500,
+	5000,
+	6250,
+	10000,
+	12500,
+	20000,
+	25000,
+	40000,
+};
+
+static const unsigned int ad4851_scale_avail_diff[] = {
+	5000,
+	10000,
+	12500,
+	20000,
+	25000,
+	40000,
+	50000,
+	80000,
+};
+
+struct ad4851_chip_info {
+	const char *name;
+	unsigned int product_id;
+	int num_scales;
+	const struct iio_chan_spec *channels;
+	unsigned int num_channels;
+	unsigned long max_sample_rate_hz;
+	unsigned int resolution;
+	int (*parse_channels)(struct iio_dev *indio_dev);
+};
+
+enum {
+	AD4851_SCAN_TYPE_NORMAL,
+	AD4851_SCAN_TYPE_RESOLUTION_BOOST,
+};
+
+struct ad4851_state {
+	struct spi_device *spi;
+	struct pwm_device *cnv;
+	struct iio_backend *back;
+	/*
+	 * Synchronize access to members the of driver state, and ensure
+	 * atomicity of consecutive regmap operations.
+	 */
+	struct mutex lock;
+	struct regmap *regmap;
+	struct regulator *vrefbuf;
+	struct regulator *vrefio;
+	const struct ad4851_chip_info *info;
+	struct gpio_desc *pd_gpio;
+	bool resolution_boost_enabled;
+	unsigned long sampling_freq;
+	unsigned int scales_se[AD4841_MAX_SCALE_AVAIL][2];
+	unsigned int scales_diff[AD4841_MAX_SCALE_AVAIL][2];
+};
+
+static int ad4851_reg_access(struct iio_dev *indio_dev,
+			     unsigned int reg,
+			     unsigned int writeval,
+			     unsigned int *readval)
+{
+	struct ad4851_state *st = iio_priv(indio_dev);
+
+	guard(mutex)(&st->lock);
+
+	if (readval)
+		return regmap_read(st->regmap, reg, readval);
+
+	return regmap_write(st->regmap, reg, writeval);
+}
+
+static int ad4851_set_sampling_freq(struct ad4851_state *st, unsigned int freq)
+{
+	struct pwm_state cnv_state = {
+		.duty_cycle = AD4851_T_CNVH_NS,
+		.enabled = true,
+	};
+	int ret;
+
+	freq = clamp(freq, 1, st->info->max_sample_rate_hz);
+
+	cnv_state.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC, freq);
+
+	ret = pwm_apply_might_sleep(st->cnv, &cnv_state);
+	if (ret)
+		return ret;
+
+	st->sampling_freq = freq;
+
+	return 0;
+}
+
+static const int ad4851_oversampling_ratios[] = {
+	1, 2, 4, 8, 16,	32, 64, 128,
+	256, 512, 1024, 2048, 4096, 8192, 16384, 32768,
+	65536,
+};
+
+static int ad4851_osr_to_regval(int ratio)
+{
+	int i;
+
+	for (i = 1; i < ARRAY_SIZE(ad4851_oversampling_ratios); i++)
+		if (ratio == ad4851_oversampling_ratios[i])
+			return i - 1;
+
+	return -EINVAL;
+}
+
+static int ad4851_set_oversampling_ratio(struct ad4851_state *st,
+					 const struct iio_chan_spec *chan,
+					 unsigned int osr)
+{
+	unsigned int val;
+	int ret;
+
+	guard(mutex)(&st->lock);
+
+	if (osr == 1) {
+		ret = regmap_clear_bits(st->regmap, AD4851_REG_OVERSAMPLE,
+					AD4851_OS_EN_MSK);
+		if (ret)
+			return ret;
+
+		ret = iio_backend_oversampling_disable(st->back);
+		if (ret)
+			return ret;
+	} else {
+		val = ad4851_osr_to_regval(osr);
+		if (val < 0)
+			return -EINVAL;
+
+		ret = regmap_set_bits(st->regmap, AD4851_REG_OVERSAMPLE,
+				      AD4851_OS_EN_MSK);
+		if (ret)
+			return ret;
+
+		ret = regmap_update_bits(st->regmap, AD4851_REG_OVERSAMPLE,
+					 AD4851_OS_RATIO_MSK, val);
+		if (ret)
+			return ret;
+
+		ret = iio_backend_oversampling_enable(st->back);
+		if (ret)
+			return ret;
+	}
+
+	switch (chan->scan_type.realbits) {
+	case 20:
+		switch (osr) {
+		case 0:
+			return -EINVAL;
+		case 1:
+			val = 20;
+			break;
+		default:
+			val = 24;
+			break;
+		}
+		break;
+	case 16:
+		val = 16;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = iio_backend_data_size_set(st->back, val);
+	if (ret)
+		return ret;
+
+	if (osr == 1 || chan->scan_type.realbits == 16) {
+		ret = regmap_clear_bits(st->regmap, AD4851_REG_PACKET,
+					AD4851_PACKET_FORMAT_MASK);
+		if (ret)
+			return ret;
+
+		st->resolution_boost_enabled = false;
+	} else {
+		ret = regmap_update_bits(st->regmap, AD4851_REG_PACKET,
+					 AD4851_PACKET_FORMAT_MASK,
+					 FIELD_PREP(AD4851_PACKET_FORMAT_MASK, 1));
+		if (ret)
+			return ret;
+
+		st->resolution_boost_enabled = true;
+	}
+
+	return 0;
+}
+
+static int ad4851_get_oversampling_ratio(struct ad4851_state *st, unsigned int *val)
+{
+	unsigned int osr;
+	int ret;
+
+	guard(mutex)(&st->lock);
+
+	ret = regmap_read(st->regmap, AD4851_REG_OVERSAMPLE, &osr);
+	if (ret)
+		return ret;
+
+	if (!FIELD_GET(AD4851_OS_EN_MSK, osr))
+		*val = 1;
+	else
+		*val = ad4851_oversampling_ratios[FIELD_GET(AD4851_OS_RATIO_MSK, osr)];
+
+	return IIO_VAL_INT;
+}
+
+static void ad4851_reg_disable(void *data)
+{
+	regulator_disable(data);
+}
+
+static void ad4851_pwm_disable(void *data)
+{
+	pwm_disable(data);
+}
+
+static int ad4851_setup(struct ad4851_state *st)
+{
+	unsigned int product_id;
+	int ret;
+
+	if (!IS_ERR(st->vrefbuf)) {
+		ret = regmap_set_bits(st->regmap, AD4851_REG_DEVICE_CTRL,
+				      AD4851_REFBUF_PD);
+		if (ret)
+			return ret;
+
+		ret = regulator_enable(st->vrefbuf);
+		if (ret)
+			return ret;
+
+		ret = devm_add_action_or_reset(&st->spi->dev, ad4851_reg_disable,
+					       st->vrefbuf);
+		if (ret)
+			return ret;
+	}
+
+	if (!IS_ERR(st->vrefio)) {
+		ret = regmap_set_bits(st->regmap, AD4851_REG_DEVICE_CTRL,
+				      AD4851_REFSEL_PD);
+		if (ret)
+			return ret;
+
+		ret = regulator_enable(st->vrefio);
+		if (ret)
+			return ret;
+
+		ret = devm_add_action_or_reset(&st->spi->dev, ad4851_reg_disable,
+					       st->vrefio);
+		if (ret)
+			return ret;
+	}
+
+	if (st->pd_gpio) {
+		/* To initiate a global reset, bring the PD pin high twice */
+		gpiod_set_value(st->pd_gpio, 1);
+		fsleep(1);
+		gpiod_set_value(st->pd_gpio, 0);
+		fsleep(1);
+		gpiod_set_value(st->pd_gpio, 1);
+		fsleep(1);
+		gpiod_set_value(st->pd_gpio, 0);
+		fsleep(1000);
+	} else {
+		ret = regmap_set_bits(st->regmap, AD4851_REG_INTERFACE_CONFIG_A,
+				      AD4851_SW_RESET);
+		if (ret)
+			return ret;
+	}
+
+	ret = regmap_write(st->regmap, AD4851_REG_INTERFACE_CONFIG_B,
+			   AD4851_SINGLE_INSTRUCTION);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(st->regmap, AD4851_REG_INTERFACE_CONFIG_A,
+			   AD4851_SDO_ENABLE);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(st->regmap, AD4851_REG_PRODUCT_ID_L, &product_id);
+	if (ret)
+		return ret;
+
+	if (product_id != st->info->product_id)
+		dev_info(&st->spi->dev, "Unknown product ID: 0x%02X\n",
+			 product_id);
+
+	ret = regmap_set_bits(st->regmap, AD4851_REG_DEVICE_CTRL,
+			      AD4851_ECHO_CLOCK_MODE);
+	if (ret)
+		return ret;
+
+	return regmap_write(st->regmap, AD4851_REG_PACKET, 0);
+}
+
+static int ad4851_find_opt(bool *field, u32 size, u32 *ret_start)
+{
+	unsigned int i, cnt = 0, max_cnt = 0, max_start = 0;
+	int start;
+
+	for (i = 0, start = -1; i < size; i++) {
+		if (field[i] == 0) {
+			if (start == -1)
+				start = i;
+			cnt++;
+		} else {
+			if (cnt > max_cnt) {
+				max_cnt = cnt;
+				max_start = start;
+			}
+			start = -1;
+			cnt = 0;
+		}
+	}
+	/*
+	 * Find the longest consecutive sequence of false values from field
+	 * and return starting index.
+	 */
+	if (cnt > max_cnt) {
+		max_cnt = cnt;
+		max_start = start;
+	}
+
+	if (!max_cnt)
+		return -ENOENT;
+
+	*ret_start = max_start;
+
+	return max_cnt;
+}
+
+static int ad4851_calibrate(struct ad4851_state *st)
+{
+	unsigned int opt_delay, num_lanes, delay, i, s, c;
+	enum iio_backend_interface_type interface_type;
+	DECLARE_BITMAP(pn_status, AD4851_MAX_LANES * AD4851_MAX_IODELAY);
+	bool status;
+	int ret;
+
+	ret = iio_backend_interface_type_get(st->back, &interface_type);
+	if (ret)
+		return ret;
+
+	switch (interface_type) {
+	case IIO_BACKEND_INTERFACE_SERIAL_CMOS:
+		num_lanes = st->info->num_channels / 2;
+		break;
+	case IIO_BACKEND_INTERFACE_SERIAL_LVDS:
+		num_lanes = 1;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (st->info->resolution == 16) {
+		ret = iio_backend_data_size_set(st->back, 24);
+		if (ret)
+			return ret;
+
+		ret = regmap_write(st->regmap, AD4851_REG_PACKET,
+				   AD4851_TEST_PAT | AD4857_PACKET_SIZE_24);
+		if (ret)
+			return ret;
+	} else {
+		ret = iio_backend_data_size_set(st->back, 32);
+		if (ret)
+			return ret;
+
+		ret = regmap_write(st->regmap, AD4851_REG_PACKET,
+				   AD4851_TEST_PAT | AD4858_PACKET_SIZE_32);
+		if (ret)
+			return ret;
+	}
+
+	for (i = 0; i < st->info->num_channels / 2; i++) {
+		ret = regmap_write(st->regmap, AD4851_REG_TESTPAT_0(i),
+				   AD4851_TESTPAT_0_DEFAULT);
+		if (ret)
+			return ret;
+
+		ret = regmap_write(st->regmap, AD4851_REG_TESTPAT_1(i),
+				   AD4851_TESTPAT_1_DEFAULT);
+		if (ret)
+			return ret;
+
+		ret = regmap_write(st->regmap, AD4851_REG_TESTPAT_2(i),
+				   AD4851_TESTPAT_2_DEFAULT);
+		if (ret)
+			return ret;
+
+		ret = regmap_write(st->regmap, AD4851_REG_TESTPAT_3(i),
+				   AD4851_TESTPAT_3_DEFAULT(i));
+		if (ret)
+			return ret;
+
+		ret = iio_backend_chan_enable(st->back, i);
+		if (ret)
+			return ret;
+	}
+
+	for (i = 0; i < num_lanes; i++) {
+		for (delay = 0; delay < AD4851_MAX_IODELAY; delay++) {
+			ret = iio_backend_iodelay_set(st->back, i, delay);
+			if (ret)
+				return ret;
+
+			ret = iio_backend_chan_status(st->back, i, &status);
+			if (ret)
+				return ret;
+
+			if (status)
+				set_bit(i * AD4851_MAX_IODELAY + delay, pn_status);
+			else
+				clear_bit(i * AD4851_MAX_IODELAY + delay, pn_status);
+		}
+	}
+
+	for (i = 0; i < num_lanes; i++) {
+		status = test_bit(i * AD4851_MAX_IODELAY, pn_status);
+		c = ad4851_find_opt(&status, AD4851_MAX_IODELAY, &s);
+		if (c < 0)
+			return c;
+
+		opt_delay = s + c / 2;
+		ret = iio_backend_iodelay_set(st->back, i, opt_delay);
+		if (ret)
+			return ret;
+	}
+
+	for (i = 0; i < st->info->num_channels / 2; i++) {
+		ret = iio_backend_chan_disable(st->back, i);
+		if (ret)
+			return ret;
+	}
+
+	ret = iio_backend_data_size_set(st->back, 20);
+	if (ret)
+		return ret;
+
+	return regmap_write(st->regmap, AD4851_REG_PACKET, 0);
+}
+
+static int ad4851_get_calibscale(struct ad4851_state *st, int ch, int *val, int *val2)
+{
+	unsigned int reg_val;
+	int gain;
+	int ret;
+
+	guard(mutex)(&st->lock);
+
+	ret = regmap_read(st->regmap, AD4851_REG_CHX_GAIN_MSB(ch), &reg_val);
+	if (ret)
+		return ret;
+
+	gain = reg_val << 8;
+
+	ret = regmap_read(st->regmap, AD4851_REG_CHX_GAIN_LSB(ch), &reg_val);
+	if (ret)
+		return ret;
+
+	gain |= reg_val;
+
+	*val = gain;
+	*val2 = 32768;
+
+	return IIO_VAL_FRACTIONAL;
+}
+
+static int ad4851_set_calibscale(struct ad4851_state *st, int ch, int val,
+				 int val2)
+{
+	u64 gain;
+	u8 buf[2];
+	int ret;
+
+	if (val < 0 || val2 < 0)
+		return -EINVAL;
+
+	gain = val * MICRO + val2;
+	gain = DIV_U64_ROUND_CLOSEST(gain * 32768, MICRO);
+
+	put_unaligned_be16(gain, buf);
+
+	guard(mutex)(&st->lock);
+
+	ret = regmap_write(st->regmap, AD4851_REG_CHX_GAIN_MSB(ch), buf[0]);
+	if (ret)
+		return ret;
+
+	return regmap_write(st->regmap, AD4851_REG_CHX_GAIN_LSB(ch), buf[1]);
+}
+
+static int ad4851_get_calibbias(struct ad4851_state *st, int ch, int *val)
+{
+	unsigned int lsb, mid, msb;
+	int ret;
+
+	guard(mutex)(&st->lock);
+
+	ret = regmap_read(st->regmap, AD4851_REG_CHX_OFFSET_MSB(ch),
+			  &msb);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(st->regmap, AD4851_REG_CHX_OFFSET_MID(ch),
+			  &mid);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(st->regmap, AD4851_REG_CHX_OFFSET_LSB(ch),
+			  &lsb);
+	if (ret)
+		return ret;
+
+	if (st->info->resolution == 16) {
+		*val = msb << 8;
+		*val |= mid;
+		*val = sign_extend32(*val, 15);
+	} else {
+		*val = msb << 12;
+		*val |= mid << 4;
+		*val |= lsb >> 4;
+		*val = sign_extend32(*val, 19);
+	}
+
+	return IIO_VAL_INT;
+}
+
+static int ad4851_set_calibbias(struct ad4851_state *st, int ch, int val)
+{
+	u8 buf[3] = { 0 };
+	int ret;
+
+	if (val < 0)
+		return -EINVAL;
+
+	if (st->info->resolution == 16)
+		put_unaligned_be16(val, buf);
+	else
+		put_unaligned_be24(val << 4, buf);
+
+	guard(mutex)(&st->lock);
+
+	ret = regmap_write(st->regmap, AD4851_REG_CHX_OFFSET_LSB(ch), buf[2]);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(st->regmap, AD4851_REG_CHX_OFFSET_MID(ch), buf[1]);
+	if (ret)
+		return ret;
+
+	return regmap_write(st->regmap, AD4851_REG_CHX_OFFSET_MSB(ch), buf[0]);
+}
+
+static void __ad4851_get_scale(struct ad4851_state *st, int scale_tbl,
+			       unsigned int *val, unsigned int *val2)
+{
+	const struct ad4851_chip_info *info = st->info;
+	const struct iio_chan_spec *chan = &info->channels[0];
+	unsigned int tmp;
+
+	tmp = ((unsigned long long)scale_tbl * MICRO) >> chan->scan_type.realbits;
+	*val = tmp / MICRO;
+	*val2 = tmp % MICRO;
+}
+
+static int ad4851_set_scale(struct ad4851_state *st,
+			    const struct iio_chan_spec *chan, int val, int val2)
+{
+	unsigned int scale_val[2];
+	unsigned int i;
+	const struct ad4851_scale *scale_table;
+	size_t table_size;
+
+	if (chan->differential) {
+		scale_table = ad4851_scale_table_diff;
+		table_size = ARRAY_SIZE(ad4851_scale_table_diff);
+	} else {
+		scale_table = ad4851_scale_table_se;
+		table_size = ARRAY_SIZE(ad4851_scale_table_se);
+	}
+
+	for (i = 0; i < table_size; i++) {
+		__ad4851_get_scale(st, scale_table[i].scale_val,
+				   &scale_val[0], &scale_val[1]);
+		if (scale_val[0] != val || scale_val[1] != val2)
+			continue;
+
+		return regmap_write(st->regmap,
+				    AD4851_REG_CHX_SOFTSPAN(chan->channel),
+				    scale_table[i].reg_val);
+	}
+
+	return -EINVAL;
+}
+
+static int ad4851_get_scale(struct ad4851_state *st,
+			    const struct iio_chan_spec *chan, int *val,
+			    int *val2)
+{
+	int i, softspan_val;
+	int ret;
+	const struct ad4851_scale *scale_table;
+	size_t table_size;
+
+	if (chan->differential) {
+		scale_table = ad4851_scale_table_diff;
+		table_size = ARRAY_SIZE(ad4851_scale_table_diff);
+	} else {
+		scale_table = ad4851_scale_table_se;
+		table_size = ARRAY_SIZE(ad4851_scale_table_se);
+	}
+
+	ret = regmap_read(st->regmap, AD4851_REG_CHX_SOFTSPAN(chan->channel),
+			  &softspan_val);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < table_size; i++) {
+		if (softspan_val == scale_table[i].reg_val)
+			break;
+	}
+
+	if (i == table_size)
+		return -EIO;
+
+	__ad4851_get_scale(st, scale_table[i].scale_val, val, val2);
+
+	return IIO_VAL_INT_PLUS_MICRO;
+}
+
+static int ad4851_scale_fill(struct ad4851_state *st)
+{
+	unsigned int i, val1, val2;
+
+	for (i = 0; i < ARRAY_SIZE(ad4851_scale_avail_se); i++) {
+		__ad4851_get_scale(st, ad4851_scale_avail_se[i], &val1, &val2);
+		st->scales_se[i][0] = val1;
+		st->scales_se[i][1] = val2;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(ad4851_scale_avail_diff); i++) {
+		__ad4851_get_scale(st, ad4851_scale_avail_diff[i], &val1, &val2);
+		st->scales_diff[i][0] = val1;
+		st->scales_diff[i][1] = val2;
+	}
+
+	return 0;
+}
+
+static int ad4851_read_raw(struct iio_dev *indio_dev,
+			   const struct iio_chan_spec *chan,
+			   int *val, int *val2, long info)
+{
+	struct ad4851_state *st = iio_priv(indio_dev);
+
+	switch (info) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*val = st->sampling_freq;
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_CALIBSCALE:
+		return ad4851_get_calibscale(st, chan->channel, val, val2);
+	case IIO_CHAN_INFO_SCALE:
+		return ad4851_get_scale(st, chan, val, val2);
+	case IIO_CHAN_INFO_CALIBBIAS:
+		return ad4851_get_calibbias(st, chan->channel, val);
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		return ad4851_get_oversampling_ratio(st, val);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad4851_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int val, int val2, long info)
+{
+	struct ad4851_state *st = iio_priv(indio_dev);
+
+	switch (info) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		return ad4851_set_sampling_freq(st, val);
+	case IIO_CHAN_INFO_SCALE:
+		return ad4851_set_scale(st, chan, val, val2);
+	case IIO_CHAN_INFO_CALIBSCALE:
+		return ad4851_set_calibscale(st, chan->channel, val, val2);
+	case IIO_CHAN_INFO_CALIBBIAS:
+		return ad4851_set_calibbias(st, chan->channel, val);
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		return ad4851_set_oversampling_ratio(st, chan, val);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad4851_update_scan_mode(struct iio_dev *indio_dev,
+				   const unsigned long *scan_mask)
+{
+	struct ad4851_state *st = iio_priv(indio_dev);
+	unsigned int c;
+	int ret;
+
+	for (c = 0; c < st->info->num_channels / 2; c++) {
+		if (test_bit(c, scan_mask))
+			ret = iio_backend_chan_enable(st->back, c);
+		else
+			ret = iio_backend_chan_disable(st->back, c);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int ad4851_read_avail(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     const int **vals, int *type, int *length,
+			     long mask)
+{
+	struct ad4851_state *st = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		if (chan->differential) {
+			*vals = (const int *)st->scales_diff;
+			*type = IIO_VAL_INT_PLUS_MICRO;
+			/* Values are stored in a 2D matrix */
+			*length = ARRAY_SIZE(ad4851_scale_avail_diff) * 2;
+		} else {
+			*vals = (const int *)st->scales_se;
+			*type = IIO_VAL_INT_PLUS_MICRO;
+			/* Values are stored in a 2D matrix */
+			*length = ARRAY_SIZE(ad4851_scale_avail_se) * 2;
+		}
+		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		*vals = ad4851_oversampling_ratios;
+		*length = ARRAY_SIZE(ad4851_oversampling_ratios);
+		*type = IIO_VAL_INT;
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_scan_type ad4851_scan_type_16 = {
+	.sign = 's',
+	.realbits = 16,
+	.storagebits = 16,
+};
+
+static const struct iio_scan_type ad4851_scan_type_20_0[] = {
+	[AD4851_SCAN_TYPE_NORMAL] = {
+		.sign = 'u',
+		.realbits = 20,
+		.storagebits = 32,
+	},
+	[AD4851_SCAN_TYPE_RESOLUTION_BOOST] = {
+		.sign = 'u',
+		.realbits = 24,
+		.storagebits = 32,
+	},
+};
+
+static const struct iio_scan_type ad4851_scan_type_20_1[] = {
+	[AD4851_SCAN_TYPE_NORMAL] = {
+		.sign = 's',
+		.realbits = 20,
+		.storagebits = 32,
+	},
+	[AD4851_SCAN_TYPE_RESOLUTION_BOOST] = {
+		.sign = 's',
+		.realbits = 24,
+		.storagebits = 32,
+	},
+};
+
+static int ad4851_get_current_scan_type(const struct iio_dev *indio_dev,
+					const struct iio_chan_spec *chan)
+{
+	struct ad4851_state *st = iio_priv(indio_dev);
+
+	return st->resolution_boost_enabled ? AD4851_SCAN_TYPE_RESOLUTION_BOOST
+					    : AD4851_SCAN_TYPE_NORMAL;
+}
+
+#define AD4851_IIO_CHANNEL(index, ch, diff)					\
+	.type = IIO_VOLTAGE,							\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_CALIBSCALE) |			\
+		BIT(IIO_CHAN_INFO_CALIBBIAS) |					\
+		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),				\
+	.info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),		\
+	.indexed = 1,								\
+	.differential = diff,							\
+	.channel = ch,								\
+	.channel2 = ch + (diff * 8),						\
+	.scan_index = index,							\
+
+#define AD4858_IIO_CHANNEL(index, ch, diff, bits)				\
+{										\
+	AD4851_IIO_CHANNEL(index, ch, diff)					\
+	.ext_scan_type = ad4851_scan_type_##bits##_##diff,			\
+	.num_ext_scan_type = ARRAY_SIZE(ad4851_scan_type_##bits##_##diff),	\
+}
+
+#define AD4857_IIO_CHANNEL(index, ch, diff, bits)				\
+{										\
+	AD4851_IIO_CHANNEL(index, ch, diff)					\
+	.scan_type = {								\
+		.sign = 's',							\
+		.realbits = bits,						\
+		.storagebits = bits,						\
+	},									\
+}
+
+static const struct iio_chan_spec ad4858_channels[] = {
+	AD4858_IIO_CHANNEL(0, 0, 0, 20),
+	AD4858_IIO_CHANNEL(1, 0, 1, 20),
+	AD4858_IIO_CHANNEL(2, 1, 0, 20),
+	AD4858_IIO_CHANNEL(3, 1, 1, 20),
+	AD4858_IIO_CHANNEL(4, 2, 0, 20),
+	AD4858_IIO_CHANNEL(5, 2, 1, 20),
+	AD4858_IIO_CHANNEL(6, 3, 0, 20),
+	AD4858_IIO_CHANNEL(7, 3, 1, 20),
+	AD4858_IIO_CHANNEL(8, 4, 0, 20),
+	AD4858_IIO_CHANNEL(9, 4, 1, 20),
+	AD4858_IIO_CHANNEL(10, 5, 0, 20),
+	AD4858_IIO_CHANNEL(11, 5, 1, 20),
+	AD4858_IIO_CHANNEL(12, 6, 0, 20),
+	AD4858_IIO_CHANNEL(13, 6, 1, 20),
+	AD4858_IIO_CHANNEL(14, 7, 0, 20),
+	AD4858_IIO_CHANNEL(15, 7, 1, 20),
+};
+
+static const struct iio_chan_spec ad4857_channels[] = {
+	AD4857_IIO_CHANNEL(0, 0, 0, 16),
+	AD4857_IIO_CHANNEL(1, 0, 1, 16),
+	AD4857_IIO_CHANNEL(2, 1, 0, 16),
+	AD4857_IIO_CHANNEL(3, 1, 1, 16),
+	AD4857_IIO_CHANNEL(4, 2, 0, 16),
+	AD4857_IIO_CHANNEL(5, 2, 1, 16),
+	AD4857_IIO_CHANNEL(6, 3, 0, 16),
+	AD4857_IIO_CHANNEL(7, 3, 1, 16),
+	AD4857_IIO_CHANNEL(8, 4, 0, 16),
+	AD4857_IIO_CHANNEL(9, 4, 1, 16),
+	AD4857_IIO_CHANNEL(10, 5, 0, 16),
+	AD4857_IIO_CHANNEL(11, 5, 1, 16),
+	AD4857_IIO_CHANNEL(12, 6, 0, 16),
+	AD4857_IIO_CHANNEL(13, 6, 1, 16),
+	AD4857_IIO_CHANNEL(14, 7, 0, 16),
+	AD4857_IIO_CHANNEL(15, 7, 1, 16),
+};
+
+static int ad4857_parse_channels(struct iio_dev *indio_dev)
+{
+	struct device *dev = indio_dev->dev.parent;
+	struct ad4851_state *st = iio_priv(indio_dev);
+	struct iio_chan_spec *ad4851_channels;
+	const struct iio_chan_spec ad4851_chan = AD4857_IIO_CHANNEL(0, 0, 0, 16);
+	const struct iio_chan_spec ad4851_chan_diff = AD4857_IIO_CHANNEL(0, 0, 1, 16);
+	unsigned int num_channels, index = 0, reg;
+	int ret;
+
+	num_channels = device_get_child_node_count(dev);
+	if (num_channels > AD4851_MAX_CH_NR)
+		return dev_err_probe(dev, -EINVAL, "Too many channels: %u\n",
+				     num_channels);
+
+	ad4851_channels = devm_kcalloc(dev, num_channels,
+				       sizeof(*ad4851_channels), GFP_KERNEL);
+	if (!ad4851_channels)
+		return -ENOMEM;
+
+	indio_dev->channels = ad4851_channels;
+	indio_dev->num_channels = num_channels;
+
+	device_for_each_child_node_scoped(dev, child) {
+		ret = fwnode_property_read_u32(child, "reg", &reg);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "Missing channel number\n");
+		if (fwnode_property_present(child, "diff-channels")) {
+			*ad4851_channels = ad4851_chan_diff;
+			ad4851_channels->scan_index = index++;
+			ad4851_channels->channel = reg;
+			ad4851_channels->channel2 = reg + AD4851_MAX_CH_NR;
+		} else {
+			*ad4851_channels = ad4851_chan;
+			ad4851_channels->scan_index = index++;
+			ad4851_channels->channel = reg;
+			ret = regmap_write(st->regmap, AD4851_REG_CHX_SOFTSPAN(reg),
+					   AD4851_SOFTSPAN_0V_40V);
+			if (ret)
+				return ret;
+		}
+		ad4851_channels++;
+	}
+
+	return 0;
+}
+
+static int ad4858_parse_channels(struct iio_dev *indio_dev)
+{
+	struct device *dev = indio_dev->dev.parent;
+	struct ad4851_state *st = iio_priv(indio_dev);
+	struct iio_chan_spec *ad4851_channels;
+	const struct iio_chan_spec ad4851_chan = AD4858_IIO_CHANNEL(0, 0, 0, 20);
+	const struct iio_chan_spec ad4851_chan_diff = AD4858_IIO_CHANNEL(0, 0, 1, 20);
+	unsigned int num_channels, index = 0, reg;
+	int ret;
+
+	num_channels = device_get_child_node_count(dev);
+	if (num_channels > AD4851_MAX_CH_NR)
+		return dev_err_probe(dev, -EINVAL, "Too many channels: %u\n",
+				     num_channels);
+
+	ad4851_channels = devm_kcalloc(dev, num_channels,
+				       sizeof(*ad4851_channels), GFP_KERNEL);
+	if (!ad4851_channels)
+		return -ENOMEM;
+
+	indio_dev->channels = ad4851_channels;
+	indio_dev->num_channels = num_channels;
+
+	device_for_each_child_node_scoped(dev, child) {
+		ret = fwnode_property_read_u32(child, "reg", &reg);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "Missing channel number\n");
+		if (fwnode_property_present(child, "diff-channels")) {
+			*ad4851_channels = ad4851_chan_diff;
+			ad4851_channels->scan_index = index++;
+			ad4851_channels->channel = reg;
+			ad4851_channels->channel2 = reg + AD4851_MAX_CH_NR;
+			ad4851_channels->ext_scan_type = ad4851_scan_type_20_1;
+			ad4851_channels->num_ext_scan_type = ARRAY_SIZE(ad4851_scan_type_20_1);
+
+		} else {
+			*ad4851_channels = ad4851_chan;
+			ad4851_channels->scan_index = index++;
+			ad4851_channels->channel = reg;
+			ad4851_channels->ext_scan_type = ad4851_scan_type_20_0;
+			ad4851_channels->num_ext_scan_type = ARRAY_SIZE(ad4851_scan_type_20_0);
+			ret = regmap_write(st->regmap, AD4851_REG_CHX_SOFTSPAN(reg),
+					   AD4851_SOFTSPAN_0V_40V);
+			if (ret)
+				return ret;
+		}
+		ad4851_channels++;
+	}
+
+	return 0;
+}
+
+static const struct ad4851_chip_info ad4851_info = {
+	.name = "ad4851",
+	.product_id = 0x67,
+	.max_sample_rate_hz = 250 * KILO,
+	.resolution = 16,
+	.parse_channels = ad4857_parse_channels,
+};
+
+static const struct ad4851_chip_info ad4852_info = {
+	.name = "ad4852",
+	.product_id = 0x66,
+	.max_sample_rate_hz = 250 * KILO,
+	.resolution = 20,
+	.parse_channels = ad4858_parse_channels,
+};
+
+static const struct ad4851_chip_info ad4853_info = {
+	.name = "ad4853",
+	.product_id = 0x65,
+	.max_sample_rate_hz = 1 * MEGA,
+	.resolution = 16,
+	.parse_channels = ad4857_parse_channels,
+};
+
+static const struct ad4851_chip_info ad4854_info = {
+	.name = "ad4854",
+	.product_id = 0x64,
+	.max_sample_rate_hz = 1 * MEGA,
+	.resolution = 20,
+	.parse_channels = ad4858_parse_channels,
+};
+
+static const struct ad4851_chip_info ad4855_info = {
+	.name = "ad4855",
+	.product_id = 0x63,
+	.max_sample_rate_hz = 250 * KILO,
+	.resolution = 16,
+	.parse_channels = ad4857_parse_channels,
+};
+
+static const struct ad4851_chip_info ad4856_info = {
+	.name = "ad4856",
+	.product_id = 0x62,
+	.max_sample_rate_hz = 250 * KILO,
+	.resolution = 20,
+	.parse_channels = ad4858_parse_channels,
+};
+
+static const struct ad4851_chip_info ad4857_info = {
+	.name = "ad4857",
+	.product_id = 0x61,
+	.max_sample_rate_hz = 1 * MEGA,
+	.resolution = 16,
+	.channels = ad4857_channels,
+	.num_channels = ARRAY_SIZE(ad4857_channels),
+	.parse_channels = ad4857_parse_channels,
+};
+
+static const struct ad4851_chip_info ad4858_info = {
+	.name = "ad4858",
+	.product_id = 0x60,
+	.max_sample_rate_hz = 1 * MEGA,
+	.resolution = 20,
+	.parse_channels = ad4858_parse_channels,
+};
+
+static const struct ad4851_chip_info ad4858i_info = {
+	.name = "ad4858i",
+	.product_id = 0x6F,
+	.max_sample_rate_hz = 1 * MEGA,
+	.resolution = 20,
+	.parse_channels = ad4858_parse_channels,
+};
+
+static const struct iio_info ad4851_iio_info = {
+	.debugfs_reg_access = ad4851_reg_access,
+	.read_raw = ad4851_read_raw,
+	.write_raw = ad4851_write_raw,
+	.update_scan_mode = ad4851_update_scan_mode,
+	.get_current_scan_type = &ad4851_get_current_scan_type,
+	.read_avail = ad4851_read_avail,
+};
+
+static const struct regmap_config regmap_config = {
+	.reg_bits = 16,
+	.val_bits = 8,
+	.read_flag_mask = BIT(7),
+};
+
+static const char * const ad4851_power_supplies[] = {
+	"vcc",	"vdd", "vee", "vio",
+};
+
+static int ad4851_probe(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev;
+	struct device *dev = &spi->dev;
+	struct ad4851_state *st;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+	st->spi = spi;
+
+	ret = devm_mutex_init(dev, &st->lock);
+	if (ret)
+		return ret;
+
+	ret = devm_regulator_bulk_get_enable(dev,
+					     ARRAY_SIZE(ad4851_power_supplies),
+					     ad4851_power_supplies);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to get and enable supplies\n");
+
+	ret = devm_regulator_get_enable_optional(dev, "vddh");
+	if (ret < 0 && ret != -ENODEV)
+		return dev_err_probe(dev, ret, "failed to enable vddh voltage\n");
+
+	ret = devm_regulator_get_enable_optional(dev, "vddl");
+	if (ret < 0 && ret != -ENODEV)
+		return dev_err_probe(dev, ret, "failed to enable vddl voltage\n");
+
+	st->vrefbuf = devm_regulator_get_optional(dev, "vrefbuf");
+	if (IS_ERR(st->vrefbuf)) {
+		if (PTR_ERR(st->vrefbuf) != -ENODEV)
+			return dev_err_probe(dev, PTR_ERR(st->vrefbuf),
+					     "Failed to get vrefbuf regulator\n");
+	}
+
+	st->vrefio = devm_regulator_get_optional(dev, "vrefio");
+	if (IS_ERR(st->vrefio)) {
+		if (PTR_ERR(st->vrefio) != -ENODEV)
+			return dev_err_probe(dev, PTR_ERR(st->vrefio),
+					     "Failed to get vrefio regulator\n");
+	}
+
+	st->pd_gpio = devm_gpiod_get_optional(dev, "pd", GPIOD_OUT_LOW);
+	if (IS_ERR(st->pd_gpio))
+		return dev_err_probe(dev, PTR_ERR(st->pd_gpio),
+				     "Error on requesting pd GPIO\n");
+
+	st->cnv = devm_pwm_get(dev, NULL);
+	if (IS_ERR(st->cnv))
+		return dev_err_probe(dev, PTR_ERR(st->cnv),
+				     "Error on requesting pwm\n");
+
+	ret = devm_add_action_or_reset(&st->spi->dev, ad4851_pwm_disable,
+				       st->cnv);
+	if (ret)
+		return ret;
+
+	st->info = spi_get_device_match_data(spi);
+	if (!st->info)
+		return -ENODEV;
+
+	st->regmap = devm_regmap_init_spi(spi, &regmap_config);
+	if (IS_ERR(st->regmap))
+		return PTR_ERR(st->regmap);
+
+	ret = ad4851_scale_fill(st);
+	if (ret)
+		return ret;
+
+	ret = ad4851_set_sampling_freq(st, HZ_PER_MHZ);
+	if (ret)
+		return ret;
+
+	ret = ad4851_setup(st);
+	if (ret)
+		return ret;
+
+	indio_dev->name = st->info->name;
+	indio_dev->info = &ad4851_iio_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	ret = st->info->parse_channels(indio_dev);
+	if (ret)
+		return ret;
+
+	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 = ad4851_calibrate(st);
+	if (ret)
+		return ret;
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static const struct of_device_id ad4851_of_match[] = {
+	{ .compatible = "adi,ad4851", .data = &ad4851_info, },
+	{ .compatible = "adi,ad4852", .data = &ad4852_info, },
+	{ .compatible = "adi,ad4853", .data = &ad4853_info, },
+	{ .compatible = "adi,ad4854", .data = &ad4854_info, },
+	{ .compatible = "adi,ad4855", .data = &ad4855_info, },
+	{ .compatible = "adi,ad4856", .data = &ad4856_info, },
+	{ .compatible = "adi,ad4857", .data = &ad4857_info, },
+	{ .compatible = "adi,ad4858", .data = &ad4858_info, },
+	{ .compatible = "adi,ad4858i", .data = &ad4858i_info, },
+	{ }
+};
+
+static const struct spi_device_id ad4851_spi_id[] = {
+	{ "ad4851", (kernel_ulong_t)&ad4851_info },
+	{ "ad4852", (kernel_ulong_t)&ad4852_info },
+	{ "ad4853", (kernel_ulong_t)&ad4853_info },
+	{ "ad4854", (kernel_ulong_t)&ad4854_info },
+	{ "ad4855", (kernel_ulong_t)&ad4855_info },
+	{ "ad4856", (kernel_ulong_t)&ad4856_info },
+	{ "ad4857", (kernel_ulong_t)&ad4857_info },
+	{ "ad4858", (kernel_ulong_t)&ad4858_info },
+	{ "ad4858i", (kernel_ulong_t)&ad4858i_info },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, ad4851_spi_id);
+
+static struct spi_driver ad4851_driver = {
+	.probe = ad4851_probe,
+	.driver = {
+		.name = "ad4851",
+		.of_match_table = ad4851_of_match,
+	},
+	.id_table = ad4851_spi_id,
+};
+module_spi_driver(ad4851_driver);
+
+MODULE_AUTHOR("Sergiu Cuciurean <sergiu.cuciurean@analog.com>");
+MODULE_AUTHOR("Dragos Bogdan <dragos.bogdan@analog.com>");
+MODULE_AUTHOR("Antoniu Miclaus <antoniu.miclaus@analog.com>");
+MODULE_DESCRIPTION("Analog Devices AD4851 DAS driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(IIO_BACKEND);
-- 
2.47.1
Re: [PATCH v7 8/8] iio: adc: ad4851: add ad485x driver
Posted by Uwe Kleine-König 2 weeks, 4 days ago
Hello Antoniu,

On Fri, Nov 29, 2024 at 05:35:46PM +0200, Antoniu Miclaus wrote:
> +static int ad4851_set_sampling_freq(struct ad4851_state *st, unsigned int freq)
> +{
> +	struct pwm_state cnv_state = {
> +		.duty_cycle = AD4851_T_CNVH_NS,
> +		.enabled = true,
> +	};
> +	int ret;
> +
> +	freq = clamp(freq, 1, st->info->max_sample_rate_hz);
> +
> +	cnv_state.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC, freq);
> +
> +	ret = pwm_apply_might_sleep(st->cnv, &cnv_state);
> +	if (ret)
> +		return ret;

If you want to be sure that pwm_apply_might_sleep() doesn't round down
.period (and .duty_cycle?) too much, you might consider using
pwm_round_waveform_might_sleep(). But note that this function only works
for two hwpwm drivers currently.

> +
> +	st->sampling_freq = freq;
> +
> +	return 0;
> +}
> +
> +static const int ad4851_oversampling_ratios[] = {
> +	1, 2, 4, 8, 16,	32, 64, 128,
> +	256, 512, 1024, 2048, 4096, 8192, 16384, 32768,
> +	65536,
> +};
> +
> +static int ad4851_osr_to_regval(int ratio)

This function is called with an unsigned parameter only.

> +{
> +	int i;
> +
> +	for (i = 1; i < ARRAY_SIZE(ad4851_oversampling_ratios); i++)
> +		if (ratio == ad4851_oversampling_ratios[i])
> +			return i - 1;

Given that ad4851_oversampling_ratios[i] == 1 << i you could simplify
that. Something like:

	if (is_power_of_2(ratio) && ratio <= 65536 && ratio > 0)
		return ilog2(ratio);

> +
> +	return -EINVAL;
> +}
> +
> +static int ad4851_set_oversampling_ratio(struct ad4851_state *st,
> +					 const struct iio_chan_spec *chan,
> +					 unsigned int osr)
> +{
> +	unsigned int val;
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	if (osr == 1) {
> +		ret = regmap_clear_bits(st->regmap, AD4851_REG_OVERSAMPLE,
> +					AD4851_OS_EN_MSK);
> +		if (ret)
> +			return ret;
> +
> +		ret = iio_backend_oversampling_disable(st->back);
> +		if (ret)
> +			return ret;
> +	} else {
> +		val = ad4851_osr_to_regval(osr);
> +		if (val < 0)
> +			return -EINVAL;
> +
> +		ret = regmap_set_bits(st->regmap, AD4851_REG_OVERSAMPLE,
> +				      AD4851_OS_EN_MSK);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_update_bits(st->regmap, AD4851_REG_OVERSAMPLE,
> +					 AD4851_OS_RATIO_MSK, val);

You set this register twice in a row. Can this be done in a single
register access?

> +		if (ret)
> +			return ret;

Best regards
Uwe
Re: [PATCH v7 8/8] iio: adc: ad4851: add ad485x driver
Posted by David Lechner 3 weeks ago
On 11/29/24 9:35 AM, Antoniu Miclaus wrote:
> Add support for the AD485X a fully buffered, 8-channel simultaneous
> sampling, 16/20-bit, 1 MSPS data acquisition system (DAS) with
> differential, wide common-mode range inputs.
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> ---

...

> +static const struct ad4851_scale ad4851_scale_table_se[] = {
> +	{ 2500, 0x0 },
> +	{ 5000, 0x2 },
> +	{ 6250, 0x4 },
> +	{ 10000, 0x6 },
> +	{ 12500, 0x8 },
> +	{ 20000, 0xA },
> +	{ 25000, 0xC },
> +	{ 40000, 0xE },
> +};
> +
> +static const struct ad4851_scale ad4851_scale_table_diff[] = {
> +	{ 5000, 0x1 },
> +	{ 10000, 0x3 },
> +	{ 12500, 0x5 },
> +	{ 20000, 0x7 },
> +	{ 25000, 0x9 },
> +	{ 40000, 0xB },
> +	{ 50000, 0xD },
> +	{ 80000, 0xF },
> +};

The logic circuits in my brain thank you for separating these. :-)

Much easier for me to understand how the code works now.

> +
> +static const unsigned int ad4851_scale_avail_se[] = {
> +	2500,
> +	5000,
> +	6250,
> +	10000,
> +	12500,
> +	20000,
> +	25000,
> +	40000,
> +};
> +
> +static const unsigned int ad4851_scale_avail_diff[] = {
> +	5000,
> +	10000,
> +	12500,
> +	20000,
> +	25000,
> +	40000,
> +	50000,
> +	80000,
> +};
> +
> +struct ad4851_chip_info {
> +	const char *name;
> +	unsigned int product_id;
> +	int num_scales;
> +	const struct iio_chan_spec *channels;
> +	unsigned int num_channels;
> +	unsigned long max_sample_rate_hz;
> +	unsigned int resolution;
> +	int (*parse_channels)(struct iio_dev *indio_dev);
> +};
> +
> +enum {
> +	AD4851_SCAN_TYPE_NORMAL,
> +	AD4851_SCAN_TYPE_RESOLUTION_BOOST,
> +};
> +
> +struct ad4851_state {
> +	struct spi_device *spi;
> +	struct pwm_device *cnv;
> +	struct iio_backend *back;
> +	/*
> +	 * Synchronize access to members the of driver state, and ensure
> +	 * atomicity of consecutive regmap operations.
> +	 */
> +	struct mutex lock;
> +	struct regmap *regmap;
> +	struct regulator *vrefbuf;
> +	struct regulator *vrefio;
> +	const struct ad4851_chip_info *info;
> +	struct gpio_desc *pd_gpio;
> +	bool resolution_boost_enabled;
> +	unsigned long sampling_freq;
> +	unsigned int scales_se[AD4841_MAX_SCALE_AVAIL][2];
> +	unsigned int scales_diff[AD4841_MAX_SCALE_AVAIL][2];
> +};
> +
> +static int ad4851_reg_access(struct iio_dev *indio_dev,
> +			     unsigned int reg,
> +			     unsigned int writeval,
> +			     unsigned int *readval)
> +{
> +	struct ad4851_state *st = iio_priv(indio_dev);
> +
> +	guard(mutex)(&st->lock);
> +
> +	if (readval)
> +		return regmap_read(st->regmap, reg, readval);
> +
> +	return regmap_write(st->regmap, reg, writeval);
> +}
> +
> +static int ad4851_set_sampling_freq(struct ad4851_state *st, unsigned int freq)
> +{
> +	struct pwm_state cnv_state = {
> +		.duty_cycle = AD4851_T_CNVH_NS,

This still doesn't seem safe due to possible rounding of this minium
time. See previous reviews.

> +		.enabled = true,
> +	};
> +	int ret;
> +
> +	freq = clamp(freq, 1, st->info->max_sample_rate_hz);
> +
> +	cnv_state.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC, freq);
> +
> +	ret = pwm_apply_might_sleep(st->cnv, &cnv_state);
> +	if (ret)
> +		return ret;
> +

There is now a pwm_get_state_hw() function we can use to get the actual
PWM period the hardware actually selected. We can use that here to more
accuratly set st->sampling_freq instead of using the requested value.

> +	st->sampling_freq = freq;
> +
> +	return 0;
> +}
> +
> +static const int ad4851_oversampling_ratios[] = {
> +	1, 2, 4, 8, 16,	32, 64, 128,
> +	256, 512, 1024, 2048, 4096, 8192, 16384, 32768,
> +	65536,
> +};
> +
> +static int ad4851_osr_to_regval(int ratio)
> +{
> +	int i;
> +
> +	for (i = 1; i < ARRAY_SIZE(ad4851_oversampling_ratios); i++)
> +		if (ratio == ad4851_oversampling_ratios[i])
> +			return i - 1;
> +
> +	return -EINVAL;
> +}
> +
> +static int ad4851_set_oversampling_ratio(struct ad4851_state *st,
> +					 const struct iio_chan_spec *chan,
> +					 unsigned int osr)
> +{
> +	unsigned int val;
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	if (osr == 1) {
> +		ret = regmap_clear_bits(st->regmap, AD4851_REG_OVERSAMPLE,
> +					AD4851_OS_EN_MSK);
> +		if (ret)
> +			return ret;
> +
> +		ret = iio_backend_oversampling_disable(st->back);
> +		if (ret)
> +			return ret;
> +	} else {
> +		val = ad4851_osr_to_regval(osr);
> +		if (val < 0)
> +			return -EINVAL;
> +
> +		ret = regmap_set_bits(st->regmap, AD4851_REG_OVERSAMPLE,
> +				      AD4851_OS_EN_MSK);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_update_bits(st->regmap, AD4851_REG_OVERSAMPLE,
> +					 AD4851_OS_RATIO_MSK, val);

Wouldn't hurt to use FIELD_PREP() on val here.

> +		if (ret)
> +			return ret;
> +
> +		ret = iio_backend_oversampling_enable(st->back);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	switch (chan->scan_type.realbits) {

When using ext_scan_type, we have to use iio_get_current_scan_type()
to get the scan type instead of chan->scan_type. Otherwise we will
be accessing a member of a union that is currently being used for
something else and will get some unknown value.

We also have to be extra careful here since changing the oversampling
ratio will change the scan type. So here, it would probably be best to
use the resolution from the chip info instead of scan_type to get the
16 or 20. Otherwise, realbits could be 24 if oversampling is already
enabled on a 20-bit chip.

> +	case 20:
> +		switch (osr) {
> +		case 0:
> +			return -EINVAL;
> +		case 1:
> +			val = 20;
> +			break;
> +		default:
> +			val = 24;
> +			break;
> +		}
> +		break;
> +	case 16:
> +		val = 16;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	ret = iio_backend_data_size_set(st->back, val);
> +	if (ret)
> +		return ret;
> +
> +	if (osr == 1 || chan->scan_type.realbits == 16) {

Again here, consider chip info resolution instead of scan_type.

> +		ret = regmap_clear_bits(st->regmap, AD4851_REG_PACKET,
> +					AD4851_PACKET_FORMAT_MASK);
> +		if (ret)
> +			return ret;
> +
> +		st->resolution_boost_enabled = false;
> +	} else {
> +		ret = regmap_update_bits(st->regmap, AD4851_REG_PACKET,
> +					 AD4851_PACKET_FORMAT_MASK,
> +					 FIELD_PREP(AD4851_PACKET_FORMAT_MASK, 1));
> +		if (ret)
> +			return ret;
> +
> +		st->resolution_boost_enabled = true;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ad4851_get_oversampling_ratio(struct ad4851_state *st, unsigned int *val)
> +{
> +	unsigned int osr;
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	ret = regmap_read(st->regmap, AD4851_REG_OVERSAMPLE, &osr);
> +	if (ret)
> +		return ret;
> +
> +	if (!FIELD_GET(AD4851_OS_EN_MSK, osr))
> +		*val = 1;
> +	else
> +		*val = ad4851_oversampling_ratios[FIELD_GET(AD4851_OS_RATIO_MSK, osr)];
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static void ad4851_reg_disable(void *data)
> +{
> +	regulator_disable(data);
> +}
> +
> +static void ad4851_pwm_disable(void *data)
> +{
> +	pwm_disable(data);
> +}
> +
> +static int ad4851_setup(struct ad4851_state *st)
> +{
> +	unsigned int product_id;
> +	int ret;
> +
> +	if (!IS_ERR(st->vrefbuf)) {
> +		ret = regmap_set_bits(st->regmap, AD4851_REG_DEVICE_CTRL,
> +				      AD4851_REFBUF_PD);
> +		if (ret)
> +			return ret;
> +
> +		ret = regulator_enable(st->vrefbuf);
> +		if (ret)
> +			return ret;
> +
> +		ret = devm_add_action_or_reset(&st->spi->dev, ad4851_reg_disable,
> +					       st->vrefbuf);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (!IS_ERR(st->vrefio)) {
> +		ret = regmap_set_bits(st->regmap, AD4851_REG_DEVICE_CTRL,
> +				      AD4851_REFSEL_PD);
> +		if (ret)
> +			return ret;
> +
> +		ret = regulator_enable(st->vrefio);
> +		if (ret)
> +			return ret;
> +
> +		ret = devm_add_action_or_reset(&st->spi->dev, ad4851_reg_disable,
> +					       st->vrefio);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (st->pd_gpio) {
> +		/* To initiate a global reset, bring the PD pin high twice */
> +		gpiod_set_value(st->pd_gpio, 1);
> +		fsleep(1);
> +		gpiod_set_value(st->pd_gpio, 0);
> +		fsleep(1);
> +		gpiod_set_value(st->pd_gpio, 1);
> +		fsleep(1);
> +		gpiod_set_value(st->pd_gpio, 0);
> +		fsleep(1000);
> +	} else {
> +		ret = regmap_set_bits(st->regmap, AD4851_REG_INTERFACE_CONFIG_A,
> +				      AD4851_SW_RESET);
> +		if (ret)
> +			return ret;
> +	}

Resetting here will reset the REFBUF/REFSEL that were already configured
in the DEVICE_CTRL register. The reset should be done before any other config.

> +
> +	ret = regmap_write(st->regmap, AD4851_REG_INTERFACE_CONFIG_B,
> +			   AD4851_SINGLE_INSTRUCTION);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD4851_REG_INTERFACE_CONFIG_A,
> +			   AD4851_SDO_ENABLE);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(st->regmap, AD4851_REG_PRODUCT_ID_L, &product_id);
> +	if (ret)
> +		return ret;
> +
> +	if (product_id != st->info->product_id)
> +		dev_info(&st->spi->dev, "Unknown product ID: 0x%02X\n",
> +			 product_id);
> +
> +	ret = regmap_set_bits(st->regmap, AD4851_REG_DEVICE_CTRL,
> +			      AD4851_ECHO_CLOCK_MODE);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_write(st->regmap, AD4851_REG_PACKET, 0);
> +}
> +

...

> +
> +static void __ad4851_get_scale(struct ad4851_state *st, int scale_tbl,
> +			       unsigned int *val, unsigned int *val2)
> +{
> +	const struct ad4851_chip_info *info = st->info;
> +	const struct iio_chan_spec *chan = &info->channels[0];
> +	unsigned int tmp;
> +
> +	tmp = ((unsigned long long)scale_tbl * MICRO) >> chan->scan_type.realbits;

As above, we need to use iio_get_current_scan_type() to safely get the
current scan_type struct.

Can also abbrivate unsigned long long as u64.

> +	*val = tmp / MICRO;
> +	*val2 = tmp % MICRO;
> +}
> +

...

> +
> +static int ad4851_scale_fill(struct ad4851_state *st)
> +{
> +	unsigned int i, val1, val2;
> +
> +	for (i = 0; i < ARRAY_SIZE(ad4851_scale_avail_se); i++) {
> +		__ad4851_get_scale(st, ad4851_scale_avail_se[i], &val1, &val2);
> +		st->scales_se[i][0] = val1;
> +		st->scales_se[i][1] = val2;
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(ad4851_scale_avail_diff); i++) {
> +		__ad4851_get_scale(st, ad4851_scale_avail_diff[i], &val1, &val2);
> +		st->scales_diff[i][0] = val1;
> +		st->scales_diff[i][1] = val2;
> +	}

Isn't this just copying the static structs directly? If that is the case,
why not just use the static structs directly in the code instead of making
a copy?

> +
> +	return 0;
> +}
> +
> +static int ad4851_read_raw(struct iio_dev *indio_dev,
> +			   const struct iio_chan_spec *chan,
> +			   int *val, int *val2, long info)
> +{
> +	struct ad4851_state *st = iio_priv(indio_dev);
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*val = st->sampling_freq;

I didn't think about this before, but oversampling ratio will affect the
sampling freqency. Userspace tools like iio-oscilloscope expect the rate
reported by the sampling_frequency attribute to mean how often we read one
sample from the chip or one batch of samples from a simeltaneous sampling
chip like this one. But st->sampling_freq is the rate at which we are
toggling the CNV pin. When oversampling is enabled, we toggle CNV N times
before doing a read where N is the oversamping ratio. So to get the correct
value to userspace, we need to divide st->st->sampling_freq by the
oversampling ratio.

Could also be a good idea to rename st->sampling_freq to something like
st->cnv_trigger_rate_hz or something like that to avoid confusion.

> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_CALIBSCALE:
> +		return ad4851_get_calibscale(st, chan->channel, val, val2);
> +	case IIO_CHAN_INFO_SCALE:
> +		return ad4851_get_scale(st, chan, val, val2);
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		return ad4851_get_calibbias(st, chan->channel, val);
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		return ad4851_get_oversampling_ratio(st, val);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad4851_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int val, int val2, long info)
> +{
> +	struct ad4851_state *st = iio_priv(indio_dev);
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:

Similar to above, we need to take into account oversampling ratio here.

> +		return ad4851_set_sampling_freq(st, val);
> +	case IIO_CHAN_INFO_SCALE:
> +		return ad4851_set_scale(st, chan, val, val2);
> +	case IIO_CHAN_INFO_CALIBSCALE:
> +		return ad4851_set_calibscale(st, chan->channel, val, val2);
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		return ad4851_set_calibbias(st, chan->channel, val);
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		return ad4851_set_oversampling_ratio(st, chan, val);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad4851_update_scan_mode(struct iio_dev *indio_dev,
> +				   const unsigned long *scan_mask)
> +{
> +	struct ad4851_state *st = iio_priv(indio_dev);
> +	unsigned int c;
> +	int ret;
> +
> +	for (c = 0; c < st->info->num_channels / 2; c++) {

This doesn't look quite right. This means only the first half of the
channels could ever be enabled. Also scan_index doesn't correspond to
channel number any more.

Since each physical input has 2 IIO channels with consecutive scan_index,
we could read 2 bits at a time from the scan_mask.

If that value == 0x3, then return -EINVAL because we tried to enable
both the differential and single-ended input on the same physical
input pin.

If the value is 0x0, disable the channel in the backend. Otherwise,
enable the channel in the backend (value is 0x1 or 0x2).

> +		if (test_bit(c, scan_mask))
> +			ret = iio_backend_chan_enable(st->back, c);
> +		else
> +			ret = iio_backend_chan_disable(st->back, c);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ad4851_read_avail(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     const int **vals, int *type, int *length,
> +			     long mask)
> +{
> +	struct ad4851_state *st = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		if (chan->differential) {
> +			*vals = (const int *)st->scales_diff;
> +			*type = IIO_VAL_INT_PLUS_MICRO;
> +			/* Values are stored in a 2D matrix */
> +			*length = ARRAY_SIZE(ad4851_scale_avail_diff) * 2;
> +		} else {
> +			*vals = (const int *)st->scales_se;
> +			*type = IIO_VAL_INT_PLUS_MICRO;
> +			/* Values are stored in a 2D matrix */
> +			*length = ARRAY_SIZE(ad4851_scale_avail_se) * 2;
> +		}

This one gets a bit tricky since on 20-bit chips, enabling oversampling
also changes the number of realbits. So we need different list of scales
available in that case.

> +		return IIO_AVAIL_LIST;
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		*vals = ad4851_oversampling_ratios;
> +		*length = ARRAY_SIZE(ad4851_oversampling_ratios);
> +		*type = IIO_VAL_INT;
> +		return IIO_AVAIL_LIST;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_scan_type ad4851_scan_type_16 = {
> +	.sign = 's',
> +	.realbits = 16,
> +	.storagebits = 16,
> +};

Don't we need an unsigned version of this one too? Or is this struct
unused and can be removed?

> +
> +static const struct iio_scan_type ad4851_scan_type_20_0[] = {
> +	[AD4851_SCAN_TYPE_NORMAL] = {
> +		.sign = 'u',
> +		.realbits = 20,
> +		.storagebits = 32,
> +	},
> +	[AD4851_SCAN_TYPE_RESOLUTION_BOOST] = {
> +		.sign = 'u',
> +		.realbits = 24,
> +		.storagebits = 32,
> +	},
> +};
> +
> +static const struct iio_scan_type ad4851_scan_type_20_1[] = {
> +	[AD4851_SCAN_TYPE_NORMAL] = {
> +		.sign = 's',
> +		.realbits = 20,
> +		.storagebits = 32,
> +	},
> +	[AD4851_SCAN_TYPE_RESOLUTION_BOOST] = {
> +		.sign = 's',
> +		.realbits = 24,
> +		.storagebits = 32,
> +	},
> +};
> +
> +static int ad4851_get_current_scan_type(const struct iio_dev *indio_dev,
> +					const struct iio_chan_spec *chan)
> +{
> +	struct ad4851_state *st = iio_priv(indio_dev);
> +
> +	return st->resolution_boost_enabled ? AD4851_SCAN_TYPE_RESOLUTION_BOOST
> +					    : AD4851_SCAN_TYPE_NORMAL;
> +}
> +
> +#define AD4851_IIO_CHANNEL(index, ch, diff)					\
> +	.type = IIO_VOLTAGE,							\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_CALIBSCALE) |			\
> +		BIT(IIO_CHAN_INFO_CALIBBIAS) |					\
> +		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),				\
> +	.info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),		\
> +	.indexed = 1,								\
> +	.differential = diff,							\
> +	.channel = ch,								\
> +	.channel2 = ch + (diff * 8),						\

Did you mean diff * (ch + 8)?

> +	.scan_index = index,							\
> +
> +#define AD4858_IIO_CHANNEL(index, ch, diff, bits)				\
> +{										\
> +	AD4851_IIO_CHANNEL(index, ch, diff)					\
> +	.ext_scan_type = ad4851_scan_type_##bits##_##diff,			\
> +	.num_ext_scan_type = ARRAY_SIZE(ad4851_scan_type_##bits##_##diff),	\
> +}
> +
> +#define AD4857_IIO_CHANNEL(index, ch, diff, bits)				\
> +{										\
> +	AD4851_IIO_CHANNEL(index, ch, diff)					\
> +	.scan_type = {								\
> +		.sign = 's',							\

sign should be 's' for diff and 'u' for !diff

> +		.realbits = bits,						\
> +		.storagebits = bits,						\

Could just hard code 16 here and avoid passing it as a parameter.

> +	},									\
> +}
> +
> +static const struct iio_chan_spec ad4858_channels[] = {
> +	AD4858_IIO_CHANNEL(0, 0, 0, 20),
> +	AD4858_IIO_CHANNEL(1, 0, 1, 20),
> +	AD4858_IIO_CHANNEL(2, 1, 0, 20),
> +	AD4858_IIO_CHANNEL(3, 1, 1, 20),
> +	AD4858_IIO_CHANNEL(4, 2, 0, 20),
> +	AD4858_IIO_CHANNEL(5, 2, 1, 20),
> +	AD4858_IIO_CHANNEL(6, 3, 0, 20),
> +	AD4858_IIO_CHANNEL(7, 3, 1, 20),
> +	AD4858_IIO_CHANNEL(8, 4, 0, 20),
> +	AD4858_IIO_CHANNEL(9, 4, 1, 20),
> +	AD4858_IIO_CHANNEL(10, 5, 0, 20),
> +	AD4858_IIO_CHANNEL(11, 5, 1, 20),
> +	AD4858_IIO_CHANNEL(12, 6, 0, 20),
> +	AD4858_IIO_CHANNEL(13, 6, 1, 20),
> +	AD4858_IIO_CHANNEL(14, 7, 0, 20),
> +	AD4858_IIO_CHANNEL(15, 7, 1, 20),
> +};
> +
> +static const struct iio_chan_spec ad4857_channels[] = {
> +	AD4857_IIO_CHANNEL(0, 0, 0, 16),
> +	AD4857_IIO_CHANNEL(1, 0, 1, 16),
> +	AD4857_IIO_CHANNEL(2, 1, 0, 16),
> +	AD4857_IIO_CHANNEL(3, 1, 1, 16),
> +	AD4857_IIO_CHANNEL(4, 2, 0, 16),
> +	AD4857_IIO_CHANNEL(5, 2, 1, 16),
> +	AD4857_IIO_CHANNEL(6, 3, 0, 16),
> +	AD4857_IIO_CHANNEL(7, 3, 1, 16),
> +	AD4857_IIO_CHANNEL(8, 4, 0, 16),
> +	AD4857_IIO_CHANNEL(9, 4, 1, 16),
> +	AD4857_IIO_CHANNEL(10, 5, 0, 16),
> +	AD4857_IIO_CHANNEL(11, 5, 1, 16),
> +	AD4857_IIO_CHANNEL(12, 6, 0, 16),
> +	AD4857_IIO_CHANNEL(13, 6, 1, 16),
> +	AD4857_IIO_CHANNEL(14, 7, 0, 16),
> +	AD4857_IIO_CHANNEL(15, 7, 1, 16),
> +};
> +
> +static int ad4857_parse_channels(struct iio_dev *indio_dev)
> +{
> +	struct device *dev = indio_dev->dev.parent;
> +	struct ad4851_state *st = iio_priv(indio_dev);
> +	struct iio_chan_spec *ad4851_channels;
> +	const struct iio_chan_spec ad4851_chan = AD4857_IIO_CHANNEL(0, 0, 0, 16);
> +	const struct iio_chan_spec ad4851_chan_diff = AD4857_IIO_CHANNEL(0, 0, 1, 16);
> +	unsigned int num_channels, index = 0, reg;
> +	int ret;
> +
> +	num_channels = device_get_child_node_count(dev);
> +	if (num_channels > AD4851_MAX_CH_NR)
> +		return dev_err_probe(dev, -EINVAL, "Too many channels: %u\n",
> +				     num_channels);
> +
> +	ad4851_channels = devm_kcalloc(dev, num_channels,
> +				       sizeof(*ad4851_channels), GFP_KERNEL);
> +	if (!ad4851_channels)
> +		return -ENOMEM;
> +
> +	indio_dev->channels = ad4851_channels;
> +	indio_dev->num_channels = num_channels;
> +
> +	device_for_each_child_node_scoped(dev, child) {
> +		ret = fwnode_property_read_u32(child, "reg", &reg);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Missing channel number\n");
> +		if (fwnode_property_present(child, "diff-channels")) {
> +			*ad4851_channels = ad4851_chan_diff;
> +			ad4851_channels->scan_index = index++;
> +			ad4851_channels->channel = reg;
> +			ad4851_channels->channel2 = reg + AD4851_MAX_CH_NR;
> +		} else {
> +			*ad4851_channels = ad4851_chan;
> +			ad4851_channels->scan_index = index++;
> +			ad4851_channels->channel = reg;
> +			ret = regmap_write(st->regmap, AD4851_REG_CHX_SOFTSPAN(reg),
> +					   AD4851_SOFTSPAN_0V_40V);
> +			if (ret)
> +				return ret;
> +		}
> +		ad4851_channels++;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ad4858_parse_channels(struct iio_dev *indio_dev)
> +{
> +	struct device *dev = indio_dev->dev.parent;
> +	struct ad4851_state *st = iio_priv(indio_dev);
> +	struct iio_chan_spec *ad4851_channels;
> +	const struct iio_chan_spec ad4851_chan = AD4858_IIO_CHANNEL(0, 0, 0, 20);
> +	const struct iio_chan_spec ad4851_chan_diff = AD4858_IIO_CHANNEL(0, 0, 1, 20);
> +	unsigned int num_channels, index = 0, reg;
> +	int ret;
> +
> +	num_channels = device_get_child_node_count(dev);
> +	if (num_channels > AD4851_MAX_CH_NR)
> +		return dev_err_probe(dev, -EINVAL, "Too many channels: %u\n",
> +				     num_channels);
> +
> +	ad4851_channels = devm_kcalloc(dev, num_channels,
> +				       sizeof(*ad4851_channels), GFP_KERNEL);
> +	if (!ad4851_channels)
> +		return -ENOMEM;
> +
> +	indio_dev->channels = ad4851_channels;
> +	indio_dev->num_channels = num_channels;
> +
> +	device_for_each_child_node_scoped(dev, child) {
> +		ret = fwnode_property_read_u32(child, "reg", &reg);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Missing channel number\n");
> +		if (fwnode_property_present(child, "diff-channels")) {
> +			*ad4851_channels = ad4851_chan_diff;
> +			ad4851_channels->scan_index = index++;
> +			ad4851_channels->channel = reg;
> +			ad4851_channels->channel2 = reg + AD4851_MAX_CH_NR;
> +			ad4851_channels->ext_scan_type = ad4851_scan_type_20_1;
> +			ad4851_channels->num_ext_scan_type = ARRAY_SIZE(ad4851_scan_type_20_1);
> +
> +		} else {
> +			*ad4851_channels = ad4851_chan;
> +			ad4851_channels->scan_index = index++;
> +			ad4851_channels->channel = reg;
> +			ad4851_channels->ext_scan_type = ad4851_scan_type_20_0;
> +			ad4851_channels->num_ext_scan_type = ARRAY_SIZE(ad4851_scan_type_20_0);
> +			ret = regmap_write(st->regmap, AD4851_REG_CHX_SOFTSPAN(reg),
> +					   AD4851_SOFTSPAN_0V_40V);
> +			if (ret)
> +				return ret;
> +		}
> +		ad4851_channels++;
> +	}
> +
> +	return 0;
> +}

Do we actually need these 2 channel parsing functions? I thought the goal
here was to provide both a differential and single-ended version of each
channel always so we wouldn't be limited to one or the other by the
devicetree.

static const struct iio_chan_spec ad4858_channels and ad4857_channels seem
like they should be sufficint to me without dynamically creating channels
during probe.
Re: [PATCH v7 8/8] iio: adc: ad4851: add ad485x driver
Posted by Jonathan Cameron 3 weeks, 4 days ago
On Fri, 29 Nov 2024 17:35:46 +0200
Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:

> Add support for the AD485X a fully buffered, 8-channel simultaneous
> sampling, 16/20-bit, 1 MSPS data acquisition system (DAS) with
> differential, wide common-mode range inputs.
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> ---
> changes in v7:
>  - use new iio backend os enable/disable functions
>  - implement separate scan_type for both signed and unsigned.
>  - drop ext_scan_type for 16-bit chips
>  - rework scan_index ordering.
>  - add separate scales for diff/single-ended channels
>  - parse iio channels via dts properties.
Hi Antoniu

The bot clearly found a few places where data got added but not used
that need fixing up.  Some other comments below.

> diff --git a/drivers/iio/adc/ad4851.c b/drivers/iio/adc/ad4851.c
> new file mode 100644
> index 000000000000..e8e5c0def29e
> --- /dev/null
> +++ b/drivers/iio/adc/ad4851.c
> @@ -0,0 +1,1346 @@

> +struct ad4851_chip_info {
> +	const char *name;
> +	unsigned int product_id;
> +	int num_scales;
> +	const struct iio_chan_spec *channels;
> +	unsigned int num_channels;
Some of these appear to be optional. If so, I think this structure should
have some docs to explain why.

> +	unsigned long max_sample_rate_hz;
> +	unsigned int resolution;
> +	int (*parse_channels)(struct iio_dev *indio_dev);
> +};


> +#define AD4851_IIO_CHANNEL(index, ch, diff)					\
> +	.type = IIO_VOLTAGE,							\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_CALIBSCALE) |			\
> +		BIT(IIO_CHAN_INFO_CALIBBIAS) |					\
> +		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),				\
> +	.info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),		\
> +	.indexed = 1,								\
> +	.differential = diff,							\
> +	.channel = ch,								\
> +	.channel2 = ch + (diff * 8),						\
> +	.scan_index = index,							\
Why the final line continuation?

> +
> +#define AD4858_IIO_CHANNEL(index, ch, diff, bits)				\
> +{										\
> +	AD4851_IIO_CHANNEL(index, ch, diff)					\
> +	.ext_scan_type = ad4851_scan_type_##bits##_##diff,			\
> +	.num_ext_scan_type = ARRAY_SIZE(ad4851_scan_type_##bits##_##diff),	\

Seems you set this again below.

> +}
> +
> +#define AD4857_IIO_CHANNEL(index, ch, diff, bits)				\
> +{										\
> +	AD4851_IIO_CHANNEL(index, ch, diff)					\
> +	.scan_type = {								\
> +		.sign = 's',							\
> +		.realbits = bits,						\
> +		.storagebits = bits,						\
> +	},									\
> +}
> +
> +static const struct iio_chan_spec ad4858_channels[] = {
> +	AD4858_IIO_CHANNEL(0, 0, 0, 20),
> +	AD4858_IIO_CHANNEL(1, 0, 1, 20),
> +	AD4858_IIO_CHANNEL(2, 1, 0, 20),
> +	AD4858_IIO_CHANNEL(3, 1, 1, 20),
> +	AD4858_IIO_CHANNEL(4, 2, 0, 20),
> +	AD4858_IIO_CHANNEL(5, 2, 1, 20),
> +	AD4858_IIO_CHANNEL(6, 3, 0, 20),
> +	AD4858_IIO_CHANNEL(7, 3, 1, 20),
> +	AD4858_IIO_CHANNEL(8, 4, 0, 20),
> +	AD4858_IIO_CHANNEL(9, 4, 1, 20),
> +	AD4858_IIO_CHANNEL(10, 5, 0, 20),
> +	AD4858_IIO_CHANNEL(11, 5, 1, 20),
> +	AD4858_IIO_CHANNEL(12, 6, 0, 20),
> +	AD4858_IIO_CHANNEL(13, 6, 1, 20),
> +	AD4858_IIO_CHANNEL(14, 7, 0, 20),
> +	AD4858_IIO_CHANNEL(15, 7, 1, 20),
> +};
> +
> +static const struct iio_chan_spec ad4857_channels[] = {
> +	AD4857_IIO_CHANNEL(0, 0, 0, 16),
> +	AD4857_IIO_CHANNEL(1, 0, 1, 16),
> +	AD4857_IIO_CHANNEL(2, 1, 0, 16),
> +	AD4857_IIO_CHANNEL(3, 1, 1, 16),
> +	AD4857_IIO_CHANNEL(4, 2, 0, 16),
> +	AD4857_IIO_CHANNEL(5, 2, 1, 16),
> +	AD4857_IIO_CHANNEL(6, 3, 0, 16),
> +	AD4857_IIO_CHANNEL(7, 3, 1, 16),
> +	AD4857_IIO_CHANNEL(8, 4, 0, 16),
> +	AD4857_IIO_CHANNEL(9, 4, 1, 16),
> +	AD4857_IIO_CHANNEL(10, 5, 0, 16),
> +	AD4857_IIO_CHANNEL(11, 5, 1, 16),
> +	AD4857_IIO_CHANNEL(12, 6, 0, 16),
> +	AD4857_IIO_CHANNEL(13, 6, 1, 16),
> +	AD4857_IIO_CHANNEL(14, 7, 0, 16),
> +	AD4857_IIO_CHANNEL(15, 7, 1, 16),
> +};
> +
> +static int ad4857_parse_channels(struct iio_dev *indio_dev)
> +{
> +	struct device *dev = indio_dev->dev.parent;
> +	struct ad4851_state *st = iio_priv(indio_dev);
> +	struct iio_chan_spec *ad4851_channels;
> +	const struct iio_chan_spec ad4851_chan = AD4857_IIO_CHANNEL(0, 0, 0, 16);
> +	const struct iio_chan_spec ad4851_chan_diff = AD4857_IIO_CHANNEL(0, 0, 1, 16);
> +	unsigned int num_channels, index = 0, reg;
> +	int ret;
> +
> +	num_channels = device_get_child_node_count(dev);
> +	if (num_channels > AD4851_MAX_CH_NR)
> +		return dev_err_probe(dev, -EINVAL, "Too many channels: %u\n",
> +				     num_channels);
> +
> +	ad4851_channels = devm_kcalloc(dev, num_channels,
> +				       sizeof(*ad4851_channels), GFP_KERNEL);
> +	if (!ad4851_channels)
> +		return -ENOMEM;
> +
> +	indio_dev->channels = ad4851_channels;
> +	indio_dev->num_channels = num_channels;
> +
> +	device_for_each_child_node_scoped(dev, child) {
> +		ret = fwnode_property_read_u32(child, "reg", &reg);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Missing channel number\n");
> +		if (fwnode_property_present(child, "diff-channels")) {
> +			*ad4851_channels = ad4851_chan_diff;
> +			ad4851_channels->scan_index = index++;
> +			ad4851_channels->channel = reg;
> +			ad4851_channels->channel2 = reg + AD4851_MAX_CH_NR;
> +		} else {
> +			*ad4851_channels = ad4851_chan;
> +			ad4851_channels->scan_index = index++;
> +			ad4851_channels->channel = reg;
> +			ret = regmap_write(st->regmap, AD4851_REG_CHX_SOFTSPAN(reg),
> +					   AD4851_SOFTSPAN_0V_40V);
> +			if (ret)
> +				return ret;
> +		}
> +		ad4851_channels++;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ad4858_parse_channels(struct iio_dev *indio_dev)

> +{
> +	struct device *dev = indio_dev->dev.parent;
> +	struct ad4851_state *st = iio_priv(indio_dev);
> +	struct iio_chan_spec *ad4851_channels;
> +	const struct iio_chan_spec ad4851_chan = AD4858_IIO_CHANNEL(0, 0, 0, 20);
> +	const struct iio_chan_spec ad4851_chan_diff = AD4858_IIO_CHANNEL(0, 0, 1, 20);
> +	unsigned int num_channels, index = 0, reg;
> +	int ret;
> +
> +	num_channels = device_get_child_node_count(dev);
> +	if (num_channels > AD4851_MAX_CH_NR)
> +		return dev_err_probe(dev, -EINVAL, "Too many channels: %u\n",
> +				     num_channels);
> +
> +	ad4851_channels = devm_kcalloc(dev, num_channels,
> +				       sizeof(*ad4851_channels), GFP_KERNEL);
> +	if (!ad4851_channels)
> +		return -ENOMEM;
> +
> +	indio_dev->channels = ad4851_channels;
> +	indio_dev->num_channels = num_channels;
> +
> +	device_for_each_child_node_scoped(dev, child) {
> +		ret = fwnode_property_read_u32(child, "reg", &reg);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Missing channel number\n");
> +		if (fwnode_property_present(child, "diff-channels")) {
> +			*ad4851_channels = ad4851_chan_diff;
> +			ad4851_channels->scan_index = index++;
> +			ad4851_channels->channel = reg;
> +			ad4851_channels->channel2 = reg + AD4851_MAX_CH_NR;
> +			ad4851_channels->ext_scan_type = ad4851_scan_type_20_1;
i think this is already set appropriately in the AD4858_IIO_CHANNEL() macro.
 
> +			ad4851_channels->num_ext_scan_type = ARRAY_SIZE(ad4851_scan_type_20_1);
> +
> +		} else {
> +			*ad4851_channels = ad4851_chan;
> +			ad4851_channels->scan_index = index++;
> +			ad4851_channels->channel = reg;
> +			ad4851_channels->ext_scan_type = ad4851_scan_type_20_0;
> +			ad4851_channels->num_ext_scan_type = ARRAY_SIZE(ad4851_scan_type_20_0);
as above.

With those dealt with there is a huge amount of duplication between this and
 ad4857_parse_channels.  Perhaps factor out a helper function into which
the two iio_chan_spec are passed.

> +			ret = regmap_write(st->regmap, AD4851_REG_CHX_SOFTSPAN(reg),
> +					   AD4851_SOFTSPAN_0V_40V);
> +			if (ret)
> +				return ret;
> +		}
> +		ad4851_channels++;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct ad4851_chip_info ad4851_info = {
> +	.name = "ad4851",
> +	.product_id = 0x67,
> +	.max_sample_rate_hz = 250 * KILO,
> +	.resolution = 16,
> +	.parse_channels = ad4857_parse_channels,
> +};
> +
> +static const struct ad4851_chip_info ad4852_info = {
> +	.name = "ad4852",
> +	.product_id = 0x66,
> +	.max_sample_rate_hz = 250 * KILO,
> +	.resolution = 20,
> +	.parse_channels = ad4858_parse_channels,
> +};
> +
> +static const struct ad4851_chip_info ad4853_info = {
> +	.name = "ad4853",
> +	.product_id = 0x65,
> +	.max_sample_rate_hz = 1 * MEGA,
> +	.resolution = 16,
> +	.parse_channels = ad4857_parse_channels,
> +};
> +
> +static const struct ad4851_chip_info ad4854_info = {
> +	.name = "ad4854",
> +	.product_id = 0x64,
> +	.max_sample_rate_hz = 1 * MEGA,
> +	.resolution = 20,
> +	.parse_channels = ad4858_parse_channels,
> +};
> +
> +static const struct ad4851_chip_info ad4855_info = {
> +	.name = "ad4855",
> +	.product_id = 0x63,
> +	.max_sample_rate_hz = 250 * KILO,
> +	.resolution = 16,
> +	.parse_channels = ad4857_parse_channels,
> +};
> +
> +static const struct ad4851_chip_info ad4856_info = {
> +	.name = "ad4856",
> +	.product_id = 0x62,
> +	.max_sample_rate_hz = 250 * KILO,
> +	.resolution = 20,
> +	.parse_channels = ad4858_parse_channels,
> +};
> +
> +static const struct ad4851_chip_info ad4857_info = {
> +	.name = "ad4857",
> +	.product_id = 0x61,
> +	.max_sample_rate_hz = 1 * MEGA,
> +	.resolution = 16,
> +	.channels = ad4857_channels,
> +	.num_channels = ARRAY_SIZE(ad4857_channels),
> +	.parse_channels = ad4857_parse_channels,
> +};
> +
> +static const struct ad4851_chip_info ad4858_info = {
> +	.name = "ad4858",
> +	.product_id = 0x60,
> +	.max_sample_rate_hz = 1 * MEGA,
> +	.resolution = 20,

A lot of these are not setting all the fields.
If intentional I'd like some comments in here to remind us
why.

> +	.parse_channels = ad4858_parse_channels,
> +};
> +
> +static const struct ad4851_chip_info ad4858i_info = {
> +	.name = "ad4858i",
> +	.product_id = 0x6F,
> +	.max_sample_rate_hz = 1 * MEGA,
> +	.resolution = 20,
> +	.parse_channels = ad4858_parse_channels,
> +};
> +
> +static const struct iio_info ad4851_iio_info = {
> +	.debugfs_reg_access = ad4851_reg_access,
> +	.read_raw = ad4851_read_raw,
> +	.write_raw = ad4851_write_raw,
> +	.update_scan_mode = ad4851_update_scan_mode,
> +	.get_current_scan_type = &ad4851_get_current_scan_type,
> +	.read_avail = ad4851_read_avail,
> +};

> +
> +static int ad4851_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;
> +	struct device *dev = &spi->dev;
> +	struct ad4851_state *st;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->spi = spi;
> +
> +	ret = devm_mutex_init(dev, &st->lock);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_regulator_bulk_get_enable(dev,
> +					     ARRAY_SIZE(ad4851_power_supplies),
> +					     ad4851_power_supplies);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to get and enable supplies\n");
> +
> +	ret = devm_regulator_get_enable_optional(dev, "vddh");
> +	if (ret < 0 && ret != -ENODEV)> +		return dev_err_probe(dev, ret, "failed to enable vddh voltage\n");
> +
> +	ret = devm_regulator_get_enable_optional(dev, "vddl");
> +	if (ret < 0 && ret != -ENODEV)
> +		return dev_err_probe(dev, ret, "failed to enable vddl voltage\n");
> +
> +	st->vrefbuf = devm_regulator_get_optional(dev, "vrefbuf");
> +	if (IS_ERR(st->vrefbuf)) {
> +		if (PTR_ERR(st->vrefbuf) != -ENODEV)
> +			return dev_err_probe(dev, PTR_ERR(st->vrefbuf),
> +					     "Failed to get vrefbuf regulator\n");
> +	}
> +
> +	st->vrefio = devm_regulator_get_optional(dev, "vrefio");
> +	if (IS_ERR(st->vrefio)) {
> +		if (PTR_ERR(st->vrefio) != -ENODEV)
> +			return dev_err_probe(dev, PTR_ERR(st->vrefio),
> +					     "Failed to get vrefio regulator\n");
> +	}
> +
> +	st->pd_gpio = devm_gpiod_get_optional(dev, "pd", GPIOD_OUT_LOW);
> +	if (IS_ERR(st->pd_gpio))
> +		return dev_err_probe(dev, PTR_ERR(st->pd_gpio),
> +				     "Error on requesting pd GPIO\n");
> +
> +	st->cnv = devm_pwm_get(dev, NULL);
> +	if (IS_ERR(st->cnv))
> +		return dev_err_probe(dev, PTR_ERR(st->cnv),
> +				     "Error on requesting pwm\n");
> +
> +	ret = devm_add_action_or_reset(&st->spi->dev, ad4851_pwm_disable,

I think this belongs after ad4841_set_sampling_freq as that includes
enabling the pwm.  A devm cleanup action should be registered immediately
after whatever it is undoing.


> +				       st->cnv);
> +	if (ret)
> +		return ret;
> +
> +	st->info = spi_get_device_match_data(spi);
> +	if (!st->info)
> +		return -ENODEV;
> +
> +	st->regmap = devm_regmap_init_spi(spi, &regmap_config);
> +	if (IS_ERR(st->regmap))
> +		return PTR_ERR(st->regmap);
> +
> +	ret = ad4851_scale_fill(st);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4851_set_sampling_freq(st, HZ_PER_MHZ);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4851_setup(st);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->name = st->info->name;
> +	indio_dev->info = &ad4851_iio_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	ret = st->info->parse_channels(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	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 = ad4851_calibrate(st);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}
Re: [PATCH v7 8/8] iio: adc: ad4851: add ad485x driver
Posted by kernel test robot 3 weeks, 4 days ago
Hi Antoniu,

kernel test robot noticed the following build errors:

[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on linus/master v6.12 next-20241128]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Antoniu-Miclaus/iio-backend-add-API-for-interface-get/20241129-233931
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/20241129153546.63584-9-antoniu.miclaus%40analog.com
patch subject: [PATCH v7 8/8] iio: adc: ad4851: add ad485x driver
config: openrisc-allyesconfig (https://download.01.org/0day-ci/archive/20241130/202411301859.sT9xRNb1-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241130/202411301859.sT9xRNb1-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411301859.sT9xRNb1-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

>> drivers/iio/adc/ad4851.c:963:35: warning: 'ad4858_channels' defined but not used [-Wunused-const-variable=]
     963 | static const struct iio_chan_spec ad4858_channels[] = {
         |                                   ^~~~~~~~~~~~~~~
>> drivers/iio/adc/ad4851.c:889:35: warning: 'ad4851_scan_type_16' defined but not used [-Wunused-const-variable=]
     889 | static const struct iio_scan_type ad4851_scan_type_16 = {
         |                                   ^~~~~~~~~~~~~~~~~~~
--
   arch/openrisc/kernel/head.o: in function `_dispatch_do_ipage_fault':
>> (.text+0x900): relocation truncated to fit: R_OR1K_INSN_REL_26 against `no symbol'
   (.text+0xa00): relocation truncated to fit: R_OR1K_INSN_REL_26 against `no symbol'
   arch/openrisc/kernel/head.o: in function `exit_with_no_dtranslation':
>> (.head.text+0x21bc): relocation truncated to fit: R_OR1K_INSN_REL_26 against `no symbol'
   arch/openrisc/kernel/head.o: in function `exit_with_no_itranslation':
   (.head.text+0x2264): relocation truncated to fit: R_OR1K_INSN_REL_26 against `no symbol'
   init/main.o: in function `trace_event_raw_event_initcall_level':
   main.c:(.text+0x28c): relocation truncated to fit: R_OR1K_INSN_REL_26 against symbol `strlen' defined in .text section in lib/string.o
   init/main.o: in function `initcall_blacklisted':
   main.c:(.text+0x6f4): relocation truncated to fit: R_OR1K_INSN_REL_26 against symbol `strcmp' defined in .text section in lib/string.o
   init/main.o: in function `trace_initcall_finish_cb':
   main.c:(.text+0x814): relocation truncated to fit: R_OR1K_INSN_REL_26 against symbol `__muldi3' defined in .text section in ../lib/gcc/or1k-linux/14.2.0/libgcc.a(_muldi3.o)
   main.c:(.text+0x864): relocation truncated to fit: R_OR1K_INSN_REL_26 against symbol `__muldi3' defined in .text section in ../lib/gcc/or1k-linux/14.2.0/libgcc.a(_muldi3.o)
   main.c:(.text+0x894): relocation truncated to fit: R_OR1K_INSN_REL_26 against symbol `__muldi3' defined in .text section in ../lib/gcc/or1k-linux/14.2.0/libgcc.a(_muldi3.o)
   main.c:(.text+0x8d0): relocation truncated to fit: R_OR1K_INSN_REL_26 against symbol `__muldi3' defined in .text section in ../lib/gcc/or1k-linux/14.2.0/libgcc.a(_muldi3.o)
   main.c:(.text+0x934): additional relocation overflows omitted from the output


vim +/ad4858_channels +963 drivers/iio/adc/ad4851.c

   888	
 > 889	static const struct iio_scan_type ad4851_scan_type_16 = {
   890		.sign = 's',
   891		.realbits = 16,
   892		.storagebits = 16,
   893	};
   894	
   895	static const struct iio_scan_type ad4851_scan_type_20_0[] = {
   896		[AD4851_SCAN_TYPE_NORMAL] = {
   897			.sign = 'u',
   898			.realbits = 20,
   899			.storagebits = 32,
   900		},
   901		[AD4851_SCAN_TYPE_RESOLUTION_BOOST] = {
   902			.sign = 'u',
   903			.realbits = 24,
   904			.storagebits = 32,
   905		},
   906	};
   907	
   908	static const struct iio_scan_type ad4851_scan_type_20_1[] = {
   909		[AD4851_SCAN_TYPE_NORMAL] = {
   910			.sign = 's',
   911			.realbits = 20,
   912			.storagebits = 32,
   913		},
   914		[AD4851_SCAN_TYPE_RESOLUTION_BOOST] = {
   915			.sign = 's',
   916			.realbits = 24,
   917			.storagebits = 32,
   918		},
   919	};
   920	
   921	static int ad4851_get_current_scan_type(const struct iio_dev *indio_dev,
   922						const struct iio_chan_spec *chan)
   923	{
   924		struct ad4851_state *st = iio_priv(indio_dev);
   925	
   926		return st->resolution_boost_enabled ? AD4851_SCAN_TYPE_RESOLUTION_BOOST
   927						    : AD4851_SCAN_TYPE_NORMAL;
   928	}
   929	
   930	#define AD4851_IIO_CHANNEL(index, ch, diff)					\
   931		.type = IIO_VOLTAGE,							\
   932		.info_mask_separate = BIT(IIO_CHAN_INFO_CALIBSCALE) |			\
   933			BIT(IIO_CHAN_INFO_CALIBBIAS) |					\
   934			BIT(IIO_CHAN_INFO_SCALE),					\
   935		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) |		\
   936			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),				\
   937		.info_mask_shared_by_all_available =					\
   938			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),				\
   939		.info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),		\
   940		.indexed = 1,								\
   941		.differential = diff,							\
   942		.channel = ch,								\
   943		.channel2 = ch + (diff * 8),						\
   944		.scan_index = index,							\
   945	
   946	#define AD4858_IIO_CHANNEL(index, ch, diff, bits)				\
   947	{										\
   948		AD4851_IIO_CHANNEL(index, ch, diff)					\
   949		.ext_scan_type = ad4851_scan_type_##bits##_##diff,			\
   950		.num_ext_scan_type = ARRAY_SIZE(ad4851_scan_type_##bits##_##diff),	\
   951	}
   952	
   953	#define AD4857_IIO_CHANNEL(index, ch, diff, bits)				\
   954	{										\
   955		AD4851_IIO_CHANNEL(index, ch, diff)					\
   956		.scan_type = {								\
   957			.sign = 's',							\
   958			.realbits = bits,						\
   959			.storagebits = bits,						\
   960		},									\
   961	}
   962	
 > 963	static const struct iio_chan_spec ad4858_channels[] = {
   964		AD4858_IIO_CHANNEL(0, 0, 0, 20),
   965		AD4858_IIO_CHANNEL(1, 0, 1, 20),
   966		AD4858_IIO_CHANNEL(2, 1, 0, 20),
   967		AD4858_IIO_CHANNEL(3, 1, 1, 20),
   968		AD4858_IIO_CHANNEL(4, 2, 0, 20),
   969		AD4858_IIO_CHANNEL(5, 2, 1, 20),
   970		AD4858_IIO_CHANNEL(6, 3, 0, 20),
   971		AD4858_IIO_CHANNEL(7, 3, 1, 20),
   972		AD4858_IIO_CHANNEL(8, 4, 0, 20),
   973		AD4858_IIO_CHANNEL(9, 4, 1, 20),
   974		AD4858_IIO_CHANNEL(10, 5, 0, 20),
   975		AD4858_IIO_CHANNEL(11, 5, 1, 20),
   976		AD4858_IIO_CHANNEL(12, 6, 0, 20),
   977		AD4858_IIO_CHANNEL(13, 6, 1, 20),
   978		AD4858_IIO_CHANNEL(14, 7, 0, 20),
   979		AD4858_IIO_CHANNEL(15, 7, 1, 20),
   980	};
   981	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki