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