[PATCH 0/8] iio: adc: dfsdm: add scaling support

[PATCH 0/8] iio: adc: dfsdm: add scaling support

[Olivier Moysan]

The aim of this serie is to add scaling support to STM32 DFSDM
peripheral in the analog context.

The DFSDM currently operates as a consumer of IIO channels
provided by a generic SD modulator. As previously discussed in RFC [1],
this topology is not suitable for implementing scaling.

This series brings the integration of the DFSDM driver with the new 
IIO backend framework [2], enabling the DFSDM IIO device to offer 
scaling feature based on reference voltage data obtained from the
IIO SD modulator backend. This generic SD modulator backend takes the
place of the former SD modulator, used with legacy implementation.

The DFSDM driver has been updated to adopt the generic ADC channel
binding [3]. The reasons for this include:
- Reducing the use of proprietary properties
- Simplifying the coexistence of legacy and new backend bindings
- Prepare the support of the MDF peripheral on STM32MP25 SoC

Backward compatibility is maintained through legacy support.

This series extends the backend framework with the following APIs 
(which may need further refinement):
- read_raw:
	This API is intented to retrieve the voltage information from the
	backend. It is based on IIO read_raw API, but maybe we need
	a more specialized API here.
- iio_backend_disable / iio_backend_enable:
	backend enable/disable to be used for PM management
- devm_iio_backend_subnode_get
	Intended for parsing DT subnodes to allow generic channel binding
	support, as generic channel DT nodes are not populated as devices.
	Ideally, a single API would be better, but I could not 
	identify a suitable alternative that doesn't affect the existing API
	(devm_iio_backend_get).

[1]: https://lore.kernel.org/lkml/20200204101008.11411-1-olivier.moysan@st.com/
[2]: https://lore.kernel.org/all/20240206-iio-backend-v9-0-df66d159c000@analog.com/
[3]: devicetree/bindings/iio/adc/adc.yaml

Olivier Moysan (8):
  iio: add read raw service to iio backend framework
  iio: add enable and disable services to iio backend framework
  iio: add child nodes support in iio backend framework
  dt-bindings: iio: dfsdm: move to backend framework
  dt-bindings: iio: add sigma delta modulator backend
  iio: adc: stm32-dfsdm: adopt generic channels bindings
  iio: add sd modulator generic iio backend
  iio: adc: stm32-dfsdm: add scaling support to dfsdm

 .../iio/adc/sd-modulator-backend.yaml         |  43 +++
 .../bindings/iio/adc/st,stm32-dfsdm-adc.yaml  | 158 ++++++++-
 drivers/iio/adc/Kconfig                       |  11 +
 drivers/iio/adc/Makefile                      |   1 +
 drivers/iio/adc/sd_adc_backend.c              | 110 +++++++
 drivers/iio/adc/stm32-dfsdm-adc.c             | 302 +++++++++++++++---
 drivers/iio/industrialio-backend.c            |  90 +++++-
 include/linux/iio/backend.h                   |   7 +
 8 files changed, 664 insertions(+), 58 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/sd-modulator-backend.yaml
 create mode 100644 drivers/iio/adc/sd_adc_backend.c


base-commit: 7db8a847f98caae68c70bdab9ba92d1af38e5656
-- 
2.25.1

[PATCH 1/8] iio: add read raw service to iio backend framework

[Olivier Moysan]

Add iio_backend_read_raw() service to support attributes read
from an IIO backend.

Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
---
 drivers/iio/industrialio-backend.c | 16 ++++++++++++++++
 include/linux/iio/backend.h        |  3 +++
 2 files changed, 19 insertions(+)

diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c
index 929aff4040ed..b950e30018ca 100644
--- a/drivers/iio/industrialio-backend.c
+++ b/drivers/iio/industrialio-backend.c
@@ -357,6 +357,22 @@ int devm_iio_backend_request_buffer(struct device *dev,
 }
 EXPORT_SYMBOL_NS_GPL(devm_iio_backend_request_buffer, IIO_BACKEND);
 
+/**
+ * iio_backend_read_raw - Request a value from the backend.
+ * @back:	Backend device
+ * @val:	First element of the returned value
+ * @val2:	Second element of the returned value
+ * @mask:	Specify value to retrieve
+ *
+ * RETURNS:
+ * 0 on success, negative error number on failure.
+ */
+int iio_backend_read_raw(struct iio_backend *back, int *val, int *val2, long mask)
+{
+	return iio_backend_op_call(back, read_raw, val, val2, mask);
+}
+EXPORT_SYMBOL_NS_GPL(iio_backend_read_raw, IIO_BACKEND);
+
 static struct iio_backend *iio_backend_from_indio_dev_parent(const struct device *dev)
 {
 	struct iio_backend *back = ERR_PTR(-ENODEV), *iter;
diff --git a/include/linux/iio/backend.h b/include/linux/iio/backend.h
index 8099759d7242..cff486699054 100644
--- a/include/linux/iio/backend.h
+++ b/include/linux/iio/backend.h
@@ -81,6 +81,7 @@ enum iio_backend_sample_trigger {
  * @extend_chan_spec: Extend an IIO channel.
  * @ext_info_set: Extended info setter.
  * @ext_info_get: Extended info getter.
+ * @read_raw: Read value from a backend device
  **/
 struct iio_backend_ops {
 	int (*enable)(struct iio_backend *back);
@@ -113,6 +114,7 @@ struct iio_backend_ops {
 			    const char *buf, size_t len);
 	int (*ext_info_get)(struct iio_backend *back, uintptr_t private,
 			    const struct iio_chan_spec *chan, char *buf);
+	int (*read_raw)(struct iio_backend *back, int *val, int *val2, long mask);
 };
 
 int iio_backend_chan_enable(struct iio_backend *back, unsigned int chan);
@@ -141,6 +143,7 @@ ssize_t iio_backend_ext_info_set(struct iio_dev *indio_dev, uintptr_t private,
 				 const char *buf, size_t len);
 ssize_t iio_backend_ext_info_get(struct iio_dev *indio_dev, uintptr_t private,
 				 const struct iio_chan_spec *chan, char *buf);
+int iio_backend_read_raw(struct iio_backend *back, int *val, int *val2, long mask);
 
 int iio_backend_extend_chan_spec(struct iio_dev *indio_dev,
 				 struct iio_backend *back,
-- 
2.25.1

[PATCH 2/8] iio: add enable and disable services to iio backend framework

[Olivier Moysan]

Add iio_backend_disable() and iio_backend_enable() APIs to allow
IIO backend consumer to request backend disabling and enabling.

Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
---
 drivers/iio/industrialio-backend.c | 26 ++++++++++++++++++++++++++
 include/linux/iio/backend.h        |  2 ++
 2 files changed, 28 insertions(+)

diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c
index b950e30018ca..d3db048c086b 100644
--- a/drivers/iio/industrialio-backend.c
+++ b/drivers/iio/industrialio-backend.c
@@ -166,6 +166,32 @@ int devm_iio_backend_enable(struct device *dev, struct iio_backend *back)
 }
 EXPORT_SYMBOL_NS_GPL(devm_iio_backend_enable, IIO_BACKEND);
 
+/**
+ * iio_backend_enable - Backend enable
+ * @dev: Consumer device for the backend
+ * @back: Backend device
+ *
+ * RETURNS:
+ * 0 on success, negative error number on failure.
+ */
+int iio_backend_enable(struct device *dev, struct iio_backend *back)
+{
+	return iio_backend_op_call(back, enable);
+}
+EXPORT_SYMBOL_NS_GPL(iio_backend_enable, IIO_BACKEND);
+
+/**
+ * iio_backend_disable - Backend disable
+ * @dev: Consumer device for the backend
+ * @back: Backend device
+ *
+ */
+void iio_backend_disable(struct device *dev, struct iio_backend *back)
+{
+	iio_backend_void_op_call(back, disable);
+}
+EXPORT_SYMBOL_NS_GPL(iio_backend_disable, IIO_BACKEND);
+
 /**
  * iio_backend_data_format_set - Configure the channel data format
  * @back: Backend device
diff --git a/include/linux/iio/backend.h b/include/linux/iio/backend.h
index cff486699054..81277e5b6160 100644
--- a/include/linux/iio/backend.h
+++ b/include/linux/iio/backend.h
@@ -120,6 +120,8 @@ struct iio_backend_ops {
 int iio_backend_chan_enable(struct iio_backend *back, unsigned int chan);
 int iio_backend_chan_disable(struct iio_backend *back, unsigned int chan);
 int devm_iio_backend_enable(struct device *dev, struct iio_backend *back);
+int iio_backend_enable(struct device *dev, struct iio_backend *back);
+void iio_backend_disable(struct device *dev, struct iio_backend *back);
 int iio_backend_data_format_set(struct iio_backend *back, unsigned int chan,
 				const struct iio_backend_data_fmt *data);
 int iio_backend_data_source_set(struct iio_backend *back, unsigned int chan,
-- 
2.25.1

[PATCH 3/8] iio: add child nodes support in iio backend framework

[Olivier Moysan]

Add an API to support IIO generic channels binding:
http://devicetree.org/schemas/iio/adc/adc.yaml#
This new API is needed, as generic channel DT node
isn't populated as a device.
Add devm_iio_backend_subnode_get() to allow an IIO device
backend consumer to configure backend phandles in its
child nodes.

Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
---
 drivers/iio/industrialio-backend.c | 48 ++++++++++++++++++++++--------
 include/linux/iio/backend.h        |  2 ++
 2 files changed, 38 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c
index d3db048c086b..e9d29131634d 100644
--- a/drivers/iio/industrialio-backend.c
+++ b/drivers/iio/industrialio-backend.c
@@ -575,17 +575,8 @@ static int __devm_iio_backend_get(struct device *dev, struct iio_backend *back)
 	return 0;
 }
 
-/**
- * devm_iio_backend_get - Device managed backend device get
- * @dev: Consumer device for the backend
- * @name: Backend name
- *
- * Get's the backend associated with @dev.
- *
- * RETURNS:
- * A backend pointer, negative error pointer otherwise.
- */
-struct iio_backend *devm_iio_backend_get(struct device *dev, const char *name)
+static struct iio_backend *__devm_iio_backend_node_get(struct device *dev, const char *name,
+						       struct fwnode_handle *node)
 {
 	struct fwnode_handle *fwnode;
 	struct iio_backend *back;
@@ -602,7 +593,7 @@ struct iio_backend *devm_iio_backend_get(struct device *dev, const char *name)
 		index = 0;
 	}
 
-	fwnode = fwnode_find_reference(dev_fwnode(dev), "io-backends", index);
+	fwnode = fwnode_find_reference(node, "io-backends", index);
 	if (IS_ERR(fwnode)) {
 		dev_err_probe(dev, PTR_ERR(fwnode),
 			      "Cannot get Firmware reference\n");
@@ -625,8 +616,41 @@ struct iio_backend *devm_iio_backend_get(struct device *dev, const char *name)
 	fwnode_handle_put(fwnode);
 	return ERR_PTR(-EPROBE_DEFER);
 }
+
+/**
+ * devm_iio_backend_get - Device managed backend device get
+ * @dev: Consumer device for the backend
+ * @name: Backend name
+ *
+ * Get's the backend associated with @dev.
+ *
+ * RETURNS:
+ * A backend pointer, negative error pointer otherwise.
+ */
+struct iio_backend *devm_iio_backend_get(struct device *dev, const char *name)
+{
+	return __devm_iio_backend_node_get(dev, name, dev_fwnode(dev));
+}
 EXPORT_SYMBOL_NS_GPL(devm_iio_backend_get, IIO_BACKEND);
 
+/**
+ * devm_iio_backend_subnode_get - Device managed backend device get
+ * @dev: Consumer device for the backend
+ * @name: Backend name
+ * @node: Firmware node of the backend consumer
+ *
+ * Get's the backend associated with @dev.
+ *
+ * RETURNS:
+ * A backend pointer, negative error pointer otherwise.
+ */
+struct iio_backend *devm_iio_backend_subnode_get(struct device *dev, const char *name,
+						 struct fwnode_handle *node)
+{
+	return __devm_iio_backend_node_get(dev, name, node);
+}
+EXPORT_SYMBOL_NS_GPL(devm_iio_backend_subnode_get, IIO_BACKEND);
+
 /**
  * __devm_iio_backend_get_from_fwnode_lookup - Device managed fwnode backend device get
  * @dev: Consumer device for the backend
diff --git a/include/linux/iio/backend.h b/include/linux/iio/backend.h
index 81277e5b6160..1050ab68e4e4 100644
--- a/include/linux/iio/backend.h
+++ b/include/linux/iio/backend.h
@@ -152,6 +152,8 @@ int iio_backend_extend_chan_spec(struct iio_dev *indio_dev,
 				 struct iio_chan_spec *chan);
 void *iio_backend_get_priv(const struct iio_backend *conv);
 struct iio_backend *devm_iio_backend_get(struct device *dev, const char *name);
+struct iio_backend *devm_iio_backend_subnode_get(struct device *dev, const char *name,
+						 struct fwnode_handle *node);
 struct iio_backend *
 __devm_iio_backend_get_from_fwnode_lookup(struct device *dev,
 					  struct fwnode_handle *fwnode);
-- 
2.25.1

[PATCH 4/8] dt-bindings: iio: dfsdm: move to backend framework

[Olivier Moysan]

Change the DFSDM binding to use the new IIO backend framework,
along with the adoption of IIO generic channels.
This binding change allows to add scaling support to the DFSDM.

Keep the legacy binding as deprecated for backward compatibility.

The io-backends property is supported only in generic IIO channel
binding.

- Channel description with the generic binding (Audio and Analog):

  Properties supersed by generic properties:
    st,adc-channels: becomes "reg" property in channel node
    st,adc-channel-names: becomes "label" property in channel node
  Properties moved to channel child node:
    st,adc-channel-types, st,adc-channel-clk-src, st,adc-alt-channel

- Analog binding:

  DFSDM filter channel is configured as an IIO backend consumer.
  Add io-backends property in channel child nodes.

  DFSDM is no more configured as a channel consumer from SD modulator.
  Use of io-channels in DFSDM node is deprecated.

- Audio binding:

  DFSDM audio DAI is configured as a channel consumer from DFSDM filter.
  No change compare to legacy.

Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
---
 .../bindings/iio/adc/st,stm32-dfsdm-adc.yaml  | 158 +++++++++++++++++-
 1 file changed, 152 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/adc/st,stm32-dfsdm-adc.yaml b/Documentation/devicetree/bindings/iio/adc/st,stm32-dfsdm-adc.yaml
index c1b1324fa132..dd414bab74c1 100644
--- a/Documentation/devicetree/bindings/iio/adc/st,stm32-dfsdm-adc.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/st,stm32-dfsdm-adc.yaml
@@ -102,9 +102,11 @@ patternProperties:
         items:
           minimum: 0
           maximum: 7
+        deprecated: true
 
       st,adc-channel-names:
         description: List of single-ended channel names.
+        deprecated: true
 
       st,filter-order:
         description: |
@@ -118,6 +120,12 @@ patternProperties:
       "#io-channel-cells":
         const: 1
 
+      '#address-cells':
+        const: 1
+
+      '#size-cells':
+        const: 0
+
       st,adc-channel-types:
         description: |
           Single-ended channel input type.
@@ -128,6 +136,7 @@ patternProperties:
         items:
           enum: [ SPI_R, SPI_F, MANCH_R, MANCH_F ]
         $ref: /schemas/types.yaml#/definitions/non-unique-string-array
+        deprecated: true
 
       st,adc-channel-clk-src:
         description: |
@@ -139,6 +148,7 @@ patternProperties:
         items:
           enum: [ CLKIN, CLKOUT, CLKOUT_F, CLKOUT_R ]
         $ref: /schemas/types.yaml#/definitions/non-unique-string-array
+        deprecated: true
 
       st,adc-alt-channel:
         description:
@@ -147,6 +157,7 @@ patternProperties:
           If not set, channel n is connected to SPI input n.
           If set, channel n is connected to SPI input n + 1.
         type: boolean
+        deprecated: true
 
       st,filter0-sync:
         description:
@@ -165,11 +176,65 @@ patternProperties:
       - compatible
       - reg
       - interrupts
-      - st,adc-channels
-      - st,adc-channel-names
       - st,filter-order
       - "#io-channel-cells"
 
+    patternProperties:
+      "^channel@([0-9]|1[0-9])$":
+        type: object
+        $ref: adc.yaml
+        description: Represents the external channels which are connected to the DFSDM.
+
+        properties:
+          reg:
+            items:
+              minimum: 0
+              maximum: 8
+
+          label:
+            description:
+              Unique name to identify which channel this is.
+
+          st,adc-channel-types:
+            description: |
+              Single-ended channel input type.
+              - "SPI_R": SPI with data on rising edge (default)
+              - "SPI_F": SPI with data on falling edge
+              - "MANCH_R": manchester codec, rising edge = logic 0, falling edge = logic 1
+              - "MANCH_F": manchester codec, rising edge = logic 1, falling edge = logic 0
+            items:
+              enum: [ SPI_R, SPI_F, MANCH_R, MANCH_F ]
+            $ref: /schemas/types.yaml#/definitions/non-unique-string-array
+
+          st,adc-channel-clk-src:
+            description: |
+              Conversion clock source.
+              - "CLKIN": external SPI clock (CLKIN x)
+              - "CLKOUT": internal SPI clock (CLKOUT) (default)
+              - "CLKOUT_F": internal SPI clock divided by 2 (falling edge).
+              - "CLKOUT_R": internal SPI clock divided by 2 (rising edge).
+            items:
+              enum: [ CLKIN, CLKOUT, CLKOUT_F, CLKOUT_R ]
+            $ref: /schemas/types.yaml#/definitions/non-unique-string-array
+
+          st,adc-alt-channel:
+            description:
+              Must be defined if two sigma delta modulators are
+              connected on same SPI input.
+              If not set, channel n is connected to SPI input n.
+              If set, channel n is connected to SPI input n + 1.
+            type: boolean
+
+          io-backends:
+            description:
+              From common IIO binding. Used to pipe external sigma delta
+              modulator or internal ADC backend to DFSDM channel.
+
+        required:
+          - reg
+
+        additionalProperties: false
+
     allOf:
       - if:
           properties:
@@ -199,9 +264,19 @@ patternProperties:
               description:
                 From common IIO binding. Used to pipe external sigma delta
                 modulator or internal ADC output to DFSDM channel.
+              deprecated: true
 
-          required:
-            - io-channels
+          if:
+            required:
+              - st,adc-channels
+          then:
+            required:
+              - io-channels
+
+          patternProperties:
+            "^channel@([0-9]|1[0-9])$":
+              required:
+                - io-backends
 
       - if:
           properties:
@@ -294,7 +369,77 @@ examples:
       #address-cells = <1>;
       #size-cells = <0>;
 
+      // Example 1: Audio use case with generic binding
       dfsdm0: filter@0 {
+        compatible = "st,stm32-dfsdm-dmic";
+        reg = <0>;
+        interrupts = <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
+        dmas = <&dmamux1 101 0x400 0x01>;
+        dma-names = "rx";
+        #io-channel-cells = <1>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+        st,filter-order = <5>;
+
+        channel@1 {
+          reg = <1>;
+          label = "dmic0";
+          st,adc-channel-types = "SPI_R";
+          st,adc-channel-clk-src = "CLKOUT";
+          st,adc-alt-channel;
+        };
+
+        asoc_pdm0: dfsdm-dai {
+          compatible = "st,stm32h7-dfsdm-dai";
+          #sound-dai-cells = <0>;
+          io-channels = <&dfsdm0 0>;
+        };
+      };
+
+      // Example 1: Analog use case with generic binding
+      dfsdm1: filter@1 {
+        compatible = "st,stm32-dfsdm-adc";
+        reg = <1>;
+        interrupts = <GIC_SPI 111 IRQ_TYPE_LEVEL_HIGH>;
+        dmas = <&dmamux1 102 0x400 0x01>;
+        dma-names = "rx";
+        st,filter-order = <1>;
+        #io-channel-cells = <1>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        channel@2 {
+          reg = <2>;
+          label = "in2";
+          st,adc-channel-types = "SPI_F";
+          st,adc-channel-clk-src = "CLKOUT";
+          st,adc-alt-channel;
+          io-backends = <&sd_adc2>;
+        };
+
+        channel@3 {
+          reg = <3>;
+          label = "in3";
+          st,adc-channel-types = "SPI_R";
+          st,adc-channel-clk-src = "CLKOUT";
+          io-backends = <&sd_adc3>;
+        };
+      };
+    };
+
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/clock/stm32mp1-clks.h>
+    dfsdm_2: dfsdm@4400d000 {
+      compatible = "st,stm32mp1-dfsdm";
+      reg = <0x4400d000 0x800>;
+      clocks = <&rcc DFSDM_K>, <&rcc ADFSDM_K>;
+      clock-names = "dfsdm", "audio";
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      // Example 3: Audio use case with legacy binding
+      dfsdm0_2: filter@0 {
         compatible = "st,stm32-dfsdm-dmic";
         reg = <0>;
         interrupts = <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
@@ -307,14 +452,15 @@ examples:
         st,adc-channel-clk-src = "CLKOUT";
         st,filter-order = <5>;
 
-        asoc_pdm0: dfsdm-dai {
+        asoc_pdm0_2: dfsdm-dai {
           compatible = "st,stm32h7-dfsdm-dai";
           #sound-dai-cells = <0>;
           io-channels = <&dfsdm0 0>;
         };
       };
 
-      dfsdm_pdm1: filter@1 {
+      // Example 3: Analog use case with legacy binding
+      dfsdm1_2: filter@1 {
         compatible = "st,stm32-dfsdm-adc";
         reg = <1>;
         interrupts = <GIC_SPI 111 IRQ_TYPE_LEVEL_HIGH>;
-- 
2.25.1

[PATCH 5/8] dt-bindings: iio: add sigma delta modulator backend

[Olivier Moysan]

Add documentation of device tree bindings to support
sigma delta modulator backend in IIO framework.

Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
---
 .../iio/adc/sd-modulator-backend.yaml         | 43 +++++++++++++++++++
 1 file changed, 43 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/sd-modulator-backend.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/sd-modulator-backend.yaml b/Documentation/devicetree/bindings/iio/adc/sd-modulator-backend.yaml
new file mode 100644
index 000000000000..b0fa71b75cd0
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/sd-modulator-backend.yaml
@@ -0,0 +1,43 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/sd-modulator-backend.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sigma delta modulator backend
+
+maintainers:
+  - Olivier Moysan <olivier.moysan@foss.st.com>
+
+properties:
+  compatible:
+    description: |
+      "sd-backend" can be used as a generic SD modulator backend,
+      if the modulator is not specified in the compatible list.
+    enum:
+      - sd-backend
+      - ads1201
+
+  '#io-backend-cells':
+    const: 0
+
+  reg:
+    maxItems: 1
+
+  vref-supply:
+    description: Phandle to the vref input analog reference voltage.
+
+required:
+  - compatible
+  - '#io-backend-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    ads1201: adc {
+      compatible = "sd-backend";
+      #io-backend-cells = <0>;
+    };
+
+...
-- 
2.25.1

[PATCH 6/8] iio: adc: stm32-dfsdm: adopt generic channels bindings

[Olivier Moysan]

Move to generic channels binding to ease new backend framework adoption
and prepare the convergence with MDF IP support on STM32MP2 SoC family.

Legacy binding:
DFSDM is an IIO channel consumer.
SD modulator is an IIO channels provider.
The channel phandles are provided in DT through io-channels property
and channel indexes through st,adc-channels property.

New binding:
DFSDM is an IIO channel provider.
The channel indexes are given by reg property in channel child node.

This new binding is intended to be used with SD modulator IIO backends.
It does not support SD modulator legacy IIO devices.
The st,adc-channels property presence is used to discriminate
between legacy and backend bindings.

The support of the DFSDM legacy channels and SD modulator IIO devices
is kept for backward compatibility.

Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
---
 drivers/iio/adc/stm32-dfsdm-adc.c | 208 ++++++++++++++++++++++++------
 1 file changed, 171 insertions(+), 37 deletions(-)

diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
index 9a47d2c87f05..69b4764d7cba 100644
--- a/drivers/iio/adc/stm32-dfsdm-adc.c
+++ b/drivers/iio/adc/stm32-dfsdm-adc.c
@@ -666,6 +666,64 @@ static int stm32_dfsdm_channel_parse_of(struct stm32_dfsdm *dfsdm,
 	return 0;
 }
 
+static int stm32_dfsdm_generic_channel_parse_of(struct stm32_dfsdm *dfsdm,
+						struct iio_dev *indio_dev,
+						struct iio_chan_spec *ch,
+						struct fwnode_handle *node)
+{
+	struct stm32_dfsdm_channel *df_ch;
+	const char *of_str;
+	int ret, val;
+
+	ret = fwnode_property_read_u32(node, "reg", &ch->channel);
+	if (ret < 0) {
+		dev_err(&indio_dev->dev, "Missing channel index %d\n", ret);
+		return ret;
+	}
+
+	if (ch->channel >= dfsdm->num_chs) {
+		dev_err(&indio_dev->dev, " Error bad channel number %d (max = %d)\n",
+			ch->channel, dfsdm->num_chs);
+		return -EINVAL;
+	}
+
+	ret = fwnode_property_read_string(node, "label", &ch->datasheet_name);
+	if (ret < 0) {
+		dev_err(&indio_dev->dev,
+			" Error parsing 'label' for idx %d\n", ch->channel);
+		return ret;
+	}
+
+	df_ch =  &dfsdm->ch_list[ch->channel];
+	df_ch->id = ch->channel;
+
+	ret = fwnode_property_read_string(node, "st,adc-channel-types", &of_str);
+	if (!ret) {
+		val = stm32_dfsdm_str2val(of_str, stm32_dfsdm_chan_type);
+		if (val < 0)
+			return val;
+	} else {
+		val = 0;
+	}
+	df_ch->type = val;
+
+	ret = fwnode_property_read_string(node, "st,adc-channel-clk-src", &of_str);
+	if (!ret) {
+		val = stm32_dfsdm_str2val(of_str, stm32_dfsdm_chan_src);
+		if (val < 0)
+			return val;
+	} else {
+		val = 0;
+	}
+	df_ch->src = val;
+
+	ret = fwnode_property_read_u32(node, "st,adc-alt-channel", &df_ch->alt_si);
+	if (ret != -EINVAL)
+		df_ch->alt_si = 0;
+
+	return 0;
+}
+
 static ssize_t dfsdm_adc_audio_get_spiclk(struct iio_dev *indio_dev,
 					  uintptr_t priv,
 					  const struct iio_chan_spec *chan,
@@ -1231,7 +1289,8 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
 		ret = iio_device_claim_direct_mode(indio_dev);
 		if (ret)
 			return ret;
-		ret = iio_hw_consumer_enable(adc->hwc);
+		if (adc->hwc)
+			ret = iio_hw_consumer_enable(adc->hwc);
 		if (ret < 0) {
 			dev_err(&indio_dev->dev,
 				"%s: IIO enable failed (channel %d)\n",
@@ -1240,7 +1299,8 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
 			return ret;
 		}
 		ret = stm32_dfsdm_single_conv(indio_dev, chan, val);
-		iio_hw_consumer_disable(adc->hwc);
+		if (adc->hwc)
+			iio_hw_consumer_disable(adc->hwc);
 		if (ret < 0) {
 			dev_err(&indio_dev->dev,
 				"%s: Conversion failed (channel %d)\n",
@@ -1367,15 +1427,20 @@ static int stm32_dfsdm_dma_request(struct device *dev,
 	return 0;
 }
 
-static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev,
-					 struct iio_chan_spec *ch)
+static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev, struct iio_chan_spec *ch,
+					 struct fwnode_handle *child)
 {
 	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
 	int ret;
 
-	ret = stm32_dfsdm_channel_parse_of(adc->dfsdm, indio_dev, ch);
-	if (ret < 0)
+	if (child)
+		ret = stm32_dfsdm_generic_channel_parse_of(adc->dfsdm, indio_dev, ch, child);
+	else /* Legacy binding */
+		ret = stm32_dfsdm_channel_parse_of(adc->dfsdm, indio_dev, ch);
+	if (ret < 0) {
+		dev_err(&indio_dev->dev, "Failed to parse channel\n");
 		return ret;
+	}
 
 	ch->type = IIO_VOLTAGE;
 	ch->indexed = 1;
@@ -1390,6 +1455,7 @@ static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev,
 
 	if (adc->dev_data->type == DFSDM_AUDIO) {
 		ch->ext_info = dfsdm_adc_audio_ext_info;
+		ch->scan_index = 0;
 	} else {
 		ch->scan_type.shift = 8;
 	}
@@ -1397,8 +1463,51 @@ static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev,
 	ch->scan_type.realbits = 24;
 	ch->scan_type.storagebits = 32;
 
-	return stm32_dfsdm_chan_configure(adc->dfsdm,
-					  &adc->dfsdm->ch_list[ch->channel]);
+	return stm32_dfsdm_chan_configure(adc->dfsdm, &adc->dfsdm->ch_list[ch->channel]);
+}
+
+static int stm32_dfsdm_chan_init(struct iio_dev *indio_dev, struct iio_chan_spec *channels)
+{
+	int num_ch = indio_dev->num_channels;
+	int chan_idx = 0, ret = 0;
+
+	for (chan_idx = 0; chan_idx < num_ch; chan_idx++) {
+		channels[chan_idx].scan_index = chan_idx;
+		ret = stm32_dfsdm_adc_chan_init_one(indio_dev, &channels[chan_idx], NULL);
+		if (ret < 0) {
+			dev_err(&indio_dev->dev, "Channels init failed\n");
+			return ret;
+		}
+	}
+
+	return ret;
+}
+
+static int stm32_dfsdm_generic_chan_init(struct iio_dev *indio_dev, struct iio_chan_spec *channels)
+{
+	struct fwnode_handle *child;
+	int chan_idx = 0, ret;
+
+	device_for_each_child_node(&indio_dev->dev, child) {
+		/* Skip DAI node in DFSDM audio nodes */
+		if (fwnode_property_present(child, "compatible"))
+			continue;
+
+		channels[chan_idx].scan_index = chan_idx;
+		ret = stm32_dfsdm_adc_chan_init_one(indio_dev, &channels[chan_idx], child);
+		if (ret < 0) {
+			dev_err(&indio_dev->dev, "Channels init failed\n");
+			goto err;
+		}
+
+		chan_idx++;
+	}
+	return chan_idx;
+
+err:
+	fwnode_handle_put(child);
+
+	return ret;
 }
 
 static int stm32_dfsdm_audio_init(struct device *dev, struct iio_dev *indio_dev)
@@ -1406,15 +1515,26 @@ static int stm32_dfsdm_audio_init(struct device *dev, struct iio_dev *indio_dev)
 	struct iio_chan_spec *ch;
 	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
 	struct stm32_dfsdm_channel *d_ch;
-	int ret;
+	bool legacy = false;
+	int num_ch, ret;
+
+	/* If st,adc-channels is defined legacy binding is used. Else assume generic binding. */
+	num_ch = of_property_count_u32_elems(indio_dev->dev.of_node, "st,adc-channels");
+	if (num_ch == 1)
+		legacy = true;
 
 	ch = devm_kzalloc(&indio_dev->dev, sizeof(*ch), GFP_KERNEL);
 	if (!ch)
 		return -ENOMEM;
 
-	ch->scan_index = 0;
+	indio_dev->num_channels = 1;
+	indio_dev->channels = ch;
+
+	if (legacy)
+		ret = stm32_dfsdm_chan_init(indio_dev, ch);
+	else
+		ret = stm32_dfsdm_generic_chan_init(indio_dev, ch);
 
-	ret = stm32_dfsdm_adc_chan_init_one(indio_dev, ch);
 	if (ret < 0) {
 		dev_err(&indio_dev->dev, "Channels init failed\n");
 		return ret;
@@ -1425,9 +1545,6 @@ static int stm32_dfsdm_audio_init(struct device *dev, struct iio_dev *indio_dev)
 	if (d_ch->src != DFSDM_CHANNEL_SPI_CLOCK_EXTERNAL)
 		adc->spi_freq = adc->dfsdm->spi_master_freq;
 
-	indio_dev->num_channels = 1;
-	indio_dev->channels = ch;
-
 	return stm32_dfsdm_dma_request(dev, indio_dev);
 }
 
@@ -1435,43 +1552,60 @@ static int stm32_dfsdm_adc_init(struct device *dev, struct iio_dev *indio_dev)
 {
 	struct iio_chan_spec *ch;
 	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
-	int num_ch;
-	int ret, chan_idx;
+	int num_ch, ret;
+	bool legacy = false;
 
 	adc->oversamp = DFSDM_DEFAULT_OVERSAMPLING;
 	ret = stm32_dfsdm_compute_all_osrs(indio_dev, adc->oversamp);
 	if (ret < 0)
 		return ret;
 
-	num_ch = of_property_count_u32_elems(indio_dev->dev.of_node,
-					     "st,adc-channels");
-	if (num_ch < 0 || num_ch > adc->dfsdm->num_chs) {
-		dev_err(&indio_dev->dev, "Bad st,adc-channels\n");
-		return num_ch < 0 ? num_ch : -EINVAL;
+	num_ch = device_get_child_node_count(&indio_dev->dev);
+	if (!num_ch) {
+		/* No channels nodes found. Assume legacy binding */
+		num_ch = of_property_count_u32_elems(indio_dev->dev.of_node, "st,adc-channels");
+		if (num_ch < 0) {
+			dev_err(&indio_dev->dev, "Bad st,adc-channels\n");
+			return num_ch;
+		}
+
+		legacy = true;
 	}
 
-	/* Bind to SD modulator IIO device */
-	adc->hwc = devm_iio_hw_consumer_alloc(&indio_dev->dev);
-	if (IS_ERR(adc->hwc))
-		return -EPROBE_DEFER;
+	if (num_ch > adc->dfsdm->num_chs) {
+		dev_err(&indio_dev->dev, "Number of channel [%d] exceeds [%d]\n",
+			num_ch, adc->dfsdm->num_chs);
+		return -EINVAL;
+	}
+	indio_dev->num_channels = num_ch;
 
-	ch = devm_kcalloc(&indio_dev->dev, num_ch, sizeof(*ch),
-			  GFP_KERNEL);
-	if (!ch)
-		return -ENOMEM;
+	if (legacy) {
+		/* Bind to SD modulator IIO device. */
+		adc->hwc = devm_iio_hw_consumer_alloc(&indio_dev->dev);
+		if (IS_ERR(adc->hwc))
+			return -EPROBE_DEFER;
+	} else {
+		/* Generic binding. SD modulator IIO device not used. Use SD modulator backend. */
+		adc->hwc = NULL;
 
-	for (chan_idx = 0; chan_idx < num_ch; chan_idx++) {
-		ch[chan_idx].scan_index = chan_idx;
-		ret = stm32_dfsdm_adc_chan_init_one(indio_dev, &ch[chan_idx]);
-		if (ret < 0) {
-			dev_err(&indio_dev->dev, "Channels init failed\n");
-			return ret;
-		}
+		adc->backend = devm_kzalloc(&indio_dev->dev, sizeof(*adc->backend) * num_ch,
+					    GFP_KERNEL);
+		if (!adc->backend)
+			return -ENOMEM;
 	}
 
-	indio_dev->num_channels = num_ch;
+	ch = devm_kcalloc(&indio_dev->dev, num_ch, sizeof(*ch), GFP_KERNEL);
+	if (!ch)
+		return -ENOMEM;
 	indio_dev->channels = ch;
 
+	if (legacy)
+		ret = stm32_dfsdm_chan_init(indio_dev, ch);
+	else
+		ret = stm32_dfsdm_generic_chan_init(indio_dev, ch);
+	if (ret < 0)
+		return ret;
+
 	init_completion(&adc->completion);
 
 	/* Optionally request DMA */
-- 
2.25.1

[PATCH 7/8] iio: add sd modulator generic iio backend

[Olivier Moysan]

Add a generic driver to support sigma delta modulators.
Typically, this device is a hardware connected to an IIO device
in charge of the conversion. The device is exposed as an IIO backend
device. This backend device and the associated conversion device
can be seen as an aggregate device from IIO framework.

Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
---
 drivers/iio/adc/Kconfig          |  10 +++
 drivers/iio/adc/Makefile         |   1 +
 drivers/iio/adc/sd_adc_backend.c | 110 +++++++++++++++++++++++++++++++
 3 files changed, 121 insertions(+)
 create mode 100644 drivers/iio/adc/sd_adc_backend.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 3d91015af6ea..f3dfdaa80678 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1155,6 +1155,16 @@ config SPEAR_ADC
 	  To compile this driver as a module, choose M here: the
 	  module will be called spear_adc.
 
+config SD_ADC_BACKEND
+	tristate "Generic sigma delta modulator IIO backend"
+	select IIO_BACKEND
+	help
+	  Select this option to enables sigma delta modulator. This driver can
+	  support generic sigma delta modulators, as IIO backend devices.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called sd_adc_backend.
+
 config SD_ADC_MODULATOR
 	tristate "Generic sigma delta modulator"
 	select IIO_BUFFER
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 37ac689a0209..9aee2e4307d7 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -139,3 +139,4 @@ obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
 obj-$(CONFIG_XILINX_AMS) += xilinx-ams.o
 xilinx-xadc-y := xilinx-xadc-core.o xilinx-xadc-events.o
 obj-$(CONFIG_XILINX_XADC) += xilinx-xadc.o
+obj-$(CONFIG_SD_ADC_BACKEND) += sd_adc_backend.o
diff --git a/drivers/iio/adc/sd_adc_backend.c b/drivers/iio/adc/sd_adc_backend.c
new file mode 100644
index 000000000000..556a49dc537b
--- /dev/null
+++ b/drivers/iio/adc/sd_adc_backend.c
@@ -0,0 +1,110 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Generic sigma delta modulator IIO backend
+ *
+ * Copyright (C) 2024, STMicroelectronics - All Rights Reserved
+ */
+
+#include <linux/iio/backend.h>
+#include <linux/iio/iio.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+
+struct iio_sd_backend_priv {
+	struct regulator *vref;
+	int vref_mv;
+};
+
+static int sd_backend_enable(struct iio_backend *backend)
+{
+	struct iio_sd_backend_priv *priv = iio_backend_get_priv(backend);
+
+	return regulator_enable(priv->vref);
+};
+
+static void sd_backend_disable(struct iio_backend *backend)
+{
+	struct iio_sd_backend_priv *priv = iio_backend_get_priv(backend);
+
+	regulator_disable(priv->vref);
+};
+
+static int sd_backend_read(struct iio_backend *backend, int *val, int *val2, long mask)
+{
+	struct iio_sd_backend_priv *priv = iio_backend_get_priv(backend);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		*val = priv->vref_mv;
+		*val2 = 0;
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_OFFSET:
+		*val = 0;
+		*val2 = 0;
+		return IIO_VAL_INT;
+	}
+
+	return -EINVAL;
+};
+
+static const struct iio_backend_ops sd_backend_ops = {
+	.enable = sd_backend_enable,
+	.disable = sd_backend_disable,
+	.read_raw = sd_backend_read,
+};
+
+static int iio_sd_backend_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct regulator *vref;
+	struct iio_sd_backend_priv *priv;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	vref = devm_regulator_get_optional(dev, "vref");
+	if (IS_ERR(vref)) {
+		if (PTR_ERR(vref) != -ENODEV)
+			return dev_err_probe(dev, PTR_ERR(vref), "Failed to get vref\n");
+	} else {
+		ret = regulator_get_voltage(vref);
+		if (ret < 0)
+			return ret;
+
+		priv->vref = vref;
+		priv->vref_mv = ret / 1000;
+	}
+
+	ret = devm_iio_backend_register(&pdev->dev, &sd_backend_ops, priv);
+	if (ret)
+		return ret;
+
+	return 0;
+};
+
+static const struct of_device_id sd_backend_of_match[] = {
+	{ .compatible = "sd-backend" },
+	{ .compatible = "ads1201" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, sd_backend_of_match);
+
+static struct platform_driver iio_sd_backend_adc = {
+	.driver = {
+		.name = "iio_sd_adc_backend",
+		.of_match_table = sd_backend_of_match,
+	},
+	.probe = iio_sd_backend_probe,
+};
+
+module_platform_driver(iio_sd_backend_adc);
+
+MODULE_DESCRIPTION("Basic sigma delta modulator IIO backend");
+MODULE_AUTHOR("Olivier Moysan <olivier.moysan@foss.st.com>");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(IIO_BACKEND);
-- 
2.25.1

[PATCH 8/8] iio: adc: stm32-dfsdm: add scaling support to dfsdm

[Olivier Moysan]

Add scaling support to STM32 DFSDM.

Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
---
 drivers/iio/adc/Kconfig           |  1 +
 drivers/iio/adc/stm32-dfsdm-adc.c | 94 ++++++++++++++++++++++++++++++-
 2 files changed, 92 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index f3dfdaa80678..858ae8161fa4 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1223,6 +1223,7 @@ config STM32_DFSDM_ADC
 	select IIO_BUFFER
 	select IIO_BUFFER_HW_CONSUMER
 	select IIO_TRIGGERED_BUFFER
+	select IIO_BACKEND
 	help
 	  Select this option to support ADCSigma delta modulator for
 	  STMicroelectronics STM32 digital filter for sigma delta converter.
diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
index 69b4764d7cba..93bf6035bd6d 100644
--- a/drivers/iio/adc/stm32-dfsdm-adc.c
+++ b/drivers/iio/adc/stm32-dfsdm-adc.c
@@ -9,6 +9,7 @@
 #include <linux/dmaengine.h>
 #include <linux/dma-mapping.h>
 #include <linux/iio/adc/stm32-dfsdm-adc.h>
+#include <linux/iio/backend.h>
 #include <linux/iio/buffer.h>
 #include <linux/iio/hw-consumer.h>
 #include <linux/iio/sysfs.h>
@@ -78,6 +79,7 @@ struct stm32_dfsdm_adc {
 	/* ADC specific */
 	unsigned int oversamp;
 	struct iio_hw_consumer *hwc;
+	struct iio_backend **backend;
 	struct completion completion;
 	u32 *buffer;
 
@@ -672,6 +674,8 @@ static int stm32_dfsdm_generic_channel_parse_of(struct stm32_dfsdm *dfsdm,
 						struct fwnode_handle *node)
 {
 	struct stm32_dfsdm_channel *df_ch;
+	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
+	struct iio_backend *backend;
 	const char *of_str;
 	int ret, val;
 
@@ -721,6 +725,14 @@ static int stm32_dfsdm_generic_channel_parse_of(struct stm32_dfsdm *dfsdm,
 	if (ret != -EINVAL)
 		df_ch->alt_si = 0;
 
+	if (adc->dev_data->type == DFSDM_IIO) {
+		backend = devm_iio_backend_subnode_get(&indio_dev->dev, NULL, node);
+		if (IS_ERR(backend))
+			return dev_err_probe(&indio_dev->dev, PTR_ERR(backend),
+					     "Failed to get backend\n");
+		adc->backend[ch->scan_index] = backend;
+	}
+
 	return 0;
 }
 
@@ -1060,7 +1072,7 @@ static int stm32_dfsdm_update_scan_mode(struct iio_dev *indio_dev,
 static int stm32_dfsdm_postenable(struct iio_dev *indio_dev)
 {
 	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
-	int ret;
+	int i = 0, ret;
 
 	/* Reset adc buffer index */
 	adc->bufi = 0;
@@ -1071,6 +1083,15 @@ static int stm32_dfsdm_postenable(struct iio_dev *indio_dev)
 			return ret;
 	}
 
+	if (adc->backend) {
+		while (adc->backend[i]) {
+			ret = iio_backend_enable(&indio_dev->dev, adc->backend[i]);
+			if (ret < 0)
+				return ret;
+			i++;
+		}
+	}
+
 	ret = stm32_dfsdm_start_dfsdm(adc->dfsdm);
 	if (ret < 0)
 		goto err_stop_hwc;
@@ -1103,6 +1124,7 @@ static int stm32_dfsdm_postenable(struct iio_dev *indio_dev)
 static int stm32_dfsdm_predisable(struct iio_dev *indio_dev)
 {
 	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
+	int i = 0;
 
 	stm32_dfsdm_stop_conv(indio_dev);
 
@@ -1110,6 +1132,13 @@ static int stm32_dfsdm_predisable(struct iio_dev *indio_dev)
 
 	stm32_dfsdm_stop_dfsdm(adc->dfsdm);
 
+	if (adc->backend) {
+		while (adc->backend[i]) {
+			iio_backend_disable(&indio_dev->dev, adc->backend[i]);
+			i++;
+		}
+	}
+
 	if (adc->hwc)
 		iio_hw_consumer_disable(adc->hwc);
 
@@ -1282,7 +1311,14 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
 				int *val2, long mask)
 {
 	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
-	int ret;
+
+	struct stm32_dfsdm_filter *fl = &adc->dfsdm->fl_list[adc->fl_id];
+	struct stm32_dfsdm_filter_osr *flo = &fl->flo[fl->fast];
+	u32 max = flo->max << (flo->lshift - chan->scan_type.shift);
+	int ret, idx = chan->scan_index;
+
+	if (flo->lshift < chan->scan_type.shift)
+		max = flo->max >> (chan->scan_type.shift - flo->lshift);
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
@@ -1291,6 +1327,8 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
 			return ret;
 		if (adc->hwc)
 			ret = iio_hw_consumer_enable(adc->hwc);
+		if (adc->backend[idx])
+			ret = iio_backend_enable(&indio_dev->dev, adc->backend[idx]);
 		if (ret < 0) {
 			dev_err(&indio_dev->dev,
 				"%s: IIO enable failed (channel %d)\n",
@@ -1301,6 +1339,8 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
 		ret = stm32_dfsdm_single_conv(indio_dev, chan, val);
 		if (adc->hwc)
 			iio_hw_consumer_disable(adc->hwc);
+		if (adc->backend[idx])
+			iio_backend_disable(&indio_dev->dev, adc->backend[idx]);
 		if (ret < 0) {
 			dev_err(&indio_dev->dev,
 				"%s: Conversion failed (channel %d)\n",
@@ -1320,6 +1360,45 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
 		*val = adc->sample_freq;
 
 		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		/*
+		 * Scale is expressed in mV.
+		 * When fast mode is disabled, actual resolution may be lower
+		 * than 2^n, where n=realbits-1.
+		 * This leads to underestimating input voltage. To
+		 * compensate this deviation, the voltage reference can be
+		 * corrected with a factor = realbits resolution / actual max
+		 */
+		if (adc->backend[idx]) {
+			iio_backend_read_raw(adc->backend[idx], val, val2, mask);
+
+			*val = div_u64((u64)*val * (u64)BIT(DFSDM_DATA_RES - 1), max);
+			*val2 = chan->scan_type.realbits;
+			if (chan->differential)
+				*val *= 2;
+		}
+		return IIO_VAL_FRACTIONAL_LOG2;
+
+	case IIO_CHAN_INFO_OFFSET:
+		/*
+		 * DFSDM output data are in the range [-2^n,2^n],
+		 * with n=realbits-1.
+		 * - Differential modulator:
+		 * Offset correspond to SD modulator offset.
+		 * - Single ended modulator:
+		 * Input is in [0V,Vref] range, where 0V corresponds to -2^n, and Vref to 2^n.
+		 * Add 2^n to offset. (i.e. middle of input range)
+		 * offset = offset(sd) * vref / res(sd) * max / vref.
+		 */
+		if (adc->backend[idx]) {
+			iio_backend_read_raw(adc->backend[idx], val, val2, mask);
+
+			*val = div_u64((u64)max * *val, BIT(*val2 - 1));
+			if (!chan->differential)
+				*val += max;
+		}
+		return IIO_VAL_INT;
 	}
 
 	return -EINVAL;
@@ -1449,7 +1528,15 @@ static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev, struct iio_c
 	 * IIO_CHAN_INFO_RAW: used to compute regular conversion
 	 * IIO_CHAN_INFO_OVERSAMPLING_RATIO: used to set oversampling
 	 */
-	ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
+	if (child) {
+		ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+					BIT(IIO_CHAN_INFO_SCALE) |
+					BIT(IIO_CHAN_INFO_OFFSET);
+	} else {
+		/* Legacy. Scaling not supported */
+		ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
+	}
+
 	ch->info_mask_shared_by_all = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) |
 					BIT(IIO_CHAN_INFO_SAMP_FREQ);
 
@@ -1816,3 +1903,4 @@ module_platform_driver(stm32_dfsdm_adc_driver);
 MODULE_DESCRIPTION("STM32 sigma delta ADC");
 MODULE_AUTHOR("Arnaud Pouliquen <arnaud.pouliquen@st.com>");
 MODULE_LICENSE("GPL v2");
+MODULE_IMPORT_NS(IIO_BACKEND);
-- 
2.25.1

Re: [PATCH 4/8] dt-bindings: iio: dfsdm: move to backend framework

[Conor Dooley]

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

On Tue, Jun 18, 2024 at 06:08:30PM +0200, Olivier Moysan wrote:
> Change the DFSDM binding to use the new IIO backend framework,
> along with the adoption of IIO generic channels.
> This binding change allows to add scaling support to the DFSDM.
> 
> Keep the legacy binding as deprecated for backward compatibility.
> 
> The io-backends property is supported only in generic IIO channel
> binding.
> 
> - Channel description with the generic binding (Audio and Analog):
> 
>   Properties supersed by generic properties:
>     st,adc-channels: becomes "reg" property in channel node
>     st,adc-channel-names: becomes "label" property in channel node
>   Properties moved to channel child node:
>     st,adc-channel-types, st,adc-channel-clk-src, st,adc-alt-channel
> 
> - Analog binding:
> 
>   DFSDM filter channel is configured as an IIO backend consumer.
>   Add io-backends property in channel child nodes.
> 
>   DFSDM is no more configured as a channel consumer from SD modulator.
>   Use of io-channels in DFSDM node is deprecated.
> 
> - Audio binding:
> 
>   DFSDM audio DAI is configured as a channel consumer from DFSDM filter.
>   No change compare to legacy.
> 
> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
> ---
>  .../bindings/iio/adc/st,stm32-dfsdm-adc.yaml  | 158 +++++++++++++++++-
>  1 file changed, 152 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/st,stm32-dfsdm-adc.yaml b/Documentation/devicetree/bindings/iio/adc/st,stm32-dfsdm-adc.yaml
> index c1b1324fa132..dd414bab74c1 100644
> --- a/Documentation/devicetree/bindings/iio/adc/st,stm32-dfsdm-adc.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/st,stm32-dfsdm-adc.yaml
> @@ -102,9 +102,11 @@ patternProperties:
>          items:
>            minimum: 0
>            maximum: 7
> +        deprecated: true
>  
>        st,adc-channel-names:
>          description: List of single-ended channel names.
> +        deprecated: true
>  
>        st,filter-order:
>          description: |
> @@ -118,6 +120,12 @@ patternProperties:
>        "#io-channel-cells":
>          const: 1
>  
> +      '#address-cells':
> +        const: 1
> +
> +      '#size-cells':
> +        const: 0
> +
>        st,adc-channel-types:
>          description: |
>            Single-ended channel input type.
> @@ -128,6 +136,7 @@ patternProperties:
>          items:
>            enum: [ SPI_R, SPI_F, MANCH_R, MANCH_F ]
>          $ref: /schemas/types.yaml#/definitions/non-unique-string-array
> +        deprecated: true
>  
>        st,adc-channel-clk-src:
>          description: |
> @@ -139,6 +148,7 @@ patternProperties:
>          items:
>            enum: [ CLKIN, CLKOUT, CLKOUT_F, CLKOUT_R ]
>          $ref: /schemas/types.yaml#/definitions/non-unique-string-array
> +        deprecated: true
>  
>        st,adc-alt-channel:
>          description:
> @@ -147,6 +157,7 @@ patternProperties:
>            If not set, channel n is connected to SPI input n.
>            If set, channel n is connected to SPI input n + 1.
>          type: boolean
> +        deprecated: true
>  
>        st,filter0-sync:
>          description:
> @@ -165,11 +176,65 @@ patternProperties:
>        - compatible
>        - reg
>        - interrupts
> -      - st,adc-channels
> -      - st,adc-channel-names
>        - st,filter-order
>        - "#io-channel-cells"
>  
> +    patternProperties:
> +      "^channel@([0-9]|1[0-9])$":
> +        type: object
> +        $ref: adc.yaml
> +        description: Represents the external channels which are connected to the DFSDM.
> +
> +        properties:
> +          reg:
> +            items:
> +              minimum: 0
> +              maximum: 8
> +
> +          label:
> +            description:
> +              Unique name to identify which channel this is.
> +
> +          st,adc-channel-types:
> +            description: |
> +              Single-ended channel input type.
> +              - "SPI_R": SPI with data on rising edge (default)
> +              - "SPI_F": SPI with data on falling edge
> +              - "MANCH_R": manchester codec, rising edge = logic 0, falling edge = logic 1
> +              - "MANCH_F": manchester codec, rising edge = logic 1, falling edge = logic 0
> +            items:
> +              enum: [ SPI_R, SPI_F, MANCH_R, MANCH_F ]
> +            $ref: /schemas/types.yaml#/definitions/non-unique-string-array

Why is this an array? And why is the property plural? Can a channel have
more than one type?

> +
> +          st,adc-channel-clk-src:
> +            description: |
> +              Conversion clock source.
> +              - "CLKIN": external SPI clock (CLKIN x)
> +              - "CLKOUT": internal SPI clock (CLKOUT) (default)
> +              - "CLKOUT_F": internal SPI clock divided by 2 (falling edge).
> +              - "CLKOUT_R": internal SPI clock divided by 2 (rising edge).
> +            items:
> +              enum: [ CLKIN, CLKOUT, CLKOUT_F, CLKOUT_R ]
> +            $ref: /schemas/types.yaml#/definitions/non-unique-string-array

Ditto here, but s/type/clock source/

Thanks,
Conor.

> +
> +          st,adc-alt-channel:
> +            description:
> +              Must be defined if two sigma delta modulators are
> +              connected on same SPI input.
> +              If not set, channel n is connected to SPI input n.
> +              If set, channel n is connected to SPI input n + 1.
> +            type: boolean
> +
> +          io-backends:
> +            description:
> +              From common IIO binding.

Drop this from the description.

> Used to pipe external sigma delta
> +              modulator or internal ADC backend to DFSDM channel.
> +
> +        required:
> +          - reg
> +
> +        additionalProperties: false
> +
>      allOf:
>        - if:
>            properties:
> @@ -199,9 +264,19 @@ patternProperties:
>                description:
>                  From common IIO binding. Used to pipe external sigma delta
>                  modulator or internal ADC output to DFSDM channel.
> +              deprecated: true
>  
> -          required:
> -            - io-channels
> +          if:
> +            required:
> +              - st,adc-channels
> +          then:
> +            required:
> +              - io-channels
> +
> +          patternProperties:
> +            "^channel@([0-9]|1[0-9])$":
> +              required:
> +                - io-backends

Why is this here, rather than with reg above? Only some channels require
a backend?

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

Re: [PATCH 5/8] dt-bindings: iio: add sigma delta modulator backend

[Conor Dooley]

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

On Tue, Jun 18, 2024 at 06:08:31PM +0200, Olivier Moysan wrote:
> Add documentation of device tree bindings to support
> sigma delta modulator backend in IIO framework.
> 
> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
> ---
>  .../iio/adc/sd-modulator-backend.yaml         | 43 +++++++++++++++++++
>  1 file changed, 43 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/sd-modulator-backend.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/sd-modulator-backend.yaml b/Documentation/devicetree/bindings/iio/adc/sd-modulator-backend.yaml
> new file mode 100644
> index 000000000000..b0fa71b75cd0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/sd-modulator-backend.yaml
> @@ -0,0 +1,43 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/sd-modulator-backend.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sigma delta modulator backend
> +
> +maintainers:
> +  - Olivier Moysan <olivier.moysan@foss.st.com>
> +
> +properties:
> +  compatible:
> +    description: |
> +      "sd-backend" can be used as a generic SD modulator backend,
> +      if the modulator is not specified in the compatible list.
> +    enum:
> +      - sd-backend

I'd rather not have a generic compatible like this. Something generic as
a fallback for the driver to binding against I would be fine with, but
not something that avoids people documenting their devices.

Also, I think "backend" should be dropped from the
filename/title/descriptions, the ads1201 is "just" an delta-sigma
modulator.:wq

> +      - ads1201

Missing vendor prefix.

Thanks,
Conor.


> +
> +  '#io-backend-cells':
> +    const: 0
> +
> +  reg:
> +    maxItems: 1
> +
> +  vref-supply:
> +    description: Phandle to the vref input analog reference voltage.
> +
> +required:
> +  - compatible
> +  - '#io-backend-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    ads1201: adc {
> +      compatible = "sd-backend";
> +      #io-backend-cells = <0>;
> +    };
> +
> +...
> -- 
> 2.25.1
> 

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

Re: [PATCH 2/8] iio: add enable and disable services to iio backend framework

[Nuno Sá]

On Tue, 2024-06-18 at 18:08 +0200, Olivier Moysan wrote:
> Add iio_backend_disable() and iio_backend_enable() APIs to allow
> IIO backend consumer to request backend disabling and enabling.
> 
> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
> ---

Hi Olivier,

small notes from me...

>  drivers/iio/industrialio-backend.c | 26 ++++++++++++++++++++++++++
>  include/linux/iio/backend.h        |  2 ++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-
> backend.c
> index b950e30018ca..d3db048c086b 100644
> --- a/drivers/iio/industrialio-backend.c
> +++ b/drivers/iio/industrialio-backend.c
> @@ -166,6 +166,32 @@ int devm_iio_backend_enable(struct device *dev, struct
> iio_backend *back)
>  }
>  EXPORT_SYMBOL_NS_GPL(devm_iio_backend_enable, IIO_BACKEND);
>  
> +/**
> + * iio_backend_enable - Backend enable
> + * @dev: Consumer device for the backend
> + * @back: Backend device
> + *
> + * RETURNS:
> + * 0 on success, negative error number on failure.
> + */
> +int iio_backend_enable(struct device *dev, struct iio_backend *back)
> +{
> +	return iio_backend_op_call(back, enable);
> +}
> +EXPORT_SYMBOL_NS_GPL(iio_backend_enable, IIO_BACKEND);

We do already have devm_iio_backend_enable(). From a correctness stand point and even
scalability, that API should now call your new iio_backend_enable() instead of
directly call iio_backend_op_call(). I guess that change could be in this patch.

> +
> +/**
> + * iio_backend_disable - Backend disable
> + * @dev: Consumer device for the backend
> + * @back: Backend device
> + *
> + */
> +void iio_backend_disable(struct device *dev, struct iio_backend *back)
> +{
> +	iio_backend_void_op_call(back, disable);
> +}
> +EXPORT_SYMBOL_NS_GPL(iio_backend_disable, IIO_BACKEND);
> +

We also have __iio_backend_disable() which is static since all users were using
devm_iio_backend_enable(). I understand that's not suitable for you but I would
instead rename the existing function to iio_backend_disable() and export it.

With the above changes:

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

- Nuno Sá

Re: [PATCH 1/8] iio: add read raw service to iio backend framework

[Nuno Sá]

On Tue, 2024-06-18 at 18:08 +0200, Olivier Moysan wrote:
> Add iio_backend_read_raw() service to support attributes read
> from an IIO backend.
> 
> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
> ---

Small note. With it:

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

>  drivers/iio/industrialio-backend.c | 16 ++++++++++++++++
>  include/linux/iio/backend.h        |  3 +++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-
> backend.c
> index 929aff4040ed..b950e30018ca 100644
> --- a/drivers/iio/industrialio-backend.c
> +++ b/drivers/iio/industrialio-backend.c
> @@ -357,6 +357,22 @@ int devm_iio_backend_request_buffer(struct device *dev,
>  }
>  EXPORT_SYMBOL_NS_GPL(devm_iio_backend_request_buffer, IIO_BACKEND);
>  
> +/**
> + * iio_backend_read_raw - Request a value from the backend.
> + * @back:	Backend device
> + * @val:	First element of the returned value
> + * @val2:	Second element of the returned value
> + * @mask:	Specify value to retrieve
> + *
> + * RETURNS:
> + * 0 on success, negative error number on failure.
> + */
> +int iio_backend_read_raw(struct iio_backend *back, int *val, int *val2, long mask)
> +{
> +	return iio_backend_op_call(back, read_raw, val, val2, mask);
> +}
> +EXPORT_SYMBOL_NS_GPL(iio_backend_read_raw, IIO_BACKEND);
> +

I would completely mimic the IIO API (only change being indio_dev -> back). Meaning
that 'struct iio_chan_spec const *chan' is missing.

- Nuno Sá

Re: [PATCH 3/8] iio: add child nodes support in iio backend framework

[Nuno Sá]

On Tue, 2024-06-18 at 18:08 +0200, Olivier Moysan wrote:
> Add an API to support IIO generic channels binding:
> http://devicetree.org/schemas/iio/adc/adc.yaml#
> This new API is needed, as generic channel DT node
> isn't populated as a device.
> Add devm_iio_backend_subnode_get() to allow an IIO device
> backend consumer to configure backend phandles in its
> child nodes.
> 
> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
> ---

Again small notes nits. With it:

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

>  drivers/iio/industrialio-backend.c | 48 ++++++++++++++++++++++--------
>  include/linux/iio/backend.h        |  2 ++
>  2 files changed, 38 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-
> backend.c
> index d3db048c086b..e9d29131634d 100644
> --- a/drivers/iio/industrialio-backend.c
> +++ b/drivers/iio/industrialio-backend.c
> @@ -575,17 +575,8 @@ static int __devm_iio_backend_get(struct device *dev, struct
> iio_backend *back)
>  	return 0;
>  }
>  
> -/**
> - * devm_iio_backend_get - Device managed backend device get
> - * @dev: Consumer device for the backend
> - * @name: Backend name
> - *
> - * Get's the backend associated with @dev.
> - *
> - * RETURNS:
> - * A backend pointer, negative error pointer otherwise.
> - */
> -struct iio_backend *devm_iio_backend_get(struct device *dev, const char *name)
> +static struct iio_backend *__devm_iio_backend_node_get(struct device *dev, const
> char *name,
> +						       struct fwnode_handle *node)

I would call it __devm_iio_backend_fwnode_get(). Also the parameter node -> fwnode.

>  {
>  	struct fwnode_handle *fwnode;
>  	struct iio_backend *back;
> @@ -602,7 +593,7 @@ struct iio_backend *devm_iio_backend_get(struct device *dev,
> const char *name)
>  		index = 0;
>  	}
>  
> -	fwnode = fwnode_find_reference(dev_fwnode(dev), "io-backends", index);
> +	fwnode = fwnode_find_reference(node, "io-backends", index);
>  	if (IS_ERR(fwnode)) {
>  		dev_err_probe(dev, PTR_ERR(fwnode),
>  			      "Cannot get Firmware reference\n");
> @@ -625,8 +616,41 @@ struct iio_backend *devm_iio_backend_get(struct device *dev,
> const char *name)
>  	fwnode_handle_put(fwnode);
>  	return ERR_PTR(-EPROBE_DEFER);
>  }
> +
> +/**
> + * devm_iio_backend_get - Device managed backend device get
> + * @dev: Consumer device for the backend
> + * @name: Backend name
> + *
> + * Get's the backend associated with @dev.
> + *
> + * RETURNS:
> + * A backend pointer, negative error pointer otherwise.
> + */
> +struct iio_backend *devm_iio_backend_get(struct device *dev, const char *name)
> +{
> +	return __devm_iio_backend_node_get(dev, name, dev_fwnode(dev));
> +}
>  EXPORT_SYMBOL_NS_GPL(devm_iio_backend_get, IIO_BACKEND);
>  
> +/**
> + * devm_iio_backend_subnode_get - Device managed backend device get
> + * @dev: Consumer device for the backend
> + * @name: Backend name
> + * @node: Firmware node of the backend consumer
> + *
> + * Get's the backend associated with @dev.
> + *
> + * RETURNS:
> + * A backend pointer, negative error pointer otherwise.
> + */
> +struct iio_backend *devm_iio_backend_subnode_get(struct device *dev, const char
> *name,
> +						 struct fwnode_handle *node)

Again, for consistency devm_iio_backend_fwnode_get(). And node -> fwnode

- Nuno Sá

Re: [PATCH 8/8] iio: adc: stm32-dfsdm: add scaling support to dfsdm

[Nuno Sá]

On Tue, 2024-06-18 at 18:08 +0200, Olivier Moysan wrote:
> Add scaling support to STM32 DFSDM.
> 
> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
> ---

Just some general comments... 

Acked-by: Nuno Sa <nuno.sa@analog.com>

>  drivers/iio/adc/Kconfig           |  1 +
>  drivers/iio/adc/stm32-dfsdm-adc.c | 94 ++++++++++++++++++++++++++++++-
>  2 files changed, 92 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index f3dfdaa80678..858ae8161fa4 100644
> 

...

> @@ -1301,6 +1339,8 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
>  		ret = stm32_dfsdm_single_conv(indio_dev, chan, val);
>  		if (adc->hwc)
>  			iio_hw_consumer_disable(adc->hwc);
> +		if (adc->backend[idx])
> +			iio_backend_disable(&indio_dev->dev, adc->backend[idx]);
>  		if (ret < 0) {
>  			dev_err(&indio_dev->dev,
>  				"%s: Conversion failed (channel %d)\n",
> @@ -1320,6 +1360,45 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
>  		*val = adc->sample_freq;
>  
>  		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		/*
> +		 * Scale is expressed in mV.
> +		 * When fast mode is disabled, actual resolution may be lower
> +		 * than 2^n, where n=realbits-1.
> +		 * This leads to underestimating input voltage. To
> +		 * compensate this deviation, the voltage reference can be
> +		 * corrected with a factor = realbits resolution / actual max
> +		 */
> +		if (adc->backend[idx]) {
> +			iio_backend_read_raw(adc->backend[idx], val, val2, mask);
> +

This is something that I've been thinking about since the beginning of backends
support. Basically having the "catch all" read_raw()/write_raw() or more dedicated
interfaces. For example, in your usecase I think it would make more sense to have
dedicated API's like iio_backend_read_scale() and iio_backend_read_offset() as you're
extending that functionality. Calling iio_backend_read_raw() seems a bit weird to me.

OTOH, iio_backend_read_raw() could be useful in cases where frontends call
iio_backend_extend_chan_spec() and may get some functionality they are not aware of.
And so, in the default: statement this "catch all" API could be used.

Anyways, this are really minor (likely pedantic) details and you kind of came first
and made the decision for me. As I don't really have strong feelings about it, I'm
fine with it as-is.

> +			*val = div_u64((u64)*val * (u64)BIT(DFSDM_DATA_RES - 1),
> max);
> +			*val2 = chan->scan_type.realbits;
> +			if (chan->differential)
> +				*val *= 2;
> +		}
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +
> +	case IIO_CHAN_INFO_OFFSET:
> +		/*
> +		 * DFSDM output data are in the range [-2^n,2^n],
> +		 * with n=realbits-1.
> +		 * - Differential modulator:
> +		 * Offset correspond to SD modulator offset.
> +		 * - Single ended modulator:
> +		 * Input is in [0V,Vref] range, where 0V corresponds to -2^n, and
> Vref to 2^n.
> +		 * Add 2^n to offset. (i.e. middle of input range)
> +		 * offset = offset(sd) * vref / res(sd) * max / vref.
> +		 */
> +		if (adc->backend[idx]) {
> +			iio_backend_read_raw(adc->backend[idx], val, val2, mask);
> +
> +			*val = div_u64((u64)max * *val, BIT(*val2 - 1));
> +			if (!chan->differential)
> +				*val += max;
> +		}
> +		return IIO_VAL_INT;
>  	}
>  
>  	return -EINVAL;
> @@ -1449,7 +1528,15 @@ static int stm32_dfsdm_adc_chan_init_one(struct iio_dev
> *indio_dev, struct iio_c
>  	 * IIO_CHAN_INFO_RAW: used to compute regular conversion
>  	 * IIO_CHAN_INFO_OVERSAMPLING_RATIO: used to set oversampling
>  	 */
> -	ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> +	if (child) {
> +		ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +					BIT(IIO_CHAN_INFO_SCALE) |
> +					BIT(IIO_CHAN_INFO_OFFSET);

Not sure if the above SCALE and OFFSET are solely properties if the backend. If so,
you could maybe use iio_backend_extend_chan_spec().

- Nuno Sá

Re: [PATCH 2/8] iio: add enable and disable services to iio backend framework

[Olivier MOYSAN]

Hi Nuno,

On 6/19/24 07:21, Nuno Sá wrote:
> On Tue, 2024-06-18 at 18:08 +0200, Olivier Moysan wrote:
>> Add iio_backend_disable() and iio_backend_enable() APIs to allow
>> IIO backend consumer to request backend disabling and enabling.
>>
>> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
>> ---
> 
> Hi Olivier,
> 
> small notes from me...
> 
>>   drivers/iio/industrialio-backend.c | 26 ++++++++++++++++++++++++++
>>   include/linux/iio/backend.h        |  2 ++
>>   2 files changed, 28 insertions(+)
>>
>> diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-
>> backend.c
>> index b950e30018ca..d3db048c086b 100644
>> --- a/drivers/iio/industrialio-backend.c
>> +++ b/drivers/iio/industrialio-backend.c
>> @@ -166,6 +166,32 @@ int devm_iio_backend_enable(struct device *dev, struct
>> iio_backend *back)
>>   }
>>   EXPORT_SYMBOL_NS_GPL(devm_iio_backend_enable, IIO_BACKEND);
>>   
>> +/**
>> + * iio_backend_enable - Backend enable
>> + * @dev: Consumer device for the backend
>> + * @back: Backend device
>> + *
>> + * RETURNS:
>> + * 0 on success, negative error number on failure.
>> + */
>> +int iio_backend_enable(struct device *dev, struct iio_backend *back)
>> +{
>> +	return iio_backend_op_call(back, enable);
>> +}
>> +EXPORT_SYMBOL_NS_GPL(iio_backend_enable, IIO_BACKEND);
> 
> We do already have devm_iio_backend_enable(). From a correctness stand point and even
> scalability, that API should now call your new iio_backend_enable() instead of
> directly call iio_backend_op_call(). I guess that change could be in this patch.
>

Sure. I have updated the patch.

>> +
>> +/**
>> + * iio_backend_disable - Backend disable
>> + * @dev: Consumer device for the backend
>> + * @back: Backend device
>> + *
>> + */
>> +void iio_backend_disable(struct device *dev, struct iio_backend *back)
>> +{
>> +	iio_backend_void_op_call(back, disable);
>> +}
>> +EXPORT_SYMBOL_NS_GPL(iio_backend_disable, IIO_BACKEND);
>> +
> 
> We also have __iio_backend_disable() which is static since all users were using
> devm_iio_backend_enable(). I understand that's not suitable for you but I would
> instead rename the existing function to iio_backend_disable() and export it.
> 

Just renaming is not sufficient. The reason is that 
devm_add_action_or_reset() require an action with action(void *) 
prototype. So the prototype of iio_backend_disable() has to be changed 
to void iio_backend_disable(void *back).
I placed the same arguments in enable and disable for symmetry, but *dev 
is not required for time being in disable API. So it can make sense to 
change iio_backend_disable() prototype.
alternatively, we can call __iio_backend_disable() through this API.
Please, let me know is you have a preference.

Thanks
Olivier

> With the above changes:
> 
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> 
> - Nuno Sá
> 

Re: [PATCH 3/8] iio: add child nodes support in iio backend framework

[Olivier MOYSAN]

On 6/19/24 07:31, Nuno Sá wrote:
> On Tue, 2024-06-18 at 18:08 +0200, Olivier Moysan wrote:
>> Add an API to support IIO generic channels binding:
>> http://devicetree.org/schemas/iio/adc/adc.yaml#
>> This new API is needed, as generic channel DT node
>> isn't populated as a device.
>> Add devm_iio_backend_subnode_get() to allow an IIO device
>> backend consumer to configure backend phandles in its
>> child nodes.
>>
>> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
>> ---
> 
> Again small notes nits. With it:
> 
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> 
>>   drivers/iio/industrialio-backend.c | 48 ++++++++++++++++++++++--------
>>   include/linux/iio/backend.h        |  2 ++
>>   2 files changed, 38 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-
>> backend.c
>> index d3db048c086b..e9d29131634d 100644
>> --- a/drivers/iio/industrialio-backend.c
>> +++ b/drivers/iio/industrialio-backend.c
>> @@ -575,17 +575,8 @@ static int __devm_iio_backend_get(struct device *dev, struct
>> iio_backend *back)
>>   	return 0;
>>   }
>>   
>> -/**
>> - * devm_iio_backend_get - Device managed backend device get
>> - * @dev: Consumer device for the backend
>> - * @name: Backend name
>> - *
>> - * Get's the backend associated with @dev.
>> - *
>> - * RETURNS:
>> - * A backend pointer, negative error pointer otherwise.
>> - */
>> -struct iio_backend *devm_iio_backend_get(struct device *dev, const char *name)
>> +static struct iio_backend *__devm_iio_backend_node_get(struct device *dev, const
>> char *name,
>> +						       struct fwnode_handle *node)
> 
> I would call it __devm_iio_backend_fwnode_get(). Also the parameter node -> fwnode.
> 
>>   {
>>   	struct fwnode_handle *fwnode;
>>   	struct iio_backend *back;
>> @@ -602,7 +593,7 @@ struct iio_backend *devm_iio_backend_get(struct device *dev,
>> const char *name)
>>   		index = 0;
>>   	}
>>   
>> -	fwnode = fwnode_find_reference(dev_fwnode(dev), "io-backends", index);
>> +	fwnode = fwnode_find_reference(node, "io-backends", index);
>>   	if (IS_ERR(fwnode)) {
>>   		dev_err_probe(dev, PTR_ERR(fwnode),
>>   			      "Cannot get Firmware reference\n");
>> @@ -625,8 +616,41 @@ struct iio_backend *devm_iio_backend_get(struct device *dev,
>> const char *name)
>>   	fwnode_handle_put(fwnode);
>>   	return ERR_PTR(-EPROBE_DEFER);
>>   }
>> +
>> +/**
>> + * devm_iio_backend_get - Device managed backend device get
>> + * @dev: Consumer device for the backend
>> + * @name: Backend name
>> + *
>> + * Get's the backend associated with @dev.
>> + *
>> + * RETURNS:
>> + * A backend pointer, negative error pointer otherwise.
>> + */
>> +struct iio_backend *devm_iio_backend_get(struct device *dev, const char *name)
>> +{
>> +	return __devm_iio_backend_node_get(dev, name, dev_fwnode(dev));
>> +}
>>   EXPORT_SYMBOL_NS_GPL(devm_iio_backend_get, IIO_BACKEND);
>>   
>> +/**
>> + * devm_iio_backend_subnode_get - Device managed backend device get
>> + * @dev: Consumer device for the backend
>> + * @name: Backend name
>> + * @node: Firmware node of the backend consumer
>> + *
>> + * Get's the backend associated with @dev.
>> + *
>> + * RETURNS:
>> + * A backend pointer, negative error pointer otherwise.
>> + */
>> +struct iio_backend *devm_iio_backend_subnode_get(struct device *dev, const char
>> *name,
>> +						 struct fwnode_handle *node)
> 
> Again, for consistency devm_iio_backend_fwnode_get(). And node -> fwnode
> 

Done.
Thanks
Olivier

> - Nuno Sá
> 
> 

Re: [PATCH 4/8] dt-bindings: iio: dfsdm: move to backend framework

[Olivier MOYSAN]

Hi Conor,

On 6/18/24 20:10, Conor Dooley wrote:
> On Tue, Jun 18, 2024 at 06:08:30PM +0200, Olivier Moysan wrote:
>> Change the DFSDM binding to use the new IIO backend framework,
>> along with the adoption of IIO generic channels.
>> This binding change allows to add scaling support to the DFSDM.
>>
>> Keep the legacy binding as deprecated for backward compatibility.
>>
>> The io-backends property is supported only in generic IIO channel
>> binding.
>>
>> - Channel description with the generic binding (Audio and Analog):
>>
>>    Properties supersed by generic properties:
>>      st,adc-channels: becomes "reg" property in channel node
>>      st,adc-channel-names: becomes "label" property in channel node
>>    Properties moved to channel child node:
>>      st,adc-channel-types, st,adc-channel-clk-src, st,adc-alt-channel
>>
>> - Analog binding:
>>
>>    DFSDM filter channel is configured as an IIO backend consumer.
>>    Add io-backends property in channel child nodes.
>>
>>    DFSDM is no more configured as a channel consumer from SD modulator.
>>    Use of io-channels in DFSDM node is deprecated.
>>
>> - Audio binding:
>>
>>    DFSDM audio DAI is configured as a channel consumer from DFSDM filter.
>>    No change compare to legacy.
>>
>> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
>> ---
>>   .../bindings/iio/adc/st,stm32-dfsdm-adc.yaml  | 158 +++++++++++++++++-
>>   1 file changed, 152 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/st,stm32-dfsdm-adc.yaml b/Documentation/devicetree/bindings/iio/adc/st,stm32-dfsdm-adc.yaml
>> index c1b1324fa132..dd414bab74c1 100644
>> --- a/Documentation/devicetree/bindings/iio/adc/st,stm32-dfsdm-adc.yaml
>> +++ b/Documentation/devicetree/bindings/iio/adc/st,stm32-dfsdm-adc.yaml
>> @@ -102,9 +102,11 @@ patternProperties:
>>           items:
>>             minimum: 0
>>             maximum: 7
>> +        deprecated: true
>>   
>>         st,adc-channel-names:
>>           description: List of single-ended channel names.
>> +        deprecated: true
>>   
>>         st,filter-order:
>>           description: |
>> @@ -118,6 +120,12 @@ patternProperties:
>>         "#io-channel-cells":
>>           const: 1
>>   
>> +      '#address-cells':
>> +        const: 1
>> +
>> +      '#size-cells':
>> +        const: 0
>> +
>>         st,adc-channel-types:
>>           description: |
>>             Single-ended channel input type.
>> @@ -128,6 +136,7 @@ patternProperties:
>>           items:
>>             enum: [ SPI_R, SPI_F, MANCH_R, MANCH_F ]
>>           $ref: /schemas/types.yaml#/definitions/non-unique-string-array
>> +        deprecated: true
>>   
>>         st,adc-channel-clk-src:
>>           description: |
>> @@ -139,6 +148,7 @@ patternProperties:
>>           items:
>>             enum: [ CLKIN, CLKOUT, CLKOUT_F, CLKOUT_R ]
>>           $ref: /schemas/types.yaml#/definitions/non-unique-string-array
>> +        deprecated: true
>>   
>>         st,adc-alt-channel:
>>           description:
>> @@ -147,6 +157,7 @@ patternProperties:
>>             If not set, channel n is connected to SPI input n.
>>             If set, channel n is connected to SPI input n + 1.
>>           type: boolean
>> +        deprecated: true
>>   
>>         st,filter0-sync:
>>           description:
>> @@ -165,11 +176,65 @@ patternProperties:
>>         - compatible
>>         - reg
>>         - interrupts
>> -      - st,adc-channels
>> -      - st,adc-channel-names
>>         - st,filter-order
>>         - "#io-channel-cells"
>>   
>> +    patternProperties:
>> +      "^channel@([0-9]|1[0-9])$":
>> +        type: object
>> +        $ref: adc.yaml
>> +        description: Represents the external channels which are connected to the DFSDM.
>> +
>> +        properties:
>> +          reg:
>> +            items:
>> +              minimum: 0
>> +              maximum: 8
>> +
>> +          label:
>> +            description:
>> +              Unique name to identify which channel this is.
>> +
>> +          st,adc-channel-types:
>> +            description: |
>> +              Single-ended channel input type.
>> +              - "SPI_R": SPI with data on rising edge (default)
>> +              - "SPI_F": SPI with data on falling edge
>> +              - "MANCH_R": manchester codec, rising edge = logic 0, falling edge = logic 1
>> +              - "MANCH_F": manchester codec, rising edge = logic 1, falling edge = logic 0
>> +            items:
>> +              enum: [ SPI_R, SPI_F, MANCH_R, MANCH_F ]
>> +            $ref: /schemas/types.yaml#/definitions/non-unique-string-array
> 
> Why is this an array? And why is the property plural? Can a channel have
> more than one type?
> 

You are right. In channel node context, this property is no more an 
array. It has to be managed as a single string (This is already the case 
in the driver). I will change the type in binding and the name, to 
reflect this.

>> +
>> +          st,adc-channel-clk-src:
>> +            description: |
>> +              Conversion clock source.
>> +              - "CLKIN": external SPI clock (CLKIN x)
>> +              - "CLKOUT": internal SPI clock (CLKOUT) (default)
>> +              - "CLKOUT_F": internal SPI clock divided by 2 (falling edge).
>> +              - "CLKOUT_R": internal SPI clock divided by 2 (rising edge).
>> +            items:
>> +              enum: [ CLKIN, CLKOUT, CLKOUT_F, CLKOUT_R ]
>> +            $ref: /schemas/types.yaml#/definitions/non-unique-string-array
> 
> Ditto here, but s/type/clock source/
> 

Same here.

> Thanks,
> Conor.
> 
>> +
>> +          st,adc-alt-channel:
>> +            description:
>> +              Must be defined if two sigma delta modulators are
>> +              connected on same SPI input.
>> +              If not set, channel n is connected to SPI input n.
>> +              If set, channel n is connected to SPI input n + 1.
>> +            type: boolean
>> +
>> +          io-backends:
>> +            description:
>> +              From common IIO binding.
> 
> Drop this from the description.
> 
>> Used to pipe external sigma delta
>> +              modulator or internal ADC backend to DFSDM channel.
>> +
>> +        required:
>> +          - reg
>> +
>> +        additionalProperties: false
>> +
>>       allOf:
>>         - if:
>>             properties:
>> @@ -199,9 +264,19 @@ patternProperties:
>>                 description:
>>                   From common IIO binding. Used to pipe external sigma delta
>>                   modulator or internal ADC output to DFSDM channel.
>> +              deprecated: true
>>   
>> -          required:
>> -            - io-channels
>> +          if:
>> +            required:
>> +              - st,adc-channels
>> +          then:
>> +            required:
>> +              - io-channels
>> +
>> +          patternProperties:
>> +            "^channel@([0-9]|1[0-9])$":
>> +              required:
>> +                - io-backends
> 
> Why is this here, rather than with reg above? Only some channels require
> a backend?

The io-backends property is required only when we use st,stm32-dfsdm-adc 
compatible. In other words, when we are in an analog use case. In this 
case the channel is a consumer of a backend (typically a sd modulator)
In an audio use case (compatible st,stm32-dfsdm-dmic) the backend is not 
required.

BRs
Olivier

Re: [PATCH 4/8] dt-bindings: iio: dfsdm: move to backend framework

[Conor Dooley]

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

On Thu, Jun 20, 2024 at 10:03:44AM +0200, Olivier MOYSAN wrote:
> On 6/18/24 20:10, Conor Dooley wrote:
> > On Tue, Jun 18, 2024 at 06:08:30PM +0200, Olivier Moysan wrote:
> > >       allOf:
> > >         - if:
> > >             properties:
> > > @@ -199,9 +264,19 @@ patternProperties:
> > >                 description:
> > >                   From common IIO binding. Used to pipe external sigma delta
> > >                   modulator or internal ADC output to DFSDM channel.
> > > +              deprecated: true
> > > -          required:
> > > -            - io-channels
> > > +          if:
> > > +            required:
> > > +              - st,adc-channels
> > > +          then:
> > > +            required:
> > > +              - io-channels
> > > +
> > > +          patternProperties:
> > > +            "^channel@([0-9]|1[0-9])$":
> > > +              required:
> > > +                - io-backends
> > 
> > Why is this here, rather than with reg above? Only some channels require
> > a backend?
> 
> The io-backends property is required only when we use st,stm32-dfsdm-adc
> compatible. In other words, when we are in an analog use case. In this case
> the channel is a consumer of a backend (typically a sd modulator)
> In an audio use case (compatible st,stm32-dfsdm-dmic) the backend is not
> required.

Ahh, I think the hunks and indent confused me here. What you're doing is
making io-backends required based on the compatible, but what I thought
you were doing was trying to make io-backends required in channels if
st,adc-channels was set.

Thanks for the explanation,
Conor.

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

Re: [PATCH 2/8] iio: add enable and disable services to iio backend framework

[Nuno Sá]

On Wed, 2024-06-19 at 17:59 +0200, Olivier MOYSAN wrote:
> Hi Nuno,
> 
> On 6/19/24 07:21, Nuno Sá wrote:
> > On Tue, 2024-06-18 at 18:08 +0200, Olivier Moysan wrote:
> > > Add iio_backend_disable() and iio_backend_enable() APIs to allow
> > > IIO backend consumer to request backend disabling and enabling.
> > > 
> > > Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
> > > ---
> > 
> > Hi Olivier,
> > 
> > small notes from me...
> > 
> > >   drivers/iio/industrialio-backend.c | 26 ++++++++++++++++++++++++++
> > >   include/linux/iio/backend.h        |  2 ++
> > >   2 files changed, 28 insertions(+)
> > > 
> > > diff --git a/drivers/iio/industrialio-backend.c
> > > b/drivers/iio/industrialio-
> > > backend.c
> > > index b950e30018ca..d3db048c086b 100644
> > > --- a/drivers/iio/industrialio-backend.c
> > > +++ b/drivers/iio/industrialio-backend.c
> > > @@ -166,6 +166,32 @@ int devm_iio_backend_enable(struct device *dev,
> > > struct
> > > iio_backend *back)
> > >   }
> > >   EXPORT_SYMBOL_NS_GPL(devm_iio_backend_enable, IIO_BACKEND);
> > >   
> > > +/**
> > > + * iio_backend_enable - Backend enable
> > > + * @dev: Consumer device for the backend
> > > + * @back: Backend device
> > > + *
> > > + * RETURNS:
> > > + * 0 on success, negative error number on failure.
> > > + */
> > > +int iio_backend_enable(struct device *dev, struct iio_backend *back)
> > > +{
> > > +	return iio_backend_op_call(back, enable);
> > > +}
> > > +EXPORT_SYMBOL_NS_GPL(iio_backend_enable, IIO_BACKEND);
> > 
> > We do already have devm_iio_backend_enable(). From a correctness stand point
> > and even
> > scalability, that API should now call your new iio_backend_enable() instead
> > of
> > directly call iio_backend_op_call(). I guess that change could be in this
> > patch.
> > 
> 
> Sure. I have updated the patch.
> 
> > > +
> > > +/**
> > > + * iio_backend_disable - Backend disable
> > > + * @dev: Consumer device for the backend
> > > + * @back: Backend device
> > > + *
> > > + */
> > > +void iio_backend_disable(struct device *dev, struct iio_backend *back)
> > > +{
> > > +	iio_backend_void_op_call(back, disable);
> > > +}
> > > +EXPORT_SYMBOL_NS_GPL(iio_backend_disable, IIO_BACKEND);
> > > +
> > 
> > We also have __iio_backend_disable() which is static since all users were
> > using
> > devm_iio_backend_enable(). I understand that's not suitable for you but I
> > would
> > instead rename the existing function to iio_backend_disable() and export it.
> > 
> 
> Just renaming is not sufficient. The reason is that 
> devm_add_action_or_reset() require an action with action(void *) 
> prototype. So the prototype of iio_backend_disable() has to be changed 
> to void iio_backend_disable(void *back).
> I placed the same arguments in enable and disable for symmetry, but *dev 
> is not required for time being in disable API. So it can make sense to 
> change iio_backend_disable() prototype.
> alternatively, we can call __iio_backend_disable() through this API.
> Please, let me know is you have a preference.
> 

Oh, yes, you're right. I would prefer your later option. Call
__iio_backend_disable() from __iio_backend_disable() with a proper typed
parameter. I also just realized your 'struct device *dev' parameter. I think it
can be removed for these APIs. The only reason for it is for
devm_add_action_or_reset() which we don't need-

- Nuno Sá
> > 

Re: [PATCH 8/8] iio: adc: stm32-dfsdm: add scaling support to dfsdm

[Olivier MOYSAN]

Hi Nuno,

On 6/19/24 07:47, Nuno Sá wrote:
> On Tue, 2024-06-18 at 18:08 +0200, Olivier Moysan wrote:
>> Add scaling support to STM32 DFSDM.
>>
>> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
>> ---
> 
> Just some general comments...
> 
> Acked-by: Nuno Sa <nuno.sa@analog.com>
> 
>>   drivers/iio/adc/Kconfig           |  1 +
>>   drivers/iio/adc/stm32-dfsdm-adc.c | 94 ++++++++++++++++++++++++++++++-
>>   2 files changed, 92 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index f3dfdaa80678..858ae8161fa4 100644
>>
> 
> ...
> 
>> @@ -1301,6 +1339,8 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
>>   		ret = stm32_dfsdm_single_conv(indio_dev, chan, val);
>>   		if (adc->hwc)
>>   			iio_hw_consumer_disable(adc->hwc);
>> +		if (adc->backend[idx])
>> +			iio_backend_disable(&indio_dev->dev, adc->backend[idx]);
>>   		if (ret < 0) {
>>   			dev_err(&indio_dev->dev,
>>   				"%s: Conversion failed (channel %d)\n",
>> @@ -1320,6 +1360,45 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
>>   		*val = adc->sample_freq;
>>   
>>   		return IIO_VAL_INT;
>> +
>> +	case IIO_CHAN_INFO_SCALE:
>> +		/*
>> +		 * Scale is expressed in mV.
>> +		 * When fast mode is disabled, actual resolution may be lower
>> +		 * than 2^n, where n=realbits-1.
>> +		 * This leads to underestimating input voltage. To
>> +		 * compensate this deviation, the voltage reference can be
>> +		 * corrected with a factor = realbits resolution / actual max
>> +		 */
>> +		if (adc->backend[idx]) {
>> +			iio_backend_read_raw(adc->backend[idx], val, val2, mask);
>> +
> 
> This is something that I've been thinking about since the beginning of backends
> support. Basically having the "catch all" read_raw()/write_raw() or more dedicated
> interfaces. For example, in your usecase I think it would make more sense to have
> dedicated API's like iio_backend_read_scale() and iio_backend_read_offset() as you're
> extending that functionality. Calling iio_backend_read_raw() seems a bit weird to me.
> 
> OTOH, iio_backend_read_raw() could be useful in cases where frontends call
> iio_backend_extend_chan_spec() and may get some functionality they are not aware of.
> And so, in the default: statement this "catch all" API could be used.
> 
> Anyways, this are really minor (likely pedantic) details and you kind of came first
> and made the decision for me. As I don't really have strong feelings about it, I'm
> fine with it as-is.
> 

My first idea was to stick to IIO read_raw API ('struct iio_chan_spec 
const *chan' should at least be added to prototype as you pointed it 
out). But I agree that iio_backend_read_raw() is not explicit regarding 
the requested info.
I consider using the existing API iio_backend_ext_info_get() for v2, as 
you suggested.

BRs
Olivier

>> +			*val = div_u64((u64)*val * (u64)BIT(DFSDM_DATA_RES - 1),
>> max);
>> +			*val2 = chan->scan_type.realbits;
>> +			if (chan->differential)
>> +				*val *= 2;
>> +		}
>> +		return IIO_VAL_FRACTIONAL_LOG2;
>> +
>> +	case IIO_CHAN_INFO_OFFSET:
>> +		/*
>> +		 * DFSDM output data are in the range [-2^n,2^n],
>> +		 * with n=realbits-1.
>> +		 * - Differential modulator:
>> +		 * Offset correspond to SD modulator offset.
>> +		 * - Single ended modulator:
>> +		 * Input is in [0V,Vref] range, where 0V corresponds to -2^n, and
>> Vref to 2^n.
>> +		 * Add 2^n to offset. (i.e. middle of input range)
>> +		 * offset = offset(sd) * vref / res(sd) * max / vref.
>> +		 */
>> +		if (adc->backend[idx]) {
>> +			iio_backend_read_raw(adc->backend[idx], val, val2, mask);
>> +
>> +			*val = div_u64((u64)max * *val, BIT(*val2 - 1));
>> +			if (!chan->differential)
>> +				*val += max;
>> +		}
>> +		return IIO_VAL_INT;
>>   	}
>>   
>>   	return -EINVAL;
>> @@ -1449,7 +1528,15 @@ static int stm32_dfsdm_adc_chan_init_one(struct iio_dev
>> *indio_dev, struct iio_c
>>   	 * IIO_CHAN_INFO_RAW: used to compute regular conversion
>>   	 * IIO_CHAN_INFO_OVERSAMPLING_RATIO: used to set oversampling
>>   	 */
>> -	ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
>> +	if (child) {
>> +		ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> +					BIT(IIO_CHAN_INFO_SCALE) |
>> +					BIT(IIO_CHAN_INFO_OFFSET);
> 
> Not sure if the above SCALE and OFFSET are solely properties if the backend. If so,
> you could maybe use iio_backend_extend_chan_spec().
> 
> - Nuno Sá
> 

Re: [PATCH 2/8] iio: add enable and disable services to iio backend framework

[Jonathan Cameron]

On Thu, 20 Jun 2024 12:07:47 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Wed, 2024-06-19 at 17:59 +0200, Olivier MOYSAN wrote:
> > Hi Nuno,
> > 
> > On 6/19/24 07:21, Nuno Sá wrote:  
> > > On Tue, 2024-06-18 at 18:08 +0200, Olivier Moysan wrote:  
> > > > Add iio_backend_disable() and iio_backend_enable() APIs to allow
> > > > IIO backend consumer to request backend disabling and enabling.
> > > > 
> > > > Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
> > > > ---  
> > > 
> > > Hi Olivier,
> > > 
> > > small notes from me...
> > >   
> > > >   drivers/iio/industrialio-backend.c | 26 ++++++++++++++++++++++++++
> > > >   include/linux/iio/backend.h        |  2 ++
> > > >   2 files changed, 28 insertions(+)
> > > > 
> > > > diff --git a/drivers/iio/industrialio-backend.c
> > > > b/drivers/iio/industrialio-
> > > > backend.c
> > > > index b950e30018ca..d3db048c086b 100644
> > > > --- a/drivers/iio/industrialio-backend.c
> > > > +++ b/drivers/iio/industrialio-backend.c
> > > > @@ -166,6 +166,32 @@ int devm_iio_backend_enable(struct device *dev,
> > > > struct
> > > > iio_backend *back)
> > > >   }
> > > >   EXPORT_SYMBOL_NS_GPL(devm_iio_backend_enable, IIO_BACKEND);
> > > >   
> > > > +/**
> > > > + * iio_backend_enable - Backend enable
> > > > + * @dev: Consumer device for the backend
> > > > + * @back: Backend device
> > > > + *
> > > > + * RETURNS:
> > > > + * 0 on success, negative error number on failure.
> > > > + */
> > > > +int iio_backend_enable(struct device *dev, struct iio_backend *back)
> > > > +{
> > > > +	return iio_backend_op_call(back, enable);
> > > > +}
> > > > +EXPORT_SYMBOL_NS_GPL(iio_backend_enable, IIO_BACKEND);  
> > > 
> > > We do already have devm_iio_backend_enable(). From a correctness stand point
> > > and even
> > > scalability, that API should now call your new iio_backend_enable() instead
> > > of
> > > directly call iio_backend_op_call(). I guess that change could be in this
> > > patch.
> > >   
> > 
> > Sure. I have updated the patch.
> >   
> > > > +
> > > > +/**
> > > > + * iio_backend_disable - Backend disable
> > > > + * @dev: Consumer device for the backend
> > > > + * @back: Backend device
> > > > + *
> > > > + */
> > > > +void iio_backend_disable(struct device *dev, struct iio_backend *back)
> > > > +{
> > > > +	iio_backend_void_op_call(back, disable);
> > > > +}
> > > > +EXPORT_SYMBOL_NS_GPL(iio_backend_disable, IIO_BACKEND);
> > > > +  
> > > 
> > > We also have __iio_backend_disable() which is static since all users were
> > > using
> > > devm_iio_backend_enable(). I understand that's not suitable for you but I
> > > would
> > > instead rename the existing function to iio_backend_disable() and export it.
> > >   
> > 
> > Just renaming is not sufficient. The reason is that 
> > devm_add_action_or_reset() require an action with action(void *) 
> > prototype. So the prototype of iio_backend_disable() has to be changed 
> > to void iio_backend_disable(void *back).
> > I placed the same arguments in enable and disable for symmetry, but *dev 
> > is not required for time being in disable API. So it can make sense to 
> > change iio_backend_disable() prototype.
> > alternatively, we can call __iio_backend_disable() through this API.
> > Please, let me know is you have a preference.
> >   
> 
> Oh, yes, you're right. I would prefer your later option. Call
> __iio_backend_disable() from __iio_backend_disable() with a proper typed
?  That looks like an infinite loop :)  
> parameter. I also just realized your 'struct device *dev' parameter. I think it
> can be removed for these APIs. The only reason for it is for
> devm_add_action_or_reset() which we don't need-
> 
> - Nuno Sá
> > >   
> 

Re: [PATCH 3/8] iio: add child nodes support in iio backend framework

[Jonathan Cameron]

On Tue, 18 Jun 2024 18:08:29 +0200
Olivier Moysan <olivier.moysan@foss.st.com> wrote:

> Add an API to support IIO generic channels binding:
> http://devicetree.org/schemas/iio/adc/adc.yaml#
> This new API is needed, as generic channel DT node
> isn't populated as a device.
> Add devm_iio_backend_subnode_get() to allow an IIO device
> backend consumer to configure backend phandles in its
> child nodes.

Really trivial stuff:

Don't wrap so far below 75 chars that is typical limit
for commit messages.

> 
> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>

> +
> +/**
> + * devm_iio_backend_get - Device managed backend device get
> + * @dev: Consumer device for the backend
> + * @name: Backend name
> + *
> + * Get's the backend associated with @dev.
> + *
> + * RETURNS:
> + * A backend pointer, negative error pointer otherwise.
> + */
> +struct iio_backend *devm_iio_backend_get(struct device *dev, const char *name)
> +{
> +	return __devm_iio_backend_node_get(dev, name, dev_fwnode(dev));
> +}
>  EXPORT_SYMBOL_NS_GPL(devm_iio_backend_get, IIO_BACKEND);
>  
> +/**
> + * devm_iio_backend_subnode_get - Device managed backend device get

Same short description as the one above.  Fairly sure this wants to be different!

> + * @dev: Consumer device for the backend
> + * @name: Backend name
> + * @node: Firmware node of the backend consumer
> + *
> + * Get's the backend associated with @dev.
> + *
> + * RETURNS:
> + * A backend pointer, negative error pointer otherwise.
> + */
> +struct iio_backend *devm_iio_backend_subnode_get(struct device *dev, const char *name,
> +						 struct fwnode_handle *node)
> +{
> +	return __devm_iio_backend_node_get(dev, name, node);
> +}
> +EXPORT_SYMBOL_NS_GPL(devm_iio_backend_subnode_get, IIO_BACKEND);

Re: [PATCH 6/8] iio: adc: stm32-dfsdm: adopt generic channels bindings

[Jonathan Cameron]

On Tue, 18 Jun 2024 18:08:32 +0200
Olivier Moysan <olivier.moysan@foss.st.com> wrote:

> Move to generic channels binding to ease new backend framework adoption
> and prepare the convergence with MDF IP support on STM32MP2 SoC family.
> 
> Legacy binding:
> DFSDM is an IIO channel consumer.
> SD modulator is an IIO channels provider.
> The channel phandles are provided in DT through io-channels property
> and channel indexes through st,adc-channels property.
> 
> New binding:
> DFSDM is an IIO channel provider.
> The channel indexes are given by reg property in channel child node.
> 
> This new binding is intended to be used with SD modulator IIO backends.
> It does not support SD modulator legacy IIO devices.
> The st,adc-channels property presence is used to discriminate
> between legacy and backend bindings.
> 
> The support of the DFSDM legacy channels and SD modulator IIO devices
> is kept for backward compatibility.
> 
> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
Hi Oliver

A few minor things inline.

Jonathan

> ---
>  drivers/iio/adc/stm32-dfsdm-adc.c | 208 ++++++++++++++++++++++++------
>  1 file changed, 171 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
> index 9a47d2c87f05..69b4764d7cba 100644
> --- a/drivers/iio/adc/stm32-dfsdm-adc.c
> +++ b/drivers/iio/adc/stm32-dfsdm-adc.c
> @@ -666,6 +666,64 @@ static int stm32_dfsdm_channel_parse_of(struct stm32_dfsdm *dfsdm,
>  	return 0;
>  }
>  
> +static int stm32_dfsdm_generic_channel_parse_of(struct stm32_dfsdm *dfsdm,
> +						struct iio_dev *indio_dev,
> +						struct iio_chan_spec *ch,
> +						struct fwnode_handle *node)
> +{
> +	struct stm32_dfsdm_channel *df_ch;
> +	const char *of_str;
> +	int ret, val;
> +
> +	ret = fwnode_property_read_u32(node, "reg", &ch->channel);
> +	if (ret < 0) {
> +		dev_err(&indio_dev->dev, "Missing channel index %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (ch->channel >= dfsdm->num_chs) {
> +		dev_err(&indio_dev->dev, " Error bad channel number %d (max = %d)\n",
> +			ch->channel, dfsdm->num_chs);
> +		return -EINVAL;
> +	}
> +
> +	ret = fwnode_property_read_string(node, "label", &ch->datasheet_name);
> +	if (ret < 0) {
> +		dev_err(&indio_dev->dev,
> +			" Error parsing 'label' for idx %d\n", ch->channel);
> +		return ret;
> +	}
> +
> +	df_ch =  &dfsdm->ch_list[ch->channel];
> +	df_ch->id = ch->channel;
> +
> +	ret = fwnode_property_read_string(node, "st,adc-channel-types", &of_str);
> +	if (!ret) {
> +		val = stm32_dfsdm_str2val(of_str, stm32_dfsdm_chan_type);
> +		if (val < 0)
> +			return val;
> +	} else {
> +		val = 0;

Sometimes better to set a default, then override if if the property is read
successfully.
	df_ch->type = 0;
	if (!fwnode_property_read_string()) {
		int val = ...

		df_ch->type = val;
	}
	
etc
	
> +	}
> +	df_ch->type = val;
> +
> +	ret = fwnode_property_read_string(node, "st,adc-channel-clk-src", &of_str);
> +	if (!ret) {
> +		val = stm32_dfsdm_str2val(of_str, stm32_dfsdm_chan_src);
> +		if (val < 0)
> +			return val;
> +	} else {
> +		val = 0;
> +	}
> +	df_ch->src = val;
> +
> +	ret = fwnode_property_read_u32(node, "st,adc-alt-channel", &df_ch->alt_si);
> +	if (ret != -EINVAL)
> +		df_ch->alt_si = 0;

I'm confused. If it does == EINVAL we just silently carry on with alt_si sort
of undefined. I guess 0?

> +
> +	return 0;
> +}
> +

...

> +static int stm32_dfsdm_chan_init(struct iio_dev *indio_dev, struct iio_chan_spec *channels)
> +{
> +	int num_ch = indio_dev->num_channels;
> +	int chan_idx = 0, ret = 0;
> +
> +	for (chan_idx = 0; chan_idx < num_ch; chan_idx++) {
> +		channels[chan_idx].scan_index = chan_idx;
> +		ret = stm32_dfsdm_adc_chan_init_one(indio_dev, &channels[chan_idx], NULL);
> +		if (ret < 0) {
> +			dev_err(&indio_dev->dev, "Channels init failed\n");
> +			return ret;
> +		}
> +	}
> +
> +	return ret;
return 0;    I don't think it's ever positive and can't get here with it negative.

> +}
> +
> +static int stm32_dfsdm_generic_chan_init(struct iio_dev *indio_dev, struct iio_chan_spec *channels)
> +{
> +	struct fwnode_handle *child;
> +	int chan_idx = 0, ret;
> +
> +	device_for_each_child_node(&indio_dev->dev, child) {

device_for_each_child_node_scoped() and direct returns should tidy this up a tiny bit.


> +		/* Skip DAI node in DFSDM audio nodes */
> +		if (fwnode_property_present(child, "compatible"))
> +			continue;
> +
> +		channels[chan_idx].scan_index = chan_idx;
> +		ret = stm32_dfsdm_adc_chan_init_one(indio_dev, &channels[chan_idx], child);
> +		if (ret < 0) {
> +			dev_err(&indio_dev->dev, "Channels init failed\n");
> +			goto err;

This is consistent with existing code, but would be nice to make extensive
use of dev_err_probe() in this driver and this is a gone place for that.
			return dev_err_probe(indio_dev->dev, ret, "...);


> +		}
> +
> +		chan_idx++;
> +	}
> +	return chan_idx;
> +
> +err:
> +	fwnode_handle_put(child);
> +
> +	return ret;
>  }
>  

Re: [PATCH 4/8] dt-bindings: iio: dfsdm: move to backend framework

[Jonathan Cameron]

On Tue, 18 Jun 2024 18:08:30 +0200
Olivier Moysan <olivier.moysan@foss.st.com> wrote:

> Change the DFSDM binding to use the new IIO backend framework,
> along with the adoption of IIO generic channels.
> This binding change allows to add scaling support to the DFSDM.
> 
> Keep the legacy binding as deprecated for backward compatibility.
> 
> The io-backends property is supported only in generic IIO channel
> binding.
> 
> - Channel description with the generic binding (Audio and Analog):
> 
>   Properties supersed by generic properties:
superseded

>     st,adc-channels: becomes "reg" property in channel node
>     st,adc-channel-names: becomes "label" property in channel node
>   Properties moved to channel child node:
>     st,adc-channel-types, st,adc-channel-clk-src, st,adc-alt-channel
> 
> - Analog binding:
> 
>   DFSDM filter channel is configured as an IIO backend consumer.
>   Add io-backends property in channel child nodes.
> 
>   DFSDM is no more configured as a channel consumer from SD modulator.
>   Use of io-channels in DFSDM node is deprecated.
> 
> - Audio binding:
> 
>   DFSDM audio DAI is configured as a channel consumer from DFSDM filter.
>   No change compare to legacy.
> 
> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>

Re: [PATCH 7/8] iio: add sd modulator generic iio backend

[Jonathan Cameron]

On Tue, 18 Jun 2024 18:08:33 +0200
Olivier Moysan <olivier.moysan@foss.st.com> wrote:

> Add a generic driver to support sigma delta modulators.
> Typically, this device is a hardware connected to an IIO device
> in charge of the conversion. The device is exposed as an IIO backend
> device. This backend device and the associated conversion device
> can be seen as an aggregate device from IIO framework.
> 
> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>

Trivial comments inline.

> diff --git a/drivers/iio/adc/sd_adc_backend.c b/drivers/iio/adc/sd_adc_backend.c
> new file mode 100644
> index 000000000000..556a49dc537b
> --- /dev/null
> +++ b/drivers/iio/adc/sd_adc_backend.c
> @@ -0,0 +1,110 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Generic sigma delta modulator IIO backend
> + *
> + * Copyright (C) 2024, STMicroelectronics - All Rights Reserved
> + */
> +
> +#include <linux/iio/backend.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +
> +struct iio_sd_backend_priv {
> +	struct regulator *vref;
> +	int vref_mv;
> +};
> +
> +static int sd_backend_enable(struct iio_backend *backend)
> +{
> +	struct iio_sd_backend_priv *priv = iio_backend_get_priv(backend);
> +
> +	return regulator_enable(priv->vref);
> +};
> +
> +static void sd_backend_disable(struct iio_backend *backend)
> +{
> +	struct iio_sd_backend_priv *priv = iio_backend_get_priv(backend);
> +
> +	regulator_disable(priv->vref);
> +};
> +
> +static int sd_backend_read(struct iio_backend *backend, int *val, int *val2, long mask)
Nothing to do with this patch as such:

One day I'm going to bit the bullet and fix that naming.
Long long ago when the Earth was young it actually was a bitmap which
I miscalled a mask - it only had one bit ever set, which was a dumb
bit of API.  It's not been true for a long time.
Anyhow, one more instances isn't too much of a problem I guess.

> +{
> +	struct iio_sd_backend_priv *priv = iio_backend_get_priv(backend);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = priv->vref_mv;

This doesn't really feel right as what are we calling to?  Using it to pass the
reference voltage doesn't make sense under normal handling of these.  So at very
least needs a comment.


> +		*val2 = 0;
No need to set val2.
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_OFFSET:
> +		*val = 0;
> +		*val2 = 0;
> +		return IIO_VAL_INT;
Normally we just don't provide this but I guess you are requiring all of these?
Long term that won't scale, so you need your caller to handle a suitable
error return, -EINVAL will work to say not supported.

> +	}
> +
> +	return -EINVAL;
> +};
> +
> +static const struct iio_backend_ops sd_backend_ops = {
> +	.enable = sd_backend_enable,
> +	.disable = sd_backend_disable,
> +	.read_raw = sd_backend_read,
> +};
> +
> +static int iio_sd_backend_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct regulator *vref;
> +	struct iio_sd_backend_priv *priv;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	vref = devm_regulator_get_optional(dev, "vref");

New devm_regulator_get_enable_read_voltage() slightly simplifies this
and means you don't need to keep vref around.

> +	if (IS_ERR(vref)) {
> +		if (PTR_ERR(vref) != -ENODEV)
> +			return dev_err_probe(dev, PTR_ERR(vref), "Failed to get vref\n");
> +	} else {
> +		ret = regulator_get_voltage(vref);
You haven't turned it on so it's not guaranteed to give you a useful
answer.

Use the enable_read_voltage variant and that will handle this for you.

> +		if (ret < 0)
> +			return ret;
> +
> +		priv->vref = vref;
> +		priv->vref_mv = ret / 1000;
> +	}
> +
> +	ret = devm_iio_backend_register(&pdev->dev, &sd_backend_ops, priv);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

return devm_iio_....

> +};
> +
> +static const struct of_device_id sd_backend_of_match[] = {
> +	{ .compatible = "sd-backend" },
> +	{ .compatible = "ads1201" },

Conor pointed out ti,ads1201
At least I assume ti?

> +	{ }
> +};

Re: [PATCH 8/8] iio: adc: stm32-dfsdm: add scaling support to dfsdm

[Jonathan Cameron]

On Tue, 18 Jun 2024 18:08:34 +0200
Olivier Moysan <olivier.moysan@foss.st.com> wrote:

> Add scaling support to STM32 DFSDM.
Perhaps a short description here of how this works?  Where does the scale come
from, what assumptions are made etc.
> 
> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>

Some minor stuff.

> diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
> index 69b4764d7cba..93bf6035bd6d 100644
> --- a/drivers/iio/adc/stm32-dfsdm-adc.c
> +++ b/drivers/iio/adc/stm32-dfsdm-adc.c
urn 0;
>  }
>  
> @@ -1060,7 +1072,7 @@ static int stm32_dfsdm_update_scan_mode(struct iio_dev *indio_dev,
>  static int stm32_dfsdm_postenable(struct iio_dev *indio_dev)
>  {
>  	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> -	int ret;
> +	int i = 0, ret;

Don't mix assigned and unassigned variable declarations. Just use a separate line
as this can mean subtle assignment or lack of assignment issues sneak in.

>  
>  	/* Reset adc buffer index */
>  	adc->bufi = 0;
> @@ -1071,6 +1083,15 @@ static int stm32_dfsdm_postenable(struct iio_dev *indio_dev)
>  			return ret;
>  	}
>  
> +	if (adc->backend) {
> +		while (adc->backend[i]) {

Could do similar to the suggestion below.
Mostly I don't like the index variable manipulation.

> +			ret = iio_backend_enable(&indio_dev->dev, adc->backend[i]);
> +			if (ret < 0)
> +				return ret;
> +			i++;
> +		}
> +	}
> +
>  	ret = stm32_dfsdm_start_dfsdm(adc->dfsdm);
>  	if (ret < 0)
>  		goto err_stop_hwc;
> @@ -1103,6 +1124,7 @@ static int stm32_dfsdm_postenable(struct iio_dev *indio_dev)
>  static int stm32_dfsdm_predisable(struct iio_dev *indio_dev)
>  {
>  	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> +	int i = 0;
>  
>  	stm32_dfsdm_stop_conv(indio_dev);
>  
> @@ -1110,6 +1132,13 @@ static int stm32_dfsdm_predisable(struct iio_dev *indio_dev)
>  
>  	stm32_dfsdm_stop_dfsdm(adc->dfsdm);
>  
> +	if (adc->backend) {
> +		while (adc->backend[i]) {
> +			iio_backend_disable(&indio_dev->dev, adc->backend[i]);
> +			i++;
> +		}
Something like
		struct iio_backend **be = &adc->backend[0];
		do {
			iio_backend_disable(&indio-dev->dev, be);
		} while (be++);

maybe. Up to you.
		

> +	}

> @@ -1320,6 +1360,45 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
>  		*val = adc->sample_freq;
>  
>  		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		/*
> +		 * Scale is expressed in mV.
> +		 * When fast mode is disabled, actual resolution may be lower
> +		 * than 2^n, where n=realbits-1.

As below, use a few more spaces.

> +		 * This leads to underestimating input voltage. To
> +		 * compensate this deviation, the voltage reference can be
> +		 * corrected with a factor = realbits resolution / actual max
> +		 */
> +		if (adc->backend[idx]) {
> +			iio_backend_read_raw(adc->backend[idx], val, val2, mask);
> +
> +			*val = div_u64((u64)*val * (u64)BIT(DFSDM_DATA_RES - 1), max);
> +			*val2 = chan->scan_type.realbits;
> +			if (chan->differential)
> +				*val *= 2;
> +		}
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +
> +	case IIO_CHAN_INFO_OFFSET:
> +		/*
> +		 * DFSDM output data are in the range [-2^n,2^n],
Use a few more spaces. [-2^2, 2^n]
> +		 * with n=realbits-1.
n = realbits - 1

Just to keep it closer to the C coding style.

> +		 * - Differential modulator:
> +		 * Offset correspond to SD modulator offset.
> +		 * - Single ended modulator:
> +		 * Input is in [0V,Vref] range, where 0V corresponds to -2^n, and Vref to 2^n.

Avoid that long line with a suitable line break.

> +		 * Add 2^n to offset. (i.e. middle of input range)
> +		 * offset = offset(sd) * vref / res(sd) * max / vref.
> +		 */
> +		if (adc->backend[idx]) {
> +			iio_backend_read_raw(adc->backend[idx], val, val2, mask);
> +
> +			*val = div_u64((u64)max * *val, BIT(*val2 - 1));
> +			if (!chan->differential)
> +				*val += max;
> +		}
> +		return IIO_VAL_INT;
>  	}
>  
>  	return -EINVAL;
> @@ -1449,7 +1528,15 @@ static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev, struct iio_c
>  	 * IIO_CHAN_INFO_RAW: used to compute regular conversion
>  	 * IIO_CHAN_INFO_OVERSAMPLING_RATIO: used to set oversampling
>  	 */
> -	ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> +	if (child) {
> +		ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +					BIT(IIO_CHAN_INFO_SCALE) |
> +					BIT(IIO_CHAN_INFO_OFFSET);

Indent looks a little odd. Maybe one more space neede?

> +	} else {
> +		/* Legacy. Scaling not supported */
> +		ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> +	}
> +
>  	ch->info_mask_shared_by_all = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) |
>  					BIT(IIO_CHAN_INFO_SAMP_FREQ);
>  
> @@ -1816,3 +1903,4 @@ module_platform_driver(stm32_dfsdm_adc_driver);
>  MODULE_DESCRIPTION("STM32 sigma delta ADC");
>  MODULE_AUTHOR("Arnaud Pouliquen <arnaud.pouliquen@st.com>");
>  MODULE_LICENSE("GPL v2");
> +MODULE_IMPORT_NS(IIO_BACKEND);

Re: [PATCH 2/8] iio: add enable and disable services to iio backend framework

[Nuno Sá]

On Sun, 2024-06-23 at 14:56 +0100, Jonathan Cameron wrote:
> On Thu, 20 Jun 2024 12:07:47 +0200
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
> > On Wed, 2024-06-19 at 17:59 +0200, Olivier MOYSAN wrote:
> > > Hi Nuno,
> > > 
> > > On 6/19/24 07:21, Nuno Sá wrote:  
> > > > On Tue, 2024-06-18 at 18:08 +0200, Olivier Moysan wrote:  
> > > > > Add iio_backend_disable() and iio_backend_enable() APIs to allow
> > > > > IIO backend consumer to request backend disabling and enabling.
> > > > > 
> > > > > Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
> > > > > ---  
> > > > 
> > > > Hi Olivier,
> > > > 
> > > > small notes from me...
> > > >   
> > > > >   drivers/iio/industrialio-backend.c | 26 ++++++++++++++++++++++++++
> > > > >   include/linux/iio/backend.h        |  2 ++
> > > > >   2 files changed, 28 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/iio/industrialio-backend.c
> > > > > b/drivers/iio/industrialio-
> > > > > backend.c
> > > > > index b950e30018ca..d3db048c086b 100644
> > > > > --- a/drivers/iio/industrialio-backend.c
> > > > > +++ b/drivers/iio/industrialio-backend.c
> > > > > @@ -166,6 +166,32 @@ int devm_iio_backend_enable(struct device *dev,
> > > > > struct
> > > > > iio_backend *back)
> > > > >   }
> > > > >   EXPORT_SYMBOL_NS_GPL(devm_iio_backend_enable, IIO_BACKEND);
> > > > >   
> > > > > +/**
> > > > > + * iio_backend_enable - Backend enable
> > > > > + * @dev: Consumer device for the backend
> > > > > + * @back: Backend device
> > > > > + *
> > > > > + * RETURNS:
> > > > > + * 0 on success, negative error number on failure.
> > > > > + */
> > > > > +int iio_backend_enable(struct device *dev, struct iio_backend *back)
> > > > > +{
> > > > > +	return iio_backend_op_call(back, enable);
> > > > > +}
> > > > > +EXPORT_SYMBOL_NS_GPL(iio_backend_enable, IIO_BACKEND);  
> > > > 
> > > > We do already have devm_iio_backend_enable(). From a correctness stand point
> > > > and even
> > > > scalability, that API should now call your new iio_backend_enable() instead
> > > > of
> > > > directly call iio_backend_op_call(). I guess that change could be in this
> > > > patch.
> > > >   
> > > 
> > > Sure. I have updated the patch.
> > >   
> > > > > +
> > > > > +/**
> > > > > + * iio_backend_disable - Backend disable
> > > > > + * @dev: Consumer device for the backend
> > > > > + * @back: Backend device
> > > > > + *
> > > > > + */
> > > > > +void iio_backend_disable(struct device *dev, struct iio_backend *back)
> > > > > +{
> > > > > +	iio_backend_void_op_call(back, disable);
> > > > > +}
> > > > > +EXPORT_SYMBOL_NS_GPL(iio_backend_disable, IIO_BACKEND);
> > > > > +  
> > > > 
> > > > We also have __iio_backend_disable() which is static since all users were
> > > > using
> > > > devm_iio_backend_enable(). I understand that's not suitable for you but I
> > > > would
> > > > instead rename the existing function to iio_backend_disable() and export it.
> > > >   
> > > 
> > > Just renaming is not sufficient. The reason is that 
> > > devm_add_action_or_reset() require an action with action(void *) 
> > > prototype. So the prototype of iio_backend_disable() has to be changed 
> > > to void iio_backend_disable(void *back).
> > > I placed the same arguments in enable and disable for symmetry, but *dev 
> > > is not required for time being in disable API. So it can make sense to 
> > > change iio_backend_disable() prototype.
> > > alternatively, we can call __iio_backend_disable() through this API.
> > > Please, let me know is you have a preference.
> > >   
> > 
> > Oh, yes, you're right. I would prefer your later option. Call
> > __iio_backend_disable() from __iio_backend_disable() with a proper typed
> ?  That looks like an infinite loop :)  

Ahaha, yes it looks. But hopefully you got what I really meant :)

- Nuno Sá
> 

Re: [PATCH 7/8] iio: add sd modulator generic iio backend

[Olivier MOYSAN]

Hi Jonathan,

On 6/23/24 17:11, Jonathan Cameron wrote:
> On Tue, 18 Jun 2024 18:08:33 +0200
> Olivier Moysan <olivier.moysan@foss.st.com> wrote:
> 
>> Add a generic driver to support sigma delta modulators.
>> Typically, this device is a hardware connected to an IIO device
>> in charge of the conversion. The device is exposed as an IIO backend
>> device. This backend device and the associated conversion device
>> can be seen as an aggregate device from IIO framework.
>>
>> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
> 
> Trivial comments inline.
> 
>> diff --git a/drivers/iio/adc/sd_adc_backend.c b/drivers/iio/adc/sd_adc_backend.c
>> new file mode 100644
>> index 000000000000..556a49dc537b
>> --- /dev/null
>> +++ b/drivers/iio/adc/sd_adc_backend.c
>> @@ -0,0 +1,110 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Generic sigma delta modulator IIO backend
>> + *
>> + * Copyright (C) 2024, STMicroelectronics - All Rights Reserved
>> + */
>> +
>> +#include <linux/iio/backend.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/module.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regulator/consumer.h>
>> +
>> +struct iio_sd_backend_priv {
>> +	struct regulator *vref;
>> +	int vref_mv;
>> +};
>> +
>> +static int sd_backend_enable(struct iio_backend *backend)
>> +{
>> +	struct iio_sd_backend_priv *priv = iio_backend_get_priv(backend);
>> +
>> +	return regulator_enable(priv->vref);
>> +};
>> +
>> +static void sd_backend_disable(struct iio_backend *backend)
>> +{
>> +	struct iio_sd_backend_priv *priv = iio_backend_get_priv(backend);
>> +
>> +	regulator_disable(priv->vref);
>> +};
>> +
>> +static int sd_backend_read(struct iio_backend *backend, int *val, int *val2, long mask)
> Nothing to do with this patch as such:
> 
> One day I'm going to bit the bullet and fix that naming.
> Long long ago when the Earth was young it actually was a bitmap which
> I miscalled a mask - it only had one bit ever set, which was a dumb
> bit of API.  It's not been true for a long time.
> Anyhow, one more instances isn't too much of a problem I guess.
> 

I changed the callback .read_raw to .ext_info_get to take Nuno's comment 
about iio_backend_read_raw() API, into account.
So, I changed this function to
static int sd_backend_ext_info_get(struct iio_backend *back, uintptr_t 
private, const struct iio_chan_spec *chan, char *buf)
for v2 version.

>> +{
>> +	struct iio_sd_backend_priv *priv = iio_backend_get_priv(backend);
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_SCALE:
>> +		*val = priv->vref_mv;
> 
> This doesn't really feel right as what are we calling to?  Using it to pass the
> reference voltage doesn't make sense under normal handling of these.  So at very
> least needs a comment.
> 
> 
>> +		*val2 = 0;
> No need to set val2.

Removed in v2 also.

>> +		return IIO_VAL_INT;
>> +
>> +	case IIO_CHAN_INFO_OFFSET:
>> +		*val = 0;
>> +		*val2 = 0;
>> +		return IIO_VAL_INT;
> Normally we just don't provide this but I guess you are requiring all of these?
> Long term that won't scale, so you need your caller to handle a suitable
> error return, -EINVAL will work to say not supported.
>

Offset support is not strictly required for time being, as reported as 
zero. The aim was to anticipate further needs, but it may be dropped for 
now. I will remove it from v2, If you suggest keeping only the bare 
essentials here.

Error changed to "-EOPNOTSUPP" for unknown requests.

>> +	}
>> +
>> +	return -EINVAL;
>> +};
>> +
>> +static const struct iio_backend_ops sd_backend_ops = {
>> +	.enable = sd_backend_enable,
>> +	.disable = sd_backend_disable,
>> +	.read_raw = sd_backend_read,
>> +};
>> +
>> +static int iio_sd_backend_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct regulator *vref;
>> +	struct iio_sd_backend_priv *priv;
>> +	int ret;
>> +
>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	vref = devm_regulator_get_optional(dev, "vref");
> 
> New devm_regulator_get_enable_read_voltage() slightly simplifies this
> and means you don't need to keep vref around.
> 
>> +	if (IS_ERR(vref)) {
>> +		if (PTR_ERR(vref) != -ENODEV)
>> +			return dev_err_probe(dev, PTR_ERR(vref), "Failed to get vref\n");
>> +	} else {
>> +		ret = regulator_get_voltage(vref);
> You haven't turned it on so it's not guaranteed to give you a useful
> answer.
> 

My understanding is that regulator_get_voltage() always returns the 
regulator voltage, whatever the regulator state, as documented in the 
API description:
"* NOTE: If the regulator is disabled it will return the voltage value.
* This function should not be used to determine regulator state."

So, my logic was to enable the regulator only when requested, through 
enable/disable callbacks to manage power.

Please, let me know if I missed something here.

> Use the enable_read_voltage variant and that will handle this for you.
> 
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		priv->vref = vref;
>> +		priv->vref_mv = ret / 1000;
>> +	}
>> +
>> +	ret = devm_iio_backend_register(&pdev->dev, &sd_backend_ops, priv);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
> 
> return devm_iio_....
> 

Done

>> +};
>> +
>> +static const struct of_device_id sd_backend_of_match[] = {
>> +	{ .compatible = "sd-backend" },
>> +	{ .compatible = "ads1201" },
> 
> Conor pointed out ti,ads1201
> At least I assume ti?
> 

Ack. changed to ti,ads1201

>> +	{ }
>> +};
> 
> 

Regards
Olivier

Re: [PATCH 7/8] iio: add sd modulator generic iio backend

[Nuno Sá]

Hi Olivier,

On Mon, 2024-06-24 at 14:43 +0200, Olivier MOYSAN wrote:
> Hi Jonathan,
> 
> On 6/23/24 17:11, Jonathan Cameron wrote:
> > On Tue, 18 Jun 2024 18:08:33 +0200
> > Olivier Moysan <olivier.moysan@foss.st.com> wrote:
> > 
> > > Add a generic driver to support sigma delta modulators.
> > > Typically, this device is a hardware connected to an IIO device
> > > in charge of the conversion. The device is exposed as an IIO backend
> > > device. This backend device and the associated conversion device
> > > can be seen as an aggregate device from IIO framework.
> > > 
> > > Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
> > 
> > Trivial comments inline.
> > 
> > > diff --git a/drivers/iio/adc/sd_adc_backend.c
> > > b/drivers/iio/adc/sd_adc_backend.c
> > > new file mode 100644
> > > index 000000000000..556a49dc537b
> > > --- /dev/null
> > > +++ b/drivers/iio/adc/sd_adc_backend.c
> > > @@ -0,0 +1,110 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Generic sigma delta modulator IIO backend
> > > + *
> > > + * Copyright (C) 2024, STMicroelectronics - All Rights Reserved
> > > + */
> > > +
> > > +#include <linux/iio/backend.h>
> > > +#include <linux/iio/iio.h>
> > > +#include <linux/module.h>
> > > +#include <linux/mod_devicetable.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/regulator/consumer.h>
> > > +
> > > +struct iio_sd_backend_priv {
> > > +	struct regulator *vref;
> > > +	int vref_mv;
> > > +};
> > > +
> > > +static int sd_backend_enable(struct iio_backend *backend)
> > > +{
> > > +	struct iio_sd_backend_priv *priv = iio_backend_get_priv(backend);
> > > +
> > > +	return regulator_enable(priv->vref);
> > > +};
> > > +
> > > +static void sd_backend_disable(struct iio_backend *backend)
> > > +{
> > > +	struct iio_sd_backend_priv *priv = iio_backend_get_priv(backend);
> > > +
> > > +	regulator_disable(priv->vref);
> > > +};
> > > +
> > > +static int sd_backend_read(struct iio_backend *backend, int *val, int *val2,
> > > long mask)
> > Nothing to do with this patch as such:
> > 
> > One day I'm going to bit the bullet and fix that naming.
> > Long long ago when the Earth was young it actually was a bitmap which
> > I miscalled a mask - it only had one bit ever set, which was a dumb
> > bit of API.  It's not been true for a long time.
> > Anyhow, one more instances isn't too much of a problem I guess.
> > 
> 
> I changed the callback .read_raw to .ext_info_get to take Nuno's comment 
> about iio_backend_read_raw() API, into account.
> So, I changed this function to
> static int sd_backend_ext_info_get(struct iio_backend *back, uintptr_t 
> private, const struct iio_chan_spec *chan, char *buf)
> for v2 version.
> 

Maybe I'm missing something but I think I did not explained myself very well. What I
had in mind was that since you're calling .read_raw() from IIO_CHAN_INFO_SCALE and
IIO_CHAN_INFO_OFFSET, it could make sense to have more dedicated API's. Meaning:

iio_backend_read_scale(...)
iio_backend_read_offset(...)

The iio_backend_read_raw() may make sense when frontends call
iio_backend_extend_chan_spec() and have no idea what the backend may have added to
the channel. So, in those cases something like this could make sense:

switch (mask)
IIO_CHAN_INFO_RAW:

...

default:
	return iio_backend_read_raw();

but like I said maybe this is me over-complicating and a simple
iio_backend_read_raw() is sufficient. But I think I never mentioned something like
.read_raw -> .ext_info_get.

The other thing I mentioned was to instead of having:


if (child) {
	ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
				 BIT(IIO_CHAN_INFO_SCALE) |
				 BIT(IIO_CHAN_INFO_OFFSET);
}

You could use iio_backend_extend_chan_spec() and have the backend set the SCALE and
OFFSET bits for you as it seems these functionality depends on the backend.

But none of the above were critical or things that I feel to strong about.

Anyways, just wanted to give some clarification as it seems there were some
misunderstandings (I think).

- Nuno Sá

> > 

Re: [PATCH 7/8] iio: add sd modulator generic iio backend

[Olivier MOYSAN]

Hi Nuno,

On 6/24/24 17:22, Nuno Sá wrote:
> Hi Olivier,
> 
> On Mon, 2024-06-24 at 14:43 +0200, Olivier MOYSAN wrote:
>> Hi Jonathan,
>>
>> On 6/23/24 17:11, Jonathan Cameron wrote:
>>> On Tue, 18 Jun 2024 18:08:33 +0200
>>> Olivier Moysan <olivier.moysan@foss.st.com> wrote:
>>>
>>>> Add a generic driver to support sigma delta modulators.
>>>> Typically, this device is a hardware connected to an IIO device
>>>> in charge of the conversion. The device is exposed as an IIO backend
>>>> device. This backend device and the associated conversion device
>>>> can be seen as an aggregate device from IIO framework.
>>>>
>>>> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
>>>
>>> Trivial comments inline.
>>>
>>>> diff --git a/drivers/iio/adc/sd_adc_backend.c
>>>> b/drivers/iio/adc/sd_adc_backend.c
>>>> new file mode 100644
>>>> index 000000000000..556a49dc537b
>>>> --- /dev/null
>>>> +++ b/drivers/iio/adc/sd_adc_backend.c
>>>> @@ -0,0 +1,110 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>> +/*
>>>> + * Generic sigma delta modulator IIO backend
>>>> + *
>>>> + * Copyright (C) 2024, STMicroelectronics - All Rights Reserved
>>>> + */
>>>> +
>>>> +#include <linux/iio/backend.h>
>>>> +#include <linux/iio/iio.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/mod_devicetable.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/regulator/consumer.h>
>>>> +
>>>> +struct iio_sd_backend_priv {
>>>> +	struct regulator *vref;
>>>> +	int vref_mv;
>>>> +};
>>>> +
>>>> +static int sd_backend_enable(struct iio_backend *backend)
>>>> +{
>>>> +	struct iio_sd_backend_priv *priv = iio_backend_get_priv(backend);
>>>> +
>>>> +	return regulator_enable(priv->vref);
>>>> +};
>>>> +
>>>> +static void sd_backend_disable(struct iio_backend *backend)
>>>> +{
>>>> +	struct iio_sd_backend_priv *priv = iio_backend_get_priv(backend);
>>>> +
>>>> +	regulator_disable(priv->vref);
>>>> +};
>>>> +
>>>> +static int sd_backend_read(struct iio_backend *backend, int *val, int *val2,
>>>> long mask)
>>> Nothing to do with this patch as such:
>>>
>>> One day I'm going to bit the bullet and fix that naming.
>>> Long long ago when the Earth was young it actually was a bitmap which
>>> I miscalled a mask - it only had one bit ever set, which was a dumb
>>> bit of API.  It's not been true for a long time.
>>> Anyhow, one more instances isn't too much of a problem I guess.
>>>
>>
>> I changed the callback .read_raw to .ext_info_get to take Nuno's comment
>> about iio_backend_read_raw() API, into account.
>> So, I changed this function to
>> static int sd_backend_ext_info_get(struct iio_backend *back, uintptr_t
>> private, const struct iio_chan_spec *chan, char *buf)
>> for v2 version.
>>
> 
> Maybe I'm missing something but I think I did not explained myself very well. What I
> had in mind was that since you're calling .read_raw() from IIO_CHAN_INFO_SCALE and
> IIO_CHAN_INFO_OFFSET, it could make sense to have more dedicated API's. Meaning:
> 
> iio_backend_read_scale(...)
> iio_backend_read_offset(...)
> 
> The iio_backend_read_raw() may make sense when frontends call
> iio_backend_extend_chan_spec() and have no idea what the backend may have added to
> the channel. So, in those cases something like this could make sense:
> 
> switch (mask)
> IIO_CHAN_INFO_RAW:
> 
> ...
> 
> default:
> 	return iio_backend_read_raw();
> 
> but like I said maybe this is me over-complicating and a simple
> iio_backend_read_raw() is sufficient. But I think I never mentioned something like
> .read_raw -> .ext_info_get.
> 

Thanks for clarification. Your previous message was actually clear 
enough regarding iio_backend_read_raw() API.

However, your comment about extend_chan_spec(), let me think that I 
could maybe spare a new API, and just re-use iio_backend_ext_info_get() 
callback.
Nevertheless, this API cannot be used directly, as it can be used only 
for a frontend associated to a single backend. There is a comment in 
iio_backend_ext_info_get() about the need of another API for such case.

So I considered introducing this new API (instead of read_raw):
ssize_t iio_backend_ext_info_get_from_backend(struct iio_backend *back, 
uintptr_t private, const struct iio_chan_spec *chan, char *buf)
(I'm not sure this name is the most relevant).

But if you don't like this alternative too much, I will keep the initial 
"catch all" iio_backend_read_raw() API.

BRs
Olivier

> The other thing I mentioned was to instead of having:
> 
> 
> if (child) {
> 	ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> 				 BIT(IIO_CHAN_INFO_SCALE) |
> 				 BIT(IIO_CHAN_INFO_OFFSET);
> }
> 
> You could use iio_backend_extend_chan_spec() and have the backend set the SCALE and
> OFFSET bits for you as it seems these functionality depends on the backend.
> 
> But none of the above were critical or things that I feel to strong about.
> 
> Anyways, just wanted to give some clarification as it seems there were some
> misunderstandings (I think).
> > - Nuno Sá
> 
>>>

Re: [PATCH 7/8] iio: add sd modulator generic iio backend

[Jonathan Cameron]

> >> +	}
> >> +
> >> +	return -EINVAL;
> >> +};
> >> +
> >> +static const struct iio_backend_ops sd_backend_ops = {
> >> +	.enable = sd_backend_enable,
> >> +	.disable = sd_backend_disable,
> >> +	.read_raw = sd_backend_read,
> >> +};
> >> +
> >> +static int iio_sd_backend_probe(struct platform_device *pdev)
> >> +{
> >> +	struct device *dev = &pdev->dev;
> >> +	struct regulator *vref;
> >> +	struct iio_sd_backend_priv *priv;
> >> +	int ret;
> >> +
> >> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> >> +	if (!priv)
> >> +		return -ENOMEM;
> >> +
> >> +	vref = devm_regulator_get_optional(dev, "vref");  
> > 
> > New devm_regulator_get_enable_read_voltage() slightly simplifies this
> > and means you don't need to keep vref around.
> >   
> >> +	if (IS_ERR(vref)) {
> >> +		if (PTR_ERR(vref) != -ENODEV)
> >> +			return dev_err_probe(dev, PTR_ERR(vref), "Failed to get vref\n");
> >> +	} else {
> >> +		ret = regulator_get_voltage(vref);  
> > You haven't turned it on so it's not guaranteed to give you a useful
> > answer.
> >   
> 
> My understanding is that regulator_get_voltage() always returns the 
> regulator voltage, whatever the regulator state, as documented in the 
> API description:
> "* NOTE: If the regulator is disabled it will return the voltage value.
> * This function should not be used to determine regulator state."
> 
> So, my logic was to enable the regulator only when requested, through 
> enable/disable callbacks to manage power.
> 
> Please, let me know if I missed something here.

Ah ok. I had a vague and it seems incorrect recollection that you had
to turn the regs on to get voltage in some cases.  Ah well. Clearly not :)
What you have is fine.  Add a comment though just so no one replaces
this code with the helper.  

Jonathan

 

Re: [PATCH 5/8] dt-bindings: iio: add sigma delta modulator backend

[Olivier MOYSAN]

Hi Conor,

On 6/18/24 20:13, Conor Dooley wrote:
> On Tue, Jun 18, 2024 at 06:08:31PM +0200, Olivier Moysan wrote:
>> Add documentation of device tree bindings to support
>> sigma delta modulator backend in IIO framework.
>>
>> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
>> ---
>>   .../iio/adc/sd-modulator-backend.yaml         | 43 +++++++++++++++++++
>>   1 file changed, 43 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/iio/adc/sd-modulator-backend.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/sd-modulator-backend.yaml b/Documentation/devicetree/bindings/iio/adc/sd-modulator-backend.yaml
>> new file mode 100644
>> index 000000000000..b0fa71b75cd0
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/sd-modulator-backend.yaml
>> @@ -0,0 +1,43 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/iio/adc/sd-modulator-backend.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Sigma delta modulator backend
>> +
>> +maintainers:
>> +  - Olivier Moysan <olivier.moysan@foss.st.com>
>> +
>> +properties:
>> +  compatible:
>> +    description: |
>> +      "sd-backend" can be used as a generic SD modulator backend,
>> +      if the modulator is not specified in the compatible list.
>> +    enum:
>> +      - sd-backend
> 
> I'd rather not have a generic compatible like this. Something generic as
> a fallback for the driver to binding against I would be fine with, but
> not something that avoids people documenting their devices.
> 

This binding was modeled on the following binding
Documentation/devicetree/bindings/iio/adc/sigma-delta-modulator.yaml
But, I understand that we need to encourage people to use a real 
compatible. So, I will remove this generic compatible from the binding 
in v2.

> Also, I think "backend" should be dropped from the
> filename/title/descriptions, the ads1201 is "just" an delta-sigma
> modulator.:wq
> 

There is already a generic sigma delta modulator driver: 
"sd_adc_modulator.c"
This driver follows a different approach, as it registers an IIO device. 
It has to be kept for backward compatibility.
The current patch introduces a new sigma delta modulator generic driver
based on the new IIO backend framework.
So, we have two drivers dedicated to the same type of hardware, but 
intented to be used with different topologies in IIO.
I used "backend" suffix as a differentiator here.
I did not find a better alternative to manage this diversity.
If, you have another suggestion please let me know.

>> +      - ads1201
> 
> Missing vendor prefix.
> 

Ack

BRs
Olivier

> Thanks,
> Conor.
> 
> 
>> +
>> +  '#io-backend-cells':
>> +    const: 0
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  vref-supply:
>> +    description: Phandle to the vref input analog reference voltage.
>> +
>> +required:
>> +  - compatible
>> +  - '#io-backend-cells'
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    ads1201: adc {
>> +      compatible = "sd-backend";
>> +      #io-backend-cells = <0>;
>> +    };
>> +
>> +...
>> -- 
>> 2.25.1
>>

Re: [PATCH 8/8] iio: adc: stm32-dfsdm: add scaling support to dfsdm

[Olivier MOYSAN]

Hi Jonathan,

On 6/23/24 17:21, Jonathan Cameron wrote:
> On Tue, 18 Jun 2024 18:08:34 +0200
> Olivier Moysan <olivier.moysan@foss.st.com> wrote:
> 
>> Add scaling support to STM32 DFSDM.
> Perhaps a short description here of how this works?  Where does the scale come
> from, what assumptions are made etc.
>>

Sure. Requires more explanations. Done

>> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
> 
> Some minor stuff.
> 
>> diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
>> index 69b4764d7cba..93bf6035bd6d 100644
>> --- a/drivers/iio/adc/stm32-dfsdm-adc.c
>> +++ b/drivers/iio/adc/stm32-dfsdm-adc.c
> urn 0;
>>   }
>>   
>> @@ -1060,7 +1072,7 @@ static int stm32_dfsdm_update_scan_mode(struct iio_dev *indio_dev,
>>   static int stm32_dfsdm_postenable(struct iio_dev *indio_dev)
>>   {
>>   	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
>> -	int ret;
>> +	int i = 0, ret;
> 
> Don't mix assigned and unassigned variable declarations. Just use a separate line
> as this can mean subtle assignment or lack of assignment issues sneak in.
> 

Ack

>>   
>>   	/* Reset adc buffer index */
>>   	adc->bufi = 0;
>> @@ -1071,6 +1083,15 @@ static int stm32_dfsdm_postenable(struct iio_dev *indio_dev)
>>   			return ret;
>>   	}
>>   
>> +	if (adc->backend) {
>> +		while (adc->backend[i]) {
> 
> Could do similar to the suggestion below.
> Mostly I don't like the index variable manipulation.
> 
>> +			ret = iio_backend_enable(&indio_dev->dev, adc->backend[i]);
>> +			if (ret < 0)
>> +				return ret;
>> +			i++;
>> +		}
>> +	}
>> +
>>   	ret = stm32_dfsdm_start_dfsdm(adc->dfsdm);
>>   	if (ret < 0)
>>   		goto err_stop_hwc;
>> @@ -1103,6 +1124,7 @@ static int stm32_dfsdm_postenable(struct iio_dev *indio_dev)
>>   static int stm32_dfsdm_predisable(struct iio_dev *indio_dev)
>>   {
>>   	struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
>> +	int i = 0;
>>   
>>   	stm32_dfsdm_stop_conv(indio_dev);
>>   
>> @@ -1110,6 +1132,13 @@ static int stm32_dfsdm_predisable(struct iio_dev *indio_dev)
>>   
>>   	stm32_dfsdm_stop_dfsdm(adc->dfsdm);
>>   
>> +	if (adc->backend) {
>> +		while (adc->backend[i]) {
>> +			iio_backend_disable(&indio_dev->dev, adc->backend[i]);
>> +			i++;
>> +		}
> Something like
> 		struct iio_backend **be = &adc->backend[0];
> 		do {
> 			iio_backend_disable(&indio-dev->dev, be);
> 		} while (be++);
> 
> maybe. Up to you.
> 

If you don't mind, I will keep indexes. Pointers here looks more 
difficult to read, and pre/post incrementation in the loop condition can 
easily introduce bugs, imho.
		
> 
>> +	}
> 
>> @@ -1320,6 +1360,45 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
>>   		*val = adc->sample_freq;
>>   
>>   		return IIO_VAL_INT;
>> +
>> +	case IIO_CHAN_INFO_SCALE:
>> +		/*
>> +		 * Scale is expressed in mV.
>> +		 * When fast mode is disabled, actual resolution may be lower
>> +		 * than 2^n, where n=realbits-1.
> 
> As below, use a few more spaces.
> 
>> +		 * This leads to underestimating input voltage. To
>> +		 * compensate this deviation, the voltage reference can be
>> +		 * corrected with a factor = realbits resolution / actual max
>> +		 */
>> +		if (adc->backend[idx]) {
>> +			iio_backend_read_raw(adc->backend[idx], val, val2, mask);
>> +
>> +			*val = div_u64((u64)*val * (u64)BIT(DFSDM_DATA_RES - 1), max);
>> +			*val2 = chan->scan_type.realbits;
>> +			if (chan->differential)
>> +				*val *= 2;
>> +		}
>> +		return IIO_VAL_FRACTIONAL_LOG2;
>> +
>> +	case IIO_CHAN_INFO_OFFSET:
>> +		/*
>> +		 * DFSDM output data are in the range [-2^n,2^n],
> Use a few more spaces. [-2^2, 2^n]
>> +		 * with n=realbits-1.
> n = realbits - 1
> 
> Just to keep it closer to the C coding style.
> 

Done

>> +		 * - Differential modulator:
>> +		 * Offset correspond to SD modulator offset.
>> +		 * - Single ended modulator:
>> +		 * Input is in [0V,Vref] range, where 0V corresponds to -2^n, and Vref to 2^n.
> 
> Avoid that long line with a suitable line break.
> 

Ack

>> +		 * Add 2^n to offset. (i.e. middle of input range)
>> +		 * offset = offset(sd) * vref / res(sd) * max / vref.
>> +		 */
>> +		if (adc->backend[idx]) {
>> +			iio_backend_read_raw(adc->backend[idx], val, val2, mask);
>> +
>> +			*val = div_u64((u64)max * *val, BIT(*val2 - 1));
>> +			if (!chan->differential)
>> +				*val += max;
>> +		}
>> +		return IIO_VAL_INT;
>>   	}
>>   
>>   	return -EINVAL;
>> @@ -1449,7 +1528,15 @@ static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev, struct iio_c
>>   	 * IIO_CHAN_INFO_RAW: used to compute regular conversion
>>   	 * IIO_CHAN_INFO_OVERSAMPLING_RATIO: used to set oversampling
>>   	 */
>> -	ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
>> +	if (child) {
>> +		ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> +					BIT(IIO_CHAN_INFO_SCALE) |
>> +					BIT(IIO_CHAN_INFO_OFFSET);
> 
> Indent looks a little odd. Maybe one more space neede?
> 

Ack

>> +	} else {
>> +		/* Legacy. Scaling not supported */
>> +		ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
>> +	}
>> +
>>   	ch->info_mask_shared_by_all = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) |
>>   					BIT(IIO_CHAN_INFO_SAMP_FREQ);
>>   
>> @@ -1816,3 +1903,4 @@ module_platform_driver(stm32_dfsdm_adc_driver);
>>   MODULE_DESCRIPTION("STM32 sigma delta ADC");
>>   MODULE_AUTHOR("Arnaud Pouliquen <arnaud.pouliquen@st.com>");
>>   MODULE_LICENSE("GPL v2");
>> +MODULE_IMPORT_NS(IIO_BACKEND);
> 

