[PATCH v3] i2c-hid: introduce HID over i2c specification implementation

[PATCH v3] i2c-hid: introduce HID over i2c specification implementation

[Benjamin Tissoires]

Microsoft published the protocol specification of HID over i2c:
http://msdn.microsoft.com/en-us/library/windows/hardware/hh852380.aspx

This patch introduces an implementation of this protocol.

This implementation does not includes the ACPI part of the specification.
This will come when ACPI 5.0 devices enumeration will be available.

Once the ACPI part is done, OEM will not have to declare HID over I2C
devices in their platform specific driver.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---

Hi Guys,

here is the v3 of the HID over I2C driver.

Changes in v3:
* fix i2c_hid_alloc_buffers when allocation failed
* fix error messages to be more "human"
* fix i2c_hid_set_report as it didn't followed the protocol
* remove i2c_hid_write_out_report (hid-core doesn't present the callback that
could use this feature of the protocol)
* reject HID_OUTPUT_REPORT in i2c_hid_get_raw_report
* fix output report id in i2c_hid_output_raw_report and reject HID_INPUT_REPORT

Changes in v2:
* s/i2chid/i2c_hid/
* remove i2c_hid_print_buffer function and use "%*ph" instead
* define i2c_hid_dbg macro
* remove retries in __i2c_hid_command
* fix return values
* fix imports (I hope)
* drop hiddev and pidff_init support as the used functions were usb only
* make memory allocation more static
* remove spinlock
* don't use an array for the commands, but static const declarations instead
* move i2c-hid into drivers/hid
* remove HID_MAX_BUFFER_SIZE test
* implement missing output reports sending (not tested)
* typos, spacing, and many small thinks that appeared in the previous review

Cheers,
Benjamin

 drivers/hid/Kconfig           |   2 +
 drivers/hid/Makefile          |   1 +
 drivers/hid/i2c-hid/Kconfig   |  21 +
 drivers/hid/i2c-hid/Makefile  |   5 +
 drivers/hid/i2c-hid/i2c-hid.c | 974 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/i2c/i2c-hid.h   |  35 ++
 6 files changed, 1038 insertions(+)
 create mode 100644 drivers/hid/i2c-hid/Kconfig
 create mode 100644 drivers/hid/i2c-hid/Makefile
 create mode 100644 drivers/hid/i2c-hid/i2c-hid.c
 create mode 100644 include/linux/i2c/i2c-hid.h

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index f0ac277..e7d6a13 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -737,4 +737,6 @@ endif # HID
 
 source "drivers/hid/usbhid/Kconfig"
 
+source "drivers/hid/i2c-hid/Kconfig"
+
 endmenu
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 78eae69..b622157 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -119,3 +119,4 @@ obj-$(CONFIG_USB_HID)		+= usbhid/
 obj-$(CONFIG_USB_MOUSE)		+= usbhid/
 obj-$(CONFIG_USB_KBD)		+= usbhid/
 
+obj-$(CONFIG_I2C_HID)		+= i2c-hid/
diff --git a/drivers/hid/i2c-hid/Kconfig b/drivers/hid/i2c-hid/Kconfig
new file mode 100644
index 0000000..5b7d4d8
--- /dev/null
+++ b/drivers/hid/i2c-hid/Kconfig
@@ -0,0 +1,21 @@
+menu "I2C HID support"
+	depends on I2C
+
+config I2C_HID
+	tristate "HID over I2C transport layer"
+	default n
+	depends on I2C && INPUT
+	select HID
+	---help---
+	  Say Y here if you want to use the HID over i2c protocol
+	  implementation.
+
+	  If unsure, say N.
+
+	  This support is also available as a module.  If so, the module
+	  will be called i2c-hid.
+
+comment "Input core support is needed for HID over I2C input layer"
+	depends on I2C_HID && INPUT=n
+
+endmenu
diff --git a/drivers/hid/i2c-hid/Makefile b/drivers/hid/i2c-hid/Makefile
new file mode 100644
index 0000000..832d8f9
--- /dev/null
+++ b/drivers/hid/i2c-hid/Makefile
@@ -0,0 +1,5 @@
+#
+# Makefile for the I2C input drivers
+#
+
+obj-$(CONFIG_I2C_HID)				+= i2c-hid.o
diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
new file mode 100644
index 0000000..11140bd
--- /dev/null
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -0,0 +1,974 @@
+/*
+ * HID over I2C protocol implementation
+ *
+ * Copyright (c) 2012 Benjamin Tissoires <benjamin.tissoires@gmail.com>
+ * Copyright (c) 2012 Ecole Nationale de l'Aviation Civile, France
+ * Copyright (c) 2012 Red Hat, Inc
+ *
+ * This code is partly based on "USB HID support for Linux":
+ *
+ *  Copyright (c) 1999 Andreas Gal
+ *  Copyright (c) 2000-2005 Vojtech Pavlik <vojtech@suse.cz>
+ *  Copyright (c) 2005 Michael Haboustak <mike-@cinci.rr.com> for Concept2, Inc
+ *  Copyright (c) 2007-2008 Oliver Neukum
+ *  Copyright (c) 2006-2010 Jiri Kosina
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file COPYING in the main directory of this archive for
+ * more details.
+ */
+
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/input.h>
+#include <linux/delay.h>
+#include <linux/slab.h>
+#include <linux/pm.h>
+#include <linux/device.h>
+#include <linux/wait.h>
+#include <linux/err.h>
+#include <linux/string.h>
+#include <linux/list.h>
+#include <linux/jiffies.h>
+#include <linux/kernel.h>
+#include <linux/bug.h>
+#include <linux/hid.h>
+
+#include <linux/i2c/i2c-hid.h>
+
+/* flags */
+#define I2C_HID_STARTED		(1 << 0)
+#define I2C_HID_RESET_PENDING	(1 << 1)
+#define I2C_HID_READ_PENDING	(1 << 2)
+
+#define I2C_HID_PWR_ON		0x00
+#define I2C_HID_PWR_SLEEP	0x01
+
+/* debug option */
+static bool debug = false;
+module_param(debug, bool, 0444);
+MODULE_PARM_DESC(debug, "print a lot of debug information");
+
+#define i2c_hid_dbg(ihid, fmt, arg...)	\
+	if (debug)			\
+		dev_printk(KERN_DEBUG, &(ihid)->client->dev, fmt, ##arg)
+
+struct i2c_hid_desc {
+	__le16 wHIDDescLength;
+	__le16 bcdVersion;
+	__le16 wReportDescLength;
+	__le16 wReportDescRegister;
+	__le16 wInputRegister;
+	__le16 wMaxInputLength;
+	__le16 wOutputRegister;
+	__le16 wMaxOutputLength;
+	__le16 wCommandRegister;
+	__le16 wDataRegister;
+	__le16 wVendorID;
+	__le16 wProductID;
+	__le16 wVersionID;
+} __packed;
+
+struct i2c_hid_cmd {
+	unsigned int registerIndex;
+	__u8 opcode;
+	unsigned int length;
+	bool wait;
+};
+
+union command {
+	u8 data[0];
+	struct cmd {
+		__le16 reg;
+		__u8 reportTypeID;
+		__u8 opcode;
+	} __packed c;
+};
+
+#define I2C_HID_CMD(opcode_) \
+	.opcode = opcode_, .length = 4, \
+	.registerIndex = offsetof(struct i2c_hid_desc, wCommandRegister)
+
+/* fetch HID descriptor */
+static const struct i2c_hid_cmd hid_descr_cmd = { .length = 2 };
+/* fetch report descriptors */
+static const struct i2c_hid_cmd hid_report_descr_cmd = {
+		.registerIndex = offsetof(struct i2c_hid_desc,
+			wReportDescRegister),
+		.opcode = 0x00,
+		.length = 2 };
+/* commands */
+static const struct i2c_hid_cmd hid_reset_cmd =		{ I2C_HID_CMD(0x01),
+							  .wait = true };
+static const struct i2c_hid_cmd hid_get_report_cmd =	{ I2C_HID_CMD(0x02) };
+static const struct i2c_hid_cmd hid_set_report_cmd =	{ I2C_HID_CMD(0x03) };
+static const struct i2c_hid_cmd hid_get_idle_cmd =	{ I2C_HID_CMD(0x04) };
+static const struct i2c_hid_cmd hid_set_idle_cmd =	{ I2C_HID_CMD(0x05) };
+static const struct i2c_hid_cmd hid_get_protocol_cmd =	{ I2C_HID_CMD(0x06) };
+static const struct i2c_hid_cmd hid_set_protocol_cmd =	{ I2C_HID_CMD(0x07) };
+static const struct i2c_hid_cmd hid_set_power_cmd =	{ I2C_HID_CMD(0x08) };
+/* read/write data register */
+static const struct i2c_hid_cmd hid_data_cmd = {
+		.registerIndex = offsetof(struct i2c_hid_desc, wDataRegister),
+		.opcode = 0x00,
+		.length = 2 };
+/* write output reports */
+static const struct i2c_hid_cmd hid_out_cmd = {
+		.registerIndex = offsetof(struct i2c_hid_desc,
+			wOutputRegister),
+		.opcode = 0x00,
+		.length = 2 };
+
+/* The main device structure */
+struct i2c_hid {
+	struct i2c_client	*client;	/* i2c client */
+	struct hid_device	*hid;	/* pointer to corresponding HID dev */
+	union {
+		__u8 hdesc_buffer[sizeof(struct i2c_hid_desc)];
+		struct i2c_hid_desc hdesc;	/* the HID Descriptor */
+	};
+	__le16			wHIDDescRegister; /* location of the i2c
+						   * register of the HID
+						   * descriptor. */
+	unsigned int		bufsize;	/* i2c buffer size */
+	char			*inbuf;		/* Input buffer */
+	char			*cmdbuf;	/* Command buffer */
+	char			*argsbuf;	/* Command arguments buffer */
+
+	unsigned long		flags;		/* device flags */
+
+	int			irq;		/* the interrupt line irq */
+
+	wait_queue_head_t	wait;		/* For waiting the interrupt */
+};
+
+static int __i2c_hid_command(struct i2c_client *client,
+		const struct i2c_hid_cmd *command, u8 reportID,
+		u8 reportType, u8 *args, int args_len,
+		unsigned char *buf_recv, int data_len)
+{
+	struct i2c_hid *ihid = i2c_get_clientdata(client);
+	union command *cmd = (union command *)ihid->cmdbuf;
+	int ret;
+	struct i2c_msg msg[2];
+	int msg_num = 1;
+
+	int length = command->length;
+	bool wait = command->wait;
+	unsigned int registerIndex = command->registerIndex;
+
+	/* special case for hid_descr_cmd */
+	if (command == &hid_descr_cmd) {
+		cmd->c.reg = ihid->wHIDDescRegister;
+	} else {
+		cmd->data[0] = ihid->hdesc_buffer[registerIndex];
+		cmd->data[1] = ihid->hdesc_buffer[registerIndex + 1];
+	}
+
+	if (length > 2) {
+		cmd->c.opcode = command->opcode;
+		cmd->c.reportTypeID = reportID | reportType << 4;
+	}
+
+	memcpy(cmd->data + length, args, args_len);
+	length += args_len;
+
+	i2c_hid_dbg(ihid, "%s: cmd=%*ph\n", __func__, length, cmd->data);
+
+	msg[0].addr = client->addr;
+	msg[0].flags = client->flags & I2C_M_TEN;
+	msg[0].len = length;
+	msg[0].buf = cmd->data;
+	if (data_len > 0) {
+		msg[1].addr = client->addr;
+		msg[1].flags = client->flags & I2C_M_TEN;
+		msg[1].flags |= I2C_M_RD;
+		msg[1].len = data_len;
+		msg[1].buf = buf_recv;
+		msg_num = 2;
+		set_bit(I2C_HID_READ_PENDING, &ihid->flags);
+	}
+
+	if (wait)
+		set_bit(I2C_HID_RESET_PENDING, &ihid->flags);
+
+	ret = i2c_transfer(client->adapter, msg, msg_num);
+
+	if (data_len > 0)
+		clear_bit(I2C_HID_READ_PENDING, &ihid->flags);
+
+	if (ret != msg_num)
+		return ret < 0 ? ret : -EIO;
+
+	ret = 0;
+
+	if (wait) {
+		i2c_hid_dbg(ihid, "%s: waiting...\n", __func__);
+		if (!wait_event_timeout(ihid->wait,
+				!test_bit(I2C_HID_RESET_PENDING, &ihid->flags),
+				msecs_to_jiffies(5000)))
+			ret = -ENODATA;
+		i2c_hid_dbg(ihid, "%s: finished.\n", __func__);
+	}
+
+	return ret;
+}
+
+static int i2c_hid_command(struct i2c_client *client,
+		const struct i2c_hid_cmd *command,
+		unsigned char *buf_recv, int data_len)
+{
+	return __i2c_hid_command(client, command, 0, 0, NULL, 0,
+				buf_recv, data_len);
+}
+
+static int i2c_hid_get_report(struct i2c_client *client, u8 reportType,
+		u8 reportID, unsigned char *buf_recv, int data_len)
+{
+	struct i2c_hid *ihid = i2c_get_clientdata(client);
+	u8 args[3];
+	int ret;
+	int args_len = 0;
+	u16 readRegister = le16_to_cpu(ihid->hdesc.wDataRegister);
+
+	i2c_hid_dbg(ihid, "%s\n", __func__);
+
+	if (reportID >= 0x0F) {
+		args[args_len++] = reportID;
+		reportID = 0x0F;
+	}
+
+	args[args_len++] = readRegister & 0xFF;
+	args[args_len++] = readRegister >> 8;
+
+	ret = __i2c_hid_command(client, &hid_get_report_cmd, reportID,
+		reportType, args, args_len, buf_recv, data_len);
+	if (ret) {
+		dev_err(&client->dev,
+			"failed to retrieve report from device.\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int i2c_hid_set_report(struct i2c_client *client, u8 reportType,
+		u8 reportID, unsigned char *buf, size_t data_len)
+{
+	struct i2c_hid *ihid = i2c_get_clientdata(client);
+	u8 *args = ihid->argsbuf;
+	int ret;
+	u16 dataRegister = le16_to_cpu(ihid->hdesc.wDataRegister);
+
+	/* hidraw already checked that data_len < HID_MAX_BUFFER_SIZE */
+	u16 size =	2			/* size */ +
+			(reportID ? 1 : 0)	/* reportID */ +
+			data_len		/* buf */;
+	int args_len =	(reportID >= 0x0F ? 1 : 0) /* optional third byte */ +
+			2			/* dataRegister */ +
+			size			/* args */;
+	int index = 0;
+
+	i2c_hid_dbg(ihid, "%s\n", __func__);
+
+	if (reportID >= 0x0F) {
+		args[index++] = reportID;
+		reportID = 0x0F;
+	}
+
+	args[index++] = dataRegister & 0xFF;
+	args[index++] = dataRegister >> 8;
+
+	args[index++] = size & 0xFF;
+	args[index++] = size >> 8;
+
+	if (reportID)
+		args[index++] = reportID;
+
+	memcpy(&args[index], buf, data_len);
+
+	ret = __i2c_hid_command(client, &hid_set_report_cmd, reportID,
+		reportType, args, args_len, NULL, 0);
+	if (ret) {
+		dev_err(&client->dev, "failed to set a report to device.\n");
+		return -EINVAL;
+	}
+
+	return data_len;
+}
+
+static int i2c_hid_set_power(struct i2c_client *client, int power_state)
+{
+	struct i2c_hid *ihid = i2c_get_clientdata(client);
+	int ret;
+
+	i2c_hid_dbg(ihid, "%s\n", __func__);
+
+	ret = __i2c_hid_command(client, &hid_set_power_cmd, power_state,
+		0, NULL, 0, NULL, 0);
+	if (ret)
+		dev_err(&client->dev, "failed to change power setting.\n");
+
+	return ret;
+}
+
+static int i2c_hid_hwreset(struct i2c_client *client)
+{
+	struct i2c_hid *ihid = i2c_get_clientdata(client);
+	int ret;
+
+	i2c_hid_dbg(ihid, "%s\n", __func__);
+
+	ret = i2c_hid_set_power(client, I2C_HID_PWR_ON);
+	if (ret)
+		return ret;
+
+	i2c_hid_dbg(ihid, "resetting...\n");
+
+	ret = i2c_hid_command(client, &hid_reset_cmd, NULL, 0);
+	if (ret) {
+		dev_err(&client->dev, "failed to reset device.\n");
+		i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int i2c_hid_get_input(struct i2c_hid *ihid)
+{
+	int ret, ret_size;
+	int size = le16_to_cpu(ihid->hdesc.wMaxInputLength);
+
+	ret = i2c_master_recv(ihid->client, ihid->inbuf, size);
+	if (ret != size) {
+		if (ret < 0)
+			return ret;
+
+		dev_err(&ihid->client->dev, "%s: got %d data instead of %d\n",
+			__func__, ret, size);
+		return ret;
+	}
+
+	ret_size = ihid->inbuf[0] | ihid->inbuf[1] << 8;
+
+	if (!ret_size) {
+		/* host or device initiated RESET completed */
+		if (test_and_clear_bit(I2C_HID_RESET_PENDING, &ihid->flags))
+			wake_up(&ihid->wait);
+		return 0;
+	}
+
+	if (ret_size > size) {
+		dev_err(&ihid->client->dev, "%s: incomplete report (%d/%d)\n",
+			__func__, size, ret_size);
+		return -EIO;
+	}
+
+	i2c_hid_dbg(ihid, "input: %*ph\n", ret_size, ihid->inbuf);
+
+	if (test_bit(I2C_HID_STARTED, &ihid->flags))
+		hid_input_report(ihid->hid, HID_INPUT_REPORT, ihid->inbuf + 2,
+				ret_size - 2, 1);
+
+	return 0;
+}
+
+static irqreturn_t i2c_hid_irq(int irq, void *dev_id)
+{
+	struct i2c_hid *ihid = dev_id;
+
+	if (test_bit(I2C_HID_READ_PENDING, &ihid->flags))
+		return IRQ_HANDLED;
+
+	i2c_hid_get_input(ihid);
+
+	return IRQ_HANDLED;
+}
+
+static int i2c_hid_get_report_length(struct hid_report *report)
+{
+	return ((report->size - 1) >> 3) + 1 +
+		report->device->report_enum[report->type].numbered + 2;
+}
+
+static void i2c_hid_init_report(struct hid_report *report, u8 *buffer,
+	size_t bufsize)
+{
+	struct hid_device *hid = report->device;
+	struct i2c_client *client = hid->driver_data;
+	struct i2c_hid *ihid = i2c_get_clientdata(client);
+	unsigned int size, ret_size;
+
+	size = i2c_hid_get_report_length(report);
+	i2c_hid_get_report(client,
+			report->type == HID_FEATURE_REPORT ? 0x03 : 0x01,
+			report->id, buffer, size);
+
+	i2c_hid_dbg(ihid, "report (len=%d): %*ph\n", size, size, ihid->inbuf);
+
+	ret_size = buffer[0] | (buffer[1] << 8);
+
+	if (ret_size != size) {
+		dev_err(&client->dev, "error in %s size:%d / ret_size:%d\n",
+			__func__, size, ret_size);
+		return;
+	}
+
+	/* hid->driver_lock is held as we are in probe function,
+	 * we just need to setup the input fields, so using
+	 * hid_report_raw_event is safe. */
+	hid_report_raw_event(hid, report->type, buffer + 2, size - 2, 1);
+}
+
+/*
+ * Initialize all reports
+ */
+static void i2c_hid_init_reports(struct hid_device *hid)
+{
+	struct hid_report *report;
+	struct i2c_client *client = hid->driver_data;
+	struct i2c_hid *ihid = i2c_get_clientdata(client);
+	u8 *inbuf = kzalloc(ihid->bufsize, GFP_KERNEL);
+
+	if (!inbuf)
+		return;
+
+	list_for_each_entry(report,
+		&hid->report_enum[HID_INPUT_REPORT].report_list, list)
+		i2c_hid_init_report(report, inbuf, ihid->bufsize);
+
+	list_for_each_entry(report,
+		&hid->report_enum[HID_FEATURE_REPORT].report_list, list)
+		i2c_hid_init_report(report, inbuf, ihid->bufsize);
+
+	kfree(inbuf);
+}
+
+/*
+ * Traverse the supplied list of reports and find the longest
+ */
+static void i2c_hid_find_max_report(struct hid_device *hid, unsigned int type,
+		unsigned int *max)
+{
+	struct hid_report *report;
+	unsigned int size;
+
+	/* We should not rely on wMaxInputLength, as some devices may set it to
+	 * a wrong length. */
+	list_for_each_entry(report, &hid->report_enum[type].report_list, list) {
+		size = i2c_hid_get_report_length(report);
+		if (*max < size)
+			*max = size;
+	}
+}
+
+static int i2c_hid_alloc_buffers(struct i2c_hid *ihid)
+{
+	/* the worst case is computed from the set_report command with a
+	 * reportID > 15 and the maximum report length */
+	int args_len = sizeof(__u8) + /* optional ReportID byte */
+		       sizeof(__u16) + /* data register */
+		       sizeof(__u16) + /* size of the report */
+		       ihid->bufsize; /* report */
+
+	ihid->inbuf = kzalloc(ihid->bufsize, GFP_KERNEL);
+
+	if (!ihid->inbuf)
+		return -ENOMEM;
+
+	ihid->argsbuf = kzalloc(args_len, GFP_KERNEL);
+
+	if (!ihid->argsbuf) {
+		kfree(ihid->inbuf);
+		return -ENOMEM;
+	}
+
+	ihid->cmdbuf = kzalloc(sizeof(union command) + args_len, GFP_KERNEL);
+
+	if (!ihid->cmdbuf) {
+		kfree(ihid->inbuf);
+		kfree(ihid->argsbuf);
+		ihid->inbuf = NULL;
+		ihid->argsbuf = NULL;
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static void i2c_hid_free_buffers(struct i2c_hid *ihid)
+{
+	kfree(ihid->inbuf);
+	kfree(ihid->argsbuf);
+	kfree(ihid->cmdbuf);
+	ihid->inbuf = NULL;
+	ihid->cmdbuf = NULL;
+	ihid->argsbuf = NULL;
+}
+
+static int i2c_hid_get_raw_report(struct hid_device *hid,
+		unsigned char report_number, __u8 *buf, size_t count,
+		unsigned char report_type)
+{
+	struct i2c_client *client = hid->driver_data;
+	struct i2c_hid *ihid = i2c_get_clientdata(client);
+	int ret;
+
+	if (report_type == HID_OUTPUT_REPORT)
+		return -EINVAL;
+
+	if (count > ihid->bufsize)
+		count = ihid->bufsize;
+
+	ret = i2c_hid_get_report(client,
+			report_type == HID_FEATURE_REPORT ? 0x03 : 0x01,
+			report_number, ihid->inbuf, count);
+
+	if (ret < 0)
+		return ret;
+
+	count = ihid->inbuf[0] | (ihid->inbuf[1] << 8);
+
+	memcpy(buf, ihid->inbuf + 2, count);
+
+	return count;
+}
+
+static int i2c_hid_output_raw_report(struct hid_device *hid, __u8 *buf,
+		size_t count, unsigned char report_type)
+{
+	struct i2c_client *client = hid->driver_data;
+	int report_id = buf[0];
+
+	if (report_type == HID_INPUT_REPORT)
+		return -EINVAL;
+
+	return i2c_hid_set_report(client,
+				report_type == HID_FEATURE_REPORT ? 0x03 : 0x02,
+				report_id, buf, count);
+}
+
+static int i2c_hid_parse(struct hid_device *hid)
+{
+	struct i2c_client *client = hid->driver_data;
+	struct i2c_hid *ihid = i2c_get_clientdata(client);
+	struct i2c_hid_desc *hdesc = &ihid->hdesc;
+	unsigned int rsize;
+	char *rdesc;
+	int ret;
+	int tries = 3;
+
+	i2c_hid_dbg(ihid, "entering %s\n", __func__);
+
+	rsize = le16_to_cpu(hdesc->wReportDescLength);
+	if (!rsize || rsize > HID_MAX_DESCRIPTOR_SIZE) {
+		dbg_hid("weird size of report descriptor (%u)\n", rsize);
+		return -EINVAL;
+	}
+
+	do {
+		ret = i2c_hid_hwreset(client);
+		if (ret)
+			msleep(1000);
+	} while (tries-- > 0 && ret);
+
+	if (ret)
+		return ret;
+
+	rdesc = kzalloc(rsize, GFP_KERNEL);
+
+	if (!rdesc) {
+		dbg_hid("couldn't allocate rdesc memory\n");
+		return -ENOMEM;
+	}
+
+	i2c_hid_dbg(ihid, "asking HID report descriptor\n");
+
+	ret = i2c_hid_command(client, &hid_report_descr_cmd, rdesc, rsize);
+	if (ret) {
+		hid_err(hid, "reading report descriptor failed\n");
+		kfree(rdesc);
+		return -EIO;
+	}
+
+	i2c_hid_dbg(ihid, "Report Descriptor: %*ph\n", rsize, rdesc);
+
+	ret = hid_parse_report(hid, rdesc, rsize);
+	kfree(rdesc);
+	if (ret) {
+		dbg_hid("parsing report descriptor failed\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int i2c_hid_start(struct hid_device *hid)
+{
+	struct i2c_client *client = hid->driver_data;
+	struct i2c_hid *ihid = i2c_get_clientdata(client);
+	int ret;
+	int old_bufsize = ihid->bufsize;
+
+	ihid->bufsize = HID_MIN_BUFFER_SIZE;
+	i2c_hid_find_max_report(hid, HID_INPUT_REPORT, &ihid->bufsize);
+	i2c_hid_find_max_report(hid, HID_OUTPUT_REPORT, &ihid->bufsize);
+	i2c_hid_find_max_report(hid, HID_FEATURE_REPORT, &ihid->bufsize);
+
+	if (ihid->bufsize > old_bufsize || !ihid->inbuf || !ihid->cmdbuf) {
+		i2c_hid_free_buffers(ihid);
+
+		ret = i2c_hid_alloc_buffers(ihid);
+
+		if (ret) {
+			ihid->bufsize = old_bufsize;
+			return ret;
+		}
+	}
+
+	if (!(hid->quirks & HID_QUIRK_NO_INIT_REPORTS))
+		i2c_hid_init_reports(hid);
+
+	return 0;
+}
+
+static void i2c_hid_stop(struct hid_device *hid)
+{
+	struct i2c_client *client = hid->driver_data;
+	struct i2c_hid *ihid = i2c_get_clientdata(client);
+
+	hid->claimed = 0;
+
+	i2c_hid_free_buffers(ihid);
+}
+
+static int i2c_hid_open(struct hid_device *hid)
+{
+	struct i2c_client *client = hid->driver_data;
+	struct i2c_hid *ihid = i2c_get_clientdata(client);
+	int ret;
+
+	if (!hid->open++) {
+		ret = i2c_hid_set_power(client, I2C_HID_PWR_ON);
+		if (ret) {
+			hid->open--;
+			return -EIO;
+		}
+		set_bit(I2C_HID_STARTED, &ihid->flags);
+	}
+	return 0;
+}
+
+static void i2c_hid_close(struct hid_device *hid)
+{
+	struct i2c_client *client = hid->driver_data;
+	struct i2c_hid *ihid = i2c_get_clientdata(client);
+
+	/* protecting hid->open to make sure we don't restart
+	 * data acquistion due to a resumption we no longer
+	 * care about
+	 */
+	if (!--hid->open) {
+		clear_bit(I2C_HID_STARTED, &ihid->flags);
+
+		/* Save some power */
+		i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
+	}
+}
+
+static int i2c_hid_power(struct hid_device *hid, int lvl)
+{
+	struct i2c_client *client = hid->driver_data;
+	struct i2c_hid *ihid = i2c_get_clientdata(client);
+	int ret = 0;
+
+	i2c_hid_dbg(ihid, "%s lvl:%d\n", __func__, lvl);
+
+	switch (lvl) {
+	case PM_HINT_FULLON:
+		ret = i2c_hid_set_power(client, I2C_HID_PWR_ON);
+		break;
+	case PM_HINT_NORMAL:
+		ret = i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
+		break;
+	}
+	return ret;
+}
+
+static int i2c_hid_hidinput_input_event(struct input_dev *dev,
+		unsigned int type, unsigned int code, int value)
+{
+	struct hid_device *hid = input_get_drvdata(dev);
+	struct hid_field *field;
+	int offset;
+
+	if (type == EV_FF)
+		return input_ff_event(dev, type, code, value);
+
+	if (type != EV_LED)
+		return -1;
+
+	offset = hidinput_find_field(hid, type, code, &field);
+
+	if (offset == -1) {
+		hid_warn(dev, "event field not found\n");
+		return -1;
+	}
+
+	hid_set_field(field, offset, value);
+
+	return 0;
+}
+
+static struct hid_ll_driver i2c_hid_ll_driver = {
+	.parse = i2c_hid_parse,
+	.start = i2c_hid_start,
+	.stop = i2c_hid_stop,
+	.open = i2c_hid_open,
+	.close = i2c_hid_close,
+	.power = i2c_hid_power,
+	.hidinput_input_event = i2c_hid_hidinput_input_event,
+};
+
+static int __devinit i2c_hid_init_irq(struct i2c_client *client)
+{
+	struct i2c_hid *ihid = i2c_get_clientdata(client);
+	int ret;
+
+	dev_dbg(&client->dev, "Requesting IRQ: %d\n", client->irq);
+
+	ret = request_threaded_irq(client->irq, NULL, i2c_hid_irq,
+			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+			client->name, ihid);
+	if (ret < 0) {
+		dev_dbg(&client->dev,
+			"Could not register for %s interrupt, irq = %d,"
+			" ret = %d\n",
+		client->name, client->irq, ret);
+
+		return ret;
+	}
+
+	ihid->irq = client->irq;
+
+	return 0;
+}
+
+static int __devinit i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
+{
+	struct i2c_client *client = ihid->client;
+	struct i2c_hid_desc *hdesc = &ihid->hdesc;
+	unsigned int dsize;
+	int ret;
+
+	/* Fetch the length of HID description, retrieve the 4 first bytes:
+	 * bytes 0-1 -> length
+	 * bytes 2-3 -> bcdVersion (has to be 1.00) */
+	ret = i2c_hid_command(client, &hid_descr_cmd, ihid->hdesc_buffer, 4);
+
+	i2c_hid_dbg(ihid, "%s, ihid->hdesc_buffer: %*ph\n",
+			__func__, 4, ihid->hdesc_buffer);
+
+	if (ret) {
+		dev_err(&client->dev, "HID_DESCR_LENGTH_CMD Fail (ret=%d)\n",
+			ret);
+		return -ENODEV;
+	}
+
+	dsize = le16_to_cpu(hdesc->wHIDDescLength);
+	if (!dsize || dsize > HID_MAX_DESCRIPTOR_SIZE) {
+		dev_err(&client->dev, "weird size of HID descriptor (%u)\n",
+			dsize);
+		return -ENODEV;
+	}
+
+	/* check bcdVersion == 1.0 */
+	if (le16_to_cpu(hdesc->bcdVersion) != 0x0100) {
+		dev_err(&client->dev,
+			"unexpected HID descriptor bcdVersion (0x%04x)\n",
+			le16_to_cpu(hdesc->bcdVersion));
+		return -ENODEV;
+	}
+
+	i2c_hid_dbg(ihid, "Fetching the HID descriptor\n");
+
+	ret = i2c_hid_command(client, &hid_descr_cmd, ihid->hdesc_buffer,
+				dsize);
+	if (ret) {
+		dev_err(&client->dev, "hid_descr_cmd Fail\n");
+		return -ENODEV;
+	}
+
+	i2c_hid_dbg(ihid, "HID Descriptor: %*ph\n", dsize, ihid->hdesc_buffer);
+
+	return 0;
+}
+
+static int __devinit i2c_hid_probe(struct i2c_client *client,
+		const struct i2c_device_id *dev_id)
+{
+	int ret;
+	struct i2c_hid *ihid;
+	struct hid_device *hid;
+	__u16 hidRegister;
+	struct i2c_hid_platform_data *platform_data = client->dev.platform_data;
+
+	dbg_hid("HID probe called for i2c 0x%02x\n", client->addr);
+
+	if (!platform_data) {
+		dev_err(&client->dev, "HID register address not provided\n");
+		return -EINVAL;
+	}
+
+	if (!client->irq) {
+		dev_err(&client->dev,
+			"HID over i2c has not been provided an Int IRQ\n");
+		return -EINVAL;
+	}
+
+	ihid = kzalloc(sizeof(struct i2c_hid), GFP_KERNEL);
+	if (!ihid)
+		return -ENOMEM;
+
+	i2c_set_clientdata(client, ihid);
+
+	ihid->client = client;
+
+	hidRegister = platform_data->hid_descriptor_address;
+	ihid->wHIDDescRegister = cpu_to_le16(hidRegister);
+
+	init_waitqueue_head(&ihid->wait);
+
+	/* we need to allocate the command buffer without knowing the maximum
+	 * size of the reports. Let's use HID_MIN_BUFFER_SIZE, then we do the
+	 * real computation later. */
+	ihid->bufsize = HID_MIN_BUFFER_SIZE;
+	i2c_hid_alloc_buffers(ihid);
+
+	ret = i2c_hid_fetch_hid_descriptor(ihid);
+	if (ret < 0)
+		goto err;
+
+	ret = i2c_hid_init_irq(client);
+	if (ret < 0)
+		goto err;
+
+	hid = hid_allocate_device();
+	if (IS_ERR(hid)) {
+		ret = PTR_ERR(hid);
+		goto err;
+	}
+
+	ihid->hid = hid;
+
+	hid->driver_data = client;
+	hid->ll_driver = &i2c_hid_ll_driver;
+	hid->hid_get_raw_report = i2c_hid_get_raw_report;
+	hid->hid_output_raw_report = i2c_hid_output_raw_report;
+	hid->dev.parent = &client->dev;
+	hid->bus = BUS_I2C;
+	hid->version = le16_to_cpu(ihid->hdesc.bcdVersion);
+	hid->vendor = le16_to_cpu(ihid->hdesc.wVendorID);
+	hid->product = le16_to_cpu(ihid->hdesc.wProductID);
+
+	snprintf(hid->name, sizeof(hid->name), "%s %04X:%04X",
+		 client->name, hid->vendor, hid->product);
+
+	ret = hid_add_device(hid);
+	if (ret) {
+		if (ret != -ENODEV)
+			hid_err(client, "can't add hid device: %d\n", ret);
+		goto err_mem_free;
+	}
+
+	return 0;
+
+err_mem_free:
+	hid_destroy_device(hid);
+
+err:
+	if (ihid->irq)
+		free_irq(ihid->irq, ihid);
+
+	kfree(ihid);
+	return ret;
+}
+
+static int __devexit i2c_hid_remove(struct i2c_client *client)
+{
+	struct i2c_hid *ihid = i2c_get_clientdata(client);
+	struct hid_device *hid;
+
+	if (WARN_ON(!ihid))
+		return -1;
+
+	hid = ihid->hid;
+	hid_destroy_device(hid);
+
+	free_irq(client->irq, ihid);
+
+	kfree(ihid);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int i2c_hid_suspend(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct i2c_hid *ihid = i2c_get_clientdata(client);
+
+	if (device_may_wakeup(&client->dev))
+		enable_irq_wake(ihid->irq);
+
+	/* Save some power */
+	i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
+
+	return 0;
+}
+
+static int i2c_hid_resume(struct device *dev)
+{
+	int ret;
+	struct i2c_client *client = to_i2c_client(dev);
+
+	ret = i2c_hid_hwreset(client);
+	if (ret)
+		return ret;
+
+	if (device_may_wakeup(&client->dev))
+		disable_irq_wake(client->irq);
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(i2c_hid_pm, i2c_hid_suspend, i2c_hid_resume);
+
+static const struct i2c_device_id i2c_hid_id_table[] = {
+	{ "i2c_hid", 0 },
+	{ },
+};
+MODULE_DEVICE_TABLE(i2c, i2c_hid_id_table);
+
+
+static struct i2c_driver i2c_hid_driver = {
+	.driver = {
+		.name	= "i2c_hid",
+		.owner	= THIS_MODULE,
+		.pm	= &i2c_hid_pm,
+	},
+
+	.probe		= i2c_hid_probe,
+	.remove		= __devexit_p(i2c_hid_remove),
+
+	.id_table	= i2c_hid_id_table,
+};
+
+module_i2c_driver(i2c_hid_driver);
+
+MODULE_DESCRIPTION("HID over I2C core driver");
+MODULE_AUTHOR("Benjamin Tissoires <benjamin.tissoires@gmail.com>");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/i2c/i2c-hid.h b/include/linux/i2c/i2c-hid.h
new file mode 100644
index 0000000..60e411d
--- /dev/null
+++ b/include/linux/i2c/i2c-hid.h
@@ -0,0 +1,35 @@
+/*
+ * HID over I2C protocol implementation
+ *
+ * Copyright (c) 2012 Benjamin Tissoires <benjamin.tissoires@gmail.com>
+ * Copyright (c) 2012 Ecole Nationale de l'Aviation Civile, France
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file COPYING in the main directory of this archive for
+ * more details.
+ */
+
+#ifndef __LINUX_I2C_HID_H
+#define __LINUX_I2C_HID_H
+
+#include <linux/types.h>
+
+/**
+ * struct i2chid_platform_data - used by hid over i2c implementation.
+ * @hid_descriptor_address: i2c register where the HID descriptor is stored.
+ *
+ * Note that it is the responsibility of the platform driver (or the acpi 5.0
+ * driver) to setup the irq related to the gpio in the struct i2c_board_info.
+ * The platform driver should also setup the gpio according to the device:
+ *
+ * A typical example is the following:
+ *	irq = gpio_to_irq(intr_gpio);
+ *	hkdk4412_i2c_devs5[0].irq = irq; // store the irq in i2c_board_info
+ *	gpio_request(intr_gpio, "elan-irq");
+ *	s3c_gpio_setpull(intr_gpio, S3C_GPIO_PULL_UP);
+ */
+struct i2c_hid_platform_data {
+	u16 hid_descriptor_address;
+};
+
+#endif /* __LINUX_I2C_HID_H */
-- 
1.8.0

Re: [PATCH v3] i2c-hid: introduce HID over i2c specification implementation

[Jiri Kosina]

On Mon, 12 Nov 2012, Benjamin Tissoires wrote:

> Microsoft published the protocol specification of HID over i2c:
> http://msdn.microsoft.com/en-us/library/windows/hardware/hh852380.aspx
> 
> This patch introduces an implementation of this protocol.
> 
> This implementation does not includes the ACPI part of the specification.
> This will come when ACPI 5.0 devices enumeration will be available.
> 
> Once the ACPI part is done, OEM will not have to declare HID over I2C
> devices in their platform specific driver.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>

Out of curiosity -- has this been tested on a real device (is there any 
such device available anyway?), or is that just the implementation of the 
defined protocol?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v3] i2c-hid: introduce HID over i2c specification implementation

[Benjamin Tissoires]

On Thu, Nov 15, 2012 at 2:51 PM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Mon, 12 Nov 2012, Benjamin Tissoires wrote:
>
>> Microsoft published the protocol specification of HID over i2c:
>> http://msdn.microsoft.com/en-us/library/windows/hardware/hh852380.aspx
>>
>> This patch introduces an implementation of this protocol.
>>
>> This implementation does not includes the ACPI part of the specification.
>> This will come when ACPI 5.0 devices enumeration will be available.
>>
>> Once the ACPI part is done, OEM will not have to declare HID over I2C
>> devices in their platform specific driver.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
>
> Out of curiosity -- has this been tested on a real device (is there any
> such device available anyway?), or is that just the implementation of the
> defined protocol?

It has been tested on an ELAN microelectronics device (a prototype),
on an odroid-x board. That's how we figure out the bug in the
set_report command.
I think we manage to test all main features of the protocol
(get_report, irqs, hid descriptor, report descriptors, set_report).

I'm currently waiting for a Synaptics touchpad to check if it's also
working with their devices.

The thing is that HID over i2c for x86 platform will presumably
require the Haswell platform from Intel (we need ACPI 5 for
enumeration), but it would be very nice to get this in the kernel just
before hardware arrive on the market :)
However, I won't be surprise if android OEMs also start using this
specification because it won't force them to write kernel drivers...

Cheers,
Benjamin

>
> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs

Re: [PATCH v3] i2c-hid: introduce HID over i2c specification implementation

[Benjamin Tissoires]

On Thu, Nov 15, 2012 at 3:04 PM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
> On Thu, Nov 15, 2012 at 2:51 PM, Jiri Kosina <jkosina@suse.cz> wrote:
>> On Mon, 12 Nov 2012, Benjamin Tissoires wrote:
>>
>>> Microsoft published the protocol specification of HID over i2c:
>>> http://msdn.microsoft.com/en-us/library/windows/hardware/hh852380.aspx
>>>
>>> This patch introduces an implementation of this protocol.
>>>
>>> This implementation does not includes the ACPI part of the specification.
>>> This will come when ACPI 5.0 devices enumeration will be available.
>>>
>>> Once the ACPI part is done, OEM will not have to declare HID over I2C
>>> devices in their platform specific driver.
>>>
>>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
>>
>> Out of curiosity -- has this been tested on a real device (is there any
>> such device available anyway?), or is that just the implementation of the
>> defined protocol?
>
> It has been tested on an ELAN microelectronics device (a prototype),
> on an odroid-x board. That's how we figure out the bug in the
> set_report command.
> I think we manage to test all main features of the protocol
> (get_report, irqs, hid descriptor, report descriptors, set_report).
>
> I'm currently waiting for a Synaptics touchpad to check if it's also
> working with their devices.
>
> The thing is that HID over i2c for x86 platform will presumably
> require the Haswell platform from Intel (we need ACPI 5 for
> enumeration), but it would be very nice to get this in the kernel just
> before hardware arrive on the market :)
> However, I won't be surprise if android OEMs also start using this
> specification because it won't force them to write kernel drivers...

And as a complement, Ramalingam tested it for Nvidia on an early
NVIDIA's Tegra reference board for PISMO which is registered at
http://www.arm.linux.org.uk/developer/machines/list.php?id=4439 , with
a HID over i2c keyboard.
He is up to test also his Synaptics touchpad.

Cheers,
Benjamin

>
> Cheers,
> Benjamin
>
>>
>> Thanks,
>>
>> --
>> Jiri Kosina
>> SUSE Labs

Re: [PATCH v3] i2c-hid: introduce HID over i2c specification implementation

[Jiri Kosina]

On Mon, 12 Nov 2012, Benjamin Tissoires wrote:

> Microsoft published the protocol specification of HID over i2c:
> http://msdn.microsoft.com/en-us/library/windows/hardware/hh852380.aspx
> 
> This patch introduces an implementation of this protocol.
> 
> This implementation does not includes the ACPI part of the specification.
> This will come when ACPI 5.0 devices enumeration will be available.
> 
> Once the ACPI part is done, OEM will not have to declare HID over I2C
> devices in their platform specific driver.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
> 
> Hi Guys,
> 
> here is the v3 of the HID over I2C driver.
> 
> Changes in v3:
> * fix i2c_hid_alloc_buffers when allocation failed
> * fix error messages to be more "human"
> * fix i2c_hid_set_report as it didn't followed the protocol
> * remove i2c_hid_write_out_report (hid-core doesn't present the callback that
> could use this feature of the protocol)
> * reject HID_OUTPUT_REPORT in i2c_hid_get_raw_report
> * fix output report id in i2c_hid_output_raw_report and reject HID_INPUT_REPORT

Benjamin,

I am now queuing this in for-3.8/i2c-hid branch, so please send any 
potential followup patches on top of that.

Thanks a lot for all your work on this so far.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v3] i2c-hid: introduce HID over i2c specification implementation

[Benjamin Tissoires]

On Mon, Nov 19, 2012 at 11:06 AM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Mon, 12 Nov 2012, Benjamin Tissoires wrote:
>
>> Microsoft published the protocol specification of HID over i2c:
>> http://msdn.microsoft.com/en-us/library/windows/hardware/hh852380.aspx
>>
>> This patch introduces an implementation of this protocol.
>>
>> This implementation does not includes the ACPI part of the specification.
>> This will come when ACPI 5.0 devices enumeration will be available.
>>
>> Once the ACPI part is done, OEM will not have to declare HID over I2C
>> devices in their platform specific driver.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
>> ---
>>
>> Hi Guys,
>>
>> here is the v3 of the HID over I2C driver.
>>
>> Changes in v3:
>> * fix i2c_hid_alloc_buffers when allocation failed
>> * fix error messages to be more "human"
>> * fix i2c_hid_set_report as it didn't followed the protocol
>> * remove i2c_hid_write_out_report (hid-core doesn't present the callback that
>> could use this feature of the protocol)
>> * reject HID_OUTPUT_REPORT in i2c_hid_get_raw_report
>> * fix output report id in i2c_hid_output_raw_report and reject HID_INPUT_REPORT
>
> Benjamin,
>
> I am now queuing this in for-3.8/i2c-hid branch, so please send any
> potential followup patches on top of that.

Ok, thanks a lot Jiri.

Cheers,
Benjamin

>
> Thanks a lot for all your work on this so far.
>
> --
> Jiri Kosina
> SUSE Labs

RE: [PATCH v3] i2c-hid: introduce HID over i2c specification implementation

[Ramalingam C]

Hi,

> -----Original Message-----
> From: Benjamin Tissoires [mailto:benjamin.tissoires@gmail.com]
> Sent: Friday, November 16, 2012 8:26 PM
> To: Jiri Kosina; Ramalingam C
> Cc: Dmitry Torokhov; Stephane Chatty; fabien.andre@gmail.com;
> scott.liu@emc.com.tw; Jean Delvare; JJ Ding; Jiri Slaby; Shubhrajyoti Datta;
> linux-i2c@vger.kernel.org; linux-input@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH v3] i2c-hid: introduce HID over i2c specification
> implementation
> 
> On Thu, Nov 15, 2012 at 3:04 PM, Benjamin Tissoires
> <benjamin.tissoires@gmail.com> wrote:
> > On Thu, Nov 15, 2012 at 2:51 PM, Jiri Kosina <jkosina@suse.cz> wrote:
> >> On Mon, 12 Nov 2012, Benjamin Tissoires wrote:
> >>
> >>> Microsoft published the protocol specification of HID over i2c:
> >>> http://msdn.microsoft.com/en-
> us/library/windows/hardware/hh852380.as
> >>> px
> >>>
> >>> This patch introduces an implementation of this protocol.
> >>>
> >>> This implementation does not includes the ACPI part of the specification.
> >>> This will come when ACPI 5.0 devices enumeration will be available.
> >>>
> >>> Once the ACPI part is done, OEM will not have to declare HID over
> >>> I2C devices in their platform specific driver.
> >>>
> >>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> >>
> >> Out of curiosity -- has this been tested on a real device (is there
> >> any such device available anyway?), or is that just the
> >> implementation of the defined protocol?
> >
> > It has been tested on an ELAN microelectronics device (a prototype),
> > on an odroid-x board. That's how we figure out the bug in the
> > set_report command.
> > I think we manage to test all main features of the protocol
> > (get_report, irqs, hid descriptor, report descriptors, set_report).
> >
> > I'm currently waiting for a Synaptics touchpad to check if it's also
> > working with their devices.
> >
> > The thing is that HID over i2c for x86 platform will presumably
> > require the Haswell platform from Intel (we need ACPI 5 for
> > enumeration), but it would be very nice to get this in the kernel just
> > before hardware arrive on the market :) However, I won't be surprise
> > if android OEMs also start using this specification because it won't
> > force them to write kernel drivers...
> 
> And as a complement, Ramalingam tested it for Nvidia on an early NVIDIA's
> Tegra reference board for PISMO which is registered at
> http://www.arm.linux.org.uk/developer/machines/list.php?id=4439 , with a
> HID over i2c keyboard.
> He is up to test also his Synaptics touchpad.

I have tested with Synaptics trackpad. It is working fine. Thank you Benjamin.

Thanks and Regards,
Ramalingam C

> 
> Cheers,
> Benjamin
> 
> >
> > Cheers,
> > Benjamin
> >
> >>
> >> Thanks,
> >>
> >> --
> >> Jiri Kosina
> >> SUSE Labs

Re: [PATCH v3] i2c-hid: introduce HID over i2c specification implementation

[Jean Delvare]

Hi Benjamin, Jiri,

Sorry for the late review. But better late than never I guess...

On Mon, 12 Nov 2012 15:42:59 +0100, Benjamin Tissoires wrote:
> Microsoft published the protocol specification of HID over i2c:
> http://msdn.microsoft.com/en-us/library/windows/hardware/hh852380.aspx
> 
> This patch introduces an implementation of this protocol.
> 
> This implementation does not includes the ACPI part of the specification.
> This will come when ACPI 5.0 devices enumeration will be available.
> 
> Once the ACPI part is done, OEM will not have to declare HID over I2C
> devices in their platform specific driver.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
> (..)
>  drivers/hid/Kconfig           |   2 +
>  drivers/hid/Makefile          |   1 +
>  drivers/hid/i2c-hid/Kconfig   |  21 +
>  drivers/hid/i2c-hid/Makefile  |   5 +
>  drivers/hid/i2c-hid/i2c-hid.c | 974 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/i2c/i2c-hid.h   |  35 ++
>  6 files changed, 1038 insertions(+)
>  create mode 100644 drivers/hid/i2c-hid/Kconfig
>  create mode 100644 drivers/hid/i2c-hid/Makefile
>  create mode 100644 drivers/hid/i2c-hid/i2c-hid.c
>  create mode 100644 include/linux/i2c/i2c-hid.h

Looks much better. I still have several random comments which you may
be interested in.

> (...)
> diff --git a/drivers/hid/i2c-hid/Kconfig b/drivers/hid/i2c-hid/Kconfig
> new file mode 100644
> index 0000000..5b7d4d8
> --- /dev/null
> +++ b/drivers/hid/i2c-hid/Kconfig
> @@ -0,0 +1,21 @@
> +menu "I2C HID support"
> +	depends on I2C
> +
> +config I2C_HID
> +	tristate "HID over I2C transport layer"
> +	default n
> +	depends on I2C && INPUT
> +	select HID
> +	---help---
> +	  Say Y here if you want to use the HID over i2c protocol
> +	  implementation.


s/i2c/I2C/

The USB equivalent lists a number of examples, maybe you could do the
same here so that the reader knows if he/she needs this driver.

> +
> +	  If unsure, say N.
> +
> +	  This support is also available as a module.  If so, the module
> +	  will be called i2c-hid.
> +
> +comment "Input core support is needed for HID over I2C input layer"
> +	depends on I2C_HID && INPUT=n

Given that the HID menu itself depends on INPUT, I fail to see how this
comment would ever be displayed. Same question holds for the USB
equivalent.

> +
> +endmenu
> (...)
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> new file mode 100644
> index 0000000..11140bd
> --- /dev/null
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> (...)
> +/* flags */
> +#define I2C_HID_STARTED		(1 << 0)
> +#define I2C_HID_RESET_PENDING	(1 << 1)
> +#define I2C_HID_READ_PENDING	(1 << 2)
> +
> +#define I2C_HID_PWR_ON		0x00
> +#define I2C_HID_PWR_SLEEP	0x01
> +
> +/* debug option */
> +static bool debug = false;

Whether you like it or not, that's still an error for checkpatch:
ERROR: do not initialise statics to 0 or NULL
#240: FILE: drivers/hid/i2c-hid/i2c-hid.c:49:
+static bool debug = false;

The rationale is that the compiler can write more efficient code if
initializations to 0 or NULL are implicit.

> +module_param(debug, bool, 0444);
> +MODULE_PARM_DESC(debug, "print a lot of debug information");
> +
> +#define i2c_hid_dbg(ihid, fmt, arg...)	\
> +	if (debug)			\
> +		dev_printk(KERN_DEBUG, &(ihid)->client->dev, fmt, ##arg)

This is considered an error by checkpatch too:
ERROR: Macros with complex values should be enclosed in parenthesis
#244: FILE: drivers/hid/i2c-hid/i2c-hid.c:53:
+#define i2c_hid_dbg(ihid, fmt, arg...)	\
+	if (debug)			\
+		dev_printk(KERN_DEBUG, &(ihid)->client->dev, fmt, ##arg)

And I wouldn't disagree, as the construct above can lead to bugs which
are hard to see... Consider the following piece of code:

	if (condition)
		i2c_hid_dbg(ihid, "Blah blah %d\n", i);
	else
		do_something_very_important();

It looks correct, however with the macro definition above, this expands
to the unexpected:

	if (condition) {
		if (debug)			\
			dev_printk(KERN_DEBUG, &ihid->client->dev,
				   "Blah blah %d\n", i);
		else
			do_something_very_important();
	}

That is, the condition to call do_something_very_important() is
condition && !debug, and not !condition as the code suggests. This is
only an example and your driver doesn't currently use any such
construct. Still I believe this should be fixed.

> (...)
> +#define I2C_HID_CMD(opcode_) \
> +	.opcode = opcode_, .length = 4, \
> +	.registerIndex = offsetof(struct i2c_hid_desc, wCommandRegister)
> +
> +/* fetch HID descriptor */
> +static const struct i2c_hid_cmd hid_descr_cmd = { .length = 2 };
> +/* fetch report descriptors */
> +static const struct i2c_hid_cmd hid_report_descr_cmd = {
> +		.registerIndex = offsetof(struct i2c_hid_desc,
> +			wReportDescRegister),
> +		.opcode = 0x00,
> +		.length = 2 };
> +/* commands */
> +static const struct i2c_hid_cmd hid_reset_cmd =		{ I2C_HID_CMD(0x01),
> +							  .wait = true };
> +static const struct i2c_hid_cmd hid_get_report_cmd =	{ I2C_HID_CMD(0x02) };
> +static const struct i2c_hid_cmd hid_set_report_cmd =	{ I2C_HID_CMD(0x03) };
> +static const struct i2c_hid_cmd hid_get_idle_cmd =	{ I2C_HID_CMD(0x04) };
> +static const struct i2c_hid_cmd hid_set_idle_cmd =	{ I2C_HID_CMD(0x05) };
> +static const struct i2c_hid_cmd hid_get_protocol_cmd =	{ I2C_HID_CMD(0x06) };
> +static const struct i2c_hid_cmd hid_set_protocol_cmd =	{ I2C_HID_CMD(0x07) };

The previous four are never used.

> +static const struct i2c_hid_cmd hid_set_power_cmd =	{ I2C_HID_CMD(0x08) };
> +/* read/write data register */
> +static const struct i2c_hid_cmd hid_data_cmd = {
> +		.registerIndex = offsetof(struct i2c_hid_desc, wDataRegister),
> +		.opcode = 0x00,
> +		.length = 2 };
> +/* write output reports */
> +static const struct i2c_hid_cmd hid_out_cmd = {
> +		.registerIndex = offsetof(struct i2c_hid_desc,
> +			wOutputRegister),
> +		.opcode = 0x00,
> +		.length = 2 };

These two are never used either.

> (...)
> +static int i2c_hid_get_report(struct i2c_client *client, u8 reportType,
> +		u8 reportID, unsigned char *buf_recv, int data_len)
> +{
> +	struct i2c_hid *ihid = i2c_get_clientdata(client);
> +	u8 args[3];
> +	int ret;
> +	int args_len = 0;
> +	u16 readRegister = le16_to_cpu(ihid->hdesc.wDataRegister);
> +
> +	i2c_hid_dbg(ihid, "%s\n", __func__);
> +
> +	if (reportID >= 0x0F) {
> +		args[args_len++] = reportID;
> +		reportID = 0x0F;
> +	}
> +
> +	args[args_len++] = readRegister & 0xFF;
> +	args[args_len++] = readRegister >> 8;
> +
> +	ret = __i2c_hid_command(client, &hid_get_report_cmd, reportID,
> +		reportType, args, args_len, buf_recv, data_len);
> +	if (ret) {
> +		dev_err(&client->dev,
> +			"failed to retrieve report from device.\n");
> +		return -EINVAL;

Why don't you pass the error value from __i2c_hid_command to the
caller? -EINVAL is for invalid input parameters, which isn't the case
here.

> +	}
> +
> +	return 0;
> +}
> +
> +static int i2c_hid_set_report(struct i2c_client *client, u8 reportType,
> +		u8 reportID, unsigned char *buf, size_t data_len)
> +{
> +	struct i2c_hid *ihid = i2c_get_clientdata(client);
> +	u8 *args = ihid->argsbuf;
> +	int ret;
> +	u16 dataRegister = le16_to_cpu(ihid->hdesc.wDataRegister);
> +
> +	/* hidraw already checked that data_len < HID_MAX_BUFFER_SIZE */
> +	u16 size =	2			/* size */ +
> +			(reportID ? 1 : 0)	/* reportID */ +
> +			data_len		/* buf */;
> +	int args_len =	(reportID >= 0x0F ? 1 : 0) /* optional third byte */ +
> +			2			/* dataRegister */ +
> +			size			/* args */;
> +	int index = 0;
> +
> +	i2c_hid_dbg(ihid, "%s\n", __func__);
> +
> +	if (reportID >= 0x0F) {
> +		args[index++] = reportID;
> +		reportID = 0x0F;
> +	}
> +
> +	args[index++] = dataRegister & 0xFF;
> +	args[index++] = dataRegister >> 8;
> +
> +	args[index++] = size & 0xFF;
> +	args[index++] = size >> 8;
> +
> +	if (reportID)
> +		args[index++] = reportID;
> +
> +	memcpy(&args[index], buf, data_len);
> +
> +	ret = __i2c_hid_command(client, &hid_set_report_cmd, reportID,
> +		reportType, args, args_len, NULL, 0);
> +	if (ret) {
> +		dev_err(&client->dev, "failed to set a report to device.\n");
> +		return -EINVAL;

Same here, returning "ret" would be more informative than hard-coding
an arbitrary error code.

> +	}
> +
> +	return data_len;
> +}
> + (...)
> +static int i2c_hid_get_input(struct i2c_hid *ihid)

The caller never checks the return value. If there is nothing useful
the caller can do with the value then you should consider making
i2c_hid_get_input return void.

> +{
> +	int ret, ret_size;
> +	int size = le16_to_cpu(ihid->hdesc.wMaxInputLength);
> +
> +	ret = i2c_master_recv(ihid->client, ihid->inbuf, size);
> +	if (ret != size) {
> +		if (ret < 0)
> +			return ret;
> +
> +		dev_err(&ihid->client->dev, "%s: got %d data instead of %d\n",
> +			__func__, ret, size);
> +		return ret;

If you don't change this function to return void, you should return
-Esomething instead of ret here, so that the caller doesn't get an
unexpected positive number.

> +	}
> +
> +	ret_size = ihid->inbuf[0] | ihid->inbuf[1] << 8;
> +
> +	if (!ret_size) {
> +		/* host or device initiated RESET completed */
> +		if (test_and_clear_bit(I2C_HID_RESET_PENDING, &ihid->flags))
> +			wake_up(&ihid->wait);
> +		return 0;
> +	}
> +
> +	if (ret_size > size) {
> +		dev_err(&ihid->client->dev, "%s: incomplete report (%d/%d)\n",
> +			__func__, size, ret_size);
> +		return -EIO;
> +	}
> +
> +	i2c_hid_dbg(ihid, "input: %*ph\n", ret_size, ihid->inbuf);
> +
> +	if (test_bit(I2C_HID_STARTED, &ihid->flags))
> +		hid_input_report(ihid->hid, HID_INPUT_REPORT, ihid->inbuf + 2,
> +				ret_size - 2, 1);
> +
> +	return 0;
> +}
> +
> +static irqreturn_t i2c_hid_irq(int irq, void *dev_id)
> +{
> +	struct i2c_hid *ihid = dev_id;
> +
> +	if (test_bit(I2C_HID_READ_PENDING, &ihid->flags))
> +		return IRQ_HANDLED;
> +
> +	i2c_hid_get_input(ihid);
> +
> +	return IRQ_HANDLED;
> +}
> + (...)
> +static void i2c_hid_init_report(struct hid_report *report, u8 *buffer,
> +	size_t bufsize)
> +{
> +	struct hid_device *hid = report->device;
> +	struct i2c_client *client = hid->driver_data;
> +	struct i2c_hid *ihid = i2c_get_clientdata(client);
> +	unsigned int size, ret_size;
> +
> +	size = i2c_hid_get_report_length(report);
> +	i2c_hid_get_report(client,
> +			report->type == HID_FEATURE_REPORT ? 0x03 : 0x01,
> +			report->id, buffer, size);

This can fail.

> +
> +	i2c_hid_dbg(ihid, "report (len=%d): %*ph\n", size, size, ihid->inbuf);
> +
> +	ret_size = buffer[0] | (buffer[1] << 8);
> +
> +	if (ret_size != size) {
> +		dev_err(&client->dev, "error in %s size:%d / ret_size:%d\n",
> +			__func__, size, ret_size);
> +		return;
> +	}
> +
> +	/* hid->driver_lock is held as we are in probe function,
> +	 * we just need to setup the input fields, so using
> +	 * hid_report_raw_event is safe. */
> +	hid_report_raw_event(hid, report->type, buffer + 2, size - 2, 1);
> +}
> +
> +/*
> + * Initialize all reports
> + */
> +static void i2c_hid_init_reports(struct hid_device *hid)
> +{
> +	struct hid_report *report;
> +	struct i2c_client *client = hid->driver_data;
> +	struct i2c_hid *ihid = i2c_get_clientdata(client);
> +	u8 *inbuf = kzalloc(ihid->bufsize, GFP_KERNEL);
> +
> +	if (!inbuf)
> +		return;

No error logged, no error reported to the caller?

> +
> +	list_for_each_entry(report,
> +		&hid->report_enum[HID_INPUT_REPORT].report_list, list)
> +		i2c_hid_init_report(report, inbuf, ihid->bufsize);
> +
> +	list_for_each_entry(report,
> +		&hid->report_enum[HID_FEATURE_REPORT].report_list, list)
> +		i2c_hid_init_report(report, inbuf, ihid->bufsize);
> +
> +	kfree(inbuf);
> +}
> (...)
> +static int i2c_hid_alloc_buffers(struct i2c_hid *ihid)
> +{
> +	/* the worst case is computed from the set_report command with a
> +	 * reportID > 15 and the maximum report length */
> +	int args_len = sizeof(__u8) + /* optional ReportID byte */
> +		       sizeof(__u16) + /* data register */
> +		       sizeof(__u16) + /* size of the report */
> +		       ihid->bufsize; /* report */
> +
> +	ihid->inbuf = kzalloc(ihid->bufsize, GFP_KERNEL);
> +
> +	if (!ihid->inbuf)
> +		return -ENOMEM;
> +
> +	ihid->argsbuf = kzalloc(args_len, GFP_KERNEL);
> +
> +	if (!ihid->argsbuf) {
> +		kfree(ihid->inbuf);

If it is important to set ihid->inbuf and ihid->argsbuf back to NULL in
the error path below, then I suppose it is equally important to set
ihid->inbuf back to NULL here. BTW, I would like to suggest an easier
and more efficient way to handle this, see below.

> +		return -ENOMEM;
> +	}
> +
> +	ihid->cmdbuf = kzalloc(sizeof(union command) + args_len, GFP_KERNEL);
> +
> +	if (!ihid->cmdbuf) {
> +		kfree(ihid->inbuf);
> +		kfree(ihid->argsbuf);
> +		ihid->inbuf = NULL;
> +		ihid->argsbuf = NULL;
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +

With proper function ordering, the above could be simplified to:

static int i2c_hid_alloc_buffers(struct i2c_hid *ihid)
{
	/* the worst case is computed from the set_report command with a
	 * reportID > 15 and the maximum report length */
	int args_len = sizeof(__u8) + /* optional ReportID byte */
		       sizeof(__u16) + /* data register */
		       sizeof(__u16) + /* size of the report */
		       ihid->bufsize; /* report */

	ihid->inbuf = kzalloc(ihid->bufsize, GFP_KERNEL);
	ihid->argsbuf = kzalloc(args_len, GFP_KERNEL);
	ihid->cmdbuf = kzalloc(sizeof(union command) + args_len, GFP_KERNEL);

	if (!ihid->inbuf || !ihid->argsbuf || !ihid->cmdbuf) {
		i2c_hid_free_buffers(hid);
		return -ENOMEM;
	}

	return 0;
}

> +static void i2c_hid_free_buffers(struct i2c_hid *ihid)
> +{
> +	kfree(ihid->inbuf);
> +	kfree(ihid->argsbuf);
> +	kfree(ihid->cmdbuf);
> +	ihid->inbuf = NULL;
> +	ihid->cmdbuf = NULL;
> +	ihid->argsbuf = NULL;
> +}
> +
> +static int i2c_hid_get_raw_report(struct hid_device *hid,
> +		unsigned char report_number, __u8 *buf, size_t count,
> +		unsigned char report_type)
> +{
> +	struct i2c_client *client = hid->driver_data;
> +	struct i2c_hid *ihid = i2c_get_clientdata(client);
> +	int ret;
> +
> +	if (report_type == HID_OUTPUT_REPORT)
> +		return -EINVAL;
> +
> +	if (count > ihid->bufsize)
> +		count = ihid->bufsize;
> +
> +	ret = i2c_hid_get_report(client,
> +			report_type == HID_FEATURE_REPORT ? 0x03 : 0x01,
> +			report_number, ihid->inbuf, count);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	count = ihid->inbuf[0] | (ihid->inbuf[1] << 8);
> +
> +	memcpy(buf, ihid->inbuf + 2, count);

What guarantee do you have that count is not larger than buf's length?
I hope you don't just count on all hardware out there being nice and
sane, do you? ;)

> +
> +	return count;
> +}
> + (...)
> +static int i2c_hid_parse(struct hid_device *hid)
> +{
> +	struct i2c_client *client = hid->driver_data;
> +	struct i2c_hid *ihid = i2c_get_clientdata(client);
> +	struct i2c_hid_desc *hdesc = &ihid->hdesc;
> +	unsigned int rsize;
> +	char *rdesc;
> +	int ret;
> +	int tries = 3;
> +
> +	i2c_hid_dbg(ihid, "entering %s\n", __func__);
> +
> +	rsize = le16_to_cpu(hdesc->wReportDescLength);
> +	if (!rsize || rsize > HID_MAX_DESCRIPTOR_SIZE) {
> +		dbg_hid("weird size of report descriptor (%u)\n", rsize);
> +		return -EINVAL;
> +	}
> +
> +	do {
> +		ret = i2c_hid_hwreset(client);
> +		if (ret)
> +			msleep(1000);
> +	} while (tries-- > 0 && ret);
> +
> +	if (ret)
> +		return ret;
> +
> +	rdesc = kzalloc(rsize, GFP_KERNEL);
> +
> +	if (!rdesc) {
> +		dbg_hid("couldn't allocate rdesc memory\n");

hid_err() would suit better IMHO.

> +		return -ENOMEM;
> +	}
> +
> +	i2c_hid_dbg(ihid, "asking HID report descriptor\n");
> +
> +	ret = i2c_hid_command(client, &hid_report_descr_cmd, rdesc, rsize);
> +	if (ret) {
> +		hid_err(hid, "reading report descriptor failed\n");
> +		kfree(rdesc);
> +		return -EIO;
> +	}
> +
> +	i2c_hid_dbg(ihid, "Report Descriptor: %*ph\n", rsize, rdesc);
> +
> +	ret = hid_parse_report(hid, rdesc, rsize);
> +	kfree(rdesc);
> +	if (ret) {
> +		dbg_hid("parsing report descriptor failed\n");

Here too, if this is an unexpected event hid_warn() or hid_err() would
be better.

> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int i2c_hid_start(struct hid_device *hid)
> +{
> +	struct i2c_client *client = hid->driver_data;
> +	struct i2c_hid *ihid = i2c_get_clientdata(client);
> +	int ret;
> +	int old_bufsize = ihid->bufsize;
> +
> +	ihid->bufsize = HID_MIN_BUFFER_SIZE;
> +	i2c_hid_find_max_report(hid, HID_INPUT_REPORT, &ihid->bufsize);
> +	i2c_hid_find_max_report(hid, HID_OUTPUT_REPORT, &ihid->bufsize);
> +	i2c_hid_find_max_report(hid, HID_FEATURE_REPORT, &ihid->bufsize);
> +
> +	if (ihid->bufsize > old_bufsize || !ihid->inbuf || !ihid->cmdbuf) {

I don't quite get this condition. As far as I can see, either all 3
buffers are allocated, or none is. A single test should tell you. If
you set ihid->bufsize to 0 on memory allocation failure, testing for
ihid->bufsize > old_bufsize should be sufficient.

> +		i2c_hid_free_buffers(ihid);
> +
> +		ret = i2c_hid_alloc_buffers(ihid);
> +
> +		if (ret) {
> +			ihid->bufsize = old_bufsize;

Not really... If we fail here, this means that all 3 buffers have been
freed, so ihid->bufsize is more like 0.

> +			return ret;
> +		}
> +	}
> +
> +	if (!(hid->quirks & HID_QUIRK_NO_INIT_REPORTS))
> +		i2c_hid_init_reports(hid);
> +
> +	return 0;
> +}
> +
> +static void i2c_hid_stop(struct hid_device *hid)
> +{
> +	struct i2c_client *client = hid->driver_data;
> +	struct i2c_hid *ihid = i2c_get_clientdata(client);
> +
> +	hid->claimed = 0;
> +
> +	i2c_hid_free_buffers(ihid);
> +}
> +
> +static int i2c_hid_open(struct hid_device *hid)
> +{
> +	struct i2c_client *client = hid->driver_data;
> +	struct i2c_hid *ihid = i2c_get_clientdata(client);
> +	int ret;
> +
> +	if (!hid->open++) {

This looks racy. The usbhid driver protects hid->open with a mutex and
I suspect you should do the same.

> +		ret = i2c_hid_set_power(client, I2C_HID_PWR_ON);
> +		if (ret) {
> +			hid->open--;
> +			return -EIO;
> +		}
> +		set_bit(I2C_HID_STARTED, &ihid->flags);
> +	}
> +	return 0;
> +}
> +
> +static void i2c_hid_close(struct hid_device *hid)
> +{
> +	struct i2c_client *client = hid->driver_data;
> +	struct i2c_hid *ihid = i2c_get_clientdata(client);
> +
> +	/* protecting hid->open to make sure we don't restart
> +	 * data acquistion due to a resumption we no longer
> +	 * care about
> +	 */
> +	if (!--hid->open) {
> +		clear_bit(I2C_HID_STARTED, &ihid->flags);
> +
> +		/* Save some power */
> +		i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
> +	}
> +}
> (...)
> +static int i2c_hid_hidinput_input_event(struct input_dev *dev,
> +		unsigned int type, unsigned int code, int value)
> +{
> +	struct hid_device *hid = input_get_drvdata(dev);
> +	struct hid_field *field;
> +	int offset;
> +
> +	if (type == EV_FF)
> +		return input_ff_event(dev, type, code, value);
> +
> +	if (type != EV_LED)
> +		return -1;
> +
> +	offset = hidinput_find_field(hid, type, code, &field);
> +
> +	if (offset == -1) {
> +		hid_warn(dev, "event field not found\n");
> +		return -1;
> +	}
> +
> +	hid_set_field(field, offset, value);
> +
> +	return 0;

hid_set_field() can fail, so maybe

	return hid_set_field(field, offset, value);

> +}
> + (...)
> +static int __devinit i2c_hid_init_irq(struct i2c_client *client)
> +{
> +	struct i2c_hid *ihid = i2c_get_clientdata(client);
> +	int ret;
> +
> +	dev_dbg(&client->dev, "Requesting IRQ: %d\n", client->irq);
> +
> +	ret = request_threaded_irq(client->irq, NULL, i2c_hid_irq,
> +			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +			client->name, ihid);
> +	if (ret < 0) {
> +		dev_dbg(&client->dev,
> +			"Could not register for %s interrupt, irq = %d,"
> +			" ret = %d\n",
> +		client->name, client->irq, ret);

dev_warn()? Indentation is broken too.

> +
> +		return ret;
> +	}
> +
> +	ihid->irq = client->irq;

I don't think you need this. Everywhere you use ihid->irq, you have
client at hand so you could as well use client->irq. This would avoid
inconsistencies like free_irq(ihid->irq, ihid) in probing error path
vs. free_irq(client->irq, ihid) in removal path, or
enable_irq_wake(ihid->irq) vs. disable_irq_wake(client->irq) in
suspend/resume.

> +
> +	return 0;
> +}
> +
> +static int __devinit i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
> +{
> +	struct i2c_client *client = ihid->client;
> +	struct i2c_hid_desc *hdesc = &ihid->hdesc;
> +	unsigned int dsize;
> +	int ret;
> +
> +	/* Fetch the length of HID description, retrieve the 4 first bytes:
> +	 * bytes 0-1 -> length
> +	 * bytes 2-3 -> bcdVersion (has to be 1.00) */
> +	ret = i2c_hid_command(client, &hid_descr_cmd, ihid->hdesc_buffer, 4);
> +
> +	i2c_hid_dbg(ihid, "%s, ihid->hdesc_buffer: %*ph\n",
> +			__func__, 4, ihid->hdesc_buffer);
> +
> +	if (ret) {
> +		dev_err(&client->dev, "HID_DESCR_LENGTH_CMD Fail (ret=%d)\n",
> +			ret);

This message is a little confusing as HID_DESCR_LENGTH_CMD no longer
exists.

> +		return -ENODEV;
> +	}
> +
> +	dsize = le16_to_cpu(hdesc->wHIDDescLength);
> +	if (!dsize || dsize > HID_MAX_DESCRIPTOR_SIZE) {
> +		dev_err(&client->dev, "weird size of HID descriptor (%u)\n",
> +			dsize);
> +		return -ENODEV;
> +	}
> +
> +	/* check bcdVersion == 1.0 */
> +	if (le16_to_cpu(hdesc->bcdVersion) != 0x0100) {
> +		dev_err(&client->dev,
> +			"unexpected HID descriptor bcdVersion (0x%04x)\n",

Should be 0x%04hx.

> +			le16_to_cpu(hdesc->bcdVersion));
> +		return -ENODEV;
> +	}
> +
> +	i2c_hid_dbg(ihid, "Fetching the HID descriptor\n");
> +
> +	ret = i2c_hid_command(client, &hid_descr_cmd, ihid->hdesc_buffer,
> +				dsize);
> +	if (ret) {
> +		dev_err(&client->dev, "hid_descr_cmd Fail\n");
> +		return -ENODEV;
> +	}
> +
> +	i2c_hid_dbg(ihid, "HID Descriptor: %*ph\n", dsize, ihid->hdesc_buffer);
> +
> +	return 0;
> +}
> +
> +static int __devinit i2c_hid_probe(struct i2c_client *client,
> +		const struct i2c_device_id *dev_id)
> +{
> +	int ret;
> +	struct i2c_hid *ihid;
> +	struct hid_device *hid;
> +	__u16 hidRegister;
> +	struct i2c_hid_platform_data *platform_data = client->dev.platform_data;
> +
> +	dbg_hid("HID probe called for i2c 0x%02x\n", client->addr);
> +
> +	if (!platform_data) {
> +		dev_err(&client->dev, "HID register address not provided\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!client->irq) {
> +		dev_err(&client->dev,
> +			"HID over i2c has not been provided an Int IRQ\n");
> +		return -EINVAL;
> +	}
> +
> +	ihid = kzalloc(sizeof(struct i2c_hid), GFP_KERNEL);
> +	if (!ihid)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(client, ihid);
> +
> +	ihid->client = client;
> +
> +	hidRegister = platform_data->hid_descriptor_address;
> +	ihid->wHIDDescRegister = cpu_to_le16(hidRegister);
> +
> +	init_waitqueue_head(&ihid->wait);
> +
> +	/* we need to allocate the command buffer without knowing the maximum
> +	 * size of the reports. Let's use HID_MIN_BUFFER_SIZE, then we do the
> +	 * real computation later. */
> +	ihid->bufsize = HID_MIN_BUFFER_SIZE;
> +	i2c_hid_alloc_buffers(ihid);

This could fail, yet you don't check for an error.

> +
> +	ret = i2c_hid_fetch_hid_descriptor(ihid);
> +	if (ret < 0)
> +		goto err;
> +
> +	ret = i2c_hid_init_irq(client);
> +	if (ret < 0)
> +		goto err;
> +
> +	hid = hid_allocate_device();
> +	if (IS_ERR(hid)) {
> +		ret = PTR_ERR(hid);
> +		goto err;
> +	}
> +
> +	ihid->hid = hid;
> +
> +	hid->driver_data = client;
> +	hid->ll_driver = &i2c_hid_ll_driver;
> +	hid->hid_get_raw_report = i2c_hid_get_raw_report;
> +	hid->hid_output_raw_report = i2c_hid_output_raw_report;
> +	hid->dev.parent = &client->dev;
> +	hid->bus = BUS_I2C;
> +	hid->version = le16_to_cpu(ihid->hdesc.bcdVersion);
> +	hid->vendor = le16_to_cpu(ihid->hdesc.wVendorID);
> +	hid->product = le16_to_cpu(ihid->hdesc.wProductID);
> +
> +	snprintf(hid->name, sizeof(hid->name), "%s %04X:%04X",

Should be %04hX:%04hX.

> +		 client->name, hid->vendor, hid->product);
> +
> +	ret = hid_add_device(hid);
> +	if (ret) {
> +		if (ret != -ENODEV)
> +			hid_err(client, "can't add hid device: %d\n", ret);
> +		goto err_mem_free;
> +	}
> +
> +	return 0;
> +
> +err_mem_free:
> +	hid_destroy_device(hid);
> +
> +err:
> +	if (ihid->irq)
> +		free_irq(ihid->irq, ihid);
> +
> +	kfree(ihid);
> +	return ret;
> +}
> +
> +static int __devexit i2c_hid_remove(struct i2c_client *client)
> +{
> +	struct i2c_hid *ihid = i2c_get_clientdata(client);
> +	struct hid_device *hid;
> +
> +	if (WARN_ON(!ihid))
> +		return -1;

This just can't happen...?

> +
> +	hid = ihid->hid;
> +	hid_destroy_device(hid);
> +
> +	free_irq(client->irq, ihid);
> +

Is there any guarantee that i2c_hid_stop() has been called before
i2c_hid_remove() is? If not, you're missing a call to
i2c_hid_free_buffers() here. BTW I'm not quite sure why
i2c_hid_remove() frees the buffers in the first place, but then again I
don't know a thing about the HID infrastructure.

> +	kfree(ihid);
> +
> +	return 0;
> +}
> +(...)
> +static const struct i2c_device_id i2c_hid_id_table[] = {
> +	{ "i2c_hid", 0 },

I am just realizing "i2c_hid" is a little redundant as an i2c device
name. Can we make this just "hid"?

> +	{ },
> +};
> +MODULE_DEVICE_TABLE(i2c, i2c_hid_id_table);

-- 
Jean Delvare

Re: [PATCH v3] i2c-hid: introduce HID over i2c specification implementation

[Benjamin Tissoires]

Hi Jean,

On Fri, Nov 30, 2012 at 3:56 PM, Jean Delvare <khali@linux-fr.org> wrote:
> Hi Benjamin, Jiri,
>
> Sorry for the late review. But better late than never I guess...

Sure! Thanks for the review. As the driver is already in Jiri's tree,
I'll do small incremental patches based on this release.

I'll try to address all of your comments.

I have a few answers to some of your remarks (I fully agree with all
of the others):

>
> On Mon, 12 Nov 2012 15:42:59 +0100, Benjamin Tissoires wrote:
>> Microsoft published the protocol specification of HID over i2c:
>> http://msdn.microsoft.com/en-us/library/windows/hardware/hh852380.aspx
>>
>> This patch introduces an implementation of this protocol.
>>
>> This implementation does not includes the ACPI part of the specification.
>> This will come when ACPI 5.0 devices enumeration will be available.
>>
>> Once the ACPI part is done, OEM will not have to declare HID over I2C
>> devices in their platform specific driver.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
>> ---
>> (..)
>>  drivers/hid/Kconfig           |   2 +
>>  drivers/hid/Makefile          |   1 +
>>  drivers/hid/i2c-hid/Kconfig   |  21 +
>>  drivers/hid/i2c-hid/Makefile  |   5 +
>>  drivers/hid/i2c-hid/i2c-hid.c | 974 ++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/i2c/i2c-hid.h   |  35 ++
>>  6 files changed, 1038 insertions(+)
>>  create mode 100644 drivers/hid/i2c-hid/Kconfig
>>  create mode 100644 drivers/hid/i2c-hid/Makefile
>>  create mode 100644 drivers/hid/i2c-hid/i2c-hid.c
>>  create mode 100644 include/linux/i2c/i2c-hid.h
>
> Looks much better. I still have several random comments which you may
> be interested in.
>
> (...)
>> +/* debug option */
>> +static bool debug = false;
>
> Whether you like it or not, that's still an error for checkpatch:
> ERROR: do not initialise statics to 0 or NULL
> #240: FILE: drivers/hid/i2c-hid/i2c-hid.c:49:
> +static bool debug = false;
>
> The rationale is that the compiler can write more efficient code if
> initializations to 0 or NULL are implicit.

ok, will do.

>
>> +module_param(debug, bool, 0444);
>> +MODULE_PARM_DESC(debug, "print a lot of debug information");
>> +
>> +#define i2c_hid_dbg(ihid, fmt, arg...)       \
>> +     if (debug)                      \
>> +             dev_printk(KERN_DEBUG, &(ihid)->client->dev, fmt, ##arg)
>
> This is considered an error by checkpatch too:
> ERROR: Macros with complex values should be enclosed in parenthesis
> #244: FILE: drivers/hid/i2c-hid/i2c-hid.c:53:
> +#define i2c_hid_dbg(ihid, fmt, arg...) \
> +       if (debug)                      \
> +               dev_printk(KERN_DEBUG, &(ihid)->client->dev, fmt, ##arg)
>
> And I wouldn't disagree, as the construct above can lead to bugs which
> are hard to see... Consider the following piece of code:
>
>         if (condition)
>                 i2c_hid_dbg(ihid, "Blah blah %d\n", i);
>         else
>                 do_something_very_important();
>
> It looks correct, however with the macro definition above, this expands
> to the unexpected:
>
>         if (condition) {
>                 if (debug)                      \
>                         dev_printk(KERN_DEBUG, &ihid->client->dev,
>                                    "Blah blah %d\n", i);
>                 else
>                         do_something_very_important();
>         }
>
> That is, the condition to call do_something_very_important() is
> condition && !debug, and not !condition as the code suggests. This is
> only an example and your driver doesn't currently use any such
> construct. Still I believe this should be fixed.

With your explanation, you've convinced me. I was not sure on how
should I handle this warning as I copied this macro from one in hid.
I will fix the two then.

>
> (...)
> static int i2c_hid_alloc_buffers(struct i2c_hid *ihid)
> {
>         /* the worst case is computed from the set_report command with a
>          * reportID > 15 and the maximum report length */
>         int args_len = sizeof(__u8) + /* optional ReportID byte */
>                        sizeof(__u16) + /* data register */
>                        sizeof(__u16) + /* size of the report */
>                        ihid->bufsize; /* report */
>
>         ihid->inbuf = kzalloc(ihid->bufsize, GFP_KERNEL);
>         ihid->argsbuf = kzalloc(args_len, GFP_KERNEL);
>         ihid->cmdbuf = kzalloc(sizeof(union command) + args_len, GFP_KERNEL);
>
>         if (!ihid->inbuf || !ihid->argsbuf || !ihid->cmdbuf) {
>                 i2c_hid_free_buffers(hid);
>                 return -ENOMEM;
>         }
>
>         return 0;
> }

Much better, thanks!

>
>> +static void i2c_hid_free_buffers(struct i2c_hid *ihid)
>> +{
>> +     kfree(ihid->inbuf);
>> +     kfree(ihid->argsbuf);
>> +     kfree(ihid->cmdbuf);
>> +     ihid->inbuf = NULL;
>> +     ihid->cmdbuf = NULL;
>> +     ihid->argsbuf = NULL;
>> +}
>> +
>> +static int i2c_hid_get_raw_report(struct hid_device *hid,
>> +             unsigned char report_number, __u8 *buf, size_t count,
>> +             unsigned char report_type)
>> +{
>> +     struct i2c_client *client = hid->driver_data;
>> +     struct i2c_hid *ihid = i2c_get_clientdata(client);
>> +     int ret;
>> +
>> +     if (report_type == HID_OUTPUT_REPORT)
>> +             return -EINVAL;
>> +
>> +     if (count > ihid->bufsize)
>> +             count = ihid->bufsize;
>> +
>> +     ret = i2c_hid_get_report(client,
>> +                     report_type == HID_FEATURE_REPORT ? 0x03 : 0x01,
>> +                     report_number, ihid->inbuf, count);
>> +
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     count = ihid->inbuf[0] | (ihid->inbuf[1] << 8);
>> +
>> +     memcpy(buf, ihid->inbuf + 2, count);
>
> What guarantee do you have that count is not larger than buf's length?
> I hope you don't just count on all hardware out there being nice and
> sane, do you? ;)

Hehe, this function is never called from the device, but from the user
space. It's called by hidraw_get_report in drivers/hid/hidraw.c, and
the caller makes the allocation of buf with a size of count.
There is an other usage in hid-input.c with "buf, sizeof(buf)," as arguments.
So this should never be a problem as long as anybody else call this
function without making sure count is the right size.

>
>> +
>> +     return count;
>> +}
>(...)
>> +static int __devinit i2c_hid_init_irq(struct i2c_client *client)
>> +{
>> +     struct i2c_hid *ihid = i2c_get_clientdata(client);
>> +     int ret;
>> +
>> +     dev_dbg(&client->dev, "Requesting IRQ: %d\n", client->irq);
>> +
>> +     ret = request_threaded_irq(client->irq, NULL, i2c_hid_irq,
>> +                     IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>> +                     client->name, ihid);
>> +     if (ret < 0) {
>> +             dev_dbg(&client->dev,
>> +                     "Could not register for %s interrupt, irq = %d,"
>> +                     " ret = %d\n",
>> +             client->name, client->irq, ret);
>
> dev_warn()? Indentation is broken too.
>
>> +
>> +             return ret;
>> +     }
>> +
>> +     ihid->irq = client->irq;
>
> I don't think you need this. Everywhere you use ihid->irq, you have
> client at hand so you could as well use client->irq. This would avoid
> inconsistencies like free_irq(ihid->irq, ihid) in probing error path
> vs. free_irq(client->irq, ihid) in removal path, or
> enable_irq_wake(ihid->irq) vs. disable_irq_wake(client->irq) in
> suspend/resume.

Yep, these inconsistency are pretty bad. IIRC, I used that to know if
the setup was fine for the irq, but it may comes from an earlier
version where I had polling, which is not allowed by the spec.
So definitively, I'll have to rework that.

>
>> +
>> +     return 0;
>> +}
>> +
>(...)
>> +static int __devexit i2c_hid_remove(struct i2c_client *client)
>> +{
>> +     struct i2c_hid *ihid = i2c_get_clientdata(client);
>> +     struct hid_device *hid;
>> +
>> +     if (WARN_ON(!ihid))
>> +             return -1;
>
> This just can't happen...?
>
>> +
>> +     hid = ihid->hid;
>> +     hid_destroy_device(hid);
>> +
>> +     free_irq(client->irq, ihid);
>> +
>
> Is there any guarantee that i2c_hid_stop() has been called before
> i2c_hid_remove() is? If not, you're missing a call to
> i2c_hid_free_buffers() here. BTW I'm not quite sure why
> i2c_hid_remove() frees the buffers in the first place, but then again I
> don't know a thing about the HID infrastructure.

Calling i2c_hid_stop() is the responsibility of the hid driver at his
remove. By hid driver, I mean the driver that relies on hid to handle
the device (hid-generic in 80% of the cases) So as long as this hid
driver is loaded, we can not remove i2c_hid as it is used by the hid
driver. So it seems that this guarantees that i2c_hid_stop() has been
called before i2c_hid_remove() is.

But now that I think of it, there are cases where i2c_hid_stop() is
not called: when the hid driver failed to probe. So definitively,
there is a mem leak here. Thanks.


>
>> +     kfree(ihid);
>> +
>> +     return 0;
>> +}
>> +(...)
>> +static const struct i2c_device_id i2c_hid_id_table[] = {
>> +     { "i2c_hid", 0 },
>
> I am just realizing "i2c_hid" is a little redundant as an i2c device
> name. Can we make this just "hid"?

I know that it already have been used by one Nvidia team and by Elan
for internal tests. So I don't know if it's possible to change it now
(though it's not a big deal).
Anyway, "hid" is a little weird to me (but this is because I started
hacking the kernel from there), as it's really short and does not
contain i2c :)

Cheers,
Benjamin

>
>> +     { },
>> +};
>> +MODULE_DEVICE_TABLE(i2c, i2c_hid_id_table);
>
> --
> Jean Delvare

Re: [PATCH v3] i2c-hid: introduce HID over i2c specification implementation

[Jean Delvare]

Hi Benjamin,

On Mon, 3 Dec 2012 12:32:03 +0100, Benjamin Tissoires wrote:
> Hi Jean,
> 
> On Fri, Nov 30, 2012 at 3:56 PM, Jean Delvare <khali@linux-fr.org> wrote:
> > Hi Benjamin, Jiri,
> >
> > Sorry for the late review. But better late than never I guess...
> 
> Sure! Thanks for the review. As the driver is already in Jiri's tree,
> I'll do small incremental patches based on this release.
> 
> I'll try to address all of your comments.
> 
> I have a few answers to some of your remarks (I fully agree with all
> of the others):
> 
> > On Mon, 12 Nov 2012 15:42:59 +0100, Benjamin Tissoires wrote:
> >> +static int i2c_hid_get_raw_report(struct hid_device *hid,
> >> +             unsigned char report_number, __u8 *buf, size_t count,
> >> +             unsigned char report_type)
> >> +{
> >> +     struct i2c_client *client = hid->driver_data;
> >> +     struct i2c_hid *ihid = i2c_get_clientdata(client);
> >> +     int ret;
> >> +
> >> +     if (report_type == HID_OUTPUT_REPORT)
> >> +             return -EINVAL;
> >> +
> >> +     if (count > ihid->bufsize)
> >> +             count = ihid->bufsize;
> >> +
> >> +     ret = i2c_hid_get_report(client,
> >> +                     report_type == HID_FEATURE_REPORT ? 0x03 : 0x01,
> >> +                     report_number, ihid->inbuf, count);
> >> +
> >> +     if (ret < 0)
> >> +             return ret;
> >> +
> >> +     count = ihid->inbuf[0] | (ihid->inbuf[1] << 8);
> >> +
> >> +     memcpy(buf, ihid->inbuf + 2, count);
> >
> > What guarantee do you have that count is not larger than buf's length?
> > I hope you don't just count on all hardware out there being nice and
> > sane, do you? ;)
> 
> Hehe, this function is never called from the device, but from the user
> space. It's called by hidraw_get_report in drivers/hid/hidraw.c, and
> the caller makes the allocation of buf with a size of count.
> There is an other usage in hid-input.c with "buf, sizeof(buf)," as arguments.
> So this should never be a problem as long as anybody else call this
> function without making sure count is the right size.

Not sure I follow you here.

There are two flavors of "count" in this function. The first one is
passed as a parameter by the caller and used to set the buffer length
as passed to i2c_hid_get_report(). This part is fine and I have no
problem with it. The second flavor is extracted from ihid->inbuf as
provided by i2c_hid_get_report(). As I understand it,
i2c_hid_get_report() fills the buffer with data it receives from the
hardware, so I fail to see how you can have any guarantee that this
second flavor of count is not greater than buf's length.

In fact I don't think you should reuse "count" in the first place,
that's confusing. Then, looking at the code again, I don't think the
test "count > ihid->bufsize", and more generally changing the value of
"count", makes sense. You don't care about the size of "buf" when
calling i2c_hid_get_report(), you care about the size of ihid->inbuf,
which should be at least 2 more than the size of "buf". You care about
the size of "buf" _after_ the call to i2c_hid_get_report(), as you are
copying the data from ihid->bufsize to "buf". This is precisely the
check which I claim is missing.

> >> (...)
> >> +
> >> +     hid = ihid->hid;
> >> +     hid_destroy_device(hid);
> >> +
> >> +     free_irq(client->irq, ihid);
> >> +
> >
> > Is there any guarantee that i2c_hid_stop() has been called before
> > i2c_hid_remove() is? If not, you're missing a call to
> > i2c_hid_free_buffers() here. BTW I'm not quite sure why
> > i2c_hid_remove() frees the buffers in the first place, but then again I
> > don't know a thing about the HID infrastructure.
> 
> Calling i2c_hid_stop() is the responsibility of the hid driver at his
> remove. By hid driver, I mean the driver that relies on hid to handle
> the device (hid-generic in 80% of the cases) So as long as this hid
> driver is loaded, we can not remove i2c_hid as it is used by the hid
> driver. So it seems that this guarantees that i2c_hid_stop() has been
> called before i2c_hid_remove() is.
> 
> But now that I think of it, there are cases where i2c_hid_stop() is
> not called: when the hid driver failed to probe. So definitively,
> there is a mem leak here. Thanks.

Actually this path was fixed by Jiri already:
http://git.kernel.org/?p=linux/kernel/git/jikos/hid.git;a=commitdiff;h=3c62602434c13744df62b3ab0ab7950cd36f24db

My worry was that, when probe succeeds, i2c_hid_start/stop() could
never be called if the module was unloaded immediately for example,
before user-space has a chance to use the device. But if you are
certain it can't happen, then alright.

> >> +(...)
> >> +static const struct i2c_device_id i2c_hid_id_table[] = {
> >> +     { "i2c_hid", 0 },
> >
> > I am just realizing "i2c_hid" is a little redundant as an i2c device
> > name. Can we make this just "hid"?
> 
> I know that it already have been used by one Nvidia team and by Elan
> for internal tests. So I don't know if it's possible to change it now
> (though it's not a big deal).

Yes it is possible, as long as the code isn't in Linus' tree (and I'd
even say: as long as it is not in any kernel released by Linus.)
Whoever is already using your driver will have to adjust their code.
They'll certainly want to anyway, in order to get the bug fixes.

> Anyway, "hid" is a little weird to me (but this is because I started
> hacking the kernel from there), as it's really short and does not
> contain i2c :)

This is exactly my point: no other i2c client (to my knowledge) has
"i2c" in its name, because it is redundant. 

I would agree that this is kind of a special case as this is a generic
driver and not a device-specific driver. I would still prefer "hid" but
if you don't feel like changing it, I guess I can live with that.

-- 
Jean Delvare

Re: [PATCH v3] i2c-hid: introduce HID over i2c specification implementation

[Benjamin Tissoires]

On Mon, Dec 3, 2012 at 2:02 PM, Jean Delvare <khali@linux-fr.org> wrote:
> Hi Benjamin,
>
> On Mon, 3 Dec 2012 12:32:03 +0100, Benjamin Tissoires wrote:
>> Hi Jean,
>>
>> On Fri, Nov 30, 2012 at 3:56 PM, Jean Delvare <khali@linux-fr.org> wrote:
>> > Hi Benjamin, Jiri,
>> >
>> > Sorry for the late review. But better late than never I guess...
>>
>> Sure! Thanks for the review. As the driver is already in Jiri's tree,
>> I'll do small incremental patches based on this release.
>>
>> I'll try to address all of your comments.
>>
>> I have a few answers to some of your remarks (I fully agree with all
>> of the others):
>>
>> > On Mon, 12 Nov 2012 15:42:59 +0100, Benjamin Tissoires wrote:
>> >> +static int i2c_hid_get_raw_report(struct hid_device *hid,
>> >> +             unsigned char report_number, __u8 *buf, size_t count,
>> >> +             unsigned char report_type)
>> >> +{
>> >> +     struct i2c_client *client = hid->driver_data;
>> >> +     struct i2c_hid *ihid = i2c_get_clientdata(client);
>> >> +     int ret;
>> >> +
>> >> +     if (report_type == HID_OUTPUT_REPORT)
>> >> +             return -EINVAL;
>> >> +
>> >> +     if (count > ihid->bufsize)
>> >> +             count = ihid->bufsize;
>> >> +
>> >> +     ret = i2c_hid_get_report(client,
>> >> +                     report_type == HID_FEATURE_REPORT ? 0x03 : 0x01,
>> >> +                     report_number, ihid->inbuf, count);
>> >> +
>> >> +     if (ret < 0)
>> >> +             return ret;
>> >> +
>> >> +     count = ihid->inbuf[0] | (ihid->inbuf[1] << 8);
>> >> +
>> >> +     memcpy(buf, ihid->inbuf + 2, count);
>> >
>> > What guarantee do you have that count is not larger than buf's length?
>> > I hope you don't just count on all hardware out there being nice and
>> > sane, do you? ;)
>>
>> Hehe, this function is never called from the device, but from the user
>> space. It's called by hidraw_get_report in drivers/hid/hidraw.c, and
>> the caller makes the allocation of buf with a size of count.
>> There is an other usage in hid-input.c with "buf, sizeof(buf)," as arguments.
>> So this should never be a problem as long as anybody else call this
>> function without making sure count is the right size.
>
> Not sure I follow you here.
>
> There are two flavors of "count" in this function. The first one is
> passed as a parameter by the caller and used to set the buffer length
> as passed to i2c_hid_get_report(). This part is fine and I have no
> problem with it. The second flavor is extracted from ihid->inbuf as
> provided by i2c_hid_get_report(). As I understand it,
> i2c_hid_get_report() fills the buffer with data it receives from the
> hardware, so I fail to see how you can have any guarantee that this
> second flavor of count is not greater than buf's length.
>
> In fact I don't think you should reuse "count" in the first place,
> that's confusing. Then, looking at the code again, I don't think the
> test "count > ihid->bufsize", and more generally changing the value of
> "count", makes sense. You don't care about the size of "buf" when
> calling i2c_hid_get_report(), you care about the size of ihid->inbuf,
> which should be at least 2 more than the size of "buf". You care about
> the size of "buf" _after_ the call to i2c_hid_get_report(), as you are
> copying the data from ihid->bufsize to "buf". This is precisely the
> check which I claim is missing.

Oops. Really sorry. You are perfectly right.
This mismatch comes from the reuse of count for 2 different purposes,
so it was not a good idea at all.

Will fix it. And thanks!


>
>> >> (...)
>> >> +
>> >> +     hid = ihid->hid;
>> >> +     hid_destroy_device(hid);
>> >> +
>> >> +     free_irq(client->irq, ihid);
>> >> +
>> >
>> > Is there any guarantee that i2c_hid_stop() has been called before
>> > i2c_hid_remove() is? If not, you're missing a call to
>> > i2c_hid_free_buffers() here. BTW I'm not quite sure why
>> > i2c_hid_remove() frees the buffers in the first place, but then again I
>> > don't know a thing about the HID infrastructure.
>>
>> Calling i2c_hid_stop() is the responsibility of the hid driver at his
>> remove. By hid driver, I mean the driver that relies on hid to handle
>> the device (hid-generic in 80% of the cases) So as long as this hid
>> driver is loaded, we can not remove i2c_hid as it is used by the hid
>> driver. So it seems that this guarantees that i2c_hid_stop() has been
>> called before i2c_hid_remove() is.
>>
>> But now that I think of it, there are cases where i2c_hid_stop() is
>> not called: when the hid driver failed to probe. So definitively,
>> there is a mem leak here. Thanks.
>
> Actually this path was fixed by Jiri already:
> http://git.kernel.org/?p=linux/kernel/git/jikos/hid.git;a=commitdiff;h=3c62602434c13744df62b3ab0ab7950cd36f24db

Not exactly. This commit fixes the memory leak when i2c_hid_probe
fails. Not when the hid driver (hid-generic or hid-multitouch in our
cases) fails.

>
> My worry was that, when probe succeeds, i2c_hid_start/stop() could
> never be called if the module was unloaded immediately for example,
> before user-space has a chance to use the device. But if you are
> certain it can't happen, then alright.

I made a few tests after sending the mail. Contrary to what I said,
it's possible to unload the i2c-hid module before the hid module. But
in this case, the function i2c_hid_stop() is called.

So the only missing case is when no hid driver use the module (either
because it has failed or because it's not loaded). As
i2c_hid_alloc_buffer is called in the probe, we should dealloc them in
.remove.

>
>> >> +(...)
>> >> +static const struct i2c_device_id i2c_hid_id_table[] = {
>> >> +     { "i2c_hid", 0 },
>> >
>> > I am just realizing "i2c_hid" is a little redundant as an i2c device
>> > name. Can we make this just "hid"?
>>
>> I know that it already have been used by one Nvidia team and by Elan
>> for internal tests. So I don't know if it's possible to change it now
>> (though it's not a big deal).
>
> Yes it is possible, as long as the code isn't in Linus' tree (and I'd
> even say: as long as it is not in any kernel released by Linus.)
> Whoever is already using your driver will have to adjust their code.
> They'll certainly want to anyway, in order to get the bug fixes.
>
>> Anyway, "hid" is a little weird to me (but this is because I started
>> hacking the kernel from there), as it's really short and does not
>> contain i2c :)
>
> This is exactly my point: no other i2c client (to my knowledge) has
> "i2c" in its name, because it is redundant.
>
> I would agree that this is kind of a special case as this is a generic
> driver and not a device-specific driver. I would still prefer "hid" but
> if you don't feel like changing it, I guess I can live with that.

Ok, I'll change the name.
My concerns were just because from my point of view, "hid" refers to
the core implementation. But for OEMs, they need to declare a "hid"
device, so it makes sense. Anyway, when I'll be able to get my hand on
an ACPI 5 device with hid over i2c, I'll be able to write the missing
autoloading code, and this name will be only used by a small part of
OEMs (I hope).

Cheers,
Benjamin

Re: [PATCH v3] i2c-hid: introduce HID over i2c specification implementation

[Jiri Kosina]

On Mon, 3 Dec 2012, Benjamin Tissoires wrote:

> >> I know that it already have been used by one Nvidia team and by Elan
> >> for internal tests. So I don't know if it's possible to change it now
> >> (though it's not a big deal).
> >
> > Yes it is possible, as long as the code isn't in Linus' tree (and I'd
> > even say: as long as it is not in any kernel released by Linus.)
> > Whoever is already using your driver will have to adjust their code.
> > They'll certainly want to anyway, in order to get the bug fixes.
> >
> >> Anyway, "hid" is a little weird to me (but this is because I started
> >> hacking the kernel from there), as it's really short and does not
> >> contain i2c :)
> >
> > This is exactly my point: no other i2c client (to my knowledge) has
> > "i2c" in its name, because it is redundant.
> >
> > I would agree that this is kind of a special case as this is a generic
> > driver and not a device-specific driver. I would still prefer "hid" but
> > if you don't feel like changing it, I guess I can live with that.
> 
> Ok, I'll change the name.
> My concerns were just because from my point of view, "hid" refers to
> the core implementation. But for OEMs, they need to declare a "hid"
> device, so it makes sense. Anyway, when I'll be able to get my hand on
> an ACPI 5 device with hid over i2c, I'll be able to write the missing
> autoloading code, and this name will be only used by a small part of
> OEMs (I hope).

If you are going to change the name, please either send me the patch 
before merge window for 3.8 opens, or explicitly ask me not to include 
this driver in pull request that goes to Linus yet.

Once the driver is in Linus' tree, I'd rather not change the name.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v3] i2c-hid: introduce HID over i2c specification implementation

[Benjamin Tissoires]

On Tue, Dec 4, 2012 at 11:31 AM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Mon, 3 Dec 2012, Benjamin Tissoires wrote:
>
>> >> I know that it already have been used by one Nvidia team and by Elan
>> >> for internal tests. So I don't know if it's possible to change it now
>> >> (though it's not a big deal).
>> >
>> > Yes it is possible, as long as the code isn't in Linus' tree (and I'd
>> > even say: as long as it is not in any kernel released by Linus.)
>> > Whoever is already using your driver will have to adjust their code.
>> > They'll certainly want to anyway, in order to get the bug fixes.
>> >
>> >> Anyway, "hid" is a little weird to me (but this is because I started
>> >> hacking the kernel from there), as it's really short and does not
>> >> contain i2c :)
>> >
>> > This is exactly my point: no other i2c client (to my knowledge) has
>> > "i2c" in its name, because it is redundant.
>> >
>> > I would agree that this is kind of a special case as this is a generic
>> > driver and not a device-specific driver. I would still prefer "hid" but
>> > if you don't feel like changing it, I guess I can live with that.
>>
>> Ok, I'll change the name.
>> My concerns were just because from my point of view, "hid" refers to
>> the core implementation. But for OEMs, they need to declare a "hid"
>> device, so it makes sense. Anyway, when I'll be able to get my hand on
>> an ACPI 5 device with hid over i2c, I'll be able to write the missing
>> autoloading code, and this name will be only used by a small part of
>> OEMs (I hope).
>
> If you are going to change the name, please either send me the patch
> before merge window for 3.8 opens, or explicitly ask me not to include
> this driver in pull request that goes to Linus yet.

Sure, I intend to send some patches today (with the change of the name
included). I'll send the patch related to the name in the first
position so that you can safely pick it up even if you are not
satisfied with the other changes.

Thanks,
Benjamin

>
> Once the driver is in Linus' tree, I'd rather not change the name.
>
> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs