diff mbox series

[v2] ASoc: tas2781: Playback can work when only RCA binary loading without dsp firmware loading

Message ID 20240525014727.197-1-shenghao-ding@ti.com (mailing list archive)
State New, archived
Headers show
Series [v2] ASoc: tas2781: Playback can work when only RCA binary loading without dsp firmware loading | expand

Commit Message

Shenghao Ding May 25, 2024, 1:47 a.m. UTC 
In only RCA binary loading case, only default dsp program inside the
chip will be work.

Fixes: ef3bcde75d06 ("ASoC: tas2781: Add tas2781 driver")
Signed-off-by: Shenghao Ding <shenghao-ding@ti.com>

---
v2:
 - correct comment.
 - Add Fixes.
 - Move header file to the first.
v1:
 - Split out the different logical changes into different patches.
 - rename tasdevice_dsp_fw_state -> tasdevice_fw_state, the fw are not
   only dsp fw, but also RCA(Reconfigurable data, such as acoustic data
   and register setting, etc).
 - Add TASDEVICE_RCA_FW_OK in tasdevice_fw_state to identify the state
   that only RCA binary file has been download successfully, but dsp fw
   is not loaded or loading failure.
 - Add the this strategy into tasdevice_tuning_switch.
 - If one side of the if/else has a braces both should in
   tasdevice_tuning_switch.
 - Identify whehter both RCA and DSP have been loaded or only RCA has
   been loaded in tasdevice_fw_ready.
 - Add check fw load status in tasdevice_startup.
 - remove ret in tasdevice_startup to make th code neater.
---
 include/sound/tas2781-dsp.h       |  3 ++-
 sound/soc/codecs/tas2781-fmwlib.c | 14 ++++++++++----
 sound/soc/codecs/tas2781-i2c.c    | 30 ++++++++++++++++++------------
 3 files changed, 30 insertions(+), 17 deletions(-)

Comments

Pierre-Louis Bossart May 27, 2024, 1:44 p.m. UTC | #1 
On 5/24/24 20:47, Shenghao Ding wrote:
> In only RCA binary loading case, only default dsp program inside the
> chip will be work.

What does 'RCA' stand for?

Also clarify the commit title without double negatives, e.g.

"Enable RCA-based playback without DSP firmware download"
> -	if (tas_priv->fw_state == TASDEVICE_DSP_FW_FAIL) {
> -		dev_err(tas_priv->dev, "DSP bin file not loaded\n");
> +	/*
> +	 * Only RCA file loaded can still work with default dsp program inside
> +	 * the chip?

reword the commit and remove question mark.

> +	 */
> +	if (!(tas_priv->fw_state == TASDEVICE_RCA_FW_OK ||
> +		tas_priv->fw_state == TASDEVICE_DSP_FW_ALL_OK)) {
> +		dev_err(tas_priv->dev, "No firmware loaded\n");
>  		return;
>  	}
>  
>  	if (state == 0) {
> -		if (tas_priv->cur_prog < tas_fmw->nr_programs) {
> +		if (tas_fmw && tas_priv->cur_prog < tas_fmw->nr_programs) {
>  			/*dsp mode or tuning mode*/

spaces in comments

>  			profile_cfg_id = tas_priv->rcabin.profile_cfg_id;
>  			tasdevice_select_tuningprm_cfg(tas_priv,
> @@ -2340,9 +2345,10 @@ void tasdevice_tuning_switch(void *context, int state)
>  
>  		tasdevice_select_cfg_blk(tas_priv, profile_cfg_id,
>  			TASDEVICE_BIN_BLK_PRE_POWER_UP);
> -	} else
> +	} else {
>  		tasdevice_select_cfg_blk(tas_priv, profile_cfg_id,
>  			TASDEVICE_BIN_BLK_PRE_SHUTDOWN);
> +	}
>  }
>  EXPORT_SYMBOL_NS_GPL(tasdevice_tuning_switch,
>  	SND_SOC_TAS2781_FMWLIB);
> diff --git a/sound/soc/codecs/tas2781-i2c.c b/sound/soc/codecs/tas2781-i2c.c
> index 9350972dfefe..ccb9313ada9b 100644
> --- a/sound/soc/codecs/tas2781-i2c.c
> +++ b/sound/soc/codecs/tas2781-i2c.c
> @@ -380,23 +380,30 @@ static void tasdevice_fw_ready(const struct firmware *fmw,
>  	mutex_lock(&tas_priv->codec_lock);
>  
>  	ret = tasdevice_rca_parser(tas_priv, fmw);
> -	if (ret)
> +	if (ret) {
> +		tasdevice_config_info_remove(tas_priv);
>  		goto out;
> +	}
>  	tasdevice_create_control(tas_priv);
>  
>  	tasdevice_dsp_remove(tas_priv);
>  	tasdevice_calbin_remove(tas_priv);
> -	tas_priv->fw_state = TASDEVICE_DSP_FW_PENDING;
> +	tas_priv->fw_state = TASDEVICE_RCA_FW_OK;
>  	scnprintf(tas_priv->coef_binaryname, 64, "%s_coef.bin",
>  		tas_priv->dev_name);
> +
>  	ret = tasdevice_dsp_parser(tas_priv);
>  	if (ret) {
>  		dev_err(tas_priv->dev, "dspfw load %s error\n",
>  			tas_priv->coef_binaryname);
> -		tas_priv->fw_state = TASDEVICE_DSP_FW_FAIL;
>  		goto out;
>  	}
> -	tasdevice_dsp_create_ctrls(tas_priv);
> +
> +	ret = tasdevice_dsp_create_ctrls(tas_priv);
> +	if (ret) {
> +		dev_err(tas_priv->dev, "dsp controls error\n");
> +		goto out;
> +	}

this seems unrelated to the boot process? Move to a different patch?

>  
>  	tas_priv->fw_state = TASDEVICE_DSP_FW_ALL_OK;
>  
> @@ -417,9 +424,8 @@ static void tasdevice_fw_ready(const struct firmware *fmw,
>  	tasdevice_prmg_load(tas_priv, 0);
>  	tas_priv->cur_prog = 0;
>  out:
> -	if (tas_priv->fw_state == TASDEVICE_DSP_FW_FAIL) {
> -		/*If DSP FW fail, kcontrol won't be created */
> -		tasdevice_config_info_remove(tas_priv);
> +	if (tas_priv->fw_state == TASDEVICE_RCA_FW_OK) {
> +		/*If DSP FW fail, DSP kcontrol won't be created */

It looks like you're no longer using PENDING and FAIL states?
The state machine is becoming really hard to follow.

>  		tasdevice_dsp_remove(tas_priv);
>  	}
>  	mutex_unlock(&tas_priv->codec_lock);
> @@ -466,14 +472,14 @@ static int tasdevice_startup(struct snd_pcm_substream *substream,
>  {
>  	struct snd_soc_component *codec = dai->component;
>  	struct tasdevice_priv *tas_priv = snd_soc_component_get_drvdata(codec);
> -	int ret = 0;
>  
> -	if (tas_priv->fw_state != TASDEVICE_DSP_FW_ALL_OK) {
> -		dev_err(tas_priv->dev, "DSP bin file not loaded\n");
> -		ret = -EINVAL;
> +	if (!(tas_priv->fw_state == TASDEVICE_DSP_FW_ALL_OK ||
> +		tas_priv->fw_state == TASDEVICE_RCA_FW_OK)) {
> +		dev_err(tas_priv->dev, "Bin file not loaded\n");
> +		return -EINVAL;
>  	}
>  
> -	return ret;
> +	return 0;
>  }
>  
>  static int tasdevice_hw_params(struct snd_pcm_substream *substream,
Shenghao Ding May 28, 2024, 3:10 a.m. UTC | #2 
Thanks for your comments.

> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Sent: Monday, May 27, 2024 9:44 PM
> To: Ding, Shenghao <shenghao-ding@ti.com>; broonie@kernel.org
> Cc: andriy.shevchenko@linux.intel.com; lgirdwood@gmail.com;
> perex@perex.cz; 13916275206@139.com; alsa-devel@alsa-project.org;
> Salazar, Ivan <i-salazar@ti.com>; linux-kernel@vger.kernel.org; Chadha,
> Jasjot Singh <j-chadha@ti.com>; liam.r.girdwood@intel.com; Yue, Jaden
> <jaden-yue@ti.com>; yung-chuan.liao@linux.intel.com; Rao, Dipa
> <dipa@ti.com>; Lu, Kevin <kevin-lu@ti.com>; yuhsuan@google.com;
> tiwai@suse.de; Xu, Baojun <baojun.xu@ti.com>; soyer@irl.hu;
> Baojun.Xu@fpt.com; judyhsiao@google.com; Navada Kanyana, Mukund
> <navada@ti.com>; cujomalainey@google.com; Kutty, Aanya
> <aanya@ti.com>; Mahmud, Nayeem <nayeem.mahmud@ti.com>
> Subject: [EXTERNAL] Re: [PATCH v2] ASoc: tas2781: Playback can work when
> only RCA binary loading without dsp firmware loading
> 
> 
> 
> On 5/24/24 20:47, Shenghao Ding wrote:
> > In only RCA binary loading case, only default dsp program inside the
> > chip will be work.
> 
> What does 'RCA' stand for?
Reconfigurable architecture binary file
> 
> Also clarify the commit title without double negatives, e.g.
> 
> "Enable RCA-based playback without DSP firmware download"
> > -	if (tas_priv->fw_state == TASDEVICE_DSP_FW_FAIL) {
> > -		dev_err(tas_priv->dev, "DSP bin file not loaded\n");
> > +	/*
> > +	 * Only RCA file loaded can still work with default dsp program inside
> > +	 * the chip?
> 
> reword the commit and remove question mark.
accept
> 
> > +	 */
> > +	if (!(tas_priv->fw_state == TASDEVICE_RCA_FW_OK ||
> > +		tas_priv->fw_state == TASDEVICE_DSP_FW_ALL_OK)) {
> > +		dev_err(tas_priv->dev, "No firmware loaded\n");
> >  		return;
> >  	}
> >
> >  	if (state == 0) {
> > -		if (tas_priv->cur_prog < tas_fmw->nr_programs) {
> > +		if (tas_fmw && tas_priv->cur_prog < tas_fmw->nr_programs)
> {
> >  			/*dsp mode or tuning mode*/
> 
> spaces in comments
accept
> 
> >  			profile_cfg_id = tas_priv->rcabin.profile_cfg_id;
> >  			tasdevice_select_tuningprm_cfg(tas_priv,
> > @@ -2340,9 +2345,10 @@ void tasdevice_tuning_switch(void *context, int
> > state)
> >
> >  		tasdevice_select_cfg_blk(tas_priv, profile_cfg_id,
> >  			TASDEVICE_BIN_BLK_PRE_POWER_UP);
> > -	} else
> > +	} else {
> >  		tasdevice_select_cfg_blk(tas_priv, profile_cfg_id,
> >  			TASDEVICE_BIN_BLK_PRE_SHUTDOWN);
> > +	}
> >  }
> >  EXPORT_SYMBOL_NS_GPL(tasdevice_tuning_switch,
> >  	SND_SOC_TAS2781_FMWLIB);
> > diff --git a/sound/soc/codecs/tas2781-i2c.c
> > b/sound/soc/codecs/tas2781-i2c.c index 9350972dfefe..ccb9313ada9b
> > 100644
> > --- a/sound/soc/codecs/tas2781-i2c.c
> > +++ b/sound/soc/codecs/tas2781-i2c.c
> > @@ -380,23 +380,30 @@ static void tasdevice_fw_ready(const struct
> firmware *fmw,
> >  	mutex_lock(&tas_priv->codec_lock);
> >
> >  	ret = tasdevice_rca_parser(tas_priv, fmw);
> > -	if (ret)
> > +	if (ret) {
> > +		tasdevice_config_info_remove(tas_priv);
> >  		goto out;
> > +	}
> >  	tasdevice_create_control(tas_priv);
> >
> >  	tasdevice_dsp_remove(tas_priv);
> >  	tasdevice_calbin_remove(tas_priv);
> > -	tas_priv->fw_state = TASDEVICE_DSP_FW_PENDING;
> > +	tas_priv->fw_state = TASDEVICE_RCA_FW_OK;
> >  	scnprintf(tas_priv->coef_binaryname, 64, "%s_coef.bin",
> >  		tas_priv->dev_name);
> > +
> >  	ret = tasdevice_dsp_parser(tas_priv);
> >  	if (ret) {
> >  		dev_err(tas_priv->dev, "dspfw load %s error\n",
> >  			tas_priv->coef_binaryname);
> > -		tas_priv->fw_state = TASDEVICE_DSP_FW_FAIL;
> >  		goto out;
> >  	}
> > -	tasdevice_dsp_create_ctrls(tas_priv);
> > +
> > +	ret = tasdevice_dsp_create_ctrls(tas_priv);
> > +	if (ret) {
> > +		dev_err(tas_priv->dev, "dsp controls error\n");
> > +		goto out;
> > +	}
> 
> this seems unrelated to the boot process? Move to a different patch?
It is a part of boot process, if no dsp-related kcontrol created, the dsp resource will be freed.
> 
> >
> >  	tas_priv->fw_state = TASDEVICE_DSP_FW_ALL_OK;
> >
> > @@ -417,9 +424,8 @@ static void tasdevice_fw_ready(const struct
> firmware *fmw,
> >  	tasdevice_prmg_load(tas_priv, 0);
> >  	tas_priv->cur_prog = 0;
> >  out:
> > -	if (tas_priv->fw_state == TASDEVICE_DSP_FW_FAIL) {
> > -		/*If DSP FW fail, kcontrol won't be created */
> > -		tasdevice_config_info_remove(tas_priv);
> > +	if (tas_priv->fw_state == TASDEVICE_RCA_FW_OK) {
> > +		/*If DSP FW fail, DSP kcontrol won't be created */
> 
> It looks like you're no longer using PENDING and FAIL states?
> The state machine is becoming really hard to follow.
Remove unused states.
> 
> >  		tasdevice_dsp_remove(tas_priv);
> >  	}
> >  	mutex_unlock(&tas_priv->codec_lock);
> > @@ -466,14 +472,14 @@ static int tasdevice_startup(struct
> > snd_pcm_substream *substream,  {
> >  	struct snd_soc_component *codec = dai->component;
> >  	struct tasdevice_priv *tas_priv =
> snd_soc_component_get_drvdata(codec);
> > -	int ret = 0;
> >
> > -	if (tas_priv->fw_state != TASDEVICE_DSP_FW_ALL_OK) {
> > -		dev_err(tas_priv->dev, "DSP bin file not loaded\n");
> > -		ret = -EINVAL;
> > +	if (!(tas_priv->fw_state == TASDEVICE_DSP_FW_ALL_OK ||
> > +		tas_priv->fw_state == TASDEVICE_RCA_FW_OK)) {
> > +		dev_err(tas_priv->dev, "Bin file not loaded\n");
> > +		return -EINVAL;
> >  	}
> >
> > -	return ret;
> > +	return 0;
> >  }
> >
> >  static int tasdevice_hw_params(struct snd_pcm_substream *substream,
Shenghao Ding May 29, 2024, 7:12 a.m. UTC | #3 
> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Sent: Monday, May 27, 2024 9:44 PM
> To: Ding, Shenghao <shenghao-ding@ti.com>; broonie@kernel.org
> Cc: andriy.shevchenko@linux.intel.com; lgirdwood@gmail.com;
> perex@perex.cz; 13916275206@139.com; alsa-devel@alsa-project.org;
> Salazar, Ivan <i-salazar@ti.com>; linux-kernel@vger.kernel.org; Chadha,
> Jasjot Singh <j-chadha@ti.com>; liam.r.girdwood@intel.com; Yue, Jaden
> <jaden-yue@ti.com>; yung-chuan.liao@linux.intel.com; Rao, Dipa
> <dipa@ti.com>; Lu, Kevin <kevin-lu@ti.com>; yuhsuan@google.com;
> tiwai@suse.de; Xu, Baojun <baojun.xu@ti.com>; soyer@irl.hu;
> Baojun.Xu@fpt.com; judyhsiao@google.com; Navada Kanyana, Mukund
> <navada@ti.com>; cujomalainey@google.com; Kutty, Aanya
> <aanya@ti.com>; Mahmud, Nayeem <nayeem.mahmud@ti.com>
> Subject: [EXTERNAL] Re: [PATCH v2] ASoc: tas2781: Playback can work when
> only RCA binary loading without dsp firmware loading
> 
> On 5/24/24 20: 47, Shenghao Ding wrote: > In only RCA binary loading case,
> only default dsp program inside the > chip will be work. What does 'RCA'
> stand for? Also clarify the commit title without double negatives, e. g. "Enable
> RCA-based ZjQcmQRYFpfptBannerStart This message was sent from outside
> of Texas Instruments.
> Do not click links or open attachments unless you recognize the source of this
> email and know the content is safe. If you wish to report this message to IT
> Security, please forward the message as an attachment to
> phishing@list.ti.com
> 
> ZjQcmQRYFpfptBannerEnd
> 
> 
> On 5/24/24 20:47, Shenghao Ding wrote:
> > In only RCA binary loading case, only default dsp program inside the
> > chip will be work.
> 
......................................................................
> > -	if (tas_priv->fw_state == TASDEVICE_DSP_FW_FAIL) {
> > -		/*If DSP FW fail, kcontrol won't be created */
> > -		tasdevice_config_info_remove(tas_priv);
> > +	if (tas_priv->fw_state == TASDEVICE_RCA_FW_OK) {
> > +		/*If DSP FW fail, DSP kcontrol won't be created */
> 
> It looks like you're no longer using PENDING and FAIL states?
> The state machine is becoming really hard to follow.
PENDING and FAIL states have been used in HDA-based tas2563/tas2781 driver

> 
> >  		tasdevice_dsp_remove(tas_priv);
> >  	}
> >  	mutex_unlock(&tas_priv->codec_lock);
> > @@ -466,14 +472,14 @@ static int tasdevice_startup(struct
> > snd_pcm_substream *substream,  {
> >  	struct snd_soc_component *codec = dai->component;
> >  	struct tasdevice_priv *tas_priv =
..................................................................................
diff mbox series

Patch

diff --git a/include/sound/tas2781-dsp.h b/include/sound/tas2781-dsp.h
index 7fba7ea26a4b..92d68ca5eafb 100644
--- a/include/sound/tas2781-dsp.h
+++ b/include/sound/tas2781-dsp.h
@@ -117,10 +117,11 @@  struct tasdevice_fw {
 	struct device *dev;
 };
 
-enum tasdevice_dsp_fw_state {
+enum tasdevice_fw_state {
 	TASDEVICE_DSP_FW_NONE = 0,
 	TASDEVICE_DSP_FW_PENDING,
 	TASDEVICE_DSP_FW_FAIL,
+	TASDEVICE_RCA_FW_OK,
 	TASDEVICE_DSP_FW_ALL_OK,
 };
 
diff --git a/sound/soc/codecs/tas2781-fmwlib.c b/sound/soc/codecs/tas2781-fmwlib.c
index 265a8ca25cbb..cfa022ef4a59 100644
--- a/sound/soc/codecs/tas2781-fmwlib.c
+++ b/sound/soc/codecs/tas2781-fmwlib.c
@@ -2324,13 +2324,18 @@  void tasdevice_tuning_switch(void *context, int state)
 	struct tasdevice_fw *tas_fmw = tas_priv->fmw;
 	int profile_cfg_id = tas_priv->rcabin.profile_cfg_id;
 
-	if (tas_priv->fw_state == TASDEVICE_DSP_FW_FAIL) {
-		dev_err(tas_priv->dev, "DSP bin file not loaded\n");
+	/*
+	 * Only RCA file loaded can still work with default dsp program inside
+	 * the chip?
+	 */
+	if (!(tas_priv->fw_state == TASDEVICE_RCA_FW_OK ||
+		tas_priv->fw_state == TASDEVICE_DSP_FW_ALL_OK)) {
+		dev_err(tas_priv->dev, "No firmware loaded\n");
 		return;
 	}
 
 	if (state == 0) {
-		if (tas_priv->cur_prog < tas_fmw->nr_programs) {
+		if (tas_fmw && tas_priv->cur_prog < tas_fmw->nr_programs) {
 			/*dsp mode or tuning mode*/
 			profile_cfg_id = tas_priv->rcabin.profile_cfg_id;
 			tasdevice_select_tuningprm_cfg(tas_priv,
@@ -2340,9 +2345,10 @@  void tasdevice_tuning_switch(void *context, int state)
 
 		tasdevice_select_cfg_blk(tas_priv, profile_cfg_id,
 			TASDEVICE_BIN_BLK_PRE_POWER_UP);
-	} else
+	} else {
 		tasdevice_select_cfg_blk(tas_priv, profile_cfg_id,
 			TASDEVICE_BIN_BLK_PRE_SHUTDOWN);
+	}
 }
 EXPORT_SYMBOL_NS_GPL(tasdevice_tuning_switch,
 	SND_SOC_TAS2781_FMWLIB);
diff --git a/sound/soc/codecs/tas2781-i2c.c b/sound/soc/codecs/tas2781-i2c.c
index 9350972dfefe..ccb9313ada9b 100644
--- a/sound/soc/codecs/tas2781-i2c.c
+++ b/sound/soc/codecs/tas2781-i2c.c
@@ -380,23 +380,30 @@  static void tasdevice_fw_ready(const struct firmware *fmw,
 	mutex_lock(&tas_priv->codec_lock);
 
 	ret = tasdevice_rca_parser(tas_priv, fmw);
-	if (ret)
+	if (ret) {
+		tasdevice_config_info_remove(tas_priv);
 		goto out;
+	}
 	tasdevice_create_control(tas_priv);
 
 	tasdevice_dsp_remove(tas_priv);
 	tasdevice_calbin_remove(tas_priv);
-	tas_priv->fw_state = TASDEVICE_DSP_FW_PENDING;
+	tas_priv->fw_state = TASDEVICE_RCA_FW_OK;
 	scnprintf(tas_priv->coef_binaryname, 64, "%s_coef.bin",
 		tas_priv->dev_name);
+
 	ret = tasdevice_dsp_parser(tas_priv);
 	if (ret) {
 		dev_err(tas_priv->dev, "dspfw load %s error\n",
 			tas_priv->coef_binaryname);
-		tas_priv->fw_state = TASDEVICE_DSP_FW_FAIL;
 		goto out;
 	}
-	tasdevice_dsp_create_ctrls(tas_priv);
+
+	ret = tasdevice_dsp_create_ctrls(tas_priv);
+	if (ret) {
+		dev_err(tas_priv->dev, "dsp controls error\n");
+		goto out;
+	}
 
 	tas_priv->fw_state = TASDEVICE_DSP_FW_ALL_OK;
 
@@ -417,9 +424,8 @@  static void tasdevice_fw_ready(const struct firmware *fmw,
 	tasdevice_prmg_load(tas_priv, 0);
 	tas_priv->cur_prog = 0;
 out:
-	if (tas_priv->fw_state == TASDEVICE_DSP_FW_FAIL) {
-		/*If DSP FW fail, kcontrol won't be created */
-		tasdevice_config_info_remove(tas_priv);
+	if (tas_priv->fw_state == TASDEVICE_RCA_FW_OK) {
+		/*If DSP FW fail, DSP kcontrol won't be created */
 		tasdevice_dsp_remove(tas_priv);
 	}
 	mutex_unlock(&tas_priv->codec_lock);
@@ -466,14 +472,14 @@  static int tasdevice_startup(struct snd_pcm_substream *substream,
 {
 	struct snd_soc_component *codec = dai->component;
 	struct tasdevice_priv *tas_priv = snd_soc_component_get_drvdata(codec);
-	int ret = 0;
 
-	if (tas_priv->fw_state != TASDEVICE_DSP_FW_ALL_OK) {
-		dev_err(tas_priv->dev, "DSP bin file not loaded\n");
-		ret = -EINVAL;
+	if (!(tas_priv->fw_state == TASDEVICE_DSP_FW_ALL_OK ||
+		tas_priv->fw_state == TASDEVICE_RCA_FW_OK)) {
+		dev_err(tas_priv->dev, "Bin file not loaded\n");
+		return -EINVAL;
 	}
 
-	return ret;
+	return 0;
 }
 
 static int tasdevice_hw_params(struct snd_pcm_substream *substream,