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

[Krzysztof Kozlowski]

On 06/10/2026 17:48, Olivier MOYSAN wrote:
>>>
>>> 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

Sure, that's fine.

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

You answered why you need it in DTS. I question why do you need it in
the binding? Do you see a warning?

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


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

Why was this model chosen in that dfsdm? The child has no resources, so
it should never been made a separate node.


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


Unfortunately this leaves impression of incomplete DT or too granular
split of devices to match your driver model.

Best regards,
Krzysztof