[PATCH v4 0/6] Add ChromeOS Embedded Controller support

[PATCH v4 0/6] Add ChromeOS Embedded Controller support

[Simon Glass]

The ChromeOS Embedded Controller (EC) is an Open Source EC implementation
used on ARM and Intel Chromebooks. Current implementations use a Cortex-M3
connected on a bus (such as I2C, SPI, LPC) to the AP. A separate interrupt
line is used to indicate when the EC needs service.

Functions performed by the EC vary by platform, but typically include
battery charging, keyboard scanning and power sequencing.

This series includes support for the EC message protocol, and implements
a matrix keyboard handler for Linux using the protocol. The EC performs
key scanning and passes scan data in response to AP requests. This is
used on the Samsung ARM Chromebook. No driver is available for LPC at
present.

This series can in principle operate on any hardware, but for it to actually
work on the Samsung ARM Chromebook, it needs patches which are currently in
progress to mainline: Exynos FDT interrupt support and I2C bus arbitration.

The driver is device-tree-enabled and a suitable binding is included in
this series. Example device tree nodes are included in the examples,
but no device tree patch for exynos5250-snow is provided at this stage, since
we must wait for the above-mentioned patches to land to avoid errors from
dtc. This can be added with a follow-on patch when that work is complete.

Changes in v4:
- Fix up trvial logging comments
- Remove messages reporting out of memory
- Add compatible string for cros-ec-keyb
- Remove wake notifier and let drivers use their own handlers instead
- Add 'depends on MFD_CROS_EC' to Kconfig
- Remove use of wake_notifier
- Remove manual code to locate device tree node
- Add resume handler to clear keyboard scan buffer if required

Changes in v3:
- Add stub for matrix_keypad_parse_of_params() when no CONFIG_OF
- Put back full DT range checking in tca8418 driver
- Remove 'select MFD_CROS_EC' from Kconfig as it isn't necessary
- Remove old_state by using input layer's idev->key
- Move inner loop of cros_ec_keyb_has_ghosting() into its own function and simplify
- Add check for not finding the device tree node
- Remove comment about leaking matrix_keypad_build_keymap()
- Use platform_get_drvdata() where possible
- Remove call to input_free_device() after input_unregister_device()

Changes in v2:
- Remove use of __devinit/__devexit
- Remove use of __devinit/__devexit
- Remove use of __devinit/__devexit
- Add new patch to decode matrix-keypad DT binding
- Remove use of __devinit/__devexit
- Use function to read matrix-keypad parameters from DT
- Remove key autorepeat parameters from DT binding and driver
- Use unsigned int for rows/cols

Simon Glass (6):
  mfd: Add ChromeOS EC messages header
  mfd: Add ChromeOS EC implementation
  mfd: Add ChromeOS EC I2C driver
  mfd: Add ChromeOS EC SPI driver
  Input: matrix-keymap: Add function to read the new DT binding
  Input: Add ChromeOS EC keyboard driver

 .../devicetree/bindings/input/cros-ec-keyb.txt     |   72 +
 Documentation/devicetree/bindings/mfd/cros-ec.txt  |   56 +
 drivers/input/keyboard/Kconfig                     |   12 +
 drivers/input/keyboard/Makefile                    |    1 +
 drivers/input/keyboard/cros_ec_keyb.c              |  356 +++++
 drivers/input/keyboard/lpc32xx-keys.c              |   11 +-
 drivers/input/keyboard/omap4-keypad.c              |   16 +-
 drivers/input/keyboard/tca8418_keypad.c            |    7 +-
 drivers/input/matrix-keymap.c                      |   19 +
 drivers/mfd/Kconfig                                |   28 +
 drivers/mfd/Makefile                               |    3 +
 drivers/mfd/cros_ec.c                              |  206 +++
 drivers/mfd/cros_ec_i2c.c                          |  262 ++++
 drivers/mfd/cros_ec_spi.c                          |  412 ++++++
 include/linux/input/matrix_keypad.h                |   19 +
 include/linux/mfd/cros_ec.h                        |  189 +++
 include/linux/mfd/cros_ec_commands.h               | 1369 ++++++++++++++++++++
 17 files changed, 3020 insertions(+), 18 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/cros-ec-keyb.txt
 create mode 100644 Documentation/devicetree/bindings/mfd/cros-ec.txt
 create mode 100644 drivers/input/keyboard/cros_ec_keyb.c
 create mode 100644 drivers/mfd/cros_ec.c
 create mode 100644 drivers/mfd/cros_ec_i2c.c
 create mode 100644 drivers/mfd/cros_ec_spi.c
 create mode 100644 include/linux/mfd/cros_ec.h
 create mode 100644 include/linux/mfd/cros_ec_commands.h

-- 
1.8.1.3

[PATCH v4 2/6] mfd: Add ChromeOS EC implementation

[Simon Glass]

This is the base EC implementation, which provides a high level
interface to the EC for use by the rest of the kernel. The actual
communcations is dealt with by a separate protocol driver which
registers itself with this interface.

Interrupts are passed on through a notifier.

A simple message structure is used to pass messages to the
protocol driver.
Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Signed-off-by: Che-Liang Chiou <clchiou-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Signed-off-by: Jonathan Kliegman <kliegs-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Signed-off-by: Luigi Semenzato <semenzato-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Signed-off-by: Olof Johansson <olofj-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Signed-off-by: Vincent Palatin <vpalatin-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
Changes in v4:
- Fix up trvial logging comments
- Remove messages reporting out of memory
- Add compatible string for cros-ec-keyb
- Remove wake notifier and let drivers use their own handlers instead

Changes in v3: None
Changes in v2:
- Remove use of __devinit/__devexit

 Documentation/devicetree/bindings/mfd/cros-ec.txt |  56 ++++++
 drivers/mfd/Kconfig                               |   8 +
 drivers/mfd/Makefile                              |   1 +
 drivers/mfd/cros_ec.c                             | 206 ++++++++++++++++++++++
 include/linux/mfd/cros_ec.h                       | 189 ++++++++++++++++++++
 5 files changed, 460 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/cros-ec.txt
 create mode 100644 drivers/mfd/cros_ec.c
 create mode 100644 include/linux/mfd/cros_ec.h

