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 <[email protected]> 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/[email protected]?part=4
