[PATCH v4 0/2] i2c: imx: rework imx error handling.

[PATCH v4 0/2] i2c: imx: rework imx error handling.

[Oleksij Rempel]

changes:
20190107 v4:
 - split one patch to separate patches
 - revert back devm_free(), we need it for -ENODEV case.
 - reword comments.

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

Oleksij Rempel (2):
  i2c: imx: notify about real errors on dma i2c_imx_dma_request
  i2c: imx: forward error values out of i2c_imx_dma_request()

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

-- 
2.19.1

[PATCH v4 1/2] i2c: imx: notify about real errors on dma i2c_imx_dma_request

[Oleksij Rempel]

Current implementation will print "can't use DMA, using PIO instead",
which is confusing and mostly interpreted as error. At least on i.MX5x,
DMA event for I2C is muxed with SDHC. So it is project specific
configuration. On other hand, real error have debug level.

With this patch real error should be printed and configuration specific
state will become silent.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/i2c/busses/i2c-imx.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index fa9ad53845d9..e28ef494dac8 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -285,9 +285,11 @@ static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
 	if (!dma)
 		return;
 
-	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;
 	}
 
@@ -332,7 +336,6 @@ static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
 	dma_release_channel(dma->chan_tx);
 fail_al:
 	devm_kfree(dev, dma);
-	dev_info(dev, "can't use DMA, using PIO instead.\n");
 }
 
 static void i2c_imx_dma_callback(void *arg)
-- 
2.19.1

Re: [PATCH v4 1/2] i2c: imx: notify about real errors on dma i2c_imx_dma_request

[Uwe Kleine-König]

Hello,

On Mon, Jan 07, 2019 at 09:47:37AM +0100, Oleksij Rempel wrote:
> Current implementation will print "can't use DMA, using PIO instead",
> which is confusing and mostly interpreted as error. At least on i.MX5x,
> DMA event for I2C is muxed with SDHC. So it is project specific
> configuration. On other hand, real error have debug level.
> 
> With this patch real error should be printed and configuration specific
> state will become silent.

I'd write:

At least on i.MX5x, the DMA events for I2C and SDHC use the same channel
and there can only be a single user. So in this case there should be no
message emitted that looks like an error if the I2C device doesn't have
an assigned DMA channel. In contrast real problems that were only
emitted at debug level before should be described at a higher level
to be better visible and so understandable.

> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

The patch is fine and can have my 

Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH v4 2/2] i2c: imx: forward error values out of i2c_imx_dma_request()

[Oleksij Rempel]

Make sure, i2c_imx_probe() well get and forward same error values as
i2c_imx_dma_request().

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/i2c/busses/i2c-imx.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index e28ef494dac8..487ccc2af999 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,7 +283,7 @@ 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_chan(dev, "tx");
 	if (IS_ERR(dma->chan_tx)) {
@@ -328,7 +328,7 @@ 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);
@@ -336,6 +336,8 @@ static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
 	dma_release_channel(dma->chan_tx);
 fail_al:
 	devm_kfree(dev, dma);
+	/* return successfully if there is no dma support */
+	return ret == -ENODEV ? 0 : ret;
 }
 
 static void i2c_imx_dma_callback(void *arg)
@@ -1163,11 +1165,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:
-- 
2.19.1

Re: [PATCH v4 2/2] i2c: imx: forward error values out of i2c_imx_dma_request()

[Uwe Kleine-König]

On Mon, Jan 07, 2019 at 09:47:38AM +0100, Oleksij Rempel wrote:
> Make sure, i2c_imx_probe() well get and forward same error values as
> i2c_imx_dma_request().

Maybe better: Improve the error handling in i2c_imx_dma_request() and
let it return an error indication that the caller then can handle
accordingly.

> -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)

This is a good opportunity to realign the continuation line to the
opening parenthesis.

Otherwise the patch looks good.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |