| Message ID | 20200622225728.330-1-gerbilsoft@gerbilsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | [1/2] HID: wiimote: Initialize the controller LEDs with a device ID value | expand |
Hi On Tue, Jun 23, 2020 at 12:57 AM David Korth <gerbilsoft@gerbilsoft.com> wrote: > > Based on a similar commit for Sony Sixaxis and DualShock 4 controllers: > HID: sony: Initialize the controller LEDs with a device ID value > > Wii remotes have the same player LED layout as Sixaxis controllers, > so the wiimote setup is based on the Sixaxis code. Please include a description of the patch in the commit-message. It took me quite a while to understand what the intention of this patch is. > Signed-off-by: David Korth <gerbilsoft@gerbilsoft.com> > --- > drivers/hid/hid-wiimote-core.c | 57 ++++++++++++++++++++++++++++++- > drivers/hid/hid-wiimote-modules.c | 7 ---- > drivers/hid/hid-wiimote.h | 1 + > 3 files changed, 57 insertions(+), 8 deletions(-) > > diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c > index 92874dbe4d4a..9662c2ce5e99 100644 > --- a/drivers/hid/hid-wiimote-core.c > +++ b/drivers/hid/hid-wiimote-core.c > @@ -14,9 +14,12 @@ > #include <linux/module.h> > #include <linux/mutex.h> > #include <linux/spinlock.h> > +#include <linux/idr.h> > #include "hid-ids.h" > #include "hid-wiimote.h" > > +static DEFINE_IDA(wiimote_device_id_allocator); > + > /* output queue handling */ > > static int wiimote_hid_send(struct hid_device *hdev, __u8 *buffer, > @@ -694,6 +697,10 @@ static void wiimote_modules_unload(struct wiimote_data *wdata) > wdata->state.devtype = WIIMOTE_DEV_UNKNOWN; > spin_unlock_irqrestore(&wdata->state.lock, flags); > > + if (wdata->device_id >= 0) > + ida_simple_remove(&wiimote_device_id_allocator, > + wdata->device_id); > + > /* find end of list */ > for (iter = mods; *iter != WIIMOD_NULL; ++iter) > /* empty */ ; > @@ -802,6 +809,20 @@ static const char *wiimote_devtype_names[WIIMOTE_DEV_NUM] = { > [WIIMOTE_DEV_PRO_CONTROLLER] = "Nintendo Wii U Pro Controller", > }; > > +static __u8 wiimote_set_leds_from_id(int id) > +{ > + static const __u8 wiimote_leds[10] = { > + 0x01, 0x02, 0x04, 0x08, > + 0x09, 0x0A, 0x0C, 0x0D, > + 0x0E, 0x0F > + }; > + > + if (id < 0) > + return wiimote_leds[0]; > + > + return wiimote_leds[id % 10]; > +} > + > /* Try to guess the device type based on all collected information. We > * first try to detect by static extension types, then VID/PID and the > * device name. If we cannot detect the device, we use > @@ -810,8 +831,10 @@ static void wiimote_init_set_type(struct wiimote_data *wdata, > __u8 exttype) > { > __u8 devtype = WIIMOTE_DEV_GENERIC; > + __u8 leds; > __u16 vendor, product; > const char *name; > + int device_id; > > vendor = wdata->hdev->vendor; > product = wdata->hdev->product; > @@ -858,6 +881,24 @@ static void wiimote_init_set_type(struct wiimote_data *wdata, > wiimote_devtype_names[devtype]); > > wiimote_modules_load(wdata, devtype); > + > + /* set player number to stop initial LED-blinking */ > + device_id = ida_simple_get(&wiimote_device_id_allocator, 0, 0, > + GFP_KERNEL); > + if (device_id < 0) { > + /* Unable to get a device ID. */ > + /* Set LED1 anyway to stop the blinking. */ > + leds = WIIPROTO_FLAG_LED1; > + wdata->device_id = -1; > + } else { > + /* Device ID obtained. */ > + leds = wiimote_set_leds_from_id(device_id); > + wdata->device_id = device_id; > + } > + > + spin_lock_irq(&wdata->state.lock); > + wiiproto_req_leds(wdata, leds); > + spin_unlock_irq(&wdata->state.lock); So what you are trying is to allocate a unique ID to each connected wiimote, so they automatically display unique IDs, right? Can you explain why this has to be done in the kernel driver? Why isn't user-space assigning the right ID? User-space needs to attach controllers to their respective engine anyway, in which case the IDs the kernel assigns would be wrong, right? How does user-space display the matching ID in their UI (e.g., for configuration use-cases)? The way you set them does not allow user-space to query the ID, does it? Lastly, wouldn't a device-reconnect want the same ID to be assigned again? With the logic you apply, user-space would have to override every ID for that to work. Is there an actual use-case for this? Or is this just to align the driver with the other gamepads? Thanks David > } > > static void wiimote_init_detect(struct wiimote_data *wdata) > @@ -1750,6 +1791,8 @@ static struct wiimote_data *wiimote_create(struct hid_device *hdev) > wdata->state.drm = WIIPROTO_REQ_DRM_K; > wdata->state.cmd_battery = 0xff; > > + wdata->device_id = -1; > + > INIT_WORK(&wdata->init_worker, wiimote_init_worker); > timer_setup(&wdata->timer, wiimote_init_timeout, 0); > > @@ -1879,7 +1922,19 @@ static struct hid_driver wiimote_hid_driver = { > .remove = wiimote_hid_remove, > .raw_event = wiimote_hid_event, > }; > -module_hid_driver(wiimote_hid_driver); > + > +static int __init wiimote_init(void) > +{ > + return hid_register_driver(&wiimote_hid_driver); > +} > + > +static void __exit wiimote_exit(void) > +{ > + ida_destroy(&wiimote_device_id_allocator); > + hid_unregister_driver(&wiimote_hid_driver); > +} > +module_init(wiimote_init); > +module_exit(wiimote_exit); > > MODULE_LICENSE("GPL"); > MODULE_AUTHOR("David Herrmann <dh.herrmann@gmail.com>"); > diff --git a/drivers/hid/hid-wiimote-modules.c b/drivers/hid/hid-wiimote-modules.c > index 2c3925357857..0cdd6c219b5d 100644 > --- a/drivers/hid/hid-wiimote-modules.c > +++ b/drivers/hid/hid-wiimote-modules.c > @@ -362,13 +362,6 @@ static int wiimod_led_probe(const struct wiimod_ops *ops, > if (ret) > goto err_free; > > - /* enable LED1 to stop initial LED-blinking */ > - if (ops->arg == 0) { > - spin_lock_irqsave(&wdata->state.lock, flags); > - wiiproto_req_leds(wdata, WIIPROTO_FLAG_LED1); > - spin_unlock_irqrestore(&wdata->state.lock, flags); > - } > - > return 0; > > err_free: > diff --git a/drivers/hid/hid-wiimote.h b/drivers/hid/hid-wiimote.h > index b2a26a0a8f12..800849427947 100644 > --- a/drivers/hid/hid-wiimote.h > +++ b/drivers/hid/hid-wiimote.h > @@ -160,6 +160,7 @@ struct wiimote_data { > struct wiimote_queue queue; > struct wiimote_state state; > struct work_struct init_worker; > + int device_id; > }; > > /* wiimote modules */ > -- > 2.27.0 >
On Wednesday, June 24, 2020 6:04:55 AM EDT David Rheinsberg wrote: > Hi > > On Tue, Jun 23, 2020 at 12:57 AM David Korth <gerbilsoft@gerbilsoft.com> wrote: > > Based on a similar commit for Sony Sixaxis and DualShock 4 controllers: > > HID: sony: Initialize the controller LEDs with a device ID value > > > > Wii remotes have the same player LED layout as Sixaxis controllers, > > so the wiimote setup is based on the Sixaxis code. > > Please include a description of the patch in the commit-message. It > took me quite a while to understand what the intention of this patch > is. Will do. > So what you are trying is to allocate a unique ID to each connected > wiimote, so they automatically display unique IDs, right? > > Can you explain why this has to be done in the kernel driver? Why > isn't user-space assigning the right ID? User-space needs to attach > controllers to their respective engine anyway, in which case the IDs > the kernel assigns would be wrong, right? How does user-space display > the matching ID in their UI (e.g., for configuration use-cases)? The > way you set them does not allow user-space to query the ID, does it? > Lastly, wouldn't a device-reconnect want the same ID to be assigned > again? With the logic you apply, user-space would have to override > every ID for that to work. > > Is there an actual use-case for this? Or is this just to align the > driver with the other gamepads? Most userspace programs aren't aware of controller LEDs. The only one I know of that is, Steam, has its own input layer and bypasses the HID drivers entirely. As far as I know, there's no simple "set player ID" API that could be used to set a device-independent player number, so every program would need to support each type of controller in order to set the LEDs. I've been manually setting the player IDs on Wii controllers when running multiplayer games by writing to the /sys/class/leds/ directory. Having the hid-wiimote driver do this itself significantly reduces setup time. > > Thanks > David >
Hi On Thu, 25 Jun 2020 at 00:09, David Korth <gerbilsoft@gerbilsoft.com> wrote: > I've been manually setting the player IDs on Wii controllers when running > multiplayer games by writing to the /sys/class/leds/ directory. Having the > hid-wiimote driver do this itself significantly reduces setup time. What do you mean with "reduces setup time significantly"? Why would it take that long to set the LEDs? Thanks David
On Thursday, June 25, 2020 3:09:46 AM EDT David Rheinsberg wrote: > Hi > > On Thu, 25 Jun 2020 at 00:09, David Korth <gerbilsoft@gerbilsoft.com> wrote: > > I've been manually setting the player IDs on Wii controllers when running > > multiplayer games by writing to the /sys/class/leds/ directory. Having the > > hid-wiimote driver do this itself significantly reduces setup time. > > What do you mean with "reduces setup time significantly"? Why would it > take that long to set the LEDs? > > Thanks > David The LED setup in this case is done entirely manually by me writing to the individual files in /sys/class/leds/. This has to be done when the controllers are connected initially, and if a controller has to be reconnected for some reason (e.g. it runs out of batteries). I don't know of any userspace tools that would make this easier to automate, except maybe a shell script, and I'd probably still need to run it manually. Both the Sixaxis and Xpad drivers appear to implement something similar, so perhaps a higher-level "player number" mechanism that works with all controllers would be worth looking into. This could in theory be done with a userspace daemon too (or a udev hook). As it is right now, I still think implementing it in the wiimote driver is the best method to keep it consistent with the rest of the drivers without having to install additional userspace tools. Thanks
diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c index 92874dbe4d4a..9662c2ce5e99 100644 --- a/drivers/hid/hid-wiimote-core.c +++ b/drivers/hid/hid-wiimote-core.c @@ -14,9 +14,12 @@ #include <linux/module.h> #include <linux/mutex.h> #include <linux/spinlock.h> +#include <linux/idr.h> #include "hid-ids.h" #include "hid-wiimote.h" +static DEFINE_IDA(wiimote_device_id_allocator); + /* output queue handling */ static int wiimote_hid_send(struct hid_device *hdev, __u8 *buffer, @@ -694,6 +697,10 @@ static void wiimote_modules_unload(struct wiimote_data *wdata) wdata->state.devtype = WIIMOTE_DEV_UNKNOWN; spin_unlock_irqrestore(&wdata->state.lock, flags); + if (wdata->device_id >= 0) + ida_simple_remove(&wiimote_device_id_allocator, + wdata->device_id); + /* find end of list */ for (iter = mods; *iter != WIIMOD_NULL; ++iter) /* empty */ ; @@ -802,6 +809,20 @@ static const char *wiimote_devtype_names[WIIMOTE_DEV_NUM] = { [WIIMOTE_DEV_PRO_CONTROLLER] = "Nintendo Wii U Pro Controller", }; +static __u8 wiimote_set_leds_from_id(int id) +{ + static const __u8 wiimote_leds[10] = { + 0x01, 0x02, 0x04, 0x08, + 0x09, 0x0A, 0x0C, 0x0D, + 0x0E, 0x0F + }; + + if (id < 0) + return wiimote_leds[0]; + + return wiimote_leds[id % 10]; +} + /* Try to guess the device type based on all collected information. We * first try to detect by static extension types, then VID/PID and the * device name. If we cannot detect the device, we use @@ -810,8 +831,10 @@ static void wiimote_init_set_type(struct wiimote_data *wdata, __u8 exttype) { __u8 devtype = WIIMOTE_DEV_GENERIC; + __u8 leds; __u16 vendor, product; const char *name; + int device_id; vendor = wdata->hdev->vendor; product = wdata->hdev->product; @@ -858,6 +881,24 @@ static void wiimote_init_set_type(struct wiimote_data *wdata, wiimote_devtype_names[devtype]); wiimote_modules_load(wdata, devtype); + + /* set player number to stop initial LED-blinking */ + device_id = ida_simple_get(&wiimote_device_id_allocator, 0, 0, + GFP_KERNEL); + if (device_id < 0) { + /* Unable to get a device ID. */ + /* Set LED1 anyway to stop the blinking. */ + leds = WIIPROTO_FLAG_LED1; + wdata->device_id = -1; + } else { + /* Device ID obtained. */ + leds = wiimote_set_leds_from_id(device_id); + wdata->device_id = device_id; + } + + spin_lock_irq(&wdata->state.lock); + wiiproto_req_leds(wdata, leds); + spin_unlock_irq(&wdata->state.lock); } static void wiimote_init_detect(struct wiimote_data *wdata) @@ -1750,6 +1791,8 @@ static struct wiimote_data *wiimote_create(struct hid_device *hdev) wdata->state.drm = WIIPROTO_REQ_DRM_K; wdata->state.cmd_battery = 0xff; + wdata->device_id = -1; + INIT_WORK(&wdata->init_worker, wiimote_init_worker); timer_setup(&wdata->timer, wiimote_init_timeout, 0); @@ -1879,7 +1922,19 @@ static struct hid_driver wiimote_hid_driver = { .remove = wiimote_hid_remove, .raw_event = wiimote_hid_event, }; -module_hid_driver(wiimote_hid_driver); + +static int __init wiimote_init(void) +{ + return hid_register_driver(&wiimote_hid_driver); +} + +static void __exit wiimote_exit(void) +{ + ida_destroy(&wiimote_device_id_allocator); + hid_unregister_driver(&wiimote_hid_driver); +} +module_init(wiimote_init); +module_exit(wiimote_exit); MODULE_LICENSE("GPL"); MODULE_AUTHOR("David Herrmann <dh.herrmann@gmail.com>"); diff --git a/drivers/hid/hid-wiimote-modules.c b/drivers/hid/hid-wiimote-modules.c index 2c3925357857..0cdd6c219b5d 100644 --- a/drivers/hid/hid-wiimote-modules.c +++ b/drivers/hid/hid-wiimote-modules.c @@ -362,13 +362,6 @@ static int wiimod_led_probe(const struct wiimod_ops *ops, if (ret) goto err_free; - /* enable LED1 to stop initial LED-blinking */ - if (ops->arg == 0) { - spin_lock_irqsave(&wdata->state.lock, flags); - wiiproto_req_leds(wdata, WIIPROTO_FLAG_LED1); - spin_unlock_irqrestore(&wdata->state.lock, flags); - } - return 0; err_free: diff --git a/drivers/hid/hid-wiimote.h b/drivers/hid/hid-wiimote.h index b2a26a0a8f12..800849427947 100644 --- a/drivers/hid/hid-wiimote.h +++ b/drivers/hid/hid-wiimote.h @@ -160,6 +160,7 @@ struct wiimote_data { struct wiimote_queue queue; struct wiimote_state state; struct work_struct init_worker; + int device_id; }; /* wiimote modules */
Based on a similar commit for Sony Sixaxis and DualShock 4 controllers: HID: sony: Initialize the controller LEDs with a device ID value Wii remotes have the same player LED layout as Sixaxis controllers, so the wiimote setup is based on the Sixaxis code. Signed-off-by: David Korth <gerbilsoft@gerbilsoft.com> --- drivers/hid/hid-wiimote-core.c | 57 ++++++++++++++++++++++++++++++- drivers/hid/hid-wiimote-modules.c | 7 ---- drivers/hid/hid-wiimote.h | 1 + 3 files changed, 57 insertions(+), 8 deletions(-)