Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932350AbdDDTmj (ORCPT ); Tue, 4 Apr 2017 15:42:39 -0400 Received: from mail-yw0-f174.google.com ([209.85.161.174]:34747 "EHLO mail-yw0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752935AbdDDTmh (ORCPT ); Tue, 4 Apr 2017 15:42:37 -0400 Date: Tue, 4 Apr 2017 15:42:34 -0400 From: Sean Paul To: Jeffy Chen Cc: linux-kernel@vger.kernel.org, briannorris@chromium.org, dianders@chromium.org, tfiga@chromium.org, seanpaul@chromium.org, zyw@rock-chips.com, mark.yao@rock-chips.com, Heiko Stuebner , dri-devel@lists.freedesktop.org, linux-rockchip@lists.infradead.org, David Airlie , linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v2 5/9] drm/rockchip: vop: Enable pm domain when resetting vop Message-ID: <20170404194234.vhsedpjkf6eqz4oe@art_vandelay> References: <1491046529-4310-1-git-send-email-jeffy.chen@rock-chips.com> <1491046529-4310-6-git-send-email-jeffy.chen@rock-chips.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1491046529-4310-6-git-send-email-jeffy.chen@rock-chips.com> User-Agent: NeoMutt/20170306-97-7656f1-dirty (1.8.0) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2975 Lines: 109 On Sat, Apr 01, 2017 at 07:35:25PM +0800, Jeffy Chen wrote: Hi Jeffy, Could you please add commit messages describing *why* you're making the change? It might be obvious to you (and maybe me), but others will benefit from better descriptions. > Signed-off-by: Jeffy Chen > --- > > Changes in v2: None > > drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 31 +++++++++++++++++++---------- > 1 file changed, 21 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > index 76c79ac..1d85319 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c > @@ -1365,6 +1365,12 @@ static int vop_initial(struct vop *vop) > return ret; > } > > + ret = pm_runtime_get_sync(vop->dev); > + if (ret < 0) { > + dev_err(vop->dev, "failed to get pm runtime: %d\n", ret); > + goto err_put_pm_runtime; This should go to err_unprepare_dclk, the next error condition should go to err_put_pm_runtime > + } > + > /* Enable both the hclk and aclk to setup the vop */ > ret = clk_prepare_enable(vop->hclk); > if (ret < 0) { > @@ -1422,6 +1428,8 @@ static int vop_initial(struct vop *vop) > > vop->is_enabled = false; > > + pm_runtime_put_sync(vop->dev); > + > return 0; > > err_disable_aclk: > @@ -1430,6 +1438,8 @@ static int vop_initial(struct vop *vop) > clk_disable_unprepare(vop->hclk); > err_unprepare_dclk: > clk_unprepare(vop->dclk); > +err_put_pm_runtime: > + pm_runtime_put_sync(vop->dev); Should be above err_unprepare_dclk > return ret; > } > > @@ -1530,12 +1540,6 @@ static int vop_bind(struct device *dev, struct device *master, void *data) > if (!vop->regsbak) > return -ENOMEM; > > - ret = vop_initial(vop); > - if (ret < 0) { > - dev_err(&pdev->dev, "cannot initial vop dev - err %d\n", ret); > - return ret; > - } > - > irq = platform_get_irq(pdev, 0); > if (irq < 0) { > dev_err(dev, "cannot find irq for vop\n"); > @@ -1556,15 +1560,22 @@ static int vop_bind(struct device *dev, struct device *master, void *data) > /* IRQ is initially disabled; it gets enabled in power_on */ > disable_irq(vop->irq); > > + pm_runtime_enable(&pdev->dev); > + > + ret = vop_initial(vop); > + if (ret < 0) { > + dev_err(&pdev->dev, "cannot initial vop dev - err %d\n", ret); > + goto err_disable_pm_runtime; > + } This function move would be a lot easier to understand with a commit message ;-) > + > ret = vop_create_crtc(vop); > if (ret) > - goto err_enable_irq; > - > - pm_runtime_enable(&pdev->dev); > + goto err_disable_pm_runtime; Do you have anything to clean up from vop_initial()? > > return 0; > > -err_enable_irq: > +err_disable_pm_runtime: > + pm_runtime_disable(&pdev->dev); > enable_irq(vop->irq); /* To balance out the disable_irq above */ > return ret; > } > -- > 2.1.4 > -- Sean Paul, Software Engineer, Google / Chromium OS