> -----Original Message----- > From: Marc Zyngier <[email protected]> > Sent: 2020年7月25日 21:05 > To: Joakim Zhang <[email protected]> > Cc: [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; > dl-linux-imx <[email protected]>; [email protected] > Subject: Re: [PATCH V2 1/1] irqchip: imx-intmux: implement intmux PM > > On Mon, 20 Jul 2020 11:42:37 +0100, > Joakim Zhang <[email protected]> wrote: > > > > When system suspended, we could explicitly disable clock to save power. > > And we need save registers' state since it could be lost after power > > off. > > > > Implement PM which will: > > 1) Without CONFIG_PM, clock is always on after probe stage. > > 2) With CONFIG_PM, clock is off after probe stage. > > 3) Disable clock and save registers' state when do system suspend and > > enable clock and restore registers' state while system resume. > > 4) Make Power Domain framework be able to shutdown the corresponding > > power domain of this device. > > > > Signed-off-by: Joakim Zhang <[email protected]> > > --- > > drivers/irqchip/irq-imx-intmux.c | 70 > > +++++++++++++++++++++++++++++++- > > 1 file changed, 68 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/irqchip/irq-imx-intmux.c > > b/drivers/irqchip/irq-imx-intmux.c > > index c27577c81126..5971603cc607 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; > > @@ -70,6 +72,7 @@ struct intmux_data { > > void __iomem *regs; > > struct clk *ipg_clk; > > int channum; > > + u32 *saved_reg; > > struct intmux_irqchip_data irqchip_data[]; > > }; > > > > @@ -120,8 +123,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; > > } > > @@ -232,6 +237,19 @@ static int imx_intmux_probe(struct platform_device > *pdev) > > data->channum = channum; > > raw_spin_lock_init(&data->lock); > > > > + if (IS_ENABLED(CONFIG_PM)) { > > + /* save CHANIER register */ > > + data->saved_reg = devm_kzalloc(&pdev->dev, > > + sizeof(unsigned int) * channum, > > This isn't consistent with the type of data->saved_reg. Consider using > sizeof(*data->saved_reg), which is guaranteed to be the right type.
Sorry for my mistake, I forgot to modify this unit. > It also begs the question: since this saved_reg array is allocated on a per > channel basis, why don't you have a per-channel additional u32 in the > intmux_irqchip_data structure instead? This would sidestep this extra > allocation altogether. Yes, this make sense. Thanks. Best Regards, Joakim Zhang > Thanks, > > M. > > -- > Without deviation from the norm, progress is not possible.
