* [PATCH 4/8] dt-bindings: iio: dfsdm: move to backend framework
2024-06-18 16:08 [PATCH 0/8] iio: adc: dfsdm: add scaling support Olivier Moysan
@ 2024-06-18 16:08 ` Olivier Moysan
2024-06-18 18:10 ` Conor Dooley
2024-06-23 15:01 ` Jonathan Cameron
2024-06-18 16:08 ` [PATCH 6/8] iio: adc: stm32-dfsdm: adopt generic channels bindings Olivier Moysan
2024-06-18 16:08 ` [PATCH 8/8] iio: adc: stm32-dfsdm: add scaling support to dfsdm Olivier Moysan
2 siblings, 2 replies; 13+ messages in thread
From: Olivier Moysan @ 2024-06-18 16:08 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
Change the DFSDM binding to use the new IIO backend framework,
along with the adoption of IIO generic channels.
This binding change allows to add scaling support to the DFSDM.
Keep the legacy binding as deprecated for backward compatibility.
The io-backends property is supported only in generic IIO channel
binding.
- Channel description with the generic binding (Audio and Analog):
Properties supersed by generic properties:
st,adc-channels: becomes "reg" property in channel node
st,adc-channel-names: becomes "label" property in channel node
Properties moved to channel child node:
st,adc-channel-types, st,adc-channel-clk-src, st,adc-alt-channel
- Analog binding:
DFSDM filter channel is configured as an IIO backend consumer.
Add io-backends property in channel child nodes.
DFSDM is no more configured as a channel consumer from SD modulator.
Use of io-channels in DFSDM node is deprecated.
- Audio binding:
DFSDM audio DAI is configured as a channel consumer from DFSDM filter.
No change compare to legacy.
Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
---
.../bindings/iio/adc/st,stm32-dfsdm-adc.yaml | 158 +++++++++++++++++-
1 file changed, 152 insertions(+), 6 deletions(-)
diff --git a/Documentation/devicetree/bindings/iio/adc/st,stm32-dfsdm-adc.yaml b/Documentation/devicetree/bindings/iio/adc/st,stm32-dfsdm-adc.yaml
index c1b1324fa132..dd414bab74c1 100644
--- a/Documentation/devicetree/bindings/iio/adc/st,stm32-dfsdm-adc.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/st,stm32-dfsdm-adc.yaml
@@ -102,9 +102,11 @@ patternProperties:
items:
minimum: 0
maximum: 7
+ deprecated: true
st,adc-channel-names:
description: List of single-ended channel names.
+ deprecated: true
st,filter-order:
description: |
@@ -118,6 +120,12 @@ patternProperties:
"#io-channel-cells":
const: 1
+ '#address-cells':
+ const: 1
+
+ '#size-cells':
+ const: 0
+
st,adc-channel-types:
description: |
Single-ended channel input type.
@@ -128,6 +136,7 @@ patternProperties:
items:
enum: [ SPI_R, SPI_F, MANCH_R, MANCH_F ]
$ref: /schemas/types.yaml#/definitions/non-unique-string-array
+ deprecated: true
st,adc-channel-clk-src:
description: |
@@ -139,6 +148,7 @@ patternProperties:
items:
enum: [ CLKIN, CLKOUT, CLKOUT_F, CLKOUT_R ]
$ref: /schemas/types.yaml#/definitions/non-unique-string-array
+ deprecated: true
st,adc-alt-channel:
description:
@@ -147,6 +157,7 @@ patternProperties:
If not set, channel n is connected to SPI input n.
If set, channel n is connected to SPI input n + 1.
type: boolean
+ deprecated: true
st,filter0-sync:
description:
@@ -165,11 +176,65 @@ patternProperties:
- compatible
- reg
- interrupts
- - st,adc-channels
- - st,adc-channel-names
- st,filter-order
- "#io-channel-cells"
+ patternProperties:
+ "^channel@([0-9]|1[0-9])$":
+ type: object
+ $ref: adc.yaml
+ description: Represents the external channels which are connected to the DFSDM.
+
+ properties:
+ reg:
+ items:
+ minimum: 0
+ maximum: 8
+
+ label:
+ description:
+ Unique name to identify which channel this is.
+
+ st,adc-channel-types:
+ description: |
+ Single-ended channel input type.
+ - "SPI_R": SPI with data on rising edge (default)
+ - "SPI_F": SPI with data on falling edge
+ - "MANCH_R": manchester codec, rising edge = logic 0, falling edge = logic 1
+ - "MANCH_F": manchester codec, rising edge = logic 1, falling edge = logic 0
+ items:
+ enum: [ SPI_R, SPI_F, MANCH_R, MANCH_F ]
+ $ref: /schemas/types.yaml#/definitions/non-unique-string-array
+
+ st,adc-channel-clk-src:
+ description: |
+ Conversion clock source.
+ - "CLKIN": external SPI clock (CLKIN x)
+ - "CLKOUT": internal SPI clock (CLKOUT) (default)
+ - "CLKOUT_F": internal SPI clock divided by 2 (falling edge).
+ - "CLKOUT_R": internal SPI clock divided by 2 (rising edge).
+ items:
+ enum: [ CLKIN, CLKOUT, CLKOUT_F, CLKOUT_R ]
+ $ref: /schemas/types.yaml#/definitions/non-unique-string-array
+
+ st,adc-alt-channel:
+ description:
+ Must be defined if two sigma delta modulators are
+ connected on same SPI input.
+ If not set, channel n is connected to SPI input n.
+ If set, channel n is connected to SPI input n + 1.
+ type: boolean
+
+ io-backends:
+ description:
+ From common IIO binding. Used to pipe external sigma delta
+ modulator or internal ADC backend to DFSDM channel.
+
+ required:
+ - reg
+
+ additionalProperties: false
+
allOf:
- if:
properties:
@@ -199,9 +264,19 @@ patternProperties:
description:
From common IIO binding. Used to pipe external sigma delta
modulator or internal ADC output to DFSDM channel.
+ deprecated: true
- required:
- - io-channels
+ if:
+ required:
+ - st,adc-channels
+ then:
+ required:
+ - io-channels
+
+ patternProperties:
+ "^channel@([0-9]|1[0-9])$":
+ required:
+ - io-backends
- if:
properties:
@@ -294,7 +369,77 @@ examples:
#address-cells = <1>;
#size-cells = <0>;
+ // Example 1: Audio use case with generic binding
dfsdm0: filter@0 {
+ compatible = "st,stm32-dfsdm-dmic";
+ reg = <0>;
+ interrupts = <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
+ dmas = <&dmamux1 101 0x400 0x01>;
+ dma-names = "rx";
+ #io-channel-cells = <1>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ st,filter-order = <5>;
+
+ channel@1 {
+ reg = <1>;
+ label = "dmic0";
+ st,adc-channel-types = "SPI_R";
+ st,adc-channel-clk-src = "CLKOUT";
+ st,adc-alt-channel;
+ };
+
+ asoc_pdm0: dfsdm-dai {
+ compatible = "st,stm32h7-dfsdm-dai";
+ #sound-dai-cells = <0>;
+ io-channels = <&dfsdm0 0>;
+ };
+ };
+
+ // Example 1: Analog use case with generic binding
+ dfsdm1: filter@1 {
+ compatible = "st,stm32-dfsdm-adc";
+ reg = <1>;
+ interrupts = <GIC_SPI 111 IRQ_TYPE_LEVEL_HIGH>;
+ dmas = <&dmamux1 102 0x400 0x01>;
+ dma-names = "rx";
+ st,filter-order = <1>;
+ #io-channel-cells = <1>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ channel@2 {
+ reg = <2>;
+ label = "in2";
+ st,adc-channel-types = "SPI_F";
+ st,adc-channel-clk-src = "CLKOUT";
+ st,adc-alt-channel;
+ io-backends = <&sd_adc2>;
+ };
+
+ channel@3 {
+ reg = <3>;
+ label = "in3";
+ st,adc-channel-types = "SPI_R";
+ st,adc-channel-clk-src = "CLKOUT";
+ io-backends = <&sd_adc3>;
+ };
+ };
+ };
+
+ - |
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ #include <dt-bindings/clock/stm32mp1-clks.h>
+ dfsdm_2: dfsdm@4400d000 {
+ compatible = "st,stm32mp1-dfsdm";
+ reg = <0x4400d000 0x800>;
+ clocks = <&rcc DFSDM_K>, <&rcc ADFSDM_K>;
+ clock-names = "dfsdm", "audio";
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ // Example 3: Audio use case with legacy binding
+ dfsdm0_2: filter@0 {
compatible = "st,stm32-dfsdm-dmic";
reg = <0>;
interrupts = <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
@@ -307,14 +452,15 @@ examples:
st,adc-channel-clk-src = "CLKOUT";
st,filter-order = <5>;
- asoc_pdm0: dfsdm-dai {
+ asoc_pdm0_2: dfsdm-dai {
compatible = "st,stm32h7-dfsdm-dai";
#sound-dai-cells = <0>;
io-channels = <&dfsdm0 0>;
};
};
- dfsdm_pdm1: filter@1 {
+ // Example 3: Analog use case with legacy binding
+ dfsdm1_2: filter@1 {
compatible = "st,stm32-dfsdm-adc";
reg = <1>;
interrupts = <GIC_SPI 111 IRQ_TYPE_LEVEL_HIGH>;
--
2.25.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 4/8] dt-bindings: iio: dfsdm: move to backend framework
2024-06-18 16:08 ` [PATCH 4/8] dt-bindings: iio: dfsdm: move to backend framework Olivier Moysan
@ 2024-06-18 18:10 ` Conor Dooley
2024-06-20 8:03 ` Olivier MOYSAN
2024-06-23 15:01 ` Jonathan Cameron
1 sibling, 1 reply; 13+ messages in thread
From: Conor Dooley @ 2024-06-18 18:10 UTC (permalink / raw)
To: Olivier Moysan
Cc: Arnaud Pouliquen, Jonathan Cameron, Lars-Peter Clausen,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Maxime Coquelin,
Alexandre Torgue, Fabrice Gasnier, alsa-devel, linux-iio,
devicetree, linux-stm32, linux-arm-kernel, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 6722 bytes --]
On Tue, Jun 18, 2024 at 06:08:30PM +0200, Olivier Moysan wrote:
> Change the DFSDM binding to use the new IIO backend framework,
> along with the adoption of IIO generic channels.
> This binding change allows to add scaling support to the DFSDM.
>
> Keep the legacy binding as deprecated for backward compatibility.
>
> The io-backends property is supported only in generic IIO channel
> binding.
>
> - Channel description with the generic binding (Audio and Analog):
>
> Properties supersed by generic properties:
> st,adc-channels: becomes "reg" property in channel node
> st,adc-channel-names: becomes "label" property in channel node
> Properties moved to channel child node:
> st,adc-channel-types, st,adc-channel-clk-src, st,adc-alt-channel
>
> - Analog binding:
>
> DFSDM filter channel is configured as an IIO backend consumer.
> Add io-backends property in channel child nodes.
>
> DFSDM is no more configured as a channel consumer from SD modulator.
> Use of io-channels in DFSDM node is deprecated.
>
> - Audio binding:
>
> DFSDM audio DAI is configured as a channel consumer from DFSDM filter.
> No change compare to legacy.
>
> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
> ---
> .../bindings/iio/adc/st,stm32-dfsdm-adc.yaml | 158 +++++++++++++++++-
> 1 file changed, 152 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/st,stm32-dfsdm-adc.yaml b/Documentation/devicetree/bindings/iio/adc/st,stm32-dfsdm-adc.yaml
> index c1b1324fa132..dd414bab74c1 100644
> --- a/Documentation/devicetree/bindings/iio/adc/st,stm32-dfsdm-adc.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/st,stm32-dfsdm-adc.yaml
> @@ -102,9 +102,11 @@ patternProperties:
> items:
> minimum: 0
> maximum: 7
> + deprecated: true
>
> st,adc-channel-names:
> description: List of single-ended channel names.
> + deprecated: true
>
> st,filter-order:
> description: |
> @@ -118,6 +120,12 @@ patternProperties:
> "#io-channel-cells":
> const: 1
>
> + '#address-cells':
> + const: 1
> +
> + '#size-cells':
> + const: 0
> +
> st,adc-channel-types:
> description: |
> Single-ended channel input type.
> @@ -128,6 +136,7 @@ patternProperties:
> items:
> enum: [ SPI_R, SPI_F, MANCH_R, MANCH_F ]
> $ref: /schemas/types.yaml#/definitions/non-unique-string-array
> + deprecated: true
>
> st,adc-channel-clk-src:
> description: |
> @@ -139,6 +148,7 @@ patternProperties:
> items:
> enum: [ CLKIN, CLKOUT, CLKOUT_F, CLKOUT_R ]
> $ref: /schemas/types.yaml#/definitions/non-unique-string-array
> + deprecated: true
>
> st,adc-alt-channel:
> description:
> @@ -147,6 +157,7 @@ patternProperties:
> If not set, channel n is connected to SPI input n.
> If set, channel n is connected to SPI input n + 1.
> type: boolean
> + deprecated: true
>
> st,filter0-sync:
> description:
> @@ -165,11 +176,65 @@ patternProperties:
> - compatible
> - reg
> - interrupts
> - - st,adc-channels
> - - st,adc-channel-names
> - st,filter-order
> - "#io-channel-cells"
>
> + patternProperties:
> + "^channel@([0-9]|1[0-9])$":
> + type: object
> + $ref: adc.yaml
> + description: Represents the external channels which are connected to the DFSDM.
> +
> + properties:
> + reg:
> + items:
> + minimum: 0
> + maximum: 8
> +
> + label:
> + description:
> + Unique name to identify which channel this is.
> +
> + st,adc-channel-types:
> + description: |
> + Single-ended channel input type.
> + - "SPI_R": SPI with data on rising edge (default)
> + - "SPI_F": SPI with data on falling edge
> + - "MANCH_R": manchester codec, rising edge = logic 0, falling edge = logic 1
> + - "MANCH_F": manchester codec, rising edge = logic 1, falling edge = logic 0
> + items:
> + enum: [ SPI_R, SPI_F, MANCH_R, MANCH_F ]
> + $ref: /schemas/types.yaml#/definitions/non-unique-string-array
Why is this an array? And why is the property plural? Can a channel have
more than one type?
> +
> + st,adc-channel-clk-src:
> + description: |
> + Conversion clock source.
> + - "CLKIN": external SPI clock (CLKIN x)
> + - "CLKOUT": internal SPI clock (CLKOUT) (default)
> + - "CLKOUT_F": internal SPI clock divided by 2 (falling edge).
> + - "CLKOUT_R": internal SPI clock divided by 2 (rising edge).
> + items:
> + enum: [ CLKIN, CLKOUT, CLKOUT_F, CLKOUT_R ]
> + $ref: /schemas/types.yaml#/definitions/non-unique-string-array
Ditto here, but s/type/clock source/
Thanks,
Conor.
> +
> + st,adc-alt-channel:
> + description:
> + Must be defined if two sigma delta modulators are
> + connected on same SPI input.
> + If not set, channel n is connected to SPI input n.
> + If set, channel n is connected to SPI input n + 1.
> + type: boolean
> +
> + io-backends:
> + description:
> + From common IIO binding.
Drop this from the description.
> Used to pipe external sigma delta
> + modulator or internal ADC backend to DFSDM channel.
> +
> + required:
> + - reg
> +
> + additionalProperties: false
> +
> allOf:
> - if:
> properties:
> @@ -199,9 +264,19 @@ patternProperties:
> description:
> From common IIO binding. Used to pipe external sigma delta
> modulator or internal ADC output to DFSDM channel.
> + deprecated: true
>
> - required:
> - - io-channels
> + if:
> + required:
> + - st,adc-channels
> + then:
> + required:
> + - io-channels
> +
> + patternProperties:
> + "^channel@([0-9]|1[0-9])$":
> + required:
> + - io-backends
Why is this here, rather than with reg above? Only some channels require
a backend?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 4/8] dt-bindings: iio: dfsdm: move to backend framework
2024-06-18 18:10 ` Conor Dooley
@ 2024-06-20 8:03 ` Olivier MOYSAN
2024-06-20 8:51 ` Conor Dooley
0 siblings, 1 reply; 13+ messages in thread
From: Olivier MOYSAN @ 2024-06-20 8:03 UTC (permalink / raw)
To: Conor Dooley
Cc: Arnaud Pouliquen, Jonathan Cameron, Lars-Peter Clausen,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Maxime Coquelin,
Alexandre Torgue, Fabrice Gasnier, alsa-devel, linux-iio,
devicetree, linux-stm32, linux-arm-kernel, linux-kernel
Hi Conor,
On 6/18/24 20:10, Conor Dooley wrote:
> On Tue, Jun 18, 2024 at 06:08:30PM +0200, Olivier Moysan wrote:
>> Change the DFSDM binding to use the new IIO backend framework,
>> along with the adoption of IIO generic channels.
>> This binding change allows to add scaling support to the DFSDM.
>>
>> Keep the legacy binding as deprecated for backward compatibility.
>>
>> The io-backends property is supported only in generic IIO channel
>> binding.
>>
>> - Channel description with the generic binding (Audio and Analog):
>>
>> Properties supersed by generic properties:
>> st,adc-channels: becomes "reg" property in channel node
>> st,adc-channel-names: becomes "label" property in channel node
>> Properties moved to channel child node:
>> st,adc-channel-types, st,adc-channel-clk-src, st,adc-alt-channel
>>
>> - Analog binding:
>>
>> DFSDM filter channel is configured as an IIO backend consumer.
>> Add io-backends property in channel child nodes.
>>
>> DFSDM is no more configured as a channel consumer from SD modulator.
>> Use of io-channels in DFSDM node is deprecated.
>>
>> - Audio binding:
>>
>> DFSDM audio DAI is configured as a channel consumer from DFSDM filter.
>> No change compare to legacy.
>>
>> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
>> ---
>> .../bindings/iio/adc/st,stm32-dfsdm-adc.yaml | 158 +++++++++++++++++-
>> 1 file changed, 152 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/st,stm32-dfsdm-adc.yaml b/Documentation/devicetree/bindings/iio/adc/st,stm32-dfsdm-adc.yaml
>> index c1b1324fa132..dd414bab74c1 100644
>> --- a/Documentation/devicetree/bindings/iio/adc/st,stm32-dfsdm-adc.yaml
>> +++ b/Documentation/devicetree/bindings/iio/adc/st,stm32-dfsdm-adc.yaml
>> @@ -102,9 +102,11 @@ patternProperties:
>> items:
>> minimum: 0
>> maximum: 7
>> + deprecated: true
>>
>> st,adc-channel-names:
>> description: List of single-ended channel names.
>> + deprecated: true
>>
>> st,filter-order:
>> description: |
>> @@ -118,6 +120,12 @@ patternProperties:
>> "#io-channel-cells":
>> const: 1
>>
>> + '#address-cells':
>> + const: 1
>> +
>> + '#size-cells':
>> + const: 0
>> +
>> st,adc-channel-types:
>> description: |
>> Single-ended channel input type.
>> @@ -128,6 +136,7 @@ patternProperties:
>> items:
>> enum: [ SPI_R, SPI_F, MANCH_R, MANCH_F ]
>> $ref: /schemas/types.yaml#/definitions/non-unique-string-array
>> + deprecated: true
>>
>> st,adc-channel-clk-src:
>> description: |
>> @@ -139,6 +148,7 @@ patternProperties:
>> items:
>> enum: [ CLKIN, CLKOUT, CLKOUT_F, CLKOUT_R ]
>> $ref: /schemas/types.yaml#/definitions/non-unique-string-array
>> + deprecated: true
>>
>> st,adc-alt-channel:
>> description:
>> @@ -147,6 +157,7 @@ patternProperties:
>> If not set, channel n is connected to SPI input n.
>> If set, channel n is connected to SPI input n + 1.
>> type: boolean
>> + deprecated: true
>>
>> st,filter0-sync:
>> description:
>> @@ -165,11 +176,65 @@ patternProperties:
>> - compatible
>> - reg
>> - interrupts
>> - - st,adc-channels
>> - - st,adc-channel-names
>> - st,filter-order
>> - "#io-channel-cells"
>>
>> + patternProperties:
>> + "^channel@([0-9]|1[0-9])$":
>> + type: object
>> + $ref: adc.yaml
>> + description: Represents the external channels which are connected to the DFSDM.
>> +
>> + properties:
>> + reg:
>> + items:
>> + minimum: 0
>> + maximum: 8
>> +
>> + label:
>> + description:
>> + Unique name to identify which channel this is.
>> +
>> + st,adc-channel-types:
>> + description: |
>> + Single-ended channel input type.
>> + - "SPI_R": SPI with data on rising edge (default)
>> + - "SPI_F": SPI with data on falling edge
>> + - "MANCH_R": manchester codec, rising edge = logic 0, falling edge = logic 1
>> + - "MANCH_F": manchester codec, rising edge = logic 1, falling edge = logic 0
>> + items:
>> + enum: [ SPI_R, SPI_F, MANCH_R, MANCH_F ]
>> + $ref: /schemas/types.yaml#/definitions/non-unique-string-array
>
> Why is this an array? And why is the property plural? Can a channel have
> more than one type?
>
You are right. In channel node context, this property is no more an
array. It has to be managed as a single string (This is already the case
in the driver). I will change the type in binding and the name, to
reflect this.
>> +
>> + st,adc-channel-clk-src:
>> + description: |
>> + Conversion clock source.
>> + - "CLKIN": external SPI clock (CLKIN x)
>> + - "CLKOUT": internal SPI clock (CLKOUT) (default)
>> + - "CLKOUT_F": internal SPI clock divided by 2 (falling edge).
>> + - "CLKOUT_R": internal SPI clock divided by 2 (rising edge).
>> + items:
>> + enum: [ CLKIN, CLKOUT, CLKOUT_F, CLKOUT_R ]
>> + $ref: /schemas/types.yaml#/definitions/non-unique-string-array
>
> Ditto here, but s/type/clock source/
>
Same here.
> Thanks,
> Conor.
>
>> +
>> + st,adc-alt-channel:
>> + description:
>> + Must be defined if two sigma delta modulators are
>> + connected on same SPI input.
>> + If not set, channel n is connected to SPI input n.
>> + If set, channel n is connected to SPI input n + 1.
>> + type: boolean
>> +
>> + io-backends:
>> + description:
>> + From common IIO binding.
>
> Drop this from the description.
>
>> Used to pipe external sigma delta
>> + modulator or internal ADC backend to DFSDM channel.
>> +
>> + required:
>> + - reg
>> +
>> + additionalProperties: false
>> +
>> allOf:
>> - if:
>> properties:
>> @@ -199,9 +264,19 @@ patternProperties:
>> description:
>> From common IIO binding. Used to pipe external sigma delta
>> modulator or internal ADC output to DFSDM channel.
>> + deprecated: true
>>
>> - required:
>> - - io-channels
>> + if:
>> + required:
>> + - st,adc-channels
>> + then:
>> + required:
>> + - io-channels
>> +
>> + patternProperties:
>> + "^channel@([0-9]|1[0-9])$":
>> + required:
>> + - io-backends
>
> Why is this here, rather than with reg above? Only some channels require
> a backend?
The io-backends property is required only when we use st,stm32-dfsdm-adc
compatible. In other words, when we are in an analog use case. In this
case the channel is a consumer of a backend (typically a sd modulator)
In an audio use case (compatible st,stm32-dfsdm-dmic) the backend is not
required.
BRs
Olivier
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 4/8] dt-bindings: iio: dfsdm: move to backend framework
2024-06-20 8:03 ` Olivier MOYSAN
@ 2024-06-20 8:51 ` Conor Dooley
0 siblings, 0 replies; 13+ messages in thread
From: Conor Dooley @ 2024-06-20 8:51 UTC (permalink / raw)
To: Olivier MOYSAN
Cc: Conor Dooley, Arnaud Pouliquen, Jonathan Cameron,
Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Maxime Coquelin, Alexandre Torgue, Fabrice Gasnier,
alsa-devel, linux-iio, devicetree, linux-stm32, linux-arm-kernel,
linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1639 bytes --]
On Thu, Jun 20, 2024 at 10:03:44AM +0200, Olivier MOYSAN wrote:
> On 6/18/24 20:10, Conor Dooley wrote:
> > On Tue, Jun 18, 2024 at 06:08:30PM +0200, Olivier Moysan wrote:
> > > allOf:
> > > - if:
> > > properties:
> > > @@ -199,9 +264,19 @@ patternProperties:
> > > description:
> > > From common IIO binding. Used to pipe external sigma delta
> > > modulator or internal ADC output to DFSDM channel.
> > > + deprecated: true
> > > - required:
> > > - - io-channels
> > > + if:
> > > + required:
> > > + - st,adc-channels
> > > + then:
> > > + required:
> > > + - io-channels
> > > +
> > > + patternProperties:
> > > + "^channel@([0-9]|1[0-9])$":
> > > + required:
> > > + - io-backends
> >
> > Why is this here, rather than with reg above? Only some channels require
> > a backend?
>
> The io-backends property is required only when we use st,stm32-dfsdm-adc
> compatible. In other words, when we are in an analog use case. In this case
> the channel is a consumer of a backend (typically a sd modulator)
> In an audio use case (compatible st,stm32-dfsdm-dmic) the backend is not
> required.
Ahh, I think the hunks and indent confused me here. What you're doing is
making io-backends required based on the compatible, but what I thought
you were doing was trying to make io-backends required in channels if
st,adc-channels was set.
Thanks for the explanation,
Conor.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 4/8] dt-bindings: iio: dfsdm: move to backend framework
2024-06-18 16:08 ` [PATCH 4/8] dt-bindings: iio: dfsdm: move to backend framework Olivier Moysan
2024-06-18 18:10 ` Conor Dooley
@ 2024-06-23 15:01 ` Jonathan Cameron
1 sibling, 0 replies; 13+ messages in thread
From: Jonathan Cameron @ 2024-06-23 15:01 UTC (permalink / raw)
To: Olivier Moysan
Cc: Arnaud Pouliquen, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Maxime Coquelin,
Alexandre Torgue, Fabrice Gasnier, alsa-devel, linux-iio,
devicetree, linux-stm32, linux-arm-kernel, linux-kernel
On Tue, 18 Jun 2024 18:08:30 +0200
Olivier Moysan <olivier.moysan@foss.st.com> wrote:
> Change the DFSDM binding to use the new IIO backend framework,
> along with the adoption of IIO generic channels.
> This binding change allows to add scaling support to the DFSDM.
>
> Keep the legacy binding as deprecated for backward compatibility.
>
> The io-backends property is supported only in generic IIO channel
> binding.
>
> - Channel description with the generic binding (Audio and Analog):
>
> Properties supersed by generic properties:
superseded
> st,adc-channels: becomes "reg" property in channel node
> st,adc-channel-names: becomes "label" property in channel node
> Properties moved to channel child node:
> st,adc-channel-types, st,adc-channel-clk-src, st,adc-alt-channel
>
> - Analog binding:
>
> DFSDM filter channel is configured as an IIO backend consumer.
> Add io-backends property in channel child nodes.
>
> DFSDM is no more configured as a channel consumer from SD modulator.
> Use of io-channels in DFSDM node is deprecated.
>
> - Audio binding:
>
> DFSDM audio DAI is configured as a channel consumer from DFSDM filter.
> No change compare to legacy.
>
> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 6/8] iio: adc: stm32-dfsdm: adopt generic channels bindings
2024-06-18 16:08 [PATCH 0/8] iio: adc: dfsdm: add scaling support Olivier Moysan
2024-06-18 16:08 ` [PATCH 4/8] dt-bindings: iio: dfsdm: move to backend framework Olivier Moysan
@ 2024-06-18 16:08 ` Olivier Moysan
2024-06-23 15:01 ` Jonathan Cameron
2024-06-18 16:08 ` [PATCH 8/8] iio: adc: stm32-dfsdm: add scaling support to dfsdm Olivier Moysan
2 siblings, 1 reply; 13+ messages in thread
From: Olivier Moysan @ 2024-06-18 16:08 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
Move to generic channels binding to ease new backend framework adoption
and prepare the convergence with MDF IP support on STM32MP2 SoC family.
Legacy binding:
DFSDM is an IIO channel consumer.
SD modulator is an IIO channels provider.
The channel phandles are provided in DT through io-channels property
and channel indexes through st,adc-channels property.
New binding:
DFSDM is an IIO channel provider.
The channel indexes are given by reg property in channel child node.
This new binding is intended to be used with SD modulator IIO backends.
It does not support SD modulator legacy IIO devices.
The st,adc-channels property presence is used to discriminate
between legacy and backend bindings.
The support of the DFSDM legacy channels and SD modulator IIO devices
is kept for backward compatibility.
Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
---
drivers/iio/adc/stm32-dfsdm-adc.c | 208 ++++++++++++++++++++++++------
1 file changed, 171 insertions(+), 37 deletions(-)
diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
index 9a47d2c87f05..69b4764d7cba 100644
--- a/drivers/iio/adc/stm32-dfsdm-adc.c
+++ b/drivers/iio/adc/stm32-dfsdm-adc.c
@@ -666,6 +666,64 @@ static int stm32_dfsdm_channel_parse_of(struct stm32_dfsdm *dfsdm,
return 0;
}
+static int stm32_dfsdm_generic_channel_parse_of(struct stm32_dfsdm *dfsdm,
+ struct iio_dev *indio_dev,
+ struct iio_chan_spec *ch,
+ struct fwnode_handle *node)
+{
+ struct stm32_dfsdm_channel *df_ch;
+ const char *of_str;
+ int ret, val;
+
+ ret = fwnode_property_read_u32(node, "reg", &ch->channel);
+ if (ret < 0) {
+ dev_err(&indio_dev->dev, "Missing channel index %d\n", ret);
+ return ret;
+ }
+
+ if (ch->channel >= dfsdm->num_chs) {
+ dev_err(&indio_dev->dev, " Error bad channel number %d (max = %d)\n",
+ ch->channel, dfsdm->num_chs);
+ return -EINVAL;
+ }
+
+ ret = fwnode_property_read_string(node, "label", &ch->datasheet_name);
+ if (ret < 0) {
+ dev_err(&indio_dev->dev,
+ " Error parsing 'label' for idx %d\n", ch->channel);
+ return ret;
+ }
+
+ df_ch = &dfsdm->ch_list[ch->channel];
+ df_ch->id = ch->channel;
+
+ ret = fwnode_property_read_string(node, "st,adc-channel-types", &of_str);
+ if (!ret) {
+ val = stm32_dfsdm_str2val(of_str, stm32_dfsdm_chan_type);
+ if (val < 0)
+ return val;
+ } else {
+ val = 0;
+ }
+ df_ch->type = val;
+
+ ret = fwnode_property_read_string(node, "st,adc-channel-clk-src", &of_str);
+ if (!ret) {
+ val = stm32_dfsdm_str2val(of_str, stm32_dfsdm_chan_src);
+ if (val < 0)
+ return val;
+ } else {
+ val = 0;
+ }
+ df_ch->src = val;
+
+ ret = fwnode_property_read_u32(node, "st,adc-alt-channel", &df_ch->alt_si);
+ if (ret != -EINVAL)
+ df_ch->alt_si = 0;
+
+ return 0;
+}
+
static ssize_t dfsdm_adc_audio_get_spiclk(struct iio_dev *indio_dev,
uintptr_t priv,
const struct iio_chan_spec *chan,
@@ -1231,7 +1289,8 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
ret = iio_device_claim_direct_mode(indio_dev);
if (ret)
return ret;
- ret = iio_hw_consumer_enable(adc->hwc);
+ if (adc->hwc)
+ ret = iio_hw_consumer_enable(adc->hwc);
if (ret < 0) {
dev_err(&indio_dev->dev,
"%s: IIO enable failed (channel %d)\n",
@@ -1240,7 +1299,8 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
return ret;
}
ret = stm32_dfsdm_single_conv(indio_dev, chan, val);
- iio_hw_consumer_disable(adc->hwc);
+ if (adc->hwc)
+ iio_hw_consumer_disable(adc->hwc);
if (ret < 0) {
dev_err(&indio_dev->dev,
"%s: Conversion failed (channel %d)\n",
@@ -1367,15 +1427,20 @@ static int stm32_dfsdm_dma_request(struct device *dev,
return 0;
}
-static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev,
- struct iio_chan_spec *ch)
+static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev, struct iio_chan_spec *ch,
+ struct fwnode_handle *child)
{
struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
int ret;
- ret = stm32_dfsdm_channel_parse_of(adc->dfsdm, indio_dev, ch);
- if (ret < 0)
+ if (child)
+ ret = stm32_dfsdm_generic_channel_parse_of(adc->dfsdm, indio_dev, ch, child);
+ else /* Legacy binding */
+ ret = stm32_dfsdm_channel_parse_of(adc->dfsdm, indio_dev, ch);
+ if (ret < 0) {
+ dev_err(&indio_dev->dev, "Failed to parse channel\n");
return ret;
+ }
ch->type = IIO_VOLTAGE;
ch->indexed = 1;
@@ -1390,6 +1455,7 @@ static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev,
if (adc->dev_data->type == DFSDM_AUDIO) {
ch->ext_info = dfsdm_adc_audio_ext_info;
+ ch->scan_index = 0;
} else {
ch->scan_type.shift = 8;
}
@@ -1397,8 +1463,51 @@ static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev,
ch->scan_type.realbits = 24;
ch->scan_type.storagebits = 32;
- return stm32_dfsdm_chan_configure(adc->dfsdm,
- &adc->dfsdm->ch_list[ch->channel]);
+ return stm32_dfsdm_chan_configure(adc->dfsdm, &adc->dfsdm->ch_list[ch->channel]);
+}
+
+static int stm32_dfsdm_chan_init(struct iio_dev *indio_dev, struct iio_chan_spec *channels)
+{
+ int num_ch = indio_dev->num_channels;
+ int chan_idx = 0, ret = 0;
+
+ for (chan_idx = 0; chan_idx < num_ch; chan_idx++) {
+ channels[chan_idx].scan_index = chan_idx;
+ ret = stm32_dfsdm_adc_chan_init_one(indio_dev, &channels[chan_idx], NULL);
+ if (ret < 0) {
+ dev_err(&indio_dev->dev, "Channels init failed\n");
+ return ret;
+ }
+ }
+
+ return ret;
+}
+
+static int stm32_dfsdm_generic_chan_init(struct iio_dev *indio_dev, struct iio_chan_spec *channels)
+{
+ struct fwnode_handle *child;
+ int chan_idx = 0, ret;
+
+ device_for_each_child_node(&indio_dev->dev, child) {
+ /* Skip DAI node in DFSDM audio nodes */
+ if (fwnode_property_present(child, "compatible"))
+ continue;
+
+ channels[chan_idx].scan_index = chan_idx;
+ ret = stm32_dfsdm_adc_chan_init_one(indio_dev, &channels[chan_idx], child);
+ if (ret < 0) {
+ dev_err(&indio_dev->dev, "Channels init failed\n");
+ goto err;
+ }
+
+ chan_idx++;
+ }
+ return chan_idx;
+
+err:
+ fwnode_handle_put(child);
+
+ return ret;
}
static int stm32_dfsdm_audio_init(struct device *dev, struct iio_dev *indio_dev)
@@ -1406,15 +1515,26 @@ static int stm32_dfsdm_audio_init(struct device *dev, struct iio_dev *indio_dev)
struct iio_chan_spec *ch;
struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
struct stm32_dfsdm_channel *d_ch;
- int ret;
+ bool legacy = false;
+ int num_ch, ret;
+
+ /* If st,adc-channels is defined legacy binding is used. Else assume generic binding. */
+ num_ch = of_property_count_u32_elems(indio_dev->dev.of_node, "st,adc-channels");
+ if (num_ch == 1)
+ legacy = true;
ch = devm_kzalloc(&indio_dev->dev, sizeof(*ch), GFP_KERNEL);
if (!ch)
return -ENOMEM;
- ch->scan_index = 0;
+ indio_dev->num_channels = 1;
+ indio_dev->channels = ch;
+
+ if (legacy)
+ ret = stm32_dfsdm_chan_init(indio_dev, ch);
+ else
+ ret = stm32_dfsdm_generic_chan_init(indio_dev, ch);
- ret = stm32_dfsdm_adc_chan_init_one(indio_dev, ch);
if (ret < 0) {
dev_err(&indio_dev->dev, "Channels init failed\n");
return ret;
@@ -1425,9 +1545,6 @@ static int stm32_dfsdm_audio_init(struct device *dev, struct iio_dev *indio_dev)
if (d_ch->src != DFSDM_CHANNEL_SPI_CLOCK_EXTERNAL)
adc->spi_freq = adc->dfsdm->spi_master_freq;
- indio_dev->num_channels = 1;
- indio_dev->channels = ch;
-
return stm32_dfsdm_dma_request(dev, indio_dev);
}
@@ -1435,43 +1552,60 @@ static int stm32_dfsdm_adc_init(struct device *dev, struct iio_dev *indio_dev)
{
struct iio_chan_spec *ch;
struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
- int num_ch;
- int ret, chan_idx;
+ int num_ch, ret;
+ bool legacy = false;
adc->oversamp = DFSDM_DEFAULT_OVERSAMPLING;
ret = stm32_dfsdm_compute_all_osrs(indio_dev, adc->oversamp);
if (ret < 0)
return ret;
- num_ch = of_property_count_u32_elems(indio_dev->dev.of_node,
- "st,adc-channels");
- if (num_ch < 0 || num_ch > adc->dfsdm->num_chs) {
- dev_err(&indio_dev->dev, "Bad st,adc-channels\n");
- return num_ch < 0 ? num_ch : -EINVAL;
+ num_ch = device_get_child_node_count(&indio_dev->dev);
+ if (!num_ch) {
+ /* No channels nodes found. Assume legacy binding */
+ num_ch = of_property_count_u32_elems(indio_dev->dev.of_node, "st,adc-channels");
+ if (num_ch < 0) {
+ dev_err(&indio_dev->dev, "Bad st,adc-channels\n");
+ return num_ch;
+ }
+
+ legacy = true;
}
- /* Bind to SD modulator IIO device */
- adc->hwc = devm_iio_hw_consumer_alloc(&indio_dev->dev);
- if (IS_ERR(adc->hwc))
- return -EPROBE_DEFER;
+ if (num_ch > adc->dfsdm->num_chs) {
+ dev_err(&indio_dev->dev, "Number of channel [%d] exceeds [%d]\n",
+ num_ch, adc->dfsdm->num_chs);
+ return -EINVAL;
+ }
+ indio_dev->num_channels = num_ch;
- ch = devm_kcalloc(&indio_dev->dev, num_ch, sizeof(*ch),
- GFP_KERNEL);
- if (!ch)
- return -ENOMEM;
+ if (legacy) {
+ /* Bind to SD modulator IIO device. */
+ adc->hwc = devm_iio_hw_consumer_alloc(&indio_dev->dev);
+ if (IS_ERR(adc->hwc))
+ return -EPROBE_DEFER;
+ } else {
+ /* Generic binding. SD modulator IIO device not used. Use SD modulator backend. */
+ adc->hwc = NULL;
- for (chan_idx = 0; chan_idx < num_ch; chan_idx++) {
- ch[chan_idx].scan_index = chan_idx;
- ret = stm32_dfsdm_adc_chan_init_one(indio_dev, &ch[chan_idx]);
- if (ret < 0) {
- dev_err(&indio_dev->dev, "Channels init failed\n");
- return ret;
- }
+ adc->backend = devm_kzalloc(&indio_dev->dev, sizeof(*adc->backend) * num_ch,
+ GFP_KERNEL);
+ if (!adc->backend)
+ return -ENOMEM;
}
- indio_dev->num_channels = num_ch;
+ ch = devm_kcalloc(&indio_dev->dev, num_ch, sizeof(*ch), GFP_KERNEL);
+ if (!ch)
+ return -ENOMEM;
indio_dev->channels = ch;
+ if (legacy)
+ ret = stm32_dfsdm_chan_init(indio_dev, ch);
+ else
+ ret = stm32_dfsdm_generic_chan_init(indio_dev, ch);
+ if (ret < 0)
+ return ret;
+
init_completion(&adc->completion);
/* Optionally request DMA */
--
2.25.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 6/8] iio: adc: stm32-dfsdm: adopt generic channels bindings
2024-06-18 16:08 ` [PATCH 6/8] iio: adc: stm32-dfsdm: adopt generic channels bindings Olivier Moysan
@ 2024-06-23 15:01 ` Jonathan Cameron
0 siblings, 0 replies; 13+ messages in thread
From: Jonathan Cameron @ 2024-06-23 15:01 UTC (permalink / raw)
To: Olivier Moysan
Cc: Lars-Peter Clausen, Maxime Coquelin, Alexandre Torgue, linux-iio,
linux-stm32, linux-arm-kernel, linux-kernel
On Tue, 18 Jun 2024 18:08:32 +0200
Olivier Moysan <olivier.moysan@foss.st.com> wrote:
> Move to generic channels binding to ease new backend framework adoption
> and prepare the convergence with MDF IP support on STM32MP2 SoC family.
>
> Legacy binding:
> DFSDM is an IIO channel consumer.
> SD modulator is an IIO channels provider.
> The channel phandles are provided in DT through io-channels property
> and channel indexes through st,adc-channels property.
>
> New binding:
> DFSDM is an IIO channel provider.
> The channel indexes are given by reg property in channel child node.
>
> This new binding is intended to be used with SD modulator IIO backends.
> It does not support SD modulator legacy IIO devices.
> The st,adc-channels property presence is used to discriminate
> between legacy and backend bindings.
>
> The support of the DFSDM legacy channels and SD modulator IIO devices
> is kept for backward compatibility.
>
> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
Hi Oliver
A few minor things inline.
Jonathan
> ---
> drivers/iio/adc/stm32-dfsdm-adc.c | 208 ++++++++++++++++++++++++------
> 1 file changed, 171 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
> index 9a47d2c87f05..69b4764d7cba 100644
> --- a/drivers/iio/adc/stm32-dfsdm-adc.c
> +++ b/drivers/iio/adc/stm32-dfsdm-adc.c
> @@ -666,6 +666,64 @@ static int stm32_dfsdm_channel_parse_of(struct stm32_dfsdm *dfsdm,
> return 0;
> }
>
> +static int stm32_dfsdm_generic_channel_parse_of(struct stm32_dfsdm *dfsdm,
> + struct iio_dev *indio_dev,
> + struct iio_chan_spec *ch,
> + struct fwnode_handle *node)
> +{
> + struct stm32_dfsdm_channel *df_ch;
> + const char *of_str;
> + int ret, val;
> +
> + ret = fwnode_property_read_u32(node, "reg", &ch->channel);
> + if (ret < 0) {
> + dev_err(&indio_dev->dev, "Missing channel index %d\n", ret);
> + return ret;
> + }
> +
> + if (ch->channel >= dfsdm->num_chs) {
> + dev_err(&indio_dev->dev, " Error bad channel number %d (max = %d)\n",
> + ch->channel, dfsdm->num_chs);
> + return -EINVAL;
> + }
> +
> + ret = fwnode_property_read_string(node, "label", &ch->datasheet_name);
> + if (ret < 0) {
> + dev_err(&indio_dev->dev,
> + " Error parsing 'label' for idx %d\n", ch->channel);
> + return ret;
> + }
> +
> + df_ch = &dfsdm->ch_list[ch->channel];
> + df_ch->id = ch->channel;
> +
> + ret = fwnode_property_read_string(node, "st,adc-channel-types", &of_str);
> + if (!ret) {
> + val = stm32_dfsdm_str2val(of_str, stm32_dfsdm_chan_type);
> + if (val < 0)
> + return val;
> + } else {
> + val = 0;
Sometimes better to set a default, then override if if the property is read
successfully.
df_ch->type = 0;
if (!fwnode_property_read_string()) {
int val = ...
df_ch->type = val;
}
etc
> + }
> + df_ch->type = val;
> +
> + ret = fwnode_property_read_string(node, "st,adc-channel-clk-src", &of_str);
> + if (!ret) {
> + val = stm32_dfsdm_str2val(of_str, stm32_dfsdm_chan_src);
> + if (val < 0)
> + return val;
> + } else {
> + val = 0;
> + }
> + df_ch->src = val;
> +
> + ret = fwnode_property_read_u32(node, "st,adc-alt-channel", &df_ch->alt_si);
> + if (ret != -EINVAL)
> + df_ch->alt_si = 0;
I'm confused. If it does == EINVAL we just silently carry on with alt_si sort
of undefined. I guess 0?
> +
> + return 0;
> +}
> +
...
> +static int stm32_dfsdm_chan_init(struct iio_dev *indio_dev, struct iio_chan_spec *channels)
> +{
> + int num_ch = indio_dev->num_channels;
> + int chan_idx = 0, ret = 0;
> +
> + for (chan_idx = 0; chan_idx < num_ch; chan_idx++) {
> + channels[chan_idx].scan_index = chan_idx;
> + ret = stm32_dfsdm_adc_chan_init_one(indio_dev, &channels[chan_idx], NULL);
> + if (ret < 0) {
> + dev_err(&indio_dev->dev, "Channels init failed\n");
> + return ret;
> + }
> + }
> +
> + return ret;
return 0; I don't think it's ever positive and can't get here with it negative.
> +}
> +
> +static int stm32_dfsdm_generic_chan_init(struct iio_dev *indio_dev, struct iio_chan_spec *channels)
> +{
> + struct fwnode_handle *child;
> + int chan_idx = 0, ret;
> +
> + device_for_each_child_node(&indio_dev->dev, child) {
device_for_each_child_node_scoped() and direct returns should tidy this up a tiny bit.
> + /* Skip DAI node in DFSDM audio nodes */
> + if (fwnode_property_present(child, "compatible"))
> + continue;
> +
> + channels[chan_idx].scan_index = chan_idx;
> + ret = stm32_dfsdm_adc_chan_init_one(indio_dev, &channels[chan_idx], child);
> + if (ret < 0) {
> + dev_err(&indio_dev->dev, "Channels init failed\n");
> + goto err;
This is consistent with existing code, but would be nice to make extensive
use of dev_err_probe() in this driver and this is a gone place for that.
return dev_err_probe(indio_dev->dev, ret, "...);
> + }
> +
> + chan_idx++;
> + }
> + return chan_idx;
> +
> +err:
> + fwnode_handle_put(child);
> +
> + return ret;
> }
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 8/8] iio: adc: stm32-dfsdm: add scaling support to dfsdm
2024-06-18 16:08 [PATCH 0/8] iio: adc: dfsdm: add scaling support Olivier Moysan
2024-06-18 16:08 ` [PATCH 4/8] dt-bindings: iio: dfsdm: move to backend framework Olivier Moysan
2024-06-18 16:08 ` [PATCH 6/8] iio: adc: stm32-dfsdm: adopt generic channels bindings Olivier Moysan
@ 2024-06-18 16:08 ` Olivier Moysan
2024-06-19 5:47 ` Nuno Sá
2024-06-23 15:21 ` Jonathan Cameron
2 siblings, 2 replies; 13+ messages in thread
From: Olivier Moysan @ 2024-06-18 16:08 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Maxime Coquelin,
Alexandre Torgue
Cc: Olivier Moysan, linux-iio, linux-kernel, linux-stm32,
linux-arm-kernel
Add scaling support to STM32 DFSDM.
Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
---
drivers/iio/adc/Kconfig | 1 +
drivers/iio/adc/stm32-dfsdm-adc.c | 94 ++++++++++++++++++++++++++++++-
2 files changed, 92 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index f3dfdaa80678..858ae8161fa4 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1223,6 +1223,7 @@ config STM32_DFSDM_ADC
select IIO_BUFFER
select IIO_BUFFER_HW_CONSUMER
select IIO_TRIGGERED_BUFFER
+ select IIO_BACKEND
help
Select this option to support ADCSigma delta modulator for
STMicroelectronics STM32 digital filter for sigma delta converter.
diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
index 69b4764d7cba..93bf6035bd6d 100644
--- a/drivers/iio/adc/stm32-dfsdm-adc.c
+++ b/drivers/iio/adc/stm32-dfsdm-adc.c
@@ -9,6 +9,7 @@
#include <linux/dmaengine.h>
#include <linux/dma-mapping.h>
#include <linux/iio/adc/stm32-dfsdm-adc.h>
+#include <linux/iio/backend.h>
#include <linux/iio/buffer.h>
#include <linux/iio/hw-consumer.h>
#include <linux/iio/sysfs.h>
@@ -78,6 +79,7 @@ struct stm32_dfsdm_adc {
/* ADC specific */
unsigned int oversamp;
struct iio_hw_consumer *hwc;
+ struct iio_backend **backend;
struct completion completion;
u32 *buffer;
@@ -672,6 +674,8 @@ static int stm32_dfsdm_generic_channel_parse_of(struct stm32_dfsdm *dfsdm,
struct fwnode_handle *node)
{
struct stm32_dfsdm_channel *df_ch;
+ struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
+ struct iio_backend *backend;
const char *of_str;
int ret, val;
@@ -721,6 +725,14 @@ static int stm32_dfsdm_generic_channel_parse_of(struct stm32_dfsdm *dfsdm,
if (ret != -EINVAL)
df_ch->alt_si = 0;
+ if (adc->dev_data->type == DFSDM_IIO) {
+ backend = devm_iio_backend_subnode_get(&indio_dev->dev, NULL, node);
+ if (IS_ERR(backend))
+ return dev_err_probe(&indio_dev->dev, PTR_ERR(backend),
+ "Failed to get backend\n");
+ adc->backend[ch->scan_index] = backend;
+ }
+
return 0;
}
@@ -1060,7 +1072,7 @@ static int stm32_dfsdm_update_scan_mode(struct iio_dev *indio_dev,
static int stm32_dfsdm_postenable(struct iio_dev *indio_dev)
{
struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
- int ret;
+ int i = 0, ret;
/* Reset adc buffer index */
adc->bufi = 0;
@@ -1071,6 +1083,15 @@ static int stm32_dfsdm_postenable(struct iio_dev *indio_dev)
return ret;
}
+ if (adc->backend) {
+ while (adc->backend[i]) {
+ ret = iio_backend_enable(&indio_dev->dev, adc->backend[i]);
+ if (ret < 0)
+ return ret;
+ i++;
+ }
+ }
+
ret = stm32_dfsdm_start_dfsdm(adc->dfsdm);
if (ret < 0)
goto err_stop_hwc;
@@ -1103,6 +1124,7 @@ static int stm32_dfsdm_postenable(struct iio_dev *indio_dev)
static int stm32_dfsdm_predisable(struct iio_dev *indio_dev)
{
struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
+ int i = 0;
stm32_dfsdm_stop_conv(indio_dev);
@@ -1110,6 +1132,13 @@ static int stm32_dfsdm_predisable(struct iio_dev *indio_dev)
stm32_dfsdm_stop_dfsdm(adc->dfsdm);
+ if (adc->backend) {
+ while (adc->backend[i]) {
+ iio_backend_disable(&indio_dev->dev, adc->backend[i]);
+ i++;
+ }
+ }
+
if (adc->hwc)
iio_hw_consumer_disable(adc->hwc);
@@ -1282,7 +1311,14 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
int *val2, long mask)
{
struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
- int ret;
+
+ struct stm32_dfsdm_filter *fl = &adc->dfsdm->fl_list[adc->fl_id];
+ struct stm32_dfsdm_filter_osr *flo = &fl->flo[fl->fast];
+ u32 max = flo->max << (flo->lshift - chan->scan_type.shift);
+ int ret, idx = chan->scan_index;
+
+ if (flo->lshift < chan->scan_type.shift)
+ max = flo->max >> (chan->scan_type.shift - flo->lshift);
switch (mask) {
case IIO_CHAN_INFO_RAW:
@@ -1291,6 +1327,8 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
return ret;
if (adc->hwc)
ret = iio_hw_consumer_enable(adc->hwc);
+ if (adc->backend[idx])
+ ret = iio_backend_enable(&indio_dev->dev, adc->backend[idx]);
if (ret < 0) {
dev_err(&indio_dev->dev,
"%s: IIO enable failed (channel %d)\n",
@@ -1301,6 +1339,8 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
ret = stm32_dfsdm_single_conv(indio_dev, chan, val);
if (adc->hwc)
iio_hw_consumer_disable(adc->hwc);
+ if (adc->backend[idx])
+ iio_backend_disable(&indio_dev->dev, adc->backend[idx]);
if (ret < 0) {
dev_err(&indio_dev->dev,
"%s: Conversion failed (channel %d)\n",
@@ -1320,6 +1360,45 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
*val = adc->sample_freq;
return IIO_VAL_INT;
+
+ case IIO_CHAN_INFO_SCALE:
+ /*
+ * Scale is expressed in mV.
+ * When fast mode is disabled, actual resolution may be lower
+ * than 2^n, where n=realbits-1.
+ * This leads to underestimating input voltage. To
+ * compensate this deviation, the voltage reference can be
+ * corrected with a factor = realbits resolution / actual max
+ */
+ if (adc->backend[idx]) {
+ iio_backend_read_raw(adc->backend[idx], val, val2, mask);
+
+ *val = div_u64((u64)*val * (u64)BIT(DFSDM_DATA_RES - 1), max);
+ *val2 = chan->scan_type.realbits;
+ if (chan->differential)
+ *val *= 2;
+ }
+ return IIO_VAL_FRACTIONAL_LOG2;
+
+ case IIO_CHAN_INFO_OFFSET:
+ /*
+ * DFSDM output data are in the range [-2^n,2^n],
+ * with n=realbits-1.
+ * - Differential modulator:
+ * Offset correspond to SD modulator offset.
+ * - Single ended modulator:
+ * Input is in [0V,Vref] range, where 0V corresponds to -2^n, and Vref to 2^n.
+ * Add 2^n to offset. (i.e. middle of input range)
+ * offset = offset(sd) * vref / res(sd) * max / vref.
+ */
+ if (adc->backend[idx]) {
+ iio_backend_read_raw(adc->backend[idx], val, val2, mask);
+
+ *val = div_u64((u64)max * *val, BIT(*val2 - 1));
+ if (!chan->differential)
+ *val += max;
+ }
+ return IIO_VAL_INT;
}
return -EINVAL;
@@ -1449,7 +1528,15 @@ static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev, struct iio_c
* IIO_CHAN_INFO_RAW: used to compute regular conversion
* IIO_CHAN_INFO_OVERSAMPLING_RATIO: used to set oversampling
*/
- ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
+ if (child) {
+ ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_SCALE) |
+ BIT(IIO_CHAN_INFO_OFFSET);
+ } else {
+ /* Legacy. Scaling not supported */
+ ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
+ }
+
ch->info_mask_shared_by_all = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) |
BIT(IIO_CHAN_INFO_SAMP_FREQ);
@@ -1816,3 +1903,4 @@ module_platform_driver(stm32_dfsdm_adc_driver);
MODULE_DESCRIPTION("STM32 sigma delta ADC");
MODULE_AUTHOR("Arnaud Pouliquen <arnaud.pouliquen@st.com>");
MODULE_LICENSE("GPL v2");
+MODULE_IMPORT_NS(IIO_BACKEND);
--
2.25.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 8/8] iio: adc: stm32-dfsdm: add scaling support to dfsdm
2024-06-18 16:08 ` [PATCH 8/8] iio: adc: stm32-dfsdm: add scaling support to dfsdm Olivier Moysan
@ 2024-06-19 5:47 ` Nuno Sá
2024-06-20 14:15 ` Olivier MOYSAN
2024-06-23 15:21 ` Jonathan Cameron
1 sibling, 1 reply; 13+ messages in thread
From: Nuno Sá @ 2024-06-19 5:47 UTC (permalink / raw)
To: Olivier Moysan, Jonathan Cameron, Lars-Peter Clausen,
Maxime Coquelin, Alexandre Torgue
Cc: linux-iio, linux-kernel, linux-stm32, linux-arm-kernel
On Tue, 2024-06-18 at 18:08 +0200, Olivier Moysan wrote:
> Add scaling support to STM32 DFSDM.
>
> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
> ---
Just some general comments...
Acked-by: Nuno Sa <nuno.sa@analog.com>
> drivers/iio/adc/Kconfig | 1 +
> drivers/iio/adc/stm32-dfsdm-adc.c | 94 ++++++++++++++++++++++++++++++-
> 2 files changed, 92 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index f3dfdaa80678..858ae8161fa4 100644
>
...
> @@ -1301,6 +1339,8 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
> ret = stm32_dfsdm_single_conv(indio_dev, chan, val);
> if (adc->hwc)
> iio_hw_consumer_disable(adc->hwc);
> + if (adc->backend[idx])
> + iio_backend_disable(&indio_dev->dev, adc->backend[idx]);
> if (ret < 0) {
> dev_err(&indio_dev->dev,
> "%s: Conversion failed (channel %d)\n",
> @@ -1320,6 +1360,45 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
> *val = adc->sample_freq;
>
> return IIO_VAL_INT;
> +
> + case IIO_CHAN_INFO_SCALE:
> + /*
> + * Scale is expressed in mV.
> + * When fast mode is disabled, actual resolution may be lower
> + * than 2^n, where n=realbits-1.
> + * This leads to underestimating input voltage. To
> + * compensate this deviation, the voltage reference can be
> + * corrected with a factor = realbits resolution / actual max
> + */
> + if (adc->backend[idx]) {
> + iio_backend_read_raw(adc->backend[idx], val, val2, mask);
> +
This is something that I've been thinking about since the beginning of backends
support. Basically having the "catch all" read_raw()/write_raw() or more dedicated
interfaces. For example, in your usecase I think it would make more sense to have
dedicated API's like iio_backend_read_scale() and iio_backend_read_offset() as you're
extending that functionality. Calling iio_backend_read_raw() seems a bit weird to me.
OTOH, iio_backend_read_raw() could be useful in cases where frontends call
iio_backend_extend_chan_spec() and may get some functionality they are not aware of.
And so, in the default: statement this "catch all" API could be used.
Anyways, this are really minor (likely pedantic) details and you kind of came first
and made the decision for me. As I don't really have strong feelings about it, I'm
fine with it as-is.
> + *val = div_u64((u64)*val * (u64)BIT(DFSDM_DATA_RES - 1),
> max);
> + *val2 = chan->scan_type.realbits;
> + if (chan->differential)
> + *val *= 2;
> + }
> + return IIO_VAL_FRACTIONAL_LOG2;
> +
> + case IIO_CHAN_INFO_OFFSET:
> + /*
> + * DFSDM output data are in the range [-2^n,2^n],
> + * with n=realbits-1.
> + * - Differential modulator:
> + * Offset correspond to SD modulator offset.
> + * - Single ended modulator:
> + * Input is in [0V,Vref] range, where 0V corresponds to -2^n, and
> Vref to 2^n.
> + * Add 2^n to offset. (i.e. middle of input range)
> + * offset = offset(sd) * vref / res(sd) * max / vref.
> + */
> + if (adc->backend[idx]) {
> + iio_backend_read_raw(adc->backend[idx], val, val2, mask);
> +
> + *val = div_u64((u64)max * *val, BIT(*val2 - 1));
> + if (!chan->differential)
> + *val += max;
> + }
> + return IIO_VAL_INT;
> }
>
> return -EINVAL;
> @@ -1449,7 +1528,15 @@ static int stm32_dfsdm_adc_chan_init_one(struct iio_dev
> *indio_dev, struct iio_c
> * IIO_CHAN_INFO_RAW: used to compute regular conversion
> * IIO_CHAN_INFO_OVERSAMPLING_RATIO: used to set oversampling
> */
> - ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> + if (child) {
> + ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE) |
> + BIT(IIO_CHAN_INFO_OFFSET);
Not sure if the above SCALE and OFFSET are solely properties if the backend. If so,
you could maybe use iio_backend_extend_chan_spec().
- Nuno Sá
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 8/8] iio: adc: stm32-dfsdm: add scaling support to dfsdm
2024-06-19 5:47 ` Nuno Sá
@ 2024-06-20 14:15 ` Olivier MOYSAN
0 siblings, 0 replies; 13+ messages in thread
From: Olivier MOYSAN @ 2024-06-20 14:15 UTC (permalink / raw)
To: Nuno Sá, Jonathan Cameron, Lars-Peter Clausen,
Maxime Coquelin, Alexandre Torgue
Cc: linux-iio, linux-kernel, linux-stm32, linux-arm-kernel
Hi Nuno,
On 6/19/24 07:47, Nuno Sá wrote:
> On Tue, 2024-06-18 at 18:08 +0200, Olivier Moysan wrote:
>> Add scaling support to STM32 DFSDM.
>>
>> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
>> ---
>
> Just some general comments...
>
> Acked-by: Nuno Sa <nuno.sa@analog.com>
>
>> drivers/iio/adc/Kconfig | 1 +
>> drivers/iio/adc/stm32-dfsdm-adc.c | 94 ++++++++++++++++++++++++++++++-
>> 2 files changed, 92 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index f3dfdaa80678..858ae8161fa4 100644
>>
>
> ...
>
>> @@ -1301,6 +1339,8 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
>> ret = stm32_dfsdm_single_conv(indio_dev, chan, val);
>> if (adc->hwc)
>> iio_hw_consumer_disable(adc->hwc);
>> + if (adc->backend[idx])
>> + iio_backend_disable(&indio_dev->dev, adc->backend[idx]);
>> if (ret < 0) {
>> dev_err(&indio_dev->dev,
>> "%s: Conversion failed (channel %d)\n",
>> @@ -1320,6 +1360,45 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
>> *val = adc->sample_freq;
>>
>> return IIO_VAL_INT;
>> +
>> + case IIO_CHAN_INFO_SCALE:
>> + /*
>> + * Scale is expressed in mV.
>> + * When fast mode is disabled, actual resolution may be lower
>> + * than 2^n, where n=realbits-1.
>> + * This leads to underestimating input voltage. To
>> + * compensate this deviation, the voltage reference can be
>> + * corrected with a factor = realbits resolution / actual max
>> + */
>> + if (adc->backend[idx]) {
>> + iio_backend_read_raw(adc->backend[idx], val, val2, mask);
>> +
>
> This is something that I've been thinking about since the beginning of backends
> support. Basically having the "catch all" read_raw()/write_raw() or more dedicated
> interfaces. For example, in your usecase I think it would make more sense to have
> dedicated API's like iio_backend_read_scale() and iio_backend_read_offset() as you're
> extending that functionality. Calling iio_backend_read_raw() seems a bit weird to me.
>
> OTOH, iio_backend_read_raw() could be useful in cases where frontends call
> iio_backend_extend_chan_spec() and may get some functionality they are not aware of.
> And so, in the default: statement this "catch all" API could be used.
>
> Anyways, this are really minor (likely pedantic) details and you kind of came first
> and made the decision for me. As I don't really have strong feelings about it, I'm
> fine with it as-is.
>
My first idea was to stick to IIO read_raw API ('struct iio_chan_spec
const *chan' should at least be added to prototype as you pointed it
out). But I agree that iio_backend_read_raw() is not explicit regarding
the requested info.
I consider using the existing API iio_backend_ext_info_get() for v2, as
you suggested.
BRs
Olivier
>> + *val = div_u64((u64)*val * (u64)BIT(DFSDM_DATA_RES - 1),
>> max);
>> + *val2 = chan->scan_type.realbits;
>> + if (chan->differential)
>> + *val *= 2;
>> + }
>> + return IIO_VAL_FRACTIONAL_LOG2;
>> +
>> + case IIO_CHAN_INFO_OFFSET:
>> + /*
>> + * DFSDM output data are in the range [-2^n,2^n],
>> + * with n=realbits-1.
>> + * - Differential modulator:
>> + * Offset correspond to SD modulator offset.
>> + * - Single ended modulator:
>> + * Input is in [0V,Vref] range, where 0V corresponds to -2^n, and
>> Vref to 2^n.
>> + * Add 2^n to offset. (i.e. middle of input range)
>> + * offset = offset(sd) * vref / res(sd) * max / vref.
>> + */
>> + if (adc->backend[idx]) {
>> + iio_backend_read_raw(adc->backend[idx], val, val2, mask);
>> +
>> + *val = div_u64((u64)max * *val, BIT(*val2 - 1));
>> + if (!chan->differential)
>> + *val += max;
>> + }
>> + return IIO_VAL_INT;
>> }
>>
>> return -EINVAL;
>> @@ -1449,7 +1528,15 @@ static int stm32_dfsdm_adc_chan_init_one(struct iio_dev
>> *indio_dev, struct iio_c
>> * IIO_CHAN_INFO_RAW: used to compute regular conversion
>> * IIO_CHAN_INFO_OVERSAMPLING_RATIO: used to set oversampling
>> */
>> - ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
>> + if (child) {
>> + ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> + BIT(IIO_CHAN_INFO_SCALE) |
>> + BIT(IIO_CHAN_INFO_OFFSET);
>
> Not sure if the above SCALE and OFFSET are solely properties if the backend. If so,
> you could maybe use iio_backend_extend_chan_spec().
>
> - Nuno Sá
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 8/8] iio: adc: stm32-dfsdm: add scaling support to dfsdm
2024-06-18 16:08 ` [PATCH 8/8] iio: adc: stm32-dfsdm: add scaling support to dfsdm Olivier Moysan
2024-06-19 5:47 ` Nuno Sá
@ 2024-06-23 15:21 ` Jonathan Cameron
2024-06-25 9:39 ` Olivier MOYSAN
1 sibling, 1 reply; 13+ messages in thread
From: Jonathan Cameron @ 2024-06-23 15:21 UTC (permalink / raw)
To: Olivier Moysan
Cc: Lars-Peter Clausen, Maxime Coquelin, Alexandre Torgue, linux-iio,
linux-kernel, linux-stm32, linux-arm-kernel
On Tue, 18 Jun 2024 18:08:34 +0200
Olivier Moysan <olivier.moysan@foss.st.com> wrote:
> Add scaling support to STM32 DFSDM.
Perhaps a short description here of how this works? Where does the scale come
from, what assumptions are made etc.
>
> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
Some minor stuff.
> diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
> index 69b4764d7cba..93bf6035bd6d 100644
> --- a/drivers/iio/adc/stm32-dfsdm-adc.c
> +++ b/drivers/iio/adc/stm32-dfsdm-adc.c
urn 0;
> }
>
> @@ -1060,7 +1072,7 @@ static int stm32_dfsdm_update_scan_mode(struct iio_dev *indio_dev,
> static int stm32_dfsdm_postenable(struct iio_dev *indio_dev)
> {
> struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> - int ret;
> + int i = 0, ret;
Don't mix assigned and unassigned variable declarations. Just use a separate line
as this can mean subtle assignment or lack of assignment issues sneak in.
>
> /* Reset adc buffer index */
> adc->bufi = 0;
> @@ -1071,6 +1083,15 @@ static int stm32_dfsdm_postenable(struct iio_dev *indio_dev)
> return ret;
> }
>
> + if (adc->backend) {
> + while (adc->backend[i]) {
Could do similar to the suggestion below.
Mostly I don't like the index variable manipulation.
> + ret = iio_backend_enable(&indio_dev->dev, adc->backend[i]);
> + if (ret < 0)
> + return ret;
> + i++;
> + }
> + }
> +
> ret = stm32_dfsdm_start_dfsdm(adc->dfsdm);
> if (ret < 0)
> goto err_stop_hwc;
> @@ -1103,6 +1124,7 @@ static int stm32_dfsdm_postenable(struct iio_dev *indio_dev)
> static int stm32_dfsdm_predisable(struct iio_dev *indio_dev)
> {
> struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
> + int i = 0;
>
> stm32_dfsdm_stop_conv(indio_dev);
>
> @@ -1110,6 +1132,13 @@ static int stm32_dfsdm_predisable(struct iio_dev *indio_dev)
>
> stm32_dfsdm_stop_dfsdm(adc->dfsdm);
>
> + if (adc->backend) {
> + while (adc->backend[i]) {
> + iio_backend_disable(&indio_dev->dev, adc->backend[i]);
> + i++;
> + }
Something like
struct iio_backend **be = &adc->backend[0];
do {
iio_backend_disable(&indio-dev->dev, be);
} while (be++);
maybe. Up to you.
> + }
> @@ -1320,6 +1360,45 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
> *val = adc->sample_freq;
>
> return IIO_VAL_INT;
> +
> + case IIO_CHAN_INFO_SCALE:
> + /*
> + * Scale is expressed in mV.
> + * When fast mode is disabled, actual resolution may be lower
> + * than 2^n, where n=realbits-1.
As below, use a few more spaces.
> + * This leads to underestimating input voltage. To
> + * compensate this deviation, the voltage reference can be
> + * corrected with a factor = realbits resolution / actual max
> + */
> + if (adc->backend[idx]) {
> + iio_backend_read_raw(adc->backend[idx], val, val2, mask);
> +
> + *val = div_u64((u64)*val * (u64)BIT(DFSDM_DATA_RES - 1), max);
> + *val2 = chan->scan_type.realbits;
> + if (chan->differential)
> + *val *= 2;
> + }
> + return IIO_VAL_FRACTIONAL_LOG2;
> +
> + case IIO_CHAN_INFO_OFFSET:
> + /*
> + * DFSDM output data are in the range [-2^n,2^n],
Use a few more spaces. [-2^2, 2^n]
> + * with n=realbits-1.
n = realbits - 1
Just to keep it closer to the C coding style.
> + * - Differential modulator:
> + * Offset correspond to SD modulator offset.
> + * - Single ended modulator:
> + * Input is in [0V,Vref] range, where 0V corresponds to -2^n, and Vref to 2^n.
Avoid that long line with a suitable line break.
> + * Add 2^n to offset. (i.e. middle of input range)
> + * offset = offset(sd) * vref / res(sd) * max / vref.
> + */
> + if (adc->backend[idx]) {
> + iio_backend_read_raw(adc->backend[idx], val, val2, mask);
> +
> + *val = div_u64((u64)max * *val, BIT(*val2 - 1));
> + if (!chan->differential)
> + *val += max;
> + }
> + return IIO_VAL_INT;
> }
>
> return -EINVAL;
> @@ -1449,7 +1528,15 @@ static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev, struct iio_c
> * IIO_CHAN_INFO_RAW: used to compute regular conversion
> * IIO_CHAN_INFO_OVERSAMPLING_RATIO: used to set oversampling
> */
> - ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> + if (child) {
> + ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE) |
> + BIT(IIO_CHAN_INFO_OFFSET);
Indent looks a little odd. Maybe one more space neede?
> + } else {
> + /* Legacy. Scaling not supported */
> + ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> + }
> +
> ch->info_mask_shared_by_all = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) |
> BIT(IIO_CHAN_INFO_SAMP_FREQ);
>
> @@ -1816,3 +1903,4 @@ module_platform_driver(stm32_dfsdm_adc_driver);
> MODULE_DESCRIPTION("STM32 sigma delta ADC");
> MODULE_AUTHOR("Arnaud Pouliquen <arnaud.pouliquen@st.com>");
> MODULE_LICENSE("GPL v2");
> +MODULE_IMPORT_NS(IIO_BACKEND);
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 8/8] iio: adc: stm32-dfsdm: add scaling support to dfsdm
2024-06-23 15:21 ` Jonathan Cameron
@ 2024-06-25 9:39 ` Olivier MOYSAN
0 siblings, 0 replies; 13+ messages in thread
From: Olivier MOYSAN @ 2024-06-25 9:39 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lars-Peter Clausen, Maxime Coquelin, Alexandre Torgue, linux-iio,
linux-kernel, linux-stm32, linux-arm-kernel
Hi Jonathan,
On 6/23/24 17:21, Jonathan Cameron wrote:
> On Tue, 18 Jun 2024 18:08:34 +0200
> Olivier Moysan <olivier.moysan@foss.st.com> wrote:
>
>> Add scaling support to STM32 DFSDM.
> Perhaps a short description here of how this works? Where does the scale come
> from, what assumptions are made etc.
>>
Sure. Requires more explanations. Done
>> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
>
> Some minor stuff.
>
>> diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
>> index 69b4764d7cba..93bf6035bd6d 100644
>> --- a/drivers/iio/adc/stm32-dfsdm-adc.c
>> +++ b/drivers/iio/adc/stm32-dfsdm-adc.c
> urn 0;
>> }
>>
>> @@ -1060,7 +1072,7 @@ static int stm32_dfsdm_update_scan_mode(struct iio_dev *indio_dev,
>> static int stm32_dfsdm_postenable(struct iio_dev *indio_dev)
>> {
>> struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
>> - int ret;
>> + int i = 0, ret;
>
> Don't mix assigned and unassigned variable declarations. Just use a separate line
> as this can mean subtle assignment or lack of assignment issues sneak in.
>
Ack
>>
>> /* Reset adc buffer index */
>> adc->bufi = 0;
>> @@ -1071,6 +1083,15 @@ static int stm32_dfsdm_postenable(struct iio_dev *indio_dev)
>> return ret;
>> }
>>
>> + if (adc->backend) {
>> + while (adc->backend[i]) {
>
> Could do similar to the suggestion below.
> Mostly I don't like the index variable manipulation.
>
>> + ret = iio_backend_enable(&indio_dev->dev, adc->backend[i]);
>> + if (ret < 0)
>> + return ret;
>> + i++;
>> + }
>> + }
>> +
>> ret = stm32_dfsdm_start_dfsdm(adc->dfsdm);
>> if (ret < 0)
>> goto err_stop_hwc;
>> @@ -1103,6 +1124,7 @@ static int stm32_dfsdm_postenable(struct iio_dev *indio_dev)
>> static int stm32_dfsdm_predisable(struct iio_dev *indio_dev)
>> {
>> struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
>> + int i = 0;
>>
>> stm32_dfsdm_stop_conv(indio_dev);
>>
>> @@ -1110,6 +1132,13 @@ static int stm32_dfsdm_predisable(struct iio_dev *indio_dev)
>>
>> stm32_dfsdm_stop_dfsdm(adc->dfsdm);
>>
>> + if (adc->backend) {
>> + while (adc->backend[i]) {
>> + iio_backend_disable(&indio_dev->dev, adc->backend[i]);
>> + i++;
>> + }
> Something like
> struct iio_backend **be = &adc->backend[0];
> do {
> iio_backend_disable(&indio-dev->dev, be);
> } while (be++);
>
> maybe. Up to you.
>
If you don't mind, I will keep indexes. Pointers here looks more
difficult to read, and pre/post incrementation in the loop condition can
easily introduce bugs, imho.
>
>> + }
>
>> @@ -1320,6 +1360,45 @@ static int stm32_dfsdm_read_raw(struct iio_dev *indio_dev,
>> *val = adc->sample_freq;
>>
>> return IIO_VAL_INT;
>> +
>> + case IIO_CHAN_INFO_SCALE:
>> + /*
>> + * Scale is expressed in mV.
>> + * When fast mode is disabled, actual resolution may be lower
>> + * than 2^n, where n=realbits-1.
>
> As below, use a few more spaces.
>
>> + * This leads to underestimating input voltage. To
>> + * compensate this deviation, the voltage reference can be
>> + * corrected with a factor = realbits resolution / actual max
>> + */
>> + if (adc->backend[idx]) {
>> + iio_backend_read_raw(adc->backend[idx], val, val2, mask);
>> +
>> + *val = div_u64((u64)*val * (u64)BIT(DFSDM_DATA_RES - 1), max);
>> + *val2 = chan->scan_type.realbits;
>> + if (chan->differential)
>> + *val *= 2;
>> + }
>> + return IIO_VAL_FRACTIONAL_LOG2;
>> +
>> + case IIO_CHAN_INFO_OFFSET:
>> + /*
>> + * DFSDM output data are in the range [-2^n,2^n],
> Use a few more spaces. [-2^2, 2^n]
>> + * with n=realbits-1.
> n = realbits - 1
>
> Just to keep it closer to the C coding style.
>
Done
>> + * - Differential modulator:
>> + * Offset correspond to SD modulator offset.
>> + * - Single ended modulator:
>> + * Input is in [0V,Vref] range, where 0V corresponds to -2^n, and Vref to 2^n.
>
> Avoid that long line with a suitable line break.
>
Ack
>> + * Add 2^n to offset. (i.e. middle of input range)
>> + * offset = offset(sd) * vref / res(sd) * max / vref.
>> + */
>> + if (adc->backend[idx]) {
>> + iio_backend_read_raw(adc->backend[idx], val, val2, mask);
>> +
>> + *val = div_u64((u64)max * *val, BIT(*val2 - 1));
>> + if (!chan->differential)
>> + *val += max;
>> + }
>> + return IIO_VAL_INT;
>> }
>>
>> return -EINVAL;
>> @@ -1449,7 +1528,15 @@ static int stm32_dfsdm_adc_chan_init_one(struct iio_dev *indio_dev, struct iio_c
>> * IIO_CHAN_INFO_RAW: used to compute regular conversion
>> * IIO_CHAN_INFO_OVERSAMPLING_RATIO: used to set oversampling
>> */
>> - ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
>> + if (child) {
>> + ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> + BIT(IIO_CHAN_INFO_SCALE) |
>> + BIT(IIO_CHAN_INFO_OFFSET);
>
> Indent looks a little odd. Maybe one more space neede?
>
Ack
>> + } else {
>> + /* Legacy. Scaling not supported */
>> + ch->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
>> + }
>> +
>> ch->info_mask_shared_by_all = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) |
>> BIT(IIO_CHAN_INFO_SAMP_FREQ);
>>
>> @@ -1816,3 +1903,4 @@ module_platform_driver(stm32_dfsdm_adc_driver);
>> MODULE_DESCRIPTION("STM32 sigma delta ADC");
>> MODULE_AUTHOR("Arnaud Pouliquen <arnaud.pouliquen@st.com>");
>> MODULE_LICENSE("GPL v2");
>> +MODULE_IMPORT_NS(IIO_BACKEND);
>
BRs
Olivier
^ permalink raw reply [flat|nested] 13+ messages in thread