Re: [PATCH] HID: wacom: Support switching from vendor-defined input format on G9 and G11

[Jason Gerecke <killertofu@gmail.com>]

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