Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <7effc23b-b61a-9887-3875-d102b8fa270e@linaro.org>
Date:   Wed, 3 Aug 2022 08:29:18 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.12.0
Subject: Re: [PATCH] dt-bindings: sound: tlv320aic3x: Convert to dtschema
Content-Language: en-US
To:     Jai Luthra <j-luthra@ti.com>, Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
Cc:     Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>, alsa-devel@alsa-project.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20220802214811.29033-1-j-luthra@ti.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20220802214811.29033-1-j-luthra@ti.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 02/08/2022 23:48, Jai Luthra wrote:
> Convert bindings for TI's TLV320AIC3x audio codecs to dtschema.
> 
> Signed-off-by: Jai Luthra <j-luthra@ti.com>
> ---


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

> diff --git a/Documentation/devicetree/bindings/sound/tlv320aic3x.yaml b/Documentation/devicetree/bindings/sound/tlv320aic3x.yaml
> new file mode 100644
> index 000000000000..6efb1d459543
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/tlv320aic3x.yaml
> @@ -0,0 +1,145 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/tlv320aic3x.yaml#

Filename with vendor prefix, so ti,tlv320aic3x.yaml

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Texas Instruments TLV320AIC3x Codec Device Tree Bindings

s/Device Tree Bindings//

> +
> +maintainers:
> +  - Jai Luthra <j-luthra@ti.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ti,tlv320aic3x
> +      - ti,tlv320aic33
> +      - ti,tlv320aic3007
> +      - ti,tlv320aic3106
> +      - ti,tlv320aic3104
> +
> +  reg:
> +    maxItems: 1
> +    description: i2c slave address

Skip description.

> +
> +  reset-gpios:
> +    maxItems: 1
> +    description:
> +      GPIO specification for the active low RESET input.
> +
> +  ai3x-gpio-func:
> +    description: AIC3X_GPIO1 & AIC3X_GPIO2 Functionality
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minItems: 3

uint32-array. Old bindings say about two items only. Mention any changes
to binding in cover letter.

> +    maxItems: 3
> +

You lost gpio-reset property. Also not explained in commit msg.

> +  ai3x-micbias-vg:
> +    description: MicBias required voltage. If node is omitted then MicBias is powered down.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    oneOf:
> +      - const: 1
> +        description: MICBIAS output is powered to 2.0V.
> +      - const: 2
> +        description: MICBIAS output is powered to 2.5V.
> +      - const: 3
> +        description: MICBIAS output is connected to AVDD.
> +
> +  ai3x-ocmv:
> +    description: Output Common-Mode Voltage selection.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    oneOf:
> +      - const: 0
> +        description: 1.35V
> +      - const: 1
> +        description: 1.5V
> +      - const: 2
> +        description: 1.65V
> +      - const: 3
> +        description: 1.8V
> +
> +  AVDD-supply:
> +    description: Analog DAC voltage.

New properties?

> +
> +  IOVDD-supply:
> +    description: I/O voltage.
> +
> +  DRVDD-supply:
> +    description: ADC analog and output driver voltage.
> +
> +  DVDD-supply:
> +    description: Digital core voltage.
> +
> +  '#sound-dai-cells':
> +    const: 0
> +
> +required:
> +  - compatible
> +  - reg
> +
> +#The pins can be used in referring sound node's audio-routing property.
> +
> +#CODEC output pins:
> +  # LLOUT
> +  # RLOUT
> +  # MONO_LOUT
> +  # HPLOUT
> +  # HPROUT
> +  # HPLCOM
> +  # HPRCOM
> +
> +#CODEC input pins for TLV320AIC3104:
> +  # MIC2L
> +  # MIC2R
> +  # LINE1L
> +  # LINE1R
> +
> +#CODEC input pins for other compatible codecs:
> +  # MIC3L
> +  # MIC3R
> +  # LINE1L
> +  # LINE2L
> +  # LINE1R
> +  # LINE2R

All this goes to top level description.


Best regards,
Krzysztof