[PATCH 00/12] i2c-hid: fixes for unnumbered reports and other improvements

[PATCH 00/12] i2c-hid: fixes for unnumbered reports and other improvements

[Dmitry Torokhov]

Hi,

This series came about after I reviewed Angela's patch that fixed issues
with incorrect handling of high-numbered reports (15 and above) in
i2c-hid driver:

- it appears to me that the driver did not handle unnumbered reports
  correctly as the kernel internally expects unnumbered reports to be
  still prepended with report number 0, but i2c_hid_get_raw_report() and
  i2c_hid_output_raw_report() only expected report ID to be present for
  numbered reports.

- the driver tried to consolidate both command handling and sending
  output reports in __i2c_hid_command() but the rules for different
  commands vary significantly and because of that the logic was hard to
  follow and it bled out from __i2c_hid_command() to callers. I decided
  to introduce a few simple helpers and have the data encoding for
  individual commands done at the call site. I believe this made it
  easier to validate the rules and the logic and allowed to remove
  special handling for the HID descriptor retrieval, among other things.

- the driver does too many copying of data; encoding the data for
  commands at the call site allowed to only copy data once into the
  transfer buffers.

I tested this on a couple of Chromebooks with i2c-hid touchscreens, but
it would be great if other folks tried it out as well.

Thanks.

Angela Czubak (1):
  HID: i2c-hid: fix handling numbered reports with IDs of 15 and above

Dmitry Torokhov (11):
  HID: i2c-hid: fix GET/SET_REPORT for unnumbered reports
  HID: i2c-hid: use "struct i2c_hid" as argument in most calls
  HID: i2c-hid: refactor reset command
  HID: i2c-hid: explicitly code setting and sending reports
  HID: i2c-hid: define i2c_hid_read_register() and use it
  HID: i2c-hid: create a helper for SET_POWER command
  HID: i2c-hid: convert i2c_hid_execute_reset() to use i2c_hid_xfer()
  HID: i2c-hid: rework i2c_hid_get_report() to use i2c_hid_xfer()
  HID: i2c-hid: use helpers to do endian conversion in i2c_hid_get_input()
  HID: i2c-hid: no longer need raw access to HID descriptor structure
  HID: i2c-hid: note that I2C xfer buffers are DMA-safe

 drivers/hid/i2c-hid/i2c-hid-core.c | 593 +++++++++++++++--------------
 1 file changed, 313 insertions(+), 280 deletions(-)

-- 
Dmitry

[PATCH 01/12] HID: i2c-hid: fix handling numbered reports with IDs of 15 and above

[Dmitry Torokhov]

From: Angela Czubak <acz@semihalf.com>

Special handling of numbered reports with IDs of 15 and above is only
needed when executing what HID-I2C spec is calling "Class Specific
Requests", and not when simply sending output reports.

Additionally, our mangling of report ID in i2c_hid_set_or_send_report()
resulted in incorrect report ID being written into SET_REPORT command
payload.

To solve it let's move all the report ID manipulation into
__i2c_hid_command() where we form the command data structure.

Signed-off-by: Angela Czubak <acz@semihalf.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 517141138b00..bd7b0eeca3ea 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -97,6 +97,7 @@ union command {
 		__le16 reg;
 		__u8 reportTypeID;
 		__u8 opcode;
+		__u8 reportID;
 	} __packed c;
 };
 
@@ -232,7 +233,13 @@ static int __i2c_hid_command(struct i2c_client *client,
 
 	if (length > 2) {
 		cmd->c.opcode = command->opcode;
-		cmd->c.reportTypeID = reportID | reportType << 4;
+		if (reportID < 0x0F) {
+			cmd->c.reportTypeID = reportType << 4 | reportID;
+		} else {
+			cmd->c.reportTypeID = reportType << 4 | 0x0F;
+			cmd->c.reportID = reportID;
+			length++;
+		}
 	}
 
 	memcpy(cmd->data + length, args, args_len);
@@ -293,18 +300,13 @@ 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];
+	u8 args[2];
 	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;
 
@@ -350,18 +352,12 @@ static int i2c_hid_set_or_send_report(struct i2c_client *client, u8 reportType,
 	size =		2			/* size */ +
 			(reportID ? 1 : 0)	/* reportID */ +
 			data_len		/* buf */;
-	args_len =	(reportID >= 0x0F ? 1 : 0) /* optional third byte */ +
-			2			/* dataRegister */ +
+	args_len =	2			/* dataRegister */ +
 			size			/* args */;
 
 	if (!use_data && maxOutputLength == 0)
 		return -ENOSYS;
 
-	if (reportID >= 0x0F) {
-		args[index++] = reportID;
-		reportID = 0x0F;
-	}
-
 	/*
 	 * use the data register for feature reports or if the device does not
 	 * support the output register
-- 
2.34.1.703.g22d0c6ccf7-goog

[PATCH 02/12] HID: i2c-hid: fix GET/SET_REPORT for unnumbered reports

[Dmitry Torokhov]

Internally kernel prepends all report buffers, for both numbered and
unnumbered reports, with report ID, therefore to properly handle unnumbered
reports we should

For the same reason we should skip the first byte of the buffer when
calling i2c_hid_set_or_send_report() which then will take care of properly
formatting the transfer buffer based on its separate report ID argument
along with report payload.

Fixes: 9b5a9ae88573 ("HID: i2c-hid: implement ll_driver transport-layer callbacks")
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 32 ++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index bd7b0eeca3ea..b383003ff676 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -611,6 +611,17 @@ static int i2c_hid_get_raw_report(struct hid_device *hid,
 	if (report_type == HID_OUTPUT_REPORT)
 		return -EINVAL;
 
+	/*
+	 * In case of unnumbered reports the response from the device will
+	 * not have the report ID that the upper layers expect, so we need
+	 * to stash it the buffer ourselves and adjust the data size.
+	 */
+	if (!report_number) {
+		buf[0] = 0;
+		buf++;
+		count--;
+	}
+
 	/* +2 bytes to include the size of the reply in the query buffer */
 	ask_count = min(count + 2, (size_t)ihid->bufsize);
 
@@ -632,6 +643,9 @@ static int i2c_hid_get_raw_report(struct hid_device *hid,
 	count = min(count, ret_count - 2);
 	memcpy(buf, ihid->rawbuf + 2, count);
 
+	if (!report_number)
+		count++;
+
 	return count;
 }
 
@@ -648,17 +662,19 @@ static int i2c_hid_output_raw_report(struct hid_device *hid, __u8 *buf,
 
 	mutex_lock(&ihid->reset_lock);
 
-	if (report_id) {
-		buf++;
-		count--;
-	}
-
+	/*
+	 * Note that both numbered and unnumbered reports passed here
+	 * are supposed to have report ID stored in the 1st byte of the
+	 * buffer, so we strip it off unconditionally before passing payload
+	 * to i2c_hid_set_or_send_report which takes care of encoding
+	 * everything properly.
+	 */
 	ret = i2c_hid_set_or_send_report(client,
 				report_type == HID_FEATURE_REPORT ? 0x03 : 0x02,
-				report_id, buf, count, use_data);
+				report_id, buf + 1, count - 1, use_data);
 
-	if (report_id && ret >= 0)
-		ret++; /* add report_id to the number of transfered bytes */
+	if (ret >= 0)
+		ret++; /* add report_id to the number of transferred bytes */
 
 	mutex_unlock(&ihid->reset_lock);
 
-- 
2.34.1.703.g22d0c6ccf7-goog

[PATCH 03/12] HID: i2c-hid: use "struct i2c_hid" as argument in most calls

[Dmitry Torokhov]

The main object in the driver is struct i2c_hid so it makes more sense
to pass it around instead of passing i2c_client and then fetching
i2c_hid associated with it.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 75 ++++++++++++++----------------
 1 file changed, 36 insertions(+), 39 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index b383003ff676..bae3e7a9b2e4 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -208,12 +208,12 @@ static u32 i2c_hid_lookup_quirk(const u16 idVendor, const u16 idProduct)
 	return quirks;
 }
 
-static int __i2c_hid_command(struct i2c_client *client,
+static int __i2c_hid_command(struct i2c_hid *ihid,
 		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);
+	struct i2c_client *client = ihid->client;
 	union command *cmd = (union command *)ihid->cmdbuf;
 	int ret;
 	struct i2c_msg msg[2];
@@ -288,18 +288,17 @@ static int __i2c_hid_command(struct i2c_client *client,
 	return ret;
 }
 
-static int i2c_hid_command(struct i2c_client *client,
+static int i2c_hid_command(struct i2c_hid *ihid,
 		const struct i2c_hid_cmd *command,
 		unsigned char *buf_recv, int data_len)
 {
-	return __i2c_hid_command(client, command, 0, 0, NULL, 0,
+	return __i2c_hid_command(ihid, command, 0, 0, NULL, 0,
 				buf_recv, data_len);
 }
 
-static int i2c_hid_get_report(struct i2c_client *client, u8 reportType,
+static int i2c_hid_get_report(struct i2c_hid *ihid, u8 reportType,
 		u8 reportID, unsigned char *buf_recv, int data_len)
 {
-	struct i2c_hid *ihid = i2c_get_clientdata(client);
 	u8 args[2];
 	int ret;
 	int args_len = 0;
@@ -310,10 +309,10 @@ static int i2c_hid_get_report(struct i2c_client *client, u8 reportType,
 	args[args_len++] = readRegister & 0xFF;
 	args[args_len++] = readRegister >> 8;
 
-	ret = __i2c_hid_command(client, &hid_get_report_cmd, reportID,
+	ret = __i2c_hid_command(ihid, &hid_get_report_cmd, reportID,
 		reportType, args, args_len, buf_recv, data_len);
 	if (ret) {
-		dev_err(&client->dev,
+		dev_err(&ihid->client->dev,
 			"failed to retrieve report from device.\n");
 		return ret;
 	}
@@ -323,17 +322,16 @@ static int i2c_hid_get_report(struct i2c_client *client, u8 reportType,
 
 /**
  * i2c_hid_set_or_send_report: forward an incoming report to the device
- * @client: the i2c_client of the device
+ * @ihid: the i2c hid device
  * @reportType: 0x03 for HID_FEATURE_REPORT ; 0x02 for HID_OUTPUT_REPORT
  * @reportID: the report ID
  * @buf: the actual data to transfer, without the report ID
  * @data_len: size of buf
  * @use_data: true: use SET_REPORT HID command, false: send plain OUTPUT report
  */
-static int i2c_hid_set_or_send_report(struct i2c_client *client, u8 reportType,
+static int i2c_hid_set_or_send_report(struct i2c_hid *ihid, u8 reportType,
 		u8 reportID, unsigned char *buf, size_t data_len, bool use_data)
 {
-	struct i2c_hid *ihid = i2c_get_clientdata(client);
 	u8 *args = ihid->argsbuf;
 	const struct i2c_hid_cmd *hidcmd;
 	int ret;
@@ -380,19 +378,19 @@ static int i2c_hid_set_or_send_report(struct i2c_client *client, u8 reportType,
 
 	memcpy(&args[index], buf, data_len);
 
-	ret = __i2c_hid_command(client, hidcmd, reportID,
+	ret = __i2c_hid_command(ihid, hidcmd, reportID,
 		reportType, args, args_len, NULL, 0);
 	if (ret) {
-		dev_err(&client->dev, "failed to set a report to device.\n");
+		dev_err(&ihid->client->dev,
+			"failed to set a report to device.\n");
 		return ret;
 	}
 
 	return data_len;
 }
 
-static int i2c_hid_set_power(struct i2c_client *client, int power_state)
+static int i2c_hid_set_power(struct i2c_hid *ihid, int power_state)
 {
-	struct i2c_hid *ihid = i2c_get_clientdata(client);
 	int ret;
 
 	i2c_hid_dbg(ihid, "%s\n", __func__);
@@ -404,18 +402,18 @@ static int i2c_hid_set_power(struct i2c_client *client, int power_state)
 	 */
 	if (power_state == I2C_HID_PWR_ON &&
 	    ihid->quirks & I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV) {
-		ret = i2c_hid_command(client, &hid_set_power_cmd, NULL, 0);
+		ret = i2c_hid_command(ihid, &hid_set_power_cmd, NULL, 0);
 
 		/* Device was already activated */
 		if (!ret)
 			goto set_pwr_exit;
 	}
 
-	ret = __i2c_hid_command(client, &hid_set_power_cmd, power_state,
+	ret = __i2c_hid_command(ihid, &hid_set_power_cmd, power_state,
 		0, NULL, 0, NULL, 0);
-
 	if (ret)
-		dev_err(&client->dev, "failed to change power setting.\n");
+		dev_err(&ihid->client->dev,
+			"failed to change power setting.\n");
 
 set_pwr_exit:
 
@@ -434,9 +432,8 @@ static int i2c_hid_set_power(struct i2c_client *client, int power_state)
 	return ret;
 }
 
-static int i2c_hid_hwreset(struct i2c_client *client)
+static int i2c_hid_hwreset(struct i2c_hid *ihid)
 {
-	struct i2c_hid *ihid = i2c_get_clientdata(client);
 	int ret;
 
 	i2c_hid_dbg(ihid, "%s\n", __func__);
@@ -448,22 +445,22 @@ static int i2c_hid_hwreset(struct i2c_client *client)
 	 */
 	mutex_lock(&ihid->reset_lock);
 
-	ret = i2c_hid_set_power(client, I2C_HID_PWR_ON);
+	ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON);
 	if (ret)
 		goto out_unlock;
 
 	i2c_hid_dbg(ihid, "resetting...\n");
 
-	ret = i2c_hid_command(client, &hid_reset_cmd, NULL, 0);
+	ret = i2c_hid_command(ihid, &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);
+		dev_err(&ihid->client->dev, "failed to reset device.\n");
+		i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP);
 		goto out_unlock;
 	}
 
 	/* At least some SIS devices need this after reset */
 	if (!(ihid->quirks & I2C_HID_QUIRK_NO_WAKEUP_AFTER_RESET))
-		ret = i2c_hid_set_power(client, I2C_HID_PWR_ON);
+		ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON);
 
 out_unlock:
 	mutex_unlock(&ihid->reset_lock);
@@ -625,7 +622,7 @@ static int i2c_hid_get_raw_report(struct hid_device *hid,
 	/* +2 bytes to include the size of the reply in the query buffer */
 	ask_count = min(count + 2, (size_t)ihid->bufsize);
 
-	ret = i2c_hid_get_report(client,
+	ret = i2c_hid_get_report(ihid,
 			report_type == HID_FEATURE_REPORT ? 0x03 : 0x01,
 			report_number, ihid->rawbuf, ask_count);
 
@@ -669,7 +666,7 @@ static int i2c_hid_output_raw_report(struct hid_device *hid, __u8 *buf,
 	 * to i2c_hid_set_or_send_report which takes care of encoding
 	 * everything properly.
 	 */
-	ret = i2c_hid_set_or_send_report(client,
+	ret = i2c_hid_set_or_send_report(ihid,
 				report_type == HID_FEATURE_REPORT ? 0x03 : 0x02,
 				report_id, buf + 1, count - 1, use_data);
 
@@ -724,7 +721,7 @@ static int i2c_hid_parse(struct hid_device *hid)
 	}
 
 	do {
-		ret = i2c_hid_hwreset(client);
+		ret = i2c_hid_hwreset(ihid);
 		if (ret)
 			msleep(1000);
 	} while (tries-- > 0 && ret);
@@ -748,7 +745,7 @@ static int i2c_hid_parse(struct hid_device *hid)
 
 		i2c_hid_dbg(ihid, "asking HID report descriptor\n");
 
-		ret = i2c_hid_command(client, &hid_report_descr_cmd,
+		ret = i2c_hid_command(ihid, &hid_report_descr_cmd,
 				      rdesc, rsize);
 		if (ret) {
 			hid_err(hid, "reading report descriptor failed\n");
@@ -868,11 +865,11 @@ static int i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
 			*i2c_hid_get_dmi_i2c_hid_desc_override(client->name);
 	} else {
 		i2c_hid_dbg(ihid, "Fetching the HID descriptor\n");
-		ret = i2c_hid_command(client, &hid_descr_cmd,
+		ret = i2c_hid_command(ihid, &hid_descr_cmd,
 				      ihid->hdesc_buffer,
 				      sizeof(struct i2c_hid_desc));
 		if (ret) {
-			dev_err(&client->dev, "hid_descr_cmd failed\n");
+			dev_err(&ihid->client->dev, "hid_descr_cmd failed\n");
 			return -ENODEV;
 		}
 	}
@@ -882,7 +879,7 @@ static int i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
 	 * bytes 2-3 -> bcdVersion (has to be 1.00) */
 	/* check bcdVersion == 1.0 */
 	if (le16_to_cpu(hdesc->bcdVersion) != 0x0100) {
-		dev_err(&client->dev,
+		dev_err(&ihid->client->dev,
 			"unexpected HID descriptor bcdVersion (0x%04hx)\n",
 			le16_to_cpu(hdesc->bcdVersion));
 		return -ENODEV;
@@ -891,8 +888,8 @@ static int i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
 	/* Descriptor length should be 30 bytes as per the specification */
 	dsize = le16_to_cpu(hdesc->wHIDDescLength);
 	if (dsize != sizeof(struct i2c_hid_desc)) {
-		dev_err(&client->dev, "weird size of HID descriptor (%u)\n",
-			dsize);
+		dev_err(&ihid->client->dev,
+			"weird size of HID descriptor (%u)\n", dsize);
 		return -ENODEV;
 	}
 	i2c_hid_dbg(ihid, "HID Descriptor: %*ph\n", dsize, ihid->hdesc_buffer);
@@ -1059,7 +1056,7 @@ void i2c_hid_core_shutdown(struct i2c_client *client)
 {
 	struct i2c_hid *ihid = i2c_get_clientdata(client);
 
-	i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
+	i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP);
 	free_irq(client->irq, ihid);
 
 	i2c_hid_core_shutdown_tail(ihid);
@@ -1082,7 +1079,7 @@ static int i2c_hid_core_suspend(struct device *dev)
 	}
 
 	/* Save some power */
-	i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
+	i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP);
 
 	disable_irq(client->irq);
 
@@ -1130,9 +1127,9 @@ static int i2c_hid_core_resume(struct device *dev)
 	 * let's still reset them here.
 	 */
 	if (ihid->quirks & I2C_HID_QUIRK_RESET_ON_RESUME)
-		ret = i2c_hid_hwreset(client);
+		ret = i2c_hid_hwreset(ihid);
 	else
-		ret = i2c_hid_set_power(client, I2C_HID_PWR_ON);
+		ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON);
 
 	if (ret)
 		return ret;
-- 
2.34.1.703.g22d0c6ccf7-goog

[PATCH 04/12] HID: i2c-hid: refactor reset command

[Dmitry Torokhov]

"Reset" is the only command that needs to wait for interrupt from
the device before continuing, so let's factor our waiting logic from
__i2c_hid_command() to make it simpler.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 63 ++++++++++++++++++------------
 1 file changed, 38 insertions(+), 25 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index bae3e7a9b2e4..6c1741d9211d 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -88,7 +88,6 @@ struct i2c_hid_cmd {
 	unsigned int registerIndex;
 	__u8 opcode;
 	unsigned int length;
-	bool wait;
 };
 
 union command {
@@ -114,8 +113,7 @@ static const struct i2c_hid_cmd hid_report_descr_cmd = {
 		.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_reset_cmd =		{ I2C_HID_CMD(0x01) };
 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_set_power_cmd =	{ I2C_HID_CMD(0x08) };
@@ -220,7 +218,6 @@ static int __i2c_hid_command(struct i2c_hid *ihid,
 	int msg_num = 1;
 
 	int length = command->length;
-	bool wait = command->wait;
 	unsigned int registerIndex = command->registerIndex;
 
 	/* special case for hid_descr_cmd */
@@ -261,9 +258,6 @@ static int __i2c_hid_command(struct i2c_hid *ihid,
 		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)
@@ -272,20 +266,7 @@ static int __i2c_hid_command(struct i2c_hid *ihid,
 	if (ret != msg_num)
 		return ret < 0 ? ret : -EIO;
 
-	ret = 0;
-
-	if (wait && (ihid->quirks & I2C_HID_QUIRK_NO_IRQ_AFTER_RESET)) {
-		msleep(100);
-	} else 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;
+	return 0;
 }
 
 static int i2c_hid_command(struct i2c_hid *ihid,
@@ -432,6 +413,39 @@ static int i2c_hid_set_power(struct i2c_hid *ihid, int power_state)
 	return ret;
 }
 
+static int i2c_hid_execute_reset(struct i2c_hid *ihid)
+{
+	int ret;
+
+	i2c_hid_dbg(ihid, "resetting...\n");
+
+	set_bit(I2C_HID_RESET_PENDING, &ihid->flags);
+
+	ret = i2c_hid_command(ihid, &hid_reset_cmd, NULL, 0);
+	if (ret) {
+		dev_err(&ihid->client->dev, "failed to reset device.\n");
+		goto out;
+	}
+
+	if (ihid->quirks & I2C_HID_QUIRK_NO_IRQ_AFTER_RESET) {
+		msleep(100);
+		goto out;
+	}
+
+	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;
+		goto out;
+	}
+	i2c_hid_dbg(ihid, "%s: finished.\n", __func__);
+
+out:
+	clear_bit(I2C_HID_RESET_PENDING, &ihid->flags);
+	return ret;
+}
+
 static int i2c_hid_hwreset(struct i2c_hid *ihid)
 {
 	int ret;
@@ -449,11 +463,10 @@ static int i2c_hid_hwreset(struct i2c_hid *ihid)
 	if (ret)
 		goto out_unlock;
 
-	i2c_hid_dbg(ihid, "resetting...\n");
-
-	ret = i2c_hid_command(ihid, &hid_reset_cmd, NULL, 0);
+	ret = i2c_hid_execute_reset(ihid);
 	if (ret) {
-		dev_err(&ihid->client->dev, "failed to reset device.\n");
+		dev_err(&ihid->client->dev,
+			"failed to reset device: %d\n", ret);
 		i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP);
 		goto out_unlock;
 	}
-- 
2.34.1.703.g22d0c6ccf7-goog

[PATCH 05/12] HID: i2c-hid: explicitly code setting and sending reports

[Dmitry Torokhov]

Instead of relying on __i2c_hid_command() that tries to handle all
commands and because of that is very complicated, let's define a
new dumb helper i2c_hid_xfer() that actually transfers (write and
read) data, and use it when sending and setting reports. By doing
that we can save on number of copy operations we have to execute,
and make logic of sending reports much clearer.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 269 ++++++++++++++++-------------
 1 file changed, 151 insertions(+), 118 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 6c1741d9211d..c48b75bd81e0 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -35,6 +35,7 @@
 #include <linux/kernel.h>
 #include <linux/hid.h>
 #include <linux/mutex.h>
+#include <asm/unaligned.h>
 
 #include "../hid-ids.h"
 #include "i2c-hid.h"
@@ -47,6 +48,15 @@
 #define I2C_HID_QUIRK_BAD_INPUT_SIZE		BIT(6)
 #define I2C_HID_QUIRK_NO_WAKEUP_AFTER_RESET	BIT(7)
 
+/* Command opcodes */
+#define I2C_HID_OPCODE_RESET			0x01
+#define I2C_HID_OPCODE_GET_REPORT		0x02
+#define I2C_HID_OPCODE_SET_REPORT		0x03
+#define I2C_HID_OPCODE_GET_IDLE			0x04
+#define I2C_HID_OPCODE_SET_IDLE			0x05
+#define I2C_HID_OPCODE_GET_PROTOCOL		0x06
+#define I2C_HID_OPCODE_SET_PROTOCOL		0x07
+#define I2C_HID_OPCODE_SET_POWER		0x08
 
 /* flags */
 #define I2C_HID_STARTED		0
@@ -90,16 +100,6 @@ struct i2c_hid_cmd {
 	unsigned int length;
 };
 
-union command {
-	u8 data[0];
-	struct cmd {
-		__le16 reg;
-		__u8 reportTypeID;
-		__u8 opcode;
-		__u8 reportID;
-	} __packed c;
-};
-
 #define I2C_HID_CMD(opcode_) \
 	.opcode = opcode_, .length = 4, \
 	.registerIndex = offsetof(struct i2c_hid_desc, wCommandRegister)
@@ -115,9 +115,7 @@ static const struct i2c_hid_cmd hid_report_descr_cmd = {
 /* commands */
 static const struct i2c_hid_cmd hid_reset_cmd =		{ I2C_HID_CMD(0x01) };
 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_set_power_cmd =	{ I2C_HID_CMD(0x08) };
-static const struct i2c_hid_cmd hid_no_cmd =		{ .length = 0 };
 
 /*
  * These definitions are not used here, but are defined by the spec.
@@ -144,7 +142,6 @@ struct i2c_hid {
 	u8			*inbuf;		/* Input buffer */
 	u8			*rawbuf;	/* Raw Input buffer */
 	u8			*cmdbuf;	/* Command buffer */
-	u8			*argsbuf;	/* Command arguments buffer */
 
 	unsigned long		flags;		/* device flags */
 	unsigned long		quirks;		/* Various quirks */
@@ -206,67 +203,90 @@ static u32 i2c_hid_lookup_quirk(const u16 idVendor, const u16 idProduct)
 	return quirks;
 }
 
+static int i2c_hid_xfer(struct i2c_hid *ihid,
+			u8 *send_buf, int send_len, u8 *recv_buf, int recv_len)
+{
+	struct i2c_client *client = ihid->client;
+	struct i2c_msg msgs[2] = { 0 };
+	int n = 0;
+	int ret;
+
+	if (send_len) {
+		i2c_hid_dbg(ihid, "%s: cmd=%*ph\n",
+			    __func__, send_len, send_buf);
+
+		msgs[n].addr = client->addr;
+		msgs[n].flags = client->flags & I2C_M_TEN;
+		msgs[n].len = send_len;
+		msgs[n].buf = send_buf;
+		n++;
+	}
+
+	if (recv_len) {
+		msgs[n].addr = client->addr;
+		msgs[n].flags = (client->flags & I2C_M_TEN) | I2C_M_RD;
+		msgs[n].len = recv_len;
+		msgs[n].buf = recv_buf;
+		n++;
+
+		set_bit(I2C_HID_READ_PENDING, &ihid->flags);
+	}
+
+	ret = i2c_transfer(client->adapter, msgs, n);
+
+	if (recv_len)
+		clear_bit(I2C_HID_READ_PENDING, &ihid->flags);
+
+	if (ret != n)
+		return ret < 0 ? ret : -EIO;
+
+	return 0;
+}
+
+static size_t i2c_hid_encode_command(u8 *buf, u8 opcode,
+				     int report_type, int report_id)
+{
+	size_t length = 0;
+
+	if (report_id < 0x0F) {
+		buf[length++] = report_type << 4 | report_id;
+		buf[length++] = opcode;
+	} else {
+		buf[length++] = report_type << 4 | 0x0F;
+		buf[length++] = opcode;
+		buf[length++] = report_id;
+	}
+
+	return length;
+}
+
 static int __i2c_hid_command(struct i2c_hid *ihid,
 		const struct i2c_hid_cmd *command, u8 reportID,
 		u8 reportType, u8 *args, int args_len,
 		unsigned char *buf_recv, int data_len)
 {
-	struct i2c_client *client = ihid->client;
-	union command *cmd = (union command *)ihid->cmdbuf;
-	int ret;
-	struct i2c_msg msg[2];
-	int msg_num = 1;
-
 	int length = command->length;
 	unsigned int registerIndex = command->registerIndex;
 
 	/* special case for hid_descr_cmd */
 	if (command == &hid_descr_cmd) {
-		cmd->c.reg = ihid->wHIDDescRegister;
+		*(__le16 *)ihid->cmdbuf = ihid->wHIDDescRegister;
 	} else {
-		cmd->data[0] = ihid->hdesc_buffer[registerIndex];
-		cmd->data[1] = ihid->hdesc_buffer[registerIndex + 1];
+		ihid->cmdbuf[0] = ihid->hdesc_buffer[registerIndex];
+		ihid->cmdbuf[1] = ihid->hdesc_buffer[registerIndex + 1];
 	}
 
 	if (length > 2) {
-		cmd->c.opcode = command->opcode;
-		if (reportID < 0x0F) {
-			cmd->c.reportTypeID = reportType << 4 | reportID;
-		} else {
-			cmd->c.reportTypeID = reportType << 4 | 0x0F;
-			cmd->c.reportID = reportID;
-			length++;
-		}
+		length = sizeof(__le16) + /* register */
+			 i2c_hid_encode_command(ihid->cmdbuf + sizeof(__le16),
+						command->opcode,
+						reportType, reportID);
 	}
 
-	memcpy(cmd->data + length, args, args_len);
+	memcpy(ihid->cmdbuf + 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);
-	}
-
-	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;
-
-	return 0;
+	return i2c_hid_xfer(ihid, ihid->cmdbuf, length, buf_recv, data_len);
 }
 
 static int i2c_hid_command(struct i2c_hid *ihid,
@@ -301,70 +321,81 @@ static int i2c_hid_get_report(struct i2c_hid *ihid, u8 reportType,
 	return 0;
 }
 
+static size_t i2c_hid_format_report(u8 *buf, int report_id,
+				    const u8 *data, size_t size)
+{
+	size_t length = sizeof(__le16); /* reserve space to store size */
+
+	if (report_id)
+		buf[length++] = report_id;
+
+	memcpy(buf + length, data, size);
+	length += size;
+
+	/* Store overall size in the beginning of the buffer */
+	put_unaligned_le16(length, buf);
+
+	return length;
+}
+
 /**
  * i2c_hid_set_or_send_report: forward an incoming report to the device
  * @ihid: the i2c hid device
- * @reportType: 0x03 for HID_FEATURE_REPORT ; 0x02 for HID_OUTPUT_REPORT
- * @reportID: the report ID
+ * @report_type: 0x03 for HID_FEATURE_REPORT ; 0x02 for HID_OUTPUT_REPORT
+ * @report_id: the report ID
  * @buf: the actual data to transfer, without the report ID
  * @data_len: size of buf
- * @use_data: true: use SET_REPORT HID command, false: send plain OUTPUT report
+ * @do_set: true: use SET_REPORT HID command, false: send plain OUTPUT report
  */
-static int i2c_hid_set_or_send_report(struct i2c_hid *ihid, u8 reportType,
-		u8 reportID, unsigned char *buf, size_t data_len, bool use_data)
+static int i2c_hid_set_or_send_report(struct i2c_hid *ihid,
+				      u8 report_type, u8 report_id,
+				      const u8 *buf, size_t data_len,
+				      bool do_set)
 {
-	u8 *args = ihid->argsbuf;
-	const struct i2c_hid_cmd *hidcmd;
-	int ret;
-	u16 dataRegister = le16_to_cpu(ihid->hdesc.wDataRegister);
-	u16 outputRegister = le16_to_cpu(ihid->hdesc.wOutputRegister);
-	u16 maxOutputLength = le16_to_cpu(ihid->hdesc.wMaxOutputLength);
-	u16 size;
-	int args_len;
-	int index = 0;
+	size_t length = 0;
+	int error;
 
 	i2c_hid_dbg(ihid, "%s\n", __func__);
 
 	if (data_len > ihid->bufsize)
 		return -EINVAL;
 
-	size =		2			/* size */ +
-			(reportID ? 1 : 0)	/* reportID */ +
-			data_len		/* buf */;
-	args_len =	2			/* dataRegister */ +
-			size			/* args */;
-
-	if (!use_data && maxOutputLength == 0)
+	if (!do_set && le16_to_cpu(ihid->hdesc.wMaxOutputLength) == 0)
 		return -ENOSYS;
 
-	/*
-	 * use the data register for feature reports or if the device does not
-	 * support the output register
-	 */
-	if (use_data) {
-		args[index++] = dataRegister & 0xFF;
-		args[index++] = dataRegister >> 8;
-		hidcmd = &hid_set_report_cmd;
+	if (do_set) {
+		/* Command register goes first */
+		*(__le16 *)ihid->cmdbuf = ihid->hdesc.wCommandRegister;
+		length += sizeof(__le16);
+		/* Next is SET_REPORT command */
+		length += i2c_hid_encode_command(ihid->cmdbuf + length,
+						 I2C_HID_OPCODE_SET_REPORT,
+						 report_type, report_id);
+		/*
+		 * Report data will go into the data register. Because
+		 * command can be either 2 or 3 bytes destination for
+		 * the data register may be not aligned.
+		*/
+		put_unaligned_le16(le16_to_cpu(ihid->hdesc.wDataRegister),
+				   ihid->cmdbuf + length);
+		length += sizeof(__le16);
 	} else {
-		args[index++] = outputRegister & 0xFF;
-		args[index++] = outputRegister >> 8;
-		hidcmd = &hid_no_cmd;
+		/*
+		 * With simple "send report" all data goes into the output
+		 * register.
+		 */
+		*(__le16 *)ihid->cmdbuf = ihid->hdesc.wCommandRegister;
+		length += sizeof(__le16);
 	}
 
-	args[index++] = size & 0xFF;
-	args[index++] = size >> 8;
-
-	if (reportID)
-		args[index++] = reportID;
+	length += i2c_hid_format_report(ihid->cmdbuf + length,
+					report_id, buf, data_len);
 
-	memcpy(&args[index], buf, data_len);
-
-	ret = __i2c_hid_command(ihid, hidcmd, reportID,
-		reportType, args, args_len, NULL, 0);
-	if (ret) {
+	error = i2c_hid_xfer(ihid, ihid->cmdbuf, length, NULL, 0);
+	if (error) {
 		dev_err(&ihid->client->dev,
-			"failed to set a report to device.\n");
-		return ret;
+			"failed to set a report to device: %d\n", error);
+		return error;
 	}
 
 	return data_len;
@@ -575,31 +606,33 @@ static void i2c_hid_free_buffers(struct i2c_hid *ihid)
 {
 	kfree(ihid->inbuf);
 	kfree(ihid->rawbuf);
-	kfree(ihid->argsbuf);
 	kfree(ihid->cmdbuf);
 	ihid->inbuf = NULL;
 	ihid->rawbuf = 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) + /* ReportID */
-		       sizeof(__u8) + /* optional ReportID byte */
-		       sizeof(__u16) + /* data register */
-		       sizeof(__u16) + /* size of the report */
-		       report_size; /* report */
+	/*
+	 * The worst case is computed from the set_report command with a
+	 * reportID > 15 and the maximum report length.
+	 */
+	int cmd_len = sizeof(__le16) +	/* command register */
+		      sizeof(u8) +	/* encoded report type/ID */
+		      sizeof(u8) +	/* opcode */
+		      sizeof(u8) +	/* optional 3rd byte report ID */
+		      sizeof(__le16) +	/* data register */
+		      sizeof(__le16) +	/* report data size */
+		      sizeof(u8) +	/* report ID if numbered report */
+		      report_size;
 
 	ihid->inbuf = kzalloc(report_size, GFP_KERNEL);
 	ihid->rawbuf = kzalloc(report_size, GFP_KERNEL);
-	ihid->argsbuf = kzalloc(args_len, GFP_KERNEL);
-	ihid->cmdbuf = kzalloc(sizeof(union command) + args_len, GFP_KERNEL);
+	ihid->cmdbuf = kzalloc(cmd_len, GFP_KERNEL);
 
-	if (!ihid->inbuf || !ihid->rawbuf || !ihid->argsbuf || !ihid->cmdbuf) {
+	if (!ihid->inbuf || !ihid->rawbuf || !ihid->cmdbuf) {
 		i2c_hid_free_buffers(ihid);
 		return -ENOMEM;
 	}
@@ -659,8 +692,9 @@ static int i2c_hid_get_raw_report(struct hid_device *hid,
 	return count;
 }
 
-static int i2c_hid_output_raw_report(struct hid_device *hid, __u8 *buf,
-		size_t count, unsigned char report_type, bool use_data)
+static int i2c_hid_output_raw_report(struct hid_device *hid,
+				     const u8 *buf, size_t count,
+				     u8 report_type, bool do_set)
 {
 	struct i2c_client *client = hid->driver_data;
 	struct i2c_hid *ihid = i2c_get_clientdata(client);
@@ -681,7 +715,7 @@ static int i2c_hid_output_raw_report(struct hid_device *hid, __u8 *buf,
 	 */
 	ret = i2c_hid_set_or_send_report(ihid,
 				report_type == HID_FEATURE_REPORT ? 0x03 : 0x02,
-				report_id, buf + 1, count - 1, use_data);
+				report_id, buf + 1, count - 1, do_set);
 
 	if (ret >= 0)
 		ret++; /* add report_id to the number of transferred bytes */
@@ -691,11 +725,10 @@ static int i2c_hid_output_raw_report(struct hid_device *hid, __u8 *buf,
 	return ret;
 }
 
-static int i2c_hid_output_report(struct hid_device *hid, __u8 *buf,
-		size_t count)
+static int i2c_hid_output_report(struct hid_device *hid, u8 *buf, size_t count)
 {
 	return i2c_hid_output_raw_report(hid, buf, count, HID_OUTPUT_REPORT,
-			false);
+					 false);
 }
 
 static int i2c_hid_raw_request(struct hid_device *hid, unsigned char reportnum,
-- 
2.34.1.703.g22d0c6ccf7-goog

[PATCH 06/12] HID: i2c-hid: define i2c_hid_read_register() and use it

[Dmitry Torokhov]

Handling simple read of device registers in __i2c_hid_command() makes it
too complicated and the need of special handling for the HID descriptor
register adds even more complexity. Instead, let's create simple
i2c_hid_read_register() helper on base of i2c_hid_xfer() and use it.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 45 +++++++++++++++---------------
 1 file changed, 22 insertions(+), 23 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index c48b75bd81e0..b1a2c6ad374d 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -104,14 +104,6 @@ struct i2c_hid_cmd {
 	.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) };
 static const struct i2c_hid_cmd hid_get_report_cmd =	{ I2C_HID_CMD(0x02) };
@@ -243,6 +235,14 @@ static int i2c_hid_xfer(struct i2c_hid *ihid,
 	return 0;
 }
 
+static int i2c_hid_read_register(struct i2c_hid *ihid, __le16 reg,
+				 void *buf, size_t len)
+{
+	*(__le16 *)ihid->cmdbuf = reg;
+
+	return i2c_hid_xfer(ihid, ihid->cmdbuf, sizeof(__le16), buf, len);
+}
+
 static size_t i2c_hid_encode_command(u8 *buf, u8 opcode,
 				     int report_type, int report_id)
 {
@@ -268,13 +268,8 @@ static int __i2c_hid_command(struct i2c_hid *ihid,
 	int length = command->length;
 	unsigned int registerIndex = command->registerIndex;
 
-	/* special case for hid_descr_cmd */
-	if (command == &hid_descr_cmd) {
-		*(__le16 *)ihid->cmdbuf = ihid->wHIDDescRegister;
-	} else {
-		ihid->cmdbuf[0] = ihid->hdesc_buffer[registerIndex];
-		ihid->cmdbuf[1] = ihid->hdesc_buffer[registerIndex + 1];
-	}
+	ihid->cmdbuf[0] = ihid->hdesc_buffer[registerIndex];
+	ihid->cmdbuf[1] = ihid->hdesc_buffer[registerIndex + 1];
 
 	if (length > 2) {
 		length = sizeof(__le16) + /* register */
@@ -791,8 +786,9 @@ static int i2c_hid_parse(struct hid_device *hid)
 
 		i2c_hid_dbg(ihid, "asking HID report descriptor\n");
 
-		ret = i2c_hid_command(ihid, &hid_report_descr_cmd,
-				      rdesc, rsize);
+		ret = i2c_hid_read_register(ihid,
+					    ihid->hdesc.wReportDescRegister,
+					    rdesc, rsize);
 		if (ret) {
 			hid_err(hid, "reading report descriptor failed\n");
 			kfree(rdesc);
@@ -902,7 +898,7 @@ static int 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;
+	int error;
 
 	/* i2c hid fetch using a fixed descriptor size (30 bytes) */
 	if (i2c_hid_get_dmi_i2c_hid_desc_override(client->name)) {
@@ -911,11 +907,14 @@ static int i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
 			*i2c_hid_get_dmi_i2c_hid_desc_override(client->name);
 	} else {
 		i2c_hid_dbg(ihid, "Fetching the HID descriptor\n");
-		ret = i2c_hid_command(ihid, &hid_descr_cmd,
-				      ihid->hdesc_buffer,
-				      sizeof(struct i2c_hid_desc));
-		if (ret) {
-			dev_err(&ihid->client->dev, "hid_descr_cmd failed\n");
+		error = i2c_hid_read_register(ihid,
+					      ihid->wHIDDescRegister,
+					      &ihid->hdesc,
+					      sizeof(ihid->hdesc));
+		if (error) {
+			dev_err(&ihid->client->dev,
+				"failed to fetch HID descriptor: %d\n",
+				error);
 			return -ENODEV;
 		}
 	}
-- 
2.34.1.703.g22d0c6ccf7-goog

[PATCH 07/12] HID: i2c-hid: create a helper for SET_POWER command

[Dmitry Torokhov]

Another case where creating a dedicated helper allows for cleaner code that
shows exactly what communication happens with the device when toggling its
power.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index b1a2c6ad374d..da673e3f2910 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -107,7 +107,6 @@ struct i2c_hid_cmd {
 /* commands */
 static const struct i2c_hid_cmd hid_reset_cmd =		{ I2C_HID_CMD(0x01) };
 static const struct i2c_hid_cmd hid_get_report_cmd =	{ I2C_HID_CMD(0x02) };
-static const struct i2c_hid_cmd hid_set_power_cmd =	{ I2C_HID_CMD(0x08) };
 
 /*
  * These definitions are not used here, but are defined by the spec.
@@ -396,6 +395,22 @@ static int i2c_hid_set_or_send_report(struct i2c_hid *ihid,
 	return data_len;
 }
 
+static int i2c_hid_set_power_command(struct i2c_hid *ihid, int power_state)
+{
+	size_t length;
+
+	/* SET_POWER uses command register */
+	*(__le16 *)ihid->cmdbuf = ihid->hdesc.wCommandRegister;
+	length = sizeof(__le16);
+
+	/* Now the command itself */
+	length += i2c_hid_encode_command(ihid->cmdbuf + length,
+					 I2C_HID_OPCODE_SET_POWER,
+					 0, power_state);
+
+	return i2c_hid_xfer(ihid, ihid->cmdbuf, length, NULL, 0);
+}
+
 static int i2c_hid_set_power(struct i2c_hid *ihid, int power_state)
 {
 	int ret;
@@ -409,15 +424,14 @@ static int i2c_hid_set_power(struct i2c_hid *ihid, int power_state)
 	 */
 	if (power_state == I2C_HID_PWR_ON &&
 	    ihid->quirks & I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV) {
-		ret = i2c_hid_command(ihid, &hid_set_power_cmd, NULL, 0);
+		ret = i2c_hid_set_power_command(ihid, I2C_HID_PWR_ON);
 
 		/* Device was already activated */
 		if (!ret)
 			goto set_pwr_exit;
 	}
 
-	ret = __i2c_hid_command(ihid, &hid_set_power_cmd, power_state,
-		0, NULL, 0, NULL, 0);
+	ret = i2c_hid_set_power_command(ihid, power_state);
 	if (ret)
 		dev_err(&ihid->client->dev,
 			"failed to change power setting.\n");
-- 
2.34.1.703.g22d0c6ccf7-goog

[PATCH 08/12] HID: i2c-hid: convert i2c_hid_execute_reset() to use i2c_hid_xfer()

[Dmitry Torokhov]

This will allow us to drop i2c_hid_command() wrapper and get close
to removing __i2c_hid_command().

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index da673e3f2910..1515fc892e61 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -105,7 +105,6 @@ struct i2c_hid_cmd {
 	.registerIndex = offsetof(struct i2c_hid_desc, wCommandRegister)
 
 /* commands */
-static const struct i2c_hid_cmd hid_reset_cmd =		{ I2C_HID_CMD(0x01) };
 static const struct i2c_hid_cmd hid_get_report_cmd =	{ I2C_HID_CMD(0x02) };
 
 /*
@@ -283,14 +282,6 @@ static int __i2c_hid_command(struct i2c_hid *ihid,
 	return i2c_hid_xfer(ihid, ihid->cmdbuf, length, buf_recv, data_len);
 }
 
-static int i2c_hid_command(struct i2c_hid *ihid,
-		const struct i2c_hid_cmd *command,
-		unsigned char *buf_recv, int data_len)
-{
-	return __i2c_hid_command(ihid, command, 0, 0, NULL, 0,
-				buf_recv, data_len);
-}
-
 static int i2c_hid_get_report(struct i2c_hid *ihid, u8 reportType,
 		u8 reportID, unsigned char *buf_recv, int data_len)
 {
@@ -455,13 +446,21 @@ static int i2c_hid_set_power(struct i2c_hid *ihid, int power_state)
 
 static int i2c_hid_execute_reset(struct i2c_hid *ihid)
 {
+	size_t length = 0;
 	int ret;
 
 	i2c_hid_dbg(ihid, "resetting...\n");
 
+	/* Prepare reset command. Command register goes first. */
+	*(__le16 *)ihid->cmdbuf = ihid->hdesc.wCommandRegister;
+	length += sizeof(__le16);
+	/* Next is RESET command itself */
+	length += i2c_hid_encode_command(ihid->cmdbuf + length,
+					 I2C_HID_OPCODE_RESET, 0, 0);
+
 	set_bit(I2C_HID_RESET_PENDING, &ihid->flags);
 
-	ret = i2c_hid_command(ihid, &hid_reset_cmd, NULL, 0);
+	ret = i2c_hid_xfer(ihid, ihid->cmdbuf, length, NULL, 0);
 	if (ret) {
 		dev_err(&ihid->client->dev, "failed to reset device.\n");
 		goto out;
-- 
2.34.1.703.g22d0c6ccf7-goog

[PATCH 09/12] HID: i2c-hid: rework i2c_hid_get_report() to use i2c_hid_xfer()

[Dmitry Torokhov]

Explicitly prepare command for i2c_hid_get_report() which makes the logic
clearer and allows us to get rid of __i2c_hid_command() and related command
definitions.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 150 ++++++++++++-----------------
 1 file changed, 59 insertions(+), 91 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 1515fc892e61..433b6692f277 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -94,29 +94,6 @@ struct i2c_hid_desc {
 	__le32 reserved;
 } __packed;
 
-struct i2c_hid_cmd {
-	unsigned int registerIndex;
-	__u8 opcode;
-	unsigned int length;
-};
-
-#define I2C_HID_CMD(opcode_) \
-	.opcode = opcode_, .length = 4, \
-	.registerIndex = offsetof(struct i2c_hid_desc, wCommandRegister)
-
-/* commands */
-static const struct i2c_hid_cmd hid_get_report_cmd =	{ I2C_HID_CMD(0x02) };
-
-/*
- * 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 {
 	struct i2c_client	*client;	/* i2c client */
@@ -258,52 +235,62 @@ static size_t i2c_hid_encode_command(u8 *buf, u8 opcode,
 	return length;
 }
 
-static int __i2c_hid_command(struct i2c_hid *ihid,
-		const struct i2c_hid_cmd *command, u8 reportID,
-		u8 reportType, u8 *args, int args_len,
-		unsigned char *buf_recv, int data_len)
+static int i2c_hid_get_report(struct i2c_hid *ihid,
+			      u8 report_type, u8 report_id,
+			      u8 *recv_buf, size_t recv_len)
 {
-	int length = command->length;
-	unsigned int registerIndex = command->registerIndex;
-
-	ihid->cmdbuf[0] = ihid->hdesc_buffer[registerIndex];
-	ihid->cmdbuf[1] = ihid->hdesc_buffer[registerIndex + 1];
+	size_t length = 0;
+	size_t ret_count;
+	int error;
 
-	if (length > 2) {
-		length = sizeof(__le16) + /* register */
-			 i2c_hid_encode_command(ihid->cmdbuf + sizeof(__le16),
-						command->opcode,
-						reportType, reportID);
-	}
+	i2c_hid_dbg(ihid, "%s\n", __func__);
 
-	memcpy(ihid->cmdbuf + length, args, args_len);
-	length += args_len;
+	/* Command register goes first */
+	*(__le16 *)ihid->cmdbuf = ihid->hdesc.wCommandRegister;
+	length += sizeof(__le16);
+	/* Next is GET_REPORT command */
+	length += i2c_hid_encode_command(ihid->cmdbuf + length,
+					 I2C_HID_OPCODE_GET_REPORT,
+					 report_type, report_id);
+	/*
+	 * Device will send report data through data register. Because
+	 * command can be either 2 or 3 bytes destination for the data
+	 * register may be not aligned.
+	 */
+	put_unaligned_le16(le16_to_cpu(ihid->hdesc.wDataRegister),
+			   ihid->cmdbuf + length);
+	length += sizeof(__le16);
 
-	return i2c_hid_xfer(ihid, ihid->cmdbuf, length, buf_recv, data_len);
-}
+	/*
+	 * In addition to report data device will supply data length
+	 * in the first 2 bytes of the response, so adjust .
+	 */
+	error = i2c_hid_xfer(ihid, ihid->cmdbuf, length,
+			     ihid->rawbuf, recv_len + sizeof(__le16));
+	if (error) {
+		dev_err(&ihid->client->dev,
+			"failed to set a report to device: %d\n", error);
+		return error;
+	}
 
-static int i2c_hid_get_report(struct i2c_hid *ihid, u8 reportType,
-		u8 reportID, unsigned char *buf_recv, int data_len)
-{
-	u8 args[2];
-	int ret;
-	int args_len = 0;
-	u16 readRegister = le16_to_cpu(ihid->hdesc.wDataRegister);
+	/* The buffer is sufficiently aligned */
+	ret_count = le16_to_cpup((__le16 *)ihid->rawbuf);
 
-	i2c_hid_dbg(ihid, "%s\n", __func__);
+	/* Check for empty report response */
+	if (ret_count <= sizeof(__le16))
+		return 0;
 
-	args[args_len++] = readRegister & 0xFF;
-	args[args_len++] = readRegister >> 8;
+	recv_len = min(recv_len, ret_count - sizeof(__le16));
+	memcpy(recv_buf, ihid->rawbuf + sizeof(__le16), recv_len);
 
-	ret = __i2c_hid_command(ihid, &hid_get_report_cmd, reportID,
-		reportType, args, args_len, buf_recv, data_len);
-	if (ret) {
+	if (report_id && recv_len != 0 && recv_buf[0] != report_id) {
 		dev_err(&ihid->client->dev,
-			"failed to retrieve report from device.\n");
-		return ret;
+			"device returned incorrect report (%d vs %d expected)\n",
+			recv_buf[0], report_id);
+		return -EINVAL;
 	}
 
-	return 0;
+	return recv_len;
 }
 
 static size_t i2c_hid_format_report(u8 *buf, int report_id,
@@ -651,13 +638,12 @@ static int i2c_hid_alloc_buffers(struct i2c_hid *ihid, size_t report_size)
 }
 
 static int i2c_hid_get_raw_report(struct hid_device *hid,
-		unsigned char report_number, __u8 *buf, size_t count,
-		unsigned char report_type)
+				  u8 report_type, u8 report_id,
+				  u8 *buf, size_t count)
 {
 	struct i2c_client *client = hid->driver_data;
 	struct i2c_hid *ihid = i2c_get_clientdata(client);
-	size_t ret_count, ask_count;
-	int ret;
+	int ret_count;
 
 	if (report_type == HID_OUTPUT_REPORT)
 		return -EINVAL;
@@ -667,42 +653,24 @@ static int i2c_hid_get_raw_report(struct hid_device *hid,
 	 * not have the report ID that the upper layers expect, so we need
 	 * to stash it the buffer ourselves and adjust the data size.
 	 */
-	if (!report_number) {
+	if (!report_id) {
 		buf[0] = 0;
 		buf++;
 		count--;
 	}
 
-	/* +2 bytes to include the size of the reply in the query buffer */
-	ask_count = min(count + 2, (size_t)ihid->bufsize);
-
-	ret = i2c_hid_get_report(ihid,
+	ret_count = i2c_hid_get_report(ihid,
 			report_type == HID_FEATURE_REPORT ? 0x03 : 0x01,
-			report_number, ihid->rawbuf, ask_count);
-
-	if (ret < 0)
-		return ret;
-
-	ret_count = ihid->rawbuf[0] | (ihid->rawbuf[1] << 8);
-
-	if (ret_count <= 2)
-		return 0;
-
-	ret_count = min(ret_count, ask_count);
-
-	/* The query buffer contains the size, dropping it in the reply */
-	count = min(count, ret_count - 2);
-	memcpy(buf, ihid->rawbuf + 2, count);
+			report_id, buf, count);
 
-	if (!report_number)
-		count++;
+	if (ret_count > 0 && !report_id)
+		ret_count++;
 
-	return count;
+	return ret_count;
 }
 
-static int i2c_hid_output_raw_report(struct hid_device *hid,
-				     const u8 *buf, size_t count,
-				     u8 report_type, bool do_set)
+static int i2c_hid_output_raw_report(struct hid_device *hid, u8 report_type,
+				     const u8 *buf, size_t count, bool do_set)
 {
 	struct i2c_client *client = hid->driver_data;
 	struct i2c_hid *ihid = i2c_get_clientdata(client);
@@ -735,7 +703,7 @@ static int i2c_hid_output_raw_report(struct hid_device *hid,
 
 static int i2c_hid_output_report(struct hid_device *hid, u8 *buf, size_t count)
 {
-	return i2c_hid_output_raw_report(hid, buf, count, HID_OUTPUT_REPORT,
+	return i2c_hid_output_raw_report(hid, HID_OUTPUT_REPORT, buf, count,
 					 false);
 }
 
@@ -745,11 +713,11 @@ static int i2c_hid_raw_request(struct hid_device *hid, unsigned char reportnum,
 {
 	switch (reqtype) {
 	case HID_REQ_GET_REPORT:
-		return i2c_hid_get_raw_report(hid, reportnum, buf, len, rtype);
+		return i2c_hid_get_raw_report(hid, rtype, reportnum, buf, len);
 	case HID_REQ_SET_REPORT:
 		if (buf[0] != reportnum)
 			return -EINVAL;
-		return i2c_hid_output_raw_report(hid, buf, len, rtype, true);
+		return i2c_hid_output_raw_report(hid, rtype, buf, len, true);
 	default:
 		return -EIO;
 	}
-- 
2.34.1.703.g22d0c6ccf7-goog

[PATCH 10/12] HID: i2c-hid: use helpers to do endian conversion in i2c_hid_get_input()

[Dmitry Torokhov]

It is better to use helpers to do endian conversion as it documents
and draws attention to it, and might be a bit more performant as
well.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 433b6692f277..07c2ea057013 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -508,9 +508,9 @@ static int i2c_hid_hwreset(struct i2c_hid *ihid)
 
 static void i2c_hid_get_input(struct i2c_hid *ihid)
 {
+	u16 size = le16_to_cpu(ihid->hdesc.wMaxInputLength);
+	u16 ret_size;
 	int ret;
-	u32 ret_size;
-	int size = le16_to_cpu(ihid->hdesc.wMaxInputLength);
 
 	if (size > ihid->bufsize)
 		size = ihid->bufsize;
@@ -525,8 +525,8 @@ static void i2c_hid_get_input(struct i2c_hid *ihid)
 		return;
 	}
 
-	ret_size = ihid->inbuf[0] | ihid->inbuf[1] << 8;
-
+	/* Receiving buffer is properly aligned */
+	ret_size = le16_to_cpup((__le16 *)ihid->inbuf);
 	if (!ret_size) {
 		/* host or device initiated RESET completed */
 		if (test_and_clear_bit(I2C_HID_RESET_PENDING, &ihid->flags))
@@ -534,19 +534,20 @@ static void i2c_hid_get_input(struct i2c_hid *ihid)
 		return;
 	}
 
-	if (ihid->quirks & I2C_HID_QUIRK_BOGUS_IRQ && ret_size == 0xffff) {
-		dev_warn_once(&ihid->client->dev, "%s: IRQ triggered but "
-			      "there's no data\n", __func__);
+	if ((ihid->quirks & I2C_HID_QUIRK_BOGUS_IRQ) && ret_size == 0xffff) {
+		dev_warn_once(&ihid->client->dev,
+			      "%s: IRQ triggered but there's no data\n",
+			      __func__);
 		return;
 	}
 
-	if ((ret_size > size) || (ret_size < 2)) {
+	if (ret_size > size || ret_size < sizeof(__le16)) {
 		if (ihid->quirks & I2C_HID_QUIRK_BAD_INPUT_SIZE) {
-			ihid->inbuf[0] = size & 0xff;
-			ihid->inbuf[1] = size >> 8;
+			*(__le16 *)ihid->inbuf = cpu_to_le16(size);
 			ret_size = size;
 		} else {
-			dev_err(&ihid->client->dev, "%s: incomplete report (%d/%d)\n",
+			dev_err(&ihid->client->dev,
+				"%s: incomplete report (%d/%d)\n",
 				__func__, size, ret_size);
 			return;
 		}
@@ -555,10 +556,9 @@ static void i2c_hid_get_input(struct i2c_hid *ihid)
 	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;
+		hid_input_report(ihid->hid, HID_INPUT_REPORT,
+				 ihid->inbuf + sizeof(__le16),
+				 ret_size - sizeof(__le16), 1);
 }
 
 static irqreturn_t i2c_hid_irq(int irq, void *dev_id)
-- 
2.34.1.703.g22d0c6ccf7-goog

[PATCH 11/12] HID: i2c-hid: no longer need raw access to HID descriptor structure

[Dmitry Torokhov]

We can stop defining a union for HID descriptor data as we now only access
individual members of it by names and using proper types instead of
accessing by offset from the beginning of the data structure.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 07c2ea057013..aa7c573b35bc 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -98,10 +98,7 @@ struct i2c_hid_desc {
 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 */
-	};
+	struct i2c_hid_desc hdesc;		/* the HID Descriptor */
 	__le16			wHIDDescRegister; /* location of the i2c
 						   * register of the HID
 						   * descriptor. */
@@ -918,7 +915,7 @@ static int i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
 			"weird size of HID descriptor (%u)\n", dsize);
 		return -ENODEV;
 	}
-	i2c_hid_dbg(ihid, "HID Descriptor: %*ph\n", dsize, ihid->hdesc_buffer);
+	i2c_hid_dbg(ihid, "HID Descriptor: %*ph\n", dsize, &ihid->hdesc);
 	return 0;
 }
 
-- 
2.34.1.703.g22d0c6ccf7-goog

[PATCH 12/12] HID: i2c-hid: note that I2C xfer buffers are DMA-safe

[Dmitry Torokhov]

All I2C communications in the driver use driver-private buffers that are
DMA-safe, so mark them as such.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index aa7c573b35bc..92dd86c42975 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -180,7 +180,7 @@ static int i2c_hid_xfer(struct i2c_hid *ihid,
 			    __func__, send_len, send_buf);
 
 		msgs[n].addr = client->addr;
-		msgs[n].flags = client->flags & I2C_M_TEN;
+		msgs[n].flags = (client->flags & I2C_M_TEN) | I2C_M_DMA_SAFE;
 		msgs[n].len = send_len;
 		msgs[n].buf = send_buf;
 		n++;
@@ -188,7 +188,8 @@ static int i2c_hid_xfer(struct i2c_hid *ihid,
 
 	if (recv_len) {
 		msgs[n].addr = client->addr;
-		msgs[n].flags = (client->flags & I2C_M_TEN) | I2C_M_RD;
+		msgs[n].flags = (client->flags & I2C_M_TEN) |
+				I2C_M_RD | I2C_M_DMA_SAFE;
 		msgs[n].len = recv_len;
 		msgs[n].buf = recv_buf;
 		n++;
-- 
2.34.1.703.g22d0c6ccf7-goog

Re: [PATCH 05/12] HID: i2c-hid: explicitly code setting and sending reports

[Angela Czubak]

Hi Dmitry,

On Tue, Jan 18, 2022 at 8:26 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Instead of relying on __i2c_hid_command() that tries to handle all
> commands and because of that is very complicated, let's define a
> new dumb helper i2c_hid_xfer() that actually transfers (write and
> read) data, and use it when sending and setting reports. By doing
> that we can save on number of copy operations we have to execute,
> and make logic of sending reports much clearer.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/hid/i2c-hid/i2c-hid-core.c | 269 ++++++++++++++++-------------
>  1 file changed, 151 insertions(+), 118 deletions(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> index 6c1741d9211d..c48b75bd81e0 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -35,6 +35,7 @@
>  #include <linux/kernel.h>
>  #include <linux/hid.h>
>  #include <linux/mutex.h>
> +#include <asm/unaligned.h>
>
>  #include "../hid-ids.h"
>  #include "i2c-hid.h"
> @@ -47,6 +48,15 @@
>  #define I2C_HID_QUIRK_BAD_INPUT_SIZE           BIT(6)
>  #define I2C_HID_QUIRK_NO_WAKEUP_AFTER_RESET    BIT(7)
>
> +/* Command opcodes */
> +#define I2C_HID_OPCODE_RESET                   0x01
> +#define I2C_HID_OPCODE_GET_REPORT              0x02
> +#define I2C_HID_OPCODE_SET_REPORT              0x03
> +#define I2C_HID_OPCODE_GET_IDLE                        0x04
> +#define I2C_HID_OPCODE_SET_IDLE                        0x05
> +#define I2C_HID_OPCODE_GET_PROTOCOL            0x06
> +#define I2C_HID_OPCODE_SET_PROTOCOL            0x07
> +#define I2C_HID_OPCODE_SET_POWER               0x08
>
>  /* flags */
>  #define I2C_HID_STARTED                0
> @@ -90,16 +100,6 @@ struct i2c_hid_cmd {
>         unsigned int length;
>  };
>
> -union command {
> -       u8 data[0];
> -       struct cmd {
> -               __le16 reg;
> -               __u8 reportTypeID;
> -               __u8 opcode;
> -               __u8 reportID;
> -       } __packed c;
> -};
> -
>  #define I2C_HID_CMD(opcode_) \
>         .opcode = opcode_, .length = 4, \
>         .registerIndex = offsetof(struct i2c_hid_desc, wCommandRegister)
> @@ -115,9 +115,7 @@ static const struct i2c_hid_cmd hid_report_descr_cmd = {
>  /* commands */
>  static const struct i2c_hid_cmd hid_reset_cmd =                { I2C_HID_CMD(0x01) };
>  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_set_power_cmd =    { I2C_HID_CMD(0x08) };
> -static const struct i2c_hid_cmd hid_no_cmd =           { .length = 0 };
>
>  /*
>   * These definitions are not used here, but are defined by the spec.
> @@ -144,7 +142,6 @@ struct i2c_hid {
>         u8                      *inbuf;         /* Input buffer */
>         u8                      *rawbuf;        /* Raw Input buffer */
>         u8                      *cmdbuf;        /* Command buffer */
> -       u8                      *argsbuf;       /* Command arguments buffer */
>
>         unsigned long           flags;          /* device flags */
>         unsigned long           quirks;         /* Various quirks */
> @@ -206,67 +203,90 @@ static u32 i2c_hid_lookup_quirk(const u16 idVendor, const u16 idProduct)
>         return quirks;
>  }
>
> +static int i2c_hid_xfer(struct i2c_hid *ihid,
> +                       u8 *send_buf, int send_len, u8 *recv_buf, int recv_len)
> +{
> +       struct i2c_client *client = ihid->client;
> +       struct i2c_msg msgs[2] = { 0 };
> +       int n = 0;
> +       int ret;
> +
> +       if (send_len) {
> +               i2c_hid_dbg(ihid, "%s: cmd=%*ph\n",
> +                           __func__, send_len, send_buf);
> +
> +               msgs[n].addr = client->addr;
> +               msgs[n].flags = client->flags & I2C_M_TEN;
> +               msgs[n].len = send_len;
> +               msgs[n].buf = send_buf;
> +               n++;
> +       }
> +
> +       if (recv_len) {
> +               msgs[n].addr = client->addr;
> +               msgs[n].flags = (client->flags & I2C_M_TEN) | I2C_M_RD;
> +               msgs[n].len = recv_len;
> +               msgs[n].buf = recv_buf;
> +               n++;
> +
> +               set_bit(I2C_HID_READ_PENDING, &ihid->flags);
> +       }
> +
> +       ret = i2c_transfer(client->adapter, msgs, n);
> +
> +       if (recv_len)
> +               clear_bit(I2C_HID_READ_PENDING, &ihid->flags);
> +
> +       if (ret != n)
> +               return ret < 0 ? ret : -EIO;
> +
> +       return 0;
> +}
> +
> +static size_t i2c_hid_encode_command(u8 *buf, u8 opcode,
> +                                    int report_type, int report_id)

Can it ever be used to encode something else than the command register?
If not, perhaps we could fill it here as well.

> +{
> +       size_t length = 0;
> +
> +       if (report_id < 0x0F) {
> +               buf[length++] = report_type << 4 | report_id;
> +               buf[length++] = opcode;
> +       } else {
> +               buf[length++] = report_type << 4 | 0x0F;
> +               buf[length++] = opcode;
> +               buf[length++] = report_id;
> +       }
> +
> +       return length;
> +}
> +
>  static int __i2c_hid_command(struct i2c_hid *ihid,
>                 const struct i2c_hid_cmd *command, u8 reportID,
>                 u8 reportType, u8 *args, int args_len,
>                 unsigned char *buf_recv, int data_len)
>  {
> -       struct i2c_client *client = ihid->client;
> -       union command *cmd = (union command *)ihid->cmdbuf;
> -       int ret;
> -       struct i2c_msg msg[2];
> -       int msg_num = 1;
> -
>         int length = command->length;
>         unsigned int registerIndex = command->registerIndex;
>
>         /* special case for hid_descr_cmd */
>         if (command == &hid_descr_cmd) {
> -               cmd->c.reg = ihid->wHIDDescRegister;
> +               *(__le16 *)ihid->cmdbuf = ihid->wHIDDescRegister;
>         } else {
> -               cmd->data[0] = ihid->hdesc_buffer[registerIndex];
> -               cmd->data[1] = ihid->hdesc_buffer[registerIndex + 1];
> +               ihid->cmdbuf[0] = ihid->hdesc_buffer[registerIndex];
> +               ihid->cmdbuf[1] = ihid->hdesc_buffer[registerIndex + 1];
>         }
>
>         if (length > 2) {
> -               cmd->c.opcode = command->opcode;
> -               if (reportID < 0x0F) {
> -                       cmd->c.reportTypeID = reportType << 4 | reportID;
> -               } else {
> -                       cmd->c.reportTypeID = reportType << 4 | 0x0F;
> -                       cmd->c.reportID = reportID;
> -                       length++;
> -               }
> +               length = sizeof(__le16) + /* register */
> +                        i2c_hid_encode_command(ihid->cmdbuf + sizeof(__le16),
> +                                               command->opcode,
> +                                               reportType, reportID);
>         }
>
> -       memcpy(cmd->data + length, args, args_len);
> +       memcpy(ihid->cmdbuf + 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);
> -       }
> -
> -       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;
> -
> -       return 0;
> +       return i2c_hid_xfer(ihid, ihid->cmdbuf, length, buf_recv, data_len);
>  }
>
>  static int i2c_hid_command(struct i2c_hid *ihid,
> @@ -301,70 +321,81 @@ static int i2c_hid_get_report(struct i2c_hid *ihid, u8 reportType,
>         return 0;
>  }
>
> +static size_t i2c_hid_format_report(u8 *buf, int report_id,
> +                                   const u8 *data, size_t size)
> +{
> +       size_t length = sizeof(__le16); /* reserve space to store size */
> +
> +       if (report_id)
> +               buf[length++] = report_id;
> +
> +       memcpy(buf + length, data, size);
> +       length += size;
> +
> +       /* Store overall size in the beginning of the buffer */
> +       put_unaligned_le16(length, buf);
> +
> +       return length;
> +}
> +
>  /**
>   * i2c_hid_set_or_send_report: forward an incoming report to the device
>   * @ihid: the i2c hid device
> - * @reportType: 0x03 for HID_FEATURE_REPORT ; 0x02 for HID_OUTPUT_REPORT
> - * @reportID: the report ID
> + * @report_type: 0x03 for HID_FEATURE_REPORT ; 0x02 for HID_OUTPUT_REPORT
> + * @report_id: the report ID
>   * @buf: the actual data to transfer, without the report ID
>   * @data_len: size of buf
> - * @use_data: true: use SET_REPORT HID command, false: send plain OUTPUT report
> + * @do_set: true: use SET_REPORT HID command, false: send plain OUTPUT report
>   */
> -static int i2c_hid_set_or_send_report(struct i2c_hid *ihid, u8 reportType,
> -               u8 reportID, unsigned char *buf, size_t data_len, bool use_data)
> +static int i2c_hid_set_or_send_report(struct i2c_hid *ihid,
> +                                     u8 report_type, u8 report_id,
> +                                     const u8 *buf, size_t data_len,
> +                                     bool do_set)
>  {
> -       u8 *args = ihid->argsbuf;
> -       const struct i2c_hid_cmd *hidcmd;
> -       int ret;
> -       u16 dataRegister = le16_to_cpu(ihid->hdesc.wDataRegister);
> -       u16 outputRegister = le16_to_cpu(ihid->hdesc.wOutputRegister);
> -       u16 maxOutputLength = le16_to_cpu(ihid->hdesc.wMaxOutputLength);
> -       u16 size;
> -       int args_len;
> -       int index = 0;
> +       size_t length = 0;
> +       int error;
>
>         i2c_hid_dbg(ihid, "%s\n", __func__);
>
>         if (data_len > ihid->bufsize)
>                 return -EINVAL;
>
> -       size =          2                       /* size */ +
> -                       (reportID ? 1 : 0)      /* reportID */ +
> -                       data_len                /* buf */;
> -       args_len =      2                       /* dataRegister */ +
> -                       size                    /* args */;
> -
> -       if (!use_data && maxOutputLength == 0)
> +       if (!do_set && le16_to_cpu(ihid->hdesc.wMaxOutputLength) == 0)
>                 return -ENOSYS;
>
> -       /*
> -        * use the data register for feature reports or if the device does not
> -        * support the output register
> -        */
> -       if (use_data) {
> -               args[index++] = dataRegister & 0xFF;
> -               args[index++] = dataRegister >> 8;
> -               hidcmd = &hid_set_report_cmd;
> +       if (do_set) {
> +               /* Command register goes first */
> +               *(__le16 *)ihid->cmdbuf = ihid->hdesc.wCommandRegister;
> +               length += sizeof(__le16);
> +               /* Next is SET_REPORT command */
> +               length += i2c_hid_encode_command(ihid->cmdbuf + length,
> +                                                I2C_HID_OPCODE_SET_REPORT,
> +                                                report_type, report_id);
> +               /*
> +                * Report data will go into the data register. Because
> +                * command can be either 2 or 3 bytes destination for
> +                * the data register may be not aligned.
> +               */
> +               put_unaligned_le16(le16_to_cpu(ihid->hdesc.wDataRegister),
> +                                  ihid->cmdbuf + length);
> +               length += sizeof(__le16);
>         } else {
> -               args[index++] = outputRegister & 0xFF;
> -               args[index++] = outputRegister >> 8;
> -               hidcmd = &hid_no_cmd;
> +               /*
> +                * With simple "send report" all data goes into the output
> +                * register.
> +                */
> +               *(__le16 *)ihid->cmdbuf = ihid->hdesc.wCommandRegister;

I believe you meant ' *(__le16 *)ihid->cmdbuf = ihid->hdesc.wOutputRegister;' :)

> +               length += sizeof(__le16);
>         }
>
> -       args[index++] = size & 0xFF;
> -       args[index++] = size >> 8;
> -
> -       if (reportID)
> -               args[index++] = reportID;
> +       length += i2c_hid_format_report(ihid->cmdbuf + length,
> +                                       report_id, buf, data_len);
>
> -       memcpy(&args[index], buf, data_len);
> -
> -       ret = __i2c_hid_command(ihid, hidcmd, reportID,
> -               reportType, args, args_len, NULL, 0);
> -       if (ret) {
> +       error = i2c_hid_xfer(ihid, ihid->cmdbuf, length, NULL, 0);
> +       if (error) {
>                 dev_err(&ihid->client->dev,
> -                       "failed to set a report to device.\n");
> -               return ret;
> +                       "failed to set a report to device: %d\n", error);
> +               return error;
>         }
>
>         return data_len;
> @@ -575,31 +606,33 @@ static void i2c_hid_free_buffers(struct i2c_hid *ihid)
>  {
>         kfree(ihid->inbuf);
>         kfree(ihid->rawbuf);
> -       kfree(ihid->argsbuf);
>         kfree(ihid->cmdbuf);
>         ihid->inbuf = NULL;
>         ihid->rawbuf = 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) + /* ReportID */
> -                      sizeof(__u8) + /* optional ReportID byte */
> -                      sizeof(__u16) + /* data register */
> -                      sizeof(__u16) + /* size of the report */
> -                      report_size; /* report */
> +       /*
> +        * The worst case is computed from the set_report command with a
> +        * reportID > 15 and the maximum report length.
> +        */
> +       int cmd_len = sizeof(__le16) +  /* command register */
> +                     sizeof(u8) +      /* encoded report type/ID */
> +                     sizeof(u8) +      /* opcode */
> +                     sizeof(u8) +      /* optional 3rd byte report ID */
> +                     sizeof(__le16) +  /* data register */
> +                     sizeof(__le16) +  /* report data size */
> +                     sizeof(u8) +      /* report ID if numbered report */
> +                     report_size;
>
>         ihid->inbuf = kzalloc(report_size, GFP_KERNEL);
>         ihid->rawbuf = kzalloc(report_size, GFP_KERNEL);
> -       ihid->argsbuf = kzalloc(args_len, GFP_KERNEL);
> -       ihid->cmdbuf = kzalloc(sizeof(union command) + args_len, GFP_KERNEL);
> +       ihid->cmdbuf = kzalloc(cmd_len, GFP_KERNEL);
>
> -       if (!ihid->inbuf || !ihid->rawbuf || !ihid->argsbuf || !ihid->cmdbuf) {
> +       if (!ihid->inbuf || !ihid->rawbuf || !ihid->cmdbuf) {
>                 i2c_hid_free_buffers(ihid);
>                 return -ENOMEM;
>         }
> @@ -659,8 +692,9 @@ static int i2c_hid_get_raw_report(struct hid_device *hid,
>         return count;
>  }
>
> -static int i2c_hid_output_raw_report(struct hid_device *hid, __u8 *buf,
> -               size_t count, unsigned char report_type, bool use_data)
> +static int i2c_hid_output_raw_report(struct hid_device *hid,
> +                                    const u8 *buf, size_t count,
> +                                    u8 report_type, bool do_set)
>  {
>         struct i2c_client *client = hid->driver_data;
>         struct i2c_hid *ihid = i2c_get_clientdata(client);
> @@ -681,7 +715,7 @@ static int i2c_hid_output_raw_report(struct hid_device *hid, __u8 *buf,
>          */
>         ret = i2c_hid_set_or_send_report(ihid,
>                                 report_type == HID_FEATURE_REPORT ? 0x03 : 0x02,
> -                               report_id, buf + 1, count - 1, use_data);
> +                               report_id, buf + 1, count - 1, do_set);
>
>         if (ret >= 0)
>                 ret++; /* add report_id to the number of transferred bytes */
> @@ -691,11 +725,10 @@ static int i2c_hid_output_raw_report(struct hid_device *hid, __u8 *buf,
>         return ret;
>  }
>
> -static int i2c_hid_output_report(struct hid_device *hid, __u8 *buf,
> -               size_t count)
> +static int i2c_hid_output_report(struct hid_device *hid, u8 *buf, size_t count)
>  {
>         return i2c_hid_output_raw_report(hid, buf, count, HID_OUTPUT_REPORT,
> -                       false);
> +                                        false);
>  }
>
>  static int i2c_hid_raw_request(struct hid_device *hid, unsigned char reportnum,
> --
> 2.34.1.703.g22d0c6ccf7-goog
>

Re: [PATCH 05/12] HID: i2c-hid: explicitly code setting and sending reports

[Dmitry Torokhov]

Hi Angela,

On Wed, Jan 19, 2022 at 04:31:03PM +0100, Angela Czubak wrote:
> Hi Dmitry,
> 
> On Tue, Jan 18, 2022 at 8:26 AM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > Instead of relying on __i2c_hid_command() that tries to handle all
> > commands and because of that is very complicated, let's define a
> > new dumb helper i2c_hid_xfer() that actually transfers (write and
> > read) data, and use it when sending and setting reports. By doing
> > that we can save on number of copy operations we have to execute,
> > and make logic of sending reports much clearer.
> >
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> >  drivers/hid/i2c-hid/i2c-hid-core.c | 269 ++++++++++++++++-------------
> >  1 file changed, 151 insertions(+), 118 deletions(-)
> >
> > diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> > index 6c1741d9211d..c48b75bd81e0 100644
> > --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> > +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> > @@ -35,6 +35,7 @@
> >  #include <linux/kernel.h>
> >  #include <linux/hid.h>
> >  #include <linux/mutex.h>
> > +#include <asm/unaligned.h>
> >
> >  #include "../hid-ids.h"
> >  #include "i2c-hid.h"
> > @@ -47,6 +48,15 @@
> >  #define I2C_HID_QUIRK_BAD_INPUT_SIZE           BIT(6)
> >  #define I2C_HID_QUIRK_NO_WAKEUP_AFTER_RESET    BIT(7)
> >
> > +/* Command opcodes */
> > +#define I2C_HID_OPCODE_RESET                   0x01
> > +#define I2C_HID_OPCODE_GET_REPORT              0x02
> > +#define I2C_HID_OPCODE_SET_REPORT              0x03
> > +#define I2C_HID_OPCODE_GET_IDLE                        0x04
> > +#define I2C_HID_OPCODE_SET_IDLE                        0x05
> > +#define I2C_HID_OPCODE_GET_PROTOCOL            0x06
> > +#define I2C_HID_OPCODE_SET_PROTOCOL            0x07
> > +#define I2C_HID_OPCODE_SET_POWER               0x08
> >
> >  /* flags */
> >  #define I2C_HID_STARTED                0
> > @@ -90,16 +100,6 @@ struct i2c_hid_cmd {
> >         unsigned int length;
> >  };
> >
> > -union command {
> > -       u8 data[0];
> > -       struct cmd {
> > -               __le16 reg;
> > -               __u8 reportTypeID;
> > -               __u8 opcode;
> > -               __u8 reportID;
> > -       } __packed c;
> > -};
> > -
> >  #define I2C_HID_CMD(opcode_) \
> >         .opcode = opcode_, .length = 4, \
> >         .registerIndex = offsetof(struct i2c_hid_desc, wCommandRegister)
> > @@ -115,9 +115,7 @@ static const struct i2c_hid_cmd hid_report_descr_cmd = {
> >  /* commands */
> >  static const struct i2c_hid_cmd hid_reset_cmd =                { I2C_HID_CMD(0x01) };
> >  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_set_power_cmd =    { I2C_HID_CMD(0x08) };
> > -static const struct i2c_hid_cmd hid_no_cmd =           { .length = 0 };
> >
> >  /*
> >   * These definitions are not used here, but are defined by the spec.
> > @@ -144,7 +142,6 @@ struct i2c_hid {
> >         u8                      *inbuf;         /* Input buffer */
> >         u8                      *rawbuf;        /* Raw Input buffer */
> >         u8                      *cmdbuf;        /* Command buffer */
> > -       u8                      *argsbuf;       /* Command arguments buffer */
> >
> >         unsigned long           flags;          /* device flags */
> >         unsigned long           quirks;         /* Various quirks */
> > @@ -206,67 +203,90 @@ static u32 i2c_hid_lookup_quirk(const u16 idVendor, const u16 idProduct)
> >         return quirks;
> >  }
> >
> > +static int i2c_hid_xfer(struct i2c_hid *ihid,
> > +                       u8 *send_buf, int send_len, u8 *recv_buf, int recv_len)
> > +{
> > +       struct i2c_client *client = ihid->client;
> > +       struct i2c_msg msgs[2] = { 0 };
> > +       int n = 0;
> > +       int ret;
> > +
> > +       if (send_len) {
> > +               i2c_hid_dbg(ihid, "%s: cmd=%*ph\n",
> > +                           __func__, send_len, send_buf);
> > +
> > +               msgs[n].addr = client->addr;
> > +               msgs[n].flags = client->flags & I2C_M_TEN;
> > +               msgs[n].len = send_len;
> > +               msgs[n].buf = send_buf;
> > +               n++;
> > +       }
> > +
> > +       if (recv_len) {
> > +               msgs[n].addr = client->addr;
> > +               msgs[n].flags = (client->flags & I2C_M_TEN) | I2C_M_RD;
> > +               msgs[n].len = recv_len;
> > +               msgs[n].buf = recv_buf;
> > +               n++;
> > +
> > +               set_bit(I2C_HID_READ_PENDING, &ihid->flags);
> > +       }
> > +
> > +       ret = i2c_transfer(client->adapter, msgs, n);
> > +
> > +       if (recv_len)
> > +               clear_bit(I2C_HID_READ_PENDING, &ihid->flags);
> > +
> > +       if (ret != n)
> > +               return ret < 0 ? ret : -EIO;
> > +
> > +       return 0;
> > +}
> > +
> > +static size_t i2c_hid_encode_command(u8 *buf, u8 opcode,
> > +                                    int report_type, int report_id)
> 
> Can it ever be used to encode something else than the command register?
> If not, perhaps we could fill it here as well.

It indeed always called after setting command register, however I
believe the register and command are logically distinct entities and
that it why I prefer not to handle the register here.

> 
> > +{
> > +       size_t length = 0;
> > +
> > +       if (report_id < 0x0F) {
> > +               buf[length++] = report_type << 4 | report_id;
> > +               buf[length++] = opcode;
> > +       } else {
> > +               buf[length++] = report_type << 4 | 0x0F;
> > +               buf[length++] = opcode;
> > +               buf[length++] = report_id;
> > +       }
> > +
> > +       return length;
> > +}
> > +
> >  static int __i2c_hid_command(struct i2c_hid *ihid,
> >                 const struct i2c_hid_cmd *command, u8 reportID,
> >                 u8 reportType, u8 *args, int args_len,
> >                 unsigned char *buf_recv, int data_len)
> >  {
> > -       struct i2c_client *client = ihid->client;
> > -       union command *cmd = (union command *)ihid->cmdbuf;
> > -       int ret;
> > -       struct i2c_msg msg[2];
> > -       int msg_num = 1;
> > -
> >         int length = command->length;
> >         unsigned int registerIndex = command->registerIndex;
> >
> >         /* special case for hid_descr_cmd */
> >         if (command == &hid_descr_cmd) {
> > -               cmd->c.reg = ihid->wHIDDescRegister;
> > +               *(__le16 *)ihid->cmdbuf = ihid->wHIDDescRegister;
> >         } else {
> > -               cmd->data[0] = ihid->hdesc_buffer[registerIndex];
> > -               cmd->data[1] = ihid->hdesc_buffer[registerIndex + 1];
> > +               ihid->cmdbuf[0] = ihid->hdesc_buffer[registerIndex];
> > +               ihid->cmdbuf[1] = ihid->hdesc_buffer[registerIndex + 1];
> >         }
> >
> >         if (length > 2) {
> > -               cmd->c.opcode = command->opcode;
> > -               if (reportID < 0x0F) {
> > -                       cmd->c.reportTypeID = reportType << 4 | reportID;
> > -               } else {
> > -                       cmd->c.reportTypeID = reportType << 4 | 0x0F;
> > -                       cmd->c.reportID = reportID;
> > -                       length++;
> > -               }
> > +               length = sizeof(__le16) + /* register */
> > +                        i2c_hid_encode_command(ihid->cmdbuf + sizeof(__le16),
> > +                                               command->opcode,
> > +                                               reportType, reportID);
> >         }
> >
> > -       memcpy(cmd->data + length, args, args_len);
> > +       memcpy(ihid->cmdbuf + 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);
> > -       }
> > -
> > -       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;
> > -
> > -       return 0;
> > +       return i2c_hid_xfer(ihid, ihid->cmdbuf, length, buf_recv, data_len);
> >  }
> >
> >  static int i2c_hid_command(struct i2c_hid *ihid,
> > @@ -301,70 +321,81 @@ static int i2c_hid_get_report(struct i2c_hid *ihid, u8 reportType,
> >         return 0;
> >  }
> >
> > +static size_t i2c_hid_format_report(u8 *buf, int report_id,
> > +                                   const u8 *data, size_t size)
> > +{
> > +       size_t length = sizeof(__le16); /* reserve space to store size */
> > +
> > +       if (report_id)
> > +               buf[length++] = report_id;
> > +
> > +       memcpy(buf + length, data, size);
> > +       length += size;
> > +
> > +       /* Store overall size in the beginning of the buffer */
> > +       put_unaligned_le16(length, buf);
> > +
> > +       return length;
> > +}
> > +
> >  /**
> >   * i2c_hid_set_or_send_report: forward an incoming report to the device
> >   * @ihid: the i2c hid device
> > - * @reportType: 0x03 for HID_FEATURE_REPORT ; 0x02 for HID_OUTPUT_REPORT
> > - * @reportID: the report ID
> > + * @report_type: 0x03 for HID_FEATURE_REPORT ; 0x02 for HID_OUTPUT_REPORT
> > + * @report_id: the report ID
> >   * @buf: the actual data to transfer, without the report ID
> >   * @data_len: size of buf
> > - * @use_data: true: use SET_REPORT HID command, false: send plain OUTPUT report
> > + * @do_set: true: use SET_REPORT HID command, false: send plain OUTPUT report
> >   */
> > -static int i2c_hid_set_or_send_report(struct i2c_hid *ihid, u8 reportType,
> > -               u8 reportID, unsigned char *buf, size_t data_len, bool use_data)
> > +static int i2c_hid_set_or_send_report(struct i2c_hid *ihid,
> > +                                     u8 report_type, u8 report_id,
> > +                                     const u8 *buf, size_t data_len,
> > +                                     bool do_set)
> >  {
> > -       u8 *args = ihid->argsbuf;
> > -       const struct i2c_hid_cmd *hidcmd;
> > -       int ret;
> > -       u16 dataRegister = le16_to_cpu(ihid->hdesc.wDataRegister);
> > -       u16 outputRegister = le16_to_cpu(ihid->hdesc.wOutputRegister);
> > -       u16 maxOutputLength = le16_to_cpu(ihid->hdesc.wMaxOutputLength);
> > -       u16 size;
> > -       int args_len;
> > -       int index = 0;
> > +       size_t length = 0;
> > +       int error;
> >
> >         i2c_hid_dbg(ihid, "%s\n", __func__);
> >
> >         if (data_len > ihid->bufsize)
> >                 return -EINVAL;
> >
> > -       size =          2                       /* size */ +
> > -                       (reportID ? 1 : 0)      /* reportID */ +
> > -                       data_len                /* buf */;
> > -       args_len =      2                       /* dataRegister */ +
> > -                       size                    /* args */;
> > -
> > -       if (!use_data && maxOutputLength == 0)
> > +       if (!do_set && le16_to_cpu(ihid->hdesc.wMaxOutputLength) == 0)
> >                 return -ENOSYS;
> >
> > -       /*
> > -        * use the data register for feature reports or if the device does not
> > -        * support the output register
> > -        */
> > -       if (use_data) {
> > -               args[index++] = dataRegister & 0xFF;
> > -               args[index++] = dataRegister >> 8;
> > -               hidcmd = &hid_set_report_cmd;
> > +       if (do_set) {
> > +               /* Command register goes first */
> > +               *(__le16 *)ihid->cmdbuf = ihid->hdesc.wCommandRegister;
> > +               length += sizeof(__le16);
> > +               /* Next is SET_REPORT command */
> > +               length += i2c_hid_encode_command(ihid->cmdbuf + length,
> > +                                                I2C_HID_OPCODE_SET_REPORT,
> > +                                                report_type, report_id);
> > +               /*
> > +                * Report data will go into the data register. Because
> > +                * command can be either 2 or 3 bytes destination for
> > +                * the data register may be not aligned.
> > +               */
> > +               put_unaligned_le16(le16_to_cpu(ihid->hdesc.wDataRegister),
> > +                                  ihid->cmdbuf + length);
> > +               length += sizeof(__le16);
> >         } else {
> > -               args[index++] = outputRegister & 0xFF;
> > -               args[index++] = outputRegister >> 8;
> > -               hidcmd = &hid_no_cmd;
> > +               /*
> > +                * With simple "send report" all data goes into the output
> > +                * register.
> > +                */
> > +               *(__le16 *)ihid->cmdbuf = ihid->hdesc.wCommandRegister;
> 
> I believe you meant ' *(__le16 *)ihid->cmdbuf = ihid->hdesc.wOutputRegister;' :)

Yes, indeed. Thank you for spotting this!

-- 
Dmitry

Re: [PATCH 00/12] i2c-hid: fixes for unnumbered reports and other improvements

[Jiri Kosina]

On Mon, 17 Jan 2022, Dmitry Torokhov wrote:

> Hi,
> 
> This series came about after I reviewed Angela's patch that fixed issues
> with incorrect handling of high-numbered reports (15 and above) in
> i2c-hid driver:
> 
> - it appears to me that the driver did not handle unnumbered reports
>   correctly as the kernel internally expects unnumbered reports to be
>   still prepended with report number 0, but i2c_hid_get_raw_report() and
>   i2c_hid_output_raw_report() only expected report ID to be present for
>   numbered reports.
> 
> - the driver tried to consolidate both command handling and sending
>   output reports in __i2c_hid_command() but the rules for different
>   commands vary significantly and because of that the logic was hard to
>   follow and it bled out from __i2c_hid_command() to callers. I decided
>   to introduce a few simple helpers and have the data encoding for
>   individual commands done at the call site. I believe this made it
>   easier to validate the rules and the logic and allowed to remove
>   special handling for the HID descriptor retrieval, among other things.
> 
> - the driver does too many copying of data; encoding the data for
>   commands at the call site allowed to only copy data once into the
>   transfer buffers.
> 
> I tested this on a couple of Chromebooks with i2c-hid touchscreens, but
> it would be great if other folks tried it out as well.

Benjamin,

is this something you could feed into your testing machinery as well? (not 
sure whether it's handling i2c-hid specifics).

Thanks!

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 00/12] i2c-hid: fixes for unnumbered reports and other improvements

[Benjamin Tissoires]

On Wed, Feb 2, 2022 at 2:56 PM Jiri Kosina <jikos@kernel.org> wrote:
>
> On Mon, 17 Jan 2022, Dmitry Torokhov wrote:
>
> > Hi,
> >
> > This series came about after I reviewed Angela's patch that fixed issues
> > with incorrect handling of high-numbered reports (15 and above) in
> > i2c-hid driver:
> >
> > - it appears to me that the driver did not handle unnumbered reports
> >   correctly as the kernel internally expects unnumbered reports to be
> >   still prepended with report number 0, but i2c_hid_get_raw_report() and
> >   i2c_hid_output_raw_report() only expected report ID to be present for
> >   numbered reports.
> >
> > - the driver tried to consolidate both command handling and sending
> >   output reports in __i2c_hid_command() but the rules for different
> >   commands vary significantly and because of that the logic was hard to
> >   follow and it bled out from __i2c_hid_command() to callers. I decided
> >   to introduce a few simple helpers and have the data encoding for
> >   individual commands done at the call site. I believe this made it
> >   easier to validate the rules and the logic and allowed to remove
> >   special handling for the HID descriptor retrieval, among other things.
> >
> > - the driver does too many copying of data; encoding the data for
> >   commands at the call site allowed to only copy data once into the
> >   transfer buffers.
> >
> > I tested this on a couple of Chromebooks with i2c-hid touchscreens, but
> > it would be great if other folks tried it out as well.
>
> Benjamin,
>
> is this something you could feed into your testing machinery as well? (not
> sure whether it's handling i2c-hid specifics).

Not really. I mean I have a sample touchpad connected on an USB to I2C
bridge that I included in the automated tests (though it may be
failing in the latest release because I need to update the patch that
enables it), but it would probably not catch all the corner cases.

I can add this series to my local tree and test on my XPS with both
and Elan touchpad and a Wacom touchscreen/panel. It will still add a
few more tests :)

I'll try to report that tomorrow now that I think I fixed my tablet series.

Cheers,
Benjamin

>
> Thanks!
>
> --
> Jiri Kosina
> SUSE Labs
>

Re: [PATCH 02/12] HID: i2c-hid: fix GET/SET_REPORT for unnumbered reports

[Benjamin Tissoires]

On Tue, Jan 18, 2022 at 8:26 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Internally kernel prepends all report buffers, for both numbered and
> unnumbered reports, with report ID, therefore to properly handle unnumbered
> reports we should

Nitpick but it seems that this sentence is not

:)

Cheers,
Benjamin

>
> For the same reason we should skip the first byte of the buffer when
> calling i2c_hid_set_or_send_report() which then will take care of properly
> formatting the transfer buffer based on its separate report ID argument
> along with report payload.
>
> Fixes: 9b5a9ae88573 ("HID: i2c-hid: implement ll_driver transport-layer callbacks")
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/hid/i2c-hid/i2c-hid-core.c | 32 ++++++++++++++++++++++--------
>  1 file changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> index bd7b0eeca3ea..b383003ff676 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -611,6 +611,17 @@ static int i2c_hid_get_raw_report(struct hid_device *hid,
>         if (report_type == HID_OUTPUT_REPORT)
>                 return -EINVAL;
>
> +       /*
> +        * In case of unnumbered reports the response from the device will
> +        * not have the report ID that the upper layers expect, so we need
> +        * to stash it the buffer ourselves and adjust the data size.
> +        */
> +       if (!report_number) {
> +               buf[0] = 0;
> +               buf++;
> +               count--;
> +       }
> +
>         /* +2 bytes to include the size of the reply in the query buffer */
>         ask_count = min(count + 2, (size_t)ihid->bufsize);
>
> @@ -632,6 +643,9 @@ static int i2c_hid_get_raw_report(struct hid_device *hid,
>         count = min(count, ret_count - 2);
>         memcpy(buf, ihid->rawbuf + 2, count);
>
> +       if (!report_number)
> +               count++;
> +
>         return count;
>  }
>
> @@ -648,17 +662,19 @@ static int i2c_hid_output_raw_report(struct hid_device *hid, __u8 *buf,
>
>         mutex_lock(&ihid->reset_lock);
>
> -       if (report_id) {
> -               buf++;
> -               count--;
> -       }
> -
> +       /*
> +        * Note that both numbered and unnumbered reports passed here
> +        * are supposed to have report ID stored in the 1st byte of the
> +        * buffer, so we strip it off unconditionally before passing payload
> +        * to i2c_hid_set_or_send_report which takes care of encoding
> +        * everything properly.
> +        */
>         ret = i2c_hid_set_or_send_report(client,
>                                 report_type == HID_FEATURE_REPORT ? 0x03 : 0x02,
> -                               report_id, buf, count, use_data);
> +                               report_id, buf + 1, count - 1, use_data);
>
> -       if (report_id && ret >= 0)
> -               ret++; /* add report_id to the number of transfered bytes */
> +       if (ret >= 0)
> +               ret++; /* add report_id to the number of transferred bytes */
>
>         mutex_unlock(&ihid->reset_lock);
>
> --
> 2.34.1.703.g22d0c6ccf7-goog
>

Re: [PATCH 00/12] i2c-hid: fixes for unnumbered reports and other improvements

[Benjamin Tissoires]

On Wed, Feb 2, 2022 at 6:59 PM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Wed, Feb 2, 2022 at 2:56 PM Jiri Kosina <jikos@kernel.org> wrote:
> >
> > On Mon, 17 Jan 2022, Dmitry Torokhov wrote:
> >
> > > Hi,
> > >
> > > This series came about after I reviewed Angela's patch that fixed issues
> > > with incorrect handling of high-numbered reports (15 and above) in
> > > i2c-hid driver:
> > >
> > > - it appears to me that the driver did not handle unnumbered reports
> > >   correctly as the kernel internally expects unnumbered reports to be
> > >   still prepended with report number 0, but i2c_hid_get_raw_report() and
> > >   i2c_hid_output_raw_report() only expected report ID to be present for
> > >   numbered reports.
> > >
> > > - the driver tried to consolidate both command handling and sending
> > >   output reports in __i2c_hid_command() but the rules for different
> > >   commands vary significantly and because of that the logic was hard to
> > >   follow and it bled out from __i2c_hid_command() to callers. I decided
> > >   to introduce a few simple helpers and have the data encoding for
> > >   individual commands done at the call site. I believe this made it
> > >   easier to validate the rules and the logic and allowed to remove
> > >   special handling for the HID descriptor retrieval, among other things.
> > >
> > > - the driver does too many copying of data; encoding the data for
> > >   commands at the call site allowed to only copy data once into the
> > >   transfer buffers.
> > >
> > > I tested this on a couple of Chromebooks with i2c-hid touchscreens, but
> > > it would be great if other folks tried it out as well.
> >
> > Benjamin,
> >
> > is this something you could feed into your testing machinery as well? (not
> > sure whether it's handling i2c-hid specifics).
>
> Not really. I mean I have a sample touchpad connected on an USB to I2C
> bridge that I included in the automated tests (though it may be
> failing in the latest release because I need to update the patch that
> enables it), but it would probably not catch all the corner cases.
>
> I can add this series to my local tree and test on my XPS with both
> and Elan touchpad and a Wacom touchscreen/panel. It will still add a
> few more tests :)

OK, So I applied the series on my development laptop.
I had to apply it on top of v5.16 and then rebase on top of
hid.git/for-next because there is a minor conflict.

I changed the register as mentioned in 5/12, and gave it a try.
Both the Elan touchpad and the Wacom panel on my XPS-13 are behaving
properly, suspend/resume works also as expected.

Tested-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

I gave a quick look at the individual patches, they all seem sane to
me, but haven't dug into enough detail to be able to formally give my
reviewed-by.
Note that I have a small comment on patch 2, but if you want to apply
it nevertheless Jiri (with the change in 5/12) it should be fine.

Cheers,
Benjamin

> I'll try to report that tomorrow now that I think I fixed my tablet series.
>
> Cheers,
> Benjamin
>
> >
> > Thanks!
> >
> > --
> > Jiri Kosina
> > SUSE Labs
> >

Re: [PATCH 00/12] i2c-hid: fixes for unnumbered reports and other improvements

[Jiri Kosina]

On Thu, 3 Feb 2022, Benjamin Tissoires wrote:

> OK, So I applied the series on my development laptop.
> I had to apply it on top of v5.16 and then rebase on top of
> hid.git/for-next because there is a minor conflict.
> 
> I changed the register as mentioned in 5/12, and gave it a try.
> Both the Elan touchpad and the Wacom panel on my XPS-13 are behaving
> properly, suspend/resume works also as expected.
> 
> Tested-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> 
> I gave a quick look at the individual patches, they all seem sane to
> me, but haven't dug into enough detail to be able to formally give my
> reviewed-by.
> Note that I have a small comment on patch 2, but if you want to apply
> it nevertheless Jiri (with the change in 5/12) it should be fine.

Thanks a lot for testing. I've tested in on my I2C hardware, and haven't 
spotted any issues either.

I plan to finish going through the whole set today and tomorrow, and apply 
it right afterwards with the 5/12 change.

Thanks,

-- 
Jiri Kosina
SUSE Labs