On Jan 31, 2015 5:37 PM, "Heiko Stübner" <<a href="mailto:heiko@sntech.de">heiko@sntech.de</a>> 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 <<a href="mailto:heiko@sntech.de">heiko@sntech.de</a>> 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 <<a href="mailto:heiko@sntech.de">heiko@sntech.de</a>> > > > > > > --- > > > > > > 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 <<a href="mailto:djkurtz@chromium.org">djkurtz@chromium.org</a>> > > > Heiko