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

[PATCH v6 00/11] 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 output
configuration and GPIO support. Additionally, this includes fixes 
for SPI communication and for IIO interface, and also code improvements
to enhance maintainability and readability.

---
Changes in v6:
* Changed description and addressed other nits in the gpio-trigger patch.
* Rewrote the #trigger-sources-cells description and removed mentions 
  to offload engine.
* Added adi,ad7768-1.h header with macros for the trigger source cells.
* removed of_match_ptr() from regulator_desc.
* Replaced deprecated .set callback with .set_rv in the gpio controller
  patch.
* Use `trigger-sources` as an alternative to `adi,sync-in-gpios`
  (now optional), instead of replacing it.
* Check trigger source by the compatible string (and the dev node for the
  self triggering).
* Addressed review comments, see individual pacthes.
* Link to v5: https://lore.kernel.org/linux-iio/cover.1744325346.git.Jonathan.Santos@analog.com/T/#t 

Changes in v5:
* Added gpio-trigger binding patch.
* Include START pin and DRDY in the trigger-sources description.
* increased trigger-source-cells to 1: this cell will define the trigger
  source type.
* Fixed the holes in the regmap ranges.
* replace old iio_device_claim_direct_mode() for the new 
  iio_device_claim/release_direct() functions.
* Changed some commit messages.
* Link to v4: https://lore.kernel.org/linux-iio/cover.1741268122.git.Jonathan.Santos@analog.com/T/#t

Changes in v4:
* Added missing `select REGMAP_SPI` and `select REGULATOR` to the device's Kconfig.
* VCM output regulator property renamed.
* Added direct mode conditional locks to regulator controller callbacks.
* Renamed regulator controller.
* Created helper function to precalculate the sampling frequency table and avoid
  race conditions.
* Link to v3: https://lore.kernel.org/linux-iio/cover.1739368121.git.Jonathan.Santos@analog.com/T/#t

Changes in v3:
* Fixed irregular or missing SoBs.
* Moved MOSI idle state patch to the start of the patch, as the other fix.
* fixed dt-binding errors.
* Trigger-sources is handled in a different way, as an alternative to sync-in-gpio.
  (this way we avoid breaking old applications).
* VCM output is controlled by the regulator framework.
* Added a second regmap for 24-bit register values.
* Add new preparatory patch replacing the manual attribute declarations for
  the read_avail from struct iio_info.
* included sinc3+rej60 filter type.
* Addressed review comments, see individual pacthes.
* Link to v2: https://lore.kernel.org/linux-iio/cover.1737985435.git.Jonathan.Santos@analog.com/T/#u

Changes in v2:
* Removed synchronization over SPI property and replaced it for trigger-sources.
* Added GPIO controller documentation.
* VCM output control changed from an IIO attribute to a devicetree property (static value).
* Converted driver to use regmap and dropped spi_read_reg and spi_write_reg pacthes.
* replaced decimation_rate attribute for oversampling_ratio and dropped device specific documentation patch.
* Added low pass -3dB cutoff attribute.
* Addressed review comments, see individual pacthes.
* Link to v1: https://lore.kernel.org/linux-iio/cover.1736201898.git.Jonathan.Santos@analog.com/T/#t

Jonathan Santos (10):
  dt-bindings: trigger-source: add generic GPIO trigger source
  dt-bindings: iio: adc: ad7768-1: add trigger-sources property
  dt-bindings: iio: adc: ad7768-1: Document GPIO controller
  dt-bindings: iio: adc: ad7768-1: document regulator provider property
  iio: adc: ad7768-1: add regulator to control VCM output
  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: replace manual attribute declaration
  iio: adc: ad7768-1: add filter type and oversampling ratio attributes
  iio: adc: ad7768-1: add low pass -3dB cutoff attribute

Sergiu Cuciurean (1):
  iio: adc: ad7768-1: Add GPIO controller support

 .../bindings/iio/adc/adi,ad7768-1.yaml        |  60 +-
 .../bindings/trigger-source/gpio-trigger.yaml |  40 +
 drivers/iio/adc/Kconfig                       |   1 +
 drivers/iio/adc/ad7768-1.c                    | 918 ++++++++++++++++--
 include/dt-bindings/iio/adc/adi,ad7768-1.h    |  10 +
 5 files changed, 928 insertions(+), 101 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/trigger-source/gpio-trigger.yaml
 create mode 100644 include/dt-bindings/iio/adc/adi,ad7768-1.h


base-commit: 9415c8b5b9b7ba927d98f80022a2890e8639e9e4
prerequisite-patch-id: fbb33747cd0293bacd5b6d801d6cfc087449a28e
-- 
2.34.1

[PATCH v6 01/11] dt-bindings: trigger-source: add generic GPIO trigger source

[Jonathan Santos]

Inspired by pwm-trigger, create a new binding for using a GPIO
line as a trigger source.

Link: https://lore.kernel.org/linux-iio/20250207-dlech-mainline-spi-engine-offload-2-v8-3-e48a489be48c@baylibre.com/
Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
---
v6 Changes:
* Changed description.
* Fixed typos and replaced GPIO pin with GPIO line.
* Added link reference for pwm-trigger.

v5 Changes:
* New patch in v5.
---
 .../bindings/trigger-source/gpio-trigger.yaml | 40 +++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/trigger-source/gpio-trigger.yaml

diff --git a/Documentation/devicetree/bindings/trigger-source/gpio-trigger.yaml b/Documentation/devicetree/bindings/trigger-source/gpio-trigger.yaml
new file mode 100644
index 000000000000..1331d153ee82
--- /dev/null
+++ b/Documentation/devicetree/bindings/trigger-source/gpio-trigger.yaml
@@ -0,0 +1,40 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/trigger-source/gpio-trigger.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Generic trigger source using GPIO
+
+description: A GPIO used as a trigger source.
+
+maintainers:
+  - Jonathan Santos <Jonathan.Santos@analog.com>
+
+properties:
+  compatible:
+    const: gpio-trigger
+
+  '#trigger-source-cells':
+    const: 0
+
+  gpios:
+    maxItems: 1
+    description: GPIO to be used as a trigger source.
+
+required:
+  - compatible
+  - '#trigger-source-cells'
+  - gpios
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    trigger {
+        compatible = "gpio-trigger";
+        #trigger-source-cells = <0>;
+        gpios = <&gpio1 15 GPIO_ACTIVE_HIGH>;
+    };
-- 
2.34.1

[PATCH v6 02/11] dt-bindings: iio: adc: ad7768-1: add trigger-sources property

[Jonathan Santos]

In addition to GPIO synchronization, The AD7768-1 also supports
synchronization over SPI, which use is recommended when the GPIO
cannot provide a pulse synchronous with the base MCLK signal. It
consists of looping back the SYNC_OUT to the SYNC_IN pin and send
a command via SPI to trigger the synchronization.

Introduce the 'trigger-sources' property to enable SPI-based
synchronization via SYNC_OUT pin, along with additional optional
entries for GPIO3 and DRDY pins.

Also create #trigger-source-cells property to differentiate the trigger
sources provided by the ADC. To improve readability, create a
adi,ad7768-1.h header with the macros for the cell values.

While at it, add description to the interrupts property.

Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
---
v6 Changes:
* Removed references to offload.
* Changed trigger-sources-cells description. Each cell value indicates
  a gpio line from the ADC.
* Added adi,ad7768-1.h header with macros for the trigger source cells.
* Removed offload trigger entry from trigger-sources.

v5 Changes:
* Include START pin and DRDY in the trigger-sources description.
* Fixed "#trigger-source-cells" value and description.
* sync-in-gpios is represented in the trigger-sources property.

v4 Changes:
* none

v3 Changes:
* Fixed dt-bindings errors.
* Trigger-source is set as an alternative to sync-in-gpios, so we
  don't break the previous ABI.
* increased maxItems from trigger-sources to 2.

v2 Changes:
* Patch added as replacement for adi,sync-in-spi patch.
* addressed the request for a description to interrupts property.
---
 .../bindings/iio/adc/adi,ad7768-1.yaml        | 31 ++++++++++++++++++-
 include/dt-bindings/iio/adc/adi,ad7768-1.h    | 10 ++++++
 2 files changed, 40 insertions(+), 1 deletion(-)
 create mode 100644 include/dt-bindings/iio/adc/adi,ad7768-1.h

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
index 3ce59d4d065f..1f476aa15305 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
@@ -26,7 +26,28 @@ properties:
   clock-names:
     const: mclk
 
+  trigger-sources:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    minItems: 1
+    maxItems: 2
+    description: |
+      A list of phandles referencing trigger source providers. Each entry
+      represents a trigger source for the ADC:
+
+        - First entry specifies the device responsible for driving the
+          synchronization (SYNC_IN) pin, as an alternative to adi,sync-in-gpios.
+          This can be a `gpio-trigger` or another `ad7768-1` device. If the
+          device's own SYNC_OUT pin is internally connected to its SYNC_IN pin,
+          reference the device itself or omit this property.
+        - Second entry optionally defines a GPIO3 pin used as a START signal trigger.
+
+      Use the accompanying trigger source cell to identify the type of each entry.
+
   interrupts:
+    description:
+      Specifies the interrupt line associated with the ADC. This refers
+      to the DRDY (Data Ready) pin, which signals when conversion results are
+      available.
     maxItems: 1
 
   '#address-cells':
@@ -57,6 +78,15 @@ properties:
   "#io-channel-cells":
     const: 1
 
+  "#trigger-source-cells":
+    description: |
+      Cell indicates the trigger output signal: 0 = SYNC_OUT, 1 = GPIO3,
+      2 = DRDY.
+
+      For better readability, macros for these values are available in
+      dt-bindings/iio/adc/adi,ad7768-1.h.
+    const: 1
+
 required:
   - compatible
   - reg
@@ -65,7 +95,6 @@ required:
   - vref-supply
   - spi-cpol
   - spi-cpha
-  - adi,sync-in-gpios
 
 patternProperties:
   "^channel@([0-9]|1[0-5])$":
diff --git a/include/dt-bindings/iio/adc/adi,ad7768-1.h b/include/dt-bindings/iio/adc/adi,ad7768-1.h
new file mode 100644
index 000000000000..34d92856a50b
--- /dev/null
+++ b/include/dt-bindings/iio/adc/adi,ad7768-1.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+
+#ifndef _DT_BINDINGS_ADI_AD7768_1_H
+#define _DT_BINDINGS_ADI_AD7768_1_H
+
+#define AD7768_TRIGGER_SOURCE_SYNC_OUT  0
+#define AD7768_TRIGGER_SOURCE_GPIO3     1
+#define AD7768_TRIGGER_SOURCE_DRDY      2
+
+#endif /* _DT_BINDINGS_ADI_AD7768_1_H */
-- 
2.34.1

[PATCH v6 03/11] dt-bindings: iio: adc: ad7768-1: Document GPIO controller

[Jonathan Santos]

The AD7768-1 ADC exports four bidirectional GPIOs accessible
via register map.

Document GPIO properties necessary to enable GPIO controller for this
device.

Acked-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
---
v6 Changes:
* none.

v5 Changes:
* none.

v4 Changes:
* none.

v3 Changes:
* none.

v2 Changes:
* New patch in v2.
---
 .../devicetree/bindings/iio/adc/adi,ad7768-1.yaml      | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
index 1f476aa15305..25d4995c63a5 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
@@ -87,6 +87,14 @@ properties:
       dt-bindings/iio/adc/adi,ad7768-1.h.
     const: 1
 
+  gpio-controller: true
+
+  "#gpio-cells":
+    const: 2
+    description: |
+      The first cell is for the GPIO number: 0 to 3.
+      The second cell takes standard GPIO flags.
+
 required:
   - compatible
   - reg
@@ -134,6 +142,8 @@ examples:
             spi-max-frequency = <2000000>;
             spi-cpol;
             spi-cpha;
+            gpio-controller;
+            #gpio-cells = <2>;
             vref-supply = <&adc_vref>;
             interrupts = <25 IRQ_TYPE_EDGE_RISING>;
             interrupt-parent = <&gpio>;
-- 
2.34.1

[PATCH v6 04/11] dt-bindings: iio: adc: ad7768-1: document regulator provider property

[Jonathan Santos]

The AD7768-1 provides a buffered common-mode voltage output
on the VCM pin that can be used to bias analog input signals.

Add regulators property to enable the use of the VCM output,
referenced here as vcm-output, by any other device.

Acked-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
---
v6 Changes:
* None.

v5 Changes:
* removed `regulator-min-microvolt` and `regulator-max-microvolt`. 

v4 Changes:
* replace "vcm_output" property name for "vcm-output". 

v3 Changes:
* VCM is now provided as a regulator within the device, instead of a 
  custom property.

v2 Changes:
* New patch in v2.
---
 .../bindings/iio/adc/adi,ad7768-1.yaml        | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
index 25d4995c63a5..5083ee7c0256 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
@@ -68,6 +68,19 @@ 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.
 
+  regulators:
+    type: object
+    description:
+      list of regulators provided by this controller.
+
+    properties:
+      vcm-output:
+        $ref: /schemas/regulator/regulator.yaml#
+        type: object
+        unevaluatedProperties: false
+
+    additionalProperties: false
+
   reset-gpios:
     maxItems: 1
 
@@ -159,6 +172,12 @@ examples:
                 reg = <0>;
                 label = "channel_0";
             };
+
+            regulators {
+              vcm_reg: vcm-output {
+                regulator-name = "ad7768-1-vcm";
+              };
+            };
         };
     };
 ...
-- 
2.34.1

[PATCH v6 05/11] iio: adc: ad7768-1: add regulator to control VCM output

[Jonathan Santos]

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

This change allows the user to configure VCM output using the regulator
framework.

Acked-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
---
v6 Changes:
* Rearranged iio_device_release_direct() calls to avoid
 some gotos.
* removed of_match_ptr() from regulator_desc.
* Addressed other nits.

v5 Changes:
* enforce AD7768_REG_ANALOG2_VCM macro argument evaluation.
* switched to the new iio_device_claim/release_direct() functions.

v4 Changes:
* Added iio_device_claim_direct_mode() to regulator callbacks to avoid register access
  while in buffered mode.
* Changed regulator name to "ad7768-1-vcm".
* When regulator enable is called, it will set the last voltage selector configured.
* Disabled regulator before configuring it.
* Addressed other nits.

v3 Changes:
* Register VCM output via the regulator framework for improved flexibility
  and external integration.

v2 Changes:
* VCM output support is now defined by a devicetree property, instead of 
  and IIO attribute.
---
 drivers/iio/adc/Kconfig    |   1 +
 drivers/iio/adc/ad7768-1.c | 173 +++++++++++++++++++++++++++++++++++++
 2 files changed, 174 insertions(+)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index ad06cf556785..8dc4cc4c25af 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -329,6 +329,7 @@ config AD7766
 config AD7768_1
 	tristate "Analog Devices AD7768-1 ADC driver"
 	depends on SPI
+	select REGULATOR
 	select REGMAP_SPI
 	select IIO_BUFFER
 	select IIO_TRIGGER
diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
index 09e4ab76e2b6..0364d73329b0 100644
--- a/drivers/iio/adc/ad7768-1.c
+++ b/drivers/iio/adc/ad7768-1.c
@@ -12,8 +12,10 @@
 #include <linux/gpio/consumer.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
+#include <linux/regulator/driver.h>
 #include <linux/sysfs.h>
 #include <linux/spi/spi.h>
 
@@ -80,6 +82,12 @@
 #define AD7768_CONV_MODE_MSK		GENMASK(2, 0)
 #define AD7768_CONV_MODE(x)		FIELD_PREP(AD7768_CONV_MODE_MSK, x)
 
+/* AD7768_REG_ANALOG2 */
+#define AD7768_REG_ANALOG2_VCM_MSK	GENMASK(2, 0)
+#define AD7768_REG_ANALOG2_VCM(x)	FIELD_PREP(AD7768_REG_ANALOG2_VCM_MSK, (x))
+
+#define AD7768_VCM_OFF			0x07
+
 enum ad7768_conv_mode {
 	AD7768_CONTINUOUS,
 	AD7768_ONE_SHOT,
@@ -157,6 +165,8 @@ struct ad7768_state {
 	struct regmap *regmap;
 	struct regmap *regmap24;
 	struct regulator *vref;
+	struct regulator_dev *vcm_rdev;
+	unsigned int vcm_output_sel;
 	struct clk *mclk;
 	unsigned int mclk_freq;
 	unsigned int samp_freq;
@@ -644,6 +654,164 @@ static int ad7768_triggered_buffer_alloc(struct iio_dev *indio_dev)
 					       &ad7768_buffer_ops);
 }
 
+static int ad7768_vcm_enable(struct regulator_dev *rdev)
+{
+	struct iio_dev *indio_dev = rdev_get_drvdata(rdev);
+	struct ad7768_state *st = iio_priv(indio_dev);
+	int ret, regval;
+
+	if (!indio_dev)
+		return -EINVAL;
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+
+	/* To enable, set the last selected output */
+	regval = AD7768_REG_ANALOG2_VCM(st->vcm_output_sel + 1);
+	ret = regmap_update_bits(st->regmap, AD7768_REG_ANALOG2,
+				 AD7768_REG_ANALOG2_VCM_MSK, regval);
+	iio_device_release_direct(indio_dev);
+
+	return ret;
+}
+
+static int ad7768_vcm_disable(struct regulator_dev *rdev)
+{
+	struct iio_dev *indio_dev = rdev_get_drvdata(rdev);
+	struct ad7768_state *st = iio_priv(indio_dev);
+	int ret;
+
+	if (!indio_dev)
+		return -EINVAL;
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+
+	ret = regmap_update_bits(st->regmap, AD7768_REG_ANALOG2,
+				 AD7768_REG_ANALOG2_VCM_MSK, AD7768_VCM_OFF);
+	iio_device_release_direct(indio_dev);
+
+	return ret;
+}
+
+static int ad7768_vcm_is_enabled(struct regulator_dev *rdev)
+{
+	struct iio_dev *indio_dev = rdev_get_drvdata(rdev);
+	struct ad7768_state *st = iio_priv(indio_dev);
+	int ret, val;
+
+	if (!indio_dev)
+		return -EINVAL;
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+
+	ret = regmap_read(st->regmap, AD7768_REG_ANALOG2, &val);
+	iio_device_release_direct(indio_dev);
+	if (ret)
+		return ret;
+
+	return FIELD_GET(AD7768_REG_ANALOG2_VCM_MSK, val) != AD7768_VCM_OFF;
+}
+
+static int ad7768_set_voltage_sel(struct regulator_dev *rdev,
+				  unsigned int selector)
+{
+	unsigned int regval = AD7768_REG_ANALOG2_VCM(selector + 1);
+	struct iio_dev *indio_dev = rdev_get_drvdata(rdev);
+	struct ad7768_state *st = iio_priv(indio_dev);
+	int ret;
+
+	if (!indio_dev)
+		return -EINVAL;
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+
+	ret = regmap_update_bits(st->regmap, AD7768_REG_ANALOG2,
+				 AD7768_REG_ANALOG2_VCM_MSK, regval);
+	iio_device_release_direct(indio_dev);
+	if (ret)
+		return ret;
+
+	st->vcm_output_sel = selector;
+
+	return 0;
+}
+
+static int ad7768_get_voltage_sel(struct regulator_dev *rdev)
+{
+	struct iio_dev *indio_dev = rdev_get_drvdata(rdev);
+	struct ad7768_state *st = iio_priv(indio_dev);
+	int ret, val;
+
+	if (!indio_dev)
+		return -EINVAL;
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+
+	ret = regmap_read(st->regmap, AD7768_REG_ANALOG2, &val);
+	iio_device_release_direct(indio_dev);
+	if (ret)
+		return ret;
+
+	val = FIELD_GET(AD7768_REG_ANALOG2_VCM_MSK, val);
+	return clamp(val, 1, (int)rdev->desc->n_voltages) - 1;
+}
+
+static const struct regulator_ops vcm_regulator_ops = {
+	.enable = ad7768_vcm_enable,
+	.disable = ad7768_vcm_disable,
+	.is_enabled = ad7768_vcm_is_enabled,
+	.list_voltage = regulator_list_voltage_table,
+	.set_voltage_sel = ad7768_set_voltage_sel,
+	.get_voltage_sel = ad7768_get_voltage_sel,
+};
+
+static const unsigned int vcm_voltage_table[] = {
+	2500000,
+	2050000,
+	1650000,
+	1900000,
+	1100000,
+	900000,
+};
+
+static const struct regulator_desc vcm_desc = {
+	.name = "ad7768-1-vcm",
+	.of_match = "vcm-output",
+	.regulators_node = "regulators",
+	.n_voltages = ARRAY_SIZE(vcm_voltage_table),
+	.volt_table = vcm_voltage_table,
+	.ops = &vcm_regulator_ops,
+	.type = REGULATOR_VOLTAGE,
+	.owner = THIS_MODULE,
+};
+
+static int ad7768_register_regulators(struct device *dev, struct ad7768_state *st,
+				      struct iio_dev *indio_dev)
+{
+	struct regulator_config config = {
+		.dev = dev,
+		.driver_data = indio_dev,
+	};
+	int ret;
+
+	/* Disable the regulator before registering it */
+	ret = regmap_update_bits(st->regmap, AD7768_REG_ANALOG2,
+				 AD7768_REG_ANALOG2_VCM_MSK, AD7768_VCM_OFF);
+	if (ret)
+		return ret;
+
+	st->vcm_rdev = devm_regulator_register(dev, &vcm_desc, &config);
+	if (IS_ERR(st->vcm_rdev))
+		return dev_err_probe(dev, PTR_ERR(st->vcm_rdev),
+				     "failed to register VCM regulator\n");
+
+	return 0;
+}
+
 static int ad7768_probe(struct spi_device *spi)
 {
 	struct ad7768_state *st;
@@ -708,6 +876,11 @@ static int ad7768_probe(struct spi_device *spi)
 	indio_dev->info = &ad7768_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
+	/* Register VCM output regulator */
+	ret = ad7768_register_regulators(&spi->dev, st, indio_dev);
+	if (ret)
+		return ret;
+
 	ret = ad7768_setup(st);
 	if (ret < 0) {
 		dev_err(&spi->dev, "AD7768 setup failed\n");
-- 
2.34.1

[PATCH v6 06/11] 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.

Add direct mode conditional locks in the gpio callbacks to prevent register
access when the device is in buffered mode.

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

Reviewed-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
Acked-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
Co-developed-by: Jonathan Santos <Jonathan.Santos@analog.com>
Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
---
v6 Changes:
* Replaced deprecated .set callback with .set_rv.

v5 Changes:
* Use the new new iio_device_claim/release_direct() functions.
* replaced gpiochip_add_data() for devm_gpiochip_add_data().

v4 Changes:
* Mentioned in the commit message that we cannot tweak the GPIO controller
  when the device is not in direct mode.

v3 Changes:
* Fixed SoB order.
* Added missing iio_device_release_direct_mode().
* Simplified some regmap writes.
* Removed ad7768_gpio_request() callback.
* Fixed line wrapping.

v2 Changes:
* Replaced mutex for iio_device_claim_direct_mode().
* Use gpio-controller property to conditionally enable the
  GPIO support.
* OBS: when the GPIO is configured as output, we should read 
  the current state value from AD7768_REG_GPIO_WRITE.
---
 drivers/iio/adc/ad7768-1.c | 141 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 139 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
index 0364d73329b0..1a546a0dc654 100644
--- a/drivers/iio/adc/ad7768-1.c
+++ b/drivers/iio/adc/ad7768-1.c
@@ -9,6 +9,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>
@@ -86,6 +88,16 @@
 #define AD7768_REG_ANALOG2_VCM_MSK	GENMASK(2, 0)
 #define AD7768_REG_ANALOG2_VCM(x)	FIELD_PREP(AD7768_REG_ANALOG2_VCM_MSK, (x))
 
+/* AD7768_REG_GPIO_CONTROL */
+#define AD7768_GPIO_UNIVERSAL_EN	BIT(7)
+#define AD7768_GPIO_CONTROL_MSK		GENMASK(3, 0)
+
+/* 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_VCM_OFF			0x07
 
 enum ad7768_conv_mode {
@@ -168,6 +180,7 @@ struct ad7768_state {
 	struct regulator_dev *vcm_rdev;
 	unsigned int vcm_output_sel;
 	struct clk *mclk;
+	struct gpio_chip gpiochip;
 	unsigned int mclk_freq;
 	unsigned int samp_freq;
 	struct completion completion;
@@ -353,6 +366,122 @@ 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 iio_dev *indio_dev = gpiochip_get_data(chip);
+	struct ad7768_state *st = iio_priv(indio_dev);
+	int ret;
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+
+	ret = regmap_clear_bits(st->regmap, AD7768_REG_GPIO_CONTROL,
+				BIT(offset));
+	iio_device_release_direct(indio_dev);
+
+	return ret;
+}
+
+static int ad7768_gpio_direction_output(struct gpio_chip *chip,
+					unsigned int offset, int value)
+{
+	struct iio_dev *indio_dev = gpiochip_get_data(chip);
+	struct ad7768_state *st = iio_priv(indio_dev);
+	int ret;
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+
+	ret = regmap_set_bits(st->regmap, AD7768_REG_GPIO_CONTROL,
+			      BIT(offset));
+	iio_device_release_direct(indio_dev);
+
+	return ret;
+}
+
+static int ad7768_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct iio_dev *indio_dev = gpiochip_get_data(chip);
+	struct ad7768_state *st = iio_priv(indio_dev);
+	unsigned int val;
+	int ret;
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+
+	ret = regmap_read(st->regmap, AD7768_REG_GPIO_CONTROL, &val);
+	if (ret)
+		goto err_release;
+
+	/*
+	 * If the GPIO is configured as an output, read the current value from
+	 * AD7768_REG_GPIO_WRITE. Otherwise, read the input value from
+	 * AD7768_REG_GPIO_READ.
+	 */
+	if (val & BIT(offset))
+		ret = regmap_read(st->regmap, AD7768_REG_GPIO_WRITE, &val);
+	else
+		ret = regmap_read(st->regmap, AD7768_REG_GPIO_READ, &val);
+	if (ret)
+		goto err_release;
+
+	ret = !!(val & BIT(offset));
+err_release:
+	iio_device_release_direct(indio_dev);
+
+	return ret;
+}
+
+static int ad7768_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
+{
+	struct iio_dev *indio_dev = gpiochip_get_data(chip);
+	struct ad7768_state *st = iio_priv(indio_dev);
+	unsigned int val;
+	int ret;
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+
+	ret = regmap_read(st->regmap, AD7768_REG_GPIO_CONTROL, &val);
+	if (ret)
+		goto err_release;
+
+	if (val & BIT(offset))
+		ret = regmap_update_bits(st->regmap, AD7768_REG_GPIO_WRITE,
+					 BIT(offset), value << offset);
+
+err_release:
+	iio_device_release_direct(indio_dev);
+
+	return ret;
+}
+
+static int ad7768_gpio_init(struct iio_dev *indio_dev)
+{
+	struct ad7768_state *st = iio_priv(indio_dev);
+	int ret;
+
+	ret = regmap_write(st->regmap, AD7768_REG_GPIO_CONTROL,
+			   AD7768_GPIO_UNIVERSAL_EN);
+	if (ret)
+		return ret;
+
+	st->gpiochip = (struct gpio_chip) {
+		.label = "ad7768_1_gpios",
+		.base = -1,
+		.ngpio = 4,
+		.parent = &st->spi->dev,
+		.can_sleep = true,
+		.direction_input = ad7768_gpio_direction_input,
+		.direction_output = ad7768_gpio_direction_output,
+		.get = ad7768_gpio_get,
+		.set_rv = ad7768_gpio_set,
+		.owner = THIS_MODULE,
+	};
+
+	return devm_gpiochip_add_data(&st->spi->dev, &st->gpiochip, indio_dev);
+}
+
 static int ad7768_set_freq(struct ad7768_state *st,
 			   unsigned int freq)
 {
@@ -494,8 +623,9 @@ 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 iio_dev *indio_dev)
 {
+	struct ad7768_state *st = iio_priv(indio_dev);
 	int ret;
 
 	st->gpio_reset = devm_gpiod_get_optional(&st->spi->dev, "reset",
@@ -528,6 +658,13 @@ static int ad7768_setup(struct ad7768_state *st)
 	if (IS_ERR(st->gpio_sync_in))
 		return PTR_ERR(st->gpio_sync_in);
 
+	/* Only create a Chip GPIO if flagged for it */
+	if (device_property_read_bool(&st->spi->dev, "gpio-controller")) {
+		ret = ad7768_gpio_init(indio_dev);
+		if (ret < 0)
+			return ret;
+	}
+
 	/* Set the default sampling frequency to 32000 kSPS */
 	return ad7768_set_freq(st, 32000);
 }
@@ -881,7 +1018,7 @@ static int ad7768_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
-	ret = ad7768_setup(st);
+	ret = ad7768_setup(indio_dev);
 	if (ret < 0) {
 		dev_err(&spi->dev, "AD7768 setup failed\n");
 		return ret;
-- 
2.34.1

[PATCH v6 07/11] iio: adc: ad7768-1: add multiple scan types to support 16-bits mode

[Jonathan Santos]

When the device is configured to decimation x8, only possible in the
sinc5 filter, 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.

Reviewed-by: David Lechner <dlechner@baylire.com>
Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
---
v6 Changes:
* None.

v5 Changes:
* None.

v4 Changes:
* None.

v3 Changes:
* Decreased storagebits to 16 for AD7768_SCAN_TYPE_HIGH_SPEED
  scan type.

v2 Changes:
* Included the ".shift" value back to scan_type.
* Changed the number of bytes from regmap_read instead of shifting the 
  ADC sample value when the word size is lower (16-bits).
---
 drivers/iio/adc/ad7768-1.c | 74 ++++++++++++++++++++++++++++++++------
 1 file changed, 64 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
index 1a546a0dc654..087478afb5bf 100644
--- a/drivers/iio/adc/ad7768-1.c
+++ b/drivers/iio/adc/ad7768-1.c
@@ -139,6 +139,15 @@ struct ad7768_clk_configuration {
 	enum ad7768_pwrmode pwrmode;
 };
 
+enum ad7768_scan_type {
+	AD7768_SCAN_TYPE_NORMAL,
+	AD7768_SCAN_TYPE_HIGH_SPEED,
+};
+
+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 },
@@ -153,6 +162,22 @@ 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,
+		.shift = 8,
+		.endianness = IIO_BE,
+	},
+	[AD7768_SCAN_TYPE_HIGH_SPEED] = {
+		.sign = 's',
+		.realbits = 16,
+		.storagebits = 16,
+		.endianness = IIO_BE,
+	},
+};
+
 static const struct iio_chan_spec ad7768_channels[] = {
 	{
 		.type = IIO_VOLTAGE,
@@ -162,13 +187,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),
 	},
 };
 
