hid-ntrig documentation and firmware id

hid-ntrig documentation and firmware id

[Rafi Rubin]

This adds the requested documentation for the driver and some code to
identify the current running firmware to aid debug and support.

Rafi

[PATCH 1/4] Adding documention

[Rafi Rubin]

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 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

Re: [PATCH 1/4] Adding documention

[Henrik Rydberg]

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

Re: [PATCH 1/4] Adding documention

[Rafi Rubin]

-----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-----

Re: [PATCH 1/4] Adding documention

[Jiri Kosina]

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.

[PATCH 2/4] a bit of whitespace cleanup

[Rafi Rubin]

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,
-		                        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

[PATCH 3/4] identify firmware version

[Rafi Rubin]

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, 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

Re: [PATCH 3/4] identify firmware version

[Henrik Rydberg]

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 <rafi@seas.upenn.edu>


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

Re: [PATCH 3/4] identify firmware version

[Rafi Rubin]

-----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 <rafi@seas.upenn.edu>
> 
> 
> 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-----

[PATCH 4/4] firmware sysfs node

[Rafi Rubin]

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

Re: [PATCH 4/4] firmware sysfs node

[Henrik Rydberg]

On 08/26/2010 06:54 AM, Rafi Rubin wrote:

> 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");


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

Re: [PATCH 4/4] firmware sysfs node

[Dmitry Torokhov]

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 <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");
> 
> 
> 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

Re: [PATCH 4/4] firmware sysfs node

[Rafi Rubin]

-----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 <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");
>>
>>
>> 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-----

Re: [PATCH 4/4] firmware sysfs node

[Dmitry Torokhov]

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 <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");
> >>
> >>
> >> 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

[PATCH] identify firmware version

[Rafi Rubin]

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,
+			      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]);
+	}
+
 	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

Re: [PATCH] identify firmware version

[Rafi Rubin]

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

On 09/01/10 05:48, 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<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,
> +			      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]);
> +	}
> +
>   	ret = sysfs_create_group(&hdev->dev.kobj,
>   			&ntrig_attribute_group);
>
>   	return 0;
> +err_free_data:
> +	kfree(data);
>   err_free:
>   	kfree(nd);
>   	return ret;

Re: [PATCH] identify firmware version

[Henrik Rydberg]

On 09/01/2010 11:48 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 <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)


const on the input variable would help seeing which argument is which.

> +{
> +	__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,
> +			      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]);


Freeing buf? Since the firmware name is very much a property of the device, why
not add it to the ntrig_data struct.

> +	}
> +
>  	ret = sysfs_create_group(&hdev->dev.kobj,
>  			&ntrig_attribute_group);
>  
>  	return 0;
> +err_free_data:
> +	kfree(data);
>  err_free:
>  	kfree(nd);
>  	return ret;


Henrik

Re: [PATCH] identify firmware version

[Jiri Slaby]

On 09/01/2010 11:48 AM, Rafi Rubin wrote:
> --- 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;

This won't compile with gcc 3.4 which we still support. Maybe time to
kill the support?

...
> @@ -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,
> +			      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]);
> +	}
> +
>  	ret = sysfs_create_group(&hdev->dev.kobj,
>  			&ntrig_attribute_group);
>  
>  	return 0;

Two leaks here.

> +err_free_data:
> +	kfree(data);
>  err_free:
>  	kfree(nd);
>  	return ret;


-- 
js

Re: [PATCH] identify firmware version

[Rafi Rubin]

On 09/01/2010 04:12 PM, Jiri Slaby wrote:
> On 09/01/2010 11:48 AM, Rafi Rubin wrote:
>> --- 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;
>
> This won't compile with gcc 3.4 which we still support. Maybe time to
> kill the support?
>
> ...

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, 
then I'd be afraid, there's nothing fancy here.

