Messages in this thread | | | From | Linus Walleij <> | Date | Thu, 7 Jan 2021 10:46:51 +0100 | Subject | Re: [PATCH V4 4/5] gpio: gpio-xilinx: Add support for suspend and resume |
| |
On Wed, Jan 6, 2021 at 1:27 PM Srinivas Neeli <srinivas.neeli@xilinx.com> wrote:
> Add support for suspend and resume, pm runtime suspend and resume. > Added free and request calls. > > Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com> (...)
> +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.
> /** > * 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
| |