| Message ID | 20230131043816.4525-5-steev@kali.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Attempt at adding WCN6855 BT support | expand |
On 31/01/2023 05:38, Steev Klimaszewski wrote: > Signed-off-by: Steev Klimaszewski <steev@kali.org> > --- > .../qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 68 +++++++++++++++++++ > 1 file changed, 68 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts > index f936b020a71d..951438ac5946 100644 > --- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts > +++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts > @@ -24,6 +24,8 @@ / { > aliases { > i2c4 = &i2c4; > i2c21 = &i2c21; > + serial0 = &uart17; > + serial1 = &uart2; > }; > > wcd938x: audio-codec { > @@ -712,6 +714,32 @@ &qup0 { > status = "okay"; > }; > > +&uart2 { > + status = "okay"; > + > + pinctrl-names = "default"; > + pinctrl-0 = <&uart2_state>; > + > + bluetooth { > + compatible = "qcom,wcn6855-bt"; > + > +/* Why dead code should be in the kernel? > + vddio-supply = <&vreg_s4a_1p8>; > + vddxo-supply = <&vreg_l7a_1p8>; > + vddrf-supply = <&vreg_l17a_1p3>; > + vddch0-supply = <&vreg_l25a_3p3>; > + vddch1-supply = <&vreg_l23a_3p3>; > +*/ > + max-speed = <3200000>; > + > + enable-gpios = <&tlmm 133 GPIO_ACTIVE_HIGH>; > + swctrl-gpios = <&tlmm 132 GPIO_ACTIVE_HIGH>; > + > + pinctrl-names = "default"; > + pinctrl-0 = <&bt_en>; > + }; > +}; > + > &qup1 { > status = "okay"; > }; > @@ -720,6 +748,12 @@ &qup2 { > status = "okay"; > }; > > +&uart17 { > + compatible = "qcom,geni-debug-uart"; > + > + status = "okay"; > +}; > + > &remoteproc_adsp { > firmware-name = "qcom/sc8280xp/LENOVO/21BX/qcadsp8280.mbn"; > > @@ -980,6 +1014,19 @@ hastings_reg_en: hastings-reg-en-state { > &tlmm { > gpio-reserved-ranges = <70 2>, <74 6>, <83 4>, <125 2>, <128 2>, <154 7>; > > + bt_en: bt-en-state { > + hstp-sw-ctrl { Does not look like you tested the DTS against bindings. Please run `make dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst for instructions). > + pins = "gpio132"; > + function = "gpio"; > + }; > + > + hstp-bt-en { > + pins = "gpio133"; > + function = "gpio"; > + drive-strength = <16>; > + }; > + }; > + > edp_reg_en: edp-reg-en-state { > pins = "gpio25"; > function = "gpio"; > @@ -1001,6 +1048,27 @@ i2c4_default: i2c4-default-state { > bias-disable; > }; > > + uart2_state: uart2-state { > + cts { Does not look like you tested the DTS against bindings. Please run `make dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst for instructions). Best regards, Krzysztof
>On 31/01/2023 05:38, Steev Klimaszewski wrote: >> Signed-off-by: Steev Klimaszewski <steev@kali.org> >> --- >> .../qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 68 +++++++++++++++++++ >> 1 file changed, 68 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts >> index f936b020a71d..951438ac5946 100644 >> --- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts >> +++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts >> @@ -24,6 +24,8 @@ / { >> aliases { >> i2c4 = &i2c4; >> i2c21 = &i2c21; >> + serial0 = &uart17; >> + serial1 = &uart2; >> }; >> >> wcd938x: audio-codec { >> @@ -712,6 +714,32 @@ &qup0 { >> status = "okay"; >> }; >> >> +&uart2 { >> + status = "okay"; >> + >> + pinctrl-names = "default"; >> + pinctrl-0 = <&uart2_state>; >> + >> + bluetooth { >> + compatible = "qcom,wcn6855-bt"; >> + >> +/* > Why dead code should be in the kernel? As mentioned in the cover letter, this is a bit closer to an RFC than ready to go in, and I do apologize that it wasn't clear enough. I do not have access to the schematics, and based on my reading of the schema for bluetooth, these entries are supposed to be required, however, like the wcn6750, I have dummy data entered into the qca_soc_data_wcn6855 struct. I know that these should be there, I just do not have access to the correct information to put, if that makes sense? <snip> >Does not look like you tested the DTS against bindings. Please run `make >dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst >for instructions). Correct I had not, but I have now, and will make the corrections test and they will be included in v3. >Best regards, >Krzysztof I appreciate the guidance for what I was doing incorrectly. -- steev
On 01/02/2023 04:13, Steev Klimaszewski wrote: >> On 31/01/2023 05:38, Steev Klimaszewski wrote: >>> Signed-off-by: Steev Klimaszewski <steev@kali.org> >>> --- >>> .../qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 68 +++++++++++++++++++ >>> 1 file changed, 68 insertions(+) >>> >>> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts >>> index f936b020a71d..951438ac5946 100644 >>> --- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts >>> +++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts >>> @@ -24,6 +24,8 @@ / { >>> aliases { >>> i2c4 = &i2c4; >>> i2c21 = &i2c21; >>> + serial0 = &uart17; >>> + serial1 = &uart2; >>> }; >>> >>> wcd938x: audio-codec { >>> @@ -712,6 +714,32 @@ &qup0 { >>> status = "okay"; >>> }; >>> >>> +&uart2 { >>> + status = "okay"; >>> + >>> + pinctrl-names = "default"; >>> + pinctrl-0 = <&uart2_state>; >>> + >>> + bluetooth { >>> + compatible = "qcom,wcn6855-bt"; >>> + >>> +/* > >> Why dead code should be in the kernel? > > As mentioned in the cover letter, this is a bit closer to an RFC than ready to > go in, and I do apologize that it wasn't clear enough. I do not have access to > the schematics, and based on my reading of the schema for bluetooth, these > entries are supposed to be required, however, like the wcn6750, I have dummy > data entered into the qca_soc_data_wcn6855 struct. I know that these should be > there, I just do not have access to the correct information to put, if that > makes sense? Keeping them commented out, does not solve the "these should be there". Drop or add them. Best regards, Krzysztof
Hi Steev, On Tue, Jan 31, 2023 at 7:13 PM Steev Klimaszewski <steev@kali.org> wrote: > > >On 31/01/2023 05:38, Steev Klimaszewski wrote: > >> Signed-off-by: Steev Klimaszewski <steev@kali.org> > >> --- > >> .../qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 68 +++++++++++++++++++ > >> 1 file changed, 68 insertions(+) > >> > >> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts > >> index f936b020a71d..951438ac5946 100644 > >> --- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts > >> +++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts > >> @@ -24,6 +24,8 @@ / { > >> aliases { > >> i2c4 = &i2c4; > >> i2c21 = &i2c21; > >> + serial0 = &uart17; > >> + serial1 = &uart2; > >> }; > >> > >> wcd938x: audio-codec { > >> @@ -712,6 +714,32 @@ &qup0 { > >> status = "okay"; > >> }; > >> > >> +&uart2 { > >> + status = "okay"; > >> + > >> + pinctrl-names = "default"; > >> + pinctrl-0 = <&uart2_state>; > >> + > >> + bluetooth { > >> + compatible = "qcom,wcn6855-bt"; > >> + > >> +/* > > > Why dead code should be in the kernel? > > As mentioned in the cover letter, this is a bit closer to an RFC than ready to > go in, and I do apologize that it wasn't clear enough. I do not have access to > the schematics, and based on my reading of the schema for bluetooth, these > entries are supposed to be required, however, like the wcn6750, I have dummy > data entered into the qca_soc_data_wcn6855 struct. I know that these should be > there, I just do not have access to the correct information to put, if that > makes sense? Well you don't have the RFC set in the subject which is probably why people are reviewing it like it is supposed to be merged, that said I do wonder if there is to indicate these entries are to be considered sort of experimental so we don't end up enabling it by default? > > <snip> > > >Does not look like you tested the DTS against bindings. Please run `make > >dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst > >for instructions). > > Correct I had not, but I have now, and will make the corrections test and they > will be included in v3. > > >Best regards, > >Krzysztof > > I appreciate the guidance for what I was doing incorrectly. > > -- steev
Hi Luiz, >Hi Steev, >On Tue, Jan 31, 2023 at 7:13 PM Steev Klimaszewski <steev@kali.org> wrote: >> >> >On 31/01/2023 05:38, Steev Klimaszewski wrote: >> >> Signed-off-by: Steev Klimaszewski <steev@kali.org> >> >> --- >> >> .../qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 68 +++++++++++++++++++ >> >> 1 file changed, 68 insertions(+) >> >> >> >> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts >> >> index f936b020a71d..951438ac5946 100644 >> >> --- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts >> >> +++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts >> >> @@ -24,6 +24,8 @@ / { >> >> aliases { >> >> i2c4 = &i2c4; >> >> i2c21 = &i2c21; >> >> + serial0 = &uart17; >> >> + serial1 = &uart2; >> >> }; >> >> >> >> wcd938x: audio-codec { >> >> @@ -712,6 +714,32 @@ &qup0 { >> >> status = "okay"; >> >> }; >> >> >> >> +&uart2 { >> >> + status = "okay"; >> >> + >> >> + pinctrl-names = "default"; >> >> + pinctrl-0 = <&uart2_state>; >> >> + >> >> + bluetooth { >> >> + compatible = "qcom,wcn6855-bt"; >> >> + >> >> +/* >> > Why dead code should be in the kernel? >> As mentioned in the cover letter, this is a bit closer to an RFC than ready to >> go in, and I do apologize that it wasn't clear enough. I do not have access to >> the schematics, and based on my reading of the schema for bluetooth, these >> entries are supposed to be required, however, like the wcn6750, I have dummy >> data entered into the qca_soc_data_wcn6855 struct. I know that these should be >> there, I just do not have access to the correct information to put, if that >> makes sense? >Well you don't have the RFC set in the subject which is probably why >people are reviewing it like it is supposed to be merged, that said I >do wonder if there is to indicate these entries are to be considered >sort of experimental so we don't end up enabling it by default? > Initially, it was meant to be more of an RFC/RFT, but as it turns out it works pretty good with the defaults in the bluetooth driver, so I've made a change in v3 to just make a note that it's a TODO? I'm not sure if that's okay or not, but I'm sure people will let me know :) >> >> <snip> >> >> -- steev >-- >Luiz Agusto von Dentz
diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts index f936b020a71d..951438ac5946 100644 --- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts +++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts @@ -24,6 +24,8 @@ / { aliases { i2c4 = &i2c4; i2c21 = &i2c21; + serial0 = &uart17; + serial1 = &uart2; }; wcd938x: audio-codec { @@ -712,6 +714,32 @@ &qup0 { status = "okay"; }; +&uart2 { + status = "okay"; + + pinctrl-names = "default"; + pinctrl-0 = <&uart2_state>; + + bluetooth { + compatible = "qcom,wcn6855-bt"; + +/* + vddio-supply = <&vreg_s4a_1p8>; + vddxo-supply = <&vreg_l7a_1p8>; + vddrf-supply = <&vreg_l17a_1p3>; + vddch0-supply = <&vreg_l25a_3p3>; + vddch1-supply = <&vreg_l23a_3p3>; +*/ + max-speed = <3200000>; + + enable-gpios = <&tlmm 133 GPIO_ACTIVE_HIGH>; + swctrl-gpios = <&tlmm 132 GPIO_ACTIVE_HIGH>; + + pinctrl-names = "default"; + pinctrl-0 = <&bt_en>; + }; +}; + &qup1 { status = "okay"; }; @@ -720,6 +748,12 @@ &qup2 { status = "okay"; }; +&uart17 { + compatible = "qcom,geni-debug-uart"; + + status = "okay"; +}; + &remoteproc_adsp { firmware-name = "qcom/sc8280xp/LENOVO/21BX/qcadsp8280.mbn"; @@ -980,6 +1014,19 @@ hastings_reg_en: hastings-reg-en-state { &tlmm { gpio-reserved-ranges = <70 2>, <74 6>, <83 4>, <125 2>, <128 2>, <154 7>; + bt_en: bt-en-state { + hstp-sw-ctrl { + pins = "gpio132"; + function = "gpio"; + }; + + hstp-bt-en { + pins = "gpio133"; + function = "gpio"; + drive-strength = <16>; + }; + }; + edp_reg_en: edp-reg-en-state { pins = "gpio25"; function = "gpio"; @@ -1001,6 +1048,27 @@ i2c4_default: i2c4-default-state { bias-disable; }; + uart2_state: uart2-state { + cts { + pins = "gpio122"; + function = "qup2"; + bias-disable; + }; + + rts-tx { + pins = "gpio122", "gpio123"; + function = "qup2"; + drive-strength = <2>; + bias-disable; + }; + + rx { + pins = "gpio124"; + function = "qup2"; + bias-pull-up; + }; + }; + i2c21_default: i2c21-default-state { pins = "gpio81", "gpio82"; function = "qup21";
Signed-off-by: Steev Klimaszewski <steev@kali.org> --- .../qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 68 +++++++++++++++++++ 1 file changed, 68 insertions(+)