LinuxLists.cc - [PATCH 0/2] Add Apple SPI keyboard and trackpad driver

2019-02-04 08:20:26

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

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

2019-02-04 08:20:35

by Life is hard, and then you die

[permalink] [raw]

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

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 <[email protected]>
CC: Andrzej Hajda <[email protected]>
Signed-off-by: Ronald Tschalär <[email protected]>
---
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

2019-02-04 08:20:48

by Life is hard, and then you die

[permalink] [raw]

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

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 <[email protected]>
CC: Lukas Wunner <[email protected]>
CC: Andy Shevchenko <[email protected]>
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 <[email protected]>
---
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

2019-02-04 18:26:59

by kernel test robot

[permalink] [raw]

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

Attachments:

(No filename) (5.32 kB)
.config.gz (49.25 kB)
Download all attachments

2019-02-04 18:46:07

by kernel test robot

[permalink] [raw]

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

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 <[email protected]>
:::::: CC: Mark Brown <[email protected]>

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

Attachments:

(No filename) (10.86 kB)
.config.gz (49.27 kB)
Download all attachments

2019-02-04 21:51:09

by Henrik Rydberg

[permalink] [raw]

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

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

2019-02-05 10:31:13

by Dan Carpenter

[permalink] [raw]

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

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

2019-02-05 11:45:46

by Andy Shevchenko

[permalink] [raw]

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

2019-02-05 13:34:11

by Life is hard, and then you die

[permalink] [raw]

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

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

2019-02-05 13:35:11

by Life is hard, and then you die

[permalink] [raw]

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

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

2019-02-05 13:38:33

by Life is hard, and then you die

[permalink] [raw]

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

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

2019-02-05 15:34:00

by Andy Shevchenko

[permalink] [raw]

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

On Tue, Feb 5, 2019 at 4:21 PM Life is hard, and then you die
<[email protected]> 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

2019-02-06 20:23:36

by Andy Shevchenko

[permalink] [raw]

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

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

2019-02-10 11:20:48

by Life is hard, and then you die

[permalink] [raw]

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

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

2019-02-16 10:03:35

by Andy Shevchenko

[permalink] [raw]

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

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

2019-02-18 09:09:43

by Life is hard, and then you die

[permalink] [raw]

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

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 ([email protected])
+ *
+ * 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

2019-02-18 12:28:55

by Andy Shevchenko

[permalink] [raw]

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

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 ([email protected])
> + *
> + * 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