diff mbox series

[v5,2/3] ASoC: tas2781: Fix wrong loading calibrated data sequence

Message ID 20240512025040.1276-2-shenghao-ding@ti.com
State Superseded
Headers show
Series [v5,1/3] ASoC: tas2781: Fix wrong loading calibrated data sequence | expand

Commit Message

Shenghao Ding May 12, 2024, 2:50 a.m. UTC
Calibrated data will be set to default after loading DSP config params,
which will cause speaker protection work abnormally. Reload calibrated
data after loading DSP config params.

Fixes: ef3bcde75d06 ("ASoc: tas2781: Add tas2781 driver")
Signed-off-by: Shenghao Ding <shenghao-ding@ti.com>

---
v5:
 - No change.
v4:
 - Use the the culprit of the bug itself as the fixes tag.
 - Better variant for tasdev_load_calibrated_data in order to much easier
   to read and understand and maintain, as it makes harder to squeeze the
   code.
 - Fix the indentation and move operator to the previous line.
v3:
 - Remove redundant return in tasdev_load_calibrated_data
 - Put the second function parameter into the previous line for
   tasdev_load_calibrated_data
 - | Reported-by: kernel test robot <lkp@intel.com>
   | Closes: https://lore.kernel.org/oe-kbuild-all/202405021200.YHInjV43-lkp@intel.com/
v2:
 - In the Subject, fixed --> Fix
 - In tas2781-fmwlib.c, tasdevice-fmw.c ---> tas2781-fmwlib.c
 - dsp --> DSP
 - Remove unneeded parentheses for & (dereference) operator
 - Add Fixes tag
v1:
 - Download calibrated data after loading the new DSP config params
 - call tasdevice_prmg_load instead of tasdevice_prmg_calibdata_load, it
   is unnecessary to load calibrated data after loading DSP program. Load
   it after loading DSP config params each time.
 - Remove tasdevice_prmg_calibdata_load, because it is unnecessary to load
   calibrated data after loading DSP program.
---
 sound/soc/codecs/tas2781-fmwlib.c | 103 ++++++++----------------------
 1 file changed, 27 insertions(+), 76 deletions(-)

Comments

Andy Shevchenko May 13, 2024, 9:59 a.m. UTC | #1
On Sun, May 12, 2024 at 10:50:38AM +0800, Shenghao Ding wrote:
> Calibrated data will be set to default after loading DSP config params,
> which will cause speaker protection work abnormally. Reload calibrated
> data after loading DSP config params.

...

> -int tasdevice_prmg_calibdata_load(void *context, int prm_no)

> -EXPORT_SYMBOL_NS_GPL(tasdevice_prmg_calibdata_load,
> -	SND_SOC_TAS2781_FMWLIB);

AFAICS the i2c glue driver still uses this, how can't this break the build?
Shenghao Ding May 14, 2024, 6:31 a.m. UTC | #2
Hi Andy
Thanks for your comment.

> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Sent: Monday, May 13, 2024 6:00 PM
> To: Ding, Shenghao <shenghao-ding@ti.com>
> Cc: broonie@kernel.org; lgirdwood@gmail.com; perex@perex.cz; pierre-
> louis.bossart@linux.intel.com; 13916275206@139.com; alsa-devel@alsa-
> project.org; linux-kernel@vger.kernel.org; liam.r.girdwood@intel.com;
> bard.liao@intel.com; yung-chuan.liao@linux.intel.com; Lu, Kevin <kevin-
> lu@ti.com>; cameron.berkenpas@gmail.com; tiwai@suse.de; Xu, Baojun
> <baojun.xu@ti.com>; soyer@irl.hu; Baojun.Xu@fpt.com
> Subject: [EXTERNAL] Re: [PATCH v5 2/3] ASoC: tas2781: Fix wrong loading
> calibrated data sequence
> 
> On Sun, May 12, 2024 at 10:50:38AM +0800, Shenghao Ding wrote:
> > Calibrated data will be set to default after loading DSP config
> > params, which will cause speaker protection work abnormally. Reload
> > calibrated data after loading DSP config params.
> 
> ...
> 
> > -int tasdevice_prmg_calibdata_load(void *context, int prm_no)
> 
> > -EXPORT_SYMBOL_NS_GPL(tasdevice_prmg_calibdata_load,
> > -	SND_SOC_TAS2781_FMWLIB);
> 
> AFAICS the i2c glue driver still uses this, how can't this break the build?
[PATCH v5 3/3] has removed tasdevice_prmg_calibdata_load, and 
tasdevice_prmg_load(tas_priv, 0) has been called instead of 
tasdevice_prmg_calibdata_load
> 
> --
> With Best Regards,
> Andy Shevchenko
>
diff mbox series

Patch

diff --git a/sound/soc/codecs/tas2781-fmwlib.c b/sound/soc/codecs/tas2781-fmwlib.c
index a6be81adcb83..265a8ca25cbb 100644
--- a/sound/soc/codecs/tas2781-fmwlib.c
+++ b/sound/soc/codecs/tas2781-fmwlib.c
@@ -2151,6 +2151,24 @@  static int tasdevice_load_data(struct tasdevice_priv *tas_priv,
 	return ret;
 }
 
