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

["Ding, Shenghao" <shenghao-ding@ti.com>]

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,