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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHb3i=tERsM+gwmQN1+vjnML9o5NxRK=uBokEUsd-Ljyje4s3A@mail.gmail.com>
Date:   Mon, 11 May 2020 14:28:50 +0300
From:   Tali Perry <tali.perry1@...il.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     ofery@...gle.com, Brendan Higgins <brendanhiggins@...gle.com>,
        avifishman70@...il.com, Tomer Maimon <tmaimon77@...il.com>,
        kfting@...oton.com, Patrick Venture <venture@...gle.com>,
        Nancy Yuen <yuenn@...gle.com>,
        Benjamin Fair <benjaminfair@...gle.com>,
        Rob Herring <robh+dt@...nel.org>,
        Wolfram Sang <wsa@...-dreams.de>,
        linux-arm-kernel@...ts.infradead.org, linux-i2c@...r.kernel.org,
        OpenBMC Maillist <openbmc@...ts.ozlabs.org>,
        devicetree <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v10 2/3] i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver

On Mon, May 11, 2020 at 12:18 PM Andy Shevchenko
<andriy.shevchenko@...ux.intel.com> 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@...oton.com
> > + */
>
> 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