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

[Matthew Schwartz <matthew.schwartz@linux.dev>]

On 10/20/26 1:41 PM, Pierre-Louis Bossart wrote:
> 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...

Yeah, I certainly can't argue with you there... I think we need vendor input
to get a better understanding of how these are meant to work together.

This was broken on real hardware, so my patches were pretty focused on trying
to unbreak that specific device. Speaking candidly, I'm not sure I'm in a
position to take on all of the tech debt that's been surfacing here.