From: Linus Walleij <linus.walleij@linaro.org> To: Srinivas Neeli <srinivas.neeli@xilinx.com> Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>, Michal Simek <michal.simek@xilinx.com>, Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>, sgoud@xilinx.com, Robert Hancock <hancock@sedsystems.ca>, William Breathitt Gray <vilhelm.gray@gmail.com>, Syed Nayyar Waris <syednwaris@gmail.com>, "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>, Linux ARM <linux-arm-kernel@lists.infradead.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, git@xilinx.com Subject: Re: [PATCH V4 4/5] gpio: gpio-xilinx: Add support for suspend and resume Date: Thu, 7 Jan 2021 10:46:51 +0100 [thread overview] Message-ID: <CACRpkdYLp0uuB-QO5HvLH222TkCjk54JmftveHgpiW1JExF7DQ@mail.gmail.com> (raw) In-Reply-To: <1609936000-28378-5-git-send-email-srinivas.neeli@xilinx.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Linus Walleij <linus.walleij@linaro.org> To: Srinivas Neeli <srinivas.neeli@xilinx.com> Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>, sgoud@xilinx.com, Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>, William Breathitt Gray <vilhelm.gray@gmail.com>, Michal Simek <michal.simek@xilinx.com>, Robert Hancock <hancock@sedsystems.ca>, Bartosz Golaszewski <bgolaszewski@baylibre.com>, git@xilinx.com, Syed Nayyar Waris <syednwaris@gmail.com>, Linux ARM <linux-arm-kernel@lists.infradead.org> Subject: Re: [PATCH V4 4/5] gpio: gpio-xilinx: Add support for suspend and resume Date: Thu, 7 Jan 2021 10:46:51 +0100 [thread overview] Message-ID: <CACRpkdYLp0uuB-QO5HvLH222TkCjk54JmftveHgpiW1JExF7DQ@mail.gmail.com> (raw) In-Reply-To: <1609936000-28378-5-git-send-email-srinivas.neeli@xilinx.com> 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-01-07 9:47 UTC|newest] Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-01-06 12:26 [PATCH V4 0/5] gpio-xilinx: Update on xilinx gpio driver Srinivas Neeli 2021-01-06 12:26 ` Srinivas Neeli 2021-01-06 12:26 ` [PATCH V4 1/5] gpio: gpio-xilinx: Simplify with dev_err_probe() Srinivas Neeli 2021-01-06 12:26 ` Srinivas Neeli 2021-01-07 9:13 ` Linus Walleij 2021-01-07 9:13 ` Linus Walleij 2021-01-06 12:26 ` [PATCH V4 2/5] gpio: gpio-xilinx: Reduce spinlock array to array Srinivas Neeli 2021-01-06 12:26 ` Srinivas Neeli 2021-01-07 9:14 ` Linus Walleij 2021-01-07 9:14 ` Linus Walleij 2021-01-06 12:26 ` [PATCH V4 3/5] gpio: gpio-xilinx: Add interrupt support Srinivas Neeli 2021-01-06 12:26 ` Srinivas Neeli 2021-01-07 9:26 ` Linus Walleij 2021-01-07 9:26 ` Linus Walleij 2021-01-06 12:26 ` [PATCH V4 4/5] gpio: gpio-xilinx: Add support for suspend and resume Srinivas Neeli 2021-01-06 12:26 ` Srinivas Neeli 2021-01-07 9:46 ` Linus Walleij [this message] 2021-01-07 9:46 ` Linus Walleij 2021-01-08 11:41 ` Srinivas Neeli 2021-01-08 11:41 ` Srinivas Neeli 2021-01-09 0:25 ` Linus Walleij 2021-01-09 0:25 ` Linus Walleij 2021-01-06 12:26 ` [PATCH V4 5/5] gpio: gpio-xilinx: Add check if width exceeds 32 Srinivas Neeli 2021-01-06 12:26 ` Srinivas Neeli 2021-01-07 10:17 ` Linus Walleij 2021-01-07 10:17 ` Linus Walleij 2021-01-07 10:29 ` Michal Simek 2021-01-07 10:29 ` Michal Simek 2021-01-07 10:47 ` Linus Walleij 2021-01-07 10:47 ` Linus Walleij 2021-01-07 10:52 ` Michal Simek 2021-01-07 10:52 ` Michal Simek
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CACRpkdYLp0uuB-QO5HvLH222TkCjk54JmftveHgpiW1JExF7DQ@mail.gmail.com \ --to=linus.walleij@linaro.org \ --cc=bgolaszewski@baylibre.com \ --cc=git@xilinx.com \ --cc=hancock@sedsystems.ca \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-gpio@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=michal.simek@xilinx.com \ --cc=sgoud@xilinx.com \ --cc=shubhrajyoti.datta@xilinx.com \ --cc=srinivas.neeli@xilinx.com \ --cc=syednwaris@gmail.com \ --cc=vilhelm.gray@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.