Re: [PATCH v3] i2c: imx-lpi2c: reset controller in probe stage

[Frank Li]

On Thu, May 21, 2026 at 06:35:30PM +0800, Carlos Song (OSS) wrote:
> From: Carlos Song <carlos.song@xxxxxxx>
>
> 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@xxxxxxx>
> ---
> Change for v3:
>   - Reset the Target logic via LPI2C_SCR.
>   - Replacing pm_runtime_put_sync() with pm_runtime_disable() +
>     pm_runtime_set_suspended() + pm_runtime_put_noidle() to avoid
>     triggering the suspend callback, and explicitly calling
>     clk_bulk_disable_unprepare() for clock cleanup.
>   - Add new error path free_irq and clk_disable to cover more
>     complete error recovery.
>   - Remake commit log adding SCR RST section.
> Change for v2:
>   - Jump to rpm_disable instread of returning directly if the IRQ request
>     fails
> ---
>  drivers/i2c/busses/i2c-imx-lpi2c.c | 52 +++++++++++++++++++++---------
>  1 file changed, 37 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> index e6c24a9d934d..4929f85ab485 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);
>
> @@ -1527,14 +1522,18 @@ 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 = 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);
> @@ -1546,27 +1545,43 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
>  	pm_runtime_set_active(&pdev->dev);
>  	pm_runtime_enable(&pdev->dev);
>
> +	/*
> +	 * Reset all internal controller logic and registers to avoid effects of previous status
> +	 * Reset both Master and Target logic to prevent interrupt storms
> +	 */
> +	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);
> +

This patch should only include this part. other adjust should be new patch

>  	temp = readl(lpi2c_imx->base + LPI2C_PARAM);
>  	lpi2c_imx->txfifosize = 1 << (temp & 0x0f);
>  	lpi2c_imx->rxfifosize = 1 << ((temp >> 8) & 0x0f);
>
> +	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;
> +	}
> +
>  	/* Init optional bus recovery function */
>  	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);
>
> @@ -1574,10 +1589,17 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
>
>  	return 0;
>
> +free_irq:
> +	devm_free_irq(&pdev->dev, lpi2c_imx->irq, lpi2c_imx);
> +

if use devm_(), need devm_free_irq() here.

Frank
>  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
>