Re: [PATCH v2] i2c: add support for microchip fpga i2c controllers

[<Conor.Dooley@microchip.com>]

On 04/05/2022 10:16, Hillf Danton wrote:
> [You don't often get email from hdanton@sina.com. 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.