From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751932AbdDCWlp (ORCPT ); Mon, 3 Apr 2017 18:41:45 -0400 Received: from mail-pg0-f66.google.com ([74.125.83.66]:35747 "EHLO mail-pg0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751756AbdDCWlo (ORCPT ); Mon, 3 Apr 2017 18:41:44 -0400 Date: Mon, 3 Apr 2017 15:41:40 -0700 From: Dmitry Torokhov To: Brian Norris Cc: Jeffy Chen , 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 Message-ID: <20170403224140.GB5613@dtor-ws> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170403205352.GA138246@google.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. Thanks. -- Dmitry