>> @@ -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,
>> +			      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]);
>> +	}
>> +
>>   	ret = sysfs_create_group(&hdev->dev.kobj,
>>   			&ntrig_attribute_group);
>>
>>   	return 0;
>
> Two leaks here.

buf and data, noted and will fix.

>> +err_free_data:
>> +	kfree(data);
>>   err_free:
>>   	kfree(nd);
>>   	return ret;
>
>

Re: [PATCH] identify firmware version

[Jiri Slaby]

On 09/02/2010 02:12 AM, Rafi Rubin wrote:
> 
> 
> On 09/01/2010 04:12 PM, Jiri Slaby wrote:
>> On 09/01/2010 11:48 AM, Rafi Rubin wrote:
>>> --- 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;
>>
>> This won't compile with gcc 3.4 which we still support. Maybe time to
>> kill the support?
>>
>> ...
> 
> Why not?

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

Re: [PATCH] identify firmware version

[Rafi Rubin]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/02/10 04:03, Jiri Slaby wrote:
> On 09/02/2010 02:12 AM, Rafi Rubin wrote:
>>
>>
>> On 09/01/2010 04:12 PM, Jiri Slaby wrote:
>>> On 09/01/2010 11:48 AM, Rafi Rubin wrote:
>>>> --- 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;
>>>
>>> This won't compile with gcc 3.4 which we still support. Maybe time to
>>> kill the support?
>>>
>>> ...
>>
>> Why not?
> 
> 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.)

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-----

Re: [PATCH] identify firmware version

[Rafi Rubin]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/02/10 14:00, Rafi Rubin wrote:
> On 09/02/10 04:03, Jiri Slaby wrote:
>> On 09/02/2010 02:12 AM, Rafi Rubin wrote:
>>>
>>>
>>> On 09/01/2010 04:12 PM, Jiri Slaby wrote:
>>>> On 09/01/2010 11:48 AM, Rafi Rubin wrote:
>>>>> --- 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;
>>>>
>>>> This won't compile with gcc 3.4 which we still support. Maybe time to
>>>> kill the support?
>>>>
>>>> ...
>>>
>>> Why not?
> 
>> 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.)
> 
> Sorry, would you please clarify "it".  Which bit of syntax in that code is
> unsupported by the older versions of gcc?

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-----

[PATCH] identify firmware version

[Rafi Rubin]

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 |
+			      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]);
+
+		kfree(buff);
+	}
+
 	ret = sysfs_create_group(&hdev->dev.kobj,
 			&ntrig_attribute_group);
 
+	kfree(data);
+
 	return 0;
+err_free_data:
+	kfree(data);
 err_free:
 	kfree(nd);
 	return ret;
-- 
1.7.1

Re: [PATCH] identify firmware version

[Dmitry Torokhov]

Hi Rafi,

On Mon, Sep 06, 2010 at 12:42:42PM -0400, 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 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 |
> +			      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;
> +		}

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
error handling here.

> +
> +		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]);
> +
> +		kfree(buff);
> +	}
> +
>  	ret = sysfs_create_group(&hdev->dev.kobj,
>  			&ntrig_attribute_group);
>  
> +	kfree(data);
> +
>  	return 0;
> +err_free_data:
> +	kfree(data);
>  err_free:
>  	kfree(nd);
>  	return ret;
> -- 
> 1.7.1
> 

-- 
Dmitry

Re: [PATCH] identify firmware version

[Jiri Slaby]

On 09/06/2010 09:48 PM, Dmitry Torokhov wrote:
>> @@ -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 |
>> +			      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;
>> +		}
> 
> Why do you allocate this from heap? Surely we can spare 20 bytes on
> stack (you aren't doing DMA into it).

Hi, yeah, I think so too.

> I'd also split all this code into ntrig_report_version() to simplifu
> error handling here.
> 
>> +
>> +		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]);
>> +
>> +		kfree(buff);

In any case, this doesn't compile...

>> +	}

regards,
-- 
js

Re: [PATCH] identify firmware version

[Rafi Rubin]

On Mon, Sep 06, 2010 at 11:22:50PM +0200, Jiri Slaby wrote:
> On 09/06/2010 09:48 PM, Dmitry Torokhov wrote:
> >> @@ -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 |
> >> +			      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;
> >> +		}
> > 
> > Why do you allocate this from heap? Surely we can spare 20 bytes on
> > stack (you aren't doing DMA into it).
> 
> Hi, yeah, I think so too.

No arguments from me, that was just sloppy.

> > I'd also split all this code into ntrig_report_version() to simplifu
> > error handling here.
> > 
> >> +
> >> +		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]);
> >> +
> >> +		kfree(buff);
> 
> In any case, this doesn't compile...
> 
> >> +	}

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 hid_device *hdev, const struct hid_device_id *id)
 	if (report)
 		usbhid_submit_report(hdev, report, USB_DIR_OUT);
 
+	ntrig_report_version(hdev);
+
 	ret = sysfs_create_group(&hdev->dev.kobj,
 			&ntrig_attribute_group);
 
-- 
1.7.1

Re: [PATCH] identify firmware version

[Dmitry Torokhov]

On Mon, Sep 06, 2010 at 07:32:03PM -0400, Rafi Rubin wrote:
> On Mon, Sep 06, 2010 at 11:22:50PM +0200, Jiri Slaby wrote:
> > On 09/06/2010 09:48 PM, Dmitry Torokhov wrote:
> > >> @@ -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 |
> > >> +			      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;
> > >> +		}
> > > 
> > > Why do you allocate this from heap? Surely we can spare 20 bytes on
> > > stack (you aren't doing DMA into it).
> > 
> > Hi, yeah, I think so too.
> 
> No arguments from me, that was just sloppy.
> 
> > > I'd also split all this code into ntrig_report_version() to simplifu
> > > error handling here.
> > > 
> > >> +
> > >> +		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]);
> > >> +
> > >> +		kfree(buff);
> > 
> > In any case, this doesn't compile...
> > 
> > >> +	}
> 
> 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.
> 

Looks good to me, thanks for making changes Rafi.

-- 
Dmitry

Re: [PATCH] identify firmware version

[Jiri Slaby]

On 09/07/2010 01:32 AM, Rafi Rubin wrote:
> On Mon, Sep 06, 2010 at 11:22:50PM +0200, Jiri Slaby wrote:
>>>> +
>>>> +		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]);
>>>> +
>>>> +		kfree(buff);
>>
>> In any case, this doesn't compile...
>>
>>>> +	}
> 
> 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).

This version is OK. In the previous one, there was
char *buf;
...
kfree(buff);

thanks,
-- 
js

Re: [PATCH] identify firmware version

[Jiri Kosina]

On Tue, 7 Sep 2010, Jiri Slaby wrote:

> >>>> +
> >>>> +		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]);
> >>>> +
> >>>> +		kfree(buff);
> >>
> >> In any case, this doesn't compile...
> >>
> >>>> +	}
> > 
> > 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).
> 
> This version is OK. In the previous one, there was
> char *buf;
> ...
> kfree(buff);

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.

Re: [PATCH] identify firmware version

[Rafi Rubin]

On 09/08/10 05:47, Jiri Kosina wrote:
> On Tue, 7 Sep 2010, Jiri Slaby wrote:
> 
>>>>>> +
>>>>>> +		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]);
>>>>>> +
>>>>>> +		kfree(buff);
>>>>
>>>> In any case, this doesn't compile...
>>>>
>>>>>> +	}
>>>
>>> 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).
>>
>> This version is OK. In the previous one, there was
>> char *buf;
>> ...
>> kfree(buff);
> 
> Dmitry, Jiri, thanks a lot for taking care reviewing the patch while I 
> have been lagging behind.
> 
> Now applied, thanks Rafi.

Thanks Jiri, and thanks for all the feedback.