| Message ID | 20210520224715.680919-1-djogorchock@gmail.com |
|---|---|
| Headers | show |
| Series | HID: nintendo | expand |
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
On Thu, 20 May 2021, Daniel J. Ogorchock wrote: > I've neglected updating this for awhile. Not much has changed in this > revision. It has a couple bug fixes caught in the prior rev in addition > to altering how rumble is handled I have gone through this series, and haven't found anything outstanding, so unless anyone voices any objections immediately, I am planning to queue it for 5.15. Thanks, -- Jiri Kosina SUSE Labs
Hi Jiri, From my perspective the driver looked fine. For me the only thing is the LED naming for the player LEDs and any other LEDs. We are going through the same thing now for hid-playstation and need the blessing of our the LED maintainers. Thanks, Roderick On Thu, Jul 15, 2021 at 11:53 AM Jiri Kosina <jikos@kernel.org> wrote: > > On Thu, 20 May 2021, Daniel J. Ogorchock wrote: > > > I've neglected updating this for awhile. Not much has changed in this > > revision. It has a couple bug fixes caught in the prior rev in addition > > to altering how rumble is handled > > I have gone through this series, and haven't found anything outstanding, > so unless anyone voices any objections immediately, I am planning to queue > it for 5.15. > > Thanks, > > -- > Jiri Kosina > SUSE Labs >
I've neglected updating this for awhile. Not much has changed in this revision. It has a couple bug fixes caught in the prior rev in addition to altering how rumble is handled Version 13 changes: - Switched to using the dedicated rumble data message type, rather than constantly resending the rumble enabled subcommand. This more closely resembles how the console itself handles rumble data. - Applied revisions based on Silvan Jegen's feedback on v12. Version 12 changes: - Added support for reading user calibration from the controller's SPI flash (written when someone calibrates the controller on the Nintendo switch). - Added patch to prevent sending rumble subcommands when no effect is being played. This turned out to drastically improve bluetooth connection reliability. - Set the battery description to POWER_SUPPLY_TYPE_BATTERY (was missing in previous revisions due to oversight). This fixes problems with desktop environments not handling the controller batteries properly. - Reintroduced IMU patch with improvements to documentation, packet drop handling, and increased precision for gyro readings. Also now blacklists the IMU input dev from joydev like hid-sony. Version 11 changes: - Removed IMU patch for now, since it has some issues to work out. - Fixed bug introduced in v10 which led to the joy-cons' S-triggers not being configured as an input. - Changed the pro controller's d-pad input from buttons to a hat to be more in line with other controller drivers. Version 10 changes: - Removed duplicate reporting of one of the triggers that Billy noticed - The joy-cons now only report having the buttons they actually have (they used to register the input devices with the buttons of the other joy-con as well). - The input device is now created after the LEDs/power supply. - The removed state handling bool has been removed, instead opting to add a new controller state (removed). - Eliminated a 1 second delay when probing a USB controller. - Added support for the IMU. This mostly consisted of merging in some work provided by Carl. I'm not incredibly familiar with proper gyro/accelerometer handling in linux, so this might need some tweaking. Preliminary tests in evtest show the gyro/accel values being reported. - Added support for the joy-con USB charging grip. Version 9 changes: - Fixed compiler errors on gcc versions older than 8.2 - Set input device's uniq value to the controller's MAC address Version 8 changes: - Corrected the handshaking protocol with USB pro controllers. A handshake now occurs both prior and after the baudrate set. This doesn't appear to have a noticeable difference, but it more accurately follows documentation found online. - Fixed potential race condition which could lead to a slightly longer delay sending subcommands in rare circumstances. - Moved the rumble worker to its own workqueue, since it can block. This prevents it from having a negative impact on the default kernel workqueue. It also prevents dropped subcommands due to something else blocking the kernel workqueue. The benefit is most obvious when using multiple controllers at once, since the controller subcommand timings are very picky. - Added a patch to set the most significant bit of the hid hw version. Roderick had mentioned needing to probably do this awhile ago, but I had forgotten about it until now. This is the same thing hid-sony does. It allows SDL2 to have different mappings for the hid-nintendo driver vs the default hid mappings. Version 7 changes: - Changed name to hid-nintendo to fit modern naming conventions - Removed joycon_ctlr_destroy(), since it wasn't needed an could potentially invalidate a mutex while it could be in use on other threads - Implemented minor code improvements suggested by Silvan - The driver now waits to send subcommands until after receiving an input report. This significantly reduces dropped subcommands. - Reduced the number of error messages when disconnecting a controller. Version 6 changes: - Improved subcommand sending reliabilty - Decreased rumble period to 50ms - Added rumble queue to avoid missing ff_effects if sent too quickly - Code cleanup and minor refactoring - Added default analog stick calibration Version 5 changes: - Removed sysfs interface to control motor frequencies. - Improved rumble reliability by using subcommands to set it. - Changed mapping of the SL/SR triggers on the joy-cons to map to whichever triggers they lack (e.g. a left joycon's sl/sr map to TR and TR2). This allows userspace to distinguish between the normal and S triggers. - Minor refactors Version 4 changes: - Added support for the Home button LED for the pro controller and right joy-con - Changed name from hid-switchcon to hid-joycon - Added rumble support - Removed ctlr->type and use hdev->product instead - Use POWER_SUPPLY_CAPACITY_LEVEL enum instead of manually translating to capacity percentages - Misc. minor refactors based on v3 feedback Version 3 changes: - Added led_classdev support for the 4 player LEDs - Added power_supply support for the controller's battery - Made the controller number mutex static - Minor refactoring/style fixes based on Roderick's feedback from v2 Version 2 changes: - Switched to using a synchronous method for configuring the controller. - Removed any pairing/orientation logic in the driver. Every controller now corresponds to its own input device. - Store controller button data as a single u32. - Style corrections Daniel J. Ogorchock (15): HID: nintendo: add nintendo switch controller driver HID: nintendo: add player led support HID: nintendo: add power supply support HID: nintendo: add home led support HID: nintendo: add rumble support HID: nintendo: improve subcommand reliability HID: nintendo: send subcommands after receiving input report HID: nintendo: reduce device removal subcommand errors HID: nintendo: patch hw version for userspace HID mappings HID: nintendo: set controller uniq to MAC HID: nintendo: add support for charging grip HID: nintendo: add support for reading user calibration HID: nintendo: prevent needless queueing of the rumble worker HID: nintendo: add IMU support HID: nintendo: improve rumble performance and stability MAINTAINERS | 6 + drivers/hid/Kconfig | 24 + drivers/hid/Makefile | 1 + drivers/hid/hid-ids.h | 4 + drivers/hid/hid-nintendo.c | 2274 ++++++++++++++++++++++++++++++++++++ drivers/input/joydev.c | 10 + 6 files changed, 2319 insertions(+) create mode 100644 drivers/hid/hid-nintendo.c