Re: [PATCH v2 3/5] platform/surface: aggregator_registry: Add Surface Pro 9 5G

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Looks good. Two very small nit-picks below, if this goes for a v3:

On 9/9/24 12:35 AM, Jérôme de Bretagne wrote:
Add SAM client device nodes for the Surface Pro 9 5G, with the usual
battery/AC and HID nodes for keyboard and touchpad support.

Signed-off-by: Jérôme de Bretagne <jerome.debretagne@xxxxxxxxx>
---
  .../surface/surface_aggregator_registry.c       | 17 +++++++++++++++++
  1 file changed, 17 insertions(+)

diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/drivers/platform/surface/surface_aggregator_registry.c
index 25c8aa2131d6..8b34d7e465c2 100644
--- a/drivers/platform/surface/surface_aggregator_registry.c
+++ b/drivers/platform/surface/surface_aggregator_registry.c
@@ -390,6 +390,21 @@ static const struct software_node *ssam_node_group_sp9[] = {
  	NULL,
  };
+/* Devices for Surface Pro 9 5G. */

Would be nice if you could change the comment on the SP9 node group to
"Surface Pro 9 (Intel/x86)" and the comment here to "Surface Pro 9 5G
(ARM/QCOM)" or something along those lines to make things a bit more
clear.

+static const struct software_node *ssam_node_group_sp9_5G[] = {

(This is really just me being a bit obsessive:) It would be nice to have
all-lowercase variable names (regarding the 5G).

+	&ssam_node_root,
+	&ssam_node_hub_kip,
+	&ssam_node_bat_ac,
+	&ssam_node_bat_main,
+	&ssam_node_tmp_sensors,
+	&ssam_node_hid_kip_keyboard,
+	&ssam_node_hid_kip_penstash,
+	&ssam_node_hid_kip_touchpad,
+	&ssam_node_hid_kip_fwupd,
+	&ssam_node_hid_sam_sensors,
+	&ssam_node_kip_tablet_switch,
+	NULL,
+};
/* -- SSAM platform/meta-hub driver. ---------------------------------------- */ @@ -462,6 +477,8 @@ static const struct acpi_device_id ssam_platform_hub_acpi_match[] = {
  MODULE_DEVICE_TABLE(acpi, ssam_platform_hub_acpi_match);
static const struct of_device_id ssam_platform_hub_of_match[] __maybe_unused = {
+	/* Surface Pro 9 5G */
+	{ .compatible = "microsoft,arcata", (void *)ssam_node_group_sp9_5G },
  	/* Surface Laptop 7 */
  	{ .compatible = "microsoft,romulus13", (void *)ssam_node_group_sl7 },
  	{ .compatible = "microsoft,romulus15", (void *)ssam_node_group_sl7 },

Thanks!

Reviewed-by: Maximilian Luz <luzmaximilian@xxxxxxxxx>

On a related note: I'm wondering whether we should also already add the
ACPI IDs for the as-of-now ARM-only devices. Technically, things won't
work with ACPI right now, but it's not because of the SAM driver stack.
Or maybe at least add them to the comments as a way of documenting it.

Best regards,
Max




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux