Hi Quan,
[...]
- /* Ack all interrupts except for Rx done */
- writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE,
- bus->base + ASPEED_I2C_INTR_STS_REG);
+
+ /*
+ * Early acking of INTR_RX_DONE and INTR_TX_[ACK|NAK] would indicate HW to
+ * start receiving or sending new data, and this may cause a race condition
+ * as the irq handler has not yet handled these irqs but is being acked.
+ * Let's ack them late at the end of the irq handler when those are truly processed.
+ */
+ irq_ack_last = ASPEED_I2CD_INTR_RX_DONE | ASPEED_I2CD_INTR_TX_ACK | ASPEED_I2CD_INTR_TX_NAK;
+ writel(irq_received & ~irq_ack_last, bus->base + ASPEED_I2C_INTR_STS_REG);
I like Andrews suggestion of having irq_ack_last as a define that
is already negated, instead of negating it in the writel, which
makes it a bit difficult to read.
Besides, ack_last, as a name is not very meaningful, I'd rather
call it irq_ack_rx_tx (or something similar).
But I'm not going to block it for this, up to you if you want to
send a new version.
Reviewed-by: Andi Shyti <andi.shyti@xxxxxxxxxx>