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

[Gergo Koteles <soyer@xxxxxx>]

On Wed, 2024-10-09 at 16:55 +0200, Takashi Iwai wrote:
> 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()? 
> 

I think, yes. I tested it with TAS2563 (Lenovo Yoga 7 14ARB7 Laptop).

> 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.
> 

The runtime_resume() checks whether "Speaker Program Id" kcontrol (as
cur_prog variable) has changed with tasdevice_prmg_load() function. If
so, updates it, so I don't think it can be dropped easily without
moving this functionality somewhere else.

I'm not sure if the calibration data needs to be updated before each
power-up or only after a program change.


Thanks,
Gergo Koteles