[PATCH v2 0/1] Set cros_ec_keyb as a wakeup source

[PATCH v2 0/1] Set cros_ec_keyb as a wakeup source

[Jeffy Chen]

This patches set cros_ec_keyb as a wakeup source, so the chromeos's powerd
can control the keyboard's wakeup ability along with other ec event sources.

Changes in v2:
Remove unneeded dts changes.

Jeffy Chen (1):
  input: cros_ec_keyb: Report wakeup events

 drivers/input/keyboard/cros_ec_keyb.c | 9 +++++++++
 1 file changed, 9 insertions(+)

-- 
2.1.4

[PATCH v2] input: cros_ec_keyb: Report wakeup events

[Jeffy Chen]

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);
+	if (err) {
+		dev_err(dev, "cannot init wakeup: %d\n", err);
+		return err;
+	}
+
 	return 0;
 }
 
-- 
2.1.4

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

[Dmitry Torokhov]

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.

> +	if (err) {
> +		dev_err(dev, "cannot init wakeup: %d\n", err);
> +		return err;
> +	}
> +
>  	return 0;
>  }
>  
> -- 
> 2.1.4
> 
> 

Thanks.

-- 
Dmitry

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

[Brian Norris]

+ 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

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

[Dmitry Torokhov]

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

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

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

[jeffy]

Hi dmitry,

On 04/04/2017 06:41 AM, Dmitry Torokhov wrote:
> 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.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...
>
> 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.
right, we do have a issue about gyro sensor break 
suspend(https://partnerissuetracker.corp.google.com/issues/36705709)

it would be better if we move wakeup codes to sub drivers. and if you do 
this, it would also solve the original issue of this patchset ;)
>
> Thanks.
>

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

[jeffy]

Hi guys,

On 04/05/2017 09:20 AM, jeffy wrote:
> Hi dmitry,
>
> On 04/04/2017 06:41 AM, Dmitry Torokhov wrote:
>> 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.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...
>>
>> 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.

right, so i'll add this:
+       device_init_wakeup(dev,
+                       device_property_read_bool(dev, "wakeup-source"));

>>
>>>
>>> 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?

the double-reporting wakeup could be harmless, but i saw we added a wake 
mask in our 4.4 kernel(for non-wake events):
         if (device_may_wakeup(ec_dev->dev) && wake_event)
                 pm_wakeup_event(ec_dev->dev, 0);

maybe we can do something similar to filter out wakeup events already 
handled by sub devices?

>>
>> 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.
hmmm, it looks like the i2c-based ec also do this, but through i2c-core:

         if (of_get_property(node, "wakeup-source", NULL))
                 info.flags |= I2C_CLIENT_WAKE;
...
         if (client->flags & I2C_CLIENT_WAKE) {
                 device_init_wakeup(&client->dev, true);

exynos5250-spring.dts:
         cros_ec: embedded-controller@1e {
                 compatible = "google,cros-ec-i2c";
...
                 wakeup-source;

and the binding document said we need to add wakeup-source for cros ec spi:
Documentation/devicetree/bindings/mfd/cros-ec.txt

spi@131b0000 {
         ec@0 {
                 compatible = "google,cros-ec-spi";
                 reg = <0x0>;
                 interrupts = <14 0>;
                 interrupt-parent = <&wakeup_eint>;
                 wakeup-source;


so do we need to add wakeup-source property support in cros_ec_spi? or 
maybe even in spi core(like i2c core)?
>>
>> 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.
we have this https://chromium-review.googlesource.com/c/372399/ in cros 
4.4 kernel, but somehow not upstream.

but it would still be good to move wakeup to sub devices.
> right, we do have a issue about gyro sensor break
> suspend(https://partnerissuetracker.corp.google.com/issues/36705709)
>
> it would be better if we move wakeup codes to sub drivers. and if you do
> this, it would also solve the original issue of this patchset ;)
>>
>> Thanks.
>>
>

so i'll try to add wakeup-source property for spi core and 
cros-ec-keyboard as a first step