Re: [PATCH v2 3/4] arm64: dts: qcom: Add HONOR MagicBook Art 14 device tree

[Konrad Dybcio]

On 5/15/26 7:29 PM, Konstantin Shabanov wrote:
> Introduce support for the HONOR MagicBook Art 14 laptop.
> This version is based on the initial work by Kirill A. Korinsky [1]
> and Valentin Manea [2].
> 
> Supported:
> 
> - Sound (with alsa-ucm-conf config [3])
>   - Speakers
>   - Headphone jack
> - Bluetooth
> - Battery
> - HDMI
> - Touchpad
> - Keyboard (with backlight)
> - Touchscreen
> - WiFi
> - USB-C ports
> - USB-A port
> - UFS
> - H/W accel
> - DP over USB-C
> 
> Untested:
> 
> - Camera
> - Fingerprint reader
> - Sleep/Suspend
> 
> Broken:
> 
> - eDP
> 
> [1]: https://lore.kernel.org/all/871px910m1.wl-kirill@xxxxxxxxx/
> [2]: https://github.com/vamanea/linux-magicbook/blob/x1e80100-magicbook-6.19/arch/arm64/boot/dts/qcom/x1e80100-honor-magicbook-art-14.dts
> [3]: https://github.com/alsa-project/alsa-ucm-conf/pull/755
> 
> Signed-off-by: Konstantin Shabanov <mail@xxxxxxxxxxx>
> ---

[...]

> +	sound {
> +		compatible = "qcom,x1e80100-sndcard";
> +		model = "X1E80100-CRD";

This should point to the actual name of the board, then you can make a
""symlink"" in ALSA UCM (there's a file that binds compatible soundcards
in there)

[...]

> +		wcd-playback-dai-link {
> +			link-name = "WCD Playback";
> +
> +			cpu {
> +				sound-dai = <&q6apmbedai RX_CODEC_DMA_RX_0>;
> +			};
> +
> +			codec {
> +				sound-dai = <&wcd938x 0>, <&swr1 0>, <&lpass_rxmacro 0>;
> +			};

'co'dec < 'cp'u

[...]

> +	vreg_misc_3p3: regulator-misc-3p3 {
> +		compatible = "regulator-fixed";
> +
> +		regulator-name = "VREG_MISC_3P3";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +
> +		gpio = <&pm8550ve_8_gpios 6 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&misc_3p3_reg_en>;

property-n
property-names

in this order, please

[...]

> +&iris {
> +	status = "okay";
> +	firmware-name = "qcom/x1e80100/HONOR/MRO-XXX/qcvss8380.mbn";

status should be the last property, and it would be neat to keep a \n before it

[...]

> +&mdss_dp0_out {
> +	data-lanes = <0 1>;

If you change this to <0 1 2 3>, you should get 4-lane DP working

[...]

> +	eusb5_reset_n: eusb5-reset-n-state {
> +		pins = "gpio7";
> +		function = "gpio";
> +		drive-strength = <2>;
> +		bias-disable;
> +		output-low;
> +	};
> +
> +	eusb6_reset_n: eusb6-reset-n-state {
> +		pins = "gpio184";
> +		function = "gpio";
> +		drive-strength = <2>;
> +		bias-disable;
> +		output-low;
> +	};

drop the output-foo properties from these two, they are controlled
by the driver

[...]


> +	wcd_default: wcd-reset-n-active-state {
> +		pins = "gpio191";
> +		function = "gpio";
> +		drive-strength = <16>;
> +		bias-disable;
> +		output-low;

ditto

[...]

> +/* usb1 covers left side typec ports */
> +/* back (towards the display) typec port */

This is already described in a comment near pmic-glink {}

[...]

> +/* MP0 goes to the USB-A port(USB3) and FPC */

"MP0" refers to the first USB3+USB2 port on the multiport controller.
Is there a hub inbetween? Should we describe it? Do we know if VBUS on
the USB-A port is controllable?

[...]

> +&usb_2_hsphy {

"usb_2" < "usb_mp"
> +	vdd-supply = <&vreg_l3j_0p8>;
> +	vdda12-supply = <&vreg_l2j_1p2>;
> +	phys = <&eusb5_repeater>;
> +	status = "okay";

Let's keep an \n before status

Konrad