Received-SPF: pass (google.com: domain of linux-kernel+bounces-98334-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Sun, 10 Mar 2024 21:58:37 +0100
To: Martino Fontana <tinozzo123@gmail.com>
Cc: djogorchock@gmail.com, jikos@kernel.org, benjamin.tissoires@redhat.com,
 linux-input@vger.kernel.org, linux-kernel@vger.kernel.org, Ryan McClelland
 <rymcclel@gmail.com>
Subject: Re: [PATCH v2] HID: nintendo: use ida for LED player id
From: "Silvan Jegen" <s.jegen@gmail.com>
References: <20240310180322.25508-2-tinozzo123@gmail.com>
In-Reply-To: <20240310180322.25508-2-tinozzo123@gmail.com>
Message-Id: <2TCU3VNKI7TD9.2PWW1CG9ZHYBA@homearch.localdomain>
User-Agent: mblaze/1.2-27-gea64d39 (2024-02-07)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Heyhey

One question below.

Martino Fontana <tinozzo123@gmail.com> wrote:
> Previously, the leds pattern would just increment with every controller
> connected. This wouldn't take into consideration when controllers are
> disconnected. The same controller could be connected and disconnected
> with the pattern increasing player count each time.
>=20
> This patch changes it by using an ID allocator in order to assign the
> player id, the same way hid-playstation does.
>=20
> Signed-off-by: Martino Fontana <tinozzo123@gmail.com>
> Signed-off-by: Ryan McClelland <rymcclel@gmail.com>
> ---
> Changes for v2:
>=20
> ida_free now frees the correct id, instead of an id that got moduloed.
>=20
>  drivers/hid/hid-nintendo.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
>=20
> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> index ccc4032fb2b0..6ab4c2ec4a5d 100644
> --- a/drivers/hid/hid-nintendo.c
> +++ b/drivers/hid/hid-nintendo.c
> @@ -34,6 +34,7 @@
>  #include <linux/device.h>
>  #include <linux/kernel.h>
>  #include <linux/hid.h>
> +#include <linux/idr.h>
>  #include <linux/input.h>
>  #include <linux/jiffies.h>
>  #include <linux/leds.h>
> @@ -569,6 +570,7 @@ static const enum led_brightness joycon_player_led_pa=
tterns[JC_NUM_LED_PATTERNS]
>  struct joycon_ctlr {
>  	struct hid_device *hdev;
>  	struct input_dev *input;
> +	u32 player_id;
>  	struct led_classdev leds[JC_NUM_LEDS]; /* player leds */
>  	struct led_classdev home_led;
>  	enum joycon_ctlr_state ctlr_state;
> @@ -2283,7 +2285,8 @@ static int joycon_home_led_brightness_set(struct le=
d_classdev *led,
>  	return ret;
>  }
> =20
> -static DEFINE_SPINLOCK(joycon_input_num_spinlock);
> +static DEFINE_IDA(nintendo_player_id_allocator);
> +
>  static int joycon_leds_create(struct joycon_ctlr *ctlr)
>  {
>  	struct hid_device *hdev =3D ctlr->hdev;
> @@ -2294,20 +2297,19 @@ static int joycon_leds_create(struct joycon_ctlr =
*ctlr)
>  	char *name;
>  	int ret;
>  	int i;
> -	unsigned long flags;
>  	int player_led_pattern;
> -	static int input_num;
> -
> -	/*
> -	 * Set the player leds based on controller number
> -	 * Because there is no standard concept of "player number", the pattern=

> -	 * number will simply increase by 1 every time a controller is connecte=
d.
> -	 */
> -	spin_lock_irqsave(&joycon_input_num_spinlock, flags);
> -	player_led_pattern =3D input_num++ % JC_NUM_LED_PATTERNS;
> -	spin_unlock_irqrestore(&joycon_input_num_spinlock, flags);
> =20
>  	/* configure the player LEDs */
> +	ctlr->player_id =3D U32_MAX;
> +	ret =3D ida_alloc(&nintendo_player_id_allocator, GFP_KERNEL);
> +	if (ret < 0) {
> +		hid_warn(hdev, "Failed to allocate player ID, skipping; ret=3D%d\n", r=
et);
> +		goto home_led;
> +	}
> +	ctlr->player_id =3D ret;
> +	player_led_pattern =3D ret % JC_NUM_LED_PATTERNS;
> +	hid_info(ctlr->hdev, "assigned player %d led pattern", player_led_patte=
rn + 1);
> +
>  	for (i =3D 0; i < JC_NUM_LEDS; i++) {
>  		name =3D devm_kasprintf(dev, GFP_KERNEL, "%s:%s:%s",
>  				      d_name,
> @@ -2789,6 +2791,7 @@ static void nintendo_hid_remove(struct hid_device *=
hdev)
>  	spin_unlock_irqrestore(&ctlr->lock, flags);
> =20
>  	destroy_workqueue(ctlr->rumble_queue);
> +	ida_free(&nintendo_player_id_allocator, ctlr->player_id);

The PlayStation driver also destroys the allocator on module exit. Do
we not have to do the same in this module?

Cheers,
Silvan

>  	hid_hw_close(hdev);
>  	hid_hw_stop(hdev);