Re: [PATCH V4 4/5] gpio: gpio-xilinx: Add support for suspend and resume
From: Linus Walleij <hidden>
Date: 2021-01-07 09:47:45
Also in:
linux-arm-kernel, lkml
On Wed, Jan 6, 2021 at 1:27 PM Srinivas Neeli [off-list ref] wrote:
Add support for suspend and resume, pm runtime suspend and resume. Added free and request calls. Signed-off-by: Srinivas Neeli <redacted>
(...)
+static int xgpio_request(struct gpio_chip *chip, unsigned int offset)
+{
+ int ret;
+
+ ret = pm_runtime_get_sync(chip->parent);
+ /*
+ * If the device is already active pm_runtime_get() will return 1 on
+ * success, but gpio_request still needs to return 0.
+ */
+ return ret < 0 ? ret : 0;
+}That's clever. I think more GPIO drivers should be doing it like this, today I think most just ignore the return code.
+static int __maybe_unused xgpio_suspend(struct device *dev) +static int __maybe_unused xgpio_resume(struct device *dev)
Those look good.
quoted hunk ↗ jump to hunk
/** * xgpio_remove - Remove method for the GPIO device. * @pdev: pointer to the platform device@@ -289,7 +323,10 @@ static int xgpio_remove(struct platform_device *pdev) { struct xgpio_instance *gpio = platform_get_drvdata(pdev); - clk_disable_unprepare(gpio->clk); + if (!pm_runtime_suspended(&pdev->dev)) + clk_disable_unprepare(gpio->clk); + + pm_runtime_disable(&pdev->dev);
This looks complex and racy. What if the device is resumed after you executed the first part of the statement. The normal sequence is: pm_runtime_get_sync(dev); pm_runtime_put_noidle(dev); pm_runtime_disable(dev); This will make sure the clock is enabled and pm runtime is disabled. After this you can unconditionally call clk_disable_unprepare(gpio->clk); It is what you are doing on the errorpath of probe(). Yours, Linus Walleij