[PATCH RFC] gpio: pca953x: Configure wake-up path when wake-up is enabled

[PATCH RFC] gpio: pca953x: Configure wake-up path when wake-up is enabled

[Geert Uytterhoeven]

If a device is part of the wake-up path, it should indicate this by
setting its power.wakeup_path field.  This allows the genpd core code to
keep the device enabled during system suspend when needed.

As regulators powering devices are not handled by genpd, the driver
handles these itself, and thus must skip regulator control when the
device is part of the wake-up path.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Note that I don't really need this on the Renesas Ebisu-4D board, as
there is no regulator or PM Domain controlling power to the GPIO
expander on that board.  I did want to have all wake-up path processing
implemented in the driver for completeness, and did test its behavior
with gpio-keys configured as a wake-up source.

However, while this approach is known to work fine on other boards, with
other GPIO and interrupt controllers (gpio-rcar, irq-renesas-irqc,
irq-renesas-intc-irqpin), it wouldn't work on Ebisu-4D, due to different
device suspend ordering.

The proper ordering is:
  1. When gpio-keys is suspended, its suspend handler calls
     enable_irq_wake(), invoking pca953x_irq_set_wake(), and causing
     pca953x_chip.wakeup_path to be incremented,
  2. When gpio-pca953x is suspended, it checks pca953x_chip.wakeup_path,
     and marks the device to be part of the wake-up path.

However, gpio-keys is suspended _after_ gpio-pca953x, breaking the
scheme :-(

So depending on topology, this may work, or not...
---
 drivers/gpio/gpio-pca953x.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index 88c94d155e218535..349d0ccb5285a6c4 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -153,6 +153,7 @@ struct pca953x_chip {
 	u8 irq_trig_fall[MAX_BANK];
 	struct irq_chip irq_chip;
 #endif
+	atomic_t wakeup_path;
 
 	struct i2c_client *client;
 	struct gpio_chip gpio_chip;
@@ -581,6 +582,11 @@ static int pca953x_irq_set_wake(struct irq_data *d, unsigned int on)
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct pca953x_chip *chip = gpiochip_get_data(gc);
 
+	if (on)
+		atomic_inc(&chip->wakeup_path);
+	else
+		atomic_dec(&chip->wakeup_path);
+
 	return irq_set_irq_wake(chip->client->irq, on);
 }
 
@@ -1100,7 +1106,10 @@ static int pca953x_suspend(struct device *dev)
 
 	regcache_cache_only(chip->regmap, true);
 
-	regulator_disable(chip->regulator);
+	if (atomic_read(&chip->wakeup_path))
+		device_set_wakeup_path(dev);
+	else
+		regulator_disable(chip->regulator);
 
 	return 0;
 }
@@ -1110,10 +1119,12 @@ static int pca953x_resume(struct device *dev)
 	struct pca953x_chip *chip = dev_get_drvdata(dev);
 	int ret;
 
