Re: [PATCH 3/3] ASoC: tas2783-sdw: refuse playback when the init writes failed

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

On 10/19/26 9:10 AM, Pierre-Louis Bossart wrote:
> On 10/18/26 22:00, Matthew Schwartz wrote:
>> When the SDCA function initialization writes fail, tas_io_init() leaves
>> hw_init false, but the firmware download has already set fw_dl_success,
>> so tas_sdw_hw_params() goes on to play through an amp that is missing
>> part of its setup. On the ASUS ROG Xbox Ally X20 those writes come from
>> the ACPI DisCo initialization table and include the posture, which the
>> BIOS sets differently on each amp.
>>
>> Refuse playback until the init writes have completed as well, and log
>> which of the two steps is missing. A stream that is only prepared again
>> after a resume does not go through hw_params, so check in the port
>> prepare as well.
> 
> Same comment as the previous patch. This patch logs an error message for
> a critical error, which seems fine, but what does it take to make the
> init writes succeed in the first place?
> IOW, this patch flags an incorrect setup without giving information on
> what possible corrective actions could be taken.

Same as the last one, I haven't seen this fail on the X20. The init
writes come from the DisCo table on the X20, or from tas2783_init_seq
on platforms without one, so they'd fail either on a bus error while
the amp attaches or on an entry that the regmap or the amp rejects. A
bus error can clear up, since hw_init stays false and the init runs
again on the next attach, which on the X20 is the next idle wake. A
rejected entry fails every time, and fixing it means knowing which
entry it was, which the log doesn't tell you today, as
sdca_regmap_write_init() just returns the first error. I'll log the
failing address and value in v2 so a report says what needs fixing.

> 
>> Fixes: ca8f3611dcf8 ("ASoC: tas2783A: use acpi initialisation table")
>> Signed-off-by: Matthew Schwartz <matthew.schwartz@linux.dev>
>> ---
>>  sound/soc/codecs/tas2783-sdw.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/sound/soc/codecs/tas2783-sdw.c b/sound/soc/codecs/tas2783-sdw.c
>> index 8e7376889b6d..6c4cc8868415 100644
>> --- a/sound/soc/codecs/tas2783-sdw.c
>> +++ b/sound/soc/codecs/tas2783-sdw.c
>> @@ -1055,6 +1055,11 @@ static s32 tas_sdw_hw_params(struct snd_pcm_substream *substream,
>>  		return -EINVAL;
>>  	}
>>  
>> +	if (!tas_dev->hw_init) {
>> +		dev_err(tas_dev->dev, "error playback without init writes");
>> +		return -EINVAL;
>> +	}
>> +
>>  	sdw_stream = snd_soc_dai_get_dma_data(dai, substream);
>>  	if (!sdw_stream)
>>  		return -EINVAL;
>> @@ -1427,6 +1432,12 @@ static int tas_port_prep(struct sdw_slave *slave, struct sdw_prepare_ch *prep_ch
>>  	addr = SDW_DPN_PREPARECTRL(prep_ch->num);
>>  	switch (pre_ops) {
>>  	case SDW_OPS_PORT_PRE_PREP:
>> +		if (!tas_dev->hw_init) {
>> +			dev_err(dev, "error port %d prepare without init writes\n",
>> +				prep_ch->num);
>> +			return -EINVAL;
>> +		}
>> +
>>  		/*
>>  		 * The Function has to be powered before the port can complete
>>  		 * channel preparation.  hw_params() does that when a stream is
>