Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757485AbcCUXdf (ORCPT ); Mon, 21 Mar 2016 19:33:35 -0400 Received: from mail-vk0-f54.google.com ([209.85.213.54]:36436 "EHLO mail-vk0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754653AbcCUXde (ORCPT ); Mon, 21 Mar 2016 19:33:34 -0400 MIME-Version: 1.0 In-Reply-To: <1457511092-2216-1-git-send-email-shawn.lin@rock-chips.com> References: <1457511043-2058-1-git-send-email-shawn.lin@rock-chips.com> <1457511092-2216-1-git-send-email-shawn.lin@rock-chips.com> Date: Mon, 21 Mar 2016 16:33:32 -0700 X-Google-Sender-Auth: kciT5V4BeZzF6Zt3QItNw_6xfeM Message-ID: Subject: Re: [PATCH 3/3] spi: rockchip: check requesting dma channel with EPROBE_DEFER From: Doug Anderson To: Shawn Lin Cc: Mark Brown , linux-spi@vger.kernel.org, "linux-kernel@vger.kernel.org" , Dan Carpenter , "open list:ARM/Rockchip SoC..." Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2808 Lines: 67 Shawn, On Wed, Mar 9, 2016 at 12:11 AM, Shawn Lin wrote: > Let's defer probing the driver if the return value of > dma_request_slave_channel is ERR_PTR(-EPROBE_DEFER) instead > of disabling dma capability directly. > > Signed-off-by: Shawn Lin > --- > > drivers/spi/spi-rockchip.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c > index ca4f4e0..75fa990 100644 > --- a/drivers/spi/spi-rockchip.c > +++ b/drivers/spi/spi-rockchip.c > @@ -737,8 +737,14 @@ static int rockchip_spi_probe(struct platform_device *pdev) > master->handle_err = rockchip_spi_handle_err; > > rs->dma_tx.ch = dma_request_slave_channel(rs->dev, "tx"); > - if (!rs->dma_tx.ch) > + if (IS_ERR_OR_NULL(rs->dma_tx.ch)) { > + /* 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; > + } > dev_warn(rs->dev, "Failed to request TX DMA channel\n"); Presumably Dan would be happy if you just add this right after the dev_warn(): rs->dma_tx.ch = NULL; Presumably from Dan's email it would also be wise to make sure you don't pass NULL to PTR_ERR, which you could probably do by just using ERR_PTR instead of PTR_ERR. I think you could structure like this: rs->dma_tx.ch = dma_request_slave_channel(rs->dev, "tx"); - if (!rs->dma_tx.ch) + if (IS_ERR_OR_NULL(rs->dma_tx.ch)) { + /* Check tx to see if we need defer probing driver */ + if (rs->dma_tx.ch == ERR_PTR(-EPROBE_DEFER)) { + ret = -EPROBE_DEFER; + goto err_get_fifo_len; + } dev_warn(rs->dev, "Failed to request TX DMA channel\n"); + rs->dma_tx.ch = NULL; + } With that change your patch should be happy, I think. If some new unknown error return gets added to dma_request_slave_channel() then your code will continue to work properly. Such a change is simple and safe, so presumably you could just spin your patch with that fix. Although unlikely, it's probably good to check for IS_ERR_OR_NULL() when requesting the "rx" channel too. ...but, looking at this, presumably before landing any patch that made dma_request_slave_channel() return -EPROBE_DEFER you'd need to modify _all_ users of dma_request_slave_channel to handle error pointers being returned. Right now dma_request_slave_channel() says it returns a pointer to a channel or NULL and the function explicitly avoids returning any errors. That might be possible, but it's a big change... -Doug