Hi Benjamin,
On Fri, 14 Sep 2012 15:41:43 +0200, benjamin.tissoires wrote:
> From: Benjamin Tissoires <[email protected]>
>
> 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 <[email protected]>
> ---
>
> 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
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 <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>>
>> 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.
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
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
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.-
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
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.
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
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 <[email protected]> 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
On Sat, Oct 6, 2012 at 11:39 PM, St?phane Chatty <[email protected]> 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.
>
>
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.
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