Re: [PATCH 03/11] arm64: dts: qcom: sc8280xp-arcata: Enable the eDP display

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The patch silently removes the `&dispcc1` status override, disabling the secondary Display Clock Controller.
- [Low] The patch alters global power behavior by implicitly removing `regulator-always-on` from `vreg_l6b`.
- [Low] Usage of the deprecated top-level `data-lanes` property in the DisplayPort controller node.
--

commit a8b912654646fb57f736aa1acf5bb9207884cc9d
Author: Jérôme de Bretagne <jerome.debretagne@gmail.com>

arm64: dts: qcom: sc8280xp-arcata: Enable the eDP display

This commit adds the vreg_edp_3p3, edp_reg_en, and mdss0_dp3 nodes to
enable the eDP-based LCD display on the Surface Pro 9 5G. It also maps
GPIO 36 for panel enable based on decompiled ACPI dumps.

Link: https://github.com/alexVinarskis/qcom-aeob-dumps/blob/master/microsoft-surface-pro-9-5G-1996/qcdxkmext8280_ARC_part2.json#L156-L168

> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-microsoft-arcata.dts b/arch/arm64/boot/dts/qcom/sc8280xp-microsoft-arcata.dts
> index 14287092fb07e..7f1cc18990025 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp-microsoft-arcata.dts
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp-microsoft-arcata.dts

[ ... ]

> @@ -361,7 +377,6 @@ vreg_l6b: ldo6 {
>  			regulator-max-microvolt = <880000>;
>  			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
>  			regulator-boot-on;
> -			regulator-always-on;	// FIXME: VDD_A_EDP_0_0P9

[Severity: Low]
Is it intentional to leave this removal out of the commit message?

The commit correctly reassigns the vreg_l6b regulator to be dynamically
managed by the eDP PHY and removes its always-on property, resolving the
FIXME comment. While logically correct, this change in the power
lifecycle is an undocumented side effect. Could this be noted in the
commit message?

>  		};
>  	};

[ ... ]

> @@ -465,10 +480,6 @@ &dispcc0 {
>  	status = "okay";
>  };
>  
> -&dispcc1 {
> -	status = "okay";
> -};
> -

[Severity: Medium]
Does this silently disable the secondary Display Clock Controller?

While this might be an intentional cleanup since mdss1 is not enabled on
this board and dispcc0 is used for the eDP, removing a major clock
controller is an implicit behavioral change. Could this modification be
explicitly documented in the commit message to prevent confusion?

>  &gpi_dma0 {
>  	status = "okay";
>  };

[ ... ]

> @@ -511,6 +522,42 @@ &mdss0_dp1_out {
>  	remote-endpoint = <&usb_1_qmpphy_dp_in>;
>  };
>  
> +&mdss0_dp3 {
> +	compatible = "qcom,sc8280xp-edp";
> +	/delete-property/ #sound-dai-cells;
> +
> +	data-lanes = <0 1 2 3>;

[Severity: Low]
Is it correct to use the data-lanes property at the top level of the
DisplayPort controller node?

According to the MSM DP controller YAML binding schema
(Documentation/devicetree/bindings/display/msm/dp-controller.yaml),
defining data-lanes at the top level is explicitly marked as deprecated.

To comply with standard graph bindings, should this property be placed
inside the controller's output endpoint instead (for example, inside the
&mdss0_dp3_out port@1/endpoint)?

> +
> +	status = "okay";
> +
> +	aux-bus {
> +		panel {
> +			compatible = "edp-panel";

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260520-surface-sp9-5g-for-next-v1-0-9df52552bf87@gmail.com?part=3