| Message ID | 20240908223505.21011-4-jerome.debretagne@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Microsoft Surface Pro 9 5G support | expand |
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@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> 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
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
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
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
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.
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
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
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.
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 },
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(+)