[PATCH 1/2] usbhid: Add quirks for Mayflash/Dragonrise GameCube and PS3 adapters.

[PATCH 1/2] usbhid: Add quirks for Mayflash/Dragonrise GameCube and PS3 adapters.

[Marcel Hasler]

All known gamepad adapters by Mayflash (identified as Dragonrise) need HID_QUIRK_MULTI_INPUT to
split them up into four input devices. Without this quirk those adapters are falsely recognized
as tablets. Fixes bug 115841 (https://bugzilla.kernel.org/show_bug.cgi?id=115841).

Signed-off-by: Marcel Hasler <mahasler@gmail.com>
---
 drivers/hid/hid-ids.h           | 6 ++++--
 drivers/hid/usbhid/hid-quirks.c | 2 ++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 6cfb5ca..642e648 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -314,8 +314,10 @@
 #define USB_VENDOR_ID_DMI		0x0c0b
 #define USB_DEVICE_ID_DMI_ENC		0x5fab
 
-#define USB_VENDOR_ID_DRAGONRISE	0x0079
-#define USB_DEVICE_ID_DRAGONRISE_WIIU	0x1800
+#define USB_VENDOR_ID_DRAGONRISE		0x0079
+#define USB_DEVICE_ID_DRAGONRISE_WIIU		0x1800
+#define USB_DEVICE_ID_DRAGONRISE_PS3		0x1801
+#define USB_DEVICE_ID_DRAGONRISE_GAMECUBE	0x1843
 
 #define USB_VENDOR_ID_DWAV		0x0eef
 #define USB_DEVICE_ID_EGALAX_TOUCHCONTROLLER	0x0001
diff --git a/drivers/hid/usbhid/hid-quirks.c b/drivers/hid/usbhid/hid-quirks.c
index 354d49e..05f6f61 100644
--- a/drivers/hid/usbhid/hid-quirks.c
+++ b/drivers/hid/usbhid/hid-quirks.c
@@ -81,6 +81,8 @@ static const struct hid_blacklist {
 	{ USB_VENDOR_ID_CREATIVELABS, USB_DEVICE_ID_CREATIVE_SB_OMNI_SURROUND_51, HID_QUIRK_NOGET },
 	{ USB_VENDOR_ID_DMI, USB_DEVICE_ID_DMI_ENC, HID_QUIRK_NOGET },
 	{ USB_VENDOR_ID_DRAGONRISE, USB_DEVICE_ID_DRAGONRISE_WIIU, HID_QUIRK_MULTI_INPUT },
+	{ USB_VENDOR_ID_DRAGONRISE, USB_DEVICE_ID_DRAGONRISE_PS3, HID_QUIRK_MULTI_INPUT },
+	{ USB_VENDOR_ID_DRAGONRISE, USB_DEVICE_ID_DRAGONRISE_GAMECUBE, HID_QUIRK_MULTI_INPUT },
 	{ USB_VENDOR_ID_ELAN, HID_ANY_ID, HID_QUIRK_ALWAYS_POLL },
 	{ USB_VENDOR_ID_ELO, USB_DEVICE_ID_ELO_TS2700, HID_QUIRK_NOGET },
 	{ USB_VENDOR_ID_FORMOSA, USB_DEVICE_ID_FORMOSA_IR_RECEIVER, HID_QUIRK_NO_INIT_REPORTS },
-- 
2.10.1

Re: [PATCH 1/2] usbhid: Add quirks for Mayflash/Dragonrise GameCube and PS3 adapters.

[Benjamin Tissoires]

On Oct 30 2016 or thereabouts, Marcel Hasler wrote:
> All known gamepad adapters by Mayflash (identified as Dragonrise) need HID_QUIRK_MULTI_INPUT to
> split them up into four input devices. Without this quirk those adapters are falsely recognized
> as tablets. Fixes bug 115841 (https://bugzilla.kernel.org/show_bug.cgi?id=115841).
> 
> Signed-off-by: Marcel Hasler <mahasler@gmail.com>
> ---
>  drivers/hid/hid-ids.h           | 6 ++++--
>  drivers/hid/usbhid/hid-quirks.c | 2 ++
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 6cfb5ca..642e648 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -314,8 +314,10 @@
>  #define USB_VENDOR_ID_DMI		0x0c0b
>  #define USB_DEVICE_ID_DMI_ENC		0x5fab
>  
> -#define USB_VENDOR_ID_DRAGONRISE	0x0079
> -#define USB_DEVICE_ID_DRAGONRISE_WIIU	0x1800
> +#define USB_VENDOR_ID_DRAGONRISE		0x0079
> +#define USB_DEVICE_ID_DRAGONRISE_WIIU		0x1800
> +#define USB_DEVICE_ID_DRAGONRISE_PS3		0x1801

I already gave my rev-by of this patch earlier today, but if you are to
switch the device to be using a specific driver, I'd rather see the HID
quirk set into the driver itself. The rational is that if you have to
use a Bluetooth connection or a replay of the device through uhid,
you'll get the quirk applied, while this patch only applies to
USB physically connected devices.

Cheers,
Benjamin

> +#define USB_DEVICE_ID_DRAGONRISE_GAMECUBE	0x1843
>  
>  #define USB_VENDOR_ID_DWAV		0x0eef
>  #define USB_DEVICE_ID_EGALAX_TOUCHCONTROLLER	0x0001
> diff --git a/drivers/hid/usbhid/hid-quirks.c b/drivers/hid/usbhid/hid-quirks.c
> index 354d49e..05f6f61 100644
> --- a/drivers/hid/usbhid/hid-quirks.c
> +++ b/drivers/hid/usbhid/hid-quirks.c
> @@ -81,6 +81,8 @@ static const struct hid_blacklist {
>  	{ USB_VENDOR_ID_CREATIVELABS, USB_DEVICE_ID_CREATIVE_SB_OMNI_SURROUND_51, HID_QUIRK_NOGET },
>  	{ USB_VENDOR_ID_DMI, USB_DEVICE_ID_DMI_ENC, HID_QUIRK_NOGET },
>  	{ USB_VENDOR_ID_DRAGONRISE, USB_DEVICE_ID_DRAGONRISE_WIIU, HID_QUIRK_MULTI_INPUT },
> +	{ USB_VENDOR_ID_DRAGONRISE, USB_DEVICE_ID_DRAGONRISE_PS3, HID_QUIRK_MULTI_INPUT },
> +	{ USB_VENDOR_ID_DRAGONRISE, USB_DEVICE_ID_DRAGONRISE_GAMECUBE, HID_QUIRK_MULTI_INPUT },
>  	{ USB_VENDOR_ID_ELAN, HID_ANY_ID, HID_QUIRK_ALWAYS_POLL },
>  	{ USB_VENDOR_ID_ELO, USB_DEVICE_ID_ELO_TS2700, HID_QUIRK_NOGET },
>  	{ USB_VENDOR_ID_FORMOSA, USB_DEVICE_ID_FORMOSA_IR_RECEIVER, HID_QUIRK_NO_INIT_REPORTS },
> -- 
> 2.10.1
> 

[PATCH 2/2] Add new force feedback driver for Mayflash game controller adapters.

[Marcel Hasler]

Add a new module named hid-mf that implements force feedback for game controller adapters
manufactured by Mayflash. Currently only the PS3 adapter is supported, other adapters still need
to be tested.

Signed-off-by: Marcel Hasler <mahasler@gmail.com>
---
 drivers/hid/Kconfig    |   8 +++
 drivers/hid/Makefile   |   1 +
 drivers/hid/hid-core.c |   1 +
 drivers/hid/hid-mf.c   | 165 +++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 175 insertions(+)
 create mode 100644 drivers/hid/hid-mf.c

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index cd4599c..1530d28 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -512,6 +512,14 @@ config HID_MAGICMOUSE
 	Say Y here if you want support for the multi-touch features of the
 	Apple Wireless "Magic" Mouse and the Apple Wireless "Magic" Trackpad.
 
+config HID_MAYFLASH
+	tristate "Mayflash game controller adapter force feedback"
+	depends on HID
+	select INPUT_FF_MEMLESS
+	---help---
+	Say Y here if you have HJZ Mayflash PS3 game controller adapters
+	and want to enable force feedback support.
+
 config HID_MICROSOFT
 	tristate "Microsoft non-fully HID-compliant devices"
 	depends on HID
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 86b2b57..c0453f1 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -58,6 +58,7 @@ obj-$(CONFIG_HID_LOGITECH)	+= hid-logitech.o
 obj-$(CONFIG_HID_LOGITECH_DJ)	+= hid-logitech-dj.o
 obj-$(CONFIG_HID_LOGITECH_HIDPP)	+= hid-logitech-hidpp.o
 obj-$(CONFIG_HID_MAGICMOUSE)	+= hid-magicmouse.o
+obj-$(CONFIG_HID_MAYFLASH)	+= hid-mf.o
 obj-$(CONFIG_HID_MICROSOFT)	+= hid-microsoft.o
 obj-$(CONFIG_HID_MONTEREY)	+= hid-monterey.o
 obj-$(CONFIG_HID_MULTITOUCH)	+= hid-multitouch.o
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 2b89c70..8470e22 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1883,6 +1883,7 @@ static const struct hid_device_id hid_have_special_driver[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_DELCOM, USB_DEVICE_ID_DELCOM_VISUAL_IND) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, 0x0006) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, 0x0011) },
+	{ HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, USB_DEVICE_ID_DRAGONRISE_PS3) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, USB_DEVICE_ID_DREAM_CHEEKY_WN) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, USB_DEVICE_ID_DREAM_CHEEKY_FA) },
 	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_BM084) },
diff --git a/drivers/hid/hid-mf.c b/drivers/hid/hid-mf.c
new file mode 100644
index 0000000..6791ce7
--- /dev/null
+++ b/drivers/hid/hid-mf.c
@@ -0,0 +1,165 @@
+/*
+ * Force feedback support for Mayflash game controller adapters.
+ *
+ * These devices are manufactured by Mayflash but identify themselves
+ * using the vendor ID of DragonRise Inc.
+ *
+ * Tested with:
+ * 0079:1801 "DragonRise Inc. Mayflash PS3 Game Controller Adapter"
+ *
+ * The following adapters probably work too, but need to be tested:
+ * 0079:1800 "DragonRise Inc. Mayflash WIIU Game Controller Adapter"
+ * 0079:1843 "DragonRise Inc. Mayflash GameCube Game Controller Adapter"
+ *
+ * Copyright (c) 2016 Marcel Hasler <mahasler@gmail.com>
+ */
+
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/input.h>
+#include <linux/slab.h>
+#include <linux/hid.h>
+#include <linux/module.h>
+
+#include "hid-ids.h"
+
+struct mf_device {
+	struct hid_report *report;
+};
+
+static int mf_play(struct input_dev *dev, void *data, struct ff_effect *effect)
+{
+	struct hid_device *hid = input_get_drvdata(dev);
+	struct mf_device *mf = data;
+	int strong, weak;
+
+	strong = effect->u.rumble.strong_magnitude;
+	weak = effect->u.rumble.weak_magnitude;
+
+	dbg_hid("Called with 0x%04x 0x%04x.\n", strong, weak);
+
+	strong = strong * 0xff / 0xffff;
+	weak = weak * 0xff / 0xffff;
+
+	dbg_hid("Running with 0x%02x 0x%02x.\n", strong, weak);
+
+	mf->report->field[0]->value[0] = weak;
+	mf->report->field[0]->value[1] = strong;
+	hid_hw_request(hid, mf->report, HID_REQ_SET_REPORT);
+
+	return 0;
+}
+
+static int mf_init(struct hid_device *hid)
+{
+	struct mf_device *mf;
+
+	struct list_head *report_list =
+			&hid->report_enum[HID_OUTPUT_REPORT].report_list;
+
+	struct list_head *report_ptr;
+	struct hid_report *report;
+
+	struct hid_input *input =
+			list_first_entry(&hid->inputs, struct hid_input, list);
+
+	struct input_dev *dev;
+
+	int error;
+
+	/* Setup each of the four inputs */
+	list_for_each(report_ptr, report_list) {
+		report = list_entry(report_ptr, struct hid_report, list);
+
+		if (report->maxfield < 1 || report->field[0]->report_count < 2) {
+			hid_err(hid, "Invalid report, this should never happen!\n");
+			return -ENODEV;
+		}
+
+		if (input == NULL) {
+			hid_err(hid, "Missing input, this should never happen!\n");
+			return -ENODEV;
+		}
+
+		dev = input->input;
+		input = list_next_entry(input, list);
+
+		mf = kzalloc(sizeof(struct mf_device), GFP_KERNEL);
+		if (!mf)
+			return -ENOMEM;
+
+		set_bit(FF_RUMBLE, dev->ffbit);
+
+		error = input_ff_create_memless(dev, mf, mf_play);
+		if (error) {
+			kfree(mf);
+			return error;
+		}
+
+		mf->report = report;
+		mf->report->field[0]->value[0] = 0x00;
+		mf->report->field[0]->value[1] = 0x00;
+		hid_hw_request(hid, mf->report, HID_REQ_SET_REPORT);
+	}
+
+	hid_info(hid, "Force feedback for HJZ Mayflash game controller "
+		      "adapters by Marcel Hasler <mahasler@gmail.com>\n");
+
+	return 0;
+}
+
+static int mf_probe(struct hid_device *hdev, const struct hid_device_id *id)
+{
+	int error;
+
+	dev_dbg(&hdev->dev, "Mayflash HID hardware probe...\n");
+
+	/* Split device into four inputs */
+	hdev->quirks |= HID_QUIRK_MULTI_INPUT;
+
+	error = hid_parse(hdev);
+	if (error) {
+		hid_err(hdev, "HID parse failed.\n");
+		return error;
+	}
+
+	error = hid_hw_start(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_FF);
+	if (error) {
+		hid_err(hdev, "HID hw start failed\n");
+		return error;
+	}
+
+	error = mf_init(hdev);
+	if (error) {
+		hid_err(hdev, "Force feedback init failed.\n");
+		hid_hw_stop(hdev);
+		return error;
+	}
+
+	return 0;
+}
+
+static const struct hid_device_id mf_devices[] = {
+	{ HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, USB_DEVICE_ID_DRAGONRISE_PS3),  },
+	{ }
+};
+MODULE_DEVICE_TABLE(hid, mf_devices);
+
+static struct hid_driver mf_driver = {
+	.name = "hid_mf",
+	.id_table = mf_devices,
+	.probe = mf_probe,
+};
+module_hid_driver(mf_driver);
+
+MODULE_LICENSE("GPL");
-- 
2.10.1

Re: [PATCH 2/2] Add new force feedback driver for Mayflash game controller adapters.

[Benjamin Tissoires]

On Oct 30 2016 or thereabouts, Marcel Hasler wrote:
> Add a new module named hid-mf that implements force feedback for game controller adapters
> manufactured by Mayflash. Currently only the PS3 adapter is supported, other adapters still need
> to be tested.
> 
> Signed-off-by: Marcel Hasler <mahasler@gmail.com>
> ---
>  drivers/hid/Kconfig    |   8 +++
>  drivers/hid/Makefile   |   1 +
>  drivers/hid/hid-core.c |   1 +
>  drivers/hid/hid-mf.c   | 165 +++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 175 insertions(+)
>  create mode 100644 drivers/hid/hid-mf.c
> 
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index cd4599c..1530d28 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -512,6 +512,14 @@ config HID_MAGICMOUSE
>  	Say Y here if you want support for the multi-touch features of the
>  	Apple Wireless "Magic" Mouse and the Apple Wireless "Magic" Trackpad.
>  
> +config HID_MAYFLASH
> +	tristate "Mayflash game controller adapter force feedback"
> +	depends on HID
> +	select INPUT_FF_MEMLESS
> +	---help---
> +	Say Y here if you have HJZ Mayflash PS3 game controller adapters
> +	and want to enable force feedback support.
> +
>  config HID_MICROSOFT
>  	tristate "Microsoft non-fully HID-compliant devices"
>  	depends on HID
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index 86b2b57..c0453f1 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -58,6 +58,7 @@ obj-$(CONFIG_HID_LOGITECH)	+= hid-logitech.o
>  obj-$(CONFIG_HID_LOGITECH_DJ)	+= hid-logitech-dj.o
>  obj-$(CONFIG_HID_LOGITECH_HIDPP)	+= hid-logitech-hidpp.o
>  obj-$(CONFIG_HID_MAGICMOUSE)	+= hid-magicmouse.o
> +obj-$(CONFIG_HID_MAYFLASH)	+= hid-mf.o
>  obj-$(CONFIG_HID_MICROSOFT)	+= hid-microsoft.o
>  obj-$(CONFIG_HID_MONTEREY)	+= hid-monterey.o
>  obj-$(CONFIG_HID_MULTITOUCH)	+= hid-multitouch.o
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 2b89c70..8470e22 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1883,6 +1883,7 @@ static const struct hid_device_id hid_have_special_driver[] = {
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_DELCOM, USB_DEVICE_ID_DELCOM_VISUAL_IND) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, 0x0006) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, 0x0011) },
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, USB_DEVICE_ID_DRAGONRISE_PS3) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, USB_DEVICE_ID_DREAM_CHEEKY_WN) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, USB_DEVICE_ID_DREAM_CHEEKY_FA) },
>  	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_BM084) },
> diff --git a/drivers/hid/hid-mf.c b/drivers/hid/hid-mf.c
> new file mode 100644
> index 0000000..6791ce7
> --- /dev/null
> +++ b/drivers/hid/hid-mf.c
> @@ -0,0 +1,165 @@
> +/*
> + * Force feedback support for Mayflash game controller adapters.
> + *
> + * These devices are manufactured by Mayflash but identify themselves
> + * using the vendor ID of DragonRise Inc.
> + *
> + * Tested with:
> + * 0079:1801 "DragonRise Inc. Mayflash PS3 Game Controller Adapter"
> + *
> + * The following adapters probably work too, but need to be tested:
> + * 0079:1800 "DragonRise Inc. Mayflash WIIU Game Controller Adapter"
> + * 0079:1843 "DragonRise Inc. Mayflash GameCube Game Controller Adapter"
> + *
> + * Copyright (c) 2016 Marcel Hasler <mahasler@gmail.com>
> + */
> +
> +/*
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/input.h>
> +#include <linux/slab.h>
> +#include <linux/hid.h>
> +#include <linux/module.h>
> +
> +#include "hid-ids.h"
> +
> +struct mf_device {
> +	struct hid_report *report;
> +};
> +
> +static int mf_play(struct input_dev *dev, void *data, struct ff_effect *effect)
> +{
> +	struct hid_device *hid = input_get_drvdata(dev);
> +	struct mf_device *mf = data;
> +	int strong, weak;
> +
> +	strong = effect->u.rumble.strong_magnitude;
> +	weak = effect->u.rumble.weak_magnitude;
> +
> +	dbg_hid("Called with 0x%04x 0x%04x.\n", strong, weak);
> +
> +	strong = strong * 0xff / 0xffff;
> +	weak = weak * 0xff / 0xffff;
> +
> +	dbg_hid("Running with 0x%02x 0x%02x.\n", strong, weak);
> +
> +	mf->report->field[0]->value[0] = weak;
> +	mf->report->field[0]->value[1] = strong;
> +	hid_hw_request(hid, mf->report, HID_REQ_SET_REPORT);
> +
> +	return 0;
> +}
> +
> +static int mf_init(struct hid_device *hid)
> +{
> +	struct mf_device *mf;
> +
> +	struct list_head *report_list =
> +			&hid->report_enum[HID_OUTPUT_REPORT].report_list;
> +
> +	struct list_head *report_ptr;
> +	struct hid_report *report;
> +
> +	struct hid_input *input =
> +			list_first_entry(&hid->inputs, struct hid_input, list);
> +
> +	struct input_dev *dev;
> +
> +	int error;
> +
> +	/* Setup each of the four inputs */
> +	list_for_each(report_ptr, report_list) {
> +		report = list_entry(report_ptr, struct hid_report, list);
> +
> +		if (report->maxfield < 1 || report->field[0]->report_count < 2) {
> +			hid_err(hid, "Invalid report, this should never happen!\n");
> +			return -ENODEV;
> +		}
> +
> +		if (input == NULL) {
> +			hid_err(hid, "Missing input, this should never happen!\n");
> +			return -ENODEV;
> +		}
> +
> +		dev = input->input;
> +		input = list_next_entry(input, list);

Wouldn't it make more sense to use .input_configured instead of this
after-the-fact loop?

> +
> +		mf = kzalloc(sizeof(struct mf_device), GFP_KERNEL);

I might not be fully awoken, but where is the corresponding kfree?
If you do not want to manually free the allocated memory, you can use
devm_kzalloc, as long as there is no races when removing the device.

> +		if (!mf)
> +			return -ENOMEM;
> +
> +		set_bit(FF_RUMBLE, dev->ffbit);
> +
> +		error = input_ff_create_memless(dev, mf, mf_play);
> +		if (error) {
> +			kfree(mf);
> +			return error;
> +		}
> +
> +		mf->report = report;
> +		mf->report->field[0]->value[0] = 0x00;
> +		mf->report->field[0]->value[1] = 0x00;
> +		hid_hw_request(hid, mf->report, HID_REQ_SET_REPORT);
> +	}
> +
> +	hid_info(hid, "Force feedback for HJZ Mayflash game controller "
> +		      "adapters by Marcel Hasler <mahasler@gmail.com>\n");
> +
> +	return 0;
> +}
> +
> +static int mf_probe(struct hid_device *hdev, const struct hid_device_id *id)
> +{
> +	int error;
> +
> +	dev_dbg(&hdev->dev, "Mayflash HID hardware probe...\n");
> +
> +	/* Split device into four inputs */
> +	hdev->quirks |= HID_QUIRK_MULTI_INPUT;

Looks like the entry in the previous patch was not required apparently
:)

> +
> +	error = hid_parse(hdev);
> +	if (error) {
> +		hid_err(hdev, "HID parse failed.\n");
> +		return error;
> +	}
> +
> +	error = hid_hw_start(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_FF);
> +	if (error) {
> +		hid_err(hdev, "HID hw start failed\n");
> +		return error;
> +	}
> +
> +	error = mf_init(hdev);

This seems completely racy. hid_hw_start() exports the input nodes to
user-space and the ff initialization is done after. It would be much
better to initialize the ff part in .input_configured when the device
hasn't been registered to user space yet.

Cheers,
Benjamin

> +	if (error) {
> +		hid_err(hdev, "Force feedback init failed.\n");
> +		hid_hw_stop(hdev);
> +		return error;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct hid_device_id mf_devices[] = {
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, USB_DEVICE_ID_DRAGONRISE_PS3),  },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(hid, mf_devices);
> +
> +static struct hid_driver mf_driver = {
> +	.name = "hid_mf",
> +	.id_table = mf_devices,
> +	.probe = mf_probe,
> +};
> +module_hid_driver(mf_driver);
> +
> +MODULE_LICENSE("GPL");
> -- 
> 2.10.1
> 

Re: [PATCH 2/2] Add new force feedback driver for Mayflash game controller adapters.

[Marcel Hasler]

Hi

2016-10-31 12:17 GMT+01:00 Benjamin Tissoires <benjamin.tissoires@redhat.com>:
> On Oct 30 2016 or thereabouts, Marcel Hasler wrote:
>> Add a new module named hid-mf that implements force feedback for game controller adapters
>> manufactured by Mayflash. Currently only the PS3 adapter is supported, other adapters still need
>> to be tested.
>>
>> Signed-off-by: Marcel Hasler <mahasler@gmail.com>
>> ---
>>  drivers/hid/Kconfig    |   8 +++
>>  drivers/hid/Makefile   |   1 +
>>  drivers/hid/hid-core.c |   1 +
>>  drivers/hid/hid-mf.c   | 165 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 175 insertions(+)
>>  create mode 100644 drivers/hid/hid-mf.c
>>
>> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
>> index cd4599c..1530d28 100644
>> --- a/drivers/hid/Kconfig
>> +++ b/drivers/hid/Kconfig
>> @@ -512,6 +512,14 @@ config HID_MAGICMOUSE
>>       Say Y here if you want support for the multi-touch features of the
>>       Apple Wireless "Magic" Mouse and the Apple Wireless "Magic" Trackpad.
>>
>> +config HID_MAYFLASH
>> +     tristate "Mayflash game controller adapter force feedback"
>> +     depends on HID
>> +     select INPUT_FF_MEMLESS
>> +     ---help---
>> +     Say Y here if you have HJZ Mayflash PS3 game controller adapters
>> +     and want to enable force feedback support.
>> +
>>  config HID_MICROSOFT
>>       tristate "Microsoft non-fully HID-compliant devices"
>>       depends on HID
>> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
>> index 86b2b57..c0453f1 100644
>> --- a/drivers/hid/Makefile
>> +++ b/drivers/hid/Makefile
>> @@ -58,6 +58,7 @@ obj-$(CONFIG_HID_LOGITECH)  += hid-logitech.o
>>  obj-$(CONFIG_HID_LOGITECH_DJ)        += hid-logitech-dj.o
>>  obj-$(CONFIG_HID_LOGITECH_HIDPP)     += hid-logitech-hidpp.o
>>  obj-$(CONFIG_HID_MAGICMOUSE) += hid-magicmouse.o
>> +obj-$(CONFIG_HID_MAYFLASH)   += hid-mf.o
>>  obj-$(CONFIG_HID_MICROSOFT)  += hid-microsoft.o
>>  obj-$(CONFIG_HID_MONTEREY)   += hid-monterey.o
>>  obj-$(CONFIG_HID_MULTITOUCH) += hid-multitouch.o
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index 2b89c70..8470e22 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -1883,6 +1883,7 @@ static const struct hid_device_id hid_have_special_driver[] = {
>>       { HID_USB_DEVICE(USB_VENDOR_ID_DELCOM, USB_DEVICE_ID_DELCOM_VISUAL_IND) },
>>       { HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, 0x0006) },
>>       { HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, 0x0011) },
>> +     { HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, USB_DEVICE_ID_DRAGONRISE_PS3) },
>>       { HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, USB_DEVICE_ID_DREAM_CHEEKY_WN) },
>>       { HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, USB_DEVICE_ID_DREAM_CHEEKY_FA) },
>>       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_BM084) },
>> diff --git a/drivers/hid/hid-mf.c b/drivers/hid/hid-mf.c
>> new file mode 100644
>> index 0000000..6791ce7
>> --- /dev/null
>> +++ b/drivers/hid/hid-mf.c
>> @@ -0,0 +1,165 @@
>> +/*
>> + * Force feedback support for Mayflash game controller adapters.
>> + *
>> + * These devices are manufactured by Mayflash but identify themselves
>> + * using the vendor ID of DragonRise Inc.
>> + *
>> + * Tested with:
>> + * 0079:1801 "DragonRise Inc. Mayflash PS3 Game Controller Adapter"
>> + *
>> + * The following adapters probably work too, but need to be tested:
>> + * 0079:1800 "DragonRise Inc. Mayflash WIIU Game Controller Adapter"
>> + * 0079:1843 "DragonRise Inc. Mayflash GameCube Game Controller Adapter"
>> + *
>> + * Copyright (c) 2016 Marcel Hasler <mahasler@gmail.com>
>> + */
>> +
>> +/*
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/input.h>
>> +#include <linux/slab.h>
>> +#include <linux/hid.h>
>> +#include <linux/module.h>
>> +
>> +#include "hid-ids.h"
>> +
>> +struct mf_device {
>> +     struct hid_report *report;
>> +};
>> +
>> +static int mf_play(struct input_dev *dev, void *data, struct ff_effect *effect)
>> +{
>> +     struct hid_device *hid = input_get_drvdata(dev);
>> +     struct mf_device *mf = data;
>> +     int strong, weak;
>> +
>> +     strong = effect->u.rumble.strong_magnitude;
>> +     weak = effect->u.rumble.weak_magnitude;
>> +
>> +     dbg_hid("Called with 0x%04x 0x%04x.\n", strong, weak);
>> +
>> +     strong = strong * 0xff / 0xffff;
>> +     weak = weak * 0xff / 0xffff;
>> +
>> +     dbg_hid("Running with 0x%02x 0x%02x.\n", strong, weak);
>> +
>> +     mf->report->field[0]->value[0] = weak;
>> +     mf->report->field[0]->value[1] = strong;
>> +     hid_hw_request(hid, mf->report, HID_REQ_SET_REPORT);
>> +
>> +     return 0;
>> +}
>> +
>> +static int mf_init(struct hid_device *hid)
>> +{
>> +     struct mf_device *mf;
>> +
>> +     struct list_head *report_list =
>> +                     &hid->report_enum[HID_OUTPUT_REPORT].report_list;
>> +
>> +     struct list_head *report_ptr;
>> +     struct hid_report *report;
>> +
>> +     struct hid_input *input =
>> +                     list_first_entry(&hid->inputs, struct hid_input, list);
>> +
>> +     struct input_dev *dev;
>> +
>> +     int error;
>> +
>> +     /* Setup each of the four inputs */
>> +     list_for_each(report_ptr, report_list) {
>> +             report = list_entry(report_ptr, struct hid_report, list);
>> +
>> +             if (report->maxfield < 1 || report->field[0]->report_count < 2) {
>> +                     hid_err(hid, "Invalid report, this should never happen!\n");
>> +                     return -ENODEV;
>> +             }
>> +
>> +             if (input == NULL) {
>> +                     hid_err(hid, "Missing input, this should never happen!\n");
>> +                     return -ENODEV;
>> +             }
>> +
>> +             dev = input->input;
>> +             input = list_next_entry(input, list);
>
> Wouldn't it make more sense to use .input_configured instead of this
> after-the-fact loop?
>
>> +
>> +             mf = kzalloc(sizeof(struct mf_device), GFP_KERNEL);
>
> I might not be fully awoken, but where is the corresponding kfree?
> If you do not want to manually free the allocated memory, you can use
> devm_kzalloc, as long as there is no races when removing the device.
>

The corresponding kfree for mf in this case is in ml_ff_destroy().

>> +             if (!mf)
>> +                     return -ENOMEM;
>> +
>> +             set_bit(FF_RUMBLE, dev->ffbit);
>> +
>> +             error = input_ff_create_memless(dev, mf, mf_play);
>> +             if (error) {
>> +                     kfree(mf);
>> +                     return error;
>> +             }
>> +
>> +             mf->report = report;
>> +             mf->report->field[0]->value[0] = 0x00;
>> +             mf->report->field[0]->value[1] = 0x00;
>> +             hid_hw_request(hid, mf->report, HID_REQ_SET_REPORT);
>> +     }
>> +
>> +     hid_info(hid, "Force feedback for HJZ Mayflash game controller "
>> +                   "adapters by Marcel Hasler <mahasler@gmail.com>\n");
>> +
>> +     return 0;
>> +}
>> +
>> +static int mf_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> +{
>> +     int error;
>> +
>> +     dev_dbg(&hdev->dev, "Mayflash HID hardware probe...\n");
>> +
>> +     /* Split device into four inputs */
>> +     hdev->quirks |= HID_QUIRK_MULTI_INPUT;
>
> Looks like the entry in the previous patch was not required apparently
> :)
>
>> +
>> +     error = hid_parse(hdev);
>> +     if (error) {
>> +             hid_err(hdev, "HID parse failed.\n");
>> +             return error;
>> +     }
>> +
>> +     error = hid_hw_start(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_FF);
>> +     if (error) {
>> +             hid_err(hdev, "HID hw start failed\n");
>> +             return error;
>> +     }
>> +
>> +     error = mf_init(hdev);
>
> This seems completely racy. hid_hw_start() exports the input nodes to
> user-space and the ff initialization is done after. It would be much
> better to initialize the ff part in .input_configured when the device
> hasn't been registered to user space yet.
>
> Cheers,
> Benjamin
>
>> +     if (error) {
>> +             hid_err(hdev, "Force feedback init failed.\n");
>> +             hid_hw_stop(hdev);
>> +             return error;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static const struct hid_device_id mf_devices[] = {
>> +     { HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, USB_DEVICE_ID_DRAGONRISE_PS3),  },
>> +     { }
>> +};
>> +MODULE_DEVICE_TABLE(hid, mf_devices);
>> +
>> +static struct hid_driver mf_driver = {
>> +     .name = "hid_mf",
>> +     .id_table = mf_devices,
>> +     .probe = mf_probe,
>> +};
>> +module_hid_driver(mf_driver);
>> +
>> +MODULE_LICENSE("GPL");
>> --
>> 2.10.1
>>

Thanks for the hints. I tried your idea with .input_configured but
there's a bit of a problem with that. For each of the four gamepads
two input device nodes are created, one for the buttons/axes and one
for force feedback. Unfortunately systemd only assigns the first nodes
the uaccess tag. Now, I looked at the other ff drivers and essentially
followed their example. None of these use .input_configured, but
instead just use the very first hid_input and make ff available
through that. This works and actually has the desired effect that
normal users can use force feedback without having to be in the
'input' group (which as I understand is deprecated).

So while I completely agree that using .input_configured would be the
proper thing to do, I think either all ff drivers should use it (and
systemd should be fixed to make those input nodes accessible) or
hid-mf should do what all others do (at least for the time being).

Best regards
Marcel

Re: [PATCH 2/2] Add new force feedback driver for Mayflash game controller adapters.

[Benjamin Tissoires]

On Oct 31 2016 or thereabouts, Marcel Hasler wrote:
> Hi
> 
> 2016-10-31 12:17 GMT+01:00 Benjamin Tissoires <benjamin.tissoires@redhat.com>:
> > On Oct 30 2016 or thereabouts, Marcel Hasler wrote:
> >> Add a new module named hid-mf that implements force feedback for game controller adapters
> >> manufactured by Mayflash. Currently only the PS3 adapter is supported, other adapters still need
> >> to be tested.
> >>
> >> Signed-off-by: Marcel Hasler <mahasler@gmail.com>
> >> ---
> >>  drivers/hid/Kconfig    |   8 +++
> >>  drivers/hid/Makefile   |   1 +
> >>  drivers/hid/hid-core.c |   1 +
> >>  drivers/hid/hid-mf.c   | 165 +++++++++++++++++++++++++++++++++++++++++++++++++
> >>  4 files changed, 175 insertions(+)
> >>  create mode 100644 drivers/hid/hid-mf.c
> >>
> >> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> >> index cd4599c..1530d28 100644
> >> --- a/drivers/hid/Kconfig
> >> +++ b/drivers/hid/Kconfig
> >> @@ -512,6 +512,14 @@ config HID_MAGICMOUSE
> >>       Say Y here if you want support for the multi-touch features of the
> >>       Apple Wireless "Magic" Mouse and the Apple Wireless "Magic" Trackpad.
> >>
> >> +config HID_MAYFLASH
> >> +     tristate "Mayflash game controller adapter force feedback"
> >> +     depends on HID
> >> +     select INPUT_FF_MEMLESS
> >> +     ---help---
> >> +     Say Y here if you have HJZ Mayflash PS3 game controller adapters
> >> +     and want to enable force feedback support.
> >> +
> >>  config HID_MICROSOFT
> >>       tristate "Microsoft non-fully HID-compliant devices"
> >>       depends on HID
> >> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> >> index 86b2b57..c0453f1 100644
> >> --- a/drivers/hid/Makefile
> >> +++ b/drivers/hid/Makefile
> >> @@ -58,6 +58,7 @@ obj-$(CONFIG_HID_LOGITECH)  += hid-logitech.o
> >>  obj-$(CONFIG_HID_LOGITECH_DJ)        += hid-logitech-dj.o
> >>  obj-$(CONFIG_HID_LOGITECH_HIDPP)     += hid-logitech-hidpp.o
> >>  obj-$(CONFIG_HID_MAGICMOUSE) += hid-magicmouse.o
> >> +obj-$(CONFIG_HID_MAYFLASH)   += hid-mf.o
> >>  obj-$(CONFIG_HID_MICROSOFT)  += hid-microsoft.o
> >>  obj-$(CONFIG_HID_MONTEREY)   += hid-monterey.o
> >>  obj-$(CONFIG_HID_MULTITOUCH) += hid-multitouch.o
> >> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> >> index 2b89c70..8470e22 100644
> >> --- a/drivers/hid/hid-core.c
> >> +++ b/drivers/hid/hid-core.c
> >> @@ -1883,6 +1883,7 @@ static const struct hid_device_id hid_have_special_driver[] = {
> >>       { HID_USB_DEVICE(USB_VENDOR_ID_DELCOM, USB_DEVICE_ID_DELCOM_VISUAL_IND) },
> >>       { HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, 0x0006) },
> >>       { HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, 0x0011) },
> >> +     { HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, USB_DEVICE_ID_DRAGONRISE_PS3) },
> >>       { HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, USB_DEVICE_ID_DREAM_CHEEKY_WN) },
> >>       { HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, USB_DEVICE_ID_DREAM_CHEEKY_FA) },
> >>       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_BM084) },
> >> diff --git a/drivers/hid/hid-mf.c b/drivers/hid/hid-mf.c
> >> new file mode 100644
> >> index 0000000..6791ce7
> >> --- /dev/null
> >> +++ b/drivers/hid/hid-mf.c
> >> @@ -0,0 +1,165 @@
> >> +/*
> >> + * Force feedback support for Mayflash game controller adapters.
> >> + *
> >> + * These devices are manufactured by Mayflash but identify themselves
> >> + * using the vendor ID of DragonRise Inc.
> >> + *
> >> + * Tested with:
> >> + * 0079:1801 "DragonRise Inc. Mayflash PS3 Game Controller Adapter"
> >> + *
> >> + * The following adapters probably work too, but need to be tested:
> >> + * 0079:1800 "DragonRise Inc. Mayflash WIIU Game Controller Adapter"
> >> + * 0079:1843 "DragonRise Inc. Mayflash GameCube Game Controller Adapter"
> >> + *
> >> + * Copyright (c) 2016 Marcel Hasler <mahasler@gmail.com>
> >> + */
> >> +
> >> +/*
> >> + * This program is free software; you can redistribute it and/or modify
> >> + * it under the terms of the GNU General Public License as published by
> >> + * the Free Software Foundation; either version 2 of the License, or
> >> + * (at your option) any later version.
> >> + *
> >> + * This program is distributed in the hope that it will be useful,
> >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >> + * GNU General Public License for more details.
> >> + */
> >> +
> >> +#include <linux/input.h>
> >> +#include <linux/slab.h>
> >> +#include <linux/hid.h>
> >> +#include <linux/module.h>
> >> +
> >> +#include "hid-ids.h"
> >> +
> >> +struct mf_device {
> >> +     struct hid_report *report;
> >> +};
> >> +
> >> +static int mf_play(struct input_dev *dev, void *data, struct ff_effect *effect)
> >> +{
> >> +     struct hid_device *hid = input_get_drvdata(dev);
> >> +     struct mf_device *mf = data;
> >> +     int strong, weak;
> >> +
> >> +     strong = effect->u.rumble.strong_magnitude;
> >> +     weak = effect->u.rumble.weak_magnitude;
> >> +
> >> +     dbg_hid("Called with 0x%04x 0x%04x.\n", strong, weak);
> >> +
> >> +     strong = strong * 0xff / 0xffff;
> >> +     weak = weak * 0xff / 0xffff;
> >> +
> >> +     dbg_hid("Running with 0x%02x 0x%02x.\n", strong, weak);
> >> +
> >> +     mf->report->field[0]->value[0] = weak;
> >> +     mf->report->field[0]->value[1] = strong;
> >> +     hid_hw_request(hid, mf->report, HID_REQ_SET_REPORT);
> >> +
> >> +     return 0;
> >> +}
> >> +
> >> +static int mf_init(struct hid_device *hid)
> >> +{
> >> +     struct mf_device *mf;
> >> +
> >> +     struct list_head *report_list =
> >> +                     &hid->report_enum[HID_OUTPUT_REPORT].report_list;
> >> +
> >> +     struct list_head *report_ptr;
> >> +     struct hid_report *report;
> >> +
> >> +     struct hid_input *input =
> >> +                     list_first_entry(&hid->inputs, struct hid_input, list);
> >> +
> >> +     struct input_dev *dev;
> >> +
> >> +     int error;
> >> +
> >> +     /* Setup each of the four inputs */
> >> +     list_for_each(report_ptr, report_list) {
> >> +             report = list_entry(report_ptr, struct hid_report, list);
> >> +
> >> +             if (report->maxfield < 1 || report->field[0]->report_count < 2) {
> >> +                     hid_err(hid, "Invalid report, this should never happen!\n");
> >> +                     return -ENODEV;
> >> +             }
> >> +
> >> +             if (input == NULL) {
> >> +                     hid_err(hid, "Missing input, this should never happen!\n");
> >> +                     return -ENODEV;
> >> +             }
> >> +
> >> +             dev = input->input;
> >> +             input = list_next_entry(input, list);
> >
> > Wouldn't it make more sense to use .input_configured instead of this
> > after-the-fact loop?
> >
> >> +
> >> +             mf = kzalloc(sizeof(struct mf_device), GFP_KERNEL);
> >
> > I might not be fully awoken, but where is the corresponding kfree?
> > If you do not want to manually free the allocated memory, you can use
> > devm_kzalloc, as long as there is no races when removing the device.
> >
> 
> The corresponding kfree for mf in this case is in ml_ff_destroy().

K, thanks. I should definitively have taken more attention while reading
the patch.

> 
> >> +             if (!mf)
> >> +                     return -ENOMEM;
> >> +
> >> +             set_bit(FF_RUMBLE, dev->ffbit);
> >> +
> >> +             error = input_ff_create_memless(dev, mf, mf_play);
> >> +             if (error) {
> >> +                     kfree(mf);
> >> +                     return error;
> >> +             }
> >> +
> >> +             mf->report = report;
> >> +             mf->report->field[0]->value[0] = 0x00;
> >> +             mf->report->field[0]->value[1] = 0x00;
> >> +             hid_hw_request(hid, mf->report, HID_REQ_SET_REPORT);
> >> +     }
> >> +
> >> +     hid_info(hid, "Force feedback for HJZ Mayflash game controller "
> >> +                   "adapters by Marcel Hasler <mahasler@gmail.com>\n");
> >> +
> >> +     return 0;
> >> +}
> >> +
> >> +static int mf_probe(struct hid_device *hdev, const struct hid_device_id *id)
> >> +{
> >> +     int error;
> >> +
> >> +     dev_dbg(&hdev->dev, "Mayflash HID hardware probe...\n");
> >> +
> >> +     /* Split device into four inputs */
> >> +     hdev->quirks |= HID_QUIRK_MULTI_INPUT;
> >
> > Looks like the entry in the previous patch was not required apparently
> > :)
> >
> >> +
> >> +     error = hid_parse(hdev);
> >> +     if (error) {
> >> +             hid_err(hdev, "HID parse failed.\n");
> >> +             return error;
> >> +     }
> >> +
> >> +     error = hid_hw_start(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_FF);
> >> +     if (error) {
> >> +             hid_err(hdev, "HID hw start failed\n");
> >> +             return error;
> >> +     }
> >> +
> >> +     error = mf_init(hdev);
> >
> > This seems completely racy. hid_hw_start() exports the input nodes to
> > user-space and the ff initialization is done after. It would be much
> > better to initialize the ff part in .input_configured when the device
> > hasn't been registered to user space yet.
> >
> > Cheers,
> > Benjamin
> >
> >> +     if (error) {
> >> +             hid_err(hdev, "Force feedback init failed.\n");
> >> +             hid_hw_stop(hdev);
> >> +             return error;
> >> +     }
> >> +
> >> +     return 0;
> >> +}
> >> +
> >> +static const struct hid_device_id mf_devices[] = {
> >> +     { HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, USB_DEVICE_ID_DRAGONRISE_PS3),  },
> >> +     { }
> >> +};
> >> +MODULE_DEVICE_TABLE(hid, mf_devices);
> >> +
> >> +static struct hid_driver mf_driver = {
> >> +     .name = "hid_mf",
> >> +     .id_table = mf_devices,
> >> +     .probe = mf_probe,
> >> +};
> >> +module_hid_driver(mf_driver);
> >> +
> >> +MODULE_LICENSE("GPL");
> >> --
> >> 2.10.1
> >>
> 
> Thanks for the hints. I tried your idea with .input_configured but
> there's a bit of a problem with that. For each of the four gamepads
> two input device nodes are created, one for the buttons/axes and one
> for force feedback. Unfortunately systemd only assigns the first nodes
> the uaccess tag. Now, I looked at the other ff drivers and essentially
> followed their example. None of these use .input_configured, but
> instead just use the very first hid_input and make ff available
> through that. This works and actually has the desired effect that
> normal users can use force feedback without having to be in the
> 'input' group (which as I understand is deprecated).

I haven't taken much attention to the ff implementation both in the
kernel and in userspace. After your explanation. I am not sure I want to
spend more time on it though :)

> 
> So while I completely agree that using .input_configured would be the
> proper thing to do, I think either all ff drivers should use it (and
> systemd should be fixed to make those input nodes accessible) or
> hid-mf should do what all others do (at least for the time being).

Yes, please stick to what other kernel drivers do, and hope for the
best. If I understand correctly, you'll need to remove the for loop and
only check for the first available input node, right?

Cheers,
Benjamin

> 
> Best regards
> Marcel

Re: [PATCH 2/2] Add new force feedback driver for Mayflash game controller adapters.

[mahasler]

On Mon, Oct 31, 2016 at 02:48:46PM +0100, Benjamin Tissoires wrote:
> On Oct 31 2016 or thereabouts, Marcel Hasler wrote:
> > Hi
> > 
> > 2016-10-31 12:17 GMT+01:00 Benjamin Tissoires <benjamin.tissoires@redhat.com>:
> > > On Oct 30 2016 or thereabouts, Marcel Hasler wrote:
> > >> Add a new module named hid-mf that implements force feedback for game controller adapters
> > >> manufactured by Mayflash. Currently only the PS3 adapter is supported, other adapters still need
> > >> to be tested.
> > >>
> > >> Signed-off-by: Marcel Hasler <mahasler@gmail.com>
> > >> ---
> > >>  drivers/hid/Kconfig    |   8 +++
> > >>  drivers/hid/Makefile   |   1 +
> > >>  drivers/hid/hid-core.c |   1 +
> > >>  drivers/hid/hid-mf.c   | 165 +++++++++++++++++++++++++++++++++++++++++++++++++
> > >>  4 files changed, 175 insertions(+)
> > >>  create mode 100644 drivers/hid/hid-mf.c
> > >>
> > >> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> > >> index cd4599c..1530d28 100644
> > >> --- a/drivers/hid/Kconfig
> > >> +++ b/drivers/hid/Kconfig
> > >> @@ -512,6 +512,14 @@ config HID_MAGICMOUSE
> > >>       Say Y here if you want support for the multi-touch features of the
> > >>       Apple Wireless "Magic" Mouse and the Apple Wireless "Magic" Trackpad.
> > >>
> > >> +config HID_MAYFLASH
> > >> +     tristate "Mayflash game controller adapter force feedback"
> > >> +     depends on HID
> > >> +     select INPUT_FF_MEMLESS
> > >> +     ---help---
> > >> +     Say Y here if you have HJZ Mayflash PS3 game controller adapters
> > >> +     and want to enable force feedback support.
> > >> +
> > >>  config HID_MICROSOFT
> > >>       tristate "Microsoft non-fully HID-compliant devices"
> > >>       depends on HID
> > >> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> > >> index 86b2b57..c0453f1 100644
> > >> --- a/drivers/hid/Makefile
> > >> +++ b/drivers/hid/Makefile
> > >> @@ -58,6 +58,7 @@ obj-$(CONFIG_HID_LOGITECH)  += hid-logitech.o
> > >>  obj-$(CONFIG_HID_LOGITECH_DJ)        += hid-logitech-dj.o
> > >>  obj-$(CONFIG_HID_LOGITECH_HIDPP)     += hid-logitech-hidpp.o
> > >>  obj-$(CONFIG_HID_MAGICMOUSE) += hid-magicmouse.o
> > >> +obj-$(CONFIG_HID_MAYFLASH)   += hid-mf.o
> > >>  obj-$(CONFIG_HID_MICROSOFT)  += hid-microsoft.o
> > >>  obj-$(CONFIG_HID_MONTEREY)   += hid-monterey.o
> > >>  obj-$(CONFIG_HID_MULTITOUCH) += hid-multitouch.o
> > >> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > >> index 2b89c70..8470e22 100644
> > >> --- a/drivers/hid/hid-core.c
> > >> +++ b/drivers/hid/hid-core.c
> > >> @@ -1883,6 +1883,7 @@ static const struct hid_device_id hid_have_special_driver[] = {
> > >>       { HID_USB_DEVICE(USB_VENDOR_ID_DELCOM, USB_DEVICE_ID_DELCOM_VISUAL_IND) },
> > >>       { HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, 0x0006) },
> > >>       { HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, 0x0011) },
> > >> +     { HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, USB_DEVICE_ID_DRAGONRISE_PS3) },
> > >>       { HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, USB_DEVICE_ID_DREAM_CHEEKY_WN) },
> > >>       { HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, USB_DEVICE_ID_DREAM_CHEEKY_FA) },
> > >>       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_BM084) },
> > >> diff --git a/drivers/hid/hid-mf.c b/drivers/hid/hid-mf.c
> > >> new file mode 100644
> > >> index 0000000..6791ce7
> > >> --- /dev/null
> > >> +++ b/drivers/hid/hid-mf.c
> > >> @@ -0,0 +1,165 @@
> > >> +/*
> > >> + * Force feedback support for Mayflash game controller adapters.
> > >> + *
> > >> + * These devices are manufactured by Mayflash but identify themselves
> > >> + * using the vendor ID of DragonRise Inc.
> > >> + *
> > >> + * Tested with:
> > >> + * 0079:1801 "DragonRise Inc. Mayflash PS3 Game Controller Adapter"
> > >> + *
> > >> + * The following adapters probably work too, but need to be tested:
> > >> + * 0079:1800 "DragonRise Inc. Mayflash WIIU Game Controller Adapter"
> > >> + * 0079:1843 "DragonRise Inc. Mayflash GameCube Game Controller Adapter"
> > >> + *
> > >> + * Copyright (c) 2016 Marcel Hasler <mahasler@gmail.com>
> > >> + */
> > >> +
> > >> +/*
> > >> + * This program is free software; you can redistribute it and/or modify
> > >> + * it under the terms of the GNU General Public License as published by
> > >> + * the Free Software Foundation; either version 2 of the License, or
> > >> + * (at your option) any later version.
> > >> + *
> > >> + * This program is distributed in the hope that it will be useful,
> > >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > >> + * GNU General Public License for more details.
> > >> + */
> > >> +
> > >> +#include <linux/input.h>
> > >> +#include <linux/slab.h>
> > >> +#include <linux/hid.h>
> > >> +#include <linux/module.h>
> > >> +
> > >> +#include "hid-ids.h"
> > >> +
> > >> +struct mf_device {
> > >> +     struct hid_report *report;
> > >> +};
> > >> +
> > >> +static int mf_play(struct input_dev *dev, void *data, struct ff_effect *effect)
> > >> +{
> > >> +     struct hid_device *hid = input_get_drvdata(dev);
> > >> +     struct mf_device *mf = data;
> > >> +     int strong, weak;
> > >> +
> > >> +     strong = effect->u.rumble.strong_magnitude;
> > >> +     weak = effect->u.rumble.weak_magnitude;
> > >> +
> > >> +     dbg_hid("Called with 0x%04x 0x%04x.\n", strong, weak);
> > >> +
> > >> +     strong = strong * 0xff / 0xffff;
> > >> +     weak = weak * 0xff / 0xffff;
> > >> +
> > >> +     dbg_hid("Running with 0x%02x 0x%02x.\n", strong, weak);
> > >> +
> > >> +     mf->report->field[0]->value[0] = weak;
> > >> +     mf->report->field[0]->value[1] = strong;
> > >> +     hid_hw_request(hid, mf->report, HID_REQ_SET_REPORT);
> > >> +
> > >> +     return 0;
> > >> +}
> > >> +
> > >> +static int mf_init(struct hid_device *hid)
> > >> +{
> > >> +     struct mf_device *mf;
> > >> +
> > >> +     struct list_head *report_list =
> > >> +                     &hid->report_enum[HID_OUTPUT_REPORT].report_list;
> > >> +
> > >> +     struct list_head *report_ptr;
> > >> +     struct hid_report *report;
> > >> +
> > >> +     struct hid_input *input =
> > >> +                     list_first_entry(&hid->inputs, struct hid_input, list);
> > >> +
> > >> +     struct input_dev *dev;
> > >> +
> > >> +     int error;
> > >> +
> > >> +     /* Setup each of the four inputs */
> > >> +     list_for_each(report_ptr, report_list) {
> > >> +             report = list_entry(report_ptr, struct hid_report, list);
> > >> +
> > >> +             if (report->maxfield < 1 || report->field[0]->report_count < 2) {
> > >> +                     hid_err(hid, "Invalid report, this should never happen!\n");
> > >> +                     return -ENODEV;
> > >> +             }
> > >> +
> > >> +             if (input == NULL) {
> > >> +                     hid_err(hid, "Missing input, this should never happen!\n");
> > >> +                     return -ENODEV;
> > >> +             }
> > >> +
> > >> +             dev = input->input;
> > >> +             input = list_next_entry(input, list);
> > >
> > > Wouldn't it make more sense to use .input_configured instead of this
> > > after-the-fact loop?
> > >
> > >> +
> > >> +             mf = kzalloc(sizeof(struct mf_device), GFP_KERNEL);
> > >
> > > I might not be fully awoken, but where is the corresponding kfree?
> > > If you do not want to manually free the allocated memory, you can use
> > > devm_kzalloc, as long as there is no races when removing the device.
> > >
> > 
> > The corresponding kfree for mf in this case is in ml_ff_destroy().
> 
> K, thanks. I should definitively have taken more attention while reading
> the patch.
> 
> > 
> > >> +             if (!mf)
> > >> +                     return -ENOMEM;
> > >> +
> > >> +             set_bit(FF_RUMBLE, dev->ffbit);
> > >> +
> > >> +             error = input_ff_create_memless(dev, mf, mf_play);
> > >> +             if (error) {
> > >> +                     kfree(mf);
> > >> +                     return error;
> > >> +             }
> > >> +
> > >> +             mf->report = report;
> > >> +             mf->report->field[0]->value[0] = 0x00;
> > >> +             mf->report->field[0]->value[1] = 0x00;
> > >> +             hid_hw_request(hid, mf->report, HID_REQ_SET_REPORT);
> > >> +     }
> > >> +
> > >> +     hid_info(hid, "Force feedback for HJZ Mayflash game controller "
> > >> +                   "adapters by Marcel Hasler <mahasler@gmail.com>\n");
> > >> +
> > >> +     return 0;
> > >> +}
> > >> +
> > >> +static int mf_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > >> +{
> > >> +     int error;
> > >> +
> > >> +     dev_dbg(&hdev->dev, "Mayflash HID hardware probe...\n");
> > >> +
> > >> +     /* Split device into four inputs */
> > >> +     hdev->quirks |= HID_QUIRK_MULTI_INPUT;
> > >
> > > Looks like the entry in the previous patch was not required apparently
> > > :)
> > >
> > >> +
> > >> +     error = hid_parse(hdev);
> > >> +     if (error) {
> > >> +             hid_err(hdev, "HID parse failed.\n");
> > >> +             return error;
> > >> +     }
> > >> +
> > >> +     error = hid_hw_start(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_FF);
> > >> +     if (error) {
> > >> +             hid_err(hdev, "HID hw start failed\n");
> > >> +             return error;
> > >> +     }
> > >> +
> > >> +     error = mf_init(hdev);
> > >
> > > This seems completely racy. hid_hw_start() exports the input nodes to
> > > user-space and the ff initialization is done after. It would be much
> > > better to initialize the ff part in .input_configured when the device
> > > hasn't been registered to user space yet.
> > >
> > > Cheers,
> > > Benjamin
> > >
> > >> +     if (error) {
> > >> +             hid_err(hdev, "Force feedback init failed.\n");
> > >> +             hid_hw_stop(hdev);
> > >> +             return error;
> > >> +     }
> > >> +
> > >> +     return 0;
> > >> +}
> > >> +
> > >> +static const struct hid_device_id mf_devices[] = {
> > >> +     { HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, USB_DEVICE_ID_DRAGONRISE_PS3),  },
> > >> +     { }
> > >> +};
> > >> +MODULE_DEVICE_TABLE(hid, mf_devices);
> > >> +
> > >> +static struct hid_driver mf_driver = {
> > >> +     .name = "hid_mf",
> > >> +     .id_table = mf_devices,
> > >> +     .probe = mf_probe,
> > >> +};
> > >> +module_hid_driver(mf_driver);
> > >> +
> > >> +MODULE_LICENSE("GPL");
> > >> --
> > >> 2.10.1
> > >>
> > 
> > Thanks for the hints. I tried your idea with .input_configured but
> > there's a bit of a problem with that. For each of the four gamepads
> > two input device nodes are created, one for the buttons/axes and one
> > for force feedback. Unfortunately systemd only assigns the first nodes
> > the uaccess tag. Now, I looked at the other ff drivers and essentially
> > followed their example. None of these use .input_configured, but
> > instead just use the very first hid_input and make ff available
> > through that. This works and actually has the desired effect that
> > normal users can use force feedback without having to be in the
> > 'input' group (which as I understand is deprecated).
> 
> I haven't taken much attention to the ff implementation both in the
> kernel and in userspace. After your explanation. I am not sure I want to
> spend more time on it though :)
> 
> > 
> > So while I completely agree that using .input_configured would be the
> > proper thing to do, I think either all ff drivers should use it (and
> > systemd should be fixed to make those input nodes accessible) or
> > hid-mf should do what all others do (at least for the time being).
> 
> Yes, please stick to what other kernel drivers do, and hope for the
> best. If I understand correctly, you'll need to remove the for loop and
> only check for the first available input node, right?
> 
> Cheers,
> Benjamin
> 
> > 
> > Best regards
> > Marcel

In this case the outer loop is needed because otherwise only one of the four possible gamepads 
would get force feedback. Unfortunately I don't know of any elegant way to map a ff hidinput to the 
corresponding button/axis hidinput. That's why I essentially have to loop over two lists. If 
anyone can think of a better way, I'd be happy to change that.

I did just now revert a small change that I made yesterday shortly before submitting the patch. 
Actually the change was to get rid of a 80-character warning from checkpatch.pl but I think 
I'd prefer clean code and ignore that warning.

I'll submit an updated version in a moment.

About the quirk, I would propse to leave it in since the usbhid driver still needs it to work 
properly. If e.g. someone builds a kernel without hid-mf, the device should still work under 
usbhid (just without ff).

Regards
Marcel

Re: [PATCH 2/2] Add new force feedback driver for Mayflash game controller adapters.

[Benjamin Tissoires]

On Oct 31 2016 or thereabouts, mahasler@gmail.com wrote:
> On Mon, Oct 31, 2016 at 02:48:46PM +0100, Benjamin Tissoires wrote:
> > On Oct 31 2016 or thereabouts, Marcel Hasler wrote:
> > > Hi
> > > 
> > > 2016-10-31 12:17 GMT+01:00 Benjamin Tissoires <benjamin.tissoires@redhat.com>:
> > > > On Oct 30 2016 or thereabouts, Marcel Hasler wrote:
> > > >> Add a new module named hid-mf that implements force feedback for game controller adapters
> > > >> manufactured by Mayflash. Currently only the PS3 adapter is supported, other adapters still need
> > > >> to be tested.
> > > >>
> > > >> Signed-off-by: Marcel Hasler <mahasler@gmail.com>
> > > >> ---
> > > >>  drivers/hid/Kconfig    |   8 +++
> > > >>  drivers/hid/Makefile   |   1 +
> > > >>  drivers/hid/hid-core.c |   1 +
> > > >>  drivers/hid/hid-mf.c   | 165 +++++++++++++++++++++++++++++++++++++++++++++++++
> > > >>  4 files changed, 175 insertions(+)
> > > >>  create mode 100644 drivers/hid/hid-mf.c
> > > >>
> > > >> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> > > >> index cd4599c..1530d28 100644
> > > >> --- a/drivers/hid/Kconfig
> > > >> +++ b/drivers/hid/Kconfig
> > > >> @@ -512,6 +512,14 @@ config HID_MAGICMOUSE
> > > >>       Say Y here if you want support for the multi-touch features of the
> > > >>       Apple Wireless "Magic" Mouse and the Apple Wireless "Magic" Trackpad.
> > > >>
> > > >> +config HID_MAYFLASH
> > > >> +     tristate "Mayflash game controller adapter force feedback"
> > > >> +     depends on HID
> > > >> +     select INPUT_FF_MEMLESS
> > > >> +     ---help---
> > > >> +     Say Y here if you have HJZ Mayflash PS3 game controller adapters
> > > >> +     and want to enable force feedback support.
> > > >> +
> > > >>  config HID_MICROSOFT
> > > >>       tristate "Microsoft non-fully HID-compliant devices"
> > > >>       depends on HID
> > > >> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> > > >> index 86b2b57..c0453f1 100644
> > > >> --- a/drivers/hid/Makefile
> > > >> +++ b/drivers/hid/Makefile
> > > >> @@ -58,6 +58,7 @@ obj-$(CONFIG_HID_LOGITECH)  += hid-logitech.o
> > > >>  obj-$(CONFIG_HID_LOGITECH_DJ)        += hid-logitech-dj.o
> > > >>  obj-$(CONFIG_HID_LOGITECH_HIDPP)     += hid-logitech-hidpp.o
> > > >>  obj-$(CONFIG_HID_MAGICMOUSE) += hid-magicmouse.o
> > > >> +obj-$(CONFIG_HID_MAYFLASH)   += hid-mf.o
> > > >>  obj-$(CONFIG_HID_MICROSOFT)  += hid-microsoft.o
> > > >>  obj-$(CONFIG_HID_MONTEREY)   += hid-monterey.o
> > > >>  obj-$(CONFIG_HID_MULTITOUCH) += hid-multitouch.o
> > > >> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > > >> index 2b89c70..8470e22 100644
> > > >> --- a/drivers/hid/hid-core.c
> > > >> +++ b/drivers/hid/hid-core.c
> > > >> @@ -1883,6 +1883,7 @@ static const struct hid_device_id hid_have_special_driver[] = {
> > > >>       { HID_USB_DEVICE(USB_VENDOR_ID_DELCOM, USB_DEVICE_ID_DELCOM_VISUAL_IND) },
> > > >>       { HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, 0x0006) },
> > > >>       { HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, 0x0011) },
> > > >> +     { HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, USB_DEVICE_ID_DRAGONRISE_PS3) },
> > > >>       { HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, USB_DEVICE_ID_DREAM_CHEEKY_WN) },
> > > >>       { HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, USB_DEVICE_ID_DREAM_CHEEKY_FA) },
> > > >>       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_BM084) },
> > > >> diff --git a/drivers/hid/hid-mf.c b/drivers/hid/hid-mf.c
> > > >> new file mode 100644
> > > >> index 0000000..6791ce7
> > > >> --- /dev/null
> > > >> +++ b/drivers/hid/hid-mf.c
> > > >> @@ -0,0 +1,165 @@
> > > >> +/*
> > > >> + * Force feedback support for Mayflash game controller adapters.
> > > >> + *
> > > >> + * These devices are manufactured by Mayflash but identify themselves
> > > >> + * using the vendor ID of DragonRise Inc.
> > > >> + *
> > > >> + * Tested with:
> > > >> + * 0079:1801 "DragonRise Inc. Mayflash PS3 Game Controller Adapter"
> > > >> + *
> > > >> + * The following adapters probably work too, but need to be tested:
> > > >> + * 0079:1800 "DragonRise Inc. Mayflash WIIU Game Controller Adapter"
> > > >> + * 0079:1843 "DragonRise Inc. Mayflash GameCube Game Controller Adapter"
> > > >> + *
> > > >> + * Copyright (c) 2016 Marcel Hasler <mahasler@gmail.com>
> > > >> + */
> > > >> +
> > > >> +/*
> > > >> + * This program is free software; you can redistribute it and/or modify
> > > >> + * it under the terms of the GNU General Public License as published by
> > > >> + * the Free Software Foundation; either version 2 of the License, or
> > > >> + * (at your option) any later version.
> > > >> + *
> > > >> + * This program is distributed in the hope that it will be useful,
> > > >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > >> + * GNU General Public License for more details.
> > > >> + */
> > > >> +
> > > >> +#include <linux/input.h>
> > > >> +#include <linux/slab.h>
> > > >> +#include <linux/hid.h>
> > > >> +#include <linux/module.h>
> > > >> +
> > > >> +#include "hid-ids.h"
> > > >> +
> > > >> +struct mf_device {
> > > >> +     struct hid_report *report;
> > > >> +};
> > > >> +
> > > >> +static int mf_play(struct input_dev *dev, void *data, struct ff_effect *effect)
> > > >> +{
> > > >> +     struct hid_device *hid = input_get_drvdata(dev);
> > > >> +     struct mf_device *mf = data;
> > > >> +     int strong, weak;
> > > >> +
> > > >> +     strong = effect->u.rumble.strong_magnitude;
> > > >> +     weak = effect->u.rumble.weak_magnitude;
> > > >> +
> > > >> +     dbg_hid("Called with 0x%04x 0x%04x.\n", strong, weak);
> > > >> +
> > > >> +     strong = strong * 0xff / 0xffff;
> > > >> +     weak = weak * 0xff / 0xffff;
> > > >> +
> > > >> +     dbg_hid("Running with 0x%02x 0x%02x.\n", strong, weak);
> > > >> +
> > > >> +     mf->report->field[0]->value[0] = weak;
> > > >> +     mf->report->field[0]->value[1] = strong;
> > > >> +     hid_hw_request(hid, mf->report, HID_REQ_SET_REPORT);
> > > >> +
> > > >> +     return 0;
> > > >> +}
> > > >> +
> > > >> +static int mf_init(struct hid_device *hid)
> > > >> +{
> > > >> +     struct mf_device *mf;
> > > >> +
> > > >> +     struct list_head *report_list =
> > > >> +                     &hid->report_enum[HID_OUTPUT_REPORT].report_list;
> > > >> +
> > > >> +     struct list_head *report_ptr;
> > > >> +     struct hid_report *report;
> > > >> +
> > > >> +     struct hid_input *input =
> > > >> +                     list_first_entry(&hid->inputs, struct hid_input, list);
> > > >> +
> > > >> +     struct input_dev *dev;
> > > >> +
> > > >> +     int error;
> > > >> +
> > > >> +     /* Setup each of the four inputs */
> > > >> +     list_for_each(report_ptr, report_list) {
> > > >> +             report = list_entry(report_ptr, struct hid_report, list);
> > > >> +
> > > >> +             if (report->maxfield < 1 || report->field[0]->report_count < 2) {
> > > >> +                     hid_err(hid, "Invalid report, this should never happen!\n");
> > > >> +                     return -ENODEV;
> > > >> +             }
> > > >> +
> > > >> +             if (input == NULL) {
> > > >> +                     hid_err(hid, "Missing input, this should never happen!\n");
> > > >> +                     return -ENODEV;
> > > >> +             }
> > > >> +
> > > >> +             dev = input->input;
> > > >> +             input = list_next_entry(input, list);
> > > >
> > > > Wouldn't it make more sense to use .input_configured instead of this
> > > > after-the-fact loop?
> > > >
> > > >> +
> > > >> +             mf = kzalloc(sizeof(struct mf_device), GFP_KERNEL);
> > > >
> > > > I might not be fully awoken, but where is the corresponding kfree?
> > > > If you do not want to manually free the allocated memory, you can use
> > > > devm_kzalloc, as long as there is no races when removing the device.
> > > >
> > > 
> > > The corresponding kfree for mf in this case is in ml_ff_destroy().
> > 
> > K, thanks. I should definitively have taken more attention while reading
> > the patch.
> > 
> > > 
> > > >> +             if (!mf)
> > > >> +                     return -ENOMEM;
> > > >> +
> > > >> +             set_bit(FF_RUMBLE, dev->ffbit);
> > > >> +
> > > >> +             error = input_ff_create_memless(dev, mf, mf_play);
> > > >> +             if (error) {
> > > >> +                     kfree(mf);
> > > >> +                     return error;
> > > >> +             }
> > > >> +
> > > >> +             mf->report = report;
> > > >> +             mf->report->field[0]->value[0] = 0x00;
> > > >> +             mf->report->field[0]->value[1] = 0x00;
> > > >> +             hid_hw_request(hid, mf->report, HID_REQ_SET_REPORT);
> > > >> +     }
> > > >> +
> > > >> +     hid_info(hid, "Force feedback for HJZ Mayflash game controller "
> > > >> +                   "adapters by Marcel Hasler <mahasler@gmail.com>\n");
> > > >> +
> > > >> +     return 0;
> > > >> +}
> > > >> +
> > > >> +static int mf_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > > >> +{
> > > >> +     int error;
> > > >> +
> > > >> +     dev_dbg(&hdev->dev, "Mayflash HID hardware probe...\n");
> > > >> +
> > > >> +     /* Split device into four inputs */
> > > >> +     hdev->quirks |= HID_QUIRK_MULTI_INPUT;
> > > >
> > > > Looks like the entry in the previous patch was not required apparently
> > > > :)
> > > >
> > > >> +
> > > >> +     error = hid_parse(hdev);
> > > >> +     if (error) {
> > > >> +             hid_err(hdev, "HID parse failed.\n");
> > > >> +             return error;
> > > >> +     }
> > > >> +
> > > >> +     error = hid_hw_start(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_FF);
> > > >> +     if (error) {
> > > >> +             hid_err(hdev, "HID hw start failed\n");
> > > >> +             return error;
> > > >> +     }
> > > >> +
> > > >> +     error = mf_init(hdev);
> > > >
> > > > This seems completely racy. hid_hw_start() exports the input nodes to
> > > > user-space and the ff initialization is done after. It would be much
> > > > better to initialize the ff part in .input_configured when the device
> > > > hasn't been registered to user space yet.
> > > >
> > > > Cheers,
> > > > Benjamin
> > > >
> > > >> +     if (error) {
> > > >> +             hid_err(hdev, "Force feedback init failed.\n");
> > > >> +             hid_hw_stop(hdev);
> > > >> +             return error;
> > > >> +     }
> > > >> +
> > > >> +     return 0;
> > > >> +}
> > > >> +
> > > >> +static const struct hid_device_id mf_devices[] = {
> > > >> +     { HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, USB_DEVICE_ID_DRAGONRISE_PS3),  },
> > > >> +     { }
> > > >> +};
> > > >> +MODULE_DEVICE_TABLE(hid, mf_devices);
> > > >> +
> > > >> +static struct hid_driver mf_driver = {
> > > >> +     .name = "hid_mf",
> > > >> +     .id_table = mf_devices,
> > > >> +     .probe = mf_probe,
> > > >> +};
> > > >> +module_hid_driver(mf_driver);
> > > >> +
> > > >> +MODULE_LICENSE("GPL");
> > > >> --
> > > >> 2.10.1
> > > >>
> > > 
> > > Thanks for the hints. I tried your idea with .input_configured but
> > > there's a bit of a problem with that. For each of the four gamepads
> > > two input device nodes are created, one for the buttons/axes and one
> > > for force feedback. Unfortunately systemd only assigns the first nodes
> > > the uaccess tag. Now, I looked at the other ff drivers and essentially
> > > followed their example. None of these use .input_configured, but
> > > instead just use the very first hid_input and make ff available
> > > through that. This works and actually has the desired effect that
> > > normal users can use force feedback without having to be in the
> > > 'input' group (which as I understand is deprecated).
> > 
> > I haven't taken much attention to the ff implementation both in the
> > kernel and in userspace. After your explanation. I am not sure I want to
> > spend more time on it though :)
> > 
> > > 
> > > So while I completely agree that using .input_configured would be the
> > > proper thing to do, I think either all ff drivers should use it (and
> > > systemd should be fixed to make those input nodes accessible) or
> > > hid-mf should do what all others do (at least for the time being).
> > 
> > Yes, please stick to what other kernel drivers do, and hope for the
> > best. If I understand correctly, you'll need to remove the for loop and
> > only check for the first available input node, right?
> > 
> > Cheers,
> > Benjamin
> > 
> > > 
> > > Best regards
> > > Marcel
> 
> In this case the outer loop is needed because otherwise only one of the four possible gamepads 
> would get force feedback. Unfortunately I don't know of any elegant way to map a ff hidinput to the 
> corresponding button/axis hidinput. That's why I essentially have to loop over two lists. If 
> anyone can think of a better way, I'd be happy to change that.
> 
> I did just now revert a small change that I made yesterday shortly before submitting the patch. 
> Actually the change was to get rid of a 80-character warning from checkpatch.pl but I think 
> I'd prefer clean code and ignore that warning.
> 
> I'll submit an updated version in a moment.
> 
> About the quirk, I would propse to leave it in since the usbhid driver still needs it to work 
> properly. If e.g. someone builds a kernel without hid-mf, the device should still work under 
> usbhid (just without ff).

