* [PATCH] hid: microsoft: Add rumble support for Xbox One S controller @ 2018-08-10 0:17 Andrey Smirnov 2018-08-10 9:36 ` Benjamin Tissoires ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: Andrey Smirnov @ 2018-08-10 0:17 UTC (permalink / raw) To: Jiri Kosina Cc: Andrey Smirnov, Dmitry Torokhov, Benjamin Tissoires, linux-input, linux-kernel, Pierre-Loup A . Griffais, Juha Kuikka Add HID quirk driver for Xbox One S controller over bluetooth. This driver only adds support for rumble. Standard controller functionality is exposed by default HID driver. Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> Cc: Jiri Kosina <jikos@kernel.org> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com> Cc: linux-input@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: Pierre-Loup A. Griffais <pgriffais@valvesoftware.com> Signed-off-by: Juha Kuikka <juha.kuikka@gmail.com> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> --- drivers/hid/Kconfig | 8 ++ drivers/hid/hid-ids.h | 1 + drivers/hid/hid-microsoft.c | 142 ++++++++++++++++++++++++++++++++++-- drivers/hid/hid-quirks.c | 1 + 4 files changed, 147 insertions(+), 5 deletions(-) diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig index a49a10437c40..b2e983cb32f0 100644 --- a/drivers/hid/Kconfig +++ b/drivers/hid/Kconfig @@ -589,6 +589,14 @@ config HID_MICROSOFT ---help--- Support for Microsoft devices that are not fully compliant with HID standard. +config MICROSOFT_FF + bool "Microsoft Xbox One S controller force feedback support" + depends on HID_MICROSOFT + select INPUT_FF_MEMLESS + ---help--- + Say Y here if you have a Microsoft Xbox One S controller and want to enable + force feedback support for it. + config HID_MONTEREY tristate "Monterey Genius KB29E keyboard" depends on HID diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h index c7981ddd8776..c8b4bc7fe053 100644 --- a/drivers/hid/hid-ids.h +++ b/drivers/hid/hid-ids.h @@ -798,6 +798,7 @@ #define USB_DEVICE_ID_MS_TOUCH_COVER_2 0x07a7 #define USB_DEVICE_ID_MS_TYPE_COVER_2 0x07a9 #define USB_DEVICE_ID_MS_POWER_COVER 0x07da +#define USB_DEVICE_ID_MS_XBOX_ONE_S_CONTROLLER 0x02fd #define USB_VENDOR_ID_MOJO 0x8282 #define USB_DEVICE_ID_RETRO_ADAPTER 0x3201 diff --git a/drivers/hid/hid-microsoft.c b/drivers/hid/hid-microsoft.c index 96e7d3231d2f..e2bd31d54e62 100644 --- a/drivers/hid/hid-microsoft.c +++ b/drivers/hid/hid-microsoft.c @@ -29,10 +29,29 @@ #define MS_NOGET 0x10 #define MS_DUPLICATE_USAGES 0x20 +struct ms_data { + unsigned long quirks; + struct hid_device *hdev; + struct work_struct ff_worker; + __u8 strong; + __u8 weak; + __u8 *output_report_dmabuf; +}; + +struct xb1s_ff_report { + __u8 report_id; + __u8 enable; + __u8 magnitude[4]; + __u8 duration_10ms; + __u8 start_delay_10ms; + __u8 loop_count; +}; + static __u8 *ms_report_fixup(struct hid_device *hdev, __u8 *rdesc, unsigned int *rsize) { - unsigned long quirks = (unsigned long)hid_get_drvdata(hdev); + struct ms_data *ms = hid_get_drvdata(hdev); + unsigned long quirks = ms->quirks; /* * Microsoft Wireless Desktop Receiver (Model 1028) has @@ -134,7 +153,8 @@ 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) { - unsigned long quirks = (unsigned long)hid_get_drvdata(hdev); + struct ms_data *ms = hid_get_drvdata(hdev); + unsigned long quirks = ms->quirks; if (quirks & MS_ERGONOMY) { int ret = ms_ergonomy_kb_quirk(hi, usage, bit, max); @@ -153,7 +173,8 @@ static int ms_input_mapped(struct hid_device *hdev, struct hid_input *hi, struct hid_field *field, struct hid_usage *usage, unsigned long **bit, int *max) { - unsigned long quirks = (unsigned long)hid_get_drvdata(hdev); + struct ms_data *ms = hid_get_drvdata(hdev); + unsigned long quirks = ms->quirks; if (quirks & MS_DUPLICATE_USAGES) clear_bit(usage->code, *bit); @@ -164,7 +185,8 @@ static int ms_input_mapped(struct hid_device *hdev, struct hid_input *hi, static int ms_event(struct hid_device *hdev, struct hid_field *field, struct hid_usage *usage, __s32 value) { - unsigned long quirks = (unsigned long)hid_get_drvdata(hdev); + struct ms_data *ms = hid_get_drvdata(hdev); + unsigned long quirks = ms->quirks; struct input_dev *input; if (!(hdev->claimed & HID_CLAIMED_INPUT) || !field->hidinput || @@ -219,12 +241,96 @@ static int ms_event(struct hid_device *hdev, struct hid_field *field, return 0; } +#ifdef CONFIG_MICROSOFT_FF + +#define XB1S_FF_REPORT 3 +#define ENABLE_WEAK BIT(0) +#define ENABLE_STRONG BIT(1) +#define MAGNITUDE_WEAK 3 +#define MAGNITUDE_STRONG 2 + +static void ms_ff_worker(struct work_struct *work) +{ + struct ms_data *ms = container_of(work, struct ms_data, ff_worker); + struct hid_device *hdev = ms->hdev; + struct xb1s_ff_report *r = + (struct xb1s_ff_report *) ms->output_report_dmabuf; + int ret; + + memset(r, 0, sizeof(*r)); + + r->report_id = XB1S_FF_REPORT; + r->enable = ENABLE_WEAK | ENABLE_STRONG; + /* + * Specifying maximum duration and maximum loop count should + * cover maximum duration of a single effect: 65536 ms + */ + r->duration_10ms = U8_MAX; + r->loop_count = U8_MAX; + r->magnitude[MAGNITUDE_STRONG] = ms->strong; /* left actuator */ + r->magnitude[MAGNITUDE_WEAK] = ms->weak; /* right actuator */ + + ret = hid_hw_output_report(hdev, (__u8 *)r, sizeof(*r)); + if (ret) + hid_warn(hdev, "failed to send FF report\n"); +} + +static int ms_play_effect(struct input_dev *dev, void *data, + struct ff_effect *effect) +{ + struct hid_device *hid = input_get_drvdata(dev); + struct ms_data *ms = hid_get_drvdata(hid); + + if (effect->type != FF_RUMBLE) + return 0; + + /* Magnitude is 0..100 so scale the 16-bit input here */ + ms->strong = ((u32) effect->u.rumble.strong_magnitude * 100) / U16_MAX; + ms->weak = ((u32) effect->u.rumble.weak_magnitude * 100) / U16_MAX; + + schedule_work(&ms->ff_worker); + return 0; +} + +static int ms_init_ff(struct hid_device *hdev, struct ms_data *ms) +{ + struct hid_input *hidinput = list_entry(hdev->inputs.next, + struct hid_input, list); + struct input_dev *input_dev = hidinput->input; + + ms->hdev = hdev; + INIT_WORK(&ms->ff_worker, ms_ff_worker); + + ms->output_report_dmabuf = devm_kzalloc(&hdev->dev, + sizeof(struct xb1s_ff_report), + GFP_KERNEL); + if (ms->output_report_dmabuf == NULL) + return -ENOMEM; + + input_set_capability(input_dev, EV_FF, FF_RUMBLE); + return input_ff_create_memless(input_dev, NULL, ms_play_effect); +} + +#else +static int ms_init_ff(struct hid_device *hdev, struct ms_data *ms) +{ + return 0; +} +#endif + static int ms_probe(struct hid_device *hdev, const struct hid_device_id *id) { unsigned long quirks = id->driver_data; + struct ms_data *ms; int ret; - hid_set_drvdata(hdev, (void *)quirks); + ms = devm_kzalloc(&hdev->dev, sizeof(*ms), GFP_KERNEL); + if (ms == NULL) + return -ENOMEM; + + ms->quirks = quirks; + + hid_set_drvdata(hdev, ms); if (quirks & MS_NOGET) hdev->quirks |= HID_QUIRK_NOGET; @@ -242,11 +348,32 @@ static int ms_probe(struct hid_device *hdev, const struct hid_device_id *id) goto err_free; } + if (IS_ENABLED(CONFIG_MICROSOFT_FF) && + hdev->product == USB_DEVICE_ID_MS_XBOX_ONE_S_CONTROLLER) { + ret = ms_init_ff(hdev, ms); + if (ret) { + hid_err(hdev, + "could not initialize ff, continuing anyway"); + } + } + return 0; err_free: return ret; } +static void ms_remove(struct hid_device *hdev) +{ + hid_hw_stop(hdev); + + if (IS_ENABLED(CONFIG_MICROSOFT_FF) && + hdev->product == USB_DEVICE_ID_MS_XBOX_ONE_S_CONTROLLER) { + struct ms_data *ms = hid_get_drvdata(hdev); + + cancel_work_sync(&ms->ff_worker); + } +} + static const struct hid_device_id ms_devices[] = { { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_SIDEWINDER_GV), .driver_data = MS_HIDINPUT }, @@ -281,6 +408,10 @@ static const struct hid_device_id ms_devices[] = { { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_PRESENTER_8K_BT), .driver_data = MS_PRESENTER }, +#ifdef CONFIG_MICROSOFT_FF + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_XBOX_ONE_S_CONTROLLER), + .driver_data = 0 }, +#endif { } }; MODULE_DEVICE_TABLE(hid, ms_devices); @@ -293,6 +424,7 @@ static struct hid_driver ms_driver = { .input_mapped = ms_input_mapped, .event = ms_event, .probe = ms_probe, + .remove = ms_remove, }; module_hid_driver(ms_driver); diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c index 249d49b6b16c..10fefd0ed43c 100644 --- a/drivers/hid/hid-quirks.c +++ b/drivers/hid/hid-quirks.c @@ -496,6 +496,7 @@ static const struct hid_device_id hid_have_special_driver[] = { { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_DIGITAL_MEDIA_3KV1) }, { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_POWER_COVER) }, { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_PRESENTER_8K_BT) }, + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_XBOX_ONE_S_CONTROLLER) }, #endif #if IS_ENABLED(CONFIG_HID_MONTEREY) { HID_USB_DEVICE(USB_VENDOR_ID_MONTEREY, USB_DEVICE_ID_GENIUS_KB29E) }, -- 2.17.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] hid: microsoft: Add rumble support for Xbox One S controller 2018-08-10 0:17 [PATCH] hid: microsoft: Add rumble support for Xbox One S controller Andrey Smirnov @ 2018-08-10 9:36 ` Benjamin Tissoires 2018-08-13 14:25 ` Andrey Smirnov 2018-08-10 11:38 ` Bastien Nocera 2018-09-26 12:51 ` Dollinger Florian 2 siblings, 1 reply; 14+ messages in thread From: Benjamin Tissoires @ 2018-08-10 9:36 UTC (permalink / raw) To: andrew.smirnov Cc: Jiri Kosina, Dmitry Torokhov, open list:HID CORE LAYER, lkml, Pierre-Loup A. Griffais, juha.kuikka Hi Andrew, On Fri, Aug 10, 2018 at 2:17 AM Andrey Smirnov <andrew.smirnov@gmail.com> wrote: > > Add HID quirk driver for Xbox One S controller over bluetooth. > > This driver only adds support for rumble. Standard controller > functionality is exposed by default HID driver. > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > Cc: Jiri Kosina <jikos@kernel.org> > Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com> > Cc: linux-input@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: Pierre-Loup A. Griffais <pgriffais@valvesoftware.com> > Signed-off-by: Juha Kuikka <juha.kuikka@gmail.com> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > --- > drivers/hid/Kconfig | 8 ++ > drivers/hid/hid-ids.h | 1 + > drivers/hid/hid-microsoft.c | 142 ++++++++++++++++++++++++++++++++++-- > drivers/hid/hid-quirks.c | 1 + > 4 files changed, 147 insertions(+), 5 deletions(-) > > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig > index a49a10437c40..b2e983cb32f0 100644 > --- a/drivers/hid/Kconfig > +++ b/drivers/hid/Kconfig > @@ -589,6 +589,14 @@ config HID_MICROSOFT > ---help--- > Support for Microsoft devices that are not fully compliant with HID standard. > > +config MICROSOFT_FF > + bool "Microsoft Xbox One S controller force feedback support" > + depends on HID_MICROSOFT > + select INPUT_FF_MEMLESS > + ---help--- > + Say Y here if you have a Microsoft Xbox One S controller and want to enable > + force feedback support for it. > + Is there really a need to have a kernel that doesn't enable FF on those particular devices? It adds a lot of #ifdefs in the code and I would think users want it enabled by default. > config HID_MONTEREY > tristate "Monterey Genius KB29E keyboard" > depends on HID > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h > index c7981ddd8776..c8b4bc7fe053 100644 > --- a/drivers/hid/hid-ids.h > +++ b/drivers/hid/hid-ids.h > @@ -798,6 +798,7 @@ > #define USB_DEVICE_ID_MS_TOUCH_COVER_2 0x07a7 > #define USB_DEVICE_ID_MS_TYPE_COVER_2 0x07a9 > #define USB_DEVICE_ID_MS_POWER_COVER 0x07da > +#define USB_DEVICE_ID_MS_XBOX_ONE_S_CONTROLLER 0x02fd > > #define USB_VENDOR_ID_MOJO 0x8282 > #define USB_DEVICE_ID_RETRO_ADAPTER 0x3201 > diff --git a/drivers/hid/hid-microsoft.c b/drivers/hid/hid-microsoft.c > index 96e7d3231d2f..e2bd31d54e62 100644 > --- a/drivers/hid/hid-microsoft.c > +++ b/drivers/hid/hid-microsoft.c > @@ -29,10 +29,29 @@ > #define MS_NOGET 0x10 > #define MS_DUPLICATE_USAGES 0x20 > > +struct ms_data { > + unsigned long quirks; > + struct hid_device *hdev; > + struct work_struct ff_worker; > + __u8 strong; > + __u8 weak; > + __u8 *output_report_dmabuf; > +}; > + > +struct xb1s_ff_report { > + __u8 report_id; > + __u8 enable; > + __u8 magnitude[4]; > + __u8 duration_10ms; > + __u8 start_delay_10ms; > + __u8 loop_count; > +}; > + > static __u8 *ms_report_fixup(struct hid_device *hdev, __u8 *rdesc, > unsigned int *rsize) > { > - unsigned long quirks = (unsigned long)hid_get_drvdata(hdev); > + struct ms_data *ms = hid_get_drvdata(hdev); > + unsigned long quirks = ms->quirks; > > /* > * Microsoft Wireless Desktop Receiver (Model 1028) has > @@ -134,7 +153,8 @@ 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) > { > - unsigned long quirks = (unsigned long)hid_get_drvdata(hdev); > + struct ms_data *ms = hid_get_drvdata(hdev); > + unsigned long quirks = ms->quirks; Could you split the change of the quirks mechanism in its own patch. This will make the whole code easier to review and spot errors if there are. > > if (quirks & MS_ERGONOMY) { > int ret = ms_ergonomy_kb_quirk(hi, usage, bit, max); > @@ -153,7 +173,8 @@ static int ms_input_mapped(struct hid_device *hdev, struct hid_input *hi, > struct hid_field *field, struct hid_usage *usage, > unsigned long **bit, int *max) > { > - unsigned long quirks = (unsigned long)hid_get_drvdata(hdev); > + struct ms_data *ms = hid_get_drvdata(hdev); > + unsigned long quirks = ms->quirks; > > if (quirks & MS_DUPLICATE_USAGES) > clear_bit(usage->code, *bit); > @@ -164,7 +185,8 @@ static int ms_input_mapped(struct hid_device *hdev, struct hid_input *hi, > static int ms_event(struct hid_device *hdev, struct hid_field *field, > struct hid_usage *usage, __s32 value) > { > - unsigned long quirks = (unsigned long)hid_get_drvdata(hdev); > + struct ms_data *ms = hid_get_drvdata(hdev); > + unsigned long quirks = ms->quirks; > struct input_dev *input; > > if (!(hdev->claimed & HID_CLAIMED_INPUT) || !field->hidinput || > @@ -219,12 +241,96 @@ static int ms_event(struct hid_device *hdev, struct hid_field *field, > return 0; > } > > +#ifdef CONFIG_MICROSOFT_FF > + > +#define XB1S_FF_REPORT 3 > +#define ENABLE_WEAK BIT(0) > +#define ENABLE_STRONG BIT(1) > +#define MAGNITUDE_WEAK 3 > +#define MAGNITUDE_STRONG 2 > + > +static void ms_ff_worker(struct work_struct *work) > +{ > + struct ms_data *ms = container_of(work, struct ms_data, ff_worker); > + struct hid_device *hdev = ms->hdev; > + struct xb1s_ff_report *r = > + (struct xb1s_ff_report *) ms->output_report_dmabuf; > + int ret; > + > + memset(r, 0, sizeof(*r)); > + > + r->report_id = XB1S_FF_REPORT; > + r->enable = ENABLE_WEAK | ENABLE_STRONG; > + /* > + * Specifying maximum duration and maximum loop count should > + * cover maximum duration of a single effect: 65536 ms > + */ > + r->duration_10ms = U8_MAX; > + r->loop_count = U8_MAX; > + r->magnitude[MAGNITUDE_STRONG] = ms->strong; /* left actuator */ > + r->magnitude[MAGNITUDE_WEAK] = ms->weak; /* right actuator */ > + > + ret = hid_hw_output_report(hdev, (__u8 *)r, sizeof(*r)); > + if (ret) > + hid_warn(hdev, "failed to send FF report\n"); > +} > + > +static int ms_play_effect(struct input_dev *dev, void *data, > + struct ff_effect *effect) > +{ > + struct hid_device *hid = input_get_drvdata(dev); > + struct ms_data *ms = hid_get_drvdata(hid); > + > + if (effect->type != FF_RUMBLE) > + return 0; > + > + /* Magnitude is 0..100 so scale the 16-bit input here */ > + ms->strong = ((u32) effect->u.rumble.strong_magnitude * 100) / U16_MAX; > + ms->weak = ((u32) effect->u.rumble.weak_magnitude * 100) / U16_MAX; > + > + schedule_work(&ms->ff_worker); > + return 0; > +} > + > +static int ms_init_ff(struct hid_device *hdev, struct ms_data *ms) > +{ > + struct hid_input *hidinput = list_entry(hdev->inputs.next, > + struct hid_input, list); > + struct input_dev *input_dev = hidinput->input; > + > + ms->hdev = hdev; > + INIT_WORK(&ms->ff_worker, ms_ff_worker); > + > + ms->output_report_dmabuf = devm_kzalloc(&hdev->dev, > + sizeof(struct xb1s_ff_report), > + GFP_KERNEL); > + if (ms->output_report_dmabuf == NULL) > + return -ENOMEM; > + > + input_set_capability(input_dev, EV_FF, FF_RUMBLE); > + return input_ff_create_memless(input_dev, NULL, ms_play_effect); > +} > + > +#else > +static int ms_init_ff(struct hid_device *hdev, struct ms_data *ms) > +{ > + return 0; > +} > +#endif > + > static int ms_probe(struct hid_device *hdev, const struct hid_device_id *id) > { > unsigned long quirks = id->driver_data; > + struct ms_data *ms; > int ret; > > - hid_set_drvdata(hdev, (void *)quirks); > + ms = devm_kzalloc(&hdev->dev, sizeof(*ms), GFP_KERNEL); > + if (ms == NULL) > + return -ENOMEM; > + > + ms->quirks = quirks; > + > + hid_set_drvdata(hdev, ms); > > if (quirks & MS_NOGET) > hdev->quirks |= HID_QUIRK_NOGET; > @@ -242,11 +348,32 @@ static int ms_probe(struct hid_device *hdev, const struct hid_device_id *id) > goto err_free; > } > > + if (IS_ENABLED(CONFIG_MICROSOFT_FF) && > + hdev->product == USB_DEVICE_ID_MS_XBOX_ONE_S_CONTROLLER) { nitpick: I'd rather have this test (against the PID) in ms_init_ff. In the long run, we might want to add more devices and it would be better to have the test where it should be. An even better solution would be to add a new MS_QUIRK_FF and test this here (or in ms_init_ff()). > + ret = ms_init_ff(hdev, ms); > + if (ret) { > + hid_err(hdev, > + "could not initialize ff, continuing anyway"); > + } > + } > + > return 0; > err_free: > return ret; > } > > +static void ms_remove(struct hid_device *hdev) > +{ > + hid_hw_stop(hdev); > + > + if (IS_ENABLED(CONFIG_MICROSOFT_FF) && > + hdev->product == USB_DEVICE_ID_MS_XBOX_ONE_S_CONTROLLER) { A quirk would be better (see my previous comment) > + struct ms_data *ms = hid_get_drvdata(hdev); > + > + cancel_work_sync(&ms->ff_worker); > + } > +} > + > static const struct hid_device_id ms_devices[] = { > { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_SIDEWINDER_GV), > .driver_data = MS_HIDINPUT }, > @@ -281,6 +408,10 @@ static const struct hid_device_id ms_devices[] = { > > { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_PRESENTER_8K_BT), > .driver_data = MS_PRESENTER }, > +#ifdef CONFIG_MICROSOFT_FF > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_XBOX_ONE_S_CONTROLLER), > + .driver_data = 0 }, > +#endif That seems fragile. Also even if we keep the Kconfig option, is there any point at conditioning the handling of this device from hid-microsoft? If it works with or without the Kconfig, I'd just drop the #ifdef. > { } > }; > MODULE_DEVICE_TABLE(hid, ms_devices); > @@ -293,6 +424,7 @@ static struct hid_driver ms_driver = { > .input_mapped = ms_input_mapped, > .event = ms_event, > .probe = ms_probe, > + .remove = ms_remove, > }; > module_hid_driver(ms_driver); > > diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c > index 249d49b6b16c..10fefd0ed43c 100644 > --- a/drivers/hid/hid-quirks.c > +++ b/drivers/hid/hid-quirks.c > @@ -496,6 +496,7 @@ static const struct hid_device_id hid_have_special_driver[] = { > { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_DIGITAL_MEDIA_3KV1) }, > { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_POWER_COVER) }, > { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_PRESENTER_8K_BT) }, > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_XBOX_ONE_S_CONTROLLER) }, You don't need to add your device to hid_have_special_driver since v4.15 or v4.16. So please drop this :) Cheers, Benjamin > #endif > #if IS_ENABLED(CONFIG_HID_MONTEREY) > { HID_USB_DEVICE(USB_VENDOR_ID_MONTEREY, USB_DEVICE_ID_GENIUS_KB29E) }, > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] hid: microsoft: Add rumble support for Xbox One S controller 2018-08-10 9:36 ` Benjamin Tissoires @ 2018-08-13 14:25 ` Andrey Smirnov 0 siblings, 0 replies; 14+ messages in thread From: Andrey Smirnov @ 2018-08-13 14:25 UTC (permalink / raw) To: Benjamin Tissoires Cc: Jiri Kosina, Dmitry Torokhov, linux-input, linux-kernel, Pierre-Loup A. Griffais, Juha Kuikka On Fri, Aug 10, 2018 at 2:37 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > Hi Andrew, > > On Fri, Aug 10, 2018 at 2:17 AM Andrey Smirnov <andrew.smirnov@gmail.com> wrote: > > > > Add HID quirk driver for Xbox One S controller over bluetooth. > > > > This driver only adds support for rumble. Standard controller > > functionality is exposed by default HID driver. > > > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > Cc: Jiri Kosina <jikos@kernel.org> > > Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > Cc: linux-input@vger.kernel.org > > Cc: linux-kernel@vger.kernel.org > > Cc: Pierre-Loup A. Griffais <pgriffais@valvesoftware.com> > > Signed-off-by: Juha Kuikka <juha.kuikka@gmail.com> > > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > > --- > > drivers/hid/Kconfig | 8 ++ > > drivers/hid/hid-ids.h | 1 + > > drivers/hid/hid-microsoft.c | 142 ++++++++++++++++++++++++++++++++++-- > > drivers/hid/hid-quirks.c | 1 + > > 4 files changed, 147 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig > > index a49a10437c40..b2e983cb32f0 100644 > > --- a/drivers/hid/Kconfig > > +++ b/drivers/hid/Kconfig > > @@ -589,6 +589,14 @@ config HID_MICROSOFT > > ---help--- > > Support for Microsoft devices that are not fully compliant with HID standard. > > > > +config MICROSOFT_FF > > + bool "Microsoft Xbox One S controller force feedback support" > > + depends on HID_MICROSOFT > > + select INPUT_FF_MEMLESS > > + ---help--- > > + Say Y here if you have a Microsoft Xbox One S controller and want to enable > > + force feedback support for it. > > + > > Is there really a need to have a kernel that doesn't enable FF on > those particular devices? > It adds a lot of #ifdefs in the code and I would think users want it > enabled by default. > No, I don't think there is. This is just something that similar Sony PS4 driver does, but I am more than happy to drop this option. Will do in v2. > > config HID_MONTEREY > > tristate "Monterey Genius KB29E keyboard" > > depends on HID > > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h > > index c7981ddd8776..c8b4bc7fe053 100644 > > --- a/drivers/hid/hid-ids.h > > +++ b/drivers/hid/hid-ids.h > > @@ -798,6 +798,7 @@ > > #define USB_DEVICE_ID_MS_TOUCH_COVER_2 0x07a7 > > #define USB_DEVICE_ID_MS_TYPE_COVER_2 0x07a9 > > #define USB_DEVICE_ID_MS_POWER_COVER 0x07da > > +#define USB_DEVICE_ID_MS_XBOX_ONE_S_CONTROLLER 0x02fd > > > > #define USB_VENDOR_ID_MOJO 0x8282 > > #define USB_DEVICE_ID_RETRO_ADAPTER 0x3201 > > diff --git a/drivers/hid/hid-microsoft.c b/drivers/hid/hid-microsoft.c > > index 96e7d3231d2f..e2bd31d54e62 100644 > > --- a/drivers/hid/hid-microsoft.c > > +++ b/drivers/hid/hid-microsoft.c > > @@ -29,10 +29,29 @@ > > #define MS_NOGET 0x10 > > #define MS_DUPLICATE_USAGES 0x20 > > > > +struct ms_data { > > + unsigned long quirks; > > + struct hid_device *hdev; > > + struct work_struct ff_worker; > > + __u8 strong; > > + __u8 weak; > > + __u8 *output_report_dmabuf; > > +}; > > + > > +struct xb1s_ff_report { > > + __u8 report_id; > > + __u8 enable; > > + __u8 magnitude[4]; > > + __u8 duration_10ms; > > + __u8 start_delay_10ms; > > + __u8 loop_count; > > +}; > > + > > static __u8 *ms_report_fixup(struct hid_device *hdev, __u8 *rdesc, > > unsigned int *rsize) > > { > > - unsigned long quirks = (unsigned long)hid_get_drvdata(hdev); > > + struct ms_data *ms = hid_get_drvdata(hdev); > > + unsigned long quirks = ms->quirks; > > > > /* > > * Microsoft Wireless Desktop Receiver (Model 1028) has > > @@ -134,7 +153,8 @@ 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) > > { > > - unsigned long quirks = (unsigned long)hid_get_drvdata(hdev); > > + struct ms_data *ms = hid_get_drvdata(hdev); > > + unsigned long quirks = ms->quirks; > > Could you split the change of the quirks mechanism in its own patch. > This will make the whole code easier to review and spot errors if > there are. > Sure, sounds like a good idea, will do in v2. > > > > if (quirks & MS_ERGONOMY) { > > int ret = ms_ergonomy_kb_quirk(hi, usage, bit, max); > > @@ -153,7 +173,8 @@ static int ms_input_mapped(struct hid_device *hdev, struct hid_input *hi, > > struct hid_field *field, struct hid_usage *usage, > > unsigned long **bit, int *max) > > { > > - unsigned long quirks = (unsigned long)hid_get_drvdata(hdev); > > + struct ms_data *ms = hid_get_drvdata(hdev); > > + unsigned long quirks = ms->quirks; > > > > if (quirks & MS_DUPLICATE_USAGES) > > clear_bit(usage->code, *bit); > > @@ -164,7 +185,8 @@ static int ms_input_mapped(struct hid_device *hdev, struct hid_input *hi, > > static int ms_event(struct hid_device *hdev, struct hid_field *field, > > struct hid_usage *usage, __s32 value) > > { > > - unsigned long quirks = (unsigned long)hid_get_drvdata(hdev); > > + struct ms_data *ms = hid_get_drvdata(hdev); > > + unsigned long quirks = ms->quirks; > > struct input_dev *input; > > > > if (!(hdev->claimed & HID_CLAIMED_INPUT) || !field->hidinput || > > @@ -219,12 +241,96 @@ static int ms_event(struct hid_device *hdev, struct hid_field *field, > > return 0; > > } > > > > +#ifdef CONFIG_MICROSOFT_FF > > + > > +#define XB1S_FF_REPORT 3 > > +#define ENABLE_WEAK BIT(0) > > +#define ENABLE_STRONG BIT(1) > > +#define MAGNITUDE_WEAK 3 > > +#define MAGNITUDE_STRONG 2 > > + > > +static void ms_ff_worker(struct work_struct *work) > > +{ > > + struct ms_data *ms = container_of(work, struct ms_data, ff_worker); > > + struct hid_device *hdev = ms->hdev; > > + struct xb1s_ff_report *r = > > + (struct xb1s_ff_report *) ms->output_report_dmabuf; > > + int ret; > > + > > + memset(r, 0, sizeof(*r)); > > + > > + r->report_id = XB1S_FF_REPORT; > > + r->enable = ENABLE_WEAK | ENABLE_STRONG; > > + /* > > + * Specifying maximum duration and maximum loop count should > > + * cover maximum duration of a single effect: 65536 ms > > + */ > > + r->duration_10ms = U8_MAX; > > + r->loop_count = U8_MAX; > > + r->magnitude[MAGNITUDE_STRONG] = ms->strong; /* left actuator */ > > + r->magnitude[MAGNITUDE_WEAK] = ms->weak; /* right actuator */ > > + > > + ret = hid_hw_output_report(hdev, (__u8 *)r, sizeof(*r)); > > + if (ret) > > + hid_warn(hdev, "failed to send FF report\n"); > > +} > > + > > +static int ms_play_effect(struct input_dev *dev, void *data, > > + struct ff_effect *effect) > > +{ > > + struct hid_device *hid = input_get_drvdata(dev); > > + struct ms_data *ms = hid_get_drvdata(hid); > > + > > + if (effect->type != FF_RUMBLE) > > + return 0; > > + > > + /* Magnitude is 0..100 so scale the 16-bit input here */ > > + ms->strong = ((u32) effect->u.rumble.strong_magnitude * 100) / U16_MAX; > > + ms->weak = ((u32) effect->u.rumble.weak_magnitude * 100) / U16_MAX; > > + > > + schedule_work(&ms->ff_worker); > > + return 0; > > +} > > + > > +static int ms_init_ff(struct hid_device *hdev, struct ms_data *ms) > > +{ > > + struct hid_input *hidinput = list_entry(hdev->inputs.next, > > + struct hid_input, list); > > + struct input_dev *input_dev = hidinput->input; > > + > > + ms->hdev = hdev; > > + INIT_WORK(&ms->ff_worker, ms_ff_worker); > > + > > + ms->output_report_dmabuf = devm_kzalloc(&hdev->dev, > > + sizeof(struct xb1s_ff_report), > > + GFP_KERNEL); > > + if (ms->output_report_dmabuf == NULL) > > + return -ENOMEM; > > + > > + input_set_capability(input_dev, EV_FF, FF_RUMBLE); > > + return input_ff_create_memless(input_dev, NULL, ms_play_effect); > > +} > > + > > +#else > > +static int ms_init_ff(struct hid_device *hdev, struct ms_data *ms) > > +{ > > + return 0; > > +} > > +#endif > > + > > static int ms_probe(struct hid_device *hdev, const struct hid_device_id *id) > > { > > unsigned long quirks = id->driver_data; > > + struct ms_data *ms; > > int ret; > > > > - hid_set_drvdata(hdev, (void *)quirks); > > + ms = devm_kzalloc(&hdev->dev, sizeof(*ms), GFP_KERNEL); > > + if (ms == NULL) > > + return -ENOMEM; > > + > > + ms->quirks = quirks; > > + > > + hid_set_drvdata(hdev, ms); > > > > if (quirks & MS_NOGET) > > hdev->quirks |= HID_QUIRK_NOGET; > > @@ -242,11 +348,32 @@ static int ms_probe(struct hid_device *hdev, const struct hid_device_id *id) > > goto err_free; > > } > > > > + if (IS_ENABLED(CONFIG_MICROSOFT_FF) && > > + hdev->product == USB_DEVICE_ID_MS_XBOX_ONE_S_CONTROLLER) { > > nitpick: > I'd rather have this test (against the PID) in ms_init_ff. In the long > run, we might want to add more devices and it would be better to have > the test where it should be. > An even better solution would be to add a new MS_QUIRK_FF and test > this here (or in ms_init_ff()). OK, sounds good. Will do both in v2. > > > + ret = ms_init_ff(hdev, ms); > > + if (ret) { > > + hid_err(hdev, > > + "could not initialize ff, continuing anyway"); > > + } > > + } > > + > > return 0; > > err_free: > > return ret; > > } > > > > +static void ms_remove(struct hid_device *hdev) > > +{ > > + hid_hw_stop(hdev); > > + > > + if (IS_ENABLED(CONFIG_MICROSOFT_FF) && > > + hdev->product == USB_DEVICE_ID_MS_XBOX_ONE_S_CONTROLLER) { > > A quirk would be better (see my previous comment) > Ditto. > > + struct ms_data *ms = hid_get_drvdata(hdev); > > + > > + cancel_work_sync(&ms->ff_worker); > > + } > > +} > > + > > static const struct hid_device_id ms_devices[] = { > > { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_SIDEWINDER_GV), > > .driver_data = MS_HIDINPUT }, > > @@ -281,6 +408,10 @@ static const struct hid_device_id ms_devices[] = { > > > > { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_PRESENTER_8K_BT), > > .driver_data = MS_PRESENTER }, > > +#ifdef CONFIG_MICROSOFT_FF > > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_XBOX_ONE_S_CONTROLLER), > > + .driver_data = 0 }, > > +#endif > > That seems fragile. Also even if we keep the Kconfig option, is there > any point at conditioning the handling of this device from > hid-microsoft? > If it works with or without the Kconfig, I'd just drop the #ifdef. > Will do in v2. > > { } > > }; > > MODULE_DEVICE_TABLE(hid, ms_devices); > > @@ -293,6 +424,7 @@ static struct hid_driver ms_driver = { > > .input_mapped = ms_input_mapped, > > .event = ms_event, > > .probe = ms_probe, > > + .remove = ms_remove, > > }; > > module_hid_driver(ms_driver); > > > > diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c > > index 249d49b6b16c..10fefd0ed43c 100644 > > --- a/drivers/hid/hid-quirks.c > > +++ b/drivers/hid/hid-quirks.c > > @@ -496,6 +496,7 @@ static const struct hid_device_id hid_have_special_driver[] = { > > { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_DIGITAL_MEDIA_3KV1) }, > > { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_POWER_COVER) }, > > { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_PRESENTER_8K_BT) }, > > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_XBOX_ONE_S_CONTROLLER) }, > > You don't need to add your device to hid_have_special_driver since > v4.15 or v4.16. So please drop this :) Good to know! Will do in v2. Thanks, Andrey Smirnov ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] hid: microsoft: Add rumble support for Xbox One S controller 2018-08-10 0:17 [PATCH] hid: microsoft: Add rumble support for Xbox One S controller Andrey Smirnov 2018-08-10 9:36 ` Benjamin Tissoires @ 2018-08-10 11:38 ` Bastien Nocera 2018-08-13 14:37 ` Andrey Smirnov 2018-09-26 12:51 ` Dollinger Florian 2 siblings, 1 reply; 14+ messages in thread From: Bastien Nocera @ 2018-08-10 11:38 UTC (permalink / raw) To: Andrey Smirnov, Jiri Kosina Cc: Dmitry Torokhov, Benjamin Tissoires, linux-input, linux-kernel, Pierre-Loup A . Griffais, Juha Kuikka On Thu, 2018-08-09 at 17:17 -0700, Andrey Smirnov wrote: > Add HID quirk driver for Xbox One S controller over bluetooth. > > This driver only adds support for rumble. Standard controller > functionality is exposed by default HID driver. Did you manage to make the joypad work without hacks in the Bluetooth stack[1]? [1]: https://github.com/ValveSoftware/steamos_kernel/commit/549c3dc10fa3749b3999549a2672b14bf0e9786d ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] hid: microsoft: Add rumble support for Xbox One S controller 2018-08-10 11:38 ` Bastien Nocera @ 2018-08-13 14:37 ` Andrey Smirnov 2018-08-13 14:52 ` Bastien Nocera 0 siblings, 1 reply; 14+ messages in thread From: Andrey Smirnov @ 2018-08-13 14:37 UTC (permalink / raw) To: hadess Cc: Jiri Kosina, Dmitry Torokhov, Benjamin Tissoires, linux-input, linux-kernel, Pierre-Loup A. Griffais, Juha Kuikka On Fri, Aug 10, 2018 at 4:38 AM Bastien Nocera <hadess@hadess.net> wrote: > > On Thu, 2018-08-09 at 17:17 -0700, Andrey Smirnov wrote: > > Add HID quirk driver for Xbox One S controller over bluetooth. > > > > This driver only adds support for rumble. Standard controller > > functionality is exposed by default HID driver. > > Did you manage to make the joypad work without hacks in the Bluetooth > stack[1]? I was not aware this hack actually existed, but now that I see it I think it explains why doing echo 1 > /sys/module/bluetooth/parameters/disable_ertm was necessary to make the controller connect over Bluetooth on my machine. It looks like disabling ERTM just happens to have the same effect as the hack that you mention. I'd like/plan to look into finding a proper solution to replace that hack, but that'd probably be a separate patch. Thanks, Andrey Smirnov ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] hid: microsoft: Add rumble support for Xbox One S controller 2018-08-13 14:37 ` Andrey Smirnov @ 2018-08-13 14:52 ` Bastien Nocera 0 siblings, 0 replies; 14+ messages in thread From: Bastien Nocera @ 2018-08-13 14:52 UTC (permalink / raw) To: Andrey Smirnov Cc: Jiri Kosina, Dmitry Torokhov, Benjamin Tissoires, linux-input, linux-kernel, Pierre-Loup A. Griffais, Juha Kuikka On Mon, 2018-08-13 at 07:37 -0700, Andrey Smirnov wrote: > On Fri, Aug 10, 2018 at 4:38 AM Bastien Nocera <hadess@hadess.net> > wrote: > > > > On Thu, 2018-08-09 at 17:17 -0700, Andrey Smirnov wrote: > > > Add HID quirk driver for Xbox One S controller over bluetooth. > > > > > > This driver only adds support for rumble. Standard controller > > > functionality is exposed by default HID driver. > > > > Did you manage to make the joypad work without hacks in the > > Bluetooth > > stack[1]? > > I was not aware this hack actually existed, but now that I see it I > think it explains why doing > > echo 1 > /sys/module/bluetooth/parameters/disable_ertm > > was necessary to make the controller connect over Bluetooth on my > machine. It looks like disabling ERTM just happens to have the same > effect as the hack that you mention. I'd like/plan to look into > finding a proper solution to replace that hack, but that'd probably > be > a separate patch. I gave it a try a couple of months back, but without much success. L2CAP is unfortunately a bit too low-level to know anything about the device which requested this. I think that some level of "STREAMING" support is needed for the Bluetooth audio portion of the device. Marcel was also interested in looking into the problem, but didn't get to it. Let me know if you manage to get anything working without the big hammer of disabling ERTM support for everything. Cheers ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: hid: microsoft: Add rumble support for Xbox One S controller 2018-08-10 0:17 [PATCH] hid: microsoft: Add rumble support for Xbox One S controller Andrey Smirnov 2018-08-10 9:36 ` Benjamin Tissoires 2018-08-10 11:38 ` Bastien Nocera @ 2018-09-26 12:51 ` Dollinger Florian 2018-09-26 13:45 ` Bastien Nocera 2 siblings, 1 reply; 14+ messages in thread From: Dollinger Florian @ 2018-09-26 12:51 UTC (permalink / raw) To: andrew.smirnov Cc: linux-kernel, linux-input-owner, linux-input, Florian Dollinger From: Florian Dollinger <dollinger.florian@gmx.de> Hi there! Why do you re-engineer the wheel? :) There is already a fully functional and tested driver out there (https://github.com/atar-axis/xpadneo). Would be much easier to help me (the owner of xpadneo) to push it into the kernel. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: hid: microsoft: Add rumble support for Xbox One S controller 2018-09-26 12:51 ` Dollinger Florian @ 2018-09-26 13:45 ` Bastien Nocera 2018-09-26 15:15 ` AW: " dollinger.florian ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: Bastien Nocera @ 2018-09-26 13:45 UTC (permalink / raw) To: Dollinger Florian, andrew.smirnov Cc: linux-kernel, linux-input-owner, linux-input Hey Florian, On Wed, 2018-09-26 at 14:51 +0200, Dollinger Florian wrote: > From: Florian Dollinger <dollinger.florian@gmx.de> > > Hi there! Why do you re-engineer the wheel? :) There is already a > fully functional and tested driver out there ( > https://github.com/atar-axis/xpadneo). Would be much easier to help > me (the owner of xpadneo) to push it into the kernel. Probably because he didn't know about it, and how would he? I also didn't know about it, because it didn't exist last I worked on those joypads. I spent quite a bit of time trying to get the XBox One S controller working over Bluetooth, without success, and I see that you have a patch for that which you didn't send upstream either: https://github.com/atar-axis/xpadneo/blob/master/misc/kernel_patches/0001-fix_bluetooth_reconnect.patch I can imagine that a large portion of the driver can be integrated in the existing XBox pad driver, with each feature added in individual patches. If I get the time, there are good chances I will send a patch to integrate the battery reporting in the existing driver at least, and then add support for missing buttons if there's a problem there (I see that mentioned in the README). "Trigger Force Feedback" is likely something that would need to be integrated at a lower level, this is probably not something we'd want to have replicated in each driver. Cheers ^ permalink raw reply [flat|nested] 14+ messages in thread
* AW: hid: microsoft: Add rumble support for Xbox One S controller 2018-09-26 13:45 ` Bastien Nocera @ 2018-09-26 15:15 ` dollinger.florian 2018-09-26 18:02 ` Bastien Nocera 2018-09-26 16:09 ` XBox One S controller, Bluetooth support (was Re: hid: microsoft: Add rumble support for Xbox One S controller) Bastien Nocera 2018-09-26 17:43 ` Bastien Nocera 2 siblings, 1 reply; 14+ messages in thread From: dollinger.florian @ 2018-09-26 15:15 UTC (permalink / raw) To: 'Bastien Nocera', andrew.smirnov Cc: linux-kernel, linux-input-owner, linux-input Hey Bastien, > Probably because he didn't know about it, and how would he? Hum, it's the first hit when you search for something like 'xbox one s driver linux' since half an year or longer 😊 > I spent quite a bit of time trying to get the XBox One S controller working over Bluetooth, without success, and I see that you have a patch for that which you didn't send upstream either: https://github.com/atar-axis/xpadneo/blob/master/misc/kernel_patches/0001-fix_bluetooth_reconnect.patch Jap, I just never pushed it because I never pushed anything to the linux kernel and I don't really know how the whole process works^^ Furthermore I wanted to fix possible before, but it looks like the driver is stable enough to push it now. > I can imagine that a large portion of the driver can be integrated in the existing XBox pad driver, with each feature added in individual patches. Would be great! But ist the xpad driver really the right place? First of all, the BT interface is using HID, USB does not. Furthermore, the driver is currently around 1,3k lines long - for a single device and only BT... (okay yes, there are a lot of comments in it). But anyway, is it really a good idea to put everything into hid-microsoft? Moreover, there is another interface which I am trying to support at the moment - the one which is used by the XBOX (WiFi). I think in the end the whole hid-microsoft thingy will get blown-up a bit - right? Or, everything is scattered over many places (hid-microsoft, xpad, another driver fort he wifi-interface). Isn't it possible to use "one driver per device" in the kernel too, like I did? > If I get the time, there are good chances I will send a patch to integrate the battery reporting in the existing driver at least, and then add support for missing buttons if there's a problem there (I see that mentioned in the README). Jap, the wrong mapping is possibly the biggest problem of all. Battery support and FF is more "nice to have" but not really crucial. > "Trigger Force Feedback" is likely something that would need to be integrated at a lower level, this is probably not something we'd want to have replicated in each driver. True. Best regards, Flo ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: AW: hid: microsoft: Add rumble support for Xbox One S controller 2018-09-26 15:15 ` AW: " dollinger.florian @ 2018-09-26 18:02 ` Bastien Nocera 2018-09-26 22:50 ` Florian Dollinger 0 siblings, 1 reply; 14+ messages in thread From: Bastien Nocera @ 2018-09-26 18:02 UTC (permalink / raw) To: dollinger.florian, andrew.smirnov Cc: linux-kernel, linux-bluetooth, linux-input On Wed, 2018-09-26 at 17:15 +0200, dollinger.florian@gmx.de wrote: > Hey Bastien, > > > Probably because he didn't know about it, and how would he? > > Hum, it's the first hit when you search for something like 'xbox one > s driver linux' since half an year or longer 😊 Sure it is, but: - it's not an "XBox One S controller", it's a variant of the "XBox One controller" shipped with the XBox One S. - we discussed this on the linux-bluetooth mailing-list in August 2016: https://www.spinics.net/lists/linux-bluetooth/msg68102.html revived the thread in 2017: https://www.spinics.net/lists/linux-bluetooth/msg72750.html and discussed the force feedback in particular in August this year: https://www.spinics.net/lists/linux-input/msg57744.html - why would we look for a "driver" when there's already one in the kernel that supports all the xbox controllers except this one? :) Really, as much as it's nice to find working code for this device, it's surprising you didn't contact any kernel developer before, rather than us having to find you. > > I can imagine that a large portion of the driver can be integrated > > in the existing XBox pad driver, with each feature added in > > individual patches. > > Would be great! But ist the xpad driver really the right place? > > First of all, the BT interface is using HID, USB does not. > Furthermore, the driver is currently around 1,3k lines long - for a > single device and only BT... (okay yes, there are a lot of comments > in it). > But anyway, is it really a good idea to put everything into hid- > microsoft? > Moreover, there is another interface which I am trying to support at > the moment - the one which is used by the XBOX (WiFi). > > I think in the end the whole hid-microsoft thingy will get blown-up a > bit - right? > Or, everything is scattered over many places (hid-microsoft, xpad, > another driver fort he wifi-interface). > > Isn't it possible to use "one driver per device" in the kernel too, > like I did? Yes, but that's not the way the drivers are usually arranged. They're arranged by vendors, so this driver would need to be merged into the "hid-microsoft.c" driver. I'm pretty sure that the USB version can also be made to use HID. I have no idea how the RF protocol would work though. I imagine it requires a dongle, or does it actually use Wi-Fi? (that would be surprising...). Anyway, I don't have much time to work on it right this minute, but it would be great if you could send your Bluetooth patch to start with, and we can iterate on that, and fix the other problems as we clear the priority list. Cheers ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: hid: microsoft: Add rumble support for Xbox One S controller 2018-09-26 18:02 ` Bastien Nocera @ 2018-09-26 22:50 ` Florian Dollinger 0 siblings, 0 replies; 14+ messages in thread From: Florian Dollinger @ 2018-09-26 22:50 UTC (permalink / raw) To: Bastien Nocera, andrew.smirnov; +Cc: linux-kernel, linux-bluetooth, linux-input > Sure it is, but: > - it's not an "XBox One S controller", it's a variant of the "XBox One > controller" shipped with the XBox One S. > - we discussed this on the linux-bluetooth mailing-list in August 2016: > https://www.spinics.net/lists/linux-bluetooth/msg68102.html > revived the thread in 2017: > https://www.spinics.net/lists/linux-bluetooth/msg72750.html > and discussed the force feedback in particular in August this year: > https://www.spinics.net/lists/linux-input/msg57744.html > - why would we look for a "driver" when there's already one in the > kernel that supports all the xbox controllers except this one? :) > > Really, as much as it's nice to find working code for this device, it's > surprising you didn't contact any kernel developer before, rather than > us having to find you. Fair enough :) > Yes, but that's not the way the drivers are usually arranged. They're > arranged by vendors, so this driver would need to be merged into the > "hid-microsoft.c" driver. That's probably the main reason for not getting in touch earlier, I really don't like the idea of putting everything into the same driver just because it is the same vendor - or at leas the same name (even if Microsoft Xbox has nearly nothing to do with other parts of Microsoft). > I'm pretty sure that the USB version can also be made to use HID. I really don't think so, I already gave it a try. At least not "out of the box". > I have no idea how the RF protocol would work though. I imagine it > requires a dongle, or does it actually use Wi-Fi? (that would be > surprising...). In theory it does need one, but it is nothing more than normal Wi-Fi (2,4 and 5 GHz) - you can talk to the gamepad using any Wi-Fi card / stick you want. The problem is, that the connection is secured somehow. If you are interested, take a look here: https://github.com/atar-axis/xpadneo/wiki/Microsoft-Xbox-One-Wireless-Adapter > Anyway, I don't have much time to work on it right this minute, but it > would be great if you could send your Bluetooth patch to start with, > and we can iterate on that, and fix the other problems as we clear the > priority list. Jap, I will do that - Thank you so far! > Cheers Dito. ^ permalink raw reply [flat|nested] 14+ messages in thread
* XBox One S controller, Bluetooth support (was Re: hid: microsoft: Add rumble support for Xbox One S controller) 2018-09-26 13:45 ` Bastien Nocera 2018-09-26 15:15 ` AW: " dollinger.florian @ 2018-09-26 16:09 ` Bastien Nocera 2018-09-26 17:43 ` Bastien Nocera 2 siblings, 0 replies; 14+ messages in thread From: Bastien Nocera @ 2018-09-26 16:09 UTC (permalink / raw) To: Dollinger Florian, andrew.smirnov Cc: linux-kernel, linux-bluetooth, linux-input On Wed, 2018-09-26 at 15:45 +0200, Bastien Nocera wrote: > Hey Florian, > > On Wed, 2018-09-26 at 14:51 +0200, Dollinger Florian wrote: > > From: Florian Dollinger <dollinger.florian@gmx.de> > > > > Hi there! Why do you re-engineer the wheel? :) There is already a > > fully functional and tested driver out there ( > > https://github.com/atar-axis/xpadneo). Would be much easier to help > > me (the owner of xpadneo) to push it into the kernel. > > Probably because he didn't know about it, and how would he? I also > didn't know about it, because it didn't exist last I worked on those > joypads. > > I spent quite a bit of time trying to get the XBox One S controller > working over Bluetooth, without success, and I see that you have a > patch for that which you didn't send upstream either: > https://github.com/atar-axis/xpadneo/blob/master/misc/kernel_patches/0001-fix_bluetooth_reconnect.patch For this patch, after speaking with the Bluetooth maintainers, we would need to: - add relevant btmon outputs before and after the patch in the commit message - remove the mention of "many devices" (if you've seen other devices with that problem in the wild, please mention them, otherwise mention that it's for "XBox One S" controllers, and clones) - run the patch against the "Profile Tuning Suite Tool" from the Bluetooth SIG. This requires a Windows laptop, and a piece of hardware from the Bluetooth SIG, see below. The first 2 should be pretty easy to do, just send your patch using "git send-email" to the linux-bluetooth list (CC:ed), with the data attached. For the qualification test, I was told Szymon has a test suite that he could use to double-check the tests. If not, I should be able to buy the adapter and run that test suite locally. Cheers ^ permalink raw reply [flat|nested] 14+ messages in thread
* XBox One S controller, Bluetooth support (was Re: hid: microsoft: Add rumble support for Xbox One S controller) 2018-09-26 13:45 ` Bastien Nocera 2018-09-26 15:15 ` AW: " dollinger.florian 2018-09-26 16:09 ` XBox One S controller, Bluetooth support (was Re: hid: microsoft: Add rumble support for Xbox One S controller) Bastien Nocera @ 2018-09-26 17:43 ` Bastien Nocera 2018-09-26 22:35 ` Florian Dollinger 2 siblings, 1 reply; 14+ messages in thread From: Bastien Nocera @ 2018-09-26 17:43 UTC (permalink / raw) To: Dollinger Florian, andrew.smirnov Cc: linux-kernel, linux-bluetooth, linux-input (Resend to the correct Bluetooth list, sorry for the dupes) On Wed, 2018-09-26 at 15:45 +0200, Bastien Nocera wrote: > Hey Florian, > > On Wed, 2018-09-26 at 14:51 +0200, Dollinger Florian wrote: > > From: Florian Dollinger <dollinger.florian@gmx.de> > > > > Hi there! Why do you re-engineer the wheel? :) There is already a > > fully functional and tested driver out there ( > > https://github.com/atar-axis/xpadneo). Would be much easier to help > > me (the owner of xpadneo) to push it into the kernel. > > Probably because he didn't know about it, and how would he? I also > didn't know about it, because it didn't exist last I worked on those > joypads. > > I spent quite a bit of time trying to get the XBox One S controller > working over Bluetooth, without success, and I see that you have a > patch for that which you didn't send upstream either: > https://github.com/atar-axis/xpadneo/blob/master/misc/kernel_patches/0001-fix_bluetooth_reconnect.patch For this patch, after speaking with the Bluetooth maintainers, we would need to: - add relevant btmon outputs before and after the patch in the commit message - remove the mention of "many devices" (if you've seen other devices with that problem in the wild, please mention them, otherwise mention that it's for "XBox One S" controllers, and clones) - run the patch against the "Profile Tuning Suite Tool" from the Bluetooth SIG. This requires a Windows laptop, and a piece of hardware from the Bluetooth SIG, see below. The first 2 should be pretty easy to do, just send your patch using "git send-email" to the linux-bluetooth list (CC:ed), with the data attached. For the qualification test, I was told Szymon has a test suite that he could use to double-check the tests. If not, I should be able to buy the adapter and run that test suite locally. Cheers ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: XBox One S controller, Bluetooth support (was Re: hid: microsoft: Add rumble support for Xbox One S controller) 2018-09-26 17:43 ` Bastien Nocera @ 2018-09-26 22:35 ` Florian Dollinger 0 siblings, 0 replies; 14+ messages in thread From: Florian Dollinger @ 2018-09-26 22:35 UTC (permalink / raw) To: Bastien Nocera, andrew.smirnov; +Cc: linux-kernel, linux-bluetooth, linux-input > For this patch, after speaking with the Bluetooth maintainers, we would > need to: > - add relevant btmon outputs before and after the patch in the commit > message > - remove the mention of "many devices" (if you've seen other devices > with that problem in the wild, please mention them, otherwise mention > that it's for "XBox One S" controllers, and clones) > - run the patch against the "Profile Tuning Suite Tool" from the > Bluetooth SIG. This requires a Windows laptop, and a piece of hardware > from the Bluetooth SIG, see below. > > The first 2 should be pretty easy to do, just send your patch using > "git send-email" to the linux-bluetooth list (CC:ed), with the data > attached. > > For the qualification test, I was told Szymon has a test suite that he > could use to double-check the tests. If not, I should be able to buy > the adapter and run that test suite locally. > > Cheers Thanks, I will give it a try tomorrow (later) :) ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2018-09-26 22:50 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-10 0:17 [PATCH] hid: microsoft: Add rumble support for Xbox One S controller Andrey Smirnov 2018-08-10 9:36 ` Benjamin Tissoires 2018-08-13 14:25 ` Andrey Smirnov 2018-08-10 11:38 ` Bastien Nocera 2018-08-13 14:37 ` Andrey Smirnov 2018-08-13 14:52 ` Bastien Nocera 2018-09-26 12:51 ` Dollinger Florian 2018-09-26 13:45 ` Bastien Nocera 2018-09-26 15:15 ` AW: " dollinger.florian 2018-09-26 18:02 ` Bastien Nocera 2018-09-26 22:50 ` Florian Dollinger 2018-09-26 16:09 ` XBox One S controller, Bluetooth support (was Re: hid: microsoft: Add rumble support for Xbox One S controller) Bastien Nocera 2018-09-26 17:43 ` Bastien Nocera 2018-09-26 22:35 ` Florian Dollinger
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).