Re: [PATCH v13 02/15] HID: nintendo: add player led support

Hi Barnabás and Daniel,

I agree with Barnabás in regards to LED naming. Earlier this year for
the new "hid-playstation" driver we ran into issues in regards to the
naming scheme of LEDs moving forward (see some of the archived
threads). I still have to address this for "hid-playstation", but
didn't get to submitting the changes yet due to paternity leave. I did
implement a proof-of-concept, which I can probably clean up over the
next few days.

Basically the LED team would like LEDs to follow the naming scheme
"devicename:color:function" instead of legacy names containing mac
addresses or other strings.

There are 2 issues. One is the "devicename", where there is not
necessarily a clear mapping in particular for device with multiple
functions such as DualShock 4 / DualSense / Joycons. The suggested
name was to use the 'hid device name' something with the bus, device
etcetera numbers in there. I need to check the email thread on what
Benjamin suggested there.

The second issue is the "function" part of the the naming scheme. The
current official naming is like "LED_FUNCTION_DISK" and others as
defined by "include/dt-bindings/leds/common.h". I was proposing to
have a new player type "LED_FUNCTION_PLAYER", which is what the
Nintendo controller would need as well.

I will try to clean up PoC version of my patches and include you on
the thread as well.

Thanks,
Roderick

On Thu, May 20, 2021 at 4:35 PM Barnabás Pőcze [off-list ref] wrote:

Hi

please cc the linux-leds mailing list and the appropriate subsystem maintainers
at least on the relevant patches.


2021. május 21., péntek 0:47 keltezéssel, Daniel J. Ogorchock írta:

quoted

This patch adds led_classdev functionality to the switch controller
driver. It adds support for the 4 player LEDs. The Home Button LED still
needs to be supported on the pro controllers and right joy-con.

Signed-off-by: Daniel J. Ogorchock <djogorchock@gmail.com>
---
 drivers/hid/Kconfig        |  2 +
 drivers/hid/hid-nintendo.c | 95 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 95 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 1f23e84f8bdf3..86a6129d3d89f 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig

@@ -722,6 +722,8 @@ config HID_MULTITOUCH
 config HID_NINTENDO
      tristate "Nintendo Joy-Con and Pro Controller support"
      depends on HID
+     depends on NEW_LEDS
+     depends on LEDS_CLASS
      help
      Adds support for the Nintendo Switch Joy-Cons and Pro Controller.
      All controllers support bluetooth, and the Pro Controller also supports

diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
index b6c0e5e36d8b0..c21682b4a2e62 100644
--- a/drivers/hid/hid-nintendo.c
+++ b/drivers/hid/hid-nintendo.c

@@ -25,6 +25,7 @@
 #include <linux/device.h>
 #include <linux/hid.h>
 #include <linux/input.h>
+#include <linux/leds.h>
 #include <linux/module.h>
 #include <linux/spinlock.h>

@@ -183,11 +184,13 @@ struct joycon_input_report {
 } __packed;

 #define JC_MAX_RESP_SIZE     (sizeof(struct joycon_input_report) + 35)
+#define JC_NUM_LEDS          4

I think it'd be better if you could guarantee that `JC_NUM_LEDS` and
size of the `joycon_player_led_names` won't go out of sync. E.g.
define `JC_NUM_LEDS` in terms of ARRAY_SIZE(), use static_assert(), etc.

quoted

 /* Each physical controller is associated with a joycon_ctlr struct */
 struct joycon_ctlr {
      struct hid_device *hdev;
      struct input_dev *input;
+     struct led_classdev leds[JC_NUM_LEDS];
      enum joycon_ctlr_state ctlr_state;

      /* The following members are used for synchronous sends/receives */

@@ -553,11 +556,9 @@ static const unsigned int joycon_dpad_inputs_jc[] = {
      BTN_DPAD_UP, BTN_DPAD_DOWN, BTN_DPAD_LEFT, BTN_DPAD_RIGHT,
 };

-static DEFINE_MUTEX(joycon_input_num_mutex);
 static int joycon_input_create(struct joycon_ctlr *ctlr)
 {
      struct hid_device *hdev;
-     static int input_num = 1;
      const char *name;
      int ret;
      int i;

@@ -643,6 +644,66 @@ static int joycon_input_create(struct joycon_ctlr *ctlr)
      if (ret)
              return ret;

+     return 0;
+}
+
+static int joycon_player_led_brightness_set(struct led_classdev *led,
+                                         enum led_brightness brightness)
+{
+     struct device *dev = led->dev->parent;
+     struct hid_device *hdev = to_hid_device(dev);
+     struct joycon_ctlr *ctlr;
+     int val = 0;
+     int i;
+     int ret;
+     int num;
+
+     ctlr = hid_get_drvdata(hdev);
+     if (!ctlr) {
+             hid_err(hdev, "No controller data\n");
+             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;
+
+     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);
+
+     return ret;
+}
+
+static const char * const joycon_player_led_names[] = {
+     "player1",
+     "player2",
+     "player3",
+     "player4"

Small thing: after non-sentinel values at the end the comma is usually not omitted.

quoted

+};
+
+static DEFINE_MUTEX(joycon_input_num_mutex);
+static int joycon_player_leds_create(struct joycon_ctlr *ctlr)
+{
+     struct hid_device *hdev = ctlr->hdev;
+     struct device *dev = &hdev->dev;
+     const char *d_name = dev_name(dev);
+     struct led_classdev *led;
+     char *name;
+     int ret = 0;
+     int i;
+     static int input_num = 1;
+
      /* Set the default controller player leds based on controller number */
      mutex_lock(&joycon_input_num_mutex);
      mutex_lock(&ctlr->output_mutex);

@@ -650,6 +711,29 @@ static int joycon_input_create(struct joycon_ctlr *ctlr)
      if (ret)
              hid_warn(ctlr->hdev, "Failed to set leds; ret=%d\n", ret);
      mutex_unlock(&ctlr->output_mutex);
+
+     /* configure the player LEDs */
+     for (i = 0; i < JC_NUM_LEDS; i++) {
+             name = devm_kasprintf(dev, GFP_KERNEL, "%s:%s", d_name,

This does not seem to be an appropriate name for an LED class device.
See Documentation/leds/leds-class.rst.

quoted

+                                   joycon_player_led_names[i]);
+             if (!name)
+                     return -ENOMEM;
+
+             led = &ctlr->leds[i];
+             led->name = name;
+             led->brightness = ((i + 1) <= input_num) ? LED_ON : LED_OFF;
+             led->max_brightness = LED_ON;

LED_{ON,OFF,...} constants have been deprecated to the best of my knowledge,
use integer values as appropriate.

quoted

+             led->brightness_set_blocking =
+                                     joycon_player_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 %s LED\n", led->name);
+                     break;
+             }
+     }
+
      if (++input_num > 4)
              input_num = 1;
      mutex_unlock(&joycon_input_num_mutex);

@@ -813,6 +897,13 @@ static int nintendo_hid_probe(struct hid_device *hdev,

      mutex_unlock(&ctlr->output_mutex);

+     /* Initialize the leds */
+     ret = joycon_player_leds_create(ctlr);
+     if (ret) {
+             hid_err(hdev, "Failed to create leds; ret=%d\n", ret);
+             goto err_close;
+     }
+
      ret = joycon_input_create(ctlr);
      if (ret) {
              hid_err(hdev, "Failed to create input device; ret=%d\n", ret);
--

2.31.1

Regards,
Barnabás Pőcze