Messages in this thread | | | Date | Wed, 5 May 2021 22:33:15 +0000 | From | Jonathan Neuschäfer <> | Subject | Re: [PATCH 1/4] HID: wiiu-drc: Add a driver for this gamepad |
| |
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@heyquark.com> > > 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@heyquark.com> > Signed-off-by: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr> > ---
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@heyquark.com>");
Since you're submitting the driver, rather than Ash, maybe adjust the author field here? (totally your choice.)
Thanks, Jonathan [unhandled content-type:application/pgp-signature] | |