Message-ID: <20210506100705.5bcpywy25kfqwgkn@luna>
Date:   Thu, 6 May 2021 12:07:05 +0200
From:   Emmanuel Gil Peyrot <linkmauve@...kmauve.fr>
To:     Jonathan Neuschäfer <j.ne@...teo.net>
Cc:     linux-input@...r.kernel.org, Ash Logan <ash@...quark.com>,
        Jiri Kosina <jikos@...nel.org>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/4] HID: wiiu-drc: Add a driver for this gamepad

On Wed, May 05, 2021 at 10:33:15PM +0000, Jonathan Neuschäfer wrote:
> Hi,

Hi,

> 
> some mostly trivial remarks and questions of curiosity below, because
> I'm not very qualified to review the input subsystem side of things.

Thanks for the questions anyway, I can probably make things clearer in
the patch thanks to them. :)

[…]
> Out of curiosity:
> 
> Do the HID reports travel over the wireless link from DRC to DRH, or are
> they formed in DRH firmware?

This HID report is a 1:1 copy of what the DRC sends, with no
modification that I could find.

> 
> 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.

You were very close, the input report is described here:
https://libdrc.org/docs/re/sc-input.html

This project wrote a userland driver for using the DRC without the DRH,
but it requires a very specific wifi chip which makes it quite
cumbersome to use.

> 
> 
> >  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.

Will do in v2.

> 
> > +#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?

As far as I know, yes.

But the DRC also sends microphone and camera data, which gets exposed by
the DRH, but juuuuuust not quite standard enough to work as is using
snd_usb_audio or uvcvideo.  There is also a NFC reader which no one has
reversed yet to my knowledge.

There are two DRCs exposed by the DRH, despite only one of them being
bundled with each Wii U, and no game ever making use of more.

> 
> 
> > +++ 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)

Ack, will be fixed in v2.

> 
> 
> > +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.

Oops, thanks for noticing, this will be fixed in v2.

> 
> 
> > +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.

"Nintendo Wii U gamepad buttons and sticks" would be better I think.

> 
> 
> > +
> > +	/* 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?

It does draw a line of text near the bottom of the screen, saying “TV
Remote can be configured in System Settings.”, but also sends the button
as a normal button in the report.  It could be possible to change its
behaviour (in System Settings perhaps?) but so far I’ve been avoiding
interacting with the proprietary OS.

The power button also has a special behaviour: when it is held for four
seconds, it will power off the DRC.

> 
> 
> > +MODULE_AUTHOR("Ash Logan <ash@...quark.com>");
> 
> Since you're submitting the driver, rather than Ash, maybe adjust the
> author field here? (totally your choice.)

I’ll ask them, I’m perfectly fine with becoming the author, but they
wrote most of that code, I only fixed the last few missing pieces and
did some cleanup.

> 
> 
> 
> Thanks,
> Jonathan

Thanks!

-- 
Emmanuel Gil Peyrot

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists