mbox series

[v3,0/3] OF support for Surface System Aggregator Module

Message ID 20240814-topic-sam-v3-0-a84588aad233@quicinc.com
Headers show
Series OF support for Surface System Aggregator Module | expand

Message

Konrad Dybcio Aug. 14, 2024, 10:27 a.m. UTC
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,

Comments

Hans de Goede Aug. 19, 2024, 11:57 a.m. UTC | #1
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,
Konrad Dybcio Aug. 19, 2024, 1:27 p.m. UTC | #2
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
Maximilian Luz Aug. 19, 2024, 8:07 p.m. UTC | #3
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
Andy Shevchenko Aug. 26, 2024, 8:55 p.m. UTC | #4
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.