[PATCH v2 00/10] Add iio backend compatibility for ad7606

[PATCH v2 00/10] 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>
---
Changes in v2:
- Logical change in dt-bindings, using a flag for the interface instead of
  infering it from the value of the "reg" property.
- Removal of get_platform_match_data addition, instead the logic is
  directly used in the file.
- Removal of use and export of pwm_get_state_hw, returning the configured
  frequency instead of the running one.
- Correction on various typos, whitespaces, bad order of includes.
- Separation of SPI conditions and PWM disabling for no backend in other
  commits.
- Link to v1: https://lore.kernel.org/r/20240815-ad7606_add_iio_backend_support-v1-0-cea3e11b1aa4@baylibre.com

---
Guillaume Stols (10):
      dt-bindings: iio: adc: ad7606: Set the correct polarity
      dt-bindings: iio: adc: ad7606: Make corrections on spi conditions
      dt-bindings: iio: adc: ad7606: Add iio backend bindings
      Documentation: iio: Document ad7606 driver
      iio: adc: ad7606: Sort includes in alphabetical order
      iio: adc: ad7606: Add PWM support for conversion trigger
      iio: adc: ad7606: Add compatibility to fw_nodes
      iio: adc: ad7606: Fix typo in the driver name
      iio: adc: ad7606: Add iio-backend support
      iio: adc: ad7606: Disable PWM usage for non backend version

 .../devicetree/bindings/iio/adc/adi,ad7606.yaml    |  97 ++++-
 Documentation/iio/ad7606.rst                       | 143 +++++++
 drivers/iio/adc/Kconfig                            |   4 +-
 drivers/iio/adc/ad7606.c                           | 474 +++++++++++++++------
 drivers/iio/adc/ad7606.h                           |  51 ++-
 drivers/iio/adc/ad7606_par.c                       | 126 +++++-
 drivers/iio/adc/ad7606_spi.c                       |  33 +-
 7 files changed, 749 insertions(+), 179 deletions(-)
---
base-commit: 8bea3878a1511bceadc2fbf284b00bcc5a2ef28d
change-id: 20240725-ad7606_add_iio_backend_support-c401305a6924

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

[PATCH v2 01/10] dt-bindings: iio: adc: ad7606: Set the correct polarity

[Guillaume Stols]

According to the datasheet, "Data is clocked in from SDI on the falling
edge of SCLK, while data is clocked out on DOUTA on the rising edge of
SCLK".
Also, even if not stated textually in the datasheet, it is made clear on
the diagrams that sclk idles at high.

So the documentation is erroneously stating that spi-cpha is required, and
the example is erroneously setting both spi-cpol and spi-cpha.

Fixes: 416f882c3b40 ("dt-bindings: iio: adc: Migrate AD7606 documentation to yaml")
Fixes: 6e33a125df66 ("dt-bindings: iio: adc: Add docs for AD7606 ADC")

Signed-off-by: Guillaume Stols <gstols@baylibre.com>
---
 Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
index 69408cae3db9..75334a033539 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
@@ -29,8 +29,6 @@ properties:
   reg:
     maxItems: 1
 
-  spi-cpha: true
-
   spi-cpol: true
 
   avcc-supply: true
@@ -117,7 +115,7 @@ properties:
 required:
   - compatible
   - reg
-  - spi-cpha
+  - spi-cpol
   - avcc-supply
   - vdrive-supply
   - interrupts
@@ -185,7 +183,6 @@ examples:
             reg = <0>;
             spi-max-frequency = <1000000>;
             spi-cpol;
-            spi-cpha;
 
             avcc-supply = <&adc_vref>;
             vdrive-supply = <&vdd_supply>;

-- 
2.34.1

[PATCH v2 02/10] 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      | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
index 75334a033539..12995ebcddc2 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
@@ -112,18 +112,32 @@ properties:
       assumed that the pins are hardwired to VDD.
     type: boolean
 
+  parallel-interface:
+    description:
+      If the parallel interface is used, be it directly or through a backend,
+      this property must be defined.
+    type: boolean
+
 required:
   - compatible
   - reg
-  - spi-cpol
   - avcc-supply
   - vdrive-supply
   - interrupts
   - adi,conversion-start-gpios
 
-allOf:
-  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+oneOf:
+  - required:
+      - parallel-interface
+  - allOf:
+      - properties:
+          parallel-interface: false
+          spi-cpol: true
+      - $ref: /schemas/spi/spi-peripheral-props.yaml#
+      - required:
+          - spi-cpol
 
+allOf:
   - if:
       properties:
         compatible:

-- 
2.34.1

[PATCH v2 03/10] 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 transfer) 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    | 76 +++++++++++++++++++++-
 1 file changed, 74 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
index 12995ebcddc2..74a8680904b1 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
@@ -118,13 +118,32 @@ properties:
       this property must be defined.
     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:
+    description:
+      The name of each PWM, the first is connected to CONVST, and the second is
+      connected to CONVST2 if CONVST2 is available and not connected to CONVST1.
+    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
 
 oneOf:
   - required:
@@ -138,6 +157,34 @@ oneOf:
           - spi-cpol
 
 allOf:
+  - 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:
@@ -179,12 +226,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";
+            parallel-interface;
+            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 v2 04/10] 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 | 143 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 143 insertions(+)

diff --git a/Documentation/iio/ad7606.rst b/Documentation/iio/ad7606.rst
new file mode 100644
index 000000000000..270a49aae685
--- /dev/null
+++ b/Documentation/iio/ad7606.rst
@@ -0,0 +1,143 @@
+.. 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 through 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 chips that support software mode have more values available 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 sets 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, otherwise 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 v2 05/10] iio: adc: ad7606: Sort includes in alphabetical order

[Guillaume Stols]

Some of the includes were not in alphabetical order, this commit fixes
it.

Signed-off-by: Guillaume Stols <gstols@baylibre.com>
---
 drivers/iio/adc/ad7606_par.c | 6 +++---
 drivers/iio/adc/ad7606_spi.c | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/ad7606_par.c b/drivers/iio/adc/ad7606_par.c
index 02d8c309304e..d651639c45eb 100644
--- a/drivers/iio/adc/ad7606_par.c
+++ b/drivers/iio/adc/ad7606_par.c
@@ -5,13 +5,13 @@
  * Copyright 2011 Analog Devices Inc.
  */
 
+#include <linux/err.h>
+#include <linux/gpio/consumer.h>
+#include <linux/io.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
-#include <linux/gpio/consumer.h>
 #include <linux/platform_device.h>
 #include <linux/types.h>
-#include <linux/err.h>
-#include <linux/io.h>
 
 #include <linux/iio/iio.h>
 #include "ad7606.h"
diff --git a/drivers/iio/adc/ad7606_spi.c b/drivers/iio/adc/ad7606_spi.c
index 62ec12195307..d3fbc7c7a823 100644
--- a/drivers/iio/adc/ad7606_spi.c
+++ b/drivers/iio/adc/ad7606_spi.c
@@ -5,10 +5,10 @@
  * Copyright 2011 Analog Devices Inc.
  */
 
+#include <linux/err.h>
 #include <linux/module.h>
 #include <linux/spi/spi.h>
 #include <linux/types.h>
-#include <linux/err.h>
 
 #include <linux/iio/iio.h>
 #include "ad7606.h"

-- 
2.34.1

[PATCH v2 06/10] 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 | 170 ++++++++++++++++++++++++++++++++++++++++-------
 drivers/iio/adc/ad7606.h |   2 +
 2 files changed, 148 insertions(+), 24 deletions(-)

diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index 9b457472d49c..b98057138295 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>
@@ -83,6 +85,80 @@ 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;
@@ -117,9 +193,22 @@ 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 set it back. */
+		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) {
@@ -130,7 +219,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);
 
@@ -395,8 +489,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);
 
@@ -465,6 +560,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;
 }
@@ -474,6 +570,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;
 }
@@ -521,6 +618,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)
@@ -611,20 +713,47 @@ 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);
+		/*
+		 * PWM is not disabled when sampling stops, but instead its duty cycle is set
+		 * to 0% to be sure we have a "low" state. After we unload the driver, let's
+		 * disable the 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,
@@ -633,13 +762,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);
diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
index 6649e84d25de..c13dda444526 100644
--- a/drivers/iio/adc/ad7606.h
+++ b/drivers/iio/adc/ad7606.h
@@ -65,6 +65,7 @@ struct ad7606_chip_info {
  * @bops		bus operations (SPI or parallel)
  * @range		voltage range selection, selects which scale to apply
  * @oversampling	oversampling selection
+ * @cnvst_pwm		pointer to the PWM device connected to the cnvst pin
  * @base_address	address from where to read data in parallel operation
  * @sw_mode_en		software mode enabled
  * @scale_avail		pointer to the array which stores the available scales
@@ -94,6 +95,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 v2 07/10] iio: adc: ad7606: Add compatibility to fw_nodes

[Guillaume Stols]

On the parallel version, the current implementation is only compatible
with id tables and won't work with fw_nodes, this commit intends to fix
it.

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     | 208 ++++++++++++++++++++++++-------------------
 drivers/iio/adc/ad7606.h     |  34 +++++--
 drivers/iio/adc/ad7606_par.c |  29 +++---
 drivers/iio/adc/ad7606_spi.c |  31 ++++---
 4 files changed, 173 insertions(+), 129 deletions(-)

diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index b98057138295..7f2ff1674638 100644
--- a/drivers/iio/adc/ad7606.c
+++ b/drivers/iio/adc/ad7606.c
@@ -51,6 +51,116 @@ 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),
+};
+
+const struct ad7606_chip_info ad7605_4_info = {
+	.channels = ad7605_channels,
+	.num_channels = 5,
+	.name = "ad7605-4",
+	.id = ID_AD7605_4,
+};
+EXPORT_SYMBOL_NS_GPL(ad7605_4_info, IIO_AD7606);
+
+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,
+};
+EXPORT_SYMBOL_NS_GPL(ad7606_8_info, IIO_AD7606);
+
+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,
+};
+EXPORT_SYMBOL_NS_GPL(ad7606_6_info, IIO_AD7606);
+
+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,
+};
+EXPORT_SYMBOL_NS_GPL(ad7606_4_info, IIO_AD7606);
+
+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,
+};
+EXPORT_SYMBOL_NS_GPL(ad7606b_info, IIO_AD7606);
+
+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,
+};
+EXPORT_SYMBOL_NS_GPL(ad7616_info, IIO_AD7606);
+
 int ad7606_reset(struct ad7606_state *st)
 {
 	if (st->gpio_reset) {
@@ -395,96 +505,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;
@@ -624,7 +644,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;
@@ -653,7 +673,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;
@@ -676,7 +696,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;
 
@@ -758,7 +778,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 c13dda444526..18c87fe9a41a 100644
--- a/drivers/iio/adc/ad7606.h
+++ b/drivers/iio/adc/ad7606.h
@@ -38,8 +38,19 @@
 	AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),\
 		0, BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO))
 
+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
@@ -50,6 +61,8 @@
  *			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;
@@ -150,19 +163,22 @@ 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);
 
 int ad7606_reset(struct ad7606_state *st);
 
-enum ad7606_supported_device_ids {
-	ID_AD7605_4,
-	ID_AD7606_8,
-	ID_AD7606_6,
-	ID_AD7606_4,
-	ID_AD7606B,
-	ID_AD7616,
-};
+extern const struct ad7606_chip_info ad7605_4_info;
+
+extern const struct ad7606_chip_info ad7606_8_info;
+
+extern const struct ad7606_chip_info ad7606_6_info;
+
+extern const struct ad7606_chip_info ad7606_4_info;
+
+extern const struct ad7606_chip_info ad7606b_info;
+
+extern const struct ad7606_chip_info ad7616_info;
 
 #ifdef CONFIG_PM_SLEEP
 extern const struct dev_pm_ops ad7606_pm_ops;
diff --git a/drivers/iio/adc/ad7606_par.c b/drivers/iio/adc/ad7606_par.c
index d651639c45eb..7bac39033955 100644
--- a/drivers/iio/adc/ad7606_par.c
+++ b/drivers/iio/adc/ad7606_par.c
@@ -11,6 +11,7 @@
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/property.h>
 #include <linux/types.h>
 
 #include <linux/iio/iio.h>
@@ -89,12 +90,20 @@ 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;
+	const struct platform_device_id *id;
 	struct resource *res;
 	void __iomem *addr;
 	resource_size_t remap_size;
 	int irq;
 
+	if (dev_fwnode(&pdev->dev)) {
+		chip_info = device_get_match_data(&pdev->dev);
+	} else {
+		id = platform_get_device_id(pdev);
+		chip_info = (const struct ad7606_chip_info *)id->driver_data;
+	}
+
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0)
 		return irq;
@@ -106,25 +115,25 @@ 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 d3fbc7c7a823..719f9e09a5f7 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 v2 08/10] iio: adc: ad7606: Fix typo in the driver name

[Guillaume Stols]

The parallel driver's name is ad7606_par and not ad7606_parallel.

Fixes: 0046a46a8f93 ("staging/ad7606: Actually build the interface modules")

Signed-off-by: Guillaume Stols <gstols@baylibre.com>
---
 drivers/iio/adc/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 97ece1a4b7e3..4ab1a3092d88 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -229,7 +229,7 @@ config AD7606_IFACE_PARALLEL
 	  ad7605-4, ad7606, ad7606-6, ad7606-4 analog to digital converters (ADC).
 
 	  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"

-- 
2.34.1

[PATCH v2 09/10] 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.

Signed-off-by: Guillaume Stols <gstols@baylibre.com>
---
 drivers/iio/adc/Kconfig      |  2 +
 drivers/iio/adc/ad7606.c     | 94 +++++++++++++++++++++++++++++++++++++-------
 drivers/iio/adc/ad7606.h     | 15 +++++++
 drivers/iio/adc/ad7606_par.c | 91 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 187 insertions(+), 15 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 4ab1a3092d88..9b52d5b2c592 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -224,9 +224,11 @@ config AD7606_IFACE_PARALLEL
 	tristate "Analog Devices AD7606 ADC driver with parallel interface support"
 	depends on HAS_IOPORT
 	select AD7606
+	select IIO_BACKEND
 	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_par.
diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index 7f2ff1674638..f710445bdc22 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>
@@ -271,7 +272,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;
 
 	return st->bops->read_block(st->dev, num, data);
@@ -319,11 +328,14 @@ 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;
+
+	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);
@@ -349,6 +361,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:
@@ -369,6 +382,10 @@ 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(st->cnvst_pwm, &cnvst_pwm_state);
+		*val = DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, cnvst_pwm_state.period);
+		return IIO_VAL_INT;
 	}
 	return -EINVAL;
 }
@@ -458,6 +475,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;
 	}
@@ -595,14 +614,49 @@ static int ad7606_buffer_predisable(struct iio_dev *indio_dev)
 	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 = {
@@ -610,6 +664,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 = {
@@ -618,6 +673,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 = {
@@ -625,6 +681,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 = {
@@ -632,6 +689,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 = {
@@ -700,8 +758,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");
@@ -774,14 +830,22 @@ 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) {
+		ret = st->bops->iio_backend_config(dev, indio_dev);
+		if (ret)
+			return ret;
+		indio_dev->setup_ops = &ad7606_pwm_buffer_ops;
+	} else {
+		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 18c87fe9a41a..53cd8eb4898e 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))
@@ -59,6 +65,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;
@@ -69,6 +76,7 @@ struct ad7606_chip_info {
 	unsigned int			oversampling_num;
 	bool				os_req_reset;
 	unsigned long			init_delay_ms;
+	bool				has_backend;
 };
 
 /**
@@ -115,6 +123,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);
 
@@ -139,16 +148,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);
@@ -159,6 +173,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);
 };
 
diff --git a/drivers/iio/adc/ad7606_par.c b/drivers/iio/adc/ad7606_par.c
index 7bac39033955..564284ede997 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/err.h>
@@ -15,8 +17,80 @@
 #include <linux/types.h>
 
 #include <linux/iio/iio.h>
+#include <linux/iio/backend.h>
+
 #include "ad7606.h"
 
+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;
+	struct iio_backend_data_fmt data = {
+		.sign_extend = true,
+		.enable = true,
+	};
+
+	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;
+
+	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,
+};
+
 static int ad7606_par16_read_block(struct device *dev,
 				   int count, void *buf)
 {
@@ -96,9 +170,23 @@ static int ad7606_par_probe(struct platform_device *pdev)
 	void __iomem *addr;
 	resource_size_t remap_size;
 	int irq;
+	struct iio_backend *back;
 
+	/*
+	 * If a firmware node is available (ACPI or DT), platform_device_id is null
+	 * and we must use get_match_data.
+	 */
 	if (dev_fwnode(&pdev->dev)) {
 		chip_info = device_get_match_data(&pdev->dev);
+		back = devm_iio_backend_get(&pdev->dev, NULL);
+		if (!IS_ERR(back))
+			/*
+			 * If a backend is available in the device tree, call the core
+			 * probe with backend bops, otherwise use the former bops.
+			 */
+			return ad7606_probe(&pdev->dev, 0, NULL,
+					    chip_info,
+					    &ad7606_bi_bops);
 	} else {
 		id = platform_get_device_id(pdev);
 		chip_info = (const struct ad7606_chip_info *)id->driver_data;
@@ -125,6 +213,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);
@@ -134,6 +223,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);
@@ -153,3 +243,4 @@ MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
 MODULE_DESCRIPTION("Analog Devices AD7606 ADC");
 MODULE_LICENSE("GPL v2");
 MODULE_IMPORT_NS(IIO_AD7606);
+MODULE_IMPORT_NS(IIO_BACKEND);

-- 
2.34.1

[PATCH v2 10/10] iio: adc: ad7606: Disable PWM usage for non backend version

[Guillaume Stols]

Since the pwm was introduced before backend, there was an example use
whit triggered buffers. However, using it may be dangerous, because if
the PWM goes too fast, a new conversion can be triggered before the
transmission is over, whit the associated mess in the buffers.
Until a solution is found to mitigate this risk, for instante CRC
support, the PWM will be disabled.

Signed-off-by: Guillaume Stols <gstols@baylibre.com>
---
 drivers/iio/adc/ad7606.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index f710445bdc22..0c12ca237ee9 100644
--- a/drivers/iio/adc/ad7606.c
+++ b/drivers/iio/adc/ad7606.c
@@ -599,7 +599,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;
 }
@@ -609,7 +608,6 @@ 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;
 }
@@ -838,6 +836,12 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
 		indio_dev->setup_ops = &ad7606_pwm_buffer_ops;
 	} else {
 		init_completion(&st->completion);
+
+		/* Reserve the PWM use only for backend (force gpio_convst definition) */
+		if (!st->gpio_convst)
+			return dev_err_probe(dev, -EINVAL,
+					     "No backend, connect convst to a GPIO");
+
 		ret = devm_request_threaded_irq(dev, irq,
 						NULL,
 						&ad7606_interrupt,

-- 
2.34.1

Re: [PATCH v2 01/10] dt-bindings: iio: adc: ad7606: Set the correct polarity

[Uwe Kleine-König]

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

Hello Guillaume,

On Fri, Sep 20, 2024 at 05:33:21PM +0000, Guillaume Stols wrote:
> According to the datasheet, "Data is clocked in from SDI on the falling
> edge of SCLK, while data is clocked out on DOUTA on the rising edge of
> SCLK".
> Also, even if not stated textually in the datasheet, it is made clear on
> the diagrams that sclk idles at high.
> 
> So the documentation is erroneously stating that spi-cpha is required, and
> the example is erroneously setting both spi-cpol and spi-cpha.

I would expect that the communication with the chip is at least
unreliable if not outright broken with the wrong polarity. So maybe add
something like:

	On $MyMachine dropping the spi-cpha property reduces IO errors / fixes
	measurement readout / improves somehow differently.

to the commit log?

> Fixes: 416f882c3b40 ("dt-bindings: iio: adc: Migrate AD7606 documentation to yaml")
> Fixes: 6e33a125df66 ("dt-bindings: iio: adc: Add docs for AD7606 ADC")
> 
> Signed-off-by: Guillaume Stols <gstols@baylibre.com>

The empty line between Fixes and S-o-b is unusual. Assuming you resend
anyway, please drop it.

> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> index 69408cae3db9..75334a033539 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> @@ -29,8 +29,6 @@ properties:
>    reg:
>      maxItems: 1
>  
> -  spi-cpha: true
> -
>    spi-cpol: true
>  
>    avcc-supply: true
> @@ -117,7 +115,7 @@ properties:
>  required:
>    - compatible
>    - reg
> -  - spi-cpha
> +  - spi-cpol

Adding cpol seems unrelated to this patch. (And you remove it again in
patch #2.)

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

Best regards
Uwe

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

Re: [PATCH v2 02/10] dt-bindings: iio: adc: ad7606: Make corrections on spi conditions

[Conor Dooley]

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

On Fri, Sep 20, 2024 at 05:33:22PM +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.

And, yaknow, not that the bus you're on is a spi bus? I don't think this
comment is relevant to the binding, especially given you have a property
for it.

> There is also a correction on the spi-cpha that is not required when SPI
> interface is selected, while spi-cpol is.

I don't see this change in your patch, there's no cpha in the before.

> 
> Signed-off-by: Guillaume Stols <gstols@baylibre.com>
> ---
>  .../devicetree/bindings/iio/adc/adi,ad7606.yaml      | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> index 75334a033539..12995ebcddc2 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> @@ -112,18 +112,32 @@ properties:
>        assumed that the pins are hardwired to VDD.
>      type: boolean
>  
> +  parallel-interface:
> +    description:
> +      If the parallel interface is used, be it directly or through a backend,
> +      this property must be defined.
> +    type: boolean

The type you would want here is actually "flag", but I'm not sure why a
property is needed. If you're using the parallel interface, why would
you still be on a spi bus? I think I'm a bit confused here as to how
this interface is supposed to be used.

Thanks,
Conor.

> +
>  required:
>    - compatible
>    - reg
> -  - spi-cpol
>    - avcc-supply
>    - vdrive-supply
>    - interrupts
>    - adi,conversion-start-gpios
>  
> -allOf:
> -  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +oneOf:
> +  - required:
> +      - parallel-interface
> +  - allOf:
> +      - properties:
> +          parallel-interface: false
> +          spi-cpol: true
> +      - $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 v2 03/10] dt-bindings: iio: adc: ad7606: Add iio backend bindings

[Conor Dooley]

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

On Fri, Sep 20, 2024 at 05:33:23PM +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 transfer) 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    | 76 +++++++++++++++++++++-
>  1 file changed, 74 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> index 12995ebcddc2..74a8680904b1 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> @@ -118,13 +118,32 @@ properties:
>        this property must be defined.
>      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

Please use an items list to describe what each item is, rather than
doing so in the pwm-names description below.

> +
> +  pwm-names:
> +    description:
> +      The name of each PWM, the first is connected to CONVST, and the second is
> +      connected to CONVST2 if CONVST2 is available and not connected to CONVST1.
> +    minItems: 1
> +    maxItems: 2

You need to define what the names actually are, otherwise you have no
ABI.

Cheers,
Conor.

> +
> +  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
>  
>  oneOf:
>    - required:
> @@ -138,6 +157,34 @@ oneOf:
>            - spi-cpol
>  
>  allOf:
> +  - 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:
> @@ -179,12 +226,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";
> +            parallel-interface;
> +            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 v2 10/10] iio: adc: ad7606: Disable PWM usage for non backend version

[David Lechner]

On Fri, Sep 20, 2024 at 7:33 PM Guillaume Stols <gstols@baylibre.com> wrote:
>
> Since the pwm was introduced before backend, there was an example use
> whit triggered buffers. However, using it may be dangerous, because if

with

> the PWM goes too fast, a new conversion can be triggered before the
> transmission is over, whit the associated mess in the buffers.

with

> Until a solution is found to mitigate this risk, for instante CRC

instance

> support, the PWM will be disabled.
>
> Signed-off-by: Guillaume Stols <gstols@baylibre.com>
> ---

Re: [PATCH v2 09/10] iio: adc: ad7606: Add iio-backend support

[David Lechner]

On Fri, Sep 20, 2024 at 7:33 PM 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.
>
> Signed-off-by: Guillaume Stols <gstols@baylibre.com>
> ---
>  drivers/iio/adc/Kconfig      |  2 +
>  drivers/iio/adc/ad7606.c     | 94 +++++++++++++++++++++++++++++++++++++-------
>  drivers/iio/adc/ad7606.h     | 15 +++++++
>  drivers/iio/adc/ad7606_par.c | 91 ++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 187 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 4ab1a3092d88..9b52d5b2c592 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -224,9 +224,11 @@ config AD7606_IFACE_PARALLEL
>         tristate "Analog Devices AD7606 ADC driver with parallel interface support"
>         depends on HAS_IOPORT
>         select AD7606
> +       select IIO_BACKEND
>         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_par.
> diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
> index 7f2ff1674638..f710445bdc22 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>
> @@ -271,7 +272,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;

Probably better to introduce a new num_voltage_channels field to
chip_info instead of trying to reverse engineer if there is a
timestamp channel or not.

> +
> +       /*
> +        * Timestamp channel does not contain sample, and no timestamp channel if
> +        * backend is used.
> +        */
> +       if (!st->back)
> +               num--;
> +
>         u16 *data = st->data;
>
>         return st->bops->read_block(st->dev, num, data);
> @@ -319,11 +328,14 @@ 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;
> +
> +       if (!st->back) {
> +               ret = wait_for_completion_timeout(&st->completion,
> +                                                 msecs_to_jiffies(1000));
> +               if (!ret) {
> +                       ret = -ETIMEDOUT;
> +                       goto error_ret;
> +               }
>         }

Would it be better to just make a different scan_direct function for
the case when we don't have the BUSY interrtup?

>
>         ret = ad7606_read_samples(st);
> @@ -349,6 +361,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:
> @@ -369,6 +382,10 @@ 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(st->cnvst_pwm, &cnvst_pwm_state);
> +               *val = DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, cnvst_pwm_state.period);
> +               return IIO_VAL_INT;
>         }
>         return -EINVAL;
>  }
> @@ -458,6 +475,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;
>         }
> @@ -595,14 +614,49 @@ static int ad7606_buffer_predisable(struct iio_dev *indio_dev)
>         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;
> +}

Usually, we try to avoid else after return.

So perhaps simpler:

if (!st->bops->update_scan_mode)
        return 0;

return st->bops->update_scan_mode(indio_dev, scan_mask);

> +
>  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 = {
> @@ -610,6 +664,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 = {
> @@ -618,6 +673,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 = {
> @@ -625,6 +681,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 = {
> @@ -632,6 +689,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 = {
> @@ -700,8 +758,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");
> @@ -774,14 +830,22 @@ 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) {
> +               ret = st->bops->iio_backend_config(dev, indio_dev);
> +               if (ret)
> +                       return ret;
> +               indio_dev->setup_ops = &ad7606_pwm_buffer_ops;
> +       } else {
> +               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 18c87fe9a41a..53cd8eb4898e 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))
> @@ -59,6 +65,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;
> @@ -69,6 +76,7 @@ struct ad7606_chip_info {
>         unsigned int                    oversampling_num;
>         bool                            os_req_reset;
>         unsigned long                   init_delay_ms;
> +       bool                            has_backend;

It isn't clear to me what this flag is for. If the flag is true, does
it mean that the chip requires to use the IIO backend and not the
older parallel interface? What if there is some chip that need to
support both?

>  };
>
>  /**
> @@ -115,6 +123,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);
>
> @@ -139,16 +148,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);
> @@ -159,6 +173,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);
>  };
>
> diff --git a/drivers/iio/adc/ad7606_par.c b/drivers/iio/adc/ad7606_par.c
> index 7bac39033955..564284ede997 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.

Can just add year to existing copyright line.

> + * Copyright 2024 BayLibre SAS.
>   */
>
>  #include <linux/err.h>
> @@ -15,8 +17,80 @@
>  #include <linux/types.h>
>
>  #include <linux/iio/iio.h>
> +#include <linux/iio/backend.h>

Alphabetical order?

> +
>  #include "ad7606.h"
>
> +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;
> +       struct iio_backend_data_fmt data = {
> +               .sign_extend = true,
> +               .enable = true,
> +       };
> +
> +       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;

Probably useful to print an error message here since EINVAL can be a
lot of things.

> +
> +       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;
> +
> +       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;

Can use ARRAY_SIZE(ad7606b_bi_channels) instead of hard-coding 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,
> +};
> +
>  static int ad7606_par16_read_block(struct device *dev,
>                                    int count, void *buf)
>  {
> @@ -96,9 +170,23 @@ static int ad7606_par_probe(struct platform_device *pdev)
>         void __iomem *addr;
>         resource_size_t remap_size;
>         int irq;
> +       struct iio_backend *back;
>
> +       /*
> +        * If a firmware node is available (ACPI or DT), platform_device_id is null
> +        * and we must use get_match_data.
> +        */
>         if (dev_fwnode(&pdev->dev)) {
>                 chip_info = device_get_match_data(&pdev->dev);
> +               back = devm_iio_backend_get(&pdev->dev, NULL);
> +               if (!IS_ERR(back))
> +                       /*
> +                        * If a backend is available in the device tree, call the core
> +                        * probe with backend bops, otherwise use the former bops.
> +                        */
> +                       return ad7606_probe(&pdev->dev, 0, NULL,
> +                                           chip_info,
> +                                           &ad7606_bi_bops);

It seems strange to be this adding inside the if statement for the DT
case. It would be more future proof to have it after instead, e.g. if
you bring back the patch for ad7606b_bi_channels().

>         } else {
>                 id = platform_get_device_id(pdev);
>                 chip_info = (const struct ad7606_chip_info *)id->driver_data;
> @@ -125,6 +213,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);
> @@ -134,6 +223,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);
> @@ -153,3 +243,4 @@ MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
>  MODULE_DESCRIPTION("Analog Devices AD7606 ADC");
>  MODULE_LICENSE("GPL v2");
>  MODULE_IMPORT_NS(IIO_AD7606);
> +MODULE_IMPORT_NS(IIO_BACKEND);
>
> --
> 2.34.1
>ad7606b_bi_channels

Re: [PATCH v2 02/10] dt-bindings: iio: adc: ad7606: Make corrections on spi conditions

[Guillaume Stols]

On 9/21/24 23:55, Conor Dooley wrote:
> On Fri, Sep 20, 2024 at 05:33:22PM +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.
> And, yaknow, not that the bus you're on is a spi bus? I don't think this
> comment is relevant to the binding, especially given you have a property
> for it.

Apologies, I missed to change the commit message, it will be fixed in 
the next series.

Since Jonathan did not like very much inferring the interface with the 
reg's value that I used i the previous verison, I introduced this flag.

However this is only intended to be use in bindings, to determine 
whether or not spi properties should be added.

In the driver side of things, the bus interface is inferred by the 
parent's node (SPI driver is an module_spi_driver while parallel driver 
is module_platform_driver).

>
>> There is also a correction on the spi-cpha that is not required when SPI
>> interface is selected, while spi-cpol is.
> I don't see this change in your patch, there's no cpha in the before.
>
Again a problem with the commit message, this belongs now to another commit.
>> Signed-off-by: Guillaume Stols <gstols@baylibre.com>
>> ---
>>   .../devicetree/bindings/iio/adc/adi,ad7606.yaml      | 20 +++++++++++++++++---
>>   1 file changed, 17 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
>> index 75334a033539..12995ebcddc2 100644
>> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
>> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
>> @@ -112,18 +112,32 @@ properties:
>>         assumed that the pins are hardwired to VDD.
>>       type: boolean
>>   
>> +  parallel-interface:
>> +    description:
>> +      If the parallel interface is used, be it directly or through a backend,
>> +      this property must be defined.
>> +    type: boolean
> The type you would want here is actually "flag", but I'm not sure why a
> property is needed. If you're using the parallel interface, why would
> you still be on a spi bus? I think I'm a bit confused here as to how
> this interface is supposed to be used.
>
> Thanks,
> Conor.
>
>> +
>>   required:
>>     - compatible
>>     - reg
>> -  - spi-cpol
>>     - avcc-supply
>>     - vdrive-supply
>>     - interrupts
>>     - adi,conversion-start-gpios
>>   
>> -allOf:
>> -  - $ref: /schemas/spi/spi-peripheral-props.yaml#
>> +oneOf:
>> +  - required:
>> +      - parallel-interface
>> +  - allOf:
>> +      - properties:
>> +          parallel-interface: false
>> +          spi-cpol: true
>> +      - $ref: /schemas/spi/spi-peripheral-props.yaml#
>> +      - required:
>> +          - spi-cpol
>>   
>> +allOf:
>>     - if:
>>         properties:
>>           compatible:
>>
>> -- 
>> 2.34.1
>>

Re: [PATCH v2 02/10] dt-bindings: iio: adc: ad7606: Make corrections on spi conditions

[Conor Dooley]

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

On Tue, Sep 24, 2024 at 04:41:50PM +0200, Guillaume Stols wrote:
> 
> On 9/21/24 23:55, Conor Dooley wrote:
> > On Fri, Sep 20, 2024 at 05:33:22PM +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.
> > And, yaknow, not that the bus you're on is a spi bus? I don't think this
> > comment is relevant to the binding, especially given you have a property
> > for it.
> 
> Apologies, I missed to change the commit message, it will be fixed in the
> next series.
> 
> Since Jonathan did not like very much inferring the interface with the reg's
> value that I used i the previous verison, I introduced this flag.
> 
> However this is only intended to be use in bindings, to determine whether or
> not spi properties should be added.

To be honest, if it is not needed by software to understand what bus the
device is on, it shouldn't be in the bindings at all. What was Jonathan
opposed to? Doing an if reg < 1000: do y, otherwise do x?
I'd not bother with any of that, and just make cpha (or w/e it was)
optional with a description explaining the circumstances in which is it
needed.

> In the driver side of things, the bus interface is inferred by the parent's
> node (SPI driver is an module_spi_driver while parallel driver is
> module_platform_driver).

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

Re: [PATCH v2 07/10] iio: adc: ad7606: Add compatibility to fw_nodes

[David Lechner]

On Fri, Sep 20, 2024 at 7:33 PM Guillaume Stols <gstols@baylibre.com> wrote:
>
> On the parallel version, the current implementation is only compatible
> with id tables and won't work with fw_nodes, this commit intends to fix
> it.
>
> 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 sounds like two unrelated changes, so maybe we should have two patches?


>  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 },