diff --git a/Documentation/devicetree/bindings/mfd/cros-ec.txt b/Documentation/devicetree/bindings/mfd/cros-ec.txt
new file mode 100644
index 0000000..e0e59c5
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/cros-ec.txt
@@ -0,0 +1,56 @@
+ChromeOS Embedded Controller
+
+Google's ChromeOS EC is a Cortex-M device which talks to the AP and
+implements various function such as keyboard and battery charging.
+
+The EC can be connect through various means (I2C, SPI, LPC) and the
+compatible string used depends on the inteface. Each connection method has
+its own driver which connects to the top level interface-agnostic EC driver.
+Other Linux driver (such as cros-ec-keyb for the matrix keyboard) connect to
+the top-level driver.
+
+Required properties (I2C):
+- compatible: "google,cros-ec-i2c"
+- reg: I2C slave address
+
+Required properties (SPI):
+- compatible: "google,cros-ec-spi"
+- reg: SPI chip select
+
+Required properties (LPC):
+- compatible: "google,cros-ec-lpc"
+- reg: List of (IO address, size) pairs defining the interface uses
+
+
+Example for I2C:
+
+i2c@12CA0000 {
+	cros-ec@1e {
+		reg = <0x1e>;
+		compatible = "google,cros-ec-i2c";
+		interrupts = <14 0>;
+		interrupt-parent = <&wakeup_eint>;
+		wakeup-source;
+	};
+
+
+Example for SPI:
+
+spi@131b0000 {
+	ec@0 {
+		compatible = "google,cros-ec-spi";
+		reg = <0x0>;
+		interrupts = <14 0>;
+		interrupt-parent = <&wakeup_eint>;
+		wakeup-source;
+		spi-max-frequency = <5000000>;
+		controller-data {
+		cs-gpio = <&gpf0 3 4 3 0>;
+		samsung,spi-cs;
+		samsung,spi-feedback-delay = <2>;
+		};
+	};
+};
+
+
+Example for LPC is not supplied as it is not yet implemented.
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 671f5b1..837a16b 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -21,6 +21,14 @@ config MFD_88PM860X
 	  select individual components like voltage regulators, RTC and
 	  battery-charger under the corresponding menus.
 
+config MFD_CROS_EC
+	bool "Support ChromeOS Embedded Controller"
+	help
+	  If you say yes here you get support for the ChromeOS Embedded
+	  Controller (EC) providing keyboard, battery and power services.
+	  You also ned to enable the driver for the bus you are using. The
+	  protocol for talking to the EC is defined by the bus driver.
+
 config MFD_88PM800
 	tristate "Support Marvell 88PM800"
 	depends on I2C=y && GENERIC_HARDIRQS
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index b90409c..967767d 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_MFD_88PM800)	+= 88pm800.o 88pm80x.o
 obj-$(CONFIG_MFD_88PM805)	+= 88pm805.o 88pm80x.o
 obj-$(CONFIG_MFD_SM501)		+= sm501.o
 obj-$(CONFIG_MFD_ASIC3)		+= asic3.o tmio_core.o
+obj-$(CONFIG_MFD_CROS_EC)	+= cros_ec.o
 
 rtsx_pci-objs			:= rtsx_pcr.o rts5209.o rts5229.o rtl8411.o rts5227.o
 obj-$(CONFIG_MFD_RTSX_PCI)	+= rtsx_pci.o
diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
new file mode 100644
index 0000000..2650d6e
--- /dev/null
+++ b/drivers/mfd/cros_ec.c
@@ -0,0 +1,206 @@
+/*
+ * ChromeOS EC multi-function device
+ *
+ * Copyright (C) 2012 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.
+ *
+ * The ChromeOS EC multi function device is used to mux all the requests
+ * to the EC device for its multiple features: keyboard controller,
+ * battery charging and regulator control, firmware update.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/slab.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/cros_ec.h>
+#include <linux/mfd/cros_ec_commands.h>
+
+int cros_ec_prepare_tx(struct cros_ec_device *ec_dev,
+		       struct cros_ec_msg *msg)
+{
+	uint8_t *out;
+	int csum, i;
+
+	BUG_ON(msg->out_len > EC_HOST_PARAM_SIZE);
+	out = ec_dev->dout;
+	out[0] = EC_CMD_VERSION0 + msg->version;
+	out[1] = msg->cmd;
+	out[2] = msg->out_len;
+	csum = out[0] + out[1] + out[2];
+	for (i = 0; i < msg->out_len; i++)
+		csum += out[EC_MSG_TX_HEADER_BYTES + i] = msg->out_buf[i];
+	out[EC_MSG_TX_HEADER_BYTES + msg->out_len] = (uint8_t)(csum & 0xff);
+
+	return EC_MSG_TX_PROTO_BYTES + msg->out_len;
+}
+
+static int cros_ec_command_sendrecv(struct cros_ec_device *ec_dev,
+		uint16_t cmd, void *out_buf, int out_len,
+		void *in_buf, int in_len)
+{
+	struct cros_ec_msg msg;
+
+	msg.version = cmd >> 8;
+	msg.cmd = cmd & 0xff;
+	msg.out_buf = out_buf;
+	msg.out_len = out_len;
+	msg.in_buf = in_buf;
+	msg.in_len = in_len;
+
+	return ec_dev->command_xfer(ec_dev, &msg);
+}
+
+static int cros_ec_command_recv(struct cros_ec_device *ec_dev,
+		uint16_t cmd, void *buf, int buf_len)
+{
+	return cros_ec_command_sendrecv(ec_dev, cmd, NULL, 0, buf, buf_len);
+}
+
+static int cros_ec_command_send(struct cros_ec_device *ec_dev,
+		uint16_t cmd, void *buf, int buf_len)
+{
+	return cros_ec_command_sendrecv(ec_dev, cmd, buf, buf_len, NULL, 0);
+}
+
+struct cros_ec_device *cros_ec_alloc(const char *name)
+{
+	struct cros_ec_device *ec_dev;
+
+	ec_dev = kzalloc(sizeof(*ec_dev), GFP_KERNEL);
+	if (ec_dev == NULL)
+		return NULL;
+	ec_dev->name = name;
+
+	return ec_dev;
+}
+
+void cros_ec_free(struct cros_ec_device *ec)
+{
+	kfree(ec);
+}
+
+static irqreturn_t ec_irq_thread(int irq, void *data)
+{
+	struct cros_ec_device *ec_dev = data;
+
+	if (device_may_wakeup(ec_dev->dev))
+		pm_wakeup_event(ec_dev->dev, 0);
+
+	blocking_notifier_call_chain(&ec_dev->event_notifier, 1, ec_dev);
+
+	return IRQ_HANDLED;
+}
+
+static struct mfd_cell cros_devs[] = {
+	{
+		.name = "cros-ec-keyb",
+		.id = 1,
+		.of_compatible = "cros-ec-keyb",
+	},
+};
+
+int cros_ec_register(struct cros_ec_device *ec_dev)
+{
+	struct device *dev = ec_dev->dev;
+	int err = 0;
+
+	BLOCKING_INIT_NOTIFIER_HEAD(&ec_dev->event_notifier);
+
+	ec_dev->command_send = cros_ec_command_send;
+	ec_dev->command_recv = cros_ec_command_recv;
+	ec_dev->command_sendrecv = cros_ec_command_sendrecv;
+
+	if (ec_dev->din_size) {
+		ec_dev->din = kmalloc(ec_dev->din_size, GFP_KERNEL);
+		if (!ec_dev->din) {
+			err = -ENOMEM;
+			goto fail_din;
+		}
+	}
+	if (ec_dev->dout_size) {
+		ec_dev->dout = kmalloc(ec_dev->dout_size, GFP_KERNEL);
+		if (!ec_dev->dout) {
+			err = -ENOMEM;
+			goto fail_dout;
+		}
+	}
+
+	if (!ec_dev->irq) {
+		dev_dbg(dev, "no valid IRQ: %d\n", ec_dev->irq);
+		goto fail_irq;
+	}
+
+	err = request_threaded_irq(ec_dev->irq, NULL, ec_irq_thread,
+				   IRQF_TRIGGER_LOW | IRQF_ONESHOT,
+				   "chromeos-ec", ec_dev);
+	if (err) {
+		dev_err(dev, "request irq %d: error %d\n", ec_dev->irq, err);
+		goto fail_irq;
+	}
+
+	err = mfd_add_devices(dev, 0, cros_devs,
+			      ARRAY_SIZE(cros_devs),
+			      NULL, ec_dev->irq, NULL);
+	if (err) {
+		dev_err(dev, "failed to add mfd devices\n");
+		goto fail_mfd;
+	}
+
+	dev_info(dev, "Chrome EC (%s)\n", ec_dev->name);
+
+	return 0;
+
+fail_mfd:
+	free_irq(ec_dev->irq, ec_dev);
+fail_irq:
+	kfree(ec_dev->dout);
+fail_dout:
+	kfree(ec_dev->din);
+fail_din:
+	return err;
+}
+
+int cros_ec_remove(struct cros_ec_device *ec_dev)
+{
+	mfd_remove_devices(ec_dev->dev);
+	free_irq(ec_dev->irq, ec_dev);
+	kfree(ec_dev->dout);
+	kfree(ec_dev->din);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+int cros_ec_suspend(struct cros_ec_device *ec_dev)
+{
+	struct device *dev = ec_dev->dev;
+
+	if (device_may_wakeup(dev))
+		ec_dev->wake_enabled = !enable_irq_wake(ec_dev->irq);
+
+	disable_irq(ec_dev->irq);
+	ec_dev->was_wake_device = ec_dev->wake_enabled;
+
+	return 0;
+}
+
+int cros_ec_resume(struct cros_ec_device *ec_dev)
+{
+	enable_irq(ec_dev->irq);
+
+	if (ec_dev->wake_enabled) {
+		disable_irq_wake(ec_dev->irq);
+		ec_dev->wake_enabled = 0;
+	}
+
+	return 0;
+}
+#endif
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
new file mode 100644
index 0000000..63b9a53
--- /dev/null
+++ b/include/linux/mfd/cros_ec.h
@@ -0,0 +1,189 @@
+/*
+ * ChromeOS EC multi-function device
+ *
+ * Copyright (C) 2012 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.
+ */
+
+#ifndef __LINUX_MFD_CROS_EC_H
+#define __LINUX_MFD_CROS_EC_H
+
+struct i2c_msg;
+
+#include <linux/mfd/cros_ec_commands.h>
+
+/*
+ * Command interface between EC and AP, for LPC, I2C and SPI interfaces.
+ */
+enum {
+	EC_MSG_TX_HEADER_BYTES	= 3,
+	EC_MSG_TX_TRAILER_BYTES	= 1,
+	EC_MSG_TX_PROTO_BYTES	= EC_MSG_TX_HEADER_BYTES +
+					EC_MSG_TX_TRAILER_BYTES,
+	EC_MSG_RX_PROTO_BYTES	= 3,
+
+	/* Max length of messages */
+	EC_MSG_BYTES		= EC_HOST_PARAM_SIZE + EC_MSG_TX_PROTO_BYTES,
+
+};
+
+/**
+ * struct cros_ec_msg - A message sent to the EC, and its reply
+ *
+ * @version: Command version number (often 0)
+ * @cmd: Command to send (EC_CMD_...)
+ * @out_buf: Outgoing payload (to EC)
+ * @outlen: Outgoing length
+ * @in_buf: Incoming payload (from EC)
+ * @in_len: Incoming length
+ */
+struct cros_ec_msg {
+	u8 version;
+	u8 cmd;
+	uint8_t *out_buf;
+	int out_len;
+	uint8_t *in_buf;
+	int in_len;
+};
+
+/**
+ * struct cros_ec_device - Information about a ChromeOS EC device
+ *
+ * @name: Name of this EC interface
+ * @priv: Private data
+ * @irq: Interrupt to use
+ * @din: input buffer (from EC)
+ * @dout: output buffer (to EC)
+ * \note
+ * These two buffers will always be dword-aligned and include enough
+ * space for up to 7 word-alignment bytes also, so we can ensure that
+ * the body of the message is always dword-aligned (64-bit).
+ *
+ * We use this alignment to keep ARM and x86 happy. Probably word
+ * alignment would be OK, there might be a small performance advantage
+ * to using dword.
+ * @din_size: size of din buffer
+ * @dout_size: size of dout buffer
+ * @command_send: send a command
+ * @command_recv: receive a command
+ * @get_name: return name of EC device (e.g. 'chromeos-ec')
+ * @get_phys_name: return name of physical comms layer (e.g. 'i2c-4')
+ * @get_parent: return pointer to parent device (e.g. i2c or spi device)
+ * @dev: Device pointer
+ * dev_lock: Lock to prevent concurrent access
+ * @wake_enabled: true if this device can wake the system from sleep
+ * @was_wake_device: true if this device was set to wake the system from
+ * sleep at the last suspend
+ * @event_notifier: interrupt event notifier for transport devices
+ */
+struct cros_ec_device {
+	const char *name;
+	void *priv;
+	int irq;
+	uint8_t *din;
+	uint8_t *dout;
+	int din_size;
+	int dout_size;
+	int (*command_send)(struct cros_ec_device *ec,
+			uint16_t cmd, void *out_buf, int out_len);
+	int (*command_recv)(struct cros_ec_device *ec,
+			uint16_t cmd, void *in_buf, int in_len);
+	int (*command_sendrecv)(struct cros_ec_device *ec,
+			uint16_t cmd, void *out_buf, int out_len,
+			void *in_buf, int in_len);
+	int (*command_xfer)(struct cros_ec_device *ec,
+			struct cros_ec_msg *msg);
+
+	const char *(*get_name)(struct cros_ec_device *ec_dev);
+
+	const char *(*get_phys_name)(struct cros_ec_device *ec_dev);
+
+	struct device *(*get_parent)(struct cros_ec_device *ec_dev);
+
+	/* These are --private-- fields - do not assign */
+	struct device *dev;
+	struct mutex dev_lock;
+	bool wake_enabled;
+	bool was_wake_device;
+	struct blocking_notifier_head event_notifier;
+};
+
+/**
+ * cros_ec_suspend - Handle a suspend operation for the ChromeOS EC device
+ *
+ * This can be called by drivers to handle a suspend event.
+ *
+ * ec_dev: Device to suspend
+ * @return 0 if ok, -ve on error
+ */
+int cros_ec_suspend(struct cros_ec_device *ec_dev);
+
+/**
+ * cros_ec_resume - Handle a resume operation for the ChromeOS EC device
+ *
+ * This can be called by drivers to handle a resume event.
+ *
+ * @ec_dev: Device to resume
+ * @return 0 if ok, -ve on error
+ */
+int cros_ec_resume(struct cros_ec_device *ec_dev);
+
+/**
+ * cros_ec_prepare_tx - Prepare an outgoing message in the output buffer
+ *
+ * This is intended to be used by all ChromeOS EC drivers, but at present
+ * only SPI uses it. Once LPC uses the same protocol it can start using it.
+ * I2C could use it now, with a refactor of the existing code.
+ *
+ * @ec_dev: Device to register
+ * @msg: Message to write
+ */
+int cros_ec_prepare_tx(struct cros_ec_device *ec_dev,
+		       struct cros_ec_msg *msg);
+
+/**
+ * cros_ec_remove - Remove a ChromeOS EC
+ *
+ * Call this to deregister a ChromeOS EC. After this you should call
+ * cros_ec_free().
+ *
+ * @ec_dev: Device to register
+ * @return 0 if ok, -ve on error
+ */
+int cros_ec_remove(struct cros_ec_device *ec_dev);
+
+/**
+ * cros_ec_register - Register a new ChromeOS EC, using the provided info
+ *
+ * Before calling this, call cros_ec_alloc() to get a pointer to a new device
+ * and then fill in all the fields up to the --private-- marker.
+ *
+ * @ec_dev: Device to register
+ * @return 0 if ok, -ve on error
+ */
+int cros_ec_register(struct cros_ec_device *ec_dev);
+
+/**
+ * cros_ec_alloc - Allocate a new ChromeOS EC
+ *
+ * @name: Name of EC (typically the interface it connects on)
+ * @return pointer to created device, or NULL on failure
+ */
+struct cros_ec_device *cros_ec_alloc(const char *name);
+
+/**
+ * cros_ec_free - Free a ChromeOS EC, the opposite of cros_ec_alloc().
+ *
+ * @ec_dev: Device to free (call cros_ec_remove() first)
+ */
+void cros_ec_free(struct cros_ec_device *ec_dev);
+
+#endif /* __LINUX_MFD_CROS_EC_H */
-- 
1.8.1.3

