-
Notifications
You must be signed in to change notification settings - Fork 140
Sound support for the Samsung Galaxy Book 4 #5616
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
base: topic/sof-dev
Are you sure you want to change the base?
Conversation
|
Can one of the admins verify this patch?
|
|
I've tested this on my own machine (Galaxy Book4 Pro 360 NP960QGK) under Arch Linux and can confirm sound works as expected. The quality is not exactly the same but pretty comparable to Windows. Nice work @BreadJS! |
|
This also fixes the following kernel bugzillas: |
|
Thank you @stephanlensky |
|
Should an entry also be added to |
|
@melvyn2 Good call. I added it. ( I have no idea if there is a difference between the 14 and 16 inch variants and such. $ lspci -vvv | grep "Multimedia audio"
00:1f.3 Multimedia audio controller: Intel Corporation Meteor Lake-P HD Audio Controller (rev 20)
$ cat /sys/bus/pci/devices/0000:00:1f.3/subsystem_device
0xc892 <- this is what we need |
|
|
sound/hda/codecs/realtek/alc269.c
Outdated
| {.id = ALC298_FIXUP_SAMSUNG_AMP, .name = "alc298-samsung-amp"}, | ||
| {.id = ALC298_FIXUP_SAMSUNG_AMP_V2_2_AMPS, .name = "alc298-samsung-amp-v2-2-amps"}, | ||
| {.id = ALC298_FIXUP_SAMSUNG_AMP_V2_4_AMPS, .name = "alc298-samsung-amp-v2-4-amps"}, | ||
| {.id = ALC298_FIXUP_SAMSUNG_AMP_V2_4_AMPS, .name = "alc298-samsung-amp-v2-4-amps"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this line was accidentally duplicated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed! Thank you for noticing!
Wait really? This worked on the Samsung Galaxy Book 5 Pro 360? |
Here's my dev info for Book4 Ultra: The current patch does not work on this model. (Maybe it wasn't even supposed to?) |
Here's my dev info for Galaxy Book4 Pro (16-inch) NP960XGK: |
It most likely did not work because the PCI Quirk was not added. It is now, Please try again and see if it works now. If it still does not work, please connect me at the official discord server: https://discord.gg/wR8mM9pvVa |
|
@BreadJS Excellent news that this was solved! As none of the changes fall under SOF (linux/sound/soc/sof/), patches submitted here cannot me merged here. Can you submit this series to Linux upstream ? [email protected] and see https://kernel.org/doc/html/latest/process/submitting-patches.html Please do refer to existing bugs like https://bugzilla.kernel.org/show_bug.cgi?id=220808 , this will likely help a lot of affected users. |
|
@kv2019i Aah okay! I thought that this patch was for SOF. Thank you for letting me know! I will be trying to do that! Thank you very much! |
|
Hi, I’m currently running Linux Mint (Ubuntu-based) and would be very happy to test this once a patched kernel is available for Ubuntu/Mint users. Thanks a lot for the amazing work on this! |
|
@BreadJS can you update your Stripe account in bountyhub for the bounty reward? I tried to reach you through email, and I'm not sure if you got my email. |
I do not have a stripe account, is this required? Can I not use paypal? |
it's an Express connected account that can be created from bountyhub's dashboard, not a Stripe account for businesses. Fees are lower than PayPal. |
It should be resolved now### |
|
Confirmed working after c4a897b on Book4 Ultra |
|
Dope, thanks a lot for this! |
arter97
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just performed some minor code reviews. I'm no authority in kernel maintaining in any subsystems but hope this helps.
Let me know if you need any help in upstreaming this.
While having SOF review this is nice, we ultimately need to do the email method for upstreaming.
|
|
||
| switch (action) { | ||
| case HDA_GEN_PCM_ACT_OPEN: | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove whitespace line.
| strscpy(comp->name, dev_name(dev), sizeof(comp->name)); | ||
| comp->playback_hook = max98390_hda_playback_hook; | ||
|
|
||
| dev_info(dev, "MAX98390 HDA component bound (index %d)\n", priv->index); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace pretty much all dev_info()s to dev_dbg(). These are too verbose for upstream.
| return ret; | ||
|
|
||
| ret = component_add(dev, &max98390_hda_comp_ops); | ||
| if (ret) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove curly-brackets for single-lined branch.
| @@ -0,0 +1,334 @@ | |||
| // High-pass filter configuration for MAX98390 HDA driver | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing SPDK identifier.
| @@ -0,0 +1,214 @@ | |||
| // SPDX-License-Identifier: GPL-2.0 | |||
| // | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Use /* */?
| // Configure DSM with appropriate firmware based on speaker type | ||
| // cutoff_freq: cutoff frequency in Hz (unused - filter built into tweeter firmware) | ||
| // is_tweeter: true for tweeters (0x3C, 0x3D), false for woofers (0x38, 0x39) | ||
| void max98390_configure_high_pass_filter(struct max98390_hda_priv *priv, int cutoff_freq, bool is_tweeter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not used outside. Mark it static and remove it from the header file.
| } | ||
| } | ||
|
|
||
| // For WOOFERS: Apply EasyEffects EQ using BQ1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh? Why is EasyEffects mentioned here? Are we applying kernel-level equalizer configurations? Or is this something that stock Windows drivers perform?
| void max98390_configure_filters(struct max98390_hda_priv *priv); | ||
| void max98390_configure_high_pass_filter(struct max98390_hda_priv *priv, int cutoff_freq, bool is_tweeter); | ||
|
|
||
| #endif /* __MAX98390_HDA_FILTERS_H */ No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All files must have an empty line at the end.
Why this PR?
I bought my Samsung Galaxy Book 4 Pro almost a year ago and wanted to run Linux. But was unsatisfied with the fact that there were no sound drivers available. After opening up the laptop I discovered there was a MAX98390 chip in the system connected through the ACPI. But after a long time there was no success on getting this working, until now!
What does this PR do?
The file
sound/soc/codecs/max98390.cwas missing a match for theMAX98390. It only was sayingMX98390and notMAX98390. Not only that, but lots of registers were initialized and also includes the DSM coefficients for basic but good enough sound.More information
Sorry if this is not the right way to include this in the linux kernel. This is my first time making a PR to such big repo. Let me know if I need to change things and will try my best. Others are able to do this for me too if really needed. I will most likely add a second PR later, to add DSM coefficients from windows. This is being extracted by someone I'm working with.