[PATCH] ASoC: soc-acpi: add alternative id field for machine driver matching

[PATCH] ASoC: soc-acpi: add alternative id field for machine driver matching

[Brent Lu]

Current design to support second headphone driver in the same machine
driver is to duplicate the entries in snd_soc_acpi_mach array and
board configs in machine driver. We can avoid this by adding an id_alt
field in snd_soc_acpi_mach structure to specify alternative ACPI HIDs
for machine driver enumeration and leave the codec type detection to
machine driver if necessary.

Signed-off-by: Brent Lu <brent.lu@intel.com>
---
 include/sound/soc-acpi.h |  2 ++
 sound/soc/soc-acpi.c     | 21 ++++++++++++++++++++-
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/include/sound/soc-acpi.h b/include/sound/soc-acpi.h
index 2f3fa385c092..fcf6bae9f9d7 100644
--- a/include/sound/soc-acpi.h
+++ b/include/sound/soc-acpi.h
@@ -129,6 +129,7 @@ struct snd_soc_acpi_link_adr {
  * all firmware/topology related fields.
  *
  * @id: ACPI ID (usually the codec's) used to find a matching machine driver.
+ * @id_alt: array of ACPI IDs used as an alternative of id field.
  * @link_mask: describes required board layout, e.g. for SoundWire.
  * @links: array of link _ADR descriptors, null terminated.
  * @drv_name: machine driver name
@@ -146,6 +147,7 @@ struct snd_soc_acpi_link_adr {
 /* Descriptor for SST ASoC machine driver */
 struct snd_soc_acpi_mach {
 	const u8 id[ACPI_ID_LEN];
+	struct snd_soc_acpi_codecs *id_alt;
 	const u32 link_mask;
 	const struct snd_soc_acpi_link_adr *links;
 	const char *drv_name;
diff --git a/sound/soc/soc-acpi.c b/sound/soc/soc-acpi.c
index 395229bf5c51..ab67d640c20f 100644
--- a/sound/soc/soc-acpi.c
+++ b/sound/soc/soc-acpi.c
@@ -8,6 +8,25 @@
 #include <linux/module.h>
 #include <sound/soc-acpi.h>
 
+static bool snd_soc_acpi_id_present(struct snd_soc_acpi_mach *machine)
+{
+	struct snd_soc_acpi_codecs *id_alt = machine->id_alt;
+	int i;
+
+	if (acpi_dev_present(machine->id, NULL, -1))
+		return true;
+
+	if (id_alt == NULL)
+		return false;
+
+	for (i = 0; i < id_alt->num_codecs; i++) {
+		if (acpi_dev_present(id_alt->codecs[i], NULL, -1))
+			return true;
+	}
+
+	return false;
+}
+
 struct snd_soc_acpi_mach *
 snd_soc_acpi_find_machine(struct snd_soc_acpi_mach *machines)
 {
@@ -15,7 +34,7 @@ snd_soc_acpi_find_machine(struct snd_soc_acpi_mach *machines)
 	struct snd_soc_acpi_mach *mach_alt;
 
 	for (mach = machines; mach->id[0]; mach++) {
-		if (acpi_dev_present(mach->id, NULL, -1)) {
+		if (snd_soc_acpi_id_present(mach) != false) {
 			if (mach->machine_quirk) {
 				mach_alt = mach->machine_quirk(mach);
 				if (!mach_alt)
-- 
2.25.1

Re: [PATCH] ASoC: soc-acpi: add alternative id field for machine driver matching

[Pierre-Louis Bossart]

On 10/6/21 3:43 AM, Brent Lu wrote:
> Current design to support second headphone driver in the same machine
> driver is to duplicate the entries in snd_soc_acpi_mach array and
> board configs in machine driver. We can avoid this by adding an id_alt
> field in snd_soc_acpi_mach structure to specify alternative ACPI HIDs
> for machine driver enumeration and leave the codec type detection to
> machine driver if necessary.

I am not following your suggestion. The machine drivers for I2S/TDM
platforms are typically based on specific headphone codecs, and they we
add possible swaps for amplifiers. The key to find a machine is
typically the headphone HID. Exhibit A for this in your own contribution
in the recent weeks with the sof_cs42l42.c machine driver.

Are you suggesting we unify e.g. sof_rt5682.c and sof_cs42l42.c?

The other problem is that today we have one main HID along with
'quirk_data' for amplifiers. If we have alternate HIDs, what would be
the rule for quirk_data? Can the quirks apply to all possible alternate
HIDs? Only one of them?

Without an example where this new alternate ID is used it's hard to see
what the ask and directions might be.

Care to elaborate?
Thanks!

> Signed-off-by: Brent Lu <brent.lu@intel.com>
> ---
>  include/sound/soc-acpi.h |  2 ++
>  sound/soc/soc-acpi.c     | 21 ++++++++++++++++++++-
>  2 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/include/sound/soc-acpi.h b/include/sound/soc-acpi.h
> index 2f3fa385c092..fcf6bae9f9d7 100644
> --- a/include/sound/soc-acpi.h
> +++ b/include/sound/soc-acpi.h
> @@ -129,6 +129,7 @@ struct snd_soc_acpi_link_adr {
>   * all firmware/topology related fields.
>   *
>   * @id: ACPI ID (usually the codec's) used to find a matching machine driver.
> + * @id_alt: array of ACPI IDs used as an alternative of id field.
>   * @link_mask: describes required board layout, e.g. for SoundWire.
>   * @links: array of link _ADR descriptors, null terminated.
>   * @drv_name: machine driver name
> @@ -146,6 +147,7 @@ struct snd_soc_acpi_link_adr {
>  /* Descriptor for SST ASoC machine driver */
>  struct snd_soc_acpi_mach {
>  	const u8 id[ACPI_ID_LEN];
> +	struct snd_soc_acpi_codecs *id_alt;
>  	const u32 link_mask;
>  	const struct snd_soc_acpi_link_adr *links;
>  	const char *drv_name;
> diff --git a/sound/soc/soc-acpi.c b/sound/soc/soc-acpi.c
> index 395229bf5c51..ab67d640c20f 100644
> --- a/sound/soc/soc-acpi.c
> +++ b/sound/soc/soc-acpi.c
> @@ -8,6 +8,25 @@
>  #include <linux/module.h>
>  #include <sound/soc-acpi.h>
>  
> +static bool snd_soc_acpi_id_present(struct snd_soc_acpi_mach *machine)
> +{
> +	struct snd_soc_acpi_codecs *id_alt = machine->id_alt;
> +	int i;
> +
> +	if (acpi_dev_present(machine->id, NULL, -1))
> +		return true;
> +
> +	if (id_alt == NULL)
> +		return false;
> +
> +	for (i = 0; i < id_alt->num_codecs; i++) {
> +		if (acpi_dev_present(id_alt->codecs[i], NULL, -1))
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
>  struct snd_soc_acpi_mach *
>  snd_soc_acpi_find_machine(struct snd_soc_acpi_mach *machines)
>  {
> @@ -15,7 +34,7 @@ snd_soc_acpi_find_machine(struct snd_soc_acpi_mach *machines)
>  	struct snd_soc_acpi_mach *mach_alt;
>  
>  	for (mach = machines; mach->id[0]; mach++) {
> -		if (acpi_dev_present(mach->id, NULL, -1)) {
> +		if (snd_soc_acpi_id_present(mach) != false) {
>  			if (mach->machine_quirk) {
>  				mach_alt = mach->machine_quirk(mach);
>  				if (!mach_alt)
> 

RE: [PATCH] ASoC: soc-acpi: add alternative id field for machine driver matching

[Lu, Brent]

> 
> I am not following your suggestion. The machine drivers for I2S/TDM platforms
> are typically based on specific headphone codecs, and they we add possible
> swaps for amplifiers. The key to find a machine is typically the headphone HID.
> Exhibit A for this in your own contribution in the recent weeks with the
> sof_cs42l42.c machine driver.
> 
> Are you suggesting we unify e.g. sof_rt5682.c and sof_cs42l42.c?
> 
> The other problem is that today we have one main HID along with 'quirk_data'
> for amplifiers. If we have alternate HIDs, what would be the rule for quirk_data?
> Can the quirks apply to all possible alternate HIDs? Only one of them?
> 
> Without an example where this new alternate ID is used it's hard to see what the
> ask and directions might be.
> 

I've sent the patch again with two follow-up patches for the changes to machine driver
and the enumeration. Currently the sof-rt5682.c supports two headphone codec drivers,
rt5682 and rt5682s, enumerated by different HID. So we need to duplicate all the entries
in enumeration tables and board configs in machine driver to support rt5682s. By adding
an extra field id_alt to the enumeration table, we just need to modify existing entries
instead of creating new ones.

For amplifier, I think we can implement an new machine_quirk function to support multiple
amplifier HID.