| Message ID | 1398175209-9565-10-git-send-email-madcatxster@devoid-pointer.net (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
On Tue, 2014-04-22 at 15:59 +0200, Michal Malý wrote: > static int drff_play(struct input_dev *dev, void *data, > - struct ff_effect *effect) > + const struct mlnx_effect_command *command) > { > struct hid_device *hid = input_get_drvdata(dev); > struct drff_device *drff = data; > + const struct mlnx_rumble_force *rumble_force = > &command->u.rumble_force; > int strong, weak; > > - strong = effect->u.rumble.strong_magnitude; > - weak = effect->u.rumble.weak_magnitude; > + strong = rumble_force->strong; > + weak = rumble_force->weak; > > dbg_hid("called with 0x%04x 0x%04x", strong, weak); > > - if (strong || weak) { > - strong = strong * 0xff / 0xffff; > - weak = weak * 0xff / 0xffff; > - > - /* While reverse engineering this device, I found that > when > - this value is set, it causes the strong rumble to > function > - at a near maximum speed, so we'll bypass it. */ > - if (weak == 0x0a) > - weak = 0x0b; > - > - drff->report->field[0]->value[0] = 0x51; > - drff->report->field[0]->value[1] = 0x00; > - drff->report->field[0]->value[2] = weak; > - drff->report->field[0]->value[4] = strong; > - hid_hw_request(hid, drff->report, HID_REQ_SET_REPORT); > - > - drff->report->field[0]->value[0] = 0xfa; > - drff->report->field[0]->value[1] = 0xfe; > - } else { > + switch (command->cmd) { > + case MLNX_START_RUMBLE: > + if (strong || weak) { > + strong = strong * 0xff / 0xffff; > + weak = weak * 0xff / 0xffff; > + > + /* While reverse engineering this device, I > found that when > + this value is set, it causes the strong rumble > to function > + at a near maximum speed, so we'll bypass it. > */ > + if (weak == 0x0a) > + weak = 0x0b; > + > + drff->report->field[0]->value[0] = 0x51; > + drff->report->field[0]->value[1] = 0x00; > + drff->report->field[0]->value[2] = weak; > + drff->report->field[0]->value[4] = strong; This looks like an endianness bug. > + hid_hw_request(hid, drff->report, > HID_REQ_SET_REPORT); Regards Oliver -- 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 Wed, Apr 23, 2014 at 03:14:44PM +0000, madcatxster@devoid-pointer.net > wrote: >> This is another case where even the old code was flawed, right? Should >> I try to stuff the fixes into these patches or would a few extra >> patches addressing these problems be an easier to review solution? I >> can append such patches to the MLNX patchset. > > Changes addressing pre-existing problem should go into separate patches > (preferably applicable first). > As a by-stander who would like to see MLNX move forward, should it be heldback by pre-existing problems in drivers that the MLNX dev(s) don't have hardware to test against...? Simon. -- 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 Wednesday 23 of April 2014 11:47:05 simon@mungewell.org wrote: > > On Wed, Apr 23, 2014 at 03:14:44PM +0000, madcatxster@devoid-pointer.net > > > > wrote: > >> This is another case where even the old code was flawed, right? Should > >> I try to stuff the fixes into these patches or would a few extra > >> patches addressing these problems be an easier to review solution? I > >> can append such patches to the MLNX patchset. > > > > Changes addressing pre-existing problem should go into separate patches > > (preferably applicable first). > > As a by-stander who would like to see MLNX move forward, should it be > heldback by pre-existing problems in drivers that the MLNX dev(s) don't > have hardware to test against...? > > Simon. Either approach is fine be me - I can rebase the MLNX patchset against the fixes and submit it again. I suppose that this is a good opportunity to fix a bunch old issues that would pass unnoticed otherwise. I would however appreciate as much comments regarding MLNX itself before I begin cleaning the ancient dust. Thanks for your input, Michal -- 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 Wednesday 23 of April 2014 15:41:03 Oliver Neukum wrote: > On Tue, 2014-04-22 at 15:59 +0200, Michal Malý wrote: > > static int drff_play(struct input_dev *dev, void *data, > > > > - struct ff_effect *effect) > > + const struct mlnx_effect_command *command) > > > > { > > > > struct hid_device *hid = input_get_drvdata(dev); > > struct drff_device *drff = data; > > > > + const struct mlnx_rumble_force *rumble_force = > > &command->u.rumble_force; > > > > int strong, weak; > > > > - strong = effect->u.rumble.strong_magnitude; > > - weak = effect->u.rumble.weak_magnitude; > > + strong = rumble_force->strong; > > + weak = rumble_force->weak; > > > > dbg_hid("called with 0x%04x 0x%04x", strong, weak); > > > > - if (strong || weak) { > > - strong = strong * 0xff / 0xffff; > > - weak = weak * 0xff / 0xffff; > > - > > - /* While reverse engineering this device, I found that > > when > > - this value is set, it causes the strong rumble to > > function > > - at a near maximum speed, so we'll bypass it. */ > > - if (weak == 0x0a) > > - weak = 0x0b; > > - > > - drff->report->field[0]->value[0] = 0x51; > > - drff->report->field[0]->value[1] = 0x00; > > - drff->report->field[0]->value[2] = weak; > > - drff->report->field[0]->value[4] = strong; > > - hid_hw_request(hid, drff->report, HID_REQ_SET_REPORT); > > - > > - drff->report->field[0]->value[0] = 0xfa; > > - drff->report->field[0]->value[1] = 0xfe; > > - } else { > > + switch (command->cmd) { > > + case MLNX_START_RUMBLE: > > + if (strong || weak) { > > + strong = strong * 0xff / 0xffff; > > + weak = weak * 0xff / 0xffff; > > + > > + /* While reverse engineering this device, I > > found that when > > + this value is set, it causes the strong rumble > > to function > > + at a near maximum speed, so we'll bypass it. > > */ > > + if (weak == 0x0a) > > + weak = 0x0b; > > + > > + drff->report->field[0]->value[0] = 0x51; > > + drff->report->field[0]->value[1] = 0x00; > > + drff->report->field[0]->value[2] = weak; > > + drff->report->field[0]->value[4] = strong; > > This looks like an endianness bug. I don't have a big endian machine to check but why would this be an endianness issue? We're dealing with values all the time here, not addresses so I'd expect the 'weak' and 'strong' values to be truncated if they won't fit into byte. Division done beforehand makes sure that the values are within <0; 255> range. As far as I can see this is quite common in the HID and Input code. Am I missing something here? Thanks, Michal -- 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 Thu, 2014-04-24 at 12:32 +0200, Michal Malý wrote: > On Wednesday 23 of April 2014 15:41:03 Oliver Neukum wrote: > > On Tue, 2014-04-22 at 15:59 +0200, Michal Malý wrote: > > > static int drff_play(struct input_dev *dev, void *data, > > > > > > - struct ff_effect *effect) > > > + const struct mlnx_effect_command *command) > > > > > > { > > > > > > struct hid_device *hid = input_get_drvdata(dev); > > > struct drff_device *drff = data; > > > > > > + const struct mlnx_rumble_force *rumble_force = > > > &command->u.rumble_force; > > > > > > int strong, weak; > > > > > > - strong = effect->u.rumble.strong_magnitude; > > > - weak = effect->u.rumble.weak_magnitude; > > > + strong = rumble_force->strong; > > > + weak = rumble_force->weak; > > > > > > dbg_hid("called with 0x%04x 0x%04x", strong, weak); > > > > > > - if (strong || weak) { > > > - strong = strong * 0xff / 0xffff; > > > - weak = weak * 0xff / 0xffff; > > > - > > > - /* While reverse engineering this device, I found that > > > when > > > - this value is set, it causes the strong rumble to > > > function > > > - at a near maximum speed, so we'll bypass it. */ > > > - if (weak == 0x0a) > > > - weak = 0x0b; > > > - > > > - drff->report->field[0]->value[0] = 0x51; > > > - drff->report->field[0]->value[1] = 0x00; > > > - drff->report->field[0]->value[2] = weak; > > > - drff->report->field[0]->value[4] = strong; > > > - hid_hw_request(hid, drff->report, HID_REQ_SET_REPORT); > > > - > > > - drff->report->field[0]->value[0] = 0xfa; > > > - drff->report->field[0]->value[1] = 0xfe; > > > - } else { > > > + switch (command->cmd) { > > > + case MLNX_START_RUMBLE: > > > + if (strong || weak) { > > > + strong = strong * 0xff / 0xffff; > > > + weak = weak * 0xff / 0xffff; > > > + > > > + /* While reverse engineering this device, I > > > found that when > > > + this value is set, it causes the strong rumble > > > to function > > > + at a near maximum speed, so we'll bypass it. > > > */ > > > + if (weak == 0x0a) > > > + weak = 0x0b; > > > + > > > + drff->report->field[0]->value[0] = 0x51; > > > + drff->report->field[0]->value[1] = 0x00; > > > + drff->report->field[0]->value[2] = weak; > > > + drff->report->field[0]->value[4] = strong; > > > > This looks like an endianness bug. > > I don't have a big endian machine to check but why would this be an endianness > issue? We're dealing with values all the time here, not addresses so I'd > expect the 'weak' and 'strong' values to be truncated if they won't fit into > byte. Division done beforehand makes sure that the values are within <0; 255> > range. As far as I can see this is quite common in the HID and Input code. Am > I missing something here? Sorry, I thought you were writing to 16bit variables. Regards Oliver -- 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 6e233d2..0ba1962 100644 --- a/drivers/hid/Kconfig +++ b/drivers/hid/Kconfig @@ -196,7 +196,7 @@ config HID_DRAGONRISE config DRAGONRISE_FF bool "DragonRise Inc. force feedback" depends on HID_DRAGONRISE - select INPUT_FF_MEMLESS + select INPUT_FF_MEMLESS_NEXT ---help--- Say Y here if you want to enable force feedback support for DragonRise Inc. game controllers. diff --git a/drivers/hid/hid-dr.c b/drivers/hid/hid-dr.c index ce06444..b95c676 100644 --- a/drivers/hid/hid-dr.c +++ b/drivers/hid/hid-dr.c @@ -31,8 +31,10 @@ #include <linux/slab.h> #include <linux/hid.h> #include <linux/module.h> +#include <linux/input/ff-memless-next.h> #include "hid-ids.h" +#define FF_UPDATE_RATE 50 #ifdef CONFIG_DRAGONRISE_FF @@ -41,38 +43,49 @@ struct drff_device { }; static int drff_play(struct input_dev *dev, void *data, - struct ff_effect *effect) + const struct mlnx_effect_command *command) { struct hid_device *hid = input_get_drvdata(dev); struct drff_device *drff = data; + const struct mlnx_rumble_force *rumble_force = &command->u.rumble_force; int strong, weak; - strong = effect->u.rumble.strong_magnitude; - weak = effect->u.rumble.weak_magnitude; + strong = rumble_force->strong; + weak = rumble_force->weak; dbg_hid("called with 0x%04x 0x%04x", strong, weak); - if (strong || weak) { - strong = strong * 0xff / 0xffff; - weak = weak * 0xff / 0xffff; - - /* While reverse engineering this device, I found that when - this value is set, it causes the strong rumble to function - at a near maximum speed, so we'll bypass it. */ - if (weak == 0x0a) - weak = 0x0b; - - drff->report->field[0]->value[0] = 0x51; - drff->report->field[0]->value[1] = 0x00; - drff->report->field[0]->value[2] = weak; - drff->report->field[0]->value[4] = strong; - hid_hw_request(hid, drff->report, HID_REQ_SET_REPORT); - - drff->report->field[0]->value[0] = 0xfa; - drff->report->field[0]->value[1] = 0xfe; - } else { + switch (command->cmd) { + case MLNX_START_RUMBLE: + if (strong || weak) { + strong = strong * 0xff / 0xffff; + weak = weak * 0xff / 0xffff; + + /* While reverse engineering this device, I found that when + this value is set, it causes the strong rumble to function + at a near maximum speed, so we'll bypass it. */ + if (weak == 0x0a) + weak = 0x0b; + + drff->report->field[0]->value[0] = 0x51; + drff->report->field[0]->value[1] = 0x00; + drff->report->field[0]->value[2] = weak; + drff->report->field[0]->value[4] = strong; + hid_hw_request(hid, drff->report, HID_REQ_SET_REPORT); + + drff->report->field[0]->value[0] = 0xfa; + drff->report->field[0]->value[1] = 0xfe; + } else { + drff->report->field[0]->value[0] = 0xf3; + drff->report->field[0]->value[1] = 0x00; + } + break; + case MLNX_STOP_RUMBLE: drff->report->field[0]->value[0] = 0xf3; drff->report->field[0]->value[1] = 0x00; + break; + default: + return -EINVAL; } drff->report->field[0]->value[2] = 0x00; @@ -116,7 +129,7 @@ static int drff_init(struct hid_device *hid) set_bit(FF_RUMBLE, dev->ffbit); - error = input_ff_create_memless(dev, drff, drff_play); + error = input_ff_create_mlnx(dev, drff, drff_play, FF_UPDATE_RATE); if (error) { kfree(drff); return error;
Port hid-dr to ff-memless-next Signed-off-by: Michal Malý <madcatxster@devoid-pointer.net> --- drivers/hid/Kconfig | 2 +- drivers/hid/hid-dr.c | 59 ++++++++++++++++++++++++++++++++-------------------- 2 files changed, 37 insertions(+), 24 deletions(-)