[PATCH v1 00/15] iio: adc: ad7768-1: Add features, improvements, and fixes

[PATCH v1 00/15] iio: adc: ad7768-1: Add features, improvements, and fixes

[Jonathan Santos]

This patch series introduces some new features, improvements,
and fixes for the AD7768-1 ADC driver. 

The goal is to support all key functionalities listed in the device
datasheet, including filter mode selection, common mode voltage 
configuration and GPIO support. Additionally, this includes fixes 
for SPI communication and for IIO interface, and also code improvements
to enhance maintainability and readability.

Jonathan Santos (8):
  dt-bindings: iio: adc: ad7768-1: add synchronization over SPI property
  Documentation: ABI: add wideband filter type to  sysfs-bus-iio
  Documentation: ABI: testing: ad7768-1: Add device specific ABI
    documentation.
  iio: adc: ad7768-1: set MOSI idle state to high
  iio: adc: ad7768-1: use guard(mutex) to simplify code
  iio: adc: ad7768-1: add multiple scan types to support 16-bits mode
  iio: adc: ad7768-1: add support for Synchronization over SPI
  iio: adc: ad7768-1: add filter type and decimation rate attributes

Sergiu Cuciurean (7):
  iio: adc: ad7768-1: Fix conversion result sign
  iio: adc: ad7768-1: Update reg_read function
  iio: adc: ad7768-1: Add reset gpio
  iio: adc: ad7768-1: Move buffer allocation to a separate function
  iio: adc: ad7768-1: Add support for variable VCM
  iio: adc: ad7768-1: Add reg_write_masked function
  iio: adc: ad7768-1: Add GPIO controller support

 Documentation/ABI/testing/sysfs-bus-iio       |   2 +
 .../ABI/testing/sysfs-bus-iio-adc-ad7768-1    |  13 +
 .../bindings/iio/adc/adi,ad7768-1.yaml        |  24 +-
 drivers/iio/adc/ad7768-1.c                    | 830 +++++++++++++++---
 4 files changed, 747 insertions(+), 122 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-ad7768-1


base-commit: 5de07b8a24cf44cdb78adeab790704bf577c2c1d
prerequisite-patch-id: 8b531bca46f7c7ea1c0f6d232d162fd05fda52f7
prerequisite-patch-id: c8c8637cb3343097c3224a9aa315fc45dca15f45
prerequisite-patch-id: e0baac9ef84e6e8a0a272fc6191fc1cb48143e44
prerequisite-patch-id: 7e5cad70809fa7d37e917628147c197427c11594
prerequisite-patch-id: 28dcdb0ebc3ca3c02713f83c94a4eedbe81095dc
prerequisite-patch-id: 4a15293f3c2a6504db29ef3de9f09b61accff156
prerequisite-patch-id: 97d1b49e91b5130e527a2e0ac8bd402c2f4a871c
prerequisite-patch-id: 3370db9ec1e67ba97b55607f445ff37c60929668
prerequisite-patch-id: d686dd309e1d3d39d038613f514e58ff5893ae42
prerequisite-patch-id: c832285d7bcc22433f2314a144566aa9437fd5da
prerequisite-patch-id: 3f758a121e36edd43789e80379ff81beeb2d75ce
prerequisite-patch-id: 0ef36ec4d6cf23f08bdb3bc4399ced2561a2a69b
prerequisite-patch-id: c8e7f0e10a2630bd0029ee160f8dfc3f742378ba
prerequisite-patch-id: 5e85d52a87f2a833893eeeac5d1651bda46d0931
prerequisite-patch-id: cd75aba06cf77f8cd398dc7d0c33d94e1277d1f3
prerequisite-patch-id: b813c25db823f1b02d0e9005188d41c0d89eb291
prerequisite-patch-id: 024ac23a16e45e802b70afe9bc464d1caeb41fcc
prerequisite-patch-id: bc084c859bfa93c5764e656bbcbfd4d14e031299
prerequisite-patch-id: 51ebb591fbbb3535899332ce1b106a3f8d6497da
prerequisite-patch-id: 2b396d1069227fee1c5a7bcf33bc63a56681441b
prerequisite-patch-id: c00b841cea6e331e19fb1f31beae831572bce4f5
prerequisite-patch-id: 4fe5fcebfeb745a83a7054390a304a1e250d74d1
prerequisite-patch-id: 461cce4f81f88bbec71580c0743b8970a504899c
prerequisite-patch-id: 59cf79cfa5f091815f578aad884ba0e3f9ae2175
prerequisite-patch-id: d48f6e531e64ee7797890e9f36f849f881884f1a
prerequisite-patch-id: 5f48c69023ecae6b3de595c9a209d1c4d65b5ba2
prerequisite-patch-id: 73e2fc3be282880231105142342b47b00b23ab6d
prerequisite-patch-id: d71deacf6bb4e90e8059a12a94ade36866729fa0
prerequisite-patch-id: 6173a25ddf92a3d1446923d9e87b15642b761034
prerequisite-patch-id: 5b248ee02cc148eeea4f01c435e701b74bc07c60
prerequisite-patch-id: 9fa4f11d62ba0e1ef9f3ca08ef1ee5c1f0711038
prerequisite-patch-id: 6cf99f094cfa8d984a1c1cab8813d1078ee48f05
prerequisite-patch-id: b42c4bf1ce430dccca920f942c6040f641c8307e
prerequisite-patch-id: 89aa3f9cc30768f64d25d4f78c1894c7788ac0d2
prerequisite-patch-id: da2577988502200e7c41abe2097b471e1fe960cc
prerequisite-patch-id: f13b82cb6948394fd2e6b82202dea1e3550a19cc
prerequisite-patch-id: 8f43f603074e7678017e883973abdb200294108c
-- 
2.34.1

[PATCH v1 01/15] dt-bindings: iio: adc: ad7768-1: add synchronization over SPI property

[Jonathan Santos]

Add adi,sync-in-spi property to enable synchronization over SPI.
This should be used in the case when the GPIO cannot provide a
pulse synchronous with the base MCLK signal.

User can choose between SPI, GPIO synchronization or neither of them,
but only if a external pulse can be provided, for example, by another
device in a multidevice setup.

Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
---
 .../bindings/iio/adc/adi,ad7768-1.yaml        | 24 ++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
index 3ce59d4d065f..55cec27bfe60 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
@@ -47,6 +47,15 @@ properties:
       in any way, for example if the filter decimation rate changes.
       As the line is active low, it should be marked GPIO_ACTIVE_LOW.
 
+  adi,sync-in-spi:
+    description:
+      Enables synchronization of multiple devices over SPI. This property is
+      used when a signal synchronous to the base MCLK signal cannot be provided
+      via GPIO. It requires the SYNC_OUT pin to be connected to the SYNC_IN pin
+      on the ADC. In the case of multiple devices, the SYNC_OUT pin of one device
+      should be routed to the SYNC_IN pins of the other devices.
+    type: boolean
+
   reset-gpios:
     maxItems: 1
 
@@ -65,7 +74,6 @@ required:
   - vref-supply
   - spi-cpol
   - spi-cpha
-  - adi,sync-in-gpios
 
 patternProperties:
   "^channel@([0-9]|1[0-5])$":
@@ -89,6 +97,20 @@ patternProperties:
 allOf:
   - $ref: /schemas/spi/spi-peripheral-props.yaml#
 
+  # adi,sync-in-gpios and adi,sync-in-spi are mutually exclusive (neither is also valid)
+  - if:
+      required:
+        - adi,sync-in-gpios
+    then:
+      properties:
+        adi,sync-in-spi: false
+  - if:
+      required:
+        - adi,sync-in-spi
+    then:
+      properties:
+        adi,sync-in-gpios: false
+
 unevaluatedProperties: false
 
 examples:
-- 
2.34.1

[PATCH v1 02/15] Documentation: ABI: add wideband filter type to sysfs-bus-iio

[Jonathan Santos]

The Wideband Low Ripple FIR filter is used for AD7768-1 Driver.
Document wideband filter option into filter_type_avaialable
attribute.

Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
---
 Documentation/ABI/testing/sysfs-bus-iio | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index f83bd6829285..c4c21a7bfba1 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -2291,6 +2291,8 @@ Description:
 		* "sinc3+pf2" - Sinc3 + device specific Post Filter 2.
 		* "sinc3+pf3" - Sinc3 + device specific Post Filter 3.
 		* "sinc3+pf4" - Sinc3 + device specific Post Filter 4.
+		* "wideband" - FIR filter with wideband low ripple passband
+		  and sharp transition band.
 
 What:		/sys/.../events/in_proximity_thresh_either_runningperiod
 KernelVersion:	6.6
-- 
2.34.1

[PATCH v1 03/15] Documentation: ABI: testing: ad7768-1: Add device specific ABI documentation.

[Jonathan Santos]

Add ABI documentation specific to the ad7768-1 device, detailing
the decimation_rate attribute for better clarity and usability.

Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
---
 .../ABI/testing/sysfs-bus-iio-adc-ad7768-1          | 13 +++++++++++++
 1 file changed, 13 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-ad7768-1

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7768-1 b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7768-1
new file mode 100644
index 000000000000..065247f07cfb
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7768-1
@@ -0,0 +1,13 @@
+What:		/sys/bus/iio/devices/iio:deviceX/decimation_rate_available
+KernelVersion:
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Reading returns a range of possible decimation rate values.
+
+What:		/sys/bus/iio/devices/iio:deviceX/decimation_rate
+KernelVersion:
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Sets up the decimation rate for the digital filter. This can
+		directly impact in the final sampling frequency. Reading returns
+		the decimation rate. Writing sets the decimation rate.
-- 
2.34.1

[PATCH v1 04/15] iio: adc: ad7768-1: Fix conversion result sign

[Jonathan Santos]

From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>

The ad7768-1 is a fully differential ADC, meaning that the voltage
conversion result is a signed value. Since the value is a 24 bit one,
stored in a 32 bit variable, the sign should be extended in order to get
the correct representation.

Also the channel description has been updated to signed representation,
to match the ADC specifications.

Fixes: a5f8c7da3dbe ("iio: adc: Add AD7768-1 ADC basic support")
Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
---
 drivers/iio/adc/ad7768-1.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
index 113703fb7245..c3cf04311c40 100644
--- a/drivers/iio/adc/ad7768-1.c
+++ b/drivers/iio/adc/ad7768-1.c
@@ -142,7 +142,7 @@ static const struct iio_chan_spec ad7768_channels[] = {
 		.channel = 0,
 		.scan_index = 0,
 		.scan_type = {
-			.sign = 'u',
+			.sign = 's',
 			.realbits = 24,
 			.storagebits = 32,
 			.shift = 8,
@@ -371,7 +371,7 @@ static int ad7768_read_raw(struct iio_dev *indio_dev,
 
 		ret = ad7768_scan_direct(indio_dev);
 		if (ret >= 0)
-			*val = ret;
+			*val = sign_extend32(ret, chan->scan_type.realbits - 1);
 
 		iio_device_release_direct_mode(indio_dev);
 		if (ret < 0)
-- 
2.34.1

[PATCH v1 05/15] iio: adc: ad7768-1: set MOSI idle state to high

[Jonathan Santos]

All supported parts require that the MOSI line stays high
while in idle.

Configure SPI controller to set MOSI idle state to high.

Fixes: a5f8c7da3dbe ("iio: adc: Add AD7768-1 ADC basic support")
Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
---
 drivers/iio/adc/ad7768-1.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
index c3cf04311c40..463a28d09c2e 100644
--- a/drivers/iio/adc/ad7768-1.c
+++ b/drivers/iio/adc/ad7768-1.c
@@ -574,6 +574,15 @@ static int ad7768_probe(struct spi_device *spi)
 		return -ENOMEM;
 
 	st = iio_priv(indio_dev);
+	/*
+	 * The ADC SDI line must be kept high when
+	 * data is not being clocked out of the controller.
+	 * Request the SPI controller to make MOSI idle high.
+	 */
+	spi->mode |= SPI_MOSI_IDLE_HIGH;
+	ret = spi_setup(spi);
+	if (ret < 0)
+		return ret;
 	st->spi = spi;
 
 	st->vref = devm_regulator_get(&spi->dev, "vref");
-- 
2.34.1

[PATCH v1 06/15] iio: adc: ad7768-1: Update reg_read function

[Jonathan Santos]

From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>

This patch adds an additional parameter to the register read function.
By passing the data pointer to the function, the returned value will used
only for status check.
With this change, the status check won't be confused with a register value
check:

ret = ad7768_spi_reg_read()
if (ret){}

Also this change removes the probability to interpret a negative value as
a return code (the ADC is a differential one)."

Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
---
 drivers/iio/adc/ad7768-1.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
index 463a28d09c2e..881446462ff5 100644
--- a/drivers/iio/adc/ad7768-1.c
+++ b/drivers/iio/adc/ad7768-1.c
@@ -177,7 +177,7 @@ struct ad7768_state {
 };
 
 static int ad7768_spi_reg_read(struct ad7768_state *st, unsigned int addr,
-			       unsigned int len)
+			       unsigned int *data, unsigned int len)
 {
 	unsigned int shift;
 	int ret;
@@ -190,7 +190,9 @@ static int ad7768_spi_reg_read(struct ad7768_state *st, unsigned int addr,
 	if (ret < 0)
 		return ret;
 
-	return (be32_to_cpu(st->data.d32) >> shift);
+	*data = (be32_to_cpu(st->data.d32) >> shift);
+
+	return ret;
 }
 
 static int ad7768_spi_reg_write(struct ad7768_state *st,
@@ -206,11 +208,11 @@ static int ad7768_spi_reg_write(struct ad7768_state *st,
 static int ad7768_set_mode(struct ad7768_state *st,
 			   enum ad7768_conv_mode mode)
 {
-	int regval;
+	int ret, regval;
 
-	regval = ad7768_spi_reg_read(st, AD7768_REG_CONVERSION, 1);
-	if (regval < 0)
-		return regval;
+	ret = ad7768_spi_reg_read(st, AD7768_REG_CONVERSION, &regval, 1);
+	if (ret < 0)
+		return ret;
 
 	regval &= ~AD7768_CONV_MODE_MSK;
 	regval |= AD7768_CONV_MODE(mode);
@@ -234,9 +236,9 @@ static int ad7768_scan_direct(struct iio_dev *indio_dev)
 	if (!ret)
 		return -ETIMEDOUT;
 
-	readval = ad7768_spi_reg_read(st, AD7768_REG_ADC_DATA, 3);
-	if (readval < 0)
-		return readval;
+	ret = ad7768_spi_reg_read(st, AD7768_REG_ADC_DATA, &readval, 3);
+	if (ret < 0)
+		return ret;
 	/*
 	 * Any SPI configuration of the AD7768-1 can only be
 	 * performed in continuous conversion mode.
@@ -258,11 +260,9 @@ static int ad7768_reg_access(struct iio_dev *indio_dev,
 
 	mutex_lock(&st->lock);
 	if (readval) {
-		ret = ad7768_spi_reg_read(st, reg, 1);
+		ret = ad7768_spi_reg_read(st, reg, readval, 1);
 		if (ret < 0)
 			goto err_unlock;
-		*readval = ret;
-		ret = 0;
 	} else {
 		ret = ad7768_spi_reg_write(st, reg, writeval);
 	}
@@ -515,12 +515,13 @@ static int ad7768_buffer_postenable(struct iio_dev *indio_dev)
 static int ad7768_buffer_predisable(struct iio_dev *indio_dev)
 {
 	struct ad7768_state *st = iio_priv(indio_dev);
+	unsigned int regval;
 
 	/*
 	 * To exit continuous read mode, perform a single read of the ADC_DATA
 	 * reg (0x2C), which allows further configuration of the device.
 	 */
-	return ad7768_spi_reg_read(st, AD7768_REG_ADC_DATA, 3);
+	return ad7768_spi_reg_read(st, AD7768_REG_ADC_DATA, &regval, 3);
 }
 
 static const struct iio_buffer_setup_ops ad7768_buffer_ops = {
-- 
2.34.1

[PATCH v1 07/15] iio: adc: ad7768-1: Add reset gpio

[Jonathan Santos]

From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>

Depending on the controller, the default state of a gpio can vary. This
change excludes the probability that the dafult state of the ADC reset
gpio will be HIGH if it will be passed as reference in the deivcetree.

Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
---
 drivers/iio/adc/ad7768-1.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
index 881446462ff5..f73b9aec8b0f 100644
--- a/drivers/iio/adc/ad7768-1.c
+++ b/drivers/iio/adc/ad7768-1.c
@@ -161,6 +161,7 @@ struct ad7768_state {
 	struct completion completion;
 	struct iio_trigger *trig;
 	struct gpio_desc *gpio_sync_in;
+	struct gpio_desc *gpio_reset;
 	const char *labels[ARRAY_SIZE(ad7768_channels)];
 	/*
 	 * DMA (thus cache coherency maintenance) may require the
@@ -441,6 +442,18 @@ static int ad7768_setup(struct ad7768_state *st)
 {
 	int ret;
 
+	st->gpio_reset = devm_gpiod_get_optional(&st->spi->dev, "reset",
+						 GPIOD_OUT_LOW);
+	if (IS_ERR(st->gpio_reset))
+		return PTR_ERR(st->gpio_reset);
+
+	if (st->gpio_reset) {
+		gpiod_direction_output(st->gpio_reset, 1);
+		usleep_range(10, 15);
+		gpiod_direction_output(st->gpio_reset, 0);
+		usleep_range(10, 15);
+	}
+
 	/*
 	 * Two writes to the SPI_RESET[1:0] bits are required to initiate
 	 * a software reset. The bits must first be set to 11, and then
-- 
2.34.1

[PATCH v1 08/15] iio: adc: ad7768-1: use guard(mutex) to simplify code

[Jonathan Santos]

Use guard(mutex) from cleanup.h to remove most of the gotos and to make
the code simpler and less likely to fail due to forgetting to unlock
the resources.

Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
---
 drivers/iio/adc/ad7768-1.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
index f73b9aec8b0f..cd1b08053105 100644
--- a/drivers/iio/adc/ad7768-1.c
+++ b/drivers/iio/adc/ad7768-1.c
@@ -5,6 +5,7 @@
  * Copyright 2017 Analog Devices Inc.
  */
 #include <linux/bitfield.h>
+#include <linux/cleanup.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/device.h>
@@ -257,20 +258,12 @@ static int ad7768_reg_access(struct iio_dev *indio_dev,
 			     unsigned int *readval)
 {
 	struct ad7768_state *st = iio_priv(indio_dev);
-	int ret;
 
-	mutex_lock(&st->lock);
-	if (readval) {
-		ret = ad7768_spi_reg_read(st, reg, readval, 1);
-		if (ret < 0)
-			goto err_unlock;
-	} else {
-		ret = ad7768_spi_reg_write(st, reg, writeval);
-	}
-err_unlock:
-	mutex_unlock(&st->lock);
+	guard(mutex)(&st->lock);
+	if (readval)
+		return ad7768_spi_reg_read(st, reg, readval, 1);
 
-	return ret;
+	return ad7768_spi_reg_write(st, reg, writeval);
 }
 
 static int ad7768_set_dig_fil(struct ad7768_state *st,
@@ -484,7 +477,7 @@ static irqreturn_t ad7768_trigger_handler(int irq, void *p)
 	struct ad7768_state *st = iio_priv(indio_dev);
 	int ret;
 
-	mutex_lock(&st->lock);
+	guard(mutex)(&st->lock);
 
 	ret = spi_read(st->spi, &st->data.scan.chan, 3);
 	if (ret < 0)
@@ -495,7 +488,6 @@ static irqreturn_t ad7768_trigger_handler(int irq, void *p)
 
 err_unlock:
 	iio_trigger_notify_done(indio_dev->trig);
-	mutex_unlock(&st->lock);
 
 	return IRQ_HANDLED;
 }
-- 
2.34.1

[PATCH v1 09/15] iio: adc: ad7768-1: Move buffer allocation to a separate function

[Jonathan Santos]

From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>

This change moves the buffer allocation in a separate function, making
space for adding another type of iio buffer if needed.

Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
---
 drivers/iio/adc/ad7768-1.c | 64 ++++++++++++++++++++++----------------
 1 file changed, 37 insertions(+), 27 deletions(-)

diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
index cd1b08053105..eaa9a12737ac 100644
--- a/drivers/iio/adc/ad7768-1.c
+++ b/drivers/iio/adc/ad7768-1.c
@@ -163,6 +163,7 @@ struct ad7768_state {
 	struct iio_trigger *trig;
 	struct gpio_desc *gpio_sync_in;
 	struct gpio_desc *gpio_reset;
+	int irq;
 	const char *labels[ARRAY_SIZE(ad7768_channels)];
 	/*
 	 * DMA (thus cache coherency maintenance) may require the
@@ -569,6 +570,40 @@ static int ad7768_set_channel_label(struct iio_dev *indio_dev,
 	return 0;
 }
 
+static int ad7768_triggered_buffer_alloc(struct iio_dev *indio_dev)
+{
+	struct ad7768_state *st = iio_priv(indio_dev);
+	int ret;
+
+	st->trig = devm_iio_trigger_alloc(indio_dev->dev.parent, "%s-dev%d",
+					  indio_dev->name,
+					  iio_device_id(indio_dev));
+	if (!st->trig)
+		return -ENOMEM;
+
+	st->trig->ops = &ad7768_trigger_ops;
+	iio_trigger_set_drvdata(st->trig, indio_dev);
+	ret = devm_iio_trigger_register(indio_dev->dev.parent, st->trig);
+	if (ret)
+		return ret;
+
+	indio_dev->trig = iio_trigger_get(st->trig);
+
+	init_completion(&st->completion);
+
+	ret = devm_request_irq(indio_dev->dev.parent, st->irq,
+			       &ad7768_interrupt,
+			       IRQF_TRIGGER_RISING | IRQF_ONESHOT,
+			       indio_dev->name, indio_dev);
+	if (ret)
+		return ret;
+
+	return devm_iio_triggered_buffer_setup(indio_dev->dev.parent, indio_dev,
+					       &iio_pollfunc_store_time,
+					       &ad7768_trigger_handler,
+					       &ad7768_buffer_ops);
+}
+
 static int ad7768_probe(struct spi_device *spi)
 {
 	struct ad7768_state *st;
@@ -610,6 +645,7 @@ static int ad7768_probe(struct spi_device *spi)
 		return PTR_ERR(st->mclk);
 
 	st->mclk_freq = clk_get_rate(st->mclk);
+	st->irq = spi->irq;
 
 	mutex_init(&st->lock);
 
@@ -625,37 +661,11 @@ static int ad7768_probe(struct spi_device *spi)
 		return ret;
 	}
 
-	st->trig = devm_iio_trigger_alloc(&spi->dev, "%s-dev%d",
-					  indio_dev->name,
-					  iio_device_id(indio_dev));
-	if (!st->trig)
-		return -ENOMEM;
-
-	st->trig->ops = &ad7768_trigger_ops;
-	iio_trigger_set_drvdata(st->trig, indio_dev);
-	ret = devm_iio_trigger_register(&spi->dev, st->trig);
-	if (ret)
-		return ret;
-
-	indio_dev->trig = iio_trigger_get(st->trig);
-
-	init_completion(&st->completion);
-
 	ret = ad7768_set_channel_label(indio_dev, ARRAY_SIZE(ad7768_channels));
 	if (ret)
 		return ret;
 
-	ret = devm_request_irq(&spi->dev, spi->irq,
-			       &ad7768_interrupt,
-			       IRQF_TRIGGER_RISING | IRQF_ONESHOT,
-			       indio_dev->name, indio_dev);
-	if (ret)
-		return ret;
-
-	ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
-					      &iio_pollfunc_store_time,
-					      &ad7768_trigger_handler,
-					      &ad7768_buffer_ops);
+	ret = ad7768_triggered_buffer_alloc(indio_dev);
 	if (ret)
 		return ret;
 
-- 
2.34.1

[PATCH v1 10/15] iio: adc: ad7768-1: Add support for variable VCM

[Jonathan Santos]

From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>

The VCM output voltage can be used as a common-mode voltage within the
amplifier preconditioning circuits external to the AD7768-1.

This change exports the VCM as an iio attribute and makes it available
for configuration.

Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
---
 drivers/iio/adc/ad7768-1.c | 56 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
index eaa9a12737ac..574d735f2c3a 100644
--- a/drivers/iio/adc/ad7768-1.c
+++ b/drivers/iio/adc/ad7768-1.c
@@ -119,6 +119,17 @@ struct ad7768_clk_configuration {
 	enum ad7768_pwrmode pwrmode;
 };
 
+static const char * const ad7768_vcm_modes[] = {
+	"(AVDD1-AVSS)/2",
+	"2V5",
+	"2V05",
+	"1V9",
+	"1V65",
+	"1V1",
+	"0V9",
+	"OFF",
+};
+
 static const struct ad7768_clk_configuration ad7768_clk_config[] = {
 	{ AD7768_MCLK_DIV_2, AD7768_DEC_RATE_8, 16,  AD7768_FAST_MODE },
 	{ AD7768_MCLK_DIV_2, AD7768_DEC_RATE_16, 32,  AD7768_FAST_MODE },
@@ -133,12 +144,34 @@ static const struct ad7768_clk_configuration ad7768_clk_config[] = {
 	{ AD7768_MCLK_DIV_16, AD7768_DEC_RATE_1024, 16384, AD7768_ECO_MODE },
 };
 
+static int ad7768_get_vcm(struct iio_dev *dev, const struct iio_chan_spec *chan);
+static int ad7768_set_vcm(struct iio_dev *dev, const struct iio_chan_spec *chan,
+			  unsigned int mode);
+
+static const struct iio_enum ad7768_vcm_mode_enum = {
+	.items = ad7768_vcm_modes,
+	.num_items = ARRAY_SIZE(ad7768_vcm_modes),
+	.set = ad7768_set_vcm,
+	.get = ad7768_get_vcm,
+};
+
+static struct iio_chan_spec_ext_info ad7768_ext_info[] = {
+	IIO_ENUM("common_mode_voltage",
+		 IIO_SHARED_BY_ALL,
+		 &ad7768_vcm_mode_enum),
+	IIO_ENUM_AVAILABLE("common_mode_voltage",
+			   IIO_SHARED_BY_ALL,
+			   &ad7768_vcm_mode_enum),
+	{ },
+};
+
 static const struct iio_chan_spec ad7768_channels[] = {
 	{
 		.type = IIO_VOLTAGE,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
 		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
+		.ext_info = ad7768_ext_info,
 		.indexed = 1,
 		.channel = 0,
 		.scan_index = 0,
@@ -159,6 +192,7 @@ struct ad7768_state {
 	struct clk *mclk;
 	unsigned int mclk_freq;
 	unsigned int samp_freq;
+	unsigned int common_mode_voltage;
 	struct completion completion;
 	struct iio_trigger *trig;
 	struct gpio_desc *gpio_sync_in;
@@ -329,6 +363,28 @@ static int ad7768_set_freq(struct ad7768_state *st,
 	return 0;
 }
 
+static int ad7768_get_vcm(struct iio_dev *dev, const struct iio_chan_spec *chan)
+{
+	struct ad7768_state *st = iio_priv(dev);
+
+	return st->common_mode_voltage;
+}
+
+static int ad7768_set_vcm(struct iio_dev *dev,
+			  const struct iio_chan_spec *chan,
+			  unsigned int mode)
+{
+	int ret;
+	struct ad7768_state *st = iio_priv(dev);
+
+	ret = ad7768_spi_reg_write(st, AD7768_REG_ANALOG2, mode);
+
+	if (ret == 0)
+		st->common_mode_voltage = mode;
+
+	return ret;
+}
+
 static ssize_t ad7768_sampling_freq_avail(struct device *dev,
 					  struct device_attribute *attr,
 					  char *buf)
-- 
2.34.1

[PATCH v1 11/15] iio: adc: ad7768-1: Add reg_write_masked function

[Jonathan Santos]

From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>

This commit adds the ad7768_spi_reg_write_masked() which is a helper
function for writing specific bits inside a register, without interfering
with the other bit values.

Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
---
 drivers/iio/adc/ad7768-1.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
index 574d735f2c3a..675af9ea856d 100644
--- a/drivers/iio/adc/ad7768-1.c
+++ b/drivers/iio/adc/ad7768-1.c
@@ -242,6 +242,21 @@ static int ad7768_spi_reg_write(struct ad7768_state *st,
 	return spi_write(st->spi, st->data.d8, 2);
 }
 
+static int ad7768_spi_reg_write_masked(struct ad7768_state *st,
+				       unsigned int addr,
+				       unsigned int mask,
+				       unsigned int val)
+{
+	unsigned int reg_val;
+	int ret;
+
+	ret = ad7768_spi_reg_read(st, addr, &reg_val, 1);
+	if (ret < 0)
+		return ret;
+
+	return ad7768_spi_reg_write(st, addr, (reg_val & ~mask) | val);
+}
+
 static int ad7768_set_mode(struct ad7768_state *st,
 			   enum ad7768_conv_mode mode)
 {
-- 
2.34.1

[PATCH v1 12/15] iio: adc: ad7768-1: Add GPIO controller support

[Jonathan Santos]

From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>

The AD7768-1 has the ability to control other local hardware (such as gain
stages),to power down other blocks in the signal chain, or read local
status signals over the SPI interface.

This change exports the AD7768-1's four gpios and makes them accessible
at an upper layer.

Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
---
 drivers/iio/adc/ad7768-1.c | 121 +++++++++++++++++++++++++++++++++++++
 1 file changed, 121 insertions(+)

diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
index 675af9ea856d..9741a6d47942 100644
--- a/drivers/iio/adc/ad7768-1.c
+++ b/drivers/iio/adc/ad7768-1.c
@@ -10,6 +10,8 @@
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/err.h>
+#include <linux/gpio.h>
+#include <linux/gpio/driver.h>
 #include <linux/gpio/consumer.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -77,6 +79,19 @@
 #define AD7768_CONV_MODE_MSK		GENMASK(2, 0)
 #define AD7768_CONV_MODE(x)		FIELD_PREP(AD7768_CONV_MODE_MSK, x)
 
+/* AD7768_REG_GPIO_CONTROL */
+#define AD7768_GPIO_CONTROL_MSK		GENMASK(3, 0)
+#define AD7768_GPIO_UNIVERSAL_EN	BIT(7)
+
+/* AD7768_REG_GPIO_WRITE */
+#define AD7768_GPIO_WRITE_MSK		GENMASK(3, 0)
+
+/* AD7768_REG_GPIO_READ */
+#define AD7768_GPIO_READ_MSK		GENMASK(3, 0)
+
+#define AD7768_GPIO_INPUT(x)		0x00
+#define AD7768_GPIO_OUTPUT(x)		BIT(x)
+
 #define AD7768_RD_FLAG_MSK(x)		(BIT(6) | ((x) & 0x3F))
 #define AD7768_WR_FLAG_MSK(x)		((x) & 0x3F)
 
@@ -190,6 +205,8 @@ struct ad7768_state {
 	struct regulator *vref;
 	struct mutex lock;
 	struct clk *mclk;
+	struct gpio_chip gpiochip;
+	unsigned int gpio_avail_map;
 	unsigned int mclk_freq;
 	unsigned int samp_freq;
 	unsigned int common_mode_voltage;
@@ -338,6 +355,106 @@ static int ad7768_set_dig_fil(struct ad7768_state *st,
 	return 0;
 }
 
+static int ad7768_gpio_direction_input(struct gpio_chip *chip, unsigned int offset)
+{
+	struct ad7768_state *st = gpiochip_get_data(chip);
+
+	guard(mutex)(&st->lock);
+	return ad7768_spi_reg_write_masked(st,
+					  AD7768_REG_GPIO_CONTROL,
+					  BIT(offset),
+					  AD7768_GPIO_INPUT(offset));
+}
+
+static int ad7768_gpio_direction_output(struct gpio_chip *chip,
+					unsigned int offset, int value)
+{
+	struct ad7768_state *st = gpiochip_get_data(chip);
+
+	guard(mutex)(&st->lock);
+	return ad7768_spi_reg_write_masked(st,
+					  AD7768_REG_GPIO_CONTROL,
+					  BIT(offset),
+					  AD7768_GPIO_OUTPUT(offset));
+}
+
+static int ad7768_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct ad7768_state *st = gpiochip_get_data(chip);
+	unsigned int val;
+	int ret;
+
+	guard(mutex)(&st->lock);
+	ret = ad7768_spi_reg_read(st, AD7768_REG_GPIO_CONTROL, &val, 1);
+	if (ret < 0)
+		return ret;
+
+	if (val & BIT(offset))
+		ret = ad7768_spi_reg_read(st, AD7768_REG_GPIO_WRITE, &val, 1);
+	else
+		ret = ad7768_spi_reg_read(st, AD7768_REG_GPIO_READ, &val, 1);
+	if (ret < 0)
+		return ret;
+
+	return !!(val & BIT(offset));
+}
+
+static void ad7768_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
+{
+	struct ad7768_state *st = gpiochip_get_data(chip);
+	unsigned int val;
+	int ret;
+
+	guard(mutex)(&st->lock);
+	ret = ad7768_spi_reg_read(st, AD7768_REG_GPIO_CONTROL, &val, 1);
+	if (ret < 0)
+		return;
+
+	if (val & BIT(offset))
+		ad7768_spi_reg_write_masked(st,
+					    AD7768_REG_GPIO_WRITE,
+					    BIT(offset),
+					    (value << offset));
+}
+
+static int ad7768_gpio_request(struct gpio_chip *chip, unsigned int offset)
+{
+	struct ad7768_state *st = gpiochip_get_data(chip);
+
+	if (!(st->gpio_avail_map & BIT(offset)))
+		return -ENODEV;
+
+	st->gpio_avail_map &= ~BIT(offset);
+
+	return 0;
+}
+
+static int ad7768_gpio_init(struct ad7768_state *st)
+{
+	int ret;
+
+	ret = ad7768_spi_reg_write(st,
+				   AD7768_REG_GPIO_CONTROL,
+				   AD7768_GPIO_UNIVERSAL_EN);
+	if (ret < 0)
+		return ret;
+
+	st->gpio_avail_map = AD7768_GPIO_CONTROL_MSK;
+	st->gpiochip.label = "ad7768_1_gpios";
+	st->gpiochip.base = -1;
+	st->gpiochip.ngpio = 4;
+	st->gpiochip.parent = &st->spi->dev;
+	st->gpiochip.can_sleep = true;
+	st->gpiochip.direction_input = ad7768_gpio_direction_input;
+	st->gpiochip.direction_output = ad7768_gpio_direction_output;
+	st->gpiochip.get = ad7768_gpio_get;
+	st->gpiochip.set = ad7768_gpio_set;
+	st->gpiochip.request = ad7768_gpio_request;
+	st->gpiochip.owner = THIS_MODULE;
+
+	return gpiochip_add_data(&st->gpiochip, st);
+}
+
 static int ad7768_set_freq(struct ad7768_state *st,
 			   unsigned int freq)
 {
@@ -538,6 +655,10 @@ static int ad7768_setup(struct ad7768_state *st)
 	if (IS_ERR(st->gpio_sync_in))
 		return PTR_ERR(st->gpio_sync_in);
 
+	ret = ad7768_gpio_init(st);
+	if (ret < 0)
+		return ret;
+
 	/* Set the default sampling frequency to 32000 kSPS */
 	return ad7768_set_freq(st, 32000);
 }
-- 
2.34.1

[PATCH v1 13/15] iio: adc: ad7768-1: add multiple scan types to support 16-bits mode

[Jonathan Santos]

When the device is configured to Sinc5 filter and decimation x8,
output data is reduced to 16-bits in order to support 1 MHz of
sampling frequency due to clock limitation.

Use multiple scan types feature to enable the driver to switch
scan type in runtime, making possible to support both 24-bit and
16-bit resolution.

Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
---
 drivers/iio/adc/ad7768-1.c | 65 ++++++++++++++++++++++++++++++++------
 1 file changed, 56 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
index 9741a6d47942..5e4e7d387f9a 100644
--- a/drivers/iio/adc/ad7768-1.c
+++ b/drivers/iio/adc/ad7768-1.c
@@ -134,6 +134,11 @@ struct ad7768_clk_configuration {
 	enum ad7768_pwrmode pwrmode;
 };
 
+enum ad7768_scan_type {
+	AD7768_SCAN_TYPE_NORMAL,
+	AD7768_SCAN_TYPE_HIGH_SPEED,
+};
+
 static const char * const ad7768_vcm_modes[] = {
 	"(AVDD1-AVSS)/2",
 	"2V5",
@@ -145,6 +150,10 @@ static const char * const ad7768_vcm_modes[] = {
 	"OFF",
 };
 
+static const int ad7768_mclk_div_rates[4] = {
+	16, 8, 4, 2,
+};
+
 static const struct ad7768_clk_configuration ad7768_clk_config[] = {
 	{ AD7768_MCLK_DIV_2, AD7768_DEC_RATE_8, 16,  AD7768_FAST_MODE },
 	{ AD7768_MCLK_DIV_2, AD7768_DEC_RATE_16, 32,  AD7768_FAST_MODE },
@@ -159,6 +168,21 @@ static const struct ad7768_clk_configuration ad7768_clk_config[] = {
 	{ AD7768_MCLK_DIV_16, AD7768_DEC_RATE_1024, 16384, AD7768_ECO_MODE },
 };
 
+static const struct iio_scan_type ad7768_scan_type[] = {
+	[AD7768_SCAN_TYPE_NORMAL] = {
+		.sign = 's',
+		.realbits = 24,
+		.storagebits = 32,
+		.endianness = IIO_BE,
+	},
+	[AD7768_SCAN_TYPE_HIGH_SPEED] = {
+		.sign = 's',
+		.realbits = 16,
+		.storagebits = 32,
+		.endianness = IIO_BE,
+	},
+};
+
 static int ad7768_get_vcm(struct iio_dev *dev, const struct iio_chan_spec *chan);
 static int ad7768_set_vcm(struct iio_dev *dev, const struct iio_chan_spec *chan,
 			  unsigned int mode);
@@ -190,13 +214,9 @@ static const struct iio_chan_spec ad7768_channels[] = {
 		.indexed = 1,
 		.channel = 0,
 		.scan_index = 0,
-		.scan_type = {
-			.sign = 's',
-			.realbits = 24,
-			.storagebits = 32,
-			.shift = 8,
-			.endianness = IIO_BE,
-		},
+		.has_ext_scan_type = 1,
+		.ext_scan_type = ad7768_scan_type,
+		.num_ext_scan_type = ARRAY_SIZE(ad7768_scan_type),
 	},
 };
 
@@ -208,6 +228,7 @@ struct ad7768_state {
 	struct gpio_chip gpiochip;
 	unsigned int gpio_avail_map;
 	unsigned int mclk_freq;
+	unsigned int dec_rate;
 	unsigned int samp_freq;
 	unsigned int common_mode_voltage;
 	struct completion completion;
@@ -308,6 +329,15 @@ static int ad7768_scan_direct(struct iio_dev *indio_dev)
 	ret = ad7768_spi_reg_read(st, AD7768_REG_ADC_DATA, &readval, 3);
 	if (ret < 0)
 		return ret;
+
+	/*
+	 * When the decimation rate is set to x8, the ADC data precision is reduced
+	 * from 24 bits to 16 bits. Since the AD7768_REG_ADC_DATA register provides
+	 * 24-bit data, the precision is reduced by right-shifting the read value
+	 * by 8 bits.
+	 */
+	if (st->dec_rate == 8)
+		readval = readval >> 8;
 	/*
 	 * Any SPI configuration of the AD7768-1 can only be
 	 * performed in continuous conversion mode.
@@ -489,6 +519,8 @@ static int ad7768_set_freq(struct ad7768_state *st,
 	if (ret < 0)
 		return ret;
 
+	st->dec_rate = ad7768_clk_config[idx].clk_div /
+		       ad7768_mclk_div_rates[ad7768_clk_config[idx].mclk_div];
 	st->samp_freq = DIV_ROUND_CLOSEST(st->mclk_freq,
 					  ad7768_clk_config[idx].clk_div);
 
@@ -544,8 +576,13 @@ static int ad7768_read_raw(struct iio_dev *indio_dev,
 			   int *val, int *val2, long info)
 {
 	struct ad7768_state *st = iio_priv(indio_dev);
+	const struct iio_scan_type *scan_type;
 	int scale_uv, ret;
 
+	scan_type = iio_get_current_scan_type(indio_dev, chan);
+	if (IS_ERR(scan_type))
+		return PTR_ERR(scan_type);
+
 	switch (info) {
 	case IIO_CHAN_INFO_RAW:
 		ret = iio_device_claim_direct_mode(indio_dev);
@@ -554,7 +591,7 @@ static int ad7768_read_raw(struct iio_dev *indio_dev,
 
 		ret = ad7768_scan_direct(indio_dev);
 		if (ret >= 0)
-			*val = sign_extend32(ret, chan->scan_type.realbits - 1);
+			*val = sign_extend32(ret, scan_type->realbits - 1);
 
 		iio_device_release_direct_mode(indio_dev);
 		if (ret < 0)
@@ -568,7 +605,7 @@ static int ad7768_read_raw(struct iio_dev *indio_dev,
 			return scale_uv;
 
 		*val = (scale_uv * 2) / 1000;
-		*val2 = chan->scan_type.realbits;
+		*val2 = scan_type->realbits;
 
 		return IIO_VAL_FRACTIONAL_LOG2;
 
@@ -612,11 +649,21 @@ static const struct attribute_group ad7768_group = {
 	.attrs = ad7768_attributes,
 };
 
+static int ad7768_get_current_scan_type(const struct iio_dev *indio_dev,
+					const struct iio_chan_spec *chan)
+{
+	struct ad7768_state *st = iio_priv(indio_dev);
+
+	return st->dec_rate == 8 ? AD7768_SCAN_TYPE_HIGH_SPEED :
+		AD7768_SCAN_TYPE_NORMAL;
+}
+
 static const struct iio_info ad7768_info = {
 	.attrs = &ad7768_group,
 	.read_raw = &ad7768_read_raw,
 	.write_raw = &ad7768_write_raw,
 	.read_label = ad7768_read_label,
+	.get_current_scan_type = &ad7768_get_current_scan_type,
 	.debugfs_reg_access = &ad7768_reg_access,
 };
 
-- 
2.34.1

[PATCH v1 14/15] iio: adc: ad7768-1: add support for Synchronization over SPI

[Jonathan Santos]

The synchronization method using GPIO requires the generated pulse to be
truly synchronous with the base MCLK signal. When it is not possible to
do that in hardware, the datasheet recommends using synchronization over
SPI, where the generated pulse is already synchronous with MCLK. This
requires the SYNC_OUT pin to be connected to SYNC_IN pin.

Add the option to handle device synchronization over SPI.

Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
---
 drivers/iio/adc/ad7768-1.c | 33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
index 5e4e7d387f9a..0b0cb3b396ff 100644
--- a/drivers/iio/adc/ad7768-1.c
+++ b/drivers/iio/adc/ad7768-1.c
@@ -235,6 +235,7 @@ struct ad7768_state {
 	struct iio_trigger *trig;
 	struct gpio_desc *gpio_sync_in;
 	struct gpio_desc *gpio_reset;
+	bool en_spi_sync;
 	int irq;
 	const char *labels[ARRAY_SIZE(ad7768_channels)];
 	/*
@@ -295,6 +296,19 @@ static int ad7768_spi_reg_write_masked(struct ad7768_state *st,
 	return ad7768_spi_reg_write(st, addr, (reg_val & ~mask) | val);
 }
 
+static int ad7768_send_sync_pulse(struct ad7768_state *st)
+{
+	if (st->en_spi_sync)
+		return ad7768_spi_reg_write(st, AD7768_REG_SYNC_RESET, 0x00);
+
+	if (st->gpio_sync_in) {
+		gpiod_set_value_cansleep(st->gpio_sync_in, 1);
+		gpiod_set_value_cansleep(st->gpio_sync_in, 0);
+	}
+
+	return 0;
+}
+
 static int ad7768_set_mode(struct ad7768_state *st,
 			   enum ad7768_conv_mode mode)
 {
@@ -379,10 +393,7 @@ static int ad7768_set_dig_fil(struct ad7768_state *st,
 		return ret;
 
 	/* A sync-in pulse is required every time the filter dec rate changes */
-	gpiod_set_value(st->gpio_sync_in, 1);
-	gpiod_set_value(st->gpio_sync_in, 0);
-
-	return 0;
+	return ad7768_send_sync_pulse(st);
 }
 
 static int ad7768_gpio_direction_input(struct gpio_chip *chip, unsigned int offset)
@@ -697,11 +708,21 @@ static int ad7768_setup(struct ad7768_state *st)
 	if (ret)
 		return ret;
 
-	st->gpio_sync_in = devm_gpiod_get(&st->spi->dev, "adi,sync-in",
-					  GPIOD_OUT_LOW);
+	st->gpio_sync_in = devm_gpiod_get_optional(&st->spi->dev, "adi,sync-in",
+						   GPIOD_OUT_LOW);
 	if (IS_ERR(st->gpio_sync_in))
 		return PTR_ERR(st->gpio_sync_in);
 
+	if (device_property_present(&st->spi->dev, "adi,sync-in-spi"))
+		st->en_spi_sync = true;
+
+	/*
+	 * GPIO and SPI Synchronization are mutually exclusive.
+	 * Return error if both are enabled
+	 */
+	if (st->gpio_sync_in && st->en_spi_sync)
+		return -EINVAL;
+
 	ret = ad7768_gpio_init(st);
 	if (ret < 0)
 		return ret;
-- 
2.34.1

[PATCH v1 15/15] iio: adc: ad7768-1: add filter type and decimation rate attributes

[Jonathan Santos]

Separate filter type and decimation rate from the sampling frequency
attribute. The new filter type attribute enables SINC3 and WIDEBAND
filters, which were previously unavailable.

Previously, combining decimation and MCLK divider in the sampling
frequency obscured performance trade-offs. Lower MCLK divider
settings increase power usage, while lower decimation rates reduce
precision by decreasing averaging. By creating a decimation attribute,
users gain finer control over performance.

The addition of those attributes allows a wider range of sampling
frequencies and more access to the device features.

Co-developed-by: PopPaul2021 <paul.pop@analog.com>
Signed-off-by: PopPaul2021 <paul.pop@analog.com>
Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
---
 drivers/iio/adc/ad7768-1.c | 429 +++++++++++++++++++++++++++++++------
 1 file changed, 366 insertions(+), 63 deletions(-)

diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
index 0b0cb3b396ff..842acef1410d 100644
--- a/drivers/iio/adc/ad7768-1.c
+++ b/drivers/iio/adc/ad7768-1.c
@@ -17,6 +17,7 @@
 #include <linux/module.h>
 #include <linux/regulator/consumer.h>
 #include <linux/sysfs.h>
+#include "linux/util_macros.h"
 #include <linux/spi/spi.h>
 
 #include <linux/iio/buffer.h>
@@ -75,6 +76,10 @@
 #define AD7768_DIG_FIL_DEC_MSK		GENMASK(2, 0)
 #define AD7768_DIG_FIL_DEC_RATE(x)	FIELD_PREP(AD7768_DIG_FIL_DEC_MSK, x)
 
+/* AD7768_SINC3_DEC_RATE */
+#define AD7768_SINC3_DEC_RATE_MSB_MSK	GENMASK(12, 8)
+#define AD7768_SINC3_DEC_RATE_LSB_MSK	GENMASK(7, 0)
+
 /* AD7768_REG_CONVERSION */
 #define AD7768_CONV_MODE_MSK		GENMASK(2, 0)
 #define AD7768_CONV_MODE(x)		FIELD_PREP(AD7768_CONV_MODE_MSK, x)
@@ -95,6 +100,18 @@
 #define AD7768_RD_FLAG_MSK(x)		(BIT(6) | ((x) & 0x3F))
 #define AD7768_WR_FLAG_MSK(x)		((x) & 0x3F)
 
+/* Decimation Rate Limits */
+#define SINC5_DEC_RATE_MIN		8
+#define SINC5_DEC_RATE_MAX		1024
+#define SINC3_DEC_RATE_MIN		32
+#define SINC3_DEC_RATE_MAX		163840
+#define WIDEBAND_DEC_RATE_MIN		32
+#define WIDEBAND_DEC_RATE_MAX		1024
+
+enum {
+	DEC_RATE,
+};
+
 enum ad7768_conv_mode {
 	AD7768_CONTINUOUS,
 	AD7768_ONE_SHOT,
@@ -116,22 +133,12 @@ enum ad7768_mclk_div {
 	AD7768_MCLK_DIV_2
 };
 
-enum ad7768_dec_rate {
-	AD7768_DEC_RATE_32 = 0,
-	AD7768_DEC_RATE_64 = 1,
-	AD7768_DEC_RATE_128 = 2,
-	AD7768_DEC_RATE_256 = 3,
-	AD7768_DEC_RATE_512 = 4,
-	AD7768_DEC_RATE_1024 = 5,
-	AD7768_DEC_RATE_8 = 9,
-	AD7768_DEC_RATE_16 = 10
-};
-
-struct ad7768_clk_configuration {
-	enum ad7768_mclk_div mclk_div;
-	enum ad7768_dec_rate dec_rate;
-	unsigned int clk_div;
-	enum ad7768_pwrmode pwrmode;
+enum ad7768_flt_type {
+	SINC5,
+	SINC5_DEC_X8,
+	SINC5_DEC_X16,
+	SINC3,
+	WIDEBAND
 };
 
 enum ad7768_scan_type {
@@ -154,18 +161,16 @@ static const int ad7768_mclk_div_rates[4] = {
 	16, 8, 4, 2,
 };
 
-static const struct ad7768_clk_configuration ad7768_clk_config[] = {
-	{ AD7768_MCLK_DIV_2, AD7768_DEC_RATE_8, 16,  AD7768_FAST_MODE },
-	{ AD7768_MCLK_DIV_2, AD7768_DEC_RATE_16, 32,  AD7768_FAST_MODE },
-	{ AD7768_MCLK_DIV_2, AD7768_DEC_RATE_32, 64, AD7768_FAST_MODE },
-	{ AD7768_MCLK_DIV_2, AD7768_DEC_RATE_64, 128, AD7768_FAST_MODE },
-	{ AD7768_MCLK_DIV_2, AD7768_DEC_RATE_128, 256, AD7768_FAST_MODE },
-	{ AD7768_MCLK_DIV_4, AD7768_DEC_RATE_128, 512, AD7768_MED_MODE },
-	{ AD7768_MCLK_DIV_4, AD7768_DEC_RATE_256, 1024, AD7768_MED_MODE },
-	{ AD7768_MCLK_DIV_4, AD7768_DEC_RATE_512, 2048, AD7768_MED_MODE },
-	{ AD7768_MCLK_DIV_4, AD7768_DEC_RATE_1024, 4096, AD7768_MED_MODE },
-	{ AD7768_MCLK_DIV_8, AD7768_DEC_RATE_1024, 8192, AD7768_MED_MODE },
-	{ AD7768_MCLK_DIV_16, AD7768_DEC_RATE_1024, 16384, AD7768_ECO_MODE },
+static const int dec_rate_values[6] = {
+	32, 64, 128, 256, 512, 1024,
+};
+
+static const  int sinc3_dec_rate_max_values[4] = {
+	20480, 40960, 81920, 163840,
+};
+
+static const char * const ad7768_filter_enum[] = {
+	"sinc5", "sinc3", "wideband"
 };
 
 static const struct iio_scan_type ad7768_scan_type[] = {
@@ -186,6 +191,23 @@ static const struct iio_scan_type ad7768_scan_type[] = {
 static int ad7768_get_vcm(struct iio_dev *dev, const struct iio_chan_spec *chan);
 static int ad7768_set_vcm(struct iio_dev *dev, const struct iio_chan_spec *chan,
 			  unsigned int mode);
+static int ad7768_get_fil_type_attr(struct iio_dev *dev,
+				    const struct iio_chan_spec *chan);
+static int ad7768_set_fil_type_attr(struct iio_dev *dev,
+				    const struct iio_chan_spec *chan, unsigned int filter);
+static ssize_t ad7768_ext_info_write(struct iio_dev *indio_dev,
+				     uintptr_t private, const struct iio_chan_spec *chan,
+				     const char *buf, size_t len);
+static ssize_t ad7768_ext_info_read(struct iio_dev *indio_dev,
+				    uintptr_t private, const struct iio_chan_spec *chan,
+				    char *buf);
+
+static const struct iio_enum ad7768_flt_type_iio_enum = {
+	.items = ad7768_filter_enum,
+	.num_items = ARRAY_SIZE(ad7768_filter_enum),
+	.set = ad7768_set_fil_type_attr,
+	.get = ad7768_get_fil_type_attr,
+};
 
 static const struct iio_enum ad7768_vcm_mode_enum = {
 	.items = ad7768_vcm_modes,
@@ -201,6 +223,15 @@ static struct iio_chan_spec_ext_info ad7768_ext_info[] = {
 	IIO_ENUM_AVAILABLE("common_mode_voltage",
 			   IIO_SHARED_BY_ALL,
 			   &ad7768_vcm_mode_enum),
+	IIO_ENUM("filter_type", IIO_SHARED_BY_ALL, &ad7768_flt_type_iio_enum),
+	IIO_ENUM_AVAILABLE("filter_type", IIO_SHARED_BY_ALL, &ad7768_flt_type_iio_enum),
+	{
+		.name = "decimation_rate",
+		.read = ad7768_ext_info_read,
+		.write = ad7768_ext_info_write,
+		.shared = IIO_SHARED_BY_ALL,
+		.private = DEC_RATE,
+	},
 	{ },
 };
 
@@ -228,7 +259,9 @@ struct ad7768_state {
 	struct gpio_chip gpiochip;
 	unsigned int gpio_avail_map;
 	unsigned int mclk_freq;
+	unsigned int mclk_div;
 	unsigned int dec_rate;
+	enum ad7768_flt_type filter_type;
 	unsigned int samp_freq;
 	unsigned int common_mode_voltage;
 	struct completion completion;
@@ -252,6 +285,9 @@ struct ad7768_state {
 	} data __aligned(IIO_DMA_MINALIGN);
 };
 
+static int ad7768_set_freq(struct ad7768_state *st,
+			   unsigned int freq);
+
 static int ad7768_spi_reg_read(struct ad7768_state *st, unsigned int addr,
 			       unsigned int *data, unsigned int len)
 {
@@ -309,6 +345,31 @@ static int ad7768_send_sync_pulse(struct ad7768_state *st)
 	return 0;
 }
 
+static int ad7768_set_mclk_div(struct ad7768_state *st, unsigned int mclk_div)
+{
+	unsigned int mclk_div_value;
+	int ret;
+
+	guard(mutex)(&st->lock);
+	ret = ad7768_spi_reg_read(st, AD7768_REG_POWER_CLOCK, &mclk_div_value, 1);
+	if (ret)
+		return ret;
+
+	mclk_div_value &= ~(AD7768_PWR_MCLK_DIV_MSK | AD7768_PWR_PWRMODE_MSK);
+	/* Set mclk_div value */
+	mclk_div_value |= AD7768_PWR_MCLK_DIV(mclk_div);
+	/*
+	 * Set power mode based on mclk_div value.
+	 * ECO_MODE is only recommended for MCLK_DIV 16
+	 */
+	if (mclk_div > AD7768_MCLK_DIV_16)
+		mclk_div_value |= AD7768_PWR_PWRMODE(AD7768_FAST_MODE);
+	else
+		mclk_div_value |= AD7768_PWR_PWRMODE(AD7768_ECO_MODE);
+
+	return ad7768_spi_reg_write(st, AD7768_REG_POWER_CLOCK, mclk_div_value);
+}
+
 static int ad7768_set_mode(struct ad7768_state *st,
 			   enum ad7768_conv_mode mode)
 {
@@ -377,23 +438,185 @@ static int ad7768_reg_access(struct iio_dev *indio_dev,
 	return ad7768_spi_reg_write(st, reg, writeval);
 }
 
-static int ad7768_set_dig_fil(struct ad7768_state *st,
-			      enum ad7768_dec_rate dec_rate)
+static int ad7768_set_sinc3_dec_rate(struct ad7768_state *st,
+				     unsigned int dec_rate)
+{
+	unsigned int dec_rate_msb, dec_rate_lsb, max_dec_rate;
+	int ret;
+
+	guard(mutex)(&st->lock);
+	/*
+	 * Maximum dec_rate is limited by the MCLK_DIV value
+	 * and by the ODR. The edge case is for MCLK_DIV = 2
+	 * ODR = 50 SPS.
+	 * max_dec_rate <= MCLK / (2 * 50)
+	 */
+	max_dec_rate = st->mclk_freq / 100;
+	dec_rate = clamp_t(unsigned int, dec_rate, 32, max_dec_rate);
+	/*
+	 * Calculate the equivalent value to sinc3 decimation ratio
+	 * to be written on the SINC3_DECIMATION_RATE register:
+	 *  Value = (DEC_RATE / 32) -1
+	 */
+	dec_rate = DIV_ROUND_UP(dec_rate, 32) - 1;
+	dec_rate_msb = FIELD_GET(AD7768_SINC3_DEC_RATE_MSB_MSK, dec_rate);
+	dec_rate_lsb = FIELD_GET(AD7768_SINC3_DEC_RATE_LSB_MSK, dec_rate);
+
+	ret = ad7768_spi_reg_write(st, AD7768_REG_SINC3_DEC_RATE_MSB, dec_rate_msb);
+	if (ret)
+		return ret;
+
+	ret = ad7768_spi_reg_write(st, AD7768_REG_SINC3_DEC_RATE_LSB, dec_rate_lsb);
+	if (ret)
+		return ret;
+
+	st->dec_rate = (dec_rate + 1) * 32;
+
+	return 0;
+}
+
+static int ad7768_set_dec_rate(struct ad7768_state *st, unsigned int dec_rate)
 {
+	unsigned int mode, dec_rate_reg;
+	int ret;
+
+	dec_rate_reg = find_closest(dec_rate, dec_rate_values,
+				    ARRAY_SIZE(dec_rate_values));
+
+	guard(mutex)(&st->lock);
+	ret = ad7768_spi_reg_read(st, AD7768_REG_DIGITAL_FILTER, &mode, 1);
+	if (ret)
+		return ret;
+
+	mode &= ~AD7768_DIG_FIL_DEC_MSK;
+	mode |= AD7768_DIG_FIL_DEC_RATE(dec_rate_reg);
+	ret = ad7768_spi_reg_write(st, AD7768_REG_DIGITAL_FILTER, mode);
+	if (ret)
+		return ret;
+
+	st->dec_rate = dec_rate_values[dec_rate_reg];
+
+	return 0;
+}
+
+static int ad7768_set_filter_type(struct iio_dev *dev,
+				  enum ad7768_flt_type filter_type)
+{
+	struct ad7768_state *st = iio_priv(dev);
 	unsigned int mode;
 	int ret;
 
-	if (dec_rate == AD7768_DEC_RATE_8 || dec_rate == AD7768_DEC_RATE_16)
-		mode = AD7768_DIG_FIL_FIL(dec_rate);
-	else
-		mode = AD7768_DIG_FIL_DEC_RATE(dec_rate);
+	guard(mutex)(&st->lock);
+	ret = ad7768_spi_reg_read(st, AD7768_REG_DIGITAL_FILTER, &mode, 1);
+	if (ret)
+		return ret;
+
+	mode &= ~AD7768_DIG_FIL_FIL_MSK;
+	mode |= AD7768_DIG_FIL_FIL(filter_type);
 
 	ret = ad7768_spi_reg_write(st, AD7768_REG_DIGITAL_FILTER, mode);
 	if (ret < 0)
 		return ret;
 
-	/* A sync-in pulse is required every time the filter dec rate changes */
-	return ad7768_send_sync_pulse(st);
+	st->filter_type = filter_type;
+
+	return 0;
+}
+
+static int ad7768_configure_dig_fil(struct iio_dev *dev,
+				    enum ad7768_flt_type filter_type,
+				    unsigned int dec_rate)
+{
+	struct ad7768_state *st = iio_priv(dev);
+	int ret;
+
+	if (filter_type == SINC3) {
+		ret = ad7768_set_filter_type(dev, SINC3);
+		if (ret)
+			return ret;
+
+		/* recalculate the decimation for this filter mode */
+		ret = ad7768_set_sinc3_dec_rate(st, dec_rate);
+	} else if (filter_type == WIDEBAND) {
+		ret = ad7768_set_filter_type(dev, filter_type);
+		if (ret)
+			return ret;
+
+		/* recalculate the decimation rate */
+		ret = ad7768_set_dec_rate(st, dec_rate);
+	} else {
+		/* For SINC5 filter */
+		/* Decimation 8 and 16 are set in the digital filter field */
+		if (dec_rate <= 8) {
+			ret = ad7768_set_filter_type(dev, SINC5_DEC_X8);
+			if (ret)
+				return ret;
+
+			st->dec_rate = 8;
+		} else if (dec_rate <= 16) {
+			ret = ad7768_set_filter_type(dev, SINC5_DEC_X16);
+			if (ret)
+				return ret;
+
+			st->dec_rate = 16;
+		} else {
+			ret = ad7768_set_filter_type(dev, SINC5);
+			if (ret)
+				return ret;
+
+			ret = ad7768_set_dec_rate(st, dec_rate);
+		}
+	}
+
+	/* Update scale table: scale values vary according to the precision */
+	ad7768_fill_scale_tbl(dev);
+
+	return ret;
+}
+
+static int ad7768_set_fil_type_attr(struct iio_dev *dev,
+				    const struct iio_chan_spec *chan,
+				    unsigned int filter)
+{
+	struct ad7768_state *st = iio_priv(dev);
+	int ret;
+
+	/*
+	 * Filters of types 0, 1, and 2 correspond to SINC5.
+	 * For SINC3 and wideband filter types, an offset of 2 is added
+	 * to align with the expected register values.
+	 */
+	if (filter != SINC5)
+		filter += 2;
+
+	ret = ad7768_configure_dig_fil(dev, filter, st->dec_rate);
+	if (ret)
+		return ret;
+
+	/* Update sampling frequency */
+	return ad7768_set_freq(st, st->samp_freq);
+}
+
+static int ad7768_get_fil_type_attr(struct iio_dev *dev,
+				    const struct iio_chan_spec *chan)
+{
+	struct ad7768_state *st = iio_priv(dev);
+	int ret;
+	unsigned int mode;
+
+	ret = ad7768_spi_reg_read(st, AD7768_REG_DIGITAL_FILTER, &mode, 1);
+	if (ret)
+		return ret;
+
+	mode = FIELD_GET(AD7768_DIG_FIL_FIL_MSK, mode);
+	/* Filter types from 0 to 2 are represented as SINC5 */
+	if (mode < SINC3)
+		return SINC5;
+
+	/* Remove the offset for the sinc3 and wideband filters
+	 * to get the corresponding attribute enum value
+	 */
+	return mode - 2;
 }
 
 static int ad7768_gpio_direction_input(struct gpio_chip *chip, unsigned int offset)
@@ -496,46 +719,84 @@ static int ad7768_gpio_init(struct ad7768_state *st)
 	return gpiochip_add_data(&st->gpiochip, st);
 }
 
+static ssize_t ad7768_ext_info_read(struct iio_dev *indio_dev,
+				    uintptr_t private,
+				    const struct iio_chan_spec *chan, char *buf)
+{
+	struct ad7768_state *st = iio_priv(indio_dev);
+
+	switch (private) {
+	case DEC_RATE:
+		return sysfs_emit(buf, "%d\n", st->dec_rate);
+	default:
+		return -EINVAL;
+	}
+}
+
+static ssize_t ad7768_ext_info_write(struct iio_dev *indio_dev,
+				     uintptr_t private,
+				     const struct iio_chan_spec *chan,
+				     const char *buf, size_t len)
+{
+	int ret = -EINVAL;
+	long long dec_rate;
+	struct ad7768_state *st = iio_priv(indio_dev);
+
+	switch (private) {
+	case DEC_RATE:
+		ret = kstrtoll(buf, 10, &dec_rate);
+		if (ret)
+			return ret;
+
+		ret = ad7768_configure_dig_fil(indio_dev, st->filter_type, dec_rate);
+		if (ret)
+			return ret;
+
+		/* Update sampling frequency */
+		ret = ad7768_set_freq(st, st->samp_freq);
+		if (ret)
+			return ret;
+
+		return len;
+	default:
+		return -EINVAL;
+	}
+}
+
 static int ad7768_set_freq(struct ad7768_state *st,
 			   unsigned int freq)
 {
-	unsigned int diff_new, diff_old, pwr_mode, i, idx;
+	unsigned int diff_new, diff_old, i, idx;
 	int res, ret;
 
+	freq = clamp_t(unsigned int, freq, 50, 1024000);
 	diff_old = U32_MAX;
 	idx = 0;
 
-	res = DIV_ROUND_CLOSEST(st->mclk_freq, freq);
+	if (freq == 0)
+		return -EINVAL;
+
+	res = DIV_ROUND_CLOSEST(st->mclk_freq, freq * st->dec_rate);
 
 	/* Find the closest match for the desired sampling frequency */
-	for (i = 0; i < ARRAY_SIZE(ad7768_clk_config); i++) {
-		diff_new = abs(res - ad7768_clk_config[i].clk_div);
+	for (i = 0; i < ARRAY_SIZE(ad7768_mclk_div_rates); i++) {
+		diff_new = abs(res - ad7768_mclk_div_rates[i]);
 		if (diff_new < diff_old) {
 			diff_old = diff_new;
 			idx = i;
 		}
 	}
 
-	/*
-	 * Set both the mclk_div and pwrmode with a single write to the
-	 * POWER_CLOCK register
-	 */
-	pwr_mode = AD7768_PWR_MCLK_DIV(ad7768_clk_config[idx].mclk_div) |
-		   AD7768_PWR_PWRMODE(ad7768_clk_config[idx].pwrmode);
-	ret = ad7768_spi_reg_write(st, AD7768_REG_POWER_CLOCK, pwr_mode);
-	if (ret < 0)
-		return ret;
-
-	ret =  ad7768_set_dig_fil(st, ad7768_clk_config[idx].dec_rate);
-	if (ret < 0)
+	/* Set both the mclk_div and pwrmode */
+	ret = ad7768_set_mclk_div(st, idx);
+	if (ret)
 		return ret;
 
-	st->dec_rate = ad7768_clk_config[idx].clk_div /
-		       ad7768_mclk_div_rates[ad7768_clk_config[idx].mclk_div];
 	st->samp_freq = DIV_ROUND_CLOSEST(st->mclk_freq,
-					  ad7768_clk_config[idx].clk_div);
+					  ad7768_mclk_div_rates[idx] * st->dec_rate);
 
-	return 0;
+	/* A sync-in pulse is required every time the filter dec rate changes */
+	return ad7768_send_sync_pulse(st);
 }
 
 static int ad7768_get_vcm(struct iio_dev *dev, const struct iio_chan_spec *chan)
@@ -566,13 +827,16 @@ static ssize_t ad7768_sampling_freq_avail(struct device *dev,
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct ad7768_state *st = iio_priv(indio_dev);
-	unsigned int freq;
+	unsigned int freq, freq_filtered;
 	int i, len = 0;
 
-	for (i = 0; i < ARRAY_SIZE(ad7768_clk_config); i++) {
-		freq = DIV_ROUND_CLOSEST(st->mclk_freq,
-					 ad7768_clk_config[i].clk_div);
-		len += scnprintf(buf + len, PAGE_SIZE - len, "%d ", freq);
+	freq_filtered = DIV_ROUND_CLOSEST(st->mclk_freq, st->dec_rate);
+	for (i = 0; i < ARRAY_SIZE(ad7768_mclk_div_rates); i++) {
+		freq = DIV_ROUND_CLOSEST(freq_filtered,
+					 ad7768_mclk_div_rates[i]);
+		/* Sampling frequency cannot be lower than the minimum of 50 SPS */
+		if (freq >= 50)
+			len += sysfs_emit_at(buf, len, "%d ", freq);
 	}
 
 	buf[len - 1] = '\n';
@@ -582,6 +846,38 @@ static ssize_t ad7768_sampling_freq_avail(struct device *dev,
 
 static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(ad7768_sampling_freq_avail);
 
+static ssize_t decimation_rate_available_show(struct device *dev,
+					      struct device_attribute *attr,
+					      char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct ad7768_state *st = iio_priv(indio_dev);
+	int len = 0;
+
+	/* Return decimation rate available in range format */
+	buf[len++] = '[';
+	if (st->filter_type == SINC3) {
+		len += sysfs_emit_at(buf, len, "%d ", SINC3_DEC_RATE_MIN);
+		len += sysfs_emit_at(buf, len, "%d ", SINC3_DEC_RATE_MIN);
+		len += sysfs_emit_at(buf, len, "%d ", SINC3_DEC_RATE_MAX);
+	} else if (st->filter_type == WIDEBAND) {
+		len += sysfs_emit_at(buf, len, "%d ", WIDEBAND_DEC_RATE_MIN);
+		len += sysfs_emit_at(buf, len, "%d ", WIDEBAND_DEC_RATE_MIN);
+		len += sysfs_emit_at(buf, len, "%d ", WIDEBAND_DEC_RATE_MAX);
+	} else {
+		len += sysfs_emit_at(buf, len, "%d ", SINC5_DEC_RATE_MIN);
+		len += sysfs_emit_at(buf, len, "%d ", SINC5_DEC_RATE_MIN);
+		len += sysfs_emit_at(buf, len, "%d ", SINC5_DEC_RATE_MAX);
+	}
+
+	buf[len - 1] = ']';
+	buf[len++] = '\n';
+
+	return len;
+}
+
+static IIO_DEVICE_ATTR_RO(decimation_rate_available, 0);
+
 static int ad7768_read_raw(struct iio_dev *indio_dev,
 			   struct iio_chan_spec const *chan,
 			   int *val, int *val2, long info)
@@ -653,6 +949,7 @@ static int ad7768_read_label(struct iio_dev *indio_dev,
 
 static struct attribute *ad7768_attributes[] = {
 	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
+	&iio_dev_attr_decimation_rate_available.dev_attr.attr,
 	NULL
 };
 
@@ -678,7 +975,7 @@ static const struct iio_info ad7768_info = {
 	.debugfs_reg_access = &ad7768_reg_access,
 };
 
-static int ad7768_setup(struct ad7768_state *st)
+static int ad7768_setup(struct ad7768_state *st, struct iio_dev *indio_dev)
 {
 	int ret;
 
@@ -727,6 +1024,12 @@ static int ad7768_setup(struct ad7768_state *st)
 	if (ret < 0)
 		return ret;
 
+	/*
+	 * Set Default Digital Filter configuration:
+	 * SINC5 filter with x32 Decimation rate
+	 */
+	ret = ad7768_configure_dig_fil(indio_dev, SINC5, 32);
+
 	/* Set the default sampling frequency to 32000 kSPS */
 	return ad7768_set_freq(st, 32000);
 }
@@ -915,7 +1218,7 @@ static int ad7768_probe(struct spi_device *spi)
 	indio_dev->info = &ad7768_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
-	ret = ad7768_setup(st);
+	ret = ad7768_setup(st, indio_dev);
 	if (ret < 0) {
 		dev_err(&spi->dev, "AD7768 setup failed\n");
 		return ret;
-- 
2.34.1

Re: [PATCH v1 00/15] iio: adc: ad7768-1: Add features, improvements, and fixes

[David Lechner]

On 1/7/25 9:23 AM, Jonathan Santos wrote:
> This patch series introduces some new features, improvements,
> and fixes for the AD7768-1 ADC driver. 
> 
> The goal is to support all key functionalities listed in the device
> datasheet, including filter mode selection, common mode voltage 
> configuration and GPIO support. Additionally, this includes fixes 
> for SPI communication and for IIO interface, and also code improvements
> to enhance maintainability and readability.

It looks like some of the patches also have to do with a sync trigger for one
or more chips. There was a recent discussion about this for another ADC that
seems relevant:

[1]: https://lore.kernel.org/linux-iio/20241128125811.11913-1-alisa.roman@analog.com/

> 
> Jonathan Santos (8):
>   dt-bindings: iio: adc: ad7768-1: add synchronization over SPI property
>   Documentation: ABI: add wideband filter type to  sysfs-bus-iio
>   Documentation: ABI: testing: ad7768-1: Add device specific ABI
>     documentation.
>   iio: adc: ad7768-1: set MOSI idle state to high
>   iio: adc: ad7768-1: use guard(mutex) to simplify code
>   iio: adc: ad7768-1: add multiple scan types to support 16-bits mode
>   iio: adc: ad7768-1: add support for Synchronization over SPI
>   iio: adc: ad7768-1: add filter type and decimation rate attributes
> 
> Sergiu Cuciurean (7):
>   iio: adc: ad7768-1: Fix conversion result sign
>   iio: adc: ad7768-1: Update reg_read function
>   iio: adc: ad7768-1: Add reset gpio
>   iio: adc: ad7768-1: Move buffer allocation to a separate function
>   iio: adc: ad7768-1: Add support for variable VCM
>   iio: adc: ad7768-1: Add reg_write_masked function
>   iio: adc: ad7768-1: Add GPIO controller support
> 
>  Documentation/ABI/testing/sysfs-bus-iio       |   2 +
>  .../ABI/testing/sysfs-bus-iio-adc-ad7768-1    |  13 +
>  .../bindings/iio/adc/adi,ad7768-1.yaml        |  24 +-
>  drivers/iio/adc/ad7768-1.c                    | 830 +++++++++++++++---
>  4 files changed, 747 insertions(+), 122 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-ad7768-1
> 
> 
> base-commit: 5de07b8a24cf44cdb78adeab790704bf577c2c1d

What are all of these prerequisites?

> prerequisite-patch-id: 8b531bca46f7c7ea1c0f6d232d162fd05fda52f7
> prerequisite-patch-id: c8c8637cb3343097c3224a9aa315fc45dca15f45
> prerequisite-patch-id: e0baac9ef84e6e8a0a272fc6191fc1cb48143e44
> prerequisite-patch-id: 7e5cad70809fa7d37e917628147c197427c11594
> prerequisite-patch-id: 28dcdb0ebc3ca3c02713f83c94a4eedbe81095dc
> ...

Re: [PATCH v1 01/15] dt-bindings: iio: adc: ad7768-1: add synchronization over SPI property

[David Lechner]

On 1/7/25 9:24 AM, Jonathan Santos wrote:
> Add adi,sync-in-spi property to enable synchronization over SPI.
> This should be used in the case when the GPIO cannot provide a
> pulse synchronous with the base MCLK signal.
> 
> User can choose between SPI, GPIO synchronization or neither of them,
> but only if a external pulse can be provided, for example, by another
> device in a multidevice setup.
> 

While we are fixing up these bindings, we could add some more trivial things,
like power supplies.

Also, the interrupt property could use a description since the chip has multiple
output pins. I assume it means the /DRDY pin?

> Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> ---
>  .../bindings/iio/adc/adi,ad7768-1.yaml        | 24 ++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
> index 3ce59d4d065f..55cec27bfe60 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
> @@ -47,6 +47,15 @@ properties:
>        in any way, for example if the filter decimation rate changes.
>        As the line is active low, it should be marked GPIO_ACTIVE_LOW.
>  
> +  adi,sync-in-spi:

If this is saying that SYNC_OUT is connected to SYNC_IN, then I think the name
should be something like adi,sync-in-sync-out. SPI seems irrelevant here since
we should just be describing how things are wired up, not how it is being used.

But if we also need to consider the case where SYNC_OUT of one chip is connected
to SYNC_IN of another chip, we might want to consider using trigger-source
bindings instead (recently standardized in dtschema).

> +    description:
> +      Enables synchronization of multiple devices over SPI. This property is
> +      used when a signal synchronous to the base MCLK signal cannot be provided
> +      via GPIO. It requires the SYNC_OUT pin to be connected to the SYNC_IN pin
> +      on the ADC. In the case of multiple devices, the SYNC_OUT pin of one device
> +      should be routed to the SYNC_IN pins of the other devices.
> +    type: boolean
> +
>    reset-gpios:
>      maxItems: 1
>  
> @@ -65,7 +74,6 @@ required:
>    - vref-supply
>    - spi-cpol
>    - spi-cpha
> -  - adi,sync-in-gpios
>  
>  patternProperties:
>    "^channel@([0-9]|1[0-5])$":
> @@ -89,6 +97,20 @@ patternProperties:
>  allOf:
>    - $ref: /schemas/spi/spi-peripheral-props.yaml#
>  
> +  # adi,sync-in-gpios and adi,sync-in-spi are mutually exclusive (neither is also valid)
> +  - if:
> +      required:
> +        - adi,sync-in-gpios
> +    then:
> +      properties:
> +        adi,sync-in-spi: false
> +  - if:
> +      required:
> +        - adi,sync-in-spi
> +    then:
> +      properties:
> +        adi,sync-in-gpios: false

I think this can be simplified to using oneOf: to provide XOR validation

> +
>  unevaluatedProperties: false
>  
>  examples:

Re: [PATCH v1 02/15] Documentation: ABI: add wideband filter type to sysfs-bus-iio

[David Lechner]

On 1/7/25 9:24 AM, Jonathan Santos wrote:
> The Wideband Low Ripple FIR filter is used for AD7768-1 Driver.
> Document wideband filter option into filter_type_avaialable

s/avaialable/available/

> attribute.
> 
> Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-iio | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index f83bd6829285..c4c21a7bfba1 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -2291,6 +2291,8 @@ Description:
>  		* "sinc3+pf2" - Sinc3 + device specific Post Filter 2.
>  		* "sinc3+pf3" - Sinc3 + device specific Post Filter 3.
>  		* "sinc3+pf4" - Sinc3 + device specific Post Filter 4.
> +		* "wideband" - FIR filter with wideband low ripple passband

I think "fir" would be a more specific filter type name than "wideband". (i.e.
there are wikipedia pages for sinc and FIR filters, but not one for "wideband"
filters)

> +		  and sharp transition band.
>  
>  What:		/sys/.../events/in_proximity_thresh_either_runningperiod
>  KernelVersion:	6.6

Re: [PATCH v1 03/15] Documentation: ABI: testing: ad7768-1: Add device specific ABI documentation.

[David Lechner]

On 1/7/25 9:24 AM, Jonathan Santos wrote:
> Add ABI documentation specific to the ad7768-1 device, detailing
> the decimation_rate attribute for better clarity and usability.
> 
> Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> ---
>  .../ABI/testing/sysfs-bus-iio-adc-ad7768-1          | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-ad7768-1
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7768-1 b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7768-1
> new file mode 100644
> index 000000000000..065247f07cfb
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7768-1
> @@ -0,0 +1,13 @@
> +What:		/sys/bus/iio/devices/iio:deviceX/decimation_rate_available
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Reading returns a range of possible decimation rate values.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/decimation_rate
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Sets up the decimation rate for the digital filter. This can
> +		directly impact in the final sampling frequency. Reading returns
> +		the decimation rate. Writing sets the decimation rate.

If this only affects the filter, I would suggest to add `filter_` to the
beginning of the attribute names.

Also, an explanation of how to interpret the numbers would be helpful. It looks
like a unitless number that acts a sort of a multiplier or divider, but that
part isn't so clear to me. 

Or...

Since the decimation rate affects the -3dB point of the filters we could use
the standard IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY instead of introducing
a new attribute.

Re: [PATCH v1 04/15] iio: adc: ad7768-1: Fix conversion result sign

[David Lechner]

On 1/7/25 9:25 AM, Jonathan Santos wrote:
> From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> 
> The ad7768-1 is a fully differential ADC, meaning that the voltage
> conversion result is a signed value. Since the value is a 24 bit one,
> stored in a 32 bit variable, the sign should be extended in order to get
> the correct representation.
> 
> Also the channel description has been updated to signed representation,
> to match the ADC specifications.
> 
> Fixes: a5f8c7da3dbe ("iio: adc: Add AD7768-1 ADC basic support")
> Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> ---
Reviewed-by: David Lechner <dlechner@baylibre.com>

Re: [PATCH v1 05/15] iio: adc: ad7768-1: set MOSI idle state to high

[David Lechner]

On 1/7/25 9:25 AM, Jonathan Santos wrote:
> All supported parts require that the MOSI line stays high
> while in idle.
> 
> Configure SPI controller to set MOSI idle state to high.
> 
> Fixes: a5f8c7da3dbe ("iio: adc: Add AD7768-1 ADC basic support")
> Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> ---
>  drivers/iio/adc/ad7768-1.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> index c3cf04311c40..463a28d09c2e 100644
> --- a/drivers/iio/adc/ad7768-1.c
> +++ b/drivers/iio/adc/ad7768-1.c
> @@ -574,6 +574,15 @@ static int ad7768_probe(struct spi_device *spi)
>  		return -ENOMEM;
>  
>  	st = iio_priv(indio_dev);
> +	/*
> +	 * The ADC SDI line must be kept high when
> +	 * data is not being clocked out of the controller.
> +	 * Request the SPI controller to make MOSI idle high.
> +	 */
> +	spi->mode |= SPI_MOSI_IDLE_HIGH;
> +	ret = spi_setup(spi);
> +	if (ret < 0)
> +		return ret;
>  	st->spi = spi;
>  
>  	st->vref = devm_regulator_get(&spi->dev, "vref");

Very few SPI controllers currently have the SPI_MOSI_IDLE_HIGH capability flag
set in Linux right now (whether they actually support it or not), so this could
break existing users.

The datasheet says:

	When reading back data with CS held low, it is recommended that SDI
	idle high to prevent an accidental reset of the device where SCLK is
	free running (see the Reset section).

And the reset section says:

	When CS is held low, it is possible to provide a reset by clocking
	in a 1 followed by 63 zeros on SDI, which is the SPI resume command
	reset function used to exit power-down mode.

Since the largest xfer we do is 3 bytes before deasserting CS, I don't think
we have any risk of accidentally resetting right now.

If we ever do implement a data read of more than 64 bits without toggling CS,
then we could just set the TX data to be all 0xFF and have the same effect
without requiring the SPI controller to support SPI_MOSI_IDLE_HIGH.

Re: [PATCH v1 07/15] iio: adc: ad7768-1: Add reset gpio

[David Lechner]

On 1/7/25 9:25 AM, Jonathan Santos wrote:
> From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> 
> Depending on the controller, the default state of a gpio can vary. This
> change excludes the probability that the dafult state of the ADC reset
> gpio will be HIGH if it will be passed as reference in the deivcetree.
> 
> Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> ---
>  drivers/iio/adc/ad7768-1.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> index 881446462ff5..f73b9aec8b0f 100644
> --- a/drivers/iio/adc/ad7768-1.c
> +++ b/drivers/iio/adc/ad7768-1.c
> @@ -161,6 +161,7 @@ struct ad7768_state {
>  	struct completion completion;
>  	struct iio_trigger *trig;
>  	struct gpio_desc *gpio_sync_in;
> +	struct gpio_desc *gpio_reset;
>  	const char *labels[ARRAY_SIZE(ad7768_channels)];
>  	/*
>  	 * DMA (thus cache coherency maintenance) may require the
> @@ -441,6 +442,18 @@ static int ad7768_setup(struct ad7768_state *st)
>  {
>  	int ret;
>  
> +	st->gpio_reset = devm_gpiod_get_optional(&st->spi->dev, "reset",
> +						 GPIOD_OUT_LOW);

Could be simplified by setting this to GPIOD_OUT_HIGH and drop

	gpiod_direction_output(st->gpio_reset, 1);

> +	if (IS_ERR(st->gpio_reset))
> +		return PTR_ERR(st->gpio_reset);
> +
> +	if (st->gpio_reset) {
> +		gpiod_direction_output(st->gpio_reset, 1);
> +		usleep_range(10, 15);
> +		gpiod_direction_output(st->gpio_reset, 0);
> +		usleep_range(10, 15);

prefer fsleep()

> +	}
> +

We can move the code below in an else since we don't need to do 2 resets.

>  	/*
>  	 * Two writes to the SPI_RESET[1:0] bits are required to initiate
>  	 * a software reset. The bits must first be set to 11, and then

Re: [PATCH v1 08/15] iio: adc: ad7768-1: use guard(mutex) to simplify code

[David Lechner]

On 1/7/25 9:25 AM, Jonathan Santos wrote:
> Use guard(mutex) from cleanup.h to remove most of the gotos and to make
> the code simpler and less likely to fail due to forgetting to unlock
> the resources.
> 
> Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> ---

...

> @@ -484,7 +477,7 @@ static irqreturn_t ad7768_trigger_handler(int irq, void *p)
>  	struct ad7768_state *st = iio_priv(indio_dev);
>  	int ret;
>  
> -	mutex_lock(&st->lock);
> +	guard(mutex)(&st->lock);
>  
>  	ret = spi_read(st->spi, &st->data.scan.chan, 3);
>  	if (ret < 0)
> @@ -495,7 +488,6 @@ static irqreturn_t ad7768_trigger_handler(int irq, void *p)
>  
>  err_unlock:

nit: also rename the label since it is no longer unlocking

>  	iio_trigger_notify_done(indio_dev->trig);
> -	mutex_unlock(&st->lock);
>  
>  	return IRQ_HANDLED;
>  }

I'm also wondering if we should just drop this lock. It is only protecting
a triggered buffer SPI xfer and debugfs register access from happening at the
same time.

Since we have to write a magic value to exit conversion mode, reading registers
during a buffered read is going to cause problems anyway. So we could just
remove the mutex lock and use iio_device_claim_direct_mode() instead in
ad7768_reg_access().

Re: [PATCH v1 10/15] iio: adc: ad7768-1: Add support for variable VCM

[David Lechner]

On 1/7/25 9:26 AM, Jonathan Santos wrote:
> From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> 
> The VCM output voltage can be used as a common-mode voltage within the
> amplifier preconditioning circuits external to the AD7768-1.
> 
> This change exports the VCM as an iio attribute and makes it available
> for configuration.

We model common mode voltage inputs as a regulator consumer (e.g. vcm-supply in
DT), so should we make this a regulator provider instead?

It could be used with "regulator-output" to be able to control it from sysfs
or if the amplifier ends up in the devicetree for other reasons, the amplifier
driver could control the regulator instead of requiring it to by done in sysfs.

Re: [PATCH v1 11/15] iio: adc: ad7768-1: Add reg_write_masked function

[David Lechner]

On 1/7/25 9:26 AM, Jonathan Santos wrote:
> From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> 
> This commit adds the ad7768_spi_reg_write_masked() which is a helper
> function for writing specific bits inside a register, without interfering
> with the other bit values.
> 
> Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> ---
>  drivers/iio/adc/ad7768-1.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> index 574d735f2c3a..675af9ea856d 100644
> --- a/drivers/iio/adc/ad7768-1.c
> +++ b/drivers/iio/adc/ad7768-1.c
> @@ -242,6 +242,21 @@ static int ad7768_spi_reg_write(struct ad7768_state *st,
>  	return spi_write(st->spi, st->data.d8, 2);
>  }
>  
> +static int ad7768_spi_reg_write_masked(struct ad7768_state *st,
> +				       unsigned int addr,
> +				       unsigned int mask,
> +				       unsigned int val)
> +{
> +	unsigned int reg_val;
> +	int ret;
> +
> +	ret = ad7768_spi_reg_read(st, addr, &reg_val, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	return ad7768_spi_reg_write(st, addr, (reg_val & ~mask) | val);
> +}
> +
>  static int ad7768_set_mode(struct ad7768_state *st,
>  			   enum ad7768_conv_mode mode)
>  {

Can we convert this driver to use regmap instead of reinventing it?

Re: [PATCH v1 12/15] iio: adc: ad7768-1: Add GPIO controller support

[David Lechner]

On 1/7/25 9:26 AM, Jonathan Santos wrote:
> From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> 
> The AD7768-1 has the ability to control other local hardware (such as gain
> stages),to power down other blocks in the signal chain, or read local
> status signals over the SPI interface.
> 
> This change exports the AD7768-1's four gpios and makes them accessible
> at an upper layer.

We will also need a dt-bindings patch to add gpio-controller and #gpio-cells
properties.

> 
> Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> ---
>  drivers/iio/adc/ad7768-1.c | 121 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 121 insertions(+)
> 
> diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> index 675af9ea856d..9741a6d47942 100644
> --- a/drivers/iio/adc/ad7768-1.c
> +++ b/drivers/iio/adc/ad7768-1.c
> @@ -10,6 +10,8 @@
>  #include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/err.h>
> +#include <linux/gpio.h>
> +#include <linux/gpio/driver.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> @@ -77,6 +79,19 @@
>  #define AD7768_CONV_MODE_MSK		GENMASK(2, 0)
>  #define AD7768_CONV_MODE(x)		FIELD_PREP(AD7768_CONV_MODE_MSK, x)
>  
> +/* AD7768_REG_GPIO_CONTROL */
> +#define AD7768_GPIO_CONTROL_MSK		GENMASK(3, 0)
> +#define AD7768_GPIO_UNIVERSAL_EN	BIT(7)

Bits should be ordered highest to lowest (from top to bottom) to be consistent
with other code.

> +
> +/* AD7768_REG_GPIO_WRITE */
> +#define AD7768_GPIO_WRITE_MSK		GENMASK(3, 0)
> +
> +/* AD7768_REG_GPIO_READ */
> +#define AD7768_GPIO_READ_MSK		GENMASK(3, 0)
> +
> +#define AD7768_GPIO_INPUT(x)		0x00
> +#define AD7768_GPIO_OUTPUT(x)		BIT(x)
> +
>  #define AD7768_RD_FLAG_MSK(x)		(BIT(6) | ((x) & 0x3F))
>  #define AD7768_WR_FLAG_MSK(x)		((x) & 0x3F)
>  
> @@ -190,6 +205,8 @@ struct ad7768_state {
>  	struct regulator *vref;
>  	struct mutex lock;
>  	struct clk *mclk;
> +	struct gpio_chip gpiochip;
> +	unsigned int gpio_avail_map;
>  	unsigned int mclk_freq;
>  	unsigned int samp_freq;
>  	unsigned int common_mode_voltage;
> @@ -338,6 +355,106 @@ static int ad7768_set_dig_fil(struct ad7768_state *st,
>  	return 0;
>  }
>  
> +static int ad7768_gpio_direction_input(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct ad7768_state *st = gpiochip_get_data(chip);
> +
> +	guard(mutex)(&st->lock);

As mentioned in another patch, this should be iio_device_claim_direct_mode()
instead of st->lock since we can't access registers during a buffered read
because the chip is in conversion mode. Same applies to other functions below.

> +	return ad7768_spi_reg_write_masked(st,
> +					  AD7768_REG_GPIO_CONTROL,
> +					  BIT(offset),
> +					  AD7768_GPIO_INPUT(offset));
> +}
> +
> +static int ad7768_gpio_direction_output(struct gpio_chip *chip,
> +					unsigned int offset, int value)
> +{
> +	struct ad7768_state *st = gpiochip_get_data(chip);
> +
> +	guard(mutex)(&st->lock);
> +	return ad7768_spi_reg_write_masked(st,
> +					  AD7768_REG_GPIO_CONTROL,
> +					  BIT(offset),
> +					  AD7768_GPIO_OUTPUT(offset));
> +}
> +
> +static int ad7768_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct ad7768_state *st = gpiochip_get_data(chip);
> +	unsigned int val;
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +	ret = ad7768_spi_reg_read(st, AD7768_REG_GPIO_CONTROL, &val, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (val & BIT(offset))
> +		ret = ad7768_spi_reg_read(st, AD7768_REG_GPIO_WRITE, &val, 1);
> +	else
> +		ret = ad7768_spi_reg_read(st, AD7768_REG_GPIO_READ, &val, 1);

It doesn't work to just always read AD7768_REG_GPIO_READ?

> +	if (ret < 0)
> +		return ret;
> +
> +	return !!(val & BIT(offset));
> +}
> +
> +static void ad7768_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
> +{
> +	struct ad7768_state *st = gpiochip_get_data(chip);
> +	unsigned int val;
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +	ret = ad7768_spi_reg_read(st, AD7768_REG_GPIO_CONTROL, &val, 1);
> +	if (ret < 0)
> +		return;
> +
> +	if (val & BIT(offset))
> +		ad7768_spi_reg_write_masked(st,
> +					    AD7768_REG_GPIO_WRITE,
> +					    BIT(offset),
> +					    (value << offset));
> +}
> +
> +static int ad7768_gpio_request(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct ad7768_state *st = gpiochip_get_data(chip);
> +
> +	if (!(st->gpio_avail_map & BIT(offset)))
> +		return -ENODEV;
> +
> +	st->gpio_avail_map &= ~BIT(offset);
> +
> +	return 0;
> +}
> +
> +static int ad7768_gpio_init(struct ad7768_state *st)
> +{
> +	int ret;
> +
> +	ret = ad7768_spi_reg_write(st,
> +				   AD7768_REG_GPIO_CONTROL,

nit: this can fit on the previous line

> +				   AD7768_GPIO_UNIVERSAL_EN);
> +	if (ret < 0)
> +		return ret;
> +
> +	st->gpio_avail_map = AD7768_GPIO_CONTROL_MSK;
> +	st->gpiochip.label = "ad7768_1_gpios";
> +	st->gpiochip.base = -1;
> +	st->gpiochip.ngpio = 4;
> +	st->gpiochip.parent = &st->spi->dev;
> +	st->gpiochip.can_sleep = true;
> +	st->gpiochip.direction_input = ad7768_gpio_direction_input;
> +	st->gpiochip.direction_output = ad7768_gpio_direction_output;
> +	st->gpiochip.get = ad7768_gpio_get;
> +	st->gpiochip.set = ad7768_gpio_set;
> +	st->gpiochip.request = ad7768_gpio_request;
> +	st->gpiochip.owner = THIS_MODULE;
> +
> +	return gpiochip_add_data(&st->gpiochip, st);
> +}
> +
>  static int ad7768_set_freq(struct ad7768_state *st,
>  			   unsigned int freq)
>  {
> @@ -538,6 +655,10 @@ static int ad7768_setup(struct ad7768_state *st)
>  	if (IS_ERR(st->gpio_sync_in))
>  		return PTR_ERR(st->gpio_sync_in);
>  

Since the GPIO pins can also be wired up as mode input pins, this should not be
always enabled. We can use the `gpio-controller` DT property as a flag to
conditionally enable GPIO support when it is actually wired up for that purpose.

> +	ret = ad7768_gpio_init(st);
> +	if (ret < 0)
> +		return ret;
> +
>  	/* Set the default sampling frequency to 32000 kSPS */
>  	return ad7768_set_freq(st, 32000);
>  }

Re: [PATCH v1 13/15] iio: adc: ad7768-1: add multiple scan types to support 16-bits mode

[David Lechner]

On 1/7/25 9:26 AM, Jonathan Santos wrote:
> When the device is configured to Sinc5 filter and decimation x8,
> output data is reduced to 16-bits in order to support 1 MHz of
> sampling frequency due to clock limitation.

We aren't going to get a 1 MHz sample rate without SPI offload support so maybe
we should save this patch until then?

In this patch, we are still reading 24-bits per sample, so we aren't really
getting any benefit. It is probably fine for now to leave it as 24-bit even if
the last 8 bits are all 0 or just noise.

Also, the datasheet says:

	this path allows viewing of wider bandwidth; however, it is quantization
	noise limited so that output data is reduced to 16 bits

So this doesn't actually seem related to higher sample rates. There is a CONVLEN
bit in the INTERFACE_FORMAT register that globally reduces the output size to
16-bit, which I suspect would be what we will need for achieving the highest
sample rate when we add SPI offload support.

> 
> Use multiple scan types feature to enable the driver to switch
> scan type in runtime, making possible to support both 24-bit and
> 16-bit resolution.
> 
> Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> ---
>  drivers/iio/adc/ad7768-1.c | 65 ++++++++++++++++++++++++++++++++------
>  1 file changed, 56 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> index 9741a6d47942..5e4e7d387f9a 100644
> --- a/drivers/iio/adc/ad7768-1.c
> +++ b/drivers/iio/adc/ad7768-1.c
> @@ -134,6 +134,11 @@ struct ad7768_clk_configuration {
>  	enum ad7768_pwrmode pwrmode;
>  };
>  
> +enum ad7768_scan_type {
> +	AD7768_SCAN_TYPE_NORMAL,
> +	AD7768_SCAN_TYPE_HIGH_SPEED,
> +};
> +
>  static const char * const ad7768_vcm_modes[] = {
>  	"(AVDD1-AVSS)/2",
>  	"2V5",
> @@ -145,6 +150,10 @@ static const char * const ad7768_vcm_modes[] = {
>  	"OFF",
>  };
>  
> +static const int ad7768_mclk_div_rates[4] = {
> +	16, 8, 4, 2,
> +};
> +
>  static const struct ad7768_clk_configuration ad7768_clk_config[] = {
>  	{ AD7768_MCLK_DIV_2, AD7768_DEC_RATE_8, 16,  AD7768_FAST_MODE },
>  	{ AD7768_MCLK_DIV_2, AD7768_DEC_RATE_16, 32,  AD7768_FAST_MODE },
> @@ -159,6 +168,21 @@ static const struct ad7768_clk_configuration ad7768_clk_config[] = {
>  	{ AD7768_MCLK_DIV_16, AD7768_DEC_RATE_1024, 16384, AD7768_ECO_MODE },
>  };
>  
> +static const struct iio_scan_type ad7768_scan_type[] = {
> +	[AD7768_SCAN_TYPE_NORMAL] = {
> +		.sign = 's',
> +		.realbits = 24,
> +		.storagebits = 32,

What happened to .shift = 8, ? If there is a reason for removing it, please add
that to the commit description.

> +		.endianness = IIO_BE,
> +	},
> +	[AD7768_SCAN_TYPE_HIGH_SPEED] = {
> +		.sign = 's',
> +		.realbits = 16,
> +		.storagebits = 32,

I guess it doesn't matter much since we are reading one sample at a time, but
I would expect storagebits to be 16 instead of 32. Or if it really needs to be
32, does it need shift = 16?

> +		.endianness = IIO_BE,
> +	},
> +};
> +
>  static int ad7768_get_vcm(struct iio_dev *dev, const struct iio_chan_spec *chan);
>  static int ad7768_set_vcm(struct iio_dev *dev, const struct iio_chan_spec *chan,
>  			  unsigned int mode);

...

> @@ -308,6 +329,15 @@ static int ad7768_scan_direct(struct iio_dev *indio_dev)
>  	ret = ad7768_spi_reg_read(st, AD7768_REG_ADC_DATA, &readval, 3);
>  	if (ret < 0)
>  		return ret;
> +
> +	/*
> +	 * When the decimation rate is set to x8, the ADC data precision is reduced
> +	 * from 24 bits to 16 bits. Since the AD7768_REG_ADC_DATA register provides
> +	 * 24-bit data, the precision is reduced by right-shifting the read value
> +	 * by 8 bits.
> +	 */
> +	if (st->dec_rate == 8)
> +		readval = readval >> 8;

Why not change size of ad7768_spi_reg_read() instead of reading 3 bytes and
throwing one away?

>  	/*
>  	 * Any SPI configuration of the AD7768-1 can only be
>  	 * performed in continuous conversion mode.

Re: [PATCH v1 14/15] iio: adc: ad7768-1: add support for Synchronization over SPI

[David Lechner]

On 1/7/25 9:27 AM, Jonathan Santos wrote:
> The synchronization method using GPIO requires the generated pulse to be
> truly synchronous with the base MCLK signal. When it is not possible to
> do that in hardware, the datasheet recommends using synchronization over
> SPI, where the generated pulse is already synchronous with MCLK. This
> requires the SYNC_OUT pin to be connected to SYNC_IN pin.
> 
> Add the option to handle device synchronization over SPI.
> 
> Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> ---

...

>  static int ad7768_gpio_direction_input(struct gpio_chip *chip, unsigned int offset)
> @@ -697,11 +708,21 @@ static int ad7768_setup(struct ad7768_state *st)
>  	if (ret)
>  		return ret;
>  
> -	st->gpio_sync_in = devm_gpiod_get(&st->spi->dev, "adi,sync-in",
> -					  GPIOD_OUT_LOW);
> +	st->gpio_sync_in = devm_gpiod_get_optional(&st->spi->dev, "adi,sync-in",
> +						   GPIOD_OUT_LOW);
>  	if (IS_ERR(st->gpio_sync_in))
>  		return PTR_ERR(st->gpio_sync_in);
>  
> +	if (device_property_present(&st->spi->dev, "adi,sync-in-spi"))
> +		st->en_spi_sync = true;
> +
> +	/*
> +	 * GPIO and SPI Synchronization are mutually exclusive.
> +	 * Return error if both are enabled

Should it also be an error if we have neither? Otherwise it sounds like
decimation won't work correctly since there is a comment that says we have
to toggle this after updating the decimation rate register.

> +	 */
> +	if (st->gpio_sync_in && st->en_spi_sync)
> +		return -EINVAL;

A dev_err_probe() message would be helpful here when creating a new DT and
bringing up a new system since it is easy to forget a property or make a typo
that could lead to this error.

> +
>  	ret = ad7768_gpio_init(st);
>  	if (ret < 0)
>  		return ret;

Re: [PATCH v1 15/15] iio: adc: ad7768-1: add filter type and decimation rate attributes

[David Lechner]

On 1/7/25 9:27 AM, Jonathan Santos wrote:
> Separate filter type and decimation rate from the sampling frequency
> attribute. The new filter type attribute enables SINC3 and WIDEBAND
> filters, which were previously unavailable.

See related comments in my reply to the documentation patches about wideband vs.
FIR and decimation rate vs. -3dB cutoff.

> 
> Previously, combining decimation and MCLK divider in the sampling
> frequency obscured performance trade-offs. Lower MCLK divider
> settings increase power usage, while lower decimation rates reduce
> precision by decreasing averaging. By creating a decimation attribute,
> users gain finer control over performance.

It seems like we would also want a power_mode attribute. We already have an
attribute for this for used by accelerometers so there is some precedent for
such an attribute.

> 
> The addition of those attributes allows a wider range of sampling
> frequencies and more access to the device features.

Re: [PATCH v1 01/15] dt-bindings: iio: adc: ad7768-1: add synchronization over SPI property

[Marcelo Schmitt]

On 01/07, Jonathan Santos wrote:
> Add adi,sync-in-spi property to enable synchronization over SPI.
> This should be used in the case when the GPIO cannot provide a
> pulse synchronous with the base MCLK signal.
> 
> User can choose between SPI, GPIO synchronization or neither of them,
> but only if a external pulse can be provided, for example, by another
> device in a multidevice setup.
> 
> Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> ---
>  .../bindings/iio/adc/adi,ad7768-1.yaml        | 24 ++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
> index 3ce59d4d065f..55cec27bfe60 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
> @@ -47,6 +47,15 @@ properties:
>        in any way, for example if the filter decimation rate changes.
>        As the line is active low, it should be marked GPIO_ACTIVE_LOW.
>  
> +  adi,sync-in-spi:
> +    description:
> +      Enables synchronization of multiple devices over SPI. This property is
> +      used when a signal synchronous to the base MCLK signal cannot be provided
> +      via GPIO. It requires the SYNC_OUT pin to be connected to the SYNC_IN pin
> +      on the ADC. In the case of multiple devices, the SYNC_OUT pin of one device
> +      should be routed to the SYNC_IN pins of the other devices.
So, if I'm getting it right, /SYNC_IN may be driven by a GPIO (ADAQ7768-1
datasheet Figure 131), /SYNC_IN may be driven by own device /SYNC_OUT
(ADAQ7768-1 datasheet Figure 133), or /SYNC_IN may be driven by other AD7768-1
/SYNC_OUT pin (also Figure 133).
That is too much to describe with a boolean.

If David's suggestion of using a trigger-source doesn't fit, this property
should at least become an enum or string.

> +    type: boolean
> +
>    reset-gpios:
>      maxItems: 1
>  
> @@ -65,7 +74,6 @@ required:
>    - vref-supply
>    - spi-cpol
>    - spi-cpha
> -  - adi,sync-in-gpios
>  
>  patternProperties:
>    "^channel@([0-9]|1[0-5])$":
> @@ -89,6 +97,20 @@ patternProperties:
>  allOf:
>    - $ref: /schemas/spi/spi-peripheral-props.yaml#
>  
> +  # adi,sync-in-gpios and adi,sync-in-spi are mutually exclusive (neither is also valid)
> +  - if:
> +      required:
> +        - adi,sync-in-gpios
> +    then:
> +      properties:
> +        adi,sync-in-spi: false
> +  - if:
> +      required:
> +        - adi,sync-in-spi
> +    then:
> +      properties:
> +        adi,sync-in-gpios: false
> +
>  unevaluatedProperties: false
>  
>  examples:
> -- 
> 2.34.1
> 

Re: [PATCH v1 04/15] iio: adc: ad7768-1: Fix conversion result sign

[Marcelo Schmitt]

On 01/07, Jonathan Santos wrote:
> From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> 
> The ad7768-1 is a fully differential ADC, meaning that the voltage
> conversion result is a signed value. Since the value is a 24 bit one,
Hmm, I think the reason why we sign _raw values should be because of the ADC
output code format. There are differential ADCs that can measure a negative
difference between IN+ and IN- but outputting straight binary data format (not
signed values). In those cases, the _offset attribute is used to "shift" the
_raw value so that output codes that represent IN+ < IN- are adjusted to a
negative decimal value (the _raw + _offset part of IIO ABI to get to mV units).
For AD7768-1/ADAQ7768-1, the ADC output code is indeed two's complement and thus
signed so the code change is correct for it.
Since you are probably going to re-spin on the patch series, will be nice
to adjust the message to something like:
The ad7768-1 ADC output code is two's complement, meaning that the voltage
conversion result is a signed value. ...

With that,
Reviewed-by: Marcelo Schmitt <marcelo.schmitt@analog.com>

> stored in a 32 bit variable, the sign should be extended in order to get
> the correct representation.
> 
> Also the channel description has been updated to signed representation,
> to match the ADC specifications.
> 
> Fixes: a5f8c7da3dbe ("iio: adc: Add AD7768-1 ADC basic support")
> Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> ---
>  drivers/iio/adc/ad7768-1.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> index 113703fb7245..c3cf04311c40 100644
> --- a/drivers/iio/adc/ad7768-1.c
> +++ b/drivers/iio/adc/ad7768-1.c
> @@ -142,7 +142,7 @@ static const struct iio_chan_spec ad7768_channels[] = {
>  		.channel = 0,
>  		.scan_index = 0,
>  		.scan_type = {
> -			.sign = 'u',
> +			.sign = 's',
>  			.realbits = 24,
>  			.storagebits = 32,
>  			.shift = 8,
> @@ -371,7 +371,7 @@ static int ad7768_read_raw(struct iio_dev *indio_dev,
>  
>  		ret = ad7768_scan_direct(indio_dev);
>  		if (ret >= 0)
> -			*val = ret;
> +			*val = sign_extend32(ret, chan->scan_type.realbits - 1);
>  
>  		iio_device_release_direct_mode(indio_dev);
>  		if (ret < 0)
> -- 
> 2.34.1
> 

Re: [PATCH v1 05/15] iio: adc: ad7768-1: set MOSI idle state to high

[Marcelo Schmitt]

On 01/07, David Lechner wrote:
> On 1/7/25 9:25 AM, Jonathan Santos wrote:
> > All supported parts require that the MOSI line stays high
> > while in idle.
> > 
> > Configure SPI controller to set MOSI idle state to high.
> > 
> > Fixes: a5f8c7da3dbe ("iio: adc: Add AD7768-1 ADC basic support")
> > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> > ---
...
> > @@ -574,6 +574,15 @@ static int ad7768_probe(struct spi_device *spi)
> >  		return -ENOMEM;
> >  
> >  	st = iio_priv(indio_dev);
> > +	/*
> > +	 * The ADC SDI line must be kept high when
> > +	 * data is not being clocked out of the controller.
> > +	 * Request the SPI controller to make MOSI idle high.
> > +	 */
> > +	spi->mode |= SPI_MOSI_IDLE_HIGH;
> > +	ret = spi_setup(spi);
> > +	if (ret < 0)
> > +		return ret;
> >  	st->spi = spi;
> >  
> >  	st->vref = devm_regulator_get(&spi->dev, "vref");
> 
> Very few SPI controllers currently have the SPI_MOSI_IDLE_HIGH capability flag
> set in Linux right now (whether they actually support it or not), so this could
> break existing users.

Good point. Maybe only dev_warn() if SPI_MOSI_IDLE_HIGH is not supported?

> 
...
> 
> If we ever do implement a data read of more than 64 bits without toggling CS,
> then we could just set the TX data to be all 0xFF and have the same effect
> without requiring the SPI controller to support SPI_MOSI_IDLE_HIGH.

One point of having SPI_MOSI_IDLE_HIGH is that the controller may bring MOSI low
between data words of a transfer. I think all transfer words are going to be
either 16 or 24 with the new patches setting bits_per_word in all transfers but
that might still not be enough if eventually the controller is unable to support
those word sizes. Plus you would have the complication of filling the tx_buf for
all transfers.

For the part that instigated the development of SPI_MOSI_IDLE_HIGH, the MOSI line
also had to be high in between transfers. The diagrams at AD7768-1 datasheet
page 51 suggest the same would be needed for this chip too.

Re: [PATCH v1 06/15] iio: adc: ad7768-1: Update reg_read function

[Marcelo Schmitt]

On 01/07, Jonathan Santos wrote:
> From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> 
> This patch adds an additional parameter to the register read function.
> By passing the data pointer to the function, the returned value will used
> only for status check.
> With this change, the status check won't be confused with a register value
> check:
> 
> ret = ad7768_spi_reg_read()
> if (ret){}
> 
> Also this change removes the probability to interpret a negative value as
> a return code (the ADC is a differential one)."
> 
> Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> ---
...
> @@ -190,7 +190,9 @@ static int ad7768_spi_reg_read(struct ad7768_state *st, unsigned int addr,
>  	if (ret < 0)
>  		return ret;
>  
> -	return (be32_to_cpu(st->data.d32) >> shift);
> +	*data = (be32_to_cpu(st->data.d32) >> shift);
Are the outer parenthesis needed?
*data = be32_to_cpu(st->data.d32) >> shift;
?

> +
> +	return ret;
>  }
>  

Re: [PATCH v1 09/15] iio: adc: ad7768-1: Move buffer allocation to a separate function

[Marcelo Schmitt]

On 01/07, Jonathan Santos wrote:
> From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> 
> This change moves the buffer allocation in a separate function, making
> space for adding another type of iio buffer if needed.
> 
> Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> ---
>  drivers/iio/adc/ad7768-1.c | 64 ++++++++++++++++++++++----------------
>  1 file changed, 37 insertions(+), 27 deletions(-)
> 
...
> @@ -625,37 +661,11 @@ static int ad7768_probe(struct spi_device *spi)
>  		return ret;
>  	}
>  
> -	st->trig = devm_iio_trigger_alloc(&spi->dev, "%s-dev%d",
> -					  indio_dev->name,
> -					  iio_device_id(indio_dev));
> -	if (!st->trig)
> -		return -ENOMEM;
> -
> -	st->trig->ops = &ad7768_trigger_ops;
> -	iio_trigger_set_drvdata(st->trig, indio_dev);
> -	ret = devm_iio_trigger_register(&spi->dev, st->trig);
> -	if (ret)
> -		return ret;
> -
> -	indio_dev->trig = iio_trigger_get(st->trig);
> -
> -	init_completion(&st->completion);

Isn't the completion also used for single-shot reads?
Well, if triggered_buffer_setup fails the whole probe fails and we never get to
do a single-shot read, but that makes me wonder ... why don't we ever try to
recover from iio_triggered_buffer_setup()? Should we ever do so?

> -
>  	ret = ad7768_set_channel_label(indio_dev, ARRAY_SIZE(ad7768_channels));
>  	if (ret)
>  		return ret;
>  
> -	ret = devm_request_irq(&spi->dev, spi->irq,
> -			       &ad7768_interrupt,
> -			       IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> -			       indio_dev->name, indio_dev);
> -	if (ret)
> -		return ret;
> -
> -	ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
> -					      &iio_pollfunc_store_time,
> -					      &ad7768_trigger_handler,
> -					      &ad7768_buffer_ops);
> +	ret = ad7768_triggered_buffer_alloc(indio_dev);
>  	if (ret)
>  		return ret;
>  
> -- 
> 2.34.1
> 

Re: [PATCH v1 15/15] iio: adc: ad7768-1: add filter type and decimation rate attributes

[Marcelo Schmitt]

On 01/07, Jonathan Santos wrote:
> Separate filter type and decimation rate from the sampling frequency
> attribute. The new filter type attribute enables SINC3 and WIDEBAND
> filters, which were previously unavailable.
> 
> Previously, combining decimation and MCLK divider in the sampling
> frequency obscured performance trade-offs. Lower MCLK divider
> settings increase power usage, while lower decimation rates reduce
> precision by decreasing averaging. By creating a decimation attribute,
> users gain finer control over performance.

If not going to use -3dB ABI instead of decimation rate thing, maybe throw the
formula for the sampling frequency here (or near its use in the code) to help
make the relation between samp_freq, master clock divider, and decimation rate
clearer?
Is it samp_freq = mclk / (mclk_div * dec_rate) ?

> 
> The addition of those attributes allows a wider range of sampling
> frequencies and more access to the device features.
> 
> Co-developed-by: PopPaul2021 <paul.pop@analog.com>
> Signed-off-by: PopPaul2021 <paul.pop@analog.com>
> Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> ---
>  drivers/iio/adc/ad7768-1.c | 429 +++++++++++++++++++++++++++++++------
...
> +static const int dec_rate_values[6] = {
> +	32, 64, 128, 256, 512, 1024,
> +};
> +
> +static const  int sinc3_dec_rate_max_values[4] = {
> +	20480, 40960, 81920, 163840,
> +};
sinc3_dec_rate_max_values unused?

> +
> +static const char * const ad7768_filter_enum[] = {
> +	"sinc5", "sinc3", "wideband"
>  };
>  
...
> +static int ad7768_configure_dig_fil(struct iio_dev *dev,
> +				    enum ad7768_flt_type filter_type,
> +				    unsigned int dec_rate)
> +{
> +	struct ad7768_state *st = iio_priv(dev);
> +	int ret;
> +
> +	if (filter_type == SINC3) {
neat: switch (filter_type) { ?

> +		ret = ad7768_set_filter_type(dev, SINC3);
> +		if (ret)
> +			return ret;
> +
> +		/* recalculate the decimation for this filter mode */
> +		ret = ad7768_set_sinc3_dec_rate(st, dec_rate);
> +	} else if (filter_type == WIDEBAND) {
> +		ret = ad7768_set_filter_type(dev, filter_type);
> +		if (ret)
> +			return ret;
> +
> +		/* recalculate the decimation rate */
> +		ret = ad7768_set_dec_rate(st, dec_rate);
> +	} else {
> +		/* For SINC5 filter */
> +		/* Decimation 8 and 16 are set in the digital filter field */
> +		if (dec_rate <= 8) {
> +			ret = ad7768_set_filter_type(dev, SINC5_DEC_X8);
> +			if (ret)
> +				return ret;
> +
> +			st->dec_rate = 8;
> +		} else if (dec_rate <= 16) {
> +			ret = ad7768_set_filter_type(dev, SINC5_DEC_X16);
> +			if (ret)
> +				return ret;
> +
> +			st->dec_rate = 16;
> +		} else {
> +			ret = ad7768_set_filter_type(dev, SINC5);
> +			if (ret)
> +				return ret;
> +
> +			ret = ad7768_set_dec_rate(st, dec_rate);
> +		}
> +	}
> +
> +	/* Update scale table: scale values vary according to the precision */
> +	ad7768_fill_scale_tbl(dev);
> +
> +	return ret;
> +}
> +
...
> +static ssize_t decimation_rate_available_show(struct device *dev,
> +					      struct device_attribute *attr,
> +					      char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct ad7768_state *st = iio_priv(indio_dev);
> +	int len = 0;
> +
> +	/* Return decimation rate available in range format */
> +	buf[len++] = '[';
> +	if (st->filter_type == SINC3) {
switch (st->filter_type) { ?

> +		len += sysfs_emit_at(buf, len, "%d ", SINC3_DEC_RATE_MIN);
> +		len += sysfs_emit_at(buf, len, "%d ", SINC3_DEC_RATE_MIN);
> +		len += sysfs_emit_at(buf, len, "%d ", SINC3_DEC_RATE_MAX);
> +	} else if (st->filter_type == WIDEBAND) {
> +		len += sysfs_emit_at(buf, len, "%d ", WIDEBAND_DEC_RATE_MIN);
> +		len += sysfs_emit_at(buf, len, "%d ", WIDEBAND_DEC_RATE_MIN);
> +		len += sysfs_emit_at(buf, len, "%d ", WIDEBAND_DEC_RATE_MAX);
> +	} else {
> +		len += sysfs_emit_at(buf, len, "%d ", SINC5_DEC_RATE_MIN);
> +		len += sysfs_emit_at(buf, len, "%d ", SINC5_DEC_RATE_MIN);
> +		len += sysfs_emit_at(buf, len, "%d ", SINC5_DEC_RATE_MAX);
> +	}
> +
> +	buf[len - 1] = ']';
> +	buf[len++] = '\n';
> +
> +	return len;
> +}
> +

Re: [PATCH v1 11/15] iio: adc: ad7768-1: Add reg_write_masked function

[Marcelo Schmitt]

On 01/07, Jonathan Santos wrote:
> From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> 
> This commit adds the ad7768_spi_reg_write_masked() which is a helper
> function for writing specific bits inside a register, without interfering
> with the other bit values.
> 
> Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> ---
>  drivers/iio/adc/ad7768-1.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> index 574d735f2c3a..675af9ea856d 100644
> --- a/drivers/iio/adc/ad7768-1.c
> +++ b/drivers/iio/adc/ad7768-1.c
> @@ -242,6 +242,21 @@ static int ad7768_spi_reg_write(struct ad7768_state *st,
>  	return spi_write(st->spi, st->data.d8, 2);
>  }
>  
> +static int ad7768_spi_reg_write_masked(struct ad7768_state *st,
> +				       unsigned int addr,
> +				       unsigned int mask,
> +				       unsigned int val)
> +{
> +	unsigned int reg_val;
> +	int ret;
> +
> +	ret = ad7768_spi_reg_read(st, addr, &reg_val, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	return ad7768_spi_reg_write(st, addr, (reg_val & ~mask) | val);
> +}
> +
ad7768_spi_reg_write_masked() should be added together with at least one
user of it otherwise it leads to build warnings.
Though, better if able to convert to regmap as David suggested.
regmap_set_bits() and regmap_update_bits() come for free.

>  static int ad7768_set_mode(struct ad7768_state *st,
>  			   enum ad7768_conv_mode mode)
>  {
> -- 
> 2.34.1
> 

Re: [PATCH v1 00/15] iio: adc: ad7768-1: Add features, improvements, and fixes

[Jonathan Santos]

On 01/07, David Lechner wrote:
> On 1/7/25 9:23 AM, Jonathan Santos wrote:
> > This patch series introduces some new features, improvements,
> > and fixes for the AD7768-1 ADC driver. 
> > 
> > The goal is to support all key functionalities listed in the device
> > datasheet, including filter mode selection, common mode voltage 
> > configuration and GPIO support. Additionally, this includes fixes 
> > for SPI communication and for IIO interface, and also code improvements
> > to enhance maintainability and readability.
> 
> It looks like some of the patches also have to do with a sync trigger for one
> or more chips. There was a recent discussion about this for another ADC that
> seems relevant:
> 
> [1]: https://lore.kernel.org/linux-iio/20241128125811.11913-1-alisa.roman@analog.com/
> 
Interesting. That would cover the case where we control the synchronization with a GPIO,
but it gets more complex when using /SYNC_OUT in a multidevice setup, since it requires
sending a SPI command to the "main device" to toggle the pin.

The main device being the one which its /SYNC_OUT drives the others SYNC_IN.
> > 
> > Jonathan Santos (8):
> >   dt-bindings: iio: adc: ad7768-1: add synchronization over SPI property
> >   Documentation: ABI: add wideband filter type to  sysfs-bus-iio
> >   Documentation: ABI: testing: ad7768-1: Add device specific ABI
> >     documentation.
> >   iio: adc: ad7768-1: set MOSI idle state to high
> >   iio: adc: ad7768-1: use guard(mutex) to simplify code
> >   iio: adc: ad7768-1: add multiple scan types to support 16-bits mode
> >   iio: adc: ad7768-1: add support for Synchronization over SPI
> >   iio: adc: ad7768-1: add filter type and decimation rate attributes
> > 
> > Sergiu Cuciurean (7):
> >   iio: adc: ad7768-1: Fix conversion result sign
> >   iio: adc: ad7768-1: Update reg_read function
> >   iio: adc: ad7768-1: Add reset gpio
> >   iio: adc: ad7768-1: Move buffer allocation to a separate function
> >   iio: adc: ad7768-1: Add support for variable VCM
> >   iio: adc: ad7768-1: Add reg_write_masked function
> >   iio: adc: ad7768-1: Add GPIO controller support
> > 
> >  Documentation/ABI/testing/sysfs-bus-iio       |   2 +
> >  .../ABI/testing/sysfs-bus-iio-adc-ad7768-1    |  13 +
> >  .../bindings/iio/adc/adi,ad7768-1.yaml        |  24 +-
> >  drivers/iio/adc/ad7768-1.c                    | 830 +++++++++++++++---
> >  4 files changed, 747 insertions(+), 122 deletions(-)
> >  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-ad7768-1
> > 
> > 
> > base-commit: 5de07b8a24cf44cdb78adeab790704bf577c2c1d
> 
> What are all of these prerequisites?
> 
I might have done something wrong while generating the pacthes, i will check this.
> > prerequisite-patch-id: 8b531bca46f7c7ea1c0f6d232d162fd05fda52f7
> > prerequisite-patch-id: c8c8637cb3343097c3224a9aa315fc45dca15f45
> > prerequisite-patch-id: e0baac9ef84e6e8a0a272fc6191fc1cb48143e44
> > prerequisite-patch-id: 7e5cad70809fa7d37e917628147c197427c11594
> > prerequisite-patch-id: 28dcdb0ebc3ca3c02713f83c94a4eedbe81095dc
> > ...

Re: [PATCH v1 01/15] dt-bindings: iio: adc: ad7768-1: add synchronization over SPI property

[Jonathan Santos]

On 01/07, David Lechner wrote:
> On 1/7/25 9:24 AM, Jonathan Santos wrote:
> > Add adi,sync-in-spi property to enable synchronization over SPI.
> > This should be used in the case when the GPIO cannot provide a
> > pulse synchronous with the base MCLK signal.
> > 
> > User can choose between SPI, GPIO synchronization or neither of them,
> > but only if a external pulse can be provided, for example, by another
> > device in a multidevice setup.
> > 
> 
> While we are fixing up these bindings, we could add some more trivial things,
> like power supplies.
> 
> Also, the interrupt property could use a description since the chip has multiple
> output pins. I assume it means the /DRDY pin?
> 

Right! Yes, the interrupt pin refers to the /DRDY.

> > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> > ---
> >  .../bindings/iio/adc/adi,ad7768-1.yaml        | 24 ++++++++++++++++++-
> >  1 file changed, 23 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
> > index 3ce59d4d065f..55cec27bfe60 100644
> > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
> > @@ -47,6 +47,15 @@ properties:
> >        in any way, for example if the filter decimation rate changes.
> >        As the line is active low, it should be marked GPIO_ACTIVE_LOW.
> >  
> > +  adi,sync-in-spi:
> 
> If this is saying that SYNC_OUT is connected to SYNC_IN, then I think the name
> should be something like adi,sync-in-sync-out. SPI seems irrelevant here since
> we should just be describing how things are wired up, not how it is being used.
> 
> But if we also need to consider the case where SYNC_OUT of one chip is connected
> to SYNC_IN of another chip, we might want to consider using trigger-source
> bindings instead (recently standardized in dtschema).
> 

Do you mean the trigger-sources used for LEDs? I can try to see if it works, but would it
handle the non-GPIO case? While testing a multidevice setup, I found it simpler to 
have a single device to manage everything. It lets us toggle the GPIO or /SYNC_OUT
without referencing another device and makes simultaneous buffered reads easier.

Maybe we could stick to synchronization within the chip for now.

> > +    description:
> > +      Enables synchronization of multiple devices over SPI. This property is
> > +      used when a signal synchronous to the base MCLK signal cannot be provided
> > +      via GPIO. It requires the SYNC_OUT pin to be connected to the SYNC_IN pin
> > +      on the ADC. In the case of multiple devices, the SYNC_OUT pin of one device
> > +      should be routed to the SYNC_IN pins of the other devices.
> > +    type: boolean
> > +
> >    reset-gpios:
> >      maxItems: 1
> >  
> > @@ -65,7 +74,6 @@ required:
> >    - vref-supply
> >    - spi-cpol
> >    - spi-cpha
> > -  - adi,sync-in-gpios
> >  
> >  patternProperties:
> >    "^channel@([0-9]|1[0-5])$":
> > @@ -89,6 +97,20 @@ patternProperties:
> >  allOf:
> >    - $ref: /schemas/spi/spi-peripheral-props.yaml#
> >  
> > +  # adi,sync-in-gpios and adi,sync-in-spi are mutually exclusive (neither is also valid)
> > +  - if:
> > +      required:
> > +        - adi,sync-in-gpios
> > +    then:
> > +      properties:
> > +        adi,sync-in-spi: false
> > +  - if:
> > +      required:
> > +        - adi,sync-in-spi
> > +    then:
> > +      properties:
> > +        adi,sync-in-gpios: false
> 
> I think this can be simplified to using oneOf: to provide XOR validation
> 

Right!

> > +
> >  unevaluatedProperties: false
> >  
> >  examples:
> 

Re: [PATCH v1 02/15] Documentation: ABI: add wideband filter type to sysfs-bus-iio

[Jonathan Santos]

On 01/07, David Lechner wrote:
> On 1/7/25 9:24 AM, Jonathan Santos wrote:
> > The Wideband Low Ripple FIR filter is used for AD7768-1 Driver.
> > Document wideband filter option into filter_type_avaialable
> 
> s/avaialable/available/
> 
> > attribute.
> > 
> > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> > ---
> >  Documentation/ABI/testing/sysfs-bus-iio | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> > index f83bd6829285..c4c21a7bfba1 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-iio
> > +++ b/Documentation/ABI/testing/sysfs-bus-iio
> > @@ -2291,6 +2291,8 @@ Description:
> >  		* "sinc3+pf2" - Sinc3 + device specific Post Filter 2.
> >  		* "sinc3+pf3" - Sinc3 + device specific Post Filter 3.
> >  		* "sinc3+pf4" - Sinc3 + device specific Post Filter 4.
> > +		* "wideband" - FIR filter with wideband low ripple passband
> 
> I think "fir" would be a more specific filter type name than "wideband". (i.e.
> there are wikipedia pages for sinc and FIR filters, but not one for "wideband"
> filters)
> 

Isn't "fir" a bit too generic for this case? Since Wideband here is a class of a FIR filter.
Maybe something like "wideband-fir" or "fir-wideband" would work better?

> > +		  and sharp transition band.
> >  
> >  What:		/sys/.../events/in_proximity_thresh_either_runningperiod
> >  KernelVersion:	6.6
> 

Re: [PATCH v1 03/15] Documentation: ABI: testing: ad7768-1: Add device specific ABI documentation.

[Jonathan Santos]

On 01/07, David Lechner wrote:
> On 1/7/25 9:24 AM, Jonathan Santos wrote:
> > Add ABI documentation specific to the ad7768-1 device, detailing
> > the decimation_rate attribute for better clarity and usability.
> > 
> > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> > ---
> >  .../ABI/testing/sysfs-bus-iio-adc-ad7768-1          | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-ad7768-1
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7768-1 b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7768-1
> > new file mode 100644
> > index 000000000000..065247f07cfb
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7768-1
> > @@ -0,0 +1,13 @@
> > +What:		/sys/bus/iio/devices/iio:deviceX/decimation_rate_available
> > +KernelVersion:
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Reading returns a range of possible decimation rate values.
> > +
> > +What:		/sys/bus/iio/devices/iio:deviceX/decimation_rate
> > +KernelVersion:
> > +Contact:	linux-iio@vger.kernel.org
> > +Description:
> > +		Sets up the decimation rate for the digital filter. This can
> > +		directly impact in the final sampling frequency. Reading returns
> > +		the decimation rate. Writing sets the decimation rate.
> 
> If this only affects the filter, I would suggest to add `filter_` to the
> beginning of the attribute names.
> 
> Also, an explanation of how to interpret the numbers would be helpful. It looks
> like a unitless number that acts a sort of a multiplier or divider, but that
> part isn't so clear to me. 
> 
> Or...
> 
> Since the decimation rate affects the -3dB point of the filters we could use
> the standard IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY instead of introducing
> a new attribute.

Well, here the -3dB cutoff depends on the ODR, which is determined by both the MCLK
divider and decimation rate.

Wideband: -3dB at 0.433 × ODR
Sinc5: -3dB at 0.204 × ODR
Sinc3: -3dB at 0.2617 × ODR

If we use _filter_low_pass_3db_frequency to control the decimation and _sampling_frequency
to control the MCLK divider, wouldn’t it be confusing for one to always affect the other?
A different ODR would result in a different cutoff, and vice versa.

Would something like <type>[_name]_oversampling_ratio make more sense? Let me know what you think

Re: [PATCH v1 04/15] iio: adc: ad7768-1: Fix conversion result sign

[Jonathan Santos]

On 01/10, Marcelo Schmitt wrote:
> On 01/07, Jonathan Santos wrote:
> > From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> > 
> > The ad7768-1 is a fully differential ADC, meaning that the voltage
> > conversion result is a signed value. Since the value is a 24 bit one,
> Hmm, I think the reason why we sign _raw values should be because of the ADC
> output code format. There are differential ADCs that can measure a negative
> difference between IN+ and IN- but outputting straight binary data format (not
> signed values). In those cases, the _offset attribute is used to "shift" the
> _raw value so that output codes that represent IN+ < IN- are adjusted to a
> negative decimal value (the _raw + _offset part of IIO ABI to get to mV units).
> For AD7768-1/ADAQ7768-1, the ADC output code is indeed two's complement and thus
> signed so the code change is correct for it.
> Since you are probably going to re-spin on the patch series, will be nice
> to adjust the message to something like:
> The ad7768-1 ADC output code is two's complement, meaning that the voltage
> conversion result is a signed value. ...
> 
> With that,
> Reviewed-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
>

You are right, Thanks! will do that.

> > stored in a 32 bit variable, the sign should be extended in order to get
> > the correct representation.
> > 
> > Also the channel description has been updated to signed representation,
> > to match the ADC specifications.
> > 
> > Fixes: a5f8c7da3dbe ("iio: adc: Add AD7768-1 ADC basic support")
> > Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> > ---
> >  drivers/iio/adc/ad7768-1.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> > index 113703fb7245..c3cf04311c40 100644
> > --- a/drivers/iio/adc/ad7768-1.c
> > +++ b/drivers/iio/adc/ad7768-1.c
> > @@ -142,7 +142,7 @@ static const struct iio_chan_spec ad7768_channels[] = {
> >  		.channel = 0,
> >  		.scan_index = 0,
> >  		.scan_type = {
> > -			.sign = 'u',
> > +			.sign = 's',
> >  			.realbits = 24,
> >  			.storagebits = 32,
> >  			.shift = 8,
> > @@ -371,7 +371,7 @@ static int ad7768_read_raw(struct iio_dev *indio_dev,
> >  
> >  		ret = ad7768_scan_direct(indio_dev);
> >  		if (ret >= 0)
> > -			*val = ret;
> > +			*val = sign_extend32(ret, chan->scan_type.realbits - 1);
> >  
> >  		iio_device_release_direct_mode(indio_dev);
> >  		if (ret < 0)
> > -- 
> > 2.34.1
> > 

Re: [PATCH v1 08/15] iio: adc: ad7768-1: use guard(mutex) to simplify code

[Jonathan Santos]

On 01/07, David Lechner wrote:
> On 1/7/25 9:25 AM, Jonathan Santos wrote:
> > Use guard(mutex) from cleanup.h to remove most of the gotos and to make
> > the code simpler and less likely to fail due to forgetting to unlock
> > the resources.
> > 
> > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> > ---
> 
> ...
> 
> > @@ -484,7 +477,7 @@ static irqreturn_t ad7768_trigger_handler(int irq, void *p)
> >  	struct ad7768_state *st = iio_priv(indio_dev);
> >  	int ret;
> >  
> > -	mutex_lock(&st->lock);
> > +	guard(mutex)(&st->lock);
> >  
> >  	ret = spi_read(st->spi, &st->data.scan.chan, 3);
> >  	if (ret < 0)
> > @@ -495,7 +488,6 @@ static irqreturn_t ad7768_trigger_handler(int irq, void *p)
> >  
> >  err_unlock:
> 
> nit: also rename the label since it is no longer unlocking
> 
> >  	iio_trigger_notify_done(indio_dev->trig);
> > -	mutex_unlock(&st->lock);
> >  
> >  	return IRQ_HANDLED;
> >  }
> 
> I'm also wondering if we should just drop this lock. It is only protecting
> a triggered buffer SPI xfer and debugfs register access from happening at the
> same time.
> 
> Since we have to write a magic value to exit conversion mode, reading registers
> during a buffered read is going to cause problems anyway. So we could just
> remove the mutex lock and use iio_device_claim_direct_mode() instead in
> ad7768_reg_access().
>
Ok, that makes sense. I will change in the next version.

Re: [PATCH v1 10/15] iio: adc: ad7768-1: Add support for variable VCM

[Jonathan Santos]

On 01/07, David Lechner wrote:
> On 1/7/25 9:26 AM, Jonathan Santos wrote:
> > From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> > 
> > The VCM output voltage can be used as a common-mode voltage within the
> > amplifier preconditioning circuits external to the AD7768-1.
> > 
> > This change exports the VCM as an iio attribute and makes it available
> > for configuration.
> 
> We model common mode voltage inputs as a regulator consumer (e.g. vcm-supply in
> DT), so should we make this a regulator provider instead?
> 
> It could be used with "regulator-output" to be able to control it from sysfs
> or if the amplifier ends up in the devicetree for other reasons, the amplifier
> driver could control the regulator instead of requiring it to by done in sysfs.
>

Ok, that is an interesting approach, I will try this. It makes sense to
have it in the devicetree. 

Re: [PATCH v1 11/15] iio: adc: ad7768-1: Add reg_write_masked function

[Jonathan Santos]

On 01/10, Marcelo Schmitt wrote:
> On 01/07, Jonathan Santos wrote:
> > From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> > 
> > This commit adds the ad7768_spi_reg_write_masked() which is a helper
> > function for writing specific bits inside a register, without interfering
> > with the other bit values.
> > 
> > Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> > ---
> >  drivers/iio/adc/ad7768-1.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> > index 574d735f2c3a..675af9ea856d 100644
> > --- a/drivers/iio/adc/ad7768-1.c
> > +++ b/drivers/iio/adc/ad7768-1.c
> > @@ -242,6 +242,21 @@ static int ad7768_spi_reg_write(struct ad7768_state *st,
> >  	return spi_write(st->spi, st->data.d8, 2);
> >  }
> >  
> > +static int ad7768_spi_reg_write_masked(struct ad7768_state *st,
> > +				       unsigned int addr,
> > +				       unsigned int mask,
> > +				       unsigned int val)
> > +{
> > +	unsigned int reg_val;
> > +	int ret;
> > +
> > +	ret = ad7768_spi_reg_read(st, addr, &reg_val, 1);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return ad7768_spi_reg_write(st, addr, (reg_val & ~mask) | val);
> > +}
> > +
> ad7768_spi_reg_write_masked() should be added together with at least one
> user of it otherwise it leads to build warnings.
> Though, better if able to convert to regmap as David suggested.
> regmap_set_bits() and regmap_update_bits() come for free.
> 

I also think David suggestion is better, i will convert to regmap. Will
make our lives easier later as well.

> >  static int ad7768_set_mode(struct ad7768_state *st,
> >  			   enum ad7768_conv_mode mode)
> >  {
> > -- 
> > 2.34.1
> > 

Re: [PATCH v1 13/15] iio: adc: ad7768-1: add multiple scan types to support 16-bits mode

[Jonathan Santos]

On 01/07, David Lechner wrote:
> On 1/7/25 9:26 AM, Jonathan Santos wrote:
> > When the device is configured to Sinc5 filter and decimation x8,
> > output data is reduced to 16-bits in order to support 1 MHz of
> > sampling frequency due to clock limitation.
> 
> We aren't going to get a 1 MHz sample rate without SPI offload support so maybe
> we should save this patch until then?
> 
> In this patch, we are still reading 24-bits per sample, so we aren't really
> getting any benefit. It is probably fine for now to leave it as 24-bit even if
> the last 8 bits are all 0 or just noise.

Indeed we cannot achieve 1 MHz yet, but I believe it is good have this
now so it is more mature for the time SPI offload is supported. Also, will
allow us to backport this patch to other repos.

> 
> Also, the datasheet says:
> 
> 	this path allows viewing of wider bandwidth; however, it is quantization
> 	noise limited so that output data is reduced to 16 bits
> 
> So this doesn't actually seem related to higher sample rates. There is a CONVLEN
> bit in the INTERFACE_FORMAT register that globally reduces the output size to
> 16-bit, which I suspect would be what we will need for achieving the highest
> sample rate when we add SPI offload support.
> 

Right, that is true, but the reason we did this patch was to fix the
output size when we configure the filter to sinc5 decx8. The datasheet
says:

	To configure the sinc5 filter for 1.024 MSPS output data rate,
	write 001 to the FILTER bits [6:4] of the DIGITAL_FILTER register
	(Register 0x19). The ADAQ7768-1 automatically changes the decimation
	rate to 8 and output data length is reduced to 16 bits from 24 bits 
	due to the maximum speed limitation of the digital serial interface.

In this case we don't even need to change the value of CONVLEN

> > 
> > Use multiple scan types feature to enable the driver to switch
> > scan type in runtime, making possible to support both 24-bit and
> > 16-bit resolution.
> > 
> > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> > ---
> >  drivers/iio/adc/ad7768-1.c | 65 ++++++++++++++++++++++++++++++++------
> >  1 file changed, 56 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> > index 9741a6d47942..5e4e7d387f9a 100644
> > --- a/drivers/iio/adc/ad7768-1.c
> > +++ b/drivers/iio/adc/ad7768-1.c
> > @@ -134,6 +134,11 @@ struct ad7768_clk_configuration {
> >  	enum ad7768_pwrmode pwrmode;
> >  };
> >  
> > +enum ad7768_scan_type {
> > +	AD7768_SCAN_TYPE_NORMAL,
> > +	AD7768_SCAN_TYPE_HIGH_SPEED,
> > +};
> > +
> >  static const char * const ad7768_vcm_modes[] = {
> >  	"(AVDD1-AVSS)/2",
> >  	"2V5",
> > @@ -145,6 +150,10 @@ static const char * const ad7768_vcm_modes[] = {
> >  	"OFF",
> >  };
> >  
> > +static const int ad7768_mclk_div_rates[4] = {
> > +	16, 8, 4, 2,
> > +};
> > +
> >  static const struct ad7768_clk_configuration ad7768_clk_config[] = {
> >  	{ AD7768_MCLK_DIV_2, AD7768_DEC_RATE_8, 16,  AD7768_FAST_MODE },
> >  	{ AD7768_MCLK_DIV_2, AD7768_DEC_RATE_16, 32,  AD7768_FAST_MODE },
> > @@ -159,6 +168,21 @@ static const struct ad7768_clk_configuration ad7768_clk_config[] = {
> >  	{ AD7768_MCLK_DIV_16, AD7768_DEC_RATE_1024, 16384, AD7768_ECO_MODE },
> >  };
> >  
> > +static const struct iio_scan_type ad7768_scan_type[] = {
> > +	[AD7768_SCAN_TYPE_NORMAL] = {
> > +		.sign = 's',
> > +		.realbits = 24,
> > +		.storagebits = 32,
> 
> What happened to .shift = 8, ? If there is a reason for removing it, please add
> that to the commit description.
> 

Sorry, will fix this

> > +		.endianness = IIO_BE,
> > +	},
> > +	[AD7768_SCAN_TYPE_HIGH_SPEED] = {
> > +		.sign = 's',
> > +		.realbits = 16,
> > +		.storagebits = 32,
> 
> I guess it doesn't matter much since we are reading one sample at a time, but
> I would expect storagebits to be 16 instead of 32. Or if it really needs to be
> 32, does it need shift = 16?
> 

This is because the hw is configured to return the samples in a 32 bits
format, so if storage is 16 we will get wrong data.

> > +		.endianness = IIO_BE,
> > +	},
> > +};
> > +
> >  static int ad7768_get_vcm(struct iio_dev *dev, const struct iio_chan_spec *chan);
> >  static int ad7768_set_vcm(struct iio_dev *dev, const struct iio_chan_spec *chan,
> >  			  unsigned int mode);
> 
> ...
> 
> > @@ -308,6 +329,15 @@ static int ad7768_scan_direct(struct iio_dev *indio_dev)
> >  	ret = ad7768_spi_reg_read(st, AD7768_REG_ADC_DATA, &readval, 3);
> >  	if (ret < 0)
> >  		return ret;
> > +
> > +	/*
> > +	 * When the decimation rate is set to x8, the ADC data precision is reduced
> > +	 * from 24 bits to 16 bits. Since the AD7768_REG_ADC_DATA register provides
> > +	 * 24-bit data, the precision is reduced by right-shifting the read value
> > +	 * by 8 bits.
> > +	 */
> > +	if (st->dec_rate == 8)
> > +		readval = readval >> 8;
> 
> Why not change size of ad7768_spi_reg_read() instead of reading 3 bytes and
> throwing one away?
> 

Right, i will check this and fix in the next version

> >  	/*
> >  	 * Any SPI configuration of the AD7768-1 can only be
> >  	 * performed in continuous conversion mode.

Re: [PATCH v1 01/15] dt-bindings: iio: adc: ad7768-1: add synchronization over SPI property

[Jonathan Cameron]

On Fri, 10 Jan 2025 18:51:06 -0300
Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote:

> On 01/07, Jonathan Santos wrote:
> > Add adi,sync-in-spi property to enable synchronization over SPI.
> > This should be used in the case when the GPIO cannot provide a
> > pulse synchronous with the base MCLK signal.
> > 
> > User can choose between SPI, GPIO synchronization or neither of them,
> > but only if a external pulse can be provided, for example, by another
> > device in a multidevice setup.
> > 
> > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> > ---
> >  .../bindings/iio/adc/adi,ad7768-1.yaml        | 24 ++++++++++++++++++-
> >  1 file changed, 23 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
> > index 3ce59d4d065f..55cec27bfe60 100644
> > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
> > @@ -47,6 +47,15 @@ properties:
> >        in any way, for example if the filter decimation rate changes.
> >        As the line is active low, it should be marked GPIO_ACTIVE_LOW.
> >  
> > +  adi,sync-in-spi:
> > +    description:
> > +      Enables synchronization of multiple devices over SPI. This property is
> > +      used when a signal synchronous to the base MCLK signal cannot be provided
> > +      via GPIO. It requires the SYNC_OUT pin to be connected to the SYNC_IN pin
> > +      on the ADC. In the case of multiple devices, the SYNC_OUT pin of one device
> > +      should be routed to the SYNC_IN pins of the other devices.  
> So, if I'm getting it right,

Datasheet on this is indeed complex!

>/SYNC_IN may be driven by a GPIO (ADAQ7768-1 datasheet Figure 131),

For that we expose a gpio binding already. If that's present we know what is going on.

>/SYNC_IN may be driven by own device /SYNC_OUT (ADAQ7768-1 datasheet Figure 133),

This is the default - no information provided so it isn't wired externally.
We don't normally bother to describe required chip to chip connections.
I couldn't entirely figure out if this is 'required' if we aren't driving explicitly
from GPIO or another chip but i think it is(?).

>/SYNC_IN may be driven by other AD7768-1 > /SYNC_OUT pin (also Figure 133).
This is only case we are about for sync in I think.

As long as there isn't a valid 'not connected' It think we are fine with a boolean.

> That is too much to describe with a boolean.
> 
> If David's suggestion of using a trigger-source doesn't fit, this property
> should at least become an enum or string.
> 
> > +    type: boolean
> > +
> >    reset-gpios:
> >      maxItems: 1
> >  
> > @@ -65,7 +74,6 @@ required:
> >    - vref-supply
> >    - spi-cpol
> >    - spi-cpha
> > -  - adi,sync-in-gpios
> >  
> >  patternProperties:
> >    "^channel@([0-9]|1[0-5])$":
> > @@ -89,6 +97,20 @@ patternProperties:
> >  allOf:
> >    - $ref: /schemas/spi/spi-peripheral-props.yaml#
> >  
> > +  # adi,sync-in-gpios and adi,sync-in-spi are mutually exclusive (neither is also valid)
> > +  - if:
> > +      required:
> > +        - adi,sync-in-gpios
> > +    then:
> > +      properties:
> > +        adi,sync-in-spi: false
> > +  - if:
> > +      required:
> > +        - adi,sync-in-spi
> > +    then:
> > +      properties:
> > +        adi,sync-in-gpios: false
> > +
> >  unevaluatedProperties: false
> >  
> >  examples:
> > -- 
> > 2.34.1
> >   

Re: [PATCH v1 01/15] dt-bindings: iio: adc: ad7768-1: add synchronization over SPI property

[Jonathan Cameron]

On Sat, 11 Jan 2025 19:34:14 -0300
Jonathan Santos <jonath4nns@gmail.com> wrote:

> On 01/07, David Lechner wrote:
> > On 1/7/25 9:24 AM, Jonathan Santos wrote:  
> > > Add adi,sync-in-spi property to enable synchronization over SPI.
> > > This should be used in the case when the GPIO cannot provide a
> > > pulse synchronous with the base MCLK signal.
> > > 
> > > User can choose between SPI, GPIO synchronization or neither of them,
> > > but only if a external pulse can be provided, for example, by another
> > > device in a multidevice setup.
> > >   
> > 
> > While we are fixing up these bindings, we could add some more trivial things,
> > like power supplies.
> > 
> > Also, the interrupt property could use a description since the chip has multiple
> > output pins. I assume it means the /DRDY pin?
> >   
> 
> Right! Yes, the interrupt pin refers to the /DRDY.
> 
> > > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> > > ---
> > >  .../bindings/iio/adc/adi,ad7768-1.yaml        | 24 ++++++++++++++++++-
> > >  1 file changed, 23 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
> > > index 3ce59d4d065f..55cec27bfe60 100644
> > > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
> > > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
> > > @@ -47,6 +47,15 @@ properties:
> > >        in any way, for example if the filter decimation rate changes.
> > >        As the line is active low, it should be marked GPIO_ACTIVE_LOW.
> > >  
> > > +  adi,sync-in-spi:  
> > 
> > If this is saying that SYNC_OUT is connected to SYNC_IN, then I think the name
> > should be something like adi,sync-in-sync-out. SPI seems irrelevant here since
> > we should just be describing how things are wired up, not how it is being used.
> > 
> > But if we also need to consider the case where SYNC_OUT of one chip is connected
> > to SYNC_IN of another chip, we might want to consider using trigger-source
> > bindings instead (recently standardized in dtschema).
> >   
> 
> Do you mean the trigger-sources used for LEDs? I can try to see if it works, but would it
> handle the non-GPIO case? While testing a multidevice setup, I found it simpler to 
> have a single device to manage everything. It lets us toggle the GPIO or /SYNC_OUT
> without referencing another device and makes simultaneous buffered reads easier.

Daisy-chain mode (figure 131)?  In that case we normally end up with a single presented device
with a 'lot' of channels. (See the electric car style battery charging chips, those can
be chained in very large numbers!)

Probably similar for figure 133 (which is a dual SPI setup) as the SPI clock must
be shared so we still see it over a single interface.

If those are the only two options then keeping this within the driver is fine.
For daisy chain there are examples in tree and it normally means we have to
have a DT parameter that says how long the chain is, though we maybe can
do that with per channel nodes as well if those make sense here.

Jonathan


> 
> Maybe we could stick to synchronization within the chip for now.
> 
> > > +    description:
> > > +      Enables synchronization of multiple devices over SPI. This property is
> > > +      used when a signal synchronous to the base MCLK signal cannot be provided
> > > +      via GPIO. It requires the SYNC_OUT pin to be connected to the SYNC_IN pin
> > > +      on the ADC. In the case of multiple devices, the SYNC_OUT pin of one device
> > > +      should be routed to the SYNC_IN pins of the other devices.
> > > +    type: boolean
> > > +
> > >    reset-gpios:
> > >      maxItems: 1
> > >  
> > > @@ -65,7 +74,6 @@ required:
> > >    - vref-supply
> > >    - spi-cpol
> > >    - spi-cpha
> > > -  - adi,sync-in-gpios
> > >  
> > >  patternProperties:
> > >    "^channel@([0-9]|1[0-5])$":
> > > @@ -89,6 +97,20 @@ patternProperties:
> > >  allOf:
> > >    - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > >  
> > > +  # adi,sync-in-gpios and adi,sync-in-spi are mutually exclusive (neither is also valid)
> > > +  - if:
> > > +      required:
> > > +        - adi,sync-in-gpios
> > > +    then:
> > > +      properties:
> > > +        adi,sync-in-spi: false
> > > +  - if:
> > > +      required:
> > > +        - adi,sync-in-spi
> > > +    then:
> > > +      properties:
> > > +        adi,sync-in-gpios: false  
> > 
> > I think this can be simplified to using oneOf: to provide XOR validation
> >   
> 
> Right!
> 
> > > +
> > >  unevaluatedProperties: false
> > >  
> > >  examples:  
> >   

Re: [PATCH v1 02/15] Documentation: ABI: add wideband filter type to sysfs-bus-iio

[Jonathan Cameron]

On Sat, 11 Jan 2025 19:50:18 -0300
Jonathan Santos <jonath4nns@gmail.com> wrote:

> On 01/07, David Lechner wrote:
> > On 1/7/25 9:24 AM, Jonathan Santos wrote:  
> > > The Wideband Low Ripple FIR filter is used for AD7768-1 Driver.
> > > Document wideband filter option into filter_type_avaialable  
> > 
> > s/avaialable/available/
> >   
> > > attribute.
> > > 
> > > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> > > ---
> > >  Documentation/ABI/testing/sysfs-bus-iio | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> > > index f83bd6829285..c4c21a7bfba1 100644
> > > --- a/Documentation/ABI/testing/sysfs-bus-iio
> > > +++ b/Documentation/ABI/testing/sysfs-bus-iio
> > > @@ -2291,6 +2291,8 @@ Description:
> > >  		* "sinc3+pf2" - Sinc3 + device specific Post Filter 2.
> > >  		* "sinc3+pf3" - Sinc3 + device specific Post Filter 3.
> > >  		* "sinc3+pf4" - Sinc3 + device specific Post Filter 4.
> > > +		* "wideband" - FIR filter with wideband low ripple passband  
> > 
> > I think "fir" would be a more specific filter type name than "wideband". (i.e.
> > there are wikipedia pages for sinc and FIR filters, but not one for "wideband"
> > filters)
> >   
> 
> Isn't "fir" a bit too generic for this case? Since Wideband here is a class of a FIR filter.
> Maybe something like "wideband-fir" or "fir-wideband" would work better?


Not sure FIR is even useful. That's just a particular filter architecture, not
related directly to the characteristics userspace cares about.
You can sometimes at least build a very similar response from an IIR filter.
The sinc ones describe the pattern they let through, FIR isn't that specific.
So I'd not mention FIR anywhere.

> 
> > > +		  and sharp transition band.
> > >  
> > >  What:		/sys/.../events/in_proximity_thresh_either_runningperiod
> > >  KernelVersion:	6.6  
> >   

Re: [PATCH v1 03/15] Documentation: ABI: testing: ad7768-1: Add device specific ABI documentation.

[Jonathan Cameron]

On Sat, 11 Jan 2025 20:22:36 -0300
Jonathan Santos <jonath4nns@gmail.com> wrote:

> On 01/07, David Lechner wrote:
> > On 1/7/25 9:24 AM, Jonathan Santos wrote:  
> > > Add ABI documentation specific to the ad7768-1 device, detailing
> > > the decimation_rate attribute for better clarity and usability.
> > > 
> > > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> > > ---
> > >  .../ABI/testing/sysfs-bus-iio-adc-ad7768-1          | 13 +++++++++++++
> > >  1 file changed, 13 insertions(+)
> > >  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-ad7768-1
> > > 
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7768-1 b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7768-1
> > > new file mode 100644
> > > index 000000000000..065247f07cfb
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7768-1
> > > @@ -0,0 +1,13 @@
> > > +What:		/sys/bus/iio/devices/iio:deviceX/decimation_rate_available
> > > +KernelVersion:
> > > +Contact:	linux-iio@vger.kernel.org
> > > +Description:
> > > +		Reading returns a range of possible decimation rate values.
> > > +
> > > +What:		/sys/bus/iio/devices/iio:deviceX/decimation_rate
> > > +KernelVersion:
> > > +Contact:	linux-iio@vger.kernel.org
> > > +Description:
> > > +		Sets up the decimation rate for the digital filter. This can
> > > +		directly impact in the final sampling frequency. Reading returns
> > > +		the decimation rate. Writing sets the decimation rate.  
> > 
> > If this only affects the filter, I would suggest to add `filter_` to the
> > beginning of the attribute names.
> > 
> > Also, an explanation of how to interpret the numbers would be helpful. It looks
> > like a unitless number that acts a sort of a multiplier or divider, but that
> > part isn't so clear to me. 
> > 
> > Or...
> > 
> > Since the decimation rate affects the -3dB point of the filters we could use
> > the standard IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY instead of introducing
> > a new attribute.  
> 
> Well, here the -3dB cutoff depends on the ODR, which is determined by both the MCLK
> divider and decimation rate.
> 
> Wideband: -3dB at 0.433 × ODR
> Sinc5: -3dB at 0.204 × ODR
> Sinc3: -3dB at 0.2617 × ODR
> 
> If we use _filter_low_pass_3db_frequency to control the decimation and _sampling_frequency
> to control the MCLK divider, wouldn’t it be confusing for one to always affect the other?
> A different ODR would result in a different cutoff, and vice versa.

We should definitely not have a filter control changing sampling frequency (which tends to
be a more common control for users to fiddle with).  However the other way around is
fine.  So for a given _sampling_frequency present via
in_xx_filter_low_pass_3db_frequency_available the list of
possible 3db frequencies and use them to configure the decimation.

> 
> Would something like <type>[_name]_oversampling_ratio make more sense? Let me know what you think

I'd rather not if we can avoid that new ABI, but it is is better than a new term
like decimation_rate.

Jonathan

> 

Re: [PATCH v1 04/15] iio: adc: ad7768-1: Fix conversion result sign

[Jonathan Cameron]

On Tue, 7 Jan 2025 12:25:07 -0300
Jonathan Santos <Jonathan.Santos@analog.com> wrote:

> From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> 
> The ad7768-1 is a fully differential ADC, meaning that the voltage
> conversion result is a signed value. Since the value is a 24 bit one,
> stored in a 32 bit variable, the sign should be extended in order to get
> the correct representation.
> 
> Also the channel description has been updated to signed representation,
> to match the ADC specifications.
> 
> Fixes: a5f8c7da3dbe ("iio: adc: Add AD7768-1 ADC basic support")
> Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
Looks good, but for v2, please pull fixes to the start of the patch set.

Whilst it doesn't matter here as the earlier patches are all docs,
it is still good practice as anyone looking for fixes tends to look 
there.

Jonathan

> ---
>  drivers/iio/adc/ad7768-1.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> index 113703fb7245..c3cf04311c40 100644
> --- a/drivers/iio/adc/ad7768-1.c
> +++ b/drivers/iio/adc/ad7768-1.c
> @@ -142,7 +142,7 @@ static const struct iio_chan_spec ad7768_channels[] = {
>  		.channel = 0,
>  		.scan_index = 0,
>  		.scan_type = {
> -			.sign = 'u',
> +			.sign = 's',
>  			.realbits = 24,
>  			.storagebits = 32,
>  			.shift = 8,
> @@ -371,7 +371,7 @@ static int ad7768_read_raw(struct iio_dev *indio_dev,
>  
>  		ret = ad7768_scan_direct(indio_dev);
>  		if (ret >= 0)
> -			*val = ret;
> +			*val = sign_extend32(ret, chan->scan_type.realbits - 1);
>  
>  		iio_device_release_direct_mode(indio_dev);
>  		if (ret < 0)

Re: [PATCH v1 05/15] iio: adc: ad7768-1: set MOSI idle state to high

[Jonathan Cameron]

On Fri, 10 Jan 2025 18:56:26 -0300
Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote:

> On 01/07, David Lechner wrote:
> > On 1/7/25 9:25 AM, Jonathan Santos wrote:  
> > > All supported parts require that the MOSI line stays high
> > > while in idle.
> > > 
> > > Configure SPI controller to set MOSI idle state to high.
> > > 
> > > Fixes: a5f8c7da3dbe ("iio: adc: Add AD7768-1 ADC basic support")
> > > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> > > ---  
> ...
> > > @@ -574,6 +574,15 @@ static int ad7768_probe(struct spi_device *spi)
> > >  		return -ENOMEM;
> > >  
> > >  	st = iio_priv(indio_dev);
> > > +	/*
> > > +	 * The ADC SDI line must be kept high when
> > > +	 * data is not being clocked out of the controller.
> > > +	 * Request the SPI controller to make MOSI idle high.
> > > +	 */
> > > +	spi->mode |= SPI_MOSI_IDLE_HIGH;
> > > +	ret = spi_setup(spi);
> > > +	if (ret < 0)
> > > +		return ret;
> > >  	st->spi = spi;
> > >  
> > >  	st->vref = devm_regulator_get(&spi->dev, "vref");  
> > 
> > Very few SPI controllers currently have the SPI_MOSI_IDLE_HIGH capability flag
> > set in Linux right now (whether they actually support it or not), so this could
> > break existing users.  
> 
> Good point. Maybe only dev_warn() if SPI_MOSI_IDLE_HIGH is not supported?
> 
> >   
> ...
> > 
> > If we ever do implement a data read of more than 64 bits without toggling CS,
> > then we could just set the TX data to be all 0xFF and have the same effect
> > without requiring the SPI controller to support SPI_MOSI_IDLE_HIGH.  
> 
> One point of having SPI_MOSI_IDLE_HIGH is that the controller may bring MOSI low
> between data words of a transfer. I think all transfer words are going to be
> either 16 or 24 with the new patches setting bits_per_word in all transfers but
> that might still not be enough if eventually the controller is unable to support
> those word sizes. 

Can we make the use of SPI_MOSI_IDLE_HIGH only apply if controller doesn't support
what is required to do the transfers in one go?

> Plus you would have the complication of filling the tx_buf for
> all transfers.

Wrap that up in a regmap, or read and write functions and that should be easy enough.

> 
> For the part that instigated the development of SPI_MOSI_IDLE_HIGH, the MOSI line
> also had to be high in between transfers. The diagrams at AD7768-1 datasheet
> page 51 suggest the same would be needed for this chip too.

Whilst the datasheet indeed draws lines for that, i doubt it notices except on
clock transitions and between transfers the clock won't do anything.
If we confirm that the device does notice, then I don't mind limiting the controllers
to those with that can ensure it doesn't get set wrong.

Jonathan

Re: [PATCH v1 07/15] iio: adc: ad7768-1: Add reset gpio

[Jonathan Cameron]

On Tue, 7 Jan 2025 17:40:53 -0600
David Lechner <dlechner@baylibre.com> wrote:

> On 1/7/25 9:25 AM, Jonathan Santos wrote:
> > From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> > 
> > Depending on the controller, the default state of a gpio can vary. This
> > change excludes the probability that the dafult state of the ADC reset
> > gpio will be HIGH if it will be passed as reference in the deivcetree.

devicetree

> > 
> > Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> > ---
> >  drivers/iio/adc/ad7768-1.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> > index 881446462ff5..f73b9aec8b0f 100644
> > --- a/drivers/iio/adc/ad7768-1.c
> > +++ b/drivers/iio/adc/ad7768-1.c
> > @@ -161,6 +161,7 @@ struct ad7768_state {
> >  	struct completion completion;
> >  	struct iio_trigger *trig;
> >  	struct gpio_desc *gpio_sync_in;
> > +	struct gpio_desc *gpio_reset;
> >  	const char *labels[ARRAY_SIZE(ad7768_channels)];
> >  	/*
> >  	 * DMA (thus cache coherency maintenance) may require the
> > @@ -441,6 +442,18 @@ static int ad7768_setup(struct ad7768_state *st)
> >  {
> >  	int ret;
> >  
> > +	st->gpio_reset = devm_gpiod_get_optional(&st->spi->dev, "reset",
> > +						 GPIOD_OUT_LOW);  
> 
> Could be simplified by setting this to GPIOD_OUT_HIGH and drop
> 
> 	gpiod_direction_output(st->gpio_reset, 1);
> 
> > +	if (IS_ERR(st->gpio_reset))
> > +		return PTR_ERR(st->gpio_reset);
> > +
> > +	if (st->gpio_reset) {
> > +		gpiod_direction_output(st->gpio_reset, 1);

It's an output already, gpiod_set_value() should work I think?
Possibly even then sleep allowing variants


> > +		usleep_range(10, 15);
> > +		gpiod_direction_output(st->gpio_reset, 0);
> > +		usleep_range(10, 15);  
> 
> prefer fsleep()
> 
> > +	}
> > +  
> 
> We can move the code below in an else since we don't need to do 2 resets.
> 
> >  	/*
> >  	 * Two writes to the SPI_RESET[1:0] bits are required to initiate
> >  	 * a software reset. The bits must first be set to 11, and then  
> 

Re: [PATCH v1 09/15] iio: adc: ad7768-1: Move buffer allocation to a separate function

[Jonathan Cameron]

On Fri, 10 Jan 2025 19:01:35 -0300
Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote:

> On 01/07, Jonathan Santos wrote:
> > From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> > 
> > This change moves the buffer allocation in a separate function, making
> > space for adding another type of iio buffer if needed.
> > 
> > Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> > ---
> >  drivers/iio/adc/ad7768-1.c | 64 ++++++++++++++++++++++----------------
> >  1 file changed, 37 insertions(+), 27 deletions(-)
> >   
> ...
> > @@ -625,37 +661,11 @@ static int ad7768_probe(struct spi_device *spi)
> >  		return ret;
> >  	}
> >  
> > -	st->trig = devm_iio_trigger_alloc(&spi->dev, "%s-dev%d",
> > -					  indio_dev->name,
> > -					  iio_device_id(indio_dev));
> > -	if (!st->trig)
> > -		return -ENOMEM;
> > -
> > -	st->trig->ops = &ad7768_trigger_ops;
> > -	iio_trigger_set_drvdata(st->trig, indio_dev);
> > -	ret = devm_iio_trigger_register(&spi->dev, st->trig);
> > -	if (ret)
> > -		return ret;
> > -
> > -	indio_dev->trig = iio_trigger_get(st->trig);
> > -
> > -	init_completion(&st->completion);  
> 
> Isn't the completion also used for single-shot reads?

If so, it should be left in the caller given that. Makes not functional
difference but prevents implication that it is only used for triggered buffer
cases.

> Well, if triggered_buffer_setup fails the whole probe fails and we never get to
> do a single-shot read, but that makes me wonder ... why don't we ever try to
> recover from iio_triggered_buffer_setup()? Should we ever do so?

Nope :)  For it to fail, something is going horribly wrong. Most likely
a bug, so I'm not seeing any reason to carry on.

> 
> > -
> >  	ret = ad7768_set_channel_label(indio_dev, ARRAY_SIZE(ad7768_channels));
> >  	if (ret)
> >  		return ret;
> >  
> > -	ret = devm_request_irq(&spi->dev, spi->irq,
> > -			       &ad7768_interrupt,
> > -			       IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> > -			       indio_dev->name, indio_dev);
> > -	if (ret)
> > -		return ret;
> > -
> > -	ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
> > -					      &iio_pollfunc_store_time,
> > -					      &ad7768_trigger_handler,
> > -					      &ad7768_buffer_ops);
> > +	ret = ad7768_triggered_buffer_alloc(indio_dev);
> >  	if (ret)
> >  		return ret;
> >  
> > -- 
> > 2.34.1
> >   

Re: [PATCH v1 09/15] iio: adc: ad7768-1: Move buffer allocation to a separate function

[Jonathan Cameron]

On Tue, 7 Jan 2025 12:26:05 -0300
Jonathan Santos <Jonathan.Santos@analog.com> wrote:

> From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> 
> This change moves the buffer allocation in a separate function, making
> space for adding another type of iio buffer if needed.
> 
> Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> ---
>  drivers/iio/adc/ad7768-1.c | 64 ++++++++++++++++++++++----------------
>  1 file changed, 37 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> index cd1b08053105..eaa9a12737ac 100644
> --- a/drivers/iio/adc/ad7768-1.c
> +++ b/drivers/iio/adc/ad7768-1.c
> @@ -163,6 +163,7 @@ struct ad7768_state {
>  	struct iio_trigger *trig;
>  	struct gpio_desc *gpio_sync_in;
>  	struct gpio_desc *gpio_reset;
> +	int irq;
>  	const char *labels[ARRAY_SIZE(ad7768_channels)];
>  	/*
>  	 * DMA (thus cache coherency maintenance) may require the
> @@ -569,6 +570,40 @@ static int ad7768_set_channel_label(struct iio_dev *indio_dev,
>  	return 0;
>  }
>  
> +static int ad7768_triggered_buffer_alloc(struct iio_dev *indio_dev)
> +{
> +	struct ad7768_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	st->trig = devm_iio_trigger_alloc(indio_dev->dev.parent, "%s-dev%d",
> +					  indio_dev->name,
> +					  iio_device_id(indio_dev));
> +	if (!st->trig)
> +		return -ENOMEM;
> +
> +	st->trig->ops = &ad7768_trigger_ops;
> +	iio_trigger_set_drvdata(st->trig, indio_dev);
> +	ret = devm_iio_trigger_register(indio_dev->dev.parent, st->trig);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->trig = iio_trigger_get(st->trig);
> +
> +	init_completion(&st->completion);
> +
> +	ret = devm_request_irq(indio_dev->dev.parent, st->irq,
> +			       &ad7768_interrupt,
> +			       IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> +			       indio_dev->name, indio_dev);
> +	if (ret)

As with the completion, the interrupt is not just for triggered buffer use
either, so probably doesn't belong in this function.  Again, not
a bug to do so, but hurts readability a little.

> +		return ret;
> +
> +	return devm_iio_triggered_buffer_setup(indio_dev->dev.parent, indio_dev,
> +					       &iio_pollfunc_store_time,
> +					       &ad7768_trigger_handler,
> +					       &ad7768_buffer_ops);
> +}
> +
>  static int ad7768_probe(struct spi_device *spi)
>  {
>  	struct ad7768_state *st;
> @@ -610,6 +645,7 @@ static int ad7768_probe(struct spi_device *spi)
>  		return PTR_ERR(st->mclk);
>  
>  	st->mclk_freq = clk_get_rate(st->mclk);
> +	st->irq = spi->irq;
>  
>  	mutex_init(&st->lock);
>  
> @@ -625,37 +661,11 @@ static int ad7768_probe(struct spi_device *spi)
>  		return ret;
>  	}
>  
> -	st->trig = devm_iio_trigger_alloc(&spi->dev, "%s-dev%d",
> -					  indio_dev->name,
> -					  iio_device_id(indio_dev));
> -	if (!st->trig)
> -		return -ENOMEM;
> -
> -	st->trig->ops = &ad7768_trigger_ops;
> -	iio_trigger_set_drvdata(st->trig, indio_dev);
> -	ret = devm_iio_trigger_register(&spi->dev, st->trig);
> -	if (ret)
> -		return ret;
> -
> -	indio_dev->trig = iio_trigger_get(st->trig);
> -
> -	init_completion(&st->completion);
> -
>  	ret = ad7768_set_channel_label(indio_dev, ARRAY_SIZE(ad7768_channels));
>  	if (ret)
>  		return ret;
>  
> -	ret = devm_request_irq(&spi->dev, spi->irq,
> -			       &ad7768_interrupt,
> -			       IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> -			       indio_dev->name, indio_dev);
> -	if (ret)
> -		return ret;
> -
> -	ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
> -					      &iio_pollfunc_store_time,
> -					      &ad7768_trigger_handler,
> -					      &ad7768_buffer_ops);
> +	ret = ad7768_triggered_buffer_alloc(indio_dev);
>  	if (ret)
>  		return ret;
>  

Re: [PATCH v1 10/15] iio: adc: ad7768-1: Add support for variable VCM

[Jonathan Cameron]

On Sat, 11 Jan 2025 23:37:29 -0300
Jonathan Santos <jonath4nns@gmail.com> wrote:

> On 01/07, David Lechner wrote:
> > On 1/7/25 9:26 AM, Jonathan Santos wrote:  
> > > From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> > > 
> > > The VCM output voltage can be used as a common-mode voltage within the
> > > amplifier preconditioning circuits external to the AD7768-1.
> > > 
> > > This change exports the VCM as an iio attribute and makes it available
> > > for configuration.  
> > 
> > We model common mode voltage inputs as a regulator consumer (e.g. vcm-supply in
> > DT), so should we make this a regulator provider instead?
> > 
> > It could be used with "regulator-output" to be able to control it from sysfs
> > or if the amplifier ends up in the devicetree for other reasons, the amplifier
> > driver could control the regulator instead of requiring it to by done in sysfs.
> >  
> 
> Ok, that is an interesting approach, I will try this. It makes sense to
> have it in the devicetree. 

I'd be a bit surprised if this ended up being varied in real systems.
So maybe start with a solution that has DT provide a value suitable to the
amplifier (ideally because the amplifier requests it) and we can figure
more dynamic stuff out later if needed.

Jonathan

Re: [PATCH v1 13/15] iio: adc: ad7768-1: add multiple scan types to support 16-bits mode

[Jonathan Cameron]

On Sun, 12 Jan 2025 00:21:37 -0300
Jonathan Santos <jonath4nns@gmail.com> wrote:

> On 01/07, David Lechner wrote:
> > On 1/7/25 9:26 AM, Jonathan Santos wrote:  
> > > When the device is configured to Sinc5 filter and decimation x8,
> > > output data is reduced to 16-bits in order to support 1 MHz of
> > > sampling frequency due to clock limitation.  
> > 
> > We aren't going to get a 1 MHz sample rate without SPI offload support so maybe
> > we should save this patch until then?
> > 
> > In this patch, we are still reading 24-bits per sample, so we aren't really
> > getting any benefit. It is probably fine for now to leave it as 24-bit even if
> > the last 8 bits are all 0 or just noise.  
> 
> Indeed we cannot achieve 1 MHz yet, but I believe it is good have this
> now so it is more mature for the time SPI offload is supported. Also, will
> allow us to backport this patch to other repos.
> 
> > 
> > Also, the datasheet says:
> > 
> > 	this path allows viewing of wider bandwidth; however, it is quantization
> > 	noise limited so that output data is reduced to 16 bits
> > 
> > So this doesn't actually seem related to higher sample rates. There is a CONVLEN
> > bit in the INTERFACE_FORMAT register that globally reduces the output size to
> > 16-bit, which I suspect would be what we will need for achieving the highest
> > sample rate when we add SPI offload support.
> >   
> 
> Right, that is true, but the reason we did this patch was to fix the
> output size when we configure the filter to sinc5 decx8. The datasheet
> says:
> 
> 	To configure the sinc5 filter for 1.024 MSPS output data rate,
> 	write 001 to the FILTER bits [6:4] of the DIGITAL_FILTER register
> 	(Register 0x19). The ADAQ7768-1 automatically changes the decimation
> 	rate to 8 and output data length is reduced to 16 bits from 24 bits 
> 	due to the maximum speed limitation of the digital serial interface.
> 
> In this case we don't even need to change the value of CONVLEN
> 
> > > 
> > > Use multiple scan types feature to enable the driver to switch
> > > scan type in runtime, making possible to support both 24-bit and
> > > 16-bit resolution.
> > > 
> > > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> > > ---
> > >  drivers/iio/adc/ad7768-1.c | 65 ++++++++++++++++++++++++++++++++------
> > >  1 file changed, 56 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> > > index 9741a6d47942..5e4e7d387f9a 100644
> > > --- a/drivers/iio/adc/ad7768-1.c
> > > +++ b/drivers/iio/adc/ad7768-1.c
> > > @@ -134,6 +134,11 @@ struct ad7768_clk_configuration {
> > >  	enum ad7768_pwrmode pwrmode;
> > >  };
> > >  
> > > +enum ad7768_scan_type {
> > > +	AD7768_SCAN_TYPE_NORMAL,
> > > +	AD7768_SCAN_TYPE_HIGH_SPEED,
> > > +};
> > > +
> > >  static const char * const ad7768_vcm_modes[] = {
> > >  	"(AVDD1-AVSS)/2",
> > >  	"2V5",
> > > @@ -145,6 +150,10 @@ static const char * const ad7768_vcm_modes[] = {
> > >  	"OFF",
> > >  };
> > >  
> > > +static const int ad7768_mclk_div_rates[4] = {
> > > +	16, 8, 4, 2,
> > > +};
> > > +
> > >  static const struct ad7768_clk_configuration ad7768_clk_config[] = {
> > >  	{ AD7768_MCLK_DIV_2, AD7768_DEC_RATE_8, 16,  AD7768_FAST_MODE },
> > >  	{ AD7768_MCLK_DIV_2, AD7768_DEC_RATE_16, 32,  AD7768_FAST_MODE },
> > > @@ -159,6 +168,21 @@ static const struct ad7768_clk_configuration ad7768_clk_config[] = {
> > >  	{ AD7768_MCLK_DIV_16, AD7768_DEC_RATE_1024, 16384, AD7768_ECO_MODE },
> > >  };
> > >  
> > > +static const struct iio_scan_type ad7768_scan_type[] = {
> > > +	[AD7768_SCAN_TYPE_NORMAL] = {
> > > +		.sign = 's',
> > > +		.realbits = 24,
> > > +		.storagebits = 32,  
> > 
> > What happened to .shift = 8, ? If there is a reason for removing it, please add
> > that to the commit description.
> >   
> 
> Sorry, will fix this
> 
> > > +		.endianness = IIO_BE,
> > > +	},
> > > +	[AD7768_SCAN_TYPE_HIGH_SPEED] = {
> > > +		.sign = 's',
> > > +		.realbits = 16,
> > > +		.storagebits = 32,  
> > 
> > I guess it doesn't matter much since we are reading one sample at a time, but
> > I would expect storagebits to be 16 instead of 32. Or if it really needs to be
> > 32, does it need shift = 16?
> >   
> 
> This is because the hw is configured to return the samples in a 32 bits
> format, so if storage is 16 we will get wrong data.

Currently we only support one channel (daisy chain mode support might change
that). Not particularly painful to repack and it doubles the data we can fit
in a fifo of a given size.

If this is tricky because of later patches, throw in a common on why.


> 
> > > +		.endianness = IIO_BE,
> > > +	},
> > > +};
> > > +
> > >  static int ad7768_get_vcm(struct iio_dev *dev, const struct iio_chan_spec *chan);
> > >  static int ad7768_set_vcm(struct iio_dev *dev, const struct iio_chan_spec *chan,
> > >  			  unsigned int mode);  
> > 
> > ...
> >   
> > > @@ -308,6 +329,15 @@ static int ad7768_scan_direct(struct iio_dev *indio_dev)
> > >  	ret = ad7768_spi_reg_read(st, AD7768_REG_ADC_DATA, &readval, 3);
> > >  	if (ret < 0)
> > >  		return ret;
> > > +
> > > +	/*
> > > +	 * When the decimation rate is set to x8, the ADC data precision is reduced
> > > +	 * from 24 bits to 16 bits. Since the AD7768_REG_ADC_DATA register provides
> > > +	 * 24-bit data, the precision is reduced by right-shifting the read value
> > > +	 * by 8 bits.
> > > +	 */
> > > +	if (st->dec_rate == 8)
> > > +		readval = readval >> 8;  
> > 
> > Why not change size of ad7768_spi_reg_read() instead of reading 3 bytes and
> > throwing one away?
> >   
> 
> Right, i will check this and fix in the next version
> 
> > >  	/*
> > >  	 * Any SPI configuration of the AD7768-1 can only be
> > >  	 * performed in continuous conversion mode.  

Re: [PATCH v1 14/15] iio: adc: ad7768-1: add support for Synchronization over SPI

[Jonathan Cameron]

On Tue, 7 Jan 2025 17:50:29 -0600
David Lechner <dlechner@baylibre.com> wrote:

> On 1/7/25 9:27 AM, Jonathan Santos wrote:
> > The synchronization method using GPIO requires the generated pulse to be
> > truly synchronous with the base MCLK signal. When it is not possible to
> > do that in hardware, the datasheet recommends using synchronization over
> > SPI, where the generated pulse is already synchronous with MCLK. This
> > requires the SYNC_OUT pin to be connected to SYNC_IN pin.
> > 
> > Add the option to handle device synchronization over SPI.
> > 
> > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> > ---  
> 
> ...
> 
> >  static int ad7768_gpio_direction_input(struct gpio_chip *chip, unsigned int offset)
> > @@ -697,11 +708,21 @@ static int ad7768_setup(struct ad7768_state *st)
> >  	if (ret)
> >  		return ret;
> >  
> > -	st->gpio_sync_in = devm_gpiod_get(&st->spi->dev, "adi,sync-in",
> > -					  GPIOD_OUT_LOW);
> > +	st->gpio_sync_in = devm_gpiod_get_optional(&st->spi->dev, "adi,sync-in",
> > +						   GPIOD_OUT_LOW);
> >  	if (IS_ERR(st->gpio_sync_in))
> >  		return PTR_ERR(st->gpio_sync_in);
> >  
> > +	if (device_property_present(&st->spi->dev, "adi,sync-in-spi"))
> > +		st->en_spi_sync = true;
> > +
> > +	/*
> > +	 * GPIO and SPI Synchronization are mutually exclusive.
> > +	 * Return error if both are enabled  
> 
> Should it also be an error if we have neither? Otherwise it sounds like
> decimation won't work correctly since there is a comment that says we have
> to toggle this after updating the decimation rate register.

I'm not quite sure how this interacts with potential future support
for daisy chaining but for now, if we have no GPIO specified why can't
we assume spi sync is the way to go?

If no GPIO is provided and for a single device SYNC_OUT is not wired
to SYNC_IN I think the board is broken anyway and we don't have to care.

Jonathan

> 
> > +	 */
> > +	if (st->gpio_sync_in && st->en_spi_sync)
> > +		return -EINVAL;  
> 
> A dev_err_probe() message would be helpful here when creating a new DT and
> bringing up a new system since it is easy to forget a property or make a typo
> that could lead to this error.
> 
> > +
> >  	ret = ad7768_gpio_init(st);
> >  	if (ret < 0)
> >  		return ret;  
> 
> 

Re: [PATCH v1 15/15] iio: adc: ad7768-1: add filter type and decimation rate attributes

[Jonathan Cameron]

On Tue, 7 Jan 2025 17:50:56 -0600
David Lechner <dlechner@baylibre.com> wrote:

> On 1/7/25 9:27 AM, Jonathan Santos wrote:
> > Separate filter type and decimation rate from the sampling frequency
> > attribute. The new filter type attribute enables SINC3 and WIDEBAND
> > filters, which were previously unavailable.  
> 
> See related comments in my reply to the documentation patches about wideband vs.
> FIR and decimation rate vs. -3dB cutoff.
> 
> > 
> > Previously, combining decimation and MCLK divider in the sampling
> > frequency obscured performance trade-offs. Lower MCLK divider
> > settings increase power usage, while lower decimation rates reduce
> > precision by decreasing averaging. By creating a decimation attribute,
> > users gain finer control over performance.  
> 
> It seems like we would also want a power_mode attribute. We already have an
> attribute for this for used by accelerometers so there is some precedent for
> such an attribute.

I'm not sure that attribute was ever a good idea :(
So would prefer we don't use it again unless we are really really stuck.

Usual assumption tends to be if anyone wants to reduce power they
should be able to do so with other controls (i.e. reduce sampling rate or
oversampling). Those are easier to interpret than magic low power mode
attributes.

Jonathan


> 
> > 
> > The addition of those attributes allows a wider range of sampling
> > frequencies and more access to the device features.  
> 
> 

Re: [PATCH v1 03/15] Documentation: ABI: testing: ad7768-1: Add device specific ABI documentation.

[Jonathan Cameron]

On Sun, 12 Jan 2025 12:20:47 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Sat, 11 Jan 2025 20:22:36 -0300
> Jonathan Santos <jonath4nns@gmail.com> wrote:
> 
> > On 01/07, David Lechner wrote:  
> > > On 1/7/25 9:24 AM, Jonathan Santos wrote:    
> > > > Add ABI documentation specific to the ad7768-1 device, detailing
> > > > the decimation_rate attribute for better clarity and usability.
> > > > 
> > > > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> > > > ---
> > > >  .../ABI/testing/sysfs-bus-iio-adc-ad7768-1          | 13 +++++++++++++
> > > >  1 file changed, 13 insertions(+)
> > > >  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-ad7768-1
> > > > 
> > > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7768-1 b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7768-1
> > > > new file mode 100644
> > > > index 000000000000..065247f07cfb
> > > > --- /dev/null
> > > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7768-1
> > > > @@ -0,0 +1,13 @@
> > > > +What:		/sys/bus/iio/devices/iio:deviceX/decimation_rate_available
> > > > +KernelVersion:
> > > > +Contact:	linux-iio@vger.kernel.org
> > > > +Description:
> > > > +		Reading returns a range of possible decimation rate values.
> > > > +
> > > > +What:		/sys/bus/iio/devices/iio:deviceX/decimation_rate
> > > > +KernelVersion:
> > > > +Contact:	linux-iio@vger.kernel.org
> > > > +Description:
> > > > +		Sets up the decimation rate for the digital filter. This can
> > > > +		directly impact in the final sampling frequency. Reading returns
> > > > +		the decimation rate. Writing sets the decimation rate.    
> > > 
> > > If this only affects the filter, I would suggest to add `filter_` to the
> > > beginning of the attribute names.
> > > 
> > > Also, an explanation of how to interpret the numbers would be helpful. It looks
> > > like a unitless number that acts a sort of a multiplier or divider, but that
> > > part isn't so clear to me. 
> > > 
> > > Or...
> > > 
> > > Since the decimation rate affects the -3dB point of the filters we could use
> > > the standard IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY instead of introducing
> > > a new attribute.    
> > 
> > Well, here the -3dB cutoff depends on the ODR, which is determined by both the MCLK
> > divider and decimation rate.
> > 
> > Wideband: -3dB at 0.433 × ODR
> > Sinc5: -3dB at 0.204 × ODR
> > Sinc3: -3dB at 0.2617 × ODR
> > 
> > If we use _filter_low_pass_3db_frequency to control the decimation and _sampling_frequency
> > to control the MCLK divider, wouldn’t it be confusing for one to always affect the other?
> > A different ODR would result in a different cutoff, and vice versa.  
> 
> We should definitely not have a filter control changing sampling frequency (which tends to
> be a more common control for users to fiddle with).  However the other way around is
> fine.  So for a given _sampling_frequency present via
> in_xx_filter_low_pass_3db_frequency_available the list of
> possible 3db frequencies and use them to configure the decimation.
> 
> > 
> > Would something like <type>[_name]_oversampling_ratio make more sense? Let me know what you think  
> 
> I'd rather not if we can avoid that new ABI, but it is is better than a new term
> like decimation_rate.
Reading the code I realised I'd misunderstood this question.
Yes for controlling decimation via oversampling.  Decimation (to me at least)
means 'ignoring' data but seems here it means averaging it.

Controls should be in order of preference
sampling_frequency (try to keep this constant as others chagnes - if you have to because of range issues,
  tweak it) This is the rate we get data at after filter.
oversampling_ratio (again try to keep constant) - controls samples taken per output one.
_3db_frequency - anything left to control on the filter, or just a RO output of what it currently means.

Jonathan



> 
> Jonathan
> 
> >   
> 
> 

Re: [PATCH v1 01/15] dt-bindings: iio: adc: ad7768-1: add synchronization over SPI property

[David Lechner]

On 1/11/25 4:34 PM, Jonathan Santos wrote:
> On 01/07, David Lechner wrote:
>> On 1/7/25 9:24 AM, Jonathan Santos wrote:
>>> Add adi,sync-in-spi property to enable synchronization over SPI.
>>> This should be used in the case when the GPIO cannot provide a
>>> pulse synchronous with the base MCLK signal.
>>>
>>> User can choose between SPI, GPIO synchronization or neither of them,
>>> but only if a external pulse can be provided, for example, by another
>>> device in a multidevice setup.
>>>
>>
>> While we are fixing up these bindings, we could add some more trivial things,
>> like power supplies.
>>
>> Also, the interrupt property could use a description since the chip has multiple
>> output pins. I assume it means the /DRDY pin?
>>
> 
> Right! Yes, the interrupt pin refers to the /DRDY.
> 
>>> Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
>>> ---
>>>  .../bindings/iio/adc/adi,ad7768-1.yaml        | 24 ++++++++++++++++++-
>>>  1 file changed, 23 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
>>> index 3ce59d4d065f..55cec27bfe60 100644
>>> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
>>> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
>>> @@ -47,6 +47,15 @@ properties:
>>>        in any way, for example if the filter decimation rate changes.
>>>        As the line is active low, it should be marked GPIO_ACTIVE_LOW.
>>>  
>>> +  adi,sync-in-spi:
>>
>> If this is saying that SYNC_OUT is connected to SYNC_IN, then I think the name
>> should be something like adi,sync-in-sync-out. SPI seems irrelevant here since
>> we should just be describing how things are wired up, not how it is being used.
>>
>> But if we also need to consider the case where SYNC_OUT of one chip is connected
>> to SYNC_IN of another chip, we might want to consider using trigger-source
>> bindings instead (recently standardized in dtschema).
>>
> 
> Do you mean the trigger-sources used for LEDs?

Sort of. There is a more general version of this binding in dt-schema now [1].
But it is essentially the same binding.

[1]: https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/trigger/trigger-source.yaml

> I can try to see if it works, but would it handle the non-GPIO case?

Since the ADC chip is both a trigger provider and a trigger consumer, it would
actually have both properties. A chip that has SYNC_OUT wired to SYNC_IN would
look something like this:

adc1: adc@0 {
  ...
  properties:
    #trigger-source-cells = <0>;
    trigger-sources = <&adc1>
}

> While testing a multidevice setup, I found it simpler to 
> have a single device to manage everything. It lets us toggle the GPIO or /SYNC_OUT
> without referencing another device and makes simultaneous buffered reads easier.
> 
> Maybe we could stick to synchronization within the chip for now.

In the driver, sure. But for the DT bindings, we want to make sure it makes
sense for future use cases as well since it can't be changed later.

Re: [PATCH v1 02/15] Documentation: ABI: add wideband filter type to sysfs-bus-iio

[David Lechner]

On 1/11/25 4:50 PM, Jonathan Santos wrote:
> On 01/07, David Lechner wrote:
>> On 1/7/25 9:24 AM, Jonathan Santos wrote:
>>> The Wideband Low Ripple FIR filter is used for AD7768-1 Driver.
>>> Document wideband filter option into filter_type_avaialable
>>
>> s/avaialable/available/
>>
>>> attribute.
>>>
>>> Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
>>> ---
>>>  Documentation/ABI/testing/sysfs-bus-iio | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
>>> index f83bd6829285..c4c21a7bfba1 100644
>>> --- a/Documentation/ABI/testing/sysfs-bus-iio
>>> +++ b/Documentation/ABI/testing/sysfs-bus-iio
>>> @@ -2291,6 +2291,8 @@ Description:
>>>  		* "sinc3+pf2" - Sinc3 + device specific Post Filter 2.
>>>  		* "sinc3+pf3" - Sinc3 + device specific Post Filter 3.
>>>  		* "sinc3+pf4" - Sinc3 + device specific Post Filter 4.
>>> +		* "wideband" - FIR filter with wideband low ripple passband
>>
>> I think "fir" would be a more specific filter type name than "wideband". (i.e.
>> there are wikipedia pages for sinc and FIR filters, but not one for "wideband"
>> filters)
>>
> 
> Isn't "fir" a bit too generic for this case? Since Wideband here is a class of a FIR filter.
> Maybe something like "wideband-fir" or "fir-wideband" would work better?

For this particular chip, no, there is only one FIR filter option on the chip,
so having additional qualifiers doesn't really make a difference.

If we can find a chip that has more than one FIR filter option, then we could
use that to figure out what additional classifiers are might make sense.
"fir-wideband" would only be useful if the same chip also had a "fir-narrowband"
option. But we might find that instead the chip is actually "fir-type1"
"fir-type2" or something like that.

(FWIW, my signal processing textbook describes Type I, II, III, IV FIR filters
but doesn't say anything about "wideband" FIR filters.)

> 
>>> +		  and sharp transition band.
>>>  
>>>  What:		/sys/.../events/in_proximity_thresh_either_runningperiod
>>>  KernelVersion:	6.6
>>

Re: [PATCH v1 02/15] Documentation: ABI: add wideband filter type to sysfs-bus-iio

[David Lechner]

On 1/12/25 11:39 AM, David Lechner wrote:
> On 1/11/25 4:50 PM, Jonathan Santos wrote:
>> On 01/07, David Lechner wrote:
>>> On 1/7/25 9:24 AM, Jonathan Santos wrote:
>>>> The Wideband Low Ripple FIR filter is used for AD7768-1 Driver.
>>>> Document wideband filter option into filter_type_avaialable
>>>
>>> s/avaialable/available/
>>>
>>>> attribute.
>>>>
>>>> Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
>>>> ---
>>>>  Documentation/ABI/testing/sysfs-bus-iio | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
>>>> index f83bd6829285..c4c21a7bfba1 100644
>>>> --- a/Documentation/ABI/testing/sysfs-bus-iio
>>>> +++ b/Documentation/ABI/testing/sysfs-bus-iio
>>>> @@ -2291,6 +2291,8 @@ Description:
>>>>  		* "sinc3+pf2" - Sinc3 + device specific Post Filter 2.
>>>>  		* "sinc3+pf3" - Sinc3 + device specific Post Filter 3.
>>>>  		* "sinc3+pf4" - Sinc3 + device specific Post Filter 4.
>>>> +		* "wideband" - FIR filter with wideband low ripple passband
>>>
>>> I think "fir" would be a more specific filter type name than "wideband". (i.e.
>>> there are wikipedia pages for sinc and FIR filters, but not one for "wideband"
>>> filters)
>>>
>>
>> Isn't "fir" a bit too generic for this case? Since Wideband here is a class of a FIR filter.
>> Maybe something like "wideband-fir" or "fir-wideband" would work better?
> 
> For this particular chip, no, there is only one FIR filter option on the chip,
> so having additional qualifiers doesn't really make a difference.
> 
> If we can find a chip that has more than one FIR filter option, then we could
> use that to figure out what additional classifiers are might make sense.
> "fir-wideband" would only be useful if the same chip also had a "fir-narrowband"
> option. But we might find that instead the chip is actually "fir-type1"
> "fir-type2" or something like that.
> 
> (FWIW, my signal processing textbook describes Type I, II, III, IV FIR filters
> but doesn't say anything about "wideband" FIR filters.)

Oops, didn't read Jonathan's response first, so disregard this comment.

Re: [PATCH v1 13/15] iio: adc: ad7768-1: add multiple scan types to support 16-bits mode

[David Lechner]

On 1/12/25 6:50 AM, Jonathan Cameron wrote:
> On Sun, 12 Jan 2025 00:21:37 -0300
> Jonathan Santos <jonath4nns@gmail.com> wrote:
> 
>> On 01/07, David Lechner wrote:
>>> On 1/7/25 9:26 AM, Jonathan Santos wrote:  
>>>> When the device is configured to Sinc5 filter and decimation x8,
>>>> output data is reduced to 16-bits in order to support 1 MHz of
>>>> sampling frequency due to clock limitation.  
>>>
>>> We aren't going to get a 1 MHz sample rate without SPI offload support so maybe
>>> we should save this patch until then?
>>>
>>> In this patch, we are still reading 24-bits per sample, so we aren't really
>>> getting any benefit. It is probably fine for now to leave it as 24-bit even if
>>> the last 8 bits are all 0 or just noise.  
>>
>> Indeed we cannot achieve 1 MHz yet, but I believe it is good have this
>> now so it is more mature for the time SPI offload is supported. Also, will
>> allow us to backport this patch to other repos.
>>
>>>
>>> Also, the datasheet says:
>>>
>>> 	this path allows viewing of wider bandwidth; however, it is quantization
>>> 	noise limited so that output data is reduced to 16 bits
>>>
>>> So this doesn't actually seem related to higher sample rates. There is a CONVLEN
>>> bit in the INTERFACE_FORMAT register that globally reduces the output size to
>>> 16-bit, which I suspect would be what we will need for achieving the highest
>>> sample rate when we add SPI offload support.
>>>   
>>
>> Right, that is true, but the reason we did this patch was to fix the
>> output size when we configure the filter to sinc5 decx8. The datasheet
>> says:
>>
>> 	To configure the sinc5 filter for 1.024 MSPS output data rate,
>> 	write 001 to the FILTER bits [6:4] of the DIGITAL_FILTER register
>> 	(Register 0x19). The ADAQ7768-1 automatically changes the decimation
>> 	rate to 8 and output data length is reduced to 16 bits from 24 bits 
>> 	due to the maximum speed limitation of the digital serial interface.
>>
>> In this case we don't even need to change the value of CONVLEN
>>
>>>>
>>>> Use multiple scan types feature to enable the driver to switch
>>>> scan type in runtime, making possible to support both 24-bit and
>>>> 16-bit resolution.
>>>>
>>>> Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
>>>> ---
>>>>  drivers/iio/adc/ad7768-1.c | 65 ++++++++++++++++++++++++++++++++------
>>>>  1 file changed, 56 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
>>>> index 9741a6d47942..5e4e7d387f9a 100644
>>>> --- a/drivers/iio/adc/ad7768-1.c
>>>> +++ b/drivers/iio/adc/ad7768-1.c
>>>> @@ -134,6 +134,11 @@ struct ad7768_clk_configuration {
>>>>  	enum ad7768_pwrmode pwrmode;
>>>>  };
>>>>  
>>>> +enum ad7768_scan_type {
>>>> +	AD7768_SCAN_TYPE_NORMAL,
>>>> +	AD7768_SCAN_TYPE_HIGH_SPEED,
>>>> +};
>>>> +
>>>>  static const char * const ad7768_vcm_modes[] = {
>>>>  	"(AVDD1-AVSS)/2",
>>>>  	"2V5",
>>>> @@ -145,6 +150,10 @@ static const char * const ad7768_vcm_modes[] = {
>>>>  	"OFF",
>>>>  };
>>>>  
>>>> +static const int ad7768_mclk_div_rates[4] = {
>>>> +	16, 8, 4, 2,
>>>> +};
>>>> +
>>>>  static const struct ad7768_clk_configuration ad7768_clk_config[] = {
>>>>  	{ AD7768_MCLK_DIV_2, AD7768_DEC_RATE_8, 16,  AD7768_FAST_MODE },
>>>>  	{ AD7768_MCLK_DIV_2, AD7768_DEC_RATE_16, 32,  AD7768_FAST_MODE },
>>>> @@ -159,6 +168,21 @@ static const struct ad7768_clk_configuration ad7768_clk_config[] = {
>>>>  	{ AD7768_MCLK_DIV_16, AD7768_DEC_RATE_1024, 16384, AD7768_ECO_MODE },
>>>>  };
>>>>  
>>>> +static const struct iio_scan_type ad7768_scan_type[] = {
>>>> +	[AD7768_SCAN_TYPE_NORMAL] = {
>>>> +		.sign = 's',
>>>> +		.realbits = 24,
>>>> +		.storagebits = 32,  
>>>
>>> What happened to .shift = 8, ? If there is a reason for removing it, please add
>>> that to the commit description.
>>>   
>>
>> Sorry, will fix this
>>
>>>> +		.endianness = IIO_BE,
>>>> +	},
>>>> +	[AD7768_SCAN_TYPE_HIGH_SPEED] = {
>>>> +		.sign = 's',
>>>> +		.realbits = 16,
>>>> +		.storagebits = 32,  
>>>
>>> I guess it doesn't matter much since we are reading one sample at a time, but
>>> I would expect storagebits to be 16 instead of 32. Or if it really needs to be
>>> 32, does it need shift = 16?
>>>   
>>
>> This is because the hw is configured to return the samples in a 32 bits
>> format, so if storage is 16 we will get wrong data.

Ah, right, I was forgetting that the data is already coming separately through
the IIO backend interface on a different bus from SPI. So that would be the
comment Jonathan is looking for. :-)

> 
> Currently we only support one channel (daisy chain mode support might change
> that). Not particularly painful to repack and it doubles the data we can fit
> in a fifo of a given size.
> 
> If this is tricky because of later patches, throw in a common on why.
> 
> 
>>
>>>> +		.endianness = IIO_BE,
>>>> +	},
>>>> +};
>>>> +
>>>>  static int ad7768_get_vcm(struct iio_dev *dev, const struct iio_chan_spec *chan);
>>>>  static int ad7768_set_vcm(struct iio_dev *dev, const struct iio_chan_spec *chan,
>>>>  			  unsigned int mode);  
>>>
>>> ...
>>>   
>>>> @@ -308,6 +329,15 @@ static int ad7768_scan_direct(struct iio_dev *indio_dev)
>>>>  	ret = ad7768_spi_reg_read(st, AD7768_REG_ADC_DATA, &readval, 3);
>>>>  	if (ret < 0)
>>>>  		return ret;
>>>> +
>>>> +	/*
>>>> +	 * When the decimation rate is set to x8, the ADC data precision is reduced
>>>> +	 * from 24 bits to 16 bits. Since the AD7768_REG_ADC_DATA register provides
>>>> +	 * 24-bit data, the precision is reduced by right-shifting the read value
>>>> +	 * by 8 bits.
>>>> +	 */
>>>> +	if (st->dec_rate == 8)
>>>> +		readval = readval >> 8;  
>>>
>>> Why not change size of ad7768_spi_reg_read() instead of reading 3 bytes and
>>> throwing one away?
>>>   
>>
>> Right, i will check this and fix in the next version
>>
>>>>  	/*
>>>>  	 * Any SPI configuration of the AD7768-1 can only be
>>>>  	 * performed in continuous conversion mode.  
> 

Re: [PATCH v1 05/15] iio: adc: ad7768-1: set MOSI idle state to high

[Marcelo Schmitt]

On 01/12, Jonathan Cameron wrote:
> On Fri, 10 Jan 2025 18:56:26 -0300
> Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote:
> 
> > On 01/07, David Lechner wrote:
> > > On 1/7/25 9:25 AM, Jonathan Santos wrote:  
> > > > All supported parts require that the MOSI line stays high
> > > > while in idle.
> > > > 
> > > > Configure SPI controller to set MOSI idle state to high.
> > > > 
> > > > Fixes: a5f8c7da3dbe ("iio: adc: Add AD7768-1 ADC basic support")
> > > > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> > > > ---  
> > ...
> > > > @@ -574,6 +574,15 @@ static int ad7768_probe(struct spi_device *spi)
> > > >  		return -ENOMEM;
> > > >  
> > > >  	st = iio_priv(indio_dev);
> > > > +	/*
> > > > +	 * The ADC SDI line must be kept high when
> > > > +	 * data is not being clocked out of the controller.
> > > > +	 * Request the SPI controller to make MOSI idle high.
> > > > +	 */
> > > > +	spi->mode |= SPI_MOSI_IDLE_HIGH;
> > > > +	ret = spi_setup(spi);
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > >  	st->spi = spi;
> > > >  
> > > >  	st->vref = devm_regulator_get(&spi->dev, "vref");  
> > > 
> > > Very few SPI controllers currently have the SPI_MOSI_IDLE_HIGH capability flag
> > > set in Linux right now (whether they actually support it or not), so this could
> > > break existing users.  
> > 
> > Good point. Maybe only dev_warn() if SPI_MOSI_IDLE_HIGH is not supported?
> > 
> > >   
> > ...
> > > 
> > > If we ever do implement a data read of more than 64 bits without toggling CS,
> > > then we could just set the TX data to be all 0xFF and have the same effect
> > > without requiring the SPI controller to support SPI_MOSI_IDLE_HIGH.  
> > 
> > One point of having SPI_MOSI_IDLE_HIGH is that the controller may bring MOSI low
> > between data words of a transfer. I think all transfer words are going to be
> > either 16 or 24 with the new patches setting bits_per_word in all transfers but
> > that might still not be enough if eventually the controller is unable to support
> > those word sizes. 
> 
> Can we make the use of SPI_MOSI_IDLE_HIGH only apply if controller doesn't support
> what is required to do the transfers in one go?

I think so, but that would require tweaking spi controller drivers since we
don't know at spi_setup() what transfers will ask for their bits_per_word.
Not excited with this idea but may try something if that makes it easier to
support these unusual SPI devices.

> 
> > Plus you would have the complication of filling the tx_buf for
> > all transfers.
> 
> Wrap that up in a regmap, or read and write functions and that should be easy enough.
> 
> > 
> > For the part that instigated the development of SPI_MOSI_IDLE_HIGH, the MOSI line
> > also had to be high in between transfers. The diagrams at AD7768-1 datasheet
> > page 51 suggest the same would be needed for this chip too.
> 
> Whilst the datasheet indeed draws lines for that, i doubt it notices except on
> clock transitions and between transfers the clock won't do anything.
> If we confirm that the device does notice, then I don't mind limiting the controllers
> to those with that can ensure it doesn't get set wrong.
> 
> Jonathan
> 
> 

Re: [PATCH v1 01/15] dt-bindings: iio: adc: ad7768-1: add synchronization over SPI property

[Jonathan Santos]

On 01/12, Jonathan Cameron wrote:
> On Sat, 11 Jan 2025 19:34:14 -0300
> Jonathan Santos <jonath4nns@gmail.com> wrote:
> 
> > On 01/07, David Lechner wrote:
> > > On 1/7/25 9:24 AM, Jonathan Santos wrote:  
> > > > Add adi,sync-in-spi property to enable synchronization over SPI.
> > > > This should be used in the case when the GPIO cannot provide a
> > > > pulse synchronous with the base MCLK signal.
> > > > 
> > > > User can choose between SPI, GPIO synchronization or neither of them,
> > > > but only if a external pulse can be provided, for example, by another
> > > > device in a multidevice setup.
> > > >   
> > > 
> > > While we are fixing up these bindings, we could add some more trivial things,
> > > like power supplies.
> > > 
> > > Also, the interrupt property could use a description since the chip has multiple
> > > output pins. I assume it means the /DRDY pin?
> > >   
> > 
> > Right! Yes, the interrupt pin refers to the /DRDY.
> > 
> > > > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> > > > ---
> > > >  .../bindings/iio/adc/adi,ad7768-1.yaml        | 24 ++++++++++++++++++-
> > > >  1 file changed, 23 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
> > > > index 3ce59d4d065f..55cec27bfe60 100644
> > > > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
> > > > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
> > > > @@ -47,6 +47,15 @@ properties:
> > > >        in any way, for example if the filter decimation rate changes.
> > > >        As the line is active low, it should be marked GPIO_ACTIVE_LOW.
> > > >  
> > > > +  adi,sync-in-spi:  
> > > 
> > > If this is saying that SYNC_OUT is connected to SYNC_IN, then I think the name
> > > should be something like adi,sync-in-sync-out. SPI seems irrelevant here since
> > > we should just be describing how things are wired up, not how it is being used.
> > > 
> > > But if we also need to consider the case where SYNC_OUT of one chip is connected
> > > to SYNC_IN of another chip, we might want to consider using trigger-source
> > > bindings instead (recently standardized in dtschema).
> > >   
> > 
> > Do you mean the trigger-sources used for LEDs? I can try to see if it works, but would it
> > handle the non-GPIO case? While testing a multidevice setup, I found it simpler to 
> > have a single device to manage everything. It lets us toggle the GPIO or /SYNC_OUT
> > without referencing another device and makes simultaneous buffered reads easier.
> 
> Daisy-chain mode (figure 131)?  In that case we normally end up with a single presented device
> with a 'lot' of channels. (See the electric car style battery charging chips, those can
> be chained in very large numbers!)
>

Actually, it is more like Figure 133 , but the premise is similar. We
have here a Quad setup.

> Probably similar for figure 133 (which is a dual SPI setup) as the SPI clock must
> be shared so we still see it over a single interface.
> 

We could view them as a single device with multiple channels, and since
the goal is to read them simultaneously with buffered reads, some parameters
such as sampling frequency should be equal to all devices.

However, there are some implications: If we do the above, we have
limitations in the customization of the "channels", they would have
the same filter, frequency modulator and scale (we plan to add support
for ADAQ776x-1 series, which include PGA and AAF gain).

To customize them separetely, we would need to assert only the
corresponding chip select, which is only possible with different
instances, as far as I know.

> If those are the only two options then keeping this within the driver is fine.
> For daisy chain there are examples in tree and it normally means we have to
> have a DT parameter that says how long the chain is, though we maybe can
> do that with per channel nodes as well if those make sense here.
> 
> Jonathan
> 

Those are the options in the datasheet and in hardware so far. I was 
considering other scenarios in case the user combine them differently.
I believe keping within the driver covers the main cases. 

> 
> > 
> > Maybe we could stick to synchronization within the chip for now.
> > 
> > > > +    description:
> > > > +      Enables synchronization of multiple devices over SPI. This property is
> > > > +      used when a signal synchronous to the base MCLK signal cannot be provided
> > > > +      via GPIO. It requires the SYNC_OUT pin to be connected to the SYNC_IN pin
> > > > +      on the ADC. In the case of multiple devices, the SYNC_OUT pin of one device
> > > > +      should be routed to the SYNC_IN pins of the other devices.
> > > > +    type: boolean
> > > > +
> > > >    reset-gpios:
> > > >      maxItems: 1
> > > >  
> > > > @@ -65,7 +74,6 @@ required:
> > > >    - vref-supply
> > > >    - spi-cpol
> > > >    - spi-cpha
> > > > -  - adi,sync-in-gpios
> > > >  
> > > >  patternProperties:
> > > >    "^channel@([0-9]|1[0-5])$":
> > > > @@ -89,6 +97,20 @@ patternProperties:
> > > >  allOf:
> > > >    - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > > >  
> > > > +  # adi,sync-in-gpios and adi,sync-in-spi are mutually exclusive (neither is also valid)
> > > > +  - if:
> > > > +      required:
> > > > +        - adi,sync-in-gpios
> > > > +    then:
> > > > +      properties:
> > > > +        adi,sync-in-spi: false
> > > > +  - if:
> > > > +      required:
> > > > +        - adi,sync-in-spi
> > > > +    then:
> > > > +      properties:
> > > > +        adi,sync-in-gpios: false  
> > > 
> > > I think this can be simplified to using oneOf: to provide XOR validation
> > >   
> > 
> > Right!
> > 
> > > > +
> > > >  unevaluatedProperties: false
> > > >  
> > > >  examples:  
> > >   
> 

Re: [PATCH v1 01/15] dt-bindings: iio: adc: ad7768-1: add synchronization over SPI property

[Jonathan Santos]

On 01/12, Jonathan Cameron wrote:
> On Fri, 10 Jan 2025 18:51:06 -0300
> Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote:
> 
> > On 01/07, Jonathan Santos wrote:
> > > Add adi,sync-in-spi property to enable synchronization over SPI.
> > > This should be used in the case when the GPIO cannot provide a
> > > pulse synchronous with the base MCLK signal.
> > > 
> > > User can choose between SPI, GPIO synchronization or neither of them,
> > > but only if a external pulse can be provided, for example, by another
> > > device in a multidevice setup.
> > > 
> > > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> > > ---
> > >  .../bindings/iio/adc/adi,ad7768-1.yaml        | 24 ++++++++++++++++++-
> > >  1 file changed, 23 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
> > > index 3ce59d4d065f..55cec27bfe60 100644
> > > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
> > > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
> > > @@ -47,6 +47,15 @@ properties:
> > >        in any way, for example if the filter decimation rate changes.
> > >        As the line is active low, it should be marked GPIO_ACTIVE_LOW.
> > >  
> > > +  adi,sync-in-spi:
> > > +    description:
> > > +      Enables synchronization of multiple devices over SPI. This property is
> > > +      used when a signal synchronous to the base MCLK signal cannot be provided
> > > +      via GPIO. It requires the SYNC_OUT pin to be connected to the SYNC_IN pin
> > > +      on the ADC. In the case of multiple devices, the SYNC_OUT pin of one device
> > > +      should be routed to the SYNC_IN pins of the other devices.  
> > So, if I'm getting it right,
> 
> Datasheet on this is indeed complex!
> 
> >/SYNC_IN may be driven by a GPIO (ADAQ7768-1 datasheet Figure 131),
> 
> For that we expose a gpio binding already. If that's present we know what is going on.
> 
> >/SYNC_IN may be driven by own device /SYNC_OUT (ADAQ7768-1 datasheet Figure 133),
> 
> This is the default - no information provided so it isn't wired externally.
> We don't normally bother to describe required chip to chip connections.
> I couldn't entirely figure out if this is 'required' if we aren't driving explicitly
> from GPIO or another chip but i think it is(?).
>

It is. If the device is not being driven by a GPIO or an external
device, the user should connect the /SYNC_OUT to /SYNC_IN. We could
assume that this is the case if the GPIO is not present in the
devicetree, maybe put that into the description. The sync-out-sync-in
property as a boolean can be quite redundant, since we also need to
remove the GPIO.

> >/SYNC_IN may be driven by other AD7768-1 > /SYNC_OUT pin (also Figure 133).
> This is only case we are about for sync in I think.
> 
> As long as there isn't a valid 'not connected' It think we are fine with a boolean.
> 

If opt for a single node instace, thats ok; otherwise, David's
trigger-sources suggestion seems to be the best approach. 

> > That is too much to describe with a boolean.
> > 
> > If David's suggestion of using a trigger-source doesn't fit, this property
> > should at least become an enum or string.
> > 
> > > +    type: boolean
> > > +
> > >    reset-gpios:
> > >      maxItems: 1
> > >  
> > > @@ -65,7 +74,6 @@ required:
> > >    - vref-supply
> > >    - spi-cpol
> > >    - spi-cpha
> > > -  - adi,sync-in-gpios
> > >  
> > >  patternProperties:
> > >    "^channel@([0-9]|1[0-5])$":
> > > @@ -89,6 +97,20 @@ patternProperties:
> > >  allOf:
> > >    - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > >  
> > > +  # adi,sync-in-gpios and adi,sync-in-spi are mutually exclusive (neither is also valid)
> > > +  - if:
> > > +      required:
> > > +        - adi,sync-in-gpios
> > > +    then:
> > > +      properties:
> > > +        adi,sync-in-spi: false
> > > +  - if:
> > > +      required:
> > > +        - adi,sync-in-spi
> > > +    then:
> > > +      properties:
> > > +        adi,sync-in-gpios: false
> > > +
> > >  unevaluatedProperties: false
> > >  
> > >  examples:
> > > -- 
> > > 2.34.1
> > >   
> 

Re: [PATCH v1 02/15] Documentation: ABI: add wideband filter type to sysfs-bus-iio

[Jonathan Santos]

On 01/12, Jonathan Cameron wrote:
> On Sat, 11 Jan 2025 19:50:18 -0300
> Jonathan Santos <jonath4nns@gmail.com> wrote:
> 
> > On 01/07, David Lechner wrote:
> > > On 1/7/25 9:24 AM, Jonathan Santos wrote:  
> > > > The Wideband Low Ripple FIR filter is used for AD7768-1 Driver.
> > > > Document wideband filter option into filter_type_avaialable  
> > > 
> > > s/avaialable/available/
> > >   
> > > > attribute.
> > > > 
> > > > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> > > > ---
> > > >  Documentation/ABI/testing/sysfs-bus-iio | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> > > > index f83bd6829285..c4c21a7bfba1 100644
> > > > --- a/Documentation/ABI/testing/sysfs-bus-iio
> > > > +++ b/Documentation/ABI/testing/sysfs-bus-iio
> > > > @@ -2291,6 +2291,8 @@ Description:
> > > >  		* "sinc3+pf2" - Sinc3 + device specific Post Filter 2.
> > > >  		* "sinc3+pf3" - Sinc3 + device specific Post Filter 3.
> > > >  		* "sinc3+pf4" - Sinc3 + device specific Post Filter 4.
> > > > +		* "wideband" - FIR filter with wideband low ripple passband  
> > > 
> > > I think "fir" would be a more specific filter type name than "wideband". (i.e.
> > > there are wikipedia pages for sinc and FIR filters, but not one for "wideband"
> > > filters)
> > >   
> > 
> > Isn't "fir" a bit too generic for this case? Since Wideband here is a class of a FIR filter.
> > Maybe something like "wideband-fir" or "fir-wideband" would work better?
> 
> 
> Not sure FIR is even useful. That's just a particular filter architecture, not
> related directly to the characteristics userspace cares about.
> You can sometimes at least build a very similar response from an IIR filter.
> The sinc ones describe the pattern they let through, FIR isn't that specific.
> So I'd not mention FIR anywhere.
> 

So is "wideband" ok for this situation? Removing the FIR mention.

> > 
> > > > +		  and sharp transition band.
> > > >  
> > > >  What:		/sys/.../events/in_proximity_thresh_either_runningperiod
> > > >  KernelVersion:	6.6  
> > >   
> 

Re: [PATCH v1 03/15] Documentation: ABI: testing: ad7768-1: Add device specific ABI documentation.

[Jonathan Santos]

On 01/12, Jonathan Cameron wrote:
> On Sun, 12 Jan 2025 12:20:47 +0000
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > On Sat, 11 Jan 2025 20:22:36 -0300
> > Jonathan Santos <jonath4nns@gmail.com> wrote:
> > 
> > > On 01/07, David Lechner wrote:  
> > > > On 1/7/25 9:24 AM, Jonathan Santos wrote:    
> > > > > Add ABI documentation specific to the ad7768-1 device, detailing
> > > > > the decimation_rate attribute for better clarity and usability.
> > > > > 
> > > > > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> > > > > ---
> > > > >  .../ABI/testing/sysfs-bus-iio-adc-ad7768-1          | 13 +++++++++++++
> > > > >  1 file changed, 13 insertions(+)
> > > > >  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-ad7768-1
> > > > > 
> > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7768-1 b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7768-1
> > > > > new file mode 100644
> > > > > index 000000000000..065247f07cfb
> > > > > --- /dev/null
> > > > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7768-1
> > > > > @@ -0,0 +1,13 @@
> > > > > +What:		/sys/bus/iio/devices/iio:deviceX/decimation_rate_available
> > > > > +KernelVersion:
> > > > > +Contact:	linux-iio@vger.kernel.org
> > > > > +Description:
> > > > > +		Reading returns a range of possible decimation rate values.
> > > > > +
> > > > > +What:		/sys/bus/iio/devices/iio:deviceX/decimation_rate
> > > > > +KernelVersion:
> > > > > +Contact:	linux-iio@vger.kernel.org
> > > > > +Description:
> > > > > +		Sets up the decimation rate for the digital filter. This can
> > > > > +		directly impact in the final sampling frequency. Reading returns
> > > > > +		the decimation rate. Writing sets the decimation rate.    
> > > > 
> > > > If this only affects the filter, I would suggest to add `filter_` to the
> > > > beginning of the attribute names.
> > > > 
> > > > Also, an explanation of how to interpret the numbers would be helpful. It looks
> > > > like a unitless number that acts a sort of a multiplier or divider, but that
> > > > part isn't so clear to me. 
> > > > 
> > > > Or...
> > > > 
> > > > Since the decimation rate affects the -3dB point of the filters we could use
> > > > the standard IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY instead of introducing
> > > > a new attribute.    
> > > 
> > > Well, here the -3dB cutoff depends on the ODR, which is determined by both the MCLK
> > > divider and decimation rate.
> > > 
> > > Wideband: -3dB at 0.433 × ODR
> > > Sinc5: -3dB at 0.204 × ODR
> > > Sinc3: -3dB at 0.2617 × ODR
> > > 
> > > If we use _filter_low_pass_3db_frequency to control the decimation and _sampling_frequency
> > > to control the MCLK divider, wouldn’t it be confusing for one to always affect the other?
> > > A different ODR would result in a different cutoff, and vice versa.  
> > 
> > We should definitely not have a filter control changing sampling frequency (which tends to
> > be a more common control for users to fiddle with).  However the other way around is
> > fine.  So for a given _sampling_frequency present via
> > in_xx_filter_low_pass_3db_frequency_available the list of
> > possible 3db frequencies and use them to configure the decimation.
> > 
> > > 
> > > Would something like <type>[_name]_oversampling_ratio make more sense? Let me know what you think  
> > 
> > I'd rather not if we can avoid that new ABI, but it is is better than a new term
> > like decimation_rate.
> Reading the code I realised I'd misunderstood this question.
> Yes for controlling decimation via oversampling.  Decimation (to me at least)
> means 'ignoring' data but seems here it means averaging it.
> 

Yes, for this device the decimation is indeed averaging the data.

> Controls should be in order of preference
> sampling_frequency (try to keep this constant as others chagnes - if you have to because of range issues,
>   tweak it) This is the rate we get data at after filter.
> oversampling_ratio (again try to keep constant) - controls samples taken per output one.
> _3db_frequency - anything left to control on the filter, or just a RO output of what it currently means.
> 
> Jonathan
> 

Great, then. Decimation will be controlled by oversampling, frequency 
modulator controlled by sampling frequency and _3db_frequency as RO 
attribute, since there are no parameters left.

> 
> 
> > 
> > Jonathan
> > 
> > >   
> > 
> > 
> 

Re: [PATCH v1 09/15] iio: adc: ad7768-1: Move buffer allocation to a separate function

[Jonathan Santos]

On 01/12, Jonathan Cameron wrote:
> On Tue, 7 Jan 2025 12:26:05 -0300
> Jonathan Santos <Jonathan.Santos@analog.com> wrote:
> 
> > From: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> > 
> > This change moves the buffer allocation in a separate function, making
> > space for adding another type of iio buffer if needed.
> > 
> > Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> > ---
> >  drivers/iio/adc/ad7768-1.c | 64 ++++++++++++++++++++++----------------
> >  1 file changed, 37 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> > index cd1b08053105..eaa9a12737ac 100644
> > --- a/drivers/iio/adc/ad7768-1.c
> > +++ b/drivers/iio/adc/ad7768-1.c
> > @@ -163,6 +163,7 @@ struct ad7768_state {
> >  	struct iio_trigger *trig;
> >  	struct gpio_desc *gpio_sync_in;
> >  	struct gpio_desc *gpio_reset;
> > +	int irq;
> >  	const char *labels[ARRAY_SIZE(ad7768_channels)];
> >  	/*
> >  	 * DMA (thus cache coherency maintenance) may require the
> > @@ -569,6 +570,40 @@ static int ad7768_set_channel_label(struct iio_dev *indio_dev,
> >  	return 0;
> >  }
> >  
> > +static int ad7768_triggered_buffer_alloc(struct iio_dev *indio_dev)
> > +{
> > +	struct ad7768_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	st->trig = devm_iio_trigger_alloc(indio_dev->dev.parent, "%s-dev%d",
> > +					  indio_dev->name,
> > +					  iio_device_id(indio_dev));
> > +	if (!st->trig)
> > +		return -ENOMEM;
> > +
> > +	st->trig->ops = &ad7768_trigger_ops;
> > +	iio_trigger_set_drvdata(st->trig, indio_dev);
> > +	ret = devm_iio_trigger_register(indio_dev->dev.parent, st->trig);
> > +	if (ret)
> > +		return ret;
> > +
> > +	indio_dev->trig = iio_trigger_get(st->trig);
> > +
> > +	init_completion(&st->completion);
> > +
> > +	ret = devm_request_irq(indio_dev->dev.parent, st->irq,
> > +			       &ad7768_interrupt,
> > +			       IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> > +			       indio_dev->name, indio_dev);
> > +	if (ret)
> 
> As with the completion, the interrupt is not just for triggered buffer use
> either, so probably doesn't belong in this function.  Again, not
> a bug to do so, but hurts readability a little.
> 

OK, i see it now. I will make the modifcations.

> > +		return ret;
> > +
> > +	return devm_iio_triggered_buffer_setup(indio_dev->dev.parent, indio_dev,
> > +					       &iio_pollfunc_store_time,
> > +					       &ad7768_trigger_handler,
> > +					       &ad7768_buffer_ops);
> > +}
> > +
> >  static int ad7768_probe(struct spi_device *spi)
> >  {
> >  	struct ad7768_state *st;
> > @@ -610,6 +645,7 @@ static int ad7768_probe(struct spi_device *spi)
> >  		return PTR_ERR(st->mclk);
> >  
> >  	st->mclk_freq = clk_get_rate(st->mclk);
> > +	st->irq = spi->irq;
> >  
> >  	mutex_init(&st->lock);
> >  
> > @@ -625,37 +661,11 @@ static int ad7768_probe(struct spi_device *spi)
> >  		return ret;
> >  	}
> >  
> > -	st->trig = devm_iio_trigger_alloc(&spi->dev, "%s-dev%d",
> > -					  indio_dev->name,
> > -					  iio_device_id(indio_dev));
> > -	if (!st->trig)
> > -		return -ENOMEM;
> > -
> > -	st->trig->ops = &ad7768_trigger_ops;
> > -	iio_trigger_set_drvdata(st->trig, indio_dev);
> > -	ret = devm_iio_trigger_register(&spi->dev, st->trig);
> > -	if (ret)
> > -		return ret;
> > -
> > -	indio_dev->trig = iio_trigger_get(st->trig);
> > -
> > -	init_completion(&st->completion);
> > -
> >  	ret = ad7768_set_channel_label(indio_dev, ARRAY_SIZE(ad7768_channels));
> >  	if (ret)
> >  		return ret;
> >  
> > -	ret = devm_request_irq(&spi->dev, spi->irq,
> > -			       &ad7768_interrupt,
> > -			       IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> > -			       indio_dev->name, indio_dev);
> > -	if (ret)
> > -		return ret;
> > -
> > -	ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
> > -					      &iio_pollfunc_store_time,
> > -					      &ad7768_trigger_handler,
> > -					      &ad7768_buffer_ops);
> > +	ret = ad7768_triggered_buffer_alloc(indio_dev);
> >  	if (ret)
> >  		return ret;
> >  
> 

Re: [PATCH v1 14/15] iio: adc: ad7768-1: add support for Synchronization over SPI

[Jonathan Santos]

On 01/12, Jonathan Cameron wrote:
> On Tue, 7 Jan 2025 17:50:29 -0600
> David Lechner <dlechner@baylibre.com> wrote:
> 
> > On 1/7/25 9:27 AM, Jonathan Santos wrote:
> > > The synchronization method using GPIO requires the generated pulse to be
> > > truly synchronous with the base MCLK signal. When it is not possible to
> > > do that in hardware, the datasheet recommends using synchronization over
> > > SPI, where the generated pulse is already synchronous with MCLK. This
> > > requires the SYNC_OUT pin to be connected to SYNC_IN pin.
> > > 
> > > Add the option to handle device synchronization over SPI.
> > > 
> > > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> > > ---  
> > 
> > ...
> > 
> > >  static int ad7768_gpio_direction_input(struct gpio_chip *chip, unsigned int offset)
> > > @@ -697,11 +708,21 @@ static int ad7768_setup(struct ad7768_state *st)
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > -	st->gpio_sync_in = devm_gpiod_get(&st->spi->dev, "adi,sync-in",
> > > -					  GPIOD_OUT_LOW);
> > > +	st->gpio_sync_in = devm_gpiod_get_optional(&st->spi->dev, "adi,sync-in",
> > > +						   GPIOD_OUT_LOW);
> > >  	if (IS_ERR(st->gpio_sync_in))
> > >  		return PTR_ERR(st->gpio_sync_in);
> > >  
> > > +	if (device_property_present(&st->spi->dev, "adi,sync-in-spi"))
> > > +		st->en_spi_sync = true;
> > > +
> > > +	/*
> > > +	 * GPIO and SPI Synchronization are mutually exclusive.
> > > +	 * Return error if both are enabled  
> > 
> > Should it also be an error if we have neither? Otherwise it sounds like
> > decimation won't work correctly since there is a comment that says we have
> > to toggle this after updating the decimation rate register.
> 
> I'm not quite sure how this interacts with potential future support
> for daisy chaining but for now, if we have no GPIO specified why can't
> we assume spi sync is the way to go?
> 
> If no GPIO is provided and for a single device SYNC_OUT is not wired
> to SYNC_IN I think the board is broken anyway and we don't have to care.
> 
> Jonathan
> 

I also think it is safe to assume the spi sync if we don't have the GPIO
defined. For Daisy chaining it would be as problem if we decided for
multiple instances in case of multiple devices. With the single device
approach it still works.

> > 
> > > +	 */
> > > +	if (st->gpio_sync_in && st->en_spi_sync)
> > > +		return -EINVAL;  
> > 
> > A dev_err_probe() message would be helpful here when creating a new DT and
> > bringing up a new system since it is easy to forget a property or make a typo
> > that could lead to this error.
> > 
> > > +
> > >  	ret = ad7768_gpio_init(st);
> > >  	if (ret < 0)
> > >  		return ret;  
> > 
> > 
> 

Re: [PATCH v1 15/15] iio: adc: ad7768-1: add filter type and decimation rate attributes

[Jonathan Santos]

On 01/12, Jonathan Cameron wrote:
> On Tue, 7 Jan 2025 17:50:56 -0600
> David Lechner <dlechner@baylibre.com> wrote:
> 
> > On 1/7/25 9:27 AM, Jonathan Santos wrote:
> > > Separate filter type and decimation rate from the sampling frequency
> > > attribute. The new filter type attribute enables SINC3 and WIDEBAND
> > > filters, which were previously unavailable.  
> > 
> > See related comments in my reply to the documentation patches about wideband vs.
> > FIR and decimation rate vs. -3dB cutoff.
> > 
> > > 
> > > Previously, combining decimation and MCLK divider in the sampling
> > > frequency obscured performance trade-offs. Lower MCLK divider
> > > settings increase power usage, while lower decimation rates reduce
> > > precision by decreasing averaging. By creating a decimation attribute,
> > > users gain finer control over performance.  
> > 
> > It seems like we would also want a power_mode attribute. We already have an
> > attribute for this for used by accelerometers so there is some precedent for
> > such an attribute.
> 
> I'm not sure that attribute was ever a good idea :(
> So would prefer we don't use it again unless we are really really stuck.
> 
> Usual assumption tends to be if anyone wants to reduce power they
> should be able to do so with other controls (i.e. reduce sampling rate or
> oversampling). Those are easier to interpret than magic low power mode
> attributes.
> 
> Jonathan
> 

I tend do agree with Jonathan in this one. the power mode is directly linked to
MCLK divider, which is under sampling frequency now. Basicly, we assume
that higher sample rates lead to an increase in power usage.
Even oversampling does not affect the power mode. 

Configuring an inadequate power mode, depending on the mclk divider, may
result in malfunction.

> 
> > 
> > > 
> > > The addition of those attributes allows a wider range of sampling
> > > frequencies and more access to the device features.  
> > 
> > 
> 

Re: [PATCH v1 01/15] dt-bindings: iio: adc: ad7768-1: add synchronization over SPI property

[David Lechner]

On 1/13/25 6:18 PM, Jonathan Santos wrote:
> On 01/12, Jonathan Cameron wrote:
>> On Sat, 11 Jan 2025 19:34:14 -0300
>> Jonathan Santos <jonath4nns@gmail.com> wrote:
>>
>>> On 01/07, David Lechner wrote:
>>>> On 1/7/25 9:24 AM, Jonathan Santos wrote:  
>>>>> Add adi,sync-in-spi property to enable synchronization over SPI.
>>>>> This should be used in the case when the GPIO cannot provide a
>>>>> pulse synchronous with the base MCLK signal.
>>>>>
>>>>> User can choose between SPI, GPIO synchronization or neither of them,
>>>>> but only if a external pulse can be provided, for example, by another
>>>>> device in a multidevice setup.
>>>>>   
>>>>
>>>> While we are fixing up these bindings, we could add some more trivial things,
>>>> like power supplies.
>>>>
>>>> Also, the interrupt property could use a description since the chip has multiple
>>>> output pins. I assume it means the /DRDY pin?
>>>>   
>>>
>>> Right! Yes, the interrupt pin refers to the /DRDY.
>>>
>>>>> Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
>>>>> ---
>>>>>  .../bindings/iio/adc/adi,ad7768-1.yaml        | 24 ++++++++++++++++++-
>>>>>  1 file changed, 23 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
>>>>> index 3ce59d4d065f..55cec27bfe60 100644
>>>>> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
>>>>> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
>>>>> @@ -47,6 +47,15 @@ properties:
>>>>>        in any way, for example if the filter decimation rate changes.
>>>>>        As the line is active low, it should be marked GPIO_ACTIVE_LOW.
>>>>>  
>>>>> +  adi,sync-in-spi:  
>>>>
>>>> If this is saying that SYNC_OUT is connected to SYNC_IN, then I think the name
>>>> should be something like adi,sync-in-sync-out. SPI seems irrelevant here since
>>>> we should just be describing how things are wired up, not how it is being used.
>>>>
>>>> But if we also need to consider the case where SYNC_OUT of one chip is connected
>>>> to SYNC_IN of another chip, we might want to consider using trigger-source
>>>> bindings instead (recently standardized in dtschema).
>>>>   
>>>
>>> Do you mean the trigger-sources used for LEDs? I can try to see if it works, but would it
>>> handle the non-GPIO case? While testing a multidevice setup, I found it simpler to 
>>> have a single device to manage everything. It lets us toggle the GPIO or /SYNC_OUT
>>> without referencing another device and makes simultaneous buffered reads easier.
>>
>> Daisy-chain mode (figure 131)?  In that case we normally end up with a single presented device
>> with a 'lot' of channels. (See the electric car style battery charging chips, those can
>> be chained in very large numbers!)
>>
> 
> Actually, it is more like Figure 133 , but the premise is similar. We
> have here a Quad setup.
> 
>> Probably similar for figure 133 (which is a dual SPI setup) as the SPI clock must
>> be shared so we still see it over a single interface.
>>
> 
> We could view them as a single device with multiple channels, and since
> the goal is to read them simultaneously with buffered reads, some parameters
> such as sampling frequency should be equal to all devices.
> 
> However, there are some implications: If we do the above, we have
> limitations in the customization of the "channels", they would have
> the same filter, frequency modulator and scale (we plan to add support
> for ADAQ776x-1 series, which include PGA and AAF gain).
> 
> To customize them separetely, we would need to assert only the
> corresponding chip select, which is only possible with different
> instances, as far as I know.

FYI, I've been discussing with the HDL folks at ADI about how we could make a
multi-bus SPI controller, similar to controllers used for parallel SPI flash
memories that are used as a single logical device. So that is the solution I
am hoping for here. It would would allow a single IIO device instance for
multiple chips. But the SPI controller would allow addressing individual chips
for configuration and addressing all chips at the same time for reading sample
data.

> 
>> If those are the only two options then keeping this within the driver is fine.
>> For daisy chain there are examples in tree and it normally means we have to
>> have a DT parameter that says how long the chain is, though we maybe can
>> do that with per channel nodes as well if those make sense here.
>>
>> Jonathan
>>
> 
> Those are the options in the datasheet and in hardware so far. I was 
> considering other scenarios in case the user combine them differently.
> I believe keping within the driver covers the main cases. 
> 

Re: [PATCH v1 01/15] dt-bindings: iio: adc: ad7768-1: add synchronization over SPI property

[Jonathan Cameron]

On Tue, 14 Jan 2025 10:05:02 -0600
David Lechner <dlechner@baylibre.com> wrote:

> On 1/13/25 6:18 PM, Jonathan Santos wrote:
> > On 01/12, Jonathan Cameron wrote:  
> >> On Sat, 11 Jan 2025 19:34:14 -0300
> >> Jonathan Santos <jonath4nns@gmail.com> wrote:
> >>  
> >>> On 01/07, David Lechner wrote:  
> >>>> On 1/7/25 9:24 AM, Jonathan Santos wrote:    
> >>>>> Add adi,sync-in-spi property to enable synchronization over SPI.
> >>>>> This should be used in the case when the GPIO cannot provide a
> >>>>> pulse synchronous with the base MCLK signal.
> >>>>>
> >>>>> User can choose between SPI, GPIO synchronization or neither of them,
> >>>>> but only if a external pulse can be provided, for example, by another
> >>>>> device in a multidevice setup.
> >>>>>     
> >>>>
> >>>> While we are fixing up these bindings, we could add some more trivial things,
> >>>> like power supplies.
> >>>>
> >>>> Also, the interrupt property could use a description since the chip has multiple
> >>>> output pins. I assume it means the /DRDY pin?
> >>>>     
> >>>
> >>> Right! Yes, the interrupt pin refers to the /DRDY.
> >>>  
> >>>>> Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> >>>>> ---
> >>>>>  .../bindings/iio/adc/adi,ad7768-1.yaml        | 24 ++++++++++++++++++-
> >>>>>  1 file changed, 23 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
> >>>>> index 3ce59d4d065f..55cec27bfe60 100644
> >>>>> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
> >>>>> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
> >>>>> @@ -47,6 +47,15 @@ properties:
> >>>>>        in any way, for example if the filter decimation rate changes.
> >>>>>        As the line is active low, it should be marked GPIO_ACTIVE_LOW.
> >>>>>  
> >>>>> +  adi,sync-in-spi:    
> >>>>
> >>>> If this is saying that SYNC_OUT is connected to SYNC_IN, then I think the name
> >>>> should be something like adi,sync-in-sync-out. SPI seems irrelevant here since
> >>>> we should just be describing how things are wired up, not how it is being used.
> >>>>
> >>>> But if we also need to consider the case where SYNC_OUT of one chip is connected
> >>>> to SYNC_IN of another chip, we might want to consider using trigger-source
> >>>> bindings instead (recently standardized in dtschema).
> >>>>     
> >>>
> >>> Do you mean the trigger-sources used for LEDs? I can try to see if it works, but would it
> >>> handle the non-GPIO case? While testing a multidevice setup, I found it simpler to 
> >>> have a single device to manage everything. It lets us toggle the GPIO or /SYNC_OUT
> >>> without referencing another device and makes simultaneous buffered reads easier.  
> >>
> >> Daisy-chain mode (figure 131)?  In that case we normally end up with a single presented device
> >> with a 'lot' of channels. (See the electric car style battery charging chips, those can
> >> be chained in very large numbers!)
> >>  
> > 
> > Actually, it is more like Figure 133 , but the premise is similar. We
> > have here a Quad setup.
> >   
> >> Probably similar for figure 133 (which is a dual SPI setup) as the SPI clock must
> >> be shared so we still see it over a single interface.
> >>  
> > 
> > We could view them as a single device with multiple channels, and since
> > the goal is to read them simultaneously with buffered reads, some parameters
> > such as sampling frequency should be equal to all devices.
> > 
> > However, there are some implications: If we do the above, we have
> > limitations in the customization of the "channels", they would have
> > the same filter, frequency modulator and scale (we plan to add support
> > for ADAQ776x-1 series, which include PGA and AAF gain).
> > 
> > To customize them separetely, we would need to assert only the
> > corresponding chip select, which is only possible with different
> > instances, as far as I know.  

Ah.  This is different from the daisy chain cases where even this
is done by writing through the single interface (usually you have
to update same register on all devices).

To support this they probably have to remain separate devices because
of how the SPI subystem will present them.  It's not impossible to
have multiple spi parents feed into a single child device but it is
complex. Or I guess we end up with something magic via a backend like
David describes below.

> 
> FYI, I've been discussing with the HDL folks at ADI about how we could make a
> multi-bus SPI controller, similar to controllers used for parallel SPI flash
> memories that are used as a single logical device. So that is the solution I
> am hoping for here. It would would allow a single IIO device instance for
> multiple chips. But the SPI controller would allow addressing individual chips
> for configuration and addressing all chips at the same time for reading sample
> data.

Maybe this could be presented as if it were a typical daisy chain.
So a long write sends the correct writes to each chip.  However, even
if we have HDL like this it isn't very general if someone wires this up
to a different HDL it will look quite different :(

I guess we don't have to keep to conventions of this looking like an SPI
a device given there is going to be a backend involved.

Nice to keep the bindings in line with conventional SPI though.

Jonathan
> 
> >   
> >> If those are the only two options then keeping this within the driver is fine.
> >> For daisy chain there are examples in tree and it normally means we have to
> >> have a DT parameter that says how long the chain is, though we maybe can
> >> do that with per channel nodes as well if those make sense here.
> >>
> >> Jonathan
> >>  
> > 
> > Those are the options in the datasheet and in hardware so far. I was 
> > considering other scenarios in case the user combine them differently.
> > I believe keping within the driver covers the main cases. 
> >   

Re: [PATCH v1 01/15] dt-bindings: iio: adc: ad7768-1: add synchronization over SPI property

[Jonathan Cameron]

On Mon, 13 Jan 2025 21:41:04 -0300
Jonathan Santos <jonath4nns@gmail.com> wrote:

> On 01/12, Jonathan Cameron wrote:
> > On Fri, 10 Jan 2025 18:51:06 -0300
> > Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote:
> >   
> > > On 01/07, Jonathan Santos wrote:  
> > > > Add adi,sync-in-spi property to enable synchronization over SPI.
> > > > This should be used in the case when the GPIO cannot provide a
> > > > pulse synchronous with the base MCLK signal.
> > > > 
> > > > User can choose between SPI, GPIO synchronization or neither of them,
> > > > but only if a external pulse can be provided, for example, by another
> > > > device in a multidevice setup.
> > > > 
> > > > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> > > > ---
> > > >  .../bindings/iio/adc/adi,ad7768-1.yaml        | 24 ++++++++++++++++++-
> > > >  1 file changed, 23 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
> > > > index 3ce59d4d065f..55cec27bfe60 100644
> > > > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
> > > > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
> > > > @@ -47,6 +47,15 @@ properties:
> > > >        in any way, for example if the filter decimation rate changes.
> > > >        As the line is active low, it should be marked GPIO_ACTIVE_LOW.
> > > >  
> > > > +  adi,sync-in-spi:
> > > > +    description:
> > > > +      Enables synchronization of multiple devices over SPI. This property is
> > > > +      used when a signal synchronous to the base MCLK signal cannot be provided
> > > > +      via GPIO. It requires the SYNC_OUT pin to be connected to the SYNC_IN pin
> > > > +      on the ADC. In the case of multiple devices, the SYNC_OUT pin of one device
> > > > +      should be routed to the SYNC_IN pins of the other devices.    
> > > So, if I'm getting it right,  
> > 
> > Datasheet on this is indeed complex!
> >   
> > >/SYNC_IN may be driven by a GPIO (ADAQ7768-1 datasheet Figure 131),  
> > 
> > For that we expose a gpio binding already. If that's present we know what is going on.
> >   
> > >/SYNC_IN may be driven by own device /SYNC_OUT (ADAQ7768-1 datasheet Figure 133),  
> > 
> > This is the default - no information provided so it isn't wired externally.
> > We don't normally bother to describe required chip to chip connections.
> > I couldn't entirely figure out if this is 'required' if we aren't driving explicitly
> > from GPIO or another chip but i think it is(?).
> >  
> 
> It is. If the device is not being driven by a GPIO or an external
> device, the user should connect the /SYNC_OUT to /SYNC_IN. We could
> assume that this is the case if the GPIO is not present in the
> devicetree, maybe put that into the description. The sync-out-sync-in
> property as a boolean can be quite redundant, since we also need to
> remove the GPIO.
> 
> > >/SYNC_IN may be driven by other AD7768-1 > /SYNC_OUT pin (also Figure 133).  
> > This is only case we are about for sync in I think.
> > 
> > As long as there isn't a valid 'not connected' It think we are fine with a boolean.
> >   
> 
> If opt for a single node instace, thats ok; otherwise, David's
> trigger-sources suggestion seems to be the best approach. 
That probably works on assumption that if no gpio, or trigger source etc
bindings are present then we assume the pins are wired together and it's
simple SPI triggering (so what we'd have if none of this complexity exists!)

Jonathan

> 
> > > That is too much to describe with a boolean.
> > > 
> > > If David's suggestion of using a trigger-source doesn't fit, this property
> > > should at least become an enum or string.
> > >   
> > > > +    type: boolean
> > > > +
> > > >    reset-gpios:
> > > >      maxItems: 1
> > > >  
> > > > @@ -65,7 +74,6 @@ required:
> > > >    - vref-supply
> > > >    - spi-cpol
> > > >    - spi-cpha
> > > > -  - adi,sync-in-gpios
> > > >  
> > > >  patternProperties:
> > > >    "^channel@([0-9]|1[0-5])$":
> > > > @@ -89,6 +97,20 @@ patternProperties:
> > > >  allOf:
> > > >    - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > > >  
> > > > +  # adi,sync-in-gpios and adi,sync-in-spi are mutually exclusive (neither is also valid)
> > > > +  - if:
> > > > +      required:
> > > > +        - adi,sync-in-gpios
> > > > +    then:
> > > > +      properties:
> > > > +        adi,sync-in-spi: false
> > > > +  - if:
> > > > +      required:
> > > > +        - adi,sync-in-spi
> > > > +    then:
> > > > +      properties:
> > > > +        adi,sync-in-gpios: false
> > > > +
> > > >  unevaluatedProperties: false
> > > >  
> > > >  examples:
> > > > -- 
> > > > 2.34.1
> > > >     
> >   

Re: [PATCH v1 05/15] iio: adc: ad7768-1: set MOSI idle state to high

[Jonathan Cameron]

On Mon, 13 Jan 2025 09:19:42 -0300
Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote:

> On 01/12, Jonathan Cameron wrote:
> > On Fri, 10 Jan 2025 18:56:26 -0300
> > Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote:
> >   
> > > On 01/07, David Lechner wrote:  
> > > > On 1/7/25 9:25 AM, Jonathan Santos wrote:    
> > > > > All supported parts require that the MOSI line stays high
> > > > > while in idle.
> > > > > 
> > > > > Configure SPI controller to set MOSI idle state to high.
> > > > > 
> > > > > Fixes: a5f8c7da3dbe ("iio: adc: Add AD7768-1 ADC basic support")
> > > > > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> > > > > ---    
> > > ...  
> > > > > @@ -574,6 +574,15 @@ static int ad7768_probe(struct spi_device *spi)
> > > > >  		return -ENOMEM;
> > > > >  
> > > > >  	st = iio_priv(indio_dev);
> > > > > +	/*
> > > > > +	 * The ADC SDI line must be kept high when
> > > > > +	 * data is not being clocked out of the controller.
> > > > > +	 * Request the SPI controller to make MOSI idle high.
> > > > > +	 */
> > > > > +	spi->mode |= SPI_MOSI_IDLE_HIGH;
> > > > > +	ret = spi_setup(spi);
> > > > > +	if (ret < 0)
> > > > > +		return ret;
> > > > >  	st->spi = spi;
> > > > >  
> > > > >  	st->vref = devm_regulator_get(&spi->dev, "vref");    
> > > > 
> > > > Very few SPI controllers currently have the SPI_MOSI_IDLE_HIGH capability flag
> > > > set in Linux right now (whether they actually support it or not), so this could
> > > > break existing users.    
> > > 
> > > Good point. Maybe only dev_warn() if SPI_MOSI_IDLE_HIGH is not supported?
> > >   
> > > >     
> > > ...  
> > > > 
> > > > If we ever do implement a data read of more than 64 bits without toggling CS,
> > > > then we could just set the TX data to be all 0xFF and have the same effect
> > > > without requiring the SPI controller to support SPI_MOSI_IDLE_HIGH.    
> > > 
> > > One point of having SPI_MOSI_IDLE_HIGH is that the controller may bring MOSI low
> > > between data words of a transfer. I think all transfer words are going to be
> > > either 16 or 24 with the new patches setting bits_per_word in all transfers but
> > > that might still not be enough if eventually the controller is unable to support
> > > those word sizes.   
> > 
> > Can we make the use of SPI_MOSI_IDLE_HIGH only apply if controller doesn't support
> > what is required to do the transfers in one go?  
> 
> I think so, but that would require tweaking spi controller drivers since we
> don't know at spi_setup() what transfers will ask for their bits_per_word.
> Not excited with this idea but may try something if that makes it easier to
> support these unusual SPI devices.

I'm confused. Here it is a client driver question I think. That driver knows what
it is asking for.  It can query if that word length is supported, if not query
if SPI_MOSI_IDLE_HIGH is possible and if neither fail to probe with suitable
error message. 

Jonathan


> 
> >   
> > > Plus you would have the complication of filling the tx_buf for
> > > all transfers.  
> > 
> > Wrap that up in a regmap, or read and write functions and that should be easy enough.
> >   
> > > 
> > > For the part that instigated the development of SPI_MOSI_IDLE_HIGH, the MOSI line
> > > also had to be high in between transfers. The diagrams at AD7768-1 datasheet
> > > page 51 suggest the same would be needed for this chip too.  
> > 
> > Whilst the datasheet indeed draws lines for that, i doubt it notices except on
> > clock transitions and between transfers the clock won't do anything.
> > If we confirm that the device does notice, then I don't mind limiting the controllers
> > to those with that can ensure it doesn't get set wrong.
> > 
> > Jonathan
> > 
> >   

Re: [PATCH v1 05/15] iio: adc: ad7768-1: set MOSI idle state to high

[Marcelo Schmitt]

On 01/18, Jonathan Cameron wrote:
> On Mon, 13 Jan 2025 09:19:42 -0300
> Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote:
> 
> > On 01/12, Jonathan Cameron wrote:
> > > On Fri, 10 Jan 2025 18:56:26 -0300
> > > Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote:
> > >   
> > > > On 01/07, David Lechner wrote:  
> > > > > On 1/7/25 9:25 AM, Jonathan Santos wrote:    
> > > > > > All supported parts require that the MOSI line stays high
> > > > > > while in idle.
> > > > > > 
> > > > > > Configure SPI controller to set MOSI idle state to high.
> > > > > > 
> > > > > > Fixes: a5f8c7da3dbe ("iio: adc: Add AD7768-1 ADC basic support")
> > > > > > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> > > > > > ---    
> > > > ...  
> > > > > > @@ -574,6 +574,15 @@ static int ad7768_probe(struct spi_device *spi)
> > > > > >  		return -ENOMEM;
> > > > > >  
> > > > > >  	st = iio_priv(indio_dev);
> > > > > > +	/*
> > > > > > +	 * The ADC SDI line must be kept high when
> > > > > > +	 * data is not being clocked out of the controller.
> > > > > > +	 * Request the SPI controller to make MOSI idle high.
> > > > > > +	 */
> > > > > > +	spi->mode |= SPI_MOSI_IDLE_HIGH;
> > > > > > +	ret = spi_setup(spi);
> > > > > > +	if (ret < 0)
> > > > > > +		return ret;
> > > > > >  	st->spi = spi;
> > > > > >  
> > > > > >  	st->vref = devm_regulator_get(&spi->dev, "vref");    
> > > > > 
> > > > > Very few SPI controllers currently have the SPI_MOSI_IDLE_HIGH capability flag
> > > > > set in Linux right now (whether they actually support it or not), so this could
> > > > > break existing users.    
> > > > 
> > > > Good point. Maybe only dev_warn() if SPI_MOSI_IDLE_HIGH is not supported?
> > > >   
> > > > >     
> > > > ...  
> > > > > 
> > > > > If we ever do implement a data read of more than 64 bits without toggling CS,
> > > > > then we could just set the TX data to be all 0xFF and have the same effect
> > > > > without requiring the SPI controller to support SPI_MOSI_IDLE_HIGH.    
> > > > 
> > > > One point of having SPI_MOSI_IDLE_HIGH is that the controller may bring MOSI low
> > > > between data words of a transfer. I think all transfer words are going to be
> > > > either 16 or 24 with the new patches setting bits_per_word in all transfers but
> > > > that might still not be enough if eventually the controller is unable to support
> > > > those word sizes.   
> > > 
> > > Can we make the use of SPI_MOSI_IDLE_HIGH only apply if controller doesn't support
> > > what is required to do the transfers in one go?  
> > 
> > I think so, but that would require tweaking spi controller drivers since we
> > don't know at spi_setup() what transfers will ask for their bits_per_word.
> > Not excited with this idea but may try something if that makes it easier to
> > support these unusual SPI devices.
> 
> I'm confused. Here it is a client driver question I think. That driver knows what
> it is asking for.  It can query if that word length is supported, if not query
> if SPI_MOSI_IDLE_HIGH is possible and if neither fail to probe with suitable
> error message. 
> 
> Jonathan

Ah yes, I think that would be a better way to go. I thought your previous
question was about making SPI_MOSI_IDLE_HIGH support within the SPI subsystem
only apply if the controller couldn't support all of the bits_per_word an ADC
driver would want for it's transfers. Sorry for the confusion.

I'm still a bit skeptical about whether the device really works without
SPI_MOSI_IDLE_HIGH. Though, if setting proper bits_per_word is enough then
that's great because it will allow the device to work with a wider range of
controllers.

Marcelo

> 
> 
> > 
> > >   
> > > > Plus you would have the complication of filling the tx_buf for
> > > > all transfers.  
> > > 
> > > Wrap that up in a regmap, or read and write functions and that should be easy enough.
> > >   
> > > > 
> > > > For the part that instigated the development of SPI_MOSI_IDLE_HIGH, the MOSI line
> > > > also had to be high in between transfers. The diagrams at AD7768-1 datasheet
> > > > page 51 suggest the same would be needed for this chip too.  
> > > 
> > > Whilst the datasheet indeed draws lines for that, i doubt it notices except on
> > > clock transitions and between transfers the clock won't do anything.
> > > If we confirm that the device does notice, then I don't mind limiting the controllers
> > > to those with that can ensure it doesn't get set wrong.
> > > 
> > > Jonathan
> > > 
> > >   
>