[PATCH v7 0/8] Add support for AD485x DAS Family

[PATCH v7 0/8] Add support for AD485x DAS Family

[Antoniu Miclaus]

Add support for AD485X fully buffered, 8-channel simultaneous sampling,
16/20-bit, 1 MSPS data acquisition system (DAS) with differential, wide
common-mode range inputs.

Some particularities:
1. softspan - the devices support multiple softspans which are represented in iio
              through offset/scale. The current handling implies changing both
              the scale and the offset separately via IIO, therefore in order to
              properly set the softspan, each time the offset changes the softspan
              is set to the default value. And only after changing also the scale
              the desired softspan is set. This is the approach we are suggesting
              since we need the softspan configurable from userspace and not from
              devicetree.

2. packet format - Data provided on the CMOS and LVDS conversion data output buses
                   are packaged into eight channel packets. This is currently handled
                   as extended info.


Antoniu Miclaus (8):
  iio: backend: add API for interface get
  iio: backend: add support for data size set
  iio: backend: add API for oversampling
  iio: adc: adi-axi-adc: add interface type
  iio: adc: adi-axi-adc: set data format
  iio: adc: adi-axi-adc: add oversampling
  dt-bindings: iio: adc: add ad4851
  iio: adc: ad4851: add ad485x driver

 .../bindings/iio/adc/adi,ad4851.yaml          |  139 ++
 drivers/iio/adc/Kconfig                       |   13 +
 drivers/iio/adc/Makefile                      |    1 +
 drivers/iio/adc/ad4851.c                      | 1346 +++++++++++++++++
 drivers/iio/adc/adi-axi-adc.c                 |   73 +
 drivers/iio/industrialio-backend.c            |   71 +
 include/linux/iio/backend.h                   |   20 +
 7 files changed, 1663 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4851.yaml
 create mode 100644 drivers/iio/adc/ad4851.c

-- 
2.47.1

[PATCH v7 1/8] iio: backend: add API for interface get

[Antoniu Miclaus]

Add backend support for obtaining the interface type used.

Reviewed-by: David Lechner <dlechner@baylibre.com>
Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
no changes in v7.
 drivers/iio/industrialio-backend.c | 24 ++++++++++++++++++++++++
 include/linux/iio/backend.h        | 11 +++++++++++
 2 files changed, 35 insertions(+)

diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c
index 20b3b5212da7..c792cd1e24e8 100644
--- a/drivers/iio/industrialio-backend.c
+++ b/drivers/iio/industrialio-backend.c
@@ -636,6 +636,30 @@ ssize_t iio_backend_ext_info_set(struct iio_dev *indio_dev, uintptr_t private,
 }
 EXPORT_SYMBOL_NS_GPL(iio_backend_ext_info_set, IIO_BACKEND);
 