Since we have .compatible = , we should also have .data = for the chip info.

Re: [PATCH v2 07/10] iio: adc: ad7606: Add compatibility to fw_nodes

[Guillaume Stols]

On 9/24/24 17:28, David Lechner wrote:
> On Fri, Sep 20, 2024 at 7:33 PM Guillaume Stols <gstols@baylibre.com> wrote:
>> On the parallel version, the current implementation is only compatible
>> with id tables and won't work with fw_nodes, this commit intends to fix
>> it.
>>
>> 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 sounds like two unrelated changes, so maybe we should have two patches?
Those changes are closely related to each other, in the sense that we 
now gather the ad7606_chip_info structure directly from the id or match 
structure, and not anymore the id which is an index where you can get it 
as an element. I will update the commit message to highlight it more.
>
>
>>   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 },
> Since we have .compatible = , we should also have .data = for the chip info.
ack

Re: [PATCH v2 02/10] dt-bindings: iio: adc: ad7606: Make corrections on spi conditions

[Guillaume Stols]

On 9/24/24 16:59, Conor Dooley wrote:
> On Tue, Sep 24, 2024 at 04:41:50PM +0200, Guillaume Stols wrote:
>> On 9/21/24 23:55, Conor Dooley wrote:
>>> On Fri, Sep 20, 2024 at 05:33:22PM +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.
>>> And, yaknow, not that the bus you're on is a spi bus? I don't think this
>>> comment is relevant to the binding, especially given you have a property
>>> for it.
>> Apologies, I missed to change the commit message, it will be fixed in the
>> next series.
>>
>> Since Jonathan did not like very much inferring the interface with the reg's
>> value that I used i the previous verison, I introduced this flag.
>>
>> However this is only intended to be use in bindings, to determine whether or
>> not spi properties should be added.
> To be honest, if it is not needed by software to understand what bus the
> device is on, it shouldn't be in the bindings at all. What was Jonathan
> opposed to? Doing an if reg < 1000: do y, otherwise do x?
> I'd not bother with any of that, and just make cpha (or w/e it was)
> optional with a description explaining the circumstances in which is it
> needed.
OK, it will be removed from the series and sent as a side patch because 
it anyways does not really belong to this series.
>> In the driver side of things, the bus interface is inferred by the parent's
>> node (SPI driver is an module_spi_driver while parallel driver is
>> module_platform_driver).

Re: [PATCH v2 01/10] dt-bindings: iio: adc: ad7606: Set the correct polarity

[Jonathan Cameron]

On Sat, 21 Sep 2024 11:11:31 +0200
Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:

> Hello Guillaume,
> 
> On Fri, Sep 20, 2024 at 05:33:21PM +0000, Guillaume Stols wrote:
> > According to the datasheet, "Data is clocked in from SDI on the falling
> > edge of SCLK, while data is clocked out on DOUTA on the rising edge of
> > SCLK".
> > Also, even if not stated textually in the datasheet, it is made clear on
> > the diagrams that sclk idles at high.
> > 
> > So the documentation is erroneously stating that spi-cpha is required, and
> > the example is erroneously setting both spi-cpol and spi-cpha.  
> 
> I would expect that the communication with the chip is at least
> unreliable if not outright broken with the wrong polarity. So maybe add
> something like:
> 
> 	On $MyMachine dropping the spi-cpha property reduces IO errors / fixes
> 	measurement readout / improves somehow differently.
> 
> to the commit log?
> 
> > Fixes: 416f882c3b40 ("dt-bindings: iio: adc: Migrate AD7606 documentation to yaml")
> > Fixes: 6e33a125df66 ("dt-bindings: iio: adc: Add docs for AD7606 ADC")
> > 
> > Signed-off-by: Guillaume Stols <gstols@baylibre.com>  
> 
> The empty line between Fixes and S-o-b is unusual. Assuming you resend
> anyway, please drop it.

It's also scanned for in linux-next so to reduce chances I forget to fix this
definitely resend.

> 
> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> > index 69408cae3db9..75334a033539 100644
> > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> > @@ -29,8 +29,6 @@ properties:
> >    reg:
> >      maxItems: 1
> >  
> > -  spi-cpha: true
> > -
> >    spi-cpol: true
> >  
> >    avcc-supply: true
> > @@ -117,7 +115,7 @@ properties:
> >  required:
> >    - compatible
> >    - reg
> > -  - spi-cpha
> > +  - spi-cpol  
> 
> Adding cpol seems unrelated to this patch. (And you remove it again in
> patch #2.)
> 
> >    - avcc-supply
> >    - vdrive-supply
> >    - interrupts  
> 
> Best regards
> Uwe

Re: [PATCH v2 02/10] dt-bindings: iio: adc: ad7606: Make corrections on spi conditions

[Jonathan Cameron]

On Wed, 25 Sep 2024 17:28:30 +0200
Guillaume Stols <gstols@baylibre.com> wrote:

> On 9/24/24 16:59, Conor Dooley wrote:
> > On Tue, Sep 24, 2024 at 04:41:50PM +0200, Guillaume Stols wrote:  
> >> On 9/21/24 23:55, Conor Dooley wrote:  
> >>> On Fri, Sep 20, 2024 at 05:33:22PM +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.  
> >>> And, yaknow, not that the bus you're on is a spi bus? I don't think this
> >>> comment is relevant to the binding, especially given you have a property
> >>> for it.  
> >> Apologies, I missed to change the commit message, it will be fixed in the
> >> next series.
> >>
> >> Since Jonathan did not like very much inferring the interface with the reg's
> >> value that I used i the previous verison, I introduced this flag.
> >>
> >> However this is only intended to be use in bindings, to determine whether or
> >> not spi properties should be added.  
> > To be honest, if it is not needed by software to understand what bus the
> > device is on, it shouldn't be in the bindings at all. What was Jonathan
> > opposed to? Doing an if reg < 1000: do y, otherwise do x?
> > I'd not bother with any of that, and just make cpha (or w/e it was)
> > optional with a description explaining the circumstances in which is it
> > needed.  
> OK, it will be removed from the series and sent as a side patch because 
> it anyways does not really belong to this series.
I can't remember the original thread (or immediately find it).
So I may have this totally wrong. 
- I don't want checks on reg value to change how the binding works as that
  is a wieird corner and in theory this device could be at a small address anyway.

- Fine to do as Conor suggests and just add a comment for this
  corner case rather than making it required.

Jonathan
> >> In the driver side of things, the bus interface is inferred by the parent's
> >> node (SPI driver is an module_spi_driver while parallel driver is
> >> module_platform_driver).  

Re: [PATCH v2 04/10] Documentation: iio: Document ad7606 driver

[Jonathan Cameron]

On Fri, 20 Sep 2024 17:33:24 +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>
> ---
>  Documentation/iio/ad7606.rst | 143 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 143 insertions(+)
> 
> diff --git a/Documentation/iio/ad7606.rst b/Documentation/iio/ad7606.rst
> new file mode 100644
> index 000000000000..270a49aae685
> --- /dev/null
> +++ b/Documentation/iio/ad7606.rst
> @@ -0,0 +1,143 @@
> +.. 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

If intent here is AD7606 and AD7606B only as covered by that wildcard, then
I'd just state them both explicitly.  If the others are intended that wildcard
is wrong.  If it's all of them, just say These ADCs

> +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 through 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
> ++---+                                       +-----------------------------
> +

I think we should introduce an annual award for best kernel ASCII art.
This one is nice.

> +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. 

That's a dodge I think.  There is no timestamp because we have no way to insert
one into the DMA buffer!  I'd drop the hence
"stable.  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.
> +

Nice docs in general.

Jonathan

Re: [PATCH v2 06/10] iio: adc: ad7606: Add PWM support for conversion trigger

[Jonathan Cameron]

On Fri, 20 Sep 2024 17:33:26 +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.
Spell check patch descriptions.  conjunction

Note this is a do as I say, not as I do because my spelling is terrible and
I frequently forget to check them.

A few trivial things inline.

Jonathan

> 
> Signed-off-by: Guillaume Stols <gstols@baylibre.com>
> ---
>  drivers/iio/adc/ad7606.c | 170 ++++++++++++++++++++++++++++++++++++++++-------
>  drivers/iio/adc/ad7606.h |   2 +
>  2 files changed, 148 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
> index 9b457472d49c..b98057138295 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>
> @@ -83,6 +85,80 @@ 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;

Trivial but add a blank line here to separate the sanity check from the
guts of the function.

> +	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;

Likewise blank line here.

> +	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;
And 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;
> +}
> @@ -130,7 +219,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];
> -

I'd keep a blank line here as this code unconnected to previous block.

> +	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);

Re: [PATCH v2 07/10] iio: adc: ad7606: Add compatibility to fw_nodes

[Jonathan Cameron]

On Fri, 20 Sep 2024 17:33:27 +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 fw_nodes, this commit intends to fix
> it.
> 
> Also, chip info is moved in the .h file so to be accessible to all the

chip info is not moved (I was going to say no to that) but an
extern is used to make it available. So say that rather than moved here.

> driver files that can set a pointer to the corresponding chip as the
> driver data.
> 

>  
> diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
> index c13dda444526..18c87fe9a41a 100644
> --- a/drivers/iio/adc/ad7606.h
> +++ b/drivers/iio/adc/ad7606.h
> @@ -38,8 +38,19 @@
>  	AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),\
>  		0, BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO))
>  
> +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

ID in chip info normally indicates something bad in the design. In that somewhere
we have code that is ID dependent rather than all such code / data being
found directly in this structure (or callbacks found from here).
Can we avoid it here?

>   * @channels:		channel specification
>   * @num_channels:	number of channels
>   * @oversampling_avail	pointer to the array which stores the available
> @@ -50,6 +61,8 @@

...

> diff --git a/drivers/iio/adc/ad7606_par.c b/drivers/iio/adc/ad7606_par.c
> index d651639c45eb..7bac39033955 100644
> --- a/drivers/iio/adc/ad7606_par.c
> +++ b/drivers/iio/adc/ad7606_par.c
> @@ -11,6 +11,7 @@
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> +#include <linux/property.h>
>  #include <linux/types.h>
>  
>  #include <linux/iio/iio.h>
> @@ -89,12 +90,20 @@ 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;
> +	const struct platform_device_id *id;
>  	struct resource *res;
>  	void __iomem *addr;
>  	resource_size_t remap_size;
>  	int irq;
>  
> +	if (dev_fwnode(&pdev->dev)) {
> +		chip_info = device_get_match_data(&pdev->dev);
> +	} else {
> +		id = platform_get_device_id(pdev);
> +		chip_info = (const struct ad7606_chip_info *)id->driver_data;
> +	}
> +
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq < 0)
>  		return irq;
> @@ -106,25 +115,25 @@ 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,
Rewrap to move chip_info up a line perhaps.

> +			    chip_info,
>  			    remap_size > 1 ? &ad7606_par16_bops :
>  			    &ad7606_par8_bops);

Re: [PATCH v2 08/10] iio: adc: ad7606: Fix typo in the driver name

[Jonathan Cameron]

On Fri, 20 Sep 2024 17:33:28 +0000
Guillaume Stols <gstols@baylibre.com> wrote:

> The parallel driver's name is ad7606_par and not ad7606_parallel.
> 
> Fixes: 0046a46a8f93 ("staging/ad7606: Actually build the interface modules")

No blank line here and drag this to start of patch set.
> 
> Signed-off-by: Guillaume Stols <gstols@baylibre.com>
> ---
>  drivers/iio/adc/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 97ece1a4b7e3..4ab1a3092d88 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -229,7 +229,7 @@ config AD7606_IFACE_PARALLEL
>  	  ad7605-4, ad7606, ad7606-6, ad7606-4 analog to digital converters (ADC).
>  
>  	  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"
> 

Re: [PATCH v2 09/10] iio: adc: ad7606: Add iio-backend support

[Jonathan Cameron]

On Fri, 20 Sep 2024 17:33:29 +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.
> 
> Signed-off-by: Guillaume Stols <gstols@baylibre.com>
One trivial comment to add to what David covered.
I'm also curious now on what the limitation is that meant we didn't
support the AD7606B with parallel interface before, and what stops us
supporting other devices with the backend IP?  Is there a fundamental difference?
> ---
>  drivers/iio/adc/Kconfig      |  2 +
>  drivers/iio/adc/ad7606.c     | 94 +++++++++++++++++++++++++++++++++++++-------
>  drivers/iio/adc/ad7606.h     | 15 +++++++
>  drivers/iio/adc/ad7606_par.c | 91 ++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 187 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 4ab1a3092d88..9b52d5b2c592 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -224,9 +224,11 @@ config AD7606_IFACE_PARALLEL
>  	tristate "Analog Devices AD7606 ADC driver with parallel interface support"
>  	depends on HAS_IOPORT
>  	select AD7606
> +	select IIO_BACKEND
>  	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.

But not for other devices?  Is the expectation that they will need
a different IP for the backend, or just a case of not tested yet?


>  
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called ad7606_par.

> +
> +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.
	/*
	 * The update...

> +	 * 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;
> +}
> +

Re: [PATCH v2 10/10] iio: adc: ad7606: Disable PWM usage for non backend version

[Jonathan Cameron]

On Fri, 20 Sep 2024 17:33:30 +0000
Guillaume Stols <gstols@baylibre.com> wrote:

> Since the pwm was introduced before backend, there was an example use
> whit triggered buffers. However, using it may be dangerous, because if
> the PWM goes too fast, a new conversion can be triggered before the
> transmission is over, whit the associated mess in the buffers.
> Until a solution is found to mitigate this risk, for instante CRC
> support, the PWM will be disabled.
> 
> Signed-off-by: Guillaume Stols <gstols@baylibre.com>

Whilst I can see the logic introducing it in two steps, maybe
shout a bit more about that in the original patch as otherwise
some foolish unnamed maintainer might pick up part of this series
leaving things in a bad state for a kernel release.

The earlier patch 'recommends' it isn't used which is a bit week.
Say support is going to be removed later.
> ---
>  drivers/iio/adc/ad7606.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
> index f710445bdc22..0c12ca237ee9 100644
> --- a/drivers/iio/adc/ad7606.c
> +++ b/drivers/iio/adc/ad7606.c
> @@ -599,7 +599,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;
>  }
> @@ -609,7 +608,6 @@ 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;
>  }
> @@ -838,6 +836,12 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
>  		indio_dev->setup_ops = &ad7606_pwm_buffer_ops;
>  	} else {
>  		init_completion(&st->completion);
> +
> +		/* Reserve the PWM use only for backend (force gpio_convst definition) */
> +		if (!st->gpio_convst)
> +			return dev_err_probe(dev, -EINVAL,
> +					     "No backend, connect convst to a GPIO");
> +
>  		ret = devm_request_threaded_irq(dev, irq,
>  						NULL,
>  						&ad7606_interrupt,
> 

Re: [PATCH v2 07/10] iio: adc: ad7606: Add compatibility to fw_nodes

[Guillaume Stols]

On 9/29/24 14:44, Jonathan Cameron wrote:
> On Fri, 20 Sep 2024 17:33:27 +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 fw_nodes, this commit intends to fix
>> it.
>>
>> Also, chip info is moved in the .h file so to be accessible to all the
> chip info is not moved (I was going to say no to that) but an
> extern is used to make it available. So say that rather than moved here.
>
>> driver files that can set a pointer to the corresponding chip as the
>> driver data.
>>
>>   
>> diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
>> index c13dda444526..18c87fe9a41a 100644
>> --- a/drivers/iio/adc/ad7606.h
>> +++ b/drivers/iio/adc/ad7606.h
>> @@ -38,8 +38,19 @@
>>   	AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),\
>>   		0, BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO))
>>   
>> +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
> ID in chip info normally indicates something bad in the design. In that somewhere
> we have code that is ID dependent rather than all such code / data being
> found directly in this structure (or callbacks found from here).
> Can we avoid it here?

Hi Jonathan,

chip_info has to describe the chip hardwarewise, but there are different 
bops depending on the wiring (interface used, and backend/no backend).

The easiest way I found was to use the ID in a switch/case to 
determinate which bops I should take (well it was only needed in the spi 
version since it is the one supporting almost all the chips while the 
other ones still support only one). For instance, the ad7606B will use 
ad7606_bi_bops if it has a backend and ad7606B_spi_bops for spi version.

If I can't use the ID, the only way I see is creating 3 fields in 
chip_info (spi_ops, par_ops, backend_ops) and to initialize every 
chip_info structure with its associated op(s) for the associated 
interface. This would also lead to declare the different instances of 
ad7606_bus_ops directly in ad7606.h  (I dont like it very much but see 
no other option).

Do you think it's better that way ? Or do you have any other idea ?

Regards,

Guillaume

>
>>    * @channels:		channel specification
>>    * @num_channels:	number of channels
>>    * @oversampling_avail	pointer to the array which stores the available
>> @@ -50,6 +61,8 @@
> ...
>
>> diff --git a/drivers/iio/adc/ad7606_par.c b/drivers/iio/adc/ad7606_par.c
>> index d651639c45eb..7bac39033955 100644
>> --- a/drivers/iio/adc/ad7606_par.c
>> +++ b/drivers/iio/adc/ad7606_par.c
>> @@ -11,6 +11,7 @@
>>   #include <linux/mod_devicetable.h>
>>   #include <linux/module.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/property.h>
>>   #include <linux/types.h>
>>   
>>   #include <linux/iio/iio.h>
>> @@ -89,12 +90,20 @@ 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;
>> +	const struct platform_device_id *id;
>>   	struct resource *res;
>>   	void __iomem *addr;
>>   	resource_size_t remap_size;
>>   	int irq;
>>   
>> +	if (dev_fwnode(&pdev->dev)) {
>> +		chip_info = device_get_match_data(&pdev->dev);
>> +	} else {
>> +		id = platform_get_device_id(pdev);
>> +		chip_info = (const struct ad7606_chip_info *)id->driver_data;
>> +	}
>> +
>>   	irq = platform_get_irq(pdev, 0);
>>   	if (irq < 0)
>>   		return irq;
>> @@ -106,25 +115,25 @@ 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,
> Rewrap to move chip_info up a line perhaps.
>
>> +			    chip_info,
>>   			    remap_size > 1 ? &ad7606_par16_bops :
>>   			    &ad7606_par8_bops);

Re: [PATCH v2 07/10] iio: adc: ad7606: Add compatibility to fw_nodes

[Jonathan Cameron]

On Wed, 2 Oct 2024 02:12:28 +0200
Guillaume Stols <gstols@baylibre.com> wrote:

> On 9/29/24 14:44, Jonathan Cameron wrote:
> > On Fri, 20 Sep 2024 17:33:27 +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 fw_nodes, this commit intends to fix
> >> it.
> >>
> >> Also, chip info is moved in the .h file so to be accessible to all the  
> > chip info is not moved (I was going to say no to that) but an
> > extern is used to make it available. So say that rather than moved here.
> >  
> >> driver files that can set a pointer to the corresponding chip as the
> >> driver data.
> >>
> >>   
> >> diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
> >> index c13dda444526..18c87fe9a41a 100644
> >> --- a/drivers/iio/adc/ad7606.h
> >> +++ b/drivers/iio/adc/ad7606.h
> >> @@ -38,8 +38,19 @@
> >>   	AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),\
> >>   		0, BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO))
> >>   
> >> +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  
> > ID in chip info normally indicates something bad in the design. In that somewhere
> > we have code that is ID dependent rather than all such code / data being
> > found directly in this structure (or callbacks found from here).
> > Can we avoid it here?  
> 
> Hi Jonathan,
> 
> chip_info has to describe the chip hardwarewise, but there are different 
> bops depending on the wiring (interface used, and backend/no backend).

Normal solution to this is multiple chip specific structures so they
become specific to a chip + some wiring option. Then you just
pick between static const structures.

Does that work here?

You will need them exposed (extern) from your header but that's
not too bad.

Aim is to pick just one structure that describes all the 'specific'
stuff for the driver.  That brings all that stuff into one place and
provides an easy way to extend to new combinations of options for
other devices.

Jonathan


> 
> The easiest way I found was to use the ID in a switch/case to 
> determinate which bops I should take (well it was only needed in the spi 
> version since it is the one supporting almost all the chips while the 
> other ones still support only one). For instance, the ad7606B will use 
> ad7606_bi_bops if it has a backend and ad7606B_spi_bops for spi version.
> 
> If I can't use the ID, the only way I see is creating 3 fields in 
> chip_info (spi_ops, par_ops, backend_ops) and to initialize every 
> chip_info structure with its associated op(s) for the associated 
> interface. This would also lead to declare the different instances of 
> ad7606_bus_ops directly in ad7606.h  (I dont like it very much but see 
> no other option).
> 
> Do you think it's better that way ? Or do you have any other idea ?
> 
> Regards,
> 
> Guillaume
> 
> >  
> >>    * @channels:		channel specification
> >>    * @num_channels:	number of channels
> >>    * @oversampling_avail	pointer to the array which stores the available
> >> @@ -50,6 +61,8 @@  
> > ...
> >  
> >> diff --git a/drivers/iio/adc/ad7606_par.c b/drivers/iio/adc/ad7606_par.c
> >> index d651639c45eb..7bac39033955 100644
> >> --- a/drivers/iio/adc/ad7606_par.c
> >> +++ b/drivers/iio/adc/ad7606_par.c
> >> @@ -11,6 +11,7 @@
> >>   #include <linux/mod_devicetable.h>
> >>   #include <linux/module.h>
> >>   #include <linux/platform_device.h>
> >> +#include <linux/property.h>
> >>   #include <linux/types.h>
> >>   
> >>   #include <linux/iio/iio.h>
> >> @@ -89,12 +90,20 @@ 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;
> >> +	const struct platform_device_id *id;
> >>   	struct resource *res;
> >>   	void __iomem *addr;
> >>   	resource_size_t remap_size;
> >>   	int irq;
> >>   
> >> +	if (dev_fwnode(&pdev->dev)) {
> >> +		chip_info = device_get_match_data(&pdev->dev);
> >> +	} else {
> >> +		id = platform_get_device_id(pdev);
> >> +		chip_info = (const struct ad7606_chip_info *)id->driver_data;
> >> +	}
> >> +
> >>   	irq = platform_get_irq(pdev, 0);
> >>   	if (irq < 0)
> >>   		return irq;
> >> @@ -106,25 +115,25 @@ 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,  
> > Rewrap to move chip_info up a line perhaps.
> >  
> >> +			    chip_info,
> >>   			    remap_size > 1 ? &ad7606_par16_bops :
> >>   			    &ad7606_par8_bops);  
> 

Re: [PATCH v2 07/10] iio: adc: ad7606: Add compatibility to fw_nodes

[Guillaume Stols]

On 10/4/24 16:25, Jonathan Cameron wrote:
> On Wed, 2 Oct 2024 02:12:28 +0200
> Guillaume Stols <gstols@baylibre.com> wrote:
>
>> On 9/29/24 14:44, Jonathan Cameron wrote:
>>> On Fri, 20 Sep 2024 17:33:27 +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 fw_nodes, this commit intends to fix
>>>> it.
>>>>
>>>> Also, chip info is moved in the .h file so to be accessible to all the
>>> chip info is not moved (I was going to say no to that) but an
>>> extern is used to make it available. So say that rather than moved here.
>>>   
>>>> driver files that can set a pointer to the corresponding chip as the
>>>> driver data.
>>>>
>>>>    
>>>> diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
>>>> index c13dda444526..18c87fe9a41a 100644
>>>> --- a/drivers/iio/adc/ad7606.h
>>>> +++ b/drivers/iio/adc/ad7606.h
>>>> @@ -38,8 +38,19 @@
>>>>    	AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),\
>>>>    		0, BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO))
>>>>    
>>>> +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
>>> ID in chip info normally indicates something bad in the design. In that somewhere
>>> we have code that is ID dependent rather than all such code / data being
>>> found directly in this structure (or callbacks found from here).
>>> Can we avoid it here?
>> Hi Jonathan,
>>
>> chip_info has to describe the chip hardwarewise, but there are different
>> bops depending on the wiring (interface used, and backend/no backend).
> Normal solution to this is multiple chip specific structures so they
> become specific to a chip + some wiring option. Then you just
> pick between static const structures.
>
> Does that work here?
>
> You will need them exposed (extern) from your header but that's
> not too bad.
>
> Aim is to pick just one structure that describes all the 'specific'
> stuff for the driver.  That brings all that stuff into one place and
> provides an easy way to extend to new combinations of options for
> other devices.
>
> Jonathan

I finally wrote a structure containing the couple chip_info/bops

/**
  * struct ad7606_bus_info - agregate ad7606_chip_info and ad7606_bus_ops
  * @chip_info        entry in the table of chips that describes this device
  * @bops        bus operations (SPI or parallel)
  */
struct ad7606_bus_info {
     const struct ad7606_chip_info    *chip_info;
     const struct ad7606_bus_ops    *bops;
};

and declared the following way

const struct ad7606_bus_info ad7606b_bus_info = {
     .chip_info = &ad7606b_info,
     .bops = &ad7606b_spi_bops,
};

const struct ad7606_bus_info ad7606c_16_bus_info = {
     .chip_info = &ad7606c_16_info,
     .bops = &ad7606b_spi_bops,

etc...

Will send the series later today, just doing some last checks.

>
>
>> The easiest way I found was to use the ID in a switch/case to
>> determinate which bops I should take (well it was only needed in the spi
>> version since it is the one supporting almost all the chips while the
>> other ones still support only one). For instance, the ad7606B will use
>> ad7606_bi_bops if it has a backend and ad7606B_spi_bops for spi version.
>>
>> If I can't use the ID, the only way I see is creating 3 fields in
>> chip_info (spi_ops, par_ops, backend_ops) and to initialize every
>> chip_info structure with its associated op(s) for the associated
>> interface. This would also lead to declare the different instances of
>> ad7606_bus_ops directly in ad7606.h  (I dont like it very much but see
>> no other option).
>>
>> Do you think it's better that way ? Or do you have any other idea ?
>>
>> Regards,
>>
>> Guillaume
>>
>>>   
>>>>     * @channels:		channel specification
>>>>     * @num_channels:	number of channels
>>>>     * @oversampling_avail	pointer to the array which stores the available
>>>> @@ -50,6 +61,8 @@
>>> ...
>>>   
>>>> diff --git a/drivers/iio/adc/ad7606_par.c b/drivers/iio/adc/ad7606_par.c
>>>> index d651639c45eb..7bac39033955 100644
>>>> --- a/drivers/iio/adc/ad7606_par.c
>>>> +++ b/drivers/iio/adc/ad7606_par.c
>>>> @@ -11,6 +11,7 @@
>>>>    #include <linux/mod_devicetable.h>
>>>>    #include <linux/module.h>
>>>>    #include <linux/platform_device.h>
>>>> +#include <linux/property.h>
>>>>    #include <linux/types.h>
>>>>    
>>>>    #include <linux/iio/iio.h>
>>>> @@ -89,12 +90,20 @@ 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;
>>>> +	const struct platform_device_id *id;
>>>>    	struct resource *res;
>>>>    	void __iomem *addr;
>>>>    	resource_size_t remap_size;
>>>>    	int irq;
>>>>    
>>>> +	if (dev_fwnode(&pdev->dev)) {
>>>> +		chip_info = device_get_match_data(&pdev->dev);
>>>> +	} else {
>>>> +		id = platform_get_device_id(pdev);
>>>> +		chip_info = (const struct ad7606_chip_info *)id->driver_data;
>>>> +	}
>>>> +
>>>>    	irq = platform_get_irq(pdev, 0);
>>>>    	if (irq < 0)
>>>>    		return irq;
>>>> @@ -106,25 +115,25 @@ 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,
>>> Rewrap to move chip_info up a line perhaps.
>>>   
>>>> +			    chip_info,
>>>>    			    remap_size > 1 ? &ad7606_par16_bops :
>>>>    			    &ad7606_par8_bops);