Re: [PATCH 1/2] ALSA: hda: tas2781: Fix missing setup at runtime PM

[Gergo Koteles <soyer@xxxxxx>]

Hi Takashi,

On Tue, 2024-10-08 at 14:26 +0200, Takashi Iwai wrote:
> tas2781_runtime_suspend() clears the playback_started flag, hence it
> leads to inconsistent state after runtime suspend is triggered, as if
> the device hasn't been opened yet.  Also, the counterpart,
> alc2781_runtime_resume() doesn't call tasdevice_tuning_switch(), and
> this also causes the inconsistency.
> 
> This patch corrects the superfluous flag clearance and the missing
> call.
> 
> Fixes: 5be27f1e3ec9 ("ALSA: hda/tas2781: Add tas2781 HDA driver")
> Link: https://bugzilla.suse.com/show_bug.cgi?id=1230132
> Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
> ---
>  sound/pci/hda/tas2781_hda_i2c.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c
> index 370d847517f9..6d173b721fd0 100644
> --- a/sound/pci/hda/tas2781_hda_i2c.c
> +++ b/sound/pci/hda/tas2781_hda_i2c.c
> @@ -864,10 +864,8 @@ static int tas2781_runtime_suspend(struct device *dev)
>  	/* The driver powers up the amplifiers at module load time.
>  	 * Stop the playback if it's unused.
>  	 */
> -	if (tas_hda->priv->playback_started) {
> +	if (tas_hda->priv->playback_started)
>  		tasdevice_tuning_switch(tas_hda->priv, 1);
> -		tas_hda->priv->playback_started = false;
> -	}
>  

This if-branch were added because I wanted to power up the amplifier at
module load time in tasdev_fw_ready() with:

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

but I didn't want it to stay powered forever, if there aren't any 
tas2781_hda_playback_hook() call with HDA_GEN_PCM_ACT_CLOSE action.

This seemed like a good idea at the time because the music didn't have
to be restarted (for HDA_GEN_PCM_ACT_OPEN action) when the module was
reloaded. But this is a rare use case, today I don't think it's
important to have it in there.

In general cases, tas2781_hda_playback_hook() turns the amplifier on
and off, and sets the playback_started flag.
If I understand correctly, the playback_started will be off at the time
of runtime_suspend()/runtime_resume() calls.

So I think the mentioned two lines from the tasdev_fw_ready() function
and the mentioned if-branch can be deleted.


Thanks,
Gergo Koteles