Le 14 oct. 10 à 11:38, Henrik Rydberg a écrit :
+ bool valid; /* did we just get valid contact data for this slot? */
+ bool prev_valid;/* was this slot previously valid/active? */
I do think these should be named "touch" or "prox" rather than
"valid". The
latter term is used in many drivers no represent "not null", and
mixing the
semantics with proximity is best avoided.
Actually, the mix of semantics exists because of the mismatch between
the (static) HID protocol and the (dynamic) data it transports. Most
of the time, TipSwitch=0 occurs when we get placeholder data: there
is nothing to report at all, but the message format imposes that
something is sent. And sometimes (rarely), we get TipSwitch=0 because
a finger has been released. We are lucky that we can handle the two
situations in the same way.
See below for a discussion of how we can solve this.
+ bool curvalid; /* is the current contact valid? */
+ __u16 curcontactid; /* ContactID of the current contact */
+ __u16 curx, cury, curp; /* other attributes of the current
contact */
Could be a struct mt_slot instead.
Well, it's not *exactly* the same data. Especially if, as I'm
beginning to understand thanks to one of your later comments, there
is a difference in the semantics of 'valid' between slots and the
current contact as read in the HID message:
- my above discussion applies to the current contact
- and in slots, this information gets filtered to become actual
proximity information.
This would give:
- curvalid in mt_data
- touch in mt_slot (and not prox, because prox will mean something
else in devices that have hover detection)
+};
+
+struct mt_class {
+ int (*compute_slot)(struct mt_device *);
+ __u8 maxcontacts;
+ __s8 inputmode; /* InputMode HID feature number, -1 if non-
existent */
It would be nice to have additional information here, like signal-
to-noise ratio.
I thought you would say that. My answer was ready: I left it to you
to add it afterwards :-)
+/* contact data that only some devices report */
+#define PRESSURE (1 << 0)
+#define SIZE (1 << 1)
The names here are a bit too general, IMO.
I thought so too, but was too lazy to imagine something else and
decided that what the heck, these are private names. Any suggestion?
+
+/*
+ * these device-dependent functions determine what slot corresponds
+ * to a valid contact that was just read.
+ */
+
+static int slot_from_contactid(struct mt_device *td)
+{
+ return td->curcontactid;
+}
I suppose this one is meant to loop over available slots for some
implementations.
See the following patches for examples. So far, looping was not
necessary.
+
+struct mt_class mt_classes[] = {
+ /* DUAL1 */ { slot_from_contactid, 2, -1 },
+};
Off placement of comment.
Please, please don't force me to add a useless field to the struct so
as to make sure that the type is read before the data :-) Oh well...
A helper function that simplifies the repeated usage of
input_set_abs_params()
would be nice.
A local variable for hi->input would be nice.
If we want to support fuzz, and do not want to add it to the hid
structure
(because it is not available in the reports), we should consider
setting up the
input_dev completely within this driver, as done in the 3M and
egalax drivers.
Yes, my idea was that you'd like to take over and introduce the kind
of work you are doing for 3M and eGalax. As for the helper function
and hi->input, hey, I just copied your code :-)
+ if (slotnum >= 0 && slotnum <= td->mtclass->maxcontacts - 1) {
The returned slotnum should always be valid, so this test is
redundant.
Nope. See Cypress devices.
+ slot = td->slots + slotnum;
+
+ slot->valid = true;
Rather that setting valid here, it should simply be the proximity
(touch) state.
All slot data in our buffer is actually valid at all times.
+ slot->x = td->curx;
+ slot->y = td->cury;
+ slot->p = td->curp;
Using a temporary slot struct for curx etc would be nice.
+ }
+ }
+ td->curcontact++;
See my previous discussion on curvalid vs valid/prox/touch.
I think it is unnecessary to have both a slot number and a
curcontact value.
Either the firmware reports the slot itself, or it reports a
contact id that
needs to be looked up from the list of slots. In neither case does
anything but
slot number and contact id enter the equation.
See the following patches for examples of this being wrong.
+ if (s->prev_valid) {
+ input_mt_slot(input, i);
The logic is a bit easier if input_mt_slot() appears once before
all the
branching. If nothing else is emitted, the input core does not emit
the slot event.
I'm not a big fan of this kind of 'blind pass'. Jiri?
How about touch_major, touch_minor and orientation here?
I have no device available to test this at the moment. What I had in
mind was to add them when we add support for a device that has them.
+ s->prev_valid = true;
+ s->valid = false;
Changing the touch state to false here seems wrong.
But it's necessary, believe me. There are devices for which we don't
get a chance to change it to false when the finger is released.
Setting it to false every time and resetting it to true when we get
confirmation that the contact is still alive is the failsafe way of
doing things.
+ input_event(input, EV_KEY, BTN_TOUCH, 1);
+ input_event(input, EV_ABS, ABS_X, oldest->x);
+ input_event(input, EV_ABS, ABS_Y, oldest->y);
Pressure here as well.
Oops.
+ } else {
+ input_event(input, EV_KEY, BTN_TOUCH, 0);
+ }
+
+ input_sync(input);
+ td->curcontact = 0;
The curcontact semantics could be clearer, as noted elsewhere.
Another name, perhaps? The bottom line is that we need to keep trace
of the rank of the contact in the current message.
+}
+
+
+
+static int mt_event(struct hid_device *hid, struct hid_field *field,
+ struct hid_usage *usage, __s32 value)
+{
+ struct mt_device *td = hid_get_drvdata(hid);
+
+ if (hid->claimed & HID_CLAIMED_INPUT) {
+ struct input_dev *input = field->hidinput->input;
+ switch (usage->hid) {
+ case HID_DG_INRANGE:
This one is used to report the validity of the current reported
contact on many
devices. Maybe all, even?
Actually, only on a minority of device last time I checked.
+ case HID_DG_TIPSWITCH:
+ td->curvalid = value;
This is really the touch state, the proximity.
I wish.
+ break;
+ case HID_DG_CONFIDENCE:
+ break;
+ case HID_DG_CONTACTID:
+ td->curcontactid = value;
Here is where I would but the conversion to slot id. And it should
definitely be
guarded so that it is always a valid index.
I'll pay you a beer if you find how to make this work for all
devices :-)
+ case HID_GD_Y:
+ td->cury = value;
+ /* works for devices where Y is last in a contact */
+ mt_complete_slot(td);
Relying on a particular field to come last seems non-generic. Since
there is
already a bit field started for various features of the device, why
not complete
that list with all data reported per contact, and use it know when
a contact is
complete? Something like "recorded_mask == expected_mask".
What I had in mind was that the information is available in lower
level code, and that we could handle this when switching to a raw hid
device. Otherwise, we'd just be reinventing the wheel, that is
rebuilding information that's already there. Same for
HID_CONTACTCOUNT below: I use it because it marks the end of messages
on most devices, but there should be a better way of knowing that we
are at the end of a message.
+ break;
+ case HID_DG_CONTACTCOUNT:
+ /*
+ * works for devices where contact count is
+ * the last field in a message
+ */
+ if (value)
+ td->maxcontact = value - 1;
The usage of maxcontact is a bit odd. Perhaps there should be a
"maxslot" which
never changes, and a "contactcount" which starts at zero.
maxcontact works with curcontact. I found the name 'contactcount'
confusing, because in HID it means something else. Still looking for
a better name for 'curcontact'.
+ if (td->curcontact > td->maxcontact)
+ mt_emit_event(td, input);
So curcontact is here a counter, which implements the logic found
in the 3M,
with partial reports combining into a single one. Maybe we need a
counter, or
maybe we can get by using the CONTACTID field.
Yes, curcontact is a counter. And we also need it for computing the
slot id sometimes.
+
+#if 0
+ /*
+ * todo: activate this as soon as the patch where the
quirk below
+ * is defined is commited. This will allow the driver to
correctly
+ * support devices that emit events over several HID
messages.
+ */
+ hdev->quirks |= HID_QUIRK_NO_INPUT_SYNC;
+#endif
This code should be active.
Then it won't compile in 2.6.36-rc7.
+
+ td = kzalloc(sizeof(struct mt_device), GFP_KERNEL);
+ if (!td) {
+ dev_err(&hdev->dev, "cannot allocate multitouch data\n");
+ return -ENOMEM;
+ }
+ td->mtclass = mt_classes + id->driver_data;
+ td->slots = kzalloc(td->mtclass->maxcontacts * sizeof(struct
mt_slot),
+ GFP_KERNEL);
Allocating slots in the same range as mt_device would be nice.
Agreed.
+ struct hid_report_enum *re = hdev->report_enum
+ + HID_FEATURE_REPORT;
&hdev->report_enum[HID_FEATURE_REPORT]
Won't fit on one line, then. Visually, it gets worse. Someone else,
an opinion?
That's it for now :-)
Then, on to patches 2, 3 and 4 :-)
St.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
