Re: [PATCH 1/4] HID: wiiu-drc: Add a driver for this gamepad

["Jonathan Neuschäfer" <j.ne@posteo.net>]

[-- Attachment #1: Type: text/plain, Size: 3600 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]