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@xxxxxxxxx> 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@xxxxxxxxx> > > --- > > 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