Re: [PATCH v1] i2c-hid: introduce HID over i2c specification implementation

[Stéphane Chatty]

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 <benjamin.tissoires@xxxxxxx>
>> 
>> Microsoft published the protocol specification of HID over i2c:
>> http://msdn.microsoft.com/en-us/library/windows/hardware/hh852380.aspx
>> 
>> This patch introduces an implementation of this protocol.
>> 
>> This implementation does not includes the ACPI part of the specification.
>> This will come when ACPI 5.0 devices will be available.
>> 
>> Once the ACPI part will be done, OEM will not have to declare HID over I2C
>> devices in their platform specific driver.
>> 
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@xxxxxxx>
>> ---
>> 
>> Hi,
>> 
>> this is finally my first implementation of HID over I2C.
>> 
>> This has been tested on an Elan Microelectronics HID over I2C device, with
>> a Samsung Exynos 4412 board.
>> 
>> Any comments are welcome.
>> 
>> Cheers,
>> Benjamin
>> 
>> drivers/i2c/Kconfig         |    8 +
>> drivers/i2c/Makefile        |    1 +
>> drivers/i2c/i2c-hid.c       | 1027 +++++++++++++++++++++++++++++++++++++++++++
>> include/linux/i2c/i2c-hid.h |   35 ++
>> 4 files changed, 1071 insertions(+)
>> create mode 100644 drivers/i2c/i2c-hid.c
>> create mode 100644 include/linux/i2c/i2c-hid.h
> 
> 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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/