[PATCH v1 0/2] ALSA: cs35l41: prevent old firmwares using unsupported commands
Some systems use older firmware which does not support newer commands which are used to enable external boost. For those systems, we can workaround this by writing the registers directly.
We can use the firmware version, stored inside cs_dsp, to determine whether or not the command is supported. To achieve this, it requires a cleanup in the api, to pass the cs_dsp struct into the function.
We can also remove the redundant boolean firmware_running from the HDA driver, and use the equivalent state inside cs_dsp.
This chain is based on Mark's branch, since the api change was made to the function in sound/soc/codecs/cs35l41-lib.c.
Stefan Binding (2): ALSA: hda: cs35l41: Remove unnecessary boolean state variable firmware_running ALSA: cs35l41: Fix for old systems which do not support command
include/sound/cs35l41.h | 2 +- sound/pci/hda/cs35l41_hda.c | 28 ++++++++++++---------------- sound/pci/hda/cs35l41_hda.h | 1 - sound/soc/codecs/cs35l41-lib.c | 6 ++++-- sound/soc/codecs/cs35l41.c | 4 ++-- 5 files changed, 19 insertions(+), 22 deletions(-)
This state duplicates the running state inside cs_dsp, so is not necessary. Remove it, and use cs_dsp.running instead. This brings the CS35L41 HDA driver more inline with its ASoC version, allowing the same state to be used when calling library functions.
Signed-off-by: Stefan Binding sbinding@opensource.cirrus.com --- sound/pci/hda/cs35l41_hda.c | 28 ++++++++++++---------------- sound/pci/hda/cs35l41_hda.h | 1 - 2 files changed, 12 insertions(+), 17 deletions(-)
diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c index c74faa2ff46c..28f6ef32937d 100644 --- a/sound/pci/hda/cs35l41_hda.c +++ b/sound/pci/hda/cs35l41_hda.c @@ -458,7 +458,6 @@ static void cs35l41_shutdown_dsp(struct cs35l41_hda *cs35l41)
cs_dsp_stop(dsp); cs_dsp_power_down(dsp); - cs35l41->firmware_running = false; dev_dbg(cs35l41->dev, "Unloaded Firmware\n"); }
@@ -504,7 +503,7 @@ static void cs35l41_hda_play_start(struct device *dev)
cs35l41->playback_started = true;
- if (cs35l41->firmware_running) { + if (cs35l41->cs_dsp.running) { regmap_multi_reg_write(reg, cs35l41_hda_config_dsp, ARRAY_SIZE(cs35l41_hda_config_dsp)); regmap_update_bits(reg, CS35L41_PWR_CTRL2, @@ -528,8 +527,8 @@ static void cs35l41_hda_play_done(struct device *dev) dev_dbg(dev, "Play (Complete)\n");
cs35l41_global_enable(dev, reg, cs35l41->hw_cfg.bst_type, 1, - cs35l41->firmware_running); - if (cs35l41->firmware_running) { + cs35l41->cs_dsp.running); + if (cs35l41->cs_dsp.running) { regmap_multi_reg_write(reg, cs35l41_hda_unmute_dsp, ARRAY_SIZE(cs35l41_hda_unmute_dsp)); } else { @@ -547,7 +546,7 @@ static void cs35l41_hda_pause_start(struct device *dev)
regmap_multi_reg_write(reg, cs35l41_hda_mute, ARRAY_SIZE(cs35l41_hda_mute)); cs35l41_global_enable(dev, reg, cs35l41->hw_cfg.bst_type, 0, - cs35l41->firmware_running); + cs35l41->cs_dsp.running); }
static void cs35l41_hda_pause_done(struct device *dev) @@ -560,7 +559,7 @@ static void cs35l41_hda_pause_done(struct device *dev) 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) { + if (cs35l41->cs_dsp.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, @@ -610,7 +609,7 @@ static void cs35l41_hda_playback_hook(struct device *dev, int action) break; case HDA_GEN_PCM_ACT_CLOSE: mutex_lock(&cs35l41->fw_mutex); - if (!cs35l41->firmware_running && cs35l41->request_fw_load && + if (!cs35l41->cs_dsp.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; @@ -670,7 +669,7 @@ static int cs35l41_ready_for_reset(struct cs35l41_hda *cs35l41) int ret = 0;
mutex_lock(&cs35l41->fw_mutex); - if (cs35l41->firmware_running) { + if (cs35l41->cs_dsp.running) {
regcache_cache_only(cs35l41->regmap, false);
@@ -748,7 +747,6 @@ 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);
@@ -818,7 +816,7 @@ static int cs35l41_runtime_suspend(struct device *dev)
mutex_lock(&cs35l41->fw_mutex);
- if (cs35l41->firmware_running) { + if (cs35l41->cs_dsp.running) { ret = cs35l41_enter_hibernate(cs35l41->dev, cs35l41->regmap, cs35l41->hw_cfg.bst_type); if (ret) @@ -852,7 +850,7 @@ static int cs35l41_runtime_resume(struct device *dev)
regcache_cache_only(cs35l41->regmap, false);
- if (cs35l41->firmware_running) { + if (cs35l41->cs_dsp.running) { ret = cs35l41_exit_hibernate(cs35l41->dev, cs35l41->regmap); if (ret) { dev_warn(cs35l41->dev, "Unable to exit Hibernate."); @@ -919,8 +917,6 @@ static int cs35l41_smart_amp(struct cs35l41_hda *cs35l41) goto clean_dsp; }
- cs35l41->firmware_running = true; - return 0;
clean_dsp: @@ -930,10 +926,10 @@ static int cs35l41_smart_amp(struct cs35l41_hda *cs35l41)
static void cs35l41_load_firmware(struct cs35l41_hda *cs35l41, bool load) { - if (cs35l41->firmware_running && !load) { + if (cs35l41->cs_dsp.running && !load) { dev_dbg(cs35l41->dev, "Unloading Firmware\n"); cs35l41_shutdown_dsp(cs35l41); - } else if (!cs35l41->firmware_running && load) { + } else if (!cs35l41->cs_dsp.running && load) { dev_dbg(cs35l41->dev, "Loading Firmware\n"); cs35l41_smart_amp(cs35l41); } else { @@ -1130,7 +1126,7 @@ static int cs35l41_hda_bind(struct device *dev, struct device *master, void *mas cs35l41->acpi_subsystem_id, cs35l41->hw_cfg.bst_type, cs35l41->hw_cfg.gpio1.func == CS35l41_VSPK_SWITCH, cs35l41->hw_cfg.spk_pos ? 'R' : 'L', - cs35l41->firmware_running, cs35l41->speaker_id); + cs35l41->cs_dsp.running, cs35l41->speaker_id);
return ret; } diff --git a/sound/pci/hda/cs35l41_hda.h b/sound/pci/hda/cs35l41_hda.h index b93bf762976e..f712a87473c8 100644 --- a/sound/pci/hda/cs35l41_hda.h +++ b/sound/pci/hda/cs35l41_hda.h @@ -64,7 +64,6 @@ struct cs35l41_hda { struct work_struct fw_load_work;
struct regmap_irq_chip_data *irq_data; - bool firmware_running; bool request_fw_load; bool fw_request_ongoing; bool halo_initialized;
Some older laptops using cs35l41 use firmware which does not support the CSPL_MBOX_CMD_SPK_OUT_ENABLE command. Firmware versions v0.28.0 and older do not support this command.
Fixes: fa3efcc36aac ("ALSA: cs35l41: Use mbox command to enable speaker output for external boost")
Signed-off-by: Stefan Binding sbinding@opensource.cirrus.com --- include/sound/cs35l41.h | 2 +- sound/pci/hda/cs35l41_hda.c | 4 ++-- sound/soc/codecs/cs35l41-lib.c | 6 ++++-- sound/soc/codecs/cs35l41.c | 4 ++-- 4 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/include/sound/cs35l41.h b/include/sound/cs35l41.h index 2fe8c6b0d4cf..ccee065958a3 100644 --- a/include/sound/cs35l41.h +++ b/include/sound/cs35l41.h @@ -903,6 +903,6 @@ int cs35l41_init_boost(struct device *dev, struct regmap *regmap, bool cs35l41_safe_reset(struct regmap *regmap, enum cs35l41_boost_type b_type); int cs35l41_mdsync_up(struct regmap *regmap); int cs35l41_global_enable(struct device *dev, struct regmap *regmap, enum cs35l41_boost_type b_type, - int enable, bool firmware_running); + int enable, struct cs_dsp *dsp);
#endif /* __CS35L41_H */ diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c index 28f6ef32937d..698fb7c5a028 100644 --- a/sound/pci/hda/cs35l41_hda.c +++ b/sound/pci/hda/cs35l41_hda.c @@ -527,7 +527,7 @@ static void cs35l41_hda_play_done(struct device *dev) dev_dbg(dev, "Play (Complete)\n");
cs35l41_global_enable(dev, reg, cs35l41->hw_cfg.bst_type, 1, - cs35l41->cs_dsp.running); + &cs35l41->cs_dsp); if (cs35l41->cs_dsp.running) { regmap_multi_reg_write(reg, cs35l41_hda_unmute_dsp, ARRAY_SIZE(cs35l41_hda_unmute_dsp)); @@ -546,7 +546,7 @@ static void cs35l41_hda_pause_start(struct device *dev)
regmap_multi_reg_write(reg, cs35l41_hda_mute, ARRAY_SIZE(cs35l41_hda_mute)); cs35l41_global_enable(dev, reg, cs35l41->hw_cfg.bst_type, 0, - cs35l41->cs_dsp.running); + &cs35l41->cs_dsp); }
static void cs35l41_hda_pause_done(struct device *dev) diff --git a/sound/soc/codecs/cs35l41-lib.c b/sound/soc/codecs/cs35l41-lib.c index 2ec5fdc875b1..f18fbd4b3ede 100644 --- a/sound/soc/codecs/cs35l41-lib.c +++ b/sound/soc/codecs/cs35l41-lib.c @@ -16,6 +16,8 @@
#include <sound/cs35l41.h>
+#define CS35L41_FIRMWARE_OLD_VERSION 0x001C00 /* v0.28.0 */ + static const struct reg_default cs35l41_reg[] = { { CS35L41_PWR_CTRL1, 0x00000000 }, { CS35L41_PWR_CTRL2, 0x00000000 }, @@ -1213,7 +1215,7 @@ EXPORT_SYMBOL_GPL(cs35l41_safe_reset); * the PLL Lock interrupt, in the IRQ handler. */ int cs35l41_global_enable(struct device *dev, struct regmap *regmap, enum cs35l41_boost_type b_type, - int enable, bool firmware_running) + int enable, struct cs_dsp *dsp) { int ret; unsigned int gpio1_func, pad_control, pwr_ctrl1, pwr_ctrl3, int_status, pup_pdn_mask; @@ -1308,7 +1310,7 @@ int cs35l41_global_enable(struct device *dev, struct regmap *regmap, enum cs35l4 } regmap_write(regmap, CS35L41_IRQ1_STATUS1, CS35L41_PUP_DONE_MASK);
- if (firmware_running) + if (dsp->running && dsp->fw_id_version > CS35L41_FIRMWARE_OLD_VERSION) ret = cs35l41_set_cspl_mbox_cmd(dev, regmap, CSPL_MBOX_CMD_SPK_OUT_ENABLE); else diff --git a/sound/soc/codecs/cs35l41.c b/sound/soc/codecs/cs35l41.c index 4bc64ba71cd6..df326fe90447 100644 --- a/sound/soc/codecs/cs35l41.c +++ b/sound/soc/codecs/cs35l41.c @@ -520,11 +520,11 @@ static int cs35l41_main_amp_event(struct snd_soc_dapm_widget *w, ARRAY_SIZE(cs35l41_pup_patch));
ret = cs35l41_global_enable(cs35l41->dev, cs35l41->regmap, cs35l41->hw_cfg.bst_type, - 1, cs35l41->dsp.cs_dsp.running); + 1, &cs35l41->dsp.cs_dsp); break; case SND_SOC_DAPM_POST_PMD: ret = cs35l41_global_enable(cs35l41->dev, cs35l41->regmap, cs35l41->hw_cfg.bst_type, - 0, cs35l41->dsp.cs_dsp.running); + 0, &cs35l41->dsp.cs_dsp);
regmap_multi_reg_write_bypassed(cs35l41->regmap, cs35l41_pdn_patch,
On Fri, Sep 22, 2023 at 03:28:18PM +0100, Stefan Binding wrote:
Some older laptops using cs35l41 use firmware which does not support the CSPL_MBOX_CMD_SPK_OUT_ENABLE command. Firmware versions v0.28.0 and older do not support this command.
Acked-by: Mark Brown broonie@kernel.org
On Fri, 22 Sep 2023 16:28:16 +0200, Stefan Binding wrote:
Some systems use older firmware which does not support newer commands which are used to enable external boost. For those systems, we can workaround this by writing the registers directly.
We can use the firmware version, stored inside cs_dsp, to determine whether or not the command is supported. To achieve this, it requires a cleanup in the api, to pass the cs_dsp struct into the function.
We can also remove the redundant boolean firmware_running from the HDA driver, and use the equivalent state inside cs_dsp.
So those are fixes needed for 6.6 kernel? Or they are something new?
This chain is based on Mark's branch, since the api change was made to the function in sound/soc/codecs/cs35l41-lib.c.
I'd need a PR from Mark before applying those, then.
thanks,
Takashi
On Fri, Sep 22, 2023 at 04:35:45PM +0200, Takashi Iwai wrote:
Stefan Binding wrote:
This chain is based on Mark's branch, since the api change was made to
The term is patch series.
the function in sound/soc/codecs/cs35l41-lib.c.
I'd need a PR from Mark before applying those, then.
Or if they're 6.7 stuff I guess I could just carry them.
On Fri, 22 Sep 2023 16:38:28 +0200, Mark Brown wrote:
On Fri, Sep 22, 2023 at 04:35:45PM +0200, Takashi Iwai wrote:
Stefan Binding wrote:
This chain is based on Mark's branch, since the api change was made to
The term is patch series.
the function in sound/soc/codecs/cs35l41-lib.c.
I'd need a PR from Mark before applying those, then.
Or if they're 6.7 stuff I guess I could just carry them.
Yes, that's another option. But there are already changes for HDA cs35l41 stuff for 6.7, and you might need to pull from my for-next branch beforehand. A bit messy in either way.
thanks,
Takashi
-----Original Message----- From: Takashi Iwai tiwai@suse.de Sent: Friday, September 22, 2023 3:36 PM To: Stefan Binding sbinding@opensource.cirrus.com Cc: Mark Brown broonie@kernel.org; Jaroslav Kysela perex@perex.cz; Takashi Iwai tiwai@suse.com; alsa-devel@alsa- project.org; linux-kernel@vger.kernel.org; patches@opensource.cirrus.com Subject: Re: [PATCH v1 0/2] ALSA: cs35l41: prevent old firmwares
using
unsupported commands
On Fri, 22 Sep 2023 16:28:16 +0200, Stefan Binding wrote:
Some systems use older firmware which does not support newer
commands
which are used to enable external boost. For those systems, we can workaround this by writing the registers directly.
We can use the firmware version, stored inside cs_dsp, to
determine
whether or not the command is supported. To achieve this, it requires a cleanup in the api, to pass the
cs_dsp
struct into the function.
We can also remove the redundant boolean firmware_running from the
HDA
driver, and use the equivalent state inside cs_dsp.
So those are fixes needed for 6.6 kernel? Or they are something
new?
These are to fix the issue that was reported on the Lenovo Legion 7 16ACHg6, which was introduced after the fixes to CS35L41 HDA System Suspend.
Thanks, Stefan
This chain is based on Mark's branch, since the api change was
made to
the function in sound/soc/codecs/cs35l41-lib.c.
I'd need a PR from Mark before applying those, then.
thanks,
Takashi
On Fri, Sep 22, 2023 at 03:44:30PM +0100, Stefan Binding wrote:
So those are fixes needed for 6.6 kernel? Or they are something
new?
These are to fix the issue that was reported on the Lenovo Legion 7 16ACHg6, which was introduced after the fixes to CS35L41 HDA System Suspend.
Could you be more specific about which fixes these are and which tree they're in? If they're fixes then I don't have anything queued for 6.6 so I'm confused about why you say there's a dependency on my tree.
On Fri, 22 Sep 2023 16:48:03 +0200, Mark Brown wrote:
On Fri, Sep 22, 2023 at 03:44:30PM +0100, Stefan Binding wrote:
So those are fixes needed for 6.6 kernel? Or they are something
new?
These are to fix the issue that was reported on the Lenovo Legion 7 16ACHg6, which was introduced after the fixes to CS35L41 HDA System Suspend.
Could you be more specific about which fixes these are and which tree they're in? If they're fixes then I don't have anything queued for 6.6 so I'm confused about why you say there's a dependency on my tree.
Yeah. The suspend fix has been landed in 6.6-rc1, so it's for 6.6, I suppose.
Stefan, if it's a fix for a known commit, please put Fixes tag at the next time.
thanks,
Takashi
participants (3)
-
Mark Brown -
Stefan Binding -
Takashi Iwai