| Message ID | 20230712121433.26134-1-tinozzo123@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | HID: nintendo: cleanup LED code | expand |
Hi! Some comments inline below. 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 (still not a complete match, see https://bugzilla.kernel.org/show_bug.cgi?id=216225) > - 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 > - 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 player LEDs fails, still attempt setting the home LED (I don't know if this is actually possible, but who knows...) > - Use `JC_NUM_LEDS` when appropriate instead of 4 > - Print return codes Wrapping the git commit message around 78 chars or so is conventional. > Signed-off-by: Martino Fontana <tinozzo123@gmail.com> > --- > drivers/hid/hid-nintendo.c | 116 ++++++++++++++++++------------------- > 1 file changed, 55 insertions(+), 61 deletions(-) > > diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c > index 586c7f8d7..89631e19b 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,33 @@ 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; 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); > - return ret; > - } > + if (ret) > + hid_err(hdev, "Failed registering %s LED; ret=%d\n", led->name, 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,17 +1971,18 @@ 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); > - if (ret) { > - hid_err(hdev, "Failed registering home led\n"); > - return ret; > - } > + > /* Set the home LED to 0 as default state */ > - ret = joycon_home_led_brightness_set(led, 0); > + mutex_lock(&ctlr->output_mutex); > + ret = joycon_set_home_led(ctlr, 0); > + mutex_unlock(&ctlr->output_mutex); We shouldn't use any spaces for indentation here. > if (ret) { > - hid_warn(hdev, "Failed to set home LED default, unregistering home LED"); > - devm_led_classdev_unregister(&hdev->dev, led); > + hid_warn(hdev, "Failed to set home LED; ret=%d\n", ret); > + return 0; > } Adding an empty line here would be more consistent with the rest of the code. > + ret = devm_led_classdev_register(&hdev->dev, led); > + if (ret) > + hid_err(hdev, "Failed registering home led; ret=%d\n", ret); In the old code we returned "ret" in this case. We probably want to do the same here. Cheers, Silvan > } > > return 0;
The reason I avoid returning `ret` is that, if something other than 0 is returned, then `nintendo_hid_probe` (the function calling `joycon_leds_create`) would fail completely. That obviously shouldn't happen, LEDs aren't a vital piece for the gamepad functionality. Alternatively, instead of avoiding returning anything other than 0, `nintendo_hid_probe` could be modified by changing `if (ret)` in that section to something else. I'm not an expert in C, so... opinions? Il giorno gio 20 lug 2023 alle ore 22:04 Silvan Jegen <s.jegen@gmail.com> ha scritto: > > Hi! > > Some comments inline below. > > 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 (still not a complete match, see https://bugzilla.kernel.org/show_bug.cgi?id=216225) > > - 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 > > - 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 player LEDs fails, still attempt setting the home LED (I don't know if this is actually possible, but who knows...) > > - Use `JC_NUM_LEDS` when appropriate instead of 4 > > - Print return codes > > Wrapping the git commit message around 78 chars or so is conventional. > > > > Signed-off-by: Martino Fontana <tinozzo123@gmail.com> > > --- > > drivers/hid/hid-nintendo.c | 116 ++++++++++++++++++------------------- > > 1 file changed, 55 insertions(+), 61 deletions(-) > > > > diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c > > index 586c7f8d7..89631e19b 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,33 @@ 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; 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); > > - return ret; > > - } > > + if (ret) > > + hid_err(hdev, "Failed registering %s LED; ret=%d\n", led->name, 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,17 +1971,18 @@ 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); > > - if (ret) { > > - hid_err(hdev, "Failed registering home led\n"); > > - return ret; > > - } > > + > > /* Set the home LED to 0 as default state */ > > - ret = joycon_home_led_brightness_set(led, 0); > > + mutex_lock(&ctlr->output_mutex); > > + ret = joycon_set_home_led(ctlr, 0); > > + mutex_unlock(&ctlr->output_mutex); > > We shouldn't use any spaces for indentation here. > > > if (ret) { > > - hid_warn(hdev, "Failed to set home LED default, unregistering home LED"); > > - devm_led_classdev_unregister(&hdev->dev, led); > > + hid_warn(hdev, "Failed to set home LED; ret=%d\n", ret); > > + return 0; > > } > > Adding an empty line here would be more consistent with the rest of > the code. > > > + ret = devm_led_classdev_register(&hdev->dev, led); > > + if (ret) > > + hid_err(hdev, "Failed registering home led; ret=%d\n", ret); > > In the old code we returned "ret" in this case. We probably want to do > the same here. > > Cheers, > Silvan > > > } > > > > return 0; > >
Hi Martino Martino Fontana <tinozzo123@gmail.com> wrote: > The reason I avoid returning `ret` is that, if something other than 0 > is returned, then `nintendo_hid_probe` (the function calling > `joycon_leds_create`) would fail completely. That obviously shouldn't > happen, LEDs aren't a vital piece for the gamepad functionality. While this might be true, if this fails it would indicate a potential bug in the driver. Failing loudly in this case (especially if this was the behavior before your changes) would lead to this bug being investigated earlier. If we just ignore the failure, people using the code may not be aware that the LEDs were supposed to work but failed for some reason. In that case we may never get a bug report and the LED creation would stay broken in this case. That's why I would prefer to fail the probe as well if the LED creation fails. This is just my opinion though. Cheers, Silvan > Alternatively, instead of avoiding returning anything other than 0, > `nintendo_hid_probe` could be modified by changing `if (ret)` in that > section to something else. > I'm not an expert in C, so... opinions? > > Il giorno gio 20 lug 2023 alle ore 22:04 Silvan Jegen > <s.jegen@gmail.com> ha scritto: > > > > Hi! > > > > Some comments inline below. > > > > 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 (still not a complete match, see https://bugzilla.kernel.org/show_bug.cgi?id=216225) > > > - 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 > > > - 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 player LEDs fails, still attempt setting the home LED (I don't know if this is actually possible, but who knows...) > > > - Use `JC_NUM_LEDS` when appropriate instead of 4 > > > - Print return codes > > > > Wrapping the git commit message around 78 chars or so is conventional. > > > > > > > Signed-off-by: Martino Fontana <tinozzo123@gmail.com> > > > --- > > > drivers/hid/hid-nintendo.c | 116 ++++++++++++++++++------------------- > > > 1 file changed, 55 insertions(+), 61 deletions(-) > > > > > > diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c > > > index 586c7f8d7..89631e19b 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,33 @@ 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; 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); > > > - return ret; > > > - } > > > + if (ret) > > > + hid_err(hdev, "Failed registering %s LED; ret=%d\n", led->name, 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,17 +1971,18 @@ 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); > > > - if (ret) { > > > - hid_err(hdev, "Failed registering home led\n"); > > > - return ret; > > > - } > > > + > > > /* Set the home LED to 0 as default state */ > > > - ret = joycon_home_led_brightness_set(led, 0); > > > + mutex_lock(&ctlr->output_mutex); > > > + ret = joycon_set_home_led(ctlr, 0); > > > + mutex_unlock(&ctlr->output_mutex); > > > > We shouldn't use any spaces for indentation here. > > > > > if (ret) { > > > - hid_warn(hdev, "Failed to set home LED default, unregistering home LED"); > > > - devm_led_classdev_unregister(&hdev->dev, led); > > > + hid_warn(hdev, "Failed to set home LED; ret=%d\n", ret); > > > + return 0; > > > } > > > > Adding an empty line here would be more consistent with the rest of > > the code. > > > > > + ret = devm_led_classdev_register(&hdev->dev, led); > > > + if (ret) > > > + hid_err(hdev, "Failed registering home led; ret=%d\n", ret); > > > > In the old code we returned "ret" in this case. We probably want to do > > the same here. > > > > Cheers, > > Silvan > > > > > } > > > > > > return 0; > > > >
diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c index 586c7f8d7..89631e19b 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,33 @@ 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; 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); - return ret; - } + if (ret) + hid_err(hdev, "Failed registering %s LED; ret=%d\n", led->name, 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,17 +1971,18 @@ 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); - if (ret) { - hid_err(hdev, "Failed registering home led\n"); - return ret; - } + /* Set the home LED to 0 as default state */ - ret = joycon_home_led_brightness_set(led, 0); + mutex_lock(&ctlr->output_mutex); + ret = joycon_set_home_led(ctlr, 0); + mutex_unlock(&ctlr->output_mutex); if (ret) { - hid_warn(hdev, "Failed to set home LED default, unregistering home LED"); - devm_led_classdev_unregister(&hdev->dev, led); + hid_warn(hdev, "Failed to set home LED; ret=%d\n", ret); + return 0; } + ret = devm_led_classdev_register(&hdev->dev, led); + if (ret) + hid_err(hdev, "Failed registering home led; ret=%d\n", ret); } return 0;
- 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 (still not a complete match, see https://bugzilla.kernel.org/show_bug.cgi?id=216225) - 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 - 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 player LEDs fails, still attempt setting the home LED (I don't know if this is actually possible, but who knows...) - Use `JC_NUM_LEDS` when appropriate instead of 4 - Print return codes Signed-off-by: Martino Fontana <tinozzo123@gmail.com> --- drivers/hid/hid-nintendo.c | 116 ++++++++++++++++++------------------- 1 file changed, 55 insertions(+), 61 deletions(-)