Re: [PATCH v9 4/4] arm64: dts: qcom: Add Lenovo ThinkBook 16 G7 QOY device tree

[Jens Glathe]

On 30.07.25 10:50, Konrad Dybcio wrote:
> On 7/1/25 7:02 PM, Jens Glathe via B4 Relay wrote:
> >   
> > -		pm8010-thermal {
> > +		pm8010_thermal: pm8010-thermal {
> Let's rebase on:
>
> https://lore.kernel.org/linux-arm-msm/20250701183625.1968246-1-alex.vinarskis@xxxxxxxxx/

Done.

> > +		wcd-playback-dai-link {
> > +			link-name = "WCD Playback";
> > +
> > +			cpu {
> > +				sound-dai = <&q6apmbedai RX_CODEC_DMA_RX_0>;
> > +			};
> > +
> > +			codec {
> 'co'dec < 'cp'u
Done and checked
> > +&i2c2 {
> > +	clock-frequency = <400000>;
> > +
> > +	pinctrl-0 = <&qup_i2c2_data_clk>, <&tpad_default>, <&kybd_default>;
> > +	pinctrl-names = "default";
> > +	status = "okay";
> > +
> > +	/* ELAN06FA */
> > +	touchpad@15 {
> 5 touchpad variants is a lot. Does DSDT give any clues as to whether
> all of them are plausibly present on production hw?
I took sort of a survey among users, and we got mostly @2c (CIRQ1080 or 
SYNA2BA6) and one with @38  (FTCS0038). All the others are defined and 
look like wired up in the dsdt. There's also  3 display panel variants 
defined, all with the same totally odd pwm backlight mechanism according 
to the dsdt-embedded xml structures for these. Survey showed all the 
same panel as mine, except the one with the diverging touchpad. The 
sample size is ~5, so not really representative.
> > +&mdss_dp0_out {
> > +	data-lanes = <0 1 2 3>;
> > +	link-frequencies = /bits/ 64 <1620000000 2700000000 5400000000 8100000000>;
> > +};
> > +
> > +&mdss_dp1 {
> > +	status = "okay";
> > +};
> > +
> > +&mdss_dp1_out {
> > +	data-lanes = <0 1 2 3>;
> Does defining all 4 lanes not upset the driver without additional
> patches?
Yes, corrected, thank you.
> > +	link-frequencies = /bits/ 64 <1620000000 2700000000 5400000000 8100000000>;
> > +};
> > +
> > +&mdss_dp2 {
> > +	status = "okay";
> > +};
> > +
> > +&mdss_dp2_out {
> > +	data-lanes = <0 1 2 3>;
> > +	link-frequencies = /bits/ 64 <1620000000 2700000000 5400000000 8100000000>;
> > +};
> > +
> > +&mdss_dp2_phy {
> > +//	vdda-phy-supply = <&vreg_l3b>;
> > +// 	vdda-pll-supply = <&vreg_l6b>;
> The regulators are usecase-specialized, feel free to trust what
> is present on the CRD

Removed mdss_dp2 since it's not working reliably yet.

> > +	ports {
> > +		port@1 {
> > +			reg = <1>;
> > +
> > +			mdss_dp3_out: endpoint {
> > +				data-lanes = <0 1 2 3>;
> > +				link-frequencies = /bits/ 64 <1620000000 2700000000 5400000000 8100000000>;
> > +
> > +				remote-endpoint = <&edp_panel_in>;
> > +			};
> > +		};
> > +	};
> > +};
> Please rebase on:
>
> https://lore.kernel.org/linux-arm-msm/20250724-move-edp-endpoints-v1-3-6ca569812838@xxxxxxxxxxxxxxxx/
Done.
> > +&sdhc_2 {
> > +	cd-gpios = <&tlmm 71 GPIO_ACTIVE_LOW>;
> > +	pinctrl-0 = <&sdc2_default &sdc2_card_det_n>;
> > +	pinctrl-1 = <&sdc2_sleep &sdc2_card_det_n>;
> > +	pinctrl-names = "default", "sleep";
> > +	vmmc-supply = <&vreg_l9b_2p9>;
> > +	vqmmc-supply = <&vreg_l6b_1p8>;
> > +//	bus-width = <4>;
> > +//	no-sdio;
> > +//	no-mmc;
> Please either remove the '//' or remove the lines
Done. I have an idea which driver it should be (BayHubTech driver for 
the PCI variant) since the Ideapad 5 has actually both variants - the 
Slim 5x on pcie3, and the 2in1 on sdhc_2. The Windows driver supports 
both interfaces.
> > +&tlmm {
> > +	gpio-reserved-ranges = <34 2>, /* Unused */
> > +			       <72 2>, /* Secure EC I2C connection (?) */
> > +			       <238 1>; /* UFS Reset */
> Try removing the UFS reset block and see if the platform still
> boots, this turned out to be a false flag on CRD
Done, still working.
> I'd also suggest removing voltage suffixes from regulator names (i.e.
> turn them to e.g. vreg_l6b).
I'm not really a fan of this. The X1E dtbs all have the voltage suffix, 
and it is actually quite human-readable. Which of course is not the goal 
of a dts, but well, as a human writing these I like some orientation help.

with best regards

Jens