On Thu, May 21, 2020 at 05:45:03PM +0300, Tali Perry wrote: > On Thu, May 21, 2020 at 5:31 PM Wolfram Sang <wsa@xxxxxxxxxxxxx> wrote: > > 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@xxxxxxxxxxxxxxx> > > > > 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. 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
