[PATCH v2 0/4] ALSA: hda/tas2781: fixes for 6.9-rc1

newer
[PATCH 00/25] Introduce support of...

Gergo Koteles

26 Mar 2024 26 Mar '24

5:18 p.m.

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

Changes in v2: - Do not remove dvc_tlv scale from tas2781-tlv.h as it is also used by sound/soc/codecs/tas2781-i2c.c - Add kcontrol name to debug statements - Add a new patch to remove a useless debug statement.

[1]: https://lore.kernel.org/all/cover.1711401621.git.soyer@irl.hu/

Gergo Koteles (4): ALSA: hda/tas2781: remove digital gain kcontrol ALSA: hda/tas2781: add locks to kcontrols ALSA: hda/tas2781: add debug statements to kcontrols ALSA: hda/tas2781: remove useless dev_dbg from playback_hook

sound/pci/hda/tas2781_hda_i2c.c | 118 +++++++++++++++++++++----------- 1 file changed, 77 insertions(+), 41 deletions(-)

base-commit: 4cece764965020c22cff7665b18a012006359095

-- 2.44.0

Show replies by date

Gergo Koteles

26 Mar 26 Mar

5:18 p.m.

New subject: [PATCH v2 1/4] 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: stable@vger.kernel.org Signed-off-by: Gergo Koteles soyer@irl.hu --- sound/pci/hda/tas2781_hda_i2c.c | 37 +-------------------------------- 1 file changed, 1 insertion(+), 36 deletions(-)

diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c index 4475cea8e9f7..5acb475c10a7 100644 --- a/sound/pci/hda/tas2781_hda_i2c.c +++ b/sound/pci/hda/tas2781_hda_i2c.c @@ -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), };

-- 2.44.0

Gergo Koteles

5:18 p.m.

New subject: [PATCH v2 2/4] ALSA: hda/tas2781: add locks to kcontrols

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: stable@vger.kernel.org Signed-off-by: Gergo Koteles soyer@irl.hu --- sound/pci/hda/tas2781_hda_i2c.c | 50 +++++++++++++++++++++++++++++++-- 1 file changed, 48 insertions(+), 2 deletions(-)

diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c index 5acb475c10a7..9a43f563bb9e 100644 --- a/sound/pci/hda/tas2781_hda_i2c.c +++ b/sound/pci/hda/tas2781_hda_i2c.c @@ -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; }

-- 2.44.0

Gergo Koteles

5:18 p.m.

New subject: [PATCH v2 3/4] ALSA: hda/tas2781: add debug statements to kcontrols

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

Add debug statements to each kcontrol.

Signed-off-by: Gergo Koteles soyer@irl.hu --- sound/pci/hda/tas2781_hda_i2c.c | 35 +++++++++++++++++++++++++++++---- 1 file changed, 31 insertions(+), 4 deletions(-)

diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c index 9a43f563bb9e..f495caee38e1 100644 --- a/sound/pci/hda/tas2781_hda_i2c.c +++ b/sound/pci/hda/tas2781_hda_i2c.c @@ -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: kcontrol %s: %d\n", + __func__, kcontrol->id.name, tas_priv->rcabin.profile_cfg_id); + mutex_unlock(&tas_priv->codec_lock);

return 0; @@ -206,6 +209,10 @@ static int tasdevice_set_profile_id(struct snd_kcontrol *kcontrol,