[PATCH 0/6] wcd938x/wcd939x fixes

[PATCH 0/6] wcd938x/wcd939x fixes

[Jonathan Marek]

Fixes for two major issues I encountered
- pop/click
- not being able to enable both left/right channels

And a fix for an issue I noticed in the wsa codecs.

Jonathan Marek (6):
  ASoC: codecs: wsa884x: remove mute_unmute_on_trigger
  ASoC: codecs: wsa883x: remove mute_unmute_on_trigger
  ASoC: codecs: wcd939x: fix headphone pop/click sound
  ASoC: codecs: wcd938x: fix headphone pop/click sound
  ASoC: codecs: wcd939x: fix get_swr_port behavior
  ASoC: codecs: wcd938x: fix get_swr_port behavior

 sound/soc/codecs/wcd938x.c | 85 +++++++++++++++++------------------
 sound/soc/codecs/wcd938x.h |  1 -
 sound/soc/codecs/wcd939x.c | 92 ++++++++++++++++++--------------------
 sound/soc/codecs/wcd939x.h |  1 -
 sound/soc/codecs/wsa883x.c |  1 -
 sound/soc/codecs/wsa884x.c |  1 -
 6 files changed, 84 insertions(+), 97 deletions(-)

-- 
2.51.0

[PATCH 1/6] ASoC: codecs: wsa884x: remove mute_unmute_on_trigger

[Jonathan Marek]

