Hi Steve, Thanks for the review! Few comments inline. On 25.03.2019 19:12, Steven Price wrote: > On 25/03/2019 15:30, laurentiu.tudor@xxxxxxx wrote: >> From: Laurentiu Tudor <laurentiu.tudor@xxxxxxx> >> >> If the dma controller is not yet probed, defer i2c probe. >> The error path in probe was slightly modified (no functional change) > > There is a functional change for cases like: > >> ret = pm_runtime_get_sync(&pdev->dev); >> if (ret < 0) >> goto rpm_disable; > > ...as rpm_disable will no longer fall through to the call of > clk_disable_unprepare(). Good point, I missed that. >> to avoid triggering this WARN_ON(): >> "cg-pll0-div1 already disabled >> WARNING: CPU: 1 PID: 1 at drivers/clk/clk.c:828 clk_core_disable+0xa8/0xb0" > > I'm also not sure how this WARN_ON was hit. i2c_imx_probe() calls > clk_prepare_enable() which should increment the reference count. So it > should always be possible to decrememt the enable_count. What am I missing? I am no longer able to replicate this. Perhaps in the mean time changes in the clk core fixed this corner case. >> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@xxxxxxx> >> --- >> drivers/i2c/busses/i2c-imx.c | 21 +++++++++++++-------- >> 1 file changed, 13 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c >> index 42fed40198a0..4e34b1572756 100644 >> --- a/drivers/i2c/busses/i2c-imx.c >> +++ b/drivers/i2c/busses/i2c-imx.c >> @@ -1111,7 +1111,8 @@ static int i2c_imx_probe(struct platform_device *pdev) >> pdev->name, i2c_imx); >> if (ret) { >> dev_err(&pdev->dev, "can't claim irq %d\n", irq); >> - goto clk_disable; >> + clk_disable_unprepare(i2c_imx->clk); >> + return ret; >> } >> >> /* Init queue */ >> @@ -1161,19 +1162,25 @@ static int i2c_imx_probe(struct platform_device *pdev) >> pm_runtime_mark_last_busy(&pdev->dev); >> pm_runtime_put_autosuspend(&pdev->dev); >> >> + /* Init DMA config if supported */ >> + ret = i2c_imx_dma_request(i2c_imx, phy_addr); >> + if (ret) { >> + if (ret != -EPROBE_DEFER) >> + dev_info(&pdev->dev, "can't use DMA, using PIO instead.\n"); >> + else >> + goto del_adapter; >> + } >> + > > This can be simplified by reversing the condition: Well, in the mean time I just found out that this commit [1] fixes the issue I was seeing so I think the patch is no longer needed. [1] https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git/commit/?h=i2c/for-next&id=e1ab9a468e3b1 --- Best Regards, Laurentiu