[PATCH 1/3] gpio: mcp23s08: request a shared interrupt

[PATCH 1/3] gpio: mcp23s08: request a shared interrupt

[Alexander Stein]

Request a shared interrupt when requesting a mcp23s08 GPIO interrupt.

Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
 drivers/gpio/gpio-mcp23s08.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-mcp23s08.c b/drivers/gpio/gpio-mcp23s08.c
index d5e9586..9295b6e 100644
--- a/drivers/gpio/gpio-mcp23s08.c
+++ b/drivers/gpio/gpio-mcp23s08.c
@@ -485,7 +485,8 @@ static int mcp23s08_irq_setup(struct mcp23s08 *mcp)
 		return -ENODEV;
 
 	err = devm_request_threaded_irq(chip->dev, mcp->irq, NULL, mcp23s08_irq,
-					IRQF_TRIGGER_LOW | IRQF_ONESHOT,
+					IRQF_TRIGGER_LOW | IRQF_ONESHOT |
+					IRQF_SHARED,
 					dev_name(chip->dev), mcp);
 	if (err != 0) {
 		dev_err(chip->dev, "unable to request IRQ#%d: %d\n",
-- 
2.0.4

[PATCH 2/3] gpio: mcp23s08: Add simple IRQ support for SPI devices

[Alexander Stein]

Currently this implementation only supports one IRQ for (all) SPI devices
using the same chip select.

Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
 drivers/gpio/gpio-mcp23s08.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpio/gpio-mcp23s08.c b/drivers/gpio/gpio-mcp23s08.c
index 9295b6e..529025e 100644
--- a/drivers/gpio/gpio-mcp23s08.c
+++ b/drivers/gpio/gpio-mcp23s08.c
@@ -935,11 +935,14 @@ static int mcp23s08_probe(struct spi_device *spi)
 		return -ENOMEM;
 	spi_set_drvdata(spi, data);
 
+	spi->irq = irq_of_parse_and_map(spi->dev.of_node, 0);
+
 	for (addr = 0; addr < ARRAY_SIZE(pdata->chip); addr++) {
 		if (!(spi_present_mask & (1 << addr)))
 			continue;
 		chips--;
 		data->mcp[addr] = &data->chip[chips];
+		data->mcp[addr]->irq = spi->irq;
 		status = mcp23s08_probe_one(data->mcp[addr], &spi->dev, spi,
 					    0x40 | (addr << 1), type, pdata,
 					    addr);
@@ -980,6 +983,8 @@ static int mcp23s08_remove(struct spi_device *spi)
 		if (!data->mcp[addr])
 			continue;
 
+		if (spi->irq && data->mcp[addr]->irq_controller)
+			mcp23s08_irq_teardown(data->mcp[addr]);
 		gpiochip_remove(&data->mcp[addr]->chip);
 	}
 	kfree(data);
-- 
2.0.4

[PATCH 3/3] gpio: mcp23s08: Add option to configure IRQ as active high

[Alexander Stein]

Default is active low, but if property is specified in DT set INTPOL flag.

Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
 .../devicetree/bindings/gpio/gpio-mcp23s08.txt     |  2 ++
 drivers/gpio/gpio-mcp23s08.c                       | 30 +++++++++++++++++-----
 2 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt b/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt
index c306a2d0..47dab72 100644
--- a/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt
@@ -57,6 +57,8 @@ Optional device specific properties:
         occurred on. If it is not set, the interrupt are only generated for the
         bank they belong to.
         On devices with only one interrupt output this property is useless.
+- microchip,irq-active-high: Sets the INTPOL flag in the IOCON register. This
+        configures the IRQ output as active high.
 
 Example I2C (with interrupt):
 gpiom1: gpio@20 {
diff --git a/drivers/gpio/gpio-mcp23s08.c b/drivers/gpio/gpio-mcp23s08.c
index 529025e..1d5ffb4 100644
--- a/drivers/gpio/gpio-mcp23s08.c
+++ b/drivers/gpio/gpio-mcp23s08.c
@@ -65,6 +65,7 @@ struct mcp23s08_ops {
 
 struct mcp23s08 {
 	u8			addr;
+	bool			irq_active_high;
 
 	u16			cache[11];
 	u16			irq_rise;
@@ -476,6 +477,7 @@ static int mcp23s08_irq_setup(struct mcp23s08 *mcp)
 {
 	struct gpio_chip *chip = &mcp->chip;
 	int err, irq, j;
+	unsigned long irqflags = IRQF_ONESHOT | IRQF_SHARED;
 
 	mutex_init(&mcp->irq_lock);
 
@@ -484,10 +486,13 @@ static int mcp23s08_irq_setup(struct mcp23s08 *mcp)
 	if (!mcp->irq_domain)
 		return -ENODEV;
 
+	if (mcp->irq_active_high)
+		irqflags |= IRQF_TRIGGER_HIGH;
+	else
+		irqflags |= IRQF_TRIGGER_LOW;
+
 	err = devm_request_threaded_irq(chip->dev, mcp->irq, NULL, mcp23s08_irq,
-					IRQF_TRIGGER_LOW | IRQF_ONESHOT |
-					IRQF_SHARED,
-					dev_name(chip->dev), mcp);
+					irqflags, dev_name(chip->dev), mcp);
 	if (err != 0) {
 		dev_err(chip->dev, "unable to request IRQ#%d: %d\n",
 			mcp->irq, err);
@@ -589,6 +594,7 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
 
 	mcp->data = data;
 	mcp->addr = addr;
+	mcp->irq_active_high = false;
 
 	mcp->chip.direction_input = mcp23s08_direction_input;
 	mcp->chip.get = mcp23s08_get;
@@ -648,14 +654,24 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
 		goto fail;
 
 	mcp->irq_controller = pdata->irq_controller;
-	if (mcp->irq && mcp->irq_controller && (type == MCP_TYPE_017))
-		mirror = pdata->mirror;
+	if (mcp->irq && mcp->irq_controller) {
+		mcp->irq_active_high = of_property_read_bool(mcp->chip.of_node,
+				"microchip,irq-active-high");
 
-	if ((status & IOCON_SEQOP) || !(status & IOCON_HAEN) || mirror) {
+		if (type == MCP_TYPE_017)
+			mirror = pdata->mirror;
+	}
+
+	if ((status & IOCON_SEQOP) || !(status & IOCON_HAEN) || mirror ||
+	     mcp->irq_active_high) {
 		/* mcp23s17 has IOCON twice, make sure they are in sync */
 		status &= ~(IOCON_SEQOP | (IOCON_SEQOP << 8));
 		status |= IOCON_HAEN | (IOCON_HAEN << 8);
-		status &= ~(IOCON_INTPOL | (IOCON_INTPOL << 8));
+		if (mcp->irq_active_high)
+			status |= IOCON_INTPOL | (IOCON_INTPOL << 8);
+		else
+			status &= ~(IOCON_INTPOL | (IOCON_INTPOL << 8));
+
 		if (mirror)
 			status |= IOCON_MIRROR | (IOCON_MIRROR << 8);
 
-- 
2.0.4

Re: [PATCH 1/3] gpio: mcp23s08: request a shared interrupt

[Linus Walleij]

On Mon, Nov 17, 2014 at 9:38 AM, Alexander Stein
<alexander.stein@systec-electronic.com> wrote:

> Request a shared interrupt when requesting a mcp23s08 GPIO interrupt.
>
> Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH 2/3] gpio: mcp23s08: Add simple IRQ support for SPI devices

[Linus Walleij]

On Mon, Nov 17, 2014 at 9:38 AM, Alexander Stein
<alexander.stein@systec-electronic.com> wrote:

> Currently this implementation only supports one IRQ for (all) SPI devices
> using the same chip select.
>
> Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH 3/3] gpio: mcp23s08: Add option to configure IRQ as active high

[Linus Walleij]

On Mon, Nov 17, 2014 at 9:38 AM, Alexander Stein
<alexander.stein@systec-electronic.com> wrote:

> Default is active low, but if property is specified in DT set INTPOL flag.
>
> Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
> ---
>  .../devicetree/bindings/gpio/gpio-mcp23s08.txt     |  2 ++
>  drivers/gpio/gpio-mcp23s08.c                       | 30 +++++++++++++++++-----
>  2 files changed, 25 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt b/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt
> index c306a2d0..47dab72 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt
> @@ -57,6 +57,8 @@ Optional device specific properties:
>          occurred on. If it is not set, the interrupt are only generated for the
>          bank they belong to.
>          On devices with only one interrupt output this property is useless.
> +- microchip,irq-active-high: Sets the INTPOL flag in the IOCON register. This
> +        configures the IRQ output as active high.

I don't understand this AT ALL.

The normal way is that the *client* using the resource specify the
polarity.

#include <dt-bindings/interrupt-controller/irq.h>

interrupts = <0 60 IRQ_TYPE_LEVEL_LOW>;

This will then result in .set_type() being called on the irqchip.

I suspect the real change you want to do is to support
IRQ_TYPE_LEVEL_HIGH and IRQ_TYPE_LEVEL_LOW
in the .set_type() callback or something, or just switch all
clients from rising to falling edge or vice versa.

And it says is "configures the IRQ output", this seems more like
an input trigger?

Sorry too confused by this, can you explain?

Yours,
Linus Walleij

Re: [PATCH 3/3] gpio: mcp23s08: Add option to configure IRQ as active high

[Alexander Stein]

On Thursday 27 November 2014 14:58:39, Linus Walleij wrote:
> On Mon, Nov 17, 2014 at 9:38 AM, Alexander Stein
> <alexander.stein@systec-electronic.com> wrote:
> 
> > Default is active low, but if property is specified in DT set INTPOL flag.
> >
> > Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
> > ---
> >  .../devicetree/bindings/gpio/gpio-mcp23s08.txt     |  2 ++
> >  drivers/gpio/gpio-mcp23s08.c                       | 30 +++++++++++++++++-----
> >  2 files changed, 25 insertions(+), 7 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt b/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt
> > index c306a2d0..47dab72 100644
> > --- a/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt
> > +++ b/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt
> > @@ -57,6 +57,8 @@ Optional device specific properties:
> >          occurred on. If it is not set, the interrupt are only generated for the
> >          bank they belong to.
> >          On devices with only one interrupt output this property is useless.
> > +- microchip,irq-active-high: Sets the INTPOL flag in the IOCON register. This
> > +        configures the IRQ output as active high.
> 
> I don't understand this AT ALL.
> 
> The normal way is that the *client* using the resource specify the
> polarity.
> 
> #include <dt-bindings/interrupt-controller/irq.h>
> 
> interrupts = <0 60 IRQ_TYPE_LEVEL_LOW>;
> 
> This will then result in .set_type() being called on the irqchip.
> 
> I suspect the real change you want to do is to support
> IRQ_TYPE_LEVEL_HIGH and IRQ_TYPE_LEVEL_LOW
> in the .set_type() callback or something, or just switch all
> clients from rising to falling edge or vice versa.
> 
> And it says is "configures the IRQ output", this seems more like
> an input trigger?
> 
> Sorry too confused by this, can you explain?

Sure. Until now ODR is cleared as reset default while INTPOL explicitly gets cleared. So the INT output pin on the MCP23S17 is an active-low output pin which will trigger an active-low interrupt. While this is useually great, as most interrupt pins are active low, our board design requires an active-high output. This is done that the voltage level shift required for the interrupt pin can be done by a simple transistor.
So, setting IRQ_TYPE_LEVEL_HIGH is simply wrong and this still won't work either as no interrupt will ever arive at the controller. What I need to do, is to change the interrupt polarity (to active high) at MCP23S17 while retaining the interrupt polarity on the controller as active-low.
So I configure the chip to set the interrupt line as high upon IRQ, thus IRQF_TRIGGER_HIGH has to be passed to request_*irq.
You might wonder why I then also changed IRQF_TRIGGER_LOW to IRQF_TRIGGER_HIGH for my board the interrupt line is actually active low. This is due the fact that GIC only supports active high-interrupts (any other flag results in request fail) but the microcontroller has a feature to inverse these interrupt triggers.
I don't know if there is a feature to inverse interrupt polarity individually due to board designs.
I hope this makes things a bit more clear.

Best regards,
Alexander

Re: [PATCH 3/3] gpio: mcp23s08: Add option to configure IRQ as active high

[Linus Walleij]

On Thu, Nov 27, 2014 at 3:36 PM, Alexander Stein
<alexander.stein@systec-electronic.com> wrote:
> On Thursday 27 November 2014 14:58:39, Linus Walleij wrote:

>> Sorry too confused by this, can you explain?
>
> Sure. Until now ODR is cleared as reset default while INTPOL explicitly gets cleared.
> So the INT output pin on the MCP23S17 is an active-low output pin which will trigger
> an active-low interrupt.

OK... I thought it was some kind of input line to the device. Now I get it, it
is what is generated *out* of this device. Please state this in the bindings.

> While this is useually great, as most interrupt pins are active low, our board design
> requires an active-high output.

OK I get it :)

> You might wonder why I then also changed IRQF_TRIGGER_LOW to
> IRQF_TRIGGER_HIGH for my board the interrupt line is actually active low.
> This is due the fact that GIC only supports active high-interrupts (any other flag
> results in request fail) but the microcontroller has a feature to inverse these interrupt triggers.

Very convoluted, but common in electronics, OK, no problem.

Yours,
Linus Walleij