Re: [PATCH v2] mfd: Add driver for RAVE Supervisory Processor

[Lee Jones]

On Fri, 23 Jun 2017, Andrey Smirnov wrote:

> On Tue, Jun 20, 2017 at 1:19 AM, Lee Jones <lee.jones@xxxxxxxxxx> wrote:
> > On Mon, 12 Jun 2017, 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.
> >>
> >> Cc: cphealy@xxxxxxxxx
> >> Cc: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
> >> Cc: Nikita Yushchenko <nikita.yoush@xxxxxxxxxxxxxxxxxx>
> >> Cc: Rob Herring <robh+dt@xxxxxxxxxx>
> >> Cc: Mark Rutland <mark.rutland@xxxxxxx>
> >> Cc: devicetree@xxxxxxxxxxxxxxx
> >> Cc: linux-kernel@xxxxxxxxxxxxxxx
> >> Signed-off-by: Andrey Smirnov <andrew.smirnov@xxxxxxxxx>
> >> ---
> >>
> >> Changes since [v1]:
> >>
> >>  - Fix MODULE_LICENSE to specify "GPL v2"
> >>
> >>  - Fix a bug in rave_sp_get_status()
> >>
> >>  - Use %zu to fix warning repored by kbuild test robot
> >>
> >>  - Remove "status" properties from examples zii,rave-sp.txt as well as
> >>    clarify the fact that device node is expected to be a child of a
> >>    serial device node
> >>
> >> NOTE:
> >>
> >>  * The driver for "zii,rave-sp-watchdog" exists, but I haven't
> >>    submitted it yet, becuase I wanted to make sure that API exposed by
> >>    this MFD is acceptable and doesn't need drastic changes
> >>
> >>  * This driver is dependent on crc_ccitt_false() introduced in
> >>    2da9378d531f8cc6670c7497f20d936b706ab80b in 'linux-next'
> >>
> >>
> >> [v1] lkml.kernel.org/r/20170606180643.14258-1-andrew.smirnov@xxxxxxxxx
> >>
> >>
> >>  .../devicetree/bindings/mfd/zii,rave-sp.txt        |   36 +
> >
> > This should be a separate patch.
> 
> OK, will fix in v3.
> 
> >
> >>  drivers/mfd/Kconfig                                |    9 +
> >>  drivers/mfd/Makefile                               |    1 +
> >>  drivers/mfd/rave-sp.c                              | 1009 ++++++++++++++++++++
> >>  include/linux/rave-sp.h                            |   54 ++
> >>  5 files changed, 1109 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/mfd/zii,rave-sp.txt
> >>  create mode 100644 drivers/mfd/rave-sp.c
> >>  create mode 100644 include/linux/rave-sp.h
> >>
> >> diff --git a/Documentation/devicetree/bindings/mfd/zii,rave-sp.txt b/Documentation/devicetree/bindings/mfd/zii,rave-sp.txt
> >> new file mode 100644
> >> index 0000000..903c889
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/mfd/zii,rave-sp.txt
> >> @@ -0,0 +1,36 @@
> >> +Zodiac Inflight Innovations RAVE Supervisory Processor
> >> +
> >> +RAVE Supervisory Processor communicates with SoC over UART and is
> >> +presented to the kernel as a "serdev". Given that it is expected that
> >
> > Please revisit this sentence.
> >
> 
> Just to make sure I understand you correctly we are talking about
> "Given that it is expected that", correct?

Right.

> >> +its device-tree node is specified as a child of a node corresponding
> >> +to UART controller used for communication.
> >
> > This is unusual.  I believe we sometimes do this with I2C devices, but
> > we don't normally have the supported comms medium as the parent.
> >
> 
> I don't think there's any other way to specify a "serdev" device. It
> has to be a child of corresponding serial controller. In that aspect
> it is no different from I2C device.
> 
> >> +Required parent device properties:
> >> +
> >> + - compatible: Should be one of:
> >> +     - "zii,rave-sp-niu"
> >> +     - "zii,rave-sp-mezz"
> >> +     - "zii,rave-sp-esb"
> >> +     - "zii,rave-sp-rdu1"
> >> +     - "zii,rave-sp-rdu2"
> >> +
> >> + - current-speed: Should be set to baud rate SP device is using
> >> +
> >> +RAVE SP consists of the following sub-devices:
> >> +
> >> +Device                                Description
> >> +------                                -----------
> >> +rave-sp-wdt                  : Watchdog
> >
> > What else does it contain.
> >
> > MFDs always have more than one device.
> >
> 
> It also exposes EEPROM, sensor/hwmon device, backlight driver, LED
> driver and an input device.

So why aren't they listed?

> >> +Example of usage:
> >> +
> >> +     rdu {
> >
> > What is an RDU?
> >
> 
> RDU stands for "removable display unit".

And what is one of those?

> >> +             compatible = "zii,rave-sp-rdu2";
> >> +             current-speed = <1000000>;
> >> +
> >> +             watchdog {
> >> +                     compatible = "zii,rave-sp-watchdog";
> >> +             };
> >> +     };
> >> +
> >> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> >> index 3eb5c93..6cab311 100644
> >> --- a/drivers/mfd/Kconfig
> >> +++ b/drivers/mfd/Kconfig
> >> @@ -867,6 +867,15 @@ config MFD_SPMI_PMIC
> >>         Say M here if you want to include support for the SPMI PMIC
> >>         series as a module.  The module will be called "qcom-spmi-pmic".
> >>
> >> +config MFD_RAVE_SP
> >> +     tristate "RAVE SP MCU core driver"
> >> +     select MFD_CORE
> >> +     select SERIAL_DEV_BUS
> >> +     select CRC_CCITT
> >> +     help
> >> +       Select this to get support for the RAVE Supervisory
> >> +       Processor driver
> >
> > Missing '.'.
> 
> OK, will fix in v3.
> 
> >
> >>  config MFD_RDC321X
> >>       tristate "RDC R-321x southbridge"
> >>       select MFD_CORE
> >> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> >> index c16bf1e..bc3df0a 100644
> >> --- a/drivers/mfd/Makefile
> >> +++ b/drivers/mfd/Makefile
> >> @@ -221,3 +221,4 @@ obj-$(CONFIG_MFD_SUN4I_GPADC)     += sun4i-gpadc.o
> >>
> >>  obj-$(CONFIG_MFD_STM32_TIMERS)       += stm32-timers.o
> >>  obj-$(CONFIG_MFD_MXS_LRADC)     += mxs-lradc.o
> >> +obj-$(CONFIG_MFD_RAVE_SP)    += rave-sp.o
> >> diff --git a/drivers/mfd/rave-sp.c b/drivers/mfd/rave-sp.c
> >> new file mode 100644
> >> index 0000000..41cdbd2
> >> --- /dev/null
> >> +++ b/drivers/mfd/rave-sp.c
> >> @@ -0,0 +1,1009 @@
> >> +/*
> >> + * rave-sp.c - Multifunction core driver for Zodiac Inflight Innovations
> >
> > Drop the filename please, they have a habit of becoming incorrect
> >
> 
> OK, will do in v3.
> 
> >> + * SP MCU that is connected via dedicated UART port
> >> + *
> >> + * Copyright (C) 2017 Zodiac Inflight Innovations
> >> + *
> >> + * This program is free software; you can redistribute it and/or
> >> + * modify it under the terms of the GNU General Public License as
> >> + * published by the Free Software Foundation; either version 2 of
> >> + * the License, or (at your option) any later version.
> >> + *
> >> + * This program is distributed in the hope that it will be useful,
> >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> >> + * GNU General Public License for more details.
> >> + *
> >> + * You should have received a copy of the GNU General Public License
> >> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> >
> > Are you able to use the short version?
> >
> 
> I don't know what you mean by "short version". Is it short version of
> GPL notice above?

Yes, it's a few lines and implies the same thing.

> >> + */
> >> +
> >> +/*
> >> + * UART protocol using following entities:
> >> + *  - message to MCU => ACK response
> >> + *  - event from MCU => event ACK
> >> + *
> >> + * Frame structure:
> >> + * <STX> <DATA> <CHECKSUM> <ETX>
> >> + * Where:
> >> + * - STX - is start of transmission character
> >> + * - ETX - end of transmission
> >> + * - DATA - payload
> >> + * - CHECKSUM - checksum calculated on <DATA>
> >> + *
> >> + * If <DATA> or <CHECKSUM> contain one of control characters, then it is
> >> + * escaped using <DLE> control code. Added <DLE> does not participate in
> >> + * checksum calculation.
> >> + */
> >> +
> >> +/* #define DEBUG */
> >
> > If you're going to do this, add a comment to the tune of:
> >
> > "Uncomment this to provide driver specific debugging."
> >
> 
> That's a leftover I missed. Will remove in v3.
> 
> >> +#include <linux/atomic.h>
> >> +#include <linux/crc-ccitt.h>
> >> +#include <linux/delay.h>
> >> +#include <linux/export.h>
> >> +#include <linux/init.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/module.h>
> >> +#include <linux/of.h>
> >> +#include <linux/of_device.h>
> >> +#include <linux/sched.h>
> >> +#include <linux/serdev.h>
> >> +#include <linux/rave-sp.h>
> >> +
> >> +enum {
> >> +     STX = 0x02,
> >> +     ETX = 0x03,
> >> +     DLE = 0x10,
> >> +};
> >
> > We usually prefix defines/enums.
> >
> 
> OK, will do in v3.
> 
> > Also these are not very human readable.  Either improve the
> > nomenclature or add a comment/header.
> >
> 
> OK, will add comments in v3.
> 
> >> +enum {
> >> +     RAVE_SP_BOOT_SOURCE_GET = 0,
> >> +     RAVE_SP_BOOT_SOURCE_SET = 1,
> >> +
> >> +     RAVE_SP_MAX_DATA_SIZE   = 64,
> >> +     RAVE_SP_CHECKSUM_SIZE   = 2, /* Worst case scenariou on RDU2 */
> >> +     /*
> >> +      * We don't store STX, ETX and unescaped bytes, so Rx is only
> >> +      * DATA + CSUM
> >> +      */
> >> +     RAVE_SP_RX_BUFFER_SIZE  = RAVE_SP_MAX_DATA_SIZE +
> >> +                               RAVE_SP_CHECKSUM_SIZE,
> >> +     RAVE_SP_STX_ETX_SIZE    = 2,
> >> +     /*
> >> +      * For Tx we have to have space for everything, STX, EXT and
> >> +      * potentially stuffed DATA + CSUM data + csum
> >> +      */
> >> +     RAVE_SP_TX_BUFFER_SIZE  = RAVE_SP_STX_ETX_SIZE +
> >> +                               2 * RAVE_SP_RX_BUFFER_SIZE,
> >> +};
> >
> > This format is unusual for an enum.
> >
> > These would better suit #includes I think.
> >
> 
> Not sure what you mean here. You want me to move that enum into a header file?

No, but I do want you to convert them to #defines.

> >> +enum rave_sp_deframer_state {
> >> +     RAVE_SP_EXPECT_SOF,
> >> +     RAVE_SP_EXPECT_DATA,
> >> +     RAVE_SP_EXPECT_ESCAPED_DATA,
> >> +};
> >
> > It's normal to set the first attribute as 0.
> >
> > I can't remember what the C standard says about that.
> >
> 
> It's initialized to 0 if no value is specified.
> 
> >> +struct rave_sp_deframer {
> >> +     enum rave_sp_deframer_state state;
> >> +     unsigned char data[RAVE_SP_RX_BUFFER_SIZE];
> >> +     size_t length;
> >> +};
> >> +
> >> +struct rave_sp_reply {
> >> +     size_t length;
> >> +     void  *data;
> >> +     u8     code;
> >> +     u8     ackid;
> >> +     struct completion received;
> >> +};
> >> +
> >> +struct rave_sp_checksum {
> >> +     size_t length;
> >> +     void (*subroutine)(const u8 *, size_t, u8 *);
> >> +};
> >> +
> >> +enum rave_sp_boot_source {
> >> +     RAVE_SP_BOOT_SOURCE_SD          = 0,
> >> +     RAVE_SP_BOOT_SOURCE_EMMC        = 1,
> >> +     RAVE_SP_BOOT_SOURCE_NOR         = 2,
> >> +};
> >
> > Just set the first one.  The C standard will do the rest.
> >
> 
> Those correspond to values specified in device's ICD, so I specified
> all of the values explicitly to make it easier to compare against the
> documentation. I'll change it if you insist, but my preference would
> be to keep it as is.

