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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 27 Feb 2024 10:01:55 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Alexandre Mergnat <amergnat@...libre.com>,
 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

On 26/02/2024 15:01, Alexandre Mergnat wrote:
> Add MT8365 audio front-end bindings
> 
> Signed-off-by: Alexandre Mergnat <amergnat@...libre.com>
> ---
>  .../bindings/sound/mediatek,mt8365-afe.yaml        | 164 +++++++++++++++++++++
>  1 file changed, 164 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/sound/mediatek,mt8365-afe.yaml b/Documentation/devicetree/bindings/sound/mediatek,mt8365-afe.yaml
> new file mode 100644
> index 000000000000..1d7eb2532ad2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/mediatek,mt8365-afe.yaml
> @@ -0,0 +1,164 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/mediatek,mt8365-afe.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek AFE PCM controller for MT8365
> +
> +maintainers:
> +  - Alexandre Mergnat <amergnat@...libre.com>
> +
> +properties:
> +  compatible:
> +    const: mediatek,mt8365-afe-pcm
> +
> +  reg:
> +    maxItems: 2

You must describe the items.

> +
> +  interrupts:
> +    maxItems: 1
> +
> +  mediatek,topckgen:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: The phandle of the mediatek topckgen controller

What for? Don't repeat the property name. Say something useful.

> +
> +  power-domains:
> +    maxItems: 1
> +
> +  "#sound-dai-cells":
> +    const: 0
> +
> +  clocks:
> +    items:
> +      - description: 26M clock
> +      - description: mux for audio clock
> +      - description: audio i2s0 mck
> +      - description: audio i2s1 mck
> +      - description: audio i2s2 mck
> +      - description: audio i2s3 mck
> +      - description: engen 1 clock
> +      - description: engen 2 clock
> +      - description: audio 1 clock
> +      - description: audio 2 clock
> +      - description: mux for i2s0
> +      - description: mux for i2s1
> +      - description: mux for i2s2
> +      - description: mux for i2s3
> +
> +  clock-names:
> +    items:
> +      - const: top_clk26m_clk
> +      - const: top_audio_sel
> +      - const: audio_i2s0_m
> +      - const: audio_i2s1_m
> +      - const: audio_i2s2_m
> +      - const: audio_i2s3_m
> +      - const: engen1
> +      - const: engen2
> +      - const: aud1
> +      - const: aud2
> +      - const: i2s0_m_sel
> +      - const: i2s1_m_sel
> +      - const: i2s2_m_sel
> +      - const: i2s3_m_sel
> +
> +  mediatek,i2s-shared-clock:

Why do you need this property? Linux (and other systems) handle sharing
clocks properly.

> +    description:
> +      i2s modules can share the same external clock pin.
> +      If this property is not present the clock mode is separrate.

Typo

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

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

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


> +    type: boolean
> +
> +

Just one blank line.

> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - power-domains
> +  - mediatek,topckgen
> +  - clocks
> +  - clock-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/mediatek,mt8365-clk.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/power/mediatek,mt8365-power.h>
> +
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        afe@...20000 {

What is AFE?

https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


> +            compatible = "mediatek,mt8365-afe-pcm";


Best regards,
Krzysztof