[PATCH v3 1/6] iio: backend: add API for interface get

[PATCH v3 1/6] iio: backend: add API for interface get

[Antoniu Miclaus]

Add backend support for obtaining the interface type used.

Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
changes in v3:
 - fix spelling
 - add serial cmos/lvds in interface type enum
 - drop comma for last element in enum.
 drivers/iio/industrialio-backend.c | 24 ++++++++++++++++++++++++
 include/linux/iio/backend.h        | 13 +++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c
index efe05be284b6..ef1fd4cb0b24 100644
--- a/drivers/iio/industrialio-backend.c
+++ b/drivers/iio/industrialio-backend.c
@@ -449,6 +449,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
  * @indio_dev: IIO device
diff --git a/include/linux/iio/backend.h b/include/linux/iio/backend.h
index 8099759d7242..ad9fa0ada9b2 100644
--- a/include/linux/iio/backend.h
+++ b/include/linux/iio/backend.h
@@ -63,6 +63,14 @@ enum iio_backend_sample_trigger {
 	IIO_BACKEND_SAMPLE_TRIGGER_MAX
 };
 
+enum iio_backend_interface_type {
+	IIO_BACKEND_INTERFACE_LVDS,
+	IIO_BACKEND_INTERFACE_CMOS,
+	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.
@@ -81,6 +89,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.
  **/
 struct iio_backend_ops {
 	int (*enable)(struct iio_backend *back);
@@ -113,6 +122,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 iio_backend_chan_enable(struct iio_backend *back, unsigned int chan);
@@ -142,6 +153,8 @@ ssize_t iio_backend_ext_info_set(struct iio_dev *indio_dev, uintptr_t private,
 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_extend_chan_spec(struct iio_dev *indio_dev,
 				 struct iio_backend *back,
 				 struct iio_chan_spec *chan);
-- 
2.46.2

[PATCH v3 2/6] 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 v3.
 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 ef1fd4cb0b24..2cb97c294ba7 100644
--- a/drivers/iio/industrialio-backend.c
+++ b/drivers/iio/industrialio-backend.c
@@ -473,6 +473,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.
+ */
+size_t iio_backend_data_size_set(struct iio_backend *back, size_t 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
  * @indio_dev: IIO device
diff --git a/include/linux/iio/backend.h b/include/linux/iio/backend.h
index ad9fa0ada9b2..c2ffb61c5a92 100644
--- a/include/linux/iio/backend.h
+++ b/include/linux/iio/backend.h
@@ -90,6 +90,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.
  **/
 struct iio_backend_ops {
 	int (*enable)(struct iio_backend *back);
@@ -124,6 +125,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, ssize_t size);
 };
 
 int iio_backend_chan_enable(struct iio_backend *back, unsigned int chan);
@@ -155,6 +157,7 @@ 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);
+size_t iio_backend_data_size_set(struct iio_backend *back, size_t size);
 int iio_backend_extend_chan_spec(struct iio_dev *indio_dev,
 				 struct iio_backend *back,
 				 struct iio_chan_spec *chan);
-- 
2.46.2

[PATCH v3 3/6] 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.

Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
no changes in v3.
 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 21ce7564e83d..ff48f26e02a3 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)
 
@@ -249,6 +252,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_CMOS;
+	else
+		*type = IIO_BACKEND_INTERFACE_LVDS;
+
+	return 0;
+}
+
 static struct iio_buffer *axi_adc_request_buffer(struct iio_backend *back,
 						 struct iio_dev *indio_dev)
 {
@@ -285,6 +307,7 @@ static const struct iio_backend_ops adi_axi_adc_generic = {
 	.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,
 };
 
 static int adi_axi_adc_probe(struct platform_device *pdev)
-- 
2.46.2

Re: [PATCH v3 3/6] iio: adc: adi-axi-adc: add interface type

[Andy Shevchenko]

On Mon, Oct 14, 2024 at 12:40:37PM +0300, Antoniu Miclaus wrote:
> Add support for getting the interface (CMOS or LVDS) used by the AXI ADC
> ip.

IP

...

>  #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)

Side note, the field definitions are indented with different amount of white
spaces.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 4/6] 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>
---
no changes in v3.
 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 ff48f26e02a3..363cc29b4c18 100644
--- a/drivers/iio/adc/adi-axi-adc.c
+++ b/drivers/iio/adc/adi-axi-adc.c
@@ -45,6 +45,9 @@
 #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 ADI_AXI_ADC_CNTRL_3_CUSTOM_CONTROL_MSK	GENMASK(7, 0)
+
 #define ADI_AXI_ADC_REG_DRP_STATUS		0x0074
 #define   ADI_AXI_ADC_DRP_LOCKED		BIT(17)
 
@@ -271,6 +274,25 @@ static int axi_adc_interface_type_get(struct iio_backend *back,
 	return 0;
 }
 
+static int axi_adc_data_size_set(struct iio_backend *back,
+				 ssize_t size)
+{
+	struct adi_axi_adc_state *st = iio_backend_get_priv(back);
+	unsigned int val;
+
+	if (size <= 20)
+		val = 0;
+	else if (size <= 24)
+		val = 1;
+	else if (size <= 32)
+		val = 3;
+	else
+		return -EINVAL;
+
+	return regmap_update_bits(st->regmap, ADI_AXI_ADC_REG_CNTRL_3,
+				  ADI_AXI_ADC_CNTRL_3_CUSTOM_CONTROL_MSK, val);
+}
+
 static struct iio_buffer *axi_adc_request_buffer(struct iio_backend *back,
 						 struct iio_dev *indio_dev)
 {
@@ -308,6 +330,7 @@ static const struct iio_backend_ops adi_axi_adc_generic = {
 	.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,
 };
 
 static int adi_axi_adc_probe(struct platform_device *pdev)
-- 
2.46.2

Re: [PATCH v3 4/6] iio: adc: adi-axi-adc: set data format

[Andy Shevchenko]

On Mon, Oct 14, 2024 at 12:40:38PM +0300, Antoniu Miclaus wrote:
> Add support for selecting the data format within the AXI ADC ip.

IP

...

>  #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 ADI_AXI_ADC_CNTRL_3_CUSTOM_CONTROL_MSK	GENMASK(7, 0)

And here is no (additional) indentation at all and no proper prefix...
Can you be consistent (to some extent) with the existing code? Or
update it to be consistent all over the places.

>  #define ADI_AXI_ADC_REG_DRP_STATUS		0x0074
>  #define   ADI_AXI_ADC_DRP_LOCKED		BIT(17)

...

> +static int axi_adc_data_size_set(struct iio_backend *back,
> +				 ssize_t size)

This is perfectly one line. We wrap at 80. I believe your text editor
configuration has to be adjusted for this project.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 5/6] 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 v3:
 - add filename matching a compatible string.
 - rename bindings file to match a device name.
 - add support for refio and refbuf supplies.
 - add description for the devices supported.
 - use numeric order for compatibles and datasheet links.
 .../bindings/iio/adc/adi,ad4851.yaml          | 103 ++++++++++++++++++
 1 file changed, 103 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..5f0f2e6e7a8e
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4851.yaml
@@ -0,0 +1,103 @@
+# 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
+
+required:
+  - compatible
+  - reg
+  - vcc-supply
+  - vee-supply
+  - vdd-supply
+  - vio-supply
+  - pwms
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@0{
+            compatible = "adi,ad4858";
+            reg = <0>;
+            spi-max-frequency = <10000000>;
+            vcc-supply = <&vcc>;
+            vdd-supply = <&vdd>;
+            vee-supply = <&vee>;
+            vddh-supply = <&vddh>;
+            vddl-supply = <&vddh>;
+            vio-supply = <&vio>;
+            pwms = <&pwm_gen 0 0>;
+            io-backends = <&iio_backend>;
+        };
+    };
+...
-- 
2.46.2

[PATCH v3 6/6] 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>
---
 - rename ad485x occurences with ad4851
 - list all supported parts in Kconfig
 - drop product id macro definitions and use values inline.
 - drop scale/offset from device state and encode them directly.
 - use switch..case for realbits if..else statement.
 - replace {} -> { }
 - drop scale table from chip info and use it inline.
 - use alphanumeric order for spi_id table and of_match table.
 - improve commit description.
 - use bitmap instead of bool matrix.
 - use pow-of-2 alignment for scale_avail
 - use MICRO/MEGA where missing.
 - include bitfield.h
 - increse lower limit of clamp() to 1.
 - add error message for devm_pwm_get().
 - implement oversampling ratio and adjust the packet_format based on it.
 drivers/iio/adc/Kconfig  |   13 +
 drivers/iio/adc/Makefile |    1 +
 drivers/iio/adc/ad4851.c | 1113 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 1127 insertions(+)
 create mode 100644 drivers/iio/adc/ad4851.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index f60fe85a30d5..93e53794ce89 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -36,6 +36,19 @@ config AD4130
 	  To compile this driver as a module, choose M here: the module will be
 	  called ad4130.
 
+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 d370e066544e..1a873bf1a917 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -7,6 +7,7 @@
 obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o
 obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
 obj-$(CONFIG_AD4130) += ad4130.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..99c9367de383
--- /dev/null
+++ b/drivers/iio/adc/ad4851.c
@@ -0,0 +1,1113 @@
+// 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/units.h>
+
+#include <linux/iio/backend.h>
+#include <linux/iio/iio.h>
+
+#include <asm/unaligned.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_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
+
+struct ad4851_chip_info {
+	const char *name;
+	unsigned int product_id;
+	const unsigned int (*scale_table)[2];
+	int num_scales;
+	const int *offset_table;
+	int num_offset;
+	const struct iio_chan_spec *channels;
+	unsigned int num_channels;
+	unsigned long throughput;
+	unsigned int resolution;
+};
+
+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;
+	const struct ad4851_chip_info *info;
+	struct gpio_desc *pd_gpio;
+	unsigned long sampling_freq;
+	unsigned int (*scales)[2];
+	int *offsets;
+};
+
+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);
+
+	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->throughput);
+
+	cnv_state.period = DIV_ROUND_CLOSEST_ULL(GIGA, 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_update_bits(st->regmap, AD4851_REG_OVERSAMPLE,
+					 AD4851_OS_EN_MSK, 0);
+		if (ret)
+			return ret;
+	} else {
+		ret = regmap_update_bits(st->regmap, AD4851_REG_OVERSAMPLE,
+					 AD4851_OS_EN_MSK, AD4851_OS_EN_MSK);
+		if (ret)
+			return ret;
+
+		val = ad4851_osr_to_regval(osr);
+		if (val < 0)
+			return -EINVAL;
+
+		ret = regmap_update_bits(st->regmap, AD4851_REG_OVERSAMPLE,
+					 AD4851_OS_RATIO_MSK, val);
+		if (ret)
+			return ret;
+	}
+
+	switch (chan->scan_type.realbits) {
+	case 20:
+		switch (osr) {
+		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;
+
+	return regmap_update_bits(st->regmap, AD4851_REG_PACKET,
+				  AD4851_PACKET_FORMAT_MASK, (osr == 1) ? 0 : 1);
+}
+
+static int ad4851_get_oversampling_ratio(struct ad4851_state *st, unsigned int *val)
+{
+	unsigned int osr;
+	int ret;
+
+	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 int ad4851_setup(struct ad4851_state *st)
+{
+	unsigned int product_id;
+	int ret;
+
+	ret = ad4851_set_sampling_freq(st, HZ_PER_MHZ);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(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_write(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;
+		}
+	}
+
+	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, lane_num, 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_CMOS:
+		lane_num = st->info->num_channels;
+		break;
+	case IIO_BACKEND_INTERFACE_LVDS:
+		lane_num = 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; 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 < lane_num; 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 < lane_num; 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; 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 & 0xFF) << 8;
+
+	ret = regmap_read(st->regmap, AD4851_REG_CHX_GAIN_LSB(ch),
+			  &reg_val);
+	if (ret)
+		return ret;
+
+	gain |= reg_val & 0xFF;
+
+	*val = gain;
+	*val2 = 32768;
+
+	return IIO_VAL_FRACTIONAL;
+}
+
+static int ad4851_set_calibscale(struct ad4851_state *st, int ch, int val,
+				 int val2)
+{
+	unsigned long long gain;
+	u8 buf[0];
+	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 const unsigned int ad4851_scale_table[][2] = {
+	{ 2500, 0x0 },
+	{ 5000, 0x1 },
+	{ 5000, 0x2 },
+	{ 10000, 0x3 },
+	{ 6250, 0x04 },
+	{ 12500, 0x5 },
+	{ 10000, 0x6 },
+	{ 20000, 0x7 },
+	{ 12500, 0x8 },
+	{ 25000, 0x9 },
+	{ 20000, 0xA },
+	{ 40000, 0xB },
+	{ 25000, 0xC },
+	{ 50000, 0xD },
+	{ 40000, 0xE },
+	{ 80000, 0xF },
+};
+
+static const int ad4857_offset_table[] = {
+	0, -32768,
+};
+
+static const int ad4858_offset_table[] = {
+	0, -524288,
+};
+
+static const unsigned int ad4851_scale_avail[] = {
+	2500, 5000,
+	10000, 6250,
+	12500, 20000,
+	25000, 40000,
+	50000, 80000,
+};
+
+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;
+	bool single_ended = false;
+
+	for (i = 0; i < ARRAY_SIZE(ad4851_scale_table); i++) {
+		__ad4851_get_scale(st, ad4851_scale_table[i][0],
+				   &scale_val[0], &scale_val[1]);
+		if (scale_val[0] != val || scale_val[1] != val2)
+			continue;
+
+		/*
+		 * Adjust the softspan value (differential or single ended)
+		 * based on the scale value selected and current offset of
+		 * the channel.
+		 *
+		 * If the offset is 0 then continue iterations until finding
+		 * the next matching scale value which always corresponds to
+		 * the single ended mode.
+		 */
+		if (!st->offsets[chan->channel] && !single_ended) {
+			single_ended = true;
+			continue;
+		}
+
+		return regmap_write(st->regmap,
+				    AD4851_REG_CHX_SOFTSPAN(chan->channel),
+				    ad4851_scale_table[i][1]);
+	}
+
+	return -EINVAL;
+}
+
+static int ad4851_get_scale(struct ad4851_state *st,
+			    const struct iio_chan_spec *chan, int *val,
+			    int *val2)
+{
+	unsigned int i, softspan_val;
+	int ret;
+
+	ret = regmap_read(st->regmap, AD4851_REG_CHX_SOFTSPAN(chan->channel),
+			  &softspan_val);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < ARRAY_SIZE(ad4851_scale_table); i++) {
+		if (softspan_val == ad4851_scale_table[i][1])
+			break;
+	}
+
+	if (i == ARRAY_SIZE(ad4851_scale_table))
+		return -EIO;
+
+	__ad4851_get_scale(st, ad4851_scale_table[i][0], val, val2);
+
+	return IIO_VAL_INT_PLUS_MICRO;
+}
+
+static int ad4851_set_offset(struct ad4851_state *st,
+			     const struct iio_chan_spec *chan, int val)
+{
+	guard(mutex)(&st->lock);
+
+	if (val != st->offsets[chan->channel])
+		return 0;
+
+	st->offsets[chan->channel] = val;
+	/* Restore to the default range if offset changes */
+	if (st->offsets[chan->channel])
+		return regmap_write(st->regmap,
+					AD4851_REG_CHX_SOFTSPAN(chan->channel),
+					AD4851_SOFTSPAN_N40V_40V);
+	return regmap_write(st->regmap,
+				AD4851_REG_CHX_SOFTSPAN(chan->channel),
+				AD4851_SOFTSPAN_0V_40V);
+}
+
+static int ad4851_scale_offset_fill(struct ad4851_state *st)
+{
+	unsigned int i, val1, val2;
+
+	st->offsets = devm_kcalloc(&st->spi->dev, st->info->num_channels,
+				   sizeof(*st->offsets), GFP_KERNEL);
+	if (!st->offsets)
+		return -ENOMEM;
+
+	st->scales = devm_kmalloc_array(&st->spi->dev, ARRAY_SIZE(ad4851_scale_avail),
+					sizeof(*st->scales), GFP_KERNEL);
+	if (!st->scales)
+		return -ENOMEM;
+
+	for (i = 0; i < ARRAY_SIZE(ad4851_scale_avail); i++) {
+		__ad4851_get_scale(st, ad4851_scale_avail[i], &val1, &val2);
+		st->scales[i][0] = val1;
+		st->scales[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_OFFSET:
+		scoped_guard(mutex, &st->lock)
+			*val = st->offsets[chan->channel];
+		return IIO_VAL_INT;
+	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_OFFSET:
+		return ad4851_set_offset(st, chan, 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; 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:
+		*vals = (const int *)st->scales;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		/* Values are stored in a 2D matrix */
+		*length = ARRAY_SIZE(ad4851_scale_avail) * 2;
+		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_OFFSET:
+		*vals = st->info->offset_table;
+		*type = IIO_VAL_INT;
+		*length = st->info->num_offset;
+		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;
+	}
+}
+
+#define AD4851_IIO_CHANNEL(index, real, storage)			\
+{									\
+	.type = IIO_VOLTAGE,						\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_CALIBSCALE) |		\
+		BIT(IIO_CHAN_INFO_CALIBBIAS) |				\
+		BIT(IIO_CHAN_INFO_SCALE) |				\
+		BIT(IIO_CHAN_INFO_OFFSET),				\
+	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) |	\
+		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),			\
+	.info_mask_shared_by_type_available =				\
+		BIT(IIO_CHAN_INFO_SCALE) |				\
+		BIT(IIO_CHAN_INFO_OFFSET) |				\
+		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),			\
+	.indexed = 1,							\
+	.channel = index,						\
+	.scan_index = index,						\
+	.scan_type = {							\
+		.sign = 's',						\
+		.realbits = real,					\
+		.storagebits = storage,					\
+	},								\
+}
+
+static const struct iio_chan_spec ad4858_channels[] = {
+	AD4851_IIO_CHANNEL(0, 20, 32),
+	AD4851_IIO_CHANNEL(1, 20, 32),
+	AD4851_IIO_CHANNEL(2, 20, 32),
+	AD4851_IIO_CHANNEL(3, 20, 32),
+	AD4851_IIO_CHANNEL(4, 20, 32),
+	AD4851_IIO_CHANNEL(5, 20, 32),
+	AD4851_IIO_CHANNEL(6, 20, 32),
+	AD4851_IIO_CHANNEL(7, 20, 32),
+};
+
+static const struct iio_chan_spec ad4857_channels[] = {
+	AD4851_IIO_CHANNEL(0, 16, 16),
+	AD4851_IIO_CHANNEL(1, 16, 16),
+	AD4851_IIO_CHANNEL(2, 16, 16),
+	AD4851_IIO_CHANNEL(3, 16, 16),
+	AD4851_IIO_CHANNEL(4, 16, 16),
+	AD4851_IIO_CHANNEL(5, 16, 16),
+	AD4851_IIO_CHANNEL(6, 16, 16),
+	AD4851_IIO_CHANNEL(7, 16, 16),
+};
+
+static const struct ad4851_chip_info ad4851_info = {
+	.name = "ad4851",
+	.product_id = 0x67,
+	.offset_table = ad4857_offset_table,
+	.num_offset = ARRAY_SIZE(ad4857_offset_table),
+	.channels = ad4857_channels,
+	.num_channels = ARRAY_SIZE(ad4857_channels),
+	.throughput = 250 * KILO,
+	.resolution = 16,
+};
+
+static const struct ad4851_chip_info ad4852_info = {
+	.name = "ad4852",
+	.product_id = 0x66,
+	.offset_table = ad4858_offset_table,
+	.num_offset = ARRAY_SIZE(ad4858_offset_table),
+	.channels = ad4858_channels,
+	.num_channels = ARRAY_SIZE(ad4858_channels),
+	.throughput = 250 * KILO,
+	.resolution = 20,
+};
+
+static const struct ad4851_chip_info ad4853_info = {
+	.name = "ad4853",
+	.product_id = 0x65,
+	.offset_table = ad4857_offset_table,
+	.num_offset = ARRAY_SIZE(ad4857_offset_table),
+	.channels = ad4857_channels,
+	.num_channels = ARRAY_SIZE(ad4857_channels),
+	.throughput = 1 * MEGA,
+	.resolution = 16,
+};
+
+static const struct ad4851_chip_info ad4854_info = {
+	.name = "ad4854",
+	.product_id = 0x64,
+	.offset_table = ad4858_offset_table,
+	.num_offset = ARRAY_SIZE(ad4858_offset_table),
+	.channels = ad4858_channels,
+	.num_channels = ARRAY_SIZE(ad4858_channels),
+	.throughput = 1 * MEGA,
+	.resolution = 20,
+};
+
+static const struct ad4851_chip_info ad4855_info = {
+	.name = "ad4855",
+	.product_id = 0x63,
+	.offset_table = ad4857_offset_table,
+	.num_offset = ARRAY_SIZE(ad4857_offset_table),
+	.channels = ad4857_channels,
+	.num_channels = ARRAY_SIZE(ad4857_channels),
+	.throughput = 250 * KILO,
+	.resolution = 16,
+};
+
+static const struct ad4851_chip_info ad4856_info = {
+	.name = "ad4856",
+	.product_id = 0x62,
+	.offset_table = ad4858_offset_table,
+	.num_offset = ARRAY_SIZE(ad4858_offset_table),
+	.channels = ad4858_channels,
+	.num_channels = ARRAY_SIZE(ad4858_channels),
+	.throughput = 250 * KILO,
+	.resolution = 20,
+};
+
+static const struct ad4851_chip_info ad4857_info = {
+	.name = "ad4857",
+	.product_id = 0x61,
+	.offset_table = ad4857_offset_table,
+	.num_offset = ARRAY_SIZE(ad4857_offset_table),
+	.channels = ad4857_channels,
+	.num_channels = ARRAY_SIZE(ad4857_channels),
+	.throughput = 1 * MEGA,
+	.resolution = 16,
+};
+
+static const struct ad4851_chip_info ad4858_info = {
+	.name = "ad4858",
+	.product_id = 0x60,
+	.offset_table = ad4858_offset_table,
+	.num_offset = ARRAY_SIZE(ad4858_offset_table),
+	.channels = ad4858_channels,
+	.num_channels = ARRAY_SIZE(ad4858_channels),
+	.throughput = 1 * MEGA,
+	.resolution = 20,
+};
+
+static const struct ad4851_chip_info ad4858i_info = {
+	.name = "ad4858i",
+	.product_id = 0x6F,
+	.offset_table = ad4858_offset_table,
+	.num_offset = ARRAY_SIZE(ad4858_offset_table),
+	.channels = ad4858_channels,
+	.num_channels = ARRAY_SIZE(ad4858_channels),
+	.throughput = 1 * MEGA,
+	.resolution = 20,
+};
+
+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,
+	.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 ad4851_state *st;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+	st->spi = spi;
+
+	ret = devm_mutex_init(&spi->dev, &st->lock);
+	if (ret)
+		return ret;
+
+	ret = devm_regulator_bulk_get_enable(&spi->dev,
+					     ARRAY_SIZE(ad4851_power_supplies),
+					     ad4851_power_supplies);
+	if (ret)
+		return dev_err_probe(&spi->dev, ret,
+				     "failed to get and enable supplies\n");
+
+	ret = devm_regulator_get_enable_optional(&spi->dev, "vddh");
+	if (ret < 0 && ret != -ENODEV)
+		return dev_err_probe(&spi->dev, ret, "failed to get vddh voltage\n");
+
+	ret = devm_regulator_get_enable_optional(&spi->dev, "vddl");
+	if (ret < 0 && ret != -ENODEV)
+		return dev_err_probe(&spi->dev, ret, "failed to get vddl voltage\n");
+
+	ret = devm_regulator_get_enable_optional(&spi->dev, "vrefbuf");
+	if (ret < 0 && ret != -ENODEV)
+		return dev_err_probe(&spi->dev, ret, "failed to get vrefbuf voltage\n");
+
+	ret = devm_regulator_get_enable_optional(&spi->dev, "vddl");
+	if (ret < 0 && ret != -ENODEV)
+		return dev_err_probe(&spi->dev, ret, "failed to get vrefio voltage\n");
+
+	st->pd_gpio = devm_gpiod_get_optional(&spi->dev, "pd", GPIOD_OUT_LOW);
+	if (IS_ERR(st->pd_gpio))
+		return dev_err_probe(&spi->dev, PTR_ERR(st->pd_gpio),
+				     "Error on requesting pd GPIO\n");
+
+	st->cnv = devm_pwm_get(&spi->dev, NULL);
+	if (IS_ERR(st->cnv))
+		return dev_err_probe(&spi->dev, PTR_ERR(st->cnv),
+				     "Error on requesting pwm\n");
+
+	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_offset_fill(st);
+	if (ret)
+		return ret;
+
+	ret = ad4851_setup(st);
+	if (ret)
+		return ret;
+
+	indio_dev->name = st->info->name;
+	indio_dev->channels = st->info->channels;
+	indio_dev->num_channels = st->info->num_channels;
+	indio_dev->info = &ad4851_iio_info;
+
+	st->back = devm_iio_backend_get(&spi->dev, NULL);
+	if (IS_ERR(st->back))
+		return PTR_ERR(st->back);
+
+	ret = devm_iio_backend_request_buffer(&spi->dev, st->back, indio_dev);
+	if (ret)
+		return ret;
+
+	ret = devm_iio_backend_enable(&spi->dev, st->back);
+	if (ret)
+		return ret;
+
+	ret = ad4851_calibrate(st);
+	if (ret)
+		return ret;
+
+	return devm_iio_device_register(&spi->dev, indio_dev);
+}
+
+static const struct of_device_id ad4851_of_match[] = {
+	{ .compatible = "adi,ad4858", .data = &ad4851_info, },
+	{ .compatible = "adi,ad4857", .data = &ad4852_info, },
+	{ .compatible = "adi,ad4856", .data = &ad4853_info, },
+	{ .compatible = "adi,ad4855", .data = &ad4854_info, },
+	{ .compatible = "adi,ad4854", .data = &ad4855_info, },
+	{ .compatible = "adi,ad4853", .data = &ad4856_info, },
+	{ .compatible = "adi,ad4852", .data = &ad4857_info, },
+	{ .compatible = "adi,ad4851", .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.46.2

Re: [PATCH v3 6/6] iio: adc: ad4851: add ad485x driver

[Andy Shevchenko]

On Mon, Oct 14, 2024 at 12:40:40PM +0300, 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.

...

> +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).

I think I already commented on this... Anyway, it's much better to support when
this list is broke down on per device per line. In such a case it's less churn
if we need to remove or add an entry in the future.

> +	  To compile this driver as a module, choose M here: the module will be
> +	  called ad4851.

Also, with all these devices to be supported why not ad485x as the name of
the driver? Is it a preference by the IIO subsystem?

...

> +#include <asm/unaligned.h>

linux/unaligned nowadays (I learnt it quite recently).
(It requires v6.12-rc2).

...

> +struct ad4851_chip_info {

Have you run `pahole`? It seems you may reduce the memory footprint of this
structure.

> +	const char *name;
> +	unsigned int product_id;
> +	const unsigned int (*scale_table)[2];
> +	int num_scales;
> +	const int *offset_table;
> +	int num_offset;
> +	const struct iio_chan_spec *channels;
> +	unsigned int num_channels;
> +	unsigned long throughput;
> +	unsigned int resolution;
> +};

...

> +static const int ad4851_oversampling_ratios[] = {
> +	1,
> +	2,
> +	4,
> +	8,
> +	16,
> +	32,
> +	64,
> +	128,
> +	256,
> +	512,
> +	1024,
> +	2048,
> +	4096,
> +	8192,
> +	16384,
> +	32768,
> +	65536,

I believe you can compact them to be 4 or 8 per line

	1, 2, 4, 8, 16, 32, 64, 128,			/* 0-7 */
	256, 512, 1024, 2048, 4096, 8192, 16384, 32768,	/* 8-15 */
	65536,						/* 16 */

> +};

...

> +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_update_bits(st->regmap, AD4851_REG_OVERSAMPLE,
> +					 AD4851_OS_EN_MSK, 0);
> +		if (ret)
> +			return ret;
> +	} else {

0 is listed here. Is it a problem?

> +		ret = regmap_update_bits(st->regmap, AD4851_REG_OVERSAMPLE,
> +					 AD4851_OS_EN_MSK, AD4851_OS_EN_MSK);
> +		if (ret)
> +			return ret;
> +
> +		val = ad4851_osr_to_regval(osr);
> +		if (val < 0)
> +			return -EINVAL;
> +
> +		ret = regmap_update_bits(st->regmap, AD4851_REG_OVERSAMPLE,
> +					 AD4851_OS_RATIO_MSK, val);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	switch (chan->scan_type.realbits) {
> +	case 20:
> +		switch (osr) {
> +		case 1:
> +			val = 20;
> +			break;
> +		default:

Ditto.

> +			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;
> +
> +	return regmap_update_bits(st->regmap, AD4851_REG_PACKET,
> +				  AD4851_PACKET_FORMAT_MASK, (osr == 1) ? 0 : 1);

I would do it with a conditional

	if (osr ...)
		return regmap_update_bits(st->regmap, AD4851_REG_PACKET,
					  AD4851_PACKET_FORMAT_MASK, 0);

	return regmap_update_bits(st->regmap, AD4851_REG_PACKET,
				  AD4851_PACKET_FORMAT_MASK, 1);

But looking at the above I would split this to three functions, that outer will
look like

int ...(...)
{
	if (osr ...)
		return _osr_X(...);
	return _osr_Y(...);
}

> +}

...

> +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;
> +		}
> +	}

