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.c...@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.
>
> 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
