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@xxxxxxxxx> 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@xxxxxxxxx>
> ---
>  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