Re: [PATCH v3 10/12] dt-bindings: mediatek: mt8188: add audio afe document

[Trevor Wu (吳文良)]

On Fri, 2022-12-09 at 11:15 +0100, Krzysztof Kozlowski wrote:
> On 08/12/2022 04:31, Trevor Wu wrote:
> > Add mt8188 audio afe document.
> 
> Use subject prefixes matching the subsystem (git log --oneline --
> ...).
> 
> > 
> > Signed-off-by: Trevor Wu <trevor.wu@xxxxxxxxxxxx>
> > ---
> >  .../devicetree/bindings/sound/mt8188-afe.yaml | 196
> > ++++++++++++++++++
> >  1 file changed, 196 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/sound/mt8188-
> > afe.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/sound/mt8188-
> > afe.yaml b/Documentation/devicetree/bindings/sound/mt8188-afe.yaml
> > new file mode 100644
> > index 000000000000..6ab26494d924
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/mt8188-afe.yaml
> 
> 
> This is a friendly reminder during the review process.
> 
> It seems my previous comments were not fully addressed. Maybe my
> feedback got lost between the quotes, maybe you just forgot to apply
> it.
> Please go back to the previous discussion and either implement all
> requested changes or keep discussing them.
> 
> Thank you.
> 
> Comment was about filename matching compatible, so with vendor
> prefix.

Hi Krzysztof,

Thanks for your review first.
I aplogyize for my misunderstanding to your comment.
I just renamed the file name(mt8188-afe-pcm.yaml -> mt8188-afe.yaml),
and I didn't notice vendor prefix should be included.
I will correct the name in v4.

> 
> > @@ -0,0 +1,196 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: 
> > https://urldefense.com/v3/__http://devicetree.org/schemas/sound/mt8188-afe.yaml*__;Iw!!CTRNKA9wMg0ARbw!kiYsaIYR-KZ9KTQq3rOBRr3GeIC2UtnjFMWMKExmd_V7pW75AaEz30DuKv0nifWtMlknW8os7MiJ2pCCVT7ixRMsg7TpXw$ ;
> >  
> > +$schema: 
> > https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!kiYsaIYR-KZ9KTQq3rOBRr3GeIC2UtnjFMWMKExmd_V7pW75AaEz30DuKv0nifWtMlknW8os7MiJ2pCCVT7ixRO-2YqhFw$ ;
> >  
> > +
> > +title: MediaTek AFE PCM controller for mt8188
> > +
> > +maintainers:
> > +  - Trevor Wu <trevor.wu@xxxxxxxxxxxx>
> > +
> > +properties:
> > +  compatible:
> > +    const: mediatek,mt8188-afe
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  resets:
> > +    maxItems: 1
> > +
> > +  reset-names:
> > +    const: audiosys
> > +
> > +  mediatek,topckgen:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: The phandle of the mediatek topckgen controller
> > +
> > +  power-domains:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    items:
> > +      - description: 26M clock
> > +      - description: audio pll1 clock
> > +      - description: audio pll2 clock
> > +      - description: clock divider for i2si1_mck
> > +      - description: clock divider for i2si2_mck
> > +      - description: clock divider for i2so1_mck
> > +      - description: clock divider for i2so2_mck
> > +      - description: clock divider for dptx_mck
> > +      - description: a1sys hoping clock
> > +      - description: audio intbus clock
> > +      - description: audio hires clock
> > +      - description: audio local bus clock
> > +      - description: mux for dptx_mck
> > +      - description: mux for i2so1_mck
> > +      - description: mux for i2so2_mck
> > +      - description: mux for i2si1_mck
> > +      - description: mux for i2si2_mck
> > +      - description: audio 26m clock
> > +
> > +  clock-names:
> > +    items:
> > +      - const: clk26m
> > +      - const: apll1_ck
> > +      - const: apll2_ck
> > +      - const: apll12_div0
> > +      - const: apll12_div1
> > +      - const: apll12_div2
> > +      - const: apll12_div3
> > +      - const: apll12_div9
> > +      - const: a1sys_hp_sel
> > +      - const: aud_intbus_sel
> > +      - const: audio_h_sel
> > +      - const: audio_local_bus_sel
> > +      - const: dptx_m_sel
> > +      - const: i2so1_m_sel
> > +      - const: i2so2_m_sel
> > +      - const: i2si1_m_sel
> > +      - const: i2si2_m_sel
> > +      - const: adsp_audio_26m
> > +
> > +patternProperties:
> > +  "^mediatek,etdm-in[1-2]-chn-disabled$":
> > +    $ref: /schemas/types.yaml#/definitions/uint8-array
> > +    minItems: 1
> > +    maxItems: 16
> > +    description:
> > +      By default, all data received from ETDM pins will be
> > outputed to
> > +      memory. etdm in supports disable_out in direct mode(w/o
> > interconn).
> > +      User can specify the channel ID which they hope dropping and
> > then
> > +      the specified channel won't be seen on memory.
> 
> So we know what are the IDs but it's a mystery what are the values.
> Especially with unique values - how any of these should case that
> channel "won't be seen in memory"?
> 
For example,
FE can support 14ch, but BE(etdm) can't support 14ch(it can support
16ch).
In the case, we can configure 16ch to etdm and make use of the property
to disable the last two channels.

