[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <f9cbd1c3-eb05-4262-bdc6-6d37e83179e5@gmail.com>
Date: Tue, 10 Sep 2024 23:29:48 +0200
From: Maximilian Luz <luzmaximilian@...il.com>
To: Jérôme de Bretagne <jerome.debretagne@...il.com>,
Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio <konradybcio@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>
Cc: Johan Hovold <johan+linaro@...nel.org>, linux-arm-msm@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/5] platform/surface: aggregator_registry: Add Surface
Pro 9 5G
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@...il.com>
> ---
> .../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@...il.com>
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
Powered by blists - more mailing lists