From cf5b2fb012c037fe20f617a11eb048ef70ce4b9e Mon Sep 17 00:00:00 2001 From: Angela Czubak Date: Mon, 17 Jan 2022 23:26:17 -0800 Subject: [PATCH 01/13] HID: i2c-hid: fix handling numbered reports with IDs of 15 and above 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 Signed-off-by: Dmitry Torokhov Tested-by: Benjamin Tissoires Signed-off-by: Jiri Kosina --- 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 6726567d72976..899441b9c05b2 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 From a5e5e03e94764148a01757b2fa4737d3445c13a6 Mon Sep 17 00:00:00 2001 From: Dmitry Torokhov Date: Mon, 17 Jan 2022 23:26:18 -0800 Subject: [PATCH 02/13] HID: i2c-hid: fix GET/SET_REPORT for unnumbered reports Internally kernel prepends all report buffers, for both numbered and unnumbered reports, with report ID, therefore to properly handle unnumbered reports we should prepend it ourselves. 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. [jkosina@suse.cz: finalize trimmed sentence in changelog as spotted by Benjamin] Fixes: 9b5a9ae88573 ("HID: i2c-hid: implement ll_driver transport-layer callbacks") Signed-off-by: Dmitry Torokhov Tested-by: Benjamin Tissoires Signed-off-by: Jiri Kosina --- 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 899441b9c05b2..748619e36f95a 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -614,6 +614,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); @@ -635,6 +646,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; } @@ -651,17 +665,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); From d34c6105499bd0c3861c4a514bb7d953b851bc91 Mon Sep 17 00:00:00 2001 From: Dmitry Torokhov Date: Mon, 17 Jan 2022 23:26:19 -0800 Subject: [PATCH 03/13] HID: i2c-hid: use "struct i2c_hid" as argument in most calls 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 Tested-by: Benjamin Tissoires Signed-off-by: Jiri Kosina --- 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 748619e36f95a..4f5f7574c823a 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); @@ -628,7 +625,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); @@ -672,7 +669,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); @@ -727,7 +724,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); @@ -751,7 +748,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"); @@ -871,11 +868,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; } } @@ -885,7 +882,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; @@ -894,8 +891,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); @@ -1064,7 +1061,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); @@ -1085,7 +1082,7 @@ static int i2c_hid_core_suspend(struct device *dev) return ret; /* 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); @@ -1133,9 +1130,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; From b26fc3161b7874ae6300151e652d2d6a15f199f9 Mon Sep 17 00:00:00 2001 From: Dmitry Torokhov Date: Mon, 17 Jan 2022 23:26:20 -0800 Subject: [PATCH 04/13] HID: i2c-hid: refactor reset command "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 Tested-by: Benjamin Tissoires Signed-off-by: Jiri Kosina --- 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 4f5f7574c823a..57f109bc8ec96 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; } From dbe0dd5fd2e0c687ad50fc590d5a90400f26adcc Mon Sep 17 00:00:00 2001 From: Dmitry Torokhov Date: Mon, 17 Jan 2022 23:26:21 -0800 Subject: [PATCH 05/13] HID: i2c-hid: explicitly code setting and sending reports 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 Tested-by: Benjamin Tissoires Signed-off-by: Jiri Kosina --- 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 57f109bc8ec96..856778d8592af 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -35,6 +35,7 @@ #include #include #include +#include #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.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; @@ -578,31 +609,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; } @@ -662,8 +695,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); @@ -684,7 +718,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 */ @@ -694,11 +728,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, From 8399bd01026eab803d6ebb5372cf28b5e1f335f8 Mon Sep 17 00:00:00 2001 From: Dmitry Torokhov Date: Mon, 17 Jan 2022 23:26:22 -0800 Subject: [PATCH 06/13] HID: i2c-hid: define i2c_hid_read_register() and use it 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 Tested-by: Benjamin Tissoires Signed-off-by: Jiri Kosina --- 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 856778d8592af..32d275d2d5dc9 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 */ @@ -794,8 +789,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); @@ -905,7 +901,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)) { @@ -914,11 +910,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; } } From acb8dd95974dda4b6072410bfe09bc83d2c22d23 Mon Sep 17 00:00:00 2001 From: Dmitry Torokhov Date: Mon, 17 Jan 2022 23:26:23 -0800 Subject: [PATCH 07/13] HID: i2c-hid: create a helper for SET_POWER command 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 Tested-by: Benjamin Tissoires Signed-off-by: Jiri Kosina --- 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 32d275d2d5dc9..7b7127534e24e 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"); From 50c5249fcafc9f9c64e4a367e336c2e17f1c978f Mon Sep 17 00:00:00 2001 From: Dmitry Torokhov Date: Mon, 17 Jan 2022 23:26:24 -0800 Subject: [PATCH 08/13] HID: i2c-hid: convert i2c_hid_execute_reset() to use i2c_hid_xfer() This will allow us to drop i2c_hid_command() wrapper and get close to removing __i2c_hid_command(). Signed-off-by: Dmitry Torokhov Tested-by: Benjamin Tissoires Signed-off-by: Jiri Kosina --- 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 7b7127534e24e..30b545c67eb81 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; From 85df713377ddc0482071c3e6b64c37bd1e48f1f1 Mon Sep 17 00:00:00 2001 From: Dmitry Torokhov Date: Mon, 17 Jan 2022 23:26:25 -0800 Subject: [PATCH 09/13] HID: i2c-hid: rework i2c_hid_get_report() to use i2c_hid_xfer() 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 Tested-by: Benjamin Tissoires Signed-off-by: Jiri Kosina --- 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 30b545c67eb81..67c80bffc0db5 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, @@ -654,13 +641,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; @@ -670,42 +656,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); @@ -738,7 +706,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); } @@ -748,11 +716,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; } From 86fc3fd28157b6d4d044bdc00b3ee72899e554fc Mon Sep 17 00:00:00 2001 From: Dmitry Torokhov Date: Mon, 17 Jan 2022 23:26:26 -0800 Subject: [PATCH 10/13] HID: i2c-hid: use helpers to do endian conversion in i2c_hid_get_input() 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 Tested-by: Benjamin Tissoires Signed-off-by: Jiri Kosina --- drivers/hid/i2c-hid/i2c-hid-core.c | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index 67c80bffc0db5..a32502db15e4f 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; } @@ -557,8 +558,9 @@ static void i2c_hid_get_input(struct i2c_hid *ihid) if (test_bit(I2C_HID_STARTED, &ihid->flags)) { pm_wakeup_event(&ihid->client->dev, 0); - hid_input_report(ihid->hid, HID_INPUT_REPORT, ihid->inbuf + 2, - ret_size - 2, 1); + hid_input_report(ihid->hid, HID_INPUT_REPORT, + ihid->inbuf + sizeof(__le16), + ret_size - sizeof(__le16), 1); } return; From 551117c52237e7d92bc050df961b6a63e86c5bcd Mon Sep 17 00:00:00 2001 From: Dmitry Torokhov Date: Mon, 17 Jan 2022 23:26:27 -0800 Subject: [PATCH 11/13] HID: i2c-hid: no longer need raw access to HID descriptor structure 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 Tested-by: Benjamin Tissoires Signed-off-by: Jiri Kosina --- 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 a32502db15e4f..f312d2afd3196 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. */ @@ -923,7 +920,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; } From 1c4d6cd4cb48ae5d097c610a5ca4bd15e9f41bfb Mon Sep 17 00:00:00 2001 From: Dmitry Torokhov Date: Mon, 17 Jan 2022 23:26:28 -0800 Subject: [PATCH 12/13] HID: i2c-hid: note that I2C xfer buffers are DMA-safe All I2C communications in the driver use driver-private buffers that are DMA-safe, so mark them as such. Signed-off-by: Dmitry Torokhov Tested-by: Benjamin Tissoires Signed-off-by: Jiri Kosina --- 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 f312d2afd3196..91b2fa0d33c7c 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++; From 269ecc0c894c6db7ea13ae076c01bd24e87b15e6 Mon Sep 17 00:00:00 2001 From: Yang Li Date: Wed, 16 Feb 2022 09:50:42 +0800 Subject: [PATCH 13/13] HID: i2c-hid: remove unneeded semicolon Eliminate the following coccicheck warning: ./drivers/hid/i2c-hid/i2c-hid-core.c:357:56-57: Unneeded semicolon Reported-by: Abaci Robot Signed-off-by: Yang Li Signed-off-by: Jiri Kosina --- drivers/hid/i2c-hid/i2c-hid-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index 91b2fa0d33c7c..c078f09a2318a 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -354,7 +354,7 @@ static int i2c_hid_set_or_send_report(struct i2c_hid *ihid, * With simple "send report" all data goes into the output * register. */ - *(__le16 *)ihid->cmdbuf = ihid->hdesc.wOutputRegister;; + *(__le16 *)ihid->cmdbuf = ihid->hdesc.wOutputRegister; length += sizeof(__le16); }