From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751808AbdDCUx5 (ORCPT ); Mon, 3 Apr 2017 16:53:57 -0400 Received: from mail-pg0-f42.google.com ([74.125.83.42]:33527 "EHLO mail-pg0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751210AbdDCUx4 (ORCPT ); Mon, 3 Apr 2017 16:53:56 -0400 Date: Mon, 3 Apr 2017 13:53:53 -0700 From: Brian Norris To: Dmitry Torokhov 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: <20170403205352.GA138246@google.com> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170403184336.GD34530@dtor-ws> 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 + 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... 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? 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. Regards, Brian > > + if (err) { > > + dev_err(dev, "cannot init wakeup: %d\n", err); > > + return err; > > + } > > + > > return 0; > > } > > > > -- > > 2.1.4 > > > > > > Thanks. > > -- > Dmitry