Re: [PATCH v3] i2c-hid: introduce HID over i2c specification implementation
On Mon, 3 Dec 2012, Benjamin Tissoires wrote: I know that it already have been used by one Nvidia team and by Elan for internal tests. So I don't know if it's possible to change it now (though it's not a big deal). Yes it is possible, as long as the code isn't in Linus' tree (and I'd even say: as long as it is not in any kernel released by Linus.) Whoever is already using your driver will have to adjust their code. They'll certainly want to anyway, in order to get the bug fixes. Anyway, hid is a little weird to me (but this is because I started hacking the kernel from there), as it's really short and does not contain i2c :) This is exactly my point: no other i2c client (to my knowledge) has i2c in its name, because it is redundant. I would agree that this is kind of a special case as this is a generic driver and not a device-specific driver. I would still prefer hid but if you don't feel like changing it, I guess I can live with that. Ok, I'll change the name. My concerns were just because from my point of view, hid refers to the core implementation. But for OEMs, they need to declare a hid device, so it makes sense. Anyway, when I'll be able to get my hand on an ACPI 5 device with hid over i2c, I'll be able to write the missing autoloading code, and this name will be only used by a small part of OEMs (I hope). If you are going to change the name, please either send me the patch before merge window for 3.8 opens, or explicitly ask me not to include this driver in pull request that goes to Linus yet. Once the driver is in Linus' tree, I'd rather not change the name. Thanks, -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] i2c-hid: introduce HID over i2c specification implementation
On Tue, Dec 4, 2012 at 11:31 AM, Jiri Kosina jkos...@suse.cz wrote: On Mon, 3 Dec 2012, Benjamin Tissoires wrote: I know that it already have been used by one Nvidia team and by Elan for internal tests. So I don't know if it's possible to change it now (though it's not a big deal). Yes it is possible, as long as the code isn't in Linus' tree (and I'd even say: as long as it is not in any kernel released by Linus.) Whoever is already using your driver will have to adjust their code. They'll certainly want to anyway, in order to get the bug fixes. Anyway, hid is a little weird to me (but this is because I started hacking the kernel from there), as it's really short and does not contain i2c :) This is exactly my point: no other i2c client (to my knowledge) has i2c in its name, because it is redundant. I would agree that this is kind of a special case as this is a generic driver and not a device-specific driver. I would still prefer hid but if you don't feel like changing it, I guess I can live with that. Ok, I'll change the name. My concerns were just because from my point of view, hid refers to the core implementation. But for OEMs, they need to declare a hid device, so it makes sense. Anyway, when I'll be able to get my hand on an ACPI 5 device with hid over i2c, I'll be able to write the missing autoloading code, and this name will be only used by a small part of OEMs (I hope). If you are going to change the name, please either send me the patch before merge window for 3.8 opens, or explicitly ask me not to include this driver in pull request that goes to Linus yet. Sure, I intend to send some patches today (with the change of the name included). I'll send the patch related to the name in the first position so that you can safely pick it up even if you are not satisfied with the other changes. Thanks, Benjamin Once the driver is in Linus' tree, I'd rather not change the name. Thanks, -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] i2c-hid: introduce HID over i2c specification implementation
Hi Jean,
On Fri, Nov 30, 2012 at 3:56 PM, Jean Delvare kh...@linux-fr.org wrote:
Hi Benjamin, Jiri,
Sorry for the late review. But better late than never I guess...
Sure! Thanks for the review. As the driver is already in Jiri's tree,
I'll do small incremental patches based on this release.
I'll try to address all of your comments.
I have a few answers to some of your remarks (I fully agree with all
of the others):
On Mon, 12 Nov 2012 15:42:59 +0100, Benjamin Tissoires wrote:
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 enumeration will be available.
Once the ACPI part is done, OEM will not have to declare HID over I2C
devices in their platform specific driver.
Signed-off-by: Benjamin Tissoires benjamin.tissoi...@gmail.com
---
(..)
drivers/hid/Kconfig | 2 +
drivers/hid/Makefile | 1 +
drivers/hid/i2c-hid/Kconfig | 21 +
drivers/hid/i2c-hid/Makefile | 5 +
drivers/hid/i2c-hid/i2c-hid.c | 974
++
include/linux/i2c/i2c-hid.h | 35 ++
6 files changed, 1038 insertions(+)
create mode 100644 drivers/hid/i2c-hid/Kconfig
create mode 100644 drivers/hid/i2c-hid/Makefile
create mode 100644 drivers/hid/i2c-hid/i2c-hid.c
create mode 100644 include/linux/i2c/i2c-hid.h
Looks much better. I still have several random comments which you may
be interested in.
(...)
+/* debug option */
+static bool debug = false;
Whether you like it or not, that's still an error for checkpatch:
ERROR: do not initialise statics to 0 or NULL
#240: FILE: drivers/hid/i2c-hid/i2c-hid.c:49:
+static bool debug = false;
The rationale is that the compiler can write more efficient code if
initializations to 0 or NULL are implicit.
ok, will do.
+module_param(debug, bool, 0444);
+MODULE_PARM_DESC(debug, print a lot of debug information);
+
+#define i2c_hid_dbg(ihid, fmt, arg...) \
+ if (debug) \
+ dev_printk(KERN_DEBUG, (ihid)-client-dev, fmt, ##arg)
This is considered an error by checkpatch too:
ERROR: Macros with complex values should be enclosed in parenthesis
#244: FILE: drivers/hid/i2c-hid/i2c-hid.c:53:
+#define i2c_hid_dbg(ihid, fmt, arg...) \
+ if (debug) \
+ dev_printk(KERN_DEBUG, (ihid)-client-dev, fmt, ##arg)
And I wouldn't disagree, as the construct above can lead to bugs which
are hard to see... Consider the following piece of code:
if (condition)
i2c_hid_dbg(ihid, Blah blah %d\n, i);
else
do_something_very_important();
It looks correct, however with the macro definition above, this expands
to the unexpected:
if (condition) {
if (debug) \
dev_printk(KERN_DEBUG, ihid-client-dev,
Blah blah %d\n, i);
else
do_something_very_important();
}
That is, the condition to call do_something_very_important() is
condition !debug, and not !condition as the code suggests. This is
only an example and your driver doesn't currently use any such
construct. Still I believe this should be fixed.
With your explanation, you've convinced me. I was not sure on how
should I handle this warning as I copied this macro from one in hid.
I will fix the two then.
(...)
static int i2c_hid_alloc_buffers(struct i2c_hid *ihid)
{
/* the worst case is computed from the set_report command with a
* reportID 15 and the maximum report length */
int args_len = sizeof(__u8) + /* optional ReportID byte */
sizeof(__u16) + /* data register */
sizeof(__u16) + /* size of the report */
ihid-bufsize; /* report */
ihid-inbuf = kzalloc(ihid-bufsize, GFP_KERNEL);
ihid-argsbuf = kzalloc(args_len, GFP_KERNEL);
ihid-cmdbuf = kzalloc(sizeof(union command) + args_len, GFP_KERNEL);
if (!ihid-inbuf || !ihid-argsbuf || !ihid-cmdbuf) {
i2c_hid_free_buffers(hid);
return -ENOMEM;
}
return 0;
}
Much better, thanks!
+static void i2c_hid_free_buffers(struct i2c_hid *ihid)
+{
+ kfree(ihid-inbuf);
+ kfree(ihid-argsbuf);
+ kfree(ihid-cmdbuf);
+ ihid-inbuf = NULL;
+ ihid-cmdbuf = NULL;
+ ihid-argsbuf = NULL;
+}
+
+static int i2c_hid_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 i2c_hid *ihid =
Re: [PATCH v3] i2c-hid: introduce HID over i2c specification implementation
Hi Benjamin,
On Mon, 3 Dec 2012 12:32:03 +0100, Benjamin Tissoires wrote:
Hi Jean,
On Fri, Nov 30, 2012 at 3:56 PM, Jean Delvare kh...@linux-fr.org wrote:
Hi Benjamin, Jiri,
Sorry for the late review. But better late than never I guess...
Sure! Thanks for the review. As the driver is already in Jiri's tree,
I'll do small incremental patches based on this release.
I'll try to address all of your comments.
I have a few answers to some of your remarks (I fully agree with all
of the others):
On Mon, 12 Nov 2012 15:42:59 +0100, Benjamin Tissoires wrote:
+static int i2c_hid_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 i2c_hid *ihid = i2c_get_clientdata(client);
+ int ret;
+
+ if (report_type == HID_OUTPUT_REPORT)
+ return -EINVAL;
+
+ if (count ihid-bufsize)
+ count = ihid-bufsize;
+
+ ret = i2c_hid_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);
What guarantee do you have that count is not larger than buf's length?
I hope you don't just count on all hardware out there being nice and
sane, do you? ;)
Hehe, this function is never called from the device, but from the user
space. It's called by hidraw_get_report in drivers/hid/hidraw.c, and
the caller makes the allocation of buf with a size of count.
There is an other usage in hid-input.c with buf, sizeof(buf), as arguments.
So this should never be a problem as long as anybody else call this
function without making sure count is the right size.
Not sure I follow you here.
There are two flavors of count in this function. The first one is
passed as a parameter by the caller and used to set the buffer length
as passed to i2c_hid_get_report(). This part is fine and I have no
problem with it. The second flavor is extracted from ihid-inbuf as
provided by i2c_hid_get_report(). As I understand it,
i2c_hid_get_report() fills the buffer with data it receives from the
hardware, so I fail to see how you can have any guarantee that this
second flavor of count is not greater than buf's length.
In fact I don't think you should reuse count in the first place,
that's confusing. Then, looking at the code again, I don't think the
test count ihid-bufsize, and more generally changing the value of
count, makes sense. You don't care about the size of buf when
calling i2c_hid_get_report(), you care about the size of ihid-inbuf,
which should be at least 2 more than the size of buf. You care about
the size of buf _after_ the call to i2c_hid_get_report(), as you are
copying the data from ihid-bufsize to buf. This is precisely the
check which I claim is missing.
(...)
+
+ hid = ihid-hid;
+ hid_destroy_device(hid);
+
+ free_irq(client-irq, ihid);
+
Is there any guarantee that i2c_hid_stop() has been called before
i2c_hid_remove() is? If not, you're missing a call to
i2c_hid_free_buffers() here. BTW I'm not quite sure why
i2c_hid_remove() frees the buffers in the first place, but then again I
don't know a thing about the HID infrastructure.
Calling i2c_hid_stop() is the responsibility of the hid driver at his
remove. By hid driver, I mean the driver that relies on hid to handle
the device (hid-generic in 80% of the cases) So as long as this hid
driver is loaded, we can not remove i2c_hid as it is used by the hid
driver. So it seems that this guarantees that i2c_hid_stop() has been
called before i2c_hid_remove() is.
But now that I think of it, there are cases where i2c_hid_stop() is
not called: when the hid driver failed to probe. So definitively,
there is a mem leak here. Thanks.
Actually this path was fixed by Jiri already:
http://git.kernel.org/?p=linux/kernel/git/jikos/hid.git;a=commitdiff;h=3c62602434c13744df62b3ab0ab7950cd36f24db
My worry was that, when probe succeeds, i2c_hid_start/stop() could
never be called if the module was unloaded immediately for example,
before user-space has a chance to use the device. But if you are
certain it can't happen, then alright.
+(...)
+static const struct i2c_device_id i2c_hid_id_table[] = {
+ { i2c_hid, 0 },
I am just realizing i2c_hid is a little redundant as an i2c device
name. Can we make this just hid?
I know that it already have been used by one Nvidia team and by Elan
for internal tests. So I don't know if it's possible to change it now
(though it's not a big deal).
Yes it is possible, as long as the code isn't in Linus' tree (and I'd
even say: as long as it is not in any kernel released by Linus.)
Whoever is already using your driver
Re: [PATCH v3] i2c-hid: introduce HID over i2c specification implementation
On Mon, Dec 3, 2012 at 2:02 PM, Jean Delvare kh...@linux-fr.org wrote:
Hi Benjamin,
On Mon, 3 Dec 2012 12:32:03 +0100, Benjamin Tissoires wrote:
Hi Jean,
On Fri, Nov 30, 2012 at 3:56 PM, Jean Delvare kh...@linux-fr.org wrote:
Hi Benjamin, Jiri,
Sorry for the late review. But better late than never I guess...
Sure! Thanks for the review. As the driver is already in Jiri's tree,
I'll do small incremental patches based on this release.
I'll try to address all of your comments.
I have a few answers to some of your remarks (I fully agree with all
of the others):
On Mon, 12 Nov 2012 15:42:59 +0100, Benjamin Tissoires wrote:
+static int i2c_hid_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 i2c_hid *ihid = i2c_get_clientdata(client);
+ int ret;
+
+ if (report_type == HID_OUTPUT_REPORT)
+ return -EINVAL;
+
+ if (count ihid-bufsize)
+ count = ihid-bufsize;
+
+ ret = i2c_hid_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);
What guarantee do you have that count is not larger than buf's length?
I hope you don't just count on all hardware out there being nice and
sane, do you? ;)
Hehe, this function is never called from the device, but from the user
space. It's called by hidraw_get_report in drivers/hid/hidraw.c, and
the caller makes the allocation of buf with a size of count.
There is an other usage in hid-input.c with buf, sizeof(buf), as arguments.
So this should never be a problem as long as anybody else call this
function without making sure count is the right size.
Not sure I follow you here.
There are two flavors of count in this function. The first one is
passed as a parameter by the caller and used to set the buffer length
as passed to i2c_hid_get_report(). This part is fine and I have no
problem with it. The second flavor is extracted from ihid-inbuf as
provided by i2c_hid_get_report(). As I understand it,
i2c_hid_get_report() fills the buffer with data it receives from the
hardware, so I fail to see how you can have any guarantee that this
second flavor of count is not greater than buf's length.
In fact I don't think you should reuse count in the first place,
that's confusing. Then, looking at the code again, I don't think the
test count ihid-bufsize, and more generally changing the value of
count, makes sense. You don't care about the size of buf when
calling i2c_hid_get_report(), you care about the size of ihid-inbuf,
which should be at least 2 more than the size of buf. You care about
the size of buf _after_ the call to i2c_hid_get_report(), as you are
copying the data from ihid-bufsize to buf. This is precisely the
check which I claim is missing.
Oops. Really sorry. You are perfectly right.
This mismatch comes from the reuse of count for 2 different purposes,
so it was not a good idea at all.
Will fix it. And thanks!
(...)
+
+ hid = ihid-hid;
+ hid_destroy_device(hid);
+
+ free_irq(client-irq, ihid);
+
Is there any guarantee that i2c_hid_stop() has been called before
i2c_hid_remove() is? If not, you're missing a call to
i2c_hid_free_buffers() here. BTW I'm not quite sure why
i2c_hid_remove() frees the buffers in the first place, but then again I
don't know a thing about the HID infrastructure.
Calling i2c_hid_stop() is the responsibility of the hid driver at his
remove. By hid driver, I mean the driver that relies on hid to handle
the device (hid-generic in 80% of the cases) So as long as this hid
driver is loaded, we can not remove i2c_hid as it is used by the hid
driver. So it seems that this guarantees that i2c_hid_stop() has been
called before i2c_hid_remove() is.
But now that I think of it, there are cases where i2c_hid_stop() is
not called: when the hid driver failed to probe. So definitively,
there is a mem leak here. Thanks.
Actually this path was fixed by Jiri already:
http://git.kernel.org/?p=linux/kernel/git/jikos/hid.git;a=commitdiff;h=3c62602434c13744df62b3ab0ab7950cd36f24db
Not exactly. This commit fixes the memory leak when i2c_hid_probe
fails. Not when the hid driver (hid-generic or hid-multitouch in our
cases) fails.
My worry was that, when probe succeeds, i2c_hid_start/stop() could
never be called if the module was unloaded immediately for example,
before user-space has a chance to use the device. But if you are
certain it can't happen, then alright.
I made a few tests after sending the mail. Contrary to what I said,
it's possible to unload the i2c-hid module before the hid
Re: [PATCH v3] i2c-hid: introduce HID over i2c specification implementation
Hi Benjamin, Jiri,
Sorry for the late review. But better late than never I guess...
On Mon, 12 Nov 2012 15:42:59 +0100, Benjamin Tissoires wrote:
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 enumeration will be available.
Once the ACPI part is done, OEM will not have to declare HID over I2C
devices in their platform specific driver.
Signed-off-by: Benjamin Tissoires benjamin.tissoi...@gmail.com
---
(..)
drivers/hid/Kconfig | 2 +
drivers/hid/Makefile | 1 +
drivers/hid/i2c-hid/Kconfig | 21 +
drivers/hid/i2c-hid/Makefile | 5 +
drivers/hid/i2c-hid/i2c-hid.c | 974
++
include/linux/i2c/i2c-hid.h | 35 ++
6 files changed, 1038 insertions(+)
create mode 100644 drivers/hid/i2c-hid/Kconfig
create mode 100644 drivers/hid/i2c-hid/Makefile
create mode 100644 drivers/hid/i2c-hid/i2c-hid.c
create mode 100644 include/linux/i2c/i2c-hid.h
Looks much better. I still have several random comments which you may
be interested in.
(...)
diff --git a/drivers/hid/i2c-hid/Kconfig b/drivers/hid/i2c-hid/Kconfig
new file mode 100644
index 000..5b7d4d8
--- /dev/null
+++ b/drivers/hid/i2c-hid/Kconfig
@@ -0,0 +1,21 @@
+menu I2C HID support
+ depends on I2C
+
+config I2C_HID
+ tristate HID over I2C transport layer
+ default n
+ depends on I2C INPUT
+ select HID
+ ---help---
+ Say Y here if you want to use the HID over i2c protocol
+ implementation.
s/i2c/I2C/
The USB equivalent lists a number of examples, maybe you could do the
same here so that the reader knows if he/she needs this driver.
+
+ If unsure, say N.
+
+ This support is also available as a module. If so, the module
+ will be called i2c-hid.
+
+comment Input core support is needed for HID over I2C input layer
+ depends on I2C_HID INPUT=n
Given that the HID menu itself depends on INPUT, I fail to see how this
comment would ever be displayed. Same question holds for the USB
equivalent.
+
+endmenu
(...)
diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
new file mode 100644
index 000..11140bd
--- /dev/null
+++ b/drivers/hid/i2c-hid/i2c-hid.c
(...)
+/* flags */
+#define I2C_HID_STARTED (1 0)
+#define I2C_HID_RESET_PENDING(1 1)
+#define I2C_HID_READ_PENDING (1 2)
+
+#define I2C_HID_PWR_ON 0x00
+#define I2C_HID_PWR_SLEEP0x01
+
+/* debug option */
+static bool debug = false;
Whether you like it or not, that's still an error for checkpatch:
ERROR: do not initialise statics to 0 or NULL
#240: FILE: drivers/hid/i2c-hid/i2c-hid.c:49:
+static bool debug = false;
The rationale is that the compiler can write more efficient code if
initializations to 0 or NULL are implicit.
+module_param(debug, bool, 0444);
+MODULE_PARM_DESC(debug, print a lot of debug information);
+
+#define i2c_hid_dbg(ihid, fmt, arg...) \
+ if (debug) \
+ dev_printk(KERN_DEBUG, (ihid)-client-dev, fmt, ##arg)
This is considered an error by checkpatch too:
ERROR: Macros with complex values should be enclosed in parenthesis
#244: FILE: drivers/hid/i2c-hid/i2c-hid.c:53:
+#define i2c_hid_dbg(ihid, fmt, arg...) \
+ if (debug) \
+ dev_printk(KERN_DEBUG, (ihid)-client-dev, fmt, ##arg)
And I wouldn't disagree, as the construct above can lead to bugs which
are hard to see... Consider the following piece of code:
if (condition)
i2c_hid_dbg(ihid, Blah blah %d\n, i);
else
do_something_very_important();
It looks correct, however with the macro definition above, this expands
to the unexpected:
if (condition) {
if (debug) \
dev_printk(KERN_DEBUG, ihid-client-dev,
Blah blah %d\n, i);
else
do_something_very_important();
}
That is, the condition to call do_something_very_important() is
condition !debug, and not !condition as the code suggests. This is
only an example and your driver doesn't currently use any such
construct. Still I believe this should be fixed.
(...)
+#define I2C_HID_CMD(opcode_) \
+ .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,
+
Re: [PATCH v3] i2c-hid: introduce HID over i2c specification implementation
On Mon, 12 Nov 2012, Benjamin Tissoires wrote: 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 enumeration will be available. Once the ACPI part is done, OEM will not have to declare HID over I2C devices in their platform specific driver. Signed-off-by: Benjamin Tissoires benjamin.tissoi...@gmail.com --- Hi Guys, here is the v3 of the HID over I2C driver. Changes in v3: * fix i2c_hid_alloc_buffers when allocation failed * fix error messages to be more human * fix i2c_hid_set_report as it didn't followed the protocol * remove i2c_hid_write_out_report (hid-core doesn't present the callback that could use this feature of the protocol) * reject HID_OUTPUT_REPORT in i2c_hid_get_raw_report * fix output report id in i2c_hid_output_raw_report and reject HID_INPUT_REPORT Benjamin, I am now queuing this in for-3.8/i2c-hid branch, so please send any potential followup patches on top of that. Thanks a lot for all your work on this so far. -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] i2c-hid: introduce HID over i2c specification implementation
On Thu, Nov 15, 2012 at 3:04 PM, Benjamin Tissoires benjamin.tissoi...@gmail.com wrote: On Thu, Nov 15, 2012 at 2:51 PM, Jiri Kosina jkos...@suse.cz wrote: On Mon, 12 Nov 2012, Benjamin Tissoires wrote: 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 enumeration will be available. Once the ACPI part is done, OEM will not have to declare HID over I2C devices in their platform specific driver. Signed-off-by: Benjamin Tissoires benjamin.tissoi...@gmail.com Out of curiosity -- has this been tested on a real device (is there any such device available anyway?), or is that just the implementation of the defined protocol? It has been tested on an ELAN microelectronics device (a prototype), on an odroid-x board. That's how we figure out the bug in the set_report command. I think we manage to test all main features of the protocol (get_report, irqs, hid descriptor, report descriptors, set_report). I'm currently waiting for a Synaptics touchpad to check if it's also working with their devices. The thing is that HID over i2c for x86 platform will presumably require the Haswell platform from Intel (we need ACPI 5 for enumeration), but it would be very nice to get this in the kernel just before hardware arrive on the market :) However, I won't be surprise if android OEMs also start using this specification because it won't force them to write kernel drivers... And as a complement, Ramalingam tested it for Nvidia on an early NVIDIA's Tegra reference board for PISMO which is registered at http://www.arm.linux.org.uk/developer/machines/list.php?id=4439 , with a HID over i2c keyboard. He is up to test also his Synaptics touchpad. Cheers, Benjamin Cheers, Benjamin Thanks, -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] i2c-hid: introduce HID over i2c specification implementation
On Mon, 12 Nov 2012, Benjamin Tissoires wrote: 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 enumeration will be available. Once the ACPI part is done, OEM will not have to declare HID over I2C devices in their platform specific driver. Signed-off-by: Benjamin Tissoires benjamin.tissoi...@gmail.com Out of curiosity -- has this been tested on a real device (is there any such device available anyway?), or is that just the implementation of the defined protocol? Thanks, -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] i2c-hid: introduce HID over i2c specification implementation
On Thu, Nov 15, 2012 at 2:51 PM, Jiri Kosina jkos...@suse.cz wrote: On Mon, 12 Nov 2012, Benjamin Tissoires wrote: 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 enumeration will be available. Once the ACPI part is done, OEM will not have to declare HID over I2C devices in their platform specific driver. Signed-off-by: Benjamin Tissoires benjamin.tissoi...@gmail.com Out of curiosity -- has this been tested on a real device (is there any such device available anyway?), or is that just the implementation of the defined protocol? It has been tested on an ELAN microelectronics device (a prototype), on an odroid-x board. That's how we figure out the bug in the set_report command. I think we manage to test all main features of the protocol (get_report, irqs, hid descriptor, report descriptors, set_report). I'm currently waiting for a Synaptics touchpad to check if it's also working with their devices. The thing is that HID over i2c for x86 platform will presumably require the Haswell platform from Intel (we need ACPI 5 for enumeration), but it would be very nice to get this in the kernel just before hardware arrive on the market :) However, I won't be surprise if android OEMs also start using this specification because it won't force them to write kernel drivers... Cheers, Benjamin Thanks, -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
