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

[Andrey Smirnov]

On Fri, Oct 13, 2017 at 9:17 AM, Lee Jones <lee.jones@xxxxxxxxxx> wrote:
> On Fri, 13 Oct 2017, Andrey Smirnov wrote:
>
>> On Fri, Oct 13, 2017 at 12:27 AM, Johan Hovold <johan@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx
>> >> Cc: cphealy@xxxxxxxxx
>> >> Cc: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
>> >> Cc: Nikita Yushchenko <nikita.yoush@xxxxxxxxxxxxxxxxxx>
>> >> Cc: Lee Jones <lee.jones@xxxxxxxxxx>
>> >> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>> >> Cc: Pavel Machek <pavel@xxxxxx>
>> >> Tested-by: Chris Healy <cphealy@xxxxxxxxx>
>> >> Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
>> >> Signed-off-by: Andrey Smirnov <andrew.smirnov@xxxxxxxxx>
>> >> ---
>> >>  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