Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mpsl: fem: increase initialization priority #10120

Conversation

jciupis
Copy link

@jciupis jciupis commented Feb 1, 2023

Front-End Module initialization is a dependency of HCI driver initialization. Increase its priority so that there's a clear hierarchy between them.

Signed-off-by: Jędrzej Ciupis jedrzej.ciupis@nordicsemi.no

Front-End Module initialization is a dependency of HCI driver
initialization. Increase its priority so that there's a clear hierarchy
between them.

Signed-off-by: Jędrzej Ciupis <jedrzej.ciupis@nordicsemi.no>
@jciupis jciupis requested review from too1 and ryanjh February 1, 2023 09:28
@github-actions github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Feb 1, 2023
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Feb 1, 2023

Test specification

CI/Jenkins/NRF

  • Integration Platforms

CI/Jenkins/integration

Test Module File based changes Manually selected West overwrite
test-fw-nrfconnect-ble X
test-fw-nrfconnect-ble_samples X
test-fw-nrfconnect-chip X
test-fw-nrfconnect-fem X
test-fw-nrfconnect-rs X
test-fw-nrfconnect-thread X
test-fw-nrfconnect-zigbee X
test-sdk-find-my X
test-sdk-homekit X

Detailed information of selected test modules

Note: This message is automatically posted and updated by the CI

@jciupis jciupis requested a review from ahasztag February 1, 2023 11:19
@jciupis jciupis added this to the 2.3.0 milestone Feb 1, 2023
Copy link
Contributor

@ahedin25 ahedin25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth mentioning in the commit message that this fixes an issue with the output power?

@@ -293,7 +293,7 @@ static int mpsl_fem_init(const struct device *dev)
return fem_nrf21540_gpio_configure();
}

SYS_INIT(mpsl_fem_init, POST_KERNEL, CONFIG_KERNEL_INIT_PRIORITY_DEVICE);
SYS_INIT(mpsl_fem_init, POST_KERNEL, CONFIG_KERNEL_INIT_PRIORITY_DEFAULT);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid if this is done this way at some point we will forget, why the change was introduced. Is there any way to make this initialization priority relative to SDC initialization priority?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One way that comes to my mind is that there could be a dedicated Kconfig option representing MPSL FEM initialization priority and another one representing HCI driver initialization priority. We could then give them correct default values and put a build assert to enforce the dependency. It feels to be a bit of an overkill though. What do you think? Do you have other ideas?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be exactly the approach I think I would choose.

Jędrzej Ciupis added 2 commits February 2, 2023 14:45
It's easier to track dependencies between initialization priorities if
they have their own Kconfig entries.

Signed-off-by: Jędrzej Ciupis <jedrzej.ciupis@nordicsemi.no>
The new Kconfig option is used for a build assert to enforce an explicit
dependency on initialization priority of MPSL FEM.

Signed-off-by: Jędrzej Ciupis <jedrzej.ciupis@nordicsemi.no>
@rugeGerritsen
Copy link
Contributor

I would really like you to update the commit message to describe what problem you are trying to solve

@NordicBuilder
Copy link
Contributor

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publishing GitHub Action.

@rugeGerritsen
Copy link
Contributor

Maybe #10143 does the same thing?

@jciupis
Copy link
Author

jciupis commented Feb 3, 2023

Superseded by #10143

@jciupis jciupis closed this Feb 3, 2023
@rugeGerritsen rugeGerritsen added the doc-required PR must not be merged without tech writer approval. label Feb 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. doc-required PR must not be merged without tech writer approval.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants