Hi Richard,
thanks for this new version. Just some more comments on top of Henrik's ones.
On Tue, Mar 8, 2011 at 09:04, Henrik Rydberg <rydberg@xxxxxxxxxxx> wrote:
> Hi Richard,
>
>> Hi Benjamin, Hendrick, Jiri and Stéphan,
>
> The name is Henrik.
>
>> I would like to give this patch another shot, after Benjamin reviewed it (thanks for that!).
>> As he pointed out, testing for the capacitive kind of egalax devices is needed. Maybe Stéphan,
>> has the hardware and would be so kind to test it? I have tried it with my Samsung MB30 display
>> and it worked:
>> Bus 002 Device 002: ID 0eef:480e D-WAV Scientific Co., Ltd
>> (thats USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH3)
>>
>> I am looking forward to your comments,
>> Richard
>>
>>
>> PS: The patch should apply to 2fa403e8b49 of Jiris for-next branch, although I had to cheat
>> a little and rebase his tree to 2.6.38-rc7 because of some unrelated breakage (ecryptfs).
>
> Thanks for making this patch. Please find some comments inline. Also,
> since you are removing copyright from something that was largely
> copied into this driver, perhaps something should be done about that.
>
>> [PATCH] [input-hid] Add hid-egalax driver to the unified hid-multitouch framework.
>>
>> This patch merges the hid-egalax driver into hid-multitouch and
>> therefore adds two device classes (MT_CLS_EGALAX_CAPACITIVE/RESISTIVE).
>> It also gains the capability to work around broken hid reports by
>> overriding X/Y limits and emitting events for each finger
>> (MT_QUIRK_SEND_AT_EACH_REPORT). As a side effect, this patch
>> fixes the broken suspend/resume behavior with the old driver.
>>
>> Signed-off-by: Richard Nauber <Richard.Nauber@xxxxxxxxx>
>> ---
>> drivers/hid/Kconfig | 9 +-
>> drivers/hid/Makefile | 1 -
>> drivers/hid/hid-egalax.c | 279 ------------------------------------------
>> drivers/hid/hid-multitouch.c | 68 ++++++++++-
>> 4 files changed, 68 insertions(+), 289 deletions(-)
>> delete mode 100644 drivers/hid/hid-egalax.c
>>
> [...]
>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
>> index 65e7f20..a00f3d8 100644
>> --- a/drivers/hid/hid-multitouch.c
>> +++ b/drivers/hid/hid-multitouch.c
>> @@ -37,6 +37,7 @@ MODULE_LICENSE("GPL");
>> #define MT_QUIRK_SLOT_IS_CONTACTNUMBER (1 << 3)
>> #define MT_QUIRK_VALID_IS_INRANGE (1 << 4)
>> #define MT_QUIRK_VALID_IS_CONFIDENCE (1 << 5)
>> +#define MT_QUIRK_SEND_AT_EACH_REPORT (1 << 6)
>>
>> struct mt_slot {
>> __s32 x, y, p, w, h;
>> @@ -63,6 +64,9 @@ struct mt_class {
>> __s32 sn_move; /* Signal/noise ratio for move events */
>> __s32 sn_pressure; /* Signal/noise ratio for pressure events */
>> __u8 maxcontacts;
>> + __u8 override_logical_limits; /* correct the reported X/Y range */
>> + __u32 logical_min[2];
>> + __u32 logical_max[2];
>
> Please think about byte alignment here, keeping elements of the same
> size together. Also, the override needs a specific name, given that it
> applies to the whole class, not just the x and y positions. Perhaps
> the override should be triggered with a quirk instead?
I'm not in favor of a quirk in this particular case: the information
is already here: max > 0.
For the specific name, the too fields logical_min/max are not well chosen:
either use an anonymous struct to have .x, .y so we can add .pressure,
.width, etc...
or rename logical_min to logical_min_xy.
I just saw that Henrik suggested max_x, max_y, etc... chose this one, please.
The alignment makes sense!
>
>> };
>>
>> /* classes of device behavior */
>> @@ -70,6 +74,8 @@ struct mt_class {
>> #define MT_CLS_DUAL_INRANGE_CONTACTID 2
>> #define MT_CLS_DUAL_INRANGE_CONTACTNUMBER 3
>> #define MT_CLS_CYPRESS 4
>> +#define MT_CLS_EGALAX_CAPACITIVE 5
>> +#define MT_CLS_EGALAX_RESISTIVE 6
>>
>> /*
>> * these device-dependent functions determine what slot corresponds
>> @@ -119,7 +125,25 @@ struct mt_class mt_classes[] = {
>> .quirks = MT_QUIRK_NOT_SEEN_MEANS_UP |
>> MT_QUIRK_CYPRESS,
>> .maxcontacts = 10 },
>> -
>> + { .name = MT_CLS_EGALAX_CAPACITIVE,
>> + .quirks = MT_QUIRK_SLOT_IS_CONTACTID |
>> + MT_QUIRK_VALID_IS_INRANGE,
>> + .maxcontacts = 2,
>> + .sn_move = 4096,
>> + .sn_pressure = 32,
>> + .override_logical_limits = 1,
>> + .logical_min = { 0, 0 },
>> + .logical_max = { 32760, 32760 } },
>> + { .name = MT_CLS_EGALAX_RESISTIVE,
>> + .quirks = MT_QUIRK_SLOT_IS_CONTACTID |
>> + MT_QUIRK_VALID_IS_INRANGE |
>> + MT_QUIRK_SEND_AT_EACH_REPORT,
>> + .maxcontacts = 2,
>> + .sn_move = 4096,
>> + .sn_pressure = 32,
>> + .override_logical_limits = 1,
>> + .logical_min = { 0, 0 },
>> + .logical_max = { 32760, 32760 } },
>> { }
>> };
>>
>> @@ -147,11 +171,19 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>> {
>> struct mt_device *td = hid_get_drvdata(hdev);
>> struct mt_class *cls = td->mtclass;
>> +
>
> Please check for patch for unrelated changes like this.
>
>> switch (usage->hid & HID_USAGE_PAGE) {
>>
>> case HID_UP_GENDESK:
>> +
>> switch (usage->hid) {
>> case HID_GD_X:
>> + /* fix up the reported X limits if necessary*/
>> + if (cls->override_logical_limits) {
>> + field->logical_minimum = cls->logical_min[0];
>> + field->logical_maximum = cls->logical_max[0];
>> + }
>> +
>
> Something like "if (quirks & MT_QUIRK_FIX_X)", and then on next line "field->logical_maximum = cls->max_x".
or just "if (cls->max_x)" and then the two "field->logical_maximum =
cls->max_x" and "field->logical_minimum = cls->min_x".
>
>> hid_map_usage(hi, usage, bit, max,
>> EV_ABS, ABS_MT_POSITION_X);
>> set_abs(hi->input, ABS_MT_POSITION_X, field,
>> @@ -161,6 +193,11 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>> td->last_slot_field = usage->hid;
>> return 1;
>> case HID_GD_Y:
>> + /* fix up the reported Y limits if nessecary*/
>> + if (cls->override_logical_limits) {
>> + field->logical_minimum = cls->logical_min[1];
>> + field->logical_maximum = cls->logical_max[1];
>> + }
>
> Ditto.
>
>> hid_map_usage(hi, usage, bit, max,
>> EV_ABS, ABS_MT_POSITION_Y);
>> set_abs(hi->input, ABS_MT_POSITION_Y, field,
>> @@ -204,6 +241,10 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>> td->last_slot_field = usage->hid;
>> return 1;
>> case HID_DG_TIPPRESSURE:
>> + /* fix up the pressure range for some devices
>> + with broken report */
>> + field->logical_minimum = 0;
>> +
>> hid_map_usage(hi, usage, bit, max,
>> EV_ABS, ABS_MT_PRESSURE);
>> set_abs(hi->input, ABS_MT_PRESSURE, field,
>> @@ -367,8 +408,12 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
>> if (usage->hid == td->last_slot_field)
>> mt_complete_slot(td);
>>
>> - if (field->index == td->last_field_index
>> + if ((field->index == td->last_field_index
>> && td->num_received >= td->num_expected)
>> + || ((quirks & MT_QUIRK_SEND_AT_EACH_REPORT)
>> + && (usage->hid == td->last_slot_field)))
>> + /* emit an event for every finger to work around
>> + a corrupt last_field_index in the hid report*/
>> mt_emit_event(td, field->hidinput->input);
>>
>> }
>> @@ -484,6 +529,25 @@ static const struct hid_device_id mt_devices[] = {
>> HID_USB_DEVICE(USB_VENDOR_ID_CANDO,
>> USB_DEVICE_ID_CANDO_PIXCIR_MULTI_TOUCH) },
>>
>> + /* Resistive eGalax devices */
>> + { .driver_data = MT_CLS_EGALAX_RESISTIVE,
>> + HID_USB_DEVICE(USB_VENDOR_ID_DWAV,
>> + USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH) },
>> + { .driver_data = MT_CLS_EGALAX_RESISTIVE,
>> + HID_USB_DEVICE(USB_VENDOR_ID_DWAV,
>> + USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH3) },
>> +
>> + /* Capacitive eGalax devices */
>> + { .driver_data = MT_CLS_EGALAX_CAPACITIVE,
>> + HID_USB_DEVICE(USB_VENDOR_ID_DWAV,
>> + USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH1) },
>> + { .driver_data = MT_CLS_EGALAX_CAPACITIVE,
>> + HID_USB_DEVICE(USB_VENDOR_ID_DWAV,
>> + USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH2) },
>> + { .driver_data = MT_CLS_EGALAX_CAPACITIVE,
>> + HID_USB_DEVICE(USB_VENDOR_ID_DWAV,
>> + USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH4) },
>> +
>> { }
>> };
>> MODULE_DEVICE_TABLE(hid, mt_devices);
>> --
>> 1.7.1
>>
>
> Thanks,
> Henrik
>
Thanks,
Benjamin
--
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
