| Message ID | 20240606124105.1492-1-shenghao-ding@ti.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RESEND,v4] ASoc: tas2781: Enable RCA-based playback without DSP firmware download | expand |
I am afraid there are still circular logic issues, or the code/comments don't capture what you are trying to do.... > diff --git a/include/sound/tas2781-dsp.h b/include/sound/tas2781-dsp.h > index 7fba7ea26a4b..3cda9da14f6d 100644 > --- a/include/sound/tas2781-dsp.h > +++ b/include/sound/tas2781-dsp.h > @@ -117,10 +117,17 @@ struct tasdevice_fw { > struct device *dev; > }; > > -enum tasdevice_dsp_fw_state { > - TASDEVICE_DSP_FW_NONE = 0, > +enum tasdevice_fw_state { > + /* Driver in startup mode, not load any firmware. */ > TASDEVICE_DSP_FW_PENDING, > + /* DSP firmware in the system, but parsing error. */ > TASDEVICE_DSP_FW_FAIL, > + /* > + * Only RCA (Reconfigurable Architecture) firmware load > + * successfully. > + */ > + TASDEVICE_RCA_FW_OK, > + /* Both RCA and DSP firmware load successfully. */ > TASDEVICE_DSP_FW_ALL_OK, > }; > > diff --git a/sound/soc/codecs/tas2781-fmwlib.c b/sound/soc/codecs/tas2781-fmwlib.c > index 265a8ca25cbb..838d29fead96 100644 > --- a/sound/soc/codecs/tas2781-fmwlib.c > +++ b/sound/soc/codecs/tas2781-fmwlib.c > @@ -2324,14 +2324,21 @@ 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-based Playback can still work with no dsp program running > + * inside the chip. > + */ > + switch (tas_priv->fw_state) { > + case TASDEVICE_RCA_FW_OK: > + case TASDEVICE_DSP_FW_ALL_OK: [1] so according to both the comment and code the RCA_FW_OK is fine and the device 'works', but ... > + break; > + default: > return; > } > > if (state == 0) { > - if (tas_priv->cur_prog < tas_fmw->nr_programs) { > - /*dsp mode or tuning mode*/ > + 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, > tas_priv->cur_prog, tas_priv->cur_conf, > @@ -2340,9 +2347,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..9c3c89cb36de 100644 > --- a/sound/soc/codecs/tas2781-i2c.c > +++ b/sound/soc/codecs/tas2781-i2c.c > @@ -380,23 +380,32 @@ 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; ... in this branch the fw_state remains TASDEVICE_RCA_FW_OK, so > goto out; > } > - tasdevice_dsp_create_ctrls(tas_priv); > + > + /* > + * If no dsp-related kcontrol created, the dsp resource will be freed. > + */ > + ret = tasdevice_dsp_create_ctrls(tas_priv); > + if (ret) { > + dev_err(tas_priv->dev, "dsp controls error\n"); > + goto out; > + } ... we never reach this line ... > > tas_priv->fw_state = TASDEVICE_DSP_FW_ALL_OK; > > @@ -417,9 +426,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. */ ... and here this TASDEVICE_RCA_FW_OK really means a fail. so how can [1] consider TASDEVICE_RCA_FW_OK as a success case? Or this this saying that the baseline is the RCA case, and then the code attempts to load firmware but in case of failures just keep going, i.e. failing to load firmware is NOT an error? That would be somewhat different to the commit title that says 'without DSP firmware download'. Would you mind clarifying the steps please? > tasdevice_dsp_remove(tas_priv); > } > mutex_unlock(&tas_priv->codec_lock); > @@ -466,14 +474,14 @@ static int tasdevice_startup(struct snd_pcm_substream *substream, > { > struct snd_soc_component *codec = dai->component; > struct tasdevice_priv TASDEVICE_RCA_FW_OK*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; > + switch (tas_priv->fw_state) { > + case TASDEVICE_RCA_FW_OK: > + case TASDEVICE_DSP_FW_ALL_OK: > + return 0; > + default: > + return -EINVAL; > } > - > - return ret; > } > > static int tasdevice_hw_params(struct snd_pcm_substream *substream,
Hi Pierre Sorry for missing your valuable review comments, thanks for Broonie's remind. > -----Original Message----- > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > Sent: Thursday, June 6, 2024 9:09 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; zhourui@huaqin.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>; yuhsuan@google.com; > Lo, Henry <henry.lo@ti.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>; > savyasanchi.shukla@netradyne.com; flaviopr@microsoft.com; Ji, Jesse <jesse- > ji@ti.com>; darren.ye@mediatek.com > Subject: [EXTERNAL] Re: [RESEND PATCH v4] ASoc: tas2781: Enable RCA-based > playback without DSP firmware download > > I am afraid there are still circular logic issues, or the code/comments don't > capture what you are trying to do. . . . > diff --git a/include/sound/tas2781- > dsp. h b/include/sound/tas2781-dsp. h > index 7fba7ea26a4b. . 3cda9da14f6d > 100644 > 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 > I am afraid there are still circular logic issues, or the code/comments don't > capture what you are trying to do.... > > .................................. > ... and here this TASDEVICE_RCA_FW_OK really means a fail. > > so how can [1] consider TASDEVICE_RCA_FW_OK as a success case? > > Or this this saying that the baseline is the RCA case, and then the code > attempts to load firmware but in case of failures just keep going, i.e. > failing to load firmware is NOT an error? Correct. > > That would be somewhat different to the commit title that says 'without DSP > firmware download'. > > Would you mind clarifying the steps please? There's two bin files for tas2781, one is register settings(RCA bin file), the other is the dsp firmware and filter coeff. If no RCA bin file is load, the tas2781 can't work, it will be TASDEVICE_DSP_FW_FAIL. If only RCA bin file load, the tas2781 will work in bypass mode, which dsp do not work, neither spk protection nor acoustic algorithm is enabled (TASDEVICE_RCA_FW_OK). If both RCA bin and dsp firmware are loaded, that is TASDEVICE_DSP_FW_ALL_OK, tas2781 work in dsp mode, both spk protection and acoustic algorithm are enabled > > > > tasdevice_dsp_remove(tas_priv); > > } > > mutex_unlock(&tas_priv->codec_lock); > > @@ -466,14 +474,14 @@ static int tasdevice_startup(struct > > snd_pcm_substream *substream, { > > struct snd_soc_component *codec = dai->component; > > struct tasdevice_priv TASDEVICE_RCA_FW_OK*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; > > + switch (tas_priv->fw_state) { > > + case TASDEVICE_RCA_FW_OK: > > + case TASDEVICE_DSP_FW_ALL_OK: > > + return 0; > > + default: > > + return -EINVAL; > > } > > - > > - return ret; > > } > > > > static int tasdevice_hw_params(struct snd_pcm_substream *substream,
>> Or this this saying that the baseline is the RCA case, and then the code >> attempts to load firmware but in case of failures just keep going, i.e. >> failing to load firmware is NOT an error? > Correct. >> >> That would be somewhat different to the commit title that says 'without DSP >> firmware download'. >> >> Would you mind clarifying the steps please? > There's two bin files for tas2781, one is register settings(RCA bin file), the other is the dsp firmware and filter coeff. > If no RCA bin file is load, the tas2781 can't work, it will be TASDEVICE_DSP_FW_FAIL. > If only RCA bin file load, the tas2781 will work in bypass mode, which dsp do not work, neither spk protection nor acoustic > algorithm is enabled > (TASDEVICE_RCA_FW_OK). > If both RCA bin and dsp firmware are loaded, that is TASDEVICE_DSP_FW_ALL_OK, tas2781 work in dsp mode, both spk protection > and acoustic algorithm are enabled Now I get it, and I guess I was thrown off by the title of your commit message and previous comments that the DSP_FW_FAIL state is used for the HDaudio mode. It's not that the RCA mode is enabled by this patch. It was present already in the existing driver code. This patch allows this RCA mode to become a fallback if the DSP firmware load fails, but the DSP_FW_FAIL is still used on RCA bin load problems. So you may want to clarify the commit title and message, but from a code perspective things looks ok: Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> >>> tasdevice_dsp_remove(tas_priv); >>> } >>> mutex_unlock(&tas_priv->codec_lock); >>> @@ -466,14 +474,14 @@ static int tasdevice_startup(struct >>> snd_pcm_substream *substream, { >>> struct snd_soc_component *codec = dai->component; >>> struct tasdevice_priv TASDEVICE_RCA_FW_OK*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; >>> + switch (tas_priv->fw_state) { >>> + case TASDEVICE_RCA_FW_OK: >>> + case TASDEVICE_DSP_FW_ALL_OK: >>> + return 0; >>> + default: >>> + return -EINVAL; >>> } >>> - >>> - return ret; >>> } >>> >>> static int tasdevice_hw_params(struct snd_pcm_substream *substream,
diff --git a/include/sound/tas2781-dsp.h b/include/sound/tas2781-dsp.h index 7fba7ea26a4b..3cda9da14f6d 100644 --- a/include/sound/tas2781-dsp.h +++ b/include/sound/tas2781-dsp.h @@ -117,10 +117,17 @@ struct tasdevice_fw { struct device *dev; }; -enum tasdevice_dsp_fw_state { - TASDEVICE_DSP_FW_NONE = 0, +enum tasdevice_fw_state { + /* Driver in startup mode, not load any firmware. */ TASDEVICE_DSP_FW_PENDING, + /* DSP firmware in the system, but parsing error. */ TASDEVICE_DSP_FW_FAIL, + /* + * Only RCA (Reconfigurable Architecture) firmware load + * successfully. + */ + TASDEVICE_RCA_FW_OK, + /* Both RCA and DSP firmware load successfully. */ TASDEVICE_DSP_FW_ALL_OK, }; diff --git a/sound/soc/codecs/tas2781-fmwlib.c b/sound/soc/codecs/tas2781-fmwlib.c index 265a8ca25cbb..838d29fead96 100644 --- a/sound/soc/codecs/tas2781-fmwlib.c +++ b/sound/soc/codecs/tas2781-fmwlib.c @@ -2324,14 +2324,21 @@ 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-based Playback can still work with no dsp program running + * inside the chip. + */ + switch (tas_priv->fw_state) { + case TASDEVICE_RCA_FW_OK: + case TASDEVICE_DSP_FW_ALL_OK: + break; + default: return; } if (state == 0) { - if (tas_priv->cur_prog < tas_fmw->nr_programs) { - /*dsp mode or tuning mode*/ + 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, tas_priv->cur_prog, tas_priv->cur_conf, @@ -2340,9 +2347,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..9c3c89cb36de 100644 --- a/sound/soc/codecs/tas2781-i2c.c +++ b/sound/soc/codecs/tas2781-i2c.c @@ -380,23 +380,32 @@ 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); + + /* + * If no dsp-related kcontrol created, the dsp resource will be freed. + */ + 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 +426,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 +474,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; + switch (tas_priv->fw_state) { + case TASDEVICE_RCA_FW_OK: + case TASDEVICE_DSP_FW_ALL_OK: + return 0; + default: + return -EINVAL; } - - return ret; } static int tasdevice_hw_params(struct snd_pcm_substream *substream,
In only RCA (Reconfigurable Architecture) binary case, no DSP program will be working inside tas2563/tas2781, that is dsp-bypass mode, do not support speaker protection, and audio acoustic algorithms in this mode. Fixes: ef3bcde75d06 ("ASoC: tas2781: Add tas2781 driver") Signed-off-by: Shenghao Ding <shenghao-ding@ti.com> --- v4: - add a description of what the state machine looks like, and why FW_PENDING/FAIL remain but are not used. - Remove TASDEVICE_DSP_FW_NONE because of unused. - Remove stray change. - Fix broken indentation. v3: - Add description on RCA is Reconfigurable Architecture. - Add the description on enabling - Reword the commit - Remove question mark in the comments. - Add spaces in comments. 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 the code neater. --- include/sound/tas2781-dsp.h | 11 ++++++++-- sound/soc/codecs/tas2781-fmwlib.c | 18 +++++++++++----- sound/soc/codecs/tas2781-i2c.c | 34 +++++++++++++++++++------------ 3 files changed, 43 insertions(+), 20 deletions(-)