kmod: sof_remove: add snd_soc_sof_da7219 #1107
Conversation
There was a problem hiding this comment.
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.
|
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 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Please add a very short comment explaining which name changed.
There was a problem hiding this comment.
to the commit message or to the script?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Actually, the commit title is still "kmod: sof_remove: add snd_soc_sof_da7219" even when it's not removed anymore!
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.
There was a problem hiding this comment.
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 <brent.lu@intel.com>
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 <brent.lu@intel.com>
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
|
SOFCI TEST |
brentlu
force-pushed
the
update_kmod_name
branch
from
a9bed6e
to
d4cf6d3
Compare
brentlu
changed the title
brentlu
force-pushed
the
update_kmod_name
branch
from
d4cf6d3
to
9aeccab
Compare
There was a problem hiding this comment.
let's merge to unlock thesofproject/linux#4897 and fix incrementally if needed.
| @@ -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 | |||
There was a problem hiding this comment.
Actually, the commit title is still "kmod: sof_remove: add snd_soc_sof_da7219" even when it's not removed anymore!
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 <brent.lu@intel.com>
An new helper module has been created for nuvoton speaker amplifier. Add the new module to the script. Signed-off-by: Brent Lu <brent.lu@intel.com>
An new helper module has been created for cirrus logic speaker amplifier. Add the new module to the script. Signed-off-by: Brent Lu <brent.lu@intel.com>
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 <brent.lu@intel.com>
I'm deeply sorry: I read too fast, that title was correct. |
|
SOFCI TEST |
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.