[PATCH v2 00/11] Fix support for System Suspend for CS35L41 HDA

[PATCH v2 00/11] Fix support for System Suspend for CS35L41 HDA

[Stefan Binding]

There have been a couple of customer reports of intermittant issues after
system resume, where sometimes the DSP firmware stops responding.
Investigations into this issue show that there is a race between receiving
a prepare from the HDA core, and the firmware reload which is started by
the system resume. This can causes the Global Enable on the CS35L41 to be
enabled during the firmware load, which can sometimes cause issues in the
DSP.

The existing system resume behaviour also did not resume the audio, if
audio was previously playing when it was suspended.
In addition, during investigation, it was found there were additional
problems in the System Resume sequence, as well as the Playback sequence
with External Boost, where the driver does not correctly follow its
enable sequence for this mode. This can cause additional issues such as
pops and clicks.

This chain intends to correct the sequences for playback and system
suspend/resume so that the driver: obeys the external boost enable sequence;
resumes audio on system resume; and avoids the race condition on firmware
load and playback during system resume.

Changes since v1:
- Split patch 1 into 2 separate patches
- Combine Patches 6 and 9

Stefan Binding (11):
  ALSA: cs35l41: Use mbox command to enable speaker output for external
    boost
  ALSA: cs35l41: Poll for Power Up/Down rather than waiting a fixed
    delay
  ALSA: hda: cs35l41: Check mailbox status of pause command after
    firmware load
  ALSA: hda: cs35l41: Ensure we correctly re-sync regmap before system
    suspending.
  ALSA: hda: cs35l41: Ensure we pass up any errors during system
    suspend.
  ALSA: hda: cs35l41: Move Play and Pause into separate functions
  ALSA: hda: hda_component: Add pre and post playback hooks to
    hda_component
  ALSA: hda: cs35l41: Use pre and post playback hooks
  ALSA: hda: cs35l41: Rework System Suspend to ensure correct call
    separation
  ALSA: hda: cs35l41: Add device_link between HDA and cs35l41_hda
  ALSA: hda: cs35l41: Ensure amp is only unmuted during playback

 include/sound/cs35l41.h        |   5 +-
 sound/pci/hda/cs35l41_hda.c    | 288 +++++++++++++++++++++++++--------
 sound/pci/hda/hda_component.h  |   2 +
 sound/pci/hda/patch_realtek.c  |  10 +-
 sound/soc/codecs/cs35l41-lib.c | 118 ++++++++++++--
 sound/soc/codecs/cs35l41.c     |  18 +--
 6 files changed, 343 insertions(+), 98 deletions(-)

-- 
2.34.1

[PATCH v2 01/11] ALSA: cs35l41: Use mbox command to enable speaker output for external boost

[Stefan Binding]

To enable the speaker output in external boost mode, 2 registers must
be set, one after another. The longer the time between the writes of
the two registers, the more likely, and more loudly a pop may occur.
To minimize this, an mbox command can be used to allow the firmware
to perform this action, minimizing any delay between write, thus
minimizing any pop or click as a result. The old method will remain
when running without firmware.

Acked-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com>
---
 include/sound/cs35l41.h        |  5 ++-
 sound/pci/hda/cs35l41_hda.c    |  9 ++--
 sound/soc/codecs/cs35l41-lib.c | 76 +++++++++++++++++++++++++++-------
 sound/soc/codecs/cs35l41.c     |  8 ++--
 4 files changed, 74 insertions(+), 24 deletions(-)

diff --git a/include/sound/cs35l41.h b/include/sound/cs35l41.h
index 7239d943942cb..1bf757901d024 100644
--- a/include/sound/cs35l41.h
+++ b/include/sound/cs35l41.h
@@ -829,6 +829,7 @@ enum cs35l41_cspl_mbox_cmd {
 	CSPL_MBOX_CMD_STOP_PRE_REINIT = 4,
 	CSPL_MBOX_CMD_HIBERNATE = 5,
 	CSPL_MBOX_CMD_OUT_OF_HIBERNATE = 6,
+	CSPL_MBOX_CMD_SPK_OUT_ENABLE = 7,
 	CSPL_MBOX_CMD_UNKNOWN_CMD = -1,
 	CSPL_MBOX_CMD_INVALID_SEQUENCE = -2,
 };
@@ -901,7 +902,7 @@ int cs35l41_exit_hibernate(struct device *dev, struct regmap *regmap);
 int cs35l41_init_boost(struct device *dev, struct regmap *regmap,
 		       struct cs35l41_hw_cfg *hw_cfg);
 bool cs35l41_safe_reset(struct regmap *regmap, enum cs35l41_boost_type b_type);
-int cs35l41_global_enable(struct regmap *regmap, enum cs35l41_boost_type b_type, int enable,
-			  struct completion *pll_lock);
+int cs35l41_global_enable(struct device *dev, struct regmap *regmap, enum cs35l41_boost_type b_type,
+			  int enable, struct completion *pll_lock, bool firmware_running);
 
 #endif /* __CS35L41_H */
diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c
index ce5faa6205170..f9c97270db6f6 100644
--- a/sound/pci/hda/cs35l41_hda.c
+++ b/sound/pci/hda/cs35l41_hda.c
@@ -514,13 +514,15 @@ static void cs35l41_hda_playback_hook(struct device *dev, int action)
 		break;
 	case HDA_GEN_PCM_ACT_PREPARE:
 		mutex_lock(&cs35l41->fw_mutex);
-		ret = cs35l41_global_enable(reg, cs35l41->hw_cfg.bst_type, 1, NULL);
+		ret = cs35l41_global_enable(dev, reg, cs35l41->hw_cfg.bst_type, 1, NULL,
+					    cs35l41->firmware_running);
 		mutex_unlock(&cs35l41->fw_mutex);
 		break;
 	case HDA_GEN_PCM_ACT_CLEANUP:
 		mutex_lock(&cs35l41->fw_mutex);
 		regmap_multi_reg_write(reg, cs35l41_hda_mute, ARRAY_SIZE(cs35l41_hda_mute));
-		ret = cs35l41_global_enable(reg, cs35l41->hw_cfg.bst_type, 0, NULL);
+		ret = cs35l41_global_enable(dev, reg, cs35l41->hw_cfg.bst_type, 0, NULL,
+					    cs35l41->firmware_running);
 		mutex_unlock(&cs35l41->fw_mutex);
 		break;
 	case HDA_GEN_PCM_ACT_CLOSE:
