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

[Rok Markovič]

17.7.2026 14:13, je sashiko-...@kernel.org napisal

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 <r...@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 {

Fixed. Removed unused clock.

[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>;
+                       };
+               };
+       };
+

Fixed. Testing if lvds->enabled in disable.

Will post patchset v2.