| Message ID | 20240814-topic-sam-v3-0-a84588aad233@quicinc.com |
|---|---|
| Headers | show |
| Series | OF support for Surface System Aggregator Module | expand |
On 8/14/24 12:27 PM, Konrad Dybcio wrote: > From: Konrad Dybcio <quic_kdybcio@quicinc.com> > > Add basic support for registering the aggregator module on Device Tree- > based platforms. These include at least three generations of Qualcomm > Snapdragon-based Surface devices: > > - SC8180X / SQ1 / SQ2: Pro X, > - SC8280XP / SQ3: Devkit 2023, Pro 9 > - X Elite: Laptop 7 / Pro11 > > Thankfully, the aggregators on these seem to be configured in an > identical way, which allows for using these settings as defaults and > no DT properties need to be introduced (until that changes, anyway). > > Based on the work done by Maximilian Luz, largely rewritten. > > Signed-off-by: Konrad Dybcio <quic_kdybcio@quicinc.com> > --- Looks good to me and works without issues on my Surface Pro X. Thanks again for picking this up! Reviewed-by: Maximilian Luz <luzmaximilian@gmail.com> Tested-by: Maximilian Luz <luzmaximilian@gmail.com>
Hi, On 8/14/24 12:27 PM, Konrad Dybcio wrote: > Wire up OF support for SSAM drivers, to use with Surface Laptop 7 and > other Qualcomm-based devices. > > Patch 3 references compatible strings introduced in [1] > > [1] https://lore.kernel.org/linux-arm-msm/20240809-topic-sl7-v1-1-2090433d8dfc@quicinc.com/T/#u > > Signed-off-by: Konrad Dybcio <quic_kdybcio@quicinc.com> Thank you for your patch-series, I've applied the series to my review-hans branch: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans I did notice the following compiler warning when test building: drivers/platform/surface/surface_aggregator_registry.c:278:36: warning: ‘ssam_node_group_sl7’ defined but not used [-Wunused-variable] 278 | static const struct software_node *ssam_node_group_sl7[] = { | ^~~~~~~~~~~~~~~~~~~ One way to fix this would be add #ifdef CONFIG_OF around the definition of ssam_node_group_sl7, but then future devicetree based surface devices would need more #ifdef-s so instead I've solved it by squashing in this fix: diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/drivers/platform/surface/surface_aggregator_registry.c index 495cb4300617..ac96e883cb57 100644 --- a/drivers/platform/surface/surface_aggregator_registry.c +++ b/drivers/platform/surface/surface_aggregator_registry.c @@ -415,14 +415,12 @@ static const struct acpi_device_id ssam_platform_hub_acpi_match[] = { }; MODULE_DEVICE_TABLE(acpi, ssam_platform_hub_acpi_match); -#ifdef CONFIG_OF -static const struct of_device_id ssam_platform_hub_of_match[] = { +static const struct of_device_id ssam_platform_hub_of_match[] __maybe_unused = { /* Surface Laptop 7 */ { .compatible = "microsoft,romulus13", (void *)ssam_node_group_sl7 }, { .compatible = "microsoft,romulus15", (void *)ssam_node_group_sl7 }, { }, }; -#endif static int ssam_platform_hub_probe(struct platform_device *pdev) { Once I've run some tests on this branch the patches there will be added to the platform-drivers-x86/for-next branch and eventually will be included in the pdx86 pull-request to Linus for the next merge-window. Regards, Hans > --- > Changes in v3: > - Drop unnecessary nullchecks > - Add MODULE_ALIAS in the platform hub driver > - Fix MODULE_DEVICE_TABLE after rename > - Prolong the '----' comment to 80 lines > - Change the current-speed bindings description to ": true", in > preparation for krzk's serial device bindings reorganization > - Link to v2: https://lore.kernel.org/r/20240810-topic-sam-v2-0-8a8eb368a4f0@quicinc.com > > Changes in v2: > - Fix kerneldoc > - Drop the drivers/acpi change (oops) > - Style fixes > - Don't assign int to acpi_status > - Don't scan the bus twice in SAM core probe > - Link to v1: https://lore.kernel.org/r/20240809-topic-sam-v1-0-05bca1932614@quicinc.com > > --- > Konrad Dybcio (3): > dt-bindings: serial: Allow embedded-controller as child node > dt-bindings: platform: Add Surface System Aggregator Module > platform/surface: Add OF support > > .../bindings/platform/microsoft,surface-sam.yaml | 47 +++++++++++++ > .../devicetree/bindings/serial/serial.yaml | 2 +- > drivers/platform/surface/aggregator/bus.c | 2 + > drivers/platform/surface/aggregator/controller.c | 67 ++++++++++++++---- > drivers/platform/surface/aggregator/core.c | 82 +++++++++++++++++----- > drivers/platform/surface/surface3_power.c | 1 + > drivers/platform/surface/surface_acpi_notify.c | 1 + > .../platform/surface/surface_aggregator_registry.c | 47 +++++++++++-- > 8 files changed, 210 insertions(+), 39 deletions(-) > --- > base-commit: 1e391b34f6aa043c7afa40a2103163a0ef06d179 > change-id: 20240809-topic-sam-5de2f0ec9370 > > Best regards,
On 19.08.2024 1:57 PM, Hans de Goede wrote: > Hi, > > On 8/14/24 12:27 PM, Konrad Dybcio wrote: >> Wire up OF support for SSAM drivers, to use with Surface Laptop 7 and >> other Qualcomm-based devices. >> >> Patch 3 references compatible strings introduced in [1] >> >> [1] https://lore.kernel.org/linux-arm-msm/20240809-topic-sl7-v1-1-2090433d8dfc@quicinc.com/T/#u >> >> Signed-off-by: Konrad Dybcio <quic_kdybcio@quicinc.com> > > Thank you for your patch-series, I've applied the series to my > review-hans branch: > https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans > > I did notice the following compiler warning when test building: > > drivers/platform/surface/surface_aggregator_registry.c:278:36: warning: ‘ssam_node_group_sl7’ defined but not used [-Wunused-variable] > 278 | static const struct software_node *ssam_node_group_sl7[] = { > | ^~~~~~~~~~~~~~~~~~~ > > One way to fix this would be add #ifdef CONFIG_OF around the definition > of ssam_node_group_sl7, but then future devicetree based surface devices > would need more #ifdef-s so instead I've solved it by squashing in this fix: > > diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/drivers/platform/surface/surface_aggregator_registry.c > index 495cb4300617..ac96e883cb57 100644 > --- a/drivers/platform/surface/surface_aggregator_registry.c > +++ b/drivers/platform/surface/surface_aggregator_registry.c > @@ -415,14 +415,12 @@ static const struct acpi_device_id ssam_platform_hub_acpi_match[] = { > }; > MODULE_DEVICE_TABLE(acpi, ssam_platform_hub_acpi_match); > > -#ifdef CONFIG_OF > -static const struct of_device_id ssam_platform_hub_of_match[] = { > +static const struct of_device_id ssam_platform_hub_of_match[] __maybe_unused = { > /* Surface Laptop 7 */ > { .compatible = "microsoft,romulus13", (void *)ssam_node_group_sl7 }, > { .compatible = "microsoft,romulus15", (void *)ssam_node_group_sl7 }, > { }, > }; > -#endif > > static int ssam_platform_hub_probe(struct platform_device *pdev) > { > > Once I've run some tests on this branch the patches there will be > added to the platform-drivers-x86/for-next branch and eventually > will be included in the pdx86 pull-request to Linus for the next > merge-window. Thanks for pointing this out. Your fix seems to be the best solution I can think of, so I'm all for it Konrad
On 8/19/24 1:57 PM, Hans de Goede wrote: > Hi, > > On 8/14/24 12:27 PM, Konrad Dybcio wrote: >> Wire up OF support for SSAM drivers, to use with Surface Laptop 7 and >> other Qualcomm-based devices. >> >> Patch 3 references compatible strings introduced in [1] >> >> [1] https://lore.kernel.org/linux-arm-msm/20240809-topic-sl7-v1-1-2090433d8dfc@quicinc.com/T/#u >> >> Signed-off-by: Konrad Dybcio <quic_kdybcio@quicinc.com> > > Thank you for your patch-series, I've applied the series to my > review-hans branch: > https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans > > I did notice the following compiler warning when test building: > > drivers/platform/surface/surface_aggregator_registry.c:278:36: warning: ‘ssam_node_group_sl7’ defined but not used [-Wunused-variable] > 278 | static const struct software_node *ssam_node_group_sl7[] = { > | ^~~~~~~~~~~~~~~~~~~ > > One way to fix this would be add #ifdef CONFIG_OF around the definition > of ssam_node_group_sl7, but then future devicetree based surface devices > would need more #ifdef-s so instead I've solved it by squashing in this fix: > > diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/drivers/platform/surface/surface_aggregator_registry.c > index 495cb4300617..ac96e883cb57 100644 > --- a/drivers/platform/surface/surface_aggregator_registry.c > +++ b/drivers/platform/surface/surface_aggregator_registry.c > @@ -415,14 +415,12 @@ static const struct acpi_device_id ssam_platform_hub_acpi_match[] = { > }; > MODULE_DEVICE_TABLE(acpi, ssam_platform_hub_acpi_match); > > -#ifdef CONFIG_OF > -static const struct of_device_id ssam_platform_hub_of_match[] = { > +static const struct of_device_id ssam_platform_hub_of_match[] __maybe_unused = { > /* Surface Laptop 7 */ > { .compatible = "microsoft,romulus13", (void *)ssam_node_group_sl7 }, > { .compatible = "microsoft,romulus15", (void *)ssam_node_group_sl7 }, > { }, > }; > -#endif > > static int ssam_platform_hub_probe(struct platform_device *pdev) > { > > Once I've run some tests on this branch the patches there will be > added to the platform-drivers-x86/for-next branch and eventually > will be included in the pdx86 pull-request to Linus for the next > merge-window. I agree with Konrad, this looks like the best way to address this. Thanks! Best regards, Max
Wed, Aug 14, 2024 at 12:27:24PM +0200, Konrad Dybcio kirjoitti: > Wire up OF support for SSAM drivers, to use with Surface Laptop 7 and > other Qualcomm-based devices. > > Patch 3 references compatible strings introduced in [1] > > [1] https://lore.kernel.org/linux-arm-msm/20240809-topic-sl7-v1-1-2090433d8dfc@quicinc.com/T/#u Please, Cc to me the v4.
Wire up OF support for SSAM drivers, to use with Surface Laptop 7 and other Qualcomm-based devices. Patch 3 references compatible strings introduced in [1] [1] https://lore.kernel.org/linux-arm-msm/20240809-topic-sl7-v1-1-2090433d8dfc@quicinc.com/T/#u Signed-off-by: Konrad Dybcio <quic_kdybcio@quicinc.com> --- Changes in v3: - Drop unnecessary nullchecks - Add MODULE_ALIAS in the platform hub driver - Fix MODULE_DEVICE_TABLE after rename - Prolong the '----' comment to 80 lines - Change the current-speed bindings description to ": true", in preparation for krzk's serial device bindings reorganization - Link to v2: https://lore.kernel.org/r/20240810-topic-sam-v2-0-8a8eb368a4f0@quicinc.com Changes in v2: - Fix kerneldoc - Drop the drivers/acpi change (oops) - Style fixes - Don't assign int to acpi_status - Don't scan the bus twice in SAM core probe - Link to v1: https://lore.kernel.org/r/20240809-topic-sam-v1-0-05bca1932614@quicinc.com --- Konrad Dybcio (3): dt-bindings: serial: Allow embedded-controller as child node dt-bindings: platform: Add Surface System Aggregator Module platform/surface: Add OF support .../bindings/platform/microsoft,surface-sam.yaml | 47 +++++++++++++ .../devicetree/bindings/serial/serial.yaml | 2 +- drivers/platform/surface/aggregator/bus.c | 2 + drivers/platform/surface/aggregator/controller.c | 67 ++++++++++++++---- drivers/platform/surface/aggregator/core.c | 82 +++++++++++++++++----- drivers/platform/surface/surface3_power.c | 1 + drivers/platform/surface/surface_acpi_notify.c | 1 + .../platform/surface/surface_aggregator_registry.c | 47 +++++++++++-- 8 files changed, 210 insertions(+), 39 deletions(-) --- base-commit: 1e391b34f6aa043c7afa40a2103163a0ef06d179 change-id: 20240809-topic-sam-5de2f0ec9370 Best regards,