* [PATCH 0/4] ASoC: cs35l56: Use PCI SSID to select specific firmware
@ 2023-09-12 16:32 Richard Fitzgerald
2023-09-12 16:32 ` [PATCH 1/4] ASoC: soc-card: Add storage for PCI SSID Richard Fitzgerald
` (5 more replies)
0 siblings, 6 replies; 10+ messages in thread
From: Richard Fitzgerald @ 2023-09-12 16:32 UTC (permalink / raw)
To: broonie, pierre-louis.bossart, yung-chuan.liao, kai.vehmanen,
peter.ujfalusi
Cc: alsa-devel, patches, linux-kernel, Richard Fitzgerald
The PCI device registers contain a subsystem ID (SSID), that is
separate from the silicon ID. The PCI specification defines it thus:
"They provide a mechanism for board vendors to distiguish their
boards from one another even thought the boards may have the same
PCI controller on them."
This allows the driver for the silicon part to apply board-speficic
settings based on this SSID.
The CS35L56 driver uses this to select the correct firmware file for
the board. The actual ID is part of the PCI register set of the
host audio interface so this set of patches includes extracting the
SSID from the Intel audio controller and passing it to the machine
driver and then to ASoC components. Other PCI audio controllers
will have the same SSID registers, so can use the same mechanism to
pass the SSID.
Richard Fitzgerald (4):
ASoC: soc-card: Add storage for PCI SSID
ASoC: SOF: Pass PCI SSID to machine driver
ASoC: Intel: sof_sdw: Copy PCI SSID to struct snd_soc_card
ASoC: cs35l56: Use PCI SSID as the firmware UID
include/sound/soc-acpi.h | 7 ++++++
include/sound/soc-card.h | 37 ++++++++++++++++++++++++++++++++
include/sound/soc.h | 11 ++++++++++
include/sound/sof.h | 8 +++++++
sound/soc/codecs/cs35l56.c | 11 ++++++++++
sound/soc/intel/boards/sof_sdw.c | 6 ++++++
sound/soc/sof/sof-audio.c | 7 ++++++
sound/soc/sof/sof-pci-dev.c | 8 +++++++
8 files changed, 95 insertions(+)
--
2.30.2
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/4] ASoC: soc-card: Add storage for PCI SSID
2023-09-12 16:32 [PATCH 0/4] ASoC: cs35l56: Use PCI SSID to select specific firmware Richard Fitzgerald
@ 2023-09-12 16:32 ` Richard Fitzgerald
2023-09-13 10:56 ` Amadeusz Sławiński
2023-09-12 16:32 ` [PATCH 2/4] ASoC: SOF: Pass PCI SSID to machine driver Richard Fitzgerald
` (4 subsequent siblings)
5 siblings, 1 reply; 10+ messages in thread
From: Richard Fitzgerald @ 2023-09-12 16:32 UTC (permalink / raw)
To: broonie, pierre-louis.bossart, yung-chuan.liao, kai.vehmanen,
peter.ujfalusi
Cc: alsa-devel, patches, linux-kernel, Richard Fitzgerald
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 <rf@opensource.cirrus.com>
---
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
@@ -59,6 +59,43 @@ int snd_soc_card_add_dai_link(struct snd_soc_card *card,
void snd_soc_card_remove_dai_link(struct snd_soc_card *card,
struct snd_soc_dai_link *dai_link);
+#ifdef CONFIG_PCI
+static inline void snd_soc_card_set_pci_ssid(struct snd_soc_card *card,
+ unsigned short vendor,
+ unsigned short device)
+{
+ card->pci_subsystem_vendor = vendor;
+ card->pci_subsystem_device = device;
+ card->pci_subsystem_set = true;
+}
+
+static inline int snd_soc_card_get_pci_ssid(struct snd_soc_card *card,
+ unsigned short *vendor,
+ unsigned short *device)
+{
+ if (!card->pci_subsystem_set)
+ return -ENOENT;
+
+ *vendor = card->pci_subsystem_vendor;
+ *device = card->pci_subsystem_device;
+
+ return 0;
+}
+#else /* !CONFIG_PCI */
+static inline void snd_soc_card_set_pci_ssid(struct snd_soc_card *card,
+ unsigned short vendor,
+ unsigned short device)
+{
+}
+
+static inline int snd_soc_card_get_pci_ssid(struct snd_soc_card *card,
+ unsigned short *vendor,
+ unsigned short *device)
+{
+ return -ENOENT;
+}
+#endif /* CONFIG_PCI */
+
/* device driver data */
static inline void snd_soc_card_set_drvdata(struct snd_soc_card *card,
void *data)
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;
--
2.30.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/4] ASoC: SOF: Pass PCI SSID to machine driver
2023-09-12 16:32 [PATCH 0/4] ASoC: cs35l56: Use PCI SSID to select specific firmware Richard Fitzgerald
2023-09-12 16:32 ` [PATCH 1/4] ASoC: soc-card: Add storage for PCI SSID Richard Fitzgerald
@ 2023-09-12 16:32 ` Richard Fitzgerald
2023-09-12 16:32 ` [PATCH 3/4] ASoC: Intel: sof_sdw: Copy PCI SSID to struct snd_soc_card Richard Fitzgerald
` (3 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Richard Fitzgerald @ 2023-09-12 16:32 UTC (permalink / raw)
To: broonie, pierre-louis.bossart, yung-chuan.liao, kai.vehmanen,
peter.ujfalusi
Cc: alsa-devel, patches, linux-kernel, Richard Fitzgerald
Pass the PCI SSID of the audio interface through to the machine driver.
This allows the machine driver to use the SSID to uniquely identify the
specific hardware configuration and apply any platform-specific
configuration.
struct snd_sof_pdata is passed around inside the SOF code, but it then
passes configuration information to the machine driver through
struct snd_soc_acpi_mach and struct snd_soc_acpi_mach_params. So SSID
information has been added to both snd_sof_pdata and
snd_soc_acpi_mach_params.
PCI does not define 0x0000 as an invalid value so we can't use zero to
indicate that the struct member was not written. Instead a flag is
included to indicate that a value has been written to the
subsystem_vendor and subsystem_device members.
sof_pci_probe() creates the struct snd_sof_pdata. It is passed a struct
pci_dev so it can fill in the SSID value.
sof_machine_check() finds the appropriate struct snd_soc_acpi_mach. It
copies the SSID information across to the struct snd_soc_acpi_mach_params.
This done before calling any custom set_mach_params() so that it could be
used by the set_mach_params() callback to apply variant params.
The machine driver receives the struct snd_soc_acpi_mach as its
platform_data.
Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
include/sound/soc-acpi.h | 7 +++++++
include/sound/sof.h | 8 ++++++++
sound/soc/sof/sof-audio.c | 7 +++++++
sound/soc/sof/sof-pci-dev.c | 8 ++++++++
4 files changed, 30 insertions(+)
diff --git a/include/sound/soc-acpi.h b/include/sound/soc-acpi.h
index 6d31d535e8f6..23d6d6bfb073 100644
--- a/include/sound/soc-acpi.h
+++ b/include/sound/soc-acpi.h
@@ -68,6 +68,10 @@ static inline struct snd_soc_acpi_mach *snd_soc_acpi_codec_list(void *arg)
* @i2s_link_mask: I2S/TDM links enabled on the board
* @num_dai_drivers: number of elements in @dai_drivers
* @dai_drivers: pointer to dai_drivers, used e.g. in nocodec mode
+ * @subsystem_vendor: optional PCI SSID vendor value
+ * @subsystem_device: optional PCI SSID device value
+ * @subsystem_id_set: true if a value has been written to
+ * subsystem_vendor and subsystem_device.
*/
struct snd_soc_acpi_mach_params {
u32 acpi_ipc_irq_index;
@@ -80,6 +84,9 @@ struct snd_soc_acpi_mach_params {
u32 i2s_link_mask;
u32 num_dai_drivers;
struct snd_soc_dai_driver *dai_drivers;
+ unsigned short subsystem_vendor;
+ unsigned short subsystem_device;
+ bool subsystem_id_set;
};
/**
diff --git a/include/sound/sof.h b/include/sound/sof.h
index d3c41f87ac31..51294f2ba302 100644
--- a/include/sound/sof.h
+++ b/include/sound/sof.h
@@ -64,6 +64,14 @@ struct snd_sof_pdata {
const char *name;
const char *platform;
+ /*
+ * PCI SSID. As PCI does not define 0 as invalid, the subsystem_id_set
+ * flag indicates that a value has been written to these members.
+ */
+ unsigned short subsystem_vendor;
+ unsigned short subsystem_device;
+ bool subsystem_id_set;
+
struct device *dev;
/*
diff --git a/sound/soc/sof/sof-audio.c b/sound/soc/sof/sof-audio.c
index e7ef77012c35..9c2359d10ecf 100644
--- a/sound/soc/sof/sof-audio.c
+++ b/sound/soc/sof/sof-audio.c
@@ -1031,6 +1031,13 @@ int sof_machine_check(struct snd_sof_dev *sdev)
mach = snd_sof_machine_select(sdev);
if (mach) {
sof_pdata->machine = mach;
+
+ if (sof_pdata->subsystem_id_set) {
+ mach->mach_params.subsystem_vendor = sof_pdata->subsystem_vendor;
+ mach->mach_params.subsystem_device = sof_pdata->subsystem_device;
+ mach->mach_params.subsystem_id_set = true;
+ }
+
snd_sof_set_mach_params(mach, sdev);
return 0;
}
diff --git a/sound/soc/sof/sof-pci-dev.c b/sound/soc/sof/sof-pci-dev.c
index f5ece43d0ec2..146d25983b08 100644
--- a/sound/soc/sof/sof-pci-dev.c
+++ b/sound/soc/sof/sof-pci-dev.c
@@ -214,6 +214,14 @@ int sof_pci_probe(struct pci_dev *pci, const struct pci_device_id *pci_id)
return ret;
sof_pdata->name = pci_name(pci);
+
+ /* PCI defines a vendor ID of 0xFFFF as invalid. */
+ if (pci->subsystem_vendor != 0xFFFF) {
+ sof_pdata->subsystem_vendor = pci->subsystem_vendor;
+ sof_pdata->subsystem_device = pci->subsystem_device;
+ sof_pdata->subsystem_id_set = true;
+ }
+
sof_pdata->desc = desc;
sof_pdata->dev = dev;
--
2.30.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/4] ASoC: Intel: sof_sdw: Copy PCI SSID to struct snd_soc_card
2023-09-12 16:32 [PATCH 0/4] ASoC: cs35l56: Use PCI SSID to select specific firmware Richard Fitzgerald
2023-09-12 16:32 ` [PATCH 1/4] ASoC: soc-card: Add storage for PCI SSID Richard Fitzgerald
2023-09-12 16:32 ` [PATCH 2/4] ASoC: SOF: Pass PCI SSID to machine driver Richard Fitzgerald
@ 2023-09-12 16:32 ` Richard Fitzgerald
2023-09-12 16:32 ` [PATCH 4/4] ASoC: cs35l56: Use PCI SSID as the firmware UID Richard Fitzgerald
` (2 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Richard Fitzgerald @ 2023-09-12 16:32 UTC (permalink / raw)
To: broonie, pierre-louis.bossart, yung-chuan.liao, kai.vehmanen,
peter.ujfalusi
Cc: alsa-devel, patches, linux-kernel, Richard Fitzgerald
If the PCI SSID has been set in the struct snd_soc_acpi_mach_params,
copy this to struct snd_soc_card so that it can be used by other
ASoC components.
This is important for components that must apply system-specific
configuration.
Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
sound/soc/intel/boards/sof_sdw.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/sound/soc/intel/boards/sof_sdw.c b/sound/soc/intel/boards/sof_sdw.c
index 5a1c750e6ae6..961241100012 100644
--- a/sound/soc/intel/boards/sof_sdw.c
+++ b/sound/soc/intel/boards/sof_sdw.c
@@ -1924,6 +1924,12 @@ static int mc_probe(struct platform_device *pdev)
for (i = 0; i < ARRAY_SIZE(codec_info_list); i++)
codec_info_list[i].amp_num = 0;
+ if (mach->mach_params.subsystem_id_set) {
+ snd_soc_card_set_pci_ssid(card,
+ mach->mach_params.subsystem_vendor,
+ mach->mach_params.subsystem_device);
+ }
+
ret = sof_card_dai_links_create(card);
if (ret < 0)
return ret;
--
2.30.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 4/4] ASoC: cs35l56: Use PCI SSID as the firmware UID
2023-09-12 16:32 [PATCH 0/4] ASoC: cs35l56: Use PCI SSID to select specific firmware Richard Fitzgerald
` (2 preceding siblings ...)
2023-09-12 16:32 ` [PATCH 3/4] ASoC: Intel: sof_sdw: Copy PCI SSID to struct snd_soc_card Richard Fitzgerald
@ 2023-09-12 16:32 ` Richard Fitzgerald
2023-09-12 17:39 ` [PATCH 0/4] ASoC: cs35l56: Use PCI SSID to select specific firmware Pierre-Louis Bossart
2023-09-14 11:19 ` Mark Brown
5 siblings, 0 replies; 10+ messages in thread
From: Richard Fitzgerald @ 2023-09-12 16:32 UTC (permalink / raw)
To: broonie, pierre-louis.bossart, yung-chuan.liao, kai.vehmanen,
peter.ujfalusi
Cc: alsa-devel, patches, linux-kernel, Richard Fitzgerald
If the driver properties do not define a cirrus,firmware-uid try to get the
PCI SSID as the UID.
On PCI-based systems the PCI SSID is used to uniquely identify the specific
sound hardware. This is the standard mechanism for x86 systems and is the
way to get a unique system identifier for systems that use the CS35L56 on
SoundWire.
For non-SoundWire systems there is no Windows equivalent of the ASoC driver
in I2C/SPI mode. These would be:
1. HDA systems, which are handled by the HDA subsystem.
2. Linux-specific systems.
3. Composite devices where the cs35l56 is not present in ACPI and is
configured using software nodes.
Case 2 can use the firmware-uid property, though the PCI SSID is supported
as an alternative, as it is the standard PCI mechanism.
Case 3 is a SoundWire system where some other codec is the SoundWire bridge
device and CS35L56 is not listed in ACPI. As these are SoundWire systems
they will normally use the PCI SSID.
Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
sound/soc/codecs/cs35l56.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/sound/soc/codecs/cs35l56.c b/sound/soc/codecs/cs35l56.c
index f2e7c6d0be46..e6e366333a47 100644
--- a/sound/soc/codecs/cs35l56.c
+++ b/sound/soc/codecs/cs35l56.c
@@ -772,9 +772,20 @@ static int cs35l56_component_probe(struct snd_soc_component *component)
{
struct cs35l56_private *cs35l56 = snd_soc_component_get_drvdata(component);
struct dentry *debugfs_root = component->debugfs_root;
+ unsigned short vendor, device;
BUILD_BUG_ON(ARRAY_SIZE(cs35l56_tx_input_texts) != ARRAY_SIZE(cs35l56_tx_input_values));
+ if (!cs35l56->dsp.system_name &&
+ (snd_soc_card_get_pci_ssid(component->card, &vendor, &device) == 0)) {
+ cs35l56->dsp.system_name = devm_kasprintf(cs35l56->base.dev,
+ GFP_KERNEL,
+ "%04x%04x",
+ vendor, device);
+ if (!cs35l56->dsp.system_name)
+ return -ENOMEM;
+ }
+
if (!wait_for_completion_timeout(&cs35l56->init_completion,
msecs_to_jiffies(5000))) {
dev_err(cs35l56->base.dev, "%s: init_completion timed out\n", __func__);
--
2.30.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 0/4] ASoC: cs35l56: Use PCI SSID to select specific firmware
2023-09-12 16:32 [PATCH 0/4] ASoC: cs35l56: Use PCI SSID to select specific firmware Richard Fitzgerald
` (3 preceding siblings ...)
2023-09-12 16:32 ` [PATCH 4/4] ASoC: cs35l56: Use PCI SSID as the firmware UID Richard Fitzgerald
@ 2023-09-12 17:39 ` Pierre-Louis Bossart
2023-09-14 11:19 ` Mark Brown
5 siblings, 0 replies; 10+ messages in thread
From: Pierre-Louis Bossart @ 2023-09-12 17:39 UTC (permalink / raw)
To: Richard Fitzgerald, broonie, yung-chuan.liao, kai.vehmanen,
peter.ujfalusi
Cc: alsa-devel, patches, linux-kernel
On 9/12/23 12:32, Richard Fitzgerald wrote:
> The PCI device registers contain a subsystem ID (SSID), that is
> separate from the silicon ID. The PCI specification defines it thus:
>
> "They provide a mechanism for board vendors to distiguish their
> boards from one another even thought the boards may have the same
> PCI controller on them."
>
> This allows the driver for the silicon part to apply board-speficic
> settings based on this SSID.
>
> The CS35L56 driver uses this to select the correct firmware file for
> the board. The actual ID is part of the PCI register set of the
> host audio interface so this set of patches includes extracting the
> SSID from the Intel audio controller and passing it to the machine
> driver and then to ASoC components. Other PCI audio controllers
> will have the same SSID registers, so can use the same mechanism to
> pass the SSID.
>
> Richard Fitzgerald (4):
> ASoC: soc-card: Add storage for PCI SSID
> ASoC: SOF: Pass PCI SSID to machine driver
> ASoC: Intel: sof_sdw: Copy PCI SSID to struct snd_soc_card
> ASoC: cs35l56: Use PCI SSID as the firmware UID
for the series
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>
> include/sound/soc-acpi.h | 7 ++++++
> include/sound/soc-card.h | 37 ++++++++++++++++++++++++++++++++
> include/sound/soc.h | 11 ++++++++++
> include/sound/sof.h | 8 +++++++
> sound/soc/codecs/cs35l56.c | 11 ++++++++++
> sound/soc/intel/boards/sof_sdw.c | 6 ++++++
> sound/soc/sof/sof-audio.c | 7 ++++++
> sound/soc/sof/sof-pci-dev.c | 8 +++++++
> 8 files changed, 95 insertions(+)
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/4] ASoC: soc-card: Add storage for PCI SSID
2023-09-12 16:32 ` [PATCH 1/4] ASoC: soc-card: Add storage for PCI SSID Richard Fitzgerald
@ 2023-09-13 10:56 ` Amadeusz Sławiński
2023-09-13 12:58 ` Mark Brown
0 siblings, 1 reply; 10+ messages in thread
From: Amadeusz Sławiński @ 2023-09-13 10:56 UTC (permalink / raw)
To: Richard Fitzgerald, broonie, pierre-louis.bossart,
yung-chuan.liao, kai.vehmanen, peter.ujfalusi
Cc: alsa-devel, patches, linux-kernel, Cezary Rojewski
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 <rf@opensource.cirrus.com>
> ---
> 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?
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/4] ASoC: soc-card: Add storage for PCI SSID
2023-09-13 10:56 ` Amadeusz Sławiński
@ 2023-09-13 12:58 ` Mark Brown
2023-09-13 13:29 ` Richard Fitzgerald
0 siblings, 1 reply; 10+ messages in thread
From: Mark Brown @ 2023-09-13 12:58 UTC (permalink / raw)
To: Amadeusz Sławiński
Cc: Richard Fitzgerald, pierre-louis.bossart, yung-chuan.liao,
kai.vehmanen, peter.ujfalusi, alsa-devel, patches, linux-kernel,
Cezary Rojewski
[-- Attachment #1: Type: text/plain, Size: 1008 bytes --]
On Wed, Sep 13, 2023 at 12:56:03PM +0200, Amadeusz Sławiński wrote:
> On 9/12/2023 6:32 PM, Richard Fitzgerald wrote:
> > +#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?
You're right that it's a bit messy but if we use driver data then it
becomes specific to a particular driver and there's a goal here to share
with subfunction drivers. I was thinking we could refactor to a union
or otherwise extend if we find other users with a similar need.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/4] ASoC: soc-card: Add storage for PCI SSID
2023-09-13 12:58 ` Mark Brown
@ 2023-09-13 13:29 ` Richard Fitzgerald
0 siblings, 0 replies; 10+ messages in thread
From: Richard Fitzgerald @ 2023-09-13 13:29 UTC (permalink / raw)
To: Mark Brown, Amadeusz Sławiński
Cc: pierre-louis.bossart, yung-chuan.liao, kai.vehmanen,
peter.ujfalusi, alsa-devel, patches, linux-kernel,
Cezary Rojewski
On 13/09/2023 13:58, Mark Brown wrote:
> On Wed, Sep 13, 2023 at 12:56:03PM +0200, Amadeusz Sławiński wrote:
>> On 9/12/2023 6:32 PM, Richard Fitzgerald wrote:
>
>>> +#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?
>
> You're right that it's a bit messy but if we use driver data then it
> becomes specific to a particular driver and there's a goal here to share
> with subfunction drivers. I was thinking we could refactor to a union
> or otherwise extend if we find other users with a similar need.
Yes, I was trying to avoid multiple custom ways of passing the same
data around. A significant advantage of explicitly passing the SSID
(if it's available) rather than a more abstract identifier (such as a
char *) is that's it's very well defined exactly what a PCI SSID is so
we know we can use it verbatim. A more abstract identifier creates an
implied trust (or mistrust) between the machine driver and the component
receiving it whether it's unique and in a useful format.
I could de-ugly it a bit by moving it out into a separate struct/union
and having just a member of that struct type in snd_soc_card.
An alternative was to add a function like the existing
snd_soc_component_set_whatever() family but that means adding a callback
pointer to struct snd_soc_component_driver, which is creating more
space overhead than one value in the snd_soc_card.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/4] ASoC: cs35l56: Use PCI SSID to select specific firmware
2023-09-12 16:32 [PATCH 0/4] ASoC: cs35l56: Use PCI SSID to select specific firmware Richard Fitzgerald
` (4 preceding siblings ...)
2023-09-12 17:39 ` [PATCH 0/4] ASoC: cs35l56: Use PCI SSID to select specific firmware Pierre-Louis Bossart
@ 2023-09-14 11:19 ` Mark Brown
5 siblings, 0 replies; 10+ messages in thread
From: Mark Brown @ 2023-09-14 11:19 UTC (permalink / raw)
To: pierre-louis.bossart, yung-chuan.liao, kai.vehmanen,
peter.ujfalusi, Richard Fitzgerald
Cc: alsa-devel, patches, linux-kernel
On Tue, 12 Sep 2023 17:32:03 +0100, Richard Fitzgerald wrote:
> The PCI device registers contain a subsystem ID (SSID), that is
> separate from the silicon ID. The PCI specification defines it thus:
>
> "They provide a mechanism for board vendors to distiguish their
> boards from one another even thought the boards may have the same
> PCI controller on them."
>
> [...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/4] ASoC: soc-card: Add storage for PCI SSID
commit: 47f56e38a199bd45514b8e0142399cba4feeaf1a
[2/4] ASoC: SOF: Pass PCI SSID to machine driver
commit: ba2de401d32625fe538d3f2c00ca73740dd2d516
[3/4] ASoC: Intel: sof_sdw: Copy PCI SSID to struct snd_soc_card
commit: d8b387544ff4d02eda1d1839a0c601de4b037c33
[4/4] ASoC: cs35l56: Use PCI SSID as the firmware UID
commit: 1a1c3d794ef65ef2978c5e65e1aed3fe6f014e90
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-09-14 11:19 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-12 16:32 [PATCH 0/4] ASoC: cs35l56: Use PCI SSID to select specific firmware Richard Fitzgerald
2023-09-12 16:32 ` [PATCH 1/4] ASoC: soc-card: Add storage for PCI SSID Richard Fitzgerald
2023-09-13 10:56 ` Amadeusz Sławiński
2023-09-13 12:58 ` Mark Brown
2023-09-13 13:29 ` Richard Fitzgerald
2023-09-12 16:32 ` [PATCH 2/4] ASoC: SOF: Pass PCI SSID to machine driver Richard Fitzgerald
2023-09-12 16:32 ` [PATCH 3/4] ASoC: Intel: sof_sdw: Copy PCI SSID to struct snd_soc_card Richard Fitzgerald
2023-09-12 16:32 ` [PATCH 4/4] ASoC: cs35l56: Use PCI SSID as the firmware UID Richard Fitzgerald
2023-09-12 17:39 ` [PATCH 0/4] ASoC: cs35l56: Use PCI SSID to select specific firmware Pierre-Louis Bossart
2023-09-14 11:19 ` Mark Brown
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox