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 <[email protected]>
---
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 | 3 ++-
sound/soc/codecs/tas2781-fmwlib.c | 16 ++++++++++-----
sound/soc/codecs/tas2781-i2c.c | 33 ++++++++++++++++++++-----------
3 files changed, 34 insertions(+), 18 deletions(-)
diff --git a/include/sound/tas2781-dsp.h b/include/sound/tas2781-dsp.h
index 7fba7ea26a4b..92d68ca5eafb 100644
--- a/include/sound/tas2781-dsp.h
+++ b/include/sound/tas2781-dsp.h
@@ -117,10 +117,11 @@ struct tasdevice_fw {
struct device *dev;
};
-enum tasdevice_dsp_fw_state {
+enum tasdevice_fw_state {
TASDEVICE_DSP_FW_NONE = 0,
TASDEVICE_DSP_FW_PENDING,
TASDEVICE_DSP_FW_FAIL,
+ TASDEVICE_RCA_FW_OK,
TASDEVICE_DSP_FW_ALL_OK,
};
diff --git a/sound/soc/codecs/tas2781-fmwlib.c b/sound/soc/codecs/tas2781-fmwlib.c
index 265a8ca25cbb..5db1a0ac6036 100644
--- a/sound/soc/codecs/tas2781-fmwlib.c
+++ b/sound/soc/codecs/tas2781-fmwlib.c
@@ -2324,14 +2324,19 @@ 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?
+ */
+ 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) {
- /*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 +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..9b85a44511c2 100644
--- a/sound/soc/codecs/tas2781-i2c.c
+++ b/sound/soc/codecs/tas2781-i2c.c
@@ -380,23 +380,33 @@ 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 +427,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 +475,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,
--
2.34.1
On Wed, May 29, 2024 at 06:35:41PM +0800, Shenghao Ding wrote:
> In only RCA(Reconfigurable Architecture) binary case, no DSP program will
"...RCA (..."
> be working inside tas2563/tas2781, that is dsp-bypass mode, do not support
> speaker protection, and audio acoustic algorithms in this mode.
Some minor issues below.
..
> - 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?
> + */
> + if (!(tas_priv->fw_state == TASDEVICE_RCA_FW_OK ||
> + tas_priv->fw_state == TASDEVICE_DSP_FW_ALL_OK)) {
This line has broken indentation and I already pointed out a few times to
such issues. It makes harder to read the code.
> + dev_err(tas_priv->dev, "No firmware loaded\n");
> return;
> }
..
> scnprintf(tas_priv->coef_binaryname, 64, "%s_coef.bin",
> tas_priv->dev_name);
> +
Stray change?
> ret = tasdevice_dsp_parser(tas_priv);
..
> + if (tas_priv->fw_state == TASDEVICE_RCA_FW_OK) {
> + /*If DSP FW fail, DSP kcontrol won't be created */
Mind spaces in the comment.
> tasdevice_dsp_remove(tas_priv);
> }
..
> - 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)) {
Broken indentation.
> + dev_err(tas_priv->dev, "Bin file not loaded\n");
> + return -EINVAL;
> }
--
With Best Regards,
Andy Shevchenko
> 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.
The commit title and body are much better, thank you!
> -enum tasdevice_dsp_fw_state {
> +enum tasdevice_fw_state {
> TASDEVICE_DSP_FW_NONE = 0,
> TASDEVICE_DSP_FW_PENDING,
> TASDEVICE_DSP_FW_FAIL,
> + TASDEVICE_RCA_FW_OK,
> TASDEVICE_DSP_FW_ALL_OK,
You should really add a description of what the state machine looks
like, and why FW_PENDING/FAIL remain but are not used.
> };
>
> diff --git a/sound/soc/codecs/tas2781-fmwlib.c b/sound/soc/codecs/tas2781-fmwlib.c
> index 265a8ca25cbb..5db1a0ac6036 100644
> --- a/sound/soc/codecs/tas2781-fmwlib.c
> +++ b/sound/soc/codecs/tas2781-fmwlib.c
> @@ -2324,14 +2324,19 @@ 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?
> + */
> + 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) {
> - /*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 +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..9b85a44511c2 100644
> --- a/sound/soc/codecs/tas2781-i2c.c
> +++ b/sound/soc/codecs/tas2781-i2c.c
> @@ -380,23 +380,33 @@ 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 +427,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 */
please add spaces on each side of a comment.
> tasdevice_dsp_remove(tas_priv);
So the state "FW_OK" means a fail now? It's not clear if this branch
will work in all cases, including the original configuration where RCA
was NOT used.
Your previous explanation on the states is very hard to follow
>> It looks like you're no longer using PENDING and FAIL states?
> The state machine is becoming really hard to follow.
PENDING and FAIL states have been used in HDA-based tas2563/tas2781 driver
Not following why the DSP boot state would depend on what interface is
used to load firmware?
If this is because the definition is included in two separate drivers,
is this ok to add an intermediate state on one side but not the other?
Is there any merit to a shared definition if the states are completely
different?
> }
> mutex_unlock(&tas_priv->codec_lock);
> @@ -466,14 +475,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,