| Message ID | 20240310180322.25508-2-tinozzo123@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] HID: nintendo: use ida for LED player id | expand |
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);
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
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);