linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] cros_ec_keyb: Increment the wakeup count to the specific mfd device.
@ 2018-05-23 18:29 Ravi Chandra Sadineni
  2018-05-24 23:42 ` Dmitry Torokhov
  0 siblings, 1 reply; 5+ messages in thread
From: Ravi Chandra Sadineni @ 2018-05-23 18:29 UTC (permalink / raw)
  To: dmitry.torokhov, lee.jones, ravisadineni, ravisadineni, dtor
  Cc: tbroch, linux-kernel, linux-input, rajatja, bleung

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 <ravisadineni@chromium.org>
---
 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

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] cros_ec_keyb: Increment the wakeup count to the specific mfd device.
  2018-05-23 18:29 [PATCH] cros_ec_keyb: Increment the wakeup count to the specific mfd device Ravi Chandra Sadineni
@ 2018-05-24 23:42 ` Dmitry Torokhov
  2018-05-26  1:14   ` [PATCH V2] cros_ec_keyb: Mark cros_ec_keyb driver as wake enabled device Ravi Chandra Sadineni
  0 siblings, 1 reply; 5+ messages in thread
From: Dmitry Torokhov @ 2018-05-24 23:42 UTC (permalink / raw)
  To: Ravi Chandra Sadineni
  Cc: lee.jones, ravisadineni, dtor, tbroch, linux-kernel, linux-input,
	rajatja, bleung

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 <ravisadineni@chromium.org>
> ---
>  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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH V2] cros_ec_keyb: Mark cros_ec_keyb driver as wake enabled device.
  2018-05-24 23:42 ` Dmitry Torokhov
@ 2018-05-26  1:14   ` Ravi Chandra Sadineni
  2018-06-04  6:18     ` Lee Jones
  2018-06-05  1:32     ` Brian Norris
  0 siblings, 2 replies; 5+ messages in thread
From: Ravi Chandra Sadineni @ 2018-05-26  1:14 UTC (permalink / raw)
  To: dmitry.torokhov, lee.jones, ravisadineni, ravisadineni, dtor,
	briannorris
  Cc: tbroch, linux-kernel, linux-input, rajatja, bleung

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 <ravisadineni@chromium.org>
---
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

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH V2] cros_ec_keyb: Mark cros_ec_keyb driver as wake enabled device.
  2018-05-26  1:14   ` [PATCH V2] cros_ec_keyb: Mark cros_ec_keyb driver as wake enabled device Ravi Chandra Sadineni
@ 2018-06-04  6:18     ` Lee Jones
  2018-06-05  1:32     ` Brian Norris
  1 sibling, 0 replies; 5+ messages in thread
From: Lee Jones @ 2018-06-04  6:18 UTC (permalink / raw)
  To: Ravi Chandra Sadineni
  Cc: dmitry.torokhov, ravisadineni, dtor, briannorris, tbroch,
	linux-kernel, linux-input, rajatja, bleung

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 <ravisadineni@chromium.org>
> ---
> 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 <lee.jones@linaro.org>

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH V2] cros_ec_keyb: Mark cros_ec_keyb driver as wake enabled device.
  2018-05-26  1:14   ` [PATCH V2] cros_ec_keyb: Mark cros_ec_keyb driver as wake enabled device Ravi Chandra Sadineni
  2018-06-04  6:18     ` Lee Jones
@ 2018-06-05  1:32     ` Brian Norris
  1 sibling, 0 replies; 5+ messages in thread
From: Brian Norris @ 2018-06-05  1:32 UTC (permalink / raw)
  To: Ravi Chandra Sadineni
  Cc: dmitry.torokhov, lee.jones, ravisadineni, dtor, tbroch,
	linux-kernel, linux-input, rajatja, bleung

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 <ravisadineni@chromium.org>
> ---
> 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 <briannorris@chromium.org>

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
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2018-06-05  1:32 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-23 18:29 [PATCH] cros_ec_keyb: Increment the wakeup count to the specific mfd device Ravi Chandra Sadineni
2018-05-24 23:42 ` Dmitry Torokhov
2018-05-26  1:14   ` [PATCH V2] cros_ec_keyb: Mark cros_ec_keyb driver as wake enabled device Ravi Chandra Sadineni
2018-06-04  6:18     ` Lee Jones
2018-06-05  1:32     ` Brian Norris

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).