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.