trigger is atomic (non-schedulable), and soundwire register writes are not
safe to run in an atomic context. (bus is locked with a mutex, and qcom
driver's callback can also sleep if the FIFO is full).

The important part of fixing the click/pop issue was removing the PA_EN
writes from the dapm events, AFAICT this flag doesn't help anyway.

Fixes: 28b0b18d5346 ("ASoC: codec: wsa884x: make use of new mute_unmute_on_trigger flag")
Signed-off-by: Jonathan Marek <jonathan@marek.ca>
---
 sound/soc/codecs/wsa884x.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/sound/soc/codecs/wsa884x.c b/sound/soc/codecs/wsa884x.c
index 2484d4b8e2d94..0218dfc13bc77 100644
--- a/sound/soc/codecs/wsa884x.c
+++ b/sound/soc/codecs/wsa884x.c
@@ -1840,7 +1840,6 @@ static const struct snd_soc_dai_ops wsa884x_dai_ops = {
 	.hw_free = wsa884x_hw_free,
 	.mute_stream = wsa884x_mute_stream,
 	.set_stream = wsa884x_set_stream,
-	.mute_unmute_on_trigger = true,
 };
 
 static struct snd_soc_dai_driver wsa884x_dais[] = {
-- 
2.51.0

[PATCH 2/6] ASoC: codecs: wsa883x: remove mute_unmute_on_trigger

[Jonathan Marek]

trigger is atomic (non-schedulable), and soundwire register writes are not
safe to run in an atomic context. (bus is locked with a mutex, and qcom
driver's callback can also sleep if the FIFO is full).

The important part of fixing the click/pop issue was removing the PA_EN
writes from the dapm events, AFAICT this flag doesn't help anyway.

Note I only tested the equivalent wsa884x change.

Fixes: 805ce81826c8 ("ASoC: codecs: wsa883x: make use of new mute_unmute_on_trigger flag")
Signed-off-by: Jonathan Marek <jonathan@marek.ca>
---
 sound/soc/codecs/wsa883x.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/sound/soc/codecs/wsa883x.c b/sound/soc/codecs/wsa883x.c
index ca4520ade79aa..06e963b5e853e 100644
--- a/sound/soc/codecs/wsa883x.c
+++ b/sound/soc/codecs/wsa883x.c
@@ -1396,7 +1396,6 @@ static const struct snd_soc_dai_ops wsa883x_dai_ops = {
 	.hw_free = wsa883x_hw_free,
 	.mute_stream = wsa883x_digital_mute,
 	.set_stream = wsa883x_set_sdw_stream,
-	.mute_unmute_on_trigger = true,
 };
 
 static struct snd_soc_dai_driver wsa883x_dais[] = {
-- 
2.51.0

[PATCH 3/6] ASoC: codecs: wcd939x: fix headphone pop/click sound

[Jonathan Marek]

PA enable must happen while the soundwire stream is enabled (between
sdw_enable_stream and sdw_disable_stream) to avoid issues. This is
between the link prepare and hw_free for qcom drivers.

Move the PA enable/disable to a mute_stream callback, which satisfies this
condition.

Note the dapm events already cleared HPHL_ENABLE/etc. bits, so only writes
to set them need to be added.

I used the DAC events to determine if PA should be enabled, which is not
exactly the same as before, but practically it shouldn't make a difference.

Signed-off-by: Jonathan Marek <jonathan@marek.ca>
---
 sound/soc/codecs/wcd939x.c | 84 ++++++++++++++++++--------------------
 1 file changed, 39 insertions(+), 45 deletions(-)

diff --git a/sound/soc/codecs/wcd939x.c b/sound/soc/codecs/wcd939x.c
index e74e6f0131318..48f82a92722dd 100644
--- a/sound/soc/codecs/wcd939x.c
+++ b/sound/soc/codecs/wcd939x.c
@@ -209,6 +209,8 @@ struct wcd939x_priv {
 	bool comp1_enable;
 	bool comp2_enable;
 	bool ldoh;
+	bool hphl_enable;
+	bool hphr_enable;
 };
 
 static const char * const wcd939x_supplies[] = {
@@ -508,6 +510,7 @@ static int wcd939x_codec_hphl_dac_event(struct snd_soc_dapm_widget *w,
 
 	switch (event) {
 	case SND_SOC_DAPM_PRE_PMU:
+		wcd939x->hphl_enable = true;
 		snd_soc_component_write_field(component, WCD939X_HPH_RDAC_CLK_CTL1,
 					      WCD939X_RDAC_CLK_CTL1_OPAMP_CHOP_CLK_EN,
 					      false);
@@ -547,6 +550,7 @@ static int wcd939x_codec_hphl_dac_event(struct snd_soc_dapm_widget *w,
 					      WCD939X_RDAC_HD2_CTL_L_HD2_RES_DIV_CTL_L, 1);
 		snd_soc_component_write_field(component, WCD939X_DIGITAL_CDC_HPH_GAIN_CTL,
 					      WCD939X_CDC_HPH_GAIN_CTL_HPHL_RX_EN, false);
+		wcd939x->hphl_enable = false;
 		break;
 	}
 
@@ -565,6 +569,7 @@ static int wcd939x_codec_hphr_dac_event(struct snd_soc_dapm_widget *w,
 
 	switch (event) {
 	case SND_SOC_DAPM_PRE_PMU:
+		wcd939x->hphr_enable = true;
 		snd_soc_component_write_field(component, WCD939X_HPH_RDAC_CLK_CTL1,
 					      WCD939X_RDAC_CLK_CTL1_OPAMP_CHOP_CLK_EN,
 					      false);
@@ -603,6 +608,7 @@ static int wcd939x_codec_hphr_dac_event(struct snd_soc_dapm_widget *w,
 					      WCD939X_RDAC_HD2_CTL_R_HD2_RES_DIV_CTL_R, 1);
 		snd_soc_component_write_field(component, WCD939X_DIGITAL_CDC_HPH_GAIN_CTL,
 					      WCD939X_CDC_HPH_GAIN_CTL_HPHR_RX_EN, false);
+		wcd939x->hphr_enable = false;
 		break;
 	}
 
@@ -641,16 +647,12 @@ static int wcd939x_codec_ear_dac_event(struct snd_soc_dapm_widget *w,
 	return 0;
 }
 
-static int wcd939x_codec_enable_hphr_pa(struct snd_soc_dapm_widget *w,
-					struct snd_kcontrol *kcontrol,
-					int event)
+static void wcd939x_codec_enable_hphr_pa(struct snd_soc_component *component, int enable)
 {
-	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
 	struct wcd939x_priv *wcd939x = snd_soc_component_get_drvdata(component);
 	int hph_mode = wcd939x->hph_mode;
 
-	switch (event) {
-	case SND_SOC_DAPM_PRE_PMU:
+	if (enable) {
 		if (wcd939x->ldoh)
 			snd_soc_component_write_field(component, WCD939X_LDOH_MODE,
 						      WCD939X_MODE_LDOH_EN, true);
@@ -679,8 +681,7 @@ static int wcd939x_codec_enable_hphr_pa(struct snd_soc_dapm_widget *w,
 		set_bit(HPH_PA_DELAY, &wcd939x->status_mask);
 		snd_soc_component_write_field(component, WCD939X_DIGITAL_PDM_WD_CTL1,
 					      WCD939X_PDM_WD_CTL1_PDM_WD_EN, 3);
-		break;
-	case SND_SOC_DAPM_POST_PMU:
+
 		/*
 		 * 7ms sleep is required if compander is enabled as per
 		 * HW requirement. If compander is disabled, then
@@ -708,9 +709,11 @@ static int wcd939x_codec_enable_hphr_pa(struct snd_soc_dapm_widget *w,
 						      WCD939X_RX_SUPPLIES_REGULATOR_MODE,
 						      true);
 
+		snd_soc_component_write_field(component, WCD939X_ANA_HPH,
+					      WCD939X_HPH_HPHR_ENABLE, true);
+
 		enable_irq(wcd939x->hphr_pdm_wd_int);
-		break;
-	case SND_SOC_DAPM_PRE_PMD:
+	} else {
 		disable_irq_nosync(wcd939x->hphr_pdm_wd_int);
 		/*
 		 * 7ms sleep is required if compander is enabled as per
@@ -728,8 +731,7 @@ static int wcd939x_codec_enable_hphr_pa(struct snd_soc_dapm_widget *w,
 		wcd_mbhc_event_notify(wcd939x->wcd_mbhc,
 				      WCD_EVENT_PRE_HPHR_PA_OFF);
 		set_bit(HPH_PA_DELAY, &wcd939x->status_mask);
-		break;
-	case SND_SOC_DAPM_POST_PMD:
+
 		/*
 		 * 7ms sleep is required if compander is enabled as per
 		 * HW requirement. If compander is disabled, then
@@ -755,25 +757,15 @@ static int wcd939x_codec_enable_hphr_pa(struct snd_soc_dapm_widget *w,
 		if (wcd939x->ldoh)
 			snd_soc_component_write_field(component, WCD939X_LDOH_MODE,
 						      WCD939X_MODE_LDOH_EN, false);
-		break;
 	}
-
-	return 0;
 }
 
-static int wcd939x_codec_enable_hphl_pa(struct snd_soc_dapm_widget *w,
-					struct snd_kcontrol *kcontrol,
-					int event)
+static void wcd939x_codec_enable_hphl_pa(struct snd_soc_component *component, bool enable)
 {
-	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
 	struct wcd939x_priv *wcd939x = snd_soc_component_get_drvdata(component);
 	int hph_mode = wcd939x->hph_mode;
 
-	dev_dbg(component->dev, "%s wname: %s event: %d\n", __func__,
-		w->name, event);
-
-	switch (event) {
-	case SND_SOC_DAPM_PRE_PMU:
+	if (enable) {
 		if (wcd939x->ldoh)
 			snd_soc_component_write_field(component, WCD939X_LDOH_MODE,
 						      WCD939X_MODE_LDOH_EN, true);
@@ -802,8 +794,7 @@ static int wcd939x_codec_enable_hphl_pa(struct snd_soc_dapm_widget *w,
 		set_bit(HPH_PA_DELAY, &wcd939x->status_mask);
 		snd_soc_component_write_field(component, WCD939X_DIGITAL_PDM_WD_CTL0,
 					      WCD939X_PDM_WD_CTL0_PDM_WD_EN, 3);
-		break;
-	case SND_SOC_DAPM_POST_PMU:
+
 		/*
 		 * 7ms sleep is required if compander is enabled as per
 		 * HW requirement. If compander is disabled, then
@@ -829,9 +820,12 @@ static int wcd939x_codec_enable_hphl_pa(struct snd_soc_dapm_widget *w,
 			snd_soc_component_write_field(component, WCD939X_ANA_RX_SUPPLIES,
 						      WCD939X_RX_SUPPLIES_REGULATOR_MODE,
 						      true);
+
+		snd_soc_component_write_field(component, WCD939X_ANA_HPH,
+					      WCD939X_HPH_HPHL_ENABLE, true);
+
 		enable_irq(wcd939x->hphl_pdm_wd_int);
-		break;
-	case SND_SOC_DAPM_PRE_PMD:
+	} else {
 		disable_irq_nosync(wcd939x->hphl_pdm_wd_int);
 		/*
 		 * 7ms sleep is required if compander is enabled as per
@@ -848,8 +842,7 @@ static int wcd939x_codec_enable_hphl_pa(struct snd_soc_dapm_widget *w,
 
 		wcd_mbhc_event_notify(wcd939x->wcd_mbhc, WCD_EVENT_PRE_HPHL_PA_OFF);
 		set_bit(HPH_PA_DELAY, &wcd939x->status_mask);
-		break;
-	case SND_SOC_DAPM_POST_PMD:
+
 		/*
 		 * 7ms sleep is required if compander is enabled as per
 		 * HW requirement. If compander is disabled, then
@@ -873,10 +866,7 @@ static int wcd939x_codec_enable_hphl_pa(struct snd_soc_dapm_widget *w,
 		if (wcd939x->ldoh)
 			snd_soc_component_write_field(component, WCD939X_LDOH_MODE,
 						      WCD939X_MODE_LDOH_EN, false);
-		break;
 	}
-
-	return 0;
 }
 
 static int wcd939x_codec_enable_ear_pa(struct snd_soc_dapm_widget *w,
@@ -2736,14 +2726,6 @@ static const struct snd_soc_dapm_widget wcd939x_dapm_widgets[] = {
 			   wcd939x_codec_enable_ear_pa,
 			   SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU |
 			   SND_SOC_DAPM_PRE_PMD | SND_SOC_DAPM_POST_PMD),
-	SND_SOC_DAPM_PGA_E("HPHL PGA", WCD939X_ANA_HPH, 7, 0, NULL, 0,
-			   wcd939x_codec_enable_hphl_pa,
-			   SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU |
-			   SND_SOC_DAPM_PRE_PMD | SND_SOC_DAPM_POST_PMD),
-	SND_SOC_DAPM_PGA_E("HPHR PGA", WCD939X_ANA_HPH, 6, 0, NULL, 0,
-			   wcd939x_codec_enable_hphr_pa,
-			   SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU |
-			   SND_SOC_DAPM_PRE_PMD | SND_SOC_DAPM_POST_PMD),
 
 	SND_SOC_DAPM_DAC_E("RDAC1", NULL, SND_SOC_NOPM, 0, 0,
 			   wcd939x_codec_hphl_dac_event,
@@ -2858,8 +2840,7 @@ static const struct snd_soc_dapm_route wcd939x_audio_map[] = {
 	{"RX1", NULL, "RXCLK"},
 	{"RDAC1", NULL, "RX1"},
 	{"HPHL_RDAC", "Switch", "RDAC1"},
-	{"HPHL PGA", NULL, "HPHL_RDAC"},
-	{"HPHL", NULL, "HPHL PGA"},
+	{"HPHL", NULL, "HPHL_RDAC"},
 
 	{"IN2_HPHR", NULL, "VDD_BUCK"},
 	{"IN2_HPHR", NULL, "CLS_H_PORT"},
@@ -2867,8 +2848,7 @@ static const struct snd_soc_dapm_route wcd939x_audio_map[] = {
 	{"RDAC2", NULL, "RX2"},
 	{"RX2", NULL, "RXCLK"},
 	{"HPHR_RDAC", "Switch", "RDAC2"},
-	{"HPHR PGA", NULL, "HPHR_RDAC"},
-	{"HPHR", NULL, "HPHR PGA"},
+	{"HPHR", NULL, "HPHR_RDAC"},
 
 	{"IN3_EAR", NULL, "VDD_BUCK"},
 	{"RX3", NULL, "IN3_EAR"},
@@ -3258,6 +3238,19 @@ static int wcd939x_codec_free(struct snd_pcm_substream *substream,
 	return wcd939x_sdw_free(wcd, substream, dai);
 }
 
+static int wcd939x_codec_mute_stream(struct snd_soc_dai *dai, int mute, int stream)
+{
+	struct wcd939x_priv *wcd939x = dev_get_drvdata(dai->dev);
+	struct snd_soc_component *component = dai->component;
+
+	if (wcd939x->hphl_enable)
+		wcd939x_codec_enable_hphl_pa(component, !mute);
+	if (wcd939x->hphr_enable)
+		wcd939x_codec_enable_hphr_pa(component, !mute);
+
+	return 0;
+}
+
 static int wcd939x_codec_set_sdw_stream(struct snd_soc_dai *dai,
 					void *stream, int direction)
 {
@@ -3270,6 +3263,7 @@ static int wcd939x_codec_set_sdw_stream(struct snd_soc_dai *dai,
 static const struct snd_soc_dai_ops wcd939x_sdw_dai_ops = {
 	.hw_params = wcd939x_codec_hw_params,
 	.hw_free = wcd939x_codec_free,
+	.mute_stream = wcd939x_codec_mute_stream,
 	.set_stream = wcd939x_codec_set_sdw_stream,
 };
 
-- 
2.51.0

[PATCH 4/6] ASoC: codecs: wcd938x: fix headphone pop/click sound

[Jonathan Marek]

PA enable must happen while the soundwire stream is enabled (between
sdw_enable_stream and sdw_disable_stream) to avoid issues. This is
between the link prepare and hw_free for qcom drivers.

Move the PA enable/disable to a mute_stream callback, which satisfies this
condition.

Note the dapm events already cleared HPHL_ENABLE/etc. bits, so only writes
to set them need to be added.

I used the DAC events to determine if PA should be enabled, which is not
exactly the same as before, but practically it shouldn't make a difference.

Signed-off-by: Jonathan Marek <jonathan@marek.ca>
---
 sound/soc/codecs/wcd938x.c | 76 ++++++++++++++++++--------------------
 1 file changed, 36 insertions(+), 40 deletions(-)

diff --git a/sound/soc/codecs/wcd938x.c b/sound/soc/codecs/wcd938x.c
index e1a4783b984c1..cf15190dd61a4 100644
--- a/sound/soc/codecs/wcd938x.c
+++ b/sound/soc/codecs/wcd938x.c
@@ -178,6 +178,8 @@ struct wcd938x_priv {
 	bool comp2_enable;
 	bool ldoh;
 	bool mux_setup_done;
+	bool hphl_enable;
+	bool hphr_enable;
 };
 
 static const char * const wcd938x_supplies[] = {
@@ -468,6 +470,7 @@ static int wcd938x_codec_hphl_dac_event(struct snd_soc_dapm_widget *w,
 
 	switch (event) {
 	case SND_SOC_DAPM_PRE_PMU:
+		wcd938x->hphl_enable = true;
 		snd_soc_component_write_field(component,
 				WCD938X_DIGITAL_CDC_DIG_CLK_CTL,
 				WCD938X_RXD0_CLK_EN_MASK, 0x01);
@@ -507,6 +510,7 @@ static int wcd938x_codec_hphl_dac_event(struct snd_soc_dapm_widget *w,
 		snd_soc_component_write_field(component,
 			WCD938X_HPH_NEW_INT_RDAC_HD2_CTL_R,
 			WCD938X_HPH_RES_DIV_MASK, 0x1);
+		wcd938x->hphl_enable = false;
 		break;
 	}
 
@@ -522,6 +526,7 @@ static int wcd938x_codec_hphr_dac_event(struct snd_soc_dapm_widget *w,
 
 	switch (event) {
 	case SND_SOC_DAPM_PRE_PMU:
+		wcd938x->hphr_enable = true;
 		snd_soc_component_write_field(component,
 				WCD938X_DIGITAL_CDC_DIG_CLK_CTL,
 				WCD938X_RXD1_CLK_EN_MASK, 1);
@@ -558,6 +563,7 @@ static int wcd938x_codec_hphr_dac_event(struct snd_soc_dapm_widget *w,
 		}
 		break;
 	case SND_SOC_DAPM_POST_PMD:
+		wcd938x->hphr_enable = false;
 		snd_soc_component_write_field(component,
 			WCD938X_HPH_NEW_INT_RDAC_HD2_CTL_R,
 			WCD938X_HPH_RES_DIV_MASK, 0x01);
@@ -683,15 +689,12 @@ static int wcd938x_codec_aux_dac_event(struct snd_soc_dapm_widget *w,
 
 }
 
-static int wcd938x_codec_enable_hphr_pa(struct snd_soc_dapm_widget *w,
-					struct snd_kcontrol *kcontrol, int event)
+static void wcd938x_codec_enable_hphr_pa(struct snd_soc_component *component, int enable)
 {
-	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
 	struct wcd938x_priv *wcd938x = snd_soc_component_get_drvdata(component);
 	int hph_mode = wcd938x->hph_mode;
 
-	switch (event) {
-	case SND_SOC_DAPM_PRE_PMU:
+	if (enable) {
 		if (wcd938x->ldoh)
 			snd_soc_component_write_field(component, WCD938X_LDOH_MODE,
 						      WCD938X_LDOH_EN_MASK, 1);
@@ -714,8 +717,7 @@ static int wcd938x_codec_enable_hphr_pa(struct snd_soc_dapm_widget *w,
 		snd_soc_component_write_field(component,
 					      WCD938X_DIGITAL_PDM_WD_CTL1,
 					      WCD938X_PDM_WD_EN_MASK, 0x3);
-		break;
-	case SND_SOC_DAPM_POST_PMU:
+
 		/*
 		 * 7ms sleep is required if compander is enabled as per
 		 * HW requirement. If compander is disabled, then
@@ -741,9 +743,10 @@ static int wcd938x_codec_enable_hphr_pa(struct snd_soc_dapm_widget *w,
 			snd_soc_component_write_field(component, WCD938X_ANA_RX_SUPPLIES,
 					WCD938X_REGULATOR_MODE_MASK,
 					WCD938X_REGULATOR_MODE_CLASS_AB);
+		snd_soc_component_write_field(component, WCD938X_ANA_HPH,
+					      WCD938X_HPHR_EN_MASK, 1);
 		enable_irq(wcd938x->hphr_pdm_wd_int);
-		break;
-	case SND_SOC_DAPM_PRE_PMD:
+	} else {
 		disable_irq_nosync(wcd938x->hphr_pdm_wd_int);
 		/*
 		 * 7ms sleep is required if compander is enabled as per
@@ -759,8 +762,7 @@ static int wcd938x_codec_enable_hphr_pa(struct snd_soc_dapm_widget *w,
 		wcd_mbhc_event_notify(wcd938x->wcd_mbhc,
 					     WCD_EVENT_PRE_HPHR_PA_OFF);
 		set_bit(HPH_PA_DELAY, &wcd938x->status_mask);
-		break;
-	case SND_SOC_DAPM_POST_PMD:
+
 		/*
 		 * 7ms sleep is required if compander is enabled as per
 		 * HW requirement. If compander is disabled, then
@@ -784,21 +786,15 @@ static int wcd938x_codec_enable_hphr_pa(struct snd_soc_dapm_widget *w,
 		if (wcd938x->ldoh)
 			snd_soc_component_write_field(component, WCD938X_LDOH_MODE,
 						      WCD938X_LDOH_EN_MASK, 0);
-		break;
 	}
-
-	return 0;
 }
 
-static int wcd938x_codec_enable_hphl_pa(struct snd_soc_dapm_widget *w,
-					struct snd_kcontrol *kcontrol, int event)
+static void wcd938x_codec_enable_hphl_pa(struct snd_soc_component *component, int enable)
 {
-	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
 	struct wcd938x_priv *wcd938x = snd_soc_component_get_drvdata(component);
 	int hph_mode = wcd938x->hph_mode;
 
-	switch (event) {
-	case SND_SOC_DAPM_PRE_PMU:
+	if (enable) {
 		if (wcd938x->ldoh)
 			snd_soc_component_write_field(component, WCD938X_LDOH_MODE,
 						      WCD938X_LDOH_EN_MASK, 1);
@@ -820,8 +816,7 @@ static int wcd938x_codec_enable_hphl_pa(struct snd_soc_dapm_widget *w,
 		snd_soc_component_write_field(component,
 					WCD938X_DIGITAL_PDM_WD_CTL0,
 					WCD938X_PDM_WD_EN_MASK, 0x3);
-		break;
-	case SND_SOC_DAPM_POST_PMU:
+
 		/*
 		 * 7ms sleep is required if compander is enabled as per
 		 * HW requirement. If compander is disabled, then
@@ -847,9 +842,10 @@ static int wcd938x_codec_enable_hphl_pa(struct snd_soc_dapm_widget *w,
 			snd_soc_component_write_field(component, WCD938X_ANA_RX_SUPPLIES,
 					WCD938X_REGULATOR_MODE_MASK,
 					WCD938X_REGULATOR_MODE_CLASS_AB);
+		snd_soc_component_write_field(component, WCD938X_ANA_HPH,
+					      WCD938X_HPHL_EN_MASK, 1);
 		enable_irq(wcd938x->hphl_pdm_wd_int);
-		break;
-	case SND_SOC_DAPM_PRE_PMD:
+	} else {
 		disable_irq_nosync(wcd938x->hphl_pdm_wd_int);
 		/*
 		 * 7ms sleep is required if compander is enabled as per
@@ -864,8 +860,7 @@ static int wcd938x_codec_enable_hphl_pa(struct snd_soc_dapm_widget *w,
 					      WCD938X_HPHL_EN_MASK, 0);
 		wcd_mbhc_event_notify(wcd938x->wcd_mbhc, WCD_EVENT_PRE_HPHL_PA_OFF);
 		set_bit(HPH_PA_DELAY, &wcd938x->status_mask);
-		break;
-	case SND_SOC_DAPM_POST_PMD:
+
 		/*
 		 * 7ms sleep is required if compander is enabled as per
 		 * HW requirement. If compander is disabled, then
@@ -889,10 +884,7 @@ static int wcd938x_codec_enable_hphl_pa(struct snd_soc_dapm_widget *w,
 		if (wcd938x->ldoh)
 			snd_soc_component_write_field(component, WCD938X_LDOH_MODE,
 						      WCD938X_LDOH_EN_MASK, 0);
-		break;
 	}
-
-	return 0;
 }
 
 static int wcd938x_codec_enable_aux_pa(struct snd_soc_dapm_widget *w,
@@ -2815,14 +2807,6 @@ static const struct snd_soc_dapm_widget wcd938x_dapm_widgets[] = {
 			   wcd938x_codec_enable_aux_pa,
 			   SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU |
 			   SND_SOC_DAPM_PRE_PMD | SND_SOC_DAPM_POST_PMD),
-	SND_SOC_DAPM_PGA_E("HPHL PGA", WCD938X_ANA_HPH, 7, 0, NULL, 0,
-			   wcd938x_codec_enable_hphl_pa,
-			   SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU |
-			   SND_SOC_DAPM_PRE_PMD | SND_SOC_DAPM_POST_PMD),
-	SND_SOC_DAPM_PGA_E("HPHR PGA", WCD938X_ANA_HPH, 6, 0, NULL, 0,
-			   wcd938x_codec_enable_hphr_pa,
-			   SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU |
-			   SND_SOC_DAPM_PRE_PMD | SND_SOC_DAPM_POST_PMD),
 
 	SND_SOC_DAPM_DAC_E("RDAC1", NULL, SND_SOC_NOPM, 0, 0,
 			   wcd938x_codec_hphl_dac_event,
@@ -2935,8 +2919,7 @@ static const struct snd_soc_dapm_route wcd938x_audio_map[] = {
 	{"RX1", NULL, "RXCLK"},
 	{"RDAC1", NULL, "RX1"},
 	{"HPHL_RDAC", "Switch", "RDAC1"},
-	{"HPHL PGA", NULL, "HPHL_RDAC"},
-	{"HPHL", NULL, "HPHL PGA"},
+	{"HPHL", NULL, "HPHL_RDAC"},
 
 	{"IN2_HPHR", NULL, "VDD_BUCK"},
 	{"IN2_HPHR", NULL, "CLS_H_PORT"},
@@ -2944,8 +2927,7 @@ static const struct snd_soc_dapm_route wcd938x_audio_map[] = {
 	{"RDAC2", NULL, "RX2"},
 	{"RX2", NULL, "RXCLK"},
 	{"HPHR_RDAC", "Switch", "RDAC2"},
-	{"HPHR PGA", NULL, "HPHR_RDAC"},
-	{"HPHR", NULL, "HPHR PGA"},
+	{"HPHR", NULL, "HPHR_RDAC"},
 
 	{"IN3_AUX", NULL, "VDD_BUCK"},
 	{"IN3_AUX", NULL, "CLS_H_PORT"},
@@ -3290,6 +3272,19 @@ static int wcd938x_codec_free(struct snd_pcm_substream *substream,
 	return wcd938x_sdw_free(wcd, substream, dai);
 }
 
+static int wcd938x_codec_mute_stream(struct snd_soc_dai *dai, int mute, int stream)
+{
+	struct wcd938x_priv *wcd938x = dev_get_drvdata(dai->dev);
+	struct snd_soc_component *component = dai->component;
+
+	if (wcd938x->hphl_enable)
+		wcd938x_codec_enable_hphl_pa(component, !mute);
+	if (wcd938x->hphr_enable)
+		wcd938x_codec_enable_hphr_pa(component, !mute);
+
+	return 0;
+}
+
 static int wcd938x_codec_set_sdw_stream(struct snd_soc_dai *dai,
 				  void *stream, int direction)
 {
@@ -3303,6 +3298,7 @@ static int wcd938x_codec_set_sdw_stream(struct snd_soc_dai *dai,
 static const struct snd_soc_dai_ops wcd938x_sdw_dai_ops = {
 	.hw_params = wcd938x_codec_hw_params,
 	.hw_free = wcd938x_codec_free,
+	.mute_stream = wcd938x_codec_mute_stream,
 	.set_stream = wcd938x_codec_set_sdw_stream,
 };
 
-- 
2.51.0

[PATCH 5/6] ASoC: codecs: wcd939x: fix get_swr_port behavior

[Jonathan Marek]

For example trying to do this:

amixer sset HPHL on
amixer sset HPHR on

Both HPHL and HPHR share the same "portidx" value. After HPHL is enabled,
wcd939x_get_swr_port returns an 'on' state for HPHR as well, and nothing
happens when trying to set HPHR on because it looks like it is already on.
The result is audio plays only on the left side.

To fix this, drop the port_enable array and use ch_mask in port_config to
determine if the switch was enabled.

Signed-off-by: Jonathan Marek <jonathan@marek.ca>
---
 sound/soc/codecs/wcd939x.c | 8 ++++----
 sound/soc/codecs/wcd939x.h | 1 -
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/sound/soc/codecs/wcd939x.c b/sound/soc/codecs/wcd939x.c
index 48f82a92722dd..8a95fe39792ea 100644
--- a/sound/soc/codecs/wcd939x.c
+++ b/sound/soc/codecs/wcd939x.c
@@ -1783,8 +1783,10 @@ static int wcd939x_get_swr_port(struct snd_kcontrol *kcontrol,
 	struct wcd939x_priv *wcd939x = snd_soc_component_get_drvdata(comp);
 	struct wcd939x_sdw_priv *wcd = wcd939x->sdw_priv[mixer->shift];
 	unsigned int portidx = wcd->ch_info[mixer->reg].port_num;
+	u8 ch_mask = wcd->ch_info[mixer->reg].ch_mask;
+	struct sdw_port_config *port_config = &wcd->port_config[portidx - 1];
 
-	ucontrol->value.integer.value[0] = wcd->port_enable[portidx] ? 1 : 0;
+	ucontrol->value.integer.value[0] = !!(port_config->ch_mask & ch_mask);
 
 	return 0;
 }
@@ -1811,9 +1813,7 @@ static int wcd939x_set_swr_port(struct snd_kcontrol *kcontrol,
 	struct wcd939x_sdw_priv *wcd = wcd939x->sdw_priv[mixer->shift];
 	unsigned int portidx = wcd->ch_info[mixer->reg].port_num;
 
-	wcd->port_enable[portidx] = !!ucontrol->value.integer.value[0];
-
-	wcd939x_connect_port(wcd, portidx, mixer->reg, wcd->port_enable[portidx]);
+	wcd939x_connect_port(wcd, portidx, mixer->reg, !!ucontrol->value.integer.value[0]);
 
 	return 1;
 }
diff --git a/sound/soc/codecs/wcd939x.h b/sound/soc/codecs/wcd939x.h
index 6bd2366587a8f..dab7ef108b485 100644
--- a/sound/soc/codecs/wcd939x.h
+++ b/sound/soc/codecs/wcd939x.h
@@ -899,7 +899,6 @@ struct wcd939x_sdw_priv {
 	struct sdw_stream_runtime *sruntime;
 	struct sdw_port_config port_config[WCD939X_MAX_SWR_PORTS];
 	const struct wcd_sdw_ch_info *ch_info;
-	bool port_enable[WCD939X_MAX_SWR_CH_IDS];
 	int active_ports;
 	bool is_tx;
 	struct wcd939x_priv *wcd939x;
-- 
2.51.0

[PATCH 6/6] ASoC: codecs: wcd938x: fix get_swr_port behavior

[Jonathan Marek]

For example trying to do this:

amixer sset HPHL on
amixer sset HPHR on

Both HPHL and HPHR share the same "portidx" value. After HPHL is enabled,
wcd939x_get_swr_port returns an 'on' state for HPHR as well, and nothing
happens when trying to set HPHR on because it looks like it is already on.
The result is audio plays only on the left side.

To fix this, drop the port_enable array and use ch_mask in port_config to
determine if the switch was enabled.

Signed-off-by: Jonathan Marek <jonathan@marek.ca>
---
 sound/soc/codecs/wcd938x.c | 9 +++++----
 sound/soc/codecs/wcd938x.h | 1 -
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/sound/soc/codecs/wcd938x.c b/sound/soc/codecs/wcd938x.c
index cf15190dd61a4..1768209e35ea0 100644
--- a/sound/soc/codecs/wcd938x.c
+++ b/sound/soc/codecs/wcd938x.c
@@ -1839,14 +1839,17 @@ static int wcd938x_get_swr_port(struct snd_kcontrol *kcontrol,
 	struct wcd938x_priv *wcd938x = snd_soc_component_get_drvdata(comp);
 	struct wcd938x_sdw_priv *wcd;
 	struct soc_mixer_control *mixer = (struct soc_mixer_control *)kcontrol->private_value;
+	struct sdw_port_config *port_config;
 	int dai_id = mixer->shift;
-	int portidx, ch_idx = mixer->reg;
+	int portidx, ch_idx = mixer->reg, ch_mask;
 
 
 	wcd = wcd938x->sdw_priv[dai_id];
 	portidx = wcd->ch_info[ch_idx].port_num;
+	ch_mask = wcd->ch_info[ch_idx].ch_mask;
+	port_config = &wcd->port_config[portidx - 1];
 
-	ucontrol->value.integer.value[0] = wcd->port_enable[portidx];
+	ucontrol->value.integer.value[0] = !!(port_config->ch_mask & ch_mask);
 
 	return 0;
 }
@@ -1872,8 +1875,6 @@ static int wcd938x_set_swr_port(struct snd_kcontrol *kcontrol,
 	else
 		enable = false;
 
-	wcd->port_enable[portidx] = enable;
-
 	wcd938x_connect_port(wcd, portidx, ch_idx, enable);
 
 	return 1;
diff --git a/sound/soc/codecs/wcd938x.h b/sound/soc/codecs/wcd938x.h
index c18610466d7d8..6650c3788d6e5 100644
--- a/sound/soc/codecs/wcd938x.h
+++ b/sound/soc/codecs/wcd938x.h
@@ -639,7 +639,6 @@ struct wcd938x_sdw_priv {
 	struct sdw_stream_runtime *sruntime;
 	struct sdw_port_config port_config[WCD938X_MAX_SWR_PORTS];
 	const struct wcd_sdw_ch_info *ch_info;
-	bool port_enable[WCD938X_MAX_SWR_CH_IDS];
 	int active_ports;
 	bool is_tx;
 	struct wcd938x_priv *wcd938x;
-- 
2.51.0

Re: [PATCH 1/6] ASoC: codecs: wsa884x: remove mute_unmute_on_trigger

[Srinivas Kandagatla]

On 11/24/25 6:45 AM, Jonathan Marek wrote:
> trigger is atomic (non-schedulable), and soundwire register writes are not
> safe to run in an atomic context. (bus is locked with a mutex, and qcom
> driver's callback can also sleep if the FIFO is full).
> 
Thanks Jonathan for the patch,

We have nonatomic=1 flag set for all the Qualcomm sound cards, Did you
hit any schedule while atomic bug?



In-fact this change has helped suppress most of the click and pop noises
on laptops, specially with wsa codecs as they accumulate static if the
ports are kept open without sending any data.

--srini


> The important part of fixing the click/pop issue was removing the PA_EN
> writes from the dapm events, AFAICT this flag doesn't help anyway.
> 
> Fixes: 28b0b18d5346 ("ASoC: codec: wsa884x: make use of new mute_unmute_on_trigger flag")
> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
> ---
>  sound/soc/codecs/wsa884x.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/sound/soc/codecs/wsa884x.c b/sound/soc/codecs/wsa884x.c
> index 2484d4b8e2d94..0218dfc13bc77 100644
> --- a/sound/soc/codecs/wsa884x.c
> +++ b/sound/soc/codecs/wsa884x.c
> @@ -1840,7 +1840,6 @@ static const struct snd_soc_dai_ops wsa884x_dai_ops = {
>  	.hw_free = wsa884x_hw_free,
>  	.mute_stream = wsa884x_mute_stream,
>  	.set_stream = wsa884x_set_stream,
> -	.mute_unmute_on_trigger = true,
>  };
>  
>  static struct snd_soc_dai_driver wsa884x_dais[] = {

Re: [PATCH 1/6] ASoC: codecs: wsa884x: remove mute_unmute_on_trigger

[Jonathan Marek]

On 11/24/25 9:08 AM, Srinivas Kandagatla wrote:
> 
> 
> On 11/24/25 6:45 AM, Jonathan Marek wrote:
>> trigger is atomic (non-schedulable), and soundwire register writes are not
>> safe to run in an atomic context. (bus is locked with a mutex, and qcom
>> driver's callback can also sleep if the FIFO is full).
>>
> Thanks Jonathan for the patch,
> 
> We have nonatomic=1 flag set for all the Qualcomm sound cards, Did you
> hit any schedule while atomic bug?
> 

Right, I missed that. I'm using a different driver which does not set 
nonatomic. But this driver to not need nonatomic - 
mute_unmute_on_trigger is a hack, if there is a timing requirement - 
then it needs to be explicit, the different timing with this flag is not 
reliable).

> 
> 
> In-fact this change has helped suppress most of the click and pop noises
> on laptops, specially with wsa codecs as they accumulate static if the
> ports are kept open without sending any data.
> 

28b0b18d5346 is important to fix the click and pop noises. But the 
useful part is the rest of the commit, not the mute_unmute_on_trigger 
flag. As long as the mute_stream() happens while the soundwire stream is 
enabled (between sdw_enable_stream and sdw_disable_stream), there should 
be no pop click.

AFAIK the pop/click is because of PDM: zeros (soundwire stream off) 
represent the minimum (negative maximum) amplitude, and the soundwire 
stream needs to be enabled to output a zero amplitude (alternating 
ones/zeros). Turning on the amp while the soundwire stream is not 
enabled will cause jumps between the minimum and zero amplitude.

> --srini
> 
> 
>> The important part of fixing the click/pop issue was removing the PA_EN
>> writes from the dapm events, AFAICT this flag doesn't help anyway.
>>
>> Fixes: 28b0b18d5346 ("ASoC: codec: wsa884x: make use of new mute_unmute_on_trigger flag")
>> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
>> ---
>>   sound/soc/codecs/wsa884x.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/sound/soc/codecs/wsa884x.c b/sound/soc/codecs/wsa884x.c
>> index 2484d4b8e2d94..0218dfc13bc77 100644
>> --- a/sound/soc/codecs/wsa884x.c
>> +++ b/sound/soc/codecs/wsa884x.c
>> @@ -1840,7 +1840,6 @@ static const struct snd_soc_dai_ops wsa884x_dai_ops = {
>>   	.hw_free = wsa884x_hw_free,
>>   	.mute_stream = wsa884x_mute_stream,
>>   	.set_stream = wsa884x_set_stream,
>> -	.mute_unmute_on_trigger = true,
>>   };
>>   
>>   static struct snd_soc_dai_driver wsa884x_dais[] = {
> 

Re: [PATCH 1/6] ASoC: codecs: wsa884x: remove mute_unmute_on_trigger

[Alexey Klimov]

On Mon Nov 24, 2025 at 2:55 PM GMT, Jonathan Marek wrote:
> On 11/24/25 9:08 AM, Srinivas Kandagatla wrote:
>> On 11/24/25 6:45 AM, Jonathan Marek wrote:
>>> trigger is atomic (non-schedulable), and soundwire register writes are not
>>> safe to run in an atomic context. (bus is locked with a mutex, and qcom
>>> driver's callback can also sleep if the FIFO is full).
>>>
>> Thanks Jonathan for the patch,
>> 
>> We have nonatomic=1 flag set for all the Qualcomm sound cards, Did you
>> hit any schedule while atomic bug?
>
> Right, I missed that. I'm using a different driver which does not set 
> nonatomic. But this driver to not need nonatomic - 
> mute_unmute_on_trigger is a hack, if there is a timing requirement - 
> then it needs to be explicit, the different timing with this flag is not 
> reliable).
>
>> In-fact this change has helped suppress most of the click and pop noises
>> on laptops, specially with wsa codecs as they accumulate static if the
>> ports are kept open without sending any data.
>> 
>
> 28b0b18d5346 is important to fix the click and pop noises. But the 
> useful part is the rest of the commit, not the mute_unmute_on_trigger 
> flag. As long as the mute_stream() happens while the soundwire stream is 
> enabled (between sdw_enable_stream and sdw_disable_stream), there should 
> be no pop click.
>
> AFAIK the pop/click is because of PDM: zeros (soundwire stream off) 
> represent the minimum (negative maximum) amplitude, and the soundwire 
> stream needs to be enabled to output a zero amplitude (alternating 
> ones/zeros). Turning on the amp while the soundwire stream is not 
> enabled will cause jumps between the minimum and zero amplitude.

>>> -	.mute_unmute_on_trigger = true,

FWIW for wsa881x in analog mode on RB1/RB2 boards I noticed that pop/clicks
sound behaviour is much better when mute_unmute_on_trigger is false.

Although, for wsa881x in soundwire mode the mute_unmute_on_trigger = true
gives decent result of supressing pop/clicks noise on the devices I have.

Maybe it varies from platform to platform and from board to board.

BR,
Alexey

Re: [PATCH 1/6] ASoC: codecs: wsa884x: remove mute_unmute_on_trigger

[Srinivas Kandagatla]

On 11/24/25 2:55 PM, Jonathan Marek wrote:
> On 11/24/25 9:08 AM, Srinivas Kandagatla wrote:
>>
Sorry for the delay, I have been trying to test these patches,

>>
>> On 11/24/25 6:45 AM, Jonathan Marek wrote:
>>> trigger is atomic (non-schedulable), and soundwire register writes
>>> are not
>>> safe to run in an atomic context. (bus is locked with a mutex, and qcom
>>> driver's callback can also sleep if the FIFO is full).
>>>
>> Thanks Jonathan for the patch,
>>
>> We have nonatomic=1 flag set for all the Qualcomm sound cards, Did you
>> hit any schedule while atomic bug?
>>
> 
> Right, I missed that. I'm using a different driver which does not set
> nonatomic. But this driver to not need nonatomic -


Interesting, I guess this is not an upstream driver. The change and log
itself does not make any sense on upstream. pl correct me otherwise.

> mute_unmute_on_trigger is a hack, if there is a timing requirement -
> then it needs to be explicit, the different timing with this flag is not
> reliable).
> 
>>
>>
>> In-fact this change has helped suppress most of the click and pop noises
>> on laptops, specially with wsa codecs as they accumulate static if the
>> ports are kept open without sending any data.
>>
> 
> 28b0b18d5346 is important to fix the click and pop noises. But the
> useful part is the rest of the commit, not the mute_unmute_on_trigger
> flag. As long as the mute_stream() happens while the soundwire stream is
> enabled (between sdw_enable_stream and sdw_disable_stream), there should
> be no pop click.
> 
> AFAIK the pop/click is because of PDM: zeros (soundwire stream off)
> represent the minimum (negative maximum) amplitude, and the soundwire
> stream needs to be enabled to output a zero amplitude (alternating ones/
> zeros). Turning on the amp while the soundwire stream is not enabled
> will cause jumps between the minimum and zero amplitude.
That is true, The issue that enable stream happens at machine driver
prepare and disable happens in hw_params_free.
We have window of opportunity for click and pop between the mute and
enable and in reverse direction.

Am not sure this patch is improving or fixing anything on this side.

Also I noticed during testing on T14s one of the speakers (left) went
into mute, not sure if this is something which is an existing issue but
reverting the patch made it work again.. Need more testing and debugging
to understand what could be going wrong.

I do acknowledge there is still some cases of pop-n-clicks which needs
fixing.


--srini

> 
>> --srini
>>
>>
>>> The important part of fixing the click/pop issue was removing the PA_EN
>>> writes from the dapm events, AFAICT this flag doesn't help anyway.
>>>
>>> Fixes: 28b0b18d5346 ("ASoC: codec: wsa884x: make use of new
>>> mute_unmute_on_trigger flag")
>>> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
>>> ---
>>>   sound/soc/codecs/wsa884x.c | 1 -
>>>   1 file changed, 1 deletion(-)
>>>
>>> diff --git a/sound/soc/codecs/wsa884x.c b/sound/soc/codecs/wsa884x.c
>>> index 2484d4b8e2d94..0218dfc13bc77 100644
>>> --- a/sound/soc/codecs/wsa884x.c
>>> +++ b/sound/soc/codecs/wsa884x.c
>>> @@ -1840,7 +1840,6 @@ static const struct snd_soc_dai_ops
>>> wsa884x_dai_ops = {
>>>       .hw_free = wsa884x_hw_free,
>>>       .mute_stream = wsa884x_mute_stream,
>>>       .set_stream = wsa884x_set_stream,
>>> -    .mute_unmute_on_trigger = true,
>>>   };
>>>     static struct snd_soc_dai_driver wsa884x_dais[] = {
>>

Re: [PATCH 1/6] ASoC: codecs: wsa884x: remove mute_unmute_on_trigger

[Jonathan Marek]

On 11/27/25 5:14 AM, Srinivas Kandagatla wrote:

> 
> 
> Interesting, I guess this is not an upstream driver. The change and log
> itself does not make any sense on upstream. pl correct me otherwise.
> 

> That is true, The issue that enable stream happens at machine driver
> prepare and disable happens in hw_params_free.
> We have window of opportunity for click and pop between the mute and
> enable and in reverse direction.
> 
> Am not sure this patch is improving or fixing anything on this side.
> 

This patch isn't improving or fixing anything (at least upstream),
its removing a hidden (and unnecessary) nonatomic dependency. It 
shouldn't change anything with the pop/click situation. Without 
mute_unmute_on_trigger the mute/unmute still happens inside that "window 
of opportunity".

My driver is not upstream but it could be, and I need atomic trigger. 
You can imagine that I am using the lpass-cpu driver (which is similar 
and upstream), which also does not want nonatomic trigger.

note:
- the common qcom sndcard parsing only sets nonatomic if a "platform" is 
set, which the lpass-cpu dts don't have
- none of the upstream dts that use lpass-cpu use wsa884x/wsa883x, but 
its possible to use these parts with sc7280
- upstream sc7280 uses wcd9385, so using mute_unmute_on_trigger in that 
driver would break upstream sc7280 audio

> Also I noticed during testing on T14s one of the speakers (left) went
> into mute, not sure if this is something which is an existing issue but
> reverting the patch made it work again.. Need more testing and debugging
> to understand what could be going wrong.
> 

This workaround might be relevant:

https://github.com/flto/linux/commit/83b6c10d7c6248b9633a1516b816f211607d4eca