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

From: Lee Jones <lee.jones@linaro.org>
To: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Johan Hovold <johan@kernel.org>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	Chris Healy <cphealy@gmail.com>,
	Lucas Stach <l.stach@pengutronix.de>,
	Nikita Yushchenko <nikita.yoush@cogentembedded.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Pavel Machek <pavel@ucw.cz>
Subject: Re: [RESEND PATCH v7 1/1] platform: Add driver for RAVE Supervisory Processor
Date: Fri, 13 Oct 2017 17:17:00 +0100	[thread overview]
Message-ID: <20171013161700.2anaqwthq4rnco6f@dell> (raw)
In-Reply-To: <CAHQ1cqF16zGQDOzuAGLFaGN+2ZEsf=OQXy8iAVXUjBx=Uq_cyg@mail.gmail.com>

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.  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?

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