login
Search
This adds the requested documentation for the driver and some code to identify the current running firmware to aid debug and support. Rafi --
This adds firmware version polling to the end of probe and reports the
version both in the raw form and proccessed to match the formatting used
by ntrig in windows.
Signed-off-by: Rafi Rubin <rafi@seas.upenn.edu>
---
drivers/hid/hid-ntrig.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 60 insertions(+), 0 deletions(-)
diff --git a/drivers/hid/hid-ntrig.c b/drivers/hid/hid-ntrig.c
index 43e95de..ab0ca7f 100644
--- a/drivers/hid/hid-ntrig.c
+++ b/drivers/hid/hid-ntrig.c
@@ -63,6 +63,9 @@ struct ntrig_data {
bool reading_mt;
+ /* The raw firmware version code */
+ __u8 firmware_version[4];
+
__u8 mt_footer[4];
__u8 mt_foot_count;
@@ -90,6 +93,26 @@ struct ntrig_data {
};
+/*
+ * This function converts the 4 byte raw firmware code into
+ * a string containing 5 comma separated numbers.
+ */
+static int ntrig_version_string(unsigned char *raw, char *buf)
+{
+ __u8 a = (raw[1] & 0b00001110) >> 1;
+ __u8 b = (raw[0] & 0b00111100) >> 2;
+ __u8 c = ((raw[0] & 0b00000011) << 3) | ((raw[3] & 0b11100000) >> 5);
+ __u8 d = ((raw[3] & 0b00000111) << 3) | ((raw[2] & 0b11100000) >> 5);
+ __u8 e = raw[2] & 0b00000111;
+
+ /*
+ * As yet unmapped bits:
+ * 0b11000000 0b11110001 0b00011000 0b00011000
+ */
+
+ return sprintf(buf, "%u.%u.%u.%u.%u", a, b, c, d, e);
+}
+
static ssize_t show_phys_width(struct device *dev,
struct device_attribute *attr,
char *buf)
@@ -776,6 +799,9 @@ static int ntrig_probe(struct hid_device *hdev, const struct hid_device_id *id)
struct hid_input *hidinput;
struct input_dev *input;
struct hid_report *report;
+ unsigned char *data;
+ struct usb_device *usb_dev = hid_to_usb_dev(hdev);
+ char *buf;
if (id->driver_data)
hdev->quirks |= HID_QUIRK_MULTI_INPUT;
@@ -848,10 +874,44 @@ static int ntrig_probe(struct hid_device *hdev, const struct hid_device_id *id)
if (report)
usbhid_submit_report(hdev, report, USB_DIR_OUT);
+ data = kmalloc(8, ...The version field of the input_id struct is a 16-bit number that can be used to code device-specific version information, and is retrievable via EVIOCGID. Perhaps one could code the firmware version in there. Henrik --
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Thanks, I missed that field and will update my approach accordingly. I suppose in that case, I should also keep the firmware decoding as a userspace tool. Micki, would you care to comment on the decoding? Any misplaced bits, or additional bits you would like to add mappings for? Rafi -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.10 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAkx6u0EACgkQwuRiAT9o60+8SwCgrjqd+FNJPSEve/tdM7+C0i4/ 0xsAmwVg0YDjz3QFukfPyawrZwdoPD3U =htK1 -----END PGP SIGNATURE----- --
The doctumentation includes a brief introduction to the driver and explanations of the filtering parameters as well as a discussion of the need for and working of the filters. Signed-off-by: Rafi Rubin <rafi@seas.upenn.edu> --- Documentation/input/ntrig.txt | 126 +++++++++++++++++++++++++++++++++++++++++ 1 files changed, 126 insertions(+), 0 deletions(-) create mode 100644 Documentation/input/ntrig.txt diff --git a/Documentation/input/ntrig.txt b/Documentation/input/ntrig.txt new file mode 100644 index 0000000..be1fd98 --- /dev/null +++ b/Documentation/input/ntrig.txt @@ -0,0 +1,126 @@ +N-Trig touchscreen Driver +------------------------- + Copyright (c) 2008-2010 Rafi Rubin <rafi@seas.upenn.edu> + Copyright (c) 2009-2010 Stephane Chatty + +This driver provides support for N-Trig pen and multi-touch sensors. Single +and multi-touch events are translated to the appropriate protocols for +the hid and input systems. Pen events are sufficiently hid compliant and +are left to the hid core. The driver also provides additional filtering +and utility functions accessible with sysfs and module parameters. + +This driver has been reported to work properly with multiple N-Trig devices +attached. + + +Parameters +---------- + +Note: values set at load time are global and will apply to all applicable +devices. Adjusting parameters with sysfs will override the load time values, +but only for that one device. + +The following parameters are used to configure filters to reduce noise: + +activate_slack number of fingers to ignore before processing events + +activation_height size threshold to activate immediately +activation_width + +min_height size threshold bellow which fingers are ignored +min_width both to decide activation and during activity + +deactivate_slack the number of "no contact" frames to ignore before + propagating the end of activity events + +When the last finger is removed from the device, it sends a number of empty +frames. By holding off ...
On 08/26/2010 06:54 AM, Rafi Rubin wrote: Without contact tracking, it is hard to imagine activation filtering to work properly. I would advocate to remove this functionality from the driver, and add it in userspace instead. Henrik --
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 I don't think its quite time to remove these filters. There still isn't a proper replacement that's readily accessible to most users. From what I've heard many still use the wacom driver to support touch in X. Tracking only helps if you increase the activation slack to more than 1 contact (the current default), and only if you assume the you will see ghosts span multiple frames in two different locations, which may be even less likely than seeing a ghost in one spot for two frames. Maybe in a few more months or another year, it will make more sense to remove the filters from this driver. In the mean time, is it really preferable to leave them undocumented? Rafi -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.10 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAkx6umIACgkQwuRiAT9o608sTQCg38F+0v0PSA+jqKSy84yDlVRW df8AoNWxO6zpnpY1Wvgu8xUrnH2uvFaB =uEW9 -----END PGP SIGNATURE----- --
Agreed. I have now applied the patch, thanks Rafi. -- Jiri Kosina SUSE Labs, Novell Inc. --
Signed-off-by: Rafi Rubin <rafi@seas.upenn.edu>
---
drivers/hid/hid-ntrig.c | 18 +++++++++---------
1 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/hid/hid-ntrig.c b/drivers/hid/hid-ntrig.c
index fb69b8c..43e95de 100644
--- a/drivers/hid/hid-ntrig.c
+++ b/drivers/hid/hid-ntrig.c
@@ -377,8 +377,8 @@ static struct attribute_group ntrig_attribute_group = {
*/
static int ntrig_input_mapping(struct hid_device *hdev, struct hid_input *hi,
- struct hid_field *field, struct hid_usage *usage,
- unsigned long **bit, int *max)
+ struct hid_field *field, struct hid_usage *usage,
+ unsigned long **bit, int *max)
{
struct ntrig_data *nd = hid_get_drvdata(hdev);
@@ -448,13 +448,13 @@ static int ntrig_input_mapping(struct hid_device *hdev, struct hid_input *hi,
/* width/height mapped on TouchMajor/TouchMinor/Orientation */
case HID_DG_WIDTH:
hid_map_usage(hi, usage, bit, max,
- EV_ABS, ABS_MT_TOUCH_MAJOR);
+ EV_ABS, ABS_MT_TOUCH_MAJOR);
return 1;
case HID_DG_HEIGHT:
hid_map_usage(hi, usage, bit, max,
- EV_ABS, ABS_MT_TOUCH_MINOR);
+ EV_ABS, ABS_MT_TOUCH_MINOR);
input_set_abs_params(hi->input, ABS_MT_ORIENTATION,
- 0, 1, 0, 0);
+ 0, 1, 0, 0);
return 1;
}
return 0;
@@ -468,8 +468,8 @@ static int ntrig_input_mapping(struct hid_device *hdev, struct hid_input *hi,
}
static int ntrig_input_mapped(struct hid_device *hdev, struct hid_input *hi,
- struct hid_field *field, struct hid_usage *usage,
- unsigned long **bit, int *max)
+ struct hid_field *field, struct hid_usage *usage,
+ unsigned long **bit, int *max)
{
/* No special mappings needed for the pen and single touch */
if (field->physical)
@@ -489,7 +489,7 @@ static int ntrig_input_mapped(struct hid_device *hdev, struct hid_input *hi,
* and call input_mt_sync after each point if necessary
*/
static int ntrig_event (struct hid_device *hid, struct hid_field *field,
- ...Signed-off-by: Rafi Rubin <rafi@seas.upenn.edu>
---
drivers/hid/hid-ntrig.c | 21 +++++++++++++++++++++
1 files changed, 21 insertions(+), 0 deletions(-)
diff --git a/drivers/hid/hid-ntrig.c b/drivers/hid/hid-ntrig.c
index ab0ca7f..e341e88 100644
--- a/drivers/hid/hid-ntrig.c
+++ b/drivers/hid/hid-ntrig.c
@@ -375,6 +375,26 @@ static ssize_t set_deactivate_slack(struct device *dev,
static DEVICE_ATTR(deactivate_slack, S_IWUSR | S_IRUGO, show_deactivate_slack,
set_deactivate_slack);
+static ssize_t show_firmware(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct hid_device *hdev = container_of(dev, struct hid_device, dev);
+ struct ntrig_data *nd = hid_get_drvdata(hdev);
+
+ if (!(nd->firmware_version[0] || nd->firmware_version[1] ||
+ nd->firmware_version[2] || nd->firmware_version[3]))
+ return sprintf(buf, "Firmware version unavailable");
+
+ ntrig_version_string(nd->firmware_version, buf);
+
+ return sprintf(buf, "%s (%02x%02x %02x%02x)\n", buf,
+ nd->firmware_version[0], nd->firmware_version[1],
+ nd->firmware_version[2], nd->firmware_version[3]);
+}
+
+static DEVICE_ATTR(firmware, S_IRUGO, show_firmware, NULL);
+
static struct attribute *sysfs_attrs[] = {
&dev_attr_sensor_physical_width.attr,
&dev_attr_sensor_physical_height.attr,
@@ -386,6 +406,7 @@ static struct attribute *sysfs_attrs[] = {
&dev_attr_activation_width.attr,
&dev_attr_activation_height.attr,
&dev_attr_deactivate_slack.attr,
+ &dev_attr_firmware.attr,
NULL
};
--
1.7.1
--
If this sysfs node should really be added (see EVIO), it is probably better if it returns the same format for all devices. If all numbers are zero, that is Henrik --
Yes, I think we should stick it into input_id and be done with it. Note that input_id is not only available via EVIOCGID ioctl but also already exported in sysfs. -- Dmitry --
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 The version in input_id is only 16 bits, whereas the ntrig version codes seem to be 32 bits. Actually I've only mapped 21 bits out of 64, but I figure the first and last 8 are not actually part of the version, but that's still more than 16. So, would you prefer that I increase the size of that field, or keep the firmware version code separate? Also does it make sense to have a provide a pretty printer in the kernel, or should that be left to userspace? The hardware returns a raw version code in the form: 1a08 a521 In the ntrig utilities and documentation the where firmware version is mentioned it looks more like this: 4.6.17.13.5 My intent was to make that second form more accessible to keep things simple for users, who if they are checking that probably already have enough troubling them :) Rafi -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.10 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAkx8Y5wACgkQwuRiAT9o608LGwCfYlJHAqxPXXt+wmEE42PWNsSG d4kAnA6wdbMh8cj557ytMSYcVHFIowRp =F3J9 -----END PGP SIGNATURE----- --
Hmm, changing size would require ABI change which I am hesitant to do without _very_ good reason. If debugging aid is the only purpose maybe we should just dump the data Yeah, splitting into segments is pretty cheap. -- Dmitry --
This adds firmware version polling to the end of probe and reports the
version both in the raw form and proccessed to match the formatting used
by ntrig in windows.
Signed-off-by: Rafi Rubin <rafi@seas.upenn.edu>
---
drivers/hid/hid-ntrig.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 52 insertions(+), 0 deletions(-)
diff --git a/drivers/hid/hid-ntrig.c b/drivers/hid/hid-ntrig.c
index 43e95de..c91ad4a 100644
--- a/drivers/hid/hid-ntrig.c
+++ b/drivers/hid/hid-ntrig.c
@@ -90,6 +90,26 @@ struct ntrig_data {
};
+/*
+ * This function converts the 4 byte raw firmware code into
+ * a string containing 5 comma separated numbers.
+ */
+static int ntrig_version_string(unsigned char *raw, char *buf)
+{
+ __u8 a = (raw[1] & 0b00001110) >> 1;
+ __u8 b = (raw[0] & 0b00111100) >> 2;
+ __u8 c = ((raw[0] & 0b00000011) << 3) | ((raw[3] & 0b11100000) >> 5);
+ __u8 d = ((raw[3] & 0b00000111) << 3) | ((raw[2] & 0b11100000) >> 5);
+ __u8 e = raw[2] & 0b00000111;
+
+ /*
+ * As yet unmapped bits:
+ * 0b11000000 0b11110001 0b00011000 0b00011000
+ */
+
+ return sprintf(buf, "%u.%u.%u.%u.%u", a, b, c, d, e);
+}
+
static ssize_t show_phys_width(struct device *dev,
struct device_attribute *attr,
char *buf)
@@ -776,6 +796,9 @@ static int ntrig_probe(struct hid_device *hdev, const struct hid_device_id *id)
struct hid_input *hidinput;
struct input_dev *input;
struct hid_report *report;
+ unsigned char *data;
+ struct usb_device *usb_dev = hid_to_usb_dev(hdev);
+ char *buf;
if (id->driver_data)
hdev->quirks |= HID_QUIRK_MULTI_INPUT;
@@ -848,10 +871,39 @@ static int ntrig_probe(struct hid_device *hdev, const struct hid_device_id *id)
if (report)
usbhid_submit_report(hdev, report, USB_DIR_OUT);
+ data = kmalloc(8, GFP_KERNEL);
+ if (!data) {
+ ret = -ENOMEM;
+ goto err_free;
+ }
+
+ ret = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0),
+ USB_REQ_CLEAR_FEATURE,
+ ...Just to be clear, this is basically the same as the previous patch 3/4. I just pulled the storage of the value. Even if later someone wants a sysfs node, we can just poll the device on read. Thanks Henrik for pointing out that it would be nice to dump the version during init. Rafi --
Freeing buf? Since the firmware name is very much a property of the device, why Henrik --
This won't compile with gcc 3.4 which we still support. Maybe time to kill the support? -- js --
Why not? If its the binary notation, I was just trying to show the mapping more visually, hex is fine with me if preferred. If its anything aside from that, --
Beacuse it's a gnu extension added in gcc 4.3. (I though it's in gcc 4.0 initially, but it's not. So you cannot use it in the kernel code at all.) regards, -- js --
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Sorry, would you please clarify "it". Which bit of syntax in that code is unsupported by the older versions of gcc? Rafi -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.10 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAkx/5j8ACgkQwuRiAT9o60/uiACfTJkcLG/0m/4WQpBi82ugunc3 WY4AoIpJglKNJRm4ST1VP5nE+AiMAMJ0 =k6/0 -----END PGP SIGNATURE----- --
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 I have verified its the binary notation that gcc-3.4 does not support. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.10 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAkx/6LcACgkQwuRiAT9o608A9gCdHKerwZIDMBVgsmBkLU1WEhVU oFsAoKz18MHUfIKSjkWErU4ZDktMCjZX =1T0c -----END PGP SIGNATURE----- --
This adds firmware version polling to the end of probe and reports the
version both in the raw form and proccessed to match the formatting used
by N-Trig.
Signed-off-by: Rafi Rubin <rafi@seas.upenn.edu>
---
drivers/hid/hid-ntrig.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 56 insertions(+), 0 deletions(-)
diff --git a/drivers/hid/hid-ntrig.c b/drivers/hid/hid-ntrig.c
index 43e95de..3472b3b 100644
--- a/drivers/hid/hid-ntrig.c
+++ b/drivers/hid/hid-ntrig.c
@@ -90,6 +90,26 @@ struct ntrig_data {
};
+/*
+ * This function converts the 4 byte raw firmware code into
+ * a string containing 5 comma separated numbers.
+ */
+static int ntrig_version_string(unsigned char *raw, char *buf)
+{
+ __u8 a = (raw[1] & 0x0e) >> 1;
+ __u8 b = (raw[0] & 0x3c) >> 2;
+ __u8 c = ((raw[0] & 0x03) << 3) | ((raw[3] & 0xe0) >> 5);
+ __u8 d = ((raw[3] & 0x07) << 3) | ((raw[2] & 0xe0) >> 5);
+ __u8 e = raw[2] & 0x07;
+
+ /*
+ * As yet unmapped bits:
+ * 0b11000000 0b11110001 0b00011000 0b00011000
+ */
+
+ return sprintf(buf, "%u.%u.%u.%u.%u", a, b, c, d, e);
+}
+
static ssize_t show_phys_width(struct device *dev,
struct device_attribute *attr,
char *buf)
@@ -776,6 +796,9 @@ static int ntrig_probe(struct hid_device *hdev, const struct hid_device_id *id)
struct hid_input *hidinput;
struct input_dev *input;
struct hid_report *report;
+ unsigned char *data;
+ struct usb_device *usb_dev = hid_to_usb_dev(hdev);
+ char *buf;
if (id->driver_data)
hdev->quirks |= HID_QUIRK_MULTI_INPUT;
@@ -848,10 +871,43 @@ static int ntrig_probe(struct hid_device *hdev, const struct hid_device_id *id)
if (report)
usbhid_submit_report(hdev, report, USB_DIR_OUT);
+ data = kmalloc(8, GFP_KERNEL);
+ if (!data) {
+ ret = -ENOMEM;
+ goto err_free;
+ }
+
+ ret = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0),
+ USB_REQ_CLEAR_FEATURE,
+ USB_TYPE_CLASS | USB_RECIP_INTERFACE |
+ ...Hi Rafi, Why do you allocate this from heap? Surely we can spare 20 bytes on stack (you aren't doing DMA into it). I'd also split all this code into ntrig_report_version() to simplifu -- Dmitry --
Jiri, I moved the code to a separate function as Dmitry suggested, and compiled a kernel from a clean tree using
gcc-3.4 (I think).
If this version fails for you, would you mind being a bit more verbose about the failure.
Rafi
---
drivers/hid/hid-ntrig.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 51 insertions(+), 0 deletions(-)
diff --git a/drivers/hid/hid-ntrig.c b/drivers/hid/hid-ntrig.c
index 43e95de..69169ef 100644
--- a/drivers/hid/hid-ntrig.c
+++ b/drivers/hid/hid-ntrig.c
@@ -90,6 +90,55 @@ struct ntrig_data {
};
+/*
+ * This function converts the 4 byte raw firmware code into
+ * a string containing 5 comma separated numbers.
+ */
+static int ntrig_version_string(unsigned char *raw, char *buf)
+{
+ __u8 a = (raw[1] & 0x0e) >> 1;
+ __u8 b = (raw[0] & 0x3c) >> 2;
+ __u8 c = ((raw[0] & 0x03) << 3) | ((raw[3] & 0xe0) >> 5);
+ __u8 d = ((raw[3] & 0x07) << 3) | ((raw[2] & 0xe0) >> 5);
+ __u8 e = raw[2] & 0x07;
+
+ /*
+ * As yet unmapped bits:
+ * 0b11000000 0b11110001 0b00011000 0b00011000
+ */
+
+ return sprintf(buf, "%u.%u.%u.%u.%u", a, b, c, d, e);
+}
+
+static void ntrig_report_version(struct hid_device *hdev)
+{
+ int ret;
+ char buf[20];
+ struct usb_device *usb_dev = hid_to_usb_dev(hdev);
+ unsigned char *data = kmalloc(8, GFP_KERNEL);
+
+ if (!data)
+ goto err_free;
+
+ ret = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0),
+ USB_REQ_CLEAR_FEATURE,
+ USB_TYPE_CLASS | USB_RECIP_INTERFACE |
+ USB_DIR_IN,
+ 0x30c, 1, data, 8,
+ USB_CTRL_SET_TIMEOUT);
+
+ if (ret == 8) {
+ ret = ntrig_version_string(&data[2], buf);
+
+ dev_info(&hdev->dev,
+ "Firmware version: %s (%02x%02x %02x%02x)\n",
+ buf, data[2], data[3], data[4], data[5]);
+ }
+
+err_free:
+ kfree(data);
+}
+
static ssize_t show_phys_width(struct device *dev,
struct device_attribute *attr,
char *buf)
@@ -848,6 +897,8 @@ static int ntrig_probe(struct ...Looks good to me, thanks for making changes Rafi. -- Dmitry --
This version is OK. In the previous one, there was char *buf; ... kfree(buff); thanks, -- js --
Dmitry, Jiri, thanks a lot for taking care reviewing the patch while I have been lagging behind. Now applied, thanks Rafi. -- Jiri Kosina SUSE Labs, Novell Inc. --
Thanks Jiri, and thanks for all the feedback. --
