On Fri, 23 Jun 2017, Andrey Smirnov wrote:

On Tue, Jun 20, 2017 at 1:19 AM, Lee Jones [off-list ref] wrote:

quoted

On Mon, 12 Jun 2017, Andrey Smirnov wrote:

quoted

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
Cc: Lucas Stach <redacted>
Cc: Nikita Yushchenko <nikita.yoush-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8@public.gmane.org>
Cc: Rob Herring <redacted>
Cc: Mark Rutland <redacted>
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Andrey Smirnov <redacted>
---

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org


 .../devicetree/bindings/mfd/zii,rave-sp.txt        |   36 +

This should be a separate patch.

OK, will fix in v3.

quoted

quoted

 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.

quoted

quoted

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

quoted

quoted

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

quoted

quoted

+Example of usage:
+
+     rdu {

What is an RDU?

RDU stands for "removable display unit".

And what is one of those?

quoted

quoted

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

quoted

quoted

 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.

quoted

quoted

+ * 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.

quoted

quoted

+ */
+
+/*
+ * 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.

quoted

quoted

+#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.

quoted

Also these are not very human readable.  Either improve the
nomenclature or add a comment/header.

OK, will add comments in v3.

quoted

quoted

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

quoted

quoted

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

quoted

quoted

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

quoted

quoted

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

quoted

quoted

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

quoted

[...]

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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html