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

["Rok Markovič" <rok@kanardia.eu>]

17.7.2026 14:13, je sashiko-bot@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 <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 {

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.