mbox series

[0/4] RFC: HID: wiiu-drc: Add a driver for the Wii U gamepad

Message ID 20210502232836.26134-1-linkmauve@linkmauve.fr
Headers show
Series RFC: HID: wiiu-drc: Add a driver for the Wii U gamepad | expand

Message

Emmanuel Gil Peyrot May 2, 2021, 11:28 p.m. UTC
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.

I tried to use this driver on master over usbip on my laptop, but usbip
disconnects the device right after the driver created the
/dev/input/event* files, so instead I have only tested this driver on
the 4.19 branch of the linux-wiiu[1] downstream.

Other than that, pretty much all of the HID parts of the gamepad work,
it’s only missing microphone, camera and NFC input now but those are
mostly standard (read require quirks) and pertain to other subsystems,
so I felt like this can be upstreamed already.

I’ve still put the RFC tag on this pull request because of two known
problems in these patches (annotated with TODOs in the code):
- The magnetometer is exposed using non-sensical ABS_* values, it seems
  most (all?) magnetometers are exposed in the iio subsystem instead,
  should I go the same way despite it clearly being part of the same HID
  device?
- The battery number is currently based on a static int being
  incremented every time a new gamepad is “plugged in”, while I’d prefer
  to reuse the interface number for that.

Thanks for your guidance. :)

[1] https://gitlab.com/linux-wiiu/linux-wiiu

Ash Logan (1):
  HID: wiiu-drc: Add a driver for this gamepad

Emmanuel Gil Peyrot (3):
  HID: wiiu-drc: Implement touch reports
  HID: wiiu-drc: Add accelerometer, gyroscope and magnetometer readings
  HID: wiiu-drc: Add battery reporting

 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 | 522 +++++++++++++++++++++++++++++++++++++
 5 files changed, 534 insertions(+)
 create mode 100644 drivers/hid/hid-wiiu-drc.c

Comments

Jonathan Neuschäfer May 5, 2021, 10:33 p.m. UTC | #1
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
Emmanuel Gil Peyrot May 6, 2021, 10:07 a.m. UTC | #2
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@heyquark.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
Jonathan Neuschäfer May 6, 2021, 10:29 a.m. UTC | #3
On Thu, May 06, 2021 at 12:07:05PM +0200, Emmanuel Gil Peyrot wrote:
> On Wed, May 05, 2021 at 10:33:15PM +0000, Jonathan Neuschäfer wrote:

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


Ok, I think this link would be somewhat useful to have in the commit
message or a comment.

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


I see.


Thanks,
Jonathan
Barnabás Pőcze May 6, 2021, 11:53 a.m. UTC | #4
Hi


2021. május 3., hétfő 1:28 keltezéssel, Emmanuel Gil Peyrot írta:

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

>

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

> ---

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

> +	depends on HID

> +	help

> +	  Support for the Wii U gamepad, when connected with the Wii U’s

> +	  internal DRH chip.

> +

>  config HID_WIIMOTE

>  	tristate "Nintendo Wii / Wii U peripherals"

>  	depends on HID

> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile

> index 193431ec4db8..8fcaaeae4d65 100644

> --- a/drivers/hid/Makefile

> +++ b/drivers/hid/Makefile

> @@ -134,6 +134,7 @@ wacom-objs			:= wacom_wac.o wacom_sys.o

>  obj-$(CONFIG_HID_WACOM)		+= wacom.o

>  obj-$(CONFIG_HID_WALTOP)	+= hid-waltop.o

>  obj-$(CONFIG_HID_WIIMOTE)	+= hid-wiimote.o

> +obj-$(CONFIG_HID_WIIU_DRC)	+= hid-wiiu-drc.o

>  obj-$(CONFIG_HID_SENSOR_HUB)	+= hid-sensor-hub.o

>  obj-$(CONFIG_HID_SENSOR_CUSTOM_SENSOR)	+= hid-sensor-custom.o

>

> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h

> index 84b8da3e7d09..fbac0dd021f1 100644

> --- a/drivers/hid/hid-ids.h

> +++ b/drivers/hid/hid-ids.h

> @@ -916,6 +916,7 @@

>  #define USB_VENDOR_ID_NINTENDO		0x057e

>  #define USB_DEVICE_ID_NINTENDO_WIIMOTE	0x0306

>  #define USB_DEVICE_ID_NINTENDO_WIIMOTE2	0x0330

> +#define USB_DEVICE_ID_NINTENDO_WIIU_DRH	0x0341

>

>  #define USB_VENDOR_ID_NOVATEK		0x0603

>  #define USB_DEVICE_ID_NOVATEK_PCT	0x0600

> diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c

> index 3dd6f15f2a67..af400177537e 100644

> --- a/drivers/hid/hid-quirks.c

> +++ b/drivers/hid/hid-quirks.c

> @@ -513,6 +513,9 @@ static const struct hid_device_id hid_have_special_driver[] = {

>  	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_NINTENDO, USB_DEVICE_ID_NINTENDO_WIIMOTE) },

>  	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_NINTENDO, USB_DEVICE_ID_NINTENDO_WIIMOTE2) },

>  #endif

> +#if IS_ENABLED(CONFIG_HID_WIIU_DRC)

> +	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_NINTENDO, USB_DEVICE_ID_NINTENDO_WIIU_DRH) },

> +#endif

>  #if IS_ENABLED(CONFIG_HID_NTI)

>  	{ HID_USB_DEVICE(USB_VENDOR_ID_NTI, USB_DEVICE_ID_USB_SUN) },

>  #endif

> diff --git a/drivers/hid/hid-wiiu-drc.c b/drivers/hid/hid-wiiu-drc.c

> new file mode 100644

> index 000000000000..018cbdb53a2c

> --- /dev/null

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

> + *

> + * Copyright (C) 2021 Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>

> + * Copyright (C) 2019 Ash Logan <ash@heyquark.com>

> + * Copyright (C) 2013 Mema Hacking

> + *

> + * Based on the excellent work at http://libdrc.org/docs/re/sc-input.html and

> + * https://bitbucket.org/memahaxx/libdrc/src/master/src/input-receiver.cpp .

> + * libdrc code is licensed under BSD 2-Clause.

> + * Driver based on hid-udraw-ps3.c.

> + */

> +

> +#include <linux/device.h>

> +#include <linux/hid.h>

> +#include <linux/module.h>

> +#include "hid-ids.h"


It's usually best if you don't rely indirect includes. Include everything you use.
E.g. here linux/input.h is missing.


> +

> +#define DEVICE_NAME	"Nintendo Wii U gamepad"

> +

> +/* Button and stick constants */

> +#define VOLUME_MIN	0

> +#define VOLUME_MAX	255

> +#define NUM_STICK_AXES	4

> +#define STICK_MIN	900

> +#define STICK_MAX	3200

> +

> +#define BUTTON_SYNC	BIT(0)

> +#define BUTTON_HOME	BIT(1)

> +#define BUTTON_MINUS	BIT(2)

> +#define BUTTON_PLUS	BIT(3)

> +#define BUTTON_R	BIT(4)

> +#define BUTTON_L	BIT(5)

> +#define BUTTON_ZR	BIT(6)

> +#define BUTTON_ZL	BIT(7)

> +#define BUTTON_DOWN	BIT(8)

> +#define BUTTON_UP	BIT(9)

> +#define BUTTON_RIGHT	BIT(10)

> +#define BUTTON_LEFT	BIT(11)

> +#define BUTTON_Y	BIT(12)

> +#define BUTTON_X	BIT(13)

> +#define BUTTON_B	BIT(14)

> +#define BUTTON_A	BIT(15)

> +

> +#define BUTTON_TV	BIT(21)

> +#define BUTTON_R3	BIT(22)

> +#define BUTTON_L3	BIT(23)

> +

> +#define BUTTON_POWER	BIT(25)

> +

> +/*

> + * The device is setup with multiple input devices:

> + * - A joypad with the buttons and sticks.

> + */

> +

> +struct drc {

> +	struct input_dev *joy_input_dev;

> +	struct hid_device *hdev;

> +};

> +

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

> +

> +	buttons = (data[4] << 24) | (data[80] << 16) | (data[2] << 8) | data[3];

> +	/* joypad */

> +	input_report_key(drc->joy_input_dev, BTN_DPAD_RIGHT, buttons & BUTTON_RIGHT);

> +	input_report_key(drc->joy_input_dev, BTN_DPAD_DOWN, buttons & BUTTON_DOWN);

> +	input_report_key(drc->joy_input_dev, BTN_DPAD_LEFT, buttons & BUTTON_LEFT);

> +	input_report_key(drc->joy_input_dev, BTN_DPAD_UP, buttons & BUTTON_UP);

> +

