diff mbox series

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

Message ID 20210520224715.680919-3-djogorchock@gmail.com (mailing list archive)
State New
Delegated to: Jiri Kosina
Headers show
Series HID: nintendo | expand

Commit Message

Daniel J. Ogorchock May 20, 2021, 10:47 p.m. UTC
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(-)

Comments

Barnabás Pőcze May 20, 2021, 11:35 p.m. UTC | #1
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:

> 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.


>
>  /* 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.


> +};
> +
> +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.


> +				      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.


> +		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
Roderick Colenbrander May 21, 2021, 12:26 a.m. UTC | #2
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 <pobrn@protonmail.com> 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:
>
> > 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.
>
>
> >
> >  /* 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.
>
>
> > +};
> > +
> > +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.
>
>
> > +                                   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.
>
>
> > +             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
diff mbox series

Patch

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
 
 /* 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"
+};
+
+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,
+				      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->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);