If you provide a comment to that effect you don't have to explicitly
set the values.  It's common practice.

"These must not be reordered.", or similar.

> >> +struct rave_sp_variant {
> >> +     const struct rave_sp_checksum *checksum;
> >> +
> >> +     struct {
> >> +             int (*translate)(enum rave_sp_command);
> >> +             int (*get_boot_source)(struct rave_sp *);
> >> +             int (*set_boot_source)(struct rave_sp *,
> >> +                                    enum rave_sp_boot_source);
> >> +     } cmd;
> >
> > Declare this struct separately, then reference it.
> >
> 
> OK, will fix in v3.
> 
> >> +     void (*init)(struct rave_sp *);
> >> +
> >> +     struct attribute_group group;
> >> +};
> >> +
> >> +struct rave_sp {
> >> +     struct serdev_device *serdev;
> >> +
> >> +     struct rave_sp_deframer deframer;
> >> +     atomic_t ackid;
> >> +
> >> +     struct mutex bus_lock;
> >> +     struct mutex reply_lock;
> >> +     struct rave_sp_reply *reply;
> >> +
> >> +     const char *part_number_firmware;
> >> +     const char *part_number_bootloader;
> >> +
> >> +     const char *reset_reason;
> >> +     const char *copper_rev_rmb;
> >> +     const char *copper_rev_deb;
> >> +     const char *silicon_devid;
> >> +     const char *silicon_devrev;
> >> +
> >> +     const char *copper_mod_rmb;
> >> +     const char *copper_mod_deb;
> >> +
> >> +     const struct rave_sp_variant *variant;
> >> +
> >> +     struct blocking_notifier_head event_notifier_list;
> >> +
> >> +     struct attribute_group *group;
> >> +};
> >
> > This is going to require a kerneldoc header.
> >
> 
> OK. Will fix in v3.
> 
> > [...]
> >
> > I'm going to stop the review here.  Looking at the rest of the code,
> > it looks like you're submitting to the wrong subsystem.
> >
> > Please consider submitting to drivers/platform instead.
> >
> 
> Could you elaborate a bit more why you think this driver doesn't
> belong in MFD subsystem? My reasoning for putting it there was that
> it's a driver for a device that connects to SoC via a serial bus, it
> implements multiple distinct functions that are exposed to MFD cell
> drivers it creates via shared API. There seemed to be number of I2C
> devices in MFD that seem to be exactly that. Did I miss some crucial
> difference?

There is some overlap between the 2 subsystems.  Many of the drivers
which we now describe as 'platform' used to be MFD drivers.  Once the
aforementioned shared API reaches a level of complexity, it is no
longer considered to be a fairly simple IC which provides >1 piece of
functionality.  Instead it's considered more of a platform driver.
Your driver surpasses that level of complexity and needs to be handled
slightly differently.

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