| Message ID | 1416213491-9775-3-git-send-email-alexander.stein@systec-electronic.com |
|---|---|
| State | Needs Review / ACK, archived |
| Headers | show |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 1 errors, 0 warnings, 0 lines checked |
| robh/patch-applied | success |
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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(-)