Heyhey
One question below.
Martino Fontana <tinozzo123@xxxxxxxxx> 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@xxxxxxxxx>
> Signed-off-by: Ryan McClelland <rymcclel@xxxxxxxxx>
> ---
> 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);
