Messages in this thread | | | Date | Wed, 21 Jun 2017 16:40:12 +0800 | From | jeffy <> | Subject | Re: [PATCH v2] input: cros_ec_keyb: Report wakeup events |
| |
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@rock-chips.com> >>>>> --- >>>>> >>>>> 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
| |