From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753604AbdFMR3j (ORCPT ); Tue, 13 Jun 2017 13:29:39 -0400 Received: from mail-pf0-f172.google.com ([209.85.192.172]:35063 "EHLO mail-pf0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751826AbdFMR3g (ORCPT ); Tue, 13 Jun 2017 13:29:36 -0400 Date: Tue, 13 Jun 2017 10:29:32 -0700 From: Brian Norris To: Jeffy Chen Cc: linux-kernel@vger.kernel.org, broonie@kernel.org, dianders@chromium.org, heiko@sntech.de, linux-spi@vger.kernel.org, linux-rockchip@lists.infradead.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v2 1/4] spi: rockchip: fix error handling when probe Message-ID: <20170613172931.GA9026@google.com> References: <1497331543-8565-1-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: <1497331543-8565-1-git-send-email-jeffy.chen@rock-chips.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jun 13, 2017 at 01:25:40PM +0800, Jeffy Chen wrote: > After failed to request dma tx chain, we need to disable pm_runtime. > Also cleanup error labels for better readability. > > Signed-off-by: Jeffy Chen > --- > > Changes in v2: None Looks good to me. I guess the original code was sort of trying to do a different style of error handling, where you name the labels after the point where the failure comes from (i.e., if ioremap fails, you 'goto ioremap_fail' or something like that). But since we have enough devm_* usage here that doesn't need unwound explicitly, and we didn't add extra labels for all them, then that "style" doesn't really make much sense. Reviewed-by: Brian Norris > drivers/spi/spi-rockchip.c | 27 ++++++++++++++------------- > 1 file changed, 14 insertions(+), 13 deletions(-) > > diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c > index acf31f3..bab9b13 100644 > --- a/drivers/spi/spi-rockchip.c > +++ b/drivers/spi/spi-rockchip.c > @@ -684,33 +684,33 @@ static int rockchip_spi_probe(struct platform_device *pdev) > rs->regs = devm_ioremap_resource(&pdev->dev, mem); > if (IS_ERR(rs->regs)) { > ret = PTR_ERR(rs->regs); > - goto err_ioremap_resource; > + goto err_put_master; > } > > rs->apb_pclk = devm_clk_get(&pdev->dev, "apb_pclk"); > if (IS_ERR(rs->apb_pclk)) { > dev_err(&pdev->dev, "Failed to get apb_pclk\n"); > ret = PTR_ERR(rs->apb_pclk); > - goto err_ioremap_resource; > + goto err_put_master; > } > > rs->spiclk = devm_clk_get(&pdev->dev, "spiclk"); > if (IS_ERR(rs->spiclk)) { > dev_err(&pdev->dev, "Failed to get spi_pclk\n"); > ret = PTR_ERR(rs->spiclk); > - goto err_ioremap_resource; > + goto err_put_master; > } > > ret = clk_prepare_enable(rs->apb_pclk); > if (ret) { > dev_err(&pdev->dev, "Failed to enable apb_pclk\n"); > - goto err_ioremap_resource; > + goto err_put_master; > } > > ret = clk_prepare_enable(rs->spiclk); > if (ret) { > dev_err(&pdev->dev, "Failed to enable spi_clk\n"); > - goto err_spiclk_enable; > + goto err_disable_apbclk; > } > > spi_enable_chip(rs, 0); > @@ -728,7 +728,7 @@ static int rockchip_spi_probe(struct platform_device *pdev) > if (!rs->fifo_len) { > dev_err(&pdev->dev, "Failed to get fifo length\n"); > ret = -EINVAL; > - goto err_get_fifo_len; > + goto err_disable_spiclk; > } > > spin_lock_init(&rs->lock); > @@ -755,7 +755,7 @@ static int rockchip_spi_probe(struct platform_device *pdev) > /* Check tx to see if we need defer probing driver */ > if (PTR_ERR(rs->dma_tx.ch) == -EPROBE_DEFER) { > ret = -EPROBE_DEFER; > - goto err_get_fifo_len; > + goto err_disable_pm_runtime; > } > dev_warn(rs->dev, "Failed to request TX DMA channel\n"); > rs->dma_tx.ch = NULL; > @@ -786,23 +786,24 @@ static int rockchip_spi_probe(struct platform_device *pdev) > ret = devm_spi_register_master(&pdev->dev, master); > if (ret) { > dev_err(&pdev->dev, "Failed to register master\n"); > - goto err_register_master; > + goto err_free_dma_rx; > } > > return 0; > > -err_register_master: > - pm_runtime_disable(&pdev->dev); > +err_free_dma_rx: > if (rs->dma_rx.ch) > dma_release_channel(rs->dma_rx.ch); > err_free_dma_tx: > if (rs->dma_tx.ch) > dma_release_channel(rs->dma_tx.ch); > -err_get_fifo_len: > +err_disable_pm_runtime: > + pm_runtime_disable(&pdev->dev); > +err_disable_spiclk: > clk_disable_unprepare(rs->spiclk); > -err_spiclk_enable: > +err_disable_apbclk: > clk_disable_unprepare(rs->apb_pclk); > -err_ioremap_resource: > +err_put_master: > spi_master_put(master); > > return ret; > -- > 2.1.4 > >