diff mbox series

[v2] gpio: mxc: add runtime pm support

Message ID 20230705153713.2511647-1-shenwei.wang@nxp.com
State Superseded
Headers show
Series [v2] gpio: mxc: add runtime pm support | expand

Commit Message

Shenwei Wang July 5, 2023, 3:37 p.m. UTC 
Adds 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.

Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
---
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 | 64 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 62 insertions(+), 2 deletions(-)

--
2.34.1

Comments

Andy Shevchenko July 5, 2023, 7:03 p.m. UTC | #1 
On Wed, Jul 5, 2023 at 10:01 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Wed, Jul 5, 2023 at 6:37 PM Shenwei Wang <shenwei.wang@nxp.com> wrote:
> >
> > Adds runtime PM support and allow the GPIO controller to enter

Add

> > 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.

...

> > +       pm_runtime_set_active(&pdev->dev);
> > +       pm_runtime_enable(&pdev->dev);
> > +       err = pm_runtime_resume_and_get(&pdev->dev);
> > +       if (err < 0)
> > +               goto out_pm_dis;
>
> So, after this if an error happens, you will have PM left enabled and
> the next probe won't work as expected, right?

Just noticed that this patch completely screwed up the error handling
WRT clock and PM.

...

> >         platform_set_drvdata(pdev, port);
> > +       pm_runtime_put_autosuspend(&pdev->dev);
> >
> >         return 0;
>
> ...something here is missing?
>
> > +out_pm_dis:
> > +       pm_runtime_disable(&pdev->dev);
> > +       clk_disable_unprepare(port->clk);

This is definitely in the wrong position to begin with.

> >  out_irqdomain_remove:
> >         irq_domain_remove(port->domain);
> >  out_bgio:
diff mbox series

Patch

diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
index 9d0cec4b82a3..db9947342975 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,12 @@  static int mxc_gpio_probe(struct platform_device *pdev)
 	if (of_device_is_compatible(np, "fsl,imx7d-gpio"))
 		port->power_off = true;

+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+	err = pm_runtime_resume_and_get(&pdev->dev);
+	if (err < 0)
+		goto out_pm_dis;
+
 	/* disable the interrupt and clear the status */
 	writel(0, port->base + GPIO_IMR);
 	writel(~0, port->base + GPIO_ISR);
@@ -459,8 +483,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 +506,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,9 +516,13 @@  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_pm_dis:
+	pm_runtime_disable(&pdev->dev);
+	clk_disable_unprepare(port->clk);
 out_irqdomain_remove:
 	irq_domain_remove(port->domain);
 out_bgio:
@@ -572,6 +602,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 +651,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 +684,7 @@  static void mxc_gpio_syscore_resume(void)
 			return;
 		}
 		mxc_gpio_restore_regs(port);
+		clk_disable_unprepare(port->clk);
 	}
 }