linux-kernel - Re: [PATCH v12 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: <20200521143100.GA16812@ninjato>
Date:   Thu, 21 May 2020 16:31:00 +0200
From:   Wolfram Sang <wsa@...-dreams.de>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     Tali Perry <tali.perry1@...il.com>, ofery@...gle.com,
        brendanhiggins@...gle.com, avifishman70@...il.com,
        tmaimon77@...il.com, kfting@...oton.com, venture@...gle.com,
        yuenn@...gle.com, benjaminfair@...gle.com, robh+dt@...nel.org,
        linux-arm-kernel@...ts.infradead.org, linux-i2c@...r.kernel.org,
        openbmc@...ts.ozlabs.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v12 2/3] i2c: npcm7xx: Add Nuvoton NPCM I2C controller
 driver

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@...ux.intel.com>

Thanks, Andy, for all the review!

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 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.

> > +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.

Regards,

   Wolfram


Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)