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

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 <[email protected]>
> ---
>  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