* [PATCH RESEND v2 0/7] platform/chrome: Add user-space dev inferface support
@ 2015-01-02 13:32 Javier Martinez Canillas
2015-01-02 13:32 ` [PATCH RESEND v2 1/7] mfd: cros_ec: Use fixed size arrays to transfer data with the EC Javier Martinez Canillas
` (7 more replies)
0 siblings, 8 replies; 35+ messages in thread
From: Javier Martinez Canillas @ 2015-01-02 13:32 UTC (permalink / raw)
To: Lee Jones
Cc: Olof Johansson, Doug Anderson, Bill Richardson, Simon Glass,
Gwendal Grignou, Jonathan Corbet, linux-samsung-soc,
linux-kernel, 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:
- Low Pin Count (LPC) interface
- User-space device 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 is a second version of a series that adds support for the first two of
the missing features: the EC LPC and EC character device interfaces that
are used by user-space to access the ChromeOS EC. The support patches were
taken from the downstream ChromiumOS 3.14 tree with the fixes and cleanups
squashed to have a minimal patch-set.
The version of the ChromeOS EC chardev driver in this series still does not
reflect the latest one that is in the downstream ChromiumOS 3.14 kernel but
makes the delta shorter. Following patches will add the remaining missing
features until both trees are in sync. I preferred to first add the initial
support and then adding the other features to both maintain the original
patch history in the downstream kernel and so preserve the patch authorship
and also make the diff to have a working cros user-space interface smaller.
Version 1 of this series was [0] and added the Chrome EC chardev driver
and the sysfs interface to drivers/mfd since that is what is done in the
downstream ChromiumOS kernel but Lee Jones asked to find a better place
since those are not really multi-function device drivers. So this version
places them under drivers/platform/chrome since MAINTAINERS says that this
sub-directory is "CHROME HARDWARE PLATFORM SUPPORT" which seems a good fit.
A big change in this version is that the ioctl API is modified to make it
64-bit safe and compatible with both 64 and 32 bit user-space binaries.
The data structures passed as arguments to ioctl commands had pointers fields
and these have different byte boundaries alignment requirement so the previous
version had a compat ioctl interface. The feedback was that this had to be
avoided since this was a new ioctl API so the pointers fields were replaced
with a set of fixed-size arrays to be used instead. This has the drawback that
more data could be used and copied between user and kernel space so feedback
is welcomed if there is a better approach to solve this kind of issues.
The patches were tested on an Exynos5420 Peach Pit Chromebook and (thanks to
Bill Richardson) on an x86 Pixel Chromebook using an ectool [1] modified to
use the new ioctl API. The LPC interface driver and the lightbar sysfs driver
were also tested on the Pixel Chromebook.
The series is composed of the following patches:
Bill Richardson (4):
mfd: cros_ec: Add cros_ec_lpc driver for x86 devices
platform/chrome: Add Chrome OS EC userspace device interface
platform/chrome: Create sysfs attributes for the ChromeOS EC.
platform/chrome: Expose Chrome OS Lightbar to users
Javier Martinez Canillas (3):
mfd: cros_ec: Use fixed size arrays to transfer data with the EC
mfd: cros_ec: Add char dev and virtual dev pointers
mfd: cros_ec: Instantiate ChromeOS EC character device
Documentation/ioctl/ioctl-number.txt | 1 +
drivers/i2c/busses/i2c-cros-ec-tunnel.c | 51 +---
drivers/input/keyboard/cros_ec_keyb.c | 13 +-
drivers/mfd/Kconfig | 10 +
drivers/mfd/Makefile | 1 +
drivers/mfd/cros_ec.c | 19 +-
drivers/mfd/cros_ec_lpc.c | 305 ++++++++++++++++++++++++
drivers/platform/chrome/Kconfig | 14 +-
drivers/platform/chrome/Makefile | 2 +
drivers/platform/chrome/cros_ec_dev.c | 274 +++++++++++++++++++++
drivers/platform/chrome/cros_ec_dev.h | 53 +++++
drivers/platform/chrome/cros_ec_lightbar.c | 367 +++++++++++++++++++++++++++++
drivers/platform/chrome/cros_ec_sysfs.c | 271 +++++++++++++++++++++
include/linux/mfd/cros_ec.h | 23 +-
14 files changed, 1342 insertions(+), 62 deletions(-)
create mode 100644 drivers/mfd/cros_ec_lpc.c
create mode 100644 drivers/platform/chrome/cros_ec_dev.c
create mode 100644 drivers/platform/chrome/cros_ec_dev.h
create mode 100644 drivers/platform/chrome/cros_ec_lightbar.c
create mode 100644 drivers/platform/chrome/cros_ec_sysfs.c
Patch #1 modified the struct cros_ec_command structure so it can be used
as an ioctl argument and be 64 and 32 bit safe and patch #2 adds fields
to the struct cros_ec_device that will be needed by the EC chardev driver.
Patch #3 adds support for the EC LPC interface used on x86 Chromebooks.
Patch #4 adds the ChromeOS chardev driver and patch #5 instantiates it
from the mfd cros_ec driver.
Patch #6 exposes sysfs attributes that can be used by user space programs
to get information and control the ChromeOS EC.
Patch #7 exposes sysfs attributes that are used to control the lightbar
RGB LEDs found on the Pixel Chromebook.
The patches must be applied together and in that order due dependencies,
probably through the mfd tree.
Best regards,
Javier
[0]: https://lkml.org/lkml/2014/11/17/429
[1]: git://git.collabora.co.uk/git/user/javier/ec.git mainline-ioctl
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH RESEND v2 1/7] mfd: cros_ec: Use fixed size arrays to transfer data with the EC
2015-01-02 13:32 [PATCH RESEND v2 0/7] platform/chrome: Add user-space dev inferface support Javier Martinez Canillas
@ 2015-01-02 13:32 ` Javier Martinez Canillas
2015-01-20 7:48 ` Lee Jones
2015-01-02 13:32 ` [PATCH RESEND v2 2/7] mfd: cros_ec: Add char dev and virtual dev pointers Javier Martinez Canillas
` (6 subsequent siblings)
7 siblings, 1 reply; 35+ messages in thread
From: Javier Martinez Canillas @ 2015-01-02 13:32 UTC (permalink / raw)
To: Lee Jones
Cc: Olof Johansson, Doug Anderson, Bill Richardson, Simon Glass,
Gwendal Grignou, Jonathan Corbet, linux-samsung-soc,
linux-kernel, Javier Martinez Canillas
The struct cros_ec_command will be used as an ioctl() argument for the
API to control the ChromeOS EC from user-space. So the data structure
has to be 64-bit safe to make it compatible between 32 and 64 avoiding
the need for a compat ioctl interface. Since pointers are self-aligned
to different byte boundaries, use fixed size arrays instead of pointers
for transferring ingoing and outgoing data with the Embedded Controller.
Also, re-arrange struct members by decreasing alignment requirements to
reduce the needing padding size.
Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
Hello,
I choose EC_PROTO2_MAX_PARAM_SIZE as the maximum length for the input and
output buffers since I see that is what is assumed in the cros_ec driver
that is the maximum lengths. But the downstream kernel has also suppport
for the EC host command protocol v3 even though there is currently no bus
specific code to handle v3 packets. So I wonder if this is a good max len
or if a different size should be used instead.
Best regards,
Javier
Changes since v1: None, new patch
drivers/i2c/busses/i2c-cros-ec-tunnel.c | 51 +++++++--------------------------
drivers/input/keyboard/cros_ec_keyb.c | 13 +++++----
drivers/mfd/cros_ec.c | 15 +++++-----
include/linux/mfd/cros_ec.h | 8 +++---
4 files changed, 29 insertions(+), 58 deletions(-)
diff --git a/drivers/i2c/busses/i2c-cros-ec-tunnel.c b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
index 875c22a..fa8dedd 100644
--- a/drivers/i2c/busses/i2c-cros-ec-tunnel.c
+++ b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
@@ -182,72 +182,41 @@ static int ec_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg i2c_msgs[],
const u16 bus_num = bus->remote_bus;
int request_len;
int response_len;
- u8 *request = NULL;
- u8 *response = NULL;
int result;
- struct cros_ec_command msg;
+ struct cros_ec_command msg = { };
request_len = ec_i2c_count_message(i2c_msgs, num);
if (request_len < 0) {
dev_warn(dev, "Error constructing message %d\n", request_len);
- result = request_len;
- goto exit;
+ return request_len;
}
+
response_len = ec_i2c_count_response(i2c_msgs, num);
if (response_len < 0) {
/* Unexpected; no errors should come when NULL response */
dev_warn(dev, "Error preparing response %d\n", response_len);
- result = response_len;
- goto exit;
- }
-
- if (request_len <= ARRAY_SIZE(bus->request_buf)) {
- request = bus->request_buf;
- } else {
- request = kzalloc(request_len, GFP_KERNEL);
- if (request == NULL) {
- result = -ENOMEM;
- goto exit;
- }
- }
- if (response_len <= ARRAY_SIZE(bus->response_buf)) {
- response = bus->response_buf;
- } else {
- response = kzalloc(response_len, GFP_KERNEL);
- if (response == NULL) {
- result = -ENOMEM;
- goto exit;
- }
+ return response_len;
}
- result = ec_i2c_construct_message(request, i2c_msgs, num, bus_num);
+ result = ec_i2c_construct_message(msg.outdata, i2c_msgs, num, bus_num);
if (result)
- goto exit;
+ return result;
msg.version = 0;
msg.command = EC_CMD_I2C_PASSTHRU;
- msg.outdata = request;
msg.outsize = request_len;
- msg.indata = response;
msg.insize = response_len;
result = cros_ec_cmd_xfer(bus->ec, &msg);
if (result < 0)
- goto exit;
+ return result;
- result = ec_i2c_parse_response(response, i2c_msgs, &num);
+ result = ec_i2c_parse_response(msg.indata, i2c_msgs, &num);
if (result < 0)
- goto exit;
+ return result;
/* Indicate success by saying how many messages were sent */
- result = num;
-exit:
- if (request != bus->request_buf)
- kfree(request);
- if (response != bus->response_buf)
- kfree(response);
-
- return result;
+ return num;
}
static u32 ec_i2c_functionality(struct i2c_adapter *adap)
diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
index ffa989f..769f8f7 100644
--- a/drivers/input/keyboard/cros_ec_keyb.c
+++ b/drivers/input/keyboard/cros_ec_keyb.c
@@ -148,16 +148,19 @@ static void cros_ec_keyb_process(struct cros_ec_keyb *ckdev,
static int cros_ec_keyb_get_state(struct cros_ec_keyb *ckdev, uint8_t *kb_state)
{
+ int ret;
struct cros_ec_command msg = {
- .version = 0,
.command = EC_CMD_MKBP_STATE,
- .outdata = NULL,
- .outsize = 0,
- .indata = kb_state,
.insize = ckdev->cols,
};
- return cros_ec_cmd_xfer(ckdev->ec, &msg);
+ ret = cros_ec_cmd_xfer(ckdev->ec, &msg);
+ if (ret < 0)
+ return ret;
+
+ memcpy(kb_state, msg.indata, ckdev->cols);
+
+ return 0;
}
static irqreturn_t cros_ec_keyb_irq(int irq, void *data)
diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
index fc0c81e..c872e1b 100644
--- a/drivers/mfd/cros_ec.c
+++ b/drivers/mfd/cros_ec.c
@@ -74,15 +74,11 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
ret = ec_dev->cmd_xfer(ec_dev, msg);
if (msg->result == EC_RES_IN_PROGRESS) {
int i;
- struct cros_ec_command status_msg;
- struct ec_response_get_comms_status status;
+ struct cros_ec_command status_msg = { };
+ struct ec_response_get_comms_status *status;
- status_msg.version = 0;
status_msg.command = EC_CMD_GET_COMMS_STATUS;
- status_msg.outdata = NULL;
- status_msg.outsize = 0;
- status_msg.indata = (uint8_t *)&status;
- status_msg.insize = sizeof(status);
+ status_msg.insize = sizeof(*status);
/*
* Query the EC's status until it's no longer busy or
@@ -98,7 +94,10 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
msg->result = status_msg.result;
if (status_msg.result != EC_RES_SUCCESS)
break;
- if (!(status.flags & EC_COMMS_STATUS_PROCESSING))
+
+ status = (struct ec_response_get_comms_status *)
+ status_msg.indata;
+ if (!(status->flags & EC_COMMS_STATUS_PROCESSING))
break;
}
}
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
index 0e166b9..71675b1 100644
--- a/include/linux/mfd/cros_ec.h
+++ b/include/linux/mfd/cros_ec.h
@@ -38,20 +38,20 @@ enum {
/*
* @version: Command version number (often 0)
* @command: Command to send (EC_CMD_...)
- * @outdata: Outgoing data to EC
* @outsize: Outgoing length in bytes
- * @indata: Where to put the incoming data from EC
* @insize: Max number of bytes to accept from EC
* @result: EC's response to the command (separate from communication failure)
+ * @outdata: Outgoing data to EC
+ * @indata: Where to put the incoming data from EC
*/
struct cros_ec_command {
uint32_t version;
uint32_t command;
- uint8_t *outdata;
uint32_t outsize;
- uint8_t *indata;
uint32_t insize;
uint32_t result;
+ uint8_t outdata[EC_PROTO2_MAX_PARAM_SIZE];
+ uint8_t indata[EC_PROTO2_MAX_PARAM_SIZE];
};
/**
--
2.1.3
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH RESEND v2 2/7] mfd: cros_ec: Add char dev and virtual dev pointers
2015-01-02 13:32 [PATCH RESEND v2 0/7] platform/chrome: Add user-space dev inferface support Javier Martinez Canillas
2015-01-02 13:32 ` [PATCH RESEND v2 1/7] mfd: cros_ec: Use fixed size arrays to transfer data with the EC Javier Martinez Canillas
@ 2015-01-02 13:32 ` Javier Martinez Canillas
2015-01-20 7:50 ` Lee Jones
2015-01-02 13:32 ` [PATCH RESEND v2 3/7] mfd: cros_ec: Add cros_ec_lpc driver for x86 devices Javier Martinez Canillas
` (5 subsequent siblings)
7 siblings, 1 reply; 35+ messages in thread
From: Javier Martinez Canillas @ 2015-01-02 13:32 UTC (permalink / raw)
To: Lee Jones
Cc: Olof Johansson, Doug Anderson, Bill Richardson, Simon Glass,
Gwendal Grignou, Jonathan Corbet, linux-samsung-soc,
linux-kernel, Javier Martinez Canillas
The ChromeOS Embedded Controller has to be accessed by applications.
A virtual character device is used as an interface with user-space.
Extend the struct cros_ec_device with the fields needed by the driver
of this virtual character device.
Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
Changes since v1: None, new patch.
include/linux/mfd/cros_ec.h | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
index 71675b1..324a346 100644
--- a/include/linux/mfd/cros_ec.h
+++ b/include/linux/mfd/cros_ec.h
@@ -16,6 +16,7 @@
#ifndef __LINUX_MFD_CROS_EC_H
#define __LINUX_MFD_CROS_EC_H
+#include <linux/cdev.h>
#include <linux/notifier.h>
#include <linux/mfd/cros_ec_commands.h>
#include <linux/mutex.h>
@@ -59,9 +60,17 @@ 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
+ * @cdev: Character device structure for virtual comms device
* @was_wake_device: true if this device was set to wake the system from
* sleep at the last suspend
+ * @cmd_readmem: 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 +99,12 @@ struct cros_ec_device {
const char *ec_name;
const char *phys_name;
struct device *dev;
+ struct device *vdev;
+ struct cdev cdev;
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;
--
2.1.3
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH RESEND v2 3/7] mfd: cros_ec: Add cros_ec_lpc driver for x86 devices
2015-01-02 13:32 [PATCH RESEND v2 0/7] platform/chrome: Add user-space dev inferface support Javier Martinez Canillas
2015-01-02 13:32 ` [PATCH RESEND v2 1/7] mfd: cros_ec: Use fixed size arrays to transfer data with the EC Javier Martinez Canillas
2015-01-02 13:32 ` [PATCH RESEND v2 2/7] mfd: cros_ec: Add char dev and virtual dev pointers Javier Martinez Canillas
@ 2015-01-02 13:32 ` Javier Martinez Canillas
2015-01-20 8:11 ` Lee Jones
2015-01-02 13:32 ` [PATCH RESEND v2 4/7] platform/chrome: Add Chrome OS EC userspace device interface Javier Martinez Canillas
` (4 subsequent siblings)
7 siblings, 1 reply; 35+ messages in thread
From: Javier Martinez Canillas @ 2015-01-02 13:32 UTC (permalink / raw)
To: Lee Jones
Cc: Olof Johansson, Doug Anderson, Bill Richardson, Simon Glass,
Gwendal Grignou, Jonathan Corbet, linux-samsung-soc,
linux-kernel, Javier Martinez Canillas
From: Bill Richardson <wfrichar@chromium.org>
This adds the LPC interface to the Chrome OS EC. Like the
I2C and SPI drivers, this allows userspace access to the EC.
Signed-off-by: Bill Richardson <wfrichar@chromium.org>
Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
Changes since v1: None, new patch.
drivers/mfd/Kconfig | 10 ++
drivers/mfd/Makefile | 1 +
drivers/mfd/cros_ec_lpc.c | 305 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 316 insertions(+)
create mode 100644 drivers/mfd/cros_ec_lpc.c
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 2e6b731..7563786 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -110,6 +110,16 @@ config MFD_CROS_EC_I2C
a checksum. Failing accesses will be retried three times to
improve reliability.
+config MFD_CROS_EC_LPC
+ tristate "ChromeOS Embedded Controller (LPC)"
+ depends on MFD_CROS_EC
+
+ help
+ If you say Y here, you get support for talking to the ChromeOS EC
+ over an LPC bus. This uses a simple byte-level protocol with a
+ checksum. This is used for userspace access only. The kernel
+ typically has its own communication methods.
+
config MFD_CROS_EC_SPI
tristate "ChromeOS Embedded Controller (SPI)"
depends on MFD_CROS_EC && SPI && OF
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 53467e2..94c1516 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_MFD_ASIC3) += asic3.o tmio_core.o
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_LPC) += cros_ec_lpc.o
obj-$(CONFIG_MFD_CROS_EC_SPI) += cros_ec_spi.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_lpc.c b/drivers/mfd/cros_ec_lpc.c
new file mode 100644
index 0000000..700e4cf
--- /dev/null
+++ b/drivers/mfd/cros_ec_lpc.c
@@ -0,0 +1,305 @@
+/*
+ * cros_ec_lpc - LPC access to the Chrome OS Embedded Controller
+ *
+ * Copyright (C) 2012-2015 Google, Inc
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * 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.
+ *
+ * This driver uses the Chrome OS EC byte-level message-based protocol for
+ * communicating the keyboard state (which keys are pressed) from a keyboard EC
+ * to the AP over some bus (such as i2c, lpc, spi). The EC does debouncing,
+ * but everything else (including deghosting) is done here. The main
+ * motivation for this is to keep the EC firmware as simple as possible, since
+ * it cannot be easily upgraded and EC flash/IRAM space is relatively
+ * expensive.
+ */
+
+#include <linux/delay.h>
+#include <linux/mfd/cros_ec.h>
+#include <linux/mfd/cros_ec_commands.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/printk.h>
+
+#define MYNAME "cros_ec_lpc"
+
+static int ec_response_timed_out(void)
+{
+ unsigned long one_second = jiffies + HZ;
+
+ usleep_range(200, 300);
+ do {
+ if (!(inb(EC_LPC_ADDR_HOST_CMD) & EC_LPC_STATUS_BUSY_MASK))
+ return 0;
+ usleep_range(100, 200);
+ } while (time_before(jiffies, one_second));
+
+ return 1;
+}
+
+static int cros_ec_cmd_xfer_lpc(struct cros_ec_device *ec,
+ struct cros_ec_command *msg)
+{
+ struct ec_lpc_host_args args;
+ int csum;
+ int i;
+ int ret = 0;
+
+ if (msg->outsize > EC_PROTO2_MAX_PARAM_SIZE ||
+ msg->insize > EC_PROTO2_MAX_PARAM_SIZE) {
+ dev_err(ec->dev,
+ "invalid buffer sizes (out %d, in %d)\n",
+ msg->outsize, msg->insize);
+ return -EINVAL;
+ }
+
+ /* Now actually send the command to the EC and get the result */
+ args.flags = EC_HOST_ARGS_FLAG_FROM_HOST;
+ args.command_version = msg->version;
+ args.data_size = msg->outsize;
+
+ /* Initialize checksum */
+ csum = msg->command + args.flags +
+ args.command_version + args.data_size;
+
+ /* Copy data and update checksum */
+ for (i = 0; i < msg->outsize; i++) {
+ outb(msg->outdata[i], EC_LPC_ADDR_HOST_PARAM + i);
+ csum += msg->outdata[i];
+ }
+
+ /* Finalize checksum and write args */
+ args.checksum = csum & 0xFF;
+ outb(args.flags, EC_LPC_ADDR_HOST_ARGS);
+ outb(args.command_version, EC_LPC_ADDR_HOST_ARGS + 1);
+ outb(args.data_size, EC_LPC_ADDR_HOST_ARGS + 2);
+ outb(args.checksum, EC_LPC_ADDR_HOST_ARGS + 3);
+
+ /* Here we go */
+ outb(msg->command, EC_LPC_ADDR_HOST_CMD);
+
+ if (ec_response_timed_out()) {
+ dev_warn(ec->dev, "EC responsed timed out\n");
+ ret = -EIO;
+ goto done;
+ }
+
+ /* Check result */
+ msg->result = inb(EC_LPC_ADDR_HOST_DATA);
+ switch (msg->result) {
+ case EC_RES_SUCCESS:
+ break;
+ case EC_RES_IN_PROGRESS:
+ ret = -EAGAIN;
+ dev_dbg(ec->dev, "command 0x%02x in progress\n",
+ msg->command);
+ goto done;
+ default:
+ dev_dbg(ec->dev, "command 0x%02x returned %d\n",
+ msg->command, msg->result);
+ }
+
+ /* Read back args */
+ args.flags = inb(EC_LPC_ADDR_HOST_ARGS);
+ args.command_version = inb(EC_LPC_ADDR_HOST_ARGS + 1);
+ args.data_size = inb(EC_LPC_ADDR_HOST_ARGS + 2);
+ args.checksum = inb(EC_LPC_ADDR_HOST_ARGS + 3);
+
+ if (args.data_size > msg->insize) {
+ dev_err(ec->dev,
+ "packet too long (%d bytes, expected %d)",
+ args.data_size, msg->insize);
+ ret = -ENOSPC;
+ goto done;
+ }
+
+ /* Start calculating response checksum */
+ csum = msg->command + args.flags +
+ args.command_version + args.data_size;
+
+ /* Read response and update checksum */
+ for (i = 0; i < args.data_size; i++) {
+ msg->indata[i] = inb(EC_LPC_ADDR_HOST_PARAM + i);
+ csum += msg->indata[i];
+ }
+
+ /* Verify checksum */
+ if (args.checksum != (csum & 0xFF)) {
+ dev_err(ec->dev,
+ "bad packet checksum, expected %02x, got %02x\n",
+ args.checksum, csum & 0xFF);
+ ret = -EBADMSG;
+ goto done;
+ }
+
+ /* Return actual amount of data received */
+ ret = args.data_size;
+done:
+ return ret;
+}
+
+/* Returns num bytes read, or negative on error. Doesn't need locking. */
+static int cros_ec_lpc_readmem(struct cros_ec_device *ec, unsigned int offset,
+ unsigned int bytes, void *dest)
+{
+ int i = offset;
+ char *s = dest;
+ int cnt = 0;
+
+ if (offset >= EC_MEMMAP_SIZE - bytes)
+ return -EINVAL;
+
+ /* fixed length */
+ if (bytes) {
+ for (; cnt < bytes; i++, s++, cnt++)
+ *s = inb(EC_LPC_ADDR_MEMMAP + i);
+ return cnt;
+ }
+
+ /* string */
+ for (; i < EC_MEMMAP_SIZE; i++, s++) {
+ *s = inb(EC_LPC_ADDR_MEMMAP + i);
+ cnt++;
+ if (!*s)
+ break;
+ }
+
+ return cnt;
+}
+
+static int cros_ec_lpc_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct cros_ec_device *ec_dev;
+ int err = -ENOTTY;
+
+ if (!request_region(EC_LPC_ADDR_MEMMAP, EC_MEMMAP_SIZE, MYNAME)) {
+ dev_warn(dev, "couldn't reserve memmap region\n");
+ goto failed_memmap;
+ }
+
+ if ((inb(EC_LPC_ADDR_MEMMAP + EC_MEMMAP_ID) != 'E') ||
+ (inb(EC_LPC_ADDR_MEMMAP + EC_MEMMAP_ID + 1) != 'C')) {
+ dev_warn(dev, "EC ID not detected\n");
+ goto failed_ec_probe;
+ }
+
+ if (!request_region(EC_HOST_CMD_REGION0, EC_HOST_CMD_REGION_SIZE,
+ MYNAME)) {
+ dev_warn(dev, "couldn't reserve region0\n");
+ goto failed_region0;
+ }
+ if (!request_region(EC_HOST_CMD_REGION1, EC_HOST_CMD_REGION_SIZE,
+ MYNAME)) {
+ dev_warn(dev, "couldn't reserve region1\n");
+ goto failed_region1;
+ }
+
+ ec_dev = devm_kzalloc(dev, sizeof(*ec_dev), GFP_KERNEL);
+ if (!ec_dev) {
+ err = -ENOMEM;
+ goto failed_ec_dev;
+ }
+
+ platform_set_drvdata(pdev, ec_dev);
+ ec_dev->dev = dev;
+ ec_dev->ec_name = pdev->name;
+ ec_dev->phys_name = dev_name(dev);
+ ec_dev->parent = dev;
+ ec_dev->cmd_xfer = cros_ec_cmd_xfer_lpc;
+ ec_dev->cmd_readmem = cros_ec_lpc_readmem;
+
+ err = cros_ec_register(ec_dev);
+ if (err) {
+ dev_warn(dev, "couldn't register ec_dev\n");
+ goto failed_ec_dev;
+ }
+
+ return 0;
+
+failed_ec_dev:
+ release_region(EC_HOST_CMD_REGION1, EC_HOST_CMD_REGION_SIZE);
+failed_region1:
+ release_region(EC_HOST_CMD_REGION0, EC_HOST_CMD_REGION_SIZE);
+failed_region0:
+failed_ec_probe:
+ release_region(EC_LPC_ADDR_MEMMAP, EC_MEMMAP_SIZE);
+failed_memmap:
+ return err;
+}
+
+static int cros_ec_lpc_remove(struct platform_device *pdev)
+{
+ struct cros_ec_device *ec_dev;
+
+ ec_dev = platform_get_drvdata(pdev);
+ cros_ec_remove(ec_dev);
+
+ release_region(EC_HOST_CMD_REGION1, EC_HOST_CMD_REGION_SIZE);
+ release_region(EC_HOST_CMD_REGION0, EC_HOST_CMD_REGION_SIZE);
+ release_region(EC_LPC_ADDR_MEMMAP, EC_MEMMAP_SIZE);
+
+ return 0;
+}
+
+static struct platform_driver cros_ec_lpc_driver = {
+ .driver = {
+ .name = MYNAME,
+ .owner = THIS_MODULE,
+ },
+ .probe = cros_ec_lpc_probe,
+ .remove = cros_ec_lpc_remove,
+};
+
+static void do_nothing(struct device *dev)
+{
+ /* not a physical device */
+}
+
+static struct platform_device cros_ec_lpc_device = {
+ .name = MYNAME,
+ .dev = {
+ .release = do_nothing,
+ },
+};
+
+static int __init cros_ec_lpc_init(void)
+{
+ int ret;
+
+ /* Register the driver */
+ ret = platform_driver_register(&cros_ec_lpc_driver);
+ if (ret < 0) {
+ pr_warn(MYNAME ": can't register driver: %d\n", ret);
+ return ret;
+ }
+
+ /* Register the device, and it'll get hooked up automatically */
+ ret = platform_device_register(&cros_ec_lpc_device);
+ if (ret < 0) {
+ pr_warn(MYNAME ": can't register device: %d\n", ret);
+ platform_driver_unregister(&cros_ec_lpc_driver);
+ return ret;
+ }
+
+ return 0;
+}
+
+static void __exit cros_ec_lpc_exit(void)
+{
+ platform_device_unregister(&cros_ec_lpc_device);
+ platform_driver_unregister(&cros_ec_lpc_driver);
+}
+
+module_init(cros_ec_lpc_init);
+module_exit(cros_ec_lpc_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("ChromeOS EC LPC driver");
--
2.1.3
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH RESEND v2 4/7] platform/chrome: Add Chrome OS EC userspace device interface
2015-01-02 13:32 [PATCH RESEND v2 0/7] platform/chrome: Add user-space dev inferface support Javier Martinez Canillas
` (2 preceding siblings ...)
2015-01-02 13:32 ` [PATCH RESEND v2 3/7] mfd: cros_ec: Add cros_ec_lpc driver for x86 devices Javier Martinez Canillas
@ 2015-01-02 13:32 ` Javier Martinez Canillas
2015-01-13 23:40 ` Gwendal Grignou
2015-01-02 13:32 ` [PATCH RESEND v2 5/7] mfd: cros_ec: Instantiate ChromeOS EC character device Javier Martinez Canillas
` (3 subsequent siblings)
7 siblings, 1 reply; 35+ messages in thread
From: Javier Martinez Canillas @ 2015-01-02 13:32 UTC (permalink / raw)
To: Lee Jones
Cc: Olof Johansson, Doug Anderson, Bill Richardson, Simon Glass,
Gwendal Grignou, Jonathan Corbet, linux-samsung-soc,
linux-kernel, Javier Martinez Canillas
From: Bill Richardson <wfrichar@chromium.org>
This patch adds a device interface to access the
Chrome OS Embedded Controller from user-space.
Signed-off-by: Bill Richardson <wfrichar@chromium.org>
Reviewed-by: Simon Glass <sjg@google.com>
Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
Changes since v1:
- The cros_ec_dev driver does not belong to drivers/mfd (Lee Jones)
- Don't call class_create in the probe function (Gwendal Grignou)
- Don't use the deprecated register_chrdev() function (Gwendal Grignou)
- Remove struct cros_ec_device *ec global variable (Lee Jones)
- Arrange structures to be 64-bit safe instead using the IOCTL compact API
(Alan Cox)
- Use _CHARDEV instead of _DEV as a postfix to denote that is a character
device user-space interface (Lee Jones)
- Remove the CROS_CLASS_NAME define and just use the name directly (Lee Jones)
- Remove unncessary include headers (Lee Jones)
- Add a newline when appropiate and remove double new lines (Lee Jones)
- If *offset == 0, there is no need to do *offset +=, just use = (Lee Jones)
- Use dev_err() instead of pr_err() when possible (Lee Jones)
- Remove unnecesary goto and just return an error instead (Lee Jones)
- Don't set .owner to THIS_MODULE since that is made by the core (Lee Jones)
- Document the ioctl number used in Documentation/ioctl/ioctl-number.txt
Documentation/ioctl/ioctl-number.txt | 1 +
drivers/platform/chrome/Kconfig | 14 +-
drivers/platform/chrome/Makefile | 1 +
drivers/platform/chrome/cros_ec_dev.c | 268 ++++++++++++++++++++++++++++++++++
drivers/platform/chrome/cros_ec_dev.h | 47 ++++++
5 files changed, 328 insertions(+), 3 deletions(-)
create mode 100644 drivers/platform/chrome/cros_ec_dev.c
create mode 100644 drivers/platform/chrome/cros_ec_dev.h
diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt
index 8136e1f..51f4221 100644
--- a/Documentation/ioctl/ioctl-number.txt
+++ b/Documentation/ioctl/ioctl-number.txt
@@ -321,6 +321,7 @@ Code Seq#(hex) Include File Comments
0xDB 00-0F drivers/char/mwave/mwavepub.h
0xDD 00-3F ZFCP device driver see drivers/s390/scsi/
<mailto:aherrman@de.ibm.com>
+0xEC 00-01 drivers/platform/chrome/cros_ec_dev.h ChromeOS EC driver
0xF3 00-3F drivers/usb/misc/sisusbvga/sisusb.h sisfb (in development)
<mailto:thomas@winischhofer.net>
0xF4 00-1F video/mbxfb.h mbxfb
diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
index 440ed77..75dc514 100644
--- a/drivers/platform/chrome/Kconfig
+++ b/drivers/platform/chrome/Kconfig
@@ -4,7 +4,7 @@
menuconfig CHROME_PLATFORMS
bool "Platform support for Chrome hardware"
- depends on X86
+ depends on X86 || ARM
---help---
Say Y here to get to see options for platform support for
various Chromebooks and Chromeboxes. This option alone does
@@ -16,8 +16,7 @@ if CHROME_PLATFORMS
config CHROMEOS_LAPTOP
tristate "Chrome OS Laptop"
- depends on I2C
- depends on DMI
+ depends on I2C && DMI && X86
---help---
This driver instantiates i2c and smbus devices such as
light sensors and touchpads.
@@ -27,6 +26,7 @@ config CHROMEOS_LAPTOP
config CHROMEOS_PSTORE
tristate "Chrome OS pstore support"
+ depends on X86
---help---
This module instantiates the persistent storage on x86 ChromeOS
devices. It can be used to store away console logs and crash
@@ -38,5 +38,13 @@ config CHROMEOS_PSTORE
If you have a supported Chromebook, choose Y or M here.
The module will be called chromeos_pstore.
+config CROS_EC_CHARDEV
+ tristate "Chrome OS Embedded Controller userspace device interface"
+ depends on MFD_CROS_EC
+ ---help---
+ This driver adds support to talk with the ChromeOS EC from userspace.
+
+ If you have a supported Chromebook, choose Y or M here.
+ The module will be called cros_ec_dev.
endif # CHROMEOS_PLATFORMS
diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
index 2b860ca..10f1361 100644
--- a/drivers/platform/chrome/Makefile
+++ b/drivers/platform/chrome/Makefile
@@ -1,3 +1,4 @@
obj-$(CONFIG_CHROMEOS_LAPTOP) += chromeos_laptop.o
obj-$(CONFIG_CHROMEOS_PSTORE) += chromeos_pstore.o
+obj-$(CONFIG_CROS_EC_CHARDEV) += cros_ec_dev.o
diff --git a/drivers/platform/chrome/cros_ec_dev.c b/drivers/platform/chrome/cros_ec_dev.c
new file mode 100644
index 0000000..04cc8eb
--- /dev/null
+++ b/drivers/platform/chrome/cros_ec_dev.c
@@ -0,0 +1,268 @@
+/*
+ * cros_ec_dev - expose the Chrome OS Embedded Controller to user-space
+ *
+ * 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/>.
+ */
+
+#include <linux/fs.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/uaccess.h>
+
+#include "cros_ec_dev.h"
+
+/* Device variables */
+#define CROS_MAX_DEV 128
+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 = { 0 },
+ .outsize = 0,
+ .indata = { 0 },
+ .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;
+ }
+
+ resp = (struct ec_response_get_version *)msg.indata;
+ 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)
+{
+ filp->private_data = container_of(inode->i_cdev,
+ struct cros_ec_device, cdev);
+ 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)
+{
+ struct cros_ec_device *ec = filp->private_data;
+ 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(struct cros_ec_device *ec, void __user *arg)
+{
+ long ret;
+ struct cros_ec_command s_cmd = { };
+
+ if (copy_from_user(&s_cmd, arg, sizeof(s_cmd)))
+ return -EFAULT;
+
+ ret = cros_ec_cmd_xfer(ec, &s_cmd);
+ /* Only copy data to userland if data was received. */
+ if (ret < 0)
+ return ret;
+
+ if (copy_to_user(arg, &s_cmd, sizeof(s_cmd)))
+ return -EFAULT;
+
+ return 0;
+}
+
+static long ec_device_ioctl_readmem(struct cros_ec_device *ec, void __user *arg)
+{
+ struct cros_ec_readmem s_mem = { };
+ long num;
+
+ /* Not every platform supports direct reads */
+ if (!ec->cmd_readmem)
+ return -ENOTTY;
+
+ if (copy_from_user(&s_mem, arg, sizeof(s_mem)))
+ return -EFAULT;
+
+ num = ec->cmd_readmem(ec, s_mem.offset, s_mem.bytes, s_mem.buffer);
+ if (num <= 0)
+ return num;
+
+ if (copy_to_user((void __user *)arg, &s_mem, sizeof(s_mem)))
+ return -EFAULT;
+
+ return 0;
+}
+
+static long ec_device_ioctl(struct file *filp, unsigned int cmd,
+ unsigned long arg)
+{
+ struct cros_ec_device *ec = filp->private_data;
+
+ if (_IOC_TYPE(cmd) != CROS_EC_DEV_IOC)
+ return -ENOTTY;
+
+ switch (cmd) {
+ case CROS_EC_DEV_IOCXCMD:
+ return ec_device_ioctl_xcmd(ec, (void __user *)arg);
+ case CROS_EC_DEV_IOCRDMEM:
+ return ec_device_ioctl_readmem(ec, (void __user *)arg);
+ }
+
+ return -ENOTTY;
+}
+
+/* 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,
+};
+
+static int ec_device_probe(struct platform_device *pdev)
+{
+ struct cros_ec_device *ec = dev_get_drvdata(pdev->dev.parent);
+ int retval = -ENOTTY;
+ dev_t devno = MKDEV(ec_major, 0);
+
+ /* Instantiate it (and remember the EC) */
+ cdev_init(&ec->cdev, &fops);
+
+ retval = cdev_add(&ec->cdev, devno, 1);
+ if (retval) {
+ dev_err(&pdev->dev, ": failed to add character device\n");
+ return retval;
+ }
+
+ ec->vdev = device_create(cros_class, NULL, devno, ec,
+ CROS_EC_DEV_NAME);
+ if (IS_ERR(ec->vdev)) {
+ retval = PTR_ERR(ec->vdev);
+ dev_err(&pdev->dev, ": failed to create device\n");
+ cdev_del(&ec->cdev);
+ return retval;
+ }
+
+ return 0;
+}
+
+static int ec_device_remove(struct platform_device *pdev)
+{
+ struct cros_ec_device *ec = dev_get_drvdata(pdev->dev.parent);
+
+ device_destroy(cros_class, MKDEV(ec_major, 0));
+ cdev_del(&ec->cdev);
+ return 0;
+}
+
+static struct platform_driver cros_ec_dev_driver = {
+ .driver = {
+ .name = "cros-ec-dev",
+ },
+ .probe = ec_device_probe,
+ .remove = ec_device_remove,
+};
+
+static int __init cros_ec_dev_init(void)
+{
+ int ret;
+ dev_t dev = 0;
+
+ cros_class = class_create(THIS_MODULE, "chromeos");
+ if (IS_ERR(cros_class)) {
+ pr_err(CROS_EC_DEV_NAME ": failed to register device class\n");
+ return PTR_ERR(cros_class);
+ }
+
+ /* Get a range of minor numbers (starting with 0) to work with */
+ ret = alloc_chrdev_region(&dev, 0, CROS_MAX_DEV, CROS_EC_DEV_NAME);
+ if (ret < 0) {
+ pr_err(CROS_EC_DEV_NAME ": alloc_chrdev_region() failed\n");
+ goto failed_chrdevreg;
+ }
+ ec_major = MAJOR(dev);
+
+ /* Register the driver */
+ ret = platform_driver_register(&cros_ec_dev_driver);
+ if (ret < 0) {
+ pr_warn(CROS_EC_DEV_NAME ": can't register driver: %d\n", ret);
+ goto failed_devreg;
+ }
+ return 0;
+
+failed_devreg:
+ unregister_chrdev_region(MKDEV(ec_major, 0), CROS_MAX_DEV);
+failed_chrdevreg:
+ class_destroy(cros_class);
+ return ret;
+}
+
+static void __exit cros_ec_dev_exit(void)
+{
+ platform_driver_unregister(&cros_ec_dev_driver);
+ unregister_chrdev(ec_major, CROS_EC_DEV_NAME);
+ class_destroy(cros_class);
+}
+
+module_init(cros_ec_dev_init);
+module_exit(cros_ec_dev_exit);
+
+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/drivers/platform/chrome/cros_ec_dev.h b/drivers/platform/chrome/cros_ec_dev.h
new file mode 100644
index 0000000..15c54c4
--- /dev/null
+++ b/drivers/platform/chrome/cros_ec_dev.h
@@ -0,0 +1,47 @@
+/*
+ * 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/>.
+ */
+
+#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;
+ uint8_t buffer[EC_MEMMAP_SIZE];
+};
+
+#define CROS_EC_DEV_IOC 0xEC
+#define CROS_EC_DEV_IOCXCMD _IOWR(CROS_EC_DEV_IOC, 0, struct cros_ec_command)
+#define CROS_EC_DEV_IOCRDMEM _IOWR(CROS_EC_DEV_IOC, 1, struct cros_ec_readmem)
+
+#endif /* _CROS_EC_DEV_H_ */
--
2.1.3
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH RESEND v2 5/7] mfd: cros_ec: Instantiate ChromeOS EC character device
2015-01-02 13:32 [PATCH RESEND v2 0/7] platform/chrome: Add user-space dev inferface support Javier Martinez Canillas
` (3 preceding siblings ...)
2015-01-02 13:32 ` [PATCH RESEND v2 4/7] platform/chrome: Add Chrome OS EC userspace device interface Javier Martinez Canillas
@ 2015-01-02 13:32 ` Javier Martinez Canillas
2015-01-20 8:20 ` Lee Jones
2015-01-02 13:32 ` [PATCH RESEND v2 6/7] platform/chrome: Create sysfs attributes for the ChromeOS EC Javier Martinez Canillas
` (2 subsequent siblings)
7 siblings, 1 reply; 35+ messages in thread
From: Javier Martinez Canillas @ 2015-01-02 13:32 UTC (permalink / raw)
To: Lee Jones
Cc: Olof Johansson, Doug Anderson, Bill Richardson, Simon Glass,
Gwendal Grignou, Jonathan Corbet, linux-samsung-soc,
linux-kernel, Javier Martinez Canillas
The ChromeOS EC character device is an user-space interface to
allow applications to access the Embedded Controller.
Add a cell for this device so it's spawned from the mfd driver.
Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
Changes since v1: None, new patch.
drivers/mfd/cros_ec.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
index c872e1b..70f9ed5 100644
--- a/drivers/mfd/cros_ec.c
+++ b/drivers/mfd/cros_ec.c
@@ -118,6 +118,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)
--
2.1.3
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH RESEND v2 6/7] platform/chrome: Create sysfs attributes for the ChromeOS EC.
2015-01-02 13:32 [PATCH RESEND v2 0/7] platform/chrome: Add user-space dev inferface support Javier Martinez Canillas
` (4 preceding siblings ...)
2015-01-02 13:32 ` [PATCH RESEND v2 5/7] mfd: cros_ec: Instantiate ChromeOS EC character device Javier Martinez Canillas
@ 2015-01-02 13:32 ` Javier Martinez Canillas
2015-01-02 13:32 ` [PATCH RESEND v2 7/7] platform/chrome: Expose Chrome OS Lightbar to users Javier Martinez Canillas
2015-01-12 10:18 ` [PATCH RESEND v2 0/7] platform/chrome: Add user-space dev inferface support Javier Martinez Canillas
7 siblings, 0 replies; 35+ messages in thread
From: Javier Martinez Canillas @ 2015-01-02 13:32 UTC (permalink / raw)
To: Lee Jones
Cc: Olof Johansson, Doug Anderson, Bill Richardson, Simon Glass,
Gwendal Grignou, Jonathan Corbet, linux-samsung-soc,
linux-kernel, 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>
---
Changes since v1:
- Moved from drivers/mfd to drivers/platform/chrome. Suggested by Lee Jones.
- Modify to use the fixed-size arrays for cros_ec_command in and out buffers.
drivers/platform/chrome/Makefile | 3 +-
drivers/platform/chrome/cros_ec_dev.c | 4 +
drivers/platform/chrome/cros_ec_dev.h | 3 +
drivers/platform/chrome/cros_ec_sysfs.c | 271 ++++++++++++++++++++++++++++++++
4 files changed, 280 insertions(+), 1 deletion(-)
create mode 100644 drivers/platform/chrome/cros_ec_sysfs.c
diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
index 10f1361..3e7980b 100644
--- a/drivers/platform/chrome/Makefile
+++ b/drivers/platform/chrome/Makefile
@@ -1,4 +1,5 @@
obj-$(CONFIG_CHROMEOS_LAPTOP) += chromeos_laptop.o
obj-$(CONFIG_CHROMEOS_PSTORE) += chromeos_pstore.o
-obj-$(CONFIG_CROS_EC_CHARDEV) += cros_ec_dev.o
+cros_ec_devs-objs := cros_ec_dev.o cros_ec_sysfs.o
+obj-$(CONFIG_CROS_EC_CHARDEV) += cros_ec_devs.o
diff --git a/drivers/platform/chrome/cros_ec_dev.c b/drivers/platform/chrome/cros_ec_dev.c
index 04cc8eb..557f2e8 100644
--- a/drivers/platform/chrome/cros_ec_dev.c
+++ b/drivers/platform/chrome/cros_ec_dev.c
@@ -198,6 +198,9 @@ static int ec_device_probe(struct platform_device *pdev)
return retval;
}
+ /* Initialize extra interfaces */
+ ec_dev_sysfs_init(ec);
+
return 0;
}
@@ -205,6 +208,7 @@ static int ec_device_remove(struct platform_device *pdev)
{
struct cros_ec_device *ec = dev_get_drvdata(pdev->dev.parent);
+ ec_dev_sysfs_remove(ec);
device_destroy(cros_class, MKDEV(ec_major, 0));
cdev_del(&ec->cdev);
return 0;
diff --git a/drivers/platform/chrome/cros_ec_dev.h b/drivers/platform/chrome/cros_ec_dev.h
index 15c54c4..f036132 100644
--- a/drivers/platform/chrome/cros_ec_dev.h
+++ b/drivers/platform/chrome/cros_ec_dev.h
@@ -44,4 +44,7 @@ struct cros_ec_readmem {
#define CROS_EC_DEV_IOCXCMD _IOWR(CROS_EC_DEV_IOC, 0, struct cros_ec_command)
#define CROS_EC_DEV_IOCRDMEM _IOWR(CROS_EC_DEV_IOC, 1, struct cros_ec_readmem)
+void ec_dev_sysfs_init(struct cros_ec_device *);
+void ec_dev_sysfs_remove(struct cros_ec_device *);
+
#endif /* _CROS_EC_DEV_H_ */
diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
new file mode 100644
index 0000000..fb62ab6
--- /dev/null
+++ b/drivers/platform/chrome/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/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 cros_ec_command msg = { 0 };
+ struct ec_params_reboot_ec *param =
+ (struct ec_params_reboot_ec *)msg.outdata;
+ 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.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 };
+ 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.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);
+
+ r_ver = (struct ec_response_get_version *)msg.indata;
+ /* 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.insize = sizeof(msg.indata);
+ 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 {
+ msg.indata[sizeof(msg.indata) - 1] = '\0';
+ count += scnprintf(buf + count, PAGE_SIZE - count,
+ "Build info: %s\n", msg.indata);
+ }
+
+ /* Get chip info. */
+ msg.command = EC_CMD_GET_CHIP_INFO;
+ 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 = (struct ec_response_get_chip_info *)msg.indata;
+
+ 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.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 {
+ r_board = (struct ec_response_board_version *)msg.indata;
+
+ 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.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);
+
+ resp = (struct ec_response_flash_info *)msg.indata;
+
+ 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.3
^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH RESEND v2 7/7] platform/chrome: Expose Chrome OS Lightbar to users
2015-01-02 13:32 [PATCH RESEND v2 0/7] platform/chrome: Add user-space dev inferface support Javier Martinez Canillas
` (5 preceding siblings ...)
2015-01-02 13:32 ` [PATCH RESEND v2 6/7] platform/chrome: Create sysfs attributes for the ChromeOS EC Javier Martinez Canillas
@ 2015-01-02 13:32 ` Javier Martinez Canillas
2015-01-12 10:18 ` [PATCH RESEND v2 0/7] platform/chrome: Add user-space dev inferface support Javier Martinez Canillas
7 siblings, 0 replies; 35+ messages in thread
From: Javier Martinez Canillas @ 2015-01-02 13:32 UTC (permalink / raw)
To: Lee Jones
Cc: Olof Johansson, Doug Anderson, Bill Richardson, Simon Glass,
Gwendal Grignou, Jonathan Corbet, linux-samsung-soc,
linux-kernel, 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>
---
Olof,
Lee Jones suggested that this should live under drivers/led instead but this
an ad-hoc sysfs interface for the Chromebook Pixel lightbar and does not use
neither the LED class nor its sysfs interface (/sys/class/leds/).
Also the sysfs attributes are created in the ChromeOS EC chardev driver probe
function so I belive is part of that driver and it should be in the same dir.
Best regards,
Javier
Changes since v1:
- Moved from drivers/mfd to drivers/platform/chrome.
- Modify to use the fixed-size arrays for cros_ec_command in and out buffers.
drivers/platform/chrome/Makefile | 2 +-
drivers/platform/chrome/cros_ec_dev.c | 2 +
drivers/platform/chrome/cros_ec_dev.h | 3 +
drivers/platform/chrome/cros_ec_lightbar.c | 367 +++++++++++++++++++++++++++++
4 files changed, 373 insertions(+), 1 deletion(-)
create mode 100644 drivers/platform/chrome/cros_ec_lightbar.c
diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
index 3e7980b..cc5d2f2 100644
--- a/drivers/platform/chrome/Makefile
+++ b/drivers/platform/chrome/Makefile
@@ -1,5 +1,5 @@
obj-$(CONFIG_CHROMEOS_LAPTOP) += chromeos_laptop.o
obj-$(CONFIG_CHROMEOS_PSTORE) += chromeos_pstore.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_CROS_EC_CHARDEV) += cros_ec_devs.o
diff --git a/drivers/platform/chrome/cros_ec_dev.c b/drivers/platform/chrome/cros_ec_dev.c
index 557f2e8..7bbcb98 100644
--- a/drivers/platform/chrome/cros_ec_dev.c
+++ b/drivers/platform/chrome/cros_ec_dev.c
@@ -200,6 +200,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;
}
@@ -208,6 +209,7 @@ static int ec_device_remove(struct platform_device *pdev)
{
struct cros_ec_device *ec = dev_get_drvdata(pdev->dev.parent);
+ ec_dev_lightbar_remove(ec);
ec_dev_sysfs_remove(ec);
device_destroy(cros_class, MKDEV(ec_major, 0));
cdev_del(&ec->cdev);
diff --git a/drivers/platform/chrome/cros_ec_dev.h b/drivers/platform/chrome/cros_ec_dev.h
index f036132..45d67f7 100644
--- a/drivers/platform/chrome/cros_ec_dev.h
+++ b/drivers/platform/chrome/cros_ec_dev.h
@@ -47,4 +47,7 @@ struct cros_ec_readmem {
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 /* _CROS_EC_DEV_H_ */
diff --git a/drivers/platform/chrome/cros_ec_lightbar.c b/drivers/platform/chrome/cros_ec_lightbar.c
new file mode 100644
index 0000000..35fc892
--- /dev/null
+++ b/drivers/platform/chrome/cros_ec_lightbar.c
@@ -0,0 +1,367 @@
+/*
+ * 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/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, \
+ .outsize = sizeof(*P), \
+ .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 = (struct ec_params_lightbar *)msg.outdata;
+ 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:
+ resp = (struct ec_response_lightbar *)msg.indata;
+
+ /* 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 = (struct ec_params_lightbar *)msg.outdata;
+ 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;
+
+ ret = sscanf(buf, "%i", &val[i++]);
+ if (ret == 0)
+ return -EINVAL;
+
+ if (i == 4) {
+ param = (struct ec_params_lightbar *)msg.outdata;
+ 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 = (struct ec_params_lightbar *)msg.outdata;
+ 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);
+
+ resp = (struct ec_response_lightbar *)msg.indata;
+ 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)) {
+ ret = kstrtouint(buf, 0, &num);
+ if (ret)
+ return ret;
+ }
+
+ param = (struct ec_params_lightbar *)msg.outdata;
+ 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.3
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH RESEND v2 0/7] platform/chrome: Add user-space dev inferface support
2015-01-02 13:32 [PATCH RESEND v2 0/7] platform/chrome: Add user-space dev inferface support Javier Martinez Canillas
` (6 preceding siblings ...)
2015-01-02 13:32 ` [PATCH RESEND v2 7/7] platform/chrome: Expose Chrome OS Lightbar to users Javier Martinez Canillas
@ 2015-01-12 10:18 ` Javier Martinez Canillas
7 siblings, 0 replies; 35+ messages in thread
From: Javier Martinez Canillas @ 2015-01-12 10:18 UTC (permalink / raw)
To: Lee Jones, Olof Johansson
Cc: Doug Anderson, Bill Richardson, Simon Glass, Gwendal Grignou,
Jonathan Corbet, linux-samsung-soc, linux-kernel
Hello,
On 01/02/2015 02:32 PM, Javier Martinez Canillas wrote:
>
> The mainline ChromeOS Embedded Controller (EC) driver is still missing some
> features that are present in the downstream ChromiumOS tree. These are:
>
> - Low Pin Count (LPC) interface
> - User-space device 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 is a second version of a series that adds support for the first two of
> the missing features: the EC LPC and EC character device interfaces that
> are used by user-space to access the ChromeOS EC. The support patches were
> taken from the downstream ChromiumOS 3.14 tree with the fixes and cleanups
> squashed to have a minimal patch-set.
>
Any comments about this series? This is already a resend of a series posted
first on December, 9. I know it has been holiday season but I would really
appreciate some feedback since the patches have been in the list for a month.
Thanks a lot and best regards,
Javier
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH RESEND v2 4/7] platform/chrome: Add Chrome OS EC userspace device interface
2015-01-02 13:32 ` [PATCH RESEND v2 4/7] platform/chrome: Add Chrome OS EC userspace device interface Javier Martinez Canillas
@ 2015-01-13 23:40 ` Gwendal Grignou
0 siblings, 0 replies; 35+ messages in thread
From: Gwendal Grignou @ 2015-01-13 23:40 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Lee Jones, Olof Johansson, Doug Anderson, Bill Richardson,
Simon Glass, Jonathan Corbet, linux-samsung-soc, Linux Kernel
On Fri, Jan 2, 2015 at 5:32 AM, Javier Martinez Canillas
<javier.martinez@collabora.co.uk> wrote:
>
> From: Bill Richardson <wfrichar@chromium.org>
>
> This patch adds a device interface to access the
> Chrome OS Embedded Controller from user-space.
>
> Signed-off-by: Bill Richardson <wfrichar@chromium.org>
> Reviewed-by: Simon Glass <sjg@google.com>
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
> ---
>
> Changes since v1:
> - The cros_ec_dev driver does not belong to drivers/mfd (Lee Jones)
> - Don't call class_create in the probe function (Gwendal Grignou)
> - Don't use the deprecated register_chrdev() function (Gwendal Grignou)
> - Remove struct cros_ec_device *ec global variable (Lee Jones)
> - Arrange structures to be 64-bit safe instead using the IOCTL compact API
> (Alan Cox)
> - Use _CHARDEV instead of _DEV as a postfix to denote that is a character
> device user-space interface (Lee Jones)
> - Remove the CROS_CLASS_NAME define and just use the name directly (Lee Jones)
> - Remove unncessary include headers (Lee Jones)
> - Add a newline when appropiate and remove double new lines (Lee Jones)
> - If *offset == 0, there is no need to do *offset +=, just use = (Lee Jones)
> - Use dev_err() instead of pr_err() when possible (Lee Jones)
> - Remove unnecesary goto and just return an error instead (Lee Jones)
> - Don't set .owner to THIS_MODULE since that is made by the core (Lee Jones)
> - Document the ioctl number used in Documentation/ioctl/ioctl-number.txt
>
> Documentation/ioctl/ioctl-number.txt | 1 +
> drivers/platform/chrome/Kconfig | 14 +-
> drivers/platform/chrome/Makefile | 1 +
> drivers/platform/chrome/cros_ec_dev.c | 268 ++++++++++++++++++++++++++++++++++
> drivers/platform/chrome/cros_ec_dev.h | 47 ++++++
> 5 files changed, 328 insertions(+), 3 deletions(-)
> create mode 100644 drivers/platform/chrome/cros_ec_dev.c
> create mode 100644 drivers/platform/chrome/cros_ec_dev.h
>
> diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt
> index 8136e1f..51f4221 100644
> --- a/Documentation/ioctl/ioctl-number.txt
> +++ b/Documentation/ioctl/ioctl-number.txt
> @@ -321,6 +321,7 @@ Code Seq#(hex) Include File Comments
> 0xDB 00-0F drivers/char/mwave/mwavepub.h
> 0xDD 00-3F ZFCP device driver see drivers/s390/scsi/
> <mailto:aherrman@de.ibm.com>
> +0xEC 00-01 drivers/platform/chrome/cros_ec_dev.h ChromeOS EC driver
> 0xF3 00-3F drivers/usb/misc/sisusbvga/sisusb.h sisfb (in development)
> <mailto:thomas@winischhofer.net>
> 0xF4 00-1F video/mbxfb.h mbxfb
> diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
> index 440ed77..75dc514 100644
> --- a/drivers/platform/chrome/Kconfig
> +++ b/drivers/platform/chrome/Kconfig
> @@ -4,7 +4,7 @@
>
> menuconfig CHROME_PLATFORMS
> bool "Platform support for Chrome hardware"
> - depends on X86
> + depends on X86 || ARM
> ---help---
> Say Y here to get to see options for platform support for
> various Chromebooks and Chromeboxes. This option alone does
> @@ -16,8 +16,7 @@ if CHROME_PLATFORMS
>
> config CHROMEOS_LAPTOP
> tristate "Chrome OS Laptop"
> - depends on I2C
> - depends on DMI
> + depends on I2C && DMI && X86
> ---help---
> This driver instantiates i2c and smbus devices such as
> light sensors and touchpads.
> @@ -27,6 +26,7 @@ config CHROMEOS_LAPTOP
>
> config CHROMEOS_PSTORE
> tristate "Chrome OS pstore support"
> + depends on X86
> ---help---
> This module instantiates the persistent storage on x86 ChromeOS
> devices. It can be used to store away console logs and crash
> @@ -38,5 +38,13 @@ config CHROMEOS_PSTORE
> If you have a supported Chromebook, choose Y or M here.
> The module will be called chromeos_pstore.
>
> +config CROS_EC_CHARDEV
> + tristate "Chrome OS Embedded Controller userspace device interface"
> + depends on MFD_CROS_EC
> + ---help---
> + This driver adds support to talk with the ChromeOS EC from userspace.
> +
> + If you have a supported Chromebook, choose Y or M here.
> + The module will be called cros_ec_dev.
>
> endif # CHROMEOS_PLATFORMS
> diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
> index 2b860ca..10f1361 100644
> --- a/drivers/platform/chrome/Makefile
> +++ b/drivers/platform/chrome/Makefile
> @@ -1,3 +1,4 @@
>
> obj-$(CONFIG_CHROMEOS_LAPTOP) += chromeos_laptop.o
> obj-$(CONFIG_CHROMEOS_PSTORE) += chromeos_pstore.o
> +obj-$(CONFIG_CROS_EC_CHARDEV) += cros_ec_dev.o
> diff --git a/drivers/platform/chrome/cros_ec_dev.c b/drivers/platform/chrome/cros_ec_dev.c
> new file mode 100644
> index 0000000..04cc8eb
> --- /dev/null
> +++ b/drivers/platform/chrome/cros_ec_dev.c
> @@ -0,0 +1,268 @@
> +/*
> + * cros_ec_dev - expose the Chrome OS Embedded Controller to user-space
> + *
> + * 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/>.
> + */
> +
> +#include <linux/fs.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/uaccess.h>
> +
> +#include "cros_ec_dev.h"
> +
> +/* Device variables */
> +#define CROS_MAX_DEV 128
> +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 = { 0 },
> + .outsize = 0,
> + .indata = { 0 },
> + .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;
> + }
> +
> + resp = (struct ec_response_get_version *)msg.indata;
> + 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)
> +{
> + filp->private_data = container_of(inode->i_cdev,
> + struct cros_ec_device, cdev);
> + 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)
> +{
> + struct cros_ec_device *ec = filp->private_data;
> + 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(struct cros_ec_device *ec, void __user *arg)
> +{
> + long ret;
> + struct cros_ec_command s_cmd = { };
> +
> + if (copy_from_user(&s_cmd, arg, sizeof(s_cmd)))
> + return -EFAULT;
> +
> + ret = cros_ec_cmd_xfer(ec, &s_cmd);
> + /* Only copy data to userland if data was received. */
> + if (ret < 0)
> + return ret;
> +
> + if (copy_to_user(arg, &s_cmd, sizeof(s_cmd)))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> +static long ec_device_ioctl_readmem(struct cros_ec_device *ec, void __user *arg)
> +{
> + struct cros_ec_readmem s_mem = { };
> + long num;
> +
> + /* Not every platform supports direct reads */
> + if (!ec->cmd_readmem)
> + return -ENOTTY;
> +
> + if (copy_from_user(&s_mem, arg, sizeof(s_mem)))
> + return -EFAULT;
> +
> + num = ec->cmd_readmem(ec, s_mem.offset, s_mem.bytes, s_mem.buffer);
> + if (num <= 0)
> + return num;
> +
> + if (copy_to_user((void __user *)arg, &s_mem, sizeof(s_mem)))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> +static long ec_device_ioctl(struct file *filp, unsigned int cmd,
> + unsigned long arg)
> +{
> + struct cros_ec_device *ec = filp->private_data;
> +
> + if (_IOC_TYPE(cmd) != CROS_EC_DEV_IOC)
> + return -ENOTTY;
> +
> + switch (cmd) {
> + case CROS_EC_DEV_IOCXCMD:
> + return ec_device_ioctl_xcmd(ec, (void __user *)arg);
> + case CROS_EC_DEV_IOCRDMEM:
> + return ec_device_ioctl_readmem(ec, (void __user *)arg);
> + }
> +
> + return -ENOTTY;
> +}
> +
> +/* 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,
> +};
> +
> +static int ec_device_probe(struct platform_device *pdev)
> +{
> + struct cros_ec_device *ec = dev_get_drvdata(pdev->dev.parent);
> + int retval = -ENOTTY;
> + dev_t devno = MKDEV(ec_major, 0);
> +
> + /* Instantiate it (and remember the EC) */
> + cdev_init(&ec->cdev, &fops);
> +
> + retval = cdev_add(&ec->cdev, devno, 1);
> + if (retval) {
> + dev_err(&pdev->dev, ": failed to add character device\n");
> + return retval;
> + }
> +
> + ec->vdev = device_create(cros_class, NULL, devno, ec,
> + CROS_EC_DEV_NAME);
> + if (IS_ERR(ec->vdev)) {
> + retval = PTR_ERR(ec->vdev);
> + dev_err(&pdev->dev, ": failed to create device\n");
> + cdev_del(&ec->cdev);
> + return retval;
> + }
> +
> + return 0;
> +}
> +
> +static int ec_device_remove(struct platform_device *pdev)
> +{
> + struct cros_ec_device *ec = dev_get_drvdata(pdev->dev.parent);
> +
> + device_destroy(cros_class, MKDEV(ec_major, 0));
> + cdev_del(&ec->cdev);
> + return 0;
> +}
> +
> +static struct platform_driver cros_ec_dev_driver = {
> + .driver = {
> + .name = "cros-ec-dev",
> + },
> + .probe = ec_device_probe,
> + .remove = ec_device_remove,
> +};
> +
> +static int __init cros_ec_dev_init(void)
> +{
> + int ret;
> + dev_t dev = 0;
> +
> + cros_class = class_create(THIS_MODULE, "chromeos");
> + if (IS_ERR(cros_class)) {
> + pr_err(CROS_EC_DEV_NAME ": failed to register device class\n");
> + return PTR_ERR(cros_class);
> + }
> +
> + /* Get a range of minor numbers (starting with 0) to work with */
> + ret = alloc_chrdev_region(&dev, 0, CROS_MAX_DEV, CROS_EC_DEV_NAME);
> + if (ret < 0) {
> + pr_err(CROS_EC_DEV_NAME ": alloc_chrdev_region() failed\n");
> + goto failed_chrdevreg;
> + }
> + ec_major = MAJOR(dev);
> +
> + /* Register the driver */
> + ret = platform_driver_register(&cros_ec_dev_driver);
> + if (ret < 0) {
> + pr_warn(CROS_EC_DEV_NAME ": can't register driver: %d\n", ret);
> + goto failed_devreg;
> + }
> + return 0;
> +
> +failed_devreg:
> + unregister_chrdev_region(MKDEV(ec_major, 0), CROS_MAX_DEV);
> +failed_chrdevreg:
> + class_destroy(cros_class);
> + return ret;
> +}
> +
> +static void __exit cros_ec_dev_exit(void)
> +{
> + platform_driver_unregister(&cros_ec_dev_driver);
> + unregister_chrdev(ec_major, CROS_EC_DEV_NAME);
> + class_destroy(cros_class);
> +}
> +
> +module_init(cros_ec_dev_init);
> +module_exit(cros_ec_dev_exit);
> +
> +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/drivers/platform/chrome/cros_ec_dev.h b/drivers/platform/chrome/cros_ec_dev.h
> new file mode 100644
> index 0000000..15c54c4
> --- /dev/null
> +++ b/drivers/platform/chrome/cros_ec_dev.h
> @@ -0,0 +1,47 @@
> +/*
> + * 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/>.
> + */
> +
> +#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;
> + uint8_t buffer[EC_MEMMAP_SIZE];
> +};
> +
> +#define CROS_EC_DEV_IOC 0xEC
> +#define CROS_EC_DEV_IOCXCMD _IOWR(CROS_EC_DEV_IOC, 0, struct cros_ec_command)
> +#define CROS_EC_DEV_IOCRDMEM _IOWR(CROS_EC_DEV_IOC, 1, struct cros_ec_readmem)
> +
> +#endif /* _CROS_EC_DEV_H_ */
> --
> 2.1.3
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH RESEND v2 1/7] mfd: cros_ec: Use fixed size arrays to transfer data with the EC
2015-01-02 13:32 ` [PATCH RESEND v2 1/7] mfd: cros_ec: Use fixed size arrays to transfer data with the EC Javier Martinez Canillas
@ 2015-01-20 7:48 ` Lee Jones
2015-01-20 15:40 ` Javier Martinez Canillas
0 siblings, 1 reply; 35+ messages in thread
From: Lee Jones @ 2015-01-20 7:48 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Olof Johansson, Doug Anderson, Bill Richardson, Simon Glass,
Gwendal Grignou, Jonathan Corbet, linux-samsung-soc,
linux-kernel
On Fri, 02 Jan 2015, Javier Martinez Canillas wrote:
> The struct cros_ec_command will be used as an ioctl() argument for the
> API to control the ChromeOS EC from user-space. So the data structure
> has to be 64-bit safe to make it compatible between 32 and 64 avoiding
> the need for a compat ioctl interface. Since pointers are self-aligned
> to different byte boundaries, use fixed size arrays instead of pointers
> for transferring ingoing and outgoing data with the Embedded Controller.
>
> Also, re-arrange struct members by decreasing alignment requirements to
> reduce the needing padding size.
>
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> ---
>
> Hello,
>
> I choose EC_PROTO2_MAX_PARAM_SIZE as the maximum length for the input and
> output buffers since I see that is what is assumed in the cros_ec driver
> that is the maximum lengths. But the downstream kernel has also suppport
> for the EC host command protocol v3 even though there is currently no bus
> specific code to handle v3 packets. So I wonder if this is a good max len
> or if a different size should be used instead.
>
> Best regards,
> Javier
>
> Changes since v1: None, new patch
>
> drivers/i2c/busses/i2c-cros-ec-tunnel.c | 51 +++++++--------------------------
> drivers/input/keyboard/cros_ec_keyb.c | 13 +++++----
> drivers/mfd/cros_ec.c | 15 +++++-----
> include/linux/mfd/cros_ec.h | 8 +++---
> 4 files changed, 29 insertions(+), 58 deletions(-)
Looks okay to me, but I'd be happy with some more reviews from the
Chrome guys. I would especially like some knowledgeable type to
answer your EC_PROTO2_MAX_PARAM_SIZE question. If no one does, I
guess it can always be changed later.
Acked-by: Lee Jones <lee.jones@linaro.org>
> diff --git a/drivers/i2c/busses/i2c-cros-ec-tunnel.c b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
> index 875c22a..fa8dedd 100644
> --- a/drivers/i2c/busses/i2c-cros-ec-tunnel.c
> +++ b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
> @@ -182,72 +182,41 @@ static int ec_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg i2c_msgs[],
> const u16 bus_num = bus->remote_bus;
> int request_len;
> int response_len;
> - u8 *request = NULL;
> - u8 *response = NULL;
> int result;
> - struct cros_ec_command msg;
> + struct cros_ec_command msg = { };
>
> request_len = ec_i2c_count_message(i2c_msgs, num);
> if (request_len < 0) {
> dev_warn(dev, "Error constructing message %d\n", request_len);
> - result = request_len;
> - goto exit;
> + return request_len;
> }
> +
> response_len = ec_i2c_count_response(i2c_msgs, num);
> if (response_len < 0) {
> /* Unexpected; no errors should come when NULL response */
> dev_warn(dev, "Error preparing response %d\n", response_len);
> - result = response_len;
> - goto exit;
> - }
> -
> - if (request_len <= ARRAY_SIZE(bus->request_buf)) {
> - request = bus->request_buf;
> - } else {
> - request = kzalloc(request_len, GFP_KERNEL);
> - if (request == NULL) {
> - result = -ENOMEM;
> - goto exit;
> - }
> - }
> - if (response_len <= ARRAY_SIZE(bus->response_buf)) {
> - response = bus->response_buf;
> - } else {
> - response = kzalloc(response_len, GFP_KERNEL);
> - if (response == NULL) {
> - result = -ENOMEM;
> - goto exit;
> - }
> + return response_len;
> }
>
> - result = ec_i2c_construct_message(request, i2c_msgs, num, bus_num);
> + result = ec_i2c_construct_message(msg.outdata, i2c_msgs, num, bus_num);
> if (result)
> - goto exit;
> + return result;
>
> msg.version = 0;
> msg.command = EC_CMD_I2C_PASSTHRU;
> - msg.outdata = request;
> msg.outsize = request_len;
> - msg.indata = response;
> msg.insize = response_len;
>
> result = cros_ec_cmd_xfer(bus->ec, &msg);
> if (result < 0)
> - goto exit;
> + return result;
>
> - result = ec_i2c_parse_response(response, i2c_msgs, &num);
> + result = ec_i2c_parse_response(msg.indata, i2c_msgs, &num);
> if (result < 0)
> - goto exit;
> + return result;
>
> /* Indicate success by saying how many messages were sent */
> - result = num;
> -exit:
> - if (request != bus->request_buf)
> - kfree(request);
> - if (response != bus->response_buf)
> - kfree(response);
> -
> - return result;
> + return num;
> }
>
> static u32 ec_i2c_functionality(struct i2c_adapter *adap)
> diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
> index ffa989f..769f8f7 100644
> --- a/drivers/input/keyboard/cros_ec_keyb.c
> +++ b/drivers/input/keyboard/cros_ec_keyb.c
> @@ -148,16 +148,19 @@ static void cros_ec_keyb_process(struct cros_ec_keyb *ckdev,
>
> static int cros_ec_keyb_get_state(struct cros_ec_keyb *ckdev, uint8_t *kb_state)
> {
> + int ret;
> struct cros_ec_command msg = {
> - .version = 0,
> .command = EC_CMD_MKBP_STATE,
> - .outdata = NULL,
> - .outsize = 0,
> - .indata = kb_state,
> .insize = ckdev->cols,
> };
>
> - return cros_ec_cmd_xfer(ckdev->ec, &msg);
> + ret = cros_ec_cmd_xfer(ckdev->ec, &msg);
> + if (ret < 0)
> + return ret;
> +
> + memcpy(kb_state, msg.indata, ckdev->cols);
> +
> + return 0;
> }
>
> static irqreturn_t cros_ec_keyb_irq(int irq, void *data)
> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> index fc0c81e..c872e1b 100644
> --- a/drivers/mfd/cros_ec.c
> +++ b/drivers/mfd/cros_ec.c
> @@ -74,15 +74,11 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
> ret = ec_dev->cmd_xfer(ec_dev, msg);
> if (msg->result == EC_RES_IN_PROGRESS) {
> int i;
> - struct cros_ec_command status_msg;
> - struct ec_response_get_comms_status status;
> + struct cros_ec_command status_msg = { };
> + struct ec_response_get_comms_status *status;
>
> - status_msg.version = 0;
> status_msg.command = EC_CMD_GET_COMMS_STATUS;
> - status_msg.outdata = NULL;
> - status_msg.outsize = 0;
> - status_msg.indata = (uint8_t *)&status;
> - status_msg.insize = sizeof(status);
> + status_msg.insize = sizeof(*status);
>
> /*
> * Query the EC's status until it's no longer busy or
> @@ -98,7 +94,10 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
> msg->result = status_msg.result;
> if (status_msg.result != EC_RES_SUCCESS)
> break;
> - if (!(status.flags & EC_COMMS_STATUS_PROCESSING))
> +
> + status = (struct ec_response_get_comms_status *)
> + status_msg.indata;
> + if (!(status->flags & EC_COMMS_STATUS_PROCESSING))
> break;
> }
> }
> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> index 0e166b9..71675b1 100644
> --- a/include/linux/mfd/cros_ec.h
> +++ b/include/linux/mfd/cros_ec.h
> @@ -38,20 +38,20 @@ enum {
> /*
> * @version: Command version number (often 0)
> * @command: Command to send (EC_CMD_...)
> - * @outdata: Outgoing data to EC
> * @outsize: Outgoing length in bytes
> - * @indata: Where to put the incoming data from EC
> * @insize: Max number of bytes to accept from EC
> * @result: EC's response to the command (separate from communication failure)
> + * @outdata: Outgoing data to EC
> + * @indata: Where to put the incoming data from EC
> */
> struct cros_ec_command {
> uint32_t version;
> uint32_t command;
> - uint8_t *outdata;
> uint32_t outsize;
> - uint8_t *indata;
> uint32_t insize;
> uint32_t result;
> + uint8_t outdata[EC_PROTO2_MAX_PARAM_SIZE];
> + uint8_t indata[EC_PROTO2_MAX_PARAM_SIZE];
> };
>
> /**
--
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] 35+ messages in thread
* Re: [PATCH RESEND v2 2/7] mfd: cros_ec: Add char dev and virtual dev pointers
2015-01-02 13:32 ` [PATCH RESEND v2 2/7] mfd: cros_ec: Add char dev and virtual dev pointers Javier Martinez Canillas
@ 2015-01-20 7:50 ` Lee Jones
2015-01-20 15:41 ` Javier Martinez Canillas
0 siblings, 1 reply; 35+ messages in thread
From: Lee Jones @ 2015-01-20 7:50 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Olof Johansson, Doug Anderson, Bill Richardson, Simon Glass,
Gwendal Grignou, Jonathan Corbet, linux-samsung-soc,
linux-kernel
On Fri, 02 Jan 2015, Javier Martinez Canillas wrote:
> The ChromeOS Embedded Controller has to be accessed by applications.
> A virtual character device is used as an interface with user-space.
>
> Extend the struct cros_ec_device with the fields needed by the driver
> of this virtual character device.
>
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> ---
>
> Changes since v1: None, new patch.
>
> include/linux/mfd/cros_ec.h | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> index 71675b1..324a346 100644
> --- a/include/linux/mfd/cros_ec.h
> +++ b/include/linux/mfd/cros_ec.h
> @@ -16,6 +16,7 @@
> #ifndef __LINUX_MFD_CROS_EC_H
> #define __LINUX_MFD_CROS_EC_H
>
> +#include <linux/cdev.h>
> #include <linux/notifier.h>
> #include <linux/mfd/cros_ec_commands.h>
> #include <linux/mutex.h>
> @@ -59,9 +60,17 @@ 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
> + * @cdev: Character device structure for virtual comms device
> * @was_wake_device: true if this device was set to wake the system from
> * sleep at the last suspend
> + * @cmd_readmem: 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 +99,12 @@ struct cros_ec_device {
> const char *ec_name;
> const char *phys_name;
> struct device *dev;
> + struct device *vdev;
> + struct cdev cdev;
> bool was_wake_device;
> struct class *cros_class;
> + int (*cmd_readmem)(struct cros_ec_device *ec, unsigned int offset,
> + unsigned int bytes, void *dest);
Is this safe? Are you sure it's okay to provide an interface from
userspace to read (kernel?) memory?
> /* These are used to implement the platform-specific interface */
> void *priv;
--
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] 35+ messages in thread
* Re: [PATCH RESEND v2 3/7] mfd: cros_ec: Add cros_ec_lpc driver for x86 devices
2015-01-02 13:32 ` [PATCH RESEND v2 3/7] mfd: cros_ec: Add cros_ec_lpc driver for x86 devices Javier Martinez Canillas
@ 2015-01-20 8:11 ` Lee Jones
2015-01-20 15:58 ` Javier Martinez Canillas
0 siblings, 1 reply; 35+ messages in thread
From: Lee Jones @ 2015-01-20 8:11 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Olof Johansson, Doug Anderson, Bill Richardson, Simon Glass,
Gwendal Grignou, Jonathan Corbet, linux-samsung-soc,
linux-kernel
On Fri, 02 Jan 2015, Javier Martinez Canillas wrote:
> From: Bill Richardson <wfrichar@chromium.org>
>
> This adds the LPC interface to the Chrome OS EC. Like the
> I2C and SPI drivers, this allows userspace access to the EC.
I'm fairly certain that this is _not_ an MFD device. Please locate it
to the proper subsystem (input?).
> Signed-off-by: Bill Richardson <wfrichar@chromium.org>
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> ---
>
> Changes since v1: None, new patch.
>
> drivers/mfd/Kconfig | 10 ++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/cros_ec_lpc.c | 305 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 316 insertions(+)
> create mode 100644 drivers/mfd/cros_ec_lpc.c
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 2e6b731..7563786 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -110,6 +110,16 @@ config MFD_CROS_EC_I2C
> a checksum. Failing accesses will be retried three times to
> improve reliability.
>
> +config MFD_CROS_EC_LPC
> + tristate "ChromeOS Embedded Controller (LPC)"
> + depends on MFD_CROS_EC
> +
> + help
> + If you say Y here, you get support for talking to the ChromeOS EC
> + over an LPC bus. This uses a simple byte-level protocol with a
> + checksum. This is used for userspace access only. The kernel
> + typically has its own communication methods.
> +
> config MFD_CROS_EC_SPI
> tristate "ChromeOS Embedded Controller (SPI)"
> depends on MFD_CROS_EC && SPI && OF
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 53467e2..94c1516 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_MFD_ASIC3) += asic3.o tmio_core.o
> 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_LPC) += cros_ec_lpc.o
> obj-$(CONFIG_MFD_CROS_EC_SPI) += cros_ec_spi.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_lpc.c b/drivers/mfd/cros_ec_lpc.c
> new file mode 100644
> index 0000000..700e4cf
> --- /dev/null
> +++ b/drivers/mfd/cros_ec_lpc.c
> @@ -0,0 +1,305 @@
> +/*
> + * cros_ec_lpc - LPC access to the Chrome OS Embedded Controller
> + *
> + * Copyright (C) 2012-2015 Google, Inc
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * 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.
> + *
> + * This driver uses the Chrome OS EC byte-level message-based protocol for
> + * communicating the keyboard state (which keys are pressed) from a keyboard EC
> + * to the AP over some bus (such as i2c, lpc, spi). The EC does debouncing,
> + * but everything else (including deghosting) is done here. The main
> + * motivation for this is to keep the EC firmware as simple as possible, since
> + * it cannot be easily upgraded and EC flash/IRAM space is relatively
> + * expensive.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/mfd/cros_ec.h>
> +#include <linux/mfd/cros_ec_commands.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/printk.h>
> +
> +#define MYNAME "cros_ec_lpc"
> +
> +static int ec_response_timed_out(void)
> +{
> + unsigned long one_second = jiffies + HZ;
> +
> + usleep_range(200, 300);
> + do {
> + if (!(inb(EC_LPC_ADDR_HOST_CMD) & EC_LPC_STATUS_BUSY_MASK))
> + return 0;
> + usleep_range(100, 200);
> + } while (time_before(jiffies, one_second));
> +
> + return 1;
> +}
> +
> +static int cros_ec_cmd_xfer_lpc(struct cros_ec_device *ec,
> + struct cros_ec_command *msg)
> +{
> + struct ec_lpc_host_args args;
> + int csum;
> + int i;
> + int ret = 0;
> +
> + if (msg->outsize > EC_PROTO2_MAX_PARAM_SIZE ||
> + msg->insize > EC_PROTO2_MAX_PARAM_SIZE) {
> + dev_err(ec->dev,
> + "invalid buffer sizes (out %d, in %d)\n",
> + msg->outsize, msg->insize);
> + return -EINVAL;
> + }
> +
> + /* Now actually send the command to the EC and get the result */
> + args.flags = EC_HOST_ARGS_FLAG_FROM_HOST;
> + args.command_version = msg->version;
> + args.data_size = msg->outsize;
> +
> + /* Initialize checksum */
> + csum = msg->command + args.flags +
> + args.command_version + args.data_size;
> +
> + /* Copy data and update checksum */
> + for (i = 0; i < msg->outsize; i++) {
> + outb(msg->outdata[i], EC_LPC_ADDR_HOST_PARAM + i);
> + csum += msg->outdata[i];
> + }
> +
> + /* Finalize checksum and write args */
> + args.checksum = csum & 0xFF;
> + outb(args.flags, EC_LPC_ADDR_HOST_ARGS);
> + outb(args.command_version, EC_LPC_ADDR_HOST_ARGS + 1);
> + outb(args.data_size, EC_LPC_ADDR_HOST_ARGS + 2);
> + outb(args.checksum, EC_LPC_ADDR_HOST_ARGS + 3);
> +
> + /* Here we go */
> + outb(msg->command, EC_LPC_ADDR_HOST_CMD);
> +
> + if (ec_response_timed_out()) {
> + dev_warn(ec->dev, "EC responsed timed out\n");
> + ret = -EIO;
> + goto done;
> + }
> +
> + /* Check result */
> + msg->result = inb(EC_LPC_ADDR_HOST_DATA);
> + switch (msg->result) {
> + case EC_RES_SUCCESS:
> + break;
> + case EC_RES_IN_PROGRESS:
> + ret = -EAGAIN;
> + dev_dbg(ec->dev, "command 0x%02x in progress\n",
> + msg->command);
> + goto done;
> + default:
> + dev_dbg(ec->dev, "command 0x%02x returned %d\n",
> + msg->command, msg->result);
> + }
> +
> + /* Read back args */
> + args.flags = inb(EC_LPC_ADDR_HOST_ARGS);
> + args.command_version = inb(EC_LPC_ADDR_HOST_ARGS + 1);
> + args.data_size = inb(EC_LPC_ADDR_HOST_ARGS + 2);
> + args.checksum = inb(EC_LPC_ADDR_HOST_ARGS + 3);
> +
> + if (args.data_size > msg->insize) {
> + dev_err(ec->dev,
> + "packet too long (%d bytes, expected %d)",
> + args.data_size, msg->insize);
> + ret = -ENOSPC;
> + goto done;
> + }
> +
> + /* Start calculating response checksum */
> + csum = msg->command + args.flags +
> + args.command_version + args.data_size;
> +
> + /* Read response and update checksum */
> + for (i = 0; i < args.data_size; i++) {
> + msg->indata[i] = inb(EC_LPC_ADDR_HOST_PARAM + i);
> + csum += msg->indata[i];
> + }
> +
> + /* Verify checksum */
> + if (args.checksum != (csum & 0xFF)) {
> + dev_err(ec->dev,
> + "bad packet checksum, expected %02x, got %02x\n",
> + args.checksum, csum & 0xFF);
> + ret = -EBADMSG;
> + goto done;
> + }
> +
> + /* Return actual amount of data received */
> + ret = args.data_size;
> +done:
> + return ret;
> +}
> +
> +/* Returns num bytes read, or negative on error. Doesn't need locking. */
> +static int cros_ec_lpc_readmem(struct cros_ec_device *ec, unsigned int offset,
> + unsigned int bytes, void *dest)
> +{
> + int i = offset;
> + char *s = dest;
> + int cnt = 0;
> +
> + if (offset >= EC_MEMMAP_SIZE - bytes)
> + return -EINVAL;
> +
> + /* fixed length */
> + if (bytes) {
> + for (; cnt < bytes; i++, s++, cnt++)
> + *s = inb(EC_LPC_ADDR_MEMMAP + i);
> + return cnt;
> + }
> +
> + /* string */
> + for (; i < EC_MEMMAP_SIZE; i++, s++) {
> + *s = inb(EC_LPC_ADDR_MEMMAP + i);
> + cnt++;
> + if (!*s)
> + break;
> + }
> +
> + return cnt;
> +}
> +
> +static int cros_ec_lpc_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct cros_ec_device *ec_dev;
> + int err = -ENOTTY;
> +
> + if (!request_region(EC_LPC_ADDR_MEMMAP, EC_MEMMAP_SIZE, MYNAME)) {
> + dev_warn(dev, "couldn't reserve memmap region\n");
> + goto failed_memmap;
> + }
> +
> + if ((inb(EC_LPC_ADDR_MEMMAP + EC_MEMMAP_ID) != 'E') ||
> + (inb(EC_LPC_ADDR_MEMMAP + EC_MEMMAP_ID + 1) != 'C')) {
> + dev_warn(dev, "EC ID not detected\n");
> + goto failed_ec_probe;
> + }
> +
> + if (!request_region(EC_HOST_CMD_REGION0, EC_HOST_CMD_REGION_SIZE,
> + MYNAME)) {
> + dev_warn(dev, "couldn't reserve region0\n");
> + goto failed_region0;
> + }
> + if (!request_region(EC_HOST_CMD_REGION1, EC_HOST_CMD_REGION_SIZE,
> + MYNAME)) {
> + dev_warn(dev, "couldn't reserve region1\n");
> + goto failed_region1;
> + }
> +
> + ec_dev = devm_kzalloc(dev, sizeof(*ec_dev), GFP_KERNEL);
> + if (!ec_dev) {
> + err = -ENOMEM;
> + goto failed_ec_dev;
> + }
> +
> + platform_set_drvdata(pdev, ec_dev);
> + ec_dev->dev = dev;
> + ec_dev->ec_name = pdev->name;
> + ec_dev->phys_name = dev_name(dev);
> + ec_dev->parent = dev;
> + ec_dev->cmd_xfer = cros_ec_cmd_xfer_lpc;
> + ec_dev->cmd_readmem = cros_ec_lpc_readmem;
> +
> + err = cros_ec_register(ec_dev);
> + if (err) {
> + dev_warn(dev, "couldn't register ec_dev\n");
> + goto failed_ec_dev;
> + }
> +
> + return 0;
> +
> +failed_ec_dev:
> + release_region(EC_HOST_CMD_REGION1, EC_HOST_CMD_REGION_SIZE);
> +failed_region1:
> + release_region(EC_HOST_CMD_REGION0, EC_HOST_CMD_REGION_SIZE);
> +failed_region0:
> +failed_ec_probe:
> + release_region(EC_LPC_ADDR_MEMMAP, EC_MEMMAP_SIZE);
> +failed_memmap:
> + return err;
> +}
> +
> +static int cros_ec_lpc_remove(struct platform_device *pdev)
> +{
> + struct cros_ec_device *ec_dev;
> +
> + ec_dev = platform_get_drvdata(pdev);
> + cros_ec_remove(ec_dev);
> +
> + release_region(EC_HOST_CMD_REGION1, EC_HOST_CMD_REGION_SIZE);
> + release_region(EC_HOST_CMD_REGION0, EC_HOST_CMD_REGION_SIZE);
> + release_region(EC_LPC_ADDR_MEMMAP, EC_MEMMAP_SIZE);
> +
> + return 0;
> +}
> +
> +static struct platform_driver cros_ec_lpc_driver = {
> + .driver = {
> + .name = MYNAME,
> + .owner = THIS_MODULE,
> + },
> + .probe = cros_ec_lpc_probe,
> + .remove = cros_ec_lpc_remove,
> +};
> +
> +static void do_nothing(struct device *dev)
> +{
> + /* not a physical device */
> +}
> +
> +static struct platform_device cros_ec_lpc_device = {
> + .name = MYNAME,
> + .dev = {
> + .release = do_nothing,
> + },
> +};
> +
> +static int __init cros_ec_lpc_init(void)
> +{
> + int ret;
> +
> + /* Register the driver */
> + ret = platform_driver_register(&cros_ec_lpc_driver);
> + if (ret < 0) {
> + pr_warn(MYNAME ": can't register driver: %d\n", ret);
> + return ret;
> + }
> +
> + /* Register the device, and it'll get hooked up automatically */
> + ret = platform_device_register(&cros_ec_lpc_device);
> + if (ret < 0) {
> + pr_warn(MYNAME ": can't register device: %d\n", ret);
> + platform_driver_unregister(&cros_ec_lpc_driver);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static void __exit cros_ec_lpc_exit(void)
> +{
> + platform_device_unregister(&cros_ec_lpc_device);
> + platform_driver_unregister(&cros_ec_lpc_driver);
> +}
> +
> +module_init(cros_ec_lpc_init);
> +module_exit(cros_ec_lpc_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("ChromeOS EC LPC driver");
--
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] 35+ messages in thread
* Re: [PATCH RESEND v2 5/7] mfd: cros_ec: Instantiate ChromeOS EC character device
2015-01-02 13:32 ` [PATCH RESEND v2 5/7] mfd: cros_ec: Instantiate ChromeOS EC character device Javier Martinez Canillas
@ 2015-01-20 8:20 ` Lee Jones
2015-01-20 16:03 ` Javier Martinez Canillas
0 siblings, 1 reply; 35+ messages in thread
From: Lee Jones @ 2015-01-20 8:20 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Olof Johansson, Doug Anderson, Bill Richardson, Simon Glass,
Gwendal Grignou, Jonathan Corbet, linux-samsung-soc,
linux-kernel
On Fri, 02 Jan 2015, Javier Martinez Canillas wrote:
> The ChromeOS EC character device is an user-space interface to
> allow applications to access the Embedded Controller.
>
> Add a cell for this device so it's spawned from the mfd driver.
>
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> ---
>
> Changes since v1: None, new patch.
>
> drivers/mfd/cros_ec.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> index c872e1b..70f9ed5 100644
> --- a/drivers/mfd/cros_ec.c
> +++ b/drivers/mfd/cros_ec.c
> @@ -118,6 +118,10 @@ static const struct mfd_cell cros_devs[] = {
> .id = 2,
> .of_compatible = "google,cros-ec-i2c-tunnel",
> },
> + {
> + .name = "cros-ec-dev",
*-dev is seldom suitable for naming things.
Please be more imaginative/descriptive in your naming.
> + .id = 3,
> + },
> };
>
> int cros_ec_register(struct cros_ec_device *ec_dev)
--
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] 35+ messages in thread
* Re: [PATCH RESEND v2 1/7] mfd: cros_ec: Use fixed size arrays to transfer data with the EC
2015-01-20 7:48 ` Lee Jones
@ 2015-01-20 15:40 ` Javier Martinez Canillas
0 siblings, 0 replies; 35+ messages in thread
From: Javier Martinez Canillas @ 2015-01-20 15:40 UTC (permalink / raw)
To: Lee Jones
Cc: Olof Johansson, Doug Anderson, Bill Richardson, Simon Glass,
Gwendal Grignou, Jonathan Corbet, linux-samsung-soc,
linux-kernel
Hello Lee,
On 01/20/2015 08:48 AM, Lee Jones wrote:
>
> Looks okay to me, but I'd be happy with some more reviews from the
> Chrome guys. I would especially like some knowledgeable type to
> answer your EC_PROTO2_MAX_PARAM_SIZE question. If no one does, I
> guess it can always be changed later.
>
> Acked-by: Lee Jones <lee.jones@linaro.org>
>
Great, thanks a lot.
Best regards,
Javier
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH RESEND v2 2/7] mfd: cros_ec: Add char dev and virtual dev pointers
2015-01-20 7:50 ` Lee Jones
@ 2015-01-20 15:41 ` Javier Martinez Canillas
2015-01-20 16:36 ` Lee Jones
0 siblings, 1 reply; 35+ messages in thread
From: Javier Martinez Canillas @ 2015-01-20 15:41 UTC (permalink / raw)
To: Lee Jones
Cc: Olof Johansson, Doug Anderson, Bill Richardson, Simon Glass,
Gwendal Grignou, Jonathan Corbet, linux-samsung-soc,
linux-kernel
Hello Lee,
Thanks a lot for your feedback.
On 01/20/2015 08:50 AM, Lee Jones wrote:
>> @@ -59,9 +60,17 @@ 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
>> + * @cdev: Character device structure for virtual comms device
>> * @was_wake_device: true if this device was set to wake the system from
>> * sleep at the last suspend
>> + * @cmd_readmem: 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 +99,12 @@ struct cros_ec_device {
>> const char *ec_name;
>> const char *phys_name;
>> struct device *dev;
>> + struct device *vdev;
>> + struct cdev cdev;
>> bool was_wake_device;
>> struct class *cros_class;
>> + int (*cmd_readmem)(struct cros_ec_device *ec, unsigned int offset,
>> + unsigned int bytes, void *dest);
>
> Is this safe? Are you sure it's okay to provide an interface from
> userspace to read (kernel?) memory?
>
This interface is not to read any kernel memory but only the memory mapped
I/O region for the Low Pin Count (LPC) bus. So user-space only can choose
and offset and a number of bytes using the CROS_EC_DEV_IOCRDMEM ioctl cmd
which uses the following structure as argument:
/*
* @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;
uint8_t buffer[EC_MEMMAP_SIZE];
};
The cros_ec_lpc_readmem() handler that the function pointer is set only
reads bytes from EC_LPC_ADDR_MEMMAP + offset if offset < EC_MEMMAP_SIZE
and the data is copied to the user-space buffer from the structure passed
as argument with copy_to_user().
So in that sense is similar to the spidev or i2c-dev interfaces that are
used to access these buses from user-space.
Best regards,
Javier
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH RESEND v2 3/7] mfd: cros_ec: Add cros_ec_lpc driver for x86 devices
2015-01-20 8:11 ` Lee Jones
@ 2015-01-20 15:58 ` Javier Martinez Canillas
2015-01-20 16:34 ` Lee Jones
0 siblings, 1 reply; 35+ messages in thread
From: Javier Martinez Canillas @ 2015-01-20 15:58 UTC (permalink / raw)
To: Lee Jones
Cc: Olof Johansson, Doug Anderson, Bill Richardson, Simon Glass,
Gwendal Grignou, Jonathan Corbet, linux-samsung-soc,
linux-kernel
Hello Lee,
On 01/20/2015 09:11 AM, Lee Jones wrote:
> On Fri, 02 Jan 2015, Javier Martinez Canillas wrote:
>
>> From: Bill Richardson <wfrichar@chromium.org>
>>
>> This adds the LPC interface to the Chrome OS EC. Like the
>> I2C and SPI drivers, this allows userspace access to the EC.
>
> I'm fairly certain that this is _not_ an MFD device. Please locate it
> to the proper subsystem (input?).
>
Sorry, it wasn't my intention to use the mfd subsystem as a place to dump
random drivers. Is that I still find hard to understand what is the line
between what falls under mfd and what doesn't.
For example, I see that mfd drivers are for devices which have multiple
functions and the mfd driver is the one that spawns the platform devices
and provide an interface to access the I/O registers used by the different
platform drivers of the sub-devices.
So, the Embedded Controller driver (drivers/mfd/cros_ec.c) falls into that
category and in fact has been in the mfd driver for a long time. Now, if
an mfd device support different type of buses (e.g: i2c, spi, etc) I see
that both the core driver and the driver for the transport method are
in the drivers/mfd directory. As an example:
drivers/mfd/arizona-{core,i2c,spi}.c
drivers/mfd/da9052-{core,i2c,spi}.c
drivers/mfd/mc13xxx-{core,i2c,spi}.c
drivers/mfd/tps65912-{core,i2c,spi}.c
drivers/mfd/wm831x-{core,i2c,spi,otp}.c
In the cros_ec case, we already have drivers/mfd/cros_ec_{i2c,spi}.c so
since the Low Pin Count is another transport method I thought that this
driver belonged to the drivers/mfd directory.
Now, all those drivers may be wrong and the buses don't belong to the mfd
subsystem but then I think we need to document that since it seems that is
the correct way to do it just by looking at the other drivers.
Best regards,
Javier
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH RESEND v2 5/7] mfd: cros_ec: Instantiate ChromeOS EC character device
2015-01-20 8:20 ` Lee Jones
@ 2015-01-20 16:03 ` Javier Martinez Canillas
2015-01-20 16:29 ` Lee Jones
0 siblings, 1 reply; 35+ messages in thread
From: Javier Martinez Canillas @ 2015-01-20 16:03 UTC (permalink / raw)
To: Lee Jones
Cc: Olof Johansson, Doug Anderson, Bill Richardson, Simon Glass,
Gwendal Grignou, Jonathan Corbet, linux-samsung-soc,
linux-kernel
Hello Lee,
Thanks a lot for your feedback.
On 01/20/2015 09:20 AM, Lee Jones wrote:
> On Fri, 02 Jan 2015, Javier Martinez Canillas wrote:
>
>> The ChromeOS EC character device is an user-space interface to
>> allow applications to access the Embedded Controller.
>>
>> Add a cell for this device so it's spawned from the mfd driver.
>>
>> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
>> ---
>>
>> Changes since v1: None, new patch.
>>
>> drivers/mfd/cros_ec.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
>> index c872e1b..70f9ed5 100644
>> --- a/drivers/mfd/cros_ec.c
>> +++ b/drivers/mfd/cros_ec.c
>> @@ -118,6 +118,10 @@ static const struct mfd_cell cros_devs[] = {
>> .id = 2,
>> .of_compatible = "google,cros-ec-i2c-tunnel",
>> },
>> + {
>> + .name = "cros-ec-dev",
>
> *-dev is seldom suitable for naming things.
>
> Please be more imaginative/descriptive in your naming.
>
You know this is one of the two hard problems in CS ;-)
But seriously, this is the name used in the downstream ChromiumsOS kernel
and tbh I don't think is that bad since after all is a character device
interface to access the Embedded Controller from user-space. So in that
sense is not worse than the i2c-dev or spidev names but I can try to come
up with something more creative if that will block the patch to be merged.
Best regards,
Javier
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH RESEND v2 5/7] mfd: cros_ec: Instantiate ChromeOS EC character device
2015-01-20 16:03 ` Javier Martinez Canillas
@ 2015-01-20 16:29 ` Lee Jones
2015-01-20 16:36 ` Javier Martinez Canillas
0 siblings, 1 reply; 35+ messages in thread
From: Lee Jones @ 2015-01-20 16:29 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Olof Johansson, Doug Anderson, Bill Richardson, Simon Glass,
Gwendal Grignou, Jonathan Corbet, linux-samsung-soc,
linux-kernel
On Tue, 20 Jan 2015, Javier Martinez Canillas wrote:
> Hello Lee,
>
> Thanks a lot for your feedback.
>
> On 01/20/2015 09:20 AM, Lee Jones wrote:
> > On Fri, 02 Jan 2015, Javier Martinez Canillas wrote:
> >
> >> The ChromeOS EC character device is an user-space interface to
> >> allow applications to access the Embedded Controller.
> >>
> >> Add a cell for this device so it's spawned from the mfd driver.
> >>
> >> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> >> ---
> >>
> >> Changes since v1: None, new patch.
> >>
> >> drivers/mfd/cros_ec.c | 4 ++++
> >> 1 file changed, 4 insertions(+)
> >>
> >> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> >> index c872e1b..70f9ed5 100644
> >> --- a/drivers/mfd/cros_ec.c
> >> +++ b/drivers/mfd/cros_ec.c
> >> @@ -118,6 +118,10 @@ static const struct mfd_cell cros_devs[] = {
> >> .id = 2,
> >> .of_compatible = "google,cros-ec-i2c-tunnel",
> >> },
> >> + {
> >> + .name = "cros-ec-dev",
> >
> > *-dev is seldom suitable for naming things.
> >
> > Please be more imaginative/descriptive in your naming.
> >
>
> You know this is one of the two hard problems in CS ;-)
>
> But seriously, this is the name used in the downstream ChromiumsOS kernel
> and tbh I don't think is that bad since after all is a character device
> interface to access the Embedded Controller from user-space. So in that
> sense is not worse than the i2c-dev or spidev names but I can try to come
> up with something more creative if that will block the patch to be merged.
It's not a blocker, but it is a ridiculous name to use inside the
driver/ directory 'cos almost everything is a dev(ice) here.
--
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] 35+ messages in thread
* Re: [PATCH RESEND v2 3/7] mfd: cros_ec: Add cros_ec_lpc driver for x86 devices
2015-01-20 15:58 ` Javier Martinez Canillas
@ 2015-01-20 16:34 ` Lee Jones
2015-01-20 16:52 ` Javier Martinez Canillas
0 siblings, 1 reply; 35+ messages in thread
From: Lee Jones @ 2015-01-20 16:34 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Olof Johansson, Doug Anderson, Bill Richardson, Simon Glass,
Gwendal Grignou, Jonathan Corbet, linux-samsung-soc,
linux-kernel
On Tue, 20 Jan 2015, Javier Martinez Canillas wrote:
> Hello Lee,
>
> On 01/20/2015 09:11 AM, Lee Jones wrote:
> > On Fri, 02 Jan 2015, Javier Martinez Canillas wrote:
> >
> >> From: Bill Richardson <wfrichar@chromium.org>
> >>
> >> This adds the LPC interface to the Chrome OS EC. Like the
> >> I2C and SPI drivers, this allows userspace access to the EC.
> >
> > I'm fairly certain that this is _not_ an MFD device. Please locate it
> > to the proper subsystem (input?).
> >
>
> Sorry, it wasn't my intention to use the mfd subsystem as a place to dump
> random drivers. Is that I still find hard to understand what is the line
> between what falls under mfd and what doesn't.
>
> For example, I see that mfd drivers are for devices which have multiple
> functions and the mfd driver is the one that spawns the platform devices
> and provide an interface to access the I/O registers used by the different
> platform drivers of the sub-devices.
>
> So, the Embedded Controller driver (drivers/mfd/cros_ec.c) falls into that
> category and in fact has been in the mfd driver for a long time. Now, if
> an mfd device support different type of buses (e.g: i2c, spi, etc) I see
> that both the core driver and the driver for the transport method are
> in the drivers/mfd directory. As an example:
>
> drivers/mfd/arizona-{core,i2c,spi}.c
> drivers/mfd/da9052-{core,i2c,spi}.c
> drivers/mfd/mc13xxx-{core,i2c,spi}.c
> drivers/mfd/tps65912-{core,i2c,spi}.c
> drivers/mfd/wm831x-{core,i2c,spi,otp}.c
>
> In the cros_ec case, we already have drivers/mfd/cros_ec_{i2c,spi}.c so
> since the Low Pin Count is another transport method I thought that this
> driver belonged to the drivers/mfd directory.
>
> Now, all those drivers may be wrong and the buses don't belong to the mfd
> subsystem but then I think we need to document that since it seems that is
> the correct way to do it just by looking at the other drivers.
I don't think the drivers you mentioned above do anything practical.
For instance, they are not SPI/IC2/etc drivers. They should only
offer some abstraction layers which are used to communicate with the
device. The driver you are submitting looks a lot more like a device
driver, which should live somewhere else. Don't ask me where though,
I'm not even sure what a Low Pin Controller does.
--
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] 35+ messages in thread
* Re: [PATCH RESEND v2 5/7] mfd: cros_ec: Instantiate ChromeOS EC character device
2015-01-20 16:29 ` Lee Jones
@ 2015-01-20 16:36 ` Javier Martinez Canillas
2015-01-20 16:55 ` Lee Jones
0 siblings, 1 reply; 35+ messages in thread
From: Javier Martinez Canillas @ 2015-01-20 16:36 UTC (permalink / raw)
To: Lee Jones
Cc: Olof Johansson, Doug Anderson, Bill Richardson, Simon Glass,
Gwendal Grignou, Jonathan Corbet, linux-samsung-soc,
linux-kernel
Hello Lee,
On 01/20/2015 05:29 PM, Lee Jones wrote:
>
> It's not a blocker, but it is a ridiculous name to use inside the
> driver/ directory 'cos almost everything is a dev(ice) here.
>
Right, do you think that "cros-ec-chardev" will be a more suitable
name? Sorry, I'm really bad at naming things...
Best regards,
Javier
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH RESEND v2 2/7] mfd: cros_ec: Add char dev and virtual dev pointers
2015-01-20 15:41 ` Javier Martinez Canillas
@ 2015-01-20 16:36 ` Lee Jones
2015-01-20 16:45 ` Javier Martinez Canillas
0 siblings, 1 reply; 35+ messages in thread
From: Lee Jones @ 2015-01-20 16:36 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Olof Johansson, Doug Anderson, Bill Richardson, Simon Glass,
Gwendal Grignou, Jonathan Corbet, linux-samsung-soc,
linux-kernel
On Tue, 20 Jan 2015, Javier Martinez Canillas wrote:
> Hello Lee,
>
> Thanks a lot for your feedback.
>
> On 01/20/2015 08:50 AM, Lee Jones wrote:
> >> @@ -59,9 +60,17 @@ 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
> >> + * @cdev: Character device structure for virtual comms device
> >> * @was_wake_device: true if this device was set to wake the system from
> >> * sleep at the last suspend
> >> + * @cmd_readmem: 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 +99,12 @@ struct cros_ec_device {
> >> const char *ec_name;
> >> const char *phys_name;
> >> struct device *dev;
> >> + struct device *vdev;
> >> + struct cdev cdev;
> >> bool was_wake_device;
> >> struct class *cros_class;
> >> + int (*cmd_readmem)(struct cros_ec_device *ec, unsigned int offset,
> >> + unsigned int bytes, void *dest);
> >
> > Is this safe? Are you sure it's okay to provide an interface from
> > userspace to read (kernel?) memory?
> >
>
> This interface is not to read any kernel memory but only the memory mapped
> I/O region for the Low Pin Count (LPC) bus. So user-space only can choose
> and offset and a number of bytes using the CROS_EC_DEV_IOCRDMEM ioctl cmd
> which uses the following structure as argument:
>
> /*
> * @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;
> uint8_t buffer[EC_MEMMAP_SIZE];
> };
>
> The cros_ec_lpc_readmem() handler that the function pointer is set only
> reads bytes from EC_LPC_ADDR_MEMMAP + offset if offset < EC_MEMMAP_SIZE
> and the data is copied to the user-space buffer from the structure passed
> as argument with copy_to_user().
>
> So in that sense is similar to the spidev or i2c-dev interfaces that are
> used to access these buses from user-space.
Very well. The purpose of my question was to be provocative and to
make you think about the interface. As long as you're sure it can't
be abused, then I'm happy.
--
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] 35+ messages in thread
* Re: [PATCH RESEND v2 2/7] mfd: cros_ec: Add char dev and virtual dev pointers
2015-01-20 16:36 ` Lee Jones
@ 2015-01-20 16:45 ` Javier Martinez Canillas
2015-01-20 16:51 ` Lee Jones
0 siblings, 1 reply; 35+ messages in thread
From: Javier Martinez Canillas @ 2015-01-20 16:45 UTC (permalink / raw)
To: Lee Jones
Cc: Olof Johansson, Doug Anderson, Bill Richardson, Simon Glass,
Gwendal Grignou, Jonathan Corbet, linux-samsung-soc,
linux-kernel
Hello Lee,
On 01/20/2015 05:36 PM, Lee Jones wrote:
>> >
>> > Is this safe? Are you sure it's okay to provide an interface from
>> > userspace to read (kernel?) memory?
>> >
>>
>> This interface is not to read any kernel memory but only the memory mapped
>> I/O region for the Low Pin Count (LPC) bus. So user-space only can choose
>> and offset and a number of bytes using the CROS_EC_DEV_IOCRDMEM ioctl cmd
>> which uses the following structure as argument:
>>
>> /*
>> * @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;
>> uint8_t buffer[EC_MEMMAP_SIZE];
>> };
>>
>> The cros_ec_lpc_readmem() handler that the function pointer is set only
>> reads bytes from EC_LPC_ADDR_MEMMAP + offset if offset < EC_MEMMAP_SIZE
>> and the data is copied to the user-space buffer from the structure passed
>> as argument with copy_to_user().
>>
>> So in that sense is similar to the spidev or i2c-dev interfaces that are
>> used to access these buses from user-space.
>
> Very well. The purpose of my question was to be provocative and to
> make you think about the interface. As long as you're sure it can't
> be abused, then I'm happy.
>
Ok, thanks a lot. Does it mean I've your Acked-by for this patch too?
Best regards,
Javier
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH RESEND v2 2/7] mfd: cros_ec: Add char dev and virtual dev pointers
2015-01-20 16:45 ` Javier Martinez Canillas
@ 2015-01-20 16:51 ` Lee Jones
0 siblings, 0 replies; 35+ messages in thread
From: Lee Jones @ 2015-01-20 16:51 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Olof Johansson, Doug Anderson, Bill Richardson, Simon Glass,
Gwendal Grignou, Jonathan Corbet, linux-samsung-soc,
linux-kernel
On Tue, 20 Jan 2015, Javier Martinez Canillas wrote:
> Hello Lee,
>
> On 01/20/2015 05:36 PM, Lee Jones wrote:
> >> >
> >> > Is this safe? Are you sure it's okay to provide an interface from
> >> > userspace to read (kernel?) memory?
> >> >
> >>
> >> This interface is not to read any kernel memory but only the memory mapped
> >> I/O region for the Low Pin Count (LPC) bus. So user-space only can choose
> >> and offset and a number of bytes using the CROS_EC_DEV_IOCRDMEM ioctl cmd
> >> which uses the following structure as argument:
> >>
> >> /*
> >> * @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;
> >> uint8_t buffer[EC_MEMMAP_SIZE];
> >> };
> >>
> >> The cros_ec_lpc_readmem() handler that the function pointer is set only
> >> reads bytes from EC_LPC_ADDR_MEMMAP + offset if offset < EC_MEMMAP_SIZE
> >> and the data is copied to the user-space buffer from the structure passed
> >> as argument with copy_to_user().
> >>
> >> So in that sense is similar to the spidev or i2c-dev interfaces that are
> >> used to access these buses from user-space.
> >
> > Very well. The purpose of my question was to be provocative and to
> > make you think about the interface. As long as you're sure it can't
> > be abused, then I'm happy.
> >
>
> Ok, thanks a lot. Does it mean I've your Acked-by for this patch too?
Yes: Acked-by: Lee Jones <lee.jones@linaro.org>
--
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] 35+ messages in thread
* Re: [PATCH RESEND v2 3/7] mfd: cros_ec: Add cros_ec_lpc driver for x86 devices
2015-01-20 16:34 ` Lee Jones
@ 2015-01-20 16:52 ` Javier Martinez Canillas
2015-01-21 17:05 ` Javier Martinez Canillas
2015-01-22 8:42 ` Lee Jones
0 siblings, 2 replies; 35+ messages in thread
From: Javier Martinez Canillas @ 2015-01-20 16:52 UTC (permalink / raw)
To: Lee Jones
Cc: Olof Johansson, Doug Anderson, Bill Richardson, Simon Glass,
Gwendal Grignou, Jonathan Corbet, linux-samsung-soc,
linux-kernel
Hello Lee,
On 01/20/2015 05:34 PM, Lee Jones wrote:
>>
>> So, the Embedded Controller driver (drivers/mfd/cros_ec.c) falls into that
>> category and in fact has been in the mfd driver for a long time. Now, if
>> an mfd device support different type of buses (e.g: i2c, spi, etc) I see
>> that both the core driver and the driver for the transport method are
>> in the drivers/mfd directory. As an example:
>>
>> drivers/mfd/arizona-{core,i2c,spi}.c
>> drivers/mfd/da9052-{core,i2c,spi}.c
>> drivers/mfd/mc13xxx-{core,i2c,spi}.c
>> drivers/mfd/tps65912-{core,i2c,spi}.c
>> drivers/mfd/wm831x-{core,i2c,spi,otp}.c
>>
>> In the cros_ec case, we already have drivers/mfd/cros_ec_{i2c,spi}.c so
>> since the Low Pin Count is another transport method I thought that this
>> driver belonged to the drivers/mfd directory.
>>
>> Now, all those drivers may be wrong and the buses don't belong to the mfd
>> subsystem but then I think we need to document that since it seems that is
>> the correct way to do it just by looking at the other drivers.
>
> I don't think the drivers you mentioned above do anything practical.
> For instance, they are not SPI/IC2/etc drivers. They should only
> offer some abstraction layers which are used to communicate with the
> device. The driver you are submitting looks a lot more like a device
> driver, which should live somewhere else. Don't ask me where though,
> I'm not even sure what a Low Pin Controller does.
>
The driver added by $subject doesn't really do anything practical either.
LPC [0] is just another transport method like i2c or spi that is used on
x86 Chromebooks to access the Embedded Controller.
So the driver is really not that different than the cros_ec_{i2c,spi}.c
drivers.
Best regards,
Javier
[0]: http://en.wikipedia.org/wiki/Low_Pin_Count
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH RESEND v2 5/7] mfd: cros_ec: Instantiate ChromeOS EC character device
2015-01-20 16:36 ` Javier Martinez Canillas
@ 2015-01-20 16:55 ` Lee Jones
2015-01-20 17:11 ` Javier Martinez Canillas
0 siblings, 1 reply; 35+ messages in thread
From: Lee Jones @ 2015-01-20 16:55 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Olof Johansson, Doug Anderson, Bill Richardson, Simon Glass,
Gwendal Grignou, Jonathan Corbet, linux-samsung-soc,
linux-kernel
On Tue, 20 Jan 2015, Javier Martinez Canillas wrote:
> Hello Lee,
>
> On 01/20/2015 05:29 PM, Lee Jones wrote:
> >
> > It's not a blocker, but it is a ridiculous name to use inside the
> > driver/ directory 'cos almost everything is a dev(ice) here.
> >
>
> Right, do you think that "cros-ec-chardev" will be a more suitable
> name? Sorry, I'm really bad at naming things...
But is it really a chardev? Don't chardevs usually live in
drivers/char? It probably uses a chardev node in /dev, but what does
it really do? What information can/will userspace obtain from this
memory block?
--
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] 35+ messages in thread
* Re: [PATCH RESEND v2 5/7] mfd: cros_ec: Instantiate ChromeOS EC character device
2015-01-20 16:55 ` Lee Jones
@ 2015-01-20 17:11 ` Javier Martinez Canillas
2015-01-21 16:56 ` Javier Martinez Canillas
0 siblings, 1 reply; 35+ messages in thread
From: Javier Martinez Canillas @ 2015-01-20 17:11 UTC (permalink / raw)
To: Lee Jones
Cc: Olof Johansson, Doug Anderson, Bill Richardson, Simon Glass,
Gwendal Grignou, Jonathan Corbet, linux-samsung-soc,
linux-kernel
Hello Lee,
On 01/20/2015 05:55 PM, Lee Jones wrote:
> On Tue, 20 Jan 2015, Javier Martinez Canillas wrote:
>
>> Hello Lee,
>>
>> On 01/20/2015 05:29 PM, Lee Jones wrote:
>> >
>> > It's not a blocker, but it is a ridiculous name to use inside the
>> > driver/ directory 'cos almost everything is a dev(ice) here.
>> >
>>
>> Right, do you think that "cros-ec-chardev" will be a more suitable
>> name? Sorry, I'm really bad at naming things...
>
> But is it really a chardev? Don't chardevs usually live in
> drivers/char? It probably uses a chardev node in /dev, but what does
> it really do? What information can/will userspace obtain from this
> memory block?
>
Right, is a driver that register a chardev but mostly to expose an ioctl
interface to send commands to the Embedded Controller from user-space.
The Application Processor communicates with Embedded Controller by sending
commands over an interface. This can be either spi or i2c on ARM (depending
on the Chromebook model) or LPC on x86 Chromebooks so the platform driver
instantiated by the "cros-ec-dev" mfd cell is to allow user-space to send
commands to the Embedded Controller (using the correct transport method).
So this chardev is used by the ectool binary in ChromeOS to communicate
with the Embedded Controller.
The actual driver is located in drivers/platform/chrome/cros_ec_dev.c and
is added by "platform/chrome: Add Chrome OS EC userspace device interface"
[0] in this series.
In the downstream ChromiumOS kernel tree it is located in drivers/mfd but
I moved to drivers/platform/chrome/ since you asked me to find a better
place for it.
Best regards,
Javier
[0]: https://lkml.org/lkml/2015/1/2/81
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH RESEND v2 5/7] mfd: cros_ec: Instantiate ChromeOS EC character device
2015-01-20 17:11 ` Javier Martinez Canillas
@ 2015-01-21 16:56 ` Javier Martinez Canillas
0 siblings, 0 replies; 35+ messages in thread
From: Javier Martinez Canillas @ 2015-01-21 16:56 UTC (permalink / raw)
To: Lee Jones
Cc: Olof Johansson, Doug Anderson, Bill Richardson, Simon Glass,
Gwendal Grignou, Jonathan Corbet, linux-samsung-soc,
linux-kernel
Hello Lee,
On 01/20/2015 06:11 PM, Javier Martinez Canillas wrote:
>>
>> But is it really a chardev? Don't chardevs usually live in
>> drivers/char? It probably uses a chardev node in /dev, but what does
>> it really do? What information can/will userspace obtain from this
>> memory block?
>>
>
> Right, is a driver that register a chardev but mostly to expose an ioctl
> interface to send commands to the Embedded Controller from user-space.
>
> The Application Processor communicates with Embedded Controller by sending
> commands over an interface. This can be either spi or i2c on ARM (depending
> on the Chromebook model) or LPC on x86 Chromebooks so the platform driver
> instantiated by the "cros-ec-dev" mfd cell is to allow user-space to send
> commands to the Embedded Controller (using the correct transport method).
>
> So this chardev is used by the ectool binary in ChromeOS to communicate
> with the Embedded Controller.
>
Just FYI, I'll rename it to "cros-ec-ctl" since as you said is not really
a chardev but that just happens to be the interface chosen to send the ioctl
commands to the driver.
Thanks a lot for your suggestion.
Best regards,
Javier
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH RESEND v2 3/7] mfd: cros_ec: Add cros_ec_lpc driver for x86 devices
2015-01-20 16:52 ` Javier Martinez Canillas
@ 2015-01-21 17:05 ` Javier Martinez Canillas
2015-01-22 8:42 ` Lee Jones
1 sibling, 0 replies; 35+ messages in thread
From: Javier Martinez Canillas @ 2015-01-21 17:05 UTC (permalink / raw)
To: Lee Jones
Cc: Olof Johansson, Doug Anderson, Bill Richardson, Simon Glass,
Gwendal Grignou, Jonathan Corbet, linux-samsung-soc,
linux-kernel
Hello Lee,
On 01/20/2015 05:52 PM, Javier Martinez Canillas wrote:
>>>
>>> Now, all those drivers may be wrong and the buses don't belong to the mfd
>>> subsystem but then I think we need to document that since it seems that is
>>> the correct way to do it just by looking at the other drivers.
>>
>> I don't think the drivers you mentioned above do anything practical.
>> For instance, they are not SPI/IC2/etc drivers. They should only
>> offer some abstraction layers which are used to communicate with the
>> device. The driver you are submitting looks a lot more like a device
>> driver, which should live somewhere else. Don't ask me where though,
>> I'm not even sure what a Low Pin Controller does.
>>
>
> The driver added by $subject doesn't really do anything practical either.
> LPC [0] is just another transport method like i2c or spi that is used on
> x86 Chromebooks to access the Embedded Controller.
>
> So the driver is really not that different than the cros_ec_{i2c,spi}.c
> drivers.
>
> Best regards,
> Javier
>
> [0]: http://en.wikipedia.org/wiki/Low_Pin_Count
>
Maybe the problem is that the commit message didn't explain this clearly?
I took the patch from the ChromiumOS tree verbatim but I can reword the
commit message to add something like this instead:
Author: Bill Richardson <wfrichar@chromium.org>
Date: Tue Dec 23 15:13:23 2014 +0100
mfd: cros_ec: Add cros_ec_lpc driver for x86 devices
Chromebooks have an Embedded Controller (EC) that is used to
implement various functions such as keyboard, power and battery.
The AP can communicate with the EC through different bus types
such as I2C, SPI or LPC.
The cros_ec mfd driver is then composed of a core driver that
register the sub-devices as mfd cells and provide a high level
communication interface that is used by the rest of the kernel
and bus specific interfaces modules.
Each connection method then has its own driver, which register
with the EC driver interface-agnostic interface.
Currently, there are drivers to communicate with the EC over
I2C and SPI and this driver adds support for LPC.
Signed-off-by: Bill Richardson <wfrichar@chromium.org>
Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
If you agree with this commit message then I can change it on the next revision
but really $subject doesn't do anything special besides providing a transport
method to access the mfd device using a LPC bus.
Best regards,
Javier
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH RESEND v2 3/7] mfd: cros_ec: Add cros_ec_lpc driver for x86 devices
2015-01-20 16:52 ` Javier Martinez Canillas
2015-01-21 17:05 ` Javier Martinez Canillas
@ 2015-01-22 8:42 ` Lee Jones
2015-01-22 9:08 ` Javier Martinez Canillas
1 sibling, 1 reply; 35+ messages in thread
From: Lee Jones @ 2015-01-22 8:42 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Olof Johansson, Doug Anderson, Bill Richardson, Simon Glass,
Gwendal Grignou, Jonathan Corbet, linux-samsung-soc,
linux-kernel
On Tue, 20 Jan 2015, Javier Martinez Canillas wrote:
> Hello Lee,
>
> On 01/20/2015 05:34 PM, Lee Jones wrote:
> >>
> >> So, the Embedded Controller driver (drivers/mfd/cros_ec.c) falls into that
> >> category and in fact has been in the mfd driver for a long time. Now, if
> >> an mfd device support different type of buses (e.g: i2c, spi, etc) I see
> >> that both the core driver and the driver for the transport method are
> >> in the drivers/mfd directory. As an example:
> >>
> >> drivers/mfd/arizona-{core,i2c,spi}.c
> >> drivers/mfd/da9052-{core,i2c,spi}.c
> >> drivers/mfd/mc13xxx-{core,i2c,spi}.c
> >> drivers/mfd/tps65912-{core,i2c,spi}.c
> >> drivers/mfd/wm831x-{core,i2c,spi,otp}.c
> >>
> >> In the cros_ec case, we already have drivers/mfd/cros_ec_{i2c,spi}.c so
> >> since the Low Pin Count is another transport method I thought that this
> >> driver belonged to the drivers/mfd directory.
> >>
> >> Now, all those drivers may be wrong and the buses don't belong to the mfd
> >> subsystem but then I think we need to document that since it seems that is
> >> the correct way to do it just by looking at the other drivers.
> >
> > I don't think the drivers you mentioned above do anything practical.
> > For instance, they are not SPI/IC2/etc drivers. They should only
> > offer some abstraction layers which are used to communicate with the
> > device. The driver you are submitting looks a lot more like a device
> > driver, which should live somewhere else. Don't ask me where though,
> > I'm not even sure what a Low Pin Controller does.
> >
>
> The driver added by $subject doesn't really do anything practical either.
> LPC [0] is just another transport method like i2c or spi that is used on
> x86 Chromebooks to access the Embedded Controller.
I'm not sure that's true. It's pretty simple I grant you, but it
still looks like a driver, rather than an abstraction layer.
I would expect to see something more like:
static int cros_ec_lpc_readmem(...)
{
return call_to_driver_to_read_memory(...);
}
... instead of all those memory/register reads/writes.
Are there any other Low Pin Count drivers in the kernel?
> So the driver is really not that different than the cros_ec_{i2c,spi}.c
> drivers.
>
> Best regards,
> Javier
>
> [0]: http://en.wikipedia.org/wiki/Low_Pin_Count
--
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] 35+ messages in thread
* Re: [PATCH RESEND v2 3/7] mfd: cros_ec: Add cros_ec_lpc driver for x86 devices
2015-01-22 8:42 ` Lee Jones
@ 2015-01-22 9:08 ` Javier Martinez Canillas
2015-01-22 9:46 ` Lee Jones
0 siblings, 1 reply; 35+ messages in thread
From: Javier Martinez Canillas @ 2015-01-22 9:08 UTC (permalink / raw)
To: Lee Jones
Cc: Olof Johansson, Doug Anderson, Bill Richardson, Simon Glass,
Gwendal Grignou, Jonathan Corbet, linux-samsung-soc,
linux-kernel
Hello Lee,
On 01/22/2015 09:42 AM, Lee Jones wrote:
>> >
>> > I don't think the drivers you mentioned above do anything practical.
>> > For instance, they are not SPI/IC2/etc drivers. They should only
>> > offer some abstraction layers which are used to communicate with the
>> > device. The driver you are submitting looks a lot more like a device
>> > driver, which should live somewhere else. Don't ask me where though,
>> > I'm not even sure what a Low Pin Controller does.
>> >
>>
>> The driver added by $subject doesn't really do anything practical either.
>> LPC [0] is just another transport method like i2c or spi that is used on
>> x86 Chromebooks to access the Embedded Controller.
>
> I'm not sure that's true. It's pretty simple I grant you, but it
> still looks like a driver, rather than an abstraction layer.
>
> I would expect to see something more like:
>
> static int cros_ec_lpc_readmem(...)
> {
> return call_to_driver_to_read_memory(...);
>
> }
>
> ... instead of all those memory/register reads/writes.
>
Yeah... in that sense I've to admit that is more complex than the I2C and SPI
drivers, yet those have a subsystem in the kernel with helpers functions to
do most of the communication:
static int cros_ec_cmd_xfer_i2c(struct cros_ec_device *ec_dev,
struct cros_ec_command *msg)
{
...
ret = i2c_transfer(client->adapter, i2c_msg, 2);
...
}
static int cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev,
struct cros_ec_command *ec_msg)
{
...
spi_message_add_tail(&trans, &msg);
ret = spi_sync(ec_spi->spi, &msg);
...
}
But there doesn't seem to be a LPC subsystem in the kernel so we don't have a
nice abstraction layer in this case.
> Are there any other Low Pin Count drivers in the kernel?
>
I don't know tbh, I didn't even know what LPC was before I picked this patch
to push it upstream. I searched in the Linux codebase for other LPC drivers
but I didn't find anything, that doesn't mean that it doesn't exist though.
Best regards,
Javier
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH RESEND v2 3/7] mfd: cros_ec: Add cros_ec_lpc driver for x86 devices
2015-01-22 9:08 ` Javier Martinez Canillas
@ 2015-01-22 9:46 ` Lee Jones
2015-01-22 10:36 ` Javier Martinez Canillas
0 siblings, 1 reply; 35+ messages in thread
From: Lee Jones @ 2015-01-22 9:46 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Olof Johansson, Doug Anderson, Bill Richardson, Simon Glass,
Gwendal Grignou, Jonathan Corbet, linux-samsung-soc,
linux-kernel, arnd
On Thu, 22 Jan 2015, Javier Martinez Canillas wrote:
> Hello Lee,
>
> On 01/22/2015 09:42 AM, Lee Jones wrote:
> >> >
> >> > I don't think the drivers you mentioned above do anything practical.
> >> > For instance, they are not SPI/IC2/etc drivers. They should only
> >> > offer some abstraction layers which are used to communicate with the
> >> > device. The driver you are submitting looks a lot more like a device
> >> > driver, which should live somewhere else. Don't ask me where though,
> >> > I'm not even sure what a Low Pin Controller does.
> >> >
> >>
> >> The driver added by $subject doesn't really do anything practical either.
> >> LPC [0] is just another transport method like i2c or spi that is used on
> >> x86 Chromebooks to access the Embedded Controller.
> >
> > I'm not sure that's true. It's pretty simple I grant you, but it
> > still looks like a driver, rather than an abstraction layer.
> >
> > I would expect to see something more like:
> >
> > static int cros_ec_lpc_readmem(...)
> > {
> > return call_to_driver_to_read_memory(...);
> >
> > }
> >
> > ... instead of all those memory/register reads/writes.
> >
>
> Yeah... in that sense I've to admit that is more complex than the I2C and SPI
> drivers, yet those have a subsystem in the kernel with helpers functions to
> do most of the communication:
>
> static int cros_ec_cmd_xfer_i2c(struct cros_ec_device *ec_dev,
> struct cros_ec_command *msg)
> {
> ...
> ret = i2c_transfer(client->adapter, i2c_msg, 2);
> ...
> }
>
> static int cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev,
> struct cros_ec_command *ec_msg)
> {
> ...
> spi_message_add_tail(&trans, &msg);
> ret = spi_sync(ec_spi->spi, &msg);
> ...
> }
>
> But there doesn't seem to be a LPC subsystem in the kernel so we don't have a
> nice abstraction layer in this case.
This is the crux of the problem. However, I feel bad for MFD, as it
is, once more, being used as an "well it doesn't fit anywhere else, so
let's shoehorn it in there" type of dumping ground.
> > Are there any other Low Pin Count drivers in the kernel?
> >
>
> I don't know tbh, I didn't even know what LPC was before I picked this patch
> to push it upstream. I searched in the Linux codebase for other LPC drivers
> but I didn't find anything, that doesn't mean that it doesn't exist though.
I agree. Perhaps a suitable driver should live in drivers/misc until
there are enough of them to warrant its own subsystem.
Anyone else have an opinion?
--
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] 35+ messages in thread
* Re: [PATCH RESEND v2 3/7] mfd: cros_ec: Add cros_ec_lpc driver for x86 devices
2015-01-22 9:46 ` Lee Jones
@ 2015-01-22 10:36 ` Javier Martinez Canillas
2015-01-22 10:56 ` Lee Jones
0 siblings, 1 reply; 35+ messages in thread
From: Javier Martinez Canillas @ 2015-01-22 10:36 UTC (permalink / raw)
To: Lee Jones
Cc: Olof Johansson, Doug Anderson, Bill Richardson, Simon Glass,
Gwendal Grignou, Jonathan Corbet, linux-samsung-soc,
linux-kernel, arnd
Hello Lee,
On 01/22/2015 10:46 AM, Lee Jones wrote:
>>
>> But there doesn't seem to be a LPC subsystem in the kernel so we don't have a
>> nice abstraction layer in this case.
>
> This is the crux of the problem. However, I feel bad for MFD, as it
> is, once more, being used as an "well it doesn't fit anywhere else, so
> let's shoehorn it in there" type of dumping ground.
>
Yes, I completely understand your point, is that I didn't think that a ~300
lines driver was that bad specially since the communication bits that reads
and writes the register is not a complex logic IMHO.
>> > Are there any other Low Pin Count drivers in the kernel?
>> >
>>
>> I don't know tbh, I didn't even know what LPC was before I picked this patch
>> to push it upstream. I searched in the Linux codebase for other LPC drivers
>> but I didn't find anything, that doesn't mean that it doesn't exist though.
>
> I agree. Perhaps a suitable driver should live in drivers/misc until
> there are enough of them to warrant its own subsystem.
>
Yes, I can move the driver to drivers/misc if you think that is more suitable
to be there.
I've taken another look and AFAICT there are two other mfd drivers that use an
LPC bus, these are drivers/mfd/lpc_{i,s}ch.c for Intel's I/O Controller HUB and
System Controller Hub respectively.
> Anyone else have an opinion?
>
Best regards,
Javier
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH RESEND v2 3/7] mfd: cros_ec: Add cros_ec_lpc driver for x86 devices
2015-01-22 10:36 ` Javier Martinez Canillas
@ 2015-01-22 10:56 ` Lee Jones
2015-01-22 11:17 ` Javier Martinez Canillas
0 siblings, 1 reply; 35+ messages in thread
From: Lee Jones @ 2015-01-22 10:56 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Olof Johansson, Doug Anderson, Bill Richardson, Simon Glass,
Gwendal Grignou, Jonathan Corbet, linux-samsung-soc,
linux-kernel, arnd
On Thu, 22 Jan 2015, Javier Martinez Canillas wrote:
> On 01/22/2015 10:46 AM, Lee Jones wrote:
> >>
> >> But there doesn't seem to be a LPC subsystem in the kernel so we don't have a
> >> nice abstraction layer in this case.
> >
> > This is the crux of the problem. However, I feel bad for MFD, as it
> > is, once more, being used as an "well it doesn't fit anywhere else, so
> > let's shoehorn it in there" type of dumping ground.
> >
> Yes, I completely understand your point, is that I didn't think that a ~300
> lines driver was that bad specially since the communication bits that reads
> and writes the register is not a complex logic IMHO.
This has nothing to do with LoC, it's the principle of the matter.
> >> > Are there any other Low Pin Count drivers in the kernel?
> >> >
> >>
> >> I don't know tbh, I didn't even know what LPC was before I picked this patch
> >> to push it upstream. I searched in the Linux codebase for other LPC drivers
> >> but I didn't find anything, that doesn't mean that it doesn't exist though.
> >
> > I agree. Perhaps a suitable driver should live in drivers/misc until
> > there are enough of them to warrant its own subsystem.
> >
>
> Yes, I can move the driver to drivers/misc if you think that is more suitable
> to be there.
>
> I've taken another look and AFAICT there are two other mfd drivers that use an
> LPC bus, these are drivers/mfd/lpc_{i,s}ch.c for Intel's I/O Controller HUB and
> System Controller Hub respectively.
These looked like PCI aggregates when I looked at them last?
--
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] 35+ messages in thread
* Re: [PATCH RESEND v2 3/7] mfd: cros_ec: Add cros_ec_lpc driver for x86 devices
2015-01-22 10:56 ` Lee Jones
@ 2015-01-22 11:17 ` Javier Martinez Canillas
0 siblings, 0 replies; 35+ messages in thread
From: Javier Martinez Canillas @ 2015-01-22 11:17 UTC (permalink / raw)
To: Lee Jones
Cc: Olof Johansson, Doug Anderson, Bill Richardson, Simon Glass,
Gwendal Grignou, Jonathan Corbet, linux-samsung-soc,
linux-kernel, arnd
Hello Lee,
On 01/22/2015 11:56 AM, Lee Jones wrote:
>> >
>> Yes, I completely understand your point, is that I didn't think that a ~300
>> lines driver was that bad specially since the communication bits that reads
>> and writes the register is not a complex logic IMHO.
>
> This has nothing to do with LoC, it's the principle of the matter.
>
Ok, got it.
>> >> > Are there any other Low Pin Count drivers in the kernel?
>> >> >
>> >>
>> >> I don't know tbh, I didn't even know what LPC was before I picked this patch
>> >> to push it upstream. I searched in the Linux codebase for other LPC drivers
>> >> but I didn't find anything, that doesn't mean that it doesn't exist though.
>> >
>> > I agree. Perhaps a suitable driver should live in drivers/misc until
>> > there are enough of them to warrant its own subsystem.
>> >
>>
>> Yes, I can move the driver to drivers/misc if you think that is more suitable
>> to be there.
>>
>> I've taken another look and AFAICT there are two other mfd drivers that use an
>> LPC bus, these are drivers/mfd/lpc_{i,s}ch.c for Intel's I/O Controller HUB and
>> System Controller Hub respectively.
>
> These looked like PCI aggregates when I looked at them last?
>
Yes, it seems the LPC bus is behind a PCI bus on these chips. Or at least
that's what I understood on a quick read to the Intel SCH datasheet [0].
Now going back to the Cros EC LPC driver, you are right that this may not
be suitable to drivers/mfd so I'll move it to drivers/misc instead.
Thanks a lot for your feedback.
Best regards,
Javier
[0]: http://www.intel.com/content/www/us/en/intelligent-systems/menlow/sch-datasheet.html
^ permalink raw reply [flat|nested] 35+ messages in thread
end of thread, other threads:[~2015-01-22 11:17 UTC | newest]
Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-02 13:32 [PATCH RESEND v2 0/7] platform/chrome: Add user-space dev inferface support Javier Martinez Canillas
2015-01-02 13:32 ` [PATCH RESEND v2 1/7] mfd: cros_ec: Use fixed size arrays to transfer data with the EC Javier Martinez Canillas
2015-01-20 7:48 ` Lee Jones
2015-01-20 15:40 ` Javier Martinez Canillas
2015-01-02 13:32 ` [PATCH RESEND v2 2/7] mfd: cros_ec: Add char dev and virtual dev pointers Javier Martinez Canillas
2015-01-20 7:50 ` Lee Jones
2015-01-20 15:41 ` Javier Martinez Canillas
2015-01-20 16:36 ` Lee Jones
2015-01-20 16:45 ` Javier Martinez Canillas
2015-01-20 16:51 ` Lee Jones
2015-01-02 13:32 ` [PATCH RESEND v2 3/7] mfd: cros_ec: Add cros_ec_lpc driver for x86 devices Javier Martinez Canillas
2015-01-20 8:11 ` Lee Jones
2015-01-20 15:58 ` Javier Martinez Canillas
2015-01-20 16:34 ` Lee Jones
2015-01-20 16:52 ` Javier Martinez Canillas
2015-01-21 17:05 ` Javier Martinez Canillas
2015-01-22 8:42 ` Lee Jones
2015-01-22 9:08 ` Javier Martinez Canillas
2015-01-22 9:46 ` Lee Jones
2015-01-22 10:36 ` Javier Martinez Canillas
2015-01-22 10:56 ` Lee Jones
2015-01-22 11:17 ` Javier Martinez Canillas
2015-01-02 13:32 ` [PATCH RESEND v2 4/7] platform/chrome: Add Chrome OS EC userspace device interface Javier Martinez Canillas
2015-01-13 23:40 ` Gwendal Grignou
2015-01-02 13:32 ` [PATCH RESEND v2 5/7] mfd: cros_ec: Instantiate ChromeOS EC character device Javier Martinez Canillas
2015-01-20 8:20 ` Lee Jones
2015-01-20 16:03 ` Javier Martinez Canillas
2015-01-20 16:29 ` Lee Jones
2015-01-20 16:36 ` Javier Martinez Canillas
2015-01-20 16:55 ` Lee Jones
2015-01-20 17:11 ` Javier Martinez Canillas
2015-01-21 16:56 ` Javier Martinez Canillas
2015-01-02 13:32 ` [PATCH RESEND v2 6/7] platform/chrome: Create sysfs attributes for the ChromeOS EC Javier Martinez Canillas
2015-01-02 13:32 ` [PATCH RESEND v2 7/7] platform/chrome: Expose Chrome OS Lightbar to users Javier Martinez Canillas
2015-01-12 10:18 ` [PATCH RESEND v2 0/7] platform/chrome: Add user-space dev inferface support Javier Martinez Canillas
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.