[PATCH 0/2] Intel: sof_rt5682: remove quirk flag

[PATCH 0/2] Intel: sof_rt5682: remove quirk flag

[Brent Lu]

We add a helper funcion to detect amplifier number according to device instance
in ACPI table so the SOF_MAX98390_TWEETER_SPEAKER_PRESENT flag and a dmi quirk
for 4-amplifier configuration could be safely removed.

Also refactory the max_98390_hw_params() function to use an array to handle the
TDM parameter.

Amplifier number detection and TDM parameter are tested on two Chromebooks. One
with 2 MAX98390 and one with 4 MAX98390 amplifier.


*** BLURB HERE ***

Brent Lu (2):
  ASoC: Intel: maxim-common: get codec number from ACPI
  ASoC: Intel: sof_rt5682: remove SOF_MAX98390_TWEETER_SPEAKER_PRESENT
    flag

 sound/soc/intel/boards/sof_maxim_common.c | 174 +++++++++++++---------
 sound/soc/intel/boards/sof_maxim_common.h |  21 ++-
 sound/soc/intel/boards/sof_rt5682.c       |  37 +----
 3 files changed, 115 insertions(+), 117 deletions(-)

-- 
2.34.1

[PATCH 1/2] ASoC: Intel: maxim-common: get codec number from ACPI

[Brent Lu]

Implement a helper function to get number of codecs from ACPI
subsystem to remove the need of quirk flag in machine driver.

Signed-off-by: Brent Lu <brent.lu@intel.com>
---
 sound/soc/intel/boards/sof_maxim_common.c | 174 +++++++++++++---------
 sound/soc/intel/boards/sof_maxim_common.h |  21 ++-
 2 files changed, 113 insertions(+), 82 deletions(-)

diff --git a/sound/soc/intel/boards/sof_maxim_common.c b/sound/soc/intel/boards/sof_maxim_common.c
index 112e89951da0..f8b44a81fec1 100644
--- a/sound/soc/intel/boards/sof_maxim_common.c
+++ b/sound/soc/intel/boards/sof_maxim_common.c
@@ -4,6 +4,7 @@
 #include <linux/module.h>
 #include <linux/string.h>
 #include <sound/pcm.h>
+#include <sound/pcm_params.h>
 #include <sound/soc.h>
 #include <sound/soc-acpi.h>
 #include <sound/soc-dai.h>
@@ -11,6 +12,21 @@
 #include <uapi/sound/asound.h>
 #include "sof_maxim_common.h"
 
+/* helper function to get the number of specific codec */
+static int get_num_codecs(const char *hid)
+{
+	struct acpi_device *adev = NULL;
+	int dev_num = 0;
+
+	do {
+		adev = acpi_dev_get_next_match_dev(adev, hid, NULL, -1);
+		if (adev)
+			dev_num++;
+	} while (adev != NULL);
+
+	return dev_num;
+}
+
 #define MAX_98373_PIN_NAME 16
 
 const struct snd_soc_dapm_route max_98373_dapm_routes[] = {
@@ -168,17 +184,6 @@ static struct snd_soc_codec_conf max_98390_codec_conf[] = {
 		.dlc = COMP_CODEC_CONF(MAX_98390_DEV1_NAME),
 		.name_prefix = "Left",
 	},
-};
-
-static struct snd_soc_codec_conf max_98390_4spk_codec_conf[] = {
-	{
-		.dlc = COMP_CODEC_CONF(MAX_98390_DEV0_NAME),
-		.name_prefix = "Right",
-	},
-	{
-		.dlc = COMP_CODEC_CONF(MAX_98390_DEV1_NAME),
-		.name_prefix = "Left",
-	},
 	{
 		.dlc = COMP_CODEC_CONF(MAX_98390_DEV2_NAME),
 		.name_prefix = "Tweeter Right",
@@ -189,19 +194,7 @@ static struct snd_soc_codec_conf max_98390_4spk_codec_conf[] = {
 	},
 };
 
-struct snd_soc_dai_link_component max_98390_components[] = {
-	{
-		.name = MAX_98390_DEV0_NAME,
-		.dai_name = MAX_98390_CODEC_DAI,
-	},
-	{
-		.name = MAX_98390_DEV1_NAME,
-		.dai_name = MAX_98390_CODEC_DAI,
-	},
-};
-EXPORT_SYMBOL_NS(max_98390_components, SND_SOC_INTEL_SOF_MAXIM_COMMON);
-
-struct snd_soc_dai_link_component max_98390_4spk_components[] = {
+static struct snd_soc_dai_link_component max_98390_components[] = {
 	{
 		.name = MAX_98390_DEV0_NAME,
 		.dai_name = MAX_98390_CODEC_DAI,
@@ -219,62 +212,56 @@ struct snd_soc_dai_link_component max_98390_4spk_components[] = {
 		.dai_name = MAX_98390_CODEC_DAI,
 	},
 };
-EXPORT_SYMBOL_NS(max_98390_4spk_components, SND_SOC_INTEL_SOF_MAXIM_COMMON);
+
+static const struct {
+	unsigned int tx;
+	unsigned int rx;
+} max_98390_tdm_mask[] = {
+	{.tx = 0x01, .rx = 0x3},
+	{.tx = 0x02, .rx = 0x3},
+	{.tx = 0x04, .rx = 0x3},
+	{.tx = 0x08, .rx = 0x3},
+};
 
 static int max_98390_hw_params(struct snd_pcm_substream *substream,
 			       struct snd_pcm_hw_params *params)
 {
 	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
 	struct snd_soc_dai *codec_dai;
-	int i;
+	int i, ret = 0;
 
 	for_each_rtd_codec_dais(rtd, i, codec_dai) {
-		if (i >= ARRAY_SIZE(max_98390_4spk_components)) {
+		if (i >= ARRAY_SIZE(max_98390_tdm_mask)) {
 			dev_err(codec_dai->dev, "invalid codec index %d\n", i);
 			return -ENODEV;
 		}
 
-		if (!strcmp(codec_dai->component->name, MAX_98390_DEV0_NAME)) {
-			/* DEV0 tdm slot configuration Right */
-			snd_soc_dai_set_tdm_slot(codec_dai, 0x01, 3, 4, 32);
-		}
-		if (!strcmp(codec_dai->component->name, MAX_98390_DEV1_NAME)) {
-			/* DEV1 tdm slot configuration Left */
-			snd_soc_dai_set_tdm_slot(codec_dai, 0x02, 3, 4, 32);
-		}
-
-		if (!strcmp(codec_dai->component->name, MAX_98390_DEV2_NAME)) {
-			/* DEVi2 tdm slot configuration Tweeter Right */
-			snd_soc_dai_set_tdm_slot(codec_dai, 0x04, 3, 4, 32);
-		}
-		if (!strcmp(codec_dai->component->name, MAX_98390_DEV3_NAME)) {
-			/* DEV3 tdm slot configuration Tweeter Left */
-			snd_soc_dai_set_tdm_slot(codec_dai, 0x08, 3, 4, 32);
+		ret = snd_soc_dai_set_tdm_slot(codec_dai, max_98390_tdm_mask[i].tx,
+					       max_98390_tdm_mask[i].rx, 4,
+					       params_width(params));
+		if (ret < 0) {
+			dev_err(codec_dai->dev, "fail to set tdm slot, ret %d\n",
+				ret);
+			return ret;
 		}
 	}
 	return 0;
 }
 
-int max_98390_spk_codec_init(struct snd_soc_pcm_runtime *rtd)
+static int max_98390_init(struct snd_soc_pcm_runtime *rtd)
 {
 	struct snd_soc_card *card = rtd->card;
+	int num_codecs = get_num_codecs(MAX_98390_ACPI_HID);
 	int ret;
 
-	/* add regular speakers dapm route */
-	ret = snd_soc_dapm_add_routes(&card->dapm, max_98390_dapm_routes,
-				      ARRAY_SIZE(max_98390_dapm_routes));
-	if (ret) {
-		dev_err(rtd->dev, "unable to add Left/Right Speaker dapm, ret %d\n", ret);
-		return ret;
-	}
-
-	/* add widgets/controls/dapm for tweeter speakers */
-	if (acpi_dev_present("MX98390", "3", -1)) {
+	switch (num_codecs) {
+	case 4:
+		/* add widgets/controls/dapm for tweeter speakers */
 		ret = snd_soc_dapm_new_controls(&card->dapm, max_98390_tt_dapm_widgets,
 						ARRAY_SIZE(max_98390_tt_dapm_widgets));
-
 		if (ret) {
-			dev_err(rtd->dev, "unable to add tweeter dapm controls, ret %d\n", ret);
+			dev_err(rtd->dev, "unable to add tweeter dapm widgets, ret %d\n",
+				ret);
 			/* Don't need to add routes if widget addition failed */
 			return ret;
 		}
@@ -282,33 +269,80 @@ int max_98390_spk_codec_init(struct snd_soc_pcm_runtime *rtd)
 		ret = snd_soc_add_card_controls(card, max_98390_tt_kcontrols,
 						ARRAY_SIZE(max_98390_tt_kcontrols));
 		if (ret) {
-			dev_err(rtd->dev, "unable to add tweeter card controls, ret %d\n", ret);
+			dev_err(rtd->dev, "unable to add tweeter controls, ret %d\n",
+				ret);
 			return ret;
 		}
 
 		ret = snd_soc_dapm_add_routes(&card->dapm, max_98390_tt_dapm_routes,
 					      ARRAY_SIZE(max_98390_tt_dapm_routes));
-		if (ret)
-			dev_err(rtd->dev,
-				"unable to add Tweeter Left/Right Speaker dapm, ret %d\n", ret);
+		if (ret) {
+			dev_err(rtd->dev, "unable to add tweeter dapm routes, ret %d\n",
+				ret);
+			return ret;
+		}
+
+		fallthrough;
+	case 2:
+		/* add regular speakers dapm route */
+		ret = snd_soc_dapm_add_routes(&card->dapm, max_98390_dapm_routes,
+					      ARRAY_SIZE(max_98390_dapm_routes));
+		if (ret) {
+			dev_err(rtd->dev, "unable to add dapm routes, ret %d\n",
+				ret);
+			return ret;
+		}
+		break;
+	default:
+		dev_err(rtd->dev, "invalid codec number %d\n", num_codecs);
+		ret = -EINVAL;
+		break;
 	}
+
 	return ret;
 }
-EXPORT_SYMBOL_NS(max_98390_spk_codec_init, SND_SOC_INTEL_SOF_MAXIM_COMMON);
 
-const struct snd_soc_ops max_98390_ops = {
+static const struct snd_soc_ops max_98390_ops = {
 	.hw_params = max_98390_hw_params,
 };
-EXPORT_SYMBOL_NS(max_98390_ops, SND_SOC_INTEL_SOF_MAXIM_COMMON);
 
-void max_98390_set_codec_conf(struct snd_soc_card *card, int ch)
+void max_98390_dai_link(struct snd_soc_dai_link *link)
+{
+	int num_codecs = get_num_codecs(MAX_98390_ACPI_HID);
+
+	link->codecs = max_98390_components;
+
+	switch (num_codecs) {
+	case 2:
+	case 4:
+		link->num_codecs = num_codecs;
+		break;
+	default:
+		pr_err("invalid codec number %d for %s\n", num_codecs,
+			MAX_98390_ACPI_HID);
+		break;
+	}
+
+	link->init = max_98390_init;
+	link->ops = &max_98390_ops;
+}
+EXPORT_SYMBOL_NS(max_98390_dai_link, SND_SOC_INTEL_SOF_MAXIM_COMMON);
+
+void max_98390_set_codec_conf(struct snd_soc_card *card)
 {
-	if (ch == ARRAY_SIZE(max_98390_4spk_codec_conf)) {
-		card->codec_conf = max_98390_4spk_codec_conf;
-		card->num_configs = ARRAY_SIZE(max_98390_4spk_codec_conf);
-	} else {
-		card->codec_conf = max_98390_codec_conf;
-		card->num_configs = ARRAY_SIZE(max_98390_codec_conf);
+	int num_codecs = get_num_codecs(MAX_98390_ACPI_HID);
+
+	card->codec_conf = max_98390_codec_conf;
+
+	switch (num_codecs) {
+	case 2:
+	case 4:
+		card->num_configs = num_codecs;
+		break;
+	default:
+		pr_err("invalid codec number %d for %s\n", num_codecs,
+			MAX_98390_ACPI_HID);
+		break;
 	}
 }
 EXPORT_SYMBOL_NS(max_98390_set_codec_conf, SND_SOC_INTEL_SOF_MAXIM_COMMON);
diff --git a/sound/soc/intel/boards/sof_maxim_common.h b/sound/soc/intel/boards/sof_maxim_common.h
index 7a8c53049e4d..a3676d68cc12 100644
--- a/sound/soc/intel/boards/sof_maxim_common.h
+++ b/sound/soc/intel/boards/sof_maxim_common.h
@@ -27,18 +27,15 @@ int max_98373_trigger(struct snd_pcm_substream *substream, int cmd);
 /*
  * Maxim MAX98390
  */
-#define MAX_98390_CODEC_DAI     "max98390-aif1"
-#define MAX_98390_DEV0_NAME     "i2c-MX98390:00"
-#define MAX_98390_DEV1_NAME     "i2c-MX98390:01"
-#define MAX_98390_DEV2_NAME     "i2c-MX98390:02"
-#define MAX_98390_DEV3_NAME     "i2c-MX98390:03"
-
-extern struct snd_soc_dai_link_component max_98390_components[2];
-extern struct snd_soc_dai_link_component max_98390_4spk_components[4];
-extern const struct snd_soc_ops max_98390_ops;
-
-void max_98390_set_codec_conf(struct snd_soc_card *card, int ch);
-int max_98390_spk_codec_init(struct snd_soc_pcm_runtime *rtd);
+#define MAX_98390_ACPI_HID	"MX98390"
+#define MAX_98390_CODEC_DAI	"max98390-aif1"
+#define MAX_98390_DEV0_NAME	"i2c-MX98390:00"
+#define MAX_98390_DEV1_NAME	"i2c-MX98390:01"
+#define MAX_98390_DEV2_NAME	"i2c-MX98390:02"
+#define MAX_98390_DEV3_NAME	"i2c-MX98390:03"
+
+void max_98390_dai_link(struct snd_soc_dai_link *link);
+void max_98390_set_codec_conf(struct snd_soc_card *card);
 
 /*
  * Maxim MAX98357A/MAX98360A
-- 
2.34.1

[PATCH 2/2] ASoC: Intel: sof_rt5682: remove SOF_MAX98390_TWEETER_SPEAKER_PRESENT flag

[Brent Lu]

Remove the SOF_MAX98390_TWEETER_SPEAKER_PRESENT flag from driver since
the number of amplifiers could be queried from ACPI by counting the
device instance.

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

diff --git a/sound/soc/intel/boards/sof_rt5682.c b/sound/soc/intel/boards/sof_rt5682.c
index b4f07bdcf8b4..9116f4df4752 100644
--- a/sound/soc/intel/boards/sof_rt5682.c
+++ b/sound/soc/intel/boards/sof_rt5682.c
@@ -59,7 +59,6 @@
 #define SOF_SSP_BT_OFFLOAD_PRESENT		BIT(22)
 #define SOF_RT5682S_HEADPHONE_CODEC_PRESENT	BIT(23)
 #define SOF_MAX98390_SPEAKER_AMP_PRESENT	BIT(24)
-#define SOF_MAX98390_TWEETER_SPEAKER_PRESENT	BIT(25)
 #define SOF_RT1019_SPEAKER_AMP_PRESENT	BIT(26)
 #define SOF_RT5650_HEADPHONE_CODEC_PRESENT	BIT(27)
 
@@ -195,23 +194,6 @@ static const struct dmi_system_id sof_rt5682_quirk_table[] = {
 					SOF_RT5682_SSP_AMP(2) |
 					SOF_RT5682_NUM_HDMIDEV(4)),
 	},
-	{
-		.callback = sof_rt5682_quirk_cb,
-		.matches = {
-			DMI_MATCH(DMI_PRODUCT_FAMILY, "Google_Brya"),
-			DMI_MATCH(DMI_OEM_STRING, "AUDIO-MAX98390_ALC5682I_I2S_4SPK"),
-		},
-		.driver_data = (void *)(SOF_RT5682_MCLK_EN |
-					SOF_RT5682_SSP_CODEC(0) |
-					SOF_SPEAKER_AMP_PRESENT |
-					SOF_MAX98390_SPEAKER_AMP_PRESENT |
-					SOF_MAX98390_TWEETER_SPEAKER_PRESENT |
-					SOF_RT5682_SSP_AMP(1) |
-					SOF_RT5682_NUM_HDMIDEV(4) |
-					SOF_BT_OFFLOAD_SSP(2) |
-					SOF_SSP_BT_OFFLOAD_PRESENT),
-
-	},
 	{
 		.callback = sof_rt5682_quirk_cb,
 		.matches = {
@@ -850,17 +832,7 @@ static struct snd_soc_dai_link *sof_card_dai_links_create(struct device *dev,
 			sof_rt1011_dai_link(&links[id]);
 		} else if (sof_rt5682_quirk &
 				SOF_MAX98390_SPEAKER_AMP_PRESENT) {
-			if (sof_rt5682_quirk &
-				SOF_MAX98390_TWEETER_SPEAKER_PRESENT) {
-				links[id].codecs = max_98390_4spk_components;
-				links[id].num_codecs = ARRAY_SIZE(max_98390_4spk_components);
-			} else {
-				links[id].codecs = max_98390_components;
-				links[id].num_codecs = ARRAY_SIZE(max_98390_components);
-			}
-			links[id].init = max_98390_spk_codec_init;
-			links[id].ops = &max_98390_ops;
-
+			max_98390_dai_link(&links[id]);
 		} else if (sof_rt5682_quirk & SOF_RT5650_HEADPHONE_CODEC_PRESENT) {
 			links[id].codecs = &rt5650_components[1];
 			links[id].num_codecs = 1;
@@ -1019,12 +991,7 @@ static int sof_audio_probe(struct platform_device *pdev)
 	else if (sof_rt5682_quirk & SOF_RT1015P_SPEAKER_AMP_PRESENT)
 		sof_rt1015p_codec_conf(&sof_audio_card_rt5682);
 	else if (sof_rt5682_quirk & SOF_MAX98390_SPEAKER_AMP_PRESENT) {
-		if (sof_rt5682_quirk & SOF_MAX98390_TWEETER_SPEAKER_PRESENT)
-			max_98390_set_codec_conf(&sof_audio_card_rt5682,
-						 ARRAY_SIZE(max_98390_4spk_components));
-		else
-			max_98390_set_codec_conf(&sof_audio_card_rt5682,
-						 ARRAY_SIZE(max_98390_components));
+		max_98390_set_codec_conf(&sof_audio_card_rt5682);
 	}
 
 	if (sof_rt5682_quirk & SOF_SSP_BT_OFFLOAD_PRESENT)
-- 
2.34.1

Re: [PATCH 0/2] Intel: sof_rt5682: remove quirk flag

[Kai Vehmanen]

Hi,

On Thu, 20 Jul 2023, Brent Lu wrote:

> We add a helper funcion to detect amplifier number according to device instance
> in ACPI table so the SOF_MAX98390_TWEETER_SPEAKER_PRESENT flag and a dmi quirk
> for 4-amplifier configuration could be safely removed.
> 
> Also refactory the max_98390_hw_params() function to use an array to handle the
> TDM parameter.
> 
> Amplifier number detection and TDM parameter are tested on two Chromebooks. One
> with 2 MAX98390 and one with 4 MAX98390 amplifier.
> 
> 
> *** BLURB HERE ***

this looks like a nice cleanup, thanks Brent. For the series:

Acked-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>

Minor nit: some spelling erros in cover letter (funcion->function, 
refactory->refactor, BLURB HERE left), but I think the intent 
comes across.

Br, Kai

Re: [PATCH 1/2] ASoC: Intel: maxim-common: get codec number from ACPI

[Pierre-Louis Bossart]

On 7/20/23 11:26, Brent Lu wrote:
> Implement a helper function to get number of codecs from ACPI
> subsystem to remove the need of quirk flag in machine driver.
> 
> Signed-off-by: Brent Lu <brent.lu@intel.com>
> ---
>  sound/soc/intel/boards/sof_maxim_common.c | 174 +++++++++++++---------
>  sound/soc/intel/boards/sof_maxim_common.h |  21 ++-
>  2 files changed, 113 insertions(+), 82 deletions(-)
> 
> diff --git a/sound/soc/intel/boards/sof_maxim_common.c b/sound/soc/intel/boards/sof_maxim_common.c
> index 112e89951da0..f8b44a81fec1 100644
> --- a/sound/soc/intel/boards/sof_maxim_common.c
> +++ b/sound/soc/intel/boards/sof_maxim_common.c
> @@ -4,6 +4,7 @@
>  #include <linux/module.h>
>  #include <linux/string.h>
>  #include <sound/pcm.h>
> +#include <sound/pcm_params.h>
>  #include <sound/soc.h>
>  #include <sound/soc-acpi.h>
>  #include <sound/soc-dai.h>
> @@ -11,6 +12,21 @@
>  #include <uapi/sound/asound.h>
>  #include "sof_maxim_common.h"
>  
> +/* helper function to get the number of specific codec */
> +static int get_num_codecs(const char *hid)
> +{
> +	struct acpi_device *adev = NULL;
> +	int dev_num = 0;
> +
> +	do {
> +		adev = acpi_dev_get_next_match_dev(adev, hid, NULL, -1);

Humm, I am a bit worried about reference counts.

See
https://elixir.bootlin.com/linux/latest/source/drivers/acpi/utils.c#L916,
it's not clear to me where the get() is done.

Adding Andy to make sure this is done right.

> +		if (adev)
> +			dev_num++;
> +	} while (adev != NULL);
> +
> +	return dev_num;
> +}

Re: [PATCH 1/2] ASoC: Intel: maxim-common: get codec number from ACPI

[Andy Shevchenko]

On Mon, Jul 24, 2023 at 11:08:17AM +0200, Pierre-Louis Bossart wrote:
> On 7/20/23 11:26, Brent Lu wrote:

...

> > +/* helper function to get the number of specific codec */

...and leak a lot of reference counts...

> > +static int get_num_codecs(const char *hid)
> > +{
> > +	struct acpi_device *adev = NULL;
> > +	int dev_num = 0;
> > +
> > +	do {
> > +		adev = acpi_dev_get_next_match_dev(adev, hid, NULL, -1);
> 
> Humm, I am a bit worried about reference counts.
> 
> See
> https://elixir.bootlin.com/linux/latest/source/drivers/acpi/utils.c#L916,
> it's not clear to me where the get() is done.
> 
> Adding Andy to make sure this is done right.

Thank you for Cc'ing.

Yes, the above code is problematic. One has to use the respective for_each
macro (defined nearby the used API).

> > +		if (adev)
> > +			dev_num++;
> > +	} while (adev != NULL);
> > +
> > +	return dev_num;
> > +}

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH 1/2] ASoC: Intel: maxim-common: get codec number from ACPI

[Lu, Brent]

> > > +/* helper function to get the number of specific codec */
> 
> ...and leak a lot of reference counts...
> 
> > > +static int get_num_codecs(const char *hid) {
> > > +	struct acpi_device *adev = NULL;
> > > +	int dev_num = 0;
> > > +
> > > +	do {
> > > +		adev = acpi_dev_get_next_match_dev(adev, hid, NULL, -1);
> >
> > Humm, I am a bit worried about reference counts.
> >
> > See
> > https://elixir.bootlin.com/linux/latest/source/drivers/acpi/utils.c#L9
> > 16, it's not clear to me where the get() is done.
> >
> > Adding Andy to make sure this is done right.
> 
> Thank you for Cc'ing.
> 
> Yes, the above code is problematic. One has to use the respective for_each macro
> (defined nearby the used API).
> 
> > > +		if (adev)
> > > +			dev_num++;
> > > +	} while (adev != NULL);
> > > +
> > > +	return dev_num;
> > > +}
> 
> --
> With Best Regards,
> Andy Shevchenko
> 
Hi Andy,

Each invocation of acpi_dev_get_next_match_dev() calls acpi_dev_put() to release the
adev from previous call. And the last call returns NULL. It seems to me the reference count
should be fine. Is my understanding correct?

I saw the macro for_each_acpi_dev_match and re-write the function as follow. Thanks for
suggesting using the macro.

/* helper function to get the number of specific codec */
static int get_num_codecs(const char *hid) {
	struct acpi_device *adev;
	int dev_num = 0;

	for_each_acpi_dev_match(adev, hid, NULL, -1)
		dev_num++;

	return dev_num;
}

Will test it in next few days.

Regards,
Brent

Re: [PATCH 1/2] ASoC: Intel: maxim-common: get codec number from ACPI

[Andy Shevchenko]

On Mon, Jul 24, 2023 at 11:06:02AM +0000, Lu, Brent wrote:
> > > > +/* helper function to get the number of specific codec */
> > 
> > ...and leak a lot of reference counts...
> > 
> > > > +static int get_num_codecs(const char *hid) {
> > > > +	struct acpi_device *adev = NULL;
> > > > +	int dev_num = 0;
> > > > +
> > > > +	do {
> > > > +		adev = acpi_dev_get_next_match_dev(adev, hid, NULL, -1);
> > >
> > > Humm, I am a bit worried about reference counts.
> > >
> > > See
> > > https://elixir.bootlin.com/linux/latest/source/drivers/acpi/utils.c#L9
> > > 16, it's not clear to me where the get() is done.
> > >
> > > Adding Andy to make sure this is done right.
> > 
> > Thank you for Cc'ing.
> > 
> > Yes, the above code is problematic. One has to use the respective for_each macro
> > (defined nearby the used API).
> > 
> > > > +		if (adev)
> > > > +			dev_num++;
> > > > +	} while (adev != NULL);
> > > > +
> > > > +	return dev_num;
> > > > +}

> Each invocation of acpi_dev_get_next_match_dev() calls acpi_dev_put() to release the
> adev from previous call. And the last call returns NULL. It seems to me the reference count
> should be fine. Is my understanding correct?

Ah, right. sorry for the confusion. That's why we have a macro
to not think about these details :-)

> I saw the macro for_each_acpi_dev_match and re-write the function as follow. Thanks for
> suggesting using the macro.
> 
> /* helper function to get the number of specific codec */
> static int get_num_codecs(const char *hid) {
> 	struct acpi_device *adev;

> 	int dev_num = 0;

size_t here or at least unsigned int is more correct.

> 	for_each_acpi_dev_match(adev, hid, NULL, -1)
> 		dev_num++;
> 
> 	return dev_num;
> }

Otherwise, yes, that's what I have in mind.

> Will test it in next few days.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH 1/2] ASoC: Intel: maxim-common: get codec number from ACPI

[Lu, Brent]

> 
> size_t here or at least unsigned int is more correct.
> 
> > 	for_each_acpi_dev_match(adev, hid, NULL, -1)
> > 		dev_num++;
> >
> > 	return dev_num;
> > }
> 
> Otherwise, yes, that's what I have in mind.
> 
> > Will test it in next few days.
> 
> --
> With Best Regards,
> Andy Shevchenko
> 

Thanks for the review. I've modified the v2 patch accordingly.

Regards,
Brent

Re: [PATCH 1/2] ASoC: Intel: maxim-common: get codec number from ACPI

[Liao, Bard]

On 7/20/2023 5:26 PM, Brent Lu wrote:
> Implement a helper function to get number of codecs from ACPI
> subsystem to remove the need of quirk flag in machine driver.
>
> Signed-off-by: Brent Lu <brent.lu@intel.com>
> ---
>   sound/soc/intel/boards/sof_maxim_common.c | 174 +++++++++++++---------
>   sound/soc/intel/boards/sof_maxim_common.h |  21 ++-
>   2 files changed, 113 insertions(+), 82 deletions(-)
>
> diff --git a/sound/soc/intel/boards/sof_maxim_common.c b/sound/soc/intel/boards/sof_maxim_common.c
> index 112e89951da0..f8b44a81fec1 100644
> --- a/sound/soc/intel/boards/sof_maxim_common.c
> +++ b/sound/soc/intel/boards/sof_maxim_common.c
> @@ -4,6 +4,7 @@
>   #include <linux/module.h>
>   #include <linux/string.h>
>   #include <sound/pcm.h>
> +#include <sound/pcm_params.h>
>   #include <sound/soc.h>
>   #include <sound/soc-acpi.h>
>   #include <sound/soc-dai.h>
> @@ -11,6 +12,21 @@
>   #include <uapi/sound/asound.h>
>   #include "sof_maxim_common.h"
>   
> +/* helper function to get the number of specific codec */
> +static int get_num_codecs(const char *hid)
> +{
> +	struct acpi_device *adev = NULL;
> +	int dev_num = 0;
> +
> +	do {
> +		adev = acpi_dev_get_next_match_dev(adev, hid, NULL, -1);
> +		if (adev)
> +			dev_num++;
> +	} while (adev != NULL);
> +
> +	return dev_num;
> +}
> +
>   #define MAX_98373_PIN_NAME 16
>   
>   const struct snd_soc_dapm_route max_98373_dapm_routes[] = {
> @@ -168,17 +184,6 @@ static struct snd_soc_codec_conf max_98390_codec_conf[] = {
>   		.dlc = COMP_CODEC_CONF(MAX_98390_DEV1_NAME),
>   		.name_prefix = "Left",
>   	},
> -};
> -
> -static struct snd_soc_codec_conf max_98390_4spk_codec_conf[] = {
> -	{
> -		.dlc = COMP_CODEC_CONF(MAX_98390_DEV0_NAME),
> -		.name_prefix = "Right",
> -	},
> -	{
> -		.dlc = COMP_CODEC_CONF(MAX_98390_DEV1_NAME),
> -		.name_prefix = "Left",
> -	},
>   	{
>   		.dlc = COMP_CODEC_CONF(MAX_98390_DEV2_NAME),
>   		.name_prefix = "Tweeter Right",
> @@ -189,19 +194,7 @@ static struct snd_soc_codec_conf max_98390_4spk_codec_conf[] = {
>   	},
>   };
>   
> -struct snd_soc_dai_link_component max_98390_components[] = {
> -	{
> -		.name = MAX_98390_DEV0_NAME,
> -		.dai_name = MAX_98390_CODEC_DAI,
> -	},
> -	{
> -		.name = MAX_98390_DEV1_NAME,
> -		.dai_name = MAX_98390_CODEC_DAI,
> -	},
> -};
> -EXPORT_SYMBOL_NS(max_98390_components, SND_SOC_INTEL_SOF_MAXIM_COMMON);
> -
> -struct snd_soc_dai_link_component max_98390_4spk_components[] = {


max_98390_components[] and max_98390_4spk_components[] are still used

by sof_rt5682.c, we should remove them in the same patch.

Maybe combine the 2 patches together?


> +static struct snd_soc_dai_link_component max_98390_components[] = {
>   	{
>   		.name = MAX_98390_DEV0_NAME,
>   		.dai_name = MAX_98390_CODEC_DAI,
> @@ -219,62 +212,56 @@ struct snd_soc_dai_link_component max_98390_4spk_components[] = {
>   		.dai_name = MAX_98390_CODEC_DAI,
>   	},
>   };
> -EXPORT_SYMBOL_NS(max_98390_4spk_components, SND_SOC_INTEL_SOF_MAXIM_COMMON);
> +
> +static const struct {
> +	unsigned int tx;
> +	unsigned int rx;
> +} max_98390_tdm_mask[] = {
> +	{.tx = 0x01, .rx = 0x3},
> +	{.tx = 0x02, .rx = 0x3},
> +	{.tx = 0x04, .rx = 0x3},
> +	{.tx = 0x08, .rx = 0x3},
> +};
>   
>   static int max_98390_hw_params(struct snd_pcm_substream *substream,
>   			       struct snd_pcm_hw_params *params)
>   {
>   	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
>   	struct snd_soc_dai *codec_dai;
> -	int i;
> +	int i, ret = 0;
>   
>   	for_each_rtd_codec_dais(rtd, i, codec_dai) {
> -		if (i >= ARRAY_SIZE(max_98390_4spk_components)) {
> +		if (i >= ARRAY_SIZE(max_98390_tdm_mask)) {
>   			dev_err(codec_dai->dev, "invalid codec index %d\n", i);
>   			return -ENODEV;
>   		}
>   
> -		if (!strcmp(codec_dai->component->name, MAX_98390_DEV0_NAME)) {
> -			/* DEV0 tdm slot configuration Right */
> -			snd_soc_dai_set_tdm_slot(codec_dai, 0x01, 3, 4, 32);
> -		}
> -		if (!strcmp(codec_dai->component->name, MAX_98390_DEV1_NAME)) {
> -			/* DEV1 tdm slot configuration Left */
> -			snd_soc_dai_set_tdm_slot(codec_dai, 0x02, 3, 4, 32);
> -		}
> -
> -		if (!strcmp(codec_dai->component->name, MAX_98390_DEV2_NAME)) {
> -			/* DEVi2 tdm slot configuration Tweeter Right */
> -			snd_soc_dai_set_tdm_slot(codec_dai, 0x04, 3, 4, 32);
> -		}
> -		if (!strcmp(codec_dai->component->name, MAX_98390_DEV3_NAME)) {
> -			/* DEV3 tdm slot configuration Tweeter Left */
> -			snd_soc_dai_set_tdm_slot(codec_dai, 0x08, 3, 4, 32);
> +		ret = snd_soc_dai_set_tdm_slot(codec_dai, max_98390_tdm_mask[i].tx,
> +					       max_98390_tdm_mask[i].rx, 4,
> +					       params_width(params));
> +		if (ret < 0) {
> +			dev_err(codec_dai->dev, "fail to set tdm slot, ret %d\n",
> +				ret);
> +			return ret;
>   		}
>   	}
>   	return 0;
>   }
>   
> -int max_98390_spk_codec_init(struct snd_soc_pcm_runtime *rtd)
> +static int max_98390_init(struct snd_soc_pcm_runtime *rtd)
>   {
>   	struct snd_soc_card *card = rtd->card;
> +	int num_codecs = get_num_codecs(MAX_98390_ACPI_HID);
>   	int ret;
>   
> -	/* add regular speakers dapm route */
> -	ret = snd_soc_dapm_add_routes(&card->dapm, max_98390_dapm_routes,
> -				      ARRAY_SIZE(max_98390_dapm_routes));

Don't we need to add max_98390_dapm_routes for the 4 speakers case?


> -	if (ret) {
> -		dev_err(rtd->dev, "unable to add Left/Right Speaker dapm, ret %d\n", ret);
> -		return ret;
> -	}
> -
> -	/* add widgets/controls/dapm for tweeter speakers */
> -	if (acpi_dev_present("MX98390", "3", -1)) {
> +	switch (num_codecs) {
> +	case 4:
> +		/* add widgets/controls/dapm for tweeter speakers */
>   		ret = snd_soc_dapm_new_controls(&card->dapm, max_98390_tt_dapm_widgets,
>   						ARRAY_SIZE(max_98390_tt_dapm_widgets));
> -
>   		if (ret) {
> -			dev_err(rtd->dev, "unable to add tweeter dapm controls, ret %d\n", ret);
> +			dev_err(rtd->dev, "unable to add tweeter dapm widgets, ret %d\n",
> +				ret);
>   			/* Don't need to add routes if widget addition failed */
>   			return ret;
>   		}
> @@ -282,33 +269,80 @@ int max_98390_spk_codec_init(struct snd_soc_pcm_runtime *rtd)
>   		ret = snd_soc_add_card_controls(card, max_98390_tt_kcontrols,
>   						ARRAY_SIZE(max_98390_tt_kcontrols));
>   		if (ret) {
> -			dev_err(rtd->dev, "unable to add tweeter card controls, ret %d\n", ret);
> +			dev_err(rtd->dev, "unable to add tweeter controls, ret %d\n",
> +				ret);
>   			return ret;
>   		}
>   
>   		ret = snd_soc_dapm_add_routes(&card->dapm, max_98390_tt_dapm_routes,
>   					      ARRAY_SIZE(max_98390_tt_dapm_routes));
> -		if (ret)
> -			dev_err(rtd->dev,
> -				"unable to add Tweeter Left/Right Speaker dapm, ret %d\n", ret);
> +		if (ret) {
> +			dev_err(rtd->dev, "unable to add tweeter dapm routes, ret %d\n",
> +				ret);
> +			return ret;
> +		}
> +
> +		fallthrough;
> +	case 2:
> +		/* add regular speakers dapm route */
> +		ret = snd_soc_dapm_add_routes(&card->dapm, max_98390_dapm_routes,
> +					      ARRAY_SIZE(max_98390_dapm_routes));
> +		if (ret) {
> +			dev_err(rtd->dev, "unable to add dapm routes, ret %d\n",
> +				ret);
> +			return ret;
> +		}
> +		break;
> +	default:
> +		dev_err(rtd->dev, "invalid codec number %d\n", num_codecs);
> +		ret = -EINVAL;
> +		break;
>   	}
> +
>   	return ret;
>   }
> -EXPORT_SYMBOL_NS(max_98390_spk_codec_init, SND_SOC_INTEL_SOF_MAXIM_COMMON);
>   
> -const struct snd_soc_ops max_98390_ops = {
> +static const struct snd_soc_ops max_98390_ops = {
>   	.hw_params = max_98390_hw_params,
>   };
> -EXPORT_SYMBOL_NS(max_98390_ops, SND_SOC_INTEL_SOF_MAXIM_COMMON);
>   
> -void max_98390_set_codec_conf(struct snd_soc_card *card, int ch)
> +void max_98390_dai_link(struct snd_soc_dai_link *link)
> +{
> +	int num_codecs = get_num_codecs(MAX_98390_ACPI_HID);
> +
> +	link->codecs = max_98390_components;
> +
> +	switch (num_codecs) {
> +	case 2:
> +	case 4:
> +		link->num_codecs = num_codecs;
> +		break;
> +	default:
> +		pr_err("invalid codec number %d for %s\n", num_codecs,
> +			MAX_98390_ACPI_HID);
> +		break;
> +	}
> +
> +	link->init = max_98390_init;
> +	link->ops = &max_98390_ops;
> +}
> +EXPORT_SYMBOL_NS(max_98390_dai_link, SND_SOC_INTEL_SOF_MAXIM_COMMON);
> +
> +void max_98390_set_codec_conf(struct snd_soc_card *card)
>   {
> -	if (ch == ARRAY_SIZE(max_98390_4spk_codec_conf)) {
> -		card->codec_conf = max_98390_4spk_codec_conf;
> -		card->num_configs = ARRAY_SIZE(max_98390_4spk_codec_conf);
> -	} else {
> -		card->codec_conf = max_98390_codec_conf;
> -		card->num_configs = ARRAY_SIZE(max_98390_codec_conf);
> +	int num_codecs = get_num_codecs(MAX_98390_ACPI_HID);
> +
> +	card->codec_conf = max_98390_codec_conf;
> +
> +	switch (num_codecs) {
> +	case 2:
> +	case 4:
> +		card->num_configs = num_codecs;
> +		break;
> +	default:
> +		pr_err("invalid codec number %d for %s\n", num_codecs,
> +			MAX_98390_ACPI_HID);
> +		break;
>   	}
>   }
>   EXPORT_SYMBOL_NS(max_98390_set_codec_conf, SND_SOC_INTEL_SOF_MAXIM_COMMON);
> diff --git a/sound/soc/intel/boards/sof_maxim_common.h b/sound/soc/intel/boards/sof_maxim_common.h
> index 7a8c53049e4d..a3676d68cc12 100644
> --- a/sound/soc/intel/boards/sof_maxim_common.h
> +++ b/sound/soc/intel/boards/sof_maxim_common.h
> @@ -27,18 +27,15 @@ int max_98373_trigger(struct snd_pcm_substream *substream, int cmd);
>   /*
>    * Maxim MAX98390
>    */
> -#define MAX_98390_CODEC_DAI     "max98390-aif1"
> -#define MAX_98390_DEV0_NAME     "i2c-MX98390:00"
> -#define MAX_98390_DEV1_NAME     "i2c-MX98390:01"
> -#define MAX_98390_DEV2_NAME     "i2c-MX98390:02"
> -#define MAX_98390_DEV3_NAME     "i2c-MX98390:03"
> -
> -extern struct snd_soc_dai_link_component max_98390_components[2];
> -extern struct snd_soc_dai_link_component max_98390_4spk_components[4];
> -extern const struct snd_soc_ops max_98390_ops;
> -
> -void max_98390_set_codec_conf(struct snd_soc_card *card, int ch);
> -int max_98390_spk_codec_init(struct snd_soc_pcm_runtime *rtd);
> +#define MAX_98390_ACPI_HID	"MX98390"
> +#define MAX_98390_CODEC_DAI	"max98390-aif1"
> +#define MAX_98390_DEV0_NAME	"i2c-MX98390:00"
> +#define MAX_98390_DEV1_NAME	"i2c-MX98390:01"
> +#define MAX_98390_DEV2_NAME	"i2c-MX98390:02"
> +#define MAX_98390_DEV3_NAME	"i2c-MX98390:03"
> +
> +void max_98390_dai_link(struct snd_soc_dai_link *link);
> +void max_98390_set_codec_conf(struct snd_soc_card *card);
>   
>   /*
>    * Maxim MAX98357A/MAX98360A

Re: [PATCH 1/2] ASoC: Intel: maxim-common: get codec number from ACPI

[Liao, Bard]

On 7/26/2023 4:47 PM, Liao, Bard wrote:
> ;
>>   -    /* add regular speakers dapm route */
>> -    ret = snd_soc_dapm_add_routes(&card->dapm, max_98390_dapm_routes,
>> -                      ARRAY_SIZE(max_98390_dapm_routes));
>
> Don't we need to add max_98390_dapm_routes for the 4 speakers case?
>
Please ignore this comment. I didn't notice that it is a fallthrough not

break in the end of case 4.

RE: [PATCH 1/2] ASoC: Intel: maxim-common: get codec number from ACPI

[Lu, Brent]

> 
> 
> max_98390_components[] and max_98390_4spk_components[] are still used
> 
> by sof_rt5682.c, we should remove them in the same patch.
> 
> Maybe combine the 2 patches together?
> 
> 

I've got your point but these two patches are doing two things: one is refactor the
code and add a detection feature, the other one is removing the quirk. Not sure if
it's proper to merge them.

Regards,
Brent

Re: [PATCH 1/2] ASoC: Intel: maxim-common: get codec number from ACPI

[Liao, Bard]

On 7/27/2023 10:14 AM, Lu, Brent wrote:
>>
>> max_98390_components[] and max_98390_4spk_components[] are still used
>>
>> by sof_rt5682.c, we should remove them in the same patch.
>>
>> Maybe combine the 2 patches together?
>>
>>
> I've got your point but these two patches are doing two things: one is refactor the
> code and add a detection feature, the other one is removing the quirk. Not sure if
> it's proper to merge them.

The point is that if you remove them and they are still used somewhere,

you will break the build. i.e. Kernel will not compile if we apply the

first patch only.


> Regards,
> Brent

Re: [PATCH 1/2] ASoC: Intel: maxim-common: get codec number from ACPI

[Pierre-Louis Bossart]

On 7/27/23 5:21 AM, Liao, Bard wrote:
> 
> On 7/27/2023 10:14 AM, Lu, Brent wrote:
>>>
>>> max_98390_components[] and max_98390_4spk_components[] are still used
>>>
>>> by sof_rt5682.c, we should remove them in the same patch.
>>>
>>> Maybe combine the 2 patches together?
>>>
>>>
>> I've got your point but these two patches are doing two things: one is 
>> refactor the
>> code and add a detection feature, the other one is removing the quirk. 
>> Not sure if
>> it's proper to merge them.
> 
> The point is that if you remove them and they are still used somewhere,
> 
> you will break the build. i.e. Kernel will not compile if we apply the
> 
> first patch only.

IOW git bisect is broken and that's a big no-no.

Re: [PATCH 1/2] ASoC: Intel: maxim-common: get codec number from ACPI

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 405 bytes --]

On Thu, Jul 27, 2023 at 07:41:53AM +0200, Pierre-Louis Bossart wrote:
> On 7/27/23 5:21 AM, Liao, Bard wrote:

> > The point is that if you remove them and they are still used somewhere,
> > 
> > you will break the build. i.e. Kernel will not compile if we apply the
> > 
> > first patch only.

> IOW git bisect is broken and that's a big no-no.

Yes, I build test every patch so I'll notice.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH 1/2] ASoC: Intel: maxim-common: get codec number from ACPI

[Lu, Brent]

> 
> The point is that if you remove them and they are still used somewhere,
> 
> you will break the build. i.e. Kernel will not compile if we apply the
> 
> first patch only.
> 
> 

Thanks for pointing this out. I will merge these two patches to avoid build
break in v4 patch.


Regards,
Brent