RE: [PATCH V2 1/1] irqchip: imx-intmux: implement intmux PM

[Joakim Zhang <qiangqing.zhang@nxp.com>]

> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: 2020年7月25日 21:05
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> Cc: tglx@linutronix.de; jason@lakedaemon.net; shawnguo@kernel.org;
> s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
> dl-linux-imx <linux-imx@nxp.com>; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH V2 1/1] irqchip: imx-intmux: implement intmux PM
> 
> On Mon, 20 Jul 2020 11:42:37 +0100,
> Joakim Zhang <qiangqing.zhang@nxp.com> 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 <qiangqing.zhang@nxp.com>
> > ---
> >  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.