[PATCH] Input: hisi_powerkey: Enable system-wakeup for s2idle

[PATCH] Input: hisi_powerkey: Enable system-wakeup for s2idle

[Ulf Hansson]

To wake up the system from s2idle when pressing the power-button, let's
convert from using pm_wakeup_event() to pm_wakeup_dev_event(), as it allows
us to specify the "hard" in-parameter, which needs to be set for s2idle.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/input/misc/hisi_powerkey.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/misc/hisi_powerkey.c b/drivers/input/misc/hisi_powerkey.c
index d3c293a95d32..d315017324d9 100644
--- a/drivers/input/misc/hisi_powerkey.c
+++ b/drivers/input/misc/hisi_powerkey.c
@@ -30,7 +30,7 @@ static irqreturn_t hi65xx_power_press_isr(int irq, void *q)
 {
 	struct input_dev *input = q;
 
-	pm_wakeup_event(input->dev.parent, MAX_HELD_TIME);
+	pm_wakeup_dev_event(input->dev.parent, MAX_HELD_TIME, true);
 	input_report_key(input, KEY_POWER, 1);
 	input_sync(input);
 
-- 
2.43.0

Re: [PATCH] Input: hisi_powerkey: Enable system-wakeup for s2idle

[Dmitry Torokhov]

Hi Ulf,

On Thu, Mar 06, 2025 at 12:50:21PM +0100, Ulf Hansson wrote:
> To wake up the system from s2idle when pressing the power-button, let's
> convert from using pm_wakeup_event() to pm_wakeup_dev_event(), as it allows
> us to specify the "hard" in-parameter, which needs to be set for s2idle.

I was looking at pm_wakeup_event() and pm_wakeup_dev_event() and I am
afraid I do not understand the distinction. Why would we want to
abort suspend by only by some wakeup sources and not by others? And why
does a driver need to know whether a system uses s2idle or some other
implementation of low power state?

FWIW we have Chromebooks that use S0ix and Chromebooks that use S3 as
well as ARM Chromebooks and I do not think they use
pm_wakeup_dev_event() variant.

I'm cc-ing Rafael to give us some guidance.

> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/input/misc/hisi_powerkey.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/input/misc/hisi_powerkey.c b/drivers/input/misc/hisi_powerkey.c
> index d3c293a95d32..d315017324d9 100644
> --- a/drivers/input/misc/hisi_powerkey.c
> +++ b/drivers/input/misc/hisi_powerkey.c
> @@ -30,7 +30,7 @@ static irqreturn_t hi65xx_power_press_isr(int irq, void *q)
>  {
>  	struct input_dev *input = q;
>  
> -	pm_wakeup_event(input->dev.parent, MAX_HELD_TIME);
> +	pm_wakeup_dev_event(input->dev.parent, MAX_HELD_TIME, true);
>  	input_report_key(input, KEY_POWER, 1);
>  	input_sync(input);
>  
> -- 
> 2.43.0
> 

Thanks.

-- 
Dmitry

Re: [PATCH] Input: hisi_powerkey: Enable system-wakeup for s2idle

[Ulf Hansson]

On Tue, 11 Mar 2025 at 06:09, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>
> Hi Ulf,
>
> On Thu, Mar 06, 2025 at 12:50:21PM +0100, Ulf Hansson wrote:
> > To wake up the system from s2idle when pressing the power-button, let's
> > convert from using pm_wakeup_event() to pm_wakeup_dev_event(), as it allows
> > us to specify the "hard" in-parameter, which needs to be set for s2idle.
>
> I was looking at pm_wakeup_event() and pm_wakeup_dev_event() and I am
> afraid I do not understand the distinction. Why would we want to
> abort suspend by only by some wakeup sources and not by others? And why
> does a driver need to know whether a system uses s2idle or some other
> implementation of low power state?

Good question!

In regards to waking up. The CPU that wakes up from suspend-to-idle
may actually decide to just go back to idle, without doing a full
system resume - unless there is a wakeup that requires the system to
resume.

In suspend-to-ram a wakeup will always trigger a full system resume.

In most cases a driver doesn't really need to distinguish between
these cases, yet the wakeup APIs are designed to allow it. That's the
reason why pm_system_wakeup() needs to be called (controlled by "hard"
in-parameter to pm_wakeup_dev_event()).

>
> FWIW we have Chromebooks that use S0ix and Chromebooks that use S3 as
> well as ARM Chromebooks and I do not think they use
> pm_wakeup_dev_event() variant.
>
> I'm cc-ing Rafael to give us some guidance.

Good, let's see if there is something I failed to explain.

>
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >  drivers/input/misc/hisi_powerkey.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/input/misc/hisi_powerkey.c b/drivers/input/misc/hisi_powerkey.c
> > index d3c293a95d32..d315017324d9 100644
> > --- a/drivers/input/misc/hisi_powerkey.c
> > +++ b/drivers/input/misc/hisi_powerkey.c
> > @@ -30,7 +30,7 @@ static irqreturn_t hi65xx_power_press_isr(int irq, void *q)
> >  {
> >       struct input_dev *input = q;
> >
> > -     pm_wakeup_event(input->dev.parent, MAX_HELD_TIME);
> > +     pm_wakeup_dev_event(input->dev.parent, MAX_HELD_TIME, true);
> >       input_report_key(input, KEY_POWER, 1);
> >       input_sync(input);
> >
> > --
> > 2.43.0
> >
>
> Thanks.
>
> --
> Dmitry

Kind regards
Uffe

Re: [PATCH] Input: hisi_powerkey: Enable system-wakeup for s2idle

[Ulf Hansson]

On Wed, 12 Mar 2025 at 17:37, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Tue, 11 Mar 2025 at 06:09, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> >
> > Hi Ulf,
> >
> > On Thu, Mar 06, 2025 at 12:50:21PM +0100, Ulf Hansson wrote:
> > > To wake up the system from s2idle when pressing the power-button, let's
> > > convert from using pm_wakeup_event() to pm_wakeup_dev_event(), as it allows
> > > us to specify the "hard" in-parameter, which needs to be set for s2idle.
> >
> > I was looking at pm_wakeup_event() and pm_wakeup_dev_event() and I am
> > afraid I do not understand the distinction. Why would we want to
> > abort suspend by only by some wakeup sources and not by others? And why
> > does a driver need to know whether a system uses s2idle or some other
> > implementation of low power state?
>
> Good question!
>
> In regards to waking up. The CPU that wakes up from suspend-to-idle
> may actually decide to just go back to idle, without doing a full
> system resume - unless there is a wakeup that requires the system to
> resume.
>
> In suspend-to-ram a wakeup will always trigger a full system resume.
>
> In most cases a driver doesn't really need to distinguish between
> these cases, yet the wakeup APIs are designed to allow it. That's the
> reason why pm_system_wakeup() needs to be called (controlled by "hard"
> in-parameter to pm_wakeup_dev_event()).
>
> >
> > FWIW we have Chromebooks that use S0ix and Chromebooks that use S3 as
> > well as ARM Chromebooks and I do not think they use
> > pm_wakeup_dev_event() variant.
> >
> > I'm cc-ing Rafael to give us some guidance.
>
> Good, let's see if there is something I failed to explain.
>
> >
> > >
> > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > ---
> > >  drivers/input/misc/hisi_powerkey.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/input/misc/hisi_powerkey.c b/drivers/input/misc/hisi_powerkey.c
> > > index d3c293a95d32..d315017324d9 100644
> > > --- a/drivers/input/misc/hisi_powerkey.c
> > > +++ b/drivers/input/misc/hisi_powerkey.c
> > > @@ -30,7 +30,7 @@ static irqreturn_t hi65xx_power_press_isr(int irq, void *q)
> > >  {
> > >       struct input_dev *input = q;
> > >
> > > -     pm_wakeup_event(input->dev.parent, MAX_HELD_TIME);
> > > +     pm_wakeup_dev_event(input->dev.parent, MAX_HELD_TIME, true);
> > >       input_report_key(input, KEY_POWER, 1);
> > >       input_sync(input);
> > >
> > > --
> > > 2.43.0
> > >
> >
> > Thanks.
> >
> > --
> > Dmitry
>
> Kind regards
> Uffe

Dmitry, is there anything else I can do to make you queue this one?

S2idle is currently broken for Hikey and - it's rather annoying.

Kind regards
Uffe

Re: [PATCH] Input: hisi_powerkey: Enable system-wakeup for s2idle

[Dmitry Torokhov]

On Thu, Mar 06, 2025 at 12:50:21PM +0100, Ulf Hansson wrote:
> To wake up the system from s2idle when pressing the power-button, let's
> convert from using pm_wakeup_event() to pm_wakeup_dev_event(), as it allows
> us to specify the "hard" in-parameter, which needs to be set for s2idle.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Applied, thank you.

-- 
Dmitry