Re: [PATCH 1/3] ASoC: tas2783-sdw: keep the volume settings across resets

[Pierre-Louis Bossart]

On 10/19/26 19:53, Matthew Schwartz wrote:
> On 10/19/26 8:56 AM, Pierre-Louis Bossart wrote:
>> On 10/18/26 22:00, Matthew Schwartz wrote:
>>> The register cache is dropped when the amp attaches uninitialized and
>>> again after the firmware download, so the Amp Volume and Speaker Volume
>>> registers are not cached while the amp is runtime suspended, and every
>>> read or write of those controls fails with -EBUSY. On a platform that
>>> powers the SoundWire link off when idle, such as the ASUS ROG Xbox Ally
>>> X20, the amp is also reset and initialized again on every wake, so a
>>> volume set while it was awake is lost the next time the link idles.
>>>
>>> Keep both settings in the driver, as the cache has to be dropped
>>> whenever the reset and the firmware download change registers behind
>>> it. Reads return the kept value, a write reaches the device only while
>>> it is runtime active and initialized, and the settings are programmed
>>> again after every init and resume. Until userspace sets a control, the
>>> value the firmware leaves after each init is adopted instead, so its
>>> tuning stays in effect. Waking the amp for a control access would
>>> instead cost a full init of both X20 amps, about 0.2 s each.
>>>
>>> Fixes: b627da430357 ("ASoC: tas2783-sdw: drop stale regcache on uninitialized re-attach")
>>
>> Erm, not sure what to think about this fix of a fix.
>>
>> This commit b627da430357 introduced the regcache_drop_region() for all
>> kinds of reasons, and now this new patch introduces a manual management
>> of the volume registers - but why limit this management to volume
>> settings only? Shouldn't other features such as e.g. bass boost or EQ
>> also be restored?
>>
>> It really feels like the problem is that the regmap is not properly
>> restored...at the end of the day, this driver is missing a
>> regcache_sync(), no?
> 
> The driver only has the two volume controls, there's no bass boost or
> EQ control. The rest of the tuning comes from the firmware, which
> tas_io_init() writes again on every init, apart from a retained PRAM.
> 
> A regcache_sync() can't restore these two because the firmware
> download writes registers with sdw_nwrite_no_pm(), behind the cache,
> and that includes AMP_LEVEL. Of the 72 TAS2783 files in linux-firmware,
> 44 set it to something other than the 0x28 in tas2783_reg_default[],
> mostly levels 9 to 16, and two set DVC to 0x04 instead of 0x00.
> Syncing the default values would replace that per-platform tuning with
> full gain. So the driver has to know whether userspace set a value,
> which needs restoring, or left it alone, in which case the firmware's
> value should stay, and regmap doesn't track that per register.
> 
> The volume was also lost before b627da430357, since the sync ran
> before the reset wiped it. The Fixes tag is for the -EBUSY that commit
> added when reading an uncached volume register while the amp is
> runtime suspended.
> 
> Certainly open to suggestions for improvement here if there's a simpler
> way to fix this.
I am afraid I don't fully grasp the intended design, with a regmap
default, a firmware download that modifies volume settings but doesn't
update the regmap value, and last a userspace-defined volume that can
override a platform-specific tuned value. It's a recipe for inconsistent
register settings...