Hi Yicong, [...] > >>>> @@ -330,6 +330,14 @@ static irqreturn_t hisi_i2c_irq(int irq, void *context) > >>>> struct hisi_i2c_controller *ctlr = context; > >>>> u32 int_stat; > >>>> > >>>> + /* > >>>> + * Don't handle the interrupt if cltr->completion is NULL. We may > >>>> + * reach here because the interrupt is spurious or the transfer is > >>>> + * started by another port rather than us. > >>>> + */ > >>>> + if (!ctlr->completion) > >>>> + return IRQ_NONE; > >>> > >>> Is this the place you should really check for completion being > >>> NULL? By reading the code I don't exclude that completion at this > >>> stage might be NULL. > >>> > >>> Can it be that the real fix is this one instead: > >> > >> Maybe not. If we handle the case as late as below, we'll operate the hardware > >> which should be handled by the firmware which start the transfer. So we check > >> it as early as possible. > > > > But if i2c_master_xfer() is not called and we receive an irq, > > most probably ctrl->completion is NULL. Right? Can this happen? > > > > Yes, this is the case. > > > I can't really tell the sequence for enabling/disabling the > > interrupt in this device. They might happen in > > hisi_i2c_start_xfer() for enabling and in hisi_i2c_xfer_msg() for > > desabling at the last message; which makes the scenario above a > > bit difficult, indeed. > > > > The driver will keep the interrupt disabled if no transfer in progress. > But since the transfer is driven by the interrupt so if the firmware > start the transfer it will enable the interrupt. In such case the driver > will receive an interrupt on the Tx FIFO empty, etc and since the > transfer is not started by the driver ctlr->completion is not > initialized. OK... makes sense... Reviewed-by: Andi Shyti <andi.shyti@xxxxxxxxxx> Thanks! Andi