@@ -182,6 +203,7 @@ struct ad7768_state {
 	struct clk *mclk;
 	struct gpio_chip gpiochip;
 	unsigned int mclk_freq;
+	unsigned int dec_rate;
 	unsigned int samp_freq;
 	struct completion completion;
 	struct iio_trigger *trig;
@@ -300,6 +322,15 @@ static int ad7768_scan_direct(struct iio_dev *indio_dev)
 	if (ret)
 		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 >>= 8;
+
 	/*
 	 * Any SPI configuration of the AD7768-1 can only be
 	 * performed in continuous conversion mode.
@@ -516,6 +547,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);
 
@@ -549,8 +582,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:
 		if (!iio_device_claim_direct(indio_dev))
@@ -561,7 +599,7 @@ static int ad7768_read_raw(struct iio_dev *indio_dev,
 		iio_device_release_direct(indio_dev);
 		if (ret < 0)
 			return ret;
-		*val = sign_extend32(ret, chan->scan_type.realbits - 1);
+		*val = sign_extend32(ret, scan_type->realbits - 1);
 
 		return IIO_VAL_INT;
 
@@ -571,7 +609,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;
 
@@ -615,11 +653,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,
 };
 
@@ -674,9 +722,15 @@ static irqreturn_t ad7768_trigger_handler(int irq, void *p)
 	struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct ad7768_state *st = iio_priv(indio_dev);
+	const struct iio_scan_type *scan_type;
 	int ret;
 
-	ret = spi_read(st->spi, &st->data.scan.chan, 3);
+	scan_type = iio_get_current_scan_type(indio_dev, &indio_dev->channels[0]);
+	if (IS_ERR(scan_type))
+		return PTR_ERR(scan_type);
+
+	ret = spi_read(st->spi, &st->data.scan.chan,
+		       BITS_TO_BYTES(scan_type->realbits));
 	if (ret < 0)
 		goto out;
 
-- 
2.34.1

[PATCH v6 08/11] 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.

Use trigger-sources property to enable device synchronization over SPI
and multi-device synchronization, as an alternative to adi,sync-in-gpios
property.

Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
---
v6 Changes:
* Created macro for the SYNC index from trigger-sources.
* Check trigger source by the compatible string (and the dev node for the
  self triggering).
* Check nargs before accessing the args array.
* Use `trigger-sources` as an alternative to `adi,sync-in-gpios`
  (now optional), instead of replacing it. 

v5 Changes:
* Allow omitting trigger-sources property.
* include gpio-trigger to trigger-sources to replace adi,sync-in-gpios
  property.
* Read trigger-sources cell value to differentiate the trigger type.

v4 Changes:
* None.

v3 Changes:
* Fixed args.fwnode leakage in the trigger-sources parsing.
* Synchronization over spi is enabled when the trigger-sources
  references the own device.
* Synchronization is kept within the device, and return error if the
  gpio is not defined and the trigger-sources reference does not match
  the current device. 

v2 Changes:
* Synchronization via SPI is enabled when the Sync GPIO is not defined.
* now trigger-sources property indicates the synchronization provider or
  main device. The main device will be used to drive the SYNC_IN when
  requested (via GPIO or SPI).
---
 drivers/iio/adc/ad7768-1.c | 126 +++++++++++++++++++++++++++++++++++--
 1 file changed, 120 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
index 087478afb5bf..c00571f17254 100644
--- a/drivers/iio/adc/ad7768-1.c
+++ b/drivers/iio/adc/ad7768-1.c
@@ -28,6 +28,8 @@
 #include <linux/iio/triggered_buffer.h>
 #include <linux/iio/trigger_consumer.h>
 
+#include <dt-bindings/iio/adc/adi,ad7768-1.h>
+
 /* AD7768 registers definition */
 #define AD7768_REG_CHIP_TYPE		0x3
 #define AD7768_REG_PROD_ID_L		0x4
@@ -100,6 +102,8 @@
 
 #define AD7768_VCM_OFF			0x07
 
+#define AD7768_TRIGGER_SOURCE_SYNC_IDX 0
+
 enum ad7768_conv_mode {
 	AD7768_CONTINUOUS,
 	AD7768_ONE_SHOT,
@@ -209,6 +213,7 @@ struct ad7768_state {
 	struct iio_trigger *trig;
 	struct gpio_desc *gpio_sync_in;
 	struct gpio_desc *gpio_reset;
+	bool en_spi_sync;
 	const char *labels[ARRAY_SIZE(ad7768_channels)];
 	/*
 	 * DMA (thus cache coherency maintenance) may require the
@@ -295,6 +300,19 @@ static const struct regmap_config ad7768_regmap24_config = {
 	.max_register = AD7768_REG24_COEFF_DATA,
 };
 
+static int ad7768_send_sync_pulse(struct ad7768_state *st)
+{
+	if (st->en_spi_sync)
+		return regmap_write(st->regmap, 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)
 {
@@ -391,10 +409,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)
@@ -671,6 +686,94 @@ static const struct iio_info ad7768_info = {
 	.debugfs_reg_access = &ad7768_reg_access,
 };
 
+static struct gpio_desc *ad7768_trigger_source_get_gpio(struct device *dev,
+							struct fwnode_handle *fwnode)
+{
+	const char *value;
+	int ret;
+
+	ret = fwnode_property_read_string(fwnode, "compatible", &value);
+	if (ret)
+		return ERR_PTR(ret);
+
+	if (strcmp("gpio-trigger", value))
+		return ERR_PTR(-EINVAL);
+
+	return devm_fwnode_gpiod_get_index(dev, fwnode, NULL, 0,
+					   GPIOD_OUT_LOW, "sync-in");
+}
+
+static int ad7768_trigger_sources_get_sync(struct device *dev,
+					   struct ad7768_state *st)
+{
+	struct fwnode_reference_args args;
+	struct fwnode_handle *fwnode = NULL;
+	int ret;
+
+	/*
+	 * The AD7768-1 allows two primary methods for driving the SYNC_IN pin
+	 * to synchronize one or more devices:
+	 * 1. Using an external GPIO.
+	 * 2. Using a SPI command, where the SYNC_OUT pin generates a
+	 *    synchronization pulse that drives the SYNC_IN pin.
+	 */
+	if (!device_property_present(dev, "trigger-sources")) {
+		/*
+		 * In the absence of trigger-sources property, enable self
+		 * synchronization over SPI (SYNC_OUT).
+		 */
+		st->en_spi_sync = true;
+		return 0;
+	}
+
+	ret = fwnode_property_get_reference_args(dev_fwnode(dev),
+						 "trigger-sources",
+						 "#trigger-source-cells",
+						 0,
+						 AD7768_TRIGGER_SOURCE_SYNC_IDX,
+						 &args);
+	if (ret)
+		return ret;
+
+	fwnode = args.fwnode;
+	/*
+	 * First, try getting the GPIO trigger source and fallback to
+	 * synchronization over SPI in case of failure.
+	 */
+	st->gpio_sync_in = ad7768_trigger_source_get_gpio(dev, fwnode);
+	if (IS_ERR(st->gpio_sync_in)) {
+		/*
+		 * For this case, it requires one argument, which indicates the
+		 * output pin referenced.
+		 */
+		if (args.nargs < 1)
+			goto err_not_supp;
+
+		if (args.args[0] != AD7768_TRIGGER_SOURCE_SYNC_OUT)
+			goto err_not_supp;
+
+		/*
+		 * Only self trigger is supported for now, i.e.,
+		 * external SYNC_OUT is not allowed.
+		 */
+		if (fwnode->dev == dev) {
+			st->en_spi_sync = true;
+			goto out_put_node;
+		}
+
+		goto err_not_supp;
+	}
+
+	goto out_put_node;
+
+err_not_supp:
+	ret = dev_err_probe(dev, -EOPNOTSUPP,
+			    "Invalid synchronization trigger source");
+out_put_node:
+	fwnode_handle_put(args.fwnode);
+	return ret;
+}
+
 static int ad7768_setup(struct iio_dev *indio_dev)
 {
 	struct ad7768_state *st = iio_priv(indio_dev);
@@ -701,11 +804,22 @@ static int ad7768_setup(struct iio_dev *indio_dev)
 			return ret;
 	}
 
-	st->gpio_sync_in = devm_gpiod_get(&st->spi->dev, "adi,sync-in",
-					  GPIOD_OUT_LOW);
+	/* For backwards compatibility, try the adi,sync-in-gpios property */
+	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 the synchronization is not defined by adi,sync-in-gpios, try the
+	 * trigger-sources.
+	 */
+	if (!st->gpio_sync_in) {
+		ret = ad7768_trigger_sources_get_sync(&st->spi->dev, st);
+		if (ret)
+			return ret;
+	}
+
 	/* Only create a Chip GPIO if flagged for it */
 	if (device_property_read_bool(&st->spi->dev, "gpio-controller")) {
 		ret = ad7768_gpio_init(indio_dev);
-- 
2.34.1

[PATCH v6 09/11] iio: adc: ad7768-1: replace manual attribute declaration

[Jonathan Santos]

Use read_avail callback from struct iio_info to replace the manual
declaration of sampling_frequency_available attribute.

Reviewed-by: David Lechner <dlechner@baylibre.com>
Reviewed-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
---
v6 Changes:
* none.

v5 Changes:
* none.

v4 Changes:
* Added ad7768_fill_samp_freq_tbl() helper function.
* Sampling frequency table is precomputed at probe.

v3 Changes:
* New patch in v3.
---
 drivers/iio/adc/ad7768-1.c | 63 +++++++++++++++++++-------------------
 1 file changed, 31 insertions(+), 32 deletions(-)

diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
index c00571f17254..10791a85d2c5 100644
--- a/drivers/iio/adc/ad7768-1.c
+++ b/drivers/iio/adc/ad7768-1.c
@@ -188,6 +188,7 @@ static const struct iio_chan_spec ad7768_channels[] = {
 		.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),
+		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),
 		.indexed = 1,
 		.channel = 0,
 		.scan_index = 0,
@@ -209,6 +210,7 @@ struct ad7768_state {
 	unsigned int mclk_freq;
 	unsigned int dec_rate;
 	unsigned int samp_freq;
+	unsigned int samp_freq_avail[ARRAY_SIZE(ad7768_clk_config)];
 	struct completion completion;
 	struct iio_trigger *trig;
 	struct gpio_desc *gpio_sync_in;
@@ -313,6 +315,15 @@ static int ad7768_send_sync_pulse(struct ad7768_state *st)
 	return 0;
 }
 
+static void ad7768_fill_samp_freq_tbl(struct ad7768_state *st)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(ad7768_clk_config); i++)
+		st->samp_freq_avail[i] = DIV_ROUND_CLOSEST(st->mclk_freq,
+							   ad7768_clk_config[i].clk_div);
+}
+
 static int ad7768_set_mode(struct ad7768_state *st,
 			   enum ad7768_conv_mode mode)
 {
@@ -570,28 +581,6 @@ static int ad7768_set_freq(struct ad7768_state *st,
 	return 0;
 }
 
-static ssize_t ad7768_sampling_freq_avail(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);
-	unsigned int freq;
-	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);
-	}
-
-	buf[len - 1] = '\n';
-
-	return len;
-}
-
-static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(ad7768_sampling_freq_avail);
-
 static int ad7768_read_raw(struct iio_dev *indio_dev,
 			   struct iio_chan_spec const *chan,
 			   int *val, int *val2, long info)
@@ -637,6 +626,24 @@ static int ad7768_read_raw(struct iio_dev *indio_dev,
 	return -EINVAL;
 }
 
+static int ad7768_read_avail(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     const int **vals, int *type, int *length,
+			     long info)
+{
+	struct ad7768_state *st = iio_priv(indio_dev);
+
+	switch (info) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*vals = (int *)st->samp_freq_avail;
+		*length = ARRAY_SIZE(ad7768_clk_config);
+		*type = IIO_VAL_INT;
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
 static int ad7768_write_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan,
 			    int val, int val2, long info)
@@ -659,15 +666,6 @@ static int ad7768_read_label(struct iio_dev *indio_dev,
 	return sprintf(label, "%s\n", st->labels[chan->channel]);
 }
 
-static struct attribute *ad7768_attributes[] = {
-	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
-	NULL
-};
-
-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)
 {
@@ -678,8 +676,8 @@ static int ad7768_get_current_scan_type(const struct iio_dev *indio_dev,
 }
 
 static const struct iio_info ad7768_info = {
-	.attrs = &ad7768_group,
 	.read_raw = &ad7768_read_raw,
+	.read_avail = &ad7768_read_avail,
 	.write_raw = &ad7768_write_raw,
 	.read_label = ad7768_read_label,
 	.get_current_scan_type = &ad7768_get_current_scan_type,
@@ -1174,6 +1172,7 @@ static int ad7768_probe(struct spi_device *spi)
 		return PTR_ERR(st->mclk);
 
 	st->mclk_freq = clk_get_rate(st->mclk);
+	ad7768_fill_samp_freq_tbl(st);
 
 	indio_dev->channels = ad7768_channels;
 	indio_dev->num_channels = ARRAY_SIZE(ad7768_channels);
-- 
2.34.1

[PATCH v6 10/11] iio: adc: ad7768-1: add filter type and oversampling ratio attributes

[Jonathan Santos]

Separate filter type and decimation rate from the sampling frequency
attribute. The new filter type attribute enables sinc3, sinc3+rej60
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 an oversampling
attribute, which controls the decimation, 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. Sampling frequency
table is updated after every digital filter parameter change.

Reviewed-by: David Lechner <dlechner@baylibre.com>
Co-developed-by: Pop Paul <paul.pop@analog.com>
Signed-off-by: Pop Paul <paul.pop@analog.com>
Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
---
v6 Changes:
* Made sinc3 decimation rate calculation clearer as requested.
* Renamed some filter functions to clarify the purpose.
* Other nits.

v5 Changes:
* Addressed some nits.
* Use the new new iio_device_claim/release_direct() functions.

v4 Changes:
* Sampling frequency table is dynamically updated after every
  filter configuration.

v3 Changes:
* removed unused variables.
* included sinc3+rej60 filter type.
* oversampling_ratio moved to info_mask_shared_by_type.
* reordered functions to avoid forward declaration.
* simplified regmap writes.
* Removed locking.
* replaced some helper functions for direct regmap_update_bits
  calls.
* Addressed other nits.

v2 Changes:
* Decimation_rate attribute replaced for oversampling_ratio.
---
 drivers/iio/adc/ad7768-1.c | 363 ++++++++++++++++++++++++++++++-------
 1 file changed, 293 insertions(+), 70 deletions(-)

diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
index 10791a85d2c5..e2b8f12260a5 100644
--- a/drivers/iio/adc/ad7768-1.c
+++ b/drivers/iio/adc/ad7768-1.c
@@ -20,6 +20,8 @@
 #include <linux/regulator/driver.h>
 #include <linux/sysfs.h>
 #include <linux/spi/spi.h>
+#include <linux/unaligned.h>
+#include <linux/util_macros.h>
 
 #include <linux/iio/buffer.h>
 #include <linux/iio/iio.h>
@@ -77,7 +79,7 @@
 #define AD7768_PWR_PWRMODE(x)		FIELD_PREP(AD7768_PWR_PWRMODE_MSK, x)
 
 /* AD7768_REG_DIGITAL_FILTER */
-#define AD7768_DIG_FIL_FIL_MSK		GENMASK(6, 4)
+#define AD7768_DIG_FIL_FIL_MSK		GENMASK(7, 4)
 #define AD7768_DIG_FIL_FIL(x)		FIELD_PREP(AD7768_DIG_FIL_FIL_MSK, x)
 #define AD7768_DIG_FIL_DEC_MSK		GENMASK(2, 0)
 #define AD7768_DIG_FIL_DEC_RATE(x)	FIELD_PREP(AD7768_DIG_FIL_DEC_MSK, x)
@@ -125,22 +127,20 @@ 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
+enum ad7768_filter_type {
+	AD7768_FILTER_SINC5,
+	AD7768_FILTER_SINC3,
+	AD7768_FILTER_WIDEBAND,
+	AD7768_FILTER_SINC3_REJ60,
 };
 
-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_filter_regval {
+	AD7768_FILTER_REGVAL_SINC5 = 0,
+	AD7768_FILTER_REGVAL_SINC5_X8 = 1,
+	AD7768_FILTER_REGVAL_SINC5_X16 = 2,
+	AD7768_FILTER_REGVAL_SINC3 = 3,
+	AD7768_FILTER_REGVAL_WIDEBAND = 4,
+	AD7768_FILTER_REGVAL_SINC3_REJ60 = 11,
 };
 
 enum ad7768_scan_type {
@@ -152,18 +152,39 @@ 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 ad7768_dec_rate_values[8] = {
+	8, 16, 32, 64, 128, 256, 512, 1024,
+};
+
+/* Decimation Rate range for each filter type */
+static const int ad7768_dec_rate_range[][3] = {
+	[AD7768_FILTER_SINC5] = { 8, 8, 1024 },
+	[AD7768_FILTER_SINC3] = { 32, 32, 163840 },
+	[AD7768_FILTER_WIDEBAND] = { 32, 32, 1024 },
+	[AD7768_FILTER_SINC3_REJ60] = { 32, 32, 163840 },
+};
+
+/*
+ * The AD7768-1 supports three primary filter types:
+ * Sinc5, Sinc3, and Wideband.
+ * However, the filter register values can also encode additional parameters
+ * such as decimation rates and 60Hz rejection. This utility array separates
+ * the filter type from these parameters.
+ */
+static const int ad7768_filter_regval_to_type[] = {
+	[AD7768_FILTER_REGVAL_SINC5] = AD7768_FILTER_SINC5,
+	[AD7768_FILTER_REGVAL_SINC5_X8] = AD7768_FILTER_SINC5,
+	[AD7768_FILTER_REGVAL_SINC5_X16] = AD7768_FILTER_SINC5,
+	[AD7768_FILTER_REGVAL_SINC3] = AD7768_FILTER_SINC3,
+	[AD7768_FILTER_REGVAL_WIDEBAND] = AD7768_FILTER_WIDEBAND,
+	[AD7768_FILTER_REGVAL_SINC3_REJ60] = AD7768_FILTER_SINC3_REJ60,
+};
+
+static const char * const ad7768_filter_enum[] = {
+	[AD7768_FILTER_SINC5] = "sinc5",
+	[AD7768_FILTER_SINC3] = "sinc3",
+	[AD7768_FILTER_WIDEBAND] = "wideband",
+	[AD7768_FILTER_SINC3_REJ60] = "sinc3+rej60"
 };
 
 static const struct iio_scan_type ad7768_scan_type[] = {
@@ -182,13 +203,34 @@ static const struct iio_scan_type ad7768_scan_type[] = {
 	},
 };
 
+static int ad7768_get_filter_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 const struct iio_enum ad7768_filter_type_iio_enum = {
+	.items = ad7768_filter_enum,
+	.num_items = ARRAY_SIZE(ad7768_filter_enum),
+	.set = ad7768_set_fil_type_attr,
+	.get = ad7768_get_filter_type_attr,
+};
+
+static struct iio_chan_spec_ext_info ad7768_ext_info[] = {
+	IIO_ENUM("filter_type", IIO_SHARED_BY_ALL, &ad7768_filter_type_iio_enum),
+	IIO_ENUM_AVAILABLE("filter_type", IIO_SHARED_BY_ALL, &ad7768_filter_type_iio_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_type = BIT(IIO_CHAN_INFO_SCALE) |
+					    BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
 		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
 		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),
+		.ext_info = ad7768_ext_info,
 		.indexed = 1,
 		.channel = 0,
 		.scan_index = 0,
@@ -208,9 +250,12 @@ struct ad7768_state {
 	struct clk *mclk;
 	struct gpio_chip gpiochip;
 	unsigned int mclk_freq;
-	unsigned int dec_rate;
+	unsigned int mclk_div;
+	unsigned int oversampling_ratio;
+	enum ad7768_filter_type filter_type;
 	unsigned int samp_freq;
-	unsigned int samp_freq_avail[ARRAY_SIZE(ad7768_clk_config)];
+	unsigned int samp_freq_avail[ARRAY_SIZE(ad7768_mclk_div_rates)];
+	unsigned int samp_freq_avail_len;
 	struct completion completion;
 	struct iio_trigger *trig;
 	struct gpio_desc *gpio_sync_in;
@@ -317,11 +362,35 @@ static int ad7768_send_sync_pulse(struct ad7768_state *st)
 
 static void ad7768_fill_samp_freq_tbl(struct ad7768_state *st)
 {
-	int i;
+	int i, freq_filtered, len = 0;
+
+	freq_filtered = DIV_ROUND_CLOSEST(st->mclk_freq, st->oversampling_ratio);
+	for (i = 0; i < ARRAY_SIZE(ad7768_mclk_div_rates); i++) {
+		st->samp_freq_avail[len] = DIV_ROUND_CLOSEST(freq_filtered,
+							     ad7768_mclk_div_rates[i]);
+		/* Sampling frequency cannot be lower than the minimum of 50 SPS */
+		if (st->samp_freq_avail[len] >= 50)
+			len++;
+	}
+	st->samp_freq_avail_len = len;
+}
+
+static int ad7768_set_mclk_div(struct ad7768_state *st, unsigned int mclk_div)
+{
+	unsigned int mclk_div_value;
 
-	for (i = 0; i < ARRAY_SIZE(ad7768_clk_config); i++)
-		st->samp_freq_avail[i] = DIV_ROUND_CLOSEST(st->mclk_freq,
-							   ad7768_clk_config[i].clk_div);
+	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
+	 */
+	mclk_div_value |= mclk_div > AD7768_MCLK_DIV_16 ?
+			  AD7768_PWR_PWRMODE(AD7768_FAST_MODE) :
+			  AD7768_PWR_PWRMODE(AD7768_ECO_MODE);
+
+	return regmap_update_bits(st->regmap, AD7768_REG_POWER_CLOCK,
+				  AD7768_PWR_MCLK_DIV_MSK | AD7768_PWR_PWRMODE_MSK,
+				  mclk_div_value);
 }
 
 static int ad7768_set_mode(struct ad7768_state *st,
@@ -357,7 +426,7 @@ static int ad7768_scan_direct(struct iio_dev *indio_dev)
 	 * register provides 24-bit data, the precision is reduced by
 	 * right-shifting the read value by 8 bits.
 	 */
-	if (st->dec_rate == 8)
+	if (st->oversampling_ratio == 8)
 		readval >>= 8;
 
 	/*
@@ -404,22 +473,110 @@ static int ad7768_reg_access(struct iio_dev *indio_dev,
 	return ret;
 }
 
-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 mode;
+	unsigned int max_dec_rate;
+	u8 dec_rate_reg[2];
+	u16 regval;
 	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);
+	/*
+	 * 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_DEC_RATE register:
+	 *  Value = (DEC_RATE / 32) -1
+	 */
+	dec_rate = DIV_ROUND_UP(dec_rate, 32) - 1;
 
-	ret = regmap_write(st->regmap, AD7768_REG_DIGITAL_FILTER, mode);
-	if (ret < 0)
+	/*
+	 * The SINC3_DEC_RATE value is a 13-bit value split across two
+	 * registers: MSB [12:8] and LSB [7:0]. Prepare the 13-bit value using
+	 * FIELD_PREP and store it with the right endianness in dec_rate_reg.
+	 */
+	regval = FIELD_PREP(GENMASK(12, 0), dec_rate);
+	put_unaligned_be16(regval, dec_rate_reg);
+	ret = regmap_bulk_write(st->regmap, AD7768_REG_SINC3_DEC_RATE_MSB,
+				dec_rate_reg, 2);
+	if (ret)
+		return ret;
+
+	st->oversampling_ratio = (dec_rate + 1) * 32;
+
+	return 0;
+}
+
+static int ad7768_configure_dig_fil(struct iio_dev *dev,
+				    enum ad7768_filter_type filter_type,
+				    unsigned int dec_rate)
+{
+	struct ad7768_state *st = iio_priv(dev);
+	unsigned int dec_rate_idx, dig_filter_regval;
+	int ret;
+
+	switch (filter_type) {
+	case AD7768_FILTER_SINC3:
+		dig_filter_regval = AD7768_DIG_FIL_FIL(AD7768_FILTER_REGVAL_SINC3);
+		break;
+	case AD7768_FILTER_SINC3_REJ60:
+		dig_filter_regval = AD7768_DIG_FIL_FIL(AD7768_FILTER_REGVAL_SINC3_REJ60);
+		break;
+	case AD7768_FILTER_WIDEBAND:
+		/* Skip decimations 8 and 16, not supported by the wideband filter */
+		dec_rate_idx = find_closest(dec_rate, &ad7768_dec_rate_values[2],
+					    ARRAY_SIZE(ad7768_dec_rate_values) - 2);
+		dig_filter_regval = AD7768_DIG_FIL_FIL(AD7768_FILTER_REGVAL_WIDEBAND) |
+				    AD7768_DIG_FIL_DEC_RATE(dec_rate_idx);
+		/* Correct the index offset */
+		dec_rate_idx += 2;
+		break;
+	case AD7768_FILTER_SINC5:
+		dec_rate_idx = find_closest(dec_rate, ad7768_dec_rate_values,
+					    ARRAY_SIZE(ad7768_dec_rate_values));
+
+		/*
+		 * Decimations 8 (idx 0) and 16 (idx 1) are set in the
+		 * FILTER[6:4] field. The other decimations are set in the
+		 * DEC_RATE[2:0] field, and the idx need to be offsetted by two.
+		 */
+		if (dec_rate_idx == 0)
+			dig_filter_regval = AD7768_DIG_FIL_FIL(AD7768_FILTER_REGVAL_SINC5_X8);
+		else if (dec_rate_idx == 1)
+			dig_filter_regval = AD7768_DIG_FIL_FIL(AD7768_FILTER_REGVAL_SINC5_X16);
+		else
+			dig_filter_regval = AD7768_DIG_FIL_FIL(AD7768_FILTER_REGVAL_SINC5) |
+					    AD7768_DIG_FIL_DEC_RATE(dec_rate_idx - 2);
+		break;
+	}
+
+	ret = regmap_write(st->regmap, AD7768_REG_DIGITAL_FILTER, dig_filter_regval);
+	if (ret)
 		return ret;
 
-	/* A sync-in pulse is required every time the filter dec rate changes */
+	st->filter_type = filter_type;
+	/*
+	 * The decimation for SINC3 filters are configured in different
+	 * registers
+	 */
+	if (filter_type == AD7768_FILTER_SINC3 ||
+	    filter_type == AD7768_FILTER_SINC3_REJ60) {
+		ret = ad7768_set_sinc3_dec_rate(st, dec_rate);
+		if (ret)
+			return ret;
+	} else {
+		st->oversampling_ratio = ad7768_dec_rate_values[dec_rate_idx];
+	}
+
+	ad7768_fill_samp_freq_tbl(st);
+
+	/* A sync-in pulse is required after every configuration change */
 	return ad7768_send_sync_pulse(st);
 }
 
@@ -542,43 +699,69 @@ static int ad7768_gpio_init(struct iio_dev *indio_dev)
 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->oversampling_ratio);
 
 	/* 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 = regmap_write(st->regmap, AD7768_REG_POWER_CLOCK, pwr_mode);
-	if (ret < 0)
+	/* Set both the mclk_div and pwrmode */
+	ret = ad7768_set_mclk_div(st, idx);
+	if (ret)
 		return ret;
 
-	ret =  ad7768_set_dig_fil(st, ad7768_clk_config[idx].dec_rate);
-	if (ret < 0)
+	st->samp_freq = DIV_ROUND_CLOSEST(st->mclk_freq,
+					  ad7768_mclk_div_rates[idx] * st->oversampling_ratio);
+
+	/* A sync-in pulse is required after every configuration change */
+	return ad7768_send_sync_pulse(st);
+}
+
+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;
+
+	ret = ad7768_configure_dig_fil(dev, filter, st->oversampling_ratio);
+	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);
+	/* Update sampling frequency */
+	return ad7768_set_freq(st, st->samp_freq);
+}
 
-	return 0;
+static int ad7768_get_filter_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 = regmap_read(st->regmap, AD7768_REG_DIGITAL_FILTER, &mode);
+	if (ret)
+		return ret;
+
+	mode = FIELD_GET(AD7768_DIG_FIL_FIL_MSK, mode);
+
+	/* From the register value, get the corresponding filter type */
+	return ad7768_filter_regval_to_type[mode];
 }
 
 static int ad7768_read_raw(struct iio_dev *indio_dev,
@@ -620,6 +803,11 @@ static int ad7768_read_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		*val = st->samp_freq;
 
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		*val = st->oversampling_ratio;
+
 		return IIO_VAL_INT;
 	}
 
@@ -634,9 +822,13 @@ static int ad7768_read_avail(struct iio_dev *indio_dev,
 	struct ad7768_state *st = iio_priv(indio_dev);
 
 	switch (info) {
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		*vals = (int *)ad7768_dec_rate_range[st->filter_type];
+		*type = IIO_VAL_INT;
+		return IIO_AVAIL_RANGE;
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		*vals = (int *)st->samp_freq_avail;
-		*length = ARRAY_SIZE(ad7768_clk_config);
+		*length = st->samp_freq_avail_len;
 		*type = IIO_VAL_INT;
 		return IIO_AVAIL_LIST;
 	default:
@@ -644,20 +836,44 @@ static int ad7768_read_avail(struct iio_dev *indio_dev,
 	}
 }
 
-static int ad7768_write_raw(struct iio_dev *indio_dev,
-			    struct iio_chan_spec const *chan,
-			    int val, int val2, long info)
+static int __ad7768_write_raw(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      int val, int val2, long info)
 {
 	struct ad7768_state *st = iio_priv(indio_dev);
+	int ret;
 
 	switch (info) {
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		return ad7768_set_freq(st, val);
+
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		ret = ad7768_configure_dig_fil(indio_dev, st->filter_type, val);
+		if (ret)
+			return ret;
+
+		/* Update sampling frequency */
+		return ad7768_set_freq(st, st->samp_freq);
 	default:
 		return -EINVAL;
 	}
 }
 
+static int ad7768_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int val, int val2, long info)
+{
+	int ret;
+
+	if (!iio_device_claim_direct(indio_dev))
+		return -EBUSY;
+
+	ret = __ad7768_write_raw(indio_dev, chan, val, val2, info);
+	iio_device_release_direct(indio_dev);
+
+	return ret;
+}
+
 static int ad7768_read_label(struct iio_dev *indio_dev,
 	const struct iio_chan_spec *chan, char *label)
 {
@@ -671,7 +887,7 @@ static int ad7768_get_current_scan_type(const struct iio_dev *indio_dev,
 {
 	struct ad7768_state *st = iio_priv(indio_dev);
 
-	return st->dec_rate == 8 ? AD7768_SCAN_TYPE_HIGH_SPEED :
+	return st->oversampling_ratio == 8 ? AD7768_SCAN_TYPE_HIGH_SPEED :
 		AD7768_SCAN_TYPE_NORMAL;
 }
 
@@ -825,6 +1041,14 @@ static int ad7768_setup(struct iio_dev *indio_dev)
 			return ret;
 	}
 
+	/*
+	 * Set Default Digital Filter configuration:
+	 * SINC5 filter with x32 Decimation rate
+	 */
+	ret = ad7768_configure_dig_fil(indio_dev, AD7768_FILTER_SINC5, 32);
+	if (ret)
+		return ret;
+
 	/* Set the default sampling frequency to 32000 kSPS */
 	return ad7768_set_freq(st, 32000);
 }
@@ -1172,7 +1396,6 @@ static int ad7768_probe(struct spi_device *spi)
 		return PTR_ERR(st->mclk);
 
 	st->mclk_freq = clk_get_rate(st->mclk);
-	ad7768_fill_samp_freq_tbl(st);
 
 	indio_dev->channels = ad7768_channels;
 	indio_dev->num_channels = ARRAY_SIZE(ad7768_channels);
-- 
2.34.1

[PATCH v6 11/11] iio: adc: ad7768-1: add low pass -3dB cutoff attribute

[Jonathan Santos]

Ad7768-1 has a different -3db frequency multiplier depending on
the filter type configured. The cutoff frequency also varies according
to the current ODR.

Add a readonly low pass -3dB frequency cutoff attribute to clarify to
the user which bandwidth is being allowed depending on the filter
configurations.

Reviewed-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
Reviewed-by: David Lechner <dlechner@baylibre.com>
Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
---
v6 Changes:
* None

v5 Changes:
* None

v4 Changes:
* None

v3 Changes:
* None

v2 Changes:
* New patch in v2.
---
 drivers/iio/adc/ad7768-1.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
index e2b8f12260a5..8734859fc24e 100644
--- a/drivers/iio/adc/ad7768-1.c
+++ b/drivers/iio/adc/ad7768-1.c
@@ -148,6 +148,17 @@ enum ad7768_scan_type {
 	AD7768_SCAN_TYPE_HIGH_SPEED,
 };
 
+/*
+ * -3dB cutoff frequency multipliers (relative to ODR) for
+ * each filter type. Values are multiplied by 1000.
+ */
+static const int ad7768_filter_3db_odr_multiplier[] = {
+	[AD7768_FILTER_SINC5] = 204,
+	[AD7768_FILTER_SINC3] = 262,
+	[AD7768_FILTER_SINC3_REJ60] = 262,
+	[AD7768_FILTER_WIDEBAND] = 433,
+};
+
 static const int ad7768_mclk_div_rates[4] = {
 	16, 8, 4, 2,
 };
@@ -226,7 +237,8 @@ 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) |
-					    BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+					    BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) |
+					    BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),
 		.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
 		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
 		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),
@@ -770,7 +782,7 @@ static int ad7768_read_raw(struct iio_dev *indio_dev,
 {
 	struct ad7768_state *st = iio_priv(indio_dev);
 	const struct iio_scan_type *scan_type;
-	int scale_uv, ret;
+	int scale_uv, ret, temp;
 
 	scan_type = iio_get_current_scan_type(indio_dev, chan);
 	if (IS_ERR(scan_type))
@@ -808,6 +820,12 @@ static int ad7768_read_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
 		*val = st->oversampling_ratio;
 
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+		temp = st->samp_freq * ad7768_filter_3db_odr_multiplier[st->filter_type];
+		*val = DIV_ROUND_CLOSEST(temp, 1000);
+
 		return IIO_VAL_INT;
 	}
 
-- 
2.34.1

Re: [PATCH v6 05/11] iio: adc: ad7768-1: add regulator to control VCM output

[Andy Shevchenko]

On Mon, Apr 28, 2025 at 3:13 AM Jonathan Santos
<Jonathan.Santos@analog.com> wrote:
>
> The VCM output voltage can be used as a common-mode voltage within the
> amplifier preconditioning circuits external to the AD7768-1.
>
> This change allows the user to configure VCM output using the regulator
> framework.

...

>  #include <linux/gpio/consumer.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/of.h>

Why?

>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/regulator/driver.h>

...

> +static int ad7768_vcm_enable(struct regulator_dev *rdev)
> +{
> +       struct iio_dev *indio_dev = rdev_get_drvdata(rdev);
> +       struct ad7768_state *st = iio_priv(indio_dev);
> +       int ret, regval;

> +       if (!indio_dev)
> +               return -EINVAL;

Isn't it a dead code? Or i.o.w. under which circumstances can this be true?
Ditto for other functions with the same check.

> +       if (!iio_device_claim_direct(indio_dev))
> +               return -EBUSY;
> +
> +       /* To enable, set the last selected output */
> +       regval = AD7768_REG_ANALOG2_VCM(st->vcm_output_sel + 1);
> +       ret = regmap_update_bits(st->regmap, AD7768_REG_ANALOG2,
> +                                AD7768_REG_ANALOG2_VCM_MSK, regval);
> +       iio_device_release_direct(indio_dev);
> +
> +       return ret;
> +}

...

> +       return clamp(val, 1, (int)rdev->desc->n_voltages) - 1;

No explicit castings in min/max/clamp, please. This may lead to subtle
mistakes. Also, don't forget to include minmax.h.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 06/11] iio: adc: ad7768-1: Add GPIO controller support

[Andy Shevchenko]

On Mon, Apr 28, 2025 at 3:13 AM Jonathan Santos
<Jonathan.Santos@analog.com> wrote:
>
> 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.
>
> Add direct mode conditional locks in the gpio callbacks to prevent register
> access when the device is in buffered mode.
>
> This change exports the AD7768-1's four gpios and makes them accessible
> at an upper layer.

...

> +#include <linux/gpio.h>

No way. This header must not be in any of the code. (Yes, there are
leftovers in the kernel, but work is ongoing to clean that up)

> +#include <linux/gpio/driver.h>
>  #include <linux/gpio/consumer.h>

>  #include <linux/kernel.h>

And since you are doing the big series for the driver, please drop
this header and replace it (if required) with what is used. No driver
code should use kernel.h.

>  #include <linux/module.h>

...

> struct ad7768_state {

>         struct regulator_dev *vcm_rdev;
>         unsigned int vcm_output_sel;
>         struct clk *mclk;
> +       struct gpio_chip gpiochip;
>         unsigned int mclk_freq;
>         unsigned int samp_freq;
>         struct completion completion;

Btw, have you run `pahole`? Is this the best place for a new field in
accordance with its output?

...

> +static int ad7768_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
> +{
> +       struct iio_dev *indio_dev = gpiochip_get_data(chip);
> +       struct ad7768_state *st = iio_priv(indio_dev);
> +       unsigned int val;
> +       int ret;
> +
> +       if (!iio_device_claim_direct(indio_dev))
> +               return -EBUSY;
> +
> +       ret = regmap_read(st->regmap, AD7768_REG_GPIO_CONTROL, &val);
> +       if (ret)
> +               goto err_release;
> +
> +       if (val & BIT(offset))
> +               ret = regmap_update_bits(st->regmap, AD7768_REG_GPIO_WRITE,
> +                                        BIT(offset), value << offset);

And if value happens to be > 1?
Also consider the use of regmap_assign_bits().

> +err_release:
> +       iio_device_release_direct(indio_dev);
> +
> +       return ret;
> +}

...

> +static int ad7768_gpio_init(struct iio_dev *indio_dev)
> +{
> +       struct ad7768_state *st = iio_priv(indio_dev);
> +       int ret;
> +
> +       ret = regmap_write(st->regmap, AD7768_REG_GPIO_CONTROL,
> +                          AD7768_GPIO_UNIVERSAL_EN);
> +       if (ret)
> +               return ret;
> +
> +       st->gpiochip = (struct gpio_chip) {

> +               .label = "ad7768_1_gpios",

What is '_1' for?
Also, what will happen if the device has two or more such ADCs
installed? Will they all provide _the same_ label?!

> +               .base = -1,
> +               .ngpio = 4,
> +               .parent = &st->spi->dev,
> +               .can_sleep = true,
> +               .direction_input = ad7768_gpio_direction_input,
> +               .direction_output = ad7768_gpio_direction_output,
> +               .get = ad7768_gpio_get,
> +               .set_rv = ad7768_gpio_set,
> +               .owner = THIS_MODULE,
> +       };
> +
> +       return devm_gpiochip_add_data(&st->spi->dev, &st->gpiochip, indio_dev);
> +}

...

> +       /* Only create a Chip GPIO if flagged for it */
> +       if (device_property_read_bool(&st->spi->dev, "gpio-controller")) {
> +               ret = ad7768_gpio_init(indio_dev);
> +               if (ret < 0)

Why ' < 0'?

> +                       return ret;
> +       }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 07/11] iio: adc: ad7768-1: add multiple scan types to support 16-bits mode

[Andy Shevchenko]

On Mon, Apr 28, 2025 at 3:13 AM Jonathan Santos
<Jonathan.Santos@analog.com> wrote:
>
> When the device is configured to decimation x8, only possible in the
> sinc5 filter, 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

at runtime
making it possible

> 16-bit resolution.

...

> +       ret = spi_read(st->spi, &st->data.scan.chan,
> +                      BITS_TO_BYTES(scan_type->realbits));
>         if (ret < 0)
>                 goto out;

Add a TODO to convert this to use a new helper from 163ddf1fea59.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 08/11] iio: adc: ad7768-1: add support for Synchronization over SPI

[Andy Shevchenko]

On Mon, Apr 28, 2025 at 3:14 AM Jonathan Santos
<Jonathan.Santos@analog.com> 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.

to the SYNC_IN

> Use trigger-sources property to enable device synchronization over SPI
> and multi-device synchronization, as an alternative to adi,sync-in-gpios
> property.

...

> +static int ad7768_send_sync_pulse(struct ad7768_state *st)
> +{
> +       if (st->en_spi_sync)
> +               return regmap_write(st->regmap, AD7768_REG_SYNC_RESET, 0x00);

> +       if (st->gpio_sync_in) {

Dup check, the following have already it.

> +               gpiod_set_value_cansleep(st->gpio_sync_in, 1);

Yes, I see the original code, but still the Q is why no delay. Perhaps
a comment explaining that the GPIO op is slow enough (?) to add.

> +               gpiod_set_value_cansleep(st->gpio_sync_in, 0);
> +       }
> +
> +       return 0;
> +}

...

> +static struct gpio_desc *ad7768_trigger_source_get_gpio(struct device *dev,
> +                                                       struct fwnode_handle *fwnode)
> +{
> +       const char *value;
> +       int ret;
> +
> +       ret = fwnode_property_read_string(fwnode, "compatible", &value);
> +       if (ret)
> +               return ERR_PTR(ret);
> +
> +       if (strcmp("gpio-trigger", value))
> +               return ERR_PTR(-EINVAL);

Reinvention of fwnode_device_is_compatible().

> +       return devm_fwnode_gpiod_get_index(dev, fwnode, NULL, 0,
> +                                          GPIOD_OUT_LOW, "sync-in");
> +}

...

> +static int ad7768_trigger_sources_get_sync(struct device *dev,
> +                                          struct ad7768_state *st)
> +{
> +       struct fwnode_reference_args args;
> +       struct fwnode_handle *fwnode = NULL;

Redundant assignment AFAICS.

> +       int ret;
> +
> +       /*
> +        * The AD7768-1 allows two primary methods for driving the SYNC_IN pin
> +        * to synchronize one or more devices:
> +        * 1. Using an external GPIO.
> +        * 2. Using a SPI command, where the SYNC_OUT pin generates a
> +        *    synchronization pulse that drives the SYNC_IN pin.
> +        */
> +       if (!device_property_present(dev, "trigger-sources")) {
> +               /*
> +                * In the absence of trigger-sources property, enable self
> +                * synchronization over SPI (SYNC_OUT).
> +                */
> +               st->en_spi_sync = true;
> +               return 0;
> +       }
> +
> +       ret = fwnode_property_get_reference_args(dev_fwnode(dev),

In this case the above is better to be also fwnode_property_present().
You save a double call to dev_fwnode().

> +                                                "trigger-sources",
> +                                                "#trigger-source-cells",
> +                                                0,
> +                                                AD7768_TRIGGER_SOURCE_SYNC_IDX,
> +                                                &args);
> +       if (ret)
> +               return ret;
> +
> +       fwnode = args.fwnode;
> +       /*
> +        * First, try getting the GPIO trigger source and fallback to
> +        * synchronization over SPI in case of failure.
> +        */
> +       st->gpio_sync_in = ad7768_trigger_source_get_gpio(dev, fwnode);
> +       if (IS_ERR(st->gpio_sync_in)) {
> +               /*
> +                * For this case, it requires one argument, which indicates the
> +                * output pin referenced.
> +                */
> +               if (args.nargs < 1)
> +                       goto err_not_supp;
> +
> +               if (args.args[0] != AD7768_TRIGGER_SOURCE_SYNC_OUT)
> +                       goto err_not_supp;
> +
> +               /*
> +                * Only self trigger is supported for now, i.e.,
> +                * external SYNC_OUT is not allowed.
> +                */
> +               if (fwnode->dev == dev) {

?!?! What is this?!

For the reference:
https://elixir.bootlin.com/linux/v6.15-rc3/source/include/linux/fwnode.h#L51

> +                       st->en_spi_sync = true;
> +                       goto out_put_node;
> +               }
> +
> +               goto err_not_supp;
> +       }
> +
> +       goto out_put_node;
> +
> +err_not_supp:
> +       ret = dev_err_probe(dev, -EOPNOTSUPP,
> +                           "Invalid synchronization trigger source");

Missing \n, and can be one line anyway (we don't complain about long
strings ending with string literals for ages, way before the 100
character limit).

> +out_put_node:
> +       fwnode_handle_put(args.fwnode);
> +       return ret;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 09/11] iio: adc: ad7768-1: replace manual attribute declaration

[Andy Shevchenko]

On Mon, Apr 28, 2025 at 3:14 AM Jonathan Santos
<Jonathan.Santos@analog.com> wrote:
>
> Use read_avail callback from struct iio_info to replace the manual
> declaration of sampling_frequency_available attribute.

...

> +static void ad7768_fill_samp_freq_tbl(struct ad7768_state *st)
> +{
> +       int i;

Why signed?

> +       for (i = 0; i < ARRAY_SIZE(ad7768_clk_config); i++)

Check that the driver includes array_size.h...

> +               st->samp_freq_avail[i] = DIV_ROUND_CLOSEST(st->mclk_freq,
> +                                                          ad7768_clk_config[i].clk_div);

...and math.h.

> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 10/11] iio: adc: ad7768-1: add filter type and oversampling ratio attributes

[Andy Shevchenko]

On Mon, Apr 28, 2025 at 3:14 AM Jonathan Santos
<Jonathan.Santos@analog.com> wrote:
>
> Separate filter type and decimation rate from the sampling frequency
> attribute. The new filter type attribute enables sinc3, sinc3+rej60
> 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 an oversampling
> attribute, which controls the decimation, 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. Sampling frequency
> table is updated after every digital filter parameter change.

...

> +static int ad7768_get_filter_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);

Would it be possible to avoid forward declarations?

...

>  static void ad7768_fill_samp_freq_tbl(struct ad7768_state *st)
>  {
> -       int i;
> +       int i, freq_filtered, len = 0;

Should 'i' be signed?

> +
> +       freq_filtered = DIV_ROUND_CLOSEST(st->mclk_freq, st->oversampling_ratio);
> +       for (i = 0; i < ARRAY_SIZE(ad7768_mclk_div_rates); i++) {
> +               st->samp_freq_avail[len] = DIV_ROUND_CLOSEST(freq_filtered,
> +                                                            ad7768_mclk_div_rates[i]);
> +               /* Sampling frequency cannot be lower than the minimum of 50 SPS */
> +               if (st->samp_freq_avail[len] >= 50)
> +                       len++;
> +       }
> +       st->samp_freq_avail_len = len;
> +}

...

> +       dec_rate = clamp_t(unsigned int, dec_rate, 32, max_dec_rate);

Can't clamp() be used? _t variants are not very good and may play a
bad joke on subtle cases.

...

> +               /*
> +                * Decimations 8 (idx 0) and 16 (idx 1) are set in the
> +                * FILTER[6:4] field. The other decimations are set in the
> +                * DEC_RATE[2:0] field, and the idx need to be offsetted by two.

needs

> +                */

...

> +       /*
> +        * The decimation for SINC3 filters are configured in different
> +        * registers

Missing period at the end.

> +        */

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 05/11] iio: adc: ad7768-1: add regulator to control VCM output

[David Lechner]

On 4/27/25 7:13 PM, Jonathan Santos wrote:
> The VCM output voltage can be used as a common-mode voltage within the
> amplifier preconditioning circuits external to the AD7768-1.
> 
> This change allows the user to configure VCM output using the regulator
> framework.
> 
> Acked-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> ---
With the issues that Andy pointed out addressed:

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

Re: [PATCH v6 02/11] dt-bindings: iio: adc: ad7768-1: add trigger-sources property

[David Lechner]

On 4/27/25 7:12 PM, Jonathan Santos wrote:
> In addition to GPIO synchronization, The AD7768-1 also supports
> synchronization over SPI, which use is recommended when the GPIO
> cannot provide a pulse synchronous with the base MCLK signal. It
> consists of looping back the SYNC_OUT to the SYNC_IN pin and send
> a command via SPI to trigger the synchronization.
> 
> Introduce the 'trigger-sources' property to enable SPI-based
> synchronization via SYNC_OUT pin, along with additional optional
> entries for GPIO3 and DRDY pins.
> 
> Also create #trigger-source-cells property to differentiate the trigger
> sources provided by the ADC. To improve readability, create a
> adi,ad7768-1.h header with the macros for the cell values.
> 
> While at it, add description to the interrupts property.
> 
> Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> ---


...

> ---
>  .../bindings/iio/adc/adi,ad7768-1.yaml        | 31 ++++++++++++++++++-
>  include/dt-bindings/iio/adc/adi,ad7768-1.h    | 10 ++++++
>  2 files changed, 40 insertions(+), 1 deletion(-)
>  create mode 100644 include/dt-bindings/iio/adc/adi,ad7768-1.h

We should add this new file to the MAINTAINERS entry.

Otherwise:

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

Re: [PATCH v6 08/11] iio: adc: ad7768-1: add support for Synchronization over SPI

[David Lechner]

On 4/27/25 7:13 PM, 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.
> 
> Use trigger-sources property to enable device synchronization over SPI
> and multi-device synchronization, as an alternative to adi,sync-in-gpios
> property.
> 
> Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> ---

...

> +static int ad7768_trigger_sources_get_sync(struct device *dev,
> +					   struct ad7768_state *st)
> +{
> +	struct fwnode_reference_args args;
> +	struct fwnode_handle *fwnode = NULL;
> +	int ret;
> +
> +	/*
> +	 * The AD7768-1 allows two primary methods for driving the SYNC_IN pin
> +	 * to synchronize one or more devices:
> +	 * 1. Using an external GPIO.
> +	 * 2. Using a SPI command, where the SYNC_OUT pin generates a
> +	 *    synchronization pulse that drives the SYNC_IN pin.
> +	 */
> +	if (!device_property_present(dev, "trigger-sources")) {
> +		/*
> +		 * In the absence of trigger-sources property, enable self
> +		 * synchronization over SPI (SYNC_OUT).
> +		 */
> +		st->en_spi_sync = true;
> +		return 0;
> +	}
> +
> +	ret = fwnode_property_get_reference_args(dev_fwnode(dev),
> +						 "trigger-sources",
> +						 "#trigger-source-cells",
> +						 0,
> +						 AD7768_TRIGGER_SOURCE_SYNC_IDX,
> +						 &args);
> +	if (ret)
> +		return ret;
> +
> +	fwnode = args.fwnode;
> +	/*
> +	 * First, try getting the GPIO trigger source and fallback to
> +	 * synchronization over SPI in case of failure.
> +	 */
> +	st->gpio_sync_in = ad7768_trigger_source_get_gpio(dev, fwnode);
> +	if (IS_ERR(st->gpio_sync_in)) {

Normally, having error be the indented path like this is preferred, but I think
this case is an exception since the following is "normal" code path, not error
return path.

I would understand this better as:

	st->gpio_sync_in = ad7768_trigger_source_get_gpio(dev, fwnode);
	if (!IS_ERR(st->gpio_sync_in))
		/* The trigger is a GPIO, our job is done here. */
		goto out_put_node;

	/* Second, ... */

> +		/*
> +		 * For this case, it requires one argument, which indicates the
> +		 * output pin referenced.
> +		 */
> +		if (args.nargs < 1)
> +			goto err_not_supp;
> +
> +		if (args.args[0] != AD7768_TRIGGER_SOURCE_SYNC_OUT)
> +			goto err_not_supp;
> +
> +		/*
> +		 * Only self trigger is supported for now, i.e.,
> +		 * external SYNC_OUT is not allowed.
> +		 */
> +		if (fwnode->dev == dev) {

As Andy pointed out, this is a bit odd. Technically, we should be allowing any
trigger provider with the right capabilities. But we don't have a trigger
subsystem yet to make that easy. Since we can already handle the SYNC_IN is
wired to SYNC_OUT of the same chip above by omitting the trigger-sources
property, I would just make a TODO here and always return the not supported
error for now.

> +			st->en_spi_sync = true;
> +			goto out_put_node;
> +		}
> +
> +		goto err_not_supp;
> +	}
> +
> +	goto out_put_node;
> +
> +err_not_supp:
> +	ret = dev_err_probe(dev, -EOPNOTSUPP,
> +			    "Invalid synchronization trigger source");
> +out_put_node:
> +	fwnode_handle_put(args.fwnode);
> +	return ret;
> +}
> +

Re: [PATCH v6 05/11] iio: adc: ad7768-1: add regulator to control VCM output

[Jonathan Santos]

On 04/28, Andy Shevchenko wrote:
> On Mon, Apr 28, 2025 at 3:13 AM Jonathan Santos
> <Jonathan.Santos@analog.com> wrote:
> >
> > The VCM output voltage can be used as a common-mode voltage within the
> > amplifier preconditioning circuits external to the AD7768-1.
> >
> > This change allows the user to configure VCM output using the regulator
> > framework.
> 
> ...
> 
> >  #include <linux/gpio/consumer.h>
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> > +#include <linux/of.h>
> 
> Why?
>

I was using of_match_ptr() before, but forgot to remove it, sorry.

> >  #include <linux/regmap.h>
> >  #include <linux/regulator/consumer.h>
> > +#include <linux/regulator/driver.h>
> 
> ...
> 
> > +static int ad7768_vcm_enable(struct regulator_dev *rdev)
> > +{
> > +       struct iio_dev *indio_dev = rdev_get_drvdata(rdev);
> > +       struct ad7768_state *st = iio_priv(indio_dev);
> > +       int ret, regval;
> 
> > +       if (!indio_dev)
> > +               return -EINVAL;
> 
> Isn't it a dead code? Or i.o.w. under which circumstances can this be true?
> Ditto for other functions with the same check.
> 

Yes, you're right. Since I defined the driver data below, there was no
need for the check.

> > +       if (!iio_device_claim_direct(indio_dev))
> > +               return -EBUSY;
> > +
> > +       /* To enable, set the last selected output */
> > +       regval = AD7768_REG_ANALOG2_VCM(st->vcm_output_sel + 1);
> > +       ret = regmap_update_bits(st->regmap, AD7768_REG_ANALOG2,
> > +                                AD7768_REG_ANALOG2_VCM_MSK, regval);
> > +       iio_device_release_direct(indio_dev);
> > +
> > +       return ret;
> > +}
> 
> ...
> 
> > +       return clamp(val, 1, (int)rdev->desc->n_voltages) - 1;
> 
> No explicit castings in min/max/clamp, please. This may lead to subtle
> mistakes. Also, don't forget to include minmax.h.
> 

Okay, thanks.

> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v6 06/11] iio: adc: ad7768-1: Add GPIO controller support

[Jonathan Santos]

On 04/28, Andy Shevchenko wrote:
> On Mon, Apr 28, 2025 at 3:13 AM Jonathan Santos
> <Jonathan.Santos@analog.com> wrote:
> >
> > 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.
> >
> > Add direct mode conditional locks in the gpio callbacks to prevent register
> > access when the device is in buffered mode.
> >
> > This change exports the AD7768-1's four gpios and makes them accessible
> > at an upper layer.
> 
> ...
> 
> > +#include <linux/gpio.h>
> 
> No way. This header must not be in any of the code. (Yes, there are
> leftovers in the kernel, but work is ongoing to clean that up)
> 

Sorry about that, will fix it.

> > +#include <linux/gpio/driver.h>
> >  #include <linux/gpio/consumer.h>
> 
> >  #include <linux/kernel.h>
> 
> And since you are doing the big series for the driver, please drop
> this header and replace it (if required) with what is used. No driver
> code should use kernel.h.
> 

Sure, noted.

> >  #include <linux/module.h>
> 
> ...
> 
> > struct ad7768_state {
> 
> >         struct regulator_dev *vcm_rdev;
> >         unsigned int vcm_output_sel;
> >         struct clk *mclk;
> > +       struct gpio_chip gpiochip;
> >         unsigned int mclk_freq;
> >         unsigned int samp_freq;
> >         struct completion completion;
> 
> Btw, have you run `pahole`? Is this the best place for a new field in
> accordance with its output?
> 

I didn't, but I am going to use it.

> ...
> 
> > +static int ad7768_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
> > +{
> > +       struct iio_dev *indio_dev = gpiochip_get_data(chip);
> > +       struct ad7768_state *st = iio_priv(indio_dev);
> > +       unsigned int val;
> > +       int ret;
> > +
> > +       if (!iio_device_claim_direct(indio_dev))
> > +               return -EBUSY;
> > +
> > +       ret = regmap_read(st->regmap, AD7768_REG_GPIO_CONTROL, &val);
> > +       if (ret)
> > +               goto err_release;
> > +
> > +       if (val & BIT(offset))
> > +               ret = regmap_update_bits(st->regmap, AD7768_REG_GPIO_WRITE,
> > +                                        BIT(offset), value << offset);
> 
> And if value happens to be > 1?
> Also consider the use of regmap_assign_bits().
> 

ok!

> > +err_release:
> > +       iio_device_release_direct(indio_dev);
> > +
> > +       return ret;
> > +}
> 
> ...
> 
> > +static int ad7768_gpio_init(struct iio_dev *indio_dev)
> > +{
> > +       struct ad7768_state *st = iio_priv(indio_dev);
> > +       int ret;
> > +
> > +       ret = regmap_write(st->regmap, AD7768_REG_GPIO_CONTROL,
> > +                          AD7768_GPIO_UNIVERSAL_EN);
> > +       if (ret)
> > +               return ret;
> > +
> > +       st->gpiochip = (struct gpio_chip) {
> 
> > +               .label = "ad7768_1_gpios",
> 
> What is '_1' for?
> Also, what will happen if the device has two or more such ADCs
> installed? Will they all provide _the same_ label?!
> 

the '_1' is because the part name is 'ad7768-1'.
Most of similar devices only define a static name, but I could use the
reg field, if you beleive it is better.

> > +               .base = -1,
> > +               .ngpio = 4,
> > +               .parent = &st->spi->dev,
> > +               .can_sleep = true,
> > +               .direction_input = ad7768_gpio_direction_input,
> > +               .direction_output = ad7768_gpio_direction_output,
> > +               .get = ad7768_gpio_get,
> > +               .set_rv = ad7768_gpio_set,
> > +               .owner = THIS_MODULE,
> > +       };
> > +
> > +       return devm_gpiochip_add_data(&st->spi->dev, &st->gpiochip, indio_dev);
> > +}
> 
> ...
> 
> > +       /* Only create a Chip GPIO if flagged for it */
> > +       if (device_property_read_bool(&st->spi->dev, "gpio-controller")) {
> > +               ret = ad7768_gpio_init(indio_dev);
> > +               if (ret < 0)
> 
> Why ' < 0'?
> 
> > +                       return ret;
> > +       }
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v6 08/11] iio: adc: ad7768-1: add support for Synchronization over SPI

[Jonathan Santos]

On 04/28, Andy Shevchenko wrote:
> On Mon, Apr 28, 2025 at 3:14 AM Jonathan Santos
> <Jonathan.Santos@analog.com> 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.
> 
> to the SYNC_IN
> 
> > Use trigger-sources property to enable device synchronization over SPI
> > and multi-device synchronization, as an alternative to adi,sync-in-gpios
> > property.
> 
> ...
> 
> > +static int ad7768_send_sync_pulse(struct ad7768_state *st)
> > +{
> > +       if (st->en_spi_sync)
> > +               return regmap_write(st->regmap, AD7768_REG_SYNC_RESET, 0x00);
> 
> > +       if (st->gpio_sync_in) {
> 
> Dup check, the following have already it.
> 
> > +               gpiod_set_value_cansleep(st->gpio_sync_in, 1);
> 
> Yes, I see the original code, but still the Q is why no delay. Perhaps
> a comment explaining that the GPIO op is slow enough (?) to add.
> 

Datasheet specifies a minimum of 1.5*Tmclk pulse width. For the
recommended mclk of 16.384 MHz, it usually takes 4 times the minimum
pulse width. If it can be less than that for other plataforms I can add
this delay.

> > +               gpiod_set_value_cansleep(st->gpio_sync_in, 0);
> > +       }
> > +
> > +       return 0;
> > +}
> 
> ...
> 
> > +static struct gpio_desc *ad7768_trigger_source_get_gpio(struct device *dev,
> > +                                                       struct fwnode_handle *fwnode)
> > +{
> > +       const char *value;
> > +       int ret;
> > +
> > +       ret = fwnode_property_read_string(fwnode, "compatible", &value);
> > +       if (ret)
> > +               return ERR_PTR(ret);
> > +
> > +       if (strcmp("gpio-trigger", value))
> > +               return ERR_PTR(-EINVAL);
> 
> Reinvention of fwnode_device_is_compatible().
> 

Thanks!

> > +       return devm_fwnode_gpiod_get_index(dev, fwnode, NULL, 0,
> > +                                          GPIOD_OUT_LOW, "sync-in");
> > +}
> 
> ...
> 
> > +static int ad7768_trigger_sources_get_sync(struct device *dev,
> > +                                          struct ad7768_state *st)
> > +{
> > +       struct fwnode_reference_args args;
> > +       struct fwnode_handle *fwnode = NULL;
> 
> Redundant assignment AFAICS.
> 
> > +       int ret;
> > +
> > +       /*
> > +        * The AD7768-1 allows two primary methods for driving the SYNC_IN pin
> > +        * to synchronize one or more devices:
> > +        * 1. Using an external GPIO.
> > +        * 2. Using a SPI command, where the SYNC_OUT pin generates a
> > +        *    synchronization pulse that drives the SYNC_IN pin.
> > +        */
> > +       if (!device_property_present(dev, "trigger-sources")) {
> > +               /*
> > +                * In the absence of trigger-sources property, enable self
> > +                * synchronization over SPI (SYNC_OUT).
> > +                */
> > +               st->en_spi_sync = true;
> > +               return 0;
> > +       }
> > +
> > +       ret = fwnode_property_get_reference_args(dev_fwnode(dev),
> 
> In this case the above is better to be also fwnode_property_present().
> You save a double call to dev_fwnode().
> 
> > +                                                "trigger-sources",
> > +                                                "#trigger-source-cells",
> > +                                                0,
> > +                                                AD7768_TRIGGER_SOURCE_SYNC_IDX,
> > +                                                &args);
> > +       if (ret)
> > +               return ret;
> > +
> > +       fwnode = args.fwnode;
> > +       /*
> > +        * First, try getting the GPIO trigger source and fallback to
> > +        * synchronization over SPI in case of failure.
> > +        */
> > +       st->gpio_sync_in = ad7768_trigger_source_get_gpio(dev, fwnode);
> > +       if (IS_ERR(st->gpio_sync_in)) {
> > +               /*
> > +                * For this case, it requires one argument, which indicates the
> > +                * output pin referenced.
> > +                */
> > +               if (args.nargs < 1)
> > +                       goto err_not_supp;
> > +
> > +               if (args.args[0] != AD7768_TRIGGER_SOURCE_SYNC_OUT)
> > +                       goto err_not_supp;
> > +
> > +               /*
> > +                * Only self trigger is supported for now, i.e.,
> > +                * external SYNC_OUT is not allowed.
> > +                */
> > +               if (fwnode->dev == dev) {
> 
> ?!?! What is this?!
> 
> For the reference:
> https://elixir.bootlin.com/linux/v6.15-rc3/source/include/linux/fwnode.h#L51
> 

I see, my bad. I will follow David's suggestion, so we will avoid this.

> > +                       st->en_spi_sync = true;
> > +                       goto out_put_node;
> > +               }
> > +
> > +               goto err_not_supp;
> > +       }
> > +
> > +       goto out_put_node;
> > +
> > +err_not_supp:
> > +       ret = dev_err_probe(dev, -EOPNOTSUPP,
> > +                           "Invalid synchronization trigger source");
> 
> Missing \n, and can be one line anyway (we don't complain about long
> strings ending with string literals for ages, way before the 100
> character limit).
> 
> > +out_put_node:
> > +       fwnode_handle_put(args.fwnode);
> > +       return ret;
> > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v6 07/11] iio: adc: ad7768-1: add multiple scan types to support 16-bits mode

