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

[PATCH v4 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, and pm_runtime_put_sync() may
   trigger the runtime suspend callback before resources are fully
   set up.

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.

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 | 52 +++++++++++++++++++++---------
 1 file changed, 36 insertions(+), 16 deletions(-)

-- 
2.43.0

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

[carlos.song]

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

When probe fails after clk_bulk_prepare_enable() succeeds but before
runtime PM is initialized, the enabled clocks are never disabled.
Additionally, calling pm_runtime_put_sync() in the error path can
trigger the runtime suspend callback, which may attempt to disable
clocks that have not been fully set up, leading to potential issues
during error unwinding.

Introduce two new error labels: clk_disable to explicitly invoke
clk_bulk_disable_unprepare(), and free_irq to release the IRQ via
devm_free_irq(). 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. Update all goto targets so that each failure site
releases only the resources acquired up to that point.

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

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
index cd4da50c4dd9..fbb9c0b0a99c 100644
--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
@@ -1520,21 +1520,25 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
 
 	ret = clk_bulk_prepare_enable(lpi2c_imx->num_clks, lpi2c_imx->clks);
 	if (ret)
-		return ret;
+		goto free_irq;
 
 	/*
 	 * Lock the parent clock rate to avoid getting parent clock upon
 	 * 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 = dev_err_probe(&pdev->dev, -EINVAL,
+				    "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 +1580,13 @@ 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);
+free_irq:
+	devm_free_irq(&pdev->dev, lpi2c_imx->irq, lpi2c_imx);
 
 	return ret;
 }
-- 
2.43.0

[PATCH v4 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 | 33 ++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
index fbb9c0b0a99c..93f263bf27b2 100644
--- a/drivers/i2c/busses/i2c-imx-lpi2c.c
+++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
@@ -1510,17 +1510,12 @@ 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);
 
 	ret = clk_bulk_prepare_enable(lpi2c_imx->num_clks, lpi2c_imx->clks);
 	if (ret)
-		goto free_irq;
+		return dev_err_probe(&pdev->dev, ret, "can't get LPI2C clock\n");
 
 	/*
 	 * Lock the parent clock rate to avoid getting parent clock upon
@@ -1550,6 +1545,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);
@@ -1558,19 +1569,19 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
 	ret = lpi2c_imx_init_recovery_info(lpi2c_imx, pdev);
 	/* Give it another chance if pinctrl used is not ready yet */
 	if (ret == -EPROBE_DEFER)
-		goto rpm_disable;
+		goto free_irq;
 
 	/* Init DMA */
 	ret = lpi2c_dma_init(&pdev->dev, phy_addr);
 	if (ret) {
 		if (ret == -EPROBE_DEFER)
-			goto rpm_disable;
+			goto free_irq;
 		dev_info(&pdev->dev, "use pio mode\n");
 	}
 
 	ret = i2c_add_adapter(&lpi2c_imx->adapter);
 	if (ret)
-		goto rpm_disable;
+		goto free_irq;
 
 	pm_runtime_put_autosuspend(&pdev->dev);
 
@@ -1578,6 +1589,8 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
 
 	return 0;
 
+free_irq:
+	devm_free_irq(&pdev->dev, lpi2c_imx->irq, lpi2c_imx);
 rpm_disable:
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
@@ -1585,8 +1598,6 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
 	pm_runtime_put_noidle(&pdev->dev);
 clk_disable:
 	clk_bulk_disable_unprepare(lpi2c_imx->num_clks, lpi2c_imx->clks);
-free_irq:
-	devm_free_irq(&pdev->dev, lpi2c_imx->irq, lpi2c_imx);
 
 	return ret;
 }
-- 
2.43.0

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

[Frank Li]

On Tue, Jun 09, 2026 at 05:51:18PM +0800, carlos.song@oss.nxp.com wrote:
> [You don't often get email from carlos.song@oss.nxp.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> From: Carlos Song <carlos.song@nxp.com>
>
> When probe fails after clk_bulk_prepare_enable() succeeds but before
> runtime PM is initialized, the enabled clocks are never disabled.
> Additionally, calling pm_runtime_put_sync() in the error path can
> trigger the runtime suspend callback, which may attempt to disable
> clocks that have not been fully set up, leading to potential issues
> during error unwinding.
>
> Introduce two new error labels: clk_disable to explicitly invoke
> clk_bulk_disable_unprepare(), and free_irq to release the IRQ via
> devm_free_irq(). 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. Update all goto targets so that each failure site
> releases only the resources acquired up to that point.
>
> Signed-off-by: Carlos Song <carlos.song@nxp.com>
> ---
>  drivers/i2c/busses/i2c-imx-lpi2c.c | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> index cd4da50c4dd9..fbb9c0b0a99c 100644
> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> @@ -1520,21 +1520,25 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
>
>         ret = clk_bulk_prepare_enable(lpi2c_imx->num_clks, lpi2c_imx->clks);
>         if (ret)
> -               return ret;
> +               goto free_irq;

If you use runtime pm, you should not manually manange clock again.

generally method is

	devm_clk_get()
	devm_runtime_pm_enable()

	call runtime_pm_get_sync(), \\there are PM AQUIRE help macro to help
elimiate goto branch.
	... // if need clock enable to do some works.
	call runtime_pm_put()

	...

and needn't call devm_free_irq().

Frank


>
>         /*
>          * Lock the parent clock rate to avoid getting parent clock upon
>          * 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 = dev_err_probe(&pdev->dev, -EINVAL,
> +                                   "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 +1580,13 @@ 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);
> +free_irq:
> +       devm_free_irq(&pdev->dev, lpi2c_imx->irq, lpi2c_imx);
>
>         return ret;
>  }
> --
> 2.43.0
>
>