[PATCH v4 5/5] irqchip/imx-irqsteer: Allow building as module
Frank Li
Frank.li at oss.nxp.com
Thu Aug 20 09:53:08 PDT 2026
On Thu, Aug 20, 2026 at 09:53:13AM +0000, Zhipeng Wang (OSS) wrote:
>
>
>
> NXP Confidential
Please remove this tags, It is public mail list. Everyone can see context.
Frank
> > -----Original Message-----
> > From: Frank Li (OSS) <frank.li at oss.nxp.com>
> > Sent: 2026年8月19日 23:17
> > To: Zhipeng Wang (OSS) <zhipeng.wang_1 at oss.nxp.com>
> > Cc: Thomas Gleixner <tglx at kernel.org>; Marc Zyngier <maz at kernel.org>;
> > Frank Li <frank.li at nxp.com>; Sascha Hauer <s.hauer at pengutronix.de>;
> > Pengutronix Kernel Team <kernel at pengutronix.de>; Fabio Estevam
> > <festevam at gmail.com>; Jindong Yue <jindong.yue at nxp.com>; Xuegang Liu
> > <xuegang.liu at nxp.com>; linux-kernel at vger.kernel.org; imx at lists.linux.dev;
> > linux-arm-kernel at lists.infradead.org
> > Subject: Re: [PATCH v4 5/5] irqchip/imx-irqsteer: Allow building as module
> >
> > On Wed, Aug 19, 2026 at 06:05:43PM +0900, Zhipeng.wang_1 at oss.nxp.com
> > wrote:
> > > From: Jindong Yue <jindong.yue at nxp.com>
> > >
> > > Make the driver buildable as a module by turning the Kconfig symbol
> > > into a tristate and using module_platform_driver() instead of
> > > builtin_platform_driver().
> > >
> > > Now that the driver can be unloaded and reloaded, 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;
> >
> > devm_clk_get_enable() will do prepare() and enable(). tear down also do
> > unprepare() and disable()
> >
> > To avoid both devm_clk() and runtime pm suspend both unprepare and
> > disable() clock to make wrong clock refererence.
> >
> > need in remove function
> >
> > /*
> > * Resume the device so runtime_resume() re-enables the clock.
> > * devm cleanup (clk_disable_unprepare) runs after .remove() returns,
> > * so the clock will be enabled and the disable is safe.
> > */
> > pm_runtime_resume_and_get(dev);
> >
> > /* devm_clk_get_enable and devm_pm_runtime_enable clean up
> > automatically */
> >
> > Frank
> >
> Hi Frank,
>
> Thanks for catching the clock reference-count underflow — you're right.
>
> With devm_clk_get_enabled() doing prepare+enable and the devres teardown
> doing unprepare+disable, having runtime suspend also drop the enable count
> means the clock can be disabled/unprepared twice (once by runtime PM on the
> suspend path, once by devres on unbind), underflowing the reference count.
>
> I've applied your suggestion in remove():
>
> /*
> * 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);
>
> What will change in the next version:
> - The series is split per tglx's request: the PM/devres rework, the
> module-enable change, and the irqdomain helper cleanup are now
> separate patches rather than one blob.
>
> - Fabio's IRQ-count validation patch, which carries the same
> Fixes: 28528fca4908 tag, is folded into the series.
>
> Thomas suggested collecting the patches that carry
> Fixes: 28528fca4908 ("irqchip/imx-irqsteer: Add multi output interrupts
> support") into a single series. This standalone fix carries that same tag:
>
> [PATCH] irqchip/imx-irqsteer: Call chained_irq_exit() on the handler error path
> https://lore.kernel.org/all/20260807072626.1231517-1-Zhipeng.wang_1@oss.nxp.com/
>
> What's your view on it?
>
> Thanks,
> Zhipeng
>
> > > - 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.
> > >
> > > With the clock, runtime PM and IRQ domain all owned by devres,
> > > remove() and the probe() error path only have to dispose of the parent
> > > IRQ mappings.
> > >
> > > Signed-off-by: Jindong Yue <jindong.yue at nxp.com>
> > > Signed-off-by: Zhipeng Wang <zhipeng.wang_1 at nxp.com>
> > > ---
> > > drivers/irqchip/Kconfig | 2 +-
> > > drivers/irqchip/irq-imx-irqsteer.c | 36
> > > +++++++++++++++++-------------
> > > 2 files changed, 22 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index
> > > 20b77fbc51ee..105108d2e6ff 100644
> > > --- a/drivers/irqchip/Kconfig
> > > +++ b/drivers/irqchip/Kconfig
> > > @@ -555,7 +555,7 @@ config CSKY_APB_INTC
> > > the controller's register.
> > >
> > > config IMX_IRQSTEER
> > > - bool "i.MX IRQSTEER support"
> > > + tristate "i.MX IRQSTEER support"
> > > depends on ARCH_MXC || ARCH_S32 || COMPILE_TEST
> > > default y if ARCH_MXC || ARCH_S32
> > > select IRQ_DOMAIN
> > > diff --git a/drivers/irqchip/irq-imx-irqsteer.c
> > > b/drivers/irqchip/irq-imx-irqsteer.c
> > > index a9909ecb6fef..a60cc527e619 100644
> > > --- a/drivers/irqchip/irq-imx-irqsteer.c
> > > +++ b/drivers/irqchip/irq-imx-irqsteer.c
> > > @@ -10,6 +10,7 @@
> > > #include <linux/irqchip/chained_irq.h> #include <linux/irqdomain.h>
> > > #include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > #include <linux/of.h>
> > > #include <linux/of_irq.h>
> > > #include <linux/platform_device.h>
> > > @@ -193,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");
> > > @@ -226,12 +227,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); @@
> > > -271,12 +266,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);
> > > return ret;
> > > }
> > >
> > > @@ -293,8 +297,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
> > > @@ -324,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;
> > > }
> > > @@ -334,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;
> > > @@ -357,6 +359,7 @@ static const struct of_device_id imx_irqsteer_dt_ids[]
> > = {
> > > { .compatible = "nxp,s32n79-irqsteer", .data = &s32n79_data },
> > > {},
> > > };
> > > +MODULE_DEVICE_TABLE(of, imx_irqsteer_dt_ids);
> > >
> > > static struct platform_driver imx_irqsteer_driver = {
> > > .driver = {
> > > @@ -367,4 +370,7 @@ static struct platform_driver imx_irqsteer_driver = {
> > > .probe = imx_irqsteer_probe,
> > > .remove = imx_irqsteer_remove,
> > > };
> > > -builtin_platform_driver(imx_irqsteer_driver);
> > > +module_platform_driver(imx_irqsteer_driver);
> > > +
> > > +MODULE_DESCRIPTION("i.MX IRQSTEER interrupt multiplexer/remapper
> > > +driver"); MODULE_LICENSE("GPL");
> > > --
> > > 2.34.1
> > >
> > >
More information about the linux-arm-kernel
mailing list