ASoC: es83xx: extract codec configuration with _DSM #4049
Conversation
|
On Linux we have UCM, tplg, pcm_hw_params and dailinks so most definitions are Windows specific. |
It's still useful to log the information, e.g. what we get from NHLT is a hack.
Yes, the idea what to get default values for quirks, but the machine driver should not try to read DSM from another device. What I am specifically interested in is which GPIOS need to be selected. This is a gray area where we need DMI quirks and if we can have information on which GPIO is used for what that would solve most of the problems. |
plbossart
force-pushed
the
fix/es8336-dsm
branch
from
847e27c
to
a7d9b45
Compare
|
@yangxiaohua2009 can you try this branch on your platforms, I can not sure why we see an undocumented value for PCM_TYPE [ 4.430963] es8316 i2c-ESSX8336:00: PLATFORM_PCM_TYPE 0xff |
|
@jwrdegoede This PR might be of interest to you as well for the bytcht-8316.c machine driver, I am not sure however if the _DSM works on those older platforms. |
|
When the windows driver see the PCM_TYPE=0xFF, it set the format to the default value (DSPA 24bit on Windows) |
|
When The Windows driver sees two GPIOs in the dsm, it take the first GPIO as Speaker GPIO amd the second as Headphone GPIO. |
@plbossart thank you, this is definitely interesting. I will try to give this series a try on one or more BYT/CHT devices with an es8316 codec. It might be quite a while before I get around to this though. |
ok, will document this. |
@yangxiaohua2009 what do you mean by 'see two GPIOS'? we have 5 parameters, and the values are not defined |
plbossart
force-pushed
the
fix/es8336-dsm
branch
from
a7d9b45
to
ef7cce3
Compare
|
For the GPIO please review the pr #4066 |
|
Most of the ES83xx codec configuration is exposed in the DSDT table and accessible via a _DSM method. Start adding basic definitions and helpers to dump the information. Co-developed-by: David Yang <[email protected]> Signed-off-by: David Yang <[email protected]> Signed-off-by: Pierre-Louis Bossart <[email protected]>
Read jack detection information from _DSM and pre-set value. The value may be overridden with a property set by the machine driver. Signed-off-by: Pierre-Louis Bossart <[email protected]>
plbossart
force-pushed
the
fix/es8336-dsm
branch
from
ef7cce3
to
6b6c678
Compare
|
@yangxiaohua2009 you are going too fast with your changes, I would recommend we first use the two GPIOs assuming the level is active HIGH, and in a second step we invert the logic. It's too confusing otherwise. @mchehab could you give this PR a try, this removes one of the two quirks for your Huawei device. I think the logic works but as usual I could be wrong. |
Read jack detection information from _DSM and pre-set value. The value may be overridden with a property set by the machine driver. Signed-off-by: Pierre-Louis Bossart <[email protected]>
There are always two GPIOS used, the first one controls the speaker and the second one the headset. This first simplification assumes the GPIOs are active high, in a future enhancement the level will be adjusted base on _DSM information. Signed-off-by: Pierre-Louis Bossart <[email protected]>
plbossart
force-pushed
the
fix/es8336-dsm
branch
from
6b6c678
to
3285a0b
Compare
|
The headphone GPIO is always on now. May be fixed later. |
I don't understand the comment @yangxiaohua2009. Keep in mind I have no background and no test devices... All this work should have been handled by the codec vendor really. |
|
The headphone GPIO can be always on, because the user can always unplug it. And they won't hear sound from headphone when it unplugged, no matter the headphone GPIO on or not. It's not the case with speaker. I may submit patches based on this PR
|
|
The GPIO status look like this now right? Future patch may turn off the HP GPIO when stream off, and a headphone_power_event may be needed to turn on the Headphone GPIO next time when stream on and Headphone plugged in.
|
|
@yangxiaohua2009 Can I ask why we care about the GPIO state in suspend? Does this prevent the codec from suspending or does it have a side effect? Usually the GPIO is only set when activating/deactivating a steam, I am not sure what the suspend case is and what should be done on resume. |
| @@ -143,6 +120,7 @@ static int sof_8336_trigger(struct snd_pcm_substream *substream, int cmd) | |||
| if (substream->stream == 0) { | |||
| cancel_delayed_work(&priv->pcm_pop_work); | |||
| gpiod_set_value_cansleep(priv->gpio_speakers, true); | |||
| gpiod_set_value_cansleep(priv->gpio_headphone, false); | |||
| } | |||
There was a problem hiding this comment.
Turning off the headphone GPIO reduces pop/click noise and hum noise. For suspend I mean deactivating a steam. And from here we can see the headphone GPIO is on even if the stream deactivated.
Sorry for taking so long to test it. With Kernel 6.0.12, without modprobe parameters, this is what it gets::
Some logs from Kernel 6.0.12: When applying fix/es8336-dsm (merged with Kernel 6.1), the switch controls both headphones and speakers at the same time, so it is a regression on detecting the right GPIOs. Those are the logs after this series: |
You're basically assuming:
However, from some experiments on my Huawei D15, what it has instead is:
So, basically:
|
| { "headphone-enable-gpios", &enable_gpio0, 1, ACPI_GPIO_QUIRK_ONLY_GPIOIO }, | ||
| { } | ||
| }; | ||
|
|
There was a problem hiding this comment.
No. For Huawei Matebook D15, the right GPIO table is acpi_enable_both_gpios_rev_order.
| static void log_quirks(struct device *dev) | ||
| { | ||
| dev_info(dev, "quirk mask %#lx\n", quirk); | ||
| dev_info(dev, "quirk SSP%ld\n", SOF_ES8336_SSP_CODEC(quirk)); | ||
| if (quirk & SOF_ES8336_ENABLE_DMIC) | ||
| dev_info(dev, "quirk DMIC enabled\n"); | ||
| if (quirk & SOF_ES8336_SPEAKERS_EN_GPIO1_QUIRK) | ||
| dev_info(dev, "Speakers GPIO1 quirk enabled\n"); |
There was a problem hiding this comment.
A quirk to specify if Speakers is at GPIO 0 or 1 is needed.
| @@ -119,8 +97,7 @@ static void pcm_pop_work_events(struct work_struct *work) | |||
|
|
|||
| gpiod_set_value_cansleep(priv->gpio_speakers, priv->speaker_en); | |||
|
|
|||
| if (quirk & SOF_ES8336_HEADPHONE_GPIO) | |||
| gpiod_set_value_cansleep(priv->gpio_headphone, priv->speaker_en); | |||
| gpiod_set_value_cansleep(priv->gpio_headphone, !priv->speaker_en); | |||
There was a problem hiding this comment.
No (at least on Huawei Matebook D15): there, the headphone GPIO is reversed: true disables, false enables.
So, the right code is, actually:
gpiod_set_value_cansleep(priv->gpio_headphone, priv->speaker_en);
as before.
| { | ||
| .callback = sof_es8336_quirk_cb, | ||
| .matches = { | ||
| DMI_MATCH(DMI_SYS_VENDOR, "HUAWEI"), | ||
| DMI_MATCH(DMI_BOARD_NAME, "BOHB-WAX9-PCB-B2"), | ||
| }, | ||
| .driver_data = (void *)(SOF_ES8336_HEADPHONE_GPIO | | ||
| SOC_ES8336_HEADSET_MIC1) |
There was a problem hiding this comment.
Due to the above, we still need at least one quirk:
- a quirk to select if GPIO 0 is speaker or microphone;
- (maybe) a quirk to indicate if true means enabled or disabled for headphone (and maybe another for speaker if needed)
There was a problem hiding this comment.
Btw, this is the patch I had to apply on the top of yours for headphones to work again:
diff --git a/sound/soc/intel/boards/sof_es8336.c b/sound/soc/intel/boards/sof_es8336.c
index f13e9aa6ae48..8db28bea1ebe 100644
--- a/sound/soc/intel/boards/sof_es8336.c
+++ b/sound/soc/intel/boards/sof_es8336.c
@@ -73,8 +73,8 @@ static const struct acpi_gpio_params enable_gpio0 = { 0, 0, true };
static const struct acpi_gpio_params enable_gpio1 = { 1, 0, true };
static const struct acpi_gpio_mapping acpi_enable_both_gpios[] = {
- { "speakers-enable-gpios", &enable_gpio0, 1, ACPI_GPIO_QUIRK_ONLY_GPIOIO },
- { "headphone-enable-gpios", &enable_gpio1, 1, ACPI_GPIO_QUIRK_ONLY_GPIOIO },
+ { "speakers-enable-gpios", &enable_gpio1, 1, ACPI_GPIO_QUIRK_ONLY_GPIOIO },
+ { "headphone-enable-gpios", &enable_gpio0, 1, ACPI_GPIO_QUIRK_ONLY_GPIOIO },
{ }
};
@@ -96,8 +96,8 @@ static void pcm_pop_work_events(struct work_struct *work)
container_of(work, struct sof_es8336_private, pcm_pop_work.work);
gpiod_set_value_cansleep(priv->gpio_speakers, priv->speaker_en);
-
- gpiod_set_value_cansleep(priv->gpio_headphone, !priv->speaker_en);
+ /* Headphone logic is inverted: 1 disables it */
+ gpiod_set_value_cansleep(priv->gpio_headphone, priv->speaker_en);
}
|
Thanks @mchehab for testing, much appreciated. The headphone mic detection is not part of this branch just yet, I only worked on the jack detection logic. For the GPIO settings, @yangxiaohua2009 made a point that it's not the GPIO that is inverted but rather the logic level at which the GPIO is active, and we can get that from the _DSM stuff. |
Headphone mic is detected correctly.
I added a debug logic to print the entire _DSM. Those are the results: And the patch itself: |
|
Looking the most relevant _DSM, we have: So:
Yet, I didn't find a _DSM entry there saying what it is connected to GPIO 0 and GPIO 1. |
|
@plbossart , @yangxiaohua2009 I submitted a PR that replaces this one, at: On this device, GPIO 1 is for Speakers, while GPIO 0 is for Microphone. |
|
@mchehab please make PR based on topic/sof-dev. Also, you don't need a quirk for speaker GPIO1. Speaker GPIO at GPIO1 is the same as speaker/headphone GPIO active low (inverting the high/low level of both GPIOs). |
Ok, I'll rebase it.
No, it is not the same, as the suspend state should be disabling both speaker and headphones, so basically, one of the GPIOs should be placed in high level and the other one in low level on suspend. Also, a quirk is still needed, as the _DSM data on this device is: (the 0xff code there is returned simply because the _DSM function returns nothing for 0x4b argument) Now, I don't have the datasheets for em83xx, but based on the definitions at es83xx-dsm-common.h: So, the information there doesn't seem to be enough to detect that the GPIOs are inverted. Perhaps the GPIO info it is stored somewhere else at _DSM, for an argument currently not documented? FYI, this is the ES8336 entry from ACPI DSDT table: es8336_acpi.txt |
|
@mchehab in the past @yangxiaohua2009 mentioned that 0xff means 'default', it's not an error reading invalid bits. If that's correct, we need to know the default for each parameter. @yangxiaohua2009 can you please help and comment on this? |
|
replaced by PR #4112 |
the _DSM method from the ACPI clear is: So, yeah, 0xff is the default value, but it can also mean an invalid value for arg2 (see https://github.com/thesofproject/linux/files/10277361/es8336_acpi.txt). |
|
For the gpio, please return 0 (active high) for value 0xff. I tested PR #4112 on my devices and it works fine. The speaker GPIO is turned off when music stopped. 0xff means default value, and it should be the value most device use. |
Support for ES83xx is a nightmare, with 10 open issues.
Rather than continue the guesswork for every end-user, we need to extract the information required to configure the codec with the _DSM method.
With this draft PR, I can see meaningful values such as
and highly suspicious ones:
@yangxiaohua2009 can you please review the definitions and specifically the FIXME that don't seem to be documented in the ASL.