BRs
Olivier

Re: [PATCH 7/8] iio: add sd modulator generic iio backend

[Nuno Sá]

On Mon, 2024-06-24 at 18:26 +0200, Olivier MOYSAN wrote:
> Hi Nuno,
> 
> On 6/24/24 17:22, Nuno Sá wrote:
> > Hi Olivier,
> > 
> > On Mon, 2024-06-24 at 14:43 +0200, Olivier MOYSAN wrote:
> > > Hi Jonathan,
> > > 
> > > On 6/23/24 17:11, Jonathan Cameron wrote:
> > > > On Tue, 18 Jun 2024 18:08:33 +0200
> > > > Olivier Moysan <olivier.moysan@foss.st.com> wrote:
> > > > 
> > > > > Add a generic driver to support sigma delta modulators.
> > > > > Typically, this device is a hardware connected to an IIO device
> > > > > in charge of the conversion. The device is exposed as an IIO backend
> > > > > device. This backend device and the associated conversion device
> > > > > can be seen as an aggregate device from IIO framework.
> > > > > 
> > > > > Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
> > > > 
> > > > Trivial comments inline.
> > > > 
> > > > > diff --git a/drivers/iio/adc/sd_adc_backend.c
> > > > > b/drivers/iio/adc/sd_adc_backend.c
> > > > > new file mode 100644
> > > > > index 000000000000..556a49dc537b
> > > > > --- /dev/null
> > > > > +++ b/drivers/iio/adc/sd_adc_backend.c
> > > > > @@ -0,0 +1,110 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > > +/*
> > > > > + * Generic sigma delta modulator IIO backend
> > > > > + *
> > > > > + * Copyright (C) 2024, STMicroelectronics - All Rights Reserved
> > > > > + */
> > > > > +
> > > > > +#include <linux/iio/backend.h>
> > > > > +#include <linux/iio/iio.h>
> > > > > +#include <linux/module.h>
> > > > > +#include <linux/mod_devicetable.h>
> > > > > +#include <linux/platform_device.h>
> > > > > +#include <linux/regulator/consumer.h>
> > > > > +
> > > > > +struct iio_sd_backend_priv {
> > > > > +	struct regulator *vref;
> > > > > +	int vref_mv;
> > > > > +};
> > > > > +
> > > > > +static int sd_backend_enable(struct iio_backend *backend)
> > > > > +{
> > > > > +	struct iio_sd_backend_priv *priv =
> > > > > iio_backend_get_priv(backend);
> > > > > +
> > > > > +	return regulator_enable(priv->vref);
> > > > > +};
> > > > > +
> > > > > +static void sd_backend_disable(struct iio_backend *backend)
> > > > > +{
> > > > > +	struct iio_sd_backend_priv *priv =
> > > > > iio_backend_get_priv(backend);
> > > > > +
> > > > > +	regulator_disable(priv->vref);
> > > > > +};
> > > > > +
> > > > > +static int sd_backend_read(struct iio_backend *backend, int *val, int
> > > > > *val2,
> > > > > long mask)
> > > > Nothing to do with this patch as such:
> > > > 
> > > > One day I'm going to bit the bullet and fix that naming.
> > > > Long long ago when the Earth was young it actually was a bitmap which
> > > > I miscalled a mask - it only had one bit ever set, which was a dumb
> > > > bit of API.  It's not been true for a long time.
> > > > Anyhow, one more instances isn't too much of a problem I guess.
> > > > 
> > > 
> > > I changed the callback .read_raw to .ext_info_get to take Nuno's comment
> > > about iio_backend_read_raw() API, into account.
> > > So, I changed this function to
> > > static int sd_backend_ext_info_get(struct iio_backend *back, uintptr_t
> > > private, const struct iio_chan_spec *chan, char *buf)
> > > for v2 version.
> > > 
> > 
> > Maybe I'm missing something but I think I did not explained myself very
> > well. What I
> > had in mind was that since you're calling .read_raw() from
> > IIO_CHAN_INFO_SCALE and
> > IIO_CHAN_INFO_OFFSET, it could make sense to have more dedicated API's.
> > Meaning:
> > 
> > iio_backend_read_scale(...)
> > iio_backend_read_offset(...)
> > 
> > The iio_backend_read_raw() may make sense when frontends call
> > iio_backend_extend_chan_spec() and have no idea what the backend may have
> > added to
> > the channel. So, in those cases something like this could make sense:
> > 
> > switch (mask)
> > IIO_CHAN_INFO_RAW:
> > 
> > ...
> > 
> > default:
> > 	return iio_backend_read_raw();
> > 
> > but like I said maybe this is me over-complicating and a simple
> > iio_backend_read_raw() is sufficient. But I think I never mentioned
> > something like
> > .read_raw -> .ext_info_get.
> > 
> 
> Thanks for clarification. Your previous message was actually clear 
> enough regarding iio_backend_read_raw() API.
> 
> However, your comment about extend_chan_spec(), let me think that I 
> could maybe spare a new API, and just re-use iio_backend_ext_info_get() 
> callback.
> Nevertheless, this API cannot be used directly, as it can be used only 
> for a frontend associated to a single backend. There is a comment in 
> iio_backend_ext_info_get() about the need of another API for such case.
> 
> So I considered introducing this new API (instead of read_raw):
> ssize_t iio_backend_ext_info_get_from_backend(struct iio_backend *back, 
> uintptr_t private, const struct iio_chan_spec *chan, char *buf)
> (I'm not sure this name is the most relevant).

Yeah, don't think that's the way to go... If you have multiple backends the idea
is to add a .get_backend() callback into struct iio_info so we can get the
backend handle of the frontend device. It was not done because we still don't
have a valid user for such a callback.

But having the said the above, I also don't think we should use any extended
info API to handle scale and offset as those are standard attributes. That would
open a dangerous precedence :).
 
> 
> But if you don't like this alternative too much, I will keep the initial 
> "catch all" iio_backend_read_raw() API.

Right...

- Nuno Sá
> > >