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

[Rob Herring <robh@kernel.org>]

On Wed, Oct 07, 2026 at 11:01:28AM +0200, Olivier MOYSAN wrote:
> Hi Conor,
> 
> Thanks for the review
> 
> On 10/1/26 20:32, Conor Dooley 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.
> > > 
> > > 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
> > > @@ -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
> > > +
> > > +  reg:
> > > +    minItems: 1
> > > +    maxItems: 2
> > 
> > This needs an items list here. The size of the regions seems like crap
> > to begin with...
> > 
> > > +
> > > +  clocks:
> > > +    maxItems: 1
> > > +
> > > +  clock-names:
> > > +    description: Internal clock used for MDF digital processing.
> > > +    items:
> > > +      - const: ker_ck
> > 
> > This is pointless when you only have one.
> > 
> 
> I agree that it could be dropped. However, it is useful for using
> devm_regmap_init_mmio_clk.

I don't know what that function is/does, but that's not justification 
for bindings. Maybe you need a helper that handles a single clock. Or 
that function could take a NULL string for single clock? 

> 
> > > +
> > > +  "#clock-cells":
> > > +    enum: [0, 1]
> > 
> > Why is this not fixed? Also why are parts of your own device consuming
> > the clocks?
> > 
> > > +
> > > +  clock-output-names:
> > > +    description: |
> > > +      CCK0 and CCK1 are optional output clocks, which share the same clock frequency,
> > > +      but can be gated independently to save power.
> > > +    minItems: 1
> > > +    maxItems: 2
> > > +    oneOf:
> > > +      - items:
> > > +          - const: cck0
> > > +      - items:
> > > +          - const: cck1
> > > +      - items:
> > > +          - const: cck0
> > > +          - const: cck1
> > > +
> > > +  clock-frequency:
> > > +    description: |
> > > +      Common clock frequency (Hz) for CCK0 and CCK1 output clocks.
> > > +      The frequency must be a multiple of the "ker_ck" clock frequency.
> > > +    maximum: 25000000
> > 
> > Should not be needed, the consumers request what they need.
> > 
> 
> The CCKx clock frequency depends on the maximum rate supported by the sigma
> delta converters (for instance a digital mic) and the expected decimation
> ratio on the bitstream. Typically this determines the frequency on the SPI
> bus.
> This rate is defined statically and shared by the CCKx clocks. So IMHO, as
> this rate is unique, it can look strange to let the consumer define it.
> Moreover, it seems to me that clock-frequency is already used to configure
> the frequency of a provider in some other bindings.
> For instance: Documentation/devicetree/bindings/clock/silabs,si570.yaml
> So, it's not clear for me, what is the restriction on clock-frequency
> property.
> > > +
> > > +  ranges: true
> > > +
> > > +  clock-ranges: true
> > > +
> > > +  resets:
> > > +    maxItems: 1
> > > +
> > > +  reset-names:
> > > +    items:
> > > +      - const: mdf
> > > +
> > > +  access-controllers:
> > > +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > > +    description: |
> > > +      Phandle to the rifsc device to check access right.
> > > +
> > > +  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
> > 
> > No idea what these even are, but this is probably not the right way to
> > represent the relationship between devices. They're apparently ADCs, but
> > this is also an ADC so I'm not sure what's going on here at all.

I don't under it either, but regardless phandle-array needs constraints 
on the items. It's really a matrix with array of phandle+args arrays.

Rob