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 <[email protected]>
---
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, 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 |
+ USB_DIR_IN,
+ 0x30c, 1, data, 8,
+ USB_CTRL_SET_TIMEOUT);
+
+ if (ret == 8) {
+ buf = kmalloc(20, GFP_KERNEL);
+ if (!buf) {
+ ret = -ENOMEM;
+ goto err_free_data;
+ }
+
+ 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]);
+
+ nd->firmware_version[0] = data[2];
+ nd->firmware_version[1] = data[3];
+ nd->firmware_version[2] = data[4];
+ nd->firmware_version[3] = data[5];
+ }
+
ret = sysfs_create_group(&hdev->dev.kobj,
&ntrig_attribute_group);
return 0;
+err_free_data:
+ kfree(data);
err_free:
kfree(nd);
return ret;
--
1.7.1
Signed-off-by: Rafi Rubin <[email protected]>
---
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
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 <[email protected]>
---
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 <[email protected]>
+ 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 deactivation for a few frames we can tolerate false
+erroneous disconnects, where the sensor may mistakenly not detect a finger that
+is still present. Thus deactivate_slack addresses problems where a users might
+see breaks in lines during drawing, or drop an object during a long drag.
+
+
+Additional sysfs items
+----------------------
+
+These nodes just provide easy access to the ranges reported by the device.
+sensor_logical_height the range for positions reported during activity
+sensor_logical_width
+
+sensor_physical_height internal ranges not used for normal events but
+sensor_physical_width useful for tuning
+
+All N-Trig devices with product id of 1 report events in the ranges of
+X: 0-9600
+Y: 0-7200
+However not all of these devices have the same physical dimensions. Most
+seem to be 12" sensors (Dell Latitude XT and XT2 and the HP TX2), and
+at least one model (Dell Studio 17) has a 17" sensor. The ratio of physical
+to logical sizes is used to adjust the size based filter parameters.
+
+
+Filtering
+---------
+
+With the release of the early multi-touch firmwares it became increasingly
+obvious that these sensors were prone to erroneous events. Users reported
+seeing both inappropriately dropped contact and ghosts, contacts reported
+where no finger was actually touching the screen.
+
+Deactivation slack helps prevent dropped contact for single touch use, but does
+not address the problem of dropping one of more contacts while other contacts
+are still active. Drops in the multi-touch context require additional
+processing and should be handled in tandem with tacking.
+
+As observed ghost contacts are similar to actual use of the sensor, but they
+seem to have different profiles. Ghost activity typically shows up as small
+short lived touches. As such, I assume that the longer the continuous stream
+of events the more likely those events are from a real contact, and that the
+larger the size of each contact the more likely it is real. Balancing the
+goals of preventing ghosts and accepting real events quickly (to minimize
+user observable latency), the filter accumulates confidence for incoming
+events until it hits thresholds and begins propagating. In the interest in
+minimizing stored state as well as the cost of operations to make a decision,
+I've kept that decision simple.
+
+Time is measured in terms of the number of fingers reported, not frames since
+the probability of multiple simultaneous ghosts is expected to drop off
+dramatically with increasing numbers. Rather than accumulate weight as a
+function of size, I just use it as a binary threshold. A sufficiently large
+contact immediately overrides the waiting period and leads to activation.
+
+Setting the activation size thresholds to large values will result in deciding
+primarily on activation slack. If you see longer lived ghosts, turning up the
+activation slack while reducing the size thresholds may suffice to eliminate
+the ghosts while keeping the screen quite responsive to firm taps.
+
+Contacts continue to be filtered with min_height and min_width even after
+the initial activation filter is satisfied. The intent is to provide
+a mechanism for filtering out ghosts in the form of an extra finger while
+you actually are using the screen. In practice this sort of ghost has
+been far less problematic or relatively rare and I've left the defaults
+set to 0 for both parameters, effectively turning off that filter.
+
+I don't know what the optimal values are for these filters. If the defaults
+don't work for you, please play with the parameters. If you do find other
+values more comfortable, I would appreciate feedback.
+
+The calibration of these devices does drift over time. If ghosts or contact
+dropping worsen and interfere with the normal usage of your device, try
+recalibrating it.
+
+
+Calibration
+-----------
+
+The N-Trig windows tools provide calibration and testing routines. Also an
+unofficial unsupported set of user space tools including a calibrator is
+available at:
+http://code.launchpad.net/~rafi-seas/+junk/ntrig_calib
+
+
+Tracking
+--------
+
+As of yet, all tested N-Trig firmwares do not track fingers. When multiple
+contacts are active they seem to be sorted primarily by Y position.
--
1.7.1
Signed-off-by: Rafi Rubin <[email protected]>
---
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,
- struct hid_usage *usage, __s32 value)
+ struct hid_usage *usage, __s32 value)
{
struct input_dev *input = field->hidinput->input;
struct ntrig_data *nd = hid_get_drvdata(hid);
@@ -860,7 +860,7 @@ err_free:
static void ntrig_remove(struct hid_device *hdev)
{
sysfs_remove_group(&hdev->dev.kobj,
- &ntrig_attribute_group);
+ &ntrig_attribute_group);
hid_hw_stop(hdev);
kfree(hid_get_drvdata(hdev));
}
--
1.7.1
On 08/26/2010 06:54 AM, Rafi Rubin wrote:
> 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 <[email protected]>
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
On 08/26/2010 06:54 AM, Rafi Rubin wrote:
[...]
> +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 deactivation for a few frames we can tolerate false
> +erroneous disconnects, where the sensor may mistakenly not detect a finger that
> +is still present. Thus deactivate_slack addresses problems where a users might
> +see breaks in lines during drawing, or drop an object during a long drag.
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
On 08/26/2010 06:54 AM, Rafi Rubin wrote:
> Signed-off-by: Rafi Rubin <[email protected]>
> ---
> 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");
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
understandable also by someone reading the node.
> +
> + 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
> };
>
Henrik
On Fri, Aug 27, 2010 at 02:09:05PM +0200, Henrik Rydberg wrote:
> On 08/26/2010 06:54 AM, Rafi Rubin wrote:
>
> > Signed-off-by: Rafi Rubin <[email protected]>
> > ---
> > 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");
>
>
> 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
> understandable also by someone reading the node.
>
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
On 08/27/10 08:06, Henrik Rydberg wrote:
> On 08/26/2010 06:54 AM, Rafi Rubin wrote:
>
> [...]
>
>> +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 deactivation for a few frames we can tolerate false
>> +erroneous disconnects, where the sensor may mistakenly not detect a finger that
>> +is still present. Thus deactivate_slack addresses problems where a users might
>> +see breaks in lines during drawing, or drop an object during a long drag.
>
>
> 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
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-----
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 08/27/10 08:01, Henrik Rydberg wrote:
> On 08/26/2010 06:54 AM, Rafi Rubin wrote:
>
>> 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 <[email protected]>
>
>
> 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
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-----
On Sun, 29 Aug 2010, Rafi Rubin wrote:
> >> +----------
> >> +
> >> +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 deactivation for a few frames we can tolerate false
> >> +erroneous disconnects, where the sensor may mistakenly not detect a finger that
> >> +is still present. Thus deactivate_slack addresses problems where a users might
> >> +see breaks in lines during drawing, or drop an object during a long drag.
> >
> >
> > 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
>
> 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?
Agreed. I have now applied the patch, thanks Rafi.
--
Jiri Kosina
SUSE Labs, Novell Inc.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 08/27/10 12:34, Dmitry Torokhov wrote:
> On Fri, Aug 27, 2010 at 02:09:05PM +0200, Henrik Rydberg wrote:
>> On 08/26/2010 06:54 AM, Rafi Rubin wrote:
>>
>>> Signed-off-by: Rafi Rubin <[email protected]>
>>> ---
>>> 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");
>>
>>
>> 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
>> understandable also by someone reading the node.
>>
>
> 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.
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-----
On Mon, Aug 30, 2010 at 10:06:23PM -0400, Rafi Rubin wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 08/27/10 12:34, Dmitry Torokhov wrote:
> > On Fri, Aug 27, 2010 at 02:09:05PM +0200, Henrik Rydberg wrote:
> >> On 08/26/2010 06:54 AM, Rafi Rubin wrote:
> >>
> >>> Signed-off-by: Rafi Rubin <[email protected]>
> >>> ---
> >>> 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");
> >>
> >>
> >> 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
> >> understandable also by someone reading the node.
> >>
> >
> > 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.
>
> 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?
>
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
into dmesg and be done with it.
>
> 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 :)
>
Yeah, splitting into segments is pretty cheap.
--
Dmitry