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

[Takashi Iwai <tiwai@xxxxxxx>]

aOn Wed, 09 Oct 2024 16:32:52 +0200,
Gergo Koteles wrote:
> 
> 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.

Ah, right, there is a tweak of runtime PM there, which makes harder to
track the state change...
Can we just drop the amp in tasdev_fw_ready()?  Then the whole runtime
suspend can be dropped.  And, the conditional amp off/on can be moved
again back to the system suspend/resume.  That makes less confusing.

In anyway, it seems that my patches aren't really correct.  Let's
discard them at first.


thanks,

Takashi