| Message ID | 20210520224715.680919-3-djogorchock@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: nintendo | expand |
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
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 --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);
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(-)