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

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

[Ronald Tschalär]

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

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

The second and third patches add a new dev_print_hex_dump() helper as
the dev_xxx() analog of print_hex_dump().

The fourth patch finally contains the new applespi driver.

Changes in v3:
  Applied all feedback from review by Andy Shevchenko, including:
  - move dev_print_hex_dump() to driver core
  - clean up keyboard modifier bits testing/modifying
  - remove DEV() macro
  - minor style issues
  The full set of changes to applespi can be viewed at
  https://github.com/roadrunner2/macbook12-spi-driver/ as individual
  commits f832caa..3a6262e in the upstreaming-review branch.

Ronald Tschalär (4):
  drm/bridge: sil_sii8620: depend on INPUT instead of selecting it.
  lib/hexdump.c: factor out generic hexdump formatting for reuse.
  driver core: add dev_print_hex_dump() logging function.
  Input: add Apple SPI keyboard and trackpad driver.

 drivers/base/core.c               |   43 +
 drivers/gpu/drm/bridge/Kconfig    |    2 +-
 drivers/input/keyboard/Kconfig    |   15 +
 drivers/input/keyboard/Makefile   |    1 +
 drivers/input/keyboard/applespi.c | 1988 +++++++++++++++++++++++++++++
 include/linux/device.h            |   15 +
 include/linux/printk.h            |   12 +
 lib/hexdump.c                     |   95 +-
 8 files changed, 2146 insertions(+), 25 deletions(-)
 create mode 100644 drivers/input/keyboard/applespi.c

-- 
2.20.1

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

[Ronald Tschalär]

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

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

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

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

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

[PATCH v3 2/4] lib/hexdump.c: factor out generic hexdump formatting for reuse.

[Ronald Tschalär]

This introduces print_hex_dump_to_cb() which contains all the hexdump
formatting minus the actual printk() call, allowing an arbitrary print
function to be supplied instead. And print_hex_dump() is re-implemented
using print_hex_dump_to_cb().

This allows other hex-dump logging functions to be provided which call
printk() differently or even log the hexdump somewhere entirely
different.
---
 include/linux/printk.h | 12 ++++++
 lib/hexdump.c          | 95 +++++++++++++++++++++++++++++++-----------
 2 files changed, 83 insertions(+), 24 deletions(-)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index 77740a506ebb..4ebdacd7a287 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -483,10 +483,16 @@ enum {
 extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
 			      int groupsize, char *linebuf, size_t linebuflen,
 			      bool ascii);
+typedef
+void (*hex_dump_callback)(const char *level, void *arg, const char *fmt, ...);
 #ifdef CONFIG_PRINTK
 extern void print_hex_dump(const char *level, const char *prefix_str,
 			   int prefix_type, int rowsize, int groupsize,
 			   const void *buf, size_t len, bool ascii);
+extern void print_hex_dump_to_cb(const char *level, const char *prefix_str,
+				 int prefix_type, int rowsize, int groupsize,
+				 const void *buf, size_t len, bool ascii,
+				 hex_dump_callback print, void *print_arg);
 #if defined(CONFIG_DYNAMIC_DEBUG)
 #define print_hex_dump_bytes(prefix_str, prefix_type, buf, len)	\
 	dynamic_hex_dump(prefix_str, prefix_type, 16, 1, buf, len, true)
@@ -500,6 +506,12 @@ static inline void print_hex_dump(const char *level, const char *prefix_str,
 				  const void *buf, size_t len, bool ascii)
 {
 }
+extern void print_hex_dump_to_cb(const char *level, const char *prefix_str,
+				 int prefix_type, int rowsize, int groupsize,
+				 const void *buf, size_t len, bool ascii,
+				 hex_dump_callback *print, void *print_arg);
+{
+}
 static inline void print_hex_dump_bytes(const char *prefix_str, int prefix_type,
 					const void *buf, size_t len)
 {
diff --git a/lib/hexdump.c b/lib/hexdump.c
index 81b70ed37209..43583cf6accd 100644
--- a/lib/hexdump.c
+++ b/lib/hexdump.c
@@ -210,7 +210,8 @@ EXPORT_SYMBOL(hex_dump_to_buffer);
 
 #ifdef CONFIG_PRINTK
 /**
- * print_hex_dump - print a text hex dump to syslog for a binary blob of data
+ * print_hex_dump_to_cb - print a text hex dump using given callback for a
+ * binary blob of data
  * @level: kernel log level (e.g. KERN_DEBUG)
  * @prefix_str: string to prefix each line with;
  *  caller supplies trailing spaces for alignment if desired
@@ -221,28 +222,18 @@ EXPORT_SYMBOL(hex_dump_to_buffer);
  * @buf: data blob to dump
  * @len: number of bytes in the @buf
  * @ascii: include ASCII after the hex output
+ * @print: the print function, called once for each line
+ * @print_arg: an arbitrary argument to pass to the print function
  *
- * Given a buffer of u8 data, print_hex_dump() prints a hex + ASCII dump
- * to the kernel log at the specified kernel log level, with an optional
- * leading prefix.
- *
- * print_hex_dump() works on one "line" of output at a time, i.e.,
- * 16 or 32 bytes of input data converted to hex + ASCII output.
- * print_hex_dump() iterates over the entire input @buf, breaking it into
- * "line size" chunks to format and print.
- *
- * E.g.:
- *   print_hex_dump(KERN_DEBUG, "raw data: ", DUMP_PREFIX_ADDRESS,
- *		    16, 1, frame->data, frame->len, true);
+ * This is a low level helper function - normally you want to use
+ * print_hex_dump() or other wrapper around it.
  *
- * Example output using %DUMP_PREFIX_OFFSET and 1-byte mode:
- * 0009ab42: 40 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e 4f  @ABCDEFGHIJKLMNO
- * Example output using %DUMP_PREFIX_ADDRESS and 4-byte mode:
- * ffffffff88089af0: 73727170 77767574 7b7a7978 7f7e7d7c  pqrstuvwxyz{|}~.
+ * See print_hex_dump() for more details and examples.
  */
-void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
-		    int rowsize, int groupsize,
-		    const void *buf, size_t len, bool ascii)
+void print_hex_dump_to_cb(const char *level, const char *prefix_str,
+			  int prefix_type, int rowsize, int groupsize,
+			  const void *buf, size_t len, bool ascii,
+			  hex_dump_callback print, void *print_arg)
 {
 	const u8 *ptr = buf;
 	int i, linelen, remaining = len;
@@ -260,18 +251,74 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
 
 		switch (prefix_type) {
 		case DUMP_PREFIX_ADDRESS:
-			printk("%s%s%p: %s\n",
-			       level, prefix_str, ptr + i, linebuf);
+			print(level, print_arg, "%s%p: %s\n", prefix_str,
+			      ptr + i, linebuf);
 			break;
 		case DUMP_PREFIX_OFFSET:
-			printk("%s%s%.8x: %s\n", level, prefix_str, i, linebuf);
+			print(level, print_arg, "%s%.8x: %s\n", prefix_str, i,
+			      linebuf);
 			break;
 		default:
-			printk("%s%s%s\n", level, prefix_str, linebuf);
+			print(level, print_arg, "%s%s\n", prefix_str, linebuf);
 			break;
 		}
 	}
 }
+EXPORT_SYMBOL(print_hex_dump_to_cb);
+
+static void print_to_printk(const char *level, void *arg, const char *fmt, ...)
+{
+	va_list args;
+	struct va_format vaf;
+
+	va_start(args, fmt);
+
+	vaf.fmt = fmt;
+	vaf.va = &args;
+
+	printk("%s%pV", level, &vaf);
+
+	va_end(args);
+}
+
+/**
+ * print_hex_dump - print a text hex dump to syslog for a binary blob of data
+ * @level: kernel log level (e.g. KERN_DEBUG)
+ * @prefix_str: string to prefix each line with;
+ *  caller supplies trailing spaces for alignment if desired
+ * @prefix_type: controls whether prefix of an offset, address, or none
+ *  is printed (%DUMP_PREFIX_OFFSET, %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_NONE)
+ * @rowsize: number of bytes to print per line; must be 16 or 32
+ * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
+ * @buf: data blob to dump
+ * @len: number of bytes in the @buf
+ * @ascii: include ASCII after the hex output
+ *
+ * Given a buffer of u8 data, print_hex_dump() prints a hex + ASCII dump
+ * to the kernel log at the specified kernel log level, with an optional
+ * leading prefix.
+ *
+ * print_hex_dump() works on one "line" of output at a time, i.e.,
+ * 16 or 32 bytes of input data converted to hex + ASCII output.
+ * print_hex_dump() iterates over the entire input @buf, breaking it into
+ * "line size" chunks to format and print.
+ *
+ * E.g.:
+ *   print_hex_dump(KERN_DEBUG, "raw data: ", DUMP_PREFIX_ADDRESS,
+ *		    16, 1, frame->data, frame->len, true);
+ *
+ * Example output using %DUMP_PREFIX_OFFSET and 1-byte mode:
+ * 0009ab42: 40 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e 4f  @ABCDEFGHIJKLMNO
+ * Example output using %DUMP_PREFIX_ADDRESS and 4-byte mode:
+ * ffffffff88089af0: 73727170 77767574 7b7a7978 7f7e7d7c  pqrstuvwxyz{|}~.
+ */
+void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
+		    int rowsize, int groupsize,
+		    const void *buf, size_t len, bool ascii)
+{
+	print_hex_dump_to_cb(level, prefix_str, prefix_type, rowsize, groupsize,
+			     buf, len, ascii, print_to_printk, NULL);
+}
 EXPORT_SYMBOL(print_hex_dump);
 
 #if !defined(CONFIG_DYNAMIC_DEBUG)
-- 
2.20.1

[PATCH v3 3/4] driver core: add dev_print_hex_dump() logging function.

[Ronald Tschalär]

This is the dev_xxx() analog to print_hex_dump(), using dev_printk()
instead of straight printk() to match other dev_xxx() logging functions.
---
 drivers/base/core.c    | 43 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/device.h | 15 +++++++++++++++
 2 files changed, 58 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 0073b09bb99f..eb260d482750 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3097,6 +3097,49 @@ define_dev_printk_level(_dev_warn, KERN_WARNING);
 define_dev_printk_level(_dev_notice, KERN_NOTICE);
 define_dev_printk_level(_dev_info, KERN_INFO);
 
+static void
+print_to_dev_printk(const char *level, void *arg, const char *fmt, ...)
+{
+	struct va_format vaf;
+	va_list args;
+
+	va_start(args, fmt);
+
+	vaf.fmt = fmt;
+	vaf.va = &args;
+
+	__dev_printk(level, (const struct device *)arg, &vaf);
+
+	va_end(args);
+}
+
+/**
+ * _dev_print_hex_dump - print a text hex dump to syslog for a binary blob of
+ * data
+ * @level: kernel log level (e.g. KERN_DEBUG)
+ * @dev: the device to which this log message pertains
+ * @prefix_str: string to prefix each line with;
+ *  caller supplies trailing spaces for alignment if desired
+ * @prefix_type: controls whether prefix of an offset, address, or none
+ *  is printed (%DUMP_PREFIX_OFFSET, %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_NONE)
+ * @rowsize: number of bytes to print per line; must be 16 or 32
+ * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
+ * @buf: data blob to dump
+ * @len: number of bytes in the @buf
+ * @ascii: include ASCII after the hex output
+ *
+ * See print_hex_dump() for additional details and examples.
+ */
+void _dev_print_hex_dump(const char *level, const struct device *dev,
+			 const char *prefix_str, int prefix_type,
+			 int rowsize, int groupsize,
+			 const void *buf, size_t len, bool ascii)
+{
+	print_hex_dump_to_cb(level, prefix_str, prefix_type, rowsize, groupsize,
+			     buf, len, ascii, print_to_dev_printk, (void *)dev);
+}
+EXPORT_SYMBOL(_dev_print_hex_dump);
+
 #endif
 
 static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
diff --git a/include/linux/device.h b/include/linux/device.h
index 6cb4640b6160..dc6fcae3002a 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1405,6 +1405,10 @@ __printf(2, 3)
 void _dev_notice(const struct device *dev, const char *fmt, ...);
 __printf(2, 3)
 void _dev_info(const struct device *dev, const char *fmt, ...);
+void _dev_print_hex_dump(const char *level, const struct device *dev,
+			 const char *prefix_str, int prefix_type,
+			 int rowsize, int groupsize,
+			 const void *buf, size_t len, bool ascii);
 
 #else
 
@@ -1445,6 +1449,12 @@ void _dev_notice(const struct device *dev, const char *fmt, ...)
 static inline __printf(2, 3)
 void _dev_info(const struct device *dev, const char *fmt, ...)
 {}
+static inline
+void _dev_print_hex_dump(const char *level, const struct device *dev,
+			 const char *prefix_str, int prefix_type,
+			 int rowsize, int groupsize,
+			 const void *buf, size_t len, bool ascii);
+{}
 
 #endif
 
@@ -1467,6 +1477,11 @@ void _dev_info(const struct device *dev, const char *fmt, ...)
 	_dev_notice(dev, dev_fmt(fmt), ##__VA_ARGS__)
 #define dev_info(dev, fmt, ...)						\
 	_dev_info(dev, dev_fmt(fmt), ##__VA_ARGS__)
+#define dev_print_hex_dump(level, dev, prefix_str, prefix_type,		\
+			   rowsize, groupsize, buf, len, ascii)		\
+	_dev_print_hex_dump(level, dev, dev_fmt(prefix_str),		\
+			    prefix_type, rowsize, groupsize, buf, len,	\
+			    ascii);
 
 #if defined(CONFIG_DYNAMIC_DEBUG)
 #define dev_dbg(dev, fmt, ...)						\
-- 
2.20.1

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

[Ronald Tschalär]

The keyboard and trackpad on recent MacBook's (since 8,1) and
MacBookPro's (13,* and 14,*) are attached to an SPI controller instead
of USB, as previously. The higher level protocol is not publicly
documented and hence has been reverse engineered. As a consequence there
are still a number of unknown fields and commands. However, the known
parts have been working well and received extensive testing and use.

In order for this driver to work, the proper SPI drivers need to be
loaded too; for MB8,1 these are spi_pxa2xx_platform and spi_pxa2xx_pci;
for all others they are spi_pxa2xx_platform and intel_lpss_pci. For this
reason enabling this driver in the config implies enabling the above
drivers.

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

diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index a878351f1643..d0a9e7fa2508 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -70,6 +70,21 @@ config KEYBOARD_AMIGA
 config ATARI_KBD_CORE
 	bool
 
+config KEYBOARD_APPLESPI
+	tristate "Apple SPI keyboard and trackpad"
+	depends on ACPI && EFI
+	depends on SPI
+	depends on X86 || 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..39e38d98869e
--- /dev/null
+++ b/drivers/input/keyboard/applespi.c
@@ -0,0 +1,1988 @@
+// 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.
+ */
+
+#include <linux/acpi.h>
+#include <linux/crc16.h>
+#include <linux/delay.h>
+#include <linux/efi.h>
+#include <linux/input.h>
+#include <linux/input/mt.h>
+#include <linux/ktime.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/spinlock.h>
+#include <linux/spi/spi.h>
+#include <linux/wait.h>
+#include <linux/workqueue.h>
+
+#include <asm/barrier.h>
+#include <asm-generic/unaligned.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_FINGERS		11
+#define MAX_FINGER_ORIENTATION	16384
+#define MAX_PKTS_PER_MSG	2
+
+#define KBD_BL_LEVEL_MIN	32U
+#define KBD_BL_LEVEL_MAX	255U
+#define KBD_BL_LEVEL_SCALE	1000000U
+#define KBD_BL_LEVEL_ADJ	\
+	((KBD_BL_LEVEL_MAX - KBD_BL_LEVEL_MIN) * KBD_BL_LEVEL_SCALE / 255U)
+
+#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, applespi, fmt, ...) \
+	do { \
+		if (debug & (mask)) \
+			dev_printk(KERN_DEBUG, &(applespi)->spi->dev, fmt, \
+				   ##__VA_ARGS__); \
+	} while (0)
+
+#define	debug_print_header(mask, applespi) \
+	debug_print(mask, applespi, "--- %s ---------------------------\n", \
+		    applespi_debug_facility(mask))
+
+#define	debug_print_buffer(mask, applespi, fmt, buf, len) \
+	do { \
+		if (debug & (mask)) \
+			dev_print_hex_dump(KERN_DEBUG, &(applespi)->spi->dev, \
+					   fmt, DUMP_PREFIX_NONE, 32, 1, buf, \
+					   len, false); \
+	} while (0)
+
+#define APPLE_FLAG_FKEY		0x01
+
+#define SPI_RW_CHG_DELAY_US	100	/* from experimentation, in µs */
+
+#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 bool iso_layout;
+module_param(iso_layout, bool, 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 char touchpad_dimensions[40];
+module_param_string(touchpad_dimensions, touchpad_dimensions,
+		    sizeof(touchpad_dimensions), 0444);
+MODULE_PARM_DESC(touchpad_dimensions, "The pixel dimensions of the touchpad, as XxY+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
+ * @crc16:		crc over the whole message struct (message header +
+ *			this struct) minus this @crc16 field
+ */
+struct keyboard_protocol {
+	__u8			unknown1;
+	__u8			modifiers;
+	__u8			unknown2;
+	__u8			keys_pressed[MAX_ROLLOVER];
+	__u8			fn_pressed;
+	__le16			crc16;
+};
+
+/**
+ * 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
+ * @crc16:		on last finger: crc over the whole message struct
+ *			(i.e. message header + this struct) minus the last
+ *			@crc16 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 crc16;
+};
+
+/**
+ * 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
+ *
+ * @crc16:		crc over the whole message struct (message header +
+ *			this struct) minus this @crc16 field
+ */
+struct command_protocol_tp_info {
+	__le16			crc16;
+};
+
+/**
+ * struct touchpad_info - touchpad info response.
+ * message.type = 0x1020, message.length = 0x006e
+ *
+ * @unknown1:		unknown
+ * @model_flags:	flags (vary by model number, but significance otherwise
+ *			unknown)
+ * @model_no:		the touchpad model number
+ * @unknown2:		unknown
+ * @crc16:		crc over the whole message struct (message header +
+ *			this struct) minus this @crc16 field
+ */
+struct touchpad_info_protocol {
+	__u8			unknown1[105];
+	__u8			model_flags;
+	__u8			model_no;
+	__u8			unknown2[3];
+	__le16			crc16;
+};
+
+/**
+ * struct command_protocol_mt_init - initialize multitouch.
+ * message.type = 0x0252, message.length = 0x0002
+ *
+ * @cmd:		value: 0x0102
+ * @crc16:		crc over the whole message struct (message header +
+ *			this struct) minus this @crc16 field
+ */
+struct command_protocol_mt_init {
+	__le16			cmd;
+	__le16			crc16;
+};
+
+/**
+ * struct command_protocol_capsl - toggle caps-lock led
+ * message.type = 0x0151, message.length = 0x0002
+ *
+ * @unknown:		value: 0x01 (length?)
+ * @led:		0 off, 2 on
+ * @crc16:		crc over the whole message struct (message header +
+ *			this struct) minus this @crc16 field
+ */
+struct command_protocol_capsl {
+	__u8			unknown;
+	__u8			led;
+	__le16			crc16;
+};
+
+/**
+ * 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)
+ * @crc16:		crc over the whole message struct (message header +
+ *			this struct) minus this @crc16 field
+ */
+struct command_protocol_bl {
+	__le16			const1;
+	__le16			level;
+	__le16			const2;
+	__le16			crc16;
+};
+
+/**
+ * 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
+ * @crc16:	crc over this whole structure minus this @crc16 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			crc16;
+};
+
+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) */
+};
+
+/* this mimics struct drm_rect */
+struct applespi_tp_info {
+	int	x_min;
+	int	y_min;
+	int	x_max;
+	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
+};
+
+/*
+ * This must have exactly as many entries as there are bits in
+ * struct keyboard_protocol.modifiers .
+ */
+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 = 0x04,	/* MB8 MB9 MB10 */
+		.tp_info = { -5087, -182, 5579, 6089 },
+	},
+	{
+		.model = 0x05,	/* MBP13,1 MBP13,2 MBP14,1 MBP14,2 */
+		.tp_info = { -6243, -170, 6749, 7685 },
+	},
+	{
+		.model = 0x06,	/* MBP13,3 MBP14,3 */
+		.tp_info = { -7456, -163, 7976, 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 "-Unrecognized log mask-";
+	}
+}
+
+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_DELAY_US;
+	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_DELAY_US;
+
+	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 status_ok[] = { 0xac, 0x27, 0x68, 0xd5 };
+
+	if (sts < 0) {
+		dev_warn(&(applespi)->spi->dev, "Error writing to device: %d\n",
+			 sts);
+		return false;
+	}
+
+	if (memcmp(applespi->tx_status, status_ok, APPLESPI_STATUS_SIZE)) {
+		dev_warn(&(applespi)->spi->dev,
+			 "Error writing to device: %*ph\n",
+			 APPLESPI_STATUS_SIZE, applespi->tx_status);
+		return false;
+	}
+
+	return true;
+}
+
+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
+		dev_warn(&(applespi)->spi->dev,
+			 "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
+		dev_warn(&(applespi)->spi->dev,
+			 "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
+		dev_warn(&(applespi)->spi->dev,
+			 "Property resetRecUsec not found\n");
+
+	dev_dbg(&(applespi)->spi->dev,
+		"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)
+{
+	acpi_status acpi_sts;
+	unsigned long long spi_status;
+
+	/* check if SPI is already enabled, so we can skip the delay below */
+	acpi_sts = acpi_evaluate_integer(applespi->sist, NULL, NULL,
+					 &spi_status);
+	if (ACPI_SUCCESS(acpi_sts) && spi_status)
+		return 0;
+
+	/* SIEN(1) will enable SPI communication */
+	acpi_sts = acpi_execute_simple_method(applespi->sien, NULL, 1);
+	if (ACPI_FAILURE(acpi_sts)) {
+		dev_err(&(applespi)->spi->dev, "SIEN failed: %s\n",
+			acpi_format_exception(acpi_sts));
+		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, applespi);
+	debug_print_buffer(applespi->cmd_log_mask, applespi, "write  ",
+			   applespi->tx_buffer, APPLESPI_PACKET_SIZE);
+	debug_print_buffer(applespi->cmd_log_mask, applespi, "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++ % (U8_MAX + 1);
+
+	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);
+	put_unaligned_le16(crc, &message->data[msg_len - 2]);
+
+	crc = crc16(0, (u8 *)packet, sizeof(*packet) - 2);
+	packet->crc16 = cpu_to_le16(crc);
+
+	/* send command */
+	sts = applespi_async(applespi, &applespi->wr_m,
+			     applespi_async_write_complete);
+	if (sts) {
+		dev_warn(&(applespi)->spi->dev,
+			 "Error queueing async write to device: %d\n", sts);
+		return sts;
+	}
+
+	applespi->cmd_msg_queued = true;
+	applespi->write_active = true;
+
+	return 0;
+}
+
+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_mt_init_cmd = true;
+	else
+		applespi->want_tp_info_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 =
+			((value * KBD_BL_LEVEL_ADJ) / KBD_BL_LEVEL_SCALE +
+			 KBD_BL_LEVEL_MIN);
+	}
+
+	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 -EINVAL;
+}
+
+/* lifted from the BCM5974 driver and renamed from raw2int */
+/* convert 16-bit little endian to signed integer */
+static inline int le16_to_int(__le16 x)
+{
+	return (signed short)le16_to_cpu(x);
+}
+
+static int applespi_dbg_dim_min_x;
+static int applespi_dbg_dim_max_x;
+static int applespi_dbg_dim_min_y;
+static int applespi_dbg_dim_max_y;
+static bool applespi_dbg_dim_updated;
+
+static void applespi_debug_update_dimensions(const struct tp_finger *f)
+{
+	#define UPDATE_DIMENSIONS(val, op, last) \
+		do { \
+			if (le16_to_int(val) op last) { \
+				last = le16_to_int(val); \
+				applespi_dbg_dim_updated = true; \
+			} \
+		} while (0)
+
+	UPDATE_DIMENSIONS(f->abs_x, <, applespi_dbg_dim_min_x);
+	UPDATE_DIMENSIONS(f->abs_x, >, applespi_dbg_dim_max_x);
+	UPDATE_DIMENSIONS(f->abs_y, <, applespi_dbg_dim_min_y);
+	UPDATE_DIMENSIONS(f->abs_y, >, applespi_dbg_dim_max_y);
+
+	#undef UPDATE_DIMENSIONS
+}
+
+static void applespi_debug_print_dimensions(struct applespi_data *applespi)
+{
+	static ktime_t last_print;
+
+	if (applespi_dbg_dim_updated &&
+	    ktime_ms_delta(ktime_get(), last_print) > 1000) {
+		debug_print(DBG_TP_DIM, applespi,
+			    "New touchpad dimensions: %dx%d+%u+%u\n",
+			    applespi_dbg_dim_min_x, applespi_dbg_dim_min_y,
+			    applespi_dbg_dim_max_x - applespi_dbg_dim_min_x,
+			    applespi_dbg_dim_max_y - applespi_dbg_dim_min_y);
+		applespi_dbg_dim_updated = false;
+		last_print = ktime_get();
+	}
+}
+
+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,
+			 le16_to_int(f->touch_major) << 1);
+	input_report_abs(input, ABS_MT_TOUCH_MINOR,
+			 le16_to_int(f->touch_minor) << 1);
+	input_report_abs(input, ABS_MT_WIDTH_MAJOR,
+			 le16_to_int(f->tool_major) << 1);
+	input_report_abs(input, ABS_MT_WIDTH_MINOR,
+			 le16_to_int(f->tool_minor) << 1);
+	input_report_abs(input, ABS_MT_ORIENTATION,
+			 MAX_FINGER_ORIENTATION - le16_to_int(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)
+{
+	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 (le16_to_int(f->touch_major) == 0)
+			continue;
+		applespi->pos[n].x = le16_to_int(f->abs_x);
+		applespi->pos[n].y = tp_info->y_min + tp_info->y_max -
+				     le16_to_int(f->abs_y);
+		n++;
+
+		if (debug & DBG_TP_DIM)
+			applespi_debug_update_dimensions(f);
+	}
+
+	if (debug & DBG_TP_DIM)
+		applespi_debug_print_dimensions(applespi);
+
+	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_translate_fn_key(unsigned int key, int fn_pressed)
+{
+	const struct applespi_key_translation *trans;
+	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;
+	}
+
+	return key;
+}
+
+static unsigned int applespi_translate_iso_layout(unsigned int key)
+{
+	const struct applespi_key_translation *trans;
+
+	trans = applespi_find_translation(apple_iso_keyboard, key);
+	if (trans)
+		key = trans->to;
+
+	return key;
+}
+
+static unsigned int applespi_code_to_key(u8 code, int fn_pressed)
+{
+	unsigned int key = applespi_scancodes[code];
+
+	if (fnmode)
+		key = applespi_translate_fn_key(key, fn_pressed);
+	if (iso_layout)
+		key = applespi_translate_iso_layout(key);
+	return key;
+}
+
+static void
+applespi_remap_fn_key(struct keyboard_protocol *keyboard_protocol)
+{
+	unsigned char tmp;
+
+	if (!fnremap || fnremap > ARRAY_SIZE(applespi_controlcodes) ||
+	    !applespi_controlcodes[fnremap - 1])
+		return;
+
+	tmp = keyboard_protocol->fn_pressed;
+	keyboard_protocol->fn_pressed =
+			!!(keyboard_protocol->modifiers & BIT(fnremap - 1));
+	if (tmp)
+		keyboard_protocol->modifiers |= BIT(fnremap - 1);
+	else
+		keyboard_protocol->modifiers &= ~BIT(fnremap - 1);
+}
+
+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;
+
+	compiletime_assert(ARRAY_SIZE(applespi_controlcodes) ==
+			   sizeof_field(struct keyboard_protocol, modifiers) * 8,
+			   "applespi_controlcodes has wrong number of entries");
+
+	/* 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)
+			continue;
+
+		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 < ARRAY_SIZE(applespi_controlcodes); i++) {
+		if (keyboard_protocol->modifiers & BIT(i))
+			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(__u8 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 int
+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 sts;
+
+	/* set up touchpad dimensions */
+	tp_info = applespi_find_touchpad_info(rcvd_tp_info->model_no);
+	if (!tp_info) {
+		dev_warn(&(applespi)->spi->dev,
+			 "Unknown touchpad model %x - falling back to MB8 touchpad\n",
+			 rcvd_tp_info->model_no);
+		tp_info = &applespi_tp_models[0].tp_info;
+	}
+
+	applespi->tp_info = *tp_info;
+
+	if (touchpad_dimensions[0]) {
+		int x, y, w, h;
+
+		sts = sscanf(touchpad_dimensions, "%dx%d+%u+%u", &x, &y, &w, &h);
+		if (sts == 4) {
+			dev_info(&(applespi)->spi->dev,
+				 "Overriding touchpad dimensions from module param\n");
+			applespi->tp_info.x_min = x;
+			applespi->tp_info.y_min = y;
+			applespi->tp_info.x_max = x + w;
+			applespi->tp_info.y_max = y + h;
+		} else {
+			dev_warn(&(applespi)->spi->dev,
+				 "Invalid touchpad dimensions '%s': must be in the form XxY+W+H\n",
+				 touchpad_dimensions);
+			touchpad_dimensions[0] = '\0';
+		}
+	}
+	if (!touchpad_dimensions[0]) {
+		snprintf(touchpad_dimensions, sizeof(touchpad_dimensions),
+			 "%dx%d+%u+%u",
+			 applespi->tp_info.x_min,
+			 applespi->tp_info.y_min,
+			 applespi->tp_info.x_max - applespi->tp_info.x_min,
+			 applespi->tp_info.y_max - applespi->tp_info.y_min);
+	}
+
+	/* create touchpad input device */
+	touchpad_input_dev = devm_input_allocate_device(&(applespi)->spi->dev);
+	if (!touchpad_input_dev) {
+		dev_err(&(applespi)->spi->dev,
+			"Failed to allocate touchpad input device\n");
+		return -ENOMEM;
+	}
+
+	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_no << 8 | rcvd_tp_info->model_flags;
+
+	/* 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 */
+	sts = input_register_device(touchpad_input_dev);
+	if (sts) {
+		dev_err(&(applespi)->spi->dev,
+			"Unable to register touchpad input device (%d)\n", sts);
+		return sts;
+	}
+
+	/* touchpad_input_dev is read async in spi callback */
+	smp_store_release(&applespi->touchpad_input_dev, touchpad_input_dev);
+
+	return 0;
+}
+
+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)
+		dev_info(&(applespi)->spi->dev, "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) {
+		dev_warn_ratelimited(&(applespi)->spi->dev,
+				     "Received corrupted packet (crc mismatch)\n");
+		debug_print_header(DBG_RD_CRC, applespi);
+		debug_print_buffer(DBG_RD_CRC, applespi, "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, applespi);
+	debug_print_buffer(dbg_mask, applespi, "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 %u)\n",
+				     len);
+		goto msg_complete;
+	}
+
+	/* 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 msg_complete;
+		}
+
+		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 msg_complete;
+		}
+
+		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 msg_complete;
+		}
+
+		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 msg_complete;
+
+	if (le16_to_cpu(message->length) != msg_len - MSG_HEADER_SIZE - 2) {
+		dev_warn_ratelimited(&(applespi)->spi->dev,
+				     "Received corrupted packet (invalid message length %u - expected %u)\n",
+				     le16_to_cpu(message->length),
+				     msg_len - MSG_HEADER_SIZE - 2);
+		goto msg_complete;
+	}
+
+	/* 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;
+		size_t tp_len;
+
+		tp = &message->touchpad;
+		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 %u - num-fingers %u, tp-len %zu)\n",
+					     le16_to_cpu(message->length),
+					     tp->number_of_fingers, tp_len);
+			goto msg_complete;
+		}
+
+		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);
+	}
+
+msg_complete:
+	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) {
+		dev_warn(&(applespi)->spi->dev,
+			 "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, applespi);
+
+	spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
+
+	if (!applespi->suspended) {
+		sts = applespi_async(applespi, &applespi->rd_m,
+				     applespi_async_read_complete);
+		if (sts)
+			dev_warn(&(applespi)->spi->dev,
+				 "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(struct applespi_data *applespi)
+{
+	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 -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)
+		dev_warn(&(applespi)->spi->dev,
+			 "Error getting backlight level from EFI vars: %d\n",
+			 sts);
+
+	kfree(efivar_entry);
+
+	return sts ? sts : efi_data;
+}
+
+static void applespi_save_bl_level(struct applespi_data *applespi,
+				   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)
+		dev_warn(&(applespi)->spi->dev,
+			 "Error saving backlight level to EFI vars: %d\n", sts);
+}
+
+static int applespi_probe(struct spi_device *spi)
+{
+	struct applespi_data *applespi;
+	acpi_status acpi_sts;
+	int sts, i;
+	unsigned long long gpe, usb_status;
+
+	/* check if the USB interface is present and enabled already */
+	acpi_sts = acpi_evaluate_integer(ACPI_HANDLE(&spi->dev), "UIST", NULL,
+					 &usb_status);
+	if (ACPI_SUCCESS(acpi_sts) && usb_status) {
+		/* let the USB driver take over instead */
+		dev_info(&spi->dev, "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_array(&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))) {
+		dev_err(&(applespi)->spi->dev,
+			"Failed to get required ACPI method handles\n");
+		return -ENODEV;
+	}
+
+	/* switch on the SPI interface */
+	sts = applespi_setup_spi(applespi);
+	if (sts)
+		return sts;
+
+	sts = applespi_enable_spi(applespi);
+	if (sts)
+		return sts;
+
+	/* 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);
+
+	sts = input_register_device(applespi->keyboard_input_dev);
+	if (sts) {
+		dev_err(&(applespi)->spi->dev,
+			"Unable to register keyboard input device (%d)\n", sts);
+		return -ENODEV;
+	}
+
+	/*
+	 * The applespi device doesn't send interrupts normally (as is described
+	 * in its DSDT), but rather seems to use ACPI GPEs.
+	 */
+	acpi_sts = acpi_evaluate_integer(applespi->handle, "_GPE", NULL, &gpe);
+	if (ACPI_FAILURE(acpi_sts)) {
+		dev_err(&(applespi)->spi->dev,
+			"Failed to obtain GPE for SPI slave device: %s\n",
+			acpi_format_exception(acpi_sts));
+		return -ENODEV;
+	}
+	applespi->gpe = (int)gpe;
+
+	acpi_sts = acpi_install_gpe_handler(NULL, applespi->gpe,
+					    ACPI_GPE_LEVEL_TRIGGERED,
+					    applespi_notify, applespi);
+	if (ACPI_FAILURE(acpi_sts)) {
+		dev_err(&(applespi)->spi->dev,
+			"Failed to install GPE handler for GPE %d: %s\n",
+			applespi->gpe, acpi_format_exception(acpi_sts));
+		return -ENODEV;
+	}
+
+	applespi->suspended = false;
+
+	acpi_sts = acpi_enable_gpe(NULL, applespi->gpe);
+	if (ACPI_FAILURE(acpi_sts)) {
+		dev_err(&(applespi)->spi->dev,
+			"Failed to enable GPE handler for GPE %d: %s\n",
+			applespi->gpe, acpi_format_exception(acpi_sts));
+		acpi_remove_gpe_handler(NULL, applespi->gpe, applespi_notify);
+		return -ENODEV;
+	}
+
+	/* trigger touchpad setup */
+	applespi_init(applespi, false);
+
+	/*
+	 * By default this device is not enabled 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 */
+	sts = applespi_get_saved_bl_level(applespi);
+	if (sts >= 0)
+		applespi_set_bl_level(&applespi->backlight_info, sts);
+
+	applespi->backlight_info.name            = "spi::kbd_backlight";
+	applespi->backlight_info.default_trigger = "kbd-backlight";
+	applespi->backlight_info.brightness_set  = applespi_set_bl_level;
+
+	sts = devm_led_classdev_register(&spi->dev, &applespi->backlight_info);
+	if (sts)
+		dev_warn(&(applespi)->spi->dev,
+			 "Unable to register keyboard backlight class dev (%d)\n",
+			 sts);
+
+	return 0;
+}
+
+static void applespi_drain_writes(struct applespi_data *applespi)
+{
+	unsigned long flags;
+
+	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);
+}
+
+static void applespi_drain_reads(struct applespi_data *applespi)
+{
+	unsigned long flags;
+
+	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);
+}
+
+static int applespi_remove(struct spi_device *spi)
+{
+	struct applespi_data *applespi = spi_get_drvdata(spi);
+
+	applespi_drain_writes(applespi);
+
+	acpi_disable_gpe(NULL, applespi->gpe);
+	acpi_remove_gpe_handler(NULL, applespi->gpe, applespi_notify);
+	device_wakeup_disable(&spi->dev);
+
+	applespi_drain_reads(applespi);
+
+	return 0;
+}
+
+static void applespi_shutdown(struct spi_device *spi)
+{
+	struct applespi_data *applespi = spi_get_drvdata(spi);
+
+	applespi_save_bl_level(applespi, 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, applespi->have_bl_level);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+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 acpi_sts;
+	int sts;
+
+	/* turn off caps-lock - it'll stay on otherwise */
+	sts = applespi_set_capsl_led(applespi, false);
+	if (sts)
+		dev_warn(&(applespi)->spi->dev,
+			 "Failed to turn off caps-lock led (%d)\n", sts);
+
+	applespi_drain_writes(applespi);
+
+	/* disable the interrupt */
+	acpi_sts = acpi_disable_gpe(NULL, applespi->gpe);
+	if (ACPI_FAILURE(acpi_sts))
+		dev_err(&(applespi)->spi->dev,
+			"Failed to disable GPE handler for GPE %d: %s\n",
+			applespi->gpe, acpi_format_exception(acpi_sts));
+
+	applespi_drain_reads(applespi);
+
+	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 acpi_sts;
+	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 */
+	acpi_sts = acpi_enable_gpe(NULL, applespi->gpe);
+	if (ACPI_FAILURE(acpi_sts))
+		dev_err(&(applespi)->spi->dev,
+			"Failed to re-enable GPE handler for GPE %d: %s\n",
+			applespi->gpe, acpi_format_exception(acpi_sts));
+
+	/* switch the touchpad into multitouch mode */
+	applespi_init(applespi, true);
+
+	return 0;
+}
+#endif
+
+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",
+		.acpi_match_table	= applespi_acpi_match,
+		.pm			= &applespi_pm_ops,
+	},
+	.probe		= applespi_probe,
+	.remove		= applespi_remove,
+	.shutdown	= applespi_shutdown,
+};
+
+module_spi_driver(applespi_driver)
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("MacBook(Pro) SPI Keyboard/Touchpad driver");
+MODULE_AUTHOR("Federico Lorenzi");
+MODULE_AUTHOR("Ronald Tschalär");
-- 
2.20.1

Re: [PATCH v3 3/4] driver core: add dev_print_hex_dump() logging function.

[Greg Kroah-Hartman]

On Tue, Mar 26, 2019 at 06:48:06PM -0700, Ronald Tschalär wrote:
> This is the dev_xxx() analog to print_hex_dump(), using dev_printk()
> instead of straight printk() to match other dev_xxx() logging functions.
> ---
>  drivers/base/core.c    | 43 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/device.h | 15 +++++++++++++++
>  2 files changed, 58 insertions(+)

No signed-off-by?

Anyway, no, please do not do this.  Please do not dump large hex values
like this to the kernel log, it does not help anyone.

You can do this while debugging, sure, but not for "real" kernel code.

Worst case, just create a debugfs file for your device that you can read
the binary data from if you really need it.  For any "normal" operation,
this is not something that you should ever need.

thanks,

greg k-h

Re: [PATCH v3 2/4] lib/hexdump.c: factor out generic hexdump formatting for reuse.

[Andy Shevchenko]

On Wed, Mar 27, 2019 at 3:49 AM Ronald Tschalär <ronald@innovation.ch> wrote:
>
> This introduces print_hex_dump_to_cb() which contains all the hexdump
> formatting minus the actual printk() call, allowing an arbitrary print
> function to be supplied instead. And print_hex_dump() is re-implemented
> using print_hex_dump_to_cb().
>
> This allows other hex-dump logging functions to be provided which call
> printk() differently or even log the hexdump somewhere entirely
> different.

No Sign-off?

In any case, don't do it like this. smaller non-recursive printf() is
better than one big receursive call.
When it looks like an optimization, it's actually a regression.

And yes, debugfs idea is not bad.

P.S. Also check %*ph specifier.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, Mar 26, 2019 at 06:48:07PM -0700, 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.

> +// SPDX-License-Identifier: GPL-2.0

According to last changes this should be GPL-2.0-only

> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/crc16.h>
> +#include <linux/delay.h>
> +#include <linux/efi.h>
> +#include <linux/input.h>
> +#include <linux/input/mt.h>
> +#include <linux/ktime.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/spinlock.h>
> +#include <linux/spi/spi.h>
> +#include <linux/wait.h>
> +#include <linux/workqueue.h>
> +
> +#include <asm/barrier.h>

> +#include <asm-generic/unaligned.h>

generic?!

#include <asm/unaligned.h>
should work.

> +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 "-Unrecognized log mask-";

I don't think '-' surroundings are needed, but this is rather minor. Up to you.

> +	}
> +}

-- 
With Best Regards,
Andy Shevchenko

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

[Andrzej Hajda]

+cc: dri-devel

On 27.03.2019 02:48, Ronald Tschalär wrote:
> commit d6abe6df706c66d803e6dd4fe98c1b6b7f125a56 (drm/bridge:
> sil_sii8620: do not have a dependency of RC_CORE) added a dependency on
> INPUT. However, this causes problems with other drivers, in particular
> an input driver that depends on MFD_INTEL_LPSS_PCI (to be added in a
> future commit):
>
>   drivers/clk/Kconfig:9:error: recursive dependency detected!
>   drivers/clk/Kconfig:9:        symbol COMMON_CLK is selected by MFD_INTEL_LPSS
>   drivers/mfd/Kconfig:566:      symbol MFD_INTEL_LPSS is selected by MFD_INTEL_LPSS_PCI
>   drivers/mfd/Kconfig:580:      symbol MFD_INTEL_LPSS_PCI is implied by KEYBOARD_APPLESPI
>   drivers/input/keyboard/Kconfig:73:    symbol KEYBOARD_APPLESPI depends on INPUT
>   drivers/input/Kconfig:8:      symbol INPUT is selected by DRM_SIL_SII8620
>   drivers/gpu/drm/bridge/Kconfig:83:    symbol DRM_SIL_SII8620 depends on DRM_BRIDGE
>   drivers/gpu/drm/bridge/Kconfig:1:     symbol DRM_BRIDGE is selected by DRM_PL111
>   drivers/gpu/drm/pl111/Kconfig:1:      symbol DRM_PL111 depends on COMMON_CLK
>
> According to the docs, select should only be used for non-visible
> symbols. Furthermore almost all other references to INPUT throughout the
> kernel config are depends, not selects. Hence this change.
>
> CC: Inki Dae <inki.dae@samsung.com>
> CC: Andrzej Hajda <a.hajda@samsung.com>
> Signed-off-by: Ronald Tschalär <ronald@innovation.ch>


This is drm bridge driver, next time please cc it to dri-devel ML also.

Anyway this is not the solution we have agreed to.

Why have you abandoned the patch [1]? It needed just some minor
polishing, as I wrote in response to this mail.


[1]:
https://lore.kernel.org/lkml/20190124082423.23139-1-ronald@innovation.ch/T/#mf620df0b1583096a214d8e2e690387078583472f


Regards

Andrzej


> ---
>  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.

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

[Greg Kroah-Hartman]

On Wed, Mar 27, 2019 at 11:35:30AM +0200, Andy Shevchenko wrote:
> On Tue, Mar 26, 2019 at 06:48:07PM -0700, 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.
> 
> > +// SPDX-License-Identifier: GPL-2.0
> 
> According to last changes this should be GPL-2.0-only

What "last changes"?  "GPL-2.0" is a totally valid SPDX identifier for
the kernel.  Don't buy into the "-only" prefix crud that the newer SPDX
version adopted for crazy reasons.  The in-kernel documentation lists
the valid identifiers and the version of SPDX we are currently using.

thanks,

greg k-h

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

[Steven Rostedt]

On Wed, 27 Mar 2019 19:45:26 +0100
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> > > +// SPDX-License-Identifier: GPL-2.0  
> > 
> > According to last changes this should be GPL-2.0-only  
> 
> What "last changes"?  "GPL-2.0" is a totally valid SPDX identifier for
> the kernel.  Don't buy into the "-only" prefix crud that the newer SPDX
> version adopted for crazy reasons.  The in-kernel documentation lists
> the valid identifiers and the version of SPDX we are currently using.

According to: LICENSES/preferred/GPL-2.0

> SPDX-URL: https://spdx.org/licenses/GPL-2.0.html
> Usage-Guide:
>   To use this license in source code, put one of the following SPDX
>   tag/value pairs into a comment according to the placement
>   guidelines in the licensing rules documentation.
>   For 'GNU General Public License (GPL) version 2 only' use:
>     SPDX-License-Identifier: GPL-2.0
>   or
>     SPDX-License-Identifier: GPL-2.0-only
>   For 'GNU General Public License (GPL) version 2 or any later version' use:
>     SPDX-License-Identifier: GPL-2.0+
>   or
>     SPDX-License-Identifier: GPL-2.0-or-later

So, GPL-2.0 is the same as GPL-2.0-only. Changing it from one to the
other doesn't make any difference.

-- Steve

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

[Andy Shevchenko]

On Wed, Mar 27, 2019 at 07:45:26PM +0100, Greg Kroah-Hartman wrote:
> On Wed, Mar 27, 2019 at 11:35:30AM +0200, Andy Shevchenko wrote:
> > On Tue, Mar 26, 2019 at 06:48:07PM -0700, 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.
> > 
> > > +// SPDX-License-Identifier: GPL-2.0
> > 
> > According to last changes this should be GPL-2.0-only
> 
> What "last changes"?  "GPL-2.0" is a totally valid SPDX identifier for
> the kernel.  Don't buy into the "-only" prefix crud that the newer SPDX
> version adopted for crazy reasons.  The in-kernel documentation lists
> the valid identifiers and the version of SPDX we are currently using.

Thanks for this clarification.

// offtopic

Bartosz, so, it seems my first mail has been a correct one: in-kernel
documentation clarifies things for kernel.

// offtopic

-- 
With Best Regards,
Andy Shevchenko

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

[Life is hard, and then you die]

  Hi Andrzej,

On Wed, Mar 27, 2019 at 03:13:37PM +0100, Andrzej Hajda wrote:
> +cc: dri-devel
> 
> On 27.03.2019 02:48, Ronald Tschalär wrote:
> > commit d6abe6df706c66d803e6dd4fe98c1b6b7f125a56 (drm/bridge:
> > sil_sii8620: do not have a dependency of RC_CORE) added a dependency on
> > INPUT. However, this causes problems with other drivers, in particular
> > an input driver that depends on MFD_INTEL_LPSS_PCI (to be added in a
> > future commit):
> >
> >   drivers/clk/Kconfig:9:error: recursive dependency detected!
> >   drivers/clk/Kconfig:9:        symbol COMMON_CLK is selected by MFD_INTEL_LPSS
> >   drivers/mfd/Kconfig:566:      symbol MFD_INTEL_LPSS is selected by MFD_INTEL_LPSS_PCI
> >   drivers/mfd/Kconfig:580:      symbol MFD_INTEL_LPSS_PCI is implied by KEYBOARD_APPLESPI
> >   drivers/input/keyboard/Kconfig:73:    symbol KEYBOARD_APPLESPI depends on INPUT
> >   drivers/input/Kconfig:8:      symbol INPUT is selected by DRM_SIL_SII8620
> >   drivers/gpu/drm/bridge/Kconfig:83:    symbol DRM_SIL_SII8620 depends on DRM_BRIDGE
> >   drivers/gpu/drm/bridge/Kconfig:1:     symbol DRM_BRIDGE is selected by DRM_PL111
> >   drivers/gpu/drm/pl111/Kconfig:1:      symbol DRM_PL111 depends on COMMON_CLK
> >
> > According to the docs, select should only be used for non-visible
> > symbols. Furthermore almost all other references to INPUT throughout the
> > kernel config are depends, not selects. Hence this change.
> >
> > CC: Inki Dae <inki.dae@samsung.com>
> > CC: Andrzej Hajda <a.hajda@samsung.com>
> > Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
> 
> This is drm bridge driver, next time please cc it to dri-devel ML also.

Ok. Though as noted in the cover letter, the patch here is meant as a
placeholder till the real thing being discussed on dri-devel is
finalized. I was trying to avoid cross-posting too much, hence the
separate submission on dri-devel.

> Anyway this is not the solution we have agreed to.
> 
> Why have you abandoned the patch [1]? It needed just some minor
> polishing, as I wrote in response to this mail.
> 
> [1]:
> https://lore.kernel.org/lkml/20190124082423.23139-1-ronald@innovation.ch/T/#mf620df0b1583096a214d8e2e690387078583472f

It seems your mail client doesn't like me :-) I got neither of your
responses. Sorry, I should've checked the archives when I didn't hear
anything. In any case thank you for your review, and I will update
that patch and send out a new version shortly.


  Cheers,

  Ronald


> > ---
> >  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.
> 
> 

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

[Life is hard, and then you die]

On Wed, Mar 27, 2019 at 11:35:30AM +0200, Andy Shevchenko wrote:
> On Tue, Mar 26, 2019 at 06:48:07PM -0700, 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.
[snip]
> > +#include <asm/barrier.h>
> 
> > +#include <asm-generic/unaligned.h>
> 
> generic?!
> 
> #include <asm/unaligned.h>
> should work.

Yes, you're right. I brought myself up-to-speed now on the difference
between the two.

> > +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 "-Unrecognized log mask-";
> 
> I don't think '-' surroundings are needed, but this is rather minor. Up to you.

I've used that to distinguish an error value from normal values; but
that's not an idiom used in the kernel AFAICT, so I'll remove them.

Thanks.


  Cheers,

  Ronald

Re: [PATCH v3 3/4] driver core: add dev_print_hex_dump() logging function.

[Life is hard, and then you die]

On Wed, Mar 27, 2019 at 11:37:57AM +0900, Greg Kroah-Hartman wrote:
> On Tue, Mar 26, 2019 at 06:48:06PM -0700, Ronald Tschalär wrote:
> > This is the dev_xxx() analog to print_hex_dump(), using dev_printk()
> > instead of straight printk() to match other dev_xxx() logging functions.
> > ---
> >  drivers/base/core.c    | 43 ++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/device.h | 15 +++++++++++++++
> >  2 files changed, 58 insertions(+)
> 
> No signed-off-by?

Aargh! Apologies, fixed for the future.

> Anyway, no, please do not do this.  Please do not dump large hex values
> like this to the kernel log, it does not help anyone.
> 
> You can do this while debugging, sure, but not for "real" kernel code.

As used by this driver, it is definitely called for debugging only and
must be explicitly enabled via a module param. But having the ability
for folks to easily generate and print out debugging info has proven
quite valuable.

> Worst case, just create a debugfs file for your device that you can read
> the binary data from if you really need it.  For any "normal" operation,
> this is not something that you should ever need.

Ok, can do that.

I'll retract the two print_hex_dump related patches then.


  Cheers,

  Ronald

Re: [PATCH v3 2/4] lib/hexdump.c: factor out generic hexdump formatting for reuse.

[Life is hard, and then you die]

On Wed, Mar 27, 2019 at 09:46:48AM +0200, Andy Shevchenko wrote:
> On Wed, Mar 27, 2019 at 3:49 AM Ronald Tschalär <ronald@innovation.ch> wrote:
> >
> > This introduces print_hex_dump_to_cb() which contains all the hexdump
> > formatting minus the actual printk() call, allowing an arbitrary print
> > function to be supplied instead. And print_hex_dump() is re-implemented
> > using print_hex_dump_to_cb().
> >
> > This allows other hex-dump logging functions to be provided which call
> > printk() differently or even log the hexdump somewhere entirely
> > different.
> 
> No Sign-off?

Yeah, my screwup.

> In any case, don't do it like this. smaller non-recursive printf() is
> better than one big receursive call.
> When it looks like an optimization, it's actually a regression.

Not sure where you see recursion here - are you referring to the
callback approach? Since dev_printk() ends up calling printk with a
dictionary as well as additional formatting, vs print_hex_dump()'s
stright use of printk, this seemed like the best way accommodate
various possible ways of logging the messages. But as per below I
guess this is moot.

> And yes, debugfs idea is not bad.

So it seems like that is the consensus. As per my other response, I'll
do this then and leave the print_hex_dump() alone.

> P.S. Also check %*ph specifier.

Thanks!


  Cheers,

  Ronald

Re: [PATCH v3 3/4] driver core: add dev_print_hex_dump() logging function.

[Greg Kroah-Hartman]

On Wed, Mar 27, 2019 at 05:28:17PM -0700, Life is hard, and then you die wrote:
> 
> On Wed, Mar 27, 2019 at 11:37:57AM +0900, Greg Kroah-Hartman wrote:
> > On Tue, Mar 26, 2019 at 06:48:06PM -0700, Ronald Tschalär wrote:
> > > This is the dev_xxx() analog to print_hex_dump(), using dev_printk()
> > > instead of straight printk() to match other dev_xxx() logging functions.
> > > ---
> > >  drivers/base/core.c    | 43 ++++++++++++++++++++++++++++++++++++++++++
> > >  include/linux/device.h | 15 +++++++++++++++
> > >  2 files changed, 58 insertions(+)
> > 
> > No signed-off-by?
> 
> Aargh! Apologies, fixed for the future.
> 
> > Anyway, no, please do not do this.  Please do not dump large hex values
> > like this to the kernel log, it does not help anyone.
> > 
> > You can do this while debugging, sure, but not for "real" kernel code.
> 
> As used by this driver, it is definitely called for debugging only and
> must be explicitly enabled via a module param. But having the ability
> for folks to easily generate and print out debugging info has proven
> quite valuable.

We have dynamic debugging, no need for module parameters at all.  This
isn't the 1990's anymore :)

thanks,

greg k-h

Re: [PATCH v3 2/4] lib/hexdump.c: factor out generic hexdump formatting for reuse.

[Andy Shevchenko]

On Wed, Mar 27, 2019 at 05:34:59PM -0700, Life is hard, and then you die wrote:
> 
> On Wed, Mar 27, 2019 at 09:46:48AM +0200, Andy Shevchenko wrote:
> > On Wed, Mar 27, 2019 at 3:49 AM Ronald Tschalär <ronald@innovation.ch> wrote:
> > >
> > > This introduces print_hex_dump_to_cb() which contains all the hexdump
> > > formatting minus the actual printk() call, allowing an arbitrary print
> > > function to be supplied instead. And print_hex_dump() is re-implemented
> > > using print_hex_dump_to_cb().
> > >
> > > This allows other hex-dump logging functions to be provided which call
> > > printk() differently or even log the hexdump somewhere entirely
> > > different.

> > In any case, don't do it like this. smaller non-recursive printf() is
> > better than one big receursive call.
> > When it looks like an optimization, it's actually a regression.
> 
> Not sure where you see recursion here - are you referring to the
> callback approach?

%pV is a recursive printf().

> Since dev_printk() ends up calling printk with a
> dictionary as well as additional formatting, vs print_hex_dump()'s
> stright use of printk, this seemed like the best way accommodate
> various possible ways of logging the messages. But as per below I
> guess this is moot.

I recommend to read this: https://lwn.net/Articles/780556/

> > And yes, debugfs idea is not bad.
> 
> So it seems like that is the consensus. As per my other response, I'll
> do this then and leave the print_hex_dump() alone.
> 
> > P.S. Also check %*ph specifier.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 3/4] driver core: add dev_print_hex_dump() logging function.

[Life is hard, and then you die]

On Thu, Mar 28, 2019 at 06:29:17AM +0100, Greg Kroah-Hartman wrote:
> On Wed, Mar 27, 2019 at 05:28:17PM -0700, Life is hard, and then you die wrote:
> > 
> > On Wed, Mar 27, 2019 at 11:37:57AM +0900, Greg Kroah-Hartman wrote:
> > > On Tue, Mar 26, 2019 at 06:48:06PM -0700, Ronald Tschalär wrote:
> > > > This is the dev_xxx() analog to print_hex_dump(), using dev_printk()
> > > > instead of straight printk() to match other dev_xxx() logging functions.
> > > > ---
> > > >  drivers/base/core.c    | 43 ++++++++++++++++++++++++++++++++++++++++++
> > > >  include/linux/device.h | 15 +++++++++++++++
> > > >  2 files changed, 58 insertions(+)
> > > 
> > > No signed-off-by?
> > 
> > Aargh! Apologies, fixed for the future.
> > 
> > > Anyway, no, please do not do this.  Please do not dump large hex values
> > > like this to the kernel log, it does not help anyone.
> > > 
> > > You can do this while debugging, sure, but not for "real" kernel code.
> > 
> > As used by this driver, it is definitely called for debugging only and
> > must be explicitly enabled via a module param. But having the ability
> > for folks to easily generate and print out debugging info has proven
> > quite valuable.
> 
> We have dynamic debugging, no need for module parameters at all.  This
> isn't the 1990's anymore :)

I am aware of dynamic debugging, but there are several issues with it
from the perspective of the logging I'm doing here (I mentioned these
in response to an earlier review already):

  1. Dynamic debugging can't be enabled at a function or line level on
     the kernel command line, so this means that to debug issues
     during boot you have to enable everything, which can be way too
     verbose

  2. The expressions to enable the individual logging statements are
     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.

     One way to work around this would be to put every single logging
     statement in a function of its own, thereby ensuring a stable
     name is associated with each one.

  3. In at least two places we log different types of packets in the
     same lines of code (protected by a "if (log_mask & PKT_TYPE)") -
     dynamic-debug would only allow enabling or disabling logging of
     all packets. This could be worked around by creating a separate
     (but essentially duplicate) logging function for each packet type
     and some lookup table to call the appropriate one. Not very
     pretty IMO, though.

  4. In a couple places we log both the sent command and the received
     response, with the log-mask determining for which types of
     packets to do this. With a log mask there is one bit to set to
     get both logged; with dynamic debugging you'd have to enable the
     writing and receiving parts separately (not a huge deal, but yet
     one place where things are a bit more complicated than
     necessary).

Except for the first, none of these are insurmountable, but together
they don't make dynamic debugging very attractive for this sort of
logging.


  Cheers,

  Ronald

Re: [PATCH v3 2/4] lib/hexdump.c: factor out generic hexdump formatting for reuse.

[Life is hard, and then you die]

On Thu, Mar 28, 2019 at 11:03:50AM +0200, Andy Shevchenko wrote:
> On Wed, Mar 27, 2019 at 05:34:59PM -0700, Life is hard, and then you die wrote:
> > 
> > On Wed, Mar 27, 2019 at 09:46:48AM +0200, Andy Shevchenko wrote:
> > > On Wed, Mar 27, 2019 at 3:49 AM Ronald Tschalär <ronald@innovation.ch> wrote:
> > > >
> > > > This introduces print_hex_dump_to_cb() which contains all the hexdump
> > > > formatting minus the actual printk() call, allowing an arbitrary print
> > > > function to be supplied instead. And print_hex_dump() is re-implemented
> > > > using print_hex_dump_to_cb().
> > > >
> > > > This allows other hex-dump logging functions to be provided which call
> > > > printk() differently or even log the hexdump somewhere entirely
> > > > different.
> 
> > > In any case, don't do it like this. smaller non-recursive printf() is
> > > better than one big receursive call.
> > > When it looks like an optimization, it's actually a regression.
> > 
> > Not sure where you see recursion here - are you referring to the
> > callback approach?
> 
> %pV is a recursive printf().

Ah!

> > Since dev_printk() ends up calling printk with a
> > dictionary as well as additional formatting, vs print_hex_dump()'s
> > stright use of printk, this seemed like the best way accommodate
> > various possible ways of logging the messages. But as per below I
> > guess this is moot.
> 
> I recommend to read this: https://lwn.net/Articles/780556/

Thanks, quite informative.


  Cheers,

  Ronald

Re: [PATCH v3 3/4] driver core: add dev_print_hex_dump() logging function.

[Greg Kroah-Hartman]

On Thu, Mar 28, 2019 at 03:27:55AM -0700, Life is hard, and then you die wrote:
> 
> On Thu, Mar 28, 2019 at 06:29:17AM +0100, Greg Kroah-Hartman wrote:
> > On Wed, Mar 27, 2019 at 05:28:17PM -0700, Life is hard, and then you die wrote:
> > > 
> > > On Wed, Mar 27, 2019 at 11:37:57AM +0900, Greg Kroah-Hartman wrote:
> > > > On Tue, Mar 26, 2019 at 06:48:06PM -0700, Ronald Tschalär wrote:
> > > > > This is the dev_xxx() analog to print_hex_dump(), using dev_printk()
> > > > > instead of straight printk() to match other dev_xxx() logging functions.
> > > > > ---
> > > > >  drivers/base/core.c    | 43 ++++++++++++++++++++++++++++++++++++++++++
> > > > >  include/linux/device.h | 15 +++++++++++++++
> > > > >  2 files changed, 58 insertions(+)
> > > > 
> > > > No signed-off-by?
> > > 
> > > Aargh! Apologies, fixed for the future.
> > > 
> > > > Anyway, no, please do not do this.  Please do not dump large hex values
> > > > like this to the kernel log, it does not help anyone.
> > > > 
> > > > You can do this while debugging, sure, but not for "real" kernel code.
> > > 
> > > As used by this driver, it is definitely called for debugging only and
> > > must be explicitly enabled via a module param. But having the ability
> > > for folks to easily generate and print out debugging info has proven
> > > quite valuable.
> > 
> > We have dynamic debugging, no need for module parameters at all.  This
> > isn't the 1990's anymore :)
> 
> I am aware of dynamic debugging, but there are several issues with it
> from the perspective of the logging I'm doing here (I mentioned these
> in response to an earlier review already):
> 
>   1. Dynamic debugging can't be enabled at a function or line level on
>      the kernel command line, so this means that to debug issues
>      during boot you have to enable everything, which can be way too
>      verbose

You can, and should enable it at a function or line level by writing to
the proper kernel file in debugfs.

You have read Documentation/admin-guide/dynamic-debug-howto.rst, right?

No need to do anything on the command line, that's so old-school :)

>   2. The expressions to enable the individual logging statements are
>      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.
> 
>      One way to work around this would be to put every single logging
>      statement in a function of its own, thereby ensuring a stable
>      name is associated with each one.

Again, read the documentation, this works today as-is.

>   3. In at least two places we log different types of packets in the
>      same lines of code (protected by a "if (log_mask & PKT_TYPE)") -
>      dynamic-debug would only allow enabling or disabling logging of
>      all packets. This could be worked around by creating a separate
>      (but essentially duplicate) logging function for each packet type
>      and some lookup table to call the appropriate one. Not very
>      pretty IMO, though.

True, but you can use tracepoints as well, that would probably be much
easier when you are logging data streams.  You can also use an ebpf
program with the tracepoints to log just what you need/want to when you
want to as well.

>   4. In a couple places we log both the sent command and the received
>      response, with the log-mask determining for which types of
>      packets to do this. With a log mask there is one bit to set to
>      get both logged; with dynamic debugging you'd have to enable the
>      writing and receiving parts separately (not a huge deal, but yet
>      one place where things are a bit more complicated than
>      necessary).
> 
> Except for the first, none of these are insurmountable, but together
> they don't make dynamic debugging very attractive for this sort of
> logging.

Look into it some more, we have all of this covered today, no need for
just a single driver to do something fancy on its own :)

thanks,

greg k-h

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

[Andrzej Hajda]

On 28.03.2019 01:07, Life is hard, and then you die wrote:
>   Hi Andrzej,
>
> On Wed, Mar 27, 2019 at 03:13:37PM +0100, Andrzej Hajda wrote:
>> +cc: dri-devel
>>
>> On 27.03.2019 02:48, Ronald Tschalär wrote:
>>> commit d6abe6df706c66d803e6dd4fe98c1b6b7f125a56 (drm/bridge:
>>> sil_sii8620: do not have a dependency of RC_CORE) added a dependency on
>>> INPUT. However, this causes problems with other drivers, in particular
>>> an input driver that depends on MFD_INTEL_LPSS_PCI (to be added in a
>>> future commit):
>>>
>>>   drivers/clk/Kconfig:9:error: recursive dependency detected!
>>>   drivers/clk/Kconfig:9:        symbol COMMON_CLK is selected by MFD_INTEL_LPSS
>>>   drivers/mfd/Kconfig:566:      symbol MFD_INTEL_LPSS is selected by MFD_INTEL_LPSS_PCI
>>>   drivers/mfd/Kconfig:580:      symbol MFD_INTEL_LPSS_PCI is implied by KEYBOARD_APPLESPI
>>>   drivers/input/keyboard/Kconfig:73:    symbol KEYBOARD_APPLESPI depends on INPUT
>>>   drivers/input/Kconfig:8:      symbol INPUT is selected by DRM_SIL_SII8620
>>>   drivers/gpu/drm/bridge/Kconfig:83:    symbol DRM_SIL_SII8620 depends on DRM_BRIDGE
>>>   drivers/gpu/drm/bridge/Kconfig:1:     symbol DRM_BRIDGE is selected by DRM_PL111
>>>   drivers/gpu/drm/pl111/Kconfig:1:      symbol DRM_PL111 depends on COMMON_CLK
>>>
>>> According to the docs, select should only be used for non-visible
>>> symbols. Furthermore almost all other references to INPUT throughout the
>>> kernel config are depends, not selects. Hence this change.
>>>
>>> CC: Inki Dae <inki.dae@samsung.com>
>>> CC: Andrzej Hajda <a.hajda@samsung.com>
>>> Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
>> This is drm bridge driver, next time please cc it to dri-devel ML also.
> Ok. Though as noted in the cover letter, the patch here is meant as a
> placeholder till the real thing being discussed on dri-devel is
> finalized. I was trying to avoid cross-posting too much, hence the
> separate submission on dri-devel.
>
>> Anyway this is not the solution we have agreed to.
>>
>> Why have you abandoned the patch [1]? It needed just some minor
>> polishing, as I wrote in response to this mail.
>>
>> [1]:
>> https://lore.kernel.org/lkml/20190124082423.23139-1-ronald@innovation.ch/T/#mf620df0b1583096a214d8e2e690387078583472f
> It seems your mail client doesn't like me :-) I got neither of your
> responses. Sorry, I should've checked the archives when I didn't hear
> anything. In any case thank you for your review, and I will update
> that patch and send out a new version shortly.


I see where is the problem: Your mail client (mutt I suppose) sets
Mail-Followup-To header to all recipients (To and Cc) without the
sender. And my client (thunderbird) after pressing "Reply-All" checks
for Mail-Followup-To field, if present it uses only it to set
recipients, so in your case it does not response to the original author.

I do not know which mail client works incorrectly in this case, but for
sure it is not what we want :)

I do not know how to solve the issue in thunderbird, maybe mutt is more
configurable?


Regards

Andrzej


>
>
>   Cheers,
>
>   Ronald
>
>
>>> ---
>>>  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.
>>
>

Re: [PATCH v3 3/4] driver core: add dev_print_hex_dump() logging function.

[Steven Rostedt]

On Thu, 28 Mar 2019 12:29:52 +0100
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> >   3. In at least two places we log different types of packets in the
> >      same lines of code (protected by a "if (log_mask & PKT_TYPE)") -
> >      dynamic-debug would only allow enabling or disabling logging of
> >      all packets. This could be worked around by creating a separate
> >      (but essentially duplicate) logging function for each packet type
> >      and some lookup table to call the appropriate one. Not very
> >      pretty IMO, though.  
> 
> True, but you can use tracepoints as well, that would probably be much
> easier when you are logging data streams.  You can also use an ebpf
> program with the tracepoints to log just what you need/want to when you
> want to as well.

And tracepoints have filters where you don't even need ebpf to do such
filtering.

See Documentation/trace/events.rst Section 5: Event Filtering

-- Steve

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

[Life is hard, and then you die]

On Thu, Mar 28, 2019 at 12:48:39PM +0100, Andrzej Hajda wrote:
> On 28.03.2019 01:07, Life is hard, and then you die wrote:
> >
> > On Wed, Mar 27, 2019 at 03:13:37PM +0100, Andrzej Hajda wrote:
> >> +cc: dri-devel
[snip]
> > It seems your mail client doesn't like me :-) I got neither of your
> > responses. Sorry, I should've checked the archives when I didn't hear
> > anything. In any case thank you for your review, and I will update
> > that patch and send out a new version shortly.
> 
> I see where is the problem: Your mail client (mutt I suppose) sets
> Mail-Followup-To header to all recipients (To and Cc) without the
> sender. And my client (thunderbird) after pressing "Reply-All" checks
> for Mail-Followup-To field, if present it uses only it to set
> recipients, so in your case it does not response to the original author.
> 
> I do not know which mail client works incorrectly in this case, but for
> sure it is not what we want :)
> 
> I do not know how to solve the issue in thunderbird, maybe mutt is more
> configurable?

Thanks for debugging this! I'm sorry, but it looks like this was an
error on my part. So for anybody else using mutt: by default, mutt
will add a Mail-Followup-To header without your own address when
sending mail to any lists you are subscribed to, as defined by the
'subscribe' command (to avoid receiving replies in duplicate, i.e.
once via to/cc and once via the list). Unfortunately I was using a
regex there that included all freedesktop.org lists, not just ones I'm
actually subscribed to, and hence it thought I was subscribed
dri-devel too, leading to the this screwup.

Apologies for this - I guess even after more than 2 decades of mutt
and mailing lists I still manage to not know all its ins and outs.
But it should be fixed now.


  Cheers,

  Ronald

Re: [PATCH v3 3/4] driver core: add dev_print_hex_dump() logging function.

[Life is hard, and then you die]

On Thu, Mar 28, 2019 at 12:29:52PM +0100, Greg Kroah-Hartman wrote:
> On Thu, Mar 28, 2019 at 03:27:55AM -0700, Life is hard, and then you die wrote:
> > 
> > On Thu, Mar 28, 2019 at 06:29:17AM +0100, Greg Kroah-Hartman wrote:
> > > On Wed, Mar 27, 2019 at 05:28:17PM -0700, Life is hard, and then you die wrote:
> > > > 
> > > > On Wed, Mar 27, 2019 at 11:37:57AM +0900, Greg Kroah-Hartman wrote:
> > > > > On Tue, Mar 26, 2019 at 06:48:06PM -0700, Ronald Tschalär wrote:
> > > > > > This is the dev_xxx() analog to print_hex_dump(), using dev_printk()
> > > > > > instead of straight printk() to match other dev_xxx() logging functions.
> > > > > > ---
> > > > > >  drivers/base/core.c    | 43 ++++++++++++++++++++++++++++++++++++++++++
> > > > > >  include/linux/device.h | 15 +++++++++++++++
> > > > > >  2 files changed, 58 insertions(+)
[snip]
> > > > > Anyway, no, please do not do this.  Please do not dump large hex values
> > > > > like this to the kernel log, it does not help anyone.
> > > > > 
> > > > > You can do this while debugging, sure, but not for "real" kernel code.
> > > > 
> > > > As used by this driver, it is definitely called for debugging only and
> > > > must be explicitly enabled via a module param. But having the ability
> > > > for folks to easily generate and print out debugging info has proven
> > > > quite valuable.
> > > 
> > > We have dynamic debugging, no need for module parameters at all.  This
> > > isn't the 1990's anymore :)
> > 
> > I am aware of dynamic debugging, but there are several issues with it
> > from the perspective of the logging I'm doing here (I mentioned these
> > in response to an earlier review already):
> > 
> >   1. Dynamic debugging can't be enabled at a function or line level on
> >      the kernel command line, so this means that to debug issues
> >      during boot you have to enable everything, which can be way too
> >      verbose
> 
> You can, and should enable it at a function or line level by writing to
> the proper kernel file in debugfs.
> 
> You have read Documentation/admin-guide/dynamic-debug-howto.rst, right?

Yes, and I've played with the parameters quite a bit.

> No need to do anything on the command line, that's so old-school :)

Sorry if I'm being unduly dense, but then how to enable debugging
during early boot? The only other alternative I see is modifying the
initrd, and asking folks to do that is noticeably more complicated
than having them add something to the command line in grub. So from my
perspective I find kernel params far from old-school :-)

> >   2. The expressions to enable the individual logging statements are
> >      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.
> > 
> >      One way to work around this would be to put every single logging
> >      statement in a function of its own, thereby ensuring a stable
> >      name is associated with each one.
> 
> Again, read the documentation, this works today as-is.

I have read it (we're talking about the dynamic debug docs here), but
I just don't see how it addresses this in any way.

> >   3. In at least two places we log different types of packets in the
> >      same lines of code (protected by a "if (log_mask & PKT_TYPE)") -
> >      dynamic-debug would only allow enabling or disabling logging of
> >      all packets. This could be worked around by creating a separate
> >      (but essentially duplicate) logging function for each packet type
> >      and some lookup table to call the appropriate one. Not very
> >      pretty IMO, though.
> 
> True, but you can use tracepoints as well, that would probably be much
> easier when you are logging data streams.  You can also use an ebpf
> program with the tracepoints to log just what you need/want to when you
> want to as well.

Thanks for the hint, I hadn't paid much attention to tracepoints till
now. They do solve most of these issues I've mentioned here, though.
So I've actually gone and implemented the tracing as tracepoints now.
Two issues I noticed though:

 1. since filters can't be enabled on the command line (and yes, we
    seem to disagree on the usefulness of the command line) it means
    I had to essentially create tracepoints for every trace+filter
    combo I may want to enable (in my case it's just one field, so I
    ended up with 8 tracepoints instead of 1). Not a big deal in my
    case, but could get ugly.

 2. in cases where there is relevant log output too, folks have to be
    told to provide both trace and dmesg output, the outputs have to
    merged back together again. Though I note that with the use of the
    tp_printk kernel param this inconvenience goes away again (but
    we're back to logging the traces in the kernel log :-) ).

> >   4. In a couple places we log both the sent command and the received
> >      response, with the log-mask determining for which types of
> >      packets to do this. With a log mask there is one bit to set to
> >      get both logged; with dynamic debugging you'd have to enable the
> >      writing and receiving parts separately (not a huge deal, but yet
> >      one place where things are a bit more complicated than
> >      necessary).
> > 
> > Except for the first, none of these are insurmountable, but together
> > they don't make dynamic debugging very attractive for this sort of
> > logging.
> 
> Look into it some more, we have all of this covered today, no need for
> just a single driver to do something fancy on its own :)

The tracepoints do indeed cover this too.


  Cheers,

  Ronald

Re: [PATCH v3 3/4] driver core: add dev_print_hex_dump() logging function.

[Greg Kroah-Hartman]

On Mon, Apr 01, 2019 at 07:47:14PM -0700, Life is hard, and then you die wrote:
> 
> On Thu, Mar 28, 2019 at 12:29:52PM +0100, Greg Kroah-Hartman wrote:
> > On Thu, Mar 28, 2019 at 03:27:55AM -0700, Life is hard, and then you die wrote:
> > > 
> > > On Thu, Mar 28, 2019 at 06:29:17AM +0100, Greg Kroah-Hartman wrote:
> > > > On Wed, Mar 27, 2019 at 05:28:17PM -0700, Life is hard, and then you die wrote:
> > > > > 
> > > > > On Wed, Mar 27, 2019 at 11:37:57AM +0900, Greg Kroah-Hartman wrote:
> > > > > > On Tue, Mar 26, 2019 at 06:48:06PM -0700, Ronald Tschalär wrote:
> > > > > > > This is the dev_xxx() analog to print_hex_dump(), using dev_printk()
> > > > > > > instead of straight printk() to match other dev_xxx() logging functions.
> > > > > > > ---
> > > > > > >  drivers/base/core.c    | 43 ++++++++++++++++++++++++++++++++++++++++++
> > > > > > >  include/linux/device.h | 15 +++++++++++++++
> > > > > > >  2 files changed, 58 insertions(+)
> [snip]
> > > > > > Anyway, no, please do not do this.  Please do not dump large hex values
> > > > > > like this to the kernel log, it does not help anyone.
> > > > > > 
> > > > > > You can do this while debugging, sure, but not for "real" kernel code.
> > > > > 
> > > > > As used by this driver, it is definitely called for debugging only and
> > > > > must be explicitly enabled via a module param. But having the ability
> > > > > for folks to easily generate and print out debugging info has proven
> > > > > quite valuable.
> > > > 
> > > > We have dynamic debugging, no need for module parameters at all.  This
> > > > isn't the 1990's anymore :)
> > > 
> > > I am aware of dynamic debugging, but there are several issues with it
> > > from the perspective of the logging I'm doing here (I mentioned these
> > > in response to an earlier review already):
> > > 
> > >   1. Dynamic debugging can't be enabled at a function or line level on
> > >      the kernel command line, so this means that to debug issues
> > >      during boot you have to enable everything, which can be way too
> > >      verbose
> > 
> > You can, and should enable it at a function or line level by writing to
> > the proper kernel file in debugfs.
> > 
> > You have read Documentation/admin-guide/dynamic-debug-howto.rst, right?
> 
> Yes, and I've played with the parameters quite a bit.
> 
> > No need to do anything on the command line, that's so old-school :)
> 
> Sorry if I'm being unduly dense, but then how to enable debugging
> during early boot? The only other alternative I see is modifying the
> initrd, and asking folks to do that is noticeably more complicated
> than having them add something to the command line in grub. So from my
> perspective I find kernel params far from old-school :-)

You can do dynamic debugging from the kernel command line, if your code
is built into the kernel (but why would a tiny driver under testing like
this, not be built into the kernel?) what specifically did not work for you?

> > >   2. The expressions to enable the individual logging statements are
> > >      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.
> > > 
> > >      One way to work around this would be to put every single logging
> > >      statement in a function of its own, thereby ensuring a stable
> > >      name is associated with each one.
> > 
> > Again, read the documentation, this works today as-is.
> 
> I have read it (we're talking about the dynamic debug docs here), but
> I just don't see how it addresses this in any way.

You can enable/disable logging per-function, which is what you want,
right?


Anyway, I'm glad tracepoints work for you, that should be all that is
now needed.

good luck with your driver!

greg k-h

Re: [PATCH v3 3/4] driver core: add dev_print_hex_dump() logging function.

[Life is hard, and then you die]

> On Mon, Apr 01, 2019 at 07:47:14PM -0700, Life is hard, and then you die wrote:
> > 
> > On Thu, Mar 28, 2019 at 12:29:52PM +0100, Greg Kroah-Hartman wrote:
> > > On Thu, Mar 28, 2019 at 03:27:55AM -0700, Life is hard, and then you die wrote:
> > > > 
> > > > On Thu, Mar 28, 2019 at 06:29:17AM +0100, Greg Kroah-Hartman wrote:
> > > > > On Wed, Mar 27, 2019 at 05:28:17PM -0700, Life is hard, and then you die wrote:
> > > > > > 
> > > > > > On Wed, Mar 27, 2019 at 11:37:57AM +0900, Greg Kroah-Hartman wrote:
> > > > > > > On Tue, Mar 26, 2019 at 06:48:06PM -0700, Ronald Tschalär wrote:
> > > > > > > > This is the dev_xxx() analog to print_hex_dump(), using dev_printk()
> > > > > > > > instead of straight printk() to match other dev_xxx() logging functions.
> > > > > > > > ---
> > > > > > > >  drivers/base/core.c    | 43 ++++++++++++++++++++++++++++++++++++++++++
> > > > > > > >  include/linux/device.h | 15 +++++++++++++++
> > > > > > > >  2 files changed, 58 insertions(+)
> > [snip]
> > > > > > > Anyway, no, please do not do this.  Please do not dump large hex values
> > > > > > > like this to the kernel log, it does not help anyone.
> > > > > > > 
> > > > > > > You can do this while debugging, sure, but not for "real" kernel code.
> > > > > > 
> > > > > > As used by this driver, it is definitely called for debugging only and
> > > > > > must be explicitly enabled via a module param. But having the ability
> > > > > > for folks to easily generate and print out debugging info has proven
> > > > > > quite valuable.
> > > > > 
> > > > > We have dynamic debugging, no need for module parameters at all.  This
> > > > > isn't the 1990's anymore :)
> > > > 
> > > > I am aware of dynamic debugging, but there are several issues with it
> > > > from the perspective of the logging I'm doing here (I mentioned these
> > > > in response to an earlier review already):
> > > > 
> > > >   1. Dynamic debugging can't be enabled at a function or line level on
> > > >      the kernel command line, so this means that to debug issues
> > > >      during boot you have to enable everything, which can be way too
> > > >      verbose
> > > 
> > > You can, and should enable it at a function or line level by writing to
> > > the proper kernel file in debugfs.
> > > 
> > > You have read Documentation/admin-guide/dynamic-debug-howto.rst, right?
> > 
> > Yes, and I've played with the parameters quite a bit.
> > 
> > > No need to do anything on the command line, that's so old-school :)
> > 
> > Sorry if I'm being unduly dense, but then how to enable debugging
> > during early boot? The only other alternative I see is modifying the
> > initrd, and asking folks to do that is noticeably more complicated
> > than having them add something to the command line in grub. So from my
> > perspective I find kernel params far from old-school :-)
> 
> You can do dynamic debugging from the kernel command line, if your code
> is built into the kernel (but why would a tiny driver under testing like
> this, not be built into the kernel?) what specifically did not work for you?

This may be part of our disconnect: there's almost no reason (and
several downsides) to building it into the kernel instead of as a
module:

- When developing, being able to just reload the module instead of
  rebooting is just so much faster and more convenient.

- For other users, having them build the driver as a dkms managed
  module is also by far the simplest and least error-prone approach -
  explaning to users how to rebuild their kernel (something most of
  them have never done) takes a bunch of time and effort on both
  sides for essentially no gain.

- Once the driver is part of the regular kernel, practically all
  distro's will build it as a module (at least I'm fairly certain
  about this).

In the case where a problem happens during early boot, if I can
reproduce myself then of course I can build the driver into the kernel
and debug it; but if I need others to test/debug things, then this is
a much harder sell.

In my experience, every time you make something even a little harder
for a user to do, the chance of them doing it and you getting useful
feedback goes down exponentially and the amount of work you need to do
to handhold them goes up significantly. So I think it's absolutely
vital to try and keep things as simple as possible.

> > > >   2. The expressions to enable the individual logging statements are
> > > >      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.
> > > > 
> > > >      One way to work around this would be to put every single logging
> > > >      statement in a function of its own, thereby ensuring a stable
> > > >      name is associated with each one.
> > > 
> > > Again, read the documentation, this works today as-is.
> > 
> > I have read it (we're talking about the dynamic debug docs here), but
> > I just don't see how it addresses this in any way.
> 
> You can enable/disable logging per-function, which is what you want,
> right?

Yes'ish: the problem is that if they are just part of regular
functions, A) the chances are higher that the function may get renamed
and hence any existing debug instructions broken, and B) this doesn't
work if there are multiple debug statements in a function. Hence my
assertion that for this work well (and yes, it can work well) you
basically need to create a separate function for each debug statement
(which that contains just that debug statement) (a sort of stable
labelling of each debug statement).


  Cheers,

  Ronald

Re: [PATCH v3 3/4] driver core: add dev_print_hex_dump() logging function.

[Andy Shevchenko]

On Sat, Apr 06, 2019 at 06:46:13PM -0700, Life is hard, and then you die wrote:
> > On Mon, Apr 01, 2019 at 07:47:14PM -0700, Life is hard, and then you die wrote:
> > > On Thu, Mar 28, 2019 at 12:29:52PM +0100, Greg Kroah-Hartman wrote:
> > > > On Thu, Mar 28, 2019 at 03:27:55AM -0700, Life is hard, and then you die wrote:
> > > > > On Thu, Mar 28, 2019 at 06:29:17AM +0100, Greg Kroah-Hartman wrote:
> > > > > > On Wed, Mar 27, 2019 at 05:28:17PM -0700, Life is hard, and then you die wrote:
> > > > > > > On Wed, Mar 27, 2019 at 11:37:57AM +0900, Greg Kroah-Hartman wrote:
> > > > > > > > On Tue, Mar 26, 2019 at 06:48:06PM -0700, Ronald Tschalär wrote:

> > You can do dynamic debugging from the kernel command line, if your code
> > is built into the kernel (but why would a tiny driver under testing like
> > this, not be built into the kernel?) what specifically did not work for you?
> 
> This may be part of our disconnect: there's almost no reason (and
> several downsides) to building it into the kernel instead of as a
> module:
> 
> - When developing, being able to just reload the module instead of
>   rebooting is just so much faster and more convenient.

modprobe -r foo
modprobe foo dyndbg=...

Not an argument.

> - For other users, having them build the driver as a dkms managed
>   module is also by far the simplest and least error-prone approach -
>   explaning to users how to rebuild their kernel (something most of
>   them have never done) takes a bunch of time and effort on both
>   sides for essentially no gain.
> 
> - Once the driver is part of the regular kernel, practically all
>   distro's will build it as a module (at least I'm fairly certain
>   about this).

Above seems a sub-items to the first one. So, reloading module with dyndbg
parameter is quite flexible. What else?

> > You can enable/disable logging per-function, which is what you want,
> > right?
> 
> Yes'ish: the problem is that if they are just part of regular
> functions, A) the chances are higher that the function may get renamed
> and hence any existing debug instructions broken, and B) this doesn't
> work if there are multiple debug statements in a function. Hence my
> assertion that for this work well (and yes, it can work well) you
> basically need to create a separate function for each debug statement
> (which that contains just that debug statement) (a sort of stable
> labelling of each debug statement).

I guess you tried and have an examples?

-- 
With Best Regards,
Andy Shevchenko