gpio: pca953x: Add wake-up support

Message ID 20190212141715.3966-1-geert+renesas@glider.be
State New
Headers show
Series
  • gpio: pca953x: Add wake-up support
Related show

Commit Message

Geert Uytterhoeven Feb. 12, 2019, 2:17 p.m.
Implement the irq_set_wake() method in the (optional) irq_chip of the
GPIO expander, and propagate wake-up settings to the upstream interrupt
controller.  This allows GPIOs connected to a PCA953X GPIO expander to
serve as wake-up sources.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Tested with a PCA9654 and gpio-keys on an R-Car Ebisu-4D board.
---
 drivers/gpio/gpio-pca953x.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

Comments

Bartosz Golaszewski Feb. 12, 2019, 2:35 p.m. | #1
wt., 12 lut 2019 o 15:17 Geert Uytterhoeven <geert+renesas@glider.be>
napisał(a):
>
> Implement the irq_set_wake() method in the (optional) irq_chip of the
> GPIO expander, and propagate wake-up settings to the upstream interrupt
> controller.  This allows GPIOs connected to a PCA953X GPIO expander to
> serve as wake-up sources.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Tested with a PCA9654 and gpio-keys on an R-Car Ebisu-4D board.
> ---
>  drivers/gpio/gpio-pca953x.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index de52f63863dbe59b..8dfb8e326b9d12ca 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -151,6 +151,7 @@ struct pca953x_chip {
>         u8 irq_trig_raise[MAX_BANK];
>         u8 irq_trig_fall[MAX_BANK];
>         struct irq_chip irq_chip;
> +       unsigned int irq_parent;
>  #endif
>
>         struct i2c_client *client;
> @@ -513,6 +514,24 @@ static void pca953x_irq_unmask(struct irq_data *d)
>         chip->irq_mask[d->hwirq / BANK_SZ] |= 1 << (d->hwirq % BANK_SZ);
>  }
>
> +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);
> +       int error = 0;
> +
> +       if (chip->irq_parent) {
> +               error = irq_set_irq_wake(chip->irq_parent, on);
> +               if (error) {
> +                       dev_dbg(&chip->client->dev,
> +                               "irq %u doesn't support irq_set_wake\n",
> +                               chip->irq_parent);
> +                       chip->irq_parent = 0;
> +               }
> +       }
> +       return error;
> +}
> +
>  static void pca953x_irq_bus_lock(struct irq_data *d)
>  {
>         struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> @@ -732,6 +751,7 @@ static int pca953x_irq_setup(struct pca953x_chip *chip,
>         irq_chip->name = dev_name(&chip->client->dev);
>         irq_chip->irq_mask = pca953x_irq_mask;
>         irq_chip->irq_unmask = pca953x_irq_unmask;
> +       irq_chip->irq_set_wake = pca953x_irq_set_wake;

Is it possible to assign this callback conditionally depending on
client->irq and avoid the if (chip->irq_parent) in
pca953x_irq_set_wake()?

Bart

>         irq_chip->irq_bus_lock = pca953x_irq_bus_lock;
>         irq_chip->irq_bus_sync_unlock = pca953x_irq_bus_sync_unlock;
>         irq_chip->irq_set_type = pca953x_irq_set_type;
> @@ -747,6 +767,7 @@ static int pca953x_irq_setup(struct pca953x_chip *chip,
>         }
>
>         gpiochip_set_nested_irqchip(&chip->gpio_chip, irq_chip, client->irq);
> +       chip->irq_parent = client->irq;
>
>         return 0;
>  }
> --
> 2.17.1
>
Linus Walleij Feb. 13, 2019, 9:44 a.m. | #2
Top-posting to Thomas Petazzoni, he's been working on this driver
recently and is also on top of thing when it comes to wakeup.

Thomas, it would be great if you can take a look at this.

Yours,
Linus Walleij

On Tue, Feb 12, 2019 at 3:17 PM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
>
> Implement the irq_set_wake() method in the (optional) irq_chip of the
> GPIO expander, and propagate wake-up settings to the upstream interrupt
> controller.  This allows GPIOs connected to a PCA953X GPIO expander to
> serve as wake-up sources.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Tested with a PCA9654 and gpio-keys on an R-Car Ebisu-4D board.
> ---
>  drivers/gpio/gpio-pca953x.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index de52f63863dbe59b..8dfb8e326b9d12ca 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -151,6 +151,7 @@ struct pca953x_chip {
>         u8 irq_trig_raise[MAX_BANK];
>         u8 irq_trig_fall[MAX_BANK];
>         struct irq_chip irq_chip;
> +       unsigned int irq_parent;
>  #endif
>
>         struct i2c_client *client;
> @@ -513,6 +514,24 @@ static void pca953x_irq_unmask(struct irq_data *d)
>         chip->irq_mask[d->hwirq / BANK_SZ] |= 1 << (d->hwirq % BANK_SZ);
>  }
>
> +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);
> +       int error = 0;
> +
> +       if (chip->irq_parent) {
> +               error = irq_set_irq_wake(chip->irq_parent, on);
> +               if (error) {
> +                       dev_dbg(&chip->client->dev,
> +                               "irq %u doesn't support irq_set_wake\n",
> +                               chip->irq_parent);
> +                       chip->irq_parent = 0;
> +               }
> +       }
> +       return error;
> +}
> +
>  static void pca953x_irq_bus_lock(struct irq_data *d)
>  {
>         struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> @@ -732,6 +751,7 @@ static int pca953x_irq_setup(struct pca953x_chip *chip,
>         irq_chip->name = dev_name(&chip->client->dev);
>         irq_chip->irq_mask = pca953x_irq_mask;
>         irq_chip->irq_unmask = pca953x_irq_unmask;
> +       irq_chip->irq_set_wake = pca953x_irq_set_wake;
>         irq_chip->irq_bus_lock = pca953x_irq_bus_lock;
>         irq_chip->irq_bus_sync_unlock = pca953x_irq_bus_sync_unlock;
>         irq_chip->irq_set_type = pca953x_irq_set_type;
> @@ -747,6 +767,7 @@ static int pca953x_irq_setup(struct pca953x_chip *chip,
>         }
>
>         gpiochip_set_nested_irqchip(&chip->gpio_chip, irq_chip, client->irq);
> +       chip->irq_parent = client->irq;
>
>         return 0;
>  }
> --
> 2.17.1
>
Geert Uytterhoeven Feb. 13, 2019, 9:58 a.m. | #3
Hi Bartosz,

CC Thomas

On Tue, Feb 12, 2019 at 3:35 PM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
> wt., 12 lut 2019 o 15:17 Geert Uytterhoeven <geert+renesas@glider.be> napisał(a):
> > Implement the irq_set_wake() method in the (optional) irq_chip of the
> > GPIO expander, and propagate wake-up settings to the upstream interrupt
> > controller.  This allows GPIOs connected to a PCA953X GPIO expander to
> > serve as wake-up sources.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

> > --- a/drivers/gpio/gpio-pca953x.c
> > +++ b/drivers/gpio/gpio-pca953x.c
> > @@ -151,6 +151,7 @@ struct pca953x_chip {
> >         u8 irq_trig_raise[MAX_BANK];
> >         u8 irq_trig_fall[MAX_BANK];
> >         struct irq_chip irq_chip;
> > +       unsigned int irq_parent;
> >  #endif
> >
> >         struct i2c_client *client;
> > @@ -513,6 +514,24 @@ static void pca953x_irq_unmask(struct irq_data *d)
> >         chip->irq_mask[d->hwirq / BANK_SZ] |= 1 << (d->hwirq % BANK_SZ);
> >  }
> >
> > +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);
> > +       int error = 0;
> > +
> > +       if (chip->irq_parent) {
> > +               error = irq_set_irq_wake(chip->irq_parent, on);
> > +               if (error) {
> > +                       dev_dbg(&chip->client->dev,
> > +                               "irq %u doesn't support irq_set_wake\n",
> > +                               chip->irq_parent);
> > +                       chip->irq_parent = 0;
> > +               }
> > +       }
> > +       return error;
> > +}
> > +
> >  static void pca953x_irq_bus_lock(struct irq_data *d)
> >  {
> >         struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> > @@ -732,6 +751,7 @@ static int pca953x_irq_setup(struct pca953x_chip *chip,
> >         irq_chip->name = dev_name(&chip->client->dev);
> >         irq_chip->irq_mask = pca953x_irq_mask;
> >         irq_chip->irq_unmask = pca953x_irq_unmask;
> > +       irq_chip->irq_set_wake = pca953x_irq_set_wake;
>
> Is it possible to assign this callback conditionally depending on
> client->irq and avoid the if (chip->irq_parent) in
> pca953x_irq_set_wake()?

Note that pca953x_irq_setup() already returns early if client->irq is not
a valid interrupt number.

chip->irq_parent is also used as a flag to indicate if the parent
interrupt controller supports wake-up.  If it doesn't, it is cleared
again.  See e.g. commit ffb8e44bd7617ede ("gpio: pcf857x: Check for
irq_set_irq_wake() failures").

Or would it better to clear irq_chip->irq_set_wake instead?
That's something we couldn't do before each instance started using its
own irq_chip instance (e.g. commit 5c4fee63c5ed8133 ("gpio: pca953x: use
a per instance irq_chip structure")).

[more hacking]

Yep, clearing irq_chip->irq_set_wake also works.

But given pca953x_irq_set_wake() doesn't do anything, besides forwarding
to the parent, it seems it can just call irq_set_irq_wake() unconditionally,
and propagate its error code.  I failed to realize that when fixing pcf857x,
where I used the solution for rcar-gpio, which does need to do other things.

Note that we can't use irq_chip_set_wake_parent() as the callback, as that
relies on CONFIG_IRQ_DOMAIN_HIERARCHY (which may not be set), and
setting up irq_data.parent_data (else it crashes).

Will send a v2, and will simplify pcf857x, after some more testing,...

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

Patch

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index de52f63863dbe59b..8dfb8e326b9d12ca 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -151,6 +151,7 @@  struct pca953x_chip {
 	u8 irq_trig_raise[MAX_BANK];
 	u8 irq_trig_fall[MAX_BANK];
 	struct irq_chip irq_chip;
+	unsigned int irq_parent;
 #endif
 
 	struct i2c_client *client;
@@ -513,6 +514,24 @@  static void pca953x_irq_unmask(struct irq_data *d)
 	chip->irq_mask[d->hwirq / BANK_SZ] |= 1 << (d->hwirq % BANK_SZ);
 }
 
+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);
+	int error = 0;
+
+	if (chip->irq_parent) {
+		error = irq_set_irq_wake(chip->irq_parent, on);
+		if (error) {
+			dev_dbg(&chip->client->dev,
+				"irq %u doesn't support irq_set_wake\n",
+				chip->irq_parent);
+			chip->irq_parent = 0;
+		}
+	}
+	return error;
+}
+
 static void pca953x_irq_bus_lock(struct irq_data *d)
 {
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
@@ -732,6 +751,7 @@  static int pca953x_irq_setup(struct pca953x_chip *chip,
 	irq_chip->name = dev_name(&chip->client->dev);
 	irq_chip->irq_mask = pca953x_irq_mask;
 	irq_chip->irq_unmask = pca953x_irq_unmask;
+	irq_chip->irq_set_wake = pca953x_irq_set_wake;
 	irq_chip->irq_bus_lock = pca953x_irq_bus_lock;
 	irq_chip->irq_bus_sync_unlock = pca953x_irq_bus_sync_unlock;
 	irq_chip->irq_set_type = pca953x_irq_set_type;
@@ -747,6 +767,7 @@  static int pca953x_irq_setup(struct pca953x_chip *chip,
 	}
 
 	gpiochip_set_nested_irqchip(&chip->gpio_chip, irq_chip, client->irq);
+	chip->irq_parent = client->irq;
 
 	return 0;
 }