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

On Thu, Aug 20, 2026 at 09:53:13AM +0000, Zhipeng Wang (OSS) wrote:
>
>
>
> 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?

Sorry, I missed, it looks good, you can combine to new fixes series.

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