Hi Dmitry, thanks for the review. On Wed, Oct 3, 2012 at 6:43 PM, Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> wrote: > Hi Benjamin, > > A few random comments... > > On Fri, Sep 14, 2012 at 03:41:43PM +0200, benjamin.tissoires wrote: >> From: Benjamin Tissoires <benjamin.tissoires@xxxxxxx> >> >> Microsoft published the protocol specification of HID over i2c: >> http://msdn.microsoft.com/en-us/library/windows/hardware/hh852380.aspx >> >> This patch introduces an implementation of this protocol. >> >> This implementation does not includes the ACPI part of the specification. >> This will come when ACPI 5.0 devices will be available. >> >> Once the ACPI part will be done, OEM will not have to declare HID over I2C >> devices in their platform specific driver. >> >> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@xxxxxxx> >> --- >> >> Hi, >> >> this is finally my first implementation of HID over I2C. >> >> This has been tested on an Elan Microelectronics HID over I2C device, with >> a Samsung Exynos 4412 board. >> >> Any comments are welcome. >> >> Cheers, >> Benjamin >> >> drivers/i2c/Kconfig | 8 + >> drivers/i2c/Makefile | 1 + >> drivers/i2c/i2c-hid.c | 1027 +++++++++++++++++++++++++++++++++++++++++++ >> include/linux/i2c/i2c-hid.h | 35 ++ >> 4 files changed, 1071 insertions(+) >> create mode 100644 drivers/i2c/i2c-hid.c >> create mode 100644 include/linux/i2c/i2c-hid.h >> >> diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig >> index 5a3bb3d..5adf65a 100644 >> --- a/drivers/i2c/Kconfig >> +++ b/drivers/i2c/Kconfig >> @@ -47,6 +47,14 @@ config I2C_CHARDEV >> This support is also available as a module. If so, the module >> will be called i2c-dev. >> >> +config I2C_HID >> + tristate "HID over I2C bus" >> + help >> + Say Y here to use the HID over i2c protocol implementation. >> + >> + This support is also available as a module. If so, the module >> + will be called i2c-hid. >> + >> config I2C_MUX >> tristate "I2C bus multiplexing support" >> help >> diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile >> index beee6b2..8f38116 100644 >> --- a/drivers/i2c/Makefile >> +++ b/drivers/i2c/Makefile >> @@ -6,6 +6,7 @@ obj-$(CONFIG_I2C_BOARDINFO) += i2c-boardinfo.o >> obj-$(CONFIG_I2C) += i2c-core.o >> obj-$(CONFIG_I2C_SMBUS) += i2c-smbus.o >> obj-$(CONFIG_I2C_CHARDEV) += i2c-dev.o >> +obj-$(CONFIG_I2C_HID) += i2c-hid.o >> obj-$(CONFIG_I2C_MUX) += i2c-mux.o >> obj-y += algos/ busses/ muxes/ >> >> diff --git a/drivers/i2c/i2c-hid.c b/drivers/i2c/i2c-hid.c >> new file mode 100644 >> index 0000000..eb17d8c >> --- /dev/null >> +++ b/drivers/i2c/i2c-hid.c >> @@ -0,0 +1,1027 @@ >> +/* >> + * HID over I2C protocol implementation >> + * >> + * Copyright (c) 2012 Benjamin Tissoires <benjamin.tissoires@xxxxxxxxx> >> + * Copyright (c) 2012 Ecole Nationale de l'Aviation Civile, France >> + * >> + * This code is partly based on "USB HID support for Linux": >> + * >> + * Copyright (c) 1999 Andreas Gal >> + * Copyright (c) 2000-2005 Vojtech Pavlik <vojtech@xxxxxxx> >> + * Copyright (c) 2005 Michael Haboustak <mike-@xxxxxxxxxxxx> for Concept2, Inc >> + * Copyright (c) 2007-2008 Oliver Neukum >> + * Copyright (c) 2006-2010 Jiri Kosina >> + * >> + * This file is subject to the terms and conditions of the GNU General Public >> + * License. See the file COPYING in the main directory of this archive for >> + * more details. >> + */ >> + >> +#include <linux/module.h> >> +#include <linux/i2c.h> >> +#include <linux/irq.h> >> +#include <linux/gpio.h> >> +#include <linux/interrupt.h> >> +#include <linux/input.h> >> +#include <linux/delay.h> >> +#include <linux/slab.h> >> +#include <linux/pm.h> >> + >> +#include <linux/i2c/i2c-hid.h> >> + >> +#include <linux/hid.h> >> +#include <linux/hiddev.h> >> +#include <linux/hidraw.h> >> + >> +#define DRIVER_NAME "i2chid" >> +#define DRIVER_DESC "HID over I2C core driver" >> + >> +#define I2C_HID_COMMAND_TRIES 3 >> + >> +/* flags */ >> +#define I2C_HID_STARTED (1 << 0) >> +#define I2C_HID_OUT_RUNNING (1 << 1) >> +#define I2C_HID_IN_RUNNING (1 << 2) >> +#define I2C_HID_RESET_PENDING (1 << 3) >> +#define I2C_HID_SUSPENDED (1 << 4) >> + >> +#define I2C_HID_PWR_ON 0x00 >> +#define I2C_HID_PWR_SLEEP 0x01 >> + >> +/* debug option */ >> +static bool debug = false; >> +module_param(debug, bool, 0444); >> +MODULE_PARM_DESC(debug, "print a lot of debug informations"); >> + >> +struct i2chid_desc { >> + __le16 wHIDDescLength; >> + __le16 bcdVersion; >> + __le16 wReportDescLength; >> + __le16 wReportDescRegister; >> + __le16 wInputRegister; >> + __le16 wMaxInputLength; >> + __le16 wOutputRegister; >> + __le16 wMaxOutputLength; >> + __le16 wCommandRegister; >> + __le16 wDataRegister; >> + __le16 wVendorID; >> + __le16 wProductID; >> + __le16 wVersionID; >> +} __packed; >> + >> +struct i2chid_cmd { >> + enum { >> + /* fecth HID descriptor */ >> + HID_DESCR_CMD, >> + >> + /* fetch report descriptors */ >> + HID_REPORT_DESCR_CMD, >> + >> + /* commands */ >> + HID_RESET_CMD, >> + HID_GET_REPORT_CMD, >> + HID_SET_REPORT_CMD, >> + HID_GET_IDLE_CMD, >> + HID_SET_IDLE_CMD, >> + HID_GET_PROTOCOL_CMD, >> + HID_SET_PROTOCOL_CMD, >> + HID_SET_POWER_CMD, >> + >> + /* read/write data register */ >> + HID_DATA_CMD, >> + } name; >> + unsigned int registerIndex; >> + __u8 opcode; >> + unsigned int length; >> + bool wait; >> +}; >> + >> +union command { >> + u8 data[0]; >> + struct cmd { >> + __le16 reg; >> + __u8 reportTypeID; >> + __u8 opcode; >> + } __packed c; >> +}; >> + >> +#define I2C_HID_CMD(name_, opcode_) \ >> + .name = name_, .opcode = opcode_, .length = 4, \ >> + .registerIndex = offsetof(struct i2chid_desc, wCommandRegister) >> + >> +static const struct i2chid_cmd cmds[] = { >> + { I2C_HID_CMD(HID_RESET_CMD, 0x01), .wait = true }, >> + { I2C_HID_CMD(HID_GET_REPORT_CMD, 0x02) }, >> + { I2C_HID_CMD(HID_SET_REPORT_CMD, 0x03) }, >> + { I2C_HID_CMD(HID_GET_IDLE_CMD, 0x04) }, >> + { I2C_HID_CMD(HID_SET_IDLE_CMD, 0x05) }, >> + { I2C_HID_CMD(HID_GET_PROTOCOL_CMD, 0x06) }, >> + { I2C_HID_CMD(HID_SET_PROTOCOL_CMD, 0x07) }, >> + { I2C_HID_CMD(HID_SET_POWER_CMD, 0x08) }, >> + >> + {.name = HID_DESCR_CMD, >> + .length = 2}, >> + >> + {.name = HID_REPORT_DESCR_CMD, >> + .registerIndex = offsetof(struct i2chid_desc, wReportDescRegister), >> + .opcode = 0x00, >> + .length = 2}, >> + >> + {.name = HID_DATA_CMD, >> + .registerIndex = offsetof(struct i2chid_desc, wDataRegister), >> + .opcode = 0x00, >> + .length = 2}, >> + >> + {}, /* terminating */ >> +}; >> + >> +/* The main device structure */ >> +struct i2chid { > > struct i2c_hid? And elsewhere s/i2chid/i2c_hid/ maybe? Yes, maybe. It's more easier to read. I chose to name it without the underscore to be closer to the usbhid driver, but it's harder to read. > >> + struct i2c_client *client; /* i2c client */ >> + struct hid_device *hid; /* pointer to corresponding HID dev */ >> + union { >> + __u8 hdesc_buffer[sizeof(struct i2chid_desc)]; >> + struct i2chid_desc hdesc; /* the HID Descriptor */ >> + }; >> + __le16 wHIDDescRegister; /* location of the i2c >> + * register of the HID >> + * descriptor. */ >> + unsigned int bufsize; /* i2c buffer size */ >> + char *inbuf; /* Input buffer */ >> + >> + spinlock_t flock; /* flags spinlock */ >> + unsigned long flags; /* device flags */ >> + >> + int irq; /* the interrupt line irq */ >> + >> + wait_queue_head_t wait; /* For waiting the interrupt */ >> +}; >> + >> +void i2chid_print_buffer(struct i2chid *ihid, u8 *buf, unsigned int n) >> +{ >> + int i; >> + char *string = kzalloc((n*3+1)*sizeof(char), GFP_KERNEL); >> + char tmpbuf[4] = ""; >> + >> + for (i = 0; i < n; ++i) { >> + sprintf(tmpbuf, "%02x ", buf[i] & 0xFF); >> + strcat(string, tmpbuf); >> + } >> + >> + dev_err(&ihid->client->dev, "%s\n", string); >> + kfree(string); > > Why not simply > > dev_XXX(&ihid->client->dev, "%*ph\n", n, buf); Indeed, it's far more simple.... sorry. > >> +} >> + >> +static int __i2chid_command(struct i2c_client *client, int command, u8 reportID, >> + u8 reportType, u8 *args, int args_len, >> + unsigned char *buf_recv, int data_len) >> +{ >> + struct i2chid *ihid = i2c_get_clientdata(client); >> + union command *cmd; >> + unsigned char *rec_buf = buf_recv; >> + int ret; >> + int tries = I2C_HID_COMMAND_TRIES; >> + int length = 0; >> + struct i2c_msg msg[2]; >> + int msg_num = 0; >> + int i; >> + bool wait = false; >> + >> + if (WARN_ON(!ihid)) >> + return -ENODEV; > > How can this happen? It shouldn't. An excess of checking while writing the first version, and now it's useless. > >> + >> + cmd = kmalloc(args_len + sizeof(union command), GFP_KERNEL); >> + if (!cmd) >> + return -ENOMEM; > > Can we have several commands pending at once? If not maybe use suffcient > preallocated buffer in i2c_hid structure? At first, I allocated it statically. However, due to some outputs commands, it's harder to get the exact maximum of this command. The set_report has the following command: fill the set report + fill the data register (with headers and size) with the report. By writing it, I realize that I should be able to get a maximum buffer length... > >> + >> + for (i = 0; cmds[i].length; i++) { >> + unsigned int registerIndex; >> + >> + if (cmds[i].name != command) >> + continue; >> + >> + registerIndex = cmds[i].registerIndex; >> + cmd->data[0] = ihid->hdesc_buffer[registerIndex]; >> + cmd->data[1] = ihid->hdesc_buffer[registerIndex + 1]; >> + cmd->c.opcode = cmds[i].opcode; >> + length = cmds[i].length; >> + wait = cmds[i].wait; >> + } >> + >> + /* special case for HID_DESCR_CMD */ >> + if (command == HID_DESCR_CMD) >> + cmd->c.reg = ihid->wHIDDescRegister; >> + >> + cmd->c.reportTypeID = reportID | reportType << 4; >> + >> + memcpy(cmd->data + length, args, args_len); >> + length += args_len; >> + >> + if (debug) { >> + char tmpstr[4] = ""; >> + char strbuf[50] = ""; >> + int i; >> + for (i = 0; i < length; i++) { >> + sprintf(tmpstr, "%02x ", cmd->data[i] & 0xFF); >> + strcat(strbuf, tmpstr); >> + } >> + dev_err(&client->dev, "%s, cmd=%s\n", __func__, strbuf); > > Agaion, try using '%*ph' format specifier instead. ok > >> + } >> + >> + msg[0].addr = client->addr; >> + msg[0].flags = client->flags & I2C_M_TEN; >> + msg[0].len = length; >> + msg[0].buf = (char *) cmd->data; >> + 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 = rec_buf; >> + >> + msg_num = data_len > 0 ? 2 : 1; >> + >> + if (wait) { >> + spin_lock_irq(&ihid->flock); >> + set_bit(I2C_HID_RESET_PENDING, &ihid->flags); >> + spin_unlock_irq(&ihid->flock); > > Why do you need to take this lock? Overall I am not sure why we take > spinlockn this driver when we use atomics to manipulate flags. Right. The spinlocks were there to protect against the reset command that must be synchronous. By re-reading the code, I think I can handle it without the need of spinlocks. So I'll drop them in v2. > >> + } >> + >> + do { >> + ret = i2c_transfer(client->adapter, msg, msg_num); >> + if (ret > 0) >> + break; >> + tries--; >> + dev_dbg(&client->dev, "retrying i2chid_command:%d (%d)\n", >> + command, tries); >> + } while (tries > 0); >> + >> + if (wait && ret > 0) { >> + if (debug) >> + dev_err(&client->dev, "%s: waiting....\n", __func__); >> + if (!wait_event_timeout(ihid->wait, >> + !test_bit(I2C_HID_RESET_PENDING, &ihid->flags), >> + msecs_to_jiffies(5000))) >> + ret = -ENODATA; >> + if (debug) >> + dev_err(&client->dev, "%s: finished.\n", __func__); > > Why do we have error level messages with debug? I know dev_dbg is > compiled out if !DEBUG, but there must be a better way. Maybe define > i2c_hid_dbg() via dev_printk() and also check debug condition there? > ooops.... yep, the i2c_hid_dbg is far better. Will do it. >> + } >> + >> + kfree(cmd); >> + >> + return ret > 0 ? ret != msg_num : ret; >> +} >> + >> +static int i2chid_command(struct i2c_client *client, int command, >> + unsigned char *buf_recv, int data_len) >> +{ >> + return __i2chid_command(client, command, 0, 0, NULL, 0, >> + buf_recv, data_len); >> +} >> + >> +static int i2chid_get_report(struct i2c_client *client, u8 reportType, >> + u32 reportID, unsigned char *buf_recv, int data_len) >> +{ >> + struct i2chid *ihid = i2c_get_clientdata(client); >> + u8 args[6]; >> + int ret; >> + int args_len = 0; >> + u16 readRegister = ihid->hdesc.wDataRegister; >> + >> + if (debug) >> + dev_err(&client->dev, "%s\n", __func__); >> + >> + if (reportID >= 15) { >> + args[args_len++] = (reportID >> 0) & 0xFF; >> + args[args_len++] = (reportID >> 8) & 0xFF; >> + args[args_len++] = (reportID >> 16) & 0xFF; >> + args[args_len++] = (reportID >> 24) & 0xFF; >> + reportID = 0x0F; >> + } >> + >> + args[args_len++] = readRegister & 0xff; >> + args[args_len++] = (readRegister >> 8) & 0xff; >> + >> + ret = __i2chid_command(client, HID_GET_REPORT_CMD, reportID & 0x0F, >> + reportType, args, args_len, buf_recv, data_len); >> + if (ret) { >> + dev_err(&client->dev, "HID_GET_REPORT_CMD Fail\n"); >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + >> +static int i2chid_set_report(struct i2c_client *client, u8 reportType, >> + u32 reportID, unsigned char *buf, int data_len) >> +{ >> + struct i2chid *ihid = i2c_get_clientdata(client); >> + u8 *args; >> + int ret; >> + u16 dataRegister = ihid->hdesc.wDataRegister; >> + int args_len = (reportID >= 15 ? 4 : 0) + >> + 2 /* dataRegister */ + >> + 2 /* size */ + >> + data_len; >> + int index = 0; >> + >> + if (debug) >> + dev_err(&client->dev, "%s\n", __func__); >> + >> + args = kmalloc(args_len, GFP_KERNEL); >> + >> + if (reportID >= 15) { >> + args[index++] = (reportID >> 0) & 0xFF; >> + args[index++] = (reportID >> 8) & 0xFF; >> + args[index++] = (reportID >> 16) & 0xFF; >> + args[index++] = (reportID >> 24) & 0xFF; >> + reportID = 0x0F; >> + } >> + >> + args[index++] = dataRegister & 0xff; >> + args[index++] = (dataRegister >> 8) & 0xff; >> + >> + args[index++] = data_len & 0xff; >> + args[index++] = (data_len >> 8) & 0xff; >> + >> + memcpy(&args[index], buf, data_len); >> + >> + ret = __i2chid_command(client, HID_SET_REPORT_CMD, reportID & 0x0F, >> + reportType, args, args_len, NULL, 0); >> + if (ret) { >> + dev_err(&client->dev, "HID_SET_REPORT_CMD Fail\n"); >> + return -EINVAL; >> + } >> + >> + kfree(args); >> + return data_len; >> +} >> + >> +static int i2chid_set_power(struct i2c_client *client, int power_state) >> +{ >> + int ret; >> + >> + if (debug) >> + dev_err(&client->dev, "%s\n", __func__); >> + >> + ret = __i2chid_command(client, HID_SET_POWER_CMD, power_state & 0x01, >> + 0, NULL, 0, NULL, 0); >> + if (ret) { >> + dev_err(&client->dev, "HID_SET_POWER_CMD Fail\n"); >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + >> +static int i2chid_hwreset(struct i2c_client *client) >> +{ >> + uint8_t buf_recv[79]; >> + int ret; >> + >> + if (debug) >> + dev_err(&client->dev, "%s\n", __func__); >> + >> + ret = i2chid_set_power(client, I2C_HID_PWR_ON); >> + if (ret) >> + return -EINVAL; >> + >> + if (debug) >> + dev_err(&client->dev, "resetting...\n"); >> + >> + ret = i2chid_command(client, HID_RESET_CMD, buf_recv, 0); >> + if (ret) { >> + dev_err(&client->dev, "HID_RESET_CMD Fail\n"); >> + return -EINVAL; >> + } >> + >> + ret = i2c_master_recv(client, buf_recv, 2); >> + if (ret != 2 || (buf_recv[0] != 0 && buf_recv[1] != 0)) { >> + dev_err(&client->dev, >> + "HID_RESET_CMD Failed: got %02x %02x, size=%d\n", >> + buf_recv[0], buf_recv[1], ret); >> + >> + i2chid_set_power(client, I2C_HID_PWR_SLEEP); >> + return -EINVAL; >> + } >> + return 0; >> +} >> + >> +static bool i2chid_get_input(struct i2chid *ihid) >> +{ >> + int ret; >> + int size = ihid->hdesc.wMaxInputLength; >> + >> + ret = i2c_master_recv(ihid->client, ihid->inbuf, size); >> + if (ret != size) { >> + dev_err(&ihid->client->dev, "%s: got %d data instead of %d\n", >> + __func__, ret, size); >> + return false; >> + } >> + >> + if (debug) >> + i2chid_print_buffer(ihid, ihid->inbuf, size); >> + >> + ret = hid_input_report(ihid->hid, HID_INPUT_REPORT, ihid->inbuf + 2, >> + size - 2, 1); >> + if (ret) >> + return false; >> + >> + return true; >> +} >> + >> +static irqreturn_t i2chid_irq(int irq, void *dev_id) >> +{ >> + struct i2chid *ihid = dev_id; >> + int ready; >> + >> + if (!ihid) >> + return IRQ_HANDLED; >> + >> + spin_lock_irq(&ihid->flock); >> + if (test_and_clear_bit(I2C_HID_RESET_PENDING, &ihid->flags)) { >> + spin_unlock_irq(&ihid->flock); >> + >> + wake_up(&ihid->wait); >> + return IRQ_HANDLED; >> + } >> + >> + ready = test_bit(I2C_HID_STARTED, &ihid->flags); >> + spin_unlock_irq(&ihid->flock); > > Why this lock? What does it protect? You just dropped it and make a > decision on something that you retrieved while holding the lock. But now > that you dropped it the condition could change by the time you get to > test it, so why bother? As mentioned above, the lock was more against the RESET_PENDING. I consider that once the device is started, we can handle the input reports. Anyway, as mentioned above, I'll try to remove them in v2, it will be more simple. > >> + >> + if (ready) >> + i2chid_get_input(ihid); >> + >> + return IRQ_HANDLED; >> +} >> + >> +static int i2chid_get_report_length(struct hid_report *report) >> +{ >> + return ((report->size - 1) >> 3) + 1 + >> + report->device->report_enum[report->type].numbered + 2; >> +} >> + >> +void i2chid_init_report(struct hid_report *report) >> +{ >> + struct hid_device *hid = report->device; >> + struct i2c_client *client = hid->driver_data; >> + struct i2chid *ihid = i2c_get_clientdata(client); >> + unsigned int size, ret_size; >> + >> + size = i2chid_get_report_length(report); >> + i2chid_get_report(client, >> + report->type == HID_FEATURE_REPORT ? 0x03 : 0x01, >> + report->id, ihid->inbuf, size); >> + >> + if (debug) >> + i2chid_print_buffer(ihid, ihid->inbuf, size); >> + >> + ret_size = ihid->inbuf[0] | (ihid->inbuf[1] << 8); >> + >> + if (ret_size != size) { >> + dev_err(&client->dev, "error in %s size:%d / ret_size:%d\n", >> + __func__, size, ret_size); >> + return; >> + } >> + >> + /* hid->driver_lock is held as we are in probe function, >> + * we just need to setup the input fields, so using >> + * hid_report_raw_event is safe. */ >> + hid_report_raw_event(hid, report->type, ihid->inbuf + 2, size - 2, 1); >> +} >> + >> +/* >> + * Initialize all reports >> + */ >> +void i2chid_init_reports(struct hid_device *hid) >> +{ >> + struct hid_report *report; >> + >> + list_for_each_entry(report, >> + &hid->report_enum[HID_INPUT_REPORT].report_list, list) >> + i2chid_init_report(report); >> + >> + list_for_each_entry(report, >> + &hid->report_enum[HID_FEATURE_REPORT].report_list, list) >> + i2chid_init_report(report); >> +} >> + >> +/* >> + * Traverse the supplied list of reports and find the longest >> + */ >> +static void i2chid_find_max_report(struct hid_device *hid, unsigned int type, >> + unsigned int *max) >> +{ >> + struct hid_report *report; >> + unsigned int size; >> + >> + /* We should not rely on wMaxInputLength, as some devices may set it to >> + * a wrong length. */ >> + list_for_each_entry(report, &hid->report_enum[type].report_list, list) { >> + size = i2chid_get_report_length(report); >> + if (*max < size) >> + *max = size; >> + } >> +} >> + >> +static int i2chid_alloc_buffers(struct i2chid *ihid) >> +{ >> + ihid->inbuf = kzalloc(ihid->bufsize, GFP_KERNEL); >> + >> + if (!ihid->inbuf) >> + return -1; >> + >> + return 0; >> +} >> + >> +static void i2chid_free_buffers(struct i2chid *ihid) >> +{ >> + kfree(ihid->inbuf); >> +} >> + >> +static int i2chid_get_raw_report(struct hid_device *hid, >> + unsigned char report_number, __u8 *buf, size_t count, >> + unsigned char report_type) >> +{ >> + struct i2c_client *client = hid->driver_data; >> + struct i2chid *ihid = i2c_get_clientdata(client); >> + int ret; >> + >> + if (count > ihid->bufsize) >> + count = ihid->bufsize; >> + >> + ret = i2chid_get_report(client, >> + report_type == HID_FEATURE_REPORT ? 0x03 : 0x01, >> + report_number, ihid->inbuf, count); >> + >> + if (ret < 0) >> + return ret; >> + >> + count = ihid->inbuf[0] | (ihid->inbuf[1] << 8); >> + >> + memcpy(buf, ihid->inbuf + 2, count); >> + >> + return count; >> +} >> + >> +static int i2chid_output_raw_report(struct hid_device *hid, __u8 *buf, >> + size_t count, unsigned char report_type) >> +{ >> + struct i2c_client *client = hid->driver_data; >> + int ret; >> + int report_id = buf[0]; >> + >> + ret = i2chid_set_report(client, >> + report_type == HID_FEATURE_REPORT ? 0x03 : 0x01, >> + report_id, buf, count); >> + >> + return ret; >> + >> +} >> + >> +static int i2chid_fetch_hid_descriptor(struct i2chid *ihid) >> +{ >> + struct i2c_client *client = ihid->client; >> + struct i2chid_desc *hdesc = &ihid->hdesc; >> + unsigned int dsize = 0; >> + int ret; >> + >> + /* Fetch the length of HID description, retrieve the 4 first bytes: >> + * bytes 0-1 -> length >> + * bytes 2-3 -> bcdVersion (has to be 1.00) */ >> + ret = i2chid_command(client, HID_DESCR_CMD, ihid->hdesc_buffer, 4); >> + >> + if (debug) >> + dev_err(&client->dev, >> + "%s, ihid->hdesc_buffer: %02x %02x %02x %02x\n", > > %*ph again. Ok. > >> + __func__, >> + ihid->hdesc_buffer[0], >> + ihid->hdesc_buffer[1], >> + ihid->hdesc_buffer[2], >> + ihid->hdesc_buffer[3]); >> + >> + if (ret) { >> + dev_err(&client->dev, "HID_DESCR_LENGTH_CMD Fail (ret=%d)\n", >> + ret); >> + return -EINVAL; >> + } >> + >> + dsize = le16_to_cpu(hdesc->wHIDDescLength); >> + if (!dsize || dsize > HID_MAX_DESCRIPTOR_SIZE) { >> + dev_err(&client->dev, "weird size of HID descriptor (%u)\n", >> + dsize); >> + return -EINVAL; >> + } >> + >> + /* check bcdVersion == 1.0 */ >> + if (le16_to_cpu(hdesc->bcdVersion) != 0x0100) { >> + dev_err(&client->dev, >> + "unexpected HID descriptor bcdVersion (0x%04x)\n", >> + le16_to_cpu(hdesc->bcdVersion)); >> + return -EINVAL; >> + } >> + >> + if (debug) >> + dev_err(&client->dev, "Fetching the HID descriptor\n"); >> + >> + ret = i2chid_command(client, HID_DESCR_CMD, ihid->hdesc_buffer, dsize); >> + if (ret) { >> + dev_err(&client->dev, "HID_DESCR_CMD Fail\n"); >> + return -EINVAL; >> + } >> + >> + if (debug) >> + i2chid_print_buffer(ihid, ihid->hdesc_buffer, dsize); >> + >> + return 0; >> +} >> + >> +static int i2chid_parse(struct hid_device *hid) >> +{ >> + struct i2c_client *client = hid->driver_data; >> + struct i2chid *ihid = i2c_get_clientdata(client); >> + struct i2chid_desc *hdesc = &ihid->hdesc; >> + unsigned int rsize = 0; >> + char *rdesc; >> + int ret; >> + int tries = 3; >> + >> + if (debug) >> + dev_err(&client->dev, "entering %s\n", __func__); >> + >> + rsize = le16_to_cpu(hdesc->wReportDescLength); >> + if (!rsize || rsize > HID_MAX_DESCRIPTOR_SIZE) { >> + dbg_hid("weird size of report descriptor (%u)\n", rsize); >> + return -EINVAL; >> + } >> + >> + do { >> + ret = i2chid_hwreset(client); >> + if (ret) >> + msleep(1000); >> + } while (tries-- > 0 && ret); >> + >> + if (ret) >> + return ret; >> + >> + rdesc = kmalloc(rsize, GFP_KERNEL); >> + >> + if (!rdesc) { >> + dbg_hid("couldn't allocate rdesc memory\n"); >> + return -ENOMEM; >> + } >> + >> + if (debug) >> + dev_err(&client->dev, "asking HID report descriptor\n"); >> + >> + ret = i2chid_command(client, HID_REPORT_DESCR_CMD, rdesc, rsize); >> + if (ret) { >> + hid_err(hid, "reading report descriptor failed\n"); >> + kfree(rdesc); >> + ret = -ENOMEM; >> + goto err; >> + } >> + >> + if (debug) >> + i2chid_print_buffer(ihid, rdesc, rsize); >> + >> + ret = hid_parse_report(hid, rdesc, rsize); >> + kfree(rdesc); >> + if (ret) { >> + dbg_hid("parsing report descriptor failed\n"); >> + goto err; >> + } >> + >> + return 0; >> +err: >> + return ret; >> +} >> + >> +static int i2chid_start(struct hid_device *hid) >> +{ >> + struct i2c_client *client = hid->driver_data; >> + struct i2chid *ihid = i2c_get_clientdata(client); >> + int ret; >> + >> + ihid->bufsize = HID_MIN_BUFFER_SIZE; >> + i2chid_find_max_report(hid, HID_INPUT_REPORT, &ihid->bufsize); >> + i2chid_find_max_report(hid, HID_OUTPUT_REPORT, &ihid->bufsize); >> + i2chid_find_max_report(hid, HID_FEATURE_REPORT, &ihid->bufsize); >> + >> + if (ihid->bufsize > HID_MAX_BUFFER_SIZE) >> + ihid->bufsize = HID_MAX_BUFFER_SIZE; >> + >> + if (i2chid_alloc_buffers(ihid)) { >> + ret = -ENOMEM; >> + goto fail; >> + } >> + >> + if (!(hid->quirks & HID_QUIRK_NO_INIT_REPORTS)) >> + i2chid_init_reports(hid); >> + >> + return 0; >> + >> +fail: >> + i2chid_free_buffers(ihid); >> + return ret; >> +} >> + >> +static void i2chid_stop(struct hid_device *hid) >> +{ >> + struct i2c_client *client = hid->driver_data; >> + struct i2chid *ihid = i2c_get_clientdata(client); >> + >> + if (WARN_ON(!ihid)) >> + return; > > How? will drop it. > >> + >> + hid->claimed = 0; > > Should it be here and not in core? This is a line that was copied/pasted from usbhid. I'll check how can I do that without interfering with core. > >> + >> + i2chid_free_buffers(ihid); >> +} >> + >> +static int i2chid_open(struct hid_device *hid) >> +{ >> + struct i2c_client *client = hid->driver_data; >> + struct i2chid *ihid = i2c_get_clientdata(client); >> + int ret; >> + >> + if (!hid->open++) { >> + ret = i2chid_set_power(client, I2C_HID_PWR_ON); >> + if (ret) { >> + hid->open--; >> + return -EIO; >> + } >> + spin_lock_irq(&ihid->flock); >> + set_bit(I2C_HID_STARTED, &ihid->flags); >> + spin_unlock_irq(&ihid->flock); >> + } >> + return 0; >> +} >> + >> +static void i2chid_close(struct hid_device *hid) >> +{ >> + struct i2c_client *client = hid->driver_data; >> + struct i2chid *ihid = i2c_get_clientdata(client); >> + >> + /* protecting hid->open to make sure we don't restart >> + * data acquistion due to a resumption we no longer >> + * care about >> + */ >> + if (!--hid->open) { >> + spin_lock_irq(&ihid->flock); >> + clear_bit(I2C_HID_STARTED, &ihid->flags); >> + spin_unlock_irq(&ihid->flock); >> + >> + /* Save some power */ >> + i2chid_set_power(client, I2C_HID_PWR_SLEEP); >> + } >> +} >> + >> +static int i2chid_power(struct hid_device *hid, int lvl) >> +{ >> + struct i2c_client *client = hid->driver_data; >> + int r = 0; >> + >> + if (debug) >> + dev_err(&client->dev, "%s lvl:%d\n", __func__, lvl); >> + >> + switch (lvl) { >> + case PM_HINT_FULLON: >> + r = i2chid_set_power(client, I2C_HID_PWR_ON); >> + break; >> + case PM_HINT_NORMAL: >> + r = i2chid_set_power(client, I2C_HID_PWR_SLEEP); >> + break; >> + } >> + return r; >> +} >> + >> +static int i2chid_hidinput_input_event(struct input_dev *dev, unsigned int type, >> + unsigned int code, int value) >> +{ >> + struct hid_device *hid = input_get_drvdata(dev); >> + struct hid_field *field; >> + int offset; >> + >> + if (type == EV_FF) >> + return input_ff_event(dev, type, code, value); >> + >> + if (type != EV_LED) >> + return -1; >> + >> + offset = hidinput_find_field(hid, type, code, &field); >> + >> + if (offset == -1) { >> + hid_warn(dev, "event field not found\n"); >> + return -1; >> + } >> + >> + hid_set_field(field, offset, value); >> + >> + return 0; >> +} >> + >> +static struct hid_ll_driver i2c_hid_driver = { >> + .parse = i2chid_parse, >> + .start = i2chid_start, >> + .stop = i2chid_stop, >> + .open = i2chid_open, >> + .close = i2chid_close, >> + .power = i2chid_power, >> + .hidinput_input_event = i2chid_hidinput_input_event, >> +}; >> + >> +static int i2chid_init_irq(struct i2c_client *client) >> +{ >> + struct i2chid *ihid = i2c_get_clientdata(client); >> + int rc; >> + >> + dev_dbg(&client->dev, "Requesting IRQ: %d\n", >> + client->irq); >> + >> + rc = request_threaded_irq(client->irq, NULL, i2chid_irq, >> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, >> + DRIVER_NAME, ihid); >> + if (rc < 0) { >> + dev_dbg(&client->dev, >> + "Could not register for %s interrupt, irq = %d," >> + " rc = %d\n", >> + DRIVER_NAME, client->irq, rc); >> + >> + return rc; >> + } >> + >> + return client->irq; >> +} >> + >> +static int __devinit i2chid_probe(struct i2c_client *client, >> + const struct i2c_device_id *dev_id) >> +{ >> + int ret; >> + struct i2chid *ihid; >> + struct hid_device *hid; >> + __u16 hidRegister; >> + unsigned char tmpstr[11]; >> + struct i2chid_platform_data *platform_data = client->dev.platform_data; >> + >> + dbg_hid("HID probe called for i2c %d\n", client->addr); >> + >> + if (!platform_data) { >> + dev_err(&client->dev, "HID register address not provided\n"); >> + return -EINVAL; >> + } >> + >> + if (client->irq < 1) {o > > Maybe !client->irq? I thought that irqs should be positive or null numbers. This can be easily changed. > >> + dev_err(&client->dev, >> + "HID over i2c has not been provided an Int IRQ\n"); >> + return -EINVAL; >> + } >> + >> + ihid = kzalloc(sizeof(struct i2chid), GFP_KERNEL); >> + if (!ihid) >> + return -ENOMEM; >> + >> + i2c_set_clientdata(client, ihid); >> + >> + ihid->client = client; >> + ihid->flags = 0; >> + >> + hidRegister = platform_data->hid_descriptor_address; >> + ihid->wHIDDescRegister = cpu_to_le16(hidRegister); >> + >> + init_waitqueue_head(&ihid->wait); >> + spin_lock_init(&ihid->flock); >> + >> + ret = i2chid_fetch_hid_descriptor(ihid); >> + if (ret < 0) >> + goto err; >> + >> + ihid->irq = i2chid_init_irq(client); >> + if (ihid->irq < 0) >> + goto err; >> + >> + hid = hid_allocate_device(); >> + if (IS_ERR(hid)) { >> + ret = -ENOMEM; >> + goto err; >> + } >> + >> + ihid->hid = hid; >> + >> + hid->driver_data = client; >> + hid->ll_driver = &i2c_hid_driver; >> + hid->hid_get_raw_report = i2chid_get_raw_report; >> + hid->hid_output_raw_report = i2chid_output_raw_report; >> + hid->ff_init = hid_pidff_init; >> +#ifdef CONFIG_USB_HIDDEV >> + hid->hiddev_connect = hiddev_connect; >> + hid->hiddev_disconnect = hiddev_disconnect; >> + hid->hiddev_hid_event = hiddev_hid_event; >> + hid->hiddev_report_event = hiddev_report_event; >> +#endif >> + hid->dev.parent = &client->dev; >> + hid->bus = BUS_I2C; >> + hid->version = le16_to_cpu(ihid->hdesc.bcdVersion); >> + hid->vendor = le16_to_cpu(ihid->hdesc.wVendorID); >> + hid->product = le16_to_cpu(ihid->hdesc.wProductID); >> + >> + snprintf(tmpstr, sizeof(tmpstr), " %04X:%04X", >> + hid->vendor, hid->product); >> + strlcpy(hid->name, client->name, sizeof(hid->name)); >> + strlcat(hid->name, tmpstr, sizeof(hid->name)); >> + >> + ret = hid_add_device(hid); >> + if (ret) { >> + if (ret != -ENODEV) >> + hid_err(client, "can't add hid device: %d\n", ret); >> + goto err_mem_free; >> + } >> + >> + return 0; >> + >> +err_mem_free: >> + hid_destroy_device(hid); >> + >> +err: >> + if (ihid->irq >= 0) >> + free_irq(ihid->irq, ihid); >> + >> + kfree(ihid); >> + return ret; >> +} >> + >> +static int __devexit i2chid_remove(struct i2c_client *client) >> +{ >> + struct i2chid *ihid = i2c_get_clientdata(client); >> + struct hid_device *hid; >> + >> + if (WARN_ON(!ihid)) >> + return -1; >> + >> + hid = ihid->hid; >> + hid_destroy_device(hid); >> + >> + free_irq(client->irq, ihid); >> + >> + kfree(ihid); >> + >> + return 0; >> +} >> + >> +#ifdef CONFIG_PM_SLEEP >> +static int i2chid_suspend(struct device *dev) >> +{ >> + struct i2c_client *client = to_i2c_client(dev); >> + struct i2chid *ihid = i2c_get_clientdata(client); >> + >> + if (device_may_wakeup(&client->dev)) >> + enable_irq_wake(ihid->irq); >> + >> + /* Save some power */ >> + i2chid_set_power(client, I2C_HID_PWR_SLEEP); >> + >> + return 0; >> +} >> + >> +static int i2chid_resume(struct device *dev) >> +{ >> + int ret; >> + struct i2c_client *client = to_i2c_client(dev); >> + >> + ret = i2chid_hwreset(client); >> + if (ret) >> + return ret; >> + >> + if (device_may_wakeup(&client->dev)) >> + enable_irq_wake(client->irq); >> + >> + return 0; >> +} >> +#endif >> + >> +static SIMPLE_DEV_PM_OPS(i2chid_pm, i2chid_suspend, i2chid_resume); >> + >> +static const struct i2c_device_id i2chid_id_table[] = { >> + { "i2chid", 0 }, >> + { }, >> +}; >> +MODULE_DEVICE_TABLE(i2c, i2chid_id_table); >> + >> + >> +static struct i2c_driver i2chid_driver = { >> + .driver = { >> + .name = DRIVER_NAME, >> + .owner = THIS_MODULE, >> + .pm = &i2chid_pm, >> + }, >> + >> + .probe = i2chid_probe, >> + .remove = __devexit_p(i2chid_remove), >> + >> + .id_table = i2chid_id_table, >> +}; >> + >> +module_i2c_driver(i2chid_driver); >> + >> +MODULE_DESCRIPTION(DRIVER_DESC); >> +MODULE_AUTHOR("Benjamin Tissoires <benjamin.tissoires@xxxxxxxxx>"); >> +MODULE_LICENSE("GPL"); >> diff --git a/include/linux/i2c/i2c-hid.h b/include/linux/i2c/i2c-hid.h >> new file mode 100644 >> index 0000000..6685605 >> --- /dev/null >> +++ b/include/linux/i2c/i2c-hid.h >> @@ -0,0 +1,35 @@ >> +/* >> + * HID over I2C protocol implementation >> + * >> + * Copyright (c) 2012 Benjamin Tissoires <benjamin.tissoires@xxxxxxxxx> >> + * Copyright (c) 2012 Ecole Nationale de l'Aviation Civile, France >> + * >> + * This file is subject to the terms and conditions of the GNU General Public >> + * License. See the file COPYING in the main directory of this archive for >> + * more details. >> + */ >> + >> +#ifndef __LINUX_I2C_HID_H >> +#define __LINUX_I2C_HID_H >> + >> +#include <linux/types.h> >> + >> +/** >> + * struct i2chid_platform_data - used by hid over i2c implementation. >> + * @hid_descriptor_address: i2c register where the HID descriptor is stored. >> + * >> + * Note that it is the responsibility for the platform driver (or the acpi 5.0 >> + * driver) to setup the irq related to the gpio in the struct i2c_board_info. >> + * The platform driver should also setup the gpio according to the device: >> + * >> + * A typical example is the following: >> + * irq = gpio_to_irq(intr_gpio); >> + * hkdk4412_i2c_devs5[0].irq = irq; // store the irq in i2c_board_info >> + * gpio_request(intr_gpio, "elan-irq"); >> + * s3c_gpio_setpull(intr_gpio, S3C_GPIO_PULL_UP); >> + */ >> +struct i2chid_platform_data { >> + u16 hid_descriptor_address; >> +}; >> + >> +#endif /* __LINUX_I2C_HID_H */ >> -- >> 1.7.11.4 >> > > Thanks. Thanks to you Dmitry. I will try to post the v2 soon. Cheers, Benjamin > > -- > Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
