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