+static void tasdev_load_calibrated_data(struct tasdevice_priv *priv, int i)
+{
+	struct tasdevice_calibration *cal;
+	struct tasdevice_fw *cal_fmw;
+
+	cal_fmw = priv->tasdevice[i].cali_data_fmw;
+
+	/* No calibrated data for current devices, playback will go ahead. */
+	if (!cal_fmw)
+		return;
+
+	cal = cal_fmw->calibrations;
+	if (cal)
+		return;
+
+	load_calib_data(priv, &cal->dev_data);
+}
+
 int tasdevice_select_tuningprm_cfg(void *context, int prm_no,
 	int cfg_no, int rca_conf_no)
 {
@@ -2210,21 +2228,9 @@  int tasdevice_select_tuningprm_cfg(void *context, int prm_no,
 		for (i = 0; i < tas_priv->ndev; i++) {
 			if (tas_priv->tasdevice[i].is_loaderr == true)
 				continue;
-			else if (tas_priv->tasdevice[i].is_loaderr == false
-				&& tas_priv->tasdevice[i].is_loading == true) {
-				struct tasdevice_fw *cal_fmw =
-					tas_priv->tasdevice[i].cali_data_fmw;
-
-				if (cal_fmw) {
-					struct tasdevice_calibration
-						*cal = cal_fmw->calibrations;
-
-					if (cal)
-						load_calib_data(tas_priv,
-							&(cal->dev_data));
-				}
+			if (tas_priv->tasdevice[i].is_loaderr == false &&
+				tas_priv->tasdevice[i].is_loading == true)
 				tas_priv->tasdevice[i].cur_prog = prm_no;
-			}
 		}
 	}
 
@@ -2245,11 +2251,15 @@  int tasdevice_select_tuningprm_cfg(void *context, int prm_no,
 		tasdevice_load_data(tas_priv, &(conf->dev_data));
 		for (i = 0; i < tas_priv->ndev; i++) {
 			if (tas_priv->tasdevice[i].is_loaderr == true) {
-				status |= 1 << (i + 4);
+				status |= BIT(i + 4);
 				continue;
-			} else if (tas_priv->tasdevice[i].is_loaderr == false
-				&& tas_priv->tasdevice[i].is_loading == true)
+			}
+
+			if (tas_priv->tasdevice[i].is_loaderr == false &&
+				tas_priv->tasdevice[i].is_loading == true) {
+				tasdev_load_calibrated_data(tas_priv, i);
 				tas_priv->tasdevice[i].cur_conf = cfg_no;
+			}
 		}
 	} else
 		dev_dbg(tas_priv->dev, "%s: Unneeded loading dsp conf %d\n",
@@ -2308,65 +2318,6 @@  int tasdevice_prmg_load(void *context, int prm_no)
 }
 EXPORT_SYMBOL_NS_GPL(tasdevice_prmg_load, SND_SOC_TAS2781_FMWLIB);
 
-int tasdevice_prmg_calibdata_load(void *context, int prm_no)
-{
-	struct tasdevice_priv *tas_priv = (struct tasdevice_priv *) context;
-	struct tasdevice_fw *tas_fmw = tas_priv->fmw;
-	struct tasdevice_prog *program;
-	int prog_status = 0;
-	int i;
-
-	if (!tas_fmw) {
-		dev_err(tas_priv->dev, "%s: Firmware is NULL\n", __func__);
-		goto out;
-	}
-
-	if (prm_no >= tas_fmw->nr_programs) {
-		dev_err(tas_priv->dev,
-			"%s: prm(%d) is not in range of Programs %u\n",
-			__func__, prm_no, tas_fmw->nr_programs);
-		goto out;
-	}
-
-	for (i = 0, prog_status = 0; i < tas_priv->ndev; i++) {
-		if (prm_no >= 0 && tas_priv->tasdevice[i].cur_prog != prm_no) {
-			tas_priv->tasdevice[i].cur_conf = -1;
-			tas_priv->tasdevice[i].is_loading = true;
-			prog_status++;
-		}
-		tas_priv->tasdevice[i].is_loaderr = false;
-	}
-
-	if (prog_status) {
-		program = &(tas_fmw->programs[prm_no]);
-		tasdevice_load_data(tas_priv, &(program->dev_data));
-		for (i = 0; i < tas_priv->ndev; i++) {
-			if (tas_priv->tasdevice[i].is_loaderr == true)
-				continue;
-			else if (tas_priv->tasdevice[i].is_loaderr == false
-				&& tas_priv->tasdevice[i].is_loading == true) {
-				struct tasdevice_fw *cal_fmw =
-					tas_priv->tasdevice[i].cali_data_fmw;
-
-				if (cal_fmw) {
-					struct tasdevice_calibration *cal =
-						cal_fmw->calibrations;
-
-					if (cal)
-						load_calib_data(tas_priv,
-							&(cal->dev_data));
-				}
-				tas_priv->tasdevice[i].cur_prog = prm_no;
-			}
-		}
-	}
-
-out:
-	return prog_status;
-}
-EXPORT_SYMBOL_NS_GPL(tasdevice_prmg_calibdata_load,
-	SND_SOC_TAS2781_FMWLIB);
-
 void tasdevice_tuning_switch(void *context, int state)
 {
 	struct tasdevice_priv *tas_priv = (struct tasdevice_priv *) context;