[Marcelo Schmitt]

Hi Jonathan,

The new adjustable sample rate / precision patch looks good to me.
My only concern is about one error handling path in the trigger handler function.
With that sorted out
Reviewed-by: Marcelo Schmitt <marcelo.schmitt@analog.com>

On 04/27, Jonathan Santos wrote:
> When the device is configured to decimation x8, only possible in the
> sinc5 filter, 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.
> 
> Reviewed-by: David Lechner <dlechner@baylire.com>
> Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> ---
...
> +enum ad7768_scan_type {
> +	AD7768_SCAN_TYPE_NORMAL,
> +	AD7768_SCAN_TYPE_HIGH_SPEED,
> +};
> +
> +static const int ad7768_mclk_div_rates[4] = {
I think we can omit the 4 constant here
static const int ad7768_mclk_div_rates[] = {

Probably not a reason for a v7 if the other parts of the series are good.
> +	16, 8, 4, 2,
> +};
> +
...
>  
> @@ -674,9 +722,15 @@ static irqreturn_t ad7768_trigger_handler(int irq, void *p)
>  	struct iio_poll_func *pf = p;
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	struct ad7768_state *st = iio_priv(indio_dev);
> +	const struct iio_scan_type *scan_type;
>  	int ret;
>  
> -	ret = spi_read(st->spi, &st->data.scan.chan, 3);
> +	scan_type = iio_get_current_scan_type(indio_dev, &indio_dev->channels[0]);
> +	if (IS_ERR(scan_type))
> +		return PTR_ERR(scan_type);

The IRQ never gets handled if we get an error from iio_get_current_scan_type()?
Maybe make it jump to out?
	if (IS_ERR(scan_type))
		goto out;

> +
> +	ret = spi_read(st->spi, &st->data.scan.chan,
> +		       BITS_TO_BYTES(scan_type->realbits));
>  	if (ret < 0)
>  		goto out;
>  

Best regards,
Marcelo

Re: [PATCH v6 02/11] dt-bindings: iio: adc: ad7768-1: add trigger-sources property

[Jonathan Cameron]

On Sun, 27 Apr 2025 21:12:16 -0300
Jonathan Santos <Jonathan.Santos@analog.com> wrote:

> In addition to GPIO synchronization, The AD7768-1 also supports
> synchronization over SPI, which use is recommended when the GPIO
> cannot provide a pulse synchronous with the base MCLK signal. It
> consists of looping back the SYNC_OUT to the SYNC_IN pin and send
> a command via SPI to trigger the synchronization.
> 
> Introduce the 'trigger-sources' property to enable SPI-based
> synchronization via SYNC_OUT pin, along with additional optional
> entries for GPIO3 and DRDY pins.
> 
> Also create #trigger-source-cells property to differentiate the trigger
> sources provided by the ADC. To improve readability, create a
> adi,ad7768-1.h header with the macros for the cell values.
> 
> While at it, add description to the interrupts property.
> 
> Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>

A few things inline, but minor stuff for if you need to do a respin.

> ---
> v6 Changes:
> * Removed references to offload.
> * Changed trigger-sources-cells description. Each cell value indicates
>   a gpio line from the ADC.
> * Added adi,ad7768-1.h header with macros for the trigger source cells.
> * Removed offload trigger entry from trigger-sources.
> 
> v5 Changes:
> * Include START pin and DRDY in the trigger-sources description.
> * Fixed "#trigger-source-cells" value and description.
> * sync-in-gpios is represented in the trigger-sources property.
> 
> v4 Changes:
> * none
> 
> v3 Changes:
> * Fixed dt-bindings errors.
> * Trigger-source is set as an alternative to sync-in-gpios, so we
>   don't break the previous ABI.
> * increased maxItems from trigger-sources to 2.
> 
> v2 Changes:
> * Patch added as replacement for adi,sync-in-spi patch.
> * addressed the request for a description to interrupts property.
> ---
>  .../bindings/iio/adc/adi,ad7768-1.yaml        | 31 ++++++++++++++++++-
>  include/dt-bindings/iio/adc/adi,ad7768-1.h    | 10 ++++++
>  2 files changed, 40 insertions(+), 1 deletion(-)
>  create mode 100644 include/dt-bindings/iio/adc/adi,ad7768-1.h
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
> index 3ce59d4d065f..1f476aa15305 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml
> @@ -26,7 +26,28 @@ properties:
>    clock-names:
>      const: mclk
>  
> +  trigger-sources:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    minItems: 1
> +    maxItems: 2
> +    description: |
> +      A list of phandles referencing trigger source providers. Each entry
> +      represents a trigger source for the ADC:
> +
> +        - First entry specifies the device responsible for driving the
> +          synchronization (SYNC_IN) pin, as an alternative to adi,sync-in-gpios.
> +          This can be a `gpio-trigger` or another `ad7768-1` device. If the
> +          device's own SYNC_OUT pin is internally connected to its SYNC_IN pin,
> +          reference the device itself or omit this property.
> +        - Second entry optionally defines a GPIO3 pin used as a START signal trigger.
> +
> +      Use the accompanying trigger source cell to identify the type of each entry.
> +
>    interrupts:
> +    description:
> +      Specifies the interrupt line associated with the ADC. This refers
> +      to the DRDY (Data Ready) pin, which signals when conversion results are
> +      available.

Trivial but this seems overly wordy. The only bit that matters is the datardy bit.

     DRDY (Data Ready) pin, which signals conversion results are available.

is probably enough.  Only bother if you are respining for other reasons however
as this really doesn't matter!

>      maxItems: 1
>  
>    '#address-cells':
> @@ -57,6 +78,15 @@ properties:
>    "#io-channel-cells":
>      const: 1
>  
> +  "#trigger-source-cells":
> +    description: |
> +      Cell indicates the trigger output signal: 0 = SYNC_OUT, 1 = GPIO3,
> +      2 = DRDY.
> +
> +      For better readability, macros for these values are available in
> +      dt-bindings/iio/adc/adi,ad7768-1.h.
> +    const: 1
> +
>  required:
>    - compatible
>    - reg
> @@ -65,7 +95,6 @@ required:
>    - vref-supply
>    - spi-cpol
>    - spi-cpha
> -  - adi,sync-in-gpios

Maybe worth requiring oneOf adi,sync-in-gpios or trigger-sources? 

>  
>  patternProperties:
>    "^channel@([0-9]|1[0-5])$":
> diff --git a/include/dt-bindings/iio/adc/adi,ad7768-1.h b/include/dt-bindings/iio/adc/adi,ad7768-1.h
> new file mode 100644
> index 000000000000..34d92856a50b
> --- /dev/null
> +++ b/include/dt-bindings/iio/adc/adi,ad7768-1.h
> @@ -0,0 +1,10 @@
> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> +
> +#ifndef _DT_BINDINGS_ADI_AD7768_1_H
> +#define _DT_BINDINGS_ADI_AD7768_1_H
> +
> +#define AD7768_TRIGGER_SOURCE_SYNC_OUT  0
> +#define AD7768_TRIGGER_SOURCE_GPIO3     1
> +#define AD7768_TRIGGER_SOURCE_DRDY      2
> +
> +#endif /* _DT_BINDINGS_ADI_AD7768_1_H */

Re: [PATCH v6 01/11] dt-bindings: trigger-source: add generic GPIO trigger source

[Jonathan Cameron]

On Sun, 27 Apr 2025 21:12:02 -0300
Jonathan Santos <Jonathan.Santos@analog.com> wrote:

> Inspired by pwm-trigger, create a new binding for using a GPIO
> line as a trigger source.
> 
> Link: https://lore.kernel.org/linux-iio/20250207-dlech-mainline-spi-engine-offload-2-v8-3-e48a489be48c@baylibre.com/

David, given you did the pwm one, maybe give this a quick look.

Maybe it's worth generalising to cover all trigger sources in MAINTAINERS?

Thanks. Otherwise I obviously need a DT review before taking this and maybe the GPIO
element suggests Linus W or Bartosz might be other good reviewers?

Jonathan

> Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> ---
> v6 Changes:
> * Changed description.
> * Fixed typos and replaced GPIO pin with GPIO line.
> * Added link reference for pwm-trigger.
> 
> v5 Changes:
> * New patch in v5.
> ---
>  .../bindings/trigger-source/gpio-trigger.yaml | 40 +++++++++++++++++++
>  1 file changed, 40 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/trigger-source/gpio-trigger.yaml
> 
> diff --git a/Documentation/devicetree/bindings/trigger-source/gpio-trigger.yaml b/Documentation/devicetree/bindings/trigger-source/gpio-trigger.yaml
> new file mode 100644
> index 000000000000..1331d153ee82
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/trigger-source/gpio-trigger.yaml
> @@ -0,0 +1,40 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/trigger-source/gpio-trigger.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Generic trigger source using GPIO
> +
> +description: A GPIO used as a trigger source.
> +
> +maintainers:
> +  - Jonathan Santos <Jonathan.Santos@analog.com>
> +
> +properties:
> +  compatible:
> +    const: gpio-trigger
> +
> +  '#trigger-source-cells':
> +    const: 0
> +
> +  gpios:
> +    maxItems: 1
> +    description: GPIO to be used as a trigger source.
> +
> +required:
> +  - compatible
> +  - '#trigger-source-cells'
> +  - gpios
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    trigger {
> +        compatible = "gpio-trigger";
> +        #trigger-source-cells = <0>;
> +        gpios = <&gpio1 15 GPIO_ACTIVE_HIGH>;
> +    };

Re: [PATCH v6 07/11] iio: adc: ad7768-1: add multiple scan types to support 16-bits mode

[Jonathan Cameron]

On Mon, 28 Apr 2025 09:55:44 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Mon, Apr 28, 2025 at 3:13 AM Jonathan Santos
> <Jonathan.Santos@analog.com> wrote:
> >
> > When the device is configured to decimation x8, only possible in the
> > sinc5 filter, 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  
> 
> at runtime
> making it possible
> 
> > 16-bit resolution.  
> 
> ...
> 
> > +       ret = spi_read(st->spi, &st->data.scan.chan,
> > +                      BITS_TO_BYTES(scan_type->realbits));
> >         if (ret < 0)
> >                 goto out;  
> 
> Add a TODO to convert this to use a new helper from 163ddf1fea59.

Mostly because I couldn't immediately track this down in any tree
I had locally.. 

https://web.git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=163ddf1fea59


> 

Re: [PATCH v6 01/11] dt-bindings: trigger-source: add generic GPIO trigger source

[David Lechner]

On 5/5/25 10:44 AM, Jonathan Cameron wrote:
> On Sun, 27 Apr 2025 21:12:02 -0300
> Jonathan Santos <Jonathan.Santos@analog.com> wrote:
> 
>> Inspired by pwm-trigger, create a new binding for using a GPIO
>> line as a trigger source.
>>
>> Link: https://lore.kernel.org/linux-iio/20250207-dlech-mainline-spi-engine-offload-2-v8-3-e48a489be48c@baylibre.com/
> 
> David, given you did the pwm one, maybe give this a quick look.

LGTM.

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

> 
> Maybe it's worth generalising to cover all trigger sources in MAINTAINERS?

Sure, I would be OK with that.

> 
> Thanks. Otherwise I obviously need a DT review before taking this and maybe the GPIO
> element suggests Linus W or Bartosz might be other good reviewers?

Linus W already replied in v5:

https://lore.kernel.org/linux-iio/cover.1744325346.git.Jonathan.Santos@analog.com/T/#mb263ff61984dae4a479632dbe33c94a66659fd42

Re: [PATCH v6 08/11] iio: adc: ad7768-1: add support for Synchronization over SPI

[Jonathan Cameron]

On Sun, 27 Apr 2025 21:13:47 -0300
Jonathan Santos <Jonathan.Santos@analog.com> 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.
> 
> Use trigger-sources property to enable device synchronization over SPI
> and multi-device synchronization, as an alternative to adi,sync-in-gpios
> property.
> 

> +static int ad7768_trigger_sources_get_sync(struct device *dev,
> +					   struct ad7768_state *st)
> +{
> +	struct fwnode_reference_args args;
> +	struct fwnode_handle *fwnode = NULL;
> +	int ret;
> +
> +	/*
> +	 * The AD7768-1 allows two primary methods for driving the SYNC_IN pin
> +	 * to synchronize one or more devices:
> +	 * 1. Using an external GPIO.
> +	 * 2. Using a SPI command, where the SYNC_OUT pin generates a
> +	 *    synchronization pulse that drives the SYNC_IN pin.
> +	 */
> +	if (!device_property_present(dev, "trigger-sources")) {
> +		/*
> +		 * In the absence of trigger-sources property, enable self
> +		 * synchronization over SPI (SYNC_OUT).
> +		 */
> +		st->en_spi_sync = true;
> +		return 0;
> +	}
> +
> +	ret = fwnode_property_get_reference_args(dev_fwnode(dev),
> +						 "trigger-sources",
> +						 "#trigger-source-cells",
> +						 0,
> +						 AD7768_TRIGGER_SOURCE_SYNC_IDX,
> +						 &args);
> +	if (ret)
> +		return ret;
> +
> +	fwnode = args.fwnode;
> +	/*
> +	 * First, try getting the GPIO trigger source and fallback to
> +	 * synchronization over SPI in case of failure.
> +	 */
> +	st->gpio_sync_in = ad7768_trigger_source_get_gpio(dev, fwnode);
> +	if (IS_ERR(st->gpio_sync_in)) {
> +		/*
> +		 * For this case, it requires one argument, which indicates the
> +		 * output pin referenced.
> +		 */
> +		if (args.nargs < 1)
> +			goto err_not_supp;
> +
> +		if (args.args[0] != AD7768_TRIGGER_SOURCE_SYNC_OUT)
> +			goto err_not_supp;
> +
> +		/*
> +		 * Only self trigger is supported for now, i.e.,
> +		 * external SYNC_OUT is not allowed.
> +		 */
> +		if (fwnode->dev == dev) {
> +			st->en_spi_sync = true;
> +			goto out_put_node;
> +		}
> +
> +		goto err_not_supp;
> +	}
> +
> +	goto out_put_node;
> +
> +err_not_supp:

Split the good and bad paths here so we don't have good paths jumping to
mid way through the block. If the good paths need to jump then I'd
suggest factoring out the stuff with the node held into a separate function.
Superficially it just looks like one condition flip and you are refactoring
this anyway based on other feedback so maybe this comment becomes irrelevant!


> +	ret = dev_err_probe(dev, -EOPNOTSUPP,
> +			    "Invalid synchronization trigger source");
> +out_put_node:
> +	fwnode_handle_put(args.fwnode);
> +	return ret;
> +}
> +

Re: [PATCH v6 10/11] iio: adc: ad7768-1: add filter type and oversampling ratio attributes

[Jonathan Cameron]

On Sun, 27 Apr 2025 21:14:17 -0300
Jonathan Santos <Jonathan.Santos@analog.com> wrote:

> Separate filter type and decimation rate from the sampling frequency
> attribute. The new filter type attribute enables sinc3, sinc3+rej60
> 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 an oversampling
> attribute, which controls the decimation, 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. Sampling frequency
> table is updated after every digital filter parameter change.
> 
> Reviewed-by: David Lechner <dlechner@baylibre.com>
> Co-developed-by: Pop Paul <paul.pop@analog.com>
> Signed-off-by: Pop Paul <paul.pop@analog.com>
> Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
A few trivial additions from me.

> ---
> v6 Changes:
> * Made sinc3 decimation rate calculation clearer as requested.
> * Renamed some filter functions to clarify the purpose.
> * Other nits.
> 
> v5 Changes:
> * Addressed some nits.
> * Use the new new iio_device_claim/release_direct() functions.
> 
> v4 Changes:
> * Sampling frequency table is dynamically updated after every
>   filter configuration.
> 
> v3 Changes:
> * removed unused variables.
> * included sinc3+rej60 filter type.
> * oversampling_ratio moved to info_mask_shared_by_type.
> * reordered functions to avoid forward declaration.
> * simplified regmap writes.
> * Removed locking.
> * replaced some helper functions for direct regmap_update_bits
>   calls.
> * Addressed other nits.
> 
> v2 Changes:
> * Decimation_rate attribute replaced for oversampling_ratio.
> ---
>  drivers/iio/adc/ad7768-1.c | 363 ++++++++++++++++++++++++++++++-------
>  1 file changed, 293 insertions(+), 70 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> index 10791a85d2c5..e2b8f12260a5 100644
> --- a/drivers/iio/adc/ad7768-1.c
> +++ b/drivers/iio/adc/ad7768-1.c
> @@ -20,6 +20,8 @@
>  #include <linux/regulator/driver.h>
>  #include <linux/sysfs.h>
>  #include <linux/spi/spi.h>
> +#include <linux/unaligned.h>
> +#include <linux/util_macros.h>
>  
>  #include <linux/iio/buffer.h>
>  #include <linux/iio/iio.h>
> @@ -77,7 +79,7 @@
>  #define AD7768_PWR_PWRMODE(x)		FIELD_PREP(AD7768_PWR_PWRMODE_MSK, x)
>  
>  /* AD7768_REG_DIGITAL_FILTER */
> -#define AD7768_DIG_FIL_FIL_MSK		GENMASK(6, 4)
> +#define AD7768_DIG_FIL_FIL_MSK		GENMASK(7, 4)

Bug?  If so does this belong in a precursor patch?

>  #define AD7768_DIG_FIL_FIL(x)		FIELD_PREP(AD7768_DIG_FIL_FIL_MSK, x)

> @@ -404,22 +473,110 @@ static int ad7768_reg_access(struct iio_dev *indio_dev,
>  	return ret;
>  }
>  
> -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 mode;
> +	unsigned int max_dec_rate;
> +	u8 dec_rate_reg[2];
> +	u16 regval;
>  	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);
> +	/*
> +	 * Maximum dec_rate is limited by the MCLK_DIV value

Oddly short wrap. Go nearer 80 chars.

> +	 * and by the ODR. The edge case is for MCLK_DIV = 2
> +	 * ODR = 50 SPS.
> +	 * max_dec_rate <= MCLK / (2 * 50)
> +	 */

