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;
Date:   Thu, 9 Jun 2022 19:07:29 +0200
From:   Stephan Gerhold <stephan@gerhold.net>
To:     Alexander Martinz <amartinz@shiftphones.com>
Cc:     ~postmarketos/upstreaming@lists.sr.ht, phone-devel@vger.kernel.org,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Dylan Van Assche <me@dylanvanassche.be>
Subject: Re: [PATCH 2/2] arm64: dts: qcom/sdm845-shift-axolotl: Add audio
 support
Message-ID: <YqIo0TJHqCx33NLj@gerhold.net>
References: <20220609095412.211060-1-amartinz@shiftphones.com>
 <20220609095412.211060-2-amartinz@shiftphones.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220609095412.211060-2-amartinz@shiftphones.com>
Precedence: bulk

On Thu, Jun 09, 2022 at 11:54:12AM +0200, Alexander Martinz wrote:
> This patch adds audio support for the SHIFT6mq phone.
> 
> The primary microphone and headphone jack are handled by the
> SDM845 sound card and WCD9340 codec.
> 
> The primary speaker needs to go through the TFA9890 speaker
> amplifier.
> 
> Signed-off-by: Alexander Martinz <amartinz@shiftphones.com>
> Tested-by: Dylan Van Assche <me@dylanvanassche.be>
> ---
>  .../boot/dts/qcom/sdm845-shift-axolotl.dts    | 141 ++++++++++++++++++
>  1 file changed, 141 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts b/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts
> index fa72f23ef0c2..8c4967d6d0e3 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts
[...]
> @@ -492,6 +494,19 @@ touchscreen@38 {
>  	};
>  };
>  
> +&i2c11 {
> +	status = "okay";
> +	clock-frequency = <400000>;
> +
> +	tfa9890_codec: tfa9890@34 {

Please use "audio-codec@" as suggested in the bindings. Also, I think a
label such as "speaker_codec: " would make it more obvious that this is
the speaker. But that's personal preference. :)

> +		compatible = "nxp,tfa9890";
> +		reg = <0x34>;
> +		vddd-supply = <&vreg_s4a_1p8>;
> +		reset-gpio = <&tlmm 7 0>;

The mainline driver does not support the reset-gpio (it resets via the
I2C registers instead). Can just drop this.

Alternatively we could document it to the DT schema and describe it here
for completeness. That way it's already there if someone ever considers
it necessary for the driver.

> +		#sound-dai-cells = <1>;

Should be <0>. You might want to check your addition with
"make dtbs_check" to make sure it matches the DT schema. :)

Also, I would recommend adding a "sound-name-prefix". The prefix is used
for the ALSA mixers which makes it more obvious that they belong to the
speaker. (See example in DT schema.)

> +	};
> +};
> +
>  &ipa {
>  	status = "okay";
>  
[...]
> @@ -756,6 +879,24 @@ &venus {
>  	firmware-name = "qcom/sdm845/axolotl/venus.mbn";
>  };
>  
> +&wcd9340 {
> +	pinctrl-0 = <&wcd_intr_default>;
> +	pinctrl-names = "default";
> +	clock-names = "extclk";
> +	clocks = <&rpmhcc RPMH_LN_BB_CLK2>;
> +	reset-gpios = <&tlmm 64 0>;

0 => GPIO_ACTIVE_HIGH

Thanks,
Stephan