[1/4] pinctrl: mcp23s08: IRQ behavior for open-drain interrupts

Message ID 9e78a4a697c8d4d3028ea8780bd925471b49af3c.1516997103.git.jan.kundrat@cesnet.cz
State New
Headers show
Series
  • Improvements for MCP23Sxx chips
Related show

Commit Message

Jan Kundrát Jan. 25, 2018, 3:08 p.m.
The hardware supports three different IRQ modes:
- open drain, i.e., an active-low output which requires an external
  pull-up and supports IRQ line sharing,
- active low, with push-pull output,
- active high, with push-pull output.

My understanding of electronics is limited, but it is "wrong" to connect
different push-pull outpus together. It could sort-of work if the
chips/board actually have some resistors there, but it's an ugly thing
to do.

In the past, this driver did not touch the ODR bit (for open-drain)
operation. During power-on-reset, it is set to zero. Unless some
platform code actually set this bit to one, this means that the code
never supported open drain operation.

This change makes this handling explicit. If you want sharing of the IRQ
line, then use the microchip,irq-open-drain mode and add an external
pull-up.

There's some risk here. If, e.g., the bootloader sets the ODR bit for
some reason, the HW shares the IRQ lines, and nobody changes the DT,
then your HW will suddenly use push-pull driver for the IRQ line. If
there is any other chip on the same lane, the HW might suffer (my chip
survived this when I "tested" this by accident with no bootloader code
to set the ODR bit).

Signed-off-by: Jan Kundrát <jan.kundrat@cesnet.cz>
---
 .../bindings/pinctrl/pinctrl-mcp23s08.txt          |  4 ++
 drivers/pinctrl/pinctrl-mcp23s08.c                 | 82 +++++++++++++++-------
 2 files changed, 61 insertions(+), 25 deletions(-)

Comments

Phil Reid Jan. 29, 2018, 1:06 a.m. | #1
G'day Jan,

On 25/01/2018 23:08, Jan Kundrát wrote:
> The hardware supports three different IRQ modes:
> - open drain, i.e., an active-low output which requires an external
>    pull-up and supports IRQ line sharing,
> - active low, with push-pull output,
> - active high, with push-pull output.
> 
> My understanding of electronics is limited, but it is "wrong" to connect
> different push-pull outpus together. It could sort-of work if the
> chips/board actually have some resistors there, but it's an ugly thing
> to do.
> 
> In the past, this driver did not touch the ODR bit (for open-drain)
> operation. During power-on-reset, it is set to zero. Unless some
> platform code actually set this bit to one, this means that the code
> never supported open drain operation.
> 
> This change makes this handling explicit. If you want sharing of the IRQ
> line, then use the microchip,irq-open-drain mode and add an external
> pull-up.
> 
> There's some risk here. If, e.g., the bootloader sets the ODR bit for
> some reason, the HW shares the IRQ lines, and nobody changes the DT,
> then your HW will suddenly use push-pull driver for the IRQ line. If
> there is any other chip on the same lane, the HW might suffer (my chip
> survived this when I "tested" this by accident with no bootloader code
> to set the ODR bit).
> 
> Signed-off-by: Jan Kundrát <jan.kundrat@cesnet.cz>
> ---
>   .../bindings/pinctrl/pinctrl-mcp23s08.txt          |  4 ++
>   drivers/pinctrl/pinctrl-mcp23s08.c                 | 82 +++++++++++++++-------
>   2 files changed, 61 insertions(+), 25 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt
> index 9c451c20dda4..92aa91027c68 100644
> --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt
> @@ -58,6 +58,10 @@ Optional device specific properties:
>           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 polarity as active high.
> +- microchip,irq-open-drain: Sets the ODR flag in the IOCON register. This
> +        configures the IRQ output as active low open-drain and allows sharing
> +	of the IRQ line among several chips. Note that irq-active-high and
> +	irq-open-drain are mutually exclusive.

I submitted a patch for this in November, But haven't followed up on the last review as yet.
See: https://patchwork.ozlabs.org/cover/839923/

I used exactly the same dt in my first propsal. But you'll see form the discussions that series
This needs to be the standard 'drive-open-drain' flag

Patch 2/5 has been reviewed by Sebastian acked Rob.


>   
>   Example I2C (with interrupt):
>   gpiom1: gpio@20 {
> diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c
> index c017860619d6..186101c575e3 100644
> --- a/drivers/pinctrl/pinctrl-mcp23s08.c
> +++ b/drivers/pinctrl/pinctrl-mcp23s08.c
> @@ -54,9 +54,15 @@
>   
>   struct mcp23s08;
>   
> +enum mcp23s08_irq_regime {
> +	MCP_IRQ_PULL_LOW,
> +	MCP_IRQ_PUSH_UP,
> +	MCP_IRQ_OPEN_DRAIN
> +};
> +
>   struct mcp23s08 {
>   	u8			addr;
> -	bool			irq_active_high;
> +	enum mcp23s08_irq_regime	irq_regime;
>   	bool			reg_shift;
>   
>   	u16			irq_rise;
> @@ -625,10 +631,17 @@ static int mcp23s08_irq_setup(struct mcp23s08 *mcp)
>   	int err;
>   	unsigned long irqflags = IRQF_ONESHOT | IRQF_SHARED;
>   
> -	if (mcp->irq_active_high)
> -		irqflags |= IRQF_TRIGGER_HIGH;
> -	else
> +	switch (mcp->irq_regime) {
> +	case MCP_IRQ_PULL_LOW:
>   		irqflags |= IRQF_TRIGGER_LOW;
> +		break;
> +	case MCP_IRQ_PUSH_UP:
> +		irqflags |= IRQF_TRIGGER_HIGH;
> +		break;
> +	case MCP_IRQ_OPEN_DRAIN:
> +		irqflags |= IRQF_TRIGGER_LOW | IRQF_SHARED;
> +		break;
> +	}
The original code does the same thing.
open drain is active-low.
Sharing can also be achieved by AND / OR gates with push-pull drivers.
So no need to treat that differently.

I'm trying to get ride f this bit of code as it should be needed.
see 1/5 of my series.

>   
>   	err = devm_request_threaded_irq(chip->parent, mcp->irq, NULL,
>   					mcp23s08_irq,
> @@ -780,7 +793,7 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
>   
>   	mcp->dev = dev;
>   	mcp->addr = addr;
> -	mcp->irq_active_high = false;
> +	mcp->irq_regime = MCP_IRQ_PULL_LOW;
>   
>   	mcp->chip.direction_input = mcp23s08_direction_input;
>   	mcp->chip.get = mcp23s08_get;
> @@ -866,33 +879,52 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
>   	mcp->irq_controller =
>   		device_property_read_bool(dev, "interrupt-controller");
>   	if (mcp->irq && mcp->irq_controller) {
> -		mcp->irq_active_high =
> -			device_property_read_bool(dev,
> -					      "microchip,irq-active-high");
> +		bool irq_active_high = device_property_read_bool(dev,
> +						"microchip,irq-active-high");
> +		bool irq_open_drain = device_property_read_bool(dev,
> +						"microchip,irq-open-drain");
> +		if (irq_active_high && irq_open_drain) {
> +			dev_err(dev, "microchip,irq-open-drain and "
> +				"microchip,irq-active-high are mutually exclusive\n");
> +			ret = -EINVAL;
> +			goto fail;
> +		} else if (irq_active_high) {
> +			mcp->irq_regime = MCP_IRQ_PUSH_UP;
> +		} else if (irq_open_drain) {
> +			mcp->irq_regime = MCP_IRQ_OPEN_DRAIN;
> +		}
>   
>   		mirror = device_property_read_bool(dev, "microchip,irq-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);
> -		if (mcp->irq_active_high)
> -			status |= IOCON_INTPOL | (IOCON_INTPOL << 8);
> -		else
> -			status &= ~(IOCON_INTPOL | (IOCON_INTPOL << 8));
> +	/* mcp23s17 has IOCON twice, make sure they are in sync */
> +	status &= ~(IOCON_SEQOP | (IOCON_SEQOP << 8));
> +	status |= IOCON_HAEN | (IOCON_HAEN << 8);
>   
> -		if (mirror)
> -			status |= IOCON_MIRROR | (IOCON_MIRROR << 8);
> +	switch (mcp->irq_regime) {
> +	case MCP_IRQ_PUSH_UP:
> +		status |= IOCON_INTPOL | (IOCON_INTPOL << 8);
> +		status &= ~(IOCON_ODR | (IOCON_ODR << 8));
> +		break;
> +	case MCP_IRQ_PULL_LOW:
> +		status &= ~(IOCON_INTPOL | (IOCON_INTPOL << 8));
> +		status &= ~(IOCON_ODR | (IOCON_ODR << 8));
> +		break;
> +	case MCP_IRQ_OPEN_DRAIN:
> +		/* ODR overrides INTPOL */
> +		status |= IOCON_ODR | (IOCON_ODR << 8);
> +		break;
> +	}
>   
> -		if (type == MCP_TYPE_S18 || type == MCP_TYPE_018)
> -			status |= IOCON_INTCC | (IOCON_INTCC << 8);
> +	if (mirror)
> +		status |= IOCON_MIRROR | (IOCON_MIRROR << 8);
>   
> -		ret = mcp_write(mcp, MCP_IOCON, status);
> -		if (ret < 0)
> -			goto fail;
> -	}
> +	if (type == MCP_TYPE_S18 || type == MCP_TYPE_018)
> +		status |= IOCON_INTCC | (IOCON_INTCC << 8);
> +
> +	ret = mcp_write(mcp, MCP_IOCON, status);
> +	if (ret < 0)
> +		goto fail;
>   
>   	ret = devm_gpiochip_add_data(dev, &mcp->chip, mcp);
>   	if (ret < 0)
>
Jan Kundrát Jan. 29, 2018, 10:27 a.m. | #2
> I submitted a patch for this in November, But haven't followed 
> up on the last review as yet.
> See: https://patchwork.ozlabs.org/cover/839923/

Hi Phil, that looks nice. Sorry for duplicating your work. I would have 
sworn I haven't seen your patch, but I *think* I actually read that IRQ 
polarity discussion. Strange, perhaps I just forgot that it was about 
MCP23xxx.

> I used exactly the same dt in my first propsal. But you'll see 
> form the discussions that series
> This needs to be the standard 'drive-open-drain' flag

I wasn't aware of that one.

> The original code does the same thing.
> open drain is active-low.
> Sharing can also be achieved by AND / OR gates with push-pull drivers.
> So no need to treat that differently.

That's a good point. I agree that decoupling the irq-controller's 
configuration from this chip's IRQ output lane configuration (and hence 
moving the IRQ_SHARED etc to the DT) makes sense.

My change also writes to the MCP_IOCON unconditionally. For example, the 
old code would fail if "something" configured the chip for irq-open-drain 
before the kernel got a chance to run.

Will you have time to re-spin the series anytime soon? I can help if you'd 
like to.

Cheers,
Jan
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Phil Reid Jan. 30, 2018, 2:24 a.m. | #3
On 29/01/2018 18:27, Jan Kundrát wrote:
>> I submitted a patch for this in November, But haven't followed up on the last review as yet.
>> See: https://patchwork.ozlabs.org/cover/839923/
> 
> Hi Phil, that looks nice. Sorry for duplicating your work. I would have sworn I haven't seen your patch, but I *think* I actually read that IRQ polarity 
> discussion. Strange, perhaps I just forgot that it was about MCP23xxx.
> 
>> I used exactly the same dt in my first propsal. But you'll see form the discussions that series
>> This needs to be the standard 'drive-open-drain' flag
> 
> I wasn't aware of that one.
> 
>> The original code does the same thing.
>> open drain is active-low.
>> Sharing can also be achieved by AND / OR gates with push-pull drivers.
>> So no need to treat that differently.
> 
> That's a good point. I agree that decoupling the irq-controller's configuration from this chip's IRQ output lane configuration (and hence moving the IRQ_SHARED 
> etc to the DT) makes sense.
> 
> My change also writes to the MCP_IOCON unconditionally. For example, the old code would fail if "something" configured the chip for irq-open-drain before the 
> kernel got a chance to run.
Yeah I don't like that conditional logic for setting the config.
Tricky to change without potentially breaking something as you say.
But if you don't want mirror set and something is setting it before hand then things are broken.


> 
> Will you have time to re-spin the series anytime soon? I can help if you'd like to.
> 

I'll try and respin the open drain bit in the next week or so.
The other part of my series is a little more work I think.

Gives me some motivation to have a look at it.
Linus Walleij Feb. 7, 2018, 1:45 p.m. | #4
On Thu, Jan 25, 2018 at 4:08 PM, Jan Kundrát <jan.kundrat@cesnet.cz> wrote:

> The hardware supports three different IRQ modes:
> - open drain, i.e., an active-low output which requires an external
>   pull-up and supports IRQ line sharing,
> - active low, with push-pull output,
> - active high, with push-pull output.
>
> My understanding of electronics is limited, but it is "wrong" to connect
> different push-pull outpus together. It could sort-of work if the
> chips/board actually have some resistors there, but it's an ugly thing
> to do.
>
> In the past, this driver did not touch the ODR bit (for open-drain)
> operation. During power-on-reset, it is set to zero. Unless some
> platform code actually set this bit to one, this means that the code
> never supported open drain operation.
>
> This change makes this handling explicit. If you want sharing of the IRQ
> line, then use the microchip,irq-open-drain mode and add an external
> pull-up.
>
> There's some risk here. If, e.g., the bootloader sets the ODR bit for
> some reason, the HW shares the IRQ lines, and nobody changes the DT,
> then your HW will suddenly use push-pull driver for the IRQ line. If
> there is any other chip on the same lane, the HW might suffer (my chip
> survived this when I "tested" this by accident with no bootloader code
> to set the ODR bit).
>
> Signed-off-by: Jan Kundrát <jan.kundrat@cesnet.cz>

I'm holding this patch back pending the discussion with Phil
and any alternative approach from him.

Just resend it if nothing happens for a while.

I still try to apply the other patches for now (2-4).

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Phil Reid Feb. 9, 2018, 3:37 a.m. | #5
On 7/02/2018 21:45, Linus Walleij wrote:
> On Thu, Jan 25, 2018 at 4:08 PM, Jan Kundrát <jan.kundrat@cesnet.cz> wrote:
> 
>> The hardware supports three different IRQ modes:
>> - open drain, i.e., an active-low output which requires an external
>>    pull-up and supports IRQ line sharing,
>> - active low, with push-pull output,
>> - active high, with push-pull output.
>>
>> My understanding of electronics is limited, but it is "wrong" to connect
>> different push-pull outpus together. It could sort-of work if the
>> chips/board actually have some resistors there, but it's an ugly thing
>> to do.
>>
>> In the past, this driver did not touch the ODR bit (for open-drain)
>> operation. During power-on-reset, it is set to zero. Unless some
>> platform code actually set this bit to one, this means that the code
>> never supported open drain operation.
>>
>> This change makes this handling explicit. If you want sharing of the IRQ
>> line, then use the microchip,irq-open-drain mode and add an external
>> pull-up.
>>
>> There's some risk here. If, e.g., the bootloader sets the ODR bit for
>> some reason, the HW shares the IRQ lines, and nobody changes the DT,
>> then your HW will suddenly use push-pull driver for the IRQ line. If
>> there is any other chip on the same lane, the HW might suffer (my chip
>> survived this when I "tested" this by accident with no bootloader code
>> to set the ODR bit).
>>
>> Signed-off-by: Jan Kundrát <jan.kundrat@cesnet.cz>
> 
> I'm holding this patch back pending the discussion with Phil
> and any alternative approach from him.
> 
> Just resend it if nothing happens for a while.
> 
> I still try to apply the other patches for now (2-4).
> 

I tried applying patches 2-4 and a couple of other mcp patches to
https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/ for-next
before resending mine as for-next is a few days behind by the look,
to make sure it'd apply ok but didn't have much success.

But my git skills are not up to figuring out what I'm doing wrong.
I'll send once I see those patches on a branch.

Patch

diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt
index 9c451c20dda4..92aa91027c68 100644
--- a/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt
@@ -58,6 +58,10 @@  Optional device specific properties:
         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 polarity as active high.
+- microchip,irq-open-drain: Sets the ODR flag in the IOCON register. This
+        configures the IRQ output as active low open-drain and allows sharing
+	of the IRQ line among several chips. Note that irq-active-high and
+	irq-open-drain are mutually exclusive.
 
 Example I2C (with interrupt):
 gpiom1: gpio@20 {
diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c
index c017860619d6..186101c575e3 100644
--- a/drivers/pinctrl/pinctrl-mcp23s08.c
+++ b/drivers/pinctrl/pinctrl-mcp23s08.c
@@ -54,9 +54,15 @@ 
 
 struct mcp23s08;
 
+enum mcp23s08_irq_regime {
+	MCP_IRQ_PULL_LOW,
+	MCP_IRQ_PUSH_UP,
+	MCP_IRQ_OPEN_DRAIN
+};
+
 struct mcp23s08 {
 	u8			addr;
-	bool			irq_active_high;
+	enum mcp23s08_irq_regime	irq_regime;
 	bool			reg_shift;
 
 	u16			irq_rise;
@@ -625,10 +631,17 @@  static int mcp23s08_irq_setup(struct mcp23s08 *mcp)
 	int err;
 	unsigned long irqflags = IRQF_ONESHOT | IRQF_SHARED;
 
-	if (mcp->irq_active_high)
-		irqflags |= IRQF_TRIGGER_HIGH;
-	else
+	switch (mcp->irq_regime) {
+	case MCP_IRQ_PULL_LOW:
 		irqflags |= IRQF_TRIGGER_LOW;
+		break;
+	case MCP_IRQ_PUSH_UP:
+		irqflags |= IRQF_TRIGGER_HIGH;
+		break;
+	case MCP_IRQ_OPEN_DRAIN:
+		irqflags |= IRQF_TRIGGER_LOW | IRQF_SHARED;
+		break;
+	}
 
 	err = devm_request_threaded_irq(chip->parent, mcp->irq, NULL,
 					mcp23s08_irq,
@@ -780,7 +793,7 @@  static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
 
 	mcp->dev = dev;
 	mcp->addr = addr;
-	mcp->irq_active_high = false;
+	mcp->irq_regime = MCP_IRQ_PULL_LOW;
 
 	mcp->chip.direction_input = mcp23s08_direction_input;
 	mcp->chip.get = mcp23s08_get;
@@ -866,33 +879,52 @@  static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
 	mcp->irq_controller =
 		device_property_read_bool(dev, "interrupt-controller");
 	if (mcp->irq && mcp->irq_controller) {
-		mcp->irq_active_high =
-			device_property_read_bool(dev,
-					      "microchip,irq-active-high");
+		bool irq_active_high = device_property_read_bool(dev,
+						"microchip,irq-active-high");
+		bool irq_open_drain = device_property_read_bool(dev,
+						"microchip,irq-open-drain");
+		if (irq_active_high && irq_open_drain) {
+			dev_err(dev, "microchip,irq-open-drain and "
+				"microchip,irq-active-high are mutually exclusive\n");
+			ret = -EINVAL;
+			goto fail;
+		} else if (irq_active_high) {
+			mcp->irq_regime = MCP_IRQ_PUSH_UP;
+		} else if (irq_open_drain) {
+			mcp->irq_regime = MCP_IRQ_OPEN_DRAIN;
+		}
 
 		mirror = device_property_read_bool(dev, "microchip,irq-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);
-		if (mcp->irq_active_high)
-			status |= IOCON_INTPOL | (IOCON_INTPOL << 8);
-		else
-			status &= ~(IOCON_INTPOL | (IOCON_INTPOL << 8));
+	/* mcp23s17 has IOCON twice, make sure they are in sync */
+	status &= ~(IOCON_SEQOP | (IOCON_SEQOP << 8));
+	status |= IOCON_HAEN | (IOCON_HAEN << 8);
 
-		if (mirror)
-			status |= IOCON_MIRROR | (IOCON_MIRROR << 8);
+	switch (mcp->irq_regime) {
+	case MCP_IRQ_PUSH_UP:
+		status |= IOCON_INTPOL | (IOCON_INTPOL << 8);
+		status &= ~(IOCON_ODR | (IOCON_ODR << 8));
+		break;
+	case MCP_IRQ_PULL_LOW:
+		status &= ~(IOCON_INTPOL | (IOCON_INTPOL << 8));
+		status &= ~(IOCON_ODR | (IOCON_ODR << 8));
+		break;
+	case MCP_IRQ_OPEN_DRAIN:
+		/* ODR overrides INTPOL */
+		status |= IOCON_ODR | (IOCON_ODR << 8);
+		break;
+	}
 
-		if (type == MCP_TYPE_S18 || type == MCP_TYPE_018)
-			status |= IOCON_INTCC | (IOCON_INTCC << 8);
+	if (mirror)
+		status |= IOCON_MIRROR | (IOCON_MIRROR << 8);
 
-		ret = mcp_write(mcp, MCP_IOCON, status);
-		if (ret < 0)
-			goto fail;
-	}
+	if (type == MCP_TYPE_S18 || type == MCP_TYPE_018)
+		status |= IOCON_INTCC | (IOCON_INTCC << 8);
+
+	ret = mcp_write(mcp, MCP_IOCON, status);
+	if (ret < 0)
+		goto fail;
 
 	ret = devm_gpiochip_add_data(dev, &mcp->chip, mcp);
 	if (ret < 0)