Re: [PATCH v10 2/3] i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver

[Tali Perry]

On Mon, May 11, 2020 at 12:18 PM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>
> On Sun, May 10, 2020 at 01:23:29PM +0300, Tali Perry wrote:
> > Add Nuvoton NPCM BMC I2C controller driver.
>
> Some cosmetic changes needs to be done.
>

Thanks for the review and the comments.
Will fix all, have a few questions (below)

> ...
>
> > +/*
> > + * Nuvoton NPCM7xx I2C Controller driver
> > + *
> > + * Copyright (C) 2020 Nuvoton Technologies tali.perry@xxxxxxxxxxx
> > + */
>
> So, entire file has C99 comment style, but this and few other places.
> Any reason of inconsistency?
>
> ...
>
> > +#if IS_ENABLED(CONFIG_DEBUG_FS)
>
> Why?

We wanted to add an optional feature to track i2c slave status.
the NPCM has 16 channels handling multiple devices each. Some of the devices
are polled periodically, and might power down.
The user wanted to implement a health monitoring option
to occasionally check the status of the buses (how many timeouts, recovery etc.)
This feature is optional and depends on CONFIG_DEBUG_FS The counters are exposed
to user through the file system.

....

> ...
>
> > +#define I2C_NUM_OF_ADDR 10
>
> Is it 10-bit address support or what?
>

No, the NPCM has an option to respond to multiple slave addresses
(10 own slave addresses)



...

> > +     // Repeat the following sequence until SDA is released
> > +     do {
> > +             // Issue a single SCL toggle
> > +             iowrite8(NPCM_I2CCST_TGSCL, bus->reg + NPCM_I2CCST);
> > +             udelay(20);
> > +             // If SDA line is inactive (high), stop
> > +             if (npcm_i2c_get_SDA(_adap)) {
> > +                     done = true;
> > +                     status = 0;
> > +             }
> > +     } while (!done && iter--);
>
> readx_poll_timeout() ?

Not exactly, readx_poll_timeout includes only a read operation, here there is a
write in the middle. (iowrite8)


>
> ...
>


...


Thanks!
Tali Perry
Nuvoton Technologies