> +	input_report_key(drc->joy_input_dev, BTN_EAST, buttons & BUTTON_A);

> +	input_report_key(drc->joy_input_dev, BTN_SOUTH, buttons & BUTTON_B);

> +	input_report_key(drc->joy_input_dev, BTN_NORTH, buttons & BUTTON_X);

> +	input_report_key(drc->joy_input_dev, BTN_WEST, buttons & BUTTON_Y);

> +

> +	input_report_key(drc->joy_input_dev, BTN_TL, buttons & BUTTON_L);

> +	input_report_key(drc->joy_input_dev, BTN_TL2, buttons & BUTTON_ZL);

> +	input_report_key(drc->joy_input_dev, BTN_TR, buttons & BUTTON_R);

> +	input_report_key(drc->joy_input_dev, BTN_TR2, buttons & BUTTON_ZR);

> +

> +	input_report_key(drc->joy_input_dev, BTN_Z, buttons & BUTTON_TV);

> +	input_report_key(drc->joy_input_dev, BTN_THUMBL, buttons & BUTTON_L3);

> +	input_report_key(drc->joy_input_dev, BTN_THUMBR, buttons & BUTTON_R3);

> +

> +	input_report_key(drc->joy_input_dev, BTN_SELECT, buttons & BUTTON_MINUS);

> +	input_report_key(drc->joy_input_dev, BTN_START, buttons & BUTTON_PLUS);

> +	input_report_key(drc->joy_input_dev, BTN_MODE, buttons & BUTTON_HOME);

> +

> +	input_report_key(drc->joy_input_dev, BTN_DEAD, buttons & BUTTON_POWER);

> +

> +	for (i = 0; i < NUM_STICK_AXES; i++) {

> +		s16 val = (data[7 + 2*i] << 8) | data[6 + 2*i];

> +		/* clamp */

> +		if (val < STICK_MIN)

> +			val = STICK_MIN;

> +		if (val > STICK_MAX)

> +			val = STICK_MAX;


There's `clamp()` in linux/minmax.h, you might want to use that.


> +

> +		switch (i) {

> +		case 0:

> +			input_report_abs(drc->joy_input_dev, ABS_X, val);

> +			break;

> +		case 1:

> +			input_report_abs(drc->joy_input_dev, ABS_Y, val);

> +			break;

> +		case 2:

> +			input_report_abs(drc->joy_input_dev, ABS_RX, val);

> +			break;

> +		case 3:

> +			input_report_abs(drc->joy_input_dev, ABS_RY, val);

> +			break;

> +		default:

> +			break;

> +		}

> +	}

> +

> +	input_report_abs(drc->joy_input_dev, ABS_VOLUME, data[14]);

> +

> +	input_sync(drc->joy_input_dev);

> +

> +	/* let hidraw and hiddev handle the report */

> +	return 0;

> +}

> +

> +static int drc_open(struct input_dev *dev)

> +{

> +	struct drc *drc = input_get_drvdata(dev);

> +

> +	return hid_hw_open(drc->hdev);

> +}

> +

> +static void drc_close(struct input_dev *dev)

> +{

> +	struct drc *drc = input_get_drvdata(dev);

> +

> +	hid_hw_close(drc->hdev);

> +}

> +

> +static struct input_dev *allocate_and_setup(struct hid_device *hdev,

> +		const char *name)

> +{

> +	struct input_dev *input_dev;

> +

> +	input_dev = devm_input_allocate_device(&hdev->dev);

> +	if (!input_dev)

> +		return NULL;

> +

> +	input_dev->name = name;

> +	input_dev->phys = hdev->phys;

> +	input_dev->dev.parent = &hdev->dev;

> +	input_dev->open = drc_open;

> +	input_dev->close = drc_close;

> +	input_dev->uniq = hdev->uniq;

> +	input_dev->id.bustype = hdev->bus;

> +	input_dev->id.vendor  = hdev->vendor;

> +	input_dev->id.product = hdev->product;

> +	input_dev->id.version = hdev->version;

> +	input_set_drvdata(input_dev, hid_get_drvdata(hdev));

> +

> +	return input_dev;

> +}

> +

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

> +	if (!input_dev)

> +		return false;

> +

> +	input_dev->evbit[0] = BIT(EV_KEY) | BIT(EV_ABS);


`input_set_abs_params()` already sets EV_ABS.


> +

> +	set_bit(BTN_DPAD_RIGHT, input_dev->keybit);

