[RESEND PATCH v7 0/1] ZII RAVE platform driver

[RESEND PATCH v7 0/1] ZII RAVE platform driver

[Andrey Smirnov]

Hi everyone,

This patch series is v7 of the driver for supervisory processor found
on RAVE series of devices from ZII. Supervisory processor is a PIC
microcontroller connected to various electrical subsystems on RAVE
devices whose firmware implements protocol to command/qery them.

Changes since [v6]:

    - Patch 2/2 has been applied by Lee so it is no longer a part of the series

    - Removed all sysfs and debugfs attribute to reduce the scope of
      the driver propsed for inclusion. This is not a critical to have
      feature and can be added/discussed later.

Changes since [v5]:

    - Fixed a build break, introduced by a last minute change in [v5]

    - Moved majority of attributes that were exposed over sysfs to debugfs

    - Document remaining sysfs attributes in Documentation/ABI/testing/sysfs-platform-rave-sp

Changes since [v4]:

    - Replaced usage of DEVICE_ATTR with DEVICE_ATTR_RW

    - Fixed a number of warnings produces by sparse tool

    - Incorporated event more feedback from Andy Shevchenko

    - Collected Reviewed-by from Andy

Changes since [v3]:

    - Re-collected lost Acked-by from Rob

    - Incorporated further feedback from Andy Shevchenko

    - Dropped useless change (stray newline) to drivers/mfd/Makefile

Changes since [v2]:

    - Fixed swapped command codes in rave_sp_common_get_boot_source()
      and rave_sp_common_set_boot_source() revealed by further testing
      of the code

    - Incorporated feedback from Andy Shevchenko

Changes since [v1]:

    - Updated wording in DT-bindings as per Rob's request.

    - Collected Rob's Acked-by for patch 2/2

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'

Feedback is greatly appreciated!

Thanks,
Andrey Smirnov

[v6] lkml.kernel.org/r/20170828163131.24815-2-andrew.smirnov@gmail.com
[v5] lkml.kernel.org/r/20170728142704.11156-1-andrew.smirnov@gmail.com
[v4] lkml.kernel.org/r/20170725184450.13171-1-andrew.smirnov@gmail.com
[v3] lkml.kernel.org/r/20170724150915.4824-1-andrew.smirnov@gmail.com
[v2] lkml.kernel.org/r/20170718175604.11735-1-andrew.smirnov@gmail.com
[v1] lkml.kernel.org/r/20170710170449.4544-1-andrew.smirnov@gmail.com

Andrey Smirnov (1):
  platform: Add driver for RAVE Supervisory Processor

 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 ++++++++++++++++++++++++++++++++++++++++
 include/linux/rave-sp.h         |  54 ++++
 6 files changed, 761 insertions(+)
 create mode 100644 drivers/platform/rave/Kconfig
 create mode 100644 drivers/platform/rave/Makefile
 create mode 100644 drivers/platform/rave/rave-sp.c
 create mode 100644 include/linux/rave-sp.h

-- 
2.13.5

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

[Andrey Smirnov]

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: 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 ++++++++++++++++++++++++++++++++++++++++
 include/linux/rave-sp.h         |  54 ++++
 6 files changed, 761 insertions(+)
 create mode 100644 drivers/platform/rave/Kconfig
 create mode 100644 drivers/platform/rave/Makefile
 create mode 100644 drivers/platform/rave/rave-sp.c
 create mode 100644 include/linux/rave-sp.h

diff --git a/drivers/platform/Kconfig b/drivers/platform/Kconfig
index c11db8bceea1..e6db685bb895 100644
--- a/drivers/platform/Kconfig
+++ b/drivers/platform/Kconfig
@@ -8,3 +8,5 @@ endif
 source "drivers/platform/goldfish/Kconfig"
 
 source "drivers/platform/chrome/Kconfig"
+
+source "drivers/platform/rave/Kconfig"
diff --git a/drivers/platform/Makefile b/drivers/platform/Makefile
index ca2692510733..17bdec5ece0c 100644
--- a/drivers/platform/Makefile
+++ b/drivers/platform/Makefile
@@ -7,3 +7,4 @@ obj-$(CONFIG_MIPS)		+= mips/
 obj-$(CONFIG_OLPC)		+= olpc/
 obj-$(CONFIG_GOLDFISH)		+= goldfish/
 obj-$(CONFIG_CHROME_PLATFORMS)	+= chrome/
+obj-y += rave/
diff --git a/drivers/platform/rave/Kconfig b/drivers/platform/rave/Kconfig
new file mode 100644
index 000000000000..6fc50ade3871
--- /dev/null
+++ b/drivers/platform/rave/Kconfig
@@ -0,0 +1,26 @@
+#
+# Platform support for Zodiac RAVE hardware
+#
+
+menuconfig RAVE_PLATFORMS
+	bool "Platform support for Zodiac RAVE hardware"
+	depends on SERIAL_DEV_BUS && SERIAL_DEV_CTRL_TTYPORT
+	help
+	  Say Y here to get to see options for platform support for
+	  various devices present in RAVE hardware. This option alone
+	  does not add any kernel code.
+
+	  If you say N, all options in this submenu will be skipped
+	  and disabled.
+
+if RAVE_PLATFORMS
+
+config RAVE_SP_CORE
+	tristate "RAVE SP MCU core driver"
+	select MFD_CORE
+	select CRC_CCITT
+	help
+	  Select this to get support for the Supervisory Processor
+	  device found on several devices in RAVE line of hardware.
+
+endif
diff --git a/drivers/platform/rave/Makefile b/drivers/platform/rave/Makefile
new file mode 100644
index 000000000000..e4c21ab2d2f5
--- /dev/null
+++ b/drivers/platform/rave/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_RAVE_SP_CORE) += rave-sp.o
diff --git a/drivers/platform/rave/rave-sp.c b/drivers/platform/rave/rave-sp.c
new file mode 100644
index 000000000000..d2660d0f8e7d
--- /dev/null
+++ b/drivers/platform/rave/rave-sp.c
@@ -0,0 +1,677 @@
+/*
+ * Multifunction core driver for Zodiac Inflight Innovations
+ * 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/>.
+ */
+
+#include <linux/atomic.h>
+#include <linux/crc-ccitt.h>
+#include <linux/delay.h>
+#include <linux/export.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/rave-sp.h>
+#include <linux/sched.h>
+#include <linux/serdev.h>
+#include <asm/unaligned.h>
+
+/*
+ * 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 RAVE_SP_STX			0x02
+#define RAVE_SP_ETX			0x03
+#define RAVE_SP_DLE			0x10
+
+#define RAVE_SP_MAX_DATA_SIZE		64
+#define RAVE_SP_CHECKSUM_SIZE		2  /* Worst case scenario on RDU2 */
+/*
+ * We don't store STX, ETX and unescaped bytes, so Rx is only
+ * DATA + CSUM
+ */
+#define RAVE_SP_RX_BUFFER_SIZE				\
+	(RAVE_SP_MAX_DATA_SIZE + RAVE_SP_CHECKSUM_SIZE)
+
+#define RAVE_SP_STX_ETX_SIZE		2
+/*
+ * For Tx we have to have space for everything, STX, EXT and
+ * potentially stuffed DATA + CSUM data + csum
+ */
+#define RAVE_SP_TX_BUFFER_SIZE				\
+	(RAVE_SP_STX_ETX_SIZE + 2 * RAVE_SP_RX_BUFFER_SIZE)
+
+#define RAVE_SP_BOOT_SOURCE_GET		0
+#define RAVE_SP_BOOT_SOURCE_SET		1
+
+#define RAVE_SP_RDU2_BOARD_TYPE_RMB	0
+#define RAVE_SP_RDU2_BOARD_TYPE_DEB	1
+
+#define RAVE_SP_BOOT_SOURCE_SD		0
+#define RAVE_SP_BOOT_SOURCE_EMMC	1
+#define RAVE_SP_BOOT_SOURCE_NOR		2
+
+/**
+ * enum rave_sp_deframer_state - Possible state for de-framer
+ *
+ * @RAVE_SP_EXPECT_SOF:		 Scanning input for start-of-frame marker
+ * @RAVE_SP_EXPECT_DATA:	 Got start of frame marker, collecting frame
+ * @RAVE_SP_EXPECT_ESCAPED_DATA: Got escape character, collecting escaped byte
+ */
+enum rave_sp_deframer_state {
+	RAVE_SP_EXPECT_SOF,
+	RAVE_SP_EXPECT_DATA,
+	RAVE_SP_EXPECT_ESCAPED_DATA,
+};
+
+/**
+ * struct rave_sp - Device protocol deframer
+ *
+ * @state:  Current state of the deframer
+ * @data:   Buffer used to collect deframed data
+ * @length: Number of bytes de-framed so far
+ */
+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 - reply as per RAVE device protocol
+ *
+ * @length:	Expected reply length
+ * @data:	Buffer to store reply payload in
+ * @code:	Expected reply code
+ * @ackid:	Expected reply ACK ID
+ * @completion: Successful reply reception completion
+ */
+struct rave_sp_reply {
+	size_t length;
+	void  *data;
+	u8     code;
+	u8     ackid;
+	struct completion received;
+};
+
+/**
+ * struct rave_sp_checksum - Variant specific checksum implementation details
+ *
+ * @length:	Caculated checksum length
+ * @subroutine: Utilized checksum algorithm implementation
+ */
+struct rave_sp_checksum {
+	size_t length;
+	void (*subroutine)(const u8 *, size_t, u8 *);
+};
+
+/**
+ * struct rave_sp_variant_cmds - Variant specific command routines
+ *
+ * @translate:	     Generic to variant specific command mapping routine
+ *
+ */
+struct rave_sp_variant_cmds {
+	int (*translate)(enum rave_sp_command);
+};
+
+/**
+ * struct rave_sp_variant - RAVE supervisory processor core variant
+ *
+ * @checksum:	Variant specific checksum implementation
+ * @cmd:	Variant specific command pointer table
+ *
+ */
+struct rave_sp_variant {
+	const struct rave_sp_checksum *checksum;
+	struct rave_sp_variant_cmds cmd;
+};
+
+/**
+ * struct rave_sp - RAVE supervisory processor core
+ *
+ * @serdev:			Pointer to underlying serdev
+ * @deframer:			Stored state of the protocol deframer
+ * @ackid:			ACK ID used in last reply sent to the device
+ * @bus_lock:			Lock to serialize access to the device
+ * @reply_lock:			Lock protecting @reply
+ * @reply:			Pointer to memory to store reply payload
+ *
+ * @variant:			Device variant specific information
+ * @event_notifier_list:	Input event notification chain
+ *
+ */
+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 struct rave_sp_variant *variant;
+
+	struct blocking_notifier_head event_notifier_list;
+};
+
+struct rave_sp_rsp_status {
+	u8 bl_bytes[6];
+	u8 fw_bytes[6];
+	u8 gs_format;
+} __packed;
+
+static bool rave_sp_id_is_event(u8 code)
+{
+	return (code & 0xF0) == RAVE_SP_EVNT_BASE;
+}
+
+static void rave_sp_unregister_event_notifier(struct device *dev, void *res)
+{
+	struct rave_sp *sp = dev_get_drvdata(dev->parent);
+	struct notifier_block *nb = *(struct notifier_block **)res;
+	struct blocking_notifier_head *bnh = &sp->event_notifier_list;
+
+	WARN_ON(blocking_notifier_chain_unregister(bnh, nb));
+}
+
+int devm_rave_sp_register_event_notifier(struct device *dev,
+					 struct notifier_block *nb)
+{
+	struct rave_sp *sp = dev_get_drvdata(dev->parent);
+	struct notifier_block **rcnb;
+	int ret;
+
+	rcnb = devres_alloc(rave_sp_unregister_event_notifier,
+			    sizeof(*rcnb), GFP_KERNEL);
+	if (!rcnb)
+		return -ENOMEM;
+
+	ret = blocking_notifier_chain_register(&sp->event_notifier_list, nb);
+	if (!ret) {
+		*rcnb = nb;
+		devres_add(dev, rcnb);
+	} else {
+		devres_free(rcnb);
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(devm_rave_sp_register_event_notifier);
+
+static void csum_8b2c(const u8 *buf, size_t size, u8 *crc)
+{
+	*crc = *buf++;
+	size--;
+
+	while (size--)
+		*crc += *buf++;
+
+	*crc = 1 + ~(*crc);
+}
+
+static void csum_ccitt(const u8 *buf, size_t size, u8 *crc)
+{
+	const u16 calculated = crc_ccitt_false(0xffff, buf, size);
+
+	/*
+	 * While the rest of the wire protocol is little-endian,
+	 * CCITT-16 CRC in RDU2 device is sent out in big-endian order.
+	 */
+	put_unaligned_be16(calculated, crc);
+}
+
+static void *stuff(unsigned char *dest, const unsigned char *src, size_t n)
+{
+	while (n--) {
+		const unsigned char byte = *src++;
+
+		switch (byte) {
+		case RAVE_SP_STX:
+		case RAVE_SP_ETX:
+		case RAVE_SP_DLE:
+			*dest++ = RAVE_SP_DLE;
+			/* FALLTHROUGH */
+		default:
+			*dest++ = byte;
+		}
+	}
+
+	return dest;
+}
+
+static int rave_sp_write(struct rave_sp *sp, const u8 *data, u8 data_size)
+{
+	const size_t checksum_length = sp->variant->checksum->length;
+	unsigned char frame[RAVE_SP_TX_BUFFER_SIZE];
+	unsigned char crc[RAVE_SP_CHECKSUM_SIZE];
+	unsigned char *dest = frame;
+	size_t length;
+
+	if (WARN_ON(checksum_length > sizeof(crc)))
+		return -ENOMEM;
+
+	if (WARN_ON(data_size > sizeof(frame)))
+		return -ENOMEM;
+
+	sp->variant->checksum->subroutine(data, data_size, crc);
+
+	*dest++ = RAVE_SP_STX;
+	dest = stuff(dest, data, data_size);
+	dest = stuff(dest, crc, checksum_length);
+	*dest++ = RAVE_SP_ETX;
+
+	length = dest - frame;
+
+	print_hex_dump(KERN_DEBUG, "rave-sp tx: ", DUMP_PREFIX_NONE,
+		       16, 1, frame, length, false);
+
+	return serdev_device_write(sp->serdev, frame, length, HZ);
+}
+
+static u8 rave_sp_reply_code(u8 command)
+{
+	/*
+	 * There isn't a single rule that describes command code ->
+	 * ACK code transformation, but, going through various
+	 * versions of ICDs, there appear to be three distinct groups
+	 * that can be described by simple transformation.
+	 */
+	switch (command) {
+	case 0xA0 ... 0xBE:
+		/*
+		 * Commands implemented by firmware found in RDU1 and
+		 * older devices all seem to obey the following rule
+		 */
+		return command + 0x20;
+	case 0xE0 ... 0xEF:
+		/*
+		 * Events emitted by all versions of the firmare use
+		 * least significant bit to get an ACK code
+		 */
+		return command | 0x01;
+	default:
+		/*
+		 * Commands implemented by firmware found in RDU2 are
+		 * similar to "old" commands, but they use slightly
+		 * different offset
+		 */
+		return command + 0x40;
+	}
+}
+
+int rave_sp_exec(struct rave_sp *sp,
+		 void *__data,  size_t data_size,
+		 void *reply_data, size_t reply_data_size)
+{
+	struct rave_sp_reply reply = {
+		.data     = reply_data,
+		.length   = reply_data_size,
+		.received = COMPLETION_INITIALIZER_ONSTACK(reply.received),
+	};
+	unsigned char *data = __data;
+	int command, ret = 0;
+	u8 ackid;
+
+	command = sp->variant->cmd.translate(data[0]);
+	if (command < 0)
+		return command;
+
+	ackid       = atomic_inc_return(&sp->ackid);
+	reply.ackid = ackid;
+	reply.code  = rave_sp_reply_code((u8)command),
+
+	mutex_lock(&sp->bus_lock);
+
+	mutex_lock(&sp->reply_lock);
+	sp->reply = &reply;
+	mutex_unlock(&sp->reply_lock);
+
+	data[0] = command;
+	data[1] = ackid;
+
+	rave_sp_write(sp, data, data_size);
+
+	if (!wait_for_completion_timeout(&reply.received, HZ)) {
+		dev_err(&sp->serdev->dev, "Command timeout\n");
+		ret = -ETIMEDOUT;
+
+		mutex_lock(&sp->reply_lock);
+		sp->reply = NULL;
+		mutex_unlock(&sp->reply_lock);
+	}
+
+	mutex_unlock(&sp->bus_lock);
+	return ret;
+}
+EXPORT_SYMBOL(rave_sp_exec);
+
+static void rave_sp_receive_event(struct rave_sp *sp,
+				  const unsigned char *data, size_t length)
+{
+	u8 cmd[] = {
+		[0] = rave_sp_reply_code(data[0]),
+		[1] = data[1],
+	};
+
+	rave_sp_write(sp, cmd, sizeof(cmd));
+
+	blocking_notifier_call_chain(&sp->event_notifier_list,
+				     rave_sp_action_pack(data[0], data[2]),
+				     NULL);
+}
+
+static void rave_sp_receive_reply(struct rave_sp *sp,
+				  const unsigned char *data, size_t length)
+{
+	struct device *dev = &sp->serdev->dev;
+	struct rave_sp_reply *reply;
+	const  size_t payload_length = length - 2;
+
+	mutex_lock(&sp->reply_lock);
+	reply = sp->reply;
+
+	if (reply) {
+		if (reply->code == data[0] && reply->ackid == data[1] &&
+		    payload_length >= reply->length) {
+			/*
+			 * We are relying on memcpy(dst, src, 0) to be a no-op
+			 * when handling commands that have a no-payload reply
+			 */
+			memcpy(reply->data, &data[2], reply->length);
+			complete(&reply->received);
+			sp->reply = NULL;
+		} else {
+			dev_err(dev, "Ignoring incorrect reply\n");
+			dev_dbg(dev, "Code:   expected = 0x%08x received = 0x%08x\n",
+				reply->code, data[0]);
+			dev_dbg(dev, "ACK ID: expected = 0x%08x received = 0x%08x\n",
+				reply->ackid, data[1]);
+			dev_dbg(dev, "Length: expected = %zu received = %zu\n",
+				reply->length, payload_length);
+		}
+	}
+
+	mutex_unlock(&sp->reply_lock);
+}
+
+static void rave_sp_receive_frame(struct rave_sp *sp,
+				  const unsigned char *data,
+				  size_t length)
+{
+	const size_t checksum_length = sp->variant->checksum->length;
+	const size_t payload_length  = length - checksum_length;
+	const u8 *crc_reported       = &data[payload_length];
+	struct device *dev           = &sp->serdev->dev;
+	u8 crc_calculated[checksum_length];
+
+	print_hex_dump(KERN_DEBUG, "rave-sp rx: ", DUMP_PREFIX_NONE,
+		       16, 1, data, length, false);
+
+	if (unlikely(length <= checksum_length)) {
+		dev_warn(dev, "Dropping short frame\n");
+		return;
+	}
+
+	sp->variant->checksum->subroutine(data, payload_length,
+					  crc_calculated);
+
+	if (memcmp(crc_calculated, crc_reported, checksum_length)) {
+		dev_warn(dev, "Dropping bad frame\n");
+		return;
+	}
+
+	if (rave_sp_id_is_event(data[0]))
+		rave_sp_receive_event(sp, data, length);
+	else
+		rave_sp_receive_reply(sp, data, length);
+}
+
+static int rave_sp_receive_buf(struct serdev_device *serdev,
+			       const unsigned char *buf, size_t size)
+{
+	struct device *dev  = &serdev->dev;
+	struct rave_sp *sp = dev_get_drvdata(dev);
+	struct rave_sp_deframer *deframer = &sp->deframer;
+	const unsigned char *src = buf;
+	const unsigned char *end = buf + size;
+	bool reset_framer = false;
+
+	while (src < end) {
+		const unsigned char byte = *src++;
+
+		switch (deframer->state) {
+
+		case RAVE_SP_EXPECT_SOF:
+			if (byte == RAVE_SP_STX)
+				deframer->state = RAVE_SP_EXPECT_DATA;
+			continue;
+
+		case RAVE_SP_EXPECT_DATA:
+			switch (byte) {
+			case RAVE_SP_ETX:
+				rave_sp_receive_frame(sp,
+						      deframer->data,
+						      deframer->length);
+				reset_framer = true;
+				break;
+			case RAVE_SP_STX:
+				dev_warn(dev, "Bad frame: STX before ETX\n");
+				reset_framer = true;
+				break;
+			case RAVE_SP_DLE:
+				deframer->state = RAVE_SP_EXPECT_ESCAPED_DATA;
+				continue;
+			}
+
+		case RAVE_SP_EXPECT_ESCAPED_DATA: /* FALLTHROUGH */
+			deframer->data[deframer->length++] = byte;
+
+			if (deframer->length == sizeof(deframer->data)) {
+				dev_warn(dev, "Bad frame: Too long\n");
+				reset_framer = true;
+				break;
+			}
+
+			deframer->state = RAVE_SP_EXPECT_DATA;
+			break;
+		}
+	}
+
+	if (reset_framer) {
+		deframer->state  = RAVE_SP_EXPECT_SOF;
+		deframer->length = 0;
+	}
+
+	return src - buf;
+}
+
+static int rave_sp_rdu1_cmd_translate(enum rave_sp_command command)
+{
+	if (command >= RAVE_SP_CMD_STATUS &&
+	    command <= RAVE_SP_CMD_CONTROL_EVENTS)
+		return command;
+
+	return -EINVAL;
+}
+
+static int rave_sp_rdu2_cmd_translate(enum rave_sp_command command)
+{
+	if (command >= RAVE_SP_CMD_GET_FIRMWARE_VERSION &&
+	    command <= RAVE_SP_CMD_GET_GPIO_STATE)
+		return command;
+
+	if (command == RAVE_SP_CMD_REQ_COPPER_REV) {
+		/*
+		 * As per RDU2 ICD 3.4.47 CMD_GET_COPPER_REV code is
+		 * different from that for RDU1 and it is set to 0x28.
+		 */
+		return 0x28;
+	}
+
+	return rave_sp_rdu1_cmd_translate(command);
+}
+
+static int rave_sp_default_cmd_translate(enum rave_sp_command command)
+{
+	/*
+	 * All of the following command codes were taken from "Table :
+	 * Communications Protocol Message Types" in section 3.3
+	 * "MESSAGE TYPES" of Rave PIC24 ICD.
+	 */
+	switch (command) {
+	case RAVE_SP_CMD_GET_FIRMWARE_VERSION:
+		return 0x11;
+	case RAVE_SP_CMD_GET_BOOTLOADER_VERSION:
+		return 0x12;
+	case RAVE_SP_CMD_BOOT_SOURCE:
+		return 0x14;
+	case RAVE_SP_CMD_SW_WDT:
+		return 0x1C;
+	case RAVE_SP_CMD_RESET:
+		return 0x1E;
+	case RAVE_SP_CMD_RESET_REASON:
+		return 0x1F;
+	default:
+		return -EINVAL;
+	}
+}
+
+
+static const struct rave_sp_checksum rave_sp_checksum_8b2c = {
+	.length     = 1,
+	.subroutine = csum_8b2c,
+};
+
+static const struct rave_sp_checksum rave_sp_checksum_ccitt = {
+	.length     = 2,
+	.subroutine = csum_ccitt,
+};
+
+static const struct rave_sp_variant rave_sp_legacy = {
+	.checksum = &rave_sp_checksum_8b2c,
+	.cmd = {
+		.translate = rave_sp_default_cmd_translate,
+	},
+};
+
+static const struct rave_sp_variant rave_sp_rdu1 = {
+	.checksum = &rave_sp_checksum_8b2c,
+	.cmd = {
+		.translate = rave_sp_rdu1_cmd_translate,
+	},
+};
+
+static const struct rave_sp_variant rave_sp_rdu2 = {
+	.checksum = &rave_sp_checksum_ccitt,
+	.cmd = {
+		.translate = rave_sp_rdu2_cmd_translate,
+	},
+};
+
+static const struct of_device_id rave_sp_dt_ids[] = {
+	{ .compatible = COMPATIBLE_RAVE_SP_NIU,  .data = &rave_sp_legacy },
+	{ .compatible = COMPATIBLE_RAVE_SP_MEZZ, .data = &rave_sp_legacy },
+	{ .compatible = COMPATIBLE_RAVE_SP_ESB,  .data = &rave_sp_legacy },
+	{ .compatible = COMPATIBLE_RAVE_SP_RDU1, .data = &rave_sp_rdu1   },
+	{ .compatible = COMPATIBLE_RAVE_SP_RDU2, .data = &rave_sp_rdu2   },
+	{ /* sentinel */ }
+};
+
+static const struct serdev_device_ops rave_sp_serdev_device_ops = {
+	.receive_buf  = rave_sp_receive_buf,
+	.write_wakeup = serdev_device_write_wakeup,
+};
+
+static int rave_sp_probe(struct serdev_device *serdev)
+{
+	struct device *dev = &serdev->dev;
+	struct rave_sp *sp;
+	u32 baud;
+	int ret;
+
+	if (of_property_read_u32(dev->of_node, "current-speed", &baud)) {
+		dev_err(dev,
+			"'current-speed' is not specified in device node\n");
+		return -EINVAL;
+	}
+
+	sp = devm_kzalloc(dev, sizeof(*sp), GFP_KERNEL);
+	if (!sp)
+		return -ENOMEM;
+
+	sp->serdev = serdev;
+	dev_set_drvdata(dev, sp);
+
+	sp->variant = of_device_get_match_data(dev);
+	if (!sp->variant)
+		return -ENODEV;
+
+	mutex_init(&sp->bus_lock);
+	mutex_init(&sp->reply_lock);
+	BLOCKING_INIT_NOTIFIER_HEAD(&sp->event_notifier_list);
+
+	serdev_device_set_client_ops(serdev, &rave_sp_serdev_device_ops);
+	ret = serdev_device_open(serdev);
+	if (ret)
+		return ret;
+
+	serdev_device_set_baudrate(serdev, baud);
+
+	return of_platform_default_populate(dev->of_node, NULL, dev);
+}
+
+static void rave_sp_remove(struct serdev_device *serdev)
+{
+	of_platform_depopulate(&serdev->dev);
+	serdev_device_close(serdev);
+}
+
+MODULE_DEVICE_TABLE(of, rave_sp_dt_ids);
+
+static struct serdev_device_driver rave_sp_drv = {
+	.probe			= rave_sp_probe,
+	.remove			= rave_sp_remove,
+	.driver = {
+		.name		= "rave-sp",
+		.of_match_table	= rave_sp_dt_ids,
+	},
+};
+module_serdev_device_driver(rave_sp_drv);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Andrey Vostrikov <andrey.vostrikov@cogentembedded.com>");
+MODULE_AUTHOR("Nikita Yushchenko <nikita.yoush@cogentembedded.com>");
+MODULE_AUTHOR("Andrey Smirnov <andrew.smirnov@gmail.com>");
+MODULE_DESCRIPTION("RAVE SP core driver");
diff --git a/include/linux/rave-sp.h b/include/linux/rave-sp.h
new file mode 100644
index 000000000000..14f9b3177b03
--- /dev/null
+++ b/include/linux/rave-sp.h
@@ -0,0 +1,54 @@
+#ifndef _LINUX_RAVE_SP_H_
+#define _LINUX_RAVE_SP_H_
+
+enum rave_sp_command {
+	RAVE_SP_CMD_GET_FIRMWARE_VERSION	= 0x20,
+	RAVE_SP_CMD_GET_BOOTLOADER_VERSION	= 0x21,
+	RAVE_SP_CMD_BOOT_SOURCE			= 0x26,
+	RAVE_SP_CMD_GET_BOARD_COPPER_REV	= 0x2B,
+	RAVE_SP_CMD_GET_GPIO_STATE		= 0x2F,
+
+	RAVE_SP_CMD_STATUS			= 0xA0,
+	RAVE_SP_CMD_SW_WDT			= 0xA1,
+	RAVE_SP_CMD_PET_WDT			= 0xA2,
+	RAVE_SP_CMD_RESET			= 0xA7,
+	RAVE_SP_CMD_RESET_REASON		= 0xA8,
+
+	RAVE_SP_CMD_REQ_COPPER_REV		= 0xB6,
+	RAVE_SP_CMD_GET_I2C_DEVICE_STATUS	= 0xBA,
+	RAVE_SP_CMD_GET_SP_SILICON_REV		= 0xB9,
+	RAVE_SP_CMD_CONTROL_EVENTS		= 0xBB,
+
+	RAVE_SP_EVNT_BASE			= 0xE0,
+};
+
+struct rave_sp;
+
+static inline unsigned long rave_sp_action_pack(u8 event, u8 value)
+{
+	return ((unsigned long)value << 8) | event;
+}
+
+static inline u8 rave_sp_action_unpack_event(unsigned long action)
+{
+	return action;
+}
+
+static inline u8 rave_sp_action_unpack_value(unsigned long action)
+{
+	return action >> 8;
+}
+
+int rave_sp_exec(struct rave_sp *sp,
+		 void *__data,  size_t data_size,
+		 void *reply_data, size_t reply_data_size);
+int devm_rave_sp_register_event_notifier(struct device *dev,
+				     struct notifier_block *nb);
+
+#define COMPATIBLE_RAVE_SP_NIU		"zii,rave-sp-niu"
+#define COMPATIBLE_RAVE_SP_MEZZ		"zii,rave-sp-mezz"
+#define COMPATIBLE_RAVE_SP_ESB		"zii,rave-sp-esb"
+#define COMPATIBLE_RAVE_SP_RDU1		"zii,rave-sp-rdu1"
+#define COMPATIBLE_RAVE_SP_RDU2		"zii,rave-sp-rdu2"
+
+#endif /* _LINUX_RAVE_SP_H_ */
-- 
2.13.5

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

[Johan Hovold]

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

>  include/linux/rave-sp.h         |  54 ++++
>  6 files changed, 761 insertions(+)
>  create mode 100644 drivers/platform/rave/Kconfig
>  create mode 100644 drivers/platform/rave/Makefile
>  create mode 100644 drivers/platform/rave/rave-sp.c
>  create mode 100644 include/linux/rave-sp.h
> 
> diff --git a/drivers/platform/Kconfig b/drivers/platform/Kconfig
> index c11db8bceea1..e6db685bb895 100644
> --- a/drivers/platform/Kconfig
> +++ b/drivers/platform/Kconfig
> @@ -8,3 +8,5 @@ endif
>  source "drivers/platform/goldfish/Kconfig"
>  
>  source "drivers/platform/chrome/Kconfig"
> +
> +source "drivers/platform/rave/Kconfig"
> diff --git a/drivers/platform/Makefile b/drivers/platform/Makefile
> index ca2692510733..17bdec5ece0c 100644
> --- a/drivers/platform/Makefile
> +++ b/drivers/platform/Makefile
> @@ -7,3 +7,4 @@ obj-$(CONFIG_MIPS)		+= mips/
>  obj-$(CONFIG_OLPC)		+= olpc/
>  obj-$(CONFIG_GOLDFISH)		+= goldfish/
>  obj-$(CONFIG_CHROME_PLATFORMS)	+= chrome/
> +obj-y += rave/
> diff --git a/drivers/platform/rave/Kconfig b/drivers/platform/rave/Kconfig
> new file mode 100644
> index 000000000000..6fc50ade3871
> --- /dev/null
> +++ b/drivers/platform/rave/Kconfig
> @@ -0,0 +1,26 @@
> +#
> +# Platform support for Zodiac RAVE hardware
> +#
> +
> +menuconfig RAVE_PLATFORMS
> +	bool "Platform support for Zodiac RAVE hardware"
> +	depends on SERIAL_DEV_BUS && SERIAL_DEV_CTRL_TTYPORT

You don't strictly depend on SERIAL_DEV_CTRL_TTYPORT even if I
understand why you added it (that controller will default to Y when
serdev is enabled soon).

Also this is not the entry that depends on serdev, RAVE_SP_CORE is the
driver that depends on it.

I think this one can just be removed, and like for normal mfd drivers,
have the "child drivers" depend on the mfd driver (e.g. RAVE_SP_CORE)
below.

> +	help
> +	  Say Y here to get to see options for platform support for
> +	  various devices present in RAVE hardware. This option alone
> +	  does not add any kernel code.
> +
> +	  If you say N, all options in this submenu will be skipped
> +	  and disabled.
> +
> +if RAVE_PLATFORMS
> +
> +config RAVE_SP_CORE
> +	tristate "RAVE SP MCU core driver"
> +	select MFD_CORE

And here you select on mfd core even though you currently don't seem to
use it at all.

> +	select CRC_CCITT
> +	help
> +	  Select this to get support for the Supervisory Processor
> +	  device found on several devices in RAVE line of hardware.
> +
> +endif
> diff --git a/drivers/platform/rave/Makefile b/drivers/platform/rave/Makefile
> new file mode 100644
> index 000000000000..e4c21ab2d2f5
> --- /dev/null
> +++ b/drivers/platform/rave/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_RAVE_SP_CORE) += rave-sp.o
> diff --git a/drivers/platform/rave/rave-sp.c b/drivers/platform/rave/rave-sp.c
> new file mode 100644
> index 000000000000..d2660d0f8e7d
> --- /dev/null
> +++ b/drivers/platform/rave/rave-sp.c

[ ignoring the driver implementation ]

> +static const struct of_device_id rave_sp_dt_ids[] = {
> +	{ .compatible = COMPATIBLE_RAVE_SP_NIU,  .data = &rave_sp_legacy },
> +	{ .compatible = COMPATIBLE_RAVE_SP_MEZZ, .data = &rave_sp_legacy },
> +	{ .compatible = COMPATIBLE_RAVE_SP_ESB,  .data = &rave_sp_legacy },
> +	{ .compatible = COMPATIBLE_RAVE_SP_RDU1, .data = &rave_sp_rdu1   },
> +	{ .compatible = COMPATIBLE_RAVE_SP_RDU2, .data = &rave_sp_rdu2   },

I think you should use the compatible strings directly here rather than
use these defines.

> +	{ /* sentinel */ }
> +};

> +static int rave_sp_probe(struct serdev_device *serdev)
> +{
> +	struct device *dev = &serdev->dev;
> +	struct rave_sp *sp;
> +	u32 baud;
> +	int ret;
> +
> +	if (of_property_read_u32(dev->of_node, "current-speed", &baud)) {
> +		dev_err(dev,
> +			"'current-speed' is not specified in device node\n");
> +		return -EINVAL;
> +	}
> +
> +	sp = devm_kzalloc(dev, sizeof(*sp), GFP_KERNEL);
> +	if (!sp)
> +		return -ENOMEM;
> +
> +	sp->serdev = serdev;
> +	dev_set_drvdata(dev, sp);
> +
> +	sp->variant = of_device_get_match_data(dev);
> +	if (!sp->variant)
> +		return -ENODEV;
> +
> +	mutex_init(&sp->bus_lock);
> +	mutex_init(&sp->reply_lock);
> +	BLOCKING_INIT_NOTIFIER_HEAD(&sp->event_notifier_list);
> +
> +	serdev_device_set_client_ops(serdev, &rave_sp_serdev_device_ops);
> +	ret = serdev_device_open(serdev);
> +	if (ret)
> +		return ret;
> +
> +	serdev_device_set_baudrate(serdev, baud);
> +
> +	return of_platform_default_populate(dev->of_node, NULL, dev);

You must close the serdev before returning on errors.

> +}
> +
> +static void rave_sp_remove(struct serdev_device *serdev)
> +{
> +	of_platform_depopulate(&serdev->dev);
> +	serdev_device_close(serdev);
> +}
> +
> +MODULE_DEVICE_TABLE(of, rave_sp_dt_ids);

> +#define COMPATIBLE_RAVE_SP_NIU		"zii,rave-sp-niu"
> +#define COMPATIBLE_RAVE_SP_MEZZ		"zii,rave-sp-mezz"
> +#define COMPATIBLE_RAVE_SP_ESB		"zii,rave-sp-esb"
> +#define COMPATIBLE_RAVE_SP_RDU1		"zii,rave-sp-rdu1"
> +#define COMPATIBLE_RAVE_SP_RDU2		"zii,rave-sp-rdu2"
> +
> +#endif /* _LINUX_RAVE_SP_H_ */

Johan

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

[Andrey Smirnov]

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

Could you elaborate more on "instead of rolling your own
mfd-implementation"? It was my understanding that using "simple-mfd"
binding and of_platform_default_populate() was the simplest way to
create a DT-only MFD and that's how the driver was implemented when I
submitted it for inclusion to MFD as well. Am I re-inventing something
and is there a simpler way?

>>  include/linux/rave-sp.h         |  54 ++++
>>  6 files changed, 761 insertions(+)
>>  create mode 100644 drivers/platform/rave/Kconfig
>>  create mode 100644 drivers/platform/rave/Makefile
>>  create mode 100644 drivers/platform/rave/rave-sp.c
>>  create mode 100644 include/linux/rave-sp.h
>>
>> diff --git a/drivers/platform/Kconfig b/drivers/platform/Kconfig
>> index c11db8bceea1..e6db685bb895 100644
>> --- a/drivers/platform/Kconfig
>> +++ b/drivers/platform/Kconfig
>> @@ -8,3 +8,5 @@ endif
>>  source "drivers/platform/goldfish/Kconfig"
>>
>>  source "drivers/platform/chrome/Kconfig"
>> +
>> +source "drivers/platform/rave/Kconfig"
>> diff --git a/drivers/platform/Makefile b/drivers/platform/Makefile
>> index ca2692510733..17bdec5ece0c 100644
>> --- a/drivers/platform/Makefile
>> +++ b/drivers/platform/Makefile
>> @@ -7,3 +7,4 @@ obj-$(CONFIG_MIPS)            += mips/
>>  obj-$(CONFIG_OLPC)           += olpc/
>>  obj-$(CONFIG_GOLDFISH)               += goldfish/
>>  obj-$(CONFIG_CHROME_PLATFORMS)       += chrome/
>> +obj-y += rave/
>> diff --git a/drivers/platform/rave/Kconfig b/drivers/platform/rave/Kconfig
>> new file mode 100644
>> index 000000000000..6fc50ade3871
>> --- /dev/null
>> +++ b/drivers/platform/rave/Kconfig
>> @@ -0,0 +1,26 @@
>> +#
>> +# Platform support for Zodiac RAVE hardware
>> +#
>> +
>> +menuconfig RAVE_PLATFORMS
>> +     bool "Platform support for Zodiac RAVE hardware"
>> +     depends on SERIAL_DEV_BUS && SERIAL_DEV_CTRL_TTYPORT
>
> You don't strictly depend on SERIAL_DEV_CTRL_TTYPORT even if I
> understand why you added it (that controller will default to Y when
> serdev is enabled soon).
>
> Also this is not the entry that depends on serdev, RAVE_SP_CORE is the
> driver that depends on it.
>
> I think this one can just be removed, and like for normal mfd drivers,
> have the "child drivers" depend on the mfd driver (e.g. RAVE_SP_CORE)
> below.
>

Sure, I can change that.

>> +     help
>> +       Say Y here to get to see options for platform support for
>> +       various devices present in RAVE hardware. This option alone
>> +       does not add any kernel code.
>> +
>> +       If you say N, all options in this submenu will be skipped
>> +       and disabled.
>> +
>> +if RAVE_PLATFORMS
>> +
>> +config RAVE_SP_CORE
>> +     tristate "RAVE SP MCU core driver"
>> +     select MFD_CORE
>
> And here you select on mfd core even though you currently don't seem to
> use it at all.
>

My bad, for some reason I thought I was using something from
mfd-core.c, but I don't. Will remove in v8.

>> +     select CRC_CCITT
>> +     help
>> +       Select this to get support for the Supervisory Processor
>> +       device found on several devices in RAVE line of hardware.
>> +
>> +endif
>> diff --git a/drivers/platform/rave/Makefile b/drivers/platform/rave/Makefile
>> new file mode 100644
>> index 000000000000..e4c21ab2d2f5
>> --- /dev/null
>> +++ b/drivers/platform/rave/Makefile
>> @@ -0,0 +1 @@
>> +obj-$(CONFIG_RAVE_SP_CORE) += rave-sp.o
>> diff --git a/drivers/platform/rave/rave-sp.c b/drivers/platform/rave/rave-sp.c
>> new file mode 100644
>> index 000000000000..d2660d0f8e7d
>> --- /dev/null
>> +++ b/drivers/platform/rave/rave-sp.c
>
> [ ignoring the driver implementation ]
>
>> +static const struct of_device_id rave_sp_dt_ids[] = {
>> +     { .compatible = COMPATIBLE_RAVE_SP_NIU,  .data = &rave_sp_legacy },
>> +     { .compatible = COMPATIBLE_RAVE_SP_MEZZ, .data = &rave_sp_legacy },
>> +     { .compatible = COMPATIBLE_RAVE_SP_ESB,  .data = &rave_sp_legacy },
>> +     { .compatible = COMPATIBLE_RAVE_SP_RDU1, .data = &rave_sp_rdu1   },
>> +     { .compatible = COMPATIBLE_RAVE_SP_RDU2, .data = &rave_sp_rdu2   },
>
> I think you should use the compatible strings directly here rather than
> use these defines.
>

Those compatible strings are also re-used by cell drivers for this
device (not a part of this series) to determine which ICD is
applicable, hence the defines instead of normal strings.

>> +     { /* sentinel */ }
>> +};
>
>> +static int rave_sp_probe(struct serdev_device *serdev)
>> +{
>> +     struct device *dev = &serdev->dev;
>> +     struct rave_sp *sp;
>> +     u32 baud;
>> +     int ret;
>> +
>> +     if (of_property_read_u32(dev->of_node, "current-speed", &baud)) {
>> +             dev_err(dev,
>> +                     "'current-speed' is not specified in device node\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     sp = devm_kzalloc(dev, sizeof(*sp), GFP_KERNEL);
>> +     if (!sp)
>> +             return -ENOMEM;
>> +
>> +     sp->serdev = serdev;
>> +     dev_set_drvdata(dev, sp);
>> +
>> +     sp->variant = of_device_get_match_data(dev);
>> +     if (!sp->variant)
>> +             return -ENODEV;
>> +
>> +     mutex_init(&sp->bus_lock);
>> +     mutex_init(&sp->reply_lock);
>> +     BLOCKING_INIT_NOTIFIER_HEAD(&sp->event_notifier_list);
>> +
>> +     serdev_device_set_client_ops(serdev, &rave_sp_serdev_device_ops);
>> +     ret = serdev_device_open(serdev);
>> +     if (ret)
>> +             return ret;
>> +
>> +     serdev_device_set_baudrate(serdev, baud);
>> +
>> +     return of_platform_default_populate(dev->of_node, NULL, dev);
>
> You must close the serdev before returning on errors.
>

Oops, missed this one, thanks. Will fix in v8.

Thanks,
Andrey Smirnov

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

[Lee Jones]

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

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@linaro.org> wrote:
> 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.

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

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

[Johan Hovold]

On Fri, Oct 13, 2017 at 08:56:00AM -0700, 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".
> 
> Could you elaborate more on "instead of rolling your own
> mfd-implementation"? It was my understanding that using "simple-mfd"
> binding and of_platform_default_populate() was the simplest way to
> create a DT-only MFD and that's how the driver was implemented when I
> submitted it for inclusion to MFD as well. Am I re-inventing something
> and is there a simpler way?

AFAICS you're not using "simple-mfd" here. You're implementing an MFD
driver and the corresponding compatible strings were listed in the
binding docs (e.g. "zii,rave-sp-rdu1").

Also note that no drivers under drivers/platforms does anything like
this.

> >> +config RAVE_SP_CORE
> >> +     tristate "RAVE SP MCU core driver"
> >> +     select MFD_CORE
> >
> > And here you select on mfd core even though you currently don't seem to
> > use it at all.
> >
> 
> My bad, for some reason I thought I was using something from
> mfd-core.c, but I don't. Will remove in v8.

> >> +static const struct of_device_id rave_sp_dt_ids[] = {
> >> +     { .compatible = COMPATIBLE_RAVE_SP_NIU,  .data = &rave_sp_legacy },
> >> +     { .compatible = COMPATIBLE_RAVE_SP_MEZZ, .data = &rave_sp_legacy },
> >> +     { .compatible = COMPATIBLE_RAVE_SP_ESB,  .data = &rave_sp_legacy },
> >> +     { .compatible = COMPATIBLE_RAVE_SP_RDU1, .data = &rave_sp_rdu1   },
> >> +     { .compatible = COMPATIBLE_RAVE_SP_RDU2, .data = &rave_sp_rdu2   },
> >
> > I think you should use the compatible strings directly here rather than
> > use these defines.
> 
> Those compatible strings are also re-used by cell drivers for this
> device (not a part of this series) to determine which ICD is
> applicable, hence the defines instead of normal strings.

I'd say that is indicative that you're doing something wrong here.
You've already matched your MFD and therefore have all the information
you need. You can pass that data to the child drivers through mfd-cells.

You should submit at least one of your subdrivers along with the core
MFD driver before we merge anything so we can see how things will end up
getting used.

Johan

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

[Andrey Smirnov]

On Mon, Oct 16, 2017 at 7:14 AM, Johan Hovold <johan@kernel.org> wrote:
> On Fri, Oct 13, 2017 at 08:56:00AM -0700, 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".
>>
>> Could you elaborate more on "instead of rolling your own
>> mfd-implementation"? It was my understanding that using "simple-mfd"
>> binding and of_platform_default_populate() was the simplest way to
>> create a DT-only MFD and that's how the driver was implemented when I
>> submitted it for inclusion to MFD as well. Am I re-inventing something
>> and is there a simpler way?
>
> AFAICS you're not using "simple-mfd" here. You're implementing an MFD
> driver and the corresponding compatible strings were listed in the
> binding docs (e.g. "zii,rave-sp-rdu1").
>

The reason I mentioned "simple-mfd" was because I assumed it was
necessary to use of_platform_default_populate(), which, re-reading the
corresponding code, seems to have been a mistake on my part. However,
now I am not sure I understand the point you are/were trying to make.
The way I understand "instead of rolling your own mfd-implementation"
is "You ignored/missed the precedent and didn't use agreed-upon way of
creating an MFD from DT and re-invented the code to do so", in which
case I'd like to know the canonical way I missed.

Form you response, however, I get a feeling that you are trying to
convince me that the driver I submitted is an MFD driver. If so, then
there's no need, we are in agreement here.

> Also note that no drivers under drivers/platforms does anything like
> this.
>

I am aware of that. As I as I mentioned before, the decision to place
this code into "drivers/platforms" was not mine.

>> >> +config RAVE_SP_CORE
>> >> +     tristate "RAVE SP MCU core driver"
>> >> +     select MFD_CORE
>> >
>> > And here you select on mfd core even though you currently don't seem to
>> > use it at all.
>> >
>>
>> My bad, for some reason I thought I was using something from
>> mfd-core.c, but I don't. Will remove in v8.
>
>> >> +static const struct of_device_id rave_sp_dt_ids[] = {
>> >> +     { .compatible = COMPATIBLE_RAVE_SP_NIU,  .data = &rave_sp_legacy },
>> >> +     { .compatible = COMPATIBLE_RAVE_SP_MEZZ, .data = &rave_sp_legacy },
>> >> +     { .compatible = COMPATIBLE_RAVE_SP_ESB,  .data = &rave_sp_legacy },
>> >> +     { .compatible = COMPATIBLE_RAVE_SP_RDU1, .data = &rave_sp_rdu1   },
>> >> +     { .compatible = COMPATIBLE_RAVE_SP_RDU2, .data = &rave_sp_rdu2   },
>> >
>> > I think you should use the compatible strings directly here rather than
>> > use these defines.
>>
>> Those compatible strings are also re-used by cell drivers for this
>> device (not a part of this series) to determine which ICD is
>> applicable, hence the defines instead of normal strings.
>
> I'd say that is indicative that you're doing something wrong here.
> You've already matched your MFD and therefore have all the information
> you need.

I have all the information I need at this layer, however there are ICD
differences that are local to functionality covered by individual cell
which I wanted to keep local to the corresponding drivers.

> You can pass that data to the child drivers through mfd-cells.
>

I don't need "that" data for individual cell drivers, they have their
own local data sets form which to chose based on the "flavor" of the
parent device. Obviously that "flavor" can be passed as custom data to
child drivers as well, but I don't think that could be done with less
code than just using of_match_device(variants_table, dev->parent)
which is why I chose that route.

As you suggested, I'll add a cell driver to v8 and, hopefully, it'll
make it more clear as to why I have those defines.

Thanks,
Andrey Smirnov

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

[Johan Hovold]

On Mon, Oct 16, 2017 at 09:45:58AM -0700, Andrey Smirnov wrote:
> On Mon, Oct 16, 2017 at 7:14 AM, Johan Hovold <johan@kernel.org> wrote:
> > On Fri, Oct 13, 2017 at 08:56:00AM -0700, 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.

> The reason I mentioned "simple-mfd" was because I assumed it was
> necessary to use of_platform_default_populate(), which, re-reading the
> corresponding code, seems to have been a mistake on my part. However,
> now I am not sure I understand the point you are/were trying to make.
> The way I understand "instead of rolling your own mfd-implementation"
> is "You ignored/missed the precedent and didn't use agreed-upon way of
> creating an MFD from DT and re-invented the code to do so", in which
> case I'd like to know the canonical way I missed.
> 
> Form you response, however, I get a feeling that you are trying to
> convince me that the driver I submitted is an MFD driver. If so, then
> there's no need, we are in agreement here.

Good, then we just need to convince Lee. ;)

I was merely pointing out that, yes, you are implementing an MFD driver,
but you placed it in drivers/platform and did not not use any facilities
provided by the MFD subsystem (hence, the "rolling your own").

Take a look at the dln2 mfd driver which I believe implement something
similar albeit using USB as a transport (and therefore does not use DT
to instantiates the children). One point being that not all mfd drivers
use i2c/spi and regmap.

> > Also note that no drivers under drivers/platforms does anything like
> > this.
> >
> 
> I am aware of that. As I as I mentioned before, the decision to place
> this code into "drivers/platforms" was not mine.

You should have put this in the changelog and commit message since its
far from obvious what happened here.

Johan

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 924 bytes --]

Hi!

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

Sending contributors around like this is quite uncool.

1st, it should not matter much in which directory the driver lives.

2nd, there's no reasonable way for contributor to proceed if he gets
"stuck" between two maintainers.

Can you discuss with MFD maintainers, and provide clear guidance?

Thanks,
								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1132 bytes --]

Hi!

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

Some drivers are more complex than other ones... I don't think that's
unexpected.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Lee Jones]

On Fri, 13 Oct 2017, Andrey Smirnov wrote:

> On Fri, Oct 13, 2017 at 9:17 AM, Lee Jones <lee.jones@linaro.org> wrote:
> > 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.
> 
> 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.

Okay, it sounds like this might fit into MFD after all.

Sorry for the fuss.

Do you want me to review the first submission, or will you submit
again?

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

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

[Johan Hovold]

On Mon, Oct 23, 2017 at 11:30:54AM +0200, Pavel Machek wrote:
> Hi!
> 
> > >  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)?
> 
> Sending contributors around like this is quite uncool.

Asking questions when things are done in unexpected ways is part of the
review process, and the backstory here wasn't documented in the patch or
cover letter.

> 1st, it should not matter much in which directory the driver lives.
> 
> 2nd, there's no reasonable way for contributor to proceed if he gets
> "stuck" between two maintainers.
> 
> Can you discuss with MFD maintainers, and provide clear guidance?

Seems we all agree this driver belongs in mfd now.

Johan

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

[Andrey Smirnov]

On Tue, Oct 24, 2017 at 3:28 AM, Lee Jones <lee.jones@linaro.org> wrote:
> On Fri, 13 Oct 2017, Andrey Smirnov wrote:
>
>> On Fri, Oct 13, 2017 at 9:17 AM, Lee Jones <lee.jones@linaro.org> wrote:
>> > 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.
>>
>> 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.
>
> Okay, it sounds like this might fit into MFD after all.
>
> Sorry for the fuss.
>
> Do you want me to review the first submission, or will you submit
> again?
>

No worries. I'll move the code back to "drivers/mfd" in v9 of this
patchset and we can go from there.

Thanks,
Andrey Smirnov

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

[Andrey Smirnov]

On Tue, Oct 24, 2017 at 8:13 AM, Johan Hovold <johan@kernel.org> wrote:
> On Mon, Oct 23, 2017 at 11:30:54AM +0200, Pavel Machek wrote:
>> Hi!
>>
>> > >  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)?
>>
>> Sending contributors around like this is quite uncool.
>
> Asking questions when things are done in unexpected ways is part of the
> review process, and the backstory here wasn't documented in the patch or
> cover letter.

