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

[Zhipeng Wang (OSS)]

> Subject: Re: [PATCH v5 6/9] irqchip/imx-irqsteer: Let devres own the clock and
> runtime PM
> 
> On Fri, 2026-08-21 at 19:10 +0900, Zhipeng.wang_1@xxxxxxxxxxx wrote:
> > From: Zhipeng Wang <zhipeng.wang_1@xxxxxxx>
> >
> > 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@xxxxxxxxxxxx>
> > Signed-off-by: Zhipeng Wang <zhipeng.wang_1@xxxxxxx>
> > ---
> >  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.
> 

Hi Radu,

Thank you for the careful review and the suggestions, they are all good
points. I'll apply them in v6:

 - You're right that the out label becomes pointless. Since the clock is
   owned by devres there is nothing left to clean up at the end, so I'll
   drop the out label and return from the failure sites directly.

 - Good idea on removing the err_irq label too. I'll move
   devm_pm_runtime_set_active_enabled() before the IRQ domain is created,
   so its failure path has nothing to unwind and can just return, which
   lets the err_irq label go away.

 - And thanks for spotting that the cleanup loop is duplicated with
   imx_irqsteer_remove(). With the change above, probe() no longer needs
   that loop at all.

Thanks again,
Zhipeng

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