[PATCH v1 00/11] Fix support for System Suspend for CS35L41 HDA
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.
Stefan Binding (11): ALSA: cs35l41: Use mbox command to enable speaker output for external boost 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/realtek: Support pre-/post- playback hooks for cs35l41 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(-)
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.
In addition, to ensure the chip has correctly powered up or down, the driver will now poll a register, rather than wait a fixed delay.
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 | 118 ++++++++++++++++++++++++++++----- sound/soc/codecs/cs35l41.c | 18 ++--- 4 files changed, 115 insertions(+), 35 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..4ec306cd2f476 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,12 @@ 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, pup_pdn_mask; + unsigned int pwr_ctl1_val; struct reg_sequence cs35l41_mdsync_down_seq[] = { {CS35L41_PWR_CTRL3, 0}, {CS35L41_GPIO_PAD_CONTROL, 0}, @@ -1204,6 +1209,20 @@ int cs35l41_global_enable(struct regmap *regmap, enum cs35l41_boost_type b_type, {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; + } 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: @@ -1240,20 +1259,85 @@ int cs35l41_global_enable(struct regmap *regmap, enum cs35l41_boost_type b_type, 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, enable << CS35L41_GLOBAL_EN_SHIFT); - usleep_range(3000, 3100); + if (ret) { + dev_err(dev, "CS35L41_PWR_CTRL1 set failed: %d\n", ret); + return ret; + } + + 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: - 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; + + 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, + 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; + } + + 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, + ARRAY_SIZE(cs35l41_active_to_safe_end)); + } break; default: ret = -EINVAL; @@ -1344,6 +1428,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..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) { @@ -500,21 +499,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 = 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); + ret = cs35l41_global_enable(cs35l41->dev, cs35l41->regmap, cs35l41->hw_cfg.bst_type, + 0, &cs35l41->pll_lock, cs35l41->dsp.cs_dsp.running);
regmap_multi_reg_write_bypassed(cs35l41->regmap, cs35l41_pdn_patch,
On Thu, Jul 20, 2023 at 02:31:37PM +0100, Stefan Binding wrote:
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.
In addition, to ensure the chip has correctly powered up or down, the driver will now poll a register, rather than wait a fixed delay.
That should probably be a separate patch.
Acked-by: Mark Brown broonie@kernel.org
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;
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)
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); }
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) {
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 ++ 1 file changed, 2 insertions(+)
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); };
On Thu, 20 Jul 2023 15:31:42 +0200, Stefan Binding wrote:
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
It's better to combine this and patch 9 for the actual calls, so that we know the full effect of the changes.
thanks,
Takashi
sound/pci/hda/hda_component.h | 2 ++ 1 file changed, 2 insertions(+)
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);
};
2.34.1
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);
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);
hda_component now supports pre and post playback hooks. These should be called for each amp in a loop before moving on to the next hook.
Signed-off-by: Stefan Binding sbinding@opensource.cirrus.com --- sound/pci/hda/patch_realtek.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c index ac29965951833..4aa49d37ed9e6 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 {
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 = {
On Thu, 20 Jul 2023 15:31:46 +0200, Stefan Binding wrote:
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);
Is lock_system_sleep() mandatory for device_link_add()? The function takes its rw lock for the race, I suppose.
thanks,
Takashi
On 20/07/2023 15:21, Takashi Iwai wrote:
On Thu, 20 Jul 2023 15:31:46 +0200, Stefan Binding wrote:
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);
Is lock_system_sleep() mandatory for device_link_add()? The function takes its rw lock for the race, I suppose.
I believe this is mandatory, to ensure we don't try to add the device link during a suspend/resume transition. Its probably unlikely that that would occur during the component bind, but just in case.
Thanks,
Stefan
thanks,
Takashi
On Fri, 21 Jul 2023 16:55:43 +0200, Stefan Binding wrote:
On 20/07/2023 15:21, Takashi Iwai wrote:
On Thu, 20 Jul 2023 15:31:46 +0200, Stefan Binding wrote:
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);
Is lock_system_sleep() mandatory for device_link_add()? The function takes its rw lock for the race, I suppose.
I believe this is mandatory, to ensure we don't try to add the device link during a suspend/resume transition. Its probably unlikely that that would occur during the component bind, but just in case.
OK, it seems needed in this case. I found the description in device_link.rst, too.
thanks,
Takashi
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);
participants (3)
-
Mark Brown -
Stefan Binding -
Takashi Iwai