If the IRQ is processed during resume, increment the wakeup count to the
specific mfd device based on the event, if the mfd device is wake enabled.
This helps in identifying the specific device that caused the wake.
Signed-off-by: Ravi Chandra Sadineni <[email protected]>
---
drivers/input/keyboard/cros_ec_keyb.c | 20 +++++++++++++++-----
drivers/mfd/cros_ec.c | 25 +++++++++++--------------
2 files changed, 26 insertions(+), 19 deletions(-)
diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
index 79eb29550c348..9b39289774405 100644
--- a/drivers/input/keyboard/cros_ec_keyb.c
+++ b/drivers/input/keyboard/cros_ec_keyb.c
@@ -245,12 +245,16 @@ static int cros_ec_keyb_work(struct notifier_block *nb,
switch (ckdev->ec->event_data.event_type) {
case EC_MKBP_EVENT_KEY_MATRIX:
/*
- * If EC is not the wake source, discard key state changes
- * during suspend.
+ * If keyb input device is not wake enabled, discard key
+ * state changes during suspend.
*/
- if (queued_during_suspend)
+ if (queued_during_suspend && ckdev->idev
+ && !device_may_wakeup(&ckdev->idev->dev))
return NOTIFY_OK;
+ else if (queued_during_suspend && ckdev->idev)
+ pm_wakeup_event(&ckdev->idev->dev, 0);
+
if (ckdev->ec->event_size != ckdev->cols) {
dev_err(ckdev->dev,
"Discarded incomplete key matrix event.\n");
@@ -270,13 +274,17 @@ static int cros_ec_keyb_work(struct notifier_block *nb,
case EC_MKBP_EVENT_BUTTON:
case EC_MKBP_EVENT_SWITCH:
/*
- * If EC is not the wake source, discard key state
+ * If bs is not wake enabled, discard key state
* changes during suspend. Switches will be re-checked in
* cros_ec_keyb_resume() to be sure nothing is lost.
*/
- if (queued_during_suspend)
+ if (queued_during_suspend && ckdev->bs_idev
+ && !device_may_wakeup(&ckdev->bs_idev->dev))
return NOTIFY_OK;
+ else if (queued_during_suspend && ckdev->bs_idev)
+ pm_wakeup_event(&ckdev->bs_idev->dev, 0);
+
if (ckdev->ec->event_data.event_type == EC_MKBP_EVENT_BUTTON) {
val = get_unaligned_le32(
&ckdev->ec->event_data.data.buttons);
@@ -522,6 +530,7 @@ static int cros_ec_keyb_register_bs(struct cros_ec_keyb *ckdev)
return ret;
}
+ device_init_wakeup(&idev->dev, true);
return 0;
}
@@ -598,6 +607,7 @@ static int cros_ec_keyb_register_matrix(struct cros_ec_keyb *ckdev)
return err;
}
+ device_init_wakeup(&idev->dev, true);
return 0;
}
diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
index d61024141e2b6..df9520365e54b 100644
--- a/drivers/mfd/cros_ec.c
+++ b/drivers/mfd/cros_ec.c
@@ -64,12 +64,14 @@ static irqreturn_t ec_irq_thread(int irq, void *data)
* cros_ec_get_next_event() returned an error (default value for
* wake_event is true)
*/
- if (wake_event && device_may_wakeup(ec_dev->dev))
+ if (device_may_wakeup(ec_dev->dev) && wake_event
+ && ec_dev->wake_enabled)
pm_wakeup_event(ec_dev->dev, 0);
if (ret > 0)
blocking_notifier_call_chain(&ec_dev->event_notifier,
- 0, ec_dev);
+ wake_event && ec_dev->wake_enabled,
+ ec_dev);
return IRQ_HANDLED;
}
@@ -229,7 +231,7 @@ int cros_ec_suspend(struct cros_ec_device *ec_dev)
}
EXPORT_SYMBOL(cros_ec_suspend);
-static void cros_ec_drain_events(struct cros_ec_device *ec_dev)
+static void cros_ec_report_events_during_suspend(struct cros_ec_device *ec_dev)
{
while (cros_ec_get_next_event(ec_dev, NULL) > 0)
blocking_notifier_call_chain(&ec_dev->event_notifier,
@@ -253,21 +255,16 @@ int cros_ec_resume(struct cros_ec_device *ec_dev)
dev_dbg(ec_dev->dev, "Error %d sending resume event to ec",
ret);
- /*
- * In some cases, we need to distinguish between events that occur
- * during suspend if the EC is not a wake source. For example,
- * keypresses during suspend should be discarded if it does not wake
- * the system.
- *
- * If the EC is not a wake source, drain the event queue and mark them
- * as "queued during suspend".
- */
if (ec_dev->wake_enabled) {
disable_irq_wake(ec_dev->irq);
ec_dev->wake_enabled = 0;
- } else {
- cros_ec_drain_events(ec_dev);
}
+ /*
+ * Let the mfd devices know about events that occur during
+ * suspend. This way the clients know what to do with them.
+ */
+ cros_ec_report_events_during_suspend(ec_dev);
+
return 0;
}
--
2.17.0.441.gb46fe60e1d-goog
Hi Ravi,
On Wed, May 23, 2018 at 11:29:58AM -0700, Ravi Chandra Sadineni wrote:
> If the IRQ is processed during resume, increment the wakeup count to the
> specific mfd device based on the event, if the mfd device is wake enabled.
> This helps in identifying the specific device that caused the wake.
>
> Signed-off-by: Ravi Chandra Sadineni <[email protected]>
> ---
> drivers/input/keyboard/cros_ec_keyb.c | 20 +++++++++++++++-----
> drivers/mfd/cros_ec.c | 25 +++++++++++--------------
> 2 files changed, 26 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
> index 79eb29550c348..9b39289774405 100644
> --- a/drivers/input/keyboard/cros_ec_keyb.c
> +++ b/drivers/input/keyboard/cros_ec_keyb.c
> @@ -245,12 +245,16 @@ static int cros_ec_keyb_work(struct notifier_block *nb,
> switch (ckdev->ec->event_data.event_type) {
> case EC_MKBP_EVENT_KEY_MATRIX:
> /*
> - * If EC is not the wake source, discard key state changes
> - * during suspend.
> + * If keyb input device is not wake enabled, discard key
> + * state changes during suspend.
> */
> - if (queued_during_suspend)
> + if (queued_during_suspend && ckdev->idev
> + && !device_may_wakeup(&ckdev->idev->dev))
> return NOTIFY_OK;
>
> + else if (queued_during_suspend && ckdev->idev)
> + pm_wakeup_event(&ckdev->idev->dev, 0);
Are we reporting wakeup event on the right device? Normally we only
report wakeups on hardware device whereas here we using input devices
which are logical abstractions...
In any case this can be collapsed a bit:
if (queued_during_suspend && ckdev->idev) {
if (!device_may_wakeup(&ckdev->idev->dev)
return NOTIFY_OK;
pm_wakeup_event(&ckdev->idev->dev, 0);
}
But I think you should actually look at ckdev->dev device here.
Also, why do we ignore EC_MKBP_EVENT_SYSRQ event?
> +
> if (ckdev->ec->event_size != ckdev->cols) {
> dev_err(ckdev->dev,
> "Discarded incomplete key matrix event.\n");
> @@ -270,13 +274,17 @@ static int cros_ec_keyb_work(struct notifier_block *nb,
> case EC_MKBP_EVENT_BUTTON:
> case EC_MKBP_EVENT_SWITCH:
> /*
> - * If EC is not the wake source, discard key state
> + * If bs is not wake enabled, discard key state
> * changes during suspend. Switches will be re-checked in
> * cros_ec_keyb_resume() to be sure nothing is lost.
> */
> - if (queued_during_suspend)
> + if (queued_during_suspend && ckdev->bs_idev
> + && !device_may_wakeup(&ckdev->bs_idev->dev))
> return NOTIFY_OK;
>
> + else if (queued_during_suspend && ckdev->bs_idev)
> + pm_wakeup_event(&ckdev->bs_idev->dev, 0);
> +
> if (ckdev->ec->event_data.event_type == EC_MKBP_EVENT_BUTTON) {
> val = get_unaligned_le32(
> &ckdev->ec->event_data.data.buttons);
> @@ -522,6 +530,7 @@ static int cros_ec_keyb_register_bs(struct cros_ec_keyb *ckdev)
> return ret;
> }
>
> + device_init_wakeup(&idev->dev, true);
> return 0;
> }
>
> @@ -598,6 +607,7 @@ static int cros_ec_keyb_register_matrix(struct cros_ec_keyb *ckdev)
> return err;
> }
>
> + device_init_wakeup(&idev->dev, true);
> return 0;
> }
>
> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> index d61024141e2b6..df9520365e54b 100644
> --- a/drivers/mfd/cros_ec.c
> +++ b/drivers/mfd/cros_ec.c
> @@ -64,12 +64,14 @@ static irqreturn_t ec_irq_thread(int irq, void *data)
> * cros_ec_get_next_event() returned an error (default value for
> * wake_event is true)
> */
> - if (wake_event && device_may_wakeup(ec_dev->dev))
> + if (device_may_wakeup(ec_dev->dev) && wake_event
> + && ec_dev->wake_enabled)
> pm_wakeup_event(ec_dev->dev, 0);
>
> if (ret > 0)
> blocking_notifier_call_chain(&ec_dev->event_notifier,
> - 0, ec_dev);
> + wake_event && ec_dev->wake_enabled,
> + ec_dev);
> return IRQ_HANDLED;
> }
>
> @@ -229,7 +231,7 @@ int cros_ec_suspend(struct cros_ec_device *ec_dev)
> }
> EXPORT_SYMBOL(cros_ec_suspend);
>
> -static void cros_ec_drain_events(struct cros_ec_device *ec_dev)
> +static void cros_ec_report_events_during_suspend(struct cros_ec_device *ec_dev)
> {
> while (cros_ec_get_next_event(ec_dev, NULL) > 0)
> blocking_notifier_call_chain(&ec_dev->event_notifier,
> @@ -253,21 +255,16 @@ int cros_ec_resume(struct cros_ec_device *ec_dev)
> dev_dbg(ec_dev->dev, "Error %d sending resume event to ec",
> ret);
>
> - /*
> - * In some cases, we need to distinguish between events that occur
> - * during suspend if the EC is not a wake source. For example,
> - * keypresses during suspend should be discarded if it does not wake
> - * the system.
> - *
> - * If the EC is not a wake source, drain the event queue and mark them
> - * as "queued during suspend".
> - */
> if (ec_dev->wake_enabled) {
> disable_irq_wake(ec_dev->irq);
> ec_dev->wake_enabled = 0;
> - } else {
> - cros_ec_drain_events(ec_dev);
> }
> + /*
> + * Let the mfd devices know about events that occur during
> + * suspend. This way the clients know what to do with them.
> + */
> + cros_ec_report_events_during_suspend(ec_dev);
> +
>
> return 0;
> }
> --
> 2.17.0.441.gb46fe60e1d-goog
>
--
Dmitry
Mark cros_ec_keyb has wake enabled by default. If we see a MKBP event
related to keyboard, call pm_wakeup_event() to make sure wakeup
triggers are accounted to keyb during suspend resume path.
Signed-off-by: Ravi Chandra Sadineni <[email protected]>
---
V2: Marked the ckdev as wake enabled instead of input devices.
drivers/input/keyboard/cros_ec_keyb.c | 21 +++++++++++++++++----
drivers/mfd/cros_ec.c | 19 +++++++------------
2 files changed, 24 insertions(+), 16 deletions(-)
diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
index 79eb29550c348..a7c96f0317123 100644
--- a/drivers/input/keyboard/cros_ec_keyb.c
+++ b/drivers/input/keyboard/cros_ec_keyb.c
@@ -245,12 +245,17 @@ static int cros_ec_keyb_work(struct notifier_block *nb,
switch (ckdev->ec->event_data.event_type) {
case EC_MKBP_EVENT_KEY_MATRIX:
/*
- * If EC is not the wake source, discard key state changes
+ * If Keyb is not wake enabled, discard key state changes
* during suspend.
*/
- if (queued_during_suspend)
+ if (queued_during_suspend
+ && !device_may_wakeup(ckdev->dev))
return NOTIFY_OK;
+ if (device_may_wakeup(ckdev->dev))
+ pm_wakeup_event(ckdev->dev, 0);
+
+
if (ckdev->ec->event_size != ckdev->cols) {
dev_err(ckdev->dev,
"Discarded incomplete key matrix event.\n");
@@ -265,18 +270,25 @@ static int cros_ec_keyb_work(struct notifier_block *nb,
val = get_unaligned_le32(&ckdev->ec->event_data.data.sysrq);
dev_dbg(ckdev->dev, "sysrq code from EC: %#x\n", val);
handle_sysrq(val);
+
+ if (device_may_wakeup(ckdev->dev))
+ pm_wakeup_event(ckdev->dev, 0);
break;
case EC_MKBP_EVENT_BUTTON:
case EC_MKBP_EVENT_SWITCH:
/*
- * If EC is not the wake source, discard key state
+ * If keyb is not wake enabled, discard key state
* changes during suspend. Switches will be re-checked in
* cros_ec_keyb_resume() to be sure nothing is lost.
*/
- if (queued_during_suspend)
+ if (queued_during_suspend
+ && !device_may_wakeup(ckdev->dev))
return NOTIFY_OK;
+ if (device_may_wakeup(ckdev->dev))
+ pm_wakeup_event(ckdev->dev, 0);
+
if (ckdev->ec->event_data.event_type == EC_MKBP_EVENT_BUTTON) {
val = get_unaligned_le32(
&ckdev->ec->event_data.data.buttons);
@@ -639,6 +651,7 @@ static int cros_ec_keyb_probe(struct platform_device *pdev)
return err;
}
+ device_init_wakeup(ckdev->dev, true);
return 0;
}
diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
index d61024141e2b6..36156a41499c9 100644
--- a/drivers/mfd/cros_ec.c
+++ b/drivers/mfd/cros_ec.c
@@ -229,7 +229,7 @@ int cros_ec_suspend(struct cros_ec_device *ec_dev)
}
EXPORT_SYMBOL(cros_ec_suspend);
-static void cros_ec_drain_events(struct cros_ec_device *ec_dev)
+static void cros_ec_report_events_during_suspend(struct cros_ec_device *ec_dev)
{
while (cros_ec_get_next_event(ec_dev, NULL) > 0)
blocking_notifier_call_chain(&ec_dev->event_notifier,
@@ -253,21 +253,16 @@ int cros_ec_resume(struct cros_ec_device *ec_dev)
dev_dbg(ec_dev->dev, "Error %d sending resume event to ec",
ret);
- /*
- * In some cases, we need to distinguish between events that occur
- * during suspend if the EC is not a wake source. For example,
- * keypresses during suspend should be discarded if it does not wake
- * the system.
- *
- * If the EC is not a wake source, drain the event queue and mark them
- * as "queued during suspend".
- */
if (ec_dev->wake_enabled) {
disable_irq_wake(ec_dev->irq);
ec_dev->wake_enabled = 0;
- } else {
- cros_ec_drain_events(ec_dev);
}
+ /*
+ * Let the mfd devices know about events that occur during
+ * suspend. This way the clients know what to do with them.
+ */
+ cros_ec_report_events_during_suspend(ec_dev);
+
return 0;
}
--
2.17.0.921.gf22659ad46-goog
On Fri, 25 May 2018, Ravi Chandra Sadineni wrote:
> Mark cros_ec_keyb has wake enabled by default. If we see a MKBP event
> related to keyboard, call pm_wakeup_event() to make sure wakeup
> triggers are accounted to keyb during suspend resume path.
>
> Signed-off-by: Ravi Chandra Sadineni <[email protected]>
> ---
> V2: Marked the ckdev as wake enabled instead of input devices.
>
> drivers/input/keyboard/cros_ec_keyb.c | 21 +++++++++++++++++----
> drivers/mfd/cros_ec.c | 19 +++++++------------
For my own reference:
Acked-for-MFD-by: Lee Jones <[email protected]>
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Hi,
On Fri, May 25, 2018 at 06:14:40PM -0700, Ravi Chandra Sadineni wrote:
> Mark cros_ec_keyb has wake enabled by default. If we see a MKBP event
> related to keyboard, call pm_wakeup_event() to make sure wakeup
> triggers are accounted to keyb during suspend resume path.
>
> Signed-off-by: Ravi Chandra Sadineni <[email protected]>
> ---
> V2: Marked the ckdev as wake enabled instead of input devices.
I'm not sure I saw v1? But FYI, if you're not marking the input devices
as wake-enabled, then you only have one knob for disabling wakeup on the
buttons and switches (e.g., power button) vs. the keyboard. I know you
were previously concerned about this, but given that the EC itself
usually has full knowledge of these situations (e.g., it knows to
disable keyboard wakeup when a convertible is in a tablet orientation,
but leave power-button wakeup enabled), this may not be a problem.
Anyway, just wanted to highlight that part.
> drivers/input/keyboard/cros_ec_keyb.c | 21 +++++++++++++++++----
> drivers/mfd/cros_ec.c | 19 +++++++------------
> 2 files changed, 24 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
> index 79eb29550c348..a7c96f0317123 100644
> --- a/drivers/input/keyboard/cros_ec_keyb.c
> +++ b/drivers/input/keyboard/cros_ec_keyb.c
> @@ -245,12 +245,17 @@ static int cros_ec_keyb_work(struct notifier_block *nb,
> switch (ckdev->ec->event_data.event_type) {
> case EC_MKBP_EVENT_KEY_MATRIX:
> /*
> - * If EC is not the wake source, discard key state changes
> + * If Keyb is not wake enabled, discard key state changes
We can use the full word; text is cheap:
s/Keyb/keyboard/
> * during suspend.
> */
> - if (queued_during_suspend)
> + if (queued_during_suspend
> + && !device_may_wakeup(ckdev->dev))
> return NOTIFY_OK;
>
> + if (device_may_wakeup(ckdev->dev))
> + pm_wakeup_event(ckdev->dev, 0);
> +
> +
I don't think you need two blank lines here. One is enough.
> if (ckdev->ec->event_size != ckdev->cols) {
> dev_err(ckdev->dev,
> "Discarded incomplete key matrix event.\n");
> @@ -265,18 +270,25 @@ static int cros_ec_keyb_work(struct notifier_block *nb,
> val = get_unaligned_le32(&ckdev->ec->event_data.data.sysrq);
> dev_dbg(ckdev->dev, "sysrq code from EC: %#x\n", val);
> handle_sysrq(val);
> +
> + if (device_may_wakeup(ckdev->dev))
> + pm_wakeup_event(ckdev->dev, 0);
> break;
>
> case EC_MKBP_EVENT_BUTTON:
> case EC_MKBP_EVENT_SWITCH:
> /*
> - * If EC is not the wake source, discard key state
> + * If keyb is not wake enabled, discard key state
s/keyb/keyboard/
Or, since this is talking about buttons and switches (which don't
technically require the "keyboard" part of this driver), you might just
leave that off ("If not wake enabled...").
Otherwise, I believe this looks good, though I may have overlooked
something:
Reviewed-by: Brian Norris <[email protected]>
And given Lee acked this, and it's mostly a keyboard change, it should
probably go through Dmitry? And I'd expect he'd be a better reviewer
than me anyway.
Brian
> * changes during suspend. Switches will be re-checked in
> * cros_ec_keyb_resume() to be sure nothing is lost.
> */
> - if (queued_during_suspend)
> + if (queued_during_suspend
> + && !device_may_wakeup(ckdev->dev))
> return NOTIFY_OK;
>
> + if (device_may_wakeup(ckdev->dev))
> + pm_wakeup_event(ckdev->dev, 0);
> +
> if (ckdev->ec->event_data.event_type == EC_MKBP_EVENT_BUTTON) {
> val = get_unaligned_le32(
> &ckdev->ec->event_data.data.buttons);
> @@ -639,6 +651,7 @@ static int cros_ec_keyb_probe(struct platform_device *pdev)
> return err;
> }
>
> + device_init_wakeup(ckdev->dev, true);
> return 0;
> }
>
> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> index d61024141e2b6..36156a41499c9 100644
> --- a/drivers/mfd/cros_ec.c
> +++ b/drivers/mfd/cros_ec.c
> @@ -229,7 +229,7 @@ int cros_ec_suspend(struct cros_ec_device *ec_dev)
> }
> EXPORT_SYMBOL(cros_ec_suspend);
>
> -static void cros_ec_drain_events(struct cros_ec_device *ec_dev)
> +static void cros_ec_report_events_during_suspend(struct cros_ec_device *ec_dev)
> {
> while (cros_ec_get_next_event(ec_dev, NULL) > 0)
> blocking_notifier_call_chain(&ec_dev->event_notifier,
> @@ -253,21 +253,16 @@ int cros_ec_resume(struct cros_ec_device *ec_dev)
> dev_dbg(ec_dev->dev, "Error %d sending resume event to ec",
> ret);
>
> - /*
> - * In some cases, we need to distinguish between events that occur
> - * during suspend if the EC is not a wake source. For example,
> - * keypresses during suspend should be discarded if it does not wake
> - * the system.
> - *
> - * If the EC is not a wake source, drain the event queue and mark them
> - * as "queued during suspend".
> - */
> if (ec_dev->wake_enabled) {
> disable_irq_wake(ec_dev->irq);
> ec_dev->wake_enabled = 0;
> - } else {
> - cros_ec_drain_events(ec_dev);
> }
> + /*
> + * Let the mfd devices know about events that occur during
> + * suspend. This way the clients know what to do with them.
> + */
> + cros_ec_report_events_during_suspend(ec_dev);
> +
>
> return 0;
> }
> --
> 2.17.0.921.gf22659ad46-goog
>