Hi Russell,

I am sorry to reply late, thanks for your patient reminding,
Please see my comments inline.

Best Regards,
Peng

-----Original Message-----
From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
Sent: 2019年11月28日 18:06
To: Peng Ma <redacted>
Cc: linux@rempel-privat.de; kernel@pengutronix.de; shawnguo@kernel.org;
s.hauer@pengutronix.de; linux-kernel@vger.kernel.org; dl-linux-imx
[off-list ref]; festevam@gmail.com;
linux-arm-kernel@lists.infradead.org; linux-i2c@vger.kernel.org
Subject: [EXT] Re: [PATCH] i2c: imx: Defer probing if EDMA not available

Caution: EXT Email

On Wed, Nov 27, 2019 at 07:12:09AM +0000, Peng Ma wrote:

quoted

EDMA may be not available or defered due to dependencies on other
modules, If these scenarios is encountered, we should defer probing.

This has been tried before in this form, and it causes regressions.

quoted

Signed-off-by: Peng Ma <redacted>
---
 drivers/i2c/busses/i2c-imx.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx.c

b/drivers/i2c/busses/i2c-imx.c index 40111a3..c2b0693 100644

--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c

@@ -369,8 +369,8 @@ static void i2c_imx_reset_regs(struct

imx_i2c_struct *i2c_imx)  }

 /* Functions for DMA support */
-static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
-                                             dma_addr_t

phy_addr)

quoted

+static int i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
+                            dma_addr_t phy_addr)
 {
      struct imx_i2c_dma *dma;
      struct dma_slave_config dma_sconfig; @@ -379,7 +379,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)) {

@@ -424,7 +424,7 @@ static void i2c_imx_dma_request(struct

imx_i2c_struct *i2c_imx,

quoted

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

@@ -432,6 +432,8 @@ static void i2c_imx_dma_request(struct

imx_i2c_struct *i2c_imx,

quoted

      dma_release_channel(dma->chan_tx);
 fail_al:
      devm_kfree(dev, dma);
+
+     return ret;

Some platforms don't have EDMA.  Doesn't this force everyone who wants
I2C to have DMA?  The last attempt at this had:

       /* return successfully if there is no dma support */
       return ret == -ENODEV ? 0 : ret;

here because of exactly this.

quoted

 }

 static void i2c_imx_dma_callback(void *arg) @@ -1605,10 +1607,14 @@
static int i2c_imx_probe(struct platform_device *pdev)
      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 == -EPROBE_DEFER)
+             goto i2c_adapter_remove;

This happens _after_ the adapter has been published to the rest of the kernel.
Claiming resources after publication is racy - the adapter may be in use by a
request at this point.  Secondly, there's been problems with this causing
regressions when EDMA is built as a module and i2c-imx is built-in.

See e8c220fac415 ("Revert "i2c: imx: improve the error handling in
i2c_imx_dma_request()"") when exactly what you're proposing was tried and
ended up having to be reverted.

AFAIK nothing has changed since, so merely reinstating the known to be broken
code, thereby reintroducing the same (and more) problems, isn't going to be
acceptable.

Sorry, but this gets a big NAK from me.

[Peng Ma] I saw the revert commit e8c220fac415 and understand your concerns.
I scan the i2c-imx.c driver, All platforms that use i2c driver and support dma use an eDMA engine,
So I change the code(compare with last patch) as follows, please review and give me your precious comments.
Thanks very much.

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 12f7934fddb4..6cafee52dd67 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c

@@ -1605,8 +1605,11 @@ static int i2c_imx_probe(struct platform_device *pdev)
 
        /* Init DMA config if supported */
        ret = i2c_imx_dma_request(i2c_imx, phy_addr);
-       if (ret == -EPROBE_DEFER)
+       if (ret == -EPROBE_DEFER) {
+#if    IS_BUILTIN(CONFIG_FSL_EDMA)
                goto i2c_adapter_remove;
+#endif
+       }

quoted

      return 0;   /* Return OK */

+i2c_adapter_remove:
+     i2c_del_adapter(&i2c_imx->adapter);
 clk_notifier_unregister:
      clk_notifier_unregister(i2c_imx->clk, &i2c_imx->clk_change_nb);
 rpm_disable:
--
2.9.5


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists
.infradead.org%2Fmailman%2Flistinfo%2Flinux-arm-kernel&data=02%7

C0

quoted

1%7Cpeng.ma%40nxp.com%7C6dbcf73ceb10495457fa08d773ea9ee1%7C686
ea1d3bc2

quoted

b4c6fa92cd99c5c301635%7C0%7C1%7C637105323843174631&sdata=d
v1UINRME

quoted

Cx6w2xG%2FyliNWNvIbTbacHpqAt8%2B6W5qFk%3D&reserved=0

--
RMK's Patch system:
https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.ar
mlinux.org.uk%2Fdeveloper%2Fpatches%2F&data=02%7C01%7Cpeng.ma
%40nxp.com%7C6dbcf73ceb10495457fa08d773ea9ee1%7C686ea1d3bc2b4c6
fa92cd99c5c301635%7C0%7C1%7C637105323843184629&sdata=9FZCA
JJxE99wP5ZMoG6ib%2FeYoXdksgq2uSzBrBtNUnU%3D&reserved=0
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps
up According to speedtest.net: 11.9Mbps down 500kbps up

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel