RE: [PATCH v4 5/5] irqchip/imx-irqsteer: Allow building as module

NXP Confidential
> -----Original Message-----
> From: Frank Li (OSS) <[email protected]>
> Sent: 2026年8月19日 23:17
> To: Zhipeng Wang (OSS) <[email protected]>
> Cc: Thomas Gleixner <[email protected]>; Marc Zyngier <[email protected]>;
> Frank Li <[email protected]>; Sascha Hauer <[email protected]>;
> Pengutronix Kernel Team <[email protected]>; Fabio Estevam
> <[email protected]>; Jindong Yue <[email protected]>; Xuegang Liu
> <[email protected]>; [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH v4 5/5] irqchip/imx-irqsteer: Allow building as module
>
> On Wed, Aug 19, 2026 at 06:05:43PM +0900, [email protected]
> wrote:
> > From: Jindong Yue <[email protected]>
> >
> > 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/[email protected]/

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 <[email protected]>
> > Signed-off-by: Zhipeng Wang <[email protected]>
> > ---
> >  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
> >
> >