[PATCH 0/3] OF support for Surface System Aggregator Module

[PATCH 0/3] OF support for Surface System Aggregator Module

[Konrad Dybcio]

Wire up OF support for SSAM drivers, to use with Surface Laptop 7 and
other Qualcomm-based devices.

Patch 3 references compatible strings introduced in [1]

[1] https://lore.kernel.org/linux-arm-msm/20240809-topic-sl7-v1-1-2090433d8dfc@quicinc.com/T/#u

Signed-off-by: Konrad Dybcio <quic_kdybcio@quicinc.com>
---
Konrad Dybcio (3):
      dt-bindings: serial: Allow embedded-controller as child node
      dt-bindings: platform: Add Surface System Aggregator Module
      platform/surface: Add OF support

 .../bindings/platform/microsoft,surface-sam.yaml   | 50 ++++++++++++++
 .../devicetree/bindings/serial/serial.yaml         |  2 +-
 drivers/acpi/scan.c                                |  3 +-
 drivers/platform/surface/aggregator/bus.c          |  2 +
 drivers/platform/surface/aggregator/controller.c   | 72 +++++++++++++++----
 drivers/platform/surface/aggregator/core.c         | 80 ++++++++++++++++++----
 .../platform/surface/surface_aggregator_registry.c | 44 ++++++++++--
 7 files changed, 218 insertions(+), 35 deletions(-)
---
base-commit: 1e391b34f6aa043c7afa40a2103163a0ef06d179
change-id: 20240809-topic-sam-5de2f0ec9370

Best regards,
-- 
Konrad Dybcio <quic_kdybcio@quicinc.com>

[PATCH 1/3] dt-bindings: serial: Allow embedded-controller as child node

[Konrad Dybcio]

From: Konrad Dybcio <quic_kdybcio@quicinc.com>

There exist some embedded controllers (like Microsoft SAM found on
Surface devices or Apple Oscar found on old iPhones) that connect to
the host device via serial.

Allow that class of devices to exist under serial interface controller
nodes.

Signed-off-by: Konrad Dybcio <quic_kdybcio@quicinc.com>
---
 Documentation/devicetree/bindings/serial/serial.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/serial/serial.yaml b/Documentation/devicetree/bindings/serial/serial.yaml
index ffc9198ae214..9b2c94796371 100644
--- a/Documentation/devicetree/bindings/serial/serial.yaml
+++ b/Documentation/devicetree/bindings/serial/serial.yaml
@@ -88,7 +88,7 @@ properties:
       TX FIFO threshold configuration (in bytes).
 
 patternProperties:
-  "^(bluetooth|bluetooth-gnss|gnss|gps|mcu|onewire)$":
+  "^(bluetooth|bluetooth-gnss|embedded-controller|gnss|gps|mcu|onewire)$":
     if:
       type: object
     then:

-- 
2.46.0

[PATCH 2/3] dt-bindings: platform: Add Surface System Aggregator Module

[Konrad Dybcio]

From: Konrad Dybcio <quic_kdybcio@quicinc.com>

Add bindings for the Surface System Aggregator Module (SAM/SSAM), the
Microsoft Surface-standard Embedded Controller, used on both x86- and
Qualcomm-based devices.

It provides a plethora of functions, depending on what's wired up to
it. That includes but is not limited to: fan control, keyboard/touchpad
support, thermal sensors, power control, special buttons, tablet mode.

Signed-off-by: Konrad Dybcio <quic_kdybcio@quicinc.com>
---
 .../bindings/platform/microsoft,surface-sam.yaml   | 50 ++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/Documentation/devicetree/bindings/platform/microsoft,surface-sam.yaml b/Documentation/devicetree/bindings/platform/microsoft,surface-sam.yaml
new file mode 100644
index 000000000000..f613738aa31d
--- /dev/null
+++ b/Documentation/devicetree/bindings/platform/microsoft,surface-sam.yaml
@@ -0,0 +1,50 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/platform/microsoft,surface-sam.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Surface System Aggregator Module (SAM, SSAM)
+
+maintainers:
+  - Konrad Dybcio <konradybcio@kernel.org>
+
+description: |
+  Surface devices use a standardized embedded controller to let the
+  operating system interface with various hardware functions. The
+  specific functionalities are modeled as subdevices and matched on
+  five levels: domain, category, target, instance and function.
+
+properties:
+  compatible:
+    const: microsoft,surface-sam
+
+  interrupts:
+    maxItems: 1
+
+  current-speed:
+    description: The baudrate in bits per second of the device as it comes
+      online, current active speed.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+required:
+  - compatible
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    uart {
+        embedded-controller {
+            compatible = "microsoft,surface-sam";
+
+            interrupts-extended = <&tlmm 91 IRQ_TYPE_EDGE_RISING>;
+
+            pinctrl-0 = <&ssam_state>;
+            pinctrl-names = "default";
+
+            current-speed = <4000000>;
+        };
+    };

-- 
2.46.0

[PATCH 3/3] platform/surface: Add OF support

[Konrad Dybcio]

From: Konrad Dybcio <quic_kdybcio@quicinc.com>

Add basic support for registering the aggregator module on Device Tree-
based platforms. These include at least three generations of Qualcomm
Snapdragon-based Surface devices:

- SC8180X / SQ1 / SQ2: Pro X,
- SC8280XP / SQ3: Devkit 2023, Pro 9
- X Elite: Laptop 7 / Pro11

Thankfully, the aggregators on these seem to be configured in an
identical way, which allows for using these settings as defaults and
no DT properties need to be introduced (until that changes, anyway).

Based on the work done by Maximilian Luz, largely rewritten.

Signed-off-by: Konrad Dybcio <quic_kdybcio@quicinc.com>
---
 drivers/acpi/scan.c                                |  3 +-
 drivers/platform/surface/aggregator/bus.c          |  2 +
 drivers/platform/surface/aggregator/controller.c   | 72 +++++++++++++++----
 drivers/platform/surface/aggregator/core.c         | 80 ++++++++++++++++++----
 .../platform/surface/surface_aggregator_registry.c | 44 ++++++++++--
 5 files changed, 167 insertions(+), 34 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 59771412686b..6c3cad894648 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -2444,7 +2444,8 @@ static int acpi_walk_dep_device_list(acpi_handle handle,
  */
 void acpi_dev_clear_dependencies(struct acpi_device *supplier)
 {
-	acpi_walk_dep_device_list(supplier->handle, acpi_scan_clear_dep, NULL);
+	if (supplier)
+		acpi_walk_dep_device_list(supplier->handle, acpi_scan_clear_dep, NULL);
 }
 EXPORT_SYMBOL_GPL(acpi_dev_clear_dependencies);
 
diff --git a/drivers/platform/surface/aggregator/bus.c b/drivers/platform/surface/aggregator/bus.c
index af8d573aae93..d68d231e716e 100644
--- a/drivers/platform/surface/aggregator/bus.c
+++ b/drivers/platform/surface/aggregator/bus.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/device.h>
+#include <linux/of.h>
 #include <linux/property.h>
 #include <linux/slab.h>
 
@@ -441,6 +442,7 @@ static int ssam_add_client_device(struct device *parent, struct ssam_controller
 
 	sdev->dev.parent = parent;
 	sdev->dev.fwnode = fwnode_handle_get(node);
+	sdev->dev.of_node = to_of_node(node);
 
 	status = ssam_device_add(sdev);
 	if (status)
diff --git a/drivers/platform/surface/aggregator/controller.c b/drivers/platform/surface/aggregator/controller.c
index 7fc602e01487..aea10e192140 100644
--- a/drivers/platform/surface/aggregator/controller.c
+++ b/drivers/platform/surface/aggregator/controller.c
@@ -1104,13 +1104,6 @@ int ssam_controller_caps_load_from_acpi(acpi_handle handle,
 	u64 funcs;
 	int status;
 
-	/* Set defaults. */
-	caps->ssh_power_profile = U32_MAX;
-	caps->screen_on_sleep_idle_timeout = U32_MAX;
-	caps->screen_off_sleep_idle_timeout = U32_MAX;
-	caps->d3_closes_handle = false;
-	caps->ssh_buffer_size = U32_MAX;
-
 	/* Pre-load supported DSM functions. */
 	status = ssam_dsm_get_functions(handle, &funcs);
 	if (status)
@@ -1149,6 +1142,57 @@ int ssam_controller_caps_load_from_acpi(acpi_handle handle,
 	return 0;
 }
 
+/**
+ * ssam_controller_caps_load_from_of() - Load controller capabilities from OF/DT.
+ * @device: The device from which to load the capabilities from.
+ * @caps:   Where to store the capabilities in.
+ *
+ * Return: Returns zero on success, a negative error code on failure.
+ */
+static int ssam_controller_caps_load_from_of(struct device *dev, struct ssam_controller_caps *caps)
+{
+	/*
+	 * Every device starting with Surface Pro X through Laptop 7 uses these
+	 * identical values, which makes them good defaults.
+	 */
+	caps->d3_closes_handle = true;
+	caps->screen_on_sleep_idle_timeout = 5000;
+	caps->screen_off_sleep_idle_timeout = 30;
+	caps->ssh_buffer_size = 48;
+	/* TODO: figure out power profile */
+
+	return 0;
+}
+
+/**
+ * ssam_controller_caps_load_from_acpi() - Load controller capabilities from
+ * ACPI _DSM.
+ * @handle: The handle of the ACPI controller/SSH device.
+ * @caps:   Where to store the capabilities in.
+ *
+ * Initializes the given controller capabilities with default values, then
+ * checks and, if the respective _DSM functions are available, loads the
+ * actual capabilities from the _DSM.
+ *
+ * Return: Returns zero on success, a negative error code on failure.
+ */
+static int ssam_controller_caps_load(struct device *dev, struct ssam_controller_caps *caps)
+{
+	acpi_handle handle = ACPI_HANDLE(dev);
+
+	/* Set defaults. */
+	caps->ssh_power_profile = U32_MAX;
+	caps->screen_on_sleep_idle_timeout = U32_MAX;
+	caps->screen_off_sleep_idle_timeout = U32_MAX;
+	caps->d3_closes_handle = false;
+	caps->ssh_buffer_size = U32_MAX;
+
+	if (handle)
+		return ssam_controller_caps_load_from_acpi(handle, caps);
+	else
+		return ssam_controller_caps_load_from_of(dev, caps);
+}
+
 /**
  * ssam_controller_init() - Initialize SSAM controller.
  * @ctrl:   The controller to initialize.
@@ -1165,13 +1209,12 @@ int ssam_controller_caps_load_from_acpi(acpi_handle handle,
 int ssam_controller_init(struct ssam_controller *ctrl,
 			 struct serdev_device *serdev)
 {
-	acpi_handle handle = ACPI_HANDLE(&serdev->dev);
 	int status;
 
 	init_rwsem(&ctrl->lock);
 	kref_init(&ctrl->kref);
 
-	status = ssam_controller_caps_load_from_acpi(handle, &ctrl->caps);
+	status = ssam_controller_caps_load(&serdev->dev, &ctrl->caps);
 	if (status)
 		return status;
 
@@ -2715,11 +2758,12 @@ int ssam_irq_setup(struct ssam_controller *ctrl)
 	const int irqf = IRQF_ONESHOT | IRQF_TRIGGER_RISING | IRQF_NO_AUTOEN;
 
 	gpiod = gpiod_get(dev, "ssam_wakeup-int", GPIOD_ASIS);
-	if (IS_ERR(gpiod))
-		return PTR_ERR(gpiod);
-
-	irq = gpiod_to_irq(gpiod);
-	gpiod_put(gpiod);
+	if (IS_ERR(gpiod)) {
+		irq = fwnode_irq_get(dev_fwnode(dev), 0);
+	} else {
+		irq = gpiod_to_irq(gpiod);
+		gpiod_put(gpiod);
+	}
 
 	if (irq < 0)
 		return irq;
diff --git a/drivers/platform/surface/aggregator/core.c b/drivers/platform/surface/aggregator/core.c
index 797d0645bd77..d39e0d7ce92b 100644
--- a/drivers/platform/surface/aggregator/core.c
+++ b/drivers/platform/surface/aggregator/core.c
@@ -17,9 +17,12 @@
 #include <linux/kernel.h>
 #include <linux/kref.h>
 #include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
 #include <linux/pm.h>
 #include <linux/serdev.h>
 #include <linux/sysfs.h>
+#include <linux/units.h>
 
 #include <linux/surface_aggregator/controller.h>
 #include <linux/surface_aggregator/device.h>
@@ -299,7 +302,7 @@ static const struct attribute_group ssam_sam_group = {
 };
 
 
-/* -- ACPI based device setup. ---------------------------------------------- */
+/* -- Serial device setup. ---------------------------------------------- */
 
 static acpi_status ssam_serdev_setup_via_acpi_crs(struct acpi_resource *rsc,
 						  void *ctx)
@@ -352,13 +355,28 @@ static acpi_status ssam_serdev_setup_via_acpi_crs(struct acpi_resource *rsc,
 	return AE_CTRL_TERMINATE;
 }
 
-static acpi_status ssam_serdev_setup_via_acpi(acpi_handle handle,
-					      struct serdev_device *serdev)
+static int ssam_serdev_setup_via_acpi(struct serdev_device *serdev, acpi_handle handle)
 {
-	return acpi_walk_resources(handle, METHOD_NAME__CRS,
-				   ssam_serdev_setup_via_acpi_crs, serdev);
+	acpi_status status;
+
+	status = acpi_walk_resources(handle, METHOD_NAME__CRS,
+				     ssam_serdev_setup_via_acpi_crs, serdev);
+
+	return status ? -ENXIO : 0;
 }
 
+static int ssam_serdev_setup(struct acpi_device *ssh, struct serdev_device *serdev)
+{
+	if (ssh)
+		return ssam_serdev_setup_via_acpi(serdev, ssh->handle);
+
+	/* TODO: these values may differ per board/implementation */
+	serdev_device_set_baudrate(serdev, 4 * HZ_PER_MHZ);
+	serdev_device_set_flow_control(serdev, true);
+	serdev_device_set_parity(serdev, SERDEV_PARITY_NONE);
+
+	return 0;
+}
 
 /* -- Power management. ----------------------------------------------------- */
 
@@ -624,13 +642,15 @@ static int ssam_serial_hub_probe(struct serdev_device *serdev)
 	acpi_status astatus;
 	int status;
 
-	status = gpiod_count(dev, NULL);
-	if (status < 0)
-		return dev_err_probe(dev, status, "no GPIO found\n");
+	if (ssh) {
+		status = gpiod_count(dev, NULL);
+		if (status < 0)
+			return dev_err_probe(dev, status, "no GPIO found\n");
 
-	status = devm_acpi_dev_add_driver_gpios(dev, ssam_acpi_gpios);
-	if (status)
-		return status;
+		status = devm_acpi_dev_add_driver_gpios(dev, ssam_acpi_gpios);
+		if (status)
+			return status;
+	}
 
 	/* Allocate controller. */
 	ctrl = kzalloc(sizeof(*ctrl), GFP_KERNEL);
@@ -655,7 +675,7 @@ static int ssam_serial_hub_probe(struct serdev_device *serdev)
 		goto err_devopen;
 	}
 
-	astatus = ssam_serdev_setup_via_acpi(ssh->handle, serdev);
+	astatus = ssam_serdev_setup(ssh, serdev);
 	if (ACPI_FAILURE(astatus)) {
 		status = dev_err_probe(dev, -ENXIO, "failed to setup serdev\n");
 		goto err_devinit;
@@ -717,10 +737,31 @@ static int ssam_serial_hub_probe(struct serdev_device *serdev)
 	 *       For now let's thus default power/wakeup to false.
 	 */
 	device_set_wakeup_capable(dev, true);
+
+	/*
+	 * When using DT, we have to register the platform hub driver manually,
+	 * as it can't be matched based on top-level board compatible (like it
+	 * does the ACPI case).
+	 */
+	if (!ssh) {
+		struct platform_device *ph_pdev =
+			platform_device_register_simple("surface_aggregator_platform_hub",
+							0, NULL, 0);
+		if (IS_ERR(ph_pdev))
+			return dev_err_probe(dev, PTR_ERR(ph_pdev),
+					     "Failed to register the platform hub driver\n");
+	}
+
+	status = ssam_register_clients(&serdev->dev, ctrl);
+	if (status)
+		goto err_clients;
+
 	acpi_dev_clear_dependencies(ssh);
 
 	return 0;
 
+err_clients:
+	ssam_clear_controller();
 err_mainref:
 	ssam_irq_free(ctrl);
 err_irq:
@@ -782,18 +823,27 @@ static void ssam_serial_hub_remove(struct serdev_device *serdev)
 	device_set_wakeup_capable(&serdev->dev, false);
 }
 
-static const struct acpi_device_id ssam_serial_hub_match[] = {
+static const struct acpi_device_id ssam_serial_hub_acpi_match[] = {
 	{ "MSHW0084", 0 },
 	{ },
 };
-MODULE_DEVICE_TABLE(acpi, ssam_serial_hub_match);
+MODULE_DEVICE_TABLE(acpi, ssam_serial_hub_acpi_match);
+
+#ifdef CONFIG_OF
+static const struct of_device_id ssam_serial_hub_of_match[] = {
+	{ .compatible = "microsoft,surface-sam", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, ssam_serial_hub_of_match);
+#endif
 
 static struct serdev_device_driver ssam_serial_hub = {
 	.probe = ssam_serial_hub_probe,
 	.remove = ssam_serial_hub_remove,
 	.driver = {
 		.name = "surface_serial_hub",
-		.acpi_match_table = ssam_serial_hub_match,
+		.acpi_match_table = ACPI_PTR(ssam_serial_hub_acpi_match),
+		.of_match_table = of_match_ptr(ssam_serial_hub_of_match),
 		.pm = &ssam_serial_hub_pm_ops,
 		.shutdown = ssam_serial_hub_shutdown,
 		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/drivers/platform/surface/surface_aggregator_registry.c
index 1c4d74db08c9..57787f2ff38b 100644
--- a/drivers/platform/surface/surface_aggregator_registry.c
+++ b/drivers/platform/surface/surface_aggregator_registry.c
@@ -12,6 +12,7 @@
 #include <linux/acpi.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/property.h>
 #include <linux/types.h>
@@ -273,6 +274,18 @@ static const struct software_node *ssam_node_group_sl5[] = {
 	NULL,
 };
 
+/* Devices for Surface Laptop 7. */
+static const struct software_node *ssam_node_group_sl7[] = {
+	&ssam_node_root,
+	&ssam_node_bat_ac,
+	&ssam_node_bat_main,
+	&ssam_node_tmp_perf_profile_with_fan,
+	&ssam_node_fan_speed,
+	&ssam_node_hid_sam_keyboard,
+	/* TODO: evaluate thermal sensors devices when we get a driver for that */
+	NULL,
+};
+
 /* Devices for Surface Laptop Studio. */
 static const struct software_node *ssam_node_group_sls[] = {
 	&ssam_node_root,
@@ -346,7 +359,7 @@ static const struct software_node *ssam_node_group_sp9[] = {
 
 /* -- SSAM platform/meta-hub driver. ---------------------------------------- */
 
-static const struct acpi_device_id ssam_platform_hub_match[] = {
+static const struct acpi_device_id ssam_platform_hub_acpi_match[] = {
 	/* Surface Pro 4, 5, and 6 (OMBR < 0x10) */
 	{ "MSHW0081", (unsigned long)ssam_node_group_gen5 },
 
@@ -402,16 +415,39 @@ static const struct acpi_device_id ssam_platform_hub_match[] = {
 };
 MODULE_DEVICE_TABLE(acpi, ssam_platform_hub_match);
 
+#ifdef CONFIG_OF
+static const struct of_device_id ssam_platform_hub_of_match[] = {
+	/* Surface Laptop 7 */
+	{ .compatible = "microsoft,romulus13", (void *)ssam_node_group_sl7 },
+	{ .compatible = "microsoft,romulus15", (void *)ssam_node_group_sl7 },
+	{ },
+};
+#endif
+
 static int ssam_platform_hub_probe(struct platform_device *pdev)
 {
 	const struct software_node **nodes;
+	const struct of_device_id *match;
+	struct device_node *fdt_root;
 	struct ssam_controller *ctrl;
 	struct fwnode_handle *root;
 	int status;
 
 	nodes = (const struct software_node **)acpi_device_get_match_data(&pdev->dev);
-	if (!nodes)
-		return -ENODEV;
+	if (!nodes) {
+		fdt_root = of_find_node_by_path("/");
+		if (!fdt_root)
+			return -ENODEV;
+
+		match = of_match_node(ssam_platform_hub_of_match, fdt_root);
+		of_node_put(fdt_root);
+		if (!match)
+			return -ENODEV;
+
+		nodes = (const struct software_node **)match->data;
+		if (!nodes)
+			return -ENODEV;
+	}
 
 	/*
 	 * As we're adding the SSAM client devices as children under this device
@@ -460,7 +496,7 @@ static struct platform_driver ssam_platform_hub_driver = {
 	.remove_new = ssam_platform_hub_remove,
 	.driver = {
 		.name = "surface_aggregator_platform_hub",
-		.acpi_match_table = ssam_platform_hub_match,
+		.acpi_match_table = ssam_platform_hub_acpi_match,
 		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
 	},
 };

-- 
2.46.0

Re: [PATCH 3/3] platform/surface: Add OF support

[Maximilian Luz]

Hi,

Thanks for taking this up! A couple of comments below:

On 8/9/24 3:48 AM, Konrad Dybcio wrote:
> From: Konrad Dybcio <quic_kdybcio@quicinc.com>
> 
> Add basic support for registering the aggregator module on Device Tree-
> based platforms. These include at least three generations of Qualcomm
> Snapdragon-based Surface devices:
> 
> - SC8180X / SQ1 / SQ2: Pro X,
> - SC8280XP / SQ3: Devkit 2023, Pro 9
> - X Elite: Laptop 7 / Pro11
> 
> Thankfully, the aggregators on these seem to be configured in an
> identical way, which allows for using these settings as defaults and
> no DT properties need to be introduced (until that changes, anyway).
> 
> Based on the work done by Maximilian Luz, largely rewritten.
> 
> Signed-off-by: Konrad Dybcio <quic_kdybcio@quicinc.com>
> ---
>   drivers/acpi/scan.c                                |  3 +-
>   drivers/platform/surface/aggregator/bus.c          |  2 +
>   drivers/platform/surface/aggregator/controller.c   | 72 +++++++++++++++----
>   drivers/platform/surface/aggregator/core.c         | 80 ++++++++++++++++++----
>   .../platform/surface/surface_aggregator_registry.c | 44 ++++++++++--
>   5 files changed, 167 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 59771412686b..6c3cad894648 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -2444,7 +2444,8 @@ static int acpi_walk_dep_device_list(acpi_handle handle,
>    */
>   void acpi_dev_clear_dependencies(struct acpi_device *supplier)
>   {
> -	acpi_walk_dep_device_list(supplier->handle, acpi_scan_clear_dep, NULL);
> +	if (supplier)
> +		acpi_walk_dep_device_list(supplier->handle, acpi_scan_clear_dep, NULL);
>   }
>   EXPORT_SYMBOL_GPL(acpi_dev_clear_dependencies);
>   
> diff --git a/drivers/platform/surface/aggregator/bus.c b/drivers/platform/surface/aggregator/bus.c
> index af8d573aae93..d68d231e716e 100644
> --- a/drivers/platform/surface/aggregator/bus.c
> +++ b/drivers/platform/surface/aggregator/bus.c
> @@ -6,6 +6,7 @@
>    */
>   
>   #include <linux/device.h>
> +#include <linux/of.h>
>   #include <linux/property.h>
>   #include <linux/slab.h>
>   
> @@ -441,6 +442,7 @@ static int ssam_add_client_device(struct device *parent, struct ssam_controller
>   
>   	sdev->dev.parent = parent;
>   	sdev->dev.fwnode = fwnode_handle_get(node);
> +	sdev->dev.of_node = to_of_node(node);
>   
>   	status = ssam_device_add(sdev);
>   	if (status)
> diff --git a/drivers/platform/surface/aggregator/controller.c b/drivers/platform/surface/aggregator/controller.c
> index 7fc602e01487..aea10e192140 100644
> --- a/drivers/platform/surface/aggregator/controller.c
> +++ b/drivers/platform/surface/aggregator/controller.c
> @@ -1104,13 +1104,6 @@ int ssam_controller_caps_load_from_acpi(acpi_handle handle,
>   	u64 funcs;
>   	int status;
>   
> -	/* Set defaults. */
> -	caps->ssh_power_profile = U32_MAX;
> -	caps->screen_on_sleep_idle_timeout = U32_MAX;
> -	caps->screen_off_sleep_idle_timeout = U32_MAX;
> -	caps->d3_closes_handle = false;
> -	caps->ssh_buffer_size = U32_MAX;
> -
>   	/* Pre-load supported DSM functions. */
>   	status = ssam_dsm_get_functions(handle, &funcs);
>   	if (status)
> @@ -1149,6 +1142,57 @@ int ssam_controller_caps_load_from_acpi(acpi_handle handle,
>   	return 0;
>   }
>   
> +/**
> + * ssam_controller_caps_load_from_of() - Load controller capabilities from OF/DT.
> + * @device: The device from which to load the capabilities from.
> + * @caps:   Where to store the capabilities in.
> + *
> + * Return: Returns zero on success, a negative error code on failure.
> + */
> +static int ssam_controller_caps_load_from_of(struct device *dev, struct ssam_controller_caps *caps)
> +{
> +	/*
> +	 * Every device starting with Surface Pro X through Laptop 7 uses these
> +	 * identical values, which makes them good defaults.
> +	 */
> +	caps->d3_closes_handle = true;
> +	caps->screen_on_sleep_idle_timeout = 5000;
> +	caps->screen_off_sleep_idle_timeout = 30;
> +	caps->ssh_buffer_size = 48;
> +	/* TODO: figure out power profile */
> +
> +	return 0;
> +}
> +
> +/**
> + * ssam_controller_caps_load_from_acpi() - Load controller capabilities from
> + * ACPI _DSM.
> + * @handle: The handle of the ACPI controller/SSH device.
> + * @caps:   Where to store the capabilities in.
> + *
> + * Initializes the given controller capabilities with default values, then
> + * checks and, if the respective _DSM functions are available, loads the
> + * actual capabilities from the _DSM.
> + *
> + * Return: Returns zero on success, a negative error code on failure.
> + */

Doc needs updating, this is just the one copied from
ssam_controller_caps_load_acpi().

> +static int ssam_controller_caps_load(struct device *dev, struct ssam_controller_caps *caps)
> +{
> +	acpi_handle handle = ACPI_HANDLE(dev);
> +
> +	/* Set defaults. */
> +	caps->ssh_power_profile = U32_MAX;
> +	caps->screen_on_sleep_idle_timeout = U32_MAX;
> +	caps->screen_off_sleep_idle_timeout = U32_MAX;
> +	caps->d3_closes_handle = false;
> +	caps->ssh_buffer_size = U32_MAX;
> +
> +	if (handle)
> +		return ssam_controller_caps_load_from_acpi(handle, caps);
> +	else
> +		return ssam_controller_caps_load_from_of(dev, caps);
> +}
> +
>   /**
>    * ssam_controller_init() - Initialize SSAM controller.
>    * @ctrl:   The controller to initialize.
> @@ -1165,13 +1209,12 @@ int ssam_controller_caps_load_from_acpi(acpi_handle handle,
>   int ssam_controller_init(struct ssam_controller *ctrl,
>   			 struct serdev_device *serdev)
>   {
> -	acpi_handle handle = ACPI_HANDLE(&serdev->dev);
>   	int status;
>   
>   	init_rwsem(&ctrl->lock);
>   	kref_init(&ctrl->kref);
>   
> -	status = ssam_controller_caps_load_from_acpi(handle, &ctrl->caps);
> +	status = ssam_controller_caps_load(&serdev->dev, &ctrl->caps);
>   	if (status)
>   		return status;
>   
> @@ -2715,11 +2758,12 @@ int ssam_irq_setup(struct ssam_controller *ctrl)
>   	const int irqf = IRQF_ONESHOT | IRQF_TRIGGER_RISING | IRQF_NO_AUTOEN;
>   
>   	gpiod = gpiod_get(dev, "ssam_wakeup-int", GPIOD_ASIS);
> -	if (IS_ERR(gpiod))
> -		return PTR_ERR(gpiod);
> -
> -	irq = gpiod_to_irq(gpiod);
> -	gpiod_put(gpiod);
> +	if (IS_ERR(gpiod)) {
> +		irq = fwnode_irq_get(dev_fwnode(dev), 0);
> +	} else {
> +		irq = gpiod_to_irq(gpiod);
> +		gpiod_put(gpiod);
> +	}
>   
>   	if (irq < 0)
>   		return irq;
> diff --git a/drivers/platform/surface/aggregator/core.c b/drivers/platform/surface/aggregator/core.c
> index 797d0645bd77..d39e0d7ce92b 100644
> --- a/drivers/platform/surface/aggregator/core.c
> +++ b/drivers/platform/surface/aggregator/core.c
> @@ -17,9 +17,12 @@
>   #include <linux/kernel.h>
>   #include <linux/kref.h>
>   #include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
>   #include <linux/pm.h>
>   #include <linux/serdev.h>
>   #include <linux/sysfs.h>
> +#include <linux/units.h>
>   
>   #include <linux/surface_aggregator/controller.h>
>   #include <linux/surface_aggregator/device.h>
> @@ -299,7 +302,7 @@ static const struct attribute_group ssam_sam_group = {
>   };
>   
>   
> -/* -- ACPI based device setup. ---------------------------------------------- */
> +/* -- Serial device setup. ---------------------------------------------- */

Nitpick, but could we maybe keep that at 80 columns please? :)

>   
>   static acpi_status ssam_serdev_setup_via_acpi_crs(struct acpi_resource *rsc,
>   						  void *ctx)
> @@ -352,13 +355,28 @@ static acpi_status ssam_serdev_setup_via_acpi_crs(struct acpi_resource *rsc,
>   	return AE_CTRL_TERMINATE;
>   }
>   
> -static acpi_status ssam_serdev_setup_via_acpi(acpi_handle handle,
> -					      struct serdev_device *serdev)
> +static int ssam_serdev_setup_via_acpi(struct serdev_device *serdev, acpi_handle handle)
>   {
> -	return acpi_walk_resources(handle, METHOD_NAME__CRS,
> -				   ssam_serdev_setup_via_acpi_crs, serdev);
> +	acpi_status status;
> +
> +	status = acpi_walk_resources(handle, METHOD_NAME__CRS,
> +				     ssam_serdev_setup_via_acpi_crs, serdev);
> +
> +	return status ? -ENXIO : 0;
>   }
>   
> +static int ssam_serdev_setup(struct acpi_device *ssh, struct serdev_device *serdev)
> +{
> +	if (ssh)
> +		return ssam_serdev_setup_via_acpi(serdev, ssh->handle);
> +
> +	/* TODO: these values may differ per board/implementation */
> +	serdev_device_set_baudrate(serdev, 4 * HZ_PER_MHZ);

Isn't this defined in the DT spec that you're adding as "current-speed"
in patch 2? Why not load it from there?

> +	serdev_device_set_flow_control(serdev, true);
> +	serdev_device_set_parity(serdev, SERDEV_PARITY_NONE);
> +
> +	return 0;
> +}
>   
>   /* -- Power management. ----------------------------------------------------- */
>   
> @@ -624,13 +642,15 @@ static int ssam_serial_hub_probe(struct serdev_device *serdev)
>   	acpi_status astatus;

This can be removed, see below.

>   	int status;
>   
> -	status = gpiod_count(dev, NULL);
> -	if (status < 0)
> -		return dev_err_probe(dev, status, "no GPIO found\n");
> +	if (ssh) {
> +		status = gpiod_count(dev, NULL);
> +		if (status < 0)
> +			return dev_err_probe(dev, status, "no GPIO found\n");
>   
> -	status = devm_acpi_dev_add_driver_gpios(dev, ssam_acpi_gpios);
> -	if (status)
> -		return status;
> +		status = devm_acpi_dev_add_driver_gpios(dev, ssam_acpi_gpios);
> +		if (status)
> +			return status;
> +	}
>   
>   	/* Allocate controller. */
>   	ctrl = kzalloc(sizeof(*ctrl), GFP_KERNEL);
> @@ -655,7 +675,7 @@ static int ssam_serial_hub_probe(struct serdev_device *serdev)
>   		goto err_devopen;
>   	}
>   
> -	astatus = ssam_serdev_setup_via_acpi(ssh->handle, serdev);
> +	astatus = ssam_serdev_setup(ssh, serdev);>   	if (ACPI_FAILURE(astatus)) {

ssam_serdev_setup() returns an int, so this should now just use
"status".

>   		status = dev_err_probe(dev, -ENXIO, "failed to setup serdev\n");
>   		goto err_devinit;
> @@ -717,10 +737,31 @@ static int ssam_serial_hub_probe(struct serdev_device *serdev)
>   	 *       For now let's thus default power/wakeup to false.
>   	 */
>   	device_set_wakeup_capable(dev, true);
> +
> +	/*
> +	 * When using DT, we have to register the platform hub driver manually,
> +	 * as it can't be matched based on top-level board compatible (like it
> +	 * does the ACPI case).
> +	 */
> +	if (!ssh) {
> +		struct platform_device *ph_pdev =
> +			platform_device_register_simple("surface_aggregator_platform_hub",
> +							0, NULL, 0);
> +		if (IS_ERR(ph_pdev))
> +			return dev_err_probe(dev, PTR_ERR(ph_pdev),
> +					     "Failed to register the platform hub driver\n");
> +	}
> +
> +	status = ssam_register_clients(&serdev->dev, ctrl);
> +	if (status)
> +		goto err_clients;

Is the ssam_register_clients() call required or is it a remnant from a
previous version? We're now not adding any children to the controller
itself but model ACPI and do all of that with the platform hub. So
unless I'm missing something, I think this should not be necessary.

> +
>   	acpi_dev_clear_dependencies(ssh);
>   
>   	return 0;
>   
> +err_clients:
> +	ssam_clear_controller();
>   err_mainref:
>   	ssam_irq_free(ctrl);
>   err_irq:
> @@ -782,18 +823,27 @@ static void ssam_serial_hub_remove(struct serdev_device *serdev)
>   	device_set_wakeup_capable(&serdev->dev, false);
>   }
>   
> -static const struct acpi_device_id ssam_serial_hub_match[] = {
> +static const struct acpi_device_id ssam_serial_hub_acpi_match[] = {
>   	{ "MSHW0084", 0 },
>   	{ },
>   };
> -MODULE_DEVICE_TABLE(acpi, ssam_serial_hub_match);
> +MODULE_DEVICE_TABLE(acpi, ssam_serial_hub_acpi_match);
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id ssam_serial_hub_of_match[] = {
> +	{ .compatible = "microsoft,surface-sam", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, ssam_serial_hub_of_match);
> +#endif
>   
>   static struct serdev_device_driver ssam_serial_hub = {
>   	.probe = ssam_serial_hub_probe,
>   	.remove = ssam_serial_hub_remove,
>   	.driver = {
>   		.name = "surface_serial_hub",
> -		.acpi_match_table = ssam_serial_hub_match,
> +		.acpi_match_table = ACPI_PTR(ssam_serial_hub_acpi_match),
> +		.of_match_table = of_match_ptr(ssam_serial_hub_of_match),
>   		.pm = &ssam_serial_hub_pm_ops,
>   		.shutdown = ssam_serial_hub_shutdown,
>   		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
> diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/drivers/platform/surface/surface_aggregator_registry.c
> index 1c4d74db08c9..57787f2ff38b 100644
> --- a/drivers/platform/surface/surface_aggregator_registry.c
> +++ b/drivers/platform/surface/surface_aggregator_registry.c
> @@ -12,6 +12,7 @@
>   #include <linux/acpi.h>
>   #include <linux/kernel.h>
>   #include <linux/module.h>
> +#include <linux/of.h>
>   #include <linux/platform_device.h>
>   #include <linux/property.h>
>   #include <linux/types.h>
> @@ -273,6 +274,18 @@ static const struct software_node *ssam_node_group_sl5[] = {
>   	NULL,
>   };
>   
> +/* Devices for Surface Laptop 7. */
> +static const struct software_node *ssam_node_group_sl7[] = {
> +	&ssam_node_root,
> +	&ssam_node_bat_ac,
> +	&ssam_node_bat_main,
> +	&ssam_node_tmp_perf_profile_with_fan,
> +	&ssam_node_fan_speed,
> +	&ssam_node_hid_sam_keyboard,

Did you check if there are any other HID devices connected? In the past,
keyboard and touchpad have been split into two separate devices, so is
it a combo keyboard + touchpad device this time? Some models also had
HID-based sensor and other devices.

Would just be good to know if this can be assumed to be complete or if
we're maybe missing something here.

> +	/* TODO: evaluate thermal sensors devices when we get a driver for that */

FYI I've posted the driver at [1]. It needs a small Kbuild dependency
fix but apart from that I think it should be final, if you want to give
that a try.

[1]: https://lore.kernel.org/lkml/20240804230832.247852-1-luzmaximilian@gmail.com/T/

The rest looks fine. I'll try to find some time to update my SPX branch
this weekend and give it a spin.

Regards,
Max

> +	NULL,
> +};
> +
>   /* Devices for Surface Laptop Studio. */
>   static const struct software_node *ssam_node_group_sls[] = {
>   	&ssam_node_root,
> @@ -346,7 +359,7 @@ static const struct software_node *ssam_node_group_sp9[] = {
>   
>   /* -- SSAM platform/meta-hub driver. ---------------------------------------- */
>   
> -static const struct acpi_device_id ssam_platform_hub_match[] = {
> +static const struct acpi_device_id ssam_platform_hub_acpi_match[] = {
>   	/* Surface Pro 4, 5, and 6 (OMBR < 0x10) */
>   	{ "MSHW0081", (unsigned long)ssam_node_group_gen5 },
>   
> @@ -402,16 +415,39 @@ static const struct acpi_device_id ssam_platform_hub_match[] = {
>   };
>   MODULE_DEVICE_TABLE(acpi, ssam_platform_hub_match);
>   
> +#ifdef CONFIG_OF
> +static const struct of_device_id ssam_platform_hub_of_match[] = {
> +	/* Surface Laptop 7 */
> +	{ .compatible = "microsoft,romulus13", (void *)ssam_node_group_sl7 },
> +	{ .compatible = "microsoft,romulus15", (void *)ssam_node_group_sl7 },
> +	{ },
> +};
> +#endif
> +
>   static int ssam_platform_hub_probe(struct platform_device *pdev)
>   {
>   	const struct software_node **nodes;
> +	const struct of_device_id *match;
> +	struct device_node *fdt_root;
>   	struct ssam_controller *ctrl;
>   	struct fwnode_handle *root;
>   	int status;
>   
>   	nodes = (const struct software_node **)acpi_device_get_match_data(&pdev->dev);
> -	if (!nodes)
> -		return -ENODEV;
> +	if (!nodes) {
> +		fdt_root = of_find_node_by_path("/");
> +		if (!fdt_root)
> +			return -ENODEV;
> +
> +		match = of_match_node(ssam_platform_hub_of_match, fdt_root);
> +		of_node_put(fdt_root);
> +		if (!match)
> +			return -ENODEV;
> +
> +		nodes = (const struct software_node **)match->data;
> +		if (!nodes)
> +			return -ENODEV;
> +	}
>   
>   	/*
>   	 * As we're adding the SSAM client devices as children under this device
> @@ -460,7 +496,7 @@ static struct platform_driver ssam_platform_hub_driver = {
>   	.remove_new = ssam_platform_hub_remove,
>   	.driver = {
>   		.name = "surface_aggregator_platform_hub",
> -		.acpi_match_table = ssam_platform_hub_match,
> +		.acpi_match_table = ssam_platform_hub_acpi_match,
>   		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
>   	},
>   };
> 

Re: [PATCH 3/3] platform/surface: Add OF support

[Konrad Dybcio]

On 9.08.2024 8:09 PM, Maximilian Luz wrote:
> Hi,
> 
> Thanks for taking this up! A couple of comments below:

[...]


> Doc needs updating, this is just the one copied from
> ssam_controller_caps_load_acpi().

Oops.

[...]

>>   };
>>     -/* -- ACPI based device setup. ---------------------------------------------- */
>> +/* -- Serial device setup. ---------------------------------------------- */
> 
> Nitpick, but could we maybe keep that at 80 columns please? :)

Sure


> 
>>     static acpi_status ssam_serdev_setup_via_acpi_crs(struct acpi_resource *rsc,
>>                             void *ctx)
>> @@ -352,13 +355,28 @@ static acpi_status ssam_serdev_setup_via_acpi_crs(struct acpi_resource *rsc,
>>       return AE_CTRL_TERMINATE;
>>   }
>>   -static acpi_status ssam_serdev_setup_via_acpi(acpi_handle handle,
>> -                          struct serdev_device *serdev)
>> +static int ssam_serdev_setup_via_acpi(struct serdev_device *serdev, acpi_handle handle)
>>   {
>> -    return acpi_walk_resources(handle, METHOD_NAME__CRS,
>> -                   ssam_serdev_setup_via_acpi_crs, serdev);
>> +    acpi_status status;
>> +
>> +    status = acpi_walk_resources(handle, METHOD_NAME__CRS,
>> +                     ssam_serdev_setup_via_acpi_crs, serdev);
>> +
>> +    return status ? -ENXIO : 0;
>>   }
>>   +static int ssam_serdev_setup(struct acpi_device *ssh, struct serdev_device *serdev)
>> +{
>> +    if (ssh)
>> +        return ssam_serdev_setup_via_acpi(serdev, ssh->handle);
>> +
>> +    /* TODO: these values may differ per board/implementation */
>> +    serdev_device_set_baudrate(serdev, 4 * HZ_PER_MHZ);
> 
> Isn't this defined in the DT spec that you're adding as "current-speed"
> in patch 2? Why not load it from there?

Yeah and it's not under `required:`.. i added it for future proofing

> 
>> +    serdev_device_set_flow_control(serdev, true);
>> +    serdev_device_set_parity(serdev, SERDEV_PARITY_NONE);
>> +
>> +    return 0;
>> +}
>>     /* -- Power management. ----------------------------------------------------- */
>>   @@ -624,13 +642,15 @@ static int ssam_serial_hub_probe(struct serdev_device *serdev)
>>       acpi_status astatus;
> 
> This can be removed, see below.
> 
>>       int status;
>>   -    status = gpiod_count(dev, NULL);
>> -    if (status < 0)
>> -        return dev_err_probe(dev, status, "no GPIO found\n");
>> +    if (ssh) {
>> +        status = gpiod_count(dev, NULL);
>> +        if (status < 0)
>> +            return dev_err_probe(dev, status, "no GPIO found\n");
>>   -    status = devm_acpi_dev_add_driver_gpios(dev, ssam_acpi_gpios);
>> -    if (status)
>> -        return status;
>> +        status = devm_acpi_dev_add_driver_gpios(dev, ssam_acpi_gpios);
>> +        if (status)
>> +            return status;
>> +    }
>>         /* Allocate controller. */
>>       ctrl = kzalloc(sizeof(*ctrl), GFP_KERNEL);
>> @@ -655,7 +675,7 @@ static int ssam_serial_hub_probe(struct serdev_device *serdev)
>>           goto err_devopen;
>>       }
>>   -    astatus = ssam_serdev_setup_via_acpi(ssh->handle, serdev);
>> +    astatus = ssam_serdev_setup(ssh, serdev);>       if (ACPI_FAILURE(astatus)) {
> 
> ssam_serdev_setup() returns an int, so this should now just use
> "status".

Right


> 
>>           status = dev_err_probe(dev, -ENXIO, "failed to setup serdev\n");
>>           goto err_devinit;
>> @@ -717,10 +737,31 @@ static int ssam_serial_hub_probe(struct serdev_device *serdev)
>>        *       For now let's thus default power/wakeup to false.
>>        */
>>       device_set_wakeup_capable(dev, true);
>> +
>> +    /*
>> +     * When using DT, we have to register the platform hub driver manually,
>> +     * as it can't be matched based on top-level board compatible (like it
>> +     * does the ACPI case).
>> +     */
>> +    if (!ssh) {
>> +        struct platform_device *ph_pdev =
>> +            platform_device_register_simple("surface_aggregator_platform_hub",
>> +                            0, NULL, 0);
>> +        if (IS_ERR(ph_pdev))
>> +            return dev_err_probe(dev, PTR_ERR(ph_pdev),
>> +                         "Failed to register the platform hub driver\n");
>> +    }
>> +
>> +    status = ssam_register_clients(&serdev->dev, ctrl);
>> +    if (status)
>> +        goto err_clients;
> 
> Is the ssam_register_clients() call required or is it a remnant from a
> previous version? We're now not adding any children to the controller
> itself but model ACPI and do all of that with the platform hub. So
> unless I'm missing something, I think this should not be necessary.

Yeah, the platform hub driver calls it anyway

[...]

>> +static const struct software_node *ssam_node_group_sl7[] = {
>> +    &ssam_node_root,
>> +    &ssam_node_bat_ac,
>> +    &ssam_node_bat_main,
>> +    &ssam_node_tmp_perf_profile_with_fan,
>> +    &ssam_node_fan_speed,
>> +    &ssam_node_hid_sam_keyboard,
> 
> Did you check if there are any other HID devices connected? In the past,
> keyboard and touchpad have been split into two separate devices, so is
> it a combo keyboard + touchpad device this time? Some models also had
> HID-based sensor and other devices.

No, touchpad is wired directly to the SoC via QSPI, same for the touch
panel

> 
> Would just be good to know if this can be assumed to be complete or if
> we're maybe missing something here.
> 
>> +    /* TODO: evaluate thermal sensors devices when we get a driver for that */
> 
> FYI I've posted the driver at [1]. It needs a small Kbuild dependency
> fix but apart from that I think it should be final, if you want to give
> that a try.
> 
> [1]: https://lore.kernel.org/lkml/20240804230832.247852-1-luzmaximilian@gmail.com/T/

I'll give it a shot, thanks

> 
> The rest looks fine. I'll try to find some time to update my SPX branch
> this weekend and give it a spin.

About time that thing lands upstream ;)

Konrad

Re: [PATCH 3/3] platform/surface: Add OF support

[Maximilian Luz]

On 8/10/24 12:44 AM, Konrad Dybcio wrote:
> On 9.08.2024 8:09 PM, Maximilian Luz wrote:
>>>    +static int ssam_serdev_setup(struct acpi_device *ssh, struct serdev_device *serdev)
>>> +{
>>> +    if (ssh)
>>> +        return ssam_serdev_setup_via_acpi(serdev, ssh->handle);
>>> +
>>> +    /* TODO: these values may differ per board/implementation */
>>> +    serdev_device_set_baudrate(serdev, 4 * HZ_PER_MHZ);
>>
>> Isn't this defined in the DT spec that you're adding as "current-speed"
>> in patch 2? Why not load it from there?
> 
> Yeah and it's not under `required:`.. i added it for future proofing

Okay.

[...]

>>> +static const struct software_node *ssam_node_group_sl7[] = {
>>> +    &ssam_node_root,
>>> +    &ssam_node_bat_ac,
>>> +    &ssam_node_bat_main,
>>> +    &ssam_node_tmp_perf_profile_with_fan,
>>> +    &ssam_node_fan_speed,
>>> +    &ssam_node_hid_sam_keyboard,
>>
>> Did you check if there are any other HID devices connected? In the past,
>> keyboard and touchpad have been split into two separate devices, so is
>> it a combo keyboard + touchpad device this time? Some models also had
>> HID-based sensor and other devices.
> 
> No, touchpad is wired directly to the SoC via QSPI, same for the touch
> panel

Ah I see. So somewhat similar to the SLS2 I believe. Does it work with
multiple fingers out-of-the-box? Or does it send raw heatmaps like on
the SLS2 that require post-processing?

Maybe some background: Since quite some time, the touchscreens on
Surface devices operate in two modes: Basic single-touch no-post-
processing-required mode or multi-touch mode where it sends raw touch
heatmaps to be processed by some driver or user-space application. So
basically, the whole touch processing stack is shifted to the software
side. And with the SLS2, they now even applied that same thing to the
touchpad... We're trying to replicate that (user-space) processing
with the IPTSd daemon, but there's still a bit of work to be done to
make this reliable.

> 
>>
>> Would just be good to know if this can be assumed to be complete or if
>> we're maybe missing something here.
>>
>>> +    /* TODO: evaluate thermal sensors devices when we get a driver for that */
>>
>> FYI I've posted the driver at [1]. It needs a small Kbuild dependency
>> fix but apart from that I think it should be final, if you want to give
>> that a try.
>>
>> [1]: https://lore.kernel.org/lkml/20240804230832.247852-1-luzmaximilian@gmail.com/T/
> 
> I'll give it a shot, thanks
> 
>>
>> The rest looks fine. I'll try to find some time to update my SPX branch
>> this weekend and give it a spin.
> 
> About time that thing lands upstream ;)

Agreed :) Thanks for taking this up!

Best regards,
Max

Re: [PATCH 3/3] platform/surface: Add OF support

[Konrad Dybcio]

On 10.08.2024 3:15 AM, Maximilian Luz wrote:
> On 8/10/24 12:44 AM, Konrad Dybcio wrote:
>> On 9.08.2024 8:09 PM, Maximilian Luz wrote:
>>>>    +static int ssam_serdev_setup(struct acpi_device *ssh, struct serdev_device *serdev)
>>>> +{
>>>> +    if (ssh)
>>>> +        return ssam_serdev_setup_via_acpi(serdev, ssh->handle);
>>>> +
>>>> +    /* TODO: these values may differ per board/implementation */
>>>> +    serdev_device_set_baudrate(serdev, 4 * HZ_PER_MHZ);
>>>
>>> Isn't this defined in the DT spec that you're adding as "current-speed"
>>> in patch 2? Why not load it from there?
>>
>> Yeah and it's not under `required:`.. i added it for future proofing
> 
> Okay.
> 
> [...]
> 
>>>> +static const struct software_node *ssam_node_group_sl7[] = {
>>>> +    &ssam_node_root,
>>>> +    &ssam_node_bat_ac,
>>>> +    &ssam_node_bat_main,
>>>> +    &ssam_node_tmp_perf_profile_with_fan,
>>>> +    &ssam_node_fan_speed,
>>>> +    &ssam_node_hid_sam_keyboard,
>>>
>>> Did you check if there are any other HID devices connected? In the past,
>>> keyboard and touchpad have been split into two separate devices, so is
>>> it a combo keyboard + touchpad device this time? Some models also had
>>> HID-based sensor and other devices.
>>
>> No, touchpad is wired directly to the SoC via QSPI, same for the touch
>> panel
> 
> Ah I see. So somewhat similar to the SLS2 I believe. Does it work with
> multiple fingers out-of-the-box? Or does it send raw heatmaps like on
> the SLS2 that require post-processing?

We'll see when the QSPI driver for this soc is done ;)

Konrad

Re: [PATCH 3/3] platform/surface: Add OF support

[Konrad Dybcio]

On 9.08.2024 3:48 AM, Konrad Dybcio wrote:
> From: Konrad Dybcio <quic_kdybcio@quicinc.com>
> 
> Add basic support for registering the aggregator module on Device Tree-
> based platforms. These include at least three generations of Qualcomm
> Snapdragon-based Surface devices:
> 
> - SC8180X / SQ1 / SQ2: Pro X,
> - SC8280XP / SQ3: Devkit 2023, Pro 9
> - X Elite: Laptop 7 / Pro11
> 
> Thankfully, the aggregators on these seem to be configured in an
> identical way, which allows for using these settings as defaults and
> no DT properties need to be introduced (until that changes, anyway).
> 
> Based on the work done by Maximilian Luz, largely rewritten.
> 
> Signed-off-by: Konrad Dybcio <quic_kdybcio@quicinc.com>
> ---

[...]

> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -2444,7 +2444,8 @@ static int acpi_walk_dep_device_list(acpi_handle handle,
>   */
>  void acpi_dev_clear_dependencies(struct acpi_device *supplier)
>  {
> -	acpi_walk_dep_device_list(supplier->handle, acpi_scan_clear_dep, NULL);
> +	if (supplier)
> +		acpi_walk_dep_device_list(supplier->handle, acpi_scan_clear_dep, NULL);
>  }
>  EXPORT_SYMBOL_GPL(acpi_dev_clear_dependencies);

Err this shouldn't have made it into this patch

Konrad