From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754868AbdDEBVJ (ORCPT ); Tue, 4 Apr 2017 21:21:09 -0400 Received: from regular1.263xmail.com ([211.150.99.133]:40494 "EHLO regular1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751396AbdDEBVH (ORCPT ); Tue, 4 Apr 2017 21:21:07 -0400 X-263anti-spam: KSV:0;BIG:0; X-MAIL-GRAY: 0 X-MAIL-DELIVERY: 1 X-KSVirus-check: 0 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: <31fee907e211b19200b7d7e104f5da21> X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Message-ID: <58E4464A.6090806@rock-chips.com> Date: Wed, 05 Apr 2017 09:20:10 +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 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> In-Reply-To: <20170403224140.GB5613@dtor-ws> 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 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. > >> >> 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? > > 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. > > 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. 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. >