mediatek,etdm-in1-chn-disabled = /bits/ 8 <0xE 0xF>;


> > +    uniqueItems: true
> > +    items:
> > +      minimum: 0
> > +      maximum: 15
> > +
> > +  "^mediatek,etdm-in[1-2]-mclk-always-on-rate-hz$":
> > +    description: Specify etdm in mclk output rate for always on
> > case.
> 
> How is it different than assigned-clock-rates?
> 
This includes clock enabling at init stage.

> > +
> > +  "^mediatek,etdm-out[1-3]-mclk-always-on-rate-hz$":
> > +    description: Specify etdm out mclk output rate for always on
> > case.
> > +
> > +  "^mediatek,etdm-in[1-2]-multi-pin-mode$":
> > +    type: boolean
> > +    description: if present, the etdm data mode is I2S.
> > +
> > +  "^mediatek,etdm-out[1-3]-multi-pin-mode$":
> > +    type: boolean
> > +    description: if present, the etdm data mode is I2S.
> > +
> > +  "^mediatek,etdm-in[1-2]-cowork-source$":
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description:
> > +      etdm modules can share the same external clock pin. Specify
> > +      which etdm clock source is required by this etdm in moudule.
> 
> typo: module
> 
Thanks, I will correct it in v4.

> > +    enum:
> > +      - 0 # etdm1_in
> > +      - 1 # etdm2_in
> > +      - 2 # etdm1_out
> 
> I don't get. This suggests that etdm1_out can be clock source of
> etdm-in1. Or etdm1_in can be clock source of etdm-in1... It does not
> make sense...
> 
You are correct.
etdm1_in should only choose other etdm as its clock source when user
needs this property, so etdm1_in should not be included in the enum
items for etdm-in1-cowork-source.
I will separate these properties in v4.

Thanks,
Trevor

> > +      - 3 # etdm2_out
> > +
> > +  "^mediatek,etdm-out[1-2]-cowork-source$":
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: |
> > +      etdm modules can share the same external clock pin. Specify
> > +      which etdm clock source is required by this etdm out
> > moudule.
> > +    enum:
> > +      - 0 # etdm1_in
> > +      - 1 # etdm2_in
> > +      - 2 # etdm1_out
> > +      - 3 # etdm2_out
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - resets
> > +  - reset-names
> > +  - mediatek,topckgen
> > +  - power-domains
> > +  - clocks
> > +  - clock-names
> > +
> 
> Best regards,
> Krzysztof
>