On 03/30/2016 02:24 AM, Benjamin Tissoires wrote: > On Mar 29 2016 or thereabouts, Jason Gerecke wrote: >> On 03/29/2016 12:05 AM, Benjamin Tissoires wrote: >>> Hi Jason, >>> >>> On Mar 28 2016 or thereabouts, Jason Gerecke wrote: >>>> A tablet PC booted into Windows may have its pen/touch hardware switched >>>> into "Wacom mode" similar to what we do with explicitly-supported hardware. >>>> Some devices appear to maintain this state across reboots, preventing their >>>> use with the generic HID driver. This patch adds support for the vendor- >>>> defined "input format" feature report of G9 and G11 chips and has the HID >>>> codepath always attempt to reset the device back to an appropriate format. >>>> >>>> Fixes: https://sourceforge.net/p/linuxwacom/bugs/307/ >>>> Fixes: https://sourceforge.net/p/linuxwacom/bugs/310/ >>>> Fixes: https://github.com/linuxwacom/input-wacom/issues/15 >>>> >>>> Signed-off-by: Jason Gerecke <jason.gerecke@xxxxxxxxx> >>>> --- >>>> drivers/hid/wacom_sys.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++-- >>>> drivers/hid/wacom_wac.h | 13 ++++++++++-- >>>> 2 files changed, 64 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c >>>> index 68a5609..d7ef7b0 100644 >>>> --- a/drivers/hid/wacom_sys.c >>>> +++ b/drivers/hid/wacom_sys.c >>>> @@ -152,6 +152,32 @@ static void wacom_feature_mapping(struct hid_device *hdev, >>>> hid_data->inputmode = field->report->id; >>>> hid_data->inputmode_index = usage->usage_index; >>>> break; >>>> + >>>> + /* The vendor-defined application collections leave most usages >>>> + * as null (0x0000), making them much more difficult to match... >>>> + */ >>> >>> Not so sure this comments adds anything to the code. The application >>> usage is well defined, so there is no need to worry I think. >>> >>> (Plus the coding style should be: >>> /* >>> * multi lines >>> * comment. >>> */ >>> ) >>> >>>> + case HID_UP_DIGITIZER: >>>> + if (field->application == WACOM_G9_DIGITIZER || >>>> + field->application == WACOM_G11_DIGITIZER) { >>>> + if (field->report->id == 0x0B) { >>> >>> I'd rather have the test on the report id first, and have only one if >>> (with id && (app || app). This way, you'll keep the extra tab below. >>> >>>> + hid_data->inputformat = field->report->id; >>>> + hid_data->inputformat_index = 0; >>> >>> Shouldn't this be usage->usage_index instead? Unless it's defined >>> several times, which would make the use of usage_index moot. >>> >> >> The field is the entire contents of one of Wacom's infamous "n >> vendor-defined bytes" reports, so although the usage itself isn't >> defined multiple times it does have a report count associated with it >> (with the same net effect). > > Fair enough. But I wonder if we need to keep the inputformat_index field > given that it will be always 0. > >> >>>> + hid_data->inputformat_value = 0; /* 0: HID; 1: Vendor-defined */ >>> >>> Looks like both usages are requesting this to be set at 0. I think it's >>> fine to keep it, but I don't know if it's that important to keep then. >>> >>>> + } >>>> + } >>>> + break; >>>> + >>>> + case WACOM_G9_PAGE: >>>> + case WACOM_G11_PAGE: >>>> + if (field->application == WACOM_G9_TOUCHSCREEN || >>>> + field->application == WACOM_G11_TOUCHSCREEN) { >>>> + if (field->report->id == 0x03) { >>> >>> Same three comments above apply here. >>> >>>> + hid_data->inputformat = field->report->id; >>>> + hid_data->inputformat_index = 0; >>>> + hid_data->inputformat_value = 0; /* 0: HID; 4: Vendor-defined */ >>>> + } >>>> + } >>>> + break; >>>> } >>>> } >>>> >>>> @@ -322,6 +348,25 @@ static int wacom_hid_set_device_mode(struct hid_device *hdev) >>>> return 0; >>>> } >>>> >>>> +static int wacom_hid_set_device_format(struct hid_device *hdev) >>>> +{ >>>> + struct wacom *wacom = hid_get_drvdata(hdev); >>>> + struct hid_data *hid_data = &wacom->wacom_wac.hid_data; >>>> + struct hid_report *r; >>>> + struct hid_report_enum *re; >>>> + >>>> + if (hid_data->inputformat < 0) >>>> + return 0; >>>> + >>>> + re = &(hdev->report_enum[HID_FEATURE_REPORT]); >>>> + r = re->report_id_hash[hid_data->inputformat]; >>>> + if (r) { >>>> + r->field[0]->value[hid_data->inputformat_index] = hid_data->inputformat_value; >>>> + hid_hw_request(hdev, r, HID_REQ_SET_REPORT); >>>> + } >>>> + return 0; >>>> +} >>>> + >>>> static int wacom_set_device_mode(struct hid_device *hdev, int report_id, >>>> int length, int mode) >>>> { >>>> @@ -414,8 +459,14 @@ static int wacom_query_tablet_data(struct hid_device *hdev, >>>> if (hdev->bus == BUS_BLUETOOTH) >>>> return wacom_bt_query_tablet_data(hdev, 1, features); >>>> >>>> - if (features->type == HID_GENERIC) >>>> - return wacom_hid_set_device_mode(hdev); >>>> + if (features->type == HID_GENERIC) { >>>> + int err; >>>> + err = wacom_hid_set_device_mode(hdev); >>>> + if (err) >>>> + return err; >>>> + err = wacom_hid_set_device_format(hdev); >>> >>> I don't really see the difference between "input mode" and "input >>> format". Does the device needs both to be set? If not, couldn't we >>> extend the input mode feature for these usages? >>> >>> Cheers, >>> Benjamin >>> >> >> Device / Input mode is a standard HID feature expected to be present on >> Windows 7-compatible touch devices which is used to switch a device >> between sending mouse, single, or multi-touch reports[1]. The >> 'wacom_hid_set_device_mode' function right now takes care of switching >> into multi-touch mode provided the hid_data->inputmode field has been set. > > Well, "Device Mode" is also used by MS for Pen devices (values are 0 - > Mouse, and 1 - Pen). So it's more a generic mode for the device than a > multitouch mode. > >> >> Input format on the other hand is a vendor-defined feature that is used >> to switch Wacom devices (both touchscreens and tablets) between sending >> standard HID reports or vendor-defined reports. Devices are supposed to >> start up sending the former, but at least two tablet PCs have had >> hardware that seems to remember being switched to the vendor-defined format. >> >> If the format is explicitly switched to HID standard, we still should >> probably set the mode as well since I don't think there's anything >> implied about which mode the tablet should use after switching. > > OK, then in that regard, it would make more sense to first switch the > device back into the HID mode, and then switch it to multitouch. > >> >> Relatedly, it's probably a good idea to rename 'wacom_set_device_mode' >> or 'wacom_hid_set_device_mode' since they each set different a different >> kind of "mode". It _might_ also be reasonable to rework >> 'wacom_set_device_mode' to be compatible with HID_GENERIC devices >> (rather than introducing 'wacom_hid_set_device_format'), but keeping >> compatibility the explicit devices might be tricky... > > OK, how about the following: > we keep wacom_set_device_mode() for switching the proprietary mode, but > add a mode_value and mode_report in wacom_wac. This way, we can detect > those proprietary mode changes in feature_mapping, and still keep the > HID Device Mode bits in place. > > I tried to come up with the following, which seems to be working on the > Bamboo PAD, Intuos Pro S, Bamboo 16FG, and Cintiq 13HDT: > > --- > diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c > index 68a5609..90fa4d4 100644 > --- a/drivers/hid/wacom_sys.c > +++ b/drivers/hid/wacom_sys.c > @@ -322,26 +322,41 @@ static int wacom_hid_set_device_mode(struct hid_device *hdev) > return 0; > } > > -static int wacom_set_device_mode(struct hid_device *hdev, int report_id, > - int length, int mode) > +static int wacom_set_device_mode(struct hid_device *hdev, > + struct wacom_wac *wacom_wac) > { > - unsigned char *rep_data; > + u8 *rep_data; > + struct hid_report *r; > + struct hid_report_enum *re; > + int length; > int error = -ENOMEM, limit = 0; > > - rep_data = kzalloc(length, GFP_KERNEL); > + if (wacom_wac->mode_report < 0) > + return 0; > + > + re = &(hdev->report_enum[HID_FEATURE_REPORT]); > + r = re->report_id_hash[wacom_wac->mode_report]; > + if (!r) > + return -EINVAL; > + > + rep_data = hid_alloc_report_buf(r, GFP_KERNEL); > if (!rep_data) > - return error; > + return -ENOMEM; > + > + length = hid_report_len(r); > > do { > - rep_data[0] = report_id; > - rep_data[1] = mode; > + rep_data[0] = wacom_wac->mode_report; > + rep_data[1] = wacom_wac->mode_value; > > error = wacom_set_report(hdev, HID_FEATURE_REPORT, rep_data, > length, 1); > if (error >= 0) > error = wacom_get_report(hdev, HID_FEATURE_REPORT, > rep_data, length, 1); > - } while (error >= 0 && rep_data[1] != mode && limit++ < WAC_MSG_RETRIES); > + } while (error >= 0 && > + rep_data[1] != wacom_wac->mode_report && > + limit++ < WAC_MSG_RETRIES); > > kfree(rep_data); > > @@ -411,32 +426,41 @@ static int wacom_bt_query_tablet_data(struct hid_device *hdev, u8 speed, > static int wacom_query_tablet_data(struct hid_device *hdev, > struct wacom_features *features) > { > + struct wacom *wacom = hid_get_drvdata(hdev); > + struct wacom_wac *wacom_wac = &wacom->wacom_wac; > + > if (hdev->bus == BUS_BLUETOOTH) > return wacom_bt_query_tablet_data(hdev, 1, features); > > - if (features->type == HID_GENERIC) > - return wacom_hid_set_device_mode(hdev); > - > - if (features->device_type & WACOM_DEVICETYPE_TOUCH) { > - if (features->type > TABLETPC) { > - /* MT Tablet PC touch */ > - return wacom_set_device_mode(hdev, 3, 4, 4); > - } > - else if (features->type == WACOM_24HDT) { > - return wacom_set_device_mode(hdev, 18, 3, 2); > - } > - else if (features->type == WACOM_27QHDT) { > - return wacom_set_device_mode(hdev, 131, 3, 2); > - } > - else if (features->type == BAMBOO_PAD) { > - return wacom_set_device_mode(hdev, 2, 2, 2); > - } > - } else if (features->device_type & WACOM_DEVICETYPE_PEN) { > - if (features->type <= BAMBOO_PT) { > - return wacom_set_device_mode(hdev, 2, 2, 2); > + if (features->type != HID_GENERIC) { > + if (features->device_type & WACOM_DEVICETYPE_TOUCH) { > + if (features->type > TABLETPC) { > + /* MT Tablet PC touch */ > + wacom_wac->mode_report = 3; > + wacom_wac->mode_value = 4; > + } else if (features->type == WACOM_24HDT) { > + wacom_wac->mode_report = 18; > + wacom_wac->mode_value = 2; > + } else if (features->type == WACOM_27QHDT) { > + wacom_wac->mode_report = 131; > + wacom_wac->mode_value = 2; > + } else if (features->type == BAMBOO_PAD) { > + wacom_wac->mode_report = 2; > + wacom_wac->mode_value = 2; > + } > + } else if (features->device_type & WACOM_DEVICETYPE_PEN) { > + if (features->type <= BAMBOO_PT) { > + wacom_wac->mode_report = 2; > + wacom_wac->mode_value = 2; > + } > } > } > > + wacom_set_device_mode(hdev, wacom_wac); > + > + if (features->type == HID_GENERIC) > + return wacom_hid_set_device_mode(hdev); > + > return 0; > } > > @@ -1817,6 +1841,9 @@ static int wacom_probe(struct hid_device *hdev, > goto fail_type; > } > > + wacom_wac->hid_data.inputmode = -1; I noticed this was missing the other day but then totally forgot to fix it. Thanks for the reminder :) > + wacom_wac->mode_report = -1; > + > wacom->usbdev = dev; > wacom->intf = intf; > mutex_init(&wacom->lock); > diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h > index 25baa7f..8f40e45 100644 > --- a/drivers/hid/wacom_wac.h > +++ b/drivers/hid/wacom_wac.h > @@ -238,6 +238,8 @@ struct wacom_wac { > int ps_connected; > u8 bt_features; > u8 bt_high_speed; > + int mode_report; > + int mode_value; > struct hid_data hid_data; > }; > > --- > > This patch infers the length of the feature from the report descriptor, > but I would expect this to be correct on all devices. > > Any tests/remarks would be appreciated :) > > Cheers, > Benjamin > I think I like the look of this :) I'll do a bit more checking on my end and get back to you shortly. Jason --- Now instead of four in the eights place / you’ve got three, ‘Cause you added one / (That is to say, eight) to the two, / But you can’t take seven from three, / So you look at the sixty-fours.... >> >> Off to work on V2... >> >> [1]: >> https://msdn.microsoft.com/en-us/library/windows/hardware/ff553739%28v=vs.85%29.aspx >> >> Jason >> --- >> Now instead of four in the eights place / >> you’ve got three, ‘Cause you added one / >> (That is to say, eight) to the two, / >> But you can’t take seven from three, / >> So you look at the sixty-fours.... >> >>>> + return err; >>>> + } >>>> >>>> if (features->device_type & WACOM_DEVICETYPE_TOUCH) { >>>> if (features->type > TABLETPC) { >>>> diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h >>>> index 25baa7f..37d5dec 100644 >>>> --- a/drivers/hid/wacom_wac.h >>>> +++ b/drivers/hid/wacom_wac.h >>>> @@ -84,6 +84,12 @@ >>>> #define WACOM_DEVICETYPE_WL_MONITOR 0x0008 >>>> >>>> #define WACOM_VENDORDEFINED_PEN 0xff0d0001 >>>> +#define WACOM_G9_PAGE 0xff090000 >>>> +#define WACOM_G9_DIGITIZER (WACOM_G9_PAGE | 0x02) >>>> +#define WACOM_G9_TOUCHSCREEN (WACOM_G9_PAGE | 0x11) >>>> +#define WACOM_G11_PAGE 0xff110000 >>>> +#define WACOM_G11_DIGITIZER (WACOM_G11_PAGE | 0x02) >>>> +#define WACOM_G11_TOUCHSCREEN (WACOM_G11_PAGE | 0x11) >>>> >>>> #define WACOM_PEN_FIELD(f) (((f)->logical == HID_DG_STYLUS) || \ >>>> ((f)->physical == HID_DG_STYLUS) || \ >>>> @@ -193,8 +199,11 @@ struct wacom_shared { >>>> }; >>>> >>>> struct hid_data { >>>> - __s16 inputmode; /* InputMode HID feature, -1 if non-existent */ >>>> - __s16 inputmode_index; /* InputMode HID feature index in the report */ >>>> + __s16 inputmode; /* InputMode HID feature, -1 if non-existent */ >>>> + __s16 inputmode_index; /* InputMode HID feature index in the report */ >>>> + __s16 inputformat; /* Vendor-defined "input format" feature, -1 if non-existent */ >>>> + __s16 inputformat_index; /* Vendor-defined "input format" feature index in the report */ >>>> + __u8 inputformat_value; /* Value expected by device to switch to HID mode */ >>>> bool inrange_state; >>>> bool invert_state; >>>> bool tipswitch; >>>> -- >>>> 2.7.3 >>>> -- 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
