Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <1e06a151-5e81-d0e6-d1c3-77f7e420f208@gmail.com>
Date:   Fri, 8 Apr 2022 20:22:33 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.0
Subject: Re: [PATCH] HID: microsoft: Fix button/axis mapping for Xbox One S
 Controller
Content-Language: en-US
To:     Daniel Bomar <dbdaniel42@gmail.com>,
        Jiri Kosina <jikos@kernel.org>,
        Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc:     linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20220408140902.15966-1-dbdaniel42@gmail.com>
From:   Cameron Gutman <aicommander@gmail.com>
In-Reply-To: <20220408140902.15966-1-dbdaniel42@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 4/8/22 09:09, Daniel Bomar wrote:
> Remaps several buttons and axes to match how these are mapped in the
> xpad driver (same controller over USB).
> 
> This is also how they are documented to be mapped in
> Documentation/input/gamepad.rst
> ---

Hi Daniel,

I don't think this is a good idea. Remapping this is going to cause
problems for userspace applications and libraries that already have
mappings for this gamepad (such as SDL [0]).

At the very least, you should set a bit in the version like hid-sony.c
does so SDL can tell the new and old mappings apart. However, I still
don't think it's worth making this change at all. In the _best_ case,
you break a ton of existing games and applications that have support
for this gamepad already and achieve very little practical gain.

I believe this change will break Android's mapping too [1]. Using
GAS/BRAKE for triggers is fairly common for gamepads designed for
Android, so it's likely that Microsoft did it this way on purpose.
I don't think we should be overruling their conscious design decisions
without very compelling reasons.

It's definitely not ideal that the mappings between USB and BT differ,
but breaking existing applications by changing it is much worse.

[0]: https://github.com/libsdl-org/SDL/blob/505d6a4a052592b2676f87456c1f564daa8d2c50/src/joystick/SDL_gamecontrollerdb.h#L795
[1]: https://cs.android.com/android/platform/superproject/+/master:frameworks/base/data/keyboards/Vendor_045e_Product_02fd.kl

Regards,
Cameron

>  drivers/hid/hid-microsoft.c | 73 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 72 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/hid-microsoft.c b/drivers/hid/hid-microsoft.c
> index 071fd093a5f4..903e09a3d898 100644
> --- a/drivers/hid/hid-microsoft.c
> +++ b/drivers/hid/hid-microsoft.c
> @@ -27,6 +27,7 @@
>  #define MS_DUPLICATE_USAGES	BIT(5)
>  #define MS_SURFACE_DIAL		BIT(6)
>  #define MS_QUIRK_FF		BIT(7)
> +#define MS_XBOX			BIT(8)
>  
>  struct ms_data {
>  	unsigned long quirks;
> @@ -179,6 +180,70 @@ static int ms_surface_dial_quirk(struct hid_input *hi, struct hid_field *field,
>  	return 0;
>  }
>  
> +#define ms_map_abs_clear(c)	hid_map_usage_clear(hi, usage, bit, max, \
> +					EV_ABS, (c))
> +/*
> + * Remap buttons and axes on Xbox controllers over bluetooth so they match
> + * with the xpad driver (USB interface) and with mapping specified in
> + * Documentation/input/gamepad.rst
> +*/
> +static int ms_xbox_quirk(struct hid_input *hi, struct hid_usage *usage,
> +		unsigned long **bit, int *max)
> +{
> +	int code;
> +	switch (usage->hid & HID_USAGE_PAGE) {
> +		/*
> +		 * Remap "Xbox" and Select buttons from consumer page to gamepad buttons.
> +		 * This allows these buttons to show up on the /dev/input/js* interface.
> +		*/
> +		case HID_UP_CONSUMER:
> +			switch (usage->hid & HID_USAGE) {
> +				case 0x223:
> +					ms_map_key_clear(BTN_MODE);
> +					return 1;
> +				case 0x224:
> +					ms_map_key_clear(BTN_SELECT);
> +					return 1;
> +			}
> +			break;
> +		/* These buttons do not physically exist on the controller. Ignore them. */
> +		case HID_UP_BUTTON:
> +			code = ((usage->hid - 1) & HID_USAGE) + BTN_GAMEPAD;
> +			switch (code) {
> +				case BTN_C:
> +				case BTN_Z:
> +				case BTN_TL2:
> +				case BTN_TR2:
> +					return -1;
> +			}
> +			break;
> +		/* Remap right joystick to RX/RY */
> +		case HID_UP_GENDESK:
> +			switch (usage->hid) {
> +				case HID_GD_Z:
> +					ms_map_abs_clear(ABS_RX);
> +					return 1;
> +				case HID_GD_RZ:
> +					ms_map_abs_clear(ABS_RY);
> +					return 1;
> +			}
> +			break;
> +		/* Remap left and right triggers from "gas" and "break" to RZ/Z */
> +		case HID_UP_SIMULATION:
> +			switch (usage->hid & HID_USAGE) {
> +				case 0xc4:
> +					ms_map_abs_clear(ABS_RZ);
> +					return 1;
> +				case 0xc5:
> +					ms_map_abs_clear(ABS_Z);
> +					return 1;
> +			}
> +			break;
> +	}
> +
> +	return 0;
> +}
> +
>  static int ms_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>  		struct hid_field *field, struct hid_usage *usage,
>  		unsigned long **bit, int *max)
> @@ -203,6 +268,12 @@ static int ms_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>  			return ret;
>  	}
>  
> +	if (quirks & MS_XBOX) {
> +		int ret = ms_xbox_quirk(hi, usage, bit, max);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -447,7 +518,7 @@ static const struct hid_device_id ms_devices[] = {
>  	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, 0x091B),
>  		.driver_data = MS_SURFACE_DIAL },
>  	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_XBOX_ONE_S_CONTROLLER),
> -		.driver_data = MS_QUIRK_FF },
> +		.driver_data = MS_QUIRK_FF | MS_XBOX },
>  	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_8BITDO_SN30_PRO_PLUS),
>  		.driver_data = MS_QUIRK_FF },
>  	{ }
>