Re: [PATCH 4/4] arm64: dts: rockchip: rk356x: add LVDS node

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing Hardware Clock Initialization for RK3568 LVDS
- [Medium] Unbalanced Hardware Resource Teardown in Error Paths
--

--- Patch [4]: [PATCH 4/4] arm64: dts: rockchip: rk356x: add LVDS node ---
commit bfe12cef9ca639b004d58e050ef9175743345f7b
Author: Rok Markovic <rok@kanardia.eu>

arm64: dts: rockchip: rk356x: add LVDS node

This commit adds the LVDS transmitter node for the RK356x SoC, assigning
its clock, physical interface, and power domains. It is left disabled by
default, to be enabled by specific board files.

> diff --git a/arch/arm64/boot/dts/rockchip/rk356x-base.dtsi b/arch/arm64/boot/dts/rockchip/rk356x-base.dtsi
> index a5832895bd392..2b3beab5507b1 100644
> --- a/arch/arm64/boot/dts/rockchip/rk356x-base.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk356x-base.dtsi
> @@ -879,6 +879,31 @@ hdmi_out: port@1 {
>  		};
>  	};
>  
> +	lvds: lvds {
> +		compatible = "rockchip,rk3568-lvds";
> +		clocks = <&cru PCLK_DSITX_0>;
> +		clock-names = "pclk_lvds";

[Severity: High]
Does the driver actually use this clock?

Looking at rk3568_lvds_probe() in drivers/gpu/drm/rockchip/rockchip_lvds.c,
it seems to completely omit fetching the required pclk_lvds clock from the
device tree:

drivers/gpu/drm/rockchip/rockchip_lvds.c:rk3568_lvds_probe() {
    ...
    lvds->dphy = devm_phy_get(&pdev->dev, "dphy");
    if (IS_ERR(lvds->dphy))
        return PTR_ERR(lvds->dphy);

    return phy_init(lvds->dphy);
}

Because lvds->pclk is left NULL, the clk_enable() call in
rk3568_lvds_poweron() acts as a no-op:

drivers/gpu/drm/rockchip/rockchip_lvds.c:rk3568_lvds_poweron() {
    ...
    ret = clk_enable(lvds->pclk);
    if (ret < 0) {
    ...
}

Will this cause hardware registers to be accessed without a running clock,
potentially leading to system hangs on device probe or modeset?

> +		phys = <&dsi_dphy0>;
> +		phy-names = "dphy";
> +		power-domains = <&power RK3568_PD_VO>;
> +		rockchip,grf = <&grf>;
> +		rockchip,output = "lvds";
> +		status = "disabled";
> +
> +		ports {

[Severity: Medium]
Will enabling this hardware expose an unbalanced resource teardown in the
driver's error paths?

If intermediate steps like phy_set_mode() fail in
rk3568_lvds_encoder_enable(), the function returns early without rolling
back earlier successful calls:

drivers/gpu/drm/rockchip/rockchip_lvds.c:rk3568_lvds_encoder_enable() {
    ...
    ret = phy_set_mode(lvds->dphy, PHY_MODE_LVDS);
    if (ret) {
        DRM_DEV_ERROR(lvds->dev, "failed to set phy mode: %d\n", ret);
        drm_panel_unprepare(lvds->panel);
        return;
    }
    ...
}

Because .enable() returns void, the DRM core assumes it succeeded and will
later call .disable(). The rk3568_lvds_encoder_disable() function
unconditionally tears down all resources:

drivers/gpu/drm/rockchip/rockchip_lvds.c:rk3568_lvds_encoder_disable() {
    struct rockchip_lvds *lvds = encoder_to_lvds(encoder);

    drm_panel_disable(lvds->panel);
    phy_power_off(lvds->dphy);
    rk3568_lvds_poweroff(lvds);
    drm_panel_unprepare(lvds->panel);
}

Could this cause refcount underflows for PHY, PM runtime, and clocks,
resulting in kernel warnings if they were never successfully enabled?

> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			lvds_in: port@0 {
> +				reg = <0>;
> +			};
> +
> +			lvds_out: port@1 {
> +				reg = <1>;
> +			};
> +		};
> +	};
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717120005.2087386-1-rok@kanardia.eu?part=4