Re: [PATCH v3] gpio: mxc: add runtime pm support

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Thu, Jul 6, 2023 at 5:56 PM Shenwei Wang <shenwei.wang@xxxxxxx> wrote:
>
> Add runtime PM support and allow the GPIO controller to enter
> into runtime suspend automatically when not in use to save power.
> However, it will automatically resume and enable clocks when a
> GPIO or IRQ is requested.
>
> While putting the GPIO module itself into power saving mode may not
> have an obvious impact on current dissipation, the function is necessary
> because the GPIO module disables its clock when idle. This enables the
> system an opportunity to power off the parent subsystem, and this conserves
> more power. The typical i.MX8 SoC features up to 8 GPIO controllers, but
> most of the controllers often remain unused.

Looks better, thank you.
Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>

> Signed-off-by: Shenwei Wang <shenwei.wang@xxxxxxx>
> ---
> v3:
>   - Improved the error handling logic in the probe() function. Thanks a
>     lot to AndyS for reviewing and pointing out the issue.
>
> v2:
>   - improved the patch comments
>   - using pm_runtime_resume_and_get to avoid reference count disbalance
>     when errors
>   - removed the __maybe_unused identifier
>
>  drivers/gpio/gpio-mxc.c | 61 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 59 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
> index 9d0cec4b82a3..a9fb6bd9aa6f 100644
> --- a/drivers/gpio/gpio-mxc.c
> +++ b/drivers/gpio/gpio-mxc.c
> @@ -17,6 +17,7 @@
>  #include <linux/irqchip/chained_irq.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
>  #include <linux/syscore_ops.h>
> @@ -382,6 +383,23 @@ static int mxc_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
>         return irq_find_mapping(port->domain, offset);
>  }
>
> +static int mxc_gpio_request(struct gpio_chip *chip, unsigned int offset)
> +{
> +       int ret;
> +
> +       ret = gpiochip_generic_request(chip, offset);
> +       if (ret)
> +               return ret;
> +
> +       return pm_runtime_resume_and_get(chip->parent);
> +}
> +
> +static void mxc_gpio_free(struct gpio_chip *chip, unsigned int offset)
> +{
> +       gpiochip_generic_free(chip, offset);
> +       pm_runtime_put(chip->parent);
> +}
> +
>  static int mxc_gpio_probe(struct platform_device *pdev)
>  {
>         struct device_node *np = pdev->dev.of_node;
> @@ -429,6 +447,10 @@ static int mxc_gpio_probe(struct platform_device *pdev)
>         if (of_device_is_compatible(np, "fsl,imx7d-gpio"))
>                 port->power_off = true;
>
> +       pm_runtime_get_noresume(&pdev->dev);
> +       pm_runtime_set_active(&pdev->dev);
> +       pm_runtime_enable(&pdev->dev);
> +
>         /* disable the interrupt and clear the status */
>         writel(0, port->base + GPIO_IMR);
>         writel(~0, port->base + GPIO_ISR);
> @@ -459,8 +481,8 @@ static int mxc_gpio_probe(struct platform_device *pdev)
>         if (err)
>                 goto out_bgio;
>
> -       port->gc.request = gpiochip_generic_request;
> -       port->gc.free = gpiochip_generic_free;
> +       port->gc.request = mxc_gpio_request;
> +       port->gc.free = mxc_gpio_free;
>         port->gc.to_irq = mxc_gpio_to_irq;
>         port->gc.base = (pdev->id < 0) ? of_alias_get_id(np, "gpio") * 32 :
>                                              pdev->id * 32;
> @@ -482,6 +504,8 @@ static int mxc_gpio_probe(struct platform_device *pdev)
>                 goto out_bgio;
>         }
>
> +       irq_domain_set_pm_device(port->domain, &pdev->dev);
> +
>         /* gpio-mxc can be a generic irq chip */
>         err = mxc_gpio_init_gc(port, irq_base);
>         if (err < 0)
> @@ -490,12 +514,15 @@ static int mxc_gpio_probe(struct platform_device *pdev)
>         list_add_tail(&port->node, &mxc_gpio_ports);
>
>         platform_set_drvdata(pdev, port);
> +       pm_runtime_put_autosuspend(&pdev->dev);
>
>         return 0;
>
>  out_irqdomain_remove:
>         irq_domain_remove(port->domain);
>  out_bgio:
> +       pm_runtime_disable(&pdev->dev);
> +       pm_runtime_put_noidle(&pdev->dev);
>         clk_disable_unprepare(port->clk);
>         dev_info(&pdev->dev, "%s failed with errno %d\n", __func__, err);
>         return err;
> @@ -572,6 +599,30 @@ static bool mxc_gpio_set_pad_wakeup(struct mxc_gpio_port *port, bool enable)
>         return ret;
>  }
>
> +static int mxc_gpio_runtime_suspend(struct device *dev)
> +{
> +       struct mxc_gpio_port *port = dev_get_drvdata(dev);
> +
> +       mxc_gpio_save_regs(port);
> +       clk_disable_unprepare(port->clk);
> +
> +       return 0;
> +}
> +
> +static int mxc_gpio_runtime_resume(struct device *dev)
> +{
> +       struct mxc_gpio_port *port = dev_get_drvdata(dev);
> +       int ret;
> +
> +       ret = clk_prepare_enable(port->clk);
> +       if (ret)
> +               return ret;
> +
> +       mxc_gpio_restore_regs(port);
> +
> +       return 0;
> +}
> +
>  static int __maybe_unused mxc_gpio_noirq_suspend(struct device *dev)
>  {
>         struct platform_device *pdev = to_platform_device(dev);
> @@ -597,14 +648,19 @@ static int __maybe_unused mxc_gpio_noirq_resume(struct device *dev)
>
>  static const struct dev_pm_ops mxc_gpio_dev_pm_ops = {
>         SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mxc_gpio_noirq_suspend, mxc_gpio_noirq_resume)
> +       SET_RUNTIME_PM_OPS(mxc_gpio_runtime_suspend, mxc_gpio_runtime_resume, NULL)
>  };
>
>  static int mxc_gpio_syscore_suspend(void)
>  {
>         struct mxc_gpio_port *port;
> +       int ret;
>
>         /* walk through all ports */
>         list_for_each_entry(port, &mxc_gpio_ports, node) {
> +               ret = clk_prepare_enable(port->clk);
> +               if (ret)
> +                       return ret;
>                 mxc_gpio_save_regs(port);
>                 clk_disable_unprepare(port->clk);
>         }
> @@ -625,6 +681,7 @@ static void mxc_gpio_syscore_resume(void)
>                         return;
>                 }
>                 mxc_gpio_restore_regs(port);
> +               clk_disable_unprepare(port->clk);
>         }
>  }
>
> --
> 2.34.1
>


-- 
With Best Regards,
Andy Shevchenko