[PATCH 0/3] Multiple headphone codec driver support

[PATCH 0/3] Multiple headphone codec driver support

[Brent Lu]

Support multiple headphone drivers in same machine driver. In this
case, both rt5682 and rt5682s are supported and enumerated by different
ACPI HID "10EC5682" and "RTL5682".

Brent Lu (3):
  ASoC: soc-acpi: add alternative id field for machine driver matching
  ASoC: Intel: sof_rt5682: detect codec variant in probe function
  ASoC: Intel: sof_rt5682: use id_alt to enumerate rt5682s

 include/sound/soc-acpi.h                      |  2 ++
 sound/soc/intel/boards/sof_rt5682.c           | 34 +++----------------
 .../intel/common/soc-acpi-intel-adl-match.c   |  8 +++++
 .../intel/common/soc-acpi-intel-byt-match.c   |  6 ++++
 .../intel/common/soc-acpi-intel-cht-match.c   |  6 ++++
 .../intel/common/soc-acpi-intel-cml-match.c   |  8 +++++
 .../intel/common/soc-acpi-intel-icl-match.c   |  6 ++++
 .../intel/common/soc-acpi-intel-jsl-match.c   | 32 +++++------------
 .../intel/common/soc-acpi-intel-tgl-match.c   |  8 +++++
 sound/soc/soc-acpi.c                          | 21 +++++++++++-
 10 files changed, 76 insertions(+), 55 deletions(-)

-- 
2.25.1

[PATCH 0/3] Multiple headphone codec driver support

[Brent Lu]

Support multiple headphone drivers in same machine driver. In this
case, both rt5682 and rt5682s are supported and enumerated by different
ACPI HID "10EC5682" and "RTL5682".

Brent Lu (3):
  ASoC: soc-acpi: add alternative id field for machine driver matching
  ASoC: Intel: sof_rt5682: detect codec variant in probe function
  ASoC: Intel: sof_rt5682: use id_alt to enumerate rt5682s

 include/sound/soc-acpi.h                      |  2 ++
 sound/soc/intel/boards/sof_rt5682.c           | 34 +++----------------
 .../intel/common/soc-acpi-intel-adl-match.c   |  8 +++++
 .../intel/common/soc-acpi-intel-byt-match.c   |  6 ++++
 .../intel/common/soc-acpi-intel-cht-match.c   |  6 ++++
 .../intel/common/soc-acpi-intel-cml-match.c   |  8 +++++
 .../intel/common/soc-acpi-intel-icl-match.c   |  6 ++++
 .../intel/common/soc-acpi-intel-jsl-match.c   | 32 +++++------------
 .../intel/common/soc-acpi-intel-tgl-match.c   |  8 +++++
 sound/soc/soc-acpi.c                          | 21 +++++++++++-
 10 files changed, 76 insertions(+), 55 deletions(-)

-- 
2.25.1

[PATCH 1/3] 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

[PATCH 1/3] 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

[PATCH 2/3] ASoC: Intel: sof_rt5682: detect codec variant in probe function

[Brent Lu]

Detect whether the headphone codec is ALC5682I-VS or not in probe
function so we don't need to duplicate all board configs for this new
variant.

Signed-off-by: Brent Lu <brent.lu@intel.com>
---
 sound/soc/intel/boards/sof_rt5682.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/sound/soc/intel/boards/sof_rt5682.c b/sound/soc/intel/boards/sof_rt5682.c
index 613662eedd0d..9f1e5ef11b13 100644
--- a/sound/soc/intel/boards/sof_rt5682.c
+++ b/sound/soc/intel/boards/sof_rt5682.c
@@ -864,6 +864,10 @@ static int sof_audio_probe(struct platform_device *pdev)
 	if ((sof_rt5682_quirk & SOF_SPEAKER_AMP_PRESENT) && !mach->quirk_data)
 		sof_rt5682_quirk &= ~SOF_SPEAKER_AMP_PRESENT;
 
+	/* Detect the headset codec variant ALC5682I-VS */
+	if (acpi_dev_present("RTL5682", NULL, -1))
+		sof_rt5682_quirk |= SOF_RT5682S_HEADPHONE_CODEC_PRESENT;
+
 	if (soc_intel_is_byt() || soc_intel_is_cht()) {
 		is_legacy_cpu = 1;
 		dmic_be_num = 0;
-- 
2.25.1

[PATCH 2/3] ASoC: Intel: sof_rt5682: detect codec variant in probe function

[Brent Lu]

Detect whether the headphone codec is ALC5682I-VS or not in probe
function so we don't need to duplicate all board configs for this new
variant.

Signed-off-by: Brent Lu <brent.lu@intel.com>
---
 sound/soc/intel/boards/sof_rt5682.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/sound/soc/intel/boards/sof_rt5682.c b/sound/soc/intel/boards/sof_rt5682.c
index 613662eedd0d..9f1e5ef11b13 100644
--- a/sound/soc/intel/boards/sof_rt5682.c
+++ b/sound/soc/intel/boards/sof_rt5682.c
@@ -864,6 +864,10 @@ static int sof_audio_probe(struct platform_device *pdev)
 	if ((sof_rt5682_quirk & SOF_SPEAKER_AMP_PRESENT) && !mach->quirk_data)
 		sof_rt5682_quirk &= ~SOF_SPEAKER_AMP_PRESENT;
 
+	/* Detect the headset codec variant ALC5682I-VS */
+	if (acpi_dev_present("RTL5682", NULL, -1))
+		sof_rt5682_quirk |= SOF_RT5682S_HEADPHONE_CODEC_PRESENT;
+
 	if (soc_intel_is_byt() || soc_intel_is_cht()) {
 		is_legacy_cpu = 1;
 		dmic_be_num = 0;
-- 
2.25.1

[PATCH 3/3] ASoC: Intel: sof_rt5682: use id_alt to enumerate rt5682s

[Brent Lu]

Use the id_alt field to enumerate rt5682s headphone codec and remove
redundant entries in tables.

Signed-off-by: Brent Lu <brent.lu@intel.com>
---
 sound/soc/intel/boards/sof_rt5682.c           | 30 -----------------
 .../intel/common/soc-acpi-intel-adl-match.c   |  8 +++++
 .../intel/common/soc-acpi-intel-byt-match.c   |  6 ++++
 .../intel/common/soc-acpi-intel-cht-match.c   |  6 ++++
 .../intel/common/soc-acpi-intel-cml-match.c   |  8 +++++
 .../intel/common/soc-acpi-intel-icl-match.c   |  6 ++++
 .../intel/common/soc-acpi-intel-jsl-match.c   | 32 +++++--------------
 .../intel/common/soc-acpi-intel-tgl-match.c   |  8 +++++
 8 files changed, 50 insertions(+), 54 deletions(-)

diff --git a/sound/soc/intel/boards/sof_rt5682.c b/sound/soc/intel/boards/sof_rt5682.c
index 9f1e5ef11b13..9819345cd84c 100644
--- a/sound/soc/intel/boards/sof_rt5682.c
+++ b/sound/soc/intel/boards/sof_rt5682.c
@@ -1050,36 +1050,6 @@ static const struct platform_device_id board_ids[] = {
 					SOF_RT5682_SSP_AMP(2) |
 					SOF_RT5682_NUM_HDMIDEV(4)),
 	},
-	{
-		.name = "jsl_rt5682s_rt1015",
-		.driver_data = (kernel_ulong_t)(SOF_RT5682_MCLK_EN |
-					SOF_RT5682_MCLK_24MHZ |
-					SOF_RT5682_SSP_CODEC(0) |
-					SOF_RT5682S_HEADPHONE_CODEC_PRESENT |
-					SOF_SPEAKER_AMP_PRESENT |
-					SOF_RT1015_SPEAKER_AMP_PRESENT |
-					SOF_RT5682_SSP_AMP(1)),
-	},
-	{
-		.name = "jsl_rt5682s_rt1015p",
-		.driver_data = (kernel_ulong_t)(SOF_RT5682_MCLK_EN |
-					SOF_RT5682_MCLK_24MHZ |
-					SOF_RT5682_SSP_CODEC(0) |
-					SOF_RT5682S_HEADPHONE_CODEC_PRESENT |
-					SOF_SPEAKER_AMP_PRESENT |
-					SOF_RT1015P_SPEAKER_AMP_PRESENT |
-					SOF_RT5682_SSP_AMP(1)),
-	},
-	{
-		.name = "jsl_rt5682s_mx98360",
-		.driver_data = (kernel_ulong_t)(SOF_RT5682_MCLK_EN |
-					SOF_RT5682_MCLK_24MHZ |
-					SOF_RT5682_SSP_CODEC(0) |
-					SOF_RT5682S_HEADPHONE_CODEC_PRESENT |
-					SOF_SPEAKER_AMP_PRESENT |
-					SOF_MAX98360A_SPEAKER_AMP_PRESENT |
-					SOF_RT5682_SSP_AMP(1)),
-	},
 	{
 		.name = "adl_mx98360_rt5682",
 		.driver_data = (kernel_ulong_t)(SOF_RT5682_MCLK_EN |
diff --git a/sound/soc/intel/common/soc-acpi-intel-adl-match.c b/sound/soc/intel/common/soc-acpi-intel-adl-match.c
index f5b21a95d222..9478e35bed38 100644
--- a/sound/soc/intel/common/soc-acpi-intel-adl-match.c
+++ b/sound/soc/intel/common/soc-acpi-intel-adl-match.c
@@ -285,9 +285,15 @@ static const struct snd_soc_acpi_codecs adl_max98360a_amp = {
 	.codecs = {"MX98360A"}
 };
 
+static struct snd_soc_acpi_codecs adl_rt5682s_hp = {
+	.num_codecs = 1,
+	.codecs = {"RTL5682"}
+};
+
 struct snd_soc_acpi_mach snd_soc_acpi_intel_adl_machines[] = {
 	{
 		.id = "10EC5682",
+		.id_alt = &adl_rt5682s_hp,
 		.drv_name = "adl_mx98373_rt5682",
 		.machine_quirk = snd_soc_acpi_codec_list,
 		.quirk_data = &adl_max98373_amp,
@@ -296,6 +302,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_adl_machines[] = {
 	},
 	{
 		.id = "10EC5682",
+		.id_alt = &adl_rt5682s_hp,
 		.drv_name = "adl_mx98357_rt5682",
 		.machine_quirk = snd_soc_acpi_codec_list,
 		.quirk_data = &adl_max98357a_amp,
@@ -304,6 +311,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_adl_machines[] = {
 	},
 	{
 		.id = "10EC5682",
+		.id_alt = &adl_rt5682s_hp,
 		.drv_name = "adl_mx98360_rt5682",
 		.machine_quirk = snd_soc_acpi_codec_list,
 		.quirk_data = &adl_max98360a_amp,
diff --git a/sound/soc/intel/common/soc-acpi-intel-byt-match.c b/sound/soc/intel/common/soc-acpi-intel-byt-match.c
index 510a5f38b7f1..8c66223d7401 100644
--- a/sound/soc/intel/common/soc-acpi-intel-byt-match.c
+++ b/sound/soc/intel/common/soc-acpi-intel-byt-match.c
@@ -120,6 +120,11 @@ static struct snd_soc_acpi_mach *byt_quirk(void *arg)
 	}
 }
 
+static struct snd_soc_acpi_codecs rt5682s_hp = {
+	.num_codecs = 1,
+	.codecs = {"RTL5682"}
+};
+
 struct snd_soc_acpi_mach  snd_soc_acpi_intel_baytrail_machines[] = {
 	{
 		.id = "10EC5640",
@@ -196,6 +201,7 @@ struct snd_soc_acpi_mach  snd_soc_acpi_intel_baytrail_machines[] = {
 	},
 	{
 		.id = "10EC5682",
+		.id_alt = &rt5682s_hp,
 		.drv_name = "sof_rt5682",
 		.sof_fw_filename = "sof-byt.ri",
 		.sof_tplg_filename = "sof-byt-rt5682.tplg",
diff --git a/sound/soc/intel/common/soc-acpi-intel-cht-match.c b/sound/soc/intel/common/soc-acpi-intel-cht-match.c
index 227424236fd5..6e52110897e9 100644
--- a/sound/soc/intel/common/soc-acpi-intel-cht-match.c
+++ b/sound/soc/intel/common/soc-acpi-intel-cht-match.c
@@ -51,6 +51,11 @@ static struct snd_soc_acpi_mach *cht_quirk(void *arg)
 		return mach;
 }
 
+static struct snd_soc_acpi_codecs rt5682s_hp = {
+	.num_codecs = 1,
+	.codecs = {"RTL5682"}
+};
+
 /* Cherryview-based platforms: CherryTrail and Braswell */
 struct snd_soc_acpi_mach  snd_soc_acpi_intel_cherrytrail_machines[] = {
 	{
@@ -153,6 +158,7 @@ struct snd_soc_acpi_mach  snd_soc_acpi_intel_cherrytrail_machines[] = {
 	},
 	{
 		.id = "10EC5682",
+		.id_alt = &rt5682s_hp,
 		.drv_name = "sof_rt5682",
 		.sof_fw_filename = "sof-cht.ri",
 		.sof_tplg_filename = "sof-cht-rt5682.tplg",
diff --git a/sound/soc/intel/common/soc-acpi-intel-cml-match.c b/sound/soc/intel/common/soc-acpi-intel-cml-match.c
index b591c6fd13fd..ac93d77f47ff 100644
--- a/sound/soc/intel/common/soc-acpi-intel-cml-match.c
+++ b/sound/soc/intel/common/soc-acpi-intel-cml-match.c
@@ -29,6 +29,11 @@ static struct snd_soc_acpi_codecs max98390_spk_codecs = {
 	.codecs = {"MX98390"}
 };
 
+static struct snd_soc_acpi_codecs rt5682s_hp = {
+	.num_codecs = 1,
+	.codecs = {"RTL5682"}
+};
+
 /*
  * The order of the three entries with .id = "10EC5682" matters
  * here, because DSDT tables expose an ACPI HID for the MAX98357A
@@ -45,6 +50,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_cml_machines[] = {
 	},
 	{
 		.id = "10EC5682",
+		.id_alt = &rt5682s_hp,
 		.drv_name = "cml_rt1015_rt5682",
 		.machine_quirk = snd_soc_acpi_codec_list,
 		.quirk_data = &rt1015_spk_codecs,
@@ -53,6 +59,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_cml_machines[] = {
 	},
 	{
 		.id = "10EC5682",
+		.id_alt = &rt5682s_hp,
 		.drv_name = "sof_rt5682",
 		.machine_quirk = snd_soc_acpi_codec_list,
 		.quirk_data = &max98357a_spk_codecs,
@@ -61,6 +68,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_cml_machines[] = {
 	},
 	{
 		.id = "10EC5682",
+		.id_alt = &rt5682s_hp,
 		.drv_name = "sof_rt5682",
 		.sof_fw_filename = "sof-cml.ri",
 		.sof_tplg_filename = "sof-cml-rt5682.tplg",
diff --git a/sound/soc/intel/common/soc-acpi-intel-icl-match.c b/sound/soc/intel/common/soc-acpi-intel-icl-match.c
index 768ed538c4ea..14430b969e6c 100644
--- a/sound/soc/intel/common/soc-acpi-intel-icl-match.c
+++ b/sound/soc/intel/common/soc-acpi-intel-icl-match.c
@@ -14,6 +14,11 @@ static struct skl_machine_pdata icl_pdata = {
 	.use_tplg_pcm = true,
 };
 
+static struct snd_soc_acpi_codecs rt5682s_hp = {
+	.num_codecs = 1,
+	.codecs = {"RTL5682"}
+};
+
 struct snd_soc_acpi_mach snd_soc_acpi_intel_icl_machines[] = {
 	{
 		.id = "INT34C2",
@@ -25,6 +30,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_icl_machines[] = {
 	},
 	{
 		.id = "10EC5682",
+		.id_alt = &rt5682s_hp,
 		.drv_name = "sof_rt5682",
 		.sof_fw_filename = "sof-icl.ri",
 		.sof_tplg_filename = "sof-icl-rt5682.tplg",
diff --git a/sound/soc/intel/common/soc-acpi-intel-jsl-match.c b/sound/soc/intel/common/soc-acpi-intel-jsl-match.c
index 20fd9dcc74af..4ffd91fd6862 100644
--- a/sound/soc/intel/common/soc-acpi-intel-jsl-match.c
+++ b/sound/soc/intel/common/soc-acpi-intel-jsl-match.c
@@ -29,6 +29,11 @@ static struct snd_soc_acpi_codecs mx98360a_spk = {
 	.codecs = {"MX98360A"}
 };
 
+static struct snd_soc_acpi_codecs rt5682s_hp = {
+	.num_codecs = 1,
+	.codecs = {"RTL5682"}
+};
+
 /*
  * When adding new entry to the snd_soc_acpi_intel_jsl_machines array,
  * use .quirk_data member to distinguish different machine driver,
@@ -51,6 +56,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_jsl_machines[] = {
 	},
 	{
 		.id = "10EC5682",
+		.id_alt = &rt5682s_hp,
 		.drv_name = "jsl_rt5682_rt1015",
 		.sof_fw_filename = "sof-jsl.ri",
 		.machine_quirk = snd_soc_acpi_codec_list,
@@ -59,6 +65,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_jsl_machines[] = {
 	},
 	{
 		.id = "10EC5682",
+		.id_alt = &rt5682s_hp,
 		.drv_name = "jsl_rt5682_rt1015p",
 		.sof_fw_filename = "sof-jsl.ri",
 		.machine_quirk = snd_soc_acpi_codec_list,
@@ -67,6 +74,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_jsl_machines[] = {
 	},
 	{
 		.id = "10EC5682",
+		.id_alt = &rt5682s_hp,
 		.drv_name = "jsl_rt5682_mx98360",
 		.sof_fw_filename = "sof-jsl.ri",
 		.machine_quirk = snd_soc_acpi_codec_list,
@@ -81,30 +89,6 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_jsl_machines[] = {
 		.quirk_data = &mx98360a_spk,
 		.sof_tplg_filename = "sof-jsl-cs42l42-mx98360a.tplg",
 	},
-	{
-		.id = "RTL5682",
-		.drv_name = "jsl_rt5682s_rt1015",
-		.sof_fw_filename = "sof-jsl.ri",
-		.machine_quirk = snd_soc_acpi_codec_list,
-		.quirk_data = &rt1015_spk,
-		.sof_tplg_filename = "sof-jsl-rt5682-rt1015.tplg",
-	},
-	{
-		.id = "RTL5682",
-		.drv_name = "jsl_rt5682s_rt1015p",
-		.sof_fw_filename = "sof-jsl.ri",
-		.machine_quirk = snd_soc_acpi_codec_list,
-		.quirk_data = &rt1015p_spk,
-		.sof_tplg_filename = "sof-jsl-rt5682-rt1015.tplg",
-	},
-	{
-		.id = "RTL5682",
-		.drv_name = "jsl_rt5682s_mx98360",
-		.sof_fw_filename = "sof-jsl.ri",
-		.machine_quirk = snd_soc_acpi_codec_list,
-		.quirk_data = &mx98360a_spk,
-		.sof_tplg_filename = "sof-jsl-rt5682-mx98360a.tplg",
-	},
 	{},
 };
 EXPORT_SYMBOL_GPL(snd_soc_acpi_intel_jsl_machines);
diff --git a/sound/soc/intel/common/soc-acpi-intel-tgl-match.c b/sound/soc/intel/common/soc-acpi-intel-tgl-match.c
index 9d89f01d6b84..e65bd6db2850 100644
--- a/sound/soc/intel/common/soc-acpi-intel-tgl-match.c
+++ b/sound/soc/intel/common/soc-acpi-intel-tgl-match.c
@@ -358,9 +358,15 @@ static const struct snd_soc_acpi_codecs tgl_rt1011_amp = {
 	.codecs = {"10EC1011"}
 };
 
+static struct snd_soc_acpi_codecs tgl_rt5682s_hp = {
+	.num_codecs = 1,
+	.codecs = {"RTL5682"}
+};
+
 struct snd_soc_acpi_mach snd_soc_acpi_intel_tgl_machines[] = {
 	{
 		.id = "10EC5682",
+		.id_alt = &tgl_rt5682s_hp,
 		.drv_name = "tgl_mx98357_rt5682",
 		.machine_quirk = snd_soc_acpi_codec_list,
 		.quirk_data = &tgl_codecs,
@@ -369,6 +375,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_tgl_machines[] = {
 	},
 	{
 		.id = "10EC5682",
+		.id_alt = &tgl_rt5682s_hp,
 		.drv_name = "tgl_mx98373_rt5682",
 		.machine_quirk = snd_soc_acpi_codec_list,
 		.quirk_data = &tgl_max98373_amp,
@@ -377,6 +384,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_tgl_machines[] = {
 	},
 	{
 		.id = "10EC5682",
+		.id_alt = &tgl_rt5682s_hp,
 		.drv_name = "tgl_rt1011_rt5682",
 		.machine_quirk = snd_soc_acpi_codec_list,
 		.quirk_data = &tgl_rt1011_amp,
-- 
2.25.1

[PATCH 3/3] ASoC: Intel: sof_rt5682: use id_alt to enumerate rt5682s

[Brent Lu]

Use the id_alt field to enumerate rt5682s headphone codec and remove
redundant entries in tables.

Signed-off-by: Brent Lu <brent.lu@intel.com>
---
 sound/soc/intel/boards/sof_rt5682.c           | 30 -----------------
 .../intel/common/soc-acpi-intel-adl-match.c   |  8 +++++
 .../intel/common/soc-acpi-intel-byt-match.c   |  6 ++++
 .../intel/common/soc-acpi-intel-cht-match.c   |  6 ++++
 .../intel/common/soc-acpi-intel-cml-match.c   |  8 +++++
 .../intel/common/soc-acpi-intel-icl-match.c   |  6 ++++
 .../intel/common/soc-acpi-intel-jsl-match.c   | 32 +++++--------------
 .../intel/common/soc-acpi-intel-tgl-match.c   |  8 +++++
 8 files changed, 50 insertions(+), 54 deletions(-)

diff --git a/sound/soc/intel/boards/sof_rt5682.c b/sound/soc/intel/boards/sof_rt5682.c
index 9f1e5ef11b13..9819345cd84c 100644
--- a/sound/soc/intel/boards/sof_rt5682.c
+++ b/sound/soc/intel/boards/sof_rt5682.c
@@ -1050,36 +1050,6 @@ static const struct platform_device_id board_ids[] = {
 					SOF_RT5682_SSP_AMP(2) |
 					SOF_RT5682_NUM_HDMIDEV(4)),
 	},
-	{
-		.name = "jsl_rt5682s_rt1015",
-		.driver_data = (kernel_ulong_t)(SOF_RT5682_MCLK_EN |
-					SOF_RT5682_MCLK_24MHZ |
-					SOF_RT5682_SSP_CODEC(0) |
-					SOF_RT5682S_HEADPHONE_CODEC_PRESENT |
-					SOF_SPEAKER_AMP_PRESENT |
-					SOF_RT1015_SPEAKER_AMP_PRESENT |
-					SOF_RT5682_SSP_AMP(1)),
-	},
-	{
-		.name = "jsl_rt5682s_rt1015p",
-		.driver_data = (kernel_ulong_t)(SOF_RT5682_MCLK_EN |
-					SOF_RT5682_MCLK_24MHZ |
-					SOF_RT5682_SSP_CODEC(0) |
-					SOF_RT5682S_HEADPHONE_CODEC_PRESENT |
-					SOF_SPEAKER_AMP_PRESENT |
-					SOF_RT1015P_SPEAKER_AMP_PRESENT |
-					SOF_RT5682_SSP_AMP(1)),
-	},
-	{
-		.name = "jsl_rt5682s_mx98360",
-		.driver_data = (kernel_ulong_t)(SOF_RT5682_MCLK_EN |
-					SOF_RT5682_MCLK_24MHZ |
-					SOF_RT5682_SSP_CODEC(0) |
-					SOF_RT5682S_HEADPHONE_CODEC_PRESENT |
-					SOF_SPEAKER_AMP_PRESENT |
-					SOF_MAX98360A_SPEAKER_AMP_PRESENT |
-					SOF_RT5682_SSP_AMP(1)),
-	},
 	{
 		.name = "adl_mx98360_rt5682",
 		.driver_data = (kernel_ulong_t)(SOF_RT5682_MCLK_EN |
diff --git a/sound/soc/intel/common/soc-acpi-intel-adl-match.c b/sound/soc/intel/common/soc-acpi-intel-adl-match.c
index f5b21a95d222..9478e35bed38 100644
--- a/sound/soc/intel/common/soc-acpi-intel-adl-match.c
+++ b/sound/soc/intel/common/soc-acpi-intel-adl-match.c
@@ -285,9 +285,15 @@ static const struct snd_soc_acpi_codecs adl_max98360a_amp = {
 	.codecs = {"MX98360A"}
 };
 
+static struct snd_soc_acpi_codecs adl_rt5682s_hp = {
+	.num_codecs = 1,
+	.codecs = {"RTL5682"}
+};
+
 struct snd_soc_acpi_mach snd_soc_acpi_intel_adl_machines[] = {
 	{
 		.id = "10EC5682",
+		.id_alt = &adl_rt5682s_hp,
 		.drv_name = "adl_mx98373_rt5682",
 		.machine_quirk = snd_soc_acpi_codec_list,
 		.quirk_data = &adl_max98373_amp,
@@ -296,6 +302,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_adl_machines[] = {
 	},
 	{
 		.id = "10EC5682",
+		.id_alt = &adl_rt5682s_hp,
 		.drv_name = "adl_mx98357_rt5682",
 		.machine_quirk = snd_soc_acpi_codec_list,
 		.quirk_data = &adl_max98357a_amp,
@@ -304,6 +311,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_adl_machines[] = {
 	},
 	{
 		.id = "10EC5682",
+		.id_alt = &adl_rt5682s_hp,
 		.drv_name = "adl_mx98360_rt5682",
 		.machine_quirk = snd_soc_acpi_codec_list,
 		.quirk_data = &adl_max98360a_amp,
diff --git a/sound/soc/intel/common/soc-acpi-intel-byt-match.c b/sound/soc/intel/common/soc-acpi-intel-byt-match.c
index 510a5f38b7f1..8c66223d7401 100644
--- a/sound/soc/intel/common/soc-acpi-intel-byt-match.c
+++ b/sound/soc/intel/common/soc-acpi-intel-byt-match.c
@@ -120,6 +120,11 @@ static struct snd_soc_acpi_mach *byt_quirk(void *arg)
 	}
 }
 
+static struct snd_soc_acpi_codecs rt5682s_hp = {
+	.num_codecs = 1,
+	.codecs = {"RTL5682"}
+};
+
 struct snd_soc_acpi_mach  snd_soc_acpi_intel_baytrail_machines[] = {
 	{
 		.id = "10EC5640",
@@ -196,6 +201,7 @@ struct snd_soc_acpi_mach  snd_soc_acpi_intel_baytrail_machines[] = {
 	},
 	{
 		.id = "10EC5682",
+		.id_alt = &rt5682s_hp,
 		.drv_name = "sof_rt5682",
 		.sof_fw_filename = "sof-byt.ri",
 		.sof_tplg_filename = "sof-byt-rt5682.tplg",
diff --git a/sound/soc/intel/common/soc-acpi-intel-cht-match.c b/sound/soc/intel/common/soc-acpi-intel-cht-match.c
index 227424236fd5..6e52110897e9 100644
--- a/sound/soc/intel/common/soc-acpi-intel-cht-match.c
+++ b/sound/soc/intel/common/soc-acpi-intel-cht-match.c
@@ -51,6 +51,11 @@ static struct snd_soc_acpi_mach *cht_quirk(void *arg)
 		return mach;
 }
 
+static struct snd_soc_acpi_codecs rt5682s_hp = {
+	.num_codecs = 1,
+	.codecs = {"RTL5682"}
+};
+
 /* Cherryview-based platforms: CherryTrail and Braswell */
 struct snd_soc_acpi_mach  snd_soc_acpi_intel_cherrytrail_machines[] = {
 	{
@@ -153,6 +158,7 @@ struct snd_soc_acpi_mach  snd_soc_acpi_intel_cherrytrail_machines[] = {
 	},
 	{
 		.id = "10EC5682",
+		.id_alt = &rt5682s_hp,
 		.drv_name = "sof_rt5682",
 		.sof_fw_filename = "sof-cht.ri",
 		.sof_tplg_filename = "sof-cht-rt5682.tplg",
diff --git a/sound/soc/intel/common/soc-acpi-intel-cml-match.c b/sound/soc/intel/common/soc-acpi-intel-cml-match.c
index b591c6fd13fd..ac93d77f47ff 100644
--- a/sound/soc/intel/common/soc-acpi-intel-cml-match.c
+++ b/sound/soc/intel/common/soc-acpi-intel-cml-match.c
@@ -29,6 +29,11 @@ static struct snd_soc_acpi_codecs max98390_spk_codecs = {
 	.codecs = {"MX98390"}
 };
 
+static struct snd_soc_acpi_codecs rt5682s_hp = {
+	.num_codecs = 1,
+	.codecs = {"RTL5682"}
+};
+
 /*
  * The order of the three entries with .id = "10EC5682" matters
  * here, because DSDT tables expose an ACPI HID for the MAX98357A
@@ -45,6 +50,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_cml_machines[] = {
 	},
 	{
 		.id = "10EC5682",
+		.id_alt = &rt5682s_hp,
 		.drv_name = "cml_rt1015_rt5682",
 		.machine_quirk = snd_soc_acpi_codec_list,
 		.quirk_data = &rt1015_spk_codecs,
@@ -53,6 +59,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_cml_machines[] = {
 	},
 	{
 		.id = "10EC5682",
+		.id_alt = &rt5682s_hp,
 		.drv_name = "sof_rt5682",
 		.machine_quirk = snd_soc_acpi_codec_list,
 		.quirk_data = &max98357a_spk_codecs,
@@ -61,6 +68,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_cml_machines[] = {
 	},
 	{
 		.id = "10EC5682",
+		.id_alt = &rt5682s_hp,
 		.drv_name = "sof_rt5682",
 		.sof_fw_filename = "sof-cml.ri",
 		.sof_tplg_filename = "sof-cml-rt5682.tplg",
diff --git a/sound/soc/intel/common/soc-acpi-intel-icl-match.c b/sound/soc/intel/common/soc-acpi-intel-icl-match.c
index 768ed538c4ea..14430b969e6c 100644
--- a/sound/soc/intel/common/soc-acpi-intel-icl-match.c
+++ b/sound/soc/intel/common/soc-acpi-intel-icl-match.c
@@ -14,6 +14,11 @@ static struct skl_machine_pdata icl_pdata = {
 	.use_tplg_pcm = true,
 };
 
+static struct snd_soc_acpi_codecs rt5682s_hp = {
+	.num_codecs = 1,
+	.codecs = {"RTL5682"}
+};
+
 struct snd_soc_acpi_mach snd_soc_acpi_intel_icl_machines[] = {
 	{
 		.id = "INT34C2",
@@ -25,6 +30,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_icl_machines[] = {
 	},
 	{
 		.id = "10EC5682",
+		.id_alt = &rt5682s_hp,
 		.drv_name = "sof_rt5682",
 		.sof_fw_filename = "sof-icl.ri",
 		.sof_tplg_filename = "sof-icl-rt5682.tplg",
diff --git a/sound/soc/intel/common/soc-acpi-intel-jsl-match.c b/sound/soc/intel/common/soc-acpi-intel-jsl-match.c
index 20fd9dcc74af..4ffd91fd6862 100644
--- a/sound/soc/intel/common/soc-acpi-intel-jsl-match.c
+++ b/sound/soc/intel/common/soc-acpi-intel-jsl-match.c
@@ -29,6 +29,11 @@ static struct snd_soc_acpi_codecs mx98360a_spk = {
 	.codecs = {"MX98360A"}
 };
 
+static struct snd_soc_acpi_codecs rt5682s_hp = {
+	.num_codecs = 1,
+	.codecs = {"RTL5682"}
+};
+
 /*
  * When adding new entry to the snd_soc_acpi_intel_jsl_machines array,
  * use .quirk_data member to distinguish different machine driver,
@@ -51,6 +56,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_jsl_machines[] = {
 	},
 	{
 		.id = "10EC5682",
+		.id_alt = &rt5682s_hp,
 		.drv_name = "jsl_rt5682_rt1015",
 		.sof_fw_filename = "sof-jsl.ri",
 		.machine_quirk = snd_soc_acpi_codec_list,
@@ -59,6 +65,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_jsl_machines[] = {
 	},
 	{
 		.id = "10EC5682",
+		.id_alt = &rt5682s_hp,
 		.drv_name = "jsl_rt5682_rt1015p",
 		.sof_fw_filename = "sof-jsl.ri",
 		.machine_quirk = snd_soc_acpi_codec_list,
@@ -67,6 +74,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_jsl_machines[] = {
 	},
 	{
 		.id = "10EC5682",
+		.id_alt = &rt5682s_hp,
 		.drv_name = "jsl_rt5682_mx98360",
 		.sof_fw_filename = "sof-jsl.ri",
 		.machine_quirk = snd_soc_acpi_codec_list,
@@ -81,30 +89,6 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_jsl_machines[] = {
 		.quirk_data = &mx98360a_spk,
 		.sof_tplg_filename = "sof-jsl-cs42l42-mx98360a.tplg",
 	},
-	{
-		.id = "RTL5682",
-		.drv_name = "jsl_rt5682s_rt1015",
-		.sof_fw_filename = "sof-jsl.ri",
-		.machine_quirk = snd_soc_acpi_codec_list,
-		.quirk_data = &rt1015_spk,
-		.sof_tplg_filename = "sof-jsl-rt5682-rt1015.tplg",
-	},
-	{
-		.id = "RTL5682",
-		.drv_name = "jsl_rt5682s_rt1015p",
-		.sof_fw_filename = "sof-jsl.ri",
-		.machine_quirk = snd_soc_acpi_codec_list,
-		.quirk_data = &rt1015p_spk,
-		.sof_tplg_filename = "sof-jsl-rt5682-rt1015.tplg",
-	},
-	{
-		.id = "RTL5682",
-		.drv_name = "jsl_rt5682s_mx98360",
-		.sof_fw_filename = "sof-jsl.ri",
-		.machine_quirk = snd_soc_acpi_codec_list,
-		.quirk_data = &mx98360a_spk,
-		.sof_tplg_filename = "sof-jsl-rt5682-mx98360a.tplg",
-	},
 	{},
 };
 EXPORT_SYMBOL_GPL(snd_soc_acpi_intel_jsl_machines);
diff --git a/sound/soc/intel/common/soc-acpi-intel-tgl-match.c b/sound/soc/intel/common/soc-acpi-intel-tgl-match.c
index 9d89f01d6b84..e65bd6db2850 100644
--- a/sound/soc/intel/common/soc-acpi-intel-tgl-match.c
+++ b/sound/soc/intel/common/soc-acpi-intel-tgl-match.c
@@ -358,9 +358,15 @@ static const struct snd_soc_acpi_codecs tgl_rt1011_amp = {
 	.codecs = {"10EC1011"}
 };
 
+static struct snd_soc_acpi_codecs tgl_rt5682s_hp = {
+	.num_codecs = 1,
+	.codecs = {"RTL5682"}
+};
+
 struct snd_soc_acpi_mach snd_soc_acpi_intel_tgl_machines[] = {
 	{
 		.id = "10EC5682",
+		.id_alt = &tgl_rt5682s_hp,
 		.drv_name = "tgl_mx98357_rt5682",
 		.machine_quirk = snd_soc_acpi_codec_list,
 		.quirk_data = &tgl_codecs,
@@ -369,6 +375,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_tgl_machines[] = {
 	},
 	{
 		.id = "10EC5682",
+		.id_alt = &tgl_rt5682s_hp,
 		.drv_name = "tgl_mx98373_rt5682",
 		.machine_quirk = snd_soc_acpi_codec_list,
 		.quirk_data = &tgl_max98373_amp,
@@ -377,6 +384,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_tgl_machines[] = {
 	},
 	{
 		.id = "10EC5682",
+		.id_alt = &tgl_rt5682s_hp,
 		.drv_name = "tgl_rt1011_rt5682",
 		.machine_quirk = snd_soc_acpi_codec_list,
 		.quirk_data = &tgl_rt1011_amp,
-- 
2.25.1

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

[Andy Shevchenko]

On Wed, Oct 6, 2021 at 7:21 PM Brent Lu <brent.lu@intel.com> 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.

...

> +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;
> +}

Wondering if you may modify data structure in a way that makes it
possible to use for_each_acpi_dev_match().

...

> +               if (snd_soc_acpi_id_present(mach) != false) {

' != fase' is redundant.



-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Wed, Oct 6, 2021 at 7:21 PM Brent Lu <brent.lu@intel.com> 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.

...

> +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;
> +}

Wondering if you may modify data structure in a way that makes it
possible to use for_each_acpi_dev_match().

...

> +               if (snd_soc_acpi_id_present(mach) != false) {

' != fase' is redundant.



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3] ASoC: Intel: sof_rt5682: use id_alt to enumerate rt5682s

[Andy Shevchenko]

On Wed, Oct 6, 2021 at 7:23 PM Brent Lu <brent.lu@intel.com> wrote:
>
> Use the id_alt field to enumerate rt5682s headphone codec and remove
> redundant entries in tables.

...

> +static struct snd_soc_acpi_codecs adl_rt5682s_hp = {
> +       .num_codecs = 1,
> +       .codecs = {"RTL5682"}

Keeping commas in non-terminator entries is always good to avoid churn
in the future.

> +};

...

> +static struct snd_soc_acpi_codecs rt5682s_hp = {
> +       .num_codecs = 1,
> +       .codecs = {"RTL5682"}

Ditto.

> +};

...

> +static struct snd_soc_acpi_codecs rt5682s_hp = {
> +       .num_codecs = 1,
> +       .codecs = {"RTL5682"}

Ditto.

> +};

...

> +static struct snd_soc_acpi_codecs rt5682s_hp = {
> +       .num_codecs = 1,
> +       .codecs = {"RTL5682"}

Ditto.

> +};

...

> +static struct snd_soc_acpi_codecs rt5682s_hp = {
> +       .num_codecs = 1,
> +       .codecs = {"RTL5682"}

Ditto.

> +};

...and so on.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3] ASoC: Intel: sof_rt5682: use id_alt to enumerate rt5682s

[Andy Shevchenko]

On Wed, Oct 6, 2021 at 7:23 PM Brent Lu <brent.lu@intel.com> wrote:
>
> Use the id_alt field to enumerate rt5682s headphone codec and remove
> redundant entries in tables.

...

> +static struct snd_soc_acpi_codecs adl_rt5682s_hp = {
> +       .num_codecs = 1,
> +       .codecs = {"RTL5682"}

Keeping commas in non-terminator entries is always good to avoid churn
in the future.

> +};

...

> +static struct snd_soc_acpi_codecs rt5682s_hp = {
> +       .num_codecs = 1,
> +       .codecs = {"RTL5682"}

Ditto.

> +};

...

> +static struct snd_soc_acpi_codecs rt5682s_hp = {
> +       .num_codecs = 1,
> +       .codecs = {"RTL5682"}

Ditto.

> +};

...

> +static struct snd_soc_acpi_codecs rt5682s_hp = {
> +       .num_codecs = 1,
> +       .codecs = {"RTL5682"}

Ditto.

> +};

...

> +static struct snd_soc_acpi_codecs rt5682s_hp = {
> +       .num_codecs = 1,
> +       .codecs = {"RTL5682"}

Ditto.

> +};

...and so on.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/3] Multiple headphone codec driver support

[Pierre-Louis Bossart]

On 10/6/21 11:18 AM, Brent Lu wrote:
> Support multiple headphone drivers in same machine driver. In this
> case, both rt5682 and rt5682s are supported and enumerated by different
> ACPI HID "10EC5682" and "RTL5682".

Ah, now I get what you're trying to do. I like this a lot!

This is really a 'compatible_id' that the machine driver is capable of
handling.

It would be quite useful indeed to prune some of the tables we have, e.g.

"10EC5640" "10EC5642", "INTCCFFD"
"10EC5645" "10EC5648", etc

In your case you pushed the logic a bit further in that the codec driver
might be different, but the machine driver can deal with it with
run-time autodetection.

Would you mind going through the tables and suggest additional
simplifications with more patches when relevant?

> Brent Lu (3):
>   ASoC: soc-acpi: add alternative id field for machine driver matching
>   ASoC: Intel: sof_rt5682: detect codec variant in probe function
>   ASoC: Intel: sof_rt5682: use id_alt to enumerate rt5682s
> 
>  include/sound/soc-acpi.h                      |  2 ++
>  sound/soc/intel/boards/sof_rt5682.c           | 34 +++----------------
>  .../intel/common/soc-acpi-intel-adl-match.c   |  8 +++++
>  .../intel/common/soc-acpi-intel-byt-match.c   |  6 ++++
>  .../intel/common/soc-acpi-intel-cht-match.c   |  6 ++++
>  .../intel/common/soc-acpi-intel-cml-match.c   |  8 +++++
>  .../intel/common/soc-acpi-intel-icl-match.c   |  6 ++++
>  .../intel/common/soc-acpi-intel-jsl-match.c   | 32 +++++------------
>  .../intel/common/soc-acpi-intel-tgl-match.c   |  8 +++++
>  sound/soc/soc-acpi.c                          | 21 +++++++++++-
>  10 files changed, 76 insertions(+), 55 deletions(-)
> 

Re: [PATCH 0/3] Multiple headphone codec driver support

[Pierre-Louis Bossart]

On 10/6/21 11:18 AM, Brent Lu wrote:
> Support multiple headphone drivers in same machine driver. In this
> case, both rt5682 and rt5682s are supported and enumerated by different
> ACPI HID "10EC5682" and "RTL5682".

Ah, now I get what you're trying to do. I like this a lot!

This is really a 'compatible_id' that the machine driver is capable of
handling.

It would be quite useful indeed to prune some of the tables we have, e.g.

"10EC5640" "10EC5642", "INTCCFFD"
"10EC5645" "10EC5648", etc

In your case you pushed the logic a bit further in that the codec driver
might be different, but the machine driver can deal with it with
run-time autodetection.

Would you mind going through the tables and suggest additional
simplifications with more patches when relevant?

> Brent Lu (3):
>   ASoC: soc-acpi: add alternative id field for machine driver matching
>   ASoC: Intel: sof_rt5682: detect codec variant in probe function
>   ASoC: Intel: sof_rt5682: use id_alt to enumerate rt5682s
> 
>  include/sound/soc-acpi.h                      |  2 ++
>  sound/soc/intel/boards/sof_rt5682.c           | 34 +++----------------
>  .../intel/common/soc-acpi-intel-adl-match.c   |  8 +++++
>  .../intel/common/soc-acpi-intel-byt-match.c   |  6 ++++
>  .../intel/common/soc-acpi-intel-cht-match.c   |  6 ++++
>  .../intel/common/soc-acpi-intel-cml-match.c   |  8 +++++
>  .../intel/common/soc-acpi-intel-icl-match.c   |  6 ++++
>  .../intel/common/soc-acpi-intel-jsl-match.c   | 32 +++++------------
>  .../intel/common/soc-acpi-intel-tgl-match.c   |  8 +++++
>  sound/soc/soc-acpi.c                          | 21 +++++++++++-
>  10 files changed, 76 insertions(+), 55 deletions(-)
> 

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

[Pierre-Louis Bossart]

> +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)

if (!id_alt)

> +		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;
> +}

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

[Pierre-Louis Bossart]

> +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)

if (!id_alt)

> +		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;
> +}

Re: [PATCH 3/3] ASoC: Intel: sof_rt5682: use id_alt to enumerate rt5682s

[Hans de Goede]

Hi,

On 10/6/21 6:18 PM, Brent Lu wrote:
> Use the id_alt field to enumerate rt5682s headphone codec and remove
> redundant entries in tables.
> 
> Signed-off-by: Brent Lu <brent.lu@intel.com>
> ---
>  sound/soc/intel/boards/sof_rt5682.c           | 30 -----------------
>  .../intel/common/soc-acpi-intel-adl-match.c   |  8 +++++
>  .../intel/common/soc-acpi-intel-byt-match.c   |  6 ++++
>  .../intel/common/soc-acpi-intel-cht-match.c   |  6 ++++
>  .../intel/common/soc-acpi-intel-cml-match.c   |  8 +++++
>  .../intel/common/soc-acpi-intel-icl-match.c   |  6 ++++
>  .../intel/common/soc-acpi-intel-jsl-match.c   | 32 +++++--------------
>  .../intel/common/soc-acpi-intel-tgl-match.c   |  8 +++++
>  8 files changed, 50 insertions(+), 54 deletions(-)
> 
> diff --git a/sound/soc/intel/boards/sof_rt5682.c b/sound/soc/intel/boards/sof_rt5682.c
> index 9f1e5ef11b13..9819345cd84c 100644
> --- a/sound/soc/intel/boards/sof_rt5682.c
> +++ b/sound/soc/intel/boards/sof_rt5682.c
> @@ -1050,36 +1050,6 @@ static const struct platform_device_id board_ids[] = {
>  					SOF_RT5682_SSP_AMP(2) |
>  					SOF_RT5682_NUM_HDMIDEV(4)),
>  	},
> -	{
> -		.name = "jsl_rt5682s_rt1015",
> -		.driver_data = (kernel_ulong_t)(SOF_RT5682_MCLK_EN |
> -					SOF_RT5682_MCLK_24MHZ |
> -					SOF_RT5682_SSP_CODEC(0) |
> -					SOF_RT5682S_HEADPHONE_CODEC_PRESENT |
> -					SOF_SPEAKER_AMP_PRESENT |
> -					SOF_RT1015_SPEAKER_AMP_PRESENT |
> -					SOF_RT5682_SSP_AMP(1)),
> -	},
> -	{
> -		.name = "jsl_rt5682s_rt1015p",
> -		.driver_data = (kernel_ulong_t)(SOF_RT5682_MCLK_EN |
> -					SOF_RT5682_MCLK_24MHZ |
> -					SOF_RT5682_SSP_CODEC(0) |
> -					SOF_RT5682S_HEADPHONE_CODEC_PRESENT |
> -					SOF_SPEAKER_AMP_PRESENT |
> -					SOF_RT1015P_SPEAKER_AMP_PRESENT |
> -					SOF_RT5682_SSP_AMP(1)),
> -	},
> -	{
> -		.name = "jsl_rt5682s_mx98360",
> -		.driver_data = (kernel_ulong_t)(SOF_RT5682_MCLK_EN |
> -					SOF_RT5682_MCLK_24MHZ |
> -					SOF_RT5682_SSP_CODEC(0) |
> -					SOF_RT5682S_HEADPHONE_CODEC_PRESENT |
> -					SOF_SPEAKER_AMP_PRESENT |
> -					SOF_MAX98360A_SPEAKER_AMP_PRESENT |
> -					SOF_RT5682_SSP_AMP(1)),
> -	},
>  	{
>  		.name = "adl_mx98360_rt5682",
>  		.driver_data = (kernel_ulong_t)(SOF_RT5682_MCLK_EN |
> diff --git a/sound/soc/intel/common/soc-acpi-intel-adl-match.c b/sound/soc/intel/common/soc-acpi-intel-adl-match.c
> index f5b21a95d222..9478e35bed38 100644
> --- a/sound/soc/intel/common/soc-acpi-intel-adl-match.c
> +++ b/sound/soc/intel/common/soc-acpi-intel-adl-match.c
> @@ -285,9 +285,15 @@ static const struct snd_soc_acpi_codecs adl_max98360a_amp = {
>  	.codecs = {"MX98360A"}
>  };
>  
> +static struct snd_soc_acpi_codecs adl_rt5682s_hp = {
> +	.num_codecs = 1,
> +	.codecs = {"RTL5682"}
> +};
> +
>  struct snd_soc_acpi_mach snd_soc_acpi_intel_adl_machines[] = {
>  	{
>  		.id = "10EC5682",
> +		.id_alt = &adl_rt5682s_hp,
>  		.drv_name = "adl_mx98373_rt5682",
>  		.machine_quirk = snd_soc_acpi_codec_list,
>  		.quirk_data = &adl_max98373_amp,
> @@ -296,6 +302,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_adl_machines[] = {
>  	},
>  	{
>  		.id = "10EC5682",
> +		.id_alt = &adl_rt5682s_hp,
>  		.drv_name = "adl_mx98357_rt5682",
>  		.machine_quirk = snd_soc_acpi_codec_list,
>  		.quirk_data = &adl_max98357a_amp,
> @@ -304,6 +311,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_adl_machines[] = {
>  	},
>  	{
>  		.id = "10EC5682",
> +		.id_alt = &adl_rt5682s_hp,
>  		.drv_name = "adl_mx98360_rt5682",
>  		.machine_quirk = snd_soc_acpi_codec_list,
>  		.quirk_data = &adl_max98360a_amp,
> diff --git a/sound/soc/intel/common/soc-acpi-intel-byt-match.c b/sound/soc/intel/common/soc-acpi-intel-byt-match.c
> index 510a5f38b7f1..8c66223d7401 100644
> --- a/sound/soc/intel/common/soc-acpi-intel-byt-match.c
> +++ b/sound/soc/intel/common/soc-acpi-intel-byt-match.c
> @@ -120,6 +120,11 @@ static struct snd_soc_acpi_mach *byt_quirk(void *arg)
>  	}
>  }
>  
> +static struct snd_soc_acpi_codecs rt5682s_hp = {
> +	.num_codecs = 1,
> +	.codecs = {"RTL5682"}
> +};
> +
>  struct snd_soc_acpi_mach  snd_soc_acpi_intel_baytrail_machines[] = {
>  	{
>  		.id = "10EC5640",
> @@ -196,6 +201,7 @@ struct snd_soc_acpi_mach  snd_soc_acpi_intel_baytrail_machines[] = {
>  	},
>  	{
>  		.id = "10EC5682",
> +		.id_alt = &rt5682s_hp,
>  		.drv_name = "sof_rt5682",
>  		.sof_fw_filename = "sof-byt.ri",
>  		.sof_tplg_filename = "sof-byt-rt5682.tplg",

So this is only useful if there actually are any BYT devices using the "RTL5682"
ACPI HID, the 100+ BYT/CHT DSDTs which I've gather over time say there aren't any.

Actually there also aren't any using the non alt "10EC5682" ACPI HID either...

Bard Liao, you added this in commit f70abd75b7c6 ("ASoC: Intel: add sof-rt5682 machine driver")
but I wonder how useful this is. I guess it may be available as (and tested on?) some dev-kit.

But I don't think there us any hardware out there in the wild using this ?





> diff --git a/sound/soc/intel/common/soc-acpi-intel-cht-match.c b/sound/soc/intel/common/soc-acpi-intel-cht-match.c
> index 227424236fd5..6e52110897e9 100644
> --- a/sound/soc/intel/common/soc-acpi-intel-cht-match.c
> +++ b/sound/soc/intel/common/soc-acpi-intel-cht-match.c
> @@ -51,6 +51,11 @@ static struct snd_soc_acpi_mach *cht_quirk(void *arg)
>  		return mach;
>  }
>  
> +static struct snd_soc_acpi_codecs rt5682s_hp = {
> +	.num_codecs = 1,
> +	.codecs = {"RTL5682"}
> +};
> +
>  /* Cherryview-based platforms: CherryTrail and Braswell */
>  struct snd_soc_acpi_mach  snd_soc_acpi_intel_cherrytrail_machines[] = {
>  	{
> @@ -153,6 +158,7 @@ struct snd_soc_acpi_mach  snd_soc_acpi_intel_cherrytrail_machines[] = {
>  	},
>  	{
>  		.id = "10EC5682",
> +		.id_alt = &rt5682s_hp,
>  		.drv_name = "sof_rt5682",
>  		.sof_fw_filename = "sof-cht.ri",
>  		.sof_tplg_filename = "sof-cht-rt5682.tplg",

Same remark as for the BYT changes.

Regards,

Hans



> diff --git a/sound/soc/intel/common/soc-acpi-intel-cml-match.c b/sound/soc/intel/common/soc-acpi-intel-cml-match.c
> index b591c6fd13fd..ac93d77f47ff 100644
> --- a/sound/soc/intel/common/soc-acpi-intel-cml-match.c
> +++ b/sound/soc/intel/common/soc-acpi-intel-cml-match.c
> @@ -29,6 +29,11 @@ static struct snd_soc_acpi_codecs max98390_spk_codecs = {
>  	.codecs = {"MX98390"}
>  };
>  
> +static struct snd_soc_acpi_codecs rt5682s_hp = {
> +	.num_codecs = 1,
> +	.codecs = {"RTL5682"}
> +};
> +
>  /*
>   * The order of the three entries with .id = "10EC5682" matters
>   * here, because DSDT tables expose an ACPI HID for the MAX98357A
> @@ -45,6 +50,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_cml_machines[] = {
>  	},
>  	{
>  		.id = "10EC5682",
> +		.id_alt = &rt5682s_hp,
>  		.drv_name = "cml_rt1015_rt5682",
>  		.machine_quirk = snd_soc_acpi_codec_list,
>  		.quirk_data = &rt1015_spk_codecs,
> @@ -53,6 +59,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_cml_machines[] = {
>  	},
>  	{
>  		.id = "10EC5682",
> +		.id_alt = &rt5682s_hp,
>  		.drv_name = "sof_rt5682",
>  		.machine_quirk = snd_soc_acpi_codec_list,
>  		.quirk_data = &max98357a_spk_codecs,
> @@ -61,6 +68,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_cml_machines[] = {
>  	},
>  	{
>  		.id = "10EC5682",
> +		.id_alt = &rt5682s_hp,
>  		.drv_name = "sof_rt5682",
>  		.sof_fw_filename = "sof-cml.ri",
>  		.sof_tplg_filename = "sof-cml-rt5682.tplg",
> diff --git a/sound/soc/intel/common/soc-acpi-intel-icl-match.c b/sound/soc/intel/common/soc-acpi-intel-icl-match.c
> index 768ed538c4ea..14430b969e6c 100644
> --- a/sound/soc/intel/common/soc-acpi-intel-icl-match.c
> +++ b/sound/soc/intel/common/soc-acpi-intel-icl-match.c
> @@ -14,6 +14,11 @@ static struct skl_machine_pdata icl_pdata = {
>  	.use_tplg_pcm = true,
>  };
>  
> +static struct snd_soc_acpi_codecs rt5682s_hp = {
> +	.num_codecs = 1,
> +	.codecs = {"RTL5682"}
> +};
> +
>  struct snd_soc_acpi_mach snd_soc_acpi_intel_icl_machines[] = {
>  	{
>  		.id = "INT34C2",
> @@ -25,6 +30,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_icl_machines[] = {
>  	},
>  	{
>  		.id = "10EC5682",
> +		.id_alt = &rt5682s_hp,
>  		.drv_name = "sof_rt5682",
>  		.sof_fw_filename = "sof-icl.ri",
>  		.sof_tplg_filename = "sof-icl-rt5682.tplg",
> diff --git a/sound/soc/intel/common/soc-acpi-intel-jsl-match.c b/sound/soc/intel/common/soc-acpi-intel-jsl-match.c
> index 20fd9dcc74af..4ffd91fd6862 100644
> --- a/sound/soc/intel/common/soc-acpi-intel-jsl-match.c
> +++ b/sound/soc/intel/common/soc-acpi-intel-jsl-match.c
> @@ -29,6 +29,11 @@ static struct snd_soc_acpi_codecs mx98360a_spk = {
>  	.codecs = {"MX98360A"}
>  };
>  
> +static struct snd_soc_acpi_codecs rt5682s_hp = {
> +	.num_codecs = 1,
> +	.codecs = {"RTL5682"}
> +};
> +
>  /*
>   * When adding new entry to the snd_soc_acpi_intel_jsl_machines array,
>   * use .quirk_data member to distinguish different machine driver,
> @@ -51,6 +56,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_jsl_machines[] = {
>  	},
>  	{
>  		.id = "10EC5682",
> +		.id_alt = &rt5682s_hp,
>  		.drv_name = "jsl_rt5682_rt1015",
>  		.sof_fw_filename = "sof-jsl.ri",
>  		.machine_quirk = snd_soc_acpi_codec_list,
> @@ -59,6 +65,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_jsl_machines[] = {
>  	},
>  	{
>  		.id = "10EC5682",
> +		.id_alt = &rt5682s_hp,
>  		.drv_name = "jsl_rt5682_rt1015p",
>  		.sof_fw_filename = "sof-jsl.ri",
>  		.machine_quirk = snd_soc_acpi_codec_list,
> @@ -67,6 +74,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_jsl_machines[] = {
>  	},
>  	{
>  		.id = "10EC5682",
> +		.id_alt = &rt5682s_hp,
>  		.drv_name = "jsl_rt5682_mx98360",
>  		.sof_fw_filename = "sof-jsl.ri",
>  		.machine_quirk = snd_soc_acpi_codec_list,
> @@ -81,30 +89,6 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_jsl_machines[] = {
>  		.quirk_data = &mx98360a_spk,
>  		.sof_tplg_filename = "sof-jsl-cs42l42-mx98360a.tplg",
>  	},
> -	{
> -		.id = "RTL5682",
> -		.drv_name = "jsl_rt5682s_rt1015",
> -		.sof_fw_filename = "sof-jsl.ri",
> -		.machine_quirk = snd_soc_acpi_codec_list,
> -		.quirk_data = &rt1015_spk,
> -		.sof_tplg_filename = "sof-jsl-rt5682-rt1015.tplg",
> -	},
> -	{
> -		.id = "RTL5682",
> -		.drv_name = "jsl_rt5682s_rt1015p",
> -		.sof_fw_filename = "sof-jsl.ri",
> -		.machine_quirk = snd_soc_acpi_codec_list,
> -		.quirk_data = &rt1015p_spk,
> -		.sof_tplg_filename = "sof-jsl-rt5682-rt1015.tplg",
> -	},
> -	{
> -		.id = "RTL5682",
> -		.drv_name = "jsl_rt5682s_mx98360",
> -		.sof_fw_filename = "sof-jsl.ri",
> -		.machine_quirk = snd_soc_acpi_codec_list,
> -		.quirk_data = &mx98360a_spk,
> -		.sof_tplg_filename = "sof-jsl-rt5682-mx98360a.tplg",
> -	},
>  	{},
>  };
>  EXPORT_SYMBOL_GPL(snd_soc_acpi_intel_jsl_machines);
> diff --git a/sound/soc/intel/common/soc-acpi-intel-tgl-match.c b/sound/soc/intel/common/soc-acpi-intel-tgl-match.c
> index 9d89f01d6b84..e65bd6db2850 100644
> --- a/sound/soc/intel/common/soc-acpi-intel-tgl-match.c
> +++ b/sound/soc/intel/common/soc-acpi-intel-tgl-match.c
> @@ -358,9 +358,15 @@ static const struct snd_soc_acpi_codecs tgl_rt1011_amp = {
>  	.codecs = {"10EC1011"}
>  };
>  
> +static struct snd_soc_acpi_codecs tgl_rt5682s_hp = {
> +	.num_codecs = 1,
> +	.codecs = {"RTL5682"}
> +};
> +
>  struct snd_soc_acpi_mach snd_soc_acpi_intel_tgl_machines[] = {
>  	{
>  		.id = "10EC5682",
> +		.id_alt = &tgl_rt5682s_hp,
>  		.drv_name = "tgl_mx98357_rt5682",
>  		.machine_quirk = snd_soc_acpi_codec_list,
>  		.quirk_data = &tgl_codecs,
> @@ -369,6 +375,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_tgl_machines[] = {
>  	},
>  	{
>  		.id = "10EC5682",
> +		.id_alt = &tgl_rt5682s_hp,
>  		.drv_name = "tgl_mx98373_rt5682",
>  		.machine_quirk = snd_soc_acpi_codec_list,
>  		.quirk_data = &tgl_max98373_amp,
> @@ -377,6 +384,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_tgl_machines[] = {
>  	},
>  	{
>  		.id = "10EC5682",
> +		.id_alt = &tgl_rt5682s_hp,
>  		.drv_name = "tgl_rt1011_rt5682",
>  		.machine_quirk = snd_soc_acpi_codec_list,
>  		.quirk_data = &tgl_rt1011_amp,
> 

Re: [PATCH 3/3] ASoC: Intel: sof_rt5682: use id_alt to enumerate rt5682s

[Hans de Goede]

Hi,

On 10/6/21 6:18 PM, Brent Lu wrote:
> Use the id_alt field to enumerate rt5682s headphone codec and remove
> redundant entries in tables.
> 
> Signed-off-by: Brent Lu <brent.lu@intel.com>
> ---
>  sound/soc/intel/boards/sof_rt5682.c           | 30 -----------------
>  .../intel/common/soc-acpi-intel-adl-match.c   |  8 +++++
>  .../intel/common/soc-acpi-intel-byt-match.c   |  6 ++++
>  .../intel/common/soc-acpi-intel-cht-match.c   |  6 ++++
>  .../intel/common/soc-acpi-intel-cml-match.c   |  8 +++++
>  .../intel/common/soc-acpi-intel-icl-match.c   |  6 ++++
>  .../intel/common/soc-acpi-intel-jsl-match.c   | 32 +++++--------------
>  .../intel/common/soc-acpi-intel-tgl-match.c   |  8 +++++
>  8 files changed, 50 insertions(+), 54 deletions(-)
> 
> diff --git a/sound/soc/intel/boards/sof_rt5682.c b/sound/soc/intel/boards/sof_rt5682.c
> index 9f1e5ef11b13..9819345cd84c 100644
> --- a/sound/soc/intel/boards/sof_rt5682.c
> +++ b/sound/soc/intel/boards/sof_rt5682.c
> @@ -1050,36 +1050,6 @@ static const struct platform_device_id board_ids[] = {
>  					SOF_RT5682_SSP_AMP(2) |
>  					SOF_RT5682_NUM_HDMIDEV(4)),
>  	},
> -	{
> -		.name = "jsl_rt5682s_rt1015",
> -		.driver_data = (kernel_ulong_t)(SOF_RT5682_MCLK_EN |
> -					SOF_RT5682_MCLK_24MHZ |
> -					SOF_RT5682_SSP_CODEC(0) |
> -					SOF_RT5682S_HEADPHONE_CODEC_PRESENT |
> -					SOF_SPEAKER_AMP_PRESENT |
> -					SOF_RT1015_SPEAKER_AMP_PRESENT |
> -					SOF_RT5682_SSP_AMP(1)),
> -	},
> -	{
> -		.name = "jsl_rt5682s_rt1015p",
> -		.driver_data = (kernel_ulong_t)(SOF_RT5682_MCLK_EN |
> -					SOF_RT5682_MCLK_24MHZ |
> -					SOF_RT5682_SSP_CODEC(0) |
> -					SOF_RT5682S_HEADPHONE_CODEC_PRESENT |
> -					SOF_SPEAKER_AMP_PRESENT |
> -					SOF_RT1015P_SPEAKER_AMP_PRESENT |
> -					SOF_RT5682_SSP_AMP(1)),
> -	},
> -	{
> -		.name = "jsl_rt5682s_mx98360",
> -		.driver_data = (kernel_ulong_t)(SOF_RT5682_MCLK_EN |
> -					SOF_RT5682_MCLK_24MHZ |
> -					SOF_RT5682_SSP_CODEC(0) |
> -					SOF_RT5682S_HEADPHONE_CODEC_PRESENT |
> -					SOF_SPEAKER_AMP_PRESENT |
> -					SOF_MAX98360A_SPEAKER_AMP_PRESENT |
> -					SOF_RT5682_SSP_AMP(1)),
> -	},
>  	{
>  		.name = "adl_mx98360_rt5682",
>  		.driver_data = (kernel_ulong_t)(SOF_RT5682_MCLK_EN |
> diff --git a/sound/soc/intel/common/soc-acpi-intel-adl-match.c b/sound/soc/intel/common/soc-acpi-intel-adl-match.c
> index f5b21a95d222..9478e35bed38 100644
> --- a/sound/soc/intel/common/soc-acpi-intel-adl-match.c
> +++ b/sound/soc/intel/common/soc-acpi-intel-adl-match.c
> @@ -285,9 +285,15 @@ static const struct snd_soc_acpi_codecs adl_max98360a_amp = {
>  	.codecs = {"MX98360A"}
>  };
>  
> +static struct snd_soc_acpi_codecs adl_rt5682s_hp = {
> +	.num_codecs = 1,
> +	.codecs = {"RTL5682"}
> +};
> +
>  struct snd_soc_acpi_mach snd_soc_acpi_intel_adl_machines[] = {
>  	{
>  		.id = "10EC5682",
> +		.id_alt = &adl_rt5682s_hp,
>  		.drv_name = "adl_mx98373_rt5682",
>  		.machine_quirk = snd_soc_acpi_codec_list,
>  		.quirk_data = &adl_max98373_amp,
> @@ -296,6 +302,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_adl_machines[] = {
>  	},
>  	{
>  		.id = "10EC5682",
> +		.id_alt = &adl_rt5682s_hp,
>  		.drv_name = "adl_mx98357_rt5682",
>  		.machine_quirk = snd_soc_acpi_codec_list,
>  		.quirk_data = &adl_max98357a_amp,
> @@ -304,6 +311,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_adl_machines[] = {
>  	},
>  	{
>  		.id = "10EC5682",
> +		.id_alt = &adl_rt5682s_hp,
>  		.drv_name = "adl_mx98360_rt5682",
>  		.machine_quirk = snd_soc_acpi_codec_list,
>  		.quirk_data = &adl_max98360a_amp,
> diff --git a/sound/soc/intel/common/soc-acpi-intel-byt-match.c b/sound/soc/intel/common/soc-acpi-intel-byt-match.c
> index 510a5f38b7f1..8c66223d7401 100644
> --- a/sound/soc/intel/common/soc-acpi-intel-byt-match.c
> +++ b/sound/soc/intel/common/soc-acpi-intel-byt-match.c
> @@ -120,6 +120,11 @@ static struct snd_soc_acpi_mach *byt_quirk(void *arg)
>  	}
>  }
>  
> +static struct snd_soc_acpi_codecs rt5682s_hp = {
> +	.num_codecs = 1,
> +	.codecs = {"RTL5682"}
> +};
> +
>  struct snd_soc_acpi_mach  snd_soc_acpi_intel_baytrail_machines[] = {
>  	{
>  		.id = "10EC5640",
> @@ -196,6 +201,7 @@ struct snd_soc_acpi_mach  snd_soc_acpi_intel_baytrail_machines[] = {
>  	},
>  	{
>  		.id = "10EC5682",
> +		.id_alt = &rt5682s_hp,
>  		.drv_name = "sof_rt5682",
>  		.sof_fw_filename = "sof-byt.ri",
>  		.sof_tplg_filename = "sof-byt-rt5682.tplg",

So this is only useful if there actually are any BYT devices using the "RTL5682"
ACPI HID, the 100+ BYT/CHT DSDTs which I've gather over time say there aren't any.

Actually there also aren't any using the non alt "10EC5682" ACPI HID either...

Bard Liao, you added this in commit f70abd75b7c6 ("ASoC: Intel: add sof-rt5682 machine driver")
but I wonder how useful this is. I guess it may be available as (and tested on?) some dev-kit.

But I don't think there us any hardware out there in the wild using this ?





> diff --git a/sound/soc/intel/common/soc-acpi-intel-cht-match.c b/sound/soc/intel/common/soc-acpi-intel-cht-match.c
> index 227424236fd5..6e52110897e9 100644
> --- a/sound/soc/intel/common/soc-acpi-intel-cht-match.c
> +++ b/sound/soc/intel/common/soc-acpi-intel-cht-match.c
> @@ -51,6 +51,11 @@ static struct snd_soc_acpi_mach *cht_quirk(void *arg)
>  		return mach;
>  }
>  
> +static struct snd_soc_acpi_codecs rt5682s_hp = {
> +	.num_codecs = 1,
> +	.codecs = {"RTL5682"}
> +};
> +
>  /* Cherryview-based platforms: CherryTrail and Braswell */
>  struct snd_soc_acpi_mach  snd_soc_acpi_intel_cherrytrail_machines[] = {
>  	{
> @@ -153,6 +158,7 @@ struct snd_soc_acpi_mach  snd_soc_acpi_intel_cherrytrail_machines[] = {
>  	},
>  	{
>  		.id = "10EC5682",
> +		.id_alt = &rt5682s_hp,
>  		.drv_name = "sof_rt5682",
>  		.sof_fw_filename = "sof-cht.ri",
>  		.sof_tplg_filename = "sof-cht-rt5682.tplg",

Same remark as for the BYT changes.

Regards,

Hans



> diff --git a/sound/soc/intel/common/soc-acpi-intel-cml-match.c b/sound/soc/intel/common/soc-acpi-intel-cml-match.c
> index b591c6fd13fd..ac93d77f47ff 100644
> --- a/sound/soc/intel/common/soc-acpi-intel-cml-match.c
> +++ b/sound/soc/intel/common/soc-acpi-intel-cml-match.c
> @@ -29,6 +29,11 @@ static struct snd_soc_acpi_codecs max98390_spk_codecs = {
>  	.codecs = {"MX98390"}
>  };
>  
> +static struct snd_soc_acpi_codecs rt5682s_hp = {
> +	.num_codecs = 1,
> +	.codecs = {"RTL5682"}
> +};
> +
>  /*
>   * The order of the three entries with .id = "10EC5682" matters
>   * here, because DSDT tables expose an ACPI HID for the MAX98357A
> @@ -45,6 +50,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_cml_machines[] = {
>  	},
>  	{
>  		.id = "10EC5682",
> +		.id_alt = &rt5682s_hp,
>  		.drv_name = "cml_rt1015_rt5682",
>  		.machine_quirk = snd_soc_acpi_codec_list,
>  		.quirk_data = &rt1015_spk_codecs,
> @@ -53,6 +59,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_cml_machines[] = {
>  	},
>  	{
>  		.id = "10EC5682",
> +		.id_alt = &rt5682s_hp,
>  		.drv_name = "sof_rt5682",
>  		.machine_quirk = snd_soc_acpi_codec_list,
>  		.quirk_data = &max98357a_spk_codecs,
> @@ -61,6 +68,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_cml_machines[] = {
>  	},
>  	{
>  		.id = "10EC5682",
> +		.id_alt = &rt5682s_hp,
>  		.drv_name = "sof_rt5682",
>  		.sof_fw_filename = "sof-cml.ri",
>  		.sof_tplg_filename = "sof-cml-rt5682.tplg",
> diff --git a/sound/soc/intel/common/soc-acpi-intel-icl-match.c b/sound/soc/intel/common/soc-acpi-intel-icl-match.c
> index 768ed538c4ea..14430b969e6c 100644
> --- a/sound/soc/intel/common/soc-acpi-intel-icl-match.c
> +++ b/sound/soc/intel/common/soc-acpi-intel-icl-match.c
> @@ -14,6 +14,11 @@ static struct skl_machine_pdata icl_pdata = {
>  	.use_tplg_pcm = true,
>  };
>  
> +static struct snd_soc_acpi_codecs rt5682s_hp = {
> +	.num_codecs = 1,
> +	.codecs = {"RTL5682"}
> +};
> +
>  struct snd_soc_acpi_mach snd_soc_acpi_intel_icl_machines[] = {
>  	{
>  		.id = "INT34C2",
> @@ -25,6 +30,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_icl_machines[] = {
>  	},
>  	{
>  		.id = "10EC5682",
> +		.id_alt = &rt5682s_hp,
>  		.drv_name = "sof_rt5682",
>  		.sof_fw_filename = "sof-icl.ri",
>  		.sof_tplg_filename = "sof-icl-rt5682.tplg",
> diff --git a/sound/soc/intel/common/soc-acpi-intel-jsl-match.c b/sound/soc/intel/common/soc-acpi-intel-jsl-match.c
> index 20fd9dcc74af..4ffd91fd6862 100644
> --- a/sound/soc/intel/common/soc-acpi-intel-jsl-match.c
> +++ b/sound/soc/intel/common/soc-acpi-intel-jsl-match.c
> @@ -29,6 +29,11 @@ static struct snd_soc_acpi_codecs mx98360a_spk = {
>  	.codecs = {"MX98360A"}
>  };
>  
> +static struct snd_soc_acpi_codecs rt5682s_hp = {
> +	.num_codecs = 1,
> +	.codecs = {"RTL5682"}
> +};
> +
>  /*
>   * When adding new entry to the snd_soc_acpi_intel_jsl_machines array,
>   * use .quirk_data member to distinguish different machine driver,
> @@ -51,6 +56,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_jsl_machines[] = {
>  	},
>  	{
>  		.id = "10EC5682",
> +		.id_alt = &rt5682s_hp,
>  		.drv_name = "jsl_rt5682_rt1015",
>  		.sof_fw_filename = "sof-jsl.ri",
>  		.machine_quirk = snd_soc_acpi_codec_list,
> @@ -59,6 +65,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_jsl_machines[] = {
>  	},
>  	{
>  		.id = "10EC5682",
> +		.id_alt = &rt5682s_hp,
>  		.drv_name = "jsl_rt5682_rt1015p",
>  		.sof_fw_filename = "sof-jsl.ri",
>  		.machine_quirk = snd_soc_acpi_codec_list,
> @@ -67,6 +74,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_jsl_machines[] = {
>  	},
>  	{
>  		.id = "10EC5682",
> +		.id_alt = &rt5682s_hp,
>  		.drv_name = "jsl_rt5682_mx98360",
>  		.sof_fw_filename = "sof-jsl.ri",
>  		.machine_quirk = snd_soc_acpi_codec_list,
> @@ -81,30 +89,6 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_jsl_machines[] = {
>  		.quirk_data = &mx98360a_spk,
>  		.sof_tplg_filename = "sof-jsl-cs42l42-mx98360a.tplg",
>  	},
> -	{
> -		.id = "RTL5682",
> -		.drv_name = "jsl_rt5682s_rt1015",
> -		.sof_fw_filename = "sof-jsl.ri",
> -		.machine_quirk = snd_soc_acpi_codec_list,
> -		.quirk_data = &rt1015_spk,
> -		.sof_tplg_filename = "sof-jsl-rt5682-rt1015.tplg",
> -	},
> -	{
> -		.id = "RTL5682",
> -		.drv_name = "jsl_rt5682s_rt1015p",
> -		.sof_fw_filename = "sof-jsl.ri",
> -		.machine_quirk = snd_soc_acpi_codec_list,
> -		.quirk_data = &rt1015p_spk,
> -		.sof_tplg_filename = "sof-jsl-rt5682-rt1015.tplg",
> -	},
> -	{
> -		.id = "RTL5682",
> -		.drv_name = "jsl_rt5682s_mx98360",
> -		.sof_fw_filename = "sof-jsl.ri",
> -		.machine_quirk = snd_soc_acpi_codec_list,
> -		.quirk_data = &mx98360a_spk,
> -		.sof_tplg_filename = "sof-jsl-rt5682-mx98360a.tplg",
> -	},
>  	{},
>  };
>  EXPORT_SYMBOL_GPL(snd_soc_acpi_intel_jsl_machines);
> diff --git a/sound/soc/intel/common/soc-acpi-intel-tgl-match.c b/sound/soc/intel/common/soc-acpi-intel-tgl-match.c
> index 9d89f01d6b84..e65bd6db2850 100644
> --- a/sound/soc/intel/common/soc-acpi-intel-tgl-match.c
> +++ b/sound/soc/intel/common/soc-acpi-intel-tgl-match.c
> @@ -358,9 +358,15 @@ static const struct snd_soc_acpi_codecs tgl_rt1011_amp = {
>  	.codecs = {"10EC1011"}
>  };
>  
> +static struct snd_soc_acpi_codecs tgl_rt5682s_hp = {
> +	.num_codecs = 1,
> +	.codecs = {"RTL5682"}
> +};
> +
>  struct snd_soc_acpi_mach snd_soc_acpi_intel_tgl_machines[] = {
>  	{
>  		.id = "10EC5682",
> +		.id_alt = &tgl_rt5682s_hp,
>  		.drv_name = "tgl_mx98357_rt5682",
>  		.machine_quirk = snd_soc_acpi_codec_list,
>  		.quirk_data = &tgl_codecs,
> @@ -369,6 +375,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_tgl_machines[] = {
>  	},
>  	{
>  		.id = "10EC5682",
> +		.id_alt = &tgl_rt5682s_hp,
>  		.drv_name = "tgl_mx98373_rt5682",
>  		.machine_quirk = snd_soc_acpi_codec_list,
>  		.quirk_data = &tgl_max98373_amp,
> @@ -377,6 +384,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_tgl_machines[] = {
>  	},
>  	{
>  		.id = "10EC5682",
> +		.id_alt = &tgl_rt5682s_hp,
>  		.drv_name = "tgl_rt1011_rt5682",
>  		.machine_quirk = snd_soc_acpi_codec_list,
>  		.quirk_data = &tgl_rt1011_amp,
> 

Re: [PATCH 3/3] ASoC: Intel: sof_rt5682: use id_alt to enumerate rt5682s

[Pierre-Louis Bossart]

>> @@ -196,6 +201,7 @@ struct snd_soc_acpi_mach  snd_soc_acpi_intel_baytrail_machines[] = {
>>  	},
>>  	{
>>  		.id = "10EC5682",
>> +		.id_alt = &rt5682s_hp,
>>  		.drv_name = "sof_rt5682",
>>  		.sof_fw_filename = "sof-byt.ri",
>>  		.sof_tplg_filename = "sof-byt-rt5682.tplg",
> 
> So this is only useful if there actually are any BYT devices using the "RTL5682"
> ACPI HID, the 100+ BYT/CHT DSDTs which I've gather over time say there aren't any.
> 
> Actually there also aren't any using the non alt "10EC5682" ACPI HID either...
> 
> Bard Liao, you added this in commit f70abd75b7c6 ("ASoC: Intel: add sof-rt5682 machine driver")
> but I wonder how useful this is. I guess it may be available as (and tested on?) some dev-kit.
> 
> But I don't think there us any hardware out there in the wild using this ?

In the past we used this configuration for SOF CI tests with the
MinnowBoard + an RT5682 eval board. We gradually fried most boards and
no longer check this capability for each SOF PR.

So I would agree we can avoid changing anything for BYT/CHT and possibly
APL, it'd be an untested configuration.

in other words, let's add this compatible/alt_id for platforms where we
know it'll be used.

Re: [PATCH 3/3] ASoC: Intel: sof_rt5682: use id_alt to enumerate rt5682s

[Pierre-Louis Bossart]

>> @@ -196,6 +201,7 @@ struct snd_soc_acpi_mach  snd_soc_acpi_intel_baytrail_machines[] = {
>>  	},
>>  	{
>>  		.id = "10EC5682",
>> +		.id_alt = &rt5682s_hp,
>>  		.drv_name = "sof_rt5682",
>>  		.sof_fw_filename = "sof-byt.ri",
>>  		.sof_tplg_filename = "sof-byt-rt5682.tplg",
> 
> So this is only useful if there actually are any BYT devices using the "RTL5682"
> ACPI HID, the 100+ BYT/CHT DSDTs which I've gather over time say there aren't any.
> 
> Actually there also aren't any using the non alt "10EC5682" ACPI HID either...
> 
> Bard Liao, you added this in commit f70abd75b7c6 ("ASoC: Intel: add sof-rt5682 machine driver")
> but I wonder how useful this is. I guess it may be available as (and tested on?) some dev-kit.
> 
> But I don't think there us any hardware out there in the wild using this ?

In the past we used this configuration for SOF CI tests with the
MinnowBoard + an RT5682 eval board. We gradually fried most boards and
no longer check this capability for each SOF PR.

So I would agree we can avoid changing anything for BYT/CHT and possibly
APL, it'd be an untested configuration.

in other words, let's add this compatible/alt_id for platforms where we
know it'll be used.

Re: [PATCH 3/3] ASoC: Intel: sof_rt5682: use id_alt to enumerate rt5682s

[Curtis Malainey]

>  };
>
> +static struct snd_soc_acpi_codecs adl_rt5682s_hp = {
> +       .num_codecs = 1,
> +       .codecs = {"RTL5682"}
> +};
> +
>  struct snd_soc_acpi_mach snd_soc_acpi_intel_adl_machines[] = {
>         {
>                 .id = "10EC5682",
> +               .id_alt = &adl_rt5682s_hp,
>                 .drv_name = "adl_mx98373_rt5682",
>                 .machine_quirk = snd_soc_acpi_codec_list,
>                 .quirk_data = &adl_max98373_amp,
> @@ -296,6 +302,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_adl_machines[] = {
>         },
>         {
>                 .id = "10EC5682",
> +               .id_alt = &adl_rt5682s_hp,
>                 .drv_name = "adl_mx98357_rt5682",
>                 .machine_quirk = snd_soc_acpi_codec_list,
>                 .quirk_data = &adl_max98357a_amp,
> @@ -304,6 +311,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_adl_machines[] = {
>         },
>         {
>                 .id = "10EC5682",
> +               .id_alt = &adl_rt5682s_hp,
>                 .drv_name = "adl_mx98360_rt5682",
>                 .machine_quirk = snd_soc_acpi_codec_list,
>                 .quirk_data = &adl_max98360a_amp,

Is there any way we can collapse this and the primary id into a single
list to avoid having 2 locations to track for the IDs?

Re: [PATCH 3/3] ASoC: Intel: sof_rt5682: use id_alt to enumerate rt5682s

[Curtis Malainey]

>  };
>
> +static struct snd_soc_acpi_codecs adl_rt5682s_hp = {
> +       .num_codecs = 1,
> +       .codecs = {"RTL5682"}
> +};
> +
>  struct snd_soc_acpi_mach snd_soc_acpi_intel_adl_machines[] = {
>         {
>                 .id = "10EC5682",
> +               .id_alt = &adl_rt5682s_hp,
>                 .drv_name = "adl_mx98373_rt5682",
>                 .machine_quirk = snd_soc_acpi_codec_list,
>                 .quirk_data = &adl_max98373_amp,
> @@ -296,6 +302,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_adl_machines[] = {
>         },
>         {
>                 .id = "10EC5682",
> +               .id_alt = &adl_rt5682s_hp,
>                 .drv_name = "adl_mx98357_rt5682",
>                 .machine_quirk = snd_soc_acpi_codec_list,
>                 .quirk_data = &adl_max98357a_amp,
> @@ -304,6 +311,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_adl_machines[] = {
>         },
>         {
>                 .id = "10EC5682",
> +               .id_alt = &adl_rt5682s_hp,
>                 .drv_name = "adl_mx98360_rt5682",
>                 .machine_quirk = snd_soc_acpi_codec_list,
>                 .quirk_data = &adl_max98360a_amp,

Is there any way we can collapse this and the primary id into a single
list to avoid having 2 locations to track for the IDs?

Re: [PATCH 3/3] ASoC: Intel: sof_rt5682: use id_alt to enumerate rt5682s

[Pierre-Louis Bossart]

On 10/6/21 1:34 PM, Curtis Malainey wrote:
>>  };
>>
>> +static struct snd_soc_acpi_codecs adl_rt5682s_hp = {
>> +       .num_codecs = 1,
>> +       .codecs = {"RTL5682"}
>> +};
>> +
>>  struct snd_soc_acpi_mach snd_soc_acpi_intel_adl_machines[] = {
>>         {
>>                 .id = "10EC5682",
>> +               .id_alt = &adl_rt5682s_hp,
>>                 .drv_name = "adl_mx98373_rt5682",
>>                 .machine_quirk = snd_soc_acpi_codec_list,
>>                 .quirk_data = &adl_max98373_amp,
>> @@ -296,6 +302,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_adl_machines[] = {
>>         },
>>         {
>>                 .id = "10EC5682",
>> +               .id_alt = &adl_rt5682s_hp,
>>                 .drv_name = "adl_mx98357_rt5682",
>>                 .machine_quirk = snd_soc_acpi_codec_list,
>>                 .quirk_data = &adl_max98357a_amp,
>> @@ -304,6 +311,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_adl_machines[] = {
>>         },
>>         {
>>                 .id = "10EC5682",
>> +               .id_alt = &adl_rt5682s_hp,
>>                 .drv_name = "adl_mx98360_rt5682",
>>                 .machine_quirk = snd_soc_acpi_codec_list,
>>                 .quirk_data = &adl_max98360a_amp,
> 
> Is there any way we can collapse this and the primary id into a single
> list to avoid having 2 locations to track for the IDs?

I was thinking about that too, but in that case we would want to have a
list of strings, rather than the address of a structure which adds one
layer of indirection.

Something like

.id = { "10EC5682", "RTL5682" }

and the .num_codecs removed and some termination added.

Re: [PATCH 3/3] ASoC: Intel: sof_rt5682: use id_alt to enumerate rt5682s

[Pierre-Louis Bossart]

On 10/6/21 1:34 PM, Curtis Malainey wrote:
>>  };
>>
>> +static struct snd_soc_acpi_codecs adl_rt5682s_hp = {
>> +       .num_codecs = 1,
>> +       .codecs = {"RTL5682"}
>> +};
>> +
>>  struct snd_soc_acpi_mach snd_soc_acpi_intel_adl_machines[] = {
>>         {
>>                 .id = "10EC5682",
>> +               .id_alt = &adl_rt5682s_hp,
>>                 .drv_name = "adl_mx98373_rt5682",
>>                 .machine_quirk = snd_soc_acpi_codec_list,
>>                 .quirk_data = &adl_max98373_amp,
>> @@ -296,6 +302,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_adl_machines[] = {
>>         },
>>         {
>>                 .id = "10EC5682",
>> +               .id_alt = &adl_rt5682s_hp,
>>                 .drv_name = "adl_mx98357_rt5682",
>>                 .machine_quirk = snd_soc_acpi_codec_list,
>>                 .quirk_data = &adl_max98357a_amp,
>> @@ -304,6 +311,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_adl_machines[] = {
>>         },
>>         {
>>                 .id = "10EC5682",
>> +               .id_alt = &adl_rt5682s_hp,
>>                 .drv_name = "adl_mx98360_rt5682",
>>                 .machine_quirk = snd_soc_acpi_codec_list,
>>                 .quirk_data = &adl_max98360a_amp,
> 
> Is there any way we can collapse this and the primary id into a single
> list to avoid having 2 locations to track for the IDs?

I was thinking about that too, but in that case we would want to have a
list of strings, rather than the address of a structure which adds one
layer of indirection.

Something like

.id = { "10EC5682", "RTL5682" }

and the .num_codecs removed and some termination added.

Re: [PATCH 3/3] ASoC: Intel: sof_rt5682: use id_alt to enumerate rt5682s

[Curtis Malainey]

On Wed, Oct 6, 2021 at 11:43 AM Pierre-Louis Bossart
<pierre-louis.bossart@linux.intel.com> wrote:
>
>
>
> On 10/6/21 1:34 PM, Curtis Malainey wrote:
> >>  };
> >>
> >> +static struct snd_soc_acpi_codecs adl_rt5682s_hp = {
> >> +       .num_codecs = 1,
> >> +       .codecs = {"RTL5682"}
> >> +};
> >> +
> >>  struct snd_soc_acpi_mach snd_soc_acpi_intel_adl_machines[] = {
> >>         {
> >>                 .id = "10EC5682",
> >> +               .id_alt = &adl_rt5682s_hp,
> >>                 .drv_name = "adl_mx98373_rt5682",
> >>                 .machine_quirk = snd_soc_acpi_codec_list,
> >>                 .quirk_data = &adl_max98373_amp,
> >> @@ -296,6 +302,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_adl_machines[] = {
> >>         },
> >>         {
> >>                 .id = "10EC5682",
> >> +               .id_alt = &adl_rt5682s_hp,
> >>                 .drv_name = "adl_mx98357_rt5682",
> >>                 .machine_quirk = snd_soc_acpi_codec_list,
> >>                 .quirk_data = &adl_max98357a_amp,
> >> @@ -304,6 +311,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_adl_machines[] = {
> >>         },
> >>         {
> >>                 .id = "10EC5682",
> >> +               .id_alt = &adl_rt5682s_hp,
> >>                 .drv_name = "adl_mx98360_rt5682",
> >>                 .machine_quirk = snd_soc_acpi_codec_list,
> >>                 .quirk_data = &adl_max98360a_amp,
> >
> > Is there any way we can collapse this and the primary id into a single
> > list to avoid having 2 locations to track for the IDs?
>
> I was thinking about that too, but in that case we would want to have a
> list of strings, rather than the address of a structure which adds one
> layer of indirection.
>
> Something like
>
> .id = { "10EC5682", "RTL5682" }
>
> and the .num_codecs removed and some termination added.
>

I don't see an issue with still using a struct since we are using the
same list across multiple machines, but this makes me wonder if maybe
we should refactor this into another layer, having the ids at a top
structure and then the speaker matches a layer down. E.g.

 struct snd_soc_acpi_mach snd_soc_acpi_intel_adl_machines[] = {
        {
                .drv_name = "adl_mx98373_rt5682",
                .machine_quirk = snd_soc_acpi_codec_list,
                .quirk_data = &adl_max98373_amp,
        },
        {
                .drv_name = "adl_mx98357_rt5682",
                .machine_quirk = snd_soc_acpi_codec_list,
                .quirk_data = &adl_max98357a_amp,
        },
        {
                .drv_name = "adl_mx98360_rt5682",
                .machine_quirk = snd_soc_acpi_codec_list,
                .quirk_data = &adl_max98360a_amp,
        }
}

struct machine_driver adl_rt5682_driver_match {
  .id = { "10EC5682", "RTL5682" }
  .instances = &adl_rt5682_machines
}

Re: [PATCH 3/3] ASoC: Intel: sof_rt5682: use id_alt to enumerate rt5682s

[Curtis Malainey]

On Wed, Oct 6, 2021 at 11:43 AM Pierre-Louis Bossart
<pierre-louis.bossart@linux.intel.com> wrote:
>
>
>
> On 10/6/21 1:34 PM, Curtis Malainey wrote:
> >>  };
> >>
> >> +static struct snd_soc_acpi_codecs adl_rt5682s_hp = {
> >> +       .num_codecs = 1,
> >> +       .codecs = {"RTL5682"}
> >> +};
> >> +
> >>  struct snd_soc_acpi_mach snd_soc_acpi_intel_adl_machines[] = {
> >>         {
> >>                 .id = "10EC5682",
> >> +               .id_alt = &adl_rt5682s_hp,
> >>                 .drv_name = "adl_mx98373_rt5682",
> >>                 .machine_quirk = snd_soc_acpi_codec_list,
> >>                 .quirk_data = &adl_max98373_amp,
> >> @@ -296,6 +302,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_adl_machines[] = {
> >>         },
> >>         {
> >>                 .id = "10EC5682",
> >> +               .id_alt = &adl_rt5682s_hp,
> >>                 .drv_name = "adl_mx98357_rt5682",
> >>                 .machine_quirk = snd_soc_acpi_codec_list,
> >>                 .quirk_data = &adl_max98357a_amp,
> >> @@ -304,6 +311,7 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_adl_machines[] = {
> >>         },
> >>         {
> >>                 .id = "10EC5682",
> >> +               .id_alt = &adl_rt5682s_hp,
> >>                 .drv_name = "adl_mx98360_rt5682",
> >>                 .machine_quirk = snd_soc_acpi_codec_list,
> >>                 .quirk_data = &adl_max98360a_amp,
> >
> > Is there any way we can collapse this and the primary id into a single
> > list to avoid having 2 locations to track for the IDs?
>
> I was thinking about that too, but in that case we would want to have a
> list of strings, rather than the address of a structure which adds one
> layer of indirection.
>
> Something like
>
> .id = { "10EC5682", "RTL5682" }
>
> and the .num_codecs removed and some termination added.
>

I don't see an issue with still using a struct since we are using the
same list across multiple machines, but this makes me wonder if maybe
we should refactor this into another layer, having the ids at a top
structure and then the speaker matches a layer down. E.g.

 struct snd_soc_acpi_mach snd_soc_acpi_intel_adl_machines[] = {
        {
                .drv_name = "adl_mx98373_rt5682",
                .machine_quirk = snd_soc_acpi_codec_list,
                .quirk_data = &adl_max98373_amp,
        },
        {
                .drv_name = "adl_mx98357_rt5682",
                .machine_quirk = snd_soc_acpi_codec_list,
                .quirk_data = &adl_max98357a_amp,
        },
        {
                .drv_name = "adl_mx98360_rt5682",
                .machine_quirk = snd_soc_acpi_codec_list,
                .quirk_data = &adl_max98360a_amp,
        }
}

struct machine_driver adl_rt5682_driver_match {
  .id = { "10EC5682", "RTL5682" }
  .instances = &adl_rt5682_machines
}

Re: [PATCH 3/3] ASoC: Intel: sof_rt5682: use id_alt to enumerate rt5682s

[Pierre-Louis Bossart]

> I don't see an issue with still using a struct since we are using the
> same list across multiple machines, but this makes me wonder if maybe
> we should refactor this into another layer, having the ids at a top
> structure and then the speaker matches a layer down. E.g.
> 
>  struct snd_soc_acpi_mach snd_soc_acpi_intel_adl_machines[] = {
>         {
>                 .drv_name = "adl_mx98373_rt5682",
>                 .machine_quirk = snd_soc_acpi_codec_list,
>                 .quirk_data = &adl_max98373_amp,
>         },
>         {
>                 .drv_name = "adl_mx98357_rt5682",
>                 .machine_quirk = snd_soc_acpi_codec_list,
>                 .quirk_data = &adl_max98357a_amp,
>         },
>         {
>                 .drv_name = "adl_mx98360_rt5682",
>                 .machine_quirk = snd_soc_acpi_codec_list,
>                 .quirk_data = &adl_max98360a_amp,
>         }
> }
> 
> struct machine_driver adl_rt5682_driver_match {
>   .id = { "10EC5682", "RTL5682" }
>   .instances = &adl_rt5682_machines
> }

We probably need to experiment various options, on one hand the proposal
removes duplication but in a lot of cases outside of Chromebooks/rt5640
there is none, so that table rework adds an indirection with no real
benefit.

Re: [PATCH 3/3] ASoC: Intel: sof_rt5682: use id_alt to enumerate rt5682s

[Pierre-Louis Bossart]

> I don't see an issue with still using a struct since we are using the
> same list across multiple machines, but this makes me wonder if maybe
> we should refactor this into another layer, having the ids at a top
> structure and then the speaker matches a layer down. E.g.
> 
>  struct snd_soc_acpi_mach snd_soc_acpi_intel_adl_machines[] = {
>         {
>                 .drv_name = "adl_mx98373_rt5682",
>                 .machine_quirk = snd_soc_acpi_codec_list,
>                 .quirk_data = &adl_max98373_amp,
>         },
>         {
>                 .drv_name = "adl_mx98357_rt5682",
>                 .machine_quirk = snd_soc_acpi_codec_list,
>                 .quirk_data = &adl_max98357a_amp,
>         },
>         {
>                 .drv_name = "adl_mx98360_rt5682",
>                 .machine_quirk = snd_soc_acpi_codec_list,
>                 .quirk_data = &adl_max98360a_amp,
>         }
> }
> 
> struct machine_driver adl_rt5682_driver_match {
>   .id = { "10EC5682", "RTL5682" }
>   .instances = &adl_rt5682_machines
> }

We probably need to experiment various options, on one hand the proposal
removes duplication but in a lot of cases outside of Chromebooks/rt5640
there is none, so that table rework adds an indirection with no real
benefit.

Re: [PATCH 3/3] ASoC: Intel: sof_rt5682: use id_alt to enumerate rt5682s

[Curtis Malainey]

On Wed, Oct 6, 2021 at 12:58 PM Pierre-Louis Bossart
<pierre-louis.bossart@linux.intel.com> wrote:
>
>
> > I don't see an issue with still using a struct since we are using the
> > same list across multiple machines, but this makes me wonder if maybe
> > we should refactor this into another layer, having the ids at a top
> > structure and then the speaker matches a layer down. E.g.
> >
> >  struct snd_soc_acpi_mach snd_soc_acpi_intel_adl_machines[] = {
> >         {
> >                 .drv_name = "adl_mx98373_rt5682",
> >                 .machine_quirk = snd_soc_acpi_codec_list,
> >                 .quirk_data = &adl_max98373_amp,
> >         },
> >         {
> >                 .drv_name = "adl_mx98357_rt5682",
> >                 .machine_quirk = snd_soc_acpi_codec_list,
> >                 .quirk_data = &adl_max98357a_amp,
> >         },
> >         {
> >                 .drv_name = "adl_mx98360_rt5682",
> >                 .machine_quirk = snd_soc_acpi_codec_list,
> >                 .quirk_data = &adl_max98360a_amp,
> >         }
> > }
> >
> > struct machine_driver adl_rt5682_driver_match {
> >   .id = { "10EC5682", "RTL5682" }
> >   .instances = &adl_rt5682_machines
> > }
>
> We probably need to experiment various options, on one hand the proposal
> removes duplication but in a lot of cases outside of Chromebooks/rt5640
> there is none, so that table rework adds an indirection with no real
> benefit.

Fair point, given the current situation this would benefit RTK boards
only. I have no idea if we will end up in the same boat with any other
boards, but given the current supply chain status I would expect this
(multi-sourcing) to be a way of the future. So maybe an idea we pocket
for now and deploy when we end up in this situation  with more
drivers.

Re: [PATCH 3/3] ASoC: Intel: sof_rt5682: use id_alt to enumerate rt5682s

[Curtis Malainey]

On Wed, Oct 6, 2021 at 12:58 PM Pierre-Louis Bossart
<pierre-louis.bossart@linux.intel.com> wrote:
>
>
> > I don't see an issue with still using a struct since we are using the
> > same list across multiple machines, but this makes me wonder if maybe
> > we should refactor this into another layer, having the ids at a top
> > structure and then the speaker matches a layer down. E.g.
> >
> >  struct snd_soc_acpi_mach snd_soc_acpi_intel_adl_machines[] = {
> >         {
> >                 .drv_name = "adl_mx98373_rt5682",
> >                 .machine_quirk = snd_soc_acpi_codec_list,
> >                 .quirk_data = &adl_max98373_amp,
> >         },
> >         {
> >                 .drv_name = "adl_mx98357_rt5682",
> >                 .machine_quirk = snd_soc_acpi_codec_list,
> >                 .quirk_data = &adl_max98357a_amp,
> >         },
> >         {
> >                 .drv_name = "adl_mx98360_rt5682",
> >                 .machine_quirk = snd_soc_acpi_codec_list,
> >                 .quirk_data = &adl_max98360a_amp,
> >         }
> > }
> >
> > struct machine_driver adl_rt5682_driver_match {
> >   .id = { "10EC5682", "RTL5682" }
> >   .instances = &adl_rt5682_machines
> > }
>
> We probably need to experiment various options, on one hand the proposal
> removes duplication but in a lot of cases outside of Chromebooks/rt5640
> there is none, so that table rework adds an indirection with no real
> benefit.

Fair point, given the current situation this would benefit RTK boards
only. I have no idea if we will end up in the same boat with any other
boards, but given the current supply chain status I would expect this
(multi-sourcing) to be a way of the future. So maybe an idea we pocket
for now and deploy when we end up in this situation  with more
drivers.

RE: [PATCH 3/3] ASoC: Intel: sof_rt5682: use id_alt to enumerate rt5682s

[Liao, Bard]

> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Sent: Thursday, October 7, 2021 1:25 AM
> To: Hans de Goede <hdegoede@redhat.com>; Lu, Brent
> <brent.lu@intel.com>; alsa-devel@alsa-project.org; Bard liao <yung-
> chuan.liao@linux.intel.com>
> Cc: Liam Girdwood <lgirdwood@gmail.com>; Mark Brown
> <broonie@kernel.org>; Jaroslav Kysela <perex@perex.cz>; Takashi Iwai
> <tiwai@suse.com>; Rojewski, Cezary <cezary.rojewski@intel.com>; Jie Yang
> <yang.jie@linux.intel.com>; Kai Vehmanen
> <kai.vehmanen@linux.intel.com>; Guennadi Liakhovetski
> <guennadi.liakhovetski@linux.intel.com>; Zhi, Yong <yong.zhi@intel.com>;
> Gopal, Vamshi Krishna <vamshi.krishna.gopal@intel.com>; linux-
> kernel@vger.kernel.org; Wang, Rander <rander.wang@intel.com>; Liao,
> Bard <bard.liao@intel.com>; Malik_Hsu <malik_hsu@wistron.corp-
> partner.google.com>; Yang, Libin <libin.yang@intel.com>; Charles Keepax
> <ckeepax@opensource.cirrus.com>; Paul Olaru <paul.olaru@oss.nxp.com>;
> Curtis Malainey <cujomalainey@chromium.org>; Chiang, Mac
> <mac.chiang@intel.com>; Song, Gongjun <gongjun.song@intel.com>
> Subject: Re: [PATCH 3/3] ASoC: Intel: sof_rt5682: use id_alt to enumerate
> rt5682s
> 
> 
> >> @@ -196,6 +201,7 @@ struct snd_soc_acpi_mach
> snd_soc_acpi_intel_baytrail_machines[] = {
> >>  	},
> >>  	{
> >>  		.id = "10EC5682",
> >> +		.id_alt = &rt5682s_hp,
> >>  		.drv_name = "sof_rt5682",
> >>  		.sof_fw_filename = "sof-byt.ri",
> >>  		.sof_tplg_filename = "sof-byt-rt5682.tplg",
> >
> > So this is only useful if there actually are any BYT devices using the
> "RTL5682"
> > ACPI HID, the 100+ BYT/CHT DSDTs which I've gather over time say there
> aren't any.
> >
> > Actually there also aren't any using the non alt "10EC5682" ACPI HID either...
> >
> > Bard Liao, you added this in commit f70abd75b7c6 ("ASoC: Intel: add
> > sof-rt5682 machine driver") but I wonder how useful this is. I guess it may
> be available as (and tested on?) some dev-kit.
> >
> > But I don't think there us any hardware out there in the wild using this ?
> 
> In the past we used this configuration for SOF CI tests with the MinnowBoard
> + an RT5682 eval board. We gradually fried most boards and no longer check
> this capability for each SOF PR.
> 
> So I would agree we can avoid changing anything for BYT/CHT and possibly
> APL, it'd be an untested configuration.
> 
> in other words, let's add this compatible/alt_id for platforms where we know
> it'll be used.

Agree with Pierre. "RTL5682" is a new codec which is different from
"10EC5682" and not tested on old platforms.

RE: [PATCH 3/3] ASoC: Intel: sof_rt5682: use id_alt to enumerate rt5682s

[Liao, Bard]

> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Sent: Thursday, October 7, 2021 1:25 AM
> To: Hans de Goede <hdegoede@redhat.com>; Lu, Brent
> <brent.lu@intel.com>; alsa-devel@alsa-project.org; Bard liao <yung-
> chuan.liao@linux.intel.com>
> Cc: Liam Girdwood <lgirdwood@gmail.com>; Mark Brown
> <broonie@kernel.org>; Jaroslav Kysela <perex@perex.cz>; Takashi Iwai
> <tiwai@suse.com>; Rojewski, Cezary <cezary.rojewski@intel.com>; Jie Yang
> <yang.jie@linux.intel.com>; Kai Vehmanen
> <kai.vehmanen@linux.intel.com>; Guennadi Liakhovetski
> <guennadi.liakhovetski@linux.intel.com>; Zhi, Yong <yong.zhi@intel.com>;
> Gopal, Vamshi Krishna <vamshi.krishna.gopal@intel.com>; linux-
> kernel@vger.kernel.org; Wang, Rander <rander.wang@intel.com>; Liao,
> Bard <bard.liao@intel.com>; Malik_Hsu <malik_hsu@wistron.corp-
> partner.google.com>; Yang, Libin <libin.yang@intel.com>; Charles Keepax
> <ckeepax@opensource.cirrus.com>; Paul Olaru <paul.olaru@oss.nxp.com>;
> Curtis Malainey <cujomalainey@chromium.org>; Chiang, Mac
> <mac.chiang@intel.com>; Song, Gongjun <gongjun.song@intel.com>
> Subject: Re: [PATCH 3/3] ASoC: Intel: sof_rt5682: use id_alt to enumerate
> rt5682s
> 
> 
> >> @@ -196,6 +201,7 @@ struct snd_soc_acpi_mach
> snd_soc_acpi_intel_baytrail_machines[] = {
> >>  	},
> >>  	{
> >>  		.id = "10EC5682",
> >> +		.id_alt = &rt5682s_hp,
> >>  		.drv_name = "sof_rt5682",
> >>  		.sof_fw_filename = "sof-byt.ri",
> >>  		.sof_tplg_filename = "sof-byt-rt5682.tplg",
> >
> > So this is only useful if there actually are any BYT devices using the
> "RTL5682"
> > ACPI HID, the 100+ BYT/CHT DSDTs which I've gather over time say there
> aren't any.
> >
> > Actually there also aren't any using the non alt "10EC5682" ACPI HID either...
> >
> > Bard Liao, you added this in commit f70abd75b7c6 ("ASoC: Intel: add
> > sof-rt5682 machine driver") but I wonder how useful this is. I guess it may
> be available as (and tested on?) some dev-kit.
> >
> > But I don't think there us any hardware out there in the wild using this ?
> 
> In the past we used this configuration for SOF CI tests with the MinnowBoard
> + an RT5682 eval board. We gradually fried most boards and no longer check
> this capability for each SOF PR.
> 
> So I would agree we can avoid changing anything for BYT/CHT and possibly
> APL, it'd be an untested configuration.
> 
> in other words, let's add this compatible/alt_id for platforms where we know
> it'll be used.

Agree with Pierre. "RTL5682" is a new codec which is different from
"10EC5682" and not tested on old platforms.

RE: [PATCH 3/3] ASoC: Intel: sof_rt5682: use id_alt to enumerate rt5682s

[Lu, Brent]

> >
> > In the past we used this configuration for SOF CI tests with the
> > MinnowBoard
> > + an RT5682 eval board. We gradually fried most boards and no longer
> > + check
> > this capability for each SOF PR.
> >
> > So I would agree we can avoid changing anything for BYT/CHT and
> > possibly APL, it'd be an untested configuration.
> >
> > in other words, let's add this compatible/alt_id for platforms where
> > we know it'll be used.
> 
> Agree with Pierre. "RTL5682" is a new codec which is different from
> "10EC5682" and not tested on old platforms.

As I know the part is introduced to JSL and ADL projects so maybe we can
keep the change to JSL/TGL/ADL boards?

RE: [PATCH 3/3] ASoC: Intel: sof_rt5682: use id_alt to enumerate rt5682s

[Lu, Brent]

> >
> > In the past we used this configuration for SOF CI tests with the
> > MinnowBoard
> > + an RT5682 eval board. We gradually fried most boards and no longer
> > + check
> > this capability for each SOF PR.
> >
> > So I would agree we can avoid changing anything for BYT/CHT and
> > possibly APL, it'd be an untested configuration.
> >
> > in other words, let's add this compatible/alt_id for platforms where
> > we know it'll be used.
> 
> Agree with Pierre. "RTL5682" is a new codec which is different from
> "10EC5682" and not tested on old platforms.

As I know the part is introduced to JSL and ADL projects so maybe we can
keep the change to JSL/TGL/ADL boards?