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,