That won't do. The change in hid-core prevent hid-generic to map your
device. So if you disable hid-mf, no hid driver will get bound to the
device and no one will be able to use the device without loading a
specific HID module for it.

Cheers,
Benjamin

> 
> Regards
> Marcel

Re: [PATCH 2/2] Add new force feedback driver for Mayflash game controller adapters.

[Marcel Hasler]

2016-10-31 15:39 GMT+01:00 Benjamin Tissoires <benjamin.tissoires@redhat.com>:
> On Oct 31 2016 or thereabouts, mahasler@gmail.com wrote:
>> On Mon, Oct 31, 2016 at 02:48:46PM +0100, Benjamin Tissoires wrote:
>> > On Oct 31 2016 or thereabouts, Marcel Hasler wrote:
>> > > Hi
>> > >
>> > > 2016-10-31 12:17 GMT+01:00 Benjamin Tissoires <benjamin.tissoires@redhat.com>:
>> > > > On Oct 30 2016 or thereabouts, Marcel Hasler wrote:
>> > > >> Add a new module named hid-mf that implements force feedback for game controller adapters
>> > > >> manufactured by Mayflash. Currently only the PS3 adapter is supported, other adapters still need
>> > > >> to be tested.
>> > > >>
>> > > >> Signed-off-by: Marcel Hasler <mahasler@gmail.com>
>> > > >> ---
>> > > >>  drivers/hid/Kconfig    |   8 +++
>> > > >>  drivers/hid/Makefile   |   1 +
>> > > >>  drivers/hid/hid-core.c |   1 +
>> > > >>  drivers/hid/hid-mf.c   | 165 +++++++++++++++++++++++++++++++++++++++++++++++++
>> > > >>  4 files changed, 175 insertions(+)
>> > > >>  create mode 100644 drivers/hid/hid-mf.c
>> > > >>
>> > > >> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
>> > > >> index cd4599c..1530d28 100644
>> > > >> --- a/drivers/hid/Kconfig
>> > > >> +++ b/drivers/hid/Kconfig
>> > > >> @@ -512,6 +512,14 @@ config HID_MAGICMOUSE
>> > > >>       Say Y here if you want support for the multi-touch features of the
>> > > >>       Apple Wireless "Magic" Mouse and the Apple Wireless "Magic" Trackpad.
>> > > >>
>> > > >> +config HID_MAYFLASH
>> > > >> +     tristate "Mayflash game controller adapter force feedback"
>> > > >> +     depends on HID
>> > > >> +     select INPUT_FF_MEMLESS
>> > > >> +     ---help---
>> > > >> +     Say Y here if you have HJZ Mayflash PS3 game controller adapters
>> > > >> +     and want to enable force feedback support.
>> > > >> +
>> > > >>  config HID_MICROSOFT
>> > > >>       tristate "Microsoft non-fully HID-compliant devices"
>> > > >>       depends on HID
>> > > >> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
>> > > >> index 86b2b57..c0453f1 100644
>> > > >> --- a/drivers/hid/Makefile
>> > > >> +++ b/drivers/hid/Makefile
>> > > >> @@ -58,6 +58,7 @@ obj-$(CONFIG_HID_LOGITECH)  += hid-logitech.o
>> > > >>  obj-$(CONFIG_HID_LOGITECH_DJ)        += hid-logitech-dj.o
>> > > >>  obj-$(CONFIG_HID_LOGITECH_HIDPP)     += hid-logitech-hidpp.o
>> > > >>  obj-$(CONFIG_HID_MAGICMOUSE) += hid-magicmouse.o
>> > > >> +obj-$(CONFIG_HID_MAYFLASH)   += hid-mf.o
>> > > >>  obj-$(CONFIG_HID_MICROSOFT)  += hid-microsoft.o
>> > > >>  obj-$(CONFIG_HID_MONTEREY)   += hid-monterey.o
>> > > >>  obj-$(CONFIG_HID_MULTITOUCH) += hid-multitouch.o
>> > > >> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> > > >> index 2b89c70..8470e22 100644
>> > > >> --- a/drivers/hid/hid-core.c
>> > > >> +++ b/drivers/hid/hid-core.c
>> > > >> @@ -1883,6 +1883,7 @@ static const struct hid_device_id hid_have_special_driver[] = {
>> > > >>       { HID_USB_DEVICE(USB_VENDOR_ID_DELCOM, USB_DEVICE_ID_DELCOM_VISUAL_IND) },
>> > > >>       { HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, 0x0006) },
>> > > >>       { HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, 0x0011) },
>> > > >> +     { HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, USB_DEVICE_ID_DRAGONRISE_PS3) },
>> > > >>       { HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, USB_DEVICE_ID_DREAM_CHEEKY_WN) },
>> > > >>       { HID_USB_DEVICE(USB_VENDOR_ID_DREAM_CHEEKY, USB_DEVICE_ID_DREAM_CHEEKY_FA) },
>> > > >>       { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_ELECOM, USB_DEVICE_ID_ELECOM_BM084) },
>> > > >> diff --git a/drivers/hid/hid-mf.c b/drivers/hid/hid-mf.c
>> > > >> new file mode 100644
>> > > >> index 0000000..6791ce7
>> > > >> --- /dev/null
>> > > >> +++ b/drivers/hid/hid-mf.c
>> > > >> @@ -0,0 +1,165 @@
>> > > >> +/*
>> > > >> + * Force feedback support for Mayflash game controller adapters.
>> > > >> + *
>> > > >> + * These devices are manufactured by Mayflash but identify themselves
>> > > >> + * using the vendor ID of DragonRise Inc.
>> > > >> + *
>> > > >> + * Tested with:
>> > > >> + * 0079:1801 "DragonRise Inc. Mayflash PS3 Game Controller Adapter"
>> > > >> + *
>> > > >> + * The following adapters probably work too, but need to be tested:
>> > > >> + * 0079:1800 "DragonRise Inc. Mayflash WIIU Game Controller Adapter"
>> > > >> + * 0079:1843 "DragonRise Inc. Mayflash GameCube Game Controller Adapter"
>> > > >> + *
>> > > >> + * Copyright (c) 2016 Marcel Hasler <mahasler@gmail.com>
>> > > >> + */
>> > > >> +
>> > > >> +/*
>> > > >> + * This program is free software; you can redistribute it and/or modify
>> > > >> + * it under the terms of the GNU General Public License as published by
>> > > >> + * the Free Software Foundation; either version 2 of the License, or
>> > > >> + * (at your option) any later version.
>> > > >> + *
>> > > >> + * This program is distributed in the hope that it will be useful,
>> > > >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> > > >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> > > >> + * GNU General Public License for more details.
>> > > >> + */
>> > > >> +
>> > > >> +#include <linux/input.h>
>> > > >> +#include <linux/slab.h>
>> > > >> +#include <linux/hid.h>
>> > > >> +#include <linux/module.h>
>> > > >> +
>> > > >> +#include "hid-ids.h"
>> > > >> +
>> > > >> +struct mf_device {
>> > > >> +     struct hid_report *report;
>> > > >> +};
>> > > >> +
>> > > >> +static int mf_play(struct input_dev *dev, void *data, struct ff_effect *effect)
>> > > >> +{
>> > > >> +     struct hid_device *hid = input_get_drvdata(dev);
>> > > >> +     struct mf_device *mf = data;
>> > > >> +     int strong, weak;
>> > > >> +
>> > > >> +     strong = effect->u.rumble.strong_magnitude;
>> > > >> +     weak = effect->u.rumble.weak_magnitude;
>> > > >> +
>> > > >> +     dbg_hid("Called with 0x%04x 0x%04x.\n", strong, weak);
>> > > >> +
>> > > >> +     strong = strong * 0xff / 0xffff;
>> > > >> +     weak = weak * 0xff / 0xffff;
>> > > >> +
>> > > >> +     dbg_hid("Running with 0x%02x 0x%02x.\n", strong, weak);
>> > > >> +
>> > > >> +     mf->report->field[0]->value[0] = weak;
>> > > >> +     mf->report->field[0]->value[1] = strong;
>> > > >> +     hid_hw_request(hid, mf->report, HID_REQ_SET_REPORT);
>> > > >> +
>> > > >> +     return 0;
>> > > >> +}
>> > > >> +
>> > > >> +static int mf_init(struct hid_device *hid)
>> > > >> +{
>> > > >> +     struct mf_device *mf;
>> > > >> +
>> > > >> +     struct list_head *report_list =
>> > > >> +                     &hid->report_enum[HID_OUTPUT_REPORT].report_list;
>> > > >> +
>> > > >> +     struct list_head *report_ptr;
>> > > >> +     struct hid_report *report;
>> > > >> +
>> > > >> +     struct hid_input *input =
>> > > >> +                     list_first_entry(&hid->inputs, struct hid_input, list);
>> > > >> +
>> > > >> +     struct input_dev *dev;
>> > > >> +
>> > > >> +     int error;
>> > > >> +
>> > > >> +     /* Setup each of the four inputs */
>> > > >> +     list_for_each(report_ptr, report_list) {
>> > > >> +             report = list_entry(report_ptr, struct hid_report, list);
>> > > >> +
>> > > >> +             if (report->maxfield < 1 || report->field[0]->report_count < 2) {
>> > > >> +                     hid_err(hid, "Invalid report, this should never happen!\n");
>> > > >> +                     return -ENODEV;
>> > > >> +             }
>> > > >> +
>> > > >> +             if (input == NULL) {
>> > > >> +                     hid_err(hid, "Missing input, this should never happen!\n");
>> > > >> +                     return -ENODEV;
>> > > >> +             }
>> > > >> +
>> > > >> +             dev = input->input;
>> > > >> +             input = list_next_entry(input, list);
>> > > >
>> > > > Wouldn't it make more sense to use .input_configured instead of this
>> > > > after-the-fact loop?
>> > > >
>> > > >> +
>> > > >> +             mf = kzalloc(sizeof(struct mf_device), GFP_KERNEL);
>> > > >
>> > > > I might not be fully awoken, but where is the corresponding kfree?
>> > > > If you do not want to manually free the allocated memory, you can use
>> > > > devm_kzalloc, as long as there is no races when removing the device.
>> > > >
>> > >
>> > > The corresponding kfree for mf in this case is in ml_ff_destroy().
>> >
>> > K, thanks. I should definitively have taken more attention while reading
>> > the patch.
>> >
>> > >
>> > > >> +             if (!mf)
>> > > >> +                     return -ENOMEM;
>> > > >> +
>> > > >> +             set_bit(FF_RUMBLE, dev->ffbit);
>> > > >> +
>> > > >> +             error = input_ff_create_memless(dev, mf, mf_play);
>> > > >> +             if (error) {
>> > > >> +                     kfree(mf);
>> > > >> +                     return error;
>> > > >> +             }
>> > > >> +
>> > > >> +             mf->report = report;
>> > > >> +             mf->report->field[0]->value[0] = 0x00;
>> > > >> +             mf->report->field[0]->value[1] = 0x00;
>> > > >> +             hid_hw_request(hid, mf->report, HID_REQ_SET_REPORT);
>> > > >> +     }
>> > > >> +
>> > > >> +     hid_info(hid, "Force feedback for HJZ Mayflash game controller "
>> > > >> +                   "adapters by Marcel Hasler <mahasler@gmail.com>\n");
>> > > >> +
>> > > >> +     return 0;
>> > > >> +}
>> > > >> +
>> > > >> +static int mf_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> > > >> +{
>> > > >> +     int error;
>> > > >> +
>> > > >> +     dev_dbg(&hdev->dev, "Mayflash HID hardware probe...\n");
>> > > >> +
>> > > >> +     /* Split device into four inputs */
>> > > >> +     hdev->quirks |= HID_QUIRK_MULTI_INPUT;
>> > > >
>> > > > Looks like the entry in the previous patch was not required apparently
>> > > > :)
>> > > >
>> > > >> +
>> > > >> +     error = hid_parse(hdev);
>> > > >> +     if (error) {
>> > > >> +             hid_err(hdev, "HID parse failed.\n");
>> > > >> +             return error;
>> > > >> +     }
>> > > >> +
>> > > >> +     error = hid_hw_start(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_FF);
>> > > >> +     if (error) {
>> > > >> +             hid_err(hdev, "HID hw start failed\n");
>> > > >> +             return error;
>> > > >> +     }
>> > > >> +
>> > > >> +     error = mf_init(hdev);
>> > > >
>> > > > This seems completely racy. hid_hw_start() exports the input nodes to
>> > > > user-space and the ff initialization is done after. It would be much
>> > > > better to initialize the ff part in .input_configured when the device
>> > > > hasn't been registered to user space yet.
>> > > >
>> > > > Cheers,
>> > > > Benjamin
>> > > >
>> > > >> +     if (error) {
>> > > >> +             hid_err(hdev, "Force feedback init failed.\n");
>> > > >> +             hid_hw_stop(hdev);
>> > > >> +             return error;
>> > > >> +     }
>> > > >> +
>> > > >> +     return 0;
>> > > >> +}
>> > > >> +
>> > > >> +static const struct hid_device_id mf_devices[] = {
>> > > >> +     { HID_USB_DEVICE(USB_VENDOR_ID_DRAGONRISE, USB_DEVICE_ID_DRAGONRISE_PS3),  },
>> > > >> +     { }
>> > > >> +};
>> > > >> +MODULE_DEVICE_TABLE(hid, mf_devices);
>> > > >> +
>> > > >> +static struct hid_driver mf_driver = {
>> > > >> +     .name = "hid_mf",
>> > > >> +     .id_table = mf_devices,
>> > > >> +     .probe = mf_probe,
>> > > >> +};
>> > > >> +module_hid_driver(mf_driver);
>> > > >> +
>> > > >> +MODULE_LICENSE("GPL");
>> > > >> --
>> > > >> 2.10.1
>> > > >>
>> > >
>> > > Thanks for the hints. I tried your idea with .input_configured but
>> > > there's a bit of a problem with that. For each of the four gamepads
>> > > two input device nodes are created, one for the buttons/axes and one
>> > > for force feedback. Unfortunately systemd only assigns the first nodes
>> > > the uaccess tag. Now, I looked at the other ff drivers and essentially
>> > > followed their example. None of these use .input_configured, but
>> > > instead just use the very first hid_input and make ff available
>> > > through that. This works and actually has the desired effect that
>> > > normal users can use force feedback without having to be in the
>> > > 'input' group (which as I understand is deprecated).
>> >
>> > I haven't taken much attention to the ff implementation both in the
>> > kernel and in userspace. After your explanation. I am not sure I want to
>> > spend more time on it though :)
>> >
>> > >
>> > > So while I completely agree that using .input_configured would be the
>> > > proper thing to do, I think either all ff drivers should use it (and
>> > > systemd should be fixed to make those input nodes accessible) or
>> > > hid-mf should do what all others do (at least for the time being).
>> >
>> > Yes, please stick to what other kernel drivers do, and hope for the
>> > best. If I understand correctly, you'll need to remove the for loop and
>> > only check for the first available input node, right?
>> >
>> > Cheers,
>> > Benjamin
>> >
>> > >
>> > > Best regards
>> > > Marcel
>>
>> In this case the outer loop is needed because otherwise only one of the four possible gamepads
>> would get force feedback. Unfortunately I don't know of any elegant way to map a ff hidinput to the
>> corresponding button/axis hidinput. That's why I essentially have to loop over two lists. If
>> anyone can think of a better way, I'd be happy to change that.
>>
>> I did just now revert a small change that I made yesterday shortly before submitting the patch.
>> Actually the change was to get rid of a 80-character warning from checkpatch.pl but I think
>> I'd prefer clean code and ignore that warning.
>>
>> I'll submit an updated version in a moment.
>>
>> About the quirk, I would propse to leave it in since the usbhid driver still needs it to work
>> properly. If e.g. someone builds a kernel without hid-mf, the device should still work under
>> usbhid (just without ff).
>
> That won't do. The change in hid-core prevent hid-generic to map your
> device. So if you disable hid-mf, no hid driver will get bound to the
> device and no one will be able to use the device without loading a
> specific HID module for it.
>
> Cheers,
> Benjamin
>
>>
>> Regards
>> Marcel

Right. I just submitted a second version of the patchset that fixes
this and also correctly iterates the hid inputs.

Best regards
Marcel

P.S. I always forget to CC the mailing list, sorry about that:-)