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

[Brian Norris <briannorris@chromium.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 <jeffy.chen@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