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

Improve quirks detection for es8336 #4112

Closed
wants to merge 10 commits into from

Conversation

mchehab
Copy link

@mchehab mchehab commented Dec 21, 2022

Use _DSM table to detect GPIO level and AMIC. This change has @plbossart patches that apply, and replaces one that doesn't actually work.

Tested on Huawei Matebook D15.

@sofci
Copy link
Collaborator

sofci commented Dec 21, 2022

Can one of the admins verify this patch?

reply test this please to run this test once

@plbossart
Copy link
Member

test this please

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.

Thanks for this work @mchehab

One comment below on whether we can simplify further.

} else {
priv->enable_spk_gpio.crs_entry_index = 0;
priv->enable_hp_gpio.crs_entry_index = 1;
}
Copy link
Member

Choose a reason for hiding this comment

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

I am lost on this one @mchehab, do we actually need a quirk at all for the speaker GPIOs? Why not assume that the speakers always has a GPIO and simplify - which was the recommendation from @yangxiaohua2009

Copy link
Author

Choose a reason for hiding this comment

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

The patch is assuming that speakers and headphones will always have GPIOs. The point is that, on Matebook D15, the speaker GPIO is 1, while the headphone GPIO is 0. Other devices could have this reversed - and this was the default before we added proper support for it.

While I would love to, I can't see a way to avoid this quirk - at least for now. Perhaps there are some information somewhere else at the ACPI tables telling what's connected to each GPIO port, but I was unable to identify where.

Choose a reason for hiding this comment

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

Can you tell us the value of GPIO0 when speaker on? If the value of GPIO0 is low when speaker on and you can hear sound from speaker then a quirk is need.

Copy link
Author

@mchehab mchehab Dec 23, 2022

Choose a reason for hiding this comment

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

Can you tell us the value of GPIO0 when speaker on? If the value of GPIO0 is low when speaker on and you can hear sound from speaker then a quirk is need.

Both GPIOs need to be changed the same way, as one is low triggered, while the other is high triggered. So, turning speaker on /headphone off means setting both to true, and turning speaker off/headphone on means setting both to false.

Copy link

@codepayne codepayne Jan 17, 2023

Choose a reason for hiding this comment

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

I have a Huawei AMD laptop with es8336, and I wrote a machine driver for it which I plan to upstream. I cherry picked these commits to add support for this DSM parsing. The GPIO stuff is not working for me. The DSM code reads that GPIO0 is active high and GPIO1 is active low:

[ 47.483034] es8316 i2c-ESSX8336:00: speaker gpio 0 active high, headphone gpio 1 active low

Now when running on speaker everything is ok, but whenever I plug in the headset, no sound can be heard in the headphones or the speaker. If I comment out the second the call to gpiod_set_value_cansleep(priv->gpio_headphone, !priv->speaker_en);

The speaker/headset switching and back works ok.

To me it seems that it is not always safe to use both gpios (at least on AMD platforms).

Maybe there is a way to detect in DSM if this system uses one or both GPIOs? I saw this entry which sounds interesting: CODEC_GPIO0_FUNC_ARG=0x1

DSM dump

[   42.265065] es8316 i2c-ESSX8336:00: PLATFORM_HPMIC_TYPE_ARG=0xcc
[   42.265116] es8316 i2c-ESSX8336:00: PLATFORM_SPK_TYPE_ARG=0x2
[   42.265171] es8316 i2c-ESSX8336:00: PLATFORM_HPDET_INV_ARG=0x0
[   42.265806] es8316 i2c-ESSX8336:00: PLATFORM_MIC_DE_POP_ARG=0x0
[   42.266477] es8316 i2c-ESSX8336:00: PLATFORM_BUS_SLOT_ARG=0x2
[   42.266546] es8316 i2c-ESSX8336:00: HP_CODEC_LINEIN_PGA_GAIN_ARG=0x5
[   42.266618] es8316 i2c-ESSX8336:00: MAIN_CODEC_LINEIN_PGA_GAIN_ARG=0x5
[   42.266703] es8316 i2c-ESSX8336:00: HP_CODEC_D2SEPGA_GAIN_ARG=0x1
[   42.266791] es8316 i2c-ESSX8336:00: MAIN_CODEC_D2SEPGA_GAIN_ARG=0x1
[   42.266884] es8316 i2c-ESSX8336:00: HP_CODEC_ADC_VOLUME_ARG=0x0
[   42.266981] es8316 i2c-ESSX8336:00: MAIN_CODEC_ADC_VOLUME_ARG=0x0
[   42.267082] es8316 i2c-ESSX8336:00: HP_CODEC_ADC_ALC_ENABLE_ARG=0x1
[   42.267187] es8316 i2c-ESSX8336:00: MAIN_CODEC_ADC_ALC_ENABLE_ARG=0x1
[   42.267297] es8316 i2c-ESSX8336:00: HP_CODEC_ADC_ALC_TARGET_LEVEL_ARG=0xa
[   42.267410] es8316 i2c-ESSX8336:00: MAIN_CODEC_ADC_ALC_TARGET_LEVEL_ARG=0xa
[   42.267528] es8316 i2c-ESSX8336:00: HP_CODEC_ADC_ALC_MAXGAIN_ARG=0x12
[   42.267651] es8316 i2c-ESSX8336:00: MAIN_CODEC_ADC_ALC_MAXGAIN_ARG=0x12
[   42.267777] es8316 i2c-ESSX8336:00: HP_CODEC_ADC_ALC_MINGAIN_ARG=0x8
[   42.267908] es8316 i2c-ESSX8336:00: MAIN_CODEC_ADC_ALC_MINGAIN_ARG=0x4
[   42.268042] es8316 i2c-ESSX8336:00: HP_CODEC_ADC_ALC_HLDTIME_ARG=0x0
[   42.268181] es8316 i2c-ESSX8336:00: MAIN_CODEC_ADC_ALC_HLDTIME_ARG=0x0
[   42.268324] es8316 i2c-ESSX8336:00: HP_CODEC_ADC_ALC_DCYTIME_ARG=0x0
[   42.268471] es8316 i2c-ESSX8336:00: MAIN_CODEC_ADC_ALC_DCYTIME_ARG=0x0
[   42.268623] es8316 i2c-ESSX8336:00: HP_CODEC_ADC_ALC_ATKTIME_ARG=0x2
[   42.268779] es8316 i2c-ESSX8336:00: MAIN_CODEC_ADC_ALC_ATKTIME_ARG=0x2
[   42.268938] es8316 i2c-ESSX8336:00: HP_CODEC_ADC_ALC_NGTYPE_ARG=0x3
[   42.269103] es8316 i2c-ESSX8336:00: MAIN_CODEC_ADC_ALC_NGTYPE_ARG=0x3
[   42.269271] es8316 i2c-ESSX8336:00: HP_CODEC_ADC_ALC_NGTHLD_ARG=0x1
[   42.269444] es8316 i2c-ESSX8336:00: MAIN_CODEC_ADC_ALC_NGTHLD_ARG=0x1
[   42.269520] es8316 i2c-ESSX8336:00: MAIN_CODEC_ADC_GUI_STEP_ARG=0x3
[   42.269596] es8316 i2c-ESSX8336:00: MAIN_CODEC_ADC_GUI_GAIN_RANGE_ARG=0x3
[   42.270364] es8316 i2c-ESSX8336:00: HP_CODEC_DAC_HPMIX_HIGAIN_ARG=0x0
[   42.270546] es8316 i2c-ESSX8336:00: SPK_CODEC_DAC_HPMIX_HIGAIN_ARG=0x0
[   42.270732] es8316 i2c-ESSX8336:00: HP_CODEC_DAC_HPMIX_VOLUME_ARG=0xbb
[   42.270922] es8316 i2c-ESSX8336:00: SPK_CODEC_DAC_HPMIX_VOLUME_ARG=0xbb
[   42.271116] es8316 i2c-ESSX8336:00: HP_CODEC_DAC_HPOUT_VOLUME_ARG=0x0
[   42.271314] es8316 i2c-ESSX8336:00: SPK_CODEC_DAC_HPOUT_VOLUME_ARG=0x0
[   42.271517] es8316 i2c-ESSX8336:00: HP_CODEC_LDAC_VOLUME_ARG=0x0
[   42.271723] es8316 i2c-ESSX8336:00: HP_CODEC_RDAC_VOLUME_ARG=0x0
[   42.271934] es8316 i2c-ESSX8336:00: SPK_CODEC_LDAC_VOLUME_ARG=0x0
[   42.272149] es8316 i2c-ESSX8336:00: SPK_CODEC_RDAC_VOLUME_ARG=0x0
[   42.272376] es8316 i2c-ESSX8336:00: HP_CODEC_DAC_AUTOMUTE_ARG=0x0
[   42.272608] es8316 i2c-ESSX8336:00: SPK_CODEC_DAC_AUTOMUTE_ARG=0x1
[   42.272828] es8316 i2c-ESSX8336:00: HP_CODEC_DAC_MONO_ARG=0x0
[   42.273051] es8316 i2c-ESSX8336:00: SPK_CODEC_DAC_MONO_ARG=0x0
[   42.273867] es8316 i2c-ESSX8336:00: SPK_CTL_IO_LEVEL_ARG=0x1
[   42.273992] es8316 i2c-ESSX8336:00: CODEC_GPIO0_FUNC_ARG=0x1
[   42.276349] es8316 i2c-ESSX8336:00: PLATFORM_MCLK_LRCK_FREQ_ARG=0x0
[   42.276393] es8316 i2c-ESSX8336:00: PLATFORM_MAINMIC_TYPE_ARG=0xbb
[   42.276439] es8316 i2c-ESSX8336:00: PLATFORM_HPMIC_TYPE_ARG=0xcc
[   42.276490] es8316 i2c-ESSX8336:00: PLATFORM_SPK_TYPE_ARG=0x2
[   42.276545] es8316 i2c-ESSX8336:00: PLATFORM_HPDET_INV_ARG=0x0
[   42.277185] es8316 i2c-ESSX8336:00: PLATFORM_MIC_DE_POP_ARG=0x0
[   42.277829] es8316 i2c-ESSX8336:00: PLATFORM_BUS_SLOT_ARG=0x2
[   42.277897] es8316 i2c-ESSX8336:00: HP_CODEC_LINEIN_PGA_GAIN_ARG=0x5
[   42.278001] es8316 i2c-ESSX8336:00: MAIN_CODEC_LINEIN_PGA_GAIN_ARG=0x5
[   42.278092] es8316 i2c-ESSX8336:00: HP_CODEC_D2SEPGA_GAIN_ARG=0x1
[   42.278181] es8316 i2c-ESSX8336:00: MAIN_CODEC_D2SEPGA_GAIN_ARG=0x1
[   42.278278] es8316 i2c-ESSX8336:00: HP_CODEC_ADC_VOLUME_ARG=0x0
[   42.278375] es8316 i2c-ESSX8336:00: MAIN_CODEC_ADC_VOLUME_ARG=0x0
[   42.278492] es8316 i2c-ESSX8336:00: HP_CODEC_ADC_ALC_ENABLE_ARG=0x1
[   42.278631] es8316 i2c-ESSX8336:00: MAIN_CODEC_ADC_ALC_ENABLE_ARG=0x1
[   42.278780] es8316 i2c-ESSX8336:00: HP_CODEC_ADC_ALC_TARGET_LEVEL_ARG=0xa
[   42.278933] es8316 i2c-ESSX8336:00: MAIN_CODEC_ADC_ALC_TARGET_LEVEL_ARG=0xa
[   42.279054] es8316 i2c-ESSX8336:00: HP_CODEC_ADC_ALC_MAXGAIN_ARG=0x12
[   42.279177] es8316 i2c-ESSX8336:00: MAIN_CODEC_ADC_ALC_MAXGAIN_ARG=0x12
[   42.279304] es8316 i2c-ESSX8336:00: HP_CODEC_ADC_ALC_MINGAIN_ARG=0x8
[   42.279435] es8316 i2c-ESSX8336:00: MAIN_CODEC_ADC_ALC_MINGAIN_ARG=0x4
[   42.279570] es8316 i2c-ESSX8336:00: HP_CODEC_ADC_ALC_HLDTIME_ARG=0x0
[   42.279709] es8316 i2c-ESSX8336:00: MAIN_CODEC_ADC_ALC_HLDTIME_ARG=0x0
[   42.279853] es8316 i2c-ESSX8336:00: HP_CODEC_ADC_ALC_DCYTIME_ARG=0x0
[   42.280001] es8316 i2c-ESSX8336:00: MAIN_CODEC_ADC_ALC_DCYTIME_ARG=0x0
[   42.280153] es8316 i2c-ESSX8336:00: HP_CODEC_ADC_ALC_ATKTIME_ARG=0x2
[   42.280309] es8316 i2c-ESSX8336:00: MAIN_CODEC_ADC_ALC_ATKTIME_ARG=0x2
[   42.280469] es8316 i2c-ESSX8336:00: HP_CODEC_ADC_ALC_NGTYPE_ARG=0x3
[   42.280633] es8316 i2c-ESSX8336:00: MAIN_CODEC_ADC_ALC_NGTYPE_ARG=0x3
[   42.280828] es8316 i2c-ESSX8336:00: HP_CODEC_ADC_ALC_NGTHLD_ARG=0x1
[   42.281014] es8316 i2c-ESSX8336:00: MAIN_CODEC_ADC_ALC_NGTHLD_ARG=0x1
[   42.281091] es8316 i2c-ESSX8336:00: MAIN_CODEC_ADC_GUI_STEP_ARG=0x3
[   42.281167] es8316 i2c-ESSX8336:00: MAIN_CODEC_ADC_GUI_GAIN_RANGE_ARG=0x3
[   42.281925] es8316 i2c-ESSX8336:00: HP_CODEC_DAC_HPMIX_HIGAIN_ARG=0x0
[   42.282148] es8316 i2c-ESSX8336:00: SPK_CODEC_DAC_HPMIX_HIGAIN_ARG=0x0
[   42.282335] es8316 i2c-ESSX8336:00: HP_CODEC_DAC_HPMIX_VOLUME_ARG=0xbb
[   42.282526] es8316 i2c-ESSX8336:00: SPK_CODEC_DAC_HPMIX_VOLUME_ARG=0xbb
[   42.282721] es8316 i2c-ESSX8336:00: HP_CODEC_DAC_HPOUT_VOLUME_ARG=0x0
[   42.282921] es8316 i2c-ESSX8336:00: SPK_CODEC_DAC_HPOUT_VOLUME_ARG=0x0
[   42.283124] es8316 i2c-ESSX8336:00: HP_CODEC_LDAC_VOLUME_ARG=0x0
[   42.283331] es8316 i2c-ESSX8336:00: HP_CODEC_RDAC_VOLUME_ARG=0x0
[   42.283543] es8316 i2c-ESSX8336:00: SPK_CODEC_LDAC_VOLUME_ARG=0x0
[   42.283757] es8316 i2c-ESSX8336:00: SPK_CODEC_RDAC_VOLUME_ARG=0x0
[   42.283985] es8316 i2c-ESSX8336:00: HP_CODEC_DAC_AUTOMUTE_ARG=0x0
[   42.284216] es8316 i2c-ESSX8336:00: SPK_CODEC_DAC_AUTOMUTE_ARG=0x1
[   42.284435] es8316 i2c-ESSX8336:00: HP_CODEC_DAC_MONO_ARG=0x0
[   42.284658] es8316 i2c-ESSX8336:00: SPK_CODEC_DAC_MONO_ARG=0x0
[   42.285477] es8316 i2c-ESSX8336:00: SPK_CTL_IO_LEVEL_ARG=0x1
[   42.285606] es8316 i2c-ESSX8336:00: CODEC_GPIO0_FUNC_ARG=0x1
[   42.288015] es8316 i2c-ESSX8336:00: PLATFORM_MCLK_LRCK_FREQ_ARG=0x0

