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

[Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>]

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?
> 
> 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@gmail.com>
> Signed-off-by: Xuecong Chen <chenxuecong2009@outlook.com>

Your commit message suggests Co-developed-by: tags would also be
fitting here

[...]

> +	chosen {
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		framebuffer0: framebuffer@c6200000 {
> +			compatible = "simple-framebuffer";
> +			reg = <0x0 0xC6200000 0x0 0x02400000>;
> +			width = <1600>;
> +			height = <2560>;
> +			stride = <(1600 * 4)>;
> +			format = "a8r8g8b8";
> +		};
> +	};

This should be redundant, as you should have efifb

[...]

> +
> +	wcd938x: audio-codec {
> +		compatible = "qcom,wcd9380-codec";
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&wcd_default>;

Please follow this order throughout the file:

property-n
property-names

[...]

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

This could use a label too - "Tablet Mode Switch", perhaps?

> +			linux,input-type = <EV_SW>;
> +			linux,code = <SW_TABLET_MODE>;
> +			debounce-interval = <10>;
> +			wakeup-source;
> +		};
> +
> +		key-vol-up {

Please place this node above the switch-mode one to preserve alphabetical
ordering (see [1])
> +			label = "Volume Up";
> +			gpios = <&pmc8280_1_gpios 6 GPIO_ACTIVE_LOW>;
> +			linux,code = <KEY_VOLUMEUP>;
> +			debounce-interval = <15>;
> +			linux,can-disable;
> +			wakeup-source;
> +		};
> +

Stray newline

[...]

> +
> +		/* s2c, s3c */

Please remove such comments

[...]

> +
> +		/* /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 = "LE_X13S";

Oh, this can be taken care of! See [2], [3].

[...]
> +
> +&sound {
> +	compatible = "qcom,sc8280xp-sndcard";
> +	model = "SC8280XP-HUAWEI-MATEBOOKEGO";
> +	audio-routing =
> +		"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 = "WCD Playback";
> +		cpu {

Please insert a newline between the last property and subnodes.

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