Merge pull request #42 from linux-surface/feature/no-no-feedback

Backport simplified feedback sending and remove no_feedback

drivers/misc/ipts/companion.c

@@ -209,22 +209,3 @@ int ipts_request_firmware_config(struct ipts_info *ipts,
  return ret;
 
 }
-
-unsigned int ipts_get_quirks(void)
-{
- unsigned int ret;
-
- // Make sure that access to the companion is synchronized
- mutex_lock(&ipts_companion_lock);
-
- // If the companion is ignored, or doesn't exist, assume that
- // the device doesn't have any quirks
- if (ipts_modparams.ignore_companion || ipts_companion == NULL)
- ret = IPTS_QUIRK_NONE;
- else
- ret = ipts_companion->get_quirks(ipts_companion);
-
- mutex_unlock(&ipts_companion_lock);
-
- return ret;
-}

drivers/misc/ipts/companion.h

@@ -15,7 +15,6 @@
 #include "ipts.h"
 
 bool ipts_companion_available(void);
-unsigned int ipts_get_quirks(void);
 
 int ipts_request_firmware(const struct firmware **fw, const char *name,
  struct device *device);

drivers/misc/ipts/companion/ipts-surface.c

@@ -26,53 +26,6 @@
  MODULE_FIRMWARE("intel/ipts/" X "/vendor_desc.bin"); \
  MODULE_FIRMWARE("intel/ipts/" X "/vendor_kernel.bin")
 
-struct ipts_surface_data {
- const char *hid;
- unsigned int quirks;
-};
-
-// Surface Book 1 / Surface Studio
-static const struct ipts_surface_data ipts_surface_mshw0076 = {
- .hid = "MSHW0076",
- .quirks = IPTS_QUIRK_NO_FEEDBACK,
-};
-
-// Surface Pro 4
-static const struct ipts_surface_data ipts_surface_mshw0078 = {
- .hid = "MSHW0078",
- .quirks = IPTS_QUIRK_NO_FEEDBACK,
-};
-
-// Surface Laptop 1 / 2
-static const struct ipts_surface_data ipts_surface_mshw0079 = {
- .hid = "MSHW0079",
- .quirks = IPTS_QUIRK_NONE,
-};
-
-// Surface Pro 5 / 6
-static const struct ipts_surface_data ipts_surface_mshw0101 = {
- .hid = "MSHW0101",
- .quirks = IPTS_QUIRK_NONE,
-};
-
-// Surface Book 2 15"
-static const struct ipts_surface_data ipts_surface_mshw0102 = {
- .hid = "MSHW0102",
- .quirks = IPTS_QUIRK_NONE,
-};
-
-// Unknown, but firmware exists
-static const struct ipts_surface_data ipts_surface_mshw0103 = {
- .hid = "MSHW0103",
- .quirks = IPTS_QUIRK_NONE,
-};
-
-// Surface Book 2 13"
-static const struct ipts_surface_data ipts_surface_mshw0137 = {
- .hid = "MSHW0137",
- .quirks = IPTS_QUIRK_NONE,
-};
-
 /*
  * Checkpatch complains about the following lines because it sees them as
  * header files mixed with .c files. However, forward declaration is perfectly
@@ -119,7 +72,6 @@ static struct ipts_bin_fw_info *ipts_surface_fw_config[] = {
 static struct ipts_companion ipts_surface_companion = {
  .firmware_request = &ipts_surface_request_firmware,
  .firmware_config = ipts_surface_fw_config,
- .get_quirks = &ipts_surface_get_quirks,
  .name = "ipts_surface",
 };
 
@@ -128,44 +80,26 @@ int ipts_surface_request_firmware(struct ipts_companion *companion,
  struct device *device)
 {
  char fw_path[MAX_IOCL_FILE_PATH_LEN];
- struct ipts_surface_data *data;
 
  if (companion == NULL || companion->data == NULL)
  return -ENOENT;
 
- data = (struct ipts_surface_data *)companion->data;
-
  snprintf(fw_path, MAX_IOCL_FILE_PATH_LEN, IPTS_SURFACE_FW_PATH_FMT,
- data->hid, name);
+ (const char *)companion->data, name);
  return request_firmware(fw, fw_path, device);
 }
 
-unsigned int ipts_surface_get_quirks(struct ipts_companion *companion)
-{
- struct ipts_surface_data *data;
-
- // In case something went wrong, assume that the
- // device doesn't have any quirks
- if (companion == NULL || companion->data == NULL)
- return IPTS_QUIRK_NONE;
-
- data = (struct ipts_surface_data *)companion->data;
-
- return data->quirks;
-}
-
 static int ipts_surface_probe(struct platform_device *pdev)
 {
  int r;
- const struct ipts_surface_data *data =
- acpi_device_get_match_data(&pdev->dev);
+ struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
 
- if (!data) {
+ if (!adev) {
  dev_err(&pdev->dev, "Unable to find ACPI info for device\n");
  return -ENODEV;
  }
 
- ipts_surface_companion.data = (void *)data;
+ ipts_surface_companion.data = (void *)acpi_device_hid(adev);
 
  r = ipts_add_companion(&ipts_surface_companion);
  if (r) {
@@ -189,13 +123,13 @@ static int ipts_surface_remove(struct platform_device *pdev)
 }
 
 static const struct acpi_device_id ipts_surface_acpi_match[] = {
- { "MSHW0076", (unsigned long)&ipts_surface_mshw0076 },
- { "MSHW0078", (unsigned long)&ipts_surface_mshw0078 },
- { "MSHW0079", (unsigned long)&ipts_surface_mshw0079 },
- { "MSHW0101", (unsigned long)&ipts_surface_mshw0101 },
- { "MSHW0102", (unsigned long)&ipts_surface_mshw0102 },
- { "MSHW0103", (unsigned long)&ipts_surface_mshw0103 },
- { "MSHW0137", (unsigned long)&ipts_surface_mshw0137 },
+ { "MSHW0076", 0 }, // Surface Book 1 / Surface Studio
+ { "MSHW0078", 0 }, // Surface Pro 4
+ { "MSHW0079", 0 }, // Surface Laptop 1 / 2
+ { "MSHW0101", 0 }, // Surface Book 2 15"
+ { "MSHW0102", 0 }, // Surface Pro 5 / 6
+ { "MSHW0103", 0 }, // Unknown
+ { "MSHW0137", 0 }, // Surface Book 2
  { },
 };
 MODULE_DEVICE_TABLE(acpi, ipts_surface_acpi_match);
@@ -220,5 +154,4 @@ IPTS_SURFACE_FIRMWARE("MSHW0079");
 IPTS_SURFACE_FIRMWARE("MSHW0101");
 IPTS_SURFACE_FIRMWARE("MSHW0102");
 IPTS_SURFACE_FIRMWARE("MSHW0103");
-
 IPTS_SURFACE_FIRMWARE("MSHW0137");

drivers/misc/ipts/hid.c

@@ -356,10 +356,11 @@ int ipts_handle_hid_data(struct ipts_info *ipts,
 static int handle_outputs(struct ipts_info *ipts, int parallel_idx)
 {
  struct kernel_output_buffer_header *out_buf_hdr;
- struct ipts_buffer_info *output_buf, *fb_buf = NULL;
+ struct ipts_buffer_info *output_buf;
  u8 *input_report, *payload;
- u32 tr_id;
- int i, payload_size, ret = 0, header_size;
+ u8 tr_id;
+ int i, payload_size, header_size;
+ bool send_feedback = false;
 
  header_size = sizeof(struct kernel_output_buffer_header);
  output_buf = ipts_get_output_buffers_by_parallel_id(ipts,
@@ -372,6 +373,9 @@ static int handle_outputs(struct ipts_info *ipts, int parallel_idx)
  if (out_buf_hdr->length < header_size)
  continue;
 
+ tr_id = *(u8 *)&out_buf_hdr->hid_private_data.transaction_id;
+ send_feedback = true;
+
  payload_size = out_buf_hdr->length - header_size;
  payload = out_buf_hdr->data;
 
@@ -394,12 +398,7 @@ static int handle_outputs(struct ipts_info *ipts, int parallel_idx)
  break;
  }
  case OUTPUT_BUFFER_PAYLOAD_FEEDBACK_BUFFER: {
- // send feedback data for raw data mode
- fb_buf = ipts_get_feedback_buffer(ipts, parallel_idx);
- tr_id = out_buf_hdr->hid_private_data.transaction_id;
-
- memcpy(fb_buf->addr, payload, payload_size);
-
+ // Ignored
  break;
  }
  case OUTPUT_BUFFER_PAYLOAD_ERROR: {
@@ -429,42 +428,10 @@ static int handle_outputs(struct ipts_info *ipts, int parallel_idx)
  }
  }
 
- /*
- * XXX: Calling the "ipts_send_feedback" function repeatedly seems to
- * be what is causing touch to crash (found by sebanc, see the link
- * below for the comment) on some models, especially on Surface Pro 4
- * and Surface Book 1.
- * The most desirable fix could be done by raising IPTS GuC priority.
- * Until we find a better solution, use this workaround.
- *
- * The decision which devices have no_feedback enabled by default is
- * made by the companion driver. If no companion driver was loaded,
- * no_feedback is disabled and the default behaviour is used.
- *
- * Link to the comment where sebanc found this workaround:
- * https://github.com/jakeday/linux-surface/issues/374#issuecomment-508234110
- * (Touch and pen issue persists · Issue #374 · jakeday/linux-surface)
- *
- * Link to the usage from kitakar5525 who made this change:
- * https://github.com/jakeday/linux-surface/issues/374#issuecomment-517289171
- * (Touch and pen issue persists · Issue #374 · jakeday/linux-surface)
- */
- if (fb_buf) {
- // A negative value means "decide by dmi table"
- if (ipts_modparams.no_feedback < 0) {
- if (ipts_get_quirks() & IPTS_QUIRK_NO_FEEDBACK)
- ipts_modparams.no_feedback = true;
- else
- ipts_modparams.no_feedback = false;
- }
 
- if (ipts_modparams.no_feedback)
- return 0;
 
- ret = ipts_send_feedback(ipts, parallel_idx, tr_id);
- if (ret)
- return ret;
- }
+ if (send_feedback)
+ return ipts_send_feedback(ipts, parallel_idx, tr_id);
 
  return 0;
 }

drivers/misc/ipts/msg-handler.c

@@ -42,16 +42,24 @@ int ipts_handle_cmd(struct ipts_info *ipts, u32 cmd, void *data, int data_size)
 int ipts_send_feedback(struct ipts_info *ipts, int buffer_idx,
  u32 transaction_id)
 {
- int cmd_len = sizeof(struct touch_sensor_feedback_ready_cmd_data);
- struct touch_sensor_feedback_ready_cmd_data fb_ready_cmd;
+ struct ipts_buffer_info feedback_buffer;
+ struct touch_feedback_hdr *feedback;
+ struct touch_sensor_feedback_ready_cmd_data cmd;
 
- memset(&fb_ready_cmd, 0, cmd_len);
+ feedback_buffer = ipts->resource.feedback_buffer[buffer_idx];
+ feedback = (struct touch_feedback_hdr *)feedback_buffer.addr;
 
- fb_ready_cmd.feedback_index = buffer_idx;
- fb_ready_cmd.transaction_id = transaction_id;
+ memset(feedback, 0, sizeof(struct touch_feedback_hdr));
+ memset(&cmd, 0, sizeof(struct touch_sensor_feedback_ready_cmd_data));
+
+ feedback->feedback_cmd_type = TOUCH_FEEDBACK_CMD_TYPE_NONE;
+ feedback->buffer_id = transaction_id;
+
+ cmd.feedback_index = buffer_idx;
+ cmd.transaction_id = transaction_id;
 
  return ipts_handle_cmd(ipts, TOUCH_SENSOR_FEEDBACK_READY_CMD,
- &fb_ready_cmd, cmd_len);
+ &cmd, sizeof(struct touch_sensor_feedback_ready_cmd_data));
 }
 
 int ipts_send_sensor_quiesce_io_cmd(struct ipts_info *ipts)
@@ -339,6 +347,7 @@ int ipts_handle_resp(struct ipts_info *ipts,
  }
  case TOUCH_SENSOR_FEEDBACK_READY_RSP: {
  if (rsp_status != TOUCH_STATUS_COMPAT_CHECK_FAIL &&
+ rsp_status != TOUCH_STATUS_INVALID_PARAMS &&
  rsp_status != 0) {
  rsp_failed(ipts, cmd, rsp_status);
  break;

drivers/misc/ipts/params.c

@@ -18,7 +18,6 @@ struct ipts_params ipts_modparams = {
  .ignore_fw_fallback = false,
  .ignore_config_fallback = false,
  .ignore_companion = false,
- .no_feedback = -1,
 
  .debug = false,
  .debug_thread = false,
@@ -33,9 +32,6 @@ IPTS_PARAM(ignore_config_fallback, bool, 0400,
 IPTS_PARAM(ignore_companion, bool, 0400,
  "Don't use a companion driver to load firmware. (default: false)"
 );
-IPTS_PARAM(no_feedback, int, 0644,
- "Disable sending feedback to ME (can prevent crashes on Skylake). (-1=auto [default], 0=false, 1=true)"
-);
 
 IPTS_PARAM(debug, bool, 0400,
  "Enable IPTS debugging output. (default: false)"

drivers/misc/ipts/params.h

@@ -15,7 +15,6 @@ struct ipts_params {
  bool ignore_fw_fallback;
  bool ignore_config_fallback;
  bool ignore_companion;
- int no_feedback;
 
  bool debug;
  bool debug_thread;

include/linux/ipts-companion.h

@@ -14,7 +14,6 @@
 #include <linux/ipts-binary.h>
 
 struct ipts_companion {
- unsigned int (*get_quirks)(struct ipts_companion *companion);
  int (*firmware_request)(struct ipts_companion *companion,
  const struct firmware **fw,
  const char *name, struct device *device);

include/linux/ipts.h

@@ -15,6 +15,5 @@
 #define MAX_IOCL_FILE_PATH_LEN 256
 
 #define IPTS_QUIRK_NONE 0
-#define IPTS_QUIRK_NO_FEEDBACK BIT(0)
 
 #endif // IPTS_H