[PATCH] drm/rockchip: fix clk enable disable mismatch in vop_crtc_mode_set

[Heiko Stübner]

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 .


Heiko