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

[PATCH v4 0/5] 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>
---
Changes in v4:
- set data source based on hw channels available (model_data),
- use a string array for data_source debugfs attribute,
- modify debugfs accessors to use the string array,
- add new "data_source_available" debugfs attr,
- fix documentation accordingly.
- Link to v3: https://lore.kernel.org/r/20250407-wip-bl-ad3552r-fixes-v3-0-61874065b60f@baylibre.com

Changes in v3:
- add mutex description,
- use devm_mutex_init and check for return value.
- Link to v2: https://lore.kernel.org/r/20250331-wip-bl-ad3552r-fixes-v2-0-cdedb430497e@baylibre.com

Changes in v2:
- doc, add few words for generic spi driver version,
- axi-dac, add a separate patch to check cntrl chan validity,
- axi-dac, return EIO on a wrong source on get, 
- add a lock on debugfs file access,
- use const strings and strlen on file access.
- Link to v1: https://lore.kernel.org/r/20250321-wip-bl-ad3552r-fixes-v1-0-3c1aa249d163@baylibre.com

---
Angelo Dureghello (5):
      iio: dac: adi-axi-dac: add cntrl chan check
      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      |  73 ++++++++++++++++
 Documentation/iio/index.rst        |   1 +
 MAINTAINERS                        |   1 +
 drivers/iio/dac/ad3552r-hs.c       | 166 +++++++++++++++++++++++++++++++++++--
 drivers/iio/dac/adi-axi-dac.c      |  54 ++++++++++++
 drivers/iio/industrialio-backend.c |  28 +++++++
 include/linux/iio/backend.h        |   5 ++
 7 files changed, 322 insertions(+), 6 deletions(-)
---
base-commit: eb870a5af7db1e5ca59330875125230b28e630f9
change-id: 20250321-wip-bl-ad3552r-fixes-4a386944c170

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

[PATCH v4 1/5] iio: dac: adi-axi-dac: add cntrl chan check

[Angelo Dureghello]

From: Angelo Dureghello <adureghello@baylibre.com>

Add validity check on CNTRL_X channels (valid as 0 to 15).

Reviewed-by: Nuno Sá <nuno.sa@analog.com>
Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
---
 drivers/iio/dac/adi-axi-dac.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c
index 892d770aec69c4259de777058801c9ab33c79923..f86acb98b0cffb09bf4d4626f932bf1edc911e2b 100644
--- a/drivers/iio/dac/adi-axi-dac.c
+++ b/drivers/iio/dac/adi-axi-dac.c
@@ -84,6 +84,7 @@
 #define AXI_DAC_CHAN_CNTRL_7_REG(c)		(0x0418 + (c) * 0x40)
 #define   AXI_DAC_CHAN_CNTRL_7_DATA_SEL		GENMASK(3, 0)
 
+#define AXI_DAC_CHAN_CNTRL_MAX			15
 #define AXI_DAC_RD_ADDR(x)			(BIT(7) | (x))
 
 /* 360 degrees in rad */
@@ -186,6 +187,9 @@ static int __axi_dac_frequency_get(struct axi_dac_state *st, unsigned int chan,
 	u32 reg, raw;
 	int ret;
 
+	if (chan > AXI_DAC_CHAN_CNTRL_MAX)
+		return -EINVAL;
+
 	if (!st->dac_clk) {
 		dev_err(st->dev, "Sampling rate is 0...\n");
 		return -EINVAL;
@@ -230,6 +234,9 @@ static int axi_dac_scale_get(struct axi_dac_state *st,
 	int ret, vals[2];
 	u32 reg, raw;
 
+	if (chan->channel > AXI_DAC_CHAN_CNTRL_MAX)
+		return -EINVAL;
+
 	if (tone_2)
 		reg = AXI_DAC_CHAN_CNTRL_3_REG(chan->channel);
 	else
@@ -264,6 +271,9 @@ static int axi_dac_phase_get(struct axi_dac_state *st,
 	u32 reg, raw, phase;
 	int ret, vals[2];
 
+	if (chan->channel > AXI_DAC_CHAN_CNTRL_MAX)
+		return -EINVAL;
+
 	if (tone_2)
 		reg = AXI_DAC_CHAN_CNTRL_4_REG(chan->channel);
 	else
@@ -291,6 +301,9 @@ static int __axi_dac_frequency_set(struct axi_dac_state *st, unsigned int chan,
 	u16 raw;
 	int ret;
 
+	if (chan > AXI_DAC_CHAN_CNTRL_MAX)
+		return -EINVAL;
+
 	if (!sample_rate || freq > sample_rate / 2) {
 		dev_err(st->dev, "Invalid frequency(%u) dac_clk(%llu)\n",
 			freq, sample_rate);
@@ -342,6 +355,9 @@ static int axi_dac_scale_set(struct axi_dac_state *st,
 	u32 raw = 0, reg;
 	int ret;
 
+	if (chan->channel > AXI_DAC_CHAN_CNTRL_MAX)
+		return -EINVAL;
+
 	ret = iio_str_to_fixpoint(buf, 100000, &integer, &frac);
 	if (ret)
 		return ret;
@@ -385,6 +401,9 @@ static int axi_dac_phase_set(struct axi_dac_state *st,
 	u32 raw, reg;
 	int ret;
 
+	if (chan->channel > AXI_DAC_CHAN_CNTRL_MAX)
+		return -EINVAL;
+
 	ret = iio_str_to_fixpoint(buf, 100000, &integer, &frac);
 	if (ret)
 		return ret;
@@ -493,6 +512,9 @@ static int axi_dac_data_source_set(struct iio_backend *back, unsigned int chan,
 {
 	struct axi_dac_state *st = iio_backend_get_priv(back);
 
+	if (chan > AXI_DAC_CHAN_CNTRL_MAX)
+		return -EINVAL;
+
 	switch (data) {
 	case IIO_BACKEND_INTERNAL_CONTINUOUS_WAVE:
 		return regmap_update_bits(st->regmap,
@@ -521,6 +543,8 @@ static int axi_dac_set_sample_rate(struct iio_backend *back, unsigned int chan,
 	unsigned int freq;
 	int ret, tone;
 
+	if (chan > AXI_DAC_CHAN_CNTRL_MAX)
+		return -EINVAL;
 	if (!sample_rate)
 		return -EINVAL;
 	if (st->reg_config & AXI_DAC_CONFIG_DDS_DISABLE)

-- 
2.49.0

[PATCH v4 2/5] 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 | 73 +++++++++++++++++++++++++++++++++++++++++++
 Documentation/iio/index.rst   |  1 +
 MAINTAINERS                   |  1 +
 3 files changed, 75 insertions(+)

diff --git a/Documentation/iio/ad3552r.rst b/Documentation/iio/ad3552r.rst
new file mode 100644
index 0000000000000000000000000000000000000000..bd63b333ca10d993230027f933cebf7054684974
--- /dev/null
+++ b/Documentation/iio/ad3552r.rst
@@ -0,0 +1,73 @@
+.. 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
+==================
+
+Generic SPI
+-----------
+Use the classic SPI S_CLK/CS/SDO/SDI connection.
+
+High speed (using axi-dac backend)
+----------------------------------
+
+::
+
+    .-----------------.                .-------.
+    |                 |--- 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 debugfs path ``/sys/kernel/debug/iio/iio:deviceX``.
+
++-----------------------+------------------------------------------------------+
+| Debugfs device files  | Description                                          |
++-----------------------+------------------------------------------------------+
+| data_source           | The used data source, as i.e.                        |
+|                       | ``iio-buffer``, ``backend-ramp-generator``, etc.     |
++-----------------------+------------------------------------------------------+
+| data_source_available | The available data sources.                          |
++-----------------------+------------------------------------------------------+
+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

[PATCH v4 3/5] 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.

Reviewed-by: Nuno Sá <nuno.sa@analog.com>
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 v4 4/5] iio: dac: adi-axi-dac: add data source get

[Angelo Dureghello]

From: Angelo Dureghello <adureghello@baylibre.com>

Add data source getter.

Reviewed-by: Nuno Sá <nuno.sa@analog.com>
Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
---
 drivers/iio/dac/adi-axi-dac.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c
index f86acb98b0cffb09bf4d4626f932bf1edc911e2b..8ed5ad1fa24cef649056bc5f4ca80abbf28b9323 100644
--- a/drivers/iio/dac/adi-axi-dac.c
+++ b/drivers/iio/dac/adi-axi-dac.c
@@ -536,6 +536,35 @@ 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;
+
+	if (chan > AXI_DAC_CHAN_CNTRL_MAX)
+		return -EINVAL;
+
+	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 -EIO;
+	}
+}
+
 static int axi_dac_set_sample_rate(struct iio_backend *back, unsigned int chan,
 				   u64 sample_rate)
 {
@@ -818,6 +847,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

[PATCH v4 5/5] 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 | 166 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 160 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/dac/ad3552r-hs.c b/drivers/iio/dac/ad3552r-hs.c
index 37397e188f225a8099745ec03f7c604da76960b1..9a8eed7a06e4f2e7b23d59764b8f2fc21e2c4537 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>
@@ -54,6 +55,18 @@ struct ad3552r_hs_state {
 	struct ad3552r_hs_platform_data *data;
 	/* INTERFACE_CONFIG_D register cache, in DDR we cannot read values. */
 	u32 config_d;
+	/* Protects backend I/O operations from concurrent accesses. */
+	struct mutex lock;
+};
+
+enum ad3552r_sources {
+	AD3552R_SRC_IIO_BUFFER,
+	AD3552R_SRC_BACKEND_RAMP_GEN,
+};
+
+static const char * const dbgfs_attr_source[] = {
+	[AD3552R_SRC_IIO_BUFFER] = "iio-buffer",
+	[AD3552R_SRC_BACKEND_RAMP_GEN] = "backend-ramp-generator",
 };
 
 static int ad3552r_hs_reg_read(struct ad3552r_hs_state *st, u32 reg, u32 *val,
@@ -65,6 +78,20 @@ 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 i, ret;
+
+	for (i = 0; i < st->model_data->num_hw_channels; ++i) {
+		ret = iio_backend_data_source_set(st->back, i, type);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int ad3552r_hs_update_reg_bits(struct ad3552r_hs_state *st, u32 reg,
 				      u32 mask, u32 val, size_t xfer_size)
 {
@@ -483,6 +510,107 @@ 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 idx, ret;
+
+	guard(mutex)(&st->lock);
+
+	ret = iio_backend_data_source_get(st->back, 0, &type);
+	if (ret)
+		return ret;
+
+	switch (type) {
+	case IIO_BACKEND_INTERNAL_RAMP_16BIT:
+		idx = AD3552R_SRC_BACKEND_RAMP_GEN;
+		break;
+	case IIO_BACKEND_EXTERNAL:
+		idx = AD3552R_SRC_IIO_BUFFER;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return simple_read_from_buffer(userbuf, count, ppos,
+				       dbgfs_attr_source[idx],
+				       strlen(dbgfs_attr_source[idx]));
+}
+
+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, source;
+
+	guard(mutex)(&st->lock);
+
+	ret = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf,
+				     count);
+	if (ret < 0)
+		return ret;
+
+	buf[count] = '\0';
+
+	ret = match_string(dbgfs_attr_source, ARRAY_SIZE(dbgfs_attr_source),
+			   buf);
+	if (ret < 0)
+		return ret;
+
+	switch (ret) {
+	case AD3552R_SRC_BACKEND_RAMP_GEN:
+		source = IIO_BACKEND_INTERNAL_RAMP_16BIT;
+		break;
+	case AD3552R_SRC_IIO_BUFFER:
+		source = IIO_BACKEND_EXTERNAL;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = ad3552r_hs_set_data_source(st, source);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+static ssize_t ad3552r_hs_show_data_source_avail(struct file *f,
+						 char __user *userbuf,
+						 size_t count, loff_t *ppos)
+{
+	ssize_t len = 0;
+	char *buf;
+	int i;
+
+	buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	for (i = 0; i < ARRAY_SIZE(dbgfs_attr_source); i++) {
+		len += scnprintf(buf + len, PAGE_SIZE - len, "%s ",
+				 dbgfs_attr_source[i]);
+	}
+	buf[len - 1] = '\n';
+
+	return simple_read_from_buffer(userbuf, count, ppos, buf, len);
+}
+
+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 const struct file_operations ad3552r_hs_data_source_avail_fops = {
+	.owner = THIS_MODULE,
+	.read = ad3552r_hs_show_data_source_avail,
+};
+
 static int ad3552r_hs_setup(struct ad3552r_hs_state *st)
 {
 	u16 id;
@@ -550,11 +678,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 +785,26 @@ 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);
+	debugfs_create_file("data_source_available", 0600, d, st,
+			    &ad3552r_hs_data_source_avail_fops);
+}
+
 static int ad3552r_hs_probe(struct platform_device *pdev)
 {
 	struct ad3552r_hs_state *st;
@@ -705,7 +849,17 @@ 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;
+
+	ret = devm_mutex_init(&pdev->dev, &st->lock);
+	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 v4 5/5] iio: dac: ad3552r-hs: add support for internal ramp

[Nuno Sá]

On Tue, 2025-04-08 at 12:18 +0200, 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>
> ---

Hi Angelo,

One issue that needs a respin and then a minor comment... With it,

Reviewed-by: Nuno Sá <nuno.sa@analog.com>

>  drivers/iio/dac/ad3552r-hs.c | 166 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 160 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/dac/ad3552r-hs.c b/drivers/iio/dac/ad3552r-hs.c
> index
> 37397e188f225a8099745ec03f7c604da76960b1..9a8eed7a06e4f2e7b23d59764b8f2fc21e2c4537
> 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>
> @@ -54,6 +55,18 @@ struct ad3552r_hs_state {
>  	struct ad3552r_hs_platform_data *data;
>  	/* INTERFACE_CONFIG_D register cache, in DDR we cannot read values. */
>  	u32 config_d;
> +	/* Protects backend I/O operations from concurrent accesses. */
> +	struct mutex lock;
> +};
> +
> +enum ad3552r_sources {
> +	AD3552R_SRC_IIO_BUFFER,
> +	AD3552R_SRC_BACKEND_RAMP_GEN,
> +};
> +
> +static const char * const dbgfs_attr_source[] = {
> +	[AD3552R_SRC_IIO_BUFFER] = "iio-buffer",
> +	[AD3552R_SRC_BACKEND_RAMP_GEN] = "backend-ramp-generator",
>  };

nit: I would use more generic strings. I assume "iio-buffer" is just the "normal"
data so use something like that. For the ramp, is it 16 bits? I would just use ex:
RAMP_16. I do not thing that the "backend" prefix (as well as "-generator") to add
much.
>  

...

> +
> +static ssize_t ad3552r_hs_show_data_source_avail(struct file *f,
> +						 char __user *userbuf,
> +						 size_t count, loff_t *ppos)
> +{
> +	ssize_t len = 0;
> +	char *buf;
> +	int i;
> +
> +	buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +

When are we freeing this memory? I also do not see the point for a PAGE_SIZE
allocation for such a small string table. I would say to simplify things and use a
local buffer with 64/128 bytes (should be more than enough). If you see this growing
in the future, you can also go with seq_file.

- Nuno Sá

> +	for (i = 0; i < ARRAY_SIZE(dbgfs_attr_source); i++) {
> +		len += scnprintf(buf + len, PAGE_SIZE - len, "%s ",
> +				 dbgfs_attr_source[i]);
> +	}
> +	buf[len - 1] = '\n';
> +
> +	return simple_read_from_buffer(userbuf, count, ppos, buf, len);
> +}
> +
> +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 const struct file_operations ad3552r_hs_data_source_avail_fops = {
> +	.owner = THIS_MODULE,
> +	.read = ad3552r_hs_show_data_source_avail,
> +};
> +
>  static int ad3552r_hs_setup(struct ad3552r_hs_state *st)
>  {
>  	u16 id;
> @@ -550,11 +678,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 +785,26 @@ 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);
> +	debugfs_create_file("data_source_available", 0600, d, st,
> +			    &ad3552r_hs_data_source_avail_fops);
> +}
> +
>  static int ad3552r_hs_probe(struct platform_device *pdev)
>  {
>  	struct ad3552r_hs_state *st;
> @@ -705,7 +849,17 @@ 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;
> +
> +	ret = devm_mutex_init(&pdev->dev, &st->lock);
> +	if (ret)
> +		return ret;
> +
> +	ad3552r_hs_debugfs_init(indio_dev);
> +
> +	return ret;
>  }
>  
>  static const struct of_device_id ad3552r_hs_of_id[] = {
> 

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

[Angelo Dureghello]

On 09.04.2025 07:32, Nuno Sá wrote:
> On Tue, 2025-04-08 at 12:18 +0200, 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>
> > ---

Hi Nuno,
> 
> Hi Angelo,
> 
> One issue that needs a respin and then a minor comment... With it,
> 
> Reviewed-by: Nuno Sá <nuno.sa@analog.com>
> 
> >  drivers/iio/dac/ad3552r-hs.c | 166 +++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 160 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/iio/dac/ad3552r-hs.c b/drivers/iio/dac/ad3552r-hs.c
> > index
> > 37397e188f225a8099745ec03f7c604da76960b1..9a8eed7a06e4f2e7b23d59764b8f2fc21e2c4537
> > 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>
> > @@ -54,6 +55,18 @@ struct ad3552r_hs_state {
> >  	struct ad3552r_hs_platform_data *data;
> >  	/* INTERFACE_CONFIG_D register cache, in DDR we cannot read values. */
> >  	u32 config_d;
> > +	/* Protects backend I/O operations from concurrent accesses. */
> > +	struct mutex lock;
> > +};
> > +
> > +enum ad3552r_sources {
> > +	AD3552R_SRC_IIO_BUFFER,
> > +	AD3552R_SRC_BACKEND_RAMP_GEN,
> > +};
> > +
> > +static const char * const dbgfs_attr_source[] = {
> > +	[AD3552R_SRC_IIO_BUFFER] = "iio-buffer",
> > +	[AD3552R_SRC_BACKEND_RAMP_GEN] = "backend-ramp-generator",
> >  };
> 
> nit: I would use more generic strings. I assume "iio-buffer" is just the "normal"
> data so use something like that. For the ramp, is it 16 bits? I would just use ex:
> RAMP_16. I do not thing that the "backend" prefix (as well as "-generator") to add
> much.
> >  

are 
"normal", "ramp-16bit"
ok ?
Or please let me know the names you prefer.

> 
> ...
> 
> > +
> > +static ssize_t ad3552r_hs_show_data_source_avail(struct file *f,
> > +						 char __user *userbuf,
> > +						 size_t count, loff_t *ppos)
> > +{
> > +	ssize_t len = 0;
> > +	char *buf;
> > +	int i;
> > +
> > +	buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
> > +	if (!buf)
> > +		return -ENOMEM;
> > +
> 
> When are we freeing this memory? I also do not see the point for a PAGE_SIZE
> allocation for such a small string table. I would say to simplify things and use a
> local buffer with 64/128 bytes (should be more than enough). If you see this growing
> in the future, you can also go with seq_file.
>
agh, frogot the free. Sorry.
Ok, i'll use 128.
 
> - Nuno Sá
> 
> > +	for (i = 0; i < ARRAY_SIZE(dbgfs_attr_source); i++) {
> > +		len += scnprintf(buf + len, PAGE_SIZE - len, "%s ",
> > +				 dbgfs_attr_source[i]);
> > +	}
> > +	buf[len - 1] = '\n';
> > +
> > +	return simple_read_from_buffer(userbuf, count, ppos, buf, len);
> > +}
> > +
> > +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 const struct file_operations ad3552r_hs_data_source_avail_fops = {
> > +	.owner = THIS_MODULE,
> > +	.read = ad3552r_hs_show_data_source_avail,
> > +};
> > +
> >  static int ad3552r_hs_setup(struct ad3552r_hs_state *st)
> >  {
> >  	u16 id;
> > @@ -550,11 +678,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 +785,26 @@ 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);
> > +	debugfs_create_file("data_source_available", 0600, d, st,
> > +			    &ad3552r_hs_data_source_avail_fops);
> > +}
> > +
> >  static int ad3552r_hs_probe(struct platform_device *pdev)
> >  {
> >  	struct ad3552r_hs_state *st;
> > @@ -705,7 +849,17 @@ 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;
> > +
> > +	ret = devm_mutex_init(&pdev->dev, &st->lock);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ad3552r_hs_debugfs_init(indio_dev);
> > +
> > +	return ret;
> >  }
> >  
> >  static const struct of_device_id ad3552r_hs_of_id[] = {
> > 
> 

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

[Nuno Sá]

On Wed, 2025-04-09 at 11:24 +0200, Angelo Dureghello wrote:
> On 09.04.2025 07:32, Nuno Sá wrote:
> > On Tue, 2025-04-08 at 12:18 +0200, 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>
> > > ---
> 
> Hi Nuno,
> > 
> > Hi Angelo,
> > 
> > One issue that needs a respin and then a minor comment... With it,
> > 
> > Reviewed-by: Nuno Sá <nuno.sa@analog.com>
> > 
> > >  drivers/iio/dac/ad3552r-hs.c | 166
> > > +++++++++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 160 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/iio/dac/ad3552r-hs.c b/drivers/iio/dac/ad3552r-hs.c
> > > index
> > > 37397e188f225a8099745ec03f7c604da76960b1..9a8eed7a06e4f2e7b23d59764b8f2fc2
> > > 1e2c4537
> > > 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>
> > > @@ -54,6 +55,18 @@ struct ad3552r_hs_state {
> > >  	struct ad3552r_hs_platform_data *data;
> > >  	/* INTERFACE_CONFIG_D register cache, in DDR we cannot read
> > > values. */
> > >  	u32 config_d;
> > > +	/* Protects backend I/O operations from concurrent accesses. */
> > > +	struct mutex lock;
> > > +};
> > > +
> > > +enum ad3552r_sources {
> > > +	AD3552R_SRC_IIO_BUFFER,
> > > +	AD3552R_SRC_BACKEND_RAMP_GEN,
> > > +};
> > > +
> > > +static const char * const dbgfs_attr_source[] = {
> > > +	[AD3552R_SRC_IIO_BUFFER] = "iio-buffer",
> > > +	[AD3552R_SRC_BACKEND_RAMP_GEN] = "backend-ramp-generator",
> > >  };
> > 
> > nit: I would use more generic strings. I assume "iio-buffer" is just the
> > "normal"
> > data so use something like that. For the ramp, is it 16 bits? I would just
> > use ex:
> > RAMP_16. I do not thing that the "backend" prefix (as well as "-generator")
> > to add
> > much.
> > >  
> 
> are 
> "normal", "ramp-16bit"
> ok ?
> Or please let me know the names you prefer.

I'm also not great with naming :). But yes, I think the above is a better choice

- Nuno Sá

> 
> > 
> > ...
> > 
> > > +
> > > +static ssize_t ad3552r_hs_show_data_source_avail(struct file *f,
> > > +						 char __user *userbuf,
> > > +						 size_t count, loff_t
> > > *ppos)
> > > +{
> > > +	ssize_t len = 0;
> > > +	char *buf;
> > > +	int i;
> > > +
> > > +	buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
> > > +	if (!buf)
> > > +		return -ENOMEM;
> > > +
> > 
> > When are we freeing this memory? I also do not see the point for a PAGE_SIZE
> > allocation for such a small string table. I would say to simplify things and
> > use a
> > local buffer with 64/128 bytes (should be more than enough). If you see this
> > growing
> > in the future, you can also go with seq_file.
> > 
> agh, frogot the free. Sorry.
> Ok, i'll use 128.
>  
> > - Nuno Sá
> > 
> > > +	for (i = 0; i < ARRAY_SIZE(dbgfs_attr_source); i++) {
> > > +		len += scnprintf(buf + len, PAGE_SIZE - len, "%s ",
> > > +				 dbgfs_attr_source[i]);
> > > +	}
> > > +	buf[len - 1] = '\n';
> > > +
> > > +	return simple_read_from_buffer(userbuf, count, ppos, buf, len);
> > > +}
> > > +
> > > +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 const struct file_operations ad3552r_hs_data_source_avail_fops = {
> > > +	.owner = THIS_MODULE,
> > > +	.read = ad3552r_hs_show_data_source_avail,
> > > +};
> > > +
> > >  static int ad3552r_hs_setup(struct ad3552r_hs_state *st)
> > >  {
> > >  	u16 id;
> > > @@ -550,11 +678,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 +785,26 @@ 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);
> > > +	debugfs_create_file("data_source_available", 0600, d, st,
> > > +			    &ad3552r_hs_data_source_avail_fops);
> > > +}
> > > +
> > >  static int ad3552r_hs_probe(struct platform_device *pdev)
> > >  {
> > >  	struct ad3552r_hs_state *st;
> > > @@ -705,7 +849,17 @@ 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;
> > > +
> > > +	ret = devm_mutex_init(&pdev->dev, &st->lock);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ad3552r_hs_debugfs_init(indio_dev);
> > > +
> > > +	return ret;
> > >  }
> > >  
> > >  static const struct of_device_id ad3552r_hs_of_id[] = {
> > > 
> >