[RFC PATCH 0/9] Add support for Microsoft Surface System Aggregator Module

[RFC PATCH 0/9] Add support for Microsoft Surface System Aggregator Module

[Maximilian Luz]

Hello,

The Surface System Aggregator Module (we'll refer to it as Surface
Aggregator or SAM below) is an embedded controller (EC) found on various
Microsoft Surface devices. Specifically, all 4th and later generation
Surface devices, i.e. Surface Pro 4, Surface Book 1 and later, with the
exception of the Surface Go series and the Surface Duo. Notably, it
seems like this EC can also be found on the ARM-based Surface Pro X [1].

Functionality provided by this EC depends on the Surface model and can
(roughly) be broken down by their generations: Starting with 5th
generation devices (Surface Pro 2017/5, Surface Book 2, Surface Laptop
1, and later), the EC provides battery and thermal readings, as well as
access to the real-time clock. On 5th and 6th generations, these
features, specifically, are provided via the ACPI interface of the EC,
referred to as Surface ACPI Notify (SAN), i.e. they act as standard ACPI
devices of that type, but require a driver translating requests written
to an ACPI operation region to requests to the EC. On 7th generation
devices, the ACPI interface is (largely) gone, and has been replaced
with custom battery and thermal drivers, directly querying the EC.

Additionally, HID keyboard and touchpad input for Surface models with
these devices built in can be handled via the EC: On the Surface Laptops
1 and 2, this includes only the keyboard, while on the Surface Laptop 3
and Book 3, this includes both touchpad and keyboard. In this case,
actual input is provided as HID data and the EC connection acts as HID
transport, thus requiring a special transport driver for those devices
to work.

Further, all known devices (5th and later generations) also support
changing of performance/cooling modes, which can influence cooling
capabilities of the device (e.g. prefer silent operation over
performance), and may influence power limits (e.g. of the discrete GPU
found on Surface Books).

While this constitutes all major functionality, some more device
specific functionality is also handled by the EC. For example, on the
Surface Books, the EC handles detaching of the clipboard (i.e. the upper
part with screen and CPU) from the base (the lower part with keyboard
and optional discrete GPU) and can influence its behavior (i.e. it
provides an interface via which detachment can be requested, aborted, or
confirmed). It can also be used to detect if there has been a base
attached to the clipboard, and if so what type.

This patch-series adds the basis for supporting this EC and the features
provided by it, by, first, implementing a communication driver providing
a fundamental API for client drivers, handling specific aspects of the
EC. Additionally, it builds on top of that to provide a dedicated bus
and device type to better manage EC clients (and break it down pseudo-
device-wise), especially in the case when these client devices are not
described in ACPI, i.e. cannot be discovered by conventional means.
Furthermore, it provides support for debugging and prototyping via an
optional DebugFS interface, and, lastly, also support for the
aforementioned ACPI interface, allowing ACPI to communicate with the EC
directly.

This series only addresses 5th and later generation Surface models as
the communication interface has changed substantially from 4th to 5th
generation, and the 4th generation interface has not been reverse-
engineered yet. Specifically, the underlying transport has been changed
from HID feature and input-/output-reports to serial communication via
an UART and a custom protocol. Support for 4th generation devices may be
added in the future, but as currently not much is known about 4th
generation SAM, it yet remains to be seen if this can happen as addition
to this subsystem, or if it will be easier to implement this as separate
platform driver. Especially as the 4th generation EC does not seem to
provide much of the functionality found on 5th and later generations
(e.g. no battery status reporting, no thermal reporting, ..., we assume
it's just clipboard detachment on the Surface Book 1 and performance
mode setting).

In more detail, this series adds a driver for the Surface Serial Hub
(SSH), the 5th- and later-generation communication channel to the EC, a
pseudo-device and driver exposing a DebugFS interface that can be used
to communicate with the EC from user-space, intended for debugging,
testing, and prototyping, as well as a driver for the Surface ACPI
Notify (SAN) device, i.e. the interface between ACPI and EC. Some more
details on those can be found on the individual commit messages.

This series, apart from the SAN and DebugFS drivers, does not add any
client drivers. This will be handled via future patches once the core
has been accepted (and the other client drivers have been cleaned up a
bit).

On the top level, EC communication via the SSH driver can be broken down
into requests (sent from host to EC), corresponding responses (sent from
EC to host, associated with and triggered by a request), and events
(sent from EC to host without association ot a request). The SSH driver
manages all communication (i.e. matches responses to requests, enables
and disables events, and manages event handlers/notifiers installed by
client drivers). On the lower levels, SSH communication is packet-based,
and described in more detail in the documentation added in this series
(specifically ssh.rst).

This set of modules and drivers has been developed out of tree at [2]
and used/tested in the kernel we provide at [3] pretty much since its
beginnings. It has been developed by reverse-engineering the SSH
protocol, mostly through the ACPI interface, communication dumps
obtained from listening in on Windows, and deduction. So things may be
wrong. There have been some attempts at reverse-engineering existing
drivers, which also gave a bit of insight for development, however, I
haven't gotten very far on that front beyond some more higher-level
concepts and detecting a couple of new EC commands/confirming the
functionality of already known commands.

Driver and module names have been chosen to align with Windows driver
names, some field, vairable, and concept names have been chosen to align
with ACPI code (or at least with what I think some of the more cryptic
names could mean and make sense in the respective context, e.g. IID ->
Instance ID, TC -> Target Category).

While I consider this submission complete, I've decided to submit this
as RFC first, mainly due to its size and it being my first submission on
this scale. Any feedback, review, comment, question, etc. is much
appreciated.

This patch-set can also be found at the following respository and
reference, if you prefer to look at a kernel tree instead of these
emails:

  https://github.com/linux-surface/kernel tags/s/surface-aggregator/rfc-v1

Thanks,
Max

[1]: The Surface Pro X is, however, currently considered unsupported due
     to a lack of test candidates and, as it seems, general lack of
     Linux support on other parts. AFAIK there is an issue preventing
     serial devices from being registered, on which the core driver in
     this series is build on, thus there is no way to even test that at
     this point. I'd be happy to work out any issues regarding SAM on
     the Pro X at some point in the future, provided someone can/wants
     to actually test it.

[2]: https://github.com/linux-surface/surface-aggregator-module
[3]: https://github.com/linux-surface/linux-surface

Maximilian Luz (9):
  misc: Add Surface Aggregator subsystem
  surface_aggregator: Add control packet allocation chaching
  surface_aggregator: Add event item allocation chaching
  surface_aggregator: Add trace points
  surface_aggregator: Add error injection capabilities
  surface_aggregator: Add dedicated bus and device type
  docs: driver-api: Add Surface Aggregator subsystem documentation
  surface_aggregator: Add DebugFS interface
  surface_aggregator: Add Surface ACPI Notify client driver

 Documentation/driver-api/index.rst            |    1 +
 .../surface_aggregator/client-api.rst         |   38 +
 .../driver-api/surface_aggregator/client.rst  |  394 +++
 .../surface_aggregator/clients/dbgdev.rst     |  130 +
 .../surface_aggregator/clients/index.rst      |   21 +
 .../surface_aggregator/clients/san.rst        |   44 +
 .../driver-api/surface_aggregator/index.rst   |   21 +
 .../surface_aggregator/internal-api.rst       |   67 +
 .../surface_aggregator/internal.rst           |   50 +
 .../surface_aggregator/overview.rst           |   76 +
 .../driver-api/surface_aggregator/ssh.rst     |  343 +++
 MAINTAINERS                                   |   10 +
 drivers/misc/Kconfig                          |    1 +
 drivers/misc/Makefile                         |    1 +
 drivers/misc/surface_aggregator/Kconfig       |   65 +
 drivers/misc/surface_aggregator/Makefile      |   17 +
 drivers/misc/surface_aggregator/bus.c         |  419 +++
 drivers/misc/surface_aggregator/bus.h         |   22 +
 .../misc/surface_aggregator/clients/Kconfig   |   38 +
 .../misc/surface_aggregator/clients/Makefile  |    4 +
 .../clients/surface_acpi_notify.c             |  882 ++++++
 .../clients/surface_aggregator_debugfs.c      |  281 ++
 drivers/misc/surface_aggregator/controller.c  | 2505 +++++++++++++++++
 drivers/misc/surface_aggregator/controller.h  |  283 ++
 drivers/misc/surface_aggregator/core.c        |  821 ++++++
 drivers/misc/surface_aggregator/ssh_msgb.h    |  196 ++
 .../surface_aggregator/ssh_packet_layer.c     | 2002 +++++++++++++
 .../surface_aggregator/ssh_packet_layer.h     |  170 ++
 drivers/misc/surface_aggregator/ssh_parser.c  |  224 ++
 drivers/misc/surface_aggregator/ssh_parser.h  |  152 +
 .../surface_aggregator/ssh_request_layer.c    | 1249 ++++++++
 .../surface_aggregator/ssh_request_layer.h    |  137 +
 drivers/misc/surface_aggregator/trace.h       |  621 ++++
 include/linux/mod_devicetable.h               |   18 +
 include/linux/surface_acpi_notify.h           |   37 +
 include/linux/surface_aggregator/controller.h |  812 ++++++
 include/linux/surface_aggregator/device.h     |  408 +++
 include/linux/surface_aggregator/serial_hub.h |  657 +++++
 scripts/mod/devicetable-offsets.c             |    8 +
 scripts/mod/file2alias.c                      |   23 +
 40 files changed, 13248 insertions(+)
 create mode 100644 Documentation/driver-api/surface_aggregator/client-api.rst
 create mode 100644 Documentation/driver-api/surface_aggregator/client.rst
 create mode 100644 Documentation/driver-api/surface_aggregator/clients/dbgdev.rst
 create mode 100644 Documentation/driver-api/surface_aggregator/clients/index.rst
 create mode 100644 Documentation/driver-api/surface_aggregator/clients/san.rst
 create mode 100644 Documentation/driver-api/surface_aggregator/index.rst
 create mode 100644 Documentation/driver-api/surface_aggregator/internal-api.rst
 create mode 100644 Documentation/driver-api/surface_aggregator/internal.rst
 create mode 100644 Documentation/driver-api/surface_aggregator/overview.rst
 create mode 100644 Documentation/driver-api/surface_aggregator/ssh.rst
 create mode 100644 drivers/misc/surface_aggregator/Kconfig
 create mode 100644 drivers/misc/surface_aggregator/Makefile
 create mode 100644 drivers/misc/surface_aggregator/bus.c
 create mode 100644 drivers/misc/surface_aggregator/bus.h
 create mode 100644 drivers/misc/surface_aggregator/clients/Kconfig
 create mode 100644 drivers/misc/surface_aggregator/clients/Makefile
 create mode 100644 drivers/misc/surface_aggregator/clients/surface_acpi_notify.c
 create mode 100644 drivers/misc/surface_aggregator/clients/surface_aggregator_debugfs.c
 create mode 100644 drivers/misc/surface_aggregator/controller.c
 create mode 100644 drivers/misc/surface_aggregator/controller.h
 create mode 100644 drivers/misc/surface_aggregator/core.c
 create mode 100644 drivers/misc/surface_aggregator/ssh_msgb.h
 create mode 100644 drivers/misc/surface_aggregator/ssh_packet_layer.c
 create mode 100644 drivers/misc/surface_aggregator/ssh_packet_layer.h
 create mode 100644 drivers/misc/surface_aggregator/ssh_parser.c
 create mode 100644 drivers/misc/surface_aggregator/ssh_parser.h
 create mode 100644 drivers/misc/surface_aggregator/ssh_request_layer.c
 create mode 100644 drivers/misc/surface_aggregator/ssh_request_layer.h
 create mode 100644 drivers/misc/surface_aggregator/trace.h
 create mode 100644 include/linux/surface_acpi_notify.h
 create mode 100644 include/linux/surface_aggregator/controller.h
 create mode 100644 include/linux/surface_aggregator/device.h
 create mode 100644 include/linux/surface_aggregator/serial_hub.h

-- 
2.28.0

[RFC PATCH 9/9] surface_aggregator: Add Surface ACPI Notify client driver

[Maximilian Luz]

The Surface ACPI Notify (SAN) device provides an ACPI interface to the
Surface Aggregator EC, specifically the Surface Serial Hub interface.
This interface allows EC requests to be made from ACPI code and can
convert a subset of EC events back to ACPI notifications.

Specifically, this interface provides a GenericSerialBus operation
region ACPI code can execute a request by writing the request command
data and payload to this operation region and reading back the
corresponding response via a write-then-read operation. Furthermore,
this interface provides a _DSM method to be called when certain events
from the EC have been received, essentially turning them into ACPI
notifications.

The driver provided in this commit essentially takes care of translating
the request data written to the operation region, executing the request,
waiting for it to finish, and finally writing and translating back the
response (if the request has one). Furthermore, this driver takes care
of enabling the events handled via ACPI _DSM calls. Lastly, this driver
also exposes an interface providing discrete GPU (dGPU) power-on
notifications on the Surface Book 2, which are also received via the
operation region interface (but not handled by the SAN driver directly),
making them accessible to other drivers (such as a dGPU hot-plug driver
that may be added later on).

On 5th and 6th generation Surface devices (Surface Pro 5/2017, Pro 6,
Book 2, Laptop 1 and 2), the SAN interface provides full battery and
thermal subsystem access, as well as other EC based functionality. On
those models, battery and thermal sensor devices are implemented as
standard ACPI devices of that type, however, forward ACPI calls to the
corresponding Surface Aggregator EC request via the SAN interface and
receive corresponding notifications (e.g. battery information change)
from it. This interface is therefore required to provide said
functionality on those devices.

Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
---
 .../surface_aggregator/clients/index.rst      |   1 +
 .../surface_aggregator/clients/san.rst        |  44 +
 MAINTAINERS                                   |   1 +
 .../misc/surface_aggregator/clients/Kconfig   |  20 +
 .../misc/surface_aggregator/clients/Makefile  |   1 +
 .../clients/surface_acpi_notify.c             | 882 ++++++++++++++++++
 include/linux/surface_acpi_notify.h           |  37 +
 7 files changed, 986 insertions(+)
 create mode 100644 Documentation/driver-api/surface_aggregator/clients/san.rst
 create mode 100644 drivers/misc/surface_aggregator/clients/surface_acpi_notify.c
 create mode 100644 include/linux/surface_acpi_notify.h

diff --git a/Documentation/driver-api/surface_aggregator/clients/index.rst b/Documentation/driver-api/surface_aggregator/clients/index.rst
index e47b752f298c..7cd91fc75e91 100644
--- a/Documentation/driver-api/surface_aggregator/clients/index.rst
+++ b/Documentation/driver-api/surface_aggregator/clients/index.rst
@@ -11,6 +11,7 @@ This is the documentation for client drivers themselves. Refer to
    :maxdepth: 1
 
    dbgdev
+   san
 
 .. only::  subproject and html
 
diff --git a/Documentation/driver-api/surface_aggregator/clients/san.rst b/Documentation/driver-api/surface_aggregator/clients/san.rst
new file mode 100644
index 000000000000..f91c0a7ab884
--- /dev/null
+++ b/Documentation/driver-api/surface_aggregator/clients/san.rst
@@ -0,0 +1,44 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+.. |san_client_link| replace:: :c:func:`san_client_link`
+.. |san_dgpu_notifier_register| replace:: :c:func:`san_dgpu_notifier_register`
+.. |san_dgpu_notifier_unregister| replace:: :c:func:`san_dgpu_notifier_unregister`
+
+===================
+Surface ACPI Notify
+===================
+
+The Surface ACPI Notify (SAN) device provides the bridge between ACPI and
+SAM controller. Specifically, ACPI code can execute requests and handle
+battery and thermal events via this interface. In addition to this, events
+relating to the discrete GPU (dGPU) of the Surface Book 2 can be sent from
+ACPI code (note: the Surface Book 3 uses a different method for this). The
+only currently known event sent via this interface is a dGPU power-on
+notification. While this driver handles the former part internally, it only
+relays the dGPU events to any other driver interested via its public API and
+does not handle them.
+
+The public interface of this driver is split into two parts: Client
+registration and notifier-block registration.
+
+A client to the SAN interface can be linked as consumer to the SAN device
+via |san_client_link|. This can be used to ensure that the a client
+receiving dGPU events does not miss any events due to the SAN interface not
+being set up as this forces the client driver to unbind once the SAN driver
+is unbound.
+
+Notifier-blocks can be registered by any device for as long as the module is
+loaded, regardless of being linked as client or not. Registration is done
+with |san_dgpu_notifier_register|. If the notifier is not needed any more, it
+should be unregistered via |san_dgpu_notifier_unregister|.
+
+Consult the API documentation below for more details.
+
+
+API Documentation
+=================
+
+.. kernel-doc:: include/linux/surface_acpi_notify.h
+
+.. kernel-doc:: drivers/misc/surface_aggregator/clients/surface_acpi_notify.c
+    :export:
diff --git a/MAINTAINERS b/MAINTAINERS
index 74122a2a792d..b3550ef15333 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11570,6 +11570,7 @@ W:	https://github.com/linux-surface/surface-aggregator-module
 C:	irc://chat.freenode.net/##linux-surface
 F:	Documentation/driver-api/surface_aggregator/
 F:	drivers/misc/surface_aggregator/
+F:	include/linux/surface_acpi_notify.h
 F:	include/linux/surface_aggregator/
 
 MICROTEK X6 SCANNER
diff --git a/drivers/misc/surface_aggregator/clients/Kconfig b/drivers/misc/surface_aggregator/clients/Kconfig
index dcaa0706074e..e0f63011f079 100644
--- a/drivers/misc/surface_aggregator/clients/Kconfig
+++ b/drivers/misc/surface_aggregator/clients/Kconfig
@@ -16,3 +16,23 @@ config SURFACE_AGGREGATOR_DEBUGFS
 
 	  The provided interface is intended for debugging and development only,
 	  and should not be used otherwise.
+
+config SURFACE_ACPI_NOTIFY
+	tristate "Surface ACPI Notify Driver"
+	depends on SURFACE_AGGREGATOR
+	default m
+	help
+	  Surface ACPI Notify (SAN) driver for Microsoft Surface devices.
+
+	  This driver provides support for the ACPI interface (called SAN) of
+	  the Surface System Aggregator Module (SSAM) EC. This interface is used
+	  on 5th- and 6th-generation Microsoft Surface devices (including
+	  Surface Pro 5 and 6, Surface Book 2, Surface Laptops 1 and 2, and in
+	  reduced functionality on the Surface Laptop 3) to execute SSAM
+	  requests directly from ACPI code, as well as receive SSAM events and
+	  turn them into ACPI notifications. It essentially acts as a
+	  translation layer between the SSAM controller and ACPI.
+
+	  Specifically, this driver may be needed for battery status reporting,
+	  thermal sensor access, and real-time clock information, depending on
+	  the Surface device in question.
diff --git a/drivers/misc/surface_aggregator/clients/Makefile b/drivers/misc/surface_aggregator/clients/Makefile
index c49b2a183d3d..98ed6fb05cf0 100644
--- a/drivers/misc/surface_aggregator/clients/Makefile
+++ b/drivers/misc/surface_aggregator/clients/Makefile
@@ -1,3 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0-or-later
 
 obj-$(CONFIG_SURFACE_AGGREGATOR_DEBUGFS)	+= surface_aggregator_debugfs.o
+obj-$(CONFIG_SURFACE_ACPI_NOTIFY)		+= surface_acpi_notify.o
diff --git a/drivers/misc/surface_aggregator/clients/surface_acpi_notify.c b/drivers/misc/surface_aggregator/clients/surface_acpi_notify.c
new file mode 100644
index 000000000000..67617d9aab57
--- /dev/null
+++ b/drivers/misc/surface_aggregator/clients/surface_acpi_notify.c
@@ -0,0 +1,882 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Driver for the Surface ACPI Notify (SAN) interface/shim.
+ *
+ * Translates communication from ACPI to Surface System Aggregator Module
+ * (SSAM/SAM) requests and back, specifically SAM-over-SSH. Translates SSAM
+ * events back to ACPI notifications. Allows handling of discrete GPU
+ * notifications sent from ACPI via the SAN interface by providing them to any
+ * registered external driver.
+ */
+
+#include <asm/unaligned.h>
+#include <linux/acpi.h>
+#include <linux/delay.h>
+#include <linux/jiffies.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/notifier.h>
+#include <linux/platform_device.h>
+#include <linux/rwsem.h>
+
+#include <linux/surface_aggregator/controller.h>
+#include <linux/surface_acpi_notify.h>
+
+
+struct san_data {
+	struct device *dev;
+	struct ssam_controller *ctrl;
+
+	struct acpi_connection_info info;
+
+	struct ssam_event_notifier nf_bat;
+	struct ssam_event_notifier nf_tmp;
+};
+
+#define to_san_data(ptr, member) \
+	container_of(ptr, struct san_data, member)
+
+
+/* -- dGPU Notifier Interface. ---------------------------------------------- */
+
+struct san_rqsg_if {
+	struct rw_semaphore lock;
+	struct device *dev;
+	struct blocking_notifier_head nh;
+};
+
+static struct san_rqsg_if san_rqsg_if = {
+	.lock = __RWSEM_INITIALIZER(san_rqsg_if.lock),
+	.dev = NULL,
+	.nh = BLOCKING_NOTIFIER_INIT(san_rqsg_if.nh),
+};
+
+static int san_set_rqsg_interface_device(struct device *dev)
+{
+	int status = 0;
+
+	down_write(&san_rqsg_if.lock);
+	if (!san_rqsg_if.dev && dev)
+		san_rqsg_if.dev = dev;
+	else
+		status = -EBUSY;
+	up_write(&san_rqsg_if.lock);
+
+	return status;
+}
+
+/**
+ * san_client_link() - Link client as consumer to SAN device.
+ * @client: The client to link.
+ *
+ * Sets up a device link between the provided client device as consumer and
+ * the SAN device as provider. This function can be used to ensure that the
+ * SAN interface has been set up and will be set up for as long as the driver
+ * of the client device is bound. This guarantees that, during that time, all
+ * dGPU events will be received by any registered notifier.
+ *
+ * The link will be automatically removed once the client device's driver is
+ * unbound.
+ *
+ * Return: Returns zero on succes, %-ENXIO if the SAN interface has not been
+ * set up yet, and %-ENOMEM if device link creation failed.
+ */
+int san_client_link(struct device *client)
+{
+	const u32 flags = DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_CONSUMER;
+	struct device_link *link;
+
+	down_read(&san_rqsg_if.lock);
+
+	if (!san_rqsg_if.dev) {
+		up_read(&san_rqsg_if.lock);
+		return -ENXIO;
+	}
+
+	link = device_link_add(client, san_rqsg_if.dev, flags);
+	if (!link) {
+		up_read(&san_rqsg_if.lock);
+		return -ENOMEM;
+	}
+
+	if (READ_ONCE(link->status) == DL_STATE_SUPPLIER_UNBIND) {
+		up_read(&san_rqsg_if.lock);
+		return -ENXIO;
+	}
+
+	up_read(&san_rqsg_if.lock);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(san_client_link);
+
+/**
+ * san_dgpu_notifier_register() - Register a SAN dGPU notifier.
+ * @nb: The notifier-block to register.
+ *
+ * Registers a SAN dGPU notifier, receiving any new SAN dGPU events sent from
+ * ACPI. The registered notifier will be called with &struct san_dgpu_event
+ * as notifier data and the command ID of that event as notifier action.
+ */
+int san_dgpu_notifier_register(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_register(&san_rqsg_if.nh, nb);
+}
+EXPORT_SYMBOL_GPL(san_dgpu_notifier_register);
+
+/**
+ * san_dgpu_notifier_unregister() - Unregister a SAN dGPU notifier.
+ * @nb: The notifier-block to unregister.
+ */
+int san_dgpu_notifier_unregister(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_unregister(&san_rqsg_if.nh, nb);
+}
+EXPORT_SYMBOL_GPL(san_dgpu_notifier_unregister);
+
+static int san_dgpu_notifier_call(struct san_dgpu_event *evt)
+{
+	int ret;
+
+	ret = blocking_notifier_call_chain(&san_rqsg_if.nh, evt->command, evt);
+	return notifier_to_errno(ret);
+}
+
+
+/* -- ACPI _DSM event relay. ------------------------------------------------ */
+
+#define SAN_DSM_REVISION	0
+
+static const guid_t SAN_DSM_UUID =
+	GUID_INIT(0x93b666c5, 0x70c6, 0x469f, 0xa2, 0x15, 0x3d,
+		  0x48, 0x7c, 0x91, 0xab, 0x3c);
+
+enum san_dsm_event_fn {
+	SAN_DSM_EVENT_FN_BAT1_STAT = 0x03,
+	SAN_DSM_EVENT_FN_BAT1_INFO = 0x04,
+	SAN_DSM_EVENT_FN_ADP1_STAT = 0x05,
+	SAN_DSM_EVENT_FN_ADP1_INFO = 0x06,
+	SAN_DSM_EVENT_FN_BAT2_STAT = 0x07,
+	SAN_DSM_EVENT_FN_BAT2_INFO = 0x08,
+	SAN_DSM_EVENT_FN_THERMAL   = 0x09,
+	SAN_DSM_EVENT_FN_DPTF      = 0x0a,
+};
+
+enum sam_event_cid_bat {
+	SAM_EVENT_CID_BAT_BIX  = 0x15,
+	SAM_EVENT_CID_BAT_BST  = 0x16,
+	SAM_EVENT_CID_BAT_ADP  = 0x17,
+	SAM_EVENT_CID_BAT_PROT = 0x18,
+	SAM_EVENT_CID_BAT_DPTF = 0x4f,
+};
+
+enum sam_event_cid_tmp {
+	SAM_EVENT_CID_TMP_TRIP = 0x0b,
+};
+
+struct san_event_work {
+	struct delayed_work work;
+	struct device *dev;
+	struct ssam_event event;	// must be last
+};
+
+static int san_acpi_notify_event(struct device *dev, u64 func,
+				 union acpi_object *param)
+{
+	acpi_handle san = ACPI_HANDLE(dev);
+	union acpi_object *obj;
+	int status = 0;
+
+	if (!acpi_check_dsm(san, &SAN_DSM_UUID, SAN_DSM_REVISION, 1 << func))
+		return 0;
+
+	dev_dbg(dev, "notify event 0x%02llx\n", func);
+
+	obj = acpi_evaluate_dsm_typed(san, &SAN_DSM_UUID, SAN_DSM_REVISION,
+				      func, param, ACPI_TYPE_BUFFER);
+	if (!obj)
+		return -EFAULT;
+
+	if (obj->buffer.length != 1 || obj->buffer.pointer[0] != 0) {
+		dev_err(dev, "got unexpected result from _DSM\n");
+		status = -EPROTO;
+	}
+
+	ACPI_FREE(obj);
+	return status;
+}
+
+static int san_evt_bat_adp(struct device *dev, const struct ssam_event *event)
+{
+	int status;
+
+	status = san_acpi_notify_event(dev, SAN_DSM_EVENT_FN_ADP1_STAT, NULL);
+	if (status)
+		return status;
+
+	/*
+	 * Enusre that the battery states get updated correctly.
+	 * When the battery is fully charged and an adapter is plugged in, it
+	 * sometimes is not updated correctly, instead showing it as charging.
+	 * Explicitly trigger battery updates to fix this.
+	 */
+
+	status = san_acpi_notify_event(dev, SAN_DSM_EVENT_FN_BAT1_STAT, NULL);
+	if (status)
+		return status;
+
+	return san_acpi_notify_event(dev, SAN_DSM_EVENT_FN_BAT2_STAT, NULL);
+}
+
+static int san_evt_bat_bix(struct device *dev, const struct ssam_event *event)
+{
+	enum san_dsm_event_fn fn;
+
+	if (event->instance_id == 0x02)
+		fn = SAN_DSM_EVENT_FN_BAT2_INFO;
+	else
+		fn = SAN_DSM_EVENT_FN_BAT1_INFO;
+
+	return san_acpi_notify_event(dev, fn, NULL);
+}
+
+static int san_evt_bat_bst(struct device *dev, const struct ssam_event *event)
+{
+	enum san_dsm_event_fn fn;
+
+	if (event->instance_id == 0x02)
+		fn = SAN_DSM_EVENT_FN_BAT2_STAT;
+	else
+		fn = SAN_DSM_EVENT_FN_BAT1_STAT;
+
+	return san_acpi_notify_event(dev, fn, NULL);
+}
+
+static int san_evt_bat_dptf(struct device *dev, const struct ssam_event *event)
+{
+	union acpi_object payload;
+
+	/*
+	 * The Surface ACPI expects a buffer and not a package. It specifically
+	 * checks for ObjectType (Arg3) == 0x03. This will cause a warning in
+	 * acpica/nsarguments.c, but that warning can be safely ignored.
+	 */
+	payload.type = ACPI_TYPE_BUFFER;
+	payload.buffer.length = event->length;
+	payload.buffer.pointer = (u8 *)&event->data[0];
+
+	return san_acpi_notify_event(dev, SAN_DSM_EVENT_FN_DPTF, &payload);
+}
+
+static unsigned long san_evt_bat_delay(u8 cid)
+{
+	switch (cid) {
+	case SAM_EVENT_CID_BAT_ADP:
+		/*
+		 * Wait for battery state to update before signalling adapter
+		 * change.
+		 */
+		return msecs_to_jiffies(5000);
+
+	case SAM_EVENT_CID_BAT_BST:
+		/* Ensure we do not miss anything important due to caching. */
+		return msecs_to_jiffies(2000);
+
+	default:
+		return 0;
+	}
+}
+
+static bool san_evt_bat(const struct ssam_event *event, struct device *dev)
+{
+	int status;
+
+	switch (event->command_id) {
+	case SAM_EVENT_CID_BAT_BIX:
+		status = san_evt_bat_bix(dev, event);
+		break;
+
+	case SAM_EVENT_CID_BAT_BST:
+		status = san_evt_bat_bst(dev, event);
+		break;
+
+	case SAM_EVENT_CID_BAT_ADP:
+		status = san_evt_bat_adp(dev, event);
+		break;
+
+	case SAM_EVENT_CID_BAT_PROT:
+		/*
+		 * TODO: Implement support for battery protection status change
+		 *       event.
+		 */
+		return true;
+
+	case SAM_EVENT_CID_BAT_DPTF:
+		status = san_evt_bat_dptf(dev, event);
+		break;
+
+	default:
+		return false;
+	}
+
+	if (status)
+		dev_err(dev, "error handling power event (cid = %x)\n",
+			event->command_id);
+
+	return true;
+}
+
+static void san_evt_bat_workfn(struct work_struct *work)
+{
+	struct san_event_work *ev;
+
+	ev = container_of(work, struct san_event_work, work.work);
+	san_evt_bat(&ev->event, ev->dev);
+	kfree(ev);
+}
+
+static u32 san_evt_bat_nf(struct ssam_event_notifier *nf,
+			  const struct ssam_event *event)
+{
+	struct san_data *d = to_san_data(nf, nf_bat);
+	struct san_event_work *work;
+	unsigned long delay = san_evt_bat_delay(event->command_id);
+
+	if (delay == 0)
+		return san_evt_bat(event, d->dev) ? SSAM_NOTIF_HANDLED : 0;
+
+	work = kzalloc(sizeof(*work) + event->length, GFP_KERNEL);
+	if (!work)
+		return ssam_notifier_from_errno(-ENOMEM);
+
+	INIT_DELAYED_WORK(&work->work, san_evt_bat_workfn);
+	work->dev = d->dev;
+
+	memcpy(&work->event, event, sizeof(struct ssam_event) + event->length);
+
+	schedule_delayed_work(&work->work, delay);
+	return SSAM_NOTIF_HANDLED;
+}
+
+static int san_evt_tmp_trip(struct device *dev, const struct ssam_event *event)
+{
+	union acpi_object param;
+
+	/*
+	 * The Surface ACPI expects an integer and not a package. This will
+	 * cause a warning in acpica/nsarguments.c, but that warning can be
+	 * safely ignored.
+	 */
+	param.type = ACPI_TYPE_INTEGER;
+	param.integer.value = event->instance_id;
+
+	return san_acpi_notify_event(dev, SAN_DSM_EVENT_FN_THERMAL, &param);
+}
+
+static bool san_evt_tmp(const struct ssam_event *event, struct device *dev)
+{
+	int status;
+
+	switch (event->command_id) {
+	case SAM_EVENT_CID_TMP_TRIP:
+		status = san_evt_tmp_trip(dev, event);
+		break;
+
+	default:
+		return false;
+	}
+
+	if (status) {
+		dev_err(dev, "error handling thermal event (cid = %x)\n",
+			event->command_id);
+	}
+
+	return true;
+}
+
+static u32 san_evt_tmp_nf(struct ssam_event_notifier *nf,
+			  const struct ssam_event *event)
+{
+	struct san_data *d = to_san_data(nf, nf_bat);
+
+	return san_evt_tmp(event, d->dev) ? SSAM_NOTIF_HANDLED : 0;
+}
+
+
+/* -- ACPI GSB OperationRegion Handler -------------------------------------- */
+
+struct gsb_data_in {
+	u8 cv;
+} __packed;
+
+struct gsb_data_rqsx {
+	u8 cv;				// command value (san_gsb_request_cv)
+	u8 tc;				// target category
+	u8 tid;				// target ID
+	u8 iid;				// instance ID
+	u8 snc;				// expect-response-flag?
+	u8 cid;				// command ID
+	u16 cdl;			// payload length
+	u8 pld[];			// payload
+} __packed;
+
+struct gsb_data_etwl {
+	u8 cv;				// command value (should be 0x02)
+	u8 etw3;			// unknown
+	u8 etw4;			// unknown
+	u8 msg[];			// error message (ASCIIZ)
+} __packed;
+
+struct gsb_data_out {
+	u8 status;			// _SSH communication status
+	u8 len;				// _SSH payload length
+	u8 pld[];			// _SSH payload
+} __packed;
+
+union gsb_buffer_data {
+	struct gsb_data_in   in;	// common input
+	struct gsb_data_rqsx rqsx;	// RQSX input
+	struct gsb_data_etwl etwl;	// ETWL input
+	struct gsb_data_out  out;	// output
+};
+
+struct gsb_buffer {
+	u8 status;			// GSB AttribRawProcess status
+	u8 len;				// GSB AttribRawProcess length
+	union gsb_buffer_data data;
+} __packed;
+
+#define SAN_GSB_MAX_RQSX_PAYLOAD  (U8_MAX - 2 - sizeof(struct gsb_data_rqsx))
+#define SAN_GSB_MAX_RESPONSE	  (U8_MAX - 2 - sizeof(struct gsb_data_out))
+
+#define SAN_GSB_COMMAND		0
+
+enum san_gsb_request_cv {
+	SAN_GSB_REQUEST_CV_RQST = 0x01,
+	SAN_GSB_REQUEST_CV_ETWL = 0x02,
+	SAN_GSB_REQUEST_CV_RQSG = 0x03,
+};
+
+#define SAN_REQUEST_NUM_TRIES	5
+
+static acpi_status san_etwl(struct san_data *d, struct gsb_buffer *b)
+{
+	struct gsb_data_etwl *etwl = &b->data.etwl;
+
+	if (b->len < sizeof(struct gsb_data_etwl)) {
+		dev_err(d->dev, "invalid ETWL package (len = %d)\n", b->len);
+		return AE_OK;
+	}
+
+	dev_err(d->dev, "ETWL(0x%02x, 0x%02x): %.*s\n", etwl->etw3, etwl->etw4,
+		(unsigned int)(b->len - sizeof(struct gsb_data_etwl)),
+		(char *)etwl->msg);
+
+	// indicate success
+	b->status = 0x00;
+	b->len = 0x00;
+
+	return AE_OK;
+}
+
+static struct gsb_data_rqsx *san_validate_rqsx(struct device *dev,
+		const char *type, struct gsb_buffer *b)
+{
+	struct gsb_data_rqsx *rqsx = &b->data.rqsx;
+
+	if (b->len < sizeof(struct gsb_data_rqsx)) {
+		dev_err(dev, "invalid %s package (len = %d)\n", type, b->len);
+		return NULL;
+	}
+
+	if (get_unaligned(&rqsx->cdl) != b->len - sizeof(struct gsb_data_rqsx)) {
+		dev_err(dev, "bogus %s package (len = %d, cdl = %d)\n",
+			type, b->len, get_unaligned(&rqsx->cdl));
+		return NULL;
+	}
+
+	if (get_unaligned(&rqsx->cdl) > SAN_GSB_MAX_RQSX_PAYLOAD) {
+		dev_err(dev, "payload for %s package too large (cdl = %d)\n",
+			type, get_unaligned(&rqsx->cdl));
+		return NULL;
+	}
+
+	return rqsx;
+}
+
+static void gsb_rqsx_response_error(struct gsb_buffer *gsb, int status)
+{
+	gsb->status = 0x00;
+	gsb->len = 0x02;
+	gsb->data.out.status = (u8)(-status);
+	gsb->data.out.len = 0x00;
+}
+
+static void gsb_rqsx_response_success(struct gsb_buffer *gsb, u8 *ptr, size_t len)
+{
+	gsb->status = 0x00;
+	gsb->len = len + 2;
+	gsb->data.out.status = 0x00;
+	gsb->data.out.len = len;
+
+	if (len)
+		memcpy(&gsb->data.out.pld[0], ptr, len);
+}
+
+static acpi_status san_rqst_fixup_suspended(struct san_data *d,
+					    struct ssam_request *rqst,
+					    struct gsb_buffer *gsb)
+{
+	if (rqst->target_category == SSAM_SSH_TC_BAS && rqst->command_id == 0x0D) {
+		u8 base_state = 1;
+
+		/* Base state quirk:
+		 * The base state may be queried from ACPI when the EC is still
+		 * suspended. In this case it will return '-EPERM'. This query
+		 * will only be triggered from the ACPI lid GPE interrupt, thus
+		 * we are either in laptop or studio mode (base status 0x01 or
+		 * 0x02). Furthermore, we will only get here if the device (and
+		 * EC) have been suspended.
+		 *
+		 * We now assume that the device is in laptop mode (0x01). This
+		 * has the drawback that it will wake the device when unfolding
+		 * it in studio mode, but it also allows us to avoid actively
+		 * waiting for the EC to wake up, which may incur a notable
+		 * delay.
+		 */
+
+		dev_dbg(d->dev, "rqst: fixup: base-state quirk\n");
+
+		gsb_rqsx_response_success(gsb, &base_state, sizeof(base_state));
+		return AE_OK;
+	}
+
+	gsb_rqsx_response_error(gsb, -ENXIO);
+	return AE_OK;
+}
+
+static acpi_status san_rqst(struct san_data *d, struct gsb_buffer *buffer)
+{
+	u8 rspbuf[SAN_GSB_MAX_RESPONSE];
+	struct gsb_data_rqsx *gsb_rqst;
+	struct ssam_request rqst;
+	struct ssam_response rsp;
+	int status = 0;
+
+	gsb_rqst = san_validate_rqsx(d->dev, "RQST", buffer);
+	if (!gsb_rqst)
+		return AE_OK;
+
+	rqst.target_category = gsb_rqst->tc;
+	rqst.target_id = gsb_rqst->tid;
+	rqst.command_id = gsb_rqst->cid;
+	rqst.instance_id = gsb_rqst->iid;
+	rqst.flags = gsb_rqst->snc ? SSAM_REQUEST_HAS_RESPONSE : 0;
+	rqst.length = get_unaligned(&gsb_rqst->cdl);
+	rqst.payload = &gsb_rqst->pld[0];
+
+	rsp.capacity = ARRAY_SIZE(rspbuf);
+	rsp.length = 0;
+	rsp.pointer = &rspbuf[0];
+
+	// handle suspended device
+	if (d->dev->power.is_suspended) {
+		dev_warn(d->dev, "rqst: device is suspended, not executing\n");
+		return san_rqst_fixup_suspended(d, &rqst, buffer);
+	}
+
+	status = ssam_retry(ssam_request_sync_onstack, SAN_REQUEST_NUM_TRIES,
+			    d->ctrl, &rqst, &rsp, SAN_GSB_MAX_RQSX_PAYLOAD);
+
+	if (!status) {
+		gsb_rqsx_response_success(buffer, rsp.pointer, rsp.length);
+	} else {
+		dev_err(d->dev, "rqst: failed with error %d\n", status);
+		gsb_rqsx_response_error(buffer, status);
+	}
+
+	return AE_OK;
+}
+
+static acpi_status san_rqsg(struct san_data *d, struct gsb_buffer *buffer)
+{
+	struct gsb_data_rqsx *gsb_rqsg;
+	struct san_dgpu_event evt;
+	int status;
+
+	gsb_rqsg = san_validate_rqsx(d->dev, "RQSG", buffer);
+	if (!gsb_rqsg)
+		return AE_OK;
+
+	evt.category = gsb_rqsg->tc;
+	evt.target = gsb_rqsg->tid;
+	evt.command = gsb_rqsg->cid;
+	evt.instance = gsb_rqsg->iid;
+	evt.length = get_unaligned(&gsb_rqsg->cdl);
+	evt.payload = &gsb_rqsg->pld[0];
+
+	status = san_dgpu_notifier_call(&evt);
+	if (!status) {
+		gsb_rqsx_response_success(buffer, NULL, 0);
+	} else {
+		dev_err(d->dev, "rqsg: failed with error %d\n", status);
+		gsb_rqsx_response_error(buffer, status);
+	}
+
+	return AE_OK;
+}
+
+static acpi_status san_opreg_handler(u32 function,
+		acpi_physical_address command, u32 bits, u64 *value64,
+		void *opreg_context, void *region_context)
+{
+	struct san_data *d = to_san_data(opreg_context, info);
+	struct gsb_buffer *buffer = (struct gsb_buffer *)value64;
+	int accessor_type = (function & 0xFFFF0000) >> 16;
+
+	if (command != SAN_GSB_COMMAND) {
+		dev_warn(d->dev, "unsupported command: 0x%02llx\n", command);
+		return AE_OK;
+	}
+
+	if (accessor_type != ACPI_GSB_ACCESS_ATTRIB_RAW_PROCESS) {
+		dev_err(d->dev, "invalid access type: 0x%02x\n", accessor_type);
+		return AE_OK;
+	}
+
+	// buffer must have at least contain the command-value
+	if (buffer->len == 0) {
+		dev_err(d->dev, "request-package too small\n");
+		return AE_OK;
+	}
+
+	switch (buffer->data.in.cv) {
+	case SAN_GSB_REQUEST_CV_RQST:
+		return san_rqst(d, buffer);
+
+	case SAN_GSB_REQUEST_CV_ETWL:
+		return san_etwl(d, buffer);
+
+	case SAN_GSB_REQUEST_CV_RQSG:
+		return san_rqsg(d, buffer);
+
+	default:
+		dev_warn(d->dev, "unsupported SAN0 request (cv: 0x%02x)\n",
+			 buffer->data.in.cv);
+		return AE_OK;
+	}
+}
+
+
+/* -- Driver setup. --------------------------------------------------------- */
+
+static int san_events_register(struct platform_device *pdev)
+{
+	struct san_data *d = platform_get_drvdata(pdev);
+	int status;
+
+	d->nf_bat.base.priority = 1;
+	d->nf_bat.base.fn = san_evt_bat_nf;
+	d->nf_bat.event.reg = SSAM_EVENT_REGISTRY_SAM;
+	d->nf_bat.event.id.target_category = SSAM_SSH_TC_BAT;
+	d->nf_bat.event.id.instance = 0;
+	d->nf_bat.event.mask = SSAM_EVENT_MASK_TARGET;
+	d->nf_bat.event.flags = SSAM_EVENT_SEQUENCED;
+
+	d->nf_tmp.base.priority = 1;
+	d->nf_tmp.base.fn = san_evt_tmp_nf;
+	d->nf_tmp.event.reg = SSAM_EVENT_REGISTRY_SAM;
+	d->nf_tmp.event.id.target_category = SSAM_SSH_TC_TMP;
+	d->nf_tmp.event.id.instance = 0;
+	d->nf_tmp.event.mask = SSAM_EVENT_MASK_TARGET;
+	d->nf_tmp.event.flags = SSAM_EVENT_SEQUENCED;
+
+	status = ssam_notifier_register(d->ctrl, &d->nf_bat);
+	if (status)
+		return status;
+
+	status = ssam_notifier_register(d->ctrl, &d->nf_tmp);
+	if (status)
+		ssam_notifier_unregister(d->ctrl, &d->nf_bat);
+
+	return status;
+}
+
+static void san_events_unregister(struct platform_device *pdev)
+{
+	struct san_data *d = platform_get_drvdata(pdev);
+
+	ssam_notifier_unregister(d->ctrl, &d->nf_bat);
+	ssam_notifier_unregister(d->ctrl, &d->nf_tmp);
+}
+
+#define san_consumer_printk(level, dev, handle, fmt, ...)			\
+do {										\
+	char *path = "<error getting consumer path>";				\
+	struct acpi_buffer buffer = {						\
+		.length = ACPI_ALLOCATE_BUFFER,					\
+		.pointer = NULL,						\
+	};									\
+										\
+	if (ACPI_SUCCESS(acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer)))	\
+		path = buffer.pointer;						\
+										\
+	dev_##level(dev, "[%s]: " fmt, path, ##__VA_ARGS__);			\
+	kfree(buffer.pointer);							\
+} while (0)
+
+#define san_consumer_dbg(dev, handle, fmt, ...) \
+	san_consumer_printk(dbg, dev, handle, fmt, ##__VA_ARGS__)
+
+#define san_consumer_warn(dev, handle, fmt, ...) \
+	san_consumer_printk(warn, dev, handle, fmt, ##__VA_ARGS__)
+
+static bool is_san_consumer(struct platform_device *pdev, acpi_handle handle)
+{
+	struct acpi_handle_list dep_devices;
+	acpi_handle supplier = ACPI_HANDLE(&pdev->dev);
+	acpi_status status;
+	int i;
+
+	if (!acpi_has_method(handle, "_DEP"))
+		return false;
+
+	status = acpi_evaluate_reference(handle, "_DEP", NULL, &dep_devices);
+	if (ACPI_FAILURE(status)) {
+		san_consumer_dbg(&pdev->dev, handle, "failed to evaluate _DEP\n");
+		return false;
+	}
+
+	for (i = 0; i < dep_devices.count; i++) {
+		if (dep_devices.handles[i] == supplier)
+			return true;
+	}
+
+	return false;
+}
+
+static acpi_status san_consumer_setup(acpi_handle handle, u32 lvl,
+				      void *context, void **rv)
+{
+	const u32 flags = DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_SUPPLIER;
+	struct platform_device *pdev = context;
+	struct acpi_device *adev;
+	struct device_link *link;
+
+	if (!is_san_consumer(pdev, handle))
+		return AE_OK;
+
+	// ignore ACPI devices that are not present
+	if (acpi_bus_get_device(handle, &adev) != 0)
+		return AE_OK;
+
+	san_consumer_dbg(&pdev->dev, handle, "creating device link\n");
+
+	// try to set up device links, ignore but log errors
+	link = device_link_add(&adev->dev, &pdev->dev, flags);
+	if (!link) {
+		san_consumer_warn(&pdev->dev, handle,
+				  "failed to create device link\n");
+		return AE_OK;
+	}
+
+	return AE_OK;
+}
+
+static int san_consumer_links_setup(struct platform_device *pdev)
+{
+	acpi_status status;
+
+	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
+				     ACPI_UINT32_MAX, san_consumer_setup, NULL,
+				     pdev, NULL);
+
+	return status ? -EFAULT : 0;
+}
+
+static int san_probe(struct platform_device *pdev)
+{
+	acpi_handle san = ACPI_HANDLE(&pdev->dev);
+	struct ssam_controller *ctrl;
+	struct san_data *data;
+	acpi_status astatus;
+	int status;
+
+	status = ssam_client_bind(&pdev->dev, &ctrl);
+	if (status)
+		return status == -ENXIO ? -EPROBE_DEFER : status;
+
+	status = san_consumer_links_setup(pdev);
+	if (status)
+		return status;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->dev = &pdev->dev;
+	data->ctrl = ctrl;
+
+	platform_set_drvdata(pdev, data);
+
+	astatus = acpi_install_address_space_handler(san, ACPI_ADR_SPACE_GSBUS,
+			&san_opreg_handler, NULL, &data->info);
+	if (ACPI_FAILURE(astatus))
+		return -ENXIO;
+
+	status = san_events_register(pdev);
+	if (status)
+		goto err_enable_events;
+
+	status = san_set_rqsg_interface_device(&pdev->dev);
+	if (status)
+		goto err_install_dev;
+
+	acpi_walk_dep_device_list(san);
+	return 0;
+
+err_install_dev:
+	san_events_unregister(pdev);
+err_enable_events:
+	acpi_remove_address_space_handler(san, ACPI_ADR_SPACE_GSBUS,
+					  &san_opreg_handler);
+	return status;
+}
+
+static int san_remove(struct platform_device *pdev)
+{
+	acpi_handle san = ACPI_HANDLE(&pdev->dev);
+
+	san_set_rqsg_interface_device(NULL);
+	acpi_remove_address_space_handler(san, ACPI_ADR_SPACE_GSBUS,
+					  &san_opreg_handler);
+	san_events_unregister(pdev);
+
+	/*
+	 * We have unregistered our event sources. Now we need to ensure that
+	 * all delayed works they may have spawned are run to completion.
+	 */
+	flush_scheduled_work();
+
+	return 0;
+}
+
+static const struct acpi_device_id san_match[] = {
+	{ "MSHW0091" },
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, san_match);
+
+static struct platform_driver surface_acpi_notify = {
+	.probe = san_probe,
+	.remove = san_remove,
+	.driver = {
+		.name = "surface_acpi_notify",
+		.acpi_match_table = san_match,
+		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
+	},
+};
+module_platform_driver(surface_acpi_notify);
+
+MODULE_AUTHOR("Maximilian Luz <luzmaximilian@gmail.com>");
+MODULE_DESCRIPTION("Surface ACPI Notify driver for Surface System Aggregator Module");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/surface_acpi_notify.h b/include/linux/surface_acpi_notify.h
new file mode 100644
index 000000000000..ee5e04f2eb48
--- /dev/null
+++ b/include/linux/surface_acpi_notify.h
@@ -0,0 +1,37 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Interface for Surface ACPI Notify (SAN) driver.
+ *
+ * Provides access to discrete GPU notifications sent from ACPI via the SAN
+ * driver, which are not handled by this driver directly.
+ */
+
+#ifndef _LINUX_SURFACE_ACPI_NOTIFY_H
+#define _LINUX_SURFACE_ACPI_NOTIFY_H
+
+#include <linux/notifier.h>
+#include <linux/types.h>
+
+/**
+ * struct san_dgpu_event - Discrete GPU ACPI event.
+ * @category: Category of the event.
+ * @target:   Target ID of the event source.
+ * @command:  Command ID of the event.
+ * @instance: Instance ID of the event source.
+ * @length:   Length of the event's payload data (in bytes).
+ * @payload:  Pointer to the event's payload data.
+ */
+struct san_dgpu_event {
+	u8 category;
+	u8 target;
+	u8 command;
+	u8 instance;
+	u16 length;
+	u8 *payload;
+};
+
+int san_client_link(struct device *client);
+int san_dgpu_notifier_register(struct notifier_block *nb);
+int san_dgpu_notifier_unregister(struct notifier_block *nb);
+
+#endif /* _LINUX_SURFACE_ACPI_NOTIFY_H */
-- 
2.28.0

Re: [RFC PATCH 0/9] Add support for Microsoft Surface System Aggregator Module

[Arnd Bergmann]

On Wed, Sep 23, 2020 at 5:15 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
>
> Hello,
>
> The Surface System Aggregator Module (we'll refer to it as Surface
> Aggregator or SAM below) is an embedded controller (EC) found on various
> Microsoft Surface devices. Specifically, all 4th and later generation
> Surface devices, i.e. Surface Pro 4, Surface Book 1 and later, with the
> exception of the Surface Go series and the Surface Duo. Notably, it
> seems like this EC can also be found on the ARM-based Surface Pro X [1].

I think this should go to drivers/platform/x86 or drivers/platform/surface/
along with other laptop vendor specific code rather than drivers/misc/.

I'll have a look at the code myself, but I'd prefer to have the maintainers
for the other laptop drivers review this properly.

       Arnd

Re: [RFC PATCH 0/9] Add support for Microsoft Surface System Aggregator Module

[Maximilian Luz]

On 9/23/20 5:30 PM, Arnd Bergmann wrote:
> On Wed, Sep 23, 2020 at 5:15 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
>>
>> Hello,
>>
>> The Surface System Aggregator Module (we'll refer to it as Surface
>> Aggregator or SAM below) is an embedded controller (EC) found on various
>> Microsoft Surface devices. Specifically, all 4th and later generation
>> Surface devices, i.e. Surface Pro 4, Surface Book 1 and later, with the
>> exception of the Surface Go series and the Surface Duo. Notably, it
>> seems like this EC can also be found on the ARM-based Surface Pro X [1].
> 
> I think this should go to drivers/platform/x86 or drivers/platform/surface/
> along with other laptop vendor specific code rather than drivers/misc/.

I initially had this under drivers/platform/x86. There are two main
reasons I changed that: First, I think it's a bit too big for
platform/x86 given that it basically introduces a new subsystem. At this
point it's really less of "a couple of odd devices here and there" and
more of a bus-type thing. Second, with the possibility of future support
for ARM devices (Pro X, Pro X 2 which is rumored to come out soon), I
thought that platform/x86 would not be a good fit.

I'd be happy to move this to platform/surface though, if that's
considered a better fit and you're okay with me adding that. Would make
sense given that there's already a platform/chrome, which, as far as I
can tell, also seems to be mainly focused on EC support.

> I'll have a look at the code myself, but I'd prefer to have the maintainers
> for the other laptop drivers review this properly.

Thanks! I'll CC them for the next version.

Regards,
Max

Re: [RFC PATCH 0/9] Add support for Microsoft Surface System Aggregator Module

[Arnd Bergmann]

On Wed, Sep 23, 2020 at 5:43 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
>
> On 9/23/20 5:30 PM, Arnd Bergmann wrote:
> > On Wed, Sep 23, 2020 at 5:15 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
> >>
> >> Hello,
> >>
> >> The Surface System Aggregator Module (we'll refer to it as Surface
> >> Aggregator or SAM below) is an embedded controller (EC) found on various
> >> Microsoft Surface devices. Specifically, all 4th and later generation
> >> Surface devices, i.e. Surface Pro 4, Surface Book 1 and later, with the
> >> exception of the Surface Go series and the Surface Duo. Notably, it
> >> seems like this EC can also be found on the ARM-based Surface Pro X [1].
> >
> > I think this should go to drivers/platform/x86 or drivers/platform/surface/
> > along with other laptop vendor specific code rather than drivers/misc/.
>
> I initially had this under drivers/platform/x86. There are two main
> reasons I changed that: First, I think it's a bit too big for
> platform/x86 given that it basically introduces a new subsystem. At this
> point it's really less of "a couple of odd devices here and there" and
> more of a bus-type thing. Second, with the possibility of future support
> for ARM devices (Pro X, Pro X 2 which is rumored to come out soon), I
> thought that platform/x86 would not be a good fit.

I don't see that as a strong reason against it. As you write yourself, the
driver won't work on the arm machines without major changes anyway,
and even if it does, it fits much better with the rest of it.

If you are worried about the size of the directory,
drivers/platform/x86/surface/
would also work.

> I'd be happy to move this to platform/surface though, if that's
> considered a better fit and you're okay with me adding that. Would make
> sense given that there's already a platform/chrome, which, as far as I
> can tell, also seems to be mainly focused on EC support.

Yes, I think the main question is how much overlap you see functionally
between this driver and the others in drivers/platform/x86.

      Arnd

Re: [RFC PATCH 0/9] Add support for Microsoft Surface System Aggregator Module

[Maximilian Luz]

On 9/23/20 9:43 PM, Arnd Bergmann wrote:
> On Wed, Sep 23, 2020 at 5:43 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
>>
>> On 9/23/20 5:30 PM, Arnd Bergmann wrote:
>>> On Wed, Sep 23, 2020 at 5:15 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
>>>>
>>>> Hello,
>>>>
>>>> The Surface System Aggregator Module (we'll refer to it as Surface
>>>> Aggregator or SAM below) is an embedded controller (EC) found on various
>>>> Microsoft Surface devices. Specifically, all 4th and later generation
>>>> Surface devices, i.e. Surface Pro 4, Surface Book 1 and later, with the
>>>> exception of the Surface Go series and the Surface Duo. Notably, it
>>>> seems like this EC can also be found on the ARM-based Surface Pro X [1].
>>>
>>> I think this should go to drivers/platform/x86 or drivers/platform/surface/
>>> along with other laptop vendor specific code rather than drivers/misc/.
>>
>> I initially had this under drivers/platform/x86. There are two main
>> reasons I changed that: First, I think it's a bit too big for
>> platform/x86 given that it basically introduces a new subsystem. At this
>> point it's really less of "a couple of odd devices here and there" and
>> more of a bus-type thing. Second, with the possibility of future support
>> for ARM devices (Pro X, Pro X 2 which is rumored to come out soon), I
>> thought that platform/x86 would not be a good fit.
> 
> I don't see that as a strong reason against it. As you write yourself, the
> driver won't work on the arm machines without major changes anyway,
> and even if it does, it fits much better with the rest of it.

Sorry, I should have written that a bit more clearly. I don't see any
reason why these drivers would not work on an ARM device such as the Pro
X right now, assuming that it boots via ACPI and the serial device it
loads against is fully functional.

The reason (at least as far as I know) it currently hasn't been tested
is that a) there aren't a lot of people around attempting to run Linux
on the currently only ARM device with that and b) it's currently blocked
by a reason unrelated to this driver itself, specifically that the
serial controller isn't being set up and thus the core driver doesn't
have a device it can attach to. My information may be outdated though
and is pretty much exclusively based on
https://github.com/Sonicadvance1/linux/issues/7.

> If you are worried about the size of the directory,
> drivers/platform/x86/surface/
> would also work.

This was the alternative I'd have considered without ARM devices.

>> I'd be happy to move this to platform/surface though, if that's
>> considered a better fit and you're okay with me adding that. Would make
>> sense given that there's already a platform/chrome, which, as far as I
>> can tell, also seems to be mainly focused on EC support.
> 
> Yes, I think the main question is how much overlap you see functionally
> between this driver and the others in drivers/platform/x86.

I think that the Pro X likely won't be the last ARM Surface device with
a SAM EC. Further, the subsystem is going to grow, and platform/x86
seems more like a collection of, if at all, loosely connected drivers,
which might give off the wrong impression. In my mind, this is just a
bit more comparable to platform/chrome than the rest of platform/x86. I
don't think I'm really qualified to make the decision on that though,
that's just my opinion.

Here's an overview of other drivers that I hopefully at some point get
in good enough shape, which are part of this subsystem/dependent on the
EC API introduced here:

- A device registry / device hub for devices that are connected to the
   EC but can't be detected via ACPI.

- A dedicated battery driver for 7th generation devices (where the
   battery isn't hanled via the ACPI shim).

- A driver properly handling clipboard detachment on the Surface Books.

- A driver for HID input/transport on the Surface Laptops and Surface
   Book 3.

- A driver for allowing users to set the performance/cooling mode via
   sysfs.

- Possibly a driver improving hot-plug handling of the discrete GPU in
   the Surface Book base.

And also some stuff that hasn't been written yet:

- A dedicated driver for temperature sensors handled via the EC on 7th
   generation devices (also handled via the ACPI shim on previous
   generations).

- Possibly a driver for real-time-clock access on 7th generation
   devices (it has yet to be tested if that interface is still
   around/required on those devices; that's also a thing handled via
   the ACPI shim on previous generations).

I doubt that those client drivers will be exclusive to x86, and I could
see (current and future) ARM devices using SAM based battery,
keyboard/HID, and performance mode drivers (which will likely also
require the device registry, because for some reason MS doesn't want to
describe those devices in ACPI on the newer generations any more...).
All of those should work as-is on ARM (or at least after the
corresponding device entries have been added to the device registry),
modulo bugs of course.

I hope this all gives a better overview of the form this may eventually
take on and helps you in your decision. I'd be completely happy to move
it to either, platform/surface or platform/x86/surface, whatever the
consensus is. I'd very much like to keep the client drivers all
contained to one sub-directory, though, and not scattered all over
platform/x86/surface_*.c. Again that's more of a personal preference
though :)

Thanks,
Max

Re: [RFC PATCH 0/9] Add support for Microsoft Surface System Aggregator Module

[Arnd Bergmann]

On Thu, Sep 24, 2020 at 1:28 AM Maximilian Luz <luzmaximilian@gmail.com> wrote:
> On 9/23/20 9:43 PM, Arnd Bergmann wrote:
> > On Wed, Sep 23, 2020 at 5:43 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
> >>
> >> On 9/23/20 5:30 PM, Arnd Bergmann wrote:
> >>> On Wed, Sep 23, 2020 at 5:15 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
> >>>>
> >>>> Hello,
> >>>>
> >>>> The Surface System Aggregator Module (we'll refer to it as Surface
> >>>> Aggregator or SAM below) is an embedded controller (EC) found on various
> >>>> Microsoft Surface devices. Specifically, all 4th and later generation
> >>>> Surface devices, i.e. Surface Pro 4, Surface Book 1 and later, with the
> >>>> exception of the Surface Go series and the Surface Duo. Notably, it
> >>>> seems like this EC can also be found on the ARM-based Surface Pro X [1].
> >>>
> >>> I think this should go to drivers/platform/x86 or drivers/platform/surface/
> >>> along with other laptop vendor specific code rather than drivers/misc/.
> >>
> >> I initially had this under drivers/platform/x86. There are two main
> >> reasons I changed that: First, I think it's a bit too big for
> >> platform/x86 given that it basically introduces a new subsystem. At this
> >> point it's really less of "a couple of odd devices here and there" and
> >> more of a bus-type thing. Second, with the possibility of future support
> >> for ARM devices (Pro X, Pro X 2 which is rumored to come out soon), I
> >> thought that platform/x86 would not be a good fit.
> >
> > I don't see that as a strong reason against it. As you write yourself, the
> > driver won't work on the arm machines without major changes anyway,
> > and even if it does, it fits much better with the rest of it.
>
> Sorry, I should have written that a bit more clearly. I don't see any
> reason why these drivers would not work on an ARM device such as the Pro
> X right now, assuming that it boots via ACPI and the serial device it
> loads against is fully functional.

As I understand, the dialect of ACPI used on the snapdragon laptops
is not really compatible with the subset expected by the kernel, so
you'd be more likely to run those laptops with a device tree description
of the hardware instead (if at all).

Making the driver talk to the hardware directly instead of going through
AML likely requires more refactoring.

> >> I'd be happy to move this to platform/surface though, if that's
> >> considered a better fit and you're okay with me adding that. Would make
> >> sense given that there's already a platform/chrome, which, as far as I
> >> can tell, also seems to be mainly focused on EC support.
> >
> > Yes, I think the main question is how much overlap you see functionally
> > between this driver and the others in drivers/platform/x86.
>
> I think that the Pro X likely won't be the last ARM Surface device with
> a SAM EC. Further, the subsystem is going to grow, and platform/x86
> seems more like a collection of, if at all, loosely connected drivers,
> which might give off the wrong impression. In my mind, this is just a
> bit more comparable to platform/chrome than the rest of platform/x86. I
> don't think I'm really qualified to make the decision on that though,
> that's just my opinion.

I would ask the drivers/platform/x86 maintainers for an opinion here,
they are probably best qualified to make that decision.

I don't really mind either way, for me this is more about who is
responsible as a subsystem maintainer than whether these are
technically x86 or not.

> Here's an overview of other drivers that I hopefully at some point get
> in good enough shape, which are part of this subsystem/dependent on the
> EC API introduced here:
>
> - A device registry / device hub for devices that are connected to the
>    EC but can't be detected via ACPI.
>
> - A dedicated battery driver for 7th generation devices (where the
>    battery isn't hanled via the ACPI shim).
>
> - A driver properly handling clipboard detachment on the Surface Books.
>
> - A driver for HID input/transport on the Surface Laptops and Surface
>    Book 3.
>
> - A driver for allowing users to set the performance/cooling mode via
>    sysfs.
>
> - Possibly a driver improving hot-plug handling of the discrete GPU in
>    the Surface Book base.

Note that drivers that connect to the bus typically don't live in the
same subdirectory as the driver that operates the bus. E.g. the
battery driver would go into drivers/power/supply and the input
would go into drivers/input/ or drivers/hid.

    Arnd

Re: [RFC PATCH 0/9] Add support for Microsoft Surface System Aggregator Module

[Andy Shevchenko]

On Wed, Sep 23, 2020 at 6:32 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Wed, Sep 23, 2020 at 5:15 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
> >
> > Hello,
> >
> > The Surface System Aggregator Module (we'll refer to it as Surface
> > Aggregator or SAM below) is an embedded controller (EC) found on various
> > Microsoft Surface devices. Specifically, all 4th and later generation
> > Surface devices, i.e. Surface Pro 4, Surface Book 1 and later, with the
> > exception of the Surface Go series and the Surface Duo. Notably, it
> > seems like this EC can also be found on the ARM-based Surface Pro X [1].
>
> I think this should go to drivers/platform/x86 or drivers/platform/surface/
> along with other laptop vendor specific code rather than drivers/misc/.

+1 here. drivers/platform/surface is a good place to start.
And you may begin with moving a few Surface drivers out of PDx86 to
the new folder.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 0/9] Add support for Microsoft Surface System Aggregator Module

[Maximilian Luz]

On 9/24/20 10:26 AM, Arnd Bergmann wrote:
> On Thu, Sep 24, 2020 at 1:28 AM Maximilian Luz <luzmaximilian@gmail.com> wrote:
>> On 9/23/20 9:43 PM, Arnd Bergmann wrote:
>>> On Wed, Sep 23, 2020 at 5:43 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
>>>>
>>>> On 9/23/20 5:30 PM, Arnd Bergmann wrote:
>>>>> On Wed, Sep 23, 2020 at 5:15 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
>>>>>>
>>>>>> Hello,
>>>>>>
>>>>>> The Surface System Aggregator Module (we'll refer to it as Surface
>>>>>> Aggregator or SAM below) is an embedded controller (EC) found on various
>>>>>> Microsoft Surface devices. Specifically, all 4th and later generation
>>>>>> Surface devices, i.e. Surface Pro 4, Surface Book 1 and later, with the
>>>>>> exception of the Surface Go series and the Surface Duo. Notably, it
>>>>>> seems like this EC can also be found on the ARM-based Surface Pro X [1].
>>>>>
>>>>> I think this should go to drivers/platform/x86 or drivers/platform/surface/
>>>>> along with other laptop vendor specific code rather than drivers/misc/.
>>>>
>>>> I initially had this under drivers/platform/x86. There are two main
>>>> reasons I changed that: First, I think it's a bit too big for
>>>> platform/x86 given that it basically introduces a new subsystem. At this
>>>> point it's really less of "a couple of odd devices here and there" and
>>>> more of a bus-type thing. Second, with the possibility of future support
>>>> for ARM devices (Pro X, Pro X 2 which is rumored to come out soon), I
>>>> thought that platform/x86 would not be a good fit.
>>>
>>> I don't see that as a strong reason against it. As you write yourself, the
>>> driver won't work on the arm machines without major changes anyway,
>>> and even if it does, it fits much better with the rest of it.
>>
>> Sorry, I should have written that a bit more clearly. I don't see any
>> reason why these drivers would not work on an ARM device such as the Pro
>> X right now, assuming that it boots via ACPI and the serial device it
>> loads against is fully functional.
> 
> As I understand, the dialect of ACPI used on the snapdragon laptops
> is not really compatible with the subset expected by the kernel, so
> you'd be more likely to run those laptops with a device tree description
> of the hardware instead (if at all).
> 
> Making the driver talk to the hardware directly instead of going through
> AML likely requires more refactoring.

Oh, I did not know that! Thanks!

>>>> I'd be happy to move this to platform/surface though, if that's
>>>> considered a better fit and you're okay with me adding that. Would make
>>>> sense given that there's already a platform/chrome, which, as far as I
>>>> can tell, also seems to be mainly focused on EC support.
>>>
>>> Yes, I think the main question is how much overlap you see functionally
>>> between this driver and the others in drivers/platform/x86.
>>
>> I think that the Pro X likely won't be the last ARM Surface device with
>> a SAM EC. Further, the subsystem is going to grow, and platform/x86
>> seems more like a collection of, if at all, loosely connected drivers,
>> which might give off the wrong impression. In my mind, this is just a
>> bit more comparable to platform/chrome than the rest of platform/x86. I
>> don't think I'm really qualified to make the decision on that though,
>> that's just my opinion.
> 
> I would ask the drivers/platform/x86 maintainers for an opinion here,
> they are probably best qualified to make that decision.
> 
> I don't really mind either way, for me this is more about who is
> responsible as a subsystem maintainer than whether these are
> technically x86 or not.

I see, okay. I'll ask them and CC them on the next submission.

>> Here's an overview of other drivers that I hopefully at some point get
>> in good enough shape, which are part of this subsystem/dependent on the
>> EC API introduced here:
>>
>> - A device registry / device hub for devices that are connected to the
>>     EC but can't be detected via ACPI.
>>
>> - A dedicated battery driver for 7th generation devices (where the
>>     battery isn't hanled via the ACPI shim).
>>
>> - A driver properly handling clipboard detachment on the Surface Books.
>>
>> - A driver for HID input/transport on the Surface Laptops and Surface
>>     Book 3.
>>
>> - A driver for allowing users to set the performance/cooling mode via
>>     sysfs.
>>
>> - Possibly a driver improving hot-plug handling of the discrete GPU in
>>     the Surface Book base.
> 
> Note that drivers that connect to the bus typically don't live in the
> same subdirectory as the driver that operates the bus. E.g. the
> battery driver would go into drivers/power/supply and the input
> would go into drivers/input/ or drivers/hid.

Right. I wonder if this also holds for devices that are directly
dependent on a special platform though? It could make sense to have them
under plaform/surface rather than in the individual subsystems as they
are only ever going to be used on this platform. On the other hand, one
could argue that having them in the subsystem directories is better for
maintainability.

Thanks,
Max

Re: [RFC PATCH 0/9] Add support for Microsoft Surface System Aggregator Module

[Maximilian Luz]

On 9/24/20 10:30 AM, Andy Shevchenko wrote:
> On Wed, Sep 23, 2020 at 6:32 PM Arnd Bergmann <arnd@arndb.de> wrote:
>>
>> On Wed, Sep 23, 2020 at 5:15 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
>>>
>>> Hello,
>>>
>>> The Surface System Aggregator Module (we'll refer to it as Surface
>>> Aggregator or SAM below) is an embedded controller (EC) found on various
>>> Microsoft Surface devices. Specifically, all 4th and later generation
>>> Surface devices, i.e. Surface Pro 4, Surface Book 1 and later, with the
>>> exception of the Surface Go series and the Surface Duo. Notably, it
>>> seems like this EC can also be found on the ARM-based Surface Pro X [1].
>>
>> I think this should go to drivers/platform/x86 or drivers/platform/surface/
>> along with other laptop vendor specific code rather than drivers/misc/.
> 
> +1 here. drivers/platform/surface is a good place to start.
> And you may begin with moving a few Surface drivers out of PDx86 to
> the new folder.

Perfect, thanks! I'll draft up a patch series over the weekend.

A couple questions regarding structure and maintenance:

  - Should I CC the platform-driver-x86 list on future submissions to
    drivers/platform/surface? I.e. is this something you would want to
    review if it doesn't touch the drivers/platform/x86 directory?

  - How would you want the layout to be, specifically regarding to the
    surface-aggregator stuff? My suggestion would be simply:

    drivers/platform/surface/
        surface_aggregator/
            Kconfig
            Makefile
            core.c
            controller.c
            ... (all core stuff built into the surface_aggregator module)
        Kconfig
        Makefile
        surface_aggregator_debugfs.c
        surface_acpi_notify.c
        surface_*.c        (any other surface platform driver as well
                            as drivers dependent on surface_aggregator)

  - Regarding future things like HID transport driver, battery/AC driver:
    Submit them to drivers/platform/surface or to their respective
    subsystem directories?

Thanks,
Max

Re: [RFC PATCH 0/9] Add support for Microsoft Surface System Aggregator Module

[Arnd Bergmann]

On Thu, Sep 24, 2020 at 8:59 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
> On 9/24/20 10:26 AM, Arnd Bergmann wrote:
> > On Thu, Sep 24, 2020 at 1:28 AM Maximilian Luz <luzmaximilian@gmail.com> wrote:

> > Note that drivers that connect to the bus typically don't live in the
> > same subdirectory as the driver that operates the bus. E.g. the
> > battery driver would go into drivers/power/supply and the input
> > would go into drivers/input/ or drivers/hid.
>
> Right. I wonder if this also holds for devices that are directly
> dependent on a special platform though? It could make sense to have them
> under plaform/surface rather than in the individual subsystems as they
> are only ever going to be used on this platform. On the other hand, one
> could argue that having them in the subsystem directories is better for
> maintainability.

Yes, absolutely. The subsystem maintainers are the ones that are
most qualified of reviewing code that uses their subsystem, regardless
of which bus is used underneath the device, and having all drivers
for a subsystem in one place makes it much easier to refactor them
all at once in case the internal interfaces are changed or common bugs
are found in multiple drivers.

       Arnd

Re: [RFC PATCH 0/9] Add support for Microsoft Surface System Aggregator Module

[Maximilian Luz]

On 9/24/20 9:38 PM, Arnd Bergmann wrote:
> On Thu, Sep 24, 2020 at 8:59 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
>> On 9/24/20 10:26 AM, Arnd Bergmann wrote:
>>> On Thu, Sep 24, 2020 at 1:28 AM Maximilian Luz <luzmaximilian@gmail.com> wrote:
> 
>>> Note that drivers that connect to the bus typically don't live in the
>>> same subdirectory as the driver that operates the bus. E.g. the
>>> battery driver would go into drivers/power/supply and the input
>>> would go into drivers/input/ or drivers/hid.
>>
>> Right. I wonder if this also holds for devices that are directly
>> dependent on a special platform though? It could make sense to have them
>> under plaform/surface rather than in the individual subsystems as they
>> are only ever going to be used on this platform. On the other hand, one
>> could argue that having them in the subsystem directories is better for
>> maintainability.
> 
> Yes, absolutely. The subsystem maintainers are the ones that are
> most qualified of reviewing code that uses their subsystem, regardless
> of which bus is used underneath the device, and having all drivers
> for a subsystem in one place makes it much easier to refactor them
> all at once in case the internal interfaces are changed or common bugs
> are found in multiple drivers.

Got it.

Thank you for bearing with me and answering all my (probably a bit
silly) questions! I really appreciate it!

Regards,
Max

Re: [RFC PATCH 0/9] Add support for Microsoft Surface System Aggregator Module

[Andy Shevchenko]

On Thu, Sep 24, 2020 at 10:38 PM Arnd Bergmann <arnd@arndb.de> wrote:
> On Thu, Sep 24, 2020 at 8:59 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
> > On 9/24/20 10:26 AM, Arnd Bergmann wrote:
> > > On Thu, Sep 24, 2020 at 1:28 AM Maximilian Luz <luzmaximilian@gmail.com> wrote:
>
> > > Note that drivers that connect to the bus typically don't live in the
> > > same subdirectory as the driver that operates the bus. E.g. the
> > > battery driver would go into drivers/power/supply and the input
> > > would go into drivers/input/ or drivers/hid.
> >
> > Right. I wonder if this also holds for devices that are directly
> > dependent on a special platform though? It could make sense to have them
> > under plaform/surface rather than in the individual subsystems as they
> > are only ever going to be used on this platform. On the other hand, one
> > could argue that having them in the subsystem directories is better for
> > maintainability.
>
> Yes, absolutely. The subsystem maintainers are the ones that are
> most qualified of reviewing code that uses their subsystem, regardless
> of which bus is used underneath the device, and having all drivers
> for a subsystem in one place makes it much easier to refactor them
> all at once in case the internal interfaces are changed or common bugs
> are found in multiple drivers.

The problem is that some of the drivers are mostly reincarnation of
board files due to the platform being Windows-oriented with badly
written ACPI tables / firmware as a whole (which means a lot of quirks
are required).

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 0/9] Add support for Microsoft Surface System Aggregator Module

[Andy Shevchenko]

On Thu, Sep 24, 2020 at 10:17 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
> On 9/24/20 10:30 AM, Andy Shevchenko wrote:
> > On Wed, Sep 23, 2020 at 6:32 PM Arnd Bergmann <arnd@arndb.de> wrote:
> >> On Wed, Sep 23, 2020 at 5:15 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:

...

> >> I think this should go to drivers/platform/x86 or drivers/platform/surface/
> >> along with other laptop vendor specific code rather than drivers/misc/.
> >
> > +1 here. drivers/platform/surface is a good place to start.
> > And you may begin with moving a few Surface drivers out of PDx86 to
> > the new folder.
>
> Perfect, thanks! I'll draft up a patch series over the weekend.
>
> A couple questions regarding structure and maintenance:
>
>   - Should I CC the platform-driver-x86 list on future submissions to
>     drivers/platform/surface? I.e. is this something you would want to
>     review if it doesn't touch the drivers/platform/x86 directory?

Include PDx86 mailing list to the list of that. Current SURFACE*
drivers have per driver record in MAINTAINERS IIRC. So, update them as
well if needed.

>   - How would you want the layout to be, specifically regarding to the
>     surface-aggregator stuff? My suggestion would be simply:
>
>     drivers/platform/surface/

>         surface_aggregator/

Don't repeat parts of the path, the aggregator is enough as a folder
name, but the driver of course should be in its own namespace
('surface').

>             Kconfig
>             Makefile
>             core.c
>             controller.c
>             ... (all core stuff built into the surface_aggregator module)
>         Kconfig
>         Makefile

>         surface_aggregator_debugfs.c

(Not sure why it's not a part of aggregator folder)

>         surface_acpi_notify.c
>         surface_*.c        (any other surface platform driver as well
>                             as drivers dependent on surface_aggregator)
>
>   - Regarding future things like HID transport driver, battery/AC driver:
>     Submit them to drivers/platform/surface or to their respective
>     subsystem directories?

Respective subsystem _if_ it is a subsystem related driver and not
kinda board file. Use common sense and existing examples.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 0/9] Add support for Microsoft Surface System Aggregator Module

[Maximilian Luz]

On 9/25/20 4:58 PM, Andy Shevchenko wrote:
> On Thu, Sep 24, 2020 at 10:17 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
>> On 9/24/20 10:30 AM, Andy Shevchenko wrote:
>>> On Wed, Sep 23, 2020 at 6:32 PM Arnd Bergmann <arnd@arndb.de> wrote:
>>>> On Wed, Sep 23, 2020 at 5:15 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
> 
> ...
> 
>>>> I think this should go to drivers/platform/x86 or drivers/platform/surface/
>>>> along with other laptop vendor specific code rather than drivers/misc/.
>>>
>>> +1 here. drivers/platform/surface is a good place to start.
>>> And you may begin with moving a few Surface drivers out of PDx86 to
>>> the new folder.
>>
>> Perfect, thanks! I'll draft up a patch series over the weekend.
>>
>> A couple questions regarding structure and maintenance:
>>
>>    - Should I CC the platform-driver-x86 list on future submissions to
>>      drivers/platform/surface? I.e. is this something you would want to
>>      review if it doesn't touch the drivers/platform/x86 directory?
> 
> Include PDx86 mailing list to the list of that. Current SURFACE*
> drivers have per driver record in MAINTAINERS IIRC. So, update them as
> well if needed.

Will do.

>>    - How would you want the layout to be, specifically regarding to the
>>      surface-aggregator stuff? My suggestion would be simply:
>>
>>      drivers/platform/surface/
> 
>>          surface_aggregator/
> 
> Don't repeat parts of the path, the aggregator is enough as a folder
> name, but the driver of course should be in its own namespace
> ('surface').

Okay.

>>              Kconfig
>>              Makefile
>>              core.c
>>              controller.c
>>              ... (all core stuff built into the surface_aggregator module)
>>          Kconfig
>>          Makefile
> 
>>          surface_aggregator_debugfs.c
> 
> (Not sure why it's not a part of aggregator folder)

I kind of thought of the aggregator folder to contain only files that
build the core module. surface_aggregator_debugfs is intended as
separate module, to be loaded when needed. So I'd consider it a client
driver to the aggregator in the same way that surface_acpi_notify is.

Let me know if you still want me to move this into the aggregator folder
though. Personally, I just feel that that might lead to a bit of
confusion, specifically the idea that it's built into the core when it's
not.

>>          surface_acpi_notify.c
>>          surface_*.c        (any other surface platform driver as well
>>                              as drivers dependent on surface_aggregator)
>>
>>    - Regarding future things like HID transport driver, battery/AC driver:
>>      Submit them to drivers/platform/surface or to their respective
>>      subsystem directories?
> 
> Respective subsystem _if_ it is a subsystem related driver and not
> kinda board file. Use common sense and existing examples.

Right, thank you!

Regards,
Max