Re: [PATCH v5 6/9] irqchip/imx-irqsteer: Let devres own the clock and runtime PM

[Radu Rendec <radu@rendec.net>]

On Fri, 2026-08-21 at 19:10 +0900, Zhipeng.wang_1@oss.nxp.com wrote:
> From: Zhipeng Wang <zhipeng.wang_1@nxp.com>
> 
> In preparation for making the driver unbindable/reloadable, let the
> driver core own the clock and runtime PM lifetime so that the probe()
> error path and remove() do not have to hand-balance them:
> 
>  - 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;
>  - enable runtime PM with devm_pm_runtime_set_active_enabled(), which
>    marks the device active (matching the enabled clock) and disables
>    runtime PM on unbind.
> 
> The device may be runtime-suspended at unbind time (autosuspend), in
> which case the runtime suspend callback has already dropped the clock
> enable count. The devres clk_disable_unprepare() that runs after
> remove() would then underflow the enable count. Resume the device in
> remove() so the clock is enabled when devres tears it down, keeping the
> count balanced.
> 
> With the clock and runtime PM owned by devres, remove() and the probe()
> error path only have to dispose of the parent IRQ mappings.
> 
> Suggested-by: Fabio Estevam <festevam@nabladev.com>
> Signed-off-by: Zhipeng Wang <zhipeng.wang_1@nxp.com>
> ---
>  drivers/irqchip/irq-imx-irqsteer.c | 37 +++++++++++++++++++-----------
>  1 file changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c
> index b63bf957ab88..c2f58787f9a8 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);
> @@ -275,12 +269,21 @@ static int imx_irqsteer_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, data);
>  
> -	pm_runtime_set_active(&pdev->dev);
> -	pm_runtime_enable(&pdev->dev);
> +	ret = devm_pm_runtime_set_active_enabled(&pdev->dev);
> +	if (ret)
> +		goto err_irq;
>  
>  	return 0;
> +
> +err_irq:
> +	for (i = 0; i < data->irq_count; i++) {
> +		if (!data->irq[i])
> +			break;
> +
> +		irq_set_chained_handler_and_data(data->irq[i], NULL, NULL);
> +		irq_dispose_mapping(data->irq[i]);
> +	}
>  out:
> -	clk_disable_unprepare(data->ipg_clk);

With this change, the "out" label becomes useless, and the error path
for devm_irq_domain_instantiate() above can simply do:
	return PTR_ERR(data->domain);

In that same vein, the err_irq label can be eliminated as well by
negating the condition on the "if" statement. What I mean is:
	ret = devm_pm_runtime_set_active_enabled(&pdev->dev);
	if (!ret)
		return 0;

	for (i = 0; i < data->irq_count; i++) {

In fact, the "for" loop is identical to the one in imx_irqsteer_remove(),
so it can be moved to an inline function. Then, assuming the new inline
function is called imx_irqsteer_unmap_parent_irq(), the probe code can
be re-written as:
	ret = devm_pm_runtime_set_active_enabled(&pdev->dev);
	if (ret)
		imx_irqsteer_unmap_parent_irq(pdev);

	return ret;

Other than that, the patch looks good to me.

>  	return ret;
>  }
>  
> @@ -289,6 +292,14 @@ static void imx_irqsteer_remove(struct platform_device *pdev)
>  	struct irqsteer_data *irqsteer_data = platform_get_drvdata(pdev);
>  	int i;
>  
> +	/*
> +	 * 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.
> +	 */
> +	pm_runtime_resume_and_get(&pdev->dev);
> +
>  	for (i = 0; i < irqsteer_data->irq_count; i++) {
>  		if (!irqsteer_data->irq[i])
>  			break;
> @@ -297,8 +308,6 @@ static void imx_irqsteer_remove(struct platform_device *pdev)
>  						 NULL, NULL);
>  		irq_dispose_mapping(irqsteer_data->irq[i]);
>  	}
> -
> -	clk_disable_unprepare(irqsteer_data->ipg_clk);
>  }
>  
>  #ifdef CONFIG_PM
> @@ -328,7 +337,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;
>  }
> @@ -338,7 +347,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;