[PATCH v1 0/8] System Suspend fixes and improvements for CS35L41 HDA

[PATCH v1 0/8] System Suspend fixes and improvements for CS35L41 HDA

[Stefan Binding]

There is a report of a single laptop which uses CS35L41 HDA having an
issue with System Suspend. This particular laptop uses S3 (Deep) Sleep.
The reported issue states that when the laptop resumes from a system
suspend, audio no longer works.

The root cause of this issue is due to the CS35L41 being returned to us
in an unexpected state after a suspend/resume cycle.
When the driver resumes, it expects the parts to have been reset, which
leads to issues with audio and firmware loading.

To prevent this issue, and the possibility of similar issues, patches
2-5 force the driver to reset during probe, system suspend, and system
resume, which ensures that the part is always in the correct state.
Patches 6-8 are improvements in the suspend and firmware loading code,
which makes it easier to detect issues in the future, as well as
simplifiying the suspend code.

Patch 1 is a fix for an incorrect configuration for the HP Zbook Fury
17, which is the laptop which had the original issue.

Stefan Binding (8):
  ALSA: hda: cs35l41: Use reset label to get GPIO for HP Zbook Fury 17
    G9
  ALSA: hda: cs35l41: Assert reset before system suspend
  ALSA: hda: cs35l41: Assert Reset prior to de-asserting in probe and
    system resume
  ALSA: hda: cs35l41: Run boot process during resume callbacks
  ALSA: hda: cs35l41: Force a software reset after hardware reset
  ALSA: hda: cs35l41: Do not unload firmware before reset in system
    suspend
  ALSA: hda: cs35l41: Check CSPL state after loading firmware
  ASoC: cs35l41: Detect CSPL errors when sending CSPL commands

 include/sound/cs35l41.h              |   3 +
 sound/pci/hda/cs35l41_hda.c          | 170 +++++++++++++++++----------
 sound/pci/hda/cs35l41_hda_property.c |  11 +-
 sound/soc/codecs/cs35l41-lib.c       |   6 +
 4 files changed, 124 insertions(+), 66 deletions(-)

-- 
2.34.1

[PATCH v1 1/8] ALSA: hda: cs35l41: Use reset label to get GPIO for HP Zbook Fury 17 G9

[Stefan Binding]

This laptop has an incorrect setting in its _DSD for boost type, but
the rest of the _DSD is correct, and we can still use the reset label
to obtain the reset gpio.

Also fix channel map so that amp 0 is right, and amp 1 is left.

Fixes: 581523ee3652 ("ALSA: hda: cs35l41: Override the _DSD for HP Zbook Fury 17 G9 to correct boost type")

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

diff --git a/sound/pci/hda/cs35l41_hda_property.c b/sound/pci/hda/cs35l41_hda_property.c
index b62a4e6968e2..c83328971728 100644
--- a/sound/pci/hda/cs35l41_hda_property.c
+++ b/sound/pci/hda/cs35l41_hda_property.c
@@ -58,9 +58,16 @@ static int hp_vision_acpi_fix(struct cs35l41_hda *cs35l41, struct device *physde
 
 	cs35l41->index = id;
 	cs35l41->channel_index = 0;
-	cs35l41->reset_gpio = gpiod_get_index(physdev, NULL, 1, GPIOD_OUT_HIGH);
+
+	/*
+	 * This system has _DSD, it just contains an error, so we can still get the reset using
+	 * the "reset" label.
+	 */
+	cs35l41->reset_gpio = fwnode_gpiod_get_index(acpi_fwnode_handle(cs35l41->dacpi), "reset",
+						     cs35l41->index, GPIOD_OUT_LOW,
+						     "cs35l41-reset");
 	cs35l41->speaker_id = -ENOENT;
-	hw_cfg->spk_pos = cs35l41->index ? 1 : 0; // right:left
+	hw_cfg->spk_pos = cs35l41->index ? 0 : 1; // right:left
 	hw_cfg->gpio1.func = CS35L41_NOT_USED;
 	hw_cfg->gpio1.valid = true;
 	hw_cfg->gpio2.func = CS35L41_INTERRUPT;
-- 
2.34.1

[PATCH v1 2/8] ALSA: hda: cs35l41: Assert reset before system suspend

[Stefan Binding]

Some system suspend modes may remove power supplies.
To ensure we are not running during this time, we should assert reset.

Note: since the amps use a shared reset, asserting reset prior to
system suspend only works if the amps are suspended in the reverse
order to probe.

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

diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c
index 28cb10ddd191..919e38213975 100644
--- a/sound/pci/hda/cs35l41_hda.c
+++ b/sound/pci/hda/cs35l41_hda.c
@@ -813,14 +813,17 @@ static int cs35l41_system_suspend(struct device *dev)
 
 	/* Shutdown DSP before system suspend */
 	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.
-	 */
+	if (cs35l41->reset_gpio) {
+		dev_info(cs35l41->dev, "Asserting Reset\n");
+		gpiod_set_value_cansleep(cs35l41->reset_gpio, 0);
+		usleep_range(2000, 2100);
+	}
+
+	dev_dbg(cs35l41->dev, "System Suspended\n");
+
 	return ret;
 }
 
