Re: [PATCH 1/3] dt-bindings: iio: adc: add bindings for stm32 mdf filter

[Olivier MOYSAN <olivier.moysan@foss.st.com>]

Hi Krzysztof,

Thank you for the review.

On 10/2/26 11:22, Krzysztof Kozlowski wrote:
> On Thu, Oct 01, 2026 at 04:56:45PM +0200, Olivier Moysan wrote:
>> Add bindings that describes STM32 MDF settings to support
>> digital filtering for Pulse Density Modulation (PDM) microphones
>> and analog sigma delta modulators.
> 
> You already received review, so a few things on top to spare you one
> more cycle:
> 
> A nit, subject: drop second/last, redundant "bindings for". The
> "dt-bindings" prefix is already stating that these are bindings.
> See also:
> https://elixir.bootlin.com/linux/v7.1-rc7/source/Documentation/devicetree/bindings/submitting-patches.rst#L23
> 

Done

>>
>> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com>
>> ---
>>   .../bindings/iio/adc/st,stm32-mdf-adc.yaml    | 383 ++++++++++++++++++
>>   1 file changed, 383 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/iio/adc/st,stm32-mdf-adc.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/st,stm32-mdf-adc.yaml b/Documentation/devicetree/bindings/iio/adc/st,stm32-mdf-adc.yaml
>> new file mode 100644
>> index 000000000000..f2fbc3e150e8
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/st,stm32-mdf-adc.yaml
> 
> Filename follows compatible, so st,stm32mp23-mdf
> 

stmp32mp25 is the main SoC, while stm32mp23 is a variant.
file renamed st,stm32mp25-mdf.yaml

>> @@ -0,0 +1,383 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/iio/adc/st,stm32-mdf-adc.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: STMicroelectronics STM32 Multi-function Digital Filter (MDF) ADC
>> +
>> +maintainers:
>> +  - Olivier Moysan <olivier.moysan@foss.st.com>
>> +
>> +description: |
>> +  STM32 MDF ADC is a sigma delta analog-to-digital converter dedicated to
>> +  interface external sigma delta modulators to STM32 micro controllers.
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - st,stm32mp25-mdf
>> +      - st,stm32mp23-mdf
> 
> Why reversed order?
> 

Ok. Reordered alphabetically

>> +  ranges: true
>> +
>> +  clock-ranges: true
> 
> Do you need it here?

clock-ranges property is used to allow the filter child nodes to inherit 
the MDF kernel clock from the parent node.

> 
>> +
>> +  resets:
>> +    maxItems: 1
>> +
>> +  reset-names:
>> +    items:
>> +      - const: mdf
> 
> Drop
> 

reset-names removed.

>> +
>> +  access-controllers:
>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>> +    description: |
>> +      Phandle to the rifsc device to check access right.
> 
> Look at other code how this is done. Don't come with own stuff.
> 

Replaced by:
   access-controllers:
     maxItems: 1

>> +
>> +  power-domains:
>> +    maxItems: 1
>> +
>> +  st,interleave:
>> +    description: |
>> +      List of phandles of interleaved filters. The indexes of interleaved filters must be
>> +      consecutives starting from 0 (i.e in range [0..N]). The samples from interleaved filters
>> +      are muxed in a single channel and retrieved through the device associated to the filter 0.
>> +      The filters 1..N have to be enabled, but inherit their configuration from filter 0.
>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - ranges
>> +  - clocks
>> +  - clock-names
>> +  - clock-ranges
>> +  - "#address-cells"
>> +  - "#size-cells"
>> +
>> +additionalProperties: false
>> +
>> +patternProperties:
> 
> And this has odd order. Please look at example-schema.
> 

ok. Reordered.

>> +  "^sitf@[0-9]+$":
>> +    type: object
>> +    description: Serial interface child node
>> +
>> +    properties:
>> +      compatible:
>> +        enum:
>> +          - st,stm32mp25-sitf-mdf
>> +
>> +      reg:
>> +        description: Specify the SITF serial interface instance
>> +        maxItems: 1
>> +
>> +      clocks:
>> +        description: |
>> +          Serial interface clock (optional depending on interface mode)
>> +        maxItems: 1
>> +
>> +      st,sitf-mode:
>> +        description: |
>> +          Select serial interface protocol
>> +          - spi: SPI mode
>> +          - lf_spi: low frequency SPI mode for low power applications
>> +        $ref: /schemas/types.yaml#/definitions/string
>> +        enum:
>> +          - spi
>> +          - lf_spi
>> +
>> +    required:
>> +      - reg
>> +      - st,sitf-mode
>> +
>> +    additionalProperties: false
>> +
>> +  "^filter@[0-9]+$":
>> +    type: object
>> +    description: Digital filter path child node
>> +
>> +    properties:
>> +      compatible:
>> +        enum:
>> +          - st,stm32mp25-mdf-dmic
>> +          - st,stm32mp25-mdf-adc
>> +
>> +      reg:
>> +        description: Specify the MDF filter instance
>> +        maxItems: 1
>> +
>> +      interrupts:
>> +        maxItems: 1
>> +
>> +      clocks:
>> +        minItems: 1
> 
> Heh? so here min? Is there any logic in your choices of code style?
> 

As the filter node always use the kernel clock from parent node we can 
remove "clocks" item. "clocks" is not relevant here as the filter is not 
supposed to use another reference.
clocks & clock-names removed

>> +        description: Internal clock used for MDF digital processing and control blocks.
>> +
>> +      clock-names:
>> +        items:
>> +          - const: ker_ck
>> +
>> +      dmas:
>> +        maxItems: 1
>> +
>> +      dma-names:
>> +        items:
>> +          - const: rx
>> +
>> +      "#io-channel-cells":
>> +        const: 1
>> +
>> +      '#address-cells':
>> +        const: 1
>> +
>> +      '#size-cells':
>> +        const: 0
>> +
>> +      st,cic-mode:
>> +        description: |
>> +          Cascaded-integrator-comb (CIC) filter configuration
>> +          - 0: MCIC & ACIC filters in FastSinc mode
>> +          - [1-3]: MCIC & ACIC filters in Sinc mode order 1 to 3
>> +          - [4-5]: Single CIC filter in Sinc mode order 4 to 5
>> +          For audio purpose it is recommended to use CIC Sinc4 or Sinc5
>> +          This property is mandatory for filter 0 or filters not used in interleave mode.
>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +        minimum: 0
>> +        maximum: 5
>> +
>> +      st,delay:
>> +        description: Filter delay in samples
>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +        maximum: 127
>> +
>> +      st,rs-filter-bypass:
>> +        description: Bypass RSFLT reshaping filter.
>> +        $ref: /schemas/types.yaml#/definitions/flag
>> +
>> +      st,hpf-filter-cutoff-bp:
>> +        description: |
>> +          High Pass Filter (HPF) cut-off frequency expressed as a fraction of the PCM sampling rate.
>> +          Cut-off frequency = st,hpf-filter-cutoff-bp x Fpcm / 10000.
>> +          If this property is not defined the HPF is disabled.
>> +        enum: [625, 1250, 2500, 9500]
>> +
>> +      st,sync:
>> +        description:
>> +          Synchronize to another filter.
>> +          Must contain the phandle of the filter providing the synchronization.
>> +        allOf:
>> +          - $ref: /schemas/types.yaml#/definitions/phandle-array
>> +          - maxItems: 1
>> +
>> +      st,sitf:
>> +        $ref: /schemas/types.yaml#/definitions/phandle-array
>> +        items:
>> +          - items:
>> +              - description: Phandle of the serial interface connected to the digital filter
>> +              - description: |
>> +                  The phandle's argument selects the bitstream on the falling or rising edge
>> +                  of the serial interface clock:
>> +                  - 0: rising edge
>> +                  - 1: falling edge
>> +                enum: [0, 1]
>> +                default: 0
>> +        description:
>> +          Should be phandle/bitstream pair.
>> +
>> +    required:
>> +      - compatible
>> +      - reg
>> +      - interrupts
>> +      - dmas
>> +      - dma-names
>> +      - "#io-channel-cells"
>> +      - "#address-cells"
>> +      - "#size-cells"
>> +      - st,sitf
>> +
>> +    unevaluatedProperties: false
>> +
>> +    patternProperties:
>> +      "^channel@([0-7])$":
>> +        type: object
>> +        $ref: adc.yaml
>> +        description: Represents the external channel which is connected to the MDF.
>> +
>> +        properties:
>> +          reg:
>> +            maximum: 7
>> +
>> +          io-backends:
>> +            description:
>> +              Used to pipe external sigma delta modulator or internal ADC backend to MDF
>> +              channel.
>> +            maxItems: 1
>> +
>> +        required:
>> +          - reg
>> +
>> +        unevaluatedProperties: false
>> +
>> +    allOf:
>> +      - if:
>> +          properties:
>> +            compatible:
>> +              contains:
>> +                const: st,stm32mp25-mdf-adc
>> +
>> +        then:
>> +          patternProperties:
>> +            "^channel@[0-7]$":
>> +              required:
>> +                - io-backends
>> +
>> +      - if:
>> +          properties:
>> +            compatible:
>> +              contains:
>> +                const: st,stm32mp25-mdf-dmic
>> +
>> +        then:
>> +          patternProperties:
>> +            "^mdf-dai+$":
> 
> This makes no sense. Why is this a pattern and why mdf-daiiiii is
> correct name?
> 

"^mdf-dai$" is intended here

> Not mentioning that your are not supposed to define properties in if
> block (do you see any code like that?). Mixing addressable and
> non-addressable children is another odd thing.
> 

This binding is inspired by the one already adopted for the DFSDM
https://www.kernel.org/doc/Documentation/devicetree/bindings/iio/adc/st,stm32-dfsdm-adc.yaml

I assume can move the mdf-dai node definition outside the conditional 
branch easily.
However, it seems to me more complicated to avoid mixing addressable and 
non-addressable nodes here. Can we keep this binding aligned with the 
DFSDM model? Or would you have another suggestion?

> This entire schema is quite chaotic and overcomplicated.
> 
>> +              type: object
>> +              description: child node
>> +
>> +              properties:
>> +                compatible:
>> +                  enum:
>> +                    - st,stm32mp25-mdf-dai
>> +
>> +                "#sound-dai-cells":
>> +                  const: 0
>> +
>> +                io-channels:
>> +                  description:
>> +                    From common IIO binding. Used to pipe external sigma delta
>> +                    modulator or internal ADC output to MDF channel.
>> +
>> +                power-domains:
>> +                  maxItems: 1
>> +
>> +                port:
>> +                  $ref: /schemas/sound/audio-graph-port.yaml#
>> +                  unevaluatedProperties: false
>> +
>> +              required:
>> +                - compatible
>> +                - "#sound-dai-cells"
>> +                - io-channels
>> +
>> +              additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/st,stm32mp25-rcc.h>
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +    mdf1: mdf@504d0000 {
> 
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> If you cannot find a name matching your device, please check in kernel
> sources for similar cases or you can grow the spec (via pull request to
> DT spec repo).
> 
> And drop unused labels.
> 

The MDF is a digital filter for sigma-delta bitstreams, rather than the 
analog-to-digital converter itself. So "adc" would not be adapted. I did 
not find "filter", that probably would be the more relevant generic name.
The closest similar case is the DFSDM peripheral, which already uses a 
specific naming:
     dfsdm: dfsdm@4400d000 { ...
https://www.kernel.org/doc/Documentation/devicetree/bindings/iio/adc/st,stm32-dfsdm-adc.yaml

What is your recommendation: keep the naming "mdf" or make a pull 
request to add "filter" or another more appropriate name ?

>> +      compatible = "st,stm32mp25-mdf";
>> +      ranges = <0 0x504d0000 0x1000>;
>> +      reg = <0x504d0000 0x8>, <0x504d0ff0 0x10>;
> 
> Address ranges of 2 and 4 words?
> 

These two sections correspond to MDF common registers managed by the core
- Control registers: 2 x 32 bits registers
- Identification registers: 4 x 32 bits registers
The other registers are managed by filter and serial interface driver

> Best regards,
> Krzysztof
> 

Best regards
Olivier