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

[Jérôme de Bretagne]

Hi,

I'm replying with Hans and Ilpo, who I initially forgot for this
patch, sorry about that.

Le mar. 10 sept. 2024 à 23:29, Maximilian Luz
<luzmaximilian@xxxxxxxxx> a écrit :
>
> Looks good. Two very small nit-picks below, if this goes for a v3:

Atm I'm not planning for a v3 as Bjorn has applied the other v2
patches earlier today.
Feel free to include the 2 small suggestions when applying this patch maybe?

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

Thanks for your review and all the work about SSAM for Surface owners!

Regards,
Jérôme

> 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