Re: [PATCH v2] input: cros_ec_keyb: Report wakeup events

From: Brian Norris
Date: Mon Apr 03 2017 - 16:54:06 EST


+ 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.chen@xxxxxxxxxxxxxx>
> > ---
> >
> > 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