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

[bgodavar]

Hi Matthias,

On 2021-12-07 20:18, Matthias Kaehlcke wrote:
> On Tue, Dec 07, 2021 at 11:43:43AM +0530, Balakrishna Godavarthi wrote:
>
> > 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.

> > From: Balakrishna Godavarthi <bgodavar@xxxxxxxxxxxxx>
> >
> > Add bluetooth SoC WCN6750 node for SC7280 IDP boards.
> >
> > Signed-off-by: Balakrishna Godavarthi <bgodavar@xxxxxxxxxxxxxx>
> > ---
> > v3:
> >   * Addressed reviewers comments
>
> that's not overly useful, instead you should describe what changed
>
> >   * 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.
> > +		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.