> +static int ad7768_get_filter_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 = regmap_read(st->regmap, AD7768_REG_DIGITAL_FILTER, &mode);
> +	if (ret)
> +		return ret;
> +
> +	mode = FIELD_GET(AD7768_DIG_FIL_FIL_MSK, mode);
> +
> +	/* From the register value, get the corresponding filter type */
> +	return ad7768_filter_regval_to_type[mode];
	return ad7768_filter_regval_to_type[FIELD_GET(AD7768_DIG_FIL_FIL_MSK, mode)];

Is fine (only just over 80 chars and I'm getting more relaxed as time moves forwards).

Also avoids the dual meaning of mode which I never like to see.

>  }

>  
> -static int ad7768_write_raw(struct iio_dev *indio_dev,
> -			    struct iio_chan_spec const *chan,
> -			    int val, int val2, long info)
> +static int __ad7768_write_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan,
> +			      int val, int val2, long info)
>  {
>  	struct ad7768_state *st = iio_priv(indio_dev);
> +	int ret;
>  
>  	switch (info) {
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		return ad7768_set_freq(st, val);

Whilst I doubt anyone will notice this looks like a functional change
that should be called out in the patch description.
Previously we allowed frequency changes in buffered mode, now we don't.

> +
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		ret = ad7768_configure_dig_fil(indio_dev, st->filter_type, val);
> +		if (ret)
> +			return ret;
> +
> +		/* Update sampling frequency */
> +		return ad7768_set_freq(st, st->samp_freq);
>  	default:
>  		return -EINVAL;
>  	}
>  }
>  
> +static int ad7768_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int val, int val2, long info)
> +{
> +	int ret;
> +
> +	if (!iio_device_claim_direct(indio_dev))
> +		return -EBUSY;
> +
> +	ret = __ad7768_write_raw(indio_dev, chan, val, val2, info);
> +	iio_device_release_direct(indio_dev);
> +
> +	return ret;
> +}
;

Re: [PATCH v6 10/11] iio: adc: ad7768-1: add filter type and oversampling ratio attributes

[Jonathan Santos]

On 05/05, Jonathan Cameron wrote:
> On Sun, 27 Apr 2025 21:14:17 -0300
> Jonathan Santos <Jonathan.Santos@analog.com> wrote:
> 
...
>  drivers/iio/adc/ad7768-1.c | 363 ++++++++++++++++++++++++++++++-------
> >  1 file changed, 293 insertions(+), 70 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> > index 10791a85d2c5..e2b8f12260a5 100644
> > --- a/drivers/iio/adc/ad7768-1.c
> > +++ b/drivers/iio/adc/ad7768-1.c
> > @@ -20,6 +20,8 @@
> >  #include <linux/regulator/driver.h>
> >  #include <linux/sysfs.h>
> >  #include <linux/spi/spi.h>
> > +#include <linux/unaligned.h>
> > +#include <linux/util_macros.h>
> >  
> >  #include <linux/iio/buffer.h>
> >  #include <linux/iio/iio.h>
> > @@ -77,7 +79,7 @@
> >  #define AD7768_PWR_PWRMODE(x)		FIELD_PREP(AD7768_PWR_PWRMODE_MSK, x)
> >  
> >  /* AD7768_REG_DIGITAL_FILTER */
> > -#define AD7768_DIG_FIL_FIL_MSK		GENMASK(6, 4)
> > +#define AD7768_DIG_FIL_FIL_MSK		GENMASK(7, 4)
> 
> Bug?  If so does this belong in a precursor patch?
> 

Actually not, this extra bit is to include the 60Hz rejection enable
for sinc3 filter

> >  #define AD7768_DIG_FIL_FIL(x)		FIELD_PREP(AD7768_DIG_FIL_FIL_MSK, x)
> 
...
 

Re: [PATCH v6 10/11] iio: adc: ad7768-1: add filter type and oversampling ratio attributes

[David Lechner]

On 5/6/25 2:03 PM, Jonathan Santos wrote:
> On 05/05, Jonathan Cameron wrote:
>> On Sun, 27 Apr 2025 21:14:17 -0300
>> Jonathan Santos <Jonathan.Santos@analog.com> wrote:
>>
> ...
>>  drivers/iio/adc/ad7768-1.c | 363 ++++++++++++++++++++++++++++++-------
>>>  1 file changed, 293 insertions(+), 70 deletions(-)
>>>
>>> diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
>>> index 10791a85d2c5..e2b8f12260a5 100644
>>> --- a/drivers/iio/adc/ad7768-1.c
>>> +++ b/drivers/iio/adc/ad7768-1.c
>>> @@ -20,6 +20,8 @@
>>>  #include <linux/regulator/driver.h>
>>>  #include <linux/sysfs.h>
>>>  #include <linux/spi/spi.h>
>>> +#include <linux/unaligned.h>
>>> +#include <linux/util_macros.h>
>>>  
>>>  #include <linux/iio/buffer.h>
>>>  #include <linux/iio/iio.h>
>>> @@ -77,7 +79,7 @@
>>>  #define AD7768_PWR_PWRMODE(x)		FIELD_PREP(AD7768_PWR_PWRMODE_MSK, x)
>>>  
>>>  /* AD7768_REG_DIGITAL_FILTER */
>>> -#define AD7768_DIG_FIL_FIL_MSK		GENMASK(6, 4)
>>> +#define AD7768_DIG_FIL_FIL_MSK		GENMASK(7, 4)
>>
>> Bug?  If so does this belong in a precursor patch?
>>
> 
> Actually not, this extra bit is to include the 60Hz rejection enable
> for sinc3 filter

Seems odd to me to group those together since they are two separate fields in
the register. It would make more sense to have a separate BIT(7) for the
EN_60HZ_REJ bit.

If we need to manipulate both at the same time in the driver, then we would
use AD7768_DIG_FIL_EN_60HZ_REJ | AD7768_DIG_FIL_FIL_MSK.

> 
>>>  #define AD7768_DIG_FIL_FIL(x)		FIELD_PREP(AD7768_DIG_FIL_FIL_MSK, x)
>>
> ...
>  

Re: [PATCH v6 02/11] dt-bindings: iio: adc: ad7768-1: add trigger-sources property

[Jonathan Santos]

On 05/05, Jonathan Cameron wrote:
> On Sun, 27 Apr 2025 21:12:16 -0300
> Jonathan Santos <Jonathan.Santos@analog.com> wrote:
> 
...
> >  required:
> >    - compatible
> >    - reg
> > @@ -65,7 +95,6 @@ required:
> >    - vref-supply
> >    - spi-cpol
> >    - spi-cpha
> > -  - adi,sync-in-gpios
> 
> Maybe worth requiring oneOf adi,sync-in-gpios or trigger-sources? 
> 

We cannot do that because we defined that self triggering is enabled
when trigger-sources is omitted (is this case adi,sync-in-gpios is not
present as well).

> >  
> >  patternProperties:
> >    "^channel@([0-9]|1[0-5])$":
> > diff --git a/include/dt-bindings/iio/adc/adi,ad7768-1.h b/include/dt-bindings/iio/adc/adi,ad7768-1.h
> > new file mode 100644
> > index 000000000000..34d92856a50b
> > --- /dev/null
> > +++ b/include/dt-bindings/iio/adc/adi,ad7768-1.h
> > @@ -0,0 +1,10 @@
> > +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> > +
> > +#ifndef _DT_BINDINGS_ADI_AD7768_1_H
> > +#define _DT_BINDINGS_ADI_AD7768_1_H
> > +
> > +#define AD7768_TRIGGER_SOURCE_SYNC_OUT  0
> > +#define AD7768_TRIGGER_SOURCE_GPIO3     1
> > +#define AD7768_TRIGGER_SOURCE_DRDY      2
> > +
> > +#endif /* _DT_BINDINGS_ADI_AD7768_1_H */
> 

Re: [PATCH v6 02/11] dt-bindings: iio: adc: ad7768-1: add trigger-sources property

[Jonathan Cameron]

On Tue, 6 May 2025 16:16:37 -0300
Jonathan Santos <jonath4nns@gmail.com> wrote:

> On 05/05, Jonathan Cameron wrote:
> > On Sun, 27 Apr 2025 21:12:16 -0300
> > Jonathan Santos <Jonathan.Santos@analog.com> wrote:
> >   
> ...
> > >  required:
> > >    - compatible
> > >    - reg
> > > @@ -65,7 +95,6 @@ required:
> > >    - vref-supply
> > >    - spi-cpol
> > >    - spi-cpha
> > > -  - adi,sync-in-gpios  
> > 
> > Maybe worth requiring oneOf adi,sync-in-gpios or trigger-sources? 
> >   
> 
> We cannot do that because we defined that self triggering is enabled
> when trigger-sources is omitted (is this case adi,sync-in-gpios is not
> present as well).
Ah. Fair enough.  More complex but we can constrain to never having
both adi,sync-in-gpios and trigger-sources at the same time.

> 
> > >  
> > >  patternProperties:
> > >    "^channel@([0-9]|1[0-5])$":
> > > diff --git a/include/dt-bindings/iio/adc/adi,ad7768-1.h b/include/dt-bindings/iio/adc/adi,ad7768-1.h
> > > new file mode 100644
> > > index 000000000000..34d92856a50b
> > > --- /dev/null
> > > +++ b/include/dt-bindings/iio/adc/adi,ad7768-1.h
> > > @@ -0,0 +1,10 @@
> > > +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> > > +
> > > +#ifndef _DT_BINDINGS_ADI_AD7768_1_H
> > > +#define _DT_BINDINGS_ADI_AD7768_1_H
> > > +
> > > +#define AD7768_TRIGGER_SOURCE_SYNC_OUT  0
> > > +#define AD7768_TRIGGER_SOURCE_GPIO3     1
> > > +#define AD7768_TRIGGER_SOURCE_DRDY      2
> > > +
> > > +#endif /* _DT_BINDINGS_ADI_AD7768_1_H */  
> >   

Re: [PATCH v6 01/11] dt-bindings: trigger-source: add generic GPIO trigger source

[Linus Walleij]

On Mon, Apr 28, 2025 at 2:12 AM Jonathan Santos
<Jonathan.Santos@analog.com> wrote:

> Inspired by pwm-trigger, create a new binding for using a GPIO
> line as a trigger source.
>
> Link: https://lore.kernel.org/linux-iio/20250207-dlech-mainline-spi-engine-offload-2-v8-3-e48a489be48c@baylibre.com/
> Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij