From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752745AbdFUIkm (ORCPT ); Wed, 21 Jun 2017 04:40:42 -0400 Received: from regular1.263xmail.com ([211.150.99.130]:47529 "EHLO regular1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752449AbdFUIkj (ORCPT ); Wed, 21 Jun 2017 04:40:39 -0400 X-263anti-spam: BIG:0; X-MAIL-GRAY: 0 X-MAIL-DELIVERY: 1 X-ADDR-CHECKED4: 1 X-ABS-CHECKED: 1 X-SKE-CHECKED: 1 X-ANTISPAM-LEVEL: 2 X-RL-SENDER: jeffy.chen@rock-chips.com X-FST-TO: dmitry.torokhov@gmail.com X-SENDER-IP: 103.29.142.67 X-LOGIN-NAME: jeffy.chen@rock-chips.com X-UNIQUE-TAG: <66b74829980bf7da9513075d03f7860b> X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Message-ID: <594A30EC.1020509@rock-chips.com> Date: Wed, 21 Jun 2017 16:40:12 +0800 From: jeffy User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:19.0) Gecko/20130126 Thunderbird/19.0 MIME-Version: 1.0 To: Dmitry Torokhov , Brian Norris CC: linux-kernel@vger.kernel.org, dbasehore@google.com, dianders@chromium.org, Enric Balletbo i Serra , gwendal@chromium.org, Lee Jones , Mark Brown Subject: Re: [PATCH v2] input: cros_ec_keyb: Report wakeup events References: <1491091659-6546-1-git-send-email-jeffy.chen@rock-chips.com> <1491091659-6546-2-git-send-email-jeffy.chen@rock-chips.com> <20170403184336.GD34530@dtor-ws> <20170403205352.GA138246@google.com> <20170403224140.GB5613@dtor-ws> <58E4464A.6090806@rock-chips.com> In-Reply-To: <58E4464A.6090806@rock-chips.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >>>>> --- >>>>> >>>>> 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