Hi Linus,

-----Original Message-----
From: Linus Walleij <redacted>
Sent: Thursday, January 7, 2021 3:17 PM
To: Srinivas Neeli <redacted>
Cc: Bartosz Golaszewski <redacted>; Michal Simek
[off-list ref]; Shubhrajyoti Datta [off-list ref]; Srinivas
Goud [off-list ref]; Robert Hancock [off-list ref];
William Breathitt Gray [off-list ref]; Syed Nayyar Waris
[off-list ref]; open list:GPIO SUBSYSTEM <linux-
gpio@vger.kernel.org>; Linux ARM [off-list ref];
linux-kernel@vger.kernel.org; git [off-list ref]
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 [off-list ref]
wrote:

quoted

Add support for suspend and resume, pm runtime suspend and resume.
Added free and request calls.

Signed-off-by: Srinivas Neeli <redacted>

(...)

quoted

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

quoted

+static int __maybe_unused xgpio_suspend(struct device *dev) static
+int __maybe_unused xgpio_resume(struct device *dev)

Those look good.

quoted

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

Could you please explain more on this.
What is the need to call pm_runtime_get_sync(); in remove API ?

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel