Re: [PATCH v2 1/3] arm64: dts: qcom: sc7280: Add nodes for va tx and rx macros and external codecs

From: Stephen Boyd
Date: Wed Jan 05 2022 - 15:16:03 EST

Quoting Srinivasa Rao Mandadapu (2022-01-03 03:12:58)
> SC7280 has VA, TX and RX macros with SoundWire Controllers to attach with
> codecs like WCD938x, max98360a using soundwire masters and i2s bus.
> Add these nodes for sc7280 based platforms audio use case.
> Add tlmm gpio property in wcd938x node for switching CTIA/OMTP Headset.
>
> Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@xxxxxxxxxxx>
> Co-developed-by: Venkata Prasad Potturu <quic_potturu@xxxxxxxxxxx>
> Signed-off-by: Venkata Prasad Potturu <quic_potturu@xxxxxxxxxxx>
> ---
> arch/arm64/boot/dts/qcom/sc7280-crd.dts | 4 ++
> arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 52 ++++++++++++++
> arch/arm64/boot/dts/qcom/sc7280.dtsi | 113 +++++++++++++++++++++++++++++++
> 3 files changed, 169 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd.dts b/arch/arm64/boot/dts/qcom/sc7280-crd.dts
> index cd2755c..035cca9 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-crd.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7280-crd.dts
> @@ -72,6 +72,10 @@ ap_ts_pen_1v8: &i2c13 {
> pins = "gpio51";
> };
>
> +&wcd938x {
> + qcom,us-euro-gpios = <&tlmm 81 0>;

Why is this a qcom prefix vs. a standard foo-gpios property?

> +};
> +
> &tlmm {
> tp_int_odl: tp-int-odl {
> pins = "gpio7";
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> index ddeb508..94614c9 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> @@ -636,3 +636,55 @@
> bias-pull-up;
> };
> };
> +
> +&swr0 {
> + wcd_rx: wcd938x-rx{

space before { please

Also, are these speakers or amps or something like that? I'd expect the
node name to be more generic.

> + compatible = "sdw20217010d00";
> + reg = <0 4>;
> + #sound-dai-cells = <1>;
> + qcom,rx-port-mapping = <1 2 3 4 5>;
> + };
> +};
> +
> +&swr1 {
> + wcd_tx: wcd938x-tx{
> + compatible = "sdw20217010d00";
> + reg = <0 3>;
> + #sound-dai-cells = <1>;
> + qcom,tx-port-mapping = <1 2 3 4>;
> + };
> +};
> +
> +&soc {
> + max98360a: audio-codec-0 {

This shouldn't be a child of the soc node.

> + compatible = "maxim,max98360a";
> + pinctrl-names = "default";
> + pinctrl-0 = <&amp_en>;
> + sdmode-gpios = <&tlmm 63 GPIO_ACTIVE_HIGH>;
> + #sound-dai-cells = <0>;
> + };
> +
> + wcd938x: codec {

Same comment.

> + compatible = "qcom,wcd9380-codec";
> + #sound-dai-cells = <1>;
> +
> + reset-gpios = <&tlmm 83 0>;
> +
> + qcom,rx-device = <&wcd_rx>;
> + qcom,tx-device = <&wcd_tx>;
> +
> + vdd-rxtx-supply = <&vreg_l18b_1p8>;
> + vdd-io-supply = <&vreg_l18b_1p8>;
> + vdd-buck-supply = <&vreg_l17b_1p8>;
> + vdd-mic-bias-supply = <&vreg_bob>;
> +
> + qcom,micbias1-microvolt = <1800000>;
> + qcom,micbias2-microvolt = <1800000>;
> + qcom,micbias3-microvolt = <1800000>;
> + qcom,micbias4-microvolt = <1800000>;
> +
> + qcom,mbhc-buttons-vthreshold-microvolt = <75000 150000 237000 500000 500000 500000 500000 500000>;
> + qcom,mbhc-headset-vthreshold-microvolt = <1700000>;
> + qcom,mbhc-headphone-vthreshold-microvolt = <50000>;
> + };
> +};
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index 6233f2c..c0d9de3 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -1744,6 +1744,119 @@
> #clock-cells = <1>;
> };
>
> + rxmacro: rxmacro@3200000 {

What is rxmacro? Maybe 'soundwire'?

> + pinctrl-names = "default";
> + pinctrl-0 = <&rx_swr_active>;
> + compatible = "qcom,sc7280-lpass-rx-macro";
> + reg = <0 0x3200000 0 0x1000>;
> +
> + clocks = <&lpass_aon LPASS_AON_CC_TX_MCLK_CLK>,
> + <&lpass_aon LPASS_AON_CC_TX_MCLK_2X_CLK>,
> + <&vamacro>;
> + clock-names = "mclk", "npl", "fsgen";
> + #clock-cells = <0>;
> + clock-frequency = <9600000>;
> + clock-output-names = "mclk";
> + #sound-dai-cells = <1>;
> + };
> +
> + /* rx macro */
> + swr0: soundwire-controller@3210000 {

Is 'controller' redundant? i.e. soundwire is always a controller? Maybe
'soundwire' is better

> + reg = <0 0x3210000 0 0x2000>;
> + compatible = "qcom,soundwire-v1.6.0";
> + interrupts = <GIC_SPI 155 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&rxmacro>;
> + clock-names = "iface";
> + label = "RX";
> +
> + qcom,din-ports = <0>;
> + qcom,dout-ports = <5>;
> + qcom,swrm-hctl-reg = <0x032a90a0>;
> +
> + qcom,ports-word-length = /bits/ 8 <0x01 0x07 0x04 0xFF 0xFF>;
> + qcom,ports-sinterval-low = /bits/ 8 <0x03 0x3F 0x1F 0x03 0x03>;
> + qcom,ports-offset1 = /bits/ 8 <0x00 0x00 0x0B 0x01 0x01>;
> + qcom,ports-offset2 = /bits/ 8 <0x00 0x00 0x0B 0x00 0x00>;
> + qcom,ports-lane-control = /bits/ 8 <0x01 0x00 0x00 0x00 0x00>;
> + qcom,ports-block-pack-mode = /bits/ 8 <0xFF 0x00 0x01 0xFF 0xFF>;
> + qcom,ports-hstart = /bits/ 8 <0xFF 0x03 0xFF 0xFF 0xFF>;
> + qcom,ports-hstop = /bits/ 8 <0xFF 0x06 0xFF 0xFF 0xFF>;
> + qcom,ports-block-group-count = /bits/ 8 <0xFF 0xFF 0xFF 0xFF 0x00>;

Why is this bit stuff in DT vs. in the device driver?

> +
> + #sound-dai-cells = <1>;
> + #address-cells = <2>;
> + #size-cells = <0>;
> + };
> +
> + txmacro: txmacro@3220000 {
> + pinctrl-names = "default";
> + pinctrl-0 = <&tx_swr_active>;
> + compatible = "qcom,sc7280-lpass-tx-macro";
> + reg = <0 0x3220000 0 0x1000>;
> +
> + clocks = <&lpass_aon LPASS_AON_CC_TX_MCLK_CLK>,
> + <&lpass_aon LPASS_AON_CC_TX_MCLK_2X_CLK>,
> + <&vamacro>;

Please align the clocks here so the < starts on the same column.

> + clock-names = "mclk", "npl", "fsgen";
> +
> + #clock-cells = <0>;
> + clock-frequency = <9600000>;
> + clock-output-names = "mclk";
> + #address-cells = <2>;
> + #size-cells = <2>;
> + #sound-dai-cells = <1>;
> + };
> +
> + /* tx macro */

