* [PATCH v2 0/2] ZII RAVE platform driver
@ 2017-07-18 17:56 Andrey Smirnov
2017-07-18 17:56 ` [PATCH v2 1/2] platform: Add driver for RAVE Supervisory Processor Andrey Smirnov
2017-07-18 17:56 ` [PATCH v2 2/2] dt-bindings: mfd: Add bindings for ZII RAVE devices Andrey Smirnov
0 siblings, 2 replies; 9+ messages in thread
From: Andrey Smirnov @ 2017-07-18 17:56 UTC (permalink / raw)
To: linux-kernel
Cc: Andrey Smirnov, cphealy, Lucas Stach, Nikita Yushchenko,
Rob Herring, Mark Rutland, devicetree
Hi everyone,
This patch series is v2 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 [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
[v1] lkml.kernel.org/r/r/20170710170449.4544-1-andrew.smirnov@gmail.com
Andrey Smirnov (2):
platform: Add driver for RAVE Supervisory Processor
dt-bindings: mfd: Add bindings for ZII RAVE devices
.../devicetree/bindings/mfd/zii,rave-sp.txt | 39 +
drivers/mfd/Makefile | 1 +
drivers/platform/Kconfig | 2 +
drivers/platform/Makefile | 1 +
drivers/platform/rave/Kconfig | 26 +
drivers/platform/rave/Makefile | 1 +
drivers/platform/rave/rave-sp.c | 1083 ++++++++++++++++++++
include/linux/rave-sp.h | 54 +
8 files changed, 1207 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/zii,rave-sp.txt
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.3
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 1/2] platform: Add driver for RAVE Supervisory Processor
2017-07-18 17:56 [PATCH v2 0/2] ZII RAVE platform driver Andrey Smirnov
@ 2017-07-18 17:56 ` Andrey Smirnov
2017-07-18 18:48 ` Andy Shevchenko
2017-07-18 17:56 ` [PATCH v2 2/2] dt-bindings: mfd: Add bindings for ZII RAVE devices Andrey Smirnov
1 sibling, 1 reply; 9+ messages in thread
From: Andrey Smirnov @ 2017-07-18 17:56 UTC (permalink / raw)
To: linux-kernel; +Cc: Andrey Smirnov, cphealy, Lucas Stach, Nikita Yushchenko
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@gmail.com
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
drivers/mfd/Makefile | 1 +
drivers/platform/Kconfig | 2 +
drivers/platform/Makefile | 1 +
drivers/platform/rave/Kconfig | 26 +
drivers/platform/rave/Makefile | 1 +
drivers/platform/rave/rave-sp.c | 1083 +++++++++++++++++++++++++++++++++++++++
include/linux/rave-sp.h | 54 ++
7 files changed, 1168 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/mfd/Makefile b/drivers/mfd/Makefile
index c16bf1ea0ea9..8796d65233ac 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
+
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..d240443fc2b8
--- /dev/null
+++ b/drivers/platform/rave/rave-sp.c
@@ -0,0 +1,1083 @@
+/*
+ * 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/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>
+
+/*
+ * 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 data stream for start of frame
+ * marker
+ * @RAVE_SP_EXPECT_DATA: start of frame marker detected, collecting
+ * frame
+ * @RAVE_SP_EXPECT_ESCAPED_DATA: escape character received, 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: Exptected 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 vtable
+ *
+ * @translate: Subroutine to translate common command identifiers to
+ * device specific variants. Different generations of the
+ * ICD employed different numberical constants to
+ * represent same/similar commands and this function is
+ * there to address that.
+ *
+ * @get_boot_source: Variant dependent implementaion of "get boot
+ * source" operation
+ * @set_boot_source: Variant dependent implementaion of "set boot
+ * source" operation
+ */
+struct rave_sp_variant_cmds {
+ int (*translate)(enum rave_sp_command);
+ int (*get_boot_source)(struct rave_sp *);
+ int (*set_boot_source)(struct rave_sp *, u8);
+};
+
+/**
+ * struct rave_sp_variant - RAVE supervisory processor core variant
+ *
+ * @checksum: Variant specific checksum implementation
+ * @cmd: Variant specific command vtable
+ * @init: Variant specific initialization sequence implementation
+ * @group: Attrubute group for exposed sysfs entries
+ */
+struct rave_sp_variant {
+ const struct rave_sp_checksum *checksum;
+ struct rave_sp_variant_cmds cmd;
+ void (*init)(struct rave_sp *);
+ struct attribute_group group;
+};
+
+/**
+ * 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
+ *
+ * @part_number_firmware: String containing firmware part number
+ * (retrived once during probing)
+ * @part_number_bootloader: String containing bootloader part number
+ * (retrived once during probing)
+ * @reset_reason: String containing reset reason
+ * (retrived once during probing)
+ * @copper_rev_rmb: String containing some copper revision
+ * (retrived once during probing)
+ * @copper_rev_deb: String containing some copper revision
+ * (retrived once during probing)
+ * @silicon_devid: String containing device uC's id
+ * (retrived once during probing)
+ * @silicon_devrev: String containing device uC's revision
+ * (retrived once during probing)
+ * @copper_mod_rmb: String containing additional copper revision
+ * (retrived once during probing)
+ * @copper_mod_deb: String containing additional copper revision
+ * (retrived once during probing)
+ *
+ * @variant: Device variant specific parameters and
+ * functions
+ * @event_notifier_list: Input event notification chain (used with
+ * corresponding input MFD cell driver)
+ * @group: Attrubute group for exposed sysfs entries
+ */
+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;
+};
+
+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
+devm_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(devm_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 const char *devm_rave_sp_version(struct device *dev, const char *buf)
+{
+ return devm_kasprintf(dev, GFP_KERNEL, "%02d%02d%02d.%c%c\n",
+ buf[0], le16_to_cpup((const __le16 *)&buf[1]),
+ buf[3], buf[4], buf[5]);
+}
+
+static int rave_sp_get_status(struct rave_sp *sp,
+ struct rave_sp_rsp_status *status)
+{
+ u8 cmd[] = {
+ [0] = RAVE_SP_CMD_STATUS,
+ [1] = 0
+ };
+ return rave_sp_exec(sp, cmd, sizeof(cmd), status, sizeof(*status));
+}
+
+static ssize_t
+rave_sp_show_part_number(char *buf, const char *version, size_t version_length)
+{
+ memcpy(buf, version, version_length + 1);
+ return version_length;
+}
+
+#define RAVE_SP_ATTR_RO_STRING(name) \
+ static ssize_t \
+ name##_show(struct device *dev, \
+ struct device_attribute *attr, \
+ char *buf) \
+ { \
+ struct rave_sp *sp = dev_get_drvdata(dev); \
+ return rave_sp_show_part_number(buf, sp->name, \
+ strlen(sp->name)); \
+ } \
+ static DEVICE_ATTR_RO(name)
+
+RAVE_SP_ATTR_RO_STRING(part_number_firmware);
+RAVE_SP_ATTR_RO_STRING(part_number_bootloader);
+RAVE_SP_ATTR_RO_STRING(copper_rev_rmb);
+RAVE_SP_ATTR_RO_STRING(copper_rev_deb);
+RAVE_SP_ATTR_RO_STRING(reset_reason);
+RAVE_SP_ATTR_RO_STRING(silicon_devrev);
+RAVE_SP_ATTR_RO_STRING(silicon_devid);
+
+RAVE_SP_ATTR_RO_STRING(copper_mod_rmb);
+RAVE_SP_ATTR_RO_STRING(copper_mod_deb);
+
+static ssize_t
+i2c_device_status_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ ssize_t ret;
+ struct rave_sp *sp = dev_get_drvdata(dev);
+ u8 status[2];
+ u8 cmd[] = {
+ [0] = RAVE_SP_CMD_GET_I2C_DEVICE_STATUS,
+ [1] = 0
+ };
+
+ ret = rave_sp_exec(sp, cmd, sizeof(cmd), &status, sizeof(status));
+ if (ret < 0)
+ return ret;
+
+ return sprintf(buf, "%04x\n", le16_to_cpup((__le16 *)status));
+}
+static DEVICE_ATTR_RO(i2c_device_status);
+
+static int rave_sp_rdu1_get_boot_source(struct rave_sp *sp)
+{
+ struct rave_sp_rsp_status status;
+ const int ret = rave_sp_get_status(sp, &status);
+
+ return (ret < 0) ? ret : (status.gs_format >> 2) & 0x03;
+}
+
+static int rave_sp_rdu1_set_boot_source(struct rave_sp *sp, u8 boot_source)
+{
+ return -ENOTSUPP;
+}
+
+static int rave_sp_common_set_boot_source(struct rave_sp *sp, u8 boot_source)
+{
+ u8 cmd[] = {
+ [0] = RAVE_SP_CMD_BOOT_SOURCE,
+ [1] = 0,
+ [2] = RAVE_SP_BOOT_SOURCE_GET,
+ [3] = boot_source,
+ };
+ return rave_sp_exec(sp, cmd, sizeof(cmd), NULL, 0);
+}
+
+static int rave_sp_common_get_boot_source(struct rave_sp *sp)
+{
+ u8 cmd[] = {
+ [0] = RAVE_SP_CMD_BOOT_SOURCE,
+ [1] = 0,
+ [2] = RAVE_SP_BOOT_SOURCE_SET,
+ [3] = 0,
+ };
+ u8 boot_source;
+ const int ret = rave_sp_exec(sp, cmd, sizeof(cmd),
+ &boot_source, sizeof(boot_source));
+
+ return (ret < 0) ? ret : boot_source;
+}
+
+static ssize_t rave_sp_show_boot_source(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct rave_sp *sp = dev_get_drvdata(dev);
+ const int ret = sp->variant->cmd.get_boot_source(sp);
+
+ return (ret < 0) ? ret : sprintf(buf, "%d\n", ret);
+}
+
+static ssize_t rave_sp_store_boot_source(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct rave_sp *sp = dev_get_drvdata(dev);
+ u8 boot_source;
+ int ret;
+
+ ret = kstrtou8(buf, 0, &boot_source);
+ if (ret)
+ return ret;
+
+ if (boot_source != RAVE_SP_BOOT_SOURCE_SD &&
+ boot_source != RAVE_SP_BOOT_SOURCE_EMMC &&
+ boot_source != RAVE_SP_BOOT_SOURCE_NOR)
+ return -EINVAL;
+
+ ret = sp->variant->cmd.set_boot_source(sp, boot_source);
+
+ return (ret < 0) ? ret : count;
+}
+
+static DEVICE_ATTR(boot_source, 0640,
+ rave_sp_show_boot_source, rave_sp_store_boot_source);
+
+static void devm_rave_sp_sysfs_group_release(struct device *dev, void *res)
+{
+ struct rave_sp *sp = *(struct rave_sp **)res;
+ const struct attribute_group *group = &sp->variant->group;
+ struct kobject *root = &sp->serdev->dev.kobj;
+
+ sysfs_remove_group(root, group);
+}
+
+static int devm_zii_sysfs_create_group(struct rave_sp *sp)
+{
+ struct rave_sp **rcsp;
+ struct device *dev = &sp->serdev->dev;
+ const struct attribute_group *group = &sp->variant->group;
+ struct kobject *root = &dev->kobj;
+ int ret;
+
+ rcsp = devres_alloc(devm_rave_sp_sysfs_group_release,
+ sizeof(*rcsp), GFP_KERNEL);
+ if (!rcsp)
+ return -ENOMEM;
+
+ ret = sysfs_create_group(root, group);
+ if (!ret) {
+ *rcsp = sp;
+ devres_add(dev, rcsp);
+ } else {
+ devres_free(rcsp);
+ }
+
+ return ret;
+}
+
+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);
+
+ *(__le16 *)crc = cpu_to_be16(calculated);
+}
+
+static void *stuff(unsigned char *dest, const unsigned char *src, size_t n)
+{
+ size_t i;
+
+ for (i = 0; i < n; i++) {
+ const unsigned char byte = *src++;
+
+ switch (byte) {
+ case RAVE_SP_STX:
+ case RAVE_SP_ETX:
+ case RAVE_SP_DLE:
+ *dest++ = RAVE_SP_DLE;
+ default:
+ *dest++ = byte;
+ }
+ }
+
+ return dest;
+}
+
+static int rave_sp_write(struct rave_sp *sp, const u8 *data, u8 data_size)
+{
+ size_t length;
+ const size_t checksum_length = sp->variant->checksum->length;
+ unsigned char crc[checksum_length];
+ unsigned char frame[RAVE_SP_TX_BUFFER_SIZE];
+ unsigned char *dest = frame;
+
+ 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)
+{
+ switch (command) {
+ case 0xA0 ... 0xBE:
+ return command + 0x20;
+ case 0xE0 ... 0xEF:
+ return command | 0x01;
+ default:
+ 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)
+{
+ int ret = 0;
+ unsigned char *data = __data;
+ const u8 ackid = (u8)atomic_inc_return(&sp->ackid);
+ const int command = sp->variant->cmd.translate(data[0]);
+ struct rave_sp_reply reply = {
+ .code = rave_sp_reply_code((u8)command),
+ .ackid = ackid,
+ .data = reply_data,
+ .length = reply_data_size,
+ .received = COMPLETION_INITIALIZER_ONSTACK(reply.received),
+ };
+
+ if (command < 0)
+ return command;
+
+ mutex_lock(&sp->bus_lock);
+
+ mutex_lock(&sp->reply_lock);
+ sp->reply = &reply;
+ mutex_unlock(&sp->reply_lock);
+
+ data[0] = (u8)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(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 && 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;
+ else
+ 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;
+ else if (command == RAVE_SP_CMD_REQ_COPPER_REV)
+ return 0x28;
+ else
+ return rave_sp_rdu1_cmd_translate(command);
+}
+
+static int rave_sp_default_cmd_translate(enum rave_sp_command command)
+{
+ switch (command) {
+ case RAVE_SP_CMD_GET_FIRMWARE_VERSION:
+ return 0x11;
+ case RAVE_SP_CMD_GET_BOOTLOADER_VERSION:
+ return 0x12;
+ case RAVE_SP_CMD_RESET_REASON:
+ return 0x1F;
+ case RAVE_SP_CMD_BOOT_SOURCE:
+ return 0x14;
+ case RAVE_SP_CMD_RESET:
+ return 0x1E;
+ case RAVE_SP_CMD_SW_WDT:
+ return 0x1C;
+ default:
+ return -EINVAL;
+ }
+}
+
+static void rave_sp_load_reset_reason(struct rave_sp *sp)
+{
+ struct device *dev = &sp->serdev->dev;
+ u8 cmd[] = {
+ [0] = RAVE_SP_CMD_RESET_REASON,
+ [1] = 0,
+ };
+ u8 reason;
+ int ret;
+
+ ret = rave_sp_exec(sp, cmd, sizeof(cmd), &reason, sizeof(reason));
+ if (!ret)
+ sp->reset_reason =
+ devm_kasprintf(dev, GFP_KERNEL, "%02x\n", reason);
+ else
+ dev_err(dev, "CMD_RESET_REASON failed %d\n", ret);
+}
+
+static void rave_sp_load_silicon_rev(struct rave_sp *sp)
+{
+ struct device *dev = &sp->serdev->dev;
+ u8 cmd[] = {
+ [0] = RAVE_SP_CMD_GET_SP_SILICON_REV,
+ [1] = 0
+ };
+ struct {
+ __le32 devid;
+ __le32 devrev;
+ } __packed reply;
+ int ret;
+
+ ret = rave_sp_exec(sp, cmd, sizeof(cmd), &reply, sizeof(reply));
+ if (!ret) {
+ sp->silicon_devid = devm_kasprintf(dev, GFP_KERNEL, "%08x\n",
+ le32_to_cpu(reply.devid));
+ sp->silicon_devrev = devm_kasprintf(dev, GFP_KERNEL, "%08x\n",
+ le32_to_cpu(reply.devrev));
+ } else {
+ dev_err(dev, "CMD_GET_SP_SILICON_REV failed %d\n", ret);
+ }
+}
+
+static void rave_sp_rdu1_init(struct rave_sp *sp)
+{
+ struct device *dev = &sp->serdev->dev;
+ u8 cmd[] = {
+ [0] = RAVE_SP_CMD_REQ_COPPER_REV,
+ [1] = 0
+ };
+ struct rave_sp_rsp_status status;
+ u8 revision[2];
+ int ret;
+
+ ret = rave_sp_get_status(sp, &status);
+ if (!ret) {
+ sp->part_number_firmware =
+ devm_rave_sp_version(dev, status.fw_bytes);
+ sp->part_number_bootloader =
+ devm_rave_sp_version(dev, status.bl_bytes);
+ } else {
+ dev_err(dev, "CMD_STATUS failed %d\n", ret);
+ }
+
+
+ ret = rave_sp_exec(sp, cmd, sizeof(cmd), &revision, sizeof(revision));
+ if (!ret) {
+ sp->copper_rev_rmb = devm_kasprintf(dev, GFP_KERNEL, "%02x\n",
+ revision[0]);
+ sp->copper_rev_deb = devm_kasprintf(dev, GFP_KERNEL, "%02x\n",
+ revision[1]);
+ } else {
+ dev_err(dev, "CMD_REQ_COPPER_REV failed %d\n", ret);
+ }
+}
+
+static void rave_sp_common_init(struct rave_sp *sp)
+{
+ struct device *dev = &sp->serdev->dev;
+ u8 version[6];
+ u8 cmd[2];
+ int ret;
+
+ cmd[0] = RAVE_SP_CMD_GET_FIRMWARE_VERSION;
+ ret = rave_sp_exec(sp, cmd, sizeof(cmd), version, sizeof(version));
+ if (!ret)
+ sp->part_number_firmware =
+ devm_rave_sp_version(dev, version);
+ else
+ dev_warn(dev, "CMD_GET_FIRMWARE_VERSION failed %d\n", ret);
+
+ cmd[0] = RAVE_SP_CMD_GET_BOOTLOADER_VERSION;
+ ret = rave_sp_exec(sp, cmd, sizeof(cmd), version, sizeof(version));
+ if (!ret)
+ sp->part_number_bootloader =
+ devm_rave_sp_version(dev, version);
+ else
+ dev_warn(dev, "CMD_GET_BOOTLOADER_VERSION failed %d\n", ret);
+}
+
+static void rave_sp_rdu2_init(struct rave_sp *sp)
+{
+ struct device *dev = &sp->serdev->dev;
+ u8 cmd[] = {
+ [0] = RAVE_SP_CMD_REQ_COPPER_REV,
+ [1] = 0,
+ [2] = RAVE_SP_RDU2_BOARD_TYPE_RMB,
+ };
+ u8 copper_rev;
+ int ret;
+
+ rave_sp_common_init(sp);
+
+ ret = rave_sp_exec(sp, cmd, sizeof(cmd),
+ &copper_rev, sizeof(copper_rev));
+ if (!ret) {
+ sp->copper_rev_rmb = devm_kasprintf(dev, GFP_KERNEL, "%01x\n",
+ copper_rev & 0x1F);
+ sp->copper_mod_rmb = devm_kasprintf(dev, GFP_KERNEL, "%01x\n",
+ copper_rev >> 5);
+ } else {
+ dev_warn(dev,
+ "RAVE_SP_CMD_REQ_COPPER_REV(RMB) failed %d\n", ret);
+ }
+
+ cmd[2] = RAVE_SP_RDU2_BOARD_TYPE_DEB;
+
+ ret = rave_sp_exec(sp, cmd, sizeof(cmd),
+ &copper_rev, sizeof(copper_rev));
+ if (!ret) {
+ sp->copper_rev_deb = devm_kasprintf(dev, GFP_KERNEL, "%01x\n",
+ copper_rev & 0x1F);
+ sp->copper_mod_deb = devm_kasprintf(dev, GFP_KERNEL, "%01x\n",
+ copper_rev >> 5);
+ } else {
+ dev_warn(dev,
+ "RAVE_SP_CMD_REQ_COPPER_REV(DEB) failed %d\n", ret);
+ }
+}
+
+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 struct attribute *rave_sp_common_attrs[] = {
+ &dev_attr_part_number_firmware.attr,
+ &dev_attr_part_number_bootloader.attr,
+ &dev_attr_boot_source.attr,
+ &dev_attr_reset_reason.attr,
+ NULL
+};
+
+static struct attribute *rave_sp_rdu1_attrs[] = {
+ &dev_attr_part_number_firmware.attr,
+ &dev_attr_part_number_bootloader.attr,
+ &dev_attr_boot_source.attr,
+ &dev_attr_reset_reason.attr,
+ &dev_attr_copper_rev_deb.attr,
+ &dev_attr_copper_rev_rmb.attr,
+ &dev_attr_i2c_device_status.attr,
+ &dev_attr_silicon_devid.attr,
+ &dev_attr_silicon_devrev.attr,
+
+ NULL
+};
+
+static struct attribute *rave_sp_rdu2_attrs[] = {
+ &dev_attr_part_number_firmware.attr,
+ &dev_attr_part_number_bootloader.attr,
+ &dev_attr_boot_source.attr,
+ &dev_attr_reset_reason.attr,
+ &dev_attr_copper_rev_deb.attr,
+ &dev_attr_copper_rev_rmb.attr,
+ &dev_attr_i2c_device_status.attr,
+ &dev_attr_silicon_devid.attr,
+ &dev_attr_silicon_devrev.attr,
+
+ &dev_attr_copper_mod_rmb.attr,
+ &dev_attr_copper_mod_deb.attr,
+
+ NULL
+};
+
+static const struct rave_sp_variant rave_sp_legacy = {
+ .checksum = &rave_sp_checksum_8b2c,
+ .cmd = {
+ .translate = rave_sp_default_cmd_translate,
+ .get_boot_source = rave_sp_common_get_boot_source,
+ .set_boot_source = rave_sp_common_set_boot_source,
+ },
+ .group = {
+ .attrs = rave_sp_common_attrs,
+ },
+ .init = rave_sp_common_init,
+};
+
+static const struct rave_sp_variant rave_sp_rdu1 = {
+ .checksum = &rave_sp_checksum_8b2c,
+ .cmd = {
+ .translate = rave_sp_rdu1_cmd_translate,
+ .get_boot_source = rave_sp_rdu1_get_boot_source,
+ .set_boot_source = rave_sp_rdu1_set_boot_source,
+ },
+ .group = {
+ .attrs = rave_sp_rdu1_attrs,
+ },
+ .init = rave_sp_rdu1_init,
+};
+
+static const struct rave_sp_variant rave_sp_rdu2 = {
+ .checksum = &rave_sp_checksum_ccitt,
+ .cmd = {
+ .translate = rave_sp_rdu2_cmd_translate,
+ .get_boot_source = rave_sp_common_get_boot_source,
+ .set_boot_source = rave_sp_common_set_boot_source,
+ },
+ .group = {
+ .attrs = rave_sp_rdu2_attrs,
+ },
+ .init = rave_sp_rdu2_init,
+};
+
+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 int rave_sp_probe(struct serdev_device *serdev)
+{
+ static const struct serdev_device_ops serdev_device_ops = {
+ .receive_buf = rave_sp_receive_buf,
+ .write_wakeup = serdev_device_write_wakeup,
+ };
+ struct rave_sp *sp;
+ struct device *dev = &serdev->dev;
+ const char *unknown = "unknown";
+ 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, &serdev_device_ops);
+ ret = serdev_device_open(serdev);
+ if (ret)
+ return ret;
+
+ serdev_device_set_baudrate(serdev, baud);
+
+ sp->silicon_devid = unknown;
+ sp->silicon_devrev = unknown;
+ sp->copper_rev_deb = unknown;
+ sp->copper_rev_rmb = unknown;
+ sp->copper_mod_deb = unknown;
+ sp->copper_mod_rmb = unknown;
+ sp->reset_reason = unknown;
+ sp->part_number_firmware = unknown;
+ sp->part_number_bootloader = unknown;
+
+ sp->variant->init(sp);
+
+ dev_info(dev, "Firmware version: %s\n", sp->part_number_firmware);
+ dev_info(dev, "Bootloader version: %s\n", sp->part_number_bootloader);
+
+ rave_sp_load_reset_reason(sp);
+ rave_sp_load_silicon_rev(sp);
+
+ ret = devm_zii_sysfs_create_group(sp);
+ if (ret) {
+ serdev_device_close(serdev);
+ return ret;
+ }
+
+ 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 = KBUILD_MODNAME,
+ .owner = THIS_MODULE,
+ .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..62baf89cbff1
--- /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(u8 event, u8 value)
+{
+ return ((unsigned long)value << 8) | event;
+}
+
+static inline u8 rave_sp_action_get_event(unsigned long action)
+{
+ return action & 0xff;
+}
+
+static inline u8 rave_sp_action_get_value(unsigned long action)
+{
+ return (action >> 8) & 0xff;
+}
+
+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.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 2/2] dt-bindings: mfd: Add bindings for ZII RAVE devices
2017-07-18 17:56 [PATCH v2 0/2] ZII RAVE platform driver Andrey Smirnov
2017-07-18 17:56 ` [PATCH v2 1/2] platform: Add driver for RAVE Supervisory Processor Andrey Smirnov
@ 2017-07-18 17:56 ` Andrey Smirnov
2017-07-20 9:04 ` Lee Jones
1 sibling, 1 reply; 9+ messages in thread
From: Andrey Smirnov @ 2017-07-18 17:56 UTC (permalink / raw)
To: linux-kernel
Cc: Andrey Smirnov, cphealy, Lucas Stach, Nikita Yushchenko,
Rob Herring, Mark Rutland, devicetree
Cc: cphealy@gmail.com
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: devicetree@vger.kernel.org
Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
.../devicetree/bindings/mfd/zii,rave-sp.txt | 39 ++++++++++++++++++++++
1 file changed, 39 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/zii,rave-sp.txt
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 000000000000..61d9105ded07
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/zii,rave-sp.txt
@@ -0,0 +1,39 @@
+Zodiac Inflight Innovations RAVE Supervisory Processor
+
+RAVE Supervisory Processor communicates with SoC over UART. It is
+expected that its device-tree node is specified as a child of a node
+corresponding to UART controller used for communication.
+
+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
+rave-sp-nvmem : Interface to onborad EEPROM
+rave-sp-backlight : Display backlight
+rave-sp-hwmon : Interface to onboard hardware sensors
+rave-sp-leds : Interface to onboard LEDs
+rave-sp-input : Interface to onboard power button
+
+Example of usage:
+
+ rdu {
+ compatible = "zii,rave-sp-rdu2";
+ current-speed = <1000000>;
+
+ watchdog {
+ compatible = "zii,rave-sp-watchdog";
+ };
+ };
+
--
2.13.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] platform: Add driver for RAVE Supervisory Processor
2017-07-18 17:56 ` [PATCH v2 1/2] platform: Add driver for RAVE Supervisory Processor Andrey Smirnov
@ 2017-07-18 18:48 ` Andy Shevchenko
2017-07-19 17:00 ` Andrey Smirnov
0 siblings, 1 reply; 9+ messages in thread
From: Andy Shevchenko @ 2017-07-18 18:48 UTC (permalink / raw)
To: Andrey Smirnov; +Cc: linux-kernel, Chris Healy, Lucas Stach, Nikita Yushchenko
On Tue, Jul 18, 2017 at 8:56 PM, Andrey Smirnov
<andrew.smirnov@gmail.com> 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.
> +/**
> + * 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
> + *
Do you need it?
> + * @part_number_firmware: String containing firmware part number
> + * (retrived once during probing)
No need to repeat this over each parameter. Just move it to (long)
description part and tell that
"part_*, copper_*, and silicon_* parameters are retrived once during probing."
> +static void
> +devm_rave_sp_unregister_event_notifier(struct device *dev, void *res)
static devm_*?!
It looks let's say interesting... (yes, better to provide releasing
devres API as well)
> +static const char *devm_rave_sp_version(struct device *dev, const char *buf)
> +{
> + return devm_kasprintf(dev, GFP_KERNEL, "%02d%02d%02d.%c%c\n",
> + buf[0], le16_to_cpup((const __le16 *)&buf[1]),
To cpu_P_?!
__le16 and %02d?
Are you sure?
> + buf[3], buf[4], buf[5]);
> +}
> +static ssize_t
> +rave_sp_show_part_number(char *buf, const char *version, size_t version_length)
> +{
> + memcpy(buf, version, version_length + 1);
> + return version_length;
Looks suspicious. If it's string, use snprintf(), it it might have
garbage, use %pE.
> +}
> + return sprintf(buf, "%04x\n", le16_to_cpup((__le16 *)status));
cpu_P_?!
> +static int rave_sp_common_get_boot_source(struct rave_sp *sp)
> +{
> + u8 cmd[] = {
> + [0] = RAVE_SP_CMD_BOOT_SOURCE,
> + [1] = 0,
> + [2] = RAVE_SP_BOOT_SOURCE_SET,
> + [3] = 0,
> + };
> + u8 boot_source;
> + const int ret = rave_sp_exec(sp, cmd, sizeof(cmd),
> + &boot_source, sizeof(boot_source));
Why not
const int ret;
ret = rave_sp_exec(sp, cmd, sizeof(cmd), &boot_source, sizeof(boot_source));
> + return (ret < 0) ? ret : boot_source;
Ditto for every similar cases.
> +}
> +static void csum_8b2c(const u8 *buf, size_t size, u8 *crc)
> +{
> + *crc = *buf++;
> + size--;
> +
> + while (size--)
*crc = 0;
while (size--)
*crc += *buf++;
?
> + *crc += *buf++;
> +
> + *crc = 1 + ~(*crc);
> +}
I dunno if lib/crc8.c helps here, worth to check.
> +static void *stuff(unsigned char *dest, const unsigned char *src, size_t n)
> +{
> + size_t i;
> +
> + for (i = 0; i < n; i++) {
Useless i.
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;
> + default:
> + *dest++ = byte;
> + }
> + }
> +
> + return dest;
> +}
> +static u8 rave_sp_reply_code(u8 command)
> +{
> + switch (command) {
> + case 0xA0 ... 0xBE:
> + return command + 0x20;
> + case 0xE0 ... 0xEF:
> + return command | 0x01;
These all look strange. Perhaps this function needs more comments.
> + default:
> + 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)
> +{
> + int ret = 0;
> + unsigned char *data = __data;
> + const u8 ackid = (u8)atomic_inc_return(&sp->ackid);
> + const int command = sp->variant->cmd.translate(data[0]);
> + struct rave_sp_reply reply = {
> + .code = rave_sp_reply_code((u8)command),
> + .ackid = ackid,
> + .data = reply_data,
> + .length = reply_data_size,
> + .received = COMPLETION_INITIALIZER_ONSTACK(reply.received),
> + };
> +
+ command = ...; here!
> + if (command < 0)
> + return command;
> +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 && 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]);
NULL pointer dereference.
> + 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);
... in all cases.
> + }
> +
> + mutex_unlock(&sp->reply_lock);
> +}
> +
> +static void rave_sp_receive_frame(struct rave_sp *sp,
> + const unsigned char *data,
> + size_t length)
> +{
> + 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_rdu1_cmd_translate(enum rave_sp_command command)
> +{
> + if (command >= RAVE_SP_CMD_STATUS &&
> + command <= RAVE_SP_CMD_CONTROL_EVENTS)
> + return command;
> + else
Redundant.
> + 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;
> + else if (command == RAVE_SP_CMD_REQ_COPPER_REV)
> + return 0x28;
> + else
> + return rave_sp_rdu1_cmd_translate(command);
Redundant else in both cases.
> +}
> +static int rave_sp_default_cmd_translate(enum rave_sp_command command)
> +{
> + switch (command) {
> + case RAVE_SP_CMD_GET_FIRMWARE_VERSION:
> + return 0x11;
> + case RAVE_SP_CMD_GET_BOOTLOADER_VERSION:
> + return 0x12;
> + case RAVE_SP_CMD_RESET_REASON:
> + return 0x1F;
> + case RAVE_SP_CMD_BOOT_SOURCE:
> + return 0x14;
> + case RAVE_SP_CMD_RESET:
> + return 0x1E;
> + case RAVE_SP_CMD_SW_WDT:
> + return 0x1C;
Can you set them in order of returned value?
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static void rave_sp_common_init(struct rave_sp *sp)
> +{
> + if (!ret)
Other way around, please.
> + sp->part_number_firmware =
> + devm_rave_sp_version(dev, version);
> + else
> + dev_warn(dev, "CMD_GET_FIRMWARE_VERSION failed %d\n", ret);
> +
> + cmd[0] = RAVE_SP_CMD_GET_BOOTLOADER_VERSION;
> + ret = rave_sp_exec(sp, cmd, sizeof(cmd), version, sizeof(version));
> + if (!ret)
Ditto.
Do this for every similar place.
> + sp->part_number_bootloader =
> + devm_rave_sp_version(dev, version);
> + else
> + dev_warn(dev, "CMD_GET_BOOTLOADER_VERSION failed %d\n", ret);
> +}
> +
> +static void rave_sp_rdu2_init(struct rave_sp *sp)
> +{
> + ret = rave_sp_exec(sp, cmd, sizeof(cmd),
> + &copper_rev, sizeof(copper_rev));
> + if (!ret) {
This is a bit unusual.
> + sp->copper_rev_rmb = devm_kasprintf(dev, GFP_KERNEL, "%01x\n",
> + copper_rev & 0x1F);
%01x and & 0x1f ?!
> + sp->copper_mod_rmb = devm_kasprintf(dev, GFP_KERNEL, "%01x\n",
> + copper_rev >> 5);
> + } else {
> + dev_warn(dev,
> + "RAVE_SP_CMD_REQ_COPPER_REV(RMB) failed %d\n", ret);
> + }
> +
> + cmd[2] = RAVE_SP_RDU2_BOARD_TYPE_DEB;
> +
> + ret = rave_sp_exec(sp, cmd, sizeof(cmd),
> + &copper_rev, sizeof(copper_rev));
> + if (!ret) {
> + sp->copper_rev_deb = devm_kasprintf(dev, GFP_KERNEL, "%01x\n",
> + copper_rev & 0x1F);
> + sp->copper_mod_deb = devm_kasprintf(dev, GFP_KERNEL, "%01x\n",
> + copper_rev >> 5);
Duplicate! Perhaps, helper function.
> + } else {
> + dev_warn(dev,
> + "RAVE_SP_CMD_REQ_COPPER_REV(DEB) failed %d\n", ret);
> + }
> +}
> +static int rave_sp_probe(struct serdev_device *serdev)
> +{
> + static const struct serdev_device_ops serdev_device_ops = {
> + .receive_buf = rave_sp_receive_buf,
> + .write_wakeup = serdev_device_write_wakeup,
> + };
Please, move outside.
> + struct rave_sp *sp;
> + struct device *dev = &serdev->dev;
Make it first line in this function.
> + u32 baud;
> + int ret;
> + mutex_init(&sp->bus_lock);
> + mutex_init(&sp->reply_lock);
> +static struct serdev_device_driver rave_sp_drv = {
> + .probe = rave_sp_probe,
> + .remove = rave_sp_remove,
> + .driver = {
> + .name = KBUILD_MODNAME,
> + .owner = THIS_MODULE,
Redundant I suppose.
> + .of_match_table = rave_sp_dt_ids,
> + },
> +};
> +static inline unsigned long rave_sp_action(u8 event, u8 value)
_pack_action?
> +{
> + return ((unsigned long)value << 8) | event;
> +}
> +
> +static inline u8 rave_sp_action_get_event(unsigned long action)
_unpack_event?
> +{
> + return action & 0xff;
> +}
> +
> +static inline u8 rave_sp_action_get_value(unsigned long action)
_unpack_value?
> +{
> + return (action >> 8) & 0xff;
> +}
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] platform: Add driver for RAVE Supervisory Processor
2017-07-18 18:48 ` Andy Shevchenko
@ 2017-07-19 17:00 ` Andrey Smirnov
0 siblings, 0 replies; 9+ messages in thread
From: Andrey Smirnov @ 2017-07-19 17:00 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: linux-kernel, Chris Healy, Lucas Stach, Nikita Yushchenko
On Tue, Jul 18, 2017 at 11:48 AM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Jul 18, 2017 at 8:56 PM, Andrey Smirnov
> <andrew.smirnov@gmail.com> 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.
>
>> +/**
>> + * 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
>
>> + *
>
> Do you need it?
Not sure what this is in reference too. The @reply field?
>
>> + * @part_number_firmware: String containing firmware part number
>
>> + * (retrived once during probing)
>
> No need to repeat this over each parameter. Just move it to (long)
> description part and tell that
> "part_*, copper_*, and silicon_* parameters are retrived once during probing."
>
OK, will change in v3.
>> +static void
>> +devm_rave_sp_unregister_event_notifier(struct device *dev, void *res)
>
> static devm_*?!
> It looks let's say interesting... (yes, better to provide releasing
> devres API as well)
>
It's just a callback provided to devres_alloc call below. It could and
should not be used as external API, but I can remove devm_ prefix from
the name if that makes things better. As for providing API for manual
unregistered -- there are no potential users of that functionality in
any of the corresponding MFD cell drivers, so I'd rather not add code
that'll never get used.
>> +static const char *devm_rave_sp_version(struct device *dev, const char *buf)
>> +{
>> + return devm_kasprintf(dev, GFP_KERNEL, "%02d%02d%02d.%c%c\n",
>> + buf[0], le16_to_cpup((const __le16 *)&buf[1]),
>
> To cpu_P_?!
Yes, I am not sure why you are surprised. It's used with &buf[1] and
it all seems legit, did I miss something?
> __le16 and %02d?
> Are you sure?
>
Yeah, legacy userspace daemon that, among other things, was in charge
of implementing similar to this driver's functionality was reporting
that particular version string in such buggy way. For the sake of
backwards compatibility I do the same. I'll add some commentary to
explain that.
>> + buf[3], buf[4], buf[5]);
>> +}
>
>> +static ssize_t
>> +rave_sp_show_part_number(char *buf, const char *version, size_t version_length)
>> +{
>> + memcpy(buf, version, version_length + 1);
>> + return version_length;
>
> Looks suspicious. If it's string, use snprintf(), it it might have
> garbage, use %pE.
>
Will change to snprintf in v3.
>> +}
>
>> + return sprintf(buf, "%04x\n", le16_to_cpup((__le16 *)status));
>
> cpu_P_?!
>
Status is declared as "u8 status[2]", so this should be OK, no?
>> +static int rave_sp_common_get_boot_source(struct rave_sp *sp)
>> +{
>> + u8 cmd[] = {
>> + [0] = RAVE_SP_CMD_BOOT_SOURCE,
>> + [1] = 0,
>> + [2] = RAVE_SP_BOOT_SOURCE_SET,
>> + [3] = 0,
>> + };
>> + u8 boot_source;
>
>> + const int ret = rave_sp_exec(sp, cmd, sizeof(cmd),
>> + &boot_source, sizeof(boot_source));
>
> Why not
>
> const int ret;
>
> ret = rave_sp_exec(sp, cmd, sizeof(cmd), &boot_source, sizeof(boot_source));
>
You'd have to drop the "const" qualifier from "ret" in order for that
snippet to compile, which I was trying to avoid. I'll drop the const
and move the call one line down in v3.
>> + return (ret < 0) ? ret : boot_source;
>
> Ditto for every similar cases.
>
>> +}
>
>> +static void csum_8b2c(const u8 *buf, size_t size, u8 *crc)
>> +{
>
>> + *crc = *buf++;
>
>> + size--;
>> +
>> + while (size--)
>
> *crc = 0;
>
> while (size--)
> *crc += *buf++;
>
> ?
I partially unrolled that look specifically because I wanted to avoid
having that "useless" "*crc = 0" line.
>
>> + *crc += *buf++;
>> +
>> + *crc = 1 + ~(*crc);
>> +}
>
> I dunno if lib/crc8.c helps here, worth to check.
>
I did check it out, but I am not sure I can use it: I am hesitant to
outright deny the possibility, but I can't think of a straight
conversion from "2's complement of sum of all bytes" to polynomial
division. Besides csum_8b2c() implements checksum algorithm as it is
described in ICD, so it has the advantage of being easier to double
check.
>> +static void *stuff(unsigned char *dest, const unsigned char *src, size_t n)
>> +{
>
>> + size_t i;
>> +
>> + for (i = 0; i < n; i++) {
>
> Useless i.
>
> while (n--) {
> ...
> }
>
Will change in v3.
>> + const unsigned char byte = *src++;
>> +
>> + switch (byte) {
>> + case RAVE_SP_STX:
>> + case RAVE_SP_ETX:
>> + case RAVE_SP_DLE:
>> + *dest++ = RAVE_SP_DLE;
>> + default:
>> + *dest++ = byte;
>> + }
>> + }
>> +
>> + return dest;
>> +}
>
>> +static u8 rave_sp_reply_code(u8 command)
>> +{
>> + switch (command) {
>> + case 0xA0 ... 0xBE:
>> + return command + 0x20;
>
>> + case 0xE0 ... 0xEF:
>
>> + return command | 0x01;
>
> These all look strange. Perhaps this function needs more comments.
>
Yeah, you're right, I'll add mover verbiage to explain what's going on there.
>> + default:
>> + 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)
>> +{
>> + int ret = 0;
>> + unsigned char *data = __data;
>> + const u8 ackid = (u8)atomic_inc_return(&sp->ackid);
>> + const int command = sp->variant->cmd.translate(data[0]);
>> + struct rave_sp_reply reply = {
>> + .code = rave_sp_reply_code((u8)command),
>> + .ackid = ackid,
>> + .data = reply_data,
>> + .length = reply_data_size,
>> + .received = COMPLETION_INITIALIZER_ONSTACK(reply.received),
>> + };
>> +
>
> + command = ...; here!
>
"reply.code" initialization depends on the value of "command", so I
can't easily move it down here.
>> + if (command < 0)
>> + return command;
>
>> +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 && 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]);
>
> NULL pointer dereference.
>
>> + 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);
>
> ... in all cases.
>
Good catch, thanks! Will fix in v3.
>> + }
>> +
>> + mutex_unlock(&sp->reply_lock);
>> +}
>> +
>> +static void rave_sp_receive_frame(struct rave_sp *sp,
>> + const unsigned char *data,
>> + size_t length)
>> +{
>
>> + 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_rdu1_cmd_translate(enum rave_sp_command command)
>> +{
>> + if (command >= RAVE_SP_CMD_STATUS &&
>> + command <= RAVE_SP_CMD_CONTROL_EVENTS)
>> + return command;
>
>> + else
>
> Redundant.
>
>> + 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;
>
>> + else if (command == RAVE_SP_CMD_REQ_COPPER_REV)
>> + return 0x28;
>> + else
>> + return rave_sp_rdu1_cmd_translate(command);
>
> Redundant else in both cases.
>
Will fix both in v3.
>> +}
>
>> +static int rave_sp_default_cmd_translate(enum rave_sp_command command)
>> +{
>> + switch (command) {
>> + case RAVE_SP_CMD_GET_FIRMWARE_VERSION:
>> + return 0x11;
>> + case RAVE_SP_CMD_GET_BOOTLOADER_VERSION:
>> + return 0x12;
>> + case RAVE_SP_CMD_RESET_REASON:
>
>> + return 0x1F;
>> + case RAVE_SP_CMD_BOOT_SOURCE:
>> + return 0x14;
>> + case RAVE_SP_CMD_RESET:
>> + return 0x1E;
>> + case RAVE_SP_CMD_SW_WDT:
>> + return 0x1C;
>
> Can you set them in order of returned value?
>
Sure, sounds like a good idea, will fix in v3.
>> + default:
>> + return -EINVAL;
>> + }
>> +}
>> +
>
>> +static void rave_sp_common_init(struct rave_sp *sp)
>> +{
>
>> + if (!ret)
>
> Other way around, please.
>
>> + sp->part_number_firmware =
>> + devm_rave_sp_version(dev, version);
>> + else
>> + dev_warn(dev, "CMD_GET_FIRMWARE_VERSION failed %d\n", ret);
>> +
>> + cmd[0] = RAVE_SP_CMD_GET_BOOTLOADER_VERSION;
>> + ret = rave_sp_exec(sp, cmd, sizeof(cmd), version, sizeof(version));
>
>> + if (!ret)
>
> Ditto.
>
> Do this for every similar place.
>
Will do in v3.
>> + sp->part_number_bootloader =
>> + devm_rave_sp_version(dev, version);
>> + else
>> + dev_warn(dev, "CMD_GET_BOOTLOADER_VERSION failed %d\n", ret);
>> +}
>> +
>
>> +static void rave_sp_rdu2_init(struct rave_sp *sp)
>> +{
>
>> + ret = rave_sp_exec(sp, cmd, sizeof(cmd),
>> + &copper_rev, sizeof(copper_rev));
>
>> + if (!ret) {
>
> This is a bit unusual.
>
>> + sp->copper_rev_rmb = devm_kasprintf(dev, GFP_KERNEL, "%01x\n",
>> + copper_rev & 0x1F);
>
> %01x and & 0x1f ?!
>
Ugh, missed this. Will change to "%02x" in v3.
>> + sp->copper_mod_rmb = devm_kasprintf(dev, GFP_KERNEL, "%01x\n",
>> + copper_rev >> 5);
>> + } else {
>> + dev_warn(dev,
>> + "RAVE_SP_CMD_REQ_COPPER_REV(RMB) failed %d\n", ret);
>> + }
>> +
>> + cmd[2] = RAVE_SP_RDU2_BOARD_TYPE_DEB;
>> +
>> + ret = rave_sp_exec(sp, cmd, sizeof(cmd),
>> + &copper_rev, sizeof(copper_rev));
>> + if (!ret) {
>
>> + sp->copper_rev_deb = devm_kasprintf(dev, GFP_KERNEL, "%01x\n",
>> + copper_rev & 0x1F);
>> + sp->copper_mod_deb = devm_kasprintf(dev, GFP_KERNEL, "%01x\n",
>> + copper_rev >> 5);
>
> Duplicate! Perhaps, helper function.
>
Will fix in v3.
>> + } else {
>> + dev_warn(dev,
>> + "RAVE_SP_CMD_REQ_COPPER_REV(DEB) failed %d\n", ret);
>> + }
>> +}
>
>> +static int rave_sp_probe(struct serdev_device *serdev)
>> +{
>
>> + static const struct serdev_device_ops serdev_device_ops = {
>> + .receive_buf = rave_sp_receive_buf,
>> + .write_wakeup = serdev_device_write_wakeup,
>> + };
>
> Please, move outside.
>
Sure, will fix in v3.
>> + struct rave_sp *sp;
>
>> + struct device *dev = &serdev->dev;
>
> Make it first line in this function.
>
OK, will fix in v3.
>> + u32 baud;
>> + int ret;
>
>> + mutex_init(&sp->bus_lock);
>> + mutex_init(&sp->reply_lock);
>
>> +static struct serdev_device_driver rave_sp_drv = {
>> + .probe = rave_sp_probe,
>> + .remove = rave_sp_remove,
>> + .driver = {
>> + .name = KBUILD_MODNAME,
>
>> + .owner = THIS_MODULE,
>
> Redundant I suppose.
>
OK, will drop in v3.
>> + .of_match_table = rave_sp_dt_ids,
>> + },
>> +};
>
>> +static inline unsigned long rave_sp_action(u8 event, u8 value)
>
> _pack_action?
>
>> +{
>> + return ((unsigned long)value << 8) | event;
>> +}
>> +
>> +static inline u8 rave_sp_action_get_event(unsigned long action)
>
> _unpack_event?
>
>> +{
>> + return action & 0xff;
>> +}
>> +
>> +static inline u8 rave_sp_action_get_value(unsigned long action)
>
> _unpack_value?
>
>> +{
>> + return (action >> 8) & 0xff;
>> +}
>
Yeah, sounds reasonable, will rename in v3.
Thanks,
Andrey Smirnov
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] dt-bindings: mfd: Add bindings for ZII RAVE devices
2017-07-18 17:56 ` [PATCH v2 2/2] dt-bindings: mfd: Add bindings for ZII RAVE devices Andrey Smirnov
@ 2017-07-20 9:04 ` Lee Jones
2017-07-20 14:26 ` Andrey Smirnov
0 siblings, 1 reply; 9+ messages in thread
From: Lee Jones @ 2017-07-20 9:04 UTC (permalink / raw)
To: Andrey Smirnov
Cc: linux-kernel, cphealy, Lucas Stach, Nikita Yushchenko,
Rob Herring, Mark Rutland, devicetree
Where is patch 1?
> Cc: cphealy@gmail.com
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: devicetree@vger.kernel.org
> Acked-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
> .../devicetree/bindings/mfd/zii,rave-sp.txt | 39 ++++++++++++++++++++++
> 1 file changed, 39 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/zii,rave-sp.txt
>
> 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 000000000000..61d9105ded07
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/zii,rave-sp.txt
> @@ -0,0 +1,39 @@
> +Zodiac Inflight Innovations RAVE Supervisory Processor
> +
> +RAVE Supervisory Processor communicates with SoC over UART. It is
> +expected that its device-tree node is specified as a child of a node
Nit: Device Tree
Once fixed, please apply my:
For my own reference:
Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
> +corresponding to UART controller used for communication.
> +
> +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
> +rave-sp-nvmem : Interface to onborad EEPROM
> +rave-sp-backlight : Display backlight
> +rave-sp-hwmon : Interface to onboard hardware sensors
> +rave-sp-leds : Interface to onboard LEDs
> +rave-sp-input : Interface to onboard power button
> +
> +Example of usage:
> +
> + rdu {
> + compatible = "zii,rave-sp-rdu2";
> + current-speed = <1000000>;
> +
> + watchdog {
> + compatible = "zii,rave-sp-watchdog";
> + };
> + };
> +
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] dt-bindings: mfd: Add bindings for ZII RAVE devices
2017-07-20 9:04 ` Lee Jones
@ 2017-07-20 14:26 ` Andrey Smirnov
2017-07-25 9:14 ` Lee Jones
0 siblings, 1 reply; 9+ messages in thread
From: Andrey Smirnov @ 2017-07-20 14:26 UTC (permalink / raw)
To: Lee Jones
Cc: linux-kernel, Chris Healy, Lucas Stach, Nikita Yushchenko,
Rob Herring, Mark Rutland, devicetree
On Thu, Jul 20, 2017 at 2:04 AM, Lee Jones <lee.jones@linaro.org> wrote:
> Where is patch 1?
>
lkml.kernel.org/r/20170718175604.11735-2-andrew.smirnov@gmail.com
I dropped you from CC list to spare you the e-mail traffic, but I can
add you back if you'd prefer that.
>> Cc: cphealy@gmail.com
>> Cc: Lucas Stach <l.stach@pengutronix.de>
>> Cc: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: devicetree@vger.kernel.org
>> Acked-by: Rob Herring <robh@kernel.org>
>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>> ---
>> .../devicetree/bindings/mfd/zii,rave-sp.txt | 39 ++++++++++++++++++++++
>> 1 file changed, 39 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/mfd/zii,rave-sp.txt
>>
>> 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 000000000000..61d9105ded07
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/zii,rave-sp.txt
>> @@ -0,0 +1,39 @@
>> +Zodiac Inflight Innovations RAVE Supervisory Processor
>> +
>> +RAVE Supervisory Processor communicates with SoC over UART. It is
>> +expected that its device-tree node is specified as a child of a node
>
> Nit: Device Tree
>
> Once fixed, please apply my:
>
> For my own reference:
> Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
>
OK. Will do.
Thanks,
Andrey Smirnov
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] dt-bindings: mfd: Add bindings for ZII RAVE devices
2017-07-20 14:26 ` Andrey Smirnov
@ 2017-07-25 9:14 ` Lee Jones
2017-07-25 12:38 ` Andrey Smirnov
0 siblings, 1 reply; 9+ messages in thread
From: Lee Jones @ 2017-07-25 9:14 UTC (permalink / raw)
To: Andrey Smirnov
Cc: linux-kernel, Chris Healy, Lucas Stach, Nikita Yushchenko,
Rob Herring, Mark Rutland, devicetree
On Thu, 20 Jul 2017, Andrey Smirnov wrote:
> On Thu, Jul 20, 2017 at 2:04 AM, Lee Jones <lee.jones@linaro.org> wrote:
> > Where is patch 1?
> >
>
> lkml.kernel.org/r/20170718175604.11735-2-andrew.smirnov@gmail.com
>
> I dropped you from CC list to spare you the e-mail traffic, but I can
> add you back if you'd prefer that.
It's always handy to see the C code which handles these bindings.
Also, it looks like there is an MFD change in that patch, so I should
have been Cc'ed anyway.
> >> Cc: cphealy@gmail.com
> >> Cc: Lucas Stach <l.stach@pengutronix.de>
> >> Cc: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
> >> Cc: Rob Herring <robh+dt@kernel.org>
> >> Cc: Mark Rutland <mark.rutland@arm.com>
> >> Cc: devicetree@vger.kernel.org
> >> Acked-by: Rob Herring <robh@kernel.org>
> >> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> >> ---
> >> .../devicetree/bindings/mfd/zii,rave-sp.txt | 39 ++++++++++++++++++++++
> >> 1 file changed, 39 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/mfd/zii,rave-sp.txt
> >>
> >> 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 000000000000..61d9105ded07
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/mfd/zii,rave-sp.txt
> >> @@ -0,0 +1,39 @@
> >> +Zodiac Inflight Innovations RAVE Supervisory Processor
> >> +
> >> +RAVE Supervisory Processor communicates with SoC over UART. It is
> >> +expected that its device-tree node is specified as a child of a node
> >
> > Nit: Device Tree
> >
> > Once fixed, please apply my:
> >
> > For my own reference:
> > Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
> >
>
> OK. Will do.
>
> Thanks,
> Andrey Smirnov
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] dt-bindings: mfd: Add bindings for ZII RAVE devices
2017-07-25 9:14 ` Lee Jones
@ 2017-07-25 12:38 ` Andrey Smirnov
0 siblings, 0 replies; 9+ messages in thread
From: Andrey Smirnov @ 2017-07-25 12:38 UTC (permalink / raw)
To: Lee Jones
Cc: linux-kernel, Chris Healy, Lucas Stach, Nikita Yushchenko,
Rob Herring, Mark Rutland, devicetree
On Tue, Jul 25, 2017 at 2:14 AM, Lee Jones <lee.jones@linaro.org> wrote:
> On Thu, 20 Jul 2017, Andrey Smirnov wrote:
>
>> On Thu, Jul 20, 2017 at 2:04 AM, Lee Jones <lee.jones@linaro.org> wrote:
>> > Where is patch 1?
>> >
>>
>> lkml.kernel.org/r/20170718175604.11735-2-andrew.smirnov@gmail.com
>>
>> I dropped you from CC list to spare you the e-mail traffic, but I can
>> add you back if you'd prefer that.
>
> It's always handy to see the C code which handles these bindings.
>
> Also, it looks like there is an MFD change in that patch, so I should
> have been Cc'ed anyway.
>
OK, I'll add you back on CC list for future versions of the patchset.
Thanks,
Andrey Smirnov
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-07-25 12:38 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-18 17:56 [PATCH v2 0/2] ZII RAVE platform driver Andrey Smirnov
2017-07-18 17:56 ` [PATCH v2 1/2] platform: Add driver for RAVE Supervisory Processor Andrey Smirnov
2017-07-18 18:48 ` Andy Shevchenko
2017-07-19 17:00 ` Andrey Smirnov
2017-07-18 17:56 ` [PATCH v2 2/2] dt-bindings: mfd: Add bindings for ZII RAVE devices Andrey Smirnov
2017-07-20 9:04 ` Lee Jones
2017-07-20 14:26 ` Andrey Smirnov
2017-07-25 9:14 ` Lee Jones
2017-07-25 12:38 ` Andrey Smirnov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).