Fix sof_8336 gpio problems by yangxiaohua2009 · Pull Request #4066 · thesofproject/linux

yangxiaohua2009 · 2022-12-02T07:18:09Z

Currently the driver uses quirks to define gpios, which means when the machine has two gpios, the user won't get sound from headphone by default. The patch fix the following problems:

Use two gpios by default
remove redundant quirks
Invert gpio status so that true means on and false means off.

Signed-off-by: Zhu Ning zhuning@everest-semi.com

Currently the driver uses quirks to define gpios, which means when the machine has two gpios, the user won't get sound from headphone by default. The patch fix the following problems: 1. Use two gpios by default 2. remove redundant quirks 3. Invert gpio status so that true means on and false means off. Signed-off-by: Zhu Ning <zhuning@everest-semi.com>

yangxiaohua2009 · 2022-12-02T07:24:11Z

Basically using two GPIOs by default is fine and the system won't crash. See [gpio.zip].(https://github.com/thesofproject/linux/files/10138979/gpio.zip)
Also, the first GPIO is always used for Speaker, and the second used for Headphone so some quirk can be removed.

yangxiaohua2009 · 2022-12-02T07:27:26Z

Problems remains: how should the Headphone GPIO be controled? Should another power event, i.e., headphone_power_event be used? @plbossart

plbossart

very very difficult to review this, this PR removes things that were added to deal with GPIO inversions and this also changes the GPIO level?

plbossart · 2022-12-02T14:22:47Z

sound/soc/intel/boards/sof_es8336.c

-	{ "headphone-enable-gpios", &enable_gpio0, 1, ACPI_GPIO_QUIRK_ONLY_GPIOIO },
-	{ }
-};
-


why is this removed? this was added for a reason...

When two GPIOs are used, Speaker GPIO is always the first and HP GPIO the second. The user may need a quirk to invert the GPIO level.

plbossart · 2022-12-02T14:23:59Z

sound/soc/intel/boards/sof_es8336.c

+	gpiod_set_value_cansleep(priv->gpio_speakers, false);

-	priv->gpio_headphone = gpiod_get_optional(codec_dev, "headphone-enable", GPIOD_OUT_LOW);
+	priv->gpio_headphone = gpiod_get_optional(codec_dev, "headphone-enable", GPIOD_OUT_HIGH);


is this intentional? why would the GPIO level change?

In the past speaker_en false means on (high) and true means off (low) because the code is taken from bytcht driver. The patch inverts this.

but this is going to break existing solutions, I really don't understand this change.

plbossart · 2022-12-02T14:24:56Z

sound/soc/intel/boards/sof_es8336.c

 					SOF_HDMI_CAPTURE_1_SSP(0) |
 					SOF_HDMI_CAPTURE_2_SSP(2) |
 					SOF_SSP_HDMI_CAPTURE_PRESENT |
-					SOF_ES8336_SPEAKERS_EN_GPIO1_QUIRK |


if gpio1 was used, why did you remove this?

On Windows GPIO0 is always the speaker GPIO. The quirk may be reused to invert the GPIO level. (The results are the same: either invert the level or invert the order.)

I can't follow this "either invert the level or invert the order". How is this equivalent?

GPIO1 GPIO2

Speaker on high low

Headphone on low high

Inverting the high/low level or changing the GPIO order are basically the same.

plbossart

It still fundamentally do not understand how the levels of the GPIOs is determined. It seems that for speakers it was changed from LOW to HIGH, so it must have some effects on existing users.

Can we not use the _DSM information to determine what the active level is for GPIO0 and GPIO1?

plbossart · 2022-12-06T14:37:19Z

sound/soc/intel/boards/sof_es8336.c


-	if (quirk & SOF_ES8336_HEADPHONE_GPIO)
-		gpiod_set_value_cansleep(priv->gpio_headphone, priv->speaker_en);
+	gpiod_set_value_cansleep(priv->gpio_headphone, true);


why is the setting for headphone independent from the speaker enablement? Shouldn't the value be !priv->speaker_en?

Speaker GPIO Headphone GPIO

Playing music Speaker on high low

Playing music Headphone on low high

Suspend low low

