Re: [PATCH 01/18] ASoC: dt-bindings: mediatek,mt8365-afe: Add audio afe document

[AngeloGioacchino Del Regno]

Il 28/02/24 10:57, Alexandre Mergnat ha scritto:
> I think I got it.
>
> - mediatek,i2s-shared-clock: will be remove from DT
> - mediatek,dmic-iir-on: will be remove from DT
> - mediatek,dmic-irr-mode: will be remove from DT
> - mediatek,dmic-two-wire-mode: rephrase description needed
>
> I've did abstraction (despite me) that IIR settings are runtime config because the 
> driver implement its usage like a one-time-setup -_-'

Yes but just one more thing I just noticed: `mediatek,dmic-two-wire-mode` - can we
please rename this to `mediatek,dmic-mode` ?

That'd be for consistency check mt6359.yaml and mt6358.txt

  mediatek,dmic-mode:
    $ref: /schemas/types.yaml#/definitions/uint32
    description: |
      Indicates how many data pins are used to transmit two channels of PDM
      signal. 0 means two wires, 1 means one wire. Default value is 0.
    enum:
      - 0 # one wire
      - 1 # two wires

Cheers,
Angelo



> Thanks for the explanations, that help.
>
> Regards,
> Alexandre
>
> On 28/02/2024 08:28, Krzysztof Kozlowski wrote:
> > On 27/02/2024 16:18, Alexandre Mergnat wrote:
> > > > > +    type: boolean
> > > > > +
> > > > > +  mediatek,dmic-iir-on:
> > > > > +    description:
> > > > > +      Boolean which specifies whether the DMIC IIR is enabled.
> > > > > +      If this property is not present the IIR is disabled.
> > > >
> > > > "is enabled" or "enable it"?
> > > >
> > > > You described the desired Linux feature or behavior, not the actual
> > > > hardware. The bindings are about the latter, so instead you need to
> > > > rephrase the property and its description to match actual hardware
> > > > capabilities/features/configuration etc.
> > >
> > > I will rephrase:
> > >
> > > True to enable the Infinite Impulse Response (IIR) filter
> > > on the digital microphone inputs.
> >
> > I still don't know why this is DT-specific. You still tell driver what
> > to do...
> >
> > > > > +    type: boolean
> > > > > +
> > > > > +  mediatek,dmic-irr-mode:
> > > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > > +    description:
> > > > > +      Selects stop band of IIR DC-removal filter.
> > > > > +      0 = Software programmable custom coeff loaded by the driver.
> > > >
> > > > Bindings are for hardware, not drivers. Why is this a property of board DTS?
> > >
> > > Actually this is a hardware feature. Mode 1 t 5 are predefined filters.
> > > Mode 0, the HW will read some "coef filter registers" to setup the
> > > custom filter. the "coef filter regs" are written by the driver.
> > > Currently the coef values are hardcoded in the driver.
> >
> > You don't get the point. Just because you choose some mode it does not
> > mean is hardware feature for DT. Sampling frequency done by hardware is
> > also "hardware feature", but do you put it to DT? No.
> >
> > Explain why this is board-specific, not runtime configuration.
> >
> > > > > +      1 = 5Hz if 48KHz mode.
> > > > > +      2 = 10Hz if 48KHz mode.
> > > > > +      3 = 25Hz if 48KHz mode.
> > > > > +      4 = 50Hz if 48KHz mode.
> > > > > +      5 = 65Hz if 48KHz mode.
> > > >
> > > > Use proper unit suffixes - hz.
> > > >
> > > >
> > > > > +    enum:
> > > > > +      - 0
> > > > > +      - 1
> > > > > +      - 2
> > > > > +      - 3
> > > > > +      - 4
> > > > > +      - 5
> > > > > +
> > > > > +  mediatek,dmic-two-wire-mode:
> > > > > +    description:
> > > > > +      Boolean which turns on digital microphone for two wire mode.
> > > > > +      If this property is not present the two wire mode is disabled.
> > > >
> > > > This looks like hardware property, but the naming looks like SW. Again
> > > > you instruct what driver should do. Standard disclaimer:
> > > >
> > > > You described the desired Linux feature or behavior, not the actual
> > > > hardware. The bindings are about the latter, so instead you need to
> > > > rephrase the property and its description to match actual hardware
> > > > capabilities/features/configuration etc.
> > >
> > > Actually this is a hardware feature. This is ALL I have to describe the
> > > HW behavior from the datasheet:
> > > "
> > > bit name: ul_dmic_two_wire_ctl
> > > Turns on digital microphone for two wire mode.
> > > 0: Turn off
> > > 1: Turn on
> >
> > That's rather suggestion it is not a description of hardware but you
> > want driver to do something...
> >
> > > "
> > >
> > > On the board schematic, SoC and DMIC and linked by 3 pins:
> > > - clk
> > > - data0
> > > - data1
> > >
> > > IMHO, "two-wire-mode" means the HW use 2 pins for data, and the SoC must
> > > be aware of that by reading the register value written by the driver,
> > > using the value found in the DTS.
> >
> > So this depends on type of connection of DMIC? Then rephrase description
> > property like this.
> >
> > > I don't get why you think it wouldn't be hardware behavior.
> >
> > Because telling what to write to the registers which is typical sign of
> > people stuffing to DT whatever they need to configure the hardware.
> >
> > > Rephrase description:
> > > "True to enable the two wire mode of the digital microphone"
> > > Is it better ?
> >
> > No, because again you describe some sort of mode. If you insist on such
> > description, then my answer is: it's runtime, so not suitable for DT.
> > Instead describe what is the hardware problem/configuration, e.g. "DMIC
> > is connected with only CLK and DATA0, without third pin" etc.
> >
> > > About the property name, "mediatek,dmic-two-wire-ctl" sound better for you ?
> >
> > To sound more like a register less like physical characteristic of the
> > board? No. The name can stay, I don't have better ideas.
> >
> >
> > Best regards,
> > Krzysztof