diff mbox series

[v3] ALSA: ASoc/tas2781: Fix wrong loading calibrated data sequence

Message ID 20240502232450.484-1-shenghao-ding@ti.com
State Superseded
Headers show
Series [v3] ALSA: ASoc/tas2781: Fix wrong loading calibrated data sequence | expand

Commit Message

Shenghao Ding May 2, 2024, 11:24 p.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: 0a0877812628 ("ASoc: tas2781: Fix spelling mistake "calibraiton"
 -> "calibration"")'
Signed-off-by: Shenghao Ding <shenghao-ding@ti.com>

---
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
 - Changed the copyright year to 2024 in the related files
 - In tas2781-dsp.h, __TASDEVICE_DSP_H__ --> __TAS2781_DSP_H__
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.
---
 include/sound/tas2781-dsp.h       |  7 +--
 sound/soc/codecs/tas2781-fmwlib.c | 99 +++++++------------------------
 sound/soc/codecs/tas2781-i2c.c    |  4 +-
 3 files changed, 27 insertions(+), 83 deletions(-)

Comments

Takashi Iwai May 3, 2024, 7:42 a.m. UTC | #1 
On Fri, 03 May 2024 01:24:49 +0200,
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.
> 
> 'Fixes: 0a0877812628 ("ASoc: tas2781: Fix spelling mistake "calibraiton"
>  -> "calibration"")'

This usage of Fixes tag is utterly wrong: first off, drop the single
quote of the whole line.  Moreover, the suggested commit looks very
dubious.  It's merely a change to correct spelling, and this can't be
the culprit of the bug itself.  Please point to the right commit.

> @@ -13,8 +13,8 @@
>  // Author: Kevin Lu <kevin-lu@ti.com>
>  //
>  
> -#ifndef __TASDEVICE_DSP_H__
> -#define __TASDEVICE_DSP_H__
> +#ifndef __TAS2781_DSP_H__
> +#define __TAS2781_DSP_H__

This is unnecessary / related change, better to keep or do it in
another patch.

> @@ -1878,7 +1878,7 @@ int tas2781_load_calibration(void *context, char *file_name,
>  {
>  	struct tasdevice_priv *tas_priv = (struct tasdevice_priv *)context;
>  	struct tasdevice *tasdev = &(tas_priv->tasdevice[i]);
> -	const struct firmware *fw_entry;
> +	const struct firmware *fw_entry = NULL;

Why is this needed?  If a NULL initialization is a must (for some
warning fix or whatever), do it in an individual fix patch instead.

The rest changes look OK, but it's a bit hard to judge because the
code has too few comments, unfortunately...


thanks,

Takashi
Andy Shevchenko May 3, 2024, 3:52 p.m. UTC | #2 
On Fri, May 03, 2024 at 07:24:49AM +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.

...

> +static void tasdev_load_calibrated_data(struct tasdevice_priv *priv, int i)
> +{

> +	struct tasdevice_fw *cal_fmw = priv->tasdevice[i].cali_data_fmw;
> +
> +	if (cal_fmw) {

Better variant is

	struct tasdevice_calibration *cal;
	struct tasdevice_fw *cal_fmw;

	cal_fmw = priv->tasdevice[i].cali_data_fmw;
	if (!cal_fmw)
		return;

> +		struct tasdevice_calibration *cal = cal_fmw->calibrations;
> +
> +		if (cal)
> +			load_calib_data(priv, &cal->dev_data);
> +	}

In the similar way

	cal = cal_fmw->calibrations;
	if (!cal)
		return;

	load_calib_data(priv, &cal->dev_data);

> +}

The result is much easier to read and understand and maintain, as it makes
harder to squeeze the code between initialization and use and breaking things.

...

> +			if (tas_priv->tasdevice[i].is_loaderr == false
> +				&& tas_priv->tasdevice[i].is_loading == true)
>  				tas_priv->tasdevice[i].cur_prog = prm_no;

While at it, make it better (fix the indentation and move operator to
the previous line):

			if (tas_priv->tasdevice[i].is_loaderr == false &&
			    tas_priv->tasdevice[i].is_loading == true)
				tas_priv->tasdevice[i].cur_prog = prm_no;

...

>  			if (tas_priv->tasdevice[i].is_loaderr == true) {
>  				status |= 1 << (i + 4);

Side note: Use BIT()

>  				continue;

...

> +			if (tas_priv->tasdevice[i].is_loaderr == false
> +				&& tas_priv->tasdevice[i].is_loading == true) {

As per above.

> +				tasdev_load_calibrated_data(tas_priv, i);
>  				tas_priv->tasdevice[i].cur_conf = cfg_no;
> +			}
diff mbox series

Patch

diff --git a/include/sound/tas2781-dsp.h b/include/sound/tas2781-dsp.h
index ea9af2726a53..7fba7ea26a4b 100644
--- a/include/sound/tas2781-dsp.h
+++ b/include/sound/tas2781-dsp.h
@@ -2,7 +2,7 @@ 
 //
 // ALSA SoC Texas Instruments TAS2781 Audio Smart Amplifier
 //
-// Copyright (C) 2022 - 2023 Texas Instruments Incorporated
+// Copyright (C) 2022 - 2024 Texas Instruments Incorporated
 // https://www.ti.com
 //
 // The TAS2781 driver implements a flexible and configurable
@@ -13,8 +13,8 @@ 
 // Author: Kevin Lu <kevin-lu@ti.com>
 //
 
-#ifndef __TASDEVICE_DSP_H__
-#define __TASDEVICE_DSP_H__
+#ifndef __TAS2781_DSP_H__
+#define __TAS2781_DSP_H__
 
 #define MAIN_ALL_DEVICES			0x0d
 #define MAIN_DEVICE_A				0x01
@@ -180,7 +180,6 @@  void tasdevice_calbin_remove(void *context);
 int tasdevice_select_tuningprm_cfg(void *context, int prm,
 	int cfg_no, int rca_conf_no);
 int tasdevice_prmg_load(void *context, int prm_no);
-int tasdevice_prmg_calibdata_load(void *context, int prm_no);
 void tasdevice_tuning_switch(void *context, int state);
 int tas2781_load_calibration(void *context, char *file_name,
 	unsigned short i);
diff --git a/sound/soc/codecs/tas2781-fmwlib.c b/sound/soc/codecs/tas2781-fmwlib.c
index 45760fe19523..2ab4c7a55b90 100644
--- a/sound/soc/codecs/tas2781-fmwlib.c
+++ b/sound/soc/codecs/tas2781-fmwlib.c
@@ -2,7 +2,7 @@ 
 //
 // tasdevice-fmw.c -- TASDEVICE firmware support
 //
-// Copyright 2023 Texas Instruments, Inc.
+// Copyright 2023 - 2024 Texas Instruments, Inc.
 //
 // Author: Shenghao Ding <shenghao-ding@ti.com>
 
@@ -1878,7 +1878,7 @@  int tas2781_load_calibration(void *context, char *file_name,
 {
 	struct tasdevice_priv *tas_priv = (struct tasdevice_priv *)context;
 	struct tasdevice *tasdev = &(tas_priv->tasdevice[i]);
-	const struct firmware *fw_entry;
+	const struct firmware *fw_entry = NULL;
 	struct tasdevice_fw *tas_fmw;
 	struct firmware fmw;
 	int offset = 0;
@@ -2151,6 +2151,18 @@  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_fw *cal_fmw = priv->tasdevice[i].cali_data_fmw;
+
+	if (cal_fmw) {
+		struct tasdevice_calibration *cal = cal_fmw->calibrations;
+
+		if (cal)
+			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 +2222,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;
-			}
 		}
 	}
 
@@ -2247,9 +2247,13 @@  int tasdevice_select_tuningprm_cfg(void *context, int prm_no,
 			if (tas_priv->tasdevice[i].is_loaderr == true) {
 				status |= 1 << (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 +2312,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;
diff --git a/sound/soc/codecs/tas2781-i2c.c b/sound/soc/codecs/tas2781-i2c.c
index b5abff230e43..9350972dfefe 100644
--- a/sound/soc/codecs/tas2781-i2c.c
+++ b/sound/soc/codecs/tas2781-i2c.c
@@ -2,7 +2,7 @@ 
 //
 // ALSA SoC Texas Instruments TAS2563/TAS2781 Audio Smart Amplifier
 //
-// Copyright (C) 2022 - 2023 Texas Instruments Incorporated
+// Copyright (C) 2022 - 2024 Texas Instruments Incorporated
 // https://www.ti.com
 //
 // The TAS2563/TAS2781 driver implements a flexible and configurable
@@ -414,7 +414,7 @@  static void tasdevice_fw_ready(const struct firmware *fmw,
 				__func__, tas_priv->cal_binaryname[i]);
 	}
 
-	tasdevice_prmg_calibdata_load(tas_priv, 0);
+	tasdevice_prmg_load(tas_priv, 0);
 	tas_priv->cur_prog = 0;
 out:
 	if (tas_priv->fw_state == TASDEVICE_DSP_FW_FAIL) {