Re: [PATCH v4 3/3] pinctrl: mcp23s08: add open drain configuration for irq output

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Thanks for looking into this.

Is there a reason why the DT documentation is updated in a separate commit?

 	if ((status & IOCON_SEQOP) || !(status & IOCON_HAEN) || mirror ||
-	     mcp->irq_active_high) {
+	     mcp->irq_active_high || open_drain) {

This `if` is getting quite complex. I don't think that saving one SPI write transaction is worth the effort here. What about just setting the chip to a known-good state unconditionally by removing that `if`?

The condition also looks wrong to me (as-is in the kernel now). If the chip is somehow configured to use active-high IRQ and the DT does not ask for either active-high or open-drain IRQ, then the reconfiguration gets skipped and the chip runs with wrong settings.

 		/* mcp23s17 has IOCON twice, make sure they are in sync */
 		status &= ~(IOCON_SEQOP | (IOCON_SEQOP << 8));
 		status |= IOCON_HAEN | (IOCON_HAEN << 8);
@@ -898,6 +900,9 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
 		if (mirror)
 			status |= IOCON_MIRROR | (IOCON_MIRROR << 8);
+ if (open_drain)
+			status |= IOCON_ODR | (IOCON_ODR << 8);
+
 		if (type == MCP_TYPE_S18 || type == MCP_TYPE_018)
 			status |= IOCON_INTCC | (IOCON_INTCC << 8);

I think that it would be better to explicitly error out if irq_active_high and open_drain are both set. When I sent my attempt at this, I did it like this:

+		if (irq_active_high && irq_open_drain) {
+			dev_err(dev, "microchip,irq-open-drain and "
+				"drive-active-high are mutually exclusive\n");
+			ret = -EINVAL;
+			goto fail;
+		} else if (irq_active_high) {
+			...
+		} else if (irq_open_drain) {
+			...
+		}

But anyway, even as-is, the patch is a net improvement, so:

Reviewed-by: Jan Kundrát <jan.kundrat@xxxxxxxxx>

With kind regards,
Jan
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux SPI]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]     [Yosemite Campsites]

  Powered by Linux