[PATCH] Enable HW LED blinking for hid-nintendo controllers

[PATCH] Enable HW LED blinking for hid-nintendo controllers

[Noa]

This is my first patch, so hopefully I'm doing this right. I noticed
when trying to use `ledtrig-timer` on the Joycon LEDS that it at times
would hang for a while (I assume the firmware isn't a fan of frequent
LED subcommands, or something), and I've tested with this patch and it
blinks consistently with carefree abandon.

Separately, I was also thinking about exposing the LEDs as evdev codes,
but it doesn't seem like there's any good options of the `LED_*`
constants to represent them.

Signed-off-by: Noa <coolreader18@gmail.com>
---
 drivers/hid/hid-nintendo.c | 45 ++++++++++++++++++++++++++++++++------
 1 file changed, 38 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
index 11ac246176ae..83dff0c4f7e1 100644
--- a/drivers/hid/hid-nintendo.c
+++ b/drivers/hid/hid-nintendo.c
@@ -2183,14 +2183,13 @@ 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)
+/* Update the on/flash status of the leds according to their led_classdev fields */
+static int joycon_update_player_leds(struct device *dev)
 {
-	struct device *dev = led->dev->parent;
 	struct hid_device *hdev = to_hid_device(dev);
 	struct joycon_ctlr *ctlr;
 	int val = 0;
+	int flash = 0;
 	int i;
 	int ret;
 
@@ -2200,16 +2199,47 @@ static int joycon_player_led_brightness_set(struct led_classdev *led,
 		return -ENODEV;
 	}
 
-	for (i = 0; i < JC_NUM_LEDS; i++)
-		val |= ctlr->leds[i].brightness << i;
+	for (i = 0; i < JC_NUM_LEDS; i++) {
+		if (ctlr->leds[i].blink_delay_on || ctlr->leds[i].blink_delay_off)
+			flash |= 1 << i;
+		else if (ctlr->leds[i].brightness)
+			val |= 1 << i;
+	}
 
 	mutex_lock(&ctlr->output_mutex);
-	ret = joycon_set_player_leds(ctlr, 0, val);
+	ret = joycon_set_player_leds(ctlr, flash, val);
 	mutex_unlock(&ctlr->output_mutex);
 
 	return ret;
 }
 