Do we need these comments? Please remove them as they're practically
useless.

> + swr1: soundwire-controller@3230000 {
> + reg = <0 0x3230000 0 0x2000>;
> + compatible = "qcom,soundwire-v1.6.0";
> +
> + interrupts-extended =
> + <&intc GIC_SPI 496 IRQ_TYPE_LEVEL_HIGH>,
> + <&pdc 130 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-names = "swr_master_irq", "swr_wake_irq";
> + clocks = <&txmacro>;
> + clock-names = "iface";
> + label = "TX";
> +
> + qcom,din-ports = <3>;
> + qcom,dout-ports = <0>;
> + qcom,swrm-hctl-reg = <0x032a90a8>;

This looks like some common register location for soundwire oops bits.
Is it another device? Or maybe it's a common register base that needs to
be a syscon?

> +
> + qcom,ports-sinterval-low = /bits/ 8 <0x01 0x03 0x03>;
> + qcom,ports-offset1 = /bits/ 8 <0x01 0x00 0x02>;
> + qcom,ports-offset2 = /bits/ 8 <0x00 0x00 0x00>;
> + qcom,ports-hstart = /bits/ 8 <0xFF 0xFF 0xFF>;
> + qcom,ports-hstop = /bits/ 8 <0xFF 0xFF 0xFF>;
> + qcom,ports-word-length = /bits/ 8 <0xFF 0x0 0xFF>;
> + qcom,ports-block-pack-mode = /bits/ 8 <0xFF 0xFF 0xFF>;
> + qcom,ports-block-group-count = /bits/ 8 <0xFF 0xFF 0xFF>;
> + qcom,ports-lane-control = /bits/ 8 <0x00 0x01 0x00>;
> + qcom,port-offset = <1>;
> +
> + #sound-dai-cells = <1>;
> + #address-cells = <2>;
> + #size-cells = <0>;
> + };
> +
> + vamacro: codec@3370000 {
> + compatible = "qcom,sc7280-lpass-va-macro";
> + pinctrl-0 = <&dmic01_active>;
> + pinctrl-names = "default";
> +
> + vdd-micb-supply = <&vreg_bob>;

The supplies need to be set in the board file, no the soc file as they
can be different depending on what the hardware engineer wires to the
pins on the SoC.

> + reg = <0 0x3370000 0 0x1000>;
> + clocks = <&lpass_aon LPASS_AON_CC_TX_MCLK_CLK>,
> + <&lpass_audiocc LPASS_AUDIO_CC_RX_MCLK_CLK>;
> + clock-names = "mclk", "macro";
> +
> + #clock-cells = <0>;
> + clock-frequency = <9600000>;
> + clock-output-names = "fsgen";
> + #sound-dai-cells = <1>;
> + };
> +
> lpass_tlmm: pinctrl@33c0000 {
> compatible = "qcom,sc7280-lpass-lpi-pinctrl";
> reg = <0 0x33c0000 0x0 0x20000>,
> --
> 2.7.4
>