Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] Meaningful code and commit message mismatch regarding error cleanup --
commit eec31d58dcee33de416927375afdb370e4851289 Author: Radiy Zhambakiev <[email protected]> drm/bridge: tc358767: check regmap_write() return values This commit propagates error values from regmap_write() in tc_set_edp_video_mode() and tc_main_link_enable(). The commit message asserts that the callers of both functions already handle failures by disabling the main link via tc_main_link_disable(). > Propagate the errors instead. The callers of both functions already > handle failures by disabling the main link via tc_main_link_disable(). [Severity: Medium] Is this description accurate for both functions? While the caller of tc_set_edp_video_mode() handles failures by calling tc_main_link_disable(), it looks like the caller of tc_main_link_enable() does not. If tc_main_link_enable() fails, its caller tc_edp_bridge_atomic_enable() only logs the error and returns: drivers/gpu/drm/bridge/tc358767.c:tc_edp_bridge_atomic_enable() { ... ret = tc_main_link_enable(tc); if (ret < 0) { dev_err(tc->dev, "main link enable error: %d\n", ret); return; } ... } Will this mismatch leave the hardware in a partially initialized state upon I2C failure if tc_main_link_enable() returns early? > diff --git a/drivers/gpu/drm/bridge/tc358767.c > b/drivers/gpu/drm/bridge/tc358767.c > index e3a57f8228dae..a98a48534bf60 100644 > --- a/drivers/gpu/drm/bridge/tc358767.c > +++ b/drivers/gpu/drm/bridge/tc358767.c [ ... ] > @@ -1144,9 +1146,14 @@ static int tc_main_link_enable(struct tc_data *tc) > /* Reset/Enable Main Links */ > dp_phy_ctrl |= DP_PHY_RST | PHY_M1_RST | PHY_M0_RST; > ret = regmap_write(tc->regmap, DP_PHY_CTRL, dp_phy_ctrl); > + if (ret) > + return ret; > + [Severity: Medium] Does returning an error here rely on the caller performing cleanup that it currently lacks? > usleep_range(100, 200); > dp_phy_ctrl &= ~(DP_PHY_RST | PHY_M1_RST | PHY_M0_RST); > ret = regmap_write(tc->regmap, DP_PHY_CTRL, dp_phy_ctrl); > + if (ret) > + return ret; > > ret = tc_poll_timeout(tc, DP_PHY_CTRL, PHY_RDY, PHY_RDY, 500, 100000); > if (ret) { -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
