Re: [PATCH v3] arm64: dts: qcom: sc7280: Add bluetooth node on SC7280

Hi Matthias,

On 2021-12-07 20:18, Matthias Kaehlcke wrote:

On Tue, Dec 07, 2021 at 11:43:43AM +0530, Balakrishna Godavarthi wrote:

quoted

Subject: arm64: dts: qcom: sc7280: Add bluetooth node on SC7280

nit: the node is added to the IDP boards, not sc7280 in general

[Bala]: will update it.

quoted

From: Balakrishna Godavarthi <redacted>

Add bluetooth SoC WCN6750 node for SC7280 IDP boards.

Signed-off-by: Balakrishna Godavarthi <redacted>
---
v3:
  * Addressed reviewers comments

that's not overly useful, instead you should describe what changed

quoted

  * Added pin config for sw_ctrl line.
v2:
  * merged two patches into one
  * Removed unused comments
  * Removed pinmux & pin conf.
  * Addressed reviewers comments

v1: initial patch
---
 arch/arm64/boot/dts/qcom/sc7280-idp.dts  |  4 ++++
 arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 37 
++++++++++++++++++++++++++++++++
 arch/arm64/boot/dts/qcom/sc7280-idp2.dts |  4 ++++
 3 files changed, 45 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dts 

b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
index 9b991ba..19bd228 100644

--- a/arch/arm64/boot/dts/qcom/sc7280-idp.dts
+++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dts

@@ -56,6 +56,10 @@
 	};
 };

+&bluetooth {
+	vddio-supply = <&vreg_l19b_1p8>;
+};
+
 &ipa {
 	status = "okay";
 	modem-init;

diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi 

b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
index d623d71..b8b00dc 100644

--- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi

@@ -14,6 +14,11 @@
 #include "pmk8350.dtsi"

 / {
+	aliases {
+		bluetooth0 = &bluetooth;
+		serial1 = &uart7;
+	};
+
 	gpio-keys {
 		compatible = "gpio-keys";
 		label = "gpio-keys";

@@ -422,6 +427,23 @@
 				<&tlmm 31 IRQ_TYPE_EDGE_FALLING>;
 	pinctrl-names = "default", "sleep";
 	pinctrl-1 = <&qup_uart7_sleep_cts>, <&qup_uart7_sleep_rts>, 

<&qup_uart7_sleep_tx>, <&qup_uart7_sleep_rx>;
+
+	bluetooth: bluetooth {
+		compatible = "qcom,wcn6750-bt";
+		pinctrl-names = "default";
+		pinctrl-0 = <&bt_en>, <&swctrl_gpio>;
+		enable-gpios = <&tlmm 85 GPIO_ACTIVE_HIGH>;
+		swctrl-gpios = <&tlmm 86 GPIO_ACTIVE_HIGH>;
+		vddaon-supply = <&vreg_s7b_0p9>;
+		vddbtcxmx-supply = <&vreg_s7b_0p9>;
+		vddrfacmn-supply = <&vreg_s7b_0p9>;
+		vddrfa0p8-supply = <&vreg_s7b_0p9>;
+		vddrfa1p7-supply = <&vreg_s1b_1p8>;
+		vddrfa1p2-supply = <&vreg_s8b_1p2>;
+		vddrfa2p2-supply = <&vreg_s1c_2p2>;
+		vddasd-supply = <&vreg_l11c_2p8>;
+		max-speed = <3200000>;
+	};
 };

 /* PINCTRL - additions to nodes defined in sc7280.dtsi */

@@ -491,6 +513,14 @@
 };

 &tlmm {
+	bt_en: bt-en {
+		pins = "gpio85";
+		function = "gpio";
+		drive-strength = <2>;

is it really necessary to specify the drive strength?

Documentation/devicetree/bindings/pinctrl/qcom,sc7280-pinctrl.yaml:

  drive-strength:
    enum: [2, 4, 6, 8, 10, 12, 14, 16]
    default: 2

The default is 2, so it shouldn't be needed.

[Bala]: will update it.

quoted

+		output-low;
+		bias-disable;
+	};
+
 	nvme_pwren: nvme-pwren {
 		function = "gpio";
 	};

@@ -554,6 +584,13 @@
 		 */
 		bias-pull-up;
 	};
+
+	swctrl_gpio: swctrl-gpio {

The 'gpio' suffix isn't really useful.

I suggest to use the signal name from the schematics "mos-sw-ctrl" or
call it "bt-sw-ctrl". If you use the schematic name then this should
be also done for the enable pin.

[Bala]: will update it.