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

[Marc Zyngier]

On Mon, 20 Jul 2020 11:42:37 +0100,
Joakim Zhang <qiangqing.zhang@xxxxxxx> 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@xxxxxxx>
> ---
>  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.

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.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.