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

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

[Jérôme de Bretagne]

Hi,

I've not sent this specific patch to the correct recipients according
to get_maintainer.pl, here is the forward to include you, Hans and
Ilpo. Sorry for the mistake.

Regards,
Jérôme

---------- Forwarded message ---------
De : Jérôme de Bretagne <jerome.debretagne@gmail.com>
Date: lun. 9 sept. 2024 à 00:40
Subject: [PATCH v2 3/5] platform/surface: aggregator_registry: Add
Surface Pro 9 5G
To: Bjorn Andersson <andersson@kernel.org>, Konrad Dybcio
<konradybcio@kernel.org>, Rob Herring <robh@kernel.org>, Krzysztof
Kozlowski <krzk+dt@kernel.org>, Conor Dooley <conor+dt@kernel.org>
Cc: Johan Hovold <johan+linaro@kernel.org>,
<linux-arm-msm@vger.kernel.org>, <devicetree@vger.kernel.org>,
<linux-kernel@vger.kernel.org>, Maximilian Luz
<luzmaximilian@gmail.com>


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@gmail.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. */
+static const struct software_node *ssam_node_group_sp9_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 },
--
2.45.2

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@gmail.com> 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@gmail.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@gmail.com>

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

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

[Hans de Goede]

Hi Jérôme,

On 7-Oct-24 8:44 PM, Jérôme de Bretagne wrote:
> Hi,
> 
> I'm replying with Hans and Ilpo, who I initially forgot for this
> patch, sorry about that.

No worries thank you for forwarding Maximilian's review.

> Le mar. 10 sept. 2024 à 23:29, Maximilian Luz
> <luzmaximilian@gmail.com> 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@gmail.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@gmail.com>
> 
> Thanks for your review and all the work about SSAM for Surface owners!

FWIW I agree with Maximilian's remarks and I would really like to
see these applied to clearly differentiate the x86 and ARM versions.

Normally I would pick up a patch like this which just adds hw-ids as
a fix for 6.12-rc# and squash in the suggested changes.

But looking at the test of the series this is more 6.13 material
since the rest is landing in 6.13, right ?

Patches for linux-next / 6.13 are managed by Ilpo this cycle.

So I'll leave it up to Ilpo if he will squash in the suggested changes
or if he wants a new version (of just this patch, no need for a v3
of the already applied patches).

Regards,

Hans

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

[Jérôme de Bretagne]

Le lun. 7 oct. 2024 à 20:54, Hans de Goede <hdegoede@redhat.com> a écrit :
>
> Hi Jérôme,
>
> On 7-Oct-24 8:44 PM, Jérôme de Bretagne wrote:
> > Hi,
> >
> > I'm replying with Hans and Ilpo, who I initially forgot for this
> > patch, sorry about that.
>
> No worries thank you for forwarding Maximilian's review.
>
> > Le mar. 10 sept. 2024 à 23:29, Maximilian Luz
> > <luzmaximilian@gmail.com> 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@gmail.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@gmail.com>
> >
> > Thanks for your review and all the work about SSAM for Surface owners!
>
> FWIW I agree with Maximilian's remarks and I would really like to
> see these applied to clearly differentiate the x86 and ARM versions.

I stuck to the official names but they can be confusing as-is.

> Normally I would pick up a patch like this which just adds hw-ids as
> a fix for 6.12-rc# and squash in the suggested changes.
>
> But looking at the test of the series this is more 6.13 material
> since the rest is landing in 6.13, right ?

The rest is in arm64-for-6.13 and drivers-for-6.13 in Bjorn's tree indeed.

> Patches for linux-next / 6.13 are managed by Ilpo this cycle.
>
> So I'll leave it up to Ilpo if he will squash in the suggested changes
> or if he wants a new version (of just this patch, no need for a v3
> of the already applied patches).
>
> Regards,
>
> Hans

Sure, let's see what Ilpo prefers.

Regards,
Jérôme

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

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 4160 bytes --]

On Mon, 7 Oct 2024, Hans de Goede wrote:

> Hi Jérôme,
> 
> On 7-Oct-24 8:44 PM, Jérôme de Bretagne wrote:
> > Hi,
> > 
> > I'm replying with Hans and Ilpo, who I initially forgot for this
> > patch, sorry about that.
> 
> No worries thank you for forwarding Maximilian's review.
> 
> > Le mar. 10 sept. 2024 à 23:29, Maximilian Luz
> > <luzmaximilian@gmail.com> 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@gmail.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@gmail.com>
> > 
> > Thanks for your review and all the work about SSAM for Surface owners!
> 
> FWIW I agree with Maximilian's remarks and I would really like to
> see these applied to clearly differentiate the x86 and ARM versions.
> 
> Normally I would pick up a patch like this which just adds hw-ids as
> a fix for 6.12-rc# and squash in the suggested changes.
> 
> But looking at the test of the series this is more 6.13 material
> since the rest is landing in 6.13, right ?
> 
> Patches for linux-next / 6.13 are managed by Ilpo this cycle.
> 
> So I'll leave it up to Ilpo if he will squash in the suggested changes
> or if he wants a new version (of just this patch, no need for a v3
> of the already applied patches).

Hi all,

I've now applied patch 3 to review-ilpo branch in pdx86 repo.

I'd appreciate if somebody confirms I got those comment edits right.

-- 
 i.

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

[Maximilian Luz]

On 10/22/24 10:56 AM, Ilpo Järvinen wrote:

[...]

> Hi all,
> 
> I've now applied patch 3 to review-ilpo branch in pdx86 repo.
> 
> I'd appreciate if somebody confirms I got those comment edits right.
> 

Hi Ilpo,

looks good to me. Thanks for fixing this up!

Best regards,
Max

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

[Jérôme de Bretagne]

On 10/23/24 7:25 PM, Maximilian Luz wrote:
>
> On 10/22/24 10:56 AM, Ilpo Järvinen wrote:
>
> [...]
>
> > Hi all,
> >
> > I've now applied patch 3 to review-ilpo branch in pdx86 repo.
> >
> > I'd appreciate if somebody confirms I got those comment edits right.
> >
>
> Hi Ilpo,
>
> looks good to me. Thanks for fixing this up!
>
> Best regards,
> Max

Hi Ilpo, hi Max,

It looks good to me too.

Thank you both,
Jérôme

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

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 397 bytes --]

On Wed, 23 Oct 2024, Maximilian Luz wrote:

> On 10/22/24 10:56 AM, Ilpo Järvinen wrote:
> 
> [...]
> 
> > Hi all,
> > 
> > I've now applied patch 3 to review-ilpo branch in pdx86 repo.
> > 
> > I'd appreciate if somebody confirms I got those comment edits right.
> > 
> 
> Hi Ilpo,
> 
> looks good to me. Thanks for fixing this up!

Great! Thanks for confirmation.

-- 
 i.