Re: [PATCH 1/7] HID: N-trig MTM Driver fix and cleanup patch 1

From: Rafi Rubin
Date: Mon Mar 08 2010 - 19:51:18 EST

On 03/08/2010 04:13 PM, mickib1@xxxxxxxxx wrote:
From: micki<micki@micki-laptop.(none)>

Add Change Log and defines of MTM firmware.
Add debug Paramater change Driver name in hid_driver structure

N-trig is changing the way people interact with computers by providing a dual-mode pen and true multi-touch input device, specifically designed for today's advanced computing world.
N-trig DuoSense solution provides a real Hands-on computing experience, and sets the stage for OEMs and ISVs to introduce innovative computer products and applications for an intuitive, Hands-on experience directly onscreen.
DuoSense digitizers are easily integrated into existing technologies, support all LCDs, keep devices slim and light, and can be implemented in a broad range of products, ranging from small notebooks to large LCDs.
N-trig has offices in Israel, the US, Taiwan and Japan.

Is that just in your .sig?

Signed-off-by: Micki Balanga<micki@xxxxxxxxxx>
---
drivers/hid/hid-ntrig.c | 94 ++++++++++++++++++++++++++++++++++++++++++++--
1 files changed, 89 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/hid-ntrig.c b/drivers/hid/hid-ntrig.c
index 3234c72..e99342d 100644
--- a/drivers/hid/hid-ntrig.c
+++ b/drivers/hid/hid-ntrig.c
@@ -3,7 +3,18 @@
*
* Copyright (c) 2008 Rafi Rubin
* Copyright (c) 2009 Stephane Chatty
+ * Copyright (c) 2010 N-TRIG
*
+ * 1.0 - Rafi Rubin ( Version From 2010-02-13 02:13:05)
+ * This cleans up the identification of multitouch
+ * groups and enables the end of group sync.
+ * Taps are now explicitly handled to adjust for the changes in the
+ * event stream in multitouch mode.
+ * Added triple and quad tap for the benefit of
+ * tools that recognize different tap types but
+ * do not have full multi touch support.
+ * 1.1 - N-trig - Add Change Log and defines of MTM firmware.
+ * Add debug Paramater change Driver name in hid_driver structure
*/

I don't know that the change log needs to be in the code file itself. The point was more that your commit messages should give us an idea about what your changing. Jiri, what's your take on that?

+/*
+ * Pen Event Field Declaration
+ */
+#define EVENT_PEN_TIP 0x03
+#define EVENT_PEN_RIGHT 0x05
+#define EVENT_TOUCH_PEN 0x07
+#define EVENT_PEN_IN_RANGE 0x01

You still haven't actually established why its necessary to fuss with pen events if we split the pen off to a separate input device.

+/*
+ * MTM 4 last bytes of report descriptor
+ */
+#define REPORT_GENERIC1 0x01
+#define REPORT_MT 0x02
+#define REPORT_PALM 0x03
+#define REPORT_GENERIC2 0x04

When its not too inconvenient, it would be better to put defines in the patches where you actually use them. I know these are actually useful, but if we stop here they aren't actually used.


+#define NTRIG_USB_DEVICE_ID 0x0001
Already defined in hid-ids.h



+
+/*
+ * MTM fields
+ */
+#define PEN_REPORT_SIZE 0x48

The report seems to specifically identify itself. Why not use that clear identification instead of something less clear that may change in the future (even if there are no plans at the moment). If nothing else this relieves a little pressure on your hardware/firmware designers.

From the rdesc of my screen in the debug fs:
INPUT(1)[INPUT]
Field(0)
Physical(Digitizers.Stylus)

Paraphrasing ntrig_probe from 2.6.33:

list_for_each_entry(hidinput, &hdev->inputs, list)
switch (hidinput->report->field[0]->application)
case HID_DG_PEN:
input->name = "N-Trig Pen";
break;
case HID_DG_TOUCHSCREEN:

Note that in this one case I do use this to identify the report, and thus need to pick a field which you could argue is a potential hazard, but you haven't made that point. So please tell us what's wrong with this before you toss it out for something different.

+#define MAX_FINGERS_SUPPORT 0x06

Again, is it really a good idea to keep this quantity fixed? Especially in light of the press release that suggests your company is considering devices that support more fingers.

+#define END_OF_REPORT 0x64

I should comment where you put the code that uses this, but you put them in separate patches.

I don't see why you tie up a group of contacts by emitting TRACKING_ID 0x64. The group should already by wrapped up with an input_sync, which you have. Therefore this extra event doesn't actually provide any additional information. Furthermore, sending a bogus tracking id may upset other user space tools.

+/*
+ * Dummy Finger Declaration
+ */
+#define X_CORD_VAL 0x00
+#define Y_CORD_VAL 0x00
+#define DX_CORD_VAL 0xFA
+#define DY_CORD_VAL 0x96
+#define GENERIC_BYTE_VAL 0x0D

The naming of these constants seems a little messy. Perhaps something more like "DUMMY_X, DUMMY_DX"....

+#define MTM_FRAME_INDEX 0xff000001
+#define MTM_PROPROETARY 0xff000002

Thank you, we should have named these constants earlier.

+#define MODULE_NAME "hid_ntrig"
- .name = "ntrig",

I left off the "hid_" prefix based on what I saw in the other hid drivers. Jiri, is there a standard to follow or do you have an opinion about this?

Rafi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/