diff mbox series

[v3] Add rumble support to latest xbox controllers

Message ID 20230425010618.2303982-1-svv@google.com (mailing list archive)
State Superseded
Delegated to: Jiri Kosina
Headers show
Series [v3] Add rumble support to latest xbox controllers | expand

Commit Message

Siarhei Vishniakou April 25, 2023, 1:06 a.m. UTC 
Currently, rumble is only supported via bluetooth on a single xbox
controller, called 'model 1708'. On the back of the device, it's named
'wireless controller for xbox one'. However, in 2021, Microsoft released
a firmware update for this controller. As part of this update, the HID
descriptor of the device changed. The product ID was also changed from
0x02fd to 0x0b20. On this controller, rumble was supported via
hid-microsoft, which matched against the old product id (0x02fd). As a
result, the firmware update broke rumble support on this controller.

The hid-microsoft driver actually supports rumble on the new firmware,
as well. So simply adding new product id is sufficient to bring back
this support.

After discussing further with the xbox team, it was pointed out that
another xbox controller, xbox elite series 2, can be supported in a
similar way.

Add rumble support for all of these devices in this patch. Two of the
devices have received firmware updates that caused their product id's to
change. Both old and new firmware versions of these devices were tested.

The tested controllers are:

1. 'wireless controller for xbox one', model 1708
2. 'xbox wireless controller', model 1914. This is also sometimes
   referred to as 'xbox series S|X'.
3. 'elite series 2', model 1797.

The tested configurations are:
1. model 1708, pid 0x02fd (old firmware)
2. model 1708, pid 0x0b20 (new firmware)
3. model 1914, pid 0x0b13
4. model 1797, pid 0x0b05 (old firmware)
5. model 1797, pid 0x0b22 (new firmware)

I verified rumble support on both bluetooth and usb.

Reviewed-by: Bastien Nocera <hadess@hadess.net>
Signed-off-by: Siarhei Vishniakou <svv@google.com>
---
 drivers/hid/hid-ids.h       |  8 +++++++-
 drivers/hid/hid-microsoft.c | 11 ++++++++++-
 2 files changed, 17 insertions(+), 2 deletions(-)

Comments

Bastien Nocera April 25, 2023, 8:26 a.m. UTC | #1 
On Mon, 2023-04-24 at 18:06 -0700, Siarhei Vishniakou wrote:
> Currently, rumble is only supported via bluetooth on a single xbox
> controller, called 'model 1708'. On the back of the device, it's named
> 'wireless controller for xbox one'. However, in 2021, Microsoft released
> a firmware update for this controller. As part of this update, the HID
> descriptor of the device changed. The product ID was also changed from
> 0x02fd to 0x0b20. On this controller, rumble was supported via
> hid-microsoft, which matched against the old product id (0x02fd). As a
> result, the firmware update broke rumble support on this controller.
> 
> The hid-microsoft driver actually supports rumble on the new firmware,
> as well. So simply adding new product id is sufficient to bring back
> this support.
> 
> After discussing further with the xbox team, it was pointed out that
> another xbox controller, xbox elite series 2, can be supported in a
> similar way.
> 
> Add rumble support for all of these devices in this patch. Two of the
> devices have received firmware updates that caused their product id's to
> change. Both old and new firmware versions of these devices were tested.
> 
> The tested controllers are:
> 
> 1. 'wireless controller for xbox one', model 1708
> 2. 'xbox wireless controller', model 1914. This is also sometimes
>    referred to as 'xbox series S|X'.
> 3. 'elite series 2', model 1797.
> 
> The tested configurations are:
> 1. model 1708, pid 0x02fd (old firmware)
> 2. model 1708, pid 0x0b20 (new firmware)
> 3. model 1914, pid 0x0b13
> 4. model 1797, pid 0x0b05 (old firmware)
> 5. model 1797, pid 0x0b22 (new firmware)
> 
> I verified rumble support on both bluetooth and usb.
> 
> Reviewed-by: Bastien Nocera <hadess@hadess.net>
> Signed-off-by: Siarhei Vishniakou <svv@google.com>
> ---
>  drivers/hid/hid-ids.h       |  8 +++++++-
>  drivers/hid/hid-microsoft.c | 11 ++++++++++-
>  2 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 053853a891c5..5d98b2a3a5e4 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -903,7 +903,13 @@
>  #define USB_DEVICE_ID_MS_TYPE_COVER_2    0x07a9
>  #define USB_DEVICE_ID_MS_POWER_COVER     0x07da
>  #define USB_DEVICE_ID_MS_SURFACE3_COVER                0x07de
> -#define USB_DEVICE_ID_MS_XBOX_ONE_S_CONTROLLER 0x02fd
> +// For a description of the Xbox controller models, refer to:
> +// https://en.wikipedia.org/wiki/Xbox_Wireless_Controller#Summary

The kernel doesn't use C++ style comments, but C-style comments.
Checking your patch with checkpatch.pl should warn about it:
https://docs.kernel.org/process/submitting-patches.html#style-check-your-changes

> +#define USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1708    0x02fd
> +#define USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1708_BLE        0x0b20

The 1708 model uses Bluetooth Classic, not Bluetooth LE.

> +#define USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1914    0x0b13
> +#define USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1797    0x0b05
> +#define USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1797_BLE        0x0b22
>  #define USB_DEVICE_ID_MS_PIXART_MOUSE    0x00cb
>  #define USB_DEVICE_ID_8BITDO_SN30_PRO_PLUS      0x02e0
>  
> diff --git a/drivers/hid/hid-microsoft.c b/drivers/hid/hid-microsoft.c
> index 071fd093a5f4..9345e2bfd56e 100644
> --- a/drivers/hid/hid-microsoft.c
> +++ b/drivers/hid/hid-microsoft.c
> @@ -446,7 +446,16 @@ static const struct hid_device_id ms_devices[] = {
>                 .driver_data = MS_PRESENTER },
>         { 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),
> +
> +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1708),
> +               .driver_data = MS_QUIRK_FF },
> +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1708_BLE),
> +               .driver_data = MS_QUIRK_FF },
> +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1914),
> +               .driver_data = MS_QUIRK_FF },
> +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1797),
> +               .driver_data = MS_QUIRK_FF },
> +       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1797_BLE),
>                 .driver_data = MS_QUIRK_FF },
>         { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_8BITDO_SN30_PRO_PLUS),
>                 .driver_data = MS_QUIRK_FF },
Bastien Nocera April 25, 2023, 4:07 p.m. UTC | #2 
On Tue, 2023-04-25 at 10:46 -0500, Edward Matijevic wrote:
> > > +#define USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1708    0x02fd
> > > +#define
> > > USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1708_BLE        0x0b20
> > The 1708 model uses Bluetooth Classic, not Bluetooth LE.
> 
> The new firmware adds Bluetooth LE support to the 1708 
> and prioritizes BLE over Classic which necessitates the change
> The controllers are broken without the "new firmware" IDs which are
> for BLE

Oh! I completely missed that. So both the 1708 and 1797 used Bluetooth
Classic with the old firmware, and support Bluetooth LE with the new
one. Am I understanding this correctly?

If that's right, looks like I might need to update Wikipedia ;)

Seeing as you will be updating the patch for that comment style
problem, you could probably add a reference to this article in the
commit message, it seems authoritative enough:
https://news.xbox.com/en-us/2021/09/08/xbox-controller-firmware-update-rolling-out-to-insiders-starting-today/
Siarhei Vishniakou April 25, 2023, 4:40 p.m. UTC | #3 
Right, confirmed on the BLE support.

I uploaded a new patch with the C-style comments and added the
xbox.com link to the commit message.

On Tue, Apr 25, 2023 at 9:07 AM Bastien Nocera <hadess@hadess.net> wrote:
>
> On Tue, 2023-04-25 at 10:46 -0500, Edward Matijevic wrote:
> > > > +#define USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1708    0x02fd
> > > > +#define
> > > > USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1708_BLE        0x0b20
> > > The 1708 model uses Bluetooth Classic, not Bluetooth LE.
> >
> > The new firmware adds Bluetooth LE support to the 1708
> > and prioritizes BLE over Classic which necessitates the change
> > The controllers are broken without the "new firmware" IDs which are
> > for BLE
>
> Oh! I completely missed that. So both the 1708 and 1797 used Bluetooth
> Classic with the old firmware, and support Bluetooth LE with the new
> one. Am I understanding this correctly?
>
> If that's right, looks like I might need to update Wikipedia ;)
>
> Seeing as you will be updating the patch for that comment style
> problem, you could probably add a reference to this article in the
> commit message, it seems authoritative enough:
> https://news.xbox.com/en-us/2021/09/08/xbox-controller-firmware-update-rolling-out-to-insiders-starting-today/
diff mbox series

Patch

diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 053853a891c5..5d98b2a3a5e4 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -903,7 +903,13 @@ 
 #define USB_DEVICE_ID_MS_TYPE_COVER_2    0x07a9
 #define USB_DEVICE_ID_MS_POWER_COVER     0x07da
 #define USB_DEVICE_ID_MS_SURFACE3_COVER		0x07de
-#define USB_DEVICE_ID_MS_XBOX_ONE_S_CONTROLLER	0x02fd
+// For a description of the Xbox controller models, refer to:
+// https://en.wikipedia.org/wiki/Xbox_Wireless_Controller#Summary
+#define USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1708	0x02fd
+#define USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1708_BLE	0x0b20
+#define USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1914	0x0b13
+#define USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1797	0x0b05
+#define USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1797_BLE	0x0b22
 #define USB_DEVICE_ID_MS_PIXART_MOUSE    0x00cb
 #define USB_DEVICE_ID_8BITDO_SN30_PRO_PLUS      0x02e0
 
diff --git a/drivers/hid/hid-microsoft.c b/drivers/hid/hid-microsoft.c
index 071fd093a5f4..9345e2bfd56e 100644
--- a/drivers/hid/hid-microsoft.c
+++ b/drivers/hid/hid-microsoft.c
@@ -446,7 +446,16 @@  static const struct hid_device_id ms_devices[] = {
 		.driver_data = MS_PRESENTER },
 	{ 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),
+
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1708),
+		.driver_data = MS_QUIRK_FF },
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1708_BLE),
+		.driver_data = MS_QUIRK_FF },
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1914),
+		.driver_data = MS_QUIRK_FF },
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1797),
+		.driver_data = MS_QUIRK_FF },
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1797_BLE),
 		.driver_data = MS_QUIRK_FF },
 	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_8BITDO_SN30_PRO_PLUS),
 		.driver_data = MS_QUIRK_FF },