This magic has to be commented... I have a déjà vu that I have commented on all
this, but it hasn't been addressed!

> +	if (cnt > max_cnt) {
> +		max_cnt = cnt;
> +		max_start = start;
> +	}
> +
> +	if (!max_cnt)
> +		return -ENOENT;
> +
> +	*ret_start = max_start;
> +
> +	return max_cnt;
> +}

Also the cover letter is missing.

I would recommend you to use my "smart" script [1] for sending series, it has
some good heuristics on whom to include into the email thread and handles
missed cover letters for the series.

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 6/6] iio: adc: ad4851: add ad485x driver

[Jonathan Cameron]

On Mon, 14 Oct 2024 16:14:27 +0300
Andy Shevchenko <andy@kernel.org> wrote:

> On Mon, Oct 14, 2024 at 12:40:40PM +0300, 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.  
> 
> ...
> 
> > +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).  
> 
> I think I already commented on this... Anyway, it's much better to support when
> this list is broke down on per device per line. In such a case it's less churn
> if we need to remove or add an entry in the future.
> 
> > +	  To compile this driver as a module, choose M here: the module will be
> > +	  called ad4851.  
> 
> Also, with all these devices to be supported why not ad485x as the name of
> the driver? Is it a preference by the IIO subsystem?

Don't.  We've been bitten by too many cases of manufacturers noticing
a hole in their part numbers and 'slotting' something unrelated in.
So it just causes confusion.  Hence strong preference for any new code
is pick a name from the list.  The wild card also implies restrictions
that tend to break overtime when other part numbers outside the range
are used.  Not using a wildcard keeps it consistently wrong so people
get used to it :)

> 
> ...
> 
> > +#include <asm/unaligned.h>  
> 
> linux/unaligned nowadays (I learnt it quite recently).
> (It requires v6.12-rc2).

Yup.  That bit me in the IIO tree 3 times so far. I've
merged rc2 in for that reason.

Re: [PATCH v3 6/6] iio: adc: ad4851: add ad485x driver

[Andy Shevchenko]

On Mon, Oct 14, 2024 at 08:15:15PM +0100, Jonathan Cameron wrote:
> On Mon, 14 Oct 2024 16:14:27 +0300
> Andy Shevchenko <andy@kernel.org> wrote:
> > On Mon, Oct 14, 2024 at 12:40:40PM +0300, Antoniu Miclaus wrote:

...

> > > +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).  
> > 
> > I think I already commented on this... Anyway, it's much better to support when
> > this list is broke down on per device per line. In such a case it's less churn
> > if we need to remove or add an entry in the future.
> > 
> > > +	  To compile this driver as a module, choose M here: the module will be
> > > +	  called ad4851.  
> > 
> > Also, with all these devices to be supported why not ad485x as the name of
> > the driver? Is it a preference by the IIO subsystem?
> 
> Don't.  We've been bitten by too many cases of manufacturers noticing
> a hole in their part numbers and 'slotting' something unrelated in.
> So it just causes confusion.  Hence strong preference for any new code
> is pick a name from the list.  The wild card also implies restrictions
> that tend to break overtime when other part numbers outside the range
> are used.  Not using a wildcard keeps it consistently wrong so people
> get used to it :)

I see your point!

But shouldn't we have a formal criteria for choosing that one from the list?
I would go with "most featured device" as it may be aligned with all enabled
features that otherwise would be questionable / confusing for the chips that
do not support them or support in a limited manner.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 6/6] iio: adc: ad4851: add ad485x driver

[David Lechner]

On 10/15/24 6:11 AM, Andy Shevchenko wrote:
> On Mon, Oct 14, 2024 at 08:15:15PM +0100, Jonathan Cameron wrote:
>> On Mon, 14 Oct 2024 16:14:27 +0300
>> Andy Shevchenko <andy@kernel.org> wrote:
>>> On Mon, Oct 14, 2024 at 12:40:40PM +0300, Antoniu Miclaus wrote:
> 
> ...
> 
>>>> +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).  
>>>
>>> I think I already commented on this... Anyway, it's much better to support when
>>> this list is broke down on per device per line. In such a case it's less churn
>>> if we need to remove or add an entry in the future.
>>>
>>>> +	  To compile this driver as a module, choose M here: the module will be
>>>> +	  called ad4851.  
>>>
>>> Also, with all these devices to be supported why not ad485x as the name of
>>> the driver? Is it a preference by the IIO subsystem?
>>
>> Don't.  We've been bitten by too many cases of manufacturers noticing
>> a hole in their part numbers and 'slotting' something unrelated in.
>> So it just causes confusion.  Hence strong preference for any new code
>> is pick a name from the list.  The wild card also implies restrictions
>> that tend to break overtime when other part numbers outside the range
>> are used.  Not using a wildcard keeps it consistently wrong so people
>> get used to it :)
> 
> I see your point!
> 
> But shouldn't we have a formal criteria for choosing that one from the list?
> I would go with "most featured device" as it may be aligned with all enabled
> features that otherwise would be questionable / confusing for the chips that
> do not support them or support in a limited manner.
> 

I always go with the lowest number supported by the driver at the time
the driver was created. It is a simple, objective criteria and no one
has to spend time looking through features to decide which one is "best".

Re: [PATCH v3 6/6] iio: adc: ad4851: add ad485x driver

[David Lechner]

On 10/14/24 8:14 AM, Andy Shevchenko wrote:
> On Mon, Oct 14, 2024 at 12:40:40PM +0300, 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.
> 

...

>> +	return regmap_update_bits(st->regmap, AD4851_REG_PACKET,
>> +				  AD4851_PACKET_FORMAT_MASK, (osr == 1) ? 0 : 1);
> 
> I would do it with a conditional
> 
> 	if (osr ...)
> 		return regmap_update_bits(st->regmap, AD4851_REG_PACKET,
> 					  AD4851_PACKET_FORMAT_MASK, 0);
> 
> 	return regmap_update_bits(st->regmap, AD4851_REG_PACKET,
> 				  AD4851_PACKET_FORMAT_MASK, 1);
> 
If we do this, regmap_set_bits() and regmap_clear_bits() would
be even better.

Re: [PATCH v3 6/6] iio: adc: ad4851: add ad485x driver

[Andy Shevchenko]

On Mon, Oct 14, 2024 at 05:08:47PM -0500, David Lechner wrote:
> On 10/14/24 8:14 AM, Andy Shevchenko wrote:
> > On Mon, Oct 14, 2024 at 12:40:40PM +0300, Antoniu Miclaus wrote:

...

> >> +	return regmap_update_bits(st->regmap, AD4851_REG_PACKET,
> >> +				  AD4851_PACKET_FORMAT_MASK, (osr == 1) ? 0 : 1);
> > 
> > I would do it with a conditional
> > 
> > 	if (osr ...)
> > 		return regmap_update_bits(st->regmap, AD4851_REG_PACKET,
> > 					  AD4851_PACKET_FORMAT_MASK, 0);
> > 
> > 	return regmap_update_bits(st->regmap, AD4851_REG_PACKET,
> > 				  AD4851_PACKET_FORMAT_MASK, 1);
> > 
> If we do this, regmap_set_bits() and regmap_clear_bits() would
> be even better.

Sure, but I want also to have the only one conditional, so the respective
helper functions can be easily read and followed.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 6/6] iio: adc: ad4851: add ad485x driver

[David Lechner]