+static int joycon_player_led_brightness_set(struct led_classdev *led,
+					    enum led_brightness brightness)
+{
+	led->brightness = brightness;
+
+	if (!brightness)
+		led->blink_delay_on = led->blink_delay_off = 0;
+
+	return joycon_update_player_leds(led->dev->parent);
+}
+
+/* the blink period of the leds can't be changed, and is always these values */
+static const JC_LED_BLINK_DELAY_ON = 500;
+static const JC_LED_BLINK_DELAY_OFF = 200;
+/* the different leds on a joycon can't actually be set to hw blink independently
+ * of each other, since they all use the same one subcommand, so this function
+ * actually resets the cycle of all the leds */
+static int joycon_player_led_blink_set(struct led_classdev *led,
+				     unsigned long *delay_on,
+				     unsigned long *delay_off)
+{
+	led->blink_delay_on = *delay_on = JC_LED_BLINK_DELAY_ON;
+	led->blink_delay_off = *delay_off = JC_LED_BLINK_DELAY_OFF;
+
+	return joycon_update_player_leds(led->dev->parent);
+}
+
 static int joycon_home_led_brightness_set(struct led_classdev *led,
 					  enum led_brightness brightness)
 {
@@ -2268,6 +2298,7 @@ static int joycon_leds_create(struct joycon_ctlr *ctlr)
 		led->max_brightness = 1;
 		led->brightness_set_blocking =
 					joycon_player_led_brightness_set;
+		led->blink_set = joycon_player_led_blink_set;
 		led->flags = LED_CORE_SUSPENDRESUME | LED_HW_PLUGGABLE;
 
 		led_val |= joycon_player_led_patterns[player_led_pattern][i] << i;
-- 
2.48.1

Re: [PATCH] Enable HW LED blinking for hid-nintendo controllers

[Daniel Ogorchock]

Hi Noa,

Thanks for this patch. Integrating the firmware LED blinking support
is something I've wanted the driver to have for a long time.
Especially for joycond when it places the controller in "pairing"
mode.

Can you prepend the commit title with "HID: nintendo: " to match the
convention for other commits in the hid folder?

On Fri, Jan 24, 2025 at 7:02 PM Noa <coolreader18@gmail.com> wrote:
>
> This is my first patch, so hopefully I'm doing this right. I noticed
> when trying to use `ledtrig-timer` on the Joycon LEDS that it at times
> would hang for a while (I assume the firmware isn't a fan of frequent
> LED subcommands, or something), and I've tested with this patch and it
> blinks consistently with carefree abandon.
>
> Separately, I was also thinking about exposing the LEDs as evdev codes,
> but it doesn't seem like there's any good options of the `LED_*`
> constants to represent them.
>
> Signed-off-by: Noa <coolreader18@gmail.com>
> ---
>  drivers/hid/hid-nintendo.c | 45 ++++++++++++++++++++++++++++++++------
>  1 file changed, 38 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> index 11ac246176ae..83dff0c4f7e1 100644
> --- a/drivers/hid/hid-nintendo.c
> +++ b/drivers/hid/hid-nintendo.c
> @@ -2183,14 +2183,13 @@ 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)
> +/* Update the on/flash status of the leds according to their led_classdev fields */
> +static int joycon_update_player_leds(struct device *dev)
>  {
> -       struct device *dev = led->dev->parent;
>         struct hid_device *hdev = to_hid_device(dev);
>         struct joycon_ctlr *ctlr;
>         int val = 0;
> +       int flash = 0;
>         int i;
>         int ret;
>
> @@ -2200,16 +2199,47 @@ static int joycon_player_led_brightness_set(struct led_classdev *led,
>                 return -ENODEV;
>         }
>
> -       for (i = 0; i < JC_NUM_LEDS; i++)
> -               val |= ctlr->leds[i].brightness << i;
> +       for (i = 0; i < JC_NUM_LEDS; i++) {
> +               if (ctlr->leds[i].blink_delay_on || ctlr->leds[i].blink_delay_off)
> +                       flash |= 1 << i;
> +               else if (ctlr->leds[i].brightness)
> +                       val |= 1 << i;
> +       }
>
>         mutex_lock(&ctlr->output_mutex);
> -       ret = joycon_set_player_leds(ctlr, 0, val);
> +       ret = joycon_set_player_leds(ctlr, flash, val);
>         mutex_unlock(&ctlr->output_mutex);
>
>         return ret;
>  }
>
> +static int joycon_player_led_brightness_set(struct led_classdev *led,
> +                                           enum led_brightness brightness)
> +{
> +       led->brightness = brightness;
> +
> +       if (!brightness)
> +               led->blink_delay_on = led->blink_delay_off = 0;
> +
> +       return joycon_update_player_leds(led->dev->parent);
> +}
> +
> +/* the blink period of the leds can't be changed, and is always these values */
> +static const JC_LED_BLINK_DELAY_ON = 500;
> +static const JC_LED_BLINK_DELAY_OFF = 200;

nit: it might be nice to append the constant names with "_MS" for
clarity on the time unit used.


> +/* the different leds on a joycon can't actually be set to hw blink independently
> + * of each other, since they all use the same one subcommand, so this function
> + * actually resets the cycle of all the leds */
> +static int joycon_player_led_blink_set(struct led_classdev *led,
> +                                    unsigned long *delay_on,
> +                                    unsigned long *delay_off)
> +{
> +       led->blink_delay_on = *delay_on = JC_LED_BLINK_DELAY_ON;
> +       led->blink_delay_off = *delay_off = JC_LED_BLINK_DELAY_OFF;
> +
> +       return joycon_update_player_leds(led->dev->parent);
> +}
> +
>  static int joycon_home_led_brightness_set(struct led_classdev *led,
>                                           enum led_brightness brightness)
>  {
> @@ -2268,6 +2298,7 @@ static int joycon_leds_create(struct joycon_ctlr *ctlr)
>                 led->max_brightness = 1;
>                 led->brightness_set_blocking =
>                                         joycon_player_led_brightness_set;
> +               led->blink_set = joycon_player_led_blink_set;
>                 led->flags = LED_CORE_SUSPENDRESUME | LED_HW_PLUGGABLE;
>
>                 led_val |= joycon_player_led_patterns[player_led_pattern][i] << i;
> --
> 2.48.1
>


Looks good overall to me other than the commit message format.

Thanks,
Daniel

[PATCH] HID: nintendo: enable HW LED blinking

[Noa]

Updated with proper commit title and clearer constant names.

Signed-off-by: Noa <coolreader18@gmail.com>
---
 drivers/hid/hid-nintendo.c | 45 ++++++++++++++++++++++++++++++++------
 1 file changed, 38 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
index 11ac246176ae..f4cdd35eef2a 100644
--- a/drivers/hid/hid-nintendo.c
+++ b/drivers/hid/hid-nintendo.c
@@ -2183,14 +2183,13 @@ 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)
+/* Update the on/flash status of the leds according to their led_classdev fields */
+static int joycon_update_player_leds(struct device *dev)
 {
-	struct device *dev = led->dev->parent;
 	struct hid_device *hdev = to_hid_device(dev);
 	struct joycon_ctlr *ctlr;
 	int val = 0;
+	int flash = 0;
 	int i;
 	int ret;
 
@@ -2200,16 +2199,47 @@ static int joycon_player_led_brightness_set(struct led_classdev *led,
 		return -ENODEV;
 	}
 
-	for (i = 0; i < JC_NUM_LEDS; i++)
-		val |= ctlr->leds[i].brightness << i;
+	for (i = 0; i < JC_NUM_LEDS; i++) {
+		if (ctlr->leds[i].blink_delay_on || ctlr->leds[i].blink_delay_off)
+			flash |= 1 << i;
+		else if (ctlr->leds[i].brightness)
+			val |= 1 << i;
+	}
 
 	mutex_lock(&ctlr->output_mutex);
-	ret = joycon_set_player_leds(ctlr, 0, val);
+	ret = joycon_set_player_leds(ctlr, flash, val);
 	mutex_unlock(&ctlr->output_mutex);
 
 	return ret;
 }
 
+static int joycon_player_led_brightness_set(struct led_classdev *led,
+					    enum led_brightness brightness)
+{
+	led->brightness = brightness;
+
+	if (!brightness)
+		led->blink_delay_on = led->blink_delay_off = 0;
+
+	return joycon_update_player_leds(led->dev->parent);
+}
+
+/* the blink period of the leds can't be changed, and is always these values */
+static const JC_LED_BLINK_DELAY_ON_MS = 500;
+static const JC_LED_BLINK_DELAY_OFF_MS = 200;
+/* the different leds on a joycon can't actually be set to hw blink independently
+ * of each other, since they all use the same one subcommand, so this function
+ * actually resets the cycle of all the leds */
+static int joycon_player_led_blink_set(struct led_classdev *led,
+				     unsigned long *delay_on,
+				     unsigned long *delay_off)
+{
+	led->blink_delay_on = *delay_on = JC_LED_BLINK_DELAY_ON_MS;
+	led->blink_delay_off = *delay_off = JC_LED_BLINK_DELAY_OFF_MS;
+
+	return joycon_update_player_leds(led->dev->parent);
+}
+
 static int joycon_home_led_brightness_set(struct led_classdev *led,
 					  enum led_brightness brightness)
 {
@@ -2268,6 +2298,7 @@ static int joycon_leds_create(struct joycon_ctlr *ctlr)
 		led->max_brightness = 1;
 		led->brightness_set_blocking =
 					joycon_player_led_brightness_set;
+		led->blink_set = joycon_player_led_blink_set;
 		led->flags = LED_CORE_SUSPENDRESUME | LED_HW_PLUGGABLE;
 
 		led_val |= joycon_player_led_patterns[player_led_pattern][i] << i;
-- 
2.48.1

Re: [PATCH] HID: nintendo: enable HW LED blinking

[Daniel Ogorchock]

Hi Noa,

I tried building with this version of the patch and hit the following error:

drivers/hid/hid-nintendo.c:2229:14: error: type defaults to ‘int’ in
declaration of ‘JC_LED_BLINK_DELAY_ON_MS’ [-Wimplicit-int]
 2229 | static const JC_LED_BLINK_DELAY_ON_MS = 500;
      |              ^~~~~~~~~~~~~~~~~~~~~~~~
drivers/hid/hid-nintendo.c:2230:14: error: type defaults to ‘int’ in
declaration of ‘JC_LED_BLINK_DELAY_OFF_MS’ [-Wimplicit-int]
 2230 | static const JC_LED_BLINK_DELAY_OFF_MS = 200;
      |              ^~~~~~~~~~~~~~~~~~~~~~~~~
make[4]: *** [scripts/Makefile.build:287: drivers/hid/hid-nintendo.o] Error 1
make[3]: *** [scripts/Makefile.build:555: drivers/hid] Error 2

Making these 'static const unsigned long' fixed that issue. Probably
toolchain version dependent whether it succeeds or fails with the
implicit int.

I also noticed that the HW blink functionality only works in bluetooth
mode. In USB mode it forces the LEDs on.
That seems to match up with the note here:
https://github.com/dekuNukem/Nintendo_Switch_Reverse_Engineering/blob/master/bluetooth_hid_subcommands_notes.md#subcommand-0x30-set-player-lights


I was able to modify your patch to gate the new HW logic to only be
active when using bluetooth.

Also please include the commit summary message/description above your
signed-off-by line like you did in v1 of the patch.

Below is my quick modification to your patch that seemed to get things
working right on my local setup. Please feel free to
incorporate it or make your own modifications in v3 to fix the USB
mode. It's probably also worth adding a comment describing
why it doesn't use the HW support for USB:

---
 drivers/hid/hid-nintendo.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
index 14aed2500a96..03f24e709949 100644
--- a/drivers/hid/hid-nintendo.c
+++ b/drivers/hid/hid-nintendo.c
@@ -2201,7 +2201,8 @@ static int joycon_update_player_leds(struct device *dev)
     }

     for (i = 0; i < JC_NUM_LEDS; i++) {
-        if (ctlr->leds[i].blink_delay_on || ctlr->leds[i].blink_delay_off)
+        if ((ctlr->leds[i].blink_delay_on || ctlr->leds[i].blink_delay_off) &&
+            !joycon_using_usb(ctlr))
             flash |= 1 << i;
         else if (ctlr->leds[i].brightness)
             val |= 1 << i;
@@ -2217,17 +2218,26 @@ static int joycon_update_player_leds(struct device *dev)
 static int joycon_player_led_brightness_set(struct led_classdev *led,
                         enum led_brightness brightness)
 {
+    struct hid_device *hdev = to_hid_device(led->dev->parent);
+    struct joycon_ctlr *ctlr;
+
+    ctlr = hid_get_drvdata(hdev);
+    if (!ctlr) {
+        hid_err(hdev, "No controller data\n");
+        return -ENODEV;
+    }
+
     led->brightness = brightness;

-    if (!brightness)
+    if (!brightness && !joycon_using_usb(ctlr))
         led->blink_delay_on = led->blink_delay_off = 0;

     return joycon_update_player_leds(led->dev->parent);
 }

 /* the blink period of the leds can't be changed, and is always these values */
-static const JC_LED_BLINK_DELAY_ON_MS = 500;
-static const JC_LED_BLINK_DELAY_OFF_MS = 200;
+static const unsigned long JC_LED_BLINK_DELAY_ON_MS = 500;
+static const unsigned long JC_LED_BLINK_DELAY_OFF_MS = 200;
 /* the different leds on a joycon can't actually be set to hw blink
independently
  * of each other, since they all use the same one subcommand, so this function
  * actually resets the cycle of all the leds */
@@ -2299,7 +2309,8 @@ static int joycon_leds_create(struct joycon_ctlr *ctlr)
         led->max_brightness = 1;
         led->brightness_set_blocking =
                     joycon_player_led_brightness_set;
-        led->blink_set = joycon_player_led_blink_set;
+        if (!joycon_using_usb(ctlr))
+            led->blink_set = joycon_player_led_blink_set;
         led->flags = LED_CORE_SUSPENDRESUME | LED_HW_PLUGGABLE;

         led_val |= joycon_player_led_patterns[player_led_pattern][i] << i;
-- 
2.51.0



On Fri, Feb 14, 2025 at 5:58 PM Noa <coolreader18@gmail.com> wrote:
>
> Updated with proper commit title and clearer constant names.
>
> Signed-off-by: Noa <coolreader18@gmail.com>
> ---
>  drivers/hid/hid-nintendo.c | 45 ++++++++++++++++++++++++++++++++------
>  1 file changed, 38 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> index 11ac246176ae..f4cdd35eef2a 100644
> --- a/drivers/hid/hid-nintendo.c
> +++ b/drivers/hid/hid-nintendo.c
> @@ -2183,14 +2183,13 @@ 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)
> +/* Update the on/flash status of the leds according to their led_classdev fields */
> +static int joycon_update_player_leds(struct device *dev)
>  {
> -       struct device *dev = led->dev->parent;
>         struct hid_device *hdev = to_hid_device(dev);
>         struct joycon_ctlr *ctlr;
>         int val = 0;
> +       int flash = 0;
>         int i;
>         int ret;
>
> @@ -2200,16 +2199,47 @@ static int joycon_player_led_brightness_set(struct led_classdev *led,
>                 return -ENODEV;
>         }
>
> -       for (i = 0; i < JC_NUM_LEDS; i++)
> -               val |= ctlr->leds[i].brightness << i;
> +       for (i = 0; i < JC_NUM_LEDS; i++) {
> +               if (ctlr->leds[i].blink_delay_on || ctlr->leds[i].blink_delay_off)
> +                       flash |= 1 << i;
> +               else if (ctlr->leds[i].brightness)
> +                       val |= 1 << i;
> +       }
>
>         mutex_lock(&ctlr->output_mutex);
> -       ret = joycon_set_player_leds(ctlr, 0, val);
> +       ret = joycon_set_player_leds(ctlr, flash, val);
>         mutex_unlock(&ctlr->output_mutex);
>
>         return ret;
>  }
>
> +static int joycon_player_led_brightness_set(struct led_classdev *led,
> +                                           enum led_brightness brightness)
> +{
> +       led->brightness = brightness;
> +
> +       if (!brightness)
> +               led->blink_delay_on = led->blink_delay_off = 0;
> +
> +       return joycon_update_player_leds(led->dev->parent);
> +}
> +
> +/* the blink period of the leds can't be changed, and is always these values */
> +static const JC_LED_BLINK_DELAY_ON_MS = 500;
> +static const JC_LED_BLINK_DELAY_OFF_MS = 200;
> +/* the different leds on a joycon can't actually be set to hw blink independently
> + * of each other, since they all use the same one subcommand, so this function
> + * actually resets the cycle of all the leds */
> +static int joycon_player_led_blink_set(struct led_classdev *led,
> +                                    unsigned long *delay_on,
> +                                    unsigned long *delay_off)
> +{
> +       led->blink_delay_on = *delay_on = JC_LED_BLINK_DELAY_ON_MS;
> +       led->blink_delay_off = *delay_off = JC_LED_BLINK_DELAY_OFF_MS;
> +
> +       return joycon_update_player_leds(led->dev->parent);
> +}
> +
>  static int joycon_home_led_brightness_set(struct led_classdev *led,
>                                           enum led_brightness brightness)
>  {
> @@ -2268,6 +2298,7 @@ static int joycon_leds_create(struct joycon_ctlr *ctlr)
>                 led->max_brightness = 1;
>                 led->brightness_set_blocking =
>                                         joycon_player_led_brightness_set;
> +               led->blink_set = joycon_player_led_blink_set;
>                 led->flags = LED_CORE_SUSPENDRESUME | LED_HW_PLUGGABLE;
>
>                 led_val |= joycon_player_led_patterns[player_led_pattern][i] << i;
> --
> 2.48.1
>

Thanks,
Daniel