-- 
2.34.1

[PATCH v1 3/8] ALSA: hda: cs35l41: Assert Reset prior to de-asserting in probe and system resume

[Stefan Binding]

To ensure we are in a known state, exiting from reset at the point of
probe or in system resume, assert reset before we de-assert it.

Since the BIOS may enter into a pre-boot environment to control the
amps (for example for boot beep), we need to ensure we start from a
known, reset state prior to probe or system resume.

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

diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c
index 919e38213975..1ac721085fb5 100644
--- a/sound/pci/hda/cs35l41_hda.c
+++ b/sound/pci/hda/cs35l41_hda.c
@@ -840,6 +840,7 @@ static int cs35l41_system_resume(struct device *dev)
 	}
 
 	if (cs35l41->reset_gpio) {
+		gpiod_set_value_cansleep(cs35l41->reset_gpio, 0);
 		usleep_range(2000, 2100);
 		gpiod_set_value_cansleep(cs35l41->reset_gpio, 1);
 	}
@@ -1693,6 +1694,7 @@ int cs35l41_hda_probe(struct device *dev, const char *device_name, int id, int i
 		}
 	}
 	if (cs35l41->reset_gpio) {
+		gpiod_set_value_cansleep(cs35l41->reset_gpio, 0);
 		usleep_range(2000, 2100);
 		gpiod_set_value_cansleep(cs35l41->reset_gpio, 1);
 	}
-- 
2.34.1

[PATCH v1 4/8] ALSA: hda: cs35l41: Run boot process during resume callbacks

[Stefan Binding]

During initial probe, after reset is asserted for the first time, the
driver goes through a boot process to ensure the amp is ready to be
used. This involves verifying a boot flag, as well as verifying the
chip ids.

This is necessary since it is possible for the amp to have been fully
reset by the system suspend calls.

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

diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c
index 1ac721085fb5..e787788c1be2 100644
--- a/sound/pci/hda/cs35l41_hda.c
+++ b/sound/pci/hda/cs35l41_hda.c
@@ -730,6 +730,34 @@ static int cs35l41_hda_channel_map(struct device *dev, unsigned int tx_num, unsi
 				    rx_slot);
 }
 
+int cs35l41_verify_id(struct cs35l41_hda *cs35l41, unsigned int *regid, unsigned int *reg_revid)
+{
+	unsigned int mtl_revid, chipid;
+	int ret;
+
+	ret = regmap_read(cs35l41->regmap, CS35L41_DEVID, regid);
+	if (ret) {
+		dev_err_probe(cs35l41->dev, ret, "Get Device ID failed\n");
+		return ret;
+	}
+
+	ret = regmap_read(cs35l41->regmap, CS35L41_REVID, reg_revid);
+	if (ret) {
+		dev_err_probe(cs35l41->dev, ret, "Get Revision ID failed\n");
+		return ret;
+	}
+
+	mtl_revid = *reg_revid & CS35L41_MTLREVID_MASK;
+
+	chipid = (mtl_revid % 2) ? CS35L41R_CHIP_ID : CS35L41_CHIP_ID;
+	if (*regid != chipid) {
+		dev_err(cs35l41->dev, "CS35L41 Device ID (%X). Expected ID %X\n", *regid, chipid);
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
 static int cs35l41_ready_for_reset(struct cs35l41_hda *cs35l41)
 {
 	int ret = 0;
@@ -827,6 +855,30 @@ static int cs35l41_system_suspend(struct device *dev)
 	return ret;
 }
 
+static int cs35l41_wait_boot_done(struct cs35l41_hda *cs35l41)
+{
+	unsigned int int_status;
+	int ret;
+
+	ret = regmap_read_poll_timeout(cs35l41->regmap, CS35L41_IRQ1_STATUS4, int_status,
+				       int_status & CS35L41_OTP_BOOT_DONE, 1000, 100000);
+	if (ret) {
+		dev_err(cs35l41->dev, "Failed waiting for OTP_BOOT_DONE\n");
+		return ret;
+	}
+
+	ret = regmap_read(cs35l41->regmap, CS35L41_IRQ1_STATUS3, &int_status);
+	if (ret || (int_status & CS35L41_OTP_BOOT_ERR)) {
+		dev_err(cs35l41->dev, "OTP Boot status %x error\n",
+			int_status & CS35L41_OTP_BOOT_ERR);
+		if (!ret)
+			ret = -EIO;
+		return ret;
+	}
+
+	return 0;
+}
+
 static int cs35l41_system_resume(struct device *dev)
 {
 	struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev);
@@ -847,6 +899,14 @@ static int cs35l41_system_resume(struct device *dev)
 
 	usleep_range(2000, 2100);
 
+	regcache_cache_only(cs35l41->regmap, false);
+
+	ret = cs35l41_wait_boot_done(cs35l41);
+	if (ret)
+		return ret;
+
+	regcache_cache_only(cs35l41->regmap, true);
+
 	ret = pm_runtime_force_resume(dev);
 	if (ret) {
 		dev_err(dev, "System Resume Failed: Unable to runtime resume: %d\n", ret);
@@ -908,6 +968,7 @@ static int cs35l41_runtime_suspend(struct device *dev)
 static int cs35l41_runtime_resume(struct device *dev)
 {
 	struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev);
+	unsigned int regid, reg_revid;
 	int ret = 0;
 
 	dev_dbg(cs35l41->dev, "Runtime Resume\n");
@@ -929,6 +990,10 @@ static int cs35l41_runtime_resume(struct device *dev)
 		}
 	}
 
+	ret = cs35l41_verify_id(cs35l41, &regid, &reg_revid);
+	if (ret)
+		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);
@@ -941,6 +1006,8 @@ static int cs35l41_runtime_resume(struct device *dev)
 	if (cs35l41->hw_cfg.bst_type == CS35L41_EXT_BOOST)
 		cs35l41_init_boost(cs35l41->dev, cs35l41->regmap, &cs35l41->hw_cfg);
 
+	dev_dbg(cs35l41->dev, "CS35L41 Resumed (%x), Revision: %02X\n", regid, reg_revid);
+
 err:
 	mutex_unlock(&cs35l41->fw_mutex);
 
