Message-ID: <CA+kEDGHyx7C7PNxQ8votwABiQpKhAAh126os3OLu-W0kDo2ySQ@mail.gmail.com>
Date: Tue, 2 Dec 2025 19:50:39 +0100
From: Jérôme de Bretagne <jerome.debretagne@...il.com>
To: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
Cc: Dale Whinham <daleyo@...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>, linux-arm-msm@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/8] arm64: dts: qcom: Add support for Surface Pro 11

Hello,

As discussed with Dale, I will take over the v3 submission since we've
worked on this patchset together until now. Here is my feedback below.

Le lun. 1 déc. 2025 à 16:35, Konrad Dybcio
<konrad.dybcio@....qualcomm.com> a écrit :
>
> On 12/1/25 2:14 AM, Dale Whinham wrote:
> > Add device trees for the Qualcomm X1E and X1P-based Microsoft Surface
> > Pro 11 machines (codenamed 'Denali').
> >
> > This device is very similar to the Surface Laptop 7 ('Romulus').
> >
> > Use a similar strategy to x1-asus-zenbook-a14.dtsi so that we can create
> > x1e and x1p-specific flavors of the device tree without too much code
> > duplication.
>
> [...]
>
> > +             pinctrl-0 = <&hall_int_n_default>;
> > +             pinctrl-names = "default";in v3
> > +
> > +             switch-lid {
> > +                     gpios = <&tlmm 2 GPIO_ACTIVE_LOW>;
> > +                     linux,input-type = <EV_SW>;
> > +                     linux,code = <SW_LID>;
>
> I.. don't think this device has a lid - what triggers this GPIO?

When a Surface tablet is connected to a Surface keyboard, opening/closing
the keyboard triggers a wakeup/suspend event. I will double-check if this
entry is involved and will remove/keep it in v3 based on this check.

> [...]
>
> > +     /*
> > +      * TODO: These two regulators are actually part of the removable M.2
> > +      * card and not the CRD mainboard. Need to describe this differently.
> > +      * Functionally it works correctly, because all we need to do is to
> > +      * turn on the actual 3.3V supply above.
>
> There's not a M.2 card, the WLAN chip is soldered on board
>
> https://www.ifixit.com/Guide/Microsoft+Surface+Pro+11+Chip+ID/174016#s370945

Indeed, I will remove this comment as it is not applicable to this model.

> > +     sound {
> > +             compatible = "qcom,x1e80100-sndcard";
> > +             model = "X1E80100-Microsoft-Surface-Pro-11";
> > +             audio-routing = "SpkrLeft IN", "WSA WSA_SPK1 OUT",
> > +                             "SpkrRight IN", "WSA WSA_SPK2 OUT",
> > +                             "VA DMIC0", "vdd-micb",
> > +                             "VA DMIC1", "vdd-micb";
> > +
> > +             wsa-dai-link {
> > +                     link-name = "WSA Playback";
> > +
> > +                     cpu {
> > +                             sound-dai = <&q6apmbedai WSA_CODEC_DMA_RX_0>;
> > +                     };
> > +
> > +                     codec {
>
> 'co'dec < 'cp'u
>
> please flip the order of the two

Sure, will do in v3. For info, this is also in the wrong order in *romulus.dtsi.

> [...]
>
> > +&i2c0 {
> > +     clock-frequency = <400000>;
> > +
> > +     status = "disabled";
>
> Is there anything connected to that controller?

Not clear yet, maybe for the touchscreen and pen, still to be investigated.

> If so, let's keep it enabled so that it's accessible through i2c-tools
> It'd be even better if you could document (in a comment, like in romulus.dtsi)
> what and at what address that is

I will enable it in v3, not sure if I'll find the right info to add a
useful comment.

> [...]
>
> > +&lpass_tlmm {
> > +     spkr_01_sd_n_active: spkr-01-sd-n-active-state {
> > +             pins = "gpio12";
> > +             function = "gpio";
> > +             drive-strength = <16>;
> > +             bias-disable;
> > +             output-low;
>
> Please drop output-low from both definitions, the output state is

Ok for v3.

> controlled manually by the WSA driver. Although from the diff below
> it looks like spkr_23_sd_n is unused and you only have 2 speakers

Looking at the public specs, it only has 2 speakers indeed.

> [...]
>
> > +&tlmm {
> > +     gpio-reserved-ranges = <44 4>, /* SPI (TPM) */
> > +                                                <238 1>; /* UFS Reset */
>
> Please ensure your tab width is set to 8

I will fix this in v3.

> [...]
> > +     cam_indicator_en: cam-indicator-en-state {
> > +             pins = "gpio225";
> > +             function = "gpio";
> > +             drive-strength = <2>;
> > +             bias-disable;
> > +     };
> > +
> > +     wcn_sw_en: wcn-sw-en-state {
> > +                     pins = "gpio214";
> > +                     function = "gpio";
> > +                     drive-strength = <2>;
> > +                     bias-disable;
> > +     };
> > +
> > +     wcn_wlan_bt_en: wcn-wlan-bt-en-state {
> > +                     pins = "gpio116", "gpio117";
> > +                     function = "gpio";
> > +                     drive-strength = <2>;
> > +                     bias-disable;
> > +     };
>
> and here (+ these last 2 entries are out of order, GPIO num-wise,
> please adjust that )

Noted for the tabs here + the 2 entries to reorder based on GPIO.

> [...]
>
> > +++ b/arch/arm64/boot/dts/qcom/x1e80100-microsoft-denali-oled.dts
> > @@ -0,0 +1,20 @@
> > +// SPDX-License-Identifier: BSD-3-Clause
> > +/*
> > + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
> > + * Copyright (c) 2025 Dale Whinham <daleyo@...il.com>
> > + */
> > +
> > +/dts-v1/;
> > +
> > +#include "x1e80100.dtsi"
> > +#include "x1-microsoft-denali.dtsi"
> > +
> > +/ {
> > +     model = "Microsoft Surface Pro 11th Edition (OLED)";
> > +     compatible = "microsoft,denali-oled", "microsoft,denali",
> > +                  "qcom,x1e80100";
>
> Are the OLED models always X1E and the LCD ones always based on X1E80100
> and LCD models always based on X1P64100?

The OLED models are always with the X1E and the LCD ones with X1P64100,
at least this is our understanding from the specs and online configurations.

> Konrad

Thanks a lot for your review,
Jérôme

> > +};
> > +
> > +&panel {
> > +     compatible = "samsung,atna30dw01", "samsung,atna33xc20";
> > +};
> > diff --git a/arch/arm64/boot/dts/qcom/x1p64100-microsoft-denali.dts b/arch/arm64/boot/dts/qcom/x1p64100-microsoft-denali.dts
> > new file mode 100644
> > index 000000000000..7c064ad49395
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/qcom/x1p64100-microsoft-denali.dts
> > @@ -0,0 +1,16 @@
> > +// SPDX-License-Identifier: BSD-3-Clause
> > +/*
> > + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
> > + * Copyright (c) 2025 Dale Whinham <daleyo@...il.com>
> > + */
> > +
> > +/dts-v1/;
> > +
> > +#include "x1e80100.dtsi"
> > +#include "x1-microsoft-denali.dtsi"
> > +
> > +/ {
> > +     model = "Microsoft Surface Pro 11th Edition (LCD)";
> > +     compatible = "microsoft,denali-lcd", "microsoft,denali",
> > +                  "qcom,x1p64100", "qcom,x1e80100";
> > +};

Powered by blists - more mailing lists