[03/16] ASoC: tas2781: disable regmap regcache

  The amp has 3 level addressing (BOOK, PAGE, REG).
The regcache couldn't handle it.

Signed-off-by: Gergo Koteles <soyer@irl.hu>
---
 sound/pci/hda/tas2781_hda_i2c.c   | 17 +----------------
 sound/soc/codecs/tas2781-comlib.c |  2 +-
 2 files changed, 2 insertions(+), 17 deletions(-)
  

On Thu, Dec 07, 2023 at 12:59:44AM +0100, Gergo Koteles wrote:

> The amp has 3 level addressing (BOOK, PAGE, REG).
> The regcache couldn't handle it.

So the books aren't currently used so the driver actually works?

>  static int tas2781_system_suspend(struct device *dev)
> @@ -770,10 +758,7 @@ static int tas2781_system_suspend(struct device *dev)
>  		return ret;
>  
>  	/* Shutdown chip before system suspend */
> -	regcache_cache_only(tas_priv->regmap, false);
>  	tasdevice_tuning_switch(tas_priv, 1);
> -	regcache_cache_only(tas_priv->regmap, true);
> -	regcache_mark_dirty(tas_priv->regmap);
>  
>  	/*
>  	 * Reset GPIO may be shared, so cannot reset here.

How can this work over system suspend?  This just removes the cache with
no replacement so if the device looses power over suspend (which seems
likely) then all the register state will be lost.  A similar issue may
potentially exist over runtime suspend on an ACPI system with
sufficiently heavily optimised power management.
  

On Thu, 2023-12-07 at 18:20 +0000, Mark Brown wrote:
> On Thu, Dec 07, 2023 at 12:59:44AM +0100, Gergo Koteles wrote:
> 
> > The amp has 3 level addressing (BOOK, PAGE, REG).
> > The regcache couldn't handle it.
> 
> So the books aren't currently used so the driver actually works?
> 
It writes to the book 0 and 8c. The initialization works with regcache,
because it writes also the i2c devices.

> >  static int tas2781_system_suspend(struct device *dev)
> > @@ -770,10 +758,7 @@ static int tas2781_system_suspend(struct device *dev)
> >  		return ret;
> >  
> >  	/* Shutdown chip before system suspend */
> > -	regcache_cache_only(tas_priv->regmap, false);
> >  	tasdevice_tuning_switch(tas_priv, 1);
> > -	regcache_cache_only(tas_priv->regmap, true);
> > -	regcache_mark_dirty(tas_priv->regmap);
> >  
> >  	/*
> >  	 * Reset GPIO may be shared, so cannot reset here.
> 
> How can this work over system suspend?  This just removes the cache with
> no replacement so if the device looses power over suspend (which seems
> likely) then all the register state will be lost.  A similar issue may
> potentially exist over runtime suspend on an ACPI system with
> sufficiently heavily optimised power management.

In runtime_resume, only one of the two amplifiers goes back.
The runtime_suspend sets the current book/prog/conf to -1 on all
devices, and tas2781_hda_playback_hook will restore the
program/configuration/profile with tasdevice_tuning_switch.

And only one, because tasdevice_change_chn_book directly changes the
address of i2c_client, so the unlucky one gets invalid values in its
actual book from regcache_sync.

system_restore doesn't work at all, because regcache_cache_only stays
true since system_suspend.

It works without the regcache functions.
  

On Thu, Dec 07, 2023 at 09:19:34PM +0100, Gergo Koteles wrote:
> On Thu, 2023-12-07 at 18:20 +0000, Mark Brown wrote:
> > On Thu, Dec 07, 2023 at 12:59:44AM +0100, Gergo Koteles wrote:

> > > The amp has 3 level addressing (BOOK, PAGE, REG).
> > > The regcache couldn't handle it.

> > So the books aren't currently used so the driver actually works?

> It writes to the book 0 and 8c. The initialization works with regcache,
> because it writes also the i2c devices.

I can't see any references to 0x8c in the driver?

> > >  static int tas2781_system_suspend(struct device *dev)
> > > @@ -770,10 +758,7 @@ static int tas2781_system_suspend(struct device *dev)
> > >  		return ret;
> > >  
> > >  	/* Shutdown chip before system suspend */
> > > -	regcache_cache_only(tas_priv->regmap, false);
> > >  	tasdevice_tuning_switch(tas_priv, 1);
> > > -	regcache_cache_only(tas_priv->regmap, true);
> > > -	regcache_mark_dirty(tas_priv->regmap);

> > How can this work over system suspend?  This just removes the cache with
> > no replacement so if the device looses power over suspend (which seems
> > likely) then all the register state will be lost.  A similar issue may
> > potentially exist over runtime suspend on an ACPI system with
> > sufficiently heavily optimised power management.

> In runtime_resume, only one of the two amplifiers goes back.
> The runtime_suspend sets the current book/prog/conf to -1 on all
> devices, and tas2781_hda_playback_hook will restore the
> program/configuration/profile with tasdevice_tuning_switch.

What does "go back" mean?  

> And only one, because tasdevice_change_chn_book directly changes the
> address of i2c_client, so the unlucky one gets invalid values in its
> actual book from regcache_sync.

The code creates the impression that writing to one tas2781 writes to
all of them, is that not the case?

> system_restore doesn't work at all, because regcache_cache_only stays
> true since system_suspend.

Presumably the next runtime resume would make the device writable again?

> It works without the regcache functions.

How would the devices get their configuration restored?

This sounds very much like a case of something working for your specific
system in your specific test through some external factor rather than
working by design, whatever problems might exist it seems fairly obvious
to inspection that this patch would make things worse for other systems.

At a minimum this patch needs a much clearer changelog (all the patches
I looked at could use clearer changelogs) which explains what's going on
here, I would really expect to see something that replaces the use of
the cache sync to restore the device state for example.
  

On Thu, 2023-12-07 at 20:36 +0000, Mark Brown wrote:
> On Thu, Dec 07, 2023 at 09:19:34PM +0100, Gergo Koteles wrote:
> > On Thu, 2023-12-07 at 18:20 +0000, Mark Brown wrote:
> > > On Thu, Dec 07, 2023 at 12:59:44AM +0100, Gergo Koteles wrote:
> 
> > > > The amp has 3 level addressing (BOOK, PAGE, REG).
> > > > The regcache couldn't handle it.
> 
> > > So the books aren't currently used so the driver actually works?
> 
> > It writes to the book 0 and 8c. The initialization works with regcache,
> > because it writes also the i2c devices.
> 
> I can't see any references to 0x8c in the driver?

The firmware has different programs. The programs have blocks, that the
driver writes to the amplifier. The address comes from the blocks.

> 
> > > >  static int tas2781_system_suspend(struct device *dev)
> > > > @@ -770,10 +758,7 @@ static int tas2781_system_suspend(struct device *dev)
> > > >  		return ret;
> > > >  
> > > >  	/* Shutdown chip before system suspend */
> > > > -	regcache_cache_only(tas_priv->regmap, false);
> > > >  	tasdevice_tuning_switch(tas_priv, 1);
> > > > -	regcache_cache_only(tas_priv->regmap, true);
> > > > -	regcache_mark_dirty(tas_priv->regmap);
> 
> > > How can this work over system suspend?  This just removes the cache with
> > > no replacement so if the device looses power over suspend (which seems
> > > likely) then all the register state will be lost.  A similar issue may
> > > potentially exist over runtime suspend on an ACPI system with
> > > sufficiently heavily optimised power management.
> 
> > In runtime_resume, only one of the two amplifiers goes back.
> > The runtime_suspend sets the current book/prog/conf to -1 on all
> > devices, and tas2781_hda_playback_hook will restore the
> > program/configuration/profile with tasdevice_tuning_switch.
> 
> What does "go back" mean?  

Sorry for imprecise wording. The speaker is silent, I didn't checked
why, maybe the amp is in error state or something.

> 
> > And only one, because tasdevice_change_chn_book directly changes the
> > address of i2c_client, so the unlucky one gets invalid values in its
> > actual book from regcache_sync.
> 
> The code creates the impression that writing to one tas2781 writes to
> all of them, is that not the case?
> 
Yes, the tasdevice_* functions, but the regcache_sync doesn't know
this.

> > system_restore doesn't work at all, because regcache_cache_only stays
> > true since system_suspend.
> 
> Presumably the next runtime resume would make the device writable again?
> 
Yes, then one of the speakers works.

> > It works without the regcache functions.
> 
> How would the devices get their configuration restored?
> 
tasdevice_tuning_switch calls tasdevice_select_tuningprm_cfg which
checks whether the devices needs a new program or configuration.

the runtime_suspend and system resume set the devices cur_prog,
cur_conf to -1.

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;
}

And the tasdevice_select_tuningprm_cfg checks with 
if (tas_priv->tasdevice[i].cur_prog != prm_no ...

If needed, it writes the new program/configuration to the device.

The tas2781_hda_playback_hook calls the tasdevice_tuning_switch


> This sounds very much like a case of something working for your specific
> system in your specific test through some external factor rather than
> working by design, whatever problems might exist it seems fairly obvious
> to inspection that this patch would make things worse for other systems.
> 
> At a minimum this patch needs a much clearer changelog (all the patches
> I looked at could use clearer changelogs) which explains what's going on
> here, I would really expect to see something that replaces the use of
> the cache sync to restore the device state for example.
  

On Thu, Dec 07, 2023 at 10:12:13PM +0100, Gergo Koteles wrote:
> On Thu, 2023-12-07 at 20:36 +0000, Mark Brown wrote:

> > > And only one, because tasdevice_change_chn_book directly changes the
> > > address of i2c_client, so the unlucky one gets invalid values in its
> > > actual book from regcache_sync.

> > The code creates the impression that writing to one tas2781 writes to
> > all of them, is that not the case?

> Yes, the tasdevice_* functions, but the regcache_sync doesn't know
> this.

So this syncing is done in software not hardware?  My understanding was
that this was a hardware thing.

> > How would the devices get their configuration restored?

> tasdevice_tuning_switch calls tasdevice_select_tuningprm_cfg which
> checks whether the devices needs a new program or configuration.

> the runtime_suspend and system resume set the devices cur_prog,
> cur_conf to -1.

...

> The tas2781_hda_playback_hook calls the tasdevice_tuning_switch

And there are no registers other than these programs?
  

On Thu, 2023-12-07 at 22:39 +0000, Mark Brown wrote:
> On Thu, Dec 07, 2023 at 10:12:13PM +0100, Gergo Koteles wrote:
> > On Thu, 2023-12-07 at 20:36 +0000, Mark Brown wrote:
> 
> > > > And only one, because tasdevice_change_chn_book directly changes the
> > > > address of i2c_client, so the unlucky one gets invalid values in its
> > > > actual book from regcache_sync.
> 
> > > The code creates the impression that writing to one tas2781 writes to
> > > all of them, is that not the case?
> 
> > Yes, the tasdevice_* functions, but the regcache_sync doesn't know
> > this.
> 
> So this syncing is done in software not hardware?  My understanding was
> that this was a hardware thing.

If you mean that the amplifier does not know that there are several
programs or configurations or profiles, but only runs the current one,
yes.

> 
> > > How would the devices get their configuration restored?
> 
> > tasdevice_tuning_switch calls tasdevice_select_tuningprm_cfg which
> > checks whether the devices needs a new program or configuration.
> 
> > the runtime_suspend and system resume set the devices cur_prog,
> > cur_conf to -1.
> 
> ...
> 
> > The tas2781_hda_playback_hook calls the tasdevice_tuning_switch
> 
> And there are no registers other than these programs?

The tas2781-hda writes 4 things:

1. Profiles from RCA file
eg. INT8866RCA2.bin has 4 profile:
Music degree 0
calibration
voice call
earpiece spk2 bypass

The profiles contain pre-power-up and pre-shutdown register+value
sequences for each amplifier.

2. Programs from DSP firmware.
eg. TAS2XXX3870.bin has 1 program:
Tuning Mode

3. Configurations from the DSP firmware.
eg. TAS2XXX3870.bin has 2 configurations:
configuration_Normal_Tuning Mode_48 KHz_s2_0
calibration_Tuning Mode_48 KHz_s2_0

Programs and configurations contain blocks with addresses where they
should be written.

4. Calibration data from EFI variables.
R0, INV_R0, R0LOW, POWER, TLIM,
Based on the chip, they should be written to 5 registers.

The code restores all of these in playback_hook, runtime_resume,
system_resume functions without regmap_cache_sync.
  

On Fri, Dec 15, 2023 at 02:17:01AM +0100, Gergo Koteles wrote:
> On Thu, 2023-12-07 at 22:39 +0000, Mark Brown wrote:
> > On Thu, Dec 07, 2023 at 10:12:13PM +0100, Gergo Koteles wrote:
> > > On Thu, 2023-12-07 at 20:36 +0000, Mark Brown wrote:

> > > > The code creates the impression that writing to one tas2781 writes to
> > > > all of them, is that not the case?

> > > Yes, the tasdevice_* functions, but the regcache_sync doesn't know
> > > this.

> > So this syncing is done in software not hardware?  My understanding was
> > that this was a hardware thing.

> If you mean that the amplifier does not know that there are several
> programs or configurations or profiles, but only runs the current one,
> yes.

No, I mean that the amplifiers don't talk to each other at a hardware
level and the grouping is all in software.
  

On Fri, 2023-12-15 at 12:55 +0000, Mark Brown wrote:
> On Fri, Dec 15, 2023 at 02:17:01AM +0100, Gergo Koteles wrote:
> > On Thu, 2023-12-07 at 22:39 +0000, Mark Brown wrote:
> > > On Thu, Dec 07, 2023 at 10:12:13PM +0100, Gergo Koteles wrote:
> > > > On Thu, 2023-12-07 at 20:36 +0000, Mark Brown wrote:
> 
> > > > > The code creates the impression that writing to one tas2781 writes to
> > > > > all of them, is that not the case?
> 
> > > > Yes, the tasdevice_* functions, but the regcache_sync doesn't know
> > > > this.
> 
> > > So this syncing is done in software not hardware?  My understanding was
> > > that this was a hardware thing.
> 
> > If you mean that the amplifier does not know that there are several
> > programs or configurations or profiles, but only runs the current one,
> > yes.
> 
> No, I mean that the amplifiers don't talk to each other at a hardware
> level and the grouping is all in software.

No, they don't talk to each other. But they have a global i2c address
to speed up configuration, but the module doesn't use it yet.
  

On Fri, Dec 15, 2023 at 03:42:43PM +0100, Gergo Koteles wrote:
> On Fri, 2023-12-15 at 12:55 +0000, Mark Brown wrote:

> > No, I mean that the amplifiers don't talk to each other at a hardware
> > level and the grouping is all in software.

> No, they don't talk to each other. But they have a global i2c address
> to speed up configuration, but the module doesn't use it yet.

That's hardware level synchronisation between the devices, that makes
all this a bit less horrifying though it seems like a lot of the issues
would go away if the broadcast write address were actually being used
more.