Hi, some mostly trivial remarks and questions of curiosity below, because I'm not very qualified to review the input subsystem side of things. On Mon, May 03, 2021 at 01:28:32AM +0200, Emmanuel Gil Peyrot wrote: > From: Ash Logan <ash@xxxxxxxxxxxx> > > This driver is for the DRC (wireless gamepad) when plugged to the DRH of > the Wii U, a chip exposing it as a USB device. s/plugged/wirelessly connected/, rather > > This first patch exposes the buttons and sticks of this device, so that > it can act as a plain game controller. > > Signed-off-by: Ash Logan <ash@xxxxxxxxxxxx> > Signed-off-by: Emmanuel Gil Peyrot <linkmauve@xxxxxxxxxxxx> > --- Out of curiosity: Do the HID reports travel over the wireless link from DRC to DRH, or are they formed in DRH firmware? Is there a reference of the device-specific HID format? I briefly looked at https://libdrc.org/docs/index.html but couldn't find it there. > drivers/hid/Kconfig | 7 + > drivers/hid/Makefile | 1 + > drivers/hid/hid-ids.h | 1 + > drivers/hid/hid-quirks.c | 3 + > drivers/hid/hid-wiiu-drc.c | 270 +++++++++++++++++++++++++++++++++++++ > 5 files changed, 282 insertions(+) > create mode 100644 drivers/hid/hid-wiiu-drc.c > > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig > index 4bf263c2d61a..01116c315459 100644 > --- a/drivers/hid/Kconfig > +++ b/drivers/hid/Kconfig > @@ -1105,6 +1105,13 @@ config HID_WACOM > To compile this driver as a module, choose M here: the > module will be called wacom. > > +config HID_WIIU_DRC > + tristate "Nintendo Wii U gamepad over internal DRH" gamepad (DRC) ... so it's clearer where the "DRC" name comes from. > +#if IS_ENABLED(CONFIG_HID_WIIU_DRC) > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_NINTENDO, USB_DEVICE_ID_NINTENDO_WIIU_DRH) }, > +#endif Is the DRC connection the only USB function that the DRH provides? > +++ b/drivers/hid/hid-wiiu-drc.c > @@ -0,0 +1,270 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * HID driver for Nintendo Wii U gamepad, connected via console-internal DRH gamepad (DRC) > +static int drc_raw_event(struct hid_device *hdev, struct hid_report *report, > + u8 *data, int len) > +{ > + struct drc *drc = hid_get_drvdata(hdev); > + int i; > + u32 buttons; > + > + if (len != 128) > + return 0; From include/linux/hid.h: * raw_event and event should return negative on error, any other value will * pass the event on to .event() typically return 0 for success. Not sure if returning 0 as you do above is appropriate. > +static bool drc_setup_joypad(struct drc *drc, > + struct hid_device *hdev) > +{ > + struct input_dev *input_dev; > + > + input_dev = allocate_and_setup(hdev, DEVICE_NAME " Joypad"); "Nintendo Wii U gamepad Joypad" looks a bit sub-optimal, but I'm not sure about the conventions here. > + > + /* These two buttons are actually TV control and Power. */ > + set_bit(BTN_Z, input_dev->keybit); > + set_bit(BTN_DEAD, input_dev->keybit); Hmm... from what I've deen the TV control button opens a menu on the gamepad itself. Does it send the input event in addition to that? Or is there a mode where it opens the TV menu, and a mode where it forwards the button press to the Wii U? > +MODULE_AUTHOR("Ash Logan <ash@xxxxxxxxxxxx>"); Since you're submitting the driver, rather than Ash, maybe adjust the author field here? (totally your choice.) Thanks, Jonathan
Attachment:
signature.asc
Description: PGP signature
