Re: [PATCH v1] i2c-hid: introduce HID over i2c specification implementation
On Sun, Oct 7, 2012 at 9:03 PM, Jean Delvare wrote: > On Sun, 7 Oct 2012 18:57:36 +0200, Benjamin Tissoires wrote: >> On Sun, Oct 7, 2012 at 4:28 PM, Jean Delvare wrote: >> > On Fri, 14 Sep 2012 15:41:43 +0200, benjamin.tissoires wrote: >> >> + u16 readRegister = ihid->hdesc.wDataRegister; >> > >> > This is missing le16_to_cpu(). >> >> I agree this is awful, but not putting it allows me to not have to >> check the endianness when I'm using it. >> But I may be totally wrong on this. > > I'm afraid I don't follow you. I want to see: > > u16 readRegister = le16_to_cpu(ihid->hdesc.wDataRegister); > > If you don't do that, your driver is broken on bigendian systems. And > there's no need to "check the endianness when you're using it", the > above should be enough for things to work just fine. > a little bit late, but yes, you are entirely right. I was confused by the fact that I only wanted to use the number coming from the device to communicate with it, but as I made bit shifting within the CPU, I need to convert it. So forget my previous words here, it is fixed in v2. Thanks Benjamin > -- > Jean Delvare -- 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 v1] i2c-hid: introduce HID over i2c specification implementation
On Sun, 7 Oct 2012, Benjamin Tissoires wrote: > It seems that hid transport layers should go in drivers/hid. > However, I don't like mixing the transport layer and the final > drivers. Maybe this is the time to rework a little bit the tree. > To minimize the moves, we could introduce: > drivers/hid/busses/usb > drivers/hid/busses/i2c > drivers/hid/busses/bluetooth Something like that, I would personally like. But I am not going to forcefully take it over without bluetooth guys agreeing on that as well. -- 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 v1] i2c-hid: introduce HID over i2c specification implementation
On Sun, 7 Oct 2012 18:57:36 +0200, Benjamin Tissoires wrote: > On Sun, Oct 7, 2012 at 4:28 PM, Jean Delvare wrote: > > On Fri, 14 Sep 2012 15:41:43 +0200, benjamin.tissoires wrote: > >> + u16 readRegister = ihid->hdesc.wDataRegister; > > > > This is missing le16_to_cpu(). > > I agree this is awful, but not putting it allows me to not have to > check the endianness when I'm using it. > But I may be totally wrong on this. I'm afraid I don't follow you. I want to see: u16 readRegister = le16_to_cpu(ihid->hdesc.wDataRegister); If you don't do that, your driver is broken on bigendian systems. And there's no need to "check the endianness when you're using it", the above should be enough for things to work just fine. -- Jean Delvare -- 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 v1] i2c-hid: introduce HID over i2c specification implementation
Hello Jean, many thanks for this detailed review. I agree to almost all of your comments, so I didn't add an 'ok' after each remark. This review will give me some work, but the driver will be much better. On Sun, Oct 7, 2012 at 4:28 PM, Jean Delvare wrote: > Salut Benjamin, > > On Fri, 14 Sep 2012 15:41:43 +0200, benjamin.tissoires wrote: >> From: Benjamin Tissoires >> >> 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 >> --- >> >> 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. > > Code review follows. It is by no way meant to be complete, as I don't > know a thing about HID. I hope you'll find it useful nevertheless. > >> >> 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" > > You are definitely missing dependencies here, CONFIG_HID at least. > >> + 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 000..eb17d8c >> --- /dev/null >> +++ b/drivers/i2c/i2c-hid.c >> @@ -0,0 +1,1027 @@ >> +/* >> + * HID over I2C protocol implementation >> + * >> + * Copyright (c) 2012 Benjamin Tissoires >> + * 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 >> + * Copyright (c) 2005 Michael Haboustak 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 >> +#include >> +#include > > I don't think you need to include that one, everything irq-related you > need comes with below. The header comment in that > file actually says: "Please do not include this file in generic code." > >> +#include > > Your driver makes no use of GPIO. > >> +#include >> +#include >> +#include >> +#include >> +#include > > You are missing for > spin_lock_irq()/spin_unlock_irq(), for > device_may_wakeup(), for wait_event_timeout(), > for IS_ERR(), for strcat()/memcpy(), > for list_for_each_entry() and for > msecs_to_jiffies(). I'd suggest including as well, for > sprintf() if nothing else, and for WARN_ON(). It seems like I've been to lazy with the includes... Thanks! > >> + >> +#include >> + >> +#include >> +#include >> +#include > > I don't think you using anything from , do you? oops... > >> + >> +#define DRIVER_NAME "i2chid" >> +#define DRIVER_DESC "HID over I2C core driver" > > I see little interest in this define, as you're only using it once. > Even DRIVER_NAME isn't so useful, 2 of the 3 occurrences would be > replaced with client->name.& > >> + >> +#define I2C_HID_COMMAND_TRIES3 >> + >> +/* fla
Re: [PATCH v1] i2c-hid: introduce HID over i2c specification implementation
Le 7 oct. 2012 à 18:07, Benjamin Tissoires a écrit : >>> >>> Basically, to me this all boils down to the question -- what is more >>> important: low-level transport being used, or the general function of the >>> device? >>> >>> To me, it's the latter, and as such, everything would belong under >>> drivers/hid. >> >> Then shouldn't is be drivers/input, rather? > > Ouch, it will introduce more and more complexity. Purely rhetorical question, I agree. But still. > > It seems that hid transport layers should go in drivers/hid. > However, I don't like mixing the transport layer and the final > drivers. Maybe this is the time to rework a little bit the tree. > To minimize the moves, we could introduce: > drivers/hid/busses/usb > drivers/hid/busses/i2c > drivers/hid/busses/bluetooth What about creating drivers/hid/core and move all generic stuff there? That is: drivers/hid/core drivers/hid/usb drivers/hid/i2c drivers/hid/bluetooth Cheers, St. -- 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 v1] i2c-hid: introduce HID over i2c specification implementation
On Sat, Oct 6, 2012 at 11:39 PM, Stéphane Chatty wrote: > > Le 6 oct. 2012 à 23:28, Jiri Kosina a écrit : > >> On Sat, 6 Oct 2012, Jiri Kosina wrote: >> My vote is a clear 3. It took me a few years to kick all users (as opposed to implementers) of i2c from drivers/i2c and finding them a proper home, I'm not going to accept new intruders. Grouping drivers according to what they implement makes it a lot easier to share code and ideas between related drivers. If you want to convince yourself, just imagine the mess it would be if all drivers for PCI devices lived under drivers/pci. >>> >>> This is more or less consistent with my original opinion when I was >>> refactoring the HID layer out of the individual drivers a few years ago. >>> >>> But Marcel objected that he wants to keep all the bluetooth-related >>> drivers under net/bluetooth, and I didn't really want to push hard against >>> this, because I don't have really super-strong personal preference either >>> way. >>> >>> But we definitely can use this oportunity to bring this up for discussion >>> again. >> >> Basically, to me this all boils down to the question -- what is more >> important: low-level transport being used, or the general function of the >> device? >> >> To me, it's the latter, and as such, everything would belong under >> drivers/hid. > > Then shouldn't is be drivers/input, rather? Ouch, it will introduce more and more complexity. It seems that hid transport layers should go in drivers/hid. However, I don't like mixing the transport layer and the final drivers. Maybe this is the time to rework a little bit the tree. To minimize the moves, we could introduce: drivers/hid/busses/usb drivers/hid/busses/i2c drivers/hid/busses/bluetooth Cheers, Benjamin > > St. > > -- 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 v1] i2c-hid: introduce HID over i2c specification implementation
Hi Jean, Thanks for the comments, the tests and the review. I'm going to try to answer most of the remarks, so here is the first: On Sat, Oct 6, 2012 at 10:04 PM, Jean Delvare wrote: > Hi Benjamin, > [...] >a > ERROR: "hiddev_report_event" [drivers/i2c/i2c-hid.ko] undefined! > ERROR: "hiddev_disconnect" [drivers/i2c/i2c-hid.ko] undefined! > ERROR: "hiddev_connect" [drivers/i2c/i2c-hid.ko] undefined! > ERROR: "hid_pidff_init" [drivers/i2c/i2c-hid.ko] undefined! > make[1]: *** [__modpost] Erreur 1 > make: *** [modules] Erreur 2 > > This is because these functions aren't exported and I tried to build > i2c-hid as a module.BTW I see that these functions are part of the > usbhid driver, which looks seriously wrong. If these functions are > transport layer-independent, they should be moved to the hid-code or > some sub-module. One should be able to enable HID-over-I2C without > HID-over-USB. It looks like I've been mislead by the generic names of these functions. By looking at the code, it appears that I can not use them in their current state as they are all usb related. I think that it will need some work to refactor the general part of hiddev so that I2C and bluetooth can use them. For the next version, I'll just drop hiddev and pidff support. I'm not sure we won't ever find a ff device connected through i2c, but the hiddev support will surely be needed. Cheers, Benjamin > > -- > Jean Delvare -- 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 v1] i2c-hid: introduce HID over i2c specification implementation
Salut Benjamin, On Fri, 14 Sep 2012 15:41:43 +0200, benjamin.tissoires wrote: > From: Benjamin Tissoires > > 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 > --- > > 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. Code review follows. It is by no way meant to be complete, as I don't know a thing about HID. I hope you'll find it useful nevertheless. > > 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" You are definitely missing dependencies here, CONFIG_HID at least. > + 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 000..eb17d8c > --- /dev/null > +++ b/drivers/i2c/i2c-hid.c > @@ -0,0 +1,1027 @@ > +/* > + * HID over I2C protocol implementation > + * > + * Copyright (c) 2012 Benjamin Tissoires > + * 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 > + * Copyright (c) 2005 Michael Haboustak 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 > +#include > +#include I don't think you need to include that one, everything irq-related you need comes with below. The header comment in that file actually says: "Please do not include this file in generic code." > +#include Your driver makes no use of GPIO. > +#include > +#include > +#include > +#include > +#include You are missing for spin_lock_irq()/spin_unlock_irq(), for device_may_wakeup(), for wait_event_timeout(), for IS_ERR(), for strcat()/memcpy(), for list_for_each_entry() and for msecs_to_jiffies(). I'd suggest including as well, for sprintf() if nothing else, and for WARN_ON(). > + > +#include > + > +#include > +#include > +#include I don't think you using anything from , do you? > + > +#define DRIVER_NAME "i2chid" > +#define DRIVER_DESC "HID over I2C core driver" I see little interest in this define, as you're only using it once. Even DRIVER_NAME isn't so useful, 2 of the 3 occurrences would be replaced with client->name.& > + > +#define I2C_HID_COMMAND_TRIES3 > + > +/* 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) 3 of these are never used, so why define them? > + > +#define I2C_HID_PWR_ON 0x00 > +#define I2C_HID_PWR_SLEEP0x01 > + > +/* debug option */ > +static bool debug = false; checkpatch.pl says: ERROR: do not initialise statics to 0 or NULL #206:
Re: [PATCH v1] i2c-hid: introduce HID over i2c specification implementation
On Sat, 6 Oct 2012 23:27:47 +0200, Stéphane Chatty wrote: > Le 6 oct. 2012 à 23:11, Jean Delvare a écrit : > > On Sat, 6 Oct 2012 22:30:00 +0200, Stéphane Chatty wrote: > >> This is a question I asked a few months back, but apparently not all > >> points of views had been expressed at the time. Currently, HID-over-USB > >> lives in drivers/hid, but HID-over-BT lives in drivers/bluetooth. When I > >> asked, Jiri explained that he maintained HID-over-USB and Marcel > >> maintained HID-over-BT, which explained the choices made. Let's try to > >> summarize what we know now: > >> > >> The question is what drives the choice of where to put HID-over-XXX, among > >> the following > >> 1- who the maintainer is. Here, Benjamin will probably maintain this so it > >> does not help. > >> 2- dependencies. HID-over-XXX depends on HID as much as it depends on XXX, > >> so it does not help. > >> 3- data flow. Indeed, HID is a client of HID-over-XXX which is a client of > >> XXX. Are there other parts of the kernel where this drives the choice of > >> where YYY-over-XXX lives? > >> > >> Jiri, Marcel, Greg, others, any opinions? > > > > My vote is a clear 3. It took me a few years to kick all users (as > > opposed to implementers) of i2c from drivers/i2c and finding them a > > proper home, I'm not going to accept new intruders. Grouping drivers > > according to what they implement makes it a lot easier to share code > > and ideas between related drivers. If you want to convince yourself, > > just imagine the mess it would be if all drivers for PCI devices lived > > under drivers/pci. > > > Having no strong opinion myself, I'm trying to get to the bottom of this :-) > Here, I see two points that need clarification: > > - I'm under the impression that the situation is exactly opposite between i2c > and USB: drivers/usb contains lots of drivers for specific devices, but > HID-over-USB is in drivers/hid. I actually found this disturbing when reading > the HID code for the first time. Mmmm. Indeed I see a lot of drivers under drivers/usb. I'm glad I am not responsible for this subsystem ;) I think drivers/pci is a much cleaner example to follow. If nothing else, grouping drivers by functionality solves the problem of devices which can be accessed through multiple transport layers. For devices with multiple functions, we have drivers/mfd, specifically designed to make it possible to put support for each function into its dedicated location. > - given your explanation, I'd say that you would agree to 2 as well, if it > meant for instance that HID-over-I2C is neither in drivers/hid nor > drivers/i2c. Actually, you don't care whether it is 1, 2, or 3 that drives > the choice as long as HID-over-I2C is not in drivers/i2c, do you? :-) I do care that things are as consistent and logical as possible. I know sometimes there are borderline cases, or things done a certain way for historical reasons, but grouping by functionality seems the more logical and efficient as a rule of thumb. -- Jean Delvare -- 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 v1] i2c-hid: introduce HID over i2c specification implementation
Le 6 oct. 2012 à 23:28, Jiri Kosina a écrit : > On Sat, 6 Oct 2012, Jiri Kosina wrote: > >>> My vote is a clear 3. It took me a few years to kick all users (as >>> opposed to implementers) of i2c from drivers/i2c and finding them a >>> proper home, I'm not going to accept new intruders. Grouping drivers >>> according to what they implement makes it a lot easier to share code >>> and ideas between related drivers. If you want to convince yourself, >>> just imagine the mess it would be if all drivers for PCI devices lived >>> under drivers/pci. >> >> This is more or less consistent with my original opinion when I was >> refactoring the HID layer out of the individual drivers a few years ago. >> >> But Marcel objected that he wants to keep all the bluetooth-related >> drivers under net/bluetooth, and I didn't really want to push hard against >> this, because I don't have really super-strong personal preference either >> way. >> >> But we definitely can use this oportunity to bring this up for discussion >> again. > > Basically, to me this all boils down to the question -- what is more > important: low-level transport being used, or the general function of the > device? > > To me, it's the latter, and as such, everything would belong under > drivers/hid. Then shouldn't is be drivers/input, rather? St. -- 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 v1] i2c-hid: introduce HID over i2c specification implementation
On Sat, 6 Oct 2012, Jiri Kosina wrote: > > My vote is a clear 3. It took me a few years to kick all users (as > > opposed to implementers) of i2c from drivers/i2c and finding them a > > proper home, I'm not going to accept new intruders. Grouping drivers > > according to what they implement makes it a lot easier to share code > > and ideas between related drivers. If you want to convince yourself, > > just imagine the mess it would be if all drivers for PCI devices lived > > under drivers/pci. > > This is more or less consistent with my original opinion when I was > refactoring the HID layer out of the individual drivers a few years ago. > > But Marcel objected that he wants to keep all the bluetooth-related > drivers under net/bluetooth, and I didn't really want to push hard against > this, because I don't have really super-strong personal preference either > way. > > But we definitely can use this oportunity to bring this up for discussion > again. Basically, to me this all boils down to the question -- what is more important: low-level transport being used, or the general function of the device? To me, it's the latter, and as such, everything would belong under drivers/hid. On the other hand, I believe the Marcel will be arguing the bluetooth devices are actually network devices, and he has got a point as well (even though I personally consider bluetooth keyboard to be much more HID device than network device). -- 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 v1] i2c-hid: introduce HID over i2c specification implementation
Le 6 oct. 2012 à 23:11, Jean Delvare a écrit : > On Sat, 6 Oct 2012 22:30:00 +0200, Stéphane Chatty wrote: >> Le 6 oct. 2012 à 22:04, Jean Delvare a écrit : >>> Looks like the wrong place for this driver. HID-over-USB support lives >>> under drivers/hid, so your driver should go there as well. Not only >>> this will be more consistent, but it also makes more sense: your driver >>> is a user, not an implementer, of the I2C layer, so it doesn't belong >>> to drivers/i2c. >> >> This is a question I asked a few months back, but apparently not all points >> of views had been expressed at the time. Currently, HID-over-USB lives in >> drivers/hid, but HID-over-BT lives in drivers/bluetooth. When I asked, Jiri >> explained that he maintained HID-over-USB and Marcel maintained HID-over-BT, >> which explained the choices made. Let's try to summarize what we know now: >> >> The question is what drives the choice of where to put HID-over-XXX, among >> the following >> 1- who the maintainer is. Here, Benjamin will probably maintain this so it >> does not help. >> 2- dependencies. HID-over-XXX depends on HID as much as it depends on XXX, >> so it does not help. >> 3- data flow. Indeed, HID is a client of HID-over-XXX which is a client of >> XXX. Are there other parts of the kernel where this drives the choice of >> where YYY-over-XXX lives? >> >> Jiri, Marcel, Greg, others, any opinions? > > My vote is a clear 3. It took me a few years to kick all users (as > opposed to implementers) of i2c from drivers/i2c and finding them a > proper home, I'm not going to accept new intruders. Grouping drivers > according to what they implement makes it a lot easier to share code > and ideas between related drivers. If you want to convince yourself, > just imagine the mess it would be if all drivers for PCI devices lived > under drivers/pci. Having no strong opinion myself, I'm trying to get to the bottom of this :-) Here, I see two points that need clarification: - I'm under the impression that the situation is exactly opposite between i2c and USB: drivers/usb contains lots of drivers for specific devices, but HID-over-USB is in drivers/hid. I actually found this disturbing when reading the HID code for the first time. Mmmm. - given your explanation, I'd say that you would agree to 2 as well, if it meant for instance that HID-over-I2C is neither in drivers/hid nor drivers/i2c. Actually, you don't care whether it is 1, 2, or 3 that drives the choice as long as HID-over-I2C is not in drivers/i2c, do you? :-) Cheers, St.-- 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 v1] i2c-hid: introduce HID over i2c specification implementation
On Sat, 6 Oct 2012, Jean Delvare wrote: > > The question is what drives the choice of where to put HID-over-XXX, among > > the following > > 1- who the maintainer is. Here, Benjamin will probably maintain this > > so it does not help. > > 2- dependencies. HID-over-XXX depends on HID as much as it depends on > > XXX, so it does not help. > > 3- data flow. Indeed, HID is a client of HID-over-XXX which is a > > client of XXX. Are there other parts of the kernel where this drives > > the choice of where YYY-over-XXX lives? > > > > Jiri, Marcel, Greg, others, any opinions? > > My vote is a clear 3. It took me a few years to kick all users (as > opposed to implementers) of i2c from drivers/i2c and finding them a > proper home, I'm not going to accept new intruders. Grouping drivers > according to what they implement makes it a lot easier to share code > and ideas between related drivers. If you want to convince yourself, > just imagine the mess it would be if all drivers for PCI devices lived > under drivers/pci. This is more or less consistent with my original opinion when I was refactoring the HID layer out of the individual drivers a few years ago. But Marcel objected that he wants to keep all the bluetooth-related drivers under net/bluetooth, and I didn't really want to push hard against this, because I don't have really super-strong personal preference either way. But we definitely can use this oportunity to bring this up for discussion again. -- 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 v1] i2c-hid: introduce HID over i2c specification implementation
On Sat, 6 Oct 2012 22:30:00 +0200, Stéphane Chatty wrote: > Le 6 oct. 2012 à 22:04, Jean Delvare a écrit : > > Looks like the wrong place for this driver. HID-over-USB support lives > > under drivers/hid, so your driver should go there as well. Not only > > this will be more consistent, but it also makes more sense: your driver > > is a user, not an implementer, of the I2C layer, so it doesn't belong > > to drivers/i2c. > > This is a question I asked a few months back, but apparently not all points > of views had been expressed at the time. Currently, HID-over-USB lives in > drivers/hid, but HID-over-BT lives in drivers/bluetooth. When I asked, Jiri > explained that he maintained HID-over-USB and Marcel maintained HID-over-BT, > which explained the choices made. Let's try to summarize what we know now: > > The question is what drives the choice of where to put HID-over-XXX, among > the following > 1- who the maintainer is. Here, Benjamin will probably maintain this so it > does not help. > 2- dependencies. HID-over-XXX depends on HID as much as it depends on XXX, > so it does not help. > 3- data flow. Indeed, HID is a client of HID-over-XXX which is a client of > XXX. Are there other parts of the kernel where this drives the choice of > where YYY-over-XXX lives? > > Jiri, Marcel, Greg, others, any opinions? My vote is a clear 3. It took me a few years to kick all users (as opposed to implementers) of i2c from drivers/i2c and finding them a proper home, I'm not going to accept new intruders. Grouping drivers according to what they implement makes it a lot easier to share code and ideas between related drivers. If you want to convince yourself, just imagine the mess it would be if all drivers for PCI devices lived under drivers/pci. -- Jean Delvare -- 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 v1] i2c-hid: introduce HID over i2c specification implementation
Hi Jean (I cc Marcel Holtmann because BT is involved too) Le 6 oct. 2012 à 22:04, Jean Delvare a écrit : > Hi Benjamin, > > On Fri, 14 Sep 2012 15:41:43 +0200, benjamin.tissoires wrote: >> From: Benjamin Tissoires >> >> 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 >> --- >> >> 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 > > Looks like the wrong place for this driver. HID-over-USB support lives > under drivers/hid, so your driver should go there as well. Not only > this will be more consistent, but it also makes more sense: your driver > is a user, not an implementer, of the I2C layer, so it doesn't belong > to drivers/i2c. This is a question I asked a few months back, but apparently not all points of views had been expressed at the time. Currently, HID-over-USB lives in drivers/hid, but HID-over-BT lives in drivers/bluetooth. When I asked, Jiri explained that he maintained HID-over-USB and Marcel maintained HID-over-BT, which explained the choices made. Let's try to summarize what we know now: The question is what drives the choice of where to put HID-over-XXX, among the following 1- who the maintainer is. Here, Benjamin will probably maintain this so it does not help. 2- dependencies. HID-over-XXX depends on HID as much as it depends on XXX, so it does not help. 3- data flow. Indeed, HID is a client of HID-over-XXX which is a client of XXX. Are there other parts of the kernel where this drives the choice of where YYY-over-XXX lives? Jiri, Marcel, Greg, others, any opinions? > > Also, you need to sort out dependencies. Your causes a link failure here: > > ERROR: "hiddev_report_event" [drivers/i2c/i2c-hid.ko] undefined! > ERROR: "hiddev_disconnect" [drivers/i2c/i2c-hid.ko] undefined! > ERROR: "hiddev_connect" [drivers/i2c/i2c-hid.ko] undefined! > ERROR: "hid_pidff_init" [drivers/i2c/i2c-hid.ko] undefined! > make[1]: *** [__modpost] Erreur 1 > make: *** [modules] Erreur 2 > > This is because these functions aren't exported and I tried to build > i2c-hid as a module.BTW I see that these functions are part of the > usbhid driver, which looks seriously wrong. If these functions are > transport layer-independent, they should be moved to the hid-code or > some sub-module. One should be able to enable HID-over-I2C without > HID-over-USB. > > -- > Jean Delvare Cheers, St. PS: Benjamin is leaving ENAC. He'll probably keep maintaining HID-over-I2C, and we'll probably share the maintenance of hid-multitouch as well as other input-related projects we have not published yet. -- 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 v1] i2c-hid: introduce HID over i2c specification implementation
Hi Benjamin, On Fri, 14 Sep 2012 15:41:43 +0200, benjamin.tissoires wrote: > From: Benjamin Tissoires > > 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 > --- > > 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 Looks like the wrong place for this driver. HID-over-USB support lives under drivers/hid, so your driver should go there as well. Not only this will be more consistent, but it also makes more sense: your driver is a user, not an implementer, of the I2C layer, so it doesn't belong to drivers/i2c. Also, you need to sort out dependencies. Your causes a link failure here: ERROR: "hiddev_report_event" [drivers/i2c/i2c-hid.ko] undefined! ERROR: "hiddev_disconnect" [drivers/i2c/i2c-hid.ko] undefined! ERROR: "hiddev_connect" [drivers/i2c/i2c-hid.ko] undefined! ERROR: "hid_pidff_init" [drivers/i2c/i2c-hid.ko] undefined! make[1]: *** [__modpost] Erreur 1 make: *** [modules] Erreur 2 This is because these functions aren't exported and I tried to build i2c-hid as a module.BTW I see that these functions are part of the usbhid driver, which looks seriously wrong. If these functions are transport layer-independent, they should be moved to the hid-code or some sub-module. One should be able to enable HID-over-I2C without HID-over-USB. -- Jean Delvare -- 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 v1] i2c-hid: introduce HID over i2c specification implementation
On Wed, 3 Oct 2012 09:43:12 -0700, Dmitry Torokhov wrote:
> On Fri, Sep 14, 2012 at 03:41:43PM +0200, benjamin.tissoires wrote:
> > + }
> > +
> > + 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?
dev_dbg() is compiled out if !DEBUG only if CONFIG_DYNAMIC_DEBUG isn't
set. These days I think every developer want to enable this option.
--
Jean Delvare
--
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 v1] i2c-hid: introduce HID over i2c specification implementation
On Thu, 4 Oct 2012, Benjamin Tissoires wrote: > >> + > >> + 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. Well, we are calling ll_driver->stop at multiple places, so having this reset in the actual ll driver callback seems to be cleaner. (if I understand the concern here correctly). -- 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 v1] i2c-hid: introduce HID over i2c specification implementation
Hi Dmitry,
thanks for the review.
On Wed, Oct 3, 2012 at 6:43 PM, Dmitry Torokhov
wrote:
> Hi Benjamin,
>
> A few random comments...
>
> On Fri, Sep 14, 2012 at 03:41:43PM +0200, benjamin.tissoires wrote:
>> From: Benjamin Tissoires
>>
>> 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
>> ---
>>
>> 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 000..eb17d8c
>> --- /dev/null
>> +++ b/drivers/i2c/i2c-hid.c
>> @@ -0,0 +1,1027 @@
>> +/*
>> + * HID over I2C protocol implementation
>> + *
>> + * Copyright (c) 2012 Benjamin Tissoires
>> + * 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
>> + * Copyright (c) 2005 Michael Haboustak 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
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +#include
>> +
>> +#include
>> +#include
>> +#include
>> +
>> +#define DRIVER_NAME "i2chid"
>> +#define DRIVER_DESC "HID over I2C core driver"
>> +
>> +#define I2C_HID_COMMAND_TRIES3
>> +
>> +/* 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_SLEEP0x01
>> +
>> +/* 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 */
>> +
Re: [PATCH v1] i2c-hid: introduce HID over i2c specification implementation
Hi Benjamin,
A few random comments...
On Fri, Sep 14, 2012 at 03:41:43PM +0200, benjamin.tissoires wrote:
> From: Benjamin Tissoires
>
> 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
> ---
>
> 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 000..eb17d8c
> --- /dev/null
> +++ b/drivers/i2c/i2c-hid.c
> @@ -0,0 +1,1027 @@
> +/*
> + * HID over I2C protocol implementation
> + *
> + * Copyright (c) 2012 Benjamin Tissoires
> + * 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
> + * Copyright (c) 2005 Michael Haboustak 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
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +
> +#include
> +#include
> +#include
> +
> +#define DRIVER_NAME "i2chid"
> +#define DRIVER_DESC "HID over I2C core driver"
> +
> +#define I2C_HID_COMMAND_TRIES3
> +
> +/* 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_SLEEP0x01
> +
> +/* 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,
> +
Re: [PATCH v1] i2c-hid: introduce HID over i2c specification implementation
On Wed, Oct 3, 2012 at 9:03 PM, Benjamin Tissoires wrote: > Hi, > > thanks also for the review. Two in the same day! I was about to send a > ping on that patch ;-) > > On Wed, Oct 3, 2012 at 8:05 AM, Shubhrajyoti Datta > wrote: >> On Fri, Sep 14, 2012 at 7:11 PM, benjamin.tissoires >> wrote: >>> From: Benjamin Tissoires >>> >>> 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 >>> --- >>> >>> 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 >> [...] > > same thing, will change it in v2. > >> >>> + >>> + if (debug) >>> + dev_err(&client->dev, "resetting...\n"); >> this is a little uncustomary. >> >> May be consider bdg > > Sorry for that. I don't get your point here. You don't like the whole > "if(debug) dev_err(...)" or just the "dev_err(...)" call? > Apologies for the typo dev_dbg. -- 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 v1] i2c-hid: introduce HID over i2c specification implementation
Hi,
thanks also for the review. Two in the same day! I was about to send a
ping on that patch ;-)
On Wed, Oct 3, 2012 at 8:05 AM, Shubhrajyoti Datta
wrote:
> On Fri, Sep 14, 2012 at 7:11 PM, benjamin.tissoires
> wrote:
>> From: Benjamin Tissoires
>>
>> 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
>> ---
>>
>> 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 000..eb17d8c
>> --- /dev/null
>> +++ b/drivers/i2c/i2c-hid.c
>> @@ -0,0 +1,1027 @@
>> +/*
>> + * HID over I2C protocol implementation
>> + *
>> + * Copyright (c) 2012 Benjamin Tissoires
>> + * 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
>> + * Copyright (c) 2005 Michael Haboustak 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
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +#include
>> +
>> +#include
>> +#include
>> +#include
>> +
>> +#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_
Re: [PATCH v1] i2c-hid: introduce HID over i2c specification implementation
Hi JJ,
On Wed, Oct 3, 2012 at 5:08 AM, Jian-Jhong Ding wrote:
> Hi Benjamin,
>
> I have one little question about __i2chid_command(), please see below.
>
> "benjamin.tissoires" writes:
>> From: Benjamin Tissoires
>>
>> 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
>> ---
>>
[...] snipped
>> + if (wait) {
>> + spin_lock_irq(&ihid->flock);
>> + set_bit(I2C_HID_RESET_PENDING, &ihid->flags);
>> + spin_unlock_irq(&ihid->flock);
>> + }
>> +
>> + 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);
>
> Do we have to do this retry here? In i2c_transfer() it already does
> retry automatically on arbitration loss (please see __i2c_transfer() in
> drivers/i2c/i2c-core.c) that's defined by individual bus driver. Maybe
> we don't have to retry here and make the code a bit simpler.
>
Indeed, this retry is not necessary. I'll remove it in v2.
Thanks for the review.
Cheers,
Benjamin
>> + 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__);
>> + }
>> +
>> + 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);
>> +}
--
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 v1] i2c-hid: introduce HID over i2c specification implementation
On Fri, Sep 14, 2012 at 7:11 PM, benjamin.tissoires
wrote:
> From: Benjamin Tissoires
>
> 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
> ---
>
> 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 000..eb17d8c
> --- /dev/null
> +++ b/drivers/i2c/i2c-hid.c
> @@ -0,0 +1,1027 @@
> +/*
> + * HID over I2C protocol implementation
> + *
> + * Copyright (c) 2012 Benjamin Tissoires
> + * 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
> + * Copyright (c) 2005 Michael Haboustak 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
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +
> +#include
> +#include
> +#include
> +
> +#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,
> +
Re: [PATCH v1] i2c-hid: introduce HID over i2c specification implementation
Hi Benjamin,
I have one little question about __i2chid_command(), please see below.
"benjamin.tissoires" writes:
> From: Benjamin Tissoires
>
> 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
> ---
>
> 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 000..eb17d8c
> --- /dev/null
> +++ b/drivers/i2c/i2c-hid.c
> @@ -0,0 +1,1027 @@
> +/*
> + * HID over I2C protocol implementation
> + *
> + * Copyright (c) 2012 Benjamin Tissoires
> + * 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
> + * Copyright (c) 2005 Michael Haboustak 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
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +
> +#include
> +#include
> +#include
> +
> +#define DRIVER_NAME "i2chid"
> +#define DRIVER_DESC "HID over I2C core driver"
> +
> +#define I2C_HID_COMMAND_TRIES3
> +
> +/* 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_SLEEP0x01
> +
> +/* 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,
> +
[PATCH v1] i2c-hid: introduce HID over i2c specification implementation
From: Benjamin Tissoires
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
---
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 000..eb17d8c
--- /dev/null
+++ b/drivers/i2c/i2c-hid.c
@@ -0,0 +1,1027 @@
+/*
+ * HID over I2C protocol implementation
+ *
+ * Copyright (c) 2012 Benjamin Tissoires
+ * 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
+ * Copyright (c) 2005 Michael Haboustak 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
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+
+#include
+#include
+#include
+
+#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 o
