Message-ID: <66e527af-0253-4565-9822-04ed84e5817c@baylibre.com>
Date: Wed, 28 Feb 2024 10:57:16 +0100
From: Alexandre Mergnat <amergnat@...libre.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
 Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
 Rob Herring <robh+dt@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>, Matthias Brugger
 <matthias.bgg@...il.com>,
 AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
 Lee Jones <lee@...nel.org>, Flora Fu <flora.fu@...iatek.com>,
 Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.com>,
 Sumit Semwal <sumit.semwal@...aro.org>,
 Christian König <christian.koenig@....com>,
 Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>
Cc: linux-sound@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-mediatek@...ts.infradead.org, linux-media@...r.kernel.org,
 dri-devel@...ts.freedesktop.org, linaro-mm-sig@...ts.linaro.org
Subject: Re: [PATCH 01/18] ASoC: dt-bindings: mediatek,mt8365-afe: Add audio
 afe document

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

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
> 

Powered by blists - more mailing lists