Choose a reason for hiding this comment

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

The headphone gpio dsm returns 0xff (default value), which should be active high instead of active low. This is a known issue which needs bo be fixed.

Copy link
Member

Choose a reason for hiding this comment

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

@yangxiaohua2009 can you clarify what needs to be fixed?
a) The BIOS? that's unlikely
b) the way the DSM function handles the headphone GPIO level?
c) something else?

Copy link
Author

Choose a reason for hiding this comment

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

The headphone gpio dsm returns 0xff (default value), which should be active high instead of active low. This is a known issue which needs bo be fixed.

Such logic works fine for Matebook D15, so it seems that we'll end needing a quirks anyway for such notebook. Btw, I quickly tested on a Matebook D14. Despite ACPI looking identical, the logic there seems to be inverted, but I need to do more tests.

In summary, it seems that we'll keep needing a quirk to override DSM. Unfortunately, I lost access to both notebooks, and I don't know when I'll be able to access them again. I'm hoping to get my hands again on Matebook D14 in Feb, and work on another version of this patch series.

Choose a reason for hiding this comment

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

@plbossart we should have a fuction like this:

switch (value) {
	case GPIO_CTL_IO_LEVEL_HIGH:
       	case 0xff:
		return 0;
	case GPIO_CTL_IO_LEVEL_LOW:
		return 1;
	default:
		dev_err(dev, "%s: invalid I/O ctl level argument (%d)\n",
			type, value);
		return -EINVAL;
	}
}

instead of

switch (value) {
	case GPIO_CTL_IO_LEVEL_HIGH:
		return 0;
	case GPIO_CTL_IO_LEVEL_LOW:
	case 0xff:
		return 1;
	default:
		dev_err(dev, "%s: invalid I/O ctl level argument (%d)\n",
			type, value);
		return -EINVAL;
	}
}

Actually BIOS can be changed by Windows Updates but this may not work for Linux.

@@ -134,15 +105,21 @@ static int sof_8336_trigger(struct snd_pcm_substream *substream, int cmd)
case SNDRV_PCM_TRIGGER_START:
case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
case SNDRV_PCM_TRIGGER_RESUME:
if (substream->stream == 0) {

Choose a reason for hiding this comment

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

You don't need delayed work here

Copy link
Author

Choose a reason for hiding this comment

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

Currently, there is a problem with the driver: if the machine boots with the headphone connected, it is currently sending the output to the speaker, as the workqueue job is never triggered. The same could also happen if one plugs or unplugs the headphone while the machine is suspended.

Now, I'm not sure if this is the best way to add such logic, or if it should, instead, call gpiod_set_value during probe time and have suspend/resume functions (either there or at es8316) to handle such cases.

Choose a reason for hiding this comment

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

I think there is an issue here which I saw happening in my case.
When I play a video in the browser, lets say I pause the video, after 5-6 seconds the trigger callback is called, with the PAUSE_PUSH/SUSPEND/STOP state. This causes both speaker and headphone GPIOs to be deactivated.

If during the next 5-6 seconds I unpause the video, the trigger callback function is called again with the START/PAUSE_RELEASE/RESUME state which doesn't enable the GPIOs. Also sof_es8316_speaker_power_event() will not get called so I have no sound. If I wait more than 10-12 seconds after pausing the video to unpause it then I also get a call to sof_es8316_speaker_power_event() which enables the speaker GPIO and I will hear the sound.

It seems to me that the GPIOs should be also handled on the START/PAUSE_RELEASE/RESUME path.

Choose a reason for hiding this comment

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

The problem is that sof_es8316_speaker_power_event is not called. This happens time to time and I don't know why.
The gpios cannot be handled in the START/PAUSE trigger function because you want to turn off the speaker when playing music and a headset plugged in. In this case the trigger fuction is not called and we rely on the sof_es8316_speaker_power_event callback function.

plbossart and others added 10 commits December 24, 2022 10:00
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.

Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>
Co-developed-by: David Yang <yangxiaohua@everest-semi.com>
Signed-off-by: David Yang <yangxiaohua@everest-semi.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Read jack detection information from _DSM and pre-set value. The value
may be overridden with a property set by the machine driver.

Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Read jack detection information from _DSM and pre-set value. The value
may be overridden with a property set by the machine driver.

Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The ACPI entry for es83xx should contain its configuration at
_DSM. It is helpful to debug problems by looking on its contents.

So, add a debug function that outputs its contents as read by the
Kernel.

Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
The enable GPIOs can either be low or high level activated.

Add a logic to detect it from _DSM.

Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
The _DSM method has an entry to detect the microphone type,
and where they're attached. Add support for checking it.

Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
Now that we can read GPIO enable level from ACPI _DSM, use it
to properly set the right GPIO levels.

Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
This is already detected from _DSM, so no need to have a quirk
inside the driver anymore.

Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
Use the _DSM data to fill the analog microphone ports,
removing another quirk.

Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
As the headphone could be plugged or unplugged any time, including
when not streaming, and the stop/suspend events may be turning GPIOs
to disable both headphones and speakers, let the delayed workqueue
to be called every time Speaker supply event happens.

Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
case GPIO_CTL_IO_LEVEL_HIGH:
return 0;
case GPIO_CTL_IO_LEVEL_LOW:
case 0xff:

Choose a reason for hiding this comment

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

Please return 0 for 0xff.

@jwrdegoede
Copy link

I have taken a quick look at this, mostly because of the usage of sound/soc/codecs/es8316.c on BYT/CHT x86 devices.

I notice that this adds this:

	/* read jack information from _DSM */
	ret = es83xx_dsm_jack_inverted(dev);
	if (ret < 0)
		dev_warn(dev, "%s: Could not get jack detection information with ACPI _DSM method\n",
			 __func__);
	else
		es8316->jd_inverted = ret;

to sound/soc/codecs/es8316.c, but es8316->jd_inverted will be overridden by:

static void es8316_enable_jack_detect(struct snd_soc_component *component,
				      struct snd_soc_jack *jack)
{
	struct es8316_priv *es8316 = snd_soc_component_get_drvdata(component);

	/*
	 * Init es8316->jd_inverted here and not in the probe, as we cannot
	 * guarantee that the bytchr-es8316 driver, which might set this
	 * property, will probe before us.
	 */
	es8316->jd_inverted = device_property_read_bool(component->dev,
							"everest,jack-detect-inverted");

Which gets called by both the sound/soc/intel/boards/sof_es8336.c and the sound/soc/intel/boards/bytcht_es8316.c board files in the form of calling snd_soc_component_set_jack().

So this won't work.

Since the _DSM is x86 and thus board specific it might be best to just make the board files set that boolean based on the _DSM results. Note that sound/soc/intel/boards/bytcht_es8316.c already sets this property currently based on DMI quirks, see the snd_byt_cht_es8316_mc_probe() function.

And I think we should also consider having the board files call es83xx_dsm_dump(dev); rather then having the codec driver itself do that.

@plbossart
Copy link
Member

Which gets called by both the sound/soc/intel/boards/sof_es8336.c and the sound/soc/intel/boards/bytcht_es8316.c board files in the form of calling snd_soc_component_set_jack().

So this won't work.

Why not @jwrdegoede ? The "everest,jack-detect-inverted" is not set by sof_es8336.c, not sure what the problem is?

The logic I wanted was to use the _DSM first, and if there is an additional property set in the machine driver override the default set by _DSM.

@yangxiaohua2009
Copy link

@plbossart because device_property_read_bool will return false even if "everest,jack-detect-inverted" properity is not set.

The logic now should be

if (device_property_read_bool("everest,jack-detect-inverted") == true)
        es8316->jd_inverted = true;
else
       do nothing

@plbossart
Copy link
Member

@plbossart because device_property_read_bool will return false even if "everest,jack-detect-inverted" properity is not set.

The logic now should be

if (device_property_read_bool("everest,jack-detect-inverted") == true)
        es8316->jd_inverted = true;
else
       do nothing

Right, but that's a minor change really. That doesn't change the fact that IF that property is defined it'll override the _DSM stuff.

@jwrdegoede
Copy link

Why not @jwrdegoede ? The "everest,jack-detect-inverted" is not set by sof_es8336.c, not sure what the problem is?

The problem is that es8316_enable_jack_detect() runs after probe() has checked the _DSM so it will override the result from the _DSM set in probe(). So at a minimum we need the change suggested by @yangxiaohua2009 .

Also as I mentioned already the _DSM is x86 specific so really all of the handling of it should be in the sound/soc/intel/boards/*.c files; and this pull-req already puts 99% there, so IMHO it would be best to just set the property based on the _DSM from the board file to and not at any knowledge about the DSM to the platform-agnostic codec driver at all.

@plbossart
Copy link
Member

@jwrdegoede I can understand your point but _DSM is ACPI specific, not x86 specific. We already have support for ACPI or OF in codec drivers, that's fine.
In addition, I would expect AMD platforms to use the same information - though I can't verify this fact, maybe @yangxiaohua2009 can comment on this. The _DSM stuff is really Device-Specific-Method so it really belongs in the scope of the codec driver IMHO.

@yangxiaohua2009
Copy link

ACPI is also used for MIPS arch so the _DSM read from codec driver can be used for many machine drivers (AMD KBL MIPS etc.) Otherwise there would be similar code in all the machine drivers which makes life harder.

@overengineer
Copy link

Hello, I have a Matebook D15. How can I help on this? I can do tests if you instruct how to do.

@yangxiaohua2009
Copy link

@overengineer
add remote url git remote add mcb https://github.com/mchehab/linux.git
git remote update
checkout branch git checkout mcb/sound-es8336
Now your can use your config to compile.

@plbossart
Copy link
Member

@mchehab @yangxiaohua2009 I'll close this PR since we have no line-of-sight for any update or merge timeline. I have done enough on my side to try and help with this ES8336 support, I don't have time to help further.

@plbossart plbossart closed this Apr 17, 2023
jwrdegoede pushed a commit to jwrdegoede/linux-sunxi that referenced this pull request Dec 2, 2023
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.

Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>
Co-developed-by: David Yang <yangxiaohua@everest-semi.com>
Signed-off-by: David Yang <yangxiaohua@everest-semi.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Move es83xx-dsm-common.c back to sound/soc/codecs like the orignal
  version from: thesofproject#4112
jwrdegoede pushed a commit to jwrdegoede/linux-sunxi that referenced this pull request Dec 2, 2023
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.

Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>
Co-developed-by: David Yang <yangxiaohua@everest-semi.com>
Signed-off-by: David Yang <yangxiaohua@everest-semi.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Move es83xx-dsm-common.c back to sound/soc/codecs like the orignal
  version from: thesofproject#4112
jwrdegoede pushed a commit to jwrdegoede/linux-sunxi that referenced this pull request Dec 2, 2023
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.

Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>
Co-developed-by: David Yang <yangxiaohua@everest-semi.com>
Signed-off-by: David Yang <yangxiaohua@everest-semi.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Move es83xx-dsm-common.c back to sound/soc/codecs like the orignal
  version from: thesofproject#4112
jwrdegoede pushed a commit to jwrdegoede/linux-sunxi that referenced this pull request Dec 3, 2023
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.

Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>
Co-developed-by: David Yang <yangxiaohua@everest-semi.com>
Signed-off-by: David Yang <yangxiaohua@everest-semi.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Move es83xx-dsm-common.c back to sound/soc/codecs like the orignal
  version from: thesofproject#4112
jwrdegoede pushed a commit to jwrdegoede/linux-sunxi that referenced this pull request Dec 4, 2023
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.

Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>
Co-developed-by: David Yang <yangxiaohua@everest-semi.com>
Signed-off-by: David Yang <yangxiaohua@everest-semi.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Move es83xx-dsm-common.c back to sound/soc/codecs like the orignal
  version from: thesofproject#4112
jwrdegoede pushed a commit to jwrdegoede/linux-sunxi that referenced this pull request Dec 4, 2023
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.

Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Co-developed-by: David Yang <yangxiaohua@everest-semi.com>
Signed-off-by: David Yang <yangxiaohua@everest-semi.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Move es83xx-dsm-common.c back to sound/soc/codecs like the orignal
  version from: thesofproject#4112
intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this pull request Dec 5, 2023
Merge series from Hans de Goede <hdegoede@redhat.com>:

This takes some of the work done to auto-configure quirks/routing
for ESS83xx codecs by getting the info from ACPI from:
thesofproject#4112

And then builds on top of this to add auto-configuration to
the bytcht_es8316 board driver.

Note compared to the pull-request, which deals with the ES8336, this
series deals with the ES8316 (for which I have several devices to test
on) and this moves handling of the _DSM from the codec driver to
the board driver since with the ES8316 the board driver takes
care of setting up various routes for things like the mic and
speakers.

After this series audio now works properly on a CHT Chuwi Hi12
tablet without needing to add an extra quirk for that model.

This has also been tested on the following devices, where things
are unchanged from before (the ACPI autoconfiguration gives the
same results as the old defaults) :

Onda V80 plus (CHT)
GP-electronic T701 (BYT)

I also tested this on a Nanote UMPC-01, here the _DSM result
for PLATFORM_SPK_TYPE_ARG wrongly returns 1 (mono) while
the device actually has 2 speakers, so this model needs to keep
its DMI quirk.

I don't have an IRBIS NB41 nor a TECLAST X98 Plus II,
so the DMI quirks for those are left in place too on
a better safe then sorry basis.
@mchehab
Copy link
Author

mchehab commented Dec 29, 2023

@mchehab @yangxiaohua2009 I'll close this PR since we have no line-of-sight for any update or merge timeline. I have done enough on my side to try and help with this ES8336 support, I don't have time to help further.

The notebook I'm using to test is now with my son. I can only work on it when he is in vacations. I'm right now updating the changeset with the proposed changes, plus a couple of new quirks for it to work on Huawei D15. I'll issue a new PR with the code, on the top of sof-dev topic at: #4756

jwrdegoede pushed a commit to jwrdegoede/linux-sunxi that referenced this pull request Jan 4, 2024
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.

Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Co-developed-by: David Yang <yangxiaohua@everest-semi.com>
Signed-off-by: David Yang <yangxiaohua@everest-semi.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Move es83xx-dsm-common.c back to sound/soc/codecs like the orignal
  version from: thesofproject#4112
jwrdegoede pushed a commit to jwrdegoede/linux-sunxi that referenced this pull request Jan 15, 2024
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.

Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Co-developed-by: David Yang <yangxiaohua@everest-semi.com>
Signed-off-by: David Yang <yangxiaohua@everest-semi.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Move es83xx-dsm-common.c back to sound/soc/codecs like the orignal
  version from: thesofproject#4112
jwrdegoede pushed a commit to jwrdegoede/linux-sunxi that referenced this pull request Jan 15, 2024
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.

Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Co-developed-by: David Yang <yangxiaohua@everest-semi.com>
Signed-off-by: David Yang <yangxiaohua@everest-semi.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Move es83xx-dsm-common.c back to sound/soc/codecs like the orignal
  version from: thesofproject#4112
jwrdegoede pushed a commit to jwrdegoede/linux-sunxi that referenced this pull request Jan 22, 2024
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.

Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Co-developed-by: David Yang <yangxiaohua@everest-semi.com>
Signed-off-by: David Yang <yangxiaohua@everest-semi.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Move es83xx-dsm-common.c back to sound/soc/codecs like the orignal
  version from: thesofproject#4112
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.

7 participants