> -----Original Message----- > From: Marc Zyngier <m...@kernel.org> > Sent: 2020年7月17日 16:58 > To: Joakim Zhang <qiangqing.zh...@nxp.com> > Cc: t...@linutronix.de; ja...@lakedaemon.net; shawn...@kernel.org; > s.ha...@pengutronix.de; ker...@pengutronix.de; feste...@gmail.com; > dl-linux-imx <linux-...@nxp.com>; linux-kernel@vger.kernel.org; > linux-arm-ker...@lists.infradead.or > Subject: Re: [PATCH 2/2] irqchip: imx-intmux: add runtime PM support > > On 2020-07-16 20:32, Joakim Zhang wrote: > > Add runtime PM support for intmux interrupt controller. > > Same as the previous patch. The changes are significant enough that you need > to write a proper change log. > > > > > Signed-off-by: Joakim Zhang <qiangqing.zh...@nxp.com> > > --- > > drivers/irqchip/irq-imx-intmux.c | 31 +++++++++++++++++++++++++------ > > 1 file changed, 25 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/irqchip/irq-imx-intmux.c > > b/drivers/irqchip/irq-imx-intmux.c > > index 6095f76c4f0d..a631815c7bb4 100644 > > --- a/drivers/irqchip/irq-imx-intmux.c > > +++ b/drivers/irqchip/irq-imx-intmux.c > > @@ -53,6 +53,7 @@ > > #include <linux/of_irq.h> > > #include <linux/of_platform.h> > > #include <linux/spinlock.h> > > +#include <linux/pm_runtime.h> > > > > #define CHANIER(n) (0x10 + (0x40 * n)) > > #define CHANIPR(n) (0x20 + (0x40 * n)) > > @@ -60,6 +61,7 @@ > > #define CHAN_MAX_NUM 0x8 > > > > struct intmux_irqchip_data { > > + struct irq_chip chip; > > int chanidx; > > int irq; > > struct irq_domain *domain; > > @@ -123,8 +125,10 @@ static struct irq_chip imx_intmux_irq_chip = { > > static int imx_intmux_irq_map(struct irq_domain *h, unsigned int irq, > > irq_hw_number_t hwirq) > > { > > - irq_set_chip_data(irq, h->host_data); > > - irq_set_chip_and_handler(irq, &imx_intmux_irq_chip, > > handle_level_irq); > > + struct intmux_irqchip_data *data = h->host_data; > > + > > + irq_set_chip_data(irq, data); > > + irq_set_chip_and_handler(irq, &data->chip, handle_level_irq); > > > > return 0; > > } > > @@ -244,6 +248,10 @@ static int imx_intmux_probe(struct > > platform_device > > *pdev) > > return -ENOMEM; > > } > > > > + pm_runtime_get_noresume(&pdev->dev); > > + pm_runtime_set_active(&pdev->dev); > > + pm_runtime_enable(&pdev->dev); > > + > > ret = clk_prepare_enable(data->ipg_clk); > > if (ret) { > > dev_err(&pdev->dev, "failed to enable ipg clk: %d\n", ret); @@ > > -251,6 +259,8 @@ static int imx_intmux_probe(struct platform_device > > *pdev) > > } > > > > for (i = 0; i < channum; i++) { > > + data->irqchip_data[i].chip = imx_intmux_irq_chip; > > + data->irqchip_data[i].chip.parent_device = &pdev->dev; > > At some point, we will have to find a way to throw the parent_device thing out > of the irq_chip structure. This thing is a liability. > Nothing you can do about it for now though. > > > data->irqchip_data[i].chanidx = i; > > > > data->irqchip_data[i].irq = irq_of_parse_and_map(np, i); @@ > > -279,6 > > +289,12 @@ static int imx_intmux_probe(struct platform_device > > *pdev) > > > > platform_set_drvdata(pdev, data); > > > > + /* > > + * Let pm_runtime_put() disable clock. > > + * If CONFIG_PM is not enabled, the clock will stay powered. > > + */ > > + pm_runtime_put(&pdev->dev); > > + > > return 0; > > out: > > clk_disable_unprepare(data->ipg_clk); > > @@ -300,7 +316,7 @@ static int imx_intmux_remove(struct > > platform_device > > *pdev) > > irq_domain_remove(data->irqchip_data[i].domain); > > } > > > > - clk_disable_unprepare(data->ipg_clk); > > + pm_runtime_disable(&pdev->dev); > > > > return 0; > > } > > @@ -322,7 +338,7 @@ static void imx_intmux_restore_regs(struct > > intmux_data *data) > > writel_relaxed(data->saved_reg[i], data->regs + CHANIER(i)); } > > > > -static int imx_intmux_suspend(struct device *dev) > > +static int imx_intmux_runtime_suspend(struct device *dev) > > { > > struct intmux_data *data = dev_get_drvdata(dev); > > > > @@ -332,7 +348,7 @@ static int imx_intmux_suspend(struct device *dev) > > return 0; > > } > > > > -static int imx_intmux_resume(struct device *dev) > > +static int imx_intmux_runtime_resume(struct device *dev) > > You just introduced these two functions, and rename them immediately? > > > { > > struct intmux_data *data = dev_get_drvdata(dev); > > int ret; > > @@ -349,7 +365,10 @@ static int imx_intmux_resume(struct device *dev) > > #endif > > > > static const struct dev_pm_ops imx_intmux_pm_ops = { > > - SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(imx_intmux_suspend, > imx_intmux_resume) > > + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, > > + pm_runtime_force_resume) > > + SET_RUNTIME_PM_OPS(imx_intmux_runtime_suspend, > > + imx_intmux_runtime_resume, NULL) > > }; > > > > static const struct of_device_id imx_intmux_id[] = { > > I think you'd might as well squash the two patches together. > Splitting the two serves little purpose.
Thanks Marc for your kindly review, I will improve all following your comments in next version. Best Regards, Joakim Zhang > Thanks, > > M. > -- > Jazz is not dead. It just smells funny...