When only one GPIO is used things are simple. The GPIO is high by default. Before playing music, the SPEAKER_ON dapm event is called and the GPIO set to high. When music stop, the GPIO is set to low so that there is no pop noise.

When HP GPIO is involved things become complicated. I want to do the same for HP GPIO but I don't know how. Maybe another SND_SOC_DAPM_SUPPLY for headphone should be invloved or maybe the headphone status can be read by snd_soc_dapm_get_pin_status(&card->dapm, "Headphone"); and the headphone can be turned on and off in sof_8336_trigger

plbossart · 2022-12-06T14:38:27Z

sound/soc/intel/boards/sof_es8336.c

-	if (priv->speaker_en == !SND_SOC_DAPM_EVENT_ON(event))
-		return 0;
+        dev_info(card->dev, "sof_es8316_speaker_power_event %d\n",SND_SOC_DAPM_EVENT_ON(event));
+        /* Redundant check, removed */


why is it redundant? and if yes, it should be a separate patch to avoid confusing reviewers between different functionality and code cleanups.

This check will return 0 when first time the event is called. So the gpios are always the default value the first time sound is played. Should be a seperate patch.

plbossart · 2022-12-06T14:38:49Z

sound/soc/intel/boards/sof_es8336.c


-	if (priv->speaker_en == !SND_SOC_DAPM_EVENT_ON(event))
-		return 0;
+        dev_info(card->dev, "sof_es8316_speaker_power_event %d\n",SND_SOC_DAPM_EVENT_ON(event));


dev_info is way too verbose.

plbossart · 2022-12-06T14:39:08Z

sound/soc/intel/boards/sof_es8336.c


 	snd_soc_component_set_jack(codec, &priv->jack, NULL);
+	ret = snd_soc_dapm_get_pin_status(&card->dapm, "Speaker");
+	dev_info(card->dev, "Speaker status %d", ret);


why is this needed?

plbossart · 2022-12-06T14:39:53Z

sound/soc/intel/boards/sof_es8336.c

 		goto err_put_codec;
 	}
-
+	


spurious line change

plbossart · 2022-12-06T14:40:20Z

sound/soc/intel/boards/sof_es8336.c

+	gpiod_set_value_cansleep(priv->gpio_speakers, false);

-	priv->gpio_headphone = gpiod_get_optional(codec_dev, "headphone-enable", GPIOD_OUT_LOW);
+	priv->gpio_headphone = gpiod_get_optional(codec_dev, "headphone-enable", GPIOD_OUT_HIGH);


but this is going to break existing solutions, I really don't understand this change.

yangxiaohua2009 · 2022-12-07T02:25:22Z

It still fundamentally do not understand how the levels of the GPIOs is determined. It seems that for speakers it was changed from LOW to HIGH, so it must have some effects on existing users.

Can we not use the _DSM information to determine what the active level is for GPIO0 and GPIO1?

Yes there are values defined for GPIO in the DSM, but I don't know how to get them in the machine driver.

/*
* TO DECLARE SPEAKER CONTROL LEVEL
* RETURN VALUE = 1, High level enable
*		 0,low level enable
*/
#define  SPK_CTL_IO_LEVEL 0x5B

/* TO DECLARE HEADPHONE SWITCH CONTROL LEVEL
 *	RETURN VALUE = 1, High level enable
 *		       0, low level enable
 */
#define HPSWITCH_CTL_IO_LEVEL 0x4B

plbossart · 2023-04-17T20:34:39Z

@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.

yangxiaohua2009 mentioned this pull request Dec 2, 2022

ASoC: es83xx: extract codec configuration with _DSM #4049

Closed

plbossart requested changes Dec 2, 2022

View reviewed changes

plbossart requested changes Dec 6, 2022

View reviewed changes

plbossart closed this Apr 17, 2023

	Speaker GPIO	Headphone GPIO
Playing music Speaker on	high	low
Playing music Headphone on	low	high
Suspend	low	low

Conversation

yangxiaohua2009 commented Dec 2, 2022

Uh oh!

yangxiaohua2009 commented Dec 2, 2022

Uh oh!

yangxiaohua2009 commented Dec 2, 2022

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yangxiaohua2009 commented Dec 7, 2022

Uh oh!

plbossart commented Apr 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants