`wake_enabled` indicates cros_ec_resume() needs to call
disable_irq_wake() to undo enable_irq_wake() in cros_ec_suspend().
Initialize `wake_enabled` in cros_ec_register() and determine the flag
in cros_ec_suspend() instead of reset-after-used in cros_ec_resume().
Signed-off-by: Tzung-Bi Shih <[email protected]>
---
Changes from v3:
(https://patchwork.kernel.org/project/chrome-platform/patch/[email protected]/)
- Change the patch title.
- Simplify by initializing wake_enabled in cros_ec_register().
No changes from v2.
Changes from v1:
(https://lore.kernel.org/lkml/[email protected]/T/#u)
- Use imperative mood in commit message.
drivers/platform/chrome/cros_ec.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
index 25cd8df6e7b0..b6604a9ab315 100644
--- a/drivers/platform/chrome/cros_ec.c
+++ b/drivers/platform/chrome/cros_ec.c
@@ -191,6 +191,7 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
ec_dev->max_passthru = 0;
ec_dev->ec = NULL;
ec_dev->pd = NULL;
+ ec_dev->wake_enabled = false;
ec_dev->din = devm_kzalloc(dev, ec_dev->din_size, GFP_KERNEL);
if (!ec_dev->din)
@@ -383,10 +384,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.
--
2.35.1.265.g69c8d7142f-goog
On Tue, Feb 15, 2022 at 8:37 PM Tzung-Bi Shih <[email protected]> wrote:
>
> `wake_enabled` indicates cros_ec_resume() needs to call
> disable_irq_wake() to undo enable_irq_wake() in cros_ec_suspend().
>
> Initialize `wake_enabled` in cros_ec_register() and determine the flag
> in cros_ec_suspend() instead of reset-after-used in cros_ec_resume().
>
> Signed-off-by: Tzung-Bi Shih <[email protected]>
One minor thing to consider, but regardless:
Reviewed-by: Prashant Malani <[email protected]>
> ---
> Changes from v3:
> (https://patchwork.kernel.org/project/chrome-platform/patch/[email protected]/)
> - Change the patch title.
> - Simplify by initializing wake_enabled in cros_ec_register().
>
> No changes from v2.
>
> Changes from v1:
> (https://lore.kernel.org/lkml/[email protected]/T/#u)
> - Use imperative mood in commit message.
>
> drivers/platform/chrome/cros_ec.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
> index 25cd8df6e7b0..b6604a9ab315 100644
> --- a/drivers/platform/chrome/cros_ec.c
> +++ b/drivers/platform/chrome/cros_ec.c
> @@ -191,6 +191,7 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
> ec_dev->max_passthru = 0;
> ec_dev->ec = NULL;
> ec_dev->pd = NULL;
> + ec_dev->wake_enabled = false;
>
> ec_dev->din = devm_kzalloc(dev, ec_dev->din_size, GFP_KERNEL);
> if (!ec_dev->din)
> @@ -383,10 +384,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;
> - }
> +
Better to leave it as is, and ensure "wake_enabled" is cleared after resume?
Will result in a smaller diff.
I'll leave it up to you.
-Prashant
On Tue, Feb 15, 2022 at 09:47:09PM -0800, Prashant Malani wrote:
> On Tue, Feb 15, 2022 at 8:37 PM Tzung-Bi Shih <[email protected]> wrote:
> >
> > `wake_enabled` indicates cros_ec_resume() needs to call
> > disable_irq_wake() to undo enable_irq_wake() in cros_ec_suspend().
> >
> > Initialize `wake_enabled` in cros_ec_register() and determine the flag
> > in cros_ec_suspend() instead of reset-after-used in cros_ec_resume().
After reconsidering the 2 options in [1], I feel the flag needs to be set in
cros_ec_suspend() just in case if someone changes the wakeup capability per
[2].
[1]: https://patchwork.kernel.org/project/chrome-platform/patch/[email protected]/#24739778
[2]: https://patchwork.kernel.org/project/chrome-platform/patch/[email protected]/#24740205
Will change it back in the next version, pardon me.
> > @@ -383,10 +384,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;
> > - }
> > +
>
> Better to leave it as is, and ensure "wake_enabled" is cleared after resume?
> Will result in a smaller diff.
No, cros_ec_suspend() uses the flag to tell cros_ec_resume(): don't forget to
call disable_irq_wake(). It shouldn't be reset after used by
cros_ec_resume().