On 10/14/24 4:40 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 int ad4851_set_sampling_freq(struct ad4851_state *st, unsigned int freq)
> +{
> +	struct pwm_state cnv_state = {
> +		.duty_cycle = AD4851_T_CNVH_NS,

PWM drivers typically round down, so this mimimum required
high time may not be met if pwm_apply cannot make an exact
match.

> +		.enabled = true,
> +	};
> +	int ret;
> +
> +	freq = clamp(freq, 1, st->info->throughput);
> +
> +	cnv_state.period = DIV_ROUND_CLOSEST_ULL(GIGA, freq);

As Uwe mentioned in v2, ROUND_CLOSEST doesn't seem like the
best choice here.

And usually we use NSEC_PER_SEC for this instead of GIGA.

> +
> +	ret = pwm_apply_might_sleep(st->cnv, &cnv_state);
> +	if (ret)
> +		return ret;

pwm_get_state_hw() is currently not public. But it would be nice
to make it public and use it here to assign st->sampling_freq to
the actual frequency the hardware is capable of instead of the
requested frequency. This way users can read back the sampling
frequency attribute to know what the real sample rate is in case
the exact requested rate was not possible.

> +
> +	st->sampling_freq = freq;
> +
> +	return 0;
> +}
> +

...

> +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_update_bits(st->regmap, AD4851_REG_OVERSAMPLE,
> +					 AD4851_OS_EN_MSK, 0);
> +		if (ret)
> +			return ret;
> +	} else {
> +		ret = regmap_update_bits(st->regmap, AD4851_REG_OVERSAMPLE,
> +					 AD4851_OS_EN_MSK, AD4851_OS_EN_MSK);
> +		if (ret)
> +			return ret;

regmap_clear_bits() and regmap_set_bits() would make this a bit
less verbose and consistent with the effort started in [1].

[1]: https://lore.kernel.org/linux-iio/20240617-review-v3-0-88d1338c4cca@baylibre.com/


> +
> +		val = ad4851_osr_to_regval(osr);
> +		if (val < 0)
> +			return -EINVAL;
> +
> +		ret = regmap_update_bits(st->regmap, AD4851_REG_OVERSAMPLE,
> +					 AD4851_OS_RATIO_MSK, val);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	switch (chan->scan_type.realbits) {
> +	case 20:
> +		switch (osr) {
> +		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;
> +
> +	return regmap_update_bits(st->regmap, AD4851_REG_PACKET,
> +				  AD4851_PACKET_FORMAT_MASK, (osr == 1) ? 0 : 1);
> +}
> +
> +static int ad4851_get_oversampling_ratio(struct ad4851_state *st, unsigned int *val)
> +{
> +	unsigned int osr;
> +	int ret;
> +
> +	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)];

Why is 1 not in the table?

> +
> +	return IIO_VAL_INT;
> +}
> +
> +static int ad4851_setup(struct ad4851_state *st)
> +{
> +	unsigned int product_id;
> +	int ret;
> +

Would be nice to do a hard reset here if possible using st->pd_gpio
(datasheet says to cycle this twice and then wait 1 ms).

> +	ret = ad4851_set_sampling_freq(st, HZ_PER_MHZ);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(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_write(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_set_calibscale(struct ad4851_state *st, int ch, int val,
> +				 int val2)
> +{
> +	unsigned long long gain;

	u64

> +	u8 buf[0];
> +	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_set_offset(struct ad4851_state *st,
> +			     const struct iio_chan_spec *chan, int val)
> +{
> +	guard(mutex)(&st->lock);
> +
> +	if (val != st->offsets[chan->channel])
> +		return 0;
> +
> +	st->offsets[chan->channel] = val;
> +	/* Restore to the default range if offset changes */
> +	if (st->offsets[chan->channel])
> +		return regmap_write(st->regmap,
> +					AD4851_REG_CHX_SOFTSPAN(chan->channel),
> +					AD4851_SOFTSPAN_N40V_40V);
> +	return regmap_write(st->regmap,
> +				AD4851_REG_CHX_SOFTSPAN(chan->channel),
> +				AD4851_SOFTSPAN_0V_40V);

Pretty sure I mentioned this in a previous review...

I don't see how changing the softspan affects the offset. A raw value
of 0 is still 0V either way.

It should only affect the scale and signed vs. unsigned data.

> +}
> +

...

> +
> +#define AD4851_IIO_CHANNEL(index, real, storage)			\
> +{									\
> +	.type = IIO_VOLTAGE,						\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_CALIBSCALE) |		\
> +		BIT(IIO_CHAN_INFO_CALIBBIAS) |				\
> +		BIT(IIO_CHAN_INFO_SCALE) |				\
> +		BIT(IIO_CHAN_INFO_OFFSET),				\
> +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) |	\
> +		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),			\
> +	.info_mask_shared_by_type_available =				\
> +		BIT(IIO_CHAN_INFO_SCALE) |				\
> +		BIT(IIO_CHAN_INFO_OFFSET) |				\
> +		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),			\
> +	.indexed = 1,							\
> +	.channel = index,						\

These chips are fully differential, so I would expect:

	.differential = 1,
	.channel = 2 * index,
	.channel2 = 2 * index + 1,

Or alternatly, if we use the devicetree to specify the type of input
attached, (differential or signle-ended) then this would be dynamically
configured.

> +	.scan_index = index,						\
> +	.scan_type = {							\
> +		.sign = 's',						\
> +		.realbits = real,					\
> +		.storagebits = storage,					\

Since enabling oversampling can change realbits, this driver will likely
need to implement scan_type_ext so that userspace is aware of the
difference when oversampling is enabled. (Adding support for oversampling
could always be a followup patch instead of trying to do everything
all at once.)

See the ad7380 driver as an example of how to impelemt this. [2]

[2]: https://lore.kernel.org/linux-iio/20240530-iio-add-support-for-multiple-scan-types-v3-5-cbc4acea2cfa@baylibre.com/

Also, I would expect the .sign value to depend on how the
input is being used. If it is differential or single-ended
bipolar, then it is signed, but if it is signle-ended unipoloar
then it is unsiged.

Typically, this is coming from the devicetree because it
depends on what is wired up to the input.

> +	},								\
> +}

...

> +static int ad4851_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;
> +	struct ad4851_state *st;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->spi = spi;
> +
> +	ret = devm_mutex_init(&spi->dev, &st->lock);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_regulator_bulk_get_enable(&spi->dev,
> +					     ARRAY_SIZE(ad4851_power_supplies),
> +					     ad4851_power_supplies);
> +	if (ret)
> +		return dev_err_probe(&spi->dev, ret,
> +				     "failed to get and enable supplies\n");
> +
> +	ret = devm_regulator_get_enable_optional(&spi->dev, "vddh");
> +	if (ret < 0 && ret != -ENODEV)
> +		return dev_err_probe(&spi->dev, ret, "failed to get vddh voltage\n");
> +
> +	ret = devm_regulator_get_enable_optional(&spi->dev, "vddl");
> +	if (ret < 0 && ret != -ENODEV)
> +		return dev_err_probe(&spi->dev, ret, "failed to get vddl voltage\n");
> +
> +	ret = devm_regulator_get_enable_optional(&spi->dev, "vrefbuf");
> +	if (ret < 0 && ret != -ENODEV)
> +		return dev_err_probe(&spi->dev, ret, "failed to get vrefbuf voltage\n");

According to the datasheet, if there is a supply cconnected to the
REFBUF pin, then "Disable the internal band-gap reference and the
internal reference buffer through the DEVICE_CTRL register in this
configuration and connect the REFIO pin to the GND pins."

So we probably shoudn't be enabling this supply until after
configuring the registers.

> +
> +	ret = devm_regulator_get_enable_optional(&spi->dev, "vddl");

Should be "vrefio", not vddl".

> +	if (ret < 0 && ret != -ENODEV)
> +		return dev_err_probe(&spi->dev, ret, "failed to get vrefio voltage\n");

We need to keep track if this supply is present or not and set
REF_SEL in the DEVICE_CTRL register accordingly.

> +
> +	st->pd_gpio = devm_gpiod_get_optional(&spi->dev, "pd", GPIOD_OUT_LOW);
> +	if (IS_ERR(st->pd_gpio))
> +		return dev_err_probe(&spi->dev, PTR_ERR(st->pd_gpio),
> +				     "Error on requesting pd GPIO\n");
> +
> +	st->cnv = devm_pwm_get(&spi->dev, NULL);
> +	if (IS_ERR(st->cnv))
> +		return dev_err_probe(&spi->dev, PTR_ERR(st->cnv),
> +				     "Error on requesting pwm\n");
> +
> +	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_offset_fill(st);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4851_setup(st);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->name = st->info->name;
> +	indio_dev->channels = st->info->channels;
> +	indio_dev->num_channels = st->info->num_channels;
> +	indio_dev->info = &ad4851_iio_info;
> +
> +	st->back = devm_iio_backend_get(&spi->dev, NULL);
> +	if (IS_ERR(st->back))
> +		return PTR_ERR(st->back);
> +
> +	ret = devm_iio_backend_request_buffer(&spi->dev, st->back, indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_iio_backend_enable(&spi->dev, st->back);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4851_calibrate(st);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(&spi->dev, indio_dev);
> +}
> +
> +static const struct of_device_id ad4851_of_match[] = {
> +	{ .compatible = "adi,ad4858", .data = &ad4851_info, },
> +	{ .compatible = "adi,ad4857", .data = &ad4852_info, },
> +	{ .compatible = "adi,ad4856", .data = &ad4853_info, },
> +	{ .compatible = "adi,ad4855", .data = &ad4854_info, },
> +	{ .compatible = "adi,ad4854", .data = &ad4855_info, },
> +	{ .compatible = "adi,ad4853", .data = &ad4856_info, },
> +	{ .compatible = "adi,ad4852", .data = &ad4857_info, },
> +	{ .compatible = "adi,ad4851", .data = &ad4858_info, },
> +	{ .compatible = "adi,ad4858i", .data = &ad4858i_info, },
> +	{}

As requested in previous reviews, { } is preferred to be consistent.

And more importantly, it looks like compatible strings are in
reverse order compared to info structs.

> +};
> +

RE: [PATCH v3 6/6] iio: adc: ad4851: add ad485x driver

[Miclaus, Antoniu]

> 
> > +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_update_bits(st->regmap,
> AD4851_REG_OVERSAMPLE,
> > +					 AD4851_OS_EN_MSK, 0);
> > +		if (ret)
> > +			return ret;
> > +	} else {
> > +		ret = regmap_update_bits(st->regmap,
> AD4851_REG_OVERSAMPLE,
> > +					 AD4851_OS_EN_MSK,
> AD4851_OS_EN_MSK);
> > +		if (ret)
> > +			return ret;
> 
> regmap_clear_bits() and regmap_set_bits() would make this a bit
> less verbose and consistent with the effort started in [1].
> 
> [1]: https://urldefense.com/v3/__https://lore.kernel.org/linux-
> iio/20240617-review-v3-0-
> 88d1338c4cca@baylibre.com/__;!!A3Ni8CS0y2Y!4LS7UI11XqIHRgT3ckx76VY
> nCyeikpTumyjO0qDTn7eF7Fd-
> jFFL8yqpYcMAxP_u3VC09bfIAB7gW_rv3yGMDWs$
>
Will do in v5.
> 
> > +
> > +		val = ad4851_osr_to_regval(osr);
> > +		if (val < 0)
> > +			return -EINVAL;
> > +
> > +		ret = regmap_update_bits(st->regmap,
> AD4851_REG_OVERSAMPLE,
> > +					 AD4851_OS_RATIO_MSK, val);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	switch (chan->scan_type.realbits) {
> > +	case 20:
> > +		switch (osr) {
> > +		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;
> > +
> > +	return regmap_update_bits(st->regmap, AD4851_REG_PACKET,
> > +				  AD4851_PACKET_FORMAT_MASK, (osr == 1)
> ? 0 : 1);
> > +}
> > +
> > +static int ad4851_get_oversampling_ratio(struct ad4851_state *st,
> unsigned int *val)
> > +{
> > +	unsigned int osr;
> > +	int ret;
> > +
> > +	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)];
> 
> Why is 1 not in the table?
Because there is no 1 in the OS_RATIO table from datasheet. 1 means you disable the OS via OS_EN bit.
> 
> > +
> > +	return IIO_VAL_INT;
> > +}
> > +
> > +static int ad4851_setup(struct ad4851_state *st)
> > +{
> > +	unsigned int product_id;
> > +	int ret;
> > +
> 
> Would be nice to do a hard reset here if possible using st->pd_gpio
> (datasheet says to cycle this twice and then wait 1 ms).

Sure, will do in v5.

> > +	ret = ad4851_set_sampling_freq(st, HZ_PER_MHZ);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_write(st->regmap,
> AD4851_REG_INTERFACE_CONFIG_A,
> > +			   AD4851_SW_RESET);
> > +	if (ret)
> > +		return ret;
> > +
...
> 
> > +	.scan_index = index,						\
> > +	.scan_type = {							\
> > +		.sign = 's',						\
> > +		.realbits = real,					\
> > +		.storagebits = storage,					\
> 
> Since enabling oversampling can change realbits, this driver will likely
> need to implement scan_type_ext so that userspace is aware of the
> difference when oversampling is enabled. (Adding support for oversampling
> could always be a followup patch instead of trying to do everything
> all at once.)

Will do in v5.

> 
> See the ad7380 driver as an example of how to impelemt this. [2]
> 
> [2]: https://urldefense.com/v3/__https://lore.kernel.org/linux-
> iio/20240530-iio-add-support-for-multiple-scan-types-v3-5-
> cbc4acea2cfa@baylibre.com/__;!!A3Ni8CS0y2Y!4LS7UI11XqIHRgT3ckx76VYn
> CyeikpTumyjO0qDTn7eF7Fd-
> jFFL8yqpYcMAxP_u3VC09bfIAB7gW_rvGoM_sEA$
> 
> Also, I would expect the .sign value to depend on how the
> input is being used. If it is differential or single-ended
> bipolar, then it is signed, but if it is signle-ended unipoloar
> then it is unsiged.
> 
> Typically, this is coming from the devicetree because it
> depends on what is wired up to the input.

This topic is mentioned in the cover letter, maybe not argued enough there.
Yes, the go-to approach is to specify the unipolar/bipolar configuration in the devicetree.
But this is a request from the actual users of the driver: to have the softspan fully
controlled from userspace. That's why the offset and scale implementations were added.
Both these attributes are influencing the softspan.

> > +	},								\
> > +}
> 
> ...

Re: [PATCH v3 6/6] iio: adc: ad4851: add ad485x driver

[David Lechner]

On 10/25/24 6:35 AM, Miclaus, Antoniu wrote:
>>
...

>>
>> See the ad7380 driver as an example of how to impelemt this. [2]
>>
>> [2]: https://urldefense.com/v3/__https://lore.kernel.org/linux-
>> iio/20240530-iio-add-support-for-multiple-scan-types-v3-5-
>> cbc4acea2cfa@baylibre.com/__;!!A3Ni8CS0y2Y!4LS7UI11XqIHRgT3ckx76VYn
>> CyeikpTumyjO0qDTn7eF7Fd-
>> jFFL8yqpYcMAxP_u3VC09bfIAB7gW_rvGoM_sEA$
>>
>> Also, I would expect the .sign value to depend on how the
>> input is being used. If it is differential or single-ended
>> bipolar, then it is signed, but if it is signle-ended unipoloar
>> then it is unsiged.
>>
>> Typically, this is coming from the devicetree because it
>> depends on what is wired up to the input.
> 
> This topic is mentioned in the cover letter, maybe not argued enough there.
> Yes, the go-to approach is to specify the unipolar/bipolar configuration in the devicetree.
> But this is a request from the actual users of the driver: to have the softspan fully
> controlled from userspace. That's why the offset and scale implementations were added.
> Both these attributes are influencing the softspan.
> 
>>> +	},								\
>>> +}
>>

The cover letter did not get sent, so we did not see this.

Still, I have doubts about using the offset attribute for
this since a 0 raw value is always 0V for both unipolar
and bipolar cases. There is never an offset to apply to
the raw value.

So I think we will need to find a different way to control
this other than the offset attribute.

Re: [PATCH v3 6/6] iio: adc: ad4851: add ad485x driver

[David Lechner]

On 10/25/24 9:29 AM, David Lechner wrote:
> On 10/25/24 6:35 AM, Miclaus, Antoniu wrote:
>>>
> ...
> 
>>>
>>> See the ad7380 driver as an example of how to impelemt this. [2]
>>>
>>> [2]: https://urldefense.com/v3/__https://lore.kernel.org/linux-
>>> iio/20240530-iio-add-support-for-multiple-scan-types-v3-5-
>>> cbc4acea2cfa@baylibre.com/__;!!A3Ni8CS0y2Y!4LS7UI11XqIHRgT3ckx76VYn
>>> CyeikpTumyjO0qDTn7eF7Fd-
>>> jFFL8yqpYcMAxP_u3VC09bfIAB7gW_rvGoM_sEA$
>>>
>>> Also, I would expect the .sign value to depend on how the
>>> input is being used. If it is differential or single-ended
>>> bipolar, then it is signed, but if it is signle-ended unipoloar
>>> then it is unsiged.
>>>
>>> Typically, this is coming from the devicetree because it
>>> depends on what is wired up to the input.
>>
>> This topic is mentioned in the cover letter, maybe not argued enough there.
>> Yes, the go-to approach is to specify the unipolar/bipolar configuration in the devicetree.
>> But this is a request from the actual users of the driver: to have the softspan fully
>> controlled from userspace. That's why the offset and scale implementations were added.
>> Both these attributes are influencing the softspan.
>>
>>>> +	},								\
>>>> +}
>>>
> 
> The cover letter did not get sent, so we did not see this.

So please resend it so we can get the full explanation.

> 
> Still, I have doubts about using the offset attribute for
> this since a 0 raw value is always 0V for both unipolar
> and bipolar cases. There is never an offset to apply to
> the raw value.
> 
> So I think we will need to find a different way to control
> this other than the offset attribute.

I thought about this some more and I have an idea to solve the
issue without using devicetree or the offset attribute.

But we should see what Jonathan thinks before implementing this
in case it isn't a good idea.

We can expose each voltage input to userspace as two different
channels, a single-ended channel and a differential channel.

For an 8 channel chip, we would have 16 IIO channels (in order
of scan_index):

in_voltage0_raw
in_voltage0-voltage8_raw
in_voltage1_raw
in_voltage1-voltage9_raw
...
in_voltage7_raw
in_voltage7-voltage15_raw

If you read the voltage using in_voltageX_raw, then the SoftSpan
for that channel gets set to the 0V to +V value based on
in_voltageX_scale. Likewise, if you read the in_voltageX-voltageY_raw
attribute, the SoftSpan gets set to -V to +V according to
in_voltageX-voltageY_scale.

For buffered reads, only one of each in_voltageX_raw/in_voltageX-voltageY_raw
pair can be enabled at the same time (because the chip is simultaneous
sampling).

Re: [PATCH v3 6/6] iio: adc: ad4851: add ad485x driver

[Jonathan Cameron]

On Fri, 25 Oct 2024 14:55:13 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 10/25/24 9:29 AM, David Lechner wrote:
> > On 10/25/24 6:35 AM, Miclaus, Antoniu wrote:  
> >>>  
> > ...
> >   
> >>>
> >>> See the ad7380 driver as an example of how to impelemt this. [2]
> >>>
> >>> [2]: https://urldefense.com/v3/__https://lore.kernel.org/linux-
> >>> iio/20240530-iio-add-support-for-multiple-scan-types-v3-5-
> >>> cbc4acea2cfa@baylibre.com/__;!!A3Ni8CS0y2Y!4LS7UI11XqIHRgT3ckx76VYn
> >>> CyeikpTumyjO0qDTn7eF7Fd-
> >>> jFFL8yqpYcMAxP_u3VC09bfIAB7gW_rvGoM_sEA$
> >>>
> >>> Also, I would expect the .sign value to depend on how the
> >>> input is being used. If it is differential or single-ended
> >>> bipolar, then it is signed, but if it is signle-ended unipoloar
> >>> then it is unsiged.
> >>>
> >>> Typically, this is coming from the devicetree because it
> >>> depends on what is wired up to the input.  
> >>
> >> This topic is mentioned in the cover letter, maybe not argued enough there.
> >> Yes, the go-to approach is to specify the unipolar/bipolar configuration in the devicetree.
> >> But this is a request from the actual users of the driver: to have the softspan fully
> >> controlled from userspace. That's why the offset and scale implementations were added.
> >> Both these attributes are influencing the softspan.
> >>  
> >>>> +	},								\
> >>>> +}  
> >>>  
> > 
> > The cover letter did not get sent, so we did not see this.  
> 
> So please resend it so we can get the full explanation.
> 
> > 
> > Still, I have doubts about using the offset attribute for
> > this since a 0 raw value is always 0V for both unipolar
> > and bipolar cases. There is never an offset to apply to
> > the raw value.
> > 
> > So I think we will need to find a different way to control
> > this other than the offset attribute.  
> 
> I thought about this some more and I have an idea to solve the
> issue without using devicetree or the offset attribute.
> 
> But we should see what Jonathan thinks before implementing this
> in case it isn't a good idea.
> 
> We can expose each voltage input to userspace as two different
> channels, a single-ended channel and a differential channel.

That was common in early drivers - such as the max1363 because they
were well prior to having sufficiently complex bindings to specify
wired channels.  We also have drivers that do this if no channel
subnodes are provided (kind of a fallback).

> 
> For an 8 channel chip, we would have 16 IIO channels (in order
> of scan_index):
> 
> in_voltage0_raw
> in_voltage0-voltage8_raw
> in_voltage1_raw
> in_voltage1-voltage9_raw
> ...
> in_voltage7_raw
> in_voltage7-voltage15_raw
> 
> If you read the voltage using in_voltageX_raw, then the SoftSpan
> for that channel gets set to the 0V to +V value based on
> in_voltageX_scale. Likewise, if you read the in_voltageX-voltageY_raw
> attribute, the SoftSpan gets set to -V to +V according to
> in_voltageX-voltageY_scale.
> 
> For buffered reads, only one of each in_voltageX_raw/in_voltageX-voltageY_raw
> pair can be enabled at the same time (because the chip is simultaneous
> sampling).
> 
This approach is fine as it's pretty much what some existing parts
are doing even if mostly people are these days preferring the
specified channel route.

Jonathan