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

[Hans de Goede <hdegoede@redhat.com>]

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