Re: [PATCH v3 2/2] i2c: aspeed: Acknowledge Tx done with and without ACK irq late

Andi Shyti
2023-12-09T20:45:02Z

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@kernel.org>

Thanks,
Andi