Re: [PATCH 3/4] drm/rockchip: lvds: add RK3568 support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unbalanced phy_power_off() in disable path permanently corrupts PHY state after a transient enable error.
- [Low] Silently ignored regmap_write() error in rk3568_lvds_grf_config().
--

--- Patch [3]: [PATCH 3/4] drm/rockchip: lvds: add RK3568 support ---
commit 42ee5aa3ee6adfe018b6f3294b12a2e7ab2076e3
Author: Rok Markovic <rok@kanardia.eu>
Date:   Fri Jul 17 12:00:04 2026 +0000

    drm/rockchip: lvds: add RK3568 support

This commit adds support for the RK3568 LVDS transmitter to the rockchip_lvds
driver. It configures the MIPI DSI0 D-PHY in PHY_MODE_LVDS and powers the
D-PHY from the encoder enable path instead of probe.

Link: https://lore.kernel.org/all/20230119184807.171132-1-a1ba.omarov@gmail.com/

> diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> index 150299c12298a..cc56ca2580a66 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
[ ... ]
> @@ -435,6 +435,133 @@ static void px30_lvds_encoder_disable(struct drm_encoder *encoder)
> +static int rk3568_lvds_grf_config(struct drm_encoder *encoder,
> +				  struct drm_display_mode *mode)
> +{
[ ... ]
> +	/*
> +	 * The LVDS block has its own dclk inversion select, separate from the
> +	 * VOP's pin polarity. Both have to agree with what the panel samples on.
> +	 */
> +	regmap_write(lvds->grf, RK3568_GRF_VO_CON2,
> +		     RK3568_LVDS0_DCLK_INV_SEL(negedge));
> +

[Severity: Low]
Does this code silently ignore a potential error from regmap_write()?

If this first regmap_write() to RK3568_GRF_VO_CON2 fails, the DCLK inversion
configuration is not applied, but the function proceeds and only returns the
result of the second regmap_write() below.

> +	/* Set format */
> +	return regmap_write(lvds->grf, RK3568_GRF_VO_CON0,
> +			    RK3568_LVDS0_SELECT(lvds->format) |
> +			    RK3568_LVDS0_MSBSEL(1));
> +}
> +
> +static void rk3568_lvds_encoder_enable(struct drm_encoder *encoder)
> +{
[ ... ]
> +	ret = rk3568_lvds_poweron(lvds);
> +	if (ret) {
> +		DRM_DEV_ERROR(lvds->dev, "failed to power on LVDS: %d\n", ret);
> +		drm_panel_unprepare(lvds->panel);
> +		return;
> +	}
> +
> +	ret = rk3568_lvds_grf_config(encoder, mode);
> +	if (ret) {
> +		DRM_DEV_ERROR(lvds->dev, "failed to configure LVDS: %d\n", ret);
> +		drm_panel_unprepare(lvds->panel);
> +		return;
> +	}
> +
> +	/*
> +	 * Only now bring the D-PHY up. phy_power_on() runs the whole
> +	 * inno_dsidphy_lvds_mode_enable() sequence - PLL and bandgap power-on,
[ ... ]
> +	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;
> +	}
> +

[Severity: High]
Can an early return here lead to an unbalanced phy_power_off() later?

If rk3568_lvds_encoder_enable() encounters an error and returns early before
calling phy_power_on(), the DRM core still treats the encoder as active since
the enable callback returns void.

When the display pipeline is subsequently disabled, the DRM core unconditionally
calls rk3568_lvds_encoder_disable(), which does this:

static void rk3568_lvds_encoder_disable(...)
{
    ...
    phy_power_off(lvds->dphy);
    ...
}

This would decrement the PHY's power_count from 0 to -1. Future calls to
phy_power_on() will see a negative count, skip the actual hardware power-on
routine, and just increment the counter to 0, which leaves the PHY permanently
broken until reboot.

> +	ret = phy_power_on(lvds->dphy);
> +	if (ret) {
> +		DRM_DEV_ERROR(lvds->dev, "failed to power on phy: %d\n", ret);
> +		drm_panel_unprepare(lvds->panel);
> +		return;
> +	}
> +
> +	drm_panel_enable(lvds->panel);
> +}

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