Re: [PATCH] gpio: pca953x: Add wake-up support

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Bartosz,

CC Thomas

On Tue, Feb 12, 2019 at 3:35 PM Bartosz Golaszewski
<bgolaszewski@xxxxxxxxxxxx> wrote:
> wt., 12 lut 2019 o 15:17 Geert Uytterhoeven <geert+renesas@xxxxxxxxx> 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@xxxxxxxxx>

> > --- 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@xxxxxxxxxxxxxx

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