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 devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
