Re: [PATCH v3 1/4] HID: wiimote: turn on the LEDs indicating the controller id

["David Rheinsberg" <david@readahead.eu>]

Hi

On Wed, Jul 29, 2026, at 6:49 PM, Rafael Passos wrote:
> The behavior in a Wii/Wii U console is to have each controller turn on
> a different LED indicating the controller id.
> This commit implements the same behavior using the ida struct.
> Unlike switch controllers, each ID only turns one LED (from 1 to 4).
>
> Signed-off-by: Rafael Passos <rafael@rcpassos.me>
> ---
>  drivers/hid/hid-wiimote-core.c | 54 ++++++++++++++++++++++++++++++----
>  drivers/hid/hid-wiimote.h      |  1 +
>  2 files changed, 49 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hid/hid-wiimote-core.c 
> b/drivers/hid/hid-wiimote-core.c
> index 63c4fa8fbb9b6..48830f2ffcb50 100644
> --- a/drivers/hid/hid-wiimote-core.c
> +++ b/drivers/hid/hid-wiimote-core.c
> @@ -621,6 +621,13 @@ static const __u8 * const 
> wiimote_devtype_mods[WIIMOTE_DEV_NUM] = {
>  	},
>  };
> 
> +static const __u8 player_leds[] = {
> +	WIIPROTO_FLAG_LED1,
> +	WIIPROTO_FLAG_LED2,
> +	WIIPROTO_FLAG_LED3,
> +	WIIPROTO_FLAG_LED4
> +};
> +
>  static void wiimote_modules_load(struct wiimote_data *wdata,
>  				 unsigned int devtype)
>  {
> @@ -671,6 +678,12 @@ static void wiimote_modules_load(struct 
> wiimote_data *wdata,
>  	spin_lock_irq(&wdata->state.lock);
>  	wdata->state.devtype = devtype;
>  	spin_unlock_irq(&wdata->state.lock);
> +
> +	scoped_guard(spinlock_irqsave, &wdata->state.lock) {
> +		/* after loading modules, set the Player ID LED cycling from 1 to 4*/
> +		wiiproto_req_leds(wdata, player_leds[(wdata->player_id - 1) % 4]);
> +	}
> +
>  	return;
> 
>  error:
> @@ -855,11 +868,11 @@ static void wiimote_init_set_type(struct 
> wiimote_data *wdata,
> 
>  done:
>  	if (devtype == WIIMOTE_DEV_GENERIC)
> -		hid_info(wdata->hdev, "cannot detect device; NAME: %s VID: %04x PID: 
> %04x EXT: %04x\n",
> -			name, vendor, product, exttype);
> +		hid_info(wdata->hdev, "cannot detect device; NAME: %s VID: %04x PID: 
> %04x EXT: %04x (%d)\n",
> +			name, vendor, product, exttype, wdata->player_id);
>  	else
> -		hid_info(wdata->hdev, "detected device: %s\n",
> -			 wiimote_devtype_names[devtype]);
> +		hid_info(wdata->hdev, "detected device: %s (%d)\n",
> +			 wiimote_devtype_names[devtype], wdata->player_id);
> 
>  	wiimote_modules_load(wdata, devtype);
>  }
> @@ -1786,11 +1799,15 @@ static void wiimote_destroy(struct wiimote_data *wdata)
>  	kfree(wdata);
>  }
> 
> +/* Global id allocator for wii remotes */
> +static DEFINE_IDA(wiimote_ida);
> +
>  static int wiimote_hid_probe(struct hid_device *hdev,
>  				const struct hid_device_id *id)
>  {
>  	struct wiimote_data *wdata;
>  	int ret;
> +	int player_id;
> 
>  	hdev->quirks |= HID_QUIRK_NO_INIT_REPORTS;
> 
> @@ -1834,7 +1851,16 @@ static int wiimote_hid_probe(struct hid_device *hdev,
>  	if (ret)
>  		goto err_free;
> 
> -	hid_info(hdev, "New device registered\n");
> +	player_id = ida_alloc_min(&wiimote_ida, 1, GFP_KERNEL);
> +	if (player_id < 1) {
> +		hid_err(hdev, "cannot allocate controller id\n");
> +		ret = player_id;
> +		goto err_free;
> +	}
> +
> +	wdata->player_id = player_id;
> +
> +	hid_info(hdev, "New device registered (Wiimote %d)\n", player_id);

`wiimote_create()` should initialize `wdata->player_id` to -1, so `ida_free()` is a no-op in the `err_free` path. Right now it is implicitly initialized to 0, and thus ida_free() will warn about an unallocated ID.

Also: Why not just use IDs starting from 0?

Lastly, this initialization can be simplified to:

ret = ida_alloc_min(&wiimote_ida, 1, GFP_KERNEL);
if (ret < 1) {
	hid_err(hdev, "cannot allocate controller id\n");
	goto err_free;
}

wdata->player_id = ret;

> 
>  	/* schedule device detection */
>  	wiimote_schedule(wdata);
> @@ -1862,6 +1888,8 @@ static void wiimote_hid_remove(struct hid_device *hdev)
>  {
>  	struct wiimote_data *wdata = hid_get_drvdata(hdev);
> 
> +	ida_free(&wiimote_ida, wdata->player_id);
> +
>  	hid_info(hdev, "Device removed\n");
>  	wiimote_destroy(wdata);
>  }
> @@ -1887,7 +1915,21 @@ 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)
> +{
> +	hid_unregister_driver(&wiimote_hid_driver);
> +	ida_destroy(&wiimote_ida);
> +}
> +
> +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.h b/drivers/hid/hid-wiimote.h
> index 9c12f63f6dd2d..a53f72d5077ef 100644
> --- a/drivers/hid/hid-wiimote.h
> +++ b/drivers/hid/hid-wiimote.h
> @@ -153,6 +153,7 @@ struct wiimote_data {
>  	struct input_dev *mp;
>  	struct timer_list timer;
>  	struct wiimote_debug *debug;
> +	__u8 player_id;

You can use `u8`. Double underscore is only needed for uapi headers.

Otherwise looks good!

David

> 
>  	union {
>  		struct input_dev *input;
> -- 
> 2.53.0