On Sat, Jan 31, 2015 at 5:43 PM, Heiko St?bner <heiko at sntech.de> wrote: > Am Samstag, 31. Januar 2015, 13:43:23 schrieb Daniel Kurtz: >> Hi Heiko, >> >> On Sat, Jan 31, 2015 at 3:28 AM, Heiko St?bner <heiko at sntech.de> wrote: >> > The function disables the dclk at the beginning, so don't simply return >> > when an error happens, but instead enable the clock again, so that >> > enable and disable calls are balanced. >> >> This function is a bit of a disaster, and needs fixing some day. >> AFAICT, the dclk_rst is actually ignored if dclk is disabled, so that >> part doesn't even work. >> >> > ret_clk is introduced to hold the clk_enable result and not mangle the >> > original error code. >> > >> > Signed-off-by: Heiko Stuebner <heiko at sntech.de> >> > >> > --- >> > >> > drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 18 ++++++++++-------- >> > 1 file changed, 10 insertions(+), 8 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c >> > b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index 04b619a..c0387f7 >> > 100644 >> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c >> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c >> > @@ -852,7 +852,7 @@ static int vop_crtc_mode_set(struct drm_crtc *crtc, >> > >> > u16 vsync_len = adjusted_mode->vsync_end - >> > adjusted_mode->vsync_start; >> > u16 vact_st = adjusted_mode->vtotal - adjusted_mode->vsync_start; >> > u16 vact_end = vact_st + vdisplay; >> > >> > - int ret; >> > + int ret, ret_clk; >> > >> > uint32_t val; >> > >> > /* >> > >> > @@ -874,7 +874,8 @@ static int vop_crtc_mode_set(struct drm_crtc *crtc, >> > >> > default: >> > DRM_ERROR("unsupport connector_type[%d]\n", >> > >> > vop->connector_type); >> > >> > - return -EINVAL; >> > + ret = -EINVAL; >> > + goto out; >> > >> > }; >> > VOP_CTRL_SET(vop, out_mode, vop->connector_out_mode); >> > >> > @@ -897,7 +898,7 @@ static int vop_crtc_mode_set(struct drm_crtc *crtc, >> > >> > ret = vop_crtc_mode_set_base(crtc, x, y, fb); >> > if (ret) >> > >> > - return ret; >> > + goto out; >> > >> > /* >> > >> > * reset dclk, take all mode config affect, so the clk would run >> > in >> > >> > @@ -908,13 +909,14 @@ static int vop_crtc_mode_set(struct drm_crtc *crtc, >> > >> > reset_control_deassert(vop->dclk_rst); >> > >> > clk_set_rate(vop->dclk, adjusted_mode->clock * 1000); >> > >> > - ret = clk_enable(vop->dclk); >> > - if (ret < 0) { >> > - dev_err(vop->dev, "failed to enable dclk - %d\n", ret); >> > - return ret; >> > +out: >> > + ret_clk = clk_enable(vop->dclk); >> > + if (ret_clk < 0) { >> > + dev_err(vop->dev, "failed to enable dclk - %d\n", >> > ret_clk); >> > + return ret_clk; >> >> Doesn't this swallow ret ? I thought the point was to still return >> the original error? > > I think if even the reenabling of the clock fails, there must be something > _really_ wrong with the system [enabled before and all] - so if this also > returns an error after the core functionality failed already, it doesn't > really matter anymore which error we return :-) . > > The original point was to not overwrite the actual error (in ret) in the case > where clk_enable simply returns 0 . Yeah, that makes more sense :-). Reviewed-by: Daniel Kurtz <djkurtz at chromium.org> > > > Heiko