Cover letter for this submission contains the link to v1 of the
patchset (marked as "[v1]"):

https://marc.info/?l=linux-kernel&m=149970632624803&w=2

whose cover letter captures why the driver was added to
"drivers/platform" to a degree and contains a link to original
submission:

https://marc.info/?l=linux-kernel&m=149677241805627&w=2

Granted it is not completely effortless to get to all of that, but I
don't think it is fair to say that all of that was not documented.

Andrey Smirnov

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

[Johan Hovold]

On Tue, Oct 24, 2017 at 11:40:38AM -0700, Andrey Smirnov wrote:
> On Tue, Oct 24, 2017 at 8:13 AM, Johan Hovold <johan@kernel.org> wrote:
> > On Mon, Oct 23, 2017 at 11:30:54AM +0200, Pavel Machek wrote:
> >> Hi!
> >>
> >> > >  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)?
> >>
> >> Sending contributors around like this is quite uncool.
> >
> > Asking questions when things are done in unexpected ways is part of the
> > review process, and the backstory here wasn't documented in the patch or
> > cover letter.
> 
> Cover letter for this submission contains the link to v1 of the
> patchset (marked as "[v1]"):
> 
> https://marc.info/?l=linux-kernel&m=149970632624803&w=2
> 
> whose cover letter captures why the driver was added to
> "drivers/platform" to a degree and contains a link to original
> submission:

You cannot expect a reviewer to go through seven revisions of a patch
series to find this information.

> Granted it is not completely effortless to get to all of that, but I
> don't think it is fair to say that all of that was not documented.

I said it "wasn't documented in the patch or cover letter", which I
still claim to be an accurate description.

You did something odd, I called it out, and now the issue is resolved.
Let's move on.

Johan