[PATCH v4 6/6] Input: Add ChromeOS EC keyboard driver

[Simon Glass]

Use the key-matrix layer to interpret key scan information from the EC
and inject input based on the FDT-supplied key map. This driver registers
itself with the ChromeOS EC driver to perform communications.

The matrix-keypad FDT binding is used with a small addition to control
ghosting.

Signed-off-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Luigi Semenzato <semenzato@chromium.org>
Signed-off-by: Vincent Palatin <vpalatin@chromium.org>
---
Changes in v4:
- Add 'depends on MFD_CROS_EC' to Kconfig
- Remove use of wake_notifier
- Remove manual code to locate device tree node
- Add resume handler to clear keyboard scan buffer if required

Changes in v3:
- Remove 'select MFD_CROS_EC' from Kconfig as it isn't necessary
- Remove old_state by using input layer's idev->key
- Move inner loop of cros_ec_keyb_has_ghosting() into its own function and simplify
- Add check for not finding the device tree node
- Remove comment about leaking matrix_keypad_build_keymap()
- Use platform_get_drvdata() where possible
- Remove call to input_free_device() after input_unregister_device()

Changes in v2:
- Remove use of __devinit/__devexit
- Use function to read matrix-keypad parameters from DT
- Remove key autorepeat parameters from DT binding and driver
- Use unsigned int for rows/cols

 .../devicetree/bindings/input/cros-ec-keyb.txt     |  72 +++++
 drivers/input/keyboard/Kconfig                     |  12 +
 drivers/input/keyboard/Makefile                    |   1 +
 drivers/input/keyboard/cros_ec_keyb.c              | 356 +++++++++++++++++++++
 4 files changed, 441 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/cros-ec-keyb.txt
 create mode 100644 drivers/input/keyboard/cros_ec_keyb.c

diff --git a/Documentation/devicetree/bindings/input/cros-ec-keyb.txt b/Documentation/devicetree/bindings/input/cros-ec-keyb.txt
new file mode 100644
index 0000000..0f6355c
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/cros-ec-keyb.txt
@@ -0,0 +1,72 @@
+ChromeOS EC Keyboard
+
+Google's ChromeOS EC Keyboard is a simple matrix keyboard implemented on
+a separate EC (Embedded Controller) device. It provides a message for reading
+key scans from the EC. These are then converted into keycodes for processing
+by the kernel.
+
+This binding is based on matrix-keymap.txt and extends/modifies it as follows:
+
+Required properties:
+- compatible: "google,cros-ec-keyb"
+
+Optional properties:
+- google,needs-ghost-filter: True to enable a ghost filter for the matrix
+keyboard. This is recommended if the EC does not have its own logic or
+hardware for this.
+
+
+Example:
+
+cros-ec-keyb {
+	compatible = "google,cros-ec-keyb";
+	keypad,num-rows = <8>;
+	keypad,num-columns = <13>;
+	google,needs-ghost-filter;
+	/*
+	 * Keymap entries take the form of 0xRRCCKKKK where
+	 * RR=Row CC=Column KKKK=Key Code
+	 * The values below are for a US keyboard layout and
+	 * are taken from the Linux driver. Note that the
+	 * 102ND key is not used for US keyboards.
+	 */
+	linux,keymap = <
+		/* CAPSLCK F1         B          F10     */
+		0x0001003a 0x0002003b 0x00030030 0x00040044
+		/* N       =          R_ALT      ESC     */
+		0x00060031 0x0008000d 0x000a0064 0x01010001
+		/* F4      G          F7         H       */
+		0x0102003e 0x01030022 0x01040041 0x01060023
+		/* '       F9         BKSPACE    L_CTRL  */
+		0x01080028 0x01090043 0x010b000e 0x0200001d
+		/* TAB     F3         T          F6      */
+		0x0201000f 0x0202003d 0x02030014 0x02040040
+		/* ]       Y          102ND      [       */
+		0x0205001b 0x02060015 0x02070056 0x0208001a
+		/* F8      GRAVE      F2         5       */
+		0x02090042 0x03010029 0x0302003c 0x03030006
+		/* F5      6          -          \       */
+		0x0304003f 0x03060007 0x0308000c 0x030b002b
+		/* R_CTRL  A          D          F       */
+		0x04000061 0x0401001e 0x04020020 0x04030021
+		/* S       K          J          ;       */
+		0x0404001f 0x04050025 0x04060024 0x04080027
+		/* L       ENTER      Z          C       */
+		0x04090026 0x040b001c 0x0501002c 0x0502002e
+		/* V       X          ,          M       */
+		0x0503002f 0x0504002d 0x05050033 0x05060032
+		/* L_SHIFT /          .          SPACE   */
+		0x0507002a 0x05080035 0x05090034 0x050B0039
+		/* 1       3          4          2       */
+		0x06010002 0x06020004 0x06030005 0x06040003
+		/* 8       7          0          9       */
+		0x06050009 0x06060008 0x0608000b 0x0609000a
+		/* L_ALT   DOWN       RIGHT      Q       */
+		0x060a0038 0x060b006c 0x060c006a 0x07010010
+		/* E       R          W          I       */
+		0x07020012 0x07030013 0x07040011 0x07050017
+		/* U       R_SHIFT    P          O       */
+		0x07060016 0x07070036 0x07080019 0x07090018
+		/* UP      LEFT    */
+		0x070b0067 0x070c0069>;
+};
diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index 078305e..094868f 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -628,4 +628,16 @@ config KEYBOARD_W90P910
 	  To compile this driver as a module, choose M here: the
 	  module will be called w90p910_keypad.
 
+config KEYBOARD_CROS_EC
+	tristate "ChromeOS EC keyboard"
+	select INPUT_MATRIXKMAP
+	depends on MFD_CROS_EC
+	help
+	  Say Y here to enable the matrix keyboard used by ChromeOS devices
+	  and implemented on the ChromeOS EC. You must enable one bus option
+	  (MFD_CROS_EC_I2C or MFD_CROS_EC_SPI) to use this.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called cros_ec_keyb.
+
 endif
diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
index 49b1645..0c43e8c 100644
--- a/drivers/input/keyboard/Makefile
+++ b/drivers/input/keyboard/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_KEYBOARD_AMIGA)		+= amikbd.o
 obj-$(CONFIG_KEYBOARD_ATARI)		+= atakbd.o
 obj-$(CONFIG_KEYBOARD_ATKBD)		+= atkbd.o
 obj-$(CONFIG_KEYBOARD_BFIN)		+= bf54x-keys.o
+obj-$(CONFIG_KEYBOARD_CROS_EC)		+= cros_ec_keyb.o
 obj-$(CONFIG_KEYBOARD_DAVINCI)		+= davinci_keyscan.o
 obj-$(CONFIG_KEYBOARD_EP93XX)		+= ep93xx_keypad.o
 obj-$(CONFIG_KEYBOARD_GOLDFISH_EVENTS)	+= goldfish_events.o
diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
new file mode 100644
index 0000000..7c32e06
--- /dev/null
+++ b/drivers/input/keyboard/cros_ec_keyb.c
@@ -0,0 +1,356 @@
+/*
+ * ChromeOS EC keyboard driver
+ *
+ * Copyright (C) 2012 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/module.h>
+#include <linux/i2c.h>
+#include <linux/input.h>
+#include <linux/kernel.h>
+#include <linux/notifier.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/input/matrix_keypad.h>
+#include <linux/mfd/cros_ec.h>
+#include <linux/mfd/cros_ec_commands.h>
+
+/*
+ * @rows: Number of rows in the keypad
+ * @cols: Number of columns in the keypad
+ * @row_shift: log2 or number of rows, rounded up
+ * @keymap_data: Matrix keymap data used to convert to keyscan values
+ * @ghost_filter: true to enable the matrix key-ghosting filter
+ * @dev: Device pointer
+ * @idev: Input device
+ * @ec: Top level ChromeOS device to use to talk to EC
+ * @event_notifier: interrupt event notifier for transport devices
+ */
+struct cros_ec_keyb {
+	unsigned int rows;
+	unsigned int cols;
+	int row_shift;
+	const struct matrix_keymap_data *keymap_data;
+	bool ghost_filter;
+
+	struct device *dev;
+	struct input_dev *idev;
+	struct cros_ec_device *ec;
+	struct notifier_block notifier;
+};
+
+
+static bool cros_ec_keyb_row_has_ghosting(struct cros_ec_keyb *ckdev,
+					  uint8_t *buf, int row)
+{
+	int pressed_in_row = 0;
+	int row_has_teeth = 0;
+	int col, mask;
+
+	mask = 1 << row;
+	for (col = 0; col < ckdev->cols; col++) {
+		if (buf[col] & mask) {
+			pressed_in_row++;
+			row_has_teeth |= buf[col] & ~mask;
+			if (pressed_in_row > 1 && row_has_teeth) {
+				/* ghosting */
+				dev_dbg(ckdev->dev,
+					"ghost found at: r%d c%d, pressed %d, teeth 0x%x\n",
+					row, col, pressed_in_row,
+					row_has_teeth);
+				return true;
+			}
+		}
+	}
+
+	return false;
+}
+
+/*
+ * Returns true when there is at least one combination of pressed keys that
+ * results in ghosting.
+ */
+static bool cros_ec_keyb_has_ghosting(struct cros_ec_keyb *ckdev, uint8_t *buf)
+{
+	int row;
+
+	/*
+	 * Ghosting happens if for any pressed key X there are other keys
+	 * pressed both in the same row and column of X as, for instance,
+	 * in the following diagram:
+	 *
+	 * . . Y . g .
+	 * . . . . . .
+	 * . . . . . .
+	 * . . X . Z .
+	 *
+	 * In this case only X, Y, and Z are pressed, but g appears to be
+	 * pressed too (see Wikipedia).
+	 *
+	 * We can detect ghosting in a single pass (*) over the keyboard state
+	 * by maintaining two arrays.  pressed_in_row counts how many pressed
+	 * keys we have found in a row.  row_has_teeth is true if any of the
+	 * pressed keys for this row has other pressed keys in its column.  If
+	 * at any point of the scan we find that a row has multiple pressed
+	 * keys, and at least one of them is at the intersection with a column
+	 * with multiple pressed keys, we're sure there is ghosting.
+	 * Conversely, if there is ghosting, we will detect such situation for
+	 * at least one key during the pass.
+	 *
+	 * (*) This looks linear in the number of keys, but it's not.  We can
+	 * cheat because the number of rows is small.
+	 */
+	for (row = 0; row < ckdev->rows; row++) {
+		if (cros_ec_keyb_row_has_ghosting(ckdev, buf, row))
+			return true;
+	}
+
+	return false;
+}
+
+/*
+ * Compares the new keyboard state to the old one and produces key
+ * press/release events accordingly.  The keyboard state is 13 bytes (one byte
+ * per column)
+ */
+static void cros_ec_keyb_process(struct cros_ec_keyb *ckdev,
+			 uint8_t *kb_state, int len)
+{
+	int col, row;
+	int new_state;
+	int num_cols;
+
+	num_cols = len;
+
+	if (ckdev->ghost_filter && cros_ec_keyb_has_ghosting(ckdev, kb_state)) {
+		/*
+		 * Simple-minded solution: ignore this state. The obvious
+		 * improvement is to only ignore changes to keys involved in
+		 * the ghosting, but process the other changes.
+		 */
+		dev_dbg(ckdev->dev, "ghosting found\n");
+		return;
+	}
+
+	for (col = 0; col < ckdev->cols; col++) {
+		for (row = 0; row < ckdev->rows; row++) {
+			int code = MATRIX_SCAN_CODE(row, col, ckdev->row_shift);
+
+			new_state = kb_state[col] & (1 << row);
+			if (!!new_state != test_bit(code, ckdev->idev->key)) {
+				dev_dbg(ckdev->dev,
+					"changed: [r%d c%d]: byte %02x\n",
+					row, col, new_state);
+
+				input_report_key(ckdev->idev, code, new_state);
+			}
+		}
+	}
+	input_sync(ckdev->idev);
+}
+
+static int cros_ec_keyb_open(struct input_dev *dev)
+{
+	struct cros_ec_keyb *ckdev = input_get_drvdata(dev);
+
+	return blocking_notifier_chain_register(&ckdev->ec->event_notifier,
+						&ckdev->notifier);
+}
+
+static void cros_ec_keyb_close(struct input_dev *dev)
+{
+	struct cros_ec_keyb *ckdev = input_get_drvdata(dev);
+
+	blocking_notifier_chain_unregister(&ckdev->ec->event_notifier,
+					   &ckdev->notifier);
+}
+
+static int cros_ec_keyb_get_state(struct cros_ec_keyb *ckdev, uint8_t *kb_state)
+{
+	return ckdev->ec->command_recv(ckdev->ec, EC_CMD_MKBP_STATE,
+					  kb_state, ckdev->cols);
+}
+
+static int cros_ec_keyb_work(struct notifier_block *nb,
+		     unsigned long state, void *_notify)
+{
+	int ret;
+	struct cros_ec_keyb *ckdev = container_of(nb, struct cros_ec_keyb,
+						    notifier);
+	uint8_t kb_state[ckdev->cols];
+
+	ret = cros_ec_keyb_get_state(ckdev, kb_state);
+	if (ret >= 0)
+		cros_ec_keyb_process(ckdev, kb_state, ret);
+
+	return NOTIFY_DONE;
+}
+
+/* Clear any keys in the buffer */
+static void cros_ec_keyb_clear_keyboard(struct cros_ec_keyb *ckdev)
+{
+	uint8_t old_state[ckdev->cols];
+	uint8_t new_state[ckdev->cols];
+	unsigned long duration;
+	int i, ret;
+
+	/*
+	 * Keep reading until we see that the scan state does not change.
+	 * That indicates that we are done.
+	 *
+	 * Assume that the EC keyscan buffer is at most 32 deep.
+	 */
+	duration = jiffies;
+	ret = cros_ec_keyb_get_state(ckdev, new_state);
+	for (i = 1; !ret && i < 32; i++) {
+		memcpy(old_state, new_state, sizeof(old_state));
+		ret = cros_ec_keyb_get_state(ckdev, new_state);
+		if (0 == memcmp(old_state, new_state, sizeof(old_state)))
+			break;
+	}
+	duration = jiffies - duration;
+	dev_info(ckdev->dev, "Discarded %d keyscan(s) in %dus\n", i,
+		jiffies_to_usecs(duration));
+}
+
+static int cros_ec_keyb_probe(struct platform_device *pdev)
+{
+	struct cros_ec_device *ec = dev_get_drvdata(pdev->dev.parent);
+	struct device *dev = ec->dev;
+	struct cros_ec_keyb *ckdev = NULL;
+	struct input_dev *idev = NULL;
+	struct device_node *np;
+	int err;
+
+	np = pdev->dev.of_node;
+	if (!np)
+		return -ENODEV;
+
+	ckdev = kzalloc(sizeof(*ckdev), GFP_KERNEL);
+	if (!ckdev) {
+		dev_err(dev, "cannot allocate memory for ckdev\n");
+		return -ENOMEM;
+	}
+	err = matrix_keypad_parse_of_params(&pdev->dev, &ckdev->rows,
+					    &ckdev->cols);
+	if (err)
+		goto fail_alloc_dev;
+
+	idev = input_allocate_device();
+	if (!idev) {
+		err = -ENOMEM;
+		dev_err(dev, "cannot allocate memory for input device\n");
+		goto fail_alloc_dev;
+	}
+
+	ckdev->ec = ec;
+	ckdev->notifier.notifier_call = cros_ec_keyb_work;
+	ckdev->dev = dev;
+	dev_set_drvdata(&pdev->dev, ckdev);
+
+	idev->name = ec->get_name(ec);
+	idev->phys = ec->get_phys_name(ec);
+	__set_bit(EV_REP, idev->evbit);
+
+	idev->id.bustype = BUS_VIRTUAL;
+	idev->id.version = 1;
+	idev->id.product = 0;
+	idev->dev.parent = &pdev->dev;
+	idev->open = cros_ec_keyb_open;
+	idev->close = cros_ec_keyb_close;
+
+	ckdev->ghost_filter = of_property_read_bool(np,
+					"google,needs-ghost-filter");
+
+	err = matrix_keypad_build_keymap(NULL, NULL, ckdev->rows, ckdev->cols,
+					 NULL, idev);
+	if (err) {
+		dev_err(dev, "cannot build key matrix\n");
+		goto fail_matrix;
+	}
+
+	ckdev->row_shift = get_count_order(ckdev->cols);
+
+	input_set_capability(idev, EV_MSC, MSC_SCAN);
+	input_set_drvdata(idev, ckdev);
+	ckdev->idev = idev;
+	err = input_register_device(ckdev->idev);
+	if (err) {
+		dev_err(dev, "cannot register input device\n");
+		goto fail_register;
+	}
+
+	return 0;
+
+fail_register:
+	kfree(idev->keycode);
+fail_matrix:
+	input_free_device(idev);
+fail_alloc_dev:
+	kfree(ckdev);
+	return err;
+}
+
+static int cros_ec_keyb_remove(struct platform_device *pdev)
+{
+	struct cros_ec_keyb *ckdev = platform_get_drvdata(pdev);
+	struct input_dev *idev = ckdev->idev;
+
+	input_unregister_device(idev);
+	kfree(ckdev);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int cros_ec_keyb_resume(struct device *dev)
+{
+	struct cros_ec_keyb *ckdev = dev_get_drvdata(dev);
+
+	/*
+	 * When the EC is not a wake source, then it could not have caused the
+	 * resume, so we clear the EC's key scan buffer. If the EC was a
+	 * wake source (e.g. the lid is open and the user might press a key to
+	 * wake) then the key scan buffer should be preserved.
+	 */
+	if (ckdev->ec->was_wake_device)
+		cros_ec_keyb_clear_keyboard(ckdev);
+
+	return 0;
+}
+
+#endif
+
+static SIMPLE_DEV_PM_OPS(cros_ec_keyb_pm_ops, NULL, cros_ec_keyb_resume);
+
+static struct platform_driver cros_ec_keyb_driver = {
+	.probe = cros_ec_keyb_probe,
+	.remove = cros_ec_keyb_remove,
+	.driver = {
+		.name = "cros-ec-keyb",
+		.pm	= &cros_ec_keyb_pm_ops,
+	},
+};
+
+module_platform_driver(cros_ec_keyb_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("ChromeOS EC keyboard driver");
+MODULE_ALIAS("platform:cros-ec-keyb");
-- 
1.8.1.3

Re: [PATCH v4 6/6] Input: Add ChromeOS EC keyboard driver

[Dmitry Torokhov]

Hi Simon,

On Fri, Feb 15, 2013 at 08:16:12PM -0800, Simon Glass wrote:
> +	for (row = 0; row < ckdev->rows; row++) {
> +		if (cros_ec_keyb_row_has_ghosting(ckdev, buf, row))
> +			return true;
> +	}

No need for curly braces here. I would not care if not for below.

> +
> +	return 0;
> +
> +fail_register:
> +	kfree(idev->keycode);

Sorry I did not notice this before, but idev->keycode is devm-managed,
so you either need to use devm_kfree() or just remove call to kfree()
and let it clean up automatically (which will happen if binding fails or
upon removal).

BTW, maybe you should move the whole driver to devm_*? We have
devm_kzalloc() for ckdev and you can use devm_input_allocate_device().
Then you can get rid of entire erro handling path and completely remove
the remove() method as well.
 
> +fail_matrix:
> +	input_free_device(idev);
> +fail_alloc_dev:
> +	kfree(ckdev);
> +	return err;
> +}
> +

Thanks.

-- 
Dmitry

Re: [PATCH v4 6/6] Input: Add ChromeOS EC keyboard driver

[Simon Glass]

Hi Dmitry,

On Sat, Feb 16, 2013 at 12:49 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi Simon,
>
> On Fri, Feb 15, 2013 at 08:16:12PM -0800, Simon Glass wrote:
>> +     for (row = 0; row < ckdev->rows; row++) {
>> +             if (cros_ec_keyb_row_has_ghosting(ckdev, buf, row))
>> +                     return true;
>> +     }
>
> No need for curly braces here. I would not care if not for below.

OK I dont't think I even knew about that rule. Actually, what is that rule?

>
>> +
>> +     return 0;
>> +
>> +fail_register:
>> +     kfree(idev->keycode);
>
> Sorry I did not notice this before, but idev->keycode is devm-managed,
> so you either need to use devm_kfree() or just remove call to kfree()
> and let it clean up automatically (which will happen if binding fails or
> upon removal).
>
> BTW, maybe you should move the whole driver to devm_*? We have
> devm_kzalloc() for ckdev and you can use devm_input_allocate_device().
> Then you can get rid of entire erro handling path and completely remove
> the remove() method as well.

Yes I was thinking about that - might as well do it now.

>
>> +fail_matrix:
>> +     input_free_device(idev);
>> +fail_alloc_dev:
>> +     kfree(ckdev);
>> +     return err;
>> +}
>> +
>
> Thanks.
>
> --
> Dmitry

Regards,
Simon

Re: [PATCH v4 6/6] Input: Add ChromeOS EC keyboard driver

[Joe Perches]

On Mon, 2013-02-18 at 20:13 -0800, Simon Glass wrote:
> On Sat, Feb 16, 2013 at 12:49 PM, Dmitry Torokhov
> > On Fri, Feb 15, 2013 at 08:16:12PM -0800, Simon Glass wrote:
> >> +     for (row = 0; row < ckdev->rows; row++) {
> >> +             if (cros_ec_keyb_row_has_ghosting(ckdev, buf, row))
> >> +                     return true;
> >> +     }
> >
> > No need for curly braces here. I would not care if not for below.
> 
> OK I dont't think I even knew about that rule. Actually, what is that rule?

There is no rule, uses with and without braces
exist in about similar numbers in the kernel.

Both are used ~2000 times.

Newer uses more commonly have braces and I think
using braces is a better style.

Re: [PATCH v4 6/6] Input: Add ChromeOS EC keyboard driver

[Dmitry Torokhov]

On Mon, Feb 18, 2013 at 11:20:43PM -0800, Joe Perches wrote:
> On Mon, 2013-02-18 at 20:13 -0800, Simon Glass wrote:
> > On Sat, Feb 16, 2013 at 12:49 PM, Dmitry Torokhov
> > > On Fri, Feb 15, 2013 at 08:16:12PM -0800, Simon Glass wrote:
> > >> +     for (row = 0; row < ckdev->rows; row++) {
> > >> +             if (cros_ec_keyb_row_has_ghosting(ckdev, buf, row))
> > >> +                     return true;
> > >> +     }
> > >
> > > No need for curly braces here. I would not care if not for below.
> > 
> > OK I dont't think I even knew about that rule. Actually, what is that rule?
> 
> There is no rule, uses with and without braces
> exist in about similar numbers in the kernel.
> 
> Both are used ~2000 times.
> 
> Newer uses more commonly have braces and I think
> using braces is a better style.

I consider this example of a single statement body (albeit nested) and
therefore no braces are necessary, as mentioned in out coding style.

Thanks.

-- 
Dmitry

Re: [PATCH v4 6/6] Input: Add ChromeOS EC keyboard driver

[li guang]

在 2013-02-15五的 20:16 -0800，Simon Glass写道：
> Use the key-matrix layer to interpret key scan information from the EC
> and inject input based on the FDT-supplied key map. This driver registers
> itself with the ChromeOS EC driver to perform communications.

[snip ...]
> +/*
> + * Returns true when there is at least one combination of pressed keys that
> + * results in ghosting.
> + */
> +static bool cros_ec_keyb_has_ghosting(struct cros_ec_keyb *ckdev, uint8_t *buf)
> +{
> +	int row;
> +
> +	/*
> +	 * Ghosting happens if for any pressed key X there are other keys
> +	 * pressed both in the same row and column of X as, for instance,
> +	 * in the following diagram:
> +	 *
> +	 * . . Y . g .
> +	 * . . . . . .
> +	 * . . . . . .
> +	 * . . X . Z .
> +	 *
> +	 * In this case only X, Y, and Z are pressed, but g appears to be
> +	 * pressed too (see Wikipedia).
> +	 *
> +	 * We can detect ghosting in a single pass (*) over the keyboard state
> +	 * by maintaining two arrays.  pressed_in_row counts how many pressed
> +	 * keys we have found in a row.  row_has_teeth is true if any of the
> +	 * pressed keys for this row has other pressed keys in its column.  If
> +	 * at any point of the scan we find that a row has multiple pressed
> +	 * keys, and at least one of them is at the intersection with a column
> +	 * with multiple pressed keys, we're sure there is ghosting.
> +	 * Conversely, if there is ghosting, we will detect such situation for
> +	 * at least one key during the pass.
> +	 *
> +	 * (*) This looks linear in the number of keys, but it's not.  We can
> +	 * cheat because the number of rows is small.
> +	 */
> +	for (row = 0; row < ckdev->rows; row++) {
> +		if (cros_ec_keyb_row_has_ghosting(ckdev, buf, row))
> +			return true;
> +	}
> +
> +	return false;
> +}

are you sure your EC's firmware did not do ghost-key detection?
or, did you test ghost-key with/without your own ghost-key detection?
as far as I know, ghost-key should be take care either by keyboard
designer or firmware.

> +
> +/*
> + * Compares the new keyboard state to the old one and produces key
> + * press/release events accordingly.  The keyboard state is 13 bytes (one byte
> + * per column)
> + */
> +static void cros_ec_keyb_process(struct cros_ec_keyb *ckdev,
> +			 uint8_t *kb_state, int len)
> +{
> +	int col, row;
> +	int new_state;
> +	int num_cols;
> +
> +	num_cols = len;
> +
> +	if (ckdev->ghost_filter && cros_ec_keyb_has_ghosting(ckdev, kb_state)) {
> +		/*
> +		 * Simple-minded solution: ignore this state. The obvious
> +		 * improvement is to only ignore changes to keys involved in
> +		 * the ghosting, but process the other changes.
> +		 */
> +		dev_dbg(ckdev->dev, "ghosting found\n");
> +		return;
> +	}
> +
> +	for (col = 0; col < ckdev->cols; col++) {
> +		for (row = 0; row < ckdev->rows; row++) {
> +			int code = MATRIX_SCAN_CODE(row, col, ckdev->row_shift);
> +
> +			new_state = kb_state[col] & (1 << row);
> +			if (!!new_state != test_bit(code, ckdev->idev->key)) {
> +				dev_dbg(ckdev->dev,
> +					"changed: [r%d c%d]: byte %02x\n",
> +					row, col, new_state);
> +
> +				input_report_key(ckdev->idev, code, new_state);
> +			}
> +		}
> +	}
> +	input_sync(ckdev->idev);
> +}
> +
> +static int cros_ec_keyb_open(struct input_dev *dev)
> +{
> +	struct cros_ec_keyb *ckdev = input_get_drvdata(dev);
> +
> +	return blocking_notifier_chain_register(&ckdev->ec->event_notifier,
> +						&ckdev->notifier);
> +}
> +
> +static void cros_ec_keyb_close(struct input_dev *dev)
> +{
> +	struct cros_ec_keyb *ckdev = input_get_drvdata(dev);
> +
> +	blocking_notifier_chain_unregister(&ckdev->ec->event_notifier,
> +					   &ckdev->notifier);
> +}
> +
> +static int cros_ec_keyb_get_state(struct cros_ec_keyb *ckdev, uint8_t *kb_state)
> +{
> +	return ckdev->ec->command_recv(ckdev->ec, EC_CMD_MKBP_STATE,
> +					  kb_state, ckdev->cols);
> +}
> +
> +static int cros_ec_keyb_work(struct notifier_block *nb,
> +		     unsigned long state, void *_notify)
> +{
> +	int ret;
> +	struct cros_ec_keyb *ckdev = container_of(nb, struct cros_ec_keyb,
> +						    notifier);
> +	uint8_t kb_state[ckdev->cols];
> +
> +	ret = cros_ec_keyb_get_state(ckdev, kb_state);
> +	if (ret >= 0)
> +		cros_ec_keyb_process(ckdev, kb_state, ret);
> +
> +	return NOTIFY_DONE;
> +}
> +
> +/* Clear any keys in the buffer */
> +static void cros_ec_keyb_clear_keyboard(struct cros_ec_keyb *ckdev)
> +{
> +	uint8_t old_state[ckdev->cols];
> +	uint8_t new_state[ckdev->cols];
> +	unsigned long duration;
> +	int i, ret;
> +
> +	/*
> +	 * Keep reading until we see that the scan state does not change.
> +	 * That indicates that we are done.
> +	 *
> +	 * Assume that the EC keyscan buffer is at most 32 deep.
> +	 */
> +	duration = jiffies;
> +	ret = cros_ec_keyb_get_state(ckdev, new_state);
> +	for (i = 1; !ret && i < 32; i++) {
> +		memcpy(old_state, new_state, sizeof(old_state));
> +		ret = cros_ec_keyb_get_state(ckdev, new_state);
> +		if (0 == memcmp(old_state, new_state, sizeof(old_state)))
> +			break;
> +	}
> +	duration = jiffies - duration;
> +	dev_info(ckdev->dev, "Discarded %d keyscan(s) in %dus\n", i,
> +		jiffies_to_usecs(duration));
> +}
> +
> +static int cros_ec_keyb_probe(struct platform_device *pdev)
> +{
> +	struct cros_ec_device *ec = dev_get_drvdata(pdev->dev.parent);
> +	struct device *dev = ec->dev;
> +	struct cros_ec_keyb *ckdev = NULL;
> +	struct input_dev *idev = NULL;
> +	struct device_node *np;
> +	int err;
> +
> +	np = pdev->dev.of_node;
> +	if (!np)
> +		return -ENODEV;
> +
> +	ckdev = kzalloc(sizeof(*ckdev), GFP_KERNEL);
> +	if (!ckdev) {
> +		dev_err(dev, "cannot allocate memory for ckdev\n");
> +		return -ENOMEM;
> +	}
> +	err = matrix_keypad_parse_of_params(&pdev->dev, &ckdev->rows,
> +					    &ckdev->cols);
> +	if (err)
> +		goto fail_alloc_dev;
> +
> +	idev = input_allocate_device();
> +	if (!idev) {
> +		err = -ENOMEM;
> +		dev_err(dev, "cannot allocate memory for input device\n");
> +		goto fail_alloc_dev;
> +	}
> +
> +	ckdev->ec = ec;
> +	ckdev->notifier.notifier_call = cros_ec_keyb_work;
> +	ckdev->dev = dev;
> +	dev_set_drvdata(&pdev->dev, ckdev);
> +
> +	idev->name = ec->get_name(ec);
> +	idev->phys = ec->get_phys_name(ec);
> +	__set_bit(EV_REP, idev->evbit);
> +
> +	idev->id.bustype = BUS_VIRTUAL;
> +	idev->id.version = 1;
> +	idev->id.product = 0;
> +	idev->dev.parent = &pdev->dev;
> +	idev->open = cros_ec_keyb_open;
> +	idev->close = cros_ec_keyb_close;
> +
> +	ckdev->ghost_filter = of_property_read_bool(np,
> +					"google,needs-ghost-filter");
> +
> +	err = matrix_keypad_build_keymap(NULL, NULL, ckdev->rows, ckdev->cols,
> +					 NULL, idev);
> +	if (err) {
> +		dev_err(dev, "cannot build key matrix\n");
> +		goto fail_matrix;
> +	}
> +
> +	ckdev->row_shift = get_count_order(ckdev->cols);
> +
> +	input_set_capability(idev, EV_MSC, MSC_SCAN);
> +	input_set_drvdata(idev, ckdev);
> +	ckdev->idev = idev;
> +	err = input_register_device(ckdev->idev);
> +	if (err) {
> +		dev_err(dev, "cannot register input device\n");
> +		goto fail_register;
> +	}
> +
> +	return 0;
> +
> +fail_register:
> +	kfree(idev->keycode);
> +fail_matrix:
> +	input_free_device(idev);
> +fail_alloc_dev:
> +	kfree(ckdev);
> +	return err;
> +}
> +
> +static int cros_ec_keyb_remove(struct platform_device *pdev)
> +{
> +	struct cros_ec_keyb *ckdev = platform_get_drvdata(pdev);
> +	struct input_dev *idev = ckdev->idev;
> +
> +	input_unregister_device(idev);
> +	kfree(ckdev);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int cros_ec_keyb_resume(struct device *dev)
> +{
> +	struct cros_ec_keyb *ckdev = dev_get_drvdata(dev);
> +
> +	/*
> +	 * When the EC is not a wake source, then it could not have caused the
> +	 * resume, so we clear the EC's key scan buffer. If the EC was a
> +	 * wake source (e.g. the lid is open and the user might press a key to
> +	 * wake) then the key scan buffer should be preserved.
> +	 */
> +	if (ckdev->ec->was_wake_device)
> +		cros_ec_keyb_clear_keyboard(ckdev);
> +
> +	return 0;
> +}
> +
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(cros_ec_keyb_pm_ops, NULL, cros_ec_keyb_resume);
> +
> +static struct platform_driver cros_ec_keyb_driver = {
> +	.probe = cros_ec_keyb_probe,
> +	.remove = cros_ec_keyb_remove,
> +	.driver = {
> +		.name = "cros-ec-keyb",
> +		.pm	= &cros_ec_keyb_pm_ops,
> +	},
> +};
> +
> +module_platform_driver(cros_ec_keyb_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("ChromeOS EC keyboard driver");
> +MODULE_ALIAS("platform:cros-ec-keyb");


--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 6/6] Input: Add ChromeOS EC keyboard driver

[Simon Glass]

Hi,

On Tue, Feb 19, 2013 at 12:36 AM, li guang <lig.fnst@cn.fujitsu.com> wrote:
> 在 2013-02-15五的 20:16 -0800，Simon Glass写道：
>> Use the key-matrix layer to interpret key scan information from the EC
>> and inject input based on the FDT-supplied key map. This driver registers
>> itself with the ChromeOS EC driver to perform communications.
>
> [snip ...]
>> +/*
>> + * Returns true when there is at least one combination of pressed keys that
>> + * results in ghosting.
>> + */
>> +static bool cros_ec_keyb_has_ghosting(struct cros_ec_keyb *ckdev, uint8_t *buf)
>> +{
>> +     int row;
>> +
>> +     /*
>> +      * Ghosting happens if for any pressed key X there are other keys
>> +      * pressed both in the same row and column of X as, for instance,
>> +      * in the following diagram:
>> +      *
>> +      * . . Y . g .
>> +      * . . . . . .
>> +      * . . . . . .
>> +      * . . X . Z .
>> +      *
>> +      * In this case only X, Y, and Z are pressed, but g appears to be
>> +      * pressed too (see Wikipedia).
>> +      *
>> +      * We can detect ghosting in a single pass (*) over the keyboard state
>> +      * by maintaining two arrays.  pressed_in_row counts how many pressed
>> +      * keys we have found in a row.  row_has_teeth is true if any of the
>> +      * pressed keys for this row has other pressed keys in its column.  If
>> +      * at any point of the scan we find that a row has multiple pressed
>> +      * keys, and at least one of them is at the intersection with a column
>> +      * with multiple pressed keys, we're sure there is ghosting.
>> +      * Conversely, if there is ghosting, we will detect such situation for
>> +      * at least one key during the pass.
>> +      *
>> +      * (*) This looks linear in the number of keys, but it's not.  We can
>> +      * cheat because the number of rows is small.
>> +      */
>> +     for (row = 0; row < ckdev->rows; row++) {
>> +             if (cros_ec_keyb_row_has_ghosting(ckdev, buf, row))
>> +                     return true;
>> +     }
>> +
>> +     return false;
>> +}
>
> are you sure your EC's firmware did not do ghost-key detection?
> or, did you test ghost-key with/without your own ghost-key detection?
> as far as I know, ghost-key should be take care either by keyboard
> designer or firmware.
>

Yes, the matrix scans are sent from the EC in a raw form - in fact the
EC on snow does not even know the keycode map. The EC does handle
debouncing though. The idea is to reduce code/complexity in the EC
where we are space-constrained.

[snip]

Regards,
Simon