@@ -1660,7 +1727,7 @@ static int cs35l41_hda_read_acpi(struct cs35l41_hda *cs35l41, const char *hid, i
 int cs35l41_hda_probe(struct device *dev, const char *device_name, int id, int irq,
 		      struct regmap *regmap)
 {
-	unsigned int int_sts, regid, reg_revid, mtl_revid, chipid, int_status;
+	unsigned int regid, reg_revid;
 	struct cs35l41_hda *cs35l41;
 	int ret;
 
@@ -1701,41 +1768,13 @@ int cs35l41_hda_probe(struct device *dev, const char *device_name, int id, int i
 
 	usleep_range(2000, 2100);
 
-	ret = regmap_read_poll_timeout(cs35l41->regmap, CS35L41_IRQ1_STATUS4, int_status,
-				       int_status & CS35L41_OTP_BOOT_DONE, 1000, 100000);
-	if (ret) {
-		dev_err_probe(cs35l41->dev, ret, "Failed waiting for OTP_BOOT_DONE\n");
-		goto err;
-	}
-
-	ret = regmap_read(cs35l41->regmap, CS35L41_IRQ1_STATUS3, &int_sts);
-	if (ret || (int_sts & CS35L41_OTP_BOOT_ERR)) {
-		dev_err_probe(cs35l41->dev, ret, "OTP Boot status %x error\n",
-			      int_sts & CS35L41_OTP_BOOT_ERR);
-		ret = -EIO;
-		goto err;
-	}
-
-	ret = regmap_read(cs35l41->regmap, CS35L41_DEVID, &regid);
-	if (ret) {
-		dev_err_probe(cs35l41->dev, ret, "Get Device ID failed\n");
-		goto err;
-	}
-
-	ret = regmap_read(cs35l41->regmap, CS35L41_REVID, &reg_revid);
-	if (ret) {
-		dev_err_probe(cs35l41->dev, ret, "Get Revision ID failed\n");
+	ret = cs35l41_wait_boot_done(cs35l41);
+	if (ret)
 		goto err;
-	}
-
-	mtl_revid = reg_revid & CS35L41_MTLREVID_MASK;
 
-	chipid = (mtl_revid % 2) ? CS35L41R_CHIP_ID : CS35L41_CHIP_ID;
-	if (regid != chipid) {
-		dev_err(cs35l41->dev, "CS35L41 Device ID (%X). Expected ID %X\n", regid, chipid);
-		ret = -ENODEV;
+	ret = cs35l41_verify_id(cs35l41, &regid, &reg_revid);
+	if (ret)
 		goto err;
-	}
 
 	ret = cs35l41_test_key_unlock(cs35l41->dev, cs35l41->regmap);
 	if (ret)
-- 
2.34.1

[PATCH v1 5/8] ALSA: hda: cs35l41: Force a software reset after hardware reset

[Stefan Binding]

To ensure the chip has correctly reset during probe and system suspend,
we need to force a software reset, in case of systems where the
hardware reset is not available.

The software reset register was labelled as volatile but not readable,
however, it is readable, (just returns 0x0). Adding it to readable
registers means it will be correctly treated as volatile, and thus
will not be cached.

Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com>
---
 include/sound/cs35l41.h        | 1 +
 sound/pci/hda/cs35l41_hda.c    | 5 +++++
 sound/soc/codecs/cs35l41-lib.c | 1 +
 3 files changed, 7 insertions(+)

diff --git a/include/sound/cs35l41.h b/include/sound/cs35l41.h
index 2fe8c6b0d4cf..80df80fe31e2 100644
--- a/include/sound/cs35l41.h
+++ b/include/sound/cs35l41.h
@@ -735,6 +735,7 @@
 #define CS35L41_REVID_B2		0xB2
 
 #define CS35L41_HALO_CORE_RESET		0x00000200
+#define CS35L41_SOFTWARE_RESET		0x5A000000
 
 #define CS35L41_FS1_WINDOW_MASK		0x000007FF
 #define CS35L41_FS2_WINDOW_MASK		0x00FFF800
diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c
index e787788c1be2..9746c64ff0dd 100644
--- a/sound/pci/hda/cs35l41_hda.c
+++ b/sound/pci/hda/cs35l41_hda.c
@@ -901,6 +901,9 @@ static int cs35l41_system_resume(struct device *dev)
 
 	regcache_cache_only(cs35l41->regmap, false);
 
+	regmap_write(cs35l41->regmap, CS35L41_SFT_RESET, CS35L41_SOFTWARE_RESET);
+	usleep_range(2000, 2100);
+
 	ret = cs35l41_wait_boot_done(cs35l41);
 	if (ret)
 		return ret;
@@ -1766,6 +1769,8 @@ int cs35l41_hda_probe(struct device *dev, const char *device_name, int id, int i
 		gpiod_set_value_cansleep(cs35l41->reset_gpio, 1);
 	}
 
+	usleep_range(2000, 2100);
+	regmap_write(cs35l41->regmap, CS35L41_SFT_RESET, CS35L41_SOFTWARE_RESET);
 	usleep_range(2000, 2100);
 
 	ret = cs35l41_wait_boot_done(cs35l41);
diff --git a/sound/soc/codecs/cs35l41-lib.c b/sound/soc/codecs/cs35l41-lib.c
index 2ec5fdc875b1..ddedb7e63cb6 100644
--- a/sound/soc/codecs/cs35l41-lib.c
+++ b/sound/soc/codecs/cs35l41-lib.c
@@ -74,6 +74,7 @@ static bool cs35l41_readable_reg(struct device *dev, unsigned int reg)
 	case CS35L41_FABID:
 	case CS35L41_RELID:
 	case CS35L41_OTPID:
+	case CS35L41_SFT_RESET:
 	case CS35L41_TEST_KEY_CTL:
 	case CS35L41_USER_KEY_CTL:
 	case CS35L41_OTP_CTRL0:
-- 
2.34.1

[PATCH v1 6/8] ALSA: hda: cs35l41: Do not unload firmware before reset in system suspend

[Stefan Binding]

Given the part is about to reset due to system suspend, and we are
already in hibernate, there is no need to wake up the amp, just to get
it ready to be reset. We just need to ensure cs_dsp is ready for reset
by resetting the states.

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

diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c
index 9746c64ff0dd..69303888be1a 100644
--- a/sound/pci/hda/cs35l41_hda.c
+++ b/sound/pci/hda/cs35l41_hda.c
@@ -760,41 +760,16 @@ int cs35l41_verify_id(struct cs35l41_hda *cs35l41, unsigned int *regid, unsigned
 
 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);
-
-		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);
+		cs35l41->cs_dsp.running = false;
+		cs35l41->cs_dsp.booted = false;
+		cs35l41->firmware_running = false;
 	}
-err:
-	regcache_cache_only(cs35l41->regmap, true);
 	regcache_mark_dirty(cs35l41->regmap);
-
 	mutex_unlock(&cs35l41->fw_mutex);
 
-	return ret;
+	return 0;
 }
 
 static int cs35l41_system_suspend_prep(struct device *dev)
-- 
2.34.1

[PATCH v1 7/8] ALSA: hda: cs35l41: Check CSPL state after loading firmware

[Stefan Binding]

CSPL firmware should be in RUNNING or PAUSED state after loading.
If not, the firmware has not been loaded correctly, and we can unload
it and pass the error up.

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

diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c
index 69303888be1a..496ff6a9d300 100644
--- a/sound/pci/hda/cs35l41_hda.c
+++ b/sound/pci/hda/cs35l41_hda.c
@@ -994,6 +994,7 @@ static int cs35l41_runtime_resume(struct device *dev)
 
 static int cs35l41_smart_amp(struct cs35l41_hda *cs35l41)
 {
+	unsigned int fw_status;
 	__be32 halo_sts;
 	int ret;
 
@@ -1027,6 +1028,23 @@ static int cs35l41_smart_amp(struct cs35l41_hda *cs35l41)
 		goto clean_dsp;
 	}
 
+	ret = regmap_read(cs35l41->regmap, CS35L41_DSP_MBOX_2, &fw_status);
+	if (ret < 0) {
+		dev_err(cs35l41->dev,
+			"Failed to read firmware status: %d\n", ret);
+		goto clean_dsp;
+	}
+
+	switch (fw_status) {
+	case CSPL_MBOX_STS_RUNNING:
+	case CSPL_MBOX_STS_PAUSED:
+		break;
+	default:
+		dev_err(cs35l41->dev, "Firmware status is invalid: %u\n",
+			fw_status);
+		goto clean_dsp;
+	}
+
 	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);
-- 
2.34.1

[PATCH v1 8/8] ASoC: cs35l41: Detect CSPL errors when sending CSPL commands

[Stefan Binding]

The existing code checks for the correct state transition after sending
a command. However, it is possible for the message box to return -1,
which indicates an error, if an error has occurred in the firmware.
We can detect if the error has occurred, and return a different error.
In addition, there is no recovering from a CSPL error, so the retry
mechanism is not needed in this case, and we can return immediately.

Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com>
---
 include/sound/cs35l41.h        | 2 ++
 sound/soc/codecs/cs35l41-lib.c | 5 +++++
 2 files changed, 7 insertions(+)

