LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 2/2] Fix force effect modifications for the Microsoft Sidewinder Force Feedback Pro 2 joystick
@ 2015-01-27 10:59 Jim Keir
  0 siblings, 0 replies; 5+ messages in thread
From: Jim Keir @ 2015-01-27 10:59 UTC (permalink / raw)
  To: linux-input, linux-usb, linux-kernel, jkosina; +Cc: Jim Keir

---
The FF2 driver (usbhid/hid-pidff.c) does not set the effect ID when
uploading an effect. The result is that the initial upload works but
subsequent uploads to modify effect parameters are all directed at the
last-created effect.

The targeted effect ID must be passed back to the device when effect
parameters are changed. This is done at the start of
"pidff_set_condition_report", "pidff_set_periodic_report" etc. based on
the value of "pidff->block_load[PID_EFFECT_
BLOCK_INDEX].value[0]".

This value is only ever set during pidff_request_effect_upload.
The result is stored in "pidff->pid_id[effect->id]" at the end of
pid_upload_effect, for later use. However, if an effect is modified and
re-sent then this identifier is not being copied back from
pidff->pid_id[effect->id] before sending the command to the device. The
fix is to do this at the start of pidff_upload_effect.

Signed-off-by: Jim Keir <jimkeir@oracledbadirect.com> 

 drivers/hid/usbhid/hid-pidff.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/hid/usbhid/hid-pidff.c b/drivers/hid/usbhid/hid-pidff.c
index 0b531c6..1b3fa70 100644
--- a/drivers/hid/usbhid/hid-pidff.c
+++ b/drivers/hid/usbhid/hid-pidff.c
@@ -568,6 +568,12 @@ static int pidff_upload_effect(struct input_dev *dev, struct ff_effect *effect,
 	int type_id;
 	int error;
 
+	pidff->block_load[PID_EFFECT_BLOCK_INDEX].value[0] = 0;
+	if (old && effect) {
+		pidff->block_load[PID_EFFECT_BLOCK_INDEX].value[0] =
+			pidff->pid_id[effect->id];
+	}
+
 	switch (effect->type) {
 	case FF_CONSTANT:
 		if (!old) {
-- 
1.9.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 2/2] Fix force effect modifications for the Microsoft Sidewinder Force Feedback Pro 2 joystick
  2015-01-24  9:41     ` Jim Keir
@ 2015-01-26 20:31       ` Jiri Kosina
  0 siblings, 0 replies; 5+ messages in thread
From: Jiri Kosina @ 2015-01-26 20:31 UTC (permalink / raw)
  To: Jim Keir; +Cc: Benjamin Tissoires, linux-input, linux-usb, linux-kernel

On Sat, 24 Jan 2015, Jim Keir wrote:

> Yes, confirmed. The description below still holds.

Could you please therefore fix this patch so that it contains proper 
changelog on proper place, and resubmit?

Also, the Subject line looks rather strange to me ... to my 
understanding, this isn't really specific for MS sidewinder hardware, 
right?

-- 
Jiri Kosina
SUSE Labs

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 2/2] Fix force effect modifications for the Microsoft Sidewinder Force Feedback Pro 2 joystick
  2015-01-23 18:54   ` Benjamin Tissoires
@ 2015-01-24  9:41     ` Jim Keir
  2015-01-26 20:31       ` Jiri Kosina
  0 siblings, 1 reply; 5+ messages in thread
From: Jim Keir @ 2015-01-24  9:41 UTC (permalink / raw)
  To: Benjamin Tissoires; +Cc: linux-input, linux-usb, linux-kernel, Jiri Kosina

Hi,

Yes, confirmed. The description below still holds.

Cheers,
Jim

On 23/01/2015 18:54, Benjamin Tissoires wrote:
> I have no clue what this code is doing (don't know much about the ff system).
> Quoting your initial 0001/0001, I thing the commit message should be
> the following.
> Jim, can you please validate it?
>
> "
> The FF2 driver (usbhid/hid-pidff.c) does not set the effect ID when
> uploading an effect. The result is that the initial upload works but
> subsequent uploads to modify effect parameters are all directed at the
> last-created effect.
>
> The targeted effect ID must be passed back to the device when effect
> parameters are changed. This is done at the start of
> "pidff_set_condition_report", "pidff_set_periodic_report" etc. based on
> the value of "pidff->block_load[PID_EFFECT_
> BLOCK_INDEX].value[0]".
> However, this value is only ever set during pidff_request_effect_upload.
> The result is stored in "pidff->pid_id[effect->id]" at the end of
> pid_upload_effect, for later use. However, if an effect is modified and
> re-sent then this identifier is not being copied back from
> pidff->pid_id[effect->id] before sending the command to the device. The
> fix is to do this at the start of pidff_upload_effect.
> "
>
> ---
>
> As said, I have no clue* of what this is supposed to do, so I can not
> add my reviewed-by here. The patch seems good however so nothing
> should prevent us to take it if it fixes your problem :)
>
> Cheers,
> Benjamin
>
> * OK, I am pushing a little bit, I just don't want today to go deep in
> the code :-)
>
>
> On Fri, Jan 23, 2015 at 12:21 PM, Jim Keir <jimkeir@oracledbadirect.com> wrote:
>> ---
>> Hi,
>>
>> Changes from previous patches:
>> - All changes removed from higher-level files
>> - Extra debug line removed
>>
>> Signed-off-by: Jim Keir <jimkeir@oracledbadirect.com>
>>
>>   drivers/hid/usbhid/hid-pidff.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/hid/usbhid/hid-pidff.c b/drivers/hid/usbhid/hid-pidff.c
>> index 0b531c6..1b3fa70 100644
>> --- a/drivers/hid/usbhid/hid-pidff.c
>> +++ b/drivers/hid/usbhid/hid-pidff.c
>> @@ -568,6 +568,12 @@ static int pidff_upload_effect(struct input_dev *dev, struct ff_effect *effect,
>>          int type_id;
>>          int error;
>>
>> +       pidff->block_load[PID_EFFECT_BLOCK_INDEX].value[0] = 0;
>> +       if (old && effect) {
>> +               pidff->block_load[PID_EFFECT_BLOCK_INDEX].value[0] =
>> +                       pidff->pid_id[effect->id];
>> +       }
>> +
>>          switch (effect->type) {
>>          case FF_CONSTANT:
>>                  if (!old) {
>> --
>> 1.9.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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 2/2] Fix force effect modifications for the Microsoft Sidewinder Force Feedback Pro 2 joystick
  2015-01-23 17:21 ` Jim Keir
@ 2015-01-23 18:54   ` Benjamin Tissoires
  2015-01-24  9:41     ` Jim Keir
  0 siblings, 1 reply; 5+ messages in thread
From: Benjamin Tissoires @ 2015-01-23 18:54 UTC (permalink / raw)
  To: Jim Keir; +Cc: linux-input, linux-usb, linux-kernel, Jiri Kosina

I have no clue what this code is doing (don't know much about the ff system).
Quoting your initial 0001/0001, I thing the commit message should be
the following.
Jim, can you please validate it?

"
The FF2 driver (usbhid/hid-pidff.c) does not set the effect ID when
uploading an effect. The result is that the initial upload works but
subsequent uploads to modify effect parameters are all directed at the
last-created effect.

The targeted effect ID must be passed back to the device when effect
parameters are changed. This is done at the start of
"pidff_set_condition_report", "pidff_set_periodic_report" etc. based on
the value of "pidff->block_load[PID_EFFECT_
BLOCK_INDEX].value[0]".
However, this value is only ever set during pidff_request_effect_upload.
The result is stored in "pidff->pid_id[effect->id]" at the end of
pid_upload_effect, for later use. However, if an effect is modified and
re-sent then this identifier is not being copied back from
pidff->pid_id[effect->id] before sending the command to the device. The
fix is to do this at the start of pidff_upload_effect.
"

---

As said, I have no clue* of what this is supposed to do, so I can not
add my reviewed-by here. The patch seems good however so nothing
should prevent us to take it if it fixes your problem :)

