On Fri, Oct 13, 2017 at 9:17 AM, Lee Jones <[email protected]> wrote: > On Fri, 13 Oct 2017, Andrey Smirnov wrote: > >> On Fri, Oct 13, 2017 at 12:27 AM, Johan Hovold <[email protected]> wrote: >> > On Thu, Oct 12, 2017 at 11:13:21PM -0700, Andrey Smirnov wrote: >> >> Add a driver for RAVE Supervisory Processor, an MCU implementing >> >> varoius bits of housekeeping functionality (watchdoging, backlight >> >> control, LED control, etc) on RAVE family of products by Zodiac >> >> Inflight Innovations. >> >> >> >> This driver implementes core MFD/serdev device as well as >> >> communication subroutines necessary for commanding the device. >> > >> > I only skimmed this, but have a few of comments below. >> > >> >> Cc: [email protected] >> >> Cc: [email protected] >> >> Cc: Lucas Stach <[email protected]> >> >> Cc: Nikita Yushchenko <[email protected]> >> >> Cc: Lee Jones <[email protected]> >> >> Cc: Greg Kroah-Hartman <[email protected]> >> >> Cc: Pavel Machek <[email protected]> >> >> Tested-by: Chris Healy <[email protected]> >> >> Reviewed-by: Andy Shevchenko <[email protected]> >> >> Signed-off-by: Andrey Smirnov <[email protected]> >> >> --- >> >> drivers/platform/Kconfig | 2 + >> >> drivers/platform/Makefile | 1 + >> >> drivers/platform/rave/Kconfig | 26 ++ >> >> drivers/platform/rave/Makefile | 1 + >> >> drivers/platform/rave/rave-sp.c | 677 >> >> ++++++++++++++++++++++++++++++++++++++++ >> > >> > First of all, why do these live in drivers/platform and why don't use >> > the mfd subsystem to implement this driver (instead of rolling your own >> > mfd-implementation)? >> > >> >> Because when I submitted this driver to MFD Lee Jones told me that it >> didn't belong to that subsystem and should be added to the kernel in >> "drivers/platform". > > When I first reviewed this driver, it looked far too complex to be an > MFD driver.
With exception of removed sysfs/debugfs entries (and minor code changes) the driver is still the same as when you reviewed it. > Most of the code doesn't deal with what I'd expect to be > handled in MFD. Why do you have ~600 lines of protocol handling? > Because there are three ever-so-slightly different ICDs that the firmware of the device implements depending on the variant/generation of the overall platform it is a part of. Thanks, Andrey Smirnov
