Re: [RESEND PATCH v7 1/1] platform: Add driver for RAVE Supervisory Processor

[Lee Jones <lee.jones@linaro.org>]

On Fri, 13 Oct 2017, Andrey Smirnov wrote:

> On Fri, Oct 13, 2017 at 9:17 AM, Lee Jones <lee.jones@linaro.org> wrote:
> > On Fri, 13 Oct 2017, Andrey Smirnov wrote:
> >
> >> On Fri, Oct 13, 2017 at 12:27 AM, Johan Hovold <johan@kernel.org> 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: linux-kernel@vger.kernel.org
> >> >> Cc: cphealy@gmail.com
> >> >> Cc: Lucas Stach <l.stach@pengutronix.de>
> >> >> Cc: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
> >> >> Cc: Lee Jones <lee.jones@linaro.org>
> >> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> >> Cc: Pavel Machek <pavel@ucw.cz>
> >> >> Tested-by: Chris Healy <cphealy@gmail.com>
> >> >> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> >> >> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> >> >> ---
> >> >>  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.

Okay, it sounds like this might fit into MFD after all.

Sorry for the fuss.

Do you want me to review the first submission, or will you submit
again?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog