Re: [PATCH] ASoC: dt-bindings: tegra20-ac97: convert to dt schema

[Krzysztof Kozlowski <krzk@kernel.org>]

On 21/04/2024 13:09, Mohammad Shehar Yaar Tausif wrote:
> Convert NVIDIA Tegra20 AC97 binding to DT schema.
> 
> Signed-off-by: Mohammad Shehar Yaar Tausif <sheharyaar48@gmail.com>

Thank you for your patch. There is something to discuss/improve.

> -};
> diff --git a/Documentation/devicetree/bindings/sound/nvidia,tegra20-ac97.yaml b/Documentation/devicetree/bindings/sound/nvidia,tegra20-ac97.yaml
> new file mode 100644
> index 000000000000..e7ffa93eec30
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/nvidia,tegra20-ac97.yaml
> @@ -0,0 +1,80 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/nvidia,tegra20-ac97.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#

All my other comments apply. Please confirm that you tested DTS.

> +
> +title: NVIDIA Tegra20 AC97 controller
> +
> +maintainers:
> +  - Thierry Reding <treding@nvidia.com>
> +  - Jon Hunter <jonathanh@nvidia.com>
> +
> +properties:
> +  compatible:
> +    const: nvidia,tegra20-ac97
> +
> +  reg:
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 1
> +
> +  reset-names:
> +    const: ac97
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    minItems: 1

Nope, this must be maxItems instead.

> +
> +  dmas:
> +    minItems: 2

No, maxItems instead.


> +
> +  dma-names:
> +    items:
> +      - const: rx
> +      - const: tx
> +
> +  nvidia,codec-reset-gpio:


-gpios instead and mention this in commit msg.

> +    description: |

Do not need '|' unless you need to preserve formatting.

> +      The Tegra GPIO controller's phandle and the number
> +      of the GPIO used to reset the external AC97 codec
> +    maxItems: 1
> +
> +  nvidia,codec-sync-gpio:
> +    description: |

Ditto

> +      The Tegra GPIO controller's phandle and the number
> +      of the GPIO corresponding with the AC97 DAP _FS line
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - resets
> +  - reset-names
> +  - interrupts
> +  - clocks
> +  - dmas
> +  - dma-names
> +  - nvidia,codec-reset-gpio
> +  - nvidia,codec-sync-gpio
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    ac97@70002000 {
> +            compatible = "nvidia,tegra20-ac97";
> +            reg = <0x70002000 0x200>;
> +            resets = <&tegra_car 3>;

All previous comments apply...

> +            reset-names = "ac97";
> +            interrupts = <0 81 0x04>;

Use proper defines.

> +            clocks = <&tegra_car 3>;
> +            dmas = <&apbdma 12>, <&apbdma 12>;
> +            dma-names = "rx", "tx";
> +            nvidia,codec-reset-gpio = <&gpio 170 0>;

Include gpio header and use proper defines.



Best regards,
Krzysztof