Re: [PATCH 3/3] arm64: dts: qcom: sc8280xp: Add Huawei Matebook E Go (sc8280xp)

[Pengyu Luo]

Oh, I sent it by gamil wrongly(forgot cc to), I resend it by git send-email again

On Fri, Dec 13, 2024 at 1:13 AM Konrad Dybcio <konrad.dybcio@xxxxxxxxxxxxxxxx> wrote:
> On 11.12.2024 4:37 PM, Pengyu Luo wrote:
> > Add an initial devicetree for the Huawei Matebook E Go, which is based on
> > sc8280xp.
> >
> > There are 3 variants, Huawei released first 2 at the same time.
> > Huawei Matebook E Go LTE(sc8180x), codename should be gaokun2.
> > Huawei Matebook E Go(sc8280xp@3.0GHz), codename is gaokun3.
> > Huawei Matebook E Go 2023(sc8280xp@2.69GHz).
> >
> > We add support for the latter two variants.
> >
> > This work started by Tianyu Gao and Xuecong Chen, they made the
> > devicetree based on existing work(i.e. the Lenovo X13s and the
> > Qualcomm CRD), it can boot with framebuffer.
> >
> > Original work:
> https://github.com/matalama80td3l/matebook-e-go-boot-works/blob/main/dts/sc8280xp-huawei-matebook-e-go.dts
> >
> > Later, I got my device, I continue their work.
> >
> > Supported features:
> > - adsp
> > - bluetooth (connect issue)
> > - charge (with a lower power)
> > - framebuffer
> > - gpu
> > - keyboard (via internal USB)
> > - pcie devices (wifi and nvme, no modem)
> > - speakers and microphones
> > - tablet mode switch
> > - touchscreen
> > - usb
> > - volume key and power key
> >
> > Some key features not supported yet:
> > - battery and charger information report (EC driver required)
> > - built-in display (cannot enable backlight yet)
> > - charging thresholds control (EC driver required)
> > - camera
> > - LID switch detection (EC driver required)
> > - USB Type-C altmode (EC driver required)
> > - USB Type-C PD (EC driver required)
>
> Does qcom_battmgr / pmic_glink not work?
>

Unfortunately, different from many sc8280xp devices, device(PMGK) _STA is
Zero,
this device is quiet strange, also, it has no PM8008,
nor PMIC Battery Miniclass(PMBM), etc.

[...]

>
> > I have finished the EC driver, once this series are upstreamed,
> > I will submit a series of patches to enable EC support.
> >
> > Signed-off-by: Tianyu Gao <gty0622@xxxxxxxxx>
> > Signed-off-by: Xuecong Chen <chenxuecong2009@xxxxxxxxxxx>
>
> Your commit message suggests Co-developed-by: tags would also be
> fitting here
>

Agree

[...]

> [...]
>
> > +     chosen {
> > +             #address-cells =3D <2>;
> > +             #size-cells =3D <2>;
> > +             ranges;
> > +
> > +             framebuffer0: framebuffer@c6200000 {
> > +                     compatible =3D "simple-framebuffer";
> > +                     reg =3D <0x0 0xC6200000 0x0 0x02400000>;
> > +                     width =3D <1600>;
> > +                     height =3D <2560>;
> > +                     stride =3D <(1600 * 4)>;
> > +                     format =3D "a8r8g8b8";
> > +             };
> > +     };
>
> This should be redundant, as you should have efifb
>

I think no, it won't boot up without it(stuck at EFI stub: Booting Linux
Kernel)

[...]

