Re: [PATCH v6 6/9] irqchip/imx-irqsteer: Let devres own the clock

[Frank Li <Frank.li@oss.nxp.com>]

On Thu, Oct 08, 2026 at 06:02:28PM +0900, Zhipeng.wang_1@oss.nxp.com wrote:
> From: Zhipeng Wang <zhipeng.wang_1@nxp.com>
>
> Let the driver core own the clock lifetime so that the probe() error
> path and remove() do not have to hand-balance it:
>
>  - acquire the clock with devm_clk_get_enabled() instead of a bare
>    devm_clk_get() followed by a manual clk_prepare_enable(), so it is
>    prepared/enabled for the device lifetime and released on unbind;
>  - keep only clk_enable()/clk_disable() in the runtime PM callbacks,
>    since prepare/unprepare is now handled once by devres.
>
> Signed-off-by: Zhipeng Wang <zhipeng.wang_1@nxp.com>
> ---
>  drivers/irqchip/irq-imx-irqsteer.c | 37 +++++++++++++++---------------
>  1 file changed, 19 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
> index fa233de9bc2b..15ee50f65d54 100644
> --- a/drivers/irqchip/irq-imx-irqsteer.c
> +++ b/drivers/irqchip/irq-imx-irqsteer.c
> @@ -194,7 +194,7 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
>  		return PTR_ERR(data->regs);
>  	}
>
> -	data->ipg_clk = devm_clk_get(&pdev->dev, "ipg");
> +	data->ipg_clk = devm_clk_get_enabled(&pdev->dev, "ipg");
>  	if (IS_ERR(data->ipg_clk))
>  		return dev_err_probe(&pdev->dev, PTR_ERR(data->ipg_clk),
>  				     "failed to get ipg clk\n");
> @@ -229,12 +229,6 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
>  			return -ENOMEM;
>  	}
>
> -	ret = clk_prepare_enable(data->ipg_clk);
> -	if (ret) {
> -		dev_err(&pdev->dev, "failed to enable ipg clk: %d\n", ret);
> -		return ret;
> -	}
> -
>  	/* steer all IRQs into configured channel */
>  	if (irqsteer_has_chanctrl(data->devtype_data))
>  		writel_relaxed(BIT(data->channel), data->regs + CHANCTRL);
> @@ -249,14 +243,13 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
>
>  	ret = devm_pm_runtime_set_active_enabled(&pdev->dev);
>  	if (ret)
> -		goto out;
> +		return ret;
>
>  	data->domain = irq_domain_create_linear(dev_fwnode(&pdev->dev), data->reg_num * 32,
>  						&imx_irqsteer_domain_ops, data);
>  	if (!data->domain) {
>  		dev_err(&pdev->dev, "failed to create IRQ domain\n");
> -		ret = -ENOMEM;
> -		goto out;
> +		return -ENOMEM;
>  	}
>  	irq_domain_set_pm_device(data->domain, &pdev->dev);
>
> @@ -273,15 +266,23 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, data);
>
>  	return 0;
> -out:
> -	clk_disable_unprepare(data->ipg_clk);
> -	return ret;
>  }
>
>  static void imx_irqsteer_remove(struct platform_device *pdev)
>  {
>  	struct irqsteer_data *irqsteer_data = platform_get_drvdata(pdev);
> -	int i;
> +	int i, ret;
> +
> +	/*
> +	 * The device may be runtime-suspended here, in which case the runtime
> +	 * suspend callback has already dropped the clock enable count. Resume
> +	 * it so the devres clk_disable_unprepare(), which runs after remove(),
> +	 * finds the clock enabled and stays balanced. On success drop the
> +	 * usage count again with pm_runtime_put_noidle(): it must not trigger a
> +	 * suspend (which would re-disable the clock) and must not leak to the
> +	 * next probe of this persistent device.
> +	 */
> +	ret = pm_runtime_resume_and_get(&pdev->dev);

I think need new API/method to handle this case, which is quite common.
Anyways, this method should work. let as it now.

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

>
>  	for (i = 0; i < irqsteer_data->irq_count; i++) {
>  		if (!irqsteer_data->irq[i])
> @@ -294,8 +295,8 @@ static void imx_irqsteer_remove(struct platform_device *pdev)
>
>  	irq_domain_remove(irqsteer_data->domain);
>
> -	if (!pm_runtime_status_suspended(&pdev->dev))
> -		clk_disable_unprepare(irqsteer_data->ipg_clk);
> +	if (ret >= 0)
> +		pm_runtime_put_noidle(&pdev->dev);
>  }
>
>  #ifdef CONFIG_PM
> @@ -325,7 +326,7 @@ static int imx_irqsteer_suspend(struct device *dev)
>  	struct irqsteer_data *irqsteer_data = dev_get_drvdata(dev);
>
>  	imx_irqsteer_save_regs(irqsteer_data);
> -	clk_disable_unprepare(irqsteer_data->ipg_clk);
> +	clk_disable(irqsteer_data->ipg_clk);
>
>  	return 0;
>  }
> @@ -335,7 +336,7 @@ static int imx_irqsteer_resume(struct device *dev)
>  	struct irqsteer_data *irqsteer_data = dev_get_drvdata(dev);
>  	int ret;
>
> -	ret = clk_prepare_enable(irqsteer_data->ipg_clk);
> +	ret = clk_enable(irqsteer_data->ipg_clk);
>  	if (ret) {
>  		dev_err(dev, "failed to enable ipg clk: %d\n", ret);
>  		return ret;
> --
> 2.34.1
>
>