[PATCH v2 0/7] iio: ad9467: support interface tuning

[PATCH v2 0/7] iio: ad9467: support interface tuning

[Nuno Sa via B4 Relay]

Hi Jonathan,

Here it goes v2. Some points were still left open on v1 but this should
already include most of the things you asked for. 

---
Changes in v2:
- Link to v1: https://lore.kernel.org/r/20240419-ad9467-new-features-v1-0-3e7628ff6d5e@analog.com

- Patch 1:
  * New patch;
  
- Patch 2:
  * Added IIO_BACKEND_NO_TESTPATERN to stop the pattern (may be a no-op);
  * Drop struct iio_backend_chan_status and use a bool pointer;
  * Rename tap -> taps;
  * Improved commit message.
  
- Patch 3 & 4:
  * Added fixes tag;
  * Imporved commit message (so one can better decide for backporting).

- Patch 6:
  * Add comment on the sleep before checking for error;
  * Use switch case in axi_adc_data_sample_trigger();
  * Add IIO_BACKEND_NO_TEST_PATTERN.

- Patch 7:
  * Instead of caching the sample_rate, use clk_get_rate() "in place";
  * Added comment clarifying dco (data clock output);
  * Remove shadow ret;
  * Moved the tuning table dump into debugfs.

---
Nuno Sa (7):
      iio: backend: change docs padding
      iio: backend: add API for interface tuning
      dt-bindings: adc: axi-adc: add clocks property
      iio: adc: axi-adc: make sure AXI clock is enabled
      iio: adc: adi-axi-adc: remove regmap max register
      iio: adc: adi-axi-adc: support digital interface calibration
      iio: adc: ad9467: support digital interface calibration

 .../devicetree/bindings/iio/adc/adi,axi-adc.yaml   |   5 +
 drivers/iio/adc/ad9467.c                           | 374 ++++++++++++++++++---
 drivers/iio/adc/adi-axi-adc.c                      | 127 ++++++-
 drivers/iio/industrialio-backend.c                 | 162 ++++++---
 include/linux/iio/backend.h                        |  74 ++--
 5 files changed, 643 insertions(+), 99 deletions(-)
---
base-commit: cd84b21017b5f7b993341ee8b5c5bf10ddba37ab
change-id: 20240419-ad9467-new-features-fbfbaa5edf06
--

Thanks!
- Nuno Sá

[PATCH v2 1/7] iio: backend: change docs padding

[Nuno Sa via B4 Relay]

From: Nuno Sa <nuno.sa@analog.com>

Using tabs and maintaining the start of the docs aligned is a pain and
may lead to lot's of unrelated changes when adding new members. Hence,
let#s change things now and just have a simple space after the member
name.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/industrialio-backend.c | 76 +++++++++++++++++++-------------------
 include/linux/iio/backend.h        | 38 +++++++++----------
 2 files changed, 57 insertions(+), 57 deletions(-)

diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c
index f08ed6d70ae5..c27243e88462 100644
--- a/drivers/iio/industrialio-backend.c
+++ b/drivers/iio/industrialio-backend.c
@@ -115,8 +115,8 @@ static DEFINE_MUTEX(iio_back_lock);
 
 /**
  * iio_backend_chan_enable - Enable a backend channel
- * @back:	Backend device
- * @chan:	Channel number
+ * @back: Backend device
+ * @chan: Channel number
  *
  * RETURNS:
  * 0 on success, negative error number on failure.
@@ -129,8 +129,8 @@ EXPORT_SYMBOL_NS_GPL(iio_backend_chan_enable, IIO_BACKEND);
 
 /**
  * iio_backend_chan_disable - Disable a backend channel
- * @back:	Backend device
- * @chan:	Channel number
+ * @back: Backend device
+ * @chan: Channel number
  *
  * RETURNS:
  * 0 on success, negative error number on failure.
@@ -148,8 +148,8 @@ static void __iio_backend_disable(void *back)
 
 /**
  * devm_iio_backend_enable - Device managed backend enable
- * @dev:	Consumer device for the backend
- * @back:	Backend device
+ * @dev: Consumer device for the backend
+ * @back: Backend device
  *
  * RETURNS:
  * 0 on success, negative error number on failure.
@@ -168,9 +168,9 @@ EXPORT_SYMBOL_NS_GPL(devm_iio_backend_enable, IIO_BACKEND);
 
 /**
  * iio_backend_data_format_set - Configure the channel data format
- * @back:	Backend device
- * @chan:	Channel number
- * @data:	Data format
+ * @back: Backend device
+ * @chan: Channel number
+ * @data: Data format
  *
  * Properly configure a channel with respect to the expected data format. A
  * @struct iio_backend_data_fmt must be passed with the settings.
@@ -190,9 +190,9 @@ EXPORT_SYMBOL_NS_GPL(iio_backend_data_format_set, IIO_BACKEND);
 
 /**
  * iio_backend_data_source_set - Select data source
- * @back:	Backend device
- * @chan:	Channel number
- * @data:	Data source
+ * @back: Backend device
+ * @chan: Channel number
+ * @data: Data source
  *
  * A given backend may have different sources to stream/sync data. This allows
  * to choose that source.
@@ -212,9 +212,9 @@ EXPORT_SYMBOL_NS_GPL(iio_backend_data_source_set, IIO_BACKEND);
 
 /**
  * iio_backend_set_sampling_freq - Set channel sampling rate
- * @back:		Backend device
- * @chan:		Channel number
- * @sample_rate_hz:	Sample rate
+ * @back: Backend device
+ * @chan: Channel number
+ * @sample_rate_hz: Sample rate
  *
  * RETURNS:
  * 0 on success, negative error number on failure.
@@ -235,9 +235,9 @@ static void iio_backend_free_buffer(void *arg)
 
 /**
  * devm_iio_backend_request_buffer - Device managed buffer request
- * @dev:	Consumer device for the backend
- * @back:	Backend device
- * @indio_dev:	IIO device
+ * @dev: Consumer device for the backend
+ * @back: Backend device
+ * @indio_dev: IIO device
  *
  * Request an IIO buffer from the backend. The type of the buffer (typically
  * INDIO_BUFFER_HARDWARE) is up to the backend to decide. This is because,
@@ -300,10 +300,10 @@ static struct iio_backend *iio_backend_from_indio_dev_parent(const struct device
 
 /**
  * iio_backend_ext_info_get - IIO ext_info read callback
- * @indio_dev:	IIO device
- * @private:	Data private to the driver
- * @chan:	IIO channel
- * @buf:	Buffer where to place the attribute data
+ * @indio_dev: IIO device
+ * @private: Data private to the driver
+ * @chan: IIO channel
+ * @buf: Buffer where to place the attribute data
  *
  * This helper is intended to be used by backends that extend an IIO channel
  * (through iio_backend_extend_chan_spec()) with extended info. In that case,
@@ -335,11 +335,11 @@ EXPORT_SYMBOL_NS_GPL(iio_backend_ext_info_get, IIO_BACKEND);
 
 /**
  * iio_backend_ext_info_set - IIO ext_info write callback
- * @indio_dev:	IIO device
- * @private:	Data private to the driver
- * @chan:	IIO channel
- * @buf:	Buffer holding the sysfs attribute
- * @len:	Buffer length
+ * @indio_dev: IIO device
+ * @private: Data private to the driver
+ * @chan: IIO channel
+ * @buf: Buffer holding the sysfs attribute
+ * @len: Buffer length
  *
  * This helper is intended to be used by backends that extend an IIO channel
  * (trough iio_backend_extend_chan_spec()) with extended info. In that case,
@@ -365,9 +365,9 @@ EXPORT_SYMBOL_NS_GPL(iio_backend_ext_info_set, IIO_BACKEND);
 
 /**
  * iio_backend_extend_chan_spec - Extend an IIO channel
- * @indio_dev:	IIO device
- * @back:	Backend device
- * @chan:	IIO channel
+ * @indio_dev: IIO device
+ * @back: Backend device
+ * @chan: IIO channel
  *
  * Some backends may have their own functionalities and hence capable of
  * extending a frontend's channel.
@@ -449,8 +449,8 @@ static int __devm_iio_backend_get(struct device *dev, struct iio_backend *back)
 
 /**
  * devm_iio_backend_get - Device managed backend device get
- * @dev:	Consumer device for the backend
- * @name:	Backend name
+ * @dev: Consumer device for the backend
+ * @name: Backend name
  *
  * Get's the backend associated with @dev.
  *
@@ -501,8 +501,8 @@ EXPORT_SYMBOL_NS_GPL(devm_iio_backend_get, IIO_BACKEND);
 
 /**
  * __devm_iio_backend_get_from_fwnode_lookup - Device managed fwnode backend device get
- * @dev:	Consumer device for the backend
- * @fwnode:	Firmware node of the backend device
+ * @dev: Consumer device for the backend
+ * @fwnode: Firmware node of the backend device
  *
  * Search the backend list for a device matching @fwnode.
  * This API should not be used and it's only present for preventing the first
@@ -536,7 +536,7 @@ EXPORT_SYMBOL_NS_GPL(__devm_iio_backend_get_from_fwnode_lookup, IIO_BACKEND);
 
 /**
  * iio_backend_get_priv - Get driver private data
- * @back:	Backend device
+ * @back: Backend device
  */
 void *iio_backend_get_priv(const struct iio_backend *back)
 {
@@ -554,9 +554,9 @@ static void iio_backend_unregister(void *arg)
 
 /**
  * devm_iio_backend_register - Device managed backend device register
- * @dev:	Backend device being registered
- * @ops:	Backend ops
- * @priv:	Device private data
+ * @dev: Backend device being registered
+ * @ops: Backend ops
+ * @priv: Device private data
  *
  * @ops is mandatory. Not providing it results in -EINVAL.
  *
diff --git a/include/linux/iio/backend.h b/include/linux/iio/backend.h
index 9d144631134d..e3e62f65db14 100644
--- a/include/linux/iio/backend.h
+++ b/include/linux/iio/backend.h
@@ -24,9 +24,9 @@ enum iio_backend_data_source {
 
 /**
  * IIO_BACKEND_EX_INFO - Helper for an IIO extended channel attribute
- * @_name:	Attribute name
- * @_shared:	Whether the attribute is shared between all channels
- * @_what:	Data private to the driver
+ * @_name: Attribute name
+ * @_shared: Whether the attribute is shared between all channels
+ * @_what: Data private to the driver
  */
 #define IIO_BACKEND_EX_INFO(_name, _shared, _what) {	\
 	.name = (_name),				\
@@ -38,10 +38,10 @@ enum iio_backend_data_source {
 
 /**
  * struct iio_backend_data_fmt - Backend data format
- * @type:		Data type.
- * @sign_extend:	Bool to tell if the data is sign extended.
- * @enable:		Enable/Disable the data format module. If disabled,
- *			not formatting will happen.
+ * @type: Data type.
+ * @sign_extend: Bool to tell if the data is sign extended.
+ * @enable: Enable/Disable the data format module. If disabled,
+ *	    not formatting will happen.
  */
 struct iio_backend_data_fmt {
 	enum iio_backend_data_type type;
@@ -51,18 +51,18 @@ struct iio_backend_data_fmt {
 
 /**
  * struct iio_backend_ops - operations structure for an iio_backend
- * @enable:		Enable backend.
- * @disable:		Disable backend.
- * @chan_enable:	Enable one channel.
- * @chan_disable:	Disable one channel.
- * @data_format_set:	Configure the data format for a specific channel.
- * @data_source_set:	Configure the data source for a specific channel.
- * @set_sample_rate:	Configure the sampling rate for a specific channel.
- * @request_buffer:	Request an IIO buffer.
- * @free_buffer:	Free an IIO buffer.
- * @extend_chan_spec:	Extend an IIO channel.
- * @ext_info_set:	Extended info setter.
- * @ext_info_get:	Extended info getter.
+ * @enable: Enable backend.
+ * @disable: Disable backend.
+ * @chan_enable: Enable one channel.
+ * @chan_disable: Disable one channel.
+ * @data_format_set: Configure the data format for a specific channel.
+ * @data_source_set: Configure the data source for a specific channel.
+ * @set_sample_rate: Configure the sampling rate for a specific channel.
+ * @request_buffer: Request an IIO buffer.
+ * @free_buffer: Free an IIO buffer.
+ * @extend_chan_spec: Extend an IIO channel.
+ * @ext_info_set: Extended info setter.
+ * @ext_info_get: Extended info getter.
  **/
 struct iio_backend_ops {
 	int (*enable)(struct iio_backend *back);

-- 
2.44.0

[PATCH v2 2/7] iio: backend: add API for interface tuning

[Nuno Sa via B4 Relay]

From: Nuno Sa <nuno.sa@analog.com>

This is in preparation for supporting interface tuning in one for the
devices using the axi-adc backend. The new added interfaces are all
needed for that calibration:

 * iio_backend_test_pattern_set();
 * iio_backend_chan_status();
 * iio_backend_iodelay_set();
 * iio_backend_data_sample_trigger().

Interface tuning is the process of going through a set of known points
(typically by the frontend), change some clk or data delays (or both)
and send/receive some known signal (so called test patterns in this
change). The receiving end (either frontend or the backend) is
responsible for validating the signal and see if it's good or not. The
goal for all of this is to come up with ideal delays at the data
interface level so we can have a proper, more reliable data transfer.

Also note that for some devices we can change the sampling rate
(which typically means changing some reference clock) and that can
affect the data interface. In that case, it's import to run the tuning
algorithm again as the values we had before may no longer be the best (or
even valid) ones.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/industrialio-backend.c | 86 ++++++++++++++++++++++++++++++++++++++
 include/linux/iio/backend.h        | 36 ++++++++++++++++
 2 files changed, 122 insertions(+)

diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c
index c27243e88462..929aff4040ed 100644
--- a/drivers/iio/industrialio-backend.c
+++ b/drivers/iio/industrialio-backend.c
@@ -226,6 +226,92 @@ int iio_backend_set_sampling_freq(struct iio_backend *back, unsigned int chan,
 }
 EXPORT_SYMBOL_NS_GPL(iio_backend_set_sampling_freq, IIO_BACKEND);
 
+/**
+ * iio_backend_test_pattern_set - Configure a test pattern
+ * @back: Backend device
+ * @chan: Channel number
+ * @pattern: Test pattern
+ *
+ * Configure a test pattern on the backend. This is typically used for
+ * calibrating the timings on the data digital interface.
+ *
+ * RETURNS:
+ * 0 on success, negative error number on failure.
+ */
+int iio_backend_test_pattern_set(struct iio_backend *back,
+				 unsigned int chan,
+				 enum iio_backend_test_pattern pattern)
+{
+	if (pattern >= IIO_BACKEND_TEST_PATTERN_MAX)
+		return -EINVAL;
+
+	return iio_backend_op_call(back, test_pattern_set, chan, pattern);
+}
+EXPORT_SYMBOL_NS_GPL(iio_backend_test_pattern_set, IIO_BACKEND);
+
+/**
+ * iio_backend_chan_status - Get the channel status
+ * @back: Backend device
+ * @chan: Channel number
+ * @error: Error indication
+ *
+ * Get the current state of the backend channel. Typically used to check if
+ * there were any errors sending/receiving data.
+ *
+ * RETURNS:
+ * 0 on success, negative error number on failure.
+ */
+int iio_backend_chan_status(struct iio_backend *back, unsigned int chan,
+			    bool *error)
+{
+	return iio_backend_op_call(back, chan_status, chan, error);
+}
+EXPORT_SYMBOL_NS_GPL(iio_backend_chan_status, IIO_BACKEND);
+
+/**
+ * iio_backend_iodelay_set - Set digital I/O delay
+ * @back: Backend device
+ * @lane: Lane number
+ * @taps: Number of taps
+ *
+ * Controls delays on sending/receiving data. One usecase for this is to
+ * calibrate the data digital interface so we get the best results when
+ * transferring data. Note that @taps has no unit since the actual delay per tap
+ * is very backend specific. Hence, frontend devices typically should go through
+ * an array of @taps (the size of that array should typically match the size of
+ * calibration points on the frontend device) and call this API.
+ *
+ * RETURNS:
+ * 0 on success, negative error number on failure.
+ */
+int iio_backend_iodelay_set(struct iio_backend *back, unsigned int lane,
+			    unsigned int taps)
+{
+	return iio_backend_op_call(back, iodelay_set, lane, taps);
+}
+EXPORT_SYMBOL_NS_GPL(iio_backend_iodelay_set, IIO_BACKEND);
+
+/**
+ * iio_backend_data_sample_trigger - Control when to sample data
+ * @back: Backend device
+ * @trigger: Data trigger
+ *
+ * Mostly useful for input backends. Configures the backend for when to sample
+ * data (eg: rising vs falling edge).
+ *
+ * RETURNS:
+ * 0 on success, negative error number on failure.
+ */
+int iio_backend_data_sample_trigger(struct iio_backend *back,
+				    enum iio_backend_sample_trigger trigger)
+{
+	if (trigger >= IIO_BACKEND_SAMPLE_TRIGGER_MAX)
+		return -EINVAL;
+
+	return iio_backend_op_call(back, data_sample_trigger, trigger);
+}
+EXPORT_SYMBOL_NS_GPL(iio_backend_data_sample_trigger, IIO_BACKEND);
+
 static void iio_backend_free_buffer(void *arg)
 {
 	struct iio_backend_buffer_pair *pair = arg;
diff --git a/include/linux/iio/backend.h b/include/linux/iio/backend.h
index e3e62f65db14..8099759d7242 100644
--- a/include/linux/iio/backend.h
+++ b/include/linux/iio/backend.h
@@ -49,6 +49,20 @@ struct iio_backend_data_fmt {
 	bool enable;
 };
 
+/* vendor specific from 32 */
+enum iio_backend_test_pattern {
+	IIO_BACKEND_NO_TEST_PATTERN,
+	/* modified prbs9 */
+	IIO_BACKEND_ADI_PRBS_9A = 32,
+	IIO_BACKEND_TEST_PATTERN_MAX
+};
+
+enum iio_backend_sample_trigger {
+	IIO_BACKEND_SAMPLE_TRIGGER_EDGE_FALLING,
+	IIO_BACKEND_SAMPLE_TRIGGER_EDGE_RISING,
+	IIO_BACKEND_SAMPLE_TRIGGER_MAX
+};
+
 /**
  * struct iio_backend_ops - operations structure for an iio_backend
  * @enable: Enable backend.
@@ -58,6 +72,10 @@ struct iio_backend_data_fmt {
  * @data_format_set: Configure the data format for a specific channel.
  * @data_source_set: Configure the data source for a specific channel.
  * @set_sample_rate: Configure the sampling rate for a specific channel.
+ * @test_pattern_set: Configure a test pattern.
+ * @chan_status: Get the channel status.
+ * @iodelay_set: Set digital I/O delay.
+ * @data_sample_trigger: Control when to sample data.
  * @request_buffer: Request an IIO buffer.
  * @free_buffer: Free an IIO buffer.
  * @extend_chan_spec: Extend an IIO channel.
@@ -75,6 +93,15 @@ struct iio_backend_ops {
 			       enum iio_backend_data_source data);
 	int (*set_sample_rate)(struct iio_backend *back, unsigned int chan,
 			       u64 sample_rate_hz);
+	int (*test_pattern_set)(struct iio_backend *back,
+				unsigned int chan,
+				enum iio_backend_test_pattern pattern);
+	int (*chan_status)(struct iio_backend *back, unsigned int chan,
+			   bool *error);
+	int (*iodelay_set)(struct iio_backend *back, unsigned int chan,
+			   unsigned int taps);
+	int (*data_sample_trigger)(struct iio_backend *back,
+				   enum iio_backend_sample_trigger trigger);
 	struct iio_buffer *(*request_buffer)(struct iio_backend *back,
 					     struct iio_dev *indio_dev);
 	void (*free_buffer)(struct iio_backend *back,
@@ -97,6 +124,15 @@ int iio_backend_data_source_set(struct iio_backend *back, unsigned int chan,
 				enum iio_backend_data_source data);
 int iio_backend_set_sampling_freq(struct iio_backend *back, unsigned int chan,
 				  u64 sample_rate_hz);
+int iio_backend_test_pattern_set(struct iio_backend *back,
+				 unsigned int chan,
+				 enum iio_backend_test_pattern pattern);
+int iio_backend_chan_status(struct iio_backend *back, unsigned int chan,
+			    bool *error);
+int iio_backend_iodelay_set(struct iio_backend *back, unsigned int lane,
+			    unsigned int taps);
+int iio_backend_data_sample_trigger(struct iio_backend *back,
+				    enum iio_backend_sample_trigger trigger);
 int devm_iio_backend_request_buffer(struct device *dev,
 				    struct iio_backend *back,
 				    struct iio_dev *indio_dev);

-- 
2.44.0

[PATCH v2 3/7] dt-bindings: adc: axi-adc: add clocks property

[Nuno Sa via B4 Relay]

From: Nuno Sa <nuno.sa@analog.com>

Add a required clock property as we can't access the device registers if
the AXI bus clock is not properly enabled.

Note this clock is a very fundamental one that is typically enabled
pretty early during boot. Independently of that, we should really rely on
it to be enabled.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Fixes: 96553a44e96d ("dt-bindings: iio: adc: add bindings doc for AXI ADC driver")
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 Documentation/devicetree/bindings/iio/adc/adi,axi-adc.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,axi-adc.yaml b/Documentation/devicetree/bindings/iio/adc/adi,axi-adc.yaml
index 3d49d21ad33d..e1f450b80db2 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,axi-adc.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,axi-adc.yaml
@@ -28,6 +28,9 @@ properties:
   reg:
     maxItems: 1
 
+  clocks:
+    maxItems: 1
+
   dmas:
     maxItems: 1
 
@@ -48,6 +51,7 @@ required:
   - compatible
   - dmas
   - reg
+  - clocks
 
 additionalProperties: false
 
@@ -58,6 +62,7 @@ examples:
         reg = <0x44a00000 0x10000>;
         dmas = <&rx_dma 0>;
         dma-names = "rx";
+        clocks = <&axi_clk>;
         #io-backend-cells = <0>;
     };
 ...

-- 
2.44.0

[PATCH v2 4/7] iio: adc: axi-adc: make sure AXI clock is enabled

[Nuno Sa via B4 Relay]

From: Nuno Sa <nuno.sa@analog.com>

We can only access the IP core registers if the bus clock is enabled. As
such we need to get and enable it and not rely on anyone else to do it.

Note this clock is a very fundamental one that is typically enabled
pretty early during boot. Independently of that, we should really rely on
it to be enabled.

Fixes: ef04070692a2 ("iio: adc: adi-axi-adc: add support for AXI ADC IP core")
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/adc/adi-axi-adc.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
index a543b91124b0..e3b215882941 100644
--- a/drivers/iio/adc/adi-axi-adc.c
+++ b/drivers/iio/adc/adi-axi-adc.c
@@ -175,6 +175,7 @@ static int adi_axi_adc_probe(struct platform_device *pdev)
 	struct adi_axi_adc_state *st;
 	void __iomem *base;
 	unsigned int ver;
+	struct clk *clk;
 	int ret;
 
 	st = devm_kzalloc(&pdev->dev, sizeof(*st), GFP_KERNEL);
@@ -195,6 +196,10 @@ static int adi_axi_adc_probe(struct platform_device *pdev)
 	if (!expected_ver)
 		return -ENODEV;
 
+	clk = devm_clk_get_enabled(&pdev->dev, NULL);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
 	/*
 	 * Force disable the core. Up to the frontend to enable us. And we can
 	 * still read/write registers...

-- 
2.44.0

Re: [PATCH v2 4/7] iio: adc: axi-adc: make sure AXI clock is enabled

[Jonathan Cameron]

On Fri, 26 Apr 2024 17:42:13 +0200
Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:

> From: Nuno Sa <nuno.sa@analog.com>
> 
> We can only access the IP core registers if the bus clock is enabled. As
> such we need to get and enable it and not rely on anyone else to do it.
> 
> Note this clock is a very fundamental one that is typically enabled
> pretty early during boot. Independently of that, we should really rely on
> it to be enabled.
> 
> Fixes: ef04070692a2 ("iio: adc: adi-axi-adc: add support for AXI ADC IP core")
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
Queued up this and the previous one.

If the rest doesn't quite make the merge window I'd like these two fixed
to do so anyway.

Also marked for stable.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/adi-axi-adc.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
> index a543b91124b0..e3b215882941 100644
> --- a/drivers/iio/adc/adi-axi-adc.c
> +++ b/drivers/iio/adc/adi-axi-adc.c
> @@ -175,6 +175,7 @@ static int adi_axi_adc_probe(struct platform_device *pdev)
>  	struct adi_axi_adc_state *st;
>  	void __iomem *base;
>  	unsigned int ver;
> +	struct clk *clk;
>  	int ret;
>  
>  	st = devm_kzalloc(&pdev->dev, sizeof(*st), GFP_KERNEL);
> @@ -195,6 +196,10 @@ static int adi_axi_adc_probe(struct platform_device *pdev)
>  	if (!expected_ver)
>  		return -ENODEV;
>  
> +	clk = devm_clk_get_enabled(&pdev->dev, NULL);
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +
>  	/*
>  	 * Force disable the core. Up to the frontend to enable us. And we can
>  	 * still read/write registers...
> 

[PATCH v2 5/7] iio: adc: adi-axi-adc: remove regmap max register

[Nuno Sa via B4 Relay]

From: Nuno Sa <nuno.sa@analog.com>

In one of the following patches, we'll have some new functionality that
requires reads/writes on registers bigger than 0x8000. Hence, as this is
an highly flexible core, don't bother in setting 'max_register' and
remove it from regmap_config.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/adc/adi-axi-adc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
index e3b215882941..b312369b7366 100644
--- a/drivers/iio/adc/adi-axi-adc.c
+++ b/drivers/iio/adc/adi-axi-adc.c
@@ -156,7 +156,6 @@ static const struct regmap_config axi_adc_regmap_config = {
 	.val_bits = 32,
 	.reg_bits = 32,
 	.reg_stride = 4,
-	.max_register = 0x0800,
 };
 
 static const struct iio_backend_ops adi_axi_adc_generic = {

-- 
2.44.0

[PATCH v2 6/7] iio: adc: adi-axi-adc: support digital interface calibration

[Nuno Sa via B4 Relay]

From: Nuno Sa <nuno.sa@analog.com>

Implement the new IIO backend APIs for calibrating the data
digital interfaces.

While at it, removed the tabs in 'struct adi_axi_adc_state' and used
spaces for the members.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/adc/adi-axi-adc.c | 121 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 119 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
index b312369b7366..3a556e72f458 100644
--- a/drivers/iio/adc/adi-axi-adc.c
+++ b/drivers/iio/adc/adi-axi-adc.c
@@ -7,11 +7,13 @@
  */
 
 #include <linux/bitfield.h>
+#include <linux/cleanup.h>
 #include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/io.h>
 #include <linux/delay.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/property.h>
@@ -37,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_CTRL			0x0044
+#define    ADI_AXI_ADC_CTRL_DDR_EDGESEL_MASK	BIT(1)
+
 /* ADC Channel controls */
 
 #define ADI_AXI_REG_CHAN_CTRL(c)		(0x0400 + (c) * 0x40)
@@ -51,14 +56,28 @@
 #define   ADI_AXI_REG_CHAN_CTRL_PN_TYPE_OWR	BIT(1)
 #define   ADI_AXI_REG_CHAN_CTRL_ENABLE		BIT(0)
 
+#define ADI_AXI_ADC_REG_CHAN_STATUS(c)		(0x0404 + (c) * 0x40)
+#define   ADI_AXI_ADC_CHAN_STAT_PN_MASK		GENMASK(2, 1)
+
+#define ADI_AXI_ADC_REG_CHAN_CTRL_3(c)		(0x0418 + (c) * 0x40)
+#define   ADI_AXI_ADC_CHAN_PN_SEL_MASK		GENMASK(19, 16)
+
+/* IO Delays */
+#define ADI_AXI_ADC_REG_DELAY(l)		(0x0800 + (l) * 0x4)
+#define   AXI_ADC_DELAY_CTRL_MASK		GENMASK(4, 0)
+
+#define ADI_AXI_ADC_MAX_IO_NUM_LANES		15
+
 #define ADI_AXI_REG_CHAN_CTRL_DEFAULTS		\
 	(ADI_AXI_REG_CHAN_CTRL_FMT_SIGNEXT |	\
 	 ADI_AXI_REG_CHAN_CTRL_FMT_EN |		\
 	 ADI_AXI_REG_CHAN_CTRL_ENABLE)
 
 struct adi_axi_adc_state {
-	struct regmap				*regmap;
-	struct device				*dev;
+	struct regmap *regmap;
+	struct device *dev;
+	/* lock to protect multiple accesses to the device registers */
+	struct mutex lock;
 };
 
 static int axi_adc_enable(struct iio_backend *back)
@@ -104,6 +123,100 @@ static int axi_adc_data_format_set(struct iio_backend *back, unsigned int chan,
 				  ADI_AXI_REG_CHAN_CTRL_FMT_MASK, val);
 }
 
+static int axi_adc_data_sample_trigger(struct iio_backend *back,
+				       enum iio_backend_sample_trigger trigger)
+{
+	struct adi_axi_adc_state *st = iio_backend_get_priv(back);
+
+	switch (trigger) {
+	case IIO_BACKEND_SAMPLE_TRIGGER_EDGE_RISING:
+		return regmap_clear_bits(st->regmap, ADI_AXI_ADC_REG_CTRL,
+					 ADI_AXI_ADC_CTRL_DDR_EDGESEL_MASK);
+	case IIO_BACKEND_SAMPLE_TRIGGER_EDGE_FALLING:
+		return regmap_set_bits(st->regmap, ADI_AXI_ADC_REG_CTRL,
+				       ADI_AXI_ADC_CTRL_DDR_EDGESEL_MASK);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int axi_adc_iodelays_set(struct iio_backend *back, unsigned int lane,
+				unsigned int tap)
+{
+	struct adi_axi_adc_state *st = iio_backend_get_priv(back);
+	int ret;
+	u32 val;
+
+	if (tap > FIELD_MAX(AXI_ADC_DELAY_CTRL_MASK))
+		return -EINVAL;
+	if (lane > ADI_AXI_ADC_MAX_IO_NUM_LANES)
+		return -EINVAL;
+
+	guard(mutex)(&st->lock);
+	ret = regmap_write(st->regmap, ADI_AXI_ADC_REG_DELAY(lane), tap);
+	if (ret)
+		return ret;
+	/*
+	 * If readback is ~0, that means there are issues with the
+	 * delay_clk.
+	 */
+	ret = regmap_read(st->regmap, ADI_AXI_ADC_REG_DELAY(lane), &val);
+	if (ret)
+		return ret;
+	if (val == U32_MAX)
+		return -EIO;
+
+	return 0;
+}
+
+static int axi_adc_test_pattern_set(struct iio_backend *back,
+				    unsigned int chan,
+				    enum iio_backend_test_pattern pattern)
+{
+	struct adi_axi_adc_state *st = iio_backend_get_priv(back);
+
+	switch (pattern) {
+	case IIO_BACKEND_NO_TEST_PATTERN:
+		/* nothing to do */
+		return 0;
+	case IIO_BACKEND_ADI_PRBS_9A:
+		return regmap_update_bits(st->regmap, ADI_AXI_ADC_REG_CHAN_CTRL_3(chan),
+					  ADI_AXI_ADC_CHAN_PN_SEL_MASK,
+					  FIELD_PREP(ADI_AXI_ADC_CHAN_PN_SEL_MASK, 0));
+	default:
+		return -EINVAL;
+	}
+}
+
+static int axi_adc_chan_status(struct iio_backend *back, unsigned int chan,
+			       bool *error)
+{
+	struct adi_axi_adc_state *st = iio_backend_get_priv(back);
+	int ret;
+	u32 val;
+
+	guard(mutex)(&st->lock);
+	/* reset test bits by setting them */
+	ret = regmap_write(st->regmap, ADI_AXI_ADC_REG_CHAN_STATUS(chan),
+			   ADI_AXI_ADC_CHAN_STAT_PN_MASK);
+	if (ret)
+		return ret;
+
+	/* let's give enough time to validate or erroring the incoming pattern */
+	fsleep(1000);
+
+	ret = regmap_read(st->regmap, ADI_AXI_ADC_REG_CHAN_STATUS(chan), &val);
+	if (ret)
+		return ret;
+
+	if (ADI_AXI_ADC_CHAN_STAT_PN_MASK & val)
+		*error = true;
+	else
+		*error = false;
+
+	return 0;
+}
+
 static int axi_adc_chan_enable(struct iio_backend *back, unsigned int chan)
 {
 	struct adi_axi_adc_state *st = iio_backend_get_priv(back);
@@ -166,6 +279,10 @@ static const struct iio_backend_ops adi_axi_adc_generic = {
 	.chan_disable = axi_adc_chan_disable,
 	.request_buffer = axi_adc_request_buffer,
 	.free_buffer = axi_adc_free_buffer,
+	.data_sample_trigger = axi_adc_data_sample_trigger,
+	.iodelay_set = axi_adc_iodelays_set,
+	.test_pattern_set = axi_adc_test_pattern_set,
+	.chan_status = axi_adc_chan_status,
 };
 
 static int adi_axi_adc_probe(struct platform_device *pdev)

-- 
2.44.0

[PATCH v2 7/7] iio: adc: ad9467: support digital interface calibration

[Nuno Sa via B4 Relay]

From: Nuno Sa <nuno.sa@analog.com>

To make sure that we have the best timings on the serial data interface
we should calibrate it. This means going through the device supported
values and see for which ones we get a successful result. To do that, we
use a prbs test pattern both in the IIO backend and in the frontend
devices. Then for each of the test points we see if there are any
errors. Note that the backend is responsible to look for those errors.

As calibrating the interface also requires that the data format is disabled
(the one thing being done in ad9467_setup()), ad9467_setup() was removed
and configuring the data fomat is now part of the calibration process.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/adc/ad9467.c | 374 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 335 insertions(+), 39 deletions(-)

diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
index 7475ec2a56c7..e85b763b9ffc 100644
--- a/drivers/iio/adc/ad9467.c
+++ b/drivers/iio/adc/ad9467.c
@@ -4,7 +4,11 @@
  *
  * Copyright 2012-2020 Analog Devices Inc.
  */
+
+#include <linux/bitmap.h>
+#include <linux/bitops.h>
 #include <linux/cleanup.h>
+#include <linux/debugfs.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/device.h>
@@ -100,6 +104,8 @@
 #define AD9467_DEF_OUTPUT_MODE		0x08
 #define AD9467_REG_VREF_MASK		0x0F
 
+#define AD9647_MAX_TEST_POINTS		32
+
 struct ad9467_chip_info {
 	const char		*name;
 	unsigned int		id;
@@ -110,6 +116,9 @@ struct ad9467_chip_info {
 	unsigned long		max_rate;
 	unsigned int		default_output_mode;
 	unsigned int		vref_mask;
+	unsigned int		num_lanes;
+	/* data clock output */
+	bool			has_dco;
 };
 
 struct ad9467_state {
@@ -119,7 +128,16 @@ struct ad9467_state {
 	struct clk			*clk;
 	unsigned int			output_mode;
 	unsigned int                    (*scales)[2];
-
+	/*
+	 * Times 2 because we may also invert the signal polarity and run the
+	 * calibration again. For some reference on the test points (ad9265) see:
+	 * https://www.analog.com/media/en/technical-documentation/data-sheets/ad9265.pdf
+	 * at page 38 for the dco output delay. On devices as ad9467, the
+	 * calibration is done at the backend level. For the ADI axi-adc:
+	 * https://wiki.analog.com/resources/fpga/docs/axi_adc_ip
+	 * at the io delay control section.
+	 */
+	DECLARE_BITMAP(calib_map, AD9647_MAX_TEST_POINTS * 2);
 	struct gpio_desc		*pwrdown_gpio;
 	/* ensure consistent state obtained on multiple related accesses */
 	struct mutex			lock;
@@ -242,6 +260,7 @@ static const struct ad9467_chip_info ad9467_chip_tbl = {
 	.num_channels = ARRAY_SIZE(ad9467_channels),
 	.default_output_mode = AD9467_DEF_OUTPUT_MODE,
 	.vref_mask = AD9467_REG_VREF_MASK,
+	.num_lanes = 8,
 };
 
 static const struct ad9467_chip_info ad9434_chip_tbl = {
@@ -254,6 +273,7 @@ static const struct ad9467_chip_info ad9434_chip_tbl = {
 	.num_channels = ARRAY_SIZE(ad9434_channels),
 	.default_output_mode = AD9434_DEF_OUTPUT_MODE,
 	.vref_mask = AD9434_REG_VREF_MASK,
+	.num_lanes = 6,
 };
 
 static const struct ad9467_chip_info ad9265_chip_tbl = {
@@ -266,6 +286,7 @@ static const struct ad9467_chip_info ad9265_chip_tbl = {
 	.num_channels = ARRAY_SIZE(ad9467_channels),
 	.default_output_mode = AD9265_DEF_OUTPUT_MODE,
 	.vref_mask = AD9265_REG_VREF_MASK,
+	.has_dco = true,
 };
 
 static int ad9467_get_scale(struct ad9467_state *st, int *val, int *val2)
@@ -321,6 +342,246 @@ static int ad9467_set_scale(struct ad9467_state *st, int val, int val2)
 	return -EINVAL;
 }
 
+static int ad9467_outputmode_set(struct spi_device *spi, unsigned int mode)
+{
+	int ret;
+
+	ret = ad9467_spi_write(spi, AN877_ADC_REG_OUTPUT_MODE, mode);
+	if (ret < 0)
+		return ret;
+
+	return ad9467_spi_write(spi, AN877_ADC_REG_TRANSFER,
+				AN877_ADC_TRANSFER_SYNC);
+}
+
+static int ad9647_calibrate_prepare(const struct ad9467_state *st)
+{
+	struct iio_backend_data_fmt data = {
+		.enable = false,
+	};
+	unsigned int c;
+	int ret;
+
+	ret = ad9467_spi_write(st->spi, AN877_ADC_REG_TEST_IO,
+			       AN877_ADC_TESTMODE_PN9_SEQ);
+	if (ret)
+		return ret;
+
+	ret = ad9467_spi_write(st->spi, AN877_ADC_REG_TRANSFER,
+			       AN877_ADC_TRANSFER_SYNC);
+	if (ret)
+		return ret;
+
+	ret = ad9467_outputmode_set(st->spi, st->info->default_output_mode);
+	if (ret)
+		return ret;
+
+	for (c = 0; c < st->info->num_channels; c++) {
+		ret = iio_backend_data_format_set(st->back, c, &data);
+		if (ret)
+			return ret;
+	}
+
+	ret = iio_backend_test_pattern_set(st->back, 0,
+					   IIO_BACKEND_ADI_PRBS_9A);
+	if (ret)
+		return ret;
+
+	return iio_backend_chan_enable(st->back, 0);
+}
+
+static int ad9647_calibrate_polarity_set(const struct ad9467_state *st,
+					 bool invert)
+{
+	enum iio_backend_sample_trigger trigger;
+
+	if (st->info->has_dco) {
+		unsigned int phase = AN877_ADC_OUTPUT_EVEN_ODD_MODE_EN;
+
+		if (invert)
+			phase |= AN877_ADC_INVERT_DCO_CLK;
+
+		return ad9467_spi_write(st->spi, AN877_ADC_REG_OUTPUT_PHASE,
+					phase);
+	}
+
+	if (invert)
+		trigger = IIO_BACKEND_SAMPLE_TRIGGER_EDGE_FALLING;
+	else
+		trigger = IIO_BACKEND_SAMPLE_TRIGGER_EDGE_RISING;
+
+	return iio_backend_data_sample_trigger(st->back, trigger);
+}
+
+/*
+ * The idea is pretty simple. Find the max number of successful points in a row
+ * and get the one in the middle.
+ */
+static unsigned int ad9467_find_optimal_point(const unsigned long *calib_map,
+					      unsigned int start,
+					      unsigned int nbits,
+					      unsigned int *val)
+{
+	unsigned int bit = start, end, start_cnt, cnt = 0;
+
+	for_each_clear_bitrange_from(bit, end, calib_map, nbits + start) {
+		if (end - bit > cnt) {
+			cnt = end - bit;
+			start_cnt = bit;
+		}
+	}
+
+	if (cnt)
+		*val = start_cnt + cnt / 2;
+
+	return cnt;
+}
+
+static int ad9467_calibrate_apply(const struct ad9467_state *st,
+				  unsigned int val)
+{
+	unsigned int lane;
+	int ret;
+
+	if (st->info->has_dco) {
+		ret = ad9467_spi_write(st->spi, AN877_ADC_REG_OUTPUT_DELAY,
+				       val);
+		if (ret)
+			return ret;
+
+		return ad9467_spi_write(st->spi, AN877_ADC_REG_TRANSFER,
+					AN877_ADC_TRANSFER_SYNC);
+	}
+
+	for (lane = 0; lane < st->info->num_lanes; lane++) {
+		ret = iio_backend_iodelay_set(st->back, lane, val);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int ad9647_calibrate_stop(const struct ad9467_state *st)
+{
+	struct iio_backend_data_fmt data = {
+		.sign_extend = true,
+		.enable = true,
+	};
+	unsigned int c, mode;
+	int ret;
+
+	ret = iio_backend_chan_disable(st->back, 0);
+	if (ret)
+		return ret;
+
+	ret = iio_backend_test_pattern_set(st->back, 0,
+					   IIO_BACKEND_NO_TEST_PATTERN);
+	if (ret)
+		return ret;
+
+	for (c = 0; c < st->info->num_channels; c++) {
+		ret = iio_backend_data_format_set(st->back, c, &data);
+		if (ret)
+			return ret;
+	}
+
+	mode = st->info->default_output_mode | AN877_ADC_OUTPUT_MODE_TWOS_COMPLEMENT;
+	ret = ad9467_outputmode_set(st->spi, mode);
+	if (ret)
+		return ret;
+
+	ret = ad9467_spi_write(st->spi, AN877_ADC_REG_TEST_IO,
+			       AN877_ADC_TESTMODE_OFF);
+	if (ret)
+		return ret;
+
+	return ad9467_spi_write(st->spi, AN877_ADC_REG_TRANSFER,
+			       AN877_ADC_TRANSFER_SYNC);
+}
+
+static int ad9467_calibrate(struct ad9467_state *st)
+{
+	unsigned int point, val, inv_val, cnt, inv_cnt = 0;
+	/*
+	 * Half of the bitmap is for the inverted signal. The number of test
+	 * points is the same though...
+	 */
+	unsigned int test_points = AD9647_MAX_TEST_POINTS;
+	unsigned long sample_rate = clk_get_rate(st->clk);
+	struct device *dev = &st->spi->dev;
+	bool invert = false, stat;
+	int ret;
+
+	/* all points invalid */
+	bitmap_fill(st->calib_map, BITS_PER_TYPE(st->calib_map));
+
+	ret = ad9647_calibrate_prepare(st);
+	if (ret)
+		return ret;
+retune:
+	ret = ad9647_calibrate_polarity_set(st, invert);
+	if (ret)
+		return ret;
+
+	for (point = 0; point < test_points; point++) {
+		ret = ad9467_calibrate_apply(st, point);
+		if (ret)
+			return ret;
+
+		ret = iio_backend_chan_status(st->back, 0, &stat);
+		if (ret)
+			return ret;
+
+		__assign_bit(point + invert * test_points, st->calib_map, stat);
+	}
+
+	if (!invert) {
+		cnt = ad9467_find_optimal_point(st->calib_map, 0, test_points,
+						&val);
+		/*
+		 * We're happy if we find, at least, three good test points in
+		 * a row.
+		 */
+		if (cnt < 3) {
+			invert = true;
+			goto retune;
+		}
+	} else {
+		inv_cnt = ad9467_find_optimal_point(st->calib_map, test_points,
+						    test_points, &inv_val);
+		if (!inv_cnt && !cnt)
+			return -EIO;
+	}
+
+	if (inv_cnt < cnt) {
+		ret = ad9647_calibrate_polarity_set(st, false);
+		if (ret)
+			return ret;
+	} else {
+		/*
+		 * polarity inverted is the last test to run. Hence, there's no
+		 * need to re-do any configuration. We just need to "normalize"
+		 * the selected value.
+		 */
+		val = inv_val - test_points;
+	}
+
+	if (st->info->has_dco)
+		dev_dbg(dev, "%sDCO 0x%X CLK %lu Hz\n", inv_cnt >= cnt ? "INVERT " : "",
+			val, sample_rate);
+	else
+		dev_dbg(dev, "%sIDELAY 0x%x\n", inv_cnt >= cnt ? "INVERT " : "",
+			val);
+
+	ret = ad9467_calibrate_apply(st, val);
+	if (ret)
+		return ret;
+
+	/* finally apply the optimal value */
+	return ad9647_calibrate_stop(st);
+}
+
 static int ad9467_read_raw(struct iio_dev *indio_dev,
 			   struct iio_chan_spec const *chan,
 			   int *val, int *val2, long m)
@@ -345,7 +606,9 @@ static int ad9467_write_raw(struct iio_dev *indio_dev,
 {
 	struct ad9467_state *st = iio_priv(indio_dev);
 	const struct ad9467_chip_info *info = st->info;
+	unsigned long sample_rate;
 	long r_clk;
+	int ret;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_SCALE:
@@ -358,7 +621,23 @@ static int ad9467_write_raw(struct iio_dev *indio_dev,
 			return -EINVAL;
 		}
 
-		return clk_set_rate(st->clk, r_clk);
+		sample_rate = clk_get_rate(st->clk);
+		/*
+		 * clk_set_rate() would also do this but since we would still
+		 * need it for avoiding an unnecessary calibration, do it now.
+		 */
+		if (sample_rate == r_clk)
+			return 0;
+
+		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
+			ret = clk_set_rate(st->clk, r_clk);
+			if (ret)
+				return ret;
+
+			guard(mutex)(&st->lock);
+			ret = ad9467_calibrate(st);
+		}
+		return ret;
 	default:
 		return -EINVAL;
 	}
@@ -411,18 +690,6 @@ static const struct iio_info ad9467_info = {
 	.read_avail = ad9467_read_avail,
 };
 
-static int ad9467_outputmode_set(struct spi_device *spi, unsigned int mode)
-{
-	int ret;
-
-	ret = ad9467_spi_write(spi, AN877_ADC_REG_OUTPUT_MODE, mode);
-	if (ret < 0)
-		return ret;
-
-	return ad9467_spi_write(spi, AN877_ADC_REG_TRANSFER,
-				AN877_ADC_TRANSFER_SYNC);
-}
-
 static int ad9467_scale_fill(struct ad9467_state *st)
 {
 	const struct ad9467_chip_info *info = st->info;
@@ -442,29 +709,6 @@ static int ad9467_scale_fill(struct ad9467_state *st)
 	return 0;
 }
 
-static int ad9467_setup(struct ad9467_state *st)
-{
-	struct iio_backend_data_fmt data = {
-		.sign_extend = true,
-		.enable = true,
-	};
-	unsigned int c, mode;
-	int ret;
-
-	mode = st->info->default_output_mode | AN877_ADC_OUTPUT_MODE_TWOS_COMPLEMENT;
-	ret = ad9467_outputmode_set(st->spi, mode);
-	if (ret)
-		return ret;
-
-	for (c = 0; c < st->info->num_channels; c++) {
-		ret = iio_backend_data_format_set(st->back, c, &data);
-		if (ret)
-			return ret;
-	}
-
-	return 0;
-}
-
 static int ad9467_reset(struct device *dev)
 {
 	struct gpio_desc *gpio;
@@ -521,6 +765,52 @@ static int ad9467_iio_backend_get(struct ad9467_state *st)
 	return -ENODEV;
 }
 
+static ssize_t ad9467_dump_calib_table(struct file *file,
+				       char __user *userbuf,
+				       size_t count, loff_t *ppos)
+{
+	struct ad9467_state *st = file->private_data;
+	unsigned int bit, size = BITS_PER_TYPE(st->calib_map);
+	/* +2 for the newline and +1 for the string termination */
+	unsigned char map[AD9647_MAX_TEST_POINTS * 2 + 3];
+	ssize_t len = 0;
+
+	guard(mutex)(&st->lock);
+	if (*ppos)
+		goto out_read;
+
+	for (bit = 0; bit < size; bit++) {
+		if (bit == size / 2)
+			len += scnprintf(map + len, sizeof(map) - len, "\n");
+
+		len += scnprintf(map + len, sizeof(map) - len, "%c",
+				 test_bit(bit, st->calib_map) ? 'x' : 'o');
+	}
+
+	len += scnprintf(map + len, sizeof(map) - len, "\n");
+out_read:
+	return simple_read_from_buffer(userbuf, count, ppos, map, len);
+}
+
+static const struct file_operations ad9467_calib_table_fops = {
+	.open = simple_open,
+	.read = ad9467_dump_calib_table,
+	.llseek = default_llseek,
+	.owner = THIS_MODULE,
+};
+
+static void ad9467_debugfs_init(struct iio_dev *indio_dev)
+{
+	struct dentry *d = iio_get_debugfs_dentry(indio_dev);
+	struct ad9467_state *st = iio_priv(indio_dev);
+
+	if (!IS_ENABLED(CONFIG_DEBUG_FS))
+		return;
+
+	debugfs_create_file("calibration_table_dump", 0400, d, st,
+			    &ad9467_calib_table_fops);
+}
+
 static int ad9467_probe(struct spi_device *spi)
 {
 	struct iio_dev *indio_dev;
@@ -580,11 +870,17 @@ static int ad9467_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
-	ret = ad9467_setup(st);
+	ret = ad9467_calibrate(st);
 	if (ret)
 		return ret;
 
-	return devm_iio_device_register(&spi->dev, indio_dev);
+	ret = devm_iio_device_register(&spi->dev, indio_dev);
+	if (ret)
+		return ret;
+
+	ad9467_debugfs_init(indio_dev);
+
+	return 0;
 }
 
 static const struct of_device_id ad9467_of_match[] = {

-- 
2.44.0

Re: [PATCH v2 7/7] iio: adc: ad9467: support digital interface calibration

[Jonathan Cameron]

On Fri, 26 Apr 2024 17:42:16 +0200
Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:

> From: Nuno Sa <nuno.sa@analog.com>
> 
> To make sure that we have the best timings on the serial data interface
> we should calibrate it. This means going through the device supported
> values and see for which ones we get a successful result. To do that, we
> use a prbs test pattern both in the IIO backend and in the frontend
> devices. Then for each of the test points we see if there are any
> errors. Note that the backend is responsible to look for those errors.
> 
> As calibrating the interface also requires that the data format is disabled
> (the one thing being done in ad9467_setup()), ad9467_setup() was removed
> and configuring the data fomat is now part of the calibration process.
> 
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>

One trivial comment.

I'd have picked up the whole series, but it feels too big to do on a Sunday
when you only posted on Friday.  Hence, lets let it sit for at least
a few more days to see if others have comments.

It might not make this cycle as a result.   I've picked up the 2 fixes
today to increase the chances those make it.

Jonathan


>  static int ad9467_read_raw(struct iio_dev *indio_dev,
>  			   struct iio_chan_spec const *chan,
>  			   int *val, int *val2, long m)
> @@ -345,7 +606,9 @@ static int ad9467_write_raw(struct iio_dev *indio_dev,
>  {
>  	struct ad9467_state *st = iio_priv(indio_dev);
>  	const struct ad9467_chip_info *info = st->info;
> +	unsigned long sample_rate;
>  	long r_clk;
> +	int ret;
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SCALE:
> @@ -358,7 +621,23 @@ static int ad9467_write_raw(struct iio_dev *indio_dev,
>  			return -EINVAL;
>  		}
>  
> -		return clk_set_rate(st->clk, r_clk);
> +		sample_rate = clk_get_rate(st->clk);
> +		/*
> +		 * clk_set_rate() would also do this but since we would still
> +		 * need it for avoiding an unnecessary calibration, do it now.
> +		 */
> +		if (sample_rate == r_clk)
> +			return 0;
> +
> +		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> +			ret = clk_set_rate(st->clk, r_clk);
> +			if (ret)
> +				return ret;
> +
> +			guard(mutex)(&st->lock);
> +			ret = ad9467_calibrate(st);
			return ad9467_calibrate(st);
> +		}
		unreachable();

not totally elegant but I think the early return makes more sense and we should
just use an unreachable() to squash the resulting compiler warning.

> +		return ret;
>  	default:
>  		return -EINVAL;
>  	}

Re: [PATCH v2 7/7] iio: adc: ad9467: support digital interface calibration

[Nuno Sá]

On Sun, 2024-04-28 at 18:32 +0100, Jonathan Cameron wrote:
> On Fri, 26 Apr 2024 17:42:16 +0200
> Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:
> 
> > From: Nuno Sa <nuno.sa@analog.com>
> > 
> > To make sure that we have the best timings on the serial data interface
> > we should calibrate it. This means going through the device supported
> > values and see for which ones we get a successful result. To do that, we
> > use a prbs test pattern both in the IIO backend and in the frontend
> > devices. Then for each of the test points we see if there are any
> > errors. Note that the backend is responsible to look for those errors.
> > 
> > As calibrating the interface also requires that the data format is disabled
> > (the one thing being done in ad9467_setup()), ad9467_setup() was removed
> > and configuring the data fomat is now part of the calibration process.
> > 
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> 
> One trivial comment.
> 
> I'd have picked up the whole series, but it feels too big to do on a Sunday
> when you only posted on Friday.  Hence, lets let it sit for at least
> a few more days to see if others have comments.

Yeah, I kind of waited till the last moment to see if you had any important
comment (on the first version open discussions) that could affect v2 :).
> 
> It might not make this cycle as a result.   I've picked up the 2 fixes
> today to increase the chances those make it.
> 
> Jonathan
> 
> 
> >  static int ad9467_read_raw(struct iio_dev *indio_dev,
> >  			   struct iio_chan_spec const *chan,
> >  			   int *val, int *val2, long m)
> > @@ -345,7 +606,9 @@ static int ad9467_write_raw(struct iio_dev *indio_dev,
> >  {
> >  	struct ad9467_state *st = iio_priv(indio_dev);
> >  	const struct ad9467_chip_info *info = st->info;
> > +	unsigned long sample_rate;
> >  	long r_clk;
> > +	int ret;
> >  
> >  	switch (mask) {
> >  	case IIO_CHAN_INFO_SCALE:
> > @@ -358,7 +621,23 @@ static int ad9467_write_raw(struct iio_dev *indio_dev,
> >  			return -EINVAL;
> >  		}
> >  
> > -		return clk_set_rate(st->clk, r_clk);
> > +		sample_rate = clk_get_rate(st->clk);
> > +		/*
> > +		 * clk_set_rate() would also do this but since we would
> > still
> > +		 * need it for avoiding an unnecessary calibration, do it
> > now.
> > +		 */
> > +		if (sample_rate == r_clk)
> > +			return 0;
> > +
> > +		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> > +			ret = clk_set_rate(st->clk, r_clk);
> > +			if (ret)
> > +				return ret;
> > +
> > +			guard(mutex)(&st->lock);
> > +			ret = ad9467_calibrate(st);
> 			return ad9467_calibrate(st);
> > +		}
> 		unreachable();
> 
> not totally elegant but I think the early return makes more sense and we
> should
> just use an unreachable() to squash the resulting compiler warning.
> 

As you might remember I'm not a big fan of the unreachable() but also no strong
feelings about it :). Do you expect a v3 for this or is this something you can
fix up while applying (assuming no other things pop by)? 

- Nuno Sá
> 

Re: [PATCH v2 7/7] iio: adc: ad9467: support digital interface calibration

[Jonathan Cameron]

On Mon, 29 Apr 2024 09:24:21 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Sun, 2024-04-28 at 18:32 +0100, Jonathan Cameron wrote:
> > On Fri, 26 Apr 2024 17:42:16 +0200
> > Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote:
> >   
> > > From: Nuno Sa <nuno.sa@analog.com>
> > > 
> > > To make sure that we have the best timings on the serial data interface
> > > we should calibrate it. This means going through the device supported
> > > values and see for which ones we get a successful result. To do that, we
> > > use a prbs test pattern both in the IIO backend and in the frontend
> > > devices. Then for each of the test points we see if there are any
> > > errors. Note that the backend is responsible to look for those errors.
> > > 
> > > As calibrating the interface also requires that the data format is disabled
> > > (the one thing being done in ad9467_setup()), ad9467_setup() was removed
> > > and configuring the data fomat is now part of the calibration process.
> > > 
> > > Signed-off-by: Nuno Sa <nuno.sa@analog.com>  
> > 
> > One trivial comment.
> > 
> > I'd have picked up the whole series, but it feels too big to do on a Sunday
> > when you only posted on Friday.  Hence, lets let it sit for at least
> > a few more days to see if others have comments.  
> 
> Yeah, I kind of waited till the last moment to see if you had any important
> comment (on the first version open discussions) that could affect v2 :).
> > 
> > It might not make this cycle as a result.   I've picked up the 2 fixes
> > today to increase the chances those make it.
> > 
> > Jonathan
> > 
> >   
> > >  static int ad9467_read_raw(struct iio_dev *indio_dev,
> > >  			   struct iio_chan_spec const *chan,
> > >  			   int *val, int *val2, long m)
> > > @@ -345,7 +606,9 @@ static int ad9467_write_raw(struct iio_dev *indio_dev,
> > >  {
> > >  	struct ad9467_state *st = iio_priv(indio_dev);
> > >  	const struct ad9467_chip_info *info = st->info;
> > > +	unsigned long sample_rate;
> > >  	long r_clk;
> > > +	int ret;
> > >  
> > >  	switch (mask) {
> > >  	case IIO_CHAN_INFO_SCALE:
> > > @@ -358,7 +621,23 @@ static int ad9467_write_raw(struct iio_dev *indio_dev,
> > >  			return -EINVAL;
> > >  		}
> > >  
> > > -		return clk_set_rate(st->clk, r_clk);
> > > +		sample_rate = clk_get_rate(st->clk);
> > > +		/*
> > > +		 * clk_set_rate() would also do this but since we would
> > > still
> > > +		 * need it for avoiding an unnecessary calibration, do it
> > > now.
> > > +		 */
> > > +		if (sample_rate == r_clk)
> > > +			return 0;
> > > +
> > > +		iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> > > +			ret = clk_set_rate(st->clk, r_clk);
> > > +			if (ret)
> > > +				return ret;
> > > +
> > > +			guard(mutex)(&st->lock);
> > > +			ret = ad9467_calibrate(st);  
> > 			return ad9467_calibrate(st);  
> > > +		}  
> > 		unreachable();
> > 
> > not totally elegant but I think the early return makes more sense and we
> > should
> > just use an unreachable() to squash the resulting compiler warning.
> >   
> 
> As you might remember I'm not a big fan of the unreachable() but also no strong
> feelings about it :). Do you expect a v3 for this or is this something you can
> fix up while applying (assuming no other things pop by)? 

I changed my mind and didn't bother adjusting this.

I've queued this up and pushed it out as testing on basis I can always drop
it again if reviews come in within the next 2-3 days, but in meantime I can
let 0-day have at it.

Jonathan

> 
> - Nuno Sá
> >