[PATCH v1 0/2] ALSA: cs35l41: prevent old firmwares using unsupported commands

[PATCH v1 0/2] ALSA: cs35l41: prevent old firmwares using unsupported commands

[Stefan Binding]

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(-)

-- 
2.34.1

[PATCH v1 1/2] ALSA: hda: cs35l41: Remove unnecessary boolean state variable firmware_running

[Stefan Binding]

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;
-- 
2.34.1

[PATCH v1 2/2] ALSA: cs35l41: Fix for old systems which do not support command

[Stefan Binding]

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,
-- 
2.34.1

Re: [PATCH v1 2/2] ALSA: cs35l41: Fix for old systems which do not support command

[Mark Brown]

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

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>

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

Re: [PATCH v1 0/2] ALSA: cs35l41: prevent old firmwares using unsupported commands

[Takashi Iwai]

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

Re: [PATCH v1 0/2] ALSA: cs35l41: prevent old firmwares using unsupported commands

[Mark Brown]

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

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.

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

Re: [PATCH v1 0/2] ALSA: cs35l41: prevent old firmwares using unsupported commands

[Takashi Iwai]

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

RE: [PATCH v1 0/2] ALSA: cs35l41: prevent old firmwares using unsupported commands

[Stefan Binding]

> -----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

Re: [PATCH v1 0/2] ALSA: cs35l41: prevent old firmwares using unsupported commands

[Mark Brown]

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

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.

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

Re: [PATCH v1 0/2] ALSA: cs35l41: prevent old firmwares using unsupported commands

[Takashi Iwai]

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