| Message ID | cbdc337b911bee0f80f805b936041fd59c1db54a.1711401621.git.soyer@irl.hu (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ALSA: hda/tas2781: fixes for 6.9-rc1 | expand |
> +++ 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: %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__
Mon, Mar 25, 2024 at 05:01:18PM -0500, Pierre-Louis Bossart kirjoitti: ... > > + 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.
Hi Pierre-Louis, On Mon, 2024-03-25 at 17:01 -0500, Pierre-Louis Bossart wrote: > > > > +++ 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: %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__ > 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
On Mon, 25 Mar 2024 23:01:18 +0100, Pierre-Louis Bossart wrote: > > > > > > +++ 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: %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
diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c index 9a43f563bb9e..b60ce4c2c090 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: %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);
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 | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)