Re: [PATCH v2] ASoC: dt-bindings: tegra30-i2s: convert to dt schema

[Krzysztof Kozlowski <krzk@xxxxxxxxxx>]

On 21/04/2024 14:30, Mohammad Shehar Yaar Tausif wrote:
> Convert NVIDIA Tegra30 I2S binding to DT schema.
> 
> Signed-off-by: Mohammad Shehar Yaar Tausif <sheharyaar48@xxxxxxxxx>
> ---
> v1->v2:
> - Removed incorrect item ref definition
> 
> Previous versions:
> v1:
> https://lore.kernel.org/all/20240420175008.140391-1-sheharyaar48@xxxxxxxxx/
> ---
>  .../bindings/sound/nvidia,tegra30-i2s.txt     | 27 --------
>  .../bindings/sound/nvidia,tegra30-i2s.yaml    | 66 +++++++++++++++++++
>  2 files changed, 66 insertions(+), 27 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/sound/nvidia,tegra30-i2s.txt
>  create mode 100644 Documentation/devicetree/bindings/sound/nvidia,tegra30-i2s.yaml
> 
> diff --git a/Documentation/devicetree/bindings/sound/nvidia,tegra30-i2s.txt b/Documentation/devicetree/bindings/sound/nvidia,tegra30-i2s.txt
> deleted file mode 100644
> index 38caa936f6f8..000000000000
> --- a/Documentation/devicetree/bindings/sound/nvidia,tegra30-i2s.txt
> +++ /dev/null
> @@ -1,27 +0,0 @@
> -NVIDIA Tegra30 I2S controller
> -
> -Required properties:
> -- compatible : For Tegra30, must contain "nvidia,tegra30-i2s".  For Tegra124,
> -  must contain "nvidia,tegra124-i2s".  Otherwise, must contain
> -  "nvidia,<chip>-i2s" plus at least one of the above, where <chip> is
> -  tegra114 or tegra132.
> -- reg : Should contain I2S registers location and length
> -- clocks : Must contain one entry, for the module clock.
> -  See ../clocks/clock-bindings.txt for details.
> -- resets : Must contain an entry for each entry in reset-names.
> -  See ../reset/reset.txt for details.
> -- reset-names : Must include the following entries:
> -  - i2s
> -- nvidia,ahub-cif-ids : The list of AHUB CIF IDs for this port, rx (playback)
> -  first, tx (capture) second. See nvidia,tegra30-ahub.txt for values.
> -
> -Example:
> -
> -i2s@70080300 {
> -	compatible = "nvidia,tegra30-i2s";
> -	reg = <0x70080300 0x100>;
> -	nvidia,ahub-cif-ids = <4 4>;
> -	clocks = <&tegra_car 11>;
> -	resets = <&tegra_car 11>;
> -	reset-names = "i2s";
> -};
> diff --git a/Documentation/devicetree/bindings/sound/nvidia,tegra30-i2s.yaml b/Documentation/devicetree/bindings/sound/nvidia,tegra30-i2s.yaml
> new file mode 100644
> index 000000000000..b4cc0b0abfb8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/nvidia,tegra30-i2s.yaml
> @@ -0,0 +1,66 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/nvidia,tegra30-i2s.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NVIDIA Tegra30 I2S controller
> +
> +maintainers:
> +  - Thierry Reding <treding@xxxxxxxxxx>
> +  - Jon Hunter <jonathanh@xxxxxxxxxx>
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - enum:
> +          - nvidia,tegra124-i2s
> +          - nvidia,tegra30-i2s
> +      - items:
> +          - enum:
> +              - nvidia,tegra114-i2s
> +              - nvidia,tegra132-i2s
> +          - const: nvidia,tegra124-i2s
> +          - const: nvidia,tegra30-i2s

That's not what the binding said and not what DTS is saying.

This points to the fact you did not really test this binding on real DTS.

When you convert the binding to DT schema, you *must* test all existing
DTS. Especially that trivial one, like this case armv7 multiplatform boards.

That's a requirement in DT conversion work.


> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 1
> +
> +  reset-names:
> +    const: i2s
> +
> +  nvidia,ahub-cif-ids:
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    description: |
> +      The list of AHUB CIF IDs for this port, rx (playback)
> +      first, tx (capture) second. See nvidia,tegra30-ahub.txt for values.
> +    minItems: 2
> +    maxItems: 2

Better list items with description
items:
 - description:
 - description:

Also provide proper constraints. Looks like RX and TX are max 5.


> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - resets
> +  - reset-names
> +  - nvidia,ahub-cif-ids
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2s@70080300 {
> +            compatible = "nvidia,tegra30-i2s";

Use 4 spaces for example indentation.



Best regards,
Krzysztof