Re: [PATCH 2/2] irqchip: imx-intmux: add runtime PM support

[Marc Zyngier <maz@kernel.org>]

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.zhang@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,

         M.
-- 
Jazz is not dead. It just smells funny...