* [RFC v2 00/11] iio: add iio backend device type
@ 2023-07-27 15:03 ` Olivier Moysan
0 siblings, 0 replies; 40+ messages in thread
From: Olivier Moysan @ 2023-07-27 15:03 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Olivier Moysan,
Arnaud Pouliquen, Maxime Coquelin, Alexandre Torgue,
Frank Rowand, Liam Girdwood, Mark Brown, Fabrice Gasnier
Cc: linux-iio, devicetree, linux-kernel, alsa-devel, linux-stm32,
linux-arm-kernel
This v2 is an addon to initial RFC:
https://lore.kernel.org/lkml/20230623140944.2613002-1-olivier.moysan@foss.st.com/
Despite the "IIO backend" naming has to be changed (as pointed out by
Jonathan previously), it has been kept here, for time being. The
appropriated naming still has to be discussed later on.
In the previous RFC the "IIO backend" concept was proposed through
a set of template APIs.
This v2 implements a functionnal exemple based on STM32 DFSDM,
to bring scaling support to this peripheral.
Olivier Moysan (11):
iio: introduce iio backend device
of: property: add device link support for io-backends
dt-bindings: iio: stm32-dfsdm-adc: add scaling support
dt-bindings: iio: adc: add scaling support to sd modulator
iio: adc: stm32-dfsdm: manage dfsdm as a channel provider
iio: adc: stm32-dfsdm: adopt generic channel bindings
iio: adc: stm32-dfsdm: add scaling support to dfsdm
iio: adc: sd modulator: add scale and offset support
ARM: dts: stm32: adopt new dfsdm bindings on stm32mp151
ARM: dts: stm32: add dfsdm pins muxing on stm32mp15
ARM: dts: stm32: add dfsdm iio support on stm32mp157c-ev
.../iio/adc/sigma-delta-modulator.yaml | 9 +-
.../bindings/iio/adc/st,stm32-dfsdm-adc.yaml | 189 ++++++------------
arch/arm/boot/dts/st/stm32mp15-pinctrl.dtsi | 39 ++++
arch/arm/boot/dts/st/stm32mp151.dtsi | 18 +-
arch/arm/boot/dts/st/stm32mp157c-ev1.dts | 68 +++++++
drivers/iio/Makefile | 1 +
drivers/iio/adc/sd_adc_modulator.c | 106 ++++++++--
drivers/iio/adc/stm32-dfsdm-adc.c | 187 +++++++++++------
drivers/iio/industrialio-backend.c | 107 ++++++++++
drivers/of/property.c | 2 +
include/linux/iio/backend.h | 56 ++++++
11 files changed, 561 insertions(+), 221 deletions(-)
create mode 100644 drivers/iio/industrialio-backend.c
create mode 100644 include/linux/iio/backend.h
--
2.25.1
^ permalink raw reply [flat|nested] 40+ messages in thread
* [RFC v2 00/11] iio: add iio backend device type
@ 2023-07-27 15:03 ` Olivier Moysan
0 siblings, 0 replies; 40+ messages in thread
From: Olivier Moysan @ 2023-07-27 15:03 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Olivier Moysan,
Arnaud Pouliquen, Maxime Coquelin, Alexandre Torgue,
Frank Rowand, Liam Girdwood, Mark Brown, Fabrice Gasnier
Cc: linux-iio, devicetree, linux-kernel, alsa-devel, linux-stm32,
linux-arm-kernel
This v2 is an addon to initial RFC:
https://lore.kernel.org/lkml/20230623140944.2613002-1-olivier.moysan@foss.st.com/
Despite the "IIO backend" naming has to be changed (as pointed out by
Jonathan previously), it has been kept here, for time being. The
appropriated naming still has to be discussed later on.
In the previous RFC the "IIO backend" concept was proposed through
a set of template APIs.
This v2 implements a functionnal exemple based on STM32 DFSDM,
to bring scaling support to this peripheral.
Olivier Moysan (11):
iio: introduce iio backend device
of: property: add device link support for io-backends
dt-bindings: iio: stm32-dfsdm-adc: add scaling support
dt-bindings: iio: adc: add scaling support to sd modulator
iio: adc: stm32-dfsdm: manage dfsdm as a channel provider
iio: adc: stm32-dfsdm: adopt generic channel bindings
iio: adc: stm32-dfsdm: add scaling support to dfsdm
iio: adc: sd modulator: add scale and offset support
ARM: dts: stm32: adopt new dfsdm bindings on stm32mp151
ARM: dts: stm32: add dfsdm pins muxing on stm32mp15
ARM: dts: stm32: add dfsdm iio support on stm32mp157c-ev
.../iio/adc/sigma-delta-modulator.yaml | 9 +-
.../bindings/iio/adc/st,stm32-dfsdm-adc.yaml | 189 ++++++------------
arch/arm/boot/dts/st/stm32mp15-pinctrl.dtsi | 39 ++++
arch/arm/boot/dts/st/stm32mp151.dtsi | 18 +-
arch/arm/boot/dts/st/stm32mp157c-ev1.dts | 68 +++++++
drivers/iio/Makefile | 1 +
drivers/iio/adc/sd_adc_modulator.c | 106 ++++++++--
drivers/iio/adc/stm32-dfsdm-adc.c | 187 +++++++++++------
drivers/iio/industrialio-backend.c | 107 ++++++++++
drivers/of/property.c | 2 +
include/linux/iio/backend.h | 56 ++++++
11 files changed, 561 insertions(+), 221 deletions(-)
create mode 100644 drivers/iio/industrialio-backend.c
create mode 100644 include/linux/iio/backend.h
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread
* [RFC v2 01/11] iio: introduce iio backend device
2023-07-27 15:03 ` Olivier Moysan
(?)
@ 2023-07-27 15:03 ` Olivier Moysan
2023-07-28 8:42 ` Nuno Sá
-1 siblings, 1 reply; 40+ messages in thread
From: Olivier Moysan @ 2023-07-27 15:03 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen
Cc: Olivier Moysan, linux-kernel, linux-iio
Add a new device type in IIO framework.
This backend device does not compute channel attributes and does not expose
them through sysfs, as done typically in iio-rescale frontend device.
Instead, it allows to report information applying to channel
attributes through callbacks. These backend devices can be cascaded
to represent chained components.
An IIO device configured as a consumer of a backend device can compute
the channel attributes of the whole chain.
Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
---
drivers/iio/Makefile | 1 +
drivers/iio/industrialio-backend.c | 107 +++++++++++++++++++++++++++++
include/linux/iio/backend.h | 56 +++++++++++++++
3 files changed, 164 insertions(+)
create mode 100644 drivers/iio/industrialio-backend.c
create mode 100644 include/linux/iio/backend.h
diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
index 9622347a1c1b..9b59c6ab1738 100644
--- a/drivers/iio/Makefile
+++ b/drivers/iio/Makefile
@@ -5,6 +5,7 @@
obj-$(CONFIG_IIO) += industrialio.o
industrialio-y := industrialio-core.o industrialio-event.o inkern.o
+industrialio-$(CONFIG_IIO_BACKEND) += industrialio-backend.o
industrialio-$(CONFIG_IIO_BUFFER) += industrialio-buffer.o
industrialio-$(CONFIG_IIO_TRIGGER) += industrialio-trigger.o
diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c
new file mode 100644
index 000000000000..7d0625889873
--- /dev/null
+++ b/drivers/iio/industrialio-backend.c
@@ -0,0 +1,107 @@
+// SPDX-License-Identifier: GPL-2.0
+/* The industrial I/O core, backend handling functions
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/device.h>
+#include <linux/property.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/backend.h>
+
+static DEFINE_IDA(iio_backend_ida);
+
+#define to_iio_backend(_device) container_of((_device), struct iio_backend, dev)
+
+static void iio_backend_release(struct device *device)
+{
+ struct iio_backend *backend = to_iio_backend(device);
+
+ kfree(backend->name);
+ kfree(backend);
+}
+
+static const struct device_type iio_backend_type = {
+ .release = iio_backend_release,
+ .name = "iio_backend_device",
+};
+
+struct iio_backend *iio_backend_alloc(struct device *parent)
+{
+ struct iio_backend *backend;
+
+ backend = devm_kzalloc(parent, sizeof(*backend), GFP_KERNEL);
+
+ backend->dev.parent = parent;
+ backend->dev.type = &iio_backend_type;
+ backend->dev.bus = &iio_bus_type;
+ device_initialize(&backend->dev);
+
+ return backend;
+};
+EXPORT_SYMBOL(iio_backend_alloc);
+
+void iio_backend_free(struct device *dev)
+{
+ if (dev)
+ put_device(dev);
+}
+EXPORT_SYMBOL(iio_backend_free);
+
+int iio_backend_register(struct iio_backend *backend)
+{
+ struct fwnode_handle *fwnode;
+ int id;
+
+ id = ida_alloc(&iio_backend_ida, GFP_KERNEL);
+ if (id < 0)
+ return id;
+ backend->id = id;
+
+ dev_set_name(&backend->dev, "backend%d", backend->id);
+
+ if (dev_fwnode(&backend->dev))
+ fwnode = dev_fwnode(&backend->dev);
+ else
+ fwnode = dev_fwnode(backend->dev.parent);
+ device_set_node(&backend->dev, fwnode);
+
+ return device_add(&backend->dev);
+};
+EXPORT_SYMBOL(iio_backend_register);
+
+void iio_backend_unregister(struct iio_backend *backend)
+{
+ device_del(&backend->dev);
+};
+EXPORT_SYMBOL(iio_backend_unregister);
+
+struct iio_backend *fwnode_iio_backend_get(struct fwnode_handle *fwnode, int index)
+{
+ struct iio_backend *backend;
+ struct fwnode_reference_args iiospec;
+ struct device *bdev;
+ int ret;
+
+ if (index < 0)
+ return ERR_PTR(-EINVAL);
+
+ ret = fwnode_property_get_reference_args(fwnode, "io-backends",
+ "#io-backend-cells", 0,
+ index, &iiospec);
+ if (ret)
+ return ERR_PTR(ret);
+
+ bdev = bus_find_device_by_fwnode(&iio_bus_type, iiospec.fwnode);
+ if (!bdev) {
+ fwnode_handle_put(iiospec.fwnode);
+ return ERR_PTR(-EPROBE_DEFER);
+ }
+
+ backend = to_iio_backend(bdev);
+
+ fwnode_handle_put(iiospec.fwnode);
+
+ return backend;
+};
+EXPORT_SYMBOL(fwnode_iio_backend_get);
diff --git a/include/linux/iio/backend.h b/include/linux/iio/backend.h
new file mode 100644
index 000000000000..d90d345a4625
--- /dev/null
+++ b/include/linux/iio/backend.h
@@ -0,0 +1,56 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * The industrial I/O core, backend handling functions
+ *
+ * Author: Olivier Moysan <olivier.moysan@foss.st.com>.
+ */
+
+#ifndef _IIO_BACKEND_H_
+#define _IIO_BACKEND_H_
+#include <linux/iio/iio.h>
+
+struct iio_backend_ops;
+struct iio_backend;
+
+/**
+ * struct iio_backend_ops - operations structure for an iio_backend
+ * @enable: switch on backend
+ * @disable: switch off backend
+ * @read_raw: read data from backend
+ **/
+struct iio_backend_ops {
+ int (*enable)(struct iio_backend *backend);
+ int (*disable)(struct iio_backend *backend);
+ int (*read_raw)(struct iio_backend *backend, int *val, int *val2, long mask);
+};
+
+/**
+ * truct iio_backend - industrial I/O backend device
+ * @ops: operations structure
+ * @module: owner of this driver module
+ * @id: unique id number
+ * @name: unique name
+ * @dev: associated device
+ * @priv: private data pointer
+ **/
+struct iio_backend {
+ const struct iio_backend_ops *ops;
+ struct module *owner;
+ int id;
+ const char *name;
+ struct device dev;
+ struct list_head list;
+ void *priv;
+};
+
+struct iio_backend *iio_backend_alloc(struct device *parent);
+
+void iio_backend_free(struct device *dev);
+
+int iio_backend_register(struct iio_backend *backend);
+
+void iio_backend_unregister(struct iio_backend *backend);
+
+struct iio_backend *fwnode_iio_backend_get(struct fwnode_handle *fwnode, int index);
+
+#endif /* _IIO_BACKEND_H_ */
--
2.25.1
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [RFC v2 02/11] of: property: add device link support for io-backends
2023-07-27 15:03 ` Olivier Moysan
(?)
(?)
@ 2023-07-27 15:03 ` Olivier Moysan
-1 siblings, 0 replies; 40+ messages in thread
From: Olivier Moysan @ 2023-07-27 15:03 UTC (permalink / raw)
To: Rob Herring, Frank Rowand; +Cc: Olivier Moysan, devicetree, linux-kernel
Add support for creating device links out of more DT properties.
Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
---
drivers/of/property.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/of/property.c b/drivers/of/property.c
index ddc75cd50825..864e29e4707c 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1244,6 +1244,7 @@ DEFINE_SIMPLE_PROP(interconnects, "interconnects", "#interconnect-cells")
DEFINE_SIMPLE_PROP(iommus, "iommus", "#iommu-cells")
DEFINE_SIMPLE_PROP(mboxes, "mboxes", "#mbox-cells")
DEFINE_SIMPLE_PROP(io_channels, "io-channel", "#io-channel-cells")
+DEFINE_SIMPLE_PROP(io_backends, "io-backend", "#io-backend-cells")
DEFINE_SIMPLE_PROP(interrupt_parent, "interrupt-parent", NULL)
DEFINE_SIMPLE_PROP(dmas, "dmas", "#dma-cells")
DEFINE_SIMPLE_PROP(power_domains, "power-domains", "#power-domain-cells")
@@ -1332,6 +1333,7 @@ static const struct supplier_bindings of_supplier_bindings[] = {
{ .parse_prop = parse_iommu_maps, .optional = true, },
{ .parse_prop = parse_mboxes, },
{ .parse_prop = parse_io_channels, },
+ { .parse_prop = parse_io_backends, },
{ .parse_prop = parse_interrupt_parent, },
{ .parse_prop = parse_dmas, .optional = true, },
{ .parse_prop = parse_power_domains, },
--
2.25.1
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [RFC v2 03/11] dt-bindings: iio: stm32-dfsdm-adc: add scaling support
2023-07-27 15:03 ` Olivier Moysan
@ 2023-07-27 15:03 ` Olivier Moysan
-1 siblings, 0 replies; 40+ messages in thread
From: Olivier Moysan @ 2023-07-27 15:03 UTC (permalink / raw)
To: Olivier Moysan, Arnaud Pouliquen, Jonathan Cameron,
Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Maxime Coquelin, Alexandre Torgue, Fabrice Gasnier
Cc: alsa-devel, linux-iio, devicetree, linux-stm32, linux-arm-kernel,
linux-kernel
Add scaling support to STM32 DFSDM.
This introduces the following changes:
- Add ADC generic channel binding and remove support of deprecated
channel bindings.
- DFSDM is now implemented as a channel provider, so remove io-channels
properties.
- Add iio-backend property to connect DFSDM to an SD modulator.
Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
---
.../bindings/iio/adc/st,stm32-dfsdm-adc.yaml | 189 ++++++------------
1 file changed, 63 insertions(+), 126 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 1970503389aa..128545cedc7f 100644
--- a/Documentation/devicetree/bindings/iio/adc/st,stm32-dfsdm-adc.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/st,stm32-dfsdm-adc.yaml
@@ -85,22 +85,14 @@ patternProperties:
description: Specifies the DFSDM filter instance used.
maxItems: 1
- interrupts:
- maxItems: 1
+ '#address-cells':
+ const: 1
- st,adc-channels:
- description: |
- List of single-ended channels muxed for this ADC.
- On stm32h7 and stm32mp1:
- - For st,stm32-dfsdm-adc: up to 8 channels numbered from 0 to 7.
- - For st,stm32-dfsdm-dmic: 1 channel numbered from 0 to 7.
- $ref: /schemas/types.yaml#/definitions/uint32-array
- items:
- minimum: 0
- maximum: 7
+ '#size-cells':
+ const: 0
- st,adc-channel-names:
- description: List of single-ended channel names.
+ interrupts:
+ maxItems: 1
st,filter-order:
description: |
@@ -111,39 +103,6 @@ patternProperties:
$ref: /schemas/types.yaml#/definitions/uint32
maximum: 5
- "#io-channel-cells":
- const: 1
-
- 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
-
st,filter0-sync:
description:
Set to 1 to synchronize with DFSDM filter instance 0.
@@ -157,14 +116,68 @@ patternProperties:
items:
- const: rx
+ 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: 19
+
+ label:
+ description: |
+ Unique name to identify channel.
+
+ 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: |
+ phandle to an external sigma delta modulator or internal ADC output.
+ $ref: /schemas/types.yaml#/definitions/phandle
+
+ required:
+ - reg
+ - io-backends
+
+ additionalProperties: false
+
required:
- compatible
- reg
- interrupts
- - st,adc-channels
- - st,adc-channel-names
- st,filter-order
- - "#io-channel-cells"
allOf:
- if:
@@ -175,14 +188,6 @@ patternProperties:
then:
properties:
- st,adc-channels:
- minItems: 1
- maxItems: 8
-
- st,adc-channel-names:
- minItems: 1
- maxItems: 8
-
st,adc-channel-types:
minItems: 1
maxItems: 8
@@ -191,14 +196,6 @@ patternProperties:
minItems: 1
maxItems: 8
- io-channels:
- description:
- From common IIO binding. Used to pipe external sigma delta
- modulator or internal ADC output to DFSDM channel.
-
- required:
- - io-channels
-
- if:
properties:
compatible:
@@ -207,12 +204,6 @@ patternProperties:
then:
properties:
- st,adc-channels:
- maxItems: 1
-
- st,adc-channel-names:
- maxItems: 1
-
st,adc-channel-types:
maxItems: 1
@@ -237,15 +228,9 @@ patternProperties:
"#sound-dai-cells":
const: 0
- io-channels:
- description:
- From common IIO binding. Used to pipe external sigma delta
- modulator or internal ADC output to DFSDM channel.
-
required:
- compatible
- "#sound-dai-cells"
- - io-channels
allOf:
- if:
@@ -278,52 +263,4 @@ allOf:
minimum: 0
maximum: 5
-examples:
- - |
- #include <dt-bindings/interrupt-controller/arm-gic.h>
- #include <dt-bindings/clock/stm32mp1-clks.h>
- dfsdm: 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>;
-
- 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>;
- st,adc-channels = <1>;
- st,adc-channel-names = "dmic0";
- st,adc-channel-types = "SPI_R";
- st,adc-channel-clk-src = "CLKOUT";
- st,filter-order = <5>;
-
- asoc_pdm0: dfsdm-dai {
- compatible = "st,stm32h7-dfsdm-dai";
- #sound-dai-cells = <0>;
- io-channels = <&dfsdm0 0>;
- };
- };
-
- dfsdm_pdm1: 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";
- #io-channel-cells = <1>;
- st,adc-channels = <2 3>;
- st,adc-channel-names = "in2", "in3";
- st,adc-channel-types = "SPI_R", "SPI_R";
- st,adc-channel-clk-src = "CLKOUT_F", "CLKOUT_F";
- io-channels = <&sd_adc2 &sd_adc3>;
- st,filter-order = <1>;
- };
- };
-
...
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [RFC v2 03/11] dt-bindings: iio: stm32-dfsdm-adc: add scaling support
@ 2023-07-27 15:03 ` Olivier Moysan
0 siblings, 0 replies; 40+ messages in thread
From: Olivier Moysan @ 2023-07-27 15:03 UTC (permalink / raw)
To: Olivier Moysan, Arnaud Pouliquen, Jonathan Cameron,
Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Maxime Coquelin, Alexandre Torgue, Fabrice Gasnier
Cc: alsa-devel, linux-iio, devicetree, linux-stm32, linux-arm-kernel,
linux-kernel
Add scaling support to STM32 DFSDM.
This introduces the following changes:
- Add ADC generic channel binding and remove support of deprecated
channel bindings.
- DFSDM is now implemented as a channel provider, so remove io-channels
properties.
- Add iio-backend property to connect DFSDM to an SD modulator.
Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
---
.../bindings/iio/adc/st,stm32-dfsdm-adc.yaml | 189 ++++++------------
1 file changed, 63 insertions(+), 126 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 1970503389aa..128545cedc7f 100644
--- a/Documentation/devicetree/bindings/iio/adc/st,stm32-dfsdm-adc.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/st,stm32-dfsdm-adc.yaml
@@ -85,22 +85,14 @@ patternProperties:
description: Specifies the DFSDM filter instance used.
maxItems: 1
- interrupts:
- maxItems: 1
+ '#address-cells':
+ const: 1
- st,adc-channels:
- description: |
- List of single-ended channels muxed for this ADC.
- On stm32h7 and stm32mp1:
- - For st,stm32-dfsdm-adc: up to 8 channels numbered from 0 to 7.
- - For st,stm32-dfsdm-dmic: 1 channel numbered from 0 to 7.
- $ref: /schemas/types.yaml#/definitions/uint32-array
- items:
- minimum: 0
- maximum: 7
+ '#size-cells':
+ const: 0
- st,adc-channel-names:
- description: List of single-ended channel names.
+ interrupts:
+ maxItems: 1
st,filter-order:
description: |
@@ -111,39 +103,6 @@ patternProperties:
$ref: /schemas/types.yaml#/definitions/uint32
maximum: 5
- "#io-channel-cells":
- const: 1
-
- 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
-
st,filter0-sync:
description:
Set to 1 to synchronize with DFSDM filter instance 0.
@@ -157,14 +116,68 @@ patternProperties:
items:
- const: rx
+ 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: 19
+
+ label:
+ description: |
+ Unique name to identify channel.
+
+ 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: |
+ phandle to an external sigma delta modulator or internal ADC output.
+ $ref: /schemas/types.yaml#/definitions/phandle
+
+ required:
+ - reg
+ - io-backends
+
+ additionalProperties: false
+
required:
- compatible
- reg
- interrupts
- - st,adc-channels
- - st,adc-channel-names
- st,filter-order
- - "#io-channel-cells"
allOf:
- if:
@@ -175,14 +188,6 @@ patternProperties:
then:
properties:
- st,adc-channels:
- minItems: 1
- maxItems: 8
-
- st,adc-channel-names:
- minItems: 1
- maxItems: 8
-
st,adc-channel-types:
minItems: 1
maxItems: 8
@@ -191,14 +196,6 @@ patternProperties:
minItems: 1
maxItems: 8
- io-channels:
- description:
- From common IIO binding. Used to pipe external sigma delta
- modulator or internal ADC output to DFSDM channel.
-
- required:
- - io-channels
-
- if:
properties:
compatible:
@@ -207,12 +204,6 @@ patternProperties:
then:
properties:
- st,adc-channels:
- maxItems: 1
-
- st,adc-channel-names:
- maxItems: 1
-
st,adc-channel-types:
maxItems: 1
@@ -237,15 +228,9 @@ patternProperties:
"#sound-dai-cells":
const: 0
- io-channels:
- description:
- From common IIO binding. Used to pipe external sigma delta
- modulator or internal ADC output to DFSDM channel.
-
required:
- compatible
- "#sound-dai-cells"
- - io-channels
allOf:
- if:
@@ -278,52 +263,4 @@ allOf:
minimum: 0
maximum: 5
-examples:
- - |
- #include <dt-bindings/interrupt-controller/arm-gic.h>
- #include <dt-bindings/clock/stm32mp1-clks.h>
- dfsdm: 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>;
-
- 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>;
- st,adc-channels = <1>;
- st,adc-channel-names = "dmic0";
- st,adc-channel-types = "SPI_R";
- st,adc-channel-clk-src = "CLKOUT";
- st,filter-order = <5>;
-
- asoc_pdm0: dfsdm-dai {
- compatible = "st,stm32h7-dfsdm-dai";
- #sound-dai-cells = <0>;
- io-channels = <&dfsdm0 0>;
- };
- };
-
- dfsdm_pdm1: 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";
- #io-channel-cells = <1>;
- st,adc-channels = <2 3>;
- st,adc-channel-names = "in2", "in3";
- st,adc-channel-types = "SPI_R", "SPI_R";
- st,adc-channel-clk-src = "CLKOUT_F", "CLKOUT_F";
- io-channels = <&sd_adc2 &sd_adc3>;
- st,filter-order = <1>;
- };
- };
-
...
--
2.25.1
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [RFC v2 04/11] dt-bindings: iio: adc: add scaling support to sd modulator
2023-07-27 15:03 ` Olivier Moysan
` (3 preceding siblings ...)
(?)
@ 2023-07-27 15:03 ` Olivier Moysan
-1 siblings, 0 replies; 40+ messages in thread
From: Olivier Moysan @ 2023-07-27 15:03 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Arnaud Pouliquen
Cc: Olivier Moysan, linux-iio, devicetree, linux-kernel
Add scaling support to SD modulator.
Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
---
.../bindings/iio/adc/sigma-delta-modulator.yaml | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/iio/adc/sigma-delta-modulator.yaml b/Documentation/devicetree/bindings/iio/adc/sigma-delta-modulator.yaml
index cab0d425eaa4..bcc446c6428c 100644
--- a/Documentation/devicetree/bindings/iio/adc/sigma-delta-modulator.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/sigma-delta-modulator.yaml
@@ -18,12 +18,15 @@ properties:
- sd-modulator
- ads1201
- '#io-channel-cells':
+ '#io-backend-cells':
const: 0
+ vref-supply:
+ description: Phandle to the SD modulator reference voltage.
+
required:
- compatible
- - '#io-channel-cells'
+ - '#io-backend-cells'
additionalProperties: false
@@ -31,7 +34,7 @@ examples:
- |
ads1202: adc {
compatible = "sd-modulator";
- #io-channel-cells = <0>;
+ #io-backend-cells = <0>;
};
...
--
2.25.1
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [RFC v2 05/11] iio: adc: stm32-dfsdm: manage dfsdm as a channel provider
2023-07-27 15:03 ` Olivier Moysan
@ 2023-07-27 15:03 ` Olivier Moysan
-1 siblings, 0 replies; 40+ messages in thread
From: Olivier Moysan @ 2023-07-27 15:03 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Maxime Coquelin, Alexandre Torgue
Cc: Olivier Moysan, linux-iio, linux-stm32, linux-arm-kernel, linux-kernel
The STM32 is currently implemented as a channels consumer
of the sigma delta modulator.
Change the topology to expose a single IIO device for DFSDM
and remove the IIO device associated to the SD modulator.
Manage the DFSDM as a channel provider to allow this change.
Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
---
drivers/iio/adc/stm32-dfsdm-adc.c | 19 -------------------
1 file changed, 19 deletions(-)
diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
index b5cc43d12b6f..20f7dffcecdd 100644
--- a/drivers/iio/adc/stm32-dfsdm-adc.c
+++ b/drivers/iio/adc/stm32-dfsdm-adc.c
@@ -77,7 +77,6 @@ struct stm32_dfsdm_adc {
/* ADC specific */
unsigned int oversamp;
- struct iio_hw_consumer *hwc;
struct completion completion;
u32 *buffer;
@@ -1007,12 +1006,6 @@ static int stm32_dfsdm_postenable(struct iio_dev *indio_dev)
/* Reset adc buffer index */
adc->bufi = 0;
- if (adc->hwc) {
- ret = iio_hw_consumer_enable(adc->hwc);
- if (ret < 0)
- return ret;
- }
-
ret = stm32_dfsdm_start_dfsdm(adc->dfsdm);
if (ret < 0)
goto err_stop_hwc;
@@ -1036,8 +1029,6 @@ static int stm32_dfsdm_postenable(struct iio_dev *indio_dev)
stop_dfsdm:
stm32_dfsdm_stop_dfsdm(adc->dfsdm);
err_stop_hwc:
- if (adc->hwc)
- iio_hw_consumer_disable(adc->hwc);
return ret;
}
@@ -1052,9 +1043,6 @@ static int stm32_dfsdm_predisable(struct iio_dev *indio_dev)
stm32_dfsdm_stop_dfsdm(adc->dfsdm);
- if (adc->hwc)
- iio_hw_consumer_disable(adc->hwc);
-
return 0;
}
@@ -1231,7 +1219,6 @@ 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 (ret < 0) {
dev_err(&indio_dev->dev,
"%s: IIO enable failed (channel %d)\n",
@@ -1240,7 +1227,6 @@ 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 (ret < 0) {
dev_err(&indio_dev->dev,
"%s: Conversion failed (channel %d)\n",
@@ -1450,11 +1436,6 @@ static int stm32_dfsdm_adc_init(struct device *dev, struct iio_dev *indio_dev)
return num_ch < 0 ? num_ch : -EINVAL;
}
- /* Bind to SD modulator IIO device */
- adc->hwc = devm_iio_hw_consumer_alloc(&indio_dev->dev);
- if (IS_ERR(adc->hwc))
- return -EPROBE_DEFER;
-
ch = devm_kcalloc(&indio_dev->dev, num_ch, sizeof(*ch),
GFP_KERNEL);
if (!ch)
--
2.25.1
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [RFC v2 05/11] iio: adc: stm32-dfsdm: manage dfsdm as a channel provider
@ 2023-07-27 15:03 ` Olivier Moysan
0 siblings, 0 replies; 40+ messages in thread
From: Olivier Moysan @ 2023-07-27 15:03 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Maxime Coquelin, Alexandre Torgue
Cc: Olivier Moysan, linux-iio, linux-stm32, linux-arm-kernel, linux-kernel
The STM32 is currently implemented as a channels consumer
of the sigma delta modulator.
Change the topology to expose a single IIO device for DFSDM
and remove the IIO device associated to the SD modulator.
Manage the DFSDM as a channel provider to allow this change.
Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
---
drivers/iio/adc/stm32-dfsdm-adc.c | 19 -------------------
1 file changed, 19 deletions(-)
diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
index b5cc43d12b6f..20f7dffcecdd 100644
--- a/drivers/iio/adc/stm32-dfsdm-adc.c
+++ b/drivers/iio/adc/stm32-dfsdm-adc.c
@@ -77,7 +77,6 @@ struct stm32_dfsdm_adc {
/* ADC specific */
unsigned int oversamp;
- struct iio_hw_consumer *hwc;
struct completion completion;
u32 *buffer;
@@ -1007,12 +1006,6 @@ static int stm32_dfsdm_postenable(struct iio_dev *indio_dev)
/* Reset adc buffer index */
adc->bufi = 0;
- if (adc->hwc) {
- ret = iio_hw_consumer_enable(adc->hwc);
- if (ret < 0)
- return ret;
- }
-
ret = stm32_dfsdm_start_dfsdm(adc->dfsdm);
if (ret < 0)
goto err_stop_hwc;
@@ -1036,8 +1029,6 @@ static int stm32_dfsdm_postenable(struct iio_dev *indio_dev)
stop_dfsdm:
stm32_dfsdm_stop_dfsdm(adc->dfsdm);
err_stop_hwc:
- if (adc->hwc)
- iio_hw_consumer_disable(adc->hwc);
return ret;
}
@@ -1052,9 +1043,6 @@ static int stm32_dfsdm_predisable(struct iio_dev *indio_dev)
stm32_dfsdm_stop_dfsdm(adc->dfsdm);
- if (adc->hwc)
- iio_hw_consumer_disable(adc->hwc);
-
return 0;
}
@@ -1231,7 +1219,6 @@ 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 (ret < 0) {
dev_err(&indio_dev->dev,
"%s: IIO enable failed (channel %d)\n",
@@ -1240,7 +1227,6 @@ 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 (ret < 0) {
dev_err(&indio_dev->dev,
"%s: Conversion failed (channel %d)\n",
@@ -1450,11 +1436,6 @@ static int stm32_dfsdm_adc_init(struct device *dev, struct iio_dev *indio_dev)
return num_ch < 0 ? num_ch : -EINVAL;
}
- /* Bind to SD modulator IIO device */
- adc->hwc = devm_iio_hw_consumer_alloc(&indio_dev->dev);
- if (IS_ERR(adc->hwc))
- return -EPROBE_DEFER;
-
ch = devm_kcalloc(&indio_dev->dev, num_ch, sizeof(*ch),
GFP_KERNEL);
if (!ch)
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [RFC v2 06/11] iio: adc: stm32-dfsdm: adopt generic channel bindings
2023-07-27 15:03 ` Olivier Moysan
@ 2023-07-27 15:03 ` Olivier Moysan
-1 siblings, 0 replies; 40+ messages in thread
From: Olivier Moysan @ 2023-07-27 15:03 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Maxime Coquelin, Alexandre Torgue
Cc: Olivier Moysan, linux-iio, linux-stm32, linux-arm-kernel, linux-kernel
Adopt the generic channel bindings to ease the configuration
of the DFSDM channels as consumers of the SD modulator backend device.
Also adopt unified device property API in the same patch for this RFC.
Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
---
drivers/iio/adc/stm32-dfsdm-adc.c | 93 ++++++++++++++++---------------
1 file changed, 49 insertions(+), 44 deletions(-)
diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
index 20f7dffcecdd..96f4e0c64cdc 100644
--- a/drivers/iio/adc/stm32-dfsdm-adc.c
+++ b/drivers/iio/adc/stm32-dfsdm-adc.c
@@ -596,45 +596,35 @@ static int stm32_dfsdm_filter_configure(struct iio_dev *indio_dev,
static int stm32_dfsdm_channel_parse_of(struct stm32_dfsdm *dfsdm,
struct iio_dev *indio_dev,
+ struct fwnode_handle *node,
struct iio_chan_spec *ch)
{
struct stm32_dfsdm_channel *df_ch;
const char *of_str;
- int chan_idx = ch->scan_index;
int ret, val;
- ret = of_property_read_u32_index(indio_dev->dev.of_node,
- "st,adc-channels", chan_idx,
- &ch->channel);
+ ret = fwnode_property_read_u32(node, "reg", &ch->channel);
if (ret < 0) {
- dev_err(&indio_dev->dev,
- " Error parsing 'st,adc-channels' for idx %d\n",
- chan_idx);
+ 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",
+ dev_err(&indio_dev->dev, " Error bad channel number %d (max = %d)\n",
ch->channel, dfsdm->num_chs);
return -EINVAL;
}
- ret = of_property_read_string_index(indio_dev->dev.of_node,
- "st,adc-channel-names", chan_idx,
- &ch->datasheet_name);
+ ret = fwnode_property_read_string(node, "label", &ch->datasheet_name);
if (ret < 0) {
dev_err(&indio_dev->dev,
- " Error parsing 'st,adc-channel-names' for idx %d\n",
- chan_idx);
+ " Error parsing 'label' for idx %d\n", ch->channel);
return ret;
}
df_ch = &dfsdm->ch_list[ch->channel];
df_ch->id = ch->channel;
- ret = of_property_read_string_index(indio_dev->dev.of_node,
- "st,adc-channel-types", chan_idx,
- &of_str);
+ 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)
@@ -644,9 +634,7 @@ static int stm32_dfsdm_channel_parse_of(struct stm32_dfsdm *dfsdm,
}
df_ch->type = val;
- ret = of_property_read_string_index(indio_dev->dev.of_node,
- "st,adc-channel-clk-src", chan_idx,
- &of_str);
+ 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)
@@ -656,10 +644,8 @@ static int stm32_dfsdm_channel_parse_of(struct stm32_dfsdm *dfsdm,
}
df_ch->src = val;
- ret = of_property_read_u32_index(indio_dev->dev.of_node,
- "st,adc-alt-channel", chan_idx,
- &df_ch->alt_si);
- if (ret < 0)
+ ret = fwnode_property_read_u32(node, "st,adc-alt-channel", &df_ch->alt_si);
+ if (ret != -EINVAL)
df_ch->alt_si = 0;
return 0;
@@ -1354,17 +1340,21 @@ static int stm32_dfsdm_dma_request(struct device *dev,
}
static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev,
+ struct fwnode_handle *child,
struct iio_chan_spec *ch)
{
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)
+ ret = stm32_dfsdm_channel_parse_of(adc->dfsdm, indio_dev, child, ch);
+ if (ret < 0) {
+ dev_err(&indio_dev->dev, "Failed to parse channel\n");
return ret;
+ }
ch->type = IIO_VOLTAGE;
ch->indexed = 1;
+ ch->scan_index = ch->channel;
/*
* IIO_CHAN_INFO_RAW: used to compute regular conversion
@@ -1387,6 +1377,30 @@ static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev,
&adc->dfsdm->ch_list[ch->channel]);
}
+static int stm32_dfsdm_generic_chan_init(struct iio_dev *indio_dev, struct stm32_dfsdm_adc *adc,
+ struct iio_chan_spec *channels)
+{
+ struct fwnode_handle *child;
+ int chan_idx = 0, ret;
+
+ device_for_each_child_node(&indio_dev->dev, child) {
+ ret = stm32_dfsdm_adc_chan_init_one(indio_dev, child, &channels[chan_idx]);
+ 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)
{
struct iio_chan_spec *ch;
@@ -1400,7 +1414,7 @@ static int stm32_dfsdm_audio_init(struct device *dev, struct iio_dev *indio_dev)
ch->scan_index = 0;
- ret = stm32_dfsdm_adc_chan_init_one(indio_dev, ch);
+ ret = stm32_dfsdm_generic_chan_init(indio_dev, adc, ch);
if (ret < 0) {
dev_err(&indio_dev->dev, "Channels init failed\n");
return ret;
@@ -1422,33 +1436,24 @@ 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 ret;
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)
+ return -EINVAL;
- ch = devm_kcalloc(&indio_dev->dev, num_ch, sizeof(*ch),
- GFP_KERNEL);
+ ch = devm_kcalloc(&indio_dev->dev, num_ch, sizeof(*ch), GFP_KERNEL);
if (!ch)
return -ENOMEM;
- 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;
- }
- }
+ stm32_dfsdm_generic_chan_init(indio_dev, adc, ch);
+ if (ret < 0)
+ return ret;
indio_dev->num_channels = num_ch;
indio_dev->channels = ch;
--
2.25.1
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [RFC v2 06/11] iio: adc: stm32-dfsdm: adopt generic channel bindings
@ 2023-07-27 15:03 ` Olivier Moysan
0 siblings, 0 replies; 40+ messages in thread
From: Olivier Moysan @ 2023-07-27 15:03 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Maxime Coquelin, Alexandre Torgue
Cc: Olivier Moysan, linux-iio, linux-stm32, linux-arm-kernel, linux-kernel
Adopt the generic channel bindings to ease the configuration
of the DFSDM channels as consumers of the SD modulator backend device.
Also adopt unified device property API in the same patch for this RFC.
Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
---
drivers/iio/adc/stm32-dfsdm-adc.c | 93 ++++++++++++++++---------------
1 file changed, 49 insertions(+), 44 deletions(-)
diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
index 20f7dffcecdd..96f4e0c64cdc 100644
--- a/drivers/iio/adc/stm32-dfsdm-adc.c
+++ b/drivers/iio/adc/stm32-dfsdm-adc.c
@@ -596,45 +596,35 @@ static int stm32_dfsdm_filter_configure(struct iio_dev *indio_dev,
static int stm32_dfsdm_channel_parse_of(struct stm32_dfsdm *dfsdm,
struct iio_dev *indio_dev,
+ struct fwnode_handle *node,
struct iio_chan_spec *ch)
{
struct stm32_dfsdm_channel *df_ch;
const char *of_str;
- int chan_idx = ch->scan_index;
int ret, val;
- ret = of_property_read_u32_index(indio_dev->dev.of_node,
- "st,adc-channels", chan_idx,
- &ch->channel);
+ ret = fwnode_property_read_u32(node, "reg", &ch->channel);
if (ret < 0) {
- dev_err(&indio_dev->dev,
- " Error parsing 'st,adc-channels' for idx %d\n",
- chan_idx);
+ 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",
+ dev_err(&indio_dev->dev, " Error bad channel number %d (max = %d)\n",
ch->channel, dfsdm->num_chs);
return -EINVAL;
}
- ret = of_property_read_string_index(indio_dev->dev.of_node,
- "st,adc-channel-names", chan_idx,
- &ch->datasheet_name);
+ ret = fwnode_property_read_string(node, "label", &ch->datasheet_name);
if (ret < 0) {
dev_err(&indio_dev->dev,
- " Error parsing 'st,adc-channel-names' for idx %d\n",
- chan_idx);
+ " Error parsing 'label' for idx %d\n", ch->channel);
return ret;
}
df_ch = &dfsdm->ch_list[ch->channel];
df_ch->id = ch->channel;
- ret = of_property_read_string_index(indio_dev->dev.of_node,
- "st,adc-channel-types", chan_idx,
- &of_str);
+ 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)
@@ -644,9 +634,7 @@ static int stm32_dfsdm_channel_parse_of(struct stm32_dfsdm *dfsdm,
}
df_ch->type = val;
- ret = of_property_read_string_index(indio_dev->dev.of_node,
- "st,adc-channel-clk-src", chan_idx,
- &of_str);
+ 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)
@@ -656,10 +644,8 @@ static int stm32_dfsdm_channel_parse_of(struct stm32_dfsdm *dfsdm,
}
df_ch->src = val;
- ret = of_property_read_u32_index(indio_dev->dev.of_node,
- "st,adc-alt-channel", chan_idx,
- &df_ch->alt_si);
- if (ret < 0)
+ ret = fwnode_property_read_u32(node, "st,adc-alt-channel", &df_ch->alt_si);
+ if (ret != -EINVAL)
df_ch->alt_si = 0;
return 0;
@@ -1354,17 +1340,21 @@ static int stm32_dfsdm_dma_request(struct device *dev,
}
static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev,
+ struct fwnode_handle *child,
struct iio_chan_spec *ch)
{
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)
+ ret = stm32_dfsdm_channel_parse_of(adc->dfsdm, indio_dev, child, ch);
+ if (ret < 0) {
+ dev_err(&indio_dev->dev, "Failed to parse channel\n");
return ret;
+ }
ch->type = IIO_VOLTAGE;
ch->indexed = 1;
+ ch->scan_index = ch->channel;
/*
* IIO_CHAN_INFO_RAW: used to compute regular conversion
@@ -1387,6 +1377,30 @@ static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev,
&adc->dfsdm->ch_list[ch->channel]);
}
+static int stm32_dfsdm_generic_chan_init(struct iio_dev *indio_dev, struct stm32_dfsdm_adc *adc,
+ struct iio_chan_spec *channels)
+{
+ struct fwnode_handle *child;
+ int chan_idx = 0, ret;
+
+ device_for_each_child_node(&indio_dev->dev, child) {
+ ret = stm32_dfsdm_adc_chan_init_one(indio_dev, child, &channels[chan_idx]);
+ 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)
{
struct iio_chan_spec *ch;
@@ -1400,7 +1414,7 @@ static int stm32_dfsdm_audio_init(struct device *dev, struct iio_dev *indio_dev)
ch->scan_index = 0;
- ret = stm32_dfsdm_adc_chan_init_one(indio_dev, ch);
+ ret = stm32_dfsdm_generic_chan_init(indio_dev, adc, ch);
if (ret < 0) {
dev_err(&indio_dev->dev, "Channels init failed\n");
return ret;
@@ -1422,33 +1436,24 @@ 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 ret;
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)
+ return -EINVAL;
- ch = devm_kcalloc(&indio_dev->dev, num_ch, sizeof(*ch),
- GFP_KERNEL);
+ ch = devm_kcalloc(&indio_dev->dev, num_ch, sizeof(*ch), GFP_KERNEL);
if (!ch)
return -ENOMEM;
- 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;
- }
- }
+ stm32_dfsdm_generic_chan_init(indio_dev, adc, ch);
+ if (ret < 0)
+ return ret;
indio_dev->num_channels = num_ch;
indio_dev->channels = ch;
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [RFC v2 07/11] iio: adc: stm32-dfsdm: add scaling support to dfsdm
2023-07-27 15:03 ` Olivier Moysan
@ 2023-07-27 15:03 ` Olivier Moysan
-1 siblings, 0 replies; 40+ messages in thread
From: Olivier Moysan @ 2023-07-27 15:03 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Maxime Coquelin, Alexandre Torgue
Cc: Olivier Moysan, linux-iio, linux-stm32, linux-arm-kernel, linux-kernel
Add scaling support to STM32 DFSDM.
Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
---
drivers/iio/adc/stm32-dfsdm-adc.c | 77 +++++++++++++++++++++++++++++--
1 file changed, 73 insertions(+), 4 deletions(-)
diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
index 96f4e0c64cdc..dba1a8ef5451 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>
@@ -77,6 +78,7 @@ struct stm32_dfsdm_adc {
/* ADC specific */
unsigned int oversamp;
+ struct iio_backend **backend;
struct completion completion;
u32 *buffer;
@@ -600,6 +602,8 @@ static int stm32_dfsdm_channel_parse_of(struct stm32_dfsdm *dfsdm,
struct iio_chan_spec *ch)
{
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;
@@ -648,6 +652,12 @@ static int stm32_dfsdm_channel_parse_of(struct stm32_dfsdm *dfsdm,
if (ret != -EINVAL)
df_ch->alt_si = 0;
+ backend = fwnode_iio_backend_get(node, 0);
+ if (IS_ERR(backend))
+ return dev_err_probe(&indio_dev->dev, PTR_ERR(backend), "Failed to get backend\n");
+
+ adc->backend[df_ch->id] = backend;
+
return 0;
}
@@ -1091,7 +1101,7 @@ static int stm32_dfsdm_single_conv(struct iio_dev *indio_dev,
{
struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
long timeout;
- int ret;
+ int ret, idx = chan->scan_index;
reinit_completion(&adc->completion);
@@ -1101,6 +1111,13 @@ static int stm32_dfsdm_single_conv(struct iio_dev *indio_dev,
if (ret < 0)
return ret;
+ if (!adc->backend[idx]->ops->enable)
+ return -EINVAL;
+
+ ret = adc->backend[idx]->ops->enable(adc->backend[idx]);
+ if (ret < 0)
+ return ret;
+
ret = regmap_update_bits(adc->dfsdm->regmap, DFSDM_CR2(adc->fl_id),
DFSDM_CR2_REOCIE_MASK, DFSDM_CR2_REOCIE(1));
if (ret < 0)
@@ -1134,6 +1151,8 @@ static int stm32_dfsdm_single_conv(struct iio_dev *indio_dev,
stm32_dfsdm_process_data(adc, res);
stop_dfsdm:
+ ret = adc->backend[idx]->ops->disable(adc->backend[idx]);
+
stm32_dfsdm_stop_dfsdm(adc->dfsdm);
return ret;
@@ -1198,7 +1217,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:
@@ -1232,6 +1258,41 @@ 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
+ */
+ adc->backend[idx]->ops->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.
+ */
+ adc->backend[idx]->ops->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;
@@ -1360,7 +1421,10 @@ static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev,
* 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);
+ ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_SCALE) |
+ BIT(IIO_CHAN_INFO_OFFSET);
+
ch->info_mask_shared_by_all = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) |
BIT(IIO_CHAN_INFO_SAMP_FREQ);
@@ -1451,7 +1515,12 @@ static int stm32_dfsdm_adc_init(struct device *dev, struct iio_dev *indio_dev)
if (!ch)
return -ENOMEM;
- stm32_dfsdm_generic_chan_init(indio_dev, adc, ch);
+ adc->backend = devm_kzalloc(&indio_dev->dev, sizeof(*adc->backend) * adc->dfsdm->num_chs,
+ GFP_KERNEL);
+ if (!adc->backend)
+ return -ENOMEM;
+
+ ret = stm32_dfsdm_generic_chan_init(indio_dev, adc, ch);
if (ret < 0)
return ret;
--
2.25.1
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [RFC v2 07/11] iio: adc: stm32-dfsdm: add scaling support to dfsdm
@ 2023-07-27 15:03 ` Olivier Moysan
0 siblings, 0 replies; 40+ messages in thread
From: Olivier Moysan @ 2023-07-27 15:03 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Maxime Coquelin, Alexandre Torgue
Cc: Olivier Moysan, linux-iio, linux-stm32, linux-arm-kernel, linux-kernel
Add scaling support to STM32 DFSDM.
Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
---
drivers/iio/adc/stm32-dfsdm-adc.c | 77 +++++++++++++++++++++++++++++--
1 file changed, 73 insertions(+), 4 deletions(-)
diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
index 96f4e0c64cdc..dba1a8ef5451 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>
@@ -77,6 +78,7 @@ struct stm32_dfsdm_adc {
/* ADC specific */
unsigned int oversamp;
+ struct iio_backend **backend;
struct completion completion;
u32 *buffer;
@@ -600,6 +602,8 @@ static int stm32_dfsdm_channel_parse_of(struct stm32_dfsdm *dfsdm,
struct iio_chan_spec *ch)
{
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;
@@ -648,6 +652,12 @@ static int stm32_dfsdm_channel_parse_of(struct stm32_dfsdm *dfsdm,
if (ret != -EINVAL)
df_ch->alt_si = 0;
+ backend = fwnode_iio_backend_get(node, 0);
+ if (IS_ERR(backend))
+ return dev_err_probe(&indio_dev->dev, PTR_ERR(backend), "Failed to get backend\n");
+
+ adc->backend[df_ch->id] = backend;
+
return 0;
}
@@ -1091,7 +1101,7 @@ static int stm32_dfsdm_single_conv(struct iio_dev *indio_dev,
{
struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
long timeout;
- int ret;
+ int ret, idx = chan->scan_index;
reinit_completion(&adc->completion);
@@ -1101,6 +1111,13 @@ static int stm32_dfsdm_single_conv(struct iio_dev *indio_dev,
if (ret < 0)
return ret;
+ if (!adc->backend[idx]->ops->enable)
+ return -EINVAL;
+
+ ret = adc->backend[idx]->ops->enable(adc->backend[idx]);
+ if (ret < 0)
+ return ret;
+
ret = regmap_update_bits(adc->dfsdm->regmap, DFSDM_CR2(adc->fl_id),
DFSDM_CR2_REOCIE_MASK, DFSDM_CR2_REOCIE(1));
if (ret < 0)
@@ -1134,6 +1151,8 @@ static int stm32_dfsdm_single_conv(struct iio_dev *indio_dev,
stm32_dfsdm_process_data(adc, res);
stop_dfsdm:
+ ret = adc->backend[idx]->ops->disable(adc->backend[idx]);
+
stm32_dfsdm_stop_dfsdm(adc->dfsdm);
return ret;
@@ -1198,7 +1217,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:
@@ -1232,6 +1258,41 @@ 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
+ */
+ adc->backend[idx]->ops->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.
+ */
+ adc->backend[idx]->ops->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;
@@ -1360,7 +1421,10 @@ static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev,
* 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);
+ ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_SCALE) |
+ BIT(IIO_CHAN_INFO_OFFSET);
+
ch->info_mask_shared_by_all = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) |
BIT(IIO_CHAN_INFO_SAMP_FREQ);
@@ -1451,7 +1515,12 @@ static int stm32_dfsdm_adc_init(struct device *dev, struct iio_dev *indio_dev)
if (!ch)
return -ENOMEM;
- stm32_dfsdm_generic_chan_init(indio_dev, adc, ch);
+ adc->backend = devm_kzalloc(&indio_dev->dev, sizeof(*adc->backend) * adc->dfsdm->num_chs,
+ GFP_KERNEL);
+ if (!adc->backend)
+ return -ENOMEM;
+
+ ret = stm32_dfsdm_generic_chan_init(indio_dev, adc, ch);
if (ret < 0)
return ret;
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [RFC v2 08/11] iio: adc: sd modulator: add scale and offset support
2023-07-27 15:03 ` Olivier Moysan
` (7 preceding siblings ...)
(?)
@ 2023-07-27 15:03 ` Olivier Moysan
2023-07-27 22:37 ` kernel test robot
2023-07-28 0:41 ` kernel test robot
-1 siblings, 2 replies; 40+ messages in thread
From: Olivier Moysan @ 2023-07-27 15:03 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Liam Girdwood, Mark Brown
Cc: Olivier Moysan, linux-iio, linux-kernel
Register the SD modulator as an IIO backend device instead of
a standard IIO device. Expose scale and offset information to
IIO backend consumer.
Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
---
drivers/iio/adc/sd_adc_modulator.c | 106 +++++++++++++++++++++++------
1 file changed, 86 insertions(+), 20 deletions(-)
diff --git a/drivers/iio/adc/sd_adc_modulator.c b/drivers/iio/adc/sd_adc_modulator.c
index 327cc2097f6c..e77e7884c403 100644
--- a/drivers/iio/adc/sd_adc_modulator.c
+++ b/drivers/iio/adc/sd_adc_modulator.c
@@ -6,44 +6,110 @@
* Author: Arnaud Pouliquen <arnaud.pouliquen@st.com>.
*/
+#include <linux/iio/backend.h>
#include <linux/iio/iio.h>
#include <linux/iio/triggered_buffer.h>
#include <linux/module.h>
#include <linux/mod_devicetable.h>
#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
-static const struct iio_info iio_sd_mod_iio_info;
+struct iio_sd_mod_priv {
+ struct regulator *vref;
+ int vref_mv;
+};
-static const struct iio_chan_spec iio_sd_mod_ch = {
- .type = IIO_VOLTAGE,
- .indexed = 1,
- .scan_type = {
- .sign = 'u',
- .realbits = 1,
- .shift = 0,
- },
+static int sd_mod_enable(struct iio_backend *backend)
+{
+ struct iio_sd_mod_priv *priv = backend->priv;
+ int ret;
+
+ ret = regulator_enable(priv->vref);
+ if (ret) {
+ dev_err(&backend->dev, "Failed to enable regulator: %d\n", ret);
+ return ret;
+ }
+
+ ret = regulator_get_voltage(priv->vref);
+ priv->vref_mv = ret / 1000;
+
+ return 0;
+};
+
+static int sd_mod_disable(struct iio_backend *backend)
+{
+ struct iio_sd_mod_priv *priv = backend->priv;
+
+ return regulator_disable(priv->vref);
+};
+
+static int sd_mod_read(struct iio_backend *backend, int *val, int *val2, long mask)
+{
+ struct iio_sd_mod_priv *priv = backend->priv;
+
+ 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_mod_ops = {
+ .enable = sd_mod_enable,
+ .disable = sd_mod_disable,
+ .read_raw = sd_mod_read,
};
static int iio_sd_mod_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
- struct iio_dev *iio;
+ struct regulator *vref;
+ struct iio_backend *backend;
+ struct iio_sd_mod_priv *priv;
+ int ret;
- iio = devm_iio_device_alloc(dev, 0);
- if (!iio)
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
return -ENOMEM;
- iio->name = dev_name(dev);
- iio->info = &iio_sd_mod_iio_info;
- iio->modes = INDIO_BUFFER_HARDWARE;
+ 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 {
+ priv->vref = vref;
+ }
- iio->num_channels = 1;
- iio->channels = &iio_sd_mod_ch;
+ backend = iio_backend_alloc(dev);
+ if (!backend) {
+ dev_err(dev, "Failed to allocate sd modulator backend data\n");
+ return -ENOMEM;
+ }
+
+ backend->priv = priv;
+ backend->ops = &sd_mod_ops;
+
+ ret = iio_backend_register(backend);
+ if (ret) {
+ dev_err(dev, "Failed to register backend: %d\n", ret);
+ goto err_free_backend;
+ }
- platform_set_drvdata(pdev, iio);
+ return 0;
- return devm_iio_device_register(&pdev->dev, iio);
-}
+err_free_backend:
+ iio_backend_free(&backend->dev);
+
+ return ret;
+};
static const struct of_device_id sd_adc_of_match[] = {
{ .compatible = "sd-modulator" },
--
2.25.1
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [RFC v2 09/11] ARM: dts: stm32: adopt new dfsdm bindings on stm32mp151
2023-07-27 15:03 ` Olivier Moysan
@ 2023-07-27 15:03 ` Olivier Moysan
-1 siblings, 0 replies; 40+ messages in thread
From: Olivier Moysan @ 2023-07-27 15:03 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Maxime Coquelin,
Alexandre Torgue
Cc: Olivier Moysan, devicetree, linux-stm32, linux-arm-kernel, linux-kernel
Adapt STM32MP151 device tree to match DFSDM new bindings.
Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
---
arch/arm/boot/dts/st/stm32mp151.dtsi | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/arch/arm/boot/dts/st/stm32mp151.dtsi b/arch/arm/boot/dts/st/stm32mp151.dtsi
index 61508917521c..338457357248 100644
--- a/arch/arm/boot/dts/st/stm32mp151.dtsi
+++ b/arch/arm/boot/dts/st/stm32mp151.dtsi
@@ -970,7 +970,8 @@ dfsdm: dfsdm@4400d000 {
dfsdm0: filter@0 {
compatible = "st,stm32-dfsdm-adc";
- #io-channel-cells = <1>;
+ #address-cells = <1>;
+ #size-cells = <0>;
reg = <0>;
interrupts = <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
dmas = <&dmamux1 101 0x400 0x01>;
@@ -980,7 +981,8 @@ dfsdm0: filter@0 {
dfsdm1: filter@1 {
compatible = "st,stm32-dfsdm-adc";
- #io-channel-cells = <1>;
+ #address-cells = <1>;
+ #size-cells = <0>;
reg = <1>;
interrupts = <GIC_SPI 111 IRQ_TYPE_LEVEL_HIGH>;
dmas = <&dmamux1 102 0x400 0x01>;
@@ -990,7 +992,8 @@ dfsdm1: filter@1 {
dfsdm2: filter@2 {
compatible = "st,stm32-dfsdm-adc";
- #io-channel-cells = <1>;
+ #address-cells = <1>;
+ #size-cells = <0>;
reg = <2>;
interrupts = <GIC_SPI 112 IRQ_TYPE_LEVEL_HIGH>;
dmas = <&dmamux1 103 0x400 0x01>;
@@ -1000,7 +1003,8 @@ dfsdm2: filter@2 {
dfsdm3: filter@3 {
compatible = "st,stm32-dfsdm-adc";
- #io-channel-cells = <1>;
+ #address-cells = <1>;
+ #size-cells = <0>;
reg = <3>;
interrupts = <GIC_SPI 113 IRQ_TYPE_LEVEL_HIGH>;
dmas = <&dmamux1 104 0x400 0x01>;
@@ -1010,7 +1014,8 @@ dfsdm3: filter@3 {
dfsdm4: filter@4 {
compatible = "st,stm32-dfsdm-adc";
- #io-channel-cells = <1>;
+ #address-cells = <1>;
+ #size-cells = <0>;
reg = <4>;
interrupts = <GIC_SPI 115 IRQ_TYPE_LEVEL_HIGH>;
dmas = <&dmamux1 91 0x400 0x01>;
@@ -1020,7 +1025,8 @@ dfsdm4: filter@4 {
dfsdm5: filter@5 {
compatible = "st,stm32-dfsdm-adc";
- #io-channel-cells = <1>;
+ #address-cells = <1>;
+ #size-cells = <0>;
reg = <5>;
interrupts = <GIC_SPI 126 IRQ_TYPE_LEVEL_HIGH>;
dmas = <&dmamux1 92 0x400 0x01>;
--
2.25.1
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [RFC v2 09/11] ARM: dts: stm32: adopt new dfsdm bindings on stm32mp151
@ 2023-07-27 15:03 ` Olivier Moysan
0 siblings, 0 replies; 40+ messages in thread
From: Olivier Moysan @ 2023-07-27 15:03 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Maxime Coquelin,
Alexandre Torgue
Cc: Olivier Moysan, devicetree, linux-stm32, linux-arm-kernel, linux-kernel
Adapt STM32MP151 device tree to match DFSDM new bindings.
Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
---
arch/arm/boot/dts/st/stm32mp151.dtsi | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/arch/arm/boot/dts/st/stm32mp151.dtsi b/arch/arm/boot/dts/st/stm32mp151.dtsi
index 61508917521c..338457357248 100644
--- a/arch/arm/boot/dts/st/stm32mp151.dtsi
+++ b/arch/arm/boot/dts/st/stm32mp151.dtsi
@@ -970,7 +970,8 @@ dfsdm: dfsdm@4400d000 {
dfsdm0: filter@0 {
compatible = "st,stm32-dfsdm-adc";
- #io-channel-cells = <1>;
+ #address-cells = <1>;
+ #size-cells = <0>;
reg = <0>;
interrupts = <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
dmas = <&dmamux1 101 0x400 0x01>;
@@ -980,7 +981,8 @@ dfsdm0: filter@0 {
dfsdm1: filter@1 {
compatible = "st,stm32-dfsdm-adc";
- #io-channel-cells = <1>;
+ #address-cells = <1>;
+ #size-cells = <0>;
reg = <1>;
interrupts = <GIC_SPI 111 IRQ_TYPE_LEVEL_HIGH>;
dmas = <&dmamux1 102 0x400 0x01>;
@@ -990,7 +992,8 @@ dfsdm1: filter@1 {
dfsdm2: filter@2 {
compatible = "st,stm32-dfsdm-adc";
- #io-channel-cells = <1>;
+ #address-cells = <1>;
+ #size-cells = <0>;
reg = <2>;
interrupts = <GIC_SPI 112 IRQ_TYPE_LEVEL_HIGH>;
dmas = <&dmamux1 103 0x400 0x01>;
@@ -1000,7 +1003,8 @@ dfsdm2: filter@2 {
dfsdm3: filter@3 {
compatible = "st,stm32-dfsdm-adc";
- #io-channel-cells = <1>;
+ #address-cells = <1>;
+ #size-cells = <0>;
reg = <3>;
interrupts = <GIC_SPI 113 IRQ_TYPE_LEVEL_HIGH>;
dmas = <&dmamux1 104 0x400 0x01>;
@@ -1010,7 +1014,8 @@ dfsdm3: filter@3 {
dfsdm4: filter@4 {
compatible = "st,stm32-dfsdm-adc";
- #io-channel-cells = <1>;
+ #address-cells = <1>;
+ #size-cells = <0>;
reg = <4>;
interrupts = <GIC_SPI 115 IRQ_TYPE_LEVEL_HIGH>;
dmas = <&dmamux1 91 0x400 0x01>;
@@ -1020,7 +1025,8 @@ dfsdm4: filter@4 {
dfsdm5: filter@5 {
compatible = "st,stm32-dfsdm-adc";
- #io-channel-cells = <1>;
+ #address-cells = <1>;
+ #size-cells = <0>;
reg = <5>;
interrupts = <GIC_SPI 126 IRQ_TYPE_LEVEL_HIGH>;
dmas = <&dmamux1 92 0x400 0x01>;
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [RFC v2 10/11] ARM: dts: stm32: add dfsdm pins muxing on stm32mp15
2023-07-27 15:03 ` Olivier Moysan
@ 2023-07-27 15:03 ` Olivier Moysan
-1 siblings, 0 replies; 40+ messages in thread
From: Olivier Moysan @ 2023-07-27 15:03 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Maxime Coquelin,
Alexandre Torgue
Cc: Olivier Moysan, devicetree, linux-stm32, linux-arm-kernel, linux-kernel
Add STM32 DFSDM pin muxings to STM32MP15.
Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
---
arch/arm/boot/dts/st/stm32mp15-pinctrl.dtsi | 39 +++++++++++++++++++++
1 file changed, 39 insertions(+)
diff --git a/arch/arm/boot/dts/st/stm32mp15-pinctrl.dtsi b/arch/arm/boot/dts/st/stm32mp15-pinctrl.dtsi
index 05c9c4f8064c..f4dd46c176f9 100644
--- a/arch/arm/boot/dts/st/stm32mp15-pinctrl.dtsi
+++ b/arch/arm/boot/dts/st/stm32mp15-pinctrl.dtsi
@@ -188,6 +188,45 @@ pins {
};
};
+ dfsdm_clkout_pins_a: dfsdm-clkout-pins-0 {
+ pins {
+ pinmux = <STM32_PINMUX('B', 13, AF3)>; /* DFSDM_CKOUT */
+ bias-disable;
+ drive-push-pull;
+ slew-rate = <0>;
+ };
+ };
+
+ dfsdm_clkout_sleep_pins_a: dfsdm-clkout-sleep-pins-0 {
+ pins {
+ pinmux = <STM32_PINMUX('B', 13, ANALOG)>; /* DFSDM_CKOUT */
+ };
+ };
+
+ dfsdm_data1_pins_a: dfsdm-data1-pins-0 {
+ pins {
+ pinmux = <STM32_PINMUX('C', 3, AF3)>; /* DFSDM_DATA1 */
+ };
+ };
+
+ dfsdm_data1_sleep_pins_a: dfsdm-data1-sleep-pins-0 {
+ pins {
+ pinmux = <STM32_PINMUX('C', 3, ANALOG)>; /* DFSDM_DATA1 */
+ };
+ };
+
+ dfsdm_data3_pins_a: dfsdm-data3-pins-0 {
+ pins {
+ pinmux = <STM32_PINMUX('F', 13, AF6)>; /* DFSDM_DATA3 */
+ };
+ };
+
+ dfsdm_data3_sleep_pins_a: dfsdm-data3-sleep-pins-0 {
+ pins {
+ pinmux = <STM32_PINMUX('F', 13, ANALOG)>; /* DFSDM_DATA3 */
+ };
+ };
+
ethernet0_rgmii_pins_a: rgmii-0 {
pins1 {
pinmux = <STM32_PINMUX('G', 5, AF11)>, /* ETH_RGMII_CLK125 */
--
2.25.1
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [RFC v2 10/11] ARM: dts: stm32: add dfsdm pins muxing on stm32mp15
@ 2023-07-27 15:03 ` Olivier Moysan
0 siblings, 0 replies; 40+ messages in thread
From: Olivier Moysan @ 2023-07-27 15:03 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Maxime Coquelin,
Alexandre Torgue
Cc: Olivier Moysan, devicetree, linux-stm32, linux-arm-kernel, linux-kernel
Add STM32 DFSDM pin muxings to STM32MP15.
Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
---
arch/arm/boot/dts/st/stm32mp15-pinctrl.dtsi | 39 +++++++++++++++++++++
1 file changed, 39 insertions(+)
diff --git a/arch/arm/boot/dts/st/stm32mp15-pinctrl.dtsi b/arch/arm/boot/dts/st/stm32mp15-pinctrl.dtsi
index 05c9c4f8064c..f4dd46c176f9 100644
--- a/arch/arm/boot/dts/st/stm32mp15-pinctrl.dtsi
+++ b/arch/arm/boot/dts/st/stm32mp15-pinctrl.dtsi
@@ -188,6 +188,45 @@ pins {
};
};
+ dfsdm_clkout_pins_a: dfsdm-clkout-pins-0 {
+ pins {
+ pinmux = <STM32_PINMUX('B', 13, AF3)>; /* DFSDM_CKOUT */
+ bias-disable;
+ drive-push-pull;
+ slew-rate = <0>;
+ };
+ };
+
+ dfsdm_clkout_sleep_pins_a: dfsdm-clkout-sleep-pins-0 {
+ pins {
+ pinmux = <STM32_PINMUX('B', 13, ANALOG)>; /* DFSDM_CKOUT */
+ };
+ };
+
+ dfsdm_data1_pins_a: dfsdm-data1-pins-0 {
+ pins {
+ pinmux = <STM32_PINMUX('C', 3, AF3)>; /* DFSDM_DATA1 */
+ };
+ };
+
+ dfsdm_data1_sleep_pins_a: dfsdm-data1-sleep-pins-0 {
+ pins {
+ pinmux = <STM32_PINMUX('C', 3, ANALOG)>; /* DFSDM_DATA1 */
+ };
+ };
+
+ dfsdm_data3_pins_a: dfsdm-data3-pins-0 {
+ pins {
+ pinmux = <STM32_PINMUX('F', 13, AF6)>; /* DFSDM_DATA3 */
+ };
+ };
+
+ dfsdm_data3_sleep_pins_a: dfsdm-data3-sleep-pins-0 {
+ pins {
+ pinmux = <STM32_PINMUX('F', 13, ANALOG)>; /* DFSDM_DATA3 */
+ };
+ };
+
ethernet0_rgmii_pins_a: rgmii-0 {
pins1 {
pinmux = <STM32_PINMUX('G', 5, AF11)>, /* ETH_RGMII_CLK125 */
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [RFC v2 11/11] ARM: dts: stm32: add dfsdm iio support on stm32mp157c-ev
2023-07-27 15:03 ` Olivier Moysan
@ 2023-07-27 15:03 ` Olivier Moysan
-1 siblings, 0 replies; 40+ messages in thread
From: Olivier Moysan @ 2023-07-27 15:03 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Maxime Coquelin,
Alexandre Torgue
Cc: Olivier Moysan, devicetree, linux-stm32, linux-arm-kernel, linux-kernel
This DT is an example of backend iio device used for STM32 DFSDM.
DFSDM filter1 has a single input channel, while filter0 is configured
to support scan mode with two input channels.
Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
---
arch/arm/boot/dts/st/stm32mp157c-ev1.dts | 68 ++++++++++++++++++++++++
1 file changed, 68 insertions(+)
diff --git a/arch/arm/boot/dts/st/stm32mp157c-ev1.dts b/arch/arm/boot/dts/st/stm32mp157c-ev1.dts
index af3800501875..edeac26f39a4 100644
--- a/arch/arm/boot/dts/st/stm32mp157c-ev1.dts
+++ b/arch/arm/boot/dts/st/stm32mp157c-ev1.dts
@@ -73,6 +73,27 @@ panel_backlight: panel-backlight {
default-on;
status = "okay";
};
+
+ sd_adc0: sd-adc0 {
+ compatible = "sd-modulator";
+ #io-backend-cells = <0>;
+ vref-supply = <&v3v3>;
+ status = "okay";
+ };
+
+ sd_adc1: sd-adc1 {
+ compatible = "sd-modulator";
+ #io-backend-cells = <0>;
+ vref-supply = <&v3v3>;
+ status = "okay";
+ };
+
+ sd_adc2: sd-adc2 {
+ compatible = "sd-modulator";
+ #io-backend-cells = <0>;
+ vref-supply = <&v3v3>;
+ status = "okay";
+ };
};
&cec {
@@ -99,6 +120,53 @@ dcmi_0: endpoint {
};
};
+&dfsdm {
+ pinctrl-names = "default", "sleep";
+ pinctrl-0 = <&dfsdm_clkout_pins_a
+ &dfsdm_data1_pins_a &dfsdm_data3_pins_a>;
+ pinctrl-1 = <&dfsdm_clkout_sleep_pins_a
+ &dfsdm_data1_sleep_pins_a &dfsdm_data3_sleep_pins_a>;
+ spi-max-frequency = <2048000>;
+ status = "okay";
+
+ dfsdm0: filter@0 {
+ compatible = "st,stm32-dfsdm-adc";
+ st,filter-order = <3>;
+ status = "okay";
+
+ channel@0 {
+ reg = <0>;
+ label = "in0";
+ st,adc-channel-types = "SPI_F";
+ st,adc-channel-clk-src = "CLKOUT";
+ st,adc-alt-channel;
+ io-backends = <&sd_adc0>;
+ };
+
+ channel@1 {
+ reg = <1>;
+ label = "in1";
+ st,adc-channel-types = "SPI_R";
+ st,adc-channel-clk-src = "CLKOUT";
+ io-backends = <&sd_adc1>;
+ };
+ };
+
+ dfsdm1: filter@1 {
+ compatible = "st,stm32-dfsdm-adc";
+ st,filter-order = <3>;
+ status = "okay";
+
+ channel@3 {
+ reg = <3>;
+ label = "in3";
+ st,adc-channel-types = "SPI_R";
+ st,adc-channel-clk-src = "CLKOUT";
+ io-backends = <&sd_adc2>;
+ };
+ };
+};
+
&dsi {
phy-dsi-supply = <®18>;
#address-cells = <1>;
--
2.25.1
^ permalink raw reply related [flat|nested] 40+ messages in thread
* [RFC v2 11/11] ARM: dts: stm32: add dfsdm iio support on stm32mp157c-ev
@ 2023-07-27 15:03 ` Olivier Moysan
0 siblings, 0 replies; 40+ messages in thread
From: Olivier Moysan @ 2023-07-27 15:03 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Maxime Coquelin,
Alexandre Torgue
Cc: Olivier Moysan, devicetree, linux-stm32, linux-arm-kernel, linux-kernel
This DT is an example of backend iio device used for STM32 DFSDM.
DFSDM filter1 has a single input channel, while filter0 is configured
to support scan mode with two input channels.
Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
---
arch/arm/boot/dts/st/stm32mp157c-ev1.dts | 68 ++++++++++++++++++++++++
1 file changed, 68 insertions(+)
diff --git a/arch/arm/boot/dts/st/stm32mp157c-ev1.dts b/arch/arm/boot/dts/st/stm32mp157c-ev1.dts
index af3800501875..edeac26f39a4 100644
--- a/arch/arm/boot/dts/st/stm32mp157c-ev1.dts
+++ b/arch/arm/boot/dts/st/stm32mp157c-ev1.dts
@@ -73,6 +73,27 @@ panel_backlight: panel-backlight {
default-on;
status = "okay";
};
+
+ sd_adc0: sd-adc0 {
+ compatible = "sd-modulator";
+ #io-backend-cells = <0>;
+ vref-supply = <&v3v3>;
+ status = "okay";
+ };
+
+ sd_adc1: sd-adc1 {
+ compatible = "sd-modulator";
+ #io-backend-cells = <0>;
+ vref-supply = <&v3v3>;
+ status = "okay";
+ };
+
+ sd_adc2: sd-adc2 {
+ compatible = "sd-modulator";
+ #io-backend-cells = <0>;
+ vref-supply = <&v3v3>;
+ status = "okay";
+ };
};
&cec {
@@ -99,6 +120,53 @@ dcmi_0: endpoint {
};
};
+&dfsdm {
+ pinctrl-names = "default", "sleep";
+ pinctrl-0 = <&dfsdm_clkout_pins_a
+ &dfsdm_data1_pins_a &dfsdm_data3_pins_a>;
+ pinctrl-1 = <&dfsdm_clkout_sleep_pins_a
+ &dfsdm_data1_sleep_pins_a &dfsdm_data3_sleep_pins_a>;
+ spi-max-frequency = <2048000>;
+ status = "okay";
+
+ dfsdm0: filter@0 {
+ compatible = "st,stm32-dfsdm-adc";
+ st,filter-order = <3>;
+ status = "okay";
+
+ channel@0 {
+ reg = <0>;
+ label = "in0";
+ st,adc-channel-types = "SPI_F";
+ st,adc-channel-clk-src = "CLKOUT";
+ st,adc-alt-channel;
+ io-backends = <&sd_adc0>;
+ };
+
+ channel@1 {
+ reg = <1>;
+ label = "in1";
+ st,adc-channel-types = "SPI_R";
+ st,adc-channel-clk-src = "CLKOUT";
+ io-backends = <&sd_adc1>;
+ };
+ };
+
+ dfsdm1: filter@1 {
+ compatible = "st,stm32-dfsdm-adc";
+ st,filter-order = <3>;
+ status = "okay";
+
+ channel@3 {
+ reg = <3>;
+ label = "in3";
+ st,adc-channel-types = "SPI_R";
+ st,adc-channel-clk-src = "CLKOUT";
+ io-backends = <&sd_adc2>;
+ };
+ };
+};
+
&dsi {
phy-dsi-supply = <®18>;
#address-cells = <1>;
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [RFC v2 07/11] iio: adc: stm32-dfsdm: add scaling support to dfsdm
2023-07-27 15:03 ` Olivier Moysan
(?)
@ 2023-07-27 22:37 ` kernel test robot
-1 siblings, 0 replies; 40+ messages in thread
From: kernel test robot @ 2023-07-27 22:37 UTC (permalink / raw)
To: Olivier Moysan; +Cc: oe-kbuild-all
Hi Olivier,
[This is a private test report for your RFC patch.]
kernel test robot noticed the following build errors:
[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on atorgue-stm32/stm32-next robh/for-next linus/master v6.5-rc3 next-20230727]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Olivier-Moysan/iio-introduce-iio-backend-device/20230727-231112
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link: https://lore.kernel.org/r/20230727150324.1157933-8-olivier.moysan%40foss.st.com
patch subject: [RFC v2 07/11] iio: adc: stm32-dfsdm: add scaling support to dfsdm
config: loongarch-randconfig-r003-20230727 (https://download.01.org/0day-ci/archive/20230728/202307280647.YJVidOPD-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230728/202307280647.YJVidOPD-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202307280647.YJVidOPD-lkp@intel.com/
All errors (new ones prefixed by >>):
loongarch64-linux-ld: drivers/iio/adc/stm32-dfsdm-adc.o: in function `stm32_dfsdm_channel_parse_of':
>> stm32-dfsdm-adc.c:(.text+0x1d60): undefined reference to `fwnode_iio_backend_get'
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC v2 08/11] iio: adc: sd modulator: add scale and offset support
2023-07-27 15:03 ` [RFC v2 08/11] iio: adc: sd modulator: add scale and offset support Olivier Moysan
@ 2023-07-27 22:37 ` kernel test robot
2023-07-28 0:41 ` kernel test robot
1 sibling, 0 replies; 40+ messages in thread
From: kernel test robot @ 2023-07-27 22:37 UTC (permalink / raw)
To: Olivier Moysan; +Cc: oe-kbuild-all
Hi Olivier,
[This is a private test report for your RFC patch.]
kernel test robot noticed the following build errors:
[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on atorgue-stm32/stm32-next robh/for-next linus/master v6.5-rc3 next-20230727]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Olivier-Moysan/iio-introduce-iio-backend-device/20230727-231112
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link: https://lore.kernel.org/r/20230727150324.1157933-9-olivier.moysan%40foss.st.com
patch subject: [RFC v2 08/11] iio: adc: sd modulator: add scale and offset support
config: um-randconfig-r002-20230727 (https://download.01.org/0day-ci/archive/20230728/202307280606.nuOkLxgk-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230728/202307280606.nuOkLxgk-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202307280606.nuOkLxgk-lkp@intel.com/
All errors (new ones prefixed by >>):
/usr/bin/ld: drivers/iio/adc/sd_adc_modulator.o: in function `iio_sd_mod_probe':
>> sd_adc_modulator.c:(.text+0x378): undefined reference to `iio_backend_alloc'
>> /usr/bin/ld: sd_adc_modulator.c:(.text+0x3da): undefined reference to `iio_backend_register'
>> /usr/bin/ld: sd_adc_modulator.c:(.text+0x4fc): undefined reference to `iio_backend_free'
collect2: error: ld returned 1 exit status
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC v2 07/11] iio: adc: stm32-dfsdm: add scaling support to dfsdm
2023-07-27 15:03 ` Olivier Moysan
(?)
(?)
@ 2023-07-27 22:57 ` kernel test robot
-1 siblings, 0 replies; 40+ messages in thread
From: kernel test robot @ 2023-07-27 22:57 UTC (permalink / raw)
To: Olivier Moysan; +Cc: llvm, oe-kbuild-all
Hi Olivier,
[This is a private test report for your RFC patch.]
kernel test robot noticed the following build errors:
[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on atorgue-stm32/stm32-next robh/for-next linus/master v6.5-rc3 next-20230727]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Olivier-Moysan/iio-introduce-iio-backend-device/20230727-231112
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link: https://lore.kernel.org/r/20230727150324.1157933-8-olivier.moysan%40foss.st.com
patch subject: [RFC v2 07/11] iio: adc: stm32-dfsdm: add scaling support to dfsdm
config: arm-randconfig-r025-20230727 (https://download.01.org/0day-ci/archive/20230728/202307280630.WdzqZfZN-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce: (https://download.01.org/0day-ci/archive/20230728/202307280630.WdzqZfZN-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202307280630.WdzqZfZN-lkp@intel.com/
All errors (new ones prefixed by >>):
>> ld.lld: error: undefined symbol: fwnode_iio_backend_get
>>> referenced by stm32-dfsdm-adc.c
>>> drivers/iio/adc/stm32-dfsdm-adc.o:(stm32_dfsdm_generic_chan_init) in archive vmlinux.a
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC v2 08/11] iio: adc: sd modulator: add scale and offset support
2023-07-27 15:03 ` [RFC v2 08/11] iio: adc: sd modulator: add scale and offset support Olivier Moysan
2023-07-27 22:37 ` kernel test robot
@ 2023-07-28 0:41 ` kernel test robot
1 sibling, 0 replies; 40+ messages in thread
From: kernel test robot @ 2023-07-28 0:41 UTC (permalink / raw)
To: Olivier Moysan; +Cc: oe-kbuild-all
Hi Olivier,
[This is a private test report for your RFC patch.]
kernel test robot noticed the following build errors:
[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on atorgue-stm32/stm32-next robh/for-next linus/master v6.5-rc3 next-20230727]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Olivier-Moysan/iio-introduce-iio-backend-device/20230727-231112
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link: https://lore.kernel.org/r/20230727150324.1157933-9-olivier.moysan%40foss.st.com
patch subject: [RFC v2 08/11] iio: adc: sd modulator: add scale and offset support
config: i386-randconfig-i013-20230727 (https://download.01.org/0day-ci/archive/20230728/202307280851.AdNONS92-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230728/202307280851.AdNONS92-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202307280851.AdNONS92-lkp@intel.com/
All errors (new ones prefixed by >>):
ld: drivers/iio/adc/sd_adc_modulator.o: in function `iio_sd_mod_probe':
>> drivers/iio/adc/sd_adc_modulator.c:91: undefined reference to `iio_backend_alloc'
>> ld: drivers/iio/adc/sd_adc_modulator.c:100: undefined reference to `iio_backend_register'
>> ld: drivers/iio/adc/sd_adc_modulator.c:109: undefined reference to `iio_backend_free'
vim +91 drivers/iio/adc/sd_adc_modulator.c
70
71 static int iio_sd_mod_probe(struct platform_device *pdev)
72 {
73 struct device *dev = &pdev->dev;
74 struct regulator *vref;
75 struct iio_backend *backend;
76 struct iio_sd_mod_priv *priv;
77 int ret;
78
79 priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
80 if (!priv)
81 return -ENOMEM;
82
83 vref = devm_regulator_get_optional(dev, "vref");
84 if (IS_ERR(vref)) {
85 if (PTR_ERR(vref) != -ENODEV)
86 return dev_err_probe(dev, PTR_ERR(vref), "Failed to get vref\n");
87 } else {
88 priv->vref = vref;
89 }
90
> 91 backend = iio_backend_alloc(dev);
92 if (!backend) {
93 dev_err(dev, "Failed to allocate sd modulator backend data\n");
94 return -ENOMEM;
95 }
96
97 backend->priv = priv;
98 backend->ops = &sd_mod_ops;
99
> 100 ret = iio_backend_register(backend);
101 if (ret) {
102 dev_err(dev, "Failed to register backend: %d\n", ret);
103 goto err_free_backend;
104 }
105
106 return 0;
107
108 err_free_backend:
> 109 iio_backend_free(&backend->dev);
110
111 return ret;
112 };
113
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC v2 01/11] iio: introduce iio backend device
2023-07-27 15:03 ` [RFC v2 01/11] iio: introduce iio backend device Olivier Moysan
@ 2023-07-28 8:42 ` Nuno Sá
2023-08-31 16:14 ` Olivier MOYSAN
0 siblings, 1 reply; 40+ messages in thread
From: Nuno Sá @ 2023-07-28 8:42 UTC (permalink / raw)
To: Olivier Moysan, Jonathan Cameron, Lars-Peter Clausen
Cc: linux-kernel, linux-iio
Hi Olivier,
On Thu, 2023-07-27 at 17:03 +0200, Olivier Moysan wrote:
> Add a new device type in IIO framework.
> This backend device does not compute channel attributes and does not expose
> them through sysfs, as done typically in iio-rescale frontend device.
> Instead, it allows to report information applying to channel
> attributes through callbacks. These backend devices can be cascaded
> to represent chained components.
> An IIO device configured as a consumer of a backend device can compute
> the channel attributes of the whole chain.
>
> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
> ---
> drivers/iio/Makefile | 1 +
> drivers/iio/industrialio-backend.c | 107 +++++++++++++++++++++++++++++
> include/linux/iio/backend.h | 56 +++++++++++++++
> 3 files changed, 164 insertions(+)
> create mode 100644 drivers/iio/industrialio-backend.c
> create mode 100644 include/linux/iio/backend.h
>
> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
> index 9622347a1c1b..9b59c6ab1738 100644
> --- a/drivers/iio/Makefile
> +++ b/drivers/iio/Makefile
> @@ -5,6 +5,7 @@
>
> obj-$(CONFIG_IIO) += industrialio.o
> industrialio-y := industrialio-core.o industrialio-event.o inkern.o
> +industrialio-$(CONFIG_IIO_BACKEND) += industrialio-backend.o
> industrialio-$(CONFIG_IIO_BUFFER) += industrialio-buffer.o
> industrialio-$(CONFIG_IIO_TRIGGER) += industrialio-trigger.o
>
> diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-
> backend.c
> new file mode 100644
> index 000000000000..7d0625889873
> --- /dev/null
> +++ b/drivers/iio/industrialio-backend.c
> @@ -0,0 +1,107 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* The industrial I/O core, backend handling functions
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/device.h>
> +#include <linux/property.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/backend.h>
> +
> +static DEFINE_IDA(iio_backend_ida);
> +
> +#define to_iio_backend(_device) container_of((_device), struct iio_backend,
> dev)
> +
> +static void iio_backend_release(struct device *device)
> +{
> + struct iio_backend *backend = to_iio_backend(device);
> +
> + kfree(backend->name);
> + kfree(backend);
> +}
> +
> +static const struct device_type iio_backend_type = {
> + .release = iio_backend_release,
> + .name = "iio_backend_device",
> +};
> +
> +struct iio_backend *iio_backend_alloc(struct device *parent)
> +{
> + struct iio_backend *backend;
> +
> + backend = devm_kzalloc(parent, sizeof(*backend), GFP_KERNEL);
>
No error checking.
I guess a lot of cleanings are still missing but the important thing I wanted to
notice is that the above pattern is not ok.
Your 'struct iio_backend *backend'' embeds a 'stuct device' which is a
refcounted object. Nevertheless, you're binding the lifetime of your object to
the parent device and that is wrong. The reason is that as soon as your parent
device get's released or just unbinded from it's driver, all the devres stuff
(including your 'struct iio_backend' object) will be released independent of
your 'struct device' refcount value...
So, you might argue this won't ever be an issue in here but the pattern is still
wrong. There are some talks about this, the last one was given at the latest
EOSS:
https://www.youtube.com/watch?v=HCiJL7djGw8&list=PLbzoR-pLrL6pY8a8zSKRC6-AihFrruOkq&index=27&ab_channel=TheLinuxFoundation
- Nuno Sá
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC v2 03/11] dt-bindings: iio: stm32-dfsdm-adc: add scaling support
2023-07-27 15:03 ` Olivier Moysan
@ 2023-08-11 17:10 ` Rob Herring
-1 siblings, 0 replies; 40+ messages in thread
From: Rob Herring @ 2023-08-11 17:10 UTC (permalink / raw)
To: Olivier Moysan
Cc: Arnaud Pouliquen, Jonathan Cameron, Lars-Peter Clausen,
Krzysztof Kozlowski, Conor Dooley, Maxime Coquelin,
Alexandre Torgue, Fabrice Gasnier, alsa-devel, linux-iio,
devicetree, linux-stm32, linux-arm-kernel, linux-kernel
On Thu, Jul 27, 2023 at 05:03:14PM +0200, Olivier Moysan wrote:
> Add scaling support to STM32 DFSDM.
>
> This introduces the following changes:
Why?
> - Add ADC generic channel binding and remove support of deprecated
> channel bindings.
When was it deprecated?
> - DFSDM is now implemented as a channel provider, so remove io-channels
> properties.
> - Add iio-backend property to connect DFSDM to an SD modulator.
io-backends
All sorts of ABI issues with this change. Please explain why you don't
care.
>
> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
> ---
> .../bindings/iio/adc/st,stm32-dfsdm-adc.yaml | 189 ++++++------------
> 1 file changed, 63 insertions(+), 126 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 1970503389aa..128545cedc7f 100644
> --- a/Documentation/devicetree/bindings/iio/adc/st,stm32-dfsdm-adc.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/st,stm32-dfsdm-adc.yaml
> @@ -85,22 +85,14 @@ patternProperties:
> description: Specifies the DFSDM filter instance used.
> maxItems: 1
>
> - interrupts:
> - maxItems: 1
> + '#address-cells':
> + const: 1
>
> - st,adc-channels:
> - description: |
> - List of single-ended channels muxed for this ADC.
> - On stm32h7 and stm32mp1:
> - - For st,stm32-dfsdm-adc: up to 8 channels numbered from 0 to 7.
> - - For st,stm32-dfsdm-dmic: 1 channel numbered from 0 to 7.
> - $ref: /schemas/types.yaml#/definitions/uint32-array
> - items:
> - minimum: 0
> - maximum: 7
> + '#size-cells':
> + const: 0
>
> - st,adc-channel-names:
> - description: List of single-ended channel names.
> + interrupts:
> + maxItems: 1
>
> st,filter-order:
> description: |
> @@ -111,39 +103,6 @@ patternProperties:
> $ref: /schemas/types.yaml#/definitions/uint32
> maximum: 5
>
> - "#io-channel-cells":
> - const: 1
> -
> - 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
> -
> st,filter0-sync:
> description:
> Set to 1 to synchronize with DFSDM filter instance 0.
> @@ -157,14 +116,68 @@ patternProperties:
> items:
> - const: rx
>
> + 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: 19
> +
> + label:
> + description: |
> + Unique name to identify channel.
> +
> + 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: |
> + phandle to an external sigma delta modulator or internal ADC output.
> + $ref: /schemas/types.yaml#/definitions/phandle
> +
> + required:
> + - reg
> + - io-backends
> +
> + additionalProperties: false
> +
> required:
> - compatible
> - reg
> - interrupts
> - - st,adc-channels
> - - st,adc-channel-names
> - st,filter-order
> - - "#io-channel-cells"
>
> allOf:
> - if:
> @@ -175,14 +188,6 @@ patternProperties:
>
> then:
> properties:
> - st,adc-channels:
> - minItems: 1
> - maxItems: 8
> -
> - st,adc-channel-names:
> - minItems: 1
> - maxItems: 8
> -
> st,adc-channel-types:
> minItems: 1
> maxItems: 8
> @@ -191,14 +196,6 @@ patternProperties:
> minItems: 1
> maxItems: 8
>
> - io-channels:
> - description:
> - From common IIO binding. Used to pipe external sigma delta
> - modulator or internal ADC output to DFSDM channel.
> -
> - required:
> - - io-channels
> -
> - if:
> properties:
> compatible:
> @@ -207,12 +204,6 @@ patternProperties:
>
> then:
> properties:
> - st,adc-channels:
> - maxItems: 1
> -
> - st,adc-channel-names:
> - maxItems: 1
> -
> st,adc-channel-types:
> maxItems: 1
>
> @@ -237,15 +228,9 @@ patternProperties:
> "#sound-dai-cells":
> const: 0
>
> - io-channels:
> - description:
> - From common IIO binding. Used to pipe external sigma delta
> - modulator or internal ADC output to DFSDM channel.
> -
> required:
> - compatible
> - "#sound-dai-cells"
> - - io-channels
>
> allOf:
> - if:
> @@ -278,52 +263,4 @@ allOf:
> minimum: 0
> maximum: 5
>
> -examples:
> - - |
> - #include <dt-bindings/interrupt-controller/arm-gic.h>
> - #include <dt-bindings/clock/stm32mp1-clks.h>
> - dfsdm: 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>;
> -
> - 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>;
> - st,adc-channels = <1>;
> - st,adc-channel-names = "dmic0";
> - st,adc-channel-types = "SPI_R";
> - st,adc-channel-clk-src = "CLKOUT";
> - st,filter-order = <5>;
> -
> - asoc_pdm0: dfsdm-dai {
> - compatible = "st,stm32h7-dfsdm-dai";
> - #sound-dai-cells = <0>;
> - io-channels = <&dfsdm0 0>;
> - };
> - };
> -
> - dfsdm_pdm1: 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";
> - #io-channel-cells = <1>;
> - st,adc-channels = <2 3>;
> - st,adc-channel-names = "in2", "in3";
> - st,adc-channel-types = "SPI_R", "SPI_R";
> - st,adc-channel-clk-src = "CLKOUT_F", "CLKOUT_F";
> - io-channels = <&sd_adc2 &sd_adc3>;
> - st,filter-order = <1>;
> - };
> - };
> -
> ...
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC v2 03/11] dt-bindings: iio: stm32-dfsdm-adc: add scaling support
@ 2023-08-11 17:10 ` Rob Herring
0 siblings, 0 replies; 40+ messages in thread
From: Rob Herring @ 2023-08-11 17:10 UTC (permalink / raw)
To: Olivier Moysan
Cc: Arnaud Pouliquen, Jonathan Cameron, Lars-Peter Clausen,
Krzysztof Kozlowski, Conor Dooley, Maxime Coquelin,
Alexandre Torgue, Fabrice Gasnier, alsa-devel, linux-iio,
devicetree, linux-stm32, linux-arm-kernel, linux-kernel
On Thu, Jul 27, 2023 at 05:03:14PM +0200, Olivier Moysan wrote:
> Add scaling support to STM32 DFSDM.
>
> This introduces the following changes:
Why?
> - Add ADC generic channel binding and remove support of deprecated
> channel bindings.
When was it deprecated?
> - DFSDM is now implemented as a channel provider, so remove io-channels
> properties.
> - Add iio-backend property to connect DFSDM to an SD modulator.
io-backends
All sorts of ABI issues with this change. Please explain why you don't
care.
>
> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
> ---
> .../bindings/iio/adc/st,stm32-dfsdm-adc.yaml | 189 ++++++------------
> 1 file changed, 63 insertions(+), 126 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 1970503389aa..128545cedc7f 100644
> --- a/Documentation/devicetree/bindings/iio/adc/st,stm32-dfsdm-adc.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/st,stm32-dfsdm-adc.yaml
> @@ -85,22 +85,14 @@ patternProperties:
> description: Specifies the DFSDM filter instance used.
> maxItems: 1
>
> - interrupts:
> - maxItems: 1
> + '#address-cells':
> + const: 1
>
> - st,adc-channels:
> - description: |
> - List of single-ended channels muxed for this ADC.
> - On stm32h7 and stm32mp1:
> - - For st,stm32-dfsdm-adc: up to 8 channels numbered from 0 to 7.
> - - For st,stm32-dfsdm-dmic: 1 channel numbered from 0 to 7.
> - $ref: /schemas/types.yaml#/definitions/uint32-array
> - items:
> - minimum: 0
> - maximum: 7
> + '#size-cells':
> + const: 0
>
> - st,adc-channel-names:
> - description: List of single-ended channel names.
> + interrupts:
> + maxItems: 1
>
> st,filter-order:
> description: |
> @@ -111,39 +103,6 @@ patternProperties:
> $ref: /schemas/types.yaml#/definitions/uint32
> maximum: 5
>
> - "#io-channel-cells":
> - const: 1
> -
> - 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
> -
> st,filter0-sync:
> description:
> Set to 1 to synchronize with DFSDM filter instance 0.
> @@ -157,14 +116,68 @@ patternProperties:
> items:
> - const: rx
>
> + 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: 19
> +
> + label:
> + description: |
> + Unique name to identify channel.
> +
> + 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: |
> + phandle to an external sigma delta modulator or internal ADC output.
> + $ref: /schemas/types.yaml#/definitions/phandle
> +
> + required:
> + - reg
> + - io-backends
> +
> + additionalProperties: false
> +
> required:
> - compatible
> - reg
> - interrupts
> - - st,adc-channels
> - - st,adc-channel-names
> - st,filter-order
> - - "#io-channel-cells"
>
> allOf:
> - if:
> @@ -175,14 +188,6 @@ patternProperties:
>
> then:
> properties:
> - st,adc-channels:
> - minItems: 1
> - maxItems: 8
> -
> - st,adc-channel-names:
> - minItems: 1
> - maxItems: 8
> -
> st,adc-channel-types:
> minItems: 1
> maxItems: 8
> @@ -191,14 +196,6 @@ patternProperties:
> minItems: 1
> maxItems: 8
>
> - io-channels:
> - description:
> - From common IIO binding. Used to pipe external sigma delta
> - modulator or internal ADC output to DFSDM channel.
> -
> - required:
> - - io-channels
> -
> - if:
> properties:
> compatible:
> @@ -207,12 +204,6 @@ patternProperties:
>
> then:
> properties:
> - st,adc-channels:
> - maxItems: 1
> -
> - st,adc-channel-names:
> - maxItems: 1
> -
> st,adc-channel-types:
> maxItems: 1
>
> @@ -237,15 +228,9 @@ patternProperties:
> "#sound-dai-cells":
> const: 0
>
> - io-channels:
> - description:
> - From common IIO binding. Used to pipe external sigma delta
> - modulator or internal ADC output to DFSDM channel.
> -
> required:
> - compatible
> - "#sound-dai-cells"
> - - io-channels
>
> allOf:
> - if:
> @@ -278,52 +263,4 @@ allOf:
> minimum: 0
> maximum: 5
>
> -examples:
> - - |
> - #include <dt-bindings/interrupt-controller/arm-gic.h>
> - #include <dt-bindings/clock/stm32mp1-clks.h>
> - dfsdm: 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>;
> -
> - 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>;
> - st,adc-channels = <1>;
> - st,adc-channel-names = "dmic0";
> - st,adc-channel-types = "SPI_R";
> - st,adc-channel-clk-src = "CLKOUT";
> - st,filter-order = <5>;
> -
> - asoc_pdm0: dfsdm-dai {
> - compatible = "st,stm32h7-dfsdm-dai";
> - #sound-dai-cells = <0>;
> - io-channels = <&dfsdm0 0>;
> - };
> - };
> -
> - dfsdm_pdm1: 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";
> - #io-channel-cells = <1>;
> - st,adc-channels = <2 3>;
> - st,adc-channel-names = "in2", "in3";
> - st,adc-channel-types = "SPI_R", "SPI_R";
> - st,adc-channel-clk-src = "CLKOUT_F", "CLKOUT_F";
> - io-channels = <&sd_adc2 &sd_adc3>;
> - st,filter-order = <1>;
> - };
> - };
> -
> ...
> --
> 2.25.1
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC v2 03/11] dt-bindings: iio: stm32-dfsdm-adc: add scaling support
2023-08-11 17:10 ` Rob Herring
@ 2023-08-31 15:53 ` Olivier MOYSAN
-1 siblings, 0 replies; 40+ messages in thread
From: Olivier MOYSAN @ 2023-08-31 15:53 UTC (permalink / raw)
To: Rob Herring
Cc: Arnaud Pouliquen, Jonathan Cameron, Lars-Peter Clausen,
Krzysztof Kozlowski, Conor Dooley, Maxime Coquelin,
Alexandre Torgue, Fabrice Gasnier, alsa-devel, linux-iio,
devicetree, linux-stm32, linux-arm-kernel, linux-kernel
Hi Rob,
On 8/11/23 19:10, Rob Herring wrote:
> On Thu, Jul 27, 2023 at 05:03:14PM +0200, Olivier Moysan wrote:
>> Add scaling support to STM32 DFSDM.
>>
>> This introduces the following changes:
>
> Why?
>
This RFC is an attempt to support scaling through a change in DFSDM
topology.
These changes have some impacts on DFSDM and sd modulator bindings.
To keep things simple in this RFC, I skipped legacy support, to put the
emphasis on the new bindings proposal.
There are two changes here: adoption of the generic IIO bindings and new
"io-backends" property. This needs to be put in two separate patches at
the end, I think (as already done for driver patches)
Anyway, the current bindings would become deprecated with these changes.
I still have to consider how to support these legacy bindings for next
step, However.
BRs
Olivier
>> - Add ADC generic channel binding and remove support of deprecated
>> channel bindings.
>
> When was it deprecated?
>
>> - DFSDM is now implemented as a channel provider, so remove io-channels
>> properties.
>> - Add iio-backend property to connect DFSDM to an SD modulator.
>
> io-backends
>
> All sorts of ABI issues with this change. Please explain why you don't
> care.
>
>>
>> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
>> ---
>> .../bindings/iio/adc/st,stm32-dfsdm-adc.yaml | 189 ++++++------------
>> 1 file changed, 63 insertions(+), 126 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 1970503389aa..128545cedc7f 100644
>> --- a/Documentation/devicetree/bindings/iio/adc/st,stm32-dfsdm-adc.yaml
>> +++ b/Documentation/devicetree/bindings/iio/adc/st,stm32-dfsdm-adc.yaml
>> @@ -85,22 +85,14 @@ patternProperties:
>> description: Specifies the DFSDM filter instance used.
>> maxItems: 1
>>
>> - interrupts:
>> - maxItems: 1
>> + '#address-cells':
>> + const: 1
>>
>> - st,adc-channels:
>> - description: |
>> - List of single-ended channels muxed for this ADC.
>> - On stm32h7 and stm32mp1:
>> - - For st,stm32-dfsdm-adc: up to 8 channels numbered from 0 to 7.
>> - - For st,stm32-dfsdm-dmic: 1 channel numbered from 0 to 7.
>> - $ref: /schemas/types.yaml#/definitions/uint32-array
>> - items:
>> - minimum: 0
>> - maximum: 7
>> + '#size-cells':
>> + const: 0
>>
>> - st,adc-channel-names:
>> - description: List of single-ended channel names.
>> + interrupts:
>> + maxItems: 1
>>
>> st,filter-order:
>> description: |
>> @@ -111,39 +103,6 @@ patternProperties:
>> $ref: /schemas/types.yaml#/definitions/uint32
>> maximum: 5
>>
>> - "#io-channel-cells":
>> - const: 1
>> -
>> - 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
>> -
>> st,filter0-sync:
>> description:
>> Set to 1 to synchronize with DFSDM filter instance 0.
>> @@ -157,14 +116,68 @@ patternProperties:
>> items:
>> - const: rx
>>
>> + 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: 19
>> +
>> + label:
>> + description: |
>> + Unique name to identify channel.
>> +
>> + 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: |
>> + phandle to an external sigma delta modulator or internal ADC output.
>> + $ref: /schemas/types.yaml#/definitions/phandle
>> +
>> + required:
>> + - reg
>> + - io-backends
>> +
>> + additionalProperties: false
>> +
>> required:
>> - compatible
>> - reg
>> - interrupts
>> - - st,adc-channels
>> - - st,adc-channel-names
>> - st,filter-order
>> - - "#io-channel-cells"
>>
>> allOf:
>> - if:
>> @@ -175,14 +188,6 @@ patternProperties:
>>
>> then:
>> properties:
>> - st,adc-channels:
>> - minItems: 1
>> - maxItems: 8
>> -
>> - st,adc-channel-names:
>> - minItems: 1
>> - maxItems: 8
>> -
>> st,adc-channel-types:
>> minItems: 1
>> maxItems: 8
>> @@ -191,14 +196,6 @@ patternProperties:
>> minItems: 1
>> maxItems: 8
>>
>> - io-channels:
>> - description:
>> - From common IIO binding. Used to pipe external sigma delta
>> - modulator or internal ADC output to DFSDM channel.
>> -
>> - required:
>> - - io-channels
>> -
>> - if:
>> properties:
>> compatible:
>> @@ -207,12 +204,6 @@ patternProperties:
>>
>> then:
>> properties:
>> - st,adc-channels:
>> - maxItems: 1
>> -
>> - st,adc-channel-names:
>> - maxItems: 1
>> -
>> st,adc-channel-types:
>> maxItems: 1
>>
>> @@ -237,15 +228,9 @@ patternProperties:
>> "#sound-dai-cells":
>> const: 0
>>
>> - io-channels:
>> - description:
>> - From common IIO binding. Used to pipe external sigma delta
>> - modulator or internal ADC output to DFSDM channel.
>> -
>> required:
>> - compatible
>> - "#sound-dai-cells"
>> - - io-channels
>>
>> allOf:
>> - if:
>> @@ -278,52 +263,4 @@ allOf:
>> minimum: 0
>> maximum: 5
>>
>> -examples:
>> - - |
>> - #include <dt-bindings/interrupt-controller/arm-gic.h>
>> - #include <dt-bindings/clock/stm32mp1-clks.h>
>> - dfsdm: 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>;
>> -
>> - 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>;
>> - st,adc-channels = <1>;
>> - st,adc-channel-names = "dmic0";
>> - st,adc-channel-types = "SPI_R";
>> - st,adc-channel-clk-src = "CLKOUT";
>> - st,filter-order = <5>;
>> -
>> - asoc_pdm0: dfsdm-dai {
>> - compatible = "st,stm32h7-dfsdm-dai";
>> - #sound-dai-cells = <0>;
>> - io-channels = <&dfsdm0 0>;
>> - };
>> - };
>> -
>> - dfsdm_pdm1: 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";
>> - #io-channel-cells = <1>;
>> - st,adc-channels = <2 3>;
>> - st,adc-channel-names = "in2", "in3";
>> - st,adc-channel-types = "SPI_R", "SPI_R";
>> - st,adc-channel-clk-src = "CLKOUT_F", "CLKOUT_F";
>> - io-channels = <&sd_adc2 &sd_adc3>;
>> - st,filter-order = <1>;
>> - };
>> - };
>> -
>> ...
>> --
>> 2.25.1
>>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC v2 03/11] dt-bindings: iio: stm32-dfsdm-adc: add scaling support
@ 2023-08-31 15:53 ` Olivier MOYSAN
0 siblings, 0 replies; 40+ messages in thread
From: Olivier MOYSAN @ 2023-08-31 15:53 UTC (permalink / raw)
To: Rob Herring
Cc: Arnaud Pouliquen, Jonathan Cameron, Lars-Peter Clausen,
Krzysztof Kozlowski, Conor Dooley, Maxime Coquelin,
Alexandre Torgue, Fabrice Gasnier, alsa-devel, linux-iio,
devicetree, linux-stm32, linux-arm-kernel, linux-kernel
Hi Rob,
On 8/11/23 19:10, Rob Herring wrote:
> On Thu, Jul 27, 2023 at 05:03:14PM +0200, Olivier Moysan wrote:
>> Add scaling support to STM32 DFSDM.
>>
>> This introduces the following changes:
>
> Why?
>
This RFC is an attempt to support scaling through a change in DFSDM
topology.
These changes have some impacts on DFSDM and sd modulator bindings.
To keep things simple in this RFC, I skipped legacy support, to put the
emphasis on the new bindings proposal.
There are two changes here: adoption of the generic IIO bindings and new
"io-backends" property. This needs to be put in two separate patches at
the end, I think (as already done for driver patches)
Anyway, the current bindings would become deprecated with these changes.
I still have to consider how to support these legacy bindings for next
step, However.
BRs
Olivier
>> - Add ADC generic channel binding and remove support of deprecated
>> channel bindings.
>
> When was it deprecated?
>
>> - DFSDM is now implemented as a channel provider, so remove io-channels
>> properties.
>> - Add iio-backend property to connect DFSDM to an SD modulator.
>
> io-backends
>
> All sorts of ABI issues with this change. Please explain why you don't
> care.
>
>>
>> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
>> ---
>> .../bindings/iio/adc/st,stm32-dfsdm-adc.yaml | 189 ++++++------------
>> 1 file changed, 63 insertions(+), 126 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 1970503389aa..128545cedc7f 100644
>> --- a/Documentation/devicetree/bindings/iio/adc/st,stm32-dfsdm-adc.yaml
>> +++ b/Documentation/devicetree/bindings/iio/adc/st,stm32-dfsdm-adc.yaml
>> @@ -85,22 +85,14 @@ patternProperties:
>> description: Specifies the DFSDM filter instance used.
>> maxItems: 1
>>
>> - interrupts:
>> - maxItems: 1
>> + '#address-cells':
>> + const: 1
>>
>> - st,adc-channels:
>> - description: |
>> - List of single-ended channels muxed for this ADC.
>> - On stm32h7 and stm32mp1:
>> - - For st,stm32-dfsdm-adc: up to 8 channels numbered from 0 to 7.
>> - - For st,stm32-dfsdm-dmic: 1 channel numbered from 0 to 7.
>> - $ref: /schemas/types.yaml#/definitions/uint32-array
>> - items:
>> - minimum: 0
>> - maximum: 7
>> + '#size-cells':
>> + const: 0
>>
>> - st,adc-channel-names:
>> - description: List of single-ended channel names.
>> + interrupts:
>> + maxItems: 1
>>
>> st,filter-order:
>> description: |
>> @@ -111,39 +103,6 @@ patternProperties:
>> $ref: /schemas/types.yaml#/definitions/uint32
>> maximum: 5
>>
>> - "#io-channel-cells":
>> - const: 1
>> -
>> - 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
>> -
>> st,filter0-sync:
>> description:
>> Set to 1 to synchronize with DFSDM filter instance 0.
>> @@ -157,14 +116,68 @@ patternProperties:
>> items:
>> - const: rx
>>
>> + 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: 19
>> +
>> + label:
>> + description: |
>> + Unique name to identify channel.
>> +
>> + 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: |
>> + phandle to an external sigma delta modulator or internal ADC output.
>> + $ref: /schemas/types.yaml#/definitions/phandle
>> +
>> + required:
>> + - reg
>> + - io-backends
>> +
>> + additionalProperties: false
>> +
>> required:
>> - compatible
>> - reg
>> - interrupts
>> - - st,adc-channels
>> - - st,adc-channel-names
>> - st,filter-order
>> - - "#io-channel-cells"
>>
>> allOf:
>> - if:
>> @@ -175,14 +188,6 @@ patternProperties:
>>
>> then:
>> properties:
>> - st,adc-channels:
>> - minItems: 1
>> - maxItems: 8
>> -
>> - st,adc-channel-names:
>> - minItems: 1
>> - maxItems: 8
>> -
>> st,adc-channel-types:
>> minItems: 1
>> maxItems: 8
>> @@ -191,14 +196,6 @@ patternProperties:
>> minItems: 1
>> maxItems: 8
>>
>> - io-channels:
>> - description:
>> - From common IIO binding. Used to pipe external sigma delta
>> - modulator or internal ADC output to DFSDM channel.
>> -
>> - required:
>> - - io-channels
>> -
>> - if:
>> properties:
>> compatible:
>> @@ -207,12 +204,6 @@ patternProperties:
>>
>> then:
>> properties:
>> - st,adc-channels:
>> - maxItems: 1
>> -
>> - st,adc-channel-names:
>> - maxItems: 1
>> -
>> st,adc-channel-types:
>> maxItems: 1
>>
>> @@ -237,15 +228,9 @@ patternProperties:
>> "#sound-dai-cells":
>> const: 0
>>
>> - io-channels:
>> - description:
>> - From common IIO binding. Used to pipe external sigma delta
>> - modulator or internal ADC output to DFSDM channel.
>> -
>> required:
>> - compatible
>> - "#sound-dai-cells"
>> - - io-channels
>>
>> allOf:
>> - if:
>> @@ -278,52 +263,4 @@ allOf:
>> minimum: 0
>> maximum: 5
>>
>> -examples:
>> - - |
>> - #include <dt-bindings/interrupt-controller/arm-gic.h>
>> - #include <dt-bindings/clock/stm32mp1-clks.h>
>> - dfsdm: 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>;
>> -
>> - 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>;
>> - st,adc-channels = <1>;
>> - st,adc-channel-names = "dmic0";
>> - st,adc-channel-types = "SPI_R";
>> - st,adc-channel-clk-src = "CLKOUT";
>> - st,filter-order = <5>;
>> -
>> - asoc_pdm0: dfsdm-dai {
>> - compatible = "st,stm32h7-dfsdm-dai";
>> - #sound-dai-cells = <0>;
>> - io-channels = <&dfsdm0 0>;
>> - };
>> - };
>> -
>> - dfsdm_pdm1: 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";
>> - #io-channel-cells = <1>;
>> - st,adc-channels = <2 3>;
>> - st,adc-channel-names = "in2", "in3";
>> - st,adc-channel-types = "SPI_R", "SPI_R";
>> - st,adc-channel-clk-src = "CLKOUT_F", "CLKOUT_F";
>> - io-channels = <&sd_adc2 &sd_adc3>;
>> - st,filter-order = <1>;
>> - };
>> - };
>> -
>> ...
>> --
>> 2.25.1
>>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC v2 01/11] iio: introduce iio backend device
2023-07-28 8:42 ` Nuno Sá
@ 2023-08-31 16:14 ` Olivier MOYSAN
2023-09-01 8:01 ` Nuno Sá
0 siblings, 1 reply; 40+ messages in thread
From: Olivier MOYSAN @ 2023-08-31 16:14 UTC (permalink / raw)
To: Nuno Sá, Jonathan Cameron, Lars-Peter Clausen
Cc: linux-kernel, linux-iio, Fabrice GASNIER
Hi Nuno,
On 7/28/23 10:42, Nuno Sá wrote:
> Hi Olivier,
>
> On Thu, 2023-07-27 at 17:03 +0200, Olivier Moysan wrote:
>> Add a new device type in IIO framework.
>> This backend device does not compute channel attributes and does not expose
>> them through sysfs, as done typically in iio-rescale frontend device.
>> Instead, it allows to report information applying to channel
>> attributes through callbacks. These backend devices can be cascaded
>> to represent chained components.
>> An IIO device configured as a consumer of a backend device can compute
>> the channel attributes of the whole chain.
>>
>> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
>> ---
>> drivers/iio/Makefile | 1 +
>> drivers/iio/industrialio-backend.c | 107 +++++++++++++++++++++++++++++
>> include/linux/iio/backend.h | 56 +++++++++++++++
>> 3 files changed, 164 insertions(+)
>> create mode 100644 drivers/iio/industrialio-backend.c
>> create mode 100644 include/linux/iio/backend.h
>>
>> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
>> index 9622347a1c1b..9b59c6ab1738 100644
>> --- a/drivers/iio/Makefile
>> +++ b/drivers/iio/Makefile
>> @@ -5,6 +5,7 @@
>>
>> obj-$(CONFIG_IIO) += industrialio.o
>> industrialio-y := industrialio-core.o industrialio-event.o inkern.o
>> +industrialio-$(CONFIG_IIO_BACKEND) += industrialio-backend.o
>> industrialio-$(CONFIG_IIO_BUFFER) += industrialio-buffer.o
>> industrialio-$(CONFIG_IIO_TRIGGER) += industrialio-trigger.o
>>
>> diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-
>> backend.c
>> new file mode 100644
>> index 000000000000..7d0625889873
>> --- /dev/null
>> +++ b/drivers/iio/industrialio-backend.c
>> @@ -0,0 +1,107 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* The industrial I/O core, backend handling functions
>> + *
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/device.h>
>> +#include <linux/property.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/backend.h>
>> +
>> +static DEFINE_IDA(iio_backend_ida);
>> +
>> +#define to_iio_backend(_device) container_of((_device), struct iio_backend,
>> dev)
>> +
>> +static void iio_backend_release(struct device *device)
>> +{
>> + struct iio_backend *backend = to_iio_backend(device);
>> +
>> + kfree(backend->name);
>> + kfree(backend);
>> +}
>> +
>> +static const struct device_type iio_backend_type = {
>> + .release = iio_backend_release,
>> + .name = "iio_backend_device",
>> +};
>> +
>> +struct iio_backend *iio_backend_alloc(struct device *parent)
>> +{
>> + struct iio_backend *backend;
>> +
>> + backend = devm_kzalloc(parent, sizeof(*backend), GFP_KERNEL);
>>
>
> No error checking.
>
> I guess a lot of cleanings are still missing but the important thing I wanted to
> notice is that the above pattern is not ok.
> Your 'struct iio_backend *backend'' embeds a 'stuct device' which is a
> refcounted object. Nevertheless, you're binding the lifetime of your object to
> the parent device and that is wrong. The reason is that as soon as your parent
> device get's released or just unbinded from it's driver, all the devres stuff
> (including your 'struct iio_backend' object) will be released independentof
> your 'struct device' refcount value...
>
> So, you might argue this won't ever be an issue in here but the pattern is still
> wrong. There are some talks about this, the last one was given at the latest
> EOSS:
>
> https://www.youtube.com/watch?v=HCiJL7djGw8&list=PLbzoR-pLrL6pY8a8zSKRC6-AihFrruOkq&index=27&ab_channel=TheLinuxFoundation
>
This is a good point. Thanks for pointing it out. Sure, there are still
many things to improve.
I have seen the comment from Jonathan on your "Add converter framework"
serie. I had a quick look at the serie. It seems that we share the need
to aggregate some IIO devices. But I need to read it more carefully to
check if we can find some convergences here.
BRs
Olivier
> - Nuno Sá
>
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC v2 01/11] iio: introduce iio backend device
2023-08-31 16:14 ` Olivier MOYSAN
@ 2023-09-01 8:01 ` Nuno Sá
2023-09-03 10:46 ` Jonathan Cameron
2023-09-05 10:06 ` Olivier MOYSAN
0 siblings, 2 replies; 40+ messages in thread
From: Nuno Sá @ 2023-09-01 8:01 UTC (permalink / raw)
To: Olivier MOYSAN, Jonathan Cameron, Lars-Peter Clausen
Cc: linux-kernel, linux-iio, Fabrice GASNIER
Hi Olivier,
On Thu, 2023-08-31 at 18:14 +0200, Olivier MOYSAN wrote:
> Hi Nuno,
>
> On 7/28/23 10:42, Nuno Sá wrote:
> > Hi Olivier,
> >
> > On Thu, 2023-07-27 at 17:03 +0200, Olivier Moysan wrote:
> > > Add a new device type in IIO framework.
> > > This backend device does not compute channel attributes and does not expose
> > > them through sysfs, as done typically in iio-rescale frontend device.
> > > Instead, it allows to report information applying to channel
> > > attributes through callbacks. These backend devices can be cascaded
> > > to represent chained components.
> > > An IIO device configured as a consumer of a backend device can compute
> > > the channel attributes of the whole chain.
> > >
> > > Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
> > > ---
> > > drivers/iio/Makefile | 1 +
> > > drivers/iio/industrialio-backend.c | 107 +++++++++++++++++++++++++++++
> > > include/linux/iio/backend.h | 56 +++++++++++++++
> > > 3 files changed, 164 insertions(+)
> > > create mode 100644 drivers/iio/industrialio-backend.c
> > > create mode 100644 include/linux/iio/backend.h
> > >
> > > diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
> > > index 9622347a1c1b..9b59c6ab1738 100644
> > > --- a/drivers/iio/Makefile
> > > +++ b/drivers/iio/Makefile
> > > @@ -5,6 +5,7 @@
> > >
> > > obj-$(CONFIG_IIO) += industrialio.o
> > > industrialio-y := industrialio-core.o industrialio-event.o inkern.o
> > > +industrialio-$(CONFIG_IIO_BACKEND) += industrialio-backend.o
> > > industrialio-$(CONFIG_IIO_BUFFER) += industrialio-buffer.o
> > > industrialio-$(CONFIG_IIO_TRIGGER) += industrialio-trigger.o
> > >
> > > diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-
> > > backend.c
> > > new file mode 100644
> > > index 000000000000..7d0625889873
> > > --- /dev/null
> > > +++ b/drivers/iio/industrialio-backend.c
> > > @@ -0,0 +1,107 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/* The industrial I/O core, backend handling functions
> > > + *
> > > + */
> > > +
> > > +#include <linux/kernel.h>
> > > +#include <linux/device.h>
> > > +#include <linux/property.h>
> > > +#include <linux/iio/iio.h>
> > > +#include <linux/iio/backend.h>
> > > +
> > > +static DEFINE_IDA(iio_backend_ida);
> > > +
> > > +#define to_iio_backend(_device) container_of((_device), struct iio_backend,
> > > dev)
> > > +
> > > +static void iio_backend_release(struct device *device)
> > > +{
> > > + struct iio_backend *backend = to_iio_backend(device);
> > > +
> > > + kfree(backend->name);
> > > + kfree(backend);
> > > +}
> > > +
> > > +static const struct device_type iio_backend_type = {
> > > + .release = iio_backend_release,
> > > + .name = "iio_backend_device",
> > > +};
> > > +
> > > +struct iio_backend *iio_backend_alloc(struct device *parent)
> > > +{
> > > + struct iio_backend *backend;
> > > +
> > > + backend = devm_kzalloc(parent, sizeof(*backend), GFP_KERNEL);
> > >
> >
> > No error checking.
> >
> > I guess a lot of cleanings are still missing but the important thing I wanted to
> > notice is that the above pattern is not ok.
> > Your 'struct iio_backend *backend'' embeds a 'stuct device' which is a
> > refcounted object. Nevertheless, you're binding the lifetime of your object to
> > the parent device and that is wrong. The reason is that as soon as your parent
> > device get's released or just unbinded from it's driver, all the devres stuff
> > (including your 'struct iio_backend' object) will be released independentof
> > your 'struct device' refcount value...
> >
> > So, you might argue this won't ever be an issue in here but the pattern is still
> > wrong. There are some talks about this, the last one was given at the latest
> > EOSS:
> >
> > https://www.youtube.com/watch?v=HCiJL7djGw8&list=PLbzoR-pLrL6pY8a8zSKRC6-AihFrruOkq&index=27&ab_channel=TheLinuxFoundation
> >
>
> This is a good point. Thanks for pointing it out. Sure, there are still
> many things to improve.
>
> I have seen the comment from Jonathan on your "Add converter framework"
> serie. I had a quick look at the serie. It seems that we share the need
> to aggregate some IIO devices. But I need to read it more carefully to
> check if we can find some convergences here.
Yeah, In my case, the backend devices are typically FPGA soft cores and the aggregate
device might connect to multiple of these backends. That was one of the reason why I
used the component API where the aggregate device is only configured when all the
devices are probed. Similarly, when one of them is unbind, the whole thing should be
torn down. Also, in my case, the frontend device needs to do a lot of setup on the
backend device so the whole thing works (so I do have/need a lot more .ops).
Anyways, it does not matter much what the backend device is and from a first glance
and looking at the .ops you have, it seems that this could easily be supported in the
framework I'm adding. The only things I'm seeing are:
1) You would need to use the component API if it's ok. Also not sure if the cascaded
usecase you mention would work with that API.
2) We would need to add the .read_raw() op. If you look at my RFC, I already have
some comments/concerns about having an option like that (see there).
Having said that, none of the above are blockers as 1), I can ditch the component API
in favour of typical FW/OF lookup (even though the component API makes some things
easier to handle) and 2), adding a .read_raw() op is not a blocker for me.
Alternatively, another (maybe crazy) idea would be to have this framework have the
really generic stuff (like lookup + generic ops) and build my iio-converter on top of
it (extending it). You know, some OO fun :). Maybe not worth the trouble though.
Let's if Jonathan has some suggestions on how to proceed...
- Nuno Sá
> >
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC v2 01/11] iio: introduce iio backend device
2023-09-01 8:01 ` Nuno Sá
@ 2023-09-03 10:46 ` Jonathan Cameron
2023-09-05 10:06 ` Olivier MOYSAN
1 sibling, 0 replies; 40+ messages in thread
From: Jonathan Cameron @ 2023-09-03 10:46 UTC (permalink / raw)
To: Nuno Sá
Cc: Olivier MOYSAN, Lars-Peter Clausen, linux-kernel, linux-iio,
Fabrice GASNIER
On Fri, 01 Sep 2023 10:01:19 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:
> Hi Olivier,
>
> On Thu, 2023-08-31 at 18:14 +0200, Olivier MOYSAN wrote:
> > Hi Nuno,
> >
> > On 7/28/23 10:42, Nuno Sá wrote:
> > > Hi Olivier,
> > >
> > > On Thu, 2023-07-27 at 17:03 +0200, Olivier Moysan wrote:
> > > > Add a new device type in IIO framework.
> > > > This backend device does not compute channel attributes and does not expose
> > > > them through sysfs, as done typically in iio-rescale frontend device.
> > > > Instead, it allows to report information applying to channel
> > > > attributes through callbacks. These backend devices can be cascaded
> > > > to represent chained components.
> > > > An IIO device configured as a consumer of a backend device can compute
> > > > the channel attributes of the whole chain.
> > > >
> > > > Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
> > > > ---
> > > > drivers/iio/Makefile | 1 +
> > > > drivers/iio/industrialio-backend.c | 107 +++++++++++++++++++++++++++++
> > > > include/linux/iio/backend.h | 56 +++++++++++++++
> > > > 3 files changed, 164 insertions(+)
> > > > create mode 100644 drivers/iio/industrialio-backend.c
> > > > create mode 100644 include/linux/iio/backend.h
> > > >
> > > > diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
> > > > index 9622347a1c1b..9b59c6ab1738 100644
> > > > --- a/drivers/iio/Makefile
> > > > +++ b/drivers/iio/Makefile
> > > > @@ -5,6 +5,7 @@
> > > >
> > > > obj-$(CONFIG_IIO) += industrialio.o
> > > > industrialio-y := industrialio-core.o industrialio-event.o inkern.o
> > > > +industrialio-$(CONFIG_IIO_BACKEND) += industrialio-backend.o
> > > > industrialio-$(CONFIG_IIO_BUFFER) += industrialio-buffer.o
> > > > industrialio-$(CONFIG_IIO_TRIGGER) += industrialio-trigger.o
> > > >
> > > > diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-
> > > > backend.c
> > > > new file mode 100644
> > > > index 000000000000..7d0625889873
> > > > --- /dev/null
> > > > +++ b/drivers/iio/industrialio-backend.c
> > > > @@ -0,0 +1,107 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/* The industrial I/O core, backend handling functions
> > > > + *
> > > > + */
> > > > +
> > > > +#include <linux/kernel.h>
> > > > +#include <linux/device.h>
> > > > +#include <linux/property.h>
> > > > +#include <linux/iio/iio.h>
> > > > +#include <linux/iio/backend.h>
> > > > +
> > > > +static DEFINE_IDA(iio_backend_ida);
> > > > +
> > > > +#define to_iio_backend(_device) container_of((_device), struct iio_backend,
> > > > dev)
> > > > +
> > > > +static void iio_backend_release(struct device *device)
> > > > +{
> > > > + struct iio_backend *backend = to_iio_backend(device);
> > > > +
> > > > + kfree(backend->name);
> > > > + kfree(backend);
> > > > +}
> > > > +
> > > > +static const struct device_type iio_backend_type = {
> > > > + .release = iio_backend_release,
> > > > + .name = "iio_backend_device",
> > > > +};
> > > > +
> > > > +struct iio_backend *iio_backend_alloc(struct device *parent)
> > > > +{
> > > > + struct iio_backend *backend;
> > > > +
> > > > + backend = devm_kzalloc(parent, sizeof(*backend), GFP_KERNEL);
> > > >
> > >
> > > No error checking.
> > >
> > > I guess a lot of cleanings are still missing but the important thing I wanted to
> > > notice is that the above pattern is not ok.
> > > Your 'struct iio_backend *backend'' embeds a 'stuct device' which is a
> > > refcounted object. Nevertheless, you're binding the lifetime of your object to
> > > the parent device and that is wrong. The reason is that as soon as your parent
> > > device get's released or just unbinded from it's driver, all the devres stuff
> > > (including your 'struct iio_backend' object) will be released independentof
> > > your 'struct device' refcount value...
> > >
> > > So, you might argue this won't ever be an issue in here but the pattern is still
> > > wrong. There are some talks about this, the last one was given at the latest
> > > EOSS:
> > >
> > > https://www.youtube.com/watch?v=HCiJL7djGw8&list=PLbzoR-pLrL6pY8a8zSKRC6-AihFrruOkq&index=27&ab_channel=TheLinuxFoundation
> > >
> >
> > This is a good point. Thanks for pointing it out. Sure, there are still
> > many things to improve.
> >
> > I have seen the comment from Jonathan on your "Add converter framework"
> > serie. I had a quick look at the serie. It seems that we share the need
> > to aggregate some IIO devices. But I need to read it more carefully to
> > check if we can find some convergences here.
>
> Yeah, In my case, the backend devices are typically FPGA soft cores and the aggregate
> device might connect to multiple of these backends. That was one of the reason why I
> used the component API where the aggregate device is only configured when all the
> devices are probed. Similarly, when one of them is unbind, the whole thing should be
> torn down. Also, in my case, the frontend device needs to do a lot of setup on the
> backend device so the whole thing works (so I do have/need a lot more .ops).
>
> Anyways, it does not matter much what the backend device is and from a first glance
> and looking at the .ops you have, it seems that this could easily be supported in the
> framework I'm adding. The only things I'm seeing are:
>
> 1) You would need to use the component API if it's ok. Also not sure if the cascaded
> usecase you mention would work with that API.
>
> 2) We would need to add the .read_raw() op. If you look at my RFC, I already have
> some comments/concerns about having an option like that (see there).
>
> Having said that, none of the above are blockers as 1), I can ditch the component API
> in favour of typical FW/OF lookup (even though the component API makes some things
> easier to handle) and 2), adding a .read_raw() op is not a blocker for me.
>
> Alternatively, another (maybe crazy) idea would be to have this framework have the
> really generic stuff (like lookup + generic ops) and build my iio-converter on top of
> it (extending it). You know, some OO fun :). Maybe not worth the trouble though.
>
> Let's if Jonathan has some suggestions on how to proceed...
The two of you are definitely the most familiar with the code and the restrictions
around it, so ideally I'd like you go figure out the path forwards and tell me :)
To me this is a non core extension of IIO so as long as we end up with something
maintainable that solves some (all?) of the ordering and dependency issues I'll
be happy. I'd rather not have two solutions of course if there is not a good
reason why they have to be different. If we do end up with two solutions I
want clear documentation for the restrictions of each so that we hopefully
don't end up with a 3rd solution down the line.
Jonathan
>
> - Nuno Sá
> > >
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC v2 01/11] iio: introduce iio backend device
2023-09-01 8:01 ` Nuno Sá
2023-09-03 10:46 ` Jonathan Cameron
@ 2023-09-05 10:06 ` Olivier MOYSAN
2023-09-11 9:39 ` Nuno Sá
1 sibling, 1 reply; 40+ messages in thread
From: Olivier MOYSAN @ 2023-09-05 10:06 UTC (permalink / raw)
To: Nuno Sá, Jonathan Cameron, Lars-Peter Clausen
Cc: linux-kernel, linux-iio, Fabrice GASNIER
Hi Nuno,
On 9/1/23 10:01, Nuno Sá wrote:
> Hi Olivier,
>
> On Thu, 2023-08-31 at 18:14 +0200, Olivier MOYSAN wrote:
>> Hi Nuno,
>>
>> On 7/28/23 10:42, Nuno Sá wrote:
>>> Hi Olivier,
>>>
>>> On Thu, 2023-07-27 at 17:03 +0200, Olivier Moysan wrote:
>>>> Add a new device type in IIO framework.
>>>> This backend device does not compute channel attributes and does not expose
>>>> them through sysfs, as done typically in iio-rescale frontend device.
>>>> Instead, it allows to report information applying to channel
>>>> attributes through callbacks. These backend devices can be cascaded
>>>> to represent chained components.
>>>> An IIO device configured as a consumer of a backend device can compute
>>>> the channel attributes of the whole chain.
>>>>
>>>> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
>>>> ---
>>>> drivers/iio/Makefile | 1 +
>>>> drivers/iio/industrialio-backend.c | 107 +++++++++++++++++++++++++++++
>>>> include/linux/iio/backend.h | 56 +++++++++++++++
>>>> 3 files changed, 164 insertions(+)
>>>> create mode 100644 drivers/iio/industrialio-backend.c
>>>> create mode 100644 include/linux/iio/backend.h
>>>>
>>>> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
>>>> index 9622347a1c1b..9b59c6ab1738 100644
>>>> --- a/drivers/iio/Makefile
>>>> +++ b/drivers/iio/Makefile
>>>> @@ -5,6 +5,7 @@
>>>>
>>>> obj-$(CONFIG_IIO) += industrialio.o
>>>> industrialio-y := industrialio-core.o industrialio-event.o inkern.o
>>>> +industrialio-$(CONFIG_IIO_BACKEND) += industrialio-backend.o
>>>> industrialio-$(CONFIG_IIO_BUFFER) += industrialio-buffer.o
>>>> industrialio-$(CONFIG_IIO_TRIGGER) += industrialio-trigger.o
>>>>
>>>> diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-
>>>> backend.c
>>>> new file mode 100644
>>>> index 000000000000..7d0625889873
>>>> --- /dev/null
>>>> +++ b/drivers/iio/industrialio-backend.c
>>>> @@ -0,0 +1,107 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/* The industrial I/O core, backend handling functions
>>>> + *
>>>> + */
>>>> +
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/device.h>
>>>> +#include <linux/property.h>
>>>> +#include <linux/iio/iio.h>
>>>> +#include <linux/iio/backend.h>
>>>> +
>>>> +static DEFINE_IDA(iio_backend_ida);
>>>> +
>>>> +#define to_iio_backend(_device) container_of((_device), struct iio_backend,
>>>> dev)
>>>> +
>>>> +static void iio_backend_release(struct device *device)
>>>> +{
>>>> + struct iio_backend *backend = to_iio_backend(device);
>>>> +
>>>> + kfree(backend->name);
>>>> + kfree(backend);
>>>> +}
>>>> +
>>>> +static const struct device_type iio_backend_type = {
>>>> + .release = iio_backend_release,
>>>> + .name = "iio_backend_device",
>>>> +};
>>>> +
>>>> +struct iio_backend *iio_backend_alloc(struct device *parent)
>>>> +{
>>>> + struct iio_backend *backend;
>>>> +
>>>> + backend = devm_kzalloc(parent, sizeof(*backend), GFP_KERNEL);
>>>>
>>>
>>> No error checking.
>>>
>>> I guess a lot of cleanings are still missing but the important thing I wanted to
>>> notice is that the above pattern is not ok.
>>> Your 'struct iio_backend *backend'' embeds a 'stuct device' which is a
>>> refcounted object. Nevertheless, you're binding the lifetime of your object to
>>> the parent device and that is wrong. The reason is that as soon as your parent
>>> device get's released or just unbinded from it's driver, all the devres stuff
>>> (including your 'struct iio_backend' object) will be released independentof
>>> your 'struct device' refcount value...
>>>
>>> So, you might argue this won't ever be an issue in here but the pattern is still
>>> wrong. There are some talks about this, the last one was given at the latest
>>> EOSS:
>>>
>>> https://www.youtube.com/watch?v=HCiJL7djGw8&list=PLbzoR-pLrL6pY8a8zSKRC6-AihFrruOkq&index=27&ab_channel=TheLinuxFoundation
>>>
>>
>> This is a good point. Thanks for pointing it out. Sure, there are still
>> many things to improve.
>>
>> I have seen the comment from Jonathan on your "Add converter framework"
>> serie. I had a quick look at the serie. It seems that we share the need
>> to aggregate some IIO devices. But I need to read it more carefully to
>> check if we can find some convergences here.
>
> Yeah, In my case, the backend devices are typically FPGA soft cores and the aggregate
> device might connect to multiple of these backends. That was one of the reason why I
> used the component API where the aggregate device is only configured when all the
> devices are probed. Similarly, when one of them is unbind, the whole thing should be
> torn down. Also, in my case, the frontend device needs to do a lot of setup on the
> backend device so the whole thing works (so I do have/need a lot more .ops).
>
> Anyways, it does not matter much what the backend device is and from a first glance
> and looking at the .ops you have, it seems that this could easily be supported in the
> framework I'm adding. The only things I'm seeing are:
Thanks for your feedback. Yes, my feeling is that the API I need for the
dfsdm use case, can be covered by the API you propose. I'm not familiar
with component API however, as I discovered it in your serie. It is not
clear for me how this affects device tree description of the hardware.
So I need to take time to look at existing examples.
I think I need also to try a template implementation of dfsdm use case
based on your API, to figure out how it could work.
>
> 1) You would need to use the component API if it's ok. Also not sure if the cascaded
> usecase you mention would work with that API.
>
The cascaded use case by itself is not a real requirement for dfsdm use
case. The idea here was to think about future possible needs, and to
ensure that the solution is scalable enough. So, it is not a strong
requirement, but we probably need to keep it in mind.
> 2) We would need to add the .read_raw() op. If you look at my RFC, I already have
> some comments/concerns about having an option like that (see there).
>
> Having said that, none of the above are blockers as 1), I can ditch the component API
> in favour of typical FW/OF lookup (even though the component API makes some things
> easier to handle) and 2), adding a .read_raw() op is not a blocker for me.
>
Yes, It would be nice to have read_raw(), as this allows to stick to
existing IIO API for standard IIO attributes. But I guess this should
not be a problem.
> Alternatively, another (maybe crazy) idea would be to have this framework have the
> really generic stuff (like lookup + generic ops) and build my iio-converter on top of
> it (extending it). You know, some OO fun :). Maybe not worth the trouble though.
>
> Let's if Jonathan has some suggestions on how to proceed...
>
> - Nuno Sá
>>>
>
Olivier
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC v2 01/11] iio: introduce iio backend device
2023-09-05 10:06 ` Olivier MOYSAN
@ 2023-09-11 9:39 ` Nuno Sá
2023-09-18 15:52 ` Olivier MOYSAN
0 siblings, 1 reply; 40+ messages in thread
From: Nuno Sá @ 2023-09-11 9:39 UTC (permalink / raw)
To: Olivier MOYSAN, Jonathan Cameron, Lars-Peter Clausen
Cc: linux-kernel, linux-iio, Fabrice GASNIER
On Tue, 2023-09-05 at 12:06 +0200, Olivier MOYSAN wrote:
> Hi Nuno,
>
> On 9/1/23 10:01, Nuno Sá wrote:
> > Hi Olivier,
> >
> > On Thu, 2023-08-31 at 18:14 +0200, Olivier MOYSAN wrote:
> > > Hi Nuno,
> > >
> > > On 7/28/23 10:42, Nuno Sá wrote:
> > > > Hi Olivier,
> > > >
> > > > On Thu, 2023-07-27 at 17:03 +0200, Olivier Moysan wrote:
> > > > > Add a new device type in IIO framework.
> > > > > This backend device does not compute channel attributes and does not
> > > > > expose
> > > > > them through sysfs, as done typically in iio-rescale frontend device.
> > > > > Instead, it allows to report information applying to channel
> > > > > attributes through callbacks. These backend devices can be cascaded
> > > > > to represent chained components.
> > > > > An IIO device configured as a consumer of a backend device can compute
> > > > > the channel attributes of the whole chain.
> > > > >
> > > > > Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
> > > > > ---
> > > > > drivers/iio/Makefile | 1 +
> > > > > drivers/iio/industrialio-backend.c | 107
> > > > > +++++++++++++++++++++++++++++
> > > > > include/linux/iio/backend.h | 56 +++++++++++++++
> > > > > 3 files changed, 164 insertions(+)
> > > > > create mode 100644 drivers/iio/industrialio-backend.c
> > > > > create mode 100644 include/linux/iio/backend.h
> > > > >
> > > > > diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
> > > > > index 9622347a1c1b..9b59c6ab1738 100644
> > > > > --- a/drivers/iio/Makefile
> > > > > +++ b/drivers/iio/Makefile
> > > > > @@ -5,6 +5,7 @@
> > > > >
> > > > > obj-$(CONFIG_IIO) += industrialio.o
> > > > > industrialio-y := industrialio-core.o industrialio-event.o inkern.o
> > > > > +industrialio-$(CONFIG_IIO_BACKEND) += industrialio-backend.o
> > > > > industrialio-$(CONFIG_IIO_BUFFER) += industrialio-buffer.o
> > > > > industrialio-$(CONFIG_IIO_TRIGGER) += industrialio-trigger.o
> > > > >
> > > > > diff --git a/drivers/iio/industrialio-backend.c
> > > > > b/drivers/iio/industrialio-
> > > > > backend.c
> > > > > new file mode 100644
> > > > > index 000000000000..7d0625889873
> > > > > --- /dev/null
> > > > > +++ b/drivers/iio/industrialio-backend.c
> > > > > @@ -0,0 +1,107 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +/* The industrial I/O core, backend handling functions
> > > > > + *
> > > > > + */
> > > > > +
> > > > > +#include <linux/kernel.h>
> > > > > +#include <linux/device.h>
> > > > > +#include <linux/property.h>
> > > > > +#include <linux/iio/iio.h>
> > > > > +#include <linux/iio/backend.h>
> > > > > +
> > > > > +static DEFINE_IDA(iio_backend_ida);
> > > > > +
> > > > > +#define to_iio_backend(_device) container_of((_device), struct
> > > > > iio_backend,
> > > > > dev)
> > > > > +
> > > > > +static void iio_backend_release(struct device *device)
> > > > > +{
> > > > > + struct iio_backend *backend = to_iio_backend(device);
> > > > > +
> > > > > + kfree(backend->name);
> > > > > + kfree(backend);
> > > > > +}
> > > > > +
> > > > > +static const struct device_type iio_backend_type = {
> > > > > + .release = iio_backend_release,
> > > > > + .name = "iio_backend_device",
> > > > > +};
> > > > > +
> > > > > +struct iio_backend *iio_backend_alloc(struct device *parent)
> > > > > +{
> > > > > + struct iio_backend *backend;
> > > > > +
> > > > > + backend = devm_kzalloc(parent, sizeof(*backend), GFP_KERNEL);
> > > > >
> > > >
> > > > No error checking.
> > > >
> > > > I guess a lot of cleanings are still missing but the important thing I
> > > > wanted to
> > > > notice is that the above pattern is not ok.
> > > > Your 'struct iio_backend *backend'' embeds a 'stuct device' which is a
> > > > refcounted object. Nevertheless, you're binding the lifetime of your
> > > > object to
> > > > the parent device and that is wrong. The reason is that as soon as your
> > > > parent
> > > > device get's released or just unbinded from it's driver, all the devres
> > > > stuff
> > > > (including your 'struct iio_backend' object) will be released
> > > > independentof
> > > > your 'struct device' refcount value...
> > > >
> > > > So, you might argue this won't ever be an issue in here but the pattern
> > > > is still
> > > > wrong. There are some talks about this, the last one was given at the
> > > > latest
> > > > EOSS:
> > > >
> > > > https://www.youtube.com/watch?v=HCiJL7djGw8&list=PLbzoR-pLrL6pY8a8zSKRC6-AihFrruOkq&index=27&ab_channel=TheLinuxFoundation
> > > >
> > >
> > > This is a good point. Thanks for pointing it out. Sure, there are still
> > > many things to improve.
> > >
> > > I have seen the comment from Jonathan on your "Add converter framework"
> > > serie. I had a quick look at the serie. It seems that we share the need
> > > to aggregate some IIO devices. But I need to read it more carefully to
> > > check if we can find some convergences here.
> >
> > Yeah, In my case, the backend devices are typically FPGA soft cores and the
> > aggregate
> > device might connect to multiple of these backends. That was one of the
> > reason why I
> > used the component API where the aggregate device is only configured when
> > all the
> > devices are probed. Similarly, when one of them is unbind, the whole thing
> > should be
> > torn down. Also, in my case, the frontend device needs to do a lot of setup
> > on the
> > backend device so the whole thing works (so I do have/need a lot more .ops).
> >
> > Anyways, it does not matter much what the backend device is and from a first
> > glance
> > and looking at the .ops you have, it seems that this could easily be
> > supported in the
> > framework I'm adding. The only things I'm seeing are:
>
> Thanks for your feedback. Yes, my feeling is that the API I need for the
> dfsdm use case, can be covered by the API you propose. I'm not familiar
> with component API however, as I discovered it in your serie. It is not
> clear for me how this affects device tree description of the hardware.
Your aggregate device (that we can think of as a frontend device needs to
properly reference all the backends it needs - in your case I guess it's just
one device). The dts properties I have for now are 'converters' and 'converter-
names'. But one thing that starts to become clear to me is that I should
probably change the name for the framework. Maybe industrialio-aggregate.c if we
keep the component API (and so the same frontend + backend naming) or just
industrialio-backend.c (as you have now) if we go with a typical OF lookup.
> So I need to take time to look at existing examples.
> I think I need also to try a template implementation of dfsdm use case
> based on your API, to figure out how it could work.
>
Please do so :).
> >
> > 1) You would need to use the component API if it's ok. Also not sure if the
> > cascaded
> > usecase you mention would work with that API.
> >
>
> The cascaded use case by itself is not a real requirement for dfsdm use
> case. The idea here was to think about future possible needs, and to
> ensure that the solution is scalable enough. So, it is not a strong
> requirement, but we probably need to keep it in mind.
>
Sure. I think one backend might be used as frontend in another aggregate device,
using the component API, but I'm 100% sure. So, yeah, something to keep in mind
and test with some dummy setup.
> > 2) We would need to add the .read_raw() op. If you look at my RFC, I already
> > have
> > some comments/concerns about having an option like that (see there).
> >
> > Having said that, none of the above are blockers as 1), I can ditch the
> > component API
> > in favour of typical FW/OF lookup (even though the component API makes some
> > things
> > easier to handle) and 2), adding a .read_raw() op is not a blocker for me.
> >
>
> Yes, It would be nice to have read_raw(), as this allows to stick to
> existing IIO API for standard IIO attributes. But I guess this should
> not be a problem.
My idea is to still make use of standard IIO attrs but with a more fine grained
approach on the callback. Here is what I reasoned about in the other thread:
"There are some IIO attributes (like scale, frequency, etc) that might
be implemented in the soft cores. I still didn't made my mind if I should just
have a catch all read_raw() and write_raw() converter_ops or more fine
tuned ops. Having the catch all reduces the number of ops but also makes
it more easier to add stuff that ends up being not used anymore and then
forgotten. There are also cases (eg: setting sampling frequency) where
we might need to apply settings in both the frontend and the backend
devices which means having the catch all write_raw() would be more
awkward in these case. I'm a bit more inclined to the more specific ops."
- Nuno Sá
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC v2 01/11] iio: introduce iio backend device
2023-09-11 9:39 ` Nuno Sá
@ 2023-09-18 15:52 ` Olivier MOYSAN
2023-09-22 8:53 ` Nuno Sá
0 siblings, 1 reply; 40+ messages in thread
From: Olivier MOYSAN @ 2023-09-18 15:52 UTC (permalink / raw)
To: Nuno Sá, Jonathan Cameron, Lars-Peter Clausen
Cc: linux-kernel, linux-iio, Fabrice GASNIER
Hi Nuno
On 9/11/23 11:39, Nuno Sá wrote:
> On Tue, 2023-09-05 at 12:06 +0200, Olivier MOYSAN wrote:
>> Hi Nuno,
>>
>> On 9/1/23 10:01, Nuno Sá wrote:
>>> Hi Olivier,
>>>
>>> On Thu, 2023-08-31 at 18:14 +0200, Olivier MOYSAN wrote:
>>>> Hi Nuno,
>>>>
>>>> On 7/28/23 10:42, Nuno Sá wrote:
>>>>> Hi Olivier,
>>>>>
>>>>> On Thu, 2023-07-27 at 17:03 +0200, Olivier Moysan wrote:
>>>>>> Add a new device type in IIO framework.
>>>>>> This backend device does not compute channel attributes and does not
>>>>>> expose
>>>>>> them through sysfs, as done typically in iio-rescale frontend device.
>>>>>> Instead, it allows to report information applying to channel
>>>>>> attributes through callbacks. These backend devices can be cascaded
>>>>>> to represent chained components.
>>>>>> An IIO device configured as a consumer of a backend device can compute
>>>>>> the channel attributes of the whole chain.
>>>>>>
>>>>>> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
>>>>>> ---
>>>>>> drivers/iio/Makefile | 1 +
>>>>>> drivers/iio/industrialio-backend.c | 107
>>>>>> +++++++++++++++++++++++++++++
>>>>>> include/linux/iio/backend.h | 56 +++++++++++++++
>>>>>> 3 files changed, 164 insertions(+)
>>>>>> create mode 100644 drivers/iio/industrialio-backend.c
>>>>>> create mode 100644 include/linux/iio/backend.h
>>>>>>
>>>>>> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
>>>>>> index 9622347a1c1b..9b59c6ab1738 100644
>>>>>> --- a/drivers/iio/Makefile
>>>>>> +++ b/drivers/iio/Makefile
>>>>>> @@ -5,6 +5,7 @@
>>>>>>
>>>>>> obj-$(CONFIG_IIO) += industrialio.o
>>>>>> industrialio-y := industrialio-core.o industrialio-event.o inkern.o
>>>>>> +industrialio-$(CONFIG_IIO_BACKEND) += industrialio-backend.o
>>>>>> industrialio-$(CONFIG_IIO_BUFFER) += industrialio-buffer.o
>>>>>> industrialio-$(CONFIG_IIO_TRIGGER) += industrialio-trigger.o
>>>>>>
>>>>>> diff --git a/drivers/iio/industrialio-backend.c
>>>>>> b/drivers/iio/industrialio-
>>>>>> backend.c
>>>>>> new file mode 100644
>>>>>> index 000000000000..7d0625889873
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/iio/industrialio-backend.c
>>>>>> @@ -0,0 +1,107 @@
>>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>>> +/* The industrial I/O core, backend handling functions
>>>>>> + *
>>>>>> + */
>>>>>> +
>>>>>> +#include <linux/kernel.h>
>>>>>> +#include <linux/device.h>
>>>>>> +#include <linux/property.h>
>>>>>> +#include <linux/iio/iio.h>
>>>>>> +#include <linux/iio/backend.h>
>>>>>> +
>>>>>> +static DEFINE_IDA(iio_backend_ida);
>>>>>> +
>>>>>> +#define to_iio_backend(_device) container_of((_device), struct
>>>>>> iio_backend,
>>>>>> dev)
>>>>>> +
>>>>>> +static void iio_backend_release(struct device *device)
>>>>>> +{
>>>>>> + struct iio_backend *backend = to_iio_backend(device);
>>>>>> +
>>>>>> + kfree(backend->name);
>>>>>> + kfree(backend);
>>>>>> +}
>>>>>> +
>>>>>> +static const struct device_type iio_backend_type = {
>>>>>> + .release = iio_backend_release,
>>>>>> + .name = "iio_backend_device",
>>>>>> +};
>>>>>> +
>>>>>> +struct iio_backend *iio_backend_alloc(struct device *parent)
>>>>>> +{
>>>>>> + struct iio_backend *backend;
>>>>>> +
>>>>>> + backend = devm_kzalloc(parent, sizeof(*backend), GFP_KERNEL);
>>>>>>
>>>>>
>>>>> No error checking.
>>>>>
>>>>> I guess a lot of cleanings are still missing but the important thing I
>>>>> wanted to
>>>>> notice is that the above pattern is not ok.
>>>>> Your 'struct iio_backend *backend'' embeds a 'stuct device' which is a
>>>>> refcounted object. Nevertheless, you're binding the lifetime of your
>>>>> object to
>>>>> the parent device and that is wrong. The reason is that as soon as your
>>>>> parent
>>>>> device get's released or just unbinded from it's driver, all the devres
>>>>> stuff
>>>>> (including your 'struct iio_backend' object) will be released
>>>>> independentof
>>>>> your 'struct device' refcount value...
>>>>>
>>>>> So, you might argue this won't ever be an issue in here but the pattern
>>>>> is still
>>>>> wrong. There are some talks about this, the last one was given at the
>>>>> latest
>>>>> EOSS:
>>>>>
>>>>> https://www.youtube.com/watch?v=HCiJL7djGw8&list=PLbzoR-pLrL6pY8a8zSKRC6-AihFrruOkq&index=27&ab_channel=TheLinuxFoundation
>>>>>
>>>>
>>>> This is a good point. Thanks for pointing it out. Sure, there are still
>>>> many things to improve.
>>>>
>>>> I have seen the comment from Jonathan on your "Add converter framework"
>>>> serie. I had a quick look at the serie. It seems that we share the need
>>>> to aggregate some IIO devices. But I need to read it more carefully to
>>>> check if we can find some convergences here.
>>>
>>> Yeah, In my case, the backend devices are typically FPGA soft cores and the
>>> aggregate
>>> device might connect to multiple of these backends. That was one of the
>>> reason why I
>>> used the component API where the aggregate device is only configured when
>>> all the
>>> devices are probed. Similarly, when one of them is unbind, the whole thing
>>> should be
>>> torn down. Also, in my case, the frontend device needs to do a lot of setup
>>> on the
>>> backend device so the whole thing works (so I do have/need a lot more .ops).
>>>
>>> Anyways, it does not matter much what the backend device is and from a first
>>> glance
>>> and looking at the .ops you have, it seems that this could easily be
>>> supported in the
>>> framework I'm adding. The only things I'm seeing are:
>>
>> Thanks for your feedback. Yes, my feeling is that the API I need for the
>> dfsdm use case, can be covered by the API you propose. I'm not familiar
>> with component API however, as I discovered it in your serie. It is not
>> clear for me how this affects device tree description of the hardware.
>
> Your aggregate device (that we can think of as a frontend device needs to
> properly reference all the backends it needs - in your case I guess it's just
> one device). The dts properties I have for now are 'converters' and 'converter-
> names'. But one thing that starts to become clear to me is that I should
> probably change the name for the framework. Maybe industrialio-aggregate.c if we
> keep the component API (and so the same frontend + backend naming) or just
> industrialio-backend.c (as you have now) if we go with a typical OF lookup.
>
In my case I have a digital filter peripheral (frontend) linked to
several sigma delta converters (backends). So, here 'converters'
property may be relevant as well. But I agree that a more generic name
seems better for the long term.
My backend devices need to get a regulator phandle from the device tree.
It seems that the component API does not offer services allowing to
retrieve DT properties for the sub-devices. Tell me if I'm wrong, but I
think this constraint require to change converter framework to a typical
OF lookup.
Could you please share the structure of your DT for your ad9476 based
example ? This will help me identify the gaps regarding my need.
>> So I need to take time to look at existing examples.
>> I think I need also to try a template implementation of dfsdm use case
>> based on your API, to figure out how it could work.
>>
>
> Please do so :).
>
Here, we need to clarify some points related to DT first, I think.
I assume that API itself should not be too much a concern.
>>>
>>> 1) You would need to use the component API if it's ok. Also not sure if the
>>> cascaded
>>> usecase you mention would work with that API.
>>>
>>
>> The cascaded use case by itself is not a real requirement for dfsdm use
>> case. The idea here was to think about future possible needs, and to
>> ensure that the solution is scalable enough. So, it is not a strong
>> requirement, but we probably need to keep it in mind.
>>
>
> Sure. I think one backend might be used as frontend in another aggregate device,
> using the component API, but I'm 100% sure. So, yeah, something to keep in mind
> and test with some dummy setup.
>
>>> 2) We would need to add the .read_raw() op. If you look at my RFC, I already
>>> have
>>> some comments/concerns about having an option like that (see there).
>>>
>>> Having said that, none of the above are blockers as 1), I can ditch the
>>> component API
>>> in favour of typical FW/OF lookup (even though the component API makes some
>>> things
>>> easier to handle) and 2), adding a .read_raw() op is not a blocker for me.
>>>
>>
>> Yes, It would be nice to have read_raw(), as this allows to stick to
>> existing IIO API for standard IIO attributes. But I guess this should
>> not be a problem.
>
> My idea is to still make use of standard IIO attrs but with a more fine grained
> approach on the callback. Here is what I reasoned about in the other thread:
>
> "There are some IIO attributes (like scale, frequency, etc) that might
> be implemented in the soft cores. I still didn't made my mind if I should just
> have a catch all read_raw() and write_raw() converter_ops or more fine
> tuned ops. Having the catch all reduces the number of ops but also makes
> it more easier to add stuff that ends up being not used anymore and then
> forgotten. There are also cases (eg: setting sampling frequency) where
> we might need to apply settings in both the frontend and the backend
> devices which means having the catch all write_raw() would be more
> awkward in these case. I'm a bit more inclined to the more specific ops."
> > - Nuno Sá
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC v2 01/11] iio: introduce iio backend device
2023-09-18 15:52 ` Olivier MOYSAN
@ 2023-09-22 8:53 ` Nuno Sá
2023-09-25 6:48 ` Nuno Sá
0 siblings, 1 reply; 40+ messages in thread
From: Nuno Sá @ 2023-09-22 8:53 UTC (permalink / raw)
To: Olivier MOYSAN, Jonathan Cameron, Lars-Peter Clausen
Cc: linux-kernel, linux-iio, Fabrice GASNIER
Hi Olivier,
Sorry for the delay...
On Mon, 2023-09-18 at 17:52 +0200, Olivier MOYSAN wrote:
> Hi Nuno
>
> On 9/11/23 11:39, Nuno Sá wrote:
> > On Tue, 2023-09-05 at 12:06 +0200, Olivier MOYSAN wrote:
> > > Hi Nuno,
> > >
> > > On 9/1/23 10:01, Nuno Sá wrote:
> > > > Hi Olivier,
> > > >
> > > > On Thu, 2023-08-31 at 18:14 +0200, Olivier MOYSAN wrote:
> > > > > Hi Nuno,
> > > > >
> > > > > On 7/28/23 10:42, Nuno Sá wrote:
> > > > > > Hi Olivier,
> > > > > >
> > > > > > On Thu, 2023-07-27 at 17:03 +0200, Olivier Moysan wrote:
> > > > > > > Add a new device type in IIO framework.
> > > > > > > This backend device does not compute channel attributes and does
> > > > > > > not
> > > > > > > expose
> > > > > > > them through sysfs, as done typically in iio-rescale frontend
> > > > > > > device.
> > > > > > > Instead, it allows to report information applying to channel
> > > > > > > attributes through callbacks. These backend devices can be
> > > > > > > cascaded
> > > > > > > to represent chained components.
> > > > > > > An IIO device configured as a consumer of a backend device can
> > > > > > > compute
> > > > > > > the channel attributes of the whole chain.
> > > > > > >
> > > > > > > Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
> > > > > > > ---
> > > > > > > drivers/iio/Makefile | 1 +
> > > > > > > drivers/iio/industrialio-backend.c | 107
> > > > > > > +++++++++++++++++++++++++++++
> > > > > > > include/linux/iio/backend.h | 56 +++++++++++++++
> > > > > > > 3 files changed, 164 insertions(+)
> > > > > > > create mode 100644 drivers/iio/industrialio-backend.c
> > > > > > > create mode 100644 include/linux/iio/backend.h
> > > > > > >
> > > > > > > diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
> > > > > > > index 9622347a1c1b..9b59c6ab1738 100644
> > > > > > > --- a/drivers/iio/Makefile
> > > > > > > +++ b/drivers/iio/Makefile
> > > > > > > @@ -5,6 +5,7 @@
> > > > > > >
> > > > > > > obj-$(CONFIG_IIO) += industrialio.o
> > > > > > > industrialio-y := industrialio-core.o industrialio-event.o
> > > > > > > inkern.o
> > > > > > > +industrialio-$(CONFIG_IIO_BACKEND) += industrialio-backend.o
> > > > > > > industrialio-$(CONFIG_IIO_BUFFER) += industrialio-buffer.o
> > > > > > > industrialio-$(CONFIG_IIO_TRIGGER) += industrialio-trigger.o
> > > > > > >
> > > > > > > diff --git a/drivers/iio/industrialio-backend.c
> > > > > > > b/drivers/iio/industrialio-
> > > > > > > backend.c
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..7d0625889873
> > > > > > > --- /dev/null
> > > > > > > +++ b/drivers/iio/industrialio-backend.c
> > > > > > > @@ -0,0 +1,107 @@
> > > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > > +/* The industrial I/O core, backend handling functions
> > > > > > > + *
> > > > > > > + */
> > > > > > > +
> > > > > > > +#include <linux/kernel.h>
> > > > > > > +#include <linux/device.h>
> > > > > > > +#include <linux/property.h>
> > > > > > > +#include <linux/iio/iio.h>
> > > > > > > +#include <linux/iio/backend.h>
> > > > > > > +
> > > > > > > +static DEFINE_IDA(iio_backend_ida);
> > > > > > > +
> > > > > > > +#define to_iio_backend(_device) container_of((_device), struct
> > > > > > > iio_backend,
> > > > > > > dev)
> > > > > > > +
> > > > > > > +static void iio_backend_release(struct device *device)
> > > > > > > +{
> > > > > > > + struct iio_backend *backend = to_iio_backend(device);
> > > > > > > +
> > > > > > > + kfree(backend->name);
> > > > > > > + kfree(backend);
> > > > > > > +}
> > > > > > > +
> > > > > > > +static const struct device_type iio_backend_type = {
> > > > > > > + .release = iio_backend_release,
> > > > > > > + .name = "iio_backend_device",
> > > > > > > +};
> > > > > > > +
> > > > > > > +struct iio_backend *iio_backend_alloc(struct device *parent)
> > > > > > > +{
> > > > > > > + struct iio_backend *backend;
> > > > > > > +
> > > > > > > + backend = devm_kzalloc(parent, sizeof(*backend),
> > > > > > > GFP_KERNEL);
> > > > > > >
> > > > > >
> > > > > > No error checking.
> > > > > >
> > > > > > I guess a lot of cleanings are still missing but the important thing
> > > > > > I
> > > > > > wanted to
> > > > > > notice is that the above pattern is not ok.
> > > > > > Your 'struct iio_backend *backend'' embeds a 'stuct device' which is
> > > > > > a
> > > > > > refcounted object. Nevertheless, you're binding the lifetime of your
> > > > > > object to
> > > > > > the parent device and that is wrong. The reason is that as soon as
> > > > > > your
> > > > > > parent
> > > > > > device get's released or just unbinded from it's driver, all the
> > > > > > devres
> > > > > > stuff
> > > > > > (including your 'struct iio_backend' object) will be released
> > > > > > independentof
> > > > > > your 'struct device' refcount value...
> > > > > >
> > > > > > So, you might argue this won't ever be an issue in here but the
> > > > > > pattern
> > > > > > is still
> > > > > > wrong. There are some talks about this, the last one was given at
> > > > > > the
> > > > > > latest
> > > > > > EOSS:
> > > > > >
> > > > > > https://www.youtube.com/watch?v=HCiJL7djGw8&list=PLbzoR-pLrL6pY8a8zSKRC6-AihFrruOkq&index=27&ab_channel=TheLinuxFoundation
> > > > > >
> > > > >
> > > > > This is a good point. Thanks for pointing it out. Sure, there are
> > > > > still
> > > > > many things to improve.
> > > > >
> > > > > I have seen the comment from Jonathan on your "Add converter
> > > > > framework"
> > > > > serie. I had a quick look at the serie. It seems that we share the
> > > > > need
> > > > > to aggregate some IIO devices. But I need to read it more carefully to
> > > > > check if we can find some convergences here.
> > > >
> > > > Yeah, In my case, the backend devices are typically FPGA soft cores and
> > > > the
> > > > aggregate
> > > > device might connect to multiple of these backends. That was one of the
> > > > reason why I
> > > > used the component API where the aggregate device is only configured
> > > > when
> > > > all the
> > > > devices are probed. Similarly, when one of them is unbind, the whole
> > > > thing
> > > > should be
> > > > torn down. Also, in my case, the frontend device needs to do a lot of
> > > > setup
> > > > on the
> > > > backend device so the whole thing works (so I do have/need a lot more
> > > > .ops).
> > > >
> > > > Anyways, it does not matter much what the backend device is and from a
> > > > first
> > > > glance
> > > > and looking at the .ops you have, it seems that this could easily be
> > > > supported in the
> > > > framework I'm adding. The only things I'm seeing are:
> > >
> > > Thanks for your feedback. Yes, my feeling is that the API I need for the
> > > dfsdm use case, can be covered by the API you propose. I'm not familiar
> > > with component API however, as I discovered it in your serie. It is not
> > > clear for me how this affects device tree description of the hardware.
> >
> > Your aggregate device (that we can think of as a frontend device needs to
> > properly reference all the backends it needs - in your case I guess it's
> > just
> > one device). The dts properties I have for now are 'converters' and
> > 'converter-
> > names'. But one thing that starts to become clear to me is that I should
> > probably change the name for the framework. Maybe industrialio-aggregate.c
> > if we
> > keep the component API (and so the same frontend + backend naming) or just
> > industrialio-backend.c (as you have now) if we go with a typical OF lookup.
> >
>
> In my case I have a digital filter peripheral (frontend) linked to
> several sigma delta converters (backends). So, here 'converters'
> property may be relevant as well. But I agree that a more generic name
> seems better for the long term.
>
> My backend devices need to get a regulator phandle from the device tree.
> It seems that the component API does not offer services allowing to
> retrieve DT properties for the sub-devices. Tell me if I'm wrong, but I
> think this constraint require to change converter framework to a typical
> OF lookup.
>
> Could you please share the structure of your DT for your ad9476 based
> example ? This will help me identify the gaps regarding my need.
>
I might be missing something but there should be no limitation in the component
stuff for this. Note your frontend/backend devices are just normal device tree
nodes (meaning that they can have all the properties they want as a normal node)
and then in the correspondent drivers you handle all the properties. For now,
the only FW properties supported in the framework I sent are 'converters' and
'converter-name' which will be used to "create" the aggregate device. This
pretty much means that the complete thing should only come up when all the
devices you set in DT probe.
Of course we can move more properties into the framework if we start to see some
generic ones that are almost always present...
One thing that Jonathan already mentioned is that the component API works in a
away that you can have either 1->1 or 1->N (frontends->backends). So, if you
have setups where you have more than one frontend (basically M->N) we need to
make sure it still works. In theory (in the component API), I think you can have
one backend associated with more than one frontend so we should be able to still
get the M->N topology. Of course the "communications link" is always between
frontend -> backend.
I'll see if I send the devicetree over the weekend (don't have it in my current
machine)
- Nuno Sá
> >
>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC v2 01/11] iio: introduce iio backend device
2023-09-22 8:53 ` Nuno Sá
@ 2023-09-25 6:48 ` Nuno Sá
2023-09-26 16:44 ` Olivier MOYSAN
0 siblings, 1 reply; 40+ messages in thread
From: Nuno Sá @ 2023-09-25 6:48 UTC (permalink / raw)
To: Olivier MOYSAN, Jonathan Cameron, Lars-Peter Clausen
Cc: linux-kernel, linux-iio, Fabrice GASNIER
Hi Olivier,
On Fri, 2023-09-22 at 10:53 +0200, Nuno Sá wrote:
> Hi Olivier,
>
> Sorry for the delay...
>
> On Mon, 2023-09-18 at 17:52 +0200, Olivier MOYSAN wrote:
> > Hi Nuno
> >
> > On 9/11/23 11:39, Nuno Sá wrote:
> > > On Tue, 2023-09-05 at 12:06 +0200, Olivier MOYSAN wrote:
> > > > Hi Nuno,
> > > >
> > > > On 9/1/23 10:01, Nuno Sá wrote:
> > > > > Hi Olivier,
> > > > >
> > > > > On Thu, 2023-08-31 at 18:14 +0200, Olivier MOYSAN wrote:
> > > > > > Hi Nuno,
> > > > > >
> > > > > > On 7/28/23 10:42, Nuno Sá wrote:
> > > > > > > Hi Olivier,
> > > > > > >
> > > > > > > On Thu, 2023-07-27 at 17:03 +0200, Olivier Moysan wrote:
> > > > > > > > Add a new device type in IIO framework.
> > > > > > > > This backend device does not compute channel attributes and does
> > > > > > > > not
> > > > > > > > expose
> > > > > > > > them through sysfs, as done typically in iio-rescale frontend
> > > > > > > > device.
> > > > > > > > Instead, it allows to report information applying to channel
> > > > > > > > attributes through callbacks. These backend devices can be
> > > > > > > > cascaded
> > > > > > > > to represent chained components.
> > > > > > > > An IIO device configured as a consumer of a backend device can
> > > > > > > > compute
> > > > > > > > the channel attributes of the whole chain.
> > > > > > > >
> > > > > > > > Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
> > > > > > > > ---
> > > > > > > > drivers/iio/Makefile | 1 +
> > > > > > > > drivers/iio/industrialio-backend.c | 107
> > > > > > > > +++++++++++++++++++++++++++++
> > > > > > > > include/linux/iio/backend.h | 56 +++++++++++++++
> > > > > > > > 3 files changed, 164 insertions(+)
> > > > > > > > create mode 100644 drivers/iio/industrialio-backend.c
> > > > > > > > create mode 100644 include/linux/iio/backend.h
> > > > > > > >
> > > > > > > > diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
> > > > > > > > index 9622347a1c1b..9b59c6ab1738 100644
> > > > > > > > --- a/drivers/iio/Makefile
> > > > > > > > +++ b/drivers/iio/Makefile
> > > > > > > > @@ -5,6 +5,7 @@
> > > > > > > >
> > > > > > > > obj-$(CONFIG_IIO) += industrialio.o
> > > > > > > > industrialio-y := industrialio-core.o industrialio-event.o
> > > > > > > > inkern.o
> > > > > > > > +industrialio-$(CONFIG_IIO_BACKEND) += industrialio-backend.o
> > > > > > > > industrialio-$(CONFIG_IIO_BUFFER) += industrialio-buffer.o
> > > > > > > > industrialio-$(CONFIG_IIO_TRIGGER) += industrialio-trigger.o
> > > > > > > >
> > > > > > > > diff --git a/drivers/iio/industrialio-backend.c
> > > > > > > > b/drivers/iio/industrialio-
> > > > > > > > backend.c
> > > > > > > > new file mode 100644
> > > > > > > > index 000000000000..7d0625889873
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/drivers/iio/industrialio-backend.c
> > > > > > > > @@ -0,0 +1,107 @@
> > > > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > > > +/* The industrial I/O core, backend handling functions
> > > > > > > > + *
> > > > > > > > + */
> > > > > > > > +
> > > > > > > > +#include <linux/kernel.h>
> > > > > > > > +#include <linux/device.h>
> > > > > > > > +#include <linux/property.h>
> > > > > > > > +#include <linux/iio/iio.h>
> > > > > > > > +#include <linux/iio/backend.h>
> > > > > > > > +
> > > > > > > > +static DEFINE_IDA(iio_backend_ida);
> > > > > > > > +
> > > > > > > > +#define to_iio_backend(_device) container_of((_device), struct
> > > > > > > > iio_backend,
> > > > > > > > dev)
> > > > > > > > +
> > > > > > > > +static void iio_backend_release(struct device *device)
> > > > > > > > +{
> > > > > > > > + struct iio_backend *backend = to_iio_backend(device);
> > > > > > > > +
> > > > > > > > + kfree(backend->name);
> > > > > > > > + kfree(backend);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static const struct device_type iio_backend_type = {
> > > > > > > > + .release = iio_backend_release,
> > > > > > > > + .name = "iio_backend_device",
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > +struct iio_backend *iio_backend_alloc(struct device *parent)
> > > > > > > > +{
> > > > > > > > + struct iio_backend *backend;
> > > > > > > > +
> > > > > > > > + backend = devm_kzalloc(parent, sizeof(*backend),
> > > > > > > > GFP_KERNEL);
> > > > > > > >
> > > > > > >
> > > > > > > No error checking.
> > > > > > >
> > > > > > > I guess a lot of cleanings are still missing but the important thing
> > > > > > > I
> > > > > > > wanted to
> > > > > > > notice is that the above pattern is not ok.
> > > > > > > Your 'struct iio_backend *backend'' embeds a 'stuct device' which is
> > > > > > > a
> > > > > > > refcounted object. Nevertheless, you're binding the lifetime of your
> > > > > > > object to
> > > > > > > the parent device and that is wrong. The reason is that as soon as
> > > > > > > your
> > > > > > > parent
> > > > > > > device get's released or just unbinded from it's driver, all the
> > > > > > > devres
> > > > > > > stuff
> > > > > > > (including your 'struct iio_backend' object) will be released
> > > > > > > independentof
> > > > > > > your 'struct device' refcount value...
> > > > > > >
> > > > > > > So, you might argue this won't ever be an issue in here but the
> > > > > > > pattern
> > > > > > > is still
> > > > > > > wrong. There are some talks about this, the last one was given at
> > > > > > > the
> > > > > > > latest
> > > > > > > EOSS:
> > > > > > >
> > > > > > > https://www.youtube.com/watch?v=HCiJL7djGw8&list=PLbzoR-pLrL6pY8a8zSKRC6-AihFrruOkq&index=27&ab_channel=TheLinuxFoundation
> > > > > > >
> > > > > >
> > > > > > This is a good point. Thanks for pointing it out. Sure, there are
> > > > > > still
> > > > > > many things to improve.
> > > > > >
> > > > > > I have seen the comment from Jonathan on your "Add converter
> > > > > > framework"
> > > > > > serie. I had a quick look at the serie. It seems that we share the
> > > > > > need
> > > > > > to aggregate some IIO devices. But I need to read it more carefully to
> > > > > > check if we can find some convergences here.
> > > > >
> > > > > Yeah, In my case, the backend devices are typically FPGA soft cores and
> > > > > the
> > > > > aggregate
> > > > > device might connect to multiple of these backends. That was one of the
> > > > > reason why I
> > > > > used the component API where the aggregate device is only configured
> > > > > when
> > > > > all the
> > > > > devices are probed. Similarly, when one of them is unbind, the whole
> > > > > thing
> > > > > should be
> > > > > torn down. Also, in my case, the frontend device needs to do a lot of
> > > > > setup
> > > > > on the
> > > > > backend device so the whole thing works (so I do have/need a lot more
> > > > > .ops).
> > > > >
> > > > > Anyways, it does not matter much what the backend device is and from a
> > > > > first
> > > > > glance
> > > > > and looking at the .ops you have, it seems that this could easily be
> > > > > supported in the
> > > > > framework I'm adding. The only things I'm seeing are:
> > > >
> > > > Thanks for your feedback. Yes, my feeling is that the API I need for the
> > > > dfsdm use case, can be covered by the API you propose. I'm not familiar
> > > > with component API however, as I discovered it in your serie. It is not
> > > > clear for me how this affects device tree description of the hardware.
> > >
> > > Your aggregate device (that we can think of as a frontend device needs to
> > > properly reference all the backends it needs - in your case I guess it's
> > > just
> > > one device). The dts properties I have for now are 'converters' and
> > > 'converter-
> > > names'. But one thing that starts to become clear to me is that I should
> > > probably change the name for the framework. Maybe industrialio-aggregate.c
> > > if we
> > > keep the component API (and so the same frontend + backend naming) or just
> > > industrialio-backend.c (as you have now) if we go with a typical OF lookup.
> > >
> >
> > In my case I have a digital filter peripheral (frontend) linked to
> > several sigma delta converters (backends). So, here 'converters'
> > property may be relevant as well. But I agree that a more generic name
> > seems better for the long term.
> >
> > My backend devices need to get a regulator phandle from the device tree.
> > It seems that the component API does not offer services allowing to
> > retrieve DT properties for the sub-devices. Tell me if I'm wrong, but I
> > think this constraint require to change converter framework to a typical
> > OF lookup.
> >
> > Could you please share the structure of your DT for your ad9476 based
> > example ? This will help me identify the gaps regarding my need.
> >
>
> I might be missing something but there should be no limitation in the component
> stuff for this. Note your frontend/backend devices are just normal device tree
> nodes (meaning that they can have all the properties they want as a normal node)
> and then in the correspondent drivers you handle all the properties. For now,
> the only FW properties supported in the framework I sent are 'converters' and
> 'converter-name' which will be used to "create" the aggregate device. This
> pretty much means that the complete thing should only come up when all the
> devices you set in DT probe.
>
> Of course we can move more properties into the framework if we start to see some
> generic ones that are almost always present...
>
> One thing that Jonathan already mentioned is that the component API works in a
> away that you can have either 1->1 or 1->N (frontends->backends). So, if you
> have setups where you have more than one frontend (basically M->N) we need to
> make sure it still works. In theory (in the component API), I think you can have
> one backend associated with more than one frontend so we should be able to still
> get the M->N topology. Of course the "communications link" is always between
> frontend -> backend.
>
> I'll see if I send the devicetree over the weekend (don't have it in my current
> machine)
>
>
Here it goes the 2 nodes of interest in my testing...
adc_ad9467: ad9467@0 {
compatible = "adi,ad9467";
reg = <0>;
dmas = <&rx_dma 0>;
dma-names = "rx";
spi-max-frequency = <10000000>;
adi,spi-3wire-enable;
clocks = <&clk_ad9517 3>;
clock-names = "adc-clk";
converters = <&cf_ad9467_core_0>;
};
cf_ad9467_core_0: cf-ad9467-core-lpc@44a00000 {
compatible = "adi,axi-adc-10.0.a";
reg = <0x44A00000 0x10000>;
clocks = <&clkc 16>;
};
Naturally, converter-names only makes sense when you have more than one backend. But
see that in 'cf_ad9467_core_0', you are free to place a regulator (as I have a clock)
as long as you handle it in the backend driver.
- Nuno Sá
> >
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC v2 01/11] iio: introduce iio backend device
2023-09-25 6:48 ` Nuno Sá
@ 2023-09-26 16:44 ` Olivier MOYSAN
2023-09-28 7:15 ` Nuno Sá
0 siblings, 1 reply; 40+ messages in thread
From: Olivier MOYSAN @ 2023-09-26 16:44 UTC (permalink / raw)
To: Nuno Sá, Jonathan Cameron, Lars-Peter Clausen
Cc: linux-kernel, linux-iio, Fabrice GASNIER
Hi Nuno,
On 9/25/23 08:48, Nuno Sá wrote:
> Hi Olivier,
>
> On Fri, 2023-09-22 at 10:53 +0200, Nuno Sá wrote:
>> Hi Olivier,
>>
>> Sorry for the delay...
>>
>> On Mon, 2023-09-18 at 17:52 +0200, Olivier MOYSAN wrote:
>>> Hi Nuno
>>>
>>> On 9/11/23 11:39, Nuno Sá wrote:
>>>> On Tue, 2023-09-05 at 12:06 +0200, Olivier MOYSAN wrote:
>>>>> Hi Nuno,
>>>>>
>>>>> On 9/1/23 10:01, Nuno Sá wrote:
>>>>>> Hi Olivier,
>>>>>>
>>>>>> On Thu, 2023-08-31 at 18:14 +0200, Olivier MOYSAN wrote:
>>>>>>> Hi Nuno,
>>>>>>>
>>>>>>> On 7/28/23 10:42, Nuno Sá wrote:
>>>>>>>> Hi Olivier,
>>>>>>>>
>>>>>>>> On Thu, 2023-07-27 at 17:03 +0200, Olivier Moysan wrote:
>>>>>>>>> Add a new device type in IIO framework.
>>>>>>>>> This backend device does not compute channel attributes and does
>>>>>>>>> not
>>>>>>>>> expose
>>>>>>>>> them through sysfs, as done typically in iio-rescale frontend
>>>>>>>>> device.
>>>>>>>>> Instead, it allows to report information applying to channel
>>>>>>>>> attributes through callbacks. These backend devices can be
>>>>>>>>> cascaded
>>>>>>>>> to represent chained components.
>>>>>>>>> An IIO device configured as a consumer of a backend device can
>>>>>>>>> compute
>>>>>>>>> the channel attributes of the whole chain.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
>>>>>>>>> ---
>>>>>>>>> drivers/iio/Makefile | 1 +
>>>>>>>>> drivers/iio/industrialio-backend.c | 107
>>>>>>>>> +++++++++++++++++++++++++++++
>>>>>>>>> include/linux/iio/backend.h | 56 +++++++++++++++
>>>>>>>>> 3 files changed, 164 insertions(+)
>>>>>>>>> create mode 100644 drivers/iio/industrialio-backend.c
>>>>>>>>> create mode 100644 include/linux/iio/backend.h
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
>>>>>>>>> index 9622347a1c1b..9b59c6ab1738 100644
>>>>>>>>> --- a/drivers/iio/Makefile
>>>>>>>>> +++ b/drivers/iio/Makefile
>>>>>>>>> @@ -5,6 +5,7 @@
>>>>>>>>>
>>>>>>>>> obj-$(CONFIG_IIO) += industrialio.o
>>>>>>>>> industrialio-y := industrialio-core.o industrialio-event.o
>>>>>>>>> inkern.o
>>>>>>>>> +industrialio-$(CONFIG_IIO_BACKEND) += industrialio-backend.o
>>>>>>>>> industrialio-$(CONFIG_IIO_BUFFER) += industrialio-buffer.o
>>>>>>>>> industrialio-$(CONFIG_IIO_TRIGGER) += industrialio-trigger.o
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/iio/industrialio-backend.c
>>>>>>>>> b/drivers/iio/industrialio-
>>>>>>>>> backend.c
>>>>>>>>> new file mode 100644
>>>>>>>>> index 000000000000..7d0625889873
>>>>>>>>> --- /dev/null
>>>>>>>>> +++ b/drivers/iio/industrialio-backend.c
>>>>>>>>> @@ -0,0 +1,107 @@
>>>>>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>>>>>> +/* The industrial I/O core, backend handling functions
>>>>>>>>> + *
>>>>>>>>> + */
>>>>>>>>> +
>>>>>>>>> +#include <linux/kernel.h>
>>>>>>>>> +#include <linux/device.h>
>>>>>>>>> +#include <linux/property.h>
>>>>>>>>> +#include <linux/iio/iio.h>
>>>>>>>>> +#include <linux/iio/backend.h>
>>>>>>>>> +
>>>>>>>>> +static DEFINE_IDA(iio_backend_ida);
>>>>>>>>> +
>>>>>>>>> +#define to_iio_backend(_device) container_of((_device), struct
>>>>>>>>> iio_backend,
>>>>>>>>> dev)
>>>>>>>>> +
>>>>>>>>> +static void iio_backend_release(struct device *device)
>>>>>>>>> +{
>>>>>>>>> + struct iio_backend *backend = to_iio_backend(device);
>>>>>>>>> +
>>>>>>>>> + kfree(backend->name);
>>>>>>>>> + kfree(backend);
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +static const struct device_type iio_backend_type = {
>>>>>>>>> + .release = iio_backend_release,
>>>>>>>>> + .name = "iio_backend_device",
>>>>>>>>> +};
>>>>>>>>> +
>>>>>>>>> +struct iio_backend *iio_backend_alloc(struct device *parent)
>>>>>>>>> +{
>>>>>>>>> + struct iio_backend *backend;
>>>>>>>>> +
>>>>>>>>> + backend = devm_kzalloc(parent, sizeof(*backend),
>>>>>>>>> GFP_KERNEL);
>>>>>>>>>
>>>>>>>>
>>>>>>>> No error checking.
>>>>>>>>
>>>>>>>> I guess a lot of cleanings are still missing but the important thing
>>>>>>>> I
>>>>>>>> wanted to
>>>>>>>> notice is that the above pattern is not ok.
>>>>>>>> Your 'struct iio_backend *backend'' embeds a 'stuct device' which is
>>>>>>>> a
>>>>>>>> refcounted object. Nevertheless, you're binding the lifetime of your
>>>>>>>> object to
>>>>>>>> the parent device and that is wrong. The reason is that as soon as
>>>>>>>> your
>>>>>>>> parent
>>>>>>>> device get's released or just unbinded from it's driver, all the
>>>>>>>> devres
>>>>>>>> stuff
>>>>>>>> (including your 'struct iio_backend' object) will be released
>>>>>>>> independentof
>>>>>>>> your 'struct device' refcount value...
>>>>>>>>
>>>>>>>> So, you might argue this won't ever be an issue in here but the
>>>>>>>> pattern
>>>>>>>> is still
>>>>>>>> wrong. There are some talks about this, the last one was given at
>>>>>>>> the
>>>>>>>> latest
>>>>>>>> EOSS:
>>>>>>>>
>>>>>>>> https://www.youtube.com/watch?v=HCiJL7djGw8&list=PLbzoR-pLrL6pY8a8zSKRC6-AihFrruOkq&index=27&ab_channel=TheLinuxFoundation
>>>>>>>>
>>>>>>>
>>>>>>> This is a good point. Thanks for pointing it out. Sure, there are
>>>>>>> still
>>>>>>> many things to improve.
>>>>>>>
>>>>>>> I have seen the comment from Jonathan on your "Add converter
>>>>>>> framework"
>>>>>>> serie. I had a quick look at the serie. It seems that we share the
>>>>>>> need
>>>>>>> to aggregate some IIO devices. But I need to read it more carefully to
>>>>>>> check if we can find some convergences here.
>>>>>>
>>>>>> Yeah, In my case, the backend devices are typically FPGA soft cores and
>>>>>> the
>>>>>> aggregate
>>>>>> device might connect to multiple of these backends. That was one of the
>>>>>> reason why I
>>>>>> used the component API where the aggregate device is only configured
>>>>>> when
>>>>>> all the
>>>>>> devices are probed. Similarly, when one of them is unbind, the whole
>>>>>> thing
>>>>>> should be
>>>>>> torn down. Also, in my case, the frontend device needs to do a lot of
>>>>>> setup
>>>>>> on the
>>>>>> backend device so the whole thing works (so I do have/need a lot more
>>>>>> .ops).
>>>>>>
>>>>>> Anyways, it does not matter much what the backend device is and from a
>>>>>> first
>>>>>> glance
>>>>>> and looking at the .ops you have, it seems that this could easily be
>>>>>> supported in the
>>>>>> framework I'm adding. The only things I'm seeing are:
>>>>>
>>>>> Thanks for your feedback. Yes, my feeling is that the API I need for the
>>>>> dfsdm use case, can be covered by the API you propose. I'm not familiar
>>>>> with component API however, as I discovered it in your serie. It is not
>>>>> clear for me how this affects device tree description of the hardware.
>>>>
>>>> Your aggregate device (that we can think of as a frontend device needs to
>>>> properly reference all the backends it needs - in your case I guess it's
>>>> just
>>>> one device). The dts properties I have for now are 'converters' and
>>>> 'converter-
>>>> names'. But one thing that starts to become clear to me is that I should
>>>> probably change the name for the framework. Maybe industrialio-aggregate.c
>>>> if we
>>>> keep the component API (and so the same frontend + backend naming) or just
>>>> industrialio-backend.c (as you have now) if we go with a typical OF lookup.
>>>>
>>>
>>> In my case I have a digital filter peripheral (frontend) linked to
>>> several sigma delta converters (backends). So, here 'converters'
>>> property may be relevant as well. But I agree that a more generic name
>>> seems better for the long term.
>>>
>>> My backend devices need to get a regulator phandle from the device tree.
>>> It seems that the component API does not offer services allowing to
>>> retrieve DT properties for the sub-devices. Tell me if I'm wrong, but I
>>> think this constraint require to change converter framework to a typical
>>> OF lookup.
>>>
>>> Could you please share the structure of your DT for your ad9476 based
>>> example ? This will help me identify the gaps regarding my need.
>>>
>>
>> I might be missing something but there should be no limitation in the component
>> stuff for this. Note your frontend/backend devices are just normal device tree
>> nodes (meaning that they can have all the properties they want as a normal node)
>> and then in the correspondent drivers you handle all the properties. For now,
>> the only FW properties supported in the framework I sent are 'converters' and
>> 'converter-name' which will be used to "create" the aggregate device. This
>> pretty much means that the complete thing should only come up when all the
>> devices you set in DT probe.
>>
>> Of course we can move more properties into the framework if we start to see some
>> generic ones that are almost always present...
>>
>> One thing that Jonathan already mentioned is that the component API works in a
>> away that you can have either 1->1 or 1->N (frontends->backends). So, if you
>> have setups where you have more than one frontend (basically M->N) we need to
>> make sure it still works. In theory (in the component API), I think you can have
>> one backend associated with more than one frontend so we should be able to still
>> get the M->N topology. Of course the "communications link" is always between
>> frontend -> backend.
>>
>> I'll see if I send the devicetree over the weekend (don't have it in my current
>> machine)
>>
>>
>
> Here it goes the 2 nodes of interest in my testing...
>
> adc_ad9467: ad9467@0 {
> compatible = "adi,ad9467";
> reg = <0>;
>
> dmas = <&rx_dma 0>;
> dma-names = "rx";
>
> spi-max-frequency = <10000000>;
> adi,spi-3wire-enable;
>
> clocks = <&clk_ad9517 3>;
> clock-names = "adc-clk";
>
> converters = <&cf_ad9467_core_0>;
> };
>
> cf_ad9467_core_0: cf-ad9467-core-lpc@44a00000 {
> compatible = "adi,axi-adc-10.0.a";
> reg = <0x44A00000 0x10000>;
>
> clocks = <&clkc 16>;
> };
>
> Naturally, converter-names only makes sense when you have more than one backend. But
> see that in 'cf_ad9467_core_0', you are free to place a regulator (as I have a clock)
> as long as you handle it in the backend driver.
>
> - Nuno Sá
Thanks for the example. This helped me prototyping a dfsdm driver based
on the converter framework. Regarding device tree and driver update this
looks fine. I could integrate the API smartly in my frontend (dfsdm) and
backend (sd modulator).
My prototype executes up to probe. I have noticed however that init
(backend & frontend) ops are not called in my implementation. I can see
that init ops are called from bind ops. component_bind_all() calls
converter bind ops, but component_bind_all() is called from converter
bind ops. So, I don't understand how initialization can proceed with
these circular calls. Maybe I missed something here.
The change in the DT has an impact (But moderated) on legacy. Breaking
the legacy was unavoidable anyway.
DFSDM legacy binding (with two channels)
dfsdm_pdm1: filter@1 {
compatible = "st,stm32-dfsdm-adc";
st,adc-channels = <2 3>;
st,adc-channel-types = "SPI_R", "SPI_R";
...
io-channels = <&sd_adc2 &sd_adc3>;
};
DFSDM binding with converter fw
dfsdm_pdm1: filter@1 {
compatible = "st,stm32-dfsdm-adc";
st,adc-channels = <2 3>;
st,adc-channel-types = "SPI_R", "SPI_R";
...
converters = <&sd_adc2 &sd_adc3>;
};
I have also the aim to change DFSDM bindings to use IIO generic channels
bindings (bindings/iio/adc/adc.yaml).
Ideally the DFSDM bindings should looks like this:
dfsdm_pdm1: filter@1 {
compatible = "st,stm32-dfsdm-adc";
channel@2 {
reg = <2>;
st,adc-channel-types = "SPI_R";
...
converters = <&sd_adc2>;
};
channel@3 {
reg = <3>;
st,adc-channel-types = "SPI_R";
...
converters = <&sd_adc3>;
};
};
But it seems that current framework converter API cannot support this
topology.
As a fallback solution the following binding may be adopted
dfsdm_pdm1: filter@1 {
compatible = "st,stm32-dfsdm-adc";
channel@2 {
reg = <2>;
st,adc-channel-types = "SPI_R";
...
};
channel@3 {
reg = <3>;
st,adc-channel-types = "SPI_R";
...
};
converters = <&sd_adc2 &sd_adc3>;
In this case the frontend driver needs a mean to map backend and
channels. It's not the smartest solution, yet. Especially since the use
of generic channel is quite common.
Perhaps the converter_frontend_add() API needs to be extended to support
generic channel configuration. Maybe the IIO core should provide the
related helpers as well. (As far as I know this does not exists).
So, still opened questions ..
That said, I feel confident that the converter framework is a good
option for the DFSDM use case.
BRs
Olivier
>>>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC v2 01/11] iio: introduce iio backend device
2023-09-26 16:44 ` Olivier MOYSAN
@ 2023-09-28 7:15 ` Nuno Sá
2023-09-28 16:30 ` Olivier MOYSAN
0 siblings, 1 reply; 40+ messages in thread
From: Nuno Sá @ 2023-09-28 7:15 UTC (permalink / raw)
To: Olivier MOYSAN, Jonathan Cameron, Lars-Peter Clausen
Cc: linux-kernel, linux-iio, Fabrice GASNIER
Hi Olivier,
On Tue, 2023-09-26 at 18:44 +0200, Olivier MOYSAN wrote:
> Hi Nuno,
>
> On 9/25/23 08:48, Nuno Sá wrote:
> > Hi Olivier,
> >
> > On Fri, 2023-09-22 at 10:53 +0200, Nuno Sá wrote:
> > > Hi Olivier,
> > >
> > > Sorry for the delay...
> > >
> > > On Mon, 2023-09-18 at 17:52 +0200, Olivier MOYSAN wrote:
> > > > Hi Nuno
> > > >
> > > > On 9/11/23 11:39, Nuno Sá wrote:
> > > > > On Tue, 2023-09-05 at 12:06 +0200, Olivier MOYSAN wrote:
> > > > > > Hi Nuno,
> > > > > >
> > > > > > On 9/1/23 10:01, Nuno Sá wrote:
> > > > > > > Hi Olivier,
> > > > > > >
> > > > > > > On Thu, 2023-08-31 at 18:14 +0200, Olivier MOYSAN wrote:
> > > > > > > > Hi Nuno,
> > > > > > > >
> > > > > > > > On 7/28/23 10:42, Nuno Sá wrote:
> > > > > > > > > Hi Olivier,
> > > > > > > > >
> > > > > > > > > On Thu, 2023-07-27 at 17:03 +0200, Olivier Moysan wrote:
> > > > > > > > > > Add a new device type in IIO framework.
> > > > > > > > > > This backend device does not compute channel attributes and does
> > > > > > > > > > not
> > > > > > > > > > expose
> > > > > > > > > > them through sysfs, as done typically in iio-rescale frontend
> > > > > > > > > > device.
> > > > > > > > > > Instead, it allows to report information applying to channel
> > > > > > > > > > attributes through callbacks. These backend devices can be
> > > > > > > > > > cascaded
> > > > > > > > > > to represent chained components.
> > > > > > > > > > An IIO device configured as a consumer of a backend device can
> > > > > > > > > > compute
> > > > > > > > > > the channel attributes of the whole chain.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
> > > > > > > > > > ---
> > > > > > > > > > drivers/iio/Makefile | 1 +
> > > > > > > > > > drivers/iio/industrialio-backend.c | 107
> > > > > > > > > > +++++++++++++++++++++++++++++
> > > > > > > > > > include/linux/iio/backend.h | 56 +++++++++++++++
> > > > > > > > > > 3 files changed, 164 insertions(+)
> > > > > > > > > > create mode 100644 drivers/iio/industrialio-backend.c
> > > > > > > > > > create mode 100644 include/linux/iio/backend.h
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
> > > > > > > > > > index 9622347a1c1b..9b59c6ab1738 100644
> > > > > > > > > > --- a/drivers/iio/Makefile
> > > > > > > > > > +++ b/drivers/iio/Makefile
> > > > > > > > > > @@ -5,6 +5,7 @@
> > > > > > > > > >
> > > > > > > > > > obj-$(CONFIG_IIO) += industrialio.o
> > > > > > > > > > industrialio-y := industrialio-core.o industrialio-event.o
> > > > > > > > > > inkern.o
> > > > > > > > > > +industrialio-$(CONFIG_IIO_BACKEND) += industrialio-backend.o
> > > > > > > > > > industrialio-$(CONFIG_IIO_BUFFER) += industrialio-buffer.o
> > > > > > > > > > industrialio-$(CONFIG_IIO_TRIGGER) += industrialio-trigger.o
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/iio/industrialio-backend.c
> > > > > > > > > > b/drivers/iio/industrialio-
> > > > > > > > > > backend.c
> > > > > > > > > > new file mode 100644
> > > > > > > > > > index 000000000000..7d0625889873
> > > > > > > > > > --- /dev/null
> > > > > > > > > > +++ b/drivers/iio/industrialio-backend.c
> > > > > > > > > > @@ -0,0 +1,107 @@
> > > > > > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > > > > > +/* The industrial I/O core, backend handling functions
> > > > > > > > > > + *
> > > > > > > > > > + */
> > > > > > > > > > +
> > > > > > > > > > +#include <linux/kernel.h>
> > > > > > > > > > +#include <linux/device.h>
> > > > > > > > > > +#include <linux/property.h>
> > > > > > > > > > +#include <linux/iio/iio.h>
> > > > > > > > > > +#include <linux/iio/backend.h>
> > > > > > > > > > +
> > > > > > > > > > +static DEFINE_IDA(iio_backend_ida);
> > > > > > > > > > +
> > > > > > > > > > +#define to_iio_backend(_device) container_of((_device), struct
> > > > > > > > > > iio_backend,
> > > > > > > > > > dev)
> > > > > > > > > > +
> > > > > > > > > > +static void iio_backend_release(struct device *device)
> > > > > > > > > > +{
> > > > > > > > > > + struct iio_backend *backend = to_iio_backend(device);
> > > > > > > > > > +
> > > > > > > > > > + kfree(backend->name);
> > > > > > > > > > + kfree(backend);
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > > +static const struct device_type iio_backend_type = {
> > > > > > > > > > + .release = iio_backend_release,
> > > > > > > > > > + .name = "iio_backend_device",
> > > > > > > > > > +};
> > > > > > > > > > +
> > > > > > > > > > +struct iio_backend *iio_backend_alloc(struct device *parent)
> > > > > > > > > > +{
> > > > > > > > > > + struct iio_backend *backend;
> > > > > > > > > > +
> > > > > > > > > > + backend = devm_kzalloc(parent, sizeof(*backend),
> > > > > > > > > > GFP_KERNEL);
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > No error checking.
> > > > > > > > >
> > > > > > > > > I guess a lot of cleanings are still missing but the important
> > > > > > > > > thing
> > > > > > > > > I
> > > > > > > > > wanted to
> > > > > > > > > notice is that the above pattern is not ok.
> > > > > > > > > Your 'struct iio_backend *backend'' embeds a 'stuct device' which
> > > > > > > > > is
> > > > > > > > > a
> > > > > > > > > refcounted object. Nevertheless, you're binding the lifetime of
> > > > > > > > > your
> > > > > > > > > object to
> > > > > > > > > the parent device and that is wrong. The reason is that as soon as
> > > > > > > > > your
> > > > > > > > > parent
> > > > > > > > > device get's released or just unbinded from it's driver, all the
> > > > > > > > > devres
> > > > > > > > > stuff
> > > > > > > > > (including your 'struct iio_backend' object) will be released
> > > > > > > > > independentof
> > > > > > > > > your 'struct device' refcount value...
> > > > > > > > >
> > > > > > > > > So, you might argue this won't ever be an issue in here but the
> > > > > > > > > pattern
> > > > > > > > > is still
> > > > > > > > > wrong. There are some talks about this, the last one was given at
> > > > > > > > > the
> > > > > > > > > latest
> > > > > > > > > EOSS:
> > > > > > > > >
> > > > > > > > > https://www.youtube.com/watch?v=HCiJL7djGw8&list=PLbzoR-pLrL6pY8a8zSKRC6-AihFrruOkq&index=27&ab_channel=TheLinuxFoundation
> > > > > > > > >
> > > > > > > >
> > > > > > > > This is a good point. Thanks for pointing it out. Sure, there are
> > > > > > > > still
> > > > > > > > many things to improve.
> > > > > > > >
> > > > > > > > I have seen the comment from Jonathan on your "Add converter
> > > > > > > > framework"
> > > > > > > > serie. I had a quick look at the serie. It seems that we share the
> > > > > > > > need
> > > > > > > > to aggregate some IIO devices. But I need to read it more carefully
> > > > > > > > to
> > > > > > > > check if we can find some convergences here.
> > > > > > >
> > > > > > > Yeah, In my case, the backend devices are typically FPGA soft cores and
> > > > > > > the
> > > > > > > aggregate
> > > > > > > device might connect to multiple of these backends. That was one of the
> > > > > > > reason why I
> > > > > > > used the component API where the aggregate device is only configured
> > > > > > > when
> > > > > > > all the
> > > > > > > devices are probed. Similarly, when one of them is unbind, the whole
> > > > > > > thing
> > > > > > > should be
> > > > > > > torn down. Also, in my case, the frontend device needs to do a lot of
> > > > > > > setup
> > > > > > > on the
> > > > > > > backend device so the whole thing works (so I do have/need a lot more
> > > > > > > .ops).
> > > > > > >
> > > > > > > Anyways, it does not matter much what the backend device is and from a
> > > > > > > first
> > > > > > > glance
> > > > > > > and looking at the .ops you have, it seems that this could easily be
> > > > > > > supported in the
> > > > > > > framework I'm adding. The only things I'm seeing are:
> > > > > >
> > > > > > Thanks for your feedback. Yes, my feeling is that the API I need for the
> > > > > > dfsdm use case, can be covered by the API you propose. I'm not familiar
> > > > > > with component API however, as I discovered it in your serie. It is not
> > > > > > clear for me how this affects device tree description of the hardware.
> > > > >
> > > > > Your aggregate device (that we can think of as a frontend device needs to
> > > > > properly reference all the backends it needs - in your case I guess it's
> > > > > just
> > > > > one device). The dts properties I have for now are 'converters' and
> > > > > 'converter-
> > > > > names'. But one thing that starts to become clear to me is that I should
> > > > > probably change the name for the framework. Maybe industrialio-aggregate.c
> > > > > if we
> > > > > keep the component API (and so the same frontend + backend naming) or just
> > > > > industrialio-backend.c (as you have now) if we go with a typical OF lookup.
> > > > >
> > > >
> > > > In my case I have a digital filter peripheral (frontend) linked to
> > > > several sigma delta converters (backends). So, here 'converters'
> > > > property may be relevant as well. But I agree that a more generic name
> > > > seems better for the long term.
> > > >
> > > > My backend devices need to get a regulator phandle from the device tree.
> > > > It seems that the component API does not offer services allowing to
> > > > retrieve DT properties for the sub-devices. Tell me if I'm wrong, but I
> > > > think this constraint require to change converter framework to a typical
> > > > OF lookup.
> > > >
> > > > Could you please share the structure of your DT for your ad9476 based
> > > > example ? This will help me identify the gaps regarding my need.
> > > >
> > >
> > > I might be missing something but there should be no limitation in the component
> > > stuff for this. Note your frontend/backend devices are just normal device tree
> > > nodes (meaning that they can have all the properties they want as a normal
> > > node)
> > > and then in the correspondent drivers you handle all the properties. For now,
> > > the only FW properties supported in the framework I sent are 'converters' and
> > > 'converter-name' which will be used to "create" the aggregate device. This
> > > pretty much means that the complete thing should only come up when all the
> > > devices you set in DT probe.
> > >
> > > Of course we can move more properties into the framework if we start to see
> > > some
> > > generic ones that are almost always present...
> > >
> > > One thing that Jonathan already mentioned is that the component API works in a
> > > away that you can have either 1->1 or 1->N (frontends->backends). So, if you
> > > have setups where you have more than one frontend (basically M->N) we need to
> > > make sure it still works. In theory (in the component API), I think you can
> > > have
> > > one backend associated with more than one frontend so we should be able to
> > > still
> > > get the M->N topology. Of course the "communications link" is always between
> > > frontend -> backend.
> > >
> > > I'll see if I send the devicetree over the weekend (don't have it in my current
> > > machine)
> > >
> > >
> >
> > Here it goes the 2 nodes of interest in my testing...
> >
> > adc_ad9467: ad9467@0 {
> > compatible = "adi,ad9467";
> > reg = <0>;
> >
> > dmas = <&rx_dma 0>;
> > dma-names = "rx";
> >
> > spi-max-frequency = <10000000>;
> > adi,spi-3wire-enable;
> >
> > clocks = <&clk_ad9517 3>;
> > clock-names = "adc-clk";
> >
> > converters = <&cf_ad9467_core_0>;
> > };
> >
> > cf_ad9467_core_0: cf-ad9467-core-lpc@44a00000 {
> > compatible = "adi,axi-adc-10.0.a";
> > reg = <0x44A00000 0x10000>;
> >
> > clocks = <&clkc 16>;
> > };
> >
> > Naturally, converter-names only makes sense when you have more than one backend.
> > But
> > see that in 'cf_ad9467_core_0', you are free to place a regulator (as I have a
> > clock)
> > as long as you handle it in the backend driver.
> >
> > - Nuno Sá
>
> Thanks for the example. This helped me prototyping a dfsdm driver based
> on the converter framework. Regarding device tree and driver update this
> looks fine. I could integrate the API smartly in my frontend (dfsdm) and
> backend (sd modulator).
>
> My prototype executes up to probe. I have noticed however that init
> (backend & frontend) ops are not called in my implementation. I can see
> that init ops are called from bind ops. component_bind_all() calls
Note that you need to call converter_frontend_add() from your frontend device (stm32-
dfsdm-adc) probe function and converter_add() from your backend's probes. And
ideally, this is the only thing you do at probe. Then, once all the elements are
probed, the complete aggregate device is initialised and the .bind()/.init() function
should be called.
And I want to reinforce the above, in the component API, things will only come up
when all the pieces (all the converters you specified in DT) are probed. The same is
true if one of the elements is unbound from it's driver - all the other elements in
the aggregate device will be torn down and converter_frontend_unbind() will be
called. This means it's an all or nothing solution... Let me know if this does not
work for you.
> converter bind ops, but component_bind_all() is called from converter
> bind ops. So, I don't understand how initialization can proceed with
> these circular calls. Maybe I missed something here.
>
This one I'm not following... component_bind_all() should be called from
converter_frontend_bind() and this will call all converter_bind() you have (depends
on how many backends you have). After all backends are initialized, .frontend_init()
is called. In there, if you need (most likely you do) an handle to a converter you
then need to call converter_get(). So, component_bind_all() should not be called from
converter bind ops but from frontend_component_ops which are the
component_master_ops. If this is not happening, then we have an issue :)
> The change in the DT has an impact (But moderated) on legacy. Breaking
> the legacy was unavoidable anyway.
>
> DFSDM legacy binding (with two channels)
> dfsdm_pdm1: filter@1 {
> compatible = "st,stm32-dfsdm-adc";
> st,adc-channels = <2 3>;
> st,adc-channel-types = "SPI_R", "SPI_R";
> ...
> io-channels = <&sd_adc2 &sd_adc3>;
> };
>
> DFSDM binding with converter fw
> dfsdm_pdm1: filter@1 {
> compatible = "st,stm32-dfsdm-adc";
> st,adc-channels = <2 3>;
> st,adc-channel-types = "SPI_R", "SPI_R";
> ...
> converters = <&sd_adc2 &sd_adc3>;
> };
>
> I have also the aim to change DFSDM bindings to use IIO generic channels
> bindings (bindings/iio/adc/adc.yaml).
>
> Ideally the DFSDM bindings should looks like this:
> dfsdm_pdm1: filter@1 {
> compatible = "st,stm32-dfsdm-adc";
> channel@2 {
> reg = <2>;
> st,adc-channel-types = "SPI_R";
> ...
> converters = <&sd_adc2>;
> };
> channel@3 {
> reg = <3>;
> st,adc-channel-types = "SPI_R";
> ...
> converters = <&sd_adc3>;
> };
> };
>
> But it seems that current framework converter API cannot support this
> topology.
>
Indeed this won't work and I honestly it never crossed my mind ehehe,
> As a fallback solution the following binding may be adopted
> dfsdm_pdm1: filter@1 {
> compatible = "st,stm32-dfsdm-adc";
> channel@2 {
> reg = <2>;
> st,adc-channel-types = "SPI_R";
> ...
> };
> channel@3 {
> reg = <3>;
> st,adc-channel-types = "SPI_R";
> ...
> };
> converters = <&sd_adc2 &sd_adc3>;
>
> In this case the frontend driver needs a mean to map backend and
> channels. It's not the smartest solution, yet. Especially since the use
> of generic channel is quite common.
Yeah, I'm also not a fan of that... To support the above topology and from the top of
my head we could either:
1) Somehow split converter_frontend_add() to give more control to the caller to call
converter_frontend_add_matches() and in this case have another API that accepts a
fwnode.
2) Just extend converter_frontend_add_matches() so that we also look into child nodes
for 'converters'
Then, on the get() side, we would need something like converters_get_from_fwnode() to
get each handle. I would likely prefer to go with 2) because 1) already implies some
FW parsing during probe that I would like to avoid.
Anyways the above is already showing that maybe going with the component API for
something more generic might be a stretch and harder to scale for everyone needs.
With an OF lookup, the above topology would be easier to accomplish (though we would
always need a converters_get_from_fwnode() kind of function).
When you say:
"In this case the frontend driver needs a mean to map backend and channels."
Could 'converter-names' be used for the above? Or would the above be trivial with an
OF lookup?
> Perhaps the converter_frontend_add() API needs to be extended to support
> generic channel configuration. Maybe the IIO core should provide the
> related helpers as well. (As far as I know this does not exists).
> So, still opened questions ..
>
No sure what do you mean by the above?
> That said, I feel confident that the converter framework is a good
> option for the DFSDM use case.
>
Yeah, I'm also confident that we can get something that suits both our usecases
either with OF or component. I must say that I'm tempted to send a version of this
with an OF lookup just so we get a look on how it would look like and compare against
the component API.
- Nuno Sá
> > >
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC v2 01/11] iio: introduce iio backend device
2023-09-28 7:15 ` Nuno Sá
@ 2023-09-28 16:30 ` Olivier MOYSAN
0 siblings, 0 replies; 40+ messages in thread
From: Olivier MOYSAN @ 2023-09-28 16:30 UTC (permalink / raw)
To: Nuno Sá, Jonathan Cameron, Lars-Peter Clausen
Cc: linux-kernel, linux-iio, Fabrice GASNIER
Hi Nuno,
On 9/28/23 09:15, Nuno Sá wrote:
> Hi Olivier,
>
> On Tue, 2023-09-26 at 18:44 +0200, Olivier MOYSAN wrote:
>> Hi Nuno,
>>
>> On 9/25/23 08:48, Nuno Sá wrote:
>>> Hi Olivier,
>>>
>>> On Fri, 2023-09-22 at 10:53 +0200, Nuno Sá wrote:
>>>> Hi Olivier,
>>>>
>>>> Sorry for the delay...
>>>>
>>>> On Mon, 2023-09-18 at 17:52 +0200, Olivier MOYSAN wrote:
>>>>> Hi Nuno
>>>>>
>>>>> On 9/11/23 11:39, Nuno Sá wrote:
>>>>>> On Tue, 2023-09-05 at 12:06 +0200, Olivier MOYSAN wrote:
>>>>>>> Hi Nuno,
>>>>>>>
>>>>>>> On 9/1/23 10:01, Nuno Sá wrote:
>>>>>>>> Hi Olivier,
>>>>>>>>
>>>>>>>> On Thu, 2023-08-31 at 18:14 +0200, Olivier MOYSAN wrote:
>>>>>>>>> Hi Nuno,
>>>>>>>>>
>>>>>>>>> On 7/28/23 10:42, Nuno Sá wrote:
>>>>>>>>>> Hi Olivier,
>>>>>>>>>>
>>>>>>>>>> On Thu, 2023-07-27 at 17:03 +0200, Olivier Moysan wrote:
>>>>>>>>>>> Add a new device type in IIO framework.
>>>>>>>>>>> This backend device does not compute channel attributes and does
>>>>>>>>>>> not
>>>>>>>>>>> expose
>>>>>>>>>>> them through sysfs, as done typically in iio-rescale frontend
>>>>>>>>>>> device.
>>>>>>>>>>> Instead, it allows to report information applying to channel
>>>>>>>>>>> attributes through callbacks. These backend devices can be
>>>>>>>>>>> cascaded
>>>>>>>>>>> to represent chained components.
>>>>>>>>>>> An IIO device configured as a consumer of a backend device can
>>>>>>>>>>> compute
>>>>>>>>>>> the channel attributes of the whole chain.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
>>>>>>>>>>> ---
>>>>>>>>>>> drivers/iio/Makefile | 1 +
>>>>>>>>>>> drivers/iio/industrialio-backend.c | 107
>>>>>>>>>>> +++++++++++++++++++++++++++++
>>>>>>>>>>> include/linux/iio/backend.h | 56 +++++++++++++++
>>>>>>>>>>> 3 files changed, 164 insertions(+)
>>>>>>>>>>> create mode 100644 drivers/iio/industrialio-backend.c
>>>>>>>>>>> create mode 100644 include/linux/iio/backend.h
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
>>>>>>>>>>> index 9622347a1c1b..9b59c6ab1738 100644
>>>>>>>>>>> --- a/drivers/iio/Makefile
>>>>>>>>>>> +++ b/drivers/iio/Makefile
>>>>>>>>>>> @@ -5,6 +5,7 @@
>>>>>>>>>>>
>>>>>>>>>>> obj-$(CONFIG_IIO) += industrialio.o
>>>>>>>>>>> industrialio-y := industrialio-core.o industrialio-event.o
>>>>>>>>>>> inkern.o
>>>>>>>>>>> +industrialio-$(CONFIG_IIO_BACKEND) += industrialio-backend.o
>>>>>>>>>>> industrialio-$(CONFIG_IIO_BUFFER) += industrialio-buffer.o
>>>>>>>>>>> industrialio-$(CONFIG_IIO_TRIGGER) += industrialio-trigger.o
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/iio/industrialio-backend.c
>>>>>>>>>>> b/drivers/iio/industrialio-
>>>>>>>>>>> backend.c
>>>>>>>>>>> new file mode 100644
>>>>>>>>>>> index 000000000000..7d0625889873
>>>>>>>>>>> --- /dev/null
>>>>>>>>>>> +++ b/drivers/iio/industrialio-backend.c
>>>>>>>>>>> @@ -0,0 +1,107 @@
>>>>>>>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>>>>>>>> +/* The industrial I/O core, backend handling functions
>>>>>>>>>>> + *
>>>>>>>>>>> + */
>>>>>>>>>>> +
>>>>>>>>>>> +#include <linux/kernel.h>
>>>>>>>>>>> +#include <linux/device.h>
>>>>>>>>>>> +#include <linux/property.h>
>>>>>>>>>>> +#include <linux/iio/iio.h>
>>>>>>>>>>> +#include <linux/iio/backend.h>
>>>>>>>>>>> +
>>>>>>>>>>> +static DEFINE_IDA(iio_backend_ida);
>>>>>>>>>>> +
>>>>>>>>>>> +#define to_iio_backend(_device) container_of((_device), struct
>>>>>>>>>>> iio_backend,
>>>>>>>>>>> dev)
>>>>>>>>>>> +
>>>>>>>>>>> +static void iio_backend_release(struct device *device)
>>>>>>>>>>> +{
>>>>>>>>>>> + struct iio_backend *backend = to_iio_backend(device);
>>>>>>>>>>> +
>>>>>>>>>>> + kfree(backend->name);
>>>>>>>>>>> + kfree(backend);
>>>>>>>>>>> +}
>>>>>>>>>>> +
>>>>>>>>>>> +static const struct device_type iio_backend_type = {
>>>>>>>>>>> + .release = iio_backend_release,
>>>>>>>>>>> + .name = "iio_backend_device",
>>>>>>>>>>> +};
>>>>>>>>>>> +
>>>>>>>>>>> +struct iio_backend *iio_backend_alloc(struct device *parent)
>>>>>>>>>>> +{
>>>>>>>>>>> + struct iio_backend *backend;
>>>>>>>>>>> +
>>>>>>>>>>> + backend = devm_kzalloc(parent, sizeof(*backend),
>>>>>>>>>>> GFP_KERNEL);
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> No error checking.
>>>>>>>>>>
>>>>>>>>>> I guess a lot of cleanings are still missing but the important
>>>>>>>>>> thing
>>>>>>>>>> I
>>>>>>>>>> wanted to
>>>>>>>>>> notice is that the above pattern is not ok.
>>>>>>>>>> Your 'struct iio_backend *backend'' embeds a 'stuct device' which
>>>>>>>>>> is
>>>>>>>>>> a
>>>>>>>>>> refcounted object. Nevertheless, you're binding the lifetime of
>>>>>>>>>> your
>>>>>>>>>> object to
>>>>>>>>>> the parent device and that is wrong. The reason is that as soon as
>>>>>>>>>> your
>>>>>>>>>> parent
>>>>>>>>>> device get's released or just unbinded from it's driver, all the
>>>>>>>>>> devres
>>>>>>>>>> stuff
>>>>>>>>>> (including your 'struct iio_backend' object) will be released
>>>>>>>>>> independentof
>>>>>>>>>> your 'struct device' refcount value...
>>>>>>>>>>
>>>>>>>>>> So, you might argue this won't ever be an issue in here but the
>>>>>>>>>> pattern
>>>>>>>>>> is still
>>>>>>>>>> wrong. There are some talks about this, the last one was given at
>>>>>>>>>> the
>>>>>>>>>> latest
>>>>>>>>>> EOSS:
>>>>>>>>>>
>>>>>>>>>> https://www.youtube.com/watch?v=HCiJL7djGw8&list=PLbzoR-pLrL6pY8a8zSKRC6-AihFrruOkq&index=27&ab_channel=TheLinuxFoundation
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> This is a good point. Thanks for pointing it out. Sure, there are
>>>>>>>>> still
>>>>>>>>> many things to improve.
>>>>>>>>>
>>>>>>>>> I have seen the comment from Jonathan on your "Add converter
>>>>>>>>> framework"
>>>>>>>>> serie. I had a quick look at the serie. It seems that we share the
>>>>>>>>> need
>>>>>>>>> to aggregate some IIO devices. But I need to read it more carefully
>>>>>>>>> to
>>>>>>>>> check if we can find some convergences here.
>>>>>>>>
>>>>>>>> Yeah, In my case, the backend devices are typically FPGA soft cores and
>>>>>>>> the
>>>>>>>> aggregate
>>>>>>>> device might connect to multiple of these backends. That was one of the
>>>>>>>> reason why I
>>>>>>>> used the component API where the aggregate device is only configured
>>>>>>>> when
>>>>>>>> all the
>>>>>>>> devices are probed. Similarly, when one of them is unbind, the whole
>>>>>>>> thing
>>>>>>>> should be
>>>>>>>> torn down. Also, in my case, the frontend device needs to do a lot of
>>>>>>>> setup
>>>>>>>> on the
>>>>>>>> backend device so the whole thing works (so I do have/need a lot more
>>>>>>>> .ops).
>>>>>>>>
>>>>>>>> Anyways, it does not matter much what the backend device is and from a
>>>>>>>> first
>>>>>>>> glance
>>>>>>>> and looking at the .ops you have, it seems that this could easily be
>>>>>>>> supported in the
>>>>>>>> framework I'm adding. The only things I'm seeing are:
>>>>>>>
>>>>>>> Thanks for your feedback. Yes, my feeling is that the API I need for the
>>>>>>> dfsdm use case, can be covered by the API you propose. I'm not familiar
>>>>>>> with component API however, as I discovered it in your serie. It is not
>>>>>>> clear for me how this affects device tree description of the hardware.
>>>>>>
>>>>>> Your aggregate device (that we can think of as a frontend device needs to
>>>>>> properly reference all the backends it needs - in your case I guess it's
>>>>>> just
>>>>>> one device). The dts properties I have for now are 'converters' and
>>>>>> 'converter-
>>>>>> names'. But one thing that starts to become clear to me is that I should
>>>>>> probably change the name for the framework. Maybe industrialio-aggregate.c
>>>>>> if we
>>>>>> keep the component API (and so the same frontend + backend naming) or just
>>>>>> industrialio-backend.c (as you have now) if we go with a typical OF lookup.
>>>>>>
>>>>>
>>>>> In my case I have a digital filter peripheral (frontend) linked to
>>>>> several sigma delta converters (backends). So, here 'converters'
>>>>> property may be relevant as well. But I agree that a more generic name
>>>>> seems better for the long term.
>>>>>
>>>>> My backend devices need to get a regulator phandle from the device tree.
>>>>> It seems that the component API does not offer services allowing to
>>>>> retrieve DT properties for the sub-devices. Tell me if I'm wrong, but I
>>>>> think this constraint require to change converter framework to a typical
>>>>> OF lookup.
>>>>>
>>>>> Could you please share the structure of your DT for your ad9476 based
>>>>> example ? This will help me identify the gaps regarding my need.
>>>>>
>>>>
>>>> I might be missing something but there should be no limitation in the component
>>>> stuff for this. Note your frontend/backend devices are just normal device tree
>>>> nodes (meaning that they can have all the properties they want as a normal
>>>> node)
>>>> and then in the correspondent drivers you handle all the properties. For now,
>>>> the only FW properties supported in the framework I sent are 'converters' and
>>>> 'converter-name' which will be used to "create" the aggregate device. This
>>>> pretty much means that the complete thing should only come up when all the
>>>> devices you set in DT probe.
>>>>
>>>> Of course we can move more properties into the framework if we start to see
>>>> some
>>>> generic ones that are almost always present...
>>>>
>>>> One thing that Jonathan already mentioned is that the component API works in a
>>>> away that you can have either 1->1 or 1->N (frontends->backends). So, if you
>>>> have setups where you have more than one frontend (basically M->N) we need to
>>>> make sure it still works. In theory (in the component API), I think you can
>>>> have
>>>> one backend associated with more than one frontend so we should be able to
>>>> still
>>>> get the M->N topology. Of course the "communications link" is always between
>>>> frontend -> backend.
>>>>
>>>> I'll see if I send the devicetree over the weekend (don't have it in my current
>>>> machine)
>>>>
>>>>
>>>
>>> Here it goes the 2 nodes of interest in my testing...
>>>
>>> adc_ad9467: ad9467@0 {
>>> compatible = "adi,ad9467";
>>> reg = <0>;
>>>
>>> dmas = <&rx_dma 0>;
>>> dma-names = "rx";
>>>
>>> spi-max-frequency = <10000000>;
>>> adi,spi-3wire-enable;
>>>
>>> clocks = <&clk_ad9517 3>;
>>> clock-names = "adc-clk";
>>>
>>> converters = <&cf_ad9467_core_0>;
>>> };
>>>
>>> cf_ad9467_core_0: cf-ad9467-core-lpc@44a00000 {
>>> compatible = "adi,axi-adc-10.0.a";
>>> reg = <0x44A00000 0x10000>;
>>>
>>> clocks = <&clkc 16>;
>>> };
>>>
>>> Naturally, converter-names only makes sense when you have more than one backend.
>>> But
>>> see that in 'cf_ad9467_core_0', you are free to place a regulator (as I have a
>>> clock)
>>> as long as you handle it in the backend driver.
>>>
>>> - Nuno Sá
>>
>> Thanks for the example. This helped me prototyping a dfsdm driver based
>> on the converter framework. Regarding device tree and driver update this
>> looks fine. I could integrate the API smartly in my frontend (dfsdm) and
>> backend (sd modulator).
>>
>> My prototype executes up to probe. I have noticed however that init
>> (backend & frontend) ops are not called in my implementation. I can see
>> that init ops are called from bind ops. component_bind_all() calls
>
> Note that you need to call converter_frontend_add() from your frontend device (stm32-
> dfsdm-adc) probe function and converter_add() from your backend's probes. And
> ideally, this is the only thing you do at probe. Then, once all the elements are
> probed, the complete aggregate device is initialised and the .bind()/.init() function
> should be called.
>
> And I want to reinforce the above, in the component API, things will only come up
> when all the pieces (all the converters you specified in DT) are probed. The same is
> true if one of the elements is unbound from it's driver - all the other elements in
> the aggregate device will be torn down and converter_frontend_unbind() will be
> called. This means it's an all or nothing solution... Let me know if this does not
> work for you.
>
>> converter bind ops, but component_bind_all() is called from converter
>> bind ops. So, I don't understand how initialization can proceed with
>> these circular calls. Maybe I missed something here.
>>
>
> This one I'm not following... component_bind_all() should be called from
> converter_frontend_bind() and this will call all converter_bind() you have (depends
> on how many backends you have). After all backends are initialized, .frontend_init()
> is called. In there, if you need (most likely you do) an handle to a converter you
> then need to call converter_get(). So, component_bind_all() should not be called from
> converter bind ops but from frontend_component_ops which are the
> component_master_ops. If this is not happening, then we have an issue :)
>
A quick update to my previous feedback:
As you mentioned it in the converter fw serie,
component_compare_fwnode() and component_release_fwnode() patch is not
included. By default I used the component_release_of() and
component_compare_of() from the component API. This was not the best
idea. With a correct compare function the init callbacks are actually
called. So, no real issue here :-)
Olivier
>> The change in the DT has an impact (But moderated) on legacy. Breaking
>> the legacy was unavoidable anyway.
>>
>> DFSDM legacy binding (with two channels)
>> dfsdm_pdm1: filter@1 {
>> compatible = "st,stm32-dfsdm-adc";
>> st,adc-channels = <2 3>;
>> st,adc-channel-types = "SPI_R", "SPI_R";
>> ...
>> io-channels = <&sd_adc2 &sd_adc3>;
>> };
>>
>> DFSDM binding with converter fw
>> dfsdm_pdm1: filter@1 {
>> compatible = "st,stm32-dfsdm-adc";
>> st,adc-channels = <2 3>;
>> st,adc-channel-types = "SPI_R", "SPI_R";
>> ...
>> converters = <&sd_adc2 &sd_adc3>;
>> };
>>
>> I have also the aim to change DFSDM bindings to use IIO generic channels
>> bindings (bindings/iio/adc/adc.yaml).
>>
>> Ideally the DFSDM bindings should looks like this:
>> dfsdm_pdm1: filter@1 {
>> compatible = "st,stm32-dfsdm-adc";
>> channel@2 {
>> reg = <2>;
>> st,adc-channel-types = "SPI_R";
>> ...
>> converters = <&sd_adc2>;
>> };
>> channel@3 {
>> reg = <3>;
>> st,adc-channel-types = "SPI_R";
>> ...
>> converters = <&sd_adc3>;
>> };
>> };
>>
>> But it seems that current framework converter API cannot support this
>> topology.
>>
>
> Indeed this won't work and I honestly it never crossed my mind ehehe,
>
>> As a fallback solution the following binding may be adopted
>> dfsdm_pdm1: filter@1 {
>> compatible = "st,stm32-dfsdm-adc";
>> channel@2 {
>> reg = <2>;
>> st,adc-channel-types = "SPI_R";
>> ...
>> };
>> channel@3 {
>> reg = <3>;
>> st,adc-channel-types = "SPI_R";
>> ...
>> };
>> converters = <&sd_adc2 &sd_adc3>;
>>
>> In this case the frontend driver needs a mean to map backend and
>> channels. It's not the smartest solution, yet. Especially since the use
>> of generic channel is quite common.
>
> Yeah, I'm also not a fan of that... To support the above topology and from the top of
> my head we could either:
>
> 1) Somehow split converter_frontend_add() to give more control to the caller to call
> converter_frontend_add_matches() and in this case have another API that accepts a
> fwnode.
>
> 2) Just extend converter_frontend_add_matches() so that we also look into child nodes
> for 'converters'
>
> Then, on the get() side, we would need something like converters_get_from_fwnode() to
> get each handle. I would likely prefer to go with 2) because 1) already implies some
> FW parsing during probe that I would like to avoid.
>
> Anyways the above is already showing that maybe going with the component API for
> something more generic might be a stretch and harder to scale for everyone needs.
> With an OF lookup, the above topology would be easier to accomplish (though we would
> always need a converters_get_from_fwnode() kind of function).
>
> When you say:
>
> "In this case the frontend driver needs a mean to map backend and channels."
>
> Could 'converter-names' be used for the above? Or would the above be trivial with an
> OF lookup?
>
>> Perhaps the converter_frontend_add() API needs to be extended to support
>> generic channel configuration. Maybe the IIO core should provide the
>> related helpers as well. (As far as I know this does not exists).
>> So, still opened questions ..
>>
>
> No sure what do you mean by the above?
>
>> That said, I feel confident that the converter framework is a good
>> option for the DFSDM use case.
>>
>
> Yeah, I'm also confident that we can get something that suits both our usecases
> either with OF or component. I must say that I'm tempted to send a version of this
> with an OF lookup just so we get a look on how it would look like and compare against
> the component API.
>
> - Nuno Sá
>>>>
^ permalink raw reply [flat|nested] 40+ messages in thread
end of thread, other threads:[~2023-09-28 16:30 UTC | newest]
Thread overview: 40+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-27 15:03 [RFC v2 00/11] iio: add iio backend device type Olivier Moysan
2023-07-27 15:03 ` Olivier Moysan
2023-07-27 15:03 ` [RFC v2 01/11] iio: introduce iio backend device Olivier Moysan
2023-07-28 8:42 ` Nuno Sá
2023-08-31 16:14 ` Olivier MOYSAN
2023-09-01 8:01 ` Nuno Sá
2023-09-03 10:46 ` Jonathan Cameron
2023-09-05 10:06 ` Olivier MOYSAN
2023-09-11 9:39 ` Nuno Sá
2023-09-18 15:52 ` Olivier MOYSAN
2023-09-22 8:53 ` Nuno Sá
2023-09-25 6:48 ` Nuno Sá
2023-09-26 16:44 ` Olivier MOYSAN
2023-09-28 7:15 ` Nuno Sá
2023-09-28 16:30 ` Olivier MOYSAN
2023-07-27 15:03 ` [RFC v2 02/11] of: property: add device link support for io-backends Olivier Moysan
2023-07-27 15:03 ` [RFC v2 03/11] dt-bindings: iio: stm32-dfsdm-adc: add scaling support Olivier Moysan
2023-07-27 15:03 ` Olivier Moysan
2023-08-11 17:10 ` Rob Herring
2023-08-11 17:10 ` Rob Herring
2023-08-31 15:53 ` Olivier MOYSAN
2023-08-31 15:53 ` Olivier MOYSAN
2023-07-27 15:03 ` [RFC v2 04/11] dt-bindings: iio: adc: add scaling support to sd modulator Olivier Moysan
2023-07-27 15:03 ` [RFC v2 05/11] iio: adc: stm32-dfsdm: manage dfsdm as a channel provider Olivier Moysan
2023-07-27 15:03 ` Olivier Moysan
2023-07-27 15:03 ` [RFC v2 06/11] iio: adc: stm32-dfsdm: adopt generic channel bindings Olivier Moysan
2023-07-27 15:03 ` Olivier Moysan
2023-07-27 15:03 ` [RFC v2 07/11] iio: adc: stm32-dfsdm: add scaling support to dfsdm Olivier Moysan
2023-07-27 15:03 ` Olivier Moysan
2023-07-27 22:37 ` kernel test robot
2023-07-27 22:57 ` kernel test robot
2023-07-27 15:03 ` [RFC v2 08/11] iio: adc: sd modulator: add scale and offset support Olivier Moysan
2023-07-27 22:37 ` kernel test robot
2023-07-28 0:41 ` kernel test robot
2023-07-27 15:03 ` [RFC v2 09/11] ARM: dts: stm32: adopt new dfsdm bindings on stm32mp151 Olivier Moysan
2023-07-27 15:03 ` Olivier Moysan
2023-07-27 15:03 ` [RFC v2 10/11] ARM: dts: stm32: add dfsdm pins muxing on stm32mp15 Olivier Moysan
2023-07-27 15:03 ` Olivier Moysan
2023-07-27 15:03 ` [RFC v2 11/11] ARM: dts: stm32: add dfsdm iio support on stm32mp157c-ev Olivier Moysan
2023-07-27 15:03 ` Olivier Moysan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.