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
