[PATCH 0/2] Add Apple SPI keyboard and trackpad driver

[PATCH 0/2] Add Apple SPI keyboard and trackpad driver

[Ronald Tschalär]

This changeset adds a driver for the SPI keyboard and trackpad on recent
MacBook's and MacBook Pro's. The driver has seen a fair amount of use
over the last 2 years (basically anybody running linux on these
machines), with only relatively small changes in the last year or so.
For those interested, the driver development has been hosted at
https://github.com/cb22/macbook12-spi-driver/ (as well as my clone at
https://github.com/roadrunner2/macbook12-spi-driver/).

The first patch is just a placeholder for now and is provided in case
somebody wants to compile the driver while it's being reviewed here; the
real patch has been submitted to dri-devel and is being discussed there,
with the intent/hope that I can get an Ack and permission to merge it
through the input subsystem tree here as part of this patch series.

Ronald Tschalär (2):
  drm/bridge: sil_sii8620: depend on INPUT instead of selecting it.
  Input: add Apple SPI keyboard and trackpad driver.

 drivers/gpu/drm/bridge/Kconfig    |    2 +-
 drivers/input/keyboard/Kconfig    |   13 +
 drivers/input/keyboard/Makefile   |    1 +
 drivers/input/keyboard/applespi.c | 1919 +++++++++++++++++++++++++++++
 4 files changed, 1934 insertions(+), 1 deletion(-)
 create mode 100644 drivers/input/keyboard/applespi.c

-- 
2.20.1

[PATCH 1/2] drm/bridge: sil_sii8620: depend on INPUT instead of selecting it.

[Ronald Tschalär]

commit d6abe6df706c66d803e6dd4fe98c1b6b7f125a56 (drm/bridge:
sil_sii8620: do not have a dependency of RC_CORE) added a dependency on
INPUT. However, this causes problems with other drivers, in particular
an input driver that depends on MFD_INTEL_LPSS_PCI (to be added in a
future commit):

  drivers/clk/Kconfig:9:error: recursive dependency detected!
  drivers/clk/Kconfig:9:        symbol COMMON_CLK is selected by MFD_INTEL_LPSS
  drivers/mfd/Kconfig:566:      symbol MFD_INTEL_LPSS is selected by MFD_INTEL_LPSS_PCI
  drivers/mfd/Kconfig:580:      symbol MFD_INTEL_LPSS_PCI is implied by KEYBOARD_APPLESPI
  drivers/input/keyboard/Kconfig:73:    symbol KEYBOARD_APPLESPI depends on INPUT
  drivers/input/Kconfig:8:      symbol INPUT is selected by DRM_SIL_SII8620
  drivers/gpu/drm/bridge/Kconfig:83:    symbol DRM_SIL_SII8620 depends on DRM_BRIDGE
  drivers/gpu/drm/bridge/Kconfig:1:     symbol DRM_BRIDGE is selected by DRM_PL111
  drivers/gpu/drm/pl111/Kconfig:1:      symbol DRM_PL111 depends on COMMON_CLK

According to the docs, select should only be used for non-visible
symbols. Furthermore almost all other references to INPUT throughout the
kernel config are depends, not selects. Hence this change.

CC: Inki Dae <inki.dae@samsung.com>
CC: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
---
 drivers/gpu/drm/bridge/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 2fee47b0d50b..eabedc83f25c 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -83,9 +83,9 @@ config DRM_PARADE_PS8622
 config DRM_SIL_SII8620
 	tristate "Silicon Image SII8620 HDMI/MHL bridge"
 	depends on OF
+	depends on INPUT
 	select DRM_KMS_HELPER
 	imply EXTCON
-	select INPUT
 	select RC_CORE
 	help
 	  Silicon Image SII8620 HDMI/MHL bridge chip driver.
-- 
2.20.1

[PATCH 2/2] Input: add Apple SPI keyboard and trackpad driver.

[Ronald Tschalär]

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.

CC: Federico Lorenzi <federico@travelground.com>
CC: Lukas Wunner <lukas@wunner.de>
CC: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=99891
Link: https://bugzilla.kernel.org/show_bug.cgi?id=108331
Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
---
 drivers/input/keyboard/Kconfig    |   13 +
 drivers/input/keyboard/Makefile   |    1 +
 drivers/input/keyboard/applespi.c | 1919 +++++++++++++++++++++++++++++
 3 files changed, 1933 insertions(+)
 create mode 100644 drivers/input/keyboard/applespi.c

diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index a878351f1643..e35afa23b1db 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -70,6 +70,19 @@ config KEYBOARD_AMIGA
 config ATARI_KBD_CORE
 	bool
 
+config KEYBOARD_APPLESPI
+	tristate "Apple SPI keyboard and trackpad"
+	depends on (X86 && ACPI && SPI) || COMPILE_TEST
+	imply SPI_PXA2XX
+	imply SPI_PXA2XX_PCI
+	imply MFD_INTEL_LPSS_PCI
+	help
+	  Say Y here if you are running Linux on any Apple MacBook8,1 or later,
+	  or any MacBookPro13,* or MacBookPro14,*.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called applespi.
+
 config KEYBOARD_ATARI
 	tristate "Atari keyboard"
 	depends on ATARI
diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
index 182e92985dbf..9283fee2505a 100644
--- a/drivers/input/keyboard/Makefile
+++ b/drivers/input/keyboard/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_KEYBOARD_ADP5520)		+= adp5520-keys.o
 obj-$(CONFIG_KEYBOARD_ADP5588)		+= adp5588-keys.o
 obj-$(CONFIG_KEYBOARD_ADP5589)		+= adp5589-keys.o
 obj-$(CONFIG_KEYBOARD_AMIGA)		+= amikbd.o
+obj-$(CONFIG_KEYBOARD_APPLESPI)		+= applespi.o
 obj-$(CONFIG_KEYBOARD_ATARI)		+= atakbd.o
 obj-$(CONFIG_KEYBOARD_ATKBD)		+= atkbd.o
 obj-$(CONFIG_KEYBOARD_BCM)		+= bcm-keypad.o
diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
new file mode 100644
index 000000000000..75780f3385c0
--- /dev/null
+++ b/drivers/input/keyboard/applespi.c
@@ -0,0 +1,1919 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * MacBook (Pro) SPI keyboard and touchpad driver
+ *
+ * Copyright (c) 2015-2018 Federico Lorenzi
+ * Copyright (c) 2017-2018 Ronald Tschalär
+ */
+
+/**
+ * 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
+
+#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>
+#include <asm/barrier.h>
+
+#define APPLESPI_PACKET_SIZE	256
+#define APPLESPI_STATUS_SIZE	4
+
+#define PACKET_TYPE_READ	0x20
+#define PACKET_TYPE_WRITE	0x40
+#define PACKET_DEV_KEYB		0x01
+#define PACKET_DEV_TPAD		0x02
+#define PACKET_DEV_INFO		0xd0
+
+#define MAX_ROLLOVER		6
+#define MAX_MODIFIERS		8
+
+#define MAX_FINGERS		11
+#define MAX_FINGER_ORIENTATION	16384
+#define MAX_PKTS_PER_MSG	2
+
+#define MIN_KBD_BL_LEVEL	32
+#define MAX_KBD_BL_LEVEL	255
+#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 EFI_BL_LEVEL_NAME	L"KeyboardBacklightLevel"
+#define EFI_BL_LEVEL_GUID	EFI_GUID(0xa076d2af, 0x9678, 0x4386, 0x8b, 0x58, 0x1f, 0xc8, 0xef, 0x04, 0x16, 0x19)
+
+#define DBG_CMD_TP_INI		BIT(0)
+#define DBG_CMD_BL		BIT(1)
+#define DBG_CMD_CL		BIT(2)
+#define DBG_RD_KEYB		BIT(8)
+#define DBG_RD_TPAD		BIT(9)
+#define DBG_RD_UNKN		BIT(10)
+#define DBG_RD_IRQ		BIT(11)
+#define DBG_RD_CRC		BIT(12)
+#define DBG_TP_DIM		BIT(16)
+
+#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)
+
+#define APPLE_FLAG_FKEY		0x01
+
+#define SPI_RW_CHG_DLY		100	/* from experimentation, in us */
+
+#define SYNAPTICS_VENDOR_ID	0x06cb
+
+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)");
+
+static unsigned int fnremap;
+module_param(fnremap, uint, 0644);
+MODULE_PARM_DESC(fnremap, "Remap fn key ([0] = no-remap; 1 = left-ctrl, 2 = left-shift, 3 = left-alt, 4 = left-meta, 6 = right-shift, 7 = right-alt, 8 = right-meta)");
+
+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)");
+
+static unsigned int debug;
+module_param(debug, uint, 0644);
+MODULE_PARM_DESC(debug, "Enable/Disable debug logging. This is a bitmask.");
+
+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 .");
+
+/**
+ * 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
+ */
+struct keyboard_protocol {
+	__u8			unknown1;
+	__u8			modifiers;
+	__u8			unknown2;
+	__u8			keys_pressed[MAX_ROLLOVER];
+	__u8			fn_pressed;
+	__le16			crc_16;
+};
+
+/**
+ * struct tp_finger - single trackpad finger structure, le16-aligned
+ *
+ * @origin:		zero when switching track finger
+ * @abs_x:		absolute x coodinate
+ * @abs_y:		absolute y coodinate
+ * @rel_x:		relative x coodinate
+ * @rel_y:		relative y coodinate
+ * @tool_major:		tool area, major axis
+ * @tool_minor:		tool area, minor axis
+ * @orientation:	16384 when point, else 15 bit angle
+ * @touch_major:	touch area, major axis
+ * @touch_minor:	touch area, minor axis
+ * @unused:		zeros
+ * @pressure:		pressure on forcetouch touchpad
+ * @multi:		one finger: varies, more fingers: constant
+ * @crc_16:		on last finger: crc over the whole message struct
+ *			(i.e. message header + this struct) minus the last
+ *			@crc_16 field; unknown on all other fingers.
+ */
+struct tp_finger {
+	__le16 origin;
+	__le16 abs_x;
+	__le16 abs_y;
+	__le16 rel_x;
+	__le16 rel_y;
+	__le16 tool_major;
+	__le16 tool_minor;
+	__le16 orientation;
+	__le16 touch_major;
+	__le16 touch_minor;
+	__le16 unused[2];
+	__le16 pressure;
+	__le16 multi;
+	__le16 crc_16;
+};
+
+/**
+ * struct touchpad_protocol - touchpad message.
+ * message.type = 0x0210
+ *
+ * @unknown1:		unknown
+ * @clicked:		1 if a button-click was detected, 0 otherwise
+ * @unknown2:		unknown
+ * @number_of_fingers:	the number of fingers being reported in @fingers
+ * @clicked2:		same as @clicked
+ * @unknown3:		unknown
+ * @fingers:		the data for each finger
+ */
+struct touchpad_protocol {
+	__u8			unknown1[1];
+	__u8			clicked;
+	__u8			unknown2[28];
+	__u8			number_of_fingers;
+	__u8			clicked2;
+	__u8			unknown3[16];
+	struct tp_finger	fingers[0];
+};
+
+/**
+ * struct command_protocol_tp_info - get touchpad info.
+ * message.type = 0x1020, message.length = 0x0000
+ *
+ * @crc_16:		crc over the whole message struct (message header +
+ *			this struct) minus this @crc_16 field
+ */
+struct command_protocol_tp_info {
+	__le16			crc_16;
+};
+
+/**
+ * struct touchpad_info - touchpad info response.
+ * message.type = 0x1020, message.length = 0x006e
+ *
+ * @unknown1:		unknown
+ * @model_id:		the touchpad model number
+ * @unknown2:		unknown
+ * @crc_16:		crc over the whole message struct (message header +
+ *			this struct) minus this @crc_16 field
+ */
+struct touchpad_info_protocol {
+	__u8			unknown1[105];
+	__le16			model_id;
+	__u8			unknown2[3];
+	__le16			crc_16;
+} __packed;
+
+/**
+ * struct command_protocol_mt_init - initialize multitouch.
+ * message.type = 0x0252, message.length = 0x0002
+ *
+ * @cmd:		value: 0x0102
+ * @crc_16:		crc over the whole message struct (message header +
+ *			this struct) minus this @crc_16 field
+ */
+struct command_protocol_mt_init {
+	__le16			cmd;
+	__le16			crc_16;
+};
+
+/**
+ * struct command_protocol_capsl - toggle caps-lock led
+ * message.type = 0x0151, message.length = 0x0002
+ *
+ * @unknown:		value: 0x01 (length?)
+ * @led:		0 off, 2 on
+ * @crc_16:		crc over the whole message struct (message header +
+ *			this struct) minus this @crc_16 field
+ */
+struct command_protocol_capsl {
+	__u8			unknown;
+	__u8			led;
+	__le16			crc_16;
+};
+
+/**
+ * struct command_protocol_bl - set keyboard backlight brightness
+ * message.type = 0xB051, message.length = 0x0006
+ *
+ * @const1:		value: 0x01B0
+ * @level:		the brightness level to set
+ * @const2:		value: 0x0001 (backlight off), 0x01F4 (backlight on)
+ * @crc_16:		crc over the whole message struct (message header +
+ *			this struct) minus this @crc_16 field
+ */
+struct command_protocol_bl {
+	__le16			const1;
+	__le16			level;
+	__le16			const2;
+	__le16			crc_16;
+};
+
+/**
+ * struct message - a complete spi message.
+ *
+ * Each message begins with fixed header, followed by a message-type specific
+ * payload, and ends with a 16-bit crc. Because of the varying lengths of the
+ * payload, the crc is defined at the end of each payload struct, rather than
+ * in this struct.
+ *
+ * @type:	the message type
+ * @zero:	always 0
+ * @counter:	incremented on each message, rolls over after 255; there is a
+ *		separate counter for each message type.
+ * @rsp_buf_len:response buffer length (the exact nature of this field is quite
+ *		speculative). On a request/write this is often the same as
+ *		@length, though in some cases it has been seen to be much larger
+ *		(e.g. 0x400); on a response/read this the same as on the
+ *		request; for reads that are not responses it is 0.
+ * @length:	length of the remainder of the data in the whole message
+ *		structure (after re-assembly in case of being split over
+ *		multiple spi-packets), minus the trailing crc. The total size
+ *		of the message struct is therefore @length + 10.
+ */
+struct message {
+	__le16		type;
+	__u8		zero;
+	__u8		counter;
+	__le16		rsp_buf_len;
+	__le16		length;
+	union {
+		struct keyboard_protocol	keyboard;
+		struct touchpad_protocol	touchpad;
+		struct touchpad_info_protocol	tp_info;
+		struct command_protocol_tp_info	tp_info_command;
+		struct command_protocol_mt_init	init_mt_command;
+		struct command_protocol_capsl	capsl_command;
+		struct command_protocol_bl	bl_command;
+		__u8				data[0];
+	};
+};
+
+/* type + zero + counter + rsp_buf_len + length */
+#define MSG_HEADER_SIZE	8
+
+/**
+ * struct spi_packet - a complete spi packet; always 256 bytes. This carries
+ * the (parts of the) message in the data. But note that this does not
+ * necessarily contain a complete message, as in some cases (e.g. many
+ * fingers pressed) the message is split over multiple packets (see the
+ * @offset, @remaining, and @length fields). In general the data parts in
+ * spi_packet's are concatenated until @remaining is 0, and the result is an
+ * message.
+ *
+ * @flags:	0x40 = write (to device), 0x20 = read (from device); note that
+ *		the response to a write still has 0x40.
+ * @device:	1 = keyboard, 2 = touchpad
+ * @offset:	specifies the offset of this packet's data in the complete
+ *		message; i.e. > 0 indicates this is a continuation packet (in
+ *		the second packet for a message split over multiple packets
+ *		this would then be the same as the @length in the first packet)
+ * @remaining:	number of message bytes remaining in subsequents packets (in
+ *		the first packet of a message split over two packets this would
+ *		then be the same as the @length in the second packet)
+ * @length:	length of the valid data in the @data in this packet
+ * @data:	all or part of a message
+ * @crc_16:	crc over this whole structure minus this @crc_16 field. This
+ *		covers just this packet, even on multi-packet messages (in
+ *		contrast to the crc in the message).
+ */
+struct spi_packet {
+	__u8			flags;
+	__u8			device;
+	__le16			offset;
+	__le16			remaining;
+	__le16			length;
+	__u8			data[246];
+	__le16			crc_16;
+};
+
+struct spi_settings {
+	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) */
+};
+
+struct applespi_tp_info {
+	int	x_min;
+	int	x_max;
+	int	y_min;
+	int	y_max;
+};
+
+struct applespi_data {
+	struct spi_device		*spi;
+	struct spi_settings		spi_settings;
+	struct input_dev		*keyboard_input_dev;
+	struct input_dev		*touchpad_input_dev;
+
+	u8				*tx_buffer;
+	u8				*tx_status;
+	u8				*rx_buffer;
+
+	u8				*msg_buf;
+	unsigned int			saved_msg_len;
+
+	struct applespi_tp_info		tp_info;
+
+	u8				last_keys_pressed[MAX_ROLLOVER];
+	u8				last_keys_fn_pressed[MAX_ROLLOVER];
+	u8				last_fn_pressed;
+	struct input_mt_pos		pos[MAX_FINGERS];
+	int				slots[MAX_FINGERS];
+	acpi_handle			handle;
+	int				gpe;
+	acpi_handle			sien;
+	acpi_handle			sist;
+
+	struct spi_transfer		dl_t;
+	struct spi_transfer		rd_t;
+	struct spi_message		rd_m;
+
+	struct spi_transfer		ww_t;
+	struct spi_transfer		wd_t;
+	struct spi_transfer		wr_t;
+	struct spi_transfer		st_t;
+	struct spi_message		wr_m;
+
+	bool				want_tp_info_cmd;
+	bool				want_mt_init_cmd;
+	bool				want_cl_led_on;
+	bool				have_cl_led_on;
+	unsigned int			want_bl_level;
+	unsigned int			have_bl_level;
+	unsigned int			cmd_msg_cntr;
+	/* lock to protect the above parameters and flags below */
+	spinlock_t			cmd_msg_lock;
+	bool				cmd_msg_queued;
+	unsigned int			cmd_log_mask;
+
+	struct led_classdev		backlight_info;
+
+	bool				suspended;
+	bool				drain;
+	wait_queue_head_t		drain_complete;
+	bool				read_active;
+	bool				write_active;
+
+	struct work_struct		work;
+	struct touchpad_info_protocol	rcvd_tp_info;
+};
+
+static const unsigned char applespi_scancodes[] = {
+	0, 0, 0, 0,
+	KEY_A, KEY_B, KEY_C, KEY_D, KEY_E, KEY_F, KEY_G, KEY_H, KEY_I, KEY_J,
+	KEY_K, KEY_L, KEY_M, KEY_N, KEY_O, KEY_P, KEY_Q, KEY_R, KEY_S, KEY_T,
+	KEY_U, KEY_V, KEY_W, KEY_X, KEY_Y, KEY_Z,
+	KEY_1, KEY_2, KEY_3, KEY_4, KEY_5, KEY_6, KEY_7, KEY_8, KEY_9, KEY_0,
+	KEY_ENTER, KEY_ESC, KEY_BACKSPACE, KEY_TAB, KEY_SPACE, KEY_MINUS,
+	KEY_EQUAL, KEY_LEFTBRACE, KEY_RIGHTBRACE, KEY_BACKSLASH, 0,
+	KEY_SEMICOLON, KEY_APOSTROPHE, KEY_GRAVE, KEY_COMMA, KEY_DOT, KEY_SLASH,
+	KEY_CAPSLOCK,
+	KEY_F1, KEY_F2, KEY_F3, KEY_F4, KEY_F5, KEY_F6, KEY_F7, KEY_F8, KEY_F9,
+	KEY_F10, KEY_F11, KEY_F12, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+	KEY_RIGHT, KEY_LEFT, KEY_DOWN, KEY_UP,
+	0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, KEY_102ND,
+	0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+	0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, KEY_RO, 0, KEY_YEN, 0, 0, 0, 0, 0,
+	0, KEY_KATAKANAHIRAGANA, KEY_MUHENKAN
+};
+
+static const unsigned char applespi_controlcodes[] = {
+	KEY_LEFTCTRL,
+	KEY_LEFTSHIFT,
+	KEY_LEFTALT,
+	KEY_LEFTMETA,
+	0,
+	KEY_RIGHTSHIFT,
+	KEY_RIGHTALT,
+	KEY_RIGHTMETA
+};
+
+struct applespi_key_translation {
+	u16 from;
+	u16 to;
+	u8 flags;
+};
+
+static const struct applespi_key_translation applespi_fn_codes[] = {
+	{ KEY_BACKSPACE, KEY_DELETE },
+	{ KEY_ENTER,	KEY_INSERT },
+	{ KEY_F1,	KEY_BRIGHTNESSDOWN,	APPLE_FLAG_FKEY },
+	{ KEY_F2,	KEY_BRIGHTNESSUP,	APPLE_FLAG_FKEY },
+	{ KEY_F3,	KEY_SCALE,		APPLE_FLAG_FKEY },
+	{ KEY_F4,	KEY_DASHBOARD,		APPLE_FLAG_FKEY },
+	{ KEY_F5,	KEY_KBDILLUMDOWN,	APPLE_FLAG_FKEY },
+	{ KEY_F6,	KEY_KBDILLUMUP,		APPLE_FLAG_FKEY },
+	{ KEY_F7,	KEY_PREVIOUSSONG,	APPLE_FLAG_FKEY },
+	{ KEY_F8,	KEY_PLAYPAUSE,		APPLE_FLAG_FKEY },
+	{ KEY_F9,	KEY_NEXTSONG,		APPLE_FLAG_FKEY },
+	{ KEY_F10,	KEY_MUTE,		APPLE_FLAG_FKEY },
+	{ KEY_F11,	KEY_VOLUMEDOWN,		APPLE_FLAG_FKEY },
+	{ KEY_F12,	KEY_VOLUMEUP,		APPLE_FLAG_FKEY },
+	{ KEY_RIGHT,	KEY_END },
+	{ KEY_LEFT,	KEY_HOME },
+	{ KEY_DOWN,	KEY_PAGEDOWN },
+	{ KEY_UP,	KEY_PAGEUP },
+	{ },
+};
+
+static const struct applespi_key_translation apple_iso_keyboard[] = {
+	{ KEY_GRAVE,	KEY_102ND },
+	{ KEY_102ND,	KEY_GRAVE },
+	{ },
+};
+
+struct applespi_tp_model_info {
+	u16			model;
+	struct applespi_tp_info	tp_info;
+};
+
+static const struct applespi_tp_model_info applespi_tp_models[] = {
+	{
+		.model = 0x0417,	/* MB8 MB9 MB10 */
+		.tp_info = { -5087, 5579, -182, 6089 },
+	},
+	{
+		.model = 0x0557,	/* MBP13,1 MBP13,2 MBP14,1 MBP14,2 */
+		.tp_info = { -6243, 6749, -170, 7685 },
+	},
+	{
+		.model = 0x06d7,	/* MBP13,3 MBP14,3 */
+		.tp_info = { -7456, 7976, -163, 9283 },
+	},
+	{}
+};
+
+static const char *applespi_debug_facility(unsigned int log_mask)
+{
+	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-";
+	}
+}
+
+static void applespi_setup_read_txfrs(struct applespi_data *applespi)
+{
+	struct spi_message *msg = &applespi->rd_m;
+	struct spi_transfer *dl_t = &applespi->dl_t;
+	struct spi_transfer *rd_t = &applespi->rd_t;
+
+	memset(dl_t, 0, sizeof(*dl_t));
+	memset(rd_t, 0, sizeof(*rd_t));
+
+	dl_t->delay_usecs = applespi->spi_settings.spi_cs_delay;
+
+	rd_t->rx_buf = applespi->rx_buffer;
+	rd_t->len = APPLESPI_PACKET_SIZE;
+
+	spi_message_init(msg);
+	spi_message_add_tail(dl_t, msg);
+	spi_message_add_tail(rd_t, msg);
+}
+
+static void applespi_setup_write_txfrs(struct applespi_data *applespi)
+{
+	struct spi_message *msg = &applespi->wr_m;
+	struct spi_transfer *wt_t = &applespi->ww_t;
+	struct spi_transfer *dl_t = &applespi->wd_t;
+	struct spi_transfer *wr_t = &applespi->wr_t;
+	struct spi_transfer *st_t = &applespi->st_t;
+
+	memset(wt_t, 0, sizeof(*wt_t));
+	memset(dl_t, 0, sizeof(*dl_t));
+	memset(wr_t, 0, sizeof(*wr_t));
+	memset(st_t, 0, sizeof(*st_t));
+
+	/*
+	 * All we need here is a delay at the beginning of the message before
+	 * asserting cs. But the current spi API doesn't support this, so we
+	 * end up with an extra unnecessary (but harmless) cs assertion and
+	 * deassertion.
+	 */
+	wt_t->delay_usecs = SPI_RW_CHG_DLY;
+	wt_t->cs_change = 1;
+
+	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 = SPI_RW_CHG_DLY;
+
+	st_t->rx_buf = applespi->tx_status;
+	st_t->len = APPLESPI_STATUS_SIZE;
+
+	spi_message_init(msg);
+	spi_message_add_tail(wt_t, msg);
+	spi_message_add_tail(dl_t, msg);
+	spi_message_add_tail(wr_t, msg);
+	spi_message_add_tail(st_t, msg);
+}
+
+static int applespi_async(struct applespi_data *applespi,
+			  struct spi_message *message, void (*complete)(void *))
+{
+	message->complete = complete;
+	message->context = applespi;
+
+	return spi_async(applespi->spi, message);
+}
+
+static inline bool applespi_check_write_status(struct applespi_data *applespi,
+					       int sts)
+{
+	static u8 sts_ok[] = { 0xac, 0x27, 0x68, 0xd5 };
+	bool ret = true;
+
+	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) {
+		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 int applespi_get_spi_settings(struct applespi_data *applespi)
+{
+	struct acpi_device *adev = ACPI_COMPANION(&applespi->spi->dev);
+	const union acpi_object *o;
+	struct spi_settings *settings = &applespi->spi_settings;
+
+	if (!acpi_dev_get_property(adev, "spiCSDelay", ACPI_TYPE_BUFFER, &o))
+		settings->spi_cs_delay = *(u64 *)o->buffer.pointer;
+	else
+		pr_warn("Property spiCSDelay not found\n");
+
+	if (!acpi_dev_get_property(adev, "resetA2RUsec", ACPI_TYPE_BUFFER, &o))
+		settings->reset_a2r_usec = *(u64 *)o->buffer.pointer;
+	else
+		pr_warn("Property resetA2RUsec not found\n");
+
+	if (!acpi_dev_get_property(adev, "resetRecUsec", ACPI_TYPE_BUFFER, &o))
+		settings->reset_rec_usec = *(u64 *)o->buffer.pointer;
+	else
+		pr_warn("Property resetRecUsec not found\n");
+
+	pr_debug("SPI settings: spi_cs_delay=%llu reset_a2r_usec=%llu reset_rec_usec=%llu\n",
+		 settings->spi_cs_delay, settings->reset_a2r_usec,
+		 settings->reset_rec_usec);
+
+	return 0;
+}
+
+static int applespi_setup_spi(struct applespi_data *applespi)
+{
+	int sts;
+
+	sts = applespi_get_spi_settings(applespi);
+	if (sts)
+		return sts;
+
+	spin_lock_init(&applespi->cmd_msg_lock);
+	init_waitqueue_head(&applespi->drain_complete);
+
+	return 0;
+}
+
+static int applespi_enable_spi(struct applespi_data *applespi)
+{
+	int result;
+	unsigned long long spi_status;
+
+	/* check if SPI is already enabled, so we can skip the delay below */
+	result = acpi_evaluate_integer(applespi->sist, NULL, NULL, &spi_status);
+	if (ACPI_SUCCESS(result) && spi_status)
+		return 0;
+
+	/* SIEN(1) will enable SPI communication */
+	result = acpi_execute_simple_method(applespi->sien, NULL, 1);
+	if (ACPI_FAILURE(result)) {
+		pr_err("SIEN failed: %s\n", acpi_format_exception(result));
+		return -ENODEV;
+	}
+
+	/*
+	 * Allow the SPI interface to come up before returning. Without this
+	 * delay, the SPI commands to enable multitouch mode may not reach
+	 * the trackpad controller, causing pointer movement to break upon
+	 * resume from sleep.
+	 */
+	msleep(50);
+
+	return 0;
+}
+
+static int applespi_send_cmd_msg(struct applespi_data *applespi);
+
+static void applespi_msg_complete(struct applespi_data *applespi,
+				  bool is_write_msg, bool is_read_compl)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
+
+	if (is_read_compl)
+		applespi->read_active = false;
+	if (is_write_msg)
+		applespi->write_active = false;
+
+	if (applespi->drain && !applespi->write_active)
+		wake_up_all(&applespi->drain_complete);
+
+	if (is_write_msg) {
+		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;
+
+	debug_print_header(applespi->cmd_log_mask);
+	debug_print_buffer(applespi->cmd_log_mask, "write  ",
+			   applespi->tx_buffer, APPLESPI_PACKET_SIZE);
+	debug_print_buffer(applespi->cmd_log_mask, "status ",
+			   applespi->tx_status, APPLESPI_STATUS_SIZE);
+
+	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);
+}
+
+static int applespi_send_cmd_msg(struct applespi_data *applespi)
+{
+	u16 crc;
+	int sts;
+	struct spi_packet *packet = (struct spi_packet *)applespi->tx_buffer;
+	struct message *message = (struct message *)packet->data;
+	u16 msg_len;
+	u8 device;
+
+	/* check if draining */
+	if (applespi->drain)
+		return 0;
+
+	/* check whether send is in progress */
+	if (applespi->cmd_msg_queued)
+		return 0;
+
+	/* set up packet */
+	memset(packet, 0, APPLESPI_PACKET_SIZE);
+
+	/* are we processing init commands? */
+	if (applespi->want_tp_info_cmd) {
+		applespi->want_tp_info_cmd = false;
+		applespi->want_mt_init_cmd = true;
+		applespi->cmd_log_mask = DBG_CMD_TP_INI;
+
+		/* build init command */
+		device = PACKET_DEV_INFO;
+
+		message->type = cpu_to_le16(0x1020);
+		msg_len = sizeof(message->tp_info_command);
+
+		message->zero = 0x02;
+		message->rsp_buf_len = cpu_to_le16(0x0200);
+
+	} else if (applespi->want_mt_init_cmd) {
+		applespi->want_mt_init_cmd = false;
+		applespi->cmd_log_mask = DBG_CMD_TP_INI;
+
+		/* build init command */
+		device = PACKET_DEV_TPAD;
+
+		message->type = cpu_to_le16(0x0252);
+		msg_len = sizeof(message->init_mt_command);
+
+		message->init_mt_command.cmd = cpu_to_le16(0x0102);
+
+	/* do we need caps-lock command? */
+	} 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;
+
+		/* build led command */
+		device = PACKET_DEV_KEYB;
+
+		message->type = cpu_to_le16(0x0151);
+		msg_len = sizeof(message->capsl_command);
+
+		message->capsl_command.unknown = 0x01;
+		message->capsl_command.led = applespi->have_cl_led_on ? 2 : 0;
+
+	/* do we need backlight command? */
+	} else if (applespi->want_bl_level != applespi->have_bl_level) {
+		applespi->have_bl_level = applespi->want_bl_level;
+		applespi->cmd_log_mask = DBG_CMD_BL;
+
+		/* build command buffer */
+		device = PACKET_DEV_KEYB;
+
+		message->type = cpu_to_le16(0xB051);
+		msg_len = sizeof(message->bl_command);
+
+		message->bl_command.const1 = cpu_to_le16(0x01B0);
+		message->bl_command.level =
+				cpu_to_le16(applespi->have_bl_level);
+
+		if (applespi->have_bl_level > 0)
+			message->bl_command.const2 = cpu_to_le16(0x01F4);
+		else
+			message->bl_command.const2 = cpu_to_le16(0x0001);
+
+	/* everything's up-to-date */
+	} else {
+		return 0;
+	}
+
+	/* finalize packet */
+	packet->flags = PACKET_TYPE_WRITE;
+	packet->device = device;
+	packet->length = cpu_to_le16(MSG_HEADER_SIZE + msg_len);
+
+	message->counter = applespi->cmd_msg_cntr++ & 0xff;
+
+	message->length = cpu_to_le16(msg_len - 2);
+	if (!message->rsp_buf_len)
+		message->rsp_buf_len = message->length;
+
+	crc = crc16(0, (u8 *)message, le16_to_cpu(packet->length) - 2);
+	*((__le16 *)&message->data[msg_len - 2]) = cpu_to_le16(crc);
+
+	crc = crc16(0, (u8 *)packet, sizeof(*packet) - 2);
+	packet->crc_16 = cpu_to_le16(crc);
+
+	/* send command */
+	sts = applespi_async(applespi, &applespi->wr_m,
+			     applespi_async_write_complete);
+
+	if (sts != 0) {
+		pr_warn("Error queueing async write to device: %d\n", sts);
+	} else {
+		applespi->cmd_msg_queued = true;
+		applespi->write_active = true;
+	}
+
+	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;
+	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)
+{
+	unsigned long flags;
+	int sts;
+
+	spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
+
+	applespi->want_cl_led_on = capslock_on;
+	sts = applespi_send_cmd_msg(applespi);
+
+	spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
+
+	return sts;
+}
+
+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);
+
+	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));
+		return 0;
+	}
+
+	return -1;
+}
+
+/* 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);
+}
+
+static void report_finger_data(struct input_dev *input, int slot,
+			       const struct input_mt_pos *pos,
+			       const struct tp_finger *f)
+{
+	input_mt_slot(input, slot);
+	input_mt_report_slot_state(input, MT_TOOL_FINGER, true);
+
+	input_report_abs(input, ABS_MT_TOUCH_MAJOR,
+			 raw2int(f->touch_major) << 1);
+	input_report_abs(input, ABS_MT_TOUCH_MINOR,
+			 raw2int(f->touch_minor) << 1);
+	input_report_abs(input, ABS_MT_WIDTH_MAJOR,
+			 raw2int(f->tool_major) << 1);
+	input_report_abs(input, ABS_MT_WIDTH_MINOR,
+			 raw2int(f->tool_minor) << 1);
+	input_report_abs(input, ABS_MT_ORIENTATION,
+			 MAX_FINGER_ORIENTATION - raw2int(f->orientation));
+	input_report_abs(input, ABS_MT_POSITION_X, pos->x);
+	input_report_abs(input, ABS_MT_POSITION_Y, pos->y);
+}
+
+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;
+
+	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++) {
+		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);
+		}
+	}
+
+	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();
+		}
+	}
+
+	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 const struct applespi_key_translation *applespi_find_translation(
+		const struct applespi_key_translation *table, u16 key)
+{
+	const struct applespi_key_translation *trans;
+
+	for (trans = table; trans->from; trans++)
+		if (trans->from == key)
+			return trans;
+
+	return NULL;
+}
+
+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;
+	}
+
+	return key;
+}
+
+static void applespi_remap_fn_key(struct keyboard_protocol
+							*keyboard_protocol)
+{
+	unsigned char tmp;
+	unsigned long *modifiers = (unsigned long *)
+						&keyboard_protocol->modifiers;
+
+	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)
+{
+	int i, j;
+	unsigned int key;
+	bool still_pressed;
+	bool is_overflow;
+
+	/* check for rollover overflow, which is signalled by all keys == 1 */
+	is_overflow = true;
+
+	for (i = 0; i < MAX_ROLLOVER; i++) {
+		if (keyboard_protocol->keys_pressed[i] != 1) {
+			is_overflow = false;
+			break;
+		}
+	}
+
+	if (is_overflow)
+		return;
+
+	/* remap fn key if desired */
+	applespi_remap_fn_key(keyboard_protocol);
+
+	/* check released keys */
+	for (i = 0; i < MAX_ROLLOVER; i++) {
+		still_pressed = false;
+		for (j = 0; j < MAX_ROLLOVER; 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;
+		}
+	}
+
+	/* check pressed keys */
+	for (i = 0; i < MAX_ROLLOVER; 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 control keys */
+	for (i = 0; i < MAX_MODIFIERS; i++) {
+		u8 *modifiers = &keyboard_protocol->modifiers;
+
+		if (test_bit(i, (unsigned long *)modifiers))
+			input_report_key(applespi->keyboard_input_dev,
+					 applespi_controlcodes[i], 1);
+		else
+			input_report_key(applespi->keyboard_input_dev,
+					 applespi_controlcodes[i], 0);
+	}
+
+	/* 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;
+
+	/* done */
+	input_sync(applespi->keyboard_input_dev);
+	memcpy(&applespi->last_keys_pressed, keyboard_protocol->keys_pressed,
+	       sizeof(applespi->last_keys_pressed));
+}
+
+static const struct applespi_tp_info *applespi_find_touchpad_info(u16 model)
+{
+	const struct applespi_tp_model_info *info;
+
+	for (info = applespi_tp_models; info->model; info++) {
+		if (info->model == model)
+			return &info->tp_info;
+	}
+
+	return NULL;
+}
+
+static void applespi_register_touchpad_device(struct applespi_data *applespi,
+				struct touchpad_info_protocol *rcvd_tp_info)
+{
+	const struct applespi_tp_info *tp_info;
+	struct input_dev *touchpad_input_dev;
+	int res;
+
+	/* set up touchpad dimensions */
+	tp_info = applespi_find_touchpad_info(rcvd_tp_info->model_id);
+	if (!tp_info) {
+		pr_warn("Unknown touchpad model %x - falling back to MB8 touchpad\n",
+			rcvd_tp_info->model_id);
+		tp_info = &applespi_tp_models[0].tp_info;
+	}
+
+	applespi->tp_info = *tp_info;
+
+	if (touchpad_dimensions[0] || touchpad_dimensions[1] ||
+	    touchpad_dimensions[2] || touchpad_dimensions[3]) {
+		pr_info("Overriding touchpad dimensions from module param\n");
+		applespi->tp_info.x_min = touchpad_dimensions[0];
+		applespi->tp_info.x_max = touchpad_dimensions[1];
+		applespi->tp_info.y_min = touchpad_dimensions[2];
+		applespi->tp_info.y_max = touchpad_dimensions[3];
+	} else {
+		touchpad_dimensions[0] = applespi->tp_info.x_min;
+		touchpad_dimensions[1] = applespi->tp_info.x_max;
+		touchpad_dimensions[2] = applespi->tp_info.y_min;
+		touchpad_dimensions[3] = applespi->tp_info.y_max;
+	}
+
+	/* create touchpad input device */
+	touchpad_input_dev = devm_input_allocate_device(&applespi->spi->dev);
+
+	if (!touchpad_input_dev) {
+		pr_err("Failed to allocate touchpad input device\n");
+		return;
+	}
+
+	touchpad_input_dev->name = "Apple SPI Touchpad";
+	touchpad_input_dev->phys = "applespi/input1";
+	touchpad_input_dev->dev.parent = &applespi->spi->dev;
+	touchpad_input_dev->id.bustype = BUS_SPI;
+	touchpad_input_dev->id.vendor = SYNAPTICS_VENDOR_ID;
+	touchpad_input_dev->id.product = rcvd_tp_info->model_id;
+
+	/* basic properties */
+	input_set_capability(touchpad_input_dev, EV_REL, REL_X);
+	input_set_capability(touchpad_input_dev, EV_REL, REL_Y);
+
+	__set_bit(INPUT_PROP_POINTER, touchpad_input_dev->propbit);
+	__set_bit(INPUT_PROP_BUTTONPAD, touchpad_input_dev->propbit);
+
+	/* finger touch area */
+	input_set_abs_params(touchpad_input_dev, ABS_MT_TOUCH_MAJOR,
+			     0, 5000, 0, 0);
+	input_set_abs_params(touchpad_input_dev, ABS_MT_TOUCH_MINOR,
+			     0, 5000, 0, 0);
+
+	/* finger approach area */
+	input_set_abs_params(touchpad_input_dev, ABS_MT_WIDTH_MAJOR,
+			     0, 5000, 0, 0);
+	input_set_abs_params(touchpad_input_dev, ABS_MT_WIDTH_MINOR,
+			     0, 5000, 0, 0);
+
+	/* finger orientation */
+	input_set_abs_params(touchpad_input_dev, ABS_MT_ORIENTATION,
+			     -MAX_FINGER_ORIENTATION, MAX_FINGER_ORIENTATION,
+			     0, 0);
+
+	/* finger position */
+	input_set_abs_params(touchpad_input_dev, ABS_MT_POSITION_X,
+			     applespi->tp_info.x_min, applespi->tp_info.x_max,
+			     0, 0);
+	input_set_abs_params(touchpad_input_dev, ABS_MT_POSITION_Y,
+			     applespi->tp_info.y_min, applespi->tp_info.y_max,
+			     0, 0);
+
+	/* touchpad button */
+	input_set_capability(touchpad_input_dev, EV_KEY, BTN_LEFT);
+
+	/* multitouch */
+	input_mt_init_slots(touchpad_input_dev, MAX_FINGERS,
+			    INPUT_MT_POINTER | INPUT_MT_DROP_UNUSED |
+			    INPUT_MT_TRACK);
+
+	/* register input device */
+	res = input_register_device(touchpad_input_dev);
+	if (res)
+		pr_err("Unabled to register touchpad input device (%d)\n", res);
+	else
+		/* touchpad_input_dev is read async in spi callback */
+		smp_store_release(&applespi->touchpad_input_dev,
+				  touchpad_input_dev);
+}
+
+static void applespi_worker(struct work_struct *work)
+{
+	struct applespi_data *applespi =
+		container_of(work, struct applespi_data, work);
+
+	applespi_register_touchpad_device(applespi, &applespi->rcvd_tp_info);
+}
+
+static void applespi_handle_cmd_response(struct applespi_data *applespi,
+					 struct spi_packet *packet,
+					 struct message *message)
+{
+	if (packet->device == PACKET_DEV_INFO &&
+	    le16_to_cpu(message->type) == 0x1020) {
+		/*
+		 * We're not allowed to sleep here, but registering an input
+		 * device can sleep.
+		 */
+		applespi->rcvd_tp_info = message->tp_info;
+		schedule_work(&applespi->work);
+		return;
+	}
+
+	if (le16_to_cpu(message->length) != 0x0000) {
+		dev_warn_ratelimited(&applespi->spi->dev,
+				     "Received unexpected write response: length=%x\n",
+				     le16_to_cpu(message->length));
+		return;
+	}
+
+	if (packet->device == PACKET_DEV_TPAD &&
+	    le16_to_cpu(message->type) == 0x0252 &&
+	    le16_to_cpu(message->rsp_buf_len) == 0x0002)
+		pr_info("modeswitch done.\n");
+}
+
+static bool applespi_verify_crc(struct applespi_data *applespi, u8 *buffer,
+				size_t buflen)
+{
+	u16 crc;
+
+	crc = crc16(0, buffer, buflen);
+	if (crc != 0) {
+		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_debug_print_read_packet(struct applespi_data *applespi,
+					     struct spi_packet *packet)
+{
+	unsigned int dbg_mask;
+
+	if (packet->flags == PACKET_TYPE_READ &&
+	    packet->device == PACKET_DEV_KEYB)
+		dbg_mask = DBG_RD_KEYB;
+	else if (packet->flags == PACKET_TYPE_READ &&
+		 packet->device == PACKET_DEV_TPAD)
+		dbg_mask = DBG_RD_TPAD;
+	else if (packet->flags == PACKET_TYPE_WRITE)
+		dbg_mask = applespi->cmd_log_mask;
+	else
+		dbg_mask = DBG_RD_UNKN;
+
+	debug_print_header(dbg_mask);
+	debug_print_buffer(dbg_mask, "read   ", applespi->rx_buffer,
+			   APPLESPI_PACKET_SIZE);
+}
+
+static void applespi_got_data(struct applespi_data *applespi)
+{
+	struct spi_packet *packet;
+	struct message *message;
+	unsigned int msg_len;
+	unsigned int off;
+	unsigned int rem;
+	unsigned int len;
+
+	/* process packet header */
+	if (!applespi_verify_crc(applespi, applespi->rx_buffer,
+				 APPLESPI_PACKET_SIZE)) {
+		unsigned long flags;
+
+		spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
+
+		if (applespi->drain) {
+			applespi->read_active = false;
+			applespi->write_active = false;
+
+			wake_up_all(&applespi->drain_complete);
+		}
+
+		spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
+
+		return;
+	}
+
+	packet = (struct spi_packet *)applespi->rx_buffer;
+
+	applespi_debug_print_read_packet(applespi, packet);
+
+	off = le16_to_cpu(packet->offset);
+	rem = le16_to_cpu(packet->remaining);
+	len = le16_to_cpu(packet->length);
+
+	if (len > sizeof(packet->data)) {
+		dev_warn_ratelimited(&applespi->spi->dev,
+				     "Received corrupted packet (invalid packet length)\n");
+		goto cleanup;
+	}
+
+	/* handle multi-packet messages */
+	if (rem > 0 || off > 0) {
+		if (off != applespi->saved_msg_len) {
+			dev_warn_ratelimited(&applespi->spi->dev,
+					     "Received unexpected offset (got %u, expected %u)\n",
+					     off, applespi->saved_msg_len);
+			goto cleanup;
+		}
+
+		if (off + rem > MAX_PKTS_PER_MSG * APPLESPI_PACKET_SIZE) {
+			dev_warn_ratelimited(&applespi->spi->dev,
+					     "Received message too large (size %u)\n",
+					     off + rem);
+			goto cleanup;
+		}
+
+		if (off + len > MAX_PKTS_PER_MSG * APPLESPI_PACKET_SIZE) {
+			dev_warn_ratelimited(&applespi->spi->dev,
+					     "Received message too large (size %u)\n",
+					     off + len);
+			goto cleanup;
+		}
+
+		memcpy(applespi->msg_buf + off, &packet->data, len);
+		applespi->saved_msg_len += len;
+
+		if (rem > 0)
+			return;
+
+		message = (struct message *)applespi->msg_buf;
+		msg_len = applespi->saved_msg_len;
+	} else {
+		message = (struct message *)&packet->data;
+		msg_len = len;
+	}
+
+	/* got complete message - verify */
+	if (!applespi_verify_crc(applespi, (u8 *)message, msg_len))
+		goto cleanup;
+
+	if (le16_to_cpu(message->length) != msg_len - MSG_HEADER_SIZE - 2) {
+		dev_warn_ratelimited(&applespi->spi->dev,
+				     "Received corrupted packet (invalid message length)\n");
+		goto cleanup;
+	}
+
+	/* handle message */
+	if (packet->flags == PACKET_TYPE_READ &&
+	    packet->device == PACKET_DEV_KEYB) {
+		applespi_handle_keyboard_event(applespi, &message->keyboard);
+
+	} 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]);
+		if (le16_to_cpu(message->length) + 2 != tp_len) {
+			dev_warn_ratelimited(&applespi->spi->dev,
+					     "Received corrupted packet (invalid message length)\n");
+			goto cleanup;
+		}
+
+		if (tp->number_of_fingers > MAX_FINGERS) {
+			dev_warn_ratelimited(&applespi->spi->dev,
+					     "Number of reported fingers (%u) exceeds max (%u))\n",
+					     tp->number_of_fingers,
+					     MAX_FINGERS);
+			tp->number_of_fingers = MAX_FINGERS;
+		}
+
+		report_tp_state(applespi, tp);
+
+	} else if (packet->flags == PACKET_TYPE_WRITE) {
+		applespi_handle_cmd_response(applespi, packet, message);
+	}
+
+cleanup:
+	/* clean up */
+	applespi->saved_msg_len = 0;
+
+	applespi_msg_complete(applespi, packet->flags == PACKET_TYPE_WRITE,
+			      true);
+}
+
+static void applespi_async_read_complete(void *context)
+{
+	struct applespi_data *applespi = context;
+
+	if (applespi->rd_m.status < 0) {
+		pr_warn("Error reading from device: %d\n",
+			applespi->rd_m.status);
+		/*
+		 * We don't actually know if this was a pure read, or a response
+		 * to a write. But this is a rare error condition that should
+		 * never occur, so clearing both flags to avoid deadlock.
+		 */
+		applespi_msg_complete(applespi, true, true);
+	} else {
+		applespi_got_data(applespi);
+	}
+
+	acpi_finish_gpe(NULL, applespi->gpe);
+}
+
+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)
+			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;
+
+	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 void applespi_save_bl_level(unsigned int level)
+{
+	efi_guid_t efi_guid;
+	u32 efi_attr;
+	unsigned long efi_data_len;
+	u16 efi_data;
+	int sts;
+
+	/* Save keyboard backlight level */
+	efi_guid = EFI_BL_LEVEL_GUID;
+	efi_data = (u16)level;
+	efi_data_len = sizeof(efi_data);
+	efi_attr = EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS |
+		   EFI_VARIABLE_RUNTIME_ACCESS;
+
+	sts = efivar_entry_set_safe(EFI_BL_LEVEL_NAME, efi_guid, efi_attr, true,
+				    efi_data_len, &efi_data);
+	if (sts)
+		pr_warn("Error saving backlight level to EFI vars: %d\n", sts);
+}
+
+static int applespi_probe(struct spi_device *spi)
+{
+	struct applespi_data *applespi;
+	int result, i;
+	unsigned long long 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);
+
+	INIT_WORK(&applespi->work, applespi_worker);
+
+	/* store the driver data */
+	spi_set_drvdata(spi, applespi);
+
+	/* create our buffers */
+	applespi->tx_buffer = devm_kmalloc(&spi->dev, APPLESPI_PACKET_SIZE,
+					   GFP_KERNEL);
+	applespi->tx_status = devm_kmalloc(&spi->dev, APPLESPI_STATUS_SIZE,
+					   GFP_KERNEL);
+	applespi->rx_buffer = devm_kmalloc(&spi->dev, APPLESPI_PACKET_SIZE,
+					   GFP_KERNEL);
+	applespi->msg_buf = devm_kmalloc(&spi->dev, MAX_PKTS_PER_MSG *
+						    APPLESPI_PACKET_SIZE,
+					 GFP_KERNEL);
+
+	if (!applespi->tx_buffer || !applespi->tx_status ||
+	    !applespi->rx_buffer || !applespi->msg_buf)
+		return -ENOMEM;
+
+	/* set up our spi messages */
+	applespi_setup_read_txfrs(applespi);
+	applespi_setup_write_txfrs(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;
+
+	/* setup the keyboard input dev */
+	applespi->keyboard_input_dev = devm_input_allocate_device(&spi->dev);
+
+	if (!applespi->keyboard_input_dev)
+		return -ENOMEM;
+
+	applespi->keyboard_input_dev->name = "Apple SPI Keyboard";
+	applespi->keyboard_input_dev->phys = "applespi/input0";
+	applespi->keyboard_input_dev->dev.parent = &spi->dev;
+	applespi->keyboard_input_dev->id.bustype = BUS_SPI;
+
+	applespi->keyboard_input_dev->evbit[0] =
+			BIT_MASK(EV_KEY) | BIT_MASK(EV_LED) | BIT_MASK(EV_REP);
+	applespi->keyboard_input_dev->ledbit[0] = BIT_MASK(LED_CAPSL);
+
+	input_set_drvdata(applespi->keyboard_input_dev, applespi);
+	applespi->keyboard_input_dev->event = applespi_event;
+
+	for (i = 0; i < ARRAY_SIZE(applespi_scancodes); i++)
+		if (applespi_scancodes[i])
+			input_set_capability(applespi->keyboard_input_dev,
+					     EV_KEY, applespi_scancodes[i]);
+
+	for (i = 0; i < ARRAY_SIZE(applespi_controlcodes); i++)
+		if (applespi_controlcodes[i])
+			input_set_capability(applespi->keyboard_input_dev,
+					     EV_KEY, applespi_controlcodes[i]);
+
+	for (i = 0; i < ARRAY_SIZE(applespi_fn_codes); i++)
+		if (applespi_fn_codes[i].to)
+			input_set_capability(applespi->keyboard_input_dev,
+					     EV_KEY, applespi_fn_codes[i].to);
+
+	input_set_capability(applespi->keyboard_input_dev, EV_KEY, KEY_FN);
+
+	result = input_register_device(applespi->keyboard_input_dev);
+	if (result) {
+		pr_err("Unabled to register keyboard input device (%d)\n",
+		       result);
+		return -ENODEV;
+	}
+
+	/*
+	 * The applespi device doesn't send interrupts normally (as is described
+	 * in its DSDT), but rather seems to use ACPI GPEs.
+	 */
+	result = acpi_evaluate_integer(applespi->handle, "_GPE", NULL, &gpe);
+	if (ACPI_FAILURE(result)) {
+		pr_err("Failed to obtain GPE for SPI slave device: %s\n",
+		       acpi_format_exception(result));
+		return -ENODEV;
+	}
+	applespi->gpe = (int)gpe;
+
+	result = acpi_install_gpe_handler(NULL, applespi->gpe,
+					  ACPI_GPE_LEVEL_TRIGGERED,
+					  applespi_notify, applespi);
+	if (ACPI_FAILURE(result)) {
+		pr_err("Failed to install GPE handler for GPE %d: %s\n",
+		       applespi->gpe, acpi_format_exception(result));
+		return -ENODEV;
+	}
+
+	applespi->suspended = false;
+
+	result = acpi_enable_gpe(NULL, applespi->gpe);
+	if (ACPI_FAILURE(result)) {
+		pr_err("Failed to enable GPE handler for GPE %d: %s\n",
+		       applespi->gpe, acpi_format_exception(result));
+		acpi_remove_gpe_handler(NULL, applespi->gpe, applespi_notify);
+		return -ENODEV;
+	}
+
+	/* trigger touchpad setup */
+	applespi_init(applespi, false);
+
+	/*
+	 * 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);
+
+	/* set up keyboard-backlight */
+	result = applespi_get_saved_bl_level();
+	if (result >= 0)
+		applespi_set_bl_level(&applespi->backlight_info, result);
+
+	applespi->backlight_info.name            = "spi::kbd_backlight";
+	applespi->backlight_info.default_trigger = "kbd-backlight";
+	applespi->backlight_info.brightness_set  = applespi_set_bl_level;
+
+	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 */
+	}
+
+	/* done */
+	pr_info("spi-device probe done: %s\n", dev_name(&spi->dev));
+
+	return 0;
+}
+
+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;
+}
+
+static void applespi_shutdown(struct spi_device *spi)
+{
+	struct applespi_data *applespi = spi_get_drvdata(spi);
+
+	applespi_save_bl_level(applespi->have_bl_level);
+}
+
+static int applespi_poweroff_late(struct device *dev)
+{
+	struct spi_device *spi = to_spi_device(dev);
+	struct applespi_data *applespi = spi_get_drvdata(spi);
+
+	applespi_save_bl_level(applespi->have_bl_level);
+
+	return 0;
+}
+
+static int applespi_suspend(struct device *dev)
+{
+	struct spi_device *spi = to_spi_device(dev);
+	struct applespi_data *applespi = spi_get_drvdata(spi);
+	acpi_status status;
+	unsigned long flags;
+	int rc;
+
+	/* turn off caps-lock - it'll stay on otherwise */
+	rc = applespi_set_capsl_led(applespi, false);
+	if (rc)
+		pr_warn("Failed to turn off caps-lock led (%d)\n", rc);
+
+	/* 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);
+
+	/* disable the interrupt */
+	status = acpi_disable_gpe(NULL, applespi->gpe);
+	if (ACPI_FAILURE(status)) {
+		pr_err("Failed to disable GPE handler for GPE %d: %s\n",
+		       applespi->gpe, acpi_format_exception(status));
+	}
+
+	/* 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);
+
+	applespi->suspended = true;
+
+	spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
+
+	pr_info("spi-device suspend done.\n");
+	return 0;
+}
+
+static int applespi_resume(struct device *dev)
+{
+	struct spi_device *spi = to_spi_device(dev);
+	struct applespi_data *applespi = spi_get_drvdata(spi);
+	acpi_status status;
+	unsigned long flags;
+
+	/* ensure our flags and state reflect a newly resumed device */
+	spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
+
+	applespi->drain = false;
+	applespi->have_cl_led_on = false;
+	applespi->have_bl_level = 0;
+	applespi->cmd_msg_queued = false;
+	applespi->read_active = false;
+	applespi->write_active = false;
+
+	applespi->suspended = false;
+
+	spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
+
+	/* switch on the SPI interface */
+	applespi_enable_spi(applespi);
+
+	/* re-enable the interrupt */
+	status = acpi_enable_gpe(NULL, applespi->gpe);
+	if (ACPI_FAILURE(status)) {
+		pr_err("Failed to re-enable GPE handler for GPE %d: %s\n",
+		       applespi->gpe, acpi_format_exception(status));
+	}
+
+	/* switch the touchpad into multitouch mode */
+	applespi_init(applespi, true);
+
+	pr_info("spi-device resume done.\n");
+
+	return 0;
+}
+
+static const struct acpi_device_id applespi_acpi_match[] = {
+	{ "APP000D", 0 },
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, applespi_acpi_match);
+
+const struct dev_pm_ops applespi_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(applespi_suspend, applespi_resume)
+	.poweroff_late	= applespi_poweroff_late,
+};
+
+static struct spi_driver applespi_driver = {
+	.driver		= {
+		.name			= "applespi",
+		.owner			= THIS_MODULE,
+
+		.acpi_match_table	= ACPI_PTR(applespi_acpi_match),
+		.pm			= &applespi_pm_ops,
+	},
+	.probe		= applespi_probe,
+	.remove		= applespi_remove,
+	.shutdown	= applespi_shutdown,
+};
+
+module_spi_driver(applespi_driver)
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("MacBook(Pro) SPI Keyboard/Touchpad driver");
+MODULE_AUTHOR("Federico Lorenzi");
+MODULE_AUTHOR("Ronald Tschalär");
-- 
2.20.1

Re: [PATCH 2/2] Input: add Apple SPI keyboard and trackpad driver.

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 5350 bytes --]

Hi Ronald,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on input/next]
[also build test ERROR on v5.0-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Ronald-Tschal-r/drm-bridge-sil_sii8620-depend-on-INPUT-instead-of-selecting-it/20190205-003319
base:   https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gnu-gcc (Debian 8.2.0-11) 8.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=8.2.0 make.cross ARCH=sh 

All error/warnings (new ones prefixed by >>):

   drivers/input/keyboard/applespi.c: In function 'applespi_enable_spi':
>> drivers/input/keyboard/applespi.c:692:11: error: implicit declaration of function 'acpi_evaluate_integer'; did you mean 'acpi_evaluate_object'? [-Werror=implicit-function-declaration]
     result = acpi_evaluate_integer(applespi->sist, NULL, NULL, &spi_status);
              ^~~~~~~~~~~~~~~~~~~~~
              acpi_evaluate_object
>> drivers/input/keyboard/applespi.c:697:11: error: implicit declaration of function 'acpi_execute_simple_method'; did you mean 'acpi_install_method'? [-Werror=implicit-function-declaration]
     result = acpi_execute_simple_method(applespi->sien, NULL, 1);
              ^~~~~~~~~~~~~~~~~~~~~~~~~~
              acpi_install_method
   At top level:
   drivers/input/keyboard/applespi.c:1851:12: warning: 'applespi_resume' defined but not used [-Wunused-function]
    static int applespi_resume(struct device *dev)
               ^~~~~~~~~~~~~~~
   drivers/input/keyboard/applespi.c:1808:12: warning: 'applespi_suspend' defined but not used [-Wunused-function]
    static int applespi_suspend(struct device *dev)
               ^~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors
--
   drivers/spi/spi-pxa2xx-pci.c: In function 'pxa2xx_spi_pci_probe':
>> drivers/spi/spi-pxa2xx-pci.c:205:8: error: implicit declaration of function 'pcim_enable_device'; did you mean 'pci_enable_device'? [-Werror=implicit-function-declaration]
     ret = pcim_enable_device(dev);
           ^~~~~~~~~~~~~~~~~~
           pci_enable_device
>> drivers/spi/spi-pxa2xx-pci.c:235:8: error: implicit declaration of function 'pci_alloc_irq_vectors'; did you mean 'pci_alloc_consistent'? [-Werror=implicit-function-declaration]
     ret = pci_alloc_irq_vectors(dev, 1, 1, PCI_IRQ_ALL_TYPES);
           ^~~~~~~~~~~~~~~~~~~~~
           pci_alloc_consistent
   drivers/spi/spi-pxa2xx-pci.c:235:41: error: 'PCI_IRQ_ALL_TYPES' undeclared (first use in this function); did you mean 'PCI_PRI_ALLOC_REQ'?
     ret = pci_alloc_irq_vectors(dev, 1, 1, PCI_IRQ_ALL_TYPES);
                                            ^~~~~~~~~~~~~~~~~
                                            PCI_PRI_ALLOC_REQ
   drivers/spi/spi-pxa2xx-pci.c:235:41: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/spi/spi-pxa2xx-pci.c:238:13: error: implicit declaration of function 'pci_irq_vector'; did you mean 'rcu_irq_enter'? [-Werror=implicit-function-declaration]
     ssp->irq = pci_irq_vector(dev, 0);
                ^~~~~~~~~~~~~~
                rcu_irq_enter
   drivers/spi/spi-pxa2xx-pci.c: At top level:
>> drivers/spi/spi-pxa2xx-pci.c:296:1: warning: data definition has no type or storage class
    module_pci_driver(pxa2xx_spi_pci_driver);
    ^~~~~~~~~~~~~~~~~
>> drivers/spi/spi-pxa2xx-pci.c:296:1: error: type defaults to 'int' in declaration of 'module_pci_driver' [-Werror=implicit-int]
>> drivers/spi/spi-pxa2xx-pci.c:296:1: warning: parameter names (without types) in function declaration
   drivers/spi/spi-pxa2xx-pci.c:289:26: warning: 'pxa2xx_spi_pci_driver' defined but not used [-Wunused-variable]
    static struct pci_driver pxa2xx_spi_pci_driver = {
                             ^~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +692 drivers/input/keyboard/applespi.c

   685	
   686	static int applespi_enable_spi(struct applespi_data *applespi)
   687	{
   688		int result;
   689		unsigned long long spi_status;
   690	
   691		/* check if SPI is already enabled, so we can skip the delay below */
 > 692		result = acpi_evaluate_integer(applespi->sist, NULL, NULL, &spi_status);
   693		if (ACPI_SUCCESS(result) && spi_status)
   694			return 0;
   695	
   696		/* SIEN(1) will enable SPI communication */
 > 697		result = acpi_execute_simple_method(applespi->sien, NULL, 1);
   698		if (ACPI_FAILURE(result)) {
   699			pr_err("SIEN failed: %s\n", acpi_format_exception(result));
   700			return -ENODEV;
   701		}
   702	
   703		/*
   704		 * Allow the SPI interface to come up before returning. Without this
   705		 * delay, the SPI commands to enable multitouch mode may not reach
   706		 * the trackpad controller, causing pointer movement to break upon
   707		 * resume from sleep.
   708		 */
   709		msleep(50);
   710	
   711		return 0;
   712	}
   713	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 50436 bytes --]

Re: [PATCH 2/2] Input: add Apple SPI keyboard and trackpad driver.

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 10986 bytes --]

Hi Ronald,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on input/next]
[also build test ERROR on v5.0-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Ronald-Tschal-r/drm-bridge-sil_sii8620-depend-on-INPUT-instead-of-selecting-it/20190205-003319
base:   https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gnu-gcc (Debian 8.2.0-11) 8.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=8.2.0 make.cross ARCH=sh 

All errors (new ones prefixed by >>):

   drivers//spi/spi-pxa2xx-pci.c: In function 'pxa2xx_spi_pci_probe':
   drivers//spi/spi-pxa2xx-pci.c:205:8: error: implicit declaration of function 'pcim_enable_device'; did you mean 'pci_enable_device'? [-Werror=implicit-function-declaration]
     ret = pcim_enable_device(dev);
           ^~~~~~~~~~~~~~~~~~
           pci_enable_device
   drivers//spi/spi-pxa2xx-pci.c:235:8: error: implicit declaration of function 'pci_alloc_irq_vectors'; did you mean 'pci_alloc_consistent'? [-Werror=implicit-function-declaration]
     ret = pci_alloc_irq_vectors(dev, 1, 1, PCI_IRQ_ALL_TYPES);
           ^~~~~~~~~~~~~~~~~~~~~
           pci_alloc_consistent
>> drivers//spi/spi-pxa2xx-pci.c:235:41: error: 'PCI_IRQ_ALL_TYPES' undeclared (first use in this function); did you mean 'PCI_PRI_ALLOC_REQ'?
     ret = pci_alloc_irq_vectors(dev, 1, 1, PCI_IRQ_ALL_TYPES);
                                            ^~~~~~~~~~~~~~~~~
                                            PCI_PRI_ALLOC_REQ
   drivers//spi/spi-pxa2xx-pci.c:235:41: note: each undeclared identifier is reported only once for each function it appears in
   drivers//spi/spi-pxa2xx-pci.c:238:13: error: implicit declaration of function 'pci_irq_vector'; did you mean 'rcu_irq_enter'? [-Werror=implicit-function-declaration]
     ssp->irq = pci_irq_vector(dev, 0);
                ^~~~~~~~~~~~~~
                rcu_irq_enter
   drivers//spi/spi-pxa2xx-pci.c: At top level:
   drivers//spi/spi-pxa2xx-pci.c:296:1: warning: data definition has no type or storage class
    module_pci_driver(pxa2xx_spi_pci_driver);
    ^~~~~~~~~~~~~~~~~
   drivers//spi/spi-pxa2xx-pci.c:296:1: error: type defaults to 'int' in declaration of 'module_pci_driver' [-Werror=implicit-int]
   drivers//spi/spi-pxa2xx-pci.c:296:1: warning: parameter names (without types) in function declaration
   drivers//spi/spi-pxa2xx-pci.c:289:26: warning: 'pxa2xx_spi_pci_driver' defined but not used [-Wunused-variable]
    static struct pci_driver pxa2xx_spi_pci_driver = {
                             ^~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +235 drivers//spi/spi-pxa2xx-pci.c

d6ba32d5c drivers/spi/spi-pxa2xx-pci.c Chew, Chiau Ee            2014-04-18  193  
d6ba32d5c drivers/spi/spi-pxa2xx-pci.c Chew, Chiau Ee            2014-04-18  194  static int pxa2xx_spi_pci_probe(struct pci_dev *dev,
d6ea3df0d drivers/spi/pxa2xx_spi_pci.c Sebastian Andrzej Siewior 2010-11-24  195  		const struct pci_device_id *ent)
d6ea3df0d drivers/spi/pxa2xx_spi_pci.c Sebastian Andrzej Siewior 2010-11-24  196  {
0202775bc drivers/spi/spi-pxa2xx-pci.c Mika Westerberg           2013-01-07  197  	struct platform_device_info pi;
d6ea3df0d drivers/spi/pxa2xx_spi_pci.c Sebastian Andrzej Siewior 2010-11-24  198  	int ret;
d6ea3df0d drivers/spi/pxa2xx_spi_pci.c Sebastian Andrzej Siewior 2010-11-24  199  	struct platform_device *pdev;
0f3e1d27a drivers/spi/pxa2xx_spi_pci.c Sebastian Andrzej Siewior 2011-02-03  200  	struct pxa2xx_spi_master spi_pdata;
d6ea3df0d drivers/spi/pxa2xx_spi_pci.c Sebastian Andrzej Siewior 2010-11-24  201  	struct ssp_device *ssp;
d6ba32d5c drivers/spi/spi-pxa2xx-pci.c Chew, Chiau Ee            2014-04-18  202  	struct pxa_spi_info *c;
afa93c901 drivers/spi/spi-pxa2xx-pci.c Chew, Chiau Ee            2014-07-25  203  	char buf[40];
d6ea3df0d drivers/spi/pxa2xx_spi_pci.c Sebastian Andrzej Siewior 2010-11-24  204  
0202775bc drivers/spi/spi-pxa2xx-pci.c Mika Westerberg           2013-01-07 @205  	ret = pcim_enable_device(dev);
d6ea3df0d drivers/spi/pxa2xx_spi_pci.c Sebastian Andrzej Siewior 2010-11-24  206  	if (ret)
d6ea3df0d drivers/spi/pxa2xx_spi_pci.c Sebastian Andrzej Siewior 2010-11-24  207  		return ret;
d6ea3df0d drivers/spi/pxa2xx_spi_pci.c Sebastian Andrzej Siewior 2010-11-24  208  
0202775bc drivers/spi/spi-pxa2xx-pci.c Mika Westerberg           2013-01-07  209  	ret = pcim_iomap_regions(dev, 1 << 0, "PXA2xx SPI");
c13463407 drivers/spi/spi-pxa2xx-pci.c Mika Westerberg           2013-03-05  210  	if (ret)
d6ea3df0d drivers/spi/pxa2xx_spi_pci.c Sebastian Andrzej Siewior 2010-11-24  211  		return ret;
d6ea3df0d drivers/spi/pxa2xx_spi_pci.c Sebastian Andrzej Siewior 2010-11-24  212  
d6ba32d5c drivers/spi/spi-pxa2xx-pci.c Chew, Chiau Ee            2014-04-18  213  	c = &spi_info_configs[ent->driver_data];
743485ea3 drivers/spi/spi-pxa2xx-pci.c Andy Shevchenko           2016-07-04  214  	if (c->setup) {
743485ea3 drivers/spi/spi-pxa2xx-pci.c Andy Shevchenko           2016-07-04  215  		ret = c->setup(dev, c);
743485ea3 drivers/spi/spi-pxa2xx-pci.c Andy Shevchenko           2016-07-04  216  		if (ret)
743485ea3 drivers/spi/spi-pxa2xx-pci.c Andy Shevchenko           2016-07-04  217  			return ret;
b729bf345 drivers/spi/spi-pxa2xx-pci.c Mika Westerberg           2014-08-19  218  	}
b729bf345 drivers/spi/spi-pxa2xx-pci.c Mika Westerberg           2014-08-19  219  
743485ea3 drivers/spi/spi-pxa2xx-pci.c Andy Shevchenko           2016-07-04  220  	memset(&spi_pdata, 0, sizeof(spi_pdata));
743485ea3 drivers/spi/spi-pxa2xx-pci.c Andy Shevchenko           2016-07-04  221  	spi_pdata.num_chipselect = (c->num_chipselect > 0) ? c->num_chipselect : dev->devfn;
743485ea3 drivers/spi/spi-pxa2xx-pci.c Andy Shevchenko           2016-07-04  222  	spi_pdata.dma_filter = c->dma_filter;
b729bf345 drivers/spi/spi-pxa2xx-pci.c Mika Westerberg           2014-08-19  223  	spi_pdata.tx_param = c->tx_param;
b729bf345 drivers/spi/spi-pxa2xx-pci.c Mika Westerberg           2014-08-19  224  	spi_pdata.rx_param = c->rx_param;
b729bf345 drivers/spi/spi-pxa2xx-pci.c Mika Westerberg           2014-08-19  225  	spi_pdata.enable_dma = c->rx_param && c->tx_param;
d6ea3df0d drivers/spi/pxa2xx_spi_pci.c Sebastian Andrzej Siewior 2010-11-24  226  
851bacf59 drivers/spi/spi-pxa2xx-pci.c Mika Westerberg           2013-01-07  227  	ssp = &spi_pdata.ssp;
d6ea3df0d drivers/spi/pxa2xx_spi_pci.c Sebastian Andrzej Siewior 2010-11-24  228  	ssp->phys_base = pci_resource_start(dev, 0);
0202775bc drivers/spi/spi-pxa2xx-pci.c Mika Westerberg           2013-01-07  229  	ssp->mmio_base = pcim_iomap_table(dev)[0];
d6ba32d5c drivers/spi/spi-pxa2xx-pci.c Chew, Chiau Ee            2014-04-18  230  	ssp->port_id = (c->port_id >= 0) ? c->port_id : dev->devfn;
d6ba32d5c drivers/spi/spi-pxa2xx-pci.c Chew, Chiau Ee            2014-04-18  231  	ssp->type = c->type;
d6ea3df0d drivers/spi/pxa2xx_spi_pci.c Sebastian Andrzej Siewior 2010-11-24  232  
64e02cb0b drivers/spi/spi-pxa2xx-pci.c Jan Kiszka                2017-01-21  233  	pci_set_master(dev);
64e02cb0b drivers/spi/spi-pxa2xx-pci.c Jan Kiszka                2017-01-21  234  
64e02cb0b drivers/spi/spi-pxa2xx-pci.c Jan Kiszka                2017-01-21 @235  	ret = pci_alloc_irq_vectors(dev, 1, 1, PCI_IRQ_ALL_TYPES);
64e02cb0b drivers/spi/spi-pxa2xx-pci.c Jan Kiszka                2017-01-21  236  	if (ret < 0)
64e02cb0b drivers/spi/spi-pxa2xx-pci.c Jan Kiszka                2017-01-21  237  		return ret;
64e02cb0b drivers/spi/spi-pxa2xx-pci.c Jan Kiszka                2017-01-21  238  	ssp->irq = pci_irq_vector(dev, 0);
64e02cb0b drivers/spi/spi-pxa2xx-pci.c Jan Kiszka                2017-01-21  239  
afa93c901 drivers/spi/spi-pxa2xx-pci.c Chew, Chiau Ee            2014-07-25  240  	snprintf(buf, sizeof(buf), "pxa2xx-spi.%d", ssp->port_id);
280af2b8e drivers/spi/spi-pxa2xx-pci.c Stephen Boyd              2016-04-19  241  	ssp->clk = clk_register_fixed_rate(&dev->dev, buf , NULL, 0,
280af2b8e drivers/spi/spi-pxa2xx-pci.c Stephen Boyd              2016-04-19  242  					   c->max_clk_rate);
afa93c901 drivers/spi/spi-pxa2xx-pci.c Chew, Chiau Ee            2014-07-25  243  	 if (IS_ERR(ssp->clk))
afa93c901 drivers/spi/spi-pxa2xx-pci.c Chew, Chiau Ee            2014-07-25  244  		return PTR_ERR(ssp->clk);
afa93c901 drivers/spi/spi-pxa2xx-pci.c Chew, Chiau Ee            2014-07-25  245  
0202775bc drivers/spi/spi-pxa2xx-pci.c Mika Westerberg           2013-01-07  246  	memset(&pi, 0, sizeof(pi));
b70cd2de0 drivers/spi/spi-pxa2xx-pci.c Andy Shevchenko           2016-08-24  247  	pi.fwnode = dev->dev.fwnode;
0202775bc drivers/spi/spi-pxa2xx-pci.c Mika Westerberg           2013-01-07  248  	pi.parent = &dev->dev;
0202775bc drivers/spi/spi-pxa2xx-pci.c Mika Westerberg           2013-01-07  249  	pi.name = "pxa2xx-spi";
0202775bc drivers/spi/spi-pxa2xx-pci.c Mika Westerberg           2013-01-07  250  	pi.id = ssp->port_id;
0202775bc drivers/spi/spi-pxa2xx-pci.c Mika Westerberg           2013-01-07  251  	pi.data = &spi_pdata;
0202775bc drivers/spi/spi-pxa2xx-pci.c Mika Westerberg           2013-01-07  252  	pi.size_data = sizeof(spi_pdata);
d6ea3df0d drivers/spi/pxa2xx_spi_pci.c Sebastian Andrzej Siewior 2010-11-24  253  
0202775bc drivers/spi/spi-pxa2xx-pci.c Mika Westerberg           2013-01-07  254  	pdev = platform_device_register_full(&pi);
afa93c901 drivers/spi/spi-pxa2xx-pci.c Chew, Chiau Ee            2014-07-25  255  	if (IS_ERR(pdev)) {
afa93c901 drivers/spi/spi-pxa2xx-pci.c Chew, Chiau Ee            2014-07-25  256  		clk_unregister(ssp->clk);
d77b5382e drivers/spi/spi-pxa2xx-pci.c Wei Yongjun               2013-02-22  257  		return PTR_ERR(pdev);
afa93c901 drivers/spi/spi-pxa2xx-pci.c Chew, Chiau Ee            2014-07-25  258  	}
0202775bc drivers/spi/spi-pxa2xx-pci.c Mika Westerberg           2013-01-07  259  
851bacf59 drivers/spi/spi-pxa2xx-pci.c Mika Westerberg           2013-01-07  260  	pci_set_drvdata(dev, pdev);
0202775bc drivers/spi/spi-pxa2xx-pci.c Mika Westerberg           2013-01-07  261  
0202775bc drivers/spi/spi-pxa2xx-pci.c Mika Westerberg           2013-01-07  262  	return 0;
d6ea3df0d drivers/spi/pxa2xx_spi_pci.c Sebastian Andrzej Siewior 2010-11-24  263  }
d6ea3df0d drivers/spi/pxa2xx_spi_pci.c Sebastian Andrzej Siewior 2010-11-24  264  

:::::: The code at line 235 was first introduced by commit
:::::: 64e02cb0bdfc7cef0a01e2ad4d567fdc0a74450e spi: pca2xx-pci: Allow MSI

:::::: TO: Jan Kiszka <jan.kiszka@siemens.com>
:::::: CC: Mark Brown <broonie@kernel.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 50455 bytes --]

Re: [PATCH 2/2] Input: add Apple SPI keyboard and trackpad driver.

[Dan Carpenter]

Hi Ronald,

Thank you for the patch! Perhaps something to improve:

url:    https://github.com/0day-ci/linux/commits/Ronald-Tschal-r/drm-bridge-sil_sii8620-depend-on-INPUT-instead-of-selecting-it/20190205-003319
base:   https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next

smatch warnings:
drivers/input/keyboard/applespi.c:1551 applespi_get_saved_bl_level() warn: returning -1 instead of -ENOMEM is sloppy

# https://github.com/0day-ci/linux/commit/cfa9f37054a5b21113aa8b00c455275145114f8e
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout cfa9f37054a5b21113aa8b00c455275145114f8e
vim +1551 drivers/input/keyboard/applespi.c

cfa9f3705 Ronald Tschalär 2019-02-04  1541  
cfa9f3705 Ronald Tschalär 2019-02-04  1542  static int applespi_get_saved_bl_level(void)
cfa9f3705 Ronald Tschalär 2019-02-04  1543  {
cfa9f3705 Ronald Tschalär 2019-02-04  1544  	struct efivar_entry *efivar_entry;
cfa9f3705 Ronald Tschalär 2019-02-04  1545  	u16 efi_data = 0;
cfa9f3705 Ronald Tschalär 2019-02-04  1546  	unsigned long efi_data_len;
cfa9f3705 Ronald Tschalär 2019-02-04  1547  	int sts;
cfa9f3705 Ronald Tschalär 2019-02-04  1548  
cfa9f3705 Ronald Tschalär 2019-02-04  1549  	efivar_entry = kmalloc(sizeof(*efivar_entry), GFP_KERNEL);
cfa9f3705 Ronald Tschalär 2019-02-04  1550  	if (!efivar_entry)
cfa9f3705 Ronald Tschalär 2019-02-04 @1551  		return -1;
cfa9f3705 Ronald Tschalär 2019-02-04  1552  
cfa9f3705 Ronald Tschalär 2019-02-04  1553  	memcpy(efivar_entry->var.VariableName, EFI_BL_LEVEL_NAME,
cfa9f3705 Ronald Tschalär 2019-02-04  1554  	       sizeof(EFI_BL_LEVEL_NAME));
cfa9f3705 Ronald Tschalär 2019-02-04  1555  	efivar_entry->var.VendorGuid = EFI_BL_LEVEL_GUID;
cfa9f3705 Ronald Tschalär 2019-02-04  1556  	efi_data_len = sizeof(efi_data);
cfa9f3705 Ronald Tschalär 2019-02-04  1557  
cfa9f3705 Ronald Tschalär 2019-02-04  1558  	sts = efivar_entry_get(efivar_entry, NULL, &efi_data_len, &efi_data);
cfa9f3705 Ronald Tschalär 2019-02-04  1559  	if (sts && sts != -ENOENT)
cfa9f3705 Ronald Tschalär 2019-02-04  1560  		pr_warn("Error getting backlight level from EFI vars: %d\n",
cfa9f3705 Ronald Tschalär 2019-02-04  1561  			sts);
cfa9f3705 Ronald Tschalär 2019-02-04  1562  
cfa9f3705 Ronald Tschalär 2019-02-04  1563  	kfree(efivar_entry);
cfa9f3705 Ronald Tschalär 2019-02-04  1564  
cfa9f3705 Ronald Tschalär 2019-02-04  1565  	return efi_data;
cfa9f3705 Ronald Tschalär 2019-02-04  1566  }
cfa9f3705 Ronald Tschalär 2019-02-04  1567  

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH 2/2] Input: add Apple SPI keyboard and trackpad driver.

[Life is hard, and then you die]

  Hi Dan,

On Tue, Feb 05, 2019 at 01:21:10PM +0300, Dan Carpenter wrote:
> Hi Ronald,
> 
> Thank you for the patch! Perhaps something to improve:
> 
> url:    https://github.com/0day-ci/linux/commits/Ronald-Tschal-r/drm-bridge-sil_sii8620-depend-on-INPUT-instead-of-selecting-it/20190205-003319
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
> 
> smatch warnings:
> drivers/input/keyboard/applespi.c:1551 applespi_get_saved_bl_level() warn: returning -1 instead of -ENOMEM is sloppy
[snip]
> cfa9f3705 Ronald Tschalär 2019-02-04  1549  	efivar_entry = kmalloc(sizeof(*efivar_entry), GFP_KERNEL);
> cfa9f3705 Ronald Tschalär 2019-02-04  1550  	if (!efivar_entry)
> cfa9f3705 Ronald Tschalär 2019-02-04 @1551  		return -1;

Agreed, that is sloppy. Fixed in for next version. Thanks!


  Cheers,

  Ronald

Re: [PATCH 2/2] Input: add Apple SPI keyboard and trackpad driver.

[Andy Shevchenko]

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.

> +config KEYBOARD_APPLESPI
> +	tristate "Apple SPI keyboard and trackpad"

> +	depends on (X86 && ACPI && SPI) || COMPILE_TEST

COMPILE_TEST more or less makes sense in conjunction with architecture selection.
It means, your code always dependant to ACPI and SPI frameworks.
That's why 0day complained.

> +	imply SPI_PXA2XX
> +	imply SPI_PXA2XX_PCI
> +	imply MFD_INTEL_LPSS_PCI
> +	help
> +	  Say Y here if you are running Linux on any Apple MacBook8,1 or later,
> +	  or any MacBookPro13,* or MacBookPro14,*.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called applespi.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] Input: add Apple SPI keyboard and trackpad driver.

[Life is hard, and then you die]

  Hi Andy,

On Tue, Feb 05, 2019 at 01:45:22PM +0200, Andy Shevchenko wrote:
> 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.
> 
> > +config KEYBOARD_APPLESPI
> > +	tristate "Apple SPI keyboard and trackpad"
> 
> > +	depends on (X86 && ACPI && SPI) || COMPILE_TEST
> 
> COMPILE_TEST more or less makes sense in conjunction with architecture selection.
> It means, your code always dependant to ACPI and SPI frameworks.
> That's why 0day complained.

Thanks. Yes, looking at this again I realized I somewhat misunderstood
the uses of COMPILE_TEST. I've changed this now to

        depends on ACPI && SPI && (X86 || COMPILE_TEST)


  Cheers,

  Ronald

Re: [PATCH 2/2] Input: add Apple SPI keyboard and trackpad driver.

[Andy Shevchenko]

On Tue, Feb 5, 2019 at 4:21 PM Life is hard, and then you die
<ronald@innovation.ch> wrote:

> > > +config KEYBOARD_APPLESPI
> > > +   tristate "Apple SPI keyboard and trackpad"
> >
> > > +   depends on (X86 && ACPI && SPI) || COMPILE_TEST
> >
> > COMPILE_TEST more or less makes sense in conjunction with architecture selection.
> > It means, your code always dependant to ACPI and SPI frameworks.
> > That's why 0day complained.
>
> Thanks. Yes, looking at this again I realized I somewhat misunderstood
> the uses of COMPILE_TEST. I've changed this now to
>
>         depends on ACPI && SPI && (X86 || COMPILE_TEST)

Better to split like

depends on SPI && ACPI
depends on X86 || COMPILE_TEST

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] Input: add Apple SPI keyboard and trackpad driver.

[Andy Shevchenko]

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

Re: [PATCH 2/2] Input: add Apple SPI keyboard and trackpad driver.

[Life is hard, and then you die]

  Hi Andy,

On Wed, Feb 06, 2019 at 10:22:56PM +0200, Andy Shevchenko wrote:
> 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.

Many thanks for your extensive review. Sorry for the delay, managed to
mess up my machine...

I've made most of the changes you suggested - below are my comments
and answers where I have questsions or I think there are good reasons
for the current approach.

> > +/**
> 
> I'm not sure it's kernel doc format comment.

Sorry, you mean you're not sure whether this should be a kernel doc vs
a regular comment? Ok, making it a regular comment.

[snip]
> > +#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.

These have been extremely useful for debugging; but yes, they are for
debugging only. They also explicitly do not use the dynamic-debug
facilities because
  1. those can't be enabled at a function or line level on the kernel
     command line (the docs indicate this should work, but it doesn't,
     or at the very least I've been unable to figure out how, at least
     for those drivers built as modules)
  2. the expressions to enable them quite brittle (in particular the
     line-based ones) since they (may) change any time the code is
     changed - having stable commands to give to users and put in
     documentation (e.g.
     "echo 0x200 > /sys/module/applespi/parameters/debug") is
     quite valuable.
  3. In at least two places we log different types of packets in the
     same lines of code - dynamic-debug would only allow enabling or
     disabling logging of all packets, unless we do something like
     create separate functions for logging each type of packet.

[snip]
> > +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 ?

The physical key is actually labelled "fn" (no uppercase). But will
certainly change it if you still wish.

> Btw, do we need all these parameters? Can't we do modify behaviour run-time
> using some Input framework facilities?

I'm not aware of any way to do so. I suppose the event callback could
be used/extended to send "configuration" data, but that would require
changes to the input core.

Also, for what it's worth, current drivers such as hid-apple (from
which 'fnmode' and 'iso_layout' were copied and intentionally kept the
same) use this approach too.

[snip]
> > +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.

Maybe it's just my grep-foo that is lacking, but I couldn't find
anything useful. There is some stuff for framebuffer video modes, but
that is too different and not really amenable for use here. OTOH, a
simple sscanf(buf, "%dx%d+%d+%d", ...) would parse this just fine
without the need for any fancier parser.

OTOH, I'm not sure this format is any better: internally we need
x/y min/max, not x/y/w/h (though obviously the two are trivially
convertable); and for the user it doesn't really matter since they would
basically be getting the dimensions from running with debug set to
0x10000 and using that output to set this param, i.e. I don't see any
inherent advantage of using x/y/w/h.

> > +/**
> > + * 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.

I used tabs here between the name and description, so it will show up
odd in a diff or where quoted, but it is fine in the original. I've seen
both styles (tabs and just spaces) used in the rest of code, so I'm not
sure what the preferred one is. I'll switch to plain spaces if that's
preferred.

> > + */
> 
> > +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?

Because 'model_id' is not aligned on a 16-bit boundary. That this is a
model id is just a guess (these are the only 2 bytes that differ
between various touchpads, and those two bytes are always the same for
a given touchpad size), and the fact that it's not aligned is
suspicious (since everything else is), so it may actually well be two
separate 8-bit fields. Looking at the known values (0x0417, 0x0557,
and 0x06d7) it very well may be a 1-byte model (0x04, 0x05, 0x06) and
a 1-byte "flags" (0x17, 0x57, 0xd7 - note how each one is a superset
of the previous in terms of bits set).

In short, I can change this to 2 1-byte fields instead and thereby
avoid the need for __packed.

[snip]
> > +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?

You mean change the order to be x_min, y_min, x_max, y_max? (the field
types and semantics already match). Ok.

> > +	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 ?

RD_UNKN signifies an unknown packet type was received; default catches
programming errors where a new log type was added without creating an
entry here (i.e. defensive programmming).

> Perhaps "Unrecognized ... "-ish better?

Sure. I've updated these now to

        case DBG_RD_UNKN:
                return "Unrecognized Event";

        default:
                return "-Unrecognized log mask-";

> > +	}
> 
> > +static inline bool applespi_check_write_status(struct applespi_data *applespi,
> > +					       int sts)
> 
> Indentation broken.

Not in the original - again, an artifact of using tabs for
indentation and the first line being quoted.

[snip]
> > +static int applespi_enable_spi(struct applespi_data *applespi)
> > +{
> > +	int result;
> 
> Sometimes you have ret, sometimes this. Better to unify across the code.

Sorry, that is indeed ugly and lazy - all status/result variables now
have the same name.

[snip]
> > +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?

Multiple of these want_xyz may be "active" simultaneously (e.g. both a
keyboard brightness change and the caps-lock-led change may be
outstanding), as well these not all being simple booleans (want_bl_level
is an actual level value).

The nearest I can think of right now would be to create a bitmap to hold
potential change requests (so e.g. setting a new backlight level would
set both the new wanted level as well a bit indicating that a backlight
level change was requested, though the above check against the current
level would still be needed), and use ffs() to pick a set bit and switch
on the result. In pseudo code:

    applespi_set_bl_level()
        want_bl_level = new-level
        set_bit(BL_CMD, outstanding_cmds)

    applespi_set_capsl_led()
        want_cl_led_on = new-led-on
        set_bit(CL_CMD, outstanding_cmds)

    applespi_send_cmd_msg()
        bool found_cmd = false
        while (!found_cmd) {
            cmd = ffs(outstanding_cmds)
            switch (cmd) {
            case CL_CMD:
                if (applespi->want_cl_led_on != applespi->have_cl_led_on) {
                    found_cmd = true
                    ...
                }
                clear_bit(CL_CMD, outstanding_cmds)
                break

            case BL_CMD:
                if (applespi->want_bl_level != applespi->have_bl_level) {
                    found_cmd = true
                    ...
                }
                clear_bit(BL_CMD, outstanding_cmds)
                break

            ... other commands ...

            default:
                return
            }
        }

Would this be preferrable?

> > +	message->counter = applespi->cmd_msg_cntr++ & 0xff;
> 
> Usual pattern for this kind of entries is
> 
>       x = ... % 256;
> 
> Btw, isn't 256 is defined somewhere?

Many things are defined as have a value of 256 :-) But I didn't find any
with the intended semantics; could use (U8_MAX+1), though.

[snip]
> Btw, consider to use dev_warn() and Co instead of pr_warn() or in cases where
> appropriate acpi_handle_warn(), etc.

I've changed all log calls to use dev_xyz() now, including the
debug_print()'s (for consistency in the resulting log messages).

Regarding acpi_handle_xyz(), though: isn't it better to have all log
messages for a given device use the same logging prefix? I.e. in this
case to use dev_xyz() instead of a mix and match of dev_xyz() and
acpi_handle_xyz()? That makes it easier to grep for all messages related
to a given module/device.

[snip]
> Besides, run checkpatch.pl!

I did/do, with --strict.

[snip]
> > +/* 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 ?

Perhaps as

    static inline int le16_to_signed_int(__le16 x)

? (raw2int seems to ambiguous for a global function)

> > +static void report_tp_state(struct applespi_data *applespi,
> > +			    struct touchpad_protocol *t)
> > +{
[snip]
> > +	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++) {
> 
> ?

n is only incremented for fingers that are down. See below.

> > +		f = &t->fingers[i];
> > +		if (raw2int(f->touch_major) == 0)
> > +			continue;

This here skips incrementing n.

> > +		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++;
> > +

[snip]
> > +static void applespi_remap_fn_key(struct keyboard_protocol
> > +							*keyboard_protocol)
> 
> Better to split like
> 
> static void
> fn(struct ...);

Ah, wasn't aware that was an acceptable variant. Thanks. So I've also
changed a few other places that I think would benefit from this sort
of split:

-static const struct applespi_key_translation *applespi_find_translation(
-		const struct applespi_key_translation *table, u16 key)
+static const struct applespi_key_translation *
+applespi_find_translation(const struct applespi_key_translation *table, u16 key)

and

-static void applespi_handle_keyboard_event(struct applespi_data *applespi,
-					   struct keyboard_protocol
-							*keyboard_protocol)
+static void
+applespi_handle_keyboard_event(struct applespi_data *applespi,
+			       struct keyboard_protocol *keyboard_protocol)

and

-static void applespi_register_touchpad_device(struct applespi_data *applespi,
-				struct touchpad_info_protocol *rcvd_tp_info)
+static int
+applespi_register_touchpad_device(struct applespi_data *applespi,
+				  struct touchpad_info_protocol *rcvd_tp_info)

[snip]
> > +	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) ?

AFAIK that only works for structs, but these are arrays.

[snip]
> > +static void applespi_got_data(struct applespi_data *applespi)
[snip]
> > +		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: ?

This is for both successful and failed messages, so "err" seems wrong.
Renamed it to 'msg_complete:' instead.

[snip]
> > +	/*
> > +	 * 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);
[snip]
> > +static int applespi_remove(struct spi_device *spi)
> > +{
[snip]
> It seems you still have wakeup enabled for the device.

Good catch - disabling on remove now.

[snip]


  Cheers,

  Ronald

Re: [PATCH 2/2] Input: add Apple SPI keyboard and trackpad driver.

[Andy Shevchenko]

On Sun, Feb 10, 2019 at 03:18:34AM -0800, Life is hard, and then you die wrote:
> On Wed, Feb 06, 2019 at 10:22:56PM +0200, Andy Shevchenko wrote:
> > On Mon, Feb 04, 2019 at 12:19:47AM -0800, Ronald Tschalär wrote:

> > > +#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.
> 
> These have been extremely useful for debugging; but yes, they are for
> debugging only. They also explicitly do not use the dynamic-debug
> facilities because
>   1. those can't be enabled at a function or line level on the kernel
>      command line (the docs indicate this should work, but it doesn't,
>      or at the very least I've been unable to figure out how, at least
>      for those drivers built as modules)

Hmm... Usually you put either module_name.dyndbg into kernel command line to
enable Dynamic Debug on built-in modules, or modprobe module_name dyndbg if
it's built as a module.

>   2. the expressions to enable them quite brittle (in particular the
>      line-based ones) since they (may) change any time the code is
>      changed - having stable commands to give to users and put in
>      documentation (e.g.
>      "echo 0x200 > /sys/module/applespi/parameters/debug") is
>      quite valuable.
>   3. In at least two places we log different types of packets in the
>      same lines of code - dynamic-debug would only allow enabling or
>      disabling logging of all packets, unless we do something like
>      create separate functions for logging each type of packet.

> > > +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 ?
> 
> The physical key is actually labelled "fn" (no uppercase). But will
> certainly change it if you still wish.

I think Fn would be suitable (on the computer I'm typing this message the
special keys are all marked in small case, but we don't change HP driver to
state 'ctrl' instead 'Ctrl', for example, do we?).

> > Btw, do we need all these parameters? Can't we do modify behaviour run-time
> > using some Input framework facilities?
> 
> I'm not aware of any way to do so. I suppose the event callback could
> be used/extended to send "configuration" data, but that would require
> changes to the input core.
> 
> Also, for what it's worth, current drivers such as hid-apple (from
> which 'fnmode' and 'iso_layout' were copied and intentionally kept the
> same) use this approach too.

Hmm... Okay, I leave this to Input maintainers.

> > > +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.
> 
> Maybe it's just my grep-foo that is lacking, but I couldn't find
> anything useful. There is some stuff for framebuffer video modes, but
> that is too different and not really amenable for use here. OTOH, a
> simple sscanf(buf, "%dx%d+%d+%d", ...) would parse this just fine
> without the need for any fancier parser.
> 
> OTOH, I'm not sure this format is any better: internally we need
> x/y min/max, not x/y/w/h (though obviously the two are trivially
> convertable); and for the user it doesn't really matter since they would
> basically be getting the dimensions from running with debug set to
> 0x10000 and using that output to set this param, i.e. I don't see any
> inherent advantage of using x/y/w/h.

I would stick with some more or less standard notation. The XxY+W+H is widely
used in X11 world.

If you have better examples for input devices, choose them. My point that it
would be better to be a standard to some extent.

> > > +/**
> > > + * 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.
> 
> I used tabs here between the name and description, so it will show up
> odd in a diff or where quoted, but it is fine in the original. I've seen
> both styles (tabs and just spaces) used in the rest of code, so I'm not
> sure what the preferred one is. I'll switch to plain spaces if that's
> preferred.

Ah, if the result is okay, I'm fine.

> > > + */

> > > +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?
> 
> Because 'model_id' is not aligned on a 16-bit boundary. That this is a
> model id is just a guess (these are the only 2 bytes that differ
> between various touchpads, and those two bytes are always the same for
> a given touchpad size), and the fact that it's not aligned is
> suspicious (since everything else is), so it may actually well be two
> separate 8-bit fields. Looking at the known values (0x0417, 0x0557,
> and 0x06d7) it very well may be a 1-byte model (0x04, 0x05, 0x06) and
> a 1-byte "flags" (0x17, 0x57, 0xd7 - note how each one is a superset
> of the previous in terms of bits set).
> 
> In short, I can change this to 2 1-byte fields instead and thereby
> avoid the need for __packed.

If you feel that it's indeed 16=bit value, better to leave it as is.
But your guess sounds very reasonable to me to split it to something like you
proposed.

> > > +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?
> 
> You mean change the order to be x_min, y_min, x_max, y_max? (the field
> types and semantics already match). Ok.

Yep.

> > > +	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 ?
> 
> RD_UNKN signifies an unknown packet type was received; default catches
> programming errors where a new log type was added without creating an
> entry here (i.e. defensive programmming).

I see.

> > Perhaps "Unrecognized ... "-ish better?
> 
> Sure. I've updated these now to
> 
>         case DBG_RD_UNKN:
>                 return "Unrecognized Event";
> 
>         default:
>                 return "-Unrecognized log mask-";

What I suggested here (in case they are different) is to leave UNKN  message as
is, but change default to use different word.

> > > +	}

> > > +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?
> 
> Multiple of these want_xyz may be "active" simultaneously (e.g. both a
> keyboard brightness change and the caps-lock-led change may be
> outstanding), as well these not all being simple booleans (want_bl_level
> is an actual level value).
> 
> The nearest I can think of right now would be to create a bitmap to hold
> potential change requests (so e.g. setting a new backlight level would
> set both the new wanted level as well a bit indicating that a backlight
> level change was requested, though the above check against the current
> level would still be needed), and use ffs() to pick a set bit and switch
> on the result. In pseudo code:
> 
>     applespi_set_bl_level()
>         want_bl_level = new-level
>         set_bit(BL_CMD, outstanding_cmds)
> 
>     applespi_set_capsl_led()
>         want_cl_led_on = new-led-on
>         set_bit(CL_CMD, outstanding_cmds)
> 
>     applespi_send_cmd_msg()
>         bool found_cmd = false
>         while (!found_cmd) {
>             cmd = ffs(outstanding_cmds)
>             switch (cmd) {
>             case CL_CMD:
>                 if (applespi->want_cl_led_on != applespi->have_cl_led_on) {
>                     found_cmd = true
>                     ...
>                 }
>                 clear_bit(CL_CMD, outstanding_cmds)
>                 break
> 
>             case BL_CMD:
>                 if (applespi->want_bl_level != applespi->have_bl_level) {
>                     found_cmd = true
>                     ...
>                 }
>                 clear_bit(BL_CMD, outstanding_cmds)
>                 break
> 
>             ... other commands ...
> 
>             default:
>                 return
>             }
>         }
> 
> Would this be preferrable?

I don't think it will make code better. Let's leave your initial proposal.

> > > +	message->counter = applespi->cmd_msg_cntr++ & 0xff;
> > 
> > Usual pattern for this kind of entries is
> > 
> >       x = ... % 256;
> > 
> > Btw, isn't 256 is defined somewhere?
> 
> Many things are defined as have a value of 256 :-) But I didn't find any
> with the intended semantics; could use (U8_MAX+1), though.

What I meant is that I saw in the same driver definition with value of 256.
Isn't it about messages?

> > Btw, consider to use dev_warn() and Co instead of pr_warn() or in cases where
> > appropriate acpi_handle_warn(), etc.
> 
> I've changed all log calls to use dev_xyz() now, including the
> debug_print()'s (for consistency in the resulting log messages).
> 
> Regarding acpi_handle_xyz(), though: isn't it better to have all log
> messages for a given device use the same logging prefix? I.e. in this
> case to use dev_xyz() instead of a mix and match of dev_xyz() and
> acpi_handle_xyz()? That makes it easier to grep for all messages related
> to a given module/device.

I'm fine with all being dev_<lvl>(). acpi_handle_<lvl>() makes sense when you
have interaction with ACPI handle and not a device.

> > > +/* 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 ?
> 
> Perhaps as
> 
>     static inline int le16_to_signed_int(__le16 x)
> 
> ? (raw2int seems to ambiguous for a global function)

I'm fine. It would need a description though.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] Input: add Apple SPI keyboard and trackpad driver.

[Life is hard, and then you die]

On Sat, Feb 16, 2019 at 02:44:26AM +0200, Andy Shevchenko wrote:
> On Sun, Feb 10, 2019 at 03:18:34AM -0800, Life is hard, and then you die wrote:
> > On Wed, Feb 06, 2019 at 10:22:56PM +0200, Andy Shevchenko wrote:
> > > On Mon, Feb 04, 2019 at 12:19:47AM -0800, Ronald Tschalär wrote:
> 
> > > > +#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.
> > 
> > These have been extremely useful for debugging; but yes, they are for
> > debugging only. They also explicitly do not use the dynamic-debug
> > facilities because
> >   1. those can't be enabled at a function or line level on the kernel
> >      command line (the docs indicate this should work, but it doesn't,
> >      or at the very least I've been unable to figure out how, at least
> >      for those drivers built as modules)
> 
> Hmm... Usually you put either module_name.dyndbg into kernel command line to
> enable Dynamic Debug on built-in modules, or modprobe module_name dyndbg if
> it's built as a module.

Right, but while that works with

    applespi.dyndbg=+p

it does not appear to work with

    applespi.dyndbg="func applespi_get_spi_settings +p"

(it is parsed correctly, but it just doesn't get applied when the
module is later loaded - I've not tried to chase this down any further
than that)

[snip]
> > > > +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.
> > 
> > Maybe it's just my grep-foo that is lacking, but I couldn't find
> > anything useful. There is some stuff for framebuffer video modes, but
> > that is too different and not really amenable for use here. OTOH, a
> > simple sscanf(buf, "%dx%d+%d+%d", ...) would parse this just fine
> > without the need for any fancier parser.
> > 
> > OTOH, I'm not sure this format is any better: internally we need
> > x/y min/max, not x/y/w/h (though obviously the two are trivially
> > convertable); and for the user it doesn't really matter since they would
> > basically be getting the dimensions from running with debug set to
> > 0x10000 and using that output to set this param, i.e. I don't see any
> > inherent advantage of using x/y/w/h.
> 
> I would stick with some more or less standard notation. The XxY+W+H is widely
> used in X11 world.
> 
> If you have better examples for input devices, choose them. My point that it
> would be better to be a standard to some extent.

I couldn't find anything useful (looked in the kernel and libinput),
so going with XxY+W+H.

[snip]
> > > > +	message->counter = applespi->cmd_msg_cntr++ & 0xff;
> > > 
> > > Usual pattern for this kind of entries is
> > > 
> > >       x = ... % 256;
> > > 
> > > Btw, isn't 256 is defined somewhere?
> > 
> > Many things are defined as have a value of 256 :-) But I didn't find any
> > with the intended semantics; could use (U8_MAX+1), though.
> 
> What I meant is that I saw in the same driver definition with value of 256.
> Isn't it about messages?

Ah, right: the packet length is 256 bytes. But the semantics are quite
different, so IMHO it wouldn't be good to use that here. I.e. I think
(U8_MAX+1) is still semantically the best.

[snip]
> > > > +/* 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 ?
> > 
> > Perhaps as
> > 
> >     static inline int le16_to_signed_int(__le16 x)
> > 
> > ? (raw2int seems to ambiguous for a global function)
> 
> I'm fine. It would need a description though.

After looking at this in more detail I don't think that
include/linux/input.h is the proper place for this, because input.h
basically represents the interface to the input core, and as such it
is devoid of helpers like above or any driver specific definitions.
Instead I'd like to suggest a new include file specific to the bcm5974
driver, as follows:

--- /dev/null
+++ b/include/linux/input/bcm5974.h
@@ -0,0 +1,27 @@
+/*
+ * Copyright (C) 2008     Henrik Rydberg (rydberg@euromail.se)
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef _BCM5974_H
+#define _BCM5974_H
+
+#include <linux/kernel.h>
+
+/**
+ * raw2int - convert a 16-bit little endian value as received from the device
+ * to a signed integer in cpu endianness.
+ * @x: the received value
+ *
+ * Returns the converted value.
+ */
+static inline int raw2int(__le16 x)
+{
+       return (signed short)le16_to_cpu(x);
+}
+
+#endif

Thoughts?

I'll send out v2 of the patches with all the changes soon.


  Cheers,

  Ronald

Re: [PATCH 2/2] Input: add Apple SPI keyboard and trackpad driver.

[Andy Shevchenko]

On Mon, Feb 18, 2019 at 01:08:51AM -0800, Life is hard, and then you die wrote:
> On Sat, Feb 16, 2019 at 02:44:26AM +0200, Andy Shevchenko wrote:
> > On Sun, Feb 10, 2019 at 03:18:34AM -0800, Life is hard, and then you die wrote:
> > > On Wed, Feb 06, 2019 at 10:22:56PM +0200, Andy Shevchenko wrote:
> > > > On Mon, Feb 04, 2019 at 12:19:47AM -0800, Ronald Tschalär wrote:

> > Hmm... Usually you put either module_name.dyndbg into kernel command line to
> > enable Dynamic Debug on built-in modules, or modprobe module_name dyndbg if
> > it's built as a module.
> 
> Right, but while that works with
> 
>     applespi.dyndbg=+p
> 
> it does not appear to work with
> 
>     applespi.dyndbg="func applespi_get_spi_settings +p"
> 
> (it is parsed correctly, but it just doesn't get applied when the
> module is later loaded - I've not tried to chase this down any further
> than that)

Of course it wouldn't work that way. If you have compiled driver as a module,
you need to supply the parameters during modprobe. Actually it's kinda luck
that +p works for you.

> > > > > +	message->counter = applespi->cmd_msg_cntr++ & 0xff;
> > > > 
> > > > Usual pattern for this kind of entries is
> > > > 
> > > >       x = ... % 256;
> > > > 
> > > > Btw, isn't 256 is defined somewhere?
> > > 
> > > Many things are defined as have a value of 256 :-) But I didn't find any
> > > with the intended semantics; could use (U8_MAX+1), though.
> > 
> > What I meant is that I saw in the same driver definition with value of 256.
> > Isn't it about messages?
> 
> Ah, right: the packet length is 256 bytes. But the semantics are quite
> different, so IMHO it wouldn't be good to use that here. I.e. I think
> (U8_MAX+1) is still semantically the best.

OK.

> > > > > +/* 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 ?
> > > 
> > > Perhaps as
> > > 
> > >     static inline int le16_to_signed_int(__le16 x)
> > > 
> > > ? (raw2int seems to ambiguous for a global function)
> > 
> > I'm fine. It would need a description though.
> 
> After looking at this in more detail I don't think that
> include/linux/input.h is the proper place for this, because input.h
> basically represents the interface to the input core, and as such it
> is devoid of helpers like above or any driver specific definitions.
> Instead I'd like to suggest a new include file specific to the bcm5974
> driver, as follows:
> 
> --- /dev/null
> +++ b/include/linux/input/bcm5974.h
> @@ -0,0 +1,27 @@
> +/*
> + * Copyright (C) 2008     Henrik Rydberg (rydberg@euromail.se)
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#ifndef _BCM5974_H
> +#define _BCM5974_H
> +
> +#include <linux/kernel.h>
> +
> +/**
> + * raw2int - convert a 16-bit little endian value as received from the device
> + * to a signed integer in cpu endianness.
> + * @x: the received value
> + *
> + * Returns the converted value.
> + */
> +static inline int raw2int(__le16 x)
> +{
> +       return (signed short)le16_to_cpu(x);
> +}
> +
> +#endif
> 
> Thoughts?

I see. Since there is no comment from input maintainers, let's stick with the
initial approach (copy'n'paste to your code with comment from where). Only one
suggestion is to name function for further use without changing it. So, in the
future we may move it to generic header.

> I'll send out v2 of the patches with all the changes soon.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/2] Add Apple SPI keyboard and trackpad driver

[Henrik Rydberg]

Hi Ronald,

> This changeset adds a driver for the SPI keyboard and trackpad on recent
> MacBook's and MacBook Pro's. The driver has seen a fair amount of use
> over the last 2 years (basically anybody running linux on these
> machines), with only relatively small changes in the last year or so.
> For those interested, the driver development has been hosted at
> https://github.com/cb22/macbook12-spi-driver/  (as well as my clone at
> https://github.com/roadrunner2/macbook12-spi-driver/).
>
> The first patch is just a placeholder for now and is provided in case
> somebody wants to compile the driver while it's being reviewed here; the
> real patch has been submitted to dri-devel and is being discussed there,
> with the intent/hope that I can get an Ack and permission to merge it
> through the input subsystem tree here as part of this patch series.

Great to see this upstream. The patch obviously has a lot in common with 
the previous keyboard and touchpad drivers; foremost this is a change of 
transport protocol and not functionality. That said, the code is compact 
and clear enough to make it hard to motivate any major effort to share 
more of existing code, at least initially. Barring detailed comments 
that are likely to produce new revisions, this looks like really good work.

Thanks,

Henrik

Re: [PATCH 0/2] Add Apple SPI keyboard and trackpad driver

[Life is hard, and then you die]

  Hi Henrik,

On Mon, Feb 04, 2019 at 09:47:55PM +0100, Henrik Rydberg wrote:
> Hi Ronald,
> 
> > This changeset adds a driver for the SPI keyboard and trackpad on recent
> > MacBook's and MacBook Pro's. The driver has seen a fair amount of use
> > over the last 2 years (basically anybody running linux on these
> > machines), with only relatively small changes in the last year or so.
> > For those interested, the driver development has been hosted at
> > https://github.com/cb22/macbook12-spi-driver/  (as well as my clone at
> > https://github.com/roadrunner2/macbook12-spi-driver/).
> > 
> > The first patch is just a placeholder for now and is provided in case
> > somebody wants to compile the driver while it's being reviewed here; the
> > real patch has been submitted to dri-devel and is being discussed there,
> > with the intent/hope that I can get an Ack and permission to merge it
> > through the input subsystem tree here as part of this patch series.
> 
> Great to see this upstream. The patch obviously has a lot in common with the
> previous keyboard and touchpad drivers; foremost this is a change of
> transport protocol and not functionality. That said, the code is compact and
> clear enough to make it hard to motivate any major effort to share more of
> existing code, at least initially.

Yes, some pieces have been copy-pasted from the existing drivers.
However, when I last reviewed those pieces they seemed a bit small and
I had a hard time seeing how to share them usefully at least for some
of it.

The pieces in question on the keyboard side (from the hid-apple
driver) are really the 'applespi_fn_codes' and 'apple_iso_keyboard'
tables, the corresponding 'applespi_find_translation()' function, and
some bits in the of the 'applespi_code_to_key()' function. Pulling out
the tables and maybe the applespi_find_translation() function into a
common include might be a simple change and take care of most of the
shared stuff.

A few lines were also lifted from the bcm5974 driver, basically the
'struct tp_finger' and the 'report_tp_state()' function. Though here
it's even harder to see how to share, as there are various small
differences scattered throughout the implemenation of that function.

> Barring detailed comments that are likely
> to produce new revisions, this looks like really good work.

Thank you for looking at this.


  Cheers,

  Ronald