> +	set_bit(BTN_DPAD_DOWN, input_dev->keybit);

> +	set_bit(BTN_DPAD_LEFT, input_dev->keybit);

> +	set_bit(BTN_DPAD_UP, input_dev->keybit);

> +	set_bit(BTN_EAST, input_dev->keybit);

> +	set_bit(BTN_SOUTH, input_dev->keybit);

> +	set_bit(BTN_NORTH, input_dev->keybit);

> +	set_bit(BTN_WEST, input_dev->keybit);

> +	set_bit(BTN_TL, input_dev->keybit);

> +	set_bit(BTN_TL2, input_dev->keybit);

> +	set_bit(BTN_TR, input_dev->keybit);

> +	set_bit(BTN_TR2, input_dev->keybit);

> +	set_bit(BTN_THUMBL, input_dev->keybit);

> +	set_bit(BTN_THUMBR, input_dev->keybit);

> +	set_bit(BTN_SELECT, input_dev->keybit);

> +	set_bit(BTN_START, input_dev->keybit);

> +	set_bit(BTN_MODE, input_dev->keybit);

> +

> +	/* These two buttons are actually TV control and Power. */

> +	set_bit(BTN_Z, input_dev->keybit);

> +	set_bit(BTN_DEAD, input_dev->keybit);


You could use `input_set_capability(device, EV_KEY, ...)` (potentially in a loop)
instead of manually setting the bits. And then `input_dev->evbit[0] = BIT(EV_KEY) | BIT(EV_ABS);`
would be unnecessary.


> +

> +	input_set_abs_params(input_dev, ABS_X, STICK_MIN, STICK_MAX, 0, 0);

> +	input_set_abs_params(input_dev, ABS_Y, STICK_MIN, STICK_MAX, 0, 0);

> +	input_set_abs_params(input_dev, ABS_RX, STICK_MIN, STICK_MAX, 0, 0);

> +	input_set_abs_params(input_dev, ABS_RY, STICK_MIN, STICK_MAX, 0, 0);

> +	input_set_abs_params(input_dev, ABS_VOLUME, VOLUME_MIN, VOLUME_MAX, 0, 0);

> +

> +	drc->joy_input_dev = input_dev;

> +

> +	return true;

> +}

> +

> +static int drc_probe(struct hid_device *hdev, const struct hid_device_id *id)

> +{

> +	struct drc *drc;

> +	int ret;

> +

> +	drc = devm_kzalloc(&hdev->dev, sizeof(struct drc), GFP_KERNEL);

> +	if (!drc)

> +		return -ENOMEM;

> +

> +	drc->hdev = hdev;

> +

> +	hid_set_drvdata(hdev, drc);

> +

> +	ret = hid_parse(hdev);

> +	if (ret) {

> +		hid_err(hdev, "parse failed\n");

> +		return ret;

> +	}

> +

> +	if (!drc_setup_joypad(drc, hdev)) {

> +		hid_err(hdev, "could not allocate interface\n");

> +		return -ENOMEM;

> +	}

> +

> +	ret = input_register_device(drc->joy_input_dev);

> +	if (ret) {

> +		hid_err(hdev, "failed to register interface\n");

> +		return ret;

> +	}

> +

> +	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW | HID_CONNECT_DRIVER);


As far as I'm aware, `hid_hw_start()` should be called before `hid_hw_open()`.
Since you register the input device first, I think it is possible that `hid_hw_open()`
will be called first.


> +	if (ret) {

> +		hid_err(hdev, "hw start failed\n");

> +		return ret;

> +	}

> +

> +	return 0;

> +}

> +

> +static const struct hid_device_id drc_devices[] = {

> +	{ HID_USB_DEVICE(USB_VENDOR_ID_NINTENDO, USB_DEVICE_ID_NINTENDO_WIIU_DRH) },

> +	{ }

> +};

> +MODULE_DEVICE_TABLE(hid, drc_devices);

> +

> +static struct hid_driver drc_driver = {

> +	.name = "hid-wiiu-drc",

> +	.id_table = drc_devices,

> +	.raw_event = drc_raw_event,

> +	.probe = drc_probe,

> +};

> +module_hid_driver(drc_driver);

> +

> +MODULE_AUTHOR("Ash Logan <ash@heyquark.com>");

> +MODULE_DESCRIPTION("Nintendo Wii U gamepad driver");

> +MODULE_LICENSE("GPL");

> --

> 2.31.1



Regards,
Barnabás Pőcze