From 2b2e951a80d8ce4e639218f10c13b6fc71deb77a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ronald=20Tschal=C3=A4r?= Date: Tue, 1 Aug 2017 01:00:44 -0700 Subject: [PATCH 01/10] applespi: Fix race during spi-master registration. The spi-master-device registration registers the device before it starts the queue and marks the device as running. Because device registration immediately runs all class hooks, we end up with a small window where we are notified that the spi-master device is available but it's not actually ready to receive commands. Therefore we now check if spi master is marked running and wait a bit and retry if not. This fixes some sporadic touchpad ininitialization failures. --- applespi.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/applespi.c b/applespi.c index 5268d8b..5614187 100644 --- a/applespi.c +++ b/applespi.c @@ -146,7 +146,7 @@ struct appleacpi_spi_registration_info { struct acpi_device *adev; struct spi_device *spi; struct spi_master *spi_master; - struct work_struct work; + struct delayed_work work; struct notifier_block slave_notifier; }; @@ -1492,7 +1492,13 @@ static int appleacpi_register_spi_device(struct spi_master *spi_master, static void appleacpi_dev_registration_worker(struct work_struct *work) { struct appleacpi_spi_registration_info *info = - container_of(work, struct appleacpi_spi_registration_info, work); + container_of(work, struct appleacpi_spi_registration_info, work.work); + + if (info->spi_master && !info->spi_master->running) { + pr_debug_ratelimited("spi-master device is not running yet\n"); + schedule_delayed_work(&info->work, usecs_to_jiffies(100)); + return; + } appleacpi_register_spi_device(info->spi_master, info->adev); } @@ -1526,7 +1532,7 @@ static int appleacpi_spi_master_added(struct device *dev, * so need to do the actual registration in a worker. */ info->spi_master = spi_master_get(spi_master); - schedule_work(&info->work); + schedule_delayed_work(&info->work, usecs_to_jiffies(100)); return 0; } @@ -1613,7 +1619,7 @@ static int appleacpi_probe(struct acpi_device *adev) } reg_info->adev = adev; - INIT_WORK(®_info->work, appleacpi_dev_registration_worker); + INIT_DELAYED_WORK(®_info->work, appleacpi_dev_registration_worker); adev->driver_data = reg_info; @@ -1673,7 +1679,7 @@ static int appleacpi_remove(struct acpi_device *adev) class_interface_unregister(®_info->cif); bus_unregister_notifier(&spi_bus_type, ®_info->slave_notifier); - cancel_work_sync(®_info->work); + cancel_delayed_work_sync(®_info->work); if (reg_info->spi) spi_unregister_device(reg_info->spi); kfree(reg_info); From 9f1673a27de7aa5bcaed49ad5b467b69ae012cee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ronald=20Tschal=C3=A4r?= Date: Sat, 5 Aug 2017 15:02:22 -0700 Subject: [PATCH 02/10] applespi: Always wait a bit between asserting cs and reading. We were already doing this for read-after-write (commit 21155ff0c9bc) and async-read (commit a6d8c1acb511). This now adds that for the flush-read during touchpad init, and refactors applespi_setup_read_txfr() to set up the delay. --- applespi.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/applespi.c b/applespi.c index 5614187..c8110f5 100644 --- a/applespi.c +++ b/applespi.c @@ -390,10 +390,13 @@ applespi_debug_facility(unsigned log_mask) static void applespi_setup_read_txfr(struct applespi_data *applespi, - struct spi_transfer *rd_t) + struct spi_transfer *dl_t, struct spi_transfer *rd_t) { + memset(dl_t, 0, sizeof *dl_t); memset(rd_t, 0, sizeof *rd_t); + dl_t->delay_usecs = applespi->spi_settings.reset_a2r_usec; + rd_t->rx_buf = applespi->rx_buffer; rd_t->len = APPLESPI_PACKET_SIZE; rd_t->delay_usecs = applespi->spi_settings.spi_cs_delay; @@ -486,9 +489,7 @@ applespi_sync_write_and_response(struct applespi_data *applespi, applespi_setup_write_txfr(applespi, &t1, &t2); t2.cs_change = 1; - memset(&t3, 0, sizeof(t3)); - t3.delay_usecs = applespi->spi_settings.reset_rec_usec; - applespi_setup_read_txfr(applespi, &t4); + applespi_setup_read_txfr(applespi, &t3, &t4); applespi_setup_spi_message(&m, 4, &t1, &t2, &t3, &t4); ret = applespi_sync(applespi, &m); @@ -510,12 +511,13 @@ applespi_sync_write_and_response(struct applespi_data *applespi, static inline ssize_t applespi_sync_read(struct applespi_data *applespi, unsigned log_mask) { + struct spi_transfer d; struct spi_transfer t; struct spi_message m; ssize_t ret; - applespi_setup_read_txfr(applespi, &t); - applespi_setup_spi_message(&m, 1, &t); + applespi_setup_read_txfr(applespi, &d, &t); + applespi_setup_spi_message(&m, 2, &d, &t); ret = applespi_sync(applespi, &m); @@ -1043,9 +1045,7 @@ static u32 applespi_notify(acpi_handle gpe_device, u32 gpe, void *context) debug_print(DBG_RD_IRQ, "--- %s ---------------------------\n", applespi_debug_facility(DBG_RD_IRQ)); - memset(&applespi->dl_t, 0, sizeof(applespi->dl_t)); - applespi->dl_t.delay_usecs = applespi->spi_settings.reset_a2r_usec; - applespi_setup_read_txfr(applespi, &applespi->rd_t); + applespi_setup_read_txfr(applespi, &applespi->dl_t, &applespi->rd_t); applespi_setup_spi_message(&applespi->rd_m, 2, &applespi->dl_t, &applespi->rd_t); applespi_async(applespi, &applespi->rd_m, applespi_async_read_complete); From 5806dcbf1583654d21fcb0c7dc2eeed4ba105207 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ronald=20Tschal=C3=A4r?= Date: Sun, 20 Aug 2017 22:16:28 -0700 Subject: [PATCH 03/10] applespi: Reorder init code to avoid races. In particular we need to ensure the keyboard input handler is registered after all structures are initialized because it registers an event handler and hence we can be called at any point after registration. Also moved the USB check to the very top, since there's no point in doing anything if that succeeds. --- applespi.c | 55 +++++++++++++++++++++++++++--------------------------- 1 file changed, 27 insertions(+), 28 deletions(-) diff --git a/applespi.c b/applespi.c index c8110f5..ff7ffdc 100644 --- a/applespi.c +++ b/applespi.c @@ -1058,12 +1058,24 @@ static int applespi_probe(struct spi_device *spi) int result, i; long long unsigned int gpe, usb_status; + /* Check if the USB interface is present and enabled already */ + result = acpi_evaluate_integer(ACPI_HANDLE(&spi->dev), "UIST", NULL, &usb_status); + if (ACPI_SUCCESS(result) && usb_status) { + /* Let the USB driver take over instead */ + pr_info("USB interface already enabled\n"); + return -ENODEV; + } + /* Allocate driver data */ applespi = devm_kzalloc(&spi->dev, sizeof(*applespi), GFP_KERNEL); if (!applespi) return -ENOMEM; applespi->spi = spi; + applespi->handle = ACPI_HANDLE(&spi->dev); + + /* Store the driver data */ + spi_set_drvdata(spi, applespi); /* Create our buffers */ applespi->tx_buffer = devm_kmalloc(&spi->dev, APPLESPI_PACKET_SIZE, GFP_KERNEL); @@ -1073,8 +1085,21 @@ static int applespi_probe(struct spi_device *spi) if (!applespi->tx_buffer || !applespi->tx_status || !applespi->rx_buffer) return -ENOMEM; - /* Store the driver data */ - spi_set_drvdata(spi, applespi); + /* Cache ACPI method handles */ + if (ACPI_FAILURE(acpi_get_handle(applespi->handle, "SIEN", &applespi->sien)) || + ACPI_FAILURE(acpi_get_handle(applespi->handle, "SIST", &applespi->sist))) { + pr_err("Failed to get required ACPI method handle\n"); + return -ENODEV; + } + + /* Switch on the SPI interface */ + result = applespi_setup_spi(applespi); + if (result) + return result; + + result = applespi_enable_spi(applespi); + if (result) + return result; /* Set up touchpad dimensions */ applespi->tp_info = dmi_first_match(applespi_touchpad_infos)->driver_data; @@ -1167,32 +1192,6 @@ static int applespi_probe(struct spi_device *spi) return -ENODEV; } - applespi->handle = ACPI_HANDLE(&spi->dev); - - /* Check if the USB interface is present and enabled already */ - result = acpi_evaluate_integer(applespi->handle, "UIST", NULL, &usb_status); - if (ACPI_SUCCESS(result) && usb_status) { - /* Let the USB driver take over instead */ - pr_info("USB interface already enabled\n"); - return -ENODEV; - } - - /* Cache ACPI method handles */ - if (ACPI_FAILURE(acpi_get_handle(applespi->handle, "SIEN", &applespi->sien)) || - ACPI_FAILURE(acpi_get_handle(applespi->handle, "SIST", &applespi->sist))) { - pr_err("Failed to get required ACPI method handle\n"); - return -ENODEV; - } - - /* Switch on the SPI interface */ - result = applespi_setup_spi(applespi); - if (result) - return result; - - result = applespi_enable_spi(applespi); - if (result) - return result; - /* Switch the touchpad into multitouch mode */ applespi_init(applespi); From 51e761744f406e34cd07a7e54a72fc641a6162c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ronald=20Tschal=C3=A4r?= Date: Sat, 12 Aug 2017 17:07:44 -0700 Subject: [PATCH 04/10] applespi: Factored out keyboard event handling into separate function. --- applespi.c | 98 +++++++++++++++++++++++++++++------------------------- 1 file changed, 53 insertions(+), 45 deletions(-) diff --git a/applespi.c b/applespi.c index ff7ffdc..2310b15 100644 --- a/applespi.c +++ b/applespi.c @@ -954,70 +954,78 @@ applespi_code_to_key(u8 code, int fn_pressed) } static void -applespi_got_data(struct applespi_data *applespi) +applespi_handle_keyboard_event(struct applespi_data *applespi, + struct keyboard_protocol *keyboard_protocol) { - struct keyboard_protocol keyboard_protocol; int i, j; unsigned int key; bool still_pressed; - memcpy(&keyboard_protocol, applespi->rx_buffer, APPLESPI_PACKET_SIZE); - if (keyboard_protocol.packet_type == PACKET_KEYBOARD) { - debug_print(DBG_RD_KEYB, "--- %s ---------------------------\n", - applespi_debug_facility(DBG_RD_KEYB)); - debug_print_buffer(DBG_RD_KEYB, "read ", applespi->rx_buffer, - APPLESPI_PACKET_SIZE); - - for (i=0; i<6; i++) { - still_pressed = false; - for (j=0; j<6; j++) { - if (applespi->last_keys_pressed[i] == keyboard_protocol.keys_pressed[j]) { - still_pressed = true; - break; - } - } - - if (! still_pressed) { - key = applespi_code_to_key(applespi->last_keys_pressed[i], applespi->last_keys_fn_pressed[i]); - input_report_key(applespi->keyboard_input_dev, key, 0); - applespi->last_keys_fn_pressed[i] = 0; + for (i=0; i<6; i++) { + still_pressed = false; + for (j=0; j<6; j++) { + if (applespi->last_keys_pressed[i] == keyboard_protocol->keys_pressed[j]) { + still_pressed = true; + break; } } - for (i=0; i<6; i++) { - if (keyboard_protocol.keys_pressed[i] < ARRAY_SIZE(applespi_scancodes) && keyboard_protocol.keys_pressed[i] > 0) { - key = applespi_code_to_key(keyboard_protocol.keys_pressed[i], keyboard_protocol.fn_pressed); - input_report_key(applespi->keyboard_input_dev, key, 1); - applespi->last_keys_fn_pressed[i] = keyboard_protocol.fn_pressed; - } + if (! still_pressed) { + key = applespi_code_to_key(applespi->last_keys_pressed[i], applespi->last_keys_fn_pressed[i]); + input_report_key(applespi->keyboard_input_dev, key, 0); + applespi->last_keys_fn_pressed[i] = 0; } + } - // Check control keys - for (i=0; i<8; i++) { - if (test_bit(i, (long unsigned int *)&keyboard_protocol.modifiers)) { - input_report_key(applespi->keyboard_input_dev, applespi_controlcodes[i], 1); - } else { - input_report_key(applespi->keyboard_input_dev, applespi_controlcodes[i], 0); - } + for (i=0; i<6; i++) { + if (keyboard_protocol->keys_pressed[i] < ARRAY_SIZE(applespi_scancodes) && keyboard_protocol->keys_pressed[i] > 0) { + key = applespi_code_to_key(keyboard_protocol->keys_pressed[i], keyboard_protocol->fn_pressed); + input_report_key(applespi->keyboard_input_dev, key, 1); + applespi->last_keys_fn_pressed[i] = keyboard_protocol->fn_pressed; } + } - // Check function key - if (keyboard_protocol.fn_pressed && !applespi->last_fn_pressed) { - input_report_key(applespi->keyboard_input_dev, KEY_FN, 1); - } else if (!keyboard_protocol.fn_pressed && applespi->last_fn_pressed) { - input_report_key(applespi->keyboard_input_dev, KEY_FN, 0); + // Check control keys + for (i=0; i<8; i++) { + if (test_bit(i, (long unsigned int *)&keyboard_protocol->modifiers)) { + input_report_key(applespi->keyboard_input_dev, applespi_controlcodes[i], 1); + } else { + input_report_key(applespi->keyboard_input_dev, applespi_controlcodes[i], 0); } - applespi->last_fn_pressed = keyboard_protocol.fn_pressed; + } + + // Check function key + if (keyboard_protocol->fn_pressed && !applespi->last_fn_pressed) { + input_report_key(applespi->keyboard_input_dev, KEY_FN, 1); + } else if (!keyboard_protocol->fn_pressed && applespi->last_fn_pressed) { + input_report_key(applespi->keyboard_input_dev, KEY_FN, 0); + } + applespi->last_fn_pressed = keyboard_protocol->fn_pressed; + + input_sync(applespi->keyboard_input_dev); + memcpy(&applespi->last_keys_pressed, keyboard_protocol->keys_pressed, sizeof(applespi->last_keys_pressed)); +} + +static void +applespi_got_data(struct applespi_data *applespi) +{ + struct keyboard_protocol *keyboard_protocol; + + keyboard_protocol = (struct keyboard_protocol*) applespi->rx_buffer; + if (keyboard_protocol->packet_type == PACKET_KEYBOARD) { + debug_print(DBG_RD_KEYB, "--- %s ---------------------------\n", + applespi_debug_facility(DBG_RD_KEYB)); + debug_print_buffer(DBG_RD_KEYB, "read ", applespi->rx_buffer, + APPLESPI_PACKET_SIZE); - input_sync(applespi->keyboard_input_dev); - memcpy(&applespi->last_keys_pressed, keyboard_protocol.keys_pressed, sizeof(applespi->last_keys_pressed)); - } else if (keyboard_protocol.packet_type == PACKET_TOUCHPAD) { + applespi_handle_keyboard_event(applespi, keyboard_protocol); + } else if (keyboard_protocol->packet_type == PACKET_TOUCHPAD) { debug_print(DBG_RD_TPAD, "--- %s ---------------------------\n", applespi_debug_facility(DBG_RD_TPAD)); debug_print_buffer(DBG_RD_TPAD, "read ", applespi->rx_buffer, APPLESPI_PACKET_SIZE); - report_tp_state(applespi, (struct touchpad_protocol*)&keyboard_protocol); + report_tp_state(applespi, (struct touchpad_protocol*) keyboard_protocol); } else { debug_print(DBG_RD_UNKN, "--- %s ---------------------------\n", applespi_debug_facility(DBG_RD_UNKN)); From a85f1f3da4903270b1c9350a433d3ee45423e268 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ronald=20Tschal=C3=A4r?= Date: Sat, 12 Aug 2017 17:31:20 -0700 Subject: [PATCH 05/10] applespi: Handle command response messages correctly. After sending any command we will receive a response message (this includes the backlight and caps-lock commands). But that may not be the next message received. However, all received messages are signaled by an interrupt. So the receiving and processing of those response messages is now left to our regular receive handler (currently all we do with the messages is log them). --- applespi.c | 49 +++++++++++++++++++++++++++++-------------------- 1 file changed, 29 insertions(+), 20 deletions(-) diff --git a/applespi.c b/applespi.c index 2310b15..ed1f6b1 100644 --- a/applespi.c +++ b/applespi.c @@ -43,8 +43,10 @@ #define APPLESPI_PACKET_SIZE 256 #define APPLESPI_STATUS_SIZE 4 -#define PACKET_KEYBOARD 288 -#define PACKET_TOUCHPAD 544 +#define PACKET_TYPE_READ 0x20 +#define PACKET_TYPE_WRITE 0x40 +#define PACKET_DEV_KEYB 0x01 +#define PACKET_DEV_TPAD 0x02 #define MAX_ROLLOVER 6 @@ -94,7 +96,8 @@ MODULE_PARM_DESC(debug, "Enable/Disable debug logging. This is a bitmask."); struct keyboard_protocol { - u16 packet_type; + u8 packet_type; + u8 device; u8 unknown1[9]; u8 counter; u8 unknown2[5]; @@ -125,7 +128,8 @@ struct tp_finger { } __attribute__((packed,aligned(2))); struct touchpad_protocol { - u16 packet_type; + u8 packet_type; + u8 device; u8 unknown1[4]; u8 number_of_fingers; u8 unknown2[4]; @@ -472,25 +476,21 @@ applespi_check_write_status(struct applespi_data *applespi, int sts) } static inline ssize_t -applespi_sync_write_and_response(struct applespi_data *applespi, - unsigned log_mask) +applespi_sync_write(struct applespi_data *applespi, unsigned log_mask) { /* The Windows driver always seems to do a 256 byte write, followed by a 4 byte read with CS still the same, followed by a toggling of - CS and a 256 byte read for the real response. + CS and a 256 byte read for the real response. We will get the real + response on a read after an interrupt. */ struct spi_transfer t1; struct spi_transfer t2; - struct spi_transfer t3; - struct spi_transfer t4; struct spi_message m; ssize_t ret; applespi_setup_write_txfr(applespi, &t1, &t2); - t2.cs_change = 1; - applespi_setup_read_txfr(applespi, &t3, &t4); - applespi_setup_spi_message(&m, 4, &t1, &t2, &t3, &t4); + applespi_setup_spi_message(&m, 2, &t1, &t2); ret = applespi_sync(applespi, &m); @@ -500,8 +500,6 @@ applespi_sync_write_and_response(struct applespi_data *applespi, APPLESPI_PACKET_SIZE); debug_print_buffer(log_mask, "status ", applespi->tx_status, APPLESPI_STATUS_SIZE); - debug_print_buffer(log_mask, "read ", applespi->rx_buffer, - APPLESPI_PACKET_SIZE); applespi_check_write_status(applespi, ret); @@ -662,9 +660,10 @@ static void applespi_init(struct applespi_data *applespi) // Do a read to flush the trackpad applespi_sync_read(applespi, DBG_CMD_TP_INI); + applespi->cmd_log_mask = DBG_CMD_TP_INI; for (i=0; i < items; i++) { memcpy(applespi->tx_buffer, applespi_init_commands[i], 256); - applespi_sync_write_and_response(applespi, DBG_CMD_TP_INI); + applespi_sync_write(applespi, DBG_CMD_TP_INI); } pr_info("modeswitch done.\n"); @@ -1012,20 +1011,30 @@ applespi_got_data(struct applespi_data *applespi) struct keyboard_protocol *keyboard_protocol; keyboard_protocol = (struct keyboard_protocol*) applespi->rx_buffer; - if (keyboard_protocol->packet_type == PACKET_KEYBOARD) { + if (keyboard_protocol->packet_type == PACKET_TYPE_READ && + keyboard_protocol->device == PACKET_DEV_KEYB) { debug_print(DBG_RD_KEYB, "--- %s ---------------------------\n", applespi_debug_facility(DBG_RD_KEYB)); debug_print_buffer(DBG_RD_KEYB, "read ", applespi->rx_buffer, APPLESPI_PACKET_SIZE); applespi_handle_keyboard_event(applespi, keyboard_protocol); - } else if (keyboard_protocol->packet_type == PACKET_TOUCHPAD) { + + } else if (keyboard_protocol->packet_type == PACKET_TYPE_READ && + keyboard_protocol->device == PACKET_DEV_TPAD) { debug_print(DBG_RD_TPAD, "--- %s ---------------------------\n", applespi_debug_facility(DBG_RD_TPAD)); debug_print_buffer(DBG_RD_TPAD, "read ", applespi->rx_buffer, APPLESPI_PACKET_SIZE); report_tp_state(applespi, (struct touchpad_protocol*) keyboard_protocol); + + } else if (keyboard_protocol->packet_type == PACKET_TYPE_WRITE) { + debug_print(applespi->cmd_log_mask, + "--- %s ---------------------------\n", + applespi_debug_facility(applespi->cmd_log_mask)); + debug_print_buffer(applespi->cmd_log_mask, "read ", + applespi->rx_buffer, APPLESPI_PACKET_SIZE); } else { debug_print(DBG_RD_UNKN, "--- %s ---------------------------\n", applespi_debug_facility(DBG_RD_UNKN)); @@ -1200,9 +1209,6 @@ static int applespi_probe(struct spi_device *spi) return -ENODEV; } - /* Switch the touchpad into multitouch mode */ - applespi_init(applespi); - /* * The applespi device doesn't send interrupts normally (as is described * in its DSDT), but rather seems to use ACPI GPEs. @@ -1230,6 +1236,9 @@ static int applespi_probe(struct spi_device *spi) return -ENODEV; } + /* Switch the touchpad into multitouch mode */ + applespi_init(applespi); + /* set up keyboard-backlight */ applespi->backlight_info.name = "spi::kbd_backlight"; applespi->backlight_info.default_trigger = "kbd-backlight"; From 2c5f054e40b91bfac045b4a7f4ea457d45233fe6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ronald=20Tschal=C3=A4r?= Date: Sun, 13 Aug 2017 21:58:20 -0700 Subject: [PATCH 06/10] applespi: Don't send new command until response has been received. This avoids occasional status codes of (9c 17 e5 f8) if a new command is sent too early. --- applespi.c | 41 ++++++++++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 13 deletions(-) diff --git a/applespi.c b/applespi.c index ed1f6b1..2bad8d9 100644 --- a/applespi.c +++ b/applespi.c @@ -462,17 +462,23 @@ applespi_async(struct applespi_data *applespi, struct spi_message *message, return spi_async(applespi->spi, message); } -static inline void +static inline bool applespi_check_write_status(struct applespi_data *applespi, int sts) { - u8 sts_ok[] = { 0xac, 0x27, 0x68, 0xd5 }; + static u8 sts_ok[] = { 0xac, 0x27, 0x68, 0xd5 }; + bool ret = true; - if (sts < 0) + if (sts < 0) { + ret = false; pr_warn("Error writing to device: %d\n", sts); - else if (memcmp(applespi->tx_status, sts_ok, APPLESPI_STATUS_SIZE) != 0) + } else if (memcmp(applespi->tx_status, sts_ok, APPLESPI_STATUS_SIZE) != 0) { + ret = false; pr_warn("Error writing to device: %x %x %x %x\n", applespi->tx_status[0], applespi->tx_status[1], applespi->tx_status[2], applespi->tx_status[3]); + } + + return ret; } static inline ssize_t @@ -661,6 +667,8 @@ static void applespi_init(struct applespi_data *applespi) applespi_sync_read(applespi, DBG_CMD_TP_INI); applespi->cmd_log_mask = DBG_CMD_TP_INI; + applespi->cmd_msg_queued = true; + for (i=0; i < items; i++) { memcpy(applespi->tx_buffer, applespi_init_commands[i], 256); applespi_sync_write(applespi, DBG_CMD_TP_INI); @@ -671,10 +679,21 @@ static void applespi_init(struct applespi_data *applespi) static int applespi_send_cmd_msg(struct applespi_data *applespi); +static void applespi_cmd_msg_complete(struct applespi_data *applespi) +{ + unsigned long flags; + + spin_lock_irqsave(&applespi->cmd_msg_lock, flags); + + applespi->cmd_msg_queued = false; + applespi_send_cmd_msg(applespi); + + spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags); +} + static void applespi_async_write_complete(void *context) { struct applespi_data *applespi = context; - unsigned long flags; debug_print(applespi->cmd_log_mask, "--- %s ---------------------------\n", applespi_debug_facility(applespi->cmd_log_mask)); @@ -683,14 +702,8 @@ static void applespi_async_write_complete(void *context) debug_print_buffer(applespi->cmd_log_mask, "status ", applespi->tx_status, APPLESPI_STATUS_SIZE); - applespi_check_write_status(applespi, applespi->wr_m.status); - - spin_lock_irqsave(&applespi->cmd_msg_lock, flags); - - applespi->cmd_msg_queued = false; - applespi_send_cmd_msg(applespi); - - spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags); + if (!applespi_check_write_status(applespi, applespi->wr_m.status)) + applespi_cmd_msg_complete(applespi); } static int @@ -1035,6 +1048,8 @@ applespi_got_data(struct applespi_data *applespi) applespi_debug_facility(applespi->cmd_log_mask)); debug_print_buffer(applespi->cmd_log_mask, "read ", applespi->rx_buffer, APPLESPI_PACKET_SIZE); + + applespi_cmd_msg_complete(applespi); } else { debug_print(DBG_RD_UNKN, "--- %s ---------------------------\n", applespi_debug_facility(DBG_RD_UNKN)); From 49e7bc61b2872a16510cfb97913e9181249c67cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ronald=20Tschal=C3=A4r?= Date: Sat, 12 Aug 2017 18:57:55 -0700 Subject: [PATCH 07/10] applespi: Clean up delays. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is an attempt to fix the last remaining occasional failures due to insufficient delays. To summarize we now have only two delays: - the spiCSDelay after asserting CS (for both reads and writes) - a fixed, 100µs delay whenever switching between writes and reads In more detail: spiCSDelay appears to be truly what it says, a delay after asserting CS and before clocking any data. This also implies that it appears to be in units of µs. Furthermore this is needed on writes, not just on reads (so this change includes the write equivalent of commit 3d20af5728f8). The delay at the end of messages (i.e. before de-asserting CS) appears to be unnecessary, and was probably an artifact of earlier versions of this code. Hence removed them. The resetXXX properties are for resetting the device only, something we do not currently implement. So stop using them. Lastly, the 100µs delay between writing and reading is also needed when switching from reading to writing (otherwise we get occasional (b3 b3 b3 b3) write statuses). Since there is no property in the DSDT that directly corresponds to this, it is hardcoded for now. --- applespi.c | 41 ++++++++++++++++++++++++++--------------- 1 file changed, 26 insertions(+), 15 deletions(-) diff --git a/applespi.c b/applespi.c index 2bad8d9..8e756ab 100644 --- a/applespi.c +++ b/applespi.c @@ -79,6 +79,7 @@ #define APPLE_FLAG_FKEY 0x01 #define SPI_DEV_CHIP_SEL 0 // from DSDT UBUF +#define SPI_RW_CHG_DLY 100 /* from experimentation, in us */ static unsigned int fnmode = 1; module_param(fnmode, uint, 0644); @@ -160,7 +161,7 @@ struct spi_settings { u64 spi_bit_order; /* 1 = MSB_FIRST, 0 = LSB_FIRST */ u64 spi_spo; /* clock polarity: 0 = low, 1 = high */ u64 spi_sph; /* clock phase: 0 = first, 1 = second */ - u64 spi_cs_delay; /* in 10us (?) */ + u64 spi_cs_delay; /* cs-to-clk delay in us */ u64 reset_a2r_usec; /* active-to-receive delay? */ u64 reset_rec_usec; /* ? (cur val: 10) */ }; @@ -214,6 +215,7 @@ struct applespi_data { struct spi_transfer rd_t; struct spi_message rd_m; + struct spi_transfer wd_t; struct spi_transfer wr_t; struct spi_transfer st_t; struct spi_message wr_m; @@ -399,27 +401,29 @@ applespi_setup_read_txfr(struct applespi_data *applespi, memset(dl_t, 0, sizeof *dl_t); memset(rd_t, 0, sizeof *rd_t); - dl_t->delay_usecs = applespi->spi_settings.reset_a2r_usec; + dl_t->delay_usecs = applespi->spi_settings.spi_cs_delay; rd_t->rx_buf = applespi->rx_buffer; rd_t->len = APPLESPI_PACKET_SIZE; - rd_t->delay_usecs = applespi->spi_settings.spi_cs_delay; } static void applespi_setup_write_txfr(struct applespi_data *applespi, - struct spi_transfer *wr_t, struct spi_transfer *st_t) + struct spi_transfer *dl_t, struct spi_transfer *wr_t, + struct spi_transfer *st_t) { + memset(dl_t, 0, sizeof *dl_t); memset(wr_t, 0, sizeof *wr_t); memset(st_t, 0, sizeof *st_t); + dl_t->delay_usecs = applespi->spi_settings.spi_cs_delay; + wr_t->tx_buf = applespi->tx_buffer; wr_t->len = APPLESPI_PACKET_SIZE; - wr_t->delay_usecs = applespi->spi_settings.spi_cs_delay; + wr_t->delay_usecs = SPI_RW_CHG_DLY; st_t->rx_buf = applespi->tx_status; st_t->len = APPLESPI_STATUS_SIZE; - st_t->delay_usecs = applespi->spi_settings.spi_cs_delay; } static void @@ -492,11 +496,12 @@ applespi_sync_write(struct applespi_data *applespi, unsigned log_mask) */ struct spi_transfer t1; struct spi_transfer t2; + struct spi_transfer t3; struct spi_message m; ssize_t ret; - applespi_setup_write_txfr(applespi, &t1, &t2); - applespi_setup_spi_message(&m, 2, &t1, &t2); + applespi_setup_write_txfr(applespi, &t1, &t2, &t3); + applespi_setup_spi_message(&m, 3, &t1, &t2, &t3); ret = applespi_sync(applespi, &m); @@ -610,9 +615,6 @@ static int applespi_get_spi_settings(acpi_handle handle, } ACPI_FREE(spi_info); - /* acpi provided value is in 10us units */ - settings->spi_cs_delay *= 10; - return 0; } @@ -764,9 +766,10 @@ applespi_send_cmd_msg(struct applespi_data *applespi) } /* send command */ - applespi_setup_write_txfr(applespi, &applespi->wr_t, &applespi->st_t); - applespi_setup_spi_message(&applespi->wr_m, 2, &applespi->wr_t, - &applespi->st_t); + applespi_setup_write_txfr(applespi, &applespi->wd_t, &applespi->wr_t, + &applespi->st_t); + applespi_setup_spi_message(&applespi->wr_m, 3, &applespi->wd_t, + &applespi->wr_t, &applespi->st_t); sts = applespi_async(applespi, &applespi->wr_m, applespi_async_write_complete); @@ -1049,13 +1052,21 @@ applespi_got_data(struct applespi_data *applespi) debug_print_buffer(applespi->cmd_log_mask, "read ", applespi->rx_buffer, APPLESPI_PACKET_SIZE); - applespi_cmd_msg_complete(applespi); } else { debug_print(DBG_RD_UNKN, "--- %s ---------------------------\n", applespi_debug_facility(DBG_RD_UNKN)); debug_print_buffer(DBG_RD_UNKN, "read ", applespi->rx_buffer, APPLESPI_PACKET_SIZE); } + + /* Note: this relies on the fact that we are blocking the processing of + * spi messages at this point, i.e. that no further transfers or cs + * changes are processed while we delay here. + */ + udelay(SPI_RW_CHG_DLY); + + if (keyboard_protocol->packet_type == PACKET_TYPE_WRITE) + applespi_cmd_msg_complete(applespi); } static void applespi_async_read_complete(void *context) From 2b55d81e36674afe7122087eb489b13298b8a60f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ronald=20Tschal=C3=A4r?= Date: Sat, 12 Aug 2017 18:15:03 -0700 Subject: [PATCH 08/10] applespi: Remove flush read before touchpad init. I believe this isn't necessary anymore, with all the fixes that have occurred in the mean time. And it has the potential to cause an event to get dropped (though unlikely). --- applespi.c | 27 --------------------------- 1 file changed, 27 deletions(-) diff --git a/applespi.c b/applespi.c index 8e756ab..a2a2ba2 100644 --- a/applespi.c +++ b/applespi.c @@ -517,30 +517,6 @@ applespi_sync_write(struct applespi_data *applespi, unsigned log_mask) return ret; } -static inline ssize_t -applespi_sync_read(struct applespi_data *applespi, unsigned log_mask) -{ - struct spi_transfer d; - struct spi_transfer t; - struct spi_message m; - ssize_t ret; - - applespi_setup_read_txfr(applespi, &d, &t); - applespi_setup_spi_message(&m, 2, &d, &t); - - ret = applespi_sync(applespi, &m); - - debug_print(log_mask, "--- %s ---------------------------\n", - applespi_debug_facility(log_mask)); - debug_print_buffer(log_mask, "read ", applespi->rx_buffer, - APPLESPI_PACKET_SIZE); - - if (ret < 0) - pr_warn("Error reading from device: %ld\n", ret); - - return ret; -} - static int applespi_find_settings_field(const char *name) { int i; @@ -665,9 +641,6 @@ static void applespi_init(struct applespi_data *applespi) int i; ssize_t items = ARRAY_SIZE(applespi_init_commands); - // Do a read to flush the trackpad - applespi_sync_read(applespi, DBG_CMD_TP_INI); - applespi->cmd_log_mask = DBG_CMD_TP_INI; applespi->cmd_msg_queued = true; From 48fafded677a95732e032626d6c35730455aab43 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ronald=20Tschal=C3=A4r?= Date: Sun, 20 Aug 2017 21:09:11 -0700 Subject: [PATCH 09/10] applespi: Make touchpad-init command async like other commands. This simplifies the code overall and ensures the contraints on commands and response are fullfilled for the init command(s) too. --- applespi.c | 106 +++++++++++++++++++++-------------------------------- 1 file changed, 41 insertions(+), 65 deletions(-) diff --git a/applespi.c b/applespi.c index a2a2ba2..b73b45c 100644 --- a/applespi.c +++ b/applespi.c @@ -220,6 +220,7 @@ struct applespi_data { struct spi_transfer st_t; struct spi_message wr_m; + int init_cmd_idx; bool want_cl_led_on; bool have_cl_led_on; unsigned want_bl_level; @@ -440,22 +441,6 @@ applespi_setup_spi_message(struct spi_message *message, int num_txfrs, ...) va_end(txfrs); } -static int -applespi_sync(struct applespi_data *applespi, struct spi_message *message) -{ - struct spi_device *spi; - int status; - - spi = applespi->spi; - - status = spi_sync(spi, message); - - if (status == 0) - status = message->actual_length; - - return status; -} - static int applespi_async(struct applespi_data *applespi, struct spi_message *message, void (*complete)(void *)) @@ -485,38 +470,6 @@ applespi_check_write_status(struct applespi_data *applespi, int sts) return ret; } -static inline ssize_t -applespi_sync_write(struct applespi_data *applespi, unsigned log_mask) -{ - /* - The Windows driver always seems to do a 256 byte write, followed - by a 4 byte read with CS still the same, followed by a toggling of - CS and a 256 byte read for the real response. We will get the real - response on a read after an interrupt. - */ - struct spi_transfer t1; - struct spi_transfer t2; - struct spi_transfer t3; - struct spi_message m; - ssize_t ret; - - applespi_setup_write_txfr(applespi, &t1, &t2, &t3); - applespi_setup_spi_message(&m, 3, &t1, &t2, &t3); - - ret = applespi_sync(applespi, &m); - - debug_print(log_mask, "--- %s ---------------------------\n", - applespi_debug_facility(log_mask)); - debug_print_buffer(log_mask, "write ", applespi->tx_buffer, - APPLESPI_PACKET_SIZE); - debug_print_buffer(log_mask, "status ", applespi->tx_status, - APPLESPI_STATUS_SIZE); - - applespi_check_write_status(applespi, ret); - - return ret; -} - static int applespi_find_settings_field(const char *name) { int i; @@ -636,22 +589,6 @@ static int applespi_enable_spi(struct applespi_data *applespi) return 0; } -static void applespi_init(struct applespi_data *applespi) -{ - int i; - ssize_t items = ARRAY_SIZE(applespi_init_commands); - - applespi->cmd_log_mask = DBG_CMD_TP_INI; - applespi->cmd_msg_queued = true; - - for (i=0; i < items; i++) { - memcpy(applespi->tx_buffer, applespi_init_commands[i], 256); - applespi_sync_write(applespi, DBG_CMD_TP_INI); - } - - pr_info("modeswitch done.\n"); -} - static int applespi_send_cmd_msg(struct applespi_data *applespi); static void applespi_cmd_msg_complete(struct applespi_data *applespi) @@ -691,8 +628,20 @@ applespi_send_cmd_msg(struct applespi_data *applespi) if (applespi->cmd_msg_queued) return 0; + /* are we processing init commands? */ + if (applespi->init_cmd_idx >= 0) { + memcpy(applespi->tx_buffer, + applespi_init_commands[applespi->init_cmd_idx], + APPLESPI_PACKET_SIZE); + + applespi->init_cmd_idx++; + if (applespi->init_cmd_idx >= ARRAY_SIZE(applespi_init_commands)) + applespi->init_cmd_idx = -1; + + applespi->cmd_log_mask = DBG_CMD_TP_INI; + /* do we need caps-lock command? */ - if (applespi->want_cl_led_on != applespi->have_cl_led_on) { + } else if (applespi->want_cl_led_on != applespi->have_cl_led_on) { applespi->have_cl_led_on = applespi->want_cl_led_on; applespi->cmd_log_mask = DBG_CMD_CL; @@ -755,6 +704,18 @@ applespi_send_cmd_msg(struct applespi_data *applespi) return sts; } +static void applespi_init(struct applespi_data *applespi) +{ + unsigned long flags; + + spin_lock_irqsave(&applespi->cmd_msg_lock, flags); + + applespi->init_cmd_idx = 0; + applespi_send_cmd_msg(applespi); + + spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags); +} + static int applespi_set_capsl_led(struct applespi_data *applespi, bool capslock_on) { @@ -994,6 +955,16 @@ applespi_handle_keyboard_event(struct applespi_data *applespi, memcpy(&applespi->last_keys_pressed, keyboard_protocol->keys_pressed, sizeof(applespi->last_keys_pressed)); } +static void +applespi_handle_cmd_response(struct applespi_data *applespi, + struct keyboard_protocol *keyboard_protocol) +{ + if (keyboard_protocol->device == PACKET_DEV_TPAD && + memcmp(((u8 *) keyboard_protocol) + 8, + applespi_init_commands[0] + 8, 4) == 0) + pr_info("modeswitch done.\n"); +} + static void applespi_got_data(struct applespi_data *applespi) { @@ -1025,6 +996,7 @@ applespi_got_data(struct applespi_data *applespi) debug_print_buffer(applespi->cmd_log_mask, "read ", applespi->rx_buffer, APPLESPI_PACKET_SIZE); + applespi_handle_cmd_response(applespi, keyboard_protocol); } else { debug_print(DBG_RD_UNKN, "--- %s ---------------------------\n", applespi_debug_facility(DBG_RD_UNKN)); @@ -1057,6 +1029,7 @@ static void applespi_async_read_complete(void *context) static u32 applespi_notify(acpi_handle gpe_device, u32 gpe, void *context) { struct applespi_data *applespi = context; + unsigned long flags; debug_print(DBG_RD_IRQ, "--- %s ---------------------------\n", applespi_debug_facility(DBG_RD_IRQ)); @@ -1064,7 +1037,10 @@ static u32 applespi_notify(acpi_handle gpe_device, u32 gpe, void *context) applespi_setup_read_txfr(applespi, &applespi->dl_t, &applespi->rd_t); applespi_setup_spi_message(&applespi->rd_m, 2, &applespi->dl_t, &applespi->rd_t); + spin_lock_irqsave(&applespi->cmd_msg_lock, flags); applespi_async(applespi, &applespi->rd_m, applespi_async_read_complete); + spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags); + return ACPI_INTERRUPT_HANDLED; } From 1faadac1ebbba6a4fcd2ca82c087dce68f943c63 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ronald=20Tschal=C3=A4r?= Date: Mon, 28 Aug 2017 00:04:18 -0700 Subject: [PATCH 10/10] applespi: Drain outstading spi commands before removing module. This ensures our callbacks are not called after we've been removed, and it also ensures that writes have been fully processed (including the associated response being received), thereby resolving occasional errors when removing and re-inserting this module. --- applespi.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 58 insertions(+), 3 deletions(-) diff --git a/applespi.c b/applespi.c index b73b45c..baf0e4b 100644 --- a/applespi.c +++ b/applespi.c @@ -30,6 +30,7 @@ #include #include #include +#include #include #include #if defined(DEBUG) || defined(CONFIG_DYNAMIC_DEBUG) @@ -231,6 +232,11 @@ struct applespi_data { unsigned cmd_log_mask; struct led_classdev backlight_info; + + bool drain; + wait_queue_head_t drain_complete; + bool read_active; + bool write_active; }; static const unsigned char applespi_scancodes[] = { @@ -557,6 +563,7 @@ static int applespi_setup_spi(struct applespi_data *applespi) return sts; spin_lock_init(&applespi->cmd_msg_lock); + init_waitqueue_head(&applespi->drain_complete); return 0; } @@ -624,6 +631,10 @@ applespi_send_cmd_msg(struct applespi_data *applespi) u16 crc; int sts; + /* check if draining */ + if (applespi->drain) + return 0; + /* check whether send is in progress */ if (applespi->cmd_msg_queued) return 0; @@ -696,10 +707,13 @@ applespi_send_cmd_msg(struct applespi_data *applespi) sts = applespi_async(applespi, &applespi->wr_m, applespi_async_write_complete); - if (sts != 0) + if (sts != 0) { pr_warn("Error queueing async write to device: %d\n", sts); - else + } else { applespi->cmd_msg_queued = true; + applespi->write_active = true; + } + return sts; } @@ -969,6 +983,7 @@ static void applespi_got_data(struct applespi_data *applespi) { struct keyboard_protocol *keyboard_protocol; + unsigned long flags; keyboard_protocol = (struct keyboard_protocol*) applespi->rx_buffer; if (keyboard_protocol->packet_type == PACKET_TYPE_READ && @@ -1010,6 +1025,19 @@ applespi_got_data(struct applespi_data *applespi) */ udelay(SPI_RW_CHG_DLY); + /* handle draining */ + spin_lock_irqsave(&applespi->cmd_msg_lock, flags); + + applespi->read_active = false; + if (keyboard_protocol->packet_type == PACKET_TYPE_WRITE) + applespi->write_active = false; + + if (applespi->drain && !applespi->write_active) + wake_up_all(&applespi->drain_complete); + + spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags); + + /* notify write complete */ if (keyboard_protocol->packet_type == PACKET_TYPE_WRITE) applespi_cmd_msg_complete(applespi); } @@ -1029,6 +1057,7 @@ static void applespi_async_read_complete(void *context) static u32 applespi_notify(acpi_handle gpe_device, u32 gpe, void *context) { struct applespi_data *applespi = context; + int sts; unsigned long flags; debug_print(DBG_RD_IRQ, "--- %s ---------------------------\n", @@ -1038,7 +1067,13 @@ static u32 applespi_notify(acpi_handle gpe_device, u32 gpe, void *context) applespi_setup_spi_message(&applespi->rd_m, 2, &applespi->dl_t, &applespi->rd_t); spin_lock_irqsave(&applespi->cmd_msg_lock, flags); - applespi_async(applespi, &applespi->rd_m, applespi_async_read_complete); + + sts = applespi_async(applespi, &applespi->rd_m, applespi_async_read_complete); + if (sts != 0) + pr_warn("Error queueing async read to device: %d\n", sts); + else + applespi->read_active = true; + spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags); return ACPI_INTERRUPT_HANDLED; @@ -1236,10 +1271,30 @@ static int applespi_probe(struct spi_device *spi) static int applespi_remove(struct spi_device *spi) { struct applespi_data *applespi = spi_get_drvdata(spi); + unsigned long flags; + + /* wait for all outstanding writes to finish */ + spin_lock_irqsave(&applespi->cmd_msg_lock, flags); + + applespi->drain = true; + wait_event_lock_irq(applespi->drain_complete, !applespi->write_active, + applespi->cmd_msg_lock); + spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags); + + /* shut things down */ acpi_disable_gpe(NULL, applespi->gpe); acpi_remove_gpe_handler(NULL, applespi->gpe, applespi_notify); + /* wait for all outstanding reads to finish */ + spin_lock_irqsave(&applespi->cmd_msg_lock, flags); + + wait_event_lock_irq(applespi->drain_complete, !applespi->read_active, + applespi->cmd_msg_lock); + + spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags); + + /* done */ pr_info("spi-device remove done: %s\n", dev_name(&spi->dev)); return 0; }