Re: [PATCH 3/5] ASoC: tas2783-sdw: use the BIOS speaker ID in the firmware name

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

On 11/2/26 3:46 PM, Mark Brown wrote:
>> Some boards have TAS2783 firmware in linux-firmware named
>> SSID-SPKID-LINK-0xUID.bin, with a speaker ID ahead of the link ID (the
>> HP 8EA1 has its amps on link 0 and ships 8EA1-{0,1}-0-0x*.bin). The
>> driver only builds the three-field SSID-LINK-0xUID name, so these boards
>> never load firmware. On the ASUS ROG Xbox Ally X20 (SSID 1954) the BIOS
>> provides the speaker ID in a 4-byte SPEAKER_ID EFI variable, and only
>> the four-field 1954-*-*-0x*.bin files exist.
> 
>> Read SPEAKER_ID once and request the four-field name first when it is
>> present. Boards without the variable keep the current names, and the
>> unprefixed three-field name is still tried if the four-field file is
>> missing.
> 
>> +static int tas2783_get_spk_id(void)
>> +{
>> +	efi_guid_t guid = TAS2783_SPKID_GUID;
>> +	unsigned long size = sizeof(u32);
>> +	efi_status_t status;
>> +	u32 spk_id = 0;
>> +
>> +	if (!efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE))
>> +		return -ENOENT;
>> +
>> +	status = efi.get_variable(L"SPEAKER_ID", &guid, NULL, &size, &spk_id);
>> +	if (status != EFI_SUCCESS)
>> +		return -ENOENT;
>> +
>> +	return spk_id;
> 
> This returns a u32 as an int so if someone decides to pick a very large
> value it'll look like an error value.  That's likely unrealistic though.
> 
>>  	for (; dev; dev = dev->parent) {
>>  		if (dev->bus == &pci_bus_type) {
>>  			pci = to_pci_dev(dev);
>> -			scnprintf(name, size, "%04X-%1X-%s%1X.bin",
>> -				  pci->subsystem_device, bus->link_id,
>> -				  fw_uid_prefix, unique_id);
>> +			if (!tas_dev->spk_id_read) {
>> +				tas_dev->spk_id = tas2783_get_spk_id();
>> +				tas_dev->spk_id_read = true;
>> +			}
>> +			if (tas_dev->spk_id >= 0 && !tas_dev->fw_use_fallback)
>> +				scnprintf(name, size, "%04X-%X-%1X-0x%1X.bin",
>> +					  pci->subsystem_device, tas_dev->spk_id,
>> +					  bus->link_id, unique_id);
>> +			else
>> +				scnprintf(name, size, "%04X-%1X-%s%1X.bin",
>> +					  pci->subsystem_device, bus->link_id,
>> +					  fw_uid_prefix, unique_id);
> 
> Mixing this in with the fallback decision seems confusing.  There's some
> cases generated in the Sashiko review which look plausible though I
> didn't fully work through everything:
> 
>    https://sashiko.dev/#/patchset/20261016232416.2628603-1-matthew.schwartz%40linux.dev

Thanks, didn't know Sashiko feedback wasn't in-line anymore. I'll take a look and send out a v2.

>