@@ -672,7 +674,8 @@ static int cs35l41_runtime_suspend(struct device *dev)
 	if (cs35l41->playback_started) {
 		regmap_multi_reg_write(cs35l41->regmap, cs35l41_hda_mute,
 				       ARRAY_SIZE(cs35l41_hda_mute));
-		cs35l41_global_enable(cs35l41->regmap, cs35l41->hw_cfg.bst_type, 0, NULL);
+		cs35l41_global_enable(cs35l41->dev, cs35l41->regmap, cs35l41->hw_cfg.bst_type, 0,
+				      NULL, cs35l41->firmware_running);
 		regmap_update_bits(cs35l41->regmap, CS35L41_PWR_CTRL2,
 				   CS35L41_AMP_EN_MASK, 0 << CS35L41_AMP_EN_SHIFT);
 		if (cs35l41->hw_cfg.bst_type == CS35L41_EXT_BOOST)
diff --git a/sound/soc/codecs/cs35l41-lib.c b/sound/soc/codecs/cs35l41-lib.c
index ac7cc492bcb05..ce3f475023756 100644
--- a/sound/soc/codecs/cs35l41-lib.c
+++ b/sound/soc/codecs/cs35l41-lib.c
@@ -1080,28 +1080,32 @@ static const struct reg_sequence cs35l41_safe_to_reset[] = {
 	{ 0x00000040,			0x00000033 },
 };
 
-static const struct reg_sequence cs35l41_active_to_safe[] = {
+static const struct reg_sequence cs35l41_active_to_safe_start[] = {
 	{ 0x00000040,			0x00000055 },
 	{ 0x00000040,			0x000000AA },
 	{ 0x00007438,			0x00585941 },
 	{ CS35L41_PWR_CTRL1,		0x00000000 },
-	{ 0x0000742C,			0x00000009, 3000 },
+	{ 0x0000742C,			0x00000009 },
+};
+
+static const struct reg_sequence cs35l41_active_to_safe_end[] = {
 	{ 0x00007438,			0x00580941 },
 	{ 0x00000040,			0x000000CC },
 	{ 0x00000040,			0x00000033 },
 };
 
-static const struct reg_sequence cs35l41_safe_to_active[] = {
+static const struct reg_sequence cs35l41_safe_to_active_start[] = {
 	{ 0x00000040,			0x00000055 },
 	{ 0x00000040,			0x000000AA },
 	{ 0x0000742C,			0x0000000F },
 	{ 0x0000742C,			0x00000079 },
 	{ 0x00007438,			0x00585941 },
-	{ CS35L41_PWR_CTRL1,		0x00000001, 3000 }, // GLOBAL_EN = 1
+	{ CS35L41_PWR_CTRL1,		0x00000001 }, // GLOBAL_EN = 1
+};
+
+static const struct reg_sequence cs35l41_safe_to_active_en_spk[] = {
 	{ 0x0000742C,			0x000000F9 },
 	{ 0x00007438,			0x00580941 },
-	{ 0x00000040,			0x000000CC },
-	{ 0x00000040,			0x00000033 },
 };
 
 static const struct reg_sequence cs35l41_reset_to_safe[] = {
@@ -1188,11 +1192,11 @@ bool cs35l41_safe_reset(struct regmap *regmap, enum cs35l41_boost_type b_type)
 }
 EXPORT_SYMBOL_GPL(cs35l41_safe_reset);
 
-int cs35l41_global_enable(struct regmap *regmap, enum cs35l41_boost_type b_type, int enable,
-			  struct completion *pll_lock)
+int cs35l41_global_enable(struct device *dev, struct regmap *regmap, enum cs35l41_boost_type b_type,
+			  int enable, struct completion *pll_lock, bool firmware_running)
 {
 	int ret;
-	unsigned int gpio1_func, pad_control, pwr_ctrl1, pwr_ctrl3;
+	unsigned int gpio1_func, pad_control, pwr_ctrl1, pwr_ctrl3, int_status;
 	struct reg_sequence cs35l41_mdsync_down_seq[] = {
 		{CS35L41_PWR_CTRL3,		0},
 		{CS35L41_GPIO_PAD_CONTROL,	0},
@@ -1204,6 +1208,14 @@ int cs35l41_global_enable(struct regmap *regmap, enum cs35l41_boost_type b_type,
 		{CS35L41_PWR_CTRL1,	0x00000001, 3000},
 	};
 
+	if ((pwr_ctl1_val & CS35L41_GLOBAL_EN_MASK) && enable) {
+		dev_dbg(dev, "Cannot set Global Enable - already set.\n");
+		return 0;
+	} else if (!(pwr_ctl1_val & CS35L41_GLOBAL_EN_MASK) && !enable) {
+		dev_dbg(dev, "Cannot unset Global Enable - not set.\n");
+		return 0;
+	}
+
 	switch (b_type) {
 	case CS35L41_SHD_BOOST_ACTV:
 	case CS35L41_SHD_BOOST_PASS:
@@ -1244,16 +1256,48 @@ int cs35l41_global_enable(struct regmap *regmap, enum cs35l41_boost_type b_type,
 	case CS35L41_INT_BOOST:
 		ret = regmap_update_bits(regmap, CS35L41_PWR_CTRL1, CS35L41_GLOBAL_EN_MASK,
 					 enable << CS35L41_GLOBAL_EN_SHIFT);
+		if (ret) {
+			dev_err(dev, "CS35L41_PWR_CTRL1 set failed: %d\n", ret);
+			return ret;
+		}
 		usleep_range(3000, 3100);
 		break;
 	case CS35L41_EXT_BOOST:
 	case CS35L41_EXT_BOOST_NO_VSPK_SWITCH:
-		if (enable)
-			ret = regmap_multi_reg_write(regmap, cs35l41_safe_to_active,
-						     ARRAY_SIZE(cs35l41_safe_to_active));
-		else
-			ret = regmap_multi_reg_write(regmap, cs35l41_active_to_safe,
-						     ARRAY_SIZE(cs35l41_active_to_safe));
+		if (enable) {
+			/* Test Key is unlocked here */
+			ret = regmap_multi_reg_write(regmap, cs35l41_safe_to_active_start,
+						     ARRAY_SIZE(cs35l41_safe_to_active_start));
+			if (ret)
+				return ret;
+
+			usleep_range(3000, 3100);
+
+			if (firmware_running)
+				ret = cs35l41_set_cspl_mbox_cmd(dev, regmap,
+								CSPL_MBOX_CMD_SPK_OUT_ENABLE);
+			else
+				ret = regmap_multi_reg_write(regmap, cs35l41_safe_to_active_en_spk,
+							ARRAY_SIZE(cs35l41_safe_to_active_en_spk));
+
+			/* Lock the test key, it was unlocked during the multi_reg_write */
+			cs35l41_test_key_lock(dev, regmap);
+		} else {
+			/* Test Key is unlocked here */
+			ret = regmap_multi_reg_write(regmap, cs35l41_active_to_safe_start,
+						     ARRAY_SIZE(cs35l41_active_to_safe_start));
+			if (ret) {
+				/* Lock the test key, it was unlocked during the multi_reg_write */
+				cs35l41_test_key_lock(dev, regmap);
+				return ret;
+			}
+
+			usleep_range(3000, 3100);
+
+			/* Test Key is locked here */
+			ret = regmap_multi_reg_write(regmap, cs35l41_active_to_safe_end,
+						     ARRAY_SIZE(cs35l41_active_to_safe_end));
+		}
 		break;
 	default:
 		ret = -EINVAL;
@@ -1344,6 +1388,8 @@ static bool cs35l41_check_cspl_mbox_sts(enum cs35l41_cspl_mbox_cmd cmd,
 		return (sts == CSPL_MBOX_STS_RUNNING);
 	case CSPL_MBOX_CMD_STOP_PRE_REINIT:
 		return (sts == CSPL_MBOX_STS_RDY_FOR_REINIT);
+	case CSPL_MBOX_CMD_SPK_OUT_ENABLE:
+		return (sts == CSPL_MBOX_STS_RUNNING);
 	default:
 		return false;
 	}
diff --git a/sound/soc/codecs/cs35l41.c b/sound/soc/codecs/cs35l41.c
index 6ac501f008eca..d4e9c9d9b50a6 100644
--- a/sound/soc/codecs/cs35l41.c
+++ b/sound/soc/codecs/cs35l41.c
@@ -500,12 +500,12 @@ static int cs35l41_main_amp_event(struct snd_soc_dapm_widget *w,
 						cs35l41_pup_patch,
 						ARRAY_SIZE(cs35l41_pup_patch));
 
-		cs35l41_global_enable(cs35l41->regmap, cs35l41->hw_cfg.bst_type, 1,
-				      &cs35l41->pll_lock);
+		ret = cs35l41_global_enable(cs35l41->dev, cs35l41->regmap, cs35l41->hw_cfg.bst_type,
+					    1, &cs35l41->pll_lock, cs35l41->dsp.cs_dsp.running);
 		break;
 	case SND_SOC_DAPM_POST_PMD:
-		cs35l41_global_enable(cs35l41->regmap, cs35l41->hw_cfg.bst_type, 0,
-				      &cs35l41->pll_lock);
+		ret = cs35l41_global_enable(cs35l41->dev, cs35l41->regmap, cs35l41->hw_cfg.bst_type,
+					    0, &cs35l41->pll_lock, cs35l41->dsp.cs_dsp.running);
 
 		ret = regmap_read_poll_timeout(cs35l41->regmap, CS35L41_IRQ1_STATUS1,
 					       val, val &  CS35L41_PDN_DONE_MASK,
-- 
2.34.1

[PATCH v2 02/11] ALSA: cs35l41: Poll for Power Up/Down rather than waiting a fixed delay

[Stefan Binding]

To ensure the chip has correctly powered up or down before continuing,
the driver will now poll a register, rather than wait a fixed delay.

Acked-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com>
---
 sound/soc/codecs/cs35l41-lib.c | 48 +++++++++++++++++++++++++++++++---
 sound/soc/codecs/cs35l41.c     | 10 -------
 2 files changed, 44 insertions(+), 14 deletions(-)

diff --git a/sound/soc/codecs/cs35l41-lib.c b/sound/soc/codecs/cs35l41-lib.c
index ce3f475023756..4ec306cd2f476 100644
--- a/sound/soc/codecs/cs35l41-lib.c
+++ b/sound/soc/codecs/cs35l41-lib.c
@@ -1196,7 +1196,8 @@ int cs35l41_global_enable(struct device *dev, struct regmap *regmap, enum cs35l4
 			  int enable, struct completion *pll_lock, bool firmware_running)
 {
 	int ret;
-	unsigned int gpio1_func, pad_control, pwr_ctrl1, pwr_ctrl3, int_status;
+	unsigned int gpio1_func, pad_control, pwr_ctrl1, pwr_ctrl3, int_status, pup_pdn_mask;
+	unsigned int pwr_ctl1_val;
 	struct reg_sequence cs35l41_mdsync_down_seq[] = {
 		{CS35L41_PWR_CTRL3,		0},
 		{CS35L41_GPIO_PAD_CONTROL,	0},
@@ -1208,6 +1209,12 @@ int cs35l41_global_enable(struct device *dev, struct regmap *regmap, enum cs35l4
 		{CS35L41_PWR_CTRL1,	0x00000001, 3000},
 	};
 
+	pup_pdn_mask = enable ? CS35L41_PUP_DONE_MASK : CS35L41_PDN_DONE_MASK;
+
+	ret = regmap_read(regmap, CS35L41_PWR_CTRL1, &pwr_ctl1_val);
+	if (ret)
+		return ret;
+
 	if ((pwr_ctl1_val & CS35L41_GLOBAL_EN_MASK) && enable) {
 		dev_dbg(dev, "Cannot set Global Enable - already set.\n");
 		return 0;
@@ -1252,6 +1259,15 @@ int cs35l41_global_enable(struct device *dev, struct regmap *regmap, enum cs35l4
 			ret = regmap_multi_reg_write(regmap, cs35l41_mdsync_up_seq,
 						     ARRAY_SIZE(cs35l41_mdsync_up_seq));
 		}
+
+		ret = regmap_read_poll_timeout(regmap, CS35L41_IRQ1_STATUS1,
+					int_status, int_status & pup_pdn_mask,
+					1000, 100000);
+		if (ret)
+			dev_err(dev, "Enable(%d) failed: %d\n", enable, ret);
+
+		// Clear PUP/PDN status
+		regmap_write(regmap, CS35L41_IRQ1_STATUS1, pup_pdn_mask);
 		break;
 	case CS35L41_INT_BOOST:
 		ret = regmap_update_bits(regmap, CS35L41_PWR_CTRL1, CS35L41_GLOBAL_EN_MASK,
@@ -1260,7 +1276,15 @@ int cs35l41_global_enable(struct device *dev, struct regmap *regmap, enum cs35l4
 			dev_err(dev, "CS35L41_PWR_CTRL1 set failed: %d\n", ret);
 			return ret;
 		}
-		usleep_range(3000, 3100);
+
+		ret = regmap_read_poll_timeout(regmap, CS35L41_IRQ1_STATUS1,
+					int_status, int_status & pup_pdn_mask,
+					1000, 100000);
+		if (ret)
+			dev_err(dev, "Enable(%d) failed: %d\n", enable, ret);
+
+		/* Clear PUP/PDN status */
+		regmap_write(regmap, CS35L41_IRQ1_STATUS1, pup_pdn_mask);
 		break;
 	case CS35L41_EXT_BOOST:
 	case CS35L41_EXT_BOOST_NO_VSPK_SWITCH:
@@ -1271,7 +1295,15 @@ int cs35l41_global_enable(struct device *dev, struct regmap *regmap, enum cs35l4
 			if (ret)
 				return ret;
 
-			usleep_range(3000, 3100);
+			ret = regmap_read_poll_timeout(regmap, CS35L41_IRQ1_STATUS1, int_status,
+				       int_status & CS35L41_PUP_DONE_MASK, 1000, 100000);
+			if (ret) {
+				dev_err(dev, "Failed waiting for CS35L41_PUP_DONE_MASK: %d\n", ret);
+				/* Lock the test key, it was unlocked during the multi_reg_write */
+				cs35l41_test_key_lock(dev, regmap);
+				return ret;
+			}
+			regmap_write(regmap, CS35L41_IRQ1_STATUS1, CS35L41_PUP_DONE_MASK);
 
 			if (firmware_running)
 				ret = cs35l41_set_cspl_mbox_cmd(dev, regmap,
@@ -1292,7 +1324,15 @@ int cs35l41_global_enable(struct device *dev, struct regmap *regmap, enum cs35l4
 				return ret;
 			}
 
-			usleep_range(3000, 3100);
+			ret = regmap_read_poll_timeout(regmap, CS35L41_IRQ1_STATUS1, int_status,
+				       int_status & CS35L41_PDN_DONE_MASK, 1000, 100000);
+			if (ret) {
+				dev_err(dev, "Failed waiting for CS35L41_PDN_DONE_MASK: %d\n", ret);
+				/* Lock the test key, it was unlocked during the multi_reg_write */
+				cs35l41_test_key_lock(dev, regmap);
+				return ret;
+			}
+			regmap_write(regmap, CS35L41_IRQ1_STATUS1, CS35L41_PDN_DONE_MASK);
 
 			/* Test Key is locked here */
 			ret = regmap_multi_reg_write(regmap, cs35l41_active_to_safe_end,
diff --git a/sound/soc/codecs/cs35l41.c b/sound/soc/codecs/cs35l41.c
index d4e9c9d9b50a6..2b3c36f02edb0 100644
--- a/sound/soc/codecs/cs35l41.c
+++ b/sound/soc/codecs/cs35l41.c
@@ -491,7 +491,6 @@ static int cs35l41_main_amp_event(struct snd_soc_dapm_widget *w,
 {
 	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
 	struct cs35l41_private *cs35l41 = snd_soc_component_get_drvdata(component);
-	unsigned int val;
 	int ret = 0;
 
 	switch (event) {
@@ -507,15 +506,6 @@ static int cs35l41_main_amp_event(struct snd_soc_dapm_widget *w,
 		ret = cs35l41_global_enable(cs35l41->dev, cs35l41->regmap, cs35l41->hw_cfg.bst_type,
 					    0, &cs35l41->pll_lock, cs35l41->dsp.cs_dsp.running);
 
-		ret = regmap_read_poll_timeout(cs35l41->regmap, CS35L41_IRQ1_STATUS1,
-					       val, val &  CS35L41_PDN_DONE_MASK,
-					       1000, 100000);
-		if (ret)
-			dev_warn(cs35l41->dev, "PDN failed: %d\n", ret);
-
-		regmap_write(cs35l41->regmap, CS35L41_IRQ1_STATUS1,
-			     CS35L41_PDN_DONE_MASK);
-
 		regmap_multi_reg_write_bypassed(cs35l41->regmap,
 						cs35l41_pdn_patch,
 						ARRAY_SIZE(cs35l41_pdn_patch));
-- 
2.34.1

[PATCH v2 03/11] ALSA: hda: cs35l41: Check mailbox status of pause command after firmware load

[Stefan Binding]

Currently, we do not check the return status of the pause command,
immediately after we load firmware. If the pause has failed,
the firmware is not running.

Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com>
---
 sound/pci/hda/cs35l41_hda.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c
index f9c97270db6f6..29f1dce45f1dc 100644
--- a/sound/pci/hda/cs35l41_hda.c
+++ b/sound/pci/hda/cs35l41_hda.c
@@ -781,7 +781,12 @@ static int cs35l41_smart_amp(struct cs35l41_hda *cs35l41)
 		goto clean_dsp;
 	}
 
-	cs35l41_set_cspl_mbox_cmd(cs35l41->dev, cs35l41->regmap, CSPL_MBOX_CMD_PAUSE);
+	ret = cs35l41_set_cspl_mbox_cmd(cs35l41->dev, cs35l41->regmap, CSPL_MBOX_CMD_PAUSE);
+	if (ret) {
+		dev_err(cs35l41->dev, "Error waiting for DSP to pause: %u\n", ret);
+		goto clean_dsp;
+	}
+
 	cs35l41->firmware_running = true;
 
 	return 0;
-- 
2.34.1

[PATCH v2 04/11] ALSA: hda: cs35l41: Ensure we correctly re-sync regmap before system suspending.

[Stefan Binding]

In order to properly system suspend, it is necessary to unload the firmware
and ensure the chip is ready for shutdown (if necessary). If the system
is currently in runtime suspend, it is necessary to wake up the device,
and then make it ready. Currently, the wake does not correctly resync
the device, which may mean it cannot suspend correctly. Fix this by
performaing a resync.

Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com>
---
 sound/pci/hda/cs35l41_hda.c | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c
index 29f1dce45f1dc..f42457147ce47 100644
--- a/sound/pci/hda/cs35l41_hda.c
+++ b/sound/pci/hda/cs35l41_hda.c
@@ -574,21 +574,43 @@ static int cs35l41_hda_channel_map(struct device *dev, unsigned int tx_num, unsi
 				    rx_slot);
 }
 
-static void cs35l41_ready_for_reset(struct cs35l41_hda *cs35l41)
+static int cs35l41_ready_for_reset(struct cs35l41_hda *cs35l41)
 {
+	int ret = 0;
+
 	mutex_lock(&cs35l41->fw_mutex);
 	if (cs35l41->firmware_running) {
 
 		regcache_cache_only(cs35l41->regmap, false);
 
-		cs35l41_exit_hibernate(cs35l41->dev, cs35l41->regmap);
+		ret = cs35l41_exit_hibernate(cs35l41->dev, cs35l41->regmap);
+		if (ret) {
+			dev_warn(cs35l41->dev, "Unable to exit Hibernate.");
+			goto err;
+		}
+
+		/* Test key needs to be unlocked to allow the OTP settings to re-apply */
+		cs35l41_test_key_unlock(cs35l41->dev, cs35l41->regmap);
+		ret = regcache_sync(cs35l41->regmap);
+		cs35l41_test_key_lock(cs35l41->dev, cs35l41->regmap);
+		if (ret) {
+			dev_err(cs35l41->dev, "Failed to restore register cache: %d\n", ret);
+			goto err;
+		}
+
+		if (cs35l41->hw_cfg.bst_type == CS35L41_EXT_BOOST)
+			cs35l41_init_boost(cs35l41->dev, cs35l41->regmap, &cs35l41->hw_cfg);
+
 		cs35l41_shutdown_dsp(cs35l41);
 		cs35l41_safe_reset(cs35l41->regmap, cs35l41->hw_cfg.bst_type);
-
-		regcache_cache_only(cs35l41->regmap, true);
-		regcache_mark_dirty(cs35l41->regmap);
 	}
+err:
+	regcache_cache_only(cs35l41->regmap, true);
+	regcache_mark_dirty(cs35l41->regmap);
+
 	mutex_unlock(&cs35l41->fw_mutex);
+
+	return ret;
 }
 
 static int cs35l41_system_suspend(struct device *dev)
-- 
2.34.1

[PATCH v2 05/11] ALSA: hda: cs35l41: Ensure we pass up any errors during system suspend.

[Stefan Binding]

There are several steps required to put the system into system suspend.
Some of these steps may fail, so the driver should pass up the errors
if they occur.

Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com>
---
 sound/pci/hda/cs35l41_hda.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c
index f42457147ce47..d4a11f7b5dbd1 100644
--- a/sound/pci/hda/cs35l41_hda.c
+++ b/sound/pci/hda/cs35l41_hda.c
@@ -626,17 +626,22 @@ static int cs35l41_system_suspend(struct device *dev)
 	}
 
 	ret = pm_runtime_force_suspend(dev);
-	if (ret)
+	if (ret) {
+		dev_err(dev, "System Suspend Failed, unable to runtime suspend: %d\n", ret);
 		return ret;
+	}
 
 	/* Shutdown DSP before system suspend */
-	cs35l41_ready_for_reset(cs35l41);
+	ret = cs35l41_ready_for_reset(cs35l41);
+
+	if (ret)
+		dev_err(dev, "System Suspend Failed, not ready for Reset: %d\n", ret);
 
 	/*
 	 * Reset GPIO may be shared, so cannot reset here.
 	 * However beyond this point, amps may be powered down.
 	 */
-	return 0;
+	return ret;
 }
 
 static int cs35l41_system_resume(struct device *dev)
@@ -659,9 +664,13 @@ static int cs35l41_system_resume(struct device *dev)
 	usleep_range(2000, 2100);
 
 	ret = pm_runtime_force_resume(dev);
+	if (ret) {
+		dev_err(dev, "System Resume Failed: Unable to runtime resume: %d\n", ret);
+		return ret;
+	}
 
 	mutex_lock(&cs35l41->fw_mutex);
-	if (!ret && cs35l41->request_fw_load && !cs35l41->fw_request_ongoing) {
+	if (cs35l41->request_fw_load && !cs35l41->fw_request_ongoing) {
 		cs35l41->fw_request_ongoing = true;
 		schedule_work(&cs35l41->fw_load_work);
 	}
-- 
2.34.1

[PATCH v2 06/11] ALSA: hda: cs35l41: Move Play and Pause into separate functions

[Stefan Binding]

This allows play and pause to be called from multiple places,
which is necessary for system suspend and resume.

Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com>
---
 sound/pci/hda/cs35l41_hda.c | 131 ++++++++++++++++++++++--------------
 1 file changed, 79 insertions(+), 52 deletions(-)

diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c
index d4a11f7b5dbd1..f77583b46b6b0 100644
--- a/sound/pci/hda/cs35l41_hda.c
+++ b/sound/pci/hda/cs35l41_hda.c
@@ -483,63 +483,103 @@ static void cs35l41_irq_release(struct cs35l41_hda *cs35l41)
 	cs35l41->irq_errors = 0;
 }
 
-static void cs35l41_hda_playback_hook(struct device *dev, int action)
+static void cs35l41_hda_play_start(struct device *dev)
 {
 	struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev);
 	struct regmap *reg = cs35l41->regmap;
-	int ret = 0;
+
+	dev_dbg(dev, "Play (Start)\n");
+
+	if (cs35l41->playback_started) {
+		dev_dbg(dev, "Playback already started.");
+		return;
+	}
+
+	cs35l41->playback_started = true;
+
+	if (cs35l41->firmware_running) {
+		regmap_multi_reg_write(reg, cs35l41_hda_config_dsp,
+				       ARRAY_SIZE(cs35l41_hda_config_dsp));
+		regmap_update_bits(reg, CS35L41_PWR_CTRL2,
+				   CS35L41_VMON_EN_MASK | CS35L41_IMON_EN_MASK,
+				   1 << CS35L41_VMON_EN_SHIFT | 1 << CS35L41_IMON_EN_SHIFT);
+		cs35l41_set_cspl_mbox_cmd(cs35l41->dev, reg, CSPL_MBOX_CMD_RESUME);
+	} else {
+		regmap_multi_reg_write(reg, cs35l41_hda_config, ARRAY_SIZE(cs35l41_hda_config));
+	}
+	regmap_update_bits(reg, CS35L41_PWR_CTRL2, CS35L41_AMP_EN_MASK, 1 << CS35L41_AMP_EN_SHIFT);
+	if (cs35l41->hw_cfg.bst_type == CS35L41_EXT_BOOST)
+		regmap_write(reg, CS35L41_GPIO1_CTRL1, 0x00008001);
+
+}
+
+static void cs35l41_hda_play_done(struct device *dev)
+{
+	struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev);
+	struct regmap *reg = cs35l41->regmap;
+
+	dev_dbg(dev, "Play (Complete)\n");
+
+	cs35l41_global_enable(dev, reg, cs35l41->hw_cfg.bst_type, 1, NULL,
+			      cs35l41->firmware_running);
+}
+
+static void cs35l41_hda_pause_start(struct device *dev)
+{
+	struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev);
+	struct regmap *reg = cs35l41->regmap;
+
+	dev_dbg(dev, "Pause (Start)\n");
+
+	regmap_multi_reg_write(reg, cs35l41_hda_mute, ARRAY_SIZE(cs35l41_hda_mute));
+	cs35l41_global_enable(dev, reg, cs35l41->hw_cfg.bst_type, 0, NULL,
+			      cs35l41->firmware_running);
+}
+
+static void cs35l41_hda_pause_done(struct device *dev)
+{
+	struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev);
+	struct regmap *reg = cs35l41->regmap;
+
+	dev_dbg(dev, "Pause (Complete)\n");
+
+	regmap_update_bits(reg, CS35L41_PWR_CTRL2, CS35L41_AMP_EN_MASK, 0 << CS35L41_AMP_EN_SHIFT);
+	if (cs35l41->hw_cfg.bst_type == CS35L41_EXT_BOOST)
+		regmap_write(reg, CS35L41_GPIO1_CTRL1, 0x00000001);
+	if (cs35l41->firmware_running) {
+		cs35l41_set_cspl_mbox_cmd(dev, reg, CSPL_MBOX_CMD_PAUSE);
+		regmap_update_bits(reg, CS35L41_PWR_CTRL2,
+				   CS35L41_VMON_EN_MASK | CS35L41_IMON_EN_MASK,
+				   0 << CS35L41_VMON_EN_SHIFT | 0 << CS35L41_IMON_EN_SHIFT);
+	}
+	cs35l41_irq_release(cs35l41);
+	cs35l41->playback_started = false;
+}
+
+static void cs35l41_hda_playback_hook(struct device *dev, int action)
+{
+	struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev);
 
 	switch (action) {
 	case HDA_GEN_PCM_ACT_OPEN:
 		pm_runtime_get_sync(dev);
 		mutex_lock(&cs35l41->fw_mutex);
-		cs35l41->playback_started = true;
-		if (cs35l41->firmware_running) {
-			regmap_multi_reg_write(reg, cs35l41_hda_config_dsp,
-					       ARRAY_SIZE(cs35l41_hda_config_dsp));
-			regmap_update_bits(cs35l41->regmap, CS35L41_PWR_CTRL2,
-					   CS35L41_VMON_EN_MASK | CS35L41_IMON_EN_MASK,
-					   1 << CS35L41_VMON_EN_SHIFT | 1 << CS35L41_IMON_EN_SHIFT);
-			cs35l41_set_cspl_mbox_cmd(cs35l41->dev, cs35l41->regmap,
-						  CSPL_MBOX_CMD_RESUME);
-		} else {
-			regmap_multi_reg_write(reg, cs35l41_hda_config,
-					       ARRAY_SIZE(cs35l41_hda_config));
-		}
-		ret = regmap_update_bits(reg, CS35L41_PWR_CTRL2,
-					 CS35L41_AMP_EN_MASK, 1 << CS35L41_AMP_EN_SHIFT);
-		if (cs35l41->hw_cfg.bst_type == CS35L41_EXT_BOOST)
-			regmap_write(reg, CS35L41_GPIO1_CTRL1, 0x00008001);
+		cs35l41_hda_play_start(dev);
 		mutex_unlock(&cs35l41->fw_mutex);
 		break;
 	case HDA_GEN_PCM_ACT_PREPARE:
 		mutex_lock(&cs35l41->fw_mutex);
-		ret = cs35l41_global_enable(dev, reg, cs35l41->hw_cfg.bst_type, 1, NULL,
-					    cs35l41->firmware_running);
+		cs35l41_hda_play_done(dev);
 		mutex_unlock(&cs35l41->fw_mutex);
 		break;
 	case HDA_GEN_PCM_ACT_CLEANUP:
 		mutex_lock(&cs35l41->fw_mutex);
-		regmap_multi_reg_write(reg, cs35l41_hda_mute, ARRAY_SIZE(cs35l41_hda_mute));
-		ret = cs35l41_global_enable(dev, reg, cs35l41->hw_cfg.bst_type, 0, NULL,
-					    cs35l41->firmware_running);
+		cs35l41_hda_pause_start(dev);
 		mutex_unlock(&cs35l41->fw_mutex);
 		break;
 	case HDA_GEN_PCM_ACT_CLOSE:
 		mutex_lock(&cs35l41->fw_mutex);
-		ret = regmap_update_bits(reg, CS35L41_PWR_CTRL2,
-					 CS35L41_AMP_EN_MASK, 0 << CS35L41_AMP_EN_SHIFT);
-		if (cs35l41->hw_cfg.bst_type == CS35L41_EXT_BOOST)
-			regmap_write(reg, CS35L41_GPIO1_CTRL1, 0x00000001);
-		if (cs35l41->firmware_running) {
-			cs35l41_set_cspl_mbox_cmd(cs35l41->dev, cs35l41->regmap,
-						  CSPL_MBOX_CMD_PAUSE);
-			regmap_update_bits(cs35l41->regmap, CS35L41_PWR_CTRL2,
-					   CS35L41_VMON_EN_MASK | CS35L41_IMON_EN_MASK,
-					   0 << CS35L41_VMON_EN_SHIFT | 0 << CS35L41_IMON_EN_SHIFT);
-		}
-		cs35l41_irq_release(cs35l41);
-		cs35l41->playback_started = false;
+		cs35l41_hda_pause_done(dev);
 		mutex_unlock(&cs35l41->fw_mutex);
 
 		pm_runtime_mark_last_busy(dev);
@@ -549,9 +589,6 @@ static void cs35l41_hda_playback_hook(struct device *dev, int action)
 		dev_warn(cs35l41->dev, "Playback action not supported: %d\n", action);
 		break;
 	}
-
-	if (ret)
-		dev_err(cs35l41->dev, "Regmap access fail: %d\n", ret);
 }
 
 static int cs35l41_hda_channel_map(struct device *dev, unsigned int tx_num, unsigned int *tx_slot,
@@ -703,18 +740,8 @@ static int cs35l41_runtime_suspend(struct device *dev)
 	mutex_lock(&cs35l41->fw_mutex);
 
 	if (cs35l41->playback_started) {
-		regmap_multi_reg_write(cs35l41->regmap, cs35l41_hda_mute,
-				       ARRAY_SIZE(cs35l41_hda_mute));
-		cs35l41_global_enable(cs35l41->dev, cs35l41->regmap, cs35l41->hw_cfg.bst_type, 0,
-				      NULL, cs35l41->firmware_running);
-		regmap_update_bits(cs35l41->regmap, CS35L41_PWR_CTRL2,
-				   CS35L41_AMP_EN_MASK, 0 << CS35L41_AMP_EN_SHIFT);
-		if (cs35l41->hw_cfg.bst_type == CS35L41_EXT_BOOST)
-			regmap_write(cs35l41->regmap, CS35L41_GPIO1_CTRL1, 0x00000001);
-		regmap_update_bits(cs35l41->regmap, CS35L41_PWR_CTRL2,
-				   CS35L41_VMON_EN_MASK | CS35L41_IMON_EN_MASK,
-				   0 << CS35L41_VMON_EN_SHIFT | 0 << CS35L41_IMON_EN_SHIFT);
-		cs35l41->playback_started = false;
+		cs35l41_hda_pause_start(dev);
+		cs35l41_hda_pause_done(dev);
 	}
 
 	if (cs35l41->firmware_running) {
-- 
2.34.1

[PATCH v2 07/11] ALSA: hda: hda_component: Add pre and post playback hooks to hda_component

[Stefan Binding]

These hooks can be used to add callbacks that would be run before and after
the main playback hooks. These hooks would be called for all amps, before
moving on to the next hook, i.e. pre_playback_hook would be called for
all amps, before the playback_hook is called for all amps, then finally
the post_playback_hook is called for all amps.

Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com>
---
 sound/pci/hda/hda_component.h |  2 ++
 sound/pci/hda/patch_realtek.c | 10 +++++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/sound/pci/hda/hda_component.h b/sound/pci/hda/hda_component.h
index 534e845b9cd1d..f170aec967c18 100644
--- a/sound/pci/hda/hda_component.h
+++ b/sound/pci/hda/hda_component.h
@@ -15,5 +15,7 @@ struct hda_component {
 	struct device *dev;
 	char name[HDA_MAX_NAME_SIZE];
 	struct hda_codec *codec;
+	void (*pre_playback_hook)(struct device *dev, int action);
 	void (*playback_hook)(struct device *dev, int action);
+	void (*post_playback_hook)(struct device *dev, int action);
 };
diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index 52ea7d757d6ad..a6585afb77071 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -6716,9 +6716,17 @@ static void comp_generic_playback_hook(struct hda_pcm_stream *hinfo, struct hda_
 	int i;
 
 	for (i = 0; i < HDA_MAX_COMPONENTS; i++) {
-		if (spec->comps[i].dev)
+		if (spec->comps[i].dev && spec->comps[i].pre_playback_hook)
+			spec->comps[i].pre_playback_hook(spec->comps[i].dev, action);
+	}
+	for (i = 0; i < HDA_MAX_COMPONENTS; i++) {
+		if (spec->comps[i].dev && spec->comps[i].playback_hook)
 			spec->comps[i].playback_hook(spec->comps[i].dev, action);
 	}
+	for (i = 0; i < HDA_MAX_COMPONENTS; i++) {
+		if (spec->comps[i].dev && spec->comps[i].post_playback_hook)
+			spec->comps[i].post_playback_hook(spec->comps[i].dev, action);
+	}
 }
 
 struct cs35l41_dev_name {
-- 
2.34.1

[PATCH v2 08/11] ALSA: hda: cs35l41: Use pre and post playback hooks

[Stefan Binding]

Use new hooks to ensure separation between play/pause actions,
as required by external boost.

External Boost on CS35L41 requires the amp to go through a
particular sequence of steps. One of these steps involes
the setting of a GPIO. This GPIO is connected to one or
more of the amps, and it may control the boost for all of
the amps. To ensure that the GPIO is set when it is safe
to do so, and to ensure that boost is ready for the rest of
the sequence to be able to continue, we must ensure that
the each part of the sequence is executed for each amp
before moving on to the next part of the sequence.

Some of the Play and Pause actions have moved from Open to
Prepare. This is because Open is not guaranteed to be called
again on system resume, whereas Prepare should.

Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com>
---
 sound/pci/hda/cs35l41_hda.c | 53 ++++++++++++++++++++++++++++++-------
 1 file changed, 43 insertions(+), 10 deletions(-)

diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c
index f77583b46b6b0..a482d4752b3f8 100644
--- a/sound/pci/hda/cs35l41_hda.c
+++ b/sound/pci/hda/cs35l41_hda.c
@@ -556,37 +556,68 @@ static void cs35l41_hda_pause_done(struct device *dev)
 	cs35l41->playback_started = false;
 }
 
+static void cs35l41_hda_pre_playback_hook(struct device *dev, int action)
+{
+	struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev);
+
+	switch (action) {
+	case HDA_GEN_PCM_ACT_CLEANUP:
+		mutex_lock(&cs35l41->fw_mutex);
+		cs35l41_hda_pause_start(dev);
+		mutex_unlock(&cs35l41->fw_mutex);
+		break;
+	default:
+		break;
+	}
+}
 static void cs35l41_hda_playback_hook(struct device *dev, int action)
 {
 	struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev);
 
 	switch (action) {
 	case HDA_GEN_PCM_ACT_OPEN:
+		/*
+		 * All amps must be resumed before we can start playing back.
+		 * This ensures, for external boost, that all amps are in AMP_SAFE mode.
+		 * Do this in HDA_GEN_PCM_ACT_OPEN, since this is run prior to any of the
+		 * other actions.
+		 */
 		pm_runtime_get_sync(dev);
-		mutex_lock(&cs35l41->fw_mutex);
-		cs35l41_hda_play_start(dev);
-		mutex_unlock(&cs35l41->fw_mutex);
 		break;
 	case HDA_GEN_PCM_ACT_PREPARE:
 		mutex_lock(&cs35l41->fw_mutex);
-		cs35l41_hda_play_done(dev);
+		cs35l41_hda_play_start(dev);
 		mutex_unlock(&cs35l41->fw_mutex);
 		break;
 	case HDA_GEN_PCM_ACT_CLEANUP:
 		mutex_lock(&cs35l41->fw_mutex);
-		cs35l41_hda_pause_start(dev);
+		cs35l41_hda_pause_done(dev);
 		mutex_unlock(&cs35l41->fw_mutex);
 		break;
 	case HDA_GEN_PCM_ACT_CLOSE:
-		mutex_lock(&cs35l41->fw_mutex);
-		cs35l41_hda_pause_done(dev);
-		mutex_unlock(&cs35l41->fw_mutex);
-
+		/*
+		 * Playback must be finished for all amps before we start runtime suspend.
+		 * This ensures no amps are playing back when we start putting them to sleep.
+		 */
 		pm_runtime_mark_last_busy(dev);
 		pm_runtime_put_autosuspend(dev);
 		break;
 	default:
-		dev_warn(cs35l41->dev, "Playback action not supported: %d\n", action);
+		break;
+	}
+}
+
+static void cs35l41_hda_post_playback_hook(struct device *dev, int action)
+{
+	struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev);
+
+	switch (action) {
+	case HDA_GEN_PCM_ACT_PREPARE:
+		mutex_lock(&cs35l41->fw_mutex);
+		cs35l41_hda_play_done(dev);
+		mutex_unlock(&cs35l41->fw_mutex);
+		break;
+	default:
 		break;
 	}
 }
@@ -1037,6 +1068,8 @@ static int cs35l41_hda_bind(struct device *dev, struct device *master, void *mas
 	ret = cs35l41_create_controls(cs35l41);
 
 	comps->playback_hook = cs35l41_hda_playback_hook;
+	comps->pre_playback_hook = cs35l41_hda_pre_playback_hook;
+	comps->post_playback_hook = cs35l41_hda_post_playback_hook;
 
 	mutex_unlock(&cs35l41->fw_mutex);
 
-- 
2.34.1

[PATCH v2 09/11] ALSA: hda: cs35l41: Rework System Suspend to ensure correct call separation

[Stefan Binding]

In order to correctly pause audio on suspend, amps using external boost
require parts of the pause sequence to be called for all amps before moving
on to the next steps.
For example, as part of pausing the audio, the VSPK GPIO must be disabled,
but since this GPIO is controlled by one amp, but controls the boost for
all amps, it is required to separate the calls.
During playback this is achieved by using the pre and post playback hooks,
however during system suspend, this is not possible, so to separate the
calls, we use both the .prepare and .suspend calls to pause the audio.

Currently, for this reason, we do not restart audio on system resume.
However, we can support this by relying on the playback hook to resume
playback after system suspend.

Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com>
---
 sound/pci/hda/cs35l41_hda.c | 40 ++++++++++++++++++++++++++++++++-----
 1 file changed, 35 insertions(+), 5 deletions(-)

diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c
index a482d4752b3f8..70aa819cfbd64 100644
--- a/sound/pci/hda/cs35l41_hda.c
+++ b/sound/pci/hda/cs35l41_hda.c
@@ -595,6 +595,15 @@ static void cs35l41_hda_playback_hook(struct device *dev, int action)
 		mutex_unlock(&cs35l41->fw_mutex);
 		break;
 	case HDA_GEN_PCM_ACT_CLOSE:
+		mutex_lock(&cs35l41->fw_mutex);
+		if (!cs35l41->firmware_running && cs35l41->request_fw_load &&
+		    !cs35l41->fw_request_ongoing) {
+			dev_info(dev, "Requesting Firmware Load after HDA_GEN_PCM_ACT_CLOSE\n");
+			cs35l41->fw_request_ongoing = true;
+			schedule_work(&cs35l41->fw_load_work);
+		}
+		mutex_unlock(&cs35l41->fw_mutex);
+
 		/*
 		 * Playback must be finished for all amps before we start runtime suspend.
 		 * This ensures no amps are playing back when we start putting them to sleep.
@@ -681,6 +690,25 @@ static int cs35l41_ready_for_reset(struct cs35l41_hda *cs35l41)
 	return ret;
 }
 
+static int cs35l41_system_suspend_prep(struct device *dev)
+{
+	struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev);
+
+	dev_dbg(cs35l41->dev, "System Suspend Prepare\n");
+
+	if (cs35l41->hw_cfg.bst_type == CS35L41_EXT_BOOST_NO_VSPK_SWITCH) {
+		dev_err_once(cs35l41->dev, "System Suspend not supported\n");
+		return 0; /* don't block the whole system suspend */
+	}
+
+	mutex_lock(&cs35l41->fw_mutex);
+	if (cs35l41->playback_started)
+		cs35l41_hda_pause_start(dev);
+	mutex_unlock(&cs35l41->fw_mutex);
+
+	return 0;
+}
+
 static int cs35l41_system_suspend(struct device *dev)
 {
 	struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev);
@@ -693,6 +721,11 @@ static int cs35l41_system_suspend(struct device *dev)
 		return 0; /* don't block the whole system suspend */
 	}
 
+	mutex_lock(&cs35l41->fw_mutex);
+	if (cs35l41->playback_started)
+		cs35l41_hda_pause_done(dev);
+	mutex_unlock(&cs35l41->fw_mutex);
+
 	ret = pm_runtime_force_suspend(dev);
 	if (ret) {
 		dev_err(dev, "System Suspend Failed, unable to runtime suspend: %d\n", ret);
@@ -738,6 +771,7 @@ static int cs35l41_system_resume(struct device *dev)
 	}
 
 	mutex_lock(&cs35l41->fw_mutex);
+
 	if (cs35l41->request_fw_load && !cs35l41->fw_request_ongoing) {
 		cs35l41->fw_request_ongoing = true;
 		schedule_work(&cs35l41->fw_load_work);
@@ -770,11 +804,6 @@ static int cs35l41_runtime_suspend(struct device *dev)
 
 	mutex_lock(&cs35l41->fw_mutex);
 
-	if (cs35l41->playback_started) {
-		cs35l41_hda_pause_start(dev);
-		cs35l41_hda_pause_done(dev);
-	}
-
 	if (cs35l41->firmware_running) {
 		ret = cs35l41_enter_hibernate(cs35l41->dev, cs35l41->regmap,
 					      cs35l41->hw_cfg.bst_type);
@@ -1641,6 +1670,7 @@ EXPORT_SYMBOL_NS_GPL(cs35l41_hda_remove, SND_HDA_SCODEC_CS35L41);
 const struct dev_pm_ops cs35l41_hda_pm_ops = {
 	RUNTIME_PM_OPS(cs35l41_runtime_suspend, cs35l41_runtime_resume,
 		       cs35l41_runtime_idle)
+	.prepare = cs35l41_system_suspend_prep,
 	SYSTEM_SLEEP_PM_OPS(cs35l41_system_suspend, cs35l41_system_resume)
 };
 EXPORT_SYMBOL_NS_GPL(cs35l41_hda_pm_ops, SND_HDA_SCODEC_CS35L41);
-- 
2.34.1

[PATCH v2 10/11] ALSA: hda: cs35l41: Add device_link between HDA and cs35l41_hda

[Stefan Binding]

To ensure consistency between the HDA core and the CS35L41 HDA
driver, add a device_link between them. This ensures that the
HDA core will suspend first, and resume second, meaning the
amp driver will not miss any events from the playback hook from
the HDA core during system suspend and resume.

Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com>
---
 sound/pci/hda/cs35l41_hda.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c
index 70aa819cfbd64..175378cdf9dfa 100644
--- a/sound/pci/hda/cs35l41_hda.c
+++ b/sound/pci/hda/cs35l41_hda.c
@@ -1063,6 +1063,7 @@ static int cs35l41_hda_bind(struct device *dev, struct device *master, void *mas
 {
 	struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev);
 	struct hda_component *comps = master_data;
+	unsigned int sleep_flags;
 	int ret = 0;
 
 	if (!comps || cs35l41->index < 0 || cs35l41->index >= HDA_MAX_COMPONENTS)
@@ -1102,6 +1103,11 @@ static int cs35l41_hda_bind(struct device *dev, struct device *master, void *mas
 
 	mutex_unlock(&cs35l41->fw_mutex);
 
+	sleep_flags = lock_system_sleep();
+	if (!device_link_add(&comps->codec->core.dev, cs35l41->dev, DL_FLAG_STATELESS))
+		dev_warn(dev, "Unable to create device link\n");
+	unlock_system_sleep(sleep_flags);
+
 	pm_runtime_mark_last_busy(dev);
 	pm_runtime_put_autosuspend(dev);
 
@@ -1112,9 +1118,14 @@ static void cs35l41_hda_unbind(struct device *dev, struct device *master, void *
 {
 	struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev);
 	struct hda_component *comps = master_data;
+	unsigned int sleep_flags;
 
-	if (comps[cs35l41->index].dev == dev)
+	if (comps[cs35l41->index].dev == dev) {
 		memset(&comps[cs35l41->index], 0, sizeof(*comps));
+		sleep_flags = lock_system_sleep();
+		device_link_remove(&comps->codec->core.dev, cs35l41->dev);
+		unlock_system_sleep(sleep_flags);
+	}
 }
 
 static const struct component_ops cs35l41_hda_comp_ops = {
-- 
2.34.1

[PATCH v2 11/11] ALSA: hda: cs35l41: Ensure amp is only unmuted during playback

[Stefan Binding]

Currently we only mute after playback has finished, and unmute
prior to setting global enable. To prevent any possible pops
and clicks, mute at probe, and then only unmute after global
enable is set.

Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com>
---
 sound/pci/hda/cs35l41_hda.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c
index 175378cdf9dfa..98feb5ccd5866 100644
--- a/sound/pci/hda/cs35l41_hda.c
+++ b/sound/pci/hda/cs35l41_hda.c
@@ -58,8 +58,6 @@ static const struct reg_sequence cs35l41_hda_config[] = {
 	{ CS35L41_DSP1_RX3_SRC,         0x00000018 }, // DSP1RX3 SRC = VMON
 	{ CS35L41_DSP1_RX4_SRC,         0x00000019 }, // DSP1RX4 SRC = IMON
 	{ CS35L41_DSP1_RX5_SRC,         0x00000020 }, // DSP1RX5 SRC = ERRVOL
-	{ CS35L41_AMP_DIG_VOL_CTRL,	0x00008000 }, // AMP_HPF_PCM_EN = 1, AMP_VOL_PCM  0.0 dB
-	{ CS35L41_AMP_GAIN_CTRL,	0x00000084 }, // AMP_GAIN_PCM 4.5 dB
 };
 
 static const struct reg_sequence cs35l41_hda_config_dsp[] = {
@@ -82,6 +80,14 @@ static const struct reg_sequence cs35l41_hda_config_dsp[] = {
 	{ CS35L41_DSP1_RX3_SRC,         0x00000018 }, // DSP1RX3 SRC = VMON
 	{ CS35L41_DSP1_RX4_SRC,         0x00000019 }, // DSP1RX4 SRC = IMON
 	{ CS35L41_DSP1_RX5_SRC,         0x00000029 }, // DSP1RX5 SRC = VBSTMON
+};
+
+static const struct reg_sequence cs35l41_hda_unmute[] = {
+	{ CS35L41_AMP_DIG_VOL_CTRL,	0x00008000 }, // AMP_HPF_PCM_EN = 1, AMP_VOL_PCM  0.0 dB
+	{ CS35L41_AMP_GAIN_CTRL,	0x00000084 }, // AMP_GAIN_PCM 4.5 dB
+};
+
+static const struct reg_sequence cs35l41_hda_unmute_dsp[] = {
 	{ CS35L41_AMP_DIG_VOL_CTRL,	0x00008000 }, // AMP_HPF_PCM_EN = 1, AMP_VOL_PCM  0.0 dB
 	{ CS35L41_AMP_GAIN_CTRL,	0x00000233 }, // AMP_GAIN_PCM = 17.5dB AMP_GAIN_PDM = 19.5dB
 };
@@ -522,6 +528,13 @@ static void cs35l41_hda_play_done(struct device *dev)
 
 	cs35l41_global_enable(dev, reg, cs35l41->hw_cfg.bst_type, 1, NULL,
 			      cs35l41->firmware_running);
+	if (cs35l41->firmware_running) {
+		regmap_multi_reg_write(reg, cs35l41_hda_unmute_dsp,
+				       ARRAY_SIZE(cs35l41_hda_unmute_dsp));
+	} else {
+		regmap_multi_reg_write(reg, cs35l41_hda_unmute,
+				       ARRAY_SIZE(cs35l41_hda_unmute));
+	}
 }
 
 static void cs35l41_hda_pause_start(struct device *dev)
@@ -1616,6 +1629,11 @@ int cs35l41_hda_probe(struct device *dev, const char *device_name, int id, int i
 	if (ret)
 		goto err;
 
+	ret = regmap_multi_reg_write(cs35l41->regmap, cs35l41_hda_mute,
+				     ARRAY_SIZE(cs35l41_hda_mute));
+	if (ret)
+		goto err;
+
 	INIT_WORK(&cs35l41->fw_load_work, cs35l41_fw_load_work);
 	mutex_init(&cs35l41->fw_mutex);
 
-- 
2.34.1

Re: [PATCH v2 00/11] Fix support for System Suspend for CS35L41 HDA

[Takashi Iwai]

On Fri, 21 Jul 2023 17:18:05 +0200,
Stefan Binding wrote:
> 
> There have been a couple of customer reports of intermittant issues after
> system resume, where sometimes the DSP firmware stops responding.
> Investigations into this issue show that there is a race between receiving
> a prepare from the HDA core, and the firmware reload which is started by
> the system resume. This can causes the Global Enable on the CS35L41 to be
> enabled during the firmware load, which can sometimes cause issues in the
> DSP.
> 
> The existing system resume behaviour also did not resume the audio, if
> audio was previously playing when it was suspended.
> In addition, during investigation, it was found there were additional
> problems in the System Resume sequence, as well as the Playback sequence
> with External Boost, where the driver does not correctly follow its
> enable sequence for this mode. This can cause additional issues such as
> pops and clicks.
> 
> This chain intends to correct the sequences for playback and system
> suspend/resume so that the driver: obeys the external boost enable sequence;
> resumes audio on system resume; and avoids the race condition on firmware
> load and playback during system resume.
> 
> Changes since v1:
> - Split patch 1 into 2 separate patches
> - Combine Patches 6 and 9
> 
> Stefan Binding (11):
>   ALSA: cs35l41: Use mbox command to enable speaker output for external
>     boost
>   ALSA: cs35l41: Poll for Power Up/Down rather than waiting a fixed
>     delay
>   ALSA: hda: cs35l41: Check mailbox status of pause command after
>     firmware load
>   ALSA: hda: cs35l41: Ensure we correctly re-sync regmap before system
>     suspending.
>   ALSA: hda: cs35l41: Ensure we pass up any errors during system
>     suspend.
>   ALSA: hda: cs35l41: Move Play and Pause into separate functions
>   ALSA: hda: hda_component: Add pre and post playback hooks to
>     hda_component
>   ALSA: hda: cs35l41: Use pre and post playback hooks
>   ALSA: hda: cs35l41: Rework System Suspend to ensure correct call
>     separation
>   ALSA: hda: cs35l41: Add device_link between HDA and cs35l41_hda
>   ALSA: hda: cs35l41: Ensure amp is only unmuted during playback

Applied all patches now to for-next branch.


thanks,

Takashi

Re: [PATCH v2 00/11] Fix support for System Suspend for CS35L41 HDA

[Takashi Iwai]

On Mon, 24 Jul 2023 11:00:38 +0200,
Takashi Iwai wrote:
> 
> On Fri, 21 Jul 2023 17:18:05 +0200,
> Stefan Binding wrote:
> > 
> > There have been a couple of customer reports of intermittant issues after
> > system resume, where sometimes the DSP firmware stops responding.
> > Investigations into this issue show that there is a race between receiving
> > a prepare from the HDA core, and the firmware reload which is started by
> > the system resume. This can causes the Global Enable on the CS35L41 to be
> > enabled during the firmware load, which can sometimes cause issues in the
> > DSP.
> > 
> > The existing system resume behaviour also did not resume the audio, if
> > audio was previously playing when it was suspended.
> > In addition, during investigation, it was found there were additional
> > problems in the System Resume sequence, as well as the Playback sequence
> > with External Boost, where the driver does not correctly follow its
> > enable sequence for this mode. This can cause additional issues such as
> > pops and clicks.
> > 
> > This chain intends to correct the sequences for playback and system
> > suspend/resume so that the driver: obeys the external boost enable sequence;
> > resumes audio on system resume; and avoids the race condition on firmware
> > load and playback during system resume.
> > 
> > Changes since v1:
> > - Split patch 1 into 2 separate patches
> > - Combine Patches 6 and 9
> > 
> > Stefan Binding (11):
> >   ALSA: cs35l41: Use mbox command to enable speaker output for external
> >     boost
> >   ALSA: cs35l41: Poll for Power Up/Down rather than waiting a fixed
> >     delay
> >   ALSA: hda: cs35l41: Check mailbox status of pause command after
> >     firmware load
> >   ALSA: hda: cs35l41: Ensure we correctly re-sync regmap before system
> >     suspending.
> >   ALSA: hda: cs35l41: Ensure we pass up any errors during system
> >     suspend.
> >   ALSA: hda: cs35l41: Move Play and Pause into separate functions
> >   ALSA: hda: hda_component: Add pre and post playback hooks to
> >     hda_component
> >   ALSA: hda: cs35l41: Use pre and post playback hooks
> >   ALSA: hda: cs35l41: Rework System Suspend to ensure correct call
> >     separation
> >   ALSA: hda: cs35l41: Add device_link between HDA and cs35l41_hda
> >   ALSA: hda: cs35l41: Ensure amp is only unmuted during playback
> 
> Applied all patches now to for-next branch.

It seems that this patch set causes occasional freeze at suspend:
  https://bugzilla.suse.com/show_bug.cgi?id=1213745

Could you take a look?


thanks,

Takashi

Re: [PATCH v2 00/11] Fix support for System Suspend for CS35L41 HDA

[Stefan Binding]

On 28/07/2023 12:10, Takashi Iwai wrote:
> On Mon, 24 Jul 2023 11:00:38 +0200,
> Takashi Iwai wrote:
>> On Fri, 21 Jul 2023 17:18:05 +0200,
>> Stefan Binding wrote:
>>> There have been a couple of customer reports of intermittant issues after
>>> system resume, where sometimes the DSP firmware stops responding.
>>> Investigations into this issue show that there is a race between receiving
>>> a prepare from the HDA core, and the firmware reload which is started by
>>> the system resume. This can causes the Global Enable on the CS35L41 to be
>>> enabled during the firmware load, which can sometimes cause issues in the
>>> DSP.
>>>
>>> The existing system resume behaviour also did not resume the audio, if
>>> audio was previously playing when it was suspended.
>>> In addition, during investigation, it was found there were additional
>>> problems in the System Resume sequence, as well as the Playback sequence
>>> with External Boost, where the driver does not correctly follow its
>>> enable sequence for this mode. This can cause additional issues such as
>>> pops and clicks.
>>>
>>> This chain intends to correct the sequences for playback and system
>>> suspend/resume so that the driver: obeys the external boost enable sequence;
>>> resumes audio on system resume; and avoids the race condition on firmware
>>> load and playback during system resume.
>>>
>>> Changes since v1:
>>> - Split patch 1 into 2 separate patches
>>> - Combine Patches 6 and 9
>>>
>>> Stefan Binding (11):
>>>    ALSA: cs35l41: Use mbox command to enable speaker output for external
>>>      boost
>>>    ALSA: cs35l41: Poll for Power Up/Down rather than waiting a fixed
>>>      delay
>>>    ALSA: hda: cs35l41: Check mailbox status of pause command after
>>>      firmware load
>>>    ALSA: hda: cs35l41: Ensure we correctly re-sync regmap before system
>>>      suspending.
>>>    ALSA: hda: cs35l41: Ensure we pass up any errors during system
>>>      suspend.
>>>    ALSA: hda: cs35l41: Move Play and Pause into separate functions
>>>    ALSA: hda: hda_component: Add pre and post playback hooks to
>>>      hda_component
>>>    ALSA: hda: cs35l41: Use pre and post playback hooks
>>>    ALSA: hda: cs35l41: Rework System Suspend to ensure correct call
>>>      separation
>>>    ALSA: hda: cs35l41: Add device_link between HDA and cs35l41_hda
>>>    ALSA: hda: cs35l41: Ensure amp is only unmuted during playback
>> Applied all patches now to for-next branch.
> It seems that this patch set causes occasional freeze at suspend:
>    https://bugzilla.suse.com/show_bug.cgi?id=1213745
>
> Could you take a look?
>
>
> thanks,
>
> Takashi

Hi Takashi,

The initial bug report shows one of the original issues that this patch 
chain was trying to fix.
 From what I can tell from the second issue, something has caused the 
CS35L41 to stop responding,
which in turn caused the system suspend call to fail, and the error is 
passed up. Since system suspend
failed, there was no corresponding system resume, which means the 
CS35L41 was stuck broken.

I'm not sure what was meant by "freeze" and "overheating" in the bug 
report, since the log seems to
indicate the laptop is still responsive, even if audio is broken.

There is some oddity in the log, since one of the errors that was 
printed should only be printed when
the CS35L41 is using External Boost, but I think this laptop is supposed 
to use Internal Boost.
We'll investigate further.

Thanks,

Stefan Binding

Re: [PATCH v2 00/11] Fix support for System Suspend for CS35L41 HDA

[Stefan Binding]

On 01/09/2023 17:48, Waldek Andrukiewicz wrote:
> On 28.07.23 15:55, Stefan Binding wrote:
>> On 28/07/2023 12:10, Takashi Iwai wrote:
>>> On Mon, 24 Jul 2023 11:00:38 +0200,
>>> Takashi Iwai wrote:
>>>> On Fri, 21 Jul 2023 17:18:05 +0200,
>>>> Stefan Binding wrote:
>>>>> There have been a couple of customer reports of intermittant issues
>>>>> after
>>>>> system resume, where sometimes the DSP firmware stops responding.
>>>>> Investigations into this issue show that there is a race between
>>>>> receiving
>>>>> a prepare from the HDA core, and the firmware reload which is
>>>>> started by
>>>>> the system resume. This can causes the Global Enable on the CS35L41
>>>>> to be
>>>>> enabled during the firmware load, which can sometimes cause issues
>>>>> in the
>>>>> DSP.
>>>>>
>>>>> The existing system resume behaviour also did not resume the audio, if
>>>>> audio was previously playing when it was suspended.
>>>>> In addition, during investigation, it was found there were additional
>>>>> problems in the System Resume sequence, as well as the Playback
>>>>> sequence
>>>>> with External Boost, where the driver does not correctly follow its
>>>>> enable sequence for this mode. This can cause additional issues
>>>>> such as
>>>>> pops and clicks.
>>>>>
>>>>> This chain intends to correct the sequences for playback and system
>>>>> suspend/resume so that the driver: obeys the external boost enable
>>>>> sequence;
>>>>> resumes audio on system resume; and avoids the race condition on
>>>>> firmware
>>>>> load and playback during system resume.
>>>>>
>>>>> Changes since v1:
>>>>> - Split patch 1 into 2 separate patches
>>>>> - Combine Patches 6 and 9
>>>>>
>>>>> Stefan Binding (11):
>>>>>     ALSA: cs35l41: Use mbox command to enable speaker output for
>>>>> external
>>>>>       boost
>>>>>     ALSA: cs35l41: Poll for Power Up/Down rather than waiting a fixed
>>>>>       delay
>>>>>     ALSA: hda: cs35l41: Check mailbox status of pause command after
>>>>>       firmware load
>>>>>     ALSA: hda: cs35l41: Ensure we correctly re-sync regmap before
>>>>> system
>>>>>       suspending.
>>>>>     ALSA: hda: cs35l41: Ensure we pass up any errors during system
>>>>>       suspend.
>>>>>     ALSA: hda: cs35l41: Move Play and Pause into separate functions
>>>>>     ALSA: hda: hda_component: Add pre and post playback hooks to
>>>>>       hda_component
>>>>>     ALSA: hda: cs35l41: Use pre and post playback hooks
>>>>>     ALSA: hda: cs35l41: Rework System Suspend to ensure correct call
>>>>>       separation
>>>>>     ALSA: hda: cs35l41: Add device_link between HDA and cs35l41_hda
>>>>>     ALSA: hda: cs35l41: Ensure amp is only unmuted during playback
>>>> Applied all patches now to for-next branch.
>>> It seems that this patch set causes occasional freeze at suspend:
>>>     https://bugzilla.suse.com/show_bug.cgi?id=1213745
>>>
>>> Could you take a look?
>>>
>>>
>>> thanks,
>>>
>>> Takashi
>> Hi Takashi,
>>
>> The initial bug report shows one of the original issues that this patch
>> chain was trying to fix.
>>  From what I can tell from the second issue, something has caused the
>> CS35L41 to stop responding,
>> which in turn caused the system suspend call to fail, and the error is
>> passed up. Since system suspend
>> failed, there was no corresponding system resume, which means the
>> CS35L41 was stuck broken.
>>
>> I'm not sure what was meant by "freeze" and "overheating" in the bug
>> report, since the log seems to
>> indicate the laptop is still responsive, even if audio is broken.
>>
>> There is some oddity in the log, since one of the errors that was
>> printed should only be printed when
>> the CS35L41 is using External Boost, but I think this laptop is supposed
>> to use Internal Boost.
>> We'll investigate further.
>>
>> Thanks,
>>
>> Stefan Binding
>
> Hello,
>
> I would like to inform you that (one of) those patches is probably
> breaking sound on Lenovo Legion 7 16ACHg6. They were applied in Manjaro
> Linux kernel here:
>
> https://gitlab.manjaro.org/packages/core/linux64/-/commit/742e66f525170fe02dec42e47aedf53d3dc85195
>
> and when I install this kernel, there is no sound anymore. There is
> nothing more than those patches in this commit. A kernel compiled from
> the previous commit works fine.
>
> Sound used to work fine from 5.17 if I remember correctly.
>
> I hope I replied correctly to this thread, if not, apologies.
>
> Best Regards,
>
> Waldek Andrukiewicz
>
>
Hi,

This is a different issue that we have been able to reproduce.
It only affects that particular laptop, and is unrelated to the other issue.
We are currently working on a fix for this.

Thanks,

Stefan

Re: [PATCH v2 00/11] Fix support for System Suspend for CS35L41 HDA

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 185 bytes --]

On Fri, Sep 01, 2023 at 04:48:21PM +0000, Waldek Andrukiewicz wrote:

[An encrypted message which probably nobody can read, it's not even
encrypted to me even though I was a recipient]

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 00/11] Fix support for System Suspend for CS35L41 HDA

[Waldek Andrukiewicz]

On 28.07.23 15:55, Stefan Binding wrote:
>
> On 28/07/2023 12:10, Takashi Iwai wrote:
>> On Mon, 24 Jul 2023 11:00:38 +0200,
>> Takashi Iwai wrote:
>>> On Fri, 21 Jul 2023 17:18:05 +0200,
>>> Stefan Binding wrote:
>>>> There have been a couple of customer reports of intermittant issues
>>>> after
>>>> system resume, where sometimes the DSP firmware stops responding.
>>>> Investigations into this issue show that there is a race between
>>>> receiving
>>>> a prepare from the HDA core, and the firmware reload which is
>>>> started by
>>>> the system resume. This can causes the Global Enable on the CS35L41
>>>> to be
>>>> enabled during the firmware load, which can sometimes cause issues
>>>> in the
>>>> DSP.
>>>>
>>>> The existing system resume behaviour also did not resume the audio, if
>>>> audio was previously playing when it was suspended.
>>>> In addition, during investigation, it was found there were additional
>>>> problems in the System Resume sequence, as well as the Playback
>>>> sequence
>>>> with External Boost, where the driver does not correctly follow its
>>>> enable sequence for this mode. This can cause additional issues
>>>> such as
>>>> pops and clicks.
>>>>
>>>> This chain intends to correct the sequences for playback and system
>>>> suspend/resume so that the driver: obeys the external boost enable
>>>> sequence;
>>>> resumes audio on system resume; and avoids the race condition on
>>>> firmware
>>>> load and playback during system resume.
>>>>
>>>> Changes since v1:
>>>> - Split patch 1 into 2 separate patches
>>>> - Combine Patches 6 and 9
>>>>
>>>> Stefan Binding (11):
>>>>    ALSA: cs35l41: Use mbox command to enable speaker output for
>>>> external
>>>>      boost
>>>>    ALSA: cs35l41: Poll for Power Up/Down rather than waiting a fixed
>>>>      delay
>>>>    ALSA: hda: cs35l41: Check mailbox status of pause command after
>>>>      firmware load
>>>>    ALSA: hda: cs35l41: Ensure we correctly re-sync regmap before
>>>> system
>>>>      suspending.
>>>>    ALSA: hda: cs35l41: Ensure we pass up any errors during system
>>>>      suspend.
>>>>    ALSA: hda: cs35l41: Move Play and Pause into separate functions
>>>>    ALSA: hda: hda_component: Add pre and post playback hooks to
>>>>      hda_component
>>>>    ALSA: hda: cs35l41: Use pre and post playback hooks
>>>>    ALSA: hda: cs35l41: Rework System Suspend to ensure correct call
>>>>      separation
>>>>    ALSA: hda: cs35l41: Add device_link between HDA and cs35l41_hda
>>>>    ALSA: hda: cs35l41: Ensure amp is only unmuted during playback
>>> Applied all patches now to for-next branch.
>> It seems that this patch set causes occasional freeze at suspend:
>>    https://bugzilla.suse.com/show_bug.cgi?id=1213745
>>
>> Could you take a look?
>>
>>
>> thanks,
>>
>> Takashi
>
> Hi Takashi,
>
> The initial bug report shows one of the original issues that this patch
> chain was trying to fix.
> From what I can tell from the second issue, something has caused the
> CS35L41 to stop responding,
> which in turn caused the system suspend call to fail, and the error is
> passed up. Since system suspend
> failed, there was no corresponding system resume, which means the
> CS35L41 was stuck broken.
>
> I'm not sure what was meant by "freeze" and "overheating" in the bug
> report, since the log seems to
> indicate the laptop is still responsive, even if audio is broken.
>
> There is some oddity in the log, since one of the errors that was
> printed should only be printed when
> the CS35L41 is using External Boost, but I think this laptop is supposed
> to use Internal Boost.
> We'll investigate further.
>
> Thanks,
>
> Stefan Binding


Hello,

I would like to inform you that (one of) those patches is probably
breaking sound on Lenovo Legion 7 16ACHg6. They were applied in Manjaro
Linux kernel here:

https://gitlab.manjaro.org/packages/core/linux64/-/commit/742e66f525170fe02dec42e47aedf53d3dc85195

and when I install this kernel, there is no sound anymore. There is
nothing more than those patches in this commit. A kernel compiled from
the previous commit works fine.

Sound used to work fine from 5.17 if I remember correctly.

I hope I replied correctly to this thread, if not, apologies.

Best Regards,

Waldek Andrukiewicz