2023-09-13 13:08:53

by Amadeusz Sławiński

[permalink] [raw]
Subject: Re: [PATCH 1/4] ASoC: soc-card: Add storage for PCI SSID

On 9/12/2023 6:32 PM, Richard Fitzgerald wrote:
> Add members to struct snd_soc_card to store the PCI subsystem ID (SSID)
> of the soundcard.
>
> The PCI specification provides two registers to store a vendor-specific
> SSID that can be read by drivers to uniquely identify a particular
> "soundcard". This is defined in the PCI specification to distinguish
> products that use the same silicon (and therefore have the same silicon
> ID) so that product-specific differences can be applied.
>
> PCI only defines 0xFFFF as an invalid value. 0x0000 is not defined as
> invalid. So the usual pattern of zero-filling the struct and then
> assuming a zero value unset will not work. A flag is included to
> indicate when the SSID information has been filled in.
>
> Unlike DMI information, which has a free-format entirely up to the vendor,
> the PCI SSID has a strictly defined format and a registry of vendor IDs.
>
> It is usual in Windows drivers that the SSID is used as the sole identifier
> of the specific end-product and the Windows driver contains tables mapping
> that to information about the hardware setup, rather than using ACPI
> properties.
>
> This SSID is important information for ASoC components that need to apply
> hardware-specific configuration on PCI-based systems.
>
> As the SSID is a generic part of the PCI specification and is treated as
> identifying the "soundcard", it is reasonable to include this information
> in struct snd_soc_card, instead of components inventing their own custom
> ways to pass this information around.
>
> Signed-off-by: Richard Fitzgerald <[email protected]>
> ---
> include/sound/soc-card.h | 37 +++++++++++++++++++++++++++++++++++++
> include/sound/soc.h | 11 +++++++++++
> 2 files changed, 48 insertions(+)
>
> diff --git a/include/sound/soc-card.h b/include/sound/soc-card.h
> index fc94dfb0021f..e8ff2e089cd0 100644
> --- a/include/sound/soc-card.h
> +++ b/include/sound/soc-card.h

...

> diff --git a/include/sound/soc.h b/include/sound/soc.h
> index 509386ff5212..81ed08c5c67d 100644
> --- a/include/sound/soc.h
> +++ b/include/sound/soc.h
> @@ -929,6 +929,17 @@ struct snd_soc_card {
> #ifdef CONFIG_DMI
> char dmi_longname[80];
> #endif /* CONFIG_DMI */
> +
> +#ifdef CONFIG_PCI
> + /*
> + * PCI does not define 0 as invalid, so pci_subsystem_set indicates
> + * whether a value has been written to these fields.
> + */
> + unsigned short pci_subsystem_vendor;
> + unsigned short pci_subsystem_device;
> + bool pci_subsystem_set;
> +#endif /* CONFIG_PCI */
> +
> char topology_shortname[32];
>
> struct device *dev;

This looks bit weird to me, snd_soc_card is _generic_ struct which is
not device specific in any way, and now you add fields for PCI, can't
this somehow be done using drvdata or something?