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

[Ding, Shenghao]

Thanks for your comments.

> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> Sent: Monday, May 27, 2024 9:44 PM
> To: Ding, Shenghao <shenghao-ding@xxxxxx>; broonie@xxxxxxxxxx
> Cc: andriy.shevchenko@xxxxxxxxxxxxxxx; lgirdwood@xxxxxxxxx;
> perex@xxxxxxxx; 13916275206@xxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx;
> Salazar, Ivan <i-salazar@xxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; Chadha,
> Jasjot Singh <j-chadha@xxxxxx>; liam.r.girdwood@xxxxxxxxx; Yue, Jaden
> <jaden-yue@xxxxxx>; yung-chuan.liao@xxxxxxxxxxxxxxx; Rao, Dipa
> <dipa@xxxxxx>; Lu, Kevin <kevin-lu@xxxxxx>; yuhsuan@xxxxxxxxxx;
> tiwai@xxxxxxx; Xu, Baojun <baojun.xu@xxxxxx>; soyer@xxxxxx;
> Baojun.Xu@xxxxxxx; judyhsiao@xxxxxxxxxx; Navada Kanyana, Mukund
> <navada@xxxxxx>; cujomalainey@xxxxxxxxxx; Kutty, Aanya
> <aanya@xxxxxx>; Mahmud, Nayeem <nayeem.mahmud@xxxxxx>
> 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,