On 04/05/2022 10:16, Hillf Danton wrote: > [You don't often get email from hdanton@xxxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification.] > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Mon, 21 Mar 2022 12:58:56 +0000 Conor Dooley wrote: >> + >> +static irqreturn_t mchp_corei2c_handle_isr(struct mchp_corei2c_dev *idev) >> +{ >> + u32 status = idev->isr_status; >> + u8 ctrl; >> + >> + if (!idev->buf) >> + return IRQ_NONE; >> + >> + switch (status) { >> + case STATUS_M_START_SENT: >> + case STATUS_M_REPEATED_START_SENT: >> + ctrl = readb(idev->base + CORE_I2C_CTRL); >> + ctrl &= ~CTRL_STA; >> + writeb(idev->addr, idev->base + CORE_I2C_DATA); >> + writeb(ctrl, idev->base + CORE_I2C_CTRL); >> + if (idev->msg_len <= 0) >> + goto finished; >> + break; >> + case STATUS_M_ARB_LOST: >> + idev->msg_err = -EAGAIN; >> + goto finished; >> + case STATUS_M_SLAW_ACK: >> + case STATUS_M_TX_DATA_ACK: >> + if (idev->msg_len > 0) >> + mchp_corei2c_fill_tx(idev); >> + else >> + goto last_byte; >> + break; >> + case STATUS_M_TX_DATA_NACK: >> + case STATUS_M_SLAR_NACK: >> + case STATUS_M_SLAW_NACK: >> + idev->msg_err = -ENXIO; >> + goto last_byte; >> + case STATUS_M_SLAR_ACK: >> + ctrl = readb(idev->base + CORE_I2C_CTRL); >> + if (idev->msg_len == 1u) { >> + ctrl &= ~CTRL_AA; >> + writeb(ctrl, idev->base + CORE_I2C_CTRL); >> + } else { >> + ctrl |= CTRL_AA; >> + writeb(ctrl, idev->base + CORE_I2C_CTRL); >> + } >> + if (idev->msg_len < 1u) >> + goto last_byte; >> + break; >> + case STATUS_M_RX_DATA_ACKED: >> + mchp_corei2c_empty_rx(idev); >> + break; >> + case STATUS_M_RX_DATA_NACKED: >> + mchp_corei2c_empty_rx(idev); >> + if (idev->msg_len == 0) >> + goto last_byte; >> + break; >> + default: >> + break; >> + } >> + >> + return IRQ_HANDLED; >> + >> +last_byte: >> + /* On the last byte to be transmitted, send STOP */ >> + mchp_corei2c_stop(idev); >> +finished: >> + complete(&idev->msg_complete); >> + return IRQ_HANDLED; >> +} >> + >> +static irqreturn_t mchp_corei2c_isr(int irq, void *_dev) >> +{ >> + struct mchp_corei2c_dev *idev = _dev; >> + irqreturn_t ret = IRQ_NONE; >> + u8 ctrl; >> + >> + ctrl = readb(idev->base + CORE_I2C_CTRL); >> + if (ctrl & CTRL_SI) { >> + idev->isr_status = readb(idev->base + CORE_I2C_STATUS); >> + ret = mchp_corei2c_handle_isr(idev); >> + } >> + >> + /* Clear the si flag */ >> + ctrl = readb(idev->base + CORE_I2C_CTRL); >> + ctrl &= ~CTRL_SI; >> + writeb(ctrl, idev->base + CORE_I2C_CTRL); >> + >> + return ret; >> +} >> + >> +static int mchp_corei2c_xfer_msg(struct mchp_corei2c_dev *idev, >> + struct i2c_msg *msg) >> +{ >> + u8 ctrl; >> + unsigned long time_left; >> + >> + if (msg->len == 0) >> + return -EINVAL; >> + >> + idev->addr = i2c_8bit_addr_from_msg(msg); >> + idev->msg_len = msg->len; >> + idev->buf = msg->buf; >> + idev->msg_err = 0; >> + idev->msg_read = (msg->flags & I2C_M_RD); >> + >> + reinit_completion(&idev->msg_complete); > > Would you specify why you need reinit completion? Is it likely to race > with the complete() above in mchp_corei2c_handle_isr()? Is it not fairly common in i2c drivers to use a reinit completion()/ wait_for_completion_timeout() in the xfer function with a complete() in the isr? I can certainly add a comment explaining why this is being done? Thanks, Conor.