Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200521110910.45518-1-tali.perry1@gmail.com> <20200521110910.45518-3-tali.perry1@gmail.com>
 <20200521142340.GM1634618@smile.fi.intel.com> <20200521143100.GA16812@ninjato>
In-Reply-To: <20200521143100.GA16812@ninjato>
From:   Tali Perry <tali.perry1@gmail.com>
Date:   Thu, 21 May 2020 17:45:03 +0300
Message-ID: <CAHb3i=vcVLWHjdiJoNZQrwJCqzszpOL7e9SAjqObsZCRH4ifwg@mail.gmail.com>
Subject: Re: [PATCH v12 2/3] i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver
To:     Wolfram Sang <wsa@the-dreams.de>
Cc:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Ofer Yehielli <ofery@google.com>,
        Brendan Higgins <brendanhiggins@google.com>,
        avifishman70@gmail.com, Tomer Maimon <tmaimon77@gmail.com>,
        kfting@nuvoton.com, Patrick Venture <venture@google.com>,
        Nancy Yuen <yuenn@google.com>,
        Benjamin Fair <benjaminfair@google.com>,
        Rob Herring <robh+dt@kernel.org>,
        linux-arm-kernel@lists.infradead.org, linux-i2c@vger.kernel.org,
        OpenBMC Maillist <openbmc@lists.ozlabs.org>,
        devicetree <devicetree@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, May 21, 2020 at 5:31 PM Wolfram Sang <wsa@the-dreams.de> wrote:
>
> Hi Tali, Andy!
>
> On Thu, May 21, 2020 at 05:23:40PM +0300, Andy Shevchenko wrote:
> > On Thu, May 21, 2020 at 02:09:09PM +0300, Tali Perry wrote:
> > > Add Nuvoton NPCM BMC I2C controller driver.
> >
> > Thanks. My comments below.
> > After addressing them, FWIW,
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> Thanks, Andy, for all the review!
>

Highly appreciate your time and patience for a newbie :)

> From a glimpse, this looks good to go. I will have a close look later
> today.
>
> > > +#ifdef CONFIG_DEBUG_FS
> >
> > Again, why is this here?
> >
> > Have you checked debugfs.h for !CONFIG_DEBUG_FS case?

I compiled both options. I removed the ifdef in most places, except in the
struct itself. Users that don't use the debugfs don't need this in the struct.

>
> I wondered also about DEBUG_FS entries. I can see their value when
> developing the driver. But since this is done now, do they really help a
> user to debug a difficult case? I am not sure, and then I wonder if we
> should have that code in upstream. I am open for discussion, though.

The user wanted to have health monitor implemented on top of the driver.
The user has 16 channels connected the multiple devices. All are operated
using various daemons in the system. Sometimes the slave devices are power down.
Therefor the user wanted to track the health status of the devices.

>
> > > +MODULE_VERSION("0.1.3");
> >
> > Module version is defined by kernel commit hash. But it's up to you and
> > subsystem maintainer to decide.
>
> Please drop it. I also think commit id's (or even kernel versions) are a
> more precise description.

will remove.

>
> Regards,
>
>    Wolfram
>

BR,
Tali