ALSA: hda/tas2781: fixes for 6.9-rc1

PATCH Superseded Gergo Koteles 5 participants 10 messages Mar 25, 2024 – Mar 27, 2024

Subsystems (3)

Stable branch Sound Texas Instruments audio (ASoC/HDA) drivers

lore

Share & tools

lore

Gergo Koteles

author

2024-03-25 21:25 UTC #0 lore raw

cover ALSA: hda/tas2781: fixes for 6.9-rc1

Hi,

This series removes the no longer needed digital gain kcontrol, which caused problems in tas2563, because the register does not exists there.

This series also adds locking and debug statements to the other kcontrols. They sometimes ran in parallel with tasdev_fw_ready, and caused weird sound problems.
https://github.com/tomsom/yoga-linux/issues/58

Regards,
Gergo

Gergo Koteles (3):
ALSA: hda/tas2781: remove digital gain kcontrol
ALSA: hda/tas2781: add locks to kcontrols
ALSA: hda/tas2781: add debug statements to kcontrols

 include/sound/tas2781-tlv.h     |   1 -
 sound/pci/hda/tas2781_hda_i2c.c | 103 +++++++++++++++++++++-----------
 2 files changed, 68 insertions(+), 36 deletions(-)

base-commit: 4cece764965020c22cff7665b18a012006359095

Signature · 1 line

2.44.0

Gergo Koteles

author

2024-03-25 21:25 UTC #1 lore raw

PATCH 1/3 ALSA: hda/tas2781: remove digital gain kcontrol

The "Speaker Digital Gain" kcontrol controls the TAS2781_DVC_LVL (0x1A) register. Unfortunately the tas2563 does not have DVC_LVL, but has INT_MASK0 in 0x1A, which has been misused so far.

Since commit c1947ce61ff4 ("ALSA: hda/realtek: tas2781: enable subwoofer volume control") the volume of the tas2781 amplifiers can be controlled by the master volume, so this digital gain kcontrol is not needed.

Remove it.

Fixes 5be27f1e3ec9 ("ALSA: hda/tas2781: Add tas2781 HDA driver")

CC [email protected]

Signed-off-by Gergo Koteles <[email protected]>

---
 include/sound/tas2781-tlv.h     |  1 -
 sound/pci/hda/tas2781_hda_i2c.c | 37 +--------------------------------
 2 files changed, 1 insertion(+), 37 deletions(-)

include/sound/tas2781-tlv.h +0 -1

index 4038dd421150..a29bcfb5bb2b 100644
@@ -15,7 +15,6 @@
 #ifndef __TAS2781_TLV_H__
 #define __TAS2781_TLV_H__
 
-static const DECLARE_TLV_DB_SCALE(dvc_tlv, -10000, 100, 0);
 static const DECLARE_TLV_DB_SCALE(amp_vol_tlv, 1100, 50, 0);
 
 #endif

sound/pci/hda/tas2781_hda_i2c.c +1 ~~-36~~

index 4475cea8e9f7..5acb475c10a7 100644
@@ -89,7 +89,7 @@ struct tas2781_hda {
 	struct snd_kcontrol *dsp_prog_ctl;
 	struct snd_kcontrol *dsp_conf_ctl;
 	struct snd_kcontrol *prof_ctl;
-	struct snd_kcontrol *snd_ctls[3];
+	struct snd_kcontrol *snd_ctls[2];
 };
 
 static int tas2781_get_i2c_res(struct acpi_resource *ares, void *data)

@@ -294,27 +294,6 @@ static int tasdevice_config_put(struct snd_kcontrol *kcontrol,
 	return ret;
 }
 