>
> > +
> > +     wcd938x: audio-codec {
> > +             compatible =3D "qcom,wcd9380-codec";
> > +
> > +             pinctrl-names =3D "default";
> > +             pinctrl-0 =3D <&wcd_default>;
>
> Please follow this order throughout the file:
>
> property-n
> property-names
>

Do you mean I should arragne as following? If so, I actually keep
reference devicetree untouched. x13s and crd are written like this.

pinctrl-0 =3D <&wcd_default>>;
pinctrl-names =3D "default";

[...]

> [...]
>
> > +
> > +             reset-gpios =3D <&tlmm 106 GPIO_ACTIVE_LOW>;
> > +
> > +             vdd-buck-supply =3D <&vreg_s10b>;
> > +             vdd-rxtx-supply =3D <&vreg_s10b>;
> > +             vdd-io-supply =3D <&vreg_s10b>;
> > +             vdd-mic-bias-supply =3D <&vreg_bob>;
> > +
> > +             qcom,micbias1-microvolt =3D <1800000>;
> > +             qcom,micbias2-microvolt =3D <1800000>;
> > +             qcom,micbias3-microvolt =3D <1800000>;
> > +             qcom,micbias4-microvolt =3D <1800000>;
> > +             qcom,mbhc-buttons-vthreshold-microvolt =3D <75000 150000
> 237000 500000 500000 500000 500000 500000>;
> > +             qcom,mbhc-headset-vthreshold-microvolt =3D <1700000>;
> > +             qcom,mbhc-headphone-vthreshold-microvolt =3D <50000>;
> > +             qcom,rx-device =3D <&wcd_rx>;
> > +             qcom,tx-device =3D <&wcd_tx>;
> > +
> > +             #sound-dai-cells =3D <1>;
> > +     };
> > +
> > +     gpio-keys {
> > +             compatible =3D "gpio-keys";
> > +
> > +             pinctrl-names =3D "default";
> > +             pinctrl-0 =3D <&mode_pin_active>, <&vol_up_n>;
> > +
> > +             switch-mode {
> > +                     gpios =3D <&tlmm 26 GPIO_ACTIVE_HIGH>;
>
> This could use a label too - "Tablet Mode Switch", perhaps?
>

This part I follow Lenovo Yoga C630 one (see [1]), it doesn't use it,
and this node is not referenced.

> > +                     linux,input-type =3D <EV_SW>;
> > +                     linux,code =3D <SW_TABLET_MODE>;
> > +                     debounce-interval =3D <10>;
> > +                     wakeup-source;
> > +             };
> > +
> > +             key-vol-up {
>
> Please place this node above the switch-mode one to preserve alphabetical
> ordering (see [1])
> > +                     label =3D "Volume Up";
> > +                     gpios =3D <&pmc8280_1_gpios 6 GPIO_ACTIVE_LOW>;
> > +                     linux,code =3D <KEY_VOLUMEUP>;
> > +                     debounce-interval =3D <15>;
> > +                     linux,can-disable;
> > +                     wakeup-source;
> > +             };
> > +
>
> Stray newline
>
> [...]
>
> > +
> > +             /* s2c, s3c */
>
> Please remove such comments
>
>
Agree

[...]

>
> > +
> > +             /* /lib/firmware/ath11k/WCN6855/hw2.1/board-2.bin
> > +              * there is no calibrate data for huawei,
> > +              * but they have the same subsystem-device id
> > +              */
> > +             qcom,ath11k-calibration-variant =3D "LE_X13S";
>
> Oh, this can be taken care of! See [2], [3].
>

I have a glance, now I know we should extract something or it won't be
there.
Question is how can I extract them? I have a quick search, got no luck.
As for windows drivers, in the folder

bdwlan.e02
bdwlan.e07
bdwlan.e1d
bdwlan.e1e
bdwlan.e23
bdwlan.e26
bdwlan.e32
bdwlan.e47
bdwlan.e81
bdwlan.e84
bdwlan.e85
bdwlan.e8c
bdwlan.e8e
bdwlan.e9f
bdwlan.ea3
bdwlan.eb8
bdwlan.elf
bdwlan.elf.g
bdwlang.e8b
bdwlang.e9f
bdwlang.ea3
bdwlang.eb8
bdwlang.elf
Data20.msc
Data.msc
m320.bin
m3.bin
qcwlan8280.cat
qcwlan8280.inf
qcwlan8280.sys
regdb.bin
wlanfw20.mbn
wlanfw.mbn

[...]

> [...]
> > +
> > +&sound {
> > +     compatible =3D "qcom,sc8280xp-sndcard";
> > +     model =3D "SC8280XP-HUAWEI-MATEBOOKEGO";
> > +     audio-routing =3D
> > +             "SpkrLeft IN", "WSA_SPK1 OUT",
>
> Please remove the line break and
>
> > +             "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 =3D "WCD Playback";
> > +             cpu {
>
> Please insert a newline between the last property and subnodes.
>

Agree, but I didn't touch them, they are from x13s and crd (see [2])

> Otherwise looks fairly good!
>
> Konrad
>
> [1] https://docs.kernel.org/devicetree/bindings/dts-coding-style.html
> [2] https://lore.kernel.org/ath11k/ZwR1hu-B0bGe4zG7@localhost.localdomain/
> [3] https://git.codelinaro.org/clo/ath-firmware/ath11k-firmware

Thanks,
Pengyu

[1] https://elixir.bootlin.com/linux/v6.12.4/source/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts#L54
[2] https://elixir.bootlin.com/linux/v6.12.4/source/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts#L1125