On Mon, Feb 04, 2019 at 12:19:47AM -0800, Ronald Tschalär wrote: > The keyboard and trackpad on recent MacBook's (since 8,1) and > MacBookPro's (13,* and 14,*) are attached to an SPI controller instead > of USB, as previously. The higher level protocol is not publicly > documented and hence has been reverse engineered. As a consequence there > are still a number of unknown fields and commands. However, the known > parts have been working well and received extensive testing and use. > > In order for this driver to work, the proper SPI drivers need to be > loaded too; for MB8,1 these are spi_pxa2xx_platform and spi_pxa2xx_pci; > for all others they are spi_pxa2xx_platform and intel_lpss_pci. For this > reason enabling this driver in the config implies enabling the above > drivers. Thanks for doing this. My review below. > +/** I'm not sure it's kernel doc format comment. > + * The keyboard and touchpad controller on the MacBookAir6, MacBookPro12, > + * MacBook8 and newer can be driven either by USB or SPI. However the USB > + * pins are only connected on the MacBookAir6 and 7 and the MacBookPro12. > + * All others need this driver. The interface is selected using ACPI methods: > + * > + * * UIEN ("USB Interface Enable"): If invoked with argument 1, disables SPI > + * and enables USB. If invoked with argument 0, disables USB. > + * * UIST ("USB Interface Status"): Returns 1 if USB is enabled, 0 otherwise. > + * * SIEN ("SPI Interface Enable"): If invoked with argument 1, disables USB > + * and enables SPI. If invoked with argument 0, disables SPI. > + * * SIST ("SPI Interface Status"): Returns 1 if SPI is enabled, 0 otherwise. > + * * ISOL: Resets the four GPIO pins used for SPI. Intended to be invoked with > + * argument 1, then once more with argument 0. > + * > + * UIEN and UIST are only provided on models where the USB pins are connected. > + * > + * SPI-based Protocol > + * ------------------ > + * > + * The device and driver exchange messages (struct message); each message is > + * encapsulated in one or more packets (struct spi_packet). There are two types > + * of exchanges: reads, and writes. A read is signaled by a GPE, upon which one > + * message can be read from the device. A write exchange consists of writing a > + * command message, immediately reading a short status packet, and then, upon > + * receiving a GPE, reading the response message. Write exchanges cannot be > + * interleaved, i.e. a new write exchange must not be started till the previous > + * write exchange is complete. Whether a received message is part of a read or > + * write exchange is indicated in the encapsulating packet's flags field. > + * > + * A single message may be too large to fit in a single packet (which has a > + * fixed, 256-byte size). In that case it will be split over multiple, > + * consecutive packets. > + */ > + > +#define pr_fmt(fmt) "applespi: " fmt Better to use usual pattern. #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > +#include <linux/platform_device.h> > +#include <linux/module.h> > +#include <linux/acpi.h> > +#include <linux/spi/spi.h> > +#include <linux/interrupt.h> > +#include <linux/property.h> > +#include <linux/delay.h> > +#include <linux/spinlock.h> > +#include <linux/crc16.h> > +#include <linux/wait.h> > +#include <linux/leds.h> > +#include <linux/ktime.h> > +#include <linux/input.h> > +#include <linux/input/mt.h> > +#include <linux/input-polldev.h> > +#include <linux/workqueue.h> > +#include <linux/efi.h> Can we keep them sorted? Do you need, btw, all of them? + blank line. > +#include <asm/barrier.h> > +#define MIN_KBD_BL_LEVEL 32 > +#define MAX_KBD_BL_LEVEL 255 I would rather use similar pattern as below, i.e. KBD_..._MIN/_MAX. > +#define KBD_BL_LEVEL_SCALE 1000000 > +#define KBD_BL_LEVEL_ADJ \ > + ((MAX_KBD_BL_LEVEL - MIN_KBD_BL_LEVEL) * KBD_BL_LEVEL_SCALE / 255) > +#define debug_print(mask, fmt, ...) \ > + do { \ > + if (debug & mask) \ > + printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__); \ > + } while (0) > + > +#define debug_print_header(mask) \ > + debug_print(mask, "--- %s ---------------------------\n", \ > + applespi_debug_facility(mask)) > + > +#define debug_print_buffer(mask, fmt, ...) \ > + do { \ > + if (debug & mask) \ > + print_hex_dump(KERN_DEBUG, pr_fmt(fmt), \ > + DUMP_PREFIX_NONE, 32, 1, ##__VA_ARGS__, \ > + false); \ > + } while (0) I'm not sure we need all of these... But okay, the driver is reverse-engineered, perhaps we can drop it later on. > +#define SPI_RW_CHG_DLY 100 /* from experimentation, in us */ In _US would be good to have in a constant name, i.e. SPI_RW_CHG_DELAY_US > +static unsigned int fnmode = 1; > +module_param(fnmode, uint, 0644); > +MODULE_PARM_DESC(fnmode, "Mode of fn key on Apple keyboards (0 = disabled, [1] = fkeyslast, 2 = fkeysfirst)"); fn -> Fn ? Ditto for the rest. Btw, do we need all these parameters? Can't we do modify behaviour run-time using some Input framework facilities? > +static unsigned int iso_layout; > +module_param(iso_layout, uint, 0644); > +MODULE_PARM_DESC(iso_layout, "Enable/Disable hardcoded ISO-layout of the keyboard. ([0] = disabled, 1 = enabled)"); bool ? > +static int touchpad_dimensions[4]; > +module_param_array(touchpad_dimensions, int, NULL, 0444); > +MODULE_PARM_DESC(touchpad_dimensions, "The pixel dimensions of the touchpad, as x_min,x_max,y_min,y_max ."); We have some parsers for similar data as in format %dx%d%+d%+d ? For example, 200x100+20+10. (But still same question, wouldn't input subsystem allows to do this run-time?) > +/** > + * struct keyboard_protocol - keyboard message. > + * message.type = 0x0110, message.length = 0x000a > + * > + * @unknown1: unknown > + * @modifiers: bit-set of modifier/control keys pressed > + * @unknown2: unknown > + * @keys_pressed: the (non-modifier) keys currently pressed > + * @fn_pressed: whether the fn key is currently pressed > + * @crc_16: crc over the whole message struct (message header + > + * this struct) minus this @crc_16 field Something wrong with indentation. Check it over all your kernel doc comments. > + */ > +struct touchpad_info_protocol { > + __u8 unknown1[105]; > + __le16 model_id; > + __u8 unknown2[3]; > + __le16 crc_16; > +} __packed; 112 % 16 == 0, why do you need __packed? > + __le16 crc_16; Can't you use crc16 everywhere for the name? > +struct applespi_tp_info { > + int x_min; > + int x_max; > + int y_min; > + int y_max; > +}; Perhaps use the same format as in struct drm_rect in order to possibility of unifying them in the future? > + { }, Terminators are better without comma. > + switch (log_mask) { > + case DBG_CMD_TP_INI: > + return "Touchpad Initialization"; > + case DBG_CMD_BL: > + return "Backlight Command"; > + case DBG_CMD_CL: > + return "Caps-Lock Command"; > + case DBG_RD_KEYB: > + return "Keyboard Event"; > + case DBG_RD_TPAD: > + return "Touchpad Event"; > + case DBG_RD_UNKN: > + return "Unknown Event"; > + case DBG_RD_IRQ: > + return "Interrupt Request"; > + case DBG_RD_CRC: > + return "Corrupted packet"; > + case DBG_TP_DIM: > + return "Touchpad Dimensions"; > + default: > + return "-Unknown-"; What the difference to RD_UNKN ? Perhaps "Unrecognized ... "-ish better? > + } > +static inline bool applespi_check_write_status(struct applespi_data *applespi, > + int sts) Indentation broken. > +{ > + static u8 sts_ok[] = { 0xac, 0x27, 0x68, 0xd5 }; Spell it fully, i.e. status_ok. > + bool ret = true; Directly return from each branch, it also will make 'else' redundant. > + > + 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) { Redundant ' != 0' part. After removing this and 'else' would be fit on one line. > + 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]); pr_warn("...: %ph\n", applespi->tx_status); > + } > + > + return ret; > +} > +static int applespi_enable_spi(struct applespi_data *applespi) > +{ > + int result; Sometimes you have ret, sometimes this. Better to unify across the code. > + unsigned long long spi_status; > + return 0; > +} > +static void applespi_async_write_complete(void *context) > +{ > + struct applespi_data *applespi = context; > + if (!applespi_check_write_status(applespi, applespi->wr_m.status)) > + /* > + * If we got an error, we presumably won't get the expected > + * response message either. > + */ > + applespi_msg_complete(applespi, true, false); Style issue, either use {} or positive condition like if (...) return; ... > +} > +static int applespi_send_cmd_msg(struct applespi_data *applespi) > +{ > + if (applespi->want_tp_info_cmd) { > + } else if (applespi->want_mt_init_cmd) { > + } else if (applespi->want_cl_led_on != applespi->have_cl_led_on) { > + } else if (applespi->want_bl_level != applespi->have_bl_level) { > + } else { > + return 0; > + } Can we refactor to use some kind of enumeration and do switch-case here? > + message->counter = applespi->cmd_msg_cntr++ & 0xff; Usual pattern for this kind of entries is x = ... % 256; Btw, isn't 256 is defined somewhere? > + crc = crc16(0, (u8 *)message, le16_to_cpu(packet->length) - 2); > + *((__le16 *)&message->data[msg_len - 2]) = cpu_to_le16(crc); put_unaligned_le16() ? > + if (sts != 0) { > + pr_warn("Error queueing async write to device: %d\n", sts); > + } else { > + applespi->cmd_msg_queued = true; > + applespi->write_active = true; > + } Usual pattern is if (sts) { ... return sts; } ... return 0; Btw, consider to use dev_warn() and Co instead of pr_warn() or in cases where appropriate acpi_handle_warn(), etc. > + > + return sts; > +} > +static void applespi_init(struct applespi_data *applespi, bool is_resume) > +{ > + unsigned long flags; > + > + spin_lock_irqsave(&applespi->cmd_msg_lock, flags); > + > + if (!is_resume) > + applespi->want_tp_info_cmd = true; > + else > + applespi->want_mt_init_cmd = true; Why not positive conditional? > + applespi_send_cmd_msg(applespi); > + > + spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags); > +} > +static void applespi_set_bl_level(struct led_classdev *led_cdev, > + enum led_brightness value) > +{ > + struct applespi_data *applespi = > + container_of(led_cdev, struct applespi_data, backlight_info); > + unsigned long flags; > + int sts; > + > + spin_lock_irqsave(&applespi->cmd_msg_lock, flags); > + > + if (value == 0) > + applespi->want_bl_level = value; > + else > + /* > + * The backlight does not turn on till level 32, so we scale > + * the range here so that from a user's perspective it turns > + * on at 1. > + */ > + applespi->want_bl_level = (unsigned int) > + ((value * KBD_BL_LEVEL_ADJ) / KBD_BL_LEVEL_SCALE + > + MIN_KBD_BL_LEVEL); Why do you need casting? Your defines better to have U or UL suffixes where appropriate. Besides, run checkpatch.pl! > + > + sts = applespi_send_cmd_msg(applespi); > + > + spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags); > +} > +static int applespi_event(struct input_dev *dev, unsigned int type, > + unsigned int code, int value) > +{ > + struct applespi_data *applespi = input_get_drvdata(dev); > + > + switch (type) { > + case EV_LED: > + applespi_set_capsl_led(applespi, > + !!test_bit(LED_CAPSL, dev->led)); I would put it on one line disregard checkpatch warnings. > + return 0; > + } > + > + return -1; Can't you use appropriate Linux error code? > +} > +/* lifted from the BCM5974 driver */ > +/* convert 16-bit little endian to signed integer */ > +static inline int raw2int(__le16 x) > +{ > + return (signed short)le16_to_cpu(x); > +} Perhaps it's time to introduce it inside include/linux/input.h ? > +static void report_tp_state(struct applespi_data *applespi, > + struct touchpad_protocol *t) > +{ > + static int min_x, max_x, min_y, max_y; > + static bool dim_updated; > + static ktime_t last_print; > + Redundant. > + const struct tp_finger *f; > + struct input_dev *input; > + const struct applespi_tp_info *tp_info = &applespi->tp_info; > + int i, n; > + > + /* touchpad_input_dev is set async in worker */ > + input = smp_load_acquire(&applespi->touchpad_input_dev); > + if (!input) > + return; /* touchpad isn't initialized yet */ > + > + n = 0; > + > + for (i = 0; i < t->number_of_fingers; i++) { for (n = 0, i = 0; i < ...; i++, n++) { ? > + f = &t->fingers[i]; > + if (raw2int(f->touch_major) == 0) > + continue; > + applespi->pos[n].x = raw2int(f->abs_x); > + applespi->pos[n].y = tp_info->y_min + tp_info->y_max - > + raw2int(f->abs_y); > + n++; > + > + if (debug & DBG_TP_DIM) { > + #define UPDATE_DIMENSIONS(val, op, last) \ > + do { \ > + if (raw2int(val) op last) { \ > + last = raw2int(val); \ > + dim_updated = true; \ > + } \ > + } while (0) > + > + UPDATE_DIMENSIONS(f->abs_x, <, min_x); > + UPDATE_DIMENSIONS(f->abs_x, >, max_x); > + UPDATE_DIMENSIONS(f->abs_y, <, min_y); > + UPDATE_DIMENSIONS(f->abs_y, >, max_y); #undef ... > + } ...and better to move this to separate helper function. > + } > + > + if (debug & DBG_TP_DIM) { > + if (dim_updated && > + ktime_ms_delta(ktime_get(), last_print) > 1000) { > + printk(KERN_DEBUG > + pr_fmt("New touchpad dimensions: %d %d %d %d\n"), > + min_x, max_x, min_y, max_y); > + dim_updated = false; > + last_print = ktime_get(); > + } > + } Same, helper function. > + > + input_mt_assign_slots(input, applespi->slots, applespi->pos, n, 0); > + > + for (i = 0; i < n; i++) > + report_finger_data(input, applespi->slots[i], > + &applespi->pos[i], &t->fingers[i]); > + > + input_mt_sync_frame(input); > + input_report_key(input, BTN_LEFT, t->clicked); > + > + input_sync(input); > +} > + > +static unsigned int applespi_code_to_key(u8 code, int fn_pressed) > +{ > + unsigned int key = applespi_scancodes[code]; > + const struct applespi_key_translation *trans; > + > + if (fnmode) { > + int do_translate; > + > + trans = applespi_find_translation(applespi_fn_codes, key); > + if (trans) { > + if (trans->flags & APPLE_FLAG_FKEY) > + do_translate = (fnmode == 2 && fn_pressed) || > + (fnmode == 1 && !fn_pressed); > + else > + do_translate = fn_pressed; > + > + if (do_translate) > + key = trans->to; > + } > + } > + > + if (iso_layout) { > + trans = applespi_find_translation(apple_iso_keyboard, key); > + if (trans) > + key = trans->to; > + } I would split this to three helpers like static unsigned int ..._code_to_fn_key() { } static unsigned int ..._code_to_iso_key() { } static unsigned int ..._code_to_key() { if (fnmode) key = ..._code_to_fn_key(); if (iso_layout) key = ..._code_to_iso_key(); return key; } > + > + return key; > +} > +static void applespi_remap_fn_key(struct keyboard_protocol > + *keyboard_protocol) Better to split like static void fn(struct ...); > +{ > + unsigned char tmp; > + unsigned long *modifiers = (unsigned long *) > + &keyboard_protocol->modifiers; Don't split casting from the rest, better ... var = (type)...; > + > + if (!fnremap || fnremap > ARRAY_SIZE(applespi_controlcodes) || > + !applespi_controlcodes[fnremap - 1]) > + return; > + > + tmp = keyboard_protocol->fn_pressed; > + keyboard_protocol->fn_pressed = test_bit(fnremap - 1, modifiers); > + if (tmp) > + __set_bit(fnremap - 1, modifiers); > + else > + __clear_bit(fnremap - 1, modifiers); > +} > + > +static void applespi_handle_keyboard_event(struct applespi_data *applespi, > + struct keyboard_protocol > + *keyboard_protocol) Put this to one line, consider reformat like I mentioned above. > +{ > + 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; > + } This could come as if (...) continue; ... > + } > + memcpy(&applespi->last_keys_pressed, keyboard_protocol->keys_pressed, > + sizeof(applespi->last_keys_pressed)); applespi->last_keys_pressed = *keyboard_protocol->keys_pressed; (if they are of the same type) ? > +} > +static void applespi_register_touchpad_device(struct applespi_data *applespi, > + struct touchpad_info_protocol *rcvd_tp_info) > +{ > + /* create touchpad input device */ > + touchpad_input_dev = devm_input_allocate_device(&applespi->spi->dev); > + Redundant. > + if (!touchpad_input_dev) { > + pr_err("Failed to allocate touchpad input device\n"); dev_err(); > + return; Shouldn't we return an error to a caller? > + } > + /* register input device */ > + res = input_register_device(touchpad_input_dev); > + if (res) > + pr_err("Unabled to register touchpad input device (%d)\n", res); > + else if (ret) { dev_err(...); return ret; } Btw, ret, res, sts, result, ... Choose one. > + /* touchpad_input_dev is read async in spi callback */ > + smp_store_release(&applespi->touchpad_input_dev, > + touchpad_input_dev); > +} > +static bool applespi_verify_crc(struct applespi_data *applespi, u8 *buffer, > + size_t buflen) > +{ > + u16 crc; > + > + crc = crc16(0, buffer, buflen); > + if (crc != 0) { if (crc) { > + dev_warn_ratelimited(&applespi->spi->dev, > + "Received corrupted packet (crc mismatch)\n"); > + debug_print_header(DBG_RD_CRC); > + debug_print_buffer(DBG_RD_CRC, "read ", buffer, buflen); > + > + return false; > + } > + > + return true; > +} > +static void applespi_got_data(struct applespi_data *applespi) > +{ > + } else if (packet->flags == PACKET_TYPE_READ && > + packet->device == PACKET_DEV_TPAD) { > + struct touchpad_protocol *tp = &message->touchpad; > + > + size_t tp_len = sizeof(*tp) + > + tp->number_of_fingers * sizeof(tp->fingers[0]); Would be better struct ...; size_t ...; ... = ...; if (...) { > + if (le16_to_cpu(message->length) + 2 != tp_len) { > + dev_warn_ratelimited(&applespi->spi->dev, > + "Received corrupted packet (invalid message length)\n"); > + goto cleanup; > + } > + } else if (packet->flags == PACKET_TYPE_WRITE) { > + applespi_handle_cmd_response(applespi, packet, message); > + } > + > +cleanup: err_msg_complete: ? > + /* clean up */ Noise. > + applespi->saved_msg_len = 0; > + > + applespi_msg_complete(applespi, packet->flags == PACKET_TYPE_WRITE, > + true); > +} > +static u32 applespi_notify(acpi_handle gpe_device, u32 gpe, void *context) > +{ > + struct applespi_data *applespi = context; > + int sts; > + unsigned long flags; > + > + debug_print_header(DBG_RD_IRQ); > + > + spin_lock_irqsave(&applespi->cmd_msg_lock, flags); > + > + if (!applespi->suspended) { > + sts = applespi_async(applespi, &applespi->rd_m, > + applespi_async_read_complete); > + if (sts != 0) if (sts) > + 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; > +} > + > +static int applespi_get_saved_bl_level(void) > +{ > + struct efivar_entry *efivar_entry; > + u16 efi_data = 0; > + unsigned long efi_data_len; > + int sts; > + > + efivar_entry = kmalloc(sizeof(*efivar_entry), GFP_KERNEL); > + if (!efivar_entry) > + return -1; -ENOMEM > + > + memcpy(efivar_entry->var.VariableName, EFI_BL_LEVEL_NAME, > + sizeof(EFI_BL_LEVEL_NAME)); > + efivar_entry->var.VendorGuid = EFI_BL_LEVEL_GUID; > + efi_data_len = sizeof(efi_data); > + > + sts = efivar_entry_get(efivar_entry, NULL, &efi_data_len, &efi_data); > + if (sts && sts != -ENOENT) > + pr_warn("Error getting backlight level from EFI vars: %d\n", > + sts); > + > + kfree(efivar_entry); > + > + return efi_data; > +} > +static int applespi_probe(struct spi_device *spi) > +{ > + applespi->msg_buf = devm_kmalloc(&spi->dev, MAX_PKTS_PER_MSG * > + APPLESPI_PACKET_SIZE, > + GFP_KERNEL); devm_kmalloc_array(); > + > + if (!applespi->tx_buffer || !applespi->tx_status || > + !applespi->rx_buffer || !applespi->msg_buf) > + return -ENOMEM; > + /* > + * By default this device is not enable for wakeup; but USB keyboards > + * generally are, so the expectation is that by default the keyboard > + * will wake the system. > + */ > + device_wakeup_enable(&spi->dev); > + result = devm_led_classdev_register(&spi->dev, > + &applespi->backlight_info); > + if (result) { > + pr_err("Unable to register keyboard backlight class dev (%d)\n", > + result); > + /* not fatal */ Noise. Instead use dev_warn(). > + } > + /* done */ > + pr_info("spi-device probe done: %s\n", dev_name(&spi->dev)); Noise. One may use "initcall_debug". > + return 0; > +} > + > +static int applespi_remove(struct spi_device *spi) > +{ > + struct applespi_data *applespi = spi_get_drvdata(spi); > + unsigned long flags; > + /* shut things down */ Noise. > + acpi_disable_gpe(NULL, applespi->gpe); > + acpi_remove_gpe_handler(NULL, applespi->gpe, applespi_notify); > + > + /* done */ > + pr_info("spi-device remove done: %s\n", dev_name(&spi->dev)); Noise. It seems you still have wakeup enabled for the device. > + return 0; > +} > +static int applespi_suspend(struct device *dev) > +{ > + int rc; Is it 6th type of naming for error code holding variable? > + /* 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); Helper? It's used in ->remove() AFAICS. > + pr_info("spi-device suspend done.\n"); Noise. One may use ftrace instead. > + return 0; > +} > + > +static int applespi_resume(struct device *dev) > +{ > + pr_info("spi-device resume done.\n"); Ditto. > + > + return 0; > +} > +static const struct acpi_device_id applespi_acpi_match[] = { > + { "APP000D", 0 }, > + { }, No comma, please. > +}; > +MODULE_DEVICE_TABLE(acpi, applespi_acpi_match); > +static struct spi_driver applespi_driver = { > + .driver = { > + .name = "applespi", > + .owner = THIS_MODULE, This set by macro. > + Redundant. > + .acpi_match_table = ACPI_PTR(applespi_acpi_match), Do you need ACPI_PTR() ? > + .pm = &applespi_pm_ops, > + }, > + .probe = applespi_probe, > + .remove = applespi_remove, > + .shutdown = applespi_shutdown, > +}; > + > +module_spi_driver(applespi_driver) > +MODULE_LICENSE("GPL"); License mismatch. -- With Best Regards, Andy Shevchenko