| Message ID | 20190212141715.3966-1-geert+renesas@glider.be |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: pca953x: Add wake-up support | expand |
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 >
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 >
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
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; }
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(+)