[PATCH 0/8] Add iio backend compatibility for ad7606

[PATCH 0/8] Add iio backend compatibility for ad7606

[Guillaume Stols]

This series aims to add iio backend support for AD7606X ADCs.

In a nutshell, iio backend is a paradigm to shift the logic establishing
the connexion between iio buffers and backend buffers into the backend's
driver.  This provides a more stable programming interface to the driver
developers, and give more flexibility in the way the hardware communicates.

The support will be first added on AD7606B, and on next patches AD7606C16
and AD7606C18 will be added.  The series have been tested on a Zedboard,
using the latest HDL available, i.e
https://github.com/analogdevicesinc/hdl/commit/7d0a4cee1b5fa403f175af513d7eb804c3bd75d0
and an AD7606B FMCZ EKV.  This HDL handles both the conversion trigger
(through a PWM), and the end of conversion interruption, and is compatible
with axi-adc, which is "iio-backendable".

More information about this HDL design can be found at:
https://wiki.analog.com/resources/eval/user-guides/ad7606x-fmc/hdl

The support is thus separated in two parts:

- PWM support was first added.  My first intention was to make it available
  for any version of the driver, but the time required to handle the
  interruption is not neglectable, and I saw drifts that would eventually
  cause an overlapping SPI read with a new conversion trigger, whith
  catastrphic consequences. To mitigate this, CRC check must be
  implemented, but indeed increasing the samplerate causes more sample to
  be lost.  Therefore, I decided to only allow PWM for iio-backend
  powered device as a first intention, leaving open the possibility to
  add the general compatibility afterwards.

- IIO backend support was added: Once the PWM support was ready, the driver
  can be extended to iio-backend. The iio-backend powered version of the
  driver is a platform driver, and an exemple devicetree node is available
  in the bindings.

The following features will be added in subsequent patch series:
 - software mode for iio backend
 - 18 bits mode (AD7606C18)
 - single read (IIO_CHAN_READ_RAW)

Signed-off-by: Guillaume Stols <gstols@baylibre.com>
---
Guillaume Stols (8):
      dt-bindings: iio: adc: ad7606: Make corrections on spi conditions
      dt-bindings: iio: adc: ad7606: Add iio backend bindings
      Documentation: iio: Document ad7606 driver
      pwm: Export pwm_get_state_hw
      platform: add platform_get_device_match_data() helper
      iio: adc: ad7606: Add PWM support for conversion trigger
      iio: adc: ad7606: Switch to xxx_get_device_match_data
      iio:adc:ad7606: Add iio-backend support

 .../devicetree/bindings/iio/adc/adi,ad7606.yaml    |  90 ++++-
 Documentation/iio/ad7606.rst                       | 142 ++++++++
 drivers/base/platform.c                            |  12 +
 drivers/iio/adc/Kconfig                            |   3 +-
 drivers/iio/adc/ad7606.c                           | 363 +++++++++++++--------
 drivers/iio/adc/ad7606.h                           | 151 ++++++++-
 drivers/iio/adc/ad7606_par.c                       | 120 ++++++-
 drivers/iio/adc/ad7606_spi.c                       |  31 +-
 drivers/pwm/core.c                                 |   3 +-
 include/linux/platform_device.h                    |   1 +
 include/linux/pwm.h                                |   1 +
 11 files changed, 733 insertions(+), 184 deletions(-)
---
base-commit: 7cad163c39cb642ed587d3eeb37a5637ee02740f
change-id: 20240725-ad7606_add_iio_backend_support-c401305a6924
prerequisite-message-id: 20240705211452.1157967-2-u.kleine-koenig@baylibre.com
prerequisite-patch-id: 0e21153cd012f41ba9db52357fd08219af53e26c
prerequisite-message-id: 20240712171821.1470833-2-u.kleine-koenig@baylibre.com
prerequisite-patch-id: b22c91bbc4e3412f8e7e1f796ed18570ae021c96
prerequisite-message-id: cover.1721040875.git.u.kleine-koenig@baylibre.com
prerequisite-patch-id: bfc36d041b9e5d417c6b18268dd91171d627d04e
prerequisite-patch-id: adec4b066442de64275ebc3bd310ebaea99a0e8d
prerequisite-patch-id: b536b9607ae40bd58f3e56c4ccd304b7880b5b90
prerequisite-patch-id: fe43e064fe174b830d5a11f83e3cd7252089820e
prerequisite-patch-id: a1cd565094d86ff473724db1fd6dbb61aca996dd
prerequisite-patch-id: d7b5d697839f0a6cea0aa37810df4d02a7762ead
prerequisite-patch-id: e86302e513cfdf80831da4d79a7a950eecf7c557
prerequisite-patch-id: 05b25465694c5640e42e67d2059e84f34e259670

Best regards,
-- 
Guillaume Stols <gstols@baylibre.com>

[PATCH 1/8] dt-bindings: iio: adc: ad7606: Make corrections on spi conditions

[Guillaume Stols]

The SPI conditions are not always required, because there is also a
parallel interface. The way used to detect that the SPI interface is
used is to check if the reg value is between 0 and 256.
There is also a correction on the spi-cpha that is not required when SPI
interface is selected, while spi-cpol is.

Signed-off-by: Guillaume Stols <gstols@baylibre.com>
---
 .../devicetree/bindings/iio/adc/adi,ad7606.yaml         | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
index 69408cae3db9..c0008d36320f 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
@@ -117,15 +117,26 @@ properties:
 required:
   - compatible
   - reg
-  - spi-cpha
   - avcc-supply
   - vdrive-supply
   - interrupts
   - adi,conversion-start-gpios
 
-allOf:
-  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+# This checks if reg is a chipselect so the device is on an SPI
+# bus, the if-clause will fail if reg is a tuple such as for a
+# platform device.
+if:
+  properties:
+    reg:
+      minimum: 0
+      maximum: 256
+then:
+  allOf:
+    - $ref: /schemas/spi/spi-peripheral-props.yaml#
+    - required:
+        - spi-cpol
 
+allOf:
   - if:
       properties:
         compatible:

-- 
2.34.1

[PATCH 2/8] dt-bindings: iio: adc: ad7606: Add iio backend bindings

[Guillaume Stols]

Add the required properties for iio-backend support, as well as an
example and the conditions to mutually exclude interruption and
conversion trigger with iio-backend.
The iio-backend's function is to controls the communication, and thus the
interruption pin won't be available anymore.
As a consequence, the conversion pin must be controlled externally since
we will miss information about when every single conversion cycle (i.e
conversion + data transfert) ends, hence a PWM is introduced to trigger
the conversions.

Signed-off-by: Guillaume Stols <gstols@baylibre.com>
---
 .../devicetree/bindings/iio/adc/adi,ad7606.yaml    | 75 +++++++++++++++++++++-
 1 file changed, 72 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
index c0008d36320f..4b324f7e3207 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
@@ -114,13 +114,28 @@ properties:
       assumed that the pins are hardwired to VDD.
     type: boolean
 
+  pwms:
+    description:
+      In case the conversion is triggered by a PWM instead of a GPIO plugged to
+      the CONVST pin, the PWM must be referenced.
+    minItems: 1
+    maxItems: 2
+
+  pwm-names:
+    minItems: 1
+    maxItems: 2
+
+  io-backends:
+    description:
+      A reference to the iio-backend, which is responsible handling the BUSY
+      pin's falling edge and communication.
+      An example of backend can be found at
+      http://analogdevicesinc.github.io/hdl/library/axi_ad7606x/index.html
+
 required:
   - compatible
-  - reg
   - avcc-supply
   - vdrive-supply
-  - interrupts
-  - adi,conversion-start-gpios
 
 # This checks if reg is a chipselect so the device is on an SPI
 # bus, the if-clause will fail if reg is a tuple such as for a
@@ -137,6 +152,35 @@ then:
         - spi-cpol
 
 allOf:
+  # Communication is handled either by the backend or an interrupt.
+  - if:
+      properties:
+        pwms: false
+    then:
+      required:
+        - adi,conversion-start-gpios
+
+  - if:
+      properties:
+        adi,conversion-start-gpios: false
+    then:
+      required:
+        - pwms
+
+  - if:
+      properties:
+        interrupts: false
+    then:
+      required:
+        - io-backends
+
+  - if:
+      properties:
+        io-backends: false
+    then:
+      required:
+        - interrupts
+
   - if:
       properties:
         compatible:
@@ -178,12 +222,37 @@ allOf:
         adi,sw-mode: false
     else:
       properties:
+        pwms:
+          maxItems: 1
+        pwm-names:
+          maxItems: 1
         adi,conversion-start-gpios:
           maxItems: 1
 
 unevaluatedProperties: false
 
 examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    / {
+        adi_adc {
+                compatible = "adi,ad7606b";
+
+                pwms = <&axi_pwm_gen 0 0>;
+
+                avcc-supply = <&adc_vref>;
+                vdrive-supply = <&vdd_supply>;
+
+                reset-gpios = <&gpio0 91 GPIO_ACTIVE_HIGH>;
+                standby-gpios = <&gpio0 90 GPIO_ACTIVE_LOW>;
+                adi,range-gpios = <&gpio0 89 GPIO_ACTIVE_HIGH>;
+                adi,oversampling-ratio-gpios = <&gpio0 88 GPIO_ACTIVE_HIGH
+                                                &gpio0 87 GPIO_ACTIVE_HIGH
+                                                &gpio0 86 GPIO_ACTIVE_HIGH>;
+                io-backends = <&iio_backend>;
+        };
+    };
+
   - |
     #include <dt-bindings/gpio/gpio.h>
     #include <dt-bindings/interrupt-controller/irq.h>

-- 
2.34.1

[PATCH 3/8] Documentation: iio: Document ad7606 driver

[Guillaume Stols]

The Analog Devices Inc. AD7606 (and similar chips) are complex ADCs that
will benefit from a detailed driver documentation.

This documents the current features supported by the driver.

Signed-off-by: Guillaume Stols <gstols@baylibre.com>
---
 Documentation/iio/ad7606.rst | 142 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 142 insertions(+)

diff --git a/Documentation/iio/ad7606.rst b/Documentation/iio/ad7606.rst
new file mode 100644
index 000000000000..e9578399e80d
--- /dev/null
+++ b/Documentation/iio/ad7606.rst
@@ -0,0 +1,142 @@
+.. SPDX-License-Identifier: GPL-2.0-only
+
+=============
+AD7606 driver
+=============
+
+ADC driver for Analog Devices Inc. AD7606 and similar devices. The module name
+is ``ad7606``.
+
+Supported devices
+=================
+
+The following chips are supported by this driver:
+
+* `AD7605 <https://www.analog.com/en/products/ad7605.html>`_
+* `AD7606 <https://www.analog.com/en/products/ad7606.html>`_
+* `AD7606B <https://www.analog.com/en/products/ad7606b.html>`_
+* `AD7616 <https://www.analog.com/en/products/ad7616.html>`_
+
+Supported features
+==================
+
+SPI wiring modes
+----------------
+
+ad7606x ADCs can output data on several SDO lines (1/2/4/8). The driver
+currently supports only 1 SDO line.
+
+Parallel wiring mode
+--------------------
+
+AD7606x ADC have also a parallel interface, with 16 lines (that can be reduced
+to 8 in byte mode). The parallel interface is selected by declaring the device
+as platform in the device tree (with no io-backends node defined, see below).
+
+IIO-backend mode
+----------------
+
+This mode allows to reach the best sample rates, but it requires an external
+hardware (eg HDL or APU) to handle the low level communication.
+The backend mode is enabled when trough the definition of the "io-backends"
+property in the device tree.
+The reference configuration for the current implementation of IIO-backend mode
+is the HDL reference provided by ADI:
+https://wiki.analog.com/resources/eval/user-guides/ad7606x-fmc/hdl
+This implementation embeds an IIO-backend compatible IP (adi-axi-adc) and a PWM
+connected to the conversion trigger pin.
+
++---+                                       +----------------------------
+|   |               +-------+               |AD76xx
+| A |  controls     |       |               |
+| D |-------------->|  PWM  |-------------->| cnvst
+| 7 |               |       |               |
+| 6 |               +-------+               |
+| 0 | controls  +-----------+------------   |
+| 6 |---------->|           |           |<--| frstdata
+|   |           | Backend   |  Backend  |<--| busy
+| D |           | Driver    |           |   |
+| R |           |           |           |-->| clk
+| I |  requests |+---------+| DMA       |   |
+| V |----------->|  Buffer ||<----      |<=>| DATA
+| E |           |+---------+|           |   |
+| R |           +-----------+-----------+   |
+|   |-------------------------------------->| reset/configuration gpios
++---+                                       +-----------------------------
+
+
+Software and hardware modes
+---------------------------
+
+While all the AD7606 series parts can be configured using GPIOs, some of them
+can be configured using register.
+The chip that support software mode have more values avalaible for configuring
+the device, as well as more settings, and allow to control the range and
+calibration per channel.
+The following settings are available per channel in software mode:
+ - Scale
+Also, there is a broader choice of oversampling ratios in software mode.
+
+Conversion triggering
+---------------------
+
+The conversion can be triggered by two distinct ways:
+
+ - A GPIO is connected to the conversion trigger pin, and this GPIO is controlled
+   by the driver directly.  In this configuration, the driver set back the
+   conversion trigger pin to high as soon as it has read all the conversions.
+
+ - An external source is connected to the conversion trigger pin. In the
+   current implementation, it must be a PWM. In this configuration, the driver
+   does not control directly the conversion trigger pin. Instead, it can
+   control the PWM's frequency. This trigger is enabled only for iio-backend.
+
+Reference voltage
+-----------------
+
+2 possible reference voltage sources are supported:
+
+ - Internal reference (2.5V)
+ - External reference (2.5V)
+
+The source is determined by the device tree. If ``refin-supply`` is present,
+then the external reference is used, else the internal reference is used.
+
+Oversampling
+------------
+
+This family supports oversampling to improve SNR.
+In software mode, the following ratios are available:
+1 (oversampling disabled)/2/4/8/16/32/64/128/256.
+
+Unimplemented features
+----------------------
+
+- 2/4/8 SDO lines
+- CRC indication
+- Calibration
+
+Device buffers
+==============
+
+IIO triggered buffer
+--------------------
+
+This driver supports IIO triggered buffers, with a "built in" trigger, i.e the
+trigger is allocated and linked by the driver, and a new conversion is triggered
+as soon as the samples are transferred, and a timestamp channel is added to make
+up for the potential jitter induced by the delays in the interrupt handling.
+
+IIO backend buffer
+------------------
+
+When IIO backend is used, the trigger is not needed, and the sample rate is
+considered as stable, hence there is no timestamp channel. The communication is
+delegated to an external logic, called a backend, and the backend's driver
+handles the buffer. When this mode is enabled, the driver cannot control the
+conversion pin, because the busy pin is bound to the backend.
+
+
+
+
+

-- 
2.34.1

[PATCH 4/8] pwm: Export pwm_get_state_hw

[Guillaume Stols]

This function can be used in some other drivers, for instance when we
want to retrieve the real frequency vs the one that was asked.

Signed-off-by: Guillaume Stols <gstols@baylibre.com>
---
 drivers/pwm/core.c  | 3 ++-
 include/linux/pwm.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 21fca27bb8a3..82e05ed88310 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -651,7 +651,7 @@ int pwm_apply_atomic(struct pwm_device *pwm, const struct pwm_state *state)
 }
 EXPORT_SYMBOL_GPL(pwm_apply_atomic);
 
