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

[Alexandre Mergnat <amergnat@baylibre.com>]

On 28/02/2024 11:25, AngeloGioacchino Del Regno wrote:
> 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` ?

Sure, I wasn't aware of this property. I will do it.

Note: the description isn't consistent with the enum comments
"
0 means two wires, 1 means one wire.
...
        - 0 # one wire
        - 1 # two wires
"

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

-- 
Regards,
Alexandre