Re: [PATCH v2] drm/bridge: tc358767: check regmap_write() return values

[sashiko-bot@kernel.org]

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 <r.zhambakiev@prosoftsystems.ru>

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/20260826132251.113084-1-r.zhambakiev@prosoftsystems.ru?part=1