[PATCH 0/3] mfd: cros_ec: Add user-space dev inferface support

[PATCH 0/3] mfd: cros_ec: Add user-space dev inferface support

[Javier Martinez Canillas]

Hello,

The mainline ChromeOS Embedded Controller (EC) driver is still missing some
features that are present in the downstream ChromiumOS tree. These are:

  - User-space device interface
  - Low Pin Count (LPC) interface
  - Access to vboot context stored on a block device
  - Access to vboot context stored on EC's nvram
  - Power Delivery Device
  - Support for multiple EC in a system

This series adds support for the first of these missing features: the cros
ec dev driver that provides the interface used by user-space to access the
EC. The support patches were taken from the downstream ChromiumOS 3.14
kernel tree with fixes and cleanups squashed to have a minimal patch-set.

The series is composed of the following patches:

Bill Richardson (3):
  mfd: cros_ec: Add Chrome OS EC userspace device interface
  mfd: cros_ec: Create sysfs attributes for the ChromeOS EC.
  mfd: cros_ec: Expose Chrome OS Lightbar to users

 drivers/mfd/Kconfig             |  12 ++
 drivers/mfd/Makefile            |   2 +
 drivers/mfd/cros_ec.c           |   4 +
 drivers/mfd/cros_ec_dev.c       | 363 ++++++++++++++++++++++++++++++++++++++++
 drivers/mfd/cros_ec_dev.h       |  29 ++++
 drivers/mfd/cros_ec_lightbar.c  | 347 ++++++++++++++++++++++++++++++++++++++
 drivers/mfd/cros_ec_sysfs.c     | 271 ++++++++++++++++++++++++++++++
 include/linux/mfd/cros_ec.h     |  12 +-
 include/linux/mfd/cros_ec_dev.h |  47 ++++++
 9 files changed, 1086 insertions(+), 1 deletion(-)
 create mode 100644 drivers/mfd/cros_ec_dev.c
 create mode 100644 drivers/mfd/cros_ec_dev.h
 create mode 100644 drivers/mfd/cros_ec_lightbar.c
 create mode 100644 drivers/mfd/cros_ec_sysfs.c
 create mode 100644 include/linux/mfd/cros_ec_dev.h

Patch #1 adds the cros_ec_dev driver that provides the dev interface to
user-space. Patch #2 adds sysfs entries that can be used to get information
and to control the EC and Patch #3 adds sysfs entries to control Chromebook
Pixel's four-element LED lightbar.

The patches were tested on a Exynos5420 Peach Pit Chromebook by using the
sysfs entries and the ectool.

Best regards,
Javier

[PATCH 1/3] mfd: cros_ec: Add Chrome OS EC userspace device interface

[Javier Martinez Canillas]

From: Bill Richardson <wfrichar@chromium.org>

This patch adds a device interface to access the
ChromeOS Embedded Controller from user-space.

Signed-off-by: Bill Richardson <wfrichar@chromium.org>
Reviewed-on: https://gerrit-int.chromium.org/38364
Reviewed-by: Simon Glass <sjg@google.com>
Tested-by: Simon Glass <sjg@google.com>
Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 drivers/mfd/Kconfig             |  12 ++
 drivers/mfd/Makefile            |   1 +
 drivers/mfd/cros_ec.c           |   4 +
 drivers/mfd/cros_ec_dev.c       | 355 ++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/cros_ec.h     |  12 +-
 include/linux/mfd/cros_ec_dev.h |  47 ++++++
 6 files changed, 430 insertions(+), 1 deletion(-)
 create mode 100644 drivers/mfd/cros_ec_dev.c
 create mode 100644 include/linux/mfd/cros_ec_dev.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 72d3808..31420a7 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -115,6 +115,18 @@ config MFD_CROS_EC_SPI
 	  response time cannot be guaranteed, we support ignoring
 	  'pre-amble' bytes before the response actually starts.
 
+config MFD_CROS_EC_DEV
+	tristate "ChromeOS Embedded Controller userspace device interface"
+	depends on MFD_CROS_EC
+
+	help
+	  If you say Y here, you get support for talking to the ChromeOS
+	  EC from userspace. This is separate from the normal kernel
+	  interfaces.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called cros_ec_dev.
+
 config MFD_ASIC3
 	bool "Compaq ASIC3"
 	depends on GPIOLIB && ARM
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 53467e2..5104a25 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_MFD_BCM590XX)	+= bcm590xx.o
 obj-$(CONFIG_MFD_CROS_EC)	+= cros_ec.o
 obj-$(CONFIG_MFD_CROS_EC_I2C)	+= cros_ec_i2c.o
 obj-$(CONFIG_MFD_CROS_EC_SPI)	+= cros_ec_spi.o
+obj-$(CONFIG_MFD_CROS_EC_DEV)	+= cros_ec_dev.o
 
 rtsx_pci-objs			:= rtsx_pcr.o rtsx_gops.o rts5209.o rts5229.o rtl8411.o rts5227.o rts5249.o
 obj-$(CONFIG_MFD_RTSX_PCI)	+= rtsx_pci.o
diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
index fc0c81e..197b273 100644
--- a/drivers/mfd/cros_ec.c
+++ b/drivers/mfd/cros_ec.c
@@ -119,6 +119,10 @@ static const struct mfd_cell cros_devs[] = {
 		.id = 2,
 		.of_compatible = "google,cros-ec-i2c-tunnel",
 	},
+	{
+		.name = "cros-ec-dev",
+		.id = 3,
+	},
 };
 
 int cros_ec_register(struct cros_ec_device *ec_dev)
diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
new file mode 100644
index 0000000..c18eb40
--- /dev/null
+++ b/drivers/mfd/cros_ec_dev.c
@@ -0,0 +1,355 @@
+/*
+ * cros_ec_dev - expose the Chrome OS Embedded Controller to userspace
+ *
+ * Copyright (C) 2014 Google, Inc.
+ *
+ * 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/>.
+ */
+
+#define pr_fmt(fmt) "cros_ec_dev: " fmt
+
+#include <linux/compat.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/fs.h>
+#include <linux/mfd/cros_ec.h>
+#include <linux/mfd/cros_ec_commands.h>
+#include <linux/mfd/cros_ec_dev.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/printk.h>
+#include <linux/types.h>
+#include <linux/uaccess.h>
+
+/* Device variables */
+#define CROS_CLASS_NAME "chromeos"
+static struct cros_ec_device *ec;
+static struct class *cros_class;
+static int ec_major;
+
+
+/* Basic communication */
+static int ec_get_version(struct cros_ec_device *ec, char *str, int maxlen)
+{
+	struct ec_response_get_version resp;
+	static const char * const current_image_name[] = {
+		"unknown", "read-only", "read-write", "invalid",
+	};
+	struct cros_ec_command msg = {
+		.version = 0,
+		.command = EC_CMD_GET_VERSION,
+		.outdata = NULL,
+		.outsize = 0,
+		.indata = (uint8_t *)&resp,
+		.insize = sizeof(resp),
+	};
+	int ret;
+
+	ret = cros_ec_cmd_xfer(ec, &msg);
+	if (ret < 0)
+		return ret;
+	if (msg.result != EC_RES_SUCCESS) {
+		snprintf(str, maxlen,
+			 "%s\nUnknown EC version: EC returned %d\n",
+			 CROS_EC_DEV_VERSION, msg.result);
+		return 0;
+	}
+	if (resp.current_image >= ARRAY_SIZE(current_image_name))
+		resp.current_image = 3; /* invalid */
+	snprintf(str, maxlen, "%s\n%s\n%s\n\%s\n", CROS_EC_DEV_VERSION,
+		 resp.version_string_ro, resp.version_string_rw,
+		 current_image_name[resp.current_image]);
+
+	return 0;
+}
+
+/* Device file ops */
+static int ec_device_open(struct inode *inode, struct file *filp)
+{
+	return 0;
+}
+
+static int ec_device_release(struct inode *inode, struct file *filp)
+{
+	return 0;
+}
+
+static ssize_t ec_device_read(struct file *filp, char __user *buffer,
+			      size_t length, loff_t *offset)
+{
+	char msg[sizeof(struct ec_response_get_version) +
+		 sizeof(CROS_EC_DEV_VERSION)];
+	size_t count;
+	int ret;
+
+	if (*offset != 0)
+		return 0;
+
+	ret = ec_get_version(ec, msg, sizeof(msg));
+	if (ret)
+		return ret;
+	count = min(length, strlen(msg));
+
+	if (copy_to_user(buffer, msg, count))
+		return -EFAULT;
+
+	*offset += count;
+	return count;
+}
+
+
+/* Ioctls */
+static long ec_device_ioctl_xcmd(void __user *argp)
+{
+	long ret;
+	struct cros_ec_command s_cmd;
+	uint8_t *user_indata;
+	uint8_t buf[EC_PROTO2_MAX_PARAM_SIZE];
+
+	if (copy_from_user(&s_cmd, argp, sizeof(s_cmd)))
+		return -EFAULT;
+	if (s_cmd.outsize &&
+	    copy_from_user(&buf, (void __user *)s_cmd.outdata, sizeof(buf)))
+		return -EFAULT;
+
+	user_indata = s_cmd.indata;
+	s_cmd.indata = buf;
+	s_cmd.outdata = buf;
+	ret = cros_ec_cmd_xfer(ec, &s_cmd);
+	s_cmd.indata = user_indata;
+
+	/* Only copy data to userland if data was received. */
+	if (ret > 0 && s_cmd.insize) {
+		unsigned size = ret;
+
+		size = min(size, s_cmd.insize);
+		if (copy_to_user((void __user *)s_cmd.indata, buf, size))
+			return -EFAULT;
+	}
+	if (copy_to_user(argp, &s_cmd, sizeof(s_cmd)))
+		return -EFAULT;
+
+	return ret;
+}
+
+static long ec_device_ioctl_readmem(void __user *argp)
+{
+	struct cros_ec_readmem s_mem;
+	char buf[EC_MEMMAP_SIZE];
+	long num;
+
+	/* Not every platform supports direct reads */
+	if (!ec->cmd_readmem)
+		return -ENOTTY;
+
+	if (copy_from_user(&s_mem, argp, sizeof(s_mem)))
+		return -EFAULT;
+	num = ec->cmd_readmem(ec, s_mem.offset, s_mem.bytes, buf);
+	if (num <= 0)
+		return num;
+	if (copy_to_user((void __user *)s_mem.buffer, buf, num))
+		return -EFAULT;
+	return num;
+}
+
+static long ec_device_ioctl(struct file *filp, unsigned int cmd,
+			    unsigned long arg)
+{
+	void __user *argp = (void __user *)arg;
+
+	if (_IOC_TYPE(cmd) != CROS_EC_DEV_IOC)
+		return -ENOTTY;
+
+	switch (cmd) {
+	case CROS_EC_DEV_IOCXCMD:
+		return ec_device_ioctl_xcmd(argp);
+	case CROS_EC_DEV_IOCRDMEM:
+		return ec_device_ioctl_readmem(argp);
+	}
+
+	return -ENOTTY;
+}
+
+#ifdef CONFIG_COMPAT
+struct compat_cros_ec_command {
+	uint32_t version;
+	uint32_t command;
+	compat_uptr_t outdata;
+	uint32_t outsize;
+	compat_uptr_t indata;
+	uint32_t insize;
+	uint32_t result;
+};
+
+struct compat_cros_ec_readmem {
+	uint32_t offset;
+	uint32_t bytes;
+	compat_uptr_t buffer;
+};
+
+#define CROS_EC_DEV_COMPAT_IOCXCMD  _IOWR(':', 0, struct compat_cros_ec_command)
+#define CROS_EC_DEV_COMPAT_IOCRDMEM _IOWR(':', 1, struct compat_cros_ec_readmem)
+
+static long ec_device_compat_ioctl_readmem(void __user *argp)
+{
+	struct compat_cros_ec_readmem compat_s_mem;
+	char buf[EC_MEMMAP_SIZE];
+	long num;
+
+	/* Not every platform supports direct reads */
+	if (!ec->cmd_readmem)
+		return -ENOTTY;
+
+	if (copy_from_user(&compat_s_mem, argp, sizeof(compat_s_mem)))
+		return -EFAULT;
+
+	num = ec->cmd_readmem(ec, compat_s_mem.offset, compat_s_mem.bytes, buf);
+	if (num <= 0)
+		return num;
+
+	if (copy_to_user(compat_ptr(compat_s_mem.buffer), buf, num))
+		return -EFAULT;
+	return num;
+}
+
+static long ec_device_compat_ioctl_xcmd(void __user *argp)
+{
+	long ret;
+	struct cros_ec_command s_cmd;
+	struct compat_cros_ec_command compat_s_cmd;
+	uint8_t buf[EC_PROTO2_MAX_PARAM_SIZE];
+
+	if (copy_from_user(&compat_s_cmd, argp, sizeof(compat_s_cmd)))
+		return -EFAULT;
+
+	s_cmd.version = compat_s_cmd.version;
+	s_cmd.command = compat_s_cmd.command;
+	s_cmd.insize = compat_s_cmd.insize;
+	s_cmd.outsize = compat_s_cmd.outsize;
+
+	if (s_cmd.outsize &&
+	    copy_from_user(&buf, compat_ptr(compat_s_cmd.outdata), sizeof(buf)))
+		return -EFAULT;
+
+	s_cmd.indata = buf;
+	s_cmd.outdata = buf;
+	ret = cros_ec_cmd_xfer(ec, &s_cmd);
+
+	compat_s_cmd.result = s_cmd.result;
+
+	/* Only copy data to userland if data was received. */
+	if (ret > 0 && s_cmd.insize) {
+		unsigned size = ret;
+
+		size = min(size, s_cmd.insize);
+		if (copy_to_user(compat_ptr(compat_s_cmd.indata), buf, size))
+			return -EFAULT;
+	}
+
+	if (copy_to_user(argp, &compat_s_cmd, sizeof(compat_s_cmd)))
+		return -EFAULT;
+
+	return ret;
+}
+
+static long ec_device_compat_ioctl(struct file *filp, unsigned int cmd,
+				   unsigned long arg)
+{
+	void __user
+	*argp = (void __user *)arg;
+	switch (cmd) {
+	case CROS_EC_DEV_COMPAT_IOCXCMD:
+		return ec_device_compat_ioctl_xcmd(argp);
+	case CROS_EC_DEV_COMPAT_IOCRDMEM:
+		return ec_device_compat_ioctl_readmem(argp);
+	}
+	return -ENOTTY;
+}
+#endif /* CONFIG_COMPAT */
+
+
+/* Module initialization */
+static const struct file_operations fops = {
+	.open = ec_device_open,
+	.release = ec_device_release,
+	.read = ec_device_read,
+	.unlocked_ioctl = ec_device_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl = ec_device_compat_ioctl,
+#endif
+};
+
+static int ec_device_probe(struct platform_device *pdev)
+{
+	int retval = -ENOTTY;
+
+	ec = dev_get_drvdata(pdev->dev.parent);
+
+	cros_class = class_create(THIS_MODULE, CROS_CLASS_NAME);
+	if (IS_ERR(cros_class)) {
+		pr_err("failed to register device class\n");
+		retval = PTR_ERR(cros_class);
+		goto failed_class;
+	}
+
+	/* Let the kernel pick the major num for us */
+	ec_major = register_chrdev(0, CROS_EC_DEV_NAME, &fops);
+	if (ec_major < 0) {
+		pr_err("failed to register device\n");
+		retval = ec_major;
+		goto failed_chrdevreg;
+	}
+
+	/* Instantiate it */
+	ec->vdev = device_create(cros_class, NULL, MKDEV(ec_major, 0),
+				 NULL, CROS_EC_DEV_NAME);
+	if (IS_ERR(ec->vdev)) {
+		pr_err("failed to create device\n");
+		retval = PTR_ERR(ec->vdev);
+		goto failed_devreg;
+	}
+
+	return 0;
+
+failed_devreg:
+	unregister_chrdev(ec_major, CROS_EC_DEV_NAME);
+failed_chrdevreg:
+	class_destroy(cros_class);
+failed_class:
+	return retval;
+}
+
+static int ec_device_remove(struct platform_device *pdev)
+{
+	device_destroy(cros_class, MKDEV(ec_major, 0));
+	unregister_chrdev(ec_major, CROS_EC_DEV_NAME);
+	class_destroy(cros_class);
+	return 0;
+}
+
+static struct platform_driver cros_ec_dev_driver = {
+	.driver = {
+		.name = "cros-ec-dev",
+		.owner = THIS_MODULE,
+	},
+	.probe = ec_device_probe,
+	.remove = ec_device_remove,
+};
+
+module_platform_driver(cros_ec_dev_driver);
+
+MODULE_AUTHOR("Bill Richardson <wfrichar@chromium.org>");
+MODULE_DESCRIPTION("Userspace interface to the Chrome OS Embedded Controller");
+MODULE_VERSION("1.0");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
index 0e166b9..b1cb934 100644
--- a/include/linux/mfd/cros_ec.h
+++ b/include/linux/mfd/cros_ec.h
@@ -59,9 +59,16 @@ struct cros_ec_command {
  *
  * @ec_name: name of EC device (e.g. 'chromeos-ec')
  * @phys_name: name of physical comms layer (e.g. 'i2c-4')
- * @dev: Device pointer
+ * @dev: Device pointer for physical comms device
+ * @vdev: Device pointer for virtual comms device
  * @was_wake_device: true if this device was set to wake the system from
  * sleep at the last suspend
+ * @cmd_read_mem: direct read of the EC memory-mapped region, if supported
+ *     @offset is within EC_LPC_ADDR_MEMMAP region.
+ *     @bytes: number of bytes to read. zero means "read a string" (including
+ *     the trailing '\0'). At most only EC_MEMMAP_SIZE bytes can be read.
+ *     Caller must ensure that the buffer is large enough for the result when
+ *     reading a string.
  *
  * @priv: Private data
  * @irq: Interrupt to use
@@ -90,8 +97,11 @@ struct cros_ec_device {
 	const char *ec_name;
 	const char *phys_name;
 	struct device *dev;
+	struct device *vdev;
 	bool was_wake_device;
 	struct class *cros_class;
+	int (*cmd_readmem)(struct cros_ec_device *ec, unsigned int offset,
+			   unsigned int bytes, void *dest);
 
 	/* These are used to implement the platform-specific interface */
 	void *priv;
diff --git a/include/linux/mfd/cros_ec_dev.h b/include/linux/mfd/cros_ec_dev.h
new file mode 100644
index 0000000..465e926
--- /dev/null
+++ b/include/linux/mfd/cros_ec_dev.h
@@ -0,0 +1,47 @@
+/*
+ * cros_ec_dev - expose the Chrome OS Embedded Controller to userspace
+ *
+ * Copyright (C) 2013 The Chromium OS Authors
+ *
+ * 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/>.
+ */
+
+#ifndef _CROS_EC_DEV_H_
+#define _CROS_EC_DEV_H_
+
+#include <linux/ioctl.h>
+#include <linux/types.h>
+#include <linux/mfd/cros_ec.h>
+
+#define CROS_EC_DEV_NAME "cros_ec"
+#define CROS_EC_DEV_VERSION "1.0.0"
+
+/*
+ * @offset: within EC_LPC_ADDR_MEMMAP region
+ * @bytes: number of bytes to read. zero means "read a string" (including '\0')
+ *         (at most only EC_MEMMAP_SIZE bytes can be read)
+ * @buffer: where to store the result
+ * ioctl returns the number of bytes read, negative on error
+ */
+struct cros_ec_readmem {
+	uint32_t offset;
+	uint32_t bytes;
+	char *buffer;
+};
+
+#define CROS_EC_DEV_IOC              ':'
+#define CROS_EC_DEV_IOCXCMD    _IOWR(':', 0, struct cros_ec_command)
+#define CROS_EC_DEV_IOCRDMEM   _IOWR(':', 1, struct cros_ec_readmem)
+
+#endif /* _CROS_EC_DEV_H_ */
-- 
2.1.0

Re: [PATCH 1/3] mfd: cros_ec: Add Chrome OS EC userspace device interface

[Lee Jones]

On Mon, 17 Nov 2014, Javier Martinez Canillas wrote:

> From: Bill Richardson <wfrichar@chromium.org>
> 
> This patch adds a device interface to access the
> ChromeOS Embedded Controller from user-space.
> 
> Signed-off-by: Bill Richardson <wfrichar@chromium.org>
> Reviewed-on: https://gerrit-int.chromium.org/38364
> Reviewed-by: Simon Glass <sjg@google.com>
> Tested-by: Simon Glass <sjg@google.com>
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> ---
>  drivers/mfd/Kconfig             |  12 ++
>  drivers/mfd/Makefile            |   1 +
>  drivers/mfd/cros_ec.c           |   4 +
>  drivers/mfd/cros_ec_dev.c       | 355 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/cros_ec.h     |  12 +-
>  include/linux/mfd/cros_ec_dev.h |  47 ++++++
>  6 files changed, 430 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/mfd/cros_ec_dev.c
>  create mode 100644 include/linux/mfd/cros_ec_dev.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 72d3808..31420a7 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -115,6 +115,18 @@ config MFD_CROS_EC_SPI
>  	  response time cannot be guaranteed, we support ignoring
>  	  'pre-amble' bytes before the response actually starts.
>  
> +config MFD_CROS_EC_DEV

_DEV as a post-fix doesn't really describe the driver.

> +	tristate "ChromeOS Embedded Controller userspace device interface"
> +	depends on MFD_CROS_EC
> +

Superfluous '\n'.

> +	help
> +	  If you say Y here, you get support for talking to the ChromeOS
> +	  EC from userspace. This is separate from the normal kernel
> +	  interfaces.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called cros_ec_dev.
> +
>  config MFD_ASIC3
>  	bool "Compaq ASIC3"
>  	depends on GPIOLIB && ARM
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 53467e2..5104a25 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_MFD_BCM590XX)	+= bcm590xx.o
>  obj-$(CONFIG_MFD_CROS_EC)	+= cros_ec.o
>  obj-$(CONFIG_MFD_CROS_EC_I2C)	+= cros_ec_i2c.o
>  obj-$(CONFIG_MFD_CROS_EC_SPI)	+= cros_ec_spi.o
> +obj-$(CONFIG_MFD_CROS_EC_DEV)	+= cros_ec_dev.o

Alphabetical?

>  rtsx_pci-objs			:= rtsx_pcr.o rtsx_gops.o rts5209.o rts5229.o rtl8411.o rts5227.o rts5249.o
>  obj-$(CONFIG_MFD_RTSX_PCI)	+= rtsx_pci.o
> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> index fc0c81e..197b273 100644
> --- a/drivers/mfd/cros_ec.c
> +++ b/drivers/mfd/cros_ec.c
> @@ -119,6 +119,10 @@ static const struct mfd_cell cros_devs[] = {
>  		.id = 2,
>  		.of_compatible = "google,cros-ec-i2c-tunnel",
>  	},
> +	{
> +		.name = "cros-ec-dev",
> +		.id = 3,
> +	},
>  };
>  
>  int cros_ec_register(struct cros_ec_device *ec_dev)
> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> new file mode 100644
> index 0000000..c18eb40
> --- /dev/null
> +++ b/drivers/mfd/cros_ec_dev.c
> @@ -0,0 +1,355 @@
> +/*
> + * cros_ec_dev - expose the Chrome OS Embedded Controller to userspace
> + *
> + * Copyright (C) 2014 Google, Inc.
> + *
> + * 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/>.
> + */
> +
> +#define pr_fmt(fmt) "cros_ec_dev: " fmt

This doesn't appear to even be used?

> +#include <linux/compat.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/fs.h>
> +#include <linux/mfd/cros_ec.h>
> +#include <linux/mfd/cros_ec_commands.h>
> +#include <linux/mfd/cros_ec_dev.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/printk.h>

What do you need this for?

> +#include <linux/types.h>
> +#include <linux/uaccess.h>
> +
> +/* Device variables */
> +#define CROS_CLASS_NAME "chromeos"

Any reason why you can't use the name directly?

> +static struct cros_ec_device *ec;

Can't you put this in a struct somewhere?

> +static struct class *cros_class;

Why this this global instead of using the one in 'struct
cros_ec_device'?

> +static int ec_major;

Same here.  Only use globals if you're forced to.

> +
> +

Superfluous '\n'.

> +/* Basic communication */
> +static int ec_get_version(struct cros_ec_device *ec, char *str, int maxlen)
> +{
> +	struct ec_response_get_version resp;
> +	static const char * const current_image_name[] = {
> +		"unknown", "read-only", "read-write", "invalid",
> +	};
> +	struct cros_ec_command msg = {
> +		.version = 0,
> +		.command = EC_CMD_GET_VERSION,
> +		.outdata = NULL,
> +		.outsize = 0,
> +		.indata = (uint8_t *)&resp,
> +		.insize = sizeof(resp),
> +	};
> +	int ret;
> +
> +	ret = cros_ec_cmd_xfer(ec, &msg);
> +	if (ret < 0)
> +		return ret;
> +	if (msg.result != EC_RES_SUCCESS) {
> +		snprintf(str, maxlen,
> +			 "%s\nUnknown EC version: EC returned %d\n",
> +			 CROS_EC_DEV_VERSION, msg.result);
> +		return 0;
> +	}
> +	if (resp.current_image >= ARRAY_SIZE(current_image_name))
> +		resp.current_image = 3; /* invalid */

Add a '\n' here.

> +	snprintf(str, maxlen, "%s\n%s\n%s\n\%s\n", CROS_EC_DEV_VERSION,
> +		 resp.version_string_ro, resp.version_string_rw,
> +		 current_image_name[resp.current_image]);
> +
> +	return 0;
> +}
> +
> +/* Device file ops */
> +static int ec_device_open(struct inode *inode, struct file *filp)
> +{
> +	return 0;
> +}
> +
> +static int ec_device_release(struct inode *inode, struct file *filp)
> +{
> +	return 0;
> +}
> +
> +static ssize_t ec_device_read(struct file *filp, char __user *buffer,
> +			      size_t length, loff_t *offset)
> +{
> +	char msg[sizeof(struct ec_response_get_version) +
> +		 sizeof(CROS_EC_DEV_VERSION)];
> +	size_t count;
> +	int ret;
> +
> +	if (*offset != 0)
> +		return 0;
> +
> +	ret = ec_get_version(ec, msg, sizeof(msg));
> +	if (ret)
> +		return ret;

New line here.  Not sure why you are bunching up these segments.

> +	count = min(length, strlen(msg));
> +
> +	if (copy_to_user(buffer, msg, count))
> +		return -EFAULT;
> +
> +	*offset += count;

You know offset is 0 here, so you can just *offset = count.

> +	return count;
> +}
> +
> +

Please remove these double line spaces throughout.

> +/* Ioctls */
> +static long ec_device_ioctl_xcmd(void __user *argp)
> +{
> +	long ret;
> +	struct cros_ec_command s_cmd;
> +	uint8_t *user_indata;
> +	uint8_t buf[EC_PROTO2_MAX_PARAM_SIZE];
> +
> +	if (copy_from_user(&s_cmd, argp, sizeof(s_cmd)))
> +		return -EFAULT;
> +	if (s_cmd.outsize &&
> +	    copy_from_user(&buf, (void __user *)s_cmd.outdata, sizeof(buf)))
> +		return -EFAULT;
> +
> +	user_indata = s_cmd.indata;
> +	s_cmd.indata = buf;
> +	s_cmd.outdata = buf;
> +	ret = cros_ec_cmd_xfer(ec, &s_cmd);
> +	s_cmd.indata = user_indata;

For conforming if you swap these round, place a '\n' after this line
and move the check for 'ret' to just after the call to
cros_ec_cmd_xfer().

> +	/* Only copy data to userland if data was received. */
> +	if (ret > 0 && s_cmd.insize) {
> +		unsigned size = ret;
> +
> +		size = min(size, s_cmd.insize);
> +		if (copy_to_user((void __user *)s_cmd.indata, buf, size))
> +			return -EFAULT;
> +	}
> +	if (copy_to_user(argp, &s_cmd, sizeof(s_cmd)))
> +		return -EFAULT;
> +
> +	return ret;
> +}
> +
> +static long ec_device_ioctl_readmem(void __user *argp)
> +{
> +	struct cros_ec_readmem s_mem;
> +	char buf[EC_MEMMAP_SIZE];
> +	long num;
> +
> +	/* Not every platform supports direct reads */
> +	if (!ec->cmd_readmem)
> +		return -ENOTTY;
> +
> +	if (copy_from_user(&s_mem, argp, sizeof(s_mem)))
> +		return -EFAULT;
> +	num = ec->cmd_readmem(ec, s_mem.offset, s_mem.bytes, buf);
> +	if (num <= 0)
> +		return num;
> +	if (copy_to_user((void __user *)s_mem.buffer, buf, num))
> +		return -EFAULT;
> +	return num;
> +}
> +
> +static long ec_device_ioctl(struct file *filp, unsigned int cmd,
> +			    unsigned long arg)
> +{
> +	void __user *argp = (void __user *)arg;
> +
> +	if (_IOC_TYPE(cmd) != CROS_EC_DEV_IOC)
> +		return -ENOTTY;
> +
> +	switch (cmd) {
> +	case CROS_EC_DEV_IOCXCMD:
> +		return ec_device_ioctl_xcmd(argp);
> +	case CROS_EC_DEV_IOCRDMEM:
> +		return ec_device_ioctl_readmem(argp);
> +	}
> +
> +	return -ENOTTY;
> +}
> +
> +#ifdef CONFIG_COMPAT
> +struct compat_cros_ec_command {
> +	uint32_t version;
> +	uint32_t command;
> +	compat_uptr_t outdata;
> +	uint32_t outsize;
> +	compat_uptr_t indata;
> +	uint32_t insize;
> +	uint32_t result;
> +};
> +
> +struct compat_cros_ec_readmem {
> +	uint32_t offset;
> +	uint32_t bytes;
> +	compat_uptr_t buffer;
> +};
> +
> +#define CROS_EC_DEV_COMPAT_IOCXCMD  _IOWR(':', 0, struct compat_cros_ec_command)
> +#define CROS_EC_DEV_COMPAT_IOCRDMEM _IOWR(':', 1, struct compat_cros_ec_readmem)
> +
> +static long ec_device_compat_ioctl_readmem(void __user *argp)
> +{
> +	struct compat_cros_ec_readmem compat_s_mem;
> +	char buf[EC_MEMMAP_SIZE];
> +	long num;
> +
> +	/* Not every platform supports direct reads */
> +	if (!ec->cmd_readmem)
> +		return -ENOTTY;
> +
> +	if (copy_from_user(&compat_s_mem, argp, sizeof(compat_s_mem)))
> +		return -EFAULT;
> +
> +	num = ec->cmd_readmem(ec, compat_s_mem.offset, compat_s_mem.bytes, buf);
> +	if (num <= 0)
> +		return num;
> +
> +	if (copy_to_user(compat_ptr(compat_s_mem.buffer), buf, num))
> +		return -EFAULT;
> +	return num;
> +}
> +
> +static long ec_device_compat_ioctl_xcmd(void __user *argp)
> +{
> +	long ret;
> +	struct cros_ec_command s_cmd;
> +	struct compat_cros_ec_command compat_s_cmd;
> +	uint8_t buf[EC_PROTO2_MAX_PARAM_SIZE];
> +
> +	if (copy_from_user(&compat_s_cmd, argp, sizeof(compat_s_cmd)))
> +		return -EFAULT;
> +
> +	s_cmd.version = compat_s_cmd.version;
> +	s_cmd.command = compat_s_cmd.command;
> +	s_cmd.insize = compat_s_cmd.insize;
> +	s_cmd.outsize = compat_s_cmd.outsize;
> +
> +	if (s_cmd.outsize &&
> +	    copy_from_user(&buf, compat_ptr(compat_s_cmd.outdata), sizeof(buf)))
> +		return -EFAULT;
> +
> +	s_cmd.indata = buf;
> +	s_cmd.outdata = buf;
> +	ret = cros_ec_cmd_xfer(ec, &s_cmd);
> +
> +	compat_s_cmd.result = s_cmd.result;
> +
> +	/* Only copy data to userland if data was received. */
> +	if (ret > 0 && s_cmd.insize) {
> +		unsigned size = ret;
> +
> +		size = min(size, s_cmd.insize);
> +		if (copy_to_user(compat_ptr(compat_s_cmd.indata), buf, size))
> +			return -EFAULT;
> +	}
> +
> +	if (copy_to_user(argp, &compat_s_cmd, sizeof(compat_s_cmd)))
> +		return -EFAULT;
> +
> +	return ret;
> +}
> +
> +static long ec_device_compat_ioctl(struct file *filp, unsigned int cmd,
> +				   unsigned long arg)
> +{
> +	void __user
> +	*argp = (void __user *)arg;

Why is this expressed over multiple lines?

> +	switch (cmd) {
> +	case CROS_EC_DEV_COMPAT_IOCXCMD:
> +		return ec_device_compat_ioctl_xcmd(argp);
> +	case CROS_EC_DEV_COMPAT_IOCRDMEM:
> +		return ec_device_compat_ioctl_readmem(argp);
> +	}

'\n'

> +	return -ENOTTY;
> +}
> +#endif /* CONFIG_COMPAT */
> +
> +

Please remove.

> +/* Module initialization */
> +static const struct file_operations fops = {
> +	.open = ec_device_open,
> +	.release = ec_device_release,
> +	.read = ec_device_read,
> +	.unlocked_ioctl = ec_device_ioctl,
> +#ifdef CONFIG_COMPAT
> +	.compat_ioctl = ec_device_compat_ioctl,
> +#endif
> +};
> +
> +static int ec_device_probe(struct platform_device *pdev)
> +{
> +	int retval = -ENOTTY;
> +
> +	ec = dev_get_drvdata(pdev->dev.parent);

Stick this as the first line in the function.

> +	cros_class = class_create(THIS_MODULE, CROS_CLASS_NAME);
> +	if (IS_ERR(cros_class)) {
> +		pr_err("failed to register device class\n");

Why are you using pr_err() over dev_err()?

> +		retval = PTR_ERR(cros_class);
> +		goto failed_class;

Remove this goto and the corresponding label and just return from here.

> +	}
> +
> +	/* Let the kernel pick the major num for us */
> +	ec_major = register_chrdev(0, CROS_EC_DEV_NAME, &fops);
> +	if (ec_major < 0) {
> +		pr_err("failed to register device\n");
> +		retval = ec_major;
> +		goto failed_chrdevreg;
> +	}
> +
> +	/* Instantiate it */
> +	ec->vdev = device_create(cros_class, NULL, MKDEV(ec_major, 0),
> +				 NULL, CROS_EC_DEV_NAME);
> +	if (IS_ERR(ec->vdev)) {
> +		pr_err("failed to create device\n");
> +		retval = PTR_ERR(ec->vdev);
> +		goto failed_devreg;
> +	}
> +
> +	return 0;
> +
> +failed_devreg:
> +	unregister_chrdev(ec_major, CROS_EC_DEV_NAME);
> +failed_chrdevreg:
> +	class_destroy(cros_class);
> +failed_class:
> +	return retval;
> +}
> +
> +static int ec_device_remove(struct platform_device *pdev)
> +{
> +	device_destroy(cros_class, MKDEV(ec_major, 0));
> +	unregister_chrdev(ec_major, CROS_EC_DEV_NAME);
> +	class_destroy(cros_class);
> +	return 0;
> +}
> +
> +static struct platform_driver cros_ec_dev_driver = {
> +	.driver = {
> +		.name = "cros-ec-dev",
> +		.owner = THIS_MODULE,

Remove this line.

> +	},
> +	.probe = ec_device_probe,
> +	.remove = ec_device_remove,
> +};

Where is this device registered from?

> +module_platform_driver(cros_ec_dev_driver);
> +
> +MODULE_AUTHOR("Bill Richardson <wfrichar@chromium.org>");
> +MODULE_DESCRIPTION("Userspace interface to the Chrome OS Embedded Controller");
> +MODULE_VERSION("1.0");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> index 0e166b9..b1cb934 100644
> --- a/include/linux/mfd/cros_ec.h
> +++ b/include/linux/mfd/cros_ec.h
> @@ -59,9 +59,16 @@ struct cros_ec_command {
>   *
>   * @ec_name: name of EC device (e.g. 'chromeos-ec')
>   * @phys_name: name of physical comms layer (e.g. 'i2c-4')
> - * @dev: Device pointer
> + * @dev: Device pointer for physical comms device
> + * @vdev: Device pointer for virtual comms device
>   * @was_wake_device: true if this device was set to wake the system from
>   * sleep at the last suspend
> + * @cmd_read_mem: direct read of the EC memory-mapped region, if supported
> + *     @offset is within EC_LPC_ADDR_MEMMAP region.
> + *     @bytes: number of bytes to read. zero means "read a string" (including
> + *     the trailing '\0'). At most only EC_MEMMAP_SIZE bytes can be read.
> + *     Caller must ensure that the buffer is large enough for the result when
> + *     reading a string.
>   *
>   * @priv: Private data
>   * @irq: Interrupt to use
> @@ -90,8 +97,11 @@ struct cros_ec_device {
>  	const char *ec_name;
>  	const char *phys_name;
>  	struct device *dev;
> +	struct device *vdev;
>  	bool was_wake_device;
>  	struct class *cros_class;
> +	int (*cmd_readmem)(struct cros_ec_device *ec, unsigned int offset,
> +			   unsigned int bytes, void *dest);
>  
>  	/* These are used to implement the platform-specific interface */
>  	void *priv;
> diff --git a/include/linux/mfd/cros_ec_dev.h b/include/linux/mfd/cros_ec_dev.h
> new file mode 100644
> index 0000000..465e926
> --- /dev/null
> +++ b/include/linux/mfd/cros_ec_dev.h
> @@ -0,0 +1,47 @@
> +/*
> + * cros_ec_dev - expose the Chrome OS Embedded Controller to userspace
> + *
> + * Copyright (C) 2013 The Chromium OS Authors
> + *
> + * 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/>.
> + */
> +
> +#ifndef _CROS_EC_DEV_H_
> +#define _CROS_EC_DEV_H_
> +
> +#include <linux/ioctl.h>
> +#include <linux/types.h>
> +#include <linux/mfd/cros_ec.h>
> +
> +#define CROS_EC_DEV_NAME "cros_ec"
> +#define CROS_EC_DEV_VERSION "1.0.0"
> +
> +/*
> + * @offset: within EC_LPC_ADDR_MEMMAP region
> + * @bytes: number of bytes to read. zero means "read a string" (including '\0')
> + *         (at most only EC_MEMMAP_SIZE bytes can be read)
> + * @buffer: where to store the result
> + * ioctl returns the number of bytes read, negative on error
> + */
> +struct cros_ec_readmem {
> +	uint32_t offset;
> +	uint32_t bytes;
> +	char *buffer;
> +};
> +
> +#define CROS_EC_DEV_IOC              ':'
> +#define CROS_EC_DEV_IOCXCMD    _IOWR(':', 0, struct cros_ec_command)
> +#define CROS_EC_DEV_IOCRDMEM   _IOWR(':', 1, struct cros_ec_readmem)
> +
> +#endif /* _CROS_EC_DEV_H_ */

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

Re: [PATCH 1/3] mfd: cros_ec: Add Chrome OS EC userspace device interface

[Javier Martinez Canillas]

Hello Lee,

Thanks a lot for your feedback.

On 11/18/2014 03:18 PM, Lee Jones wrote:
>>  
>> +config MFD_CROS_EC_DEV
> 
> _DEV as a post-fix doesn't really describe the driver.
>

the _DEV is meant to denote (user-space device interface) but is true that
a better name can be used, I'll see what I can came with.
 
>> +	tristate "ChromeOS Embedded Controller userspace device interface"
>> +	depends on MFD_CROS_EC
>> +
> 
> Superfluous '\n'.
> 

Ok

>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -12,6 +12,7 @@ obj-$(CONFIG_MFD_BCM590XX)	+= bcm590xx.o
>>  obj-$(CONFIG_MFD_CROS_EC)	+= cros_ec.o
>>  obj-$(CONFIG_MFD_CROS_EC_I2C)	+= cros_ec_i2c.o
>>  obj-$(CONFIG_MFD_CROS_EC_SPI)	+= cros_ec_spi.o
>> +obj-$(CONFIG_MFD_CROS_EC_DEV)	+= cros_ec_dev.o
> 
> Alphabetical?
>

Ok

>> +
>> +#define pr_fmt(fmt) "cros_ec_dev: " fmt
> 
> This doesn't appear to even be used?
>

It is used by pr_* family of functions, e.g: from include/linux/printk.h

#define pr_err(fmt, ...) \
         printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
 
>> +#include <linux/compat.h>
>> +#include <linux/delay.h>
>> +#include <linux/device.h>
>> +#include <linux/fs.h>
>> +#include <linux/mfd/cros_ec.h>
>> +#include <linux/mfd/cros_ec_commands.h>
>> +#include <linux/mfd/cros_ec_dev.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/printk.h>
> 
> What do you need this for?
>

the printk.h header? to use the pr_* functions but I'll make sure that only
the needed headers are included.
 
>> +#include <linux/types.h>
>> +#include <linux/uaccess.h>
>> +
>> +/* Device variables */
>> +#define CROS_CLASS_NAME "chromeos"
> 
> Any reason why you can't use the name directly?
> 

I prefer macros if possible since they cost nothing and give you an indirection
level if you want to change it later. Any reason to not use a define directive?

>> +static struct cros_ec_device *ec;
> 
> Can't you put this in a struct somewhere?
>

Yes, I'll remove the global variable and put it in struct file .private_data
 
>> +static struct class *cros_class;
> 
> Why this this global instead of using the one in 'struct
> cros_ec_device'?
> 
>> +static int ec_major;
> 
> Same here.  Only use globals if you're forced to.
>

Ok, I'll look how to refactor to remove these globals too.
 
>> +
>> +
> 
> Superfluous '\n'.
> 

Ok

>> +
>> +	ret = cros_ec_cmd_xfer(ec, &msg);
>> +	if (ret < 0)
>> +		return ret;
>> +	if (msg.result != EC_RES_SUCCESS) {
>> +		snprintf(str, maxlen,
>> +			 "%s\nUnknown EC version: EC returned %d\n",
>> +			 CROS_EC_DEV_VERSION, msg.result);
>> +		return 0;
>> +	}
>> +	if (resp.current_image >= ARRAY_SIZE(current_image_name))
>> +		resp.current_image = 3; /* invalid */
> 
> Add a '\n' here.
>

Ok

>> +	if (*offset != 0)
>> +		return 0;
>> +
>> +	ret = ec_get_version(ec, msg, sizeof(msg));
>> +	if (ret)
>> +		return ret;
> 
> New line here.  Not sure why you are bunching up these segments.
> 

Ok

>> +	count = min(length, strlen(msg));
>> +
>> +	if (copy_to_user(buffer, msg, count))
>> +		return -EFAULT;
>> +
>> +	*offset += count;
> 
> You know offset is 0 here, so you can just *offset = count.
> 

True

>> +	return count;
>> +}
>> +
>> +
> 
> Please remove these double line spaces throughout.
>

Ok
 
>> +/* Ioctls */
>> +static long ec_device_ioctl_xcmd(void __user *argp)
>> +{
>> +	long ret;
>> +	struct cros_ec_command s_cmd;
>> +	uint8_t *user_indata;
>> +	uint8_t buf[EC_PROTO2_MAX_PARAM_SIZE];
>> +
>> +	if (copy_from_user(&s_cmd, argp, sizeof(s_cmd)))
>> +		return -EFAULT;
>> +	if (s_cmd.outsize &&
>> +	    copy_from_user(&buf, (void __user *)s_cmd.outdata, sizeof(buf)))
>> +		return -EFAULT;
>> +
>> +	user_indata = s_cmd.indata;
>> +	s_cmd.indata = buf;
>> +	s_cmd.outdata = buf;
>> +	ret = cros_ec_cmd_xfer(ec, &s_cmd);
>> +	s_cmd.indata = user_indata;
> 
> For conforming if you swap these round, place a '\n' after this line
> and move the check for 'ret' to just after the call to
> cros_ec_cmd_xfer().
>

Ok
 
>> +static long ec_device_compat_ioctl(struct file *filp, unsigned int cmd,
>> +				   unsigned long arg)
>> +{
>> +	void __user
>> +	*argp = (void __user *)arg;
> 
> Why is this expressed over multiple lines?
>

No reason at all, I'll change it.
 
>> +	switch (cmd) {
>> +	case CROS_EC_DEV_COMPAT_IOCXCMD:
>> +		return ec_device_compat_ioctl_xcmd(argp);
>> +	case CROS_EC_DEV_COMPAT_IOCRDMEM:
>> +		return ec_device_compat_ioctl_readmem(argp);
>> +	}
> 
> '\n'
>

Ok
 
>> +	return -ENOTTY;
>> +}
>> +#endif /* CONFIG_COMPAT */
>> +
>> +
> 
> Please remove.
> 

Ok

>> +static int ec_device_probe(struct platform_device *pdev)
>> +{
>> +	int retval = -ENOTTY;
>> +
>> +	ec = dev_get_drvdata(pdev->dev.parent);
> 
> Stick this as the first line in the function.
>

Ok
 
>> +	cros_class = class_create(THIS_MODULE, CROS_CLASS_NAME);
>> +	if (IS_ERR(cros_class)) {
>> +		pr_err("failed to register device class\n");
> 
> Why are you using pr_err() over dev_err()?
>

You are right, I'll use dev_err() instead.

>> +		retval = PTR_ERR(cros_class);
>> +		goto failed_class;
> 
> Remove this goto and the corresponding label and just return from here.
>

Ok

>> +
>> +static struct platform_driver cros_ec_dev_driver = {
>> +	.driver = {
>> +		.name = "cros-ec-dev",
>> +		.owner = THIS_MODULE,
> 
> Remove this line.
>

Right, I've seen some cleanups efforts to remove the owner from drivers
but forgot when reviewing this driver for posting.
 
>> +	},
>> +	.probe = ec_device_probe,
>> +	.remove = ec_device_remove,
>> +};
> 
> Where is this device registered from?
> 
>> +module_platform_driver(cros_ec_dev_driver);

This preprocessor macro is expanded to (from include/linux/device.h):

module_platform_driver() -> module_driver()

#define module_driver(__driver, __register, __unregister, ...) \
static int __init __driver##_init(void) \
{ \
	return __register(&(__driver) , ##__VA_ARGS__); \
} \

Again, thanks a lot for your detailed review.

Best regards,
Javier

Re: [PATCH 1/3] mfd: cros_ec: Add Chrome OS EC userspace device interface

[Lee Jones]

[...]

> >> +#include <linux/compat.h>
> >> +#include <linux/delay.h>
> >> +#include <linux/device.h>
> >> +#include <linux/fs.h>
> >> +#include <linux/mfd/cros_ec.h>
> >> +#include <linux/mfd/cros_ec_commands.h>
> >> +#include <linux/mfd/cros_ec_dev.h>
> >> +#include <linux/module.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/printk.h>
> > 
> > What do you need this for?
> >
> 
> the printk.h header? to use the pr_* functions but I'll make sure that only
> the needed headers are included.

Right, I think don't think you should be using those on a platform device.

> >> +#include <linux/types.h>
> >> +#include <linux/uaccess.h>
> >> +
> >> +/* Device variables */
> >> +#define CROS_CLASS_NAME "chromeos"
> > 
> > Any reason why you can't use the name directly?
> > 
> 
> I prefer macros if possible since they cost nothing and give you an indirection
> level if you want to change it later. Any reason to not use a define directive?

Exactly as you said, they add a layer of (pointless)
indirection/complexity.  You only use this name once, just change it
where you use it if you wish to (but probably never will) adapt the
name. 

[...]

> >> +static struct platform_driver cros_ec_dev_driver = {
> >> +	.driver = {
> >> +		.name = "cros-ec-dev",
> >> +		.owner = THIS_MODULE,
> > 
> > Remove this line.
> >
> 
> Right, I've seen some cleanups efforts to remove the owner from drivers
> but forgot when reviewing this driver for posting.
>  
> >> +	},
> >> +	.probe = ec_device_probe,
> >> +	.remove = ec_device_remove,
> >> +};
> > 
> > Where is this device registered from?
> > 
> >> +module_platform_driver(cros_ec_dev_driver);
> 
> This preprocessor macro is expanded to (from include/linux/device.h):
> 
> module_platform_driver() -> module_driver()
> 
> #define module_driver(__driver, __register, __unregister, ...) \
> static int __init __driver##_init(void) \
> { \
> 	return __register(&(__driver) , ##__VA_ARGS__); \
> } \

I know how the device driver model works.  I'm asking where the
'device' is registered from, not the 'driver' i.e. platform data, DT,
ACPI?

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

Re: [PATCH 1/3] mfd: cros_ec: Add Chrome OS EC userspace device interface

[Javier Martinez Canillas]

Hello Lee,

On 11/20/2014 12:58 PM, Lee Jones wrote:
>> the printk.h header? to use the pr_* functions but I'll make sure that only
>> the needed headers are included.
> 
> Right, I think don't think you should be using those on a platform device.
>

Yes, I'll use dev_err() instead.
 
>> I prefer macros if possible since they cost nothing and give you an indirection
>> level if you want to change it later. Any reason to not use a define directive?
> 
> Exactly as you said, they add a layer of (pointless)
> indirection/complexity.  You only use this name once, just change it
> where you use it if you wish to (but probably never will) adapt the
> name. 
>

Fair enough, I'll remove it.
 
> 
> I know how the device driver model works.  I'm asking where the
> 'device' is registered from, not the 'driver' i.e. platform data, DT,
> ACPI?
> 

Right, sorry for misunderstanding your question and the silly comment then.

$Subject adds a "cros-ec-dev" mfd cell to the cros ec mfd driver.
So the device is registered from DT when the cros ec device node is
matched (e.g: "google,cros-ec-spi" or "google,cros-ec-i2c") and the
cros ec mfd driver probe function calls mfd_add_devices().

Best regards,
Javier

Re: [PATCH 1/3] mfd: cros_ec: Add Chrome OS EC userspace device interface

[Lee Jones]

> > I know how the device driver model works.  I'm asking where the
> > 'device' is registered from, not the 'driver' i.e. platform data, DT,
> > ACPI?
> > 
> 
> Right, sorry for misunderstanding your question and the silly comment then.
> 
> $Subject adds a "cros-ec-dev" mfd cell to the cros ec mfd driver.
> So the device is registered from DT when the cros ec device node is
> matched (e.g: "google,cros-ec-spi" or "google,cros-ec-i2c") and the
> cros ec mfd driver probe function calls mfd_add_devices().

Ah, so it's registered from another MFD device.  That's okay then.
That answers my question.  FWIW I was going to ask about the missing
match table, but that's not required in this case.

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

Re: [PATCH 1/3] mfd: cros_ec: Add Chrome OS EC userspace device interface

[One Thousand Gnomes]

> +#ifdef CONFIG_COMPAT
> +struct compat_cros_ec_command {
> +	uint32_t version;
> +	uint32_t command;
> +	compat_uptr_t outdata;
> +	uint32_t outsize;
> +	compat_uptr_t indata;
> +	uint32_t insize;
> +	uint32_t result;
> +};
> +
> +struct compat_cros_ec_readmem {
> +	uint32_t offset;
> +	uint32_t bytes;
> +	compat_uptr_t buffer;
> +};
> 

This is a new API - arrange them to be 64bit safe and properly padded,
there is no excuse for needing compat crap except for legacy interfaces
you can't fix.

Alan

Re: [PATCH 1/3] mfd: cros_ec: Add Chrome OS EC userspace device interface

[Javier Martinez Canillas]

Hello Alan,

Thanks a lot for your feedback.

On 11/18/2014 06:00 PM, One Thousand Gnomes wrote:
>> +#ifdef CONFIG_COMPAT
>> +struct compat_cros_ec_command {
>> +	uint32_t version;
>> +	uint32_t command;
>> +	compat_uptr_t outdata;
>> +	uint32_t outsize;
>> +	compat_uptr_t indata;
>> +	uint32_t insize;
>> +	uint32_t result;
>> +};
>> +
>> +struct compat_cros_ec_readmem {
>> +	uint32_t offset;
>> +	uint32_t bytes;
>> +	compat_uptr_t buffer;
>> +};
>> 
> 
> This is a new API - arrange them to be 64bit safe and properly padded,
> there is no excuse for needing compat crap except for legacy interfaces
> you can't fix.
> 

Is true that this is a new API for mainline but there is a lot of ChromeOS
installations that depends on this API which means that just replacing the
kernel with a mainline one there, will break existing user-space programs.

But I understand that since those binaries were using a non-ustream kernel
it is expected that the kernel API could be changed.

I think it would be great to keep existing binaries working but if changing
the API is required, then I can certainly do that when doing a re-spin.

Best regards,
Javier

Re: [PATCH 1/3] mfd: cros_ec: Add Chrome OS EC userspace device interface

[Olof Johansson]

On Wed, Nov 19, 2014 at 10:37 AM, Javier Martinez Canillas
<javier.martinez@collabora.co.uk> wrote:
> Hello Alan,
>
> Thanks a lot for your feedback.
>
> On 11/18/2014 06:00 PM, One Thousand Gnomes wrote:
>>> +#ifdef CONFIG_COMPAT
>>> +struct compat_cros_ec_command {
>>> +    uint32_t version;
>>> +    uint32_t command;
>>> +    compat_uptr_t outdata;
>>> +    uint32_t outsize;
>>> +    compat_uptr_t indata;
>>> +    uint32_t insize;
>>> +    uint32_t result;
>>> +};
>>> +
>>> +struct compat_cros_ec_readmem {
>>> +    uint32_t offset;
>>> +    uint32_t bytes;
>>> +    compat_uptr_t buffer;
>>> +};
>>>
>>
>> This is a new API - arrange them to be 64bit safe and properly padded,
>> there is no excuse for needing compat crap except for legacy interfaces
>> you can't fix.
>>
>
> Is true that this is a new API for mainline but there is a lot of ChromeOS
> installations that depends on this API which means that just replacing the
> kernel with a mainline one there, will break existing user-space programs.

I think we can deal with that, at least if we pick new ioctl numbers
so we can tell from the userspace tool which interface is in use
during transition.

> But I understand that since those binaries were using a non-ustream kernel
> it is expected that the kernel API could be changed.
>
> I think it would be great to keep existing binaries working but if changing
> the API is required, then I can certainly do that when doing a re-spin.

I think there's some value in that, but i'm also somewhat embarrassed
to have missed this aspect when doing internal review, and do agree
with Alan. :) And we have only a few tools that use this interface so
we should be able to cope with it.


-Olof

Re: [PATCH 1/3] mfd: cros_ec: Add Chrome OS EC userspace device interface

[Javier Martinez Canillas]

Hello Olof,

On 11/19/2014 09:45 PM, Olof Johansson wrote:
>> Is true that this is a new API for mainline but there is a lot of ChromeOS
>> installations that depends on this API which means that just replacing the
>> kernel with a mainline one there, will break existing user-space programs.
> 
> I think we can deal with that, at least if we pick new ioctl numbers
> so we can tell from the userspace tool which interface is in use
> during transition.
> 
>> But I understand that since those binaries were using a non-ustream kernel
>> it is expected that the kernel API could be changed.
>>
>> I think it would be great to keep existing binaries working but if changing
>> the API is required, then I can certainly do that when doing a re-spin.
> 
> I think there's some value in that, but i'm also somewhat embarrassed
> to have missed this aspect when doing internal review, and do agree
> with Alan. :) And we have only a few tools that use this interface so
> we should be able to cope with it.
> 
>

Thanks a lot for your feedback. I'll follow Alan suggestion then and make
the structs to be 64-bit safe and properly padded. Also, I'll follow your
suggestion and use a different magic number for the IOCTLs so user-space
programs can be backward compatible if needed.
 
> -Olof
> 

Best regards,
Javier

[PATCH 2/3] mfd: cros_ec: Create sysfs attributes for the ChromeOS EC.

[Javier Martinez Canillas]

From: Bill Richardson <wfrichar@chromium.org>

This adds the first few sysfs attributes for the Chrome OS EC. These
controls are made available under /sys/devices/virtual/chromeos/cros_ec

    flashinfo   - display current flash info
    reboot      - tell the EC to reboot in various ways
    version     - information about the EC software and hardware

Future changes will build on this to add additional controls.

>From a root shell, you should be able to do things like this:

    cd /sys/devices/virtual/chromeos/cros_ec
    cat flashinfo
    cat version
    echo rw > reboot
    cat version
    echo ro > reboot
    cat version
    echo rw > reboot
    cat version
    echo cold > reboot

That last command will reboot the AP too.

Signed-off-by: Bill Richardson <wfrichar@chromium.org>
Reviewed-by: Olof Johansson <olofj@chromium.org>
Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 drivers/mfd/Makefile        |   3 +-
 drivers/mfd/cros_ec_dev.c   |  10 +-
 drivers/mfd/cros_ec_dev.h   |  26 +++++
 drivers/mfd/cros_ec_sysfs.c | 271 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 307 insertions(+), 3 deletions(-)
 create mode 100644 drivers/mfd/cros_ec_dev.h
 create mode 100644 drivers/mfd/cros_ec_sysfs.c

diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 5104a25..eb96986 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -12,7 +12,8 @@ obj-$(CONFIG_MFD_BCM590XX)	+= bcm590xx.o
 obj-$(CONFIG_MFD_CROS_EC)	+= cros_ec.o
 obj-$(CONFIG_MFD_CROS_EC_I2C)	+= cros_ec_i2c.o
 obj-$(CONFIG_MFD_CROS_EC_SPI)	+= cros_ec_spi.o
-obj-$(CONFIG_MFD_CROS_EC_DEV)	+= cros_ec_dev.o
+cros_ec_devs-objs		:= cros_ec_dev.o cros_ec_sysfs.o
+obj-$(CONFIG_MFD_CROS_EC_DEV)	+= cros_ec_devs.o
 
 rtsx_pci-objs			:= rtsx_pcr.o rtsx_gops.o rts5209.o rts5229.o rtl8411.o rts5227.o rts5249.o
 obj-$(CONFIG_MFD_RTSX_PCI)	+= rtsx_pci.o
diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
index c18eb40..1db60dd 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -32,6 +32,8 @@
 #include <linux/types.h>
 #include <linux/uaccess.h>
 
+#include "cros_ec_dev.h"
+
 /* Device variables */
 #define CROS_CLASS_NAME "chromeos"
 static struct cros_ec_device *ec;
@@ -311,15 +313,18 @@ static int ec_device_probe(struct platform_device *pdev)
 		goto failed_chrdevreg;
 	}
 
-	/* Instantiate it */
+	/* Instantiate it (and remember the EC) */
 	ec->vdev = device_create(cros_class, NULL, MKDEV(ec_major, 0),
-				 NULL, CROS_EC_DEV_NAME);
+				 ec, CROS_EC_DEV_NAME);
 	if (IS_ERR(ec->vdev)) {
 		pr_err("failed to create device\n");
 		retval = PTR_ERR(ec->vdev);
 		goto failed_devreg;
 	}
 
+	/* Initialize extra interfaces */
+	ec_dev_sysfs_init(ec);
+
 	return 0;
 
 failed_devreg:
@@ -332,6 +337,7 @@ failed_class:
 
 static int ec_device_remove(struct platform_device *pdev)
 {
+	ec_dev_sysfs_remove(ec);
 	device_destroy(cros_class, MKDEV(ec_major, 0));
 	unregister_chrdev(ec_major, CROS_EC_DEV_NAME);
 	class_destroy(cros_class);
diff --git a/drivers/mfd/cros_ec_dev.h b/drivers/mfd/cros_ec_dev.h
new file mode 100644
index 0000000..0bb833e
--- /dev/null
+++ b/drivers/mfd/cros_ec_dev.h
@@ -0,0 +1,26 @@
+/*
+ * Copyright (C) 2014 Google, Inc.
+ *
+ * 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/>.
+ */
+
+#ifndef _DRV_CROS_EC_DEV_H_
+#define _DRV_CROS_EC_DEV_H_
+
+struct cros_ec_device;
+
+void ec_dev_sysfs_init(struct cros_ec_device *);
+void ec_dev_sysfs_remove(struct cros_ec_device *);
+
+#endif	/* _DRV_CROS_EC_DEV_H_ */
diff --git a/drivers/mfd/cros_ec_sysfs.c b/drivers/mfd/cros_ec_sysfs.c
new file mode 100644
index 0000000..7359c9f
--- /dev/null
+++ b/drivers/mfd/cros_ec_sysfs.c
@@ -0,0 +1,271 @@
+/*
+ * cros_ec_sysfs - expose the Chrome OS EC through sysfs
+ *
+ * Copyright (C) 2014 Google, Inc.
+ *
+ * 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/>.
+ */
+
+#define pr_fmt(fmt) "cros_ec_sysfs: " fmt
+
+#include <linux/ctype.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/fs.h>
+#include <linux/kobject.h>
+#include <linux/mfd/cros_ec.h>
+#include <linux/mfd/cros_ec_commands.h>
+#include <linux/mfd/cros_ec_dev.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/printk.h>
+#include <linux/stat.h>
+#include <linux/types.h>
+#include <linux/uaccess.h>
+
+#include "cros_ec_dev.h"
+
+/* Accessor functions */
+
+static ssize_t show_ec_reboot(struct device *dev,
+			      struct device_attribute *attr, char *buf)
+{
+	int count = 0;
+
+	count += scnprintf(buf + count, PAGE_SIZE - count,
+			   "ro|rw|cancel|cold|disable-jump|hibernate");
+	count += scnprintf(buf + count, PAGE_SIZE - count,
+			   " [at-shutdown]\n");
+	return count;
+}
+
+static ssize_t store_ec_reboot(struct device *dev,
+			       struct device_attribute *attr,
+			       const char *buf, size_t count)
+{
+	static const struct {
+		const char * const str;
+		uint8_t cmd;
+		uint8_t flags;
+	} words[] = {
+		{"cancel",       EC_REBOOT_CANCEL, 0},
+		{"ro",           EC_REBOOT_JUMP_RO, 0},
+		{"rw",           EC_REBOOT_JUMP_RW, 0},
+		{"cold",         EC_REBOOT_COLD, 0},
+		{"disable-jump", EC_REBOOT_DISABLE_JUMP, 0},
+		{"hibernate",    EC_REBOOT_HIBERNATE, 0},
+		{"at-shutdown",  -1, EC_REBOOT_FLAG_ON_AP_SHUTDOWN},
+	};
+	struct ec_params_reboot_ec param;
+	struct cros_ec_command msg = { 0 };
+	int got_cmd = 0, offset = 0;
+	int i;
+	int ret;
+	struct cros_ec_device *ec = dev_get_drvdata(dev);
+
+	param.flags = 0;
+	while (1) {
+		/* Find word to start scanning */
+		while (buf[offset] && isspace(buf[offset]))
+			offset++;
+		if (!buf[offset])
+			break;
+
+		for (i = 0; i < ARRAY_SIZE(words); i++) {
+			if (!strncasecmp(words[i].str, buf+offset,
+					 strlen(words[i].str))) {
+				if (words[i].flags) {
+					param.flags |= words[i].flags;
+				} else {
+					param.cmd = words[i].cmd;
+					got_cmd = 1;
+				}
+				break;
+			}
+		}
+
+		/* On to the next word, if any */
+		while (buf[offset] && !isspace(buf[offset]))
+			offset++;
+	}
+
+	if (!got_cmd)
+		return -EINVAL;
+
+	msg.command = EC_CMD_REBOOT_EC;
+	msg.outdata = (uint8_t *)&param;
+	msg.outsize = sizeof(param);
+	ret = cros_ec_cmd_xfer(ec, &msg);
+	if (ret < 0)
+		return ret;
+	if (msg.result != EC_RES_SUCCESS) {
+		dev_dbg(ec->dev, "EC result %d\n", msg.result);
+		return -EINVAL;
+	}
+
+	return count;
+}
+
+static ssize_t show_ec_version(struct device *dev,
+			       struct device_attribute *attr, char *buf)
+{
+	static const char * const image_names[] = {"unknown", "RO", "RW"};
+	struct ec_response_get_version r_ver;
+	struct ec_response_get_chip_info r_chip;
+	struct ec_response_board_version r_board;
+	struct cros_ec_command msg = { 0 };
+	char r_build_string[EC_HOST_PARAM_SIZE];
+	int ret;
+	int count = 0;
+	struct cros_ec_device *ec = dev_get_drvdata(dev);
+
+	/* Get versions. RW may change. */
+	msg.command = EC_CMD_GET_VERSION;
+	msg.indata = (uint8_t *)&r_ver;
+	msg.insize = sizeof(r_ver);
+	ret = cros_ec_cmd_xfer(ec, &msg);
+	if (ret < 0)
+		return ret;
+	if (msg.result != EC_RES_SUCCESS)
+		return scnprintf(buf, PAGE_SIZE,
+				 "ERROR: EC returned %d\n", msg.result);
+
+	/* Strings should be null-terminated, but let's be sure. */
+	r_ver.version_string_ro[sizeof(r_ver.version_string_ro) - 1] = '\0';
+	r_ver.version_string_rw[sizeof(r_ver.version_string_rw) - 1] = '\0';
+	count += scnprintf(buf + count, PAGE_SIZE - count,
+			   "RO version:    %s\n", r_ver.version_string_ro);
+	count += scnprintf(buf + count, PAGE_SIZE - count,
+			   "RW version:    %s\n", r_ver.version_string_rw);
+	count += scnprintf(buf + count, PAGE_SIZE - count,
+			   "Firmware copy: %s\n",
+			   (r_ver.current_image < ARRAY_SIZE(image_names) ?
+			    image_names[r_ver.current_image] : "?"));
+
+	/* Get build info. */
+	msg.command = EC_CMD_GET_BUILD_INFO;
+	msg.indata = (uint8_t *)r_build_string;
+	msg.insize = sizeof(r_build_string);
+	ret = cros_ec_cmd_xfer(ec, &msg);
+	if (ret < 0)
+		count += scnprintf(buf + count, PAGE_SIZE - count,
+				   "Build info:    XFER ERROR %d\n", ret);
+	else if (msg.result != EC_RES_SUCCESS)
+		count += scnprintf(buf + count, PAGE_SIZE - count,
+				   "Build info:    EC error %d\n", msg.result);
+	else {
+		r_build_string[sizeof(r_build_string) - 1] = '\0';
+		count += scnprintf(buf + count, PAGE_SIZE - count,
+				   "Build info:    %s\n", r_build_string);
+	}
+
+	/* Get chip info. */
+	msg.command = EC_CMD_GET_CHIP_INFO;
+	msg.indata = (uint8_t *)&r_chip;
+	msg.insize = sizeof(r_chip);
+	ret = cros_ec_cmd_xfer(ec, &msg);
+	if (ret < 0)
+		count += scnprintf(buf + count, PAGE_SIZE - count,
+				   "Chip info:     XFER ERROR %d\n", ret);
+	else if (msg.result != EC_RES_SUCCESS)
+		count += scnprintf(buf + count, PAGE_SIZE - count,
+				   "Chip info:     EC error %d\n", msg.result);
+	else {
+		r_chip.vendor[sizeof(r_chip.vendor) - 1] = '\0';
+		r_chip.name[sizeof(r_chip.name) - 1] = '\0';
+		r_chip.revision[sizeof(r_chip.revision) - 1] = '\0';
+		count += scnprintf(buf + count, PAGE_SIZE - count,
+				   "Chip vendor:   %s\n", r_chip.vendor);
+		count += scnprintf(buf + count, PAGE_SIZE - count,
+				   "Chip name:     %s\n", r_chip.name);
+		count += scnprintf(buf + count, PAGE_SIZE - count,
+				   "Chip revision: %s\n", r_chip.revision);
+	}
+
+	/* Get board version */
+	msg.command = EC_CMD_GET_BOARD_VERSION;
+	msg.indata = (uint8_t *)&r_board;
+	msg.insize = sizeof(r_board);
+	ret = cros_ec_cmd_xfer(ec, &msg);
+	if (ret < 0)
+		count += scnprintf(buf + count, PAGE_SIZE - count,
+				   "Board version: XFER ERROR %d\n", ret);
+	else if (msg.result != EC_RES_SUCCESS)
+		count += scnprintf(buf + count, PAGE_SIZE - count,
+				   "Board version: EC error %d\n", msg.result);
+	else {
+		count += scnprintf(buf + count, PAGE_SIZE - count,
+				   "Board version: %d\n",
+				   r_board.board_version);
+	}
+
+	return count;
+}
+
+static ssize_t show_ec_flashinfo(struct device *dev,
+				 struct device_attribute *attr, char *buf)
+{
+	struct ec_response_flash_info resp;
+	struct cros_ec_command msg = { 0 };
+	int ret;
+	struct cros_ec_device *ec = dev_get_drvdata(dev);
+
+	/* The flash info shouldn't ever change, but ask each time anyway. */
+	msg.command = EC_CMD_FLASH_INFO;
+	msg.indata = (uint8_t *)&resp;
+	msg.insize = sizeof(resp);
+	ret = cros_ec_cmd_xfer(ec, &msg);
+	if (ret < 0)
+		return ret;
+	if (msg.result != EC_RES_SUCCESS)
+		return scnprintf(buf, PAGE_SIZE,
+				 "ERROR: EC returned %d\n", msg.result);
+
+	return scnprintf(buf, PAGE_SIZE,
+			 "FlashSize %d\nWriteSize %d\n"
+			 "EraseSize %d\nProtectSize %d\n",
+			 resp.flash_size, resp.write_block_size,
+			 resp.erase_block_size, resp.protect_block_size);
+}
+
+/* Module initialization */
+
+static DEVICE_ATTR(reboot, S_IWUSR | S_IRUGO, show_ec_reboot, store_ec_reboot);
+static DEVICE_ATTR(version, S_IRUGO, show_ec_version, NULL);
+static DEVICE_ATTR(flashinfo, S_IRUGO, show_ec_flashinfo, NULL);
+
+static struct attribute *__ec_attrs[] = {
+	&dev_attr_reboot.attr,
+	&dev_attr_version.attr,
+	&dev_attr_flashinfo.attr,
+	NULL,
+};
+
+static struct attribute_group ec_attr_group = {
+	.attrs = __ec_attrs,
+};
+
+void ec_dev_sysfs_init(struct cros_ec_device *ec)
+{
+	int error;
+
+	error = sysfs_create_group(&ec->vdev->kobj, &ec_attr_group);
+	if (error)
+		pr_warn("failed to create group: %d\n", error);
+}
+
+void ec_dev_sysfs_remove(struct cros_ec_device *ec)
+{
+	sysfs_remove_group(&ec->vdev->kobj, &ec_attr_group);
+}
-- 
2.1.0

Re: [PATCH 2/3] mfd: cros_ec: Create sysfs attributes for the ChromeOS EC.

[Lee Jones]

On Mon, 17 Nov 2014, Javier Martinez Canillas wrote:

> From: Bill Richardson <wfrichar@chromium.org>
> 
> This adds the first few sysfs attributes for the Chrome OS EC. These
> controls are made available under /sys/devices/virtual/chromeos/cros_ec
> 
>     flashinfo   - display current flash info

drivers/mtd?

>     reboot      - tell the EC to reboot in various ways

drivers/power?

>     version     - information about the EC software and hardware

What's the difference between this version and the version you can
read in the new _dev driver?

> Future changes will build on this to add additional controls.
> 
> From a root shell, you should be able to do things like this:
> 
>     cd /sys/devices/virtual/chromeos/cros_ec
>     cat flashinfo
>     cat version
>     echo rw > reboot
>     cat version
>     echo ro > reboot
>     cat version
>     echo rw > reboot
>     cat version
>     echo cold > reboot
> 
> That last command will reboot the AP too.
> 
> Signed-off-by: Bill Richardson <wfrichar@chromium.org>
> Reviewed-by: Olof Johansson <olofj@chromium.org>
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> ---
>  drivers/mfd/Makefile        |   3 +-
>  drivers/mfd/cros_ec_dev.c   |  10 +-
>  drivers/mfd/cros_ec_dev.h   |  26 +++++
>  drivers/mfd/cros_ec_sysfs.c | 271 ++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 307 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/mfd/cros_ec_dev.h
>  create mode 100644 drivers/mfd/cros_ec_sysfs.c

[...]

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

Re: [PATCH 2/3] mfd: cros_ec: Create sysfs attributes for the ChromeOS EC.

[Javier Martinez Canillas]

Hello Lee,

On 11/18/2014 03:26 PM, Lee Jones wrote:
> On Mon, 17 Nov 2014, Javier Martinez Canillas wrote:
> 
>> From: Bill Richardson <wfrichar@chromium.org>
>> 
>> This adds the first few sysfs attributes for the Chrome OS EC. These
>> controls are made available under /sys/devices/virtual/chromeos/cros_ec
>> 
>>     flashinfo   - display current flash info
> 
> drivers/mtd?
> 
>>     reboot      - tell the EC to reboot in various ways
> 
> drivers/power?
>

Well this driver is special in the sense that there is a Cortex-M Embedded
Controller that has different peripherals (flash, keyboard, charger, etc).
The kernel communicates with these peripherals by using a tunnel through
the protocol used to communicate with the Embedded Controller (SPI/I2C/LPC).

But you are right that this is not suitable for drivers/mfd, I just added
there because that is the location in the downstream ChromeOS kernel but
like you said, the MFD subsystem should not be a dumping ground for devices
that is not clear where should live.

On v2 I'll add this sysfs interface and the dev to drivers/platform/chrome/
since I think is a better place for the cros_ec dev driver. It will still
be spawns from the cros_ec MFD driver though but that is OK for you AFAIU.
 
>>     version     - information about the EC software and hardware
> 
> What's the difference between this version and the version you can
> read in the new _dev driver?
>

The version information read from the cros_ec dev interface is a subset
of the information read from the version sysfs, e.g:

$ cat /dev/cros_ec
1.0.0
pit_v1.1.1198-1cd618e
pit_v1.1.1198-1cd618e
read-only

$ cat /sys/class/chromeos/cros_ec/version
RO version:    pit_v1.1.1198-1cd618e
RW version:    pit_v1.1.1198-1cd618e
Firmware copy: RO
Build info:    pit_v1.1.1198-1cd618e 2014-04-30 16:29:52 @build122-m2
Chip vendor:   stm
Chip name:     stm32l15x
Chip revision:
Board version: 0

I don't really know why there are two interfaces for this but I guess
that different user-space utilities use one or another. Maybe the
ChromiumOS folks can comment on this.

Best regards,
Javier

Re: [PATCH 2/3] mfd: cros_ec: Create sysfs attributes for the ChromeOS EC.

[Bill Richardson]

On Thu, Nov 20, 2014 at 3:58 AM, Javier Martinez Canillas
<javier.martinez@collabora.co.uk> wrote:
>
> I don't really know why there are two interfaces for this but I guess
> that different user-space utilities use one or another. Maybe the
> ChromiumOS folks can comment on this.

The /dev/cros_ec interface responds to ioctls and is the primary means
by which userspace applications talk to the EC (replacing three really
awkward bus-specific libraries). It emits a simplified version string
when read so that humans can tell if the EC is alive and speaking the
same protocol that the userspace app expects.

The /sys/class/chromeos/cros_ec/* tree was originally envisioned as a
way to provide additional human-readable interfaces to a subset of the
EC commands, but except for the lightbar it's not often used. The
version component here shows all the version-related info that the EC
can provide.

I apologize for the poor kernel conventions used here. In my
ignorance, I apparently chose the worst code possible to use as a
template, and then invented some new stupidness of my own.


Bill
-- 
Art for Art's Sake
Engineering for Money

Re: [PATCH 2/3] mfd: cros_ec: Create sysfs attributes for the ChromeOS EC.

[Javier Martinez Canillas]

Hello Bill,

On 11/20/2014 07:16 PM, Bill Richardson wrote:
>>
>> I don't really know why there are two interfaces for this but I guess
>> that different user-space utilities use one or another. Maybe the
>> ChromiumOS folks can comment on this.
> 
> The /dev/cros_ec interface responds to ioctls and is the primary means
> by which userspace applications talk to the EC (replacing three really
> awkward bus-specific libraries). It emits a simplified version string
> when read so that humans can tell if the EC is alive and speaking the
> same protocol that the userspace app expects.
> 
> The /sys/class/chromeos/cros_ec/* tree was originally envisioned as a
> way to provide additional human-readable interfaces to a subset of the
> EC commands, but except for the lightbar it's not often used. The
> version component here shows all the version-related info that the EC
> can provide.
> 

Got it, thanks a lot for the explanation.

Best regards,
Javier

[PATCH 3/3] mfd: cros_ec: Expose Chrome OS Lightbar to users

[Javier Martinez Canillas]

From: Bill Richardson <wfrichar@chromium.org>

This adds some sysfs entries to provide userspace control of the
four-element LED "lightbar" on the Chromebook Pixel. This only instantiates
the lightbar controls if the device actually exists.

To prevent DoS attacks, this interface is limited to 20 accesses/second,
although that rate can be adjusted by a privileged user.

On Chromebooks without a lightbar, this should have no effect. On the
Chromebook Pixel, you should be able to do things like this:

    $ cd /sys/devices/virtual/chromeos/cros_ec/lightbar
    $ echo 0x80 > brightness
    $ echo 255 > brightness
    $
    $ cat sequence
    S0
    $ echo konami > sequence
    $ cat sequence
    KONAMI
    $
    $ cat sequence
    S0

And

    $ cd /sys/devices/virtual/chromeos/cros_ec/lightbar
    $ echo stop > sequence
    $ echo "4 255 255 255" > led_rgb
    $ echo "0 255 0 0  1 0 255 0  2 0 0 255  3 255 255 0" > led_rgb
    $ echo run  > sequence

Test the DoS prevention with this:

    $ cd /sys/devices/virtual/chromeos/cros_ec/lightbar
    $ echo 500 > interval_msec
    $ time (cat version version version version version version version)

Signed-off-by: Bill Richardson <wfrichar@chromium.org>
Reviewed-by: Olof Johansson <olofj@chromium.org>
Tested-by: Doug Anderson <dianders@chromium.org>
Reviewed-by: Benson Leung <bleung@chromium.org>
Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 drivers/mfd/Makefile           |   2 +-
 drivers/mfd/cros_ec_dev.c      |   2 +
 drivers/mfd/cros_ec_dev.h      |   3 +
 drivers/mfd/cros_ec_lightbar.c | 347 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 353 insertions(+), 1 deletion(-)
 create mode 100644 drivers/mfd/cros_ec_lightbar.c

diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index eb96986..54ef1bf 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -12,7 +12,7 @@ obj-$(CONFIG_MFD_BCM590XX)	+= bcm590xx.o
 obj-$(CONFIG_MFD_CROS_EC)	+= cros_ec.o
 obj-$(CONFIG_MFD_CROS_EC_I2C)	+= cros_ec_i2c.o
 obj-$(CONFIG_MFD_CROS_EC_SPI)	+= cros_ec_spi.o
-cros_ec_devs-objs		:= cros_ec_dev.o cros_ec_sysfs.o
+cros_ec_devs-objs		:= cros_ec_dev.o cros_ec_sysfs.o cros_ec_lightbar.o
 obj-$(CONFIG_MFD_CROS_EC_DEV)	+= cros_ec_devs.o
 
 rtsx_pci-objs			:= rtsx_pcr.o rtsx_gops.o rts5209.o rts5229.o rtl8411.o rts5227.o rts5249.o
diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
index 1db60dd..dbffac9 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -324,6 +324,7 @@ static int ec_device_probe(struct platform_device *pdev)
 
 	/* Initialize extra interfaces */
 	ec_dev_sysfs_init(ec);
+	ec_dev_lightbar_init(ec);
 
 	return 0;
 
@@ -337,6 +338,7 @@ failed_class:
 
 static int ec_device_remove(struct platform_device *pdev)
 {
+	ec_dev_lightbar_remove(ec);
 	ec_dev_sysfs_remove(ec);
 	device_destroy(cros_class, MKDEV(ec_major, 0));
 	unregister_chrdev(ec_major, CROS_EC_DEV_NAME);
diff --git a/drivers/mfd/cros_ec_dev.h b/drivers/mfd/cros_ec_dev.h
index 0bb833e..cb17f4c 100644
--- a/drivers/mfd/cros_ec_dev.h
+++ b/drivers/mfd/cros_ec_dev.h
@@ -23,4 +23,7 @@ struct cros_ec_device;
 void ec_dev_sysfs_init(struct cros_ec_device *);
 void ec_dev_sysfs_remove(struct cros_ec_device *);
 
+void ec_dev_lightbar_init(struct cros_ec_device *);
+void ec_dev_lightbar_remove(struct cros_ec_device *);
+
 #endif	/* _DRV_CROS_EC_DEV_H_ */
diff --git a/drivers/mfd/cros_ec_lightbar.c b/drivers/mfd/cros_ec_lightbar.c
new file mode 100644
index 0000000..fb1282d
--- /dev/null
+++ b/drivers/mfd/cros_ec_lightbar.c
@@ -0,0 +1,347 @@
+/*
+ * cros_ec_lightbar - expose the Chromebook Pixel lightbar to userspace
+ *
+ * Copyright (C) 2014 Google, Inc.
+ *
+ * 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/>.
+ */
+
+#define pr_fmt(fmt) "cros_ec_lightbar: " fmt
+
+#include <linux/ctype.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/fs.h>
+#include <linux/kobject.h>
+#include <linux/mfd/cros_ec.h>
+#include <linux/mfd/cros_ec_commands.h>
+#include <linux/mfd/cros_ec_dev.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/sched.h>
+#include <linux/types.h>
+#include <linux/uaccess.h>
+
+#include "cros_ec_dev.h"
+
+/* Rate-limit the lightbar interface to prevent DoS. */
+static unsigned long lb_interval_jiffies = 50 * HZ / 1000;
+
+static ssize_t interval_msec_show(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	unsigned long msec = lb_interval_jiffies * 1000 / HZ;
+
+	return scnprintf(buf, PAGE_SIZE, "%lu\n", msec);
+}
+
+static ssize_t interval_msec_store(struct device *dev,
+				   struct device_attribute *attr,
+				   const char *buf, size_t count)
+{
+	unsigned long msec;
+
+	if (kstrtoul(buf, 0, &msec))
+		return -EINVAL;
+
+	lb_interval_jiffies = msec * HZ / 1000;
+
+	return count;
+}
+
+static DEFINE_MUTEX(lb_mutex);
+/* Return 0 if able to throttle correctly, error otherwise */
+static int lb_throttle(void)
+{
+	static unsigned long last_access;
+	unsigned long now, next_timeslot;
+	long delay;
+	int ret = 0;
+
+	mutex_lock(&lb_mutex);
+
+	now = jiffies;
+	next_timeslot = last_access + lb_interval_jiffies;
+
+	if (time_before(now, next_timeslot)) {
+		delay = (long)(next_timeslot) - (long)now;
+		set_current_state(TASK_INTERRUPTIBLE);
+		if (schedule_timeout(delay) > 0) {
+			/* interrupted - just abort */
+			ret = -EINTR;
+			goto out;
+		}
+		now = jiffies;
+	}
+
+	last_access = now;
+out:
+	mutex_unlock(&lb_mutex);
+
+	return ret;
+}
+
+#define INIT_MSG(P, R) { \
+		.command = EC_CMD_LIGHTBAR_CMD, \
+		.outdata = (uint8_t *)&P, \
+		.outsize = sizeof(P), \
+		.indata = (uint8_t *)&R, \
+		.insize = sizeof(R), \
+	}
+
+static int get_lightbar_version(struct cros_ec_device *ec,
+				uint32_t *ver_ptr, uint32_t *flg_ptr)
+{
+	struct ec_params_lightbar param;
+	struct ec_response_lightbar resp;
+	struct cros_ec_command msg = INIT_MSG(param, resp);
+	int ret;
+
+	param.cmd = LIGHTBAR_CMD_VERSION;
+	ret = cros_ec_cmd_xfer(ec, &msg);
+	if (ret < 0)
+		return 0;
+
+	switch (msg.result) {
+	case EC_RES_INVALID_PARAM:
+		/* Pixel had no version command. */
+		if (ver_ptr)
+			*ver_ptr = 0;
+		if (flg_ptr)
+			*flg_ptr = 0;
+		return 1;
+
+	case EC_RES_SUCCESS:
+		/* Future devices w/lightbars should implement this command */
+		if (ver_ptr)
+			*ver_ptr = resp.version.num;
+		if (flg_ptr)
+			*flg_ptr = resp.version.flags;
+		return 1;
+	}
+
+	/* Anything else (ie, EC_RES_INVALID_COMMAND) - no lightbar */
+	return 0;
+}
+
+static ssize_t version_show(struct device *dev,
+			    struct device_attribute *attr, char *buf)
+{
+	uint32_t version, flags;
+	struct cros_ec_device *ec = dev_get_drvdata(dev);
+	int ret;
+
+	ret = lb_throttle();
+	if (ret)
+		return ret;
+
+	/* This should always succeed, because we check during init. */
+	if (!get_lightbar_version(ec, &version, &flags))
+		return -EIO;
+
+	return scnprintf(buf, PAGE_SIZE, "%d %d\n", version, flags);
+}
+
+static ssize_t brightness_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	struct ec_params_lightbar param;
+	struct ec_response_lightbar resp;
+	struct cros_ec_command msg = INIT_MSG(param, resp);
+	int ret;
+	unsigned int val;
+	struct cros_ec_device *ec = dev_get_drvdata(dev);
+
+	if (kstrtouint(buf, 0, &val))
+		return -EINVAL;
+	param.cmd = LIGHTBAR_CMD_BRIGHTNESS;
+	param.brightness.num = val;
+	ret = lb_throttle();
+	if (ret)
+		return ret;
+	ret = cros_ec_cmd_xfer(ec, &msg);
+	if (ret < 0)
+		return ret;
+	if (msg.result != EC_RES_SUCCESS)
+		return -EINVAL;
+
+	return count;
+}
+
+
+/*
+ * We expect numbers, and we'll keep reading until we find them, skipping over
+ * any whitespace (sysfs guarantees that the input is null-terminated). Every
+ * four numbers are sent to the lightbar as <LED,R,G,B>. We fail at the first
+ * parsing error, if we don't parse any numbers, or if we have numbers left
+ * over.
+ */
+static ssize_t led_rgb_store(struct device *dev, struct device_attribute *attr,
+			     const char *buf, size_t count)
+{
+	struct ec_params_lightbar param;
+	struct ec_response_lightbar resp;
+	struct cros_ec_command msg = INIT_MSG(param, resp);
+	struct cros_ec_device *ec = dev_get_drvdata(dev);
+	unsigned int val[4];
+	int ret, i = 0, j = 0, ok = 0;
+
+	do {
+		/* Skip any whitespace */
+		while (*buf && isspace(*buf))
+			buf++;
+		if (!*buf)
+			break;
+
+		if (kstrtouint(buf, 0, &val[i++]))
+			return -EINVAL;
+
+		if (i == 4) {
+			param.cmd = LIGHTBAR_CMD_RGB;
+			param.rgb.led = val[0];
+			param.rgb.red = val[1];
+			param.rgb.green = val[2];
+			param.rgb.blue = val[3];
+			/*
+			 * Throttle only the first of every four transactions,
+			 * so that the user can update all four LEDs at once.
+			 */
+			if ((j++ % 4) == 0) {
+				ret = lb_throttle();
+				if (ret)
+					return ret;
+			}
+			ret = cros_ec_cmd_xfer(ec, &msg);
+			if (ret < 0)
+				return ret;
+			if (msg.result != EC_RES_SUCCESS)
+				return -EINVAL;
+			i = 0;
+			ok = 1;
+		}
+
+		/* Skip over the number we just read */
+		while (*buf && !isspace(*buf))
+			buf++;
+
+	} while (*buf);
+
+	return (ok && i == 0) ? count : -EINVAL;
+}
+
+static const char const *seqname[] = {
+	"ERROR", "S5", "S3", "S0", "S5S3", "S3S0",
+	"S0S3", "S3S5", "STOP", "RUN", "PULSE", "TEST", "KONAMI",
+};
+
+static ssize_t sequence_show(struct device *dev,
+			     struct device_attribute *attr, char *buf)
+{
+	struct ec_params_lightbar param;
+	struct ec_response_lightbar resp;
+	struct cros_ec_command msg = INIT_MSG(param, resp);
+	int ret;
+	struct cros_ec_device *ec = dev_get_drvdata(dev);
+
+	param.cmd = LIGHTBAR_CMD_GET_SEQ;
+	ret = lb_throttle();
+	if (ret)
+		return ret;
+	ret = cros_ec_cmd_xfer(ec, &msg);
+	if (ret < 0)
+		return ret;
+	if (msg.result != EC_RES_SUCCESS)
+		return scnprintf(buf, PAGE_SIZE,
+				 "ERROR: EC returned %d\n", msg.result);
+
+	if (resp.get_seq.num >= ARRAY_SIZE(seqname))
+		return scnprintf(buf, PAGE_SIZE, "%d\n", resp.get_seq.num);
+	else
+		return scnprintf(buf, PAGE_SIZE, "%s\n",
+				 seqname[resp.get_seq.num]);
+}
+
+static ssize_t sequence_store(struct device *dev, struct device_attribute *attr,
+			      const char *buf, size_t count)
+{
+	struct ec_params_lightbar param;
+	struct ec_response_lightbar resp;
+	struct cros_ec_command msg = INIT_MSG(param, resp);
+	unsigned int num;
+	int ret, len;
+	struct cros_ec_device *ec = dev_get_drvdata(dev);
+
+	for (len = 0; len < count; len++)
+		if (!isalnum(buf[len]))
+			break;
+
+	for (num = 0; num < ARRAY_SIZE(seqname); num++)
+		if (!strncasecmp(seqname[num], buf, len))
+			break;
+
+	if (num >= ARRAY_SIZE(seqname))
+		kstrtouint(buf, 0, &num);
+
+	param.cmd = LIGHTBAR_CMD_SEQ;
+	param.seq.num = num;
+	ret = lb_throttle();
+	if (ret)
+		return ret;
+	ret = cros_ec_cmd_xfer(ec, &msg);
+	if (ret < 0)
+		return ret;
+	if (msg.result != EC_RES_SUCCESS)
+		return -EINVAL;
+
+	return count;
+}
+
+/* Module initialization */
+
+static DEVICE_ATTR_RW(interval_msec);
+static DEVICE_ATTR_RO(version);
+static DEVICE_ATTR_WO(brightness);
+static DEVICE_ATTR_WO(led_rgb);
+static DEVICE_ATTR_RW(sequence);
+static struct attribute *__lb_cmds_attrs[] = {
+	&dev_attr_interval_msec.attr,
+	&dev_attr_version.attr,
+	&dev_attr_brightness.attr,
+	&dev_attr_led_rgb.attr,
+	&dev_attr_sequence.attr,
+	NULL,
+};
+static struct attribute_group lb_cmds_attr_group = {
+	.name = "lightbar",
+	.attrs = __lb_cmds_attrs,
+};
+
+void ec_dev_lightbar_init(struct cros_ec_device *ec)
+{
+	int ret = 0;
+
+	/* Only instantiate this stuff if the EC has a lightbar */
+	if (!get_lightbar_version(ec, NULL, NULL))
+		return;
+
+	ret = sysfs_create_group(&ec->vdev->kobj, &lb_cmds_attr_group);
+	if (ret)
+		pr_warn("sysfs_create_group() failed: %d\n", ret);
+}
+
+void ec_dev_lightbar_remove(struct cros_ec_device *ec)
+{
+	sysfs_remove_group(&ec->vdev->kobj, &lb_cmds_attr_group);
+}
-- 
2.1.0

Re: [PATCH 3/3] mfd: cros_ec: Expose Chrome OS Lightbar to users

[Lee Jones]

On Mon, 17 Nov 2014, Javier Martinez Canillas wrote:

> From: Bill Richardson <wfrichar@chromium.org>
> 
> This adds some sysfs entries to provide userspace control of the
> four-element LED "lightbar" on the Chromebook Pixel. This only instantiates
> the lightbar controls if the device actually exists.
> 
> To prevent DoS attacks, this interface is limited to 20 accesses/second,
> although that rate can be adjusted by a privileged user.
> 
> On Chromebooks without a lightbar, this should have no effect. On the
> Chromebook Pixel, you should be able to do things like this:
> 
>     $ cd /sys/devices/virtual/chromeos/cros_ec/lightbar
>     $ echo 0x80 > brightness
>     $ echo 255 > brightness
>     $
>     $ cat sequence
>     S0
>     $ echo konami > sequence
>     $ cat sequence
>     KONAMI
>     $
>     $ cat sequence
>     S0
> 
> And
> 
>     $ cd /sys/devices/virtual/chromeos/cros_ec/lightbar
>     $ echo stop > sequence
>     $ echo "4 255 255 255" > led_rgb
>     $ echo "0 255 0 0  1 0 255 0  2 0 0 255  3 255 255 0" > led_rgb
>     $ echo run  > sequence
> 
> Test the DoS prevention with this:
> 
>     $ cd /sys/devices/virtual/chromeos/cros_ec/lightbar
>     $ echo 500 > interval_msec
>     $ time (cat version version version version version version version)
> 
> Signed-off-by: Bill Richardson <wfrichar@chromium.org>
> Reviewed-by: Olof Johansson <olofj@chromium.org>
> Tested-by: Doug Anderson <dianders@chromium.org>
> Reviewed-by: Benson Leung <bleung@chromium.org>
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> ---
>  drivers/mfd/Makefile           |   2 +-
>  drivers/mfd/cros_ec_dev.c      |   2 +
>  drivers/mfd/cros_ec_dev.h      |   3 +
>  drivers/mfd/cros_ec_lightbar.c | 347 +++++++++++++++++++++++++++++++++++++++++

By the sounds of the description, it doesn't seem as though this
driver lives in MFD.  I suggest another home, such as drivers/led.

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

Re: [PATCH 3/3] mfd: cros_ec: Expose Chrome OS Lightbar to users

[Javier Martinez Canillas]

Hello Lee,

On 11/18/2014 03:22 PM, Lee Jones wrote:
>> ---
>>  drivers/mfd/Makefile           |   2 +-
>>  drivers/mfd/cros_ec_dev.c      |   2 +
>>  drivers/mfd/cros_ec_dev.h      |   3 +
>>  drivers/mfd/cros_ec_lightbar.c | 347 +++++++++++++++++++++++++++++++++++++++++
> 
> By the sounds of the description, it doesn't seem as though this
> driver lives in MFD.  I suggest another home, such as drivers/led.
> 

You are right. As I said in the other email, I'll move it to
drivers/platform/chrome/ which seems like a more suitable place.

Thanks a lot for your feedback.

Best regards,
Javier

Re: [PATCH 3/3] mfd: cros_ec: Expose Chrome OS Lightbar to users

[Lee Jones]

On Thu, 20 Nov 2014, Javier Martinez Canillas wrote:

> Hello Lee,
> 
> On 11/18/2014 03:22 PM, Lee Jones wrote:
> >> ---
> >>  drivers/mfd/Makefile           |   2 +-
> >>  drivers/mfd/cros_ec_dev.c      |   2 +
> >>  drivers/mfd/cros_ec_dev.h      |   3 +
> >>  drivers/mfd/cros_ec_lightbar.c | 347 +++++++++++++++++++++++++++++++++++++++++
> > 
> > By the sounds of the description, it doesn't seem as though this
> > driver lives in MFD.  I suggest another home, such as drivers/led.
> > 
> 
> You are right. As I said in the other email, I'll move it to
> drivers/platform/chrome/ which seems like a more suitable place.

Does it?  I would have thought 'lightbar' lives in LEDs, but I guess
that's between you and the drivers/platform/chrome maintainer now.

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

Re: [PATCH 3/3] mfd: cros_ec: Expose Chrome OS Lightbar to users

[Javier Martinez Canillas]

Hello Lee,

On 11/20/2014 02:27 PM, Lee Jones wrote:
>> > By the sounds of the description, it doesn't seem as though this
>> > driver lives in MFD.  I suggest another home, such as drivers/led.
>> > 
>> 
>> You are right. As I said in the other email, I'll move it to
>> drivers/platform/chrome/ which seems like a more suitable place.
> 
> Does it?  I would have thought 'lightbar' lives in LEDs, but I guess
> that's between you and the drivers/platform/chrome maintainer now.
> 

Ok, I'll move cros_ec_lightbar to drivers/led then and cros_ec_dev
to drivers/platform/chrome.

Thanks a lot for all your suggestions!

Best regards,
Javier