All of lore.kernel.org
 help / color / mirror / Atom feed
From: Wolfram Sang <wsa@the-dreams.de>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Tali Perry <tali.perry1@gmail.com>,
	ofery@google.com, brendanhiggins@google.com,
	avifishman70@gmail.com, tmaimon77@gmail.com, kfting@nuvoton.com,
	venture@google.com, yuenn@google.com, benjaminfair@google.com,
	robh+dt@kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-i2c@vger.kernel.org, openbmc@lists.ozlabs.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v12 2/3] i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver
Date: Thu, 21 May 2020 16:31:00 +0200	[thread overview]
Message-ID: <20200521143100.GA16812@ninjato> (raw)
In-Reply-To: <20200521142340.GM1634618@smile.fi.intel.com>

[-- Attachment #1: Type: text/plain, Size: 1136 bytes --]

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!

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

WARNING: multiple messages have this Message-ID (diff)
From: Wolfram Sang <wsa@the-dreams.de>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	tmaimon77@gmail.com, yuenn@google.com, avifishman70@gmail.com,
	venture@google.com, openbmc@lists.ozlabs.org,
	brendanhiggins@google.com, ofery@google.com,
	Tali Perry <tali.perry1@gmail.com>,
	kfting@nuvoton.com, robh+dt@kernel.org,
	linux-i2c@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	benjaminfair@google.com
Subject: Re: [PATCH v12 2/3] i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver
Date: Thu, 21 May 2020 16:31:00 +0200	[thread overview]
Message-ID: <20200521143100.GA16812@ninjato> (raw)
In-Reply-To: <20200521142340.GM1634618@smile.fi.intel.com>


[-- Attachment #1.1: Type: text/plain, Size: 1136 bytes --]

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!

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


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2020-05-21 14:31 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-21 11:09 [PATCH v12 0/3] i2c: npcm7xx: add NPCM i2c controller driver Tali Perry
2020-05-21 11:09 ` Tali Perry
2020-05-21 11:09 ` [PATCH v12 1/3] dt-bindings: i2c: npcm7xx: add NPCM I2C controller Tali Perry
2020-05-21 11:09   ` Tali Perry
2020-05-21 19:58   ` robh
2020-05-21 19:58     ` robh
2020-05-21 11:09 ` [PATCH v12 2/3] i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver Tali Perry
2020-05-21 11:09   ` Tali Perry
2020-05-21 14:23   ` Andy Shevchenko
2020-05-21 14:23     ` Andy Shevchenko
2020-05-21 14:31     ` Wolfram Sang [this message]
2020-05-21 14:31       ` Wolfram Sang
2020-05-21 14:45       ` Tali Perry
2020-05-21 14:45         ` Tali Perry
2020-05-21 14:53         ` Andy Shevchenko
2020-05-21 14:53           ` Andy Shevchenko
2020-05-21 20:37           ` Wolfram Sang
2020-05-21 20:37             ` Wolfram Sang
2020-05-21 20:47             ` Tali Perry
2020-05-21 20:47               ` Tali Perry
2020-05-21 21:21       ` Wolfram Sang
2020-05-21 21:21         ` Wolfram Sang
2020-05-22 14:45   ` Wolfram Sang
2020-05-22 14:45     ` Wolfram Sang
2020-05-21 11:09 ` [PATCH v12 3/3] i2c: npcm7xx: Add support for slave mode for Nuvoton Tali Perry
2020-05-21 11:09   ` Tali Perry
2020-05-21 14:36   ` Andy Shevchenko
2020-05-21 14:36     ` Andy Shevchenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200521143100.GA16812@ninjato \
    --to=wsa@the-dreams.de \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=avifishman70@gmail.com \
    --cc=benjaminfair@google.com \
    --cc=brendanhiggins@google.com \
    --cc=devicetree@vger.kernel.org \
    --cc=kfting@nuvoton.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-i2c@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=ofery@google.com \
    --cc=openbmc@lists.ozlabs.org \
    --cc=robh+dt@kernel.org \
    --cc=tali.perry1@gmail.com \
    --cc=tmaimon77@gmail.com \
    --cc=venture@google.com \
    --cc=yuenn@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.