RE: [EXT] Re: [PATCH] i2c: imx: Defer probing if EDMA not available
From: Peng Ma
Date: Wed Dec 11 2019 - 05:25:35 EST
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@xxxxxxxxxxxxxxx>
>Sent: 2019年11月28日 18:06
>To: Peng Ma <peng.ma@xxxxxxx>
>Cc: linux@xxxxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx; shawnguo@xxxxxxxxxx;
>s.hauer@xxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; dl-linux-imx
><linux-imx@xxxxxxx>; festevam@xxxxxxxxx;
>linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx
>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:
>> 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.
>
>> Signed-off-by: Peng Ma <peng.ma@xxxxxxx>
>> ---
>> 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)
>> +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,
>> 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,
>> 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.
>
>> }
>>
>> 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
+ }
>>
>> 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@xxxxxxxxxxxxxxxxxxx
>> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists
>> .infradead.org%2Fmailman%2Flistinfo%2Flinux-arm-kernel&data=02%7
>C0
>>
>1%7Cpeng.ma%40nxp.com%7C6dbcf73ceb10495457fa08d773ea9ee1%7C686
>ea1d3bc2
>>
>b4c6fa92cd99c5c301635%7C0%7C1%7C637105323843174631&sdata=d
>v1UINRME
>> 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