Improvements for MCP23Sxx chips

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 <[email protected]>

---
  .../bindings/pinctrl/pinctrl-mcp23s08.txt          |  4 ++
  drivers/pinctrl/pinctrl-mcp23s08.c                 | 82 +++++++++++++++-------
  2 files changed, 61 insertions(+), 25 deletions(-)

Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt +0 -0

index 9c451c20dda4..92aa91027c68 100644

Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt +0 -0

  Example I2C (with interrupt):
  gpiom1: gpio@20 {

drivers/pinctrl/pinctrl-mcp23s08.c +8 -4

index c017860619d6..186101c575e3 100644
@@ -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) {

drivers/pinctrl/pinctrl-mcp23s08.c +40 -21

  	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)

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

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.

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 4 lines

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.

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 <[email protected]>

Quoted 27 lines · Jan Kundrát

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).

Quoted 1 line

Signed-off-by Jan Kundrát <[email protected]>

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).

The SPI version of this chip allows several devices to be present on the same SPI bus via a local address. If this is in action and if the kernel has debugfs, however, the code attempts to create duplicate entries for the regmap's debugfs:

mcp23s08 spi1.1: Failed to create debugfs directory

This patch simply assigns a local name matching the device logical address to the struct regmap_config.

No changes are needed for MCP23S18 because that device does not support any logical addressing. Similarly, I2C devices do not need any action, either, because they are already different in their I2C address.

A similar problem is present for the pinctrl debugfs instance, but that one is not addressed by this patch.

Signed-off-by Jan Kundrát <[email protected]>

When several devices are sharing one hardware SPI CS, there is no visual clue in lsgpio or in /sys/kernel/debug/gpio about which one is which one. Stuff depends on the enumeration order, and therefore lower chip addresses always go first, but that's just an implementation detail. This change includes the device-specific address in the debug output:

gpiochip4: GPIOs 464-479, parent: spi/spi1.1, mcp23s17.2, can sleep:
gpiochip3: GPIOs 480-495, parent: spi/spi1.1, mcp23s17.1, can sleep:

Signed-off-by Jan Kundrát <[email protected]>

This is a bit more involved because the pinctrl core so far always assumed that one device (with a unique dev_name) only contains a single pinctrl thing. This is not true for the mcp23s08 driver for chips connected over SPI. They have a "logical address" which means that several chips can share one physical CS signal.

A downside of this patch are some possibly ugly names for the debugfs entries, such as "spi1.1-mcp23xxx-pinctrl.2", etc.

Signed-off-by Jan Kundrát <[email protected]>