HID: nintendo: cleanup LED code

- Support player LED patterns up to 8 players.
  (Note that the behavior still consinsts in increasing the player number
  every time a controller is connected, never decreasing it. It should be
  as is described in https://bugzilla.kernel.org/show_bug.cgi?id=216225.
  However, any implementation here would stop making sense as soon as a
  non-Nintendo controller is connected, which is why I'm not bothering.)

- Split part of `joycon_home_led_brightness_set` (which is called by hid)
  into `joycon_set_home_led` (which is what actually sets the LEDs), for
  consistency with player LEDs.

- `joycon_player_led_brightness_set` won't try it to "determine which
  player led this is" anymore: it's already looking at every LED
  brightness value.

- Instead of first registering the `led_classdev`, then attempting to set
  the LED and unregistering the `led_classdev` if it fails, first attempt
  to set the LED, then register the `led_classdev` only if it succeeds
  (the class is still filled up in either case).

- If setting the player LEDs fails, still attempt setting the home LED.
  (I don't know there's a third party controller where this may actually
  happen, but who knows...)

- Use `JC_NUM_LEDS` where appropriate instead of 4.

- Print return codes in more places.

- Use spinlock instead of mutex for `input_num`. Copy its value to a local
  variable, so that it can be unlocked immediately.

- `input_num` starts counting from 0

- Less holding of mutexes in general.

Signed-off-by: Martino Fontana <tinozzo123@gmail.com>
Reviewed-by: Daniel J. Ogorchock <djogorchock@gmail.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>

diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
index 250f5d2f888ab0f7c19296d96fa3a018a0163d21..5635f4333fcc1c2703564a4cea3545086f37b1f1 100644 (file)
--- a/drivers/hid/hid-nintendo.c
+++ b/drivers/hid/hid-nintendo.c
@@ -410,6 +410,18 @@ static const char * const joycon_player_led_names[] = {
        LED_FUNCTION_PLAYER4,
 };
 #define JC_NUM_LEDS            ARRAY_SIZE(joycon_player_led_names)
+#define JC_NUM_LED_PATTERNS 8
+/* Taken from https://www.nintendo.com/my/support/qa/detail/33822 */
+static const enum led_brightness joycon_player_led_patterns[JC_NUM_LED_PATTERNS][JC_NUM_LEDS] = {
+       { 1, 0, 0, 0 },
+       { 1, 1, 0, 0 },
+       { 1, 1, 1, 0 },
+       { 1, 1, 1, 1 },
+       { 1, 0, 0, 1 },
+       { 1, 0, 1, 0 },
+       { 1, 0, 1, 1 },
+       { 0, 1, 1, 0 },
+};
 
 /* Each physical controller is associated with a joycon_ctlr struct */
 struct joycon_ctlr {
@@ -699,6 +711,25 @@ static int joycon_set_player_leds(struct joycon_ctlr *ctlr, u8 flash, u8 on)
        return joycon_send_subcmd(ctlr, req, 1, HZ/4);
 }
 
+static int joycon_set_home_led(struct joycon_ctlr *ctlr, enum led_brightness brightness)
+{
+       struct joycon_subcmd_request *req;
+       u8 buffer[sizeof(*req) + 5] = { 0 };
+       u8 *data;
+
+       req = (struct joycon_subcmd_request *)buffer;
+       req->subcmd_id = JC_SUBCMD_SET_HOME_LIGHT;
+       data = req->data;
+       data[0] = 0x01;
+       data[1] = brightness << 4;
+       data[2] = brightness | (brightness << 4);
+       data[3] = 0x11;
+       data[4] = 0x11;
+
+       hid_dbg(ctlr->hdev, "setting home led brightness\n");
+       return joycon_send_subcmd(ctlr, req, 5, HZ/4);
+}
+
 static int joycon_request_spi_flash_read(struct joycon_ctlr *ctlr,
                                         u32 start_addr, u8 size, u8 **reply)
 {
@@ -1840,6 +1871,7 @@ static int joycon_input_create(struct joycon_ctlr *ctlr)
        return 0;
 }
 
+/* Because the subcommand sets all the leds at once, the brightness argument is ignored */
 static int joycon_player_led_brightness_set(struct led_classdev *led,
                                            enum led_brightness brightness)
 {
@@ -1849,7 +1881,6 @@ static int joycon_player_led_brightness_set(struct led_classdev *led,
        int val = 0;
        int i;
        int ret;
-       int num;
 
        ctlr = hid_get_drvdata(hdev);
        if (!ctlr) {
@@ -1857,21 +1888,10 @@ static int joycon_player_led_brightness_set(struct led_classdev *led,
                return -ENODEV;
        }
 
-       /* determine which player led this is */
-       for (num = 0; num < JC_NUM_LEDS; num++) {
-               if (&ctlr->leds[num] == led)
-                       break;
-       }
-       if (num >= JC_NUM_LEDS)
-               return -EINVAL;
+       for (i = 0; i < JC_NUM_LEDS; i++)
+               val |= ctlr->leds[i].brightness << i;
 
        mutex_lock(&ctlr->output_mutex);
-       for (i = 0; i < JC_NUM_LEDS; i++) {
-               if (i == num)
-                       val |= brightness << i;
-               else
-                       val |= ctlr->leds[i].brightness << i;
-       }
        ret = joycon_set_player_leds(ctlr, 0, val);
        mutex_unlock(&ctlr->output_mutex);
 
@@ -1884,9 +1904,6 @@ static int joycon_home_led_brightness_set(struct led_classdev *led,
        struct device *dev = led->dev->parent;
        struct hid_device *hdev = to_hid_device(dev);
        struct joycon_ctlr *ctlr;
-       struct joycon_subcmd_request *req;
-       u8 buffer[sizeof(*req) + 5] = { 0 };
-       u8 *data;
        int ret;
 
        ctlr = hid_get_drvdata(hdev);
@@ -1894,43 +1911,35 @@ static int joycon_home_led_brightness_set(struct led_classdev *led,
                hid_err(hdev, "No controller data\n");
                return -ENODEV;
        }
-
-       req = (struct joycon_subcmd_request *)buffer;
-       req->subcmd_id = JC_SUBCMD_SET_HOME_LIGHT;
-       data = req->data;
-       data[0] = 0x01;
-       data[1] = brightness << 4;
-       data[2] = brightness | (brightness << 4);
-       data[3] = 0x11;
-       data[4] = 0x11;
-
-       hid_dbg(hdev, "setting home led brightness\n");
        mutex_lock(&ctlr->output_mutex);
-       ret = joycon_send_subcmd(ctlr, req, 5, HZ/4);
+       ret = joycon_set_home_led(ctlr, brightness);
        mutex_unlock(&ctlr->output_mutex);
-
        return ret;
 }
 
-static DEFINE_MUTEX(joycon_input_num_mutex);
+static DEFINE_SPINLOCK(joycon_input_num_spinlock);
 static int joycon_leds_create(struct joycon_ctlr *ctlr)
 {
        struct hid_device *hdev = ctlr->hdev;
        struct device *dev = &hdev->dev;
        const char *d_name = dev_name(dev);
        struct led_classdev *led;
+       int led_val = 0;
        char *name;
-       int ret = 0;
+       int ret;
        int i;
-       static int input_num = 1;
+       unsigned long flags;
+       int player_led_pattern;
+       static int input_num;
 
-       /* Set the default controller player leds based on controller number */
-       mutex_lock(&joycon_input_num_mutex);
-       mutex_lock(&ctlr->output_mutex);
-       ret = joycon_set_player_leds(ctlr, 0, 0xF >> (4 - input_num));
-       if (ret)
-               hid_warn(ctlr->hdev, "Failed to set leds; ret=%d\n", ret);
-       mutex_unlock(&ctlr->output_mutex);
+       /*
+        * 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 */
        for (i = 0; i < JC_NUM_LEDS; i++) {
@@ -1938,31 +1947,37 @@ static int joycon_leds_create(struct joycon_ctlr *ctlr)
                                      d_name,
                                      "green",
                                      joycon_player_led_names[i]);
-               if (!name) {
-                       mutex_unlock(&joycon_input_num_mutex);
+               if (!name)
                        return -ENOMEM;
-               }
 
                led = &ctlr->leds[i];
                led->name = name;
-               led->brightness = ((i + 1) <= input_num) ? 1 : 0;
+               led->brightness = joycon_player_led_patterns[player_led_pattern][i];
                led->max_brightness = 1;
                led->brightness_set_blocking =
                                        joycon_player_led_brightness_set;
                led->flags = LED_CORE_SUSPENDRESUME | LED_HW_PLUGGABLE;
 
+               led_val |= joycon_player_led_patterns[player_led_pattern][i] << i;
+       }
+       mutex_lock(&ctlr->output_mutex);
+       ret = joycon_set_player_leds(ctlr, 0, led_val);
+       mutex_unlock(&ctlr->output_mutex);
+       if (ret) {
+               hid_warn(hdev, "Failed to set players LEDs, skipping registration; ret=%d\n", ret);
+               goto home_led;
+       }
+
+       for (i = 0; i < JC_NUM_LEDS; i++) {
+               led = &ctlr->leds[i];
                ret = devm_led_classdev_register(&hdev->dev, led);
                if (ret) {
-                       hid_err(hdev, "Failed registering %s LED\n", led->name);
-                       mutex_unlock(&joycon_input_num_mutex);
+                       hid_err(hdev, "Failed to register player %d LED; ret=%d\n", i + 1, ret);
                        return ret;
                }
        }
 
-       if (++input_num > 4)
-               input_num = 1;
-       mutex_unlock(&joycon_input_num_mutex);
-
+home_led:
        /* configure the home LED */
        if (jc_type_has_right(ctlr)) {
                name = devm_kasprintf(dev, GFP_KERNEL, "%s:%s:%s",
@@ -1978,16 +1993,20 @@ static int joycon_leds_create(struct joycon_ctlr *ctlr)
                led->max_brightness = 0xF;
                led->brightness_set_blocking = joycon_home_led_brightness_set;
                led->flags = LED_CORE_SUSPENDRESUME | LED_HW_PLUGGABLE;
-               ret = devm_led_classdev_register(&hdev->dev, led);
+
+               /* Set the home LED to 0 as default state */
+               mutex_lock(&ctlr->output_mutex);
+               ret = joycon_set_home_led(ctlr, 0);
+               mutex_unlock(&ctlr->output_mutex);
                if (ret) {
-                       hid_err(hdev, "Failed registering home led\n");
-                       return ret;
+                       hid_warn(hdev, "Failed to set home LED, skipping registration; ret=%d\n", ret);
+                       return 0;
                }
-               /* Set the home LED to 0 as default state */
-               ret = joycon_home_led_brightness_set(led, 0);
+
+               ret = devm_led_classdev_register(&hdev->dev, led);
                if (ret) {
-                       hid_warn(hdev, "Failed to set home LED default, unregistering home LED");
-                       devm_led_classdev_unregister(&hdev->dev, led);
+                       hid_err(hdev, "Failed to register home LED; ret=%d\n", ret);
+                       return ret;
                }
        }