[PATCH 0/4] iio: ad3552r-hs: add support for internal ramp generator

[PATCH 0/4] iio: ad3552r-hs: add support for internal ramp generator

[Angelo Dureghello]

Add support to enable the HDL IP core internal ramp generator,
actually managed by the adi-axi-dac backend. 

It works this way:

/sys/bus/iio/devices/iio:device0# echo 1 > buffer0/out_voltage0_en 
/sys/bus/iio/devices/iio:device0# echo 1 > buffer0/out_voltage1_en                                           
/sys/bus/iio/devices/iio:device0# echo 1 > buffer0/enable 

Activating ramp generator:

/sys/kernel/debug/iio/iio:device0# echo -n backend-ramp-generator > data_source

Deactivating:

/sys/kernel/debug/iio/iio:device0# echo -n iio-buffer > data_source

Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
---
Angelo Dureghello (4):
      docs: iio: add documentation for ad3552r driver
      iio: backend: add support for data source get
      iio: dac: adi-axi-dac: add data source get
      iio: dac: ad3552r-hs: add support for internal ramp

 Documentation/iio/ad3552r.rst      |  65 +++++++++++++++++++++++
 Documentation/iio/index.rst        |   1 +
 MAINTAINERS                        |   1 +
 drivers/iio/dac/ad3552r-hs.c       | 106 ++++++++++++++++++++++++++++++++++---
 drivers/iio/dac/adi-axi-dac.c      |  27 ++++++++++
 drivers/iio/industrialio-backend.c |  28 ++++++++++
 include/linux/iio/backend.h        |   5 ++
 7 files changed, 227 insertions(+), 6 deletions(-)
---
base-commit: eb870a5af7db1e5ca59330875125230b28e630f9
change-id: 20250321-wip-bl-ad3552r-fixes-4a386944c170

Best regards,
-- 
Angelo Dureghello <adureghello@baylibre.com>

[PATCH 1/4] docs: iio: add documentation for ad3552r driver

[Angelo Dureghello]

From: Angelo Dureghello <adureghello@baylibre.com>

Add documentation for ad3552r driver, needed to describe the high-speed
driver debugfs attributes and shows how the user may use them.

Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
---
 Documentation/iio/ad3552r.rst | 65 +++++++++++++++++++++++++++++++++++++++++++
 Documentation/iio/index.rst   |  1 +
 MAINTAINERS                   |  1 +
 3 files changed, 67 insertions(+)

diff --git a/Documentation/iio/ad3552r.rst b/Documentation/iio/ad3552r.rst
new file mode 100644
index 0000000000000000000000000000000000000000..638a62c99fb876cca026a0b1df469c81ba39ff29
--- /dev/null
+++ b/Documentation/iio/ad3552r.rst
@@ -0,0 +1,65 @@
+.. SPDX-License-Identifier: GPL-2.0-only
+
+==============
+AD3552R driver
+==============
+
+Device driver for Analog Devices Inc. AD35XXR series of DACs. The module name
+is ``ad3552r``.
+With the same module name, two different driver variants are available, the
+``generic spi`` variant, to be used with any classic SPI controllers, and the
+``hs`` (high speed) variant, for an ADI ``axi-dac`` (IP core) based controller
+that allows to reach the maximum sample rate supported from the DACs, using the
+DMA transfer and all the SPI lines available (D/QDSPI)..
+The high speed driver variant is intended to be used with the ``adi-axi-dac``
+backend support enabled, that is enabled by default when the driver is selected.
+
+Supported devices
+=================
+
+* `AD3541R <https://www.analog.com/en/products/ad3541r.html>`_
+* `AD3542R <https://www.analog.com/en/products/ad3542r.html>`_
+* `AD3551R <https://www.analog.com/en/products/ad3551r.html>`_
+* `AD3552R <https://www.analog.com/en/products/ad3552r.html>`_
+
+Wiring connections
+------------------
+
+::
+
+    .-----------------.                .-------.
+    |                 |--- D/QSPI -----|       |
+    |   DAC IP CORE   |--- SPI S_CLK --|  DAC  |
+    |                 |--- SPI CS -----|       |
+    |                 |--- LDAC -------|       |
+    |                 |--- RESET ------|       |
+    |_________________|                |_______|
+
+
+High speed features
+===================
+
+Device attributes
+-----------------
+
+The following table shows the ad35xxr related device debug files, found in the
+specific device debug folder path ``/sys/kernel/debug/iio/iio:deviceX``.
+
++----------------------+-------------------------------------------------------+
+| Debugfs device files | Description                                           |
++----------------------+-------------------------------------------------------+
+| data_source          | The used data source,                                 |
+|                      | as ``iio-buffer`` or ``backend-ramp-generator``.      |
++----------------------+-------------------------------------------------------+
+
+Usage examples
+--------------
+
+. code-block:: bash
+	root:/sys/bus/iio/devices/iio:device0# cat data_source
+	iio-buffer
+	root:/sys/bus/iio/devices/iio:device0# echo -n backend-ramp-generator > data_source
+	root:/sys/bus/iio/devices/iio:device0# cat data_source⏎
+	backend-ramp-generator
+
+
diff --git a/Documentation/iio/index.rst b/Documentation/iio/index.rst
index bbb2edce8272e7483acca500d1a757bbcc11c1e0..2d6afc5a8ed54a90cd8d5723f0dc5212b8593d16 100644
--- a/Documentation/iio/index.rst
+++ b/Documentation/iio/index.rst
@@ -19,6 +19,7 @@ Industrial I/O Kernel Drivers
 .. toctree::
    :maxdepth: 1
 
+   ad3552r
    ad4000
    ad4030
    ad4695
diff --git a/MAINTAINERS b/MAINTAINERS
index 57eaab00f6cb53df52a4799eb2c1afbbd1e77a1e..52bc56a9ee22c66b90555681c4757ea4399adae1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1295,6 +1295,7 @@ L:	linux-iio@vger.kernel.org
 S:	Supported
 W:	https://ez.analog.com/linux-software-drivers
 F:	Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
+F:	Documentation/iio/ad3552r.rst
 F:	drivers/iio/dac/ad3552r.c
 
 ANALOG DEVICES INC AD4000 DRIVER

-- 
2.49.0

Re: [PATCH 1/4] docs: iio: add documentation for ad3552r driver

[Marcelo Schmitt]

The doc seems to be all about the high-speed setup despite classical SPI support
being mentioned. It would be interesting to see how the regular SPI and hs
ad3552r IIO devices differ from each other (wiring connections, IIO device
interfaces (attributes, debug files, ...), any other relevant peculiarities).
Some comments about that inline.

On 03/21, Angelo Dureghello wrote:
> From: Angelo Dureghello <adureghello@baylibre.com>
> 
> Add documentation for ad3552r driver, needed to describe the high-speed
> driver debugfs attributes and shows how the user may use them.
> 
> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> ---
...
> +==============
> +AD3552R driver
> +==============
> +
> +Device driver for Analog Devices Inc. AD35XXR series of DACs. The module name
> +is ``ad3552r``.
> +With the same module name, two different driver variants are available, the
> +``generic spi`` variant, to be used with any classic SPI controllers, and the
> +``hs`` (high speed) variant, for an ADI ``axi-dac`` (IP core) based controller
> +that allows to reach the maximum sample rate supported from the DACs, using the
> +DMA transfer and all the SPI lines available (D/QDSPI)..
Is D/QDSPI about dual and quad SPI? If so, what about saying that more clearly? 

> +The high speed driver variant is intended to be used with the ``adi-axi-dac``
> +backend support enabled, that is enabled by default when the driver is selected.
> +
> +Supported devices
> +=================
> +
> +* `AD3541R <https://www.analog.com/en/products/ad3541r.html>`_
> +* `AD3542R <https://www.analog.com/en/products/ad3542r.html>`_
> +* `AD3551R <https://www.analog.com/en/products/ad3551r.html>`_
> +* `AD3552R <https://www.analog.com/en/products/ad3552r.html>`_
> +
> +Wiring connections
> +------------------
> +
> +::
> +
> +    .-----------------.                .-------.
> +    |                 |--- D/QSPI -----|       |
> +    |   DAC IP CORE   |--- SPI S_CLK --|  DAC  |
> +    |                 |--- SPI CS -----|       |
> +    |                 |--- LDAC -------|       |
> +    |                 |--- RESET ------|       |
> +    |_________________|                |_______|

This only describes how the HDL IP connects to the DAC which is the high speed
use case. Maybe add a diagram for the regular SPI connection wiring or say that
the above is only for the hs setup?
Also, what about adding a link to the HDL documentation page?
https://analogdevicesinc.github.io/hdl/projects/ad35xxr_evb/index.html

> +
> +
> +High speed features
> +===================
> +
> +Device attributes
> +-----------------
This is only describing the debugfs file. What about also listing the usual
IIO device channels and attributes (out_voltageX_raw, out_voltageX_en, ...)?

> +
> +The following table shows the ad35xxr related device debug files, found in the
> +specific device debug folder path ``/sys/kernel/debug/iio/iio:deviceX``.
> +
> ++----------------------+-------------------------------------------------------+
> +| Debugfs device files | Description                                           |
> ++----------------------+-------------------------------------------------------+
> +| data_source          | The used data source,                                 |
> +|                      | as ``iio-buffer`` or ``backend-ramp-generator``.      |
> ++----------------------+-------------------------------------------------------+
> +

Re: [PATCH 1/4] docs: iio: add documentation for ad3552r driver

[Angelo Dureghello]

Hi Marcelo,

On 26.03.2025 19:28, Marcelo Schmitt wrote:
> The doc seems to be all about the high-speed setup despite classical SPI support
> being mentioned. It would be interesting to see how the regular SPI and hs
> ad3552r IIO devices differ from each other (wiring connections, IIO device
> interfaces (attributes, debug files, ...), any other relevant peculiarities).
> Some comments about that inline.
> 

had to add this file mainly to describe ramp generator usage.

The ad3552r (classic SPI) is quite old stuff, may work with whatever
controller with classic simple SPI (SDI/SDO/S_CLK/CS) so no particular
wiring diagram or explainations should be needed. 

> On 03/21, Angelo Dureghello wrote:
> > From: Angelo Dureghello <adureghello@baylibre.com>
> > 
> > Add documentation for ad3552r driver, needed to describe the high-speed
> > driver debugfs attributes and shows how the user may use them.
> > 
> > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> > ---
> ...
> > +==============
> > +AD3552R driver
> > +==============
> > +
> > +Device driver for Analog Devices Inc. AD35XXR series of DACs. The module name
> > +is ``ad3552r``.
> > +With the same module name, two different driver variants are available, the
> > +``generic spi`` variant, to be used with any classic SPI controllers, and the
> > +``hs`` (high speed) variant, for an ADI ``axi-dac`` (IP core) based controller
> > +that allows to reach the maximum sample rate supported from the DACs, using the
> > +DMA transfer and all the SPI lines available (D/QDSPI)..
> Is D/QDSPI about dual and quad SPI? If so, what about saying that more clearly? 
> 
> > +The high speed driver variant is intended to be used with the ``adi-axi-dac``
> > +backend support enabled, that is enabled by default when the driver is selected.
> > +
> > +Supported devices
> > +=================
> > +
> > +* `AD3541R <https://www.analog.com/en/products/ad3541r.html>`_
> > +* `AD3542R <https://www.analog.com/en/products/ad3542r.html>`_
> > +* `AD3551R <https://www.analog.com/en/products/ad3551r.html>`_
> > +* `AD3552R <https://www.analog.com/en/products/ad3552r.html>`_
> > +
> > +Wiring connections
> > +------------------
> > +
> > +::
> > +
> > +    .-----------------.                .-------.
> > +    |                 |--- D/QSPI -----|       |
> > +    |   DAC IP CORE   |--- SPI S_CLK --|  DAC  |
> > +    |                 |--- SPI CS -----|       |
> > +    |                 |--- LDAC -------|       |
> > +    |                 |--- RESET ------|       |
> > +    |_________________|                |_______|
> 
> This only describes how the HDL IP connects to the DAC which is the high speed
> use case. Maybe add a diagram for the regular SPI connection wiring or say that
> the above is only for the hs setup?
> Also, what about adding a link to the HDL documentation page?
> https://analogdevicesinc.github.io/hdl/projects/ad35xxr_evb/index.html
> 
> > +
> > +
> > +High speed features
> > +===================
> > +
> > +Device attributes
> > +-----------------
> This is only describing the debugfs file. What about also listing the usual
> IIO device channels and attributes (out_voltageX_raw, out_voltageX_en, ...)?
> 

they are already documented, since part of the iio stuff.
Please see Documentation/ABI/testing/sysfs-bus-iio.

> > +
> > +The following table shows the ad35xxr related device debug files, found in the
> > +specific device debug folder path ``/sys/kernel/debug/iio/iio:deviceX``.
> > +
> > ++----------------------+-------------------------------------------------------+
> > +| Debugfs device files | Description                                           |
> > ++----------------------+-------------------------------------------------------+
> > +| data_source          | The used data source,                                 |
> > +|                      | as ``iio-buffer`` or ``backend-ramp-generator``.      |
> > ++----------------------+-------------------------------------------------------+
> > +

Reagrds,
angelo

Re: [PATCH 1/4] docs: iio: add documentation for ad3552r driver

[Marcelo Schmitt]

Hi Angelo,

On 03/27, Angelo Dureghello wrote:
> Hi Marcelo,
> 
> On 26.03.2025 19:28, Marcelo Schmitt wrote:
> > The doc seems to be all about the high-speed setup despite classical SPI support
> > being mentioned. It would be interesting to see how the regular SPI and hs
> > ad3552r IIO devices differ from each other (wiring connections, IIO device
> > interfaces (attributes, debug files, ...), any other relevant peculiarities).
> > Some comments about that inline.
> > 
> 
> had to add this file mainly to describe ramp generator usage.
> 
> The ad3552r (classic SPI) is quite old stuff, may work with whatever
> controller with classic simple SPI (SDI/SDO/S_CLK/CS) so no particular
> wiring diagram or explainations should be needed. 
> 
Okay, then maybe say so in the doc too. Otherwise this looks like a doc about
ad3552r driver that is said to support both SPI and hs SPI setups, but only
info about hs SPI is provided.

> > On 03/21, Angelo Dureghello wrote:
> > > From: Angelo Dureghello <adureghello@baylibre.com>
> > > 
> > > Add documentation for ad3552r driver, needed to describe the high-speed
> > > driver debugfs attributes and shows how the user may use them.
> > > 
> > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> > > ---
...
> > > +High speed features
> > > +===================
> > > +
> > > +Device attributes
> > > +-----------------
> > This is only describing the debugfs file. What about also listing the usual
> > IIO device channels and attributes (out_voltageX_raw, out_voltageX_en, ...)?
> > 
> 
> they are already documented, since part of the iio stuff.
> Please see Documentation/ABI/testing/sysfs-bus-iio.

Agree, the sysfs interface we often refer to as device attributes is documented
in sysfs-bus-iio. Though, some of the docs for other IIO drivers complement the
ABI doc by elaborating on the regular sysfs interface they provide. If there
would be anything else to say about AD3552R interface, it could be added here.
Anyway, I think it's also fine to only have the debugfs if there is nothing
special about the other sysfs files.

Regards,
Marcelo

Re: [PATCH 1/4] docs: iio: add documentation for ad3552r driver

[Jonathan Cameron]

On Fri, 21 Mar 2025 21:28:48 +0100
Angelo Dureghello <adureghello@baylibre.com> wrote:

> From: Angelo Dureghello <adureghello@baylibre.com>
> 
> Add documentation for ad3552r driver, needed to describe the high-speed
> driver debugfs attributes and shows how the user may use them.
> 
> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> ---
>  Documentation/iio/ad3552r.rst | 65 +++++++++++++++++++++++++++++++++++++++++++
>  Documentation/iio/index.rst   |  1 +
>  MAINTAINERS                   |  1 +
>  3 files changed, 67 insertions(+)
> 
> diff --git a/Documentation/iio/ad3552r.rst b/Documentation/iio/ad3552r.rst
> new file mode 100644
> index 0000000000000000000000000000000000000000..638a62c99fb876cca026a0b1df469c81ba39ff29
> --- /dev/null
> +++ b/Documentation/iio/ad3552r.rst
> @@ -0,0 +1,65 @@
> +.. SPDX-License-Identifier: GPL-2.0-only
> +
> +==============
> +AD3552R driver
> +==============
> +
> +Device driver for Analog Devices Inc. AD35XXR series of DACs. The module name
> +is ``ad3552r``.
> +With the same module name, two different driver variants are available, the
> +``generic spi`` variant, to be used with any classic SPI controllers, and the
> +``hs`` (high speed) variant, for an ADI ``axi-dac`` (IP core) based controller
> +that allows to reach the maximum sample rate supported from the DACs, using the
> +DMA transfer and all the SPI lines available (D/QDSPI)..
> +The high speed driver variant is intended to be used with the ``adi-axi-dac``
> +backend support enabled, that is enabled by default when the driver is selected.
> +
> +Supported devices
> +=================
> +
> +* `AD3541R <https://www.analog.com/en/products/ad3541r.html>`_
> +* `AD3542R <https://www.analog.com/en/products/ad3542r.html>`_
> +* `AD3551R <https://www.analog.com/en/products/ad3551r.html>`_
> +* `AD3552R <https://www.analog.com/en/products/ad3552r.html>`_
> +
> +Wiring connections
> +------------------
> +
> +::
> +
> +    .-----------------.                .-------.
> +    |                 |--- D/QSPI -----|       |
> +    |   DAC IP CORE   |--- SPI S_CLK --|  DAC  |
> +    |                 |--- SPI CS -----|       |
> +    |                 |--- LDAC -------|       |
> +    |                 |--- RESET ------|       |
> +    |_________________|                |_______|
> +
> +
> +High speed features
> +===================
> +
> +Device attributes
> +-----------------
> +
> +The following table shows the ad35xxr related device debug files, found in the
> +specific device debug folder path ``/sys/kernel/debug/iio/iio:deviceX``.
> +
> ++----------------------+-------------------------------------------------------+
> +| Debugfs device files | Description                                           |
> ++----------------------+-------------------------------------------------------+
> +| data_source          | The used data source,                                 |
> +|                      | as ``iio-buffer`` or ``backend-ramp-generator``.      |
> ++----------------------+-------------------------------------------------------+
Hmm. I'm not convinced yet that this is something that belongs in debugfs but will
read on through the series.

> +
> +Usage examples
> +--------------
> +
> +. code-block:: bash
> +	root:/sys/bus/iio/devices/iio:device0# cat data_source
> +	iio-buffer
> +	root:/sys/bus/iio/devices/iio:device0# echo -n backend-ramp-generator > data_source
> +	root:/sys/bus/iio/devices/iio:device0# cat data_source⏎

Not sure what the trailing character is.

> +	backend-ramp-generator
> +
> +
> diff --git a/Documentation/iio/index.rst b/Documentation/iio/index.rst
> index bbb2edce8272e7483acca500d1a757bbcc11c1e0..2d6afc5a8ed54a90cd8d5723f0dc5212b8593d16 100644
> --- a/Documentation/iio/index.rst
> +++ b/Documentation/iio/index.rst
> @@ -19,6 +19,7 @@ Industrial I/O Kernel Drivers
>  .. toctree::
>     :maxdepth: 1
>  
> +   ad3552r
>     ad4000
>     ad4030
>     ad4695
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 57eaab00f6cb53df52a4799eb2c1afbbd1e77a1e..52bc56a9ee22c66b90555681c4757ea4399adae1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1295,6 +1295,7 @@ L:	linux-iio@vger.kernel.org
>  S:	Supported
>  W:	https://ez.analog.com/linux-software-drivers
>  F:	Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
> +F:	Documentation/iio/ad3552r.rst
>  F:	drivers/iio/dac/ad3552r.c
>  
>  ANALOG DEVICES INC AD4000 DRIVER
> 

[PATCH 2/4] iio: backend: add support for data source get

[Angelo Dureghello]

From: Angelo Dureghello <adureghello@baylibre.com>

Add backend support for getting the data source used.

The ad3552r HDL implements an internal ramp generator, so adding the
getter to allow data source get/set by debugfs.

Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
---
 drivers/iio/industrialio-backend.c | 28 ++++++++++++++++++++++++++++
 include/linux/iio/backend.h        |  5 +++++
 2 files changed, 33 insertions(+)

diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c
index a43c8d1bb3d0f4dda4277cac94b0ea9232c071e4..c1eb9ef9db08aec8437d0d00cf77914ad6611b72 100644
--- a/drivers/iio/industrialio-backend.c
+++ b/drivers/iio/industrialio-backend.c
@@ -380,6 +380,34 @@ int iio_backend_data_source_set(struct iio_backend *back, unsigned int chan,
 }
 EXPORT_SYMBOL_NS_GPL(iio_backend_data_source_set, "IIO_BACKEND");
 
