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@xxxxxxxxx> 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@xxxxxxxxx>
> >
> > ---
> >
> > 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
