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). >> + 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. 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. 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... 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
