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/[email protected]/
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
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]>
---
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
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(-)
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
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 | 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,
mutex_lock(&tas_priv->codec_lock);
+ dev_dbg(tas_priv->dev, "%s: kcontrol %s: %d -> %d\n",
+ __func__, kcontrol->id.name,
+ 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 +260,9 @@ static int tasdevice_program_get(struct snd_kcontrol *kcontrol,
ucontrol->value.integer.value[0] = tas_priv->cur_prog;
+ dev_dbg(tas_priv->dev, "%s: kcontrol %s: %d\n",
+ __func__, kcontrol->id.name, tas_priv->cur_prog);
+
mutex_unlock(&tas_priv->codec_lock);
return 0;
@@ -271,6 +281,9 @@ static int tasdevice_program_put(struct snd_kcontrol *kcontrol,
mutex_lock(&tas_priv->codec_lock);
+ dev_dbg(tas_priv->dev, "%s: kcontrol %s: %d -> %d\n",
+ __func__, kcontrol->id.name, tas_priv->cur_prog, val);
+
if (tas_priv->cur_prog != val) {
tas_priv->cur_prog = val;
ret = 1;
@@ -290,6 +303,9 @@ static int tasdevice_config_get(struct snd_kcontrol *kcontrol,
ucontrol->value.integer.value[0] = tas_priv->cur_conf;
+ dev_dbg(tas_priv->dev, "%s: kcontrol %s: %d\n",
+ __func__, kcontrol->id.name, tas_priv->cur_conf);
+
mutex_unlock(&tas_priv->codec_lock);
return 0;
@@ -308,6 +324,9 @@ static int tasdevice_config_put(struct snd_kcontrol *kcontrol,
mutex_lock(&tas_priv->codec_lock);
+ dev_dbg(tas_priv->dev, "%s: kcontrol %s: %d -> %d\n",
+ __func__, kcontrol->id.name, tas_priv->cur_conf, val);
+
if (tas_priv->cur_conf != val) {
tas_priv->cur_conf = val;
ret = 1;
@@ -330,6 +349,9 @@ static int tas2781_amp_getvol(struct snd_kcontrol *kcontrol,
ret = tasdevice_amp_getvol(tas_priv, ucontrol, mc);
+ dev_dbg(tas_priv->dev, "%s: kcontrol %s: %ld\n",
+ __func__, kcontrol->id.name, ucontrol->value.integer.value[0]);
+
mutex_unlock(&tas_priv->codec_lock);
return ret;
@@ -345,6 +367,9 @@ static int tas2781_amp_putvol(struct snd_kcontrol *kcontrol,
mutex_lock(&tas_priv->codec_lock);
+ dev_dbg(tas_priv->dev, "%s: kcontrol %s: -> %ld\n",
+ __func__, kcontrol->id.name, ucontrol->value.integer.value[0]);
+
/* The check of the given value is in tasdevice_amp_putvol. */
ret = tasdevice_amp_putvol(tas_priv, ucontrol, mc);
@@ -361,8 +386,8 @@ static int tas2781_force_fwload_get(struct snd_kcontrol *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");
+ dev_dbg(tas_priv->dev, "%s: kcontrol %s: %d\n",
+ __func__, kcontrol->id.name, tas_priv->force_fwload_status);
mutex_unlock(&tas_priv->codec_lock);
@@ -377,14 +402,16 @@ static int tas2781_force_fwload_put(struct snd_kcontrol *kcontrol,
mutex_lock(&tas_priv->codec_lock);
+ dev_dbg(tas_priv->dev, "%s: kcontrol %s: %d -> %d\n",
+ __func__, kcontrol->id.name,
+ tas_priv->force_fwload_status, val);
+
if (tas_priv->force_fwload_status == val)
change = false;
else {
change = true;
tas_priv->force_fwload_status = val;
}
- dev_dbg(tas_priv->dev, "%s : Force FWload %s\n", __func__,
- tas_priv->force_fwload_status ? "ON" : "OFF");
mutex_unlock(&tas_priv->codec_lock);
--
2.44.0
The debug message "Playback action not supported: action" is not useful,
because the action was previously printed, and the list of supported
actions are intentional.
Remove the debug statement from the default switch case.
Signed-off-by: Gergo Koteles <[email protected]>
---
sound/pci/hda/tas2781_hda_i2c.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c
index f495caee38e1..48dae3339305 100644
--- a/sound/pci/hda/tas2781_hda_i2c.c
+++ b/sound/pci/hda/tas2781_hda_i2c.c
@@ -161,8 +161,6 @@ static void tas2781_hda_playback_hook(struct device *dev, int action)
pm_runtime_put_autosuspend(dev);
break;
default:
- dev_dbg(tas_hda->dev, "Playback action not supported: %d\n",
- action);
break;
}
}
--
2.44.0
On Tue, 26 Mar 2024 17:18:44 +0100,
Gergo Koteles wrote:
>
> 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/[email protected]/
>
> 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
Applied now. Thanks.
Takashi