| Message ID | 1459195850-11128-1-git-send-email-killertofu@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@wacom.com> > --- > 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. > + 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 > + 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@wacom.com> >> --- >> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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... + */ + case HID_UP_DIGITIZER: + if (field->application == WACOM_G9_DIGITIZER || + field->application == WACOM_G11_DIGITIZER) { + if (field->report->id == 0x0B) { + hid_data->inputformat = field->report->id; + hid_data->inputformat_index = 0; + hid_data->inputformat_value = 0; /* 0: HID; 1: Vendor-defined */ + } + } + 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) { + 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); + 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;
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@wacom.com> --- drivers/hid/wacom_sys.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++-- drivers/hid/wacom_wac.h | 13 ++++++++++-- 2 files changed, 64 insertions(+), 4 deletions(-)