[PATCH v5 0/2] i2c: imx-lpi2c: fix probe error handling and reset controller

[PATCH v5 0/2] i2c: imx-lpi2c: fix probe error handling and reset controller

[carlos.song]

From: Carlos Song <carlos.song@nxp.com>

During probe, two issues exist in the LPI2C driver:

1. The error paths do not properly unwind all acquired resources on
   failure. Clocks enabled before runtime PM initialization are never
   disabled on certain error paths, when probe fails after runtime PM
   is initialized, the previous error path called pm_runtime_put_sync(),
   however, due to different clock management strategies on different
   SoCs[1] (to avoid deadlocks between the global prepare_lock and runtime
   PM), the callback may only disable clocks without unpreparing them,
   causing an incomplete unwind. 

2. The LPI2C controller may retain unexpected state from previous
   boot stages, causing the system to hang during probe.

This series addresses both issues. Patch 1 restructures the probe
error paths to ensure correct resource cleanup on failure. Patch 2
resets the Master and Target controller logic before IRQ registration
to clear any leftover state.

[1] https://lore.kernel.org/all/20251125084718.2156168-1-carlos.song@nxp.com/
 
Changes for v5:
  - Remove devm_free_irq() and free_irq err label.
  - Change commit log to explain why not using runtime PM to manage clocks
    during the probe phase.

Changes for v4:
  - Split v3 into two patches per reviewer feedback:
    * Patch 1 contains only the error path restructuring.
    * Patch 2 contains only the controller reset and the IRQ
      relocation that is required by the reset ordering.

Changes for v3:
  - Reset the Target logic via LPI2C_SCR in addition to MCR.
  - Replace pm_runtime_put_sync() with pm_runtime_disable() +
    pm_runtime_set_suspended() + pm_runtime_put_noidle() to avoid
    triggering the suspend callback during error recovery.
  - Add clk_disable and free_irq labels for complete error unwinding.
  - Update commit log to cover the SCR reset rationale.

Changes for v2:
  - Jump to rpm_disable instead of returning directly if the IRQ
request fails.

Carlos Song (2):
  i2c: imx-lpi2c: properly unwind resources on probe failure
  i2c: imx-lpi2c: reset controller in probe stage

 drivers/i2c/busses/i2c-imx-lpi2c.c | 43 +++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 12 deletions(-)

-- 
2.43.0

[PATCH v5 1/2] i2c: imx-lpi2c: properly unwind resources on probe failure

[carlos.song]

From: Carlos Song <carlos.song@nxp.com>

When probe fails at devm_clk_rate_exclusive_get() or clk_get_rate(),
which occur before runtime PM is initialized, the clocks enabled by
clk_bulk_prepare_enable() are never disabled.

When probe fails after runtime PM is initialized, the previous error
path called pm_runtime_put_sync(), which triggers the runtime suspend
callback. However, due to different clock management strategies on
different SoCs[1] (to avoid deadlocks between the global prepare_lock
and runtime PM), the callback may only disable clocks without
unpreparing them, causing an incomplete unwind.

Introduce a new error label 'clk_disable' to explicitly invoke
clk_bulk_disable_unprepare(). Replace pm_runtime_put_sync() with the
sequence of pm_runtime_disable(), pm_runtime_set_suspended() and
pm_runtime_put_noidle() to bypass the runtime suspend callback during
error recovery. During the LPI2C driver probe phase, clock APIs are
used exclusively to manage clocks. Once probing succeeds, clock
management is handed over to the runtime PM core.

[1] https://lore.kernel.org/all/20251125084718.2156168-1-carlos.song@nxp.com/

Signed-off-by: Carlos Song <carlos.song@nxp.com>
---
 drivers/i2c/busses/i2c-imx-lpi2c.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
index e6c24a9d934d..2dc4331ed796 100644
--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
@@ -1527,14 +1527,19 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
 	 * each transfer
 	 */
 	ret = devm_clk_rate_exclusive_get(&pdev->dev, lpi2c_imx->clks[0].clk);
-	if (ret)
-		return dev_err_probe(&pdev->dev, ret,
-				     "can't lock I2C peripheral clock rate\n");
+	if (ret) {
+		dev_err_probe(&pdev->dev, ret,
+			      "can't lock I2C peripheral clock rate\n");
+		goto clk_disable;
+	}
 
 	lpi2c_imx->rate_per = clk_get_rate(lpi2c_imx->clks[0].clk);
-	if (!lpi2c_imx->rate_per)
-		return dev_err_probe(&pdev->dev, -EINVAL,
-				     "can't get I2C peripheral clock rate\n");
+	if (!lpi2c_imx->rate_per) {
+		ret = -EINVAL;
+		dev_err_probe(&pdev->dev, ret,
+			      "can't get I2C peripheral clock rate\n");
+		goto clk_disable;
+	}
 
 	if (lpi2c_imx->hwdata->need_prepare_unprepare_clk)
 		pm_runtime_set_autosuspend_delay(&pdev->dev, I2C_PM_LONG_TIMEOUT_MS);
@@ -1576,8 +1581,11 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
 
 rpm_disable:
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
-	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
+	pm_runtime_set_suspended(&pdev->dev);
+	pm_runtime_put_noidle(&pdev->dev);
+clk_disable:
+	clk_bulk_disable_unprepare(lpi2c_imx->num_clks, lpi2c_imx->clks);
 
 	return ret;
 }
-- 
2.43.0

Re: [PATCH v5 1/2] i2c: imx-lpi2c: properly unwind resources on probe failure

[Frank Li]

On Tue, Jun 30, 2026 at 06:52:22PM +0800, carlos.song@oss.nxp.com wrote:
> From: Carlos Song <carlos.song@nxp.com>
>
> When probe fails at devm_clk_rate_exclusive_get() or clk_get_rate(),
> which occur before runtime PM is initialized, the clocks enabled by
> clk_bulk_prepare_enable() are never disabled.
>
> When probe fails after runtime PM is initialized, the previous error
> path called pm_runtime_put_sync(), which triggers the runtime suspend
> callback. However, due to different clock management strategies on
> different SoCs[1] (to avoid deadlocks between the global prepare_lock
> and runtime PM), the callback may only disable clocks without
> unpreparing them, causing an incomplete unwind.
>
> Introduce a new error label 'clk_disable' to explicitly invoke
> clk_bulk_disable_unprepare(). Replace pm_runtime_put_sync() with the
> sequence of pm_runtime_disable(), pm_runtime_set_suspended() and
> pm_runtime_put_noidle() to bypass the runtime suspend callback during
> error recovery. During the LPI2C driver probe phase, clock APIs are
> used exclusively to manage clocks. Once probing succeeds, clock
> management is handed over to the runtime PM core.
>
> [1] https://lore.kernel.org/all/20251125084718.2156168-1-carlos.song@nxp.com/
>
> Signed-off-by: Carlos Song <carlos.song@nxp.com>
> ---

In future, we need work with community to fix clock framework global lock
problem. Any this one is good for now.

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/i2c/busses/i2c-imx-lpi2c.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> index e6c24a9d934d..2dc4331ed796 100644
> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> @@ -1527,14 +1527,19 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
>  	 * each transfer
>  	 */
>  	ret = devm_clk_rate_exclusive_get(&pdev->dev, lpi2c_imx->clks[0].clk);
> -	if (ret)
> -		return dev_err_probe(&pdev->dev, ret,
> -				     "can't lock I2C peripheral clock rate\n");
> +	if (ret) {
> +		dev_err_probe(&pdev->dev, ret,
> +			      "can't lock I2C peripheral clock rate\n");
> +		goto clk_disable;
> +	}
>
>  	lpi2c_imx->rate_per = clk_get_rate(lpi2c_imx->clks[0].clk);
> -	if (!lpi2c_imx->rate_per)
> -		return dev_err_probe(&pdev->dev, -EINVAL,
> -				     "can't get I2C peripheral clock rate\n");
> +	if (!lpi2c_imx->rate_per) {
> +		ret = -EINVAL;
> +		dev_err_probe(&pdev->dev, ret,
> +			      "can't get I2C peripheral clock rate\n");
> +		goto clk_disable;
> +	}
>
>  	if (lpi2c_imx->hwdata->need_prepare_unprepare_clk)
>  		pm_runtime_set_autosuspend_delay(&pdev->dev, I2C_PM_LONG_TIMEOUT_MS);
> @@ -1576,8 +1581,11 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
>
>  rpm_disable:
>  	pm_runtime_dont_use_autosuspend(&pdev->dev);
> -	pm_runtime_put_sync(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
> +	pm_runtime_set_suspended(&pdev->dev);
> +	pm_runtime_put_noidle(&pdev->dev);
> +clk_disable:
> +	clk_bulk_disable_unprepare(lpi2c_imx->num_clks, lpi2c_imx->clks);
>
>  	return ret;
>  }
> --
> 2.43.0
>

Re: [PATCH v5 1/2] i2c: imx-lpi2c: properly unwind resources on probe failure

[Markus Elfring]

…
> Introduce a new error label 'clk_disable' …

Can another bit of duplicate source code be avoided in any if branches?

Regards,
Markus

Re: [PATCH v5 1/2] i2c: imx-lpi2c: properly unwind resources on probe failure

[Carlos Song]

On Tue, Jul 14, 2026 at 04:10:20PM +0200, Markus Elfring wrote:
> …
> > Introduce a new error label 'clk_disable' …
> 
> Can another bit of duplicate source code be avoided in any if branches?
>
> Regards,
> Markus
>

Thank you! I plan to change the code like this:
'''
        ret = devm_clk_rate_exclusive_get(&pdev->dev, lpi2c_imx->clks[0].clk);
        if (ret) {
                ret = dev_err_probe(&pdev->dev, ret,
                                    "can't lock I2C peripheral clock rate\n");
                goto clk_disable;
        }

        lpi2c_imx->rate_per = clk_get_rate(lpi2c_imx->clks[0].clk);
        if (!lpi2c_imx->rate_per) {
                ret = dev_err_probe(&pdev->dev, -EINVAL,
                                    "can't get I2C peripheral clock rate\n");
                goto clk_disable;
        }
'''
Does it look better?

Carlos Song

Re: [v5 1/2] i2c: imx-lpi2c: properly unwind resources on probe failure

[Markus Elfring]

> Does it look better?

I suggest to reconsider missing tab characters in the indentation.


https://elixir.bootlin.com/linux/v7.2-rc3/source/drivers/i2c/busses/i2c-imx-lpi2c.c#L1471-L1583

Regards,
Markus

[PATCH v5 2/2] i2c: imx-lpi2c: reset controller in probe stage

[carlos.song]

From: Carlos Song <carlos.song@nxp.com>

Reset I2C controller in probe stage to avoid unexpected LPI2C controller
state left from previous stages and hang system boot.

Per the LPI2C reference manual, section 7.1.4 "Controller Control (MCR)"
and 7.1.20 Target Control (SCR), the RST bit (bit 1) description states:

  "The reset takes effect immediately and remains asserted until negated
  by software. There is no minimum delay required before clearing the
  software reset."

Therefore, it is safe to write 0 to MCR and SCR immediately after
asserting the RST bit without any additional delay.

Signed-off-by: Carlos Song <carlos.song@nxp.com>
---
 drivers/i2c/busses/i2c-imx-lpi2c.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
index 2dc4331ed796..9fbdce85d7ca 100644
--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
@@ -1510,11 +1510,6 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
 	if (ret)
 		lpi2c_imx->bitrate = I2C_MAX_STANDARD_MODE_FREQ;
 
-	ret = devm_request_irq(&pdev->dev, lpi2c_imx->irq, lpi2c_imx_isr, IRQF_NO_SUSPEND,
-			       pdev->name, lpi2c_imx);
-	if (ret)
-		return dev_err_probe(&pdev->dev, ret, "can't claim irq %d\n", lpi2c_imx->irq);
-
 	i2c_set_adapdata(&lpi2c_imx->adapter, lpi2c_imx);
 	platform_set_drvdata(pdev, lpi2c_imx);
 
@@ -1551,6 +1546,22 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
 	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
 
+	/*
+	 * Reset all internal controller registers of both Master and Target
+	 * to avoid effects of previous status.
+	 */
+	writel(MCR_RST, lpi2c_imx->base + LPI2C_MCR);
+	writel(SCR_RST, lpi2c_imx->base + LPI2C_SCR);
+	writel(0, lpi2c_imx->base + LPI2C_MCR);
+	writel(0, lpi2c_imx->base + LPI2C_SCR);
+
+	ret = devm_request_irq(&pdev->dev, lpi2c_imx->irq, lpi2c_imx_isr, IRQF_NO_SUSPEND,
+			       pdev->name, lpi2c_imx);
+	if (ret) {
+		dev_err_probe(&pdev->dev, ret, "can't claim irq %d\n", lpi2c_imx->irq);
+		goto rpm_disable;
+	}
+
 	temp = readl(lpi2c_imx->base + LPI2C_PARAM);
 	lpi2c_imx->txfifosize = 1 << (temp & 0x0f);
 	lpi2c_imx->rxfifosize = 1 << ((temp >> 8) & 0x0f);
-- 
2.43.0

Re: [PATCH v5 2/2] i2c: imx-lpi2c: reset controller in probe stage

[Frank Li]

On Tue, Jun 30, 2026 at 06:52:23PM +0800, carlos.song@oss.nxp.com wrote:
> From: Carlos Song <carlos.song@nxp.com>
>
> Reset I2C controller in probe stage to avoid unexpected LPI2C controller
> state left from previous stages and hang system boot.
>
> Per the LPI2C reference manual, section 7.1.4 "Controller Control (MCR)"
> and 7.1.20 Target Control (SCR), the RST bit (bit 1) description states:
>
>   "The reset takes effect immediately and remains asserted until negated
>   by software. There is no minimum delay required before clearing the
>   software reset."
>
> Therefore, it is safe to write 0 to MCR and SCR immediately after
> asserting the RST bit without any additional delay.
>
> Signed-off-by: Carlos Song <carlos.song@nxp.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/i2c/busses/i2c-imx-lpi2c.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> index 2dc4331ed796..9fbdce85d7ca 100644
> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> @@ -1510,11 +1510,6 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
>  	if (ret)
>  		lpi2c_imx->bitrate = I2C_MAX_STANDARD_MODE_FREQ;
>
> -	ret = devm_request_irq(&pdev->dev, lpi2c_imx->irq, lpi2c_imx_isr, IRQF_NO_SUSPEND,
> -			       pdev->name, lpi2c_imx);
> -	if (ret)
> -		return dev_err_probe(&pdev->dev, ret, "can't claim irq %d\n", lpi2c_imx->irq);
> -
>  	i2c_set_adapdata(&lpi2c_imx->adapter, lpi2c_imx);
>  	platform_set_drvdata(pdev, lpi2c_imx);
>
> @@ -1551,6 +1546,22 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
>  	pm_runtime_set_active(&pdev->dev);
>  	pm_runtime_enable(&pdev->dev);
>
> +	/*
> +	 * Reset all internal controller registers of both Master and Target
> +	 * to avoid effects of previous status.
> +	 */
> +	writel(MCR_RST, lpi2c_imx->base + LPI2C_MCR);
> +	writel(SCR_RST, lpi2c_imx->base + LPI2C_SCR);
> +	writel(0, lpi2c_imx->base + LPI2C_MCR);
> +	writel(0, lpi2c_imx->base + LPI2C_SCR);
> +
> +	ret = devm_request_irq(&pdev->dev, lpi2c_imx->irq, lpi2c_imx_isr, IRQF_NO_SUSPEND,
> +			       pdev->name, lpi2c_imx);
> +	if (ret) {
> +		dev_err_probe(&pdev->dev, ret, "can't claim irq %d\n", lpi2c_imx->irq);
> +		goto rpm_disable;
> +	}
> +
>  	temp = readl(lpi2c_imx->base + LPI2C_PARAM);
>  	lpi2c_imx->txfifosize = 1 << (temp & 0x0f);
>  	lpi2c_imx->rxfifosize = 1 << ((temp >> 8) & 0x0f);
> --
> 2.43.0
>