[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <6a8ab4f6-63be-4284-814a-d853faf000b5@linux.intel.com>
Date: Mon, 27 May 2024 08:44:06 -0500
From: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To: Shenghao Ding <shenghao-ding@...com>, broonie@...nel.org
Cc: andriy.shevchenko@...ux.intel.com, lgirdwood@...il.com, perex@...ex.cz,
13916275206@....com, alsa-devel@...a-project.org, i-salazar@...com,
linux-kernel@...r.kernel.org, j-chadha@...com, liam.r.girdwood@...el.com,
jaden-yue@...com, yung-chuan.liao@...ux.intel.com, dipa@...com,
kevin-lu@...com, yuhsuan@...gle.com, tiwai@...e.de, baojun.xu@...com,
soyer@....hu, Baojun.Xu@....com, judyhsiao@...gle.com, navada@...com,
cujomalainey@...gle.com, aanya@...com, nayeem.mahmud@...com
Subject: 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 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,
Powered by blists - more mailing lists