+ 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...
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