diff --git a/include/sound/cs35l41.h b/include/sound/cs35l41.h
index 80df80fe31e2..043f8ac65dbf 100644
--- a/include/sound/cs35l41.h
+++ b/include/sound/cs35l41.h
@@ -816,6 +816,8 @@ struct cs35l41_otp_map_element_t {
 };
 
 enum cs35l41_cspl_mbox_status {
+	CSPL_MBOX_STS_ERROR = U32_MAX,
+	CSPL_MBOX_STS_ERROR2 = 0x00ffffff, // firmware not always sign-extending 24-bit value
 	CSPL_MBOX_STS_RUNNING = 0,
 	CSPL_MBOX_STS_PAUSED = 1,
 	CSPL_MBOX_STS_RDY_FOR_REINIT = 2,
diff --git a/sound/soc/codecs/cs35l41-lib.c b/sound/soc/codecs/cs35l41-lib.c
index ddedb7e63cb6..4569e4f7cf7e 100644
--- a/sound/soc/codecs/cs35l41-lib.c
+++ b/sound/soc/codecs/cs35l41-lib.c
@@ -1474,6 +1474,11 @@ int cs35l41_set_cspl_mbox_cmd(struct device *dev, struct regmap *regmap,
 			continue;
 		}
 
+		if (sts == CSPL_MBOX_STS_ERROR || sts == CSPL_MBOX_STS_ERROR2) {
+			dev_err(dev, "CSPL Error Detected\n");
+			return -EINVAL;
+		}
+
 		if (!cs35l41_check_cspl_mbox_sts(cmd, sts))
 			dev_dbg(dev, "[%u] cmd %u returned invalid sts %u", i, cmd, sts);
 		else
-- 
2.34.1

Re: [PATCH v1 8/8] ASoC: cs35l41: Detect CSPL errors when sending CSPL commands

[Mark Brown]

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

On Thu, Oct 26, 2023 at 04:05:58PM +0100, Stefan Binding wrote:
> The existing code checks for the correct state transition after sending
> a command. However, it is possible for the message box to return -1,
> which indicates an error, if an error has occurred in the firmware.
> We can detect if the error has occurred, and return a different error.
> In addition, there is no recovering from a CSPL error, so the retry
> mechanism is not needed in this case, and we can return immediately.

Acked-by: Mark Brown <broonie@kernel.org>

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

Re: [PATCH v1 0/8] System Suspend fixes and improvements for CS35L41 HDA

[Takashi Iwai]

On Thu, 26 Oct 2023 17:05:50 +0200,
Stefan Binding wrote:
> 
> There is a report of a single laptop which uses CS35L41 HDA having an
> issue with System Suspend. This particular laptop uses S3 (Deep) Sleep.
> The reported issue states that when the laptop resumes from a system
> suspend, audio no longer works.
> 
> The root cause of this issue is due to the CS35L41 being returned to us
> in an unexpected state after a suspend/resume cycle.
> When the driver resumes, it expects the parts to have been reset, which
> leads to issues with audio and firmware loading.
> 
> To prevent this issue, and the possibility of similar issues, patches
> 2-5 force the driver to reset during probe, system suspend, and system
> resume, which ensures that the part is always in the correct state.
> Patches 6-8 are improvements in the suspend and firmware loading code,
> which makes it easier to detect issues in the future, as well as
> simplifiying the suspend code.
> 
> Patch 1 is a fix for an incorrect configuration for the HP Zbook Fury
> 17, which is the laptop which had the original issue.
> 
> Stefan Binding (8):
>   ALSA: hda: cs35l41: Use reset label to get GPIO for HP Zbook Fury 17
>     G9
>   ALSA: hda: cs35l41: Assert reset before system suspend
>   ALSA: hda: cs35l41: Assert Reset prior to de-asserting in probe and
>     system resume
>   ALSA: hda: cs35l41: Run boot process during resume callbacks
>   ALSA: hda: cs35l41: Force a software reset after hardware reset
>   ALSA: hda: cs35l41: Do not unload firmware before reset in system
>     suspend
>   ALSA: hda: cs35l41: Check CSPL state after loading firmware
>   ASoC: cs35l41: Detect CSPL errors when sending CSPL commands

Applied to for-next branch now.  Thanks.


Takashi