-/*
- * tas2781_digital_getvol - get the volum control
- * @kcontrol: control pointer
- * @ucontrol: User data
- * Customer Kcontrol for tas2781 is primarily for regmap booking, paging
- * depends on internal regmap mechanism.
- * tas2781 contains book and page two-level register map, especially
- * book switching will set the register BXXP00R7F, after switching to the
- * correct book, then leverage the mechanism for paging to access the
- * register.
- */
-static int tas2781_digital_getvol(struct snd_kcontrol *kcontrol,
-	struct snd_ctl_elem_value *ucontrol)
-{
-	struct tasdevice_priv *tas_priv = snd_kcontrol_chip(kcontrol);
-	struct soc_mixer_control *mc =
-		(struct soc_mixer_control *)kcontrol->private_value;
-
-	return tasdevice_digital_getvol(tas_priv, ucontrol, mc);
-}
-
 static int tas2781_amp_getvol(struct snd_kcontrol *kcontrol,
 	struct snd_ctl_elem_value *ucontrol)
 {

@@ -325,17 +304,6 @@ static int tas2781_amp_getvol(struct snd_kcontrol *kcontrol,
 	return tasdevice_amp_getvol(tas_priv, ucontrol, mc);
 }
 
-static int tas2781_digital_putvol(struct snd_kcontrol *kcontrol,
-	struct snd_ctl_elem_value *ucontrol)
-{
-	struct tasdevice_priv *tas_priv = snd_kcontrol_chip(kcontrol);
-	struct soc_mixer_control *mc =
-		(struct soc_mixer_control *)kcontrol->private_value;
-
-	/* The check of the given value is in tasdevice_digital_putvol. */
-	return tasdevice_digital_putvol(tas_priv, ucontrol, mc);
-}
-
 static int tas2781_amp_putvol(struct snd_kcontrol *kcontrol,
 	struct snd_ctl_elem_value *ucontrol)
 {

@@ -381,9 +349,6 @@ static const struct snd_kcontrol_new tas2781_snd_controls[] = {
 	ACARD_SINGLE_RANGE_EXT_TLV("Speaker Analog Gain", TAS2781_AMP_LEVEL,
 		1, 0, 20, 0, tas2781_amp_getvol,
 		tas2781_amp_putvol, amp_vol_tlv),
-	ACARD_SINGLE_RANGE_EXT_TLV("Speaker Digital Gain", TAS2781_DVC_LVL,
-		0, 0, 200, 1, tas2781_digital_getvol,
-		tas2781_digital_putvol, dvc_tlv),
 	ACARD_SINGLE_BOOL_EXT("Speaker Force Firmware Load", 0,
 		tas2781_force_fwload_get, tas2781_force_fwload_put),
 };

Signature · 1 line

2.44.0

kernel test robot

build bot

2024-03-26 12:58 UTC #2 lore raw

Re: [PATCH 1/3] ALSA: hda/tas2781: remove digital gain kcontrol

kernel test robot noticed the following build errors:

Hi Gergo,

kernel test robot noticed the following build errors:

[auto build test ERROR on 4cece764965020c22cff7665b18a012006359095]

url:    https://github.com/intel-lab-lkp/linux/commits/Gergo-Koteles/ALSA-hda-tas2781-remove-digital-gain-kcontrol/20240326-052937
base:   4cece764965020c22cff7665b18a012006359095
patch link:    https://lore.kernel.org/r/313e00499eb2caadd23a92284fdec418b650b7f4.1711401621.git.soyer%40irl.hu
patch subject: [PATCH 1/3] ALSA: hda/tas2781: remove digital gain kcontrol
config: i386-buildonly-randconfig-004-20240326 (https://download.01.org/0day-ci/archive/20240326/[email protected]/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240326/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All errors (new ones prefixed by >>):

In file included from sound/soc/codecs/tas2781-i2c.c:29:

>> sound/soc/codecs/tas2781-i2c.c:148:41: error: 'dvc_tlv' undeclared here (not in a function)
     148 |                 tas2781_digital_putvol, dvc_tlv),
         |                                         ^~~~~~~
   include/sound/soc.h:271:19: note: in definition of macro 'SOC_SINGLE_RANGE_EXT_TLV'
     271 |         .tlv.p = (tlv_array), \
         |                   ^~~~~~~~~

vim +/dvc_tlv +148 sound/soc/codecs/tas2781-i2c.c

ef3bcde75d06d6 Shenghao Ding 2023-06-18  141  
ef3bcde75d06d6 Shenghao Ding 2023-06-18  142  static const struct snd_kcontrol_new tas2781_snd_controls[] = {
ef3bcde75d06d6 Shenghao Ding 2023-06-18  143  	SOC_SINGLE_RANGE_EXT_TLV("Speaker Analog Gain", TAS2781_AMP_LEVEL,
ef3bcde75d06d6 Shenghao Ding 2023-06-18  144  		1, 0, 20, 0, tas2781_amp_getvol,
ef3bcde75d06d6 Shenghao Ding 2023-06-18  145  		tas2781_amp_putvol, amp_vol_tlv),
ef3bcde75d06d6 Shenghao Ding 2023-06-18  146  	SOC_SINGLE_RANGE_EXT_TLV("Speaker Digital Gain", TAS2781_DVC_LVL,
ef3bcde75d06d6 Shenghao Ding 2023-06-18  147  		0, 0, 200, 1, tas2781_digital_getvol,
ef3bcde75d06d6 Shenghao Ding 2023-06-18 @148  		tas2781_digital_putvol, dvc_tlv),
ef3bcde75d06d6 Shenghao Ding 2023-06-18  149  	SOC_SINGLE_BOOL_EXT("Speaker Force Firmware Load", 0,
ef3bcde75d06d6 Shenghao Ding 2023-06-18  150  		tas2781_force_fwload_get, tas2781_force_fwload_put),
ef3bcde75d06d6 Shenghao Ding 2023-06-18  151  };

ef3bcde75d06d6 Shenghao Ding 2023-06-18 152

Signature · 2 lines

0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

kernel test robot

build bot

2024-03-27 04:22 UTC #3 lore raw

Re: [PATCH 1/3] ALSA: hda/tas2781: remove digital gain kcontrol

In reply to Gergo Koteles

kernel test robot noticed the following build errors:

Hi Gergo,

kernel test robot noticed the following build errors:

[auto build test ERROR on 4cece764965020c22cff7665b18a012006359095]

url:    https://github.com/intel-lab-lkp/linux/commits/Gergo-Koteles/ALSA-hda-tas2781-remove-digital-gain-kcontrol/20240326-052937
base:   4cece764965020c22cff7665b18a012006359095
patch link:    https://lore.kernel.org/r/313e00499eb2caadd23a92284fdec418b650b7f4.1711401621.git.soyer%40irl.hu
patch subject: [PATCH 1/3] ALSA: hda/tas2781: remove digital gain kcontrol
config: i386-randconfig-011-20240326 (https://download.01.org/0day-ci/archive/20240327/[email protected]/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240327/[email protected]/reproduce)

All errors (new ones prefixed by >>):

>> sound/soc/codecs/tas2781-i2c.c:148:27: error: use of undeclared identifier 'dvc_tlv'
     148 |                 tas2781_digital_putvol, dvc_tlv),
         |                                         ^
>> sound/soc/codecs/tas2781-i2c.c:595:19: error: invalid application of 'sizeof' to an incomplete type 'const struct snd_kcontrol_new[]'
     595 |         .num_controls           = ARRAY_SIZE(tas2781_snd_controls),
         |                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/array_size.h:11:32: note: expanded from macro 'ARRAY_SIZE'
      11 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
         |                                ^~~~~

2 errors generated.

vim +/dvc_tlv +148 sound/soc/codecs/tas2781-i2c.c

ef3bcde75d06d6 Shenghao Ding 2023-06-18  141  
ef3bcde75d06d6 Shenghao Ding 2023-06-18  142  static const struct snd_kcontrol_new tas2781_snd_controls[] = {
ef3bcde75d06d6 Shenghao Ding 2023-06-18  143  	SOC_SINGLE_RANGE_EXT_TLV("Speaker Analog Gain", TAS2781_AMP_LEVEL,
ef3bcde75d06d6 Shenghao Ding 2023-06-18  144  		1, 0, 20, 0, tas2781_amp_getvol,
ef3bcde75d06d6 Shenghao Ding 2023-06-18  145  		tas2781_amp_putvol, amp_vol_tlv),
ef3bcde75d06d6 Shenghao Ding 2023-06-18  146  	SOC_SINGLE_RANGE_EXT_TLV("Speaker Digital Gain", TAS2781_DVC_LVL,
ef3bcde75d06d6 Shenghao Ding 2023-06-18  147  		0, 0, 200, 1, tas2781_digital_getvol,
ef3bcde75d06d6 Shenghao Ding 2023-06-18 @148  		tas2781_digital_putvol, dvc_tlv),
ef3bcde75d06d6 Shenghao Ding 2023-06-18  149  	SOC_SINGLE_BOOL_EXT("Speaker Force Firmware Load", 0,
ef3bcde75d06d6 Shenghao Ding 2023-06-18  150  		tas2781_force_fwload_get, tas2781_force_fwload_put),
ef3bcde75d06d6 Shenghao Ding 2023-06-18  151  };

ef3bcde75d06d6 Shenghao Ding 2023-06-18 152

Signature · 2 lines

0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Gergo Koteles

author

2024-03-25 21:26 UTC #4 lore raw

PATCH 2/3 ALSA: hda/tas2781: add locks to kcontrols

In reply to Gergo Koteles

The rcabin.profile_cfg_id, cur_prog, cur_conf, force_fwload_status variables are acccessible from multiple threads and therefore require locking.

Fixes 5be27f1e3ec9 ("ALSA: hda/tas2781: Add tas2781 HDA driver")

CC [email protected]

Signed-off-by Gergo Koteles <[email protected]>

---
 sound/pci/hda/tas2781_hda_i2c.c | 50 +++++++++++++++++++++++++++++++--
 1 file changed, 48 insertions(+), 2 deletions(-)

sound/pci/hda/tas2781_hda_i2c.c +48 -2

index 5acb475c10a7..9a43f563bb9e 100644
@@ -185,8 +185,12 @@ static int tasdevice_get_profile_id(struct snd_kcontrol *kcontrol,
 {
 	struct tasdevice_priv *tas_priv = snd_kcontrol_chip(kcontrol);
 
+	mutex_lock(&tas_priv->codec_lock);
+
 	ucontrol->value.integer.value[0] = tas_priv->rcabin.profile_cfg_id;
 
+	mutex_unlock(&tas_priv->codec_lock);
+
 	return 0;
 }

@@ -200,11 +204,15 @@ static int tasdevice_set_profile_id(struct snd_kcontrol *kcontrol,
 
 	val = clamp(nr_profile, 0, max);
 
+	mutex_lock(&tas_priv->codec_lock);
+
 	if (tas_priv->rcabin.profile_cfg_id != val) {
 		tas_priv->rcabin.profile_cfg_id = val;
 		ret = 1;
 	}
 
+	mutex_unlock(&tas_priv->codec_lock);
+
 	return ret;
 }

@@ -241,8 +249,12 @@ static int tasdevice_program_get(struct snd_kcontrol *kcontrol,
 {
 	struct tasdevice_priv *tas_priv = snd_kcontrol_chip(kcontrol);
 
+	mutex_lock(&tas_priv->codec_lock);
+
 	ucontrol->value.integer.value[0] = tas_priv->cur_prog;
 
+	mutex_unlock(&tas_priv->codec_lock);
+
 	return 0;
 }

@@ -257,11 +269,15 @@ static int tasdevice_program_put(struct snd_kcontrol *kcontrol,
 
 	val = clamp(nr_program, 0, max);
 
+	mutex_lock(&tas_priv->codec_lock);
+
 	if (tas_priv->cur_prog != val) {
 		tas_priv->cur_prog = val;
 		ret = 1;
 	}
 
+	mutex_unlock(&tas_priv->codec_lock);
+
 	return ret;
 }

@@ -270,8 +286,12 @@ static int tasdevice_config_get(struct snd_kcontrol *kcontrol,
 {
 	struct tasdevice_priv *tas_priv = snd_kcontrol_chip(kcontrol);
 
+	mutex_lock(&tas_priv->codec_lock);
+
 	ucontrol->value.integer.value[0] = tas_priv->cur_conf;
 
+	mutex_unlock(&tas_priv->codec_lock);
+
 	return 0;
 }

@@ -286,11 +306,15 @@ static int tasdevice_config_put(struct snd_kcontrol *kcontrol,
 
 	val = clamp(nr_config, 0, max);
 
+	mutex_lock(&tas_priv->codec_lock);
+
 	if (tas_priv->cur_conf != val) {
 		tas_priv->cur_conf = val;
 		ret = 1;
 	}
 
+	mutex_unlock(&tas_priv->codec_lock);
+
 	return ret;
 }

@@ -300,8 +324,15 @@ static int tas2781_amp_getvol(struct snd_kcontrol *kcontrol,
 	struct tasdevice_priv *tas_priv = snd_kcontrol_chip(kcontrol);
 	struct soc_mixer_control *mc =
 		(struct soc_mixer_control *)kcontrol->private_value;
+	int ret;
+
+	mutex_lock(&tas_priv->codec_lock);
+
+	ret = tasdevice_amp_getvol(tas_priv, ucontrol, mc);
+
+	mutex_unlock(&tas_priv->codec_lock);
 
-	return tasdevice_amp_getvol(tas_priv, ucontrol, mc);
+	return ret;
 }
 
 static int tas2781_amp_putvol(struct snd_kcontrol *kcontrol,

@@ -310,9 +341,16 @@ static int tas2781_amp_putvol(struct snd_kcontrol *kcontrol,
 	struct tasdevice_priv *tas_priv = snd_kcontrol_chip(kcontrol);
 	struct soc_mixer_control *mc =
 		(struct soc_mixer_control *)kcontrol->private_value;
+	int ret;
+
+	mutex_lock(&tas_priv->codec_lock);
 
 	/* The check of the given value is in tasdevice_amp_putvol. */
-	return tasdevice_amp_putvol(tas_priv, ucontrol, mc);
+	ret = tasdevice_amp_putvol(tas_priv, ucontrol, mc);
+
+	mutex_unlock(&tas_priv->codec_lock);
+
+	return ret;
 }
 
 static int tas2781_force_fwload_get(struct snd_kcontrol *kcontrol,

@@ -320,10 +358,14 @@ static int tas2781_force_fwload_get(struct snd_kcontrol *kcontrol,
 {
 	struct tasdevice_priv *tas_priv = snd_kcontrol_chip(kcontrol);
 
+	mutex_lock(&tas_priv->codec_lock);
+
 	ucontrol->value.integer.value[0] = (int)tas_priv->force_fwload_status;
 	dev_dbg(tas_priv->dev, "%s : Force FWload %s\n", __func__,
 			tas_priv->force_fwload_status ? "ON" : "OFF");
 
+	mutex_unlock(&tas_priv->codec_lock);
+
 	return 0;
 }

@@ -333,6 +375,8 @@ static int tas2781_force_fwload_put(struct snd_kcontrol *kcontrol,
 	struct tasdevice_priv *tas_priv = snd_kcontrol_chip(kcontrol);
 	bool change, val = (bool)ucontrol->value.integer.value[0];
 
+	mutex_lock(&tas_priv->codec_lock);
+
 	if (tas_priv->force_fwload_status == val)
 		change = false;
 	else {

@@ -342,6 +386,8 @@ static int tas2781_force_fwload_put(struct snd_kcontrol *kcontrol,
 	dev_dbg(tas_priv->dev, "%s : Force FWload %s\n", __func__,
 		tas_priv->force_fwload_status ? "ON" : "OFF");
 
+	mutex_unlock(&tas_priv->codec_lock);
+
 	return change;
 }

Signature · 1 line

2.44.0

Gergo Koteles

author

2024-03-25 21:26 UTC #5 lore raw

PATCH 3/3 ALSA: hda/tas2781: add debug statements to kcontrols

In reply to Gergo Koteles

Sometimes it is useful to examine the timing of kcontrol events.

Add debug statements to each kcontrol.

Signed-off-by Gergo Koteles <[email protected]>

---
 sound/pci/hda/tas2781_hda_i2c.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

sound/pci/hda/tas2781_hda_i2c.c +22 -0

index 9a43f563bb9e..b60ce4c2c090 100644
@@ -189,6 +189,9 @@ static int tasdevice_get_profile_id(struct snd_kcontrol *kcontrol,
 
 	ucontrol->value.integer.value[0] = tas_priv->rcabin.profile_cfg_id;
 
+	dev_dbg(tas_priv->dev, "%s: %d\n", __func__,
+		tas_priv->rcabin.profile_cfg_id);
+
 	mutex_unlock(&tas_priv->codec_lock);
 
 	return 0;

@@ -206,6 +209,9 @@ static int tasdevice_set_profile_id(struct snd_kcontrol *kcontrol,
 
 	mutex_lock(&tas_priv->codec_lock);
 
+	dev_dbg(tas_priv->dev, "%s: %d -> %d\n", __func__,
+		tas_priv->rcabin.profile_cfg_id, val);
+
 	if (tas_priv->rcabin.profile_cfg_id != val) {
 		tas_priv->rcabin.profile_cfg_id = val;
 		ret = 1;

@@ -253,6 +259,8 @@ static int tasdevice_program_get(struct snd_kcontrol *kcontrol,
 
 	ucontrol->value.integer.value[0] = tas_priv->cur_prog;
 
+	dev_dbg(tas_priv->dev, "%s: %d\n", __func__, tas_priv->cur_prog);
+
 	mutex_unlock(&tas_priv->codec_lock);
 
 	return 0;

@@ -271,6 +279,9 @@ static int tasdevice_program_put(struct snd_kcontrol *kcontrol,
 
 	mutex_lock(&tas_priv->codec_lock);
 
+	dev_dbg(tas_priv->dev, "%s: %d -> %d\n", __func__,
+		tas_priv->cur_prog, val);
+
 	if (tas_priv->cur_prog != val) {
 		tas_priv->cur_prog = val;
 		ret = 1;

@@ -290,6 +301,8 @@ static int tasdevice_config_get(struct snd_kcontrol *kcontrol,
 
 	ucontrol->value.integer.value[0] = tas_priv->cur_conf;
 
+	dev_dbg(tas_priv->dev, "%s: %d\n", __func__, tas_priv->cur_conf);
+
 	mutex_unlock(&tas_priv->codec_lock);
 
 	return 0;

@@ -308,6 +321,9 @@ static int tasdevice_config_put(struct snd_kcontrol *kcontrol,
 
 	mutex_lock(&tas_priv->codec_lock);
 
+	dev_dbg(tas_priv->dev, "%s: %d -> %d\n", __func__,
+		tas_priv->cur_conf, val);
+
 	if (tas_priv->cur_conf != val) {
 		tas_priv->cur_conf = val;
 		ret = 1;

@@ -330,6 +346,9 @@ static int tas2781_amp_getvol(struct snd_kcontrol *kcontrol,
 
 	ret = tasdevice_amp_getvol(tas_priv, ucontrol, mc);
 
+	dev_dbg(tas_priv->dev, "%s: %ld\n", __func__,
+		ucontrol->value.integer.value[0]);
+
 	mutex_unlock(&tas_priv->codec_lock);
 
 	return ret;

@@ -345,6 +364,9 @@ static int tas2781_amp_putvol(struct snd_kcontrol *kcontrol,
 
 	mutex_lock(&tas_priv->codec_lock);
 
+	dev_dbg(tas_priv->dev, "%s: %ld\n", __func__,
+		ucontrol->value.integer.value[0]);
+
 	/* The check of the given value is in tasdevice_amp_putvol. */
 	ret = tasdevice_amp_putvol(tas_priv, ucontrol, mc);

Signature · 1 line

2.44.0

Pierre-Louis Bossart

2024-03-25 22:01 UTC #6 lore raw

Re: [PATCH 3/3] ALSA: hda/tas2781: add debug statements to kcontrols

sound/pci/hda/tas2781_hda_i2c.c static int tasdevice_get_profile_id(struct snd_kcontrol *kcontrol, · line 192

Quoted 9 lines

+++ b/sound/pci/hda/tas2781_hda_i2c.c

sound/pci/hda/tas2781_hda_i2c.c +1 -0

@@ -189,6 +189,9 @@ static int tasdevice_get_profile_id(struct snd_kcontrol *kcontrol,
 
 	ucontrol->value.integer.value[0] = tas_priv->rcabin.profile_cfg_id;
 
+	dev_dbg(tas_priv->dev, "%s: %d\n", __func__,

Nit-pick: you don't need to add __func__ to dev_dbg logs, the user can add the information with the dyndbg parameter, e.g.

options snd_intel_dspcfg dyndbg=+pmf

dev_err/warn don't have this functionality though so in those cases there's no replacement for __func__

Andy Shevchenko

2024-03-25 22:07 UTC #7 lore raw

Re: [PATCH 3/3] ALSA: hda/tas2781: add debug statements to kcontrols

Mon, Mar 25, 2024 at 05:01:18PM -0500, Pierre-Louis Bossart kirjoitti:

...

Quoted 9 lines

sound/pci/hda/tas2781_hda_i2c.c static int tasdevice_get_profile_id(struct snd_kcontrol *kcontrol, · line 192
Quoted 1 line
sound/pci/hda/tas2781_hda_i2c.c +1 -0
+	dev_dbg(tas_priv->dev, "%s: %d\n", __func__,
Nit-pick: you don't need to add __func__ to dev_dbg logs, the user can add the information with the dyndbg parameter, e.g.
options snd_intel_dspcfg dyndbg=+pmf
dev_err/warn don't have this functionality though so in those cases there's no replacement for __func__

You beat me up to it, I just downloaded the email thread to say the same.

Since I'm here, I think __func__ in dev_err()/dev_warn() usually says about poorly written message itself (that it's not unique enough to distinguish taking into account that this has device instance name as well). While pr_*() ones indeed may benefit from having it.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Gergo Koteles

author

2024-03-25 23:13 UTC #8 lore raw

Re: [PATCH 3/3] ALSA: hda/tas2781: add debug statements to kcontrols

In reply to Pierre-Louis Bossart

Hi Pierre-Louis,

Quoted 12 lines · Pierre-Louis Bossart

sound/pci/hda/tas2781_hda_i2c.c static int tasdevice_get_profile_id(struct snd_kcontrol *kcontrol, · line 192
Quoted 8 lines
+++ b/sound/pci/hda/tas2781_hda_i2c.c
sound/pci/hda/tas2781_hda_i2c.c +1 -0
@@ -189,6 +189,9 @@ static int tasdevice_get_profile_id(struct snd_kcontrol *kcontrol,
 
 	ucontrol->value.integer.value[0] = tas_priv->rcabin.profile_cfg_id;
 
+	dev_dbg(tas_priv->dev, "%s: %d\n", __func__,
Nit-pick: you don't need to add __func__ to dev_dbg logs, the user can add the information with the dyndbg parameter, e.g.

Quoted 5 lines

options snd_intel_dspcfg dyndbg=+pmf
dev_err/warn don't have this functionality though so in those cases there's no replacement for __func__

Thanks. I just put a #define DEBUG into the first line and rebuilt the module. It will be faster this way :)

I will send a v2.

Regards,
Gergo

Takashi Iwai

2024-03-26 07:16 UTC #9 lore raw

Re: [PATCH 3/3] ALSA: hda/tas2781: add debug statements to kcontrols

In reply to Pierre-Louis Bossart

Quoted 7 lines · Pierre-Louis Bossart

Quoted 7 lines
+++ b/sound/pci/hda/tas2781_hda_i2c.c
(quoted diff) +0 -0
@@ -189,6 +189,9 @@ static int tasdevice_get_profile_id(struct snd_kcontrol *kcontrol,
 

Quoted 8 lines

sound/pci/hda/tas2781_hda_i2c.c static int tasdevice_get_profile_id(struct snd_kcontrol *kcontrol, · line 192
Quoted 3 lines
sound/pci/hda/tas2781_hda_i2c.c +1 -0
 	ucontrol->value.integer.value[0] = tas_priv->rcabin.profile_cfg_id;
 
+	dev_dbg(tas_priv->dev, "%s: %d\n", __func__,
Nit-pick: you don't need to add __func__ to dev_dbg logs, the user can add the information with the dyndbg parameter, e.g.
options snd_intel_dspcfg dyndbg=+pmf

Since this doesn't always show up, I don't mind to have the function name shown explicitly there. OTOH, what bothers me is that all those messages have a short format "__func__: %d" while the values are utterly different, depending on the function. That can be confusing. IMO, it'd be more user-friendly to indicate what values are presented, too.

thanks,

Takashi