Cheers,
Benjamin

* OK, I am pushing a little bit, I just don't want today to go deep in
the code :-)


On Fri, Jan 23, 2015 at 12:21 PM, Jim Keir <jimkeir@oracledbadirect.com> wrote:
> ---
> Hi,
>
> Changes from previous patches:
> - All changes removed from higher-level files
> - Extra debug line removed
>
> Signed-off-by: Jim Keir <jimkeir@oracledbadirect.com>
>
>  drivers/hid/usbhid/hid-pidff.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/hid/usbhid/hid-pidff.c b/drivers/hid/usbhid/hid-pidff.c
> index 0b531c6..1b3fa70 100644
> --- a/drivers/hid/usbhid/hid-pidff.c
> +++ b/drivers/hid/usbhid/hid-pidff.c
> @@ -568,6 +568,12 @@ static int pidff_upload_effect(struct input_dev *dev, struct ff_effect *effect,
>         int type_id;
>         int error;
>
> +       pidff->block_load[PID_EFFECT_BLOCK_INDEX].value[0] = 0;
> +       if (old && effect) {
> +               pidff->block_load[PID_EFFECT_BLOCK_INDEX].value[0] =
> +                       pidff->pid_id[effect->id];
> +       }
> +
>         switch (effect->type) {
>         case FF_CONSTANT:
>                 if (!old) {
> --
> 1.9.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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 2/2] Fix force effect modifications for the Microsoft Sidewinder Force Feedback Pro 2 joystick
       [not found] <1422033640-4104-1-git-send-email-jimkeir@oracledbadirect.com>
@ 2015-01-23 17:21 ` Jim Keir
  2015-01-23 18:54   ` Benjamin Tissoires
  0 siblings, 1 reply; 5+ messages in thread
From: Jim Keir @ 2015-01-23 17:21 UTC (permalink / raw)
  To: linux-input, linux-usb, linux-kernel, jkosina; +Cc: Jim Keir

---
Hi,

Changes from previous patches: 
- All changes removed from higher-level files
- Extra debug line removed

Signed-off-by: Jim Keir <jimkeir@oracledbadirect.com> 

 drivers/hid/usbhid/hid-pidff.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/hid/usbhid/hid-pidff.c b/drivers/hid/usbhid/hid-pidff.c
index 0b531c6..1b3fa70 100644
--- a/drivers/hid/usbhid/hid-pidff.c
+++ b/drivers/hid/usbhid/hid-pidff.c
@@ -568,6 +568,12 @@ static int pidff_upload_effect(struct input_dev *dev, struct ff_effect *effect,
 	int type_id;
 	int error;
 
+	pidff->block_load[PID_EFFECT_BLOCK_INDEX].value[0] = 0;
+	if (old && effect) {
+		pidff->block_load[PID_EFFECT_BLOCK_INDEX].value[0] =
+			pidff->pid_id[effect->id];
+	}
+
 	switch (effect->type) {
 	case FF_CONSTANT:
 		if (!old) {
-- 
1.9.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2015-01-27 12:05 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-27 10:59 [PATCH 2/2] Fix force effect modifications for the Microsoft Sidewinder Force Feedback Pro 2 joystick Jim Keir
     [not found] <1422033640-4104-1-git-send-email-jimkeir@oracledbadirect.com>
2015-01-23 17:21 ` Jim Keir
2015-01-23 18:54   ` Benjamin Tissoires
2015-01-24  9:41     ` Jim Keir
2015-01-26 20:31       ` Jiri Kosina

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