+/**
+ * iio_backend_interface_type_get - get the interface type used.
+ * @back: Backend device
+ * @type: Interface type
+ *
+ * RETURNS:
+ * 0 on success, negative error number on failure.
+ */
+int iio_backend_interface_type_get(struct iio_backend *back,
+				   enum iio_backend_interface_type *type)
+{
+	int ret;
+
+	ret = iio_backend_op_call(back, interface_type_get, type);
+	if (ret)
+		return ret;
+
+	if (*type >= IIO_BACKEND_INTERFACE_MAX)
+		return -EINVAL;
+
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(iio_backend_interface_type_get, IIO_BACKEND);
+
 /**
  * iio_backend_extend_chan_spec - Extend an IIO channel
  * @back: Backend device
diff --git a/include/linux/iio/backend.h b/include/linux/iio/backend.h
index 37d56914d485..e5ea90f1c3e0 100644
--- a/include/linux/iio/backend.h
+++ b/include/linux/iio/backend.h
@@ -68,6 +68,12 @@ enum iio_backend_sample_trigger {
 	IIO_BACKEND_SAMPLE_TRIGGER_MAX
 };
 
+enum iio_backend_interface_type {
+	IIO_BACKEND_INTERFACE_SERIAL_LVDS,
+	IIO_BACKEND_INTERFACE_SERIAL_CMOS,
+	IIO_BACKEND_INTERFACE_MAX
+};
+
 /**
  * struct iio_backend_ops - operations structure for an iio_backend
  * @enable: Enable backend.
@@ -86,6 +92,7 @@ enum iio_backend_sample_trigger {
  * @extend_chan_spec: Extend an IIO channel.
  * @ext_info_set: Extended info setter.
  * @ext_info_get: Extended info getter.
+ * @interface_type_get: Interface type.
  * @read_raw: Read a channel attribute from a backend device
  * @debugfs_print_chan_status: Print channel status into a buffer.
  * @debugfs_reg_access: Read or write register value of backend.
@@ -121,6 +128,8 @@ struct iio_backend_ops {
 			    const char *buf, size_t len);
 	int (*ext_info_get)(struct iio_backend *back, uintptr_t private,
 			    const struct iio_chan_spec *chan, char *buf);
+	int (*interface_type_get)(struct iio_backend *back,
+				  enum iio_backend_interface_type *type);
 	int (*read_raw)(struct iio_backend *back,
 			struct iio_chan_spec const *chan, int *val, int *val2,
 			long mask);
@@ -169,6 +178,8 @@ ssize_t iio_backend_ext_info_set(struct iio_dev *indio_dev, uintptr_t private,
 				 const char *buf, size_t len);
 ssize_t iio_backend_ext_info_get(struct iio_dev *indio_dev, uintptr_t private,
 				 const struct iio_chan_spec *chan, char *buf);
+int iio_backend_interface_type_get(struct iio_backend *back,
+				   enum iio_backend_interface_type *type);
 int iio_backend_read_raw(struct iio_backend *back,
 			 struct iio_chan_spec const *chan, int *val, int *val2,
 			 long mask);
-- 
2.47.1

[PATCH v7 2/8] iio: backend: add support for data size set

[Antoniu Miclaus]

Add backend support for setting the data size used.
This setting can be adjusted within the IP cores interfacing devices.

Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
no changes in v7.
 drivers/iio/industrialio-backend.c | 21 +++++++++++++++++++++
 include/linux/iio/backend.h        |  3 +++
 2 files changed, 24 insertions(+)

diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c
index c792cd1e24e8..ea184fc2c838 100644
--- a/drivers/iio/industrialio-backend.c
+++ b/drivers/iio/industrialio-backend.c
@@ -660,6 +660,27 @@ int iio_backend_interface_type_get(struct iio_backend *back,
 }
 EXPORT_SYMBOL_NS_GPL(iio_backend_interface_type_get, IIO_BACKEND);
 
+/**
+ * iio_backend_data_size_set - set the data width/size in the data bus.
+ * @back: Backend device
+ * @size: Size in bits
+ *
+ * Some frontend devices can dynamically control the word/data size on the
+ * interface/data bus. Hence, the backend device needs to be aware of it so
+ * data can be correctly transferred.
+ *
+ * Return:
+ * 0 on success, negative error number on failure.
+ */
+int iio_backend_data_size_set(struct iio_backend *back, unsigned int size)
+{
+	if (!size)
+		return -EINVAL;
+
+	return iio_backend_op_call(back, data_size_set, size);
+}
+EXPORT_SYMBOL_NS_GPL(iio_backend_data_size_set, IIO_BACKEND);
+
 /**
  * iio_backend_extend_chan_spec - Extend an IIO channel
  * @back: Backend device
diff --git a/include/linux/iio/backend.h b/include/linux/iio/backend.h
index e5ea90f1c3e0..59b6651b7eaf 100644
--- a/include/linux/iio/backend.h
+++ b/include/linux/iio/backend.h
@@ -93,6 +93,7 @@ enum iio_backend_interface_type {
  * @ext_info_set: Extended info setter.
  * @ext_info_get: Extended info getter.
  * @interface_type_get: Interface type.
+ * @data_size_set: Data size.
  * @read_raw: Read a channel attribute from a backend device
  * @debugfs_print_chan_status: Print channel status into a buffer.
  * @debugfs_reg_access: Read or write register value of backend.
@@ -130,6 +131,7 @@ struct iio_backend_ops {
 			    const struct iio_chan_spec *chan, char *buf);
 	int (*interface_type_get)(struct iio_backend *back,
 				  enum iio_backend_interface_type *type);
+	int (*data_size_set)(struct iio_backend *back, unsigned int size);
 	int (*read_raw)(struct iio_backend *back,
 			struct iio_chan_spec const *chan, int *val, int *val2,
 			long mask);
@@ -180,6 +182,7 @@ ssize_t iio_backend_ext_info_get(struct iio_dev *indio_dev, uintptr_t private,
 				 const struct iio_chan_spec *chan, char *buf);
 int iio_backend_interface_type_get(struct iio_backend *back,
 				   enum iio_backend_interface_type *type);
+int iio_backend_data_size_set(struct iio_backend *back, unsigned int size);
 int iio_backend_read_raw(struct iio_backend *back,
 			 struct iio_chan_spec const *chan, int *val, int *val2,
 			 long mask);
-- 
2.47.1

[PATCH v7 3/8] iio: backend: add API for oversampling

[Antoniu Miclaus]

Add backend support for enabling/disabling oversampling.

Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
changes in v7:
 - implement 2 callbacks
	iio_backend_oversampling_enable()
	iio_backend_oversampling_disable()
 drivers/iio/industrialio-backend.c | 26 ++++++++++++++++++++++++++
 include/linux/iio/backend.h        |  6 ++++++
 2 files changed, 32 insertions(+)

diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c
index ea184fc2c838..86237c1e7ab4 100644
--- a/drivers/iio/industrialio-backend.c
+++ b/drivers/iio/industrialio-backend.c
@@ -681,6 +681,32 @@ int iio_backend_data_size_set(struct iio_backend *back, unsigned int size)
 }
 EXPORT_SYMBOL_NS_GPL(iio_backend_data_size_set, IIO_BACKEND);
 
+/**
+ * iio_backend_oversampling_enable - oversampling enable
+ * @back: Backend device
+ *
+ * Return:
+ * 0 on success, negative error number on failure.
+ */
+int iio_backend_oversampling_enable(struct iio_backend *back)
+{
+	return iio_backend_op_call(back, oversampling_enable);
+}
+EXPORT_SYMBOL_NS_GPL(iio_backend_oversampling_enable, IIO_BACKEND);
+
+/**
+ * iio_backend_oversampling_disable - oversampling disable
+ * @back: Backend device
+ *
+ * Return:
+ * 0 on success, negative error number on failure.
+ */
+int iio_backend_oversampling_disable(struct iio_backend *back)
+{
+	return iio_backend_op_call(back, oversampling_disable);
+}
+EXPORT_SYMBOL_NS_GPL(iio_backend_oversampling_disable, IIO_BACKEND);
+
 /**
  * iio_backend_extend_chan_spec - Extend an IIO channel
  * @back: Backend device
diff --git a/include/linux/iio/backend.h b/include/linux/iio/backend.h
index 59b6651b7eaf..789fa9b586ec 100644
--- a/include/linux/iio/backend.h
+++ b/include/linux/iio/backend.h
@@ -94,6 +94,8 @@ enum iio_backend_interface_type {
  * @ext_info_get: Extended info getter.
  * @interface_type_get: Interface type.
  * @data_size_set: Data size.
+ * @oversampling_enable: Oversampling enable.
+ * @oversampling_disable: Oversampling disable.
  * @read_raw: Read a channel attribute from a backend device
  * @debugfs_print_chan_status: Print channel status into a buffer.
  * @debugfs_reg_access: Read or write register value of backend.
@@ -132,6 +134,8 @@ struct iio_backend_ops {
 	int (*interface_type_get)(struct iio_backend *back,
 				  enum iio_backend_interface_type *type);
 	int (*data_size_set)(struct iio_backend *back, unsigned int size);
+	int (*oversampling_enable)(struct iio_backend *back);
+	int (*oversampling_disable)(struct iio_backend *back);
 	int (*read_raw)(struct iio_backend *back,
 			struct iio_chan_spec const *chan, int *val, int *val2,
 			long mask);
@@ -183,6 +187,8 @@ ssize_t iio_backend_ext_info_get(struct iio_dev *indio_dev, uintptr_t private,
 int iio_backend_interface_type_get(struct iio_backend *back,
 				   enum iio_backend_interface_type *type);
 int iio_backend_data_size_set(struct iio_backend *back, unsigned int size);
+int iio_backend_oversampling_enable(struct iio_backend *back);
+int iio_backend_oversampling_disable(struct iio_backend *back);
 int iio_backend_read_raw(struct iio_backend *back,
 			 struct iio_chan_spec const *chan, int *val, int *val2,
 			 long mask);
-- 
2.47.1

[PATCH v7 4/8] iio: adc: adi-axi-adc: add interface type

[Antoniu Miclaus]

Add support for getting the interface (CMOS or LVDS) used by the AXI ADC
IP.

Reviewed-by: David Lechner <dlechner@baylibre.com>
Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
no changes in v7.
 drivers/iio/adc/adi-axi-adc.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
index 5c8c87eb36d1..f6475bc93796 100644
--- a/drivers/iio/adc/adi-axi-adc.c
+++ b/drivers/iio/adc/adi-axi-adc.c
@@ -39,6 +39,9 @@
 #define   ADI_AXI_REG_RSTN_MMCM_RSTN		BIT(1)
 #define   ADI_AXI_REG_RSTN_RSTN			BIT(0)
 
+#define ADI_AXI_ADC_REG_CONFIG			0x000c
+#define   ADI_AXI_ADC_REG_CONFIG_CMOS_OR_LVDS_N	BIT(7)
+
 #define ADI_AXI_ADC_REG_CTRL			0x0044
 #define    ADI_AXI_ADC_CTRL_DDR_EDGESEL_MASK	BIT(1)
 
@@ -290,6 +293,25 @@ static int axi_adc_chan_disable(struct iio_backend *back, unsigned int chan)
 				 ADI_AXI_REG_CHAN_CTRL_ENABLE);
 }
 
+static int axi_adc_interface_type_get(struct iio_backend *back,
+				      enum iio_backend_interface_type *type)
+{
+	struct adi_axi_adc_state *st = iio_backend_get_priv(back);
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(st->regmap, ADI_AXI_ADC_REG_CONFIG, &val);
+	if (ret)
+		return ret;
+
+	if (val & ADI_AXI_ADC_REG_CONFIG_CMOS_OR_LVDS_N)
+		*type = IIO_BACKEND_INTERFACE_SERIAL_CMOS;
+	else
+		*type = IIO_BACKEND_INTERFACE_SERIAL_LVDS;
+
+	return 0;
+}
+
 static struct iio_buffer *axi_adc_request_buffer(struct iio_backend *back,
 						 struct iio_dev *indio_dev)
 {
@@ -337,6 +359,7 @@ static const struct iio_backend_ops adi_axi_adc_ops = {
 	.iodelay_set = axi_adc_iodelays_set,
 	.test_pattern_set = axi_adc_test_pattern_set,
 	.chan_status = axi_adc_chan_status,
+	.interface_type_get = axi_adc_interface_type_get,
 	.debugfs_reg_access = iio_backend_debugfs_ptr(axi_adc_reg_access),
 	.debugfs_print_chan_status = iio_backend_debugfs_ptr(axi_adc_debugfs_print_chan_status),
 };
-- 
2.47.1

[PATCH v7 5/8] iio: adc: adi-axi-adc: set data format

[Antoniu Miclaus]

Add support for selecting the data format within the AXI ADC ip.

Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
changes in v7:
 - add back 16-bit case
 drivers/iio/adc/adi-axi-adc.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
index f6475bc93796..cb3b8299a65e 100644
--- a/drivers/iio/adc/adi-axi-adc.c
+++ b/drivers/iio/adc/adi-axi-adc.c
@@ -45,6 +45,12 @@
 #define ADI_AXI_ADC_REG_CTRL			0x0044
 #define    ADI_AXI_ADC_CTRL_DDR_EDGESEL_MASK	BIT(1)
 
+#define ADI_AXI_ADC_REG_CNTRL_3			0x004c
+#define   AD485X_CNTRL_3_CUSTOM_CTRL_PACKET_FORMAT_MSK	GENMASK(1, 0)
+#define   AD485X_PACKET_FORMAT_20BIT		0x0
+#define   AD485X_PACKET_FORMAT_24BIT		0x1
+#define   AD485X_PACKET_FORMAT_32BIT		0x2
+
 #define ADI_AXI_ADC_REG_DRP_STATUS		0x0074
 #define   ADI_AXI_ADC_DRP_LOCKED		BIT(17)
 
@@ -312,6 +318,30 @@ static int axi_adc_interface_type_get(struct iio_backend *back,
 	return 0;
 }
 
+static int axi_adc_data_size_set(struct iio_backend *back, unsigned int size)
+{
+	struct adi_axi_adc_state *st = iio_backend_get_priv(back);
+	unsigned int val;
+
+	switch (size) {
+	case 16:
+	case 20:
+		val = AD485X_PACKET_FORMAT_20BIT;
+		break;
+	case 24:
+		val = AD485X_PACKET_FORMAT_24BIT;
+		break;
+	case 32:
+		val = AD485X_PACKET_FORMAT_32BIT;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return regmap_update_bits(st->regmap, ADI_AXI_ADC_REG_CNTRL_3,
+				  AD485X_CNTRL_3_CUSTOM_CTRL_PACKET_FORMAT_MSK, val);
+}
+
 static struct iio_buffer *axi_adc_request_buffer(struct iio_backend *back,
 						 struct iio_dev *indio_dev)
 {
@@ -360,6 +390,7 @@ static const struct iio_backend_ops adi_axi_adc_ops = {
 	.test_pattern_set = axi_adc_test_pattern_set,
 	.chan_status = axi_adc_chan_status,
 	.interface_type_get = axi_adc_interface_type_get,
+	.data_size_set = axi_adc_data_size_set,
 	.debugfs_reg_access = iio_backend_debugfs_ptr(axi_adc_reg_access),
 	.debugfs_print_chan_status = iio_backend_debugfs_ptr(axi_adc_debugfs_print_chan_status),
 };
-- 
2.47.1

[PATCH v7 6/8] iio: adc: adi-axi-adc: add oversampling

[Antoniu Miclaus]

Add support for enabling/disabling oversampling.

Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
 drivers/iio/adc/adi-axi-adc.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
changes in v7:
 - use regmap set/clear bits functions
 - use enable/disable functions
diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
index cb3b8299a65e..3128e2543e22 100644
--- a/drivers/iio/adc/adi-axi-adc.c
+++ b/drivers/iio/adc/adi-axi-adc.c
@@ -46,6 +46,7 @@
 #define    ADI_AXI_ADC_CTRL_DDR_EDGESEL_MASK	BIT(1)
 
 #define ADI_AXI_ADC_REG_CNTRL_3			0x004c
+#define   AD485X_CNTRL_3_CUSTOM_CTRL_OS_EN_MSK	BIT(2)
 #define   AD485X_CNTRL_3_CUSTOM_CTRL_PACKET_FORMAT_MSK	GENMASK(1, 0)
 #define   AD485X_PACKET_FORMAT_20BIT		0x0
 #define   AD485X_PACKET_FORMAT_24BIT		0x1
@@ -342,6 +343,22 @@ static int axi_adc_data_size_set(struct iio_backend *back, unsigned int size)
 				  AD485X_CNTRL_3_CUSTOM_CTRL_PACKET_FORMAT_MSK, val);
 }
 
+static int axi_adc_oversampling_enable(struct iio_backend *back)
+{
+	struct adi_axi_adc_state *st = iio_backend_get_priv(back);
+
+	return regmap_set_bits(st->regmap, ADI_AXI_ADC_REG_CNTRL_3,
+			       AD485X_CNTRL_3_CUSTOM_CTRL_OS_EN_MSK);
+}
+
+static int axi_adc_oversampling_disable(struct iio_backend *back)
+{
+	struct adi_axi_adc_state *st = iio_backend_get_priv(back);
+
+	return regmap_clear_bits(st->regmap, ADI_AXI_ADC_REG_CNTRL_3,
+				 AD485X_CNTRL_3_CUSTOM_CTRL_OS_EN_MSK);
+}
+
 static struct iio_buffer *axi_adc_request_buffer(struct iio_backend *back,
 						 struct iio_dev *indio_dev)
 {
@@ -391,6 +408,8 @@ static const struct iio_backend_ops adi_axi_adc_ops = {
 	.chan_status = axi_adc_chan_status,
 	.interface_type_get = axi_adc_interface_type_get,
 	.data_size_set = axi_adc_data_size_set,
+	.oversampling_enable = axi_adc_oversampling_enable,
+	.oversampling_disable = axi_adc_oversampling_disable,
 	.debugfs_reg_access = iio_backend_debugfs_ptr(axi_adc_reg_access),
 	.debugfs_print_chan_status = iio_backend_debugfs_ptr(axi_adc_debugfs_print_chan_status),
 };
-- 
2.47.1

[PATCH v7 7/8] dt-bindings: iio: adc: add ad4851

[Antoniu Miclaus]

Add devicetree bindings for ad485x family.

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
changes in v7:
 - add adc channels support
 .../bindings/iio/adc/adi,ad4851.yaml          | 139 ++++++++++++++++++
 1 file changed, 139 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4851.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4851.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4851.yaml
new file mode 100644
index 000000000000..57e5222dee71
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4851.yaml
@@ -0,0 +1,139 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright 2024 Analog Devices Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,ad4851.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AD485X family
+
+maintainers:
+  - Sergiu Cuciurean <sergiu.cuciurean@analog.com>
+  - Dragos Bogdan <dragos.bogdan@analog.com>
+  - Antoniu Miclaus <antoniu.miclaus@analog.com>
+
+description: |
+  Analog Devices AD485X fully buffered, 8-channel simultaneous sampling,
+  16/20-bit, 1 MSPS data acquisition system (DAS) with differential, wide
+  common-mode range inputs.
+
+  https://www.analog.com/media/en/technical-documentation/data-sheets/ad4855.pdf
+  https://www.analog.com/media/en/technical-documentation/data-sheets/ad4856.pdf
+  https://www.analog.com/media/en/technical-documentation/data-sheets/ad4857.pdf
+  https://www.analog.com/media/en/technical-documentation/data-sheets/ad4858.pdf
+
+$ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+  compatible:
+    enum:
+      - adi,ad4851
+      - adi,ad4852
+      - adi,ad4853
+      - adi,ad4854
+      - adi,ad4855
+      - adi,ad4856
+      - adi,ad4857
+      - adi,ad4858
+      - adi,ad4858i
+
+  reg:
+    maxItems: 1
+
+  vcc-supply: true
+
+  vee-supply: true
+
+  vdd-supply: true
+
+  vddh-supply: true
+
+  vddl-supply: true
+
+  vio-supply: true
+
+  vrefbuf-supply: true
+
+  vrefio-supply: true
+
+  pwms:
+    description: PWM connected to the CNV pin.
+    maxItems: 1
+
+  io-backends:
+    maxItems: 1
+
+  pd-gpios:
+    maxItems: 1
+
+  spi-max-frequency:
+    maximum: 25000000
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+patternProperties:
+  "^channel(@[0-7])?$":
+    $ref: adc.yaml
+    type: object
+    description: Represents the channels which are connected to the ADC.
+
+    properties:
+      reg:
+        description: The channel number in single-ended mode.
+        minimum: 0
+        maximum: 7
+
+      diff-channels: true
+
+    required:
+      - reg
+
+    additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - vcc-supply
+  - vee-supply
+  - vdd-supply
+  - vio-supply
+  - pwms
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@0{
+            #address-cells = <1>;
+            #size-cells = <0>;
+            compatible = "adi,ad4858";
+            reg = <0>;
+            spi-max-frequency = <10000000>;
+            vcc-supply = <&vcc>;
+            vdd-supply = <&vdd>;
+            vee-supply = <&vee>;
+            vddh-supply = <&vddh>;
+            vddl-supply = <&vddl>;
+            vio-supply = <&vio>;
+            pwms = <&pwm_gen 0 0>;
+            io-backends = <&iio_backend>;
+
+            channel@0 {
+              reg = <0>;
+              diff-channels = <0 8>;
+            };
+
+            channel@1 {
+              reg = <1>;
+            };
+        };
+    };
+...
-- 
2.47.1

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

[Antoniu Miclaus]

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

[kernel test robot]

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

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

[Jonathan Cameron]

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 3/8] iio: backend: add API for oversampling

[David Lechner]

On 11/29/24 9:35 AM, Antoniu Miclaus wrote:
> Add backend support for enabling/disabling oversampling.
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> ---
> changes in v7:
>  - implement 2 callbacks
> 	iio_backend_oversampling_enable()
> 	iio_backend_oversampling_disable()

I think Jonathan's suggestion from a previous review to pass the
oversampling ratio instead of enable/disable seems like a good idea
for making this more generic.

int iio_backend_set_oversampling_ratio(struct iio_backend *back, u32 ratio);

To answer Jonathan's question [1] about why does the backend need to
know if oversampling is enabled or not... In this case, it looks
like it changes some timing (the conversion quiet time) on the LVDS/CMOS
serial data lines depending on if oversampling is enabled or not.

[1]: https://lore.kernel.org/linux-iio/20241123160559.56c57fc7@jic23-huawei/

Re: [PATCH v7 5/8] iio: adc: adi-axi-adc: set data format

[David Lechner]

On 11/29/24 9:35 AM, Antoniu Miclaus wrote:
> Add support for selecting the data format within the AXI ADC ip.
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> ---
> changes in v7:
>  - add back 16-bit case
>  drivers/iio/adc/adi-axi-adc.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
> index f6475bc93796..cb3b8299a65e 100644
> --- a/drivers/iio/adc/adi-axi-adc.c
> +++ b/drivers/iio/adc/adi-axi-adc.c
> @@ -45,6 +45,12 @@
>  #define ADI_AXI_ADC_REG_CTRL			0x0044
>  #define    ADI_AXI_ADC_CTRL_DDR_EDGESEL_MASK	BIT(1)
>  
> +#define ADI_AXI_ADC_REG_CNTRL_3			0x004c
> +#define   AD485X_CNTRL_3_CUSTOM_CTRL_PACKET_FORMAT_MSK	GENMASK(1, 0)
> +#define   AD485X_PACKET_FORMAT_20BIT		0x0
> +#define   AD485X_PACKET_FORMAT_24BIT		0x1
> +#define   AD485X_PACKET_FORMAT_32BIT		0x2
> +
>  #define ADI_AXI_ADC_REG_DRP_STATUS		0x0074
>  #define   ADI_AXI_ADC_DRP_LOCKED		BIT(17)
>  
> @@ -312,6 +318,30 @@ static int axi_adc_interface_type_get(struct iio_backend *back,
>  	return 0;
>  }
>  
> +static int axi_adc_data_size_set(struct iio_backend *back, unsigned int size)
> +{
> +	struct adi_axi_adc_state *st = iio_backend_get_priv(back);
> +	unsigned int val;
> +
> +	switch (size) {

This could use some explanation that there are two different variants of the
AXI AD485X IP block, a 16-bit and a 20-bit variant. So 0x0 (AD485X_PACKET_FORMAT_20BIT)
is really 16-bit on the 16-bit variant of the IP block.

> +	case 16:
> +	case 20:
> +		val = AD485X_PACKET_FORMAT_20BIT;
> +		break;
> +	case 24:
> +		val = AD485X_PACKET_FORMAT_24BIT;
> +		break;
> +	case 32:
> +		val = AD485X_PACKET_FORMAT_32BIT;

AFAICT, technically 0x2 (AD485X_PACKET_FORMAT_32BIT) is not valid on
the 16-bit variant of the IP block, so we should explain why it is
safe to allow this instead of returning error in that case.

Or we could solve both issues by just create two separate functions.

> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return regmap_update_bits(st->regmap, ADI_AXI_ADC_REG_CNTRL_3,
> +				  AD485X_CNTRL_3_CUSTOM_CTRL_PACKET_FORMAT_MSK, val);

To be consistent, would be nice to use FIELD_PREP() with val.

> +}
> +
>  static struct iio_buffer *axi_adc_request_buffer(struct iio_backend *back,
>  						 struct iio_dev *indio_dev)
>  {
> @@ -360,6 +390,7 @@ static const struct iio_backend_ops adi_axi_adc_ops = {
>  	.test_pattern_set = axi_adc_test_pattern_set,
>  	.chan_status = axi_adc_chan_status,
>  	.interface_type_get = axi_adc_interface_type_get,
> +	.data_size_set = axi_adc_data_size_set,

As mentioned before, this callback is specifically for the AXI AD485X version
of the IP block and doesn't apply to the generic base AXI ADC IP block.

[1] and [2] are adding DT compatible and lookup table to handle a different
AXI ADC variant. So we could build on top of that to add the variants for
AXI AD485X. We could add two compatible strings, one for the 16-bit version
and one for the 20-bit version which would allow us to have separate callback
functions as suggested above.

[1]: https://lore.kernel.org/linux-iio/20241121-ad7606_add_iio_backend_software_mode-v1-2-8a693a5e3fa9@baylibre.com/
[2]: https://lore.kernel.org/linux-iio/20241121-ad7606_add_iio_backend_software_mode-v1-6-8a693a5e3fa9@baylibre.com/

>  	.debugfs_reg_access = iio_backend_debugfs_ptr(axi_adc_reg_access),
>  	.debugfs_print_chan_status = iio_backend_debugfs_ptr(axi_adc_debugfs_print_chan_status),
>  };

Re: [PATCH v7 7/8] dt-bindings: iio: adc: add ad4851

[David Lechner]

On 11/29/24 9:35 AM, Antoniu Miclaus wrote:
> Add devicetree bindings for ad485x family.
> 
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> ---
> changes in v7:
>  - add adc channels support

What is the reason for this change? In a previous version of this series,
you explained that we didn't want to specify diff-channels in the DT
because there was a use case to use channels as both single-ended and
differential at runtime. So I am surprised to see this being added now.

> +patternProperties:
> +  "^channel(@[0-7])?$":
> +    $ref: adc.yaml
> +    type: object
> +    description: Represents the channels which are connected to the ADC.
> +
> +    properties:
> +      reg:
> +        description: The channel number in single-ended mode.
> +        minimum: 0
> +        maximum: 7
> +
> +      diff-channels: true
> +
> +    required:
> +      - reg
> +
> +    additionalProperties: false
> +

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

[David Lechner]

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

[Uwe Kleine-König]

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

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

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

RE: [PATCH v7 5/8] iio: adc: adi-axi-adc: set data format

[Miclaus, Antoniu]

--
Antoniu Miclăuş

> -----Original Message-----
> From: David Lechner <dlechner@baylibre.com>
> Sent: Thursday, December 5, 2024 2:46 AM
> To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>; jic23@kernel.org;
> robh@kernel.org; conor+dt@kernel.org; linux-iio@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> pwm@vger.kernel.org
> Subject: Re: [PATCH v7 5/8] iio: adc: adi-axi-adc: set data format
> 
> [External]
> 
> On 11/29/24 9:35 AM, Antoniu Miclaus wrote:
> > Add support for selecting the data format within the AXI ADC ip.
> >
> > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> > ---
> > changes in v7:
> >  - add back 16-bit case
> >  drivers/iio/adc/adi-axi-adc.c | 31 +++++++++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> >
> > diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
> > index f6475bc93796..cb3b8299a65e 100644
> > --- a/drivers/iio/adc/adi-axi-adc.c
> > +++ b/drivers/iio/adc/adi-axi-adc.c
> > @@ -45,6 +45,12 @@
> >  #define ADI_AXI_ADC_REG_CTRL			0x0044
> >  #define    ADI_AXI_ADC_CTRL_DDR_EDGESEL_MASK	BIT(1)
> >
> > +#define ADI_AXI_ADC_REG_CNTRL_3			0x004c
> > +#define   AD485X_CNTRL_3_CUSTOM_CTRL_PACKET_FORMAT_MSK
> 	GENMASK(1, 0)
> > +#define   AD485X_PACKET_FORMAT_20BIT		0x0
> > +#define   AD485X_PACKET_FORMAT_24BIT		0x1
> > +#define   AD485X_PACKET_FORMAT_32BIT		0x2
> > +
> >  #define ADI_AXI_ADC_REG_DRP_STATUS		0x0074
> >  #define   ADI_AXI_ADC_DRP_LOCKED		BIT(17)
> >
> > @@ -312,6 +318,30 @@ static int axi_adc_interface_type_get(struct
> iio_backend *back,
> >  	return 0;
> >  }
> >
> > +static int axi_adc_data_size_set(struct iio_backend *back, unsigned int size)
> > +{
> > +	struct adi_axi_adc_state *st = iio_backend_get_priv(back);
> > +	unsigned int val;
> > +
> > +	switch (size) {
> 
> This could use some explanation that there are two different variants of the
> AXI AD485X IP block, a 16-bit and a 20-bit variant. So 0x0
> (AD485X_PACKET_FORMAT_20BIT)
> is really 16-bit on the 16-bit variant of the IP block.
> 
> > +	case 16:
> > +	case 20:
> > +		val = AD485X_PACKET_FORMAT_20BIT;
> > +		break;
> > +	case 24:
> > +		val = AD485X_PACKET_FORMAT_24BIT;
> > +		break;
> > +	case 32:
> > +		val = AD485X_PACKET_FORMAT_32BIT;
> 
> AFAICT, technically 0x2 (AD485X_PACKET_FORMAT_32BIT) is not valid on
> the 16-bit variant of the IP block, so we should explain why it is
> safe to allow this instead of returning error in that case.
> 
> Or we could solve both issues by just create two separate functions.

I will go for the first suggestion you made: to explain the different variants inline.

> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return regmap_update_bits(st->regmap,
> ADI_AXI_ADC_REG_CNTRL_3,
> > +
> AD485X_CNTRL_3_CUSTOM_CTRL_PACKET_FORMAT_MSK, val);
> 
> To be consistent, would be nice to use FIELD_PREP() with val.
> 
> > +}
> > +
> >  static struct iio_buffer *axi_adc_request_buffer(struct iio_backend *back,
> >  						 struct iio_dev *indio_dev)
> >  {
> > @@ -360,6 +390,7 @@ static const struct iio_backend_ops adi_axi_adc_ops
> = {
> >  	.test_pattern_set = axi_adc_test_pattern_set,
> >  	.chan_status = axi_adc_chan_status,
> >  	.interface_type_get = axi_adc_interface_type_get,
> > +	.data_size_set = axi_adc_data_size_set,
> 
> As mentioned before, this callback is specifically for the AXI AD485X version
> of the IP block and doesn't apply to the generic base AXI ADC IP block.
> 
> [1] and [2] are adding DT compatible and lookup table to handle a different
> AXI ADC variant. So we could build on top of that to add the variants for
> AXI AD485X. We could add two compatible strings, one for the 16-bit version
> and one for the 20-bit version which would allow us to have separate callback
> functions as suggested above.
> 
> [1]: https://urldefense.com/v3/__https://lore.kernel.org/linux-
> iio/20241121-ad7606_add_iio_backend_software_mode-v1-2-
> 8a693a5e3fa9@baylibre.com/__;!!A3Ni8CS0y2Y!40xaV2kwv9GFnh1xLPGPw
> Gd4nFw7n6dzj4Vjo9JTHX1SAGN8A-VlWmgvVyn7Bq-
> WKlJ_dyFbOEqNja_vbqwHyCc$
> [2]: https://urldefense.com/v3/__https://lore.kernel.org/linux-
> iio/20241121-ad7606_add_iio_backend_software_mode-v1-6-
> 8a693a5e3fa9@baylibre.com/__;!!A3Ni8CS0y2Y!40xaV2kwv9GFnh1xLPGPw
> Gd4nFw7n6dzj4Vjo9JTHX1SAGN8A-VlWmgvVyn7Bq-
> WKlJ_dyFbOEqNja_vidP9fsQ$
> 
> >  	.debugfs_reg_access = iio_backend_debugfs_ptr(axi_adc_reg_access),
> >  	.debugfs_print_chan_status =
> iio_backend_debugfs_ptr(axi_adc_debugfs_print_chan_status),
> >  };

RE: [PATCH v7 7/8] dt-bindings: iio: adc: add ad4851

[Miclaus, Antoniu]

--
Antoniu Miclăuş

> -----Original Message-----
> From: David Lechner <dlechner@baylibre.com>
> Sent: Thursday, December 5, 2024 2:46 AM
> To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>; jic23@kernel.org;
> robh@kernel.org; conor+dt@kernel.org; linux-iio@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> pwm@vger.kernel.org
> Cc: Conor Dooley <conor.dooley@microchip.com>
> Subject: Re: [PATCH v7 7/8] dt-bindings: iio: adc: add ad4851
> 
> [External]
> 
> On 11/29/24 9:35 AM, Antoniu Miclaus wrote:
> > Add devicetree bindings for ad485x family.
> >
> > Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> > ---
> > changes in v7:
> >  - add adc channels support
> 
> What is the reason for this change? In a previous version of this series,
> you explained that we didn't want to specify diff-channels in the DT
> because there was a use case to use channels as both single-ended and
> differential at runtime. So I am surprised to see this being added now.
> 
We had a discussion and we decided to go for the dt approach for specifying
the channels configuration, even though in the first place we wanted to avoid this.
Overall it makes more sense.

> > +patternProperties:
> > +  "^channel(@[0-7])?$":
> > +    $ref: adc.yaml
> > +    type: object
> > +    description: Represents the channels which are connected to the ADC.
> > +
> > +    properties:
> > +      reg:
> > +        description: The channel number in single-ended mode.
> > +        minimum: 0
> > +        maximum: 7
> > +
> > +      diff-channels: true
> > +
> > +    required:
> > +      - reg
> > +
> > +    additionalProperties: false
> > +

Re: [PATCH v7 7/8] dt-bindings: iio: adc: add ad4851

[David Lechner]

On 12/9/24 8:02 AM, Miclaus, Antoniu wrote:
> 
> 
> --
> Antoniu Miclăuş
> 
>> -----Original Message-----
>> From: David Lechner <dlechner@baylibre.com>
>> Sent: Thursday, December 5, 2024 2:46 AM
>> To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>; jic23@kernel.org;
>> robh@kernel.org; conor+dt@kernel.org; linux-iio@vger.kernel.org;
>> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
>> pwm@vger.kernel.org
>> Cc: Conor Dooley <conor.dooley@microchip.com>
>> Subject: Re: [PATCH v7 7/8] dt-bindings: iio: adc: add ad4851
>>
>> [External]
>>
>> On 11/29/24 9:35 AM, Antoniu Miclaus wrote:
>>> Add devicetree bindings for ad485x family.
>>>
>>> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
>>> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
>>> ---
>>> changes in v7:
>>>  - add adc channels support
>>
>> What is the reason for this change? In a previous version of this series,
>> you explained that we didn't want to specify diff-channels in the DT
>> because there was a use case to use channels as both single-ended and
>> differential at runtime. So I am surprised to see this being added now.
>>
> We had a discussion and we decided to go for the dt approach for specifying
> the channels configuration, even though in the first place we wanted to avoid this.
> Overall it makes more sense.

OK, in that case we will also want to make use of the standard "bipolar"
property from adc.yaml as well since the chip differentiates between
unipolar and bipolar inputs.

Also, might want to drop Conor's review tag and give an explanation in
the next revision since adding these channel properties is a bit of
a big change compared to the version he reviewed.

> 
>>> +patternProperties:
>>> +  "^channel(@[0-7])?$":
>>> +    $ref: adc.yaml
>>> +    type: object
>>> +    description: Represents the channels which are connected to the ADC.
>>> +
>>> +    properties:
>>> +      reg:
>>> +        description: The channel number in single-ended mode.
>>> +        minimum: 0
>>> +        maximum: 7
>>> +
>>> +      diff-channels: true
>>> +
>>> +    required:
>>> +      - reg
>>> +
>>> +    additionalProperties: false
>>> +