Re: [PATCH v4 7/9] i2c: npcm: Handle spurious interrupts

[Tyrone Ting <warp5tw@gmail.com>]

Hi Andy:

Thank you for your comments and they will be addressed.

Andy Shevchenko <andriy.shevchenko@linux.intel.com> 於 2022年5月10日 週二 下午6:15寫道：
>
> On Tue, May 10, 2022 at 05:16:52PM +0800, Tyrone Ting wrote:
> > From: Tali Perry <tali.perry1@gmail.com>
> >
> > On some platforms in rare cases (1 to 100,000 transactions),
> > the i2c gets a spurious interrupt which means that we enter an interrupt
> > but in the interrupt handler we don't find any status bit that points to
> > the reason we got this interrupt.
> >
> > This may be a case of a rare HW issue or signal integrity issue that is
> > still under investigation.
> >
> > In order to overcome this we are doing the following:
> > 1. Disable incoming interrupts in master mode only when slave mode is not
> >    enabled.
> > 2. Clear end of busy (EOB) after every interrupt.
> > 3. Clear other status bits (just in case since we found them cleared)
> > 4. Return correct status during the interrupt that will finish the
> >    transaction.
> >
> > On next xmit transaction if the bus is still busy the master will issue a
> > recovery process before issuing the new transaction.
>
> ...
>
> > +     /* clear status bits for spurious interrupts */
>
> Clear
>
> ...
>
> > +     /*
> > +      * if irq is not one of the above, make sure EOB is disabled and all
>
> If
>
> > +      * status bits are cleared.
> > +      */
>
> ...
>
> > +             /* verify no status bits are still set after bus is released */
>
> Verify
>
> ...
>
> > +     /* check HW is OK: SDA and SCL should be high at this point. */
>
> Check
>
> ...
>
> > +     if ((npcm_i2c_get_SDA(&bus->adap) == 0) ||
> > +         (npcm_i2c_get_SCL(&bus->adap) == 0)) {
>
> This fits one line
>
> > +             dev_err(bus->dev, "I2C%d init fail: lines are low", bus->num);
> > +             dev_err(bus->dev, "SDA=%d SCL=%d", npcm_i2c_get_SDA(&bus->adap),
> > +                     npcm_i2c_get_SCL(&bus->adap));
>
> No '\n' at the end of each?!
>
> > +             return -ENXIO;
> > +     }
>
> ...
>
> > +     /* clear status bits for spurious interrupts */
>
> Clear
>
> ...
>
> > +     /* after any xfer, successful or not, stall and EOB must be disabled */
>
> After
>
> ...
>
> Maybe you chose the small letter for one-liner comments, but I see even in the
> original code the inconsistent style. Please, add an explanation to the cover
> letter and follow it, assuming you add the patch at the end of the series that
> makes comment style consistent (for the one-liners, for the multi-line comments
> we have a clear understanding about the style).
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Best Regards,
Tyrone