Re: [PATCH 1/3] ASoC: tas2783: let regmap-sdw-mbq poll for deferred transactions

[Ville Saarinen]

Hi Bossart,

Thanks for taking the time to review this. I am a bit of a kernel noob, 
but I completely agree with you and the spec: FUNCTION_STATUS cannot be 
deferred.

I think my commit message caused a misunderstanding. The patch doesn't 
defer the status read:

The hardware is actually returning COMMAND_IGNORED for the UDMPU23 
Cluster Index write, which is the only control tas2783_sdca_deferrable() 
flags as true.

I only added FUNCTION_STATUS to mbq_size to expose it to -> 
readable_reg(). Without that, regmap_sdw_mbq_poll_busy() considers the 
status register unreadable, skips checking the busy bit for the deferred 
UDMPU23 write, and instantly fails.

My main goal was just getting audio working on my personal laptop, which 
is now resolved on my end. I will leave it at this and leave it to you 
to proceed, adapt, or handle the changes however you see fit.

Thanks again for your time,

Ville

On 8/9/26 17:05, Pierre-Louis Bossart wrote:
> On 8/9/26 12:16, Ville Saarinen wrote:
>> In SDCA, a Function answering COMMAND_IGNORED to a Control write has
>> deferred the transaction rather than rejected it. regmap_sdw_mbq_write()
>> handles that by calling regmap_sdw_mbq_poll_busy(), which waits for the
>> Entity-0 Function Busy bit to clear and then retries once. Neither half
>> of that works for this codec:
>>
>>   - poll_busy only polls if ->readable_reg() accepts the Entity-0 Function
>>     Status address. tas2783_readable_register() answers out of
>>     tas2783_sdca_mbq_size(), which has no case for that address, so the
>>     poll is skipped and the core falls through to a bare
>>     fsleep(cfg.timeout_us).
>>
>>   - tas2783_mbq_cfg sets only .mbq_size, so timeout_us and retry_us are
>>     both 0. The fallback wait is fsleep(0) and the retry is therefore
>>     instantaneous.
>>
>> Every deferred write consequently fails by construction, returning
>> -ENODATA and logging "Defer on undeferrable control".
>>
>> Add the Function Status register to the mbq size table so the poll can
>> run, and to the volatile table so a later regcache_sync() never writes
>> back to a status register. Mark the UDMPU23 Cluster Index deferrable and
>> give the mbq cfg a poll interval and a deadline.
>>
>> Note the two cfg fields reach read_poll_timeout() as (sleep_us,
>> timeout_us), i.e. .timeout_us is the poll interval and .retry_us the
>> overall deadline -- the reverse of the kerneldoc on struct
>> regmap_sdw_mbq_cfg. The values here follow the code, which is what runs.
>>
>> Developed with AI assistance. The assistant traced the -ENODATA into
>> regmap_sdw_mbq_poll_busy() and drafted the fix.
>>
>> All hardware measurements quoted above were run by the submitter on the
>> affected machine. The submitter has reviewed the change, understands it
>> and takes responsibility for it.
>>
>> Assisted-by: Claude:claude-opus-5
>> Signed-off-by: Ville Saarinen <wiza@xxxxxxxxxxxxxxx>
>> ---
>>   sound/soc/codecs/tas2783-sdw.c | 37 ++++++++++++++++++++++++++++++++++
>>   1 file changed, 37 insertions(+)
>>
>> diff --git a/sound/soc/codecs/tas2783-sdw.c b/sound/soc/codecs/tas2783-sdw.c
>> index 3d0b11654..5a7ac6224 100644
>> --- a/sound/soc/codecs/tas2783-sdw.c
>> +++ b/sound/soc/codecs/tas2783-sdw.c
>> @@ -423,6 +423,13 @@ static int tas2783_sdca_mbq_size(struct device *dev, u32 reg)
>>   	case SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_FU23, 0x01, 0):
>>   	case SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_FU23, 0x01, 1):
>>   	case SDW_SDCA_CTL(1, TAS2783_SDCA_ENT_OT25, 0x04, 0):
>> +	/*
>> +	 * Entity 0 Function Status. regmap_sdw_mbq_poll_busy() only polls the
>> +	 * Function Busy bit if ->readable_reg() accepts this address; without
>> +	 * it the core drops into a bare fsleep(cfg.timeout_us) and, with that
>> +	 * left at 0, retries a deferred transaction instantly and fails.
>> +	 */
>> +	case SDW_SDCA_CTL(1, 0, SDCA_CTL_ENTITY_0_FUNCTION_STATUS, 0):
>>   		return 1;
> I don't think this is correct, sorry.
>
> This point has been discussed quite extensively in the SDCA group and
> the net result is that accesses to the FUNCTION_STATUS *cannot* be
> deferred. You cannot e.g. get a COMMAND_IGNORED response that would set
> the Function_Busy bit while trying to read the Function_Busy bit...
>
> Put differently, deferred access is only permitted in specific cases,
> and Function-level controls/status are not in that list.
>
> See 10.2.8.1 Rules About Which SDCA Resources Can and Cannot Use
> Deferred Access
>