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

[Mark Brown]

> 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

-- 
Mark Brown <broonie@xxxxxxxxxx>