* [PATCH v4] HID: nintendo: cleanup LED code
@ 2023-09-07 9:49 Martino Fontana
2023-09-08 1:22 ` Daniel Ogorchock
0 siblings, 1 reply; 3+ messages in thread
From: Martino Fontana @ 2023-09-07 9:49 UTC (permalink / raw)
To: djogorchock, jikos, benjamin.tissoires, linux-input, linux-kernel
Cc: Martino Fontana
- Only turn on the nth LED, instead of all the LEDs up to n. This better
matches Nintendo consoles' behaviour, as they never turn on more than
one LED.
(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.
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.
Signed-off-by: Martino Fontana <tinozzo123@gmail.com>
---
drivers/hid/hid-nintendo.c | 113 ++++++++++++++++++-------------------
1 file changed, 56 insertions(+), 57 deletions(-)
diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
index a5ebe857a..543098a8c 100644
--- a/drivers/hid/hid-nintendo.c
+++ b/drivers/hid/hid-nintendo.c
@@ -699,6 +699,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)
{
@@ -1849,7 +1868,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 +1875,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 +1891,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,25 +1898,13 @@ 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;
@@ -1920,17 +1912,16 @@ static int joycon_leds_create(struct joycon_ctlr *ctlr)
const char *d_name = dev_name(dev);
struct led_classdev *led;
char *name;
- int ret = 0;
+ int ret;
int i;
- static int input_num = 1;
+ unsigned long flags;
+ int player_led_number;
+ 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 */
+ spin_lock_irqsave(&joycon_input_num_spinlock, flags);
+ player_led_number = input_num++ % JC_NUM_LEDS;
+ spin_unlock_irqrestore(&joycon_input_num_spinlock, flags);
/* configure the player LEDs */
for (i = 0; i < JC_NUM_LEDS; i++) {
@@ -1938,31 +1929,35 @@ 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 = (i == player_led_number) ? 1 : 0;
led->max_brightness = 1;
led->brightness_set_blocking =
joycon_player_led_brightness_set;
led->flags = LED_CORE_SUSPENDRESUME | LED_HW_PLUGGABLE;
+ }
+ mutex_lock(&ctlr->output_mutex);
+ ret = joycon_set_player_leds(ctlr, 0, 0x1 << player_led_number);
+ 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 +1973,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.41.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v4] HID: nintendo: cleanup LED code
2023-09-07 9:49 [PATCH v4] HID: nintendo: cleanup LED code Martino Fontana
@ 2023-09-08 1:22 ` Daniel Ogorchock
2023-09-10 10:34 ` Martino Fontana
0 siblings, 1 reply; 3+ messages in thread
From: Daniel Ogorchock @ 2023-09-08 1:22 UTC (permalink / raw)
To: Martino Fontana; +Cc: jikos, benjamin.tissoires, linux-input, linux-kernel
On Thu, Sep 7, 2023 at 5:51 AM Martino Fontana <tinozzo123@gmail.com> wrote:
>
> - Only turn on the nth LED, instead of all the LEDs up to n. This better
> matches Nintendo consoles' behaviour, as they never turn on more than
> one LED.
> (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.
>
> 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.
>
> Signed-off-by: Martino Fontana <tinozzo123@gmail.com>
Hi Martino,
This cleanup looks pretty good to me.
Regarding the player LED setting though, I'm pretty sure the driver's
existing behavior matches that of an actual Switch console (at least
for the first 4 players).
1 LED lights up for P1, 2 for P2, etc.
See the nintendo documentation here:
https://www.nintendo.com/my/support/qa/detail/33822
Thanks,
Daniel
> ---
> drivers/hid/hid-nintendo.c | 113 ++++++++++++++++++-------------------
> 1 file changed, 56 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> index a5ebe857a..543098a8c 100644
> --- a/drivers/hid/hid-nintendo.c
> +++ b/drivers/hid/hid-nintendo.c
> @@ -699,6 +699,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)
> {
> @@ -1849,7 +1868,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 +1875,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 +1891,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,25 +1898,13 @@ 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;
> @@ -1920,17 +1912,16 @@ static int joycon_leds_create(struct joycon_ctlr *ctlr)
> const char *d_name = dev_name(dev);
> struct led_classdev *led;
> char *name;
> - int ret = 0;
> + int ret;
> int i;
> - static int input_num = 1;
> + unsigned long flags;
> + int player_led_number;
> + 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 */
> + spin_lock_irqsave(&joycon_input_num_spinlock, flags);
> + player_led_number = input_num++ % JC_NUM_LEDS;
> + spin_unlock_irqrestore(&joycon_input_num_spinlock, flags);
>
> /* configure the player LEDs */
> for (i = 0; i < JC_NUM_LEDS; i++) {
> @@ -1938,31 +1929,35 @@ 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 = (i == player_led_number) ? 1 : 0;
> led->max_brightness = 1;
> led->brightness_set_blocking =
> joycon_player_led_brightness_set;
> led->flags = LED_CORE_SUSPENDRESUME | LED_HW_PLUGGABLE;
> + }
> + mutex_lock(&ctlr->output_mutex);
> + ret = joycon_set_player_leds(ctlr, 0, 0x1 << player_led_number);
> + 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 +1973,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.41.0
>
--
Daniel Ogorchock
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v4] HID: nintendo: cleanup LED code
2023-09-08 1:22 ` Daniel Ogorchock
@ 2023-09-10 10:34 ` Martino Fontana
0 siblings, 0 replies; 3+ messages in thread
From: Martino Fontana @ 2023-09-10 10:34 UTC (permalink / raw)
To: Daniel Ogorchock; +Cc: jikos, benjamin.tissoires, linux-input, linux-kernel
On Fri, 8 Sept 2023 at 03:22, Daniel Ogorchock <djogorchock@gmail.com> wrote:
> Regarding the player LED setting though, I'm pretty sure the driver's
> existing behavior matches that of an actual Switch console (at least
> for the first 4 players).
>
> 1 LED lights up for P1, 2 for P2, etc.
> See the nintendo documentation here:
> https://www.nintendo.com/my/support/qa/detail/33822
Whoops, I assumed that it behaves like it does on Wii and Wii U.
Sending new patch.
Regards,
Martino.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-09-10 10:35 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-07 9:49 [PATCH v4] HID: nintendo: cleanup LED code Martino Fontana
2023-09-08 1:22 ` Daniel Ogorchock
2023-09-10 10:34 ` Martino Fontana
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).