Re: [PATCH 2/6] arm64: dts: qcom: sm8250-hdk: Add LT9611UXC HDMI bridge

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] arm64: dts: qcom: sm8250-hdk: pinctrl node lt9611-pins violates -state schema
- [Medium] arm64: dts: qcom: sm8250-hdk: unlinked mdss_dp and unrelated adsp/cdsp nodes
--

commit b1dadd1c8b42a901f8e62f9b3d2367c950faf2f4
Author: redstonee <1516235445@qq.com>

arm64: dts: qcom: sm8250-hdk: Add LT9611UXC HDMI bridge

This commit enables the LT9611UXC MIPI DSI-to-HDMI bridge on the SM8250 HDK
along with its regulator, HDMI connector, I2C bus, pinctrl state, and MDSS/DSI
nodes, as well as the ADSP, CDSP, and DisplayPort controller nodes.

> diff --git a/arch/arm64/boot/dts/qcom/sm8250-hdk.dts b/arch/arm64/boot/dts/qcom/sm8250-hdk.dts
> index 5975878ea0fd2..d92e569d87410 100644
> --- a/arch/arm64/boot/dts/qcom/sm8250-hdk.dts
> +++ b/arch/arm64/boot/dts/qcom/sm8250-hdk.dts

[ ... ]

> @@ -404,6 +431,20 @@ sd_ufs_cd_pins: sd-ufs-cd-pins {
>  		function = "gpio";
>  		bias-pull-up;
>  	};
> +
> +	lt9611_pins: lt9611-pins {
> +		irq-pins {
> +			pins = "gpio1";
> +			function = "gpio";
> +			bias-disable;
> +		};
> +
> +		rst-pins {
> +			pins = "gpio2";
> +			function = "gpio";
> +			output-high;
> +		};
> +	};
>  };
>  
>  &uart12 {

[Severity: Medium]
Should the lt9611-pins node under &tlmm (as well as sd-ufs-cd-pins from the
preceding commit) be named with a -state suffix, such as lt9611-default-state
or lt9611-state?

In arch/arm64/boot/dts/qcom/sm8250.dtsi, tlmm uses the qcom,sm8250-pinctrl
compatible string. The binding in
Documentation/devicetree/bindings/pinctrl/qcom,sm8250-pinctrl.yaml sets
unevaluatedProperties: false and restricts top-level pin configuration child
nodes to names ending in -state, while -pins is only matched for subnodes
nested inside a -state parent node:

patternProperties:
  "-state$":
    oneOf:
      - $ref: "#/$defs/qcom-sm8250-tlmm-state"
      - patternProperties:
          "-pins$":
            $ref: "#/$defs/qcom-sm8250-tlmm-state"
        additionalProperties: false

Naming the top-level child node lt9611-pins causes dtbs_check to fail on
&tlmm and skips validation of the nested irq-pins and rst-pins subnodes.

> @@ -486,3 +527,85 @@ &sdhc_2 {
>  	pinctrl-0 = <&sdc2_default_state>, <&sd_ufs_cd_pins>;
>  	pinctrl-1 = <&sdc2_sleep_state>, <&sd_ufs_cd_pins>;
>  };
> +
> +&adsp {
> +	status = "okay";
> +	firmware-name = "qcom/sm8250/adsp.mbn";
> +};
> +
> +&cdsp {
> +	status = "okay";
> +	firmware-name = "qcom/sm8250/cdsp.mbn";
> +};

[ ... ]

> +&mdss {
> +	status = "okay";
> +};
> +
> +&mdss_dp {
> +	status = "okay";
> +};
> +
> +&mdss_dp_out {
> +	data-lanes = <0 1>;
> +};

[Severity: Medium]
Were &adsp, &cdsp, &mdss_dp, and &mdss_dp_out intended to be included in this
LT9611UXC HDMI bridge patch?

The &adsp and &cdsp remoteproc nodes are unrelated to the MIPI DSI-to-HDMI
bridge and are not mentioned in the commit description.

Additionally, enabling &mdss_dp here before connecting &mdss_dp_out to
usb_1_qmpphy_dp_in (which happens in the follow-up USB-C patch "arm64: dts:
qcom: sm8250-hdk: Add FSA4480 USB-C protocol chip") leaves port@1 of mdss_dp
without a remote-endpoint:

arch/arm64/boot/dts/qcom/sm8250.dtsi:
    port@1 {
        reg = <1>;

        mdss_dp_out: endpoint {
        };
    };

Because add_mdp_components() in drivers/gpu/drm/msm/msm_drv.c skips disabled
interfaces via of_device_is_available(intf), &mdss and &mdss_dsi0 do not
require &mdss_dp to be enabled for the HDMI bridge. When &mdss_dp probes
without a remote-endpoint on port@1, msm_dp_display_probe_tail() in
drivers/gpu/drm/msm/dp/dp_display.c sets dp->next_bridge = NULL instead of
attaching the QMP PHY AUX and Type-C HPD bridges:

    dp->next_bridge = devm_drm_of_get_bridge(&dp->pdev->dev, dp->pdev->dev.of_node, 1, 0);
    if (IS_ERR(dp->next_bridge)) {
        ret = PTR_ERR(dp->next_bridge);
        dp->next_bridge = NULL;
        if (dp->is_edp || ret != -ENODEV)
            return ret;
    }

drm_bridge_connector_init() then falls back to msm_dp_bridge as the HPD
provider and calls msm_dp_bridge_hpd_enable(), which calls
pm_runtime_resume_and_get() and enables internal HPD via
msm_dp_aux_hpd_enable(), keeping the DP controller and RPMHPD_MMCX power
domain permanently resumed.

Should &mdss_dp and &mdss_dp_out be moved to the follow-up USB-C patch where
the DisplayPort graph is connected, and &adsp and &cdsp be split into a
separate commit?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022113601.2320940-1-dc_redstone@outlook.com?part=2