[PATCH 1/2] gpio: max732x: Propagate wake-up setting to parent irq controller

[PATCH 1/2] gpio: max732x: Propagate wake-up setting to parent irq controller

[Semen Protsenko]

Set .irq_set_wake callback to prevent possible issues on wake-up.

This patch was inspired by this commit:
b80eef95beb04760629822fa130aeed54cdfafca

Signed-off-by: Semen Protsenko <semen.protsenko@globallogic.com>
---
 drivers/gpio/gpio-max732x.c |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpio/gpio-max732x.c b/drivers/gpio/gpio-max732x.c
index 0fa4543..1885e5c 100644
--- a/drivers/gpio/gpio-max732x.c
+++ b/drivers/gpio/gpio-max732x.c
@@ -429,6 +429,14 @@ static int max732x_irq_set_type(struct irq_data *d, unsigned int type)
 	return 0;
 }
 
+static int max732x_irq_set_wake(struct irq_data *data, unsigned int on)
+{
+	struct max732x_chip *chip = irq_data_get_irq_chip_data(data);
+
+	irq_set_irq_wake(chip->client->irq, on);
+	return 0;
+}
+
 static struct irq_chip max732x_irq_chip = {
 	.name			= "max732x",
 	.irq_mask		= max732x_irq_mask,
@@ -436,6 +444,7 @@ static struct irq_chip max732x_irq_chip = {
 	.irq_bus_lock		= max732x_irq_bus_lock,
 	.irq_bus_sync_unlock	= max732x_irq_bus_sync_unlock,
 	.irq_set_type		= max732x_irq_set_type,
+	.irq_set_wake		= max732x_irq_set_wake,
 };
 
 static uint8_t max732x_irq_pending(struct max732x_chip *chip)
-- 
1.7.9.5

Re: [PATCH 1/2] gpio: max732x: Propagate wake-up setting to parent irq controller

[Linus Walleij]

On Tue, Apr 21, 2015 at 7:27 PM, Semen Protsenko
<semen.protsenko@globallogic.com> wrote:

> Set .irq_set_wake callback to prevent possible issues on wake-up.
>
> This patch was inspired by this commit:
> b80eef95beb04760629822fa130aeed54cdfafca
>
> Signed-off-by: Semen Protsenko <semen.protsenko@globallogic.com>

Are these real, observed issues?

Patch applied, but it seems we need a general approach to
cover a few GPIO drivers with this kind of thing.

Is this how we should always do it?

Yours,
Linus Walleij

Re: [PATCH 1/2] gpio: max732x: Propagate wake-up setting to parent irq controller

[Sam Protsenko]

On Mon, May 4, 2015 at 4:32 PM, Linus Walleij <linus.walleij@linaro.org> wrote:

> Are these real, observed issues?

The issue was observed for PCF857x expander (not by me), and it's described in
this commit: b80eef95beb04760629822fa130aeed54cdfafca .

It seems to me that the issue is real. But we need some really particular
hardware configuration and particular use-case to reproduce it. As I understand
from commit message (for mentioned commit), this issue was catch on
system with "arch/arm/boot/dts/sh73a0-kzm9g-reference.dts" device tree file.
As you can see from that file, there is a keyboard ("gpio-keys") connected
to PCF8575 expander. In order to wake system up from keyboard event (key
pressed), parent interrupt controller ("interrupt-parent" field in "pcf8575"
node) should has the same wake-up settings as PCF8575 has.

So this issue may affect other expanders (like MAX732X) as well. I haven't
tested if it's true (only validated that everything works fine with this patch).
I'm now in the middle of building my own PCB with MAX7325 chip for testing
such things (since I was relocated from project where I had board with MAX732X).

> Patch applied, but it seems we need a general approach to
> cover a few GPIO drivers with this kind of thing.
>
> Is this how we should always do it?

I think so (well, at least it seems to be correct for GPIO expanders). But I'd
verify each particular driver to be completely sure that it's really needed and
it wouldn't create some new issues. MAX732X chip seems to be very similar to
PCF857X one, so in this particular case I'm sure that this patch is a good thing
to have.

Re: [PATCH 1/2] gpio: max732x: Propagate wake-up setting to parent irq controller

[Linus Walleij]

On Mon, May 4, 2015 at 8:23 PM, Sam Protsenko
<semen.protsenko@globallogic.com> wrote:
> On Mon, May 4, 2015 at 4:32 PM, Linus Walleij <linus.walleij@linaro.org> wrote:

>> Patch applied, but it seems we need a general approach to
>> cover a few GPIO drivers with this kind of thing.
>>
>> Is this how we should always do it?
>
> I think so (well, at least it seems to be correct for GPIO expanders). But I'd
> verify each particular driver to be completely sure that it's really needed and
> it wouldn't create some new issues. MAX732X chip seems to be very similar to
> PCF857X one, so in this particular case I'm sure that this patch is a good thing
> to have.

OK yeah, maybe we could provide a list of "usual suspects", what do you
say about "my" expanders:

drivers/gpio/gpio-stmpe.c
drivers/gpio/gpio-tc3589x.c

I can surely patch and test these.

Yours,
Linus Walleij

Re: [PATCH 1/2] gpio: max732x: Propagate wake-up setting to parent irq controller

[Sam Protsenko]

On Tue, May 12, 2015 at 10:34 AM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> OK yeah, maybe we could provide a list of "usual suspects", what do you
> say about "my" expanders:
>
> drivers/gpio/gpio-stmpe.c
> drivers/gpio/gpio-tc3589x.c
>
> I can surely patch and test these.

This issue seems to be pretty common for all GPIO chips that have IRQ chip
capability. So your expanders should be patched too. But still, the best course
here is to actually reproduce the issue first, and only then proceed to
patching. So if you have boards with those expanders -- it should be easy to
come up with some use-case that reproduces the issue mentioned in the original
commit.