* [PATCH 1/2] usbhid: Add quirks for Mayflash/Dragonrise GameCube and PS3 adapters.
[not found] <cover.1477863542.git.mahasler@gmail.com>
@ 2016-10-30 22:12 ` Marcel Hasler
2016-10-31 11:10 ` Benjamin Tissoires
2016-10-30 22:13 ` [PATCH 2/2] Add new force feedback driver for Mayflash game controller adapters Marcel Hasler
1 sibling, 1 reply; 9+ messages in thread
From: Marcel Hasler @ 2016-10-30 22:12 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires; +Cc: linux-input
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] Add new force feedback driver for Mayflash game controller adapters.
[not found] <cover.1477863542.git.mahasler@gmail.com>
2016-10-30 22:12 ` [PATCH 1/2] usbhid: Add quirks for Mayflash/Dragonrise GameCube and PS3 adapters Marcel Hasler
@ 2016-10-30 22:13 ` Marcel Hasler
2016-10-31 11:17 ` Benjamin Tissoires
1 sibling, 1 reply; 9+ messages in thread
From: Marcel Hasler @ 2016-10-30 22:13 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires; +Cc: linux-input
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] usbhid: Add quirks for Mayflash/Dragonrise GameCube and PS3 adapters.
2016-10-30 22:12 ` [PATCH 1/2] usbhid: Add quirks for Mayflash/Dragonrise GameCube and PS3 adapters Marcel Hasler
@ 2016-10-31 11:10 ` Benjamin Tissoires
0 siblings, 0 replies; 9+ messages in thread
From: Benjamin Tissoires @ 2016-10-31 11:10 UTC (permalink / raw)
To: Marcel Hasler; +Cc: Jiri Kosina, linux-input
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] Add new force feedback driver for Mayflash game controller adapters.
2016-10-30 22:13 ` [PATCH 2/2] Add new force feedback driver for Mayflash game controller adapters Marcel Hasler
@ 2016-10-31 11:17 ` Benjamin Tissoires
2016-10-31 13:31 ` Marcel Hasler
0 siblings, 1 reply; 9+ messages in thread
From: Benjamin Tissoires @ 2016-10-31 11:17 UTC (permalink / raw)
To: Marcel Hasler; +Cc: Jiri Kosina, linux-input
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] Add new force feedback driver for Mayflash game controller adapters.
2016-10-31 11:17 ` Benjamin Tissoires
@ 2016-10-31 13:31 ` Marcel Hasler
2016-10-31 13:48 ` Benjamin Tissoires
0 siblings, 1 reply; 9+ messages in thread
From: Marcel Hasler @ 2016-10-31 13:31 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: Jiri Kosina, linux-input
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] Add new force feedback driver for Mayflash game controller adapters.
2016-10-31 13:31 ` Marcel Hasler
@ 2016-10-31 13:48 ` Benjamin Tissoires
2016-10-31 14:16 ` mahasler
0 siblings, 1 reply; 9+ messages in thread
From: Benjamin Tissoires @ 2016-10-31 13:48 UTC (permalink / raw)
To: Marcel Hasler; +Cc: Jiri Kosina, linux-input
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] Add new force feedback driver for Mayflash game controller adapters.
2016-10-31 13:48 ` Benjamin Tissoires
@ 2016-10-31 14:16 ` mahasler
2016-10-31 14:39 ` Benjamin Tissoires
0 siblings, 1 reply; 9+ messages in thread
From: mahasler @ 2016-10-31 14:16 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: Jiri Kosina, linux-input
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] Add new force feedback driver for Mayflash game controller adapters.
2016-10-31 14:16 ` mahasler
@ 2016-10-31 14:39 ` Benjamin Tissoires
2016-10-31 15:32 ` Marcel Hasler
0 siblings, 1 reply; 9+ messages in thread
From: Benjamin Tissoires @ 2016-10-31 14:39 UTC (permalink / raw)
To: mahasler; +Cc: Jiri Kosina, linux-input
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] Add new force feedback driver for Mayflash game controller adapters.
2016-10-31 14:39 ` Benjamin Tissoires
@ 2016-10-31 15:32 ` Marcel Hasler
0 siblings, 0 replies; 9+ messages in thread
From: Marcel Hasler @ 2016-10-31 15:32 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: Jiri Kosina, linux-input
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:-)
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-10-31 15:33 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <cover.1477863542.git.mahasler@gmail.com>
2016-10-30 22:12 ` [PATCH 1/2] usbhid: Add quirks for Mayflash/Dragonrise GameCube and PS3 adapters Marcel Hasler
2016-10-31 11:10 ` Benjamin Tissoires
2016-10-30 22:13 ` [PATCH 2/2] Add new force feedback driver for Mayflash game controller adapters Marcel Hasler
2016-10-31 11:17 ` Benjamin Tissoires
2016-10-31 13:31 ` Marcel Hasler
2016-10-31 13:48 ` Benjamin Tissoires
2016-10-31 14:16 ` mahasler
2016-10-31 14:39 ` Benjamin Tissoires
2016-10-31 15:32 ` Marcel Hasler
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).