On Thu, May 21, 2020 at 05:45:03PM +0300, Tali Perry wrote:

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

quoted

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 :)

quoted

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.

quoted

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.

Ah, then there are these options I have in mind (Wolfram, FYI as well!):
1) push with debugfs as a temporary solution and convert to devlink health protocol [1];
2) drop it and develop devlink_health solution;
3) push debugfs and wait if I²C will gain devlink health support

[1]: https://www.kernel.org/doc/html/latest/networking/devlink/devlink-health.html

-- 
With Best Regards,
Andy Shevchenko



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