Re: [PATCH v2] input: cros_ec_keyb: Report wakeup events

From: jeffy
Date: Wed Jun 21 2017 - 04:40:46 EST

Hi guys,

On 04/05/2017 09:20 AM, jeffy wrote:
Hi dmitry,

On 04/04/2017 06:41 AM, Dmitry Torokhov wrote:
On Mon, Apr 03, 2017 at 01:53:53PM -0700, Brian Norris wrote:
+ others

On Mon, Apr 03, 2017 at 11:43:36AM -0700, Dmitry Torokhov wrote:
On Sun, Apr 02, 2017 at 08:07:39AM +0800, Jeffy Chen wrote:
Report wakeup events when process events.

Signed-off-by: Jeffy Chen <jeffy.chen@xxxxxxxxxxxxxx>
---

Changes in v2:
Remove unneeded dts changes.

drivers/input/keyboard/cros_ec_keyb.c | 9 +++++++++
1 file changed, 9 insertions(+)

diff --git a/drivers/input/keyboard/cros_ec_keyb.c
b/drivers/input/keyboard/cros_ec_keyb.c
index 6a250d6..a93d55f 100644
--- a/drivers/input/keyboard/cros_ec_keyb.c
+++ b/drivers/input/keyboard/cros_ec_keyb.c
@@ -286,6 +286,9 @@ static int cros_ec_keyb_work(struct
notifier_block *nb,
return NOTIFY_DONE;
}

+ if (device_may_wakeup(ckdev->dev))
+ pm_wakeup_event(ckdev->dev, 0);
+
return NOTIFY_OK;
}

@@ -632,6 +635,12 @@ static int cros_ec_keyb_probe(struct
platform_device *pdev)
return err;
}

+ err = device_init_wakeup(dev, 1);

I would prefer if we did not mark cros_ec devices as wakeup sources
unconditionally. Your original patch series was better (except it
failed
to parse the "wakeup-source" property that you introduced.

I'm curious, why is this keyboard device different than any other
keyboard
device? I see several other drivers in drivers/input/keyboard/ that
do an
unconditional 'device_init_wakeup(..., 1)'. Keyboards tend to be wakeup
devices...

If we did something before it does not mean we should continue doing
this forever. I think providing an option to mark device as wakeup
capable should be left to the platform.

right, so i'll add this:
+ device_init_wakeup(dev,
+ device_property_read_bool(dev, "wakeup-source"));



Also, what's the idea behind sub-devices vs. the main cros-ec device
reporting
wakeups? Right now, we have this in drivers/mfd/cros_ec.c:

static irqreturn_t ec_irq_thread(int irq, void *data)
{
struct cros_ec_device *ec_dev = data;
int ret;

if (device_may_wakeup(ec_dev->dev))
pm_wakeup_event(ec_dev->dev, 0);

ret = cros_ec_get_next_event(ec_dev);
if (ret > 0)
blocking_notifier_call_chain(&ec_dev->event_notifier,
0, ec_dev);
return IRQ_HANDLED;
}

But now, we're going to start double-reporting wakeups? Is that
expected?

the double-reporting wakeup could be harmless, but i saw we added a wake mask in our 4.4 kernel(for non-wake events):
if (device_may_wakeup(ec_dev->dev) && wake_event)
pm_wakeup_event(ec_dev->dev, 0);

maybe we can do something similar to filter out wakeup events already handled by sub devices?


No, and not always (below).


I think we have a similar overlap with the RTC driver (which is being
upstreamed now?):

https://lkml.org/lkml/2017/2/14/658
[PATCH v3 3/4] rtc: cros-ec: add cros-ec-rtc driver.

except that also goes through the trouble of enabling/disabling
wakeup for the
EC IRQ. It seems to me (though I haven't dug in thoroughly) like the
main MFD shouldn't really be doing the wakeup reporting at all, and we
should depend on the sub-devices to do this. (i.e., the current patchset
is a step in the right direction, but it's not 100%.)

Anyway, I could be wrong about the above, but I think we should make
sure there's a consistent answer across the drivers tree.

Hm, it appears we have quite a mess. SPI-based EC declares entire EC as
wakeup source (unconditionally I must add; we do mention "wakeup-source"
in binding document at least). I2C-based EC does not call
device_init_wakeup() at all, presumably that is what caused the calls to
be added into sub-drivers.
hmmm, it looks like the i2c-based ec also do this, but through i2c-core:

if (of_get_property(node, "wakeup-source", NULL))
info.flags |= I2C_CLIENT_WAKE;
...
if (client->flags & I2C_CLIENT_WAKE) {
device_init_wakeup(&client->dev, true);

exynos5250-spring.dts:
cros_ec: embedded-controller@1e {
compatible = "google,cros-ec-i2c";
...
wakeup-source;

and the binding document said we need to add wakeup-source for cros ec spi:
Documentation/devicetree/bindings/mfd/cros-ec.txt

spi@131b0000 {
ec@0 {
compatible = "google,cros-ec-spi";
reg = <0x0>;
interrupts = <14 0>;
interrupt-parent = <&wakeup_eint>;
wakeup-source;


so do we need to add wakeup-source property support in cros_ec_spi? or maybe even in spi core(like i2c core)?

We need to resolve this one way or another. You probably do not want to
wake up any time you move your device (accelerometer or other sensors),
so I would try to move this property into individual devices, and try to
come up with a reasonable binding.
we have this https://chromium-review.googlesource.com/c/372399/ in cros 4.4 kernel, but somehow not upstream.

but it would still be good to move wakeup to sub devices.
right, we do have a issue about gyro sensor break
suspend(https://partnerissuetracker.corp.google.com/issues/36705709)

it would be better if we move wakeup codes to sub drivers. and if you do
this, it would also solve the original issue of this patchset ;)

Thanks.



so i'll try to add wakeup-source property for spi core and cros-ec-keyboard as a first step