linux-kernel - Re: [PATCH v2 3/5] platform/surface: aggregator

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CA+kEDGHfXa27HJ1KEcRfss9-FeQ-cCqrcQ8pYeWjWZf_N8YaHw@mail.gmail.com>
Date: Mon, 7 Oct 2024 21:59:49 +0200
From: Jérôme de Bretagne <jerome.debretagne@...il.com>
To: Hans de Goede <hdegoede@...hat.com>
Cc: Maximilian Luz <luzmaximilian@...il.com>, 
	Ilpo Järvinen <ilpo.jarvinen@...ux.intel.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>, 
	Johan Hovold <johan+linaro@...nel.org>, linux-arm-msm@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH v2 3/5] platform/surface: aggregator_registry: Add Surface
 Pro 9 5G

Le lun. 7 oct. 2024 à 20:54, Hans de Goede <hdegoede@...hat.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@...il.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@...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>
> >
> > 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