| Message ID | 20211221191743.1893185-19-acz@semihalf.com |
|---|---|
| State | New |
| Headers | show |
| Series | *** Implement simple haptic HID support *** | expand |
Hi Angela, On Tue, Dec 21, 2021 at 07:17:43PM +0000, Angela Czubak wrote: > If command & data registers are to be used and report ID >= 0xF > use the sentinel value for report ID in the command. > Do not alter the report ID itself as it needs to be inserted into the args > buffer. If output register is to be used there is no need to insert > report IDs >= 0xF. OK, I see what you mean, but I believe that i2c_hid_set_or_send_report() is the wrong place to implement this handling to begin with. How about a modified version of your patch that I am pasting below (not tested)? Thanks, Dmitry -- >8 -- >8 -- HID: i2c-hid: fix handling numbered reports with IDs of 15 and above 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 | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index 517141138b00..9381a70b2948 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); @@ -300,11 +307,6 @@ static int i2c_hid_get_report(struct i2c_client *client, u8 reportType, 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
Hi Dmitry, I tested the patch and it works fine. What is more, I like your design better. Thanks! Regards, Angela
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index 517141138b00..8cb925c86bbf 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -341,6 +341,7 @@ static int i2c_hid_set_or_send_report(struct i2c_client *client, u8 reportType, u16 size; int args_len; int index = 0; + u8 cmdReportID = reportID; i2c_hid_dbg(ihid, "%s\n", __func__); @@ -357,16 +358,15 @@ static int i2c_hid_set_or_send_report(struct i2c_client *client, u8 reportType, 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 */ if (use_data) { + if (reportID >= 0x0F) { + args[index++] = reportID; + cmdReportID = 0x0F; + } args[index++] = dataRegister & 0xFF; args[index++] = dataRegister >> 8; hidcmd = &hid_set_report_cmd; @@ -384,7 +384,7 @@ 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(client, hidcmd, cmdReportID, reportType, args, args_len, NULL, 0); if (ret) { dev_err(&client->dev, "failed to set a report to device.\n");
If command & data registers are to be used and report ID >= 0xF use the sentinel value for report ID in the command. Do not alter the report ID itself as it needs to be inserted into the args buffer. If output register is to be used there is no need to insert report IDs >= 0xF. Signed-off-by: Angela Czubak <acz@semihalf.com> --- drivers/hid/i2c-hid/i2c-hid-core.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)