On Thu, May 21, 2020 at 5:31 PM Wolfram Sang [off-list ref] wrote:

Hi Tali, Andy!

On Thu, May 21, 2020 at 05:23:40PM +0300, Andy Shevchenko wrote:

quoted

On Thu, May 21, 2020 at 02:09:09PM +0300, Tali Perry wrote:

quoted

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.

quoted

quoted

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

quoted

quoted

+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

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