[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <74608385-a3a6-4e59-9c63-6fb9d746d234@kernel.org>
Date: Wed, 4 Sep 2024 16:14:14 +0200
From: Konrad Dybcio <konradybcio@...nel.org>
To: Jérôme de Bretagne <jerome.debretagne@...il.com>,
Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio <konradybcio@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>
Cc: Johan Hovold <johan+linaro@...nel.org>, linux-arm-msm@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/4] arm64: dts: qcom: sc8280xp: Add Microsoft Surface Pro
9 5G
On 4.09.2024 12:42 AM, Jérôme de Bretagne wrote:
> Add an initial devicetree for the Microsoft Surface Pro 9 5G, based
> on SC8280XP.
>
> It enables the support for Wi-Fi, NVMe, the two USB Type-C ports,
> Bluetooth, 5G cellular modem, audio output (via Bluetooth headsets),
> external display via DisplayPort over Type-C (only the bottom USB
> Type-C port is working so far, corresponding to the usb1 / dp1 nodes),
> charging, the Surface Aggregator Module (SAM) to get keyboard and
> touchpad working with the Surface Type Cover accessories.
>
> Some key features not supported yet:
> - built-in display (but software fallback is working with efifb
> when blacklisting the msm module)
> - built-in display touchscreen
> - external display with the top USB Type-C port
> - speakers and microphones
> - physical volume up and down keys
> - LID switch detection
>
> This devicetree is based on the other SC8280XP ones, for the Lenovo
> ThinkPad X13s and the Qualcomm CRD.
>
> Signed-off-by: Jérôme de Bretagne <jerome.debretagne@...il.com>
> ---
[...]
> +
> + pmic-glink {
> + compatible = "qcom,sc8280xp-pmic-glink", "qcom,pmic-glink";
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + connector@0 {
> + compatible = "usb-c-connector";
> + reg = <0>;
> + power-role = "dual";
> + data-role = "dual";
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> +
> + pmic_glink_con0_hs: endpoint {
> + remote-endpoint = <&usb_0_dwc3_hs>;
> + };
> + };
> +
> + port@1 {
> + reg = <1>;
> +
> + pmic_glink_con0_ss: endpoint {
> + remote-endpoint = <&usb_0_qmpphy_out>;
> + };
> + };
> +
> + port@2 {
> + reg = <2>;
> +
> + pmic_glink_con0_sbu: endpoint {
> + remote-endpoint = <&usb0_sbu_mux>;
> + };
> + };
> + };
> + };
> +
> + connector@1 {
> + compatible = "usb-c-connector";
> + reg = <1>;
> + power-role = "dual";
> + data-role = "dual";
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + port@0 {
Missing newline above
Could you add a comment explaining which port is which (physically)?
See x1e device trees for an example
[...]
> + usb0-sbu-mux {
> + compatible = "pericom,pi3usb102", "gpio-sbu-mux";
> +
> + enable-gpios = <&tlmm 101 GPIO_ACTIVE_LOW>;
> + select-gpios = <&tlmm 164 GPIO_ACTIVE_HIGH>;
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <&usb0_sbu_default>;
property-n
property-names
please, all throughout the file
[...]
> +&pcie4_port0 {
> + wifi@0 {
> + compatible = "pci17cb,1103";
> + reg = <0x10000 0x0 0x0 0x0 0x0>;
> +
> + qcom,ath11k-calibration-variant = "LE_X13S";
This is most likely not a x13s, please add a new calibration variant
[...]
> +&sound {
> + compatible = "qcom,sc8280xp-sndcard";
> + model = "SC8280XP-MICROSOFT-SURFACE-PRO-9-5G";
> + audio-routing =
> + "SpkrLeft IN", "WSA_SPK1 OUT",
Drop the newline after =
> + "SpkrRight IN", "WSA_SPK2 OUT",
> + "IN1_HPHL", "HPHL_OUT",
> + "IN2_HPHR", "HPHR_OUT",
> + "AMIC2", "MIC BIAS2",
> + "VA DMIC0", "MIC BIAS1",
> + "VA DMIC1", "MIC BIAS1",
> + "VA DMIC2", "MIC BIAS3",
> + "VA DMIC0", "VA MIC BIAS1",
> + "VA DMIC1", "VA MIC BIAS1",
> + "VA DMIC2", "VA MIC BIAS3",
> + "TX SWR_ADC1", "ADC2_OUTPUT";
> +
> + wcd-playback-dai-link {
> + link-name = "WCD Playback";
> + cpu {
Add a newline between the last property and subnodes
[...]
> +
> +&uart18 {
> + status = "okay";
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <&uart18_state>;
> +
> + surface-aggregator {
> + compatible = "surface,aggregator";
This wasn't tested against the upstream driver (see
Documentation/devicetree/bindings/platform/microsoft,surface-sam.yaml)
[...]
> +
> +&tlmm {
> + gpio-reserved-ranges = <70 2>, <74 6>, <125 2>, <128 2>, <154 4>;
Please check that all of these entries are necessary (if they
aren't, removing them will break booting)
Konrad
Powered by blists - more mailing lists