Re: [PATCH 12/14] dt-bindings: mediatek: mt8196: add audio AFE document

[Krzysztof Kozlowski <krzk@kernel.org>]

On 07/03/2025 13:47, Darren.Ye wrote:
> From: Darren Ye <darren.ye@mediatek.com>
> 
> Add mt8196 audio AFE document.
> 
> Signed-off-by: Darren Ye <darren.ye@mediatek.com>

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters

> ---
>  .../bindings/sound/mediatek,mt8196-afe.yaml   | 259 ++++++++++++++++++
>  1 file changed, 259 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/mediatek,mt8196-afe.yaml
> 
> diff --git a/Documentation/devicetree/bindings/sound/mediatek,mt8196-afe.yaml b/Documentation/devicetree/bindings/sound/mediatek,mt8196-afe.yaml
> new file mode 100644
> index 000000000000..59f8fdf3167c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/mediatek,mt8196-afe.yaml
> @@ -0,0 +1,259 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/mediatek,mt8196-afe.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek Audio Front End PCM controller for MT8196
> +
> +maintainers:
> +  - Darren Ye <darren.ye@mediatek.com>
> +
> +properties:
> +  compatible:
> +    const: mediatek,mt8196-afe-pcm
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#sound-dai-cells":
> +    const: 0
> +
> +  clocks:
> +    items:
> +      - description: audio hopping clock
> +      - description: audio f26m clock
> +      - description: audio ul0 adc clock
> +      - description: audio ul0 adc hires clock
> +      - description: audio ul1 adc clock
> +      - description: audio ul1 adc hires clock
> +      - description: audio apll1 clock
> +      - description: audio apll2 clock
> +      - description: audio apll tuner1 clock
> +      - description: audio apll tuner2 clock
> +      - description: vlp mux audio int
> +      - description: vlp mux aud engen1
> +      - description: vlp mux aud engen2
> +      - description: vlp mux audio h
> +      - description: vlp clock 26m
> +      - description: ck mainpll d4 d4
> +      - description: ck mux aud 1
> +      - description: ck apll1
> +      - description: ck mux aud 2
> +      - description: ck apll2
> +      - description: ck apll1 d4
> +      - description: ck apll2 d4
> +      - description: ck i2sin0 m sel
> +      - description: ck i2sin1 m sel
> +      - description: ck fmi2s m sel
> +      - description: ck tdmout m sel
> +      - description: ck apll12 div i2sin0
> +      - description: ck apll12 div i2sin1
> +      - description: ck apll12 div fmi2s
> +      - description: ck apll12 div tdmout m
> +      - description: ck apll12 div tdmout b
> +      - description: ck adsp sel
> +      - description: ck clock 26m
> +
> +  clock-names:
> +    items:
> +      - const: aud_hopping_clk

Look how other bindings call it. s/_clk//

> +      - const: aud_f26m_clk
> +      - const: aud_ul0_adc_clk
> +      - const: aud_ul0_adc_hires_clk
> +      - const: aud_ul1_adc_clk
> +      - const: aud_ul1_adc_hires_clk
> +      - const: aud_apll1_clk
> +      - const: aud_apll2_clk
> +      - const: aud_apll_tuner1_clk
> +      - const: aud_apll_tuner2_clk
> +      - const: vlp_mux_audio_int
> +      - const: vlp_mux_aud_eng1
> +      - const: vlp_mux_aud_eng2
> +      - const: vlp_mux_audio_h
> +      - const: vlp_clk26m_clk
> +      - const: ck_mainpll_d4_d4

What does ck stand for? You should name and explain the clocks based on
this block, not the source.


> +      - const: ck_mux_aud_1
> +      - const: ck_apll1_ck
> +      - const: ck_mux_aud_2
> +      - const: ck_apll2_ck
> +      - const: ck_apll1_d4
> +      - const: ck_apll2_d4
> +      - const: ck_i2sin0_m_sel
> +      - const: ck_i2sin1_m_sel
> +      - const: ck_fmi2s_m_sel
> +      - const: ck_tdmout_m_sel
> +      - const: ck_apll12_div_i2sin0
> +      - const: ck_apll12_div_i2sin1
> +      - const: ck_apll12_div_fmi2s
> +      - const: ck_apll12_div_tdmout_m
> +      - const: ck_apll12_div_tdmout_b
> +      - const: ck_adsp_sel
> +      - const: ck_clk26m_clk

s/ck//
s/clk// and this goes probably first. Look at other bindings.

> +
> +  interrupts:
> +    maxItems: 1
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  cksys:

Again, open existing bindings.

> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: Phandle to the cksys clock controller.

This tell me not much. Why do you need it?

Drop redundant 'Phandle to' and explain how it is used.

> +
> +  vlpcksys:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: Phandle to the vlpcksys clock controller.

No, because you keep encoding clock information via non-clock API.

> +
> +  memory-region:
> +    $ref: /schemas/types.yaml#/definitions/phandle

Drop, see other bindings.


> +    description: Phandle to the reserved memory region for AFE DMA.
> +
> +  pinctrl-names:

Drop

> +    items:
> +      - const: default
> +
> +  pinctrl-0:

Drop

> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: Phandle to the pin control group for default state.
> +
> +  mediatek,etdm-out-ch:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: Number of ETDM output channels.
> +    enum: [2]

That's pointless.

> +
> +  mediatek,etdm-in-ch:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: Number of ETDM input channels.
> +    enum: [2]
> +
> +  mediatek,etdm-out-sync:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: ETDM output synchronization.
> +    enum: [0, 1]
> +
> +  mediatek,etdm-in-sync:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: ETDM input synchronization.
> +    enum: [0, 1]
> +
> +  mediatek,etdm-ip-mode:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: ETDM IP mode.
> +    enum: [0, 1]

Drop all above properties or explain why they make any sense in the
terms of board configuration.


> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - interrupts
> +  - power-domains
> +  - cksys
> +  - vlpcksys
> +  - memory-region

> +  - pinctrl-names
> +  - pinctrl-0

Why?

> +  - mediatek,etdm-out-ch
> +  - mediatek,etdm-in-ch
> +  - mediatek,etdm-out-sync
> +  - mediatek,etdm-in-sync
> +  - mediatek,etdm-ip-mode
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/mt8196-clk.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/power/mt8196-power.h>
> +
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        afe: mt8196-afe-pcm@1a110000 {

Again... look at other bindings.


Best regards,
Krzysztof