+/**
+ * iio_backend_data_source_get - Get current data source
+ * @back: Backend device
+ * @chan: Channel number
+ * @data: Pointer to receive the current source value
+ *
+ * A given backend may have different sources to stream/sync data. This allows
+ * to know what source is in use.
+ *
+ * RETURNS:
+ * 0 on success, negative error number on failure.
+ */
+int iio_backend_data_source_get(struct iio_backend *back, unsigned int chan,
+				enum iio_backend_data_source *data)
+{
+	int ret;
+
+	ret = iio_backend_op_call(back, data_source_get, chan, data);
+	if (ret)
+		return ret;
+
+	if (*data >= IIO_BACKEND_DATA_SOURCE_MAX)
+		return -EINVAL;
+
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(iio_backend_data_source_get, "IIO_BACKEND");
+
 /**
  * iio_backend_set_sampling_freq - Set channel sampling rate
  * @back: Backend device
diff --git a/include/linux/iio/backend.h b/include/linux/iio/backend.h
index e45b7dfbec35c094942a3034fc6057a7960b9772..e59d909cb65924b4872cadd4b7e5e894c13c189f 100644
--- a/include/linux/iio/backend.h
+++ b/include/linux/iio/backend.h
@@ -84,6 +84,7 @@ enum iio_backend_interface_type {
  * @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.
+ * @data_source_get: Data source getter 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.
@@ -115,6 +116,8 @@ struct iio_backend_ops {
 			       const struct iio_backend_data_fmt *data);
 	int (*data_source_set)(struct iio_backend *back, unsigned int chan,
 			       enum iio_backend_data_source data);
+	int (*data_source_get)(struct iio_backend *back, unsigned int chan,
+			       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,
@@ -176,6 +179,8 @@ int iio_backend_data_format_set(struct iio_backend *back, unsigned int chan,
 				const struct iio_backend_data_fmt *data);
 int iio_backend_data_source_set(struct iio_backend *back, unsigned int chan,
 				enum iio_backend_data_source data);
+int iio_backend_data_source_get(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,

-- 
2.49.0

[PATCH 3/4] iio: dac: adi-axi-dac: add data source get

[Angelo Dureghello]

From: Angelo Dureghello <adureghello@baylibre.com>

Add data source getter.

Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
---
 drivers/iio/dac/adi-axi-dac.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c
index 892d770aec69c4259de777058801c9ab33c79923..a6abd828ebdb34800cc08a2151e52a9acda9eba1 100644
--- a/drivers/iio/dac/adi-axi-dac.c
+++ b/drivers/iio/dac/adi-axi-dac.c
@@ -514,6 +514,32 @@ static int axi_dac_data_source_set(struct iio_backend *back, unsigned int chan,
 	}
 }
 
+static int axi_dac_data_source_get(struct iio_backend *back, unsigned int chan,
+				   enum iio_backend_data_source *data)
+{
+	struct axi_dac_state *st = iio_backend_get_priv(back);
+	int ret;
+	u32 val;
+
+	ret = regmap_read(st->regmap, AXI_DAC_CHAN_CNTRL_7_REG(chan), &val);
+	if (ret)
+		return ret;
+
+	switch (val) {
+	case AXI_DAC_DATA_INTERNAL_TONE:
+		*data = IIO_BACKEND_INTERNAL_CONTINUOUS_WAVE;
+		return 0;
+	case AXI_DAC_DATA_DMA:
+		*data = IIO_BACKEND_EXTERNAL;
+		return 0;
+	case AXI_DAC_DATA_INTERNAL_RAMP_16BIT:
+		*data = IIO_BACKEND_INTERNAL_RAMP_16BIT;
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
 static int axi_dac_set_sample_rate(struct iio_backend *back, unsigned int chan,
 				   u64 sample_rate)
 {
@@ -794,6 +820,7 @@ static const struct iio_backend_ops axi_ad3552r_ops = {
 	.request_buffer = axi_dac_request_buffer,
 	.free_buffer = axi_dac_free_buffer,
 	.data_source_set = axi_dac_data_source_set,
+	.data_source_get = axi_dac_data_source_get,
 	.ddr_enable = axi_dac_ddr_enable,
 	.ddr_disable = axi_dac_ddr_disable,
 	.data_stream_enable = axi_dac_data_stream_enable,

-- 
2.49.0

Re: [PATCH 3/4] iio: dac: adi-axi-dac: add data source get

[Nuno Sá]

On Fri, 2025-03-21 at 21:28 +0100, Angelo Dureghello wrote:
> From: Angelo Dureghello <adureghello@baylibre.com>
> 
> Add data source getter.
> 
> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> ---
>  drivers/iio/dac/adi-axi-dac.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c
> index
> 892d770aec69c4259de777058801c9ab33c79923..a6abd828ebdb34800cc08a2151e52a9acda9eba1
> 100644
> --- a/drivers/iio/dac/adi-axi-dac.c
> +++ b/drivers/iio/dac/adi-axi-dac.c
> @@ -514,6 +514,32 @@ static int axi_dac_data_source_set(struct iio_backend *back,
> unsigned int chan,
>  	}
>  }
>  
> +static int axi_dac_data_source_get(struct iio_backend *back, unsigned int chan,
> +				   enum iio_backend_data_source *data)
> +{
> +	struct axi_dac_state *st = iio_backend_get_priv(back);
> +	int ret;
> +	u32 val;
> +
> +	ret = regmap_read(st->regmap, AXI_DAC_CHAN_CNTRL_7_REG(chan), &val);
> +	if (ret)
> +		return ret;

Is chan something that we can validate? Do we reliable know max number of channels?

> +
> +	switch (val) {
> +	case AXI_DAC_DATA_INTERNAL_TONE:
> +		*data = IIO_BACKEND_INTERNAL_CONTINUOUS_WAVE;
> +		return 0;
> +	case AXI_DAC_DATA_DMA:
> +		*data = IIO_BACKEND_EXTERNAL;
> +		return 0;
> +	case AXI_DAC_DATA_INTERNAL_RAMP_16BIT:
> +		*data = IIO_BACKEND_INTERNAL_RAMP_16BIT;
> +		return 0;
> +	default:
> +		return -EINVAL;

More of a nitpick comment but I would some other error code. This is not really an
"Invalid argument" situation. Maybe -EIO as the HW is giving something unexpected? or
ENOTSUPP (likely not exactly like this)...

- Nuno Sá

>  	.ddr_disable = axi_dac_ddr_disable,
>  	.data_stream_enable = axi_dac_data_stream_enable,
> 

Re: [PATCH 3/4] iio: dac: adi-axi-dac: add data source get

[Angelo Dureghello]

On 28.03.2025 08:15, Nuno Sá wrote:
> On Fri, 2025-03-21 at 21:28 +0100, Angelo Dureghello wrote:
> > From: Angelo Dureghello <adureghello@baylibre.com>
> > 
> > Add data source getter.
> > 
> > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> > ---
> >  drivers/iio/dac/adi-axi-dac.c | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> > 
> > diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c
> > index
> > 892d770aec69c4259de777058801c9ab33c79923..a6abd828ebdb34800cc08a2151e52a9acda9eba1
> > 100644
> > --- a/drivers/iio/dac/adi-axi-dac.c
> > +++ b/drivers/iio/dac/adi-axi-dac.c
> > @@ -514,6 +514,32 @@ static int axi_dac_data_source_set(struct iio_backend *back,
> > unsigned int chan,
> >  	}
> >  }
> >  
> > +static int axi_dac_data_source_get(struct iio_backend *back, unsigned int chan,
> > +				   enum iio_backend_data_source *data)
> > +{
> > +	struct axi_dac_state *st = iio_backend_get_priv(back);
> > +	int ret;
> > +	u32 val;
> > +
> > +	ret = regmap_read(st->regmap, AXI_DAC_CHAN_CNTRL_7_REG(chan), &val);
> > +	if (ret)
> > +		return ret;
> 
> Is chan something that we can validate? Do we reliable know max number of channels?
>

Ack, will set it to 15, 0 to 15 is what the documentation says.
But at this point should be set in a diffetrent patch for the "set"
too.

Btw, there is something odd here. I tested the generator and it works,
i am enabling RAMP for both channels 0 and 1.
The channel offset here is 0x40, while in ad3552r and generic dac doc
it is 0x58. So, since ramp is working i am supposing the documentation 
can be wrong. 
 
> > +
> > +	switch (val) {
> > +	case AXI_DAC_DATA_INTERNAL_TONE:
> > +		*data = IIO_BACKEND_INTERNAL_CONTINUOUS_WAVE;
> > +		return 0;
> > +	case AXI_DAC_DATA_DMA:
> > +		*data = IIO_BACKEND_EXTERNAL;
> > +		return 0;
> > +	case AXI_DAC_DATA_INTERNAL_RAMP_16BIT:
> > +		*data = IIO_BACKEND_INTERNAL_RAMP_16BIT;
> > +		return 0;
> > +	default:
> > +		return -EINVAL;
> 
> More of a nitpick comment but I would some other error code. This is not really an
> "Invalid argument" situation. Maybe -EIO as the HW is giving something unexpected? or
> ENOTSUPP (likely not exactly like this)...
>
Correct, will set EIO.
 
> - Nuno Sá
> 
> >  	.ddr_disable = axi_dac_ddr_disable,
> >  	.data_stream_enable = axi_dac_data_stream_enable,
> > 
> 

[PATCH 4/4] iio: dac: ad3552r-hs: add support for internal ramp

[Angelo Dureghello]

From: Angelo Dureghello <adureghello@baylibre.com>

The ad3552r can be feeded from the HDL controller by an internally
generated 16bit ramp, useful for debug pourposes. Add debugfs a file
to enable or disable it.

Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
---
 drivers/iio/dac/ad3552r-hs.c | 106 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 100 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/dac/ad3552r-hs.c b/drivers/iio/dac/ad3552r-hs.c
index 37397e188f225a8099745ec03f7c604da76960b1..41fe78d982a68980db059b095fc27b37ea1a461b 100644
--- a/drivers/iio/dac/ad3552r-hs.c
+++ b/drivers/iio/dac/ad3552r-hs.c
@@ -7,6 +7,7 @@
  */
 
 #include <linux/bitfield.h>
+#include <linux/debugfs.h>
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
 #include <linux/iio/backend.h>
@@ -65,6 +66,18 @@ static int ad3552r_hs_reg_read(struct ad3552r_hs_state *st, u32 reg, u32 *val,
 	return st->data->bus_reg_read(st->back, reg, val, xfer_size);
 }
 
+static int ad3552r_hs_set_data_source(struct ad3552r_hs_state *st,
+				      enum iio_backend_data_source type)
+{
+	int ret;
+
+	ret = iio_backend_data_source_set(st->back, 0, type);
+	if (ret)
+		return ret;
+
+	return iio_backend_data_source_set(st->back, 1, type);
+}
+
 static int ad3552r_hs_update_reg_bits(struct ad3552r_hs_state *st, u32 reg,
 				      u32 mask, u32 val, size_t xfer_size)
 {
@@ -483,6 +496,66 @@ static int ad3552r_hs_reg_access(struct iio_dev *indio_dev, unsigned int reg,
 	return st->data->bus_reg_write(st->back, reg, writeval, 1);
 }
 
+static ssize_t ad3552r_hs_show_data_source(struct file *f, char __user *userbuf,
+					   size_t count, loff_t *ppos)
+{
+	struct ad3552r_hs_state *st = file_inode(f)->i_private;
+	enum iio_backend_data_source type;
+	int ret;
+
+	ret = iio_backend_data_source_get(st->back, 0, &type);
+	if (ret)
+		return ret;
+
+	switch (type) {
+	case IIO_BACKEND_INTERNAL_RAMP_16BIT:
+		return simple_read_from_buffer(userbuf, count, ppos,
+					       "backend-ramp-generator", 22);
+	case IIO_BACKEND_EXTERNAL:
+		return simple_read_from_buffer(userbuf, count, ppos,
+					       "iio-buffer", 10);
+	default:
+		return -EINVAL;
+	}
+}
+
+static ssize_t ad3552r_hs_write_data_source(struct file *f,
+					    const char __user *userbuf,
+					    size_t count, loff_t *ppos)
+{
+	struct ad3552r_hs_state *st = file_inode(f)->i_private;
+	char buf[64];
+	int ret;
+
+	ret = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf,
+				     count);
+	if (ret < 0)
+		return ret;
+
+	buf[count] = 0;
+
+	if (count == 10 && !strncmp(buf, "iio-buffer", 10)) {
+		ret = ad3552r_hs_set_data_source(st, IIO_BACKEND_EXTERNAL);
+		if (ret)
+			return ret;
+	} else if (count == 22 && !strncmp(buf, "backend-ramp-generator", 22)) {
+		ret = ad3552r_hs_set_data_source(st,
+			IIO_BACKEND_INTERNAL_RAMP_16BIT);
+		if (ret)
+			return ret;
+	} else {
+		return -EINVAL;
+	}
+
+	return count;
+}
+
+static const struct file_operations ad3552r_hs_data_source_fops = {
+	.owner = THIS_MODULE,
+	.write = ad3552r_hs_write_data_source,
+	.read = ad3552r_hs_show_data_source,
+};
+
 static int ad3552r_hs_setup(struct ad3552r_hs_state *st)
 {
 	u16 id;
@@ -550,11 +623,7 @@ static int ad3552r_hs_setup(struct ad3552r_hs_state *st)
 	if (ret)
 		return ret;
 
-	ret = iio_backend_data_source_set(st->back, 0, IIO_BACKEND_EXTERNAL);
-	if (ret)
-		return ret;
-
-	ret = iio_backend_data_source_set(st->back, 1, IIO_BACKEND_EXTERNAL);
+	ret = ad3552r_hs_set_data_source(st, IIO_BACKEND_EXTERNAL);
 	if (ret)
 		return ret;
 
@@ -661,6 +730,24 @@ static const struct iio_info ad3552r_hs_info = {
 	.debugfs_reg_access = &ad3552r_hs_reg_access,
 };
 
+static void ad3552r_hs_debugfs_init(struct iio_dev *indio_dev)
+{
+	struct ad3552r_hs_state *st = iio_priv(indio_dev);
+	struct dentry *d = iio_get_debugfs_dentry(indio_dev);
+
+	if (!IS_ENABLED(CONFIG_DEBUG_FS))
+		return;
+
+	d = iio_get_debugfs_dentry(indio_dev);
+	if (!d) {
+		dev_warn(st->dev, "can't set debugfs in driver dir\n");
+		return;
+	}
+
+	debugfs_create_file("data_source", 0600, d, st,
+			    &ad3552r_hs_data_source_fops);
+}
+
 static int ad3552r_hs_probe(struct platform_device *pdev)
 {
 	struct ad3552r_hs_state *st;
@@ -705,7 +792,14 @@ static int ad3552r_hs_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	return devm_iio_device_register(&pdev->dev, indio_dev);
+	ret = devm_iio_device_register(&pdev->dev, indio_dev);
+	if (ret)
+		return ret;
+
+	ad3552r_hs_debugfs_init(indio_dev);
+
+	return ret;
+
 }
 
 static const struct of_device_id ad3552r_hs_of_id[] = {

-- 
2.49.0

Re: [PATCH 4/4] iio: dac: ad3552r-hs: add support for internal ramp

[Marcelo Schmitt]

Hello Angelo,

Patch looks good to me.
One minor comment bellow.

On 03/21, Angelo Dureghello wrote:
> From: Angelo Dureghello <adureghello@baylibre.com>
> 
> The ad3552r can be feeded from the HDL controller by an internally
> generated 16bit ramp, useful for debug pourposes. Add debugfs a file
> to enable or disable it.
> 
> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> ---
>  drivers/iio/dac/ad3552r-hs.c | 106 ++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 100 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/dac/ad3552r-hs.c b/drivers/iio/dac/ad3552r-hs.c
> index 37397e188f225a8099745ec03f7c604da76960b1..41fe78d982a68980db059b095fc27b37ea1a461b 100644
> --- a/drivers/iio/dac/ad3552r-hs.c
> +++ b/drivers/iio/dac/ad3552r-hs.c
> @@ -7,6 +7,7 @@
...
> +
> +static ssize_t ad3552r_hs_write_data_source(struct file *f,
> +					    const char __user *userbuf,
> +					    size_t count, loff_t *ppos)
> +{
> +	struct ad3552r_hs_state *st = file_inode(f)->i_private;
> +	char buf[64];
> +	int ret;
> +
> +	ret = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf,
> +				     count);
> +	if (ret < 0)
> +		return ret;
> +
> +	buf[count] = 0;
Shouldn't it be
	buf[count] = '\0';
?

Re: [PATCH 4/4] iio: dac: ad3552r-hs: add support for internal ramp

[Angelo Dureghello]

On 26.03.2025 18:52, Marcelo Schmitt wrote:
> Hello Angelo,
> 
> Patch looks good to me.
> One minor comment bellow.
> 
> On 03/21, Angelo Dureghello wrote:
> > From: Angelo Dureghello <adureghello@baylibre.com>
> > 
> > The ad3552r can be feeded from the HDL controller by an internally
> > generated 16bit ramp, useful for debug pourposes. Add debugfs a file
> > to enable or disable it.
> > 
> > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> > ---
> >  drivers/iio/dac/ad3552r-hs.c | 106 ++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 100 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/iio/dac/ad3552r-hs.c b/drivers/iio/dac/ad3552r-hs.c
> > index 37397e188f225a8099745ec03f7c604da76960b1..41fe78d982a68980db059b095fc27b37ea1a461b 100644
> > --- a/drivers/iio/dac/ad3552r-hs.c
> > +++ b/drivers/iio/dac/ad3552r-hs.c
> > @@ -7,6 +7,7 @@
> ...
> > +
> > +static ssize_t ad3552r_hs_write_data_source(struct file *f,
> > +					    const char __user *userbuf,
> > +					    size_t count, loff_t *ppos)
> > +{
> > +	struct ad3552r_hs_state *st = file_inode(f)->i_private;
> > +	char buf[64];
> > +	int ret;
> > +
> > +	ret = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf,
> > +				     count);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	buf[count] = 0;
> Shouldn't it be
> 	buf[count] = '\0';

Why ? I am zero-terminating the string properly.

> ?

Regards,
angelo

Re: [PATCH 4/4] iio: dac: ad3552r-hs: add support for internal ramp

[Marcelo Schmitt]

On 03/27, Angelo Dureghello wrote:
> On 26.03.2025 18:52, Marcelo Schmitt wrote:
> > Hello Angelo,
> > 
> > Patch looks good to me.
> > One minor comment bellow.
> > 
> > On 03/21, Angelo Dureghello wrote:
> > > From: Angelo Dureghello <adureghello@baylibre.com>
> > > 
...
> > > +
> > > +static ssize_t ad3552r_hs_write_data_source(struct file *f,
> > > +					    const char __user *userbuf,
> > > +					    size_t count, loff_t *ppos)
> > > +{
> > > +	struct ad3552r_hs_state *st = file_inode(f)->i_private;
> > > +	char buf[64];
> > > +	int ret;
> > > +
> > > +	ret = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf,
> > > +				     count);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	buf[count] = 0;
> > Shouldn't it be
> > 	buf[count] = '\0';
> 
> Why ? I am zero-terminating the string properly.

Oh, okay. I was just more used to see '\0' as buffer/string terminator.
I see now buf[count] = 0; should work too.

> 
> > ?
> 
> Regards,
> angelo

Regards,
Marcelo

Re: [PATCH 4/4] iio: dac: ad3552r-hs: add support for internal ramp

[Nuno Sá]

On Thu, 2025-03-27 at 09:09 -0300, Marcelo Schmitt wrote:
> On 03/27, Angelo Dureghello wrote:
> > On 26.03.2025 18:52, Marcelo Schmitt wrote:
> > > Hello Angelo,
> > > 
> > > Patch looks good to me.
> > > One minor comment bellow.
> > > 
> > > On 03/21, Angelo Dureghello wrote:
> > > > From: Angelo Dureghello <adureghello@baylibre.com>
> > > > 
> ...
> > > > +
> > > > +static ssize_t ad3552r_hs_write_data_source(struct file *f,
> > > > +					    const char __user *userbuf,
> > > > +					    size_t count, loff_t *ppos)
> > > > +{
> > > > +	struct ad3552r_hs_state *st = file_inode(f)->i_private;
> > > > +	char buf[64];
> > > > +	int ret;
> > > > +
> > > > +	ret = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf,
> > > > +				     count);
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	buf[count] = 0;
> > > Shouldn't it be
> > > 	buf[count] = '\0';
> > 
> > Why ? I am zero-terminating the string properly.
> 
> Oh, okay. I was just more used to see '\0' as buffer/string terminator.
> I see now buf[count] = 0; should work too.
> 

I agree with Marcelo that the more natural/readable way for terminating a string is
using the corresponding null character (ascii). Probably not a reason for a v2
though...

- Nuno Sá 

Re: [PATCH 4/4] iio: dac: ad3552r-hs: add support for internal ramp

[Nuno Sá]

On Fri, 2025-03-21 at 21:28 +0100, Angelo Dureghello wrote:
> From: Angelo Dureghello <adureghello@baylibre.com>
> 
> The ad3552r can be feeded from the HDL controller by an internally
> generated 16bit ramp, useful for debug pourposes. Add debugfs a file
> to enable or disable it.
> 
> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> ---
>  drivers/iio/dac/ad3552r-hs.c | 106 ++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 100 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/dac/ad3552r-hs.c b/drivers/iio/dac/ad3552r-hs.c
> index
> 37397e188f225a8099745ec03f7c604da76960b1..41fe78d982a68980db059b095fc27b37ea1a461b
> 100644
> --- a/drivers/iio/dac/ad3552r-hs.c
> +++ b/drivers/iio/dac/ad3552r-hs.c
> @@ -7,6 +7,7 @@
>   */
>  
>  #include <linux/bitfield.h>
> +#include <linux/debugfs.h>
>  #include <linux/delay.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/iio/backend.h>
> @@ -65,6 +66,18 @@ static int ad3552r_hs_reg_read(struct ad3552r_hs_state *st, u32
> reg, u32 *val,
>  	return st->data->bus_reg_read(st->back, reg, val, xfer_size);
>  }
>  
> +static int ad3552r_hs_set_data_source(struct ad3552r_hs_state *st,
> +				      enum iio_backend_data_source type)
> +{
> +	int ret;
> +
> +	ret = iio_backend_data_source_set(st->back, 0, type);
> +	if (ret)
> +		return ret;
> +
> +	return iio_backend_data_source_set(st->back, 1, type);
> 

I know it's a debug thing but we could use some locking in the above...

> +}
> +
>  static int ad3552r_hs_update_reg_bits(struct ad3552r_hs_state *st, u32 reg,
>  				      u32 mask, u32 val, size_t xfer_size)
>  {
> @@ -483,6 +496,66 @@ static int ad3552r_hs_reg_access(struct iio_dev *indio_dev,
> unsigned int reg,
>  	return st->data->bus_reg_write(st->back, reg, writeval, 1);
>  }
>  
> +static ssize_t ad3552r_hs_show_data_source(struct file *f, char __user *userbuf,
> +					   size_t count, loff_t *ppos)
> +{
> +	struct ad3552r_hs_state *st = file_inode(f)->i_private;
> +	enum iio_backend_data_source type;
> +	int ret;
> +
> +	ret = iio_backend_data_source_get(st->back, 0, &type);
> +	if (ret)
> +		return ret;
> +
> +	switch (type) {
> +	case IIO_BACKEND_INTERNAL_RAMP_16BIT:
> +		return simple_read_from_buffer(userbuf, count, ppos,
> +					       "backend-ramp-generator", 22);
> +	case IIO_BACKEND_EXTERNAL:
> +		return simple_read_from_buffer(userbuf, count, ppos,
> +					       "iio-buffer", 10);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static ssize_t ad3552r_hs_write_data_source(struct file *f,
> +					    const char __user *userbuf,
> +					    size_t count, loff_t *ppos)
> +{
> +	struct ad3552r_hs_state *st = file_inode(f)->i_private;
> +	char buf[64];
> +	int ret;
> +
> +	ret = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf,
> +				     count);
> +	if (ret < 0)
> +		return ret;
> +
> +	buf[count] = 0;
> +
> +	if (count == 10 && !strncmp(buf, "iio-buffer", 10)) {
> +		ret = ad3552r_hs_set_data_source(st, IIO_BACKEND_EXTERNAL);
> +		if (ret)
> +			return ret;
> +	} else if (count == 22 && !strncmp(buf, "backend-ramp-generator", 22)) {
> +		ret = ad3552r_hs_set_data_source(st,
> +			IIO_BACKEND_INTERNAL_RAMP_16BIT);
> +		if (ret)
> +			return ret;
> +	} else {
> +		return -EINVAL;
> +	}

Are we expected to add more data types in the future? If not, this could be simply an
enable/disable ramp generator thing... It would be much simpler.

Anyways, you could define a static array and use match_string()?

Lastly, for insterfaces like this, it's always helpful to have an _available kind of
attribute.

- Nuno Sá



>  
>  static const struct of_device_id ad3552r_hs_of_id[] = {
> 

Re: [PATCH 4/4] iio: dac: ad3552r-hs: add support for internal ramp

[David Lechner]

On 3/28/25 3:28 AM, Nuno Sá wrote:
> On Fri, 2025-03-21 at 21:28 +0100, Angelo Dureghello wrote:
>> From: Angelo Dureghello <adureghello@baylibre.com>
>>
>> The ad3552r can be feeded from the HDL controller by an internally
>> generated 16bit ramp, useful for debug pourposes. Add debugfs a file
>> to enable or disable it.
>>
>> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
>> ---

...

>> +static ssize_t ad3552r_hs_write_data_source(struct file *f,
>> +					    const char __user *userbuf,
>> +					    size_t count, loff_t *ppos)
>> +{
>> +	struct ad3552r_hs_state *st = file_inode(f)->i_private;
>> +	char buf[64];
>> +	int ret;
>> +
>> +	ret = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf,
>> +				     count);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	buf[count] = 0;
>> +
>> +	if (count == 10 && !strncmp(buf, "iio-buffer", 10)) {
>> +		ret = ad3552r_hs_set_data_source(st, IIO_BACKEND_EXTERNAL);
>> +		if (ret)
>> +			return ret;
>> +	} else if (count == 22 && !strncmp(buf, "backend-ramp-generator", 22)) {
>> +		ret = ad3552r_hs_set_data_source(st,
>> +			IIO_BACKEND_INTERNAL_RAMP_16BIT);
>> +		if (ret)
>> +			return ret;
>> +	} else {
>> +		return -EINVAL;
>> +	}
> 
> Are we expected to add more data types in the future? If not, this could be simply an
> enable/disable ramp generator thing... It would be much simpler.

Angelo actually had implemented it that way originally. :-)

I suggested to change it to a string because the HDL project for this family
of DACs actually has 3 possibilities for the data source:

	* Selectable input source: DMA/ADC/TEST_RAMP;

And there are other potential sources from the generic AXI DAC like
0x00: internal tone (DDS) that seems somewhat likely to be seen in the future.

> 
> Anyways, you could define a static array and use match_string()?
> 
> Lastly, for insterfaces like this, it's always helpful to have an _available kind of
> attribute.
> 
> - Nuno Sá
> 
> 
> 
>>  
>>  static const struct of_device_id ad3552r_hs_of_id[] = {
>>
> 
> 

Re: [PATCH 4/4] iio: dac: ad3552r-hs: add support for internal ramp

[Jonathan Cameron]

On Fri, 21 Mar 2025 21:28:51 +0100
Angelo Dureghello <adureghello@baylibre.com> wrote:

> From: Angelo Dureghello <adureghello@baylibre.com>
> 
> The ad3552r can be feeded from the HDL controller by an internally
> generated 16bit ramp, useful for debug pourposes. Add debugfs a file
> to enable or disable it.
> 
> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> ---
>  drivers/iio/dac/ad3552r-hs.c | 106 ++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 100 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/dac/ad3552r-hs.c b/drivers/iio/dac/ad3552r-hs.c
> index 37397e188f225a8099745ec03f7c604da76960b1..41fe78d982a68980db059b095fc27b37ea1a461b 100644
> --- a/drivers/iio/dac/ad3552r-hs.c
> +++ b/drivers/iio/dac/ad3552r-hs.c
> @@ -7,6 +7,7 @@
>   */
>  
>  #include <linux/bitfield.h>
> +#include <linux/debugfs.h>
>  #include <linux/delay.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/iio/backend.h>
> @@ -65,6 +66,18 @@ static int ad3552r_hs_reg_read(struct ad3552r_hs_state *st, u32 reg, u32 *val,
>  	return st->data->bus_reg_read(st->back, reg, val, xfer_size);
>  }
>  
> +static int ad3552r_hs_set_data_source(struct ad3552r_hs_state *st,
> +				      enum iio_backend_data_source type)
> +{
> +	int ret;
> +
> +	ret = iio_backend_data_source_set(st->back, 0, type);
> +	if (ret)
> +		return ret;
> +
> +	return iio_backend_data_source_set(st->back, 1, type);
> +}
> +
>  static int ad3552r_hs_update_reg_bits(struct ad3552r_hs_state *st, u32 reg,
>  				      u32 mask, u32 val, size_t xfer_size)
>  {
> @@ -483,6 +496,66 @@ static int ad3552r_hs_reg_access(struct iio_dev *indio_dev, unsigned int reg,
>  	return st->data->bus_reg_write(st->back, reg, writeval, 1);
>  }
>  
> +static ssize_t ad3552r_hs_show_data_source(struct file *f, char __user *userbuf,
> +					   size_t count, loff_t *ppos)
> +{
> +	struct ad3552r_hs_state *st = file_inode(f)->i_private;
> +	enum iio_backend_data_source type;
> +	int ret;
> +
> +	ret = iio_backend_data_source_get(st->back, 0, &type);
> +	if (ret)
> +		return ret;
> +
> +	switch (type) {
> +	case IIO_BACKEND_INTERNAL_RAMP_16BIT:
> +		return simple_read_from_buffer(userbuf, count, ppos,
> +					       "backend-ramp-generator", 22);

Probably better to use a const string and then you can use strlen() to get
the length from that.  I don't much like counting characters!

> +	case IIO_BACKEND_EXTERNAL:
> +		return simple_read_from_buffer(userbuf, count, ppos,
> +					       "iio-buffer", 10);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static ssize_t ad3552r_hs_write_data_source(struct file *f,
> +					    const char __user *userbuf,
> +					    size_t count, loff_t *ppos)
> +{
> +	struct ad3552r_hs_state *st = file_inode(f)->i_private;
> +	char buf[64];
> +	int ret;
> +
> +	ret = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf,
> +				     count);
> +	if (ret < 0)
> +		return ret;
> +
> +	buf[count] = 0;
> +
> +	if (count == 10 && !strncmp(buf, "iio-buffer", 10)) {
> +		ret = ad3552r_hs_set_data_source(st, IIO_BACKEND_EXTERNAL);
> +		if (ret)
> +			return ret;
> +	} else if (count == 22 && !strncmp(buf, "backend-ramp-generator", 22)) {

As above, I'd like to see some strlen() on const strings for this.
FWIW strncmp doesn't care about NULL terminators as such so just ensure you only
compare the characters.


> +		ret = ad3552r_hs_set_data_source(st,
> +			IIO_BACKEND_INTERNAL_RAMP_16BIT);
> +		if (ret)
> +			return ret;
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	return count;
> +}
...

Thanks,

Jonathan

Re: [PATCH 0/4] iio: ad3552r-hs: add support for internal ramp generator

[Jonathan Cameron]

On Fri, 21 Mar 2025 21:28:47 +0100
Angelo Dureghello <adureghello@baylibre.com> wrote:

> Add support to enable the HDL IP core internal ramp generator,
> actually managed by the adi-axi-dac backend. 

What is it for?  Circuit testing or something else?
We have in the past had pattern generators in IIO (currently under
frequency drivers, though I'm not sure what we have in the way of
waveforms in the stuff outside staging) so I'd like to be sure
this is about debug rather than a pattern that is actually expected
to be useful.

Jonathan

> 
> It works this way:
> 
> /sys/bus/iio/devices/iio:device0# echo 1 > buffer0/out_voltage0_en 
> /sys/bus/iio/devices/iio:device0# echo 1 > buffer0/out_voltage1_en                                           
> /sys/bus/iio/devices/iio:device0# echo 1 > buffer0/enable 
> 
> Activating ramp generator:
> 
> /sys/kernel/debug/iio/iio:device0# echo -n backend-ramp-generator > data_source
> 
> Deactivating:
> 
> /sys/kernel/debug/iio/iio:device0# echo -n iio-buffer > data_source
> 
> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> ---
> Angelo Dureghello (4):
>       docs: iio: add documentation for ad3552r driver
>       iio: backend: add support for data source get
>       iio: dac: adi-axi-dac: add data source get
>       iio: dac: ad3552r-hs: add support for internal ramp
> 
>  Documentation/iio/ad3552r.rst      |  65 +++++++++++++++++++++++
>  Documentation/iio/index.rst        |   1 +
>  MAINTAINERS                        |   1 +
>  drivers/iio/dac/ad3552r-hs.c       | 106 ++++++++++++++++++++++++++++++++++---
>  drivers/iio/dac/adi-axi-dac.c      |  27 ++++++++++
>  drivers/iio/industrialio-backend.c |  28 ++++++++++
>  include/linux/iio/backend.h        |   5 ++
>  7 files changed, 227 insertions(+), 6 deletions(-)
> ---
> base-commit: eb870a5af7db1e5ca59330875125230b28e630f9
> change-id: 20250321-wip-bl-ad3552r-fixes-4a386944c170
> 
> Best regards,

Re: [PATCH 0/4] iio: ad3552r-hs: add support for internal ramp generator

[Angelo Dureghello]

Hi Jonathan,

On 30.03.2025 17:53, Jonathan Cameron wrote:
> On Fri, 21 Mar 2025 21:28:47 +0100
> Angelo Dureghello <adureghello@baylibre.com> wrote:
> 
> > Add support to enable the HDL IP core internal ramp generator,
> > actually managed by the adi-axi-dac backend. 
> 
> What is it for?  Circuit testing or something else?
> We have in the past had pattern generators in IIO (currently under
> frequency drivers, though I'm not sure what we have in the way of
> waveforms in the stuff outside staging) so I'd like to be sure
> this is about debug rather than a pattern that is actually expected
> to be useful.
> 

Sorry form some reason seen this too late, just sent a v2.

Anyway, the signal is a tooth wave at 280Hz, not sure that pattern
can be of any use except for some dabug cases, or noise tests.


Regards,
angelo

> Jonathan
> 
> > 
> > It works this way:
> > 
> > /sys/bus/iio/devices/iio:device0# echo 1 > buffer0/out_voltage0_en 
> > /sys/bus/iio/devices/iio:device0# echo 1 > buffer0/out_voltage1_en                                           
> > /sys/bus/iio/devices/iio:device0# echo 1 > buffer0/enable 
> > 
> > Activating ramp generator:
> > 
> > /sys/kernel/debug/iio/iio:device0# echo -n backend-ramp-generator > data_source
> > 
> > Deactivating:
> > 
> > /sys/kernel/debug/iio/iio:device0# echo -n iio-buffer > data_source
> > 
> > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> > ---
> > Angelo Dureghello (4):
> >       docs: iio: add documentation for ad3552r driver
> >       iio: backend: add support for data source get
> >       iio: dac: adi-axi-dac: add data source get
> >       iio: dac: ad3552r-hs: add support for internal ramp
> > 
> >  Documentation/iio/ad3552r.rst      |  65 +++++++++++++++++++++++
> >  Documentation/iio/index.rst        |   1 +
> >  MAINTAINERS                        |   1 +
> >  drivers/iio/dac/ad3552r-hs.c       | 106 ++++++++++++++++++++++++++++++++++---
> >  drivers/iio/dac/adi-axi-dac.c      |  27 ++++++++++
> >  drivers/iio/industrialio-backend.c |  28 ++++++++++
> >  include/linux/iio/backend.h        |   5 ++
> >  7 files changed, 227 insertions(+), 6 deletions(-)
> > ---
> > base-commit: eb870a5af7db1e5ca59330875125230b28e630f9
> > change-id: 20250321-wip-bl-ad3552r-fixes-4a386944c170
> > 
> > Best regards,
> 

Re: [PATCH 0/4] iio: ad3552r-hs: add support for internal ramp generator

[Nuno Sá]

On Mon, 2025-03-31 at 21:11 +0200, Angelo Dureghello wrote:
> Hi Jonathan,
> 
> On 30.03.2025 17:53, Jonathan Cameron wrote:
> > On Fri, 21 Mar 2025 21:28:47 +0100
> > Angelo Dureghello <adureghello@baylibre.com> wrote:
> > 
> > > Add support to enable the HDL IP core internal ramp generator,
> > > actually managed by the adi-axi-dac backend. 
> > 
> > What is it for?  Circuit testing or something else?
> > We have in the past had pattern generators in IIO (currently under
> > frequency drivers, though I'm not sure what we have in the way of
> > waveforms in the stuff outside staging) so I'd like to be sure
> > this is about debug rather than a pattern that is actually expected
> > to be useful.
> > 
> 
> Sorry form some reason seen this too late, just sent a v2.
> 
> Anyway, the signal is a tooth wave at 280Hz, not sure that pattern
> can be of any use except for some dabug cases, or noise tests.
> 

Yes, typical usecase for this is to debug/validate the serial interfaces. I'm
used to this see for LVDS/CMOS though (but I would assume the principle to be
the same).

- Nuno Sá