-static int pwm_get_state_hw(struct pwm_device *pwm, struct pwm_state *state)
+int pwm_get_state_hw(struct pwm_device *pwm, struct pwm_state *state)
 {
 	struct pwm_chip *chip = pwm->chip;
 	const struct pwm_ops *ops = chip->ops;
@@ -685,6 +685,7 @@ static int pwm_get_state_hw(struct pwm_device *pwm, struct pwm_state *state)
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(pwm_get_state_hw);
 
 /**
  * pwm_adjust_config() - adjust the current PWM config to the PWM arguments
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index fd100c27f109..d48ea3051e28 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -369,6 +369,7 @@ int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state)
 int pwm_apply_atomic(struct pwm_device *pwm, const struct pwm_state *state);
 int pwm_adjust_config(struct pwm_device *pwm);
 
+int pwm_get_state_hw(struct pwm_device *pwm, struct pwm_state *state);
 /**
  * pwm_config() - change a PWM device configuration
  * @pwm: PWM device

-- 
2.34.1

[PATCH 5/8] platform: add platform_get_device_match_data() helper

[Guillaume Stols]

Inspired from spi_get_device_match_data() helper: if OF or ACPI driver
data is available, platform_get_device_id will return NULL because
platform_match_id is not called, and pdev->id_entry is never populated.
This helper function checks if match data is available, and otherwise
returns the ID table data.

Signed-off-by: Guillaume Stols <gstols@baylibre.com>
---
 drivers/base/platform.c         | 12 ++++++++++++
 include/linux/platform_device.h |  1 +
 2 files changed, 13 insertions(+)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 4c3ee6521ba5..26e9a026eb05 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -1090,6 +1090,18 @@ static const struct platform_device_id *platform_match_id(
 	return NULL;
 }
 
+const void *platform_get_device_match_data(const struct platform_device *pdev)
+{
+	const struct platform_device_id *match;
+
+	match = device_get_match_data(&pdev->dev);
+	if (match)
+		return match;
+
+	return (const void *)platform_get_device_id(pdev)->driver_data;
+}
+EXPORT_SYMBOL_GPL(platform_get_device_match_data);
+
 #ifdef CONFIG_PM_SLEEP
 
 static int platform_legacy_suspend(struct device *dev, pm_message_t mesg)
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index d422db6eec63..e2cc09ecc447 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -45,6 +45,7 @@ struct platform_device {
 };
 
 #define platform_get_device_id(pdev)	((pdev)->id_entry)
+extern const void *platform_get_device_match_data(const struct platform_device *pdev);
 
 #define dev_is_platform(dev) ((dev)->bus == &platform_bus_type)
 #define to_platform_device(x) container_of((x), struct platform_device, dev)

-- 
2.34.1

[PATCH 6/8] iio: adc: ad7606: Add PWM support for conversion trigger

[Guillaume Stols]

Until now, the conversion were triggered by setting high the GPIO
connected to the convst pin. This commit gives the possibility to
connect the convst pin to a PWM.
Connecting a PWM allows to have a better control on the samplerate,
but it must be handled with care, as it is completely decorrelated of
the driver's busy pin handling.
Hence it is not recommended to be used "as is" but must be exploited
in conjonction with IIO backend, and for now only a sampling frequency
of 2 kHz is available.

Signed-off-by: Guillaume Stols <gstols@baylibre.com>
---
 drivers/iio/adc/ad7606.c | 162 +++++++++++++++++++++++++++++++++++++++--------
 drivers/iio/adc/ad7606.h |   3 +
 2 files changed, 140 insertions(+), 25 deletions(-)

diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index 539e4a8621fe..2c9ddcd0ad77 100644
--- a/drivers/iio/adc/ad7606.c
+++ b/drivers/iio/adc/ad7606.c
@@ -13,11 +13,13 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/property.h>
+#include <linux/pwm.h>
 #include <linux/regulator/consumer.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
 #include <linux/sysfs.h>
 #include <linux/util_macros.h>
+#include <linux/units.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/buffer.h>
@@ -82,6 +84,76 @@ static int ad7606_reg_access(struct iio_dev *indio_dev,
 	}
 }
 
+static int ad7606_pwm_set_high(struct ad7606_state *st)
+{
+	struct pwm_state cnvst_pwm_state;
+
+	if (!st->cnvst_pwm)
+		return -EINVAL;
+	pwm_get_state(st->cnvst_pwm, &cnvst_pwm_state);
+	cnvst_pwm_state.enabled = true;
+	cnvst_pwm_state.duty_cycle = cnvst_pwm_state.period;
+	return pwm_apply_might_sleep(st->cnvst_pwm, &cnvst_pwm_state);
+}
+
+static int ad7606_pwm_set_low(struct ad7606_state *st)
+{
+	struct pwm_state cnvst_pwm_state;
+
+	if (!st->cnvst_pwm)
+		return -EINVAL;
+	pwm_get_state(st->cnvst_pwm, &cnvst_pwm_state);
+	cnvst_pwm_state.enabled = true;
+	cnvst_pwm_state.duty_cycle = 0;
+	return pwm_apply_might_sleep(st->cnvst_pwm, &cnvst_pwm_state);
+}
+
+static int ad7606_pwm_set_swing(struct ad7606_state *st)
+{
+	struct pwm_state cnvst_pwm_state;
+
+	if (!st->cnvst_pwm)
+		return -EINVAL;
+	pwm_get_state(st->cnvst_pwm, &cnvst_pwm_state);
+	cnvst_pwm_state.enabled = true;
+	cnvst_pwm_state.duty_cycle = cnvst_pwm_state.period / 2;
+	return pwm_apply_might_sleep(st->cnvst_pwm, &cnvst_pwm_state);
+}
+
+static bool ad7606_pwm_is_swinging(struct ad7606_state *st)
+{
+	struct pwm_state cnvst_pwm_state;
+
+	if (!st->cnvst_pwm)
+		return false;
+	pwm_get_state(st->cnvst_pwm, &cnvst_pwm_state);
+	return cnvst_pwm_state.duty_cycle != cnvst_pwm_state.period &&
+	       cnvst_pwm_state.duty_cycle != 0;
+}
+
+static int ad7606_set_sampling_freq(struct ad7606_state *st, unsigned long freq)
+{
+	struct pwm_state cnvst_pwm_state;
+	bool is_swinging = ad7606_pwm_is_swinging(st);
+	bool is_high;
+
+	if (freq == 0)
+		return -EINVAL;
+
+	/*Retrieve the previous state */
+	pwm_get_state(st->cnvst_pwm, &cnvst_pwm_state);
+	is_high = cnvst_pwm_state.duty_cycle == cnvst_pwm_state.period;
+
+	cnvst_pwm_state.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC, freq);
+	cnvst_pwm_state.polarity = PWM_POLARITY_NORMAL;
+	if (is_high)
+		cnvst_pwm_state.duty_cycle = cnvst_pwm_state.period;
+	else if (is_swinging)
+		cnvst_pwm_state.duty_cycle = cnvst_pwm_state.period / 2;
+
+	return pwm_apply_might_sleep(st->cnvst_pwm, &cnvst_pwm_state);
+}
+
 static int ad7606_read_samples(struct ad7606_state *st)
 {
 	unsigned int num = st->chip_info->num_channels - 1;
@@ -141,9 +213,21 @@ static irqreturn_t ad7606_trigger_handler(int irq, void *p)
 static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
 {
 	struct ad7606_state *st = iio_priv(indio_dev);
+	struct pwm_state cnvst_pwm_state;
 	int ret;
+	bool pwm_swings = false;
 
-	gpiod_set_value(st->gpio_convst, 1);
+	if (st->gpio_convst) {
+		gpiod_set_value(st->gpio_convst, 1);
+	} else {
+		pwm_get_state(st->cnvst_pwm, &cnvst_pwm_state);
+		/* Keep the current PWM state to switch it back off if needed*/
+		if (ad7606_pwm_is_swinging(st))
+			pwm_swings = true;
+		ret = ad7606_pwm_set_high(st);
+		if (!ret)
+			return ret;
+	}
 	ret = wait_for_completion_timeout(&st->completion,
 					  msecs_to_jiffies(1000));
 	if (!ret) {
@@ -154,7 +238,12 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
 	ret = ad7606_read_samples(st);
 	if (ret == 0)
 		ret = st->data[ch];
-
+	if (!st->gpio_convst) {
+		if (!pwm_swings)
+			ret = ad7606_pwm_set_low(st);
+		else
+			ret = ad7606_pwm_set_swing(st);
+	}
 error_ret:
 	gpiod_set_value(st->gpio_convst, 0);
 
@@ -169,6 +258,7 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
 {
 	int ret, ch = 0;
 	struct ad7606_state *st = iio_priv(indio_dev);
+	struct pwm_state cnvst_pwm_state;
 
 	switch (m) {
 	case IIO_CHAN_INFO_RAW:
@@ -419,8 +509,9 @@ static int ad7606_request_gpios(struct ad7606_state *st)
 {
 	struct device *dev = st->dev;
 
-	st->gpio_convst = devm_gpiod_get(dev, "adi,conversion-start",
-					 GPIOD_OUT_LOW);
+	st->gpio_convst = devm_gpiod_get_optional(dev, "adi,conversion-start",
+						  GPIOD_OUT_LOW);
+
 	if (IS_ERR(st->gpio_convst))
 		return PTR_ERR(st->gpio_convst);
 
@@ -489,6 +580,7 @@ static int ad7606_buffer_postenable(struct iio_dev *indio_dev)
 	struct ad7606_state *st = iio_priv(indio_dev);
 
 	gpiod_set_value(st->gpio_convst, 1);
+	ad7606_pwm_set_swing(st);
 
 	return 0;
 }
@@ -498,6 +590,7 @@ static int ad7606_buffer_predisable(struct iio_dev *indio_dev)
 	struct ad7606_state *st = iio_priv(indio_dev);
 
 	gpiod_set_value(st->gpio_convst, 0);
+	ad7606_pwm_set_low(st);
 
 	return 0;
 }
@@ -545,6 +638,11 @@ static const struct iio_trigger_ops ad7606_trigger_ops = {
 	.validate_device = iio_trigger_validate_own_device,
 };
 
+static void ad7606_pwm_disable(void *data)
+{
+	pwm_disable(data);
+}
+
 int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
 		 const char *name, unsigned int id,
 		 const struct ad7606_bus_ops *bops)
@@ -635,20 +733,42 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
 			return ret;
 	}
 
-	st->trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
-					  indio_dev->name,
-					  iio_device_id(indio_dev));
-	if (!st->trig)
-		return -ENOMEM;
-
-	st->trig->ops = &ad7606_trigger_ops;
-	iio_trigger_set_drvdata(st->trig, indio_dev);
-	ret = devm_iio_trigger_register(dev, st->trig);
-	if (ret)
-		return ret;
-
-	indio_dev->trig = iio_trigger_get(st->trig);
+	/* If convst pin is not defined, setup PWM*/
+	if (!st->gpio_convst) {
+		st->cnvst_pwm = devm_pwm_get(dev, NULL);
+		if (IS_ERR(st->cnvst_pwm))
+			return PTR_ERR(st->cnvst_pwm);
+		ret = devm_add_action_or_reset(dev, ad7606_pwm_disable,
+					       st->cnvst_pwm);
+		if (ret)
+			return ret;
 
+		/*
+		 * Set the sampling rate to 2 kHz so to be sure that the interruption can be
+		 * handled between within a single pulse.
+		 */
+		ret = ad7606_set_sampling_freq(st, 2 * KILO);
+		if (ret)
+			return ret;
+	} else {
+		st->trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
+						  indio_dev->name,
+						  iio_device_id(indio_dev));
+		if (!st->trig)
+			return -ENOMEM;
+		st->trig->ops = &ad7606_trigger_ops;
+		iio_trigger_set_drvdata(st->trig, indio_dev);
+		ret = devm_iio_trigger_register(dev, st->trig);
+		if (ret)
+			return ret;
+		indio_dev->trig = iio_trigger_get(st->trig);
+		ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
+						      &iio_pollfunc_store_time,
+						      &ad7606_trigger_handler,
+						      &ad7606_buffer_ops);
+		if (ret)
+			return ret;
+	}
 	ret = devm_request_threaded_irq(dev, irq,
 					NULL,
 					&ad7606_interrupt,
@@ -657,13 +777,6 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
 	if (ret)
 		return ret;
 
-	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
-					      &iio_pollfunc_store_time,
-					      &ad7606_trigger_handler,
-					      &ad7606_buffer_ops);
-	if (ret)
-		return ret;
-
 	return devm_iio_device_register(dev, indio_dev);
 }
 EXPORT_SYMBOL_NS_GPL(ad7606_probe, IIO_AD7606);
@@ -693,7 +806,6 @@ static int ad7606_resume(struct device *dev)
 		gpiod_set_value(st->gpio_standby, 1);
 		ad7606_reset(st);
 	}
-
 	return 0;
 }
 
diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
index 0c6a88cc4695..60bac1894a91 100644
--- a/drivers/iio/adc/ad7606.h
+++ b/drivers/iio/adc/ad7606.h
@@ -38,6 +38,8 @@
 	AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),\
 		0, BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO))
 
+struct pwm_device		*cnvst_pwm;
+
 /**
  * struct ad7606_chip_info - chip specific information
  * @channels:		channel specification
@@ -94,6 +96,7 @@ struct ad7606_state {
 	const struct ad7606_bus_ops	*bops;
 	unsigned int			range[16];
 	unsigned int			oversampling;
+	struct pwm_device		*cnvst_pwm;
 	void __iomem			*base_address;
 	bool				sw_mode_en;
 	const unsigned int		*scale_avail;

-- 
2.34.1

[PATCH 7/8] iio: adc: ad7606: Switch to xxx_get_device_match_data

[Guillaume Stols]

On the parallel version, the current implementation is only compatible
with id tables and won't work with fx_nodes. So in this commit, the goal
is to switch to use get_device_match_data, in order to simplify the
logic of retrieving chip data.

Also, chip info is moved in the .h file so to be accessible to all the
driver files that can set a pointer to the corresponding chip as the
driver data.

Signed-off-by: Guillaume Stols <gstols@baylibre.com>
---
 drivers/iio/adc/ad7606.c     | 106 ++--------------------------------
 drivers/iio/adc/ad7606.h     | 132 ++++++++++++++++++++++++++++++++++++++++---
 drivers/iio/adc/ad7606_par.c |  22 ++++----
 drivers/iio/adc/ad7606_spi.c |  31 +++++-----
 4 files changed, 154 insertions(+), 137 deletions(-)

diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index 2c9ddcd0ad77..99d5ca5c2348 100644
--- a/drivers/iio/adc/ad7606.c
+++ b/drivers/iio/adc/ad7606.c
@@ -43,14 +43,6 @@ static const unsigned int ad7616_sw_scale_avail[3] = {
 	76293, 152588, 305176
 };
 
-static const unsigned int ad7606_oversampling_avail[7] = {
-	1, 2, 4, 8, 16, 32, 64,
-};
-
-static const unsigned int ad7616_oversampling_avail[8] = {
-	1, 2, 4, 8, 16, 32, 64, 128,
-};
-
 static int ad7606_reset(struct ad7606_state *st)
 {
 	if (st->gpio_reset) {
@@ -415,96 +407,6 @@ static const struct attribute_group ad7606_attribute_group_range = {
 	.attrs = ad7606_attributes_range,
 };
 
-static const struct iio_chan_spec ad7605_channels[] = {
-	IIO_CHAN_SOFT_TIMESTAMP(4),
-	AD7605_CHANNEL(0),
-	AD7605_CHANNEL(1),
-	AD7605_CHANNEL(2),
-	AD7605_CHANNEL(3),
-};
-
-static const struct iio_chan_spec ad7606_channels[] = {
-	IIO_CHAN_SOFT_TIMESTAMP(8),
-	AD7606_CHANNEL(0),
-	AD7606_CHANNEL(1),
-	AD7606_CHANNEL(2),
-	AD7606_CHANNEL(3),
-	AD7606_CHANNEL(4),
-	AD7606_CHANNEL(5),
-	AD7606_CHANNEL(6),
-	AD7606_CHANNEL(7),
-};
-
-/*
- * The current assumption that this driver makes for AD7616, is that it's
- * working in Hardware Mode with Serial, Burst and Sequencer modes activated.
- * To activate them, following pins must be pulled high:
- *	-SER/PAR
- *	-SEQEN
- * And following pins must be pulled low:
- *	-WR/BURST
- *	-DB4/SER1W
- */
-static const struct iio_chan_spec ad7616_channels[] = {
-	IIO_CHAN_SOFT_TIMESTAMP(16),
-	AD7606_CHANNEL(0),
-	AD7606_CHANNEL(1),
-	AD7606_CHANNEL(2),
-	AD7606_CHANNEL(3),
-	AD7606_CHANNEL(4),
-	AD7606_CHANNEL(5),
-	AD7606_CHANNEL(6),
-	AD7606_CHANNEL(7),
-	AD7606_CHANNEL(8),
-	AD7606_CHANNEL(9),
-	AD7606_CHANNEL(10),
-	AD7606_CHANNEL(11),
-	AD7606_CHANNEL(12),
-	AD7606_CHANNEL(13),
-	AD7606_CHANNEL(14),
-	AD7606_CHANNEL(15),
-};
-
-static const struct ad7606_chip_info ad7606_chip_info_tbl[] = {
-	/* More devices added in future */
-	[ID_AD7605_4] = {
-		.channels = ad7605_channels,
-		.num_channels = 5,
-	},
-	[ID_AD7606_8] = {
-		.channels = ad7606_channels,
-		.num_channels = 9,
-		.oversampling_avail = ad7606_oversampling_avail,
-		.oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
-	},
-	[ID_AD7606_6] = {
-		.channels = ad7606_channels,
-		.num_channels = 7,
-		.oversampling_avail = ad7606_oversampling_avail,
-		.oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
-	},
-	[ID_AD7606_4] = {
-		.channels = ad7606_channels,
-		.num_channels = 5,
-		.oversampling_avail = ad7606_oversampling_avail,
-		.oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
-	},
-	[ID_AD7606B] = {
-		.channels = ad7606_channels,
-		.num_channels = 9,
-		.oversampling_avail = ad7606_oversampling_avail,
-		.oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
-	},
-	[ID_AD7616] = {
-		.channels = ad7616_channels,
-		.num_channels = 17,
-		.oversampling_avail = ad7616_oversampling_avail,
-		.oversampling_num = ARRAY_SIZE(ad7616_oversampling_avail),
-		.os_req_reset = true,
-		.init_delay_ms = 15,
-	},
-};
-
 static int ad7606_request_gpios(struct ad7606_state *st)
 {
 	struct device *dev = st->dev;
@@ -644,7 +546,7 @@ static void ad7606_pwm_disable(void *data)
 }
 
 int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
-		 const char *name, unsigned int id,
+		 const struct ad7606_chip_info *chip_info,
 		 const struct ad7606_bus_ops *bops)
 {
 	struct ad7606_state *st;
@@ -673,7 +575,7 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
 		return dev_err_probe(dev, ret,
 				     "Failed to enable specified AVcc supply\n");
 
-	st->chip_info = &ad7606_chip_info_tbl[id];
+	st->chip_info = chip_info;
 
 	if (st->chip_info->oversampling_num) {
 		st->oversampling_avail = st->chip_info->oversampling_avail;
@@ -696,7 +598,7 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
 			indio_dev->info = &ad7606_info_no_os_or_range;
 	}
 	indio_dev->modes = INDIO_DIRECT_MODE;
-	indio_dev->name = name;
+	indio_dev->name = chip_info->name;
 	indio_dev->channels = st->chip_info->channels;
 	indio_dev->num_channels = st->chip_info->num_channels;
 
@@ -773,7 +675,7 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
 					NULL,
 					&ad7606_interrupt,
 					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
-					name, indio_dev);
+					chip_info->name, indio_dev);
 	if (ret)
 		return ret;
 
diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
index 60bac1894a91..aab8fefb84be 100644
--- a/drivers/iio/adc/ad7606.h
+++ b/drivers/iio/adc/ad7606.h
@@ -40,8 +40,19 @@
 
 struct pwm_device		*cnvst_pwm;
 
+enum ad7606_supported_device_ids {
+	ID_AD7605_4,
+	ID_AD7606_8,
+	ID_AD7606_6,
+	ID_AD7606_4,
+	ID_AD7606B,
+	ID_AD7616,
+};
+
 /**
  * struct ad7606_chip_info - chip specific information
+ * @name		device name
+ * @id			device id
  * @channels:		channel specification
  * @num_channels:	number of channels
  * @oversampling_avail	pointer to the array which stores the available
@@ -52,6 +63,8 @@ struct pwm_device		*cnvst_pwm;
  *			after a restart
  */
 struct ad7606_chip_info {
+	enum ad7606_supported_device_ids id;
+	const char			*name;
 	const struct iio_chan_spec	*channels;
 	unsigned int			num_channels;
 	const unsigned int		*oversampling_avail;
@@ -151,16 +164,119 @@ struct ad7606_bus_ops {
 };
 
 int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
-		 const char *name, unsigned int id,
+		 const struct ad7606_chip_info *info,
 		 const struct ad7606_bus_ops *bops);
 
-enum ad7606_supported_device_ids {
-	ID_AD7605_4,
-	ID_AD7606_8,
-	ID_AD7606_6,
-	ID_AD7606_4,
-	ID_AD7606B,
-	ID_AD7616,
+static const unsigned int ad7606_oversampling_avail[7] = {
+	1, 2, 4, 8, 16, 32, 64,
+};
+
+static const unsigned int ad7616_oversampling_avail[8] = {
+	1, 2, 4, 8, 16, 32, 64, 128,
+};
+
+static const struct iio_chan_spec ad7605_channels[] = {
+	IIO_CHAN_SOFT_TIMESTAMP(4),
+	AD7605_CHANNEL(0),
+	AD7605_CHANNEL(1),
+	AD7605_CHANNEL(2),
+	AD7605_CHANNEL(3),
+};
+
+static const struct iio_chan_spec ad7606_channels[] = {
+	IIO_CHAN_SOFT_TIMESTAMP(8),
+	AD7606_CHANNEL(0),
+	AD7606_CHANNEL(1),
+	AD7606_CHANNEL(2),
+	AD7606_CHANNEL(3),
+	AD7606_CHANNEL(4),
+	AD7606_CHANNEL(5),
+	AD7606_CHANNEL(6),
+	AD7606_CHANNEL(7),
+};
+
+/*
+ * The current assumption that this driver makes for AD7616, is that it's
+ * working in Hardware Mode with Serial, Burst and Sequencer modes activated.
+ * To activate them, following pins must be pulled high:
+ *	-SER/PAR
+ *	-SEQEN
+ * And following pins must be pulled low:
+ *	-WR/BURST
+ *	-DB4/SER1W
+ */
+static const struct iio_chan_spec ad7616_channels[] = {
+	IIO_CHAN_SOFT_TIMESTAMP(16),
+	AD7606_CHANNEL(0),
+	AD7606_CHANNEL(1),
+	AD7606_CHANNEL(2),
+	AD7606_CHANNEL(3),
+	AD7606_CHANNEL(4),
+	AD7606_CHANNEL(5),
+	AD7606_CHANNEL(6),
+	AD7606_CHANNEL(7),
+	AD7606_CHANNEL(8),
+	AD7606_CHANNEL(9),
+	AD7606_CHANNEL(10),
+	AD7606_CHANNEL(11),
+	AD7606_CHANNEL(12),
+	AD7606_CHANNEL(13),
+	AD7606_CHANNEL(14),
+	AD7606_CHANNEL(15),
+};
+
+static const struct ad7606_chip_info ad7605_4_info = {
+	.channels = ad7605_channels,
+	.num_channels = 5,
+	.name = "ad7605-4",
+	.id = ID_AD7605_4,
+};
+
+static const struct ad7606_chip_info ad7606_8_info = {
+	.channels = ad7606_channels,
+	.num_channels = 9,
+	.oversampling_avail = ad7606_oversampling_avail,
+	.oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
+	.name = "ad7606-8",
+	.id = ID_AD7606_8,
+};
+
+static const struct ad7606_chip_info ad7606_6_info = {
+	.channels = ad7606_channels,
+	.num_channels = 7,
+	.oversampling_avail = ad7606_oversampling_avail,
+	.oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
+	.name = "ad7606-6",
+	.id = ID_AD7606_6,
+};
+
+static const struct ad7606_chip_info ad7606_4_info = {
+	.channels = ad7606_channels,
+	.num_channels = 5,
+	.oversampling_avail = ad7606_oversampling_avail,
+	.oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
+	.name = "ad7606-4",
+	.id = ID_AD7606_4,
+};
+
+static const struct ad7606_chip_info ad7606b_info = {
+	.channels = ad7606_channels,
+	.num_channels = 9,
+	.oversampling_avail = ad7606_oversampling_avail,
+	.oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
+	.name = "ad7606B",
+	.id = ID_AD7606B,
+};
+
+static const struct ad7606_chip_info ad7616_info = {
+	.channels = ad7616_channels,
+	.num_channels = 17,
+	.oversampling_avail = ad7616_oversampling_avail,
+	.oversampling_num = ARRAY_SIZE(ad7616_oversampling_avail),
+	.os_req_reset = true,
+	.init_delay_ms = 15,
+	.name = "ad7616",
+	.id = ID_AD7616,
 };
 
 #ifdef CONFIG_PM_SLEEP
diff --git a/drivers/iio/adc/ad7606_par.c b/drivers/iio/adc/ad7606_par.c
index d8408052262e..d83c0edc1e31 100644
--- a/drivers/iio/adc/ad7606_par.c
+++ b/drivers/iio/adc/ad7606_par.c
@@ -47,7 +47,7 @@ static const struct ad7606_bus_ops ad7606_par8_bops = {
 
 static int ad7606_par_probe(struct platform_device *pdev)
 {
-	const struct platform_device_id *id = platform_get_device_id(pdev);
+	const struct ad7606_chip_info *chip_info = platform_get_device_match_data(pdev);
 	struct resource *res;
 	void __iomem *addr;
 	resource_size_t remap_size;
@@ -64,26 +64,26 @@ static int ad7606_par_probe(struct platform_device *pdev)
 	remap_size = resource_size(res);
 
 	return ad7606_probe(&pdev->dev, irq, addr,
-			    id->name, id->driver_data,
+			    chip_info,
 			    remap_size > 1 ? &ad7606_par16_bops :
 			    &ad7606_par8_bops);
 }
 
 static const struct platform_device_id ad7606_driver_ids[] = {
-	{ .name	= "ad7605-4", .driver_data = ID_AD7605_4, },
-	{ .name	= "ad7606-4", .driver_data = ID_AD7606_4, },
-	{ .name	= "ad7606-6", .driver_data = ID_AD7606_6, },
-	{ .name	= "ad7606-8", .driver_data = ID_AD7606_8, },
+	{ .name	= "ad7605-4", .driver_data = (kernel_ulong_t)&ad7605_4_info, },
+	{ .name	= "ad7606-4", .driver_data = (kernel_ulong_t)&ad7606_4_info, },
+	{ .name	= "ad7606-6", .driver_data = (kernel_ulong_t)&ad7606_6_info, },
+	{ .name	= "ad7606-8", .driver_data = (kernel_ulong_t)&ad7606_8_info, },
 	{ }
 };
 MODULE_DEVICE_TABLE(platform, ad7606_driver_ids);
 
 static const struct of_device_id ad7606_of_match[] = {
-	{ .compatible = "adi,ad7605-4" },
-	{ .compatible = "adi,ad7606-4" },
-	{ .compatible = "adi,ad7606-6" },
-	{ .compatible = "adi,ad7606-8" },
-	{ },
+	{ .compatible = "adi,ad7605-4", .data = &ad7605_4_info },
+	{ .compatible = "adi,ad7606-4", .data = &ad7606_4_info },
+	{ .compatible = "adi,ad7606-6", .data = &ad7606_6_info },
+	{ .compatible = "adi,ad7606-8", .data = &ad7606_8_info },
+	{ }
 };
 MODULE_DEVICE_TABLE(of, ad7606_of_match);
 
diff --git a/drivers/iio/adc/ad7606_spi.c b/drivers/iio/adc/ad7606_spi.c
index 287a0591533b..c23a7448f851 100644
--- a/drivers/iio/adc/ad7606_spi.c
+++ b/drivers/iio/adc/ad7606_spi.c
@@ -307,10 +307,10 @@ static const struct ad7606_bus_ops ad7606B_spi_bops = {
 
 static int ad7606_spi_probe(struct spi_device *spi)
 {
-	const struct spi_device_id *id = spi_get_device_id(spi);
+	const struct ad7606_chip_info *chip_info = spi_get_device_match_data(spi);
 	const struct ad7606_bus_ops *bops;
 
-	switch (id->driver_data) {
+	switch (chip_info->id) {
 	case ID_AD7616:
 		bops = &ad7616_spi_bops;
 		break;
@@ -323,28 +323,27 @@ static int ad7606_spi_probe(struct spi_device *spi)
 	}
 
 	return ad7606_probe(&spi->dev, spi->irq, NULL,
-			    id->name, id->driver_data,
-			    bops);
+			    chip_info, bops);
 }
 
 static const struct spi_device_id ad7606_id_table[] = {
-	{ "ad7605-4", ID_AD7605_4 },
-	{ "ad7606-4", ID_AD7606_4 },
-	{ "ad7606-6", ID_AD7606_6 },
-	{ "ad7606-8", ID_AD7606_8 },
-	{ "ad7606b",  ID_AD7606B },
-	{ "ad7616",   ID_AD7616 },
+	{ "ad7605-4", (kernel_ulong_t)&ad7605_4_info },
+	{ "ad7606-4", (kernel_ulong_t)&ad7606_4_info },
+	{ "ad7606-6", (kernel_ulong_t)&ad7606_6_info },
+	{ "ad7606-8", (kernel_ulong_t)&ad7606_8_info },
+	{ "ad7606b",  (kernel_ulong_t)&ad7606b_info },
+	{ "ad7616",   (kernel_ulong_t)&ad7616_info },
 	{}
 };
 MODULE_DEVICE_TABLE(spi, ad7606_id_table);
 
 static const struct of_device_id ad7606_of_match[] = {
-	{ .compatible = "adi,ad7605-4" },
-	{ .compatible = "adi,ad7606-4" },
-	{ .compatible = "adi,ad7606-6" },
-	{ .compatible = "adi,ad7606-8" },
-	{ .compatible = "adi,ad7606b" },
-	{ .compatible = "adi,ad7616" },
+	{ .compatible = "adi,ad7605-4", &ad7605_4_info },
+	{ .compatible = "adi,ad7606-4", &ad7606_4_info },
+	{ .compatible = "adi,ad7606-6", &ad7606_6_info },
+	{ .compatible = "adi,ad7606-8", &ad7606_8_info },
+	{ .compatible = "adi,ad7606b", &ad7606b_info },
+	{ .compatible = "adi,ad7616", &ad7616_info },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, ad7606_of_match);

-- 
2.34.1

[PATCH 8/8] iio:adc:ad7606: Add iio-backend support

[Guillaume Stols]

- Basic support for iio backend.
- Supports IIO_CHAN_INFO_SAMP_FREQ R/W.
- Only hardware mode is available, and that IIO_CHAN_INFO_RAW is not
  supported if iio-backend mode is selected.

A small correction was added to the driver's file name in the Kconfig
file's description.

Signed-off-by: Guillaume Stols <gstols@baylibre.com>
---
 drivers/iio/adc/Kconfig      |   3 +-
 drivers/iio/adc/ad7606.c     | 103 +++++++++++++++++++++++++++++++++++--------
 drivers/iio/adc/ad7606.h     |  16 +++++++
 drivers/iio/adc/ad7606_par.c |  98 +++++++++++++++++++++++++++++++++++++++-
 4 files changed, 200 insertions(+), 20 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 88e8ce2e78b3..01248b6df868 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -227,9 +227,10 @@ config AD7606_IFACE_PARALLEL
 	help
 	  Say yes here to build parallel interface support for Analog Devices:
 	  ad7605-4, ad7606, ad7606-6, ad7606-4 analog to digital converters (ADC).
+	  It also support iio_backended devices for AD7606B.
 
 	  To compile this driver as a module, choose M here: the
-	  module will be called ad7606_parallel.
+	  module will be called ad7606_par.
 
 config AD7606_IFACE_SPI
 	tristate "Analog Devices AD7606 ADC driver with spi interface support"
diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index 99d5ca5c2348..a753d5caa9f8 100644
--- a/drivers/iio/adc/ad7606.c
+++ b/drivers/iio/adc/ad7606.c
@@ -21,6 +21,7 @@
 #include <linux/util_macros.h>
 #include <linux/units.h>
 
+#include <linux/iio/backend.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/buffer.h>
 #include <linux/iio/sysfs.h>
@@ -148,7 +149,15 @@ static int ad7606_set_sampling_freq(struct ad7606_state *st, unsigned long freq)
 
 static int ad7606_read_samples(struct ad7606_state *st)
 {
-	unsigned int num = st->chip_info->num_channels - 1;
+	unsigned int num = st->chip_info->num_channels;
+
+	/*
+	 * Timestamp channel does not contain sample, and no timestamp channel if
+	 * backend is used.
+	 */
+	if (!st->back)
+		num--;
+
 	u16 *data = st->data;
 	int ret;
 
@@ -220,11 +229,15 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
 		if (!ret)
 			return ret;
 	}
-	ret = wait_for_completion_timeout(&st->completion,
-					  msecs_to_jiffies(1000));
-	if (!ret) {
-		ret = -ETIMEDOUT;
-		goto error_ret;
+
+	/* backend manages interruptions by itself.*/
+	if (!st->back) {
+		ret = wait_for_completion_timeout(&st->completion,
+						  msecs_to_jiffies(1000));
+		if (!ret) {
+			ret = -ETIMEDOUT;
+			goto error_ret;
+		}
 	}
 
 	ret = ad7606_read_samples(st);
@@ -271,6 +284,12 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
 		*val = st->oversampling;
 		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		pwm_get_state_hw(st->cnvst_pwm, &cnvst_pwm_state);
+		/* If the PWM is swinging, return the real frequency, otherwise 0 */
+		*val = ad7606_pwm_is_swinging(st) ?
+			DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, cnvst_pwm_state.period) : 0;
+		return IIO_VAL_INT;
 	}
 	return -EINVAL;
 }
@@ -360,6 +379,8 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
 			return ret;
 
 		return 0;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		return ad7606_set_sampling_freq(st, val);
 	default:
 		return -EINVAL;
 	}
@@ -482,7 +503,6 @@ static int ad7606_buffer_postenable(struct iio_dev *indio_dev)
 	struct ad7606_state *st = iio_priv(indio_dev);
 
 	gpiod_set_value(st->gpio_convst, 1);
-	ad7606_pwm_set_swing(st);
 
 	return 0;
 }
@@ -492,19 +512,53 @@ static int ad7606_buffer_predisable(struct iio_dev *indio_dev)
 	struct ad7606_state *st = iio_priv(indio_dev);
 
 	gpiod_set_value(st->gpio_convst, 0);
-	ad7606_pwm_set_low(st);
 
 	return 0;
 }
 
+static int ad7606_pwm_buffer_postenable(struct iio_dev *indio_dev)
+{
+	struct ad7606_state *st = iio_priv(indio_dev);
+
+	return ad7606_pwm_set_swing(st);
+}
+
+static int ad7606_pwm_buffer_predisable(struct iio_dev *indio_dev)
+{
+	struct ad7606_state *st = iio_priv(indio_dev);
+
+	return ad7606_pwm_set_low(st);
+}
+
+static int ad7606_update_scan_mode(struct iio_dev *indio_dev,
+				   const unsigned long *scan_mask)
+{
+	struct ad7606_state *st = iio_priv(indio_dev);
+
+	/* The update scan mode is only for iio backend compatible drivers.
+	 * If the specific update_scan_mode is not defined in the bus ops,
+	 * just do nothing and return 0.
+	 */
+	if (st->bops->update_scan_mode)
+		return st->bops->update_scan_mode(indio_dev, scan_mask);
+	else
+		return 0;
+}
+
 static const struct iio_buffer_setup_ops ad7606_buffer_ops = {
 	.postenable = &ad7606_buffer_postenable,
 	.predisable = &ad7606_buffer_predisable,
 };
 
+static const struct iio_buffer_setup_ops ad7606_pwm_buffer_ops = {
+	.postenable = &ad7606_pwm_buffer_postenable,
+	.predisable = &ad7606_pwm_buffer_predisable,
+};
+
 static const struct iio_info ad7606_info_no_os_or_range = {
 	.read_raw = &ad7606_read_raw,
 	.validate_trigger = &ad7606_validate_trigger,
+	.update_scan_mode = &ad7606_update_scan_mode,
 };
 
 static const struct iio_info ad7606_info_os_and_range = {
@@ -512,6 +566,7 @@ static const struct iio_info ad7606_info_os_and_range = {
 	.write_raw = &ad7606_write_raw,
 	.attrs = &ad7606_attribute_group_os_and_range,
 	.validate_trigger = &ad7606_validate_trigger,
+	.update_scan_mode = &ad7606_update_scan_mode,
 };
 
 static const struct iio_info ad7606_info_os_range_and_debug = {
@@ -520,6 +575,7 @@ static const struct iio_info ad7606_info_os_range_and_debug = {
 	.debugfs_reg_access = &ad7606_reg_access,
 	.attrs = &ad7606_attribute_group_os_and_range,
 	.validate_trigger = &ad7606_validate_trigger,
+	.update_scan_mode = &ad7606_update_scan_mode,
 };
 
 static const struct iio_info ad7606_info_os = {
@@ -527,6 +583,7 @@ static const struct iio_info ad7606_info_os = {
 	.write_raw = &ad7606_write_raw,
 	.attrs = &ad7606_attribute_group_os,
 	.validate_trigger = &ad7606_validate_trigger,
+	.update_scan_mode = &ad7606_update_scan_mode,
 };
 
 static const struct iio_info ad7606_info_range = {
@@ -534,6 +591,7 @@ static const struct iio_info ad7606_info_range = {
 	.write_raw = &ad7606_write_raw,
 	.attrs = &ad7606_attribute_group_range,
 	.validate_trigger = &ad7606_validate_trigger,
+	.update_scan_mode = &ad7606_update_scan_mode,
 };
 
 static const struct iio_trigger_ops ad7606_trigger_ops = {
@@ -602,8 +660,6 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
 	indio_dev->channels = st->chip_info->channels;
 	indio_dev->num_channels = st->chip_info->num_channels;
 
-	init_completion(&st->completion);
-
 	ret = ad7606_reset(st);
 	if (ret)
 		dev_warn(st->dev, "failed to RESET: no RESET GPIO specified\n");
@@ -635,7 +691,7 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
 			return ret;
 	}
 
-	/* If convst pin is not defined, setup PWM*/
+	/* If convst pin is not defined, setup PWM */
 	if (!st->gpio_convst) {
 		st->cnvst_pwm = devm_pwm_get(dev, NULL);
 		if (IS_ERR(st->cnvst_pwm))
@@ -671,14 +727,25 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
 		if (ret)
 			return ret;
 	}
-	ret = devm_request_threaded_irq(dev, irq,
-					NULL,
-					&ad7606_interrupt,
-					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
-					chip_info->name, indio_dev);
-	if (ret)
-		return ret;
 
+	if (st->bops->iio_backend_config) {
+		st->bops->iio_backend_config(dev, indio_dev);
+		indio_dev->setup_ops = &ad7606_pwm_buffer_ops;
+	} else {
+		/* Reserve the PWM use only for backend (force gpio_convst definition)*/
+		if (!st->gpio_convst)
+			return dev_err_probe(dev, -EINVAL,
+					     "Convst pin must be defined when not in backend mode");
+
+		init_completion(&st->completion);
+		ret = devm_request_threaded_irq(dev, irq,
+						NULL,
+						&ad7606_interrupt,
+						IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+						chip_info->name, indio_dev);
+		if (ret)
+			return ret;
+	}
 	return devm_iio_device_register(dev, indio_dev);
 }
 EXPORT_SYMBOL_NS_GPL(ad7606_probe, IIO_AD7606);
diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
index aab8fefb84be..9a098cd77812 100644
--- a/drivers/iio/adc/ad7606.h
+++ b/drivers/iio/adc/ad7606.h
@@ -34,6 +34,12 @@
 		BIT(IIO_CHAN_INFO_SCALE),		\
 		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO))
 
+#define AD7606_BI_CHANNEL(num)				\
+	AD760X_CHANNEL(num, 0,				\
+		BIT(IIO_CHAN_INFO_SCALE),		\
+		BIT(IIO_CHAN_INFO_SAMP_FREQ) |		\
+		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO))
+
 #define AD7616_CHANNEL(num)	\
 	AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),\
 		0, BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO))
@@ -61,6 +67,7 @@ enum ad7606_supported_device_ids {
  * @os_req_reset	some devices require a reset to update oversampling
  * @init_delay_ms	required delay in miliseconds for initialization
  *			after a restart
+ * @has_backend		defines if a backend is available for the given chip
  */
 struct ad7606_chip_info {
 	enum ad7606_supported_device_ids id;
@@ -71,6 +78,7 @@ struct ad7606_chip_info {
 	unsigned int			oversampling_num;
 	bool				os_req_reset;
 	unsigned long			init_delay_ms;
+	bool				has_backend;
 };
 
 /**
@@ -116,6 +124,7 @@ struct ad7606_state {
 	unsigned int			num_scales;
 	const unsigned int		*oversampling_avail;
 	unsigned int			num_os_ratios;
+	struct iio_backend		*back;
 	int (*write_scale)(struct iio_dev *indio_dev, int ch, int val);
 	int (*write_os)(struct iio_dev *indio_dev, int val);
 
@@ -140,16 +149,21 @@ struct ad7606_state {
 
 /**
  * struct ad7606_bus_ops - driver bus operations
+ * @iio_backend_config	function pointer for configuring the iio_backend for
+ *			the compatibles that use it
  * @read_block		function pointer for reading blocks of data
  * @sw_mode_config:	pointer to a function which configured the device
  *			for software mode
  * @reg_read	function pointer for reading spi register
  * @reg_write	function pointer for writing spi register
  * @write_mask	function pointer for write spi register with mask
+ * @update_scan_mode	function pointer for handling the calls to iio_info's update_scan
+ *			mode when enabling/disabling channels.
  * @rd_wr_cmd	pointer to the function which calculates the spi address
  */
 struct ad7606_bus_ops {
 	/* more methods added in future? */
+	int (*iio_backend_config)(struct device *dev, struct iio_dev *indio_dev);
 	int (*read_block)(struct device *dev, int num, void *data);
 	int (*sw_mode_config)(struct iio_dev *indio_dev);
 	int (*reg_read)(struct ad7606_state *st, unsigned int addr);
@@ -160,6 +174,7 @@ struct ad7606_bus_ops {
 				 unsigned int addr,
 				 unsigned long mask,
 				 unsigned int val);
+	int (*update_scan_mode)(struct iio_dev *indio_dev, const unsigned long *scan_mask);
 	u16 (*rd_wr_cmd)(int addr, char isWriteOp);
 };
 
@@ -264,6 +279,7 @@ static const struct ad7606_chip_info ad7606b_info = {
 	.num_channels = 9,
 	.oversampling_avail = ad7606_oversampling_avail,
 	.oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail),
+	.has_backend = true,
 	.name = "ad7606B",
 	.id = ID_AD7606B,
 };
diff --git a/drivers/iio/adc/ad7606_par.c b/drivers/iio/adc/ad7606_par.c
index d83c0edc1e31..5c8a04556e25 100644
--- a/drivers/iio/adc/ad7606_par.c
+++ b/drivers/iio/adc/ad7606_par.c
@@ -3,6 +3,8 @@
  * AD7606 Parallel Interface ADC driver
  *
  * Copyright 2011 Analog Devices Inc.
+ * Copyright 2024 Analog Devices Inc.
+ * Copyright 2024 BayLibre SAS.
  */
 
 #include <linux/mod_devicetable.h>
@@ -11,10 +13,86 @@
 #include <linux/types.h>
 #include <linux/err.h>
 #include <linux/io.h>
+#include <linux/pwm.h>
+#include <linux/gpio.h>
+#include <linux/delay.h>
 
 #include <linux/iio/iio.h>
+#include <linux/iio/backend.h>
 #include "ad7606.h"
 
+#ifdef CONFIG_IIO_BACKEND
+static const struct iio_chan_spec ad7606b_bi_channels[] = {
+	AD7606_BI_CHANNEL(0),
+	AD7606_BI_CHANNEL(1),
+	AD7606_BI_CHANNEL(2),
+	AD7606_BI_CHANNEL(3),
+	AD7606_BI_CHANNEL(4),
+	AD7606_BI_CHANNEL(5),
+	AD7606_BI_CHANNEL(6),
+	AD7606_BI_CHANNEL(7),
+};
+
+static int ad7606_bi_update_scan_mode(struct iio_dev *indio_dev, const unsigned long *scan_mask)
+{
+	struct ad7606_state *st = iio_priv(indio_dev);
+	unsigned int c, ret;
+
+	for (c = 0; c < indio_dev->num_channels; c++) {
+		if (test_bit(c, scan_mask))
+			ret = iio_backend_chan_enable(st->back, c);
+		else
+			ret = iio_backend_chan_disable(st->back, c);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int ad7606_bi_setup_iio_backend(struct device *dev, struct iio_dev *indio_dev)
+{
+		struct ad7606_state *st = iio_priv(indio_dev);
+		unsigned int ret, c;
+
+		st->back = devm_iio_backend_get(dev, NULL);
+		if (IS_ERR(st->back))
+			return PTR_ERR(st->back);
+
+		/* If the device is iio_backend powered the PWM is mandatory */
+		if (!st->cnvst_pwm)
+			return -EINVAL;
+
+		ret = devm_iio_backend_request_buffer(dev, st->back, indio_dev);
+		if (ret)
+			return ret;
+
+		ret = devm_iio_backend_enable(dev, st->back);
+		if (ret)
+			return ret;
+
+		struct iio_backend_data_fmt data = {
+			.sign_extend = true,
+			.enable = true,
+		};
+		for (c = 0; c < indio_dev->num_channels; c++) {
+			ret = iio_backend_data_format_set(st->back, c, &data);
+			if (ret)
+				return ret;
+		}
+
+		indio_dev->channels = ad7606b_bi_channels;
+		indio_dev->num_channels = 8;
+
+		return 0;
+}
+
+static const struct ad7606_bus_ops ad7606_bi_bops = {
+	.iio_backend_config = ad7606_bi_setup_iio_backend,
+	.update_scan_mode = ad7606_bi_update_scan_mode,
+};
+#endif
+
 static int ad7606_par16_read_block(struct device *dev,
 				   int count, void *buf)
 {
@@ -52,7 +130,20 @@ static int ad7606_par_probe(struct platform_device *pdev)
 	void __iomem *addr;
 	resource_size_t remap_size;
 	int irq;
-
+#ifdef CONFIG_IIO_BACKEND
+	struct iio_backend *back;
+
+	/*For now, only the AD7606B is backend compatible.*/
+	if (chip_info->has_backend) {
+		back = devm_iio_backend_get(&pdev->dev, NULL);
+		if (IS_ERR(back))
+			return PTR_ERR(back);
+
+		return ad7606_probe(&pdev->dev, 0, NULL,
+				    chip_info,
+				    &ad7606_bi_bops);
+	}
+#endif
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0)
 		return irq;
@@ -74,6 +165,7 @@ static const struct platform_device_id ad7606_driver_ids[] = {
 	{ .name	= "ad7606-4", .driver_data = (kernel_ulong_t)&ad7606_4_info, },
 	{ .name	= "ad7606-6", .driver_data = (kernel_ulong_t)&ad7606_6_info, },
 	{ .name	= "ad7606-8", .driver_data = (kernel_ulong_t)&ad7606_8_info, },
+	{ .name	= "ad7606b", .driver_data = (kernel_ulong_t)&ad7606b_info, },
 	{ }
 };
 MODULE_DEVICE_TABLE(platform, ad7606_driver_ids);
@@ -83,6 +175,7 @@ static const struct of_device_id ad7606_of_match[] = {
 	{ .compatible = "adi,ad7606-4", .data = &ad7606_4_info },
 	{ .compatible = "adi,ad7606-6", .data = &ad7606_6_info },
 	{ .compatible = "adi,ad7606-8", .data = &ad7606_8_info },
+	{ .compatible = "adi,ad7606b", .data = &ad7606b_info },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, ad7606_of_match);
@@ -102,3 +195,6 @@ MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
 MODULE_DESCRIPTION("Analog Devices AD7606 ADC");
 MODULE_LICENSE("GPL v2");
 MODULE_IMPORT_NS(IIO_AD7606);
+#ifdef CONFIG_IIO_BACKEND
+MODULE_IMPORT_NS(IIO_BACKEND);
+#endif

-- 
2.34.1

Re: [PATCH 1/8] dt-bindings: iio: adc: ad7606: Make corrections on spi conditions

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1761 bytes --]

On Thu, Aug 15, 2024 at 12:11:55PM +0000, Guillaume Stols wrote:
> The SPI conditions are not always required, because there is also a
> parallel interface. The way used to detect that the SPI interface is
> used is to check if the reg value is between 0 and 256.
> There is also a correction on the spi-cpha that is not required when SPI
> interface is selected, while spi-cpol is.

This feels like it should be two patches, with the first having a Fixes:
tag etc, if the original binding was incorrect.

> 
> Signed-off-by: Guillaume Stols <gstols@baylibre.com>
> ---
>  .../devicetree/bindings/iio/adc/adi,ad7606.yaml         | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> index 69408cae3db9..c0008d36320f 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> @@ -117,15 +117,26 @@ properties:
>  required:
>    - compatible
>    - reg
> -  - spi-cpha
>    - avcc-supply
>    - vdrive-supply
>    - interrupts
>    - adi,conversion-start-gpios
>  
> -allOf:
> -  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +# This checks if reg is a chipselect so the device is on an SPI
> +# bus, the if-clause will fail if reg is a tuple such as for a
> +# platform device.
> +if:
> +  properties:
> +    reg:
> +      minimum: 0
> +      maximum: 256
> +then:
> +  allOf:
> +    - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +    - required:
> +        - spi-cpol
>  
> +allOf:
>    - if:
>        properties:
>          compatible:
> 
> -- 
> 2.34.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/8] dt-bindings: iio: adc: ad7606: Add iio backend bindings

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 4269 bytes --]

On Thu, Aug 15, 2024 at 12:11:56PM +0000, Guillaume Stols wrote:
> Add the required properties for iio-backend support, as well as an
> example and the conditions to mutually exclude interruption and
> conversion trigger with iio-backend.
> The iio-backend's function is to controls the communication, and thus the
> interruption pin won't be available anymore.
> As a consequence, the conversion pin must be controlled externally since
> we will miss information about when every single conversion cycle (i.e
> conversion + data transfert) ends, hence a PWM is introduced to trigger
> the conversions.
> 
> Signed-off-by: Guillaume Stols <gstols@baylibre.com>
> ---
>  .../devicetree/bindings/iio/adc/adi,ad7606.yaml    | 75 +++++++++++++++++++++-
>  1 file changed, 72 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> index c0008d36320f..4b324f7e3207 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> @@ -114,13 +114,28 @@ properties:
>        assumed that the pins are hardwired to VDD.
>      type: boolean
>  
> +  pwms:
> +    description:
> +      In case the conversion is triggered by a PWM instead of a GPIO plugged to
> +      the CONVST pin, the PWM must be referenced.
> +    minItems: 1
> +    maxItems: 2
> +
> +  pwm-names:
> +    minItems: 1
> +    maxItems: 2

You need to describe what the pwms are.

> +  io-backends:
> +    description:
> +      A reference to the iio-backend, which is responsible handling the BUSY
> +      pin's falling edge and communication.
> +      An example of backend can be found at
> +      http://analogdevicesinc.github.io/hdl/library/axi_ad7606x/index.html
> +
>  required:
>    - compatible
> -  - reg
>    - avcc-supply
>    - vdrive-supply
> -  - interrupts
> -  - adi,conversion-start-gpios
>  
>  # This checks if reg is a chipselect so the device is on an SPI
>  # bus, the if-clause will fail if reg is a tuple such as for a
> @@ -137,6 +152,35 @@ then:
>          - spi-cpol
>  
>  allOf:
> +  # Communication is handled either by the backend or an interrupt.

This comment seems misplaced, but also superfluous?

> +  - if:
> +      properties:
> +        pwms: false
> +    then:
> +      required:
> +        - adi,conversion-start-gpios
> +
> +  - if:
> +      properties:
> +        adi,conversion-start-gpios: false
> +    then:
> +      required:
> +        - pwms
> +
> +  - if:
> +      properties:
> +        interrupts: false
> +    then:
> +      required:
> +        - io-backends
> +
> +  - if:
> +      properties:
> +        io-backends: false
> +    then:
> +      required:
> +        - interrupts
> +
>    - if:
>        properties:
>          compatible:
> @@ -178,12 +222,37 @@ allOf:
>          adi,sw-mode: false
>      else:
>        properties:
> +        pwms:
> +          maxItems: 1
> +        pwm-names:
> +          maxItems: 1
>          adi,conversion-start-gpios:
>            maxItems: 1
>  
>  unevaluatedProperties: false
>  
>  examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    / {
> +        adi_adc {
> +                compatible = "adi,ad7606b";

Just two space indent for examples please.

Cheers,
Conor.

> +
> +                pwms = <&axi_pwm_gen 0 0>;
> +
> +                avcc-supply = <&adc_vref>;
> +                vdrive-supply = <&vdd_supply>;
> +
> +                reset-gpios = <&gpio0 91 GPIO_ACTIVE_HIGH>;
> +                standby-gpios = <&gpio0 90 GPIO_ACTIVE_LOW>;
> +                adi,range-gpios = <&gpio0 89 GPIO_ACTIVE_HIGH>;
> +                adi,oversampling-ratio-gpios = <&gpio0 88 GPIO_ACTIVE_HIGH
> +                                                &gpio0 87 GPIO_ACTIVE_HIGH
> +                                                &gpio0 86 GPIO_ACTIVE_HIGH>;
> +                io-backends = <&iio_backend>;
> +        };
> +    };
> +
>    - |
>      #include <dt-bindings/gpio/gpio.h>
>      #include <dt-bindings/interrupt-controller/irq.h>
> 
> -- 
> 2.34.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 0/8] Add iio backend compatibility for ad7606

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 381 bytes --]

On Thu, Aug 15, 2024 at 12:11:54PM +0000, Guillaume Stols wrote:
> This series aims to add iio backend support for AD7606X ADCs.

Just to point out, your cc list is partially bogus as it contains:

	 20240705211452.1157967-2-u.kleine-koenig@baylibre.com,
	 20240712171821.1470833-2-u.kleine-koenig@baylibre.com,
	 cover.1721040875.git.u.kleine-koenig@baylibre.com,

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/8] dt-bindings: iio: adc: ad7606: Add iio backend bindings

[Jonathan Cameron]

On Thu, 15 Aug 2024 12:11:56 +0000
Guillaume Stols <gstols@baylibre.com> wrote:

> Add the required properties for iio-backend support, as well as an
> example and the conditions to mutually exclude interruption and
> conversion trigger with iio-backend.
> The iio-backend's function is to controls the communication, and thus the
> interruption pin won't be available anymore.
> As a consequence, the conversion pin must be controlled externally since
> we will miss information about when every single conversion cycle (i.e
> conversion + data transfert) ends, hence a PWM is introduced to trigger

transfer

> the conversions.
> 
> Signed-off-by: Guillaume Stols <gstols@baylibre.com>
> ---
>  .../devicetree/bindings/iio/adc/adi,ad7606.yaml    | 75 +++++++++++++++++++++-
>  1 file changed, 72 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> index c0008d36320f..4b324f7e3207 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> @@ -114,13 +114,28 @@ properties:
>        assumed that the pins are hardwired to VDD.
>      type: boolean
>  
> +  pwms:
> +    description:
> +      In case the conversion is triggered by a PWM instead of a GPIO plugged to
> +      the CONVST pin, the PWM must be referenced.
> +    minItems: 1
> +    maxItems: 2
> +
> +  pwm-names:
> +    minItems: 1
> +    maxItems: 2
> +
> +  io-backends:
> +    description:
> +      A reference to the iio-backend, which is responsible handling the BUSY
> +      pin's falling edge and communication.
> +      An example of backend can be found at
> +      http://analogdevicesinc.github.io/hdl/library/axi_ad7606x/index.html
> +
>  required:
>    - compatible
> -  - reg

I think we still want a reg, but only to differentiate multiple instances
perhaps.

>    - avcc-supply
>    - vdrive-supply

Re: [PATCH 3/8] Documentation: iio: Document ad7606 driver

[Jonathan Cameron]

On Thu, 15 Aug 2024 12:11:57 +0000
Guillaume Stols <gstols@baylibre.com> wrote:

> The Analog Devices Inc. AD7606 (and similar chips) are complex ADCs that
> will benefit from a detailed driver documentation.
> 
> This documents the current features supported by the driver.
> 
> Signed-off-by: Guillaume Stols <gstols@baylibre.com>
A few typos that I spotted, but other than that lgtm
> ---
>  Documentation/iio/ad7606.rst | 142 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 142 insertions(+)
> 
> diff --git a/Documentation/iio/ad7606.rst b/Documentation/iio/ad7606.rst
> new file mode 100644
> index 000000000000..e9578399e80d
> --- /dev/null
> +++ b/Documentation/iio/ad7606.rst
> @@ -0,0 +1,142 @@
> +.. SPDX-License-Identifier: GPL-2.0-only
> +
> +=============
> +AD7606 driver
> +=============
> +
> +ADC driver for Analog Devices Inc. AD7606 and similar devices. The module name
> +is ``ad7606``.
> +
> +Supported devices
> +=================
> +
> +The following chips are supported by this driver:
> +
> +* `AD7605 <https://www.analog.com/en/products/ad7605.html>`_
> +* `AD7606 <https://www.analog.com/en/products/ad7606.html>`_
> +* `AD7606B <https://www.analog.com/en/products/ad7606b.html>`_
> +* `AD7616 <https://www.analog.com/en/products/ad7616.html>`_
> +
> +Supported features
> +==================
> +
> +SPI wiring modes
> +----------------
> +
> +ad7606x ADCs can output data on several SDO lines (1/2/4/8). The driver
> +currently supports only 1 SDO line.
> +
> +Parallel wiring mode
> +--------------------
> +
> +AD7606x ADC have also a parallel interface, with 16 lines (that can be reduced
> +to 8 in byte mode). The parallel interface is selected by declaring the device
> +as platform in the device tree (with no io-backends node defined, see below).
> +
> +IIO-backend mode
> +----------------
> +
> +This mode allows to reach the best sample rates, but it requires an external
> +hardware (eg HDL or APU) to handle the low level communication.
> +The backend mode is enabled when trough the definition of the "io-backends"

through.  Spell check in general as I'm bad at spotting these.

> +property in the device tree.
> +The reference configuration for the current implementation of IIO-backend mode
> +is the HDL reference provided by ADI:
> +https://wiki.analog.com/resources/eval/user-guides/ad7606x-fmc/hdl
> +This implementation embeds an IIO-backend compatible IP (adi-axi-adc) and a PWM
> +connected to the conversion trigger pin.
> +
> ++---+                                       +----------------------------
> +|   |               +-------+               |AD76xx
> +| A |  controls     |       |               |
> +| D |-------------->|  PWM  |-------------->| cnvst
> +| 7 |               |       |               |
> +| 6 |               +-------+               |
> +| 0 | controls  +-----------+------------   |
> +| 6 |---------->|           |           |<--| frstdata
> +|   |           | Backend   |  Backend  |<--| busy
> +| D |           | Driver    |           |   |
> +| R |           |           |           |-->| clk
> +| I |  requests |+---------+| DMA       |   |
> +| V |----------->|  Buffer ||<----      |<=>| DATA
> +| E |           |+---------+|           |   |
> +| R |           +-----------+-----------+   |
> +|   |-------------------------------------->| reset/configuration gpios
> ++---+                                       +-----------------------------
> +
> +
> +Software and hardware modes
> +---------------------------
> +
> +While all the AD7606 series parts can be configured using GPIOs, some of them
> +can be configured using register.

I'd add blank lines between paragraphs as makes it easier to read as text

> +The chip that support software mode have more values avalaible for configuring

available

> +the device, as well as more settings, and allow to control the range and
> +calibration per channel.
> +The following settings are available per channel in software mode:
> + - Scale
> +Also, there is a broader choice of oversampling ratios in software mode.
> +
> +Conversion triggering
> +---------------------
> +
> +The conversion can be triggered by two distinct ways:
> +
> + - A GPIO is connected to the conversion trigger pin, and this GPIO is controlled
> +   by the driver directly.  In this configuration, the driver set back the
the driver sets back

> +   conversion trigger pin to high as soon as it has read all the conversions.
> +
> + - An external source is connected to the conversion trigger pin. In the
> +   current implementation, it must be a PWM. In this configuration, the driver
> +   does not control directly the conversion trigger pin. Instead, it can
> +   control the PWM's frequency. This trigger is enabled only for iio-backend.
> +
> +Reference voltage
> +-----------------
> +
> +2 possible reference voltage sources are supported:
> +
> + - Internal reference (2.5V)
> + - External reference (2.5V)
> +
> +The source is determined by the device tree. If ``refin-supply`` is present,
> +then the external reference is used, else the internal reference is used.
> +
> +Oversampling
> +------------
> +
> +This family supports oversampling to improve SNR.
> +In software mode, the following ratios are available:
> +1 (oversampling disabled)/2/4/8/16/32/64/128/256.
> +
> +Unimplemented features
> +----------------------
> +
> +- 2/4/8 SDO lines
> +- CRC indication
> +- Calibration
> +
> +Device buffers
> +==============
> +
> +IIO triggered buffer
> +--------------------
> +
> +This driver supports IIO triggered buffers, with a "built in" trigger, i.e the
> +trigger is allocated and linked by the driver, and a new conversion is triggered
> +as soon as the samples are transferred, and a timestamp channel is added to make
> +up for the potential jitter induced by the delays in the interrupt handling.
> +
> +IIO backend buffer
> +------------------
> +
> +When IIO backend is used, the trigger is not needed, and the sample rate is
> +considered as stable, hence there is no timestamp channel. The communication is
> +delegated to an external logic, called a backend, and the backend's driver
> +handles the buffer. When this mode is enabled, the driver cannot control the
> +conversion pin, because the busy pin is bound to the backend.
> +
> +
> +
> +
> +
Drop this trailing whitespace.

> 

Re: [PATCH 5/8] platform: add platform_get_device_match_data() helper

[Jonathan Cameron]

On Thu, 15 Aug 2024 12:11:59 +0000
Guillaume Stols <gstols@baylibre.com> wrote:

> Inspired from spi_get_device_match_data() helper: if OF or ACPI driver
> data is available, platform_get_device_id will return NULL because
> platform_match_id is not called, and pdev->id_entry is never populated.
> This helper function checks if match data is available, and otherwise
> returns the ID table data.

Likely no one will read the patch description but for these it's worth
some scary text to point out that the driver_data must be a pointer.
If people use an enum we need to do a dance to avoid the value 0.

We get quite a few buggy conversions of i2c/spi drivers as a result
of that little gotcha.

Otherwise this seems like a sensible addition to me.

Potentially worth pulling out as a precursor series with a few examples
of how it saves on repeating this same code block in a bunch
of existing drivers.  Up to GregKH I think for how he'd like this
(assuming he approves!)


Jonathan


> 
> Signed-off-by: Guillaume Stols <gstols@baylibre.com>
> ---
>  drivers/base/platform.c         | 12 ++++++++++++
>  include/linux/platform_device.h |  1 +
>  2 files changed, 13 insertions(+)
> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 4c3ee6521ba5..26e9a026eb05 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -1090,6 +1090,18 @@ static const struct platform_device_id *platform_match_id(
>  	return NULL;
>  }
>  
> +const void *platform_get_device_match_data(const struct platform_device *pdev)
> +{
> +	const struct platform_device_id *match;
> +
> +	match = device_get_match_data(&pdev->dev);
> +	if (match)
> +		return match;
> +
> +	return (const void *)platform_get_device_id(pdev)->driver_data;
> +}
> +EXPORT_SYMBOL_GPL(platform_get_device_match_data);
> +
>  #ifdef CONFIG_PM_SLEEP
>  
>  static int platform_legacy_suspend(struct device *dev, pm_message_t mesg)
> diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> index d422db6eec63..e2cc09ecc447 100644
> --- a/include/linux/platform_device.h
> +++ b/include/linux/platform_device.h
> @@ -45,6 +45,7 @@ struct platform_device {
>  };
>  
>  #define platform_get_device_id(pdev)	((pdev)->id_entry)
> +extern const void *platform_get_device_match_data(const struct platform_device *pdev);
>  
>  #define dev_is_platform(dev) ((dev)->bus == &platform_bus_type)
>  #define to_platform_device(x) container_of((x), struct platform_device, dev)
> 

Re: [PATCH 6/8] iio: adc: ad7606: Add PWM support for conversion trigger

[Jonathan Cameron]

On Thu, 15 Aug 2024 12:12:00 +0000
Guillaume Stols <gstols@baylibre.com> wrote:

> Until now, the conversion were triggered by setting high the GPIO
> connected to the convst pin. This commit gives the possibility to
> connect the convst pin to a PWM.
> Connecting a PWM allows to have a better control on the samplerate,
> but it must be handled with care, as it is completely decorrelated of
> the driver's busy pin handling.
> Hence it is not recommended to be used "as is" but must be exploited
> in conjonction with IIO backend, and for now only a sampling frequency
> of 2 kHz is available.
> 
> Signed-off-by: Guillaume Stols <gstols@baylibre.com>
Hi,

A few comments inline. I'm not particularly familiar with PWM
controls though so that bit could do with more eyes.

Jonathan

> ---

> +
> +static int ad7606_pwm_set_swing(struct ad7606_state *st)
> +{
> +	struct pwm_state cnvst_pwm_state;
> +
> +	if (!st->cnvst_pwm)
> +		return -EINVAL;
Blank line here and in similar cases where we have unrelated blocks
of code otherwise with no line between them.


> +	pwm_get_state(st->cnvst_pwm, &cnvst_pwm_state);
> +	cnvst_pwm_state.enabled = true;
> +	cnvst_pwm_state.duty_cycle = cnvst_pwm_state.period / 2;
> +	return pwm_apply_might_sleep(st->cnvst_pwm, &cnvst_pwm_state);
> +}
> +
> +static bool ad7606_pwm_is_swinging(struct ad7606_state *st)
> +{
> +	struct pwm_state cnvst_pwm_state;
> +
> +	if (!st->cnvst_pwm)
> +		return false;

Blank line here.

> +	pwm_get_state(st->cnvst_pwm, &cnvst_pwm_state);
> +	return cnvst_pwm_state.duty_cycle != cnvst_pwm_state.period &&
> +	       cnvst_pwm_state.duty_cycle != 0;
> +}
> +
> +static int ad7606_set_sampling_freq(struct ad7606_state *st, unsigned long freq)
> +{
> +	struct pwm_state cnvst_pwm_state;
> +	bool is_swinging = ad7606_pwm_is_swinging(st);
> +	bool is_high;
> +
> +	if (freq == 0)
> +		return -EINVAL;
> +
> +	/*Retrieve the previous state */
/* Ret

> +	pwm_get_state(st->cnvst_pwm, &cnvst_pwm_state);
> +	is_high = cnvst_pwm_state.duty_cycle == cnvst_pwm_state.period;
> +
> +	cnvst_pwm_state.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC, freq);
> +	cnvst_pwm_state.polarity = PWM_POLARITY_NORMAL;
> +	if (is_high)
> +		cnvst_pwm_state.duty_cycle = cnvst_pwm_state.period;
> +	else if (is_swinging)
> +		cnvst_pwm_state.duty_cycle = cnvst_pwm_state.period / 2;
> +
> +	return pwm_apply_might_sleep(st->cnvst_pwm, &cnvst_pwm_state);
> +}
> +
>  static int ad7606_read_samples(struct ad7606_state *st)
>  {
>  	unsigned int num = st->chip_info->num_channels - 1;
> @@ -141,9 +213,21 @@ static irqreturn_t ad7606_trigger_handler(int irq, void *p)
>  static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
>  {
>  	struct ad7606_state *st = iio_priv(indio_dev);
> +	struct pwm_state cnvst_pwm_state;
>  	int ret;
> +	bool pwm_swings = false;
>  
> -	gpiod_set_value(st->gpio_convst, 1);
> +	if (st->gpio_convst) {
> +		gpiod_set_value(st->gpio_convst, 1);
> +	} else {
> +		pwm_get_state(st->cnvst_pwm, &cnvst_pwm_state);
> +		/* Keep the current PWM state to switch it back off if needed*/
needed */

also back on or off I guess?

> +		if (ad7606_pwm_is_swinging(st))
> +			pwm_swings = true;
> +		ret = ad7606_pwm_set_high(st);
> +		if (!ret)
> +			return ret;
> +	}
>  	ret = wait_for_completion_timeout(&st->completion,
>  					  msecs_to_jiffies(1000));
>  	if (!ret) {
> @@ -154,7 +238,12 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
>  	ret = ad7606_read_samples(st);
>  	if (ret == 0)
>  		ret = st->data[ch];
> -
> +	if (!st->gpio_convst) {
> +		if (!pwm_swings)
> +			ret = ad7606_pwm_set_low(st);
> +		else
> +			ret = ad7606_pwm_set_swing(st);
> +	}
>  error_ret:
>  	gpiod_set_value(st->gpio_convst, 0);



> +static void ad7606_pwm_disable(void *data)
> +{
> +	pwm_disable(data);
> +}
> +
>  int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
>  		 const char *name, unsigned int id,
>  		 const struct ad7606_bus_ops *bops)
> @@ -635,20 +733,42 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
>  			return ret;
>  	}
>  
> -	st->trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
> -					  indio_dev->name,
> -					  iio_device_id(indio_dev));
> -	if (!st->trig)
> -		return -ENOMEM;
> -
> -	st->trig->ops = &ad7606_trigger_ops;
> -	iio_trigger_set_drvdata(st->trig, indio_dev);
> -	ret = devm_iio_trigger_register(dev, st->trig);
> -	if (ret)
> -		return ret;
> -
> -	indio_dev->trig = iio_trigger_get(st->trig);
> +	/* If convst pin is not defined, setup PWM*/
PWM */

> +	if (!st->gpio_convst) {
> +		st->cnvst_pwm = devm_pwm_get(dev, NULL);
> +		if (IS_ERR(st->cnvst_pwm))
> +			return PTR_ERR(st->cnvst_pwm);
> +		ret = devm_add_action_or_reset(dev, ad7606_pwm_disable,

Add a comment on why we are registering devm based disabling here.
I'm not sure where matching enable is.

> +					       st->cnvst_pwm);
> +		if (ret)
> +			return ret;
>  
> +		/*
> +		 * Set the sampling rate to 2 kHz so to be sure that the interruption can be
> +		 * handled between within a single pulse.
> +		 */
> +		ret = ad7606_set_sampling_freq(st, 2 * KILO);
> +		if (ret)
> +			return ret;
> +	} else {
> +		st->trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
> +						  indio_dev->name,
> +						  iio_device_id(indio_dev));
> +		if (!st->trig)
> +			return -ENOMEM;
> +		st->trig->ops = &ad7606_trigger_ops;
> +		iio_trigger_set_drvdata(st->trig, indio_dev);
> +		ret = devm_iio_trigger_register(dev, st->trig);
> +		if (ret)
> +			return ret;
> +		indio_dev->trig = iio_trigger_get(st->trig);
> +		ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> +						      &iio_pollfunc_store_time,
> +						      &ad7606_trigger_handler,
> +						      &ad7606_buffer_ops);
> +		if (ret)
> +			return ret;
> +	}
>  	ret = devm_request_threaded_irq(dev, irq,
>  					NULL,
>  					&ad7606_interrupt,
> @@ -657,13 +777,6 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
>  	if (ret)
>  		return ret;
>  
> -	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> -					      &iio_pollfunc_store_time,
> -					      &ad7606_trigger_handler,
> -					      &ad7606_buffer_ops);
> -	if (ret)
> -		return ret;
> -
>  	return devm_iio_device_register(dev, indio_dev);
>  }
>  EXPORT_SYMBOL_NS_GPL(ad7606_probe, IIO_AD7606);
> @@ -693,7 +806,6 @@ static int ad7606_resume(struct device *dev)
>  		gpiod_set_value(st->gpio_standby, 1);
>  		ad7606_reset(st);
>  	}
> -
Check for spurious noisy whitespace cleanup in patches.
it doesn't belong in a patch doing anything other that whitespace
changes.

>  	return 0;
>  }
>  
> diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
> index 0c6a88cc4695..60bac1894a91 100644
> --- a/drivers/iio/adc/ad7606.h
> +++ b/drivers/iio/adc/ad7606.h
> @@ -38,6 +38,8 @@
>  	AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),\
>  		0, BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO))
>  
> +struct pwm_device		*cnvst_pwm;

I'm going to guess that this started global then some internal review
or you guessed that wasn't going anywhere and added it to the state
structure below.  Then forgot to clean this up.
Anyhow this is correctly unused, get rid of it.

> +
>  /**
>   * struct ad7606_chip_info - chip specific information
>   * @channels:		channel specification
> @@ -94,6 +96,7 @@ struct ad7606_state {
>  	const struct ad7606_bus_ops	*bops;
>  	unsigned int			range[16];
>  	unsigned int			oversampling;
> +	struct pwm_device		*cnvst_pwm;
This structure had docs that need updating.

>  	void __iomem			*base_address;
>  	bool				sw_mode_en;
>  	const unsigned int		*scale_avail;
> 

Re: [PATCH 7/8] iio: adc: ad7606: Switch to xxx_get_device_match_data

[Jonathan Cameron]

On Thu, 15 Aug 2024 12:12:01 +0000
Guillaume Stols <gstols@baylibre.com> wrote:

> On the parallel version, the current implementation is only compatible
> with id tables and won't work with fx_nodes. So in this commit, the goal
> is to switch to use get_device_match_data, in order to simplify the
> logic of retrieving chip data.
> 
> Also, chip info is moved in the .h file so to be accessible to all the
> driver files that can set a pointer to the corresponding chip as the
> driver data.

This means each driver gets their own copy.

Better to use an extern in the header and keep the actual data
in the core module.

Otherwise LGTM.

Re: [PATCH 8/8] iio:adc:ad7606: Add iio-backend support

[Jonathan Cameron]

On Thu, 15 Aug 2024 12:12:02 +0000
Guillaume Stols <gstols@baylibre.com> wrote:

> - Basic support for iio backend.
> - Supports IIO_CHAN_INFO_SAMP_FREQ R/W.
> - Only hardware mode is available, and that IIO_CHAN_INFO_RAW is not
>   supported if iio-backend mode is selected.
> 
> A small correction was added to the driver's file name in the Kconfig
> file's description.
>
I'm going to want Nuno's input on this.  Given it's the summer that may
take a little while, so in meantime a few comments inline.

Jonathan

 
> Signed-off-by: Guillaume Stols <gstols@baylibre.com>
> ---
>  drivers/iio/adc/Kconfig      |   3 +-
>  drivers/iio/adc/ad7606.c     | 103 +++++++++++++++++++++++++++++++++++--------
>  drivers/iio/adc/ad7606.h     |  16 +++++++
>  drivers/iio/adc/ad7606_par.c |  98 +++++++++++++++++++++++++++++++++++++++-
>  4 files changed, 200 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 88e8ce2e78b3..01248b6df868 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -227,9 +227,10 @@ config AD7606_IFACE_PARALLEL
>  	help
>  	  Say yes here to build parallel interface support for Analog Devices:
>  	  ad7605-4, ad7606, ad7606-6, ad7606-4 analog to digital converters (ADC).
> +	  It also support iio_backended devices for AD7606B.
>  
>  	  To compile this driver as a module, choose M here: the
> -	  module will be called ad7606_parallel.
> +	  module will be called ad7606_par.
If we can avoid a rename that would be good.  Or was this always wrong?
If so spin a fix patch before this one.

>  
>  config AD7606_IFACE_SPI
>  	tristate "Analog Devices AD7606 ADC driver with spi interface support"
> diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
> index 99d5ca5c2348..a753d5caa9f8 100644
> --- a/drivers/iio/adc/ad7606.c
> +++ b/drivers/iio/adc/ad7606.c
> @@ -21,6 +21,7 @@
>  #include <linux/util_macros.h>
>  #include <linux/units.h>
>  
> +#include <linux/iio/backend.h>
>  #include <linux/iio/iio.h>
>  #include <linux/iio/buffer.h>
>  #include <linux/iio/sysfs.h>
> @@ -148,7 +149,15 @@ static int ad7606_set_sampling_freq(struct ad7606_state *st, unsigned long freq)
>  
>  static int ad7606_read_samples(struct ad7606_state *st)
>  {
> -	unsigned int num = st->chip_info->num_channels - 1;
> +	unsigned int num = st->chip_info->num_channels;
> +
> +	/*
> +	 * Timestamp channel does not contain sample, and no timestamp channel if
> +	 * backend is used.
> +	 */
> +	if (!st->back)
> +		num--;
> +
>  	u16 *data = st->data;
>  	int ret;
>  
> @@ -220,11 +229,15 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
>  		if (!ret)
>  			return ret;
>  	}
> -	ret = wait_for_completion_timeout(&st->completion,
> -					  msecs_to_jiffies(1000));
> -	if (!ret) {
> -		ret = -ETIMEDOUT;
> -		goto error_ret;
> +
> +	/* backend manages interruptions by itself.*/
> +	if (!st->back) {
> +		ret = wait_for_completion_timeout(&st->completion,
> +						  msecs_to_jiffies(1000));
> +		if (!ret) {
> +			ret = -ETIMEDOUT;
> +			goto error_ret;
> +		}
>  	}
>  
>  	ret = ad7606_read_samples(st);
> @@ -271,6 +284,12 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
>  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>  		*val = st->oversampling;
>  		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		pwm_get_state_hw(st->cnvst_pwm, &cnvst_pwm_state);
> +		/* If the PWM is swinging, return the real frequency, otherwise 0 */
So this only exists for the pwm case. In that case can we split the channel definitions
into versions with an without this and register just the right one.

A sampling frequency of 0 usually means no sampling, not that we can tell what it
is.  If we can't tell don't provide the file.


> +		*val = ad7606_pwm_is_swinging(st) ?
> +			DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, cnvst_pwm_state.period) : 0;
> +		return IIO_VAL_INT;
>  	}
>  	return -EINVAL;
>  }
> @@ -360,6 +379,8 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
>  			return ret;
>  
>  		return 0;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		return ad7606_set_sampling_freq(st, val);
>  	default:
>  		return -EINVAL;
>  	}


>  static const struct iio_trigger_ops ad7606_trigger_ops = {
> @@ -602,8 +660,6 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
>  	indio_dev->channels = st->chip_info->channels;
>  	indio_dev->num_channels = st->chip_info->num_channels;
>  
> -	init_completion(&st->completion);
> -
>  	ret = ad7606_reset(st);
>  	if (ret)
>  		dev_warn(st->dev, "failed to RESET: no RESET GPIO specified\n");
> @@ -635,7 +691,7 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
>  			return ret;
>  	}
>  
> -	/* If convst pin is not defined, setup PWM*/
> +	/* If convst pin is not defined, setup PWM */
Push into earlier patch.  Check for this sort of fix being in wrong patch
before sending a series. It just adds noise to patch and to reviews.

>  	if (!st->gpio_convst) {
>  		st->cnvst_pwm = devm_pwm_get(dev, NULL);
>  		if (IS_ERR(st->cnvst_pwm))

...

> diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
> index aab8fefb84be..9a098cd77812 100644
> --- a/drivers/iio/adc/ad7606.h
> +++ b/drivers/iio/adc/ad7606.h
> @@ -34,6 +34,12 @@
>  		BIT(IIO_CHAN_INFO_SCALE),		\
>  		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO))
>  
> +#define AD7606_BI_CHANNEL(num)				\
> +	AD760X_CHANNEL(num, 0,				\
> +		BIT(IIO_CHAN_INFO_SCALE),		\
> +		BIT(IIO_CHAN_INFO_SAMP_FREQ) |		\
> +		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO))
> +
>  #define AD7616_CHANNEL(num)	\
>  	AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),\
>  		0, BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO))
> @@ -61,6 +67,7 @@ enum ad7606_supported_device_ids {
>   * @os_req_reset	some devices require a reset to update oversampling
>   * @init_delay_ms	required delay in miliseconds for initialization
>   *			after a restart
> + * @has_backend		defines if a backend is available for the given chip

That seems to me more of a case of does the driver support it.
Linux kernel code has no way of knowing if a backend hardware exists
or not.  Modify the comment to speak about if we know it works.

Or is there something fundamental that stops the backend approach
working with some devices?

Why does the driver need this flag?

>   */
>  struct ad7606_chip_info {
>  	enum ad7606_supported_device_ids id;
> @@ -71,6 +78,7 @@ struct ad7606_chip_info {
>  	unsigned int			oversampling_num;
>  	bool				os_req_reset;
>  	unsigned long			init_delay_ms;
> +	bool				has_backend;
>  };
>  


> diff --git a/drivers/iio/adc/ad7606_par.c b/drivers/iio/adc/ad7606_par.c
> index d83c0edc1e31..5c8a04556e25 100644
> --- a/drivers/iio/adc/ad7606_par.c
> +++ b/drivers/iio/adc/ad7606_par.c
> @@ -3,6 +3,8 @@
>   * AD7606 Parallel Interface ADC driver
>   *
>   * Copyright 2011 Analog Devices Inc.
> + * Copyright 2024 Analog Devices Inc.
> + * Copyright 2024 BayLibre SAS.
>   */
>  
>  #include <linux/mod_devicetable.h>
> @@ -11,10 +13,86 @@
>  #include <linux/types.h>
>  #include <linux/err.h>
>  #include <linux/io.h>
> +#include <linux/pwm.h>
> +#include <linux/gpio.h>
> +#include <linux/delay.h>
Not on any order currently but try to minimize a future series
that might clean this up.  Preference is alphabetical though fine
to have a trailing block of IIO headers, then driver specific
ones after that. You can either fix the current order in a
separate patch, or just put your new headers in approximately the write place.

>  
>  #include <linux/iio/iio.h>
> +#include <linux/iio/backend.h>
>  #include "ad7606.h"



> +static int ad7606_bi_setup_iio_backend(struct device *dev, struct iio_dev *indio_dev)
> +{
> +		struct ad7606_state *st = iio_priv(indio_dev);
Why 2 tabs?

> +		unsigned int ret, c;
> +
> +		st->back = devm_iio_backend_get(dev, NULL);
> +		if (IS_ERR(st->back))
> +			return PTR_ERR(st->back);
> +
> +		/* If the device is iio_backend powered the PWM is mandatory */
> +		if (!st->cnvst_pwm)
> +			return -EINVAL;
> +
> +		ret = devm_iio_backend_request_buffer(dev, st->back, indio_dev);
> +		if (ret)
> +			return ret;
> +
> +		ret = devm_iio_backend_enable(dev, st->back);
> +		if (ret)
> +			return ret;
> +
> +		struct iio_backend_data_fmt data = {
> +			.sign_extend = true,
> +			.enable = true,
> +		};
> +		for (c = 0; c < indio_dev->num_channels; c++) {
> +			ret = iio_backend_data_format_set(st->back, c, &data);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		indio_dev->channels = ad7606b_bi_channels;
> +		indio_dev->num_channels = 8;
> +
> +		return 0;
> +}
> +
> +static const struct ad7606_bus_ops ad7606_bi_bops = {
> +	.iio_backend_config = ad7606_bi_setup_iio_backend,
> +	.update_scan_mode = ad7606_bi_update_scan_mode,
> +};
> +#endif
> +
>  static int ad7606_par16_read_block(struct device *dev,
>  				   int count, void *buf)
>  {
> @@ -52,7 +130,20 @@ static int ad7606_par_probe(struct platform_device *pdev)
>  	void __iomem *addr;
>  	resource_size_t remap_size;
>  	int irq;
> -
> +#ifdef CONFIG_IIO_BACKEND
> +	struct iio_backend *back;
> +
> +	/*For now, only the AD7606B is backend compatible.*/
/* For ... ble. */

> +	if (chip_info->has_backend) {
> +		back = devm_iio_backend_get(&pdev->dev, NULL);
> +		if (IS_ERR(back))
> +			return PTR_ERR(back);
> +
> +		return ad7606_probe(&pdev->dev, 0, NULL,
> +				    chip_info,
> +				    &ad7606_bi_bops);

Short wrap. Aim for 80 char limit unless it hurts readability a lot.

> +	}
> +#endif
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq < 0)
>  		return irq;
> @@ -74,6 +165,7 @@ static const struct platform_device_id ad7606_driver_ids[] = {
>  	{ .name	= "ad7606-4", .driver_data = (kernel_ulong_t)&ad7606_4_info, },
>  	{ .name	= "ad7606-6", .driver_data = (kernel_ulong_t)&ad7606_6_info, },
>  	{ .name	= "ad7606-8", .driver_data = (kernel_ulong_t)&ad7606_8_info, },
> +	{ .name	= "ad7606b", .driver_data = (kernel_ulong_t)&ad7606b_info, },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(platform, ad7606_driver_ids);
> @@ -83,6 +175,7 @@ static const struct of_device_id ad7606_of_match[] = {
>  	{ .compatible = "adi,ad7606-4", .data = &ad7606_4_info },
>  	{ .compatible = "adi,ad7606-6", .data = &ad7606_6_info },
>  	{ .compatible = "adi,ad7606-8", .data = &ad7606_8_info },
> +	{ .compatible = "adi,ad7606b", .data = &ad7606b_info },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, ad7606_of_match);
> @@ -102,3 +195,6 @@ MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
>  MODULE_DESCRIPTION("Analog Devices AD7606 ADC");
>  MODULE_LICENSE("GPL v2");
>  MODULE_IMPORT_NS(IIO_AD7606);
> +#ifdef CONFIG_IIO_BACKEND
> +MODULE_IMPORT_NS(IIO_BACKEND);

I'd not bother with config guards.  Importing a namespace we don't
use should be harmless.

> +#endif
> 

Re: [PATCH 4/8] pwm: Export pwm_get_state_hw

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2330 bytes --]

On Thu, Aug 15, 2024 at 12:11:58PM +0000, Guillaume Stols wrote:
> This function can be used in some other drivers, for instance when we
> want to retrieve the real frequency vs the one that was asked.

I'd write:

	For some drivers (here: the upcoming ad7606 adc driver) it's important
	to know the actually configured PWM state. This is in general different
	from the state returned by pwm_get_state() (i.e. the last applied state)
	because most hardware doesn't have nano second granularity. So make
	pwm_get_state_hw() a public function.

> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 21fca27bb8a3..82e05ed88310 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -651,7 +651,7 @@ int pwm_apply_atomic(struct pwm_device *pwm, const struct pwm_state *state)
>  }
>  EXPORT_SYMBOL_GPL(pwm_apply_atomic);
>  
> -static int pwm_get_state_hw(struct pwm_device *pwm, struct pwm_state *state)
> +int pwm_get_state_hw(struct pwm_device *pwm, struct pwm_state *state)
>  {
>  	struct pwm_chip *chip = pwm->chip;
>  	const struct pwm_ops *ops = chip->ops;
> @@ -685,6 +685,7 @@ static int pwm_get_state_hw(struct pwm_device *pwm, struct pwm_state *state)
>  
>  	return ret;
>  }
> +EXPORT_SYMBOL_GPL(pwm_get_state_hw);

Now that this is a public function, a kernel doc for it would be nice.

>  /**
>   * pwm_adjust_config() - adjust the current PWM config to the PWM arguments
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index fd100c27f109..d48ea3051e28 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -369,6 +369,7 @@ int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state)
>  int pwm_apply_atomic(struct pwm_device *pwm, const struct pwm_state *state);
>  int pwm_adjust_config(struct pwm_device *pwm);
>  
> +int pwm_get_state_hw(struct pwm_device *pwm, struct pwm_state *state);

Nitpick: pwm_get_state_hw() is defined in core.c before
pwm_adjust_config(). Please keep this order in the header.

>  /**
>   * pwm_config() - change a PWM device configuration
>   * @pwm: PWM device

Your patch was PGP signed, but I failed to find your key in the kernel
key repo and on https://keys.openpgp.org. To make your signature
actually useful, you might want to fix that.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/8] dt-bindings: iio: adc: ad7606: Add iio backend bindings

[David Lechner]

On 8/17/24 10:09 AM, Jonathan Cameron wrote:
> On Thu, 15 Aug 2024 12:11:56 +0000
> Guillaume Stols <gstols@baylibre.com> wrote:
> 
>> Add the required properties for iio-backend support, as well as an
>> example and the conditions to mutually exclude interruption and
>> conversion trigger with iio-backend.
>> The iio-backend's function is to controls the communication, and thus the
>> interruption pin won't be available anymore.
>> As a consequence, the conversion pin must be controlled externally since
>> we will miss information about when every single conversion cycle (i.e
>> conversion + data transfert) ends, hence a PWM is introduced to trigger
> 
> transfer
> 
>> the conversions.
>>
>> Signed-off-by: Guillaume Stols <gstols@baylibre.com>
>> ---
>>  .../devicetree/bindings/iio/adc/adi,ad7606.yaml    | 75 +++++++++++++++++++++-
>>  1 file changed, 72 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
>> index c0008d36320f..4b324f7e3207 100644
>> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
>> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
>> @@ -114,13 +114,28 @@ properties:
>>        assumed that the pins are hardwired to VDD.
>>      type: boolean
>>  
>> +  pwms:
>> +    description:
>> +      In case the conversion is triggered by a PWM instead of a GPIO plugged to
>> +      the CONVST pin, the PWM must be referenced.
>> +    minItems: 1
>> +    maxItems: 2
>> +
>> +  pwm-names:
>> +    minItems: 1
>> +    maxItems: 2
>> +
>> +  io-backends:
>> +    description:
>> +      A reference to the iio-backend, which is responsible handling the BUSY
>> +      pin's falling edge and communication.
>> +      An example of backend can be found at
>> +      http://analogdevicesinc.github.io/hdl/library/axi_ad7606x/index.html
>> +
>>  required:
>>    - compatible
>> -  - reg
> 
> I think we still want a reg, but only to differentiate multiple instances
> perhaps.

In light of the recent discussions on the similar AXI DAC
support for AD3552R [1], should we consider some of the same
things here?

Essentially, the AXI ADC IP block in this series is acting as
a parallel bus provider for the AD7606 chip. This is used both
for configuring registers on the chip and "offloading" for high
speed data capture.

So this would mean...

1. We should add a new compatible string to iio/adc/adi,axi-adc.yaml
   for the specialized version of the AXI ADC IP that is used with
   AD7606 and similar ADCs.
2. In the .dts, the AXI ADC node should be the parent of the ADC node
   since the AXI ADC IP is providing the parallel bus to the ADC.


[1]: https://lore.kernel.org/linux-iio/20240903203935.358a1423@jic23-huawei/

> 
>>    - avcc-supply
>>    - vdrive-supply
> 
> 
> 

Re: [PATCH 8/8] iio:adc:ad7606: Add iio-backend support

[Nuno Sá]

On Thu, 2024-08-15 at 12:12 +0000, Guillaume Stols wrote:
> - Basic support for iio backend.
> - Supports IIO_CHAN_INFO_SAMP_FREQ R/W.
> - Only hardware mode is available, and that IIO_CHAN_INFO_RAW is not
>   supported if iio-backend mode is selected.
> 
> A small correction was added to the driver's file name in the Kconfig
> file's description.
> 
> Signed-off-by: Guillaume Stols <gstols@baylibre.com>
> ---

Hi Guillaume,

Some initial feedback from me...

>  drivers/iio/adc/Kconfig      |   3 +-
>  drivers/iio/adc/ad7606.c     | 103 +++++++++++++++++++++++++++++++++++-------
> -
>  drivers/iio/adc/ad7606.h     |  16 +++++++
>  drivers/iio/adc/ad7606_par.c |  98 +++++++++++++++++++++++++++++++++++++++-
>  4 files changed, 200 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 88e8ce2e78b3..01248b6df868 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -227,9 +227,10 @@ config AD7606_IFACE_PARALLEL
>  	help
>  	  Say yes here to build parallel interface support for Analog
> Devices:
>  	  ad7605-4, ad7606, ad7606-6, ad7606-4 analog to digital converters
> (ADC).
> +	  It also support iio_backended devices for AD7606B.
>  
>  	  To compile this driver as a module, choose M here: the
> -	  module will be called ad7606_parallel.
> +	  module will be called ad7606_par.
>  
>  config AD7606_IFACE_SPI
>  	tristate "Analog Devices AD7606 ADC driver with spi interface
> support"
> diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
> index 99d5ca5c2348..a753d5caa9f8 100644
> --- a/drivers/iio/adc/ad7606.c
> +++ b/drivers/iio/adc/ad7606.c
> @@ -21,6 +21,7 @@
>  #include <linux/util_macros.h>
>  #include <linux/units.h>
>  
> +#include <linux/iio/backend.h>
>  #include <linux/iio/iio.h>
>  #include <linux/iio/buffer.h>
>  #include <linux/iio/sysfs.h>
> @@ -148,7 +149,15 @@ static int ad7606_set_sampling_freq(struct ad7606_state
> *st, unsigned long freq)
>  
>  static int ad7606_read_samples(struct ad7606_state *st)
>  {
> -	unsigned int num = st->chip_info->num_channels - 1;
> +	unsigned int num = st->chip_info->num_channels;
> +
> +	/*
> +	 * Timestamp channel does not contain sample, and no timestamp
> channel if
> +	 * backend is used.
> +	 */
> +	if (!st->back)
> +		num--;
> +
>  	u16 *data = st->data;
>  	int ret;
>  
> @@ -220,11 +229,15 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev,
> unsigned int ch)
>  		if (!ret)
>  			return ret;
>  	}
> -	ret = wait_for_completion_timeout(&st->completion,
> -					  msecs_to_jiffies(1000));
> -	if (!ret) {
> -		ret = -ETIMEDOUT;
> -		goto error_ret;
> +
> +	/* backend manages interruptions by itself.*/

missing space before closing the comment (also not sure the comments adds much)

> +	if (!st->back) {
> +		ret = wait_for_completion_timeout(&st->completion,
> +						  msecs_to_jiffies(1000));
> +		if (!ret) {
> +			ret = -ETIMEDOUT;
> +			goto error_ret;
> +		}
>  	}
>  
>  	ret = ad7606_read_samples(st);
> @@ -271,6 +284,12 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
>  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>  		*val = st->oversampling;
>  		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		pwm_get_state_hw(st->cnvst_pwm, &cnvst_pwm_state);
> +		/* If the PWM is swinging, return the real frequency,
> otherwise 0 */
> +		*val = ad7606_pwm_is_swinging(st) ?
> +			DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC,
> cnvst_pwm_state.period) : 0;
> +		return IIO_VAL_INT;
>  	}
>  	return -EINVAL;
>  }
> @@ -360,6 +379,8 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
>  			return ret;
>  
>  		return 0;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		return ad7606_set_sampling_freq(st, val);
>  	default:
>  		return -EINVAL;
>  	}
> @@ -482,7 +503,6 @@ static int ad7606_buffer_postenable(struct iio_dev
> *indio_dev)
>  	struct ad7606_state *st = iio_priv(indio_dev);
>  
>  	gpiod_set_value(st->gpio_convst, 1);
> -	ad7606_pwm_set_swing(st);
>  
>  	return 0;
>  }
> @@ -492,19 +512,53 @@ static int ad7606_buffer_predisable(struct iio_dev
> *indio_dev)
>  	struct ad7606_state *st = iio_priv(indio_dev);
>  
>  	gpiod_set_value(st->gpio_convst, 0);
> -	ad7606_pwm_set_low(st);
>  
>  	return 0;
>  }
>  
> +static int ad7606_pwm_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct ad7606_state *st = iio_priv(indio_dev);
> +
> +	return ad7606_pwm_set_swing(st);
> +}
> +
> +static int ad7606_pwm_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	struct ad7606_state *st = iio_priv(indio_dev);
> +
> +	return ad7606_pwm_set_low(st);
> +}

Maybe I'm missing something but are we removing the gpiod calls?
> +
> +static int ad7606_update_scan_mode(struct iio_dev *indio_dev,
> +				   const unsigned long *scan_mask)
> +{
> +	struct ad7606_state *st = iio_priv(indio_dev);
> +
> +	/* The update scan mode is only for iio backend compatible drivers.
> +	 * If the specific update_scan_mode is not defined in the bus ops,
> +	 * just do nothing and return 0.
> +	 */
> +	if (st->bops->update_scan_mode)
> +		return st->bops->update_scan_mode(indio_dev, scan_mask);
> +	else
> +		return 0;

Redundant else

> +}
> +
>  static const struct iio_buffer_setup_ops ad7606_buffer_ops = {
>  	.postenable = &ad7606_buffer_postenable,
>  	.predisable = &ad7606_buffer_predisable,
>  };
>  
> +static const struct iio_buffer_setup_ops ad7606_pwm_buffer_ops = {
> +	.postenable = &ad7606_pwm_buffer_postenable,
> +	.predisable = &ad7606_pwm_buffer_predisable,
> +};
> +
>  static const struct iio_info ad7606_info_no_os_or_range = {
>  	.read_raw = &ad7606_read_raw,
>  	.validate_trigger = &ad7606_validate_trigger,
> +	.update_scan_mode = &ad7606_update_scan_mode,
>  };
>  
>  static const struct iio_info ad7606_info_os_and_range = {
> @@ -512,6 +566,7 @@ static const struct iio_info ad7606_info_os_and_range = {
>  	.write_raw = &ad7606_write_raw,
>  	.attrs = &ad7606_attribute_group_os_and_range,
>  	.validate_trigger = &ad7606_validate_trigger,
> +	.update_scan_mode = &ad7606_update_scan_mode,
>  };
>  
>  static const struct iio_info ad7606_info_os_range_and_debug = {
> @@ -520,6 +575,7 @@ static const struct iio_info
> ad7606_info_os_range_and_debug = {
>  	.debugfs_reg_access = &ad7606_reg_access,
>  	.attrs = &ad7606_attribute_group_os_and_range,
>  	.validate_trigger = &ad7606_validate_trigger,
> +	.update_scan_mode = &ad7606_update_scan_mode,
>  };
>  
>  static const struct iio_info ad7606_info_os = {
> @@ -527,6 +583,7 @@ static const struct iio_info ad7606_info_os = {
>  	.write_raw = &ad7606_write_raw,
>  	.attrs = &ad7606_attribute_group_os,
>  	.validate_trigger = &ad7606_validate_trigger,
> +	.update_scan_mode = &ad7606_update_scan_mode,
>  };
>  
>  static const struct iio_info ad7606_info_range = {
> @@ -534,6 +591,7 @@ static const struct iio_info ad7606_info_range = {
>  	.write_raw = &ad7606_write_raw,
>  	.attrs = &ad7606_attribute_group_range,
>  	.validate_trigger = &ad7606_validate_trigger,
> +	.update_scan_mode = &ad7606_update_scan_mode,
>  };
>  
>  static const struct iio_trigger_ops ad7606_trigger_ops = {
> @@ -602,8 +660,6 @@ int ad7606_probe(struct device *dev, int irq, void __iomem
> *base_address,
>  	indio_dev->channels = st->chip_info->channels;
>  	indio_dev->num_channels = st->chip_info->num_channels;
>  
> -	init_completion(&st->completion);
> -
>  	ret = ad7606_reset(st);
>  	if (ret)
>  		dev_warn(st->dev, "failed to RESET: no RESET GPIO
> specified\n");
> @@ -635,7 +691,7 @@ int ad7606_probe(struct device *dev, int irq, void __iomem
> *base_address,
>  			return ret;
>  	}
>  
> -	/* If convst pin is not defined, setup PWM*/
> +	/* If convst pin is not defined, setup PWM */
>  	if (!st->gpio_convst) {
>  		st->cnvst_pwm = devm_pwm_get(dev, NULL);
>  		if (IS_ERR(st->cnvst_pwm))
> @@ -671,14 +727,25 @@ int ad7606_probe(struct device *dev, int irq, void
> __iomem *base_address,
>  		if (ret)
>  			return ret;
>  	}
> -	ret = devm_request_threaded_irq(dev, irq,
> -					NULL,
> -					&ad7606_interrupt,
> -					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> -					chip_info->name, indio_dev);
> -	if (ret)
> -		return ret;
>  
> +	if (st->bops->iio_backend_config) {
> +		st->bops->iio_backend_config(dev, indio_dev);
> +		indio_dev->setup_ops = &ad7606_pwm_buffer_ops;

Ignoring error code

> +	} else {
> +		/* Reserve the PWM use only for backend (force gpio_convst
> definition)*/
> +		if (!st->gpio_convst)
> +			return dev_err_probe(dev, -EINVAL,
> +					     "Convst pin must be defined when
> not in backend mode");
> +
> +		init_completion(&st->completion);
> +		ret = devm_request_threaded_irq(dev, irq,
> +						NULL,
> +						&ad7606_interrupt,
> +						IRQF_TRIGGER_FALLING |
> IRQF_ONESHOT,
> +						chip_info->name, indio_dev);
> +		if (ret)
> +			return ret;
> +	}

Are we still calling devm_iio_triggered_buffer_setup() in case we have a backend
device?

>  	return devm_iio_device_register(dev, indio_dev);
>  }

...

> 
> +#ifdef CONFIG_IIO_BACKEND

Not a fan of this #ifef... It's not that much code so I would just select
IIO_BACKEND for this driver. In fact, I don't think we can separately enable
IIO_BACKEND in the menuconfig menu?

> +static const struct iio_chan_spec ad7606b_bi_channels[] = {
> +	AD7606_BI_CHANNEL(0),
> +	AD7606_BI_CHANNEL(1),
> +	AD7606_BI_CHANNEL(2),
> +	AD7606_BI_CHANNEL(3),
> +	AD7606_BI_CHANNEL(4),
> +	AD7606_BI_CHANNEL(5),
> +	AD7606_BI_CHANNEL(6),
> +	AD7606_BI_CHANNEL(7),
> +};
> +
> +static int ad7606_bi_update_scan_mode(struct iio_dev *indio_dev, const
> unsigned long *scan_mask)
> +{
> +	struct ad7606_state *st = iio_priv(indio_dev);
> +	unsigned int c, ret;
> +
> +	for (c = 0; c < indio_dev->num_channels; c++) {
> +		if (test_bit(c, scan_mask))
> +			ret = iio_backend_chan_enable(st->back, c);
> +		else
> +			ret = iio_backend_chan_disable(st->back, c);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ad7606_bi_setup_iio_backend(struct device *dev, struct iio_dev
> *indio_dev)
> +{
> +		struct ad7606_state *st = iio_priv(indio_dev);
> +		unsigned int ret, c;
> +
> +		st->back = devm_iio_backend_get(dev, NULL);
> +		if (IS_ERR(st->back))
> +			return PTR_ERR(st->back);
> +
> +		/* If the device is iio_backend powered the PWM is mandatory
> */
> +		if (!st->cnvst_pwm)
> +			return -EINVAL;
> +
> +		ret = devm_iio_backend_request_buffer(dev, st->back,
> indio_dev);
> +		if (ret)
> +			return ret;
> +
> +		ret = devm_iio_backend_enable(dev, st->back);
> +		if (ret)
> +			return ret;
> +
> +		struct iio_backend_data_fmt data = {
> +			.sign_extend = true,
> +			.enable = true,
> +		};

I would follow typical kernel coding style and have this declared at the
beginning of the function.

> +		for (c = 0; c < indio_dev->num_channels; c++) {
> +			ret = iio_backend_data_format_set(st->back, c,
> &data);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		indio_dev->channels = ad7606b_bi_channels;
> +		indio_dev->num_channels = 8;
> +
> +		return 0;
> +}
> +
> +static const struct ad7606_bus_ops ad7606_bi_bops = {
> +	.iio_backend_config = ad7606_bi_setup_iio_backend,
> +	.update_scan_mode = ad7606_bi_update_scan_mode,
> +};
> +#endif
> +
>  static int ad7606_par16_read_block(struct device *dev,
>  				   int count, void *buf)
>  {
> @@ -52,7 +130,20 @@ static int ad7606_par_probe(struct platform_device *pdev)
>  	void __iomem *addr;
>  	resource_size_t remap_size;
>  	int irq;
> -
> +#ifdef CONFIG_IIO_BACKEND
> +	struct iio_backend *back;
> +
> +	/*For now, only the AD7606B is backend compatible.*/
> +	if (chip_info->has_backend) {
> +		back = devm_iio_backend_get(&pdev->dev, NULL);
> +		if (IS_ERR(back))
> +			return PTR_ERR(back);
> +
> +		return ad7606_probe(&pdev->dev, 0, NULL,
> +				    chip_info,
> +				    &ad7606_bi_bops);
> +	}
> +#endif

Not sure I follow the above? You also get the backend in
ad7606_bi_setup_iio_backend()? So it seems to be that the has_backend flag is
not really needed?

- Nuno Sá

Re: [PATCH 2/8] dt-bindings: iio: adc: ad7606: Add iio backend bindings

[Jonathan Cameron]

On Wed, 4 Sep 2024 11:54:30 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 8/17/24 10:09 AM, Jonathan Cameron wrote:
> > On Thu, 15 Aug 2024 12:11:56 +0000
> > Guillaume Stols <gstols@baylibre.com> wrote:
> >   
> >> Add the required properties for iio-backend support, as well as an
> >> example and the conditions to mutually exclude interruption and
> >> conversion trigger with iio-backend.
> >> The iio-backend's function is to controls the communication, and thus the
> >> interruption pin won't be available anymore.
> >> As a consequence, the conversion pin must be controlled externally since
> >> we will miss information about when every single conversion cycle (i.e
> >> conversion + data transfert) ends, hence a PWM is introduced to trigger  
> > 
> > transfer
> >   
> >> the conversions.
> >>
> >> Signed-off-by: Guillaume Stols <gstols@baylibre.com>
> >> ---
> >>  .../devicetree/bindings/iio/adc/adi,ad7606.yaml    | 75 +++++++++++++++++++++-
> >>  1 file changed, 72 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> >> index c0008d36320f..4b324f7e3207 100644
> >> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> >> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> >> @@ -114,13 +114,28 @@ properties:
> >>        assumed that the pins are hardwired to VDD.
> >>      type: boolean
> >>  
> >> +  pwms:
> >> +    description:
> >> +      In case the conversion is triggered by a PWM instead of a GPIO plugged to
> >> +      the CONVST pin, the PWM must be referenced.
> >> +    minItems: 1
> >> +    maxItems: 2
> >> +
> >> +  pwm-names:
> >> +    minItems: 1
> >> +    maxItems: 2
> >> +
> >> +  io-backends:
> >> +    description:
> >> +      A reference to the iio-backend, which is responsible handling the BUSY
> >> +      pin's falling edge and communication.
> >> +      An example of backend can be found at
> >> +      http://analogdevicesinc.github.io/hdl/library/axi_ad7606x/index.html
> >> +
> >>  required:
> >>    - compatible
> >> -  - reg  
> > 
> > I think we still want a reg, but only to differentiate multiple instances
> > perhaps.  
> 
> In light of the recent discussions on the similar AXI DAC
> support for AD3552R [1], should we consider some of the same
> things here?
> 
> Essentially, the AXI ADC IP block in this series is acting as
> a parallel bus provider for the AD7606 chip. This is used both
> for configuring registers on the chip and "offloading" for high
> speed data capture.
> 
> So this would mean...
> 
> 1. We should add a new compatible string to iio/adc/adi,axi-adc.yaml
>    for the specialized version of the AXI ADC IP that is used with
>    AD7606 and similar ADCs.
> 2. In the .dts, the AXI ADC node should be the parent of the ADC node
>    since the AXI ADC IP is providing the parallel bus to the ADC.

Ah. I'd completely failed to notice this didn't have a separate control
bus.  The existing ad7606 only does reading so I assumed that the
data path couldn't carry configuration data.  Looking at this patch
is that still the case?

If so I think it is less critical to represent the bus given the history
of not doing so in this driver.   It would be a nice to have though.

Jonathan


> 
> 
> [1]: https://lore.kernel.org/linux-iio/20240903203935.358a1423@jic23-huawei/
> 
> >   
> >>    - avcc-supply
> >>    - vdrive-supply  
> > 
> > 
> >   
> 

Re: [PATCH 8/8] iio:adc:ad7606: Add iio-backend support

[Guillaume Stols]

On 8/17/24 17:47, Jonathan Cameron wrote:
> On Thu, 15 Aug 2024 12:12:02 +0000
> Guillaume Stols <gstols@baylibre.com> wrote:
>
>> - Basic support for iio backend.
>> - Supports IIO_CHAN_INFO_SAMP_FREQ R/W.
>> - Only hardware mode is available, and that IIO_CHAN_INFO_RAW is not
>>    supported if iio-backend mode is selected.
>>
>> A small correction was added to the driver's file name in the Kconfig
>> file's description.
>>
> I'm going to want Nuno's input on this.  Given it's the summer that may
> take a little while, so in meantime a few comments inline.
>
> Jonathan
>
>   
>> Signed-off-by: Guillaume Stols <gstols@baylibre.com>
>> ---
>>   drivers/iio/adc/Kconfig      |   3 +-
>>   drivers/iio/adc/ad7606.c     | 103 +++++++++++++++++++++++++++++++++++--------
>>   drivers/iio/adc/ad7606.h     |  16 +++++++
>>   drivers/iio/adc/ad7606_par.c |  98 +++++++++++++++++++++++++++++++++++++++-
>>   4 files changed, 200 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 88e8ce2e78b3..01248b6df868 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -227,9 +227,10 @@ config AD7606_IFACE_PARALLEL
>>   	help
>>   	  Say yes here to build parallel interface support for Analog Devices:
>>   	  ad7605-4, ad7606, ad7606-6, ad7606-4 analog to digital converters (ADC).
>> +	  It also support iio_backended devices for AD7606B.
>>   
>>   	  To compile this driver as a module, choose M here: the
>> -	  module will be called ad7606_parallel.
>> +	  module will be called ad7606_par.
> If we can avoid a rename that would be good.  Or was this always wrong?
> If so spin a fix patch before this one.

It was always wrong, will fix it with a separate patch.
>
>>   
>>   config AD7606_IFACE_SPI
>>   	tristate "Analog Devices AD7606 ADC driver with spi interface support"
>> diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
>> index 99d5ca5c2348..a753d5caa9f8 100644
>> --- a/drivers/iio/adc/ad7606.c
>> +++ b/drivers/iio/adc/ad7606.c
>> @@ -21,6 +21,7 @@
>>   #include <linux/util_macros.h>
>>   #include <linux/units.h>
>>   
>> +#include <linux/iio/backend.h>
>>   #include <linux/iio/iio.h>
>>   #include <linux/iio/buffer.h>
>>   #include <linux/iio/sysfs.h>
>> @@ -148,7 +149,15 @@ static int ad7606_set_sampling_freq(struct ad7606_state *st, unsigned long freq)
>>   
>>   static int ad7606_read_samples(struct ad7606_state *st)
>>   {
>> -	unsigned int num = st->chip_info->num_channels - 1;
>> +	unsigned int num = st->chip_info->num_channels;
>> +
>> +	/*
>> +	 * Timestamp channel does not contain sample, and no timestamp channel if
>> +	 * backend is used.
>> +	 */
>> +	if (!st->back)
>> +		num--;
>> +
>>   	u16 *data = st->data;
>>   	int ret;
>>   
>> @@ -220,11 +229,15 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
>>   		if (!ret)
>>   			return ret;
>>   	}
>> -	ret = wait_for_completion_timeout(&st->completion,
>> -					  msecs_to_jiffies(1000));
>> -	if (!ret) {
>> -		ret = -ETIMEDOUT;
>> -		goto error_ret;
>> +
>> +	/* backend manages interruptions by itself.*/
>> +	if (!st->back) {
>> +		ret = wait_for_completion_timeout(&st->completion,
>> +						  msecs_to_jiffies(1000));
>> +		if (!ret) {
>> +			ret = -ETIMEDOUT;
>> +			goto error_ret;
>> +		}
>>   	}
>>   
>>   	ret = ad7606_read_samples(st);
>> @@ -271,6 +284,12 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
>>   	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>>   		*val = st->oversampling;
>>   		return IIO_VAL_INT;
>> +	case IIO_CHAN_INFO_SAMP_FREQ:
>> +		pwm_get_state_hw(st->cnvst_pwm, &cnvst_pwm_state);
>> +		/* If the PWM is swinging, return the real frequency, otherwise 0 */
> So this only exists for the pwm case. In that case can we split the channel definitions
> into versions with an without this and register just the right one.
>
> A sampling frequency of 0 usually means no sampling, not that we can tell what it
> is.  If we can't tell don't provide the file.

The file is provided only for the "backended" device 
(AD7606_BI_CHANNEL), BI being Backend Interface. This mode only works 
with PWM (and incidentally PWM is meant to be used only in conjuction 
with backend).

When the PWM is not running because e.g sampling is not enabled, or PWM 
failed to start, I return 0. Shall I always return the configured value 
instead of the real one ?

>
>
>> +		*val = ad7606_pwm_is_swinging(st) ?
>> +			DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, cnvst_pwm_state.period) : 0;
>> +		return IIO_VAL_INT;
>>   	}
>>   	return -EINVAL;
>>   }
>> @@ -360,6 +379,8 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
>>   			return ret;
>>   
>>   		return 0;
>> +	case IIO_CHAN_INFO_SAMP_FREQ:
>> +		return ad7606_set_sampling_freq(st, val);
>>   	default:
>>   		return -EINVAL;
>>   	}
>
>>   static const struct iio_trigger_ops ad7606_trigger_ops = {
>> @@ -602,8 +660,6 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
>>   	indio_dev->channels = st->chip_info->channels;
>>   	indio_dev->num_channels = st->chip_info->num_channels;
>>   
>> -	init_completion(&st->completion);
>> -
>>   	ret = ad7606_reset(st);
>>   	if (ret)
>>   		dev_warn(st->dev, "failed to RESET: no RESET GPIO specified\n");
>> @@ -635,7 +691,7 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
>>   			return ret;
>>   	}
>>   
>> -	/* If convst pin is not defined, setup PWM*/
>> +	/* If convst pin is not defined, setup PWM */
> Push into earlier patch.  Check for this sort of fix being in wrong patch
> before sending a series. It just adds noise to patch and to reviews.

Will do. Sorry for that.

>
>>   	if (!st->gpio_convst) {
>>   		st->cnvst_pwm = devm_pwm_get(dev, NULL);
>>   		if (IS_ERR(st->cnvst_pwm))
> ...
>
>> diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
>> index aab8fefb84be..9a098cd77812 100644
>> --- a/drivers/iio/adc/ad7606.h
>> +++ b/drivers/iio/adc/ad7606.h
>> @@ -34,6 +34,12 @@
>>   		BIT(IIO_CHAN_INFO_SCALE),		\
>>   		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO))
>>   
>> +#define AD7606_BI_CHANNEL(num)				\
>> +	AD760X_CHANNEL(num, 0,				\
>> +		BIT(IIO_CHAN_INFO_SCALE),		\
>> +		BIT(IIO_CHAN_INFO_SAMP_FREQ) |		\
>> +		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO))
>> +
>>   #define AD7616_CHANNEL(num)	\
>>   	AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),\
>>   		0, BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO))
>> @@ -61,6 +67,7 @@ enum ad7606_supported_device_ids {
>>    * @os_req_reset	some devices require a reset to update oversampling
>>    * @init_delay_ms	required delay in miliseconds for initialization
>>    *			after a restart
>> + * @has_backend		defines if a backend is available for the given chip
> That seems to me more of a case of does the driver support it.
> Linux kernel code has no way of knowing if a backend hardware exists
> or not.  Modify the comment to speak about if we know it works.
>
> Or is there something fundamental that stops the backend approach
> working with some devices?
>
> Why does the driver need this flag?

Potentially, I think any of those parts can have a backend and moreover, 
I don't see anything preventing any ADC to have a backend.

I introduced the flag as a way to differentiate the "new" way of 
supporting parallel interface, i.e using backend, from the "old" way 
(using port I/O).

There is a concurrency between the old implementation using port I/O and 
the new one using iio-backend, because they are both "platform", so the 
initial idea was that it would not make sense and be dangerous to look 
for a backend for the parts that have no existing (i'd rather say, like 
you pointed out,  supported) backend.

Having a second thought at it, the dt bindings already permits only 
io-backend property to be populated for the parts that actually have a 
backend, hence one of these is superfluous, or maybe even both are and 
the user is responsible for setting the right value in the dts. Any advice ?

>   
>
>>    */
>>   struct ad7606_chip_info {
>>   	enum ad7606_supported_device_ids id;
>> @@ -71,6 +78,7 @@ struct ad7606_chip_info {
>>   	unsigned int			oversampling_num;
>>   	bool				os_req_reset;
>>   	unsigned long			init_delay_ms;
>> +	bool				has_backend;
>>   };
>>   
>
>> diff --git a/drivers/iio/adc/ad7606_par.c b/drivers/iio/adc/ad7606_par.c
>> index d83c0edc1e31..5c8a04556e25 100644
>> --- a/drivers/iio/adc/ad7606_par.c
>> +++ b/drivers/iio/adc/ad7606_par.c
>> @@ -3,6 +3,8 @@
>>    * AD7606 Parallel Interface ADC driver
>>    *
>>    * Copyright 2011 Analog Devices Inc.
>> + * Copyright 2024 Analog Devices Inc.
>> + * Copyright 2024 BayLibre SAS.
>>    */
>>   
>>   #include <linux/mod_devicetable.h>
>> @@ -11,10 +13,86 @@
>>   #include <linux/types.h>
>>   #include <linux/err.h>
>>   #include <linux/io.h>
>> +#include <linux/pwm.h>
>> +#include <linux/gpio.h>
>> +#include <linux/delay.h>
> Not on any order currently but try to minimize a future series
> that might clean this up.  Preference is alphabetical though fine
> to have a trailing block of IIO headers, then driver specific
> ones after that. You can either fix the current order in a
> separate patch, or just put your new headers in approximately the write place.
np, will fix that in a separate patch.
>
>>   
>>   #include <linux/iio/iio.h>
>> +#include <linux/iio/backend.h>
>>   #include "ad7606.h"
>
>
>> +static int ad7606_bi_setup_iio_backend(struct device *dev, struct iio_dev *indio_dev)
>> +{
>> +		struct ad7606_state *st = iio_priv(indio_dev);
> Why 2 tabs?
by mistake.
>> +		unsigned int ret, c;
>> +
>> +		st->back = devm_iio_backend_get(dev, NULL);
>> +		if (IS_ERR(st->back))
>> +			return PTR_ERR(st->back);
>> +
>> +		/* If the device is iio_backend powered the PWM is mandatory */
>> +		if (!st->cnvst_pwm)
>> +			return -EINVAL;
>> +
>> +		ret = devm_iio_backend_request_buffer(dev, st->back, indio_dev);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = devm_iio_backend_enable(dev, st->back);
>> +		if (ret)
>> +			return ret;
>> +
>> +		struct iio_backend_data_fmt data = {
>> +			.sign_extend = true,
>> +			.enable = true,
>> +		};
>> +		for (c = 0; c < indio_dev->num_channels; c++) {
>> +			ret = iio_backend_data_format_set(st->back, c, &data);
>> +			if (ret)
>> +				return ret;
>> +		}
>> +
>> +		indio_dev->channels = ad7606b_bi_channels;
>> +		indio_dev->num_channels = 8;
>> +
>> +		return 0;
>> +}
>> +
>> +static const struct ad7606_bus_ops ad7606_bi_bops = {
>> +	.iio_backend_config = ad7606_bi_setup_iio_backend,
>> +	.update_scan_mode = ad7606_bi_update_scan_mode,
>> +};
>> +#endif
>> +
>>   static int ad7606_par16_read_block(struct device *dev,
>>   				   int count, void *buf)
>>   {
>> @@ -52,7 +130,20 @@ static int ad7606_par_probe(struct platform_device *pdev)
>>   	void __iomem *addr;
>>   	resource_size_t remap_size;
>>   	int irq;
>> -
>> +#ifdef CONFIG_IIO_BACKEND
>> +	struct iio_backend *back;
>> +
>> +	/*For now, only the AD7606B is backend compatible.*/
> /* For ... ble. */
again :-/
>
>> +	if (chip_info->has_backend) {
>> +		back = devm_iio_backend_get(&pdev->dev, NULL);
>> +		if (IS_ERR(back))
>> +			return PTR_ERR(back);
>> +
>> +		return ad7606_probe(&pdev->dev, 0, NULL,
>> +				    chip_info,
>> +				    &ad7606_bi_bops);
> Short wrap. Aim for 80 char limit unless it hurts readability a lot.
ok
>
>> +	}
>> +#endif
>>   	irq = platform_get_irq(pdev, 0);
>>   	if (irq < 0)
>>   		return irq;
>> @@ -74,6 +165,7 @@ static const struct platform_device_id ad7606_driver_ids[] = {
>>   	{ .name	= "ad7606-4", .driver_data = (kernel_ulong_t)&ad7606_4_info, },
>>   	{ .name	= "ad7606-6", .driver_data = (kernel_ulong_t)&ad7606_6_info, },
>>   	{ .name	= "ad7606-8", .driver_data = (kernel_ulong_t)&ad7606_8_info, },
>> +	{ .name	= "ad7606b", .driver_data = (kernel_ulong_t)&ad7606b_info, },
>>   	{ }
>>   };
>>   MODULE_DEVICE_TABLE(platform, ad7606_driver_ids);
>> @@ -83,6 +175,7 @@ static const struct of_device_id ad7606_of_match[] = {
>>   	{ .compatible = "adi,ad7606-4", .data = &ad7606_4_info },
>>   	{ .compatible = "adi,ad7606-6", .data = &ad7606_6_info },
>>   	{ .compatible = "adi,ad7606-8", .data = &ad7606_8_info },
>> +	{ .compatible = "adi,ad7606b", .data = &ad7606b_info },
>>   	{ }
>>   };
>>   MODULE_DEVICE_TABLE(of, ad7606_of_match);
>> @@ -102,3 +195,6 @@ MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
>>   MODULE_DESCRIPTION("Analog Devices AD7606 ADC");
>>   MODULE_LICENSE("GPL v2");
>>   MODULE_IMPORT_NS(IIO_AD7606);
>> +#ifdef CONFIG_IIO_BACKEND
>> +MODULE_IMPORT_NS(IIO_BACKEND);
> I'd not bother with config guards.  Importing a namespace we don't
> use should be harmless.
OK, will remove it. According to Nuno's feedback, I could also force the 
selection of CONFIG_IIO_BACKEND with the driver, which IMHO is not a bad 
idea, as it would allow to remove all those ifdefs.
>
>> +#endif
>>

Re: [PATCH 8/8] iio:adc:ad7606: Add iio-backend support

[Guillaume Stols]

On 9/5/24 10:40, Nuno Sá wrote:
> On Thu, 2024-08-15 at 12:12 +0000, Guillaume Stols wrote:
>> - Basic support for iio backend.
>> - Supports IIO_CHAN_INFO_SAMP_FREQ R/W.
>> - Only hardware mode is available, and that IIO_CHAN_INFO_RAW is not
>>    supported if iio-backend mode is selected.
>>
>> A small correction was added to the driver's file name in the Kconfig
>> file's description.
>>
>> Signed-off-by: Guillaume Stols <gstols@baylibre.com>
>> ---
> Hi Guillaume,
>
> Some initial feedback from me...
>
>>   drivers/iio/adc/Kconfig      |   3 +-
>>   drivers/iio/adc/ad7606.c     | 103 +++++++++++++++++++++++++++++++++++-------
>> -
>>   drivers/iio/adc/ad7606.h     |  16 +++++++
>>   drivers/iio/adc/ad7606_par.c |  98 +++++++++++++++++++++++++++++++++++++++-
>>   4 files changed, 200 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 88e8ce2e78b3..01248b6df868 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -227,9 +227,10 @@ config AD7606_IFACE_PARALLEL
>>   	help
>>   	  Say yes here to build parallel interface support for Analog
>> Devices:
>>   	  ad7605-4, ad7606, ad7606-6, ad7606-4 analog to digital converters
>> (ADC).
>> +	  It also support iio_backended devices for AD7606B.
>>   
>>   	  To compile this driver as a module, choose M here: the
>> -	  module will be called ad7606_parallel.
>> +	  module will be called ad7606_par.
>>   
>>   config AD7606_IFACE_SPI
>>   	tristate "Analog Devices AD7606 ADC driver with spi interface
>> support"
>> diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
>> index 99d5ca5c2348..a753d5caa9f8 100644
>> --- a/drivers/iio/adc/ad7606.c
>> +++ b/drivers/iio/adc/ad7606.c
>> @@ -21,6 +21,7 @@
>>   #include <linux/util_macros.h>
>>   #include <linux/units.h>
>> +
>> +	/* backend manages interruptions by itself.*/
> missing space before closing the comment (also not sure the comments adds much)


thx, will check again


>
>> +	if (!st->back) {
>> +		ret = wait_for_completion_timeout(&st->completion,
>> +						  msecs_to_jiffies(1000));
>> +		if (!ret) {
>> +			ret = -ETIMEDOUT;
>> +			goto error_ret;
>> +		}
>>   	}
>>   
>>   	ret = ad7606_read_samples(st);
>> @@ -271,6 +284,12 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
>>   	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>>   		*val = st->oversampling;
>>   		return IIO_VAL_INT;
>> +	case IIO_CHAN_INFO_SAMP_FREQ:
>> +		pwm_get_state_hw(st->cnvst_pwm, &cnvst_pwm_state);
>> +		/* If the PWM is swinging, return the real frequency,
>> otherwise 0 */
>> +		*val = ad7606_pwm_is_swinging(st) ?
>> +			DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC,
>> cnvst_pwm_state.period) : 0;
>> +		return IIO_VAL_INT;
>>   	}
>>   	return -EINVAL;
>>   }
>> @@ -360,6 +379,8 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
>>   			return ret;
>>   
>>   		return 0;
>> +	case IIO_CHAN_INFO_SAMP_FREQ:
>> +		return ad7606_set_sampling_freq(st, val);
>>   	default:
>>   		return -EINVAL;
>>   	}
>> @@ -482,7 +503,6 @@ static int ad7606_buffer_postenable(struct iio_dev
>> *indio_dev)
>>   	struct ad7606_state *st = iio_priv(indio_dev);
>>   
>>   	gpiod_set_value(st->gpio_convst, 1);
>> -	ad7606_pwm_set_swing(st);
>>   
>>   	return 0;
>>   }
>> @@ -492,19 +512,53 @@ static int ad7606_buffer_predisable(struct iio_dev
>> *indio_dev)
>>   	struct ad7606_state *st = iio_priv(indio_dev);
>>   
>>   	gpiod_set_value(st->gpio_convst, 0);
>> -	ad7606_pwm_set_low(st);
>>   
>>   	return 0;
>>   }
>>   
>> +static int ad7606_pwm_buffer_postenable(struct iio_dev *indio_dev)
>> +{
>> +	struct ad7606_state *st = iio_priv(indio_dev);
>> +
>> +	return ad7606_pwm_set_swing(st);
>> +}
>> +
>> +static int ad7606_pwm_buffer_predisable(struct iio_dev *indio_dev)
>> +{
>> +	struct ad7606_state *st = iio_priv(indio_dev);
>> +
>> +	return ad7606_pwm_set_low(st);
>> +}
> Maybe I'm missing something but are we removing the gpiod calls?


Well actually the pwm is meant to be used only with backend. Though it 
could be used without it, I dont think it is a very good idea because 
interrupt handling + transmission init takes quite some time, and a new 
rising edge could happen before the current samples are effectively 
transferred. However, since PWM and backend are two separate things, I 
wanted to show an usage for the PWM when introducing it, and one way to 
do it was to use it to emulate a GPIO by setting the duty cycle 100% for 
having a 1 (set_high) and 0% for having a 0 (set_low). Then on this 
patch, since we introduce iio-backend, I removed this 'mock' usage of it.

But I think that I should separate the removal into an additional patch 
to avoid confusions. Or shall I just remove the mock usage from the PWM 
patch ?


>> +
>> +static int ad7606_update_scan_mode(struct iio_dev *indio_dev,
>> +				   const unsigned long *scan_mask)
>> +{
>> +	struct ad7606_state *st = iio_priv(indio_dev);
>> +
>> +	/* The update scan mode is only for iio backend compatible drivers.
>> +	 * If the specific update_scan_mode is not defined in the bus ops,
>> +	 * just do nothing and return 0.
>> +	 */
>> +	if (st->bops->update_scan_mode)
>> +		return st->bops->update_scan_mode(indio_dev, scan_mask);
>> +	else
>> +		return 0;
> Redundant else


ack

>> -	if (ret)
>> -		return ret;
>>   
>> +	if (st->bops->iio_backend_config) {
>> +		st->bops->iio_backend_config(dev, indio_dev);
>> +		indio_dev->setup_ops = &ad7606_pwm_buffer_ops;
> Ignoring error code


will handle


>
>> +	} else {
>> +		/* Reserve the PWM use only for backend (force gpio_convst
>> definition)*/
>> +		if (!st->gpio_convst)
>> +			return dev_err_probe(dev, -EINVAL,
>> +					     "Convst pin must be defined when
>> not in backend mode");
>> +
>> +		init_completion(&st->completion);
>> +		ret = devm_request_threaded_irq(dev, irq,
>> +						NULL,
>> +						&ad7606_interrupt,
>> +						IRQF_TRIGGER_FALLING |
>> IRQF_ONESHOT,
>> +						chip_info->name, indio_dev);
>> +		if (ret)
>> +			return ret;
>> +	}
> Are we still calling devm_iio_triggered_buffer_setup() in case we have a backend
> device?


No, this portion of code is only executed if convst is defined 
(conversion trigger GPIO), which is not the case if there is a backend.


>
>>   	return devm_iio_device_register(dev, indio_dev);
>>   }
> ...
>
>> +#ifdef CONFIG_IIO_BACKEND
> Not a fan of this #ifef... It's not that much code so I would just select
> IIO_BACKEND for this driver. In fact, I don't think we can separately enable
> IIO_BACKEND in the menuconfig menu?


OK I can do it that way.

>> +static int ad7606_bi_setup_iio_backend(struct device *dev, struct iio_dev
>> *indio_dev)
>> +{
>> +		struct ad7606_state *st = iio_priv(indio_dev);
>> +		unsigned int ret, c;
>> +
>> +		st->back = devm_iio_backend_get(dev, NULL);
>> +		if (IS_ERR(st->back))
>> +			return PTR_ERR(st->back);
>> +
>> +		/* If the device is iio_backend powered the PWM is mandatory
>> */
>> +		if (!st->cnvst_pwm)
>> +			return -EINVAL;
>> +
>> +		ret = devm_iio_backend_request_buffer(dev, st->back,
>> indio_dev);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = devm_iio_backend_enable(dev, st->back);
>> +		if (ret)
>> +			return ret;
>> +
>> +		struct iio_backend_data_fmt data = {
>> +			.sign_extend = true,
>> +			.enable = true,
>> +		};
> I would follow typical kernel coding style and have this declared at the
> beginning of the function.


aouch, yes !


>> -
>> +#ifdef CONFIG_IIO_BACKEND
>> +	struct iio_backend *back;
>> +
>> +	/*For now, only the AD7606B is backend compatible.*/
>> +	if (chip_info->has_backend) {
>> +		back = devm_iio_backend_get(&pdev->dev, NULL);
>> +		if (IS_ERR(back))
>> +			return PTR_ERR(back);
>> +
>> +		return ad7606_probe(&pdev->dev, 0, NULL,
>> +				    chip_info,
>> +				    &ad7606_bi_bops);
>> +	}
>> +#endif
> Not sure I follow the above? You also get the backend in
> ad7606_bi_setup_iio_backend()? So it seems to be that the has_backend flag is
> not really needed?


The first call to devm_iio_backend_get checks if there is a backend 
available, and if so the backend bops (ad7606_bi_bops)are passed to the 
generic probe function.

At this stage, the backend cannot be stored in the ad7606_state 
structure because it is not initialized yet, it will be in the generic 
probe function, hence the second call.

The has_backend flag is discussed in my answer to Jonathan's comment, 
and will probably be removed.

>
> - Nuno Sá
>
>

Re: [PATCH 8/8] iio:adc:ad7606: Add iio-backend support

[Nuno Sá]

On Thu, 2024-09-12 at 12:13 +0200, Guillaume Stols wrote:
> On 9/5/24 10:40, Nuno Sá wrote:
> > On Thu, 2024-08-15 at 12:12 +0000, Guillaume Stols wrote:
> > > - Basic support for iio backend.
> > > - Supports IIO_CHAN_INFO_SAMP_FREQ R/W.
> > > - Only hardware mode is available, and that IIO_CHAN_INFO_RAW is not
> > >    supported if iio-backend mode is selected.
> > > 
> > > A small correction was added to the driver's file name in the Kconfig
> > > file's description.
> > > 
> > > Signed-off-by: Guillaume Stols <gstols@baylibre.com>
> > > ---
> > Hi Guillaume,
> > 
> > Some initial feedback from me...
> > 
> > >   drivers/iio/adc/Kconfig      |   3 +-
> > >   drivers/iio/adc/ad7606.c     | 103 +++++++++++++++++++++++++++++++++++-------
> > > -
> > >   drivers/iio/adc/ad7606.h     |  16 +++++++
> > >   drivers/iio/adc/ad7606_par.c |  98 +++++++++++++++++++++++++++++++++++++++-
> > >   4 files changed, 200 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > > index 88e8ce2e78b3..01248b6df868 100644
> > > --- a/drivers/iio/adc/Kconfig
> > > +++ b/drivers/iio/adc/Kconfig
> > > @@ -227,9 +227,10 @@ config AD7606_IFACE_PARALLEL
> > >   	help
> > >   	  Say yes here to build parallel interface support for Analog
> > > Devices:
> > >   	  ad7605-4, ad7606, ad7606-6, ad7606-4 analog to digital converters
> > > (ADC).
> > > +	  It also support iio_backended devices for AD7606B.
> > >   
> > >   	  To compile this driver as a module, choose M here: the
> > > -	  module will be called ad7606_parallel.
> > > +	  module will be called ad7606_par.
> > >   
> > >   config AD7606_IFACE_SPI
> > >   	tristate "Analog Devices AD7606 ADC driver with spi interface
> > > support"
> > > diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
> > > index 99d5ca5c2348..a753d5caa9f8 100644
> > > --- a/drivers/iio/adc/ad7606.c
> > > +++ b/drivers/iio/adc/ad7606.c
> > > @@ -21,6 +21,7 @@
> > >   #include <linux/util_macros.h>
> > >   #include <linux/units.h>
> > > +
> > > +	/* backend manages interruptions by itself.*/
> > missing space before closing the comment (also not sure the comments adds much)
> 
> 
> thx, will check again
> 
> 
> > 
> > > +	if (!st->back) {
> > > +		ret = wait_for_completion_timeout(&st->completion,
> > > +						  msecs_to_jiffies(1000));
> > > +		if (!ret) {
> > > +			ret = -ETIMEDOUT;
> > > +			goto error_ret;
> > > +		}
> > >   	}
> > >   
> > >   	ret = ad7606_read_samples(st);
> > > @@ -271,6 +284,12 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
> > >   	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> > >   		*val = st->oversampling;
> > >   		return IIO_VAL_INT;
> > > +	case IIO_CHAN_INFO_SAMP_FREQ:
> > > +		pwm_get_state_hw(st->cnvst_pwm, &cnvst_pwm_state);
> > > +		/* If the PWM is swinging, return the real frequency,
> > > otherwise 0 */
> > > +		*val = ad7606_pwm_is_swinging(st) ?
> > > +			DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC,
> > > cnvst_pwm_state.period) : 0;
> > > +		return IIO_VAL_INT;
> > >   	}
> > >   	return -EINVAL;
> > >   }
> > > @@ -360,6 +379,8 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
> > >   			return ret;
> > >   
> > >   		return 0;
> > > +	case IIO_CHAN_INFO_SAMP_FREQ:
> > > +		return ad7606_set_sampling_freq(st, val);
> > >   	default:
> > >   		return -EINVAL;
> > >   	}
> > > @@ -482,7 +503,6 @@ static int ad7606_buffer_postenable(struct iio_dev
> > > *indio_dev)
> > >   	struct ad7606_state *st = iio_priv(indio_dev);
> > >   
> > >   	gpiod_set_value(st->gpio_convst, 1);
> > > -	ad7606_pwm_set_swing(st);
> > >   
> > >   	return 0;
> > >   }
> > > @@ -492,19 +512,53 @@ static int ad7606_buffer_predisable(struct iio_dev
> > > *indio_dev)
> > >   	struct ad7606_state *st = iio_priv(indio_dev);
> > >   
> > >   	gpiod_set_value(st->gpio_convst, 0);
> > > -	ad7606_pwm_set_low(st);
> > >   
> > >   	return 0;
> > >   }
> > >   
> > > +static int ad7606_pwm_buffer_postenable(struct iio_dev *indio_dev)
> > > +{
> > > +	struct ad7606_state *st = iio_priv(indio_dev);
> > > +
> > > +	return ad7606_pwm_set_swing(st);
> > > +}
> > > +
> > > +static int ad7606_pwm_buffer_predisable(struct iio_dev *indio_dev)
> > > +{
> > > +	struct ad7606_state *st = iio_priv(indio_dev);
> > > +
> > > +	return ad7606_pwm_set_low(st);
> > > +}
> > Maybe I'm missing something but are we removing the gpiod calls?
> 
> 
> Well actually the pwm is meant to be used only with backend. Though it 
> could be used without it, I dont think it is a very good idea because 
> interrupt handling + transmission init takes quite some time, and a new 
> rising edge could happen before the current samples are effectively 
> transferred. However, since PWM and backend are two separate things, I 
> wanted to show an usage for the PWM when introducing it, and one way to 
> do it was to use it to emulate a GPIO by setting the duty cycle 100% for 
> having a 1 (set_high) and 0% for having a 0 (set_low). Then on this 
> patch, since we introduce iio-backend, I removed this 'mock' usage of it.
> 
> But I think that I should separate the removal into an additional patch 
> to avoid confusions. Or shall I just remove the mock usage from the PWM 
> patch ?
> 
> 

Yeah, probably better (with a proper commit message explaining the reasoning)

> > > +
> > > +static int ad7606_update_scan_mode(struct iio_dev *indio_dev,
> > > +				   const unsigned long *scan_mask)
> > > +{
> > > +	struct ad7606_state *st = iio_priv(indio_dev);
> > > +
> > > +	/* The update scan mode is only for iio backend compatible drivers.
> > > +	 * If the specific update_scan_mode is not defined in the bus ops,
> > > +	 * just do nothing and return 0.
> > > +	 */
> > > +	if (st->bops->update_scan_mode)
> > > +		return st->bops->update_scan_mode(indio_dev, scan_mask);
> > > +	else
> > > +		return 0;
> > Redundant else
> 
> 
> ack
> 
> > > -	if (ret)
> > > -		return ret;
> > >   
> > > +	if (st->bops->iio_backend_config) {
> > > +		st->bops->iio_backend_config(dev, indio_dev);
> > > +		indio_dev->setup_ops = &ad7606_pwm_buffer_ops;
> > Ignoring error code
> 
> 
> will handle
> 
> 
> > 
> > > +	} else {
> > > +		/* Reserve the PWM use only for backend (force gpio_convst
> > > definition)*/
> > > +		if (!st->gpio_convst)
> > > +			return dev_err_probe(dev, -EINVAL,
> > > +					     "Convst pin must be defined when
> > > not in backend mode");
> > > +
> > > +		init_completion(&st->completion);
> > > +		ret = devm_request_threaded_irq(dev, irq,
> > > +						NULL,
> > > +						&ad7606_interrupt,
> > > +						IRQF_TRIGGER_FALLING |
> > > IRQF_ONESHOT,
> > > +						chip_info->name, indio_dev);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > Are we still calling devm_iio_triggered_buffer_setup() in case we have a backend
> > device?
> 
> 
> No, this portion of code is only executed if convst is defined 
> (conversion trigger GPIO), which is not the case if there is a backend.
> 
> 
> > 
> > >   	return devm_iio_device_register(dev, indio_dev);
> > >   }
> > ...
> > 
> > > +#ifdef CONFIG_IIO_BACKEND
> > Not a fan of this #ifef... It's not that much code so I would just select
> > IIO_BACKEND for this driver. In fact, I don't think we can separately enable
> > IIO_BACKEND in the menuconfig menu?
> 
> 
> OK I can do it that way.
> 
> > > +static int ad7606_bi_setup_iio_backend(struct device *dev, struct iio_dev
> > > *indio_dev)
> > > +{
> > > +		struct ad7606_state *st = iio_priv(indio_dev);
> > > +		unsigned int ret, c;
> > > +
> > > +		st->back = devm_iio_backend_get(dev, NULL);
> > > +		if (IS_ERR(st->back))
> > > +			return PTR_ERR(st->back);
> > > +
> > > +		/* If the device is iio_backend powered the PWM is mandatory
> > > */
> > > +		if (!st->cnvst_pwm)
> > > +			return -EINVAL;
> > > +
> > > +		ret = devm_iio_backend_request_buffer(dev, st->back,
> > > indio_dev);
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		ret = devm_iio_backend_enable(dev, st->back);
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		struct iio_backend_data_fmt data = {
> > > +			.sign_extend = true,
> > > +			.enable = true,
> > > +		};
> > I would follow typical kernel coding style and have this declared at the
> > beginning of the function.
> 
> 
> aouch, yes !
> 
> 
> > > -
> > > +#ifdef CONFIG_IIO_BACKEND
> > > +	struct iio_backend *back;
> > > +
> > > +	/*For now, only the AD7606B is backend compatible.*/
> > > +	if (chip_info->has_backend) {
> > > +		back = devm_iio_backend_get(&pdev->dev, NULL);
> > > +		if (IS_ERR(back))
> > > +			return PTR_ERR(back);
> > > +
> > > +		return ad7606_probe(&pdev->dev, 0, NULL,
> > > +				    chip_info,
> > > +				    &ad7606_bi_bops);
> > > +	}
> > > +#endif
> > Not sure I follow the above? You also get the backend in
> > ad7606_bi_setup_iio_backend()? So it seems to be that the has_backend flag is
> > not really needed?
> 
> 
> The first call to devm_iio_backend_get checks if there is a backend 
> available, and if so the backend bops (ad7606_bi_bops)are passed to the 
> generic probe function.
> 

Why not checking for the presence of the DT property? We should only get the backend
when ready for that.

- Nuno Sá
> 

Re: [PATCH 7/8] iio: adc: ad7606: Switch to xxx_get_device_match_data

[Guillaume Stols]

On 8/17/24 17:33, Jonathan Cameron wrote:
> On Thu, 15 Aug 2024 12:12:01 +0000
> Guillaume Stols <gstols@baylibre.com> wrote:
>
>> On the parallel version, the current implementation is only compatible
>> with id tables and won't work with fx_nodes. So in this commit, the goal
>> is to switch to use get_device_match_data, in order to simplify the
>> logic of retrieving chip data.
>>
>> Also, chip info is moved in the .h file so to be accessible to all the
>> driver files that can set a pointer to the corresponding chip as the
>> driver data.
> This means each driver gets their own copy.
>
> Better to use an extern in the header and keep the actual data
> in the core module.

ack.

Given your previous comment about introducing 
platform_device_get_match_data, I guess I should instead do it directly 
in the driver's probe, like its done in axp20x_adc.c ? Somehting like that:

if (!dev_fwnode(&pdev->dev)) {
     const struct platform_device_id *id;

     id = platform_get_device_id(pdev);
     chip_info = (const struct ad7606_chip_info *)id->driver_data;
} else {
     struct device *dev = &pdev->dev;
     chip_info = device_get_match_data(dev);
}

Re: [PATCH 7/8] iio: adc: ad7606: Switch to xxx_get_device_match_data

[Jonathan Cameron]

On Sat, 14 Sep 2024 11:21:34 +0200
Guillaume Stols <gstols@baylibre.com> wrote:

> On 8/17/24 17:33, Jonathan Cameron wrote:
> > On Thu, 15 Aug 2024 12:12:01 +0000
> > Guillaume Stols <gstols@baylibre.com> wrote:
> >  
> >> On the parallel version, the current implementation is only compatible
> >> with id tables and won't work with fx_nodes. So in this commit, the goal
> >> is to switch to use get_device_match_data, in order to simplify the
> >> logic of retrieving chip data.
> >>
> >> Also, chip info is moved in the .h file so to be accessible to all the
> >> driver files that can set a pointer to the corresponding chip as the
> >> driver data.  
> > This means each driver gets their own copy.
> >
> > Better to use an extern in the header and keep the actual data
> > in the core module.  
> 
> ack.
> 
> Given your previous comment about introducing 
> platform_device_get_match_data, I guess I should instead do it directly 
> in the driver's probe, like its done in axp20x_adc.c ? Somehting like that:
> 
> if (!dev_fwnode(&pdev->dev)) {
>      const struct platform_device_id *id;
> 
>      id = platform_get_device_id(pdev);
>      chip_info = (const struct ad7606_chip_info *)id->driver_data;
> } else {
>      struct device *dev = &pdev->dev;
>      chip_info = device_get_match_data(dev);
> }

Yes, something along those lines makes sense.

If there are enough instances of this we can have a standard
definition for this similar to the i2c / spi ones that defaults
to device_get_match_data() if available, and falls back to the old
way if not.

If you want to add that great, if not it can be a separate
bit of work for another day.

Jonathan

Re: [PATCH 8/8] iio:adc:ad7606: Add iio-backend support

[Jonathan Cameron]

...

> >>   
> >>   	ret = ad7606_read_samples(st);
> >> @@ -271,6 +284,12 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
> >>   	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> >>   		*val = st->oversampling;
> >>   		return IIO_VAL_INT;
> >> +	case IIO_CHAN_INFO_SAMP_FREQ:
> >> +		pwm_get_state_hw(st->cnvst_pwm, &cnvst_pwm_state);
> >> +		/* If the PWM is swinging, return the real frequency, otherwise 0 */  
> > So this only exists for the pwm case. In that case can we split the channel definitions
> > into versions with an without this and register just the right one.
> >
> > A sampling frequency of 0 usually means no sampling, not that we can tell what it
> > is.  If we can't tell don't provide the file.  
> 
> The file is provided only for the "backended" device 
> (AD7606_BI_CHANNEL), BI being Backend Interface. This mode only works 
> with PWM (and incidentally PWM is meant to be used only in conjuction 
> with backend).
> 
> When the PWM is not running because e.g sampling is not enabled, or PWM 
> failed to start, I return 0. Shall I always return the configured value 
> instead of the real one ?

Yes. Configured should be fine I think if there is no way to ask
'what will it be when I turn it on'.


> >> diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
> >> index aab8fefb84be..9a098cd77812 100644
> >> --- a/drivers/iio/adc/ad7606.h
> >> +++ b/drivers/iio/adc/ad7606.h
> >> @@ -34,6 +34,12 @@
> >>   		BIT(IIO_CHAN_INFO_SCALE),		\
> >>   		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO))
> >>   
> >> +#define AD7606_BI_CHANNEL(num)				\
> >> +	AD760X_CHANNEL(num, 0,				\
> >> +		BIT(IIO_CHAN_INFO_SCALE),		\
> >> +		BIT(IIO_CHAN_INFO_SAMP_FREQ) |		\
> >> +		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO))
> >> +
> >>   #define AD7616_CHANNEL(num)	\
> >>   	AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),\
> >>   		0, BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO))
> >> @@ -61,6 +67,7 @@ enum ad7606_supported_device_ids {
> >>    * @os_req_reset	some devices require a reset to update oversampling
> >>    * @init_delay_ms	required delay in miliseconds for initialization
> >>    *			after a restart
> >> + * @has_backend		defines if a backend is available for the given chip  
> > That seems to me more of a case of does the driver support it.
> > Linux kernel code has no way of knowing if a backend hardware exists
> > or not.  Modify the comment to speak about if we know it works.
> >
> > Or is there something fundamental that stops the backend approach
> > working with some devices?
> >
> > Why does the driver need this flag?  
> 
> Potentially, I think any of those parts can have a backend and moreover, 
> I don't see anything preventing any ADC to have a backend.
> 
> I introduced the flag as a way to differentiate the "new" way of 
> supporting parallel interface, i.e using backend, from the "old" way 
> (using port I/O).
> 
> There is a concurrency between the old implementation using port I/O and 
> the new one using iio-backend, because they are both "platform", so the 
> initial idea was that it would not make sense and be dangerous to look 
> for a backend for the parts that have no existing (i'd rather say, like 
> you pointed out,  supported) backend.
> 
> Having a second thought at it, the dt bindings already permits only 
> io-backend property to be populated for the parts that actually have a 
> backend, hence one of these is superfluous, or maybe even both are and 
> the user is responsible for setting the right value in the dts. Any advice ?

Dt binding should be enough.  The worst that happens is the driver
tries to use an unsupported backend and that will fail I hope.

So I wouldn't have this driver try to stop it.

> >> diff --git a/drivers/iio/adc/ad7606_par.c b/drivers/iio/adc/ad7606_par.c
> >> index d83c0edc1e31..5c8a04556e25 100644
> >> --- a/drivers/iio/adc/ad7606_par.c
> >> +++ b/drivers/iio/adc/ad7606_par.c
> >> @@ -102,3 +195,6 @@ MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
> >>   MODULE_DESCRIPTION("Analog Devices AD7606 ADC");
> >>   MODULE_LICENSE("GPL v2");
> >>   MODULE_IMPORT_NS(IIO_AD7606);
> >> +#ifdef CONFIG_IIO_BACKEND
> >> +MODULE_IMPORT_NS(IIO_BACKEND);  
> > I'd not bother with config guards.  Importing a namespace we don't
> > use should be harmless.  
> OK, will remove it. According to Nuno's feedback, I could also force the 
> selection of CONFIG_IIO_BACKEND with the driver, which IMHO is not a bad 
> idea, as it would allow to remove all those ifdefs.

Hmm. I guess the questions is whether that is a bloat anyone will worry about
who is using the old way for this device.  I guess that's a problem for
Analog folk if their customers complain.  We can always relax this in future
so for now select IIO_BACKEND is fine by me.

Jonathan