2024-03-08 17:42:46

by Gergo Koteles

[permalink] [raw]
Subject: [PATCH 0/5] ALSA: hda/tas2781: restore power state after system_resume

This series makes some simplifications, speeding up the start of the
playback and restoring the power state of the amplifiers after
system_resume.

Gergo Koteles (5):
ALSA: hda/tas2781: use dev_dbg in system_resume
ALSA: hda/tas2781: add lock to system_suspend
ALSA: hda/tas2781: do not reset cur_* values in runtime_suspend
ALSA: hda/tas2781: do not call pm_runtime_force_* in
system_resume/suspend
ALSA: hda/tas2781: restore power state after system_resume

sound/pci/hda/tas2781_hda_i2c.c | 35 ++++++++++++++++-----------------
1 file changed, 17 insertions(+), 18 deletions(-)


base-commit: 90d35da658da8cff0d4ecbb5113f5fac9d00eb72
--
2.44.0



2024-03-08 17:42:52

by Gergo Koteles

[permalink] [raw]
Subject: [PATCH 2/5] ALSA: hda/tas2781: add lock to system_suspend

Add the missing lock around tasdevice_tuning_switch().

Fixes: 5be27f1e3ec9 ("ALSA: hda/tas2781: Add tas2781 HDA driver")
Signed-off-by: Gergo Koteles <[email protected]>
---
sound/pci/hda/tas2781_hda_i2c.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c
index ee3e0afc9b31..750e49fbb91e 100644
--- a/sound/pci/hda/tas2781_hda_i2c.c
+++ b/sound/pci/hda/tas2781_hda_i2c.c
@@ -885,9 +885,13 @@ static int tas2781_system_suspend(struct device *dev)
if (ret)
return ret;

+ mutex_lock(&tas_hda->priv->codec_lock);
+
/* Shutdown chip before system suspend */
tasdevice_tuning_switch(tas_hda->priv, 1);

+ mutex_unlock(&tas_hda->priv->codec_lock);
+
/*
* Reset GPIO may be shared, so cannot reset here.
* However beyond this point, amps may be powered down.
--
2.44.0


2024-03-08 17:43:00

by Gergo Koteles

[permalink] [raw]
Subject: [PATCH 3/5] ALSA: hda/tas2781: do not reset cur_* values in runtime_suspend

The amplifier doesn't loose register state in software shutdown mode, so
there is no need to reset the cur_* values.

Without these resets, the amplifier can be turned on after
runtime_suspend without waiting for the program and
profile to be restored.

Fixes: 5be27f1e3ec9 ("ALSA: hda/tas2781: Add tas2781 HDA driver")
Signed-off-by: Gergo Koteles <[email protected]>
---
sound/pci/hda/tas2781_hda_i2c.c | 7 -------
1 file changed, 7 deletions(-)

diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c
index 750e49fbb91e..0e61e872bb71 100644
--- a/sound/pci/hda/tas2781_hda_i2c.c
+++ b/sound/pci/hda/tas2781_hda_i2c.c
@@ -832,7 +832,6 @@ static void tas2781_hda_i2c_remove(struct i2c_client *clt)
static int tas2781_runtime_suspend(struct device *dev)
{
struct tas2781_hda *tas_hda = dev_get_drvdata(dev);
- int i;

dev_dbg(tas_hda->dev, "Runtime Suspend\n");

@@ -843,12 +842,6 @@ static int tas2781_runtime_suspend(struct device *dev)
tas_hda->priv->playback_started = false;
}

- for (i = 0; i < tas_hda->priv->ndev; i++) {
- tas_hda->priv->tasdevice[i].cur_book = -1;
- tas_hda->priv->tasdevice[i].cur_prog = -1;
- tas_hda->priv->tasdevice[i].cur_conf = -1;
- }
-
mutex_unlock(&tas_hda->priv->codec_lock);

return 0;
--
2.44.0


2024-03-08 17:43:10

by Gergo Koteles

[permalink] [raw]
Subject: [PATCH 4/5] ALSA: hda/tas2781: do not call pm_runtime_force_* in system_resume/suspend

The runtime_resume function calls prmg_load and apply_calibration
functions, but system_resume also calls them, so calling
pm_runtime_force_resume before reset is unnecessary.

For consistency, do not call the pm_runtime_force_suspend in
system_suspend, as runtime_suspend does the same.

Fixes: 5be27f1e3ec9 ("ALSA: hda/tas2781: Add tas2781 HDA driver")
Signed-off-by: Gergo Koteles <[email protected]>
---
sound/pci/hda/tas2781_hda_i2c.c | 11 +----------
1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c
index 0e61e872bb71..a99f490c6a23 100644
--- a/sound/pci/hda/tas2781_hda_i2c.c
+++ b/sound/pci/hda/tas2781_hda_i2c.c
@@ -870,14 +870,9 @@ static int tas2781_runtime_resume(struct device *dev)
static int tas2781_system_suspend(struct device *dev)
{
struct tas2781_hda *tas_hda = dev_get_drvdata(dev);
- int ret;

dev_dbg(tas_hda->priv->dev, "System Suspend\n");

- ret = pm_runtime_force_suspend(dev);
- if (ret)
- return ret;
-
mutex_lock(&tas_hda->priv->codec_lock);

/* Shutdown chip before system suspend */
@@ -895,14 +890,10 @@ static int tas2781_system_suspend(struct device *dev)
static int tas2781_system_resume(struct device *dev)
{
struct tas2781_hda *tas_hda = dev_get_drvdata(dev);
- int i, ret;
+ int i;

dev_dbg(tas_hda->priv->dev, "System Resume\n");

- ret = pm_runtime_force_resume(dev);
- if (ret)
- return ret;
-
mutex_lock(&tas_hda->priv->codec_lock);

for (i = 0; i < tas_hda->priv->ndev; i++) {
--
2.44.0


2024-03-08 17:43:16

by Gergo Koteles

[permalink] [raw]
Subject: [PATCH 5/5] ALSA: hda/tas2781: restore power state after system_resume

After system_resume the amplifers will remain off, even if they were on
before system_suspend.

Use playback_started bool to save the playback state, and restore power
state based on it.

Fixes: 5be27f1e3ec9 ("ALSA: hda/tas2781: Add tas2781 HDA driver")
Signed-off-by: Gergo Koteles <[email protected]>
---
sound/pci/hda/tas2781_hda_i2c.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c
index a99f490c6a23..7aef93126ed0 100644
--- a/sound/pci/hda/tas2781_hda_i2c.c
+++ b/sound/pci/hda/tas2781_hda_i2c.c
@@ -160,11 +160,13 @@ static void tas2781_hda_playback_hook(struct device *dev, int action)
pm_runtime_get_sync(dev);
mutex_lock(&tas_hda->priv->codec_lock);
tasdevice_tuning_switch(tas_hda->priv, 0);
+ tas_hda->priv->playback_started = true;
mutex_unlock(&tas_hda->priv->codec_lock);
break;
case HDA_GEN_PCM_ACT_CLOSE:
mutex_lock(&tas_hda->priv->codec_lock);
tasdevice_tuning_switch(tas_hda->priv, 1);
+ tas_hda->priv->playback_started = false;
mutex_unlock(&tas_hda->priv->codec_lock);

pm_runtime_mark_last_busy(dev);
@@ -666,6 +668,7 @@ static void tasdev_fw_ready(const struct firmware *fmw, void *context)
tasdevice_save_calibration(tas_priv);

tasdevice_tuning_switch(tas_hda->priv, 0);
+ tas_hda->priv->playback_started = true;

out:
mutex_unlock(&tas_hda->priv->codec_lock);
@@ -837,6 +840,9 @@ static int tas2781_runtime_suspend(struct device *dev)

mutex_lock(&tas_hda->priv->codec_lock);

+ /* The driver powers up the amplifiers at module load time.
+ * Stop the playback if it's unused.
+ */
if (tas_hda->priv->playback_started) {
tasdevice_tuning_switch(tas_hda->priv, 1);
tas_hda->priv->playback_started = false;
@@ -876,7 +882,8 @@ static int tas2781_system_suspend(struct device *dev)
mutex_lock(&tas_hda->priv->codec_lock);

/* Shutdown chip before system suspend */
- tasdevice_tuning_switch(tas_hda->priv, 1);
+ if (tas_hda->priv->playback_started)
+ tasdevice_tuning_switch(tas_hda->priv, 1);

mutex_unlock(&tas_hda->priv->codec_lock);

@@ -908,6 +915,10 @@ static int tas2781_system_resume(struct device *dev)
* calibrated data inside algo.
*/
tasdevice_apply_calibration(tas_hda->priv);
+
+ if (tas_hda->priv->playback_started)
+ tasdevice_tuning_switch(tas_hda->priv, 0);
+
mutex_unlock(&tas_hda->priv->codec_lock);

return 0;
--
2.44.0


2024-03-11 08:15:14

by Takashi Iwai

[permalink] [raw]
Subject: Re: [PATCH 0/5] ALSA: hda/tas2781: restore power state after system_resume

On Fri, 08 Mar 2024 18:41:39 +0100,
Gergo Koteles wrote:
>
> This series makes some simplifications, speeding up the start of the
> playback and restoring the power state of the amplifiers after
> system_resume.
>
> Gergo Koteles (5):
> ALSA: hda/tas2781: use dev_dbg in system_resume
> ALSA: hda/tas2781: add lock to system_suspend
> ALSA: hda/tas2781: do not reset cur_* values in runtime_suspend
> ALSA: hda/tas2781: do not call pm_runtime_force_* in
> system_resume/suspend
> ALSA: hda/tas2781: restore power state after system_resume

Applied all patches now. Thanks.


Takashi