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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171013161700.2anaqwthq4rnco6f@dell>
Date:   Fri, 13 Oct 2017 17:17:00 +0100
From:   Lee Jones <lee.jones@...aro.org>
To:     Andrey Smirnov <andrew.smirnov@...il.com>
Cc:     Johan Hovold <johan@...nel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Chris Healy <cphealy@...il.com>,
        Lucas Stach <l.stach@...gutronix.de>,
        Nikita Yushchenko <nikita.yoush@...entembedded.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Pavel Machek <pavel@....cz>
Subject: Re: [RESEND PATCH v7 1/1] platform: Add driver for RAVE Supervisory
 Processor

On Fri, 13 Oct 2017, Andrey Smirnov wrote:

> On Fri, Oct 13, 2017 at 12:27 AM, Johan Hovold <johan@...nel.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@...r.kernel.org
> >> Cc: cphealy@...il.com
> >> Cc: Lucas Stach <l.stach@...gutronix.de>
> >> Cc: Nikita Yushchenko <nikita.yoush@...entembedded.com>
> >> Cc: Lee Jones <lee.jones@...aro.org>
> >> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> >> Cc: Pavel Machek <pavel@....cz>
> >> Tested-by: Chris Healy <cphealy@...il.com>
> >> Reviewed-by: Andy Shevchenko <andy.shevchenko@...il.com>
> >> Signed-off-by: Andrey Smirnov <andrew.smirnov@...il.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