In only RCA binary loading case, only default dsp program inside the
chip will be work.
Fixes: ef3bcde75d06 ("ASoC: tas2781: Add tas2781 driver")
Signed-off-by: Shenghao Ding <[email protected]>
---
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 th code neater.
---
include/sound/tas2781-dsp.h | 3 ++-
sound/soc/codecs/tas2781-fmwlib.c | 14 ++++++++++----
sound/soc/codecs/tas2781-i2c.c | 30 ++++++++++++++++++------------
3 files changed, 30 insertions(+), 17 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..cfa022ef4a59 100644
--- a/sound/soc/codecs/tas2781-fmwlib.c
+++ b/sound/soc/codecs/tas2781-fmwlib.c
@@ -2324,13 +2324,18 @@ 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 file loaded can still work with default dsp program 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) {
+ 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,
@@ -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;
+ }
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 */
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,
--
2.34.1
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,
Thanks for your comments.
> -----Original Message-----
> From: Pierre-Louis Bossart <[email protected]>
> Sent: Monday, May 27, 2024 9:44 PM
> To: Ding, Shenghao <[email protected]>; [email protected]
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> Salazar, Ivan <[email protected]>; [email protected]; Chadha,
> Jasjot Singh <[email protected]>; [email protected]; Yue, Jaden
> <[email protected]>; [email protected]; Rao, Dipa
> <[email protected]>; Lu, Kevin <[email protected]>; [email protected];
> [email protected]; Xu, Baojun <[email protected]>; [email protected];
> [email protected]; [email protected]; Navada Kanyana, Mukund
> <[email protected]>; [email protected]; Kutty, Aanya
> <[email protected]>; Mahmud, Nayeem <[email protected]>
> 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,
> -----Original Message-----
> From: Pierre-Louis Bossart <[email protected]>
> Sent: Monday, May 27, 2024 9:44 PM
> To: Ding, Shenghao <[email protected]>; [email protected]
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> Salazar, Ivan <[email protected]>; [email protected]; Chadha,
> Jasjot Singh <[email protected]>; [email protected]; Yue, Jaden
> <[email protected]>; [email protected]; Rao, Dipa
> <[email protected]>; Lu, Kevin <[email protected]>; [email protected];
> [email protected]; Xu, Baojun <[email protected]>; [email protected];
> [email protected]; [email protected]; Navada Kanyana, Mukund
> <[email protected]>; [email protected]; Kutty, Aanya
> <[email protected]>; Mahmud, Nayeem <[email protected]>
> 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? Also clarify the commit title without double negatives, e. g. "Enable
> RCA-based 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
> [email protected]
>
> ZjQcmQRYFpfptBannerEnd
>
>
> 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.
>
......................................................................
> > - 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.
PENDING and FAIL states have been used in HDA-based tas2563/tas2781 driver
>
> > 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 =
..................................................................................