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

test-socwatch.sh: no need to load socwatch module manually #1157

Merged
merged 1 commit into from
Apr 10, 2024

Conversation

keqiaozhang
Copy link
Contributor

We only use socwatch tool to read the power data for power measurement in this case, we can directly use the latest released version, so there is no need to pre-compile the driver.

@keqiaozhang keqiaozhang requested a review from a team as a code owner January 31, 2024 04:50
Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

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

Please don't delete all the corresponding code. We are barely getting started in this area which means things will change in the future and we may need this again soon.

In fact it should be very easy to make the module loading and unload optional with just this:

- sudo "$SOCWATCH_PATH"/drivers/insmod-socwatch -q
+ sudo "$SOCWATCH_PATH"/drivers/insmod-socwatch -q || true
- check_socwatch_module_loaded || die "socwatch module not loaded"
+ check_socwatch_module_loaded || dlogi "socwatch module not loaded"

marc-hb
marc-hb previously approved these changes Feb 1, 2024
Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

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

I don't find the commit message very accurate but... good enough.

We do need to load the driver manually; this PR just makes that optional.

}

socwatch_test_once()
{
local i="$1"
dlogi "===== Loop($i/$loop_count) ====="
dlogi "SoCWatch version: ${SOCWATCH_VERSION}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you could have just run the command here but no big deal.

We only use socwatch tool to read the power data for power measurement
in this case, we can directly use the latest released version, so there
is no need to pre-compile the driver.

Signed-off-by: Keqiao Zhang <[email protected]>
@keqiaozhang
Copy link
Contributor Author

@marc-hb marc-hb reopened this Mar 15, 2024
@marc-hb
Copy link
Collaborator

marc-hb commented Mar 15, 2024

@fredoh9 can we merge this?

Copy link
Collaborator

@fredoh9 fredoh9 left a comment

Choose a reason for hiding this comment

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

Looks good.

@fredoh9 fredoh9 merged commit bef7519 into thesofproject:main Apr 10, 2024
6 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants