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

kmod: sof_remove: add snd_soc_sof_da7219 #1107

Merged
merged 4 commits into from
Apr 11, 2024

Conversation

brentlu
Copy link
Contributor

@brentlu brentlu commented Sep 25, 2023

Machine driver sof_da7219_max98373 has been renamed and there are two helper modules for cirrus logic and nuvoton amplifiers respectively. Update the script to reflect the linux kernel change.

@brentlu brentlu requested review from plbossart and a team as code owners September 25, 2023 01:30
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.

Machine driver sof_da7219_max98373 has been renamed

Important: we want to keep sof-test backwards-compatible for as long as it comes for free. Please keep both the old and new name with a comment that tells which Linux commit changed the name.

Less important but useful: Please also split the rename and the addition into 2 separate commits that can easily be bisected/reverted independently. This script is trickier than it looks and we had to revert things in the past.

Even less important but still useful: please also try to stuff more specific keywords in the commit subject (for git log --oneline), I mean specific keywords like da7129 and nuvoton. You can drop "update" because every commit is an update. If needed you can drop "modules" and "kmod", and "drivers" because sof_remove is enough.

@brentlu
Copy link
Contributor Author

brentlu commented Sep 25, 2023

Thanks for the review. The change has been merged to broonie repo but still waiting for linux merge window. Let me update this PR afterwards.

@@ -144,7 +144,7 @@ remove_module snd_soc_acpi_intel_match
# Machine drivers
#-------------------------------------------
remove_module snd_soc_sof_rt5682
remove_module snd_soc_sof_da7219_max98373
remove_module snd_soc_sof_da7219
Copy link
Member

Choose a reason for hiding this comment

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

you want to keep the old name for backwards compatibility.
Nothing bad will happen if you remove a nonexistent device. Bad things may happen if you keep a module loaded.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a very short comment explaining which name changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to the commit message or to the script?

Copy link
Member

Choose a reason for hiding this comment

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

commit message is fine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

When looking at confusing code, few people think about looking at past commit messages (which are sometimes hard to find because of future whitespace changes or other formatting). Even fewer people in a repo with test scripts where commit messages tend to be lower quality.

Hopefully no one will ever have to make sense of these two lines.

Copy link
Member

Choose a reason for hiding this comment

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

it's fine really, there's no need to make the script too detailed to track all the code moves in various parts of the kernel. The updated code supports old and new names, that's already very good.

Copy link
Collaborator

@marc-hb marc-hb Apr 10, 2024

Choose a reason for hiding this comment

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

Actually, the commit title is still "kmod: sof_remove: add snd_soc_sof_da7219" even when it's not removed anymore!

f93eb56

I don't mind that much if the comments are not in my preferred place. I do mind if the commit title says the opposite of what it does; please fix at least that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to me that there is something wrong with Github's GUI.

brentlu@brentlu-desktop:/workspace/sof/my_sof_test_0$ git log -p f93eb56
commit f93eb561a03c58d5055e58b8cd51bec983085a3e
Author: Brent Lu <[email protected]>
Date:   Wed Apr 10 17:26:00 2024 +0800

    kmod: sof_remove: add snd_soc_sof_da7219

    Machine driver sof_da7219_max98373 has been renamed as
    snd_soc_sof_da7219. Add the new module to the script and keep the old
    one for backward compatibility.

    Signed-off-by: Brent Lu <[email protected]>

diff --git a/tools/kmod/sof_remove.sh b/tools/kmod/sof_remove.sh
index 4907101..b0f4fb6 100755
--- a/tools/kmod/sof_remove.sh
+++ b/tools/kmod/sof_remove.sh
@@ -152,6 +152,7 @@ remove_module imx_common
 #-------------------------------------------
 remove_module snd_soc_sof_rt5682
 remove_module snd_soc_sof_da7219_max98373
+remove_module snd_soc_sof_da7219
 remove_module snd_soc_sst_bdw_rt5677_mach
 remove_module snd_soc_bdw_rt286
 remove_module snd_soc_sst_broadwell

@marc-hb
Copy link
Collaborator

marc-hb commented Oct 6, 2023

SOFCI TEST

@brentlu brentlu changed the title kmod: sof_remove: update machine driver modules kmod: sof_remove: add snd_soc_sof_da7219 Apr 10, 2024
@marc-hb marc-hb dismissed their stale review April 10, 2024 15:28

stale review

Copy link
Member

@plbossart plbossart left a comment

Choose a reason for hiding this comment

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

let's merge to unlock thesofproject/linux#4897 and fix incrementally if needed.

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.

Old commit title says the opposite of what it does, see below.

@@ -144,7 +144,7 @@ remove_module snd_soc_acpi_intel_match
# Machine drivers
#-------------------------------------------
remove_module snd_soc_sof_rt5682
remove_module snd_soc_sof_da7219_max98373
remove_module snd_soc_sof_da7219
Copy link
Collaborator

@marc-hb marc-hb Apr 10, 2024

Choose a reason for hiding this comment

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

Actually, the commit title is still "kmod: sof_remove: add snd_soc_sof_da7219" even when it's not removed anymore!

f93eb56

I don't mind that much if the comments are not in my preferred place. I do mind if the commit title says the opposite of what it does; please fix at least that.

Machine driver sof_da7219_max98373 has been renamed as
snd_soc_sof_da7219. Add the new module to the script and keep the old
one for backward compatibility.

Signed-off-by: Brent Lu <[email protected]>
An new helper module has been created for nuvoton speaker amplifier.
Add the new module to the script.

Signed-off-by: Brent Lu <[email protected]>
An new helper module has been created for cirrus logic speaker
amplifier. Add the new module to the script.

Signed-off-by: Brent Lu <[email protected]>
Kernel module snd_soc_acpi_intel_match is required by
snd_soc_intel_sof_board_helpers to detect codec type and need to be
removed before snd_soc_acpi module.

RMMOD	snd_soc_acpi_intel_match
rmmod: ERROR: Module snd_soc_acpi_intel_match is in use by: snd_soc_intel_sof_board_helpers

snd_soc_acpi_intel_match   110592  1 snd_soc_intel_sof_board_helpers
snd_soc_acpi           16384  1 snd_soc_acpi_intel_match

Signed-off-by: Brent Lu <[email protected]>
@marc-hb
Copy link
Collaborator

marc-hb commented Apr 11, 2024

Actually, the commit title is still "kmod: sof_remove: add snd_soc_sof_da7219" even when it's not removed anymore!

I'm deeply sorry: I read too fast, that title was correct.

@marc-hb
Copy link
Collaborator

marc-hb commented Apr 11, 2024

SOFCI TEST

@marc-hb marc-hb merged commit c8f7ebe into thesofproject:main Apr 11, 2024
4 of 6 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