[PATCH 0/7] *** Add support for AD485x DAS Family ***

[PATCH 0/7] *** 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 (7):
  iio: backend: add API for interface get
  iio: backend: add support for data size set
  iio: adc: adi-axi-adc: add interface type
  iio: adc: adi-axi-adc: set data format
  dt-bindings: iio: adc: add ad458x
  iio: adc: ad485x: add ad485x driver
  Documentation: ABI: testing: ad485x: add ABI docs

 .../ABI/testing/sysfs-bus-iio-adc-ad485x      |   14 +
 .../bindings/iio/adc/adi,ad485x.yaml          |   82 ++
 drivers/iio/adc/Kconfig                       |   12 +
 drivers/iio/adc/Makefile                      |    1 +
 drivers/iio/adc/ad485x.c                      | 1061 +++++++++++++++++
 drivers/iio/adc/adi-axi-adc.c                 |   44 +
 drivers/iio/industrialio-backend.c            |   45 +
 include/linux/iio/backend.h                   |   13 +
 8 files changed, 1272 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-ad485x
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad485x.yaml
 create mode 100644 drivers/iio/adc/ad485x.c

-- 
2.46.0

[PATCH v5 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 v5:
 - drop IIO_BACKEND_INTERFACE_LVDS and IIO_BACKEND_INTERFACE_CMOS from enum
 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.0

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

[David Lechner]

On 11/1/24 6:23 AM, Antoniu Miclaus wrote:
> Add backend support for obtaining the interface type used.
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> ---
Reviewed-by: David Lechner <dlechner@baylibre.com>

[PATCH v5 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 v5.
 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..6f4e38417dfd 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.
+ */
+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
  * @back: Backend device
diff --git a/include/linux/iio/backend.h b/include/linux/iio/backend.h
index e5ea90f1c3e0..83ca4f0124db 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, ssize_t 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);
+size_t iio_backend_data_size_set(struct iio_backend *back, size_t size);
 int iio_backend_read_raw(struct iio_backend *back,
 			 struct iio_chan_spec const *chan, int *val, int *val2,
 			 long mask);
-- 
2.47.0

Re: [PATCH v5 2/6] iio: backend: add support for data size set

[David Lechner]

On 11/1/24 6:23 AM, Antoniu Miclaus wrote:
> 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 v5.
>  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..6f4e38417dfd 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.
> + */
> +size_t iio_backend_data_size_set(struct iio_backend *back, size_t size)

Should be returning int, not size_t.

Also, size_t for size parameter seems a bit much when this is taking bits.
I don't think anything is ever going to need 5 billion bits. :-p

> +{
> +	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..83ca4f0124db 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, ssize_t 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);
> +size_t iio_backend_data_size_set(struct iio_backend *back, size_t size);

This will need to be changed as well.

>  int iio_backend_read_raw(struct iio_backend *back,
>  			 struct iio_chan_spec const *chan, int *val, int *val2,
>  			 long mask);

[PATCH v5 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>
---
changes in v5:
 - use IIO_BACKEND_INTERFACE_SERIAL_CMOS and IIO_BACKEND_INTERFACE_SERIAL_LVDS
 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.0

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

[David Lechner]

On 11/1/24 6:23 AM, Antoniu Miclaus wrote:
> Add support for getting the interface (CMOS or LVDS) used by the AXI ADC
> IP.
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> ---

Reviewed-by: David Lechner <dlechner@baylibre.com>

> changes in v5:
>  - use IIO_BACKEND_INTERFACE_SERIAL_CMOS and IIO_BACKEND_INTERFACE_SERIAL_LVDS
>  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)

FIELD_GET() also works here.

> +		*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),
>  };

[PATCH v5 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 v5.
 drivers/iio/adc/adi-axi-adc.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
index f6475bc93796..6f658d9b4c9d 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_CTRL_MSK	GENMASK(7, 0)
+
 #define ADI_AXI_ADC_REG_DRP_STATUS		0x0074
 #define   ADI_AXI_ADC_DRP_LOCKED		BIT(17)
 
@@ -312,6 +315,24 @@ 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)
 {
@@ -360,6 +381,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.0

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

[David Lechner]

On 11/1/24 6:23 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>
> ---
> no changes in v5.
>  drivers/iio/adc/adi-axi-adc.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
> index f6475bc93796..6f658d9b4c9d 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_CTRL_MSK	GENMASK(7, 0)
> +
>  #define ADI_AXI_ADC_REG_DRP_STATUS		0x0074
>  #define   ADI_AXI_ADC_DRP_LOCKED		BIT(17)
>  
> @@ -312,6 +315,24 @@ 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;

Should these be exact matches instead of "<="?

Also, what would val = 2 mean? Perhaps we need some macros to explain
the meanings of these values. The docs linked below give the meaning
for a different chip, but not AD485x.

> +	else
> +		return -EINVAL;
> +
> +	return regmap_update_bits(st->regmap, ADI_AXI_ADC_REG_CNTRL_3,
> +				  ADI_AXI_ADC_CNTRL_3_CUSTOM_CONTROL_MSK, val);

My understanding is that the use of REG_CHAN_CNTRL_3 is different
for every HDL project depending on what (frontend) chip is is being
used with. In the AXI DAC, we added a new compatible string for this
(and other reasons). Not sure if we need to go that far here, but I
would at least put a comment here explaining that this use of the
register is highly specific to the AXI AD485x variant [1] of the
AXI ADC IP core.

Ideally though, there should be an ID register that we can read
to get this info or use a different DT compatible string.

[1]: http://analogdevicesinc.github.io/hdl/library/axi_ad485x/index.html

> +}
> +
>  static struct iio_buffer *axi_adc_request_buffer(struct iio_backend *back,
>  						 struct iio_dev *indio_dev)
>  {
> @@ -360,6 +381,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),
>  };

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

[David Lechner]

On 11/1/24 2:52 PM, David Lechner wrote:
> On 11/1/24 6:23 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>
>> ---
>> no changes in v5.
>>  drivers/iio/adc/adi-axi-adc.c | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
>> index f6475bc93796..6f658d9b4c9d 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_CTRL_MSK	GENMASK(7, 0)
>> +
>>  #define ADI_AXI_ADC_REG_DRP_STATUS		0x0074
>>  #define   ADI_AXI_ADC_DRP_LOCKED		BIT(17)
>>  
>> @@ -312,6 +315,24 @@ 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;
> 
> Should these be exact matches instead of "<="?
> 
> Also, what would val = 2 mean? Perhaps we need some macros to explain
> the meanings of these values. The docs linked below give the meaning
> for a different chip, but not AD485x.
> 
>> +	else
>> +		return -EINVAL;
>> +
>> +	return regmap_update_bits(st->regmap, ADI_AXI_ADC_REG_CNTRL_3,
>> +				  ADI_AXI_ADC_CNTRL_3_CUSTOM_CONTROL_MSK, val);
> 

Answering my own question:

I did some digging in the HDL source code and found that there
are actually multiple field here.

So instead of ADI_AXI_ADC_CNTRL_3_CUSTOM_CTRL_MSK, we should have

#define AD485X_CNTRL_3_CUSTOM_CTRL_OVERSAMPLING_EN	BIT(2)
#define AD485X_CNTRL_3_CUSTOM_CTRL_PACKET_FORMAT	GENMASK(1, 0)

And the meaning of PACKET_FORMAT is different for 16-bit vs.
20-bit chips and in some cases if oversampling is enabled or not.

For 16-bit chips:
	0 = 16-bit data and no status bits
	1 = 16-bit data and 8 status bits

For 20-bit chips:
	0 = 20-bit data and no status bits
	1 = 20-bit data and 4 status bits OR
	    24-bit data and no status bits (oversampling)
	2 = 20-bit data and 8 status bits and 4 bit padding OR
	    24-bit data and 8 status bits (oversampling)
	3 = Same as 2

So this tells me that A) we probably need a separate oversampling
enable callback and B) we should be more clear about what "data
size" means. Do we mean just the sample data size (realbits) or
do we mean the sample data plus status bit (realbits + shift).

The implementation is fine for now (other than we should remove the
val = 3 case). But if we need to enable status bit in the future,
it won't be compatible with this function.

> My understanding is that the use of REG_CHAN_CNTRL_3 is different
> for every HDL project depending on what (frontend) chip is is being
> used with. In the AXI DAC, we added a new compatible string for this
> (and other reasons). Not sure if we need to go that far here, but I
> would at least put a comment here explaining that this use of the
> register is highly specific to the AXI AD485x variant [1] of the
> AXI ADC IP core.
> 
> Ideally though, there should be an ID register that we can read
> to get this info or use a different DT compatible string.
> 
> [1]: http://analogdevicesinc.github.io/hdl/library/axi_ad485x/index.html
> 
>> +}
>> +
>>  static struct iio_buffer *axi_adc_request_buffer(struct iio_backend *back,
>>  						 struct iio_dev *indio_dev)
>>  {
>> @@ -360,6 +381,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),
>>  };
> 

[PATCH v5 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 v5:
 - fix example by using proper regulator node.
 .../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..9e9439fed3ef
--- /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 = <&vddl>;
+            vio-supply = <&vio>;
+            pwms = <&pwm_gen 0 0>;
+            io-backends = <&iio_backend>;
+        };
+    };
+...
-- 
2.47.0

[PATCH v5 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>
---
changes in v5:
 - use linux/unaligned.h
 - drop redundant masks for reg_val
 - use regmap_clear_bits/regmap_set_bits where applicable.
 - add hard reset sequence via pd_gpio
 - use scan_type_ext and implement resolution normal/boost.
 - drop offset, implement differential channels and handle scale based on the
   channel type.
 drivers/iio/adc/Kconfig  |   13 +
 drivers/iio/adc/Makefile |    1 +
 drivers/iio/adc/ad4851.c | 1179 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 1193 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..0ef8ea0d2fc2
--- /dev/null
+++ b/drivers/iio/adc/ad4851.c
@@ -0,0 +1,1179 @@
+// 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
+
+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;
+};
+
+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)[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_DOWN_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;
+	} else {
+		ret = regmap_set_bits(st->regmap, AD4851_REG_OVERSAMPLE,
+				      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 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) {
+		ret = regmap_update_bits(st->regmap, AD4851_REG_PACKET,
+					 AD4851_PACKET_FORMAT_MASK, 0);
+		if (ret)
+			return ret;
+
+		st->resolution_boost_enabled = false;
+	} else {
+		ret = regmap_update_bits(st->regmap, AD4851_REG_PACKET,
+					 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;
+
+	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 int ad4851_setup(struct ad4851_state *st)
+{
+	unsigned int product_id;
+	int ret;
+
+	if (st->pd_gpio) {
+		gpiod_set_value(st->pd_gpio, GPIOD_OUT_HIGH);
+		fsleep(1);
+		gpiod_set_value(st->pd_gpio, GPIOD_OUT_LOW);
+		fsleep(1);
+		gpiod_set_value(st->pd_gpio, GPIOD_OUT_HIGH);
+		fsleep(1);
+		gpiod_set_value(st->pd_gpio, GPIOD_OUT_LOW);
+		fsleep(1000);
+	}
+
+	if (!IS_ERR(st->vrefbuf)) {
+		ret = regmap_update_bits(st->regmap, AD4851_REG_DEVICE_CTRL,
+					 AD4851_REFBUF_PD, 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_update_bits(st->regmap, AD4851_REG_DEVICE_CTRL,
+					 AD4851_REFSEL_PD, 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;
+	}
+
+	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;
+		}
+	}
+	/*
+	 * 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, 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_SERIAL_CMOS:
+		lane_num = st->info->num_channels;
+		break;
+	case IIO_BACKEND_INTERFACE_SERIAL_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 << 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[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 channel type.
+		 *
+		 * If the channel is not differential then continue iterations
+		 * until the next matching scale value which always corresponds
+		 * to the single ended mode.
+		 */
+		if (!chan->differential && !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)
+{
+	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_scale_fill(struct ad4851_state *st)
+{
+	unsigned int i, val1, val2;
+
+	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);
+		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_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_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[] = {
+	[AD4851_SCAN_TYPE_NORMAL] = {
+		.sign = 's',
+		.realbits = 16,
+		.storagebits = 16,
+	},
+	[AD4851_SCAN_TYPE_RESOLUTION_BOOST] = {
+		.sign = 's',
+		.realbits = 16,
+		.storagebits = 16,
+	},
+};
+
+static const struct iio_scan_type ad4851_scan_type_20[] = {
+	[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, diff, real)				\
+{									\
+	.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_type_available =				\
+		BIT(IIO_CHAN_INFO_SCALE) |				\
+		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),			\
+	.indexed = 1,							\
+	.differential = diff,						\
+	.channel = index,						\
+	.channel2 = index + diff * 8,					\
+	.scan_index = index + diff * 8,					\
+	.ext_scan_type = ad4851_scan_type_##real,			\
+	.num_ext_scan_type =						\
+		ARRAY_SIZE(ad4851_scan_type_##real),		\
+}
+
+static const struct iio_chan_spec ad4858_channels[] = {
+	AD4851_IIO_CHANNEL(0, 0, 20),
+	AD4851_IIO_CHANNEL(1, 0, 20),
+	AD4851_IIO_CHANNEL(2, 0, 20),
+	AD4851_IIO_CHANNEL(3, 0, 20),
+	AD4851_IIO_CHANNEL(4, 0, 20),
+	AD4851_IIO_CHANNEL(5, 0, 20),
+	AD4851_IIO_CHANNEL(6, 0, 20),
+	AD4851_IIO_CHANNEL(7, 0, 20),
+	AD4851_IIO_CHANNEL(0, 1, 20),
+	AD4851_IIO_CHANNEL(1, 1, 20),
+	AD4851_IIO_CHANNEL(2, 1, 20),
+	AD4851_IIO_CHANNEL(3, 1, 20),
+	AD4851_IIO_CHANNEL(4, 1, 20),
+	AD4851_IIO_CHANNEL(5, 1, 20),
+	AD4851_IIO_CHANNEL(6, 1, 20),
+	AD4851_IIO_CHANNEL(7, 1, 20),
+};
+
+static const struct iio_chan_spec ad4857_channels[] = {
+	AD4851_IIO_CHANNEL(0, 0, 16),
+	AD4851_IIO_CHANNEL(1, 0, 16),
+	AD4851_IIO_CHANNEL(2, 0, 16),
+	AD4851_IIO_CHANNEL(3, 0, 16),
+	AD4851_IIO_CHANNEL(4, 0, 16),
+	AD4851_IIO_CHANNEL(5, 0, 16),
+	AD4851_IIO_CHANNEL(6, 0, 16),
+	AD4851_IIO_CHANNEL(7, 0, 16),
+	AD4851_IIO_CHANNEL(0, 1, 16),
+	AD4851_IIO_CHANNEL(1, 1, 16),
+	AD4851_IIO_CHANNEL(2, 1, 16),
+	AD4851_IIO_CHANNEL(3, 1, 16),
+	AD4851_IIO_CHANNEL(4, 1, 16),
+	AD4851_IIO_CHANNEL(5, 1, 16),
+	AD4851_IIO_CHANNEL(6, 1, 16),
+	AD4851_IIO_CHANNEL(7, 1, 16),
+};
+
+static const struct ad4851_chip_info ad4851_info = {
+	.name = "ad4851",
+	.product_id = 0x67,
+	.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,
+	.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,
+	.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,
+	.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,
+	.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,
+	.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,
+	.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,
+	.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,
+	.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,
+	.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 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");
+
+	st->vrefbuf = devm_regulator_get_optional(&spi->dev, "vrefbuf");
+	if (IS_ERR(st->vrefbuf)) {
+		if (PTR_ERR(st->vrefbuf) != -ENODEV)
+			return dev_err_probe(&spi->dev, PTR_ERR(st->vrefbuf),
+					     "Failed to get vrefbuf regulator\n");
+	}
+
+	st->vrefio = devm_regulator_get_optional(&spi->dev, "vrefio");
+	if (IS_ERR(st->vrefio)) {
+		if (PTR_ERR(st->vrefio) != -ENODEV)
+			return dev_err_probe(&spi->dev, PTR_ERR(st->vrefio),
+					     "Failed to get vrefbuf regulator\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_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,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.0

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

[Jonathan Cameron]

On Fri, 1 Nov 2024 13:23:58 +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>
Hi Antoniu.

I only took a very quick look and one thing jumped out at me.

Jonathan

> diff --git a/drivers/iio/adc/ad4851.c b/drivers/iio/adc/ad4851.c
> new file mode 100644
> index 000000000000..0ef8ea0d2fc2
> --- /dev/null
> +++ b/drivers/iio/adc/ad4851.c

> +
> +static int ad4851_set_calibscale(struct ad4851_state *st, int ch, int val,
> +				 int val2)
> +{
> +	u64 gain;
> +	u8 buf[0];

A zero size array?

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

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

[David Lechner]

On 11/1/24 6:23 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>
> ---

This is looking better. But I think what we really need is a
through test plan to test all of the combinations of different
states since so many attributes interact with each other.

> changes in v5:
>  - use linux/unaligned.h
>  - drop redundant masks for reg_val
>  - use regmap_clear_bits/regmap_set_bits where applicable.
>  - add hard reset sequence via pd_gpio
>  - use scan_type_ext and implement resolution normal/boost.
>  - drop offset, implement differential channels and handle scale based on the
>    channel type.
>  drivers/iio/adc/Kconfig  |   13 +
>  drivers/iio/adc/Makefile |    1 +
>  drivers/iio/adc/ad4851.c | 1179 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 1193 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..0ef8ea0d2fc2
> --- /dev/null
> +++ b/drivers/iio/adc/ad4851.c
> @@ -0,0 +1,1179 @@
> +// 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
> +
> +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;

Looks like some leftover offset members here that are no longer used.

> +	const struct iio_chan_spec *channels;
> +	unsigned int num_channels;

Also looks like these two are unused.

> +	unsigned long throughput;
> +	unsigned int resolution;
> +};
> +
> +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)[2];
> +	int *offsets;

Also unused.

> +};
> +
> +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->throughput);
> +
> +	cnv_state.period = DIV_ROUND_DOWN_ULL(NSEC_PER_SEC, freq);

DIV_ROUND_UP_ULL

> +
> +	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;
> +	} else {
> +		ret = regmap_set_bits(st->regmap, AD4851_REG_OVERSAMPLE,
> +				      AD4851_OS_EN_MSK);
> +		if (ret)
> +			return ret;
> +
> +		val = ad4851_osr_to_regval(osr);
> +		if (val < 0)
> +			return -EINVAL;

We should check this before setting AD4851_REG_OVERSAMPLE, otherwise
we could end up in an invalid state if we return error here. And
we could combine the two register writes into a single call since
it is the same register.

> +
> +		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 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) {
> +		ret = regmap_update_bits(st->regmap, AD4851_REG_PACKET,
> +					 AD4851_PACKET_FORMAT_MASK, 0);

regmap_clear_bits()

> +		if (ret)
> +			return ret;
> +
> +		st->resolution_boost_enabled = false;
> +	} else {
> +		ret = regmap_update_bits(st->regmap, AD4851_REG_PACKET,
> +					 AD4851_PACKET_FORMAT_MASK, 1);

regmap_set_bits()

> +		if (ret)
> +			return ret;
> +
> +		st->resolution_boost_enabled = true;

Technically speaking, 16-bit chips don't have resolution boost. And
selecting PACKET_FORMAT = 1 here would enable extra status bits that
we aren't using. I don't think that is what we want.

> +	}
> +
> +	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 int ad4851_setup(struct ad4851_state *st)
> +{
> +	unsigned int product_id;
> +	int ret;
> +
> +	if (st->pd_gpio) {
> +		gpiod_set_value(st->pd_gpio, GPIOD_OUT_HIGH);
> +		fsleep(1);
> +		gpiod_set_value(st->pd_gpio, GPIOD_OUT_LOW);
> +		fsleep(1);
> +		gpiod_set_value(st->pd_gpio, GPIOD_OUT_HIGH);
> +		fsleep(1);
> +		gpiod_set_value(st->pd_gpio, GPIOD_OUT_LOW);

The GPIOD_OUT_* macros are not valid here. Just use 0 and 1.
Also, we can use gpiod_set_value_cansleep() here.

> +		fsleep(1000);
> +	}
> +
> +	if (!IS_ERR(st->vrefbuf)) {
> +		ret = regmap_update_bits(st->regmap, AD4851_REG_DEVICE_CTRL,
> +					 AD4851_REFBUF_PD, AD4851_REFBUF_PD);

Can be simplified to regmap_set_bits().

> +		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_update_bits(st->regmap, AD4851_REG_DEVICE_CTRL,
> +					 AD4851_REFSEL_PD, AD4851_REFSEL_PD);

Can be simplified to regmap_set_bits().


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

Probably need a time delay after reset. Also we should not need to
call this if we were able to reset using the PD gpio since the chip
was already reset.

Also, also, this needs to be moved before other register writes
above, otherwise it will reset those registers.

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

This one also needs to be done before any regmap reads (including
update_bits, set_bits, clear_bits that implicitly read).

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

Doing regmap_write here will set all other bits to 0, which will
clear previous config done above. Should be regmap_set_bits().

Other regmap_write() calls seem OK, but it's always nice to have a
comment explaining why it is OK, otherwise it looks suspicios, like
it could be hiding a bug like we have here.

> +	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, 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_SERIAL_CMOS:
> +		lane_num = st->info->num_channels;
> +		break;
> +	case IIO_BACKEND_INTERFACE_SERIAL_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 << 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[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]);
> +}
> +

I'm pretty sure that calibscale and calibbias also need to take into
account if resolution boost is enabled or not.

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

nit: a comment mentioning this is mapping voltage ranges to register
values would be helpful

> +static const unsigned int ad4851_scale_table[][2] = {
> +	{ 2500, 0x0 },
> +	{ 5000, 0x1 },
> +	{ 5000, 0x2 },
> +	{ 10000, 0x3 },
> +	{ 6250, 0x04 },

nit: drop the extra 0

> +	{ 12500, 0x5 },
> +	{ 10000, 0x6 },
> +	{ 20000, 0x7 },
> +	{ 12500, 0x8 },
> +	{ 25000, 0x9 },
> +	{ 20000, 0xA },
> +	{ 40000, 0xB },
> +	{ 25000, 0xC },
> +	{ 50000, 0xD },
> +	{ 40000, 0xE },
> +	{ 80000, 0xF },
> +};

I'm not sure how this table is supposed to work since there are
multiple entries with the same voltage value. Probably better
would be to just have the entries for the unipolar/unsigned ranges.
Then if applying this to a differential/signed channel, just add
1 to resulting register value before writing it to the register.
Or make two different tables, one for unsigned and one for signed
channels.

> +
> +static const int ad4857_offset_table[] = {
> +	0, -32768,
> +};
> +
> +static const int ad4858_offset_table[] = {
> +	0, -524288,
> +};

These are no longer used.

> +
> +static const unsigned int ad4851_scale_avail[] = {
> +	2500, 5000,
> +	10000, 6250,
> +	12500, 20000,
> +	25000, 40000,
> +	50000, 80000,
> +};

This should only go up to 40000. Or we need two different tables, one
for signed channels and one for unsigned channels. Although it would
probably be simpler to just multiply values from this table by 2 if
.differential = 1 rather than having a second table.

> +
> +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 channel type.
> +		 *
> +		 * If the channel is not differential then continue iterations
> +		 * until the next matching scale value which always corresponds
> +		 * to the single ended mode.
> +		 */
> +		if (!chan->differential && !single_ended) {
> +			single_ended = true;
> +			continue;
> +		}
> +
> +		return regmap_write(st->regmap,
> +				    AD4851_REG_CHX_SOFTSPAN(chan->channel),
> +				    ad4851_scale_table[i][1]);

Since we don't know if we are using single-ended or differential
until we actually read data, we should not be setting the softspan
register here. Instead, we should just save the selected scale
to st->scale. Then when we do a buffered read, we can set the
softspan correctly for each channel depending on which one is
enabled.

> +	}
> +
> +	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;
> +
> +	ret = regmap_read(st->regmap, AD4851_REG_CHX_SOFTSPAN(chan->channel),
> +			  &softspan_val);
> +	if (ret)
> +		return ret;

As above, we can just return the value save in st->scale instead of
reading the register.

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

If resolution boost is in effect because of oversampling, we need
to take that into account here as well.

> +
> +	return IIO_VAL_INT_PLUS_MICRO;
> +}
> +
> +static int ad4851_scale_fill(struct ad4851_state *st)
> +{
> +	unsigned int i, val1, val2;
> +
> +	st->scales = devm_kmalloc_array(&st->spi->dev, ARRAY_SIZE(ad4851_scale_avail),
> +					sizeof(*st->scales), GFP_KERNEL);
> +	if (!st->scales)
> +		return -ENOMEM;

It looks like this is a fixed size, so we could just include that
size in struct ad4851_state instead of doing a kmalloc here.

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

Maybe simpler to make st->scales a 1-dimintional array and just
store the index of the values in ad4851_scale_avail instead of
copying the values? 

> +
> +	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);
> +		return IIO_VAL_INT;

Unreachable return.

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

This is where we should also write the SoftSpan register to ensure
the correct unipolar or bipolar range is selected depending on
which channels are enabled.

Also, we will need to check here and return error if both the
single-ended and differential channel for any physical input
is enabled.

> +	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_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[] = {
> +	[AD4851_SCAN_TYPE_NORMAL] = {
> +		.sign = 's',
> +		.realbits = 16,
> +		.storagebits = 16,

Is storagebits really 16 here? The HDL engineers mentioned that on
other projects, they were trying to standardize on 32-bit storage
size everywhere, even when data is <= 16 bit.

> +	},
> +	[AD4851_SCAN_TYPE_RESOLUTION_BOOST] = {
> +		.sign = 's',
> +		.realbits = 16,
> +		.storagebits = 16,
> +	},
> +};

NORMAL and RESOLUTION_BOOST are the same on 16-bit chips, so we
don't actually need to implement ext_scan_type for those chips.

> +
> +static const struct iio_scan_type ad4851_scan_type_20[] = {
> +	[AD4851_SCAN_TYPE_NORMAL] = {
> +		.sign = 's',
> +		.realbits = 20,
> +		.storagebits = 32,
> +	},
> +	[AD4851_SCAN_TYPE_RESOLUTION_BOOST] = {
> +		.sign = 's',
> +		.realbits = 24,
> +		.storagebits = 32,
> +	},
> +};

The single-ended channels (differential = 0) have unsigned data, so we
will need different structs to handle that case.

> +
> +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, diff, real)				\

nit: "bits" would make more sense to me than "real"

> +{									\
> +	.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_type_available =				\
> +		BIT(IIO_CHAN_INFO_SCALE) |				\

Scale needs to be info_mask_separate_available to match IIO_CHAN_INFO_SCALE
flag on info_mask_separate.

> +		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),			\
> +	.indexed = 1,							\
> +	.differential = diff,						\
> +	.channel = index,						\
> +	.channel2 = index + diff * 8,					\
> +	.scan_index = index + diff * 8,					\
> +	.ext_scan_type = ad4851_scan_type_##real,			\
> +	.num_ext_scan_type =						\
> +		ARRAY_SIZE(ad4851_scan_type_##real),		\
> +}
> +
> +static const struct iio_chan_spec ad4858_channels[] = {
> +	AD4851_IIO_CHANNEL(0, 0, 20),
> +	AD4851_IIO_CHANNEL(1, 0, 20),
> +	AD4851_IIO_CHANNEL(2, 0, 20),
> +	AD4851_IIO_CHANNEL(3, 0, 20),
> +	AD4851_IIO_CHANNEL(4, 0, 20),
> +	AD4851_IIO_CHANNEL(5, 0, 20),
> +	AD4851_IIO_CHANNEL(6, 0, 20),
> +	AD4851_IIO_CHANNEL(7, 0, 20),
> +	AD4851_IIO_CHANNEL(0, 1, 20),
> +	AD4851_IIO_CHANNEL(1, 1, 20),
> +	AD4851_IIO_CHANNEL(2, 1, 20),
> +	AD4851_IIO_CHANNEL(3, 1, 20),
> +	AD4851_IIO_CHANNEL(4, 1, 20),
> +	AD4851_IIO_CHANNEL(5, 1, 20),
> +	AD4851_IIO_CHANNEL(6, 1, 20),
> +	AD4851_IIO_CHANNEL(7, 1, 20),
> +};
> +
> +static const struct iio_chan_spec ad4857_channels[] = {
> +	AD4851_IIO_CHANNEL(0, 0, 16),
> +	AD4851_IIO_CHANNEL(1, 0, 16),
> +	AD4851_IIO_CHANNEL(2, 0, 16),
> +	AD4851_IIO_CHANNEL(3, 0, 16),
> +	AD4851_IIO_CHANNEL(4, 0, 16),
> +	AD4851_IIO_CHANNEL(5, 0, 16),
> +	AD4851_IIO_CHANNEL(6, 0, 16),
> +	AD4851_IIO_CHANNEL(7, 0, 16),
> +	AD4851_IIO_CHANNEL(0, 1, 16),
> +	AD4851_IIO_CHANNEL(1, 1, 16),
> +	AD4851_IIO_CHANNEL(2, 1, 16),
> +	AD4851_IIO_CHANNEL(3, 1, 16),
> +	AD4851_IIO_CHANNEL(4, 1, 16),
> +	AD4851_IIO_CHANNEL(5, 1, 16),
> +	AD4851_IIO_CHANNEL(6, 1, 16),
> +	AD4851_IIO_CHANNEL(7, 1, 16),
> +};

I don't think it is valid for two channels to have the same scan_index.
And since this is simultaneous sampling and we don't have control over
the order in which the data is received from the backend, to get the
ordering correct, we will likely have to make this:

#define AD4851_IIO_CHANNEL(scan_index, channel, diff, bits) \
...

	AD4851_IIO_CHANNEL(0, 0, 0, 16),
	AD4851_IIO_CHANNEL(1, 0, 1, 16),
	AD4851_IIO_CHANNEL(2, 1, 0, 16),
	AD4851_IIO_CHANNEL(3, 1, 1, 16),
	AD4851_IIO_CHANNEL(4, 2, 0, 16),
	AD4851_IIO_CHANNEL(5, 2, 1, 16),
	AD4851_IIO_CHANNEL(6, 3, 0, 16),
	AD4851_IIO_CHANNEL(7, 3, 1, 16),
	AD4851_IIO_CHANNEL(8, 4, 0, 16),
	AD4851_IIO_CHANNEL(9, 4, 1, 16),
	AD4851_IIO_CHANNEL(10, 5, 0, 16),
	AD4851_IIO_CHANNEL(11, 5, 1, 16),
	AD4851_IIO_CHANNEL(12, 6, 0, 16),
	AD4851_IIO_CHANNEL(13, 6, 1, 16),
	AD4851_IIO_CHANNEL(14, 7, 0, 16),
	AD4851_IIO_CHANNEL(15, 7, 1, 16),


> +
> +static const struct ad4851_chip_info ad4851_info = {
> +	.name = "ad4851",
> +	.product_id = 0x67,
> +	.channels = ad4857_channels,
> +	.num_channels = ARRAY_SIZE(ad4857_channels),
> +	.throughput = 250 * KILO,

Is throughput the max sample rate? Better name would be max_sample_rate_hz.

> +	.resolution = 16,
> +};
> +
> +static const struct ad4851_chip_info ad4852_info = {
> +	.name = "ad4852",
> +	.product_id = 0x66,
> +	.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,
> +	.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,
> +	.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,
> +	.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,
> +	.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,
> +	.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,
> +	.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,
> +	.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,
> +	.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 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");
> +
> +	st->vrefbuf = devm_regulator_get_optional(&spi->dev, "vrefbuf");
> +	if (IS_ERR(st->vrefbuf)) {
> +		if (PTR_ERR(st->vrefbuf) != -ENODEV)
> +			return dev_err_probe(&spi->dev, PTR_ERR(st->vrefbuf),
> +					     "Failed to get vrefbuf regulator\n");
> +	}
> +
> +	st->vrefio = devm_regulator_get_optional(&spi->dev, "vrefio");
> +	if (IS_ERR(st->vrefio)) {
> +		if (PTR_ERR(st->vrefio) != -ENODEV)
> +			return dev_err_probe(&spi->dev, PTR_ERR(st->vrefio),
> +					     "Failed to get vrefbuf regulator\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");

We should also have a devm_add_action_and_reset() to call pwm_disable()
on driver remove. And to make it easier to see the full logic. we should
move ad4851_set_sampling_freq() from the ad4851_setup() here. (It should
come before registering the disable callback.) 

> +
> +	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_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,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",

nit: too many spaces betwee name and =

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

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

[Miclaus, Antoniu]

> > +	if (osr == 1) {
> > +		ret = regmap_update_bits(st->regmap, AD4851_REG_PACKET,
> > +					 AD4851_PACKET_FORMAT_MASK,
> 0);
> 
> regmap_clear_bits()
> 
> > +		if (ret)
> > +			return ret;
> > +
> > +		st->resolution_boost_enabled = false;
> > +	} else {
> > +		ret = regmap_update_bits(st->regmap, AD4851_REG_PACKET,
> > +					 AD4851_PACKET_FORMAT_MASK,
> 1);
> 
> regmap_set_bits()
Packet format is 2 bits wide. Not sure how can I write 1 if I use regmap set_bits
Should I do 2 separate masks?
> 
> > +		if (ret)
> > +			return ret;
> > +
> > +		st->resolution_boost_enabled = true;
> 
> Technically speaking, 16-bit chips don't have resolution boost. And
> selecting PACKET_FORMAT = 1 here would enable extra status bits that
> we aren't using. I don't think that is what we want.
> 
> > +	}
> > +
> > +	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 int ad4851_setup(struct ad4851_state *st)
> > +{
> > +	unsigned int product_id;
> > +	int ret;
> > +
> > +	if (st->pd_gpio) {
> > +		gpiod_set_value(st->pd_gpio, GPIOD_OUT_HIGH);
> > +		fsleep(1);
> > +		gpiod_set_value(st->pd_gpio, GPIOD_OUT_LOW);
> > +		fsleep(1);
> > +		gpiod_set_value(st->pd_gpio, GPIOD_OUT_HIGH);
> > +		fsleep(1);
> > +		gpiod_set_value(st->pd_gpio, GPIOD_OUT_LOW);
> 
> The GPIOD_OUT_* macros are not valid here. Just use 0 and 1.
> Also, we can use gpiod_set_value_cansleep() here.
> 
> > +		fsleep(1000);
> > +	}
> > +
> > +	if (!IS_ERR(st->vrefbuf)) {
> > +		ret = regmap_update_bits(st->regmap,
> AD4851_REG_DEVICE_CTRL,
> > +					 AD4851_REFBUF_PD,
> AD4851_REFBUF_PD);
> 
> Can be simplified to regmap_set_bits().
> 
> > +		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_update_bits(st->regmap,
> AD4851_REG_DEVICE_CTRL,
> > +					 AD4851_REFSEL_PD,
> AD4851_REFSEL_PD);
> 
> Can be simplified to regmap_set_bits().
> 
> 
> > +		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;
> > +	}
> > +
> > +	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;
> 
> Probably need a time delay after reset. Also we should not need to
> call this if we were able to reset using the PD gpio since the chip
> was already reset.
> 
> Also, also, this needs to be moved before other register writes
> above, otherwise it will reset those registers.
> 
> > +
> > +	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;
> 
> This one also needs to be done before any regmap reads (including
> update_bits, set_bits, clear_bits that implicitly read).
> 
> > +
> > +	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);
> 
> Doing regmap_write here will set all other bits to 0, which will
> clear previous config done above. Should be regmap_set_bits().
> 
> Other regmap_write() calls seem OK, but it's always nice to have a
> comment explaining why it is OK, otherwise it looks suspicios, like
> it could be hiding a bug like we have here.
> 
> > +	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, 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_SERIAL_CMOS:
> > +		lane_num = st->info->num_channels;
> > +		break;
> > +	case IIO_BACKEND_INTERFACE_SERIAL_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 << 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[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]);
> > +}
> > +
> 
> I'm pretty sure that calibscale and calibbias also need to take into
> account if resolution boost is enabled or not.

Can you please detail a bit on this topic? I am not sure what I should do.

> > +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]);
> > +}
> > +
> 
> nit: a comment mentioning this is mapping voltage ranges to register
> values would be helpful
> 
> > +static const unsigned int ad4851_scale_table[][2] = {
> > +	{ 2500, 0x0 },
> > +	{ 5000, 0x1 },
> > +	{ 5000, 0x2 },
> > +	{ 10000, 0x3 },
> > +	{ 6250, 0x04 },
> 
> nit: drop the extra 0
> 
> > +	{ 12500, 0x5 },
> > +	{ 10000, 0x6 },
> > +	{ 20000, 0x7 },
> > +	{ 12500, 0x8 },
> > +	{ 25000, 0x9 },
> > +	{ 20000, 0xA },
> > +	{ 40000, 0xB },
> > +	{ 25000, 0xC },
> > +	{ 50000, 0xD },
> > +	{ 40000, 0xE },
> > +	{ 80000, 0xF },
> > +};
> 
> I'm not sure how this table is supposed to work since there are
> multiple entries with the same voltage value. Probably better
> would be to just have the entries for the unipolar/unsigned ranges.
> Then if applying this to a differential/signed channel, just add
> 1 to resulting register value before writing it to the register.
> Or make two different tables, one for unsigned and one for signed
> channels.

It is stated in the set_scale function comment how this table works.
This table contains range-register value pair.
Always the second value corresponds to the single ended mode.
> 
> > +
> > +static const int ad4857_offset_table[] = {
> > +	0, -32768,
> > +};
> > +
> > +static const int ad4858_offset_table[] = {
> > +	0, -524288,
> > +};
> 
> These are no longer used.
> 
> > +
> > +static const unsigned int ad4851_scale_avail[] = {
> > +	2500, 5000,
> > +	10000, 6250,
> > +	12500, 20000,
> > +	25000, 40000,
> > +	50000, 80000,
> > +};
> 
> This should only go up to 40000. Or we need two different tables, one
> for signed channels and one for unsigned channels. Although it would
> probably be simpler to just multiply values from this table by 2 if
> .differential = 1 rather than having a second table.
> 
> > +
> > +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 channel type.
> > +		 *
> > +		 * If the channel is not differential then continue iterations
> > +		 * until the next matching scale value which always
> corresponds
> > +		 * to the single ended mode.
> > +		 */
> > +		if (!chan->differential && !single_ended) {
> > +			single_ended = true;
> > +			continue;
> > +		}
> > +
> > +		return regmap_write(st->regmap,
> > +				    AD4851_REG_CHX_SOFTSPAN(chan-
> >channel),
> > +				    ad4851_scale_table[i][1]);
> 
> Since we don't know if we are using single-ended or differential
> until we actually read data, we should not be setting the softspan
> register here. Instead, we should just save the selected scale
> to st->scale. Then when we do a buffered read, we can set the
> softspan correctly for each channel depending on which one is
> enabled.
> 
> > +	}
> > +
> > +	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;
> > +
> > +	ret = regmap_read(st->regmap, AD4851_REG_CHX_SOFTSPAN(chan-
> >channel),
> > +			  &softspan_val);
> > +	if (ret)
> > +		return ret;
> 
> As above, we can just return the value save in st->scale instead of
> reading the register.
> 
> > +
> > +	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);
> 
> If resolution boost is in effect because of oversampling, we need
> to take that into account here as well.
> 
> > +
> > +	return IIO_VAL_INT_PLUS_MICRO;
> > +}
> > +
> > +static int ad4851_scale_fill(struct ad4851_state *st)
> > +{
> > +	unsigned int i, val1, val2;
> > +
> > +	st->scales = devm_kmalloc_array(&st->spi->dev,
> ARRAY_SIZE(ad4851_scale_avail),
> > +					sizeof(*st->scales), GFP_KERNEL);
> > +	if (!st->scales)
> > +		return -ENOMEM;
> 
> It looks like this is a fixed size, so we could just include that
> size in struct ad4851_state instead of doing a kmalloc here.
> 
> > +
> > +	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;
> > +	}
> 
> Maybe simpler to make st->scales a 1-dimintional array and just
> store the index of the values in ad4851_scale_avail instead of
> copying the values?
> 
> > +
> > +	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);
> > +		return IIO_VAL_INT;
> 
> Unreachable return.
> 
> > +	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;
> > +
> 
> This is where we should also write the SoftSpan register to ensure
> the correct unipolar or bipolar range is selected depending on
> which channels are enabled.
> 
> Also, we will need to check here and return error if both the
> single-ended and differential channel for any physical input
> is enabled.
> 
> > +	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_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[] = {
> > +	[AD4851_SCAN_TYPE_NORMAL] = {
> > +		.sign = 's',
> > +		.realbits = 16,
> > +		.storagebits = 16,
> 
> Is storagebits really 16 here? The HDL engineers mentioned that on
> other projects, they were trying to standardize on 32-bit storage
> size everywhere, even when data is <= 16 bit.
> 
> > +	},
> > +	[AD4851_SCAN_TYPE_RESOLUTION_BOOST] = {
> > +		.sign = 's',
> > +		.realbits = 16,
> > +		.storagebits = 16,
> > +	},
> > +};
> 
> NORMAL and RESOLUTION_BOOST are the same on 16-bit chips, so we
> don't actually need to implement ext_scan_type for those chips.
> 
> > +
> > +static const struct iio_scan_type ad4851_scan_type_20[] = {
> > +	[AD4851_SCAN_TYPE_NORMAL] = {
> > +		.sign = 's',
> > +		.realbits = 20,
> > +		.storagebits = 32,
> > +	},
> > +	[AD4851_SCAN_TYPE_RESOLUTION_BOOST] = {
> > +		.sign = 's',
> > +		.realbits = 24,
> > +		.storagebits = 32,
> > +	},
> > +};
> 
> The single-ended channels (differential = 0) have unsigned data, so we
> will need different structs to handle that case.
> 
> > +
> > +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, diff, real)
> 	\
> 
> nit: "bits" would make more sense to me than "real"
> 
> > +{									\
> > +	.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_type_available =
> 	\
> > +		BIT(IIO_CHAN_INFO_SCALE) |				\
> 
> Scale needs to be info_mask_separate_available to match
> IIO_CHAN_INFO_SCALE
> flag on info_mask_separate.
> 
> > +		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
> 	\
> > +	.indexed = 1,							\
> > +	.differential = diff,						\
> > +	.channel = index,						\
> > +	.channel2 = index + diff * 8,					\
> > +	.scan_index = index + diff * 8,					\
> > +	.ext_scan_type = ad4851_scan_type_##real,			\
> > +	.num_ext_scan_type =						\
> > +		ARRAY_SIZE(ad4851_scan_type_##real),		\
> > +}
> > +
> > +static const struct iio_chan_spec ad4858_channels[] = {
> > +	AD4851_IIO_CHANNEL(0, 0, 20),
> > +	AD4851_IIO_CHANNEL(1, 0, 20),
> > +	AD4851_IIO_CHANNEL(2, 0, 20),
> > +	AD4851_IIO_CHANNEL(3, 0, 20),
> > +	AD4851_IIO_CHANNEL(4, 0, 20),
> > +	AD4851_IIO_CHANNEL(5, 0, 20),
> > +	AD4851_IIO_CHANNEL(6, 0, 20),
> > +	AD4851_IIO_CHANNEL(7, 0, 20),
> > +	AD4851_IIO_CHANNEL(0, 1, 20),
> > +	AD4851_IIO_CHANNEL(1, 1, 20),
> > +	AD4851_IIO_CHANNEL(2, 1, 20),
> > +	AD4851_IIO_CHANNEL(3, 1, 20),
> > +	AD4851_IIO_CHANNEL(4, 1, 20),
> > +	AD4851_IIO_CHANNEL(5, 1, 20),
> > +	AD4851_IIO_CHANNEL(6, 1, 20),
> > +	AD4851_IIO_CHANNEL(7, 1, 20),
> > +};
> > +
> > +static const struct iio_chan_spec ad4857_channels[] = {
> > +	AD4851_IIO_CHANNEL(0, 0, 16),
> > +	AD4851_IIO_CHANNEL(1, 0, 16),
> > +	AD4851_IIO_CHANNEL(2, 0, 16),
> > +	AD4851_IIO_CHANNEL(3, 0, 16),
> > +	AD4851_IIO_CHANNEL(4, 0, 16),
> > +	AD4851_IIO_CHANNEL(5, 0, 16),
> > +	AD4851_IIO_CHANNEL(6, 0, 16),
> > +	AD4851_IIO_CHANNEL(7, 0, 16),
> > +	AD4851_IIO_CHANNEL(0, 1, 16),
> > +	AD4851_IIO_CHANNEL(1, 1, 16),
> > +	AD4851_IIO_CHANNEL(2, 1, 16),
> > +	AD4851_IIO_CHANNEL(3, 1, 16),
> > +	AD4851_IIO_CHANNEL(4, 1, 16),
> > +	AD4851_IIO_CHANNEL(5, 1, 16),
> > +	AD4851_IIO_CHANNEL(6, 1, 16),
> > +	AD4851_IIO_CHANNEL(7, 1, 16),
> > +};
> 
> I don't think it is valid for two channels to have the same scan_index.
> And since this is simultaneous sampling and we don't have control over
> the order in which the data is received from the backend, to get the
> ordering correct, we will likely have to make this:
> 
I am not sure which of these channels have the same index.
scan_index is index + diff * 8 in the channel definition.

> #define AD4851_IIO_CHANNEL(scan_index, channel, diff, bits) \
> ...
> 
> 	AD4851_IIO_CHANNEL(0, 0, 0, 16),
> 	AD4851_IIO_CHANNEL(1, 0, 1, 16),
> 	AD4851_IIO_CHANNEL(2, 1, 0, 16),
> 	AD4851_IIO_CHANNEL(3, 1, 1, 16),
> 	AD4851_IIO_CHANNEL(4, 2, 0, 16),
> 	AD4851_IIO_CHANNEL(5, 2, 1, 16),
> 	AD4851_IIO_CHANNEL(6, 3, 0, 16),
> 	AD4851_IIO_CHANNEL(7, 3, 1, 16),
> 	AD4851_IIO_CHANNEL(8, 4, 0, 16),
> 	AD4851_IIO_CHANNEL(9, 4, 1, 16),
> 	AD4851_IIO_CHANNEL(10, 5, 0, 16),
> 	AD4851_IIO_CHANNEL(11, 5, 1, 16),
> 	AD4851_IIO_CHANNEL(12, 6, 0, 16),
> 	AD4851_IIO_CHANNEL(13, 6, 1, 16),
> 	AD4851_IIO_CHANNEL(14, 7, 0, 16),
> 	AD4851_IIO_CHANNEL(15, 7, 1, 16),
> 
> 
> > +
> > +static const struct ad4851_chip_info ad4851_info = {
> > +	.name = "ad4851",
> > +	.product_id = 0x67,
> > +	.channels = ad4857_channels,
> > +	.num_channels = ARRAY_SIZE(ad4857_channels),
> > +	.throughput = 250 * KILO,

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

[David Lechner]

On 11/7/24 4:51 AM, Miclaus, Antoniu wrote:
>>> +	if (osr == 1) {
>>> +		ret = regmap_update_bits(st->regmap, AD4851_REG_PACKET,
>>> +					 AD4851_PACKET_FORMAT_MASK,
>> 0);
>>
>> regmap_clear_bits()
>>
>>> +		if (ret)
>>> +			return ret;
>>> +
>>> +		st->resolution_boost_enabled = false;
>>> +	} else {
>>> +		ret = regmap_update_bits(st->regmap, AD4851_REG_PACKET,
>>> +					 AD4851_PACKET_FORMAT_MASK,
>> 1);
>>
>> regmap_set_bits()
> Packet format is 2 bits wide. Not sure how can I write 1 if I use regmap set_bits
> Should I do 2 separate masks?

Sorry, I missed that detail. In that case, using FIELD_PREP() here would
make that clear (even if it isn't technically required).


>>> +static int ad4851_set_calibscale(struct ad4851_state *st, int ch, int val,
>>> +				 int val2)
>>> +{
>>> +	u64 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]);
>>> +}
>>> +
>>
>> I'm pretty sure that calibscale and calibbias also need to take into
>> account if resolution boost is enabled or not.
> 
> Can you please detail a bit on this topic? I am not sure what I should do.
> 

We haven't implemented oversampling yet in ad4695 yet, so I don't know
exactly what we need to do either. ;-)

But this is how I would test it to see if it is working correctly or
not. We will need to test this with a 20-bit chip since that is the
only one that will change the _scale attribute when oversampling is
enabled.

First, with oversampling disabled (_oversampling_ratio = 1), generate
a constant voltage of 1V for the input. Read the _raw attribute. Let's
call this value raw0. Read the _scale attribute, call it scale0 and
the _offset attribute, call it offset0.

Then we should have (raw0 + offset0) * scale0 = 1000 mV (+/- some
noise).

Then change the offset calibrate to 100 mV. To do this, we reverse
the calculation 100 mV / scale0 = calibbias (raw units). Write the
raw value to the _calibbias attribute. Then read the _raw
attribute again, call it raw0_with_calibbias.

This time, we should have (raw0_with_calibbias + offset0) * scale0
= 1100 mV (+/- some noise).

Then set _calibbias back to 0 and repeat the above by setting the
_calibscale attribute to 0.90909 (this is 1 / 1.1, which should
add 10% to the measured raw value). Read, the _raw attribute again,
call it raw0_with_caliscale.

This time, we should have (raw0_with_caliscale + offset0) * scale0
= 1100 mV (+/- some noise).

Set _calibscale back to 0. Then set _oversampling_ratio to 2. Read
_scale and _offset again, call these scale1 and offset1.

Then repeat the steps above using scale1 and offset1 in the
calculations. The raw values will be different but the resulting
processed values (mV) should all be the same if the attributes
are implemented correctly.

>>> +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 },
>>> +};
>>
>> I'm not sure how this table is supposed to work since there are
>> multiple entries with the same voltage value. Probably better
>> would be to just have the entries for the unipolar/unsigned ranges.
>> Then if applying this to a differential/signed channel, just add
>> 1 to resulting register value before writing it to the register.
>> Or make two different tables, one for unsigned and one for signed
>> channels.
> 
> It is stated in the set_scale function comment how this table works.
> This table contains range-register value pair.
> Always the second value corresponds to the single ended mode.
>>

Yes, I understand that part. The problem is that values like 10000
are listed twice in the table, so if we have a softspan of 0..+10V
or -10V..+10V, how do we know which 10000 to use to get the right
register value? This is why I think it needs to be 2 different
tables.

>>> +
>>> +static const struct iio_chan_spec ad4858_channels[] = {
>>> +	AD4851_IIO_CHANNEL(0, 0, 20),
>>> +	AD4851_IIO_CHANNEL(1, 0, 20),
>>> +	AD4851_IIO_CHANNEL(2, 0, 20),
>>> +	AD4851_IIO_CHANNEL(3, 0, 20),
>>> +	AD4851_IIO_CHANNEL(4, 0, 20),
>>> +	AD4851_IIO_CHANNEL(5, 0, 20),
>>> +	AD4851_IIO_CHANNEL(6, 0, 20),
>>> +	AD4851_IIO_CHANNEL(7, 0, 20),
>>> +	AD4851_IIO_CHANNEL(0, 1, 20),
>>> +	AD4851_IIO_CHANNEL(1, 1, 20),
>>> +	AD4851_IIO_CHANNEL(2, 1, 20),
>>> +	AD4851_IIO_CHANNEL(3, 1, 20),
>>> +	AD4851_IIO_CHANNEL(4, 1, 20),
>>> +	AD4851_IIO_CHANNEL(5, 1, 20),
>>> +	AD4851_IIO_CHANNEL(6, 1, 20),
>>> +	AD4851_IIO_CHANNEL(7, 1, 20),
>>> +};
>>> +
>>> +static const struct iio_chan_spec ad4857_channels[] = {
>>> +	AD4851_IIO_CHANNEL(0, 0, 16),
>>> +	AD4851_IIO_CHANNEL(1, 0, 16),
>>> +	AD4851_IIO_CHANNEL(2, 0, 16),
>>> +	AD4851_IIO_CHANNEL(3, 0, 16),
>>> +	AD4851_IIO_CHANNEL(4, 0, 16),
>>> +	AD4851_IIO_CHANNEL(5, 0, 16),
>>> +	AD4851_IIO_CHANNEL(6, 0, 16),
>>> +	AD4851_IIO_CHANNEL(7, 0, 16),
>>> +	AD4851_IIO_CHANNEL(0, 1, 16),
>>> +	AD4851_IIO_CHANNEL(1, 1, 16),
>>> +	AD4851_IIO_CHANNEL(2, 1, 16),
>>> +	AD4851_IIO_CHANNEL(3, 1, 16),
>>> +	AD4851_IIO_CHANNEL(4, 1, 16),
>>> +	AD4851_IIO_CHANNEL(5, 1, 16),
>>> +	AD4851_IIO_CHANNEL(6, 1, 16),
>>> +	AD4851_IIO_CHANNEL(7, 1, 16),
>>> +};
>>
>> I don't think it is valid for two channels to have the same scan_index.
>> And since this is simultaneous sampling and we don't have control over
>> the order in which the data is received from the backend, to get the
>> ordering correct, we will likely have to make this:
>>
> I am not sure which of these channels have the same index.
> scan_index is index + diff * 8 in the channel definition.
> 

scan_index indicates the order in which a data value for a channel
will appear in the buffer when doing a buffered read. So all scan_index
for any channel 0 need to be less than all scan_index for all
channel 1, and so on.

So in the suggestion quoted below, the scan_index parameter
just gets assigned directly to .scan_index without any
additional calculations.

>> #define AD4851_IIO_CHANNEL(scan_index, channel, diff, bits) \
>> ...
>>
>> 	AD4851_IIO_CHANNEL(0, 0, 0, 16),
>> 	AD4851_IIO_CHANNEL(1, 0, 1, 16),
>> 	AD4851_IIO_CHANNEL(2, 1, 0, 16),
>> 	AD4851_IIO_CHANNEL(3, 1, 1, 16),
>> 	AD4851_IIO_CHANNEL(4, 2, 0, 16),
>> 	AD4851_IIO_CHANNEL(5, 2, 1, 16),
>> 	AD4851_IIO_CHANNEL(6, 3, 0, 16),
>> 	AD4851_IIO_CHANNEL(7, 3, 1, 16),
>> 	AD4851_IIO_CHANNEL(8, 4, 0, 16),
>> 	AD4851_IIO_CHANNEL(9, 4, 1, 16),
>> 	AD4851_IIO_CHANNEL(10, 5, 0, 16),
>> 	AD4851_IIO_CHANNEL(11, 5, 1, 16),
>> 	AD4851_IIO_CHANNEL(12, 6, 0, 16),
>> 	AD4851_IIO_CHANNEL(13, 6, 1, 16),
>> 	AD4851_IIO_CHANNEL(14, 7, 0, 16),
>> 	AD4851_IIO_CHANNEL(15, 7, 1, 16),
>>
>>

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

[David Lechner]

On 11/7/24 10:13 AM, David Lechner wrote:
> On 11/7/24 4:51 AM, Miclaus, Antoniu wrote:


>>> I'm pretty sure that calibscale and calibbias also need to take into
>>> account if resolution boost is enabled or not.
>>
>> Can you please detail a bit on this topic? I am not sure what I should do.
>>
> 
> We haven't implemented oversampling yet in ad4695 yet, so I don't know
> exactly what we need to do either. ;-)
> 
> But this is how I would test it to see if it is working correctly or
> not. We will need to test this with a 20-bit chip since that is the
> only one that will change the _scale attribute when oversampling is
> enabled.
> 
> First, with oversampling disabled (_oversampling_ratio = 1), generate
> a constant voltage of 1V for the input. Read the _raw attribute. Let's
> call this value raw0. Read the _scale attribute, call it scale0 and
> the _offset attribute, call it offset0.
> 
> Then we should have (raw0 + offset0) * scale0 = 1000 mV (+/- some
> noise).
> 
> Then change the offset calibrate to 100 mV. To do this, we reverse
> the calculation 100 mV / scale0 = calibbias (raw units). Write the
> raw value to the _calibbias attribute. Then read the _raw
> attribute again, call it raw0_with_calibbias.
> 
> This time, we should have (raw0_with_calibbias + offset0) * scale0
> = 1100 mV (+/- some noise).
> 
> Then set _calibbias back to 0 and repeat the above by setting the
> _calibscale attribute to 0.90909 (this is 1 / 1.1, which should

Now that I have written this, this has me second-guessing if I
implemented calibscale correctly on ad4695. It would seem more
logical that if we have an actual input voltage of 1 V and a
calibscale of 1.1, then the resulting processed value we read
should be 1100 mV.

Jonathan, can you set me straight? The sysfs ABI docs aren't
clear on this point.

> add 10% to the measured raw value). Read, the _raw attribute again,
> call it raw0_with_caliscale.
> 
> This time, we should have (raw0_with_caliscale + offset0) * scale0
> = 1100 mV (+/- some noise).
> 
> Set _calibscale back to 0. Then set _oversampling_ratio to 2. Read
> _scale and _offset again, call these scale1 and offset1.
> 
> Then repeat the steps above using scale1 and offset1 in the
> calculations. The raw values will be different but the resulting
> processed values (mV) should all be the same if the attributes
> are implemented correctly.
> 

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

[Jonathan Cameron]

On Thu, 7 Nov 2024 10:47:52 -0600
David Lechner <dlechner@baylibre.com> wrote:

> On 11/7/24 10:13 AM, David Lechner wrote:
> > On 11/7/24 4:51 AM, Miclaus, Antoniu wrote:  
> 
> 
> >>> I'm pretty sure that calibscale and calibbias also need to take into
> >>> account if resolution boost is enabled or not.  
> >>
> >> Can you please detail a bit on this topic? I am not sure what I should do.
> >>  
> > 
> > We haven't implemented oversampling yet in ad4695 yet, so I don't know
> > exactly what we need to do either. ;-)
> > 
> > But this is how I would test it to see if it is working correctly or
> > not. We will need to test this with a 20-bit chip since that is the
> > only one that will change the _scale attribute when oversampling is
> > enabled.
> > 
> > First, with oversampling disabled (_oversampling_ratio = 1), generate
> > a constant voltage of 1V for the input. Read the _raw attribute. Let's
> > call this value raw0. Read the _scale attribute, call it scale0 and
> > the _offset attribute, call it offset0.
> > 
> > Then we should have (raw0 + offset0) * scale0 = 1000 mV (+/- some
> > noise).
> > 
> > Then change the offset calibrate to 100 mV. To do this, we reverse
> > the calculation 100 mV / scale0 = calibbias (raw units). Write the
> > raw value to the _calibbias attribute. Then read the _raw
> > attribute again, call it raw0_with_calibbias.
> > 
> > This time, we should have (raw0_with_calibbias + offset0) * scale0
> > = 1100 mV (+/- some noise).
> > 
> > Then set _calibbias back to 0 and repeat the above by setting the
> > _calibscale attribute to 0.90909 (this is 1 / 1.1, which should  
> 
> Now that I have written this, this has me second-guessing if I
> implemented calibscale correctly on ad4695. It would seem more
> logical that if we have an actual input voltage of 1 V and a
> calibscale of 1.1, then the resulting processed value we read
> should be 1100 mV.
> 
> Jonathan, can you set me straight? The sysfs ABI docs aren't
> clear on this point.

Deliberately vague in this case.  calibbias is kind of the wild west
of ABI. Often we have no meaningful information on what the tweak
register settings actually do beyond 'up vs down'.  In some cases
the datasheets even refer to them as taps up or taps down.

I don't think we've ever said if it should be consistent as you
change other parameters.  If you care about calibration you probably
need to redo it for your new settings anyway and tweak the calibbias
/calibscale till it gives the right values.

Obviously that is easier to do if you have a consistent scheme for
a given device and if possible allow calibrating at just one setting
but I don't think we can apply general rules.

Jonathan

> 
> > add 10% to the measured raw value). Read, the _raw attribute again,
> > call it raw0_with_caliscale.
> > 
> > This time, we should have (raw0_with_caliscale + offset0) * scale0
> > = 1100 mV (+/- some noise).
> > 
> > Set _calibscale back to 0. Then set _oversampling_ratio to 2. Read
> > _scale and _offset again, call these scale1 and offset1.
> > 
> > Then repeat the steps above using scale1 and offset1 in the
> > calculations. The raw values will be different but the resulting
> > processed values (mV) should all be the same if the attributes
> > are implemented correctly.
> >   

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

[David Lechner]

On 11/9/24 9:39 AM, Jonathan Cameron wrote:
> On Thu, 7 Nov 2024 10:47:52 -0600
> David Lechner <dlechner@baylibre.com> wrote:
> 
>> On 11/7/24 10:13 AM, David Lechner wrote:
>>> On 11/7/24 4:51 AM, Miclaus, Antoniu wrote:  
>>
>>
>>>>> I'm pretty sure that calibscale and calibbias also need to take into
>>>>> account if resolution boost is enabled or not.  
>>>>
>>>> Can you please detail a bit on this topic? I am not sure what I should do.
>>>>  
>>>
>>> We haven't implemented oversampling yet in ad4695 yet, so I don't know
>>> exactly what we need to do either. ;-)
>>>
>>> But this is how I would test it to see if it is working correctly or
>>> not. We will need to test this with a 20-bit chip since that is the
>>> only one that will change the _scale attribute when oversampling is
>>> enabled.
>>>
>>> First, with oversampling disabled (_oversampling_ratio = 1), generate
>>> a constant voltage of 1V for the input. Read the _raw attribute. Let's
>>> call this value raw0. Read the _scale attribute, call it scale0 and
>>> the _offset attribute, call it offset0.
>>>
>>> Then we should have (raw0 + offset0) * scale0 = 1000 mV (+/- some
>>> noise).
>>>
>>> Then change the offset calibrate to 100 mV. To do this, we reverse
>>> the calculation 100 mV / scale0 = calibbias (raw units). Write the
>>> raw value to the _calibbias attribute. Then read the _raw
>>> attribute again, call it raw0_with_calibbias.
>>>
>>> This time, we should have (raw0_with_calibbias + offset0) * scale0
>>> = 1100 mV (+/- some noise).
>>>
>>> Then set _calibbias back to 0 and repeat the above by setting the
>>> _calibscale attribute to 0.90909 (this is 1 / 1.1, which should  
>>

After a bit more testing, I realized I was testing with a
differential channel, this math only applies to that.

For a single ended channel, applying a calibscale of 1.1 with
a generated signal of 1V will cause the measured value to change
from 1V to 1.1V as one might expect.


>> Now that I have written this, this has me second-guessing if I
>> implemented calibscale correctly on ad4695. It would seem more
>> logical that if we have an actual input voltage of 1 V and a
>> calibscale of 1.1, then the resulting processed value we read
>> should be 1100 mV.
>>
>> Jonathan, can you set me straight? The sysfs ABI docs aren't
>> clear on this point.
> 
> Deliberately vague in this case.  calibbias is kind of the wild west
> of ABI. Often we have no meaningful information on what the tweak
> register settings actually do beyond 'up vs down'.  In some cases
> the datasheets even refer to them as taps up or taps down.
> 
> I don't think we've ever said if it should be consistent as you
> change other parameters.  If you care about calibration you probably
> need to redo it for your new settings anyway and tweak the calibbias
> /calibscale till it gives the right values.
> 
> Obviously that is easier to do if you have a consistent scheme for
> a given device and if possible allow calibrating at just one setting
> but I don't think we can apply general rules.
> 

Thanks for the clarification.

> Jonathan
> 
>>
>>> add 10% to the measured raw value). Read, the _raw attribute again,
>>> call it raw0_with_caliscale.
>>>
>>> This time, we should have (raw0_with_caliscale + offset0) * scale0
>>> = 1100 mV (+/- some noise).
>>>
>>> Set _calibscale back to 0. Then set _oversampling_ratio to 2. Read
>>> _scale and _offset again, call these scale1 and offset1.
>>>
>>> Then repeat the steps above using scale1 and offset1 in the
>>> calculations. The raw values will be different but the resulting
>>> processed values (mV) should all be the same if the attributes
>>> are implemented correctly.
>>>   
> 

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

[Miclaus, Antoniu]

--
Antoniu Miclăuş

> -----Original Message-----
> From: David Lechner <dlechner@baylibre.com>
> Sent: Thursday, November 7, 2024 6:14 PM
> To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>; jic23@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 v5 6/6] iio: adc: ad4851: add ad485x driver
> 
> [External]
> 
> On 11/7/24 4:51 AM, Miclaus, Antoniu wrote:
> >>> +	if (osr == 1) {
> >>> +		ret = regmap_update_bits(st->regmap, AD4851_REG_PACKET,
> >>> +					 AD4851_PACKET_FORMAT_MASK,
> >> 0);
> >>
> >> regmap_clear_bits()
> >>
> >>> +		if (ret)
> >>> +			return ret;
> >>> +
> >>> +		st->resolution_boost_enabled = false;
> >>> +	} else {
> >>> +		ret = regmap_update_bits(st->regmap, AD4851_REG_PACKET,
> >>> +					 AD4851_PACKET_FORMAT_MASK,
> >> 1);
> >>
> >> regmap_set_bits()
> > Packet format is 2 bits wide. Not sure how can I write 1 if I use regmap
> set_bits
> > Should I do 2 separate masks?
> 
> Sorry, I missed that detail. In that case, using FIELD_PREP() here would
> make that clear (even if it isn't technically required).
> 
Sure I will do FIELD_PREP.
> 
> >>> +static int ad4851_set_calibscale(struct ad4851_state *st, int ch, int val,
> >>> +				 int val2)
> >>> +{
> >>> +	u64 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]);
> >>> +}
> >>> +
> >>
> >> I'm pretty sure that calibscale and calibbias also need to take into
> >> account if resolution boost is enabled or not.
> >
> > Can you please detail a bit on this topic? I am not sure what I should do.
> >
> 
> We haven't implemented oversampling yet in ad4695 yet, so I don't know
> exactly what we need to do either. ;-)
> 
> But this is how I would test it to see if it is working correctly or
> not. We will need to test this with a 20-bit chip since that is the
> only one that will change the _scale attribute when oversampling is
> enabled.
> 
> First, with oversampling disabled (_oversampling_ratio = 1), generate
> a constant voltage of 1V for the input. Read the _raw attribute. Let's
> call this value raw0. Read the _scale attribute, call it scale0 and
> the _offset attribute, call it offset0.
> 
> Then we should have (raw0 + offset0) * scale0 = 1000 mV (+/- some
> noise).
> 
> Then change the offset calibrate to 100 mV. To do this, we reverse
> the calculation 100 mV / scale0 = calibbias (raw units). Write the
> raw value to the _calibbias attribute. Then read the _raw
> attribute again, call it raw0_with_calibbias.
> 
> This time, we should have (raw0_with_calibbias + offset0) * scale0
> = 1100 mV (+/- some noise).
> 
> Then set _calibbias back to 0 and repeat the above by setting the
> _calibscale attribute to 0.90909 (this is 1 / 1.1, which should
> add 10% to the measured raw value). Read, the _raw attribute again,
> call it raw0_with_caliscale.
> 
> This time, we should have (raw0_with_caliscale + offset0) * scale0
> = 1100 mV (+/- some noise).
> 
> Set _calibscale back to 0. Then set _oversampling_ratio to 2. Read
> _scale and _offset again, call these scale1 and offset1.
> 
> Then repeat the steps above using scale1 and offset1 in the
> calculations. The raw values will be different but the resulting
> processed values (mV) should all be the same if the attributes
> are implemented correctly.
> 
> >>> +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 },
> >>> +};
> >>
> >> I'm not sure how this table is supposed to work since there are
> >> multiple entries with the same voltage value. Probably better
> >> would be to just have the entries for the unipolar/unsigned ranges.
> >> Then if applying this to a differential/signed channel, just add
> >> 1 to resulting register value before writing it to the register.
> >> Or make two different tables, one for unsigned and one for signed
> >> channels.
> >
> > It is stated in the set_scale function comment how this table works.
> > This table contains range-register value pair.
> > Always the second value corresponds to the single ended mode.
> >>
> 
> Yes, I understand that part. The problem is that values like 10000
> are listed twice in the table, so if we have a softspan of 0..+10V
> or -10V..+10V, how do we know which 10000 to use to get the right
> register value? This is why I think it needs to be 2 different
> tables.
Yes there are values listed twice, but there is a rule that is handled by
the set/get scale functions.
/*
* Adjust the softspan value (differential or single ended)
* based on the scale value selected channel type.
*
* If the channel is not differential then continue iterations
* until the next matching scale value which always corresponds
* to the single ended mode.
*/

I was already asked in previous patches to detail the comment in the 
set_scale function so I did it.

> >>> +
> >>> +static const struct iio_chan_spec ad4858_channels[] = {
> >>> +	AD4851_IIO_CHANNEL(0, 0, 20),
> >>> +	AD4851_IIO_CHANNEL(1, 0, 20),
> >>> +	AD4851_IIO_CHANNEL(2, 0, 20),
> >>> +	AD4851_IIO_CHANNEL(3, 0, 20),
> >>> +	AD4851_IIO_CHANNEL(4, 0, 20),
> >>> +	AD4851_IIO_CHANNEL(5, 0, 20),
> >>> +	AD4851_IIO_CHANNEL(6, 0, 20),
> >>> +	AD4851_IIO_CHANNEL(7, 0, 20),
> >>> +	AD4851_IIO_CHANNEL(0, 1, 20),
> >>> +	AD4851_IIO_CHANNEL(1, 1, 20),
> >>> +	AD4851_IIO_CHANNEL(2, 1, 20),
> >>> +	AD4851_IIO_CHANNEL(3, 1, 20),
> >>> +	AD4851_IIO_CHANNEL(4, 1, 20),
> >>> +	AD4851_IIO_CHANNEL(5, 1, 20),
> >>> +	AD4851_IIO_CHANNEL(6, 1, 20),
> >>> +	AD4851_IIO_CHANNEL(7, 1, 20),
> >>> +};
> >>> +
> >>> +static const struct iio_chan_spec ad4857_channels[] = {
> >>> +	AD4851_IIO_CHANNEL(0, 0, 16),
> >>> +	AD4851_IIO_CHANNEL(1, 0, 16),
> >>> +	AD4851_IIO_CHANNEL(2, 0, 16),
> >>> +	AD4851_IIO_CHANNEL(3, 0, 16),
> >>> +	AD4851_IIO_CHANNEL(4, 0, 16),
> >>> +	AD4851_IIO_CHANNEL(5, 0, 16),
> >>> +	AD4851_IIO_CHANNEL(6, 0, 16),
> >>> +	AD4851_IIO_CHANNEL(7, 0, 16),
> >>> +	AD4851_IIO_CHANNEL(0, 1, 16),
> >>> +	AD4851_IIO_CHANNEL(1, 1, 16),
> >>> +	AD4851_IIO_CHANNEL(2, 1, 16),
> >>> +	AD4851_IIO_CHANNEL(3, 1, 16),
> >>> +	AD4851_IIO_CHANNEL(4, 1, 16),
> >>> +	AD4851_IIO_CHANNEL(5, 1, 16),
> >>> +	AD4851_IIO_CHANNEL(6, 1, 16),
> >>> +	AD4851_IIO_CHANNEL(7, 1, 16),
> >>> +};
> >>
> >> I don't think it is valid for two channels to have the same scan_index.
> >> And since this is simultaneous sampling and we don't have control over
> >> the order in which the data is received from the backend, to get the
> >> ordering correct, we will likely have to make this:
> >>
> > I am not sure which of these channels have the same index.
> > scan_index is index + diff * 8 in the channel definition.
> >
> 
> scan_index indicates the order in which a data value for a channel
> will appear in the buffer when doing a buffered read. So all scan_index
> for any channel 0 need to be less than all scan_index for all
> channel 1, and so on.
> 
> So in the suggestion quoted below, the scan_index parameter
> just gets assigned directly to .scan_index without any
> additional calculations.
> 
> >> #define AD4851_IIO_CHANNEL(scan_index, channel, diff, bits) \
> >> ...
> >>
> >> 	AD4851_IIO_CHANNEL(0, 0, 0, 16),
> >> 	AD4851_IIO_CHANNEL(1, 0, 1, 16),
> >> 	AD4851_IIO_CHANNEL(2, 1, 0, 16),
> >> 	AD4851_IIO_CHANNEL(3, 1, 1, 16),
> >> 	AD4851_IIO_CHANNEL(4, 2, 0, 16),
> >> 	AD4851_IIO_CHANNEL(5, 2, 1, 16),
> >> 	AD4851_IIO_CHANNEL(6, 3, 0, 16),
> >> 	AD4851_IIO_CHANNEL(7, 3, 1, 16),
> >> 	AD4851_IIO_CHANNEL(8, 4, 0, 16),
> >> 	AD4851_IIO_CHANNEL(9, 4, 1, 16),
> >> 	AD4851_IIO_CHANNEL(10, 5, 0, 16),
> >> 	AD4851_IIO_CHANNEL(11, 5, 1, 16),
> >> 	AD4851_IIO_CHANNEL(12, 6, 0, 16),
> >> 	AD4851_IIO_CHANNEL(13, 6, 1, 16),
> >> 	AD4851_IIO_CHANNEL(14, 7, 0, 16),
> >> 	AD4851_IIO_CHANNEL(15, 7, 1, 16),
> >>
> >>

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

[Jonathan Cameron]

On Fri, 1 Nov 2024 13:23:58 +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>

Hi Antoniu.

Various things inline to add to David's much more thorough review.

Jonathan

> +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)[2];

As described below consider clearer type definition by using
a structure.

> +	int *offsets;
> +};
>
> +static int ad4851_setup(struct ad4851_state *st)
> +{
> +	unsigned int product_id;
> +	int ret;
> +
> +	if (st->pd_gpio) {

Not obvious why you are pulsing twice. Add a comment.

> +		gpiod_set_value(st->pd_gpio, GPIOD_OUT_HIGH);
> +		fsleep(1);
> +		gpiod_set_value(st->pd_gpio, GPIOD_OUT_LOW);
> +		fsleep(1);
> +		gpiod_set_value(st->pd_gpio, GPIOD_OUT_HIGH);
> +		fsleep(1);
> +		gpiod_set_value(st->pd_gpio, GPIOD_OUT_LOW);
> +		fsleep(1000);
> +	}
...

> +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_SERIAL_CMOS:
> +		lane_num = st->info->num_channels;

num_lanes is probably a better name as lane_num sounds like the number
of a particular lane rather than now many there are.

> +		break;
> +	case IIO_BACKEND_INTERFACE_SERIAL_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;

Blank line here is more consistent with surounding code (which looks better
than this block does).

> +			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_set_calibscale(struct ad4851_state *st, int ch, int val,
> +				 int val2)
> +{
> +	u64 gain;
> +	u8 buf[0];

As per my very brief review the other day, 0 length is a problem.

> +	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]);

Don't wrap so early. It fits under 80 chars on oneline.

> +	if (ret)
> +		return ret;
> +
> +	return regmap_write(st->regmap, AD4851_REG_CHX_GAIN_LSB(ch),
> +			    buf[1]);
> +}

> +
> +static const unsigned int ad4851_scale_table[][2] = {
I'd suggest a structure for each entry. Then you can do
ad4851_scale_table[i].reg_val etc
rather than opaque indexing to 0 or 1.

> +	{ 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 int ad4851_get_scale(struct ad4851_state *st,
> +			    const struct iio_chan_spec *chan, int *val,
> +			    int *val2)
> +{
> +	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])

See above - I'd use a structure for the scale_table entries
so this and the [0] below can be named structure elements.

> +			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_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;

struct device *dev = *spi->dev;
will shorten a lot of code in here so probably worth doing.

> +	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");
You aren't trying to get the voltage, just turn it on.  So update the commit
message to "failed to enable vddh\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");
Same here.
> +
> +	st->vrefbuf = devm_regulator_get_optional(&spi->dev, "vrefbuf");
> +	if (IS_ERR(st->vrefbuf)) {
> +		if (PTR_ERR(st->vrefbuf) != -ENODEV)
> +			return dev_err_probe(&spi->dev, PTR_ERR(st->vrefbuf),
> +					     "Failed to get vrefbuf regulator\n");
> +	}
> +
> +	st->vrefio = devm_regulator_get_optional(&spi->dev, "vrefio");
> +	if (IS_ERR(st->vrefio)) {
> +		if (PTR_ERR(st->vrefio) != -ENODEV)
> +			return dev_err_probe(&spi->dev, PTR_ERR(st->vrefio),
> +					     "Failed to get vrefbuf regulator\n");

wrong regulator in the message.

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

Whilst it's not actively used, current best practice is to set
indio_dev->modes = INDIO_DIRECT_MODE;

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

Re: [PATCH 0/7] *** Add support for AD485x DAS Family ***

[Jonathan Cameron]

On Fri, 1 Nov 2024 13:23:52 +0200
Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:

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

For future cases. The cover letter should have the version numbers as well as the
patches.

Also, the cover letter title should be similar to the patches.
[PATCH V5 0/7] iio: adc: Add ad485x driver.

How did we get this far with wild cards in file names?

In IIO we always name after a particular part (there are a few historical drivers
that don't).  Using wild cards goes wrong far too often. So rename the file as
ad4841.c + same for the dt binding file and documentation file.


Jonathan

> 
> Antoniu Miclaus (7):
>   iio: backend: add API for interface get
>   iio: backend: add support for data size set
>   iio: adc: adi-axi-adc: add interface type
>   iio: adc: adi-axi-adc: set data format
>   dt-bindings: iio: adc: add ad458x
>   iio: adc: ad485x: add ad485x driver
>   Documentation: ABI: testing: ad485x: add ABI docs
> 
>  .../ABI/testing/sysfs-bus-iio-adc-ad485x      |   14 +
>  .../bindings/iio/adc/adi,ad485x.yaml          |   82 ++
>  drivers/iio/adc/Kconfig                       |   12 +
>  drivers/iio/adc/Makefile                      |    1 +
>  drivers/iio/adc/ad485x.c                      | 1061 +++++++++++++++++
>  drivers/iio/adc/adi-axi-adc.c                 |   44 +
>  drivers/iio/industrialio-backend.c            |   45 +
>  include/linux/iio/backend.h                   |   13 +
>  8 files changed, 1272 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-ad485x
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad485x.yaml
>  create mode 100644 drivers/iio/adc/ad485x.c
>