Re: Regression in hid-multitouch?
Hi Cedric, I'm a little bit lost there. You still have the same problems than the one described in the thread in xorg-devel? I don't get the though the touchpoints are correctly recognized... Can you send us your dmesg output and the trace from evtest? I also would like to know which kernel version is working and which is not. Thanks, Benjamin On Sun, Jul 8, 2012 at 12:48 AM, Cedric Sodhi man...@gmx.net wrote: Sorry, this is not a regression. I just reverted to said commit and I still have those problems (described in detail, here: http://comments.gmane.org/gmane.comp.freedesktop.xorg.devel/31159 - though the touchpoints are correctly recognized). On Sat, Jul 07, 2012 at 11:55:41PM +0200, Cedric Sodhi wrote: What previously seems to have been fixed by http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=3ac36d15557d1bedfb1151d9911b9587b2d40759 has again stopped working in 3.5.0-rc5 - at least I get the exact same problems I used to have before 3ac36d... although the code from 3ac36d is still there. regards, Cedric -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Problems with a Zytronic touchscreen on kernel 3.5 (USB ID 0x14c8:0x0005) - regression from kernel 3.0
Hi Simon, well, it's a known bug. This panel is now fixed in 3.6-rc0, and I submitted the fix for stable. I'm still waiting for the stable approval. BTW, Zytronic 0005 is a multitouch device and not only a single touch one. So I think we should handle it through hid-multitouch. I know that Zytronic develops its own driver in parallel to fix some problems they may have with the multitouch layer. This will also allow them to flash their firmware. Cheers, Benjamin PS: the fix is at http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commit;h=4aceed37e315e8eaa26cb4c8dfd619a32fa24669 On Wed, Aug 1, 2012 at 10:20 AM, Simon Farnsworth simon.farnswo...@onelan.co.uk wrote: Hello, Greg K-H pointed me this way from linux-usb; I've had no replies there, but if you've already seen the message on linux-usb, nothing's changed in the text below or the attachments. I'm trying to get a Zytronic single-touch touchscreen to work with Linux kernel 3.5; it previously worked on a 3.0 kernel. I'm happy to try git kernels and patches, even if they're just intended to gather more information. hid-multitouch is picking up the device because it has multitouch report descriptors as well as the single-touch descriptors. There's a quirk to make it use MT_CLS_SERIAL, but that seems to be insufficient for my needs. I've got as far as determining that it's mt_event in hid-multitouch.c that consumes the events, but as the screen never sends the events hid-multitouch.c expects to see, mt_event nevers sends an input event. evtest on 3.0 shows touch events as expected. evtest on 3.5 shows no touch events. I've attached /sys/kernel/debug/hid/0003:14C8:0005.0001/rdesc as zyntronic.rdesc and /sys/kernel/debug/hid/0003:14C8:0005.0001/events while I touch the screen as zytronic.events. I had evtest /dev/input/event5 running while I captured zytronic.events, and got the following output: Input driver version is 1.0.1 Input device ID: bus 0x3 vendor 0x14c8 product 0x5 version 0x101 Input device name: Zytronic Displays Limited Zytronic Touchscreen Controller Supported events: Event type 0 (EV_SYN) Event type 1 (EV_KEY) Event code 330 (BTN_TOUCH) Event type 3 (EV_ABS) Event code 0 (ABS_X) Value 0 Min0 Max 4096 Event code 1 (ABS_Y) Value 0 Min0 Max 4096 Event code 47 (ABS_MT_SLOT) Value 0 Min0 Max9 Event code 53 (ABS_MT_POSITION_X) Value 0 Min0 Max 4096 Event code 54 (ABS_MT_POSITION_Y) Value 0 Min0 Max 4096 Event code 57 (ABS_MT_TRACKING_ID) Value 0 Min0 Max65535 Testing ... (interrupt to exit) ^C Any ideas, or instructions for gathering more debug information will be gratefully received. -- Simon Farnsworth Software Engineer ONELAN Ltd http://www.onelan.com -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch v2 1/1] HID:hid-multitouch: Add ELAN prouction request when resume
Hi Scott, we are getting closer. Just a few nitpicks: On Thu, Aug 9, 2012 at 11:22 AM, Scott Liu scott@emc.com.tw wrote: Some of ELAN's production need to with set_idle commmand when reusme. reusme - resume Signed-off-by: Scott Liu scott@emc.com.tw --- drivers/hid/hid-ids.h|3 +++ drivers/hid/hid-multitouch.c | 20 2 files changed, 23 insertions(+) diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h index 41c34f2..a4d810c 100644 --- a/drivers/hid/hid-ids.h +++ b/drivers/hid/hid-ids.h @@ -829,4 +829,7 @@ #define USB_VENDOR_ID_PRIMAX 0x0461 #define USB_DEVICE_ID_PRIMAX_KEYBOARD 0x4e05 +#define USB_VENDOR_ID_ELAN 0x04f3 +#define USB_DEVICE_ID_ELAN_MOCCA0x000a + We try to keep the list alphabetically sorted. So it should go just before USB_VENDOR_ID_ELECOM. Also, the device you sent to me has the PID 0x0732 and is behaving the same way (need to call the mt_reset command). Is 0x000a a mistake or do all of your product behave like this? If it's the latter, then we may purely remove the PID test. Jiri, any ideas? #endif diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index 59c8b5c..b06b7d3 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -767,6 +767,25 @@ static int mt_reset_resume(struct hid_device *hdev) mt_set_input_mode(hdev); return 0; } + +static int mt_resume(struct hid_device *hdev) +{ + if (hdev-vendor == USB_VENDOR_ID_ELAN + hdev-product == USB_DEVICE_ID_ELAN_MOCCA) { + + struct usb_interface *intf = to_usb_interface(hdev-dev.parent); + struct usb_host_interface *interface = intf-cur_altsetting; + struct usb_device *dev = hid_to_usb_dev(hdev); + + usb_control_msg(dev, usb_sndctrlpipe(dev, 0), + HID_REQ_SET_IDLE, USB_TYPE_CLASS | USB_RECIP_INTERFACE, As we are on it, this line is more than 80 characters. Tabs are 8 characters length. + 0, interface-desc.bInterfaceNumber, + NULL, 0, USB_CTRL_SET_TIMEOUT); +} spaces instead of tabs. + +return 0; idem, spaces instead of tabs. Anyway, I really appreciate your efforts Scott. Cheers, Benjamin +} + #endif static void mt_remove(struct hid_device *hdev) @@ -1092,6 +,7 @@ static struct hid_driver mt_driver = { .event = mt_event, #ifdef CONFIG_PM .reset_resume = mt_reset_resume, + .resume = mt_resume, #endif }; -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] HID: multitouch: Remove the redundant touch state
Hi Jiri, On Wed, Aug 29, 2012 at 12:25 AM, Jiri Kosina jkos...@suse.cz wrote: On Wed, 22 Aug 2012, Henrik Rydberg wrote: With the input_mt_sync_frame() function in place, there is no longer any need to keep the full touch state in the driver. This patch removes the slot state and replaces the lookup code with the input-mt equivalent. The initialization code is moved to mt_input_configured(), to make sure the full HID report has been seen. Signed-off-by: Henrik Rydberg rydb...@euromail.se --- Hi Benjamin, Maybe this patch works better? It has received limited testing so far. What is the status of this patch please? Henrik, Benjamin? Well, Henrik submitted a new release a few days ago (including this version). I just didn't found the time to test the whole thing on our different devices. It's now on the top of my TODO list. Cheers, Benjamin Henrik drivers/hid/hid-multitouch.c | 133 +++ 1 file changed, 46 insertions(+), 87 deletions(-) diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index c400d90..dc08a4e 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -56,7 +56,6 @@ struct mt_slot { __s32 x, y, p, w, h; __s32 contactid;/* the device ContactID assigned to this slot */ bool touch_state; /* is the touch valid? */ - bool seen_in_this_frame;/* has this slot been updated */ }; struct mt_class { @@ -93,7 +92,7 @@ struct mt_device { * 1 means we should use a serial protocol * 1 means hybrid (multitouch) protocol */ bool curvalid; /* is the current contact valid? */ - struct mt_slot *slots; + unsigned mt_flags; /* flags to pass to input-mt */ }; /* classes of device behavior */ @@ -134,25 +133,6 @@ static int cypress_compute_slot(struct mt_device *td) return -1; } -static int find_slot_from_contactid(struct mt_device *td) -{ - int i; - for (i = 0; i td-maxcontacts; ++i) { - if (td-slots[i].contactid == td-curdata.contactid - td-slots[i].touch_state) - return i; - } - for (i = 0; i td-maxcontacts; ++i) { - if (!td-slots[i].seen_in_this_frame - !td-slots[i].touch_state) - return i; - } - /* should not occurs. If this happens that means - * that the device sent more touches that it says - * in the report descriptor. It is ignored then. */ - return -1; -} - static struct mt_class mt_classes[] = { { .name = MT_CLS_DEFAULT, .quirks = MT_QUIRK_NOT_SEEN_MEANS_UP }, @@ -319,24 +299,16 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi, * We need to ignore fields that belong to other collections * such as Mouse that might have the same GenericDesktop usages. */ if (field-application == HID_DG_TOUCHSCREEN) - set_bit(INPUT_PROP_DIRECT, hi-input-propbit); + td-mt_flags |= INPUT_MT_DIRECT; else if (field-application != HID_DG_TOUCHPAD) return 0; - /* In case of an indirect device (touchpad), we need to add - * specific BTN_TOOL_* to be handled by the synaptics xorg - * driver. - * We also consider that touchscreens providing buttons are touchpads. + /* + * Model touchscreens providing buttons as touchpads. */ if (field-application == HID_DG_TOUCHPAD || - (usage-hid HID_USAGE_PAGE) == HID_UP_BUTTON || - cls-is_indirect) { - set_bit(INPUT_PROP_POINTER, hi-input-propbit); - set_bit(BTN_TOOL_FINGER, hi-input-keybit); - set_bit(BTN_TOOL_DOUBLETAP, hi-input-keybit); - set_bit(BTN_TOOL_TRIPLETAP, hi-input-keybit); - set_bit(BTN_TOOL_QUADTAP, hi-input-keybit); - } + (usage-hid HID_USAGE_PAGE) == HID_UP_BUTTON) + td-mt_flags |= INPUT_MT_POINTER; /* eGalax devices provide a Digitizer.Stylus input which overrides * the correct Digitizers.Finger X/Y ranges. @@ -353,8 +325,6 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi, EV_ABS, ABS_MT_POSITION_X); set_abs(hi-input, ABS_MT_POSITION_X, field, cls-sn_move); - /* touchscreen emulation */ - set_abs(hi-input, ABS_X, field, cls-sn_move); mt_store_field(usage, td, hi); td-last_field_index = field-index; return 1; @@ -363,8 +333,6 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi, EV_ABS, ABS_MT_POSITION_Y); set_abs(hi-input,
Re: [PATCH v2 20/20] HID: multitouch: Remove the redundant touch state
Hi Henrik, Thanks for the new version. I still didn't review in depth the patch (it seemed to be fair). However, I found out 2 bugs while testing it (related to the whole series apparently): - I don't know if it is my configuration or not, but I'm receiving at each frame (between two EV_SYN) all ABS_MT_SLOT events, even when there are not used. - After a little bit a playing with some surfaces, I can confuse the input_mt_assign_slot_by_id function: for the very same contact id, I got up to four different slots This accumulates the clicks and is not very good :-( I'll try now to spot the bug. Cheers, Benjamin On Sun, Aug 26, 2012 at 2:57 PM, Henrik Rydberg rydb...@euromail.se wrote: With the input_mt_sync_frame() function in place, there is no longer any need to keep the full touch state in the driver. This patch removes the slot state and replaces the lookup code with the input-mt equivalent. The initialization code is moved to mt_input_configured(), to make sure the full HID report has been seen. Cc: Benjamin Tissoires benjamin.tissoi...@enac.fr Signed-off-by: Henrik Rydberg rydb...@euromail.se --- drivers/hid/hid-multitouch.c | 133 +++ 1 file changed, 46 insertions(+), 87 deletions(-) diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index c400d90..dc08a4e 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -56,7 +56,6 @@ struct mt_slot { __s32 x, y, p, w, h; __s32 contactid;/* the device ContactID assigned to this slot */ bool touch_state; /* is the touch valid? */ - bool seen_in_this_frame;/* has this slot been updated */ }; struct mt_class { @@ -93,7 +92,7 @@ struct mt_device { * 1 means we should use a serial protocol * 1 means hybrid (multitouch) protocol */ bool curvalid; /* is the current contact valid? */ - struct mt_slot *slots; + unsigned mt_flags; /* flags to pass to input-mt */ }; /* classes of device behavior */ @@ -134,25 +133,6 @@ static int cypress_compute_slot(struct mt_device *td) return -1; } -static int find_slot_from_contactid(struct mt_device *td) -{ - int i; - for (i = 0; i td-maxcontacts; ++i) { - if (td-slots[i].contactid == td-curdata.contactid - td-slots[i].touch_state) - return i; - } - for (i = 0; i td-maxcontacts; ++i) { - if (!td-slots[i].seen_in_this_frame - !td-slots[i].touch_state) - return i; - } - /* should not occurs. If this happens that means -* that the device sent more touches that it says -* in the report descriptor. It is ignored then. */ - return -1; -} - static struct mt_class mt_classes[] = { { .name = MT_CLS_DEFAULT, .quirks = MT_QUIRK_NOT_SEEN_MEANS_UP }, @@ -319,24 +299,16 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi, * We need to ignore fields that belong to other collections * such as Mouse that might have the same GenericDesktop usages. */ if (field-application == HID_DG_TOUCHSCREEN) - set_bit(INPUT_PROP_DIRECT, hi-input-propbit); + td-mt_flags |= INPUT_MT_DIRECT; else if (field-application != HID_DG_TOUCHPAD) return 0; - /* In case of an indirect device (touchpad), we need to add -* specific BTN_TOOL_* to be handled by the synaptics xorg -* driver. -* We also consider that touchscreens providing buttons are touchpads. + /* +* Model touchscreens providing buttons as touchpads. */ if (field-application == HID_DG_TOUCHPAD || - (usage-hid HID_USAGE_PAGE) == HID_UP_BUTTON || - cls-is_indirect) { - set_bit(INPUT_PROP_POINTER, hi-input-propbit); - set_bit(BTN_TOOL_FINGER, hi-input-keybit); - set_bit(BTN_TOOL_DOUBLETAP, hi-input-keybit); - set_bit(BTN_TOOL_TRIPLETAP, hi-input-keybit); - set_bit(BTN_TOOL_QUADTAP, hi-input-keybit); - } + (usage-hid HID_USAGE_PAGE) == HID_UP_BUTTON) + td-mt_flags |= INPUT_MT_POINTER; /* eGalax devices provide a Digitizer.Stylus input which overrides * the correct Digitizers.Finger X/Y ranges. @@ -353,8 +325,6 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi, EV_ABS, ABS_MT_POSITION_X); set_abs(hi-input, ABS_MT_POSITION_X, field, cls-sn_move); - /* touchscreen emulation */ - set_abs(hi-input
Re: [PATCH 20/20] HID: hid-multitouch: Add missing contact count detection
On Sat, Sep 1, 2012 at 9:47 PM, Henrik Rydberg rydb...@euromail.se wrote: Some devices report the number of contacts via the CONTACTCOUNT usage, rather than using the CONTACTMAX feature. Without this patch, such devices will be capped to ten fingers, the default maximum. Fixes a long-standing regression on 3M and similar panels. Cc: Benjamin Tissoires benjamin.tissoi...@enac.fr Signed-off-by: Henrik Rydberg rydb...@euromail.se --- Since this patch looks like it could be backported as-is: In 3.6, maxcontacts may be modified after the slots have been initialized, leaving the patch harmless but ineffective. drivers/hid/hid-multitouch.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index eee19c9..2fc5335 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -388,6 +388,8 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi, td-last_field_index = field-index; return 1; case HID_DG_CONTACTCOUNT: + if (td-maxcontacts field-logical_maximum + 1) + td-maxcontacts = field-logical_maximum + 1; Hi Henrik, Unfortunately, this is not working. We have bad devices that present either: - a null logical_maximum (some Stantum do) - the value 1 is assigned to maxcontacts, and the device is turned into a mono touch - an obviously wrong value (with respect to the current state of the devices) - Quanta devices report 255 there and they support only 2 touches And of course, 3M decided to stop sending 60 touches on their recent devices: we have here a 0x506 device that present a contact_count of max 60, a maximum contact feature of max 60, but whatever I'm doing, it does not want to send more than 10 touches Cheers, Benjamin td-last_field_index = field-index; return 1; case HID_DG_CONTACTMAX: -- 1.7.12 -- To unsubscribe from this list: send the line unsubscribe linux-input 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 dmitry.torok...@gmail.com wrote: Hi Benjamin, A few random comments... On Fri, Sep 14, 2012 at 03:41:43PM +0200, benjamin.tissoires wrote: From: Benjamin Tissoires benjamin.tissoi...@enac.fr 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.tissoi...@enac.fr --- 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 benjamin.tissoi...@gmail.com + * 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 vojt...@suse.cz + * Copyright (c) 2005 Michael Haboustak mi...@cinci.rr.com for Concept2, Inc + * Copyright (c) 2007-2008 Oliver Neukum + * Copyright (c) 2006-2010 Jiri Kosina + * + * This file is subject to the terms and conditions of the GNU General Public + * License. See the file COPYING in the main directory of this archive for + * more details. + */ + +#include linux/module.h +#include linux/i2c.h +#include linux/irq.h +#include linux/gpio.h +#include linux/interrupt.h +#include linux/input.h +#include linux/delay.h +#include linux/slab.h +#include linux/pm.h + +#include linux/i2c/i2c-hid.h + +#include linux/hid.h +#include linux/hiddev.h +#include linux/hidraw.h + +#define DRIVER_NAME i2chid +#define DRIVER_DESC HID over I2C core driver + +#define I2C_HID_COMMAND_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
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 kh...@linux-fr.org 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-input 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 cha...@enac.fr 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-input 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 kh...@linux-fr.org wrote: Salut Benjamin, On Fri, 14 Sep 2012 15:41:43 +0200, benjamin.tissoires wrote: From: Benjamin Tissoires benjamin.tissoi...@enac.fr 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.tissoi...@enac.fr --- 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 benjamin.tissoi...@gmail.com + * 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 vojt...@suse.cz + * Copyright (c) 2005 Michael Haboustak mi...@cinci.rr.com for Concept2, Inc + * Copyright (c) 2007-2008 Oliver Neukum + * Copyright (c) 2006-2010 Jiri Kosina + * + * This file is subject to the terms and conditions of the GNU General Public + * License. See the file COPYING in the main directory of this archive for + * more details. + */ + +#include linux/module.h +#include linux/i2c.h +#include linux/irq.h I don't think you need to include that one, everything irq-related you need comes with linux/interrupt.h below. The header comment in that file actually says: Please do not include this file in generic code. +#include linux/gpio.h Your driver makes no use of GPIO. +#include linux/interrupt.h +#include linux/input.h +#include linux/delay.h +#include linux/slab.h +#include linux/pm.h You are missing linux/spinlock.h for spin_lock_irq()/spin_unlock_irq(), linux/device.h for device_may_wakeup(), linux/wait.h for wait_event_timeout(), linux/err.h for IS_ERR(), linux/string.h for strcat()/memcpy(), linux/list.h for list_for_each_entry() and linux/jiffies.h for msecs_to_jiffies(). I'd suggest including linux/kernel.h as well, for sprintf() if nothing else, and linux/bug.h for WARN_ON(). It seems like I've been to lazy with the includes... Thanks! + +#include linux/i2c/i2c-hid.h + +#include linux/hid.h +#include linux/hiddev.h +#include linux/hidraw.h I don't think you using anything from linux/hidraw.h, 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
[PATCH v2] i2c-hid: introduce HID over i2c specification implementation
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 is done, OEM will not have to declare HID over I2C devices in their platform specific driver. Signed-off-by: Benjamin Tissoires benjamin.tissoi...@gmail.com --- Hi Guys, here is the v2 of the HID over I2C driver. Changes since v1: * s/i2chid/i2c_hid/ * remove i2c_hid_print_buffer function and use %*ph instead * define i2c_hid_dbg macro * remove retries in __i2c_hid_command * fix return values * fix imports (I hope) * drop hiddev and pidff_init support as the used functions were usb only * make memory allocation more static {1} * remove spinlock {2} * don't use an array for the commands, but static const declarations instead * move i2c-hid into drivers/hid {3} * remove HID_MAX_BUFFER_SIZE test {4} * implement missing output reports sending (not tested) * typos, spacing, and many small thinks that appeared in the previous review Notes: {1}: I don't have all the informations in the beginning of the probe function to get the real size I need to allocate. So the behavior is to allocate first a buffer by using HID_MIN_BUFFER_SIZE and once I got the information, I can reallocate the buffer to the right size (in i2c_hid_start). {2}: This version presents no more spinlock and dynamic allocation, but there may be sources of races through two means: - irqs: if an event occurs while we are in __i2c_hid_command - this should not be a problem as there is no shared ressource for input reports. - hidraw: if several clients write commands in /dev/hidrawX - this is more problematic unless hidraw already locks the driver. {3}: I didn't moved the .h (in include/linux/i2c) because it will be used by mach-xxx and I think it's better to leave it there. But again, I may be wrong and we can move it elsewhere. {4}: This check introduced more bugs than adding security. I currently don't think it will be problematic unless we found out a very buggy hid device. Cheers, Benjamin drivers/hid/Kconfig | 2 + drivers/hid/Makefile | 1 + drivers/hid/i2c-hid/Kconfig | 21 + drivers/hid/i2c-hid/Makefile | 5 + drivers/hid/i2c-hid/i2c-hid.c | 990 ++ include/linux/i2c/i2c-hid.h | 35 ++ 6 files changed, 1054 insertions(+) create mode 100644 drivers/hid/i2c-hid/Kconfig create mode 100644 drivers/hid/i2c-hid/Makefile create mode 100644 drivers/hid/i2c-hid/i2c-hid.c create mode 100644 include/linux/i2c/i2c-hid.h diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig index fbf4950..4bc0995 100644 --- a/drivers/hid/Kconfig +++ b/drivers/hid/Kconfig @@ -696,4 +696,6 @@ endif # HID source drivers/hid/usbhid/Kconfig +source drivers/hid/i2c-hid/Kconfig + endmenu diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile index f975485..bb1e4536 100644 --- a/drivers/hid/Makefile +++ b/drivers/hid/Makefile @@ -96,3 +96,4 @@ obj-$(CONFIG_USB_HID) += usbhid/ obj-$(CONFIG_USB_MOUSE)+= usbhid/ obj-$(CONFIG_USB_KBD) += usbhid/ +obj-$(CONFIG_I2C_HID) += i2c-hid/ diff --git a/drivers/hid/i2c-hid/Kconfig b/drivers/hid/i2c-hid/Kconfig new file mode 100644 index 000..5b7d4d8 --- /dev/null +++ b/drivers/hid/i2c-hid/Kconfig @@ -0,0 +1,21 @@ +menu I2C HID support + depends on I2C + +config I2C_HID + tristate HID over I2C transport layer + default n + depends on I2C INPUT + select HID + ---help--- + Say Y here if you want to use the HID over i2c protocol + implementation. + + If unsure, say N. + + This support is also available as a module. If so, the module + will be called i2c-hid. + +comment Input core support is needed for HID over I2C input layer + depends on I2C_HID INPUT=n + +endmenu diff --git a/drivers/hid/i2c-hid/Makefile b/drivers/hid/i2c-hid/Makefile new file mode 100644 index 000..832d8f9 --- /dev/null +++ b/drivers/hid/i2c-hid/Makefile @@ -0,0 +1,5 @@ +# +# Makefile for the I2C input drivers +# + +obj-$(CONFIG_I2C_HID) += i2c-hid.o diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c new file mode 100644 index 000..8b6c31a --- /dev/null +++ b/drivers/hid/i2c-hid/i2c-hid.c @@ -0,0 +1,990 @@ +/* + * HID over I2C protocol implementation + * + * Copyright (c) 2012 Benjamin Tissoires benjamin.tissoi...@gmail.com + * 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 vojt...@suse.cz + * Copyright (c) 2005 Michael Haboustak mi...@cinci.rr.com for Concept2, Inc + * Copyright (c) 2007
Re: [PATCH v1] i2c-hid: introduce HID over i2c specification implementation
On Sun, Oct 7, 2012 at 9:03 PM, Jean Delvare kh...@linux-fr.org wrote: On Sun, 7 Oct 2012 18:57:36 +0200, Benjamin Tissoires wrote: On Sun, Oct 7, 2012 at 4:28 PM, Jean Delvare kh...@linux-fr.org 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-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] i2c-hid: introduce HID over i2c specification implementation
Hi JJ, On Thu, Oct 18, 2012 at 11:07 AM, Jian-Jhong Ding jj_d...@emc.com.tw wrote: Hi Benjamin, Some suggestions to make the error messages more human, and a little question on the return value of i2c_hid_fetch_hid_descriptor. Please see below: I fully agree with the more human error messages. However, for i2c_hid_fetch_hid_descriptor return values, I'm affraid I can't use -EINVAL. Jean Delvare (one of the i2c maintainers) told in his review of the v1: These should all be -ENODEV in this function [i2c_hid_fetch_hid_descriptor]: the device isn't what you expected. EINVAL is for invalid argument. So ENODEV is the right return value. Anyway, thanks for the review. Cheers, Benjamin Benjamin Tissoires benjamin.tissoi...@gmail.com writes: diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c new file mode 100644 index 000..8b6c31a --- /dev/null +++ b/drivers/hid/i2c-hid/i2c-hid.c @@ -0,0 +1,990 @@ +/* + * HID over I2C protocol implementation + * + * Copyright (c) 2012 Benjamin Tissoires benjamin.tissoi...@gmail.com + * 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 vojt...@suse.cz + * Copyright (c) 2005 Michael Haboustak mi...@cinci.rr.com for Concept2, Inc + * Copyright (c) 2007-2008 Oliver Neukum + * Copyright (c) 2006-2010 Jiri Kosina + * + * This file is subject to the terms and conditions of the GNU General Public + * License. See the file COPYING in the main directory of this archive for + * more details. + */ + +#include linux/module.h +#include linux/i2c.h +#include linux/interrupt.h +#include linux/input.h +#include linux/delay.h +#include linux/slab.h +#include linux/pm.h +#include linux/device.h +#include linux/wait.h +#include linux/err.h +#include linux/string.h +#include linux/list.h +#include linux/jiffies.h +#include linux/kernel.h +#include linux/bug.h +#include linux/hid.h + +#include linux/i2c/i2c-hid.h + +/* flags */ +#define I2C_HID_STARTED (1 0) +#define I2C_HID_RESET_PENDING(1 1) +#define I2C_HID_READ_PENDING (1 2) + +#define I2C_HID_PWR_ON 0x00 +#define I2C_HID_PWR_SLEEP0x01 + +/* debug option */ +static bool debug = false; +module_param(debug, bool, 0444); +MODULE_PARM_DESC(debug, print a lot of debug information); + +#define i2c_hid_dbg(ihid, fmt, arg...) \ + if (debug) \ + dev_printk(KERN_DEBUG, (ihid)-client-dev, fmt, ##arg) + +struct i2c_hid_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 i2c_hid_cmd { + unsigned int registerIndex; + __u8 opcode; + unsigned int length; + bool wait; +}; + +union command { + u8 data[0]; + struct cmd { + __le16 reg; + __u8 reportTypeID; + __u8 opcode; + } __packed c; +}; + +#define I2C_HID_CMD(opcode_) \ + .opcode = opcode_, .length = 4, \ + .registerIndex = offsetof(struct i2c_hid_desc, wCommandRegister) + +/* fetch HID descriptor */ +static const struct i2c_hid_cmd hid_descr_cmd = { .length = 2 }; +/* fetch report descriptors */ +static const struct i2c_hid_cmd hid_report_descr_cmd = { + .registerIndex = offsetof(struct i2c_hid_desc, + wReportDescRegister), + .opcode = 0x00, + .length = 2 }; +/* commands */ +static const struct i2c_hid_cmd hid_reset_cmd = { I2C_HID_CMD(0x01), + .wait = true }; +static const struct i2c_hid_cmd hid_get_report_cmd = { I2C_HID_CMD(0x02) }; +static const struct i2c_hid_cmd hid_set_report_cmd = { I2C_HID_CMD(0x03) }; +static const struct i2c_hid_cmd hid_get_idle_cmd = { I2C_HID_CMD(0x04) }; +static const struct i2c_hid_cmd hid_set_idle_cmd = { I2C_HID_CMD(0x05) }; +static const struct i2c_hid_cmd hid_get_protocol_cmd = { I2C_HID_CMD(0x06) }; +static const struct i2c_hid_cmd hid_set_protocol_cmd = { I2C_HID_CMD(0x07) }; +static const struct i2c_hid_cmd hid_set_power_cmd = { I2C_HID_CMD(0x08) }; +/* read/write data register */ +static const struct i2c_hid_cmd hid_data_cmd = { + .registerIndex = offsetof(struct i2c_hid_desc, wDataRegister), + .opcode = 0x00, + .length = 2 }; +/* write output reports */ +static const struct i2c_hid_cmd hid_out_cmd = { + .registerIndex = offsetof(struct i2c_hid_desc
Re: [PATCH v1] Support Elan Touchscreen eKTF product.
On Mon, Oct 22, 2012 at 6:07 PM, Dmitry Torokhov dmitry.torok...@gmail.com wrote: On Mon, Oct 22, 2012 at 11:47:42AM +0800, Jian-Jhong Ding wrote: Scott Liu scott@emc.com.tw writes: + +struct mt_device { + struct mt_slot curdata; /* placeholder of incoming data */ + __u8 num_received; /* how many contacts we received */ + __u8 num_expected; /* expected last contact index */ + __u8 maxcontacts; + bool curvalid; /* is the current contact valid? */ + struct mt_slot *slots; +}; With Benjamin's i2c-hid implimentation, is it possible to make hid-multitouch not depend on USBHID and reuse it to drive this device? We can already use hid-multitouch with hid over I2C devices (I'm testing my i2c devices with this module). But it's true that hid-multitouch depends on usbhid, and I the funny think is that I was removing this dependency today. The fact is that currently, i2c devices do not segfault with hid-multitouch because win8 devices do not require anymore to set some feature at plug. Exactly. Before looking any further - is this the same part that Tom Lin posted a driver for earlier this summer? I'm not Elan, and I can not be sure, but judging from the hello packets and the other commands, I doubt this device is an I2C over HID one. Anyway, reusing hid-multitouch for this specific case seams to be a little bit difficult. You would have first to provide a fake report descriptor (or inject commands as if you were hid-core) and then, you would have to reformat the incoming data into valid win7 (or 8) packets. Judging by the functions elan_touch_parse_fid, elan_touch_parse_wid and elants_parse_xy, the data seem to be mixed (all the finger ids at the beginning, and not one per touch), so it is definitively needed to recreate a valid HID packet. So I'm not surprised with the duplicated code. However, Henrik did a big job in kernel 3.7 to factorize mt code, and some part of the duplication can be achieve with these functions (look at input_mt_sync_frame and input_mt_get_slot_by_key for example). Cheers, Benjamin Thanks. -- Dmitry -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] hid: put the case in the right switch statement
Hi Alan, Yes, I saw that too. Acked-by: Benjamin Tissoires benjamin.tissoi...@gmail.com On Thu, Oct 25, 2012 at 4:35 PM, Alan Cox a...@lxorguk.ukuu.org.uk wrote: From: Alan Cox a...@linux.intel.com Signed-off-by: Alan Cox a...@linux.intel.com --- drivers/hid/hid-multitouch.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index 3eb02b9..c97011c 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -421,11 +421,11 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi, * contact max are global to the report */ td-last_field_index = field-index; return -1; - } case HID_DG_TOUCH: /* Legacy devices use TIPSWITCH and not TOUCH. * Let's just ignore this field. */ return -1; + } /* let hid-input decide for the others */ return 0; -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] hid: put the case in the right switch statement
Adding Jiri to the recipient list of the patch, otherwise, the thread may fall in the depth of his mailbox :) Cheers, Benjamin On Thu, Oct 25, 2012 at 7:08 PM, Benjamin Tissoires benjamin.tissoi...@gmail.com wrote: Hi Alan, Yes, I saw that too. Acked-by: Benjamin Tissoires benjamin.tissoi...@gmail.com On Thu, Oct 25, 2012 at 4:35 PM, Alan Cox a...@lxorguk.ukuu.org.uk wrote: From: Alan Cox a...@linux.intel.com Signed-off-by: Alan Cox a...@linux.intel.com --- drivers/hid/hid-multitouch.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index 3eb02b9..c97011c 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -421,11 +421,11 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi, * contact max are global to the report */ td-last_field_index = field-index; return -1; - } case HID_DG_TOUCH: /* Legacy devices use TIPSWITCH and not TOUCH. * Let's just ignore this field. */ return -1; + } /* let hid-input decide for the others */ return 0; -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: TPV OpticalTouchScreen (25aa:8883) Support
Hi Chris, On Sat, Oct 27, 2012 at 2:27 AM, Chris MacDonald ch...@fourthandvine.com wrote: Hi All - Firstly, apologies if this is the wrong forum for this type of problem, (maybe linux-usb?) if this is the case please let me know. I've got an Ubuntu 12.04 install and updated the kernel to 3.6.3 (I did this after looking at http://lii-enac.fr/en/architecture/linux-input/multitouch-devices.html and assuming support for my touchscreen monitor would be better the newer the kernel): Linux parsnip 3.6.3-030603-generic #201210211349 SMP Sun Oct 21 17:57:45 UTC 2012 i686 i686 i386 GNU/Linux and an HP Compaq L2206tm touchscreen monitor that gets detected like this: Oct 26 15:28:12 parsnip kernel: [ 98.500012] usb 1-2: new high-speed USB device number 5 using ehci_hcd Oct 26 15:28:12 parsnip kernel: [ 98.632382] usb 1-2: New USB device found, idVendor=0424, idProduct=2514 Oct 26 15:28:12 parsnip kernel: [ 98.632386] usb 1-2: New USB device strings: Mfr=0, Product=0, SerialNumber=0 Oct 26 15:28:12 parsnip kernel: [ 98.632542] hub 1-2:1.0: USB hub found Oct 26 15:28:12 parsnip kernel: [ 98.632625] hub 1-2:1.0: 4 ports detected Oct 26 15:28:12 parsnip kernel: [ 98.904119] usb 1-2.1: new full-speed USB device number 6 using ehci_hcd Oct 26 15:28:12 parsnip kernel: [ 98.997072] usb 1-2.1: New USB device found, idVendor=25aa, idProduct=8883 Oct 26 15:28:12 parsnip kernel: [ 98.997075] usb 1-2.1: New USB device strings: Mfr=1, Product=2, SerialNumber=4 Oct 26 15:28:12 parsnip kernel: [ 98.997078] usb 1-2.1: Product: OpticalTouchScreen Oct 26 15:28:12 parsnip kernel: [ 98.997080] usb 1-2.1: Manufacturer: TPV Oct 26 15:28:12 parsnip kernel: [ 98.997083] usb 1-2.1: SerialNumber: Oct 26 15:28:22 parsnip kernel: [ 109.75] hid-multitouch 0003:25AA:8883.0004: usb_submit_urb(ctrl) failed: -1 Oct 26 15:28:22 parsnip kernel: [ 109.82] hid-multitouch 0003:25AA:8883.0004: timeout initializing reports Oct 26 15:28:22 parsnip kernel: [ 109.000183] input: TPV OpticalTouchScreen as /devices/pci:00/:00:1d.7/usb1/1-2/1-2.1/1-2.1:1.0/input/input8 Oct 26 15:28:22 parsnip kernel: [ 109.000282] hid-multitouch 0003:25AA:8883.0004: input,hiddev0,hidraw2: USB HID v1.10 Mouse [TPV OpticalTouchScreen] on usb-:00:1d.7-2.1/input0 X happily detects the device as well: [ 5987.987] (II) config/udev: Adding input device TPV OpticalTouchScreen (/dev/input/mouse1) [ 5987.987] (II) No input driver specified, ignoring this device. [ 5987.987] (II) This device may have been added with another device file. [ 5987.987] (II) config/udev: Adding input device TPV OpticalTouchScreen (/dev/input/event7) [ 5987.987] (**) TPV OpticalTouchScreen: Applying InputClass evdev touchscreen catchall [ 5987.987] (II) Using input driver 'evdev' for 'TPV OpticalTouchScreen' [ 5987.987] (II) Loading /usr/lib/xorg/modules/input/evdev_drv.so [ 5987.987] (**) TPV OpticalTouchScreen: always reports core events [ 5987.987] (**) evdev: TPV OpticalTouchScreen: Device: /dev/input/event7 [ 5987.987] (--) evdev: TPV OpticalTouchScreen: Vendor 0x25aa Product 0x8883 [ 5987.987] (--) evdev: TPV OpticalTouchScreen: Found absolute axes [ 5987.987] (--) evdev: TPV OpticalTouchScreen: Found absolute multitouch axes [ 5987.987] (--) evdev: TPV OpticalTouchScreen: Found x and y absolute axes [ 5987.987] (--) evdev: TPV OpticalTouchScreen: Found absolute touchscreen [ 5987.987] (II) evdev: TPV OpticalTouchScreen: Configuring as touchscreen [ 5987.987] (**) evdev: TPV OpticalTouchScreen: YAxisMapping: buttons 4 and 5 [ 5987.987] (**) evdev: TPV OpticalTouchScreen: EmulateWheelButton: 4, EmulateWheelInertia: 10, EmulateWheelTimeout: 200 [ 5987.988] (**) Option config_info udev:/sys/devices/pci:00/:00:1d.7/usb1/1-2/1-2.1/1-2.1:1.0/input/input10/event7 [ 5987.988] (II) XINPUT: Adding extended input device TPV OpticalTouchScreen (type: TOUCHSCREEN, id 10) [ 5987.988] (II) evdev: TPV OpticalTouchScreen: initialized for absolute axes. [ 5987.988] (**) TPV OpticalTouchScreen: (accel) keeping acceleration scheme 1 [ 5987.988] (**) TPV OpticalTouchScreen: (accel) acceleration profile 0 [ 5987.988] (**) TPV OpticalTouchScreen: (accel) acceleration factor: 2.000 [ 5987.988] (**) TPV OpticalTouchScreen: (accel) acceleration threshold: 4 but a test of the touch capability doesn't yield anything useful as a run of evtest doesn't show any activity after startup as I tap on the display: # evtest /dev/input/event7 Input driver version is 1.0.1 Input device ID: bus 0x3 vendor 0x25aa product 0x8883 version 0x110 Input device name: TPV OpticalTouchScreen Supported events: Event type 0 (EV_SYN) Event type 1 (EV_KEY) Event code 330 (BTN_TOUCH) Event type 3 (EV_ABS) Event code 0 (ABS_X) Value 0 Min0 Max 1919 Event code 1 (ABS_Y) Value 0 Min0 Max 1079
Re: [PATCH v2 01/11] HID: hid-input: export hidinput_calc_abs_res
Hi Henrik, first thanks for the full review of the patch series. If you think it's better, I'll split the patch in 2 to put aside the DIV_ROUND_CLOSEST. Cheers, Benjamin On Mon, Oct 29, 2012 at 7:57 PM, Henrik Rydberg rydb...@euromail.se wrote: Hi Benjamin, Exporting this function allows us to calculate the resolution in third party drivers like hid-multitouch. This patch also complete the function with additional valid axes. Signed-off-by: Benjamin Tissoires benjamin.tissoi...@gmail.com --- Nice, but please see comment below. drivers/hid/hid-input.c | 11 +-- drivers/hid/hid-multitouch.c | 1 + include/linux/hid.h | 1 + 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c index d917c0d..1b0adc3 100644 --- a/drivers/hid/hid-input.c +++ b/drivers/hid/hid-input.c @@ -208,7 +208,7 @@ static int hidinput_setkeycode(struct input_dev *dev, * Only exponent 1 length units are processed. Centimeters and inches are * converted to millimeters. Degrees are converted to radians. */ -static __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code) +__s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code) { __s32 unit_exponent = field-unit_exponent; __s32 logical_extents = field-logical_maximum - @@ -229,6 +229,12 @@ static __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code) case ABS_X: case ABS_Y: case ABS_Z: + case ABS_MT_POSITION_X: + case ABS_MT_POSITION_Y: + case ABS_MT_TOOL_X: + case ABS_MT_TOOL_Y: + case ABS_MT_TOUCH_MAJOR: + case ABS_MT_TOUCH_MINOR: if (field-unit == 0x11) { /* If centimeters */ /* Convert to millimeters */ unit_exponent += 1; @@ -281,8 +287,9 @@ static __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code) } /* Calculate resolution */ - return logical_extents / physical_extents; + return DIV_ROUND_CLOSEST(logical_extents, physical_extents); This line might be best left alone; if the round-off matters, you now risk regressing a carefully tuned userland. } +EXPORT_SYMBOL_GPL(hidinput_calc_abs_res); #ifdef CONFIG_HID_BATTERY_STRENGTH static enum power_supply_property hidinput_battery_props[] = { diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index c97011c..375a38d 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -298,6 +298,7 @@ static void set_abs(struct input_dev *input, unsigned int code, int fmax = field-logical_maximum; int fuzz = snratio ? (fmax - fmin) / snratio : 0; input_set_abs_params(input, code, fmin, fmax, fuzz, 0); + input_abs_set_res(input, code, hidinput_calc_abs_res(field, code)); } static void mt_store_field(struct hid_usage *usage, struct mt_device *td, diff --git a/include/linux/hid.h b/include/linux/hid.h index 7e1f37d..9edb06c 100644 --- a/include/linux/hid.h +++ b/include/linux/hid.h @@ -743,6 +743,7 @@ int hid_input_report(struct hid_device *, int type, u8 *, int, int); int hidinput_find_field(struct hid_device *hid, unsigned int type, unsigned int code, struct hid_field **field); struct hid_field *hidinput_get_led_field(struct hid_device *hid); unsigned int hidinput_count_leds(struct hid_device *hid); +__s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code); void hid_output_report(struct hid_report *report, __u8 *data); struct hid_device *hid_allocate_device(void); struct hid_report *hid_register_report(struct hid_device *device, unsigned type, unsigned id); -- 1.7.11.7 Thanks, Henrik -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 02/11] HID: core: fix unit exponent parsing
Hi Henrik, On Mon, Oct 29, 2012 at 8:05 PM, Henrik Rydberg rydb...@euromail.se wrote: Hi Benjamin, HID spec details special values for the HID field unit exponent. Basically, the range [0x8..0xf] correspond to [-8..-1]. Signed-off-by: Benjamin Tissoires benjamin.tissoi...@gmail.com --- Standard two's complement, in other words? yes, but for 4-bits values. The thing is that I didn't managed to figure out if it was allowed to give unit exponent with more than 4 bits... drivers/hid/hid-core.c | 10 +- drivers/hid/hid-input.c | 19 +-- 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index 9904776..6bde6e4 100644 --- a/drivers/hid/hid-core.c +++ b/drivers/hid/hid-core.c @@ -315,6 +315,7 @@ static s32 item_sdata(struct hid_item *item) static int hid_parser_global(struct hid_parser *parser, struct hid_item *item) { + __u32 raw_value; switch (item-tag) { case HID_GLOBAL_ITEM_TAG_PUSH: @@ -365,7 +366,14 @@ static int hid_parser_global(struct hid_parser *parser, struct hid_item *item) return 0; case HID_GLOBAL_ITEM_TAG_UNIT_EXPONENT: - parser-global.unit_exponent = item_sdata(item); + /* Units exponent negative numbers are given through a special + * table. + * See 6.2.2.7 Global Items for more information. */ + raw_value = item_udata(item); + if ((raw_value 3) == 1) + parser-global.unit_exponent = raw_value - 16; + else + parser-global.unit_exponent = raw_value; I beleive the function you want is snto32(value, 4). but to be sure, we should still do a test against (raw_value 0xf0) for values with more than 4 bits, no? Cheers, Benjamin return 0; case HID_GLOBAL_ITEM_TAG_UNIT: diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c index 1b0adc3..fc9f2b5 100644 --- a/drivers/hid/hid-input.c +++ b/drivers/hid/hid-input.c @@ -215,7 +215,7 @@ __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code) field-logical_minimum; __s32 physical_extents = field-physical_maximum - field-physical_minimum; - __s32 prev; + __s32 prev, tmp_exponent; /* Check if the extents are sane */ if (logical_extents = 0 || physical_extents = 0) @@ -235,17 +235,24 @@ __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code) case ABS_MT_TOOL_Y: case ABS_MT_TOUCH_MAJOR: case ABS_MT_TOUCH_MINOR: - if (field-unit == 0x11) { /* If centimeters */ + if (field-unit 0xff00) /* Not a length */ + return 0; + tmp_exponent = field-unit 4; + if ((tmp_exponent 3) == 1) + tmp_exponent -= 16; Ditto. + switch (field-unit 0xf) { + case 0x1: /* If centimeters */ /* Convert to millimeters */ - unit_exponent += 1; - } else if (field-unit == 0x13) { /* If inches */ + unit_exponent += tmp_exponent; + break; + case 0x3: /* If inches */ /* Convert to millimeters */ prev = physical_extents; physical_extents *= 254; if (physical_extents prev) return 0; - unit_exponent -= 1; - } else { + unit_exponent += tmp_exponent - 2; + default: return 0; } break; -- 1.7.11.7 Thanks, Henrik -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 03/11] HID: hid-input: add usage_index argument in input_mapping and event.
On Mon, Oct 29, 2012 at 8:25 PM, Henrik Rydberg rydb...@euromail.se wrote: Hi Benjamin, Currently, there is no way to know the index of the current field in the .input_mapping and .event callbacks when this field is inside an array of HID fields. This patch forwards this index to the input_mapping and event callbacks. I agree with the idea, but the function argument list is becoming ridiculously long... Could we remove the usage pointer argument, at least? yeah, totally agree. Let me just check whether it will not introduce more problems than it solves for my driver. int (*event)(struct hid_device *hdev, struct hid_field *field, unsigned int usage_index, __s32 value); @@ -1071,19 +1072,24 @@ static void hid_input_field(struct hid_device *hid, struct hid_field *field, for (n = 0; n count; n++) { if (HID_MAIN_ITEM_VARIABLE field-flags) { - hid_process_event(hid, field, field-usage[n], value[n], interrupt); + hid_process_event(hid, field, field-usage[n], n, + value[n], interrupt); continue; } if (field-value[n] = min field-value[n] = max field-usage[field-value[n] - min].hid search(value, field-value[n], count)) - hid_process_event(hid, field, field-usage[field-value[n] - min], 0, interrupt); + hid_process_event(hid, field, + field-usage[field-value[n] - min], n, + 0, interrupt); Wrong index? oops, I'll have to check that. Thanks, Benjamin if (value[n] = min value[n] = max field-usage[value[n] - min].hid search(field-value, value[n], count)) - hid_process_event(hid, field, field-usage[value[n] - min], 1, interrupt); + hid_process_event(hid, field, + field-usage[value[n] - min], n, + 1, interrupt); Wrong index? } memcpy(field-value, value, count * sizeof(__s32)); Thanks, Henrik -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 04/11] HID: hid-multitouch: support arrays for the split of the touches in a report
Hi Henrik, On Mon, Oct 29, 2012 at 10:49 PM, Henrik Rydberg rydb...@euromail.se wrote: Hi Benjamin, Win8 certification introduced the ability to transmit two X and two Y per touch. The specification precises that it must be in an array, with a report count == 2. The number two never really enters the patch, so maybe it should be dropped to avoid confusion. It probably makes more sense to comment on in a later patch, when the reports are actually used. Yep, it seems that the commit message is not good. I'll rewrite it in v3. This test guarantees that we split the touches on the last element in this array. Signed-off-by: Benjamin Tissoires benjamin.tissoi...@enac.fr --- drivers/hid/hid-multitouch.c | 16 ++-- 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index 725d155..95562d8 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -577,12 +577,16 @@ static int mt_event(struct hid_device *hid, struct hid_field *field, return 0; } - if (usage-hid == td-last_slot_field) - mt_complete_slot(td, field-hidinput-input); - - if (field-index == td-last_field_index - td-num_received = td-num_expected) - mt_sync_frame(td, field-hidinput-input); + if (usage_index + 1 == field-report_count) { + /* we only take into account the last report + * of a field if report_count 1 */ Seems we could drop of a field if report_count 1 here, and be even more correct. oops ;-) Cheers, Benjamin + if (usage-hid == td-last_slot_field) + mt_complete_slot(td, field-hidinput-input); + + if (field-index == td-last_field_index + td-num_received = td-num_expected) + mt_sync_frame(td, field-hidinput-input); + } } -- 1.7.11.7 Thanks, Henrik -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 06/11] HID: hid-multitouch: support T and C for win8 devices
Hi Henrik, On Mon, Oct 29, 2012 at 11:00 PM, Henrik Rydberg rydb...@euromail.se wrote: Hi Benjamin, Win8 input specification clarifies the X and Y sent by devices. It distincts the position where the user wants to Touch (T) from the center of the ellipsoide (C). This patch enable supports for this distinction in hid-multitouch. We recognize Win8 certified devices from their vendor feature 0xffc5 where Microsoft put a signed blob in the report to check if the device passed the certification. Signed-off-by: Benjamin Tissoires benjamin.tissoi...@gmail.com --- drivers/hid/hid-multitouch.c | 53 1 file changed, 44 insertions(+), 9 deletions(-) This is great, just a few comments below. diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index 41f2981..000c979 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -52,9 +52,10 @@ MODULE_LICENSE(GPL); #define MT_QUIRK_VALID_IS_CONFIDENCE (1 6) #define MT_QUIRK_SLOT_IS_CONTACTID_MINUS_ONE (1 8) #define MT_QUIRK_NO_AREA (1 9) +#define MT_QUIRK_WIN_8_CERTIFIED (1 10) struct mt_slot { - __s32 x, y, p, w, h; + __s32 x, y, cx, cy, p, w, h; __s32 contactid;/* the device ContactID assigned to this slot */ bool touch_state; /* is the touch valid? */ }; @@ -292,6 +293,10 @@ static void mt_feature_mapping(struct hid_device *hdev, td-maxcontacts = td-mtclass.maxcontacts; break; + case 0xffc5: + if (field-report_count == 256 field-report_size == 8) + td-mtclass.quirks |= MT_QUIRK_WIN_8_CERTIFIED; + break; } } @@ -350,18 +355,36 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi, case HID_UP_GENDESK: switch (usage-hid) { case HID_GD_X: - hid_map_usage(hi, usage, bit, max, + if (cls-quirks MT_QUIRK_WIN_8_CERTIFIED Parenthesis, please. Precedence is not always enough. oops + usage_index) { + hid_map_usage(hi, usage, bit, max, + EV_ABS, ABS_MT_TOOL_X); + set_abs(hi-input, ABS_MT_TOOL_X, field, + cls-sn_move); + } else { + hid_map_usage(hi, usage, bit, max, EV_ABS, ABS_MT_POSITION_X); - set_abs(hi-input, ABS_MT_POSITION_X, field, - cls-sn_move); + set_abs(hi-input, ABS_MT_POSITION_X, field, + cls-sn_move); + } + Do we really want to do the latter several times, even if the device is not a win8 one? I don't get your point here. The only difference with the previous release is that it will treat differently the first in the array than the others. For non win8 devices, there is no changes in the behavior. Could you elaborate a little bit more, please? mt_store_field(usage, td, hi); td-last_field_index = field-index; return 1; case HID_GD_Y: - hid_map_usage(hi, usage, bit, max, + if (cls-quirks MT_QUIRK_WIN_8_CERTIFIED Ditto. + usage_index) { + hid_map_usage(hi, usage, bit, max, + EV_ABS, ABS_MT_TOOL_Y); + set_abs(hi-input, ABS_MT_TOOL_Y, field, + cls-sn_move); + } else { + hid_map_usage(hi, usage, bit, max, EV_ABS, ABS_MT_POSITION_Y); - set_abs(hi-input, ABS_MT_POSITION_Y, field, - cls-sn_move); + set_abs(hi-input, ABS_MT_POSITION_Y, field, + cls-sn_move); + } + Ditto. mt_store_field(usage, td, hi); td-last_field_index = field-index; return 1; @@ -502,6 +525,12 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input) input_event(input, EV_ABS, ABS_MT_POSITION_X, s-x); input_event(input, EV_ABS, ABS_MT_POSITION_Y, s-y); + if (td-mtclass.quirks MT_QUIRK_WIN_8_CERTIFIED) { + input_event(input, EV_ABS, ABS_MT_TOOL_X, + s-cx); Won't this fit on one line? I'm afraid not: 81 columns... ;-) + input_event(input, EV_ABS
Re: [PATCH v2 07/11] HID: hid-multitouch: move ALWAYS_VALID quirk check
On Mon, Oct 29, 2012 at 11:16 PM, Henrik Rydberg rydb...@euromail.se wrote: Hi Benjamin, Win 8 device specification changed the requirements for the hid usages of the multitouch devices. Now InRange is optional and must be only used when the device supports hovering. This ensures that the quirk ALWAYS_VALID is taken into account and also ensures its precedence over the other VALID* quirks. Signed-off-by: Benjamin Tissoires benjamin.tissoi...@gmail.com --- drivers/hid/hid-multitouch.c | 6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) Since we seem to never actually reset td, this seems equivalent, unless some device added ALWAYS VALID by mistake, even if INRANGE is not part of the report descriptor. Yes, it is equivalent for Win7 devices. The field InRange was mandatory, and currently, nobody mixed ALWAYS_VALID with other quirks. But now, InRange is optional, and device not supporting hovering should not use it, so it's mandatory to move it at a place we can be sure it will be taken into account. diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index 000c979..43bd8e8 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -506,7 +506,7 @@ static int mt_compute_slot(struct mt_device *td, struct input_dev *input) */ static void mt_complete_slot(struct mt_device *td, struct input_dev *input) { - if (td-curvalid) { + if (td-curvalid || td-mtclass.quirks MT_QUIRK_ALWAYS_VALID) { I found at least one presence of this construct in the kernel, but I think the overwhelming majority use parenthesis. oops, I was really angry against parenthesis in this patch series :) Cheers, Benjamin int slotnum = mt_compute_slot(td, input); struct mt_slot *s = td-curdata; @@ -561,9 +561,7 @@ static int mt_event(struct hid_device *hid, struct hid_field *field, if (hid-claimed HID_CLAIMED_INPUT) { switch (usage-hid) { case HID_DG_INRANGE: - if (quirks MT_QUIRK_ALWAYS_VALID) - td-curvalid = true; - else if (quirks MT_QUIRK_VALID_IS_INRANGE) + if (quirks MT_QUIRK_VALID_IS_INRANGE) td-curvalid = value; break; case HID_DG_TIPSWITCH: -- 1.7.11.7 Thanks, Henrik -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 08/11] HID: hid-multitouch: fix Win 8 protocol
Hi Henrik, On Mon, Oct 29, 2012 at 11:19 PM, Henrik Rydberg rydb...@euromail.se wrote: On Fri, Oct 26, 2012 at 10:44:24AM +0200, Benjamin Tissoires wrote: Win 8 specification is much more precise than the Win 7 one. Moreover devices that need to take certification must be submitted to Microsoft. The result is a better protocol support and we can rely on that to skip all the messy tests we used to do. The protocol specify the fact that each valid touch must be reported within a frame, and that the release touch coordinate must be the same than the last known touch. So we can use the always_valid quirk and dismiss reports when we touch coordiante do not follow this rule. Signed-off-by: Benjamin Tissoires benjamin.tissoi...@gmail.com --- drivers/hid/hid-multitouch.c | 16 ++-- 1 file changed, 14 insertions(+), 2 deletions(-) Why do we need this patch? This patch prevents a corner case where the device use contactID 0 for it's first reported touch. Once you got the invalid touches, most of the time, contactID will be 0, x, y, and other fields too. So this ensures to avoid conflict between valid values and garbage values. The problem lies in the fact that we don't have the whole overview of the frame (with the contact count) to decide which contacts are good and which are not. Cheers, Benjamin diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index 43bd8e8..bd23f19 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -510,6 +510,18 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input) int slotnum = mt_compute_slot(td, input); struct mt_slot *s = td-curdata; + if (td-mtclass.quirks MT_QUIRK_WIN_8_CERTIFIED + !s-touch_state) { + struct input_mt_slot *slot = input-mt-slots[slotnum]; + int prv_x = input_mt_get_value(slot, ABS_MT_POSITION_X); + int prv_y = input_mt_get_value(slot, ABS_MT_POSITION_Y); + /* valid releases occurs when the last x and y positions + * are the same as the last known touch. */ + if (!input_mt_is_active(slot) || + prv_x != s-x || prv_y != s-y) + return; + } + if (slotnum 0 || slotnum = td-maxcontacts) return; @@ -681,8 +693,8 @@ static void mt_post_parse_default_settings(struct mt_device *td) { __s32 quirks = td-mtclass.quirks; - /* unknown serial device needs special quirks */ - if (td-touches_by_report == 1) { + /* unknown serial devices or win8 ones need special quirks */ + if (td-touches_by_report == 1 || quirks MT_QUIRK_WIN_8_CERTIFIED) { quirks |= MT_QUIRK_ALWAYS_VALID; quirks = ~MT_QUIRK_NOT_SEEN_MEANS_UP; quirks = ~MT_QUIRK_VALID_IS_INRANGE; -- 1.7.11.7 Henrik -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 09/11] HID: hid-multitouch: support for hovering devices
On Mon, Oct 29, 2012 at 11:31 PM, Henrik Rydberg rydb...@euromail.se wrote: Hi Benjamin, Win8 devices supporting hovering must provides InRange HID field. The information that the finger is here but is not touching the surface is sent to the user space through ABS_MT_DISTANCE as required by the multitouch protocol. Signed-off-by: Benjamin Tissoires benjamin.tissoi...@gmail.com --- drivers/hid/hid-multitouch.c | 21 +++-- 1 file changed, 19 insertions(+), 2 deletions(-) I suppose the idea here is to send position and distance values even when there is no touch, but the code does not seem to do that? Well, the code does it, but I agree it's not very clear. The touch_state field is not for win8 with hovering the fact that the finger is touching the surface, but the fact that a finger is in range of the device. This is why tipswitch is filling z instead of touch_state. I agree, it's not that clear, I'll try to do better in v3. diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index bd23f19..c0ab1c6 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -55,7 +55,7 @@ MODULE_LICENSE(GPL); #define MT_QUIRK_WIN_8_CERTIFIED (1 10) struct mt_slot { - __s32 x, y, cx, cy, p, w, h; + __s32 x, y, cx, cy, z, p, w, h; __s32 contactid;/* the device ContactID assigned to this slot */ bool touch_state; /* is the touch valid? */ }; @@ -394,6 +394,12 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi, case HID_UP_DIGITIZER: switch (usage-hid) { case HID_DG_INRANGE: + if (cls-quirks MT_QUIRK_WIN_8_CERTIFIED) { + hid_map_usage(hi, usage, bit, max, + EV_ABS, ABS_MT_DISTANCE); + input_set_abs_params(hi-input, + ABS_MT_DISTANCE, 0, 1, 0, 0); + } mt_store_field(usage, td, hi); td-last_field_index = field-index; return 1; @@ -511,6 +517,11 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input) struct mt_slot *s = td-curdata; if (td-mtclass.quirks MT_QUIRK_WIN_8_CERTIFIED + !test_bit(ABS_MT_DISTANCE, input-absbit)) + /* If InRange is not present, rely on TipSwitch */ + s-touch_state = !s-z; + + if (td-mtclass.quirks MT_QUIRK_WIN_8_CERTIFIED !s-touch_state) { struct input_mt_slot *slot = input-mt-slots[slotnum]; int prv_x = input_mt_get_value(slot, ABS_MT_POSITION_X); @@ -543,6 +554,7 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input) input_event(input, EV_ABS, ABS_MT_TOOL_Y, s-cy); } + input_event(input, EV_ABS, ABS_MT_DISTANCE, s-z); This only happens if touch_state is true? mmm, yes, we should move it outside this condition. input_event(input, EV_ABS, ABS_MT_ORIENTATION, wide); input_event(input, EV_ABS, ABS_MT_PRESSURE, s-p); input_event(input, EV_ABS, ABS_MT_TOUCH_MAJOR, major); @@ -575,11 +587,16 @@ static int mt_event(struct hid_device *hid, struct hid_field *field, case HID_DG_INRANGE: if (quirks MT_QUIRK_VALID_IS_INRANGE) td-curvalid = value; + if (quirks MT_QUIRK_WIN_8_CERTIFIED) + td-curdata.touch_state = value; break; case HID_DG_TIPSWITCH: if (quirks MT_QUIRK_NOT_SEEN_MEANS_UP) td-curvalid = value; - td-curdata.touch_state = value; + if (quirks MT_QUIRK_WIN_8_CERTIFIED) + td-curdata.z = !value; Here, z is a boolean? this is what is written in Win 8 specification. So ABS_MT_DISTANCE will have a range of [0..1] and this behavior is right. When we will have device with real Z hovering measures, then it will be the time to change this, but currently, we only have a boolean in InRange. Cheers, Benjamin + else + td-curdata.touch_state = value; break; case HID_DG_CONFIDENCE: if (quirks MT_QUIRK_VALID_IS_CONFIDENCE) -- 1.7.11.7 Henrik -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] HID: Add driver for ION iCade
Hi Bastien, a few random comments: On Tue, Oct 30, 2012 at 8:55 PM, Bastien Nocera had...@hadess.net wrote: Add a driver for the ION iCade mini arcade cabinet [1]. The device generates a key press and release for each joystick movement or button press or release. For example, moving the stick to the left will generate the A key being pressed and the released. A list of all the combinations is available in the iCade developer guide [2]. This driver hides all this and makes the device work as a generic joystick. [1]: http://www.ionaudio.com/products/details/icade [2]: http://www.ionaudio.com/downloads/iCade_Dev_Resource_v1.3.pdf You are missing your Signed-off-by line. Without it Jiri won't be able to pick your driver. --- drivers/hid/Kconfig | 6 ++ drivers/hid/Makefile| 1 + drivers/hid/hid-core.c | 1 + drivers/hid/hid-icade.c | 194 drivers/hid/hid-ids.h | 3 + 5 files changed, 205 insertions(+) create mode 100644 drivers/hid/hid-icade.c diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig index 1630150..750d435 100644 --- a/drivers/hid/Kconfig +++ b/drivers/hid/Kconfig @@ -265,6 +265,12 @@ config HID_GYRATION ---help--- Support for Gyration remote control. +config HID_ICADE + tristate ION iCade arcade controller + depends on (BT_HIDP) + ---help--- + Support for the ION iCade arcade controller to work as a joystick. + config HID_TWINHAN tristate Twinhan IR remote control depends on USB_HID diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile index cef68ca..9860d97 100644 --- a/drivers/hid/Makefile +++ b/drivers/hid/Makefile @@ -59,6 +59,7 @@ obj-$(CONFIG_HID_LCPOWER) += hid-lcpower.o obj-$(CONFIG_HID_LENOVO_TPKBD) += hid-lenovo-tpkbd.o obj-$(CONFIG_HID_LOGITECH) += hid-logitech.o obj-$(CONFIG_HID_LOGITECH_DJ) += hid-logitech-dj.o +obj-$(CONFIG_HID_ICADE)+= hid-icade.o mmm this should go above between hid-hyperv.o and hid-kensington.o... :) obj-$(CONFIG_HID_MAGICMOUSE)+= hid-magicmouse.o obj-$(CONFIG_HID_MICROSOFT)+= hid-microsoft.o obj-$(CONFIG_HID_MONTEREY) += hid-monterey.o diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index bd3971b..0a6b36f 100644 --- a/drivers/hid/hid-core.c +++ b/drivers/hid/hid-core.c @@ -1572,6 +1572,7 @@ static const struct hid_device_id hid_have_special_driver[] = { { HID_USB_DEVICE(USB_VENDOR_ID_INTEL_8086, USB_DEVICE_ID_SENSOR_HUB_09FA) }, { HID_USB_DEVICE(USB_VENDOR_ID_INTEL_8087, USB_DEVICE_ID_SENSOR_HUB_1020) }, { HID_USB_DEVICE(USB_VENDOR_ID_INTEL_8087, USB_DEVICE_ID_SENSOR_HUB_09FA) }, + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_ION, USB_DEVICE_ID_ICADE) }, { HID_USB_DEVICE(USB_VENDOR_ID_KENSINGTON, USB_DEVICE_ID_KS_SLIMBLADE) }, { HID_USB_DEVICE(USB_VENDOR_ID_KEYTOUCH, USB_DEVICE_ID_KEYTOUCH_IEC) }, { HID_USB_DEVICE(USB_VENDOR_ID_KYE, USB_DEVICE_ID_KYE_ERGO_525V) }, diff --git a/drivers/hid/hid-icade.c b/drivers/hid/hid-icade.c new file mode 100644 index 000..54e1cb9 --- /dev/null +++ b/drivers/hid/hid-icade.c @@ -0,0 +1,194 @@ +/* + * USB HID quirks support for Linux + * + * Copyright (c) 1999 Andreas Gal + * Copyright (c) 2000-2005 Vojtech Pavlik vojt...@suse.cz + * Copyright (c) 2005 Michael Haboustak mi...@cinci.rr.com for Concept2, Inc + * Copyright (c) 2006-2007 Jiri Kosina + * Copyright (c) 2007 Paul Walmsley + * Copyright (c) 2008 Jiri Slaby jirisl...@gmail.com I'm not sure we should keep these copyrights. Actually, nearly all the code is new except the skeleton. + * Copyright (c) 2012 Bastien Nocera had...@hadess.net + * Copyright (c) 2012 Benjamin Tissoires benjamin.tissoi...@gmail.com + */ + +/* + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the Free + * Software Foundation; either version 2 of the License, or (at your option) + * any later version. + */ + +#include linux/device.h +#include linux/hid.h +#include linux/module.h +#include linux/slab.h +#include linux/usb.h Not sure usb.h is required here. + +#include hid-ids.h + +struct icade_key_translation { + u16 from_key; + u16 from_usage; /* hid_keyboard[from_usage] == from_key */ + u16 to; + u8 press : 1; checkpatch.pl says: ERROR: spaces prohibited around that ':' (ctx:WxW) +}; + +/* + * ↑ A C Y L + * ← → + * ↓ B X Z R + * + * + * UP ON,OFF = w,e + * RT ON,OFF = d,c + * DN ON,OFF = x,z + * LT ON,OFF = a,q + * A ON,OFF = y,t + * B ON,OFF = h,r + * C ON,OFF = u,f + * X ON,OFF = j,n + * Y ON,OFF = i,m + * Z ON,OFF = k,p + * L ON,OFF = o,g + * R ON,OFF = l,v + */ +static const struct icade_key_translation icade_keys
Re: [PATCH] HID: Add driver for ION iCade
Hi Bastien, this v2 is good for me: Reviewed-by: Benjamin Tissoires benjamin.tissoi...@gmail.com Cheers, Benjamin On Wed, Oct 31, 2012 at 12:10 PM, Bastien Nocera had...@hadess.net wrote: Add a driver for the ION iCade mini arcade cabinet [1]. The device generates a key press and release for each joystick movement or button press or release. For example, moving the stick to the left will generate the A key being pressed and the released. A list of all the combinations is available in the iCade developer guide [2]. This driver hides all this and makes the device work as a generic joystick. [1]: http://www.ionaudio.com/products/details/icade [2]: http://www.ionaudio.com/downloads/iCade_Dev_Resource_v1.3.pdf Signed-off-by: Bastien Nocera had...@hadess.net --- drivers/hid/Kconfig | 9 ++ drivers/hid/Makefile| 1 + drivers/hid/hid-core.c | 1 + drivers/hid/hid-icade.c | 259 drivers/hid/hid-ids.h | 3 + 5 files changed, 273 insertions(+) create mode 100644 drivers/hid/hid-icade.c diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig index 1630150..813f0fe 100644 --- a/drivers/hid/Kconfig +++ b/drivers/hid/Kconfig @@ -265,6 +265,15 @@ config HID_GYRATION ---help--- Support for Gyration remote control. +config HID_ICADE + tristate ION iCade arcade controller + depends on (BT_HIDP) + ---help--- + Support for the ION iCade arcade controller to work as a joystick. + + To compile this driver as a module, choose M here: the + module will be called hid-icade. + config HID_TWINHAN tristate Twinhan IR remote control depends on USB_HID diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile index cef68ca..85c910c 100644 --- a/drivers/hid/Makefile +++ b/drivers/hid/Makefile @@ -52,6 +52,7 @@ obj-$(CONFIG_HID_GYRATION)+= hid-gyration.o obj-$(CONFIG_HID_HOLTEK) += hid-holtek-kbd.o obj-$(CONFIG_HID_HOLTEK) += hid-holtekff.o obj-$(CONFIG_HID_HYPERV_MOUSE) += hid-hyperv.o +obj-$(CONFIG_HID_ICADE)+= hid-icade.o obj-$(CONFIG_HID_KENSINGTON) += hid-kensington.o obj-$(CONFIG_HID_KEYTOUCH) += hid-keytouch.o obj-$(CONFIG_HID_KYE) += hid-kye.o diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index bd3971b..0a6b36f 100644 --- a/drivers/hid/hid-core.c +++ b/drivers/hid/hid-core.c @@ -1572,6 +1572,7 @@ static const struct hid_device_id hid_have_special_driver[] = { { HID_USB_DEVICE(USB_VENDOR_ID_INTEL_8086, USB_DEVICE_ID_SENSOR_HUB_09FA) }, { HID_USB_DEVICE(USB_VENDOR_ID_INTEL_8087, USB_DEVICE_ID_SENSOR_HUB_1020) }, { HID_USB_DEVICE(USB_VENDOR_ID_INTEL_8087, USB_DEVICE_ID_SENSOR_HUB_09FA) }, + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_ION, USB_DEVICE_ID_ICADE) }, { HID_USB_DEVICE(USB_VENDOR_ID_KENSINGTON, USB_DEVICE_ID_KS_SLIMBLADE) }, { HID_USB_DEVICE(USB_VENDOR_ID_KEYTOUCH, USB_DEVICE_ID_KEYTOUCH_IEC) }, { HID_USB_DEVICE(USB_VENDOR_ID_KYE, USB_DEVICE_ID_KYE_ERGO_525V) }, diff --git a/drivers/hid/hid-icade.c b/drivers/hid/hid-icade.c new file mode 100644 index 000..1d6565e --- /dev/null +++ b/drivers/hid/hid-icade.c @@ -0,0 +1,259 @@ +/* + * ION iCade input driver + * + * Copyright (c) 2012 Bastien Nocera had...@hadess.net + * Copyright (c) 2012 Benjamin Tissoires benjamin.tissoi...@gmail.com + */ + +/* + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the Free + * Software Foundation; either version 2 of the License, or (at your option) + * any later version. + */ + +#include linux/device.h +#include linux/hid.h +#include linux/module.h + +#include hid-ids.h + +/* + * ↑ A C Y L + * ← → + * ↓ B X Z R + * + * + * UP ON,OFF = w,e + * RT ON,OFF = d,c + * DN ON,OFF = x,z + * LT ON,OFF = a,q + * A ON,OFF = y,t + * B ON,OFF = h,r + * C ON,OFF = u,f + * X ON,OFF = j,n + * Y ON,OFF = i,m + * Z ON,OFF = k,p + * L ON,OFF = o,g + * R ON,OFF = l,v + */ + +/* The translation code uses HID usage instead of input layer + * keys. This code generates a lookup table that makes + * translation quick. + * + * #include linux/input.h + * #include stdio.h + * #include assert.h + * + * #define unk KEY_UNKNOWN + * + * copy of hid_keyboard[] from hid-input.c + * + * struct icade_key_translation { + * int from; + * const char *to; + * int press; + * }; + * + * static const struct icade_key_translation icade_keys[] = { + *{ KEY_W,KEY_UP, 1 }, + *{ KEY_E,KEY_UP, 0 }, + *{ KEY_D,KEY_RIGHT, 1 }, + *{ KEY_C,KEY_RIGHT, 0 }, + *{ KEY_X,KEY_DOWN, 1 }, + *{ KEY_Z,KEY_DOWN, 0
Re: [PATCH v2 08/11] HID: hid-multitouch: fix Win 8 protocol
Hi Henrik, On Wed, Oct 31, 2012 at 7:53 PM, Henrik Rydberg rydb...@euromail.se wrote: Hi Benjamin, On Mon, Oct 29, 2012 at 11:19 PM, Henrik Rydberg rydb...@euromail.se wrote: On Fri, Oct 26, 2012 at 10:44:24AM +0200, Benjamin Tissoires wrote: Win 8 specification is much more precise than the Win 7 one. Moreover devices that need to take certification must be submitted to Microsoft. The result is a better protocol support and we can rely on that to skip all the messy tests we used to do. The protocol specify the fact that each valid touch must be reported within a frame, and that the release touch coordinate must be the same than the last known touch. So we can use the always_valid quirk and dismiss reports when we touch coordiante do not follow this rule. Signed-off-by: Benjamin Tissoires benjamin.tissoi...@gmail.com --- drivers/hid/hid-multitouch.c | 16 ++-- 1 file changed, 14 insertions(+), 2 deletions(-) Why do we need this patch? This patch prevents a corner case where the device use contactID 0 for it's first reported touch. Once you got the invalid touches, most of the time, contactID will be 0, x, y, and other fields too. So this ensures to avoid conflict between valid values and garbage values. The problem lies in the fact that we don't have the whole overview of the frame (with the contact count) to decide which contacts are good and which are not. I am sorry, but your explanation did not make me any wiser. :-) Are Sorry, for that. Let's try with other words. you saying that touch state changes and touch property changes are mutually exclusive? For all win8 devices, or just for the serial protocol ones? For what devices is the current implementation a problem? The goal of this patch is to implement in a reliable way Win 8 multitouch protocol (to avoid quirking many devices). Thanks to the precision they made in the specification, I think it is feasible: they add the dynamic part that were missing in Win 7 spec: When sending data in hybrid or parallel mode, a contact that is delivered in one report must be delivered in all subsequent reports until it is lifted off the screen. If time is needed to adequate determine if the contact was lifted off the surface, the device must report the last known position of the contact and then deliver the “UP” state of the contact in a subsequent report. Devices should not send a report without the information for that contact while trying to determine its current state. Thus, the quirk ALWAYS_VALID fits very well with win 8 devices (the device has to send the touch until it is lifted and out of range, and the device must send the 'up' state). The problem lies that some devices may reuse contact id 0 within the frame for the end of the report (my Win8 device doesn't has this kind of problem): With the following hid usages: I - contact Id T - tip switch X, Y - X, Y S - scan time C - contact count a friendly device would report: I:1 T:1 X:125 X:130 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:0 C:01 I:1 T:1 X:130 X:135 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:1 C:01 I:1 T:1 X:135 X:140 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:2 C:01 I:1 T:1 X:140 X:145 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:3 C:01 I:1 T:0 X:140 X:145 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:4 C:01 *but*, I've already seen win 7 devices, that do send: I:0 T:1 X:125 X:130 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:0 C:01 I:0 T:1 X:130 X:135 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:1 C:01 I:0 T:1 X:135 X:140 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:2 C:01 I:0 T:1 X:140 X:145 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:3 C:01 I:0 T:0 X:140 X:145 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:4 C:01 The difference lies in the first bit, contact id is 0. So, the quirk always valid is not sufficient because the second touch in the frame will override the values of the first (the valid one). As Microsoft says that the device must report the last known position of the contact and then deliver the “UP” state of the contact, so we can safely discard the second touch because X and Y do not match the current state of the valid touch. Then, as exposed in the How to Design and Test Multitouch Hardware Solutions for Windows 8 document here: http://msdn.microsoft.com/en-us/library/windows/hardware/hh872968.aspx when the device attempt the certification, if the up is not valid, the error Last move location different raises, which, I hope will prevent the device to get the certification. I hope it is clearer now. I am asking because this looks more like a device quirk than anything else. and yes, it looks like a quirk, we could make the Last move location different presented like a quirk, but it is only followed by win 8 devices (or it is by luck). Cheers, Benjamin Thanks, Henrik -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo
Re: [PATCH v2 08/11] HID: hid-multitouch: fix Win 8 protocol
On Sun, Nov 4, 2012 at 9:54 PM, Henrik Rydberg rydb...@euromail.se wrote: Hi Benjamin, The goal of this patch is to implement in a reliable way Win 8 multitouch protocol (to avoid quirking many devices). Thanks to the precision they made in the specification, I think it is feasible: they add the dynamic part that were missing in Win 7 spec: When sending data in hybrid or parallel mode, a contact that is delivered in one report must be delivered in all subsequent reports until it is lifted off the screen. If time is needed to adequate determine if the contact was lifted off the surface, the device must report the last known position of the contact and then deliver the “UP” state of the contact in a subsequent report. Devices should not send a report without the information for that contact while trying to determine its current state. The text seems to say that devices are not required to send touch state information in a separate frame, but if the device needs time to determine the touch state, the touch properties should stay the same during that time. Yes and no: - yes, if the device needs time to determine the up state, it should send at every frame the last known properties. - no, I never said that the information should be split in separate frames: I think you are confused by the serial protocol. I'm talking here about the parallel or the hybrid one. Thus, the quirk ALWAYS_VALID fits very well with win 8 devices (the device has to send the touch until it is lifted and out of range, and the device must send the 'up' state). One could simply add another quirk which fits the win8 case exactly instead. No need to change the existing one. I never changed the existing one. The quirk ALWAYS_VALID means that whatever the device sends, the information are valid. The fact is that the serial protocol can be handled with this quirk (otherwise, we would have called this quirk SERIAL, and not ALWAYS_VALID). So the Win 8 case doesn't need a new quirk, the ALWAYS_VALID fits. The problem lies that some devices may reuse contact id 0 within the frame for the end of the report (my Win8 device doesn't has this kind of problem): With the following hid usages: I - contact Id T - tip switch X, Y - X, Y S - scan time C - contact count a friendly device would report: I:1 T:1 X:125 X:130 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:0 C:01 I:1 T:1 X:130 X:135 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:1 C:01 I:1 T:1 X:135 X:140 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:2 C:01 I:1 T:1 X:140 X:145 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:3 C:01 I:1 T:0 X:140 X:145 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:4 C:01 *but*, I've already seen win 7 devices, that do send: I:0 T:1 X:125 X:130 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:0 C:01 I:0 T:1 X:130 X:135 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:1 C:01 I:0 T:1 X:135 X:140 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:2 C:01 I:0 T:1 X:140 X:145 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:3 C:01 I:0 T:0 X:140 X:145 Y:750 Y:755 I:0 T:0 X:0 X:0 Y:0 Y:0 S:4 C:01 I see, more brain-damaged usage. :-) Still, there seems to be a simpler way to distinguish this case: if there are more than one touch with the same id in the frame, use the one with T=1. sigh... the problem in your sentence is there: with the same id in the frame. This forces me to introduce the function input_mt_is_used in mt.h... The difference lies in the first bit, contact id is 0. So, the quirk always valid is not sufficient because the second touch in the frame will override the values of the first (the valid one). As Microsoft says that the device must report the last known position of the contact and then deliver the “UP” state of the contact, so we can safely discard the second touch because X and Y do not match the current state of the valid touch. Then, as exposed in the How to Design and Test Multitouch Hardware Solutions for Windows 8 document here: http://msdn.microsoft.com/en-us/library/windows/hardware/hh872968.aspx when the device attempt the certification, if the up is not valid, the error Last move location different raises, which, I hope will prevent the device to get the certification. I think it would be too fragile to rely on this assumption. Hopefully the suggestion above will work equally well. I'm not sure it is fragile, because otherwise that means that the certification is worthless. But anyway, your solution is valid too, so I'll implement it. Cheers, Benjamin Thanks, Henrik -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 11/11] HID: hid-multitouch: get rid of usbhid depedency for general path
Hi Henrik, On Mon, Nov 5, 2012 at 1:57 PM, Henrik Rydberg rydb...@euromail.se wrote: Hi Benjamin, This patch factorizes the hid set_feature command by using hid_device-hid_output_raw_report instead of direclty relying on usbhid. This makes the driver usb independant. However I still can't remove the 2 usb related headers because the function mt_resume has a specific patch for usb devices. Signed-off-by: Benjamin Tissoires benjamin.tissoi...@gmail.com --- drivers/hid/hid-multitouch.c | 63 ++-- 1 file changed, 37 insertions(+), 26 deletions(-) In my drawer, I have a patchset that aims to remove all usbhid dependence, from all the drivers. Perhaps the attached patch is something to consider here? yep, removing usbhid dependencies is a good thing. See my review below :) I have a tentative patch taking your comments into account, and it is likely that we want to go that way. However, as to not hold up your patchset, perhaps we could do without it for now. Regarding the hardwired usbhid dependency, I think the solution is to move that code to usbhid itself. Thanks, Henrik -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 11/11] HID: hid-multitouch: get rid of usbhid depedency for general path
Hi Henrik, grrr damn new gmail interface, I clicked on the wrong button. sorry for the noise. On Mon, Nov 5, 2012 at 2:28 PM, Benjamin Tissoires benjamin.tissoi...@gmail.com wrote: Hi Henrik, On Mon, Nov 5, 2012 at 1:57 PM, Henrik Rydberg rydb...@euromail.se wrote: Hi Benjamin, This patch factorizes the hid set_feature command by using hid_device-hid_output_raw_report instead of direclty relying on usbhid. This makes the driver usb independant. However I still can't remove the 2 usb related headers because the function mt_resume has a specific patch for usb devices. Signed-off-by: Benjamin Tissoires benjamin.tissoi...@gmail.com --- drivers/hid/hid-multitouch.c | 63 ++-- 1 file changed, 37 insertions(+), 26 deletions(-) In my drawer, I have a patchset that aims to remove all usbhid dependence, from all the drivers. Perhaps the attached patch is something to consider here? yep, removing usbhid dependencies is a good thing. See my review below :) I have a tentative patch taking your comments into account, and it is likely that we want to go that way. However, as to not hold up your patchset, perhaps we could do without it for now. so, Yes, this is not my blocking patch that prevents me from sending the new patchset. I intend to let you clean this up with your new patch. The v3 is on it's way! Regarding the hardwired usbhid dependency, I think the solution is to move that code to usbhid itself. yes, maybe, but at the beginning, we didn't want to patch it that way because it was only specific to one hid-multitouch device (though armless for the other multitouch devices). So maybe you will have to add a quirk in usbhid or sth like that. Cheers, Benjamin Thanks, Henrik -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] i2c-hid: introduce HID over i2c specification implementation
Hi Jiri, On Fri, Nov 9, 2012 at 4:49 AM, Jiri Kosina jkos...@suse.cz wrote: On Tue, 16 Oct 2012, Benjamin Tissoires wrote: --- /dev/null +++ b/drivers/hid/i2c-hid/i2c-hid.c ... +static int i2c_hid_alloc_buffers(struct i2c_hid *ihid) +{ + /* the worst case is computed from the set_report command with a + * reportID 15 and the maximum report length */ + int args_len = sizeof(__u8) + /* optional ReportID byte */ +sizeof(__u16) + /* data register */ +sizeof(__u16) + /* size of the report */ +ihid-bufsize; /* report */ + + ihid-inbuf = kzalloc(ihid-bufsize, GFP_KERNEL); + + if (!ihid-inbuf) + return -ENOMEM; + + ihid-argsbuf = kzalloc(args_len, GFP_KERNEL); + + if (!ihid-argsbuf) { + kfree(ihid-inbuf); + return -ENOMEM; + } + + ihid-cmdbuf = kzalloc(sizeof(union command) + args_len, GFP_KERNEL); + + if (!ihid-cmdbuf) { + kfree(ihid-inbuf); + kfree(ihid-argsbuf); + return -ENOMEM; + } + + return 0; If this is called from hid_start and some of the latter allocation fails here, you free the buffers appropriately. However if another start occurs (e.g. by loading another module for that particular device), it will crash, as the buffers will remain unallocated because at this point ihid-bufsize == old_bufsize. You should set ihid-bufsize back to old_bufsize if i2c_hid_alloc_buffers fails and also set the pointers to NULL here. well spotted, thanks. I'll change it in v3 then. Benjamin, just a quick check -- are you planning on submitting v3 eventually? yes sure. It was not on my top priority list since I started at Red Hat. Moreover, I've been reported a bug yesterday with the set_report command. I'll need to do a few more tests before sending the v3. If I send it by the end of the day or at the beginning of next week, will it have a chance to land into 3.8? Cheers, Benjamin Thanks, -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 00/13] Win 8 support for digitizers
Hi Jiri, On Tue, Nov 13, 2012 at 8:47 AM, Jiri Kosina jkos...@suse.cz wrote: On Wed, 7 Nov 2012, Benjamin Tissoires wrote: Hi Guys, here is the third version of this patchset. I think I included most of Henrik's comments. Happy reviewing :) Cheers, Benjamin v1 introduction: So, this is an update for supporting Win 8 multitouch devices in the kernel. As I wanted to reliably forward the resolution, I noticed a bug in the processing of the unit_exponent in the hid core layer. Thus the fixes for hid-core and hid-input. v2 changes: * added missing initial patch that prevents the series to be applied on top of Jiri's tree * update to include latest hid changes * taken into account Alan's patch: hid: put the case in the right switch statement v3 changes: * splitted round return value of hidinput_calc_abs_res in a separate patch * export snto32 in hid.h as we need to use it in hid-input.c * didn't change all drivers, but add a field in hid_usage instead * add quirk MT_QUIRK_IGNORE_DUPLICATES so that any device can rely on it * easier understandable support of hovering devices * changed scan time definition * applied new definition of scan time in hid-multitouch * some other few things. Benjamin, thanks for the patchset. I am fine with the HID-specific changes (and I have sent out my Acks to the invidivual patches). Thanks :) For hid-multitouch changes, I'd like to have Reviewed-by: from Henrik as well. Henrik, are you planning to review this patchset, please? Also the note from Dmitry on turning ABS_SCAN_TIME into EV_MSC/MSC_TIMESTAMP should be addressed. Yes, sure. I'll do it today. Once this is finalized, I'd like to queue this for 3.8 in my tree. Thanks, Benjamin Thanks, -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 11/13] HID: hid-multitouch: support for hovering devices
Hi Henrik, thanks for the review of the patchset. I'll do my best for the next version :) On Tue, Nov 13, 2012 at 5:42 PM, Henrik Rydberg rydb...@euromail.se wrote: On Wed, Nov 07, 2012 at 05:37:34PM +0100, Benjamin Tissoires wrote: Win8 devices supporting hovering must provides InRange HID field. provide the The information that the finger is here but is not touching the surface is sent to the user space through ABS_MT_DISTANCE as required by the multitouch protocol. In the absence of more detailed information, use ABS_MT_DISTANCE with a [0,1] interval to distinguish between presence (1) and touch (0). Signed-off-by: Benjamin Tissoires benjamin.tissoi...@gmail.com --- drivers/hid/hid-multitouch.c | 24 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index b393c6c..1f3d1e0 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -59,6 +59,7 @@ struct mt_slot { __s32 x, y, cx, cy, p, w, h; __s32 contactid;/* the device ContactID assigned to this slot */ bool touch_state; /* is the touch valid? */ + bool inrange_state; /* is the finger in proximity of the sensor? */ }; struct mt_class { @@ -397,6 +398,12 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi, case HID_UP_DIGITIZER: switch (usage-hid) { case HID_DG_INRANGE: + if (cls-quirks MT_QUIRK_WIN_8_CERTIFIED) { + hid_map_usage(hi, usage, bit, max, + EV_ABS, ABS_MT_DISTANCE); + input_set_abs_params(hi-input, + ABS_MT_DISTANCE, -1, 1, 0, 0); Why [-1,1] here? At first, I only used [0,1]. However, it's still the same problem with the information being kept between the touches: if you start an application after having touched your device, you normally have to ask for the different per-touche states to get x, y, distance, pressure, etc... However, this is not much mandatory because the protocol in its current form ensures that you will get the new states of the axes when a new touch occurs. ABS_MT_DISTANCE is a little bit different here because the protocol guarantees that once you get the not in range state through tracking_id == -1, distance should be 1. When the new touch of the very same slot occurs, you also have the guarantee that distance is at 1 too. So basically, if you don't ask for the slot states, you will never get that very first distance. I know that it's a user-space problem, but honestly, I don't want to fix several user-space problems when we could fix it through the protocol. I can see 2 solutions: - set the range to: [0,1] and still sending -1 (or 2) for the invalid distance value (if it's not clamped) - set the range to: [0,2] and having: 0 for touch, 1 for hovering, and 2 for not in range Does that make sense? + } mt_store_field(usage, td, hi); td-last_field_index = field-index; return 1; @@ -516,6 +523,10 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input) if (slotnum 0 || slotnum = td-maxcontacts) return; + if (!test_bit(ABS_MT_DISTANCE, input-absbit)) + /* If InRange is not present, rely on TipSwitch */ + s-inrange_state = s-touch_state; + This could be skipped, see below. if (td-mtclass.quirks MT_QUIRK_IGNORE_DUPLICATES input_mt_is_active(input-mt-slots[slotnum]) input_mt_is_used(input-mt, input-mt-slots[slotnum])) @@ -523,9 +534,9 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input) input_mt_slot(input, slotnum); input_mt_report_slot_state(input, MT_TOOL_FINGER, - s-touch_state); - if (s-touch_state) { - /* this finger is on the screen */ + s-inrange_state); + if (s-inrange_state) { + /* this finger is in proximity of the sensor */ Using (s-touch_state || s-inrange_state) here seems simpler. agree. int wide = (s-w s-h); /* divided by two to match visual scale of touch */ int major = max(s-w, s-h) 1; @@ -535,11 +546,14 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input) input_event(input, EV_ABS, ABS_MT_POSITION_Y, s-y); input_event(input, EV_ABS, ABS_MT_TOOL_X, s-cx); input_event(input, EV_ABS, ABS_MT_TOOL_Y, s-cy); + input_event(input, EV_ABS, ABS_MT_DISTANCE
Re: [PATCH v3 13/13] HID: hid-multitouch: forwards ABS_SCAN_TIME
Hi Henrik, On Tue, Nov 13, 2012 at 6:27 PM, Henrik Rydberg rydb...@euromail.se wrote: Hi Benjamin, From: Benjamin Tissoires benjamin.tissoi...@gmail.com Date: Tue, 13 Nov 2012 15:12:17 +0100 Subject: [PATCH v4] HID: hid-multitouch: forwards MSC_TIMESTAMP Computes the device timestamp according to the specification. It also ensures that if the time between two events is greater than MAX_TIMESTAMP_INTERVAL, the timestamp will be reset. Signed-off-by: Benjamin Tissoires benjamin.tissoi...@gmail.com --- drivers/hid/hid-multitouch.c | 46 +--- include/linux/hid.h | 1 + 2 files changed, 44 insertions(+), 3 deletions(-) diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index caf0f0b..3f8432d 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -32,6 +32,7 @@ #include linux/slab.h #include linux/usb.h #include linux/input/mt.h +#include linux/jiffies.h Why? #include usbhid/usbhid.h @@ -98,6 +99,9 @@ struct mt_device { bool serial_maybe; /* need to check for serial protocol */ bool curvalid; /* is the current contact valid? */ unsigned mt_flags; /* flags to pass to input-mt */ + __s32 dev_time; /* the scan time provided by the device */ + unsigned long jiffies; /* the frame's jiffies */ + unsigned timestamp; /* the timestamp to be sent */ Why not just dev_time here? because max dev_time is at least 65535 according to the norm, and the win 8 device I have has his max value of 65535. Which means that every 6 seconds and a half the counter resets, and the value is not properly reset in the way I understand the specification. The device just forwards an internal clock that is never reset. So if you wait 653500 us + 8ms, everything happens as if the device sent this frame right after the previous one (you get the same value). I think that it's the job of the kernel to provide clean and coherent values through evdev, which won't be the case if the jiffies thing is not in place: every devices will have a different behavior, leading to complicate things in the user-space. }; /* classes of device behavior */ @@ -126,6 +130,8 @@ struct mt_device { #define MT_DEFAULT_MAXCONTACT10 #define MT_MAX_MAXCONTACT250 +#define MAX_TIMESTAMP_INTERVAL 50 + Needs to be done in userland anyways, so no need to check this in the kernel. #define MT_USB_DEVICE(v, p) HID_DEVICE(BUS_USB, HID_GROUP_MULTITOUCH, v, p) #define MT_BT_DEVICE(v, p) HID_DEVICE(BUS_BLUETOOTH, HID_GROUP_MULTITOUCH, v, p) @@ -451,12 +457,19 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi, mt_store_field(usage, td, hi); td-last_field_index = field-index; return 1; + case HID_DG_SCANTIME: + hid_map_usage(hi, usage, bit, max, + EV_MSC, MSC_TIMESTAMP); + set_bit(MSC_TIMESTAMP, hi-input-mscbit); + td-last_field_index = field-index; + return 1; case HID_DG_CONTACTCOUNT: td-last_field_index = field-index; return 1; case HID_DG_CONTACTMAX: - /* we don't set td-last_slot_field as contactcount and - * contact max are global to the report */ + /* we don't set td-last_slot_field as scan time, + * contactcount and contact max are global to the + * report */ td-last_field_index = field-index; return -1; } @@ -485,7 +498,8 @@ static int mt_input_mapped(struct hid_device *hdev, struct hid_input *hi, struct hid_field *field, struct hid_usage *usage, unsigned long **bit, int *max) { - if (usage-type == EV_KEY || usage-type == EV_ABS) + if (usage-type == EV_KEY || usage-type == EV_ABS || + usage-type == EV_MSC) set_bit(usage-type, hi-input-evbit); return -1; @@ -565,11 +579,34 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input) */ static void mt_sync_frame(struct mt_device *td, struct input_dev *input) { + input_event(input, EV_MSC, MSC_TIMESTAMP, td-timestamp); I think this should go after the frame sync, input_mt_sync_frame(input); And the computation (100 * td-dev_time) should work fine. It will wrap properly. input_sync(input); td-num_received = 0; } +static void mt_compute_timestamp(struct mt_device *td, struct hid_field *field, + __s32 value) +{ + long delta = value - td-dev_time; + unsigned long jdelta = jiffies_to_usecs(jiffies - td-jiffies); + + td-jiffies = jiffies; + td
Re: [PATCH v3 00/13] Win 8 support for digitizers
Hi Henrik, On Tue, Nov 13, 2012 at 6:33 PM, Henrik Rydberg rydb...@euromail.se wrote: Hi Benjamin, here is the third version of this patchset. I think I included most of Henrik's comments. Happy reviewing :) thanks for the changes :-) and thanks for the review. Jiri, I am fine with the HID-specific changes (and I have sent out my Acks to the invidivual patches). Looks reasonable to me too (although I have not looked through the exponent code enough to want to formally ack it). For hid-multitouch changes, I'd like to have Reviewed-by: from Henrik as well. Henrik, are you planning to review this patchset, please? Yes, and finally done. Cheers, Henrik Jiri, I'll send a new patchset tomorrow with the different acked, reviewed and requested changes. Some patches at the end may need an other review, so I'll make sure to get the reviewed patches at the beginning of the set so that you can pick them for 3.8. It will also allow me not to send dozens of patches in a row :) Is it good for you? Cheers, Benjamin -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 11/13] HID: hid-multitouch: support for hovering devices
On Tue, Nov 13, 2012 at 7:04 PM, Henrik Rydberg rydb...@euromail.se wrote: Hi Benjamin, Why [-1,1] here? At first, I only used [0,1]. However, it's still the same problem with the information being kept between the touches: if you start an application after having touched your device, you normally have to ask for the different per-touche states to get x, y, distance, pressure, etc... However, this is not much mandatory because the protocol in its current form ensures that you will get the new states of the axes when a new touch occurs. Right, the stateful communication requires a full state read after opening the deivce, although in most cases this is not really necessary. This is no different for ABS_MT_DISTANCE, of course. ABS_MT_DISTANCE is a little bit different here because the protocol guarantees that once you get the not in range state through tracking_id == -1, distance should be 1. When the new touch of the very same slot occurs, you also have the guarantee that distance is at 1 too. ABS_MT_DISTANCE is just another attribute of the slot, so it really is no different. So basically, if you don't ask for the slot states, you will never get that very first distance. Which will not be important either; as long as the slot is unused, it does not matter what the attributes of that slot are. And if the slot is unused, but has been used since the plug of the device, the state is kept between the touch. I know that it's a user-space problem, but honestly, I don't want to fix several user-space problems when we could fix it through the protocol. I can see 2 solutions: - set the range to: [0,1] and still sending -1 (or 2) for the invalid distance value (if it's not clamped) - set the range to: [0,2] and having: 0 for touch, 1 for hovering, and 2 for not in range Does that make sense? I just do not see what problem you want to solve here. I just intend to force the update of the distance value at the beginning of the touch. This is just to send a coherent state when the finger goes in range, and to make sure that user-space application do not rely on the undefined value (0) whereas the kernel thought it was set to 1. Cheers, Benjamin Thanks, Henrik -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] HID: usbhid: add quirk HID_QUIRK_NOGET to TPV optical touchscreen
Without this, the device is blocked in dmesg at: hid-multitouch 0003:25AA:8883.000X: usb_submit_urb(ctrl) failed: -1 hid-multitouch 0003:25AA:8883.000X: timeout initializing reports Signed-off-by: Benjamin Tissoires benjamin.tissoi...@gmail.com --- Hi guys, Even if Chris did not answered, I had the confirmation that this device works with the quirk in place. Cheers, Benjamin drivers/hid/hid-ids.h | 3 +++ drivers/hid/usbhid/hid-quirks.c | 1 + 2 files changed, 4 insertions(+) diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h index 30bc2db..56d5528 100644 --- a/drivers/hid/hid-ids.h +++ b/drivers/hid/hid-ids.h @@ -765,6 +765,9 @@ #define USB_VENDOR_ID_TOUCHPACK0x1bfd #define USB_DEVICE_ID_TOUCHPACK_RTS0x1688 +#define USB_VENDOR_ID_TPV 0x25aa +#define USB_DEVICE_ID_TPV_OPTICAL_TOUCHSCREEN 0x8883 + #define USB_VENDOR_ID_TURBOX 0x062a #define USB_DEVICE_ID_TURBOX_KEYBOARD 0x0201 #define USB_DEVICE_ID_TURBOX_TOUCHSCREEN_MOSART0x7100 diff --git a/drivers/hid/usbhid/hid-quirks.c b/drivers/hid/usbhid/hid-quirks.c index 11c7932..38a7cdd 100644 --- a/drivers/hid/usbhid/hid-quirks.c +++ b/drivers/hid/usbhid/hid-quirks.c @@ -82,6 +82,7 @@ static const struct hid_blacklist { { USB_VENDOR_ID_SUN, USB_DEVICE_ID_RARITAN_KVM_DONGLE, HID_QUIRK_NOGET }, { USB_VENDOR_ID_SYMBOL, USB_DEVICE_ID_SYMBOL_SCANNER_1, HID_QUIRK_NOGET }, { USB_VENDOR_ID_SYMBOL, USB_DEVICE_ID_SYMBOL_SCANNER_2, HID_QUIRK_NOGET }, + { USB_VENDOR_ID_TPV, USB_DEVICE_ID_TPV_OPTICAL_TOUCHSCREEN, HID_QUIRK_NOGET }, { USB_VENDOR_ID_TURBOX, USB_DEVICE_ID_TURBOX_KEYBOARD, HID_QUIRK_NOGET }, { USB_VENDOR_ID_UCLOGIC, USB_DEVICE_ID_UCLOGIC_TABLET_PF1209, HID_QUIRK_MULTI_INPUT }, { USB_VENDOR_ID_UCLOGIC, USB_DEVICE_ID_UCLOGIC_TABLET_WP4030U, HID_QUIRK_MULTI_INPUT }, -- 1.8.0 -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 14/14] HID: hid-multitouch: forwards MSC_TIMESTAMP
Hi Henrik, On 11/14/2012 08:58 PM, Henrik Rydberg wrote: Hi Benjamin, Computes the device timestamp according to the specification. It also ensures that if the time between two events is greater than MAX_TIMESTAMP_INTERVAL, the timestamp will be reset. Signed-off-by: Benjamin Tissoires benjamin.tissoi...@gmail.com --- This was not what I envisioned from the discussion yesterday, I was probably too vague. Firstly, the absolute time interval checked seems to be 500 ms, which is arbitrary. Second, the wrap should probably use Not exactly. The time interval was 5s, near enough the minimum requirement for this field (6.5535 s). (logical_maximum + 1). Third, we are still not sure whether we should take the 'time = 0 means reset' logic literally, I think it needs to be tested. Again, the device I have never does any reset at the first touch, it just wraps the counter. The problem is that when there is no event, we now that no events occurred since the previous timestamp, modulus 6.5535 seconds. In light of this, I would like to suggest the patch below instead. It should follow the current interpretation of the definition to the letter. I imagine very few devices will actually do anything but wrap the counter, but that remains to be seen. In that case, we could simplify the code further, since we will have no hope of detecting large intervals properly anyways. Thanks, Henrik From 448808dd988e96d58b79148292fde147935305ab Mon Sep 17 00:00:00 2001 From: Henrik Rydberg rydb...@euromail.se Date: Wed, 14 Nov 2012 20:53:17 +0100 Subject: [PATCH] HID: hid-multitouch: send the device timestamp when present Compute and relay the device timestamp when present. The counter value range varies between devices, but the MSC_TIMESTAMP event is always 32 bits long. A value of zero means the time since the previous event is unknown. Signed-off-by: Henrik Rydberg rydb...@euromail.se --- drivers/hid/hid-multitouch.c | 36 +--- include/linux/hid.h | 1 + 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index 61543c0..586f9c6 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -98,6 +98,8 @@ struct mt_device { bool serial_maybe; /* need to check for serial protocol */ bool curvalid; /* is the current contact valid? */ unsigned mt_flags; /* flags to pass to input-mt */ + unsigned dev_time; /* device scan time in units of 100 us */ + unsigned timestamp; /* the timestamp to be sent */ }; /* classes of device behavior */ @@ -458,12 +460,19 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi, mt_store_field(usage, td, hi); td-last_field_index = field-index; return 1; + case HID_DG_SCANTIME: + hid_map_usage(hi, usage, bit, max, + EV_MSC, MSC_TIMESTAMP); + set_bit(MSC_TIMESTAMP, hi-input-mscbit); + td-last_field_index = field-index; + return 1; case HID_DG_CONTACTCOUNT: td-last_field_index = field-index; return 1; case HID_DG_CONTACTMAX: - /* we don't set td-last_slot_field as contactcount and - * contact max are global to the report */ + /* we don't set td-last_slot_field as scan time, + * contactcount and contact max are global to the + * report */ td-last_field_index = field-index; return -1; case HID_DG_TOUCH: @@ -492,7 +501,8 @@ static int mt_input_mapped(struct hid_device *hdev, struct hid_input *hi, struct hid_field *field, struct hid_usage *usage, unsigned long **bit, int *max) { - if (usage-type == EV_KEY || usage-type == EV_ABS) + if (usage-type == EV_KEY || usage-type == EV_ABS || + usage-type == EV_MSC) set_bit(usage-type, hi-input-evbit); return -1; @@ -571,10 +581,27 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input) static void mt_sync_frame(struct mt_device *td, struct input_dev *input) { input_mt_sync_frame(input); + input_event(input, EV_MSC, MSC_TIMESTAMP, td-timestamp); input_sync(input); td-num_received = 0; } +static void mt_compute_timestamp(struct mt_device *td, struct hid_field *field, + unsigned value) +{ + unsigned dt; + + if (value) { + dt = (value - td-dev_time) % (field-logical_maximum + 1); The curious thing is that this is not working on my kernel: this is the output
Re: [PATCH v4 00/14] Win 8 support for digitizers
On Thu, Nov 15, 2012 at 10:33 AM, Jiri Kosina jkos...@suse.cz wrote: On Wed, 14 Nov 2012, Henrik Rydberg wrote: * patches 1-9 has already been reviewed and are ready for inclusion I would say. * Jiri, I kept your ack on patch 4 even if I changed the place of the comment in hid.h * patch 10 is half new as it is splitted from a patch of the v3 * patch 11 has been changed according to Henrik's comments * patches 12-13 have been splitted since v3 to introduce QUIRK_HOVERING and setup WIN 8 devices in a better way. * patch 14 has been copied from the v3 as Dmitry wanted to use a MSC event for the timestamp. Everything but the last patch looks good now, me thinks. Thanks! Hi Henrik, Thanks for the review! I have now applied everything (with all the gathered Acks/Reviewed-by incorporated) but the patch #14. Hi Jiri, many thanks for applying it. Patch #14 is less critical, so we can take more time to include it. Cheers, Benjamin Thanks, -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] i2c-hid: introduce HID over i2c specification implementation
On Thu, Nov 15, 2012 at 3:04 PM, Benjamin Tissoires benjamin.tissoi...@gmail.com wrote: On Thu, Nov 15, 2012 at 2:51 PM, Jiri Kosina jkos...@suse.cz wrote: On Mon, 12 Nov 2012, Benjamin Tissoires wrote: Microsoft published the protocol specification of HID over i2c: http://msdn.microsoft.com/en-us/library/windows/hardware/hh852380.aspx This patch introduces an implementation of this protocol. This implementation does not includes the ACPI part of the specification. This will come when ACPI 5.0 devices enumeration will be available. Once the ACPI part is done, OEM will not have to declare HID over I2C devices in their platform specific driver. Signed-off-by: Benjamin Tissoires benjamin.tissoi...@gmail.com Out of curiosity -- has this been tested on a real device (is there any such device available anyway?), or is that just the implementation of the defined protocol? It has been tested on an ELAN microelectronics device (a prototype), on an odroid-x board. That's how we figure out the bug in the set_report command. I think we manage to test all main features of the protocol (get_report, irqs, hid descriptor, report descriptors, set_report). I'm currently waiting for a Synaptics touchpad to check if it's also working with their devices. The thing is that HID over i2c for x86 platform will presumably require the Haswell platform from Intel (we need ACPI 5 for enumeration), but it would be very nice to get this in the kernel just before hardware arrive on the market :) However, I won't be surprise if android OEMs also start using this specification because it won't force them to write kernel drivers... And as a complement, Ramalingam tested it for Nvidia on an early NVIDIA's Tegra reference board for PISMO which is registered at http://www.arm.linux.org.uk/developer/machines/list.php?id=4439 , with a HID over i2c keyboard. He is up to test also his Synaptics touchpad. Cheers, Benjamin Cheers, Benjamin Thanks, -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 14/14] HID: hid-multitouch: forwards MSC_TIMESTAMP
Hi Henrik, On Fri, Nov 16, 2012 at 9:09 PM, Henrik Rydberg rydb...@euromail.se wrote: Hi Benjamin, This was not what I envisioned from the discussion yesterday, I was probably too vague. Firstly, the absolute time interval checked seems to be 500 ms, which is arbitrary. Second, the wrap should probably use Not exactly. The time interval was 5s, near enough the minimum requirement for this field (6.5535 s). Sorry about that, but the argument remains; it should depend on logical_maximum. I must be tired when I read your mail, I only understand now what you meant by logical_maximum in your first mail... sorry. So, totally agree, we could make this kind of thing depending on logical_max. (logical_maximum + 1). Third, we are still not sure whether we should take the 'time = 0 means reset' logic literally, I think it needs to be tested. Again, the device I have never does any reset at the first touch, it just wraps the counter. The problem is that when there is no event, we now that no events occurred since the previous timestamp, modulus 6.5535 seconds. So the very first win8 device we test already diverges from the win8 specification, how nice. hehe... well, the scan time is not as critical as can be the other fields. Anyway, it's fun :) Ok, you have conviced me that comparing to a proper time makes sense. However, I am not happy about using a separate time for this, given that we will fill in the event time later in the chain. that may explains the delta I observed from the kernel time and the device time. In addition, it would make perfect sense to extend the validity of the hardware time with the event time for longer intervals. The relative error only makes a difference on milisecond intervals. A patch that seamlessly extends the validity of the hardware time, ideally using the event time, seems like a viable solution. Just to be sure: When I receive scan_time, I increment timestamp with the device value. When not, I find out the number of counter wrap I missed with the kernel timer (jiffies) to get the actual device time. Is that ok for you? +static void mt_compute_timestamp(struct mt_device *td, struct hid_field *field, +unsigned value) +{ + unsigned dt; + + if (value) { + dt = (value - td-dev_time) % (field-logical_maximum + 1); The curious thing is that this is not working on my kernel: Oups, I suspect (value - td-dev_time) needs to be explicitly converted to unsigned. I can't remember if I tried this one, but I'll try to make it one line in the next version. I don't understand the meaning of adding !td-timestamp. :/ With the definition that timestamp == 0 means an unknown interval, we do not want to send that value by accident. ok, makes sense. Cheers, Benjamin Thanks, Henrik -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Input: add appleir USB driver
Hi Bastien, (adding the input and HID maintainers to the recipient list). On Thu, Nov 15, 2012 at 7:13 PM, Bastien Nocera had...@hadess.net wrote: This driver was originally written by James McKenzie, updated by Greg Kroah-Hartman, further updated by myself, with suspend support added. More recent versions of the IR receiver are also supported through a patch by Alex Karpenko. The patch also adds support for the 2nd and 5th generation of the controller, and the menu key on newer brushed metal remotes. Tested on a MacbookAir1,1 Signed-off-by: Bastien Nocera had...@hadess.net --- Resend, as the original patch never made it. I cleaned up the patch a bit further, and test compiled it, but didn't have a chance to test it as I don't have a machine with that hardware available anymore. Fabien, in CC, gracefully accepted to test and to try to adapt this patch depending on the reviews. So we can ask for tests and changes! Documentation/input/appleir.txt | 46 drivers/hid/hid-apple.c | 4 - drivers/hid/hid-core.c | 7 +- drivers/hid/hid-ids.h | 5 +- drivers/input/misc/Kconfig | 13 + drivers/input/misc/Makefile | 1 + drivers/input/misc/appleir.c| 527 If this device presents itself as a hid device, there are much chances that we can use the hid .raw_event interface. This will help us for the usb part and remove potential bugs and support and greatly simplify this driver. 7 files changed, 596 insertions(+), 7 deletions(-) create mode 100644 Documentation/input/appleir.txt create mode 100644 drivers/input/misc/appleir.c diff --git a/Documentation/input/appleir.txt b/Documentation/input/appleir.txt new file mode 100644 index 000..db637fb --- /dev/null +++ b/Documentation/input/appleir.txt @@ -0,0 +1,46 @@ +Apple IR receiver Driver (appleir) +-- + Copyright (C) 2009 Bastien Nocera had...@hadess.net + +The appleir driver is a kernel input driver to handle Apple's IR +receivers (and associated remotes) in the kernel. + +The driver is an input driver which only handles official remotes +as built and sold by Apple. + +Authors +--- + +James McKenzie (original driver) +Alex Karpenko (05ac:8242 support) +Greg Kroah-Hartman (cleanups and original submission) +Bastien Nocera (further cleanups, brushed metal enter +button support and suspend support) + +Supported hardware +-- + +- All Apple laptops and desktops from 2005 onwards, except: + - the unibody Macbook (2009) + - Mac Pro (all versions) +- Apple TV (all revisions prior to September 2010) + +The remote will only support the 6 (old white) or 7 (brushed metal) buttons +of the remotes as sold by Apple. See the next section if you want to use +other remotes or want to use lirc with the device instead of the kernel driver. + +Using lirc (native) instead of the kernel driver + + +First, you will need to disable the kernel driver for the receiver. + +This can be achieved by passing quirks to the usbhid driver. +The quirk line would be: +usbhid.quirks=0x05ac:0x8242:0x4010 + +With 0x05ac being the vendor ID (Apple, you shouldn't need to change this) +With 0x8242 being the product ID (check the output of lsusb for your hardware) +And 0x10 being HID_QUIRK_HIDDEV_FORCE and 0x4000 being HID_QUIRK_NO_IGNORE + +This should force the creation of a hiddev device for the receiver, and +make it usable under lirc. diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c index fd7722a..30a4824 100644 --- a/drivers/hid/hid-apple.c +++ b/drivers/hid/hid-apple.c @@ -390,10 +390,6 @@ static void apple_remove(struct hid_device *hdev) } static const struct hid_device_id apple_devices[] = { - { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_ATV_IRCONTROL), - .driver_data = APPLE_HIDDEV | APPLE_IGNORE_HIDINPUT }, - { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IRCONTROL4), - .driver_data = APPLE_HIDDEV | APPLE_IGNORE_HIDINPUT }, { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MIGHTYMOUSE), .driver_data = APPLE_MIGHTYMOUSE | APPLE_INVERT_HWHEEL }, diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index f4109fd..3fd4c10 100644 --- a/drivers/hid/hid-core.c +++ b/drivers/hid/hid-core.c @@ -1471,8 +1471,6 @@ static const struct hid_device_id hid_have_special_driver[] = { { HID_USB_DEVICE(USB_VENDOR_ID_A4TECH, USB_DEVICE_ID_A4TECH_X5_005D) }, { HID_USB_DEVICE(USB_VENDOR_ID_A4TECH, USB_DEVICE_ID_A4TECH_RP_649) }, { HID_USB_DEVICE(USB_VENDOR_ID_ACRUX, 0x0802) }, - { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_ATV_IRCONTROL) }, - { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IRCONTROL4) },
Re: [PATCH] Input: add appleir USB driver
On Mon, Nov 19, 2012 at 4:44 PM, Bastien Nocera had...@hadess.net wrote: On Mon, 2012-11-19 at 16:32 +0100, Benjamin Tissoires wrote: Hi Bastien, (adding the input and HID maintainers to the recipient list). On Thu, Nov 15, 2012 at 7:13 PM, Bastien Nocera had...@hadess.net wrote: This driver was originally written by James McKenzie, updated by Greg Kroah-Hartman, further updated by myself, with suspend support added. More recent versions of the IR receiver are also supported through a patch by Alex Karpenko. The patch also adds support for the 2nd and 5th generation of the controller, and the menu key on newer brushed metal remotes. Tested on a MacbookAir1,1 Signed-off-by: Bastien Nocera had...@hadess.net --- Resend, as the original patch never made it. I cleaned up the patch a bit further, and test compiled it, but didn't have a chance to test it as I don't have a machine with that hardware available anymore. Fabien, in CC, gracefully accepted to test and to try to adapt this patch depending on the reviews. So we can ask for tests and changes! \o/ diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h index 9d7a428..a4af9a9 100644 --- a/drivers/hid/hid-ids.h +++ b/drivers/hid/hid-ids.h @@ -137,8 +137,11 @@ #define USB_DEVICE_ID_APPLE_ALU_WIRELESS_2011_ISO 0x0256 #define USB_DEVICE_ID_APPLE_FOUNTAIN_TP_ONLY 0x030a #define USB_DEVICE_ID_APPLE_GEYSER1_TP_ONLY0x030b -#define USB_DEVICE_ID_APPLE_ATV_IRCONTROL 0x8241 not sure we should change this define to an undocumented one. I don't understand the comment here. The name is an artifact of where the receiver was first seen and the Apple TV receiver is actually just another model of this same receiver. So it makes sense to consolidate below. ok, if this id is not specific to Apple TV, that makes sense. +#define USB_DEVICE_ID_APPLE_IRCONTROL 0x8240 +#define USB_DEVICE_ID_APPLE_IRCONTROL2 0x1440 +#define USB_DEVICE_ID_APPLE_IRCONTROL3 0x8241 #define USB_DEVICE_ID_APPLE_IRCONTROL4 0x8242 +#define USB_DEVICE_ID_APPLE_IRCONTROL5 0x8243 +struct appleir { + struct input_dev *input_dev; + unsigned short keymap[ARRAY_SIZE(appleir_key_table)]; why this keymap is embedded in the struct? It's basically just a copy of appleir_key_table and it's not modified anytime. It would be modified if you change the keymap. ouch, sorry, I read it too fast. You're perfectly right. That makes me wondering if this will still be possible with a hid driver that implements raw_event... Cheers, Benjamin Cheers -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: hid-multitouch: eGalax Touchscreen not resuming after suspend
Hi Jan-Matthias, On Mon, Nov 19, 2012 at 12:58 PM, Jan-Matthias Braun jan_br...@gmx.net wrote: Dear List, using the current (Linux v2.6.7) hid-multitouch driver I have the problem, that the touchscreen works fine after a fresh boot, but after a suspend the touchscreen does not come back to live and I am asking for assistance to get this working. As I can reproduce this problem on a standard tty device without X (see below) I am suspecting a driver problem, but I might as well be wrong. I assume you are talking about v3.6.7... The device in question is the Touchscreen of a Dell Inspron Duo convertable, a USB device with id 0eef:725e (D-WAV Scientific Co., Ltd), found and registered as an input device by the kernel as [6.931638] usb 4-1: Manufacturer: eGalax Inc. [ 16.186272] input: eGalax Inc. USB TouchController as /devices/pci:00/:00:1d.2/usb4/4-1/4-1:1.0/input/input10 [ 16.187162] hid-multitouch 0003:0EEF:725E.0001: input,hiddev0,hidraw0: USB HID v2.10 Pointer [eGalax Inc. USB TouchController] on usb-:00:1d.2-1/input0 I have tested the behaviour with and without X11. Without X11 I was using a standard tty and using cat on the input device. After boot the input device gives lot of output while touching the screen, after resume the device stays silent. strange. I have tested the procedure with the eGalax 0x72FA I got, and I'm not seeing this problem on a 3.6 kernel. Is the config symbol CONFIG_PM set to y in your .config file? Also, can you try to rmmod / modprobe hid-multitouch when the device is not responding and see if this solves things. I have this problem since moving to hid-multitouch for handling this device. What kind of driver did you use before? Cheers, Benjamin I will be happy to test and supply more information. Thanks a lot for looking into this, Jan-Matthias Braun PS: As I am not subscribed to the mailing list, please CC me. -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] HID: i2c-hid: fix memory leak during probe
On Tue, Nov 20, 2012 at 5:09 PM, Jiri Kosina jkos...@suse.cz wrote: In case we are returning from i2c_hid_probe() through the 'err' or err_mem_free labels, there is noone freeing the buffers allocated by i2c_hid_alloc_buffers(). ouch... thanks Jiri I hope there are not so much others like this one :) Reviewed-by: Benjamin Tissoires benjamin.tissoi...@gmail.com Signed-off-by: Jiri Kosina jkos...@suse.cz --- drivers/hid/i2c-hid/i2c-hid.c |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c index 11140bd..67ab5b7 100644 --- a/drivers/hid/i2c-hid/i2c-hid.c +++ b/drivers/hid/i2c-hid/i2c-hid.c @@ -892,6 +892,7 @@ err: if (ihid-irq) free_irq(ihid-irq, ihid); + i2c_hid_free_buffers(ihid); kfree(ihid); return ret; } -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: hid-multitouch: eGalax Touchscreen not resuming after suspend
Hi Jan-Matthias, On 11/21/2012 10:33 AM, Jan-Matthias Braun wrote: Hi Benjamin, thanks for looking into the issue. Am Dienstag, 20. November 2012, 13:47:23 schrieben Sie: Hi Jan-Matthias, On Mon, Nov 19, 2012 at 12:58 PM, Jan-Matthias Braun jan_br...@gmx.net wrote: Dear List, using the current (Linux v2.6.7) hid-multitouch driver I have the problem, that the touchscreen works fine after a fresh boot, but after a suspend the touchscreen does not come back to live and I am asking for assistance to get this working. As I can reproduce this problem on a standard tty device without X (see below) I am suspecting a driver problem, but I might as well be wrong. I assume you are talking about v3.6.7... You are right, of course. Sorry for the distraction. The device in question is the Touchscreen of a Dell Inspron Duo convertable, a USB device with id 0eef:725e (D-WAV Scientific Co., Ltd), found and registered as an input device by the kernel as [6.931638] usb 4-1: Manufacturer: eGalax Inc. [ 16.186272] input: eGalax Inc. USB TouchController as /devices/pci:00/:00:1d.2/usb4/4-1/4-1:1.0/input/input10 [ 16.187162] hid-multitouch 0003:0EEF:725E.0001: input,hiddev0,hidraw0: USB HID v2.10 Pointer [eGalax Inc. USB TouchController] on usb-:00:1d.2-1/input0 I have tested the behaviour with and without X11. Without X11 I was using a standard tty and using cat on the input device. After boot the input device gives lot of output while touching the screen, after resume the device stays silent. strange. I have tested the procedure with the eGalax 0x72FA I got, and I'm not seeing this problem on a 3.6 kernel. Is the config symbol CONFIG_PM set to y in your .config file? Yes. Also, can you try to rmmod / modprobe hid-multitouch when the device is not responding and see if this solves things. This is not working for me. On modprobe the Kernel log says [12537.335946] input: eGalax Inc. USB TouchController as /devices/pci:00/:00:1d.2/usb4/4-1/4-1:1.0/input /input13 [12537.336875] hid-multitouch 0003:0EEF:725E.0001: input,hiddev0,hidraw0: USB HID v2.10 Pointer [eGalax Inc. USB TouchController] on usb-:00:1d.2-1/input0 but /dev/input/event13 is not created. To be honest, I don't even know, if it should be created. no, the input13 here has nothing to do with the device node /dev/input/eventX What happens is, that /dev/input/event10 vanishes on removal of the hid-multitouch module and then reappears after modprobe. But a cat on it wont show a reaction while touching the screen's surface. the event10 vanishing is normal, you removed the driver, so the device is not here outside of the kernel. When you modprobe, it's back! However events should come from this node. I have this problem since moving to hid-multitouch for handling this device. What kind of driver did you use before? I think, there was a separate usb touchscreen driver for egalax devices before something like kernel version 3.2. This one I used. Long story short: I have never used the vendor supplied drivers, but those coming with the kernel. Ok, so either you must have patched hid-egalax to handle your device, either you used an other usb driver (it would be great if you can find out which). The weird thing is that hid-egalax does not do anything at resume, so I doubt we should look there. Anyway, can you try the following patch which is already include in the next 3.7 release? Cheers, Benjamin From 4e3265da686f6ff4b02fcba4c124c4af51265375 Mon Sep 17 00:00:00 2001 From: Scott Liu scott@emc.com.tw Date: Wed, 15 Aug 2012 17:21:55 +0800 Subject: [PATCH] HID: multitouch: Add ELAN production request when resume. Add ELAN production request when resume. Some Elan legacy devices require SET_IDLE to be set on resume. It should be safe to send it to other devices too. Tested on 3M, Stantum, Cypress, Zytronic, eGalax, and Elan panels. Suggested by Benjamin Tissoires benjamin.tissoi...@enac.fr Signed-off-by: Scott Liu scott@emc.com.tw Signed-off-by: Jiri Kosina jkos...@suse.cz --- drivers/hid/hid-multitouch.c | 27 +++ 1 file changed, 27 insertions(+) diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index 59c8b5c..e824c37 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -767,6 +767,32 @@ static int mt_reset_resume(struct hid_device *hdev) mt_set_input_mode(hdev); return 0; } + +static int mt_resume(struct hid_device *hdev) +{ + struct usb_interface *intf; + struct usb_host_interface *interface; + struct usb_device *dev; + + if (hdev-bus != BUS_USB) + return 0; + + intf = to_usb_interface(hdev-dev.parent); + interface = intf-cur_altsetting; + dev = hid_to_usb_dev(hdev); + + /* Some Elan legacy devices require SET_IDLE to be set on resume
Re: [PATCH v4 14/14] HID: hid-multitouch: forwards MSC_TIMESTAMP
Hi Guys, well, I'm not very satisfied with this patch. I first thought it was a good idea but I can see now several cons: 1. Henrik would like to partially base the time spent between two events when the device wraps on the *event* time. This is a great idea for consistency, but I'm afraid I won't be able to implement it because this time is computed *after* we call input_event and is only used by evdev. Thus, I still need to add an other clock and some differences may occur. 2. the user space (at least X) will not use it before a long time: they already have the time of the event and it will not add that much consistency. 3. it will wake up the whole input chain when fingers are present but no moves occurs on the digitizer: the only events we get are MSC_TIMESTAMP and EV_SYN and the user-space will be awaken just for that. 4. MSC_TIMESTAMP does not have an abs_max value, thus we are forced to compute sth consistent in the kernel that can be forwarded to the user space. So, I propose not to include this feature, and eventually reverting the patch that introduced MSC_TIMESTAMP as it's useless if we don't use it right now. Jiri, Dmitry, Henrik, are ok with that? Cheers, Benjamin On Tue, Nov 20, 2012 at 9:54 PM, Henrik Rydberg rydb...@euromail.se wrote: below logical_max (say), That was meant to read (logical_max / 2). Henrik -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 00/11] Support of dual pen/multitouch and new default for win 7 certified devices
Hi Guys, Last week, I received two new interesting devices report: - N-trig win 8 certified pen/touch panel - Samsung Nexio 42 N-trig device - The first one is the origin of patches 1 to 6. The multiouch part worked flawlessly with the win 8 patches I sent before, but the pen part was completely ignored. I could have used the quirk MULTI_INPUT, but by testing this quirk against several devices report I have (https://github.com/bentiss/hid-devices), it was a pain because some of them create 4 or 5 useless inputs. I choose to allow the hid driver to control the creation of input devices, thus patches 1 to 3. Nexio device The second one was more problematic. Indeed, it was not working at all with the current release of hid-multitouch. I had several ghost points, and any of the available quirks worked. I finaly found the trick, and this trick applies to all the win7 and win8 devices I saw so far (same url as before). So I think I finally understood why the windows driver was better than us: it first looks at the announced contact count, and treat only the right number. It was so simple... and it works so well... However, for us, I need to get this information from the raw_event because most of the devices put the contact count field at the end of the report. I also decided to change the default class as it is much more tolerant than the previous one. I could have changed all the devices, but in the end, I changed only those that get a benefit and that I could test. Debug tool -- I was able to discover this trick only recently because I made a small C program that allows me to replay the hid events through hid-multitouch. The code is here: https://github.com/bentiss/hid-replay and you will need a kernel 3.6 to make it work (it requires uhid). However, be careful, this program can be the root of many kernel oopses if the targeted hid module tries to directly handle the usb or with any of the usbhid function. So, Henrik, I really need you to push your abstraction of usbhid in all hid modules :) Anyway, this tool can be very helpful to debug hid devices, that's why I share it there... and also because I work for an open-source company :) Happy reviewing. Cheers, Benjamin Benjamin Tissoires (11): HID: hid-input factorize hid_input allocation HID: hid-input: simplify hid_input allocation and registration HID: hid-input: export hidinput_allocation function HID: hid-multitouch: creates and handle stylus report with dual-sensors HID: hid-multitouch: manually send sync event for pen input report HID: hid-multitouch: append Pen to the name of the stylus input HID: hid-multitouch: rename MT_CLS_DEFAULT into MT_CLS_NSMU HID: hid-multitouch: add support for Nexio 42 panel HID: hid-multitouch: check if ContactCount is given for default quirk HID: hid-multitouch: fix protocol for 3 devices HID: hid-multitouch: use MT_QUIRK_CONTACT_COUNT_ACCURATE for win 8 devices drivers/hid/hid-ids.h| 3 + drivers/hid/hid-input.c | 100 +- drivers/hid/hid-multitouch.c | 198 +++ include/linux/hid.h | 1 + 4 files changed, 229 insertions(+), 73 deletions(-) -- 1.8.0 -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: hid-multitouch: eGalax Touchscreen not resuming after suspend
Hi Jan-Matthias, On Sat, Nov 24, 2012 at 7:01 PM, Jan-Matthias Braun jbr...@physik3.gwdg.de wrote: Hi Benjamin, I have tested some more kernel versions. The first version, I could use the hid-multitouch module after adding my device to the blacklist of usb-core.c, was 2.6.38. Using the new_id mechanism I have then triggered hid-multitouch to use my device. I tested every version from 2.6.38 over 2.6.39 to 3.6. commit 5519cab477b61326963c8d523520db0342862b63: Driver crashes at the moment I use the new_id mechanism. 2.6.38-3.0: The device is responsive after reboot, without any additional action on my side. 3.1-3.5: After unloading/reloading the module, the device is working again. 3.6: Even unloading/reloading doesn't help, the device is lost up to the next reboot. I suspect the root of it comes either from usb or usbhid (I'm adding Jiri and Greg in CC so that they can confirm or not). The only changes in the suspend/resume code in hid-multitouch are: - one in kernel 3.3 (I just send a feature at resume). - one in kernel 3.7, that you tested as not functional (and not in the range of your targets). Now I am going to do a bit of bisection testing on both regressions. If you have some hints, on how I could speed testing up, I would welcome them. I hope that Greg or Jiri will help on that. I made a short git blame on drivers/hid/usbhid/hid-core.c, and I did not found anything either. Cheers, Benjamin Cheers, Jan Am Donnerstag, 22. November 2012, 20:00:25 schrieb Jan-Matthias Braun: Hi Benjamin, Am Mittwoch, 21. November 2012, 14:36:33 schrieb Benjamin Tissoires: On 11/21/2012 10:33 AM, Jan-Matthias Braun wrote: Am Dienstag, 20. November 2012, 13:47:23 schrieben Sie: On Mon, Nov 19, 2012 at 12:58 PM, Jan-Matthias Braun jan_br...@gmx.net wrote: using the current (Linux v2.6.7) hid-multitouch driver I have the problem, that the touchscreen works fine after a fresh boot, but after a suspend the touchscreen does not come back to live and I am asking for assistance to get this working. As I can reproduce this problem on a standard tty device without X (see below) I am suspecting a driver problem, but I might as well be wrong. I assume you are talking about v3.6.7... You are right, of course. Sorry for the distraction. The device in question is the Touchscreen of a Dell Inspron Duo convertable, a USB device with id 0eef:725e (D-WAV Scientific Co., Ltd), found and registered as an input device by the kernel as [6.931638] usb 4-1: Manufacturer: eGalax Inc. [ 16.186272] input: eGalax Inc. USB TouchController as /devices/pci:00/:00:1d.2/usb4/4-1/4-1:1.0/input/input10 [ 16.187162] hid-multitouch 0003:0EEF:725E.0001: input,hiddev0,hidraw0: USB HID v2.10 Pointer [eGalax Inc. USB TouchController] on usb-:00:1d.2-1/input0 I have tested the behaviour with and without X11. Without X11 I was using a standard tty and using cat on the input device. After boot the input device gives lot of output while touching the screen, after resume the device stays silent. strange. I have tested the procedure with the eGalax 0x72FA I got, and I'm not seeing this problem on a 3.6 kernel. Is the config symbol CONFIG_PM set to y in your .config file? Yes. Also, can you try to rmmod / modprobe hid-multitouch when the device is not responding and see if this solves things. This is not working for me. On modprobe the Kernel log says [12537.335946] input: eGalax Inc. USB TouchController as /devices/pci:00/:00:1d.2/usb4/4-1/4-1:1.0/input /input13 [12537.336875] hid-multitouch 0003:0EEF:725E.0001: input,hiddev0,hidraw0: USB HID v2.10 Pointer [eGalax Inc. USB TouchController] on usb-:00:1d.2-1/input0 but /dev/input/event13 is not created. To be honest, I don't even know, if it should be created. no, the input13 here has nothing to do with the device node /dev/input/eventX What happens is, that /dev/input/event10 vanishes on removal of the hid-multitouch module and then reappears after modprobe. But a cat on it wont show a reaction while touching the screen's surface. the event10 vanishing is normal, you removed the driver, so the device is not here outside of the kernel. When you modprobe, it's back! However events should come from this node. I have this problem since moving to hid-multitouch for handling this device. What kind of driver did you use before? I think, there was a separate usb touchscreen driver for egalax devices before something like kernel version 3.2. This one I used. Long story short: I have never used the vendor supplied drivers, but those coming with the kernel. Ok, so either you must have patched hid-egalax to handle your device, either you used an other usb driver (it would be great if you can find
Re: [PATCH 08/11] HID: hid-multitouch: add support for Nexio 42 panel
On Fri, Nov 23, 2012 at 4:31 PM, Benjamin Tissoires benjamin.tissoi...@gmail.com wrote: This device is the worst device I saw. It keeps TipSwitch and InRange at 1 for fingers that are not touching the panel. The solution is to rely on the field ContactCount, which is accurate as the correct information are packed at the begining of the frame. Unfortunately, CountactCount is most of the time at the end of the report. The solution is to pick it when we have the whole report in raw_event. Fortunately, it occurs that this behavior is pretty well compliant with all the devices I saw so far. We can make this class the default then. Signed-off-by: Benjamin Tissoires benjamin.tissoi...@gmail.com --- drivers/hid/hid-ids.h| 3 ++ drivers/hid/hid-multitouch.c | 82 2 files changed, 78 insertions(+), 7 deletions(-) diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h index b84790a..9dfc465 100644 --- a/drivers/hid/hid-ids.h +++ b/drivers/hid/hid-ids.h @@ -599,6 +599,9 @@ #define USB_VENDOR_ID_NEC 0x073e #define USB_DEVICE_ID_NEC_USB_GAME_PAD 0x0301 +#define USB_VENDOR_ID_NEXIO0x1870 +#define USB_DEVICE_ID_NEXIO_MULTITOUCH_420 0x0100 Oops, I made a typo here. The PID should be 0x010d. I can send a v2 if needed. Cheers, Benjamin + #define USB_VENDOR_ID_NEXTWINDOW 0x1926 #define USB_DEVICE_ID_NEXTWINDOW_TOUCHSCREEN 0x0003 diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index 50fb79f..36cf346 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -33,6 +33,8 @@ #include linux/usb.h #include linux/input/mt.h #include linux/string.h +#include asm/unaligned.h +#include asm/byteorder.h #include usbhid/usbhid.h @@ -55,6 +57,7 @@ MODULE_LICENSE(GPL); #define MT_QUIRK_NO_AREA (1 9) #define MT_QUIRK_IGNORE_DUPLICATES (1 10) #define MT_QUIRK_HOVERING (1 11) +#define MT_QUIRK_CONTACT_COUNT_ACCURATE(1 12) struct mt_slot { __s32 x, y, cx, cy, p, w, h; @@ -84,6 +87,8 @@ struct mt_device { struct mt_class mtclass;/* our mt device class */ struct mt_fields *fields; /* temporary placeholder for storing the multitouch fields */ + struct hid_field *contactcount; /* the hid_field contact count that + will be picked in mt_raw_event */ unsigned last_field_index; /* last field index of the report */ unsigned last_pen_field_index; /* last field index of the pen report */ unsigned last_slot_field; /* the last field of a slot */ @@ -148,7 +153,9 @@ static int cypress_compute_slot(struct mt_device *td) } static struct mt_class mt_classes[] = { - { .name = MT_CLS_DEFAULT}, + { .name = MT_CLS_DEFAULT, + .quirks = MT_QUIRK_ALWAYS_VALID | + MT_QUIRK_CONTACT_COUNT_ACCURATE }, { .name = MT_CLS_NSMU, .quirks = MT_QUIRK_NOT_SEEN_MEANS_UP }, { .name = MT_CLS_SERIAL, @@ -487,6 +494,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi, td-last_field_index = field-index; return 1; case HID_DG_CONTACTCOUNT: + td-contactcount = field; td-last_field_index = field-index; return 1; case HID_DG_CONTACTMAX: @@ -554,6 +562,10 @@ static int mt_compute_slot(struct mt_device *td, struct input_dev *input) */ static void mt_complete_slot(struct mt_device *td, struct input_dev *input) { + if ((td-mtclass.quirks MT_QUIRK_CONTACT_COUNT_ACCURATE) + td-num_received = td-num_expected) + return; + if (td-curvalid || (td-mtclass.quirks MT_QUIRK_ALWAYS_VALID)) { int slotnum = mt_compute_slot(td, input); struct mt_slot *s = td-curdata; @@ -665,12 +677,6 @@ static int mt_event(struct hid_device *hid, struct hid_field *field, td-curdata.h = value; break; case HID_DG_CONTACTCOUNT: - /* -* Includes multi-packet support where subsequent -* packets are sent with zero contactcount. -*/ - if (value) - td-num_expected = value; break; case HID_DG_TOUCH: /* do nothing */ @@ -700,6 +706,62 @@ static int mt_event(struct hid_device *hid, struct hid_field *field, return 1; } +/* + * Extract/implement a data field from/to a little endian report (bit array). + * Copied from hid-core.c. + * + * Code sort
Re: [PATCH] HID: Separate struct hid_device's driver_lock into two locks.
Hi Bruno, On Mon, Nov 26, 2012 at 6:56 PM, Bruno Prémont bonb...@linux-vserver.org wrote: Hi Andrew, [CCing David Herrmann] On Sun, 25 November 2012 Andrew de los Reyes wrote: Benjamin Tissoires and Nestor Lopez Casado have been helping me to add Linux support for new Logitech Touch Mice (T620, T400). After running into a road-block in hid-core, and solving it with this patch, we thought it was good to show the community and see if this is okay, or if there's a better solution that we've missed. Thanks for your comments, -andrew This patch separates struct hid_device's driver_lock into two. The goal is to allow hid device drivers to receive input during their probe() function call. This is necessary because some drivers need to communicate with the device to determine parameters needed during probe (e.g., size of a multi-touch surface). Historically, three functions used driver_lock: - hid_device_probe: blocks to acquire lock - hid_device_remove: blocks to acquire lock - hid_input_report: if locked returns -EBUSY, else acquires lock This patch adds another lock (driver_input_lock) which is used to block input from occurring. The lock behavior is now: - hid_device_probe: blocks to acq. driver_lock, then driver_input_lock - hid_device_remove: blocks to acq. driver_lock, then driver_input_lock - hid_input_report: if driver_input_lock locked returns -EBUSY, else acquires driver_input_lock This results in no behavior change for existing devices and drivers. However, during a probe() function call in a driver, that driver may now selectively unlock driver_input_lock to let input events come through, then re-lock. That is more or less a new approach to release the restriction added in commit 4ea5454203d991ec85264f64f89ca8855fce69b0 (HID: Fix race condition between driver core and ll-driver). From your suggested change the question could be if releasing the input lock should be connected to hw_start() / hw_stop() calls... Though connecting those together would certainly require some review of existing drivers in order not to reopen the can of worms closed by above mentioned commit. It is clear that the goal is to make commit 4ea5454203d991ec85264f64f89ca8855fce69b0 less restrictive. The problem is that this commit stops any communication with the device, even configuration communication. Logitech devices use their own protocol on top of the HID standard protocol. For touch devices, this proprietary protocol requires to ask the device for axis ranges, etc... So here, the idea is not to open the can of worm for every hid devices through hw_start() / hw_stop() calls, I think the idea of Andrew is just to allow hid-logitech-dj to get rid of this restriction in some particular circumstances. Consider this as a controlled backdoor of the can of worms :) Cheers, Benjamin -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 02/06] input/rmi4: Core files
On Mon, Nov 26, 2012 at 11:54 PM, Christopher Heiny che...@synaptics.com wrote: On 11/26/2012 10:41 AM, Benjamin Tissoires wrote: Hi Christopher, On Sat, Nov 17, 2012 at 4:58 AM, Christopher Heiny che...@synaptics.com wrote: rmi_bus.c implements the basic functionality of the RMI bus. This file is greatly simplified compared to the previous patch - we've switched from do it yourself device/driver binding to using device_type to distinguish between the two kinds of devices on the bus (sensor devices and function specific devices) and using the standard bus implementation to manage devices and drivers. rmi_driver.c is a driver for the general functionality of the RMI sensor as a whole, managing those behaviors (including IRQ handling) that are not specific to any RMI4 function. It has some unavoidable dependencies on F01 behavior, though we have worked to minimize those as far as possible. The header file rmi_driver.h provides definitions that are shared among the modules of the RMI implementation, but not thought to be necessary outside it. Greg KH - Linus Walleij recommended that we seek your input on these core files, particularly the bus implementation. Signed-off-by: Christopher Heiny che...@synaptics.com Cc: Greg Kroah-Hartman gre...@linuxfoundation.org Cc: Dmitry Torokhov dmitry.torok...@gmail.com Cc: Linus Walleij linus.wall...@stericsson.com Cc: Naveen Kumar Gaddipati naveen.gaddip...@stericsson.com Cc: Joeri de Gram j.de.g...@gmail.com --- drivers/input/rmi4/rmi_bus.c| 248 ++ drivers/input/rmi4/rmi_driver.c | 1663 +++ drivers/input/rmi4/rmi_driver.h | 139 include/uapi/linux/input.h |1 + 4 files changed, 2051 insertions(+), 0 deletions(-) [snipped] diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c new file mode 100644 index 000..05a73ae --- /dev/null +++ b/drivers/input/rmi4/rmi_driver.c [snipped] +/* extract product ID */ +void get_prod_id(struct rmi_device *rmi_dev, struct rmi_driver_data *drvdata) +{ + struct device *dev = rmi_dev-dev; + int retval; + int board = 0, rev = 0; + int i; + static const char * const pattern[] = { + tm%4d-%d, s%4d-%d, s%4d-ver%1d}; + u8 product_id[RMI_PRODUCT_ID_LENGTH+1]; + + retval = rmi_read_block(rmi_dev, + drvdata-f01_container-fd.query_base_addr+ + sizeof(struct f01_basic_queries), + product_id, RMI_PRODUCT_ID_LENGTH); + if (retval 0) { + dev_err(dev, Failed to read product id, code=%d!, retval); + return; + } + product_id[RMI_PRODUCT_ID_LENGTH] = '\0'; + + for (i = 0; i sizeof(product_id); i++) + product_id[i] = tolower(product_id[i]); + + for (i = 0; i sizeof(pattern); i++) { This should be ARRAY_SIZE(pattern). It gave me a wonderful kernel oops :) Yep, that's a bug! Oddly enough, it runs without barfing on my systems (though who knows what horrible things are happening under the hood). Thanks for letting us know. That's because the product id advertised by the touchpad you sent to me is 'DS4 R3.0'. So it's not compliant with the patterns that are advertised: tm%4d-%d, s%4d-%d, s%4d-ver%1d. If you present one of these patterns, thanks to the break, you leave the for loop. But as I'm not in this optimal case, I'm having i 2, which introduced segfault. Cheers, Benjamin Chris Cheers, Benjamin [snip] -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 06/06] input/rmi4: F11 - 2D touch interface
Hi Christopher, I did not made a full review, but at least, there is a problem in your rmi_f11_finger_handler function: On Sat, Nov 17, 2012 at 4:58 AM, Christopher Heiny che...@synaptics.com wrote: rmi_f11.c is a driver for 2D touch sensors using the RMI4 protocol. It supports both touchscreen and touchpad input, in both absolute and relative formats. Support for Type-B multitouch is the default, Type-A support is included for certain legacy sensors. Signed-off-by: Christopher Heiny che...@synaptics.com To: Henrik Rydberg rydb...@euromail.se Cc: Dmitry Torokhov dmitry.torok...@gmail.com Cc: Linus Walleij linus.wall...@stericsson.com Cc: Naveen Kumar Gaddipati naveen.gaddip...@stericsson.com Cc: Joeri de Gram j.de.g...@gmail.com --- drivers/input/rmi4/rmi_f11.c | 2813 ++ 1 files changed, 2813 insertions(+), 0 deletions(-) diff --git a/drivers/input/rmi4/rmi_f11.c b/drivers/input/rmi4/rmi_f11.c new file mode 100644 index 000..b9a84bc --- /dev/null +++ b/drivers/input/rmi4/rmi_f11.c [snipped] +static void rmi_f11_finger_handler(struct f11_data *f11, + struct f11_2d_sensor *sensor) +{ + const u8 *f_state = sensor-data.f_state; + u8 finger_state; + u8 finger_pressed_count; + u8 i; + + for (i = 0, finger_pressed_count = 0; i sensor-nbr_fingers; i++) { + /* Possible of having 4 fingers per f_statet register */ + finger_state = (f_state[i / 4] (2 * (i % 4))) + FINGER_STATE_MASK; + if (finger_state == F11_RESERVED) { + pr_err(%s: Invalid finger state[%d]:0x%02x., __func__, + i, finger_state); + continue; + } else if ((finger_state == F11_PRESENT) || + (finger_state == F11_INACCURATE)) { + finger_pressed_count++; + } + + if (sensor-data.abs_pos) + rmi_f11_abs_pos_report(f11, sensor, finger_state, i); + + if (sensor-data.rel_pos) + rmi_f11_rel_pos_report(sensor, i); + } + input_mt_sync(sensor-input); This should be input_mt_sync_frame(sensor-input); otherwise, the synaptics xorg driver won't handle the touchpad correctly. Cheers, Benjamin + input_sync(sensor-input); +} + [snipped] -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] i2c-hid: introduce HID over i2c specification implementation
Hi Jean, On Fri, Nov 30, 2012 at 3:56 PM, Jean Delvare kh...@linux-fr.org wrote: Hi Benjamin, Jiri, Sorry for the late review. But better late than never I guess... Sure! Thanks for the review. As the driver is already in Jiri's tree, I'll do small incremental patches based on this release. I'll try to address all of your comments. I have a few answers to some of your remarks (I fully agree with all of the others): On Mon, 12 Nov 2012 15:42:59 +0100, Benjamin Tissoires wrote: Microsoft published the protocol specification of HID over i2c: http://msdn.microsoft.com/en-us/library/windows/hardware/hh852380.aspx This patch introduces an implementation of this protocol. This implementation does not includes the ACPI part of the specification. This will come when ACPI 5.0 devices enumeration will be available. Once the ACPI part is done, OEM will not have to declare HID over I2C devices in their platform specific driver. Signed-off-by: Benjamin Tissoires benjamin.tissoi...@gmail.com --- (..) drivers/hid/Kconfig | 2 + drivers/hid/Makefile | 1 + drivers/hid/i2c-hid/Kconfig | 21 + drivers/hid/i2c-hid/Makefile | 5 + drivers/hid/i2c-hid/i2c-hid.c | 974 ++ include/linux/i2c/i2c-hid.h | 35 ++ 6 files changed, 1038 insertions(+) create mode 100644 drivers/hid/i2c-hid/Kconfig create mode 100644 drivers/hid/i2c-hid/Makefile create mode 100644 drivers/hid/i2c-hid/i2c-hid.c create mode 100644 include/linux/i2c/i2c-hid.h Looks much better. I still have several random comments which you may be interested in. (...) +/* debug option */ +static bool debug = false; Whether you like it or not, that's still an error for checkpatch: ERROR: do not initialise statics to 0 or NULL #240: FILE: drivers/hid/i2c-hid/i2c-hid.c:49: +static bool debug = false; The rationale is that the compiler can write more efficient code if initializations to 0 or NULL are implicit. ok, will do. +module_param(debug, bool, 0444); +MODULE_PARM_DESC(debug, print a lot of debug information); + +#define i2c_hid_dbg(ihid, fmt, arg...) \ + if (debug) \ + dev_printk(KERN_DEBUG, (ihid)-client-dev, fmt, ##arg) This is considered an error by checkpatch too: ERROR: Macros with complex values should be enclosed in parenthesis #244: FILE: drivers/hid/i2c-hid/i2c-hid.c:53: +#define i2c_hid_dbg(ihid, fmt, arg...) \ + if (debug) \ + dev_printk(KERN_DEBUG, (ihid)-client-dev, fmt, ##arg) And I wouldn't disagree, as the construct above can lead to bugs which are hard to see... Consider the following piece of code: if (condition) i2c_hid_dbg(ihid, Blah blah %d\n, i); else do_something_very_important(); It looks correct, however with the macro definition above, this expands to the unexpected: if (condition) { if (debug) \ dev_printk(KERN_DEBUG, ihid-client-dev, Blah blah %d\n, i); else do_something_very_important(); } That is, the condition to call do_something_very_important() is condition !debug, and not !condition as the code suggests. This is only an example and your driver doesn't currently use any such construct. Still I believe this should be fixed. With your explanation, you've convinced me. I was not sure on how should I handle this warning as I copied this macro from one in hid. I will fix the two then. (...) static int i2c_hid_alloc_buffers(struct i2c_hid *ihid) { /* the worst case is computed from the set_report command with a * reportID 15 and the maximum report length */ int args_len = sizeof(__u8) + /* optional ReportID byte */ sizeof(__u16) + /* data register */ sizeof(__u16) + /* size of the report */ ihid-bufsize; /* report */ ihid-inbuf = kzalloc(ihid-bufsize, GFP_KERNEL); ihid-argsbuf = kzalloc(args_len, GFP_KERNEL); ihid-cmdbuf = kzalloc(sizeof(union command) + args_len, GFP_KERNEL); if (!ihid-inbuf || !ihid-argsbuf || !ihid-cmdbuf) { i2c_hid_free_buffers(hid); return -ENOMEM; } return 0; } Much better, thanks! +static void i2c_hid_free_buffers(struct i2c_hid *ihid) +{ + kfree(ihid-inbuf); + kfree(ihid-argsbuf); + kfree(ihid-cmdbuf); + ihid-inbuf = NULL; + ihid-cmdbuf = NULL; + ihid-argsbuf = NULL; +} + +static int i2c_hid_get_raw_report(struct hid_device *hid, + unsigned char report_number, __u8 *buf, size_t count, + unsigned char report_type) +{ + struct i2c_client *client = hid-driver_data; + struct i2c_hid *ihid = i2c_get_clientdata
Re: [PATCH v3] i2c-hid: introduce HID over i2c specification implementation
On Mon, Dec 3, 2012 at 2:02 PM, Jean Delvare kh...@linux-fr.org wrote: Hi Benjamin, On Mon, 3 Dec 2012 12:32:03 +0100, Benjamin Tissoires wrote: Hi Jean, On Fri, Nov 30, 2012 at 3:56 PM, Jean Delvare kh...@linux-fr.org wrote: Hi Benjamin, Jiri, Sorry for the late review. But better late than never I guess... Sure! Thanks for the review. As the driver is already in Jiri's tree, I'll do small incremental patches based on this release. I'll try to address all of your comments. I have a few answers to some of your remarks (I fully agree with all of the others): On Mon, 12 Nov 2012 15:42:59 +0100, Benjamin Tissoires wrote: +static int i2c_hid_get_raw_report(struct hid_device *hid, + unsigned char report_number, __u8 *buf, size_t count, + unsigned char report_type) +{ + struct i2c_client *client = hid-driver_data; + struct i2c_hid *ihid = i2c_get_clientdata(client); + int ret; + + if (report_type == HID_OUTPUT_REPORT) + return -EINVAL; + + if (count ihid-bufsize) + count = ihid-bufsize; + + ret = i2c_hid_get_report(client, + report_type == HID_FEATURE_REPORT ? 0x03 : 0x01, + report_number, ihid-inbuf, count); + + if (ret 0) + return ret; + + count = ihid-inbuf[0] | (ihid-inbuf[1] 8); + + memcpy(buf, ihid-inbuf + 2, count); What guarantee do you have that count is not larger than buf's length? I hope you don't just count on all hardware out there being nice and sane, do you? ;) Hehe, this function is never called from the device, but from the user space. It's called by hidraw_get_report in drivers/hid/hidraw.c, and the caller makes the allocation of buf with a size of count. There is an other usage in hid-input.c with buf, sizeof(buf), as arguments. So this should never be a problem as long as anybody else call this function without making sure count is the right size. Not sure I follow you here. There are two flavors of count in this function. The first one is passed as a parameter by the caller and used to set the buffer length as passed to i2c_hid_get_report(). This part is fine and I have no problem with it. The second flavor is extracted from ihid-inbuf as provided by i2c_hid_get_report(). As I understand it, i2c_hid_get_report() fills the buffer with data it receives from the hardware, so I fail to see how you can have any guarantee that this second flavor of count is not greater than buf's length. In fact I don't think you should reuse count in the first place, that's confusing. Then, looking at the code again, I don't think the test count ihid-bufsize, and more generally changing the value of count, makes sense. You don't care about the size of buf when calling i2c_hid_get_report(), you care about the size of ihid-inbuf, which should be at least 2 more than the size of buf. You care about the size of buf _after_ the call to i2c_hid_get_report(), as you are copying the data from ihid-bufsize to buf. This is precisely the check which I claim is missing. Oops. Really sorry. You are perfectly right. This mismatch comes from the reuse of count for 2 different purposes, so it was not a good idea at all. Will fix it. And thanks! (...) + + hid = ihid-hid; + hid_destroy_device(hid); + + free_irq(client-irq, ihid); + Is there any guarantee that i2c_hid_stop() has been called before i2c_hid_remove() is? If not, you're missing a call to i2c_hid_free_buffers() here. BTW I'm not quite sure why i2c_hid_remove() frees the buffers in the first place, but then again I don't know a thing about the HID infrastructure. Calling i2c_hid_stop() is the responsibility of the hid driver at his remove. By hid driver, I mean the driver that relies on hid to handle the device (hid-generic in 80% of the cases) So as long as this hid driver is loaded, we can not remove i2c_hid as it is used by the hid driver. So it seems that this guarantees that i2c_hid_stop() has been called before i2c_hid_remove() is. But now that I think of it, there are cases where i2c_hid_stop() is not called: when the hid driver failed to probe. So definitively, there is a mem leak here. Thanks. Actually this path was fixed by Jiri already: http://git.kernel.org/?p=linux/kernel/git/jikos/hid.git;a=commitdiff;h=3c62602434c13744df62b3ab0ab7950cd36f24db Not exactly. This commit fixes the memory leak when i2c_hid_probe fails. Not when the hid driver (hid-generic or hid-multitouch in our cases) fails. My worry was that, when probe succeeds, i2c_hid_start/stop() could never be called if the module was unloaded immediately for example, before user-space has a chance to use the device. But if you are certain it can't happen, then alright. I made a few tests after sending the mail. Contrary to what I said, it's possible to unload the i2c-hid module before the hid
Re: [PATCH v3] i2c-hid: introduce HID over i2c specification implementation
On Tue, Dec 4, 2012 at 11:31 AM, Jiri Kosina jkos...@suse.cz wrote: On Mon, 3 Dec 2012, Benjamin Tissoires wrote: I know that it already have been used by one Nvidia team and by Elan for internal tests. So I don't know if it's possible to change it now (though it's not a big deal). Yes it is possible, as long as the code isn't in Linus' tree (and I'd even say: as long as it is not in any kernel released by Linus.) Whoever is already using your driver will have to adjust their code. They'll certainly want to anyway, in order to get the bug fixes. Anyway, hid is a little weird to me (but this is because I started hacking the kernel from there), as it's really short and does not contain i2c :) This is exactly my point: no other i2c client (to my knowledge) has i2c in its name, because it is redundant. I would agree that this is kind of a special case as this is a generic driver and not a device-specific driver. I would still prefer hid but if you don't feel like changing it, I guess I can live with that. Ok, I'll change the name. My concerns were just because from my point of view, hid refers to the core implementation. But for OEMs, they need to declare a hid device, so it makes sense. Anyway, when I'll be able to get my hand on an ACPI 5 device with hid over i2c, I'll be able to write the missing autoloading code, and this name will be only used by a small part of OEMs (I hope). If you are going to change the name, please either send me the patch before merge window for 3.8 opens, or explicitly ask me not to include this driver in pull request that goes to Linus yet. Sure, I intend to send some patches today (with the change of the name included). I'll send the patch related to the name in the first position so that you can safely pick it up even if you are not satisfied with the other changes. Thanks, Benjamin Once the driver is in Linus' tree, I'd rather not change the name. Thanks, -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 08/14] HID: i2c-hid: fix error messages
Signed-off-by: Benjamin Tissoires benjamin.tissoi...@gmail.com --- drivers/hid/i2c-hid/i2c-hid.c | 11 ++- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c index d6fdb3e..da04948 100644 --- a/drivers/hid/i2c-hid/i2c-hid.c +++ b/drivers/hid/i2c-hid/i2c-hid.c @@ -741,10 +741,10 @@ static int __devinit i2c_hid_init_irq(struct i2c_client *client) IRQF_TRIGGER_FALLING | IRQF_ONESHOT, client-name, ihid); if (ret 0) { - dev_dbg(client-dev, + dev_warn(client-dev, Could not register for %s interrupt, irq = %d, ret = %d\n, - client-name, client-irq, ret); + client-name, client-irq, ret); return ret; } @@ -770,7 +770,8 @@ static int __devinit i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid) __func__, 4, ihid-hdesc_buffer); if (ret) { - dev_err(client-dev, HID_DESCR_LENGTH_CMD Fail (ret=%d)\n, + dev_err(client-dev, + unable to fetch the size of HID descriptor (ret=%d)\n, ret); return -ENODEV; } @@ -785,7 +786,7 @@ static int __devinit i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid) /* check bcdVersion == 1.0 */ if (le16_to_cpu(hdesc-bcdVersion) != 0x0100) { dev_err(client-dev, - unexpected HID descriptor bcdVersion (0x%04x)\n, + unexpected HID descriptor bcdVersion (0x%04hx)\n, le16_to_cpu(hdesc-bcdVersion)); return -ENODEV; } @@ -871,7 +872,7 @@ static int __devinit i2c_hid_probe(struct i2c_client *client, hid-vendor = le16_to_cpu(ihid-hdesc.wVendorID); hid-product = le16_to_cpu(ihid-hdesc.wProductID); - snprintf(hid-name, sizeof(hid-name), %s %04X:%04X, + snprintf(hid-name, sizeof(hid-name), %s %04hX:%04hX, client-name, hid-vendor, hid-product); ret = hid_add_device(hid); -- 1.8.0.1 -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/14] HID: i2c-hid: fix memory corruption due to missing hid declaration
On Tue, Dec 4, 2012 at 10:42 PM, Jean Delvare kh...@linux-fr.org wrote: On Tue, 4 Dec 2012 16:27:43 +0100, Benjamin Tissoires wrote: HID descriptors contains 4 bytes of reserved field. The previous implementation was overriding the next fields in struct i2c_hid. Signed-off-by: Benjamin Tissoires benjamin.tissoi...@gmail.com --- drivers/hid/i2c-hid/i2c-hid.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c index 0fbb229..ad771a7 100644 --- a/drivers/hid/i2c-hid/i2c-hid.c +++ b/drivers/hid/i2c-hid/i2c-hid.c @@ -68,6 +68,7 @@ struct i2c_hid_desc { __le16 wVendorID; __le16 wProductID; __le16 wVersionID; + __le32 reserved; } __packed; struct i2c_hid_cmd { @@ -778,7 +779,7 @@ static int __devinit i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid) } dsize = le16_to_cpu(hdesc-wHIDDescLength); - if (!dsize || dsize HID_MAX_DESCRIPTOR_SIZE) { + if (!dsize || dsize sizeof(struct i2c_hid_desc)) { Shouldn't !dsize rather be dsize 4? You're reading hdesc-bcdVersion, which is only defined if dsize = 4 if I understand the code correctly. Yes, you are right. Thanks. Jiri, this patch is a prerequisite for [PATCH 10/14] HID: i2c-hid: reorder allocation/free of buffers. could you please what for a v2 before applying 10/14? Cheers, Benjamin dev_err(client-dev, weird size of HID descriptor (%u)\n, dsize); return -ENODEV; -- Jean Delvare -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] HID: i2c-hid: fix i2c_hid_get_raw_report count mismatches
On Thu, Dec 6, 2012 at 11:01 AM, Jiri Kosina jkos...@suse.cz wrote: - count = ihid-inbuf[0] | (ihid-inbuf[1] 8); + ret_count = ihid-inbuf[0] | (ihid-inbuf[1] 8); + if (!ret_count) I'd make this (ret_count = 2), as this would let you call memcpy with a null or even negative length. Good catch, it doesn't account for the 2 bytes needed for storing the reply size. I have fixed that and applied the patch, thanks everybody! Hi Jiri, Jean, thank you very much for the work done. I was in a meeting past two days, so I was not able to answer sooner. Cheers, Benjamin -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] HID: i2c-hid: add mutex protecting open/close race
On Wed, Dec 12, 2012 at 5:34 PM, Jean Delvare kh...@linux-fr.org wrote: Hi Benjamin, On Wed, 12 Dec 2012 17:12:16 +0100, Benjamin Tissoires wrote: We should not enter close function while someone else is in open. This mutex prevents this race. There is also no need to override the ret value with -EIO in case of a failure of i2c_hid_set_power. Signed-off-by: Benjamin Tissoires benjamin.tissoi...@gmail.com --- Looks good to me except that you forgot to include linux/mutex.h and this could cause build failures on some architectures/configs. Once this is fixed, you can add: Reviewed-by: Jean Delvare kh...@linux-fr.org Thanks, I will resend it right now. Cheers, Benjamin -- Jean Delvare -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] HID: i2c-hid: add mutex protecting open/close race
We should not enter close function while someone else is in open. This mutex prevents this race. There is also no need to override the ret value with -EIO in case of a failure of i2c_hid_set_power. Signed-off-by: Benjamin Tissoires benjamin.tissoi...@gmail.com Reviewed-by: Jean Delvare kh...@linux-fr.org --- Changes in v2: * add mutex.h Cheers, Benjamin drivers/hid/i2c-hid/i2c-hid.c | 14 +++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c index 6e1774c..9ef22244 100644 --- a/drivers/hid/i2c-hid/i2c-hid.c +++ b/drivers/hid/i2c-hid/i2c-hid.c @@ -33,6 +33,7 @@ #include linux/jiffies.h #include linux/kernel.h #include linux/hid.h +#include linux/mutex.h #include linux/i2c/i2c-hid.h @@ -117,6 +118,8 @@ static const struct i2c_hid_cmd hid_set_power_cmd = { I2C_HID_CMD(0x08) }; * static const struct i2c_hid_cmd hid_set_protocol_cmd = { I2C_HID_CMD(0x07) }; */ +static DEFINE_MUTEX(i2c_hid_open_mut); + /* The main device structure */ struct i2c_hid { struct i2c_client *client;/* i2c client */ @@ -641,17 +644,20 @@ static int i2c_hid_open(struct hid_device *hid) { struct i2c_client *client = hid-driver_data; struct i2c_hid *ihid = i2c_get_clientdata(client); - int ret; + int ret = 0; + mutex_lock(i2c_hid_open_mut); if (!hid-open++) { ret = i2c_hid_set_power(client, I2C_HID_PWR_ON); if (ret) { hid-open--; - return -EIO; + goto done; } set_bit(I2C_HID_STARTED, ihid-flags); } - return 0; +done: + mutex_unlock(i2c_hid_open_mut); + return ret; } static void i2c_hid_close(struct hid_device *hid) @@ -663,12 +669,14 @@ static void i2c_hid_close(struct hid_device *hid) * data acquistion due to a resumption we no longer * care about */ + mutex_lock(i2c_hid_open_mut); if (!--hid-open) { clear_bit(I2C_HID_STARTED, ihid-flags); /* Save some power */ i2c_hid_set_power(client, I2C_HID_PWR_SLEEP); } + mutex_unlock(i2c_hid_open_mut); } static int i2c_hid_power(struct hid_device *hid, int lvl) -- 1.8.0.2 -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/11] Support of dual pen/multitouch and new default for win 7 certified devices
On Thu, Jan 3, 2013 at 10:50 AM, Jiri Kosina jkos...@suse.cz wrote: On Fri, 23 Nov 2012, Benjamin Tissoires wrote: Last week, I received two new interesting devices report: - N-trig win 8 certified pen/touch panel - Samsung Nexio 42 Bejmanin, Henrik, what are the plans with this patchset please? Planning respin for 3.9? Hi Jiri, sure. I intend to make a respin of this patch series. I spent a lot of time lately to automate regressions tests for hid-multitouch. Lucky me, because some patches of the original series would have broken some devices. It also helped me to find out 2 other broken devices (they were broken since their inclusion in hid-multitouch) The tests automation is pretty useful, however it can not be used as this on a vanilla kernel due to the direct dependencies against usbhid. Henrik, you told us that you have a patch set fixing these dependencies on all the hid drivers, will you prepare it for 3.9? Do you want me to continue your work? Cheers, Benjamin Thanks. N-trig device - The first one is the origin of patches 1 to 6. The multiouch part worked flawlessly with the win 8 patches I sent before, but the pen part was completely ignored. I could have used the quirk MULTI_INPUT, but by testing this quirk against several devices report I have (https://github.com/bentiss/hid-devices), it was a pain because some of them create 4 or 5 useless inputs. I choose to allow the hid driver to control the creation of input devices, thus patches 1 to 3. Nexio device The second one was more problematic. Indeed, it was not working at all with the current release of hid-multitouch. I had several ghost points, and any of the available quirks worked. I finaly found the trick, and this trick applies to all the win7 and win8 devices I saw so far (same url as before). So I think I finally understood why the windows driver was better than us: it first looks at the announced contact count, and treat only the right number. It was so simple... and it works so well... However, for us, I need to get this information from the raw_event because most of the devices put the contact count field at the end of the report. I also decided to change the default class as it is much more tolerant than the previous one. I could have changed all the devices, but in the end, I changed only those that get a benefit and that I could test. Debug tool -- I was able to discover this trick only recently because I made a small C program that allows me to replay the hid events through hid-multitouch. The code is here: https://github.com/bentiss/hid-replay and you will need a kernel 3.6 to make it work (it requires uhid). However, be careful, this program can be the root of many kernel oopses if the targeted hid module tries to directly handle the usb or with any of the usbhid function. So, Henrik, I really need you to push your abstraction of usbhid in all hid modules :) Anyway, this tool can be very helpful to debug hid devices, that's why I share it there... and also because I work for an open-source company :) Happy reviewing. Cheers, Benjamin Benjamin Tissoires (11): HID: hid-input factorize hid_input allocation HID: hid-input: simplify hid_input allocation and registration HID: hid-input: export hidinput_allocation function HID: hid-multitouch: creates and handle stylus report with dual-sensors HID: hid-multitouch: manually send sync event for pen input report HID: hid-multitouch: append Pen to the name of the stylus input HID: hid-multitouch: rename MT_CLS_DEFAULT into MT_CLS_NSMU HID: hid-multitouch: add support for Nexio 42 panel HID: hid-multitouch: check if ContactCount is given for default quirk HID: hid-multitouch: fix protocol for 3 devices HID: hid-multitouch: use MT_QUIRK_CONTACT_COUNT_ACCURATE for win 8 devices drivers/hid/hid-ids.h| 3 + drivers/hid/hid-input.c | 100 +- drivers/hid/hid-multitouch.c | 198 +++ include/linux/hid.h | 1 + 4 files changed, 229 insertions(+), 73 deletions(-) -- 1.8.0 -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] HID: i2c-hid: add ACPI support
Hi Mika, On Tue, Jan 8, 2013 at 2:05 PM, Mika Westerberg mika.westerb...@linux.intel.com wrote: The HID over I2C protocol specification states that when the device is enumerated from ACPI the HID descriptor address can be obtained by executing _DSM for the device with function 1. Enable this. Hehe, funny thing, I worked on the very same patch last Friday. I was not able to send it upstream as I still don't have an ACPI 5 enabled device... So thanks for submitting (and testing) this! Before the review, I just have a quick question. All the HID/i2c devices I saw so far present a reset line (through a GPIO). Does the shipped device you have present this line, and if yes, how this meld with the code (i.e. should we take this into account). Please note that I can only compare this to my patch, based on the DSDT example Microsoft gave in its spec. So sorry if I'm asking useless things, but I like to understand... :) Here are a few comments: Signed-off-by: Mika Westerberg mika.westerb...@linux.intel.com --- drivers/hid/i2c-hid/i2c-hid.c | 73 +++-- 1 file changed, 71 insertions(+), 2 deletions(-) diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c index 9ef22244..b2eebb6 100644 --- a/drivers/hid/i2c-hid/i2c-hid.c +++ b/drivers/hid/i2c-hid/i2c-hid.c @@ -34,6 +34,7 @@ #include linux/kernel.h #include linux/hid.h #include linux/mutex.h +#include linux/acpi.h #include linux/i2c/i2c-hid.h @@ -810,6 +811,70 @@ static int __devinit i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid) return 0; } +#ifdef CONFIG_ACPI +static struct i2c_hid_platform_data * +i2c_hid_acpi_pdata(struct i2c_client *client) +{ + static u8 i2c_hid_guid[] = { + 0xF7, 0xF6, 0xDF, 0x3C, 0x67, 0x42, 0x55, 0x45, + 0xAD, 0x05, 0xB3, 0x0A, 0x3D, 0x89, 0x38, 0xDE, + }; + struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL }; + struct i2c_hid_platform_data *pdata = NULL; + union acpi_object params[4], *obj; + struct acpi_object_list input; + struct acpi_device *adev; + acpi_handle handle; + + handle = ACPI_HANDLE(client-dev); + if (!handle || acpi_bus_get_device(handle, adev)) + return NULL; + + input.count = ARRAY_SIZE(params); + input.pointer = params; + + params[0].type = ACPI_TYPE_BUFFER; + params[0].buffer.length = sizeof(i2c_hid_guid); + params[0].buffer.pointer = i2c_hid_guid; + params[1].type = ACPI_TYPE_INTEGER; + params[1].integer.value = 1; Can you confirm that any value here is ok (this is what I read from the DSDT example Microsoft gave, Arg1 is not used). + params[2].type = ACPI_TYPE_INTEGER; + params[2].integer.value = 1; /* HID function */ + params[3].type = ACPI_TYPE_INTEGER; + params[3].integer.value = 0; Again, the DSDT example showed that no Arg3 was used... But again, I never wrote ACPI driver, so I may be wrong. + + if (ACPI_FAILURE(acpi_evaluate_object(handle, _DSM, input, buf))) + return NULL; As you are returning NULL here, the error that will be raised is -EINVAL. I think it should be -ENODEV in this case. I have no strong opinion on this, but this can be achieved by returning the error from the function, and the returned i2c_hid_platform_data as an argument. Or maybe just an error message will be sufficient. + + obj = (union acpi_object *)buf.pointer; + if (obj-type != ACPI_TYPE_INTEGER) + goto fail; Again, I would like to know what happened here in case of a failure. So an error message would be good. + + pdata = devm_kzalloc(client-dev, sizeof(*pdata), GFP_KERNEL); + if (!pdata) + goto fail; idem (and returning -ENOMEM would be better IMHO). + + pdata-hid_descriptor_address = obj-integer.value; + +fail: + kfree(buf.pointer); + return pdata; +} + +static struct acpi_device_id i2c_hid_acpi_match[] = { + {ACPI0C50, 0 }, I never saw this _CID/_HID. Just out of curiosity, is this a standardized _CID or is it a _HID specific to a device? + {PNP0C50, 0 }, + { }, +}; +MODULE_DEVICE_TABLE(acpi, i2c_hid_acpi_match); +#else +static inline struct i2c_hid_platform_data * +i2c_hid_acpi_pdata(struct i2c_client *client) +{ + return NULL; +} +#endif + static int __devinit i2c_hid_probe(struct i2c_client *client, const struct i2c_device_id *dev_id) { @@ -822,8 +887,11 @@ static int __devinit i2c_hid_probe(struct i2c_client *client, dbg_hid(HID probe called for i2c 0x%02x\n, client-addr); if (!platform_data) { - dev_err(client-dev, HID register address not provided\n); - return -EINVAL; + platform_data = i2c_hid_acpi_pdata(client); + if (!platform_data) { +
Re: [PATCH] HID: i2c-hid: add ACPI support
On Wed, Jan 9, 2013 at 10:28 AM, Mika Westerberg mika.westerb...@linux.intel.com wrote: On Tue, Jan 08, 2013 at 10:55:59PM +0100, Benjamin Tissoires wrote: On Tue, Jan 8, 2013 at 7:09 PM, Mika Westerberg mika.westerb...@linux.intel.com wrote: On Tue, Jan 08, 2013 at 02:51:53PM +0100, Benjamin Tissoires wrote: Hi Mika, On Tue, Jan 8, 2013 at 2:05 PM, Mika Westerberg mika.westerb...@linux.intel.com wrote: The HID over I2C protocol specification states that when the device is enumerated from ACPI the HID descriptor address can be obtained by executing _DSM for the device with function 1. Enable this. Hehe, funny thing, I worked on the very same patch last Friday. I was not able to send it upstream as I still don't have an ACPI 5 enabled device... So thanks for submitting (and testing) this! Before the review, I just have a quick question. All the HID/i2c devices I saw so far present a reset line (through a GPIO). Does the shipped device you have present this line, and if yes, how this meld with the code (i.e. should we take this into account). Yes, there is either one or two GPIO lines. But there are also devices that don't have any GPIO lines. Ouch. But if they don't have any GPIO, how can they manage to send information? If they are using polling, then this will require some more work in i2c-hid :( Well they don't have GPIO resources because their interrupt is directly routed to the ioapic and exposed as an Interrupt resource. We probably should take this into account. I'm not too familiar with HID drivers but what if we set the hid-dev.acpi_node and let the actual HID driver to deal with the GPIO? Well, HID is a communication layer between the device and the kernel in order to make it agnostic of the transport layer (usb, bt or i2c). Even if some drivers do some transport tuning, I don't think it's a good idea to ask HID drivers to do transport handling such as setting up GPIOs. If they have reset GPIO or something like that, how else we should we handle this if not in the driver? The i2c-hid core doesn't know for what purpose a given GPIO line is. But the hid protocol aims at reducing the number of drivers. The idea is to built one driver to rule them all... :) Some stats: - hid-multitouch (the driver that drives multitouch screens) has a list of ~80 registered products, most of them are USB (few are Bluetooth). Bonus point, this list is not exhaustive because there is an auto-detection mechanism in place which forwards unknown mt-touchscreen to hid-multitouch. Now with i2c-hid, hid-multitouch can also handle any Win8 certified i2c touchscreen, so I guess it will add a lot of new devices to this list. - hid-core registered ~240 exception - it means that on all existing input devices that follow the hid protocol, only 240 need special handling. So my point is that some of the hid drivers may know the attached GPIOs, but the generic hid drivers (hid-multitouch, hid-generic and hid-core then) won't. Anyway, there may be adjustments at the beginning, but I still think we could add a common place for i2c quirks in i2c-hid, at least at the beginning. The best solution would be to be able to deduce the behavior of those GPIOs thanks to the ACPI description. Problem for me: I can't have access to the document referred in HID over i2c specification (Windows ACPI Design Guide for SOC Platform) while working at Red Hat... i2c-hid core can handle the GPIO interrupt if client-irq is not set (by converting the GPIO into interrupt number and passing it to the hid driver). I didn't implement that because we have the client-irq already set so I can't test this. But this part of the code is already in ACPI i2c enumeration, right? So why you want to rewrite it in i2c-hid? The closest thing which is already in the kernel tree is the handling of device specific quirks in usbhid. We may be forced to do such a thing if the DSDT is not explicit enough to guess the right behavior (how to trigger the reset line, etc..) Also, I missed one point in my previous review: Please note that I can only compare this to my patch, based on the DSDT example Microsoft gave in its spec. So sorry if I'm asking useless things, but I like to understand... :) Here are a few comments: Signed-off-by: Mika Westerberg mika.westerb...@linux.intel.com --- drivers/hid/i2c-hid/i2c-hid.c | 73 +++-- 1 file changed, 71 insertions(+), 2 deletions(-) diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c index 9ef22244..b2eebb6 100644 (...snipped...) + + pdata = devm_kzalloc(client-dev, sizeof(*pdata), GFP_KERNEL); Here, I don't think mixing devm_* and regular allocations is a good thing. I know it's a pain, but at the time I wrote the driver, the input layer was not devm-ized, so I was not able to use devm allocs. Now it should be feasible, but I
Re: [PATCH v2] HID: i2c-hid: add ACPI support
Hi Mika, On Wed, Jan 9, 2013 at 3:43 PM, Mika Westerberg mika.westerb...@linux.intel.com wrote: The HID over I2C protocol specification states that when the device is enumerated from ACPI the HID descriptor address can be obtained by executing _DSM for the device with function 1. Enable this. Signed-off-by: Mika Westerberg mika.westerb...@linux.intel.com I'm eager to test it (hopefully next week). Reviewed-by: Benjamin Tissoires benjamin.tissoi...@redhat.com Thanks, Benjamin --- Changes to previous version: * platform data is embedded into struct i2c_hid * i2c_hid_acpi_pdata() returns error codes or 0 on success * i2c_hid_acpi_pdata() prints out errors if _DSM execution fails or returns something we don't understand * constify i2c_hid_acpi_match * set the hid device ACPI handle We still don't handle the GpioInt in case client-irq is not set but I didn't want to implement it now as I'm unable to test it. So this only works if the ACPI device has an Interrupt or IRQ resource set. drivers/hid/i2c-hid/i2c-hid.c | 87 ++--- 1 file changed, 81 insertions(+), 6 deletions(-) diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c index 9ef22244..9e46e42 100644 --- a/drivers/hid/i2c-hid/i2c-hid.c +++ b/drivers/hid/i2c-hid/i2c-hid.c @@ -34,6 +34,7 @@ #include linux/kernel.h #include linux/hid.h #include linux/mutex.h +#include linux/acpi.h #include linux/i2c/i2c-hid.h @@ -139,6 +140,8 @@ struct i2c_hid { unsigned long flags; /* device flags */ wait_queue_head_t wait; /* For waiting the interrupt */ + + struct i2c_hid_platform_data pdata; }; static int __i2c_hid_command(struct i2c_client *client, @@ -810,6 +813,70 @@ static int __devinit i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid) return 0; } +#ifdef CONFIG_ACPI +static int i2c_hid_acpi_pdata(struct i2c_client *client, + struct i2c_hid_platform_data *pdata) +{ + static u8 i2c_hid_guid[] = { + 0xF7, 0xF6, 0xDF, 0x3C, 0x67, 0x42, 0x55, 0x45, + 0xAD, 0x05, 0xB3, 0x0A, 0x3D, 0x89, 0x38, 0xDE, + }; + struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL }; + union acpi_object params[4], *obj; + struct acpi_object_list input; + struct acpi_device *adev; + acpi_handle handle; + + handle = ACPI_HANDLE(client-dev); + if (!handle || acpi_bus_get_device(handle, adev)) + return -ENODEV; + + input.count = ARRAY_SIZE(params); + input.pointer = params; + + params[0].type = ACPI_TYPE_BUFFER; + params[0].buffer.length = sizeof(i2c_hid_guid); + params[0].buffer.pointer = i2c_hid_guid; + params[1].type = ACPI_TYPE_INTEGER; + params[1].integer.value = 1; + params[2].type = ACPI_TYPE_INTEGER; + params[2].integer.value = 1; /* HID function */ + params[3].type = ACPI_TYPE_INTEGER; + params[3].integer.value = 0; + + if (ACPI_FAILURE(acpi_evaluate_object(handle, _DSM, input, buf))) { + dev_err(client-dev, device _DSM execution failed\n); + return -ENODEV; + } + + obj = (union acpi_object *)buf.pointer; + if (obj-type != ACPI_TYPE_INTEGER) { + dev_err(client-dev, device _DSM returned invalid type: %d\n, + obj-type); + kfree(buf.pointer); + return -EINVAL; + } + + pdata-hid_descriptor_address = obj-integer.value; + + kfree(buf.pointer); + return 0; +} + +static const struct acpi_device_id i2c_hid_acpi_match[] = { + {ACPI0C50, 0 }, + {PNP0C50, 0 }, + { }, +}; +MODULE_DEVICE_TABLE(acpi, i2c_hid_acpi_match); +#else +static inline int i2c_hid_acpi_pdata(struct i2c_client *client, + struct i2c_hid_platform_data *pdata) +{ + return -ENODEV; +} +#endif + static int __devinit i2c_hid_probe(struct i2c_client *client, const struct i2c_device_id *dev_id) { @@ -821,11 +888,6 @@ static int __devinit i2c_hid_probe(struct i2c_client *client, dbg_hid(HID probe called for i2c 0x%02x\n, client-addr); - if (!platform_data) { - dev_err(client-dev, HID register address not provided\n); - return -EINVAL; - } - if (!client-irq) { dev_err(client-dev, HID over i2c has not been provided an Int IRQ\n); @@ -836,11 +898,22 @@ static int __devinit i2c_hid_probe(struct i2c_client *client, if (!ihid) return -ENOMEM; + if (!platform_data) { + ret = i2c_hid_acpi_pdata(client, ihid-pdata); + if (ret) { + dev_err(client-dev
Re: [PATCH v2] HID: i2c-hid: add ACPI support
On Wed, Jan 9, 2013 at 9:52 AM, Jiri Kosina jkos...@suse.cz wrote: On Wed, 9 Jan 2013, Benjamin Tissoires wrote: mika.westerb...@linux.intel.com wrote: The HID over I2C protocol specification states that when the device is enumerated from ACPI the HID descriptor address can be obtained by executing _DSM for the device with function 1. Enable this. Signed-off-by: Mika Westerberg mika.westerb...@linux.intel.com I'm eager to test it (hopefully next week). Reviewed-by: Benjamin Tissoires benjamin.tissoi...@redhat.com Perfect. I will wait for your Tested-by: as well before applying to public git tree. Hi Jiri, well, we tried yesterday to make all those things working, but the Haswell platform I was able to have access was not very cooperative. So I wasn't able to test the whole autodetection mechanism through ACPI (I even not managed to communicate with the various HID over i2c devices that were available to test). However, Mika managed to test this ACPI stuff, and he is able to retrieve the HID descriptor address. Given that, I think it's safe to include it right now. There will still be the last point with the gpios, but it will come when someone get access to a device presenting this feature. Cheers, Benjamin Thanks to both of you, -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] enumeration of composite devices using v-usb
On Fri, Jan 18, 2013 at 5:56 PM, Hartmut Knaack knaac...@gmx.de wrote: Hi, I've built myself a joystick adapter (project website: http://www.hexagons.de/index.php/USB_Joystickadapter), which has the purpose of connecting up to 2 Atari style joysticks (the ones with db9 connectors, found on 80's home computers like C64, Amiga,...) via USB. Current situation is: on Windows XP, 2 joystick devices get created with 2 axis and 2 buttons each (as intended); while on Linux, just one joystick device (/dev/input/js0) gets created, but with 4 axis and 4 buttons. This device provides one interface with one Interrupt-in Endpoint. The division into two joystick devices is done using an HID descriptor [1] with two configurations and the REPORT_ID tag. What I would like to know is, is there a problem in the Linux HID parser, or is its behavior intentional and those composite devices more like a dirty solution around the standards? Hi, well, currently, the HID parser does not split the different reportID into several devices. It's a known limitation and we are working on it (at least we already saw problems with that), but it will not make it in 3.9 I think. The solution that works for now is to use several usb interfaces. The usb layer splits the different interfaces, so it will give you 2 different HID devices. Oh, and if you want to contribute to split the device in the HID parser under Linux, you are welcome as well :) Cheers, Benjamin Thanks Hartmut Knaack [1] char usbHidReportDescriptor[102] PROGMEM = { // Joystick Port 1 0x05, 0x01,// USAGE_PAGE (Generic Desktop) 0x09, 0x04,// USAGE (Joystick) 0xa1, 0x01,// COLLECTION (Application) 0x85, 0x01,// REPORT_ID (1) 0x09, 0x01,// USAGE (Pointer) 0xa1, 0x00,// COLLECTION (Physical) 0x09, 0x30,// USAGE (X) 0x09, 0x31,// USAGE (Y) 0x15, 0x00,// LOGICAL_MINIMUM (0) 0x26, 0xff, 0x00, // LOGICAL_MAXIMUM (255) 0x75, 0x08,// REPORT_SIZE (8) 0x95, 0x02,// REPORT_COUNT (2) 0x81, 0x02,// INPUT (Data,Var,Abs) 0xc0, // END_COLLECTION 0x05, 0x09,// USAGE_PAGE (Button) 0x19, 0x01,// USAGE_MINIMUM (Button 1) 0x29, 0x02,// USAGE_MAXIMUM (Button 2) 0x15, 0x00,// LOGICAL_MINIMUM (0) 0x25, 0x01,// LOGICAL_MAXIMUM (1) 0x75, 0x01,// REPORT_SIZE (1) 0x95, 0x02,// REPORT_COUNT (2) 0x81, 0x02,// INPUT (Data,Var,Abs) 0x75, 0x06,// REPORT_SIZE (6) 0x95, 0x01,// REPORT_COUNT (1) 0x81, 0x03,// INPUT (Constant,Var,Abs) 0xc0, // END_COLLECTION // Joystick Port 2 0x05, 0x01,// USAGE_PAGE (Generic Desktop) 0x09, 0x04,// USAGE (Joystick) 0xa1, 0x01,// COLLECTION (Application) 0x85, 0x02,// REPORT_ID (2) 0x09, 0x01,// USAGE (Pointer) 0xa1, 0x00,// COLLECTION (Physical) 0x09, 0x30,// USAGE (X) 0x09, 0x31,// USAGE (Y) 0x15, 0x00,// LOGICAL_MINIMUM (0) 0x26, 0xff, 0x00, // LOGICAL_MAXIMUM (255) 0x75, 0x08,// REPORT_SIZE (8) 0x95, 0x02,// REPORT_COUNT (2) 0x81, 0x02,// INPUT (Data,Var,Abs) 0xc0, // END_COLLECTION 0x05, 0x09,// USAGE_PAGE (Button) 0x19, 0x01,// USAGE_MINIMUM (Button 1) 0x29, 0x02,// USAGE_MAXIMUM (Button 2) 0x15, 0x00,// LOGICAL_MINIMUM (0) 0x25, 0x01,// LOGICAL_MAXIMUM (1) 0x75, 0x01,// REPORT_SIZE (1) 0x95, 0x02,// REPORT_COUNT (2) 0x81, 0x02,// INPUT (Data,Var,Abs) 0x75, 0x06,// REPORT_SIZE (6) 0x95, 0x01,// REPORT_COUNT (1) 0x81, 0x03,// INPUT (Constant,Var,Abs) 0xc0 // END_COLLECTION }; -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to
Re: Sony NSG-MR5-U / NSG-MR5-E
Hi Jason, On Wed, Jan 23, 2013 at 6:41 AM, Jason Flatt jfl...@cox.net wrote: I am trying to add the newer Sony Google TV remote, and am looking for some advice on how to get this implemented. This is my first experience with hid drivers and multitouch. Some base info: Bluetooth only Keyboard Multitouch trackpad Accelerometer http://discover.store.sony.com/googletv/#remote Nice, seems pretty interesting. Below is the original report descriptor with my annotations. In kernel 3.6.11 the keyboard works great, but the trackpad and accelerometer don't work out of the box. I can get the trackpad to work as a one-button mouse by rewriting the report descriptor in a custom hid device module. Ok, I have several comments on the report descriptor. See later. I am thinking the 0xff01 usage page is what keeps it from being detected as a pointing device to begin with, requiring a rewrite of the report descriptor. Is this kind of approach frowned upon? You won't be able to activate both touches with just a rewrite of the report descriptor. The protocol in use here is not standard (this is why they put the vendor usage page). There are some quirks that I believe might make this device unusual compared to others. The entire trackpad surface acts as a mechanical button, i.e. the trackpad physically pushes into the remote body, so using tapping movements may be confusing to the user. Some synaptics devices and all the new trackpads from apple are using this kind of clickpads. So it's not new here, and distributions already now how to handle them. The remote also has a 'drag' button that acts like a mouse button but is only active when held down and there is trackpad motion. I think it was intended for one-handed dragging, so would have to utilize some kind of dragging-on/dragging-off state. I understand the individual mouse reports, which always contain X and Y absolute values for both contact points. There are two values that seem to be some sort of contact identifier, we'll call them a and b. With no fingers touching, a=0, b=0. With just the first finger touching, a=1, b=0. With both the first and second finger touching, a=2, b=2. Then, with just the second finger touching, a=0, b=1. Has this been seen before? Would that be used to translate into a contact identifier? definitively not a contact identifier. It could be translated to a contact count. But as it's a proprietary protocol, you can rely on it to know which contact is active and which is not. The accelerometer doesn't seem to show any events, I saw the sixaxis driver needs some kind of set_operational command before it begins working, I'd have to investigate if that sort of thing is necessary for this remote control. If I get the accelerometer working, do I leave it as-is, and leave the implementation up to userspace? What would the accelerometer do? Mimic a mouse? Mimic a joystick? I still have not investigated accelerometers, so I'm not 100% sure of my answer. I don't think accelerometers mimic a mouse or a joystick. There is a work on sensor hub this time, but I'm not sure about the protocol they use. Also, the remote has a power button and when it is pressed the bluetooth connection is dropped, just the same as when it goes to sleep. Is there a way of handling the power button? Is that something that would have to happen on the bluez, or bluetooth side of things? If you don't see any hid events, maybe not. This is something the bluetooth guys could answer. One more thing, the report ids 5, 6, and 8. Any idea what those could be? They have output values, and there are 3 LEDs on the device, including a data indicator and two Fn lights, but the amount of data in those report descriptors are quite large. Yes, they contain output reports. It means that they are used to send data back to the device. As the usage page is set to Generic Device Reserved, we can not guess what they are related to without capturing some frames of the device. Does anybody have an example rdesc of a similar multitouch trackpad that I could use in my custom rdesc to enable multitouch? No rdesc I know will work with your device. Moreover, you are encountering the current limitation of the hid parser which is not able to split the different reportID into several hid devices, which means that currently, you will have to write one big hid kernel driver that will handle all the different reportIDs. Thanks for the help. 05 01// USAGE_PAGE (Generic Desktop) 09 06// USAGE (Keyboard) a1 01// COLLECTION (Application) 85 01// REPORT_ID (1) 75 01// REPORT_SIZE (1) 95 08// REPORT_COUNT (8) 05 07// USAGE_PAGE (Keyboard/Keypad) 19 e0// USAGE_MINIMUM (224) - Left ctrl 29 e7// USAGE_MAXIMIUM (231) - Right GUI 15 00// LOGICAL_MINIMUM (0) 25 01// LOGICAL_MAXIMUM (1) 81 02// INPUT (Data,Variable,Absolute) - Usual
Re: [PATCH 02/25] HID: multitouch: add support for Nexio 42 panel
On Mon, Jan 28, 2013 at 4:01 PM, Henrik Rydberg rydb...@euromail.se wrote: Hi Benjamin, This device is the worst device I saw. It keeps TipSwitch and InRange at 1 for fingers that are not touching the panel. The solution is to rely on the field ContactCount, which is accurate as the correct information are packed at the begining of the frame. Unfortunately, CountactCount is most of the time at the end of the report. The solution is to pick it when we have the whole report in raw_event. Signed-off-by: Benjamin Tissoires benjamin.tissoi...@gmail.com --- drivers/hid/hid-ids.h| 3 ++ drivers/hid/hid-multitouch.c | 91 2 files changed, 78 insertions(+), 16 deletions(-) I think it would make more sense to introduce a method where the driver sees all report values at once. We have had reasonable cause to add it in the past, but never did. I will definitively look into it. It seems a fair idea. diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h index dad56aa..0935012 100644 --- a/drivers/hid/hid-ids.h +++ b/drivers/hid/hid-ids.h @@ -597,6 +597,9 @@ #define USB_VENDOR_ID_NEC0x073e #define USB_DEVICE_ID_NEC_USB_GAME_PAD 0x0301 +#define USB_VENDOR_ID_NEXIO 0x1870 +#define USB_DEVICE_ID_NEXIO_MULTITOUCH_420 0x010d + #define USB_VENDOR_ID_NEXTWINDOW 0x1926 #define USB_DEVICE_ID_NEXTWINDOW_TOUCHSCREEN 0x0003 diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index 46d8136..c4acdd0 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -32,6 +32,8 @@ #include linux/slab.h #include linux/usb.h #include linux/input/mt.h +#include asm/unaligned.h +#include asm/byteorder.h #include usbhid/usbhid.h @@ -54,6 +56,7 @@ MODULE_LICENSE(GPL); #define MT_QUIRK_NO_AREA (1 9) #define MT_QUIRK_IGNORE_DUPLICATES (1 10) #define MT_QUIRK_HOVERING(1 11) +#define MT_QUIRK_CONTACT_CNT_ACCURATE(1 12) struct mt_slot { __s32 x, y, cx, cy, p, w, h; @@ -83,6 +86,10 @@ struct mt_device { struct mt_class mtclass;/* our mt device class */ struct mt_fields *fields; /* temporary placeholder for storing the multitouch fields */ + struct hid_field *contactcount; /* the hid_field contact count that +will be picked in mt_raw_event */ + __s8 contactcount_index;/* the index of the usage contact count +in its hid_field. */ unsigned last_field_index; /* last field index of the report */ unsigned last_slot_field; /* the last field of a slot */ __s8 inputmode; /* InputMode HID feature, -1 if non-existent */ @@ -111,6 +118,7 @@ struct mt_device { #define MT_CLS_DUAL_INRANGE_CONTACTNUMBER0x0007 #define MT_CLS_DUAL_NSMU_CONTACTID 0x0008 #define MT_CLS_INRANGE_CONTACTNUMBER 0x0009 +#define MT_CLS_ALWAYS_TRUE 0x000a /* vendor specific classes */ #define MT_CLS_3M0x0101 @@ -170,6 +178,9 @@ static struct mt_class mt_classes[] = { { .name = MT_CLS_INRANGE_CONTACTNUMBER, .quirks = MT_QUIRK_VALID_IS_INRANGE | MT_QUIRK_SLOT_IS_CONTACTNUMBER }, + { .name = MT_CLS_ALWAYS_TRUE, + .quirks = MT_QUIRK_ALWAYS_VALID | + MT_QUIRK_CONTACT_CNT_ACCURATE }, /* * vendor specific classes @@ -250,6 +261,9 @@ static ssize_t mt_set_quirks(struct device *dev, td-mtclass.quirks = val; + if (!td-contactcount) + td-mtclass.quirks = ~MT_QUIRK_CONTACT_CNT_ACCURATE; + return count; } @@ -264,24 +278,26 @@ static struct attribute_group mt_attribute_group = { .attrs = sysfs_attrs }; +static int mt_find_usage_index(struct hid_field *field, struct hid_usage *usage) +{ + int i; + for (i = 0; i field-maxusage; i++) { + if (field-usage[i].hid == usage-hid) + return i; + } + return -1; +} + static void mt_feature_mapping(struct hid_device *hdev, struct hid_field *field, struct hid_usage *usage) { struct mt_device *td = hid_get_drvdata(hdev); - int i; switch (usage-hid) { case HID_DG_INPUTMODE: td-inputmode = field-report-id; - td-inputmode_index = 0; /* has to be updated below */ - - for (i=0; i field-maxusage; i++) { - if (field-usage[i].hid == usage-hid) { - td-inputmode_index = i; - break; - } - } - + td-inputmode_index = mt_find_usage_index(field, usage); + /* inputmode_index can't be set at -1 */ break
Re: [PATCH 00/25] Support of Nexio 42 and new default class for hid-multitouch
Hi Henrik, On Mon, Jan 28, 2013 at 4:23 PM, Henrik Rydberg rydb...@euromail.se wrote: Hi Benjamin, finally, I managed to send a new bunch of patches. Sorry for the delay from the previous version, but meanwhile, I implemented an automatic regressions tests for hid device [1]. So this series seems pretty big, but it does not break any known devices (I ran 40 successful tests for this series)[2]. Thanks for the patches. And thanks for reviewing. To sum up: - Nexio devices were problematic in the sense they use out of range values for some of the fields, and consider that the driver won't treat the extra touches based on the reported contact count. Problematic device, but I think we should add a new event function which gives all values at the same time, since those are already present in the core. It seems this will solve the current problem as well as many older workarounds. yeah, makes sense. I think it would also allows us to simplify the logic of hid-multitouch by removing some of the states we have in it. - fortunately, this behavior (relying on contact count) is compatible with all the devices I know, which leads to think that this is how the Windows 7/8 driver manage to handle such a different bunch of devices. This is a nice observation. IIRC, we used to rely more on contact count in the old drivers. - thanks to the automatic testing, I was able to fix broken devices (Sharp LC-20FE1-W screen 04dd:9681, Sitronix 1403:5001 and Cando 2087:0a02) and optimize many others. In order to allow a bisection to be done, I split the patches in many different ones, one per device type. Great tool, thank you Benjamin. Once I will do the work on the suppression of usbhid direct use, we also could rely on that for every HID devices, not only hid-multitouch. - finally, I changed the default class in order to handle the new devices in a better way. Old wisdom says differently. ;-) No: old wisdom says the exact same thing :) I did not break the current supported devices. I _kept_ the old default class for all the current supported devices, and I used the new default class only for the new devices, the one that are not registered. Cheers, Benjamin Thanks, Henrik -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] HID: i2c-hid: fix i2c_hid_output_raw_report
On Thu, Jan 31, 2013 at 5:58 PM, Jiri Kosina jkos...@suse.cz wrote: On Thu, 31 Jan 2013, Benjamin Tissoires wrote: i2c_hid_output_raw_report is used by hidraw to forward set_report requests. The current implementation of i2c_hid_set_report needs to take the report_id as an argument. The report_id is stored in the first byte of the buffer in argument of i2c_hid_output_raw_report. Not removing the report_id from the given buffer adds this byte 2 times in the command, leading to a non working command. Reported-by: Andrew Duggan adug...@synaptics.com Signed-off-by: Benjamin Tissoires benjamin.tissoi...@gmail.com Applied, will push it for 3.8 still. Thanks, Thanks Jiri! Cheers, Benjamin -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/9] Support of Nexio 42 and new default class for hid-multitouch
Hi Henrik, On Sun, Feb 3, 2013 at 2:07 PM, Henrik Rydberg rydb...@euromail.se wrote: Hi Benjamin, so, this is the v2 of the support of win7/8 devices. Looks like it is getting there, thanks. Thanks for the review. However, before sending a new patch series, I'd like to have your answers to my comments as I mostly disagree on everything :) Thanks again, Benjamin changes since v1: - removed the optimization patches, as the benefit was minimum - introduce a new callback report in hid-core that drivers can use to treat the report by having it entirely parsed - rely on this new hook to support Nexio 42 As noted in the patches comments, using raw_event() seems sufficient. side notes: - I've tested removing the heavy call to kzalloc in hid_input_field. The results are disapointing - the processing time remains the same. - I've also tested not to rely on .event hook in hid-multitouch but only on .report. Idem, I thought it would reduce the code of hid-multitouch and will enhance its processing time, but the results are a roughly same number of lines for hid-multitouch and the same processing time... :( - these 2 tests helped in cleaning the patch set from the last time. And again, finally, I've pass all the 40 regression tests of my db. \o/ Nice. :-) Thanks, Henrik -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/9] HID: core: add report hook, called once the report has been parsed
On Mon, Feb 4, 2013 at 10:34 AM, Jiri Kosina jkos...@suse.cz wrote: On Sun, 3 Feb 2013, Henrik Rydberg wrote: This callback is called when the parsing of the report has been done by hid-core (so after the calls to .event). The hid drivers can now have access to the whole report by relying on the values stored in the different fields. Signed-off-by: Benjamin Tissoires benjamin.tissoi...@gmail.com --- drivers/hid/hid-core.c | 4 include/linux/hid.h| 2 ++ 2 files changed, 6 insertions(+) diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index 5ae2cb1..b671e4e 100644 --- a/drivers/hid/hid-core.c +++ b/drivers/hid/hid-core.c @@ -1195,6 +1195,7 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size, { struct hid_report_enum *report_enum = hid-report_enum + type; struct hid_report *report; + struct hid_driver *hdrv; unsigned int a; int rsize, csize = size; u8 *cdata = data; @@ -1231,6 +1232,9 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size, if (hid-claimed != HID_CLAIMED_HIDRAW) { for (a = 0; a report-maxfield; a++) hid_input_field(hid, report-field[a], cdata, interrupt); + hdrv = hid-driver; + if (hdrv hdrv-report) + hdrv-report(hid, report); I think this is more useful if called before the individual fields. In fact, it seems raw_event() is already doing exactly that. No need for a new callback, in other words. raw_event() doesn't provide equivalent functionality though; namely, the report is not parsed. Or have I missed your point? No, you perfectly understood the purpose of the patch. raw_event() and report() are not the same kind of callbacks at all. Thanks for the extensive review, Henrik, it's really helpful. Yep, thanks Henrik, and thanks Jiri for having a look at it. Cheers, Benjamin -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/9] HID: multitouch: add support for Nexio 42 panel
On Mon, Feb 4, 2013 at 12:42 PM, Henrik Rydberg rydb...@euromail.se wrote: Hi Benjamin, Why not an index here? Just because an index is not sufficient. You need two things: an index within the field, and the actual field (a pointer to a struct hid_field). Each .value is local to a field, and even if in most of the case, the contact count is alone in its field, it would mean to take the risk that a new device does not follow this logic. The field value is passed to process_mt_event() in a fairly straight-forward fashion, I was imagining that behavior could be copied somehow. So the actual pointer to the contact count value seemed to be the shortest way to do it. But it can be easily changed. Keeping a pointer into the core structure creates unwanted dependencies to the scope of that value, making an eventual core refactoring even harder, not to mention trickier to debug. So even though it looks neat in the code, it pushes the problem forward. @@ -251,6 +257,9 @@ static ssize_t mt_set_quirks(struct device *dev, td-mtclass.quirks = val; + if (!td-contactcount) + td-mtclass.quirks = ~MT_QUIRK_CONTACT_CNT_ACCURATE; + Why override the overrider here? This callback is called from the user-space through the sysfs attribute. So, it is not called in the same time that the mt_post_parse function. This is just to avoid a user setting this quirk once the device is up and running leading to a potential oops. Yes, but the quirk _is_ user modifiable. Hence, the problem lies in equating the user-modifiable quirk with the branch control of the program. I'm not sure I understood what you meant there. The quirk is indeed user modifiable, but through the callback mt_set_quirks() only. If the HID field Contact Count is not present, this quirk should not be allowed to be present, thus the two removals of the quirk in met_set_quirks() and mt_post_parse(). As there are no other entry points, I'm quite confuse to understand where the pitfall is. An index into the the struct actually passed in mt_report() feels safer. again, you need to store field and usage-usage_index. Agree, it would be safer but it will take more space... :) If you think the code change is not only correct, but also moves the whole code base in a good direction, by all means. Ok, will do. After a deeper look at it, I can even have two int indexes (and no pointers): one for the field and one other for the value within the field. Cheers, Benjamin @@ -750,11 +765,15 @@ static void mt_post_parse_default_settings(struct mt_device *td) static void mt_post_parse(struct mt_device *td) { struct mt_fields *f = td-fields; + struct mt_class *cls = td-mtclass; if (td-touches_by_report 0) { int field_count_per_touch = f-length / td-touches_by_report; td-last_slot_field = f-usages[field_count_per_touch - 1]; } + + if (!td-contactcount) + cls-quirks = ~MT_QUIRK_CONTACT_CNT_ACCURATE; Since MT_QUIRK_CONTACT_CNT_ACCURATE is a quirk, modifiable by the user, it should probably not validate num_expected in the code. Better use the contact count index or something equivalent for that. As when the user changes the quirk, we validate it, this is not required. True, barring the comments above. Thanks, Henrik -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/9] Support of Nexio 42 and new default class for hid-multitouch
On Tue, Feb 5, 2013 at 12:13 PM, Jiri Kosina jkos...@suse.cz wrote: On Mon, 4 Feb 2013, Henrik Rydberg wrote: Thanks for the review. However, before sending a new patch series, I'd like to have your answers to my comments as I mostly disagree on everything :) With good reason, apparently. :-) I see no major problem with your patches, although the discussed details show that there is room for some refactoring. Thanks to both of you. As I don't object to the HID core change, I have now applied the patchset, so please send any further additions on top of for-3.9/multitouch branch of my tree. Ok. Thanks Jiri. I will send the patch in a minute. Also, Benjamin, perhaps it'd make sense to put a link somewhere into in-tree documentation, with pointer to your testing suite? Good idea. However, I prefer removing the dependencies between hid drivers and usbhid before including such reference to avoid testers getting kernel oops while trying to access the usb layer from the uhid device... So, yesterday and this morning, I rebased/updated Henrik's patches on this topic. They should be ready soon. Do you mind if I send the usbhid dependency and the pen+multitouch series this week, or we are too close to the 3.9 opening window? Cheers, Benjamin -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/9] Support of Nexio 42 and new default class for hid-multitouch
Do you mind if I send the usbhid dependency and the pen+multitouch series this week, or we are too close to the 3.9 opening window? Please just send the patches, and let's see whether they will make it for 3.8 or I'll queue (some of) them for 3.9. Well, I didn't wanted to push them into 3.8 (the rc6 is already there, and I doubt Linus would be happy to see a lot of patches now). I was asking if it was not too late to schedule them for 3.9, or if you'd rather wait for 3.10 :) Anyway, I'm doing my best to push them soon. Cheers, Benjamin -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] HID: extend autodetect to handle I2C sensors as well
On Mon, Feb 11, 2013 at 11:31 AM, Mika Westerberg mika.westerb...@linux.intel.com wrote: Since the advent of HID over I2C protocol, it is possible to have sensor hubs behind I2C bus as well. We can autodetect this in a same way than USB sensor hubs. Signed-off-by: Mika Westerberg mika.westerb...@linux.intel.com Reviewed-by: Benjamin Tissoires benjamin.tissoi...@redhat.com Cheers, Benjamin -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] HID: sensor-hub: get rid of unused sensor_hub_grabbed_usages[] table
On Mon, Feb 11, 2013 at 11:31 AM, Mika Westerberg mika.westerb...@linux.intel.com wrote: This table is not used anywhere in the driver so kill it. Signed-off-by: Mika Westerberg mika.westerb...@linux.intel.com --- Yep, it's rather strange that the compiler didn't complained... Anyway, Reviewed-by: Benjamin Tissoires benjamin.tissoi...@redhat.com Cheers, Benjamin -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] HID: sensor-hub: don't limit the driver only to USB bus
On Mon, Feb 11, 2013 at 11:31 AM, Mika Westerberg mika.westerb...@linux.intel.com wrote: We now have two transport mediums: USB and I2C, where sensor hubs can exists. So instead of constraining the driver to only these two we let it to match any HID bus as long as the group is HID_GROUP_SENSOR_HUB. Signed-off-by: Mika Westerberg mika.westerb...@linux.intel.com Reviewed-by: Benjamin Tissoires benjamin.tissoi...@redhat.com Thanks Mika for this series! Cheers, Benjamin -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] HID: sensor-hub: get rid of unused sensor_hub_grabbed_usages[] table
On Mon, Feb 11, 2013 at 4:53 PM, Pandruvada, Srinivas srinivas.pandruv...@intel.com wrote: This was added to load sensor hub driver on USB plug in without explicit modprobe. But with last few changes to remove vendor and product id, we need to do modprobe this driver. So removal is OK. I just figure your comment out: the change does not seem to have anything to do with hotplugging. .usage_table is used to decide which HID reports to forwards to the .event() callback, and NULL means all events. So even if it was in use, there are no functional changes. Thanks to the changes to remove the vendor id and product id, you shouldn't have to manually modprobe the driver as the hid-core subsystem will tag the hid device as a sensor one, meaning that udev will modprobe the driver if it's not already loaded. So if you still have to manually modprobe your driver (or maybe you are talking about the mfd driver), there is a bug in the detection of sensors devices. And this has to be fixed :) Cheers, Benjamin -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/7] HID: multitouch: remove usbhid dependency
Hi Henrik, On Wed, Feb 20, 2013 at 9:25 PM, Henrik Rydberg rydb...@euromail.se wrote: Hi Benjamin, Signed-off-by: Benjamin Tissoires benjamin.tissoi...@gmail.com --- drivers/hid/hid-multitouch.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) Since removing the transport layer dependency has a rationale, it might be good to mention that here. Ok, will do. Also, what about the explicit usb dependency, was that going to be moved to a usbhid quirk? Currently, the USB dependency is protected by a check on the BUS. The problem is that the BUS can be faked (by uhid). So yes, I'll add a quirk under usbhid for those devices. Cheers, Benjamin diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index 32258ba..184ac0a 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -32,7 +32,6 @@ #include linux/slab.h #include linux/usb.h #include linux/input/mt.h -#include usbhid/usbhid.h MODULE_AUTHOR(Stephane Chatty cha...@enac.fr); @@ -907,7 +906,7 @@ static int mt_resume(struct hid_device *hdev) intf = to_usb_interface(hdev-dev.parent); interface = intf-cur_altsetting; - dev = hid_to_usb_dev(hdev); + dev = interface_to_usbdev(intf); /* Some Elan legacy devices require SET_IDLE to be set on resume. * It should be safe to send it to other devices too. -- 1.8.1 Thanks, Henrik -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 4/8] HID: use hid_hw_request() instead of direct call to usbhid
This allows the hid drivers to be independent from the transport layer. The patch was constructed by replacing all occurences of usbhid_submit_report() by its hid_hw_request() counterpart. Then, drivers not requiring USB_HID anymore have their USB_HID dependency cleaned in the Kconfig file. Finally, few drivers still depends on USB_HID. Many of them are requiring the io wait callback. They are found in the next patch. Signed-off-by: Benjamin Tissoires benjamin.tissoi...@gmail.com Reviewed-by: Mika Westerberg mika.westerb...@linux.intel.com For the sensor-hub part: Tested-by: Mika Westerberg mika.westerb...@linux.intel.com --- drivers/hid/Kconfig | 30 - drivers/hid/hid-axff.c | 6 ++-- drivers/hid/hid-dr.c| 8 ++--- drivers/hid/hid-emsff.c | 6 ++-- drivers/hid/hid-gaff.c | 10 +++--- drivers/hid/hid-holtekff.c | 4 +-- drivers/hid/hid-kye.c | 4 +-- drivers/hid/hid-lenovo-tpkbd.c | 4 +-- drivers/hid/hid-lg2ff.c | 6 ++-- drivers/hid/hid-lg3ff.c | 6 ++-- drivers/hid/hid-lg4ff.c | 18 +-- drivers/hid/hid-lgff.c | 8 ++--- drivers/hid/hid-logitech-dj.c | 3 +- drivers/hid/hid-multitouch.c| 4 +-- drivers/hid/hid-ntrig.c | 6 ++-- drivers/hid/hid-picolcd.h | 4 +-- drivers/hid/hid-picolcd_backlight.c | 4 +-- drivers/hid/hid-picolcd_cir.c | 2 -- drivers/hid/hid-picolcd_core.c | 8 ++--- drivers/hid/hid-picolcd_debugfs.c | 2 -- drivers/hid/hid-picolcd_fb.c| 7 ++-- drivers/hid/hid-picolcd_lcd.c | 4 +-- drivers/hid/hid-picolcd_leds.c | 4 +-- drivers/hid/hid-pl.c| 6 ++-- drivers/hid/hid-prodikeys.c | 3 +- drivers/hid/hid-sensor-hub.c| 7 ++-- drivers/hid/hid-sjoy.c | 6 ++-- drivers/hid/hid-steelseries.c | 3 +- drivers/hid/hid-tmff.c | 6 ++-- drivers/hid/hid-zpff.c | 6 ++-- drivers/hid/usbhid/hid-core.c | 3 +- drivers/hid/usbhid/hid-pidff.c | 64 ++--- drivers/hid/usbhid/hiddev.c | 4 +-- drivers/hid/usbhid/usbhid.h | 2 -- 34 files changed, 111 insertions(+), 157 deletions(-) diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig index 13a1c5d..234954c 100644 --- a/drivers/hid/Kconfig +++ b/drivers/hid/Kconfig @@ -99,7 +99,7 @@ config HID_A4TECH config HID_ACRUX tristate ACRUX game controller support - depends on USB_HID + depends on HID ---help--- Say Y here if you want to enable support for ACRUX game controllers. @@ -151,7 +151,7 @@ config HID_CHICONY config HID_PRODIKEYS tristate Prodikeys PC-MIDI Keyboard support - depends on USB_HID SND + depends on HID SND select SND_RAWMIDI ---help--- Support for Prodikeys PC-MIDI Keyboard device support. @@ -173,7 +173,7 @@ config HID_CYPRESS config HID_DRAGONRISE tristate DragonRise Inc. game controller - depends on USB_HID + depends on HID ---help--- Say Y here if you have DragonRise Inc. game controllers. These might be branded as: @@ -192,7 +192,7 @@ config DRAGONRISE_FF config HID_EMS_FF tristate EMS Production Inc. force feedback support - depends on USB_HID + depends on HID select INPUT_FF_MEMLESS ---help--- Say Y here if you want to enable force feedback support for devices by @@ -215,7 +215,7 @@ config HID_EZKEY config HID_HOLTEK tristate Holtek HID devices - depends on USB_HID + depends on HID ---help--- Support for Holtek based devices: - Holtek On Line Grip based game controller @@ -239,7 +239,7 @@ config HID_KEYTOUCH config HID_KYE tristate KYE/Genius devices - depends on USB_HID + depends on HID ---help--- Support for KYE/Genius devices not fully compliant with HID standard: - Ergo Mouse @@ -397,7 +397,7 @@ config HID_MONTEREY config HID_MULTITOUCH tristate HID Multitouch panels - depends on USB_HID + depends on HID ---help--- Generic support for HID multitouch panels. @@ -458,7 +458,7 @@ config HID_ORTEK config HID_PANTHERLORD tristate Pantherlord/GreenAsia game controller - depends on USB_HID + depends on HID ---help--- Say Y here if you have a PantherLord/GreenAsia based game controller or adapter. @@ -592,13 +592,13 @@ config HID_SONY config HID_SPEEDLINK tristate Speedlink VAD Cezanne mouse support - depends on USB_HID + depends on HID ---help--- Support for Speedlink Vicious and Divine Cezanne mouse. config HID_STEELSERIES tristate Steelseries SRW-S1 steering wheel support - depends on USB_HID + depends
Re: [PATCH v2 0/8] hid driver transport cleanup
Hi David, On Mon, Feb 25, 2013 at 2:41 PM, David Herrmann dh.herrm...@gmail.com wrote: Hi guys On Mon, Feb 25, 2013 at 1:29 PM, Jiri Kosina jkos...@suse.cz wrote: On Mon, 25 Feb 2013, Benjamin Tissoires wrote: Hi guys, this is the v2 of the hid transport cleanup. Changes since v1: - gathered reviewed/acked/etc.. - changed commit messages of patches 4-6 - add newcomer into account (thingm) - incorporated the i2c-hid driver change into the series I still did not implemented the final usb cleanup for hid-multitouch as it may required few comments. I have now taken the series. Thanks Benjamin, thanks Henrik. I rewrote the Bluetooth HID session-management last week (patches pending on linux-bluetooth@vger) as it was horribly broken. This week I will try to fix the get_report/set_report mess with Bluetooth-HID, so I was wondering whether you could clear some things up. Sure, I'll do my best. I'm not sure that you will have a better idea after my answers because the use cases are very different and there is not a good rational for each use. (I did read the Bluetooth HID Profile specification but I have no idea how USB does it, so please bear with me if I mix things up. Maybe some day I will have the time to read the USB specs, too) * There are several drivers using dev-hid_get_raw_report() and dev-hid_output_raw_report(). Any objections to moving these to hid_ll_driver and adding wrapper functions hid_hw_get/output_raw_report()? I can't see any. Those two callbacks are transport dependent, so I also think it makes sense to put them into hid_ll_driver. I also wish we have the set/get hid idle callback (it would remove the last usb dependency in hid-multitouch). * What should the ll-report() callback exactly do? Should it simply send a GET_REPORT or SET_REPORT request depending on the direction (nonblocking)? Well, given the way it's used in hid drivers and usbhid/i2c-hid: - SET_REPORT takes an already parsed report and send it to the device - the usbhid part is nonblocking, i2c-hid is blocking. - GET_REPORT asks for the HID command GET_REPORT and once it's done, the incoming report is parsed as if it was spontaneously generated (meaning that there are no guarantees that the report the caller reads after report() is the requested one). The patch for implementing those functions in i2c-hid is IMHO simpler to understand than the usb one as i2c-hid makes no use of dma or async calls. For the nonblocking part, the usb pendant is asynchronous, thus the need of the .wait(). The i2c-hid driver is synchronous, meaning that the call is blocking (sorry). But .wait() is not required to be called, and then, we don't need to set it. * Why don't we do the hid_output_report() call in the hid_hw_report() helper and pass the raw buffers down? It would allow GFP_KERNEL and avoid duplicating code in all four backends. Well. I continued Henrik's work on that, and the idea was to not break any existing device. So, the code path should be the nearest possible to the existing one. Now that this part is done, we can look at something more efficient in terms of duplication. Actually, the implementation of i2c-hid .request() already uses the raw buffers commands, meaning that it's nearly what we need (except that we need to get the size of the buffer in some other way). However, in usbhid and i2c-hid, .hid_get_raw_report() and .hid_output_raw_report() are blocking, meaning that it will change the current behavior for drivers. * What should ll-wait() exactly do? Block until the last SET/GET_REPORT call has been ack'ed by the device? It waits for at most 10*HZ until both the CTRL (command queue for set_report/get_report and 90% of the hid communication) and the OUTPUT queues are empty. Honestly, I don't like the way the wait is done: nobody cares about the return value, meaning that nobody know why we have finished to wait. In 90% of the cases, it's used in conjunction with a .request() call. So to sum up, given its current use, I would say that .wait() blocks until all queues are flushed or for an amount of time that gives the caller good chances that his/her previous call has been transferred to the device. * Bluetooth-HID might return errors on GET/SET_REPORT. Why don't we return these in hid_hw_wait() for the last report? It would allow synchronous calls like: hid_hw_report(); ret = hid_hw_wait(); As there are several uses of .wait(), it seems difficult. Some are using it to make 'sure' that their previous command has been sent - in this case, I would say, yes, go for it. Some others are using it for it's real purpose: flush the queues before/after sending new commands - it's difficult to put the last command result as a return value in this case (the latest result does not mean anything to the caller). Maybe turning the hid_hw_report() for these cases into a blocking one would solve the problem. * Should hid_hw_report() allow multiple pending requests? Or should
Re: Question regarding multitouch input on Linux kernel
Hi Nuno, On Mon, Feb 25, 2013 at 4:00 PM, Nuno Santos nsan...@displax.com wrote: Hi, I have been experiencing an issue with a Linux driver for multitouch input that i'm responsible for maintaining. Side note. Your driver does not seems to be upstreamed (or I missed it). You should really consider put it upstream. If we make changes in the multitouch API, we can change your driver too, whereas here, you will have to maintain several releases of your driver, one per kernel version. The issue is basically the following: - I load the driver and the mouse works just fine - I touch the screen and the first touch input is delivered to the system - On that very same moment I can't use mouse left button down to click on folders on nautilus. I can only selected them using drag select. I also can't get a folder to get selected with a single touch input. - The user experience with the mouse gets inconsistent. - Unloading the module doesn't return the good experience - Restarting X fixes the problem until I report a touch input again with this driver - If I only use common pointer input, the issue doesn't occur. My questions resides in if the problem is due to bad touch reporting, or due to a bug in X/nautilus. Definitively X and nautilus problems. The very same kind of problems were observed in Fedora 17 and fixed in the X.org shipped in Fedora 18. Ubuntu is relying on an older X.org release, which contains several bugs related to multitouch. I have been analyzing a lot of examples under kernel tree for multitouch input under Linux an it seems I'm doing what is necessary. I'm using Ubuntu 12.04 but this happened with Ubuntu 11.XX already And in 12.10 also IIRC. I really hope that they will rebase X.org in 13.04. This what I do in order to declare device capabilities: input_set_abs_params(input_dev, ABS_X, 0, 6300, 0, 0); input_set_abs_params(input_dev, ABS_Y, 0, 6300, 0, 0); input_mt_init_slots(input_dev, DPX_TOUCH_MAX_COUNT); input_mt_init_slots has been changed recently, it takes an extra arg: 'flags'. input_set_abs_params(input_dev, ABS_MT_POSITION_X, 0, 6300, 0, 0); input_set_abs_params(input_dev, ABS_MT_POSITION_Y, 0, 6300, 0, 0); Not required IIRC. The params are copied from their single-touch equivalent. This is what I do in order to report touches: for (currentTouch=0;currentTouchDPX_TOUCH_MAX_COUNT touchCountpriv-context-State.Acquisition.TouchPoints;++currentTouch) { Touch = priv-context-State.Touches + currentTouch; x = Touch-CalibratedPoint.Position.X; y = Touch-CalibratedPoint.Position.Y; input_mt_slot(usbtouch-input, currentTouch); // touch down if(Touch-CurrentState==DPX_TOUCH_STATE_ACTIVE Touch-ReportedState==DPX_TOUCH_STATE_INACTIVE) { Touch-ReportedState = DPX_TOUCH_STATE_ACTIVE; input_mt_report_slot_state(usbtouch-input, MT_TOOL_FINGER, 1); input_report_abs(usbtouch-input, ABS_MT_POSITION_X, x); input_report_abs(usbtouch-input, ABS_MT_POSITION_Y, y); touchCount++; } // touch move else if (Touch-CurrentState==DPX_TOUCH_STATE_ACTIVE Touch-ReportedState==DPX_TOUCH_STATE_ACTIVE) { Touch-ReportedState = DPX_TOUCH_STATE_ACTIVE; input_mt_report_slot_state(usbtouch-input, MT_TOOL_FINGER, 1); input_report_abs(usbtouch-input, ABS_MT_POSITION_X, x); input_report_abs(usbtouch-input, ABS_MT_POSITION_Y, y); touchCount++; } // touch up else { Touch-ReportedState = DPX_TOUCH_STATE_INACTIVE; input_mt_report_slot_state(usbtouch-input, MT_TOOL_FINGER, 0); input_report_abs(usbtouch-input, ABS_MT_POSITION_X, x); input_report_abs(usbtouch-input, ABS_MT_POSITION_Y, y); No need to update ABS_MT_POSITION_X/Y in this case: they should not be sent to the user space according to the multitouch protocol. touchCount++; } } if (touchCount0) Looks like this test is always true. { input_mt_report_pointer_emulation(usbtouch-input, true); In the latest version of the kernel tree, you should rely on the input_mt_sync_frame() now. It will call input_mt_report_pointer_emulation() plus other things depending of the flags you passed to input_mt_init_slots(). input_sync(usbtouch-input); } Cheers, Benjamin -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/7] HID: multitouch: support of hybrid finger/pen devices
Hi guys, Here is the support of hybrid devices presenting pen and touch at the same time. I don't think it's possible to split the handling of the different reports by several hid drivers unless deepest changes in the HID core subsystem. The main problem is the control of the underlaying transport layer of the hid device. For instance, if two drivers (let's say hid-multitouch and hid-generic) handle the same device, and one of them is removed, then we should not call hid_hw_stop at this point, but only when the second is also removed. The other big problem lies with hid drivers that fix the hid report descriptors. We can not split the hid device before we get the fix, so those drivers should have a special behavior. To sum up, I think does not make sense to do such deep changes, and to take such a risk of breaking existing devices. So the solution consists in relying inconditionaly to the quirk MULTI_INPUT for hid-multitouch. Before registering the input device in hid-input, we can test if it has been populated, and if not, then we simply don't register it. In order to prevent a regression for drivers that do not fill the input device, we need to add an other quirk. Cheers, Benjamin Benjamin Tissoires (7): HID: input: don't register unmapped input devices HID: multitouch: breaks out touch handling in specific functions HID: multitouch: do not map usage from non used reports HID: multitouch: add handling for pen in dual-sensors device HID: multitouch: manually send sync event for pen input report HID: multitouch: append Pen to the name of the stylus input HID: multitouch: force BTN_STYLUS for pen devices drivers/hid/hid-input.c | 77 ++ drivers/hid/hid-multitouch.c | 151 --- include/linux/hid.h | 1 + 3 files changed, 207 insertions(+), 22 deletions(-) -- 1.8.1.2 -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 6/7] HID: multitouch: append Pen to the name of the stylus input
This is not just cosmetics, it can help to write udev and X.org rules. Signed-off-by: Benjamin Tissoires benjamin.tissoi...@redhat.com --- drivers/hid/hid-multitouch.c | 24 +++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index d89f0eb..faeec95 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -44,6 +44,7 @@ #include linux/slab.h #include linux/usb.h #include linux/input/mt.h +#include linux/string.h MODULE_AUTHOR(Stephane Chatty cha...@enac.fr); @@ -371,6 +372,15 @@ static int mt_pen_input_mapping(struct hid_device *hdev, struct hid_input *hi, unsigned long **bit, int *max) { struct mt_device *td = hid_get_drvdata(hdev); + char *name; + + if (hi-input-name == hdev-name) { + name = kzalloc(sizeof(hdev-name) + 5, GFP_KERNEL); + if (name) { + sprintf(name, %s Pen, hdev-name); + hi-input-name = name; + } + } td-pen_report_id = field-report-id; @@ -914,6 +924,7 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id) int ret, i; struct mt_device *td; struct mt_class *mtclass = mt_classes; /* MT_CLS_DEFAULT */ + struct hid_input *hi; for (i = 0; mt_classes[i].name ; i++) { if (id-driver_data == mt_classes[i].name) { @@ -964,7 +975,7 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id) ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT); if (ret) - goto fail; + goto hid_fail; ret = sysfs_create_group(hdev-dev.kobj, mt_attribute_group); @@ -976,6 +987,10 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id) return 0; +hid_fail: + list_for_each_entry(hi, hdev-inputs, list) + if (hi-input-name != hdev-name) + kfree(hi-input-name); fail: kfree(td-fields); kfree(td); @@ -1020,8 +1035,15 @@ static int mt_resume(struct hid_device *hdev) static void mt_remove(struct hid_device *hdev) { struct mt_device *td = hid_get_drvdata(hdev); + struct hid_input *hi; + sysfs_remove_group(hdev-dev.kobj, mt_attribute_group); hid_hw_stop(hdev); + + list_for_each_entry(hi, hdev-inputs, list) + if (hi-input-name != hdev-name) + kfree(hi-input-name); + kfree(td); hid_set_drvdata(hdev, NULL); } -- 1.8.1.2 -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/7] HID: multitouch: breaks out touch handling in specific functions
This will allow easier integration of hybrid pen and touch devices. Signed-off-by: Benjamin Tissoires benjamin.tissoi...@redhat.com --- drivers/hid/hid-multitouch.c | 75 +++- 1 file changed, 54 insertions(+), 21 deletions(-) diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index 3af9efdd..6314473 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -365,7 +365,7 @@ static void mt_store_field(struct hid_usage *usage, struct mt_device *td, f-usages[f-length++] = usage-hid; } -static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi, +static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi, struct hid_field *field, struct hid_usage *usage, unsigned long **bit, int *max) { @@ -374,13 +374,8 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi, int code; struct hid_usage *prev_usage = NULL; - /* Only map fields from TouchScreen or TouchPad collections. - * We need to ignore fields that belong to other collections - * such as Mouse that might have the same GenericDesktop usages. */ if (field-application == HID_DG_TOUCHSCREEN) td-mt_flags |= INPUT_MT_DIRECT; - else if (field-application != HID_DG_TOUCHPAD) - return 0; /* * Model touchscreens providing buttons as touchpads. @@ -389,12 +384,6 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi, (usage-hid HID_USAGE_PAGE) == HID_UP_BUTTON) td-mt_flags |= INPUT_MT_POINTER; - /* eGalax devices provide a Digitizer.Stylus input which overrides -* the correct Digitizers.Finger X/Y ranges. -* Let's just ignore this input. */ - if (field-physical == HID_DG_STYLUS) - return -1; - if (usage-usage_index) prev_usage = field-usage[usage-usage_index - 1]; @@ -526,7 +515,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi, return 0; } -static int mt_input_mapped(struct hid_device *hdev, struct hid_input *hi, +static int mt_touch_input_mapped(struct hid_device *hdev, struct hid_input *hi, struct hid_field *field, struct hid_usage *usage, unsigned long **bit, int *max) { @@ -617,7 +606,7 @@ static void mt_sync_frame(struct mt_device *td, struct input_dev *input) td-num_received = 0; } -static int mt_event(struct hid_device *hid, struct hid_field *field, +static int mt_touch_event(struct hid_device *hid, struct hid_field *field, struct hid_usage *usage, __s32 value) { /* we will handle the hidinput part later, now remains hiddev */ @@ -697,19 +686,13 @@ static void mt_process_mt_event(struct hid_device *hid, struct hid_field *field, } } -static void mt_report(struct hid_device *hid, struct hid_report *report) +static void mt_touch_report(struct hid_device *hid, struct hid_report *report) { struct mt_device *td = hid_get_drvdata(hid); struct hid_field *field; unsigned count; int r, n; - if (report-id != td-mt_report_id) - return; - - if (!(hid-claimed HID_CLAIMED_INPUT)) - return; - /* * Includes multi-packet support where subsequent * packets are sent with zero contactcount. @@ -734,6 +717,56 @@ static void mt_report(struct hid_device *hid, struct hid_report *report) } } +static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi, + struct hid_field *field, struct hid_usage *usage, + unsigned long **bit, int *max) +{ + /* Only map fields from TouchScreen or TouchPad collections. + * We need to ignore fields that belong to other collections + * such as Mouse that might have the same GenericDesktop usages. */ + if (field-application != HID_DG_TOUCHSCREEN + field-application != HID_DG_TOUCHPAD) + return 0; + + /* eGalax devices provide a Digitizer.Stylus input which overrides +* the correct Digitizers.Finger X/Y ranges. +* Let's just ignore this input. */ + if (field-physical == HID_DG_STYLUS) + return -1; + + return mt_touch_input_mapping(hdev, hi, field, usage, bit, max); +} + +static int mt_input_mapped(struct hid_device *hdev, struct hid_input *hi, + struct hid_field *field, struct hid_usage *usage, + unsigned long **bit, int *max) +{ + return mt_touch_input_mapped(hdev, hi, field, usage, bit, max); +} + +static int mt_event(struct hid_device *hid, struct hid_field *field, + struct hid_usage *usage, __s32 value) +{ + struct mt_device *td = hid_get_drvdata(hid); + + if (field-report-id == td
Re: [patch] HID: hid-logitech-dj: add some range checks
Hi Dan, On Tue, Mar 5, 2013 at 2:06 PM, Dan Carpenter dan.carpen...@oracle.com wrote: We can't trust dj_report-device_index because it comes from the user and hasn't been range checked. It is used as an offset into the djrcv_dev-paired_dj_devices[] array which has 7 elements. There is one range check already in logi_dj_recv_add_djhid_device() and I have copy and pasted that here. DJ_DEVICE_INDEX_MIN is 1. DJ_DEVICE_INDEX_MAX is 6. Signed-off-by: Dan Carpenter dan.carpen...@oracle.com --- This is a static checker fix and I'm not certain it's correct, please look it over carefully. I do not know this code well, so I don't know why a zero index is invalid. diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c index 9500f2f..c01cd25 100644 --- a/drivers/hid/hid-logitech-dj.c +++ b/drivers/hid/hid-logitech-dj.c @@ -701,6 +701,13 @@ static int logi_dj_raw_event(struct hid_device *hdev, * anything else with it. */ + if ((dj_report-device_index DJ_DEVICE_INDEX_MIN) || + (dj_report-device_index DJ_DEVICE_INDEX_MAX)) { + dev_err(hdev-dev, %s: invalid device index:%d\n, __func__, + dj_report-device_index); + return true; + } + Sorry, but it's a NACK in its current form: - device_index == 0 is the receiver. IIRC, the receiver sends the different REPORT_TYPE_NOTIF_*. So I would say that this patch blocks events from the receiver. - the DJ protocol specifies that the device index is between 1 and 6 and that 0 means the receiver. So unless there is a problem in the USB line from the receiver to the driver, device index will be between 1 and 6 for input events. This is only true as long as with stay with real firmware from Logitech, not from events from uhid. If you want to add a check, it need to be in logi_dj_recv_forward_report(). The current access to djrcv_dev-paired_dj_devices[] in delayedwork_callback() has been removed in latest HID tree: https://git.kernel.org/cgit/linux/kernel/git/jikos/hid.git/log/?h=for-3.10/logitech Cheers, Benjamin spin_lock_irqsave(djrcv_dev-lock, flags); if (dj_report-report_id == REPORT_ID_DJ_SHORT) { switch (dj_report-report_type) { -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] HID: logitech-dj: do not directly call hid_output_raw_report() during probe
hid_output_raw_report() makes a direct call to usb_control_msg(). However, some USB3 boards have shown that the usb device is not ready during the .probe(). This blocks the entire usb device, and the paired mice, keyboards are not functional. The dmesg output is the following: [ 11.912287] logitech-djreceiver 0003:046D:C52B.0003: hiddev0,hidraw0: USB HID v1.11 Device [Logitech USB Receiver] on usb-:00:14.0-2/input2 [ 11.912537] logitech-djreceiver 0003:046D:C52B.0003: logi_dj_probe:logi_dj_recv_query_paired_devices error:-32 [ 11.912636] logitech-djreceiver: probe of 0003:046D:C52B.0003 failed with error -32 Relying on the scheduled call to .hid_hw_request() fixes the problem. related bugs: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1072082 https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1039143 https://bugzilla.redhat.com/show_bug.cgi?id=840391 https://bugzilla.kernel.org/show_bug.cgi?id=49781 Reported-and-tested-by: Bob Bowles bobjohnbow...@gmail.com Signed-off-by: Benjamin Tissoires benjamin.tissoi...@redhat.com --- Hi guys, this bug has taken too long to be solved. I managed to figure out the root cause recently thanks to the work of Jelle Foks in lp#1039143. Jiri, I based this fix on top of your for-3.10/logitech branch. It *will* failed to build when you will merge it with the branch for-3.10/hid-driver-transport-cleanups. This is due to the use of usbhid_submit_report() instead of hid_hw_request(). The However, I prefer do it that way so that I can send it to stable to fix all the current releases since 3.2 (I guess only the LTS v3.4 will pick it and the current 3.8, but distros can then cherry-pick it easily). Is that ok for you? Cheers, Benjamin drivers/hid/hid-logitech-dj.c | 22 ++ 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c index 199b78c..147b59c 100644 --- a/drivers/hid/hid-logitech-dj.c +++ b/drivers/hid/hid-logitech-dj.c @@ -419,19 +419,25 @@ static int logi_dj_recv_send_report(struct dj_receiver_dev *djrcv_dev, struct dj_report *dj_report) { struct hid_device *hdev = djrcv_dev-hdev; - int sent_bytes; + struct hid_report *report; + struct hid_report_enum *output_report_enum; + u8 *data = (u8 *)(dj_report-device_index); + int i; - if (!hdev-hid_output_raw_report) { - dev_err(hdev-dev, %s: - hid_output_raw_report is null\n, __func__); + output_report_enum = hdev-report_enum[HID_OUTPUT_REPORT]; + report = output_report_enum-report_id_hash[REPORT_ID_DJ_SHORT]; + + if (!report) { + dev_err(hdev-dev, %s: unable to find dj report\n, __func__); return -ENODEV; } - sent_bytes = hdev-hid_output_raw_report(hdev, (u8 *) dj_report, -sizeof(struct dj_report), -HID_OUTPUT_REPORT); + for (i = 0; i report-field[0]-report_count; i++) + report-field[0]-value[i] = data[i]; + + usbhid_submit_report(hdev, report, USB_DIR_OUT); - return (sent_bytes 0) ? sent_bytes : 0; + return 0; } static int logi_dj_recv_query_paired_devices(struct dj_receiver_dev *djrcv_dev) -- 1.8.1.4 -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: linux-next: Tree for Mar 6 (hid)
Hi Randy, On Wed, Mar 6, 2013 at 8:26 PM, Randy Dunlap rdun...@infradead.org wrote: On 03/05/13 16:50, Stephen Rothwell wrote: Hi all, Changes since 20130305: on i386 and x86_64: Sorry, it looks like my patches went not very smoothly. ERROR: usb_control_msg [drivers/hid/hid-multitouch.ko] undefined! I've submitted a new patch removing this direct call. Maybe we should change the Kconfig while these patches are not applied. Jiri? ERROR: usb_ifnum_to_if [drivers/hid/hid-holtek-kbd.ko] undefined! oops, I'll have to double check this one. when CONFIG_USB_SUPPORT is not enabled. Should these drivers depend on USB_HID? The goal of the patch series that breaks this was to make the hid drivers independent of their transport layers, thus most of them should not depend on usbhid anymore. Cheers, Benjamin -- ~Randy -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] HID: holtek: Holtek devices depends on USB_HID
In the HID drivers tranport cleanup series, I removed the dependency between hid-holtek and usbhid. This was wrong as hid-holtek.c relies extensively on usb calls. This fixes compilation error when CONFIG_USB_SUPPORT is not enabled. Signed-off-by: Benjamin Tissoires benjamin.tissoi...@redhat.com --- drivers/hid/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig index 3abb11f..613083a 100644 --- a/drivers/hid/Kconfig +++ b/drivers/hid/Kconfig @@ -215,7 +215,7 @@ config HID_EZKEY config HID_HOLTEK tristate Holtek HID devices - depends on HID + depends on USB_HID ---help--- Support for Holtek based devices: - Holtek On Line Grip based game controller -- 1.8.1.4 -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: linux-next: Tree for Mar 6 (hid)
On Thu, Mar 7, 2013 at 11:09 AM, Jiri Kosina jkos...@suse.cz wrote: On Thu, 7 Mar 2013, Benjamin Tissoires wrote: on i386 and x86_64: Sorry, it looks like my patches went not very smoothly. ERROR: usb_control_msg [drivers/hid/hid-multitouch.ko] undefined! I've submitted a new patch removing this direct call. Maybe we should change the Kconfig while these patches are not applied. Jiri? Benjamin, which patch is that? The only hid-multitouch I still have in my queue is the hybrid finger/pen devices support, and I don't think it's fixing this problem. You should have at least one other patch for hid-mt: https://patchwork.kernel.org/patch/2193641/ And I sent as a RFC the patch I was talking about: https://patchwork.kernel.org/patch/2193801/ https://patchwork.kernel.org/patch/2193821/ These two RFCs have been reviewed by David Herrmann, so maybe we can include them in their current form as no one else complained about it ( maybe you will :) ). Cheers, Benjamin Then I have one for logitech-dj which I still need to review, but I don't see anything else ... ? Thanks, -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: linux-next: Tree for Mar 6 (hid)
On Thu, Mar 7, 2013 at 11:33 AM, Jiri Kosina jkos...@suse.cz wrote: On Thu, 7 Mar 2013, Benjamin Tissoires wrote: You should have at least one other patch for hid-mt: https://patchwork.kernel.org/patch/2193641/ Ah, right, that one is in my queue as well, sorry for not mentioning it in my previous post. No problems :) That doesn't remove the direct call either though. true And I sent as a RFC the patch I was talking about: https://patchwork.kernel.org/patch/2193801/ https://patchwork.kernel.org/patch/2193821/ These two RFCs have been reviewed by David Herrmann, so maybe we can include them in their current form as no one else complained about it ( maybe you will :) ). For some reason these two vanished between the cracks of my mailbox. Thanks for pointing those out, I will process them today and either apply it, or fix it for -next by adding a config dependency. ok, thanks for dealing with it. Cheers, Benjamin -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 1/2] HID: Extend the interface with idle requests
On Thu, Mar 7, 2013 at 3:26 PM, Jiri Kosina jkos...@suse.cz wrote: On Wed, 27 Feb 2013, Benjamin Tissoires wrote: Some drivers send the idle command directly to underlying device, creating an unwanted dependency on the underlying transport layer. This patch adds hid_hw_idle() to the interface, thereby removing usbhid from the lion share of the drivers. Signed-off-by: Benjamin Tissoires benjamin.tissoi...@redhat.com --- drivers/hid/usbhid/hid-core.c | 15 +++ include/linux/hid.h | 19 +++ 2 files changed, 34 insertions(+) diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c index 420466b..effcd3d 100644 --- a/drivers/hid/usbhid/hid-core.c +++ b/drivers/hid/usbhid/hid-core.c @@ -1253,6 +1253,20 @@ static void usbhid_request(struct hid_device *hid, struct hid_report *rep, int r } } +static int usbhid_idle(struct hid_device *hid, int report, int idle, + int reqtype) Where does the need for passing the report argument come from please? Well, I haven't checked in the USB spec, but hid_set_idle() in usbhid/hid-core.c does require the reportID, so I add it to the request. I just gave a quick look at the HID/I2C spec, and it also requires the report ID. The set_idle request is report specific. Benjamin -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] HID: logitech-dj: do not directly call hid_output_raw_report() during probe
On Thu, Mar 7, 2013 at 4:07 PM, Jiri Kosina jkos...@suse.cz wrote: On Tue, 5 Mar 2013, Benjamin Tissoires wrote: hid_output_raw_report() makes a direct call to usb_control_msg(). However, some USB3 boards have shown that the usb device is not ready during the .probe(). This blocks the entire usb device, and the paired mice, keyboards are not functional. The dmesg output is the following: [ 11.912287] logitech-djreceiver 0003:046D:C52B.0003: hiddev0,hidraw0: USB HID v1.11 Device [Logitech USB Receiver] on usb-:00:14.0-2/input2 [ 11.912537] logitech-djreceiver 0003:046D:C52B.0003: logi_dj_probe:logi_dj_recv_query_paired_devices error:-32 [ 11.912636] logitech-djreceiver: probe of 0003:046D:C52B.0003 failed with error -32 Relying on the scheduled call to .hid_hw_request() fixes the problem. related bugs: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1072082 https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1039143 https://bugzilla.redhat.com/show_bug.cgi?id=840391 https://bugzilla.kernel.org/show_bug.cgi?id=49781 Reported-and-tested-by: Bob Bowles bobjohnbow...@gmail.com Signed-off-by: Benjamin Tissoires benjamin.tissoi...@redhat.com --- Hi guys, this bug has taken too long to be solved. I managed to figure out the root cause recently thanks to the work of Jelle Foks in lp#1039143. Jiri, I based this fix on top of your for-3.10/logitech branch. It *will* failed to build when you will merge it with the branch for-3.10/hid-driver-transport-cleanups. This is due to the use of usbhid_submit_report() instead of hid_hw_request(). The However, I prefer do it that way so that I can send it to stable to fix all the current releases since 3.2 (I guess only the LTS v3.4 will pick it and the current 3.8, but distros can then cherry-pick it easily). Benjamin, Bob, excellent work, thanks for figuring it out. Given the nature of the bug, I'd prefer to push it to Linus for 3.9 still, and have it backported to -stable as well. Once it's in Linus' tree, I will handle the fallout in for-3.10/hid-driver-transport-cleanups and fix the build. Ok? That seems perfect to me. Now queued for 3.9. Thanks. And thanks also for applying the other patches Jiri. Cheers, Benjamin -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] input: mt: Support for touch cancel
On Tue, Mar 12, 2013 at 2:36 AM, Peter Hutterer peter.hutte...@who-t.net wrote: On Mon, Mar 11, 2013 at 04:34:27PM -0400, Yufeng Shen wrote: I have ran into cases where I want to make a touch end event to have a touch cancel indication. This comes from trying to solve the problem of : If the touch sequence happens before the system suspends, and the touch release event is never received after the system resumes, userspace MT state tracking could be in a bad state. ( see #5 from https://bugs.launchpad.net/ubuntu/+source/xserver-xorg-input-synaptics/+bug/968845 for an example of how this could happen from lid close/open on MBA) ftr, this a bug in the driver and should be fixed now. One possible workaround is to let the touch device driver to release all existing touches on resume, which has the effect of clearing all the MT states in userspace touch stacks. But the touch release/end event often will result in some gesture being recognized and performed, like a tap-to-click being generated. So I am wondering what's the best way to solve the problem of clearing the touch states with minimal side effect. One way I can think of is to have MTB protocol add support of a touch cancel indication on touch release, e.g. making TRACKING_ID = -2 meaning that the touch release is synthesized from the system and really has the meaning of releasing and canceling the current touch, while TRACKING_ID = -1 meaning that the touch release is reported back from the device. And from Xf86-input driver level, we can add a corresponding TouchCancel for this. I can handle touch-cancel events in the synaptics driver to avoid tap-to-click but further details get a bit nasty. To actually add TouchCancel to the client-protocol means a new XI protocol revision, plus the stuff in the server _and_ the stuff in the client. that is quite some lag time here, and if a client cannot handle TouchCancel all we can do is do a TouchEnd - which will still trigger the gesture. even if you update the touch clients you're still lacking any solution for pointer-emulated clients. again, here we can only do a ButtonRelease event which again will trigger whatever it did. All the above can be implemented though. In fact, I suspect the protocol part is the easy bit (just a flag on TouchEnd) but the server part is reasonably nasty. the real counter-argument is that I think it is a partial solution only. From an X perspective touches also end when you vt-switch away from the server (device is disabled). but the kernel won't cancel the touch event for that. Or when the device is disabled by the client (disable touchpad while typing feature), So we'd have to maintain both implicit cancel and explicit cancel in the driver anyway. so yeah, I don't think adding this to the kernel would provide any significant benefit since we still need to handle all the other cases anyway. If the same effect is seen when VT-switching, it's definitively a user space synchronization problem, not a kernel problem. I think we have all the pieces in term of protocol in the kernel for this use case: When coming back from resume, the kernel should guarantee that the current input state is correct. If fingers are still present, then their slots are still assigned, if they are missing, their slots should be silently released (as if the released occurs while sleeping). This is something the kernel can work on. As for the user-space, when coming back from a situation where inconsistency may have occurred (VT-switching, sleep/resume, events dropped due to a SYN_DROPPED event, or device disabled by the client), the user-space driver has to retrieve the current state of the kernel driver through the correct ioctls. If it doesn't do it, then that means that he is not following the evdev protocol. It's up to it to notify or not the toolkit/gesture recognizer that events have been dropped. Cheers, Benjamin -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] input: mt: Support for touch cancel
On Tue, Mar 12, 2013 at 7:18 PM, Yufeng Shen mile...@google.com wrote: On Tue, Mar 12, 2013 at 4:59 AM, Benjamin Tissoires benjamin.tissoi...@gmail.com wrote: On Tue, Mar 12, 2013 at 2:36 AM, Peter Hutterer peter.hutte...@who-t.net wrote: On Mon, Mar 11, 2013 at 04:34:27PM -0400, Yufeng Shen wrote: I have ran into cases where I want to make a touch end event to have a touch cancel indication. This comes from trying to solve the problem of : If the touch sequence happens before the system suspends, and the touch release event is never received after the system resumes, userspace MT state tracking could be in a bad state. ( see #5 from https://bugs.launchpad.net/ubuntu/+source/xserver-xorg-input-synaptics/+bug/968845 for an example of how this could happen from lid close/open on MBA) ftr, this a bug in the driver and should be fixed now. One possible workaround is to let the touch device driver to release all existing touches on resume, which has the effect of clearing all the MT states in userspace touch stacks. But the touch release/end event often will result in some gesture being recognized and performed, like a tap-to-click being generated. So I am wondering what's the best way to solve the problem of clearing the touch states with minimal side effect. One way I can think of is to have MTB protocol add support of a touch cancel indication on touch release, e.g. making TRACKING_ID = -2 meaning that the touch release is synthesized from the system and really has the meaning of releasing and canceling the current touch, while TRACKING_ID = -1 meaning that the touch release is reported back from the device. And from Xf86-input driver level, we can add a corresponding TouchCancel for this. I can handle touch-cancel events in the synaptics driver to avoid tap-to-click but further details get a bit nasty. To actually add TouchCancel to the client-protocol means a new XI protocol revision, plus the stuff in the server _and_ the stuff in the client. that is quite some lag time here, and if a client cannot handle TouchCancel all we can do is do a TouchEnd - which will still trigger the gesture. even if you update the touch clients you're still lacking any solution for pointer-emulated clients. again, here we can only do a ButtonRelease event which again will trigger whatever it did. All the above can be implemented though. In fact, I suspect the protocol part is the easy bit (just a flag on TouchEnd) but the server part is reasonably nasty. the real counter-argument is that I think it is a partial solution only. From an X perspective touches also end when you vt-switch away from the server (device is disabled). but the kernel won't cancel the touch event for that. Or when the device is disabled by the client (disable touchpad while typing feature), So we'd have to maintain both implicit cancel and explicit cancel in the driver anyway. so yeah, I don't think adding this to the kernel would provide any significant benefit since we still need to handle all the other cases anyway. If the same effect is seen when VT-switching, it's definitively a user space synchronization problem, not a kernel problem. I think we have all the pieces in term of protocol in the kernel for this use case: When coming back from resume, the kernel should guarantee that the current input state is correct. If fingers are still present, then their slots are still assigned, if they are missing, their slots should be silently released (as if the released occurs while sleeping). This is something the kernel can work on. As for the user-space, when coming back from a situation where inconsistency may have occurred (VT-switching, sleep/resume, events dropped due to a SYN_DROPPED event, or device disabled by the client), the user-space driver has to retrieve the current state of the kernel driver through the correct ioctls. If it doesn't do it, then that means that he is not following the evdev protocol. It's up to it to notify or not the toolkit/gesture recognizer that events have been dropped. what's your suggestion on implementing notify the toolkit/gesture recognizer that events have been dropped ? For xf86-input-synaptics, then this is just an internal call to stop the scrolling, tapping and others. For external gesture recognition engines and toolkits, then it's the exact same problem you were raising at the beginning of the thread: I doubt Xinput 2.2 can handle it smoothly. But in any case, this is something that will need further thoughts. Maybe there could be a generic XI event: XIAbortedEvents. Peter would answer better than me on this subject. Benjamin -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] HID: input: don't register unmapped input devices
Hi Henrik, first, thanks for the review of the series. On 03/19/2013 10:25 PM, Henrik Rydberg wrote: Hi Benjamin, There is no need to register an input device containing no events. This allows drivers using the quirk MULTI_INPUT to register one input per report effectively used. For backward compatibility, we need to add a quirk to request this behavior. Signed-off-by: Benjamin Tissoires benjamin.tissoi...@redhat.com --- drivers/hid/hid-input.c | 77 + include/linux/hid.h | 1 + 2 files changed, 78 insertions(+) diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c index 21b196c..7aaf7d3 100644 --- a/drivers/hid/hid-input.c +++ b/drivers/hid/hid-input.c @@ -1198,6 +1198,67 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid) return hidinput; } +static bool hidinput_has_been_populated(struct hid_input *hidinput) +{ +int i; +bool r = 0; + +for (i = 0; i BITS_TO_LONGS(EV_CNT); i++) +r = r || hidinput-input-evbit[i]; I believe there is a bit count method that will do this for you (weight). thanks for that. Will change it in v2. + +for (i = 0; i BITS_TO_LONGS(KEY_CNT); i++) +r = r || hidinput-input-keybit[i]; + +for (i = 0; i BITS_TO_LONGS(REL_CNT); i++) +r = r || hidinput-input-relbit[i]; + +for (i = 0; i BITS_TO_LONGS(ABS_CNT); i++) +r = r || hidinput-input-absbit[i]; + +for (i = 0; i BITS_TO_LONGS(MSC_CNT); i++) +r = r || hidinput-input-mscbit[i]; + +for (i = 0; i BITS_TO_LONGS(LED_CNT); i++) +r = r || hidinput-input-ledbit[i]; + +for (i = 0; i BITS_TO_LONGS(SND_CNT); i++) +r = r || hidinput-input-sndbit[i]; + +for (i = 0; i BITS_TO_LONGS(FF_CNT); i++) +r = r || hidinput-input-ffbit[i]; + +for (i = 0; i BITS_TO_LONGS(SW_CNT); i++) +r = r || hidinput-input-swbit[i]; + +return !!r; +} + +static void hidinput_cleanup_hidinput(struct hid_device *hid, +struct hid_input *hidinput) +{ +struct hid_report *report; +int i, k; + +list_del(hidinput-list); +input_free_device(hidinput-input); + +for (k = HID_INPUT_REPORT; k = HID_OUTPUT_REPORT; k++) { +if (k == HID_OUTPUT_REPORT +hid-quirks HID_QUIRK_SKIP_OUTPUT_REPORTS) +continue; + +list_for_each_entry(report, hid-report_enum[k].report_list, +list) { + +for (i = 0; i report-maxfield; i++) +if (report-field[i]-hidinput == hidinput) +report-field[i]-hidinput = NULL; Why test before clearing? Well, the idea is to remove blank hidinput from the hid device, keeping the populated ones. Thus, the argument struct hid_input *. In many cases, blanks hidinput and populated ones are set on the same hid device: Let's take a win7 device. hid-mt will populate the multitouch report, discarding the mouse emulation report. Thus, we need to only remove the hidinput created from the mouse emulation report, keeping the multitouch one. Does that makes sense? +} +} + +kfree(hidinput); +} + /* * Register the input device; print a message. * Configure the input layer interface @@ -1249,6 +1310,10 @@ int hidinput_connect(struct hid_device *hid, unsigned int force) hidinput_configure_usage(hidinput, report-field[i], report-field[i]-usage + j); +if ((hid-quirks HID_QUIRK_NO_EMPTY_INPUT) +!hidinput_has_been_populated(hidinput)) +continue; + Is there possibly a subset of input properties that may be populated but still not duplicated? Or the other way around? Not sure I understand your point here. The hid spec says that reports are independent. Thus, you can have several devices presenting different semantic (absolute, relative, touch, pen, 3d, gyros, etc...) within the same hid interface. If you activate both the quirks NO_EMPTY_INPUT and MULTI_INPUT, you have this behavior correctly handled as per the spec IMO. The thing is that it was not the default before. For instance, hid-magicmouse for magic trackpads use the hid-input core functionality to create its input device, but it does not populate it: input_mapping() returns -1. The declaration of the axes is done later, once the hid_hw_start() has returned. Besides the fact that there may be a race with udev checking for the device and assigning it to the touchpad class, the input is not populated here (no matter the MULTI_INPUT quirk in place or not). Thus, the need to specifically introduce this quirk. So I would say that if the driver is using NO_EMPTY_INPUT
Re: [PATCH 6/7] HID: multitouch: append Pen to the name of the stylus input
On 03/19/2013 10:38 PM, Henrik Rydberg wrote: Hi Benjamin, This is not just cosmetics, it can help to write udev and X.org rules. Signed-off-by: Benjamin Tissoires benjamin.tissoi...@redhat.com --- drivers/hid/hid-multitouch.c | 24 +++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index d89f0eb..faeec95 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -44,6 +44,7 @@ #include linux/slab.h #include linux/usb.h #include linux/input/mt.h +#include linux/string.h MODULE_AUTHOR(Stephane Chatty cha...@enac.fr); @@ -371,6 +372,15 @@ static int mt_pen_input_mapping(struct hid_device *hdev, struct hid_input *hi, unsigned long **bit, int *max) { struct mt_device *td = hid_get_drvdata(hdev); +char *name; + +if (hi-input-name == hdev-name) { +name = kzalloc(sizeof(hdev-name) + 5, GFP_KERNEL); +if (name) { +sprintf(name, %s Pen, hdev-name); +hi-input-name = name; +} +} Why not simply duplicate the the string? This kind of magic sharing is not really helping. Also, for multi input devices, the assumptions in hidinput_allocate() seem to be the culprit. Perhaps the fix should be there instead. Yes, hidinput_allocate() clearly steals the reference here. However, fixing this in hid-core would not be simpler: some drivers (ntrig does at least) assume that there is no need to free input-name, which means that: - either we need to fix each individual driver - either we introduce some kind of mechanism in order not to free const char * declarations like it happens in ntrig. In v2, I'll just realloc for each input the input-name, and then if the pen is here, then it will realloc the new name. Cheers, Benjamin td-pen_report_id = field-report-id; @@ -914,6 +924,7 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id) int ret, i; struct mt_device *td; struct mt_class *mtclass = mt_classes; /* MT_CLS_DEFAULT */ +struct hid_input *hi; for (i = 0; mt_classes[i].name ; i++) { if (id-driver_data == mt_classes[i].name) { @@ -964,7 +975,7 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id) ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT); if (ret) -goto fail; +goto hid_fail; ret = sysfs_create_group(hdev-dev.kobj, mt_attribute_group); @@ -976,6 +987,10 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id) return 0; +hid_fail: +list_for_each_entry(hi, hdev-inputs, list) +if (hi-input-name != hdev-name) +kfree(hi-input-name); fail: kfree(td-fields); kfree(td); @@ -1020,8 +1035,15 @@ static int mt_resume(struct hid_device *hdev) static void mt_remove(struct hid_device *hdev) { struct mt_device *td = hid_get_drvdata(hdev); +struct hid_input *hi; + sysfs_remove_group(hdev-dev.kobj, mt_attribute_group); hid_hw_stop(hdev); + +list_for_each_entry(hi, hdev-inputs, list) +if (hi-input-name != hdev-name) +kfree(hi-input-name); + kfree(td); hid_set_drvdata(hdev, NULL); } -- 1.8.1.2 Thanks, Henrik -- To unsubscribe from this list: send the line unsubscribe linux-input in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html