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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

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