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

[PATCH v3 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>
---
Changes in v3:
- Drop unnecessary nullchecks
- Add MODULE_ALIAS in the platform hub driver
- Fix MODULE_DEVICE_TABLE after rename
- Prolong the '----' comment to 80 lines
- Change the current-speed bindings description to ": true", in
  preparation for krzk's serial device bindings reorganization
- Link to v2: https://lore.kernel.org/r/20240810-topic-sam-v2-0-8a8eb368a4f0@quicinc.com

Changes in v2:
- Fix kerneldoc
- Drop the drivers/acpi change (oops)
- Style fixes
- Don't assign int to acpi_status
- Don't scan the bus twice in SAM core probe
- Link to v1: https://lore.kernel.org/r/20240809-topic-sam-v1-0-05bca1932614@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   | 47 +++++++++++++
 .../devicetree/bindings/serial/serial.yaml         |  2 +-
 drivers/platform/surface/aggregator/bus.c          |  2 +
 drivers/platform/surface/aggregator/controller.c   | 67 ++++++++++++++----
 drivers/platform/surface/aggregator/core.c         | 82 +++++++++++++++++-----
 drivers/platform/surface/surface3_power.c          |  1 +
 drivers/platform/surface/surface_acpi_notify.c     |  1 +
 .../platform/surface/surface_aggregator_registry.c | 47 +++++++++++--
 8 files changed, 210 insertions(+), 39 deletions(-)
---
base-commit: 1e391b34f6aa043c7afa40a2103163a0ef06d179
change-id: 20240809-topic-sam-5de2f0ec9370

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

[PATCH v3 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.

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
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 v3 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   | 47 ++++++++++++++++++++++
 1 file changed, 47 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..b33d26f15b2a
--- /dev/null
+++ b/Documentation/devicetree/bindings/platform/microsoft,surface-sam.yaml
@@ -0,0 +1,47 @@
+# 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: true
+
+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 v3 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/platform/surface/aggregator/bus.c          |  2 +
 drivers/platform/surface/aggregator/controller.c   | 67 ++++++++++++++----
 drivers/platform/surface/aggregator/core.c         | 82 +++++++++++++++++-----
 drivers/platform/surface/surface3_power.c          |  1 +
 drivers/platform/surface/surface_acpi_notify.c     |  1 +
 .../platform/surface/surface_aggregator_registry.c | 47 +++++++++++--
 6 files changed, 162 insertions(+), 38 deletions(-)

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..27eadf22b292 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,52 @@ int ssam_controller_caps_load_from_acpi(acpi_handle handle,
 	return 0;
 }
 
+/**
+ * ssam_controller_caps_load_from_of() - Load controller capabilities from OF/DT.
+ * @dev:  A pointer to the controller device
+ * @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() - Load controller capabilities
+ * @dev:  A pointer to the controller device
+ * @caps: Where to store the capabilities in.
+ *
+ * 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 +1204,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 +2753,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..c58e1fdd1a5f 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. ----------------------------------------------------- */
 
@@ -621,16 +639,17 @@ static int ssam_serial_hub_probe(struct serdev_device *serdev)
 	struct device *dev = &serdev->dev;
 	struct acpi_device *ssh = ACPI_COMPANION(dev);
 	struct ssam_controller *ctrl;
-	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,9 +674,9 @@ static int ssam_serial_hub_probe(struct serdev_device *serdev)
 		goto err_devopen;
 	}
 
-	astatus = ssam_serdev_setup_via_acpi(ssh->handle, serdev);
-	if (ACPI_FAILURE(astatus)) {
-		status = dev_err_probe(dev, -ENXIO, "failed to setup serdev\n");
+	status = ssam_serdev_setup(ssh, serdev);
+	if (status) {
+		status = dev_err_probe(dev, status, "failed to setup serdev\n");
 		goto err_devinit;
 	}
 
@@ -717,7 +736,23 @@ 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);
-	acpi_dev_clear_dependencies(ssh);
+
+	/*
+	 * 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");
+	}
+
+	if (ssh)
+		acpi_dev_clear_dependencies(ssh);
 
 	return 0;
 
@@ -782,18 +817,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/surface3_power.c b/drivers/platform/surface/surface3_power.c
index 4c0f92562a79..31835f793a10 100644
--- a/drivers/platform/surface/surface3_power.c
+++ b/drivers/platform/surface/surface3_power.c
@@ -479,6 +479,7 @@ static int mshw0011_install_space_handler(struct i2c_client *client)
 	}
 
 	acpi_dev_clear_dependencies(adev);
+
 	return 0;
 }
 
diff --git a/drivers/platform/surface/surface_acpi_notify.c b/drivers/platform/surface/surface_acpi_notify.c
index 20f3870915d2..3be1f1730046 100644
--- a/drivers/platform/surface/surface_acpi_notify.c
+++ b/drivers/platform/surface/surface_acpi_notify.c
@@ -816,6 +816,7 @@ static int san_probe(struct platform_device *pdev)
 		goto err_install_dev;
 
 	acpi_dev_clear_dependencies(san);
+
 	return 0;
 
 err_install_dev:
diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/drivers/platform/surface/surface_aggregator_registry.c
index 1c4d74db08c9..495cb4300617 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 },
 
@@ -400,18 +413,41 @@ static const struct acpi_device_id ssam_platform_hub_match[] = {
 
 	{ },
 };
-MODULE_DEVICE_TABLE(acpi, ssam_platform_hub_match);
+MODULE_DEVICE_TABLE(acpi, ssam_platform_hub_acpi_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,12 +496,13 @@ 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,
 	},
 };
 module_platform_driver(ssam_platform_hub_driver);
 
+MODULE_ALIAS("platform:surface_aggregator_platform_hub");
 MODULE_AUTHOR("Maximilian Luz <luzmaximilian@gmail.com>");
 MODULE_DESCRIPTION("Device-registry for Surface System Aggregator Module");
 MODULE_LICENSE("GPL");

-- 
2.46.0

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

[Krzysztof Kozlowski]

On 14/08/2024 12:27, Konrad Dybcio wrote:
> 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>
> ---

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

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

[Maximilian Luz]

On 8/14/24 12:27 PM, 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>
> ---

Looks good to me and works without issues on my Surface Pro X.

Thanks again for picking this up!

Reviewed-by: Maximilian Luz <luzmaximilian@gmail.com>
Tested-by: Maximilian Luz <luzmaximilian@gmail.com>

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

[Hans de Goede]

Hi,

On 8/14/24 12:27 PM, Konrad Dybcio wrote:
> 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>

Thank you for your patch-series, I've applied the series to my
review-hans branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

I did notice the following compiler warning when test building:

drivers/platform/surface/surface_aggregator_registry.c:278:36: warning: ‘ssam_node_group_sl7’ defined but not used [-Wunused-variable]
  278 | static const struct software_node *ssam_node_group_sl7[] = {
      |                                    ^~~~~~~~~~~~~~~~~~~

One way to fix this would be add #ifdef CONFIG_OF around the definition
of ssam_node_group_sl7, but then future devicetree based surface devices
would need more #ifdef-s so instead I've solved it by squashing in this fix:

diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/drivers/platform/surface/surface_aggregator_registry.c
index 495cb4300617..ac96e883cb57 100644
--- a/drivers/platform/surface/surface_aggregator_registry.c
+++ b/drivers/platform/surface/surface_aggregator_registry.c
@@ -415,14 +415,12 @@ static const struct acpi_device_id ssam_platform_hub_acpi_match[] = {
 };
 MODULE_DEVICE_TABLE(acpi, ssam_platform_hub_acpi_match);
 
-#ifdef CONFIG_OF
-static const struct of_device_id ssam_platform_hub_of_match[] = {
+static const struct of_device_id ssam_platform_hub_of_match[] __maybe_unused = {
 	/* 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)
 {

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans


> ---
> Changes in v3:
> - Drop unnecessary nullchecks
> - Add MODULE_ALIAS in the platform hub driver
> - Fix MODULE_DEVICE_TABLE after rename
> - Prolong the '----' comment to 80 lines
> - Change the current-speed bindings description to ": true", in
>   preparation for krzk's serial device bindings reorganization
> - Link to v2: https://lore.kernel.org/r/20240810-topic-sam-v2-0-8a8eb368a4f0@quicinc.com
> 
> Changes in v2:
> - Fix kerneldoc
> - Drop the drivers/acpi change (oops)
> - Style fixes
> - Don't assign int to acpi_status
> - Don't scan the bus twice in SAM core probe
> - Link to v1: https://lore.kernel.org/r/20240809-topic-sam-v1-0-05bca1932614@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   | 47 +++++++++++++
>  .../devicetree/bindings/serial/serial.yaml         |  2 +-
>  drivers/platform/surface/aggregator/bus.c          |  2 +
>  drivers/platform/surface/aggregator/controller.c   | 67 ++++++++++++++----
>  drivers/platform/surface/aggregator/core.c         | 82 +++++++++++++++++-----
>  drivers/platform/surface/surface3_power.c          |  1 +
>  drivers/platform/surface/surface_acpi_notify.c     |  1 +
>  .../platform/surface/surface_aggregator_registry.c | 47 +++++++++++--
>  8 files changed, 210 insertions(+), 39 deletions(-)
> ---
> base-commit: 1e391b34f6aa043c7afa40a2103163a0ef06d179
> change-id: 20240809-topic-sam-5de2f0ec9370
> 
> Best regards,

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

[Konrad Dybcio]

On 19.08.2024 1:57 PM, Hans de Goede wrote:
> Hi,
> 
> On 8/14/24 12:27 PM, Konrad Dybcio wrote:
>> 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>
> 
> Thank you for your patch-series, I've applied the series to my
> review-hans branch:
> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
> 
> I did notice the following compiler warning when test building:
> 
> drivers/platform/surface/surface_aggregator_registry.c:278:36: warning: ‘ssam_node_group_sl7’ defined but not used [-Wunused-variable]
>   278 | static const struct software_node *ssam_node_group_sl7[] = {
>       |                                    ^~~~~~~~~~~~~~~~~~~
> 
> One way to fix this would be add #ifdef CONFIG_OF around the definition
> of ssam_node_group_sl7, but then future devicetree based surface devices
> would need more #ifdef-s so instead I've solved it by squashing in this fix:
> 
> diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/drivers/platform/surface/surface_aggregator_registry.c
> index 495cb4300617..ac96e883cb57 100644
> --- a/drivers/platform/surface/surface_aggregator_registry.c
> +++ b/drivers/platform/surface/surface_aggregator_registry.c
> @@ -415,14 +415,12 @@ static const struct acpi_device_id ssam_platform_hub_acpi_match[] = {
>  };
>  MODULE_DEVICE_TABLE(acpi, ssam_platform_hub_acpi_match);
>  
> -#ifdef CONFIG_OF
> -static const struct of_device_id ssam_platform_hub_of_match[] = {
> +static const struct of_device_id ssam_platform_hub_of_match[] __maybe_unused = {
>  	/* 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)
>  {
> 
> Once I've run some tests on this branch the patches there will be
> added to the platform-drivers-x86/for-next branch and eventually
> will be included in the pdx86 pull-request to Linus for the next
> merge-window.

Thanks for pointing this out. Your fix seems to be the best solution
I can think of, so I'm all for it

Konrad

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

[Maximilian Luz]

On 8/19/24 1:57 PM, Hans de Goede wrote:
> Hi,
> 
> On 8/14/24 12:27 PM, Konrad Dybcio wrote:
>> 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>
> 
> Thank you for your patch-series, I've applied the series to my
> review-hans branch:
> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
> 
> I did notice the following compiler warning when test building:
> 
> drivers/platform/surface/surface_aggregator_registry.c:278:36: warning: ‘ssam_node_group_sl7’ defined but not used [-Wunused-variable]
>    278 | static const struct software_node *ssam_node_group_sl7[] = {
>        |                                    ^~~~~~~~~~~~~~~~~~~
> 
> One way to fix this would be add #ifdef CONFIG_OF around the definition
> of ssam_node_group_sl7, but then future devicetree based surface devices
> would need more #ifdef-s so instead I've solved it by squashing in this fix:
> 
> diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/drivers/platform/surface/surface_aggregator_registry.c
> index 495cb4300617..ac96e883cb57 100644
> --- a/drivers/platform/surface/surface_aggregator_registry.c
> +++ b/drivers/platform/surface/surface_aggregator_registry.c
> @@ -415,14 +415,12 @@ static const struct acpi_device_id ssam_platform_hub_acpi_match[] = {
>   };
>   MODULE_DEVICE_TABLE(acpi, ssam_platform_hub_acpi_match);
>   
> -#ifdef CONFIG_OF
> -static const struct of_device_id ssam_platform_hub_of_match[] = {
> +static const struct of_device_id ssam_platform_hub_of_match[] __maybe_unused = {
>   	/* 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)
>   {
> 
> Once I've run some tests on this branch the patches there will be
> added to the platform-drivers-x86/for-next branch and eventually
> will be included in the pdx86 pull-request to Linus for the next
> merge-window.

I agree with Konrad, this looks like the best way to address this.
Thanks!

Best regards,
Max

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

[Andy Shevchenko]

Wed, Aug 14, 2024 at 12:27:27PM +0200, Konrad Dybcio kirjoitti:
> 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.

...

>  	sdev->dev.fwnode = fwnode_handle_get(node);
> +	sdev->dev.of_node = to_of_node(node);

Please, use device_set_node() instead of those two.

...

> +static int ssam_controller_caps_load(struct device *dev, struct ssam_controller_caps *caps)
> +{

> +	acpi_handle handle = ACPI_HANDLE(dev);

It's a bit non-standard way to check if we run on DT or ACPI. The others (most
of them?) do something like this:

	struct fwnode_handle *fwnode = dev_fwnode(...);

	if (is_of_node(fwnode))
		return X;
	if (is_acpi_node(fwnode)) // also more precise _device or _data variant
		return Y;

	return ERROR;

> +	/* 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);

Yeah, I see that you use handle here, that's why it's up to you how to proceed
with that. 

> +	else
> +		return ssam_controller_caps_load_from_of(dev, caps);

But just note that we have 4 options for fwnode type here and this covers 3 and
I don't know if you ever have an ACPI data node or software node and what you
want to do with that.

> +}

...

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

Can't you try fwnode_irq_get_byname() followed by fwnode_irq_get()? And why do
you need unnamed variant to begin with? As far as I understand it's pure DT and
names are there, no?

...

>  #include <linux/kernel.h>
>  #include <linux/kref.h>
>  #include <linux/module.h>
> +#include <linux/of.h>

I do not see how you use this. You probably missed mod_devicetable.h.

> +#include <linux/platform_device.h>
>  #include <linux/pm.h>
>  #include <linux/serdev.h>
>  #include <linux/sysfs.h>

...

> +	/*
> +	 * 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");

> +	}
> +
> +	if (ssh)

Simply 'else' ? And making condition positive?

...

> -static const struct acpi_device_id ssam_serial_hub_match[] = {
> +static const struct acpi_device_id ssam_serial_hub_acpi_match[] = {
>  	{ "MSHW0084", 0 },
>  	{ },

At some point, please drop that 0 part above and the comma in the terminator
entry.

>  };
> -MODULE_DEVICE_TABLE(acpi, ssam_serial_hub_match);
> +MODULE_DEVICE_TABLE(acpi, ssam_serial_hub_acpi_match);

Do you really need this renaming?

...

> +#ifdef CONFIG_OF
> +static const struct of_device_id ssam_serial_hub_of_match[] = {
> +	{ .compatible = "microsoft,surface-sam", },

No inner comma.

> +	{ },

No comma for the terminator.

> +};
> +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),

No, please do not use ACPI_PTR(), it's more harmful than helpful.

> +		.of_match_table = of_match_ptr(ssam_serial_hub_of_match),

There is ongoing task to drop of_match_ptr(), so for ACPI_PTR().

>  		.pm = &ssam_serial_hub_pm_ops,
>  		.shutdown = ssam_serial_hub_shutdown,
>  		.probe_type = PROBE_PREFER_ASYNCHRONOUS,

...

> --- a/drivers/platform/surface/surface3_power.c
> +++ b/drivers/platform/surface/surface3_power.c
> @@ -479,6 +479,7 @@ static int mshw0011_install_space_handler(struct i2c_client *client)
>  	}
>  
>  	acpi_dev_clear_dependencies(adev);
> +
>  	return 0;
>  }

Stray change.

...

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

At some point please drop commas at the terminator entries. This will make code
more robust against quite unlikely but potential rebase-like mistakes (when a
new entry is added behind the terminator).

> +};

...

> -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 },
>  
> @@ -400,18 +413,41 @@ static const struct acpi_device_id ssam_platform_hub_match[] = {
>  
>  	{ },
>  };
> -MODULE_DEVICE_TABLE(acpi, ssam_platform_hub_match);
> +MODULE_DEVICE_TABLE(acpi, ssam_platform_hub_acpi_match);

If renaming is needed, it can be done in a separate patch.

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

As per above.

...

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

Hmm... Why this doesn't use simple device_get_match_data()?

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

This is quite strange! Where are they being defined?

> +		if (!nodes)
> +			return -ENODEV;
> +	}

...

> +MODULE_ALIAS("platform:surface_aggregator_platform_hub");

Can it be platfrom device ID table instead? But do you really need it?

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

Wed, Aug 14, 2024 at 12:27:24PM +0200, Konrad Dybcio kirjoitti:
> 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

Please, Cc to me the v4.

-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

Hi Andy,

Thank you for the review.

Note this has already been merged though.

Still there are some good suggestions here for a follow-up
cleanup patch.

Regards,

Hans


On 8/26/24 10:54 PM, Andy Shevchenko wrote:
> Wed, Aug 14, 2024 at 12:27:27PM +0200, Konrad Dybcio kirjoitti:
>> 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.
> 
> ...
> 
>>  	sdev->dev.fwnode = fwnode_handle_get(node);
>> +	sdev->dev.of_node = to_of_node(node);
> 
> Please, use device_set_node() instead of those two.
> 
> ...
> 
>> +static int ssam_controller_caps_load(struct device *dev, struct ssam_controller_caps *caps)
>> +{
> 
>> +	acpi_handle handle = ACPI_HANDLE(dev);
> 
> It's a bit non-standard way to check if we run on DT or ACPI. The others (most
> of them?) do something like this:
> 
> 	struct fwnode_handle *fwnode = dev_fwnode(...);
> 
> 	if (is_of_node(fwnode))
> 		return X;
> 	if (is_acpi_node(fwnode)) // also more precise _device or _data variant
> 		return Y;
> 
> 	return ERROR;
> 
>> +	/* 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);
> 
> Yeah, I see that you use handle here, that's why it's up to you how to proceed
> with that. 
> 
>> +	else
>> +		return ssam_controller_caps_load_from_of(dev, caps);
> 
> But just note that we have 4 options for fwnode type here and this covers 3 and
> I don't know if you ever have an ACPI data node or software node and what you
> want to do with that.
> 
>> +}
> 
> ...
> 
>>  	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);
>> +	}
> 
> Can't you try fwnode_irq_get_byname() followed by fwnode_irq_get()? And why do
> you need unnamed variant to begin with? As far as I understand it's pure DT and
> names are there, no?
> 
> ...
> 
>>  #include <linux/kernel.h>
>>  #include <linux/kref.h>
>>  #include <linux/module.h>
>> +#include <linux/of.h>
> 
> I do not see how you use this. You probably missed mod_devicetable.h.
> 
>> +#include <linux/platform_device.h>
>>  #include <linux/pm.h>
>>  #include <linux/serdev.h>
>>  #include <linux/sysfs.h>
> 
> ...
> 
>> +	/*
>> +	 * 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");
> 
>> +	}
>> +
>> +	if (ssh)
> 
> Simply 'else' ? And making condition positive?
> 
> ...
> 
>> -static const struct acpi_device_id ssam_serial_hub_match[] = {
>> +static const struct acpi_device_id ssam_serial_hub_acpi_match[] = {
>>  	{ "MSHW0084", 0 },
>>  	{ },
> 
> At some point, please drop that 0 part above and the comma in the terminator
> entry.
> 
>>  };
>> -MODULE_DEVICE_TABLE(acpi, ssam_serial_hub_match);
>> +MODULE_DEVICE_TABLE(acpi, ssam_serial_hub_acpi_match);
> 
> Do you really need this renaming?
> 
> ...
> 
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id ssam_serial_hub_of_match[] = {
>> +	{ .compatible = "microsoft,surface-sam", },
> 
> No inner comma.
> 
>> +	{ },
> 
> No comma for the terminator.
> 
>> +};
>> +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),
> 
> No, please do not use ACPI_PTR(), it's more harmful than helpful.
> 
>> +		.of_match_table = of_match_ptr(ssam_serial_hub_of_match),
> 
> There is ongoing task to drop of_match_ptr(), so for ACPI_PTR().
> 
>>  		.pm = &ssam_serial_hub_pm_ops,
>>  		.shutdown = ssam_serial_hub_shutdown,
>>  		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
> 
> ...
> 
>> --- a/drivers/platform/surface/surface3_power.c
>> +++ b/drivers/platform/surface/surface3_power.c
>> @@ -479,6 +479,7 @@ static int mshw0011_install_space_handler(struct i2c_client *client)
>>  	}
>>  
>>  	acpi_dev_clear_dependencies(adev);
>> +
>>  	return 0;
>>  }
> 
> Stray change.
> 
> ...
> 
>> +/* 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,
> 
> At some point please drop commas at the terminator entries. This will make code
> more robust against quite unlikely but potential rebase-like mistakes (when a
> new entry is added behind the terminator).
> 
>> +};
> 
> ...
> 
>> -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 },
>>  
>> @@ -400,18 +413,41 @@ static const struct acpi_device_id ssam_platform_hub_match[] = {
>>  
>>  	{ },
>>  };
>> -MODULE_DEVICE_TABLE(acpi, ssam_platform_hub_match);
>> +MODULE_DEVICE_TABLE(acpi, ssam_platform_hub_acpi_match);
> 
> If renaming is needed, it can be done in a separate patch.
> 
>> +#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
> 
> As per above.
> 
> ...
> 
>>  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);
> 
> Hmm... Why this doesn't use simple device_get_match_data()?
> 
>> -	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;
> 
> This is quite strange! Where are they being defined?
> 
>> +		if (!nodes)
>> +			return -ENODEV;
>> +	}
> 
> ...
> 
>> +MODULE_ALIAS("platform:surface_aggregator_platform_hub");
> 
> Can it be platfrom device ID table instead? But do you really need it?
> 

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

[Konrad Dybcio]

On 27.08.2024 11:07 AM, Hans de Goede wrote:
> Hi Andy,
> 
> Thank you for the review.
> 
> Note this has already been merged though.
> 
> Still there are some good suggestions here for a follow-up
> cleanup patch.

Andy, Hans

Is it fine if I submit a fat "address review comments" patch, or should
I split it up per issue?

Konrad

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

[Hans de Goede]

Hi,

On 8/27/24 3:52 PM, Konrad Dybcio wrote:
> On 27.08.2024 11:07 AM, Hans de Goede wrote:
>> Hi Andy,
>>
>> Thank you for the review.
>>
>> Note this has already been merged though.
>>
>> Still there are some good suggestions here for a follow-up
>> cleanup patch.
> 
> Andy, Hans
> 
> Is it fine if I submit a fat "address review comments" patch, or should
> I split it up per issue?

I would say use your best judgement / something in the middle.

IOW start with a fat "address review comments" patch and if
specific parts turn out to be somewhat involved do 1 patch
with smaller clean-ups and split out the involved parts in
a separate patch.

Thanks & Regards,

Hans

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

[Maximilian Luz]

Hi,

I thought I should provide some context:

Am 26/08/2024 um 22:54 schrieb Andy Shevchenko:
> Wed, Aug 14, 2024 at 12:27:27PM +0200, Konrad Dybcio kirjoitti:
>> From: Konrad Dybcio <quic_kdybcio@quicinc.com>

[...]

>> +	/*
>> +	 * 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"); 
>> +	}

[...]

>>   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);
> 
> Hmm... Why this doesn't use simple device_get_match_data()?
> 
>> -	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;
> 
> This is quite strange! Where are they being defined?

Essentially, this whole module is a giant workaround because there
doesn't seem to be a way to auto-discover which functions or subdevices
the EC actually supports. So this module builds a registry of software
nodes and matches against a Surface-model-specific ACPI ID (in ACPI
mode). Based on that ID, we retrieve the tree of software nodes that
define the EC subdevices and register them using a (virtual) platform
hub device.

The snippet way above registers the platform hub device for DT,
because there we don't have an equivalent ACPI device that we can
use. The code here retrieves the respective nodes.

>> +		if (!nodes)
>> +			return -ENODEV;
>> +	}
> 
> ...
> 
>> +MODULE_ALIAS("platform:surface_aggregator_platform_hub");
> 
> Can it be platfrom device ID table instead? But do you really need it?
> 

I think the explanation above already kind of answers this, but the
module is named differently than the driver (so that they reflect the
specific nature of each, registry vs hub device). And the platform hub
device added in the snippet I left above is named after the driver. So
for the registry module to load when the platform hub driver is
requested, it is needed.

Regards,
Max

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

[Andy Shevchenko]

On Wed, Aug 28, 2024 at 12:10 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:

> I thought I should provide some context:

Thank you, my reply below.

> Am 26/08/2024 um 22:54 schrieb Andy Shevchenko:
> > Wed, Aug 14, 2024 at 12:27:27PM +0200, Konrad Dybcio kirjoitti:
> >> From: Konrad Dybcio <quic_kdybcio@quicinc.com>

[...]

> >>      nodes = (const struct software_node **)acpi_device_get_match_data(&pdev->dev);
> >
> > Hmm... Why this doesn't use simple device_get_match_data()?
> >
> >> -    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;
> >
> > This is quite strange! Where are they being defined?
>
> Essentially, this whole module is a giant workaround because there
> doesn't seem to be a way to auto-discover which functions or subdevices
> the EC actually supports. So this module builds a registry of software
> nodes and matches against a Surface-model-specific ACPI ID (in ACPI
> mode). Based on that ID, we retrieve the tree of software nodes that
> define the EC subdevices and register them using a (virtual) platform
> hub device.
>
> The snippet way above registers the platform hub device for DT,
> because there we don't have an equivalent ACPI device that we can
> use. The code here retrieves the respective nodes.

Yes, and software nodes for DT are quite strange things! Why can't you
simply fix the DT to begin with?

> >> +            if (!nodes)
> >> +                    return -ENODEV;
> >> +    }

...

> >> +MODULE_ALIAS("platform:surface_aggregator_platform_hub");
> >
> > Can it be platfrom device ID table instead? But do you really need it?
> >
>
> I think the explanation above already kind of answers this, but the
> module is named differently than the driver (so that they reflect the
> specific nature of each, registry vs hub device). And the platform hub
> device added in the snippet I left above is named after the driver. So
> for the registry module to load when the platform hub driver is
> requested, it is needed.

So, I believe it warrants a platform device ID table to make it explicit.

-- 
With Best Regards,
Andy Shevchenko

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

[Maximilian Luz]

On 8/28/24 6:56 PM, Andy Shevchenko wrote:
> On Wed, Aug 28, 2024 at 12:10 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
> 
>> I thought I should provide some context:
> 
> Thank you, my reply below.
> 
>> Am 26/08/2024 um 22:54 schrieb Andy Shevchenko:
>>> Wed, Aug 14, 2024 at 12:27:27PM +0200, Konrad Dybcio kirjoitti:
>>>> From: Konrad Dybcio <quic_kdybcio@quicinc.com>
> 
> [...]
> 
>>>>       nodes = (const struct software_node **)acpi_device_get_match_data(&pdev->dev);
>>>
>>> Hmm... Why this doesn't use simple device_get_match_data()?
>>>
>>>> -    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;
>>>
>>> This is quite strange! Where are they being defined?
>>
>> Essentially, this whole module is a giant workaround because there
>> doesn't seem to be a way to auto-discover which functions or subdevices
>> the EC actually supports. So this module builds a registry of software
>> nodes and matches against a Surface-model-specific ACPI ID (in ACPI
>> mode). Based on that ID, we retrieve the tree of software nodes that
>> define the EC subdevices and register them using a (virtual) platform
>> hub device.
>>
>> The snippet way above registers the platform hub device for DT,
>> because there we don't have an equivalent ACPI device that we can
>> use. The code here retrieves the respective nodes.
> 
> Yes, and software nodes for DT are quite strange things! Why can't you
> simply fix the DT to begin with?

For the ARM/DT variants we could do that. But we still have to deal with
the x86/ACPI ones here. So for me it makes more sense to have it unified
and just deal with everything in this module.

Also, if we consider that at some point we might get ACPI PEP support (I
know, far fetched right now): With that, ACPI on ARM might be feasible
and then we'd have to manage the same thing in two places...

And lastly, the EC subdevices are quite contained and I don't see them
interacting with any other components in the DT, so it's more of a
stylistic choice where to put them.

>>>> +            if (!nodes)
>>>> +                    return -ENODEV;
>>>> +    }
> 
> ...
> 
>>>> +MODULE_ALIAS("platform:surface_aggregator_platform_hub");
>>>
>>> Can it be platfrom device ID table instead? But do you really need it?
>>>
>>
>> I think the explanation above already kind of answers this, but the
>> module is named differently than the driver (so that they reflect the
>> specific nature of each, registry vs hub device). And the platform hub
>> device added in the snippet I left above is named after the driver. So
>> for the registry module to load when the platform hub driver is
>> requested, it is needed.
> 
> So, I believe it warrants a platform device ID table to make it explicit.

Yes, that makes sense. (I was not arguing against that, just wanted to
explain why we need the match at all.)

Best regards,
Max

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

[Andy Shevchenko]

On Wed, Aug 28, 2024 at 8:40 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
> On 8/28/24 6:56 PM, Andy Shevchenko wrote:
> > On Wed, Aug 28, 2024 at 12:10 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:

...

> > Yes, and software nodes for DT are quite strange things! Why can't you
> > simply fix the DT to begin with?
>
> For the ARM/DT variants we could do that. But we still have to deal with
> the x86/ACPI ones here.

So, then fix it there! Currently it's an abuse of software nodes
inside the Linux kernel.

> So for me it makes more sense to have it unified
> and just deal with everything in this module.

I understand the desire, but DT is DT and ACPI is ACPI, they are
different despite having some common APIs in the Linux kernel.
Moreover, DT has a validation tools and everything, making that being
a software nodes has at least these disadvantages:
- no official schema that must be supported and users are known of
- no validation done
- bloating of the Linux kernel binary and hence memory footprint

> Also, if we consider that at some point we might get ACPI PEP support (I
> know, far fetched right now): With that, ACPI on ARM might be feasible
> and then we'd have to manage the same thing in two places...

This (PEP) is something I have no knowledge about. But I think it's
still orthogonal to the software nodes usage.

> And lastly, the EC subdevices are quite contained and I don't see them
> interacting with any other components in the DT, so it's more of a
> stylistic choice where to put them.

They are still part of hardware and DT describes hardware.

-- 
With Best Regards,
Andy Shevchenko

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

[Konrad Dybcio]

On 28.08.2024 9:06 PM, Andy Shevchenko wrote:
> On Wed, Aug 28, 2024 at 8:40 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
>> On 8/28/24 6:56 PM, Andy Shevchenko wrote:
>>> On Wed, Aug 28, 2024 at 12:10 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
> 
> ...
> 
>>> Yes, and software nodes for DT are quite strange things! Why can't you
>>> simply fix the DT to begin with?
>>
>> For the ARM/DT variants we could do that. But we still have to deal with
>> the x86/ACPI ones here.
> 
> So, then fix it there! Currently it's an abuse of software nodes
> inside the Linux kernel.
> 
>> So for me it makes more sense to have it unified
>> and just deal with everything in this module.
> 
> I understand the desire, but DT is DT and ACPI is ACPI, they are
> different despite having some common APIs in the Linux kernel.
> Moreover, DT has a validation tools and everything, making that being
> a software nodes has at least these disadvantages:
> - no official schema that must be supported and users are known of
> - no validation done
> - bloating of the Linux kernel binary and hence memory footprint

Arguably the last point isn't very strong.. DT also has to store some
strings and pointers to represent devices

> 
>> Also, if we consider that at some point we might get ACPI PEP support (I
>> know, far fetched right now): With that, ACPI on ARM might be feasible
>> and then we'd have to manage the same thing in two places...
> 
> This (PEP) is something I have no knowledge about. But I think it's
> still orthogonal to the software nodes usage.

The PEP (Power Engine Plugin) unfortunately is the reason we can't have
ACPI-based boot on WoA platforms.. This two-or-three-digit megabyte
Windows driver hardcodes almost everything related to the on-SoC power
management (buses, clocks, etc.) and only uses the bare minimum ACPI it
needs to connect devices to a bus or get notifications on standard events..

> 
>> And lastly, the EC subdevices are quite contained and I don't see them
>> interacting with any other components in the DT, so it's more of a
>> stylistic choice where to put them.
> 
> They are still part of hardware and DT describes hardware.

Unfortunately the "Surface Aggregator Module" is just a firmware
exposed on some range of MCUs running MSFT's code..

Given how.. peculiarly the "bus" that it hosts """devices""" on is
constructed (5-level-deep hierarchy without it making much sense
beyond maaaybe the first two), it's not really easy to describe in
DT in a way that would be both true to the bigger picture and make
enough sense to convince the DT maintainers, I don't think

Konrad