HID: pidff: Completely rework and fix pidff_reset function

Previously, it was assumed that DEVICE_CONTROL usage is always an array
but a lot of devices implements it as a bitmask variable. This led to
the pidff_reset function not working and causing errors in such cases.

Selectors can come in three types. One selection of a set, N selections
and Any selection in form of bitmask as from USB Hid Usage Tables v1.5,
subsection 3.4.2.1

Added pidff_send_device_control which handles usage flag check which
decides whether DEVICE_CONTROL should be handled as "One selection of a
set" or "Any selection of a set".

Reset was triggered once, on device initialization. Now, it's triggered
every time when uploading an effect to an empty device (no currently
stored effects), tracked by pidff->effect_count variable.

Co-developed-by: Makarenko Oleg <oleg@makarenk.ooo>
Signed-off-by: Makarenko Oleg <oleg@makarenk.ooo>
Signed-off-by: Tomasz Pakuła <tomasz.pakula.oficjalny@gmail.com>
Reviewed-by: Michał Kopeć <michal@nozomi.space>
Reviewed-by: Paul Dino Jones <paul@spacefreak18.xyz>
Tested-by: Paul Dino Jones <paul@spacefreak18.xyz>
Tested-by: Cristóferson Bueno <cbueno81@gmail.com>
Tested-by: Pablo Cisneros <patchkez@protonmail.com>
Signed-off-by: Jiri Kosina <jkosina@suse.com>

diff --git a/drivers/hid/usbhid/hid-pidff.c b/drivers/hid/usbhid/hid-pidff.c
index e6224e797dc6da8c94ca8ef9c4407daa9e77011e..7db63d686c621878e254b0a75b1c4465b4b4673e 100644 (file)
--- a/drivers/hid/usbhid/hid-pidff.c
+++ b/drivers/hid/usbhid/hid-pidff.c
@@ -109,9 +109,10 @@ static const u8 pidff_pool[] = { 0x80, 0x83, 0xa9 };
 /* Special field key tables used to put special field keys into arrays */
 
 #define PID_ENABLE_ACTUATORS   0
-#define PID_STOP_ALL_EFFECTS   1
-#define PID_RESET              2
-static const u8 pidff_device_control[] = { 0x97, 0x99, 0x9a };
+#define PID_DISABLE_ACTUATORS  1
+#define PID_STOP_ALL_EFFECTS   2
+#define PID_RESET              3
+static const u8 pidff_device_control[] = { 0x97, 0x98, 0x99, 0x9a };
 
 #define PID_CONSTANT   0
 #define PID_RAMP       1
@@ -190,6 +191,7 @@ struct pidff_device {
        int pid_id[PID_EFFECTS_MAX];
 
        u32 quirks;
+       u8 effect_count;
 };
 
 /*
@@ -490,9 +492,83 @@ static int pidff_needs_set_ramp(struct ff_effect *effect, struct ff_effect *old)
               effect->u.ramp.end_level != old->u.ramp.end_level;
 }
 
+/*
+ * Clear device control report
+ */
+static void pidff_send_device_control(struct pidff_device *pidff, int field)
+{
+       int i, tmp;
+       int field_index = pidff->control_id[field];
+
+       /* Detect if the field is a bitmask variable or an array */
+       if (pidff->device_control->flags & HID_MAIN_ITEM_VARIABLE) {
+               hid_dbg(pidff->hid, "DEVICE_CONTROL is a bitmask\n");
+               /* Clear current bitmask */
+               for(i = 0; i < sizeof(pidff_device_control); i++) {
+                       tmp = pidff->control_id[i];
+                       pidff->device_control->value[tmp] = 0;
+               }
+               pidff->device_control->value[field_index - 1] = 1;
+       } else {
+               hid_dbg(pidff->hid, "DEVICE_CONTROL is an array\n");
+               pidff->device_control->value[0] = field_index;
+       }
+
+       hid_hw_request(pidff->hid, pidff->reports[PID_DEVICE_CONTROL], HID_REQ_SET_REPORT);
+       hid_hw_wait(pidff->hid);
+}
+
+/*
+ * Modify actuators state
+ */
+static void pidff_modify_actuators_state(struct pidff_device *pidff, bool enable)
+{
+       hid_dbg(pidff->hid, "%s actuators\n", enable ? "Enable" : "Disable");
+       pidff_send_device_control(pidff,
+               enable ? PID_ENABLE_ACTUATORS : PID_DISABLE_ACTUATORS);
+}
+
+/*
+ * Reset the device, stop all effects, enable actuators
+ * Refetch pool report
+ */
+static void pidff_reset(struct pidff_device *pidff)
+{
+       int i = 0;
+
+       /* We reset twice as sometimes hid_wait_io isn't waiting long enough */
+       pidff_send_device_control(pidff, PID_RESET);
+       pidff_send_device_control(pidff, PID_RESET);
+       pidff->effect_count = 0;
+
+       pidff_send_device_control(pidff, PID_STOP_ALL_EFFECTS);
+       pidff_modify_actuators_state(pidff, 1);
+
+       /* pool report is sometimes messed up, refetch it */
+       hid_hw_request(pidff->hid, pidff->reports[PID_POOL], HID_REQ_GET_REPORT);
+       hid_hw_wait(pidff->hid);
+
+       if (pidff->pool[PID_SIMULTANEOUS_MAX].value) {
+               while (pidff->pool[PID_SIMULTANEOUS_MAX].value[0] < 2) {
+                       if (i++ > 20) {
+                               hid_warn(pidff->hid,
+                                        "device reports %d simultaneous effects\n",
+                                        pidff->pool[PID_SIMULTANEOUS_MAX].value[0]);
+                               break;
+                       }
+                       hid_dbg(pidff->hid, "pid_pool requested again\n");
+                       hid_hw_request(pidff->hid, pidff->reports[PID_POOL],
+                                         HID_REQ_GET_REPORT);
+                       hid_hw_wait(pidff->hid);
+               }
+       }
+}
+
 /*
  * Send a request for effect upload to the device
  *
+ * Reset and enable actuators if no effects were present on the device
+ *
  * Returns 0 if device reported success, -ENOSPC if the device reported memory
  * is full. Upon unknown response the function will retry for 60 times, if
  * still unsuccessful -EIO is returned.
@@ -501,6 +577,9 @@ static int pidff_request_effect_upload(struct pidff_device *pidff, int efnum)
 {
        int j;
 
+       if (!pidff->effect_count)
+               pidff_reset(pidff);
+
        pidff->create_new_effect_type->value[0] = efnum;
        hid_hw_request(pidff->hid, pidff->reports[PID_CREATE_NEW_EFFECT],
                        HID_REQ_SET_REPORT);
@@ -520,6 +599,8 @@ static int pidff_request_effect_upload(struct pidff_device *pidff, int efnum)
                        hid_dbg(pidff->hid, "device reported free memory: %d bytes\n",
                                 pidff->block_load[PID_RAM_POOL_AVAILABLE].value ?
                                 pidff->block_load[PID_RAM_POOL_AVAILABLE].value[0] : -1);
+
+                       pidff->effect_count++;
                        return 0;
                }
                if (pidff->block_load_status->value[0] ==
@@ -568,12 +649,16 @@ static int pidff_playback(struct input_dev *dev, int effect_id, int value)
 
 /*
  * Erase effect with PID id
+ * Decrease the device effect counter
  */
 static void pidff_erase_pid(struct pidff_device *pidff, int pid_id)
 {
        pidff->block_free[PID_EFFECT_BLOCK_INDEX].value[0] = pid_id;
        hid_hw_request(pidff->hid, pidff->reports[PID_BLOCK_FREE],
                        HID_REQ_SET_REPORT);
+
+       if (pidff->effect_count > 0)
+               pidff->effect_count--;
 }
 
 /*
@@ -1211,50 +1296,6 @@ static int pidff_init_fields(struct pidff_device *pidff, struct input_dev *dev)
        return 0;
 }
 
-/*
- * Reset the device
- */
-static void pidff_reset(struct pidff_device *pidff)
-{
-       struct hid_device *hid = pidff->hid;
-       int i = 0;
-
-       pidff->device_control->value[0] = pidff->control_id[PID_RESET];
-       /* We reset twice as sometimes hid_wait_io isn't waiting long enough */
-       hid_hw_request(hid, pidff->reports[PID_DEVICE_CONTROL], HID_REQ_SET_REPORT);
-       hid_hw_wait(hid);
-       hid_hw_request(hid, pidff->reports[PID_DEVICE_CONTROL], HID_REQ_SET_REPORT);
-       hid_hw_wait(hid);
-
-       pidff->device_control->value[0] = pidff->control_id[PID_STOP_ALL_EFFECTS];
-       hid_hw_request(hid, pidff->reports[PID_DEVICE_CONTROL], HID_REQ_SET_REPORT);
-       hid_hw_wait(hid);
-
-       pidff->device_control->value[0] =
-               pidff->control_id[PID_ENABLE_ACTUATORS];
-       hid_hw_request(hid, pidff->reports[PID_DEVICE_CONTROL], HID_REQ_SET_REPORT);
-       hid_hw_wait(hid);
-
-       /* pool report is sometimes messed up, refetch it */
-       hid_hw_request(hid, pidff->reports[PID_POOL], HID_REQ_GET_REPORT);
-       hid_hw_wait(hid);
-
-       if (pidff->pool[PID_SIMULTANEOUS_MAX].value) {
-               while (pidff->pool[PID_SIMULTANEOUS_MAX].value[0] < 2) {
-                       if (i++ > 20) {
-                               hid_warn(pidff->hid,
-                                        "device reports %d simultaneous effects\n",
-                                        pidff->pool[PID_SIMULTANEOUS_MAX].value[0]);
-                               break;
-                       }
-                       hid_dbg(pidff->hid, "pid_pool requested again\n");
-                       hid_hw_request(hid, pidff->reports[PID_POOL],
-                                         HID_REQ_GET_REPORT);
-                       hid_hw_wait(hid);
-               }
-       }
-}
-
 /*
  * Test if autocenter modification is using the supported method
  */
@@ -1320,6 +1361,7 @@ int hid_pidff_init_with_quirks(struct hid_device *hid, __u32 initial_quirks)
 
        pidff->hid = hid;
        pidff->quirks = initial_quirks;
+       pidff->effect_count = 0;
 
        hid_device_io_start(hid);
 
@@ -1336,8 +1378,6 @@ int hid_pidff_init_with_quirks(struct hid_device *hid, __u32 initial_quirks)
        if (error)
                goto fail;
 
-       pidff_reset(pidff);
-
        if (test_bit(FF_GAIN, dev->ffbit)) {
                pidff_set(&pidff->device_gain[PID_DEVICE_GAIN_FIELD], 0xffff);
                hid_hw_request(hid, pidff->reports[PID_DEVICE_GAIN],