Re: [PATCH 3/3] arm64: dts: qcom: Add Lenovo IdeaPad Slim 5x 14Q8X9

[Jens Glathe]

Hi Konrad,

On 07.09.26 11:14, Konrad Dybcio wrote:
> On 9/6/26 2:22 PM, Jens Glathe via B4 Relay wrote:
> > From: Jens Glathe <jens.glathe@xxxxxxxxxxxxxxxxxxxxxx>
> >
> > Add a common DTSI plus LCD and OLED board files for the Lenovo
> > IdeaPad Slim 5 14Q8X9 (machine type 83HL) on X1P42100. In some
> > regions this SKU is sold as "IdeaPad Slim 5x"; the "x" is the
> > Qualcomm suffix in Lenovo's current IdeaPad naming (i = Intel,
> > a = AMD, x = Qualcomm).
> [...]
>
> > +	hdmi-connector {
> > +		compatible = "hdmi-connector";
> > +		type = "a";
> > +
> > +		port {
> > +			hdmi_con: endpoint {
> > +				remote-endpoint = <&hdmi_bridge_tmds_out>;
> > +			};
> > +		};
> > +	};
> > +
> > +	gpio-keys {
> nit: 'g' < 'h'

yep, will change in V2


> [...]
>
> > +	sound {
> > +		compatible = "qcom,x1e80100-sndcard";
> > +		model = "X1E80100-LENOVO-Ideapad-5";
> > +		audio-routing = "SpkrLeft IN", "WSA WSA_SPK1 OUT",
> > +				"SpkrRight IN", "WSA WSA_SPK2 OUT",
> > +				"IN1_HPHL", "HPHL_OUT",
> > +				"IN2_HPHR", "HPHR_OUT",
> > +				"AMIC2", "MIC BIAS2",
> > +				"VA DMIC0", "MIC BIAS1",
> > +				"VA DMIC1", "MIC BIAS1",
> > +				"VA DMIC0", "VA MIC BIAS1",
> > +				"VA DMIC1", "VA MIC BIAS1",
> https://lore.kernel.org/linux-arm-msm/20241203-x1e80100-va-mic-bias-v1-2-0dfd4d9b492c@xxxxxxxxxx/

Yes will change that in V2. Thanks for the link.


> [...]
>
> > +		vreg_l4b_1p8: ldo4 {
> > +			regulator-name = "vreg_l4b_1p8";
> > +			regulator-min-microvolt = <1800000>;
> > +			regulator-max-microvolt = <1800000>;
> > +			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> > +
> > +			regulator-state-mem {
> > +				regulator-on-in-suspend;
> > +			};
> Why is this necessary?

I had a debug session on the Thinkbook 16 with Grok where I chased stuck 
mouse via USB dongle after long suspend. This is what resulted from 
this, PTN3222 losing its state on deep suspend when the vreg shuts off. 
And it solved the problem, propagated it over to the Ideapad 5 device 
trees. I also observed that on the Ideapad 5 2in1 before. Never seen on 
the Slim5x, but I got that one after the change was in. So, yes, IMO.

> [...]
>
> > +&i2c1 {
> > +	clock-frequency = <400000>;
> > +
> > +	status = "okay";
> > +
> > +	/* 3d PS883x */
> Any reason not to wire it up now?
>
> [...]
>
> > +&iris {
> > +	firmware-name = "qcom/x1p42100/LENOVO/83HL/qcvss8380.mbn";
> > +	status = "okay";
> nit: \n before status, please
>
> [...]
>
> > +&spi10 {
> > +	status = "disabled";
> > +
> > +	/* Unknown device */
> > +};
> Either enable it, or remove it, I think
>
> > +
> > +&swr0 {
> > +	status = "okay";
> > +
> > +	pinctrl-0 = <&wsa_swr_active>, <&spkr_01_sd_n_active>;
> > +	pinctrl-names = "default";
> 'status' should go last
>
> [...]
>
> > +&tlmm {
> > +	gpio-reserved-ranges = <34 2>, /* Unused */
> > +		<44 4>,
> > +		<72 2>; /* Secure EC I2C connection (?) */
> Please align the '<'s
>
> > +
> > +	eusb3_reset_n: eusb3-reset-n-state {
> > +		pins = "gpio6";
> > +		function = "gpio";
> > +		drive-strength = <2>;
> > +		bias-disable;
> > +		output-low;
> You don't need the output- properties on the TLMM pins
>
> [...]
>
> > +&usb_1_ss2 {
> > +	dr_mode = "host";
> > +	maximum-speed = "high-speed";
> > +	phys = <&usb_1_ss2_hsphy>;
> > +	phy-names = "usb2-phy";
> > +	qcom,select-utmi-as-pipe-clk;
> > +	status = "okay";
> > +	/delete-property/ port@1;
> nit: status last, please

Will amend all these in V2, too. Thanks for the review.

with best regards

Jens