| Message ID | 20230307213536.2299487-1-svv@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | [v2] Add rumble support to latest xbox controllers | expand |
On Tue, 2023-03-07 at 13:35 -0800, 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. Looks good although I would personally have preferred separate patches for each controller. Reviewed-by: Bastien Nocera <hadess@hadess.net> Would a link to: https://en.wikipedia.org/wiki/Xbox_Wireless_Controller#Summary also be useful to make which model is which clearer in the minds of future readers? Cheers > > Signed-off-by: Siarhei Vishniakou <svv@google.com> > Change-Id: I3337a7ab5f40759c85bf67bf0dbe5d4de31ce1ff > --- > drivers/hid/hid-ids.h | 6 +++++- > drivers/hid/hid-microsoft.c | 11 ++++++++++- > 2 files changed, 15 insertions(+), 2 deletions(-) > > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h > index 053853a891c5..c9b75f8ba49a 100644 > --- a/drivers/hid/hid-ids.h > +++ b/drivers/hid/hid-ids.h > @@ -903,7 +903,11 @@ > #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 > +#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 },
Thanks Bastien! I can definitely add a link to the wikipedia page. Are you talking about adding the link to the commit message, or to hid-ids.h ? On Wed, Mar 8, 2023 at 2:23 AM Bastien Nocera <hadess@hadess.net> wrote: > > On Tue, 2023-03-07 at 13:35 -0800, 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. > > Looks good although I would personally have preferred separate patches > for each controller. > > Reviewed-by: Bastien Nocera <hadess@hadess.net> > > Would a link to: > https://en.wikipedia.org/wiki/Xbox_Wireless_Controller#Summary > also be useful to make which model is which clearer in the minds of > future readers? > > Cheers > > > > > Signed-off-by: Siarhei Vishniakou <svv@google.com> > > Change-Id: I3337a7ab5f40759c85bf67bf0dbe5d4de31ce1ff > > --- > > drivers/hid/hid-ids.h | 6 +++++- > > drivers/hid/hid-microsoft.c | 11 ++++++++++- > > 2 files changed, 15 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h > > index 053853a891c5..c9b75f8ba49a 100644 > > --- a/drivers/hid/hid-ids.h > > +++ b/drivers/hid/hid-ids.h > > @@ -903,7 +903,11 @@ > > #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 > > +#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 }, >
On Wed, 2023-03-08 at 09:55 -0800, Siarhei Vishniakou wrote: > Thanks Bastien! > > I can definitely add a link to the wikipedia page. > > Are you talking about adding the link to the commit message, or to > hid-ids.h ? I think the commit message would be enough so we know which device we're talking about, but now that you mention it, maybe the source code would be a good idea too. As far as I've understood, and Benjamin can correct me, we don't need to have IDs be in hid-ids.h because we don't need to declare them both as a blocklist in the hid core, and then again in the driver itself. My take is that the patches could move the IDs from hid-ids.h to hid- microsoft.c as they're changed. You then wouldn't need the macros, just add a comment for each of variants, and that Wikipedia article can be linked above the whole XBox controller section. What do you think? > > > On Wed, Mar 8, 2023 at 2:23 AM Bastien Nocera <hadess@hadess.net> > wrote: > > > > On Tue, 2023-03-07 at 13:35 -0800, 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. > > > > Looks good although I would personally have preferred separate > > patches > > for each controller. > > > > Reviewed-by: Bastien Nocera <hadess@hadess.net> > > > > Would a link to: > > https://en.wikipedia.org/wiki/Xbox_Wireless_Controller#Summary > > also be useful to make which model is which clearer in the minds of > > future readers? > > > > Cheers > > > > > > > > Signed-off-by: Siarhei Vishniakou <svv@google.com> > > > Change-Id: I3337a7ab5f40759c85bf67bf0dbe5d4de31ce1ff > > > --- > > > drivers/hid/hid-ids.h | 6 +++++- > > > drivers/hid/hid-microsoft.c | 11 ++++++++++- > > > 2 files changed, 15 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h > > > index 053853a891c5..c9b75f8ba49a 100644 > > > --- a/drivers/hid/hid-ids.h > > > +++ b/drivers/hid/hid-ids.h > > > @@ -903,7 +903,11 @@ > > > #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 > > > +#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 }, > >
On Wed, Mar 8, 2023 at 7:54 PM Bastien Nocera <hadess@hadess.net> wrote: > > On Wed, 2023-03-08 at 09:55 -0800, Siarhei Vishniakou wrote: > > Thanks Bastien! > > > > I can definitely add a link to the wikipedia page. > > > > Are you talking about adding the link to the commit message, or to > > hid-ids.h ? > > I think the commit message would be enough so we know which device > we're talking about, but now that you mention it, maybe the source code > would be a good idea too. Agree, adding the link in the source would be fine. > > As far as I've understood, and Benjamin can correct me, we don't need > to have IDs be in hid-ids.h because we don't need to declare them both > as a blocklist in the hid core, and then again in the driver itself. We don't need to have the blocklist anymore, but using hid-ids.h is still used, and sometimes has the benefit of raising eyebrows when you add support for a new device and realize that it was already defined. So keeping the list around is not so much of a bad thing. > > My take is that the patches could move the IDs from hid-ids.h to hid- > microsoft.c as they're changed. You then wouldn't need the macros, just > add a comment for each of variants, and that Wikipedia article can be > linked above the whole XBox controller section. We are definitely in the bikeshedding phase, but I would leave the code as it is in this patch :) One more comment below: > > What do you think? > > > > > > > On Wed, Mar 8, 2023 at 2:23 AM Bastien Nocera <hadess@hadess.net> > > wrote: > > > > > > On Tue, 2023-03-07 at 13:35 -0800, 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. > > > > > > Looks good although I would personally have preferred separate > > > patches > > > for each controller. > > > > > > Reviewed-by: Bastien Nocera <hadess@hadess.net> > > > > > > Would a link to: > > > https://en.wikipedia.org/wiki/Xbox_Wireless_Controller#Summary > > > also be useful to make which model is which clearer in the minds of > > > future readers? > > > > > > Cheers > > > > > > > > > > > Signed-off-by: Siarhei Vishniakou <svv@google.com> > > > > Change-Id: I3337a7ab5f40759c85bf67bf0dbe5d4de31ce1ff That change-id should be dropped, it has no meaning to us. Cheers, Benjamin > > > > --- > > > > drivers/hid/hid-ids.h | 6 +++++- > > > > drivers/hid/hid-microsoft.c | 11 ++++++++++- > > > > 2 files changed, 15 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h > > > > index 053853a891c5..c9b75f8ba49a 100644 > > > > --- a/drivers/hid/hid-ids.h > > > > +++ b/drivers/hid/hid-ids.h > > > > @@ -903,7 +903,11 @@ > > > > #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 > > > > +#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 }, > > > >
Thanks folks, I uploaded a v3 patch with the following changes: 1. Added link to wikipedia 2. Removed Change-Id from commit message 3. Added Bastien's 'Reviewed-by'. On Thu, Mar 9, 2023 at 6:56 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > On Wed, Mar 8, 2023 at 7:54 PM Bastien Nocera <hadess@hadess.net> wrote: > > > > On Wed, 2023-03-08 at 09:55 -0800, Siarhei Vishniakou wrote: > > > Thanks Bastien! > > > > > > I can definitely add a link to the wikipedia page. > > > > > > Are you talking about adding the link to the commit message, or to > > > hid-ids.h ? > > > > I think the commit message would be enough so we know which device > > we're talking about, but now that you mention it, maybe the source code > > would be a good idea too. > > Agree, adding the link in the source would be fine. > > > > > As far as I've understood, and Benjamin can correct me, we don't need > > to have IDs be in hid-ids.h because we don't need to declare them both > > as a blocklist in the hid core, and then again in the driver itself. > > We don't need to have the blocklist anymore, but using hid-ids.h is > still used, and sometimes has the benefit of raising eyebrows when you > add support for a new device and realize that it was already defined. > So keeping the list around is not so much of a bad thing. > > > > > My take is that the patches could move the IDs from hid-ids.h to hid- > > microsoft.c as they're changed. You then wouldn't need the macros, just > > add a comment for each of variants, and that Wikipedia article can be > > linked above the whole XBox controller section. > > We are definitely in the bikeshedding phase, but I would leave the > code as it is in this patch :) > > One more comment below: > > > > > What do you think? > > > > > > > > > > > On Wed, Mar 8, 2023 at 2:23 AM Bastien Nocera <hadess@hadess.net> > > > wrote: > > > > > > > > On Tue, 2023-03-07 at 13:35 -0800, 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. > > > > > > > > Looks good although I would personally have preferred separate > > > > patches > > > > for each controller. > > > > > > > > Reviewed-by: Bastien Nocera <hadess@hadess.net> > > > > > > > > Would a link to: > > > > https://en.wikipedia.org/wiki/Xbox_Wireless_Controller#Summary > > > > also be useful to make which model is which clearer in the minds of > > > > future readers? > > > > > > > > Cheers > > > > > > > > > > > > > > Signed-off-by: Siarhei Vishniakou <svv@google.com> > > > > > Change-Id: I3337a7ab5f40759c85bf67bf0dbe5d4de31ce1ff > > That change-id should be dropped, it has no meaning to us. > > Cheers, > Benjamin > > > > > > --- > > > > > drivers/hid/hid-ids.h | 6 +++++- > > > > > drivers/hid/hid-microsoft.c | 11 ++++++++++- > > > > > 2 files changed, 15 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h > > > > > index 053853a891c5..c9b75f8ba49a 100644 > > > > > --- a/drivers/hid/hid-ids.h > > > > > +++ b/drivers/hid/hid-ids.h > > > > > @@ -903,7 +903,11 @@ > > > > > #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 > > > > > +#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 }, > > > > > > >
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h index 053853a891c5..c9b75f8ba49a 100644 --- a/drivers/hid/hid-ids.h +++ b/drivers/hid/hid-ids.h @@ -903,7 +903,11 @@ #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 +#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 },
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. Signed-off-by: Siarhei Vishniakou <svv@google.com> Change-Id: I3337a7ab5f40759c85bf67bf0dbe5d4de31ce1ff --- drivers/hid/hid-ids.h | 6 +++++- drivers/hid/hid-microsoft.c | 11 ++++++++++- 2 files changed, 15 insertions(+), 2 deletions(-)