[PATCH v2] HID: nintendo: use ida for LED player id

[PATCH v2] HID: nintendo: use ida for LED player id

[Martino Fontana]

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.

This patch changes it by using an ID allocator in order to assign the
player id, the same way hid-playstation does.

Signed-off-by: Martino Fontana <tinozzo123@gmail.com>
Signed-off-by: Ryan McClelland <rymcclel@gmail.com>
---
Changes for v2:

ida_free now frees the correct id, instead of an id that got moduloed.

 drivers/hid/hid-nintendo.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

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_patterns[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 led_classdev *led,
 	return ret;
 }
 
-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 = 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 connected.
-	 */
-	spin_lock_irqsave(&joycon_input_num_spinlock, flags);
-	player_led_pattern = input_num++ % JC_NUM_LED_PATTERNS;
-	spin_unlock_irqrestore(&joycon_input_num_spinlock, flags);
 
 	/* configure the player LEDs */
+	ctlr->player_id = U32_MAX;
+	ret = ida_alloc(&nintendo_player_id_allocator, GFP_KERNEL);
+	if (ret < 0) {
+		hid_warn(hdev, "Failed to allocate player ID, skipping; ret=%d\n", ret);
+		goto home_led;
+	}
+	ctlr->player_id = ret;
+	player_led_pattern = ret % JC_NUM_LED_PATTERNS;
+	hid_info(ctlr->hdev, "assigned player %d led pattern", player_led_pattern + 1);
+
 	for (i = 0; i < JC_NUM_LEDS; i++) {
 		name = 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);
 
 	destroy_workqueue(ctlr->rumble_queue);
+	ida_free(&nintendo_player_id_allocator, ctlr->player_id);
 
 	hid_hw_close(hdev);
 	hid_hw_stop(hdev);
-- 
2.43.1

Re: [PATCH v2] HID: nintendo: use ida for LED player id

[Silvan Jegen]

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.
> 
> This patch changes it by using an ID allocator in order to assign the
> player id, the same way hid-playstation does.
> 
> Signed-off-by: Martino Fontana <tinozzo123@gmail.com>
> Signed-off-by: Ryan McClelland <rymcclel@gmail.com>
> ---
> Changes for v2:
> 
> ida_free now frees the correct id, instead of an id that got moduloed.
> 
>  drivers/hid/hid-nintendo.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
> 
> 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_patterns[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 led_classdev *led,
>  	return ret;
>  }
>  
> -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 = 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 connected.
> -	 */
> -	spin_lock_irqsave(&joycon_input_num_spinlock, flags);
> -	player_led_pattern = input_num++ % JC_NUM_LED_PATTERNS;
> -	spin_unlock_irqrestore(&joycon_input_num_spinlock, flags);
>  
>  	/* configure the player LEDs */
> +	ctlr->player_id = U32_MAX;
> +	ret = ida_alloc(&nintendo_player_id_allocator, GFP_KERNEL);
> +	if (ret < 0) {
> +		hid_warn(hdev, "Failed to allocate player ID, skipping; ret=%d\n", ret);
> +		goto home_led;
> +	}
> +	ctlr->player_id = ret;
> +	player_led_pattern = ret % JC_NUM_LED_PATTERNS;
> +	hid_info(ctlr->hdev, "assigned player %d led pattern", player_led_pattern + 1);
> +
>  	for (i = 0; i < JC_NUM_LEDS; i++) {
>  		name = 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);
>  
>  	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);

Re: [PATCH v2] HID: nintendo: use ida for LED player id

[Martino Fontana]

Hi

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

I forgot to do that...

If everything else's good, I'll send a v3 that fixes that.

Cheers,
Martino

[PATCH v2] HID: nintendo: use ida for LED player id

[Martino Fontana]

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.

This patch changes it by using an ID allocator in order to assign the
player id, the same way hid-playstation does.

Signed-off-by: Martino Fontana <tinozzo123@gmail.com>
Signed-off-by: Ryan McClelland <rymcclel@gmail.com>
---
Changes for v2:

ida_free now frees the correct id, instead of an id that got moduloed.

 drivers/hid/hid-nintendo.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

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_patterns[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 led_classdev *led,
 	return ret;
 }
 
-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 = 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 connected.
-	 */
-	spin_lock_irqsave(&joycon_input_num_spinlock, flags);
-	player_led_pattern = input_num++ % JC_NUM_LED_PATTERNS;
-	spin_unlock_irqrestore(&joycon_input_num_spinlock, flags);
 
 	/* configure the player LEDs */
+	ctlr->player_id = U32_MAX;
+	ret = ida_alloc(&nintendo_player_id_allocator, GFP_KERNEL);
+	if (ret < 0) {
+		hid_warn(hdev, "Failed to allocate player ID, skipping; ret=%d\n", ret);
+		goto home_led;
+	}
+	ctlr->player_id = ret;
+	player_led_pattern = ret % JC_NUM_LED_PATTERNS;
+	hid_info(ctlr->hdev, "assigned player %d led pattern", player_led_pattern + 1);
+
 	for (i = 0; i < JC_NUM_LEDS; i++) {
 		name = 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);
 
 	destroy_workqueue(ctlr->rumble_queue);
+	ida_free(&nintendo_player_id_allocator, ctlr->player_id);
 
 	hid_hw_close(hdev);
 	hid_hw_stop(hdev);
-- 
2.43.0