[v5,RESEND] 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>
---
Changes for v2:

Applied suggestions, commit message explains more stuff, restored `return ret`
when `devm_led_classdev_register` fails (since all other hid drivers do this).
If setting LED fails, `hid_warn` now explicitly says "skipping registration".

Changes for v3 and v4:

Fixed errors and warnings from test robot.

Changes for v5:

I thought that when connecting the controller on an actual Nintendo Switch,
only the nth player LED would turn on, which is true... on Wii and Wii U.
So I reverted that, and to compensate, now this supports the LED patterns
up to 8 players.

 drivers/hid/hid-nintendo.c | 133 +++++++++++++++++++++----------------
 1 file changed, 76 insertions(+), 57 deletions(-)

Hi Martino,

On Sun, Sep 24, 2023 at 10:16 AM Martino Fontana <tinozzo123@gmail.com> wrote:
>
> - 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>
> ---
> Changes for v2:
>
> Applied suggestions, commit message explains more stuff, restored `return ret`
> when `devm_led_classdev_register` fails (since all other hid drivers do this).
> If setting LED fails, `hid_warn` now explicitly says "skipping registration".
>
> Changes for v3 and v4:
>
> Fixed errors and warnings from test robot.
>
> Changes for v5:
>
> I thought that when connecting the controller on an actual Nintendo Switch,
> only the nth player LED would turn on, which is true... on Wii and Wii U.
> So I reverted that, and to compensate, now this supports the LED patterns
> up to 8 players.
>
>  drivers/hid/hid-nintendo.c | 133 +++++++++++++++++++++----------------
>  1 file changed, 76 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> index 10468f727..138f154fe 100644
> --- 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;
>                 }
>         }
>
> --
> 2.42.0
>

This looks good to me. Thanks for the patch.

Reviewed-by: Daniel J. Ogorchock <djogorchock@gmail.com>