-	ret = regulator_enable(chip->regulator);
-	if (ret != 0) {
-		dev_err(dev, "Failed to enable regulator: %d\n", ret);
-		return 0;
+	if (!atomic_read(&chip->wakeup_path)) {
+		ret = regulator_enable(chip->regulator);
+		if (ret != 0) {
+			dev_err(dev, "Failed to enable regulator: %d\n", ret);
+			return 0;
+		}
 	}
 
 	regcache_cache_only(chip->regmap, false);
-- 
2.17.1

Re: [PATCH RFC] gpio: pca953x: Configure wake-up path when wake-up is enabled

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Wed, Mar 20, 2019 at 11:39:27AM +0100, Geert Uytterhoeven wrote:
> If a device is part of the wake-up path, it should indicate this by
> setting its power.wakeup_path field.  This allows the genpd core code to
> keep the device enabled during system suspend when needed.
> 
> As regulators powering devices are not handled by genpd, the driver
> handles these itself, and thus must skip regulator control when the
> device is part of the wake-up path.

It would be nice for this to be handled automatically...

> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Note that I don't really need this on the Renesas Ebisu-4D board, as
> there is no regulator or PM Domain controlling power to the GPIO
> expander on that board.  I did want to have all wake-up path processing
> implemented in the driver for completeness, and did test its behavior
> with gpio-keys configured as a wake-up source.
> 
> However, while this approach is known to work fine on other boards, with
> other GPIO and interrupt controllers (gpio-rcar, irq-renesas-irqc,
> irq-renesas-intc-irqpin), it wouldn't work on Ebisu-4D, due to different
> device suspend ordering.
> 
> The proper ordering is:
>   1. When gpio-keys is suspended, its suspend handler calls
>      enable_irq_wake(), invoking pca953x_irq_set_wake(), and causing
>      pca953x_chip.wakeup_path to be incremented,
>   2. When gpio-pca953x is suspended, it checks pca953x_chip.wakeup_path,
>      and marks the device to be part of the wake-up path.
> 
> However, gpio-keys is suspended _after_ gpio-pca953x, breaking the
> scheme :-(
> 
> So depending on topology, this may work, or not...

Could device links help there ?

> ---
>  drivers/gpio/gpio-pca953x.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index 88c94d155e218535..349d0ccb5285a6c4 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -153,6 +153,7 @@ struct pca953x_chip {
>  	u8 irq_trig_fall[MAX_BANK];
>  	struct irq_chip irq_chip;
>  #endif
> +	atomic_t wakeup_path;
>  
>  	struct i2c_client *client;
>  	struct gpio_chip gpio_chip;
> @@ -581,6 +582,11 @@ static int pca953x_irq_set_wake(struct irq_data *d, unsigned int on)
>  	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>  	struct pca953x_chip *chip = gpiochip_get_data(gc);
>  
> +	if (on)
> +		atomic_inc(&chip->wakeup_path);
> +	else
> +		atomic_dec(&chip->wakeup_path);
> +
>  	return irq_set_irq_wake(chip->client->irq, on);
>  }
>  
> @@ -1100,7 +1106,10 @@ static int pca953x_suspend(struct device *dev)
>  
>  	regcache_cache_only(chip->regmap, true);
>  
> -	regulator_disable(chip->regulator);
> +	if (atomic_read(&chip->wakeup_path))
> +		device_set_wakeup_path(dev);
> +	else
> +		regulator_disable(chip->regulator);
>  
>  	return 0;
>  }
> @@ -1110,10 +1119,12 @@ static int pca953x_resume(struct device *dev)
>  	struct pca953x_chip *chip = dev_get_drvdata(dev);
>  	int ret;
>  
> -	ret = regulator_enable(chip->regulator);
> -	if (ret != 0) {
> -		dev_err(dev, "Failed to enable regulator: %d\n", ret);
> -		return 0;
> +	if (!atomic_read(&chip->wakeup_path)) {
> +		ret = regulator_enable(chip->regulator);
> +		if (ret != 0) {
> +			dev_err(dev, "Failed to enable regulator: %d\n", ret);
> +			return 0;
> +		}
>  	}
>  
>  	regcache_cache_only(chip->regmap, false);

-- 
Regards,

Laurent Pinchart

Re: [PATCH RFC] gpio: pca953x: Configure wake-up path when wake-up is enabled

[Linus Walleij]

On Wed, Mar 20, 2019 at 5:39 PM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:

> If a device is part of the wake-up path, it should indicate this by
> setting its power.wakeup_path field.  This allows the genpd core code to
> keep the device enabled during system suspend when needed.
>
> As regulators powering devices are not handled by genpd, the driver
> handles these itself, and thus must skip regulator control when the
> device is part of the wake-up path.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Note that I don't really need this on the Renesas Ebisu-4D board, as
> there is no regulator or PM Domain controlling power to the GPIO
> expander on that board.  I did want to have all wake-up path processing
> implemented in the driver for completeness, and did test its behavior
> with gpio-keys configured as a wake-up source.
>
> However, while this approach is known to work fine on other boards, with
> other GPIO and interrupt controllers (gpio-rcar, irq-renesas-irqc,
> irq-renesas-intc-irqpin), it wouldn't work on Ebisu-4D, due to different
> device suspend ordering.
>
> The proper ordering is:
>   1. When gpio-keys is suspended, its suspend handler calls
>      enable_irq_wake(), invoking pca953x_irq_set_wake(), and causing
>      pca953x_chip.wakeup_path to be incremented,
>   2. When gpio-pca953x is suspended, it checks pca953x_chip.wakeup_path,
>      and marks the device to be part of the wake-up path.
>
> However, gpio-keys is suspended _after_ gpio-pca953x, breaking the
> scheme :-(
>
> So depending on topology, this may work, or not...

This looks like the right way to do it to me.

Ulf/Rafael: could either of you confirm that this is the right way
to handle it when we have an optional external regulator cutting
power to the chip providing the wakeup like this?

Yours,
Linus Walleij

Re: [PATCH RFC] gpio: pca953x: Configure wake-up path when wake-up is enabled

[Ulf Hansson]

On Wed, 20 Mar 2019 at 11:39, Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
>
> If a device is part of the wake-up path, it should indicate this by
> setting its power.wakeup_path field.  This allows the genpd core code to
> keep the device enabled during system suspend when needed.
>
> As regulators powering devices are not handled by genpd, the driver
> handles these itself, and thus must skip regulator control when the
> device is part of the wake-up path.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Note that I don't really need this on the Renesas Ebisu-4D board, as
> there is no regulator or PM Domain controlling power to the GPIO
> expander on that board.  I did want to have all wake-up path processing
> implemented in the driver for completeness, and did test its behavior
> with gpio-keys configured as a wake-up source.

All above makes perfect sense to me.

>
> However, while this approach is known to work fine on other boards, with
> other GPIO and interrupt controllers (gpio-rcar, irq-renesas-irqc,
> irq-renesas-intc-irqpin), it wouldn't work on Ebisu-4D, due to different
> device suspend ordering.
>
> The proper ordering is:
>   1. When gpio-keys is suspended, its suspend handler calls
>      enable_irq_wake(), invoking pca953x_irq_set_wake(), and causing
>      pca953x_chip.wakeup_path to be incremented,
>   2. When gpio-pca953x is suspended, it checks pca953x_chip.wakeup_path,
>      and marks the device to be part of the wake-up path.

Right.

>
> However, gpio-keys is suspended _after_ gpio-pca953x, breaking the
> scheme :-(

Would it make sense to fixup the ordering issue via creating a
parent/child relationship or setting up a device link?

>
> So depending on topology, this may work, or not...
> ---
>  drivers/gpio/gpio-pca953x.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index 88c94d155e218535..349d0ccb5285a6c4 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -153,6 +153,7 @@ struct pca953x_chip {
>         u8 irq_trig_fall[MAX_BANK];
>         struct irq_chip irq_chip;
>  #endif
> +       atomic_t wakeup_path;
>
>         struct i2c_client *client;
>         struct gpio_chip gpio_chip;
> @@ -581,6 +582,11 @@ static int pca953x_irq_set_wake(struct irq_data *d, unsigned int on)
>         struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>         struct pca953x_chip *chip = gpiochip_get_data(gc);
>
> +       if (on)
> +               atomic_inc(&chip->wakeup_path);
> +       else
> +               atomic_dec(&chip->wakeup_path);
> +
>         return irq_set_irq_wake(chip->client->irq, on);
>  }
>
> @@ -1100,7 +1106,10 @@ static int pca953x_suspend(struct device *dev)
>
>         regcache_cache_only(chip->regmap, true);
>
> -       regulator_disable(chip->regulator);
> +       if (atomic_read(&chip->wakeup_path))
> +               device_set_wakeup_path(dev);
> +       else
> +               regulator_disable(chip->regulator);
>
>         return 0;
>  }
> @@ -1110,10 +1119,12 @@ static int pca953x_resume(struct device *dev)
>         struct pca953x_chip *chip = dev_get_drvdata(dev);
>         int ret;
>
> -       ret = regulator_enable(chip->regulator);
> -       if (ret != 0) {
> -               dev_err(dev, "Failed to enable regulator: %d\n", ret);
> -               return 0;
> +       if (!atomic_read(&chip->wakeup_path)) {
> +               ret = regulator_enable(chip->regulator);
> +               if (ret != 0) {
> +                       dev_err(dev, "Failed to enable regulator: %d\n", ret);
> +                       return 0;
> +               }
>         }
>
>         regcache_cache_only(chip->regmap, false);
> --
> 2.17.1
>

Looks good to me!

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

Re: [PATCH RFC] gpio: pca953x: Configure wake-up path when wake-up is enabled

[Geert Uytterhoeven]

Hi Ulf,

On Thu, Apr 4, 2019 at 10:55 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On Wed, 20 Mar 2019 at 11:39, Geert Uytterhoeven
> <geert+renesas@glider.be> wrote:
> > If a device is part of the wake-up path, it should indicate this by
> > setting its power.wakeup_path field.  This allows the genpd core code to
> > keep the device enabled during system suspend when needed.
> >
> > As regulators powering devices are not handled by genpd, the driver
> > handles these itself, and thus must skip regulator control when the
> > device is part of the wake-up path.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > Note that I don't really need this on the Renesas Ebisu-4D board, as
> > there is no regulator or PM Domain controlling power to the GPIO
> > expander on that board.  I did want to have all wake-up path processing
> > implemented in the driver for completeness, and did test its behavior
> > with gpio-keys configured as a wake-up source.
>
> All above makes perfect sense to me.

> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Thanks!

> > However, while this approach is known to work fine on other boards, with
> > other GPIO and interrupt controllers (gpio-rcar, irq-renesas-irqc,
> > irq-renesas-intc-irqpin), it wouldn't work on Ebisu-4D, due to different
> > device suspend ordering.
> >
> > The proper ordering is:
> >   1. When gpio-keys is suspended, its suspend handler calls
> >      enable_irq_wake(), invoking pca953x_irq_set_wake(), and causing
> >      pca953x_chip.wakeup_path to be incremented,
> >   2. When gpio-pca953x is suspended, it checks pca953x_chip.wakeup_path,
> >      and marks the device to be part of the wake-up path.
>
> Right.
>
> >
> > However, gpio-keys is suspended _after_ gpio-pca953x, breaking the
> > scheme :-(
>
> Would it make sense to fixup the ordering issue via creating a
> parent/child relationship or setting up a device link?

Could that be due to gpio_keys not having rudimentary Runtime PM support?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH RFC] gpio: pca953x: Configure wake-up path when wake-up is enabled

[Ulf Hansson]

[...]

> > > However, while this approach is known to work fine on other boards, with
> > > other GPIO and interrupt controllers (gpio-rcar, irq-renesas-irqc,
> > > irq-renesas-intc-irqpin), it wouldn't work on Ebisu-4D, due to different
> > > device suspend ordering.
> > >
> > > The proper ordering is:
> > >   1. When gpio-keys is suspended, its suspend handler calls
> > >      enable_irq_wake(), invoking pca953x_irq_set_wake(), and causing
> > >      pca953x_chip.wakeup_path to be incremented,
> > >   2. When gpio-pca953x is suspended, it checks pca953x_chip.wakeup_path,
> > >      and marks the device to be part of the wake-up path.
> >
> > Right.
> >
> > >
> > > However, gpio-keys is suspended _after_ gpio-pca953x, breaking the
> > > scheme :-(
> >
> > Would it make sense to fixup the ordering issue via creating a
> > parent/child relationship or setting up a device link?
>
> Could that be due to gpio_keys not having rudimentary Runtime PM support?

You are saying that the parent/child relation ship is already there?

In such case, it shouldn't matter whether runtime PM is deployed or
not, the PM core should propagate the wakeup_path flag from children
to parents in __device_suspend() and in device_suspend_late(). If that
doesn't happen there is bug in the PM core.

Kind regards
Uffe

Re: [PATCH RFC] gpio: pca953x: Configure wake-up path when wake-up is enabled

[Linus Walleij]

On Wed, Mar 20, 2019 at 11:39 AM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:

> If a device is part of the wake-up path, it should indicate this by
> setting its power.wakeup_path field.  This allows the genpd core code to
> keep the device enabled during system suspend when needed.
>
> As regulators powering devices are not handled by genpd, the driver
> handles these itself, and thus must skip regulator control when the
> device is part of the wake-up path.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Patch applied with Ulf's Review tag.

If there are even better ways to do it I think we can fix that on top of
this, it is best to start with something that obviously works and
take it from there.

Yours,
Linus Walleij