| Message ID | 20220209045035.380615-4-tzungbi@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | platform/chrome: cros_ec: miscellaneous cleanups | expand |
Hi, On Tue, Feb 8, 2022 at 8:50 PM Tzung-Bi Shih <tzungbi@google.com> wrote: > > `wake_enabled` indicates cros_ec_resume() needs to call > disable_irq_wake() to undo enable_irq_wake() in cros_ec_suspend(). > > Determine `wake_enabled` in cros_ec_suspend() instead of > reset-after-used in cros_ec_resume(). It sounds like we can accomplish the same thing as this patch by either: - Initializing ec_dev->wake_enabled = false during cros_ec_register() or - Setting ec_dev->wake_enabled = false just before the device_may_wakeup(dev) check. Both of these options accomplish the same thing as this patch, with a much smaller diff.
On Tue, Feb 08, 2022 at 10:05:58PM -0800, Prashant Malani wrote: > On Tue, Feb 8, 2022 at 8:50 PM Tzung-Bi Shih <tzungbi@google.com> wrote: > > > > `wake_enabled` indicates cros_ec_resume() needs to call > > disable_irq_wake() to undo enable_irq_wake() in cros_ec_suspend(). > > > > Determine `wake_enabled` in cros_ec_suspend() instead of > > reset-after-used in cros_ec_resume(). > It sounds like we can accomplish the same thing as this patch by either: > - Initializing ec_dev->wake_enabled = false during cros_ec_register() This doesn't sound like a good idea. Value of device_may_wakeup(dev) changes during runtime. It should check in cros_ec_suspend() instead of in cros_ec_register(). > or > - Setting ec_dev->wake_enabled = false just before the > device_may_wakeup(dev) check. Did you mean: ec_dev->wake_enabled = false; if (device_may_wakeup(dev)) ec_dev->wake_enabled = !enable_irq_wake(ec_dev->irq); If so, this way is suboptimal to me. ec_dev->wake_enabled can be written once; however, it is written twice if device_may_wakeup(dev) is true.
On Wed, Feb 9, 2022 at 1:36 AM Tzung-Bi Shih <tzungbi@google.com> wrote: > > On Tue, Feb 08, 2022 at 10:05:58PM -0800, Prashant Malani wrote: > > On Tue, Feb 8, 2022 at 8:50 PM Tzung-Bi Shih <tzungbi@google.com> wrote: > > > > > > `wake_enabled` indicates cros_ec_resume() needs to call > > > disable_irq_wake() to undo enable_irq_wake() in cros_ec_suspend(). > > > > > > Determine `wake_enabled` in cros_ec_suspend() instead of > > > reset-after-used in cros_ec_resume(). > > It sounds like we can accomplish the same thing as this patch by either: > > - Initializing ec_dev->wake_enabled = false during cros_ec_register() > > This doesn't sound like a good idea. Value of device_may_wakeup(dev) changes > during runtime. It should check in cros_ec_suspend() instead of in > cros_ec_register(). Hmm, I'm pretty sure it shouldn't change for Cros EC. In any case, I'm not suggesting moving the check away from cros_ec_suspend(), just the initialization of the flag. > > > or > > - Setting ec_dev->wake_enabled = false just before the > > device_may_wakeup(dev) check. > > Did you mean: > > ec_dev->wake_enabled = false; > if (device_may_wakeup(dev)) > ec_dev->wake_enabled = !enable_irq_wake(ec_dev->irq); > > If so, this way is suboptimal to me. ec_dev->wake_enabled can be written > once; however, it is written twice if device_may_wakeup(dev) is true. I don't think it is a concern on any modern computer hardware, and seems more readable to me, but it's up to you.
On Tue, Feb 15, 2022 at 05:07:43PM -0800, Prashant Malani wrote: > On Wed, Feb 9, 2022 at 1:36 AM Tzung-Bi Shih <tzungbi@google.com> wrote: > > > > On Tue, Feb 08, 2022 at 10:05:58PM -0800, Prashant Malani wrote: > > > On Tue, Feb 8, 2022 at 8:50 PM Tzung-Bi Shih <tzungbi@google.com> wrote: > > > > > > > > `wake_enabled` indicates cros_ec_resume() needs to call > > > > disable_irq_wake() to undo enable_irq_wake() in cros_ec_suspend(). > > > > > > > > Determine `wake_enabled` in cros_ec_suspend() instead of > > > > reset-after-used in cros_ec_resume(). > > > It sounds like we can accomplish the same thing as this patch by either: > > > - Initializing ec_dev->wake_enabled = false during cros_ec_register() > > > > This doesn't sound like a good idea. Value of device_may_wakeup(dev) changes > > during runtime. It should check in cros_ec_suspend() instead of in > > cros_ec_register(). > > Hmm, I'm pretty sure it shouldn't change for Cros EC. In any case, I'm > not suggesting > moving the check away from cros_ec_suspend(), just the initialization > of the flag. Got it. I misunderstood. Let's go this direction. Will fix in the next version.
On Tue, Feb 15, 2022 at 8:13 PM Tzung-Bi Shih <tzungbi@google.com> wrote: > > On Tue, Feb 15, 2022 at 05:07:43PM -0800, Prashant Malani wrote: > > On Wed, Feb 9, 2022 at 1:36 AM Tzung-Bi Shih <tzungbi@google.com> wrote: > > > > > > On Tue, Feb 08, 2022 at 10:05:58PM -0800, Prashant Malani wrote: > > > > On Tue, Feb 8, 2022 at 8:50 PM Tzung-Bi Shih <tzungbi@google.com> wrote: > > > > > > > > > > `wake_enabled` indicates cros_ec_resume() needs to call > > > > > disable_irq_wake() to undo enable_irq_wake() in cros_ec_suspend(). > > > > > > > > > > Determine `wake_enabled` in cros_ec_suspend() instead of > > > > > reset-after-used in cros_ec_resume(). > > > > It sounds like we can accomplish the same thing as this patch by either: > > > > - Initializing ec_dev->wake_enabled = false during cros_ec_register() > > > > > > This doesn't sound like a good idea. Value of device_may_wakeup(dev) changes > > > during runtime. It should check in cros_ec_suspend() instead of in > > > cros_ec_register(). Actually, circling back to this (apologies), can you provide an example of this changing during runtime? I'm not aware of one, but I could be omitting some use cases that you might be familiar with. > > > > Hmm, I'm pretty sure it shouldn't change for Cros EC. In any case, I'm > > not suggesting > > moving the check away from cros_ec_suspend(), just the initialization > > of the flag. > > Got it. I misunderstood. Let's go this direction. Will fix in the next > version.
On Tue, Feb 15, 2022 at 09:55:13PM -0800, Prashant Malani wrote: > On Tue, Feb 15, 2022 at 8:13 PM Tzung-Bi Shih <tzungbi@google.com> wrote: > > > > On Tue, Feb 15, 2022 at 05:07:43PM -0800, Prashant Malani wrote: > > > On Wed, Feb 9, 2022 at 1:36 AM Tzung-Bi Shih <tzungbi@google.com> wrote: > > > > > > > > On Tue, Feb 08, 2022 at 10:05:58PM -0800, Prashant Malani wrote: > > > > > On Tue, Feb 8, 2022 at 8:50 PM Tzung-Bi Shih <tzungbi@google.com> wrote: > > > > > > > > > > > > `wake_enabled` indicates cros_ec_resume() needs to call > > > > > > disable_irq_wake() to undo enable_irq_wake() in cros_ec_suspend(). > > > > > > > > > > > > Determine `wake_enabled` in cros_ec_suspend() instead of > > > > > > reset-after-used in cros_ec_resume(). > > > > > It sounds like we can accomplish the same thing as this patch by either: > > > > > - Initializing ec_dev->wake_enabled = false during cros_ec_register() > > > > > > > > This doesn't sound like a good idea. Value of device_may_wakeup(dev) changes > > > > during runtime. It should check in cros_ec_suspend() instead of in > > > > cros_ec_register(). > Actually, circling back to this (apologies), can you provide an > example of this changing during runtime? > I'm not aware of one, but I could be omitting some use cases that you > might be familiar with. If someone `echo disabled > /sys/devices/.../spi0/spi0.0/power/wakeup`, the value of device_may_wakeup(dev) would change.
diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c index 303af630c03e..a97a47ae0472 100644 --- a/drivers/platform/chrome/cros_ec.c +++ b/drivers/platform/chrome/cros_ec.c @@ -342,6 +342,8 @@ int cros_ec_suspend(struct cros_ec_device *ec_dev) if (device_may_wakeup(dev)) ec_dev->wake_enabled = !enable_irq_wake(ec_dev->irq); + else + ec_dev->wake_enabled = false; disable_irq(ec_dev->irq); ec_dev->suspended = true; @@ -383,10 +385,9 @@ int cros_ec_resume(struct cros_ec_device *ec_dev) dev_dbg(ec_dev->dev, "Error %d sending resume event to ec", ret); - if (ec_dev->wake_enabled) { + if (ec_dev->wake_enabled) disable_irq_wake(ec_dev->irq); - ec_dev->wake_enabled = 0; - } + /* * Let the mfd devices know about events that occur during * suspend. This way the clients know what to do with them.
`wake_enabled` indicates cros_ec_resume() needs to call disable_irq_wake() to undo enable_irq_wake() in cros_ec_suspend(). Determine `wake_enabled` in cros_ec_suspend() instead of reset-after-used in cros_ec_resume(). Signed-off-by: Tzung-Bi Shih <tzungbi@google.com> --- drivers/platform/chrome/cros_ec.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)