[PATCH 00/14] i2c-hid cleanup and bug fixes

Hi guys,

Jean made a recent review of i2c-hid, and before this driver goes into
Linus' tree, here are some cleanups.

Patch 2 has not been detected by Jean, but it appeared while playing with
buffers allocation. So this is the only change not asked by the previous
review.

I still need to work on the mutex to protect the potential race on .open and
.close, but meanwhile, here is a first batch of patches.

Cheers,
Benjamin

Benjamin Tissoires (14):
  HID: i2c-hid: change I2C name
  HID: i2c-hid: fix memory corruption due to missing hid declarations
  HID: i2c-hid: enhance Kconfig
  HID: i2c-hid: fix checkpatch.pl warning
  HID: i2c-hid: fix i2c_hid_dbg macro
  HID: i2c-hid: remove unused static declarations
  HID: i2c-hid: fix return paths
  HID: i2c-hid: fix error messages
  HID: i2c-hid: i2c_hid_get_report may fail
  HID: i2c-hid: reorder allocation/free of buffers
  HID: i2c-hid: remove unneeded test in i2c_hid_remove
  HID: i2c-hid: remove extra .irq field in struct i2c_hid
  HID: i2c-hid: also call i2c_hid_free_buffers in i2c_hid_remove
  HID: i2c-hid: fix i2c_hid_get_raw_report count mismatches

 drivers/hid/i2c-hid/Kconfig   |   7 +-
 drivers/hid/i2c-hid/i2c-hid.c | 184 ++++++++++++++++++++----------------------
 2 files changed, 89 insertions(+), 102 deletions(-)

-- 
1.8.0.1

HID descriptors contains 4 bytes of reserved field.
The previous implementation was overriding the next fields in struct i2c_hid.

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/i2c-hid/i2c-hid.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 0fbb229..ad771a7 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c

@@ -68,6 +68,7 @@ struct i2c_hid_desc {
 	__le16 wVendorID;
 	__le16 wProductID;
 	__le16 wVersionID;
+	__le32 reserved;
 } __packed;
 
 struct i2c_hid_cmd {

@@ -778,7 +779,7 @@ static int __devinit i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
 	}
 
 	dsize = le16_to_cpu(hdesc->wHIDDescLength);
-	if (!dsize || dsize > HID_MAX_DESCRIPTOR_SIZE) {
+	if (!dsize || dsize > sizeof(struct i2c_hid_desc)) {
 		dev_err(&client->dev, "weird size of HID descriptor (%u)\n",
 			dsize);
 		return -ENODEV;

-- 
1.8.0.1

These definitions are not used here, but are defined by the specification.
Keeping some of them for documentation purposes.

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/i2c-hid/i2c-hid.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 54950be..4452611 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c

@@ -106,22 +106,17 @@ 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 };
+
+/*
+ * These definitions are not used here, but are defined by the spec.
+ * Keeping them here for documentation purposes.
+ *
+ * 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 main device structure */
 struct i2c_hid {

-- 
1.8.0.1

Simplifies i2c_hid_alloc_buffers tests, and makes this function
responsible of the assignment of ihid->bufsize.
The condition for the reallocation in i2c_hid_start is then simpler.

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/i2c-hid/i2c-hid.c | 68 ++++++++++++++++++-------------------------
 1 file changed, 28 insertions(+), 40 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index dcacfc4..4e3f80b 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c

@@ -465,48 +465,38 @@ static void i2c_hid_find_max_report(struct hid_device *hid, unsigned int type,
 	}
 }
 
-static int i2c_hid_alloc_buffers(struct i2c_hid *ihid)
+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;
+	ihid->bufsize = 0;
+}
+
+static int i2c_hid_alloc_buffers(struct i2c_hid *ihid, size_t report_size)
 {
 	/* 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;
+		       report_size; /* report */
 
+	ihid->inbuf = kzalloc(report_size, GFP_KERNEL);
 	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;
+	if (!ihid->inbuf || !ihid->argsbuf || !ihid->cmdbuf) {
+		i2c_hid_free_buffers(ihid);
 		return -ENOMEM;
 	}
 
-	return 0;
-}
+	ihid->bufsize = report_size;
 
-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;
+	return 0;
 }
 
 static int i2c_hid_get_raw_report(struct hid_device *hid,

@@ -611,22 +601,19 @@ 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;
+	unsigned int bufsize = HID_MIN_BUFFER_SIZE;
 
-	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);
+	i2c_hid_find_max_report(hid, HID_INPUT_REPORT, &bufsize);
+	i2c_hid_find_max_report(hid, HID_OUTPUT_REPORT, &bufsize);
+	i2c_hid_find_max_report(hid, HID_FEATURE_REPORT, &bufsize);
 
-	if (ihid->bufsize > old_bufsize || !ihid->inbuf || !ihid->cmdbuf) {
+	if (bufsize > ihid->bufsize) {
 		i2c_hid_free_buffers(ihid);
 
-		ret = i2c_hid_alloc_buffers(ihid);
+		ret = i2c_hid_alloc_buffers(ihid, bufsize);
 
-		if (ret) {
-			ihid->bufsize = old_bufsize;
+		if (ret)
 			return ret;
-		}
 	}
 
 	if (!(hid->quirks & HID_QUIRK_NO_INIT_REPORTS))

@@ -844,8 +831,9 @@ static int __devinit i2c_hid_probe(struct i2c_client *client,
 	/* 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_alloc_buffers(ihid, HID_MIN_BUFFER_SIZE);
+	if (ret < 0)
+		goto err;
 
 	ret = i2c_hid_fetch_hid_descriptor(ihid);
 	if (ret < 0)

-- 
1.8.0.1

The previous memcpy implementation relied on the size advertized by the
device. There were no guarantees that buf was big enough.

Some gymnastic is also required with the +2/-2 to take into account
the first 2 bytes where the total length is supplied by the device.

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/i2c-hid/i2c-hid.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 62988f1..de3566f 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c

@@ -503,13 +503,14 @@ static int i2c_hid_get_raw_report(struct hid_device *hid,
 {
 	struct i2c_client *client = hid->driver_data;
 	struct i2c_hid *ihid = i2c_get_clientdata(client);
+	size_t rcount;
 	int ret;
 
 	if (report_type == HID_OUTPUT_REPORT)
 		return -EINVAL;
 
-	if (count > ihid->bufsize)
-		count = ihid->bufsize;
+	if (count > ihid->bufsize - 2)
+		count = ihid->bufsize - 2;
 
 	ret = i2c_hid_get_report(client,
 			report_type == HID_FEATURE_REPORT ? 0x03 : 0x01,

@@ -518,7 +519,13 @@ static int i2c_hid_get_raw_report(struct hid_device *hid,
 	if (ret < 0)
 		return ret;
 
-	count = ihid->inbuf[0] | (ihid->inbuf[1] << 8);
+	rcount = ihid->inbuf[0] | (ihid->inbuf[1] << 8);
+
+	if (!rcount)
+		return 0;
+
+	if (count > rcount - 2)
+		count = rcount - 2;
 
 	memcpy(buf, ihid->inbuf + 2, count);
 

-- 
1.8.0.1

In the case where the hid driver in charge of handling the hid part
of the device (hid-generic for instance) fails at probe, neither
i2c_hid_start nor i2c_hid_stop are called.
Thus, the buffers allocated in i2c_hid_probe are never freed.

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/i2c-hid/i2c-hid.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index bea4b13..62988f1 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c

@@ -891,6 +891,9 @@ static int __devexit i2c_hid_remove(struct i2c_client *client)
 
 	free_irq(client->irq, ihid);
 
+	if (ihid->bufsize)
+		i2c_hid_free_buffers(ihid);
+
 	kfree(ihid);
 
 	return 0;

-- 
1.8.0.1

There is no point in keeping the irq in i2c_hid as it's already
there in client.

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/i2c-hid/i2c-hid.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 2b49929..bea4b13 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c

@@ -136,8 +136,6 @@ struct i2c_hid {
 
 	unsigned long		flags;		/* device flags */
 
-	int			irq;		/* the interrupt line irq */
-
 	wait_queue_head_t	wait;		/* For waiting the interrupt */
 };
 

@@ -737,8 +735,6 @@ static int __devinit i2c_hid_init_irq(struct i2c_client *client)
 		return ret;
 	}
 
-	ihid->irq = client->irq;
-
 	return 0;
 }
 

@@ -841,12 +837,12 @@ static int __devinit i2c_hid_probe(struct i2c_client *client,
 
 	ret = i2c_hid_init_irq(client);
 	if (ret < 0)
-		goto err;
+		goto err_irq;
 
 	hid = hid_allocate_device();
 	if (IS_ERR(hid)) {
 		ret = PTR_ERR(hid);
-		goto err;
+		goto err_irq;
 	}
 
 	ihid->hid = hid;

@@ -876,10 +872,10 @@ static int __devinit i2c_hid_probe(struct i2c_client *client,
 err_mem_free:
 	hid_destroy_device(hid);
 
-err:
-	if (ihid->irq)
-		free_irq(ihid->irq, ihid);
+err_irq:
+	free_irq(client->irq, ihid);
 
+err:
 	i2c_hid_free_buffers(ihid);
 	kfree(ihid);
 	return ret;

@@ -904,10 +900,9 @@ static int __devexit i2c_hid_remove(struct i2c_client *client)
 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);
+		enable_irq_wake(client->irq);
 
 	/* Save some power */
 	i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);

-- 
1.8.0.1

ihid can not be null, so there are no reasons to test it.

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/i2c-hid/i2c-hid.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 4e3f80b..2b49929 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c

@@ -890,9 +890,6 @@ 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);
 

-- 
1.8.0.1

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/i2c-hid/i2c-hid.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index d6fdb3e..da04948 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c

@@ -741,10 +741,10 @@ static int __devinit i2c_hid_init_irq(struct i2c_client *client)
 			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
 			client->name, ihid);
 	if (ret < 0) {
-		dev_dbg(&client->dev,
+		dev_warn(&client->dev,
 			"Could not register for %s interrupt, irq = %d,"
 			" ret = %d\n",
-		client->name, client->irq, ret);
+			client->name, client->irq, ret);
 
 		return ret;
 	}

@@ -770,7 +770,8 @@ static int __devinit i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
 			__func__, 4, ihid->hdesc_buffer);
 
 	if (ret) {
-		dev_err(&client->dev, "HID_DESCR_LENGTH_CMD Fail (ret=%d)\n",
+		dev_err(&client->dev,
+			"unable to fetch the size of HID descriptor (ret=%d)\n",
 			ret);
 		return -ENODEV;
 	}

@@ -785,7 +786,7 @@ static int __devinit i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
 	/* check bcdVersion == 1.0 */
 	if (le16_to_cpu(hdesc->bcdVersion) != 0x0100) {
 		dev_err(&client->dev,
-			"unexpected HID descriptor bcdVersion (0x%04x)\n",
+			"unexpected HID descriptor bcdVersion (0x%04hx)\n",
 			le16_to_cpu(hdesc->bcdVersion));
 		return -ENODEV;
 	}

@@ -871,7 +872,7 @@ static int __devinit i2c_hid_probe(struct i2c_client *client,
 	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",
+	snprintf(hid->name, sizeof(hid->name), "%s %04hX:%04hX",
 		 client->name, hid->vendor, hid->product);
 
 	ret = hid_add_device(hid);

-- 
1.8.0.1

Forwards appropriate return values.
As noone use the error returned by i2c_hid_get_input, let's make it
returning void.

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/i2c-hid/i2c-hid.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 4452611..d6fdb3e 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c

@@ -245,7 +245,7 @@ static int i2c_hid_get_report(struct i2c_client *client, u8 reportType,
 	if (ret) {
 		dev_err(&client->dev,
 			"failed to retrieve report from device.\n");
-		return -EINVAL;
+		return ret;
 	}
 
 	return 0;

@@ -290,7 +290,7 @@ static int i2c_hid_set_report(struct i2c_client *client, u8 reportType,
 		reportType, args, args_len, NULL, 0);
 	if (ret) {
 		dev_err(&client->dev, "failed to set a report to device.\n");
-		return -EINVAL;
+		return ret;
 	}
 
 	return data_len;

@@ -334,7 +334,7 @@ static int i2c_hid_hwreset(struct i2c_client *client)
 	return 0;
 }
 
-static int i2c_hid_get_input(struct i2c_hid *ihid)
+static void i2c_hid_get_input(struct i2c_hid *ihid)
 {
 	int ret, ret_size;
 	int size = le16_to_cpu(ihid->hdesc.wMaxInputLength);

@@ -342,11 +342,11 @@ static int i2c_hid_get_input(struct i2c_hid *ihid)
 	ret = i2c_master_recv(ihid->client, ihid->inbuf, size);
 	if (ret != size) {
 		if (ret < 0)
-			return ret;
+			return;
 
 		dev_err(&ihid->client->dev, "%s: got %d data instead of %d\n",
 			__func__, ret, size);
-		return ret;
+		return;
 	}
 
 	ret_size = ihid->inbuf[0] | ihid->inbuf[1] << 8;

@@ -355,13 +355,13 @@ static int i2c_hid_get_input(struct i2c_hid *ihid)
 		/* host or device initiated RESET completed */
 		if (test_and_clear_bit(I2C_HID_RESET_PENDING, &ihid->flags))
 			wake_up(&ihid->wait);
-		return 0;
+		return;
 	}
 
 	if (ret_size > size) {
 		dev_err(&ihid->client->dev, "%s: incomplete report (%d/%d)\n",
 			__func__, size, ret_size);
-		return -EIO;
+		return;
 	}
 
 	i2c_hid_dbg(ihid, "input: %*ph\n", ret_size, ihid->inbuf);

@@ -370,7 +370,7 @@ static int i2c_hid_get_input(struct i2c_hid *ihid)
 		hid_input_report(ihid->hid, HID_INPUT_REPORT, ihid->inbuf + 2,
 				ret_size - 2, 1);
 
-	return 0;
+	return;
 }
 
 static irqreturn_t i2c_hid_irq(int irq, void *dev_id)

@@ -430,8 +430,10 @@ static void i2c_hid_init_reports(struct hid_device *hid)
 	struct i2c_hid *ihid = i2c_get_clientdata(client);
 	u8 *inbuf = kzalloc(ihid->bufsize, GFP_KERNEL);
 
-	if (!inbuf)
+	if (!inbuf) {
+		dev_err(&client->dev, "can not retrieve initial reports\n");
 		return;
+	}
 
 	list_for_each_entry(report,
 		&hid->report_enum[HID_INPUT_REPORT].report_list, list)

@@ -715,9 +717,7 @@ static int i2c_hid_hidinput_input_event(struct input_dev *dev,
 		return -1;
 	}
 
-	hid_set_field(field, offset, value);
-
-	return 0;
+	return hid_set_field(field, offset, value);
 }
 
 static struct hid_ll_driver i2c_hid_ll_driver = {

-- 
1.8.0.1

The "comment" part can never be displayed, so we can remove it.

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/i2c-hid/Kconfig | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/i2c-hid/Kconfig b/drivers/hid/i2c-hid/Kconfig
index 5b7d4d8..64ce108 100644
--- a/drivers/hid/i2c-hid/Kconfig
+++ b/drivers/hid/i2c-hid/Kconfig

@@ -7,15 +7,12 @@ config I2C_HID
 	depends on I2C && INPUT
 	select HID
 	---help---
-	  Say Y here if you want to use the HID over i2c protocol
-	  implementation.
+	  Say Y here if you use a keyboard, a touchpad, a touchscreen, or any
+	  other HID based devices which is connected to your computer via I2C.
 
 	  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

-- 
1.8.0.1

no I2C driver has "i2c" in its name. It makes more sense to call this
i2c driver "hid".

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/i2c-hid/i2c-hid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 67ab5b7..0fbb229 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c

@@ -949,7 +949,7 @@ static int i2c_hid_resume(struct device *dev)
 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 },
+	{ "hid", 0 },
 	{ },
 };
 MODULE_DEVICE_TABLE(i2c, i2c_hid_id_table);

-- 
1.8.0.1

We should not initialize to 0 static declarations.

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/i2c-hid/i2c-hid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index ad771a7..9ee6555 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c

@@ -46,7 +46,7 @@
 #define I2C_HID_PWR_SLEEP	0x01
 
 /* debug option */
-static bool debug = false;
+static bool debug;
 module_param(debug, bool, 0444);
 MODULE_PARM_DESC(debug, "print a lot of debug information");
 

-- 
1.8.0.1

If i2c_hid_get_report fails, exit i2c_hid_init_report.
The printk log is already called by i2c_hid_get_report, so no need
to add some more printks.

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/i2c-hid/i2c-hid.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index da04948..dcacfc4 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c

@@ -400,9 +400,10 @@ static void i2c_hid_init_report(struct hid_report *report, u8 *buffer,
 	unsigned int size, ret_size;
 
 	size = i2c_hid_get_report_length(report);
-	i2c_hid_get_report(client,
+	if (i2c_hid_get_report(client,
 			report->type == HID_FEATURE_REPORT ? 0x03 : 0x01,
-			report->id, buffer, size);
+			report->id, buffer, size))
+		return;
 
 	i2c_hid_dbg(ihid, "report (len=%d): %*ph\n", size, size, ihid->inbuf);
 

-- 
1.8.0.1

This avoids the problematic case:

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

Which looks correct, however with the previous macro definition,
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();
}

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/i2c-hid/i2c-hid.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 9ee6555..54950be 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c

@@ -50,9 +50,11 @@ static bool debug;
 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)
+#define i2c_hid_dbg(ihid, fmt, arg...)					  \
+do {									  \
+	if (debug)							  \
+		dev_printk(KERN_DEBUG, &(ihid)->client->dev, fmt, ##arg); \
+} while (0)
 
 struct i2c_hid_desc {
 	__le16 wHIDDescLength;

-- 
1.8.0.1

On Tue,  4 Dec 2012 16:27:42 +0100, Benjamin Tissoires wrote:

Reviewed-by: Jean Delvare <redacted>

-- 
Jean Delvare

On Tue,  4 Dec 2012 16:27:43 +0100, Benjamin Tissoires wrote:

Shouldn't !dsize rather be dsize < 4? You're reading hdesc->bcdVersion,
which is only defined if dsize >= 4 if I understand the code correctly.

 		dev_err(&client->dev, "weird size of HID descriptor (%u)\n",
 			dsize);
 		return -ENODEV;

-- 
Jean Delvare

On Tue,  4 Dec 2012 16:27:44 +0100, Benjamin Tissoires wrote:

Reviewed-by: Jean Delvare <redacted>

-- 
Jean Delvare

On Tue,  4 Dec 2012 16:27:45 +0100, Benjamin Tissoires wrote:

Reviewed-by: Jean Delvare <redacted>

-- 
Jean Delvare

On Tue,  4 Dec 2012 16:27:46 +0100, Benjamin Tissoires wrote:

Reviewed-by: Jean Delvare <redacted>

-- 
Jean Delvare

On Tue,  4 Dec 2012 16:27:47 +0100, Benjamin Tissoires wrote:

Reviewed-by: Jean Delvare <redacted>

-- 
Jean Delvare

On Tue,  4 Dec 2012 16:27:48 +0100, Benjamin Tissoires wrote:

This change would have been better located in patch 07/14 IMHO, but
that's a detail.

Reviewed-by: Jean Delvare <redacted>

-- 
Jean Delvare

On Tue,  4 Dec 2012 16:27:49 +0100, Benjamin Tissoires wrote:

Reviewed-by: Jean Delvare <redacted>

-- 
Jean Delvare

On Tue, 4 Dec 2012, Jean Delvare wrote:

quoted

no I2C driver has "i2c" in its name. It makes more sense to call this
i2c driver "hid".

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/i2c-hid/i2c-hid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 67ab5b7..0fbb229 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c

@@ -949,7 +949,7 @@ static int i2c_hid_resume(struct device *dev)
 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 },
+	{ "hid", 0 },
 	{ },
 };
 MODULE_DEVICE_TABLE(i2c, i2c_hid_id_table);

Reviewed-by: Jean Delvare <redacted>

Applied, thanks.

-- 
Jiri Kosina
SUSE Labs

On Tue, 4 Dec 2012, Jean Delvare wrote:

quoted

The "comment" part can never be displayed, so we can remove it.

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/i2c-hid/Kconfig | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/i2c-hid/Kconfig b/drivers/hid/i2c-hid/Kconfig
index 5b7d4d8..64ce108 100644
--- a/drivers/hid/i2c-hid/Kconfig
+++ b/drivers/hid/i2c-hid/Kconfig

@@ -7,15 +7,12 @@ config I2C_HID
 	depends on I2C && INPUT
 	select HID
 	---help---
-	  Say Y here if you want to use the HID over i2c protocol
-	  implementation.
+	  Say Y here if you use a keyboard, a touchpad, a touchscreen, or any
+	  other HID based devices which is connected to your computer via I2C.
 
 	  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

Reviewed-by: Jean Delvare <redacted>

Applied.

-- 
Jiri Kosina
SUSE Labs

On Tue, 4 Dec 2012, Jean Delvare wrote:

quoted

We should not initialize to 0 static declarations.

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/i2c-hid/i2c-hid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index ad771a7..9ee6555 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c

@@ -46,7 +46,7 @@
 #define I2C_HID_PWR_SLEEP	0x01
 
 /* debug option */
-static bool debug = false;
+static bool debug;
 module_param(debug, bool, 0444);
 MODULE_PARM_DESC(debug, "print a lot of debug information");
 

Reviewed-by: Jean Delvare <redacted>

Applied, thanks.

-- 
Jiri Kosina
SUSE Labs

On Tue, 4 Dec 2012, Jean Delvare wrote:

quoted

This avoids the problematic case:

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

Which looks correct, however with the previous macro definition,
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();
}

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/i2c-hid/i2c-hid.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 9ee6555..54950be 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c

@@ -50,9 +50,11 @@ static bool debug;
 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)
+#define i2c_hid_dbg(ihid, fmt, arg...)					  \
+do {									  \
+	if (debug)							  \
+		dev_printk(KERN_DEBUG, &(ihid)->client->dev, fmt, ##arg); \
+} while (0)
 
 struct i2c_hid_desc {
 	__le16 wHIDDescLength;

Reviewed-by: Jean Delvare <redacted>

Applied.

-- 
Jiri Kosina
SUSE Labs

On Tue,  4 Dec 2012 16:27:50 +0100, Benjamin Tissoires wrote:

OK, although I don't quite get the rationale for not reporting the
errors from i2c_hid_init_report() and i2c_hid_init_reports() to their
respective callers. Does the device have any chance to work properly if
i2c_hid_init_reports() fails?

Reviewed-by: Jean Delvare <redacted>

-- 
Jean Delvare

On Tue, 4 Dec 2012, Jean Delvare wrote:

quoted

These definitions are not used here, but are defined by the specification.
Keeping some of them for documentation purposes.

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/i2c-hid/i2c-hid.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 54950be..4452611 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c

@@ -106,22 +106,17 @@ 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 };
+
+/*
+ * These definitions are not used here, but are defined by the spec.
+ * Keeping them here for documentation purposes.
+ *
+ * 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 main device structure */
 struct i2c_hid {

Reviewed-by: Jean Delvare <redacted>

Applied.

-- 
Jiri Kosina
SUSE Labs

On Tue, 4 Dec 2012, Benjamin Tissoires wrote:

Forwards appropriate return values.
As noone use the error returned by i2c_hid_get_input, let's make it
returning void.

Signed-off-by: Benjamin Tissoires <redacted>

Applied.

-- 
Jiri Kosina
SUSE Labs

On Wed, Dec 5, 2012 at 10:59 AM, Jean Delvare [off-list ref] wrote:

On Tue,  4 Dec 2012 16:27:50 +0100, Benjamin Tissoires wrote:

quoted

If i2c_hid_get_report fails, exit i2c_hid_init_report.
The printk log is already called by i2c_hid_get_report, so no need
to add some more printks.

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/i2c-hid/i2c-hid.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index da04948..dcacfc4 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c

@@ -400,9 +400,10 @@ static void i2c_hid_init_report(struct hid_report *report, u8 *buffer,
      unsigned int size, ret_size;

      size = i2c_hid_get_report_length(report);
-     i2c_hid_get_report(client,
+     if (i2c_hid_get_report(client,
                      report->type == HID_FEATURE_REPORT ? 0x03 : 0x01,
-                     report->id, buffer, size);
+                     report->id, buffer, size))
+             return;

      i2c_hid_dbg(ihid, "report (len=%d): %*ph\n", size, size, ihid->inbuf);

OK, although I don't quite get the rationale for not reporting the
errors from i2c_hid_init_report() and i2c_hid_init_reports() to their
respective callers. Does the device have any chance to work properly if
i2c_hid_init_reports() fails?

Hi Jean,

first thanks for the review you have started.

For your question, the answer is yes, the device works properly even
if i2c_hid_init_reports().
IIRC, the only required steps are:
- get HID descriptor
- get HID report descriptors
- send reset
- set power on

 i2c_hid_init_reports is not part of the specification for the boot
process, and the Windows driver does not retrieve the reports for
input reports at all (it does it though for the features).
Actually, the device I have does not implement get_report for inputs
(it returns 0), but it's not a problem for it to work.

I put the whole init_reports in place to get the features values
before sending them to the hid driver, and also to copy the behavior
of usbhid.

Cheers,
Benjamin

Reviewed-by: Jean Delvare <redacted>

--
Jean Delvare

On Wed, 5 Dec 2012, Jean Delvare wrote:

quoted

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/i2c-hid/i2c-hid.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index d6fdb3e..da04948 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c

@@ -741,10 +741,10 @@ static int __devinit i2c_hid_init_irq(struct i2c_client *client)
 			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
 			client->name, ihid);
 	if (ret < 0) {
-		dev_dbg(&client->dev,
+		dev_warn(&client->dev,
 			"Could not register for %s interrupt, irq = %d,"
 			" ret = %d\n",
-		client->name, client->irq, ret);
+			client->name, client->irq, ret);
 
 		return ret;
 	}

@@ -770,7 +770,8 @@ static int __devinit i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
 			__func__, 4, ihid->hdesc_buffer);
 
 	if (ret) {
-		dev_err(&client->dev, "HID_DESCR_LENGTH_CMD Fail (ret=%d)\n",
+		dev_err(&client->dev,
+			"unable to fetch the size of HID descriptor (ret=%d)\n",
 			ret);
 		return -ENODEV;
 	}

@@ -785,7 +786,7 @@ static int __devinit i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
 	/* check bcdVersion == 1.0 */
 	if (le16_to_cpu(hdesc->bcdVersion) != 0x0100) {
 		dev_err(&client->dev,
-			"unexpected HID descriptor bcdVersion (0x%04x)\n",
+			"unexpected HID descriptor bcdVersion (0x%04hx)\n",
 			le16_to_cpu(hdesc->bcdVersion));
 		return -ENODEV;
 	}

@@ -871,7 +872,7 @@ static int __devinit i2c_hid_probe(struct i2c_client *client,
 	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",
+	snprintf(hid->name, sizeof(hid->name), "%s %04hX:%04hX",
 		 client->name, hid->vendor, hid->product);
 
 	ret = hid_add_device(hid);

Reviewed-by: Jean Delvare <redacted>