diff mbox series

[v3,1/1] i2c: imx: refactor error handling on i2c_imx_dma_request()

Message ID 20181130094418.17700-1-o.rempel@pengutronix.de (mailing list archive)
State New, archived
Headers show
Series [v3,1/1] i2c: imx: refactor error handling on i2c_imx_dma_request() | expand

Commit Message

Oleksij Rempel Nov. 30, 2018, 9:44 a.m. UTC
From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Current implementation will print error if system is not configured
with DMA for I2C core. At least on i.MX5x variants is DMA event for I2C
muxed with SDHC. So it is project specific configuration and not an error.

This patch will also affect probe, since it will forward all error except
of missing DMA.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
[ore: make it actually compile, fix a corner case]
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
changes:
20181130 v3:
 - reword commit message 
20181112 v2:
 - drop "[PATCH v1 2/3] i2c: imx: probe dma only only on i.MX50 and
   later" and rebase without this patch.
 - Set proper initial author

 drivers/i2c/busses/i2c-imx.c | 36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)

Comments

Uwe Kleine-König Nov. 30, 2018, 9:52 a.m. UTC | #1
On Fri, Nov 30, 2018 at 10:44:18AM +0100, Oleksij Rempel wrote:
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> Current implementation will print error if system is not configured
> with DMA for I2C core. At least on i.MX5x variants is DMA event for I2C
> muxed with SDHC. So it is project specific configuration and not an error.
> 
> This patch will also affect probe, since it will forward all error except
> of missing DMA.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> [ore: make it actually compile, fix a corner case]
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
> changes:
> 20181130 v3:
>  - reword commit message 
> 20181112 v2:
>  - drop "[PATCH v1 2/3] i2c: imx: probe dma only only on i.MX50 and
>    later" and rebase without this patch.
>  - Set proper initial author
> 
>  drivers/i2c/busses/i2c-imx.c | 36 +++++++++++++++++++++---------------
>  1 file changed, 21 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index c406700789e1..3c12b174bbb1 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -273,7 +273,7 @@ static inline unsigned char imx_i2c_read_reg(struct imx_i2c_struct *i2c_imx,
>  }
>  
>  /* Functions for DMA support */
> -static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
> +static int i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
>  						dma_addr_t phy_addr)

If you touch the line above, maybe also realign the followup line to
start at the opening parenthesis.

>  {
>  	struct imx_i2c_dma *dma;
> @@ -283,11 +283,13 @@ static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
>  
>  	dma = devm_kzalloc(dev, sizeof(*dma), GFP_KERNEL);
>  	if (!dma)
> -		return;
> +		return -ENOMEM;
>  
> -	dma->chan_tx = dma_request_slave_channel(dev, "tx");
> +	dma->chan_tx = dma_request_chan(dev, "tx");

Maybe this is worth a separate change?

Best regards
Uwe
Uwe Kleine-König Dec. 21, 2018, 3:24 p.m. UTC | #2
Hello,

On Fri, Nov 30, 2018 at 10:44:18AM +0100, Oleksij Rempel wrote:
> @@ -1159,11 +1163,13 @@ static int i2c_imx_probe(struct platform_device *pdev)
>  	dev_dbg(&i2c_imx->adapter.dev, "device resources: %pR\n", res);
>  	dev_dbg(&i2c_imx->adapter.dev, "adapter name: \"%s\"\n",
>  		i2c_imx->adapter.name);
> -	dev_info(&i2c_imx->adapter.dev, "IMX I2C adapter registered\n");
>  
>  	/* Init DMA config if supported */
> -	i2c_imx_dma_request(i2c_imx, phy_addr);
> +	ret = i2c_imx_dma_request(i2c_imx, phy_addr);
> +	if (ret < 0)
> +		goto clk_notifier_unregister;

Just found another problem while going over my mails:

goto clk_notifier_unregister is wrong here as you have to undo
i2c_add_numbered_adapter(). Probably it also makes sense to move
initialisation of DMA before i2c_add_numbered_adapter() because
otherwise the first transfer request might come in while
i2c_imx_dma_request is in the middle of initializing the dma channels.

Best regards
Uwe
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index c406700789e1..3c12b174bbb1 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -273,7 +273,7 @@  static inline unsigned char imx_i2c_read_reg(struct imx_i2c_struct *i2c_imx,
 }
 
 /* Functions for DMA support */
-static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
+static int i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
 						dma_addr_t phy_addr)
 {
 	struct imx_i2c_dma *dma;
@@ -283,11 +283,13 @@  static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
 
 	dma = devm_kzalloc(dev, sizeof(*dma), GFP_KERNEL);
 	if (!dma)
-		return;
+		return -ENOMEM;
 
-	dma->chan_tx = dma_request_slave_channel(dev, "tx");
-	if (!dma->chan_tx) {
-		dev_dbg(dev, "can't request DMA tx channel\n");
+	dma->chan_tx = dma_request_chan(dev, "tx");
+	if (IS_ERR(dma->chan_tx)) {
+		ret = PTR_ERR(dma->chan_rx);
+		if (ret != -ENODEV && ret != -EPROBE_DEFER)
+			dev_err(dev, "can't request DMA tx channel (%d)\n", ret);
 		goto fail_al;
 	}
 
@@ -298,13 +300,15 @@  static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
 	dma_sconfig.direction = DMA_MEM_TO_DEV;
 	ret = dmaengine_slave_config(dma->chan_tx, &dma_sconfig);
 	if (ret < 0) {
-		dev_dbg(dev, "can't configure tx channel\n");
+		dev_err(dev, "can't configure tx channel (%d)\n", ret);
 		goto fail_tx;
 	}
 
-	dma->chan_rx = dma_request_slave_channel(dev, "rx");
-	if (!dma->chan_rx) {
-		dev_dbg(dev, "can't request DMA rx channel\n");
+	dma->chan_rx = dma_request_chan(dev, "rx");
+	if (IS_ERR(dma->chan_rx)) {
+		ret = PTR_ERR(dma->chan_rx);
+		if (ret != -ENODEV && ret != -EPROBE_DEFER)
+			dev_err(dev, "can't request DMA rx channel (%d)\n", ret);
 		goto fail_tx;
 	}
 
@@ -315,7 +319,7 @@  static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
 	dma_sconfig.direction = DMA_DEV_TO_MEM;
 	ret = dmaengine_slave_config(dma->chan_rx, &dma_sconfig);
 	if (ret < 0) {
-		dev_dbg(dev, "can't configure rx channel\n");
+		dev_err(dev, "can't configure rx channel (%d)\n", ret);
 		goto fail_rx;
 	}
 
@@ -324,15 +328,15 @@  static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
 	dev_info(dev, "using %s (tx) and %s (rx) for DMA transfers\n",
 		dma_chan_name(dma->chan_tx), dma_chan_name(dma->chan_rx));
 
-	return;
+	return 0;
 
 fail_rx:
 	dma_release_channel(dma->chan_rx);
 fail_tx:
 	dma_release_channel(dma->chan_tx);
 fail_al:
-	devm_kfree(dev, dma);
-	dev_info(dev, "can't use DMA, using PIO instead.\n");
+	/* return successfully if there is no dma support */
+	return ret == -ENODEV ? 0 : ret;
 }
 
 static void i2c_imx_dma_callback(void *arg)
@@ -1159,11 +1163,13 @@  static int i2c_imx_probe(struct platform_device *pdev)
 	dev_dbg(&i2c_imx->adapter.dev, "device resources: %pR\n", res);
 	dev_dbg(&i2c_imx->adapter.dev, "adapter name: \"%s\"\n",
 		i2c_imx->adapter.name);
-	dev_info(&i2c_imx->adapter.dev, "IMX I2C adapter registered\n");
 
 	/* Init DMA config if supported */
-	i2c_imx_dma_request(i2c_imx, phy_addr);
+	ret = i2c_imx_dma_request(i2c_imx, phy_addr);
+	if (ret < 0)
+		goto clk_notifier_unregister;
 
+	dev_info(&i2c_imx->adapter.dev, "IMX I2C adapter registered\n");
 	return 0;   /* Return OK */
 
 clk_notifier_unregister: