| Message ID | 20161030221